-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Adjust and refactor allowing labels to work for Kubernetes labels metrics #1301
Adjust and refactor allowing labels to work for Kubernetes labels metrics #1301
Conversation
bc02939
to
9bd4b63
Compare
docs/cli-arguments.md
Outdated
-h, --help Print Help text | ||
--host string Host to expose metrics on. (default "0.0.0.0") | ||
--kubeconfig string Absolute path to the kubeconfig file | ||
--labels-metric-allow-list string By default all labels metric will only have resource name and namespace labels, to allow more labels pass in a list of resource names in their plural form and Kubernetes labels you would like to allow, =namespaces=[k8s-label1,k8s-labeln...],pods[app]... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its hard to explain the labels-metric-allow-list
flag due to the nature of the metric labels and k8s labels, if anyone has a more clear naming, open to suggestions! I have a similar idea for annotations where it would be --annotations-metric-allow-list and you pass the resource and annotations to allow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to change the order and explain first what the flag does, then its default. Maybe something like: Allows to pass a list of additional Kubernetes label keys that will be used in the resource' labels metric. By default the metric contains only name and namespace labels. To include additional labels provide a list of resource names in their plural form and Kubernetes label keys you would like to allow for them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -93,8 +168,11 @@ func TestEndpointStore(t *testing.T) { | |||
}, | |||
} | |||
for i, c := range cases { | |||
c.Func = generator.ComposeMetricGenFuncs(endpointMetricFamilies) | |||
c.Headers = generator.ExtractMetricFamilyHeaders(endpointMetricFamilies) | |||
allowLabels := []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diff is strange here, so best expand, essentially this adds a Labels test, with labels we allow for the kube_endpoint_labels metric.
Note that I will adjust the unit tests and some other nits I noticed myself after initial thoughts for or against the approach. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might have missed it, but are you planning to set the sane defaults in another PR?
docs/cli-arguments.md
Outdated
-h, --help Print Help text | ||
--host string Host to expose metrics on. (default "0.0.0.0") | ||
--kubeconfig string Absolute path to the kubeconfig file | ||
--labels-metric-allow-list string By default all labels metric will only have resource name and namespace labels, to allow more labels pass in a list of resource names in their plural form and Kubernetes labels you would like to allow, =namespaces=[k8s-label1,k8s-labeln...],pods[app]... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--labels-metric-allow-list string By default all labels metric will only have resource name and namespace labels, to allow more labels pass in a list of resource names in their plural form and Kubernetes labels you would like to allow, =namespaces=[k8s-label1,k8s-labeln...],pods[app]... | |
--labels-metric-allow-list string By default all labels metric will only have resource name and namespace labels, to allow more labels pass in a list of resource names in their plural form and Kubernetes labels you would like to allow, =namespaces=[k8s-label1,k8s-labeln...],pods=[app]... |
pkg/options/options.go
Outdated
@@ -97,7 +97,7 @@ func (o *Options) AddFlags() { | |||
o.flags.StringVar(&o.Namespace, "pod-namespace", "", "Name of the namespace of the pod specified by --pod. "+autoshardingNotice) | |||
o.flags.BoolVarP(&o.Version, "version", "", false, "kube-state-metrics build version information") | |||
o.flags.BoolVar(&o.EnableGZIPEncoding, "enable-gzip-encoding", false, "Gzip responses when requested by clients via 'Accept-Encoding: gzip' header.") | |||
o.flags.Var(&o.LabelsAllowList, "labels-allow-list", "list of metric names and labels you would like to allow, metric_name=[label1,labeln...],metric_name[]...") | |||
o.flags.Var(&o.LabelsAllowList, "labels-metric-allow-list", "By default all labels metric will only have resource name and namespace labels, to allow more labels pass in a list of resource names in their plural form and Kubernetes labels you would like to allow, =namespaces=[k8s-label1,k8s-labeln...],pods[app]...") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
o.flags.Var(&o.LabelsAllowList, "labels-metric-allow-list", "By default all labels metric will only have resource name and namespace labels, to allow more labels pass in a list of resource names in their plural form and Kubernetes labels you would like to allow, =namespaces=[k8s-label1,k8s-labeln...],pods[app]...") | |
o.flags.Var(&o.LabelsAllowList, "labels-metric-allow-list", "By default all labels metric will only have resource name and namespace labels, to allow more labels pass in a list of resource names in their plural form and Kubernetes labels you would like to allow, =namespaces=[k8s-label1,k8s-labeln...],pods=[app]...") |
Thanks for the review, what do you mean by sane defaults, defaults for? |
Strategy wise this looks like the perfect fit for the problem to me. I think we should go with this. |
9bd4b63
to
20016d7
Compare
@brancz @dgrisonnet @jw-s please have a look, should be ready for review the details are in the description of the PR, thanks! |
Since this includes a bug fix for the labels metrics name and namespace, I changed base to release-2.0. |
Great job! Looks good :) |
71cde75
to
bdc3fdd
Compare
@@ -18,6 +18,9 @@ linters: | |||
linters-settings: | |||
goimports: | |||
local-prefixes: k8s.io/kube-state-metrics | |||
gocyclo: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will open an issue for this after the PR is merged, to refactor the podMetricFamilies function, note this function did not change, it was just a var before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is almost ready to merge. Just a few small things.
Side note: I love the parser for the label flag, and the ability to specify labels as they are in form of the Kubernetes label, I think this is much more ergonomic.
docs/cli-arguments.md
Outdated
-h, --help Print Help text | ||
--host string Host to expose metrics on. (default "0.0.0.0") | ||
--kubeconfig string Absolute path to the kubeconfig file | ||
--labels-metric-allow-list string By default all labels metric will only have resource name and namespace labels, to allow more labels pass in a list of resource names in their plural form and Kubernetes labels you would like to allow, =namespaces=[k8s-label1,k8s-labeln...],pods[app]... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this uncovered a cyclomatic complexity 127 for podMetricFamilies.
bdc3fdd
to
a551501
Compare
-h, --help Print Help text | ||
--host string Host to expose metrics on. (default "0.0.0.0") | ||
--kubeconfig string Absolute path to the kubeconfig file | ||
--labels-metric-allow-list string Allows to pass a list of additional Kubernetes label keys that will be used in the resource' labels metric. By default the metric contains only name and namespace labels. To include additional labels provide a list of resource names in their plural form and Kubernetes label keys you would like to allow for them (Example: '=namespaces=["k8s-label-1","k8s-label-n",...],pods=["app"],...)' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted the help text, are we happy with flag name?
--labels-metric-allow-list
My thinking for annotation it would be:
--annotations-metric-label-allow-list
So might need a change here as well? 🤔 It gets confusing as its label metrics for label allow list. 🤯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flag name(s) sounds good to me.
I wonder if at some point, we want to consider reading arguments from a config file for all settings. I don't think we'll hit max cmdline input length, but let's assume someone is very verbose ;)
It might also be easier to define it there in a structured way?
labels-metric-allow-list:
- namespaces:
- k8s-label1
- k8s-label2
- pods
- app
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I think if that happens we can add a flag called --labels-metric-allow-list-configfile that inputs a file, or one for both labels and annotations. Right now the parser makes certain assumptions (as it did before), I would say as we did not have a reading from a file until now I would not add it as part of this PR, but if there is something users face in the future, definitely! (For now its out of scope of this PR, as it just focuses on fixing bug and defining direction)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I was thinking about a generic --config
flag to do configuration for everything kube-state-metrics related.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, okay if that is what you need, and you have use case, please feel free to open a feature request w and we can discuss there :)
Addressed all comments, let me know if I missed some, ready for another review! Thanks all! |
Amazing work! 💯 /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brancz, lilic 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 |
During performance testing of the v2.0 release, I noticed the new filtering mechanism for labels always goes through all resources even in cases when flag is disabled, this introduces a performance regression. The use cases we have currently to filter labels are for Kubernetes resource labels metrics e.g. kube_pod_labels metrics, and in the future for annotation metrics. We heavily control other labels in metrics and make sure it does always contain a set list of labels and that set list is bound cardinality and does not contain any sensitive information. There are other ways of not exposing metrics, for example with the metric allow and deny list flags in kube-state-metrics as well as with resource allow list flag, as well as with Prometheus own mechanisms of metric relabeling.
In short, this PR introduces a flag called
--labels-metric-allow-list
which allows Kubernetes labels to be added to the specific resource labels metrics. By default all labels metrics will only have name of resource and namespace in them and all other labels are denied. This flag does not affect any other metrics and thus does not degrade performance. This same pattern and functions can be used for annotation metrics in the future.Example:
--labels-metric-allow-list=pods=["app","app.kubernetes.io/component"]
will produce a metric the following:
Note PR is WIP, I run this idea already with @brancz and also proposed this in the various issues and PRs. I am looking for actual implementation feedback now.