Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion pkg/rbac/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ type Rule struct {
// Namespace specifies the scope of the Rule.
// If not set, the Rule belongs to the generated ClusterRole.
// If set, the Rule belongs to a Role, whose namespace is specified by this field.
// The generated Role name will be suffixed with the namespace (e.g., "manager-role-namespace")
// to ensure uniqueness when multiple namespace-scoped Roles are generated. This suffix is
// ONLY applied to namespace-scoped Roles, not to ClusterRoles.
Namespace string `marker:",optional"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this break everyone using this part of controller-gen today?

Copy link
Member Author

@camilamacedo86 camilamacedo86 Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change shouldn’t break anything functionally. The RBAC files will still be re-generated; the only delta is the Role name. If someone is currently using namespace-scoped RBAC, the generated Role will now include a namespace suffix.

Honestly, I think this is a very rare use case. If the marker-based namespace RBAC generation were common, we likely would have seen this reported earlier—this is the first time it’s come up. ( it cannot work with kustomize/kubebuilder and nobody raised the issue before )

In this case we’re only changing the resource name. The main way this could “break” someone is if they have external tooling or scripts that reference the old Role name by string (e.g., CI checks, dashboards, custom automation). Otherwise, I think it should be very unlikely to cause issues.

Copy link
Member

@sbueringer sbueringer Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it lead to duplicate objects if folks still have roles with the old name around and now the name changes?

I'm not sure if we can assume that the files generated by controller-gen are always consumed by kustomize

Generally speaking Roles are usually referenced by name by RoleBindings

Copy link
Member Author

@camilamacedo86 camilamacedo86 Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we can’t assume controller-gen output is always consumed by Kustomize. That said, I think the most common adoption path is still Kubebuilder/Operator SDK + Kustomize, where controller-gen is part of the standard workflow.

Also, I don’t think the namespace= RBAC marker is used very often (which may explain why this wasn’t caught earlier). GitHub search suggests ~1.7k usages of namespace= vs ~48.1k total RBAC marker usages:

Given that context, expecting generated resources to have unique names seems reasonable. The key question for me is:

1. How do we ensure controller-gen never generates two resources with the same name here?
2. IMO, generating duplicate resource names is a bug.

What’s your suggested fix?
If a breaking change isn’t acceptable even to fix a bug, what alternative do you propose? Should we introduce a new marker resource name? What would to be acceptable?

PS: There are 23k+ refs using the latest Kubebuilder layouts (which promote controller-gen) based on go.kubebuilder.io references:
https://github.com/search?q=go.kubebuilder.io&type=code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened an alternative PR that addresses the breaking change concerns raised here: #1334

Instead of automatically appending namespace suffixes, my approach adds an optional roleName parameter to the RBAC marker:

// +kubebuilder:rbac:groups=apps,namespace=infrastructure,roleName=infra-manager,resources=deployments,verbs=get;list
// +kubebuilder:rbac:groups="",namespace=users,roleName=user-secrets,resources=secrets,verbs=get

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Thx for looking into how this can be done without a breaking change

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

}

Expand Down Expand Up @@ -347,7 +350,7 @@ func GenerateRoles(ctx *genall.GenerationContext, roleName string) ([]any, error
APIVersion: rbacv1.SchemeGroupVersion.String(),
},
ObjectMeta: metav1.ObjectMeta{
Name: roleName,
Name: fmt.Sprintf("%s-%s", roleName, ns),
Namespace: ns,
},
Rules: policyRules,
Expand Down
2 changes: 2 additions & 0 deletions pkg/rbac/testdata/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,5 @@ package controller
// +kubebuilder:rbac:groups=core;"";some-other-to-deduplicate-with-core,resources=me,verbs=list;get
// +kubebuilder:rbac:groups=deduplicate-groups5,resources=abc,verbs=get;update;patch;create,namespace=here
// +kubebuilder:rbac:groups=deduplicate-groups5,resources=abc,verbs=*,namespace=here
// +kubebuilder:rbac:groups=apps,namespace=infrastructure,resources=deployments,verbs=get;list;watch;update;patch
// +kubebuilder:rbac:groups="",namespace=users,resources=secrets,verbs=get;list;watch
39 changes: 35 additions & 4 deletions pkg/rbac/testdata/role.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ rules:
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: manager-role
name: manager-role-here
namespace: here
rules:
- apiGroups:
Expand All @@ -145,7 +145,24 @@ rules:
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: manager-role
name: manager-role-infrastructure
namespace: infrastructure
rules:
- apiGroups:
- apps
resources:
- deployments
verbs:
- get
- list
- patch
- update
- watch
---
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: manager-role-park
namespace: park
rules:
- apiGroups:
Expand All @@ -158,7 +175,22 @@ rules:
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: manager-role
name: manager-role-users
namespace: users
rules:
- apiGroups:
- ""
resources:
- secrets
verbs:
- get
- list
- watch
---
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: manager-role-zoo
namespace: zoo
rules:
- apiGroups:
Expand All @@ -168,4 +200,3 @@ rules:
- jobs
verbs:
- get

2 changes: 1 addition & 1 deletion pkg/rbac/zz_generated.markerhelp.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading