[Flow Exporter] Fix deny connections tracking for ANP baseline tier #2542

zyiou · 2021-08-05T17:41:58Z

If we apply a baseline ANP, in deny connections tracking of Flow
Exporter, it will be filled as K8s NetworkPolicy instead of ANP
since we are ignoring ANP associate tables IngressDefaultTable
and EgressDefaultTable for these connections.
This PR fixes it by changing the logic for assigning policy type.

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

codecov-commenter · 2021-08-05T17:54:06Z

Codecov Report

Merging #2542 (2d50641) into main (9ea4e86) will decrease coverage by 5.59%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2542      +/-   ##
==========================================
- Coverage   58.82%   53.23%   -5.60%     
==========================================
  Files         281      281              
  Lines       22271    22268       -3     
==========================================
- Hits        13101    11854    -1247     
- Misses       7796     9132    +1336     
+ Partials     1374     1282      -92

Flag	Coverage Δ
kind-e2e-tests	`35.13% <0.00%> (-10.17%)`	⬇️
unit-tests	`42.23% <0.00%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/agent/controller/networkpolicy/packetin.go	`4.43% <0.00%> (-68.39%)`	⬇️
pkg/apis/controlplane/v1beta2/helper.go	`25.00% <0.00%> (-75.00%)`	⬇️
...formers/externalversions/security/v1alpha1/tier.go	`0.00% <0.00%> (-64.29%)`	⬇️
...ers/externalversions/core/v1alpha2/clustergroup.go	`0.00% <0.00%> (-64.29%)`	⬇️
...s/externalversions/core/v1alpha2/externalentity.go	`0.00% <0.00%> (-64.29%)`	⬇️
...xternalversions/security/v1alpha1/networkpolicy.go	`0.00% <0.00%> (-64.29%)`	⬇️
...versions/security/v1alpha1/clusternetworkpolicy.go	`0.00% <0.00%> (-64.29%)`	⬇️
pkg/controller/networkpolicy/mutate.go	`0.00% <0.00%> (-62.07%)`	⬇️
.../agent/apiserver/handlers/networkpolicy/handler.go	`0.00% <0.00%> (-58.34%)`	⬇️
... and 74 more

pkg/agent/controller/networkpolicy/packetin.go

Dyanngg · 2021-08-11T04:19:06Z

pkg/agent/controller/networkpolicy/packetin.go

-				denyConn.EgressNetworkPolicyRuleAction = flowexporter.RuleActionToUint8(disposition)
-			}
+	} else { // Get name and namespace for Antrea Network Policy or Antrea Cluster Network Policy
+		if tableID == openflow.AntreaPolicyIngressRuleTable {


Why are cases 1,3 / 2,4 not grouped? i.e.
if tableID == openflow.AntreaPolicyIngressRuleTable || tableID == tableID == openflow.IngressDefaultTable
unless I'm missing something obvious....

You are right, these can be combined. Thanks!

Dyanngg · 2021-08-11T04:23:35Z

pkg/agent/controller/networkpolicy/packetin.go

+			denyConn.EgressNetworkPolicyType = flowexporter.PolicyTypeToUint8(policy.Type)
+			denyConn.EgressNetworkPolicyRuleName = rule.Name
+			denyConn.EgressNetworkPolicyRuleAction = flowexporter.RuleActionToUint8(disposition)
+		} else if tableID == openflow.IngressDefaultTable {


Also if we don't want to refer to ad-hoc table names here, there are actually exported functions GetAntreaPolicyEgressTables() and GetAntreaPolicyIngressTables() in pipeline.go that gives the list of tables ACNP/ANP uses for ingress/egress. Although in this particular case it seems an overkill, since we would need to check tableID for list membership to determine the direction of conn. However, it is more future-proof, in the sense that in case ACNP/ANP uses additional tables in the future, this approach saves additional changes in this file and potential bugs.

srikartati · 2021-08-11T21:53:41Z

pkg/agent/controller/networkpolicy/packetin.go

-	} else if tableID == openflow.EgressDefaultTable {
-		denyConn.EgressNetworkPolicyType = registry.PolicyTypeK8sNetworkPolicy
-		denyConn.EgressNetworkPolicyRuleAction = flowexporter.RuleActionToUint8(disposition)
-	} else { // Get name and namespace for Antrea Network Policy or Antrea Cluster Network Policy


Bug seems to be we are ignoring the ANP associated tables.

If so, please revise this description. Connection is not inside the specified table, but they are just associated with those tables.

it will be filled as K8s NetworkPolicy instead of ANP
since the connection will be in IngressDefaultTable/EgressDefaultTable.

Updated. Thanks!

antoninbas · 2021-08-12T00:54:56Z

pkg/agent/controller/networkpolicy/packetin.go

+		for _, table := range openflow.GetAntreaPolicyIngressTables() {
+			if table == tableID {
 				denyConn.IngressNetworkPolicyName = policy.Name
 				denyConn.IngressNetworkPolicyNamespace = policy.Namespace
 				denyConn.IngressNetworkPolicyType = flowexporter.PolicyTypeToUint8(policy.Type)
 				denyConn.IngressNetworkPolicyRuleName = rule.Name
 				denyConn.IngressNetworkPolicyRuleAction = flowexporter.RuleActionToUint8(disposition)
-			} else if tableID == openflow.AntreaPolicyEgressRuleTable {
+			}
+		}
+		for _, table := range openflow.GetAntreaPolicyEgressTables() {
+			if table == tableID {


I think it would be nice to define some local helper closure functions isAntreaPolicyIngressTables and isAntreaPolicyEgressTables

srikartati

LGTM

srikartati · 2021-08-13T16:11:59Z

/test-all

zyiou · 2021-08-18T19:07:40Z

/test-all

dreamtalen

LGTM

zyiou · 2021-08-19T17:36:15Z

/test-e2e

zyiou · 2021-08-25T00:35:55Z

/test-e2e
/test-ipv6-e2e
/test-ipv6-only-e2e

srikartati

LGTM

srikartati · 2021-08-26T18:48:48Z

/test-ipv6-e2e /test-ipv6-only-e2e

antoninbas · 2021-08-26T19:26:57Z

pkg/agent/controller/networkpolicy/packetin.go

+		}
+		if isAntreaPolicyEgressTable(tableID) {


nit: else if to avoid the duplicate check for ingress policy tables

Done. Thanks!

If we apply a baseline ANP, in deny connections tracking of Flow Exporter, it will be filled as K8s NetworkPolicy instead of ANP since we are ignoring ANP associate tables IngressDefaultTable and EgressDefaultTable for these connections. This PR fixes it by changing the logic for assigning policy type. Signed-off-by: zyiou <zyiou@vmware.com>

zyiou · 2021-08-26T20:44:59Z

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

zyiou force-pushed the zyiou/fix_deny_connections branch from 4e3808f to 3ab3c06 Compare August 5, 2021 17:42

zyiou force-pushed the zyiou/fix_deny_connections branch 3 times, most recently from dfa16d4 to 499d9c0 Compare August 5, 2021 20:24

zyiou marked this pull request as ready for review August 10, 2021 23:22

zyiou requested review from dreamtalen, Dyanngg and srikartati August 10, 2021 23:22

Dyanngg reviewed Aug 11, 2021

View reviewed changes

srikartati reviewed Aug 11, 2021

View reviewed changes

zyiou self-assigned this Aug 11, 2021

zyiou added area/flow-visibility Issues or PRs related to flow visibility support in Antrea area/flow-visibility/exporter Issues or PRs related to the Flow Exporter functions in the Agent labels Aug 11, 2021

zyiou force-pushed the zyiou/fix_deny_connections branch 2 times, most recently from daf7ecf to f349100 Compare August 12, 2021 00:26

antoninbas previously approved these changes Aug 12, 2021

View reviewed changes

zyiou dismissed antoninbas’s stale review via 5661170 August 12, 2021 05:25

zyiou force-pushed the zyiou/fix_deny_connections branch from f349100 to 5661170 Compare August 12, 2021 05:25

srikartati requested a review from Dyanngg August 12, 2021 22:22

srikartati previously approved these changes Aug 12, 2021

View reviewed changes

zyiou dismissed srikartati’s stale review via 4b62df4 August 13, 2021 04:07

zyiou force-pushed the zyiou/fix_deny_connections branch from 5661170 to 4b62df4 Compare August 13, 2021 04:07

zyiou force-pushed the zyiou/fix_deny_connections branch 2 times, most recently from fae2d14 to 2c64a21 Compare August 18, 2021 05:17

dreamtalen previously approved these changes Aug 18, 2021

View reviewed changes

zyiou requested review from antoninbas and srikartati August 18, 2021 21:05

srikartati previously approved these changes Aug 26, 2021

View reviewed changes

srikartati added this to the Antrea v1.3 release milestone Aug 26, 2021

antoninbas previously approved these changes Aug 26, 2021

View reviewed changes

zyiou dismissed stale reviews from antoninbas, srikartati, and dreamtalen via cdc4a3d August 26, 2021 20:42

zyiou force-pushed the zyiou/fix_deny_connections branch from 2c64a21 to cdc4a3d Compare August 26, 2021 20:42

antoninbas approved these changes Aug 26, 2021

View reviewed changes

antoninbas merged commit 55ec2ad into antrea-io:main Aug 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Flow Exporter] Fix deny connections tracking for ANP baseline tier #2542

[Flow Exporter] Fix deny connections tracking for ANP baseline tier #2542

zyiou commented Aug 5, 2021 •

edited

Loading

codecov-commenter commented Aug 5, 2021 •

edited

Loading

Dyanngg Aug 11, 2021 •

edited

Loading

zyiou Aug 12, 2021

Dyanngg Aug 11, 2021 •

edited

Loading

srikartati Aug 11, 2021

zyiou Aug 12, 2021

antoninbas Aug 12, 2021

srikartati left a comment

srikartati commented Aug 13, 2021

zyiou commented Aug 18, 2021

dreamtalen left a comment

zyiou commented Aug 19, 2021

zyiou commented Aug 25, 2021

srikartati left a comment

srikartati commented Aug 26, 2021

antoninbas Aug 26, 2021

zyiou Aug 26, 2021

zyiou commented Aug 26, 2021

[Flow Exporter] Fix deny connections tracking for ANP baseline tier #2542

[Flow Exporter] Fix deny connections tracking for ANP baseline tier #2542

Conversation

zyiou commented Aug 5, 2021 • edited Loading

codecov-commenter commented Aug 5, 2021 • edited Loading

Codecov Report

Dyanngg Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

zyiou Aug 12, 2021

Choose a reason for hiding this comment

Dyanngg Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

srikartati Aug 11, 2021

Choose a reason for hiding this comment

zyiou Aug 12, 2021

Choose a reason for hiding this comment

antoninbas Aug 12, 2021

Choose a reason for hiding this comment

srikartati left a comment

Choose a reason for hiding this comment

srikartati commented Aug 13, 2021

zyiou commented Aug 18, 2021

dreamtalen left a comment

Choose a reason for hiding this comment

zyiou commented Aug 19, 2021

zyiou commented Aug 25, 2021

srikartati left a comment

Choose a reason for hiding this comment

srikartati commented Aug 26, 2021

antoninbas Aug 26, 2021

Choose a reason for hiding this comment

zyiou Aug 26, 2021

Choose a reason for hiding this comment

zyiou commented Aug 26, 2021

zyiou commented Aug 5, 2021 •

edited

Loading

codecov-commenter commented Aug 5, 2021 •

edited

Loading

Dyanngg Aug 11, 2021 •

edited

Loading

Dyanngg Aug 11, 2021 •

edited

Loading