fix(customresourcestate): do not write GVK labels into the shared config - #3077
fix(customresourcestate): do not write GVK labels into the shared config#3077mrueg wants to merge 2 commits into
Conversation
compile takes Resource by value, but CommonLabels is a map, so it is
shared with the configured resource and with every copy made from it.
Wildcard resolution makes one copy per discovered GVK, so writing the
GVK labels in place mutates a map they all point at, and leaks them back
into the user's configuration:
expected: map[string]string{"team":"myteam"}
actual: map[string]string{"customresource_group":"myteam.io",
"customresource_kind":"Bar",
"customresource_version":"v2",
"team":"myteam"}
The emitted metrics are correct today, because each compile overwrites
all three keys before Merge copies them into the family, and factories
are built one at a time. That makes this a hazard rather than a live
bug -- but it is one concurrent or partial compile away from families
carrying another GVK's labels.
Build a new map instead of writing through the shared one.
|
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe compile path copies ChangesCommon-label isolation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change prevents GVK labels from mutating shared configuration while preserving per-resource labels; no actionable merge-blocking risk remains beyond normal checks. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bhope
left a comment
There was a problem hiding this comment.
/lgtm
/hold
Feel free to unhold.
| return out | ||
| } | ||
|
|
||
| // Wildcard resolution produces one Resource copy per discovered GVK, all sharing |
There was a problem hiding this comment.
We probably may not need them in the tests. WDYT?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bhope, mrueg The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Keep a two-line note on why the shared map matters and drop the inline comments that restated the assertions. Addresses review feedback on kubernetes#3077.
|
New changes are detected. LGTM label has been removed. |
What this PR does / why we need it:
compiletakesResourceby value, butCommonLabelsis a map, so the copy shares it with the configured resource and with every other copy made from it — and wildcard resolution produces one copy per discovered GVK:Those three writes go into a map every copy points at, and into the user's configuration. Compiling
myteam.io/v1 Fooand thenmyteam.io/v2 Barfrom one configured resource leaves the caller's map as:To be clear about severity: the emitted metrics are correct today. Each
compileoverwrites all three keys beforeMergecopies them into the family, and factories are built one at a time, so no family ends up with another GVK's labels. This is a hazard rather than a live bug — but it is one concurrent or partially-overwriting compile away from being one, and mutating configuration the caller owns is wrong regardless.Building a new map removes the sharing. The test asserts both that each resolution keeps its own GVK labels and that the configured map is left as the user wrote it; the second assertion fails on
mainwith the output above.How does this change affect the cardinality of KSM: does not change cardinality
Which issue(s) this PR fixes: N/A
Summary by CodeRabbit
Bug Fixes
Tests