KEP for flagz page for Kubernetes Components

[richabanker]

• One-line PR description: Add a KEP for flagz page in Kubernetes components.

• Issue link: Flagz for Kubernetes Components #4828

[dgrisonnet]

/assign

[dgrisonnet]

Wouldn't it be better to have the endpoints in the form of /flagz/v1? Flagz is the subdomain that we want to version, if we were to put the version first, that would be confusing with the component version as it would then be at the top-level.

[richabanker]

Yes you're right, I got that the other way round. Fixed it now, thanks!

[dgrisonnet]

One thing I was wondering since then is whether going with a query parameter might not be better. Something like:

/flagz?version=1
/flags?version=2

wdyt?

[richabanker]

This was also raised in the statusz KEP PR. I am ok with either, whichever seems the best to indicate the version for the endpoint.

[richabanker]

Updated the KEP to use query params for specifying the version.

[dgrisonnet]

IIRC you won't get this metric for free, you'll have to make sure that the handler for the flagz endpoint is instrumented.

[richabanker]

Umm unsure about how flagz handler relates with this metric which I thought is enabled by default? The idea was to use this metric to monitor general load of requests that apiserver handles and use that as a signal to check if flagz is consuming all system resources causing delays in apiserver's capability to serve other resource requests..

though yeah I guess that wouldnt be a clean signal to detect issues with the flagz endpoint in a deterministic way.. Do you suggest introducing a new metric that keeps track of request latency for just /flagz requests ?

[dgrisonnet]

I forgot to submit my comment on your PR, but you already dealt with that concern of mine in your POC: kubernetes/kubernetes#127581 (comment)

[richabanker]

cc @johnbelamaric for PRR. Hi John, could you please review this KEP for prod-readiness whenever you get a chance? Thanks a lot!

[richabanker]

/assign @johnbelamaric

whoops meant to add as assignee

[johnbelamaric]

/assign

[thockin]

how? Do flag definitions allow us to express the idea that the flag is "sensitive" ?

[richabanker]

I dont think there currently is support for marking flags like that. So we would need to introduce a way to express this.
cc @cjcullen who I just reached out to for his thoughts on the same. Also @liggitt

[thockin]

Do we need to push that up to pflag? One could argue that falgs should NEVER have sensitive information, since they are visible via ps.

[richabanker]

Do we need to push that up to pflag?

Or.. maybe we could create a separate set of "sensitive flags" and while printing out the values in the flagz handler, redact the values for those flags?

One could argue that falgs should NEVER have sensitive information, since they are visible via ps

That's indeed true. But if we still feel that exposing this info in an endpoint will be easier for attackers to get hold of, we can impose some restrictions on what data to expose in the endpoint.

[thockin]

Do we have any examples of these sorts of flags?

[richabanker]

Perhaps the TLS related flags.. (also waiting for someone from sig-auth to help answer that..)

[liggitt]

I'm not aware any flags in long-lived components (kube-apiserver, kubelet, scheduler, controller-manager, etc) that contain sensitive information directly. For things like TLS or credentials, they always point at files which contain the data.

There are shorter-lived invocations that take flags containing sensitive information (like kubeadm init with --certificate-key or --token), and components built that bind client-go user flags (like kubectl) can specify a token credential on the command-line using --token. Both of those have the option to consume those values from a file as well.

pflag does have the ability to annotate flags, which we apparently use like this to mark some flags as "classified":

// AddSecretAnnotation add secret flag to Annotation.
func (f FlagInfo) AddSecretAnnotation(flags *pflag.FlagSet) FlagInfo {
	flags.SetAnnotation(f.LongName, "classified", []string{"true"})
	return f
}

and then filter out here when building an annotation to include in API objects when the client chooses to record the change cause in an annotation (--record)

	parseFunc := func(flag *pflag.Flag, value string) error {
		flags = flags + " --" + flag.Name
		if set, ok := flag.Annotations["classified"]; !ok || len(set) == 0 {
			flags = flags + "=" + value
		} else {
			flags = flags + "=CLASSIFIED"
		}
		return nil
	}

[richabanker]

Great! Thanks for pointing out the pflag annotation feature. That's perfect for displaying the non-CLASSIFIED flags.

[dgrisonnet]

this should be updated once we've agreed on the versioning format we want to follow

[mattbailey]

shadow prod readiness review

Just some nits/clerical (checkboxes), otherwise PRR looks good.

[johnbelamaric]

Some minor points on PRR, but looks OK to me.

[johnbelamaric]

PRR looks good, I will wait for sig approval to use the magic word

[dgrisonnet]

/lgtm
/approve

[johnbelamaric]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgrisonnet, johnbelamaric, richabanker

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:

• keps/prod-readiness/OWNERS [johnbelamaric]
• keps/sig-instrumentation/OWNERS [dgrisonnet,johnbelamaric]

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