support ipv6 in bind address #76320

LexCC · 2019-04-09T07:34:38Z

What type of PR is this?

/kind bug

What this PR does / why we need it:
validateHostPort return error(must be IP:port) when using IPv6 addressing for HealthzBindAddress and MetricsBindAddress.

Which issue(s) this PR fixes:

Fixes #76289

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

kube-proxy: HealthzBindAddress and MetricsBindAddress support ipv6 address.

k8s-ci-robot · 2019-04-09T07:34:45Z

Hi @jiejhih. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

neolit123

this change needs unit tests.
also please add a release note (user facing change) as per the PR template.

/sig network
/ok-to-test
/priority backlog
/kind cleanup

MrHohn · 2019-04-10T17:59:07Z

/assign

MrHohn

Thanks for starting the fix. This is known to contain bugs and we wanted to improve it.

MrHohn · 2019-04-10T22:14:43Z

cmd/kube-proxy/app/server.go

@@ -369,6 +370,14 @@ func (o *Options) applyDeprecatedHealthzPortToConfig() {
 		return
 	}

+	// check ipv6


While you are here, do you mind somehow refactoring this out and reuse it on SetDefaults as well?

kubernetes/pkg/proxy/apis/config/v1alpha1/defaults.go

Lines 36 to 49 in 37e6266

func SetDefaults_KubeProxyConfiguration(obj *kubeproxyconfigv1alpha1.KubeProxyConfiguration) {

if len(obj.BindAddress) == 0 {

obj.BindAddress = "0.0.0.0"

}

if obj.HealthzBindAddress == "" {

obj.HealthzBindAddress = fmt.Sprintf("0.0.0.0:%v", ports.ProxyHealthzPort)

} else if !strings.Contains(obj.HealthzBindAddress, ":") {

obj.HealthzBindAddress += fmt.Sprintf(":%v", ports.ProxyHealthzPort)

}

if obj.MetricsBindAddress == "" {

obj.MetricsBindAddress = fmt.Sprintf("127.0.0.1:%v", ports.ProxyStatusPort)

} else if !strings.Contains(obj.MetricsBindAddress, ":") {

obj.MetricsBindAddress += fmt.Sprintf(":%v", ports.ProxyStatusPort)

}

You change current only has impact on flags, which are deprecated.

Good. I'm going to refactor this.
Thanks

LexCC · 2019-04-11T09:59:15Z

@MrHohn I have a question , GetBindAddressHostPort method now I write under both pkg/proxy/apis/config/v1alpha1 and cmd/kube-proxy/app package. Cause I'm new here, I'm still researching all the struct. I wanna write this method for KubeProxyConfiguration struct. Although it seems that two packages both have KubeProxyConfiguration struct, but they are in a different namespace? so they can't share a GetBindAddressHostPort func

LexCC

Needs default value

LexCC · 2019-04-11T09:41:45Z

pkg/proxy/apis/config/v1alpha1/defaults.go

+		if obj.HealthzBindAddress != "" {
+
+			if healthzHost == "" {
+				obj.HealthzBindAddress = fmt.Sprintf("[::0]:%v", ports.ProxyHealthzPort) // set ipv6 default value


@MrHohn What's the default value should I set.

For IPv4 it is 0.0.0.0, we should use the equivalent :: then?

Updated.
Thanks

LexCC · 2019-04-11T09:42:11Z

pkg/proxy/apis/config/v1alpha1/defaults.go

+		if obj.MetricsBindAddress != "" {
+
+			if metricsHost == "" {
+				obj.MetricsBindAddress = fmt.Sprintf("[::0]:%v", ports.ProxyStatusPort) // set ipv6 default value


@MrHohn What's the default value should I set.

For IPv4 it is 127.0.0.1, we should use the equivalent ::1 then?

LexCC · 2019-04-11T13:35:37Z

/retest

MrHohn · 2019-04-11T18:28:56Z

cmd/kube-proxy/app/server.go

@@ -357,6 +358,31 @@ func (o *Options) writeConfigFile() error {
 	return nil
 }

+func GetBindAddressHostPort(address string, bindPort int) (string, string, string, error) {


Need a comment for exported function - what is the purpose and what it returns?

It will return host、port、tag(ipv4 or ipv6)、error
The param bindPortis used to set a default value or the port you really want to bind when the address does not contain the port.
https://github.com/kubernetes/kubernetes/pull/76320/files#diff-48f6b24dd0fd4d66bfb6caa7523e11ebR220

MrHohn · 2019-04-11T18:31:56Z

cmd/kube-proxy/app/server.go

+		bindAddress = fmt.Sprintf("%s:%d", host, o.metricsPort)
+	}
+
+	if tag == "ipv6" {


Using switch might make this cleaner.

MrHohn · 2019-04-11T18:38:24Z

cmd/kube-proxy/app/server.go

+		bindAddress = fmt.Sprintf("[%s]:%d", host, o.healthzPort)
+	}
+
+	o.config.HealthzBindAddress = bindAddress
 }

 func (o *Options) applyDeprecatedMetricsPortToConfig() {


Would be great to dedup applyDeprecatedMetricsPortToConfig() and applyDeprecatedHealthzPortToConfig() --- they look almost the same :)

It can use o.applyDeprecatedBindAddressPortToConfig("healthz", "metrics") to handle both method now.
https://github.com/kubernetes/kubernetes/pull/76320/files#diff-cf93bbed37202b7d58b5841f07ddd89fR412

MrHohn · 2019-04-11T18:43:55Z

pkg/proxy/apis/config/v1alpha1/defaults.go

-		obj.HealthzBindAddress = fmt.Sprintf("0.0.0.0:%v", ports.ProxyHealthzPort)
-	} else if !strings.Contains(obj.HealthzBindAddress, ":") {
-		obj.HealthzBindAddress += fmt.Sprintf(":%v", ports.ProxyHealthzPort)
+	// ipv6


Humm..The ipv6 and ipv4 block looks fairly complicated. It also seems like we are duplicating the logic to handle HealthzBindAddress and MetricsBindAddress. We should extract out the common part and reuse if possible.

Putting such logic into a func would also help unit testing.

It just considers ipv4 format originally.
Now we have ipv6 format have to check.
As the following link that can make sure BindAddress HealthzBindAddress MetricsBindAddress format are ipv4 or ipv6.
But there are some unit tests failed, I'm not sure is that some test cases I haven't thought
https://github.com/kubernetes/kubernetes/pull/76320/files#diff-f1bfd0a64c0e2e6163dfbf23b47abe93R44

I haven't looked closely, just adding a quick note that some tests are failing because the bazel file is not updated (https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/76320/pull-kubernetes-verify/1116634758399397889). You might need to run ./hack/update-bazel.sh.

Good. It fixes almost.
Now only pull-kubernetes-e2e-gce is left.
Thanks for the help.
https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/76320/pull-kubernetes-e2e-gce/1116758947001274377/

Took another look, it still seems to me this is more complicated than it needs to be. I gave it a try on MrHohn@d576643. Do you mind taking a look if that works for you? Thanks.

LGTM
Thanks

I make a little change. It should give ipv6 format when bindAddress is ipv6, that is one thing the PR have to refactor.

MrHohn · 2019-04-11T18:44:49Z

Although it seems that two packages both have KubeProxyConfiguration struct, but they are in a different namespace? so they can't share a GetBindAddressHostPort func

@jiejhih I would recommend putting the func you wanted to share into a util package, and import it in both pkg/proxy/apis/config/v1alpha1 and cmd/kube-proxy/app. Maybe https://github.com/kubernetes/kubernetes/blob/master/pkg/proxy/util/utils.go?

MrHohn · 2019-04-12T17:38:40Z

pkg/proxy/util/utils.go

@@ -214,3 +215,29 @@ func filterWithCondition(strs []string, expectedCondition bool, conditionFunc fu
 	}
 	return corrects, incorrects
 }
+
+// GetBindAddressHostPort parse IP to host, port, tag=ipv4 or ipv6


Would be more clear to make ipv4 and ipv6 a type.

type AddressType string const ( AddressTypeIPv4 = "ipv4" AddressTypeIPv6 = "ipv6" )

MrHohn · 2019-04-12T17:41:23Z

pkg/proxy/util/utils.go

+	}
+
+	host, port, err := net.SplitHostPort(address)
+	if err == nil {


Read better if we return earlier in the error case:

host, port, err := net.SplitHostPort(address) if err != nil { return "", "", tag, err } ...

MrHohn · 2019-04-12T17:50:06Z

cmd/kube-proxy/app/server.go

+	case "ipv4":
+		bindAddress = fmt.Sprintf("%s:%d", host, o.healthzPort)
+	case "ipv6":
+		bindAddress = fmt.Sprintf("[%s]:%d", host, o.healthzPort)


You are still duplicating above logic in two places :)

MrHohn · 2019-04-12T23:17:11Z

pkg/proxy/apis/config/v1alpha1/defaults.go

-		obj.HealthzBindAddress = fmt.Sprintf("0.0.0.0:%v", ports.ProxyHealthzPort)
-	} else if !strings.Contains(obj.HealthzBindAddress, ":") {
-		obj.HealthzBindAddress += fmt.Sprintf(":%v", ports.ProxyHealthzPort)
+	// ipv6


Took another look, it still seems to me this is more complicated than it needs to be. I gave it a try on MrHohn@d576643. Do you mind taking a look if that works for you? Thanks.

MrHohn · 2019-04-12T23:18:23Z

cmd/kube-proxy/app/server.go

+	deprecatedMap = map[string]func(){
+		"healthz": o.applyDeprecatedHealthzPortToConfig,
+		"metrics": o.applyDeprecatedMetricsPortToConfig,
+	}


It might be unnecessary to add this map complication.

LexCC · 2019-04-15T05:11:17Z

@MrHohn PTAL.
Is the code looks good now?
Thanks.

MrHohn

Thanks for the works and sorry for adding a couple more comments. Since this is API related, I wanted to keep the codes as readable and maintainable as possible.

MrHohn · 2019-04-15T18:30:26Z

cmd/kube-proxy/app/server.go

-	if o.healthzPort == 0 {
-		o.config.HealthzBindAddress = ""
-		return
+// 1. If port is 0, disable the server (e.g. set address to empty).


Would be great to preserve some of the original comments:

// addressFromDeprecatedFlags returns server address from flags // passed on the command line based on the following rules: // ...

Updated.
Thanks

MrHohn · 2019-04-15T18:35:26Z

cmd/kube-proxy/app/server_test.go

+			healthzBindAddress: "[fd00:1::5]:12345",
+		},
+		{
+			name:               "ipvs mode, IPv6 config",


How is this ipvs mode configured? I only saw it in test name?

MrHohn · 2019-04-15T19:45:12Z

cmd/kube-proxy/app/server_test.go

+	}
+}
+
+func validateHostPort(input string) error {


Humm.. It feels a bit odd to have such logic in test. Can we simply hardcode the expected result instead of adding these validation logic, which seems irrelevant to what we are testing?

MrHohn · 2019-04-15T19:45:53Z

cmd/kube-proxy/app/server_test.go

+	}
+}
+
+func TestApplyDeprecatedMetricsPortToConfig(t *testing.T) {


This test is identical to the above test case --- I think having one TestApplyDeprecatedPortToConfig would be sufficient.

MrHohn · 2019-04-15T19:48:37Z

cmd/kube-proxy/app/server_test.go

+		},
+	}
+
+	for _, tc := range testCases {


I don't see value from deprecated flags get passed in? How is that evaluated?

To ensure test coverage but make this simple, maybe simply calling addressFromDeprecatedFlags() in test and make sure it does what we want?

I use Complete() func originally, now I write TestAddressFromDeprecatedFlags that simply calling addressFromDeprecatedFlags().

Also I remove TestApplyDeprecatedMetricsPortToConfig and TestApplyDeprecatedHealthzPortToConfig change into TestAddressFromDeprecatedFlags.

MrHohn · 2019-04-15T19:49:09Z

pkg/proxy/util/utils.go

+
+// AppendPortIfNeeded appends the given port to IP address unless it is already in
+// "ipv4:port" or "[ipv6]:port" format.
+func AppendPortIfNeeded(addr string, port int32) string {


Would be great to have a unit test for this helper func :)

Done.
Thanks

MrHohn · 2019-04-15T20:16:24Z

pkg/proxy/apis/config/v1alpha1/defaults.go

@@ -34,19 +38,37 @@ func addDefaultingFuncs(scheme *kruntime.Scheme) error {
 }

 func SetDefaults_KubeProxyConfiguration(obj *kubeproxyconfigv1alpha1.KubeProxyConfiguration) {
+	var tag proxyutil.AddressType


Since now all the logic seems to be self-contained in this file, we could avoid adding a new type (sorry for being back and forth on this). What about:

... defaultHealthzAddress, defaultMetricsAddress := getDefaultAddresses(obj.BindAddress) if obj.HealthzBindAddress == "" { obj.HealthzBindAddress = fmt.Sprintf("%s:%d", defaultHealthzAddress, ports.ProxyHealthzPort) } else { obj.HealthzBindAddress = proxyutil.AppendPortIfNeeded(obj.HealthzBindAddress, ports.ProxyHealthzPort) } if obj.MetricsBindAddress == "" { obj.MetricsBindAddress = fmt.Sprintf("%s:%d", defaultMetricsAddress, ports.ProxyStatusPort) } else { obj.MetricsBindAddress = proxyutil.AppendPortIfNeeded(obj.MetricsBindAddress, ports.ProxyStatusPort) } ... // getDefaultAddresses returns default address of healthz and metrics server // based on the given bind address. IPv6 addresses are enclosed in square // brackets for appending port. func getDefaultAddresses(bindAddress string) (defaultHealthzAddress, defaultMetricsAddress string) { if net.ParseIP(bindAddress).To4() != nil { return "0.0.0.0", "127.0.0.1" } return "[::]", "[::1]" }

Updated.
Thanks

MrHohn

Thanks! LGTM with one comment. Please squash the commits.

MrHohn · 2019-04-16T23:48:04Z

pkg/proxy/util/utils.go

@@ -32,6 +32,14 @@ import (
 	"k8s.io/klog"
 )

+// AddressType defined IP address ipv4 or ipv6


Please remove the unused codes.

Updated.
Thanks

use split host port func instead trim specific character add unit test for metrics and healthz bind address recover import package refactor set default kube proxy configuration fix ipv4 condition fix set default port condition rewrite call function occasion to reduce error set ipv6 default value move get GetBindAddressHostPort to util use one func to handle deprecated series update bazel define address type return earlier in the error case refactor set default kube proxy configuration logic recover import package preserve some of the original comments add get default address func add append port if needed unit test rewrite unit test for deprecated flags remove unused codes

LexCC · 2019-04-17T07:38:19Z

@MrHohn Squashed and pushed, thanks for your review.

dcbw · 2019-04-17T14:11:57Z

/retest

MrHohn · 2019-04-17T17:31:13Z

Thanks!
/lgtm

/assign @thockin
for approval.

dims · 2019-04-23T16:44:01Z

/assign @thockin

@thockin this looks ready and has lgtm(s), can you please look?

dims · 2019-04-23T16:46:25Z

/milestone v1.15

thockin

Thanks!

/lgtm
/approve

k8s-ci-robot · 2019-04-26T20:18:45Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~cmd/OWNERS~~ [thockin]
~~pkg/proxy/OWNERS~~ [thockin]

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

thockin · 2019-04-26T20:19:40Z

@MrHohn This should not need my approval - if you are not in OWNERS you probably should be?

MrHohn · 2019-04-26T21:45:20Z

@MrHohn This should not need my approval - if you are not in OWNERS you probably should be?

Will send a PR for the cmd/kube-proxy part :)

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 9, 2019

k8s-ci-robot requested review from mikedanese and thockin April 9, 2019 07:36

neolit123 reviewed Apr 9, 2019

View reviewed changes

k8s-ci-robot assigned MrHohn Apr 10, 2019

MrHohn reviewed Apr 10, 2019

View reviewed changes

LexCC commented Apr 11, 2019

View reviewed changes

LexCC force-pushed the config/kube_proxy branch from 42fc00d to 81fb347 Compare April 11, 2019 16:49

MrHohn mentioned this pull request Apr 11, 2019

kube-proxy: Issue with IPv6 bind addresses for metrics and healthz. #76289

Closed

MrHohn reviewed Apr 11, 2019

View reviewed changes

MrHohn suggested changes Apr 12, 2019

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 15, 2019

MrHohn reviewed Apr 15, 2019

View reviewed changes

MrHohn approved these changes Apr 16, 2019

View reviewed changes

LexCC force-pushed the config/kube_proxy branch from 7b37fb6 to 08e320f Compare April 17, 2019 07:34

k8s-ci-robot assigned thockin Apr 17, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 17, 2019

k8s-ci-robot added this to the v1.15 milestone Apr 23, 2019

thockin reviewed Apr 26, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 26, 2019

k8s-ci-robot merged commit 4dc05dd into kubernetes:master Apr 27, 2019

MrHohn mentioned this pull request Apr 27, 2019

Add OWNERS file for cmd/kube-proxy #77153

Merged

LexCC mentioned this pull request May 19, 2019

REQUEST: New membership for JieJhih kubernetes/org#833

Closed

6 tasks

	func SetDefaults_KubeProxyConfiguration(obj *kubeproxyconfigv1alpha1.KubeProxyConfiguration) {
	if len(obj.BindAddress) == 0 {
	obj.BindAddress = "0.0.0.0"
	}
	if obj.HealthzBindAddress == "" {
	obj.HealthzBindAddress = fmt.Sprintf("0.0.0.0:%v", ports.ProxyHealthzPort)
	} else if !strings.Contains(obj.HealthzBindAddress, ":") {
	obj.HealthzBindAddress += fmt.Sprintf(":%v", ports.ProxyHealthzPort)
	}
	if obj.MetricsBindAddress == "" {
	obj.MetricsBindAddress = fmt.Sprintf("127.0.0.1:%v", ports.ProxyStatusPort)
	} else if !strings.Contains(obj.MetricsBindAddress, ":") {
	obj.MetricsBindAddress += fmt.Sprintf(":%v", ports.ProxyStatusPort)
	}

support ipv6 in bind address #76320

support ipv6 in bind address #76320

Conversation

LexCC commented Apr 9, 2019 • edited

k8s-ci-robot commented Apr 9, 2019

neolit123 left a comment

Choose a reason for hiding this comment

MrHohn commented Apr 10, 2019

MrHohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LexCC commented Apr 11, 2019

LexCC 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

LexCC commented Apr 11, 2019

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

LexCC Apr 12, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LexCC Apr 12, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrHohn Apr 12, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LexCC Apr 13, 2019 • edited

Choose a reason for hiding this comment

MrHohn commented Apr 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrHohn Apr 12, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LexCC commented Apr 15, 2019

MrHohn 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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LexCC commented Apr 17, 2019

dcbw commented Apr 17, 2019

MrHohn commented Apr 17, 2019

dims commented Apr 23, 2019

dims commented Apr 23, 2019

thockin left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 26, 2019

thockin commented Apr 26, 2019

MrHohn commented Apr 26, 2019

LexCC commented Apr 9, 2019 •

edited

LexCC Apr 12, 2019 •

edited

LexCC Apr 12, 2019 •

edited

MrHohn Apr 12, 2019 •

edited

LexCC Apr 13, 2019 •

edited

MrHohn Apr 12, 2019 •

edited