diff --git a/AGENTS.md b/AGENTS.md index 40990ea33..060663a74 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,7 +17,7 @@ Deploys/configures upstream **cert-manager** (and optionally **istio-csr** / **t | CR | Scope / name | Stack | Gate | |----|--------------|-------|------| | [CertManager](harness-evals/harness-docs/domain/certmanager.md) | Cluster / `cluster` | library-go `resourceapply` | Always on | -| [IstioCSR](harness-evals/harness-docs/domain/istiocsr.md) | Namespaced / `default` | Create+Update | `IstioCSR` GA default true | +| [IstioCSR](harness-evals/harness-docs/domain/istiocsr.md) | Namespaced / `default` | SSA + Create/Update (mixed) | `IstioCSR` GA default true | | [TrustManager](harness-evals/harness-docs/domain/trustmanager.md) | Cluster / `cluster` | **SSA** | `TrustManager` TP default false | **Operands**: Certificate/Issuer/… → cert-manager; Bundle → trust-manager. @@ -25,9 +25,9 @@ Deploys/configures upstream **cert-manager** (and optionally **istio-csr** / **t ## Critical Patterns -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) +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) 2. **Never hand-edit generated assets** — bindata.go, clients, regenerated `bindata/` / `bundle/` (`make generate`, `update-manifests`, `bundle`, `verify-bindata`). -3. **Greenfield = TrustManager** — copy SSA + `HandleReconcileResult`/`FromClientError` (30s requeue); not IstioCSR. [ADR-0002](harness-evals/harness-docs/decisions/adr-0002-apply-strategies.md) +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) 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) 5. **Ignore** `certmanager_controller.go` placeholder — RBAC markers only, never started. 6. **RELATED_IMAGE triple-sync** — manager.yaml env ↔ controller constants ↔ CSV `relatedImages` + `make bundle`. diff --git a/harness-evals/harness-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md b/harness-evals/harness-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md index 4ba6d3937..128dce2e4 100644 --- a/harness-evals/harness-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md +++ b/harness-evals/harness-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md @@ -25,7 +25,7 @@ FIPS: `make build` sources `hack/go-fips.sh`. WARN (non-FIPS) builds are **local ### New controller-runtime operand (greenfield) -1. Copy **TrustManager** (SSA `Patch` + unique `FieldOwner`), not IstioCSR Create+Update +1. Copy **TrustManager** (SSA via `common.ApplyResource` + unique `FieldOwner`), not IstioCSR Create+Update 2. Feature gate — all five touchpoints ([ADR-0003](./decisions/adr-0003-feature-gates.md)) 3. Image triple-sync (below) + bindata via `hack/update-*-manifests.sh` + `make update-manifests` / `verify-bindata` 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`. ## Common Mistakes 1. Hand-edit bindata / generated clients / generated `bundle/` CSV -2. Assume SSA for IstioCSR or CertManager +2. Assume SSA for CertManager or for every IstioCSR resource 3. Logic in `certmanager_controller.go` placeholder 4. Label-filtered cache for ConfigMaps or Issuer/ClusterIssuer 5. Expect IstioCSR/TrustManager delete to GC operands (warn-only TODO) diff --git a/harness-evals/harness-docs/CERT_MANAGER_OPERATOR_TESTING.md b/harness-evals/harness-docs/CERT_MANAGER_OPERATOR_TESTING.md index a92dcb46e..5fd4108c8 100644 --- a/harness-evals/harness-docs/CERT_MANAGER_OPERATOR_TESTING.md +++ b/harness-evals/harness-docs/CERT_MANAGER_OPERATOR_TESTING.md @@ -17,10 +17,10 @@ | Controller | Assert | Fake | |------------|--------|------| | CertManager | library-go `resourceapply` / expected generation / rollout | library-go fake clientset — **no** `FakeCtrlClient` | -| IstioCSR | `CreateCallCount` / `UpdateWithRetryCallCount` | `fakes.FakeCtrlClient` — **not** `PatchCallCount` | -| TrustManager | `PatchCallCount` with `client.Apply` + FieldOwner `trust-manager-controller` | `fakes.FakeCtrlClient` — **not** Update/Create | +| IstioCSR | `ApplyCallCount` for SSA resources (Service, SA, Certificate, Role/RoleBinding, NetworkPolicy); `CreateCallCount` / `UpdateWithRetryCallCount` for Deployment, CA ConfigMap, ClusterRole/Binding | `fakes.FakeCtrlClient` | +| TrustManager | `ApplyCallCount` (SSA via `CtrlClient.Apply`, FieldOwner `trust-manager-controller`) | `fakes.FakeCtrlClient` — **not** Update/Create | -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/`. +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/`. ```bash make test-unit diff --git a/harness-evals/harness-docs/api-contracts-guidelines.md b/harness-evals/harness-docs/api-contracts-guidelines.md index bf68e235d..c71882083 100644 --- a/harness-evals/harness-docs/api-contracts-guidelines.md +++ b/harness-evals/harness-docs/api-contracts-guidelines.md @@ -175,8 +175,9 @@ types and run `make manifests` for operator CRDs; bump upstream versions and run checking its markers (`+kubebuilder:validation:*`, `+kubebuilder:default`, `XValidation`) directly. - Don't assume all three CRs share apply/reconcile semantics — CertManager uses - library-go `resourceapply` (not SSA), IstioCSR uses Create+`UpdateWithRetry`, only - TrustManager uses controller-runtime SSA with field owner `trust-manager-controller`. + library-go `resourceapply` (not SSA), IstioCSR mixes SSA (field owner `istio-csr-controller`) + with Create+`UpdateWithRetry`, and TrustManager uses controller-runtime SSA throughout with + field owner `trust-manager-controller`. Field-ownership/conflict behavior described in docs must match the actual controller. - Don't document `UnsupportedConfigOverrides.{Controller,Webhook,CAInjector}` as first-class `Spec` fields — they're helper types for parsing diff --git a/harness-evals/harness-docs/architecture/components.md b/harness-evals/harness-docs/architecture/components.md index df39e3b0a..2933e9e34 100644 --- a/harness-evals/harness-docs/architecture/components.md +++ b/harness-evals/harness-docs/architecture/components.md @@ -51,13 +51,13 @@ Note: Informers start **before** library-go controllers run — do not assume fi |-----------|-------------|----------|--------------| | Framework | library-go | controller-runtime | controller-runtime | | CR | Cluster `cluster` | Namespaced `default` | Cluster `cluster` | -| Apply | `resourceapply` / DeploymentController | Create + `UpdateWithRetry` | **SSA** `client.Apply` + `ForceOwnership` | -| Field owner | n/a (library-go) | n/a | `trust-manager-controller` | +| Apply | `resourceapply` / DeploymentController | SSA (`common.ApplyResource`) for most resources; Create + `UpdateWithRetry` for Deployment, CA ConfigMap, ClusterRole/Binding | **SSA** `common.ApplyResource` → `CtrlClient.Apply` + `ForceOwnership` | +| Field owner | n/a (library-go) | `istio-csr-controller` | `trust-manager-controller` | | Status | `operatorv1.OperatorStatus` | `ConditionalStatus` | `ConditionalStatus` | | Gate | Always on | `IstioCSR` GA default true | `TrustManager` TP default false | | Finalizer cleanup | library-go | TODO warn-only | TODO warn-only | -Evidence: TrustManager `services.go` / `deployments.go` (`Patch` + `client.Apply`); IstioCSR `services.go` / `deployments.go` (`UpdateWithRetry`); CertManager `cert_manager_networkpolicy.go` (`resourceapply.ApplyNetworkPolicy`). +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`). **Greenfield rule**: New controller-runtime operands → copy **TrustManager**, not IstioCSR. @@ -174,7 +174,7 @@ TrustManager/cluster ──ctrl-runtime SSA──► trust-manager Deployment (+ ## SME Review Recommended - Exact uninstall / finalizer cleanup design intent for IstioCSR & TrustManager. -- Whether IstioCSR will be migrated to SSA. +- Whether IstioCSR Deployment, CA ConfigMap and ClusterRole/Binding will move to SSA. - Operational gotchas not visible in code (Slack/Jira) — Phase 5.5 chai-bot skipped. - Recipe nuances for adding a fourth operand controller (CSV env, RBAC, relatedImages checklist). diff --git a/harness-evals/harness-docs/decisions/adr-0002-apply-strategies.md b/harness-evals/harness-docs/decisions/adr-0002-apply-strategies.md index 9cdb1c98a..eb8e9762e 100644 --- a/harness-evals/harness-docs/decisions/adr-0002-apply-strategies.md +++ b/harness-evals/harness-docs/decisions/adr-0002-apply-strategies.md @@ -2,6 +2,7 @@ **Status**: Accepted **Date**: 2026-03-11 (TrustManager SSA; IstioCSR remains Create+Update) +**Amended**: CM-1040 — shared `common.ApplyResource` helper; IstioCSR namespaced resources migrated to SSA **Deciders**: cert-manager-operator maintainers ## Context @@ -13,8 +14,8 @@ Documenting a single “we use SSA everywhere” claim is a known hallucination ## Decision 1. CertManager continues with library-go apply helpers. -2. IstioCSR keeps Create+Update until explicitly migrated. -3. **New controller-runtime operand reconcilers must follow TrustManager SSA** (`Patch` + `client.Apply` + `FieldOwner` + `ForceOwnership`). +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. +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). 4. Status updates for IstioCSR/TrustManager may still use `UpdateWithRetry` on the CR object. ## Consequences @@ -34,10 +35,11 @@ Documenting a single “we use SSA everywhere” claim is a known hallucination ## References +- Shared helper: `pkg/controller/common/applier.go` (`ApplyResource`), `client.go` (`CtrlClient.Apply`) - TrustManager: `pkg/controller/trustmanager/services.go`, `deployments.go`, `constants.go` (`fieldOwner`) -- IstioCSR: `pkg/controller/istiocsr/services.go`, `deployments.go` +- IstioCSR: `pkg/controller/istiocsr/services.go` (SSA), `deployments.go` (Create+Update), `constants.go` (`fieldOwner`) - CertManager: `pkg/controller/certmanager/cert_manager_networkpolicy.go` (`resourceapply.ApplyNetworkPolicy`) ## SME Review Recommended -Whether/when IstioCSR migrates to SSA; field-owner naming convention for future operands. +Whether/when the remaining IstioCSR Create+Update resources migrate to SSA; field-owner naming convention for future operands. diff --git a/harness-evals/harness-docs/domain/istiocsr.md b/harness-evals/harness-docs/domain/istiocsr.md index ca1162a06..9f5fc0346 100644 --- a/harness-evals/harness-docs/domain/istiocsr.md +++ b/harness-evals/harness-docs/domain/istiocsr.md @@ -10,7 +10,7 @@ Deploys and configures the **cert-manager-istio-csr** operand so Istio mesh workloads can obtain certificates via cert-manager issuers. -**Key Principle**: Enabled by operator feature gate `IstioCSR` (GA, default **true**) via `--unsupported-addon-features`. Controller uses **Create + UpdateWithRetry**, not SSA. +**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). ## Spec Structure @@ -66,7 +66,7 @@ Condition types (`conditions.go`): `Ready`, `Degraded`. Reasons: `Failed`, `Read | Concern | Behavior | Code | |---------|----------|------| | Image | `RELATED_IMAGE_CERT_MANAGER_ISTIOCSR` | `istiocsr/constants.go` | -| Apply | Exists? update via `UpdateWithRetry`; else Create | e.g. `services.go`, `deployments.go` | +| 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` | | Singleton | Disallows multiple IstioCSR instances | `disallowMultipleIstioCSRInstances` | | Watches | Certificate, Deployment, RBAC, Service, SA, ConfigMap, Secret (metadata), NetworkPolicy, Issuer, ClusterIssuer | `controller.go` | | Labels | Managed label value `cert-manager-istio-csr`; watch label `istiocsr.openshift.operator.io/watched-by` | `constants.go` | diff --git a/harness-evals/harness-docs/domain/trustmanager.md b/harness-evals/harness-docs/domain/trustmanager.md index f91b2ad9a..5ac4865ab 100644 --- a/harness-evals/harness-docs/domain/trustmanager.md +++ b/harness-evals/harness-docs/domain/trustmanager.md @@ -64,7 +64,7 @@ Same `Ready` / `Degraded` condition types as IstioCSR (`conditions.go`). | Concern | Behavior | Code | |---------|----------|------| | Image | `RELATED_IMAGE_CERT_MANAGER_TRUST_MANAGER` | `trustmanager/constants.go` | -| Apply | `Patch(..., client.Apply, FieldOwner("trust-manager-controller"), ForceOwnership)` | `services.go:41`, `deployments.go:40`, … | +| Apply | `common.ApplyResource` → `CtrlClient.Apply(..., FieldOwner("trust-manager-controller"), ForceOwnership)` | `services.go`, `deployments.go`, `common/applier.go` | | Skip when unchanged | Managed-field-aware `*Modified` helpers before apply | `utils.go` / resource files | | Trusted CA | Watches CNO-injected ConfigMap `cert-manager-operator-trusted-ca-bundle` (separate from CertManager `--trusted-ca-configmap`) | `controller.go`, `configmaps.go` | | 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`). ## Common Mistakes -1. Documenting “all controllers use SSA” — false; only TrustManager among the three. +1. Documenting “all controllers use SSA” — false; CertManager uses library-go, and IstioCSR still uses Create+Update for some resources. 2. Expecting Bundle reconciliation in the operator — operator deploys trust-manager; Bundle is operand API. 3. Hand-editing `bindata/trust-manager` YAML without `make update-manifests`. 4. Granting wildcard Secret **write** for SecretTargets — keep write limited to `authorizedSecrets` ResourceNames. diff --git a/harness-evals/harness-docs/operator-controllers-guidelines.md b/harness-evals/harness-docs/operator-controllers-guidelines.md index 6c91dc279..d2397fc10 100644 --- a/harness-evals/harness-docs/operator-controllers-guidelines.md +++ b/harness-evals/harness-docs/operator-controllers-guidelines.md @@ -28,12 +28,12 @@ package you're editing; do not "fix" one to look like another without an ADR upd | Package | Apply method | Evidence | |---|---|---| | `certmanager` | library-go `resourceapply.*` / `staticresourcecontroller` / `DeploymentController` | `cert_manager_networkpolicy.go` (`resourceapply.ApplyNetworkPolicy`) | -| `istiocsr` | Imperative **Create, then `UpdateWithRetry`** on diff | `services.go`, `deployments.go`: `r.Create(...)` / `r.UpdateWithRetry(...)` | -| `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` | +| `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(...)` | +| `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` | Do **not** claim "we use SSA everywhere" — this is a documented hallucination failure -mode (ADR-0002). IstioCSR remains Create+Update until explicitly migrated; there is no -committed timeline for that migration. +mode (ADR-0002). IstioCSR's Deployment, CA ConfigMap and ClusterRole/ClusterRoleBinding +remain Create+Update until explicitly migrated; there is no committed timeline for that. ## 3. Greenfield Rule: New ctrl-runtime Operand → Copy TrustManager @@ -42,7 +42,7 @@ When adding a **new** feature-gated operand controller under `pkg/controller/`: 1. **Copy the TrustManager package structure**, not IstioCSR: `controller.go` (Reconcile/SetupWithManager), `deployments.go`, `services.go`, `serviceaccounts.go`, `rbacs.go`, `webhooks.go`, `constants.go`, `install_*.go`, `utils.go`. -2. Apply resources via `client.Patch(desired, client.Apply, client.FieldOwner(-controller), client.ForceOwnership)`. +2. Apply resources via `common.ApplyResource(ctx, r.CtrlClient, ..., fieldOwner, Modified)`, which server-side applies with `client.FieldOwner(-controller)` + `client.ForceOwnership`. Pick a unique `fieldOwner` constant per controller. 3. Diff only fields you manage via SSA (see `serviceModified` pattern in `trustmanager/services.go`) — do not `reflect.DeepEqual` the whole object. diff --git a/harness-evals/harness-docs/security-guidelines.md b/harness-evals/harness-docs/security-guidelines.md index 7275ea06f..2a289a443 100644 --- a/harness-evals/harness-docs/security-guidelines.md +++ b/harness-evals/harness-docs/security-guidelines.md @@ -61,9 +61,9 @@ if effective.MinTLSVersion == configv1.VersionTLS13 { - library-go (CertManager) RBAC is not marker-driven — its ClusterRole comes from `bindata/cert-manager-deployment/**`/`config/rbac/*`. Don't add `+kubebuilder:rbac` markers expecting them to affect the CertManager controller set. -- Apply methods differ per controller (library-go `resourceapply`, IstioCSR `Create+UpdateWithRetry`, - TrustManager SSA `client.Apply` + field owner `trust-manager-controller`) — when adding a new RBAC - object for TrustManager, follow the `*Modified` diff-then-`Patch(client.Apply, ForceOwnership)` +- Apply methods differ per controller (library-go `resourceapply`, IstioCSR mixed SSA + + `Create+UpdateWithRetry`, TrustManager SSA + field owner `trust-manager-controller`) — when adding a new RBAC + object for TrustManager, follow the `*Modified` diff-then-`common.ApplyResource` (SSA, `ForceOwnership`) pattern in `rbacs.go`, not a blind re-apply. ## Secrets Handling (TrustManager `secretTargets`) diff --git a/harness-evals/harness-docs/testing-guidelines.md b/harness-evals/harness-docs/testing-guidelines.md index df4ea8ba9..10261a4b3 100644 --- a/harness-evals/harness-docs/testing-guidelines.md +++ b/harness-evals/harness-docs/testing-guidelines.md @@ -54,10 +54,10 @@ Apply strategy differs **per controller stack** — assert the call that actuall | Controller | Stack | Apply call to assert | Fake | |------------|-------|----------------------|------| | `pkg/controller/certmanager` | library-go `resourceapply` | `resourceapply.ApplyDeployment`/equivalent return values and `ExpectedGeneration`/`forceRollout` diffs | plain `library-go` fake clientset (no counterfeiter) | -| `pkg/controller/istiocsr` | ctrl-runtime, imperative | `Create` then `UpdateWithRetry` (see `services.go`, `deployments.go`) | `fakes.FakeCtrlClient` — assert `CreateCallCount`/`UpdateWithRetryCallCount`, **not** `PatchCallCount` | -| `pkg/controller/trustmanager` | ctrl-runtime **SSA** | `Patch(ctx, obj, client.Apply, client.FieldOwner("trust-manager-controller"), client.ForceOwnership)` | `fakes.FakeCtrlClient` — assert `PatchCallCount`, **not** `Update`/`Create` | +| `pkg/controller/istiocsr` | ctrl-runtime, mixed | SSA `Apply` via `common.ApplyResource` (Service, SA, Certificate, Role/RoleBinding, NetworkPolicy); `Create` then `UpdateWithRetry` (Deployment, CA ConfigMap, ClusterRole/Binding) | `fakes.FakeCtrlClient` — assert `ApplyCallCount` or `CreateCallCount`/`UpdateWithRetryCallCount` to match the resource | +| `pkg/controller/trustmanager` | ctrl-runtime **SSA** | `Apply(ctx, obj, client.FieldOwner("trust-manager-controller"), client.ForceOwnership)` via `common.ApplyResource` | `fakes.FakeCtrlClient` — assert `ApplyCallCount`, **not** `Update`/`Create` | -A TrustManager test asserting `UpdateCallCount` (or an IstioCSR test asserting `PatchCallCount`) is testing the wrong call path even if it happens to pass — verify against the controller's actual client method before trusting a green test. +A test asserting `UpdateCallCount` for an SSA resource (or `ApplyCallCount` for a Create/Update resource) is testing the wrong call path even if it happens to pass — verify against the controller's actual client method before trusting a green test. Common shape for TrustManager/IstioCSR unit tests (table-driven, one `Reconciler` + fresh fake per case): @@ -69,7 +69,7 @@ mock.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Obje r := testReconciler(t) err := r.createOrApplyDeployment(cr, labels, annotations, hash) assertError(t, err, tt.wantErr) -if got := mock.PatchCallCount(); got != tt.wantPatchCount { ... } // trustmanager +if got := mock.ApplyCallCount(); got != tt.wantApplyCount { ... } // trustmanager ``` CertManager tests are library-go-oriented and live beside the controllers (`*_test.go` in `pkg/controller/certmanager/`); they don't use the counterfeiter fake at all — use the existing `test_constants_test.go` / `deployment_helper_test.go` fixtures instead of inventing a new fake client for that package. @@ -90,6 +90,7 @@ type CtrlClient interface { Create(context.Context, client.Object, ...client.CreateOption) error Delete(context.Context, client.Object, ...client.DeleteOption) error Patch(context.Context, client.Object, client.Patch, ...client.PatchOption) error + Apply(context.Context, client.Object, ...client.ApplyOption) error Exists(context.Context, client.ObjectKey, client.Object) (bool, error) } ``` diff --git a/pkg/controller/common/applier.go b/pkg/controller/common/applier.go new file mode 100644 index 000000000..5cd19a8af --- /dev/null +++ b/pkg/controller/common/applier.go @@ -0,0 +1,77 @@ +package common + +import ( + "context" + "reflect" + + "github.com/go-logr/logr" + + corev1 "k8s.io/api/core/v1" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// ApplyResource reconciles a desired Kubernetes object to its desired state. +// It checks whether the resource already exists and whether managed fields have +// drifted. If no drift is detected it returns early. +// +// Both creation and drift correction use Server-Side Apply with the given +// fieldOwner, forcing ownership. SSA preserves fields managed by other +// controllers (e.g. the CA bundle a CA injector writes into a webhook +// configuration) and prunes fields this manager previously applied but no +// longer desires (e.g. a pod-template annotation tied to a disabled feature). +// +// SSA cannot, however, correct drift in the key fields of a granular list-map +// entry (e.g. Service spec.ports, keyed by port and protocol): the applied +// entry is added alongside the drifted one and fails validation with duplicate +// list keys. When a drift-correction apply fails, ApplyResource falls back to +// a JSON merge patch, which replaces lists wholesale and restores the desired +// entry. +// +// T is the concrete object type (e.g. *corev1.Service). The hasChanged callback +// receives typed desired and existing objects so callers can compare fields +// without type assertions. +func ApplyResource[T client.Object]( + ctx context.Context, + c CtrlClient, + log logr.Logger, + recorder record.EventRecorder, + owner client.Object, + desired T, + existing T, + fieldOwner string, + hasChanged func(desired, existing T) bool, +) error { + key := client.ObjectKeyFromObject(desired) + kind := reflect.TypeOf(desired).Elem().Name() + + log.V(4).Info("reconciling resource", "kind", kind, "name", key) + + exists, err := c.Exists(ctx, key, existing) + if err != nil { + return FromClientError(err, "failed to check if %s %q exists", kind, key) + } + if exists && !hasChanged(desired, existing) { + log.V(4).Info("resource exists and is in desired state", "kind", kind, "name", key) + return nil + } + + log.V(2).Info("applying resource to desired state", "kind", kind, "name", key) + if err := c.Apply(ctx, desired, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { + if !exists { + return FromClientError(err, "failed to apply %s %q", kind, key) + } + // SSA cannot correct drift in the key fields of a granular list-map + // entry (e.g. Service spec.ports, keyed by port and protocol): the + // applied entry is added alongside the drifted one and fails + // validation with duplicate list keys. Fall back to a JSON merge + // patch, which replaces lists wholesale and restores the desired + // entry. + if mergeErr := c.Patch(ctx, desired, client.Merge, client.FieldOwner(fieldOwner)); mergeErr != nil { + return FromClientError(err, "failed to apply %s %q", kind, key) + } + } + + recorder.Eventf(owner, corev1.EventTypeNormal, "Reconciled", "%s %s applied", kind, key) + return nil +} diff --git a/pkg/controller/common/applier_test.go b/pkg/controller/common/applier_test.go new file mode 100644 index 000000000..ab240f6ac --- /dev/null +++ b/pkg/controller/common/applier_test.go @@ -0,0 +1,281 @@ +package common + +import ( + "context" + "fmt" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +type stubCtrlClient struct { + existsResult bool + existsErr error + patchErr error + applyErr error + patchCount int + patchTypes []types.PatchType + fieldManager string + forceApply bool +} + +func (s *stubCtrlClient) Get(context.Context, client.ObjectKey, client.Object) error { + return nil +} +func (s *stubCtrlClient) List(context.Context, client.ObjectList, ...client.ListOption) error { + return nil +} +func (s *stubCtrlClient) StatusUpdate(context.Context, client.Object, ...client.SubResourceUpdateOption) error { + return nil +} +func (s *stubCtrlClient) Update(context.Context, client.Object, ...client.UpdateOption) error { + return nil +} +func (s *stubCtrlClient) UpdateWithRetry(context.Context, client.Object, ...client.UpdateOption) error { + return nil +} +func (s *stubCtrlClient) Create(context.Context, client.Object, ...client.CreateOption) error { + return nil +} +func (s *stubCtrlClient) Delete(context.Context, client.Object, ...client.DeleteOption) error { + return nil +} +func (s *stubCtrlClient) Patch(_ context.Context, _ client.Object, p client.Patch, opts ...client.PatchOption) error { + s.patchCount++ + s.patchTypes = append(s.patchTypes, p.Type()) + po := &client.PatchOptions{} + for _, o := range opts { + o.ApplyToPatch(po) + } + s.fieldManager = po.FieldManager + s.forceApply = po.Force != nil && *po.Force + return s.patchErr +} + +// Apply records server-side applies as ApplyPatchType so the assertions can +// check the order of applies and merge-patch fallbacks in one slice. +func (s *stubCtrlClient) Apply(_ context.Context, _ client.Object, opts ...client.ApplyOption) error { + s.patchCount++ + s.patchTypes = append(s.patchTypes, types.ApplyPatchType) + ao := (&client.ApplyOptions{}).ApplyOptions(opts) + s.fieldManager = ao.FieldManager + s.forceApply = ao.Force != nil && *ao.Force + if s.applyErr != nil { + return s.applyErr + } + return s.patchErr +} +func (s *stubCtrlClient) Exists(_ context.Context, _ client.ObjectKey, _ client.Object) (bool, error) { + return s.existsResult, s.existsErr +} + +func TestApplyResource_CreatesWhenNotExists(t *testing.T) { + c := &stubCtrlClient{existsResult: false} + recorder := record.NewFakeRecorder(10) + + desired := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "test-sa", Namespace: "test-ns"}, + } + owner := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "owner"}} + + err := ApplyResource( + context.Background(), c, logr.Discard(), recorder, + owner, desired, &corev1.ServiceAccount{}, "test-manager", + func(d, e *corev1.ServiceAccount) bool { return true }, + ) + require.NoError(t, err) + assert.Equal(t, 1, c.patchCount) + // Creation goes through server-side apply with force ownership. + assert.Equal(t, []types.PatchType{types.ApplyPatchType}, c.patchTypes) + assert.Equal(t, "test-manager", c.fieldManager) + assert.True(t, c.forceApply, "creation should force ownership") + + select { + case event := <-recorder.Events: + assert.Contains(t, event, "Reconciled") + default: + t.Error("expected reconcile event to be recorded") + } +} + +func TestApplyResource_NoOpWhenExistsAndUnchanged(t *testing.T) { + c := &stubCtrlClient{existsResult: true} + recorder := record.NewFakeRecorder(10) + + desired := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "test-sa", Namespace: "test-ns"}, + } + + err := ApplyResource( + context.Background(), c, logr.Discard(), recorder, + &corev1.ConfigMap{}, desired, &corev1.ServiceAccount{}, "test-manager", + func(d, e *corev1.ServiceAccount) bool { return false }, + ) + require.NoError(t, err) + assert.Equal(t, 0, c.patchCount, "should not patch when unchanged") +} + +func TestApplyResource_DriftUsesSSAWhenApplySucceeds(t *testing.T) { + c := &stubCtrlClient{existsResult: true} + recorder := record.NewFakeRecorder(10) + + desired := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "test-sa", Namespace: "test-ns"}, + } + + err := ApplyResource( + context.Background(), c, logr.Discard(), recorder, + &corev1.ConfigMap{}, desired, &corev1.ServiceAccount{}, "test-manager", + func(d, e *corev1.ServiceAccount) bool { return true }, + ) + require.NoError(t, err) + assert.Equal(t, 1, c.patchCount) + // Drift correction prefers SSA: it preserves fields owned by other + // controllers and prunes fields this manager no longer desires. + assert.Equal(t, []types.PatchType{types.ApplyPatchType}, c.patchTypes) + assert.Equal(t, "test-manager", c.fieldManager) + assert.True(t, c.forceApply, "drift correction should force ownership") +} + +func TestApplyResource_DriftFallsBackToMergeWhenApplyFails(t *testing.T) { + c := &stubCtrlClient{ + existsResult: true, + applyErr: apierrors.NewInvalid( + schema.GroupKind{Group: "core", Kind: "Service"}, "test-sa", + field.ErrorList{field.Invalid(field.NewPath("spec.ports[1].name"), "web", "Duplicate value: \"web\"")}), + } + recorder := record.NewFakeRecorder(10) + + desired := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "test-sa", Namespace: "test-ns"}, + } + + err := ApplyResource( + context.Background(), c, logr.Discard(), recorder, + &corev1.ConfigMap{}, desired, &corev1.ServiceAccount{}, "test-manager", + func(d, e *corev1.ServiceAccount) bool { return true }, + ) + require.NoError(t, err) + // SSA cannot correct drift in granular list-map key fields (the applied + // entry is added alongside the drifted one and fails validation), so the + // drift correction falls back to a JSON merge patch, which replaces lists + // wholesale. + assert.Equal(t, 2, c.patchCount) + assert.Equal(t, []types.PatchType{types.ApplyPatchType, types.MergePatchType}, c.patchTypes) + assert.Equal(t, "test-manager", c.fieldManager) + assert.False(t, c.forceApply, "merge-patch fallback should not force ownership") + + select { + case event := <-recorder.Events: + assert.Contains(t, event, "Reconciled") + default: + t.Error("expected reconcile event to be recorded") + } +} + +func TestApplyResource_DriftReturnsApplyErrorWhenBothPatchesFail(t *testing.T) { + c := &stubCtrlClient{ + existsResult: true, + applyErr: fmt.Errorf("apply failed: duplicate list keys"), + patchErr: fmt.Errorf("merge failed: forbidden"), + } + recorder := record.NewFakeRecorder(10) + + desired := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "test-sa", Namespace: "test-ns"}, + } + + err := ApplyResource( + context.Background(), c, logr.Discard(), recorder, + &corev1.ConfigMap{}, desired, &corev1.ServiceAccount{}, "test-manager", + func(d, e *corev1.ServiceAccount) bool { return true }, + ) + require.Error(t, err) + // The apply error is surfaced (not the merge fallback error) so callers + // classify the original failure. + assert.Contains(t, err.Error(), "apply failed") + assert.Contains(t, err.Error(), "failed to apply") +} + +func TestApplyResource_ExistsError(t *testing.T) { + c := &stubCtrlClient{existsErr: fmt.Errorf("connection refused")} + recorder := record.NewFakeRecorder(10) + + desired := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "test-sa", Namespace: "test-ns"}, + } + + err := ApplyResource( + context.Background(), c, logr.Discard(), recorder, + &corev1.ConfigMap{}, desired, &corev1.ServiceAccount{}, "test-manager", + func(d, e *corev1.ServiceAccount) bool { return true }, + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to check if") + assert.Equal(t, 0, c.patchCount) +} + +func TestApplyResource_PatchError(t *testing.T) { + c := &stubCtrlClient{existsResult: false, patchErr: fmt.Errorf("forbidden")} + recorder := record.NewFakeRecorder(10) + + desired := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "test-sa", Namespace: "test-ns"}, + } + + err := ApplyResource( + context.Background(), c, logr.Discard(), recorder, + &corev1.ConfigMap{}, desired, &corev1.ServiceAccount{}, "test-manager", + func(d, e *corev1.ServiceAccount) bool { return true }, + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to apply") +} + +func TestApplyResource_UnauthorizedPatchIsIrrecoverable(t *testing.T) { + c := &stubCtrlClient{existsResult: false, patchErr: apierrors.NewUnauthorized("not allowed")} + recorder := record.NewFakeRecorder(10) + + desired := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "test-sa", Namespace: "test-ns"}, + } + + err := ApplyResource( + context.Background(), c, logr.Discard(), recorder, + &corev1.ConfigMap{}, desired, &corev1.ServiceAccount{}, "test-manager", + func(d, e *corev1.ServiceAccount) bool { return true }, + ) + require.Error(t, err) + assert.True(t, IsIrrecoverableError(err), "unauthorized error should be irrecoverable") +} + +func TestApplyResource_ConflictPatchIsRetryable(t *testing.T) { + c := &stubCtrlClient{ + existsResult: false, + patchErr: apierrors.NewConflict( + schema.GroupResource{Group: "", Resource: "serviceaccounts"}, "test-sa", fmt.Errorf("conflict")), + } + recorder := record.NewFakeRecorder(10) + + desired := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "test-sa", Namespace: "test-ns"}, + } + + err := ApplyResource( + context.Background(), c, logr.Discard(), recorder, + &corev1.ConfigMap{}, desired, &corev1.ServiceAccount{}, "test-manager", + func(d, e *corev1.ServiceAccount) bool { return true }, + ) + require.Error(t, err) + assert.True(t, IsRetryRequiredError(err), "conflict error should be retryable") +} diff --git a/pkg/controller/common/client.go b/pkg/controller/common/client.go index f7d025ee2..19bb243c5 100644 --- a/pkg/controller/common/client.go +++ b/pkg/controller/common/client.go @@ -6,6 +6,8 @@ import ( "reflect" "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" @@ -30,6 +32,7 @@ type CtrlClient interface { Create(context.Context, client.Object, ...client.CreateOption) error Delete(context.Context, client.Object, ...client.DeleteOption) error Patch(context.Context, client.Object, client.Patch, ...client.PatchOption) error + Apply(context.Context, client.Object, ...client.ApplyOption) error Exists(context.Context, client.ObjectKey, client.Object) (bool, error) } @@ -137,6 +140,32 @@ func (c *ctrlClientImpl) Patch( return nil } +// Apply server-side applies obj. It converts the typed object to an +// unstructured apply configuration, since client.Client.Apply only accepts +// apply configurations, and copies the server's response back into obj. +func (c *ctrlClientImpl) Apply( + ctx context.Context, obj client.Object, opts ...client.ApplyOption, +) error { + key := client.ObjectKeyFromObject(obj) + gvk, err := c.GroupVersionKindFor(obj) + if err != nil { + return fmt.Errorf("failed to get GroupVersionKind for object %q: %w", key, err) + } + content, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj) + if err != nil { + return fmt.Errorf("failed to convert object %q to unstructured: %w", key, err) + } + u := &unstructured.Unstructured{Object: content} + u.SetGroupVersionKind(gvk) + if err := c.Client.Apply(ctx, client.ApplyConfigurationFromUnstructured(u), opts...); err != nil { + return fmt.Errorf("failed to apply object %q: %w", key, err) + } + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, obj); err != nil { + return fmt.Errorf("failed to convert applied object %q from unstructured: %w", key, err) + } + return nil +} + func (c *ctrlClientImpl) Exists(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { if err := c.Client.Get(ctx, key, obj); err != nil { if errors.IsNotFound(err) { diff --git a/pkg/controller/common/client_test.go b/pkg/controller/common/client_test.go index d0ea892ba..de652e023 100644 --- a/pkg/controller/common/client_test.go +++ b/pkg/controller/common/client_test.go @@ -2,20 +2,27 @@ package common import ( "context" + "fmt" "net/http" "testing" "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" "k8s.io/client-go/tools/events" "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/apiutil" "sigs.k8s.io/controller-runtime/pkg/config" "sigs.k8s.io/controller-runtime/pkg/healthz" "sigs.k8s.io/controller-runtime/pkg/manager" @@ -120,8 +127,6 @@ func (*sentinelClient) GroupVersionKindFor(runtime.Object) (schema.GroupVersionK } func (*sentinelClient) IsObjectNamespaced(runtime.Object) (bool, error) { return false, nil } -// TestNewClient verifies NewClient returns a CtrlClient that wraps the same client instance -// as manager.GetClient() (cache-consistent wiring). func TestNewClient(t *testing.T) { var cl client.Client = &sentinelClient{} mgr := &fakeManager{client: cl} @@ -129,8 +134,432 @@ func TestNewClient(t *testing.T) { got, err := NewClient(mgr) require.NoError(t, err) require.NotNil(t, got) - var _ CtrlClient = got impl, ok := got.(*ctrlClientImpl) require.True(t, ok, "NewClient must return *ctrlClientImpl") assert.True(t, impl.Client == cl, "wrapped client must be the exact manager client instance") } + +type mockClient struct { + client.Client + + getFunc func(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error + createFunc func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error + updateFunc func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error + deleteFunc func(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error + listFunc func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error + patchFunc func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error + applyFunc func(ctx context.Context, obj runtime.ApplyConfiguration, opts ...client.ApplyOption) error + gvkFunc func(obj runtime.Object) (schema.GroupVersionKind, error) + + statusClient *mockStatusClient +} + +type mockStatusClient struct { + updateFunc func(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error +} + +func (m *mockStatusClient) Update(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { + if m.updateFunc != nil { + return m.updateFunc(ctx, obj, opts...) + } + return nil +} + +func (m *mockStatusClient) Create(context.Context, client.Object, client.Object, ...client.SubResourceCreateOption) error { + return nil +} + +func (m *mockStatusClient) Patch(context.Context, client.Object, client.Patch, ...client.SubResourcePatchOption) error { + return nil +} + +func (m *mockStatusClient) Apply(context.Context, runtime.ApplyConfiguration, ...client.SubResourceApplyOption) error { + return nil +} + +func (m *mockClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if m.getFunc != nil { + return m.getFunc(ctx, key, obj, opts...) + } + return nil +} + +func (m *mockClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + if m.createFunc != nil { + return m.createFunc(ctx, obj, opts...) + } + return nil +} + +func (m *mockClient) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { + if m.updateFunc != nil { + return m.updateFunc(ctx, obj, opts...) + } + return nil +} + +func (m *mockClient) Delete(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error { + if m.deleteFunc != nil { + return m.deleteFunc(ctx, obj, opts...) + } + return nil +} + +func (m *mockClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + if m.listFunc != nil { + return m.listFunc(ctx, list, opts...) + } + return nil +} + +func (m *mockClient) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if m.patchFunc != nil { + return m.patchFunc(ctx, obj, patch, opts...) + } + return nil +} + +func (m *mockClient) Apply(ctx context.Context, obj runtime.ApplyConfiguration, opts ...client.ApplyOption) error { + if m.applyFunc != nil { + return m.applyFunc(ctx, obj, opts...) + } + return nil +} + +func (m *mockClient) GroupVersionKindFor(obj runtime.Object) (schema.GroupVersionKind, error) { + if m.gvkFunc != nil { + return m.gvkFunc(obj) + } + return apiutil.GVKForObject(obj, clientgoscheme.Scheme) +} + +func (m *mockClient) Status() client.SubResourceWriter { + if m.statusClient != nil { + return m.statusClient + } + return &mockStatusClient{} +} + +func newCtrlClient(mc *mockClient) *ctrlClientImpl { + return &ctrlClientImpl{Client: mc} +} + +func TestClientExists_Found(t *testing.T) { + mc := &mockClient{ + getFunc: func(_ context.Context, _ client.ObjectKey, _ client.Object, _ ...client.GetOption) error { + return nil + }, + } + c := newCtrlClient(mc) + + found, err := c.Exists(context.Background(), types.NamespacedName{Name: "test", Namespace: "default"}, &corev1.ConfigMap{}) + require.NoError(t, err) + assert.True(t, found) +} + +func TestClientExists_NotFound(t *testing.T) { + mc := &mockClient{ + getFunc: func(_ context.Context, _ client.ObjectKey, _ client.Object, _ ...client.GetOption) error { + return apierrors.NewNotFound(schema.GroupResource{Resource: "configmaps"}, "missing") + }, + } + c := newCtrlClient(mc) + + found, err := c.Exists(context.Background(), types.NamespacedName{Name: "missing", Namespace: "default"}, &corev1.ConfigMap{}) + require.NoError(t, err) + assert.False(t, found) +} + +func TestClientExists_Error(t *testing.T) { + mc := &mockClient{ + getFunc: func(_ context.Context, _ client.ObjectKey, _ client.Object, _ ...client.GetOption) error { + return fmt.Errorf("connection refused") + }, + } + c := newCtrlClient(mc) + + found, err := c.Exists(context.Background(), types.NamespacedName{Name: "test", Namespace: "default"}, &corev1.ConfigMap{}) + require.Error(t, err) + assert.False(t, found) +} + +func TestClientGet_Success(t *testing.T) { + mc := &mockClient{ + getFunc: func(_ context.Context, key client.ObjectKey, obj client.Object, _ ...client.GetOption) error { + cm := obj.(*corev1.ConfigMap) + cm.Name = key.Name + cm.Namespace = key.Namespace + cm.Data = map[string]string{"k": "v"} + return nil + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{} + err := c.Get(context.Background(), types.NamespacedName{Name: "myconfig", Namespace: "default"}, cm) + require.NoError(t, err) + assert.Equal(t, "myconfig", cm.Name) + assert.Equal(t, "v", cm.Data["k"]) +} + +func TestClientGet_NotFound(t *testing.T) { + mc := &mockClient{ + getFunc: func(_ context.Context, _ client.ObjectKey, _ client.Object, _ ...client.GetOption) error { + return apierrors.NewNotFound(schema.GroupResource{Resource: "configmaps"}, "missing") + }, + } + c := newCtrlClient(mc) + + err := c.Get(context.Background(), types.NamespacedName{Name: "missing", Namespace: "default"}, &corev1.ConfigMap{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to get object") +} + +func TestClientCreate_Success(t *testing.T) { + var created bool + mc := &mockClient{ + createFunc: func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { + created = true + return nil + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "new", Namespace: "default"}} + err := c.Create(context.Background(), cm) + require.NoError(t, err) + assert.True(t, created) +} + +func TestClientCreate_AlreadyExists(t *testing.T) { + mc := &mockClient{ + createFunc: func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { + return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "configmaps"}, "dup") + }, + } + c := newCtrlClient(mc) + + err := c.Create(context.Background(), &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "dup", Namespace: "default"}}) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to create object") +} + +func TestClientUpdate_Success(t *testing.T) { + var updated bool + mc := &mockClient{ + updateFunc: func(_ context.Context, _ client.Object, _ ...client.UpdateOption) error { + updated = true + return nil + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "upd", Namespace: "default"}} + err := c.Update(context.Background(), cm) + require.NoError(t, err) + assert.True(t, updated) +} + +func TestClientDelete_Success(t *testing.T) { + var deleted bool + mc := &mockClient{ + deleteFunc: func(_ context.Context, _ client.Object, _ ...client.DeleteOption) error { + deleted = true + return nil + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "del", Namespace: "default"}} + err := c.Delete(context.Background(), cm) + require.NoError(t, err) + assert.True(t, deleted) +} + +func TestClientList_Success(t *testing.T) { + mc := &mockClient{ + listFunc: func(_ context.Context, list client.ObjectList, _ ...client.ListOption) error { + cmList := list.(*corev1.ConfigMapList) + cmList.Items = []corev1.ConfigMap{ + {ObjectMeta: metav1.ObjectMeta{Name: "cm1"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "cm2"}}, + } + return nil + }, + } + c := newCtrlClient(mc) + + list := &corev1.ConfigMapList{} + err := c.List(context.Background(), list) + require.NoError(t, err) + assert.Len(t, list.Items, 2) +} + +func TestClientUpdateWithRetry_Success(t *testing.T) { + updateCallCount := 0 + mc := &mockClient{ + getFunc: func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) error { + obj.SetResourceVersion("123") + return nil + }, + updateFunc: func(_ context.Context, _ client.Object, _ ...client.UpdateOption) error { + updateCallCount++ + return nil + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "retry-cm", Namespace: "default"}} + err := c.UpdateWithRetry(context.Background(), cm) + require.NoError(t, err) + assert.Equal(t, 1, updateCallCount) + assert.Equal(t, "123", cm.GetResourceVersion()) +} + +func TestClientUpdateWithRetry_ConflictThenSuccess(t *testing.T) { + callCount := 0 + mc := &mockClient{ + getFunc: func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) error { + obj.SetResourceVersion(fmt.Sprintf("rv-%d", callCount)) + return nil + }, + updateFunc: func(_ context.Context, _ client.Object, _ ...client.UpdateOption) error { + callCount++ + if callCount == 1 { + return apierrors.NewConflict(schema.GroupResource{Resource: "configmaps"}, "retry-cm", fmt.Errorf("conflict")) + } + return nil + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "retry-cm", Namespace: "default"}} + err := c.UpdateWithRetry(context.Background(), cm) + require.NoError(t, err) + assert.Equal(t, 2, callCount, "update should be called twice: conflict then success") +} + +func TestClientPatch_Success(t *testing.T) { + var patched bool + mc := &mockClient{ + patchFunc: func(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error { + patched = true + return nil + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "patch-cm", Namespace: "default"}} + err := c.Patch(context.Background(), cm, client.MergeFrom(cm)) + require.NoError(t, err) + assert.True(t, patched) +} + +func TestClientPatch_Error(t *testing.T) { + mc := &mockClient{ + patchFunc: func(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error { + return fmt.Errorf("patch failed") + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "patch-cm", Namespace: "default"}} + err := c.Patch(context.Background(), cm, client.MergeFrom(cm)) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to patch object") +} + +func TestClientApply_Success(t *testing.T) { + mc := &mockClient{ + applyFunc: func(_ context.Context, ac runtime.ApplyConfiguration, opts ...client.ApplyOption) error { + obj, ok := ac.(interface { + runtime.Object + metav1.Object + }) + require.True(t, ok, "apply configuration should wrap an unstructured object") + assert.Equal(t, schema.GroupVersionKind{Version: "v1", Kind: "ConfigMap"}, obj.GetObjectKind().GroupVersionKind()) + assert.Equal(t, "apply-cm", obj.GetName()) + + ao := (&client.ApplyOptions{}).ApplyOptions(opts) + assert.Equal(t, "test-manager", ao.FieldManager) + + // Simulate the server response, which must be copied back into the typed object. + obj.SetResourceVersion("42") + return nil + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "apply-cm", Namespace: "default"}} + err := c.Apply(context.Background(), cm, client.FieldOwner("test-manager")) + require.NoError(t, err) + assert.Equal(t, "42", cm.ResourceVersion) +} + +func TestClientApply_GVKError(t *testing.T) { + var applied bool + mc := &mockClient{ + gvkFunc: func(runtime.Object) (schema.GroupVersionKind, error) { + return schema.GroupVersionKind{}, fmt.Errorf("not registered") + }, + applyFunc: func(context.Context, runtime.ApplyConfiguration, ...client.ApplyOption) error { + applied = true + return nil + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "apply-cm", Namespace: "default"}} + err := c.Apply(context.Background(), cm) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to get GroupVersionKind") + assert.False(t, applied, "apply should not be sent without a GroupVersionKind") +} + +func TestClientApply_Error(t *testing.T) { + mc := &mockClient{ + applyFunc: func(context.Context, runtime.ApplyConfiguration, ...client.ApplyOption) error { + return fmt.Errorf("apply failed") + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "apply-cm", Namespace: "default"}} + err := c.Apply(context.Background(), cm) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to apply object") +} + +func TestClientStatusUpdate_Success(t *testing.T) { + var updated bool + mc := &mockClient{ + statusClient: &mockStatusClient{ + updateFunc: func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { + updated = true + return nil + }, + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "status-cm", Namespace: "default"}} + err := c.StatusUpdate(context.Background(), cm) + require.NoError(t, err) + assert.True(t, updated) +} + +func TestClientStatusUpdate_Error(t *testing.T) { + mc := &mockClient{ + statusClient: &mockStatusClient{ + updateFunc: func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { + return fmt.Errorf("status update failed") + }, + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "status-cm", Namespace: "default"}} + err := c.StatusUpdate(context.Background(), cm) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to update status") +} diff --git a/pkg/controller/common/fakes/fake_ctrl_client.go b/pkg/controller/common/fakes/fake_ctrl_client.go index 86273f28f..cd76656ff 100644 --- a/pkg/controller/common/fakes/fake_ctrl_client.go +++ b/pkg/controller/common/fakes/fake_ctrl_client.go @@ -10,6 +10,19 @@ import ( ) type FakeCtrlClient struct { + ApplyStub func(context.Context, client.Object, ...client.ApplyOption) error + applyMutex sync.RWMutex + applyArgsForCall []struct { + arg1 context.Context + arg2 client.Object + arg3 []client.ApplyOption + } + applyReturns struct { + result1 error + } + applyReturnsOnCall map[int]struct { + result1 error + } CreateStub func(context.Context, client.Object, ...client.CreateOption) error createMutex sync.RWMutex createArgsForCall []struct { @@ -134,6 +147,69 @@ type FakeCtrlClient struct { invocationsMutex sync.RWMutex } +func (fake *FakeCtrlClient) Apply(arg1 context.Context, arg2 client.Object, arg3 ...client.ApplyOption) error { + fake.applyMutex.Lock() + ret, specificReturn := fake.applyReturnsOnCall[len(fake.applyArgsForCall)] + fake.applyArgsForCall = append(fake.applyArgsForCall, struct { + arg1 context.Context + arg2 client.Object + arg3 []client.ApplyOption + }{arg1, arg2, arg3}) + stub := fake.ApplyStub + fakeReturns := fake.applyReturns + fake.recordInvocation("Apply", []interface{}{arg1, arg2, arg3}) + fake.applyMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3...) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeCtrlClient) ApplyCallCount() int { + fake.applyMutex.RLock() + defer fake.applyMutex.RUnlock() + return len(fake.applyArgsForCall) +} + +func (fake *FakeCtrlClient) ApplyCalls(stub func(context.Context, client.Object, ...client.ApplyOption) error) { + fake.applyMutex.Lock() + defer fake.applyMutex.Unlock() + fake.ApplyStub = stub +} + +func (fake *FakeCtrlClient) ApplyArgsForCall(i int) (context.Context, client.Object, []client.ApplyOption) { + fake.applyMutex.RLock() + defer fake.applyMutex.RUnlock() + argsForCall := fake.applyArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 +} + +func (fake *FakeCtrlClient) ApplyReturns(result1 error) { + fake.applyMutex.Lock() + defer fake.applyMutex.Unlock() + fake.ApplyStub = nil + fake.applyReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeCtrlClient) ApplyReturnsOnCall(i int, result1 error) { + fake.applyMutex.Lock() + defer fake.applyMutex.Unlock() + fake.ApplyStub = nil + if fake.applyReturnsOnCall == nil { + fake.applyReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.applyReturnsOnCall[i] = struct { + result1 error + }{result1} +} + func (fake *FakeCtrlClient) Create(arg1 context.Context, arg2 client.Object, arg3 ...client.CreateOption) error { fake.createMutex.Lock() ret, specificReturn := fake.createReturnsOnCall[len(fake.createArgsForCall)] diff --git a/pkg/controller/istiocsr/certificates.go b/pkg/controller/istiocsr/certificates.go index 23b640055..4d3bf2a5f 100644 --- a/pkg/controller/istiocsr/certificates.go +++ b/pkg/controller/istiocsr/certificates.go @@ -4,9 +4,7 @@ import ( "fmt" "maps" - corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" certmanagermetav1 "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" @@ -16,43 +14,15 @@ import ( "github.com/openshift/cert-manager-operator/pkg/operator/assets" ) -func (r *Reconciler) createOrApplyCertificates(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string, istioCSRCreateRecon bool) error { +func (r *Reconciler) createOrApplyCertificates(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) error { desired, err := r.getCertificateObject(istiocsr, resourceLabels) if err != nil { return fmt.Errorf("failed to generate certificate resource for creation in %s: %w", istiocsr.GetNamespace(), err) } - certificateName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling certificate resource", "name", certificateName) - fetched := &certmanagerv1.Certificate{} - exist, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), fetched) - if err != nil { - return common.FromClientError(err, "failed to check %s certificate resource already exists", certificateName) - } - - if exist { - if istioCSRCreateRecon { - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s certificate resource already exists, maybe from previous installation", certificateName) - } - if hasObjectChanged(desired, fetched) { - r.log.V(1).Info("certificate has been modified, updating to desired state", "name", certificateName) - if err := r.UpdateWithRetry(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to update %s certificate resource", certificateName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "certificate resource %s reconciled back to desired state", certificateName) - } else { - r.log.V(4).Info("certificate resource already exists and is in expected state", "name", certificateName) - } - } - - if !exist { - if err := r.Create(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to create %s certificate resource", certificateName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "certificate resource %s created", certificateName) - } - - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, istiocsr, desired, &certmanagerv1.Certificate{}, fieldOwner, + func(d, e *certmanagerv1.Certificate) bool { return hasObjectChanged(d, e) }, + ) } func (r *Reconciler) getCertificateObject(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) (*certmanagerv1.Certificate, error) { diff --git a/pkg/controller/istiocsr/certificates_test.go b/pkg/controller/istiocsr/certificates_test.go index e9bb417eb..5361e2382 100644 --- a/pkg/controller/istiocsr/certificates_test.go +++ b/pkg/controller/istiocsr/certificates_test.go @@ -39,7 +39,7 @@ func TestCreateOrApplyCertificates(t *testing.T) { return true, nil }) }, - wantErr: `failed to check istio-test-ns/istiod certificate resource already exists: test client error`, + wantErr: `failed to check if Certificate "istio-test-ns/istiod" exists: test client error`, }, { name: "reconciliation of certificate fails while restoring to expected state", @@ -61,15 +61,11 @@ func TestCreateOrApplyCertificates(t *testing.T) { } return true, nil }) - m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { - switch obj.(type) { - case *certmanagerv1.Certificate: - return errTestClient - } - return nil - }) + m.ApplyReturns(errTestClient) + // The merge-patch fallback for the drifted object fails too. + m.PatchReturns(errTestClient) }, - wantErr: `failed to update istio-test-ns/istiod certificate resource: test client error`, + wantErr: `failed to apply Certificate "istio-test-ns/istiod": test client error`, }, { name: "reconciliation of certificate which already exists in expected state", @@ -110,15 +106,9 @@ func TestCreateOrApplyCertificates(t *testing.T) { } return true, nil }) - m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { - switch obj.(type) { - case *certmanagerv1.Certificate: - return errTestClient - } - return nil - }) + m.ApplyReturns(errTestClient) }, - wantErr: `failed to create istio-test-ns/istiod certificate resource: test client error`, + wantErr: `failed to apply Certificate "istio-test-ns/istiod": test client error`, }, { name: "reconciliation of certificate when revisions are configured", @@ -223,7 +213,7 @@ func TestCreateOrApplyCertificates(t *testing.T) { }, istiocsr); err != nil { t.Errorf("test error: %v", err) } - err := r.createOrApplyCertificates(istiocsr, controllerDefaultResourceLabels, false) + err := r.createOrApplyCertificates(istiocsr, controllerDefaultResourceLabels) if (tt.wantErr != "" || err != nil) && (err == nil || err.Error() != tt.wantErr) { t.Errorf("createOrApplyCertificates() err: %v, wantErr: %v", err, tt.wantErr) } diff --git a/pkg/controller/istiocsr/constants.go b/pkg/controller/istiocsr/constants.go index bfdc78ef8..8cf4ab3a4 100644 --- a/pkg/controller/istiocsr/constants.go +++ b/pkg/controller/istiocsr/constants.go @@ -30,6 +30,9 @@ const ( // defaultRequeueTime is the default reconcile requeue time. defaultRequeueTime = time.Second * 30 + // fieldOwner identifies this controller when using Server-Side Apply. + fieldOwner = "istio-csr-controller" + // istiocsrObjectName is the name of the istio-csr resource created by user. // istio-csr CRD enforces name to be `default`. istiocsrObjectName = "default" diff --git a/pkg/controller/istiocsr/install_instiocsr_test.go b/pkg/controller/istiocsr/install_instiocsr_test.go index d128b1f00..a7bc76c41 100644 --- a/pkg/controller/istiocsr/install_instiocsr_test.go +++ b/pkg/controller/istiocsr/install_instiocsr_test.go @@ -48,13 +48,31 @@ func TestReconcileIstioCSRDeployment(t *testing.T) { } return nil }) + // SSA-migrated resources (Service, ServiceAccount, Certificate, Role, RoleBinding) use Apply + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { + switch o := obj.(type) { + case *corev1.Service, *corev1.ServiceAccount: + if !reflect.DeepEqual(o.GetLabels(), labels) { + return fmt.Errorf("labels mismatch in %v resource; got: %v, want: %v", o, o.GetLabels(), labels) + } + case *certmanagerv1.Certificate, *rbacv1.Role, *rbacv1.RoleBinding: + l := make(map[string]string) + maps.Copy(l, labels) + l[istiocsrNamespaceMappingLabelName] = testIstioCSRNamespace + if !reflect.DeepEqual(o.GetLabels(), l) { + return fmt.Errorf("labels mismatch in %v resource; got: %v, want: %v", o, o.GetLabels(), l) + } + } + return nil + }) + // Non-SSA resources (Deployment, ClusterRole, ClusterRoleBinding) still use Create m.CreateCalls(func(ctx context.Context, obj client.Object, option ...client.CreateOption) error { switch o := obj.(type) { - case *appsv1.Deployment, *corev1.Service, *corev1.ServiceAccount: + 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) } - case *certmanagerv1.Certificate, *rbacv1.Role, *rbacv1.RoleBinding, *rbacv1.ClusterRole, *rbacv1.ClusterRoleBinding: + case *rbacv1.ClusterRole, *rbacv1.ClusterRoleBinding: l := make(map[string]string) maps.Copy(l, labels) l[istiocsrNamespaceMappingLabelName] = testIstioCSRNamespace @@ -67,9 +85,9 @@ func TestReconcileIstioCSRDeployment(t *testing.T) { }, }, { - name: "istiocsr reconciliation fails with serviceaccount creation error", + name: "istiocsr reconciliation fails with serviceaccount apply error", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { - m.CreateCalls(func(ctx context.Context, obj client.Object, option ...client.CreateOption) error { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { switch obj.(type) { case *corev1.ServiceAccount: return errTestClient @@ -77,15 +95,20 @@ func TestReconcileIstioCSRDeployment(t *testing.T) { return nil }) }, - wantErr: `failed to create istiocsr-test-ns/cert-manager-istio-csr serviceaccount resource: test client error`, + wantErr: `failed to apply ServiceAccount "istiocsr-test-ns/cert-manager-istio-csr": test client error`, }, { - name: "istiocsr reconciliation fails with role creation error", + name: "istiocsr reconciliation fails with role apply error", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { - m.CreateCalls(func(ctx context.Context, obj client.Object, option ...client.CreateOption) error { - switch o := obj.(type) { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { + switch obj.(type) { case *rbacv1.Role: return errTestClient + } + return nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, option ...client.CreateOption) error { + switch o := obj.(type) { case *rbacv1.ClusterRoleBinding: roleBinding := testClusterRoleBinding() roleBinding.DeepCopyInto(o) @@ -93,15 +116,20 @@ func TestReconcileIstioCSRDeployment(t *testing.T) { return nil }) }, - wantErr: `failed to create istio-test-ns/cert-manager-istio-csr role resource: test client error`, + wantErr: `failed to apply Role "istio-test-ns/cert-manager-istio-csr": test client error`, }, { - name: "istiocsr reconciliation fails with certificate creation error", + name: "istiocsr reconciliation fails with certificate apply error", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { - m.CreateCalls(func(ctx context.Context, obj client.Object, option ...client.CreateOption) error { - switch o := obj.(type) { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { + switch obj.(type) { case *certmanagerv1.Certificate: return errTestClient + } + return nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, option ...client.CreateOption) error { + switch o := obj.(type) { case *rbacv1.ClusterRoleBinding: roleBinding := testClusterRoleBinding() roleBinding.DeepCopyInto(o) @@ -109,7 +137,7 @@ func TestReconcileIstioCSRDeployment(t *testing.T) { return nil }) }, - wantErr: `failed to create istio-test-ns/istiod certificate resource: test client error`, + wantErr: `failed to apply Certificate "istio-test-ns/istiod": test client error`, }, } diff --git a/pkg/controller/istiocsr/install_istiocsr.go b/pkg/controller/istiocsr/install_istiocsr.go index 05f1d2457..a9dc30620 100644 --- a/pkg/controller/istiocsr/install_istiocsr.go +++ b/pkg/controller/istiocsr/install_istiocsr.go @@ -13,25 +13,23 @@ func (r *Reconciler) reconcileIstioCSRDeployment(istiocsr *v1alpha1.IstioCSR, is return common.NewIrrecoverableError(err, "%s/%s configuration validation failed", istiocsr.GetNamespace(), istiocsr.GetName()) } - // if user has set custom labels to be added to all resources created by the controller - // merge it with the controller's own default labels. resourceLabels := make(map[string]string) if istiocsr.Spec.ControllerConfig != nil && len(istiocsr.Spec.ControllerConfig.Labels) != 0 { maps.Copy(resourceLabels, istiocsr.Spec.ControllerConfig.Labels) } maps.Copy(resourceLabels, controllerDefaultResourceLabels) - if err := r.createOrApplyNetworkPolicies(istiocsr, resourceLabels, istioCSRCreateRecon); err != nil { + if err := r.createOrApplyNetworkPolicies(istiocsr, resourceLabels); err != nil { r.log.Error(err, "failed to reconcile network policy resources") return err } - if err := r.createOrApplyServices(istiocsr, resourceLabels, istioCSRCreateRecon); err != nil { + if err := r.createOrApplyServices(istiocsr, resourceLabels); err != nil { r.log.Error(err, "failed to reconcile service resource") return err } - if err := r.createOrApplyServiceAccounts(istiocsr, resourceLabels, istioCSRCreateRecon); err != nil { + if err := r.createOrApplyServiceAccounts(istiocsr, resourceLabels); err != nil { r.log.Error(err, "failed to reconcile serviceaccount resource") return err } @@ -41,7 +39,7 @@ func (r *Reconciler) reconcileIstioCSRDeployment(istiocsr *v1alpha1.IstioCSR, is return err } - if err := r.createOrApplyCertificates(istiocsr, resourceLabels, istioCSRCreateRecon); err != nil { + if err := r.createOrApplyCertificates(istiocsr, resourceLabels); err != nil { r.log.Error(err, "failed to reconcile certificate resource") return err } diff --git a/pkg/controller/istiocsr/networkpolicies.go b/pkg/controller/istiocsr/networkpolicies.go index 03bbca008..8fb361ade 100644 --- a/pkg/controller/istiocsr/networkpolicies.go +++ b/pkg/controller/istiocsr/networkpolicies.go @@ -4,27 +4,26 @@ import ( "fmt" "maps" - corev1 "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" "github.com/openshift/cert-manager-operator/pkg/controller/common" "github.com/openshift/cert-manager-operator/pkg/operator/assets" ) -func (r *Reconciler) createOrApplyNetworkPolicies(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string, istioCSRCreateRecon bool) error { +func (r *Reconciler) createOrApplyNetworkPolicies(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) error { r.log.V(4).Info("reconciling istio-csr network policies", "namespace", istiocsr.GetNamespace(), "name", istiocsr.GetName()) - // Apply static network policy assets for istio-csr for _, assetPath := range istioCSRNetworkPolicyAssets { obj, err := r.getNetworkPolicyFromAsset(assetPath, istiocsr, resourceLabels) if err != nil { return fmt.Errorf("failed to get network policy from asset %s: %w", assetPath, err) } - if err := r.createOrUpdateNetworkPolicy(obj, istioCSRCreateRecon); err != nil { - return fmt.Errorf("failed to create/update network policy from %s: %w", assetPath, err) + if err := common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, istiocsr, obj, &networkingv1.NetworkPolicy{}, fieldOwner, + func(d, e *networkingv1.NetworkPolicy) bool { return hasObjectChanged(d, e) }, + ); err != nil { + return fmt.Errorf("failed to apply network policy from %s: %w", assetPath, err) } } @@ -32,13 +31,11 @@ func (r *Reconciler) createOrApplyNetworkPolicies(istiocsr *v1alpha1.IstioCSR, r } func (r *Reconciler) getNetworkPolicyFromAsset(assetPath string, istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) (*networkingv1.NetworkPolicy, error) { - // Get the target namespace for istio-csr deployment namespace := istiocsr.GetNamespace() if namespace == "" { namespace = istiocsr.Spec.IstioCSRConfig.Istio.Namespace } - // Read the asset and decode it assetBytes := assets.MustAsset(assetPath) obj, err := runtime.Decode(codecs.UniversalDeserializer(), assetBytes) if err != nil { @@ -50,10 +47,8 @@ func (r *Reconciler) getNetworkPolicyFromAsset(assetPath string, istiocsr *v1alp return nil, fmt.Errorf("decoded object is not a NetworkPolicy, got %T", obj) } - // Set the correct namespace policy.Namespace = namespace - // Merge resource labels if policy.Labels == nil { policy.Labels = make(map[string]string) } @@ -61,43 +56,3 @@ func (r *Reconciler) getNetworkPolicyFromAsset(assetPath string, istiocsr *v1alp return policy, nil } - -func (r *Reconciler) createOrUpdateNetworkPolicy(policy *networkingv1.NetworkPolicy, istioCSRCreateRecon bool) error { - desired := policy.DeepCopy() - policyName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling network policy resource", "name", policyName) - - fetched := &networkingv1.NetworkPolicy{} - key := types.NamespacedName{ - Name: desired.GetName(), - Namespace: desired.GetNamespace(), - } - exist, err := r.Exists(r.ctx, key, fetched) - if err != nil { - return common.FromClientError(err, "failed to check %s network policy resource already exists", policyName) - } - - if exist { - if istioCSRCreateRecon { - r.eventRecorder.Eventf(policy, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s network policy resource already exists, maybe from previous installation", policyName) - } - if hasObjectChanged(desired, fetched) { - r.log.V(1).Info("network policy has been modified, updating to desired state", "name", policyName) - if err := r.UpdateWithRetry(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to update %s network policy resource", policyName) - } - r.eventRecorder.Eventf(policy, corev1.EventTypeNormal, "Reconciled", "network policy resource %s reconciled back to desired state", policyName) - } else { - r.log.V(4).Info("network policy resource already exists and is in expected state", "name", policyName) - } - } - - if !exist { - if err := r.Create(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to create %s network policy resource", policyName) - } - r.eventRecorder.Eventf(policy, corev1.EventTypeNormal, "Reconciled", "network policy resource %s created", policyName) - } - - return nil -} diff --git a/pkg/controller/istiocsr/networkpolicies_test.go b/pkg/controller/istiocsr/networkpolicies_test.go new file mode 100644 index 000000000..252f23af9 --- /dev/null +++ b/pkg/controller/istiocsr/networkpolicies_test.go @@ -0,0 +1,72 @@ +package istiocsr + +import ( + "testing" + + networkingv1 "k8s.io/api/networking/v1" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/openshift/cert-manager-operator/pkg/controller/common/fakes" +) + +func TestGetNetworkPolicyFromAsset(t *testing.T) { + r := testReconciler(t) + istiocsr := testIstioCSR() + labels := map[string]string{"app": "istio-csr", "custom": "label"} + + for _, assetPath := range istioCSRNetworkPolicyAssets { + t.Run(assetPath, func(t *testing.T) { + policy, err := r.getNetworkPolicyFromAsset(assetPath, istiocsr, labels) + require.NoError(t, err) + require.IsType(t, &networkingv1.NetworkPolicy{}, policy) + + assert.Equal(t, testIstioCSRNamespace, policy.Namespace, + "policy namespace should match istiocsr namespace") + assert.Equal(t, "istio-csr", policy.Labels["app"]) + assert.Equal(t, "label", policy.Labels["custom"]) + }) + } +} + +func TestGetNetworkPolicyFromAsset_FallbackNamespace(t *testing.T) { + r := testReconciler(t) + istiocsr := testIstioCSR() + istiocsr.Namespace = "" + + policy, err := r.getNetworkPolicyFromAsset(istioCSRNetworkPolicyAssets[0], istiocsr, nil) + require.NoError(t, err) + assert.Equal(t, testIstiodNamespace, policy.Namespace, + "should fall back to Istio namespace when istiocsr namespace is empty") +} + +func TestCreateOrApplyNetworkPolicies(t *testing.T) { + fakeClient := &fakes.FakeCtrlClient{} + fakeClient.ExistsReturns(false, nil) + fakeClient.ApplyReturns(nil) + + r := testReconciler(t) + r.CtrlClient = fakeClient + istiocsr := testIstioCSR() + labels := map[string]string{"app": "istio-csr"} + + err := r.createOrApplyNetworkPolicies(istiocsr, labels) + require.NoError(t, err) + assert.Equal(t, len(istioCSRNetworkPolicyAssets), fakeClient.ApplyCallCount(), + "should apply one patch per network policy asset") +} + +func TestCreateOrApplyNetworkPolicies_ApplyError(t *testing.T) { + fakeClient := &fakes.FakeCtrlClient{} + fakeClient.ExistsReturns(false, nil) + fakeClient.ApplyReturns(errTestClient) + + r := testReconciler(t) + r.CtrlClient = fakeClient + istiocsr := testIstioCSR() + + err := r.createOrApplyNetworkPolicies(istiocsr, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to apply network policy") +} diff --git a/pkg/controller/istiocsr/rbacs.go b/pkg/controller/istiocsr/rbacs.go index 79b69f28d..183469fa7 100644 --- a/pkg/controller/istiocsr/rbacs.go +++ b/pkg/controller/istiocsr/rbacs.go @@ -31,22 +31,22 @@ func (r *Reconciler) createOrApplyRBACResource(istiocsr *v1alpha1.IstioCSR, reso return err } - if err := r.createOrApplyRoles(istiocsr, resourceLabels, istioCSRCreateRecon); err != nil { + if err := r.createOrApplyRoles(istiocsr, resourceLabels); err != nil { r.log.Error(err, "failed to reconcile role resource") return err } - if err := r.createOrApplyRoleBindings(istiocsr, serviceAccount, resourceLabels, istioCSRCreateRecon); err != nil { + if err := r.createOrApplyRoleBindings(istiocsr, serviceAccount, resourceLabels); err != nil { r.log.Error(err, "failed to reconcile rolebinding resource") return err } - if err := r.createOrApplyRoleForLeases(istiocsr, resourceLabels, istioCSRCreateRecon); err != nil { + if err := r.createOrApplyRoleForLeases(istiocsr, resourceLabels); err != nil { r.log.Error(err, "failed to reconcile role for leases resource") return err } - if err := r.createOrApplyRoleBindingForLeases(istiocsr, serviceAccount, resourceLabels, istioCSRCreateRecon); err != nil { + if err := r.createOrApplyRoleBindingForLeases(istiocsr, serviceAccount, resourceLabels); err != nil { r.log.Error(err, "failed to reconcile rolebinding for leases resource") return err } @@ -268,40 +268,11 @@ func (r *Reconciler) updateClusterRoleBindingNameInStatus(istiocsr *v1alpha1.Ist return r.updateStatus(r.ctx, istiocsr) } -func (r *Reconciler) createOrApplyRoles(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string, istioCSRCreateRecon bool) error { +func (r *Reconciler) createOrApplyRoles(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) error { desired := r.getRoleObject(istiocsr.GetNamespace(), istiocsr.Spec.IstioCSRConfig.Istio.Namespace, resourceLabels) - - roleName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling role resource", "name", roleName) - fetched := &rbacv1.Role{} - exist, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), fetched) - if err != nil { - return common.FromClientError(err, "failed to check %s role resource already exists", roleName) - } - - if exist { - if istioCSRCreateRecon { - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s role resource already exists, maybe from previous installation", roleName) - } - if hasObjectChanged(desired, fetched) { - r.log.V(1).Info("role has been modified, updating to desired state", "name", roleName) - if err := r.UpdateWithRetry(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to update %s role resource", roleName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "role resource %s reconciled back to desired state", roleName) - } else { - r.log.V(4).Info("role resource already exists and is in expected state", "name", roleName) - } - } - - if !exist { - if err := r.Create(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to create %s role resource", roleName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "role resource %s created", roleName) - } - - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, istiocsr, desired, &rbacv1.Role{}, fieldOwner, + func(d, e *rbacv1.Role) bool { return hasObjectChanged(d, e) }, + ) } func (r *Reconciler) getRoleObject(istiocsrNamespace, roleNamespace string, resourceLabels map[string]string) *rbacv1.Role { @@ -311,40 +282,30 @@ func (r *Reconciler) getRoleObject(istiocsrNamespace, roleNamespace string, reso return role } -func (r *Reconciler) createOrApplyRoleBindings(istiocsr *v1alpha1.IstioCSR, serviceAccount string, resourceLabels map[string]string, istioCSRCreateRecon bool) error { +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) + return r.applyRoleBinding(istiocsr, desired) +} - 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) +// applyRoleBinding server-side applies desired. RoleBinding.roleRef is +// immutable, so an existing binding whose roleRef has changed is deleted first. +func (r *Reconciler) applyRoleBinding(istiocsr *v1alpha1.IstioCSR, 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 %s rolebinding resource already exists", roleBindingName) + return common.FromClientError(err, "failed to check if RoleBinding %q exists", key) } - - 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 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) } } - 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) }, + ) } func (r *Reconciler) getRoleBindingObject(serviceAccount, istiocsrNamespace, roleNamespace string, resourceLabels map[string]string) *rbacv1.RoleBinding { @@ -355,40 +316,11 @@ func (r *Reconciler) getRoleBindingObject(serviceAccount, istiocsrNamespace, rol return roleBinding } -func (r *Reconciler) createOrApplyRoleForLeases(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string, istioCSRCreateRecon bool) error { +func (r *Reconciler) createOrApplyRoleForLeases(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) error { desired := r.getRoleForLeasesObject(istiocsr.GetNamespace(), istiocsr.Spec.IstioCSRConfig.Istio.Namespace, resourceLabels) - - roleName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling role for lease resource", "name", roleName) - fetched := &rbacv1.Role{} - exist, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), fetched) - if err != nil { - return common.FromClientError(err, "failed to check %s role resource already exists", roleName) - } - - if exist { - if istioCSRCreateRecon { - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s role resource already exists, maybe from previous installation", roleName) - } - if hasObjectChanged(desired, fetched) { - r.log.V(1).Info("role has been modified, updating to desired state", "name", roleName) - if err := r.UpdateWithRetry(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to update %s role resource", roleName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "role resource %s reconciled back to desired state", roleName) - } else { - r.log.V(4).Info("role resource already exists and is in expected state", "name", roleName) - } - } - - if !exist { - if err := r.Create(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to create %s role resource", roleName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "role resource %s created", roleName) - } - - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, istiocsr, desired, &rbacv1.Role{}, fieldOwner, + func(d, e *rbacv1.Role) bool { return hasObjectChanged(d, e) }, + ) } func (r *Reconciler) getRoleForLeasesObject(istiocsrNamespace, roleNamespace string, resourceLabels map[string]string) *rbacv1.Role { @@ -398,40 +330,9 @@ func (r *Reconciler) getRoleForLeasesObject(istiocsrNamespace, roleNamespace str return role } -func (r *Reconciler) createOrApplyRoleBindingForLeases(istiocsr *v1alpha1.IstioCSR, serviceAccount string, resourceLabels map[string]string, istioCSRCreateRecon bool) error { +func (r *Reconciler) createOrApplyRoleBindingForLeases(istiocsr *v1alpha1.IstioCSR, serviceAccount string, resourceLabels map[string]string) error { desired := r.getRoleBindingForLeasesObject(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 for lease 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 r.applyRoleBinding(istiocsr, desired) } func (r *Reconciler) getRoleBindingForLeasesObject(serviceAccount, istiocsrNamespace, roleNamespace string, resourceLabels map[string]string) *rbacv1.RoleBinding { diff --git a/pkg/controller/istiocsr/rbacs_test.go b/pkg/controller/istiocsr/rbacs_test.go index 8b9793bcb..91ffee285 100644 --- a/pkg/controller/istiocsr/rbacs_test.go +++ b/pkg/controller/istiocsr/rbacs_test.go @@ -67,7 +67,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return true, nil }) }, - wantErr: `failed to check istio-test-ns/cert-manager-istio-csr role resource already exists: test client error`, + wantErr: `failed to check if Role "istio-test-ns/cert-manager-istio-csr" exists: test client error`, }, { name: "rolebindings reconciliation fails while checking if exists", @@ -80,7 +80,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return true, nil }) }, - wantErr: `failed to check istio-test-ns/cert-manager-istio-csr rolebinding resource already exists: test client error`, + wantErr: `failed to check if RoleBinding "istio-test-ns/cert-manager-istio-csr" exists: test client error`, }, { name: "role-leases reconciliation fails while checking if exists", @@ -95,7 +95,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return true, nil }) }, - wantErr: `failed to check istio-test-ns/cert-manager-istio-csr-leases role resource already exists: test client error`, + wantErr: `failed to check if Role "istio-test-ns/cert-manager-istio-csr-leases" exists: test client error`, }, { name: "rolebindings-leases reconciliation fails while checking if exists", @@ -110,7 +110,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return true, nil }) }, - wantErr: `failed to check istio-test-ns/cert-manager-istio-csr-leases rolebinding resource already exists: test client error`, + wantErr: `failed to check if RoleBinding "istio-test-ns/cert-manager-istio-csr-leases" exists: test client error`, }, { name: "clusterrolebindings reconciliation fails while listing existing resources", @@ -453,7 +453,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { wantErr: `failed to create clusterrole resource: test client error`, }, { - name: "role reconciliation updating to desired state fails", + name: "role reconciliation applying to desired state fails", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, object client.Object) (bool, error) { switch o := object.(type) { @@ -464,15 +464,17 @@ func TestCreateOrApplyRBACResource(t *testing.T) { } return true, nil }) - m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { switch obj.(type) { case *rbacv1.Role: return errTestClient } return nil }) + // The merge-patch fallback for the drifted object fails too. + m.PatchReturns(errTestClient) }, - wantErr: `failed to update istio-test-ns/cert-manager-istio-csr role resource: test client error`, + wantErr: `failed to apply Role "istio-test-ns/cert-manager-istio-csr": test client error`, }, { name: "role reconciliation creation fails", @@ -484,7 +486,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { } return true, nil }) - m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { switch obj.(type) { case *rbacv1.Role: return errTestClient @@ -492,10 +494,10 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return nil }) }, - wantErr: `failed to create istio-test-ns/cert-manager-istio-csr role resource: test client error`, + wantErr: `failed to apply Role "istio-test-ns/cert-manager-istio-csr": test client error`, }, { - name: "role-leases reconciliation updating to desired state fails", + name: "role-leases reconciliation applying to desired state fails", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, object client.Object) (bool, error) { switch o := object.(type) { @@ -508,7 +510,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { } return true, nil }) - m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { switch obj.(type) { case *rbacv1.Role: if strings.HasSuffix(obj.GetName(), "-leases") { @@ -517,8 +519,10 @@ func TestCreateOrApplyRBACResource(t *testing.T) { } return nil }) + // The merge-patch fallback for the drifted object fails too. + m.PatchReturns(errTestClient) }, - wantErr: `failed to update istio-test-ns/cert-manager-istio-csr-leases role resource: test client error`, + wantErr: `failed to apply Role "istio-test-ns/cert-manager-istio-csr-leases": test client error`, }, { name: "role-leases reconciliation creation fails", @@ -532,7 +536,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { } return true, nil }) - m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { switch obj.(type) { case *rbacv1.Role: if strings.HasSuffix(obj.GetName(), "-leases") { @@ -542,10 +546,10 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return nil }) }, - wantErr: `failed to create istio-test-ns/cert-manager-istio-csr-leases role resource: test client error`, + wantErr: `failed to apply Role "istio-test-ns/cert-manager-istio-csr-leases": test client error`, }, { - name: "rolebindings reconciliation updating to desired state fails", + name: "rolebindings reconciliation applying to desired state fails", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, object client.Object) (bool, error) { switch o := object.(type) { @@ -556,15 +560,17 @@ func TestCreateOrApplyRBACResource(t *testing.T) { } return true, nil }) - m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { switch obj.(type) { case *rbacv1.RoleBinding: return errTestClient } return nil }) + // The merge-patch fallback for the drifted object fails too. + m.PatchReturns(errTestClient) }, - wantErr: `failed to update istio-test-ns/cert-manager-istio-csr rolebinding resource: test client error`, + wantErr: `failed to apply RoleBinding "istio-test-ns/cert-manager-istio-csr": test client error`, }, { name: "rolebindings reconciliation creation fails", @@ -576,7 +582,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { } return true, nil }) - m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { switch obj.(type) { case *rbacv1.RoleBinding: return errTestClient @@ -584,10 +590,10 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return nil }) }, - wantErr: `failed to create istio-test-ns/cert-manager-istio-csr rolebinding resource: test client error`, + wantErr: `failed to apply RoleBinding "istio-test-ns/cert-manager-istio-csr": test client error`, }, { - name: "rolebinding-leases reconciliation updating to desired state fails", + name: "rolebinding-leases reconciliation applying to desired state fails", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, object client.Object) (bool, error) { switch o := object.(type) { @@ -600,7 +606,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { } return true, nil }) - m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { switch obj.(type) { case *rbacv1.RoleBinding: if strings.HasSuffix(obj.GetName(), "-leases") { @@ -609,8 +615,10 @@ func TestCreateOrApplyRBACResource(t *testing.T) { } return nil }) + // The merge-patch fallback for the drifted object fails too. + m.PatchReturns(errTestClient) }, - wantErr: `failed to update istio-test-ns/cert-manager-istio-csr-leases rolebinding resource: test client error`, + wantErr: `failed to apply RoleBinding "istio-test-ns/cert-manager-istio-csr-leases": test client error`, }, { name: "rolebinding-leases reconciliation creation fails", @@ -624,7 +632,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { } return true, nil }) - m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { switch obj.(type) { case *rbacv1.RoleBinding: if strings.HasSuffix(obj.GetName(), "-leases") { @@ -634,7 +642,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return nil }) }, - wantErr: `failed to create istio-test-ns/cert-manager-istio-csr-leases rolebinding resource: test client error`, + wantErr: `failed to apply RoleBinding "istio-test-ns/cert-manager-istio-csr-leases": test client error`, }, { name: "clusterrole reconciliation recreates with stable name from status when deleted", diff --git a/pkg/controller/istiocsr/serviceaccounts.go b/pkg/controller/istiocsr/serviceaccounts.go index 84fbce986..f221540b4 100644 --- a/pkg/controller/istiocsr/serviceaccounts.go +++ b/pkg/controller/istiocsr/serviceaccounts.go @@ -1,51 +1,24 @@ package istiocsr import ( - "fmt" - corev1 "k8s.io/api/core/v1" - "sigs.k8s.io/controller-runtime/pkg/client" "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" "github.com/openshift/cert-manager-operator/pkg/controller/common" "github.com/openshift/cert-manager-operator/pkg/operator/assets" ) -func (r *Reconciler) createOrApplyServiceAccounts(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string, istioCSRCreateRecon bool) error { +func (r *Reconciler) createOrApplyServiceAccounts(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) error { desired := r.getServiceAccountObject(istiocsr, resourceLabels) - serviceAccountName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling serviceaccount resource", "name", serviceAccountName) - fetched := &corev1.ServiceAccount{} - exist, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), fetched) - if err != nil { - return common.FromClientError(err, "failed to check %s serviceaccount resource already exists", serviceAccountName) - } - - if exist { - if istioCSRCreateRecon { - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s serviceaccount resource already exists, maybe from previous installation", serviceAccountName) - } - if hasObjectChanged(desired, fetched) { - r.log.V(1).Info("serviceaccount has been modified, updating to desired state", "name", serviceAccountName) - if err := r.UpdateWithRetry(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to update %s serviceaccount resource", serviceAccountName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "serviceaccount resource %s reconciled back to desired state", serviceAccountName) - } else { - r.log.V(4).Info("serviceaccount resource already exists and is in expected state", "name", serviceAccountName) - } - } - - if !exist { - if err := r.Create(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to create %s serviceaccount resource", serviceAccountName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "serviceaccount resource %s created", serviceAccountName) + if err := common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, istiocsr, desired, &corev1.ServiceAccount{}, fieldOwner, + func(d, e *corev1.ServiceAccount) bool { return hasObjectChanged(d, e) }, + ); err != nil { + return err } if err := r.updateServiceAccountNameInStatus(istiocsr, desired); err != nil { - return common.FromClientError(err, "failed to update %s/%s istiocsr status with %s serviceaccount resource name", istiocsr.GetNamespace(), istiocsr.GetName(), serviceAccountName) + return common.FromClientError(err, "failed to update %s/%s istiocsr status with %s serviceaccount resource name", istiocsr.GetNamespace(), istiocsr.GetName(), desired.GetName()) } return nil } diff --git a/pkg/controller/istiocsr/serviceaccounts_test.go b/pkg/controller/istiocsr/serviceaccounts_test.go index 82e0f9c4a..6927c8d5d 100644 --- a/pkg/controller/istiocsr/serviceaccounts_test.go +++ b/pkg/controller/istiocsr/serviceaccounts_test.go @@ -2,13 +2,10 @@ package istiocsr import ( "context" - "strings" "testing" - "time" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" @@ -22,12 +19,10 @@ const ( func TestCreateOrApplyServiceAccounts(t *testing.T) { tests := []struct { - name string - preReq func(*Reconciler, *fakes.FakeCtrlClient) - istioCSRCreateRecon bool - wantErr string - assertEvents func(t *testing.T, r *Reconciler) - assertCalls func(t *testing.T, mock *fakes.FakeCtrlClient) + name string + preReq func(*Reconciler, *fakes.FakeCtrlClient) + wantErr string + assertCalls func(t *testing.T, mock *fakes.FakeCtrlClient) }{ { name: "serviceaccount reconciliation successful", @@ -53,7 +48,7 @@ func TestCreateOrApplyServiceAccounts(t *testing.T) { return false, nil }) }, - wantErr: `failed to check istiocsr-test-ns/cert-manager-istio-csr serviceaccount resource already exists: test client error`, + wantErr: `failed to check if ServiceAccount "istiocsr-test-ns/cert-manager-istio-csr" exists: test client error`, }, { name: "serviceaccount reconciliation fails while updating status", @@ -66,35 +61,10 @@ func TestCreateOrApplyServiceAccounts(t *testing.T) { return nil }) }, - wantErr: `failed to update istiocsr-test-ns/istiocsr-test-resource istiocsr status with istiocsr-test-ns/cert-manager-istio-csr serviceaccount resource name: failed to update status for "istiocsr-test-ns/istiocsr-test-resource": failed to update istiocsr.openshift.operator.io "istiocsr-test-ns/istiocsr-test-resource" status: test client error`, + wantErr: `failed to update istiocsr-test-ns/istiocsr-test-resource istiocsr status with cert-manager-istio-csr serviceaccount resource name: failed to update status for "istiocsr-test-ns/istiocsr-test-resource": failed to update istiocsr.openshift.operator.io "istiocsr-test-ns/istiocsr-test-resource" status: test client error`, }, { - name: "serviceaccount already exists with istioCSRCreateRecon true records ResourceAlreadyExists event", - istioCSRCreateRecon: true, - 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 - }) - }, - assertEvents: func(t *testing.T, r *Reconciler) { - rec := r.eventRecorder.(*record.FakeRecorder) - select { - case evt := <-rec.Events: - if !strings.Contains(evt, "ResourceAlreadyExists") || !strings.Contains(evt, "serviceaccount resource already exists") { - t.Errorf("createOrApplyServiceAccounts() event: %q, want ResourceAlreadyExists and serviceaccount already exists", evt) - } - case <-time.After(time.Second): - t.Error("expected ResourceAlreadyExists event but none received") - } - }, - }, - { - name: "serviceaccount exists but modified is updated and reconciled", + name: "serviceaccount exists but modified is applied and reconciled", 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) { @@ -105,27 +75,15 @@ func TestCreateOrApplyServiceAccounts(t *testing.T) { } return true, nil }) - m.UpdateWithRetryReturns(nil) - }, - assertEvents: func(t *testing.T, r *Reconciler) { - rec := r.eventRecorder.(*record.FakeRecorder) - select { - case evt := <-rec.Events: - if !strings.Contains(evt, "Reconciled") || !strings.Contains(evt, "reconciled back to desired state") { - t.Errorf("createOrApplyServiceAccounts() event: %q, want Reconciled and reconciled back to desired state", evt) - } - case <-time.After(time.Second): - t.Error("expected Reconciled event but none received") - } }, assertCalls: func(t *testing.T, mock *fakes.FakeCtrlClient) { - if mock.UpdateWithRetryCallCount() != 1 { - t.Errorf("createOrApplyServiceAccounts() UpdateWithRetry call count: %d, want 1", mock.UpdateWithRetryCallCount()) + if mock.ApplyCallCount() != 1 { + t.Errorf("createOrApplyServiceAccounts() Apply call count: %d, want 1", mock.ApplyCallCount()) } }, }, { - name: "serviceaccount reconciliation fails while updating modified serviceaccount", + name: "serviceaccount reconciliation fails while applying modified serviceaccount", 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) { @@ -136,15 +94,11 @@ func TestCreateOrApplyServiceAccounts(t *testing.T) { } return true, nil }) - m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, _ ...client.UpdateOption) error { - switch obj.(type) { - case *corev1.ServiceAccount: - return errTestClient - } - return nil - }) + m.ApplyReturns(errTestClient) + // The merge-patch fallback for the drifted object fails too. + m.PatchReturns(errTestClient) }, - wantErr: `failed to update istiocsr-test-ns/cert-manager-istio-csr serviceaccount resource: test client error`, + wantErr: `failed to apply ServiceAccount "istiocsr-test-ns/cert-manager-istio-csr": test client error`, }, { name: "serviceaccount created when it does not exist", @@ -158,8 +112,8 @@ func TestCreateOrApplyServiceAccounts(t *testing.T) { }) }, assertCalls: func(t *testing.T, mock *fakes.FakeCtrlClient) { - if mock.CreateCallCount() != 1 { - t.Errorf("createOrApplyServiceAccounts() Create call count: %d, want 1", mock.CreateCallCount()) + if mock.ApplyCallCount() != 1 { + t.Errorf("createOrApplyServiceAccounts() Apply call count: %d, want 1", mock.ApplyCallCount()) } }, }, @@ -174,7 +128,7 @@ func TestCreateOrApplyServiceAccounts(t *testing.T) { } r.CtrlClient = mock istiocsr := testIstioCSR() - err := r.createOrApplyServiceAccounts(istiocsr, controllerDefaultResourceLabels, tt.istioCSRCreateRecon) + err := r.createOrApplyServiceAccounts(istiocsr, controllerDefaultResourceLabels) if (tt.wantErr != "" || err != nil) && (err == nil || err.Error() != tt.wantErr) { t.Errorf("createOrApplyServiceAccounts() err: %v, wantErr: %v", err, tt.wantErr) } @@ -183,9 +137,6 @@ func TestCreateOrApplyServiceAccounts(t *testing.T) { t.Errorf("createOrApplyServiceAccounts() got: %v, want: %s", istiocsr.Status.ServiceAccount, serviceAccountName) } } - if tt.assertEvents != nil { - tt.assertEvents(t, r) - } if tt.assertCalls != nil { tt.assertCalls(t, mock) } diff --git a/pkg/controller/istiocsr/services.go b/pkg/controller/istiocsr/services.go index 9a6c34cab..5b657e8a1 100644 --- a/pkg/controller/istiocsr/services.go +++ b/pkg/controller/istiocsr/services.go @@ -4,7 +4,6 @@ import ( "fmt" corev1 "k8s.io/api/core/v1" - "sigs.k8s.io/controller-runtime/pkg/client" "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" "github.com/openshift/cert-manager-operator/pkg/controller/common" @@ -12,13 +11,12 @@ import ( ) const ( - // grpcServicePortName is the name found for the GRPC service in the static manifest. grpcServicePortName = "web" ) -func (r *Reconciler) createOrApplyServices(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string, istioCSRCreateRecon bool) error { +func (r *Reconciler) createOrApplyServices(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) error { service := r.getServiceObject(istiocsr, resourceLabels) - if err := r.createOrApplyService(istiocsr, service, istioCSRCreateRecon); err != nil { + if err := r.createOrApplyService(istiocsr, service); err != nil { return err } if err := r.updateGRPCEndpointInStatus(istiocsr, service); err != nil { @@ -26,43 +24,16 @@ func (r *Reconciler) createOrApplyServices(istiocsr *v1alpha1.IstioCSR, resource } metricsService := r.getMetricsServiceObject(istiocsr, resourceLabels) - if err := r.createOrApplyService(istiocsr, metricsService, istioCSRCreateRecon); err != nil { + if err := r.createOrApplyService(istiocsr, metricsService); err != nil { return err } return nil } -func (r *Reconciler) createOrApplyService(istiocsr *v1alpha1.IstioCSR, svc *corev1.Service, istioCSRCreateRecon bool) error { - serviceName := fmt.Sprintf("%s/%s", svc.GetNamespace(), svc.GetName()) - r.log.V(4).Info("reconciling service resource", "name", serviceName) - fetched := &corev1.Service{} - exist, err := r.Exists(r.ctx, client.ObjectKeyFromObject(svc), fetched) - if err != nil { - return common.FromClientError(err, "failed to check %s service resource already exists", serviceName) - } - - if exist { - if istioCSRCreateRecon { - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s service resource already exists, maybe from previous installation", serviceName) - } - if hasObjectChanged(svc, fetched) { - r.log.V(1).Info("service has been modified, updating to desired state", "name", serviceName) - if err := r.UpdateWithRetry(r.ctx, svc); err != nil { - return common.FromClientError(err, "failed to update %s service resource", serviceName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "service resource %s reconciled back to desired state", serviceName) - } else { - r.log.V(4).Info("service resource already exists and is in expected state", "name", serviceName) - } - } - - if !exist { - if err := r.Create(r.ctx, svc); err != nil { - return common.FromClientError(err, "failed to create %s service resource", serviceName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "service resource %s created", serviceName) - } - return nil +func (r *Reconciler) createOrApplyService(istiocsr *v1alpha1.IstioCSR, svc *corev1.Service) error { + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, istiocsr, svc, &corev1.Service{}, fieldOwner, + func(d, e *corev1.Service) bool { return hasObjectChanged(d, e) }, + ) } func (r *Reconciler) getServiceObject(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) *corev1.Service { diff --git a/pkg/controller/istiocsr/services_test.go b/pkg/controller/istiocsr/services_test.go index 708a116a4..4edb98740 100644 --- a/pkg/controller/istiocsr/services_test.go +++ b/pkg/controller/istiocsr/services_test.go @@ -50,18 +50,12 @@ func TestCreateOrApplyServices(t *testing.T) { return false, nil }) }, - wantErr: `failed to check istiocsr-test-ns/cert-manager-istio-csr service resource already exists: test client error`, + wantErr: `failed to check if Service "istiocsr-test-ns/cert-manager-istio-csr" exists: test client error`, }, { - name: "service reconciliation fails while updating to desired state", + name: "service reconciliation fails while applying to desired state", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { - m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, option ...client.UpdateOption) error { - switch obj.(type) { - case *corev1.Service: - return errTestClient - } - return nil - }) + m.ApplyReturns(errTestClient) m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { switch o := obj.(type) { case *corev1.Service: @@ -72,21 +66,17 @@ func TestCreateOrApplyServices(t *testing.T) { } return false, nil }) + // The merge-patch fallback for the drifted object fails too. + m.PatchReturns(errTestClient) }, - wantErr: `failed to update istiocsr-test-ns/cert-manager-istio-csr service resource: test client error`, + wantErr: `failed to apply Service "istiocsr-test-ns/cert-manager-istio-csr": test client error`, }, { name: "service reconciliation fails while creating", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { - m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { - switch obj.(type) { - case *corev1.Service: - return errTestClient - } - return nil - }) + m.ApplyReturns(errTestClient) }, - wantErr: `failed to create istiocsr-test-ns/cert-manager-istio-csr service resource: test client error`, + wantErr: `failed to apply Service "istiocsr-test-ns/cert-manager-istio-csr": test client error`, }, { name: "service reconciliation when server config is not empty", @@ -111,7 +101,7 @@ func TestCreateOrApplyServices(t *testing.T) { if tt.updateIstioCSR != nil { tt.updateIstioCSR(istiocsr) } - err := r.createOrApplyServices(istiocsr, controllerDefaultResourceLabels, false) + err := r.createOrApplyServices(istiocsr, controllerDefaultResourceLabels) if (tt.wantErr != "" || err != nil) && (err == nil || err.Error() != tt.wantErr) { t.Errorf("createOrApplyServices() err: %v, wantErr: %v", err, tt.wantErr) } diff --git a/pkg/controller/trustmanager/certificates.go b/pkg/controller/trustmanager/certificates.go index 025ead882..b6d675424 100644 --- a/pkg/controller/trustmanager/certificates.go +++ b/pkg/controller/trustmanager/certificates.go @@ -5,9 +5,7 @@ import ( "reflect" "slices" - corev1 "k8s.io/api/core/v1" "k8s.io/utils/ptr" - "sigs.k8s.io/controller-runtime/pkg/client" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" certmanagermetav1 "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" @@ -17,29 +15,9 @@ import ( "github.com/openshift/cert-manager-operator/pkg/operator/assets" ) -// createOrApplyIssuer reconciles the self-signed Issuer used for trust-manager's webhook TLS. func (r *Reconciler) createOrApplyIssuer(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string) error { desired := getIssuerObject(resourceLabels, resourceAnnotations) - resourceName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling issuer resource", "name", resourceName) - - existing := &certmanagerv1.Issuer{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if issuer %q exists", resourceName) - } - if exists && !issuerModified(desired, existing) { - r.log.V(4).Info("issuer resource exists and is in desired state", "name", resourceName) - return nil - } - - r.log.V(2).Info("issuer resource has been modified, updating to desired state", "name", resourceName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply issuer %q", resourceName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "issuer resource %s applied", resourceName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &certmanagerv1.Issuer{}, fieldOwner, issuerModified) } func getIssuerObject(resourceLabels, resourceAnnotations map[string]string) *certmanagerv1.Issuer { @@ -51,29 +29,9 @@ func getIssuerObject(resourceLabels, resourceAnnotations map[string]string) *cer return issuer } -// createOrApplyCertificate reconciles the Certificate used for trust-manager's webhook TLS. func (r *Reconciler) createOrApplyCertificate(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string) error { desired := getCertificateObject(resourceLabels, resourceAnnotations) - resourceName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling certificate resource", "name", resourceName) - - existing := &certmanagerv1.Certificate{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if certificate %q exists", resourceName) - } - if exists && !certificateModified(desired, existing) { - r.log.V(4).Info("certificate resource exists and is in desired state", "name", resourceName) - return nil - } - - r.log.V(2).Info("certificate resource has been modified, updating to desired state", "name", resourceName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply certificate %q", resourceName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "certificate resource %s applied", resourceName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &certmanagerv1.Certificate{}, fieldOwner, certificateModified) } func getCertificateObject(resourceLabels, resourceAnnotations map[string]string) *certmanagerv1.Certificate { @@ -96,15 +54,11 @@ func getCertificateObject(resourceLabels, resourceAnnotations map[string]string) return certificate } -// issuerModified compares only the fields we manage via SSA. func issuerModified(desired, existing *certmanagerv1.Issuer) bool { return managedMetadataModified(desired, existing) || !reflect.DeepEqual(desired.Spec, existing.Spec) } -// certificateModified compares only the fields we manage via SSA. -// We compare individual spec fields rather than the full Spec because -// cert-manager's webhook may default fields we don't set (e.g. Duration). func certificateModified(desired, existing *certmanagerv1.Certificate) bool { if managedMetadataModified(desired, existing) { return true diff --git a/pkg/controller/trustmanager/certificates_test.go b/pkg/controller/trustmanager/certificates_test.go index 470520623..ebc215658 100644 --- a/pkg/controller/trustmanager/certificates_test.go +++ b/pkg/controller/trustmanager/certificates_test.go @@ -186,7 +186,7 @@ func TestIssuerReconciliation(t *testing.T) { preReq func(*Reconciler, *fakes.FakeCtrlClient) wantErr string wantExistsCount int - wantPatchCount int + wantApplyCount int }{ { name: "successful apply when not found", @@ -196,7 +196,7 @@ func TestIssuerReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "skip apply when existing matches desired", @@ -208,7 +208,7 @@ func TestIssuerReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "apply when existing has label drift", @@ -221,7 +221,7 @@ func TestIssuerReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when existing has annotation drift", @@ -236,7 +236,7 @@ func TestIssuerReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "exists error propagates", @@ -245,9 +245,9 @@ func TestIssuerReconciliation(t *testing.T) { return false, errTestClient }) }, - wantErr: "failed to check if issuer", + wantErr: "failed to check if Issuer", wantExistsCount: 1, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "patch error propagates", @@ -255,13 +255,13 @@ func TestIssuerReconciliation(t *testing.T) { m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { return false, nil }) - m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { return errTestClient }) }, - wantErr: "failed to apply issuer", + wantErr: "failed to apply Issuer", wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, } @@ -285,8 +285,8 @@ func TestIssuerReconciliation(t *testing.T) { if got := mock.ExistsCallCount(); got != tt.wantExistsCount { t.Errorf("expected %d Exists calls, got %d", tt.wantExistsCount, got) } - if got := mock.PatchCallCount(); got != tt.wantPatchCount { - t.Errorf("expected %d Patch calls, got %d", tt.wantPatchCount, got) + if got := mock.ApplyCallCount(); got != tt.wantApplyCount { + t.Errorf("expected %d Apply calls, got %d", tt.wantApplyCount, got) } }) } @@ -299,7 +299,7 @@ func TestCertificateReconciliation(t *testing.T) { preReq func(*Reconciler, *fakes.FakeCtrlClient) wantErr string wantExistsCount int - wantPatchCount int + wantApplyCount int }{ { name: "successful apply when not found", @@ -309,7 +309,7 @@ func TestCertificateReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "skip apply when existing matches desired", @@ -321,7 +321,7 @@ func TestCertificateReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "apply when existing has label drift", @@ -334,7 +334,7 @@ func TestCertificateReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when existing has annotation drift", @@ -349,7 +349,7 @@ func TestCertificateReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when existing has secret name drift", @@ -362,7 +362,7 @@ func TestCertificateReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when existing has issuer ref drift", @@ -379,7 +379,7 @@ func TestCertificateReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when existing is missing privateKey", @@ -392,7 +392,7 @@ func TestCertificateReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when existing has privateKey rotation policy drift", @@ -407,7 +407,7 @@ func TestCertificateReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "skip apply when existing privateKey has extra defaulted fields", @@ -424,7 +424,7 @@ func TestCertificateReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "exists error propagates", @@ -433,9 +433,9 @@ func TestCertificateReconciliation(t *testing.T) { return false, errTestClient }) }, - wantErr: "failed to check if certificate", + wantErr: "failed to check if Certificate", wantExistsCount: 1, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "patch error propagates", @@ -443,13 +443,13 @@ func TestCertificateReconciliation(t *testing.T) { m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { return false, nil }) - m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { return errTestClient }) }, - wantErr: "failed to apply certificate", + wantErr: "failed to apply Certificate", wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, } @@ -473,8 +473,8 @@ func TestCertificateReconciliation(t *testing.T) { if got := mock.ExistsCallCount(); got != tt.wantExistsCount { t.Errorf("expected %d Exists calls, got %d", tt.wantExistsCount, got) } - if got := mock.PatchCallCount(); got != tt.wantPatchCount { - t.Errorf("expected %d Patch calls, got %d", tt.wantPatchCount, got) + if got := mock.ApplyCallCount(); got != tt.wantApplyCount { + t.Errorf("expected %d Apply calls, got %d", tt.wantApplyCount, got) } }) } diff --git a/pkg/controller/trustmanager/configmaps.go b/pkg/controller/trustmanager/configmaps.go index 9aa0c1c1e..b85a9248e 100644 --- a/pkg/controller/trustmanager/configmaps.go +++ b/pkg/controller/trustmanager/configmaps.go @@ -60,7 +60,7 @@ func (r *Reconciler) createOrApplyDefaultCAPackageConfigMap(trustManager *v1alph } r.log.V(2).Info("default CA package ConfigMap has been modified, updating to desired state", "name", cmName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { + if err := r.Apply(r.ctx, desired, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { return "", common.FromClientError(err, "failed to apply ConfigMap %q", cmName) } diff --git a/pkg/controller/trustmanager/configmaps_test.go b/pkg/controller/trustmanager/configmaps_test.go index 4629949b0..97e29f725 100644 --- a/pkg/controller/trustmanager/configmaps_test.go +++ b/pkg/controller/trustmanager/configmaps_test.go @@ -155,7 +155,7 @@ func TestDefaultCAPackageConfigMapReconciliation(t *testing.T) { wantHash bool wantErr string wantExistsCount int - wantPatchCount int + wantApplyCount int }{ { name: "skips when policy is Disabled", @@ -163,7 +163,7 @@ func TestDefaultCAPackageConfigMapReconciliation(t *testing.T) { preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { }, wantExistsCount: 0, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "skips when policy is unset (defaults to Disabled)", @@ -171,7 +171,7 @@ func TestDefaultCAPackageConfigMapReconciliation(t *testing.T) { preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { }, wantExistsCount: 0, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "returns error when injection ConfigMap is not found", @@ -225,7 +225,7 @@ func TestDefaultCAPackageConfigMapReconciliation(t *testing.T) { }, wantHash: true, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "skips patch when existing ConfigMap matches desired", @@ -247,7 +247,7 @@ func TestDefaultCAPackageConfigMapReconciliation(t *testing.T) { }, wantHash: true, wantExistsCount: 1, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "patches when existing ConfigMap data differs", @@ -267,7 +267,7 @@ func TestDefaultCAPackageConfigMapReconciliation(t *testing.T) { }, wantHash: true, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "propagates Exists error", @@ -287,7 +287,7 @@ func TestDefaultCAPackageConfigMapReconciliation(t *testing.T) { wantExistsCount: 1, }, { - name: "propagates Patch error", + name: "propagates Apply error", tm: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicyEnabled), preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.GetCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) error { @@ -299,13 +299,13 @@ func TestDefaultCAPackageConfigMapReconciliation(t *testing.T) { m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { return false, nil }) - m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { return errTestClient }) }, wantErr: "failed to apply ConfigMap", wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, } @@ -331,8 +331,8 @@ func TestDefaultCAPackageConfigMapReconciliation(t *testing.T) { if got := mock.ExistsCallCount(); got != tt.wantExistsCount { t.Errorf("expected %d Exists calls, got %d", tt.wantExistsCount, got) } - if got := mock.PatchCallCount(); got != tt.wantPatchCount { - t.Errorf("expected %d Patch calls, got %d", tt.wantPatchCount, got) + if got := mock.ApplyCallCount(); got != tt.wantApplyCount { + t.Errorf("expected %d Apply calls, got %d", tt.wantApplyCount, got) } }) } diff --git a/pkg/controller/trustmanager/controller_test.go b/pkg/controller/trustmanager/controller_test.go index 973c7b644..5b02501e1 100644 --- a/pkg/controller/trustmanager/controller_test.go +++ b/pkg/controller/trustmanager/controller_test.go @@ -164,7 +164,7 @@ func TestProcessReconcileRequest(t *testing.T) { return nil }) // Namespace exists; all other resources return not-found so they - // are created via SSA Patch (which succeeds by default). + // are created via SSA Apply (which succeeds by default). m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { switch obj.(type) { case *corev1.Namespace: @@ -255,7 +255,7 @@ func TestProcessReconcileRequest(t *testing.T) { Reason: v1alpha1.ReasonInProgress, }, }, - wantErr: "failed to check if serviceaccount", + wantErr: "failed to check if ServiceAccount", }, { name: "trust namespace does not exist sets degraded true", @@ -314,7 +314,7 @@ func TestProcessReconcileRequest(t *testing.T) { } return nil }) - // Custom namespace exists; so SSA Patch will create or update all resources successfully. + // Custom namespace exists; so SSA Apply will create or update all resources successfully. m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { switch obj.(type) { case *corev1.Namespace: diff --git a/pkg/controller/trustmanager/deployments.go b/pkg/controller/trustmanager/deployments.go index cbfa717c2..56ed1aea6 100644 --- a/pkg/controller/trustmanager/deployments.go +++ b/pkg/controller/trustmanager/deployments.go @@ -23,26 +23,7 @@ func (r *Reconciler) createOrApplyDeployment(trustManager *v1alpha1.TrustManager return err } - deploymentName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling deployment resource", "name", deploymentName) - - existing := &appsv1.Deployment{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if deployment %q exists", deploymentName) - } - if exists && !deploymentModified(desired, existing) { - r.log.V(4).Info("deployment resource exists and is in desired state", "name", deploymentName) - return nil - } - - r.log.V(2).Info("deployment resource has been modified, updating to desired state", "name", deploymentName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply deployment %q", deploymentName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "deployment resource %s applied", deploymentName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &appsv1.Deployment{}, fieldOwner, deploymentModified) } func (r *Reconciler) getDeploymentObject(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string, caBundleHash string) (*appsv1.Deployment, error) { diff --git a/pkg/controller/trustmanager/deployments_test.go b/pkg/controller/trustmanager/deployments_test.go index 9591acda5..529792595 100644 --- a/pkg/controller/trustmanager/deployments_test.go +++ b/pkg/controller/trustmanager/deployments_test.go @@ -476,7 +476,7 @@ func TestDeploymentReconciliation(t *testing.T) { preReq func(*Reconciler, *fakes.FakeCtrlClient) wantErr string wantExistsCount int - wantPatchCount int + wantApplyCount int }{ { name: "successful apply when not found", @@ -487,7 +487,7 @@ func TestDeploymentReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "skip apply when existing matches desired", @@ -504,7 +504,7 @@ func TestDeploymentReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "apply when service account uid changes", @@ -521,7 +521,7 @@ func TestDeploymentReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "serviceaccount get error propagates", @@ -536,7 +536,7 @@ func TestDeploymentReconciliation(t *testing.T) { }, wantErr: "failed to get serviceaccount", wantExistsCount: 0, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "apply when existing has label drift", @@ -554,7 +554,7 @@ func TestDeploymentReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when existing has annotation drift", @@ -574,7 +574,7 @@ func TestDeploymentReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when defaultCAPackage changed from Enabled to Disabled", @@ -592,7 +592,7 @@ func TestDeploymentReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when existing has pod template annotation drift", @@ -613,7 +613,7 @@ func TestDeploymentReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when existing has image drift", @@ -631,7 +631,7 @@ func TestDeploymentReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when existing has replicas drift", @@ -649,7 +649,7 @@ func TestDeploymentReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when existing has args drift", @@ -669,7 +669,7 @@ func TestDeploymentReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "exists error propagates", @@ -679,9 +679,9 @@ func TestDeploymentReconciliation(t *testing.T) { return false, errTestClient }) }, - wantErr: "failed to check if deployment", + wantErr: "failed to check if Deployment", wantExistsCount: 1, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "patch error propagates", @@ -690,20 +690,20 @@ func TestDeploymentReconciliation(t *testing.T) { m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { return false, nil }) - m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { return errTestClient }) }, - wantErr: "failed to apply deployment", + wantErr: "failed to apply Deployment", wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "missing image error propagates", setImage: false, wantErr: "environment variable with trust-manager image not set", wantExistsCount: 0, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "invalid toleration config returns error", @@ -717,7 +717,7 @@ func TestDeploymentReconciliation(t *testing.T) { }), wantErr: `spec.trustManagerConfig.tolerations[0].operator: Invalid value: "should-be-empty-for-exists": value must be empty when ` + "`operator` is 'Exists'", wantExistsCount: 0, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "invalid nodeSelector config returns error", @@ -727,7 +727,7 @@ func TestDeploymentReconciliation(t *testing.T) { }), wantErr: `spec.trustManagerConfig.nodeSelector: Invalid value: "node/Label/2"`, wantExistsCount: 0, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "invalid affinity config returns error", @@ -750,7 +750,7 @@ func TestDeploymentReconciliation(t *testing.T) { }), wantErr: `spec.trustManagerConfig.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms[0].matchExpressions[0].values: Required value: must be specified when ` + "`operator` is 'In' or 'NotIn'", wantExistsCount: 0, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "invalid resource requirements config returns error", @@ -765,7 +765,7 @@ func TestDeploymentReconciliation(t *testing.T) { }), wantErr: `spec.trustManagerConfig.resources.requests: Invalid value: "2": must be less than or equal to cpu limit of 1`, wantExistsCount: 0, - wantPatchCount: 0, + wantApplyCount: 0, }, } @@ -795,8 +795,8 @@ func TestDeploymentReconciliation(t *testing.T) { if got := mock.ExistsCallCount(); got != tt.wantExistsCount { t.Errorf("expected %d Exists calls, got %d", tt.wantExistsCount, got) } - if got := mock.PatchCallCount(); got != tt.wantPatchCount { - t.Errorf("expected %d Patch calls, got %d", tt.wantPatchCount, got) + if got := mock.ApplyCallCount(); got != tt.wantApplyCount { + t.Errorf("expected %d Apply calls, got %d", tt.wantApplyCount, got) } }) } diff --git a/pkg/controller/trustmanager/rbacs.go b/pkg/controller/trustmanager/rbacs.go index 6033f6501..b9dad190d 100644 --- a/pkg/controller/trustmanager/rbacs.go +++ b/pkg/controller/trustmanager/rbacs.go @@ -1,13 +1,10 @@ package trustmanager import ( - "fmt" "reflect" "slices" - corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" - "sigs.k8s.io/controller-runtime/pkg/client" "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" "github.com/openshift/cert-manager-operator/pkg/controller/common" @@ -57,26 +54,7 @@ func (r *Reconciler) createOrApplyRBACResources(trustManager *v1alpha1.TrustMana func (r *Reconciler) createOrApplyClusterRole(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string) error { desired := getClusterRoleObject(trustManager.Spec.TrustManagerConfig.SecretTargets, resourceLabels, resourceAnnotations) - resourceName := desired.GetName() - r.log.V(4).Info("reconciling clusterrole resource", "name", resourceName) - - existing := &rbacv1.ClusterRole{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if clusterrole %q exists", resourceName) - } - if exists && !clusterRoleModified(desired, existing) { - r.log.V(4).Info("clusterrole resource exists and is in desired state", "name", resourceName) - return nil - } - - r.log.V(2).Info("clusterrole resource has been modified, updating to desired state", "name", resourceName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply clusterrole %q", resourceName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "clusterrole resource %s applied", resourceName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &rbacv1.ClusterRole{}, fieldOwner, clusterRoleModified) } func getClusterRoleObject(secretTargets v1alpha1.SecretTargetsConfig, resourceLabels, resourceAnnotations map[string]string) *rbacv1.ClusterRole { @@ -88,9 +66,6 @@ func getClusterRoleObject(secretTargets v1alpha1.SecretTargetsConfig, resourceLa return clusterRole } -// appendSecretTargetRules adds cluster-wide secret read and scoped write rules -// to the ClusterRole when the secretTargets policy is Custom. The authorizedSecrets -// list is sorted to ensure deterministic rule ordering for comparison. func appendSecretTargetRules(clusterRole *rbacv1.ClusterRole, secretTargets v1alpha1.SecretTargetsConfig) { if !secretTargetsEnabled(secretTargets) { return @@ -117,26 +92,7 @@ const clusterReaderAggregateLabel = "rbac.authorization.k8s.io/aggregate-to-clus func (r *Reconciler) createOrApplyClusterViewClusterRole(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string) error { desired := getClusterViewClusterRoleObject(resourceLabels, resourceAnnotations) - resourceName := desired.GetName() - r.log.V(4).Info("reconciling cluster-view clusterrole resource", "name", resourceName) - - existing := &rbacv1.ClusterRole{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if clusterrole %q exists", resourceName) - } - if exists && !clusterRoleModified(desired, existing) { - r.log.V(4).Info("cluster-view clusterrole resource exists and is in desired state", "name", resourceName) - return nil - } - - r.log.V(2).Info("cluster-view clusterrole resource has been modified, updating to desired state", "name", resourceName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply clusterrole %q", resourceName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "clusterrole resource %s applied", resourceName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &rbacv1.ClusterRole{}, fieldOwner, clusterRoleModified) } func getClusterViewClusterRoleObject(resourceLabels, resourceAnnotations map[string]string) *rbacv1.ClusterRole { @@ -155,26 +111,7 @@ func getClusterViewClusterRoleObject(resourceLabels, resourceAnnotations map[str func (r *Reconciler) createOrApplyClusterRoleBinding(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string) error { desired := getClusterRoleBindingObject(resourceLabels, resourceAnnotations) - resourceName := desired.GetName() - r.log.V(4).Info("reconciling clusterrolebinding resource", "name", resourceName) - - existing := &rbacv1.ClusterRoleBinding{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if clusterrolebinding %q exists", resourceName) - } - if exists && !clusterRoleBindingModified(desired, existing) { - r.log.V(4).Info("clusterrolebinding resource exists and is in desired state", "name", resourceName) - return nil - } - - r.log.V(2).Info("clusterrolebinding resource has been modified, updating to desired state", "name", resourceName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply clusterrolebinding %q", resourceName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "clusterrolebinding resource %s applied", resourceName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &rbacv1.ClusterRoleBinding{}, fieldOwner, clusterRoleBindingModified) } func getClusterRoleBindingObject(resourceLabels, resourceAnnotations map[string]string) *rbacv1.ClusterRoleBinding { @@ -187,30 +124,11 @@ func getClusterRoleBindingObject(resourceLabels, resourceAnnotations map[string] return clusterRoleBinding } -// Role for trust namespace (secrets access) +// Role for trust namespace func (r *Reconciler) createOrApplyTrustNamespaceRole(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string, trustNamespace string) error { desired := getTrustNamespaceRoleObject(resourceLabels, resourceAnnotations, trustNamespace) - resourceName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling role resource for trust namespace", "name", resourceName) - - existing := &rbacv1.Role{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if role %q exists", resourceName) - } - if exists && !roleModified(desired, existing) { - r.log.V(4).Info("role resource exists and is in desired state", "name", resourceName) - return nil - } - - r.log.V(2).Info("role resource has been modified, updating to desired state", "name", resourceName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply role %q", resourceName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "role resource %s applied", resourceName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &rbacv1.Role{}, fieldOwner, roleModified) } func getTrustNamespaceRoleObject(resourceLabels, resourceAnnotations map[string]string, trustNamespace string) *rbacv1.Role { @@ -222,30 +140,11 @@ func getTrustNamespaceRoleObject(resourceLabels, resourceAnnotations map[string] return role } -// RoleBinding for trust namespace (secrets access) +// RoleBinding for trust namespace func (r *Reconciler) createOrApplyTrustNamespaceRoleBinding(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string, trustNamespace string) error { desired := getTrustNamespaceRoleBindingObject(resourceLabels, resourceAnnotations, trustNamespace) - resourceName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling rolebinding resource for trust namespace", "name", resourceName) - - existing := &rbacv1.RoleBinding{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if rolebinding %q exists", resourceName) - } - if exists && !roleBindingModified(desired, existing) { - r.log.V(4).Info("rolebinding resource exists and is in desired state", "name", resourceName) - return nil - } - - r.log.V(2).Info("rolebinding resource has been modified, updating to desired state", "name", resourceName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply rolebinding %q", resourceName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "rolebinding resource %s applied", resourceName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &rbacv1.RoleBinding{}, fieldOwner, roleBindingModified) } func getTrustNamespaceRoleBindingObject(resourceLabels, resourceAnnotations map[string]string, trustNamespace string) *rbacv1.RoleBinding { @@ -259,30 +158,11 @@ func getTrustNamespaceRoleBindingObject(resourceLabels, resourceAnnotations map[ return roleBinding } -// Leader election Role (in operand namespace) +// Leader election Role func (r *Reconciler) createOrApplyLeaderElectionRole(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string) error { desired := getLeaderElectionRoleObject(resourceLabels, resourceAnnotations) - resourceName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling leader election role resource", "name", resourceName) - - existing := &rbacv1.Role{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if leader election role %q exists", resourceName) - } - if exists && !roleModified(desired, existing) { - r.log.V(4).Info("leader election role resource exists and is in desired state", "name", resourceName) - return nil - } - - r.log.V(2).Info("leader election role resource has been modified, updating to desired state", "name", resourceName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply leader election role %q", resourceName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "leader election role resource %s applied", resourceName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &rbacv1.Role{}, fieldOwner, roleModified) } func getLeaderElectionRoleObject(resourceLabels, resourceAnnotations map[string]string) *rbacv1.Role { @@ -294,30 +174,11 @@ func getLeaderElectionRoleObject(resourceLabels, resourceAnnotations map[string] return role } -// Leader election RoleBinding (in operand namespace) +// Leader election RoleBinding func (r *Reconciler) createOrApplyLeaderElectionRoleBinding(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string) error { desired := getLeaderElectionRoleBindingObject(resourceLabels, resourceAnnotations) - resourceName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling leader election rolebinding resource", "name", resourceName) - - existing := &rbacv1.RoleBinding{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if leader election rolebinding %q exists", resourceName) - } - if exists && !roleBindingModified(desired, existing) { - r.log.V(4).Info("leader election rolebinding resource exists and is in desired state", "name", resourceName) - return nil - } - - r.log.V(2).Info("leader election rolebinding resource has been modified, updating to desired state", "name", resourceName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply leader election rolebinding %q", resourceName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "leader election rolebinding resource %s applied", resourceName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &rbacv1.RoleBinding{}, fieldOwner, roleBindingModified) } func getLeaderElectionRoleBindingObject(resourceLabels, resourceAnnotations map[string]string) *rbacv1.RoleBinding { @@ -331,7 +192,6 @@ func getLeaderElectionRoleBindingObject(resourceLabels, resourceAnnotations map[ return roleBinding } -// updateBindingSubjects sets the ServiceAccount name and namespace on RBAC binding subjects. func updateBindingSubjects(subjects []rbacv1.Subject, serviceAccountName, namespace string) { for i := range subjects { if subjects[i].Kind == roleBindingSubjectKind { @@ -341,26 +201,22 @@ func updateBindingSubjects(subjects []rbacv1.Subject, serviceAccountName, namesp } } -// clusterRoleModified compares only the fields we manage via SSA. func clusterRoleModified(desired, existing *rbacv1.ClusterRole) bool { return managedMetadataModified(desired, existing) || !reflect.DeepEqual(desired.Rules, existing.Rules) } -// clusterRoleBindingModified compares only the fields we manage via SSA. func clusterRoleBindingModified(desired, existing *rbacv1.ClusterRoleBinding) bool { return managedMetadataModified(desired, existing) || !reflect.DeepEqual(desired.RoleRef, existing.RoleRef) || !reflect.DeepEqual(desired.Subjects, existing.Subjects) } -// roleModified compares only the fields we manage via SSA. func roleModified(desired, existing *rbacv1.Role) bool { return managedMetadataModified(desired, existing) || !reflect.DeepEqual(desired.Rules, existing.Rules) } -// roleBindingModified compares only the fields we manage via SSA. func roleBindingModified(desired, existing *rbacv1.RoleBinding) bool { return managedMetadataModified(desired, existing) || !reflect.DeepEqual(desired.RoleRef, existing.RoleRef) || diff --git a/pkg/controller/trustmanager/rbacs_test.go b/pkg/controller/trustmanager/rbacs_test.go index d9fab3654..f2ebc5f18 100644 --- a/pkg/controller/trustmanager/rbacs_test.go +++ b/pkg/controller/trustmanager/rbacs_test.go @@ -342,7 +342,7 @@ func TestRBACReconciliation(t *testing.T) { preReq func(*Reconciler, *fakes.FakeCtrlClient) wantErr string wantExistsCount int - wantPatchCount int + wantApplyCount int }{ { name: "successful apply of all RBAC resources", @@ -352,7 +352,7 @@ func TestRBACReconciliation(t *testing.T) { }) }, wantExistsCount: 7, - wantPatchCount: 7, + wantApplyCount: 7, }, { name: "skip apply when all RBAC resources match desired", @@ -387,7 +387,7 @@ func TestRBACReconciliation(t *testing.T) { }) }, wantExistsCount: 7, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "apply when cluster role has label drift", @@ -423,7 +423,7 @@ func TestRBACReconciliation(t *testing.T) { }) }, wantExistsCount: 7, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when cluster role binding has annotation drift", @@ -463,7 +463,7 @@ func TestRBACReconciliation(t *testing.T) { }) }, wantExistsCount: 7, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when role has rules drift", @@ -501,7 +501,7 @@ func TestRBACReconciliation(t *testing.T) { }) }, wantExistsCount: 7, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "exists error propagates on first resource", @@ -510,9 +510,9 @@ func TestRBACReconciliation(t *testing.T) { return false, errTestClient }) }, - wantErr: "failed to check if clusterrole", + wantErr: "failed to check if ClusterRole", wantExistsCount: 1, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "clusterrole patch error propagates", @@ -520,16 +520,16 @@ func TestRBACReconciliation(t *testing.T) { m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { return false, nil }) - m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { if _, ok := obj.(*rbacv1.ClusterRole); ok { return errTestClient } return nil }) }, - wantErr: "failed to apply clusterrole", + wantErr: "failed to apply ClusterRole", wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "clusterrolebinding patch error propagates", @@ -537,16 +537,16 @@ func TestRBACReconciliation(t *testing.T) { m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { return false, nil }) - m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { if _, ok := obj.(*rbacv1.ClusterRoleBinding); ok { return errTestClient } return nil }) }, - wantErr: "failed to apply clusterrolebinding", + wantErr: "failed to apply ClusterRoleBinding", wantExistsCount: 2, - wantPatchCount: 2, + wantApplyCount: 2, }, { name: "role patch error propagates", @@ -554,16 +554,16 @@ func TestRBACReconciliation(t *testing.T) { m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { return false, nil }) - m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { if _, ok := obj.(*rbacv1.Role); ok { return errTestClient } return nil }) }, - wantErr: "failed to apply role", + wantErr: "failed to apply Role", wantExistsCount: 3, - wantPatchCount: 3, + wantApplyCount: 3, }, { name: "rolebinding patch error propagates", @@ -571,16 +571,16 @@ func TestRBACReconciliation(t *testing.T) { m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { return false, nil }) - m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { if _, ok := obj.(*rbacv1.RoleBinding); ok { return errTestClient } return nil }) }, - wantErr: "failed to apply rolebinding", + wantErr: "failed to apply RoleBinding", wantExistsCount: 4, - wantPatchCount: 4, + wantApplyCount: 4, }, } @@ -604,8 +604,8 @@ func TestRBACReconciliation(t *testing.T) { if got := mock.ExistsCallCount(); got != tt.wantExistsCount { t.Errorf("expected %d Exists calls, got %d", tt.wantExistsCount, got) } - if got := mock.PatchCallCount(); got != tt.wantPatchCount { - t.Errorf("expected %d Patch calls, got %d", tt.wantPatchCount, got) + if got := mock.ApplyCallCount(); got != tt.wantApplyCount { + t.Errorf("expected %d Apply calls, got %d", tt.wantApplyCount, got) } }) } @@ -687,7 +687,7 @@ func TestRBACReconciliationWithSecretTargets(t *testing.T) { name string tmBuilder *trustManagerBuilder preReq func(*Reconciler, *fakes.FakeCtrlClient, *v1alpha1.TrustManager) - wantPatchCount int + wantApplyCount int wantExistsCount int }{ { @@ -725,7 +725,7 @@ func TestRBACReconciliationWithSecretTargets(t *testing.T) { }) }, wantExistsCount: 7, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "skip apply when secretTargets rules already match", @@ -762,7 +762,7 @@ func TestRBACReconciliationWithSecretTargets(t *testing.T) { }) }, wantExistsCount: 7, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "apply when authorizedSecrets list changes", @@ -802,7 +802,7 @@ func TestRBACReconciliationWithSecretTargets(t *testing.T) { }) }, wantExistsCount: 7, - wantPatchCount: 1, + wantApplyCount: 1, }, } @@ -822,8 +822,8 @@ func TestRBACReconciliationWithSecretTargets(t *testing.T) { if got := mock.ExistsCallCount(); got != tt.wantExistsCount { t.Errorf("expected %d Exists calls, got %d", tt.wantExistsCount, got) } - if got := mock.PatchCallCount(); got != tt.wantPatchCount { - t.Errorf("expected %d Patch calls, got %d", tt.wantPatchCount, got) + if got := mock.ApplyCallCount(); got != tt.wantApplyCount { + t.Errorf("expected %d Apply calls, got %d", tt.wantApplyCount, got) } }) } diff --git a/pkg/controller/trustmanager/serviceaccounts.go b/pkg/controller/trustmanager/serviceaccounts.go index 844e6420f..c4b4a4a3a 100644 --- a/pkg/controller/trustmanager/serviceaccounts.go +++ b/pkg/controller/trustmanager/serviceaccounts.go @@ -1,11 +1,8 @@ package trustmanager import ( - "fmt" - corev1 "k8s.io/api/core/v1" "k8s.io/utils/ptr" - "sigs.k8s.io/controller-runtime/pkg/client" "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" "github.com/openshift/cert-manager-operator/pkg/controller/common" @@ -14,29 +11,9 @@ import ( func (r *Reconciler) createOrApplyServiceAccounts(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string) error { desired := r.getServiceAccountObject(resourceLabels, resourceAnnotations) - serviceAccountName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling serviceaccount resource", "name", serviceAccountName) - - existing := &corev1.ServiceAccount{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if serviceaccount %q exists", serviceAccountName) - } - if exists && !serviceAccountModified(desired, existing) { - r.log.V(4).Info("serviceaccount resource exists and is in desired state", "name", serviceAccountName) - return nil - } - - r.log.V(2).Info("serviceaccount resource has been modified, updating to desired state", "name", serviceAccountName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply serviceaccount %q", serviceAccountName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "serviceaccount resource %s applied", serviceAccountName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &corev1.ServiceAccount{}, fieldOwner, serviceAccountModified) } -// serviceAccountModified compares only the fields we manage via SSA. func serviceAccountModified(desired, existing *corev1.ServiceAccount) bool { return managedMetadataModified(desired, existing) || !ptr.Equal(desired.AutomountServiceAccountToken, existing.AutomountServiceAccountToken) diff --git a/pkg/controller/trustmanager/serviceaccounts_test.go b/pkg/controller/trustmanager/serviceaccounts_test.go index 70c119582..81e17f3b5 100644 --- a/pkg/controller/trustmanager/serviceaccounts_test.go +++ b/pkg/controller/trustmanager/serviceaccounts_test.go @@ -77,7 +77,7 @@ func TestServiceAccountReconciliation(t *testing.T) { preReq func(*Reconciler, *fakes.FakeCtrlClient) wantErr string wantExistsCount int - wantPatchCount int + wantApplyCount int }{ { name: "successful apply when not found", @@ -87,7 +87,7 @@ func TestServiceAccountReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "skip apply when existing matches desired", @@ -99,7 +99,7 @@ func TestServiceAccountReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "apply when existing has label drift", @@ -112,7 +112,7 @@ func TestServiceAccountReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when existing has annotation drift", @@ -127,7 +127,7 @@ func TestServiceAccountReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when automountServiceAccountToken is modified", @@ -140,7 +140,7 @@ func TestServiceAccountReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "exists error propagates", @@ -149,9 +149,9 @@ func TestServiceAccountReconciliation(t *testing.T) { return false, errTestClient }) }, - wantErr: "failed to check if serviceaccount", + wantErr: "failed to check if ServiceAccount", wantExistsCount: 1, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "patch error propagates", @@ -159,13 +159,13 @@ func TestServiceAccountReconciliation(t *testing.T) { m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { return false, nil }) - m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { return errTestClient }) }, - wantErr: "failed to apply serviceaccount", + wantErr: "failed to apply ServiceAccount", wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, } @@ -189,8 +189,8 @@ func TestServiceAccountReconciliation(t *testing.T) { if got := mock.ExistsCallCount(); got != tt.wantExistsCount { t.Errorf("expected %d Exists calls, got %d", tt.wantExistsCount, got) } - if got := mock.PatchCallCount(); got != tt.wantPatchCount { - t.Errorf("expected %d Patch calls, got %d", tt.wantPatchCount, got) + if got := mock.ApplyCallCount(); got != tt.wantApplyCount { + t.Errorf("expected %d Apply calls, got %d", tt.wantApplyCount, got) } }) } diff --git a/pkg/controller/trustmanager/services.go b/pkg/controller/trustmanager/services.go index 85c28294e..7ab98f84a 100644 --- a/pkg/controller/trustmanager/services.go +++ b/pkg/controller/trustmanager/services.go @@ -1,12 +1,10 @@ package trustmanager import ( - "fmt" "maps" "reflect" corev1 "k8s.io/api/core/v1" - "sigs.k8s.io/controller-runtime/pkg/client" "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" "github.com/openshift/cert-manager-operator/pkg/controller/common" @@ -24,29 +22,9 @@ func (r *Reconciler) createOrApplyServices(trustManager *v1alpha1.TrustManager, } func (r *Reconciler) createOrApplyService(trustManager *v1alpha1.TrustManager, desired *corev1.Service) error { - serviceName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling service resource", "name", serviceName) - - existing := &corev1.Service{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if service %q exists", serviceName) - } - if exists && !serviceModified(desired, existing) { - r.log.V(4).Info("service resource exists and is in desired state", "name", serviceName) - return nil - } - - r.log.V(2).Info("service resource has been modified, updating to desired state", "name", serviceName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply service %q", serviceName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "service resource %s applied", serviceName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &corev1.Service{}, fieldOwner, serviceModified) } -// serviceModified compares only the fields we manage via SSA. func serviceModified(desired, existing *corev1.Service) bool { if managedMetadataModified(desired, existing) { return true diff --git a/pkg/controller/trustmanager/services_test.go b/pkg/controller/trustmanager/services_test.go index 4c7432370..a4ac5a87c 100644 --- a/pkg/controller/trustmanager/services_test.go +++ b/pkg/controller/trustmanager/services_test.go @@ -87,7 +87,7 @@ func TestServiceReconciliation(t *testing.T) { preReq func(*Reconciler, *fakes.FakeCtrlClient) wantErr string wantExistsCount int - wantPatchCount int + wantApplyCount int }{ { name: "successful apply of both services", @@ -97,7 +97,7 @@ func TestServiceReconciliation(t *testing.T) { }) }, wantExistsCount: 2, - wantPatchCount: 2, + wantApplyCount: 2, }, { name: "skip apply when both services match desired", @@ -116,7 +116,7 @@ func TestServiceReconciliation(t *testing.T) { }) }, wantExistsCount: 2, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "apply when existing has label drift", @@ -136,7 +136,7 @@ func TestServiceReconciliation(t *testing.T) { }) }, wantExistsCount: 2, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when existing has annotation drift", @@ -160,7 +160,7 @@ func TestServiceReconciliation(t *testing.T) { }) }, wantExistsCount: 2, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when existing has port drift", @@ -180,7 +180,7 @@ func TestServiceReconciliation(t *testing.T) { }) }, wantExistsCount: 2, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when existing has selector drift", @@ -200,7 +200,7 @@ func TestServiceReconciliation(t *testing.T) { }) }, wantExistsCount: 2, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "exists error propagates on first service", @@ -209,9 +209,9 @@ func TestServiceReconciliation(t *testing.T) { return false, errTestClient }) }, - wantErr: "failed to check if service", + wantErr: "failed to check if Service", wantExistsCount: 1, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "webhook service patch error propagates", @@ -219,13 +219,13 @@ func TestServiceReconciliation(t *testing.T) { m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { return false, nil }) - m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { return errTestClient }) }, - wantErr: "failed to apply service", + wantErr: "failed to apply Service", wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "metrics service patch error propagates on second call", @@ -234,7 +234,7 @@ func TestServiceReconciliation(t *testing.T) { return false, nil }) callCount := 0 - m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { callCount++ if callCount == 2 { return errTestClient @@ -242,9 +242,9 @@ func TestServiceReconciliation(t *testing.T) { return nil }) }, - wantErr: "failed to apply service", + wantErr: "failed to apply Service", wantExistsCount: 2, - wantPatchCount: 2, + wantApplyCount: 2, }, } @@ -268,8 +268,8 @@ func TestServiceReconciliation(t *testing.T) { if got := mock.ExistsCallCount(); got != tt.wantExistsCount { t.Errorf("expected %d Exists calls, got %d", tt.wantExistsCount, got) } - if got := mock.PatchCallCount(); got != tt.wantPatchCount { - t.Errorf("expected %d Patch calls, got %d", tt.wantPatchCount, got) + if got := mock.ApplyCallCount(); got != tt.wantApplyCount { + t.Errorf("expected %d Apply calls, got %d", tt.wantApplyCount, got) } }) } diff --git a/pkg/controller/trustmanager/webhooks.go b/pkg/controller/trustmanager/webhooks.go index 9c29d3a51..d2dae228b 100644 --- a/pkg/controller/trustmanager/webhooks.go +++ b/pkg/controller/trustmanager/webhooks.go @@ -6,9 +6,7 @@ import ( "reflect" "slices" - corev1 "k8s.io/api/core/v1" "k8s.io/utils/ptr" - "sigs.k8s.io/controller-runtime/pkg/client" "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" "github.com/openshift/cert-manager-operator/pkg/controller/common" @@ -19,26 +17,7 @@ import ( func (r *Reconciler) createOrApplyValidatingWebhookConfiguration(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string) error { desired := getValidatingWebhookConfigObject(resourceLabels, resourceAnnotations) - resourceName := desired.GetName() - r.log.V(4).Info("reconciling validatingwebhookconfiguration resource", "name", resourceName) - - existing := &admissionregistrationv1.ValidatingWebhookConfiguration{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if validatingwebhookconfiguration %q exists", resourceName) - } - if exists && !webhookConfigModified(desired, existing) { - r.log.V(4).Info("validatingwebhookconfiguration resource exists and is in desired state", "name", resourceName) - return nil - } - - r.log.V(2).Info("validatingwebhookconfiguration resource has been modified, updating to desired state", "name", resourceName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply validatingwebhookconfiguration %q", resourceName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "validatingwebhookconfiguration resource %s applied", resourceName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &admissionregistrationv1.ValidatingWebhookConfiguration{}, fieldOwner, webhookConfigModified) } func getValidatingWebhookConfigObject(resourceLabels, resourceAnnotations map[string]string) *admissionregistrationv1.ValidatingWebhookConfiguration { @@ -52,7 +31,6 @@ func getValidatingWebhookConfigObject(resourceLabels, resourceAnnotations map[st return webhookConfig } -// updateWebhookClientConfig sets the webhook clientConfig service name and namespace. func updateWebhookClientConfig(webhookConfig *admissionregistrationv1.ValidatingWebhookConfiguration) { for i := range webhookConfig.Webhooks { if webhookConfig.Webhooks[i].ClientConfig.Service != nil { @@ -73,9 +51,6 @@ func updateWebhookAnnotations(webhookConfig *admissionregistrationv1.ValidatingW webhookConfig.SetAnnotations(mergedAnnotations) } -// webhookConfigModified compares only the fields we manage via SSA. -// Individual webhook fields are compared explicitly because the API server -// defaults fields like matchPolicy, namespaceSelector, and objectSelector. func webhookConfigModified(desired, existing *admissionregistrationv1.ValidatingWebhookConfiguration) bool { if managedMetadataModified(desired, existing) { return true diff --git a/pkg/controller/trustmanager/webhooks_test.go b/pkg/controller/trustmanager/webhooks_test.go index c69ed52df..5c2be9863 100644 --- a/pkg/controller/trustmanager/webhooks_test.go +++ b/pkg/controller/trustmanager/webhooks_test.go @@ -117,7 +117,7 @@ func TestValidatingWebhookConfigReconciliation(t *testing.T) { preReq func(*Reconciler, *fakes.FakeCtrlClient) wantErr string wantExistsCount int - wantPatchCount int + wantApplyCount int }{ { name: "successful apply when not found", @@ -127,7 +127,7 @@ func TestValidatingWebhookConfigReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "skip apply when existing matches desired", @@ -139,7 +139,7 @@ func TestValidatingWebhookConfigReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "apply when existing has label drift", @@ -152,7 +152,7 @@ func TestValidatingWebhookConfigReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when existing has annotation drift", @@ -169,7 +169,7 @@ func TestValidatingWebhookConfigReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when existing has service reference drift", @@ -182,7 +182,7 @@ func TestValidatingWebhookConfigReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when existing has failure policy drift", @@ -195,7 +195,7 @@ func TestValidatingWebhookConfigReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when existing has rules drift", @@ -217,7 +217,7 @@ func TestValidatingWebhookConfigReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "apply when existing has admission review versions drift", @@ -230,7 +230,7 @@ func TestValidatingWebhookConfigReconciliation(t *testing.T) { }) }, wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, { name: "exists error propagates", @@ -239,9 +239,9 @@ func TestValidatingWebhookConfigReconciliation(t *testing.T) { return false, errTestClient }) }, - wantErr: "failed to check if validatingwebhookconfiguration", + wantErr: "failed to check if ValidatingWebhookConfiguration", wantExistsCount: 1, - wantPatchCount: 0, + wantApplyCount: 0, }, { name: "patch error propagates", @@ -249,13 +249,13 @@ func TestValidatingWebhookConfigReconciliation(t *testing.T) { m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { return false, nil }) - m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + m.ApplyCalls(func(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { return errTestClient }) }, - wantErr: "failed to apply validatingwebhookconfiguration", + wantErr: "failed to apply ValidatingWebhookConfiguration", wantExistsCount: 1, - wantPatchCount: 1, + wantApplyCount: 1, }, } @@ -279,8 +279,8 @@ func TestValidatingWebhookConfigReconciliation(t *testing.T) { if got := mock.ExistsCallCount(); got != tt.wantExistsCount { t.Errorf("expected %d Exists calls, got %d", tt.wantExistsCount, got) } - if got := mock.PatchCallCount(); got != tt.wantPatchCount { - t.Errorf("expected %d Patch calls, got %d", tt.wantPatchCount, got) + if got := mock.ApplyCallCount(); got != tt.wantApplyCount { + t.Errorf("expected %d Apply calls, got %d", tt.wantApplyCount, got) } }) } diff --git a/test/apis/applyresource_test.go b/test/apis/applyresource_test.go new file mode 100644 index 000000000..3534bd5d5 --- /dev/null +++ b/test/apis/applyresource_test.go @@ -0,0 +1,222 @@ +package apis + +import ( + "context" + "maps" + "reflect" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client" + logf "sigs.k8s.io/controller-runtime/pkg/log" + + "github.com/openshift/cert-manager-operator/pkg/controller/common" +) + +// envtestCtrlClient adapts the suite's client to the common.CtrlClient +// interface the reconcilers use. +type envtestCtrlClient struct { + client.Client +} + +func (c *envtestCtrlClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object) error { + return c.Client.Get(ctx, key, obj) +} + +func (c *envtestCtrlClient) UpdateWithRetry(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { + return c.Client.Update(ctx, obj, opts...) +} + +func (c *envtestCtrlClient) StatusUpdate(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { + return c.Client.Status().Update(ctx, obj, opts...) +} + +// Apply mirrors common.ctrlClientImpl.Apply: it converts the typed object to +// an unstructured apply configuration and copies the server response back. +func (c *envtestCtrlClient) Apply(ctx context.Context, obj client.Object, opts ...client.ApplyOption) error { + gvk, err := c.GroupVersionKindFor(obj) + if err != nil { + return err + } + content, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj) + if err != nil { + return err + } + u := &unstructured.Unstructured{Object: content} + u.SetGroupVersionKind(gvk) + if err := c.Client.Apply(ctx, client.ApplyConfigurationFromUnstructured(u), opts...); err != nil { + return err + } + return runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, obj) +} + +func (c *envtestCtrlClient) Exists(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { + if err := c.Client.Get(ctx, key, obj); err != nil { + if apierrors.IsNotFound(err) { + return false, nil + } + return false, err + } + return true, nil +} + +// Regression tests for ApplyResource drift correction. +// +// 1. The ISTIOCSR-018 e2e failure ("should reconcile gRPC service drift"): a +// Service port drifted by a client-side Update could not be corrected by +// re-applying it. Service spec.ports is a granular list keyed by port and +// protocol; once the e2e test changed the port, the operator's SSA apply of +// the desired entry was treated as adding a second entry and was rejected +// with `spec.ports[1].name: Duplicate value: "web"` on every reconcile. +// ApplyResource must fall back to a JSON merge patch when the SSA apply +// fails, which replaces lists wholesale. +// +// 2. The tech-preview e2e failure ("should reconcile deployment when default +// CA package policy transitions to Disabled"): an annotation this manager +// previously applied but no longer desires must be pruned when drift is +// corrected, matching plain SSA behavior. A JSON-merge-only drift +// correction merges annotation maps key-by-key and would leave the stale +// annotation behind. +var _ = Describe("ApplyResource drift correction", Ordered, func() { + var ( + ctrlClient *envtestCtrlClient + recorder *record.FakeRecorder + ns *corev1.Namespace + ) + + hasChanged := func(desired, existing *corev1.Service) bool { + return !reflect.DeepEqual(desired.Spec.Ports, existing.Spec.Ports) || + common.ObjectMetadataModified(desired, existing) || + !maps.Equal(desired.GetAnnotations(), existing.GetAnnotations()) + } + + desiredService := func() *corev1.Service { + return &corev1.Service{ + // TypeMeta is populated by the YAML decoder in the real operator + // (asset manifests carry apiVersion/kind). + TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Service"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "cert-manager-istio-csr", + Namespace: ns.Name, + Labels: map[string]string{ + "app": "cert-manager-istio-csr", + "app.kubernetes.io/name": "cert-manager-istio-csr", + "app.kubernetes.io/instance": "cert-manager-istio-csr", + "app.kubernetes.io/version": "v0.16.0", + "app.kubernetes.io/managed-by": "cert-manager-operator", + }, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + Ports: []corev1.ServicePort{{ + Name: "web", + Port: 443, + TargetPort: intstr.FromInt32(6443), + Protocol: corev1.ProtocolTCP, + }}, + Selector: map[string]string{"app": "cert-manager-istio-csr"}, + }, + } + } + + apply := func(desired *corev1.Service) error { + return common.ApplyResource( + ctx, ctrlClient, logf.Log, recorder, + ns, desired, &corev1.Service{}, "istio-csr-controller", hasChanged, + ) + } + + BeforeAll(func() { + ctrlClient = &envtestCtrlClient{Client: k8sClient} + recorder = record.NewFakeRecorder(10) + + ns = &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{GenerateName: "applyresource-test-"}} + Expect(k8sClient.Create(ctx, ns)).To(Succeed()) + }) + + AfterAll(func() { + Expect(k8sClient.Delete(ctx, ns)).To(Succeed()) + }) + + It("creates the service via server-side apply", func() { + Expect(apply(desiredService())).To(Succeed()) + + live := &corev1.Service{} + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(desiredService()), live)).To(Succeed()) + Expect(live.Spec.Ports).To(HaveLen(1)) + Expect(live.Spec.Ports[0].Port).To(Equal(int32(443))) + Expect(live.Spec.ClusterIP).NotTo(BeEmpty(), "apiserver must assign a clusterIP") + }) + + It("reverts a drifted service port", func() { + // Drift the port the way the e2e test does: client-side Update. + live := &corev1.Service{} + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(desiredService()), live)).To(Succeed()) + live.Spec.Ports[0].Port = 9443 + Expect(k8sClient.Update(ctx, live, client.FieldOwner("e2e-test"))).To(Succeed()) + + Expect(apply(desiredService())).To(Succeed()) + + corrected := &corev1.Service{} + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(desiredService()), corrected)).To(Succeed()) + Expect(corrected.Spec.Ports).To(HaveLen(1), "drifted entry must be replaced, not appended") + Expect(corrected.Spec.Ports[0].Port).To(Equal(int32(443))) + Expect(corrected.Spec.ClusterIP).To(Equal(live.Spec.ClusterIP), "merge patch must preserve assigned clusterIP") + }) + + It("restores a drifted label value", func() { + live := &corev1.Service{} + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(desiredService()), live)).To(Succeed()) + live.Labels["app.kubernetes.io/instance"] = "modified-value" + Expect(k8sClient.Update(ctx, live, client.FieldOwner("e2e-test"))).To(Succeed()) + + Expect(apply(desiredService())).To(Succeed()) + + corrected := &corev1.Service{} + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(desiredService()), corrected)).To(Succeed()) + Expect(corrected.Labels["app.kubernetes.io/instance"]).To(Equal("cert-manager-istio-csr")) + }) + + It("prunes an annotation the manager no longer desires", func() { + // Apply a variant carrying the default CA package hash annotation, + // the way the operator does while the feature is enabled. + annotated := desiredService() + annotated.Annotations = map[string]string{ + "operator.openshift.io/default-ca-package-hash": "hash-1", + } + Expect(apply(annotated)).To(Succeed()) + + present := &corev1.Service{} + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(annotated), present)).To(Succeed()) + Expect(present.Annotations).To(HaveKeyWithValue("operator.openshift.io/default-ca-package-hash", "hash-1")) + + // Re-applying the plain desired object (feature disabled) must prune + // the annotation, matching plain SSA behavior. A JSON merge patch + // would merge the annotation maps and leave it behind. + Expect(apply(desiredService())).To(Succeed()) + + corrected := &corev1.Service{} + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(annotated), corrected)).To(Succeed()) + Expect(corrected.Annotations).NotTo(HaveKey("operator.openshift.io/default-ca-package-hash")) + Expect(corrected.Spec.Ports).To(HaveLen(1)) + Expect(corrected.Spec.Ports[0].Port).To(Equal(int32(443))) + }) + + It("is a no-op when already in the desired state", func() { + Expect(apply(desiredService())).To(Succeed()) + Expect(apply(desiredService())).To(Succeed()) + + // The two applies above must not have emitted events; the recorder + // only holds the five events from the mutations in the earlier specs. + Expect(recorder.Events).To(HaveLen(5), + "one event per mutation: create, port drift, label drift, annotation add, annotation prune") + }) +})