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

Dualstack support for kube-proxy iptables mode #82462

Merged
merged 1 commit into from Jan 7, 2020

Conversation

@vllry
Copy link
Contributor

vllry commented Sep 8, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

kube-proxy: Added dual-stack IPv4/IPv6 support to the iptables proxier.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@vllry

This comment has been minimized.

Copy link
Contributor Author

vllry commented Sep 8, 2019

@vllry vllry force-pushed the vllry:dualstack-iptables branch 4 times, most recently from ac34b97 to 17dcc69 Sep 8, 2019
@vllry vllry force-pushed the vllry:dualstack-iptables branch 2 times, most recently from c3db649 to f64d914 Sep 12, 2019
@vllry vllry force-pushed the vllry:dualstack-iptables branch from f64d914 to 735881c Sep 12, 2019
@aojea

This comment has been minimized.

Copy link
Member

aojea commented Sep 12, 2019

/test pull-kubernetes-integration

@aojea aojea mentioned this pull request Sep 12, 2019
7 of 7 tasks complete
@vllry vllry force-pushed the vllry:dualstack-iptables branch from 735881c to cab0a8f Sep 14, 2019
@vllry

This comment has been minimized.

Copy link
Contributor Author

vllry commented Sep 15, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@vllry vllry force-pushed the vllry:dualstack-iptables branch from 23cb1fe to 7bd40f2 Dec 17, 2019
@vllry vllry force-pushed the vllry:dualstack-iptables branch from 7bd40f2 to 23957a6 Dec 17, 2019
@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Dec 17, 2019
Copy link
Member

thockin left a comment

relatively minor comments from me, I think, and even these could be done in a followup unless someone disagrees strongly?

klog.V(0).Info("creating dualStackProxier for iptables.")

// Create iptables handlers for both families, one is already created
// Always ordered as IPv4, IPv6

This comment has been minimized.

Copy link
@thockin

thockin Dec 17, 2019

Member

To be fair, this is copying the ipvs logic. If we want to raise the bar, we should do so for both proxiers.

Perhaps, instead of [2]utiliptables.Interface it should be a map[core.IPFamily]utiliptables.Interface. Remove order from the equasion, since that seems to be meaningless anyway (and assumed ordering is bound to explode on us some day).

@@ -164,6 +164,56 @@ func newProxyServer(
healthzServer,
config.NodePortAddresses,
)

if utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) {

This comment has been minimized.

Copy link
@thockin

thockin Dec 17, 2019

Member

ipvs follows this same logic. If dual-stack is enabled, make a dual-stack proxier. We want to diminish the meaning of cluster-cidr here, so I do not think testing that should be used. TL;DR is that we have no real way to know whether dual-stack is enabled or not except to try it, and even that could change at run time. Very little cost (I think) to setting up both modes, but we do need to handle failure gracefully if dual-stack is enabled on a single-stack node.

nodePortAddresses []string,
) (proxy.Provider, error) {
// Create an ipv4 instance of the single-stack proxier
ipv4Proxier, err := NewProxier(ipt[0], sysctl,

This comment has been minimized.

Copy link
@thockin

thockin Dec 17, 2019

Member

ipt[] is now guaranteed to be v4, v6. Are clusterCidr[] and nodeIP? It seems they are, though same comments as above - perhaps turning it into maps would be better

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Dec 17, 2019

In other words, I will approve and hold, @vllry can decide to clear the hold or fix now.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 17, 2019
@thockin

This comment has been minimized.

Copy link
Member

thockin commented Dec 17, 2019

/retest

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 17, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@vllry

This comment has been minimized.

Copy link
Contributor Author

vllry commented Dec 17, 2019

Perhaps, instead of [2]utiliptables.Interface it should be a map[core.IPFamily]utiliptables.Interface. Remove order from the equasion, since that seems to be meaningless anyway (and assumed ordering is bound to explode on us some day).

I do prefer this idea, I think it was actually in an earlier draft of this PR. Would probably suggest this be a followup, and I'll try to handle the other suggestions before merging.

@danwinship

This comment has been minimized.

Copy link
Contributor

danwinship commented Dec 17, 2019

In other words, I will approve and hold

except you forgot to actually
/hold
🙂

(Assuming the hold is wanted since @vllry said she was going to address your other suggestions.)

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Dec 17, 2019

@vllry my comments seem to be exclusively about maps. If you want to do a follow-up, feel free to clear the hold.

Thanks @danwinship for catching my flub

@vllry

This comment has been minimized.

Copy link
Contributor Author

vllry commented Jan 7, 2020

Thanks everything, this was a very long time coming. Will look at the map/ordering comments as followups, as they address issues that existed prior to this PR.

/hold cancel

@vllry

This comment has been minimized.

Copy link
Contributor Author

vllry commented Jan 7, 2020

/retest

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jan 7, 2020

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@aojea

This comment has been minimized.

Copy link
Member

aojea commented Jan 7, 2020

/retest
known flake #75355

@k8s-ci-robot k8s-ci-robot merged commit 5373fa3 into kubernetes:master Jan 7, 2020
15 checks passed
15 checks passed
cla/linuxfoundation vllry authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test 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-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind 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
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Jan 7, 2020
@lachie83

This comment has been minimized.

Copy link
Member

lachie83 commented Jan 7, 2020

What what!!!

@lachie83

This comment has been minimized.

Copy link
Member

lachie83 commented Jan 7, 2020

Awesome work getting this merged. Thank you very much @vllry and sig-network!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.