Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix panic bug in Antrea Policy #2730

Merged
merged 1 commit into from Sep 10, 2021
Merged

Conversation

wenyingd
Copy link
Contributor

@wenyingd wenyingd commented Sep 8, 2021

Check if the clause pointer is nil before looping the matches.
Fixes #2729

Signed-off-by: wenyingd wenyingd@vmware.com

@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2021

Codecov Report

Merging #2730 (de43b1e) into main (dded211) will increase coverage by 6.32%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2730      +/-   ##
==========================================
+ Coverage   59.50%   65.82%   +6.32%     
==========================================
  Files         285      285              
  Lines       23020    26389    +3369     
==========================================
+ Hits        13697    17371    +3674     
+ Misses       7865     7394     -471     
- Partials     1458     1624     +166     
Flag Coverage Δ
e2e-tests 56.73% <0.00%> (?)
kind-e2e-tests 48.38% <0.00%> (+1.63%) ⬆️
unit-tests 41.33% <70.00%> (+0.28%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/agent/openflow/network_policy.go 82.42% <50.00%> (+6.84%) ⬆️
pkg/controller/egress/ipallocator/allocator.go 65.00% <0.00%> (-15.42%) ⬇️
pkg/controller/networkpolicy/endpoint_querier.go 77.64% <0.00%> (-13.79%) ⬇️
pkg/agent/flowexporter/flowrecords/flow_records.go 69.73% <0.00%> (-11.81%) ⬇️
pkg/legacyapis/core/v1alpha2/register.go 69.23% <0.00%> (-10.77%) ⬇️
pkg/apis/stats/register.go 71.42% <0.00%> (-10.39%) ⬇️
pkg/legacyapis/stats/register.go 71.42% <0.00%> (-10.39%) ⬇️
pkg/ovs/openflow/ofctrl_meter.go 33.84% <0.00%> (-10.16%) ⬇️
pkg/legacyapis/security/v1alpha1/register.go 73.33% <0.00%> (-10.00%) ⬇️
.../registry/networkpolicy/clustergroupmember/rest.go 78.26% <0.00%> (-9.98%) ⬇️
... and 270 more

@wenyingd
Copy link
Contributor Author

wenyingd commented Sep 8, 2021

/test-all
/test-ipv6-all
/test-ipv6-only-all

abhiraut
abhiraut previously approved these changes Sep 8, 2021
Copy link
Contributor

@abhiraut abhiraut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nits

pkg/agent/openflow/network_policy.go Show resolved Hide resolved
@@ -1511,6 +1514,9 @@ func (c *client) updateConjunctionActionFlows(conj *policyRuleConjunction, updat
func (c *client) updateConjunctionMatchFlows(conj *policyRuleConjunction, newPriority uint16) {
allClause := []*clause{conj.fromClause, conj.toClause, conj.serviceClause}
for _, clause := range allClause {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not introduced by your change; but the var clause is used for the struct and for the looping var

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

qiyueyao
qiyueyao previously approved these changes Sep 8, 2021
Copy link
Contributor

@qiyueyao qiyueyao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, not related just a quick question do we need to check nil before getting flow priority?

func (c *client) GetPolicyInfoFromConjunction(ruleID uint32) (string, string) {
	conjunction := c.getPolicyRuleConjunction(ruleID)
	priorities := conjunction.ActionFlowPriorities()
	if conjunction == nil || len(priorities) == 0 {
		return "", ""
	}
	return conjunction.npRef.ToString(), priorities[0]
}

Thanks!

GraysonWu
GraysonWu previously approved these changes Sep 9, 2021
Copy link
Contributor

@GraysonWu GraysonWu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just same nits as Abhishek.

@wenyingd
Copy link
Contributor Author

wenyingd commented Sep 9, 2021

LGTM, not related just a quick question do we need to check nil before getting flow priority?

func (c *client) GetPolicyInfoFromConjunction(ruleID uint32) (string, string) {
	conjunction := c.getPolicyRuleConjunction(ruleID)
	priorities := conjunction.ActionFlowPriorities()
	if conjunction == nil || len(priorities) == 0 {
		return "", ""
	}
	return conjunction.npRef.ToString(), priorities[0]
}

Thanks!

Greate catch, I would change it together.

@wenyingd
Copy link
Contributor Author

wenyingd commented Sep 9, 2021

/test-all
/test-ipv6-all
/test-ipv6-only-all

tnqn
tnqn previously approved these changes Sep 9, 2021
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, typo in commit message: nill

abhiraut
abhiraut previously approved these changes Sep 9, 2021
qiyueyao
qiyueyao previously approved these changes Sep 9, 2021
Check if the clause pointer is nil before looping the matches.

Signed-off-by: wenyingd <wenyingd@vmware.com>
@wenyingd
Copy link
Contributor Author

/test-all
/test-ipv6-all
/test-ipv6-only-all

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tnqn tnqn merged commit 2e9055e into antrea-io:main Sep 10, 2021
@wenyingd
Copy link
Contributor Author

@tnqn Shall I backport this PR to original releases?

@antoninbas
Copy link
Contributor

@wenyingd please backport to any release which has this bug, starting with v0.13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/backport Indicates a PR that requires backports.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nil pointer dereference in network_policy
7 participants