fix(controller): silently dropped cross-namespace releases in bootstrap chart#570
Open
dermorz wants to merge 1 commit into
Open
fix(controller): silently dropped cross-namespace releases in bootstrap chart#570dermorz wants to merge 1 commit into
dermorz wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds computation and propagation of a release "uniqueName" (via effectiveUniqueName), records it on internal releaseInfo entries during conflict resolution, requires it for buildBootstrapInput, and keys bootstrap chart inputs by that uniqueName; updates tests and docs to reflect the naming contract. ChangesBootstrap Release Keying by UniqueRelease
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/target_controller.go (1)
737-752: ⚡ Quick winFail fast on empty/duplicate bootstrap release keys.
This path can silently overwrite entries again if
ri.uniqueNameis empty or duplicated due to upstream regression. Add an explicit guard and return an error instead of relying only on invariants.Proposed hardening patch
func buildBootstrapInput(target *solarv1alpha1.Target, releases []releaseInfo) (solarv1alpha1.BootstrapInput, error) { resolvedReleases := map[string]solarv1alpha1.ResourceAccess{} for _, ri := range releases { + if ri.uniqueName == "" { + return solarv1alpha1.BootstrapInput{}, fmt.Errorf("release %s has empty uniqueName", ri.name) + } + if _, exists := resolvedReleases[ri.uniqueName]; exists { + return solarv1alpha1.BootstrapInput{}, fmt.Errorf("duplicate uniqueName in bootstrap input: %s", ri.uniqueName) + } + ref, err := ociname.ParseReference(ri.chartURL) if err != nil { return solarv1alpha1.BootstrapInput{}, fmt.Errorf("failed to parse chartURL %s: %w", ri.chartURL, err) } @@ resolvedReleases[ri.uniqueName] = solarv1alpha1.ResourceAccess{ Repository: strings.TrimPrefix(repo, "oci://"), Tag: ref.Identifier(), } }🤖 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 `@pkg/controller/target_controller.go` around lines 737 - 752, The loop over releases can silently overwrite entries when ri.uniqueName is empty or duplicated; inside the loop in the code that builds resolvedReleases (iterate releases, parse chartURL, build repo/tag), add a guard that checks ri.uniqueName is non-empty and that resolvedReleases does not already contain that key, and if either condition fails return a clear error (e.g., "empty bootstrap release key" or "duplicate bootstrap release key: %s") instead of continuing; update the block that populates resolvedReleases to validate ri.uniqueName before assigning to resolvedReleases[ri.uniqueName] and return the error immediately when the guard triggers.
🤖 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 `@pkg/controller/target_controller.go`:
- Around line 737-752: The loop over releases can silently overwrite entries
when ri.uniqueName is empty or duplicated; inside the loop in the code that
builds resolvedReleases (iterate releases, parse chartURL, build repo/tag), add
a guard that checks ri.uniqueName is non-empty and that resolvedReleases does
not already contain that key, and if either condition fails return a clear error
(e.g., "empty bootstrap release key" or "duplicate bootstrap release key: %s")
instead of continuing; update the block that populates resolvedReleases to
validate ri.uniqueName before assigning to resolvedReleases[ri.uniqueName] and
return the error immediately when the guard triggers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 750d9fa1-bd7c-4404-b634-5b33b2fa7f23
📒 Files selected for processing (4)
docs/developer-guide/adrs/004-Unique-Release-Name.mddocs/developer-guide/rendering-pipeline.mdpkg/controller/target_controller.gopkg/controller/target_controller_test.go
Coverage Report for CI Build 26817890237Warning No base build found for commit Coverage: 72.569%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
…espace collisions buildBootstrapInput keyed the bootstrap Releases map on the Release object name (ri.name), which is not unique across namespaces. With cross-namespace ReleaseBindings (#541), two same-named Releases from different namespaces can both survive conflict resolution (distinct uniqueNames) and target the same Target. They collided on the shared map key and one was silently dropped from the bootstrap chart — no error, just a missing deployment. Key the map on uniqueName instead — the deduplication key resolveReleaseConflicts already guarantees is unique among accepted releases. Store the computed uniqueName back onto releaseInfo so buildBootstrapInput can reuse it. Adds a buildBootstrapInput unit test with two same-named releases from different namespaces that fails before this fix. Also extract the uniqueName derivation rule into a shared effectiveUniqueName() helper in helpers.go, replacing the duplicated inline logic in both target_controller.go and release_controller.go. Add a guard in buildBootstrapInput that returns an explicit error when ri.uniqueName is empty, surfacing the invariant violation instead of silently collapsing all releases onto the "" key.
dermorz
force-pushed
the
fix/bootstrap-input-unique-name-key
branch
from
cf5190a to
72f690c
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
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
3 participants
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.
What
Fix a bug where two same-named Releases from different namespaces silently overwrite each other in the bootstrap chart.
Why
buildBootstrapInputkeyed the bootstrapReleasesmap onri.name— the bare Kubernetes Release object name. With cross-namespace ReleaseBindings (#541) two Releases namedmy-releasefrom different namespaces can both survive conflict resolution and target the same Target, colliding on the map key and dropping one deployment silently.Fix: key on
uniqueNameinstead — already guaranteed unique among accepted releases by the resolver. Also extract the derivation rule into a sharedeffectiveUniqueName()helper (was duplicated in both controllers), and add a guard that returns an explicit error ifuniqueNameis empty.Testing
buildBootstrapInputtests: cross-namespace collision (via fullresolveReleaseConflictspath) and explicit error on emptyuniqueName.make test/make lint— clean.Notes for reviewers
uniqueName. Existing Targets where the two differ will have inner FluxCD HelmReleases recreated on next bootstrap render.effectiveUniqueName(rel, cv)lives inhelpers.go; both controllers use it.rendering-pipeline.mdand ADR-004 now document whyuniqueNameis the bootstrap identity and why the object name is unsuitable.Checklist
Summary by CodeRabbit
Release Notes