feat: allow wildcards as part of annotations and labels in allowlists#2873
feat: allow wildcards as part of annotations and labels in allowlists#2873skoef wants to merge 1 commit into
Conversation
|
Welcome @skoef! |
|
This issue is currently awaiting triage. If kube-state-metrics contributors determine this is a relevant issue, they will accept it by applying the The DetailsInstructions 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-sigs/prow repository. |
45d6189 to
1330742
Compare
1330742 to
411c93f
Compare
|
Thanks for the PR -- this solves a real issue with namespaced annotations. Left a few inline comments that need attention. |
|
/assign @bhope |
411c93f to
7b8a235
Compare
7b8a235 to
fe85b50
Compare
fbd4fe8 to
e621031
Compare
There was a problem hiding this comment.
Thanks for addressing the feedback. Left one inline nit, otherwise it's in pretty good shape.
However, the docs CI is failing -- cli-arguments.md is auto-generated from the cli flag usage strings. The wildcard documentation needs to be added to the flag descriptions in the code and then regenerate by running make doccheck locally.
|
@bhope: changing LGTM is restricted to collaborators DetailsIn response to this:
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-sigs/prow repository. |
bcd03f1 to
06049ee
Compare
bhope
left a comment
There was a problem hiding this comment.
Looks good to me.
We will wait for the CI to be green and one of the maintainers, if they have any comments.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bhope, skoef The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@CatherineF-dev could you review this MR please? |
There was a problem hiding this comment.
Pull request overview
Adds support for partial * wildcards in --metric-labels-allowlist / --metric-annotations-allowlist entries so users can allow groups of labels/annotations (e.g. monitoring.mycompany.org/*) without allowing everything.
Changes:
- Allow wildcard patterns in allowlist entries and enforce a max of 1
*per label/annotation key. - Implement wildcard matching in label/annotation extraction via regex expansion.
- Add unit tests and update CLI flag docs/help text.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/options/types_test.go | Adds allowlist parsing tests for partial wildcards and invalid multi-wildcard patterns. |
| pkg/options/types.go | Validates that each allowed label/annotation key contains at most one *. |
| pkg/options/options.go | Updates --help text to describe wildcard usage. |
| pkg/constant/utils.go | Introduces MaxPartialWildcardsPerLabel constant used by parsing/matching. |
| internal/store/utils.go | Matches allowlisted labels/annotations using wildcard-to-regex expansion. |
| internal/store/utils_test.go | Adds tests covering wildcard matching behavior and regex expansion. |
| docs/developer/cli-arguments.md | Updates documented CLI arguments to mention wildcard support. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // prepare regular expression based on potential wildcards | ||
| re, err := regexp.Compile(expandWildcard(l, constant.MaxPartialWildcardsPerLabel)) | ||
| if err != nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
createPrometheusLabelKeysValues now compiles a regexp and scans allKubeData for every allowList entry, even when the allowList entry is an exact key (no '*'). This changes the complexity from O(len(allowList)) map lookups to O(len(allowList)len(allKubeData)) and adds per-object regexp compilation. Consider fast-pathing entries without '' using a direct map lookup, and only compiling/scanning for patterns that actually contain a wildcard (or precompiling patterns once when parsing options).
There was a problem hiding this comment.
Since we use the label(-description) as the basis for the regex, we can't prepare the regex outside of this loop.
| if err != nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
We'd want to surface these to the user.
There was a problem hiding this comment.
How do you suggest I'd implement that? The function createPrometheusLabelKeysValues currently doesn't return an error. I could go over the labels/annotations and throw this error in LabelsAllowList.Set instead during startup?
|
Also PTAL at the Copilot reviews. |
Partial labels and annotations are possible in --metric-annotations-allowlist and --metric-labels-allowlist with this change Signed-off-by: Reinier Schoof <reinier@skoef.nl>
06049ee to
a59160e
Compare
|
New changes are detected. LGTM label has been removed. |
|
@rexagod Hi, could you perhaps re-review my changes? Thanks! |
| for _, test := range tests { | ||
| lal := &LabelsAllowList{} | ||
| gotError := lal.Set(test.Value) | ||
| if gotError != nil && !test.err || !reflect.DeepEqual(*lal, test.Wanted) { | ||
| if gotError != nil && test.err == nil || !reflect.DeepEqual(*lal, test.Wanted) { | ||
| t.Errorf("Test error for Desc: %s\n Want: \n%+v\n Got: \n%#+v\n Got Error: %#v", test.Desc, test.Wanted, *lal, gotError) | ||
| } |
| o.cmd.Flags().Var(&o.AnnotationsAllowList, "metric-annotations-allowlist", "Comma-separated list of Kubernetes annotation keys that will be used in the resource' labels metric. By default the annotations metrics are not exposed. To include them, provide a list of resource names in their plural form and Kubernetes annotation keys you would like to allow for them (Example: '=namespaces=[kubernetes.io/team,...],pods=[kubernetes.io/team],...)'. A single '*' can be provided per resource instead to allow any annotations, but that has severe performance implications (Example: '=pods=[*]'). Additionally, an asterisk (*) can be provided for resources, which will resolve to all resources, i.e., assuming '--resources=deployments,pods', '=*=[*]' will resolve to '=deployments=[*],pods=[*]'. Wildcards can also be use to match multiple annotations, i.e., '=pods=[something.example.org/foo-*]' will resolve to '=pods=[something.example.org/foo-bar,something.example.org/foo-baz,...]'") | ||
| o.cmd.Flags().Var(&o.LabelsAllowList, "metric-labels-allowlist", "Comma-separated list of additional Kubernetes label keys that will be used in the resource' labels metric. By default the labels metrics are not exposed. To include them, 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=[*]'). Additionally, an asterisk (*) can be provided as a key, which will resolve to all resources, i.e., assuming '--resources=deployments,pods', '=*=[*]' will resolve to '=deployments=[*],pods=[*]'. Wildcards can also be used to match multiple labels, but only a single '*' is supported per key pattern, i.e., '=pods=[something.example.org/foo-*]' will match labels such as 'something.example.org/foo-bar' and 'something.example.org/foo-baz'.") |
| for i, l := range allowList { | ||
| // only the first label can be the wildcard label | ||
| if l == options.LabelWildcard { | ||
| if i == 0 { | ||
| return kubeMapToPrometheusLabels(prefix, allKubeData) | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| for _, l := range allowList { | ||
| v, found := allKubeData[l] | ||
| if found { | ||
| allowedKubeData[l] = v | ||
| // prepare regular expression based on potential wildcards | ||
| re, err := regexp.Compile(expandWildcard(l, options.MaxPartialWildcardsPerLabel)) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| for k, v := range allKubeData { | ||
| if re.MatchString(k) { | ||
| allowedKubeData[k] = v | ||
| } | ||
| } |
| // expandWildcard expands wildcards (*) to regular expressions, up to a limited number of wildcards | ||
| func expandWildcard(pattern string, limit uint) string { | ||
| var result strings.Builder | ||
| var replacements uint | ||
| for i, literal := range strings.Split(pattern, options.LabelWildcard) { | ||
| if i > 0 { | ||
| result.WriteString(".*") | ||
| replacements++ | ||
| } | ||
|
|
||
| result.WriteString(regexp.QuoteMeta(literal)) | ||
| if replacements >= limit { | ||
| break | ||
| } | ||
| } | ||
| return "^" + result.String() + "$" | ||
| } |
What this PR does / why we need it:
In my clusters, I've add some annotations to nodes, pods and PVCs. I want to use these annotations to filter on in my alertmanager queries. For instance: I want to alert if a pod has high CPU usage, except when an annotation on that pod is set like
monitoring.mycompany.org/ignore-cpuor whatever. I have a whole bunch of these annotations.Currently, if I want to add these annotations to KSMs allowlist, I have to either use
*for all annotations (I'd rather not do this) or mention all my annotations, fornodes=,pods=, andpersistentvolumeclaims=. This could get rather annoying so I figured: why can't I use partial wildcards instead? Something likenodes=[monitoring.mycompany.org/*],pods=[monitoring.mycompany.org/*],persistentvolumeclaims=[monitoring.mycompany.org/*]or perhaps even*=[monitoring.mycompany.org/*].With this MR this would be possible, to the limit of 1
*in every match. I would like to get some feedback on this idea! 🙏How does this change affect the cardinality of KSM: does not change cardinality