Conversation
github-actions
bot
added
documentation
📊 Output Delta ReportGenerated RBAC resources from Prometheus Metrics (PR branch)📈 auth_operator_* metrics
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
MaxRink
changed the title
📊 Output Delta Report (cont.)📦 RestrictedRoleDefinitions StatusChanges from
|
MaxRink
added a commit
that referenced
this pull request
Mar 30, 2026
- Return empty slice instead of nil from EvaluateRoleDefinition when no limits - Use DefinitionClusterRole constant instead of string literal - Handle wildcard '*' in restrictedVerbs via containsStringOrWildcard helper - Fix format string bug in restricted_helpers.go: pass joined string to MarkFalse - Use policy.ViolationStrings() to eliminate duplicate string conversion - Add generic ReadyCondition constant, deprecate WebhookAuthorizerReadyCondition - Update restricted CRD tests to use ReadyCondition - Update role_evaluator_test to use constants and add wildcard test case - Improve validateConcurrency to include flag names in error messages - Fix misleading comments in webhook, e2e tests, and debug report
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 110 out of 159 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
api/authorization/v1alpha1/applyconfiguration/ssa/ssa.go:1
- BindDefinitionStatusFrom now explicitly initializes some list fields (GeneratedServiceAccounts, ExternalServiceAccounts) to ensure SSA can clear previously-populated lists, but MissingRoleRefs is not initialized. If MissingRoleRefs was previously non-empty and later becomes empty, SSA will likely not clear it because the field will be omitted (nil) from the apply payload. Fix by initializing result.MissingRoleRefs to an empty slice (capacity len(status.MissingRoleRefs)) before appending, consistent with the RestrictedBindDefinitionStatusFrom pattern.
// SPDX-FileCopyrightText: 2026 Deutsche Telekom AG
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 110 out of 159 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
api/authorization/v1alpha1/applyconfiguration/ssa/ssa.go:1
BindDefinitionStatusFromnow ensures SSA can cleargeneratedServiceAccountsandexternalServiceAccounts, but it does not do the same formissingRoleRefs. IfMissingRoleRefswas previously populated and later becomes empty, the field may be omitted from the apply payload and never cleared. Initialiseresult.MissingRoleRefsto an empty slice (even when zero-length) before appending, consistent with the other list fields.
// SPDX-FileCopyrightText: 2026 Deutsche Telekom AG
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 110 out of 159 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
api/authorization/v1alpha1/applyconfiguration/ssa/ssa.go:1
MissingRoleRefsis not initialised to an empty slice when it’s empty, so SSA may not clear a previously-populatedstatus.missingRoleRefslist (it will remain on the object if the field is omitted). To make list-clearing consistent with the new behavior forGeneratedServiceAccounts/ExternalServiceAccounts, initialiseresult.MissingRoleRefsto an empty slice before appending (same pattern used later for RestrictedBindDefinitionStatusFrom).
// SPDX-FileCopyrightText: 2026 Deutsche Telekom AG
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 110 out of 159 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
REUSE.toml:1
- Narrowing markdown coverage from
docs/**/*.mdto a few specific subpaths can unintentionally exclude existing or future markdown under otherdocs/subdirectories from REUSE checks (leading to compliance failures or unmanaged license metadata). If the intent is “all docs markdown except proposals”, consider restoring a broader glob (e.g.docs/**/*.md) with explicit exclusions, or add any other docs subfolders that exist in this repo.
# SPDX-FileCopyrightText: 2026 Deutsche Telekom AG
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 110 out of 159 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
api/authorization/v1alpha1/applyconfiguration/ssa/ssa.go:1
- BindDefinitionStatusFrom initializes some slices to empty to allow SSA to clear previously-set values, but it does not do this for MissingRoleRefs. If MissingRoleRefs was previously non-empty and later becomes empty, SSA will omit the field and the old values may persist (and/or cause repeated status apply attempts). Initialize result.MissingRoleRefs to an empty slice (even when len==0) before appending, similar to GeneratedServiceAccounts/ExternalServiceAccounts.
// SPDX-FileCopyrightText: 2026 Deutsche Telekom AG
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 110 out of 159 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
api/authorization/v1alpha1/applyconfiguration/ssa/ssa.go:1
MissingRoleRefsis not initialized to an empty slice before appending. With SSA, omitting the field when the list becomes empty can prevent clearing a previously populatedstatus.missingRoleRefsarray, leaving stale status visible to users. Align this with the approach used forGeneratedServiceAccounts/ExternalServiceAccountsby initializingresult.MissingRoleRefs = make([]string, 0, len(status.MissingRoleRefs))before the loop so SSA can clear the field.
// SPDX-FileCopyrightText: 2026 Deutsche Telekom AG
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 110 out of 159 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
api/authorization/v1alpha1/applyconfiguration/ssa/ssa.go:1
- BindDefinitionStatusFrom now initializes some slice fields (GeneratedServiceAccounts, ExternalServiceAccounts) to empty to ensure SSA can clear previously-populated arrays, but MissingRoleRefs is still only set when non-empty. With SSA, omitting the field can leave stale values behind. Initialize result.MissingRoleRefs to an empty slice (even when status.MissingRoleRefs is empty) before appending, mirroring the other list fields.
// SPDX-FileCopyrightText: 2026 Deutsche Telekom AG
This was referenced Apr 7, 2026
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 110 out of 159 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
api/authorization/v1alpha1/applyconfiguration/ssa/ssa.go:1
- BindDefinitionStatusFrom initializes some list fields to an empty slice to allow SSA to clear previously-populated values, but
MissingRoleRefsis not initialized. IfMissingRoleRefstransitions from non-empty to empty, SSA may not clear the old list and status can remain stale. Initializeresult.MissingRoleRefsto an empty slice (capacity len(status.MissingRoleRefs)) before appending, consistent with the other list fields.
// SPDX-FileCopyrightText: 2026 Deutsche Telekom AG
api/authorization/v1alpha1/applyconfiguration/authorization/v1alpha1/rbacpolicyspec.go
Show resolved
Hide resolved
… governance Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
- Use %v (not %s) for client.ObjectKey in error format to render correctly - Add nil check after impersonated client factory call to prevent type assertion panic on (nil, nil) - Use listErrorToAdmission() in RestrictedRoleDefinition webhook for consistent transient error handling - Return NewInvalid with field.ErrorList for duplicate targetName in RoleDefinition webhook (machine-parseable, consistent with immutability errors) - Update test assertion to match NewInvalid response format
…ions Remove dead RBACPolicyFinalizer constant and unused UnifiedSelector type. Add TODO comment on AppliesTo enforcement scope and design comment on MarkReady eventual consistency.
Enables manual triggering of CI and Output Delta workflows, useful for re-running checks when pull_request events don't fire on force-pushes.
- listErrorToAdmission: treat context.DeadlineExceeded/Canceled as transient so webhook timeouts produce a consistent 'please retry' signal - RoleDefinition webhook: add client.Limit(2) to duplicate targetName List calls to cap latency in multi-tenant setups - validateNoDuplicateRestrictedAPIs/RRDAPIs: replace NewBadRequest with NewInvalid+field.Duplicate to give clients precise field-path error output - validatePolicyRefExists: replace NewBadRequest with NewInvalid+field.NotFound targeting spec.policyRef.name for structured field-path errors - roleTargetCollision: reword comment to make the pre-filter precondition explicit - RestrictedBindDefinitionSpec.TargetName: fix format example to remove '/' which is not a valid Kubernetes object name character
...orization/v1alpha1/applyconfiguration/authorization/v1alpha1/restrictedbinddefinitionspec.go
Outdated
Show resolved
Hide resolved
Comment on lines
+39
to
+42
| // ValidateCreate implements admission.Validator for RestrictedRoleDefinition. | ||
| func (v *RestrictedRoleDefinitionValidator) ValidateCreate(ctx context.Context, obj *RestrictedRoleDefinition) (admission.Warnings, error) { | ||
| ctx, cancel := context.WithTimeout(ctx, WebhookCacheTimeout) | ||
| defer cancel() |
There was a problem hiding this comment.
This introduces substantial new admission behavior (policyRef existence checks, cross-type targetName collision detection, immutability enforcement, duplicate restrictedApis rejection, and version-format validation), but no dedicated webhook tests for RestrictedRoleDefinition are shown in this PR. Please add envtest/Ginkgo webhook tests similar to roledefinition_webhook_test.go to cover: (1) targetName collisions within RestrictedRoleDefinition, (2) collisions with RoleDefinition, (3) policyRef not found, (4) immutability on update (targetRole/targetName/targetNamespace/policyRef), (5) duplicate restrictedApis group names, and (6) invalid restrictedApis version strings.
Comment on lines
+51
to
+54
| // Verify that the referenced RBACPolicy exists. | ||
| if err := v.validatePolicyRefExists(ctx, obj); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
This introduces substantial new admission behavior (policyRef existence checks, cross-type targetName collision detection, immutability enforcement, duplicate restrictedApis rejection, and version-format validation), but no dedicated webhook tests for RestrictedRoleDefinition are shown in this PR. Please add envtest/Ginkgo webhook tests similar to roledefinition_webhook_test.go to cover: (1) targetName collisions within RestrictedRoleDefinition, (2) collisions with RoleDefinition, (3) policyRef not found, (4) immutability on update (targetRole/targetName/targetNamespace/policyRef), (5) duplicate restrictedApis group names, and (6) invalid restrictedApis version strings.
Comment on lines
+78
to
+79
| // Enforce immutability of targetRole, targetName, targetNamespace, and policyRef. | ||
| var allErrs field.ErrorList |
There was a problem hiding this comment.
This introduces substantial new admission behavior (policyRef existence checks, cross-type targetName collision detection, immutability enforcement, duplicate restrictedApis rejection, and version-format validation), but no dedicated webhook tests for RestrictedRoleDefinition are shown in this PR. Please add envtest/Ginkgo webhook tests similar to roledefinition_webhook_test.go to cover: (1) targetName collisions within RestrictedRoleDefinition, (2) collisions with RoleDefinition, (3) policyRef not found, (4) immutability on update (targetRole/targetName/targetNamespace/policyRef), (5) duplicate restrictedApis group names, and (6) invalid restrictedApis version strings.
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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the restricted CRD proposal from #56, adding three new cluster-scoped CRDs that enable multi-tenant RBAC governance through policy-constrained role and binding definitions.
New CRDs
Type of Change
Architecture
Key Design Decisions
pkg/policy/): Reusable validation engine withValidate()returning structured[]ViolationresultsFieldOwner("auth-operator"), cache-aware diff-before-applyinternal/controller/authorization/shared_helpers.go): Extracted common patterns (namespace enumeration, SA creation, RBAC cleanup) to reduce duplicationpolicyRefafter creation, policy existence checks, deletion protection for bound policiesReady+PolicyCompliantconditions usingpkg/conditionswith kstatus patternspec.policyRef.namefor efficient policy→resource reverse lookupsImplementation
New Files
api/authorization/v1alpha1/rbacpolicy_types.go— RBACPolicy CRD schemaapi/authorization/v1alpha1/restrictedbinddefinition_types.go— RestrictedBindDefinition CRD schemaapi/authorization/v1alpha1/restrictedroledefinition_types.go— RestrictedRoleDefinition CRD schemaapi/authorization/v1alpha1/rbacpolicy_webhook.go— RBACPolicy validation webhook (deletion protection)api/authorization/v1alpha1/restrictedbinddefinition_webhook.go— RestrictedBindDefinition validation webhookapi/authorization/v1alpha1/restrictedroledefinition_webhook.go— RestrictedRoleDefinition validation webhookinternal/controller/authorization/restrictedroledefinition_controller.go— RestrictedRoleDefinition reconcilerinternal/controller/authorization/restrictedbinddefinition_controller.go— RestrictedBindDefinition reconcilerinternal/controller/authorization/rbacpolicy_controller.go— RBACPolicy reconciler (status tracking)internal/controller/authorization/shared_helpers.go— Shared controller helperspkg/policy/— Policy validation library with comprehensive testsModified Files
cmd/controller.go— Wire new controllers with concurrency flagscmd/webhook.go— Wire new webhook handlerscmd/root.go— Register new types in schemepkg/indexer/— Add field indexes for policyRef lookupspkg/ssa/— Add apply configuration builders for new typesRelated Issues
Closes #56
Testing
Unit Tests
E2E Tests
New
restricted_e2e_test.gocovering:Samples
Comprehensive sample CRs in
config/samples/:authorization_v1alpha1_rbacpolicy.yaml— 4 policies (workload, cross-namespace, CI/CD, compliance)authorization_v1alpha1_restrictedbinddefinition.yaml— 3 bindings (SA-only, platform operator, CI deploy)authorization_v1alpha1_restrictedroledefinition.yaml— 3 roles (namespaced reader, cluster reader, CI deploy)Test Configuration
API Changes
spec.appliesTo,spec.roleLimits,spec.bindingLimits,spec.subjectLimitsspec.policyRef,spec.restrictedApis,spec.restrictedVerbsspec.policyRefCLI Flags
New controller concurrency flags:
--restrictedbinddefinition-concurrency(default: 5)--restrictedroledefinition-concurrency(default: 5)Checklist
Implementation Checklist
make generate)make manifests)pkg/policy/)