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.

Sign up for GitHub

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

Jump to bottom

Preserve client IP if the selected Endpoint is local regardless of ExternalTrafficPolicy #3604

Merged
merged 1 commit into from
Apr 26, 2022
Merged

Preserve client IP if the selected Endpoint is local regardless of ExternalTrafficPolicy #3604

merged 1 commit into from
Apr 26, 2022

Conversation

hongliangl
Copy link
Contributor

When an external client accesses to a NodePort / LoadBalancer Service on a K8s
Node, if the selected Endpoint is just on the K8s Node, then don't SNAT the
connection even the externalTrafficPolicy of the Service is Cluster.

Signed-off-by: Hongliang Liu lhongliang@vmware.com

@hongliangl hongliangl requested review from tnqn and jianjuns April 8, 2022 11:24
@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2022 •
edited
Loading

Codecov Report

Merging #3604 (2861fe4) into main (5905e98) will decrease coverage by 6.98%.
The diff coverage is 94.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3604      +/-   ##
==========================================
- Coverage   64.66%   57.68%   -6.99%     
==========================================
  Files         278      392     +114     
  Lines       39363    55216   +15853     
==========================================
+ Hits        25454    31849    +6395     
- Misses      11939    20921    +8982     
- Partials     1970     2446     +476
Flag Coverage Δ
e2e-tests 49.54% <90.90%> (?)
integration-tests 38.41% <ø> (?)
kind-e2e-tests 52.34% <86.48%> (+<0.01%) ⬆️
unit-tests 43.81% <5.40%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
pkg/agent/openflow/pipeline.go 77.33% <94.28%> (+1.82%) ⬆️
pkg/agent/openflow/client.go 73.91% <100.00%> (+1.78%) ⬆️
pkg/agent/openflow/service.go 83.90% <100.00%> (+0.18%) ⬆️
pkg/agent/controller/networkpolicy/fqdn.go 75.51% <0.00%> (-2.60%) ⬇️
pkg/agent/cniserver/ipam/antrea_ipam.go 71.89% <0.00%> (-1.77%) ⬇️
pkg/controller/ipam/antrea_ipam_controller.go 77.55% <0.00%> (-1.54%) ⬇️
...ntroller/networkpolicy/networkpolicy_controller.go 70.17% <0.00%> (-0.88%) ⬇️
...gent/controller/networkpolicy/status_controller.go 81.51% <0.00%> (-0.85%) ⬇️
pkg/ovs/ovsctl/appctl.go 41.86% <0.00%> (-0.78%) ⬇️
pkg/agent/controller/networkpolicy/reconciler.go 77.05% <0.00%> (-0.43%) ⬇️
... and 136 more

tnqn
tnqn reviewed Apr 8, 2022
View reviewed changes
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM, but would defer to @jianjuns to say whether it makes sense.

I would suggest to change the title to "Preserve client IP if the selected Endpoint is local regardless of ExternalTrafficPolicy"

@hongliangl hongliangl changed the title Don't SNAT when a local Endpoint is selected for Service connections Preserve client IP if the selected Endpoint is local regardless of ExternalTrafficPolicy Apr 8, 2022
@hongliangl hongliangl requested review from wenyingd and tnqn April 8, 2022 15:19
@hongliangl hongliangl force-pushed the service_snat_optimize branch from a229f74 to 3154fd1 Compare April 8, 2022 15:20
@hongliangl
Copy link
Contributor Author

/test-all-features-conformance
/test-conformance
/test-e2e
/test-flexible-ipam-e2e
/test-multicluster-e2e
/test-networkpolicy
/test-windows-conformance
/test-windows-e2e
/test-windows-networkpolicy
/test-windows-proxyall-e2e
/test-integration

jianjuns
jianjuns previously approved these changes Apr 8, 2022
View reviewed changes
Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

I think the change is fine.

@hongliangl
Copy link
Contributor Author

/test-e2e
/test-flexible-ipam-e2e
/test-windows-networkpolicy
/test-windows-proxyall-e2e
/test-ipv6-conformance
/test-ipv6-e2e
/test-ipv6-networkpolicy
/test-ipv6-only-conformance
/test-ipv6-only-e2e
/test-ipv6-only-networkpolicy

@hongliangl hongliangl marked this pull request as draft April 9, 2022 00:56
@hongliangl
Copy link
Contributor Author

I found that there is bug when noEncap mode.

@hongliangl hongliangl force-pushed the service_snat_optimize branch from 3154fd1 to 31e72d0 Compare April 9, 2022 03:00
@hongliangl hongliangl marked this pull request as ready for review April 9, 2022 03:01
@hongliangl hongliangl requested a review from jianjuns April 9, 2022 03:01
@hongliangl hongliangl force-pushed the service_snat_optimize branch from 31e72d0 to 712a492 Compare April 9, 2022 03:19
@hongliangl hongliangl added the action/release-note Indicates a PR that should be included in release notes. label Apr 9, 2022
@hongliangl
Copy link
Contributor Author

/test-all-features-conformance
/test-conformance
/test-e2e
/test-flexible-ipam-e2e
/test-multicluster-e2e
/test-networkpolicy
/test-windows-conformance
/test-windows-e2e
/test-windows-networkpolicy
/test-windows-proxyall-e2e
/test-integration

@hongliangl
Copy link
Contributor Author

/test-networkpolicy
/test-windows-networkpolicy

@hongliangl
Copy link
Contributor Author

/test-windows-networkpolicy

1 similar comment
@hongliangl
Copy link
Contributor Author

/test-windows-networkpolicy

@hongliangl hongliangl force-pushed the service_snat_optimize branch from 712a492 to 7db1576 Compare April 12, 2022 16:14
@hongliangl
Copy link
Contributor Author

/test-all-features-conformance
/test-conformance
/test-e2e
/test-flexible-ipam-e2e
/test-multicluster-e2e
/test-networkpolicy
/test-windows-conformance
/test-windows-e2e
/test-windows-networkpolicy
/test-windows-proxyall-e2e
/test-integration

@hongliangl
Copy link
Contributor Author

Do you have further review comments? @tnqn @jianjuns Thanks.

@hongliangl hongliangl force-pushed the service_snat_optimize branch from 7db1576 to d4fe443 Compare April 13, 2022 01:18
@hongliangl hongliangl requested a review from tnqn April 13, 2022 01:20
@hongliangl hongliangl force-pushed the service_snat_optimize branch 3 times, most recently from b945cba to 0600b80 Compare April 14, 2022 17:01
@hongliangl
Copy link
Contributor Author

/test-all-features-conformance
/test-conformance
/test-e2e
/test-flexible-ipam-e2e
/test-multicluster-e2e
/test-networkpolicy
/test-windows-conformance
/test-windows-e2e
/test-windows-networkpolicy
/test-windows-proxyall-e2e

@hongliangl
Copy link
Contributor Author

/test-windows-conformance
/test-ipv6-conformance
/test-ipv6-e2e
/test-ipv6-networkpolicy
/test-ipv6-only-conformance
/test-ipv6-only-e2e
/test-ipv6-only-networkpolicy

@hongliangl
Copy link
Contributor Author

/test-windows-conformance
/test-ipv6-only-e2e
/test-ipv6-networkpolicy
/test-ipv6-e2e

@hongliangl
Copy link
Contributor Author

/test-ipv6-e2e
/test-flexible-ipam-e2e

@hongliangl
Copy link
Contributor Author

/test-ipv6-e2e

@hongliangl
Copy link
Contributor Author

@tnqn Do you have any further comments? Thanks.

@hongliangl hongliangl requested a review from tnqn April 16, 2022 03:32
@hongliangl
Copy link
Contributor Author

/test-ipv6-e2e

1 similar comment
@hongliangl
Copy link
Contributor Author

/test-ipv6-e2e

@hongliangl
Preserve client IP if the selected Endpoint is local regardless of Ex…
2861fe4 
…ternalTrafficPolicy

When an external client accesses to a NodePort / LoadBalancer Service on a K8s
Node, if the selected Endpoint is just on the K8s Node, then don't SNAT the
connection even the externalTrafficPolicy of the Service is Cluster.

Signed-off-by: Hongliang Liu <lhongliang@vmware.com>
@hongliangl hongliangl force-pushed the service_snat_optimize branch from 0600b80 to 2861fe4 Compare April 20, 2022 23:44
@hongliangl
Copy link
Contributor Author

/test-all-features-conformance
/test-conformance
/test-e2e
/test-flexible-ipam-e2e
/test-ipv6-conformance
/test-ipv6-e2e
/test-ipv6-networkpolicy
/test-ipv6-only-conformance
/test-ipv6-only-e2e
/test-ipv6-only-networkpolicy
/test-multicluster-e2e
/test-networkpolicy
/test-windows-conformance
/test-windows-e2e
/test-windows-networkpolicy
/test-windows-proxyall-e2e
/test-integration

@hongliangl
Copy link
Contributor Author

/test-e2e

@hongliangl
Copy link
Contributor Author

/test-all-features-conformance
/test-conformance
/test-e2e
/test-flexible-ipam-e2e
/test-ipv6-conformance
/test-ipv6-e2e
/test-ipv6-networkpolicy
/test-ipv6-only-conformance
/test-ipv6-only-e2e
/test-ipv6-only-networkpolicy
/test-multicluster-e2e
/test-networkpolicy
/test-windows-conformance
/test-windows-e2e
/test-windows-networkpolicy
/test-windows-proxyall-e2e
/test-integration

@hongliangl
Copy link
Contributor Author

/test-all-features-conformance
/test-e2e
/test-ipv6-e2e
/test-multicluster-e2e
/test-networkpolicy

@hongliangl
Copy link
Contributor Author

/test-networkpolicy

@hongliangl
Copy link
Contributor Author

/test-networkpolicy
/test-ipv6-e2e

@hongliangl
Copy link
Contributor Author

/test-networkpolicy
/test-ipv6-e2e
/test-all-features-conformance

@hongliangl
Copy link
Contributor Author

/test-networkpolicy

1 similar comment
@hongliangl
Copy link
Contributor Author

/test-networkpolicy

@hongliangl
Copy link
Contributor Author

/test-networkpolicy
/test-ipv6-e2e

@hongliangl hongliangl requested a review from jianjuns April 26, 2022 01:29
@hongliangl
Copy link
Contributor Author

/test-ipv6-e2e

1 similar comment
@hongliangl
Copy link
Contributor Author

/test-ipv6-e2e

tnqn
tnqn approved these changes Apr 26, 2022
View reviewed changes
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@tnqn tnqn merged commit ffd410a into antrea-io:main Apr 26, 2022
@hongliangl hongliangl deleted the service_snat_optimize branch April 28, 2022 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnqn tnqn tnqn approved these changes

@wenyingd wenyingd Awaiting requested review from wenyingd

@jianjuns jianjuns Awaiting requested review from jianjuns

Assignees
No one assigned
Labels
action/release-note Indicates a PR that should be included in release notes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hongliangl @codecov-commenter @wenyingd @tnqn @jianjuns