-
Notifications
You must be signed in to change notification settings - Fork 366
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
[Flow Exporter] Fix deny connections tracking for ANP baseline tier #2542
Conversation
4e3808f
to
3ab3c06
Compare
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
dfa16d4
to
499d9c0
Compare
denyConn.EgressNetworkPolicyRuleAction = flowexporter.RuleActionToUint8(disposition) | ||
} | ||
} else { // Get name and namespace for Antrea Network Policy or Antrea Cluster Network Policy | ||
if tableID == openflow.AntreaPolicyIngressRuleTable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, these can be combined. Thanks!
denyConn.EgressNetworkPolicyType = flowexporter.PolicyTypeToUint8(policy.Type) | ||
denyConn.EgressNetworkPolicyRuleName = rule.Name | ||
denyConn.EgressNetworkPolicyRuleAction = flowexporter.RuleActionToUint8(disposition) | ||
} else if tableID == openflow.IngressDefaultTable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. Thanks!
daf7ecf
to
f349100
Compare
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to define some local helper closure functions isAntreaPolicyIngressTables
and isAntreaPolicyEgressTables
f349100
to
5661170
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
5661170
to
4b62df4
Compare
/test-all |
fae2d14
to
2c64a21
Compare
/test-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/test-e2e |
/test-e2e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/test-ipv6-e2e /test-ipv6-only-e2e |
} | ||
if isAntreaPolicyEgressTable(tableID) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: else if
to avoid the duplicate check for ingress policy tables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
cdc4a3d
2c64a21
to
cdc4a3d
Compare
/test-all |
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