Add wildcard option to metric-labels-allowlist #1403

Mikulas · 2021-03-03T09:14:11Z

What this PR does / why we need it:

#1301 introduced backwards compatibility issue by dropping all labels not defined in --metric-labels-allowlist. That feature is great and should arguably be enabled for all new deployments. However, constructing such a list may prove quite hard for users migrating from previous versions, especially if kube-state-metrics is managed by a different team than the ones consuming eg pod label metrics.

This patch adds a new wildcard option that allows all labels on given resources. The assumption is that using this is a last resort when migrating to 2.0 (well, 2.1) from previous version. The suggested feature is as simple as possible to allow users to migrate. Generic pattern matching (regex) are explicitly a non-goal for this PR.

Prior art:
#1289 allow metric labels by default
#1247 PR with regex support

Fixes #1309
Relates to #1246

lilic · 2021-03-10T17:32:37Z

@Mikulas Do you mind rebasing the PR, and we can have a look, thanks!

lilic

Nice work! Thank you! I am wondering if a single * would cause any problems, otherwise, this looks good apart from adding a few more test cases.

cc @tariq1890 @brancz please have a look as well as this changes our flags.

lilic · 2021-03-16T16:14:25Z

internal/store/pod_test.go

@@ -1495,8 +1498,8 @@ func TestPodStore(t *testing.T) {
 	}

 	for i, c := range cases {
-		c.Func = generator.ComposeMetricGenFuncs(podMetricFamilies(nil))
-		c.Headers = generator.ExtractMetricFamilyHeaders(podMetricFamilies(nil))
+		c.Func = generator.ComposeMetricGenFuncs(podMetricFamilies(c.AllowLabelsList))


This is great but ideally, we want a separate test case for this. Keep testing what we did before and add this as a new test.

Agreed, updated

lilic · 2021-03-16T16:17:55Z

docs/cli-arguments.md

@@ -32,7 +32,7 @@ Usage of ./kube-state-metrics:
  -h, --help                              Print Help text
      --host string                       Host to expose metrics on. (default "::")
      --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],...)'
+      --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],...)'. A single '*' can be provided per resource instead to allow any labels, but that has severe performance implications (Example: '=pods=[*]').


Just a thought: Wonder if there are scenarios where this might not work. 🤔 the argument having *. Like manifests in yaml or cli.

I don't think so, a single star is not an allowed symbol in labels (labels with length of one must match [a-z0-9A-Z]) and even if such a label key would be allowed eg kubernetes.io/*/value would not collide the ksm wildcard.
Configuration wise it should not clash with yaml features either as the argument value can/should be quoted.

I can't think of any issues. That being said an alternative syntax could be an ellipses eg pods=[...]. That has worse readability in my opinion but might be safer.

pkg/options/types.go

Mikulas · 2021-03-30T13:30:39Z

Thanks for the review :) I rebased the PR and updated tests as recommended. I'd be happy to add more tests if deemed necessary but it seems like pod_test.go is the catchall for generic behaviour and the rest of stores only test resource specific behaviour.

Mikulas · 2021-04-09T08:18:16Z

@lilic Please take a look when you get a chance :)

/assign lilic

lilic

/lgtm

I am sorry for the delay, I already reviewed but forgot to press the button 🤦‍♀️

Thank you so much for this!! 🎉

k8s-ci-robot · 2021-04-09T08:40:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lilic, Mikulas

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:

~~OWNERS~~ [lilic]

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

Mikulas · 2021-04-09T11:37:57Z

Awesome, thank you :)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 3, 2021

k8s-ci-robot requested review from brancz and lilic March 3, 2021 09:14

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 3, 2021

Mikulas force-pushed the label-allow-list-any branch 4 times, most recently from 1bc76dc to bbe7443 Compare March 3, 2021 11:16

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 5, 2021

Mikulas force-pushed the label-allow-list-any branch from bbe7443 to 4b660fc Compare March 15, 2021 09:17

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 15, 2021

lilic reviewed Mar 16, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 29, 2021

Mikulas changed the title ~~Add wildcard option to labels-metric-allow-list~~ Add wildcard option to labels-metric-allowlist Mar 30, 2021

Mikulas force-pushed the label-allow-list-any branch from 4b660fc to f5f66cb Compare March 30, 2021 13:11

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 30, 2021

Mikulas force-pushed the label-allow-list-any branch from f5f66cb to 71e0681 Compare March 30, 2021 13:21

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 30, 2021

Mikulas force-pushed the label-allow-list-any branch 2 times, most recently from 26e7cc5 to 30518dc Compare March 30, 2021 13:25

Add wildcard option to labels-metric-allow-list

70d039b

Mikulas force-pushed the label-allow-list-any branch from 30518dc to 70d039b Compare March 30, 2021 13:26

Mikulas changed the title ~~Add wildcard option to labels-metric-allowlist~~ Add wildcard option to metric-labels-allowlist Mar 30, 2021

Mikulas requested a review from lilic March 30, 2021 13:37

simonpasquier mentioned this pull request Apr 6, 2021

Bump kube-state-metrics to be synced with release-2.0 branch openshift/kube-state-metrics#48

Merged

k8s-ci-robot assigned lilic Apr 9, 2021

lilic approved these changes Apr 9, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 9, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 9, 2021

k8s-ci-robot merged commit ba36af0 into kubernetes:master Apr 9, 2021

Mikulas deleted the label-allow-list-any branch April 9, 2021 11:38

simonpasquier mentioned this pull request Apr 14, 2021

Provide an explicit list of allowed labels for the kube_node_labels metric openshift/cluster-monitoring-operator#1116

Closed

2 tasks

mrueg mentioned this pull request Apr 19, 2021

--labels-allow-list flag does not support wildcards #1246

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wildcard option to metric-labels-allowlist #1403

Add wildcard option to metric-labels-allowlist #1403

Mikulas commented Mar 3, 2021 •

edited

Loading

lilic commented Mar 10, 2021

lilic left a comment

lilic Mar 16, 2021

Mikulas Mar 30, 2021

lilic Mar 16, 2021

Mikulas Mar 30, 2021 •

edited

Loading

Mikulas commented Mar 30, 2021

Mikulas commented Apr 9, 2021

lilic left a comment

k8s-ci-robot commented Apr 9, 2021

Mikulas commented Apr 9, 2021

Add wildcard option to metric-labels-allowlist #1403

Add wildcard option to metric-labels-allowlist #1403

Conversation

Mikulas commented Mar 3, 2021 • edited Loading

lilic commented Mar 10, 2021

lilic left a comment

Choose a reason for hiding this comment

lilic Mar 16, 2021

Choose a reason for hiding this comment

Mikulas Mar 30, 2021

Choose a reason for hiding this comment

lilic Mar 16, 2021

Choose a reason for hiding this comment

Mikulas Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

Mikulas commented Mar 30, 2021

Mikulas commented Apr 9, 2021

lilic left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 9, 2021

Mikulas commented Apr 9, 2021

Mikulas commented Mar 3, 2021 •

edited

Loading

Mikulas Mar 30, 2021 •

edited

Loading