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

Harden kube-proxy for unmatched IP versions #56880

Merged
merged 8 commits into from Feb 28, 2018

Conversation

@MrHohn
Copy link
Member

MrHohn commented Dec 6, 2017

What this PR does / why we need it:
This PR makes kube-proxy omits & logs & emits event for unmatched IP versions configuration (IPv6 address in IPv4 mode or IPv4 address in IPv6 mode).

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #57219

Special notes for your reviewer:

Release note:

Fix the issue in kube-proxy iptables/ipvs mode to properly handle incorrect IP version.

@MrHohn MrHohn force-pushed the MrHohn:kube-proxy-ipv6-fix branch from a325f20 to 4c8edf0 Dec 6, 2017

@MrHohn MrHohn changed the title [WIP] Harden kube-proxy iptables mode for dual stack case Harden kube-proxy iptables mode for dual stack case Dec 6, 2017

@MrHohn MrHohn changed the title Harden kube-proxy iptables mode for dual stack case [WIP] Harden kube-proxy iptables mode for dual stack case Dec 6, 2017

@MrHohn MrHohn changed the title [WIP] Harden kube-proxy iptables mode for dual stack case Harden kube-proxy iptables mode for dual stack case Dec 6, 2017

@MrHohn

This comment has been minimized.

Copy link
Member Author

MrHohn commented Dec 6, 2017

/assign @thockin @bowei @freehan
/unassign

Will create the associated issue soon.

@k8s-ci-robot k8s-ci-robot assigned bowei, freehan and thockin and unassigned MrHohn Dec 6, 2017

@thockin
Copy link
Member

thockin left a comment

Overall OK. Can we make the eventing and logging less verbose by wrapping in a function?

@@ -56,3 +65,20 @@ func ShouldSkipService(svcName types.NamespacedName, service *api.Service) bool
}
return false
}

// This assumes ip is a valid IP.
func IsDualStackIP(ip string, isIPv6Mode bool) bool {

This comment has been minimized.

@thockin

thockin Dec 14, 2017

Member

This is an awkward API, to me. Maybe it's just the name. What if this were "IsCorrectIPVersion(ip string, expectIPv6 bool)" ? It's not really about dual-stack at all, but about not matching the expected version. Would eliminate "dual stack" from all the names

This comment has been minimized.

@MrHohn

MrHohn Dec 14, 2017

Author Member

Ack, I chose a wrong name. Will remove "dual stack"s.

This comment has been minimized.

@MrHohn

MrHohn Dec 15, 2017

Author Member

Done.

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Dec 14, 2017

@m1093782566 Do we need similar filters for ipvs mode?

@m1093782566

This comment has been minimized.

Copy link
Member

m1093782566 commented Dec 14, 2017

@thockin

Yes, we need it. I am willing to create another PR...

@@ -1550,11 +1553,69 @@ func Test_endpointsToEndpointsMap(t *testing.T) {
{endpoint: "1.1.1.1:22", isLocal: false},
},
},
}, {
// Case: should omit IPv6 address in IPv4 mode

This comment has been minimized.

@m1093782566

m1093782566 Dec 14, 2017

Member

nit: Can we add test case number, e.g. Case[9]: should omit IPv6 address in IPv4 mode?

This comment has been minimized.

@MrHohn

MrHohn Dec 15, 2017

Author Member

Done.

},
},
}, {
// Case: should omit IPv4 address in IPv6 mode

This comment has been minimized.

@m1093782566

m1093782566 Dec 14, 2017

Member

Same as above.

This comment has been minimized.

@MrHohn

MrHohn Dec 15, 2017

Author Member

Done.

desc string
newService *api.Service
isIPv6Mode bool
// This unit test only does verification on externalIPs and loadBalancerSourceRanges.

This comment has been minimized.

@m1093782566

m1093782566 Dec 14, 2017

Member

Why the test doesn't cover ClusterIP?

This comment has been minimized.

@MrHohn

MrHohn Dec 14, 2017

Author Member

Sorry, the comments are confusing, it does cover ClusterIP. Will clarify it a bit in comment,

This comment has been minimized.

@MrHohn

MrHohn Dec 15, 2017

Author Member

Done.

@MrHohn MrHohn changed the title Harden kube-proxy iptables mode for dual stack case Harden kube-proxy iptables mode for unmatched IP versions Dec 14, 2017

Name: service.Name,
Namespace: service.Namespace,
UID: service.UID,
}, v1.EventTypeWarning, "KubeProxyDualStackUnsupported", "Unsupported dual stack externalIP: %v", externalIP)

This comment has been minimized.

@MrHohn

MrHohn Dec 14, 2017

Author Member

Before I push the updated commit, one more thing to point out --- emitting event like this means if there are 1,000 nodes, we will have 1,000 events, given that the hostname in EventSource is different. Would this be a problem? Or I can seek for a way to remove the hostname source so that they can be deduped.

Example events as below:

25s        25s         1         my-nginx                             Service               Warning   KubeProxyIncorrectIPVersion   kube-proxy, e2e-test-XXX-minion-group-p1ch   Incorrect IP version in loadBalancerSourceRanges: 2600:1900::/35
25s        25s         1         my-nginx                             Service               Warning   KubeProxyIncorrectIPVersion   kube-proxy, e2e-test-XXX-minion-group-5qks   Incorrect IP version in loadBalancerSourceRanges: 2600:1900::/35
25s        25s         1         my-nginx                             Service               Warning   KubeProxyIncorrectIPVersion   kube-proxy, e2e-test-XXX-minion-group-dz4x   Incorrect IP version in loadBalancerSourceRanges: 2600:1900::/35

@MrHohn MrHohn force-pushed the MrHohn:kube-proxy-ipv6-fix branch from 4c8edf0 to c5219e9 Dec 15, 2017

@MrHohn

This comment has been minimized.

Copy link
Member Author

MrHohn commented Dec 15, 2017

Can we make the eventing and logging less verbose by wrapping in a function?

Wrapped some identical logging and eventing into a function.

Also pointing to the question on #56880 (comment) again as it somehow got folded.

@MrHohn

This comment has been minimized.

Copy link
Member Author

MrHohn commented Dec 15, 2017

/retest

@thockin
Copy link
Member

thockin left a comment

just nits

@@ -466,16 +487,33 @@ func NewProxier(ipt utiliptables.Interface,

if len(clusterCIDR) == 0 {
glog.Warningf("clusterCIDR not specified, unable to distinguish between internal and external traffic")
} else {
// Filter out the incorrect IP version case.

This comment has been minimized.

@thockin

thockin Dec 15, 2017

Member

Rather than filtering - this should probably be a real error. What do you think?

This comment has been minimized.

@MrHohn

MrHohn Dec 15, 2017

Author Member

Yes, this should be a real error. Done.

@@ -864,9 +902,16 @@ func endpointsToEndpointsMap(endpoints *api.Endpoints, hostname string) proxyEnd
glog.Warningf("ignoring invalid endpoint port %s with empty host", port.Name)
continue
}
// Filter out the incorrect IP version case.
if !utilproxy.CheckIPVersion(addr.IP, ecm.isIPv6Mode) {

This comment has been minimized.

@thockin

thockin Dec 15, 2017

Member

Maybe easier to read:

if utilproxy.IsIPv6(addr.IP) != ecm.isIPv6Mode {

Same at other callsites

This comment has been minimized.

@MrHohn

MrHohn Dec 15, 2017

Author Member

Good point, suggestion applied.

info.externalIPs = append(info.externalIPs, externalIP)
}
for _, sourceRange := range service.Spec.LoadBalancerSourceRanges {
if !utilproxy.CheckCIDRVersion(sourceRange, scm.isIPv6Mode) {

This comment has been minimized.

@thockin

thockin Dec 15, 2017

Member
if utilproxy.IsIPv6CIDR(sourceRange) != scm.isIPv6Mode {

This comment has been minimized.

@MrHohn

MrHohn Dec 15, 2017

Author Member

Applied.

@MrHohn MrHohn force-pushed the MrHohn:kube-proxy-ipv6-fix branch from c5219e9 to 877ed17 Dec 15, 2017

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Feb 27, 2018

/retest

@MrHohn MrHohn force-pushed the MrHohn:kube-proxy-ipv6-fix branch 2 times, most recently from 351b407 to c865460 Feb 27, 2018

@freehan

This comment has been minimized.

Copy link
Member

freehan commented Feb 27, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 27, 2018

@MrHohn

This comment has been minimized.

Copy link
Member Author

MrHohn commented Feb 27, 2018

Thanks! Rebasing now...

@MrHohn MrHohn force-pushed the MrHohn:kube-proxy-ipv6-fix branch from c865460 to 6004452 Feb 27, 2018

@k8s-github-robot k8s-github-robot removed the lgtm label Feb 27, 2018

@MrHohn

This comment has been minimized.

Copy link
Member Author

MrHohn commented Feb 27, 2018

@freehan Rebased PR.

@freehan

This comment has been minimized.

Copy link
Member

freehan commented Feb 27, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 27, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 27, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, freehan, MrHohn, 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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Feb 27, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@MrHohn

This comment has been minimized.

Copy link
Member Author

MrHohn commented Feb 27, 2018

/test pull-kubernetes-e2e-gce

@cblecker cblecker added this to the v1.10 milestone Feb 28, 2018

@cblecker

This comment has been minimized.

Copy link
Member

cblecker commented Feb 28, 2018

Added milestone as @bowei already approved it for the milestone

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Feb 28, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Feb 28, 2018

Automatic merge from submit-queue (batch tested with PRs 53689, 56880, 55856, 59289, 60249). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit f45f4a4 into kubernetes:master Feb 28, 2018

13 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation MrHohn authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit 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
You can’t perform that action at this time.