Refactor kube-proxy configuration #34727

ncdc · 2016-10-13T15:03:16Z

This is a proof of concept refactoring of the configuration and startup of kube-proxy. Most flags have been removed and replaced by a single config file, specified by --config. This is in regards to the component configuration improvement suggestions listed in #32215.

Also during this effort, I discovered that Hyperkube is roughly reimplementing portions of cobra, and that the current cobra command definitions are solely used to generated docs and man pages. I would like to move the individual commands as well as Hyperkube to using cobra, but that is a separate issue and discussion.

cc @mikedanese @liggitt @deads2k @eparis @sttts @smarterclayton @dgoodwin @timothysc

This change is

k8s-ci-robot · 2016-10-13T22:02:31Z

Bootstrap GCE e2e failed for commit 375ba9e. Full PR test history.

The magic incantation to run this job again is @k8s-bot bootstrap gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

mikedanese · 2016-10-14T18:32:40Z

@thockin can you sign to someone on networking for review?

ncdc · 2016-10-14T18:35:29Z

@mikedanese @thockin ideally the iptables/proxier bits would be refactored so that they have distinct New() (no side effects) vs Start() (side effects) behavior. I didn't make that effort here as my focus is on showing what the config changes could look like.

I also think I need to add a validation check to ProxyServer.Run to make sure all fields are set appropriately before continuing.

eparis · 2016-10-14T20:19:59Z

@marun would you have time to take a look. I know you won't be able to do an adequate review of the whole idea, but you can at least review the code here I think...

timothysc · 2016-10-17T14:40:36Z

Most flags have been removed and replaced by a single config file, specified by --config

Don't we have to go through at least 1 release where we mark as deprecated? Not that I'm opposed to what you're doing, as flags have gone feral. Just trying to ensure customer expectations are congruent with policy.

ncdc · 2016-10-17T17:29:19Z

@timothysc this is currently just a proof of concept of what the end state could look like. I will do whatever we all agree upon re a deprecation period and behavior.

marun · 2016-10-18T17:23:09Z

cmd/kube-proxy/app/server_test.go

 	assert.NotNil(t, proxyserver.IptInterface)
 }

 func TestGetConntrackMax(t *testing.T) {
 	ncores := runtime.NumCPU()
 	testCases := []struct {
-		config   componentconfig.KubeProxyConfiguration
+		config   componentconfig.KubeProxyConntrackConfiguration


Unrelated, but whenever I see tests written like this I want to simplify as follows:

testCases := []struct { max int32 maxPerCore int32 expected int err string }{ { expected: 0, }, ... } for i, tc:= range testCases { config := componentConfig.KubeProxyConntrackConfiguration{ Max: tc.max, MaxPerCore: tc.maxPerCore, } ... }

Or is it preferable to define objects in test tables?

It depends on the particular test, its cases, and how much flexibility you need.

marun · 2016-10-18T18:10:00Z

I like the separation of concerns this POC promotes. It makes the code easier to reason about and should allow for cleaner testing. An open question is how much work it will be to remove side-effects at configuration time (and the potential for breakage that represents), but I think it should be worth it.

sttts · 2016-10-19T07:59:59Z

cmd/kube-proxy/app/server.go

-		Conntracker:  conntracker,
-		ProxyMode:    proxyMode,
-	}, nil
+type KubeProxyOptions struct {


We are in the kube-proxy package here. Any need for the KubeProxy prefix?

Agree, though "Options" is as meaningless as "Config".

sttts · 2016-10-19T08:06:11Z

cmd/kube-proxy/app/server.go

+	flags := cmd.Flags()
+	BindKubeProxyOptions(&opts, flags)
+
+	cmd.MarkFlagFilename("config", "yaml", "yml")


No json? Then I would expect a hint for yaml in the flags description.

sttts · 2016-10-19T08:08:31Z

cmd/kube-proxy/app/server.go

 		glog.Errorf("unable to register configz: %s", err)
 	}
+
+	glog.V(4).Infof("using configuration: %#v", config)


nit: capital first letter in logs.

sttts · 2016-10-19T08:11:27Z

cmd/kube-proxy/app/server.go

+	Recorder               record.EventRecorder
+	ConntrackConfiguration componentconfig.KubeProxyConntrackConfiguration
+	Conntracker            Conntracker // if nil, ignored
+	ProxyMode              string


Is it intentional to duplicate all the config values here? Feels like a lot of bloat instead of just leaving the config reference in here (we have that at many places in other component as well, unfortunately).

I tend to agree - the more places this is replicated the harder it becomes to keep correct.

Though I feel lik the definition of the struct should be in the callee - it is the Server package's job to define what args it needs, and the main package's job to provide those args..

This is intentional. There is 1 struct for configuration (componentconfig.KubeProxyConfiguration) and 1 struct for runtime (ProxyServer). The runtime struct is used to run the component, and the config struct is the source of the configuration that is used to generate the runtime struct. We do not want to include the config struct inside the runtime struct.

Also, it's not required that you go from config struct to runtime struct. You could create only a runtime struct and use that to run the component. That is the contract for running. The config struct is a helper to go from a human-editable config file to runtime.

sttts · 2016-10-19T08:14:29Z

cmd/kube-proxy/proxy.go

 	"os"

+	"github.com/spf13/pflag"
+
+	goflag "flag"


nit: move up

sttts · 2016-10-19T08:15:37Z

hack/local-up-cluster.sh

-      --hostname-override="${HOSTNAME_OVERRIDE}" \
-      --feature-gates="${FEATURE_GATES}" \
-      --master="http://${API_HOST}:${API_PORT}" >"${PROXY_LOG}" 2>&1 &
+      --config /tmp/kube-proxy.yaml \


This seems to lose feature-gates, which is a flag defined in a different lib, and pretty important :)

Yeah, I have a TODO: https://github.com/kubernetes/kubernetes/pull/34727/files#diff-9380093d81cc2f68d59d326c28a1cc2eR75

k8s-github-robot · 2017-05-02T01:55:12Z

Automatic merge from submit-queue (batch tested with PRs 45077, 45180, 34727, 45079, 45177)

wojtek-t · 2017-05-02T11:19:28Z

This PR kind-of broke hollow-proxy (i.e. it didn't break tests, but with this PR, hollow-proxy isn't doing anything useful). I will try to send out PR fixing this soon.
@gmarek @shyamjvs

wojtek-t · 2017-05-02T11:29:02Z

#45227 is out for review

Automatic merge from submit-queue Fix hollow proxy to watch services and endpoints Ref #34727

Automatic merge from submit-queue (batch tested with PRs 46033, 46122, 46053, 46018, 45981) Restore kube-proxy --version Accidentally removed by #34727. Fixes #46026

bboreham · 2017-06-13T10:49:37Z

Where are the docs for this change, please?

ncdc · 2017-06-13T11:54:33Z

Not written yet. If you can direct me to the appropriate file, I will submit a docs PR. Thanks!

…

On Tue, Jun 13, 2017 at 6:50 AM, Bryan Boreham ***@***.***> wrote: Where are the docs for this change, please? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#34727 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAABYjP_WCGqSRuFlwKek4rmVk9IAtGoks5sDmlggaJpZM4KV_3w> .

bboreham · 2017-06-28T15:32:54Z

@ncdc kube-proxy docs are in https://github.com/kubernetes/kubernetes.github.io/blob/master/docs/admin/kube-proxy.md

ncdc · 2017-06-28T15:35:11Z

@bboreham I believe that file is autogenerated (it basically comes from kube-proxy --help). I posted a question in slack asking the best way to document this (https://kubernetes.slack.com/archives/C1J0BPD2M/p1498659434352871).

djsly · 2017-09-07T20:23:02Z

Where can we find the procedure for using the new --config approach?

The official addon seems to be creating an empty configmap

ncdc · 2017-09-07T20:24:40Z

I still owe a PR to put this in the docs. I've been on vacation but am back and hope to get to it soon.

…

On Thu, Sep 7, 2017 at 4:23 PM, Sylvain Boily ***@***.***> wrote: Where can we find the procedure for using the new --config approach? The official addon seems to be creating an empty configmap — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#34727 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAABYpK6P-w5XL-lZ_TGtndnrouCWSNDks5sgFDcgaJpZM4KV_3w> .

Refactor the kube-scheduler configuration API, command setup, and server setup according to the guidelines established in kubernetes#32215 and using the kube-proxy refactor (kubernetes#34727) as a model of a well factored component adhering to said guidelines. * Config API: clarify meaning and use of algorithm source by replacing modality derived from bools and string emptiness checks with an explicit AlgorithmSource type hierarchy. * Config API: consolidate client connection config with common structs. * Config API: split and simplify healthz/metrics server configuration. * Config API: clarify leader election configuration. * Config API: improve defaulting. * CLI: deprecate all flags except `--config`. * CLI: port all flags to new config API. * CLI: refactor to match kube-proxy Cobra command style. * Server: refactor away configurator.go to clarify application wiring. * Server: refactor to more clearly separate wiring/setup from running. Fixes kubernetes#52428.

@ncdc

Automatic merge from submit-queue (batch tested with PRs 53592, 52562, 55175, 55213). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Refactor kube-scheduler config API, command, and server setup Refactor the kube-scheduler configuration API, command setup, and server setup according to the guidelines established in #32215 and using the kube-proxy refactor (#34727) as a model of a well factored component adhering to said guidelines. * Config API: clarify meaning and use of algorithm source by replacing modality derived from bools and string emptiness checks with an explicit AlgorithmSource type hierarchy. * Config API: consolidate client connection config with common structs. * Config API: split and simplify healthz/metrics server configuration. * Config API: clarify leader election configuration. * Config API: improve defaulting. * CLI: deprecate all flags except `--config`. * CLI: port all flags to new config API. * CLI: refactor to match kube-proxy Cobra command style. * Server: refactor away configurator.go to clarify application wiring. * Server: refactor to more clearly separate wiring/setup from running. Fixes #52428. @kubernetes/api-reviewers @kubernetes/sig-cluster-lifecycle-pr-reviews @kubernetes/sig-scheduling-pr-reviews /cc @ncdc @timothysc @bsalamat ```release-note The kube-scheduler command now supports a `--config` flag which is the location of a file containing a serialized scheduler configuration. Most other kube-scheduler flags are now deprecated. ```

Refactor the kube-scheduler configuration API, command setup, and server setup according to the guidelines established in kubernetes#32215 and using the kube-proxy refactor (kubernetes#34727) as a model of a well factored component adhering to said guidelines. * Config API: clarify meaning and use of algorithm source by replacing modality derived from bools and string emptiness checks with an explicit AlgorithmSource type hierarchy. * Config API: consolidate client connection config with common structs. * Config API: split and simplify healthz/metrics server configuration. * Config API: clarify leader election configuration. * Config API: improve defaulting. * CLI: deprecate all flags except `--config`. * CLI: port all flags to new config API. * CLI: refactor to match kube-proxy Cobra command style. * Server: refactor away configurator.go to clarify application wiring. * Server: refactor to more clearly separate wiring/setup from running. Fixes kubernetes#52428.

googlebot added the cla: yes label Oct 13, 2016

k8s-github-robot assigned dchen1107 Oct 13, 2016

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Oct 13, 2016

ncdc force-pushed the kube-proxy-config branch from 6f5b2a6 to 375ba9e Compare October 13, 2016 15:43

ncdc assigned mikedanese and unassigned dchen1107 Oct 13, 2016

ncdc mentioned this pull request Oct 13, 2016

Component configuration improvement suggestions #32215

Closed

ncdc mentioned this pull request Oct 14, 2016

Server component commands and hyperkube should be using cobra #34732

Closed

15 tasks

mikedanese assigned thockin Oct 14, 2016

mikedanese mentioned this pull request Oct 14, 2016

kubeadm: create and migrate api/types.go to a componentconfig api #33715

Closed

timothysc added area/kube-proxy release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note-label-needed labels Oct 17, 2016

marun reviewed Oct 18, 2016

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2016

sttts reviewed Oct 19, 2016

View reviewed changes

cmd/kube-proxy/proxy.go Outdated

"os"

"github.com/spf13/pflag"

goflag "flag"

Copy link

Contributor

sttts Oct 19, 2016

Choose a reason for hiding this comment

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

nit: move up

sttts reviewed Oct 19, 2016

View reviewed changes

k8s-github-robot merged commit 5ffbd37 into kubernetes:master May 2, 2017

wojtek-t mentioned this pull request May 2, 2017

Fix hollow proxy to watch services and endpoints #45227

Merged

k8s-github-robot pushed a commit that referenced this pull request May 2, 2017

Merge pull request #45227 from wojtek-t/edge_based_hollow_proxy

1a09348

Automatic merge from submit-queue Fix hollow proxy to watch services and endpoints Ref #34727

shyamjvs mentioned this pull request May 8, 2017

Kubemark {e2e.go} #40139

Closed

This was referenced May 19, 2017

kube-proxy can no longer print its version #46026

Closed

Restore kube-proxy --version #46122

Merged

xiangpengzhao mentioned this pull request Jun 28, 2017

Fix kube-proxy panic when running with "--cleanup-iptables=true" #48183

Merged

erickt mentioned this pull request Jul 27, 2017

upgrade from 1.6 to 1.7 failed due to kube-proxy being passed extra arguments kubernetes/kops#3060

Closed

ironcladlou mentioned this pull request Sep 15, 2017

Refactor kube-scheduler config API, command, and server setup #52562

Merged

xiangpengzhao mentioned this pull request Oct 12, 2017

Remove deprecated kube-proxy flags #53782

Closed

dixudx mentioned this pull request Feb 4, 2018

remove the deprecated comment #59308

Closed

ncdc deleted the kube-proxy-config branch October 22, 2018 15:30

MrHohn mentioned this pull request May 24, 2019

Organize kube-proxy flags #78297

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor kube-proxy configuration #34727

Refactor kube-proxy configuration #34727

ncdc commented Oct 13, 2016 •

edited

Loading

k8s-ci-robot commented Oct 13, 2016

mikedanese commented Oct 14, 2016

ncdc commented Oct 14, 2016

eparis commented Oct 14, 2016

timothysc commented Oct 17, 2016

ncdc commented Oct 17, 2016

marun Oct 18, 2016 •

edited

Loading

ncdc Oct 18, 2016

marun commented Oct 18, 2016

sttts Oct 19, 2016

thockin Oct 19, 2016

sttts Oct 19, 2016

sttts Oct 19, 2016

sttts Oct 19, 2016

thockin Oct 19, 2016

thockin Oct 19, 2016

ncdc Oct 28, 2016

ncdc Oct 28, 2016

sttts Oct 19, 2016

sttts Oct 19, 2016

thockin Oct 19, 2016

ncdc Oct 19, 2016

k8s-github-robot commented May 2, 2017

wojtek-t commented May 2, 2017

wojtek-t commented May 2, 2017

bboreham commented Jun 13, 2017

ncdc commented Jun 13, 2017 via email

bboreham commented Jun 28, 2017

ncdc commented Jun 28, 2017

djsly commented Sep 7, 2017

ncdc commented Sep 7, 2017 via email

Refactor kube-proxy configuration #34727

Refactor kube-proxy configuration #34727

Conversation

ncdc commented Oct 13, 2016 • edited Loading

k8s-ci-robot commented Oct 13, 2016

mikedanese commented Oct 14, 2016

ncdc commented Oct 14, 2016

eparis commented Oct 14, 2016

timothysc commented Oct 17, 2016

ncdc commented Oct 17, 2016

marun Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marun commented Oct 18, 2016

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

k8s-github-robot commented May 2, 2017

wojtek-t commented May 2, 2017

wojtek-t commented May 2, 2017

bboreham commented Jun 13, 2017

ncdc commented Jun 13, 2017 via email

bboreham commented Jun 28, 2017

ncdc commented Jun 28, 2017

djsly commented Sep 7, 2017

ncdc commented Sep 7, 2017 via email

ncdc commented Oct 13, 2016 •

edited

Loading

marun Oct 18, 2016 •

edited

Loading