feat: finish ReferenceGrants implementation (#490)#541
Open
dermorz wants to merge 6 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds cross-namespace reference support via ReferenceGrants: introduces ChangesCross-Namespace References with ReferenceGrants
Sequence DiagramsequenceDiagram
participant ProfileController
participant ReleaseBinding
participant ReferenceGrant
participant TargetController
participant RenderTask
ProfileController->>ReleaseBinding: create/update (spec.targetNamespace may be set)
TargetController->>ReferenceGrant: list grants in Target namespace
ReferenceGrant-->>TargetController: authorize matching ReleaseBinding
TargetController->>ReleaseBinding: fetch Release from binding namespace
TargetController->>RenderTask: create RenderTask in release namespace
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 3
🤖 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 `@docs/developer-guide/profile_controller.md`:
- Around line 9-10: Update the broken link in profile_controller.md: replace the
relative link target string `./reference-grants.md` with the correct user-guide
path (e.g. `../user-guide/reference-grants.md`) so the "ReferenceGrants" link
from the Profile controller doc resolves to the new user-guide page; locate and
edit the occurrence in docs/developer-guide/profile_controller.md where the text
references ReferenceGrants.
In `@docs/developer-guide/target_controller.md`:
- Line 112: Update the broken ReferenceGrants link in
docs/developer-guide/target_controller.md: replace the incorrect relative link
"./reference-grants.md" (the link text "ReferenceGrants") with the correct path
into the user-guide (e.g., "../user-guide/reference-grants.md") so the reference
resolves from the developer-guide directory.
In `@pkg/controller/target_controller.go`:
- Around line 207-213: After appending cross-namespace ReleaseBindings (from
collectCrossNamespaceReleaseBindings) we still load Releases using the target's
namespace; change the Release lookup to use each ReleaseBinding's namespace when
resolving its Release. In the loop that iterates bindingList.Items (after
collectCrossNamespaceReleaseBindings), build the NamespacedName for the Release
using binding.Namespace (not target.Namespace) so cross-namespace bindings
resolve to Releases in the binding's namespace.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 339d4268-3c3a-4b2b-a46a-e01e7def9dd9
📒 Files selected for processing (16)
api/solar/registrybinding_types.goapi/solar/v1alpha1/registrybinding_types.goapi/solar/v1alpha1/zz_generated.conversion.goclient-go/applyconfigurations/solar/v1alpha1/registrybindingspec.goclient-go/openapi/zz_generated.openapi.godocs/.nav.ymldocs/developer-guide/adrs/005-Cluster-Scoped-Resources.mddocs/developer-guide/adrs/011-ReferenceGrants.mddocs/developer-guide/profile_controller.mddocs/developer-guide/target_controller.mddocs/user-guide/api-reference.mddocs/user-guide/reference-grants.mdmkdocs.ymlpkg/controller/target_controller.gotest/e2e/e2e_test.gotest/fixtures/e2e/cross-ns-referencegrant.yaml
Coverage Report for CI Build 26284570371Warning No base build found for commit Coverage: 72.833%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/controller/target_controller.go (2)
1022-1054:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
collectCrossNamespaceReleaseBindingscan return duplicate bindings.If two
ReferenceGrants intarget.Namespaceboth authorizeReleaseBindingfrom the samefrom.Namespace(or a single grant repeats aFromentry), the innerr.Listruns once per occurrence and the sameReleaseBindings are appended toresultmultiple times. Downstream, each duplicate produces another iteration in the binding loop (re-fetching the same Release/CV) and is also fed intoresolveReleaseConflictsas a separatereleaseInfowith the samebindingKey, which can corrupt skip messages and waste reconciliation work.🛡️ Suggested dedup
var result []solarv1alpha1.ReleaseBinding + seen := map[string]struct{}{} for i := range grantList.Items { grant := &grantList.Items[i] if !grantsReleaseBindingToTargetResource(grant) { continue } for _, from := range grant.Spec.From { if from.Kind != "ReleaseBinding" || from.Group != solarGroup { continue } + if _, ok := seen[from.Namespace]; ok { + continue + } + seen[from.Namespace] = struct{}{} crossBindings := &solarv1alpha1.ReleaseBindingList{}🤖 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 1022 - 1054, collectCrossNamespaceReleaseBindings can append the same ReleaseBinding multiple times when multiple ReferenceGrants or duplicate From entries reference the same from.Namespace; fix by deduplicating before returning: while iterating crossBindings.Items inside collectCrossNamespaceReleaseBindings (function name) build a seen map keyed by a stable identifier (e.g., rb.UID.String() or a namespacedName like rb.Namespace+"/"+rb.Name) and only append rb to result if not yet seen, so duplicate entries from repeated r.List calls are ignored.
292-345:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFix cross-namespace Release→RenderTask/chart collisions caused by identity keys missing namespace
releaseRenderTaskNameis derived only fromrel.Name,target.Name, andrel.GetGeneration()(norel.Namespace).computeReleaseRenderTaskSpecthen setschartName = "release-"+rel.Name,repo = target.Namespace+"/"+chartName, andtag = "v0.0."+rel.GetGeneration(). So two Releases from different namespaces with the samerel.Nameand same generation can be accepted byresolveReleaseConflicts(dedup key isrelease.Spec.UniqueName/CV component name, not namespace) yet map to the sameRenderTaskname and the same rendered chart artifact/version. The loop willGetthe already-existingRenderTaskon the second iteration and reuse itsStatus.ChartURL, so the second Release can incorrectly use the first Release’s rendered output.Include
rel.Namespace(or another namespace/unique identity) in theRenderTaskname and in the rendered chart identity (chartName/repo/tag) so each accepted Release maps to a distinct artifact.🤖 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 292 - 345, The RenderTask/ chart collisions occur because releaseRenderTaskName and computeReleaseRenderTaskSpec omit rel.Namespace, causing Releases with same name+generation across namespaces to share a RenderTask and artifact; update releaseRenderTaskName to include rel.Namespace (e.g., incorporate rel.Namespace into the generated rtName) and update computeReleaseRenderTaskSpec to include rel.Namespace in chartName/repo/tag (or otherwise derive a namespace-qualified unique identifier) so each Release (namespace+name+generation) maps to a distinct RenderTask and chart artifact; keep resolveReleaseConflicts logic intact but ensure identifiers used when creating/getting RenderTasks and rendered chart metadata include rel.Namespace to prevent cross-namespace reuse.
🧹 Nitpick comments (1)
pkg/controller/target_controller.go (1)
259-276: 💤 Low valueMinor: break out of the grant loop after a match.
for i := range grantList.Itemskeeps iterating aftergranted = true. Once a single grant permits the access, the remaining work is wasted. Addingbreakkeeps the hot path tight and mirrors the early-return pattern used inregistryGranted(lines 823–828).for i := range grantList.Items { if grantPermitsComponentVersionAccess(&grantList.Items[i], rel.Namespace) { granted = true + break } }🤖 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 259 - 276, The loop over grantList.Items in the block that checks cross-namespace ComponentVersion access (the for i := range grantList.Items loop that calls grantPermitsComponentVersionAccess and sets granted = true) should stop as soon as a permitting grant is found; after setting granted = true add a break to exit the loop immediately so we avoid unnecessary iterations and mirror the early-exit behavior used by registryGranted.
🤖 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.
Outside diff comments:
In `@pkg/controller/target_controller.go`:
- Around line 1022-1054: collectCrossNamespaceReleaseBindings can append the
same ReleaseBinding multiple times when multiple ReferenceGrants or duplicate
From entries reference the same from.Namespace; fix by deduplicating before
returning: while iterating crossBindings.Items inside
collectCrossNamespaceReleaseBindings (function name) build a seen map keyed by a
stable identifier (e.g., rb.UID.String() or a namespacedName like
rb.Namespace+"/"+rb.Name) and only append rb to result if not yet seen, so
duplicate entries from repeated r.List calls are ignored.
- Around line 292-345: The RenderTask/ chart collisions occur because
releaseRenderTaskName and computeReleaseRenderTaskSpec omit rel.Namespace,
causing Releases with same name+generation across namespaces to share a
RenderTask and artifact; update releaseRenderTaskName to include rel.Namespace
(e.g., incorporate rel.Namespace into the generated rtName) and update
computeReleaseRenderTaskSpec to include rel.Namespace in chartName/repo/tag (or
otherwise derive a namespace-qualified unique identifier) so each Release
(namespace+name+generation) maps to a distinct RenderTask and chart artifact;
keep resolveReleaseConflicts logic intact but ensure identifiers used when
creating/getting RenderTasks and rendered chart metadata include rel.Namespace
to prevent cross-namespace reuse.
---
Nitpick comments:
In `@pkg/controller/target_controller.go`:
- Around line 259-276: The loop over grantList.Items in the block that checks
cross-namespace ComponentVersion access (the for i := range grantList.Items loop
that calls grantPermitsComponentVersionAccess and sets granted = true) should
stop as soon as a permitting grant is found; after setting granted = true add a
break to exit the loop immediately so we avoid unnecessary iterations and mirror
the early-exit behavior used by registryGranted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76fdbdd9-8715-4a52-9ff5-002e4228e9ae
📒 Files selected for processing (4)
docs/developer-guide/profile_controller.mddocs/developer-guide/target_controller.mdpkg/controller/target_controller.gopkg/controller/target_controller_test.go
✅ Files skipped from review due to trivial changes (2)
- docs/developer-guide/target_controller.md
- docs/developer-guide/profile_controller.md
dermorz
force-pushed
the
feat/490-reference-grants
branch
3 times, most recently
from
a26d702 to
ab0863c
Compare
- Fix target controller bug: cross-namespace ReleaseBindings (created by Profile controller in provider-ns with spec.targetNamespace set) were never found because the controller only listed bindings in the target's own namespace. Now collects them via ReferenceGrant checks. - Fix mapper functions to use spec.targetNamespace when building reconcile requests for cross-namespace ReleaseBindings. - Add TargetNamespace field to RegistryBindingSpec as forward prep for cross-namespace RegistryBinding support (blocked on ADR-010). - Run codegen to update generated deepcopy, client-go apply configs, and OpenAPI spec. - Extend e2e test to verify a RenderTask is created for a cross-namespace target after the Profile controller creates the ReleaseBinding. - Add ADR-011 documenting the ReferenceGrants decision; mark ADR-005 superseded. - Add user guide page for ReferenceGrants with role-oriented guidance for K8s Cluster User, K8s Cluster Provider, and App Catalog Maintainer. - Update target_controller.md and profile_controller.md with ReferenceGrant watch triggers and cross-namespace notes.
Add two integration tests for the cross-namespace ReleaseBinding path in the target controller, covering the scenario where the binding and release live in a provider namespace and the target lives in a user namespace: - grant present: a RenderTask is created in the target's namespace - no grant: no RenderTask is created and ReleasesRendered=NoReleaseBindings Writing the tests revealed a third bug in the same code path: the grant check condition was `cvNamespace != target.Namespace` but should be `cvNamespace != rel.Namespace`. For same-namespace bindings these were equivalent (binding.Namespace == target.Namespace), so the bug was latent until cross-namespace bindings were exercised. Fixed alongside the tests.
…regression test Multiple ReferenceGrants or duplicate from entries covering the same provider namespace caused collectCrossNamespaceReleaseBindings to append the same binding more than once. Introduce a seen map keyed by namespace/name to skip already-collected bindings. Add an integration test that creates two overlapping grants and asserts exactly one RenderTask is produced, so any regression is caught immediately.
…with release namespace Releases with the same name from different namespaces (possible with cross-namespace ReleaseBindings) would collide on the same RenderTask name and OCI registry path. Include rel.Namespace in the releaseRenderTaskName hash input and in the repo path computed by computeReleaseRenderTaskSpec so each (namespace, name, generation) triple maps to a distinct artifact.
dermorz
force-pushed
the
feat/490-reference-grants
branch
from
ab0863c to
ef103e2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/controller/target_controller_test.go`:
- Around line 622-629: The release objects created by
newProviderRelease("xns2-release") (and the similar case at the other block)
still reference the old provider-cv, creating a hidden cross-test dependency;
update the release's spec to reference the ComponentVersion you create in the
same test (the cv variable for provider-cv2/provider-cv3) so each test points to
its own ComponentVersion instead of "provider-cv" — locate the
newProviderRelease call and modify the resulting Release object's reference
fields to use cv.Name (or the created ComponentVersion's Name) before creating
it with k8sClient.Create; apply the same change to the second occurrence
covering the other test case.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6919ee39-87cd-43db-88b6-795807250c06
📒 Files selected for processing (19)
api/solar/registrybinding_types.goapi/solar/v1alpha1/registrybinding_types.goapi/solar/v1alpha1/zz_generated.conversion.goclient-go/applyconfigurations/solar/v1alpha1/registrybindingspec.goclient-go/openapi/zz_generated.openapi.godocs/.nav.ymldocs/developer-guide/adrs/005-Cluster-Scoped-Resources.mddocs/developer-guide/adrs/011-ReferenceGrants.mddocs/developer-guide/profile_controller.mddocs/developer-guide/rendering-pipeline.mddocs/developer-guide/target_controller.mddocs/user-guide/api-reference.mddocs/user-guide/reference-grants.mdmkdocs.ymlpkg/controller/helpers.gopkg/controller/target_controller.gopkg/controller/target_controller_test.gotest/e2e/e2e_test.gotest/fixtures/e2e/cross-ns-referencegrant.yaml
✅ Files skipped from review due to trivial changes (7)
- docs/developer-guide/adrs/005-Cluster-Scoped-Resources.md
- client-go/openapi/zz_generated.openapi.go
- docs/developer-guide/profile_controller.md
- docs/user-guide/api-reference.md
- docs/developer-guide/target_controller.md
- docs/developer-guide/adrs/011-ReferenceGrants.md
- api/solar/v1alpha1/zz_generated.conversion.go
…easeBinding tests Tests 2 and 3 each create their own ComponentVersion (provider-cv2/3) but newProviderRelease hardcodes ComponentVersionRef.Name: "provider-cv", causing a hidden dependency on test 1's resources. Each test now sets rel.Spec.ComponentVersionRef.Name = cv.Name to reference its own CV.
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
Finish the ReferenceGrants feature started in #474.
Closes #490
Why
PR #474 merged the
ReferenceGrantCRD and a PoC implementation, but left several gaps:ReleaseBindingresources (created by the Profile controller in the provider namespace withspec.targetNamespaceset) were never discovered — the controller only listed bindings in the target's own namespace.ReferenceGrants.RegistryBindingSpecwas missing theTargetNamespacefield needed for future cross-namespace support (blocked on ADR-010).Testing
make test— all passingmapReleaseBindingToTargetandmapReleaseToTargetsmapper fixes covered by existing reconciler testsRenderTaskis created for the matched target (previously only verified theReleaseBindingwas created)Notes for reviewers
TargetReconciler.Reconcilenow callscollectCrossNamespaceReleaseBindingsafter the same-namespace list. This listsReferenceGrants in the target's namespace (already cached) and for each authorized namespace fetches bindings via the existingindexReleaseBindingTargetNamefield index. No new watches added — theReferenceGrantwatch was already in place.RegistryBindingSpecgains an optionalTargetNamespace stringfield. No controller logic uses it yet (seeFIXMEcomment intarget_controller.go);make codegenwas run to update generated files.superseded; ADR-011 added.ReferenceGrantuser guide page lives inTargetReconciler.Reconcilenow callscollectCrossNamespaceReleaseBindingsafter the same-namespace list. This listsReferenceGrants in the target's namespace (already cached) and for each authorized namespace fetches bindings via the existingindexReleaseBindingTargetNamefield index. No new watches added — theReferenceGrantwatch was already in place.RegistryBindingSpecgains an optionalTargetNamespace stringfield. No controller logic uses it yet (seeFIXMEcomment intarget_controller.go);make codegenwas run to update generated files.superseded; ADR-011 added.ReferenceGrantuser guide page lives indocs/user-guide/(not developer guide) — it documents what operators need to create, organised by role.generatepolicies). Wildcard namespace support is tracked as a spike for a follow-up issue.Checklist
Summary by CodeRabbit
New Features
Documentation
Tests