Skip to content

Commit b1c6da9

Browse files
CM-1040: Extract shared ApplyResource helper and migrate istiocsr to SSA
- Adds common.ApplyResource[T]() in pkg/controller/common/applier.go - Migrates trustmanager's 11 createOrApply methods to the shared helper - Migrates istiocsr's 8 simple createOrApply methods from Create/UpdateWithRetry to SSA via the shared helper - Uses SSA with force ownership for both creation and drift correction; when a drift-correction apply fails (SSA cannot replace a granular list entry whose key fields were drifted - Service spec.ports is keyed by port and protocol), falls back to a JSON merge patch, which replaces lists wholesale - Handles RoleBinding roleRef immutability with delete-and-recreate, shared by both istiocsr RoleBindings in applyRoleBinding - Adds CtrlClient.Apply, which server-side applies through client.Client.Apply (converting the typed object to an unstructured apply configuration and copying the response back), replacing the deprecated Patch(obj, client.Apply) in ApplyResource and the trustmanager default CA package ConfigMap - Updates AGENTS.md and the harness docs (ADR-0002, components, testing and controller guidelines, istiocsr/trustmanager domain docs) for the mixed istiocsr apply strategy and the Apply-based SSA call and fake assertions - Net -402 lines of non-test controller code (duplicated reconciliation boilerplate) Includes unit tests for: - ApplyResource (create, no-op, drift via SSA, merge-patch fallback on apply failure, both-writes-fail error, Exists error, apply error, write type per branch) - Network policy asset decoding and apply - CtrlClient wrapper methods (Exists, Get, Create, Update, Delete, List, UpdateWithRetry with conflict retry, Patch, Apply, StatusUpdate) - ApplyResource drift correction against a real API server (test/apis, envtest): port drift on a list-map key field, label drift, annotation pruning, no-op Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 6ef8bbe commit b1c6da9

45 files changed

Lines changed: 1553 additions & 904 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎AGENTS.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,17 +17,17 @@ Deploys/configures upstream **cert-manager** (and optionally **istio-csr** / **t
1717
| CR | Scope / name | Stack | Gate |
1818
|----|--------------|-------|------|
1919
| [CertManager](harness-evals/harness-docs/domain/certmanager.md) | Cluster / `cluster` | library-go `resourceapply` | Always on |
20-
| [IstioCSR](harness-evals/harness-docs/domain/istiocsr.md) | Namespaced / `default` | Create+Update | `IstioCSR` GA default true |
20+
| [IstioCSR](harness-evals/harness-docs/domain/istiocsr.md) | Namespaced / `default` | SSA + Create/Update (mixed) | `IstioCSR` GA default true |
2121
| [TrustManager](harness-evals/harness-docs/domain/trustmanager.md) | Cluster / `cluster` | **SSA** | `TrustManager` TP default false |
2222

2323
**Operands**: Certificate/Issuer/… → cert-manager; Bundle → trust-manager.
2424
**Quick Start**: `oc get certmanager cluster -o yaml` | `make local-run`
2525

2626
## Critical Patterns
2727

28-
1. **Never assume uniform SSA** — CertManager library-go; IstioCSR Create+`UpdateWithRetry`; TrustManager only SSA + field owner `trust-manager-controller`. [components.md](harness-evals/harness-docs/architecture/components.md)
28+
1. **Never assume uniform SSA** — CertManager library-go; IstioCSR mixed (SSA via `common.ApplyResource` for Service/SA/Certificate/Role/RoleBinding/NetworkPolicy; Create+`UpdateWithRetry` for Deployment, CA ConfigMap, ClusterRole/Binding); TrustManager all SSA + field owner `trust-manager-controller`. [components.md](harness-evals/harness-docs/architecture/components.md)
2929
2. **Never hand-edit generated assets** — bindata.go, clients, regenerated `bindata/` / `bundle/` (`make generate`, `update-manifests`, `bundle`, `verify-bindata`).
30-
3. **Greenfield = TrustManager** — copy SSA + `HandleReconcileResult`/`FromClientError` (30s requeue); not IstioCSR. [ADR-0002](harness-evals/harness-docs/decisions/adr-0002-apply-strategies.md)
30+
3. **Greenfield = TrustManager** — copy SSA (`common.ApplyResource`) + `HandleReconcileResult`/`FromClientError` (30s requeue); not IstioCSR. [ADR-0002](harness-evals/harness-docs/decisions/adr-0002-apply-strategies.md)
3131
4. **Feature gates** — `--unsupported-addon-features`; five touchpoints; no cluster FeatureSet for TrustManager (CM-1141). [ADR-0003](harness-evals/harness-docs/decisions/adr-0003-feature-gates.md)
3232
5. **Ignore** `certmanager_controller.go` placeholder — RBAC markers only, never started.
3333
6. **RELATED_IMAGE triple-sync** — manager.yaml env ↔ controller constants ↔ CSV `relatedImages` + `make bundle`.

‎harness-evals/harness-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ FIPS: `make build` sources `hack/go-fips.sh`. WARN (non-FIPS) builds are **local
2525

2626
### New controller-runtime operand (greenfield)
2727

28-
1. Copy **TrustManager** (SSA `Patch` + unique `FieldOwner`), not IstioCSR Create+Update
28+
1. Copy **TrustManager** (SSA via `common.ApplyResource` + unique `FieldOwner`), not IstioCSR Create+Update
2929
2. Feature gate — all five touchpoints ([ADR-0003](./decisions/adr-0003-feature-gates.md))
3030
3. Image triple-sync (below) + bindata via `hack/update-*-manifests.sh` + `make update-manifests` / `verify-bindata`
3131
4. RBAC via `+kubebuilder:rbac` → `make manifests` → `make bundle` (never hand-edit CSV RBAC)
@@ -62,7 +62,7 @@ the packaged operand-version env value in `manager.yaml` omits `v`.
6262
## Common Mistakes
6363

6464
1. Hand-edit bindata / generated clients / generated `bundle/` CSV
65-
2. Assume SSA for IstioCSR or CertManager
65+
2. Assume SSA for CertManager or for every IstioCSR resource
6666
3. Logic in `certmanager_controller.go` placeholder
6767
4. Label-filtered cache for ConfigMaps or Issuer/ClusterIssuer
6868
5. Expect IstioCSR/TrustManager delete to GC operands (warn-only TODO)

‎harness-evals/harness-docs/CERT_MANAGER_OPERATOR_TESTING.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,10 @@
1717
| Controller | Assert | Fake |
1818
|------------|--------|------|
1919
| CertManager | library-go `resourceapply` / expected generation / rollout | library-go fake clientset — **no** `FakeCtrlClient` |
20-
| IstioCSR | `CreateCallCount` / `UpdateWithRetryCallCount` | `fakes.FakeCtrlClient` — **not** `PatchCallCount` |
21-
| TrustManager | `PatchCallCount` with `client.Apply` + FieldOwner `trust-manager-controller` | `fakes.FakeCtrlClient` — **not** Update/Create |
20+
| IstioCSR | `ApplyCallCount` for SSA resources (Service, SA, Certificate, Role/RoleBinding, NetworkPolicy); `CreateCallCount` / `UpdateWithRetryCallCount` for Deployment, CA ConfigMap, ClusterRole/Binding | `fakes.FakeCtrlClient` |
21+
| TrustManager | `ApplyCallCount` (SSA via `CtrlClient.Apply`, FieldOwner `trust-manager-controller`) | `fakes.FakeCtrlClient` — **not** Update/Create |
2222

23-
A TrustManager test asserting `UpdateCallCount` (or IstioCSR asserting `PatchCallCount`) is the wrong path even if green. Regenerate fakes with `make generate-fakes` after changing `CtrlClient` — never hand-edit `pkg/controller/common/fakes/`.
23+
A test asserting `UpdateCallCount` for an SSA resource, or `ApplyCallCount` for a Create/Update resource, is the wrong path even if green. `PatchCallCount` only counts the JSON merge-patch fallback `ApplyResource` uses when a drift-correcting apply fails. Regenerate fakes with `make generate-fakes` after changing `CtrlClient` — never hand-edit `pkg/controller/common/fakes/`.
2424

2525
```bash
2626
make test-unit

‎harness-evals/harness-docs/api-contracts-guidelines.md‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -175,8 +175,9 @@ types and run `make manifests` for operator CRDs; bump upstream versions and run
175175
checking its markers (`+kubebuilder:validation:*`, `+kubebuilder:default`,
176176
`XValidation`) directly.
177177
- Don't assume all three CRs share apply/reconcile semantics — CertManager uses
178-
library-go `resourceapply` (not SSA), IstioCSR uses Create+`UpdateWithRetry`, only
179-
TrustManager uses controller-runtime SSA with field owner `trust-manager-controller`.
178+
library-go `resourceapply` (not SSA), IstioCSR mixes SSA (field owner `istio-csr-controller`)
179+
with Create+`UpdateWithRetry`, and TrustManager uses controller-runtime SSA throughout with
180+
field owner `trust-manager-controller`.
180181
Field-ownership/conflict behavior described in docs must match the actual controller.
181182
- Don't document `UnsupportedConfigOverrides.{Controller,Webhook,CAInjector}` as
182183
first-class `Spec` fields — they're helper types for parsing

‎harness-evals/harness-docs/architecture/components.md‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,13 +51,13 @@ Note: Informers start **before** library-go controllers run — do not assume fi
5151
|-----------|-------------|----------|--------------|
5252
| Framework | library-go | controller-runtime | controller-runtime |
5353
| CR | Cluster `cluster` | Namespaced `default` | Cluster `cluster` |
54-
| Apply | `resourceapply` / DeploymentController | Create + `UpdateWithRetry` | **SSA** `client.Apply` + `ForceOwnership` |
55-
| Field owner | n/a (library-go) | n/a | `trust-manager-controller` |
54+
| Apply | `resourceapply` / DeploymentController | SSA (`common.ApplyResource`) for most resources; Create + `UpdateWithRetry` for Deployment, CA ConfigMap, ClusterRole/Binding | **SSA** `common.ApplyResource` → `CtrlClient.Apply` + `ForceOwnership` |
55+
| Field owner | n/a (library-go) | `istio-csr-controller` | `trust-manager-controller` |
5656
| Status | `operatorv1.OperatorStatus` | `ConditionalStatus` | `ConditionalStatus` |
5757
| Gate | Always on | `IstioCSR` GA default true | `TrustManager` TP default false |
5858
| Finalizer cleanup | library-go | TODO warn-only | TODO warn-only |
5959

60-
Evidence: TrustManager `services.go` / `deployments.go` (`Patch` + `client.Apply`); IstioCSR `services.go` / `deployments.go` (`UpdateWithRetry`); CertManager `cert_manager_networkpolicy.go` (`resourceapply.ApplyNetworkPolicy`).
60+
Evidence: `common/applier.go` (`ApplyResource`); TrustManager `services.go` / `deployments.go` (`ApplyResource`); IstioCSR `services.go` (`ApplyResource`) / `deployments.go` (`UpdateWithRetry`); CertManager `cert_manager_networkpolicy.go` (`resourceapply.ApplyNetworkPolicy`).
6161

6262
**Greenfield rule**: New controller-runtime operands → copy **TrustManager**, not IstioCSR.
6363

@@ -174,7 +174,7 @@ TrustManager/cluster ──ctrl-runtime SSA──► trust-manager Deployment (+
174174
## SME Review Recommended
175175

176176
- Exact uninstall / finalizer cleanup design intent for IstioCSR & TrustManager.
177-
- Whether IstioCSR will be migrated to SSA.
177+
- Whether IstioCSR Deployment, CA ConfigMap and ClusterRole/Binding will move to SSA.
178178
- Operational gotchas not visible in code (Slack/Jira) — Phase 5.5 chai-bot skipped.
179179
- Recipe nuances for adding a fourth operand controller (CSV env, RBAC, relatedImages checklist).
180180

‎harness-evals/harness-docs/decisions/adr-0002-apply-strategies.md‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
**Status**: Accepted
44
**Date**: 2026-03-11 (TrustManager SSA; IstioCSR remains Create+Update)
5+
**Amended**: CM-1040 — shared `common.ApplyResource` helper; IstioCSR namespaced resources migrated to SSA
56
**Deciders**: cert-manager-operator maintainers
67

78
## Context
@@ -13,8 +14,8 @@ Documenting a single “we use SSA everywhere” claim is a known hallucination
1314
## Decision
1415

1516
1. CertManager continues with library-go apply helpers.
16-
2. IstioCSR keeps Create+Update until explicitly migrated.
17-
3. **New controller-runtime operand reconcilers must follow TrustManager SSA** (`Patch` + `client.Apply` + `FieldOwner` + `ForceOwnership`).
17+
2. IstioCSR uses SSA via `common.ApplyResource` for Service, ServiceAccount, Certificate, Role/RoleBinding and NetworkPolicy. Deployment, CA ConfigMap and ClusterRole/ClusterRoleBinding (generated names tracked in status) keep Create+Update until explicitly migrated.
18+
3. **New controller-runtime operand reconcilers must follow TrustManager SSA** (`common.ApplyResource` → `CtrlClient.Apply` + `FieldOwner` + `ForceOwnership`, with a JSON merge-patch fallback when a drift-correcting apply fails).
1819
4. Status updates for IstioCSR/TrustManager may still use `UpdateWithRetry` on the CR object.
1920

2021
## Consequences
@@ -34,10 +35,11 @@ Documenting a single “we use SSA everywhere” claim is a known hallucination
3435

3536
## References
3637

38+
- Shared helper: `pkg/controller/common/applier.go` (`ApplyResource`), `client.go` (`CtrlClient.Apply`)
3739
- TrustManager: `pkg/controller/trustmanager/services.go`, `deployments.go`, `constants.go` (`fieldOwner`)
38-
- IstioCSR: `pkg/controller/istiocsr/services.go`, `deployments.go`
40+
- IstioCSR: `pkg/controller/istiocsr/services.go` (SSA), `deployments.go` (Create+Update), `constants.go` (`fieldOwner`)
3941
- CertManager: `pkg/controller/certmanager/cert_manager_networkpolicy.go` (`resourceapply.ApplyNetworkPolicy`)
4042

4143
## SME Review Recommended
4244

43-
Whether/when IstioCSR migrates to SSA; field-owner naming convention for future operands.
45+
Whether/when the remaining IstioCSR Create+Update resources migrate to SSA; field-owner naming convention for future operands.

‎harness-evals/harness-docs/domain/istiocsr.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010

1111
Deploys and configures the **cert-manager-istio-csr** operand so Istio mesh workloads can obtain certificates via cert-manager issuers.
1212

13-
**Key Principle**: Enabled by operator feature gate `IstioCSR` (GA, default **true**) via `--unsupported-addon-features`. Controller uses **Create + UpdateWithRetry**, not SSA.
13+
**Key Principle**: Enabled by operator feature gate `IstioCSR` (GA, default **true**) via `--unsupported-addon-features`. Controller is **mixed**: SSA via `common.ApplyResource` for most resources, Create + `UpdateWithRetry` for the rest (see Apply below).
1414

1515
## Spec Structure
1616

@@ -66,7 +66,7 @@ Condition types (`conditions.go`): `Ready`, `Degraded`. Reasons: `Failed`, `Read
6666
| Concern | Behavior | Code |
6767
|---------|----------|------|
6868
| Image | `RELATED_IMAGE_CERT_MANAGER_ISTIOCSR` | `istiocsr/constants.go` |
69-
| Apply | Exists? update via `UpdateWithRetry`; else Create | e.g. `services.go`, `deployments.go` |
69+
| Apply | SSA via `common.ApplyResource` (field owner `istio-csr-controller`) for Service, ServiceAccount, Certificate, Role/RoleBinding, NetworkPolicy; Create + `UpdateWithRetry` for Deployment, CA ConfigMap, ClusterRole/ClusterRoleBinding | SSA: `services.go`, `serviceaccounts.go`, `certificates.go`, `rbacs.go`, `networkpolicies.go`; Create+Update: `deployments.go`, `rbacs.go` |
7070
| Singleton | Disallows multiple IstioCSR instances | `disallowMultipleIstioCSRInstances` |
7171
| Watches | Certificate, Deployment, RBAC, Service, SA, ConfigMap, Secret (metadata), NetworkPolicy, Issuer, ClusterIssuer | `controller.go` |
7272
| Labels | Managed label value `cert-manager-istio-csr`; watch label `istiocsr.openshift.operator.io/watched-by` | `constants.go` |

‎harness-evals/harness-docs/domain/trustmanager.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ Same `Ready` / `Degraded` condition types as IstioCSR (`conditions.go`).
6464
| Concern | Behavior | Code |
6565
|---------|----------|------|
6666
| Image | `RELATED_IMAGE_CERT_MANAGER_TRUST_MANAGER` | `trustmanager/constants.go` |
67-
| Apply | `Patch(..., client.Apply, FieldOwner("trust-manager-controller"), ForceOwnership)` | `services.go:41`, `deployments.go:40`, … |
67+
| Apply | `common.ApplyResource` → `CtrlClient.Apply(..., FieldOwner("trust-manager-controller"), ForceOwnership)` | `services.go`, `deployments.go`, `common/applier.go` |
6868
| Skip when unchanged | Managed-field-aware `*Modified` helpers before apply | `utils.go` / resource files |
6969
| Trusted CA | Watches CNO-injected ConfigMap `cert-manager-operator-trusted-ca-bundle` (separate from CertManager `--trusted-ca-configmap`) | `controller.go`, `configmaps.go` |
7070
| SecretTargets | `Disabled` (default) or `Custom`. Custom requires non-empty `authorizedSecrets`; write RBAC scoped via `ResourceNames` (cluster-wide read is intentional). Sort secrets before ClusterRole apply for deterministic diffs. | `rbacs.go` `appendSecretTargetRules` |
@@ -75,7 +75,7 @@ Same `Ready` / `Degraded` condition types as IstioCSR (`conditions.go`).
7575

7676
## Common Mistakes
7777

78-
1. Documenting “all controllers use SSA” — false; only TrustManager among the three.
78+
1. Documenting “all controllers use SSA” — false; CertManager uses library-go, and IstioCSR still uses Create+Update for some resources.
7979
2. Expecting Bundle reconciliation in the operator — operator deploys trust-manager; Bundle is operand API.
8080
3. Hand-editing `bindata/trust-manager` YAML without `make update-manifests`.
8181
4. Granting wildcard Secret **write** for SecretTargets — keep write limited to `authorizedSecrets` ResourceNames.

‎harness-evals/harness-docs/operator-controllers-guidelines.md‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,12 @@ package you're editing; do not "fix" one to look like another without an ADR upd
2828
| Package | Apply method | Evidence |
2929
|---|---|---|
3030
| `certmanager` | library-go `resourceapply.*` / `staticresourcecontroller` / `DeploymentController` | `cert_manager_networkpolicy.go` (`resourceapply.ApplyNetworkPolicy`) |
31-
| `istiocsr` | Imperative **Create, then `UpdateWithRetry`** on diff | `services.go`, `deployments.go`: `r.Create(...)` / `r.UpdateWithRetry(...)` |
32-
| `trustmanager` | **Server-Side Apply (SSA)**: `client.Patch(obj, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership)` | `services.go`, `deployments.go`; field owner `trust-manager-controller` in `constants.go` |
31+
| `istiocsr` | **Mixed**: SSA via `common.ApplyResource` (field owner `istio-csr-controller`) for Service, ServiceAccount, Certificate, Role/RoleBinding, NetworkPolicy; imperative **Create, then `UpdateWithRetry`** for Deployment, CA ConfigMap, ClusterRole/ClusterRoleBinding | `services.go` (`ApplyResource`); `deployments.go`: `r.Create(...)` / `r.UpdateWithRetry(...)` |
32+
| `trustmanager` | **Server-Side Apply (SSA)**: `common.ApplyResource` → `CtrlClient.Apply(obj, client.FieldOwner(fieldOwner), client.ForceOwnership)` | `services.go`, `deployments.go`; field owner `trust-manager-controller` in `constants.go` |
3333

3434
Do **not** claim "we use SSA everywhere" — this is a documented hallucination failure
35-
mode (ADR-0002). IstioCSR remains Create+Update until explicitly migrated; there is no
36-
committed timeline for that migration.
35+
mode (ADR-0002). IstioCSR's Deployment, CA ConfigMap and ClusterRole/ClusterRoleBinding
36+
remain Create+Update until explicitly migrated; there is no committed timeline for that.
3737

3838
## 3. Greenfield Rule: New ctrl-runtime Operand → Copy TrustManager
3939

@@ -42,7 +42,7 @@ When adding a **new** feature-gated operand controller under `pkg/controller/`:
4242
1. **Copy the TrustManager package structure**, not IstioCSR:
4343
`controller.go` (Reconcile/SetupWithManager), `deployments.go`, `services.go`,
4444
`serviceaccounts.go`, `rbacs.go`, `webhooks.go`, `constants.go`, `install_*.go`, `utils.go`.
45-
2. Apply resources via `client.Patch(desired, client.Apply, client.FieldOwner(<name>-controller), client.ForceOwnership)`.
45+
2. Apply resources via `common.ApplyResource(ctx, r.CtrlClient, ..., fieldOwner, <kind>Modified)`, which server-side applies with `client.FieldOwner(<name>-controller)` + `client.ForceOwnership`.
4646
Pick a unique `fieldOwner` constant per controller.
4747
3. Diff only fields you manage via SSA (see `serviceModified` pattern in
4848
`trustmanager/services.go`) — do not `reflect.DeepEqual` the whole object.

‎harness-evals/harness-docs/security-guidelines.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,9 @@ if effective.MinTLSVersion == configv1.VersionTLS13 {
6161
- library-go (CertManager) RBAC is not marker-driven — its ClusterRole comes from
6262
`bindata/cert-manager-deployment/**`/`config/rbac/*`. Don't add `+kubebuilder:rbac` markers expecting
6363
them to affect the CertManager controller set.
64-
- Apply methods differ per controller (library-go `resourceapply`, IstioCSR `Create+UpdateWithRetry`,
65-
TrustManager SSA `client.Apply` + field owner `trust-manager-controller`) — when adding a new RBAC
66-
object for TrustManager, follow the `*Modified` diff-then-`Patch(client.Apply, ForceOwnership)`
64+
- Apply methods differ per controller (library-go `resourceapply`, IstioCSR mixed SSA +
65+
`Create+UpdateWithRetry`, TrustManager SSA + field owner `trust-manager-controller`) — when adding a new RBAC
66+
object for TrustManager, follow the `*Modified` diff-then-`common.ApplyResource` (SSA, `ForceOwnership`)
6767
pattern in `rbacs.go`, not a blind re-apply.
6868

6969
## Secrets Handling (TrustManager `secretTargets`)

0 commit comments

Comments
 (0)