Skip to content

Commit f8b7c81

Browse files
sebrandon1sebrandon1
committed
CM-1040: Extract shared ApplyResource helper and migrate istiocsr to SSA
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. 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.
1 parent a2e7514 commit f8b7c81

26 files changed

Lines changed: 233 additions & 734 deletions

‎pkg/controller/common/applier.go‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
package common
2+
3+
import (
4+
"context"
5+
"fmt"
6+
7+
"github.com/go-logr/logr"
8+
9+
corev1 "k8s.io/api/core/v1"
10+
"k8s.io/client-go/tools/record"
11+
"sigs.k8s.io/controller-runtime/pkg/client"
12+
)
13+
14+
// ApplyResource reconciles a desired Kubernetes object using Server-Side Apply.
15+
// It checks whether the resource already exists and whether managed fields have
16+
// drifted. If no drift is detected it returns early. Otherwise it patches the
17+
// object with SSA using the given fieldOwner.
18+
//
19+
// T is the concrete object type (e.g. *corev1.Service). The hasChanged callback
20+
// receives typed desired and existing objects so callers can compare fields
21+
// without type assertions.
22+
func ApplyResource[T client.Object](
23+
ctx context.Context,
24+
c CtrlClient,
25+
log logr.Logger,
26+
recorder record.EventRecorder,
27+
owner client.Object,
28+
desired T,
29+
existing T,
30+
fieldOwner string,
31+
hasChanged func(desired, existing T) bool,
32+
) error {
33+
resourceName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName())
34+
if resourceName == "/" {
35+
resourceName = desired.GetName()
36+
}
37+
38+
log.V(4).Info("reconciling resource", "name", resourceName)
39+
40+
exists, err := c.Exists(ctx, client.ObjectKeyFromObject(desired), existing)
41+
if err != nil {
42+
return FromClientError(err, "failed to check if resource %q exists", resourceName)
43+
}
44+
if exists && !hasChanged(desired, existing) {
45+
log.V(4).Info("resource exists and is in desired state", "name", resourceName)
46+
return nil
47+
}
48+
49+
log.V(2).Info("applying resource to desired state", "name", resourceName)
50+
if err := c.Patch(ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil {
51+
return FromClientError(err, "failed to apply resource %q", resourceName)
52+
}
53+
54+
recorder.Eventf(owner, corev1.EventTypeNormal, "Reconciled", "resource %s applied", resourceName)
55+
return nil
56+
}

‎pkg/controller/istiocsr/certificates.go‎

Lines changed: 4 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,7 @@ import (
44
"fmt"
55
"maps"
66

7-
corev1 "k8s.io/api/core/v1"
87
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
9-
"sigs.k8s.io/controller-runtime/pkg/client"
108

119
certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
1210
certmanagermetav1 "github.com/cert-manager/cert-manager/pkg/apis/meta/v1"
@@ -16,43 +14,15 @@ import (
1614
"github.com/openshift/cert-manager-operator/pkg/operator/assets"
1715
)
1816

19-
func (r *Reconciler) createOrApplyCertificates(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string, istioCSRCreateRecon bool) error {
17+
func (r *Reconciler) createOrApplyCertificates(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) error {
2018
desired, err := r.getCertificateObject(istiocsr, resourceLabels)
2119
if err != nil {
2220
return fmt.Errorf("failed to generate certificate resource for creation in %s: %w", istiocsr.GetNamespace(), err)
2321
}
2422

25-
certificateName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName())
26-
r.log.V(4).Info("reconciling certificate resource", "name", certificateName)
27-
fetched := &certmanagerv1.Certificate{}
28-
exist, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), fetched)
29-
if err != nil {
30-
return common.FromClientError(err, "failed to check %s certificate resource already exists", certificateName)
31-
}
32-
33-
if exist {
34-
if istioCSRCreateRecon {
35-
r.eventRecorder.Eventf(istiocsr, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s certificate resource already exists, maybe from previous installation", certificateName)
36-
}
37-
if hasObjectChanged(desired, fetched) {
38-
r.log.V(1).Info("certificate has been modified, updating to desired state", "name", certificateName)
39-
if err := r.UpdateWithRetry(r.ctx, desired); err != nil {
40-
return common.FromClientError(err, "failed to update %s certificate resource", certificateName)
41-
}
42-
r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "certificate resource %s reconciled back to desired state", certificateName)
43-
} else {
44-
r.log.V(4).Info("certificate resource already exists and is in expected state", "name", certificateName)
45-
}
46-
}
47-
48-
if !exist {
49-
if err := r.Create(r.ctx, desired); err != nil {
50-
return common.FromClientError(err, "failed to create %s certificate resource", certificateName)
51-
}
52-
r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "certificate resource %s created", certificateName)
53-
}
54-
55-
return nil
23+
return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, istiocsr, desired, &certmanagerv1.Certificate{}, fieldOwner,
24+
func(d, e *certmanagerv1.Certificate) bool { return hasObjectChanged(d, e) },
25+
)
5626
}
5727

5828
func (r *Reconciler) getCertificateObject(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) (*certmanagerv1.Certificate, error) {

‎pkg/controller/istiocsr/certificates_test.go‎

Lines changed: 6 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ func TestCreateOrApplyCertificates(t *testing.T) {
3939
return true, nil
4040
})
4141
},
42-
wantErr: `failed to check istio-test-ns/istiod certificate resource already exists: test client error`,
42+
wantErr: `failed to check if resource "istio-test-ns/istiod" exists: test client error`,
4343
},
4444
{
4545
name: "reconciliation of certificate fails while restoring to expected state",
@@ -61,15 +61,9 @@ func TestCreateOrApplyCertificates(t *testing.T) {
6161
}
6262
return true, nil
6363
})
64-
m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
65-
switch obj.(type) {
66-
case *certmanagerv1.Certificate:
67-
return errTestClient
68-
}
69-
return nil
70-
})
64+
m.PatchReturns(errTestClient)
7165
},
72-
wantErr: `failed to update istio-test-ns/istiod certificate resource: test client error`,
66+
wantErr: `failed to apply resource "istio-test-ns/istiod": test client error`,
7367
},
7468
{
7569
name: "reconciliation of certificate which already exists in expected state",
@@ -110,15 +104,9 @@ func TestCreateOrApplyCertificates(t *testing.T) {
110104
}
111105
return true, nil
112106
})
113-
m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error {
114-
switch obj.(type) {
115-
case *certmanagerv1.Certificate:
116-
return errTestClient
117-
}
118-
return nil
119-
})
107+
m.PatchReturns(errTestClient)
120108
},
121-
wantErr: `failed to create istio-test-ns/istiod certificate resource: test client error`,
109+
wantErr: `failed to apply resource "istio-test-ns/istiod": test client error`,
122110
},
123111
{
124112
name: "reconciliation of certificate when revisions are configured",
@@ -223,7 +211,7 @@ func TestCreateOrApplyCertificates(t *testing.T) {
223211
}, istiocsr); err != nil {
224212
t.Errorf("test error: %v", err)
225213
}
226-
err := r.createOrApplyCertificates(istiocsr, controllerDefaultResourceLabels, false)
214+
err := r.createOrApplyCertificates(istiocsr, controllerDefaultResourceLabels)
227215
if (tt.wantErr != "" || err != nil) && (err == nil || err.Error() != tt.wantErr) {
228216
t.Errorf("createOrApplyCertificates() err: %v, wantErr: %v", err, tt.wantErr)
229217
}

‎pkg/controller/istiocsr/constants.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,9 @@ const (
3030
// defaultRequeueTime is the default reconcile requeue time.
3131
defaultRequeueTime = time.Second * 30
3232

33+
// fieldOwner identifies this controller when using Server-Side Apply.
34+
fieldOwner = "istio-csr-controller"
35+
3336
// istiocsrObjectName is the name of the istio-csr resource created by user.
3437
// istio-csr CRD enforces name to be `default`.
3538
istiocsrObjectName = "default"

‎pkg/controller/istiocsr/install_instiocsr_test.go‎

Lines changed: 41 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -48,13 +48,31 @@ func TestReconcileIstioCSRDeployment(t *testing.T) {
4848
}
4949
return nil
5050
})
51+
// SSA-migrated resources (Service, ServiceAccount, Certificate, Role, RoleBinding) use Patch
52+
m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
53+
switch o := obj.(type) {
54+
case *corev1.Service, *corev1.ServiceAccount:
55+
if !reflect.DeepEqual(o.GetLabels(), labels) {
56+
return fmt.Errorf("labels mismatch in %v resource; got: %v, want: %v", o, o.GetLabels(), labels)
57+
}
58+
case *certmanagerv1.Certificate, *rbacv1.Role, *rbacv1.RoleBinding:
59+
l := make(map[string]string)
60+
maps.Copy(l, labels)
61+
l[istiocsrNamespaceMappingLabelName] = testIstioCSRNamespace
62+
if !reflect.DeepEqual(o.GetLabels(), l) {
63+
return fmt.Errorf("labels mismatch in %v resource; got: %v, want: %v", o, o.GetLabels(), l)
64+
}
65+
}
66+
return nil
67+
})
68+
// Non-SSA resources (Deployment, ClusterRole, ClusterRoleBinding) still use Create
5169
m.CreateCalls(func(ctx context.Context, obj client.Object, option ...client.CreateOption) error {
5270
switch o := obj.(type) {
53-
case *appsv1.Deployment, *corev1.Service, *corev1.ServiceAccount:
71+
case *appsv1.Deployment:
5472
if !reflect.DeepEqual(o.GetLabels(), labels) {
5573
return fmt.Errorf("labels mismatch in %v resource; got: %v, want: %v", o, o.GetLabels(), labels)
5674
}
57-
case *certmanagerv1.Certificate, *rbacv1.Role, *rbacv1.RoleBinding, *rbacv1.ClusterRole, *rbacv1.ClusterRoleBinding:
75+
case *rbacv1.ClusterRole, *rbacv1.ClusterRoleBinding:
5876
l := make(map[string]string)
5977
maps.Copy(l, labels)
6078
l[istiocsrNamespaceMappingLabelName] = testIstioCSRNamespace
@@ -67,49 +85,59 @@ func TestReconcileIstioCSRDeployment(t *testing.T) {
6785
},
6886
},
6987
{
70-
name: "istiocsr reconciliation fails with serviceaccount creation error",
88+
name: "istiocsr reconciliation fails with serviceaccount apply error",
7189
preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) {
72-
m.CreateCalls(func(ctx context.Context, obj client.Object, option ...client.CreateOption) error {
90+
m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
7391
switch obj.(type) {
7492
case *corev1.ServiceAccount:
7593
return errTestClient
7694
}
7795
return nil
7896
})
7997
},
80-
wantErr: `failed to create istiocsr-test-ns/cert-manager-istio-csr serviceaccount resource: test client error`,
98+
wantErr: `failed to apply resource "istiocsr-test-ns/cert-manager-istio-csr": test client error`,
8199
},
82100
{
83-
name: "istiocsr reconciliation fails with role creation error",
101+
name: "istiocsr reconciliation fails with role apply error",
84102
preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) {
85-
m.CreateCalls(func(ctx context.Context, obj client.Object, option ...client.CreateOption) error {
86-
switch o := obj.(type) {
103+
m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
104+
switch obj.(type) {
87105
case *rbacv1.Role:
88106
return errTestClient
107+
}
108+
return nil
109+
})
110+
m.CreateCalls(func(ctx context.Context, obj client.Object, option ...client.CreateOption) error {
111+
switch o := obj.(type) {
89112
case *rbacv1.ClusterRoleBinding:
90113
roleBinding := testClusterRoleBinding()
91114
roleBinding.DeepCopyInto(o)
92115
}
93116
return nil
94117
})
95118
},
96-
wantErr: `failed to create istio-test-ns/cert-manager-istio-csr role resource: test client error`,
119+
wantErr: `failed to apply resource "istio-test-ns/cert-manager-istio-csr": test client error`,
97120
},
98121
{
99-
name: "istiocsr reconciliation fails with certificate creation error",
122+
name: "istiocsr reconciliation fails with certificate apply error",
100123
preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) {
101-
m.CreateCalls(func(ctx context.Context, obj client.Object, option ...client.CreateOption) error {
102-
switch o := obj.(type) {
124+
m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
125+
switch obj.(type) {
103126
case *certmanagerv1.Certificate:
104127
return errTestClient
128+
}
129+
return nil
130+
})
131+
m.CreateCalls(func(ctx context.Context, obj client.Object, option ...client.CreateOption) error {
132+
switch o := obj.(type) {
105133
case *rbacv1.ClusterRoleBinding:
106134
roleBinding := testClusterRoleBinding()
107135
roleBinding.DeepCopyInto(o)
108136
}
109137
return nil
110138
})
111139
},
112-
wantErr: `failed to create istio-test-ns/istiod certificate resource: test client error`,
140+
wantErr: `failed to apply resource "istio-test-ns/istiod": test client error`,
113141
},
114142
}
115143

‎pkg/controller/istiocsr/install_istiocsr.go‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,25 +13,23 @@ func (r *Reconciler) reconcileIstioCSRDeployment(istiocsr *v1alpha1.IstioCSR, is
1313
return common.NewIrrecoverableError(err, "%s/%s configuration validation failed", istiocsr.GetNamespace(), istiocsr.GetName())
1414
}
1515

16-
// if user has set custom labels to be added to all resources created by the controller
17-
// merge it with the controller's own default labels.
1816
resourceLabels := make(map[string]string)
1917
if istiocsr.Spec.ControllerConfig != nil && len(istiocsr.Spec.ControllerConfig.Labels) != 0 {
2018
maps.Copy(resourceLabels, istiocsr.Spec.ControllerConfig.Labels)
2119
}
2220
maps.Copy(resourceLabels, controllerDefaultResourceLabels)
2321

24-
if err := r.createOrApplyNetworkPolicies(istiocsr, resourceLabels, istioCSRCreateRecon); err != nil {
22+
if err := r.createOrApplyNetworkPolicies(istiocsr, resourceLabels); err != nil {
2523
r.log.Error(err, "failed to reconcile network policy resources")
2624
return err
2725
}
2826

29-
if err := r.createOrApplyServices(istiocsr, resourceLabels, istioCSRCreateRecon); err != nil {
27+
if err := r.createOrApplyServices(istiocsr, resourceLabels); err != nil {
3028
r.log.Error(err, "failed to reconcile service resource")
3129
return err
3230
}
3331

34-
if err := r.createOrApplyServiceAccounts(istiocsr, resourceLabels, istioCSRCreateRecon); err != nil {
32+
if err := r.createOrApplyServiceAccounts(istiocsr, resourceLabels); err != nil {
3533
r.log.Error(err, "failed to reconcile serviceaccount resource")
3634
return err
3735
}
@@ -41,7 +39,7 @@ func (r *Reconciler) reconcileIstioCSRDeployment(istiocsr *v1alpha1.IstioCSR, is
4139
return err
4240
}
4341

44-
if err := r.createOrApplyCertificates(istiocsr, resourceLabels, istioCSRCreateRecon); err != nil {
42+
if err := r.createOrApplyCertificates(istiocsr, resourceLabels); err != nil {
4543
r.log.Error(err, "failed to reconcile certificate resource")
4644
return err
4745
}

0 commit comments

Comments
 (0)