Skip to content
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

Add ServiceLocalToRemote reject type #4547

Merged
merged 2 commits into from
Mar 2, 2023
Merged

Conversation

GraysonWu
Copy link
Contributor

@GraysonWu GraysonWu commented Jan 10, 2023

Fixes #4535

For single cluster rejection, there is no difference for Service traffic or Pod-to-Pod traffic in localToRemote rejection. Service LB won't be executed on the Node where we generate reject responses. So we didn't define podLocalToRemote and serviceLocalToRemote and just let the reject response packet start from L3/L2Forwarding table based on the Node type.

But in multi-cluster situations, when the server Endpoint Pod is on the gateway Node of its cluster, where Service LB is executed, we need reject response packets to go thru the whole pipeline to be correctly UnDNATed and forwarded.

Signed-off-by: graysonwu [email protected]

@GraysonWu
Copy link
Contributor Author

/test-multicluster-e2e

@codecov
Copy link

codecov bot commented Jan 10, 2023

Codecov Report

Merging #4547 (457a989) into main (4de4392) will increase coverage by 1.50%.
The diff coverage is 78.48%.

❗ Current head 457a989 differs from pull request most recent head f69c50d. Consider uploading reports for the commit f69c50d to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4547      +/-   ##
==========================================
+ Coverage   68.31%   69.81%   +1.50%     
==========================================
  Files         400      414      +14     
  Lines       58324    58454     +130     
==========================================
+ Hits        39843    40812     +969     
+ Misses      15708    14851     -857     
- Partials     2773     2791      +18     
Flag Coverage Δ *Carryforward flag
e2e-tests 40.37% <45.35%> (+2.03%) ⬆️
integration-tests 34.46% <17.80%> (+0.01%) ⬆️ Carriedforward from e909b97
kind-e2e-tests 47.15% <50.93%> (+0.57%) ⬆️ Carriedforward from e909b97
unit-tests 59.79% <71.90%> (+1.86%) ⬆️ Carriedforward from e909b97

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
cmd/antrea-agent/agent.go 0.00% <0.00%> (ø)
pkg/agent/ipassigner/ip_assigner_windows.go 0.00% <0.00%> (ø)
pkg/agent/memberlist/cluster.go 78.81% <ø> (+0.42%) ⬆️
pkg/agent/openflow/client.go 88.66% <ø> (ø)
pkg/antctl/antctl.go 50.00% <ø> (ø)
pkg/apiserver/handlers/webhook/convert_crd.go 3.92% <0.00%> (ø)
pkg/ovs/ovsconfig/default_windows.go 100.00% <ø> (ø)
pkg/ovs/ovsconfig/ovs_client.go 66.82% <ø> (+0.12%) ⬆️
pkg/agent/controller/networkpolicy/reconciler.go 71.90% <16.66%> (+1.44%) ⬆️
...gent/controller/noderoute/node_route_controller.go 62.56% <25.00%> (-0.63%) ⬇️
... and 105 more

@GraysonWu GraysonWu requested a review from luolanzone January 10, 2023 19:30
@GraysonWu
Copy link
Contributor Author

/test-multicluster-e2e

@luolanzone luolanzone added the area/multi-cluster Issues or PRs related to multi cluster. label Jan 12, 2023
@GraysonWu
Copy link
Contributor Author

/test-multicluster-e2e

pkg/agent/controller/networkpolicy/reject.go Outdated Show resolved Hide resolved
multicluster/test/e2e/service_test.go Outdated Show resolved Hide resolved
multicluster/test/e2e/service_test.go Outdated Show resolved Hide resolved
pkg/agent/controller/networkpolicy/reject.go Show resolved Hide resolved
@GraysonWu
Copy link
Contributor Author

/test-multicluster-e2e

@GraysonWu
Copy link
Contributor Author

/test-all

@GraysonWu
Copy link
Contributor Author

/test-multicluster-e2e

multicluster/test/e2e/service_test.go Outdated Show resolved Hide resolved
multicluster/test/e2e/service_test.go Show resolved Hide resolved
multicluster/test/e2e/service_test.go Outdated Show resolved Hide resolved
@GraysonWu GraysonWu force-pushed the fix-snp-rej branch 2 times, most recently from f943287 to f5072ae Compare January 30, 2023 22:40
@GraysonWu
Copy link
Contributor Author

/test-multicluster-e2e

Dyanngg
Dyanngg previously approved these changes Feb 6, 2023
luolanzone
luolanzone previously approved these changes Feb 7, 2023
Copy link
Contributor

@luolanzone luolanzone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall, a few nits.

multicluster/test/e2e/fixtures.go Outdated Show resolved Hide resolved
multicluster/test/e2e/service_test.go Outdated Show resolved Hide resolved
multicluster/test/e2e/service_test.go Outdated Show resolved Hide resolved
@GraysonWu
Copy link
Contributor Author

/test-multicluster-e2e
/test-all

@GraysonWu
Copy link
Contributor Author

/test-all

luolanzone
luolanzone previously approved these changes Feb 9, 2023
Copy link
Contributor

@luolanzone luolanzone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @jianjuns @tnqn can you take a look? thanks.

jianjuns
jianjuns previously approved these changes Feb 14, 2023
@luolanzone
Copy link
Contributor

@GraysonWu the unit test diff coverage is 0, could you check if possible to add some unit tests? thanks.

@luolanzone
Copy link
Contributor

/test-multicluster-e2e

@jianjuns
Copy link
Contributor

/test-all

@GraysonWu
Copy link
Contributor Author

/test-all
/test-multicluster-e2e

Copy link
Contributor

@luolanzone luolanzone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits.

pkg/agent/controller/networkpolicy/reject_test.go Outdated Show resolved Hide resolved
pkg/agent/controller/networkpolicy/reject_test.go Outdated Show resolved Hide resolved
pkg/agent/controller/networkpolicy/reject_test.go Outdated Show resolved Hide resolved
pkg/agent/controller/networkpolicy/reject_test.go Outdated Show resolved Hide resolved
pkg/agent/controller/networkpolicy/reject_test.go Outdated Show resolved Hide resolved
pkg/agent/controller/networkpolicy/reject_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@luolanzone luolanzone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few nits

pkg/agent/controller/networkpolicy/reject_test.go Outdated Show resolved Hide resolved
pkg/agent/controller/networkpolicy/reject_test.go Outdated Show resolved Hide resolved
pkg/agent/controller/networkpolicy/reject_test.go Outdated Show resolved Hide resolved
Signed-off-by: graysonwu <[email protected]>
@GraysonWu
Copy link
Contributor Author

/test-all
/test-multicluster-e2e

Copy link
Contributor

@luolanzone luolanzone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@luolanzone
Copy link
Contributor

@jianjuns could you help to take a look again? @GraysonWu added a few unit tests for the change.

@vicky-liu vicky-liu added this to the Antrea v1.11 release milestone Mar 2, 2023
@jianjuns jianjuns merged commit 4e5fba7 into antrea-io:main Mar 2, 2023
tnqn pushed a commit that referenced this pull request Apr 7, 2023
jainpulkit22 pushed a commit to urharshitha/antrea that referenced this pull request Apr 28, 2023
For single cluster rejection, there is no difference for Service traffic or Pod-to-Pod traffic in localToRemote rejection. Service LB won't be executed on the Node where we generate reject responses. So we didn't define podLocalToRemote and serviceLocalToRemote and just let the reject response packet start from L3/L2Forwarding table based on the Node type.

But in multi-cluster situations, when the server Endpoint Pod is on the gateway Node of its cluster, where Service LB is executed, we need reject response packets to go thru the whole pipeline to be correctly UnDNATed and forwarded.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/multi-cluster Issues or PRs related to multi cluster.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky StretchedNetworkPolicy e2e test failure
5 participants