kube-controller-manager: fix missing global flags for --help #70216

imjching · 2018-10-25T03:49:08Z

What type of PR is this?
/kind bug

What this PR does / why we need it: In #67362, we introduced logical sections for the --help output in the kube-controller-manager binary. It seems like it does not consider global flags that were registered through the flag package. This PR fixes that issue. We will output glog related and version flags in the "global" section, and everything else in the "generic" section.

Which issue(s) this PR fixes:
See #70145.

Special notes for your reviewer: The original PR is in #70164. The plan to is to break it down into individual PRs.

Does this PR introduce a user-facing change?:

Fix missing flags in kube-controller-manager --help.

/sig cli
/cc @stewart-yu @andrewsykim @dims
/assign @deads2k

k8s-ci-robot · 2018-10-25T03:49:15Z

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

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.

k8s-ci-robot · 2018-10-25T03:49:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: imjching
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: deads2k

If they are not already assigned, you can assign the PR to them by writing /assign @deads2k in a comment when ready.

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:

cmd/kube-controller-manager/OWNERS

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

jennybuckley · 2018-10-25T20:21:36Z

cc @logicalhan

imjching · 2018-10-26T03:10:46Z

/hold

#68054 is trying to add in globalflags.go into apiserver/pkg/util/flag/. Will wait until that is merged in so that we could rebase and use methods from there.

dims · 2018-11-11T01:04:46Z

/milestone v1.13

dims · 2018-11-12T02:22:08Z

/ok-to-test

dims · 2018-11-12T02:22:47Z

@imjching can you please update this PR since #68054 has merged?

imjching · 2018-11-12T05:18:07Z

@dims Got it! I'll rebase and update this patch together with the remaining patches tomorrow. 😄

imjching · 2018-11-15T04:38:55Z

/hold cancel

imjching · 2018-11-15T05:16:15Z

/test pull-kubernetes-integration

dims · 2018-11-15T12:03:59Z

/lgtm

andrewsykim · 2018-11-16T16:23:32Z

cmd/kube-controller-manager/app/options/customflags.go

+	"github.com/spf13/pflag"
+
+	// ensure libs have a chance to globally register their flags
+	_ "k8s.io/cloud-provider"


I don't think k8s.io/cloud-provider registers any flags unless I'm missing something.

I think what you really want is k8s.io/kubernetes/pkg/cloudprovider/providers

Yes, you're right. This is the flag that I am looking for:

kubernetes/pkg/cloudprovider/providers/gce/gce_loadbalancer.go

Line 52 in a3ccea9

flag.Var(&lbSrcRngsFlag, "cloud-provider-gce-lb-src-cidrs", "CIDRs opened in GCE firewall for LB traffic proxy & health checks")

I remember using k8s.io/kubernetes/pkg/cloudprovider/providers back then, but there was a build error. I just updated the path and hopefully it's all good now.

nikopen · 2018-11-16T17:51:50Z

FYI code freeze is today at 5pm PST - this should ideally make it before the deadline since it looks ready. @sttts @andrewsykim @imjching

Signed-off-by: Jay Lim <jay@imjching.com>

andrewsykim · 2018-11-16T19:49:05Z

cmd/kube-controller-manager/app/options/customflags.go

+	global := flag.CommandLine
+	local := pflag.NewFlagSet(os.Args[0], pflag.ExitOnError)
+
+	register(global, local, "cloud-provider-gce-lb-src-cidrs")


I don't love that this is here, we shouldn't leak provider flags like this but I get that it's not so easy to work around global flags. @sttts @stewart-yu any thoughts on this?

Hmm, I agree. I'm not too sure if there's a better way to do this. At the moment, we're doing something similar for kubelet as well:

kubernetes/cmd/kubelet/app/options/globalflags.go

Lines 82 to 92 in 7ff49c9

func addCredentialProviderFlags(fs *pflag.FlagSet) {

// lookup flags in global flag set and re-register the values with our flagset

global := pflag.CommandLine

local := pflag.NewFlagSet(os.Args[0], pflag.ExitOnError)

// TODO(#58034): This is not a static file, so it's not quite as straightforward as --google-json-key.

// We need to figure out how ACR users can dynamically provide pull credentials before we can deprecate this.

pflagRegister(global, local, "azure-container-registry-config")

fs.AddFlagSet(local)

}

i think it's OK for now, and we should find out more elegant way later. @andrewsykim

imjching · 2018-11-16T20:40:14Z

/test pull-kubernetes-e2e-gce

dims · 2018-11-19T05:38:24Z

/priority critical-urgent
/lgtm

Let's get this in and revisit any changes for 1.14 ( cc @andrewsykim )

stewart-yu · 2018-11-19T06:58:01Z

@imjching can you push the add file in https://github.com/kubernetes/kubernetes/tree/master/cmd/controller-manager/app/options directory, so we can reuse it both KCM and CCM

sttts · 2018-11-19T10:40:10Z

cmd/kube-controller-manager/app/options/customflags.go

+// register adds a flag to local that targets the Value associated with the Flag named globalName in global
+func register(global *flag.FlagSet, local *pflag.FlagSet, globalName string) {
+	if f := global.Lookup(globalName); f != nil {
+		local.AddGoFlag(f)


Why a different logic as in a77652e#diff-2fd8f885c4e1d904b7d678e5991aade4R68 ?

sttts · 2018-11-19T10:41:01Z

cmd/kube-controller-manager/app/options/customflags.go

+	global := flag.CommandLine
+	local := pflag.NewFlagSet(os.Args[0], pflag.ExitOnError)
+
+	register(global, local, "cloud-provider-gce-lb-src-cidrs")


how do we know not flag is forgotten? We need a test or a runtime error (or even panic?) for that.

AishSundar · 2018-11-21T01:30:18Z

@sttts do you think this PR is good to be approved for 1.13? If so can you plz approve it to enable merge?

k8s-ci-robot assigned deads2k Oct 25, 2018

k8s-ci-robot requested review from andrewsykim, dims and stewart-yu October 25, 2018 03:49

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

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 25, 2018

imjching mentioned this pull request Oct 25, 2018

Fix missing glog flags for some commands which output flags in logical sections #70164

Closed

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 26, 2018

k8s-ci-robot added this to the v1.13 milestone Nov 11, 2018

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 12, 2018

imjching force-pushed the 70145-fix-global-flags-kube-controller-manager branch from c5e1146 to 657a09d Compare November 15, 2018 04:38

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 15, 2018

k8s-ci-robot assigned dims Nov 15, 2018

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

andrewsykim reviewed Nov 16, 2018

View reviewed changes

kube-controller-manager: fix missing global flags for --help

9e57c62

Signed-off-by: Jay Lim <jay@imjching.com>

imjching force-pushed the 70145-fix-global-flags-kube-controller-manager branch from 657a09d to 9e57c62 Compare November 16, 2018 19:11

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

andrewsykim reviewed Nov 16, 2018

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. labels Nov 19, 2018

AishSundar mentioned this pull request Nov 19, 2018

kube-apiserver: fix missing global flags for --help #70204

Merged

imjching mentioned this pull request Nov 19, 2018

cloud-controller-manager: fix missing global flags for --help #70205

Closed

sttts reviewed Nov 19, 2018

View reviewed changes

stewart-yu mentioned this pull request Nov 21, 2018

add missing flags in *-controller-manager --help #71298

Merged

imjching closed this Nov 21, 2018

imjching deleted the 70145-fix-global-flags-kube-controller-manager branch November 21, 2018 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kube-controller-manager: fix missing global flags for --help #70216

kube-controller-manager: fix missing global flags for --help #70216

imjching commented Oct 25, 2018

k8s-ci-robot commented Oct 25, 2018

k8s-ci-robot commented Oct 25, 2018

jennybuckley commented Oct 25, 2018

imjching commented Oct 26, 2018

dims commented Nov 11, 2018

dims commented Nov 12, 2018

dims commented Nov 12, 2018

imjching commented Nov 12, 2018

imjching commented Nov 15, 2018

imjching commented Nov 15, 2018

dims commented Nov 15, 2018

andrewsykim Nov 16, 2018

andrewsykim Nov 16, 2018

imjching Nov 16, 2018

nikopen commented Nov 16, 2018

andrewsykim Nov 16, 2018 •

edited

imjching Nov 16, 2018

stewart-yu Nov 19, 2018

imjching commented Nov 16, 2018

dims commented Nov 19, 2018 •

edited

stewart-yu commented Nov 19, 2018

sttts Nov 19, 2018

sttts Nov 19, 2018

AishSundar commented Nov 21, 2018

	func addCredentialProviderFlags(fs *pflag.FlagSet) {
	// lookup flags in global flag set and re-register the values with our flagset
	global := pflag.CommandLine
	local := pflag.NewFlagSet(os.Args[0], pflag.ExitOnError)

	// TODO(#58034): This is not a static file, so it's not quite as straightforward as --google-json-key.
	// We need to figure out how ACR users can dynamically provide pull credentials before we can deprecate this.
	pflagRegister(global, local, "azure-container-registry-config")

	fs.AddFlagSet(local)
	}

kube-controller-manager: fix missing global flags for --help #70216

kube-controller-manager: fix missing global flags for --help #70216

Conversation

imjching commented Oct 25, 2018

k8s-ci-robot commented Oct 25, 2018

k8s-ci-robot commented Oct 25, 2018

jennybuckley commented Oct 25, 2018

imjching commented Oct 26, 2018

dims commented Nov 11, 2018

dims commented Nov 12, 2018

dims commented Nov 12, 2018

imjching commented Nov 12, 2018

imjching commented Nov 15, 2018

imjching commented Nov 15, 2018

dims commented Nov 15, 2018

andrewsykim Nov 16, 2018

Choose a reason for hiding this comment

andrewsykim Nov 16, 2018

Choose a reason for hiding this comment

imjching Nov 16, 2018

Choose a reason for hiding this comment

nikopen commented Nov 16, 2018

andrewsykim Nov 16, 2018 • edited

Choose a reason for hiding this comment

imjching Nov 16, 2018

Choose a reason for hiding this comment

stewart-yu Nov 19, 2018

Choose a reason for hiding this comment

imjching commented Nov 16, 2018

dims commented Nov 19, 2018 • edited

stewart-yu commented Nov 19, 2018

sttts Nov 19, 2018

Choose a reason for hiding this comment

sttts Nov 19, 2018

Choose a reason for hiding this comment

AishSundar commented Nov 21, 2018

andrewsykim Nov 16, 2018 •

edited

dims commented Nov 19, 2018 •

edited