ClusterRoles management for Functions#6841
Conversation
📝 WalkthroughWalkthroughAdds a shared RBAC roles utility, refactors provider RBAC to use it, introduces FunctionRevision RBAC renderer and reconciler (with tests and fuzzing), wires function roles into RBAC setup, and updates the Helm RBAC chart for FunctionRevision permissions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor K8s as Kubernetes
participant M as Controller Manager
participant FRc as FunctionRoles Controller
participant R as Reconciler
participant API as K8s API Server
K8s->>M: FunctionRevision event
M->>FRc: Enqueue reconcile request
FRc->>R: Reconcile(ctx, req)
rect rgb(248,250,255)
note right of R: Fetch & pre-checks
R->>API: Get FunctionRevision
alt NotFound
R-->>FRc: Exit (no requeue)
else Deleted or Paused
R-->>FRc: Exit (no requeue)
end
end
rect rgb(245,255,240)
note right of R: Render RBAC ClusterRoles
R->>R: resources := roles.DefinedResources(FR.status.objectRefs)
R->>R: roles := RenderClusterRoles(FR, resources)
end
rect rgb(255,250,240)
note right of R: Apply roles with diffing
loop each ClusterRole
R->>API: Apply ClusterRole (AllowUpdateIf roles.ClusterRolesDiffer)
alt Conflict
R-->>FRc: Requeue (conflict)
else Error
R-->>FRc: Record event & return error
else Success / No-op
R-->>FRc: Record event reasonApplyRoles
end
end
end
R-->>FRc: Done (no requeue)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Would you like a short migration note about the provider role name/label convention changes for downstream tooling? Thank you for the contribution — happy to clarify any part. Pre-merge checks✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used📓 Path-based instructions (2)**/*.go⚙️ CodeRabbit configuration file
Files:
**/internal/controller/**⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (4)internal/controller/rbac/provider/roles/roles.go (1)
internal/controller/rbac/function/roles/roles.go (2)
internal/controller/rbac/provider/roles/reconciler.go (1)
internal/controller/rbac/function/roles/reconciler.go (3)
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 |
c73d2bd to
c674753
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
internal/controller/rbac/roles/roles.go (1)
78-108: Avoid duplicate resources in DefinedResourcesIf refs contain the same CRD/MRD multiple times, we’ll emit duplicate Resource entries (and duplicate plural/status later), causing noisy RBAC rules. Dedup by group+plural here to keep rendered roles stable.
Apply this diff:
func DefinedResources(refs []xpv1.TypedReference) []Resource { - out := make([]Resource, 0, len(refs)) + out := make([]Resource, 0, len(refs)) + seen := make(map[string]struct{}, len(refs)) for _, ref := range refs { @@ - out = append(out, Resource{Group: g, Plural: p}) + key := p + "." + g + if _, ok := seen[key]; ok { + continue + } + seen[key] = struct{}{} + out = append(out, Resource{Group: g, Plural: p}) } return out }internal/controller/rbac/function/roles/reconciler_test.go (1)
124-132: Use the shared pause-annotation constantPrefer meta.AnnotationKeyReconciliationPaused over a string literal to avoid drift/typos and match provider tests.
Apply this diff:
@@ - fr.SetAnnotations(map[string]string{ - "crossplane.io/paused": "true", - }) + fr.SetAnnotations(map[string]string{ + meta.AnnotationKeyReconciliationPaused: "true", + })Add import:
@@ import ( "context" "io" "testing" @@ "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/crossplane/crossplane-runtime/v2/pkg/errors" "github.com/crossplane/crossplane-runtime/v2/pkg/logging" + "github.com/crossplane/crossplane-runtime/v2/pkg/meta"As per coding guidelines.
internal/controller/rbac/function/roles/roles_test.go (1)
400-485: Add MRD coverage to DefinedResources testsConsider a case for ManagedResourceDefinition (group v1alpha1, kind ManagedResourceDefinitionKind) to exercise the MRD branch and prevent regressions.
internal/controller/rbac/provider/roles/reconciler_test.go (1)
46-47: Alias the imported roles package to avoid ambiguity in package rolesImporting internal/controller/rbac/roles as roles inside package roles is readable but easy to misinterpret. Aliasing clarifies intent and prevents confusion.
Apply these diffs:
@@ - v1 "github.com/crossplane/crossplane/v2/apis/pkg/v1" - "github.com/crossplane/crossplane/v2/internal/controller/rbac/roles" + v1 "github.com/crossplane/crossplane/v2/apis/pkg/v1" + rbacroles "github.com/crossplane/crossplane/v2/internal/controller/rbac/roles"And update references:
- WithClusterRoleRenderer(ClusterRoleRenderFn(func(*v1.ProviderRevision, []roles.Resource) []rbacv1.ClusterRole { + WithClusterRoleRenderer(ClusterRoleRenderFn(func(*v1.ProviderRevision, []rbacroles.Resource) []rbacv1.ClusterRole { @@ - WithClusterRoleRenderer(ClusterRoleRenderFn(func(*v1.ProviderRevision, []roles.Resource) []rbacv1.ClusterRole { + WithClusterRoleRenderer(ClusterRoleRenderFn(func(*v1.ProviderRevision, []rbacroles.Resource) []rbacv1.ClusterRole { @@ - WithClusterRoleRenderer(ClusterRoleRenderFn(func(*v1.ProviderRevision, []roles.Resource) []rbacv1.ClusterRole { + WithClusterRoleRenderer(ClusterRoleRenderFn(func(*v1.ProviderRevision, []rbacroles.Resource) []rbacv1.ClusterRole { @@ - WithClusterRoleRenderer(ClusterRoleRenderFn(func(*v1.ProviderRevision, []roles.Resource) []rbacv1.ClusterRole { + WithClusterRoleRenderer(ClusterRoleRenderFn(func(*v1.ProviderRevision, []rbacroles.Resource) []rbacv1.ClusterRole { @@ - want []roles.Resource + want []rbacroles.Resource @@ - got := roles.DefinedResources(tc.refs) + got := rbacroles.DefinedResources(tc.refs) @@ - got := roles.ClusterRolesDiffer(tc.current, tc.desired) + got := rbacroles.ClusterRolesDiffer(tc.current, tc.desired)This keeps the test intent obvious.
Also applies to: 160-163, 184-187, 226-229, 254-257, 285-291, 329-333, 391-394
internal/controller/rbac/provider/roles/roles_test.go (2)
30-31: Import alias nit: avoid shadowing local package name.Current package is roles; importing the shared roles package as roles can confuse readers. Consider aliasing to rbacroles or sharedroles for clarity.
76-96: Add a duplicate resources test case.Merging across groups is covered. Please add a case where the same (group, plural) appears more than once to assert we don’t produce duplicate resource names in a single PolicyRule. If we adopt dedup in renderer (suggested separately), update the test accordingly.
internal/controller/rbac/provider/roles/roles.go (1)
90-109: Deduplicate and stable‑order resources per group.When aggregating from multiple ProviderRevisions in a family, the same CRD can appear multiple times, producing duplicate entries in Resources. While Kubernetes tolerates duplicates, removing them avoids churn and reduces RBAC size. Also, explicitly sort each group’s resources for deterministic rules regardless of input ordering.
Apply this diff:
- groups := make([]string, 0) // Allows deterministic iteration over groups. - - resources := make(map[string][]string) // Resources by group. - for _, r := range rs { - if _, ok := resources[r.Group]; !ok { - resources[r.Group] = make([]string, 0) - groups = append(groups, r.Group) - } - - resources[r.Group] = append(resources[r.Group], r.Plural, r.Plural+roles.SuffixStatus) - } - - rules := []rbacv1.PolicyRule{} - for _, g := range groups { - rules = append(rules, rbacv1.PolicyRule{ - APIGroups: []string{g}, - Resources: resources[g], - }) - } + groups := make([]string, 0) // Allows deterministic iteration over groups. + + // Build a set per group to avoid duplicate resource names. + resSet := make(map[string]map[string]struct{}) + for _, r := range rs { + if _, ok := resSet[r.Group]; !ok { + resSet[r.Group] = make(map[string]struct{}) + groups = append(groups, r.Group) + } + resSet[r.Group][r.Plural] = struct{}{} + resSet[r.Group][r.Plural+roles.SuffixStatus] = struct{}{} + } + + // Build rules with stable, sorted resource lists. + rules := []rbacv1.PolicyRule{} + for _, g := range groups { + res := make([]string, 0, len(resSet[g])) + for k := range resSet[g] { + res = append(res, k) + } + sort.Strings(res) + rules = append(rules, rbacv1.PolicyRule{ + APIGroups: []string{g}, + Resources: res, + }) + }As per coding guidelines
internal/controller/rbac/provider/roles/reconciler.go (2)
226-272: Deduplicate resources aggregated across a provider family.Appending DefinedResources from family members can duplicate (group, plural) pairs, causing redundant RBAC entries. Dedup before rendering.
Apply this diff:
- resources := roles.DefinedResources(pr.Status.ObjectRefs) + resources := roles.DefinedResources(pr.Status.ObjectRefs) @@ - resources = append(resources, roles.DefinedResources(member.Status.ObjectRefs)...) + resources = append(resources, roles.DefinedResources(member.Status.ObjectRefs)...) } } + // Deduplicate resources to avoid redundant RBAC entries. + if len(resources) > 1 { + seen := make(map[roles.Resource]struct{}, len(resources)) + dedup := resources[:0] + for _, rsrc := range resources { + if _, ok := seen[rsrc]; ok { + continue + } + seen[rsrc] = struct{}{} + dedup = append(dedup, rsrc) + } + resources = dedup + }As per coding guidelines
56-57: Improve error events: include role name and distinct failure reason.Current warnings don’t include which ClusterRole failed and share the same reason as success. Include the role name in the error and use a dedicated reason to aid triage.
Apply this diff:
-const ( - reasonApplyRoles event.Reason = "ApplyClusterRoles" -) +const ( + reasonApplyRoles event.Reason = "ApplyClusterRoles" + reasonApplyRolesFailed event.Reason = "ApplyClusterRolesFailed" +) @@ - err = errors.Wrap(err, errListPRs) - r.record.Event(pr, event.Warning(reasonApplyRoles, err)) + err = errors.Wrap(err, errListPRs) + r.record.Event(pr, event.Warning(reasonApplyRolesFailed, err)) @@ - err = errors.Wrap(err, errApplyRole) - r.record.Event(pr, event.Warning(reasonApplyRoles, err)) + err = errors.Wrapf(err, "%s %q", errApplyRole, cr.GetName()) + r.record.Event(pr, event.Warning(reasonApplyRolesFailed, err))As per coding guidelines
Also applies to: 246-249, 296-299
internal/controller/rbac/function/roles/reconciler.go (2)
194-201: Consider deduplicating resources before rendering.FunctionRevisions can reference overlapping CRDs across updates; deduping avoids redundant RBAC entries.
Apply this diff:
- resources := roles.DefinedResources(fr.Status.ObjectRefs) + resources := roles.DefinedResources(fr.Status.ObjectRefs) + if len(resources) > 1 { + seen := make(map[roles.Resource]struct{}, len(resources)) + dedup := resources[:0] + for _, rsrc := range resources { + if _, ok := seen[rsrc]; ok { + continue + } + seen[rsrc] = struct{}{} + dedup = append(dedup, rsrc) + } + resources = dedup + }
51-54: Improve error events: include role name and distinct failure reason.Add a failure reason and include the ClusterRole name in the error to help users understand what failed.
Apply this diff:
-const ( - reasonApplyRoles event.Reason = "ApplyClusterRoles" -) +const ( + reasonApplyRoles event.Reason = "ApplyClusterRoles" + reasonApplyRolesFailed event.Reason = "ApplyClusterRolesFailed" +) @@ - err = errors.Wrap(err, errApplyRole) - r.record.Event(fr, event.Warning(reasonApplyRoles, err)) + err = errors.Wrapf(err, "%s %q", errApplyRole, cr.GetName()) + r.record.Event(fr, event.Warning(reasonApplyRolesFailed, err))As per coding guidelines
Also applies to: 217-221
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cluster/charts/crossplane/templates/rbac-manager-clusterrole.yaml(1 hunks)internal/controller/rbac/function/roles/fuzz_test.go(1 hunks)internal/controller/rbac/function/roles/reconciler.go(1 hunks)internal/controller/rbac/function/roles/reconciler_test.go(1 hunks)internal/controller/rbac/function/roles/roles.go(1 hunks)internal/controller/rbac/function/roles/roles_test.go(1 hunks)internal/controller/rbac/provider/roles/fuzz_test.go(2 hunks)internal/controller/rbac/provider/roles/reconciler.go(5 hunks)internal/controller/rbac/provider/roles/reconciler_test.go(8 hunks)internal/controller/rbac/provider/roles/roles.go(6 hunks)internal/controller/rbac/provider/roles/roles_test.go(12 hunks)internal/controller/rbac/rbac.go(1 hunks)internal/controller/rbac/roles/roles.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/controller/rbac/rbac.go
- internal/controller/rbac/function/roles/roles.go
- cluster/charts/crossplane/templates/rbac-manager-clusterrole.yaml
- internal/controller/rbac/provider/roles/fuzz_test.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: Enforce Crossplane-specific patterns: Use crossplane-runtime/pkg/errors
for wrapping. Check variable naming (short for local scope, descriptive
for wider scope). Ensure 'return early' pattern. Verify error scoping
(declare in conditionals when possible). For nolint directives, require
specific linter names and explanations. CRITICAL: Ensure all error
messages are meaningful to end users, not just developers - avoid
technical jargon, include context about what the user was trying to do,
and suggest next steps when possible.
Files:
internal/controller/rbac/provider/roles/reconciler_test.gointernal/controller/rbac/function/roles/fuzz_test.gointernal/controller/rbac/provider/roles/roles.gointernal/controller/rbac/provider/roles/reconciler.gointernal/controller/rbac/function/roles/reconciler_test.gointernal/controller/rbac/provider/roles/roles_test.gointernal/controller/rbac/roles/roles.gointernal/controller/rbac/function/roles/roles_test.gointernal/controller/rbac/function/roles/reconciler.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Enforce table-driven test structure: PascalCase test names (no
underscores), args/want pattern, use cmp.Diff with
cmpopts.EquateErrors() for error testing. Check for proper test case
naming and reason fields. Ensure no third-party test frameworks (no
Ginkgo, Gomega, Testify).
Files:
internal/controller/rbac/provider/roles/reconciler_test.gointernal/controller/rbac/function/roles/fuzz_test.gointernal/controller/rbac/function/roles/reconciler_test.gointernal/controller/rbac/provider/roles/roles_test.gointernal/controller/rbac/function/roles/roles_test.go
**/internal/controller/**
⚙️ CodeRabbit configuration file
**/internal/controller/**: Review controller logic for proper reconciliation patterns, error
handling, and resource management. Pay special attention to conditions
and events: Conditions must be actionable for users (not developers),
stable/deterministic, with proper Type/Reason/Message format. Events
only when something actually happens, with specific details about what
changed. No transient errors in conditions/events. All error messages
must be meaningful to end users - include context about what
resource/operation failed and why.
Files:
internal/controller/rbac/provider/roles/reconciler_test.gointernal/controller/rbac/function/roles/fuzz_test.gointernal/controller/rbac/provider/roles/roles.gointernal/controller/rbac/provider/roles/reconciler.gointernal/controller/rbac/function/roles/reconciler_test.gointernal/controller/rbac/provider/roles/roles_test.gointernal/controller/rbac/roles/roles.gointernal/controller/rbac/function/roles/roles_test.gointernal/controller/rbac/function/roles/reconciler.go
🧬 Code graph analysis (9)
internal/controller/rbac/provider/roles/reconciler_test.go (2)
internal/controller/rbac/provider/roles/reconciler.go (2)
WithClusterRoleRenderer(137-141)ClusterRoleRenderFn(66-66)internal/controller/rbac/roles/roles.go (3)
Resource(70-76)DefinedResources(79-108)ClusterRolesDiffer(113-120)
internal/controller/rbac/function/roles/fuzz_test.go (3)
internal/controller/rbac/provider/roles/fuzz_test.go (1)
FuzzRenderClusterRoles(28-43)internal/controller/rbac/roles/roles.go (1)
Resource(70-76)internal/controller/rbac/function/roles/roles.go (1)
RenderClusterRoles(39-107)
internal/controller/rbac/provider/roles/roles.go (1)
internal/controller/rbac/roles/roles.go (11)
VerbsEdit(63-63)Resource(70-76)SuffixStatus(57-57)NameSuffixEdit(36-36)ValTrue(54-54)KeyAggregateToAdmin(45-45)KeyAggregateToEdit(48-48)WithVerbs(123-131)NameSuffixView(39-39)KeyAggregateToView(51-51)VerbsView(66-66)
internal/controller/rbac/provider/roles/reconciler.go (2)
internal/controller/rbac/roles/roles.go (3)
Resource(70-76)DefinedResources(79-108)ClusterRolesDiffer(113-120)internal/controller/rbac/function/roles/reconciler.go (1)
ClusterRoleRenderFn(63-63)
internal/controller/rbac/function/roles/reconciler_test.go (2)
internal/controller/rbac/function/roles/reconciler.go (6)
ReconcilerOption(89-89)WithClientApplicator(107-111)WithLogger(92-96)WithClusterRoleRenderer(115-119)ClusterRoleRenderFn(63-63)NewReconciler(122-143)internal/controller/rbac/roles/roles.go (1)
Resource(70-76)
internal/controller/rbac/provider/roles/roles_test.go (1)
internal/controller/rbac/roles/roles.go (11)
NameSuffixEdit(36-36)NameSuffixView(39-39)Resource(70-76)KeyAggregateToCrossplane(42-42)ValTrue(54-54)KeyAggregateToAdmin(45-45)KeyAggregateToEdit(48-48)SuffixStatus(57-57)VerbsEdit(63-63)KeyAggregateToView(51-51)VerbsView(66-66)
internal/controller/rbac/roles/roles.go (1)
apis/apiextensions/v1alpha1/register.go (1)
ManagedResourceDefinitionKind(53-53)
internal/controller/rbac/function/roles/roles_test.go (2)
internal/controller/rbac/roles/roles.go (14)
NameSuffixEdit(36-36)NameSuffixView(39-39)Resource(70-76)KeyAggregateToCrossplane(42-42)ValTrue(54-54)KeyAggregateToAdmin(45-45)KeyAggregateToEdit(48-48)SuffixStatus(57-57)VerbsEdit(63-63)KeyAggregateToView(51-51)VerbsView(66-66)ClusterRolesDiffer(113-120)WithVerbs(123-131)DefinedResources(79-108)internal/controller/rbac/function/roles/roles.go (1)
RenderClusterRoles(39-107)
internal/controller/rbac/function/roles/reconciler.go (2)
internal/controller/rbac/function/roles/roles.go (1)
RenderClusterRoles(39-107)internal/controller/rbac/roles/roles.go (3)
Resource(70-76)DefinedResources(79-108)ClusterRolesDiffer(113-120)
🔇 Additional comments (5)
internal/controller/rbac/roles/roles.go (2)
110-120: LGTM: straightforward, intentional type assertionPanic on non-ClusterRole is acceptable here given AllowUpdateIf contract and the comment clarifies intent.
122-131: LGTM: WithVerbs copies without mutating inputCorrectly preserves original rules; simple and clear.
internal/controller/rbac/function/roles/fuzz_test.go (1)
28-42: LGTM: useful fuzz harnessSimple, safe fuzz coverage for RenderClusterRoles with generated FunctionRevision and resources.
internal/controller/rbac/provider/roles/roles.go (1)
169-176: LGTM: deterministic naming and owner refs.Use of roles.NameSuffix{Edit,View} and meta.AsController owner refs is clear and correct. Stable.
internal/controller/rbac/function/roles/reconciler.go (1)
70-86: LGTM: controller wiring.Setup registers watch/ownership, uses rate‑limited reconciler, and honors controller options. Looks solid.
|
applied coderabbit suggestions |
Implements RBAC role management for FunctionRevisions, automatically creating ClusterRoles for resources defined by functions. This follows the same pattern as ProviderRevisions but with key differences: - Creates only aggregated edit and view roles (no system role) - Functions are invoked via gRPC and don't need Kubernetes API access - No support for function families (not yet implemented) - No extra permissions for secrets, configmaps, events, or leases The reconciler watches FunctionRevisions and creates ClusterRoles that aggregate to crossplane, admin, edit, and view roles, allowing users to interact with CRDs installed by functions. Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
Extracts common RBAC role constants, types, and functions from provider and function role packages into a new shared package. This reduces code duplication and improves maintainability. Changes: - Add internal/controller/rbac/roles package with shared utilities - Move common constants (KeyAggregateTo*, ValTrue, SuffixStatus, etc.) - Move Resource type and DefinedResources function - Move ClusterRolesDiffer and WithVerbs helper functions - Update provider and function packages to use shared code - Update all tests to reference shared constants Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
The RBAC manager needs permissions to list/watch FunctionRevisions and update their finalizers in order to manage ClusterRoles for functions. This matches the existing pattern for ProviderRevisions. Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
Deduplicate resources at the source level to avoid redundant RBAC entries: - DefinedResources: Dedups by group+plural when extracting from objectRefs - Provider reconciler: Dedups after aggregating from provider family members - RenderClusterRoles: No deduplication (trusts input is already deduped) This single-point deduplication strategy is more efficient and simpler than deduplicating at multiple layers. Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
Add distinct failure event reason and include ClusterRole name in error messages to help users understand what failed. Follow project naming convention for formatted error constants (errFmt prefix). Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
Replace hardcoded "crossplane.io/paused" string with meta.AnnotationKeyReconciliationPaused constant for consistency. Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
Extend TestDefinedResources to verify that ManagedResourceDefinition references are properly converted to Resource structs, complementing existing CRD test coverage. Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
fa7fbfb to
a34bf92
Compare
uhm, not sure about that 🤔 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/rbac/provider/roles/roles.go (1)
63-69: Security: avoid “*” verbs on core resources in system role.roles.VerbsEdit maps to “*”, granting delete/deletecollection on secrets/configmaps/events/leases. Use verbsSystem to avoid privilege escalation.
Apply this diff:
- Verbs: roles.VerbsEdit, + Verbs: verbsSystem,
🧹 Nitpick comments (5)
internal/controller/rbac/function/roles/roles_test.go (1)
400-492: Consider adding dedup cases for DefinedResources.Add a case with duplicate CRD/MRD refs (and/or duplicate plural.group) to assert dedup behavior. Also consider a RenderClusterRoles test with duplicate resources to confirm output stability if upstream dedup were to regress.
internal/controller/rbac/function/roles/roles.go (1)
45-69: Optional: make group iteration independent of resource sort.Current “first-seen” order is deterministic due to sorted rs; if you want to decouple, sort groups explicitly before building rules. Keeping parity with provider roles is also fine.
internal/controller/rbac/provider/roles/reconciler_test.go (1)
281-335: Add a test for aggregated-resource dedup.Given the new map-based dedup in Reconciler, add a case where family members define overlapping plural.group pairs and assert only unique resources are rendered/applied.
internal/controller/rbac/provider/roles/roles.go (1)
149-151: Nit: fix comment typo.“tha provider” → “the provider”.
- // directly to the service account tha provider runs as. + // directly to the service account the provider runs as.internal/controller/rbac/function/roles/reconciler.go (1)
44-54: Consider enriching error messages for end users.The error messages (
errGetFR,errFmtApplyRole) are quite technical. Per Crossplane coding guidelines, error messages should be meaningful to end users, not just developers.While these are internal controller errors that are rare, they appear in events that users see via
kubectl describe functionrevision. Consider whether messages like:errGetFR = "cannot retrieve FunctionRevision to manage RBAC roles" errFmtApplyRole = "cannot create or update RBAC ClusterRole %q - the RBAC manager may lack necessary permissions"would help users understand what went wrong. However, since these are already wrapped with Kubernetes API error details, the current messages might be acceptable.
As per coding guidelines
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
internal/controller/rbac/function/roles/reconciler.go(1 hunks)internal/controller/rbac/function/roles/reconciler_test.go(1 hunks)internal/controller/rbac/function/roles/roles.go(1 hunks)internal/controller/rbac/function/roles/roles_test.go(1 hunks)internal/controller/rbac/provider/roles/reconciler.go(6 hunks)internal/controller/rbac/provider/roles/reconciler_test.go(8 hunks)internal/controller/rbac/provider/roles/roles.go(6 hunks)internal/controller/rbac/roles/roles.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/rbac/function/roles/reconciler_test.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: Enforce Crossplane-specific patterns: Use crossplane-runtime/pkg/errors
for wrapping. Check variable naming (short for local scope, descriptive
for wider scope). Ensure 'return early' pattern. Verify error scoping
(declare in conditionals when possible). For nolint directives, require
specific linter names and explanations. CRITICAL: Ensure all error
messages are meaningful to end users, not just developers - avoid
technical jargon, include context about what the user was trying to do,
and suggest next steps when possible.
Files:
internal/controller/rbac/provider/roles/reconciler.gointernal/controller/rbac/roles/roles.gointernal/controller/rbac/provider/roles/reconciler_test.gointernal/controller/rbac/function/roles/roles.gointernal/controller/rbac/provider/roles/roles.gointernal/controller/rbac/function/roles/roles_test.gointernal/controller/rbac/function/roles/reconciler.go
**/internal/controller/**
⚙️ CodeRabbit configuration file
**/internal/controller/**: Review controller logic for proper reconciliation patterns, error
handling, and resource management. Pay special attention to conditions
and events: Conditions must be actionable for users (not developers),
stable/deterministic, with proper Type/Reason/Message format. Events
only when something actually happens, with specific details about what
changed. No transient errors in conditions/events. All error messages
must be meaningful to end users - include context about what
resource/operation failed and why.
Files:
internal/controller/rbac/provider/roles/reconciler.gointernal/controller/rbac/roles/roles.gointernal/controller/rbac/provider/roles/reconciler_test.gointernal/controller/rbac/function/roles/roles.gointernal/controller/rbac/provider/roles/roles.gointernal/controller/rbac/function/roles/roles_test.gointernal/controller/rbac/function/roles/reconciler.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Enforce table-driven test structure: PascalCase test names (no
underscores), args/want pattern, use cmp.Diff with
cmpopts.EquateErrors() for error testing. Check for proper test case
naming and reason fields. Ensure no third-party test frameworks (no
Ginkgo, Gomega, Testify).
Files:
internal/controller/rbac/provider/roles/reconciler_test.gointernal/controller/rbac/function/roles/roles_test.go
🧬 Code graph analysis (7)
internal/controller/rbac/provider/roles/reconciler.go (1)
internal/controller/rbac/roles/roles.go (3)
Resource(70-76)DefinedResources(79-116)ClusterRolesDiffer(121-128)
internal/controller/rbac/roles/roles.go (1)
apis/apiextensions/v1alpha1/register.go (1)
ManagedResourceDefinitionKind(53-53)
internal/controller/rbac/provider/roles/reconciler_test.go (3)
internal/controller/rbac/function/roles/reconciler.go (2)
WithClusterRoleRenderer(116-120)ClusterRoleRenderFn(64-64)internal/controller/rbac/provider/roles/reconciler.go (2)
WithClusterRoleRenderer(138-142)ClusterRoleRenderFn(67-67)internal/controller/rbac/roles/roles.go (3)
Resource(70-76)DefinedResources(79-116)ClusterRolesDiffer(121-128)
internal/controller/rbac/function/roles/roles.go (2)
internal/controller/rbac/provider/roles/roles.go (1)
RenderClusterRoles(78-176)internal/controller/rbac/roles/roles.go (12)
Resource(70-76)SuffixStatus(57-57)NameSuffixEdit(36-36)KeyAggregateToCrossplane(42-42)ValTrue(54-54)KeyAggregateToAdmin(45-45)KeyAggregateToEdit(48-48)WithVerbs(131-139)VerbsEdit(63-63)NameSuffixView(39-39)KeyAggregateToView(51-51)VerbsView(66-66)
internal/controller/rbac/provider/roles/roles.go (1)
internal/controller/rbac/roles/roles.go (12)
VerbsEdit(63-63)Resource(70-76)SuffixStatus(57-57)NameSuffixEdit(36-36)KeyAggregateToCrossplane(42-42)ValTrue(54-54)KeyAggregateToAdmin(45-45)KeyAggregateToEdit(48-48)WithVerbs(131-139)NameSuffixView(39-39)KeyAggregateToView(51-51)VerbsView(66-66)
internal/controller/rbac/function/roles/roles_test.go (2)
internal/controller/rbac/roles/roles.go (14)
NameSuffixEdit(36-36)NameSuffixView(39-39)Resource(70-76)KeyAggregateToCrossplane(42-42)ValTrue(54-54)KeyAggregateToAdmin(45-45)KeyAggregateToEdit(48-48)SuffixStatus(57-57)VerbsEdit(63-63)KeyAggregateToView(51-51)VerbsView(66-66)ClusterRolesDiffer(121-128)WithVerbs(131-139)DefinedResources(79-116)internal/controller/rbac/function/roles/roles.go (1)
RenderClusterRoles(39-107)
internal/controller/rbac/function/roles/reconciler.go (2)
internal/controller/rbac/function/roles/roles.go (1)
RenderClusterRoles(39-107)internal/controller/rbac/roles/roles.go (3)
Resource(70-76)DefinedResources(79-116)ClusterRolesDiffer(121-128)
🔇 Additional comments (15)
internal/controller/rbac/function/roles/roles_test.go (2)
64-256: Solid coverage for render paths and determinism.Covers owner refs, labels, per-group rule merging, and sorting well. Nicely mirrors provider roles behavior.
371-398: Great immutability check for WithVerbs.Ensures input rules aren’t mutated. Simple and effective.
internal/controller/rbac/function/roles/roles.go (1)
71-99: Function RBAC roles: implementation looks correct and mirrors provider logic.Deterministic rule gen, correct labels, and owner refs; no system role (as intended). Nice reuse of shared helpers.
internal/controller/rbac/provider/roles/reconciler_test.go (1)
160-167: Tests aligned with shared roles utilities.Signatures and helpers updated correctly; error wrapping includes role name.
Also applies to: 184-187, 226-229, 254-257
internal/controller/rbac/provider/roles/roles.go (1)
99-109: Overall migration to shared roles helpers looks good.Correct use of roles.SuffixStatus, NameSuffixes, WithVerbs, and label keys; owner refs set after building slice.
Also applies to: 120-147, 169-176
internal/controller/rbac/provider/roles/reconciler.go (1)
227-287: RBAC reconciliation updates look correct.Using roles.DefinedResources and ClusterRolesDiffer with dedup improves correctness and avoids noisy applies. Event reasons and Wrapf usage are appropriate.
Also applies to: 294-311
internal/controller/rbac/roles/roles.go (4)
1-76: LGTM! Clean shared utilities foundation.The package structure, constants, and
Resourcetype provide a clear, well-documented foundation for RBAC role management. The choice to extract these as shared utilities reduces duplication across provider and function RBAC code.
78-116: Verify the defensive error handling approach.The function silently skips malformed resources (invalid
APIVersionat line 86, invalid CRD names at line 99-103) and returns partial results. This defensive approach is well-documented with comments explaining the edge cases.Could you confirm this is the intended behavior? An alternative would be to log warnings or emit events when skipping resources, which might help users debug why expected RBAC rules aren't being created.
To verify how often these edge cases occur in practice, you could add debug logging:
gv, _ := schema.ParseGroupVersion(ref.APIVersion) + if err != nil { + log.Debug("Skipping resource with malformed APIVersion", "ref", ref) + }And similarly for the CRD name parsing. What do you think?
118-128: LGTM! Appropriate use of type assertion and deep equality.The force type assertion is well-justified by the comment—calling with non-ClusterRole objects would be a programming error. Using
cmp.Equalfor deep comparison of labels and rules is the right approach for detecting meaningful differences.
130-139: LGTM! Correctly creates independent copies.The function properly creates a new slice and copies each rule before updating verbs, preventing unintended mutations of the input slice. This defensive approach is appropriate for a utility function.
internal/controller/rbac/function/roles/reconciler.go (5)
1-87: LGTM! Well-structured controller setup.The controller setup follows Crossplane conventions:
- Clear interface abstraction for rendering (
ClusterRoleRenderer)- Function adapter for flexibility (
ClusterRoleRenderFn)- Proper controller registration with ownership tracking and rate limiting
- Conflict-aware reconciliation with
errors.WithSilentRequeueOnConflict
122-144: Address or clarify the TODO about the Updating applicator.Line 125 contains a TODO questioning whether
NewAPIUpdatingApplicatoris appropriate. This applicator performs optimistic updates (GET, modify, UPDATE) which is suitable when you need to preserve fields set by other controllers or webhooks.For ClusterRoles owned by FunctionRevisions, could you clarify:
- Are there other controllers or webhooks modifying these ClusterRoles?
- If not, would
NewAPIPatchingApplicatorbe more efficient (avoids the GET)?If Updating is indeed appropriate, please update the comment to explain why rather than leaving it as a question.
159-175: Verify the reconciliation timeout is sufficient.A 2-minute timeout (line 165) is applied to the entire reconciliation cycle. For FunctionRevisions with many CRDs, rendering and applying multiple ClusterRoles could potentially exceed this limit on slower clusters or under load.
Consider:
- How many ClusterRoles are typically created per FunctionRevision? (Currently 2: edit and view)
- Could this scale in the future?
- Have you tested this timeout under realistic conditions?
If 2 minutes proves insufficient, you might need to either increase it or remove the timeout and rely on the controller's natural timeout mechanisms.
199-229: LGTM! Robust apply loop with proper error handling.The ClusterRole application logic is well-structured:
- Properly checks ownership and skips no-ops (lines 204-210)
- Handles conflicts gracefully by requeuing (lines 214-215)
- Emits events on failures (line 219)
- Tracks applied roles for summary reporting (lines 224-228)
The silent requeue on conflict is appropriate since conflicts are expected in distributed systems and don't represent actionable errors.
231-239: LGTM! Good summary event and noted TODO.The summary event (line 232) using
resource.StableNAndSomeMoreprovides useful feedback to users about which ClusterRoles were created/updated.The TODO about adding a condition to the FunctionRevision (lines 235-236) is reasonable. While adding a condition would provide better observability, the current implementation is functional and this can be addressed in a follow-up if needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/controller/rbac/function/roles/roles_test.go (1)
400-492: Consider consolidating shared function tests.
TestDefinedResourcestests the sharedroles.DefinedResourcesfunction, which is also tested ininternal/controller/rbac/provider/roles/reconciler_test.go(line 281). Since this is a shared utility function, consider moving these tests to a dedicated test file in theinternal/controller/rbac/rolespackage to avoid duplication and ensure the function is tested in one canonical location.internal/controller/rbac/roles/roles.go (1)
60-67: Consider documenting the nolint directive for exported vars.The
//nolint:gochecknoglobalsdirective is used with the comment "We treat these as constants." While the intent is clear, it might be helpful to explain why these are vars instead of const (since slices can't be const in Go).Something like:
-//nolint:gochecknoglobals // We treat these as constants. +// nolint:gochecknoglobals // These are treated as constants but must be vars since Go doesn't support const slices. var (
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cluster/charts/crossplane/templates/rbac-manager-clusterrole.yaml(1 hunks)internal/controller/rbac/function/roles/fuzz_test.go(1 hunks)internal/controller/rbac/function/roles/reconciler.go(1 hunks)internal/controller/rbac/function/roles/reconciler_test.go(1 hunks)internal/controller/rbac/function/roles/roles.go(1 hunks)internal/controller/rbac/function/roles/roles_test.go(1 hunks)internal/controller/rbac/provider/roles/fuzz_test.go(2 hunks)internal/controller/rbac/provider/roles/reconciler.go(6 hunks)internal/controller/rbac/provider/roles/reconciler_test.go(8 hunks)internal/controller/rbac/provider/roles/roles.go(6 hunks)internal/controller/rbac/provider/roles/roles_test.go(12 hunks)internal/controller/rbac/rbac.go(1 hunks)internal/controller/rbac/roles/roles.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/controller/rbac/function/roles/reconciler_test.go
- cluster/charts/crossplane/templates/rbac-manager-clusterrole.yaml
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: Enforce Crossplane-specific patterns: Use crossplane-runtime/pkg/errors
for wrapping. Check variable naming (short for local scope, descriptive
for wider scope). Ensure 'return early' pattern. Verify error scoping
(declare in conditionals when possible). For nolint directives, require
specific linter names and explanations. CRITICAL: Ensure all error
messages are meaningful to end users, not just developers - avoid
technical jargon, include context about what the user was trying to do,
and suggest next steps when possible.
Files:
internal/controller/rbac/function/roles/roles.gointernal/controller/rbac/function/roles/roles_test.gointernal/controller/rbac/provider/roles/reconciler_test.gointernal/controller/rbac/function/roles/fuzz_test.gointernal/controller/rbac/provider/roles/fuzz_test.gointernal/controller/rbac/rbac.gointernal/controller/rbac/roles/roles.gointernal/controller/rbac/function/roles/reconciler.gointernal/controller/rbac/provider/roles/roles_test.gointernal/controller/rbac/provider/roles/roles.gointernal/controller/rbac/provider/roles/reconciler.go
**/internal/controller/**
⚙️ CodeRabbit configuration file
**/internal/controller/**: Review controller logic for proper reconciliation patterns, error
handling, and resource management. Pay special attention to conditions
and events: Conditions must be actionable for users (not developers),
stable/deterministic, with proper Type/Reason/Message format. Events
only when something actually happens, with specific details about what
changed. No transient errors in conditions/events. All error messages
must be meaningful to end users - include context about what
resource/operation failed and why.
Files:
internal/controller/rbac/function/roles/roles.gointernal/controller/rbac/function/roles/roles_test.gointernal/controller/rbac/provider/roles/reconciler_test.gointernal/controller/rbac/function/roles/fuzz_test.gointernal/controller/rbac/provider/roles/fuzz_test.gointernal/controller/rbac/rbac.gointernal/controller/rbac/roles/roles.gointernal/controller/rbac/function/roles/reconciler.gointernal/controller/rbac/provider/roles/roles_test.gointernal/controller/rbac/provider/roles/roles.gointernal/controller/rbac/provider/roles/reconciler.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Enforce table-driven test structure: PascalCase test names (no
underscores), args/want pattern, use cmp.Diff with
cmpopts.EquateErrors() for error testing. Check for proper test case
naming and reason fields. Ensure no third-party test frameworks (no
Ginkgo, Gomega, Testify).
Files:
internal/controller/rbac/function/roles/roles_test.gointernal/controller/rbac/provider/roles/reconciler_test.gointernal/controller/rbac/function/roles/fuzz_test.gointernal/controller/rbac/provider/roles/fuzz_test.gointernal/controller/rbac/provider/roles/roles_test.go
🧬 Code graph analysis (11)
internal/controller/rbac/function/roles/roles.go (2)
internal/controller/rbac/provider/roles/roles.go (1)
RenderClusterRoles(78-176)internal/controller/rbac/roles/roles.go (12)
Resource(70-76)SuffixStatus(57-57)NameSuffixEdit(36-36)KeyAggregateToCrossplane(42-42)ValTrue(54-54)KeyAggregateToAdmin(45-45)KeyAggregateToEdit(48-48)WithVerbs(131-139)VerbsEdit(63-63)NameSuffixView(39-39)KeyAggregateToView(51-51)VerbsView(66-66)
internal/controller/rbac/function/roles/roles_test.go (2)
internal/controller/rbac/roles/roles.go (14)
NameSuffixEdit(36-36)NameSuffixView(39-39)Resource(70-76)KeyAggregateToCrossplane(42-42)ValTrue(54-54)KeyAggregateToAdmin(45-45)KeyAggregateToEdit(48-48)SuffixStatus(57-57)VerbsEdit(63-63)KeyAggregateToView(51-51)VerbsView(66-66)ClusterRolesDiffer(121-128)WithVerbs(131-139)DefinedResources(79-116)internal/controller/rbac/function/roles/roles.go (1)
RenderClusterRoles(39-107)
internal/controller/rbac/provider/roles/reconciler_test.go (3)
internal/controller/rbac/function/roles/reconciler.go (2)
WithClusterRoleRenderer(116-120)ClusterRoleRenderFn(64-64)internal/controller/rbac/provider/roles/reconciler.go (2)
WithClusterRoleRenderer(138-142)ClusterRoleRenderFn(67-67)internal/controller/rbac/roles/roles.go (3)
Resource(70-76)DefinedResources(79-116)ClusterRolesDiffer(121-128)
internal/controller/rbac/function/roles/fuzz_test.go (4)
internal/controller/rbac/provider/roles/fuzz_test.go (1)
FuzzRenderClusterRoles(28-43)internal/controller/rbac/roles/roles.go (1)
Resource(70-76)internal/controller/rbac/function/roles/roles.go (1)
RenderClusterRoles(39-107)internal/controller/rbac/provider/roles/roles.go (1)
RenderClusterRoles(78-176)
internal/controller/rbac/provider/roles/fuzz_test.go (1)
internal/controller/rbac/roles/roles.go (1)
Resource(70-76)
internal/controller/rbac/rbac.go (1)
internal/controller/rbac/function/roles/reconciler.go (1)
Setup(74-87)
internal/controller/rbac/roles/roles.go (1)
apis/apiextensions/v1alpha1/register.go (1)
ManagedResourceDefinitionKind(53-53)
internal/controller/rbac/function/roles/reconciler.go (3)
internal/controller/rbac/function/roles/roles.go (1)
RenderClusterRoles(39-107)internal/controller/rbac/roles/roles.go (3)
Resource(70-76)DefinedResources(79-116)ClusterRolesDiffer(121-128)internal/controller/rbac/rbac.go (1)
Setup(31-44)
internal/controller/rbac/provider/roles/roles_test.go (1)
internal/controller/rbac/roles/roles.go (11)
NameSuffixEdit(36-36)NameSuffixView(39-39)Resource(70-76)KeyAggregateToCrossplane(42-42)ValTrue(54-54)KeyAggregateToAdmin(45-45)KeyAggregateToEdit(48-48)SuffixStatus(57-57)VerbsEdit(63-63)KeyAggregateToView(51-51)VerbsView(66-66)
internal/controller/rbac/provider/roles/roles.go (1)
internal/controller/rbac/roles/roles.go (12)
VerbsEdit(63-63)Resource(70-76)SuffixStatus(57-57)NameSuffixEdit(36-36)KeyAggregateToCrossplane(42-42)ValTrue(54-54)KeyAggregateToAdmin(45-45)KeyAggregateToEdit(48-48)WithVerbs(131-139)NameSuffixView(39-39)KeyAggregateToView(51-51)VerbsView(66-66)
internal/controller/rbac/provider/roles/reconciler.go (1)
internal/controller/rbac/roles/roles.go (3)
Resource(70-76)DefinedResources(79-116)ClusterRolesDiffer(121-128)
🔇 Additional comments (22)
internal/controller/rbac/provider/roles/fuzz_test.go (1)
25-25: LGTM! Proper use of shared roles package.The fuzz test correctly adopts the shared
roles.Resourcetype, maintaining the same testing behavior while reducing duplication.Also applies to: 34-35
internal/controller/rbac/rbac.go (2)
25-27: Clear import aliasing improves readability.The import aliases distinguish between provider and function roles packages effectively.
35-36: Setup function signatures are consistent. Bothproviderroles.Setupandfunctionroles.Setupimplementfunc(ctrl.Manager, controller.Options) errorand integrate correctly.internal/controller/rbac/function/roles/fuzz_test.go (1)
28-43: Excellent addition of fuzz testing for function roles.The fuzz test follows the established pattern from the provider roles fuzz test and provides valuable coverage for the new
RenderClusterRolesfunction. The early return on empty resources (line 37-39) prevents unnecessary work.internal/controller/rbac/function/roles/roles_test.go (1)
34-266: Comprehensive test coverage for RenderClusterRoles.The test cases cover important scenarios:
- Empty resources handling
- Single resource rendering
- Resource grouping by API group
- Deterministic sorting
The assertions verify owner references, labels, rules, and verbs are correctly set. Well done!
internal/controller/rbac/function/roles/roles.go (4)
35-43: Clear documentation of function RBAC design decision.The comment correctly explains why functions only need edit and view roles (gRPC invocation, no direct K8s API access). This is an important architectural distinction from providers.
45-61: Deterministic resource processing ensures stable ClusterRoles.The sorting (lines 47-49) and deterministic group iteration (line 51, 57) prevent spurious ClusterRole updates caused by reordering. The concatenation
rs[i].Plural+rs[i].Groupprovides a stable sort key.
71-98: ClusterRole generation correctly implements aggregation labels.Both edit and view roles:
- Set appropriate aggregation labels for Crossplane, admin, and edit/view roles
- Apply correct verbs via the shared
roles.WithVerbshelper- Follow the naming convention established for provider roles
The edit role aggregates to Crossplane, admin, and edit (lines 75-84), while the view role only aggregates to view (line 94). This matches the expected RBAC hierarchy.
100-107: Owner references correctly tie ClusterRoles to FunctionRevision lifecycle.Setting the controller owner reference ensures ClusterRoles are garbage collected when the FunctionRevision is deleted. The loop over both edit and view roles is clean and efficient.
internal/controller/rbac/provider/roles/reconciler_test.go (2)
46-46: Consistent adoption of shared roles package in tests.The test mocks correctly use the updated
ClusterRoleRenderFnsignature with[]roles.Resource, ensuring tests remain aligned with the refactored implementation.Also applies to: 160-162, 184-186, 226-228, 254-256
166-166: Verified errFmtApplyRole provides meaningful context. Defined in reconciler.go as"cannot apply ClusterRole %q", it clearly identifies which role failed.internal/controller/rbac/provider/roles/roles.go (2)
30-30: Successful extraction of shared RBAC utilities.The import of the shared
rolespackage and the updatedRenderClusterRolessignature demonstrate effective code reuse. This reduces duplication between provider and function RBAC implementations.Also applies to: 78-78
99-99: Consistent use of shared constants improves maintainability.Replacing local constants and helpers with
roles.SuffixStatus,roles.NameSuffixEdit,roles.NameSuffixView,roles.WithVerbs,roles.VerbsEdit,roles.VerbsView, and aggregation label keys ensures consistent behavior across provider and function RBAC implementations.Also applies to: 122-122, 136-136, 141-141, 146-146, 163-166
internal/controller/rbac/provider/roles/roles_test.go (1)
30-30: Test expectations correctly updated for shared constants.The test cases now reference
roles.Resource,roles.NameSuffixEdit,roles.NameSuffixView,roles.KeyAggregate*,roles.ValTrue,roles.SuffixStatus,roles.VerbsEdit, androles.VerbsView. This ensures tests validate against the same constants used in production code.Also applies to: 47-48, 61-62, 80-93, 101-103, 109-110, 124-125, 131-132, 136-137, 190-192, 198-199, 208-209, 214-215, 228-229, 239-240, 242-243
internal/controller/rbac/provider/roles/reconciler.go (2)
276-286: Deduplication logic looks solid.The deduplication of aggregated resources from provider family members is implemented correctly. Using
roles.Resourceas a map key works well since it's a simple struct with only string fields, making it suitable for map keys and ensuring proper equality comparison.One consideration: this deduplication operates on the final aggregated list. If two family members define the same CRD, we correctly avoid redundant RBAC entries. The slice reuse pattern (
resources[:0]) is also efficient.
43-43: LGTM! Clean refactoring to shared roles package.The migration to use the shared
rolespackage is well-executed:
roles.DefinedResourcesreplaces local resource extraction logicroles.ClusterRolesDifferprovides consistent comparison semantics- Type safety maintained with
[]roles.ResourceThis reduces duplication between provider and function RBAC controllers.
Also applies to: 227-227, 272-272, 296-296
internal/controller/rbac/roles/roles.go (3)
78-116: DefinedResources implementation is thorough and correct.The function properly:
- Filters for CRDs and MRDs using type checks
- Handles malformed APIVersions gracefully (silently skipping, which is documented)
- Extracts group and plural from CRD name format
- Deduplicates by
plural.groupkey to avoid redundant RBAC entriesThe silent handling of
ParseGroupVersionerrors (line 86) is reasonable since malformed APIVersions shouldn't occur in practice, and skipping them is safer than failing the entire operation.
121-128: ClusterRolesDiffer comparison is appropriate.The comparison logic correctly checks:
- Labels equality using
cmp.Equal- Rules equality using
cmp.EqualThe force type assertion is acceptable here since calling this with non-ClusterRole objects is a programming error that should surface immediately (as documented in the comment).
131-139: WithVerbs safely creates modified copies.The function correctly creates a new slice and copies each rule before modifying verbs, preventing mutation of the original rules. This is important since the input rules might be reused elsewhere.
internal/controller/rbac/function/roles/reconciler.go (3)
71-87: LGTM! Clean controller setup.The Setup function correctly:
- Creates a controller for FunctionRevision
- Configures ownership of ClusterRoles for garbage collection
- Integrates rate limiting and conflict handling via
errors.WithSilentRequeueOnConflictThe pattern mirrors the provider reconciler, providing consistency across the codebase.
161-240: Reconcile logic is correct and user-friendly.The reconciliation flow properly handles:
- Resource not found (garbage collected)
- Paused resources (respecting meta.IsPaused)
- Deleted resources (allowing Kubernetes GC to clean up ClusterRoles)
- Conflict errors (requeue without error)
- Apply failures (wrapped with clear context including ClusterRole name)
Error messages follow the coding guidelines by being meaningful to end users (e.g., "cannot apply ClusterRole" with the specific role name).
The use of
roles.DefinedResourcesandroles.ClusterRolesDiffermaintains consistency with the provider reconciler.
125-125: TODO comment about Updating applicator.This TODO about whether
Updatingis appropriate also exists in the provider reconciler. Since both reconcilers use the same pattern and it works correctly, this seems like a question that applies to both controllers.If you'd like, I can help investigate whether
Updatingis indeed the right choice here, or if we should consider alternatives likeCreatingor standardApplysemantics.Would you like me to search the codebase for how other controllers handle ClusterRole application to see if there's an established pattern?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/controller/rbac/provider/roles/roles.go (1)
102-102: Nice optimization!Adding the capacity hint to the slice initialization avoids potential reallocation during the append operations in the following loop.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/controller/rbac/function/roles/roles.go(1 hunks)internal/controller/rbac/provider/roles/roles.go(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: Enforce Crossplane-specific patterns: Use crossplane-runtime/pkg/errors
for wrapping. Check variable naming (short for local scope, descriptive
for wider scope). Ensure 'return early' pattern. Verify error scoping
(declare in conditionals when possible). For nolint directives, require
specific linter names and explanations. CRITICAL: Ensure all error
messages are meaningful to end users, not just developers - avoid
technical jargon, include context about what the user was trying to do,
and suggest next steps when possible.
Files:
internal/controller/rbac/provider/roles/roles.gointernal/controller/rbac/function/roles/roles.go
**/internal/controller/**
⚙️ CodeRabbit configuration file
**/internal/controller/**: Review controller logic for proper reconciliation patterns, error
handling, and resource management. Pay special attention to conditions
and events: Conditions must be actionable for users (not developers),
stable/deterministic, with proper Type/Reason/Message format. Events
only when something actually happens, with specific details about what
changed. No transient errors in conditions/events. All error messages
must be meaningful to end users - include context about what
resource/operation failed and why.
Files:
internal/controller/rbac/provider/roles/roles.gointernal/controller/rbac/function/roles/roles.go
🧬 Code graph analysis (2)
internal/controller/rbac/provider/roles/roles.go (1)
internal/controller/rbac/roles/roles.go (12)
VerbsEdit(63-63)Resource(70-76)SuffixStatus(57-57)NameSuffixEdit(36-36)KeyAggregateToCrossplane(42-42)ValTrue(54-54)KeyAggregateToAdmin(45-45)KeyAggregateToEdit(48-48)WithVerbs(131-139)NameSuffixView(39-39)KeyAggregateToView(51-51)VerbsView(66-66)
internal/controller/rbac/function/roles/roles.go (2)
internal/controller/rbac/provider/roles/roles.go (1)
RenderClusterRoles(78-176)internal/controller/rbac/roles/roles.go (12)
Resource(70-76)SuffixStatus(57-57)NameSuffixEdit(36-36)KeyAggregateToCrossplane(42-42)ValTrue(54-54)KeyAggregateToAdmin(45-45)KeyAggregateToEdit(48-48)WithVerbs(131-139)VerbsEdit(63-63)NameSuffixView(39-39)KeyAggregateToView(51-51)VerbsView(66-66)
🔇 Additional comments (16)
internal/controller/rbac/function/roles/roles.go (6)
1-29: LGTM!Package declaration and imports are appropriate for the RBAC ClusterRole rendering functionality.
31-33: LGTM!The name prefix follows the established Crossplane convention, distinguishing function roles from provider roles.
35-49: LGTM!The early return guard and deterministic sorting are correct. The sorting approach (concatenating Plural+Group) matches the provider implementation and ensures consistent ClusterRole generation across reconciliations.
51-69: LGTM!The resource aggregation logic correctly groups resources by API group and includes status subresources. The approach maintains deterministic ordering through the groups slice.
71-98: LGTM!The ClusterRole definitions are correct. The edit role appropriately aggregates to Crossplane, Admin, and Edit, while the view role aggregates only to View. The omission of a system role is correct—as documented in the comment, functions are invoked via gRPC and don't need direct Kubernetes API access.
100-107: LGTM!The owner reference setup is correct, ensuring the ClusterRoles are properly tied to their owning FunctionRevision lifecycle.
internal/controller/rbac/provider/roles/roles.go (10)
30-30: LGTM!The import of the shared roles package enables the use of common RBAC utilities and constants.
37-37: LGTM!The constant rename to
keyProviderNamemakes the purpose more explicit.
67-67: LGTM!Using the shared
roles.VerbsEditconstant promotes consistency across RBAC implementations.
78-78: LGTM!The function signature now uses the shared
roles.Resourcetype, enabling code reuse across provider and function RBAC implementations.
99-99: LGTM!Using the shared
roles.SuffixStatusconstant maintains consistency with the function RBAC implementation.
122-136: LGTM!The edit ClusterRole now uses shared constants and utilities, maintaining consistency with the function RBAC implementation while preserving the existing aggregation semantics.
141-146: LGTM!The view ClusterRole refactoring mirrors the edit role changes, correctly using shared utilities.
158-158: LGTM!The system role correctly uses the shared
roles.WithVerbshelper while maintaining the provider-specificverbsSystemand finalizers logic.
163-166: LGTM!The SafeStart capability logic correctly uses the shared
roles.WithVerbsandroles.VerbsViewutilities for consistent CRD viewing permissions.
169-175: LGTM!The refactored owner reference logic creates the ClusterRoles slice upfront and then sets owner references in a loop, which aligns with the function roles pattern and maintains clarity.
c614c9c to
19b60b2
Compare
Description of your changes
This PR implements RBAC role management for FunctionRevisions, mirroring the existing ProviderRevision RBAC support. The RBAC manager now automatically creates ClusterRoles for resources defined by Functions.
This is needed because we currently deploy inputs CRDs, so tooling relying on CRDs to understand what's a crossplane resource could still try to get those resources and fail due to the lack of permissions, we could have just omitted CRDs owned by functions, but some Functions come with proper CRDs intended for human consumption, e.g. https://github.com/upbound/function-claude-status-transformer/blob/main/package/input/function-claude-status-transformer.fn.crossplane.io_functionconfigs.yaml, I didn't go as far as setting up RBAC for functions to be able to directly access those, that would be against the spec and they should go through extra-resources.
Key differences from Provider RBAC:
Changes:
Test coverage includes unit tests, fuzz testing, and verification on live cluster with function-patch-and-transform.
I have:
earthly +reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.