CM-1040: Extract shared ApplyResource helper and migrate istiocsr to SSA#420
Open
sebrandon1 wants to merge 2 commits into
Open
CM-1040: Extract shared ApplyResource helper and migrate istiocsr to SSA#420sebrandon1 wants to merge 2 commits into
sebrandon1 wants to merge 2 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@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. DetailsIn response to this:
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. |
WalkthroughThis PR adds a shared Server-Side Apply helper and routes IstioCSR and TrustManager reconciliation for services, certificates, RBAC, deployments, service accounts, network policies, and webhooks through it. Tests now validate patch-based behavior and standardized errors. ChangesServer-Side Apply reconciliation
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sebrandon1 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/controller/istiocsr/install_instiocsr_test.go (1)
68-84: ⚡ Quick winPreserve generated-name simulation for
ClusterRolein these fakes.
createOrApplyClusterRoles()still depends onCreatemutating the object with a generated name before status is written and before theClusterRoleBindinggets itsRoleRef.Name. These stubs only backfillClusterRoleBinding, so the test can still pass even if the role name is left empty. I'd add a*rbacv1.ClusterRolebranch in eachCreateCallsblock 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 winKeep the error assertion specific to the webhook config.
ApplyResourcestill includes the resource name in the returned error, so shortening these expectations to justfailed to ... resourcemakes 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 totrustManagerWebhookConfigNamehere, 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
📒 Files selected for processing (26)
pkg/controller/common/applier.gopkg/controller/istiocsr/certificates.gopkg/controller/istiocsr/certificates_test.gopkg/controller/istiocsr/constants.gopkg/controller/istiocsr/install_instiocsr_test.gopkg/controller/istiocsr/install_istiocsr.gopkg/controller/istiocsr/networkpolicies.gopkg/controller/istiocsr/rbacs.gopkg/controller/istiocsr/rbacs_test.gopkg/controller/istiocsr/serviceaccounts.gopkg/controller/istiocsr/serviceaccounts_test.gopkg/controller/istiocsr/services.gopkg/controller/istiocsr/services_test.gopkg/controller/trustmanager/certificates.gopkg/controller/trustmanager/certificates_test.gopkg/controller/trustmanager/controller_test.gopkg/controller/trustmanager/deployments.gopkg/controller/trustmanager/deployments_test.gopkg/controller/trustmanager/rbacs.gopkg/controller/trustmanager/rbacs_test.gopkg/controller/trustmanager/serviceaccounts.gopkg/controller/trustmanager/serviceaccounts_test.gopkg/controller/trustmanager/services.gopkg/controller/trustmanager/services_test.gopkg/controller/trustmanager/webhooks.gopkg/controller/trustmanager/webhooks_test.go
sebrandon1
force-pushed
the
extract-apply-resource
branch
from
ca8b171 to
fdd735e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/controller/istiocsr/serviceaccounts_test.go (1)
22-25: ⚡ Quick winCover the no-op branch explicitly.
Now that this table has
assertCalls, the"serviceaccount reconciliation successful"case should also assertPatchCallCount() == 0. Right now that case still passes ifApplyResourcestarts patching unchangedServiceAccounts 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
📒 Files selected for processing (26)
pkg/controller/common/applier.gopkg/controller/istiocsr/certificates.gopkg/controller/istiocsr/certificates_test.gopkg/controller/istiocsr/constants.gopkg/controller/istiocsr/install_instiocsr_test.gopkg/controller/istiocsr/install_istiocsr.gopkg/controller/istiocsr/networkpolicies.gopkg/controller/istiocsr/rbacs.gopkg/controller/istiocsr/rbacs_test.gopkg/controller/istiocsr/serviceaccounts.gopkg/controller/istiocsr/serviceaccounts_test.gopkg/controller/istiocsr/services.gopkg/controller/istiocsr/services_test.gopkg/controller/trustmanager/certificates.gopkg/controller/trustmanager/certificates_test.gopkg/controller/trustmanager/controller_test.gopkg/controller/trustmanager/deployments.gopkg/controller/trustmanager/deployments_test.gopkg/controller/trustmanager/rbacs.gopkg/controller/trustmanager/rbacs_test.gopkg/controller/trustmanager/serviceaccounts.gopkg/controller/trustmanager/serviceaccounts_test.gopkg/controller/trustmanager/services.gopkg/controller/trustmanager/services_test.gopkg/controller/trustmanager/webhooks.gopkg/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
sebrandon1
force-pushed
the
extract-apply-resource
branch
from
fdd735e to
26c1bb6
Compare
Member
Author
|
/retest |
sebrandon1
force-pushed
the
extract-apply-resource
branch
from
26c1bb6 to
8b48aaa
Compare
Member
Author
|
/retest |
Member
Author
|
/retest-required |
sebrandon1
force-pushed
the
extract-apply-resource
branch
from
8b48aaa to
4442995
Compare
sebrandon1
force-pushed
the
extract-apply-resource
branch
from
4442995 to
786945d
Compare
sebrandon1
force-pushed
the
extract-apply-resource
branch
from
786945d to
81c1b58
Compare
Member
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Member
Author
|
Addressed CodeRabbit feedback on RoleBinding.roleRef immutability:
All tests pass with 85.2% coverage on istiocsr package. |
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.
RoleBinding.roleRef is an immutable field in Kubernetes. When it changes, the binding must be deleted and recreated. Added roleRef drift detection to createOrApplyRoleBindings and createOrApplyRoleBindingForLeases to delete the existing binding before calling ApplyResource if roleRef has changed, preventing SSA validation errors. This mirrors the pattern already used in handleClusterRoleBindingModification.
sebrandon1
force-pushed
the
extract-apply-resource
branch
from
d4919c6 to
edaac18
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/istiocsr/rbacs.go (1)
285-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared helper for RoleBinding
roleRefdrift/delete logic.
createOrApplyRoleBindingsandcreateOrApplyRoleBindingForLeasesduplicate ~18 nearly-identical lines (Exists check,rbacRoleBindingRefModifiedcheck, log, Delete with NotFound tolerance). Given this PR's stated goal of removing duplicated boilerplate, extract this into a shared helper, e.g.deleteRoleBindingIfRoleRefChanged(desired *rbacv1.RoleBinding) error.♻️ Proposed refactor
+func (r *Reconciler) deleteRoleBindingIfRoleRefChanged(desired *rbacv1.RoleBinding) error { + key := client.ObjectKeyFromObject(desired) + existing := &rbacv1.RoleBinding{} + exists, err := r.Exists(r.ctx, key, existing) + if err != nil { + return common.FromClientError(err, "failed to check if RoleBinding %q exists", key) + } + if exists && rbacRoleBindingRefModified(desired, existing) { + r.log.V(1).Info("rolebinding roleRef changed, deleting for recreation (roleRef is immutable)", "name", key) + if err := r.Delete(r.ctx, existing); err != nil && !apierrors.IsNotFound(err) { + return common.FromClientError(err, "failed to delete RoleBinding %q to replace roleRef", key) + } + } + return nil +} + 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) - - // RoleBinding.roleRef is immutable; if it has changed, delete the existing binding first - key := client.ObjectKeyFromObject(desired) - existing := &rbacv1.RoleBinding{} - exists, err := r.Exists(r.ctx, key, existing) - if err != nil { - return common.FromClientError(err, "failed to check if RoleBinding %q exists", key) - } - if exists && rbacRoleBindingRefModified(desired, existing) { - r.log.V(1).Info("rolebinding roleRef changed, deleting for recreation (roleRef is immutable)", "name", key) - if err := r.Delete(r.ctx, existing); err != nil { - if !apierrors.IsNotFound(err) { - return common.FromClientError(err, "failed to delete RoleBinding %q to replace roleRef", key) - } - } - } + if err := r.deleteRoleBindingIfRoleRefChanged(desired); err != nil { + return err + } 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) }, ) }Apply the analogous change to
createOrApplyRoleBindingForLeases.Also applies to: 331-352
🤖 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 285 - 306, Extract the duplicated RoleBinding roleRef drift handling from createOrApplyRoleBindings and createOrApplyRoleBindingForLeases into a shared helper such as deleteRoleBindingIfRoleRefChanged, accepting the desired RoleBinding. Move the Exists check, rbacRoleBindingRefModified comparison, diagnostic log, deletion, and NotFound tolerance into that helper, then invoke it from both creation methods before applying the 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.
Nitpick comments:
In `@pkg/controller/istiocsr/rbacs.go`:
- Around line 285-306: Extract the duplicated RoleBinding roleRef drift handling
from createOrApplyRoleBindings and createOrApplyRoleBindingForLeases into a
shared helper such as deleteRoleBindingIfRoleRefChanged, accepting the desired
RoleBinding. Move the Exists check, rbacRoleBindingRefModified comparison,
diagnostic log, deletion, and NotFound tolerance into that helper, then invoke
it from both creation methods before applying the resource.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a0bb173e-0351-4833-bd59-fa1f25c895cb
📒 Files selected for processing (26)
pkg/controller/common/applier.gopkg/controller/istiocsr/certificates.gopkg/controller/istiocsr/certificates_test.gopkg/controller/istiocsr/constants.gopkg/controller/istiocsr/install_instiocsr_test.gopkg/controller/istiocsr/install_istiocsr.gopkg/controller/istiocsr/networkpolicies.gopkg/controller/istiocsr/rbacs.gopkg/controller/istiocsr/rbacs_test.gopkg/controller/istiocsr/serviceaccounts.gopkg/controller/istiocsr/serviceaccounts_test.gopkg/controller/istiocsr/services.gopkg/controller/istiocsr/services_test.gopkg/controller/trustmanager/certificates.gopkg/controller/trustmanager/certificates_test.gopkg/controller/trustmanager/controller_test.gopkg/controller/trustmanager/deployments.gopkg/controller/trustmanager/deployments_test.gopkg/controller/trustmanager/rbacs.gopkg/controller/trustmanager/rbacs_test.gopkg/controller/trustmanager/serviceaccounts.gopkg/controller/trustmanager/serviceaccounts_test.gopkg/controller/trustmanager/services.gopkg/controller/trustmanager/services_test.gopkg/controller/trustmanager/webhooks.gopkg/controller/trustmanager/webhooks_test.go
🚧 Files skipped from review as they are similar to previous changes (23)
- pkg/controller/common/applier.go
- pkg/controller/istiocsr/constants.go
- pkg/controller/istiocsr/certificates_test.go
- pkg/controller/trustmanager/serviceaccounts.go
- pkg/controller/istiocsr/certificates.go
- pkg/controller/trustmanager/deployments_test.go
- pkg/controller/trustmanager/deployments.go
- pkg/controller/istiocsr/install_istiocsr.go
- pkg/controller/istiocsr/serviceaccounts.go
- pkg/controller/trustmanager/serviceaccounts_test.go
- pkg/controller/trustmanager/webhooks_test.go
- pkg/controller/istiocsr/serviceaccounts_test.go
- pkg/controller/istiocsr/install_instiocsr_test.go
- pkg/controller/trustmanager/rbacs_test.go
- pkg/controller/trustmanager/services_test.go
- pkg/controller/trustmanager/certificates_test.go
- pkg/controller/trustmanager/certificates.go
- pkg/controller/trustmanager/webhooks.go
- pkg/controller/istiocsr/rbacs_test.go
- pkg/controller/trustmanager/rbacs.go
- pkg/controller/istiocsr/services.go
- pkg/controller/istiocsr/services_test.go
- pkg/controller/istiocsr/networkpolicies.go
Contributor
|
@sebrandon1: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
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
2 participants
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
common.ApplyResource[T]()helper for SSA-based reconciliation with pluggable drift detection callbackscreateOrApplymethods to use the shared helpercreateOrApplymethods from Create/UpdateWithRetry to SSA via the shared helperistioCSRCreateReconwarning events from migrated methods (SSA is inherently idempotent)Test plan
go test ./pkg/controller/common/...passesgo test ./pkg/controller/istiocsr/...passesgo test ./pkg/controller/trustmanager/...passesgo build ./...succeedsmake lintshows only pre-existing issues (9 total, none introduced)Summary by CodeRabbit
Refactoring
Tests