Fix dumping OVS flows of a NetworkPolicy #3335

jainpulkit22 · 2022-02-21T11:46:18Z

This PR fixes the nil pointer dereference error while dumping the ovsflows of NetworkPolicy, by adding a nil check on the NetworkPolicyReference attribute of the policyRuleConjunction Object

Signed-off-by: Pulkit Jain jainpu@vmware.com

codecov-commenter · 2022-02-21T12:56:31Z

Codecov Report

Merging #3335 (687dfb0) into main (0ffcfc9) will decrease coverage by 8.48%.
The diff coverage is 41.09%.

❗ Current head 687dfb0 differs from pull request most recent head b6389fb. Consider uploading reports for the commit b6389fb to get more accurate results

@@            Coverage Diff             @@
##             main    #3335      +/-   ##
==========================================
- Coverage   62.13%   53.65%   -8.49%     
==========================================
  Files         266      239      -27     
  Lines       26519    34037    +7518     
==========================================
+ Hits        16477    18261    +1784     
- Misses       8222    13996    +5774     
+ Partials     1820     1780      -40

Flag	Coverage Δ
e2e-tests	`53.65% <41.09%> (?)`
kind-e2e-tests	`?`
unit-tests	`?`

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

Impacted Files	Coverage Δ
pkg/agent/controller/networkpolicy/packetin.go	`68.34% <ø> (+1.19%)`	⬆️
pkg/agent/ipassigner/responder/ndp_responder.go	`0.00% <0.00%> (ø)`
pkg/agent/openflow/network_policy.go	`64.38% <0.00%> (-18.87%)`	⬇️
pkg/agent/openflow/pipeline_other.go	`3.44% <0.00%> (+1.27%)`	⬆️
pkg/agent/openflow/pipeline.go	`66.37% <16.66%> (-7.43%)`	⬇️
pkg/agent/proxy/proxier.go	`49.90% <16.66%> (-13.24%)`	⬇️
pkg/agent/openflow/client.go	`59.77% <28.57%> (+5.74%)`	⬆️
pkg/controller/networkpolicy/validate.go	`62.94% <33.33%> (+21.75%)`	⬆️
pkg/agent/ipassigner/ip_assigner_linux.go	`52.55% <43.83%> (-7.72%)`	⬇️
pkg/agent/ipassigner/responder/arp_responder.go	`63.52% <63.52%> (ø)`
... and 270 more

antoninbas

It's good to add a test when fixing a bug like this one.

tnqn

The title of the PR/commit should mention it's fixing it, i.e. "Fix dumping OVS flows of a NetworkPolicy"

tnqn · 2022-02-23T02:42:20Z

pkg/agent/openflow/network_policy.go

@@ -1455,6 +1455,11 @@ func (c *client) GetNetworkPolicyFlowKeys(npName, npNamespace string) []string {

 	for _, conjObj := range c.policyCache.List() {
 		conj := conjObj.(*policyRuleConjunction)
+		// If the NetworkPolicyReference in the policyRuleConjunction is nil then that entry in client's
+		// policyCache should be ignored because here we need to dump flows of NetworkPolicy.
+		if conj.npRef == nil {


Better to explain why the reference could be nil in the comment of this field in the struct.

tnqn

Could you squash the two commits to one?

tnqn · 2022-02-24T06:36:20Z

pkg/agent/openflow/network_policy.go

@@ -525,7 +525,8 @@ type policyRuleConjunction struct {
 	serviceClause *clause
 	actionFlows   []binding.Flow
 	metricFlows   []binding.Flow
-	// NetworkPolicy reference information for debugging usage.
+	// NetworkPolicy reference information for debugging usage, its' value can be nil


Suggested change

// NetworkPolicy reference information for debugging usage, its' value can be nil

// NetworkPolicy reference information for debugging usage, its value can be nil

Suggested change

// NetworkPolicy reference information for debugging usage, its' value can be nil

// NetworkPolicy reference information for debugging usage, its' value can be nil

tnqn · 2022-02-24T06:41:34Z

pkg/agent/openflow/network_policy.go

@@ -525,7 +525,8 @@ type policyRuleConjunction struct {
 	serviceClause *clause
 	actionFlows   []binding.Flow
 	metricFlows   []binding.Flow
-	// NetworkPolicy reference information for debugging usage.
+	// NetworkPolicy reference information for debugging usage, its' value can be nil
+	// when a new DNS Conjunction is added to the policyCache.


Suggested change

// when a new DNS Conjunction is added to the policyCache.

// for conjunctions that are not built for a specific NetworkPolicy, e.g. DNS packetin Conjunction.

tnqn · 2022-02-24T06:47:31Z

pkg/agent/openflow/network_policy_test.go

+	// dnsID will store the ID for New DNS packet, that is created,
+	// to test for nil NetworkPolicyReference.
+	dnsID := uint32(107)
+	c.NewDNSpacketInConjunction(dnsID)


Better to call this function in the begining of the test to be similar with the actual code. It should check error is nil to ensure it takes effect.

tnqn · 2022-02-24T06:53:16Z

pkg/agent/openflow/network_policy_test.go

@@ -222,6 +222,10 @@ func TestInstallPolicyRuleFlows(t *testing.T) {
 	assert.Equal(t, 3, getChangedFlowOPCount(matchFlows2, insertion))
 	err = c.applyConjunctiveMatchFlows(ctxChanges2)
 	require.Nil(t, err)
+	// dnsID will store the ID for New DNS packet, that is created,


The comment doesn't reflect the real meaning of the ID, which is the OVS rule ID of the DNS response intercept rule. Here I would suggest to just pass 1 as the ID and comment:

Create a policyRuleConjunction for the dns response interception flows to ensure nil NetworkPolicyReference is handled correctly by GetNetworkPolicyFlowKeys.

tnqn

LGTM, thanks

antoninbas · 2022-02-24T18:23:46Z

pkg/agent/openflow/network_policy_test.go

+	err := c.NewDNSpacketInConjunction(dnsID)
+	require.Nil(t, err)


require.NoError(t, c.NewDNSpacketInConjunction(dnsID))

Thank you for the suggestion.

tnqn

LGTM

tnqn · 2022-02-25T09:10:40Z

/test-conformance
/test-e2e
/test-networkpolicy

tnqn · 2022-02-25T10:22:53Z

===> Installing Golangci-lint <===
golangci/golangci-lint info checking GitHub for tag 'v1.41.1'
golangci/golangci-lint info found version: 1.41.1 for v1.41.1/linux/amd64
golangci/golangci-lint info installed .golangci-bin/golangci-lint
===> Running golangci (linux) <===
pkg/agent/openflow/network_policy_test.go:162: File is not `gofmt`-ed with `-s` (gofmt)
	// Create a policyRuleConjunction for the dns response interception flows 
make: *** [Makefile:252: golangci] Error 1
Error: Process completed with exit code 2.

@jainpulkit22 golangci-lint check failed because the comment is not gofmted.

Fixes antrea-io#3306. This commit fixes the nil pointer dereference error while dumping ovsflows of NetworkPolicy, and adds a test to validate the changes. Signed-off-by: Pulkit Jain <jainpu@vmware.com>

tnqn · 2022-02-25T11:32:31Z

/test-all

tnqn · 2022-02-25T14:49:49Z

/test-integration

Fixes antrea-io#3306. This commit fixes the nil pointer dereference error while dumping ovsflows of NetworkPolicy, and adds a test to validate the changes. Signed-off-by: Pulkit Jain <jainpu@vmware.com>

antoninbas reviewed Feb 22, 2022

View reviewed changes

tnqn reviewed Feb 23, 2022

View reviewed changes

tnqn added the action/release-note Indicates a PR that should be included in release notes. label Feb 23, 2022

jainpulkit22 force-pushed the networkPolicy-flowDump-fix branch from 7a0ceba to c3dacc6 Compare February 23, 2022 10:31

jainpulkit22 changed the title ~~Dumping OVS flows of a NetworkPolicy~~ Fix dumping OVS flows of a NetworkPolicy Feb 23, 2022

jainpulkit22 force-pushed the networkPolicy-flowDump-fix branch from c3dacc6 to cddadf1 Compare February 23, 2022 11:01

tnqn reviewed Feb 24, 2022

View reviewed changes

jainpulkit22 force-pushed the networkPolicy-flowDump-fix branch 2 times, most recently from f4791e5 to a6c1efa Compare February 24, 2022 07:56

tnqn previously approved these changes Feb 24, 2022

View reviewed changes

tnqn requested a review from antoninbas February 24, 2022 08:34

antoninbas previously approved these changes Feb 24, 2022

View reviewed changes

jainpulkit22 dismissed stale reviews from antoninbas and tnqn via 9a46a9a February 25, 2022 08:32

jainpulkit22 force-pushed the networkPolicy-flowDump-fix branch from a6c1efa to 9a46a9a Compare February 25, 2022 08:32

tnqn previously approved these changes Feb 25, 2022

View reviewed changes

Fix dumping OVS flows of a NetworkPolicy

b6389fb

Fixes antrea-io#3306. This commit fixes the nil pointer dereference error while dumping ovsflows of NetworkPolicy, and adds a test to validate the changes. Signed-off-by: Pulkit Jain <jainpu@vmware.com>

jainpulkit22 dismissed tnqn’s stale review via b6389fb February 25, 2022 10:30

jainpulkit22 force-pushed the networkPolicy-flowDump-fix branch from 9a46a9a to b6389fb Compare February 25, 2022 10:30

tnqn approved these changes Feb 25, 2022

View reviewed changes

tnqn merged commit 7692426 into antrea-io:main Feb 25, 2022

tnqn mentioned this pull request Mar 2, 2022

Antrea antctl get ovsflows response INTERNAL_ERROR #2944

Closed

jainpulkit22 deleted the networkPolicy-flowDump-fix branch March 21, 2022 05:45

jainpulkit22 restored the networkPolicy-flowDump-fix branch March 21, 2022 05:45

jainpulkit22 deleted the networkPolicy-flowDump-fix branch March 21, 2022 05:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dumping OVS flows of a NetworkPolicy #3335

Fix dumping OVS flows of a NetworkPolicy #3335

jainpulkit22 commented Feb 21, 2022

codecov-commenter commented Feb 21, 2022 •

edited

antoninbas left a comment

tnqn left a comment

tnqn Feb 23, 2022

tnqn left a comment

tnqn Feb 24, 2022

tnqn Feb 24, 2022

tnqn Feb 24, 2022

tnqn Feb 24, 2022

tnqn left a comment

antoninbas Feb 24, 2022

jainpulkit22 Feb 25, 2022

tnqn left a comment

tnqn commented Feb 25, 2022

tnqn commented Feb 25, 2022

tnqn commented Feb 25, 2022

tnqn commented Feb 25, 2022

	// NetworkPolicy reference information for debugging usage, its' value can be nil
	// NetworkPolicy reference information for debugging usage, its value can be nil

	// when a new DNS Conjunction is added to the policyCache.
	// for conjunctions that are not built for a specific NetworkPolicy, e.g. DNS packetin Conjunction.

		err := c.NewDNSpacketInConjunction(dnsID)
		require.Nil(t, err)

Fix dumping OVS flows of a NetworkPolicy #3335

Fix dumping OVS flows of a NetworkPolicy #3335

Conversation

jainpulkit22 commented Feb 21, 2022

codecov-commenter commented Feb 21, 2022 • edited

Codecov Report

antoninbas left a comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn Feb 23, 2022

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn Feb 24, 2022

Choose a reason for hiding this comment

tnqn Feb 24, 2022

Choose a reason for hiding this comment

tnqn Feb 24, 2022

Choose a reason for hiding this comment

tnqn Feb 24, 2022

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

antoninbas Feb 24, 2022

Choose a reason for hiding this comment

jainpulkit22 Feb 25, 2022

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Feb 25, 2022

tnqn commented Feb 25, 2022

tnqn commented Feb 25, 2022

tnqn commented Feb 25, 2022

codecov-commenter commented Feb 21, 2022 •

edited