Enhance ACNP Service related feature by GraysonWu · Pull Request #4261 · antrea-io/antrea

GraysonWu · 2022-09-30T01:19:10Z

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

codecov · 2022-09-30T01:35:06Z

Codecov Report

Merging #4261 (2051562) into main (1e31a78) will increase coverage by 0.25%.
The diff coverage is 44.44%.

❗ Current head 2051562 differs from pull request most recent head 8f3a6c9. Consider uploading reports for the commit 8f3a6c9 to get more accurate results

@@            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

Flag	Coverage Δ
integration-tests	`34.50% <85.71%> (+0.04%)`	⬆️
kind-e2e-tests	`48.74% <44.44%> (-0.11%)`	⬇️
unit-tests	`44.96% <11.11%> (+0.42%)`	⬆️

Impacted Files	Coverage Δ
pkg/agent/controller/networkpolicy/reject.go	`72.90% <9.09%> (-4.60%)`	⬇️
pkg/agent/openflow/client.go	`70.93% <100.00%> (-1.40%)`	⬇️
pkg/agent/openflow/pipeline.go	`84.67% <100.00%> (+1.17%)`	⬆️
pkg/agent/openflow/service.go	`91.66% <100.00%> (+0.08%)`	⬆️
pkg/controller/externalippool/controller.go	`86.16% <0.00%> (-6.25%)`	⬇️
pkg/controller/traceflow/controller.go	`74.72% <0.00%> (-2.53%)`	⬇️
pkg/agent/openflow/multicluster.go	`44.85% <0.00%> (-1.87%)`	⬇️
...trollers/multicluster/resourceexport_controller.go	`78.07% <0.00%> (-1.61%)`	⬇️
pkg/agent/util/net.go	`52.05% <0.00%> (-1.50%)`	⬇️
... and 37 more

wenyingd

Code change LGTM, please pay attention to the test coverage.

hongliangl · 2022-09-30T03:30:00Z

+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 {


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).

tnqn

LGTM except the test issue @hongliangl has pointed out.

hongliangl · 2022-10-05T01:54:19Z

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 TestProxyServiceFlows.

GraysonWu · 2022-10-07T18:06:34Z

/test-all

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>

GraysonWu · 2022-10-17T03:23:47Z

/test-all

hongliangl

LGTM, and could you help double confirm @tnqn ? Thanks

tnqn

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?

GraysonWu · 2022-10-19T01:50:21Z

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 AntreaPolicyIngressRuleTable which doesn't exist.
https://github.com/antrea-io/antrea/pull/4261/files#diff-120d06ea0809e1c6293ce9a1f6b2f4bf37d4fba0ec569be913f884aa8cdd4124R2116

tnqn · 2022-10-19T03:20:14Z

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 AntreaPolicyIngressRuleTable which doesn't exist. https://github.com/antrea-io/antrea/pull/4261/files#diff-120d06ea0809e1c6293ce9a1f6b2f4bf37d4fba0ec569be913f884aa8cdd4124R2116

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).

GraysonWu · 2022-10-19T05:10:32Z

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 AntreaPolicyIngressRuleTable which doesn't exist. https://github.com/antrea-io/antrea/pull/4261/files#diff-120d06ea0809e1c6293ce9a1f6b2f4bf37d4fba0ec569be913f884aa8cdd4124R2116

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>

GraysonWu requested a review from hongliangl September 30, 2022 01:19

GraysonWu changed the title ~~Enhance ACNP applied to NodePort Service~~ [WIP]Enhance ACNP applied to NodePort Service Sep 30, 2022

GraysonWu force-pushed the fix-np-apply-to-svc branch from bdf6c87 to 7be1bb8 Compare September 30, 2022 02:37

GraysonWu requested a review from wenyingd September 30, 2022 02:39

GraysonWu changed the title ~~[WIP]Enhance ACNP applied to NodePort Service~~ Enhance ACNP applied to NodePort Service Sep 30, 2022

wenyingd reviewed Sep 30, 2022

View reviewed changes

hongliangl reviewed Sep 30, 2022

View reviewed changes

tnqn reviewed Sep 30, 2022

View reviewed changes

GraysonWu changed the title ~~Enhance ACNP applied to NodePort Service~~ Enhance ACNP Service related feature Sep 30, 2022

GraysonWu force-pushed the fix-np-apply-to-svc branch from 265627b to c30d222 Compare October 3, 2022 16:43

GraysonWu force-pushed the fix-np-apply-to-svc branch 5 times, most recently from a7ddb86 to 8c6a4a0 Compare October 6, 2022 20:31

GraysonWu requested a review from hongliangl October 9, 2022 22:30

hongliangl reviewed Oct 10, 2022

View reviewed changes

Comment thread test/e2e/antreapolicy_test.go Outdated

tnqn added this to the Antrea v1.9 release milestone Oct 11, 2022

GraysonWu force-pushed the fix-np-apply-to-svc branch from 8c6a4a0 to ea1971a Compare October 13, 2022 07:23

hongliangl reviewed Oct 13, 2022

View reviewed changes

Comment thread pkg/agent/controller/networkpolicy/reject.go Outdated

Comment thread pkg/agent/controller/networkpolicy/reject.go Outdated

GraysonWu force-pushed the fix-np-apply-to-svc branch 2 times, most recently from 2e50dd5 to 2051562 Compare October 14, 2022 08:13

GraysonWu force-pushed the fix-np-apply-to-svc branch from 2051562 to 8f3a6c9 Compare October 14, 2022 15:48

hongliangl reviewed Oct 17, 2022

View reviewed changes

GraysonWu requested a review from tnqn October 17, 2022 06:27

tnqn approved these changes Oct 18, 2022

View reviewed changes

tnqn added action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes. labels Oct 19, 2022

tnqn merged commit 0307eff into antrea-io:main Oct 19, 2022

GraysonWu mentioned this pull request Oct 19, 2022

Automated cherry pick of #4261: Enhance ACNP Service related feature #4315

Merged

Conversation

GraysonWu commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

wenyingd left a comment

Choose a reason for hiding this comment

Uh oh!

hongliangl Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

tnqn left a comment

Choose a reason for hiding this comment

Uh oh!

hongliangl commented Oct 5, 2022

Uh oh!

GraysonWu commented Oct 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GraysonWu commented Oct 17, 2022

Uh oh!

hongliangl left a comment

Choose a reason for hiding this comment

Uh oh!

tnqn left a comment

Choose a reason for hiding this comment

Uh oh!

GraysonWu commented Oct 19, 2022

Uh oh!

tnqn commented Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GraysonWu commented Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GraysonWu commented Sep 30, 2022 •

edited

Loading

codecov Bot commented Sep 30, 2022 •

edited

Loading

tnqn commented Oct 19, 2022 •

edited

Loading

GraysonWu commented Oct 19, 2022 •

edited

Loading