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

support ipv6 in bind address #76320

Merged
merged 1 commit into from Apr 27, 2019

Conversation

@JieJhih
Copy link
Member

commented Apr 9, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

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
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

/assign

@MrHohn
Copy link
Member

left a comment

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

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

// check ipv6

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 10, 2019

Member

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

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.

This comment has been minimized.

Copy link
@JieJhih

JieJhih Apr 10, 2019

Author Member

Good. I'm going to refactor this.
Thanks

@JieJhih

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

@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

@JieJhih
Copy link
Member Author

left a comment

Needs default value

if obj.HealthzBindAddress != "" {

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

This comment has been minimized.

Copy link
@JieJhih

JieJhih Apr 11, 2019

Author Member

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

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 11, 2019

Member

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

This comment has been minimized.

Copy link
@JieJhih

JieJhih Apr 12, 2019

Author Member

Updated.
Thanks

if obj.MetricsBindAddress != "" {

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

This comment has been minimized.

Copy link
@JieJhih

JieJhih Apr 11, 2019

Author Member

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

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 11, 2019

Member

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

@JieJhih

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

/retest

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

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

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 11, 2019

Member

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

This comment has been minimized.

Copy link
@JieJhih

JieJhih Apr 12, 2019

Author Member

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

bindAddress = fmt.Sprintf("%s:%d", host, o.metricsPort)
}

if tag == "ipv6" {

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 11, 2019

Member

Using switch might make this cleaner.

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

o.config.HealthzBindAddress = bindAddress
}

func (o *Options) applyDeprecatedMetricsPortToConfig() {

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 11, 2019

Member

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

This comment has been minimized.

Copy link
@JieJhih

JieJhih Apr 12, 2019

Author Member

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

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

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 11, 2019

Member

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.

This comment has been minimized.

Copy link
@JieJhih

JieJhih Apr 12, 2019

Author Member

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

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 12, 2019

Member

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.

This comment has been minimized.

Copy link
@JieJhih

JieJhih Apr 12, 2019

Author Member

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/

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 12, 2019

Member

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.

This comment has been minimized.

Copy link
@JieJhih

JieJhih Apr 13, 2019

Author Member

LGTM
Thanks

This comment has been minimized.

Copy link
@JieJhih

JieJhih Apr 13, 2019

Author Member

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

@MrHohn

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

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?

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

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 12, 2019

Member

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

type AddressType string

const (
    AddressTypeIPv4 = "ipv4"
    AddressTypeIPv6 = "ipv6"
)
}

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

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 12, 2019

Member

Read better if we return earlier in the error case:

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

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

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 12, 2019

Member

You are still duplicating above logic in two places :)

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

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 12, 2019

Member

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.

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

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 12, 2019

Member

It might be unnecessary to add this map complication.

@JieJhih

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

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

@MrHohn
Copy link
Member

left a comment

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.

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

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 15, 2019

Member

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:
// ...

This comment has been minimized.

Copy link
@JieJhih

JieJhih Apr 16, 2019

Author Member

Updated.
Thanks

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

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 15, 2019

Member

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

}
}

func validateHostPort(input string) error {

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 15, 2019

Member

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?

}
}

func TestApplyDeprecatedMetricsPortToConfig(t *testing.T) {

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 15, 2019

Member

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

},
}

for _, tc := range testCases {

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 15, 2019

Member

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

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 15, 2019

Member

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

This comment has been minimized.

Copy link
@JieJhih

JieJhih Apr 16, 2019

Author Member

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

This comment has been minimized.

Copy link
@JieJhih

JieJhih Apr 16, 2019

Author Member

Also I remove TestApplyDeprecatedMetricsPortToConfig and TestApplyDeprecatedHealthzPortToConfig change into TestAddressFromDeprecatedFlags.


// 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 {

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 15, 2019

Member

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

This comment has been minimized.

Copy link
@JieJhih

JieJhih Apr 16, 2019

Author Member

Done.
Thanks

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

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

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 15, 2019

Member

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]"
}

This comment has been minimized.

Copy link
@JieJhih

JieJhih Apr 16, 2019

Author Member

Updated.
Thanks

@MrHohn
MrHohn approved these changes Apr 16, 2019
Copy link
Member

left a comment

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

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

// AddressType defined IP address ipv4 or ipv6

This comment has been minimized.

Copy link
@MrHohn

MrHohn Apr 16, 2019

Member

Please remove the unused codes.

This comment has been minimized.

Copy link
@JieJhih

JieJhih Apr 17, 2019

Author Member

Updated.
Thanks

support ipv6 in bind address
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

@JieJhih JieJhih force-pushed the JieJhih:config/kube_proxy branch from 7b37fb6 to 08e320f Apr 17, 2019

@JieJhih

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

@MrHohn Squashed and pushed, thanks for your review.

@dcbw

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

/retest

@MrHohn

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Thanks!
/lgtm

/assign @thockin
for approval.

@dims

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

/assign @thockin

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

@dims

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

/milestone v1.15

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

@thockin
Copy link
Member

left a comment

Thanks!

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

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

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

@thockin

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

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

@MrHohn

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

@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 k8s-ci-robot merged commit 4dc05dd into kubernetes:master Apr 27, 2019

20 checks passed

cla/linuxfoundation JieJhih authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
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-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@JieJhih JieJhih referenced this pull request May 19, 2019
6 of 6 tasks complete
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.