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

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

Merged
merged 1 commit into from Oct 16, 2018

Conversation

@stevesloka
Contributor

stevesloka commented Oct 2, 2018

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 steves@heptio.com

@@ -81,6 +81,10 @@ import (
"github.com/spf13/pflag"
)
var (
hostNameOverride string

This comment has been minimized.

@ncdc

ncdc Oct 2, 2018

Member

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

This comment has been minimized.

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

This comment has been minimized.

@ncdc

ncdc Oct 2, 2018

Member

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()

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

This comment has been minimized.

@ncdc

ncdc Oct 2, 2018

Member

Please remove

This comment has been minimized.

@stevesloka

stevesloka Oct 2, 2018

Contributor

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

This comment has been minimized.

@ncdc

ncdc Oct 2, 2018

Member

... the blank line

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

This comment has been minimized.

@ncdc

ncdc Oct 2, 2018

Member

exp->expected

@@ -204,14 +208,34 @@ func (o *Options) Complete() error {
}
}
err := utilfeature.DefaultFeatureGate.SetFromMap(o.config.FeatureGates)
err := o.processHostnameOverrideFlag()
if err != nil {

This comment has been minimized.

@ncdc

ncdc Oct 2, 2018

Member

gofmt

This comment has been minimized.

@ncdc

ncdc Oct 2, 2018

Member

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

@@ -350,7 +374,6 @@ with the apiserver API to configure the proxy.`,
if err := initForOS(opts.WindowsService); err != nil {
glog.Fatalf("failed OS init: %v", err)
}

This comment has been minimized.

@ncdc

ncdc Oct 2, 2018

Member

Revert?

@stevesloka

This comment has been minimized.

Contributor

stevesloka commented Oct 2, 2018

@ncdc I addressed your comments, thanks!

@@ -116,6 +116,9 @@ type Options struct {
scheme *runtime.Scheme
codecs serializer.CodecFactory
// hostNameOverrideArg is used to override the hostname of passed from a config file
hostNameOverrideArg string

This comment has been minimized.

@ncdc

ncdc Oct 2, 2018

Member

I would call this hostnameOverride

@@ -116,6 +116,9 @@ type Options struct {
scheme *runtime.Scheme
codecs serializer.CodecFactory
// hostNameOverrideArg is used to override the hostname of passed from a config file

This comment has been minimized.

@ncdc

ncdc Oct 2, 2018

Member

How about something like

hostnameOverride, if set from the command line flag, takes precedence over the `HostnameOverride` value from the config file
return err
}
return nil
}
// processHostnameOverrideFlag processes hostname-override argument
func (o *Options) processHostnameOverrideFlag() error {
// Check if hostname-override argument is set and use value since configFile always

This comment has been minimized.

@ncdc

ncdc Oct 2, 2018

Member

argument -> flag

@@ -221,6 +221,7 @@ nodePortAddresses:
clusterCIDR string
healthzBindAddress string
metricsBindAddress string
hostNameOverride string

This comment has been minimized.

@ncdc

ncdc Oct 2, 2018

Member

Is this needed?

This comment has been minimized.

@stevesloka

stevesloka Oct 2, 2018

Contributor

no not needed, removing

testCases := []struct {
name string
hostnameArg string

This comment has been minimized.

@ncdc

ncdc Oct 2, 2018

Member

hostnameArg -> hostnameOverrideFlag

expectedHostname: "foo",
},
{
name: "Hostname from argument",

This comment has been minimized.

@ncdc

ncdc Oct 2, 2018

Member

argument -> flag

},
}
for _, tc := range testCases {
options := NewOptions()

This comment has been minimized.

@ncdc

ncdc Oct 2, 2018

Member

Use a go subtest:

t.Run(tc.name, func(t *testing.T) {
  // body of test case
}
err := options.processHostnameOverrideFlag()
assert.NoError(t, err, "unexpected error for %s: %v", tc.name, err)
if tc.expectedHostname != options.config.HostnameOverride {
t.Fatalf("%s: expected hostname: %s, but got: %s", tc.name, tc.expectedHostname, options.config.HostnameOverride)

This comment has been minimized.

@ncdc

ncdc Oct 2, 2018

Member

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

options.hostNameOverrideArg = tc.hostnameArg
err := options.processHostnameOverrideFlag()
assert.NoError(t, err, "unexpected error for %s: %v", tc.name, err)

This comment has been minimized.

@ncdc

ncdc Oct 2, 2018

Member

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

@timothysc

This comment has been minimized.

Member

timothysc commented Oct 2, 2018

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

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

Allow hostname-override flag to be used if specified
Signed-off-by: Steve Sloka <steves@heptio.com>
@stevesloka

This comment has been minimized.

Contributor

stevesloka commented Oct 2, 2018

/retest

@timothysc

This comment has been minimized.

Member

timothysc commented Oct 11, 2018

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

This comment has been minimized.

@thockin

thockin Oct 11, 2018

Member

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

@mtaufen

This comment has been minimized.

@ncdc

ncdc Oct 11, 2018

Member

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

This comment has been minimized.

@thockin

thockin Oct 16, 2018

Member

blech

@thockin

This comment has been minimized.

Member

thockin commented Oct 16, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 16, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Oct 16, 2018

[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

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

@mtaufen

This comment has been minimized.

Contributor

mtaufen commented Oct 16, 2018

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

This comment has been minimized.

Contributor

stevesloka commented Oct 16, 2018

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

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

18 checks passed

cla/linuxfoundation stevesloka 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-100-performance 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-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details

@stevesloka stevesloka deleted the stevesloka:fixHostNameOverride branch Oct 16, 2018

@anguslees

This comment has been minimized.

Member

anguslees commented Nov 5, 2018

(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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment