kube-proxy: add --write-config-to flag #45908

ncdc · 2017-05-16T20:39:12Z

Add --write-config-to flag to kube-proxy to write the default configuration
values to the specified file location.

@deads2k suggested I create my own scheme for this, so I followed the example he shared with me. The only bit currently still referring to api.Scheme is where we create the event broadcaster recorder. In order to use the custom private scheme, I either have to pass it in to NewProxyServer(), or I have to make NewProxyServer() a member of the Options struct. If the former, then I probably need to export Options.scheme. Thoughts?

cc @mikedanese @sttts @liggitt @deads2k @smarterclayton @timothysc @kubernetes/sig-network-pr-reviews @kubernetes/sig-api-machinery-pr-reviews

Add --write-config-to flag to kube-proxy to allow users to write the default configuration settings to a file.

sttts · 2017-05-17T06:55:30Z

cmd/kube-proxy/app/server.go

@@ -88,7 +95,8 @@ func checkKnownProxyMode(proxyMode string) bool {
 // Options contains everything necessary to create and run a proxy server.
 type Options struct {
 	// ConfigFile is the location of the proxy server's configuration file.
-	ConfigFile string
+	ConfigFile  string
+	WriteConfig string


godoc please. Is it a path name? Then configWritePath would be self-explaining.

It is currently a path name. I debated having --write-config be a bool, which would mean you'd do --config=foo.yaml --write-config, but that's not what we did in OpenShift, so I went this route instead. What do you think makes the most sense?

--write-config-to or --write-config-path would show that it is not a bool.

Actually, I was talking more about the Go field name in my comment above.

Sure, I can change the variable name and doc it.

sttts · 2017-05-17T06:56:30Z

cmd/kube-proxy/app/server.go

@@ -104,11 +112,15 @@ type Options struct {
 	master string
 	// healthzPort is the port to be used by the healthz server.
 	healthzPort int32
+
+	scheme *runtime.Scheme


Having this in the Options is strange. If it is just for writing, can't be create them locally in the write func?

I wanted to share the scheme with the applyDefaults function, and hopefully also where we set up the event recorder (since it needs a scheme). Is there a better way?

It's the only state we have, right? (next to globals) It looks strange in my eyes to have that in options. But I can live with it I guess.

I could make it a package-level global but was trying to avoid that. The other thing I'll say is that Options is really just a struct that holds state/context for running this command, so something like CommandContext might be a more appropriate name.

Yeah, context sounds much better.

sttts · 2017-05-17T06:57:43Z

cmd/kube-proxy/app/server.go

+		registry             = registered.NewOrDie("")
+	)
+
+	o.scheme = runtime.NewScheme()


just move these 3 lines into the two funcs, simple enough IMO.

And we don't need the registry here. Just call componentconfig.AddToScheme and componentconfigv1alpha1.AddToScheme.

@ncdc what about the second comment above? We are trying to remove the registry from many places, e.g. from the clients. We shouldn't rely on the static information it provides, but use discovery in kubectl/proxy/client-go instead. Here it's only about encoding, we do not even need discovery.

@sttts to be honest, I'm still learning the ins and outs of API registration, schemes, etc, so whatever you all say is the right way to do this, I'm happy to do that. So if we don't want or need a registry, I'll get rid of it.

once people have learned it, we change it again :-) So don't worry.

ncdc · 2017-05-17T16:46:51Z

Updated based on first round of code review

ncdc · 2017-05-17T17:49:10Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

sttts · 2017-05-18T07:43:04Z

cmd/kube-proxy/app/server.go

-		healthzPort: 10256,
+	opts, err := NewOptions()
+	if err != nil {
+		glog.Fatalf("unable to initialize command options: %v", err)


nit: Unable

sttts · 2017-05-18T07:44:21Z

Only one nit and the Options->CommandContext (can also live with Options, just cosmetics). Otherwise, lgtm.

Add --write-config flag to kube-proxy to write the default configuration values to the specified file location.

ncdc · 2017-05-18T14:45:04Z

@sttts updated

sttts · 2017-05-18T16:14:57Z

lgtm

ncdc · 2017-05-18T16:16:05Z

@mikedanese @deads2k @liggitt want to review?

deads2k · 2017-05-18T18:47:16Z

/approve

mikedanese · 2017-05-18T20:37:12Z

Scanned it and looks good. I'm happy if sttts is happy.

/lgtm

k8s-github-robot · 2017-05-18T20:37:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, mikedanese, ncdc

Needs approval from an approver in each of these OWNERS Files:

~~cmd/OWNERS~~ [mikedanese]
~~hack/verify-flags/OWNERS~~ [deads2k]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

sttts · 2017-05-19T08:08:29Z

@ncdc worth a short release note?

ncdc · 2017-05-19T12:56:18Z

@sttts added it at the top, ptal

sttts · 2017-05-19T13:09:41Z

lgtm

ncdc · 2017-05-19T14:07:00Z

@k8s-bot kops aws e2e test this
@k8s-bot pull-kubernetes-federation-e2e-gce test this

k8s-github-robot · 2017-05-19T16:01:03Z

Automatic merge from submit-queue

thockin · 2017-05-23T04:41:48Z

cmd/kube-proxy/app/server.go

 }

 // AddFlags adds flags to fs and binds them to options.
 func AddFlags(options *Options, fs *pflag.FlagSet) {
 	fs.StringVar(&options.ConfigFile, "config", options.ConfigFile, "The path to the configuration file.")
+	fs.StringVar(&options.WriteConfigTo, "write-config-to", options.WriteConfigTo, "If set, write the default configuration values to this file and exit.")


Does this always write DEFAULT values or effective values after parsing config (e.g. configmap)

@thockin what configmap?

thockin · 2017-05-24T00:40:08Z

Hypothetical :)

…

On Tue, May 23, 2017 at 5:31 AM, Andy Goldstein ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In cmd/kube-proxy/app/server.go <#45908 (comment)> : > } // AddFlags adds flags to fs and binds them to options. func AddFlags(options *Options, fs *pflag.FlagSet) { fs.StringVar(&options.ConfigFile, "config", options.ConfigFile, "The path to the configuration file.") + fs.StringVar(&options.WriteConfigTo, "write-config-to", options.WriteConfigTo, "If set, write the default configuration values to this file and exit.") @thockin <https://github.com/thockin> what configmap? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#45908 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVGalXEnntpT9QZqEu9pT94Pjphc5ks5r8tGpgaJpZM4NdDLX> .

ncdc · 2017-05-24T13:21:52Z

@thockin it currently writes default values as that's all we have to work with.

ncdc assigned sttts, liggitt, mikedanese and deads2k May 16, 2017

k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 16, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels May 16, 2017

timothysc added this to the v1.7 milestone May 16, 2017

sttts reviewed May 17, 2017

View reviewed changes

ncdc force-pushed the kube-proxy-write-config branch from 6242f21 to 5cccd64 Compare May 17, 2017 18:50

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

sttts reviewed May 18, 2017

View reviewed changes

kube-proxy: add --write-config flag

032e2f6

Add --write-config flag to kube-proxy to write the default configuration values to the specified file location.

ncdc force-pushed the kube-proxy-write-config branch from 5cccd64 to 032e2f6 Compare May 18, 2017 14:35

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 18, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 18, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 18, 2017

k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label May 18, 2017

ncdc changed the title ~~kube-proxy: add --write-config flag~~ kube-proxy: add --write-config-to flag May 19, 2017

k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 19, 2017

ncdc removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label May 19, 2017

k8s-github-robot merged commit 9a5694b into kubernetes:master May 19, 2017

thockin reviewed May 23, 2017

View reviewed changes

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

neoaggelos mentioned this pull request Feb 15, 2022

Add --write-config-to option to kubelet #108118

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kube-proxy: add --write-config-to flag #45908

kube-proxy: add --write-config-to flag #45908

ncdc commented May 16, 2017 •

edited

Loading

sttts May 17, 2017

ncdc May 17, 2017

sttts May 17, 2017

ncdc May 17, 2017

sttts May 17, 2017

ncdc May 17, 2017

sttts May 17, 2017

ncdc May 17, 2017

sttts May 18, 2017

sttts May 17, 2017

sttts May 17, 2017

sttts May 17, 2017

ncdc May 17, 2017

sttts May 18, 2017

ncdc commented May 17, 2017

ncdc commented May 17, 2017

sttts May 18, 2017

sttts commented May 18, 2017

ncdc commented May 18, 2017

sttts commented May 18, 2017

ncdc commented May 18, 2017

deads2k commented May 18, 2017

mikedanese commented May 18, 2017

k8s-github-robot commented May 18, 2017

sttts commented May 19, 2017

ncdc commented May 19, 2017

sttts commented May 19, 2017

ncdc commented May 19, 2017

k8s-github-robot commented May 19, 2017

thockin May 23, 2017

ncdc May 23, 2017

thockin commented May 24, 2017 via email

ncdc commented May 24, 2017

kube-proxy: add --write-config-to flag #45908

kube-proxy: add --write-config-to flag #45908

Conversation

ncdc commented May 16, 2017 • edited Loading

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

ncdc commented May 17, 2017

ncdc commented May 17, 2017

Choose a reason for hiding this comment

sttts commented May 18, 2017

ncdc commented May 18, 2017

sttts commented May 18, 2017

ncdc commented May 18, 2017

deads2k commented May 18, 2017

mikedanese commented May 18, 2017

k8s-github-robot commented May 18, 2017

sttts commented May 19, 2017

ncdc commented May 19, 2017

sttts commented May 19, 2017

ncdc commented May 19, 2017

k8s-github-robot commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented May 24, 2017 via email

ncdc commented May 24, 2017

ncdc commented May 16, 2017 •

edited

Loading