Log 9591 Do not overwrite ConfigMaps owned by other resources - #3381
Open
stilwank wants to merge 3 commits into
Open
Log 9591 Do not overwrite ConfigMaps owned by other resources#3381stilwank wants to merge 3 commits into
stilwank wants to merge 3 commits into
Conversation
Refuse to update an existing ConfigMap when its ownerReferences do not match the ClusterLogForwarder. This prevents CLO from overwriting LokiStack (or other) ConfigMaps that share the same name, and surfaces the conflict as a deployment error on the forwarder. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughChangesOwned resource reconciliation
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: stilwank 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 |
openshift-ci
Bot
added
the
needs-ok-to-test
Contributor
|
Hi @stilwank. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
PR Summary by QodoPrevent overwriting ConfigMaps not owned by the ClusterLogForwarder
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/reconcile/configmaps_test.go (1)
90-105: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover conflicts where the content is already identical.
Because the existing foreign-owned ConfigMap has different data, this test would still pass if ownership validation were moved after
AreSame. Add a case with identical data and a foreign owner to verify that ownership conflicts always return an error.Suggested test case
+It("should refuse an identical ConfigMap owned by another resource", func() { + existing := runtime.NewConfigMap(namespace, name, map[string]string{ + "vector.toml": "data_dir = \"/var/lib/vector\"", + }) + utils.AddOwnerRefToObject(existing, lokiOwner) + k8sClient := newClient(existing) + + err := reconcile.Configmap(k8sClient, k8sClient, desiredCollectorCM(), comparators.CompareLabels) + Expect(err).To(HaveOccurred()) +})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/reconcile/configmaps_test.go` around lines 90 - 105, Add a test alongside “should refuse to overwrite a ConfigMap owned by another resource” using identical existing and desired ConfigMap data while retaining a foreign owner reference. Call reconcile.Configmap and assert it returns an error containing “refusing to overwrite,” confirming ownership validation occurs even when content matches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/reconcile/configmaps_test.go`:
- Around line 90-105: Add a test alongside “should refuse to overwrite a
ConfigMap owned by another resource” using identical existing and desired
ConfigMap data while retaining a foreign owner reference. Call
reconcile.Configmap and assert it returns an error containing “refusing to
overwrite,” confirming ownership validation occurs even when content matches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e6f4d65-dc5b-4152-b16f-da2b16cc7674
📒 Files selected for processing (2)
internal/reconcile/configmaps.gointernal/reconcile/configmaps_test.go
Code Review by Qodo
Context used✅ Compliance rules (platform):
9 rules 1.
|
![qodo-for-rh-openshift[bot]](https://avatars.githubusercontent.com/u/792337?s=60&v=4){kind=small}
Extend ownership checks beyond ConfigMaps to DaemonSet, Deployment, Service, ServiceAccount, ServiceMonitor, NetworkPolicy, Role/RoleBinding, ClusterRoleBinding, and trusted-CA ConfigMap. Shared helper matches owners by UID and surfaces a clear refuse-to-overwrite error. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/utils/ownership_test.go (1)
34-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for shared ownership.
The suite does not test an existing object containing the desired owner plus an additional non-controller owner. Add that case, and add a reconciler-level assertion that the additional OwnerReference remains after update.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/utils/ownership_test.go` around lines 34 - 55, Add a test in the ownership suite where the object has the desired owner plus an additional non-controller OwnerReference, and verify EnsureCanUpdateOwnedResource succeeds. Add a reconciler-level test or assertion covering the update path, confirming the additional OwnerReference is preserved after reconciliation.internal/reconcile/configmaps_test.go (1)
136-152: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that additional owner references are preserved.
This test verifies that reconciliation succeeds, but it would also pass if the implementation silently removed
extra-uid. Add an assertion that the extra OwnerReference remains after the update.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/reconcile/configmaps_test.go` around lines 136 - 152, Strengthen the test around reconcile.Configmap by asserting that the additional OwnerReference with UID "extra-uid" remains on the resulting ConfigMap. Keep the existing success and data assertions, and verify preservation using result.OwnerReferences.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/reconcile/configmaps.go`:
- Around line 30-31: Preserve unrelated OwnerReferences whenever reconciliation
updates an owned resource instead of replacing the full list with
desired.OwnerReferences: update internal/reconcile/configmaps.go:30-31,
internal/reconcile/deployment.go:20-22,
internal/reconcile/networkpolicy.go:20-22, internal/reconcile/rbac.go:19-21,
internal/reconcile/rbac.go:44-46, internal/reconcile/rbac.go:75-77, and
internal/reconcile/rbac.go:88-90 to retain existing non-target references while
ensuring desired references; in internal/reconcile/configmaps_test.go:136-152,
assert that the additional owner reference with UID extra-uid remains after
reconciliation.
In `@internal/utils/ownership.go`:
- Around line 26-41: Preserve the subset-matching behavior of IsOwnedByDesired
and add or document a merge operation that combines desired owners with existing
references without duplicates. Update owner-reference assignments in
internal/reconcile/daemon_sets.go (20-22), service.go (20-22),
service_account.go (18-20), service_monitor.go (20-22), and
internal/collector/trust_bundle.go (27-34) to retain additional existing owners
while ensuring all desired references are present.
---
Nitpick comments:
In `@internal/reconcile/configmaps_test.go`:
- Around line 136-152: Strengthen the test around reconcile.Configmap by
asserting that the additional OwnerReference with UID "extra-uid" remains on the
resulting ConfigMap. Keep the existing success and data assertions, and verify
preservation using result.OwnerReferences.
In `@internal/utils/ownership_test.go`:
- Around line 34-55: Add a test in the ownership suite where the object has the
desired owner plus an additional non-controller OwnerReference, and verify
EnsureCanUpdateOwnedResource succeeds. Add a reconciler-level test or assertion
covering the update path, confirming the additional OwnerReference is preserved
after reconciliation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9811bb02-6e6c-47d6-b218-e601542b75bc
📒 Files selected for processing (14)
internal/collector/trust_bundle.gointernal/reconcile/configmaps.gointernal/reconcile/configmaps_test.gointernal/reconcile/daemon_sets.gointernal/reconcile/daemonset_ownership_test.gointernal/reconcile/deployment.gointernal/reconcile/networkpolicy.gointernal/reconcile/rbac.gointernal/reconcile/rbac_test.gointernal/reconcile/service.gointernal/reconcile/service_account.gointernal/reconcile/service_monitor.gointernal/utils/ownership.gointernal/utils/ownership_test.go
Comment on lines
+30
to
+31
| if err := utils.EnsureCanUpdateOwnedResource(current, configMap.OwnerReferences); err != nil { | ||
| return err |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep unrelated OwnerReferences intact when the ownership guard allows a mutation.
The new contract explicitly allows a desired owner to coexist with additional OwnerReferences, but these reconciliation paths later replace the entire list with desired.OwnerReferences. That can remove another owner’s garbage-collection relationship. Preserve existing non-target references while ensuring desired references, and assert this behavior in the additional-owner test.
internal/reconcile/configmaps.go#L30-L31: preserve unrelated references when updating the ConfigMap.internal/reconcile/configmaps_test.go#L136-L152: assert thatextra-uidremains after reconciliation.internal/reconcile/deployment.go#L20-L22: preserve unrelated Deployment references.internal/reconcile/networkpolicy.go#L20-L22: preserve unrelated NetworkPolicy references.internal/reconcile/rbac.go#L19-L21: preserve unrelated Role references.internal/reconcile/rbac.go#L44-L46: preserve unrelated RoleBinding references.internal/reconcile/rbac.go#L75-L77: preserve unrelated ClusterRoleBinding references before roleRef handling.internal/reconcile/rbac.go#L88-L90: preserve unrelated ClusterRoleBinding references during update.
📍 Affects 5 files
internal/reconcile/configmaps.go#L30-L31(this comment)internal/reconcile/configmaps_test.go#L136-L152internal/reconcile/deployment.go#L20-L22internal/reconcile/networkpolicy.go#L20-L22internal/reconcile/rbac.go#L19-L21internal/reconcile/rbac.go#L44-L46internal/reconcile/rbac.go#L75-L77internal/reconcile/rbac.go#L88-L90
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/reconcile/configmaps.go` around lines 30 - 31, Preserve unrelated
OwnerReferences whenever reconciliation updates an owned resource instead of
replacing the full list with desired.OwnerReferences: update
internal/reconcile/configmaps.go:30-31, internal/reconcile/deployment.go:20-22,
internal/reconcile/networkpolicy.go:20-22, internal/reconcile/rbac.go:19-21,
internal/reconcile/rbac.go:44-46, internal/reconcile/rbac.go:75-77, and
internal/reconcile/rbac.go:88-90 to retain existing non-target references while
ensuring desired references; in internal/reconcile/configmaps_test.go:136-152,
assert that the additional owner reference with UID extra-uid remains after
reconciliation.
Comment on lines
+26
to
+41
| // IsOwnedByDesired reports whether every desired owner UID is present among current owners. | ||
| // An empty desired owner list only matches when current owners are also empty. | ||
| func IsOwnedByDesired(current, desired []metav1.OwnerReference) bool { | ||
| if len(desired) == 0 { | ||
| return len(current) == 0 | ||
| } | ||
| currentUIDs := make(map[string]struct{}, len(current)) | ||
| for _, ref := range current { | ||
| currentUIDs[string(ref.UID)] = struct{}{} | ||
| } | ||
| for _, want := range desired { | ||
| if _, ok := currentUIDs[string(want.UID)]; !ok { | ||
| return false | ||
| } | ||
| } | ||
| return true |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve additional owner references when shared ownership is accepted.
The ownership predicate allows current references to contain additional owners, but the reconcilers replace the complete list with only the desired references. This can remove another controller’s lifecycle/garbage-collection relationship.
internal/utils/ownership.go#L26-L41: retain the subset-matching contract and provide or document a merge operation for callers.internal/reconcile/daemon_sets.go#L20-L22: preserve additional DaemonSet owner references.internal/reconcile/service.go#L20-L22: preserve additional Service owner references.internal/reconcile/service_account.go#L18-L20: preserve additional ServiceAccount owner references.internal/reconcile/service_monitor.go#L20-L22: preserve additional ServiceMonitor owner references.internal/collector/trust_bundle.go#L27-L34: preserve additional trusted-bundle ConfigMap owner references.
📍 Affects 6 files
internal/utils/ownership.go#L26-L41(this comment)internal/reconcile/daemon_sets.go#L20-L22internal/reconcile/service.go#L20-L22internal/reconcile/service_account.go#L18-L20internal/reconcile/service_monitor.go#L20-L22internal/collector/trust_bundle.go#L27-L34
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/utils/ownership.go` around lines 26 - 41, Preserve the
subset-matching behavior of IsOwnedByDesired and add or document a merge
operation that combines desired owners with existing references without
duplicates. Update owner-reference assignments in
internal/reconcile/daemon_sets.go (20-22), service.go (20-22),
service_account.go (18-20), service_monitor.go (20-22), and
internal/collector/trust_bundle.go (27-34) to retain additional existing owners
while ensuring all desired references are present.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ready=False/DeploymentError) so users can rename or clean up conflicting resources.Details
Problem
When a
LokiStackand aClusterLogForwardershare the samemetadata.name, CLO previously overwrote Loki’s ConfigMap{name}-config(and could also create/collide on other same-named objects such as DaemonSet/Service/ServiceMonitor). That could leave Loki CrashLoopBackOff and is a general name-collision hazard, not Loki-specific.Ref: LOG-9591
Solution
internal/utils/ownership.go(EnsureCanUpdateOwnedResource) that refuses update when existingownerReferencesdo not match the desired owner (UID-based).ConfigMap, DaemonSet, Deployment, Service, ServiceAccount, ServiceMonitor, NetworkPolicy, Role/RoleBinding, ClusterRoleBinding, and trusted-CA ConfigMap (
trust_bundle.go).Notes
Test plan
go test ./internal/utils/ ./internal/reconcile/)Links
JIRA: https://redhat.atlassian.net/browse/LOG-9591
Supersedes: #3352
Summary by CodeRabbit