Allow hostname-override arg to be used if specified #69340

stevesloka · 2018-10-02T19:03:12Z

What this PR does / why we need it:

Currentlykube-proxy loads configuration from a config file. There are cases where the hostname needs to be overridden (See: kubernetes/kubeadm#857).

There is a flag that allows the hostname to be overridden (hostname-override), but when the config file is loaded, if this arg is set, it's overridden by whatever the config file contains making the argument ignored if it was set.

This PR checks for this argument and uses it if it is passed to kube-proxy.

Which issue(s) this PR fixes:
Fixes #57518
Fixes kubernetes/kubeadm#857

Special notes for your reviewer:

Release note:

kube-proxy argument `hostname-override` can be used to override hostname defined in the configuration file

// @timothysc

Signed-off-by: Steve Sloka [email protected]

ncdc · 2018-10-02T19:15:05Z

cmd/kube-proxy/app/server.go

 )

+var (
+	hostNameOverride string


If we decide to proceed with this one-off approach for hostnameOverride, let's make this an unexported field in Options.

cc @mtaufen

ncdc · 2018-10-02T19:15:57Z

cmd/kube-proxy/app/server.go

 			if err := opts.Complete(); err != nil {
 				glog.Fatalf("failed complete: %v", err)
 			}
+			if err := opts.ProcessArgs(); err != nil {


Instead of adding a new top-level function to the complete/validate/run flow, I'd recommend renaming ProcessArgs() to processHostnameOverrideFlag() and calling it from within Complete()

ncdc · 2018-10-02T19:16:23Z

cmd/kube-proxy/app/server_test.go

+
+// TestProcessArgs tests processing args
+func TestProcessArgs(t *testing.T) {
+


Please remove

This has been renamed to match the code it is testing.

... the blank line

ncdc · 2018-10-02T19:17:07Z

cmd/kube-proxy/app/server_test.go

+	testCases := []struct {
+		name        string
+		hostnameArg string
+		expHostname string


exp->expected

ncdc · 2018-10-02T19:17:37Z

cmd/kube-proxy/app/server.go

also: if err := ...; err != nil {

ncdc · 2018-10-02T19:18:21Z

cmd/kube-proxy/app/server.go

stevesloka · 2018-10-02T19:33:54Z

@ncdc I addressed your comments, thanks!

ncdc · 2018-10-02T19:37:07Z

cmd/kube-proxy/app/server.go

I would call this hostnameOverride

ncdc · 2018-10-02T19:40:58Z

cmd/kube-proxy/app/server.go

How about something like

hostnameOverride, if set from the command line flag, takes precedence over the `HostnameOverride` value from the config file

ncdc · 2018-10-02T19:41:39Z

cmd/kube-proxy/app/server.go

argument -> flag

ncdc · 2018-10-02T19:42:53Z

cmd/kube-proxy/app/server_test.go

Is this needed?

no not needed, removing

ncdc · 2018-10-02T19:43:58Z

cmd/kube-proxy/app/server_test.go

hostnameArg -> hostnameOverrideFlag

ncdc · 2018-10-02T19:44:06Z

cmd/kube-proxy/app/server_test.go

argument -> flag

ncdc · 2018-10-02T19:45:05Z

cmd/kube-proxy/app/server_test.go

Use a go subtest:

t.Run(tc.name, func(t *testing.T) { // body of test case }

ncdc · 2018-10-02T19:45:27Z

cmd/kube-proxy/app/server_test.go

When you switch to using a subtest, you can remove the initial %s for tc.name

ncdc · 2018-10-02T19:45:31Z

cmd/kube-proxy/app/server_test.go

When you switch to using a subtest, you can remove the initial %s for tc.name

timothysc · 2018-10-02T20:02:58Z

/ok-to-test
/sig networking
/sig clusterlifecycle

/cc @kubernetes/sig-network-pr-reviews

Signed-off-by: Steve Sloka <[email protected]>

stevesloka · 2018-10-02T20:51:58Z

/retest

timothysc · 2018-10-11T18:32:10Z

@kubernetes/sig-network-pr-reviews - re-ping!
/assign @thockin

thockin · 2018-10-11T23:49:33Z

cmd/kube-proxy/app/server.go


+// processHostnameOverrideFlag processes hostname-override flag
+func (o *Options) processHostnameOverrideFlag() error {
+	// Check if hostname-override flag is set and use value since configFile always overrides


I thought the flag would always override - the flag reads into the config struct..

@mtaufen

@thockin I did kube-proxy first, as a POC, and it got merged well before all the docs/discussions on the proper way to do things going forward. I didn't realize that there would be a need for per-instance configuration settings as well as those that are shared across all instances. The way kube-proxy specifically works, today, is that it's completely either-or. Either you use a config file, or you use flags. Flags do not override the config file.

thockin · 2018-10-16T16:36:47Z

/lgtm
/approve

k8s-ci-robot · 2018-10-16T16:36:54Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~cmd/OWNERS~~ [thockin]

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

mtaufen · 2018-10-16T18:57:17Z

You may find that this becomes a general problem.
In the Kubelet we did a general thing to make flags take precedence over config files.
I'm not saying you should exactly emulate the way the Kubelet does the general thing, because there are likely cleaner bootstrap approaches that enable flags to take precedence, but if you notice this kind of thing keeps happening, you may want to consider a more general approach.

stevesloka · 2018-10-16T19:07:40Z

@mtaufen yes this came up with discussions with @ncdc and others. I am going to look to better implement these flags in a future PR, but might need some further discussion.

anguslees · 2018-11-05T04:43:19Z

(fwiw, the --{bind-address,metrics-bind-address,healthz-bind-address} flags are further examples that are probably going to be given node-specific values via flags rather than configured in the cluster-wide config)

k8s-ci-robot requested review from dchen1107 and mikedanese October 2, 2018 19:03

stevesloka force-pushed the fixHostNameOverride branch from d1c4741 to 3f87bd7 Compare October 2, 2018 19:17

ncdc suggested changes Oct 2, 2018

View reviewed changes

stevesloka force-pushed the fixHostNameOverride branch 2 times, most recently from 5102ffa to 7b9e797 Compare October 2, 2018 19:32

ncdc reviewed Oct 2, 2018

View reviewed changes

cmd/kube-proxy/app/server.go Outdated

Copy link

Member

ncdc Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would call this hostnameOverride

ncdc reviewed Oct 2, 2018

View reviewed changes

cmd/kube-proxy/app/server.go Outdated

Copy link

Member

ncdc Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

argument -> flag

ncdc reviewed Oct 2, 2018

View reviewed changes

cmd/kube-proxy/app/server_test.go Outdated

Copy link

Member

ncdc Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hostnameArg -> hostnameOverrideFlag

ncdc reviewed Oct 2, 2018

View reviewed changes

cmd/kube-proxy/app/server_test.go Outdated

Copy link

Member

ncdc Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

argument -> flag

ncdc reviewed Oct 2, 2018

View reviewed changes

cmd/kube-proxy/app/server_test.go Outdated

Copy link

Member

ncdc Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a go subtest:

t.Run(tc.name, func(t *testing.T) { // body of test case }

ncdc reviewed Oct 2, 2018

View reviewed changes

cmd/kube-proxy/app/server_test.go Outdated

Copy link

Member

ncdc Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you switch to using a subtest, you can remove the initial %s for tc.name

ncdc reviewed Oct 2, 2018

View reviewed changes

cmd/kube-proxy/app/server_test.go Outdated

Copy link

Member

ncdc Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you switch to using a subtest, you can remove the initial %s for tc.name

k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 2, 2018

Allow hostname-override flag to be used if specified

5834f94

Signed-off-by: Steve Sloka <[email protected]>

stevesloka force-pushed the fixHostNameOverride branch from 7b9e797 to 5834f94 Compare October 2, 2018 20:09

timothysc added the kind/bug Categorizes issue or PR as related to a bug. label Oct 11, 2018

k8s-ci-robot removed the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Oct 11, 2018

k8s-ci-robot assigned thockin Oct 11, 2018

thockin reviewed Oct 11, 2018

View reviewed changes

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 16, 2018

k8s-ci-robot merged commit c2d9321 into kubernetes:master Oct 16, 2018

stevesloka deleted the fixHostNameOverride branch October 16, 2018 20:08

stevesloka mentioned this pull request Oct 26, 2018

Document the workaround for kube-proxy's mishandled "hostname-override" flag kubernetes/website#9878

Closed

BugRoger mentioned this pull request Oct 30, 2018

Fix local traffic only services sapcc/kubernikus#329

Merged

anguslees mentioned this pull request Nov 5, 2018

kube-proxy needs to be configured to override hostname in some environments kubernetes/kubeadm#857

Closed

aojea mentioned this pull request Jun 26, 2023

Modify the kube-proxy --hostname-override option is invalid #118860

Closed


		// TestProcessArgs tests processing args
		func TestProcessArgs(t *testing.T) {

Allow hostname-override arg to be used if specified #69340

Allow hostname-override arg to be used if specified #69340

Conversation

stevesloka commented Oct 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevesloka commented Oct 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timothysc commented Oct 2, 2018

Uh oh!

stevesloka commented Oct 2, 2018

Uh oh!

timothysc commented Oct 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thockin commented Oct 16, 2018

Uh oh!

k8s-ci-robot commented Oct 16, 2018

Uh oh!

mtaufen commented Oct 16, 2018

Uh oh!

stevesloka commented Oct 16, 2018

Uh oh!

anguslees commented Nov 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants