-
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
Enhance ACNP Service related feature #4261
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4261 +/- ##
==========================================
+ Coverage 62.98% 63.23% +0.25%
==========================================
Files 388 388
Lines 55129 55155 +26
==========================================
+ Hits 34721 34877 +156
+ Misses 17841 17704 -137
- Partials 2567 2574 +7
|
bdf6c87
to
7be1bb8
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.
Code change LGTM, please pay attention to the test coverage.
test/e2e/antreapolicy_test.go
Outdated
ip netns exec %[1]s ip route replace default via %[3]s && \ | ||
sleep 3600 | ||
`, testNetns, "1.1.1.1", "1.1.1.254", 24) | ||
if err := NewPodBuilder(clientName, data.testNamespace, agnhostImage).OnNode(controlPlaneNodeName()).WithCommand([]string{"sh", "-c", cmd}).InHostNetwork().Privileged().Create(data); err != nil { |
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.
To fake an external client, command should be run in the netns created in the test agnhost container, not the agnhost container itself. For example: ip netns exec test-ns curl 192.168.77.100:30001 (assuming that 192.168.77.100:30001
is a NodePort url).
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 except the test issue @hongliangl has pointed out.
265627b
to
c30d222
Compare
In E2e tests on a Kind cluster on Linux with all features enabled, the test you updated got failed. In Integration test, you need to update test |
a7ddb86
to
8c6a4a0
Compare
/test-all |
8c6a4a0
to
ea1971a
Compare
2e50dd5
to
2051562
Compare
1. Only load Service GroupID into reg when AntreaPolicy is enabled. Service GroupID is only used by AntreaPolicy "toServices" and "AppliedTo NodePort Serivces" features for now. 2. In IngressSecurityClassifierTable, only forward packet to AntreaPolicyIngressRuleTable when AntreaPolicy is enabled and proxyAll is enabled. This forward flow is only used by AntreaPolicy "AppliedTo NodePort Services" feature to avoid packets skip AntreaPolicyIngressRuleTable, where policy will be enforced, when the endpoint of this Service is not on current NodePort Node. 3. In ACNP appliedTo NodePort Service e2e test, change to add another netNS to fake external network. 4. Change to use gwOFPort as inPort of reject response for some cases. Signed-off-by: graysonwu <wgrayson@vmware.com>
2051562
to
8f3a6c9
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, and could you help double confirm @tnqn ? Thanks
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. Just want to confirm the impact of the PR: it doesn't fix any issue but reduces unnecessary cost when AntreaPolicy is not enabled, right?
It fixed one issue: when AntreaProxy is enabled and AntreaPolicy is disabled, we can't resubmit packets to |
Thanks for the information. Please file an issue or describe the fixed issue in the PR itself for future PRs, otherwise not easy to figure out the severity level and impact of a change. Based on your description, I think we should backport it to maintained releases (1.5 and later). |
Got it. Thanks. Will do it in the future. About backporting, maybe 1.8 should be enough. The issue I mentioned above was introduced by PR #3997. |
1. Only load Service GroupID into reg when AntreaPolicy is enabled. Service GroupID is only used by AntreaPolicy "toServices" and "AppliedTo NodePort Serivces" features for now. 2. In IngressSecurityClassifierTable, only forward packet to AntreaPolicyIngressRuleTable when AntreaPolicy is enabled and proxyAll is enabled. This forward flow is only used by AntreaPolicy "AppliedTo NodePort Services" feature to avoid packets skip AntreaPolicyIngressRuleTable, where policy will be enforced, when the endpoint of this Service is not on current NodePort Node. 3. In ACNP appliedTo NodePort Service e2e test, change to add another netNS to fake external network. 4. Change to use gwOFPort as inPort of reject response for some cases. Signed-off-by: graysonwu <wgrayson@vmware.com>
1. Only load Service GroupID into reg when AntreaPolicy is enabled. Service GroupID is only used by AntreaPolicy "toServices" and "AppliedTo NodePort Serivces" features for now. 2. In IngressSecurityClassifierTable, only forward packet to AntreaPolicyIngressRuleTable when AntreaPolicy is enabled and proxyAll is enabled. This forward flow is only used by AntreaPolicy "AppliedTo NodePort Services" feature to avoid packets skip AntreaPolicyIngressRuleTable, where policy will be enforced, when the endpoint of this Service is not on current NodePort Node. 3. In ACNP appliedTo NodePort Service e2e test, change to add another netNS to fake external network. 4. Change to use gwOFPort as inPort of reject response for some cases. Signed-off-by: graysonwu <wgrayson@vmware.com>
Only load Service GroupID into reg when AntreaPolicy is enabled.
Service GroupID is only used by AntreaPolicy "toServices"
and "AppliedTo NodePort Serivces" features for now.
In IngressSecurityClassifierTable, only forward packet to
AntreaPolicyIngressRuleTable when AntreaPolicy is enabled and
proxyAll is enabled.
This forward flow is only used by AntreaPolicy "AppliedTo NodePort
Services" feature to avoid packets skip
AntreaPolicyIngressRuleTable, where policy will be enforced, when
the endpoint of this Service is not on current NodePort Node.
In ACNP appliedTo NodePort Service e2e test, change to add another
netNS to fake external network.
Signed-off-by: graysonwu wgrayson@vmware.com