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

Fix ExternalTrafficPolicy support for Service ExternalIPs #88786

Merged
merged 4 commits into from Mar 14, 2020

Conversation

@freehan
Copy link
Member

freehan commented Mar 4, 2020

Ref: #69811

This PR fixes the issue for iptables/ipvs kube-proxy with a feature gate to toggle the fix.

Fix a bug where ExternalTrafficPolicy is not applied to service ExternalIPs.
@smourapina

This comment has been minimized.

Copy link
Member

smourapina commented Mar 4, 2020

@freehan, @justinsb & @dchen1107: Bug Triage here for release 1.18, with a friendly reminder that we are 1 day away from code freeze (5 March EOD), so please keep this in mind. Thanks!

@freehan

This comment has been minimized.

Copy link
Member Author

freehan commented Mar 4, 2020

/kind bug

@k8s-ci-robot k8s-ci-robot added kind/bug and removed needs-kind labels Mar 4, 2020
@freehan freehan force-pushed the freehan:externalIP branch from b004ad0 to 0d040e4 Mar 4, 2020
@smourapina

This comment has been minimized.

Copy link
Member

smourapina commented Mar 10, 2020

Hello again @freehan, @justinsb & @dchen1107:
We are now in code freeze for 1.18, so I will go ahead and set the milestone to 1.19, since this is still pending approval. Please let me know in case you have any questions!
/milestone v1.19

@k8s-ci-robot k8s-ci-robot modified the milestones: v1.18, v1.19 Mar 10, 2020
Copy link
Member

thockin left a comment

I think this does need a release-note.

/lgtm
/approve

@freehan freehan force-pushed the freehan:externalIP branch from 0d040e4 to c9f3260 Mar 11, 2020
@k8s-ci-robot k8s-ci-robot added size/L and removed size/S labels Mar 11, 2020
@freehan freehan force-pushed the freehan:externalIP branch from c9f3260 to 99bd7b2 Mar 11, 2020
@freehan freehan force-pushed the freehan:externalIP branch from 99bd7b2 to 550e540 Mar 12, 2020
@freehan

This comment has been minimized.

Copy link
Member Author

freehan commented Mar 12, 2020

/test pull-kubernetes-integration

1 similar comment
@freehan

This comment has been minimized.

Copy link
Member Author

freehan commented Mar 12, 2020

/test pull-kubernetes-integration

Copy link
Member

thockin left a comment

/lgtm
/approve

/hold

@andrewsykim @lbernail Have you looked at this for IPVS?

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 13, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Member

andrewsykim left a comment

Some questions about SNAT, if it was intentional to SNAT node local traffic for externalIPs, then LGTM

@@ -1063,6 +1063,10 @@ func (proxier *Proxier) syncProxyRules() {
}

if hasEndpoints {
destChain := svcChain
if utilfeature.DefaultFeatureGate.Enabled(features.ExternalPolicyForExternalIP) {

This comment has been minimized.

Copy link
@andrewsykim

andrewsykim Mar 13, 2020

Member

On line 1078 we're still jumping to KubeMarkMasqChain even though it can be node local now, is that expected?

This comment has been minimized.

Copy link
@freehan

freehan Mar 13, 2020

Author Member

Good catch. Will skip it then.

if err := proxier.syncEndpoint(svcName, false, serv); err != nil {

onlyNodeLocalEndpoints := false
if utilfeature.DefaultFeatureGate.Enabled(features.ExternalPolicyForExternalIP) {

This comment has been minimized.

Copy link
@andrewsykim

andrewsykim Mar 13, 2020

Member

Same question for IPVS, on line 1240 we're adding the externalIP to the ipset kubeExternalIPSet which we SNAT. Should there be a new ipset like kubeExternalIPNodeLocalIPSet that skips SNAT?

This comment has been minimized.

Copy link
@freehan

freehan Mar 13, 2020

Author Member

Fixed. Added kubeExternalIPNodeLocalIPSet that skips SNAT

@freehan freehan force-pushed the freehan:externalIP branch from 550e540 to 5c14e5c Mar 13, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 13, 2020
Copy link
Member Author

freehan left a comment

Skipped SNAT for ExternalIP when ExternalTrafficPolicy=Local. Ready for another round.

if err := proxier.syncEndpoint(svcName, false, serv); err != nil {

onlyNodeLocalEndpoints := false
if utilfeature.DefaultFeatureGate.Enabled(features.ExternalPolicyForExternalIP) {

This comment has been minimized.

Copy link
@freehan

freehan Mar 13, 2020

Author Member

Fixed. Added kubeExternalIPNodeLocalIPSet that skips SNAT

@freehan freehan force-pushed the freehan:externalIP branch from 5c14e5c to 068963f Mar 13, 2020
Copy link
Member

andrewsykim left a comment

/lgtm

@freehan

This comment has been minimized.

Copy link
Member Author

freehan commented Mar 14, 2020

/test pull-kubernetes-node-e2e

@andrewsykim

This comment has been minimized.

Copy link
Member

andrewsykim commented Mar 14, 2020

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit 0d85d16 into kubernetes:master Mar 14, 2020
17 of 18 checks passed
17 of 18 checks passed
tide Not mergeable. Retesting: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation freehan authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-kind-ga-only-parallel Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-alpha-features Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-e2e-kind-ipv6 Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.