Harden kube-proxy for unmatched IP versions #56880

MrHohn · 2017-12-06T05:36:38Z

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 · 2017-12-06T20:00:50Z

/assign @thockin @bowei @freehan
/unassign

Will create the associated issue soon.

thockin

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

thockin · 2017-12-14T05:46:52Z

pkg/proxy/util/utils.go

@@ -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 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

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

thockin · 2017-12-14T05:52:33Z

@m1093782566 Do we need similar filters for ipvs mode?

m1093782566 · 2017-12-14T06:02:38Z

@thockin

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

m1093782566 · 2017-12-14T06:13:07Z

pkg/proxy/iptables/proxier_test.go

@@ -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


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

m1093782566 · 2017-12-14T06:13:23Z

pkg/proxy/iptables/proxier_test.go

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


Same as above.

m1093782566 · 2017-12-14T06:27:08Z

pkg/proxy/iptables/proxier_test.go

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


Why the test doesn't cover ClusterIP?

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

MrHohn · 2017-12-14T19:50:41Z

pkg/proxy/iptables/proxier.go

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


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 · 2017-12-15T02:00:39Z

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 · 2017-12-15T02:37:07Z

/retest

thockin

just nits

thockin · 2017-12-15T05:31:30Z

pkg/proxy/iptables/proxier.go

@@ -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.


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

Yes, this should be a real error. Done.

thockin · 2017-12-15T05:36:32Z

pkg/proxy/iptables/proxier.go

@@ -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) {


Maybe easier to read:

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

Same at other callsites

Good point, suggestion applied.

thockin · 2017-12-15T05:39:58Z

pkg/proxy/iptables/proxier.go

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


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

BenTheElder · 2018-02-27T02:57:18Z

/retest

freehan · 2018-02-27T18:34:11Z

/lgtm

MrHohn · 2018-02-27T18:38:27Z

Thanks! Rebasing now...

…ions

MrHohn · 2018-02-27T20:23:04Z

@freehan Rebased PR.

freehan · 2018-02-27T21:51:53Z

/lgtm

k8s-ci-robot · 2018-02-27T21:52:01Z

[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:

~~pkg/OWNERS~~ [thockin]

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 · 2018-02-27T22:15:34Z

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

MrHohn · 2018-02-27T22:57:34Z

/test pull-kubernetes-e2e-gce

cblecker · 2018-02-28T04:01:40Z

Added milestone as @bowei already approved it for the milestone

k8s-github-robot · 2018-02-28T04:34:27Z

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

k8s-github-robot · 2018-02-28T08:00:27Z

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-ci-robot assigned MrHohn Dec 6, 2017

MrHohn force-pushed the kube-proxy-ipv6-fix branch from a325f20 to 4c8edf0 Compare December 6, 2017 19:45

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

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 6, 2017

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

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 6, 2017

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

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 6, 2017

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

thockin reviewed Dec 14, 2017

View reviewed changes

m1093782566 reviewed Dec 14, 2017

View reviewed changes

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

MrHohn commented Dec 14, 2017

View reviewed changes

MrHohn force-pushed the kube-proxy-ipv6-fix branch from 4c8edf0 to c5219e9 Compare December 15, 2017 01:48

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Dec 15, 2017

thockin reviewed Dec 15, 2017

View reviewed changes

MrHohn force-pushed the kube-proxy-ipv6-fix branch from c5219e9 to 877ed17 Compare December 15, 2017 06:56

MrHohn mentioned this pull request Feb 27, 2018

Better way to handle unmatched IP versions on service and endpoint #60471

Closed

MrHohn force-pushed the kube-proxy-ipv6-fix branch from f3b9a03 to 9d64f94 Compare February 27, 2018 02:52

MrHohn force-pushed the kube-proxy-ipv6-fix branch 2 times, most recently from 351b407 to c865460 Compare February 27, 2018 04:13

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2018

MrHohn added 8 commits February 27, 2018 11:05

[kube-proxy] Move Service/EndpointInfo common codes to change tracker

b485f7b

[kube-proxy] Move ipv6 related funcs to utils pkg

dfbec1a

[kube-proxy] Add more IP version related utils

ca03252

[kube-proxy] Make the import name of utilproxy consistent

2ae45e9

[kube-proxy] Harden change tracker and proxiers for unmatched IP vers…

95cde4f

…ions

[kube-proxy] Unit test for unmatched IP version

0606449

[kube-proxy] Mass service/endpoint info functions rename and comments

f6eed81

Auto-updated BUILD files

6004452

MrHohn force-pushed the kube-proxy-ipv6-fix branch from c865460 to 6004452 Compare February 27, 2018 19:18

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2018

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

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

danwinship mentioned this pull request Apr 5, 2019

enforce the interface relationship between ServicePort and BaseServiceInfo #76160

Merged

Harden kube-proxy for unmatched IP versions #56880

Harden kube-proxy for unmatched IP versions #56880

Conversation

MrHohn commented Dec 6, 2017 • edited Loading

MrHohn commented Dec 6, 2017

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Dec 14, 2017

m1093782566 commented Dec 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrHohn commented Dec 15, 2017

MrHohn commented Dec 15, 2017

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder commented Feb 27, 2018

freehan commented Feb 27, 2018

MrHohn commented Feb 27, 2018

MrHohn commented Feb 27, 2018

freehan commented Feb 27, 2018

k8s-ci-robot commented Feb 27, 2018

k8s-github-robot commented Feb 27, 2018

MrHohn commented Feb 27, 2018

cblecker commented Feb 28, 2018

k8s-github-robot commented Feb 28, 2018

k8s-github-robot commented Feb 28, 2018

MrHohn commented Dec 6, 2017 •

edited

Loading