Skip to content

CM-1040: Extract shared ApplyResource helper and migrate istiocsr to SSA#420

Open
sebrandon1 wants to merge 1 commit into
openshift:masterfrom
sebrandon1:extract-apply-resource
Open

CM-1040: Extract shared ApplyResource helper and migrate istiocsr to SSA#420
sebrandon1 wants to merge 1 commit into
openshift:masterfrom
sebrandon1:extract-apply-resource

Conversation

@sebrandon1

@sebrandon1 sebrandon1 commented May 6, 2026

Copy link
Copy Markdown
Member

Summary

  • Adds a generic common.ApplyResource[T]() helper for SSA-based reconciliation with pluggable drift detection callbacks
  • Migrates trustmanager's 11 createOrApply methods to use the shared helper
  • Migrates istiocsr's 8 simple createOrApply methods from Create/UpdateWithRetry to SSA via the shared helper
  • Keeps istiocsr's complex ClusterRole/ClusterRoleBinding methods unchanged (they use GenerateName + List fallback + Delete for immutable RoleRef)
  • Drops istioCSRCreateRecon warning events from migrated methods (SSA is inherently idempotent)
  • Net -501 lines of duplicated boilerplate

Test plan

  • go test ./pkg/controller/common/... passes
  • go test ./pkg/controller/istiocsr/... passes
  • go test ./pkg/controller/trustmanager/... passes
  • go build ./... succeeds
  • make lint shows only pre-existing issues (9 total, none introduced)

Summary by CodeRabbit

  • New Features

    • Added a generic server-side-apply applier to centralize declarative resource reconciliation.
  • Refactoring

    • Reconciler flows now use the shared applier, removing manual create/update branching and simplifying call signatures.
    • Event/emission and existence-check logic consolidated for more consistent idempotent behavior.
  • Tests

    • Updated tests to reflect patch/apply semantics and standardized, clearer error messaging.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 6, 2026
@openshift-ci-robot

openshift-ci-robot commented May 6, 2026

Copy link
Copy Markdown

@sebrandon1: This pull request references CM-1040 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Summary

  • Adds a generic common.ApplyResource[T]() helper for SSA-based reconciliation with pluggable drift detection callbacks
  • Migrates trustmanager's 11 createOrApply methods to use the shared helper
  • Migrates istiocsr's 8 simple createOrApply methods from Create/UpdateWithRetry to SSA via the shared helper
  • Keeps istiocsr's complex ClusterRole/ClusterRoleBinding methods unchanged (they use GenerateName + List fallback + Delete for immutable RoleRef)
  • Drops istioCSRCreateRecon warning events from migrated methods (SSA is inherently idempotent)
  • Net -501 lines of duplicated boilerplate

Test plan

  • go test ./pkg/controller/common/... passes
  • go test ./pkg/controller/istiocsr/... passes
  • go test ./pkg/controller/trustmanager/... passes
  • go build ./... succeeds
  • make lint shows only pre-existing issues (9 total, none introduced)

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@coderabbitai

coderabbitai Bot commented May 6, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: a76fd141-11b1-4a38-befa-abaf9bc3b724

📥 Commits

Reviewing files that changed from the base of the PR and between 26c1bb6 and 8b48aaa.

📒 Files selected for processing (26)
  • pkg/controller/common/applier.go
  • pkg/controller/istiocsr/certificates.go
  • pkg/controller/istiocsr/certificates_test.go
  • pkg/controller/istiocsr/constants.go
  • pkg/controller/istiocsr/install_instiocsr_test.go
  • pkg/controller/istiocsr/install_istiocsr.go
  • pkg/controller/istiocsr/networkpolicies.go
  • pkg/controller/istiocsr/rbacs.go
  • pkg/controller/istiocsr/rbacs_test.go
  • pkg/controller/istiocsr/serviceaccounts.go
  • pkg/controller/istiocsr/serviceaccounts_test.go
  • pkg/controller/istiocsr/services.go
  • pkg/controller/istiocsr/services_test.go
  • pkg/controller/trustmanager/certificates.go
  • pkg/controller/trustmanager/certificates_test.go
  • pkg/controller/trustmanager/controller_test.go
  • pkg/controller/trustmanager/deployments.go
  • pkg/controller/trustmanager/deployments_test.go
  • pkg/controller/trustmanager/rbacs.go
  • pkg/controller/trustmanager/rbacs_test.go
  • pkg/controller/trustmanager/serviceaccounts.go
  • pkg/controller/trustmanager/serviceaccounts_test.go
  • pkg/controller/trustmanager/services.go
  • pkg/controller/trustmanager/services_test.go
  • pkg/controller/trustmanager/webhooks.go
  • pkg/controller/trustmanager/webhooks_test.go
✅ Files skipped from review due to trivial changes (4)
  • pkg/controller/trustmanager/controller_test.go
  • pkg/controller/trustmanager/certificates_test.go
  • pkg/controller/istiocsr/constants.go
  • pkg/controller/trustmanager/services_test.go
🚧 Files skipped from review as they are similar to previous changes (20)
  • pkg/controller/trustmanager/deployments_test.go
  • pkg/controller/trustmanager/webhooks_test.go
  • pkg/controller/trustmanager/serviceaccounts_test.go
  • pkg/controller/trustmanager/rbacs_test.go
  • pkg/controller/istiocsr/install_istiocsr.go
  • pkg/controller/trustmanager/services.go
  • pkg/controller/istiocsr/serviceaccounts.go
  • pkg/controller/istiocsr/install_instiocsr_test.go
  • pkg/controller/trustmanager/certificates.go
  • pkg/controller/istiocsr/certificates_test.go
  • pkg/controller/istiocsr/serviceaccounts_test.go
  • pkg/controller/trustmanager/deployments.go
  • pkg/controller/trustmanager/webhooks.go
  • pkg/controller/istiocsr/certificates.go
  • pkg/controller/trustmanager/rbacs.go
  • pkg/controller/trustmanager/serviceaccounts.go
  • pkg/controller/istiocsr/rbacs.go
  • pkg/controller/istiocsr/services.go
  • pkg/controller/istiocsr/rbacs_test.go
  • pkg/controller/istiocsr/networkpolicies.go

Walkthrough

Adds a generic Server-Side Apply helper ApplyResource[T client.Object] and migrates IstioCSR and TrustManager reconcile helpers to use it, updating tests and error strings to reflect SSA/patch semantics.

Changes

Generic Resource Applier and Controller Refactor

Layer / File(s) Summary
Generic applier implementation
pkg/controller/common/applier.go
New generic ApplyResource[T client.Object] that checks existence, calls a hasChanged predicate, performs Server-Side Apply via Patch with FieldOwner and ForceOwnership, logs structured messages, records events, and wraps client errors.
Ownership constant
pkg/controller/istiocsr/constants.go
Added unexported fieldOwner = "istio-csr-controller" for SSA field ownership.
Controller entrypoint & label assembly
pkg/controller/istiocsr/install_istiocsr.go
Centralized resourceLabels construction and removed istioCSRCreateRecon boolean from downstream reconciliation calls.
IstioCSR resource migrations (networkpolicies, services, serviceaccounts, certificates)
pkg/controller/istiocsr/networkpolicies.go, pkg/controller/istiocsr/services.go, pkg/controller/istiocsr/serviceaccounts.go, pkg/controller/istiocsr/certificates.go
Replaced per-resource Exists/Create/Update logic with common.ApplyResource calls; asset decoding now sets target namespace and merges resourceLabels (uses maps.Copy); per-resource functions drop the istioCSRCreateRecon flag.
IstioCSR RBAC reconciliation expansion
pkg/controller/istiocsr/rbacs.go
Added apply-based reconciliation for Roles, RoleBindings, and lease-related Role/RoleBinding resources; all reconciled via ApplyResource with existing change predicates.
TrustManager migrations (certificates, deployments, rbacs, services, serviceaccounts, webhooks)
pkg/controller/trustmanager/certificates.go, pkg/controller/trustmanager/deployments.go, pkg/controller/trustmanager/rbacs.go, pkg/controller/trustmanager/services.go, pkg/controller/trustmanager/serviceaccounts.go, pkg/controller/trustmanager/webhooks.go
Replaced manual exists/patch/create flows with ApplyResource usage across TrustManager controllers; preserved per-resource *Modified predicates and simplified annotation/ClientConfig handling where applicable; trimmed imports to match new approach.
Tests and messaging updates
pkg/controller/istiocsr/*_test.go, pkg/controller/trustmanager/*_test.go, pkg/controller/istiocsr/install_instiocsr_test.go
Updated tests to SSA/patch semantics: replace Create/UpdateWithRetry expectations with Patch/PatchReturns, remove istioCSRCreateRecon test parameters, and standardize expected error message wording to resource-level phrasing (capitalized resource names and "failed to check if ", "failed to apply ").
sequenceDiagram
  participant Reconciler as Reconciler
  participant ApplyUtil as ApplyResource
  participant API as Kubernetes API Server
  participant Recorder as EventRecorder
  participant Logger as Logger

  Reconciler->>ApplyUtil: Call ApplyResource(ctx, c, log, recorder, owner, desired, existing, fieldOwner, hasChanged)
  ApplyUtil->>API: Get resource (exists check)
  API-->>ApplyUtil: Exists / NotFound / Error
  alt exists & hasChanged == false
    ApplyUtil->>Logger: emit debug "no drift, skipping"
    ApplyUtil-->>Reconciler: return nil
  else not exists or hasChanged == true
    ApplyUtil->>API: Server-Side Apply (Patch, ForceOwnership, FieldManager=fieldOwner)
    API-->>ApplyUtil: Success / Error
    alt success
      ApplyUtil->>Recorder: Event("Reconciled")
      ApplyUtil->>Logger: emit info "applied resource"
      ApplyUtil-->>Reconciler: return nil
    else error
      ApplyUtil->>Logger: emit error
      ApplyUtil-->>Reconciler: return wrapped error
    end
  end
Loading

Estimated code review effort:
🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested labels:
lgtm, qe-approved, approved, px-approved

Suggested reviewers:

  • swghosh
  • TrilokGeer
  • chiragkyal
🚥 Pre-merge checks | ✅ 12 | ❌ 3

❌ Failed checks (3 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 2.78% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Single Node Openshift (Sno) Test Compatibility ⚠️ Warning PR adds e2e test "should apply custom affinity to deployment" with PodAntiAffinity using TopologyKey "kubernetes.io/hostname", assuming multiple nodes exist on SNO. Add [Skipped:SingleReplicaTopology] label to affinity and nodeSelector tests in trustmanager_test.go, or wrap with SNO check using exutil.IsSingleNode() and g.Skip().
Ipv6 And Disconnected Network Test Compatibility ⚠️ Warning New Ginkgo e2e tests contain IPv4 hardcoded addresses (127.0.0.1, 10.10.10.10) and external image pulls (quay.io) incompatible with IPv6-only disconnected CI environments. Replace hardcoded IPs with dynamic detection; use cluster-internal registries; add [Skipped:Disconnected] label for tests requiring external connectivity.
✅ Passed checks (12 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main changes: extracting a shared ApplyResource helper and migrating istiocsr to Server-Side Apply (SSA). It is specific, concise, and directly reflects the primary objectives of the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed All 12 modified test files use standard Go tests (not Ginkgo). All test case names are stable, descriptive strings with no dynamic values, timestamps, UUIDs, or generated suffixes.
Test Structure And Quality ✅ Passed All 12 modified test files use Go testing.T table-driven tests, not Ginkgo. The check requiring Ginkgo test review is not applicable to this PR.
Microshift Test Compatibility ✅ Passed PR does not add any new Ginkgo e2e tests; it only refactors existing unit tests in pkg/controller/ and adds a helper function. Test/e2e files were not modified.
Topology-Aware Scheduling Compatibility ✅ Passed This is a refactoring-only PR. No deployment manifests with scheduling constraints were added. The new ApplyResource helper is topology-agnostic.
Ote Binary Stdout Contract ✅ Passed ApplyResource helper and controller migrations use logr.Logger.V() for logging and EventRecorder for events; no stdout writes, no TestMain/suite modifications found.
No-Weak-Crypto ✅ Passed No weak crypto algorithms, ECB mode, custom crypto implementations, or non-constant-time secret comparisons found. PR refactors Kubernetes resource reconciliation, not cryptography.
Container-Privileges ✅ Passed PR modifies only Go code; no YAML manifests changed. No privileged container settings (privileged: true, hostPID, hostNetwork, hostIPC, SYS_ADMIN) introduced in the code.
No-Sensitive-Data-In-Logs ✅ Passed ApplyResource and all migrated functions log only safe metadata (resource kind/key) and field manager names; no passwords, tokens, API keys, or PII exposed.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci openshift-ci Bot requested review from mytreya-rh and swghosh May 6, 2026 18:32
@openshift-ci

openshift-ci Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sebrandon1
Once this PR has been reviewed and has the lgtm label, please assign mytreya-rh for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
pkg/controller/istiocsr/install_instiocsr_test.go (1)

68-84: ⚡ Quick win

Preserve generated-name simulation for ClusterRole in these fakes.

createOrApplyClusterRoles() still depends on Create mutating the object with a generated name before status is written and before the ClusterRoleBinding gets its RoleRef.Name. These stubs only backfill ClusterRoleBinding, so the test can still pass even if the role name is left empty. I'd add a *rbacv1.ClusterRole branch in each CreateCalls block as well.

Representative tweak
m.CreateCalls(func(ctx context.Context, obj client.Object, option ...client.CreateOption) error {
	switch o := obj.(type) {
+	case *rbacv1.ClusterRole:
+		role := testClusterRole()
+		role.DeepCopyInto(o)
 	case *appsv1.Deployment:
 		if !reflect.DeepEqual(o.GetLabels(), labels) {
 			return fmt.Errorf("labels mismatch in %v resource; got: %v, want: %v", o, o.GetLabels(), labels)
 		}

Also applies to: 110-117, 131-138

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/controller/istiocsr/install_instiocsr_test.go` around lines 68 - 84, The
CreateCalls test stubs need to simulate Create mutating a ClusterRole with a
generated name so createOrApplyClusterRoles() sees a non-empty Role.Name; update
the CreateCalls handlers (the ones that currently switch over *appsv1.Deployment
and *rbacv1.ClusterRoleBinding) to also include a case for *rbacv1.ClusterRole
that sets o.Name to a generated value (e.g., append "-generated" or a
deterministic string) and preserves labels so subsequent logic that reads the
ClusterRole's name (and the ClusterRoleBinding RoleRef.Name) behaves as in real
Create; apply this same addition to the other CreateCalls blocks mentioned.
pkg/controller/trustmanager/webhooks_test.go (1)

207-221: ⚡ Quick win

Keep the error assertion specific to the webhook config.

ApplyResource still includes the resource name in the returned error, so shortening these expectations to just failed to ... resource makes this table much less discriminating. A wrong object name or an extra apply call in this path would still satisfy the assertion. I'd keep the expected substring specific to trustManagerWebhookConfigName here, and mirror that in the other migrated reconciliation tests.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/controller/trustmanager/webhooks_test.go` around lines 207 - 221, The
test's error expectation is too generic; update the table entries (the case with
name "patch error propagates" and similar migrated reconciliation tests) to
assert the error message includes the specific webhook resource name by matching
the substring that contains trustManagerWebhookConfigName (the Reconciler's
ApplyResource error includes the resource name), e.g. expect the returned error
to contain trustManagerWebhookConfigName along with "failed to apply resource",
and adjust other tests that assert "failed to check if resource" / "failed to
apply resource" to similarly include trustManagerWebhookConfigName so the
assertions target the specific webhook config rather than any resource.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/controller/common/applier.go`:
- Around line 33-36: The resourceName construction in applier.go currently uses
fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) which yields
"/name" for cluster-scoped objects; change the logic to detect an empty
namespace (desired.GetNamespace() == "") and build resourceName as just
desired.GetName(), otherwise build it as namespace + "/" + name so
cluster-scoped resources do not get a leading slash; update any use of the
existing resourceName variable accordingly.

---

Nitpick comments:
In `@pkg/controller/istiocsr/install_instiocsr_test.go`:
- Around line 68-84: The CreateCalls test stubs need to simulate Create mutating
a ClusterRole with a generated name so createOrApplyClusterRoles() sees a
non-empty Role.Name; update the CreateCalls handlers (the ones that currently
switch over *appsv1.Deployment and *rbacv1.ClusterRoleBinding) to also include a
case for *rbacv1.ClusterRole that sets o.Name to a generated value (e.g., append
"-generated" or a deterministic string) and preserves labels so subsequent logic
that reads the ClusterRole's name (and the ClusterRoleBinding RoleRef.Name)
behaves as in real Create; apply this same addition to the other CreateCalls
blocks mentioned.

In `@pkg/controller/trustmanager/webhooks_test.go`:
- Around line 207-221: The test's error expectation is too generic; update the
table entries (the case with name "patch error propagates" and similar migrated
reconciliation tests) to assert the error message includes the specific webhook
resource name by matching the substring that contains
trustManagerWebhookConfigName (the Reconciler's ApplyResource error includes the
resource name), e.g. expect the returned error to contain
trustManagerWebhookConfigName along with "failed to apply resource", and adjust
other tests that assert "failed to check if resource" / "failed to apply
resource" to similarly include trustManagerWebhookConfigName so the assertions
target the specific webhook config rather than any resource.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 683e1b36-07c6-463b-8cd3-d29a46add8d5

📥 Commits

Reviewing files that changed from the base of the PR and between a2e7514 and f8b7c81.

📒 Files selected for processing (26)
  • pkg/controller/common/applier.go
  • pkg/controller/istiocsr/certificates.go
  • pkg/controller/istiocsr/certificates_test.go
  • pkg/controller/istiocsr/constants.go
  • pkg/controller/istiocsr/install_instiocsr_test.go
  • pkg/controller/istiocsr/install_istiocsr.go
  • pkg/controller/istiocsr/networkpolicies.go
  • pkg/controller/istiocsr/rbacs.go
  • pkg/controller/istiocsr/rbacs_test.go
  • pkg/controller/istiocsr/serviceaccounts.go
  • pkg/controller/istiocsr/serviceaccounts_test.go
  • pkg/controller/istiocsr/services.go
  • pkg/controller/istiocsr/services_test.go
  • pkg/controller/trustmanager/certificates.go
  • pkg/controller/trustmanager/certificates_test.go
  • pkg/controller/trustmanager/controller_test.go
  • pkg/controller/trustmanager/deployments.go
  • pkg/controller/trustmanager/deployments_test.go
  • pkg/controller/trustmanager/rbacs.go
  • pkg/controller/trustmanager/rbacs_test.go
  • pkg/controller/trustmanager/serviceaccounts.go
  • pkg/controller/trustmanager/serviceaccounts_test.go
  • pkg/controller/trustmanager/services.go
  • pkg/controller/trustmanager/services_test.go
  • pkg/controller/trustmanager/webhooks.go
  • pkg/controller/trustmanager/webhooks_test.go

Comment thread pkg/controller/common/applier.go Outdated
@sebrandon1 sebrandon1 force-pushed the extract-apply-resource branch from ca8b171 to fdd735e Compare May 6, 2026 18:48

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
pkg/controller/istiocsr/serviceaccounts_test.go (1)

22-25: ⚡ Quick win

Cover the no-op branch explicitly.

Now that this table has assertCalls, the "serviceaccount reconciliation successful" case should also assert PatchCallCount() == 0. Right now that case still passes if ApplyResource starts patching unchanged ServiceAccounts on every reconcile, which is one of the main regression risks in this SSA migration.

Suggested assertion
 		{
 			name: "serviceaccount reconciliation successful",
 			preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) {
 				m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) {
 					switch o := obj.(type) {
 					case *corev1.ServiceAccount:
 						serviceaccount := testServiceAccount()
 						serviceaccount.DeepCopyInto(o)
 					}
 					return true, nil
 				})
 			},
+			assertCalls: func(t *testing.T, mock *fakes.FakeCtrlClient) {
+				if mock.PatchCallCount() != 0 {
+					t.Errorf("createOrApplyServiceAccounts() Patch call count: %d, want 0", mock.PatchCallCount())
+				}
+			},
 		},
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/controller/istiocsr/serviceaccounts_test.go` around lines 22 - 25, Update
the "serviceaccount reconciliation successful" testcase in the table-driven test
in serviceaccounts_test.go to explicitly assert that no patch operations
occurred: inside its assertCalls function, call mock.PatchCallCount() and
require it equals 0 (on the provided *fakes.FakeCtrlClient) to ensure
ApplyResource did not issue patches for unchanged ServiceAccounts; keep other
existing assertions and reference the Reconciler and ApplyResource behavior as
context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/controller/istiocsr/rbacs.go`:
- Around line 265-269: createOrApplyRoleBindings and
createOrApplyRoleBindingForLeases currently always call common.ApplyResource
(Server-Side Apply) but RoleBinding.roleRef is immutable—mirror the
ClusterRoleBinding handler's pattern: after calling common.ApplyResource (with
hasObjectChanged from getRoleBindingObject/hasObjectChanged), detect when the
failure or diff indicates only a roleRef drift (compare desired.RoleRef to the
live object.RoleRef), then delete the existing rbacv1.RoleBinding and recreate
the desired object (preserving owner refs/events/fieldOwner) as a fallback;
implement this delete-then-create flow in both createOrApplyRoleBindings and
createOrApplyRoleBindingForLeases using the same logic used in the
ClusterRoleBinding handler to avoid permanent SSA validation errors.

---

Nitpick comments:
In `@pkg/controller/istiocsr/serviceaccounts_test.go`:
- Around line 22-25: Update the "serviceaccount reconciliation successful"
testcase in the table-driven test in serviceaccounts_test.go to explicitly
assert that no patch operations occurred: inside its assertCalls function, call
mock.PatchCallCount() and require it equals 0 (on the provided
*fakes.FakeCtrlClient) to ensure ApplyResource did not issue patches for
unchanged ServiceAccounts; keep other existing assertions and reference the
Reconciler and ApplyResource behavior as context.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 3367fe15-60ea-4966-a17e-6222e23128a6

📥 Commits

Reviewing files that changed from the base of the PR and between f8b7c81 and fdd735e.

📒 Files selected for processing (26)
  • pkg/controller/common/applier.go
  • pkg/controller/istiocsr/certificates.go
  • pkg/controller/istiocsr/certificates_test.go
  • pkg/controller/istiocsr/constants.go
  • pkg/controller/istiocsr/install_instiocsr_test.go
  • pkg/controller/istiocsr/install_istiocsr.go
  • pkg/controller/istiocsr/networkpolicies.go
  • pkg/controller/istiocsr/rbacs.go
  • pkg/controller/istiocsr/rbacs_test.go
  • pkg/controller/istiocsr/serviceaccounts.go
  • pkg/controller/istiocsr/serviceaccounts_test.go
  • pkg/controller/istiocsr/services.go
  • pkg/controller/istiocsr/services_test.go
  • pkg/controller/trustmanager/certificates.go
  • pkg/controller/trustmanager/certificates_test.go
  • pkg/controller/trustmanager/controller_test.go
  • pkg/controller/trustmanager/deployments.go
  • pkg/controller/trustmanager/deployments_test.go
  • pkg/controller/trustmanager/rbacs.go
  • pkg/controller/trustmanager/rbacs_test.go
  • pkg/controller/trustmanager/serviceaccounts.go
  • pkg/controller/trustmanager/serviceaccounts_test.go
  • pkg/controller/trustmanager/services.go
  • pkg/controller/trustmanager/services_test.go
  • pkg/controller/trustmanager/webhooks.go
  • pkg/controller/trustmanager/webhooks_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • pkg/controller/trustmanager/certificates.go
  • pkg/controller/trustmanager/certificates_test.go

Comment on lines +265 to +269
func (r *Reconciler) createOrApplyRoleBindings(istiocsr *v1alpha1.IstioCSR, serviceAccount string, resourceLabels map[string]string) error {
desired := r.getRoleBindingObject(serviceAccount, istiocsr.GetNamespace(), istiocsr.Spec.IstioCSRConfig.Istio.Namespace, resourceLabels)

roleBindingName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName())
r.log.V(4).Info("reconciling rolebinding resource", "name", roleBindingName)
fetched := &rbacv1.RoleBinding{}
exist, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), fetched)
if err != nil {
return common.FromClientError(err, "failed to check %s rolebinding resource already exists", roleBindingName)
}

if exist {
if istioCSRCreateRecon {
r.eventRecorder.Eventf(istiocsr, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s rolebinding resource already exists, maybe from previous installation", roleBindingName)
}
if hasObjectChanged(desired, fetched) {
r.log.V(1).Info("rolebinding has been modified, updating to desired state", "name", roleBindingName)
if err := r.UpdateWithRetry(r.ctx, desired); err != nil {
return common.FromClientError(err, "failed to update %s rolebinding resource", roleBindingName)
}
r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "rolebinding resource %s reconciled back to desired state", roleBindingName)
} else {
r.log.V(4).Info("rolebinding resource already exists and is in expected state", "name", roleBindingName)
}
}

if !exist {
if err := r.Create(r.ctx, desired); err != nil {
return common.FromClientError(err, "failed to create %s rolebinding resource", roleBindingName)
}
r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "rolebinding resource %s created", roleBindingName)
}

return nil
return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, istiocsr, desired, &rbacv1.RoleBinding{}, fieldOwner,
func(d, e *rbacv1.RoleBinding) bool { return hasObjectChanged(d, e) },
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

Is roleRefimmutable on KubernetesRoleBinding resources, and does changing it require deleting and recreating the binding instead of patching/applying it?

💡 Result:

Yes, roleRef is immutable on Kubernetes RoleBinding resources. Changing it requires deleting and recreating the binding, as patching or applying an update to roleRef will result in a validation error.[1][2] The official Kubernetes API documentation explicitly states: "This field is immutable."[1] The RBAC documentation confirms: "After you create a binding, you cannot change the Role or ClusterRole that it refers to. If you try to change a binding's roleRef, you get a validation error. If you do want to change the roleRef for a binding, you need to remove the binding object and create a replacement."[2] This behavior is enforced in the validation code and recently formalized with the +k8s:immutable tag in Kubernetes source code (as of early 2026).[3] Multiple sources, including Stack Overflow discussions citing the docs and code, report the exact error: "cannot change roleRef".[4][5]

Citations:


🏁 Script executed:

# First, let's locate and examine the file mentioned in the review
fd -t f "rbacs.go" --exec wc -l {} \;

Repository: openshift/cert-manager-operator

Length of output: 157


🏁 Script executed:

# Now let's read the relevant sections of the file
cat -n pkg/controller/istiocsr/rbacs.go | sed -n '260,300p'

Repository: openshift/cert-manager-operator

Length of output: 2800


🏁 Script executed:

# Check the ClusterRoleBinding section mentioned (lines 329-350)
cat -n pkg/controller/istiocsr/rbacs.go | sed -n '320,360p'

Repository: openshift/cert-manager-operator

Length of output: 2181


🏁 Script executed:

# Let's also check the common.ApplyResource function to understand its behavior
fd -t f -name "*.go" | xargs grep -l "func.*ApplyResource" | head -5

Repository: openshift/cert-manager-operator

Length of output: 306


🏁 Script executed:

# Search for ApplyResource implementation
grep -r "func.*ApplyResource" --include="*.go" | head -10

Repository: openshift/cert-manager-operator

Length of output: 281


🏁 Script executed:

# Alternative search with rg
rg "func.*ApplyResource" --type go -A 5

Repository: openshift/cert-manager-operator

Length of output: 929


🏁 Script executed:

# Read the ApplyResource implementation
cat -n pkg/controller/common/applier.go | head -150

Repository: openshift/cert-manager-operator

Length of output: 2180


🏁 Script executed:

# Check file size first
wc -l pkg/controller/common/applier.go

Repository: openshift/cert-manager-operator

Length of output: 111


Preserve the delete/recreate path for RoleBinding.RoleRef drift.

Both createOrApplyRoleBindings (265–269) and createOrApplyRoleBindingForLeases (294–298) use ApplyResource, which patches via Server-Side Apply. Kubernetes treats RoleBinding.roleRef as immutable, so attempting to patch a changed roleRef will fail with a validation error and permanently break reconciliation. The ClusterRoleBinding handler (329–350) already implements the required delete/recreate fallback when roleRef changes; apply the same pattern here.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/controller/istiocsr/rbacs.go` around lines 265 - 269,
createOrApplyRoleBindings and createOrApplyRoleBindingForLeases currently always
call common.ApplyResource (Server-Side Apply) but RoleBinding.roleRef is
immutable—mirror the ClusterRoleBinding handler's pattern: after calling
common.ApplyResource (with hasObjectChanged from
getRoleBindingObject/hasObjectChanged), detect when the failure or diff
indicates only a roleRef drift (compare desired.RoleRef to the live
object.RoleRef), then delete the existing rbacv1.RoleBinding and recreate the
desired object (preserving owner refs/events/fieldOwner) as a fallback;
implement this delete-then-create flow in both createOrApplyRoleBindings and
createOrApplyRoleBindingForLeases using the same logic used in the
ClusterRoleBinding handler to avoid permanent SSA validation errors.

@sebrandon1 sebrandon1 force-pushed the extract-apply-resource branch from fdd735e to 26c1bb6 Compare May 14, 2026 19:07
@sebrandon1

Copy link
Copy Markdown
Member Author

/retest

Both istiocsr and trustmanager had 8-10 nearly identical createOrApply
methods with duplicated reconciliation boilerplate. trustmanager used
Server-Side Apply while istiocsr used Create/UpdateWithRetry.

Add a generic common.ApplyResource[T]() helper that handles the common
Exists/drift-check/Patch pattern with a pluggable hasChanged callback.
The helper derives the Kubernetes kind from the type parameter for
clear error messages and uses client.ObjectKeyFromObject for consistent
resource name formatting.

Migrate all trustmanager methods and istiocsr simple methods to use it.
ClusterRole/ClusterRoleBinding methods in istiocsr are kept as-is since
they use GenerateName with List fallback and Delete for immutable RoleRef
changes.

The istioCSRCreateRecon warning events are dropped from the migrated
methods since SSA is inherently idempotent.
@sebrandon1 sebrandon1 force-pushed the extract-apply-resource branch from 26c1bb6 to 8b48aaa Compare May 29, 2026 15:51
@sebrandon1

Copy link
Copy Markdown
Member Author

/retest

@sebrandon1

Copy link
Copy Markdown
Member Author

/retest-required

@openshift-ci

openshift-ci Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

@sebrandon1: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants