Skip to content

feat: implement clusterrole primitive#37

Open
sourcehawk wants to merge 27 commits intomainfrom
feature/clusterrole-primitive
Open

feat: implement clusterrole primitive#37
sourcehawk wants to merge 27 commits intomainfrom
feature/clusterrole-primitive

Conversation

@sourcehawk
Copy link
Owner

@sourcehawk sourcehawk commented Mar 22, 2026

Implements the clusterrole Kubernetes resource primitive following the pattern established by the existing ConfigMap and Deployment primitives.

Summary

  • Adds clusterrole primitive package under pkg/primitives/clusterrole/
  • Implements required lifecycle interfaces
  • Includes editors, mutator, flavors, and builder
  • Adds PolicyRulesEditor to pkg/mutation/editors/ (reusable for Role and ClusterRole)
  • Adds cluster-scoped resource support to internal/generic/builder_base.go

Checklist

  • Compiles cleanly
  • Tests pass
  • Follows naming conventions in CONTEXT.md
  • Shared infrastructure changes are limited to adding cluster-scope support (internal/generic/builder_base.go) and a new PolicyRulesEditor (pkg/mutation/editors/)

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a new clusterrole primitive to the operator-component-framework so controllers can manage Kubernetes rbac.authorization.k8s.io/v1 ClusterRole objects using the same builder/mutator/flavor patterns as existing primitives.

Changes:

  • Introduces pkg/primitives/clusterrole with Resource, Builder, Mutator, and field-application flavors for ClusterRole-specific behavior.
  • Adds a new shared mutation editor PolicyRulesEditor under pkg/mutation/editors to support typed .rules mutation.
  • Adds an end-to-end example (examples/clusterrole-primitive) and full primitive documentation (docs/primitives/clusterrole.md).

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
pkg/primitives/clusterrole/resource.go Defines the ClusterRole Resource wrapper and default field applicator.
pkg/primitives/clusterrole/mutator.go Implements plan-and-apply mutator for metadata, rules, and aggregationRule.
pkg/primitives/clusterrole/builder.go Provides fluent builder API and validation/workarounds for cluster-scoped objects.
pkg/primitives/clusterrole/flavors.go Adds flavors to preserve labels/annotations and externally-added rules.
pkg/primitives/clusterrole/*_test.go Adds unit tests for builder, mutator, and flavors.
pkg/mutation/editors/policyrules.go Adds a reusable PolicyRulesEditor for Role/ClusterRole rules mutation.
pkg/mutation/editors/policyrules_test.go Adds tests for PolicyRulesEditor.
examples/clusterrole-primitive/* Adds a runnable example demonstrating mutation composition and reconciliation.
docs/primitives/clusterrole.md Adds user-facing documentation for the new primitive API and usage patterns.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • Updated PR description checklist to accurately reflect that shared infrastructure files are modified (internal/generic/builder_base.go for cluster-scope support, pkg/mutation/editors/policyrules.go as a new reusable editor). The old checklist item "Does not modify shared files" has been replaced with a specific description of the shared infrastructure changes.

Intentionally ignored:
None

<!-- claude-review-cycle -->

@sourcehawk
Copy link
Owner Author

approved but wait until last

Copilot AI review requested due to automatic review settings March 23, 2026 00:18
@sourcehawk sourcehawk force-pushed the feature/clusterrole-primitive branch from 102978d to a1d3fca Compare March 23, 2026 00:18
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • Removed duplicate "valid clusterrole without namespace" test case in builder_test.go (identical to "valid clusterrole")
  • Documented cluster-scoped ownership limitation in docs/primitives/clusterrole.md (namespaced owner cannot own a ClusterRole)
  • Added comment in examples/clusterrole-primitive/main.go explaining the namespaced owner limitation with real clusters

Intentionally ignored:

  • resource.go line 28: component.DataExtractable is the consistent convention used across all primitives (deployment, configmap, clusterrole). Changing to concepts.DataExtractable would break codebase consistency.
  • builder_base.go line 96: Valid architectural concern about SetControllerReference failing for cluster-scoped dependents with namespaced owners, but this requires a framework-level change (e.g., adding SkipOwnerReference to ResourceOptions and threading it through the reconciliation path). Documented the limitation instead; framework fix should be tracked as a separate issue.

<!-- claude-review-cycle -->

Copilot AI review requested due to automatic review settings March 23, 2026 03:00
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 7 comments.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • docs/primitives/clusterrole.md line 7: Updated ownership limitation note to reflect actual framework behavior (scope mismatch is detected, owner ref is skipped with a log message, reconciliation proceeds without GC ownership)
  • docs/primitives/clusterrole.md line 319: Removed PreserveExternalRules from the feature-gating walkthrough example and added a documentation note explaining the trade-off between PreserveExternalRules and feature-gated mutations
  • examples/clusterrole-primitive/resources/clusterrole.go line 40: Removed PreserveExternalRules from the example to avoid conflict with feature-gated mutation add/remove behavior
  • examples/clusterrole-primitive/main.go line 40: Updated comments to describe the actual scope-mismatch behavior (skip with log) rather than implying reconciliation is rejected
  • examples/clusterrole-primitive/README.md line 10: Removed the PreserveExternalRules bullet from the feature list to match the updated example code
  • pkg/primitives/clusterrole/mutator.go line 30: Adopted the per-feature planning pattern (beginFeature/featurePlan) consistent with ConfigMap and Deployment primitives, so mutation ordering respects registration order across features
  • internal/generic/builder_static_test.go line 113: Removed duplicate cluster-scoped validation subtests (kept the testify require/assert versions)

Intentionally ignored:
None

<!-- claude-review-cycle -->

Copilot AI review requested due to automatic review settings March 23, 2026 16:02
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Copilot AI review requested due to automatic review settings March 23, 2026 16:59
@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • PreserveExternalRules slice-order equality (flavors.go:60): Replaced strict slice-order comparison with set-based (sort-then-compare) equality in policyRuleEqual, so reordered-but-equivalent rules from admission webhooks are correctly recognized as duplicates. Added test covering this case.
  • SetAggregationRule pointer aliasing (mutator.go:100): Added DeepCopy of the *AggregationRule at registration time so the mutator records intent immutably, consistent with AddRule which records by value. Added test verifying caller mutation after registration does not affect the applied result.
  • Build() duplicate validation (builder.go:106): Removed local nil-object and empty-name checks, delegating entirely to generic.StaticBuilder.Build()ValidateBase(), consistent with the ConfigMap primitive. Removed unused errors import.

Intentionally ignored:
None

<!-- claude-review-cycle -->

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.

sourcehawk and others added 3 commits March 23, 2026 20:16
Introduces a typed editor for mutating the .rules field of Role and
ClusterRole resources, following the same pattern as existing editors
(ConfigMapDataEditor, ObjectMetaEditor). Provides AddRule,
RemoveRuleByIndex, Clear, and Raw escape hatch operations.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Implements the ClusterRole primitive as a Static, cluster-scoped resource.
Includes builder (with name-only validation, no namespace required),
resource, mutator (EditObjectMetadata, EditRules, AddRule,
SetAggregationRule), and flavors (PreserveCurrentLabels,
PreserveCurrentAnnotations, PreserveExternalRules).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Documents the ClusterRole primitive API, mutation system, editors,
flavors, aggregation rule support, and usage patterns.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

…ot set semantics

The docs claimed slice fields were compared as "unordered sets" but the
implementation treats duplicates as significant (multiset equality).
Updated comments to accurately describe the behavior rather than changing
the comparison logic.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@sourcehawk
Copy link
Owner Author

Claude Review Cycle 2 Complete

Addressed:

  • flavors.go:72 — Updated PreserveExternalRules and policyRuleEqual doc comments to accurately describe the comparison as order-insensitive with significant duplicates (multiset equality), rather than incorrectly claiming set semantics.

Intentionally ignored:
None

<!-- claude-review-cycle -->

@sourcehawk sourcehawk requested review from Copilot and removed request for Copilot March 24, 2026 02:12
@sourcehawk sourcehawk requested review from Copilot and removed request for Copilot March 24, 2026 02:12
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

sourcehawk and others added 4 commits March 24, 2026 17:51
Align with configmap and deployment mutators — NewMutator no longer
creates an initial feature plan. BeginFeature must be called before
registering any mutations. Updates all tests accordingly and adds
constructor/plan invariant tests.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tive

Align with the framework's move to Server-Side Apply. Remove
DefaultFieldApplicator, WithCustomFieldApplicator,
WithFieldApplicationFlavor, the flavors.go file, and all associated
tests. Update builder to use the new generic constructor signature
(without defaultApplicator). Update Mutate tests to use Object() output
instead of empty structs. Strip field applicator and flavor sections
from primitive docs.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 25, 2026 14:07
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

| `pkg/primitives/deployment` | Workload | [deployment.md](primitives/deployment.md) |
| `pkg/primitives/configmap` | Static | [configmap.md](primitives/configmap.md) |
| `pkg/primitives/clusterrole` | Static | [clusterrole.md](primitives/clusterrole.md) |

Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

make run-examples currently runs a hard-coded list of examples and does not include the new clusterrole-primitive example (verified in Makefile:122-126). To keep CI/local smoke runs covering all examples, add go run ./examples/clusterrole-primitive/. to the run-examples target (or update docs/README to clarify it’s intentionally excluded).

Suggested change
The `clusterrole` primitive is exercised by the `examples/clusterrole-primitive` example. Because it requires
cluster-scoped RBAC and may need elevated permissions, it is intentionally not included in the default
`make run-examples` target used for CI/local smoke runs.

Copilot uses AI. Check for mistakes.
Comment on lines +11 to +18
// Builder is a configuration helper for creating and customizing a ClusterRole Resource.
//
// It provides a fluent API for registering mutations and data extractors.
// Build() validates the configuration and returns an initialized Resource
// ready for use in a reconciliation loop.
type Builder struct {
base *generic.StaticBuilder[*rbacv1.ClusterRole, *Mutator]
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

The PR description says the clusterrole primitive includes “flavors”, but there’s no flavor-related API/implementation in pkg/primitives/clusterrole (no Flavor references, and the builder only exposes mutations/data extractors). Either update the PR description to match what’s implemented or add the intended flavor support so the change set aligns with the stated scope.

Copilot uses AI. Check for mistakes.
Comment on lines +48 to +53
func (e *PolicyRulesEditor) RemoveRuleByIndex(index int) {
if index < 0 || index >= len(*e.rules) {
return
}
*e.rules = append((*e.rules)[:index], (*e.rules)[index+1:]...)
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

RemoveRuleByIndex uses the append(slice[:i], slice[i+1:]...) pattern, which can keep the removed element’s backing array referenced (and thus retain memory for any nested slices in rbacv1.PolicyRule). Since this repo already uses the slices package elsewhere, prefer slices.Delete(*e.rules, index, index+1) (or explicitly zero the removed slot) to avoid accidental retention and keep deletion semantics consistent.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants