From 5d36c30d3e9af64f08d852224d4e3365c1094d06 Mon Sep 17 00:00:00 2001 From: Arun Maurya Date: Fri, 31 Jul 2026 13:22:34 +0530 Subject: [PATCH 1/3] Add agentic docs and coding guidelines for AI-assisted development. Provide AGENTS.md, ai-docs architecture/domain/ADRs, and deep-dive guideline playbooks so agents and contributors can follow operator-specific patterns without rediscovering them from code. --- AGENTS.md | 80 ++++++++ ai-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md | 81 ++++++++ ai-docs/CERT_MANAGER_OPERATOR_TESTING.md | 59 ++++++ ai-docs/architecture/components.md | 174 ++++++++++++++++ .../adr-0001-dual-controller-frameworks.md | 41 ++++ .../decisions/adr-0002-apply-strategies.md | 43 ++++ ai-docs/decisions/adr-0003-feature-gates.md | 55 +++++ ai-docs/decisions/adr-template.md | 30 +++ ai-docs/domain/certmanager.md | 83 ++++++++ ai-docs/domain/istiocsr.md | 83 ++++++++ ai-docs/domain/trustmanager.md | 85 ++++++++ ai-docs/exec-plans/README.md | 15 ++ ai-docs/references/ecosystem.md | 76 +++++++ ai-docs/references/enhancements.md | 27 +++ docs/api-contracts-guidelines.md | 179 +++++++++++++++++ docs/error-handling-guidelines.md | 167 ++++++++++++++++ docs/fips-guidelines.md | 173 ++++++++++++++++ docs/integration-guidelines.md | 73 +++++++ docs/olm-packaging-guidelines.md | 186 +++++++++++++++++ docs/operator-controllers-guidelines.md | 188 ++++++++++++++++++ docs/security-guidelines.md | 160 +++++++++++++++ docs/testing-guidelines.md | 161 +++++++++++++++ 22 files changed, 2219 insertions(+) create mode 100644 AGENTS.md create mode 100644 ai-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md create mode 100644 ai-docs/CERT_MANAGER_OPERATOR_TESTING.md create mode 100644 ai-docs/architecture/components.md create mode 100644 ai-docs/decisions/adr-0001-dual-controller-frameworks.md create mode 100644 ai-docs/decisions/adr-0002-apply-strategies.md create mode 100644 ai-docs/decisions/adr-0003-feature-gates.md create mode 100644 ai-docs/decisions/adr-template.md create mode 100644 ai-docs/domain/certmanager.md create mode 100644 ai-docs/domain/istiocsr.md create mode 100644 ai-docs/domain/trustmanager.md create mode 100644 ai-docs/exec-plans/README.md create mode 100644 ai-docs/references/ecosystem.md create mode 100644 ai-docs/references/enhancements.md create mode 100644 docs/api-contracts-guidelines.md create mode 100644 docs/error-handling-guidelines.md create mode 100644 docs/fips-guidelines.md create mode 100644 docs/integration-guidelines.md create mode 100644 docs/olm-packaging-guidelines.md create mode 100644 docs/operator-controllers-guidelines.md create mode 100644 docs/security-guidelines.md create mode 100644 docs/testing-guidelines.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 000000000..85b132506 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,80 @@ +# Cert Manager Operator - Agentic Documentation + +**Component**: Cert Manager Operator for OpenShift +**Repository**: [openshift/cert-manager-operator](https://github.com/openshift/cert-manager-operator) +**Default branch**: `master` | **Go**: 1.26.0 + +> **Retrieval-first**: Prefer `ai-docs/` for architecture/API; prefer `docs/*-guidelines.md` for deep playbooks. Platform hub: [openshift/enhancements/ai-docs](https://github.com/openshift/enhancements/tree/master/ai-docs). + +## What is Cert Manager Operator? + +Deploys/configures upstream **cert-manager** (and optionally **istio-csr** / **trust-manager**) on OpenShift. Operator NS `cert-manager-operator`; operand NS `cert-manager` (hardcoded). + +**Key Principle**: Dual-stack — library-go for CertManager; controller-runtime for IstioCSR/TrustManager. **Apply methods differ** (do not assume SSA everywhere). + +## Core Components + +| CR | Scope / name | Stack | Gate | +|----|--------------|-------|------| +| [CertManager](ai-docs/domain/certmanager.md) | Cluster / `cluster` | library-go `resourceapply` | Always on | +| [IstioCSR](ai-docs/domain/istiocsr.md) | Namespaced / `default` | Create+Update | `IstioCSR` GA default true | +| [TrustManager](ai-docs/domain/trustmanager.md) | Cluster / `cluster` | **SSA** | `TrustManager` TP default false | + +**Operands**: Certificate/Issuer/… → cert-manager; Bundle → trust-manager. +**Quick Start**: `oc get certmanager cluster -o yaml` | `make local-run` + +## Critical Patterns + +1. **Never assume uniform SSA** — CertManager library-go; IstioCSR Create+`UpdateWithRetry`; TrustManager only SSA + field owner `trust-manager-controller`. [components.md](ai-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](ai-docs/decisions/adr-0002-apply-strategies.md) +4. **Feature gates** — `--unsupported-addon-features`; five touchpoints; no cluster FeatureSet for TrustManager (CM-1141). [ADR-0003](ai-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`. +7. **Cache** — no label-filtered ConfigMap/Issuer caches; use predicates when unlabeled watches are required. +8. **TLS 1.3** — strip cipher args via `StripArgsByKeys`; nil profile = Intermediate; Infrastructure discovery gates TLS + cloud-cred hooks (APIServer shares that factory, not discovered separately). + +## Key Paths + +| Area | Path | +|------|------| +| Startup | `pkg/operator/starter.go` → CertManager set → optional ctrl-runtime manager | +| Shared | `pkg/controller/common` (`CtrlClient`, errors, TLS/validation) | +| Features | `api/operator/v1alpha1/features.go`, `pkg/features` | +| Images | `RELATED_IMAGE_CERT_MANAGER_{CONTROLLER,WEBHOOK,CA_INJECTOR,ACMESOLVER,ISTIOCSR,TRUST_MANAGER}` | +| ADRs | [0001](ai-docs/decisions/adr-0001-dual-controller-frameworks.md) · [0002](ai-docs/decisions/adr-0002-apply-strategies.md) · [0003](ai-docs/decisions/adr-0003-feature-gates.md) | + +## Documentation Map + +```text +ai-docs/ # architecture, domain CRDs, ADRs, DEVELOPMENT, TESTING +docs/*-guidelines.md # deep agent playbooks (controllers, errors, OLM, FIPS, …) +docs/{proxy,cloud_credentials,operand_metrics}.md +``` + +| Need | Start here | +|------|------------| +| Spec / CEL / NetworkPolicy enum | `ai-docs/domain/*.md` | +| Errors, cache, TLS, cloud, FIPS | `ai-docs/architecture/components.md` | +| Controllers / apply / gates | `docs/operator-controllers-guidelines.md` | +| OLM / relatedImages | `docs/olm-packaging-guidelines.md` + DEVELOPMENT | +| Unit assert Patch vs Update | `ai-docs/CERT_MANAGER_OPERATOR_TESTING.md` | +| FIPS build rules | `docs/fips-guidelines.md` | + +**AI Agent Path**: domain → components.md → matching `docs/*-guidelines.md` → DEVELOPMENT/TESTING + +**Also**: [DEVELOPMENT](ai-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md) · [TESTING](ai-docs/CERT_MANAGER_OPERATOR_TESTING.md) · [enhancements](ai-docs/references/enhancements.md) · [ecosystem](ai-docs/references/ecosystem.md) + +**Guideline index**: `docs/{operator-controllers,error-handling,api-contracts,olm-packaging,integration,security,testing,fips}-guidelines.md` + +**Platform**: [hub](https://github.com/openshift/enhancements/tree/master/ai-docs) · [operator-patterns](https://github.com/openshift/enhancements/tree/master/ai-docs/platform/operator-patterns) · [status-conditions](https://github.com/openshift/enhancements/blob/master/ai-docs/platform/operator-patterns/status-conditions.md) + +## External References + +- [Product docs](https://docs.openshift.com/container-platform/latest/security/cert_manager_operator/index.html) +- [Upstream cert-manager](https://cert-manager.io/docs/) +- [README](README.md) · [docs/proxy.md](docs/proxy.md) · [docs/cloud_credentials.md](docs/cloud_credentials.md) · [docs/operand_metrics.md](docs/operand_metrics.md) + +--- + +**Platform Documentation**: [openshift/enhancements/ai-docs](https://github.com/openshift/enhancements/tree/master/ai-docs) diff --git a/ai-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md b/ai-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md new file mode 100644 index 000000000..8aaa5a63b --- /dev/null +++ b/ai-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md @@ -0,0 +1,81 @@ +# Cert Manager Operator - Development Guide + +> Layout / controller comparison: [architecture/components.md](./architecture/components.md). +> Agent playbooks: [docs/*-guidelines.md](../docs/) (operator-controllers, olm-packaging, fips, integration, …). + +## Quick Start + +- Go **1.26.0+** (`go.mod`), OpenShift + `KUBECONFIG`, container engine + +```bash +make deploy +oc scale --replicas=0 deploy --all -n cert-manager-operator +make local-run # RELATED_IMAGE_* + versions from Makefile +make build && make generate && make update-manifests && make verify +``` + +FIPS: `make build` sources `hack/go-fips.sh`. WARN (non-FIPS) builds are **local-only** — never CI/prod/image push. Keep `go.mod` replace `openshift/jetstack-cert-manager` lockstep with `CERT_MANAGER_VERSION`. See [docs/fips-guidelines.md](../docs/fips-guidelines.md). + +## Common Tasks + +### CertManager deployment overrides + +1. Spec change → `certmanager_types.go` → `make manifests generate` +2. Wire in `deployment_*.go` / `deployment_overrides.go` (library-go — **not** TrustManager SSA) + +### New controller-runtime operand (greenfield) + +1. Copy **TrustManager** (SSA `Patch` + 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) +5. Reuse `common.HandleReconcileResult` / `FromClientError` / validation helpers (`defaultRequeueTime=30s`) +6. E2E + Ginkgo labels under `test/e2e/` + +### RELATED_IMAGE / relatedImages sync + +| CSV `relatedImages.name` | Env | Consumer | +|--------------------------|-----|----------| +| `cert-manager-controller` / webhook / ca-injector / acmesolver | `RELATED_IMAGE_CERT_MANAGER_*` | `related_images.go` | +| `cert-manager-istiocsr` | `RELATED_IMAGE_CERT_MANAGER_ISTIOCSR` | `istiocsr/constants.go` | +| `cert-manager-trust-manager` | `RELATED_IMAGE_CERT_MANAGER_TRUST_MANAGER` | `trustmanager/constants.go` | + +`RELATED_IMAGE_*` / `*_OPERAND_IMAGE_VERSION` literals in **`config/manager/manager.yaml` are hand-maintained** — bumping Makefile `CERT_*_VERSION` alone does **not** update them. Edit `manager.yaml` to match, then `make bundle` (auto-fills CSV `relatedImages` from `RELATED_IMAGE_*` env). New operand image also needs controller constants/map. See [docs/olm-packaging-guidelines.md](../docs/olm-packaging-guidelines.md). + +### Bump operand versions + +1. Makefile `CERT_MANAGER_VERSION` / `ISTIO_CSR_VERSION` / `TRUST_MANAGER_VERSION` (+ bundle version) +2. `make update-manifests` + keep jetstack replace version lockstep +3. **Manually** update matching `RELATED_IMAGE_*` / `*_OPERAND_IMAGE_VERSION` in `config/manager/manager.yaml` +4. `make bundle`; refresh CSV description links / RBAC if upstream changed; run `hack/verify-crds*.sh` directly if CRDs changed (not in `make verify-scripts`) + +### Enable TrustManager locally + +`--unsupported-addon-features=TrustManager=true`; create `TrustManager` named `cluster`. + +## Common Mistakes + +1. Hand-edit bindata / generated clients / generated `bundle/` CSV +2. Assume SSA for IstioCSR or CertManager +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) +6. TLS 1.3 cipher-suite args (use `StripArgsByKeys`) +7. Create CredentialsRequest in-operator (mount-only; controller Deployment only) +8. Point cert-manager replace at upstream or ship non-FIPS image + +## Component-Specific Notes + +| Topic | Detail | +|-------|--------| +| Namespaces | Operator `cert-manager-operator`; operands `cert-manager` | +| Cloud creds | AWS `/.aws` + `AWS_SDK_LOAD_CONFIG=1`; GCP ADC path — `docs/cloud_credentials.md` | +| Trusted CA | Fixed mount path; missing CM = retryable — `docs/integration-guidelines.md` | +| Uninstall | Manual operand cleanup; `console.openshift.io/disable-operand-delete: "true"` | + +## See Also + +- [CERT_MANAGER_OPERATOR_TESTING.md](./CERT_MANAGER_OPERATOR_TESTING.md) +- [architecture/components.md](./architecture/components.md) +- [docs/operator-controllers-guidelines.md](../docs/operator-controllers-guidelines.md) +- [docs/olm-packaging-guidelines.md](../docs/olm-packaging-guidelines.md) diff --git a/ai-docs/CERT_MANAGER_OPERATOR_TESTING.md b/ai-docs/CERT_MANAGER_OPERATOR_TESTING.md new file mode 100644 index 000000000..d4612382c --- /dev/null +++ b/ai-docs/CERT_MANAGER_OPERATOR_TESTING.md @@ -0,0 +1,59 @@ +# Cert Manager Operator - Testing Guide + +> **Generic Testing Practices**: See [Platform ai-docs](https://github.com/openshift/enhancements/tree/master/ai-docs). Deep playbook: [docs/testing-guidelines.md](../docs/testing-guidelines.md). + +## Test Layout + +| Suite | Location | How to run | +|-------|----------|------------| +| Unit | `pkg/**/*_test.go` | `make test-unit` | +| API / envtest | `test/apis`, `api/operator/v1alpha1/tests/**` | `make test-apis` | +| E2E | `test/e2e/` (build tag `e2e`, Ginkgo) | `make test-e2e` | + +`make test` = manifests + generate + vet + test-apis + test-unit — **not** e2e. + +## Unit Tests — assert the real apply path + +| 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 | + +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/`. + +```bash +make test-unit +go test -count=1 ./pkg/controller/trustmanager/... +go test -count=1 ./pkg/controller/istiocsr/... +go test -count=1 ./pkg/controller/certmanager/... +``` + +## API Tests (`make test-apis`) + +envtest + Ginkgo against CEL/`XValidation` fixtures under `api/operator/v1alpha1/tests/{certmanagers,istiocsrs,trustmanagers}.operator.openshift.io/`. Required after Spec validation changes — unit tests cannot enforce CEL. + +## E2E Tests + +```bash +make test-e2e +# Default label filter (Makefile) — always single-quote overrides: +# Platform: isSubsetOf {AWS,Generic} && CredentialsMode: isSubsetOf {Mint} && !Feature:ServiceMesh +make test-e2e E2E_GINKGO_LABEL_FILTER='Feature:TrustManager && !TechPreview:Inverted' +``` + +| Area | Labels | +|------|--------| +| TrustManager | `Feature:TrustManager`, `TechPreview` | +| Gate-disabled TrustManager | `TechPreview:Inverted` (do **not** merge with enabled suite) | +| TLS profile | `Feature:TLSProfile`, `TechPreview` | +| IstioCSR | `Feature:IstioCSR` | +| ServiceMesh smoke | `Feature:IstioCSR-ServiceMesh` (excluded by default `!Feature:ServiceMesh`) | + +Tips: singleton CR names (`cluster` / `default`); operands in `cert-manager` NS; `make test-e2e-debug-cluster` on failure. + +## See Also + +- [docs/testing-guidelines.md](../docs/testing-guidelines.md) +- [CERT_MANAGER_OPERATOR_DEVELOPMENT.md](./CERT_MANAGER_OPERATOR_DEVELOPMENT.md) +- [architecture/components.md](./architecture/components.md) diff --git a/ai-docs/architecture/components.md b/ai-docs/architecture/components.md new file mode 100644 index 000000000..84544d57c --- /dev/null +++ b/ai-docs/architecture/components.md @@ -0,0 +1,174 @@ +# Architecture — Cert Manager Operator + +Dual-stack OpenShift operator: **library-go** for core CertManager operand; **controller-runtime** manager for feature-gated IstioCSR and TrustManager. + +## Repo Layout (source of truth) + +```text +api/operator/v1alpha1/ # CertManager, IstioCSR, TrustManager types + feature gates +bindata/ # Operand YAML (cert-manager, istio-csr, trust-manager, networkpolicies) — regenerated +bundle/ # OLM CSV + CRDs +config/ # Kustomize sources for manifests/bundle +docs/ # Human docs + *-guidelines.md (agent playbooks) +hack/ # update-*-manifests, go-fips, clientgen, verify scripts +pkg/ + cmd/operator/ # Cobra start + flags + controller/ + certmanager/ # library-go controller set (always on) + istiocsr/ # ctrl-runtime Create+Update + trustmanager/ # ctrl-runtime SSA (reference) + common/ # CtrlClient, ReconcileError, HandleReconcileResult, validation, TLS hook + features/ # --unsupported-addon-features + FeatureGateState + operator/ # RunOperator, setup_manager, bindata.go, generated clients + tlsprofile/ # APIServer TLS profile → operand args +test/{apis,e2e}/ # envtest CRD suites + Ginkgo e2e +``` + +**DO NOT** hand-edit: `pkg/operator/assets/bindata.go`, `pkg/operator/{clientset,informers,listers,applyconfigurations}/**`, regenerated bindata YAML. Use `make generate`, `make update-manifests`, `make verify-bindata`. + +**DO NOT** start from `pkg/controller/certmanager/certmanager_controller.go` — unused placeholder for RBAC annotations (“Needs to be deleted later”). + +## Startup Sequence + +`main.go` → `pkg/cmd/operator` → `controllercmd` → `operator.RunOperator` (`pkg/operator/starter.go`): + +1. Clients + informer factories + `OperatorClient` (+ optional Infrastructure discovery) +2. **Construct** `NewCertManagerControllerSet` + `NewDefaultCertManagerController` (controllers not running yet) +3. **Start informers** (`informer.Start`, including optional infra factory when `Applicable()`) +4. **Run** each library-go controller (`go controller.Run`) +5. `setupFeatureGates` from `--unsupported-addon-features` + optional `featuregates/cluster` read (fail-closed retries; does not abort operator on persistent FeatureGate errors) +6. If IstioCSR and/or TrustManager enabled → `NewControllerManager` → Start in goroutine +7. Block on `ctx.Done()` + +Flags (`pkg/cmd/operator/cmd.go`): `--trusted-ca-configmap`, `--cloud-credentials-secret`, `--unsupported-addon-features`. + +Note: Informers start **before** library-go controllers run — do not assume first reconcile races ahead of cache sync. + +## Controller Comparison (critical) + +| Dimension | CertManager | IstioCSR | TrustManager | +|-----------|-------------|----------|--------------| +| 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` | +| 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`). + +**Greenfield rule**: New controller-runtime operands → copy **TrustManager**, not IstioCSR. + +## CertManager Controller Set + +Wired in `cert_manager_controller_set.go` + `starter.go`: + +1. Controller static resources + deployment +2. Webhook static resources + deployment +3. CAInjector static resources + deployment +4. NetworkPolicy static + user-defined +5. DefaultCertManager + +Shared deployment hooks (`deployment_overrides.go`, `generic_deployment_controller.go`): proxy env (`operator-lib/proxy`), trusted-CA volume, optional cloud-credentials mount, SA bound token, cluster TLS profile args — **only when** the optional Infrastructure informer factory is `Applicable()` (APIServer shares it; absence ⇒ skip those hooks, don’t crash). + +## Feature Gates + +Defined in `api/operator/v1alpha1/features.go`; registered in `pkg/features`. + +| Gate | Default | PreRelease | Runtime check | +|------|---------|------------|---------------| +| `IstioCSR` | true | GA | `IsIstioCSRFeatureGateEnabled()` — internal only | +| `TrustManager` | false | TechPreview | `FeatureGateState.IsTrustManagerFeatureGateEnabled()` — internal only; cluster FeatureSet gating **removed** (CM-1141) | + +`passesClusterPreviewGating` still exists in `features.go` but is **unused** — do not resurrect without an ADR. + +**Wire a new gate (all five)**: (1) `features.go` Default+PreRelease → (2) `pkg/features` → (3) `starter.go` `--unsupported-addon-features` → (4) runtime `Is*Enabled` → (5) `setup_manager.go` cache + reconciler. CRDs ship unconditionally; only the controller is gated. + +## Image Resolution + +Makefile versions: `CERT_MANAGER_VERSION` (v1.20.3), `ISTIO_CSR_VERSION` (v0.16.0), `TRUST_MANAGER_VERSION` (v0.20.3). + +OLM injects env (CSV / `config/manager/manager.yaml`): + +- `RELATED_IMAGE_CERT_MANAGER_{WEBHOOK,CA_INJECTOR,CONTROLLER,ACMESOLVER,ISTIOCSR,TRUST_MANAGER}` +- `OPERAND_IMAGE_VERSION`, `ISTIOCSR_OPERAND_IMAGE_VERSION`, `TRUSTMANAGER_OPERAND_IMAGE_VERSION`, `OPERATOR_IMAGE_VERSION` + +CertManager remaps quay substrings → RELATED_IMAGE_* in `related_images.go`. + +## Errors & Status (ctrl-runtime path) + +Use `pkg/controller/common` (`ReconcileError`, `FromClientError`, `HandleReconcileResult`) for **IstioCSR/TrustManager business logic only**. Call `HandleReconcileResult` once at the end of `processReconcileRequest` (both use `defaultRequeueTime = 30s`). Do **not** import these into library-go CertManager sync (any `error` is retried; no irrecoverable concept). + +| `reconcileErr` | Degraded | Ready | `ctrl.Result` | +|----------------|----------|-------|---------------| +| `nil` | False / Ready | True / Ready | `{}` | +| `RetryRequiredError` | False / Ready | False / Progressing | `{RequeueAfter: 30s}` | +| `IrrecoverableError` | True / Failed | False / Failed | `{}` (no requeue until next event) | + +**`FromClientError`**: Unauthorized/Forbidden/Invalid/BadRequest/ServiceUnavailable → Irrecoverable; **everything else** (incl. NotFound/Conflict) → Retry. Don’t special-case NotFound/Conflict in reconcile — wrap with `FromClientError`. + +**`MultipleInstanceError`**: **IstioCSR only** (namespaced singleton; CEL can’t see siblings). Set Ready=False on the rejected instance, emit Warning, **`err = nil`** before returning — do **not** route through `HandleReconcileResult`. TrustManager is cluster-scoped `cluster` — Kubernetes name uniqueness + CEL name-lock already prevent duplicates. + +**Status quirks**: `SetCondition` counts as changed only if Status/Reason change (message-only ≠ write). `HandleReconcileResult` skips status write if neither Degraded nor Ready changed. If status write fails on an irrecoverable path, ctrl-runtime may still retry — accepted side effect. + +## Shared Utilities (exact symbols) + +**`pkg/controller/common`**: `ManagedResourceLabelKey` (`app`), `OperatorNamespace`, `TrustedCABundleConfigMapName`, `TrustedCABundleKey`; `CtrlClient`, `NewClient`, `UpdateWithRetry`; error constructors/predicates; `HandleReconcileResult`; `MergeContainerArgs`, `ParseArgMap`, `StripArgsByKeys`, `ArgKeysSet`; `WithClusterTLSProfileFromAPIServer`; validation helpers; metadata helpers (`UpdateName`/`UpdateNamespace`/`UpdateResourceLabels`, `ObjectMetadataModified`, …). + +**`pkg/features`**: `SetupWithFlagValue`, `NewFeatureGateState`, `IsIstioCSRFeatureGateEnabled`, `(*FeatureGateState).IsTrustManagerFeatureGateEnabled`, `DefaultFeatureGate`, `FeatureSetOKD`. + +**`pkg/tlsprofile`**: `EffectiveSpec`, `CertManagerWebhookTLSArgs`, `CertManagerOperandMetricsTLSArgs`, `CertManagerCipherSuiteArgKeys`, `ClientTLSConfig`. + +## OpenShift Integrations + +| Integration | Mechanism | +|-------------|-----------| +| Proxy | `withProxyEnv` + CSV `proxy-aware: true`; see `docs/proxy.md` | +| Trusted CA | `--trusted-ca-configmap` mounts admin-created CM at **`/etc/pki/tls/certs/cert-manager-tls-ca-bundle.crt`** (`subPath: ca-bundle.crt`). Missing CM → library-go sets **Degraded=True and retries** (not a permanent fail). TrustManager separately watches CNO CM `cert-manager-operator-trusted-ca-bundle` — **not** the CertManager flag. | +| TLS profile | Registered when **Infrastructure** informer is `Applicable()` (APIServer shares that factory; not separately discovered). Applies only when `APIServer.spec.tlsAdherence` is `StrictAllComponents`. Nil profile → **Intermediate**. TLS 1.3 → strip cipher args via `StripArgsByKeys(..., CertManagerCipherSuiteArgKeys)`. Hook **before** `withUnsupportedArgsOverrideHook`. Missing `APIServer/cluster` object → Degraded+retry (not silent). | +| Cloud credentials | Mount **existing** Secret into **controller** Deployment only — **never** create CredentialsRequest. AWS: `/.aws` + `AWS_SDK_LOAD_CONFIG=1`. GCP: `service_account.json` → `/.config/gcloud/application_default_credentials.json`. Other platforms → hard error. Missing secret → **Degraded=True + retry** (library-go). See `docs/cloud_credentials.md`. | +| Optional APIs | Discover Infrastructure first (`InitInformerIfAvailable` / `Applicable()`). NotFound ≠ error; skip cloud-cred + TLS hooks when absent. | +| Monitoring | CSV `operatorframework.io/cluster-monitoring: "true"`; operand Service labels in bindata; no operator-owned ServiceMonitor. See `docs/operand_metrics.md`. | +| FIPS | `hack/go-fips.sh` WARN branch = **local-only**. `go.mod` replace → `openshift/jetstack-cert-manager` should stay lockstep with `CERT_MANAGER_VERSION`. Don’t retarget upstream or silence WARN. Flip CSV `fips-compliant` only with a real guarantee change. See `docs/fips-guidelines.md`. | +| OLM | `replaces` / `skipRange`; uninstall requires manual operand cleanup; CSV `tls-profiles: "false"` despite runtime TLS hooks. | + +Detail playbooks: `docs/{integration,security,fips,operator-controllers}-guidelines.md`. + +## Cache Constraints (`setup_manager.go`) + +- One `labels.Selector` per GVK — **no OR across different label keys**. +- ConfigMaps: never label-filter the cache (TrustManager needs unlabeled CA bundle; IstioCSR needs `watched-by` key). Filter in **predicates**. +- Issuer / ClusterIssuer: never managed-label cache filter (user Issuers aren’t labeled). +- Shared filterable GVKs (e.g. Deployment): merge values into one `labelKey In (v1, v2)`. +- Heuristic: if any controller must watch unlabeled instances → leave type off managed lists; use predicates. + +## Data Flow (summary) + +```text +CertManager/cluster ──library-go──► Deployments in cert-manager NS + └─► cert-manager reconciles Certificate/Issuer/... + +IstioCSR/default ──ctrl-runtime Create/Update──► istio-csr Deployment (+ certs/RBAC/...) + +TrustManager/cluster ──ctrl-runtime SSA──► trust-manager Deployment (+ webhook/certs/...) + └─► trust-manager reconciles Bundle +``` + +## Anti-Patterns Observed in Code + +1. Assuming uniform apply method across packages. +2. Hand-editing bindata / generated clients. +3. Treating `CertManagerReconciler` as a real controller. +4. Expecting automatic operand garbage collection on IstioCSR/TrustManager delete (TODO). +5. Setting TLS 1.3 cipher-suite args. +6. Shipping non-FIPS builds to CI/production (`go-fips.sh` warning). + +## SME Review Recommended + +- Exact uninstall / finalizer cleanup design intent for IstioCSR & TrustManager. +- Whether IstioCSR will be migrated 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). + +**Last verified against**: repo tree on branch `master` (operand versions v1.20.3 / istio-csr v0.16.0 / trust-manager v0.20.3). diff --git a/ai-docs/decisions/adr-0001-dual-controller-frameworks.md b/ai-docs/decisions/adr-0001-dual-controller-frameworks.md new file mode 100644 index 000000000..0fd20018e --- /dev/null +++ b/ai-docs/decisions/adr-0001-dual-controller-frameworks.md @@ -0,0 +1,41 @@ +# ADR-0001: Dual Controller Frameworks (library-go + controller-runtime) + +**Status**: Accepted +**Date**: 2025-02-01 (IstioCSR era; TrustManager extended 2026-03) +**Deciders**: cert-manager-operator maintainers + +## Context + +Core CertManager operand management was built on OpenShift **library-go** (`staticresourcecontroller`, `deploymentcontroller`, `OperatorClient` / `OperatorStatus`). Newer optional operands (IstioCSR, TrustManager) were added as feature-gated controllers on a unified **controller-runtime** manager (`pkg/operator/setup_manager.go`) with custom `ConditionalStatus`. + +Migrating CertManager off library-go was not required for shipping optional operands; mixing frameworks avoids a large rewrite while allowing modern ctrl-runtime patterns for new CRDs. + +## Decision + +Keep **library-go** for CertManager (always-on). Use **controller-runtime** for IstioCSR and TrustManager, started only when `--unsupported-addon-features` enables the corresponding gates. + +## Consequences + +**Positive**: +- Incremental delivery of IstioCSR/TrustManager without rewriting CertManager. +- Shared ctrl-runtime helpers in `pkg/controller/common` (errors, status, client retry). + +**Negative / Trade-offs**: +- Agents and contributors must not assume one framework or apply method. +- Two status models (`OperatorStatus` vs `ConditionalStatus`). +- Duplicate concepts (deployment overrides, image env) expressed differently per stack. + +## Alternatives Considered + +- Rewrite CertManager on controller-runtime — high risk / deferred. +- Pure library-go for optional operands — poorer fit for multi-watch CR-centric install pipelines. + +## References + +- `pkg/operator/starter.go`, `pkg/operator/setup_manager.go` +- `pkg/controller/certmanager/`, `pkg/controller/istiocsr/`, `pkg/controller/trustmanager/` +- Enhancements: [istio-csr-controller](https://github.com/openshift/enhancements/blob/master/enhancements/cert-manager/istio-csr-controller.md), [trust-manager-controller](https://github.com/openshift/enhancements/blob/master/enhancements/cert-manager/trust-manager-controller.md) + +## SME Review Recommended + +Rationale for not unifying frameworks long-term; any timeline to migrate CertManager or IstioCSR apply style. diff --git a/ai-docs/decisions/adr-0002-apply-strategies.md b/ai-docs/decisions/adr-0002-apply-strategies.md new file mode 100644 index 000000000..9cdb1c98a --- /dev/null +++ b/ai-docs/decisions/adr-0002-apply-strategies.md @@ -0,0 +1,43 @@ +# ADR-0002: Per-Controller Resource Apply Strategy + +**Status**: Accepted +**Date**: 2026-03-11 (TrustManager SSA; IstioCSR remains Create+Update) +**Deciders**: cert-manager-operator maintainers + +## Context + +CertManager uses library-go `resourceapply` / DeploymentController. IstioCSR (earlier ctrl-runtime operand) uses imperative **Create + UpdateWithRetry**. TrustManager (newest) uses **Server-Side Apply** with field owner `trust-manager-controller` and managed-field-aware skip helpers. + +Documenting a single “we use SSA everywhere” claim is a known hallucination failure mode for this repo. + +## 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`). +4. Status updates for IstioCSR/TrustManager may still use `UpdateWithRetry` on the CR object. + +## Consequences + +**Positive**: +- Clear field ownership for TrustManager-managed resources. +- Shared `common.FromClientError` / `HandleReconcileResult` still apply. + +**Negative / Trade-offs**: +- Three apply idioms in one repo. +- IstioCSR and TrustManager diverge — copy-paste across packages is unsafe. + +## Alternatives Considered + +- Retrofit SSA onto IstioCSR immediately — not done at TrustManager introduction. +- Strategic merge patches only — rejected for TrustManager in favor of SSA. + +## References + +- TrustManager: `pkg/controller/trustmanager/services.go`, `deployments.go`, `constants.go` (`fieldOwner`) +- IstioCSR: `pkg/controller/istiocsr/services.go`, `deployments.go` +- 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. diff --git a/ai-docs/decisions/adr-0003-feature-gates.md b/ai-docs/decisions/adr-0003-feature-gates.md new file mode 100644 index 000000000..73c5aac7b --- /dev/null +++ b/ai-docs/decisions/adr-0003-feature-gates.md @@ -0,0 +1,55 @@ +# ADR-0003: Operator Feature Gates via `--unsupported-addon-features` + +**Status**: Accepted +**Date**: 2026-03 (TrustManager cluster FeatureSet requirement removed — CM-1141) +**Deciders**: cert-manager-operator maintainers + +## Context + +Optional controllers (IstioCSR, TrustManager) need runtime enablement without always-on cost. Gates are defined in `api/operator/v1alpha1/features.go` and applied through the operator flag `--unsupported-addon-features` (`pkg/features.SetupWithFlagValue`). + +TrustManager was TechPreview and previously interacted with cluster `featuregates/cluster` / FeatureSet allow-listing. CM-1141 **dropped cluster FeatureSet gating** for TrustManager; enablement is internal gate only. `FeatureGateState` still reads cluster FeatureGate for discovery/error handling and retains unused `passesClusterPreviewGating`. + +## Decision + +- Expose operator-local gates: `IstioCSR` (GA, default true), `TrustManager` (TechPreview, default false). +- Wire enablement through `--unsupported-addon-features` (and CSV/deployment args in OLM). +- Do **not** require cluster FeatureSet for TrustManager after CM-1141. +- Fail-closed on transient FeatureGate discovery errors without aborting the rest of operator startup (`starter.go` retries). +- CRDs always install; only controllers are gated. + +**Five-touchpoint wiring for any new gate**: + +1. `api/operator/v1alpha1/features.go` — `Default` + `PreRelease` +2. `pkg/features` — register / `SetupWithFlagValue` +3. `pkg/operator/starter.go` — parse flag + optional FeatureGateState +4. Runtime `Is*Enabled` check +5. `setup_manager.go` — shared cache object list + reconciler registration + +Do not resurrect unused `passesClusterPreviewGating` without a new ADR. + +## Consequences + +**Positive**: +- Operators can enable TechPreview TrustManager on standard clusters via flag/CSV without FeatureSet flips. +- IstioCSR remains on by default as GA. + +**Negative / Trade-offs**: +- Name `unsupported-addon-features` is easy to miss when searching for “featuregate”. +- Dead/unused cluster-preview helper may confuse readers of `features.go`. + +## Alternatives Considered + +- Cluster FeatureSet-only gating — removed for TrustManager (CM-1141). +- Separate Deployment for each optional operand — higher OLM complexity. + +## References + +- `api/operator/v1alpha1/features.go` +- `pkg/features/features.go` +- `pkg/operator/starter.go` (`setupFeatureGates`) +- Commit/message: CM-1141 drops cluster FeatureSet gate for TP TrustManager + +## SME Review Recommended + +Operational guidance for catalog/CSV arg defaults; whether `passesClusterPreviewGating` should be deleted or reused. diff --git a/ai-docs/decisions/adr-template.md b/ai-docs/decisions/adr-template.md new file mode 100644 index 000000000..9e8da27e5 --- /dev/null +++ b/ai-docs/decisions/adr-template.md @@ -0,0 +1,30 @@ +# ADR Template + +**Status**: Proposed | Accepted | Deprecated | Superseded +**Date**: YYYY-MM-DD +**Deciders**: [team/owners] + +## Context + +[What problem or constraint led to this decision?] + +## Decision + +[What was decided?] + +## Consequences + +**Positive**: +- … + +**Negative / Trade-offs**: +- … + +## Alternatives Considered + +- … + +## References + +- Code: `path/file.go` +- Related: [enhancement or PR] diff --git a/ai-docs/domain/certmanager.md b/ai-docs/domain/certmanager.md new file mode 100644 index 000000000..6bda7f6d4 --- /dev/null +++ b/ai-docs/domain/certmanager.md @@ -0,0 +1,83 @@ +# CertManager + +**API Group**: `operator.openshift.io/v1alpha1` +**Kind**: `CertManager` +**Scope**: Cluster (singleton name **`cluster`**) + +**API Definition**: [api/operator/v1alpha1/certmanager_types.go](https://github.com/openshift/cert-manager-operator/blob/master/api/operator/v1alpha1/certmanager_types.go) + +## Purpose + +Primary operator CR that configures the three cert-manager operand Deployments (`cert-manager`, `cert-manager-webhook`, `cert-manager-cainjector`) and optional NetworkPolicies. Missing `cluster` CR is auto-created by `DefaultCertManager` (`pkg/controller/certmanager/default_cert_manager_controller.go`). Controllers hardcode `Lister().Get("cluster")` — a misnamed CertManager CR is **silently ignored** (unlike TrustManager/IstioCSR, the `cluster`/`default` name is **not** CEL-enforced on CertManager). + +**Key Principle**: Embeds OpenShift `OperatorSpec` / `OperatorStatus` (library-go status model), unlike IstioCSR/TrustManager which use custom `ConditionalStatus`. + +## Spec Structure + +Verified in `certmanager_types.go`: + +```go +type CertManagerSpec struct { + apiv1.OperatorSpec `json:",inline"` // ManagementState, LogLevel, OperatorLogLevel, + // UnsupportedConfigOverrides, ObservedConfig + ControllerConfig *DeploymentConfig `json:"controllerConfig,omitempty"` + WebhookConfig *DeploymentConfig `json:"webhookConfig,omitempty"` + CAInjectorConfig *DeploymentConfig `json:"cainjectorConfig,omitempty"` + DefaultNetworkPolicy string `json:"defaultNetworkPolicy,omitempty"` // "true"|"false"|"" + NetworkPolicies []NetworkPolicy `json:"networkPolicies,omitempty"` +} + +type DeploymentConfig struct { + OverrideArgs []string + OverrideEnv []corev1.EnvVar + OverrideLabels map[string]string + OverrideResources CertManagerResourceRequirements // Limits, Requests + OverrideReplicas *int32 + OverrideScheduling CertManagerScheduling // NodeSelector, Tolerations +} +``` + +**NetworkPolicy** (`Name`, `ComponentName`, `Egress`): +- `defaultNetworkPolicy`: CEL ratchet — once `"true"`, cannot go to `"false"`. Enabling applies deny-all + allow bindata; static defaults are **never deleted** on disable. +- User `networkPolicies`: `name`+`componentName` immutable; egress appendable; ingress is operator-derived (not user-configurable). +- CRD Enum allows only **`CoreController`** today, even though `getPodSelectorForComponent` already has `CAInjector`/`Webhook` cases — exposing those needs extending the Enum, not the selector. + +**UnsupportedConfigOverrides** helper types (`Controller`/`Webhook`/`CAInjector` with `Args []string`) parse `OperatorSpec.UnsupportedConfigOverrides` RawExtension — not first-class Spec fields. + +## Status + +```go +type CertManagerStatus struct { + apiv1.OperatorStatus `json:",inline"` // Conditions, ObservedGeneration, Versions, Generations, ... +} +``` + +Condition types from OpenShift operator API: `Available`, `Progressing`, `Degraded`, `PrereqsSatisfied`, `Upgradeable`. + +## Lifecycle + +1. **Creation**: Operator creates `certmanagers.operator.openshift.io/cluster` if absent. +2. **Update**: library-go staticresource + deployment controllers reconcile bindata assets with overrides (args/env/labels/resources/replicas/scheduling), proxy, trusted-CA, TLS profile, optional cloud-credentials mount. +3. **Deletion / uninstall**: CSV documents that operands need **manual** cleanup (`console.openshift.io/disable-operand-delete: "true"`). + +## Component-Specific Behavior + +| Concern | Behavior | Code | +|---------|----------|------| +| Images | Env `RELATED_IMAGE_CERT_MANAGER_{CONTROLLER,WEBHOOK,CA_INJECTOR,ACMESOLVER}` | `pkg/controller/certmanager/related_images.go` | +| Apply | library-go `resourceapply` / DeploymentController — **not** ctrl-runtime SSA | `cert_manager_*_deployment.go` | +| NetworkPolicy | Static + user-defined via `resourceapply.ApplyNetworkPolicy`; defaults never deleted once applied | `cert_manager_networkpolicy.go` | +| Placeholder | `CertManagerReconciler` in `certmanager_controller.go` is **not** started — RBAC scaffold only | `starter.go` | + +**Operand CRDs** (Certificate, Issuer, ClusterIssuer, Order, Challenge) are reconciled by the **cert-manager controller** Deployment, not by this operator’s primary loop. Types live under `vendor/github.com/cert-manager/...` (replace → `openshift/jetstack-cert-manager`). + +## Common Mistakes + +1. Editing `config.openshift.io` CertManager CRD stub under `config/crd/bases/` — not wired in kustomization; no Go types; not the operator API. +2. Expecting SSA field ownership on CertManager-managed Deployments. +3. Setting `defaultNetworkPolicy: "false"` after `"true"` — API validation rejects it. +4. Setting user `networkPolicies[].componentName` to `CAInjector`/`Webhook` — CRD Enum rejects; only `CoreController` is allowed today. + +## Related Concepts + +- [IstioCSR](./istiocsr.md) | [TrustManager](./trustmanager.md) | [Architecture](../architecture/components.md) diff --git a/ai-docs/domain/istiocsr.md b/ai-docs/domain/istiocsr.md new file mode 100644 index 000000000..ca1162a06 --- /dev/null +++ b/ai-docs/domain/istiocsr.md @@ -0,0 +1,83 @@ +# IstioCSR + +**API Group**: `operator.openshift.io/v1alpha1` +**Kind**: `IstioCSR` +**Scope**: Namespaced (singleton name **`default`** in the IstioCSR namespace) + +**API Definition**: [api/operator/v1alpha1/istiocsr_types.go](https://github.com/openshift/cert-manager-operator/blob/master/api/operator/v1alpha1/istiocsr_types.go) + +## Purpose + +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. + +## Spec Structure + +Verified in `istiocsr_types.go`: + +```go +type IstioCSRSpec struct { + IstioCSRConfig IstioCSRConfig `json:"istioCSRConfig"` + ControllerConfig *ControllerConfig `json:"controllerConfig,omitempty"` // Labels map +} + +type IstioCSRConfig struct { + LogLevel int32 + LogFormat string + IstioDataPlaneNamespaceSelector string + CertManager CertManagerConfig // IssuerRef; optional IstioCACertificate ConfigMapReference + IstiodTLSConfig IstiodTLSConfig // CN, TrustDomain, DNSNames, durations, key algo/size, MaxCertificateDuration + Server *ServerConfig // ClusterID, Port + Istio IstioConfig // Revisions, Namespace + Resources corev1.ResourceRequirements + Affinity *corev1.Affinity + Tolerations []corev1.Toleration + NodeSelector map[string]string +} +``` + +`CertManagerConfig.IssuerRef` uses cert-manager `ObjectReference` / `IssuerReference` (`Name`, `Kind`, `Group`). +`ConfigMapReference`: `Name`, `Namespace`, `Key` (`meta.go`). + +## Status + +```go +type IstioCSRStatus struct { + ConditionalStatus `json:",inline,omitempty"` // Conditions []metav1.Condition + IstioCSRImage string + IstioCSRGRPCEndpoint string + ServiceAccount string + ClusterRole string + ClusterRoleBinding string +} +``` + +Condition types (`conditions.go`): `Ready`, `Degraded`. Reasons: `Failed`, `Ready`, `Progressing`. + +## Lifecycle + +1. **Enable**: Feature gate on → `setup_manager.go` registers `pkg/controller/istiocsr` on the unified controller-runtime manager. +2. **Reconcile**: Get CR → finalizer → install pipeline (SA, RBAC, Services, Certificates, Deployment, NetworkPolicies, …) → `common.HandleReconcileResult`. +3. **Delete**: Finalizer present; GA cleanup of operands is **TODO** (warn-only) — see `istiocsr/controller.go`. + +## Component-Specific Behavior + +| 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` | +| 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` | +| Finalizer | `istiocsr.openshift.operator.io/cert-manager-istio-csr-controller` | `constants.go` | + +## Common Mistakes + +1. Copying IstioCSR’s Create+Update path for new features — prefer TrustManager SSA. +2. Assuming cluster FeatureSet is required — IstioCSR is internal-gate only (`pkg/features/features.go`). +3. Filtering Issuer/ClusterIssuer cache by managed labels — `setup_manager.go` forbids this. + +## Related Concepts + +- [CertManager](./certmanager.md) | [TrustManager](./trustmanager.md) | [Enhancement](https://github.com/openshift/enhancements/blob/master/enhancements/cert-manager/istio-csr-controller.md) diff --git a/ai-docs/domain/trustmanager.md b/ai-docs/domain/trustmanager.md new file mode 100644 index 000000000..f91b2ad9a --- /dev/null +++ b/ai-docs/domain/trustmanager.md @@ -0,0 +1,85 @@ +# TrustManager + +**API Group**: `operator.openshift.io/v1alpha1` +**Kind**: `TrustManager` +**Scope**: Cluster (singleton name **`cluster`**) + +**API Definition**: [api/operator/v1alpha1/trustmanager_types.go](https://github.com/openshift/cert-manager-operator/blob/master/api/operator/v1alpha1/trustmanager_types.go) + +## Purpose + +Deploys and configures the **trust-manager** operand, which reconciles cluster-scoped `Bundle` (`trust.cert-manager.io/v1alpha1`) resources to distribute CA bundles into ConfigMaps/Secrets. + +**Key Principle**: TechPreview feature gate `TrustManager` (default **false**). Reference controller-runtime implementation using **Server-Side Apply** with field owner `trust-manager-controller`. + +## Spec Structure + +Verified in `trustmanager_types.go`: + +```go +type TrustManagerSpec struct { + TrustManagerConfig TrustManagerConfig `json:"trustManagerConfig"` + ControllerConfig TrustManagerControllerConfig `json:"controllerConfig,omitempty"` // Labels, Annotations +} + +type TrustManagerConfig struct { + LogLevel int32 + LogFormat string + TrustNamespace string + SecretTargets SecretTargetsConfig // Policy; AuthorizedSecrets + FilterExpiredCertificates FilterExpiredCertificatesPolicy + DefaultCAPackage DefaultCAPackageConfig // Policy + Resources corev1.ResourceRequirements + Affinity *corev1.Affinity + Tolerations []corev1.Toleration + NodeSelector map[string]string +} +``` + +Policy enums: `Enabled` / `Disabled`. SecretTargets policies include `Disabled` / `Custom` (see types for exact consts). + +## Status + +```go +type TrustManagerStatus struct { + ConditionalStatus `json:",inline,omitempty"` + TrustManagerImage string + TrustNamespace string + SecretTargetsPolicy SecretTargetsPolicy + DefaultCAPackagePolicy DefaultCAPackagePolicy + FilterExpiredCertificatesPolicy FilterExpiredCertificatesPolicy +} +``` + +Same `Ready` / `Degraded` condition types as IstioCSR (`conditions.go`). + +## Lifecycle + +1. **Enable**: `--unsupported-addon-features=TrustManager=true` → `IsTrustManagerFeatureGateEnabled()` → `setupTrustManagerController` (`setup_manager.go`). +2. **Install pipeline** (`install_trustmanager.go`): validate → CA ConfigMap → SA → RBAC → Services → Issuer → Certificate → Deployment → ValidatingWebhook → status observed fields. +3. **Delete**: Finalizer TODO / warn-only cleanup (same class of gap as IstioCSR). + +## Component-Specific Behavior + +| 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`, … | +| 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` | +| Bundle CRD | Shipped in `config/crd/`; reconciled by trust-manager operand, not operator primary loop | `config/crd/bases/customresourcedefinition_bundles...` | +| Finalizer | `trustmanager.openshift.operator.io/cert-manager-trust-manager-controller` | `constants.go` | + +**Cluster FeatureSet**: Previously required for TechPreview; **removed** (CM-1141 / `pkg/features` comments). Enablement is internal gate only. + +## Common Mistakes + +1. Documenting “all controllers use SSA” — false; only TrustManager among the three. +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. + +## Related Concepts + +- [CertManager](./certmanager.md) | [IstioCSR](./istiocsr.md) | [Enhancement](https://github.com/openshift/enhancements/blob/master/enhancements/cert-manager/trust-manager-controller.md) diff --git a/ai-docs/exec-plans/README.md b/ai-docs/exec-plans/README.md new file mode 100644 index 000000000..290bb1d51 --- /dev/null +++ b/ai-docs/exec-plans/README.md @@ -0,0 +1,15 @@ +# Execution Plans + +Track feature work for this component. + +## Layout + +- `active/` — plans for in-progress features (one markdown file per effort) + +## Guidance + +For plan structure, status conventions, and completion records, follow Platform documentation under [openshift/enhancements/ai-docs](https://github.com/openshift/enhancements/tree/master/ai-docs) (workflows / design philosophy). Keep plans component-specific: APIs, controllers, CSV/env, e2e labels. + +## Active Plans + +None checked in yet. Add new plans under `active/`. diff --git a/ai-docs/references/ecosystem.md b/ai-docs/references/ecosystem.md new file mode 100644 index 000000000..4400a2ce3 --- /dev/null +++ b/ai-docs/references/ecosystem.md @@ -0,0 +1,76 @@ +# Platform Ecosystem References + +Links to generic OpenShift/Kubernetes patterns in the Platform hub ([openshift/enhancements/ai-docs](https://github.com/openshift/enhancements/tree/master/ai-docs)). Component-specific patterns live in this repository’s `ai-docs/`. + +## Operator Patterns + +**Location**: [ai-docs/platform/operator-patterns/](https://github.com/openshift/enhancements/tree/master/ai-docs/platform/operator-patterns) + +- [Status Conditions](https://github.com/openshift/enhancements/blob/master/ai-docs/platform/operator-patterns/status-conditions.md) + +**Component usage**: +- CertManager embeds `operatorv1.OperatorStatus` via library-go `OperatorClient`. +- IstioCSR/TrustManager use custom `ConditionalStatus` (`Ready`/`Degraded`) via `common.HandleReconcileResult` — outcome table in [architecture/components.md](../architecture/components.md). +- Deep rules: [docs/operator-controllers-guidelines.md](../../docs/operator-controllers-guidelines.md), [docs/error-handling-guidelines.md](../../docs/error-handling-guidelines.md). + +## Testing Practices + +**Location**: Platform practices tree is still sparse in the hub; prefer component guides. + +**Component usage**: [CERT_MANAGER_OPERATOR_TESTING.md](../CERT_MANAGER_OPERATOR_TESTING.md) + [docs/testing-guidelines.md](../../docs/testing-guidelines.md) (apply-path assertions, Ginkgo label quoting). + +## Security Practices + +**Location**: Prefer OpenShift product security docs + component network-policy enhancement. + +**Component usage**: +- NetworkPolicy CEL: `defaultNetworkPolicy` ratchet; user `componentName` Enum = **CoreController only**. +- Trusted CA / TLS 1.3 strip / cloud-cred mount contracts — [docs/security-guidelines.md](../../docs/security-guidelines.md), [docs/integration-guidelines.md](../../docs/integration-guidelines.md). +- FIPS — [docs/fips-guidelines.md](../../docs/fips-guidelines.md). + +## Agent Guideline Index (repo) + +| Domain | File | +|--------|------| +| Controllers / apply / cache | [operator-controllers-guidelines.md](../../docs/operator-controllers-guidelines.md) | +| Errors / status | [error-handling-guidelines.md](../../docs/error-handling-guidelines.md) | +| API / CEL | [api-contracts-guidelines.md](../../docs/api-contracts-guidelines.md) | +| OLM packaging | [olm-packaging-guidelines.md](../../docs/olm-packaging-guidelines.md) | +| Integration hooks | [integration-guidelines.md](../../docs/integration-guidelines.md) | +| Security | [security-guidelines.md](../../docs/security-guidelines.md) | +| Testing | [testing-guidelines.md](../../docs/testing-guidelines.md) | +| FIPS | [fips-guidelines.md](../../docs/fips-guidelines.md) | + +## Reliability / Observability + +**Component usage**: CSV enables cluster monitoring; operand metrics documented in [docs/operand_metrics.md](../../docs/operand_metrics.md). + +## Kubernetes Fundamentals + +**Location**: [ai-docs/domain/kubernetes/](https://github.com/openshift/enhancements/tree/master/ai-docs/domain/kubernetes) + +- [CRDs](https://github.com/openshift/enhancements/blob/master/ai-docs/domain/kubernetes/crds.md) +- [Pod](https://github.com/openshift/enhancements/blob/master/ai-docs/domain/kubernetes/pod.md) +- [Service](https://github.com/openshift/enhancements/blob/master/ai-docs/domain/kubernetes/service.md) + +**Component usage**: Operator ships upstream cert-manager + Bundle CRDs as operands; operator-owned CRDs are under `operator.openshift.io`. + +## OpenShift Fundamentals + +**Location**: [ai-docs/domain/openshift/](https://github.com/openshift/enhancements/tree/master/ai-docs/domain/openshift) + +- [ClusterOperator](https://github.com/openshift/enhancements/blob/master/ai-docs/domain/openshift/clusteroperator.md) +- [ClusterVersion](https://github.com/openshift/enhancements/blob/master/ai-docs/domain/openshift/clusterversion.md) +- [Upgrade strategies](https://github.com/openshift/enhancements/blob/master/ai-docs/platform/openshift-specifics/upgrade-strategies.md) + +**Component usage**: Optional Infrastructure/APIServer discovery for TLS profile and cloud platform; OLM lifecycle (`replaces`/`skipRange`) rather than CVO-managed ClusterOperator for this optional operator. + +## Cross-Repository ADRs + +**Location**: Platform `ai-docs/decisions/` may be incomplete; use hub [DESIGN_PHILOSOPHY.md](https://github.com/openshift/enhancements/blob/master/ai-docs/DESIGN_PHILOSOPHY.md) and [KNOWLEDGE_GRAPH.md](https://github.com/openshift/enhancements/blob/master/ai-docs/KNOWLEDGE_GRAPH.md). + +**Component-specific ADRs**: [ai-docs/decisions/](../decisions/) + +--- + +**Last Updated**: 2026-07-31 diff --git a/ai-docs/references/enhancements.md b/ai-docs/references/enhancements.md new file mode 100644 index 000000000..88f8b6471 --- /dev/null +++ b/ai-docs/references/enhancements.md @@ -0,0 +1,27 @@ +# Enhancement Proposals & Design Docs + +Catalog of design documentation. Enhancements are the source of truth; this file is an index only. + +## OpenShift Enhancements (`openshift/enhancements`) + +| Title | Status | Link | +|-------|--------|------| +| Cert-manager Network Policies | Implemented (API + controllers in-tree) | [cert-manager-network-policies.md](https://github.com/openshift/enhancements/blob/master/enhancements/cert-manager/cert-manager-network-policies.md) | +| Istio CSR Controller | Implemented (GA feature gate default on) | [istio-csr-controller.md](https://github.com/openshift/enhancements/blob/master/enhancements/cert-manager/istio-csr-controller.md) | +| Trust Manager Controller | Implemented (TechPreview, default off) | [trust-manager-controller.md](https://github.com/openshift/enhancements/blob/master/enhancements/cert-manager/trust-manager-controller.md) | + +Related diagrams (IstioCSR): `istio-csr-create.puml` / `istio-csr-delete.puml` under the same enhancements directory. + +## In-repo design / ops docs + +| Doc | Purpose | +|-----|---------| +| [docs/proxy.md](../../docs/proxy.md) | Cluster proxy injection into operands | +| [docs/cloud_credentials.md](../../docs/cloud_credentials.md) | Cloud credential secret mounting (not CredentialsRequest creation) | +| [docs/operand_metrics.md](../../docs/operand_metrics.md) | Operand metrics / monitoring notes | +| [README.md](../../README.md) | Architecture assumptions, local-run, image build | + +## Notes + +- ADRs in `ai-docs/decisions/` capture **component architectural** choices (framework split, apply strategy, feature gates). Do not duplicate enhancement prose here. +- Cross-component platform proposals mentioning cert-manager (OLM platform operators, Route external certs, etc.) are out of scope for this index unless they change this operator’s API or controllers. diff --git a/docs/api-contracts-guidelines.md b/docs/api-contracts-guidelines.md new file mode 100644 index 000000000..863ad23a4 --- /dev/null +++ b/docs/api-contracts-guidelines.md @@ -0,0 +1,179 @@ +# API Contracts Guidelines + +Rules for changing or adding to `api/operator/v1alpha1/`. Read `ai-docs/domain/*.md` +and the relevant ADR before touching a CR's Spec/Status. **Verify every field against +the Go types under `api/operator/v1alpha1/` — never invent, assume, or backfill a +field from upstream cert-manager/trust-manager/istio-csr docs.** + +## 1. CRD Ownership: Operator vs. Operand + +This repo mixes CRDs owned by **this operator** with CRDs owned by the **operands it +deploys**. Know which is which before editing: + +| CRD | Owner | Where it lives | +|---|---|---| +| `CertManager` (`operator.openshift.io`) | This operator | `api/operator/v1alpha1/certmanager_types.go` | +| `IstioCSR` (`operator.openshift.io`) | This operator | `api/operator/v1alpha1/istiocsr_types.go` | +| `TrustManager` (`operator.openshift.io`) | This operator | `api/operator/v1alpha1/trustmanager_types.go` | +| `Certificate`, `Issuer`, `ClusterIssuer`, `Order`, `Challenge` (`cert-manager.io`/`acme.cert-manager.io`) | **cert-manager operand**, reconciled by the cert-manager controller Deployment, not this operator's loop | `vendor/github.com/cert-manager/...`; bases in `config/crd/bases/*-crd.yaml` | +| `Bundle` (`trust.cert-manager.io`) | **trust-manager operand** | `config/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.yml` | +| `CertManager` (`config.openshift.io`) | **Not wired** — stub CRD, no Go types, not in `config/crd/kustomization.yaml` | `config/crd/bases/config.openshift.io_certmanagers.yaml` | + +Rules: + +- Only add Go types for CRDs this operator's controllers actually reconcile + (`operator.openshift.io/v1alpha1`). Never add Go types for operand CRDs — those are + vendored, not owned here. +- Never edit `config.openshift.io_certmanagers.yaml`. It is dead/unused; a common + mistake is assuming it's the operator API. +- Adding a new operand CRD (e.g. a new `Bundle`-like resource) means adding a YAML base + under `config/crd/bases/` and wiring it into `config/crd/kustomization.yaml`, **not** + adding a Go type in `api/operator/v1alpha1/`. + +## 2. Spec/Status Conventions + +Two status models coexist by design (see `ai-docs/decisions/adr-0001-dual-controller-frameworks.md`). +Never assume one is used everywhere: + +- **CertManager**: embeds OpenShift `apiv1.OperatorSpec` / `apiv1.OperatorStatus` + (library-go status model: `ManagementState`, `Conditions`, `ObservedGeneration`, + `Versions`, `Generations`, `UnsupportedConfigOverrides`). +- **IstioCSR** / **TrustManager**: use the repo's own `ConditionalStatus` + (`api/operator/v1alpha1/meta.go`) — just `Conditions []metav1.Condition`. Condition + types are `Ready` / `Degraded`; reasons are `Failed` / `Ready` / `Progressing` + (`conditions.go`). Do not introduce new condition types without updating + `conditions.go` and both `ai-docs/domain/istiocsr.md` and `trustmanager.md`. + +Structural conventions to follow for any new CR or field: + +- Top-level `Spec`/`Status` fields: lowercase JSON tag, `+kubebuilder:validation:Required` + + `+required` (or `Optional`/`+optional`), and a doc comment starting with the + lowercase field name (see `TrustManager`/`IstioCSR` struct comments) — this becomes + the CRD schema description and OLM CSV doc. +- Config sub-structs are split into an operand-behavior config + (`TrustManagerConfig`, `IstioCSRConfig`) and an operator-behavior config + (`ControllerConfig` field — labels/annotations the operator applies to created + resources). Follow this split for new fields: does it configure the *operand's* + runtime behavior, or *how the operator creates* resources for it? +- New fields generally carry an explicit kubebuilder validation marker + (`+kubebuilder:validation:{Required,Optional,Enum,Minimum,Maximum,MinLength,MaxLength,MinItems,MaxItems,Pattern}`) + rather than relying on Go zero-values as implicit defaults — prefer `+kubebuilder:default:=` + where a default makes sense. +- Lists that need merge semantics use `+listType=map` with `+listMapKey=...` + (`NetworkPolicy`, `ConditionalStatus.Conditions`); simple dedup lists use + `+listType=set` (`Revisions`, `CertificateDNSNames`); free-form ordered lists use + `+listType=atomic` (`Tolerations`). +- Maps use `+mapType=atomic` (whole-map replace, e.g. `NodeSelector`) or + `+mapType=granular` (per-key ownership, e.g. `Labels`/`Annotations`) — pick based on + whether partial ownership by multiple actors is expected. + +## 3. Immutability Rules + +Immutability is enforced with CEL `+kubebuilder:validation:XValidation` using +`oldSelf`/`self`, **not** with webhooks (there is no admission webhook wired for these +CRs; `config/crd/patches/webhook_in_certmanagers.yaml` is present but commented out). +Patterns already in use: + +- **Set-once, then locked**: `oldSelf == '' || self == oldSelf` (or the zero-value + equivalent for ints) — e.g. `TrustNamespace`, `PrivateKeySize`, + `PrivateKeyAlgorithm`, `Port`. +- **One-way ratchet** (can enable, can't disable): `oldSelf != 'true' || self == 'true'` + — `DefaultNetworkPolicy`. Once `"true"`, it can never revert. +- **Fully immutable after any value is set**: `self == oldSelf` — `IssuerRef`, + `Istio.Namespace`. +- **Creation-only field** (must be present or absent consistently): the + `has(oldSelf.x) == has(self.x)` pattern — `PrivateKeyAlgorithm`/`PrivateKeySize` + companion checks, `IssuerRef` presence check on `CertManagerConfig`. +- **Immutable list identity, mutable contents**: `NetworkPolicies` — `name`/ + `componentName` pairs are immutable once present, but `egress` rules within an + existing entry are not locked by this rule. + +When adding a new immutable field: put the `XValidation` on the field (or the +containing struct for cross-field rules), write a clear `message`, and document the +immutability in the field's doc comment (not just the marker) — reviewers and +generated CRD docs both need it in prose. + +## 4. Singleton Naming + +`TrustManager` is a cluster-scoped singleton named **`cluster`**, and `IstioCSR` is a +namespaced singleton named **`default`** (one per namespace) — both are enforced via CEL +on the resource: + +```go +// +kubebuilder:validation:XValidation:rule="self.metadata.name == 'cluster'",message="... is a singleton, .metadata.name must be 'cluster'" +``` + +`CertManager` is also treated as a cluster-scoped singleton named **`cluster`** by +convention, but this is **not** CEL-enforced in `certmanager_types.go` — the library-go +controller set simply hardcodes `Lister().Get("cluster")` everywhere it reads the CR, so +a differently-named CertManager object is silently never reconciled rather than rejected +by the API. Don't assume CertManager's singleton behavior is schema-enforced when +reasoning about it. + +If you add a **new** singleton CR, follow the TrustManager/IstioCSR CEL pattern above +(adjust the required name), and it must be `+genclient:nonNamespaced` if cluster-scoped. +IstioCSR's namespaced singleton behavior is additionally enforced by the controller +(`disallowMultipleIstioCSRInstances`), not by CEL alone, since CEL can't see sibling +objects — a new namespaced singleton needs the same controller-side check. + +## 5. Feature-Gated CRDs + +`IstioCSR` (GA, default **true**) and `TrustManager` (TechPreview, default **false**) +are gated in `api/operator/v1alpha1/features.go` via +`featuregate.Feature`/`OperatorFeatureGates`, enabled at runtime through +`--unsupported-addon-features` (see `ai-docs/decisions/adr-0003-feature-gates.md`). +`CertManager` is always on and ungated. + +Rules for a new feature-gated CR: + +- The **CRD itself is always installed** (shipped unconditionally in + `config/crd/bases/` + `kustomization.yaml`); only the **controller** is gated. + Do not try to conditionally install the CRD based on the feature gate. + - Note: TrustManager's cluster `FeatureSet` gating requirement was **removed** + (CM-1141) — new gates should follow the `TrustManager`/`IstioCSR` pattern of + operator-local flag gating only, not cluster `FeatureSet` allow-listing, unless a + new ADR says otherwise. +- Add the gate to `OperatorFeatureGates` in `features.go` with an explicit `Default` + and `PreRelease` (`GA` or `"TechPreview"`), plus a doc comment linking the + enhancement doc, mirroring `FeatureIstioCSR`/`FeatureTrustManager`. +- Wire enablement/CSV args the same way as the existing two gates + (`pkg/features`, `setup_manager.go`); don't invent a new gating mechanism. + +## 6. Generation / Manifests Workflow + +Never hand-edit generated output. After changing any file in `api/operator/v1alpha1/`: + +1. `make generate` — regenerates deepcopy (`zz_generated.deepcopy.go`) via + `controller-gen object:...` and client-gen artifacts (`hack/update-clientgen.sh`). +2. `make manifests` — regenerates CRD YAML under `config/crd/bases/` and RBAC under + `config/rbac/` via `controller-gen rbac:... crd webhook`. +3. If operand versions/manifests changed (not API types), use + `make update-manifests` (`hack/update-{cert-manager,istio-csr,trust-manager}-manifests.sh`), + which is separate from `make generate`/`manifests`. +4. `make update` runs generate + update-manifests + update-bindata together; CI's + `verify-scripts` (`verify-bindata`, `verify-deepcopy.sh`, `verify-clientgen.sh`, + `verify-bundle.sh`) will fail the build if generated output is stale or hand-edited. + +Never manually edit: `zz_generated.deepcopy.go`, files under `config/crd/bases/`, +`pkg/operator/assets/bindata.go`, generated clientset/informers/listers/ +applyconfigurations, or `bindata/` YAML. Edit the Go types (or the source manifests for +operand bindata) and regenerate. + +## 7. What Not to Invent About Fields + +- Don't add fields, enum values, or defaults that aren't present in the Go type under + `api/operator/v1alpha1/` — verify in source, not from upstream cert-manager/ + istio-csr/trust-manager docs, memory, or naming conventions from other operators. +- Don't assume a field is namespaced, cluster-scoped, mutable, or defaulted without + 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`. + 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 + `OperatorSpec.UnsupportedConfigOverrides` (a `RawExtension`), not real schema fields. +- When documenting a CR, cross-check `ai-docs/domain/.md` — if your reading of + the Go type conflicts with that doc, prefer the Go source and flag the doc as + possibly stale rather than propagating the conflict into new docs. diff --git a/docs/error-handling-guidelines.md b/docs/error-handling-guidelines.md new file mode 100644 index 000000000..d5e95fe82 --- /dev/null +++ b/docs/error-handling-guidelines.md @@ -0,0 +1,167 @@ +# Error Handling Guidelines + +Scope: the controller-runtime (ctrl-runtime) stack — **IstioCSR** and **TrustManager**. CertManager +uses library-go and follows a different model (see [§7](#7-library-go-vs-ctrl-runtime-do-not-mix-patterns)). +Core types live in `pkg/controller/common/errors.go` and `reconcile_result.go`. + +## 1. `ReconcileError` — the only error type reconcile logic should return + +```go +type ReconcileError struct { + Reason ErrorReason // IrrecoverableError | RetryRequiredError | MultipleInstanceError + Message string + Err error +} +``` + +Business-logic reconcile functions (deployment/service/RBAC/certificate reconciliation, config +validation) should return a `*ReconcileError`, not a bare `error`. Construct one via a helper — +avoid building the struct literal directly: + +| Helper | When to use | +|---|---| +| `NewIrrecoverableError(err, msg, args...)` | Config/spec is invalid, or the error can never be fixed by retrying (bad issuer kind, malformed PEM, missing required key). | +| `NewRetryRequiredError(err, msg, args...)` | Transient condition that a later reconcile could resolve. | +| `NewMultipleInstanceError(err)` | Singleton-CR invariant violated (see [§5](#5-multipleinstanceerror-is-not-a-normal-error)). | +| `FromClientError(err, msg, args...)` | Any error returned from a Kubernetes API call (Get/Create/Update/Patch/List). | +| `FromError(err, msg, args...)` | Rewrapping an error that **may already be** a classified `ReconcileError` (e.g. bubbling a helper's result up with extra context) — only `IrrecoverableError` is special-cased and preserved; any other input reason (including an existing `RetryRequiredError` or `MultipleInstanceError`) collapses to `RetryRequiredError`, since the fallback branch doesn't distinguish them. | + +All constructors return `nil` for a `nil` input error, so `return NewIrrecoverableError(err, "...")` +is safe to use directly after an `if err != nil` check without an extra nil guard. + +Check classification with `IsIrrecoverableError`, `IsRetryRequiredError`, `IsMultipleInstanceError` +(all use `errors.As`, so they work through wrapping) — never compare `.Reason` directly outside +`pkg/controller/common`. + +## 2. `FromClientError` mapping (`errors.go`) + +```go +apierrors.IsUnauthorized / IsForbidden / IsInvalid / IsBadRequest / IsServiceUnavailable + → IrrecoverableError +everything else (NotFound, Conflict, AlreadyExists, timeouts, ...) + → RetryRequiredError +``` + +Rule of thumb: permission/validation-shaped API errors are treated as unfixable by retrying; +existence/contention-shaped errors are treated as transient. Do not special-case `NotFound` or +`Conflict` yourself to invent a different severity than `FromClientError` would assign — route the +raw client error through it and let it classify. + +`CtrlClient` methods (`client.go`) wrap errors with `fmt.Errorf("...: %w", err)` before returning +them; this preserves `apierrors` unwrapping, so `FromClientError` still classifies correctly even +though the error text no longer looks like a raw API error. + +## 3. `HandleReconcileResult` — requeue vs. degrade + +`HandleReconcileResult(status, reconcileErr, log, updateConditionFn, requeueDuration)` is the +**single point** that turns a `ReconcileError` into a `ctrl.Result` + condition update. Call it once, +at the end of `processReconcileRequest`, with the aggregate error from your reconcile chain — do not +call `SetCondition` ad hoc elsewhere in the "happy path" of a reconcile. + +| `reconcileErr` | `Degraded` | `Ready` | `ctrl.Result` | +|---|---|---|---| +| `nil` (success) | `False` / `Ready` | `True` / `Ready` | `{}` (no requeue; next event-driven trigger) | +| `IsRetryRequiredError` | `False` / `Ready` | `False` / `Progressing` | `{RequeueAfter: requeueDuration}` | +| `IsIrrecoverableError` | `True` / `Failed` | `False` / `Failed` | `{}` (**no requeue** — waits for spec/resource change to re-trigger) | + +Both controllers pass `defaultRequeueTime = 30s` for `requeueDuration` — keep new ctrl-runtime +operands consistent with this unless there's a documented reason to diverge. + +If `updateConditionFn` itself fails, that error is returned instead of requeuing/succeeding (status +write failure always wins). This means an irrecoverable error whose status update also fails **will** +eventually get retried by ctrl-runtime's default error backoff, even though the intent was "no +requeue" — this is a known, accepted side effect, not a bug to route around. + +## 4. What does *not* go through `ReconcileError`/`HandleReconcileResult` + +Framework-level plumbing in `Reconcile()` — fetching the CR, finalizer add/remove, deletion +clean-up — is returned as a plain (often `fmt.Errorf`-wrapped) `error` straight to ctrl-runtime, +*before* `processReconcileRequest` is called. Only the actual reconcile-the-operand logic is routed +through `ReconcileError` + `HandleReconcileResult`. When adding a new operand, follow this same +split rather than routing every error through `HandleReconcileResult`. + +`NotFound` on the initial `Get` of the CR is treated as "nothing to do" (`ctrl.Result{}, nil`), not +an error at all — the CR was deleted between enqueue and reconcile. + +## 5. `MultipleInstanceError` is not a normal error + +Only IstioCSR currently implements this. It's a *namespaced* singleton (CRD enforces name `default` +per namespace) — CEL can validate the name within one namespace but can't see sibling namespaces, so +`istiocsr/utils.go: disallowMultipleIstioCSRInstances` does the cross-namespace check at the +controller level: + +1. Set `Ready=False/Failed` on the *rejected* instance directly via `status.SetCondition` + + a manual `updateCondition` call (not via `HandleReconcileResult`). +2. Return `common.NewMultipleInstanceError(...)`. +3. In `processReconcileRequest`, check `common.IsMultipleInstanceError(err)` **before** calling + `HandleReconcileResult`, emit a `Warning` event, and swallow the error (`err = nil`) so ctrl-runtime + does not requeue a permanently-rejected instance: + +```go +if err := r.disallowMultipleIstioCSRInstances(istiocsr); err != nil { + if common.IsMultipleInstanceError(err) { + r.eventRecorder.Eventf(istiocsr, corev1.EventTypeWarning, "MultiIstioCSRInstance", "...") + err = nil + } + return ctrl.Result{}, err +} +``` + +TrustManager has no equivalent guard: it's a *cluster-scoped* singleton named `cluster`, and +Kubernetes' own object-name uniqueness combined with the CEL name-lock (see +[api-contracts-guidelines.md §4](api-contracts-guidelines.md)) already rules out a second instance, +so there's nothing for a controller-side check to catch. + +Any new **namespaced** singleton (where CEL can't see sibling namespaces) should reuse this reason +and this "warn + swallow" handling rather than treating it as `IrrecoverableError` (it isn't a config +problem with *this* instance, it's a cluster-state problem). A new cluster-scoped singleton with a +CEL-locked name likely doesn't need this at all. + +## 6. Status condition updates — things that will surprise you + +- `ConditionalStatus.SetCondition(type, status, reason, msg)` returns `true` only if `Status` or + `Reason` changed (or the condition didn't exist yet) — **a message-only change to an existing + condition does not count as a change** and will not trigger a write. Don't rely on message text + alone to force a status update. +- `HandleReconcileResult` only calls `updateConditionFn` when at least one of `Degraded`/`Ready` + actually changed (`degradedChanged || readyChanged`) — avoid adding side effects inside + `updateConditionFn` that assume it runs every reconcile. +- TrustManager's `updateConditionFn` closure (`updateCondition` in `trustmanager/utils.go`) prepends + the *reconcile* error to any *status-update* error via + `utilerrors.NewAggregate([]error{prependErr, errUpdate})`, so both are visible in logs/events even + though only one `ctrl.Result` decision is made. Treat this as the reference pattern for a new + `updateConditionFn`. IstioCSR's own `updateCondition` (`istiocsr/utils.go`) does **not** follow it — + on a status-update failure it aggregates the raw and re-wrapped update errors together and drops + `prependErr` entirely — so don't assume every `updateConditionFn` in this repo actually preserves + the original reconcile error; check the specific controller's implementation before relying on it. +- Set both `Degraded` and `Ready` together before the single `updateConditionFn` call ("atomically"), + as `HandleReconcileResult` does — don't split them across two separate status writes. + +## 7. library-go vs. ctrl-runtime — do not mix patterns + +| | CertManager (library-go) | IstioCSR / TrustManager (ctrl-runtime) | +|---|---|---| +| Error type returned from `sync`/`Reconcile` | plain `error` (`fmt.Errorf("...: %w", err)`) | `*common.ReconcileError` (business logic) / plain `error` (framework plumbing) | +| Who sets Degraded | `factory.Controller` automatically, from any non-nil `sync` error | Reconciler explicitly, via `HandleReconcileResult` | +| Retry-vs-permanent distinction | **None** — any error is retried with the factory's rate limiter | Explicit: `IrrecoverableError` never requeues, `RetryRequiredError` requeues after `requeueDuration` | +| Status model | `operatorv1.OperatorStatus` (`v1helpers`) | `v1alpha1.ConditionalStatus` (`common`/API package) | + +Because library-go has no "irrecoverable, stop retrying" concept, do **not** import the +`ReconcileError`/`HandleReconcileResult` machinery from `pkg/controller/common` into +`pkg/controller/certmanager`, and do not assume a CertManager sync error will ever stop being +retried — that only holds for the ctrl-runtime path. (Non-error `pkg/controller/common` helpers like +`MergeContainerArgs`, `ParseArgMap`, and `WithClusterTLSProfileFromAPIServer` are shared with +`certmanager` today and are fine to use — the restriction is specifically about the error- +classification types.) Conversely, do not return a bare `error` from ctrl-runtime reconcile +*business logic* just because that's what CertManager does — wrap it so `HandleReconcileResult` can +classify it. + +## 8. Adding a new ctrl-runtime operand + +Per [ADR-0002](../ai-docs/decisions/adr-0002-apply-strategies.md) and the repo's greenfield rule, +copy **TrustManager**, not IstioCSR, for both apply strategy and error handling — IstioCSR's +Create+Update path and its `updateCondition` aggregation quirk (§6) are legacy, not the target +pattern. At minimum: wrap every client call in `FromClientError`, classify programmatic/validation +failures with `NewIrrecoverableError`, route the top-level reconcile error through +`HandleReconcileResult` with a `30s`-class `requeueDuration`, and keep CR-fetch/finalizer errors +outside that flow. diff --git a/docs/fips-guidelines.md b/docs/fips-guidelines.md new file mode 100644 index 000000000..e4fdd1dd3 --- /dev/null +++ b/docs/fips-guidelines.md @@ -0,0 +1,173 @@ +# FIPS Compliance Guidelines + +This document explains how FIPS (Federal Information Processing Standards) support is +implemented and enforced for the Cert Manager Operator, and what should **not** be +changed without a deliberate, reviewed decision. + +## Why This Matters + +The operator's OLM `ClusterServiceVersion` advertises +`features.operators.openshift.io/fips-compliant: "true"` +(`bundle/manifests/cert-manager-operator.clusterserviceversion.yaml`). This is a +public promise to OpenShift customers running in FIPS mode. If the binary shipped +in the operator image is not actually built with FIPS-validated crypto, this +annotation is misleading and the operator is out of compliance. Every build-time +mechanism described below exists to keep that annotation true. + +## Build-Time FIPS Rules + +FIPS enablement happens primarily at **build time**, via `hack/go-fips.sh`, which is +sourced from the `build-operator` Makefile target: + +```1:11:hack/go-fips.sh +#!/bin/bash + +if GOEXPERIMENT="strictfipsruntime" go build ./tools; then + echo "INFO: building with FIPS support" + + export GOEXPERIMENT="strictfipsruntime" + export GOFLAGS="${GOFLAGS} -tags=strictfipsruntime,openssl" +else + echo "WARN: building without FIPS support, GOEXPERIMENT strictfipsruntime is not available in the go compiler" + echo "WARN: this build cannot be used in CI or production, due to lack of FIPS!!" +fi +``` + +```351:353:Makefile +.PHONY: build-operator +build-operator: ## Build operator binary only (no checks or code generation). + @GOFLAGS="-mod=vendor" source hack/go-fips.sh && $(GO) build $(GOBUILD_VERSION_ARGS) -o $(BIN) +``` + +The logic: + +1. `go-fips.sh` first probes whether the local Go toolchain supports + `GOEXPERIMENT=strictfipsruntime` by trying a throwaway build of `./tools`. +2. If supported, it **exports** `GOEXPERIMENT=strictfipsruntime` and appends + `-tags=strictfipsruntime,openssl` to `GOFLAGS` for the real build that follows + in the same Makefile recipe line (`source` keeps the exported vars in the same + shell). +3. If unsupported, it prints two `WARN` lines and lets the build continue + **without** FIPS flags — intended as a fallback for local development only, + not for shipped artifacts. + +The container image build (`Dockerfile`) uses `docker.io/golang:1.26` (expected to +support `strictfipsruntime`) and runs `make build`, so production and CI images are +built with FIPS mode enabled as long as that toolchain probe succeeds. There is no +separate FIPS/non-FIPS image variant — FIPS-ness is a property of the Go toolchain +used, not a build arg or Dockerfile stage. + +## When Non-FIPS Builds Are Acceptable + +A non-FIPS build (the `WARN` branch of `go-fips.sh`) is generally only appropriate when: + +- You are building **locally** on a developer machine whose Go toolchain does not + support `strictfipsruntime` (e.g. a non-RHEL/non-vendor Go distribution). +- The resulting binary is used **only** for local iteration, unit tests, or + `make local-run` against a dev cluster — never for anything that leaves your + workstation. + +Non-FIPS builds should **not**: + +- Be pushed as a CI artifact or container image (`make image-build` in CI always + uses the FIPS-capable Go image, so this should not occur in practice). +- Be used for release, QE sign-off, or any environment claiming FIPS support. +- Trigger a change to the `fips-compliant: "true"` CSV annotation — the annotation + describes what the *shipped* image guarantees, not what a given local build + happened to produce. + +If you find yourself routinely hitting the `WARN` path locally, prefer fixing your +Go toolchain/environment over "fixing" it by weakening `go-fips.sh` or its build +flags. + +## Crypto Fork Expectations (`go.mod` replace) + +```140:140:go.mod +replace github.com/cert-manager/cert-manager => github.com/openshift/jetstack-cert-manager v1.20.3 +``` + +Upstream `cert-manager/cert-manager` uses Go's standard `crypto/*` packages +directly in ways that are not guaranteed to route through the FIPS-validated +BoringCrypto/OpenSSL module when `strictfipsruntime` is active. To close this +gap, all `cert-manager/cert-manager` imports are transparently redirected to +**`openshift/jetstack-cert-manager`**, a Red Hat-maintained fork that: + +- Tracks the same upstream version tag (`v1.20.3` here — kept in lockstep with + `CERT_MANAGER_VERSION` in the Makefile). +- Carries the minimal patch set needed so the vendored crypto call paths are + compatible with `strictfipsruntime` / `openssl` build tags. + +Expectations for anyone touching this: + +- The replace target version **should match** `CERT_MANAGER_VERSION` in the + Makefile. Bumping one without the other can desync manifests + (`update-manifests`) from the vendored code. +- Avoid removing or "simplifying" this replace directive to point back at upstream + `cert-manager/cert-manager`, even temporarily for debugging — doing so silently + drops FIPS compliance for the cert-manager operand's crypto paths while the + `fips-compliant` CSV annotation would still claim otherwise. +- Fork updates (new patches, rebasing onto a newer upstream tag) should come from + the same upstream/downstream sync process that manages this fork, not from ad + hoc local patches applied to vendor code. +- After changing the replace target or version, run `make update-vendor` (which + runs `go mod tidy`, `go work sync`, `go work vendor`) and `make verify-deps` — + avoid hand-editing `vendor/` to match. + +## The `fips-compliant` CSV Annotation + +```283:287:bundle/manifests/cert-manager-operator.clusterserviceversion.yaml + features.operators.openshift.io/csi: "false" + features.operators.openshift.io/disconnected: "true" + features.operators.openshift.io/fips-compliant: "true" + features.operators.openshift.io/proxy-aware: "true" + features.operators.openshift.io/tls-profiles: "false" +``` + +This annotation is generated/maintained as part of the OLM bundle +(`config/manifests/bases/cert-manager-operator.clusterserviceversion.yaml` is the +base; `bundle/manifests/...` is the generated bundle output from `make bundle`). +It should stay `"true"` as long as: + +- `hack/go-fips.sh` is wired into `build-operator`, and +- the `openshift/jetstack-cert-manager` replace is in place for the operand + crypto paths. + +If either of those build-time guarantees is ever removed or weakened, this +annotation should be flipped to `"false"` in the same change — that is a +compliance-affecting decision that needs explicit sign-off, not something to do +as a side effect of an unrelated refactor. + +## What Agents Should Not Change Casually + +Treat the following as **high-risk, review-required** changes. Avoid modifying +them opportunistically while working on unrelated features/bugs: + +1. **`hack/go-fips.sh`** — avoid removing the `GOEXPERIMENT`/`GOFLAGS` exports, the + `strictfipsruntime`/`openssl` build tags, or silencing the `WARN` messages. +2. **`build-operator` target in `Makefile`** — avoid dropping the + `source hack/go-fips.sh` step or reordering it after the `go build` call. +3. **The `go.mod` replace for `cert-manager/cert-manager`** — avoid pointing it + back at upstream, forking it to a different repo, or bumping its version + independently of `CERT_MANAGER_VERSION` without understanding the crypto + implications. +4. **`features.operators.openshift.io/fips-compliant` in the CSV** (both the + `config/manifests/bases` source and the generated `bundle/manifests` copy) — + only change together with a real change to the build-time FIPS guarantees, and + call it out explicitly in the PR description. +5. **The builder base image in `Dockerfile`** — swapping to a Go distribution + without `strictfipsruntime` support silently downgrades every production + image to the `WARN`/non-FIPS path. + +If a task seems to require touching any of the above, stop and confirm the FIPS +implications with a human reviewer (or at minimum, call it out prominently) before +proceeding — do not treat it as routine build-config cleanup. + +## Quick Reference + +| Concern | Source of truth | +|---|---| +| FIPS build flags | `hack/go-fips.sh` | +| Wiring into build | `Makefile` → `build-operator` | +| Operand crypto fork | `go.mod` → `replace github.com/cert-manager/cert-manager => github.com/openshift/jetstack-cert-manager ...` | +| Compliance claim | `bundle/manifests/cert-manager-operator.clusterserviceversion.yaml` → `features.operators.openshift.io/fips-compliant` | +| Human-readable overview | `ai-docs/architecture/components.md` (FIPS row) | diff --git a/docs/integration-guidelines.md b/docs/integration-guidelines.md new file mode 100644 index 000000000..74328e9d0 --- /dev/null +++ b/docs/integration-guidelines.md @@ -0,0 +1,73 @@ +# OpenShift Integration Guidelines + +Rules for integrating cert-manager-operator with cluster-provided OpenShift services: egress proxy, trusted CA, TLS security profile, cloud credentials, monitoring, and optional (discoverable) APIs. All hooks are wired in `pkg/controller/certmanager/generic_deployment_controller.go` and applied per-operand deployment (`cert-manager`, `cert-manager-webhook`, `cert-manager-cainjector`) via `deploymentcontroller.DeploymentHookFunc`. + +## 1. Proxy + +- **Rule**: Never hardcode proxy env vars. OLM injects `HTTP_PROXY`/`HTTPS_PROXY`/`NO_PROXY` into the operator Deployment when a cluster-wide egress proxy exists (CSV must declare `proxy-aware: true`). +- The operator propagates these to operands via `withProxyEnv` (`pkg/controller/certmanager/deployment_overrides.go`), which reads `operator-lib/proxy.ReadProxyVarsFromEnv()` and merges into container env for every operand deployment. Do not add a per-deployment opt-out. +- See [docs/proxy.md](proxy.md). + +## 2. Trusted Certificate Authority + +- **Rule**: Never bake custom CAs into images or operand code. Trust is delivered only via a cluster-injected ConfigMap referenced by name. +- Flow: admin creates an empty ConfigMap in `cert-manager` NS, labels it `config.openshift.io/inject-trusted-cabundle=true` (CNO injects `ca-bundle.crt`), then sets `--trusted-ca-configmap=` (flag bound to `operator.TrustedCAConfigMapName`, `pkg/cmd/operator/cmd.go`) via the operator's subscription/deployment env `TRUSTED_CA_CONFIGMAP_NAME`. +- Implementation: `withCAConfigMap` (`deployment_overrides.go`) looks up the ConfigMap via the target-namespace `ConfigMapInformer` lister; if not found it returns a retryable error (`"(Retrying) trusted CA config map %q doesn't exist"`). The library-go `DeploymentController` treats every hook error the same way: it sets `Degraded=True` **and** keeps retrying via the factory's rate limiter — there is no non-degrading "just requeue" state on this stack (see [error-handling-guidelines.md](error-handling-guidelines.md) §7). It never becomes a permanent, un-retried failure, but it does surface as Degraded until the ConfigMap appears. +- Mount contract is fixed and must not change: volume name `trusted-ca`, mount path `/etc/pki/tls/certs/cert-manager-tls-ca-bundle.crt`, `subPath: ca-bundle.crt` (constants `trustedCAVolumeName`, `trustedCAPath`, `defaultCABundleKey`). +- If `trustedCAConfigmapName` is empty, `withCAConfigMap` is a no-op — do not add a default configmap name. +- See [docs/proxy.md](proxy.md) (Trusted CA section). + +## 3. TLS Security Profile + +- **Rule**: Do not hardcode TLS min-version/cipher args on operand containers. When the cluster's `APIServer/cluster` `spec.tlsAdherence` is `StrictAllComponents`, derive them from `spec.tlsSecurityProfile` via `pkg/tlsprofile`; when `tlsAdherence` is unset or `LegacyAdheringComponentsOnly`, the hook leaves operand args untouched (no cluster-derived override is applied at all) — see [security-guidelines.md](security-guidelines.md) for the adherence gate. +- `tlsprofile.EffectiveSpec(profile)` resolves `nil`/empty profile to `Intermediate` (API default semantics); supports `Old`/`Intermediate`/`Modern`/`Custom`. Never assume `Custom.Custom` is non-nil — return an error if missing. +- `CertManagerWebhookTLSArgs` / `CertManagerOperandMetricsTLSArgs` emit `--tls-min-version`, `--metrics-tls-min-version`, and cipher args, converting OpenSSL cipher names to IANA via `libgocrypto.OpenSSLToIANACipherSuites`. +- **Critical rule**: when `MinTLSVersion == VersionTLS13`, do **not** emit `--tls-cipher-suites` / `--metrics-tls-cipher-suites` at all — Go ignores cipher config for TLS 1.3 and setting it is a documented anti-pattern (`CertManagerCipherSuiteArgKeys` exists so other hooks can strip these keys, see `common.StripArgsByKeys`). +- Applied by `common.WithClusterTLSProfileFromAPIServer(apiServerInformer)` (`pkg/controller/common/tls_profile_hook.go`), registered only when the shared `config.openshift.io` informer factory is available — i.e. when the `Infrastructure` resource is discoverable (§6). This is the same gate used for `withCloudCredentials`; there is no independent `APIServer`-specific discovery check. This hook must run **before** `withUnsupportedArgsOverrideHook` in the hook chain so break-glass `unsupportedConfigOverrides` args always win over cluster-derived TLS args — do not reorder. +- Unknown deployment names are skipped gracefully (hook returns `nil`, args untouched). A missing `APIServer/cluster` object (the type is discoverable, but the singleton object doesn't exist) instead returns an error from the hook, which — like the trusted-CA/cloud-credentials errors above — is retried and sets Degraded via the library-go `DeploymentController`, not silently ignored. See `tls_profile_hook_test.go` cases `UnknownDeployment` and `APIServerNotFound`. + +## 4. Cloud Credentials (ambient credentials) + +- **Rule**: The operator only **mounts an existing Secret** into the `cert-manager` controller deployment for ACME DNS-01 ambient credentials (AWS Route53 / GCP Cloud DNS). It **must never create, own, or reconcile a `CredentialsRequest`** object — that is a cluster-admin/`ccoctl` responsibility documented for humans in [docs/cloud_credentials.md](cloud_credentials.md), not operator code. +- Flag `--cloud-credentials-secret` (`operator.CloudCredentialSecret`) names a Secret that **must already exist** in the `cert-manager` namespace before it is referenced. +- Implementation: `withCloudCredentials` (`pkg/controller/certmanager/credentials_request.go`): + - No-op for every deployment except `certmanagerControllerDeployment` — never mount on webhook/cainjector. + - No-op if `secretName` is empty. + - If the secret does not exist yet, return the same retryable `"(Retrying) cloud secret %q doesn't exist"` pattern as trusted-CA — this likewise sets `Degraded=True` on the library-go `DeploymentController` while it keeps retrying; it never becomes a permanent, un-retried failure (see [error-handling-guidelines.md](error-handling-guidelines.md) §7). + - Reads `Infrastructure/cluster` `.status.platformStatus.Type` to decide mount shape: + - **AWS**: volume `cloud-credentials` → secret mounted at `/.aws`; also sets `AWS_SDK_LOAD_CONFIG=1` env (required for the AWS SDK to bind `role_arn` from the credentials file). + - **GCP**: volume `cloud-credentials` → secret key `service_account.json` projected to `/.config/gcloud/application_default_credentials.json`. + - Any other platform type → hard error `"unsupported cloud provider %q for mounting cloud credentials secret"`. Do not silently ignore unsupported platforms. +- This hook is only registered when the Infrastructure informer is `Applicable()` (§6) — on clusters without the Infrastructure API, cloud-credentials mounting is skipped entirely, not defaulted. +- `ClusterIssuer` gets ambient credentials by default; `Issuer` requires `--issuer-ambient-credentials` on the controller (a container arg, set via CertManager spec overrides, not this hook). + +## 5. Monitoring + +- **Rule**: The operator does not create `ServiceMonitor`/`PodMonitor` objects itself. It only advertises CSV annotation `operatorframework.io/cluster-monitoring: "true"` and ships operand Services with the standard label set (`app.kubernetes.io/{name,instance,component}`) in `bindata/`; enabling scrape is a cluster/admin action. +- Operands expose Prometheus metrics on port `9402` at `/metrics` for all three components (controller, webhook, cainjector). +- Admins must enable OpenShift user-workload monitoring (`enableUserWorkload: true` in `cluster-monitoring-config`) and apply a `ServiceMonitor` selecting `cert-manager` namespace services by the labels above. +- Metrics TLS (when the cluster TLS profile requires it) is layered on via `tlsprofile.CertManagerOperandMetricsTLSArgs`, not via this doc's ServiceMonitor step — do not conflate the two; a metrics TLS listener still needs `insecureSkipVerify`/TLS config on the scraping side if enabled. +- See [docs/operand_metrics.md](operand_metrics.md) for full scrape/query walkthrough. + +## 6. Optional APIs (Infrastructure / APIServer discovery) + +- **Rule**: `config.openshift.io` types (`Infrastructure`, `APIServer`) are **not guaranteed to exist** (e.g. some OKD/hypershift-guest topologies). Never assume they are present — always discover first. +- `pkg/operator/starter.go` performs discovery once at startup: `utils.NewResourceDiscoverer(infraGVR, configClient.Discovery())` + `utils.InitInformerIfAvailable(...)` returns an `OptionalInformer[SharedInformerFactory]`. + - `Discover()` treats a discovery `NotFound` as `(false, nil)` — absence is not an error. + - The shared informer factory (covers both `Infrastructures` and `APIServers`) is only constructed if discovery of the `Infrastructure` resource succeeds; `APIServer` itself is not separately probed. +- Downstream consumers **must** gate on `optInfraInformer.Applicable()` before: + - Starting the informer factory (`starter.go`). + - Registering `withCloudCredentials` and `common.WithClusterTLSProfileFromAPIServer` hooks, and adding their informers to the controller's informer list (`generic_deployment_controller.go`). +- When not applicable: cloud-credentials mounting and cluster-TLS-profile args are simply skipped (operand runs with whatever static args ship in bindata) — this must not crash or degrade the operator. +- Never add a new optional CRD/API dependency without following this same discover-then-gate pattern; do not add a hard informer `Start()`/lister `Get()` call without a prior `Applicable()`/discovery check. + +## Reference Index + +| Concern | Flag | Hook | Doc | +|---|---|---|---| +| Proxy | (OLM-injected env) | `withProxyEnv` | [proxy.md](proxy.md) | +| Trusted CA | `--trusted-ca-configmap` | `withCAConfigMap` | [proxy.md](proxy.md) | +| TLS profile | (from `APIServer/cluster`) | `common.WithClusterTLSProfileFromAPIServer` | `pkg/tlsprofile` | +| Cloud credentials | `--cloud-credentials-secret` | `withCloudCredentials` | [cloud_credentials.md](cloud_credentials.md) | +| Monitoring | CSV `operatorframework.io/cluster-monitoring: "true"` | n/a (bindata Service labels) | [operand_metrics.md](operand_metrics.md) | +| Optional APIs | n/a | `utils.InitInformerIfAvailable` + `Applicable()` | `pkg/operator/starter.go` | diff --git a/docs/olm-packaging-guidelines.md b/docs/olm-packaging-guidelines.md new file mode 100644 index 000000000..53a84c3bb --- /dev/null +++ b/docs/olm-packaging-guidelines.md @@ -0,0 +1,186 @@ +# OLM Packaging Guidelines + +This document describes how the cert-manager Operator for Red Hat OpenShift is packaged for +Operator Lifecycle Manager (OLM), and the checklists to follow when bumping versions or changing +the deployment/RBAC surface. + +## Layout + +| Path | Purpose | +|------|---------| +| `config/manager/manager.yaml` | Source of truth for the operator `Deployment` (env, RBAC subject, image placeholder `controller:latest`). Also holds the hand-maintained `RELATED_IMAGE_*`/`*_OPERAND_IMAGE_VERSION` literal values (see below). | +| `config/rbac/*.yaml` | `ClusterRole`/`Role` generated by `controller-gen` from `+kubebuilder:rbac` markers in `pkg/controller/**`. | +| `config/manifests/bases/cert-manager-operator.clusterserviceversion.yaml` | Hand-maintained CSV **base** (metadata, description, icon, links, `replaces`, `skipRange`, uninstall message). Deployment/RBAC/CRD sections are stripped/regenerated by `operator-sdk`. | +| `config/manifests/kustomization.yaml` | Combines the CSV base with `../default`, `../samples`, `../scorecard`, `../console`; replaces `containerImage` annotation from the built `Deployment` image. | +| `bundle/manifests/*.clusterserviceversion.yaml` | **Generated** final CSV shipped in the bundle. Do not hand-edit. | +| `bundle/manifests/*_crd.yaml` | Generated CRDs (from `config/crd/bases`). | +| `bundle/metadata/annotations.yaml` | Bundle package name, channels, mediatype — must match `bundle.Dockerfile` LABELs. | +| `bundle/tests/scorecard/config.yaml` | Scorecard test config, from `config/scorecard`. | + +## Make targets + +```bash +make manifests # controller-gen rbac+crd+webhook -> config/rbac, config/crd +make bundle # kustomize + operator-sdk generate bundle + operator-sdk bundle validate ./bundle +make bundle-build / bundle-push +make catalog-build / catalog-push +``` + +`make bundle` sets the CSV `image:`/`containerImage` from `IMG` and regenerates `bundle/manifests`/`bundle/metadata` (via `operator-sdk generate bundle`) from whatever is currently checked into `config/manager/manager.yaml` and `config/manifests/bases/...csv.yaml`, then runs `operator-sdk bundle validate`. It does **not** derive `RELATED_IMAGE_*`/`OPERAND_IMAGE_VERSION` values from the `CERT_MANAGER_VERSION`/`ISTIO_CSR_VERSION`/`TRUST_MANAGER_VERSION` Make variables — those env values are hand-maintained literals in `config/manager/manager.yaml` that must be edited to match before running `make bundle` (see below). **Never hand-edit generated `bundle/` files** — edit the source (`config/manager/manager.yaml`, `config/manifests/bases/...csv.yaml`, `config/rbac/*`) and re-run `make bundle`. + +## Version knobs (Makefile) + +| Variable | Meaning | +|----------|---------| +| `DEFAULT_VERSION` | Fallback for `BUNDLE_VERSION`/`CATALOG_VERSION`. | +| `BUNDLE_VERSION` | Operator version embedded in CSV `spec.version` / `metadata.name` suffix (via `operator-sdk generate bundle --version`). | +| `IMG_VERSION` | Operator image tag (`IMG`). | +| `CATALOG_VERSION` | Catalog/index image tag. | +| `CERT_MANAGER_VERSION`, `ISTIO_CSR_VERSION`, `TRUST_MANAGER_VERSION` | Operand image versions; consumed by `make update-manifests` to pull upstream manifests into `bindata/`. Must be kept **manually** in sync with the hardcoded `RELATED_IMAGE_*`/`*_OPERAND_IMAGE_VERSION` values in `config/manager/manager.yaml` — bumping this variable alone does not update the Deployment/CSV. | +| `CHANNELS` / `DEFAULT_CHANNEL` | e.g. `stable-v1,stable-v1.20` / `stable-v1`, must match `bundle/metadata/annotations.yaml`. | + +All are overridable, e.g. `make bundle BUNDLE_VERSION=1.21.0 CERT_MANAGER_VERSION=v1.21.0`. + +## `replaces` / `skipRange` + +Set in `config/manifests/bases/cert-manager-operator.clusterserviceversion.yaml`: + +```yaml +metadata: + annotations: + olm.skipRange: '>=1.19.0 <1.20.0' +spec: + replaces: cert-manager-operator.v1.19.0 + version: 1.20.0 +``` + +- `replaces` must point at the exact previous CSV name shipped in the same channel (upgrade edge). +- `olm.skipRange` should cover all previously-published z-streams so upgrades from any older + installed version resolve correctly; bump the lower bound as old minors are dropped and the + upper bound to the new `` being released. +- Update **both** when cutting a new minor/z-stream; keep `bundle/metadata/annotations.yaml` + channel labels consistent with the release train. + +## `relatedImages` and `RELATED_IMAGE_*` + +The CSV `spec.relatedImages` list is auto-populated by `operator-sdk generate bundle` (via `make +bundle`) from every `RELATED_IMAGE_*`-prefixed container `env` entry in `config/manager/manager.yaml` +— but `manager.yaml`'s literal image tags are **not** derived from `CERT_MANAGER_VERSION` / +`ISTIO_CSR_VERSION` / `TRUST_MANAGER_VERSION`; they must be hand-edited to match whenever those +Make variables change, or `relatedImages` will silently drift from the intended operand versions. + +| CSV `relatedImages.name` | Env var | Consumed by | +|---|---|---| +| `cert-manager-controller` | `RELATED_IMAGE_CERT_MANAGER_CONTROLLER` | `pkg/controller/certmanager/related_images.go` | +| `cert-manager-webhook` | `RELATED_IMAGE_CERT_MANAGER_WEBHOOK` | same | +| `cert-manager-ca-injector` | `RELATED_IMAGE_CERT_MANAGER_CA_INJECTOR` | same | +| `cert-manager-acmesolver` | `RELATED_IMAGE_CERT_MANAGER_ACMESOLVER` | same | +| `cert-manager-istiocsr` | `RELATED_IMAGE_CERT_MANAGER_ISTIOCSR` | `pkg/controller/istiocsr/constants.go` | +| `cert-manager-trust-manager` | `RELATED_IMAGE_CERT_MANAGER_TRUST_MANAGER` | `pkg/controller/trustmanager/constants.go` | + +`OPERAND_IMAGE_VERSION`, `ISTIOCSR_OPERAND_IMAGE_VERSION`, `TRUSTMANAGER_OPERAND_IMAGE_VERSION`, +`OPERATOR_IMAGE_VERSION` are likewise hardcoded in `config/manager/manager.yaml` for +disconnected/digest-pinned overrides and must be bumped by hand alongside the image references. +Adding a **new** operand image requires: a new `RELATED_IMAGE_*` env in +`config/manager/manager.yaml`, an entry in the `imageEnvMap` (or equivalent constants file) that +reads it, and re-running `make bundle` so the corresponding `relatedImages` entry is generated — +missing any of these will make `operator-sdk bundle validate` / disconnected mirroring fail. + +## RBAC changes + +1. Add/update `+kubebuilder:rbac:groups=...,resources=...,verbs=...` markers on the owning + controller in `pkg/controller/**`. +2. Run `make manifests` — regenerates `config/rbac/role.yaml` (`ClusterRole`) via + `controller-gen rbac:roleName=manager-role`. +3. Run `make bundle` — propagates the role into the CSV's + `spec.install.spec.clusterPermissions`/`permissions` (bundle CSV, not `config/rbac/role.yaml`, + is what OLM actually applies on install). +4. Never edit `bundle/manifests/*clusterserviceversion.yaml` RBAC rules by hand; they are + overwritten on the next `make bundle`. + +## Uninstall message / manual operand cleanup + +CSV carries: + +```yaml +metadata: + annotations: + console.openshift.io/disable-operand-delete: "true" + operator.openshift.io/uninstall-message: >- + The cert-manager Operator for Red Hat OpenShift will be removed from cert-manager-operator + namespace. ... All operands created by the operator will need to be manually cleaned up. ... +``` + +- `console.openshift.io/disable-operand-delete: "true"` prevents the console from offering an + automatic operand-delete option on uninstall. +- The operator does **not** run finalizers/garbage-collection on `CertManager`/`IstioCSR`/ + `TrustManager` CRs or their downstream Certificates/Issuers/Bundles/Deployments — uninstalling + the subscription/CSV leaves operands running. Keep the uninstall-message text and the linked + product docs page current whenever cleanup steps or CRs change. + +## Bindata regeneration + +Vendored operand manifests live in `bindata/{cert-manager-deployment,istio-csr,trust-manager}` and +are compiled into `pkg/operator/assets/bindata.go`: + +```bash +make update-manifests # hack/update-{cert-manager,istio-csr,trust-manager}-manifests.sh +make update-bindata # regenerates pkg/operator/assets/bindata.go from bindata/ +make update # generate + update-manifests + update-bindata +make verify-bindata # CI check that bindata.go matches bindata/ sources +``` + +Never hand-edit `bindata/` YAML sourced from upstream Helm/jsonnet templates or +`pkg/operator/assets/bindata.go` directly — re-run the `update-*-manifests.sh` script for the +affected operand, then `make update-bindata`. + +## Verify scripts + +| Script | Checks | Wired into `make verify-scripts`? | +|---|---|---| +| `hack/verify-bundle.sh` | Runs `make bundle` and fails if the tree has uncommitted diffs. | Yes | +| `hack/verify-deepcopy.sh` | Generated `zz_generated.deepcopy.go` is current. | Yes | +| `hack/verify-clientgen.sh` | Generated clientset/informers/listers/applyconfigurations are current. | Yes | +| `hack/verify-crds.sh` / `verify-crds-version-upgrade.sh` | CRD manifests match `config/crd/bases` and upgrades are compatible. | No — run directly | + +Run `make verify` (or `make verify-scripts`) locally before sending a PR that touches +`api/`, `config/`, or `bindata/`; also run `hack/verify-crds*.sh` directly if operand CRDs changed, +since they are not invoked by any Makefile target in this repo. + +## Checklist: bumping an operand version + +- [ ] Update `CERT_MANAGER_VERSION` / `ISTIO_CSR_VERSION` / `TRUST_MANAGER_VERSION` in `Makefile`. +- [ ] `make update-manifests` (pulls new upstream manifests into `bindata/`). +- [ ] `make update-bindata` and confirm `hack/verify-bindata` (via `make verify-scripts`) is clean. +- [ ] Manually update the matching `RELATED_IMAGE_*` / `*_OPERAND_IMAGE_VERSION` literal values in + `config/manager/manager.yaml` — these are **not** derived from the Makefile variables above. +- [ ] `make bundle` — regenerates the bundle CSV/`relatedImages` from the updated `manager.yaml`; + diff the result to confirm the versions actually changed. +- [ ] Update the operand version link in the CSV `description` field (`config/manifests/bases/...csv.yaml`). +- [ ] `hack/verify-crds.sh` / `verify-crds-version-upgrade.sh` pass if operand CRDs changed (run + directly; not part of `make verify-scripts`). + +## Checklist: bumping the operator version + +- [ ] Update `DEFAULT_VERSION`/`BUNDLE_VERSION` (Makefile) to the new `X.Y.Z`. +- [ ] Update `replaces` to the previous CSV name and widen `olm.skipRange` upper bound. +- [ ] Update `CHANNELS`/`DEFAULT_CHANNEL` and `bundle/metadata/annotations.yaml` if a new channel is introduced. +- [ ] `make bundle` and review the full `bundle/manifests` diff (name, version, image digests). +- [ ] `hack/verify-bundle.sh` clean; `operator-sdk bundle validate ./bundle` passes. + +## Checklist: adding an env var / RBAC rule / relatedImage + +- [ ] Env var: add to `config/manager/manager.yaml` container `env`; if operand-image-related, wire it through the matching `pkg/controller/*/constants.go`, then rerun `make bundle` — for a `RELATED_IMAGE_*` var this auto-populates the `relatedImages` entry; never hand-add one to the generated bundle CSV. +- [ ] RBAC: add `+kubebuilder:rbac` marker on the controller; `make manifests`; `make bundle`; diff `config/rbac/role.yaml` and the CSV `clusterPermissions`. +- [ ] relatedImage: must have a paired `RELATED_IMAGE_*` env and an override in the corresponding controller's image-resolution map/constants — validated by `operator-sdk bundle validate`. +- [ ] Regenerate bindata if the change also touches an operand manifest template. + +## Checklist: manual operand cleanup on uninstall + +- [ ] If the change adds a new CR kind or operand resource kind, update the + `operator.openshift.io/uninstall-message` annotation and the linked uninstall docs page to list + it as something the admin must delete manually. +- [ ] Confirm `console.openshift.io/disable-operand-delete: "true"` remains set (no finalizer-based + auto-cleanup exists). +- [ ] Add/update the resource in the product uninstall documentation referenced by the CSV + annotation, not just in-repo docs. diff --git a/docs/operator-controllers-guidelines.md b/docs/operator-controllers-guidelines.md new file mode 100644 index 000000000..51cd7f6d9 --- /dev/null +++ b/docs/operator-controllers-guidelines.md @@ -0,0 +1,188 @@ +# Operator Controllers Guidelines + +Contributor guide for `pkg/controller/{certmanager,istiocsr,trustmanager,common}`. This +repo runs **two controller frameworks side by side** — read this before adding or +modifying reconciliation logic. See `ai-docs/architecture/components.md` and +`ai-docs/decisions/adr-0001-dual-controller-frameworks.md` / `adr-0002-apply-strategies.md` +for the full rationale; this doc is the actionable "how to work in this code" summary. + +## 1. Dual Framework — know which one you're in + +| | `pkg/controller/certmanager` | `pkg/controller/istiocsr`, `pkg/controller/trustmanager` | +|---|---|---| +| Framework | OpenShift **library-go** (`staticresourcecontroller`, `deploymentcontroller`, `factory.Controller`) | **controller-runtime** (`sigs.k8s.io/controller-runtime`) | +| Wired from | `pkg/controller/certmanager/cert_manager_controller_set.go` + `pkg/operator/starter.go` | `pkg/operator/setup_manager.go` (`NewControllerManager`) | +| Started | Always, unconditionally | Only if feature gate enabled (see §9) | +| Status type | `operatorv1.OperatorStatus` (via `OperatorClient`) | Custom `ConditionalStatus` | +| CR scope | Cluster-scoped `cluster` | IstioCSR: namespaced `default`; TrustManager: cluster-scoped `cluster` | + +**Rule**: Never assume the two stacks share helpers, lifecycle, or status shape. Code +written against one is not portable to the other without adaptation — this has been a +recurring source of incorrect PRs and hallucinated docs (ADR-0001). + +## 2. Per-Controller Apply Methods — do not assume uniformity + +Each package uses a **different** resource-apply idiom. Match the existing idiom in the +package you're editing; do not "fix" one to look like another without an ADR update. + +| 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` | + +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. + +## 3. Greenfield Rule: New ctrl-runtime Operand → Copy TrustManager + +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)`. + 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. +4. Use `common.FromClientError` / `HandleReconcileResult` for error → status translation + (§5), and `r.UpdateWithRetry` for status/finalizer updates on the CR itself (SSA does + not apply to the CR's own status subresource pattern used here). +5. Register the controller in `pkg/operator/setup_manager.go` + (`setupTrustManagerController`-style function) and wire a feature gate (§9). + +IstioCSR is a **reference for Reconcile-loop shape and finalizer plumbing only** — +not for apply method. + +## 4. Cache Constraints (`pkg/operator/setup_manager.go`) + +The unified manager builds **one shared cache** across all enabled controllers via +`newUnifiedCacheBuilder` / `buildCacheObjectList`. Hard constraints: + +- A `cache.ByObject` label selector is **one `labels.Selector` per GVK**. Kubernetes + selectors AND requirements across different keys — there is no way to express + `app in (...) OR watched-by exists` in one selector. +- **ConfigMaps must not use a label-filtered cache.** TrustManager needs its managed + ConfigMaps (labeled) *and* the unlabeled `cert-manager-operator-trusted-ca-bundle` + ConfigMap; IstioCSR needs its managed ConfigMaps *and* user ConfigMaps carrying a + different label key (`istiocsr.openshift.operator.io/watched-by`). ConfigMaps use the + default unfiltered informer; each controller applies **predicate-level filtering** + instead. +- **Issuer/ClusterIssuer must not use a managed-label cache filter.** IstioCSR reconciles + user-created Issuers referenced from its spec, which are never labeled by the operator. +- When two controllers share a GVK that *is* safely label-filterable (e.g. + `appsv1.Deployment`), `addControllerCacheConfig` merges values into a single + `labelKey In (value1, value2)` requirement — do not add a second, conflicting selector + for the same type. + +If you add a new watched type, first ask: "could any controller need to watch instances +of this type that lack the managed-resource label?" If yes, leave it off the managed +lists (`istioCSRManagedResources` / `trustManagerManagedResources`) and filter in a +predicate instead of a cache selector. + +## 5. Errors & Status (ctrl-runtime path) + +`pkg/controller/common/errors.go` defines `ReconcileError` with three reasons: + +- `IrrecoverableError` — no requeue, sets Degraded (via `HandleReconcileResult`). +- `RetryRequiredError` — requeues with backoff. +- `MultipleInstanceError` — singleton-CR violation. + +`FromClientError` classifies apiserver errors: `Unauthorized` / `Forbidden` / `Invalid` / +`BadRequest` / `ServiceUnavailable` → irrecoverable; everything else → retry-required. +Always wrap client errors through `common.FromClientError(err, "message %s", args...)` +rather than returning raw errors from Reconcile. + +## 6. Finalizers — Add/Remove Implemented, Cleanup is TODO + +Both IstioCSR and TrustManager **do** add/remove their own CR finalizer correctly +(`addFinalizer` / `removeFinalizer` in each package's `utils.go`, using +`controllerutil.AddFinalizer` + `r.UpdateWithRetry`). What is **not** implemented: + +```go +// pkg/controller/{istiocsr,trustmanager}/controller.go — cleanUp() +// TODO: For GA, handle cleaning up of resources created for installing the operand. +// As per Non-Goals in the enhancement, removing the CR does NOT remove the +// Deployment or its associated resources. +r.eventRecorder.Eventf(cr, corev1.EventTypeWarning, "RemoveDeployment", + "... remove all resources created for deployment manually") +return false, nil // never blocks finalizer removal +``` + +**Do not** assume deleting an IstioCSR/TrustManager CR garbage-collects the operand +Deployment, RBAC, Services, etc. — it does not, by design (Non-Goal in the enhancement), +and `cleanUp` is warn-only. If you're asked to implement real cleanup, this is GA-scoped +work requiring SME review (owner references vs. explicit delete vs. a validating webhook +for IstioCSR's GRPC-endpoint-in-use case — see the comment in `istiocsr/controller.go`). + +## 7. `certmanager_controller.go` is a Placeholder — Do Not Extend + +```go +// TODO: This is just a placeholder controller to contain all the required rbac +// in a single place. Needs to be deleted later. +type CertManagerReconciler struct{ ... } +func (r *CertManagerReconciler) Reconcile(...) (ctrl.Result, error) { + return ctrl.Result{}, nil // no-op +} +``` + +`CertManagerReconciler` in `pkg/controller/certmanager/certmanager_controller.go` exists +**only** to host `+kubebuilder:rbac` markers for manifest generation; its `Reconcile` is a +no-op and it is never registered with any manager in `starter.go`. Real CertManager +reconciliation lives in the library-go controller set +(`cert_manager_controller_set.go` → `default_cert_manager_controller.go`, +`cert_manager_*_deployment.go`, `cert_manager_networkpolicy.go`). **Never** point new +CertManager logic at this file or assume it runs. + +## 8. Shared `pkg/controller/common` Utilities — Reuse, Don't Reimplement + +- **Client**: `CtrlClient`, `NewClient`, `UpdateWithRetry` (CR status/finalizer updates + with conflict retry). +- **Errors**: `ReconcileError`, `NewIrrecoverableError`/`NewRetryRequiredError`/ + `NewMultipleInstanceError`, `FromClientError`, `FromError`, `HandleReconcileResult`. +- **Constants**: `ManagedResourceLabelKey` (`app`), `OperatorNamespace`, + `TrustedCABundleConfigMapName`, `TrustedCABundleKey`. +- **Args**: `MergeContainerArgs`, `ParseArgMap`, `StripArgsByKeys`, `ArgKeysSet`. +- **TLS**: `WithClusterTLSProfileFromAPIServer` (applies APIServer TLS profile as operand + args; do not set explicit cipher-suite args for TLS 1.3). +- **Metadata**: `UpdateName`/`UpdateNamespace`/`UpdateResourceLabels`, + `ObjectMetadataModified` — use these when decoding bindata assets into objects + (`common.DecodeObjBytes[*T](codecs, gv, assets.MustAsset(name))`) instead of hand-rolling + metadata mutation. +- **Validation**: `pkg/controller/common/validation.go`, `core_validation_helpers.go`. + +Before writing a new helper for arg merging, label diffing, or client retry, check +`pkg/controller/common` first — both IstioCSR and TrustManager already depend on it, and +duplicating logic per-package is an anti-pattern. + +## 9. Feature-Gate Wiring — the full chain + +1. Define in `api/operator/v1alpha1/features.go` (`IstioCSR` GA/default-true, + `TrustManager` TechPreview/default-false). +2. Register in `pkg/features` (`SetupWithFlagValue`, `NewFeatureGateState`). +3. `pkg/operator/starter.go`: `setupFeatureGates` parses `--unsupported-addon-features` + and (best-effort, fail-open with retries) reads `featuregates/cluster`; never aborts + startup on persistent failure. +4. Runtime checks are **internal-only** — `features.IsIstioCSRFeatureGateEnabled()` and + `(*FeatureGateState).IsTrustManagerFeatureGateEnabled()`. `passesClusterPreviewGating` + in `features.go` still exists but is **unused** by the TrustManager enable path + (cluster `FeatureSet` gating was removed, CM-1141) — don't resurrect it without + confirming intent. +5. `starter.go` only calls `NewControllerManager` (§1) if **either** gate is enabled; each + enabled gate adds its resource list to the shared cache (§4) and its reconciler to the + manager via `setup{IstioCSR,TrustManager}Controller` in `setup_manager.go`. + +Adding a fourth gated operand means touching all five steps above, plus the greenfield +checklist in §3. + +## 10. Quick Checklist Before Sending a PR + +- [ ] Confirmed which framework (§1) and which apply idiom (§2) the touched package uses. +- [ ] New ctrl-runtime operand copies TrustManager SSA, not IstioCSR Create+Update (§3). +- [ ] No new label-selector cache filter on ConfigMaps or Issuer/ClusterIssuer (§4). +- [ ] Errors routed through `common.FromClientError`/`ReconcileError` (§5). +- [ ] Did not assume finalizer removal cleans up operand resources (§6). +- [ ] Did not add logic to `certmanager_controller.go` (§7). +- [ ] Reused `pkg/controller/common` helpers instead of duplicating (§8). +- [ ] Feature-gate changes updated all five wiring points (§9). diff --git a/docs/security-guidelines.md b/docs/security-guidelines.md new file mode 100644 index 000000000..67e0b2682 --- /dev/null +++ b/docs/security-guidelines.md @@ -0,0 +1,160 @@ +# Security Guidelines + +Repo-specific security conventions for the cert-manager-operator. This is not a generic +Kubernetes/OpenShift security primer — see [AGENTS.md](../AGENTS.md) and `ai-docs/` for +architecture. Rules below are enforced by code, CRD validation, or CI unless noted. + +## NetworkPolicies + +- Default network policies are **opt-in and immutable once enabled**: `CertManager.spec.defaultNetworkPolicy` + has a CEL rule rejecting `"true" → "false"` transitions (`api/operator/v1alpha1/certmanager_types.go`). + Do not add a code path to unset it — the API will reject the update anyway. +- Enabling `defaultNetworkPolicy` applies a **deny-all** baseline + (`bindata/networkpolicies/cert-manager-deny-all-networkpolicy.yaml`) plus explicit allow rules for + API server egress, DNS egress, webhook ingress, and metrics ingress. Any new egress need (e.g. a new + ACME/DNS solver) must ship as an additional allow-egress bindata file, not by loosening deny-all. +- User-defined `spec.networkPolicies` egress rules are **append-only per component**: `name` + + `componentName` pairs are immutable via CEL (`oldSelf.all(...)`, max 50 entries). Only + `ComponentName: CoreController` is currently allowed by the enum (`+kubebuilder:validation:Enum=CoreController` + in `certmanager_types.go`), even though `getPodSelectorForComponent` already has cases for + `CAInjector`/`Webhook` — exposing those components requires extending the CRD enum, not touching + the selector function. +- Ingress rules for user policies are **not** user-configurable — the operator derives ingress from the + ports it already runs (`cert_manager_networkpolicy.go`); only expose an `egress`-only API surface for + new NetworkPolicy config. +- Static default policies (`certManagerNetworkPolicyAssetFiles`) are one-way: once applied they are + never deleted (`nil` delete-func in `WithConditionalResources`) — don't rely on cleanup-on-disable. + +## TLS Profile + +- Cipher/min-version handling for cert-manager operands is centralized in `pkg/tlsprofile` and + `pkg/controller/common/tls_profile_hook.go`; do not hand-roll TLS flags in a new deployment hook. +- **Prefer** stripping cipher-suite args, don't set them, when the effective `MinTLSVersion` is + `VersionTLS13` — Go ignores cipher config for TLS 1.3 and cert-manager will fail to start with them + set. `StripArgsByKeys(..., ArgKeysSet(tlsprofile.CertManagerCipherSuiteArgKeys))` already does this; + reuse it rather than special-casing TLS 1.3 elsewhere. +- The hook only activates when `apiserver.config.openshift.io/cluster` `spec.tlsAdherence` is + `StrictAllComponents` (via `libgocrypto.ShouldHonorClusterTLSProfile`). A nil/absent profile + resolves to `Intermediate`, not "no TLS enforcement" — don't treat missing config as "TLS profile + disabled". +- CSV declares `tls-profiles: "false"` despite the runtime hook existing — this is a known + inconsistency (see `ai-docs/architecture/components.md`); don't assume the annotation reflects actual + behavior when auditing. + +```go +// Correct pattern for a new TLS-aware arg (pkg/controller/common/tls_profile_hook.go) +if effective.MinTLSVersion == configv1.VersionTLS13 { + sourceArgs = StripArgsByKeys(sourceArgs, ArgKeysSet(tlsprofile.CertManagerCipherSuiteArgKeys)) +} +``` + +## RBAC + +- RBAC is defined in **three places that must stay in sync** and are regenerated, not hand-edited: + 1. `+kubebuilder:rbac` markers on controller-runtime reconcilers (`pkg/controller/{istiocsr,trustmanager}/controller.go`) + 2. `config/rbac/role.yaml` (generated by `controller-gen` via `make manifests`) + 3. `bundle/manifests/cert-manager-operator.clusterserviceversion.yaml` `clusterPermissions` (generated by `make bundle`) +- `hack/verify-bundle.sh` fails CI if `make bundle` produces a diff — always run `make bundle` (or + `make manifests generate`) after touching RBAC-relevant code and commit the resulting diff. +- **Never** widen permissions in the CSV directly to unblock a local test; add/adjust the + `+kubebuilder:rbac` marker on the reconciler function instead so the generated artifacts match. +- 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)` + pattern in `rbacs.go`, not a blind re-apply. + +## Secrets Handling (TrustManager `secretTargets`) + +- TrustManager's ability to write `Bundle` output to Secrets is gated by + `spec.trustManagerConfig.secretTargets.policy` (`Disabled` default, or `Custom`). This is enforced by + CRD CEL rules, not just documentation: + - `Custom` requires a non-empty `authorizedSecrets` list. + - Any other policy value requires `authorizedSecrets` to be empty. +- **Prefer** scoping RBAC by `resourceNames` over a wildcard grant: `Custom` grants cluster-wide + **read** (`get;list;watch` on all secrets, required for trust-manager's internal reconciliation) but + **write** (`create;update;patch;delete`) is restricted via `ResourceNames: sortedSecrets` to exactly + `authorizedSecrets` (`appendSecretTargetRules` in `pkg/controller/trustmanager/rbacs.go`). Do not + change this to a wildcard `resources: ["secrets"]` write grant. +- `authorizedSecrets` is sorted before use so ClusterRole diffing (`clusterRoleModified`) is + deterministic — if you touch this list's construction, keep the sort or you'll get spurious + reconcile churn. +- The AWS/GCP cloud-credentials secret (below) and the trusted-CA ConfigMap are read via informer + `Lister().Get(...)`, never fetched with a live API call in the hot reconcile path — follow that + pattern for any new "mount an existing secret" feature (`credentials_request.go`, + `deployment_overrides.go`). + +## Cloud Credentials + +- The operator **mounts an existing Secret**; it does not create `CredentialsRequest` objects + (`docs/cloud_credentials.md`, `credentials_request.go` despite its filename). Do not add + CredentialsRequest-creation logic here — that belongs to `ccoctl`/cloud-credential-operator flows + documented for cluster admins. +- `--cloud-credentials-secret` / `CLOUD_CREDENTIALS_SECRET_NAME` only mounts into the **CertManager + controller** deployment (`deploymentName != certmanagerControllerDeployment` short-circuits to a + no-op) — don't expect webhook/cainjector to receive this mount. +- Platform type is read from `Infrastructure/cluster` and only `AWSPlatformType` / `GCPPlatformType` + are supported; any other platform with a configured secret name is a hard error + (`unsupported cloud provider %q`), not a silent skip. +- AWS mounts the secret at `/.aws` and forces `AWS_SDK_LOAD_CONFIG=1` (required for `role_arn` + resolution); GCP mounts only the `service_account.json` key, remapped to + `application_default_credentials.json` under `/.config/gcloud`. Match these exact paths/keys if you + extend cloud credential support — the AWS SDK and gcloud libraries require them. + +## Trusted CA + +- `TRUSTED_CA_CONFIGMAP_NAME` / `--trusted-ca-configmap` mounts a ConfigMap the **cluster admin** + creates and labels with `config.openshift.io/inject-trusted-cabundle=true` — the operator never + creates or labels this ConfigMap itself (`docs/proxy.md`). If a bundle isn't found yet, the hook + returns a retryable error (`(Retrying) trusted CA config map %q doesn't exist`); don't convert this + to a fatal/Degraded condition. +- The CA bundle is always mounted at the fixed path + `/etc/pki/tls/certs/cert-manager-tls-ca-bundle.crt` with `subPath: ca-bundle.crt` + (`deployment_overrides.go`) — this matches Go's default root cert lookup path; changing it silently + breaks trust for any consuming controller. +- TrustManager has a separate mechanism: it watches the CNO-injected ConfigMap + `cert-manager-operator-trusted-ca-bundle` directly rather than reusing the CertManager + `--trusted-ca-configmap` flag — don't assume the two controllers share trusted-CA wiring. + +## Pod Security + +- All bundled operand and operator deployments already set the restricted-profile fields explicitly in + their static YAML/CSV — **preserve these on any new container**, don't rely on cluster-level defaults: + `runAsNonRoot: true`, `seccompProfile.type: RuntimeDefault` (pod-level), and per-container + `allowPrivilegeEscalation: false`, `capabilities.drop: [ALL]`, `readOnlyRootFilesystem: true`. +- The operator container additionally sets `privileged: false` explicitly and mounts an `emptyDir` at + `/tmp` since the root filesystem is read-only — any new operator container writing to disk needs the + same `emptyDir` + volume mount pattern, not a relaxed `readOnlyRootFilesystem`. +- The operator image runs as a numeric non-root user (`USER 65532:65532` in `Dockerfile`) rather than + relying solely on the pod `securityContext` — keep this if you change the base image. +- `nodeSelector: {kubernetes.io/os: linux}` is set on operand pods; the operator deployment instead uses + `nodeAffinity` restricting `kubernetes.io/arch` to the supported set — don't mix the two patterns for + the same workload. + +## Vulnerability Scanning + +- `make govulncheck` (`hack/govulncheck.sh`) runs `golang.org/x/vuln` against `./...` and is meant to be + wired into CI; it does **not** fail the raw `govulncheck` invocation but the wrapper script fails the + build if new (non-allow-listed) vulnerabilities appear. +- Known-acceptable vulnerabilities are allow-listed by ID in `KNOWN_VULNS_PATTERN` inside + `hack/govulncheck.sh`, each with an inline comment justifying why it's out of scope (e.g. vendored + k8s.io/kubernetes server-side code that doesn't affect this client). **Prefer** adding a reviewed, + linked justification to this pattern over suppressing scan failures another way. +- The script asserts the scan actually ran (`grep -q "pkg.go.dev"` in results) before evaluating + pass/fail — if you change the invocation, keep some equivalent "did the tool actually execute" guard + so a broken scan doesn't silently report success. +- FIPS: `hack/go-fips.sh` sets `GOEXPERIMENT=strictfipsruntime` and `-tags=strictfipsruntime,openssl` + when available, and **warns loudly** (not an error) when the local toolchain lacks FIPS support. Per + `ai-docs/architecture/components.md` this warning means the resulting binary must not ship to + CI/production — don't treat a successful non-FIPS local build as CI-equivalent. + +## Generated Artifacts — Don't Hand-Edit Security-Relevant Files + +Applies broadly across the sections above: `pkg/operator/assets/bindata.go`, `bindata/**` operand YAML +(including the NetworkPolicy and deployment `securityContext` files referenced here), and +`bundle/manifests/*clusterserviceversion.yaml` RBAC blocks are all generated +(`make generate` / `make update-manifests` / `make bundle`). Edit the source (kustomize base, upstream +chart pin, or `+kubebuilder:rbac` marker) and regenerate; `hack/verify-bundle.sh` and `verify-bindata` +enforce this in CI. diff --git a/docs/testing-guidelines.md b/docs/testing-guidelines.md new file mode 100644 index 000000000..bbc7bdc61 --- /dev/null +++ b/docs/testing-guidelines.md @@ -0,0 +1,161 @@ +# Testing Guidelines + +Repo-specific testing reference for `cert-manager-operator`. For generic testing philosophy, see the platform docs linked from `ai-docs/CERT_MANAGER_OPERATOR_TESTING.md`. + +## Test Suites at a Glance + +| Suite | Location | Make target | Framework | +|-------|----------|-------------|-----------| +| Unit | `pkg/**/*_test.go` | `make test-unit` | stdlib `testing` (+ table tests) | +| API / envtest | `test/apis`, `api/operator/v1alpha1/tests/**` | `make test-apis` | Ginkgo + envtest | +| E2E | `test/e2e/*_test.go` (build tag `e2e`) | `make test-e2e` | Ginkgo (labeled specs) | + +`make test` runs `manifests generate vet test-apis test-unit` — it does **not** run e2e. Run `make test-e2e` separately against a live cluster. + +## When to Use Which Suite + +- **Unit (`pkg/...`)**: default choice for controller logic — reconcile branches, resource-builder functions (`getDeploymentObject`, `getServiceObject`, etc.), validation, and drift detection. Fast, no cluster/envtest needed. Use the counterfeiter fake (`fakes.FakeCtrlClient`) to isolate a single reconciler function. +- **API tests (`test/apis`)**: use when you change CRD schema, defaults, or CEL `XValidation` rules under `api/operator/v1alpha1`. These run real API-server admission (via envtest) against fixtures in `api/operator/v1alpha1/tests/{certmanagers,istiocsrs,trustmanagers}.operator.openshift.io/`. A unit test cannot validate CEL/OpenAPI schema enforcement — only envtest can. +- **E2E (`test/e2e`)**: use for anything that requires a real OpenShift cluster and real operands — Route/Ingress integration, actual cert-manager `Certificate`/`Issuer` issuance, IstioCSR/TrustManager install and drift-repair against a live deployment, feature-gate behavior, ServiceMesh smoke tests. Requires a running cluster with the operator already installed and cert-manager operands `Available` (see `test-e2e-wait-for-stable-state`). + +Rule of thumb: if you can fake the client and assert a call count/patch payload, write a unit test. If the assertion needs a real informer/cache, real admission webhook, or a real pod running, it belongs in `test/apis` (schema only) or `test/e2e` (behavior). + +## Make Targets + +```bash +make test # manifests + generate + vet + test-apis + test-unit +make test-unit # go test ./... (excludes test/e2e, test/apis, test/utils) +make test-apis # envtest + ginkgo against api/operator/v1alpha1/tests fixtures +make test-e2e # ginkgo e2e suite against a live cluster (requires oc login) +make test-e2e-debug-cluster # dump operator/operand pod status, CSVs, CRDs on failure +``` + +Focused unit runs: + +```bash +go test -count=1 ./pkg/controller/trustmanager/... +go test -count=1 ./pkg/controller/istiocsr/... +go test -count=1 ./pkg/controller/certmanager/... +go test -count=1 ./pkg/features/... +``` + +Focused e2e runs use the `TEST` variable (passed to `-run`) plus the label filter: + +```bash +make test-e2e TEST='TestE2E' E2E_GINKGO_LABEL_FILTER='Feature:TrustManager' +``` + +`test-unit` filters packages with `grep -vE 'test/[e2e|apis|utils]'` — new top-level test helper packages under `test/` should follow the existing `test/apis`, `test/e2e`, `test/utils` naming so they're auto-excluded from unit runs. + +## Unit Test Patterns Per Controller + +Apply strategy differs **per controller stack** — assert the call that actually matches the controller under test, don't assume one apply pattern everywhere: + +| 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` | + +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. + +Common shape for TrustManager/IstioCSR unit tests (table-driven, one `Reconciler` + fresh fake per case): + +```go +mock := &fakes.FakeCtrlClient{} +mock.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { + // return drifted / matching / not-found object depending on subtest +}) +r := testReconciler(t) +err := r.createOrApplyDeployment(cr, labels, annotations, hash) +assertError(t, err, tt.wantErr) +if got := mock.PatchCallCount(); got != tt.wantPatchCount { ... } // 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. + +## Fake Client (counterfeiter) + +`common.CtrlClient` is the mockable seam: + +```go +//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -generate +//counterfeiter:generate -o fakes . CtrlClient +type CtrlClient interface { Get, List, StatusUpdate, Update, UpdateWithRetry, Create, Delete, Patch, Exists } +``` + +- Regenerate after changing the interface: `make generate-fakes` (wraps `go generate ./...`). +- The fake lives at `pkg/controller/common/fakes/fake_ctrl_client.go` — **never hand-edit it**; re-run generation instead. +- Only IstioCSR and TrustManager (ctrl-runtime stack) use `fakes.FakeCtrlClient`. CertManager (library-go stack) does not implement/consume `CtrlClient`. +- Use `Calls(func(...) {...})` to stub behavior per subtest and `CallCount()` to assert invocation counts — this is the standard pattern across `pkg/controller/{istiocsr,trustmanager}/*_test.go`. + +## API Tests (`make test-apis`) + +- Runs `hack/test-apis.sh`, which invokes `ginkgo -r -v --randomize-all --randomize-suites --keep-going --timeout=30m ./test/apis`. +- Fixtures under `api/operator/v1alpha1/tests/{certmanagers,istiocsrs,trustmanagers}.operator.openshift.io/` pair a manifest with expected accept/reject outcomes — add a new fixture pair when adding/changing a CRD field or `XValidation` rule. +- Uses envtest (`setup-envtest` + `KUBEBUILDER_ASSETS`), not a real cluster — no operand pods, no controllers running, purely API-server admission behavior. + +## E2E Tests (`test/e2e`) + +Build-tagged `e2e`; run via `go test -tags e2e ./test/e2e/...` (wrapped by `make test-e2e`). + +### Ginkgo Label Conventions + +Every top-level `Describe` carries at least a `Platform:*` label; feature-gated suites add `Feature:*` and `TechPreview`: + +```go +var _ = Describe("TrustManager", Ordered, Label("Platform:Generic", "Feature:TrustManager", "TechPreview"), func() { ... }) +var _ = Describe("TrustManager with operator feature gate disabled", Ordered, Label("Platform:Generic", "Feature:TrustManager", "TechPreview:Inverted"), func() { ... }) +var _ = Describe("Istio-CSR", Ordered, Label("Platform:Generic", "Feature:IstioCSR"), func() { ... }) +``` + +Cloud-provider-specific `Context`s inside ACME DNS-01 tests add `Platform:` + `CredentialsMode:*`: + +```go +Context("with AWS Route53", Label("Platform:AWS", "CredentialsMode:Mint"), func() { ... }) +Context("with AWS Route53 in STS environment", Label("Platform:AWS", "CredentialsMode:Manual"), func() { ... }) +``` + +Individual `It`s may add case IDs (`Label("ISTIOCSR-001")`, `Label("OSM-SMOKE-TC-001")`) for traceability. `TechPreview:Inverted` labels a suite that verifies behavior when a feature gate is **disabled**, distinct from the normal `TechPreview` (gate enabled) suite for the same feature — don't merge these into one `Describe`. + +### Default & Custom Label Filters + +The Makefile default runs AWS/Generic, Mint-credentials, non-ServiceMesh specs: + +```make +E2E_GINKGO_LABEL_FILTER ?= Platform: isSubsetOf {AWS,Generic} && CredentialsMode: isSubsetOf {Mint} && !Feature:ServiceMesh +``` + +passed through as: + +```make +-ginkgo.label-filter='$(E2E_GINKGO_LABEL_FILTER)' +``` + +**Quoting matters**: the filter is a single shell argument containing spaces, `{}`, `&&`, `!`. Always single-quote the whole `-ginkgo.label-filter='...'` value (as the Makefile does) — don't split it across unquoted `make VAR=...` tokens, or the shell will word-split on spaces/braces and silently pass a truncated filter. When overriding from the CLI, quote the override too: + +```bash +make test-e2e E2E_GINKGO_LABEL_FILTER='Feature:TrustManager && !TechPreview:Inverted' +``` + +### Component-Specific E2E Tips + +1. TrustManager e2e assumes the `TrustManager` feature gate is enabled (`--unsupported-addon-features=TrustManager=true`) unless running the `TechPreview:Inverted` suite. +2. Create operator CRs with singleton names matching production (`cluster` for CertManager/TrustManager, `default` for IstioCSR) — non-standard names are rejected by controller logic, not just tested. +3. Operand pods live in `cert-manager` namespace; operator logs in `cert-manager-operator`. `make test-e2e-wait-for-stable-state` gates the run on `cert-manager{,-cainjector,-webhook}` deployments being `Available`. +4. ServiceMesh smoke specs (`Feature:IstioCSR-ServiceMesh`) are excluded by the default filter (`!Feature:ServiceMesh`) — they need `E2E_OSM_ISTIO_VERSION`/`E2E_OSM_OPERATOR_VERSION` and a real OSM install; run them explicitly. +5. On failure, run `make test-e2e-debug-cluster` (or let the wait-for-stable-state step auto-trigger it) to dump operator/operand pod state, `ClusterOperator`, CSVs, and CRDs. + +## Coverage Builds + +`image-build-coverage`, `image-push-coverage`, `e2e-coverage-collect` build/collect coverage from an instrumented operator image running e2e — only needed when measuring e2e coverage, not for routine unit/API test iteration. + +## What Not to Duplicate Here + +Generic envtest bootstrap theory and OpenShift e2e framework tutorials belong in the platform `ai-docs`, not this file or `ai-docs/CERT_MANAGER_OPERATOR_TESTING.md`. + +## See Also + +- `ai-docs/CERT_MANAGER_OPERATOR_TESTING.md` +- `ai-docs/architecture/components.md` (apply-strategy table) +- `ai-docs/decisions/adr-0002-apply-strategies.md` From fe9e3fa2d93d51fd8ed3b894990e43b6f8d77c27 Mon Sep 17 00:00:00 2001 From: Arun Maurya Date: Tue, 4 Aug 2026 15:13:19 +0530 Subject: [PATCH 2/3] OAPE-878: Restructure agentic docs into harness-evals for OpenSpec. Move ai-docs and coding guidelines under harness-evals/harness-docs, add stage eval stubs, and keep AGENTS.md at the repo root so /opsx-constitute and OpenSpec eval gates can consume them. --- AGENTS.md | 41 ++++++++++--------- harness-evals/evals/code-generation_eval.yaml | 12 ++++++ harness-evals/evals/plan_eval.yaml | 3 ++ harness-evals/evals/repo-assessment_eval.yaml | 3 ++ harness-evals/evals/tasks_eval.yaml | 3 ++ .../CERT_MANAGER_OPERATOR_DEVELOPMENT.md | 14 +++---- .../CERT_MANAGER_OPERATOR_TESTING.md | 4 +- .../harness-docs}/api-contracts-guidelines.md | 10 ++--- .../harness-docs}/architecture/components.md | 13 +++--- .../adr-0001-dual-controller-frameworks.md | 0 .../decisions/adr-0002-apply-strategies.md | 0 .../decisions/adr-0003-feature-gates.md | 0 .../harness-docs}/decisions/adr-template.md | 0 .../harness-docs}/domain/certmanager.md | 0 .../harness-docs}/domain/istiocsr.md | 0 .../harness-docs}/domain/trustmanager.md | 0 .../error-handling-guidelines.md | 2 +- .../harness-docs}/exec-plans/README.md | 0 .../harness-docs}/fips-guidelines.md | 2 +- .../harness-docs}/integration-guidelines.md | 8 ++-- .../harness-docs}/olm-packaging-guidelines.md | 0 .../operator-controllers-guidelines.md | 4 +- .../harness-docs}/references/ecosystem.md | 28 ++++++------- .../harness-docs}/references/enhancements.md | 2 +- .../harness-docs}/security-guidelines.md | 10 ++--- .../harness-docs}/testing-guidelines.md | 10 ++--- 26 files changed, 96 insertions(+), 73 deletions(-) create mode 100644 harness-evals/evals/code-generation_eval.yaml create mode 100644 harness-evals/evals/plan_eval.yaml create mode 100644 harness-evals/evals/repo-assessment_eval.yaml create mode 100644 harness-evals/evals/tasks_eval.yaml rename {ai-docs => harness-evals/harness-docs}/CERT_MANAGER_OPERATOR_DEVELOPMENT.md (86%) rename {ai-docs => harness-evals/harness-docs}/CERT_MANAGER_OPERATOR_TESTING.md (95%) rename {docs => harness-evals/harness-docs}/api-contracts-guidelines.md (96%) rename {ai-docs => harness-evals/harness-docs}/architecture/components.md (95%) rename {ai-docs => harness-evals/harness-docs}/decisions/adr-0001-dual-controller-frameworks.md (100%) rename {ai-docs => harness-evals/harness-docs}/decisions/adr-0002-apply-strategies.md (100%) rename {ai-docs => harness-evals/harness-docs}/decisions/adr-0003-feature-gates.md (100%) rename {ai-docs => harness-evals/harness-docs}/decisions/adr-template.md (100%) rename {ai-docs => harness-evals/harness-docs}/domain/certmanager.md (100%) rename {ai-docs => harness-evals/harness-docs}/domain/istiocsr.md (100%) rename {ai-docs => harness-evals/harness-docs}/domain/trustmanager.md (100%) rename {docs => harness-evals/harness-docs}/error-handling-guidelines.md (99%) rename {ai-docs => harness-evals/harness-docs}/exec-plans/README.md (100%) rename {docs => harness-evals/harness-docs}/fips-guidelines.md (99%) rename {docs => harness-evals/harness-docs}/integration-guidelines.md (97%) rename {docs => harness-evals/harness-docs}/olm-packaging-guidelines.md (100%) rename {docs => harness-evals/harness-docs}/operator-controllers-guidelines.md (98%) rename {ai-docs => harness-evals/harness-docs}/references/ecosystem.md (70%) rename {ai-docs => harness-evals/harness-docs}/references/enhancements.md (91%) rename {docs => harness-evals/harness-docs}/security-guidelines.md (95%) rename {docs => harness-evals/harness-docs}/testing-guidelines.md (97%) diff --git a/AGENTS.md b/AGENTS.md index 85b132506..40990ea33 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,7 +4,7 @@ **Repository**: [openshift/cert-manager-operator](https://github.com/openshift/cert-manager-operator) **Default branch**: `master` | **Go**: 1.26.0 -> **Retrieval-first**: Prefer `ai-docs/` for architecture/API; prefer `docs/*-guidelines.md` for deep playbooks. Platform hub: [openshift/enhancements/ai-docs](https://github.com/openshift/enhancements/tree/master/ai-docs). +> **Retrieval-first**: Prefer `harness-evals/harness-docs/` for architecture, ADRs, and coding playbooks. Platform hub: [openshift/enhancements/ai-docs](https://github.com/openshift/enhancements/tree/master/ai-docs). ## What is Cert Manager Operator? @@ -16,19 +16,19 @@ Deploys/configures upstream **cert-manager** (and optionally **istio-csr** / **t | CR | Scope / name | Stack | Gate | |----|--------------|-------|------| -| [CertManager](ai-docs/domain/certmanager.md) | Cluster / `cluster` | library-go `resourceapply` | Always on | -| [IstioCSR](ai-docs/domain/istiocsr.md) | Namespaced / `default` | Create+Update | `IstioCSR` GA default true | -| [TrustManager](ai-docs/domain/trustmanager.md) | Cluster / `cluster` | **SSA** | `TrustManager` TP default false | +| [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 | +| [TrustManager](harness-evals/harness-docs/domain/trustmanager.md) | Cluster / `cluster` | **SSA** | `TrustManager` TP default false | **Operands**: Certificate/Issuer/… → cert-manager; Bundle → trust-manager. **Quick Start**: `oc get certmanager cluster -o yaml` | `make local-run` ## Critical Patterns -1. **Never assume uniform SSA** — CertManager library-go; IstioCSR Create+`UpdateWithRetry`; TrustManager only SSA + field owner `trust-manager-controller`. [components.md](ai-docs/architecture/components.md) +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) 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](ai-docs/decisions/adr-0002-apply-strategies.md) -4. **Feature gates** — `--unsupported-addon-features`; five touchpoints; no cluster FeatureSet for TrustManager (CM-1141). [ADR-0003](ai-docs/decisions/adr-0003-feature-gates.md) +3. **Greenfield = TrustManager** — copy SSA + `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`. 7. **Cache** — no label-filtered ConfigMap/Issuer caches; use predicates when unlabeled watches are required. @@ -42,30 +42,31 @@ Deploys/configures upstream **cert-manager** (and optionally **istio-csr** / **t | Shared | `pkg/controller/common` (`CtrlClient`, errors, TLS/validation) | | Features | `api/operator/v1alpha1/features.go`, `pkg/features` | | Images | `RELATED_IMAGE_CERT_MANAGER_{CONTROLLER,WEBHOOK,CA_INJECTOR,ACMESOLVER,ISTIOCSR,TRUST_MANAGER}` | -| ADRs | [0001](ai-docs/decisions/adr-0001-dual-controller-frameworks.md) · [0002](ai-docs/decisions/adr-0002-apply-strategies.md) · [0003](ai-docs/decisions/adr-0003-feature-gates.md) | +| ADRs | [0001](harness-evals/harness-docs/decisions/adr-0001-dual-controller-frameworks.md) · [0002](harness-evals/harness-docs/decisions/adr-0002-apply-strategies.md) · [0003](harness-evals/harness-docs/decisions/adr-0003-feature-gates.md) | ## Documentation Map ```text -ai-docs/ # architecture, domain CRDs, ADRs, DEVELOPMENT, TESTING -docs/*-guidelines.md # deep agent playbooks (controllers, errors, OLM, FIPS, …) -docs/{proxy,cloud_credentials,operand_metrics}.md +harness-evals/ + harness-docs/ # architecture, domain, ADRs, *-guidelines, DEVELOPMENT, TESTING + evals/ # OpenSpec stage eval gates (repo-assessment/plan/tasks/code-generation) +docs/{proxy,cloud_credentials,operand_metrics}.md # human product docs ``` | Need | Start here | |------|------------| -| Spec / CEL / NetworkPolicy enum | `ai-docs/domain/*.md` | -| Errors, cache, TLS, cloud, FIPS | `ai-docs/architecture/components.md` | -| Controllers / apply / gates | `docs/operator-controllers-guidelines.md` | -| OLM / relatedImages | `docs/olm-packaging-guidelines.md` + DEVELOPMENT | -| Unit assert Patch vs Update | `ai-docs/CERT_MANAGER_OPERATOR_TESTING.md` | -| FIPS build rules | `docs/fips-guidelines.md` | +| Spec / CEL / NetworkPolicy enum | `harness-evals/harness-docs/domain/*.md` | +| Errors, cache, TLS, cloud, FIPS | `harness-evals/harness-docs/architecture/components.md` | +| Controllers / apply / gates | `harness-evals/harness-docs/operator-controllers-guidelines.md` | +| OLM / relatedImages | `harness-evals/harness-docs/olm-packaging-guidelines.md` + DEVELOPMENT | +| Unit assert Patch vs Update | `harness-evals/harness-docs/CERT_MANAGER_OPERATOR_TESTING.md` | +| FIPS build rules | `harness-evals/harness-docs/fips-guidelines.md` | -**AI Agent Path**: domain → components.md → matching `docs/*-guidelines.md` → DEVELOPMENT/TESTING +**AI Agent Path**: domain → components.md → matching `*-guidelines.md` in harness-docs → DEVELOPMENT/TESTING -**Also**: [DEVELOPMENT](ai-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md) · [TESTING](ai-docs/CERT_MANAGER_OPERATOR_TESTING.md) · [enhancements](ai-docs/references/enhancements.md) · [ecosystem](ai-docs/references/ecosystem.md) +**Also**: [DEVELOPMENT](harness-evals/harness-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md) · [TESTING](harness-evals/harness-docs/CERT_MANAGER_OPERATOR_TESTING.md) · [enhancements](harness-evals/harness-docs/references/enhancements.md) · [ecosystem](harness-evals/harness-docs/references/ecosystem.md) -**Guideline index**: `docs/{operator-controllers,error-handling,api-contracts,olm-packaging,integration,security,testing,fips}-guidelines.md` +**Guideline index**: `harness-evals/harness-docs/{operator-controllers,error-handling,api-contracts,olm-packaging,integration,security,testing,fips}-guidelines.md` **Platform**: [hub](https://github.com/openshift/enhancements/tree/master/ai-docs) · [operator-patterns](https://github.com/openshift/enhancements/tree/master/ai-docs/platform/operator-patterns) · [status-conditions](https://github.com/openshift/enhancements/blob/master/ai-docs/platform/operator-patterns/status-conditions.md) diff --git a/harness-evals/evals/code-generation_eval.yaml b/harness-evals/evals/code-generation_eval.yaml new file mode 100644 index 000000000..073a14307 --- /dev/null +++ b/harness-evals/evals/code-generation_eval.yaml @@ -0,0 +1,12 @@ +stage: code-generation +artifact: fork working copy (code + tests) +version: 1 +eval_count: 0 +oape_commands: + - api-generate + - api-generate-tests + - api-implement + - e2e-generate + - manual + - any +evals: [] diff --git a/harness-evals/evals/plan_eval.yaml b/harness-evals/evals/plan_eval.yaml new file mode 100644 index 000000000..f762e64bd --- /dev/null +++ b/harness-evals/evals/plan_eval.yaml @@ -0,0 +1,3 @@ +stage: plan +template: templates/plan-template.md +evals: [] diff --git a/harness-evals/evals/repo-assessment_eval.yaml b/harness-evals/evals/repo-assessment_eval.yaml new file mode 100644 index 000000000..5949bede8 --- /dev/null +++ b/harness-evals/evals/repo-assessment_eval.yaml @@ -0,0 +1,3 @@ +stage: repo-assessment +template: templates/repo-assessment-template.md +evals: [] diff --git a/harness-evals/evals/tasks_eval.yaml b/harness-evals/evals/tasks_eval.yaml new file mode 100644 index 000000000..561ef159b --- /dev/null +++ b/harness-evals/evals/tasks_eval.yaml @@ -0,0 +1,3 @@ +stage: tasks +template: templates/tasks-template.md +evals: [] diff --git a/ai-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md b/harness-evals/harness-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md similarity index 86% rename from ai-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md rename to harness-evals/harness-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md index 8aaa5a63b..ff9096d0a 100644 --- a/ai-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md +++ b/harness-evals/harness-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md @@ -1,7 +1,7 @@ # Cert Manager Operator - Development Guide > Layout / controller comparison: [architecture/components.md](./architecture/components.md). -> Agent playbooks: [docs/*-guidelines.md](../docs/) (operator-controllers, olm-packaging, fips, integration, …). +> Agent playbooks: [*-guidelines.md](.) (operator-controllers, olm-packaging, fips, integration, …). ## Quick Start @@ -14,7 +14,7 @@ make local-run # RELATED_IMAGE_* + versions from Makefile make build && make generate && make update-manifests && make verify ``` -FIPS: `make build` sources `hack/go-fips.sh`. WARN (non-FIPS) builds are **local-only** — never CI/prod/image push. Keep `go.mod` replace `openshift/jetstack-cert-manager` lockstep with `CERT_MANAGER_VERSION`. See [docs/fips-guidelines.md](../docs/fips-guidelines.md). +FIPS: `make build` sources `hack/go-fips.sh`. WARN (non-FIPS) builds are **local-only** — never CI/prod/image push. Keep `go.mod` replace `openshift/jetstack-cert-manager` lockstep with `CERT_MANAGER_VERSION`. See [fips-guidelines.md](./fips-guidelines.md). ## Common Tasks @@ -40,7 +40,7 @@ FIPS: `make build` sources `hack/go-fips.sh`. WARN (non-FIPS) builds are **local | `cert-manager-istiocsr` | `RELATED_IMAGE_CERT_MANAGER_ISTIOCSR` | `istiocsr/constants.go` | | `cert-manager-trust-manager` | `RELATED_IMAGE_CERT_MANAGER_TRUST_MANAGER` | `trustmanager/constants.go` | -`RELATED_IMAGE_*` / `*_OPERAND_IMAGE_VERSION` literals in **`config/manager/manager.yaml` are hand-maintained** — bumping Makefile `CERT_*_VERSION` alone does **not** update them. Edit `manager.yaml` to match, then `make bundle` (auto-fills CSV `relatedImages` from `RELATED_IMAGE_*` env). New operand image also needs controller constants/map. See [docs/olm-packaging-guidelines.md](../docs/olm-packaging-guidelines.md). +`RELATED_IMAGE_*` / `*_OPERAND_IMAGE_VERSION` literals in **`config/manager/manager.yaml` are hand-maintained** — bumping Makefile `CERT_*_VERSION` alone does **not** update them. Edit `manager.yaml` to match, then `make bundle` (auto-fills CSV `relatedImages` from `RELATED_IMAGE_*` env). New operand image also needs controller constants/map. See [olm-packaging-guidelines.md](./olm-packaging-guidelines.md). ### Bump operand versions @@ -69,13 +69,13 @@ FIPS: `make build` sources `hack/go-fips.sh`. WARN (non-FIPS) builds are **local | Topic | Detail | |-------|--------| | Namespaces | Operator `cert-manager-operator`; operands `cert-manager` | -| Cloud creds | AWS `/.aws` + `AWS_SDK_LOAD_CONFIG=1`; GCP ADC path — `docs/cloud_credentials.md` | -| Trusted CA | Fixed mount path; missing CM = retryable — `docs/integration-guidelines.md` | +| Cloud creds | AWS `/.aws` + `AWS_SDK_LOAD_CONFIG=1`; GCP ADC path — `../../docs/cloud_credentials.md` | +| Trusted CA | Fixed mount path; missing CM = retryable — `integration-guidelines.md` | | Uninstall | Manual operand cleanup; `console.openshift.io/disable-operand-delete: "true"` | ## See Also - [CERT_MANAGER_OPERATOR_TESTING.md](./CERT_MANAGER_OPERATOR_TESTING.md) - [architecture/components.md](./architecture/components.md) -- [docs/operator-controllers-guidelines.md](../docs/operator-controllers-guidelines.md) -- [docs/olm-packaging-guidelines.md](../docs/olm-packaging-guidelines.md) +- [operator-controllers-guidelines.md](./operator-controllers-guidelines.md) +- [olm-packaging-guidelines.md](./olm-packaging-guidelines.md) diff --git a/ai-docs/CERT_MANAGER_OPERATOR_TESTING.md b/harness-evals/harness-docs/CERT_MANAGER_OPERATOR_TESTING.md similarity index 95% rename from ai-docs/CERT_MANAGER_OPERATOR_TESTING.md rename to harness-evals/harness-docs/CERT_MANAGER_OPERATOR_TESTING.md index d4612382c..a92dcb46e 100644 --- a/ai-docs/CERT_MANAGER_OPERATOR_TESTING.md +++ b/harness-evals/harness-docs/CERT_MANAGER_OPERATOR_TESTING.md @@ -1,6 +1,6 @@ # Cert Manager Operator - Testing Guide -> **Generic Testing Practices**: See [Platform ai-docs](https://github.com/openshift/enhancements/tree/master/ai-docs). Deep playbook: [docs/testing-guidelines.md](../docs/testing-guidelines.md). +> **Generic Testing Practices**: See [Platform ai-docs](https://github.com/openshift/enhancements/tree/master/ai-docs). Deep playbook: [testing-guidelines.md](./testing-guidelines.md). ## Test Layout @@ -54,6 +54,6 @@ Tips: singleton CR names (`cluster` / `default`); operands in `cert-manager` NS; ## See Also -- [docs/testing-guidelines.md](../docs/testing-guidelines.md) +- [testing-guidelines.md](./testing-guidelines.md) - [CERT_MANAGER_OPERATOR_DEVELOPMENT.md](./CERT_MANAGER_OPERATOR_DEVELOPMENT.md) - [architecture/components.md](./architecture/components.md) diff --git a/docs/api-contracts-guidelines.md b/harness-evals/harness-docs/api-contracts-guidelines.md similarity index 96% rename from docs/api-contracts-guidelines.md rename to harness-evals/harness-docs/api-contracts-guidelines.md index 863ad23a4..bd778fc79 100644 --- a/docs/api-contracts-guidelines.md +++ b/harness-evals/harness-docs/api-contracts-guidelines.md @@ -1,6 +1,6 @@ # API Contracts Guidelines -Rules for changing or adding to `api/operator/v1alpha1/`. Read `ai-docs/domain/*.md` +Rules for changing or adding to `api/operator/v1alpha1/`. Read `domain/*.md` and the relevant ADR before touching a CR's Spec/Status. **Verify every field against the Go types under `api/operator/v1alpha1/` — never invent, assume, or backfill a field from upstream cert-manager/trust-manager/istio-csr docs.** @@ -32,7 +32,7 @@ Rules: ## 2. Spec/Status Conventions -Two status models coexist by design (see `ai-docs/decisions/adr-0001-dual-controller-frameworks.md`). +Two status models coexist by design (see `decisions/adr-0001-dual-controller-frameworks.md`). Never assume one is used everywhere: - **CertManager**: embeds OpenShift `apiv1.OperatorSpec` / `apiv1.OperatorStatus` @@ -42,7 +42,7 @@ Never assume one is used everywhere: (`api/operator/v1alpha1/meta.go`) — just `Conditions []metav1.Condition`. Condition types are `Ready` / `Degraded`; reasons are `Failed` / `Ready` / `Progressing` (`conditions.go`). Do not introduce new condition types without updating - `conditions.go` and both `ai-docs/domain/istiocsr.md` and `trustmanager.md`. + `conditions.go` and both `domain/istiocsr.md` and `trustmanager.md`. Structural conventions to follow for any new CR or field: @@ -121,7 +121,7 @@ objects — a new namespaced singleton needs the same controller-side check. `IstioCSR` (GA, default **true**) and `TrustManager` (TechPreview, default **false**) are gated in `api/operator/v1alpha1/features.go` via `featuregate.Feature`/`OperatorFeatureGates`, enabled at runtime through -`--unsupported-addon-features` (see `ai-docs/decisions/adr-0003-feature-gates.md`). +`--unsupported-addon-features` (see `decisions/adr-0003-feature-gates.md`). `CertManager` is always on and ungated. Rules for a new feature-gated CR: @@ -174,6 +174,6 @@ operand bindata) and regenerate. - Don't document `UnsupportedConfigOverrides.{Controller,Webhook,CAInjector}` as first-class `Spec` fields — they're helper types for parsing `OperatorSpec.UnsupportedConfigOverrides` (a `RawExtension`), not real schema fields. -- When documenting a CR, cross-check `ai-docs/domain/.md` — if your reading of +- When documenting a CR, cross-check `domain/.md` — if your reading of the Go type conflicts with that doc, prefer the Go source and flag the doc as possibly stale rather than propagating the conflict into new docs. diff --git a/ai-docs/architecture/components.md b/harness-evals/harness-docs/architecture/components.md similarity index 95% rename from ai-docs/architecture/components.md rename to harness-evals/harness-docs/architecture/components.md index 84544d57c..c3a7e4bcd 100644 --- a/ai-docs/architecture/components.md +++ b/harness-evals/harness-docs/architecture/components.md @@ -9,7 +9,8 @@ api/operator/v1alpha1/ # CertManager, IstioCSR, TrustManager types + feature bindata/ # Operand YAML (cert-manager, istio-csr, trust-manager, networkpolicies) — regenerated bundle/ # OLM CSV + CRDs config/ # Kustomize sources for manifests/bundle -docs/ # Human docs + *-guidelines.md (agent playbooks) +docs/ # Human product docs (proxy, cloud_credentials, operand_metrics) +harness-evals/harness-docs/ # Agentic docs + *-guidelines.md (OpenSpec /opsx-constitute) hack/ # update-*-manifests, go-fips, clientgen, verify scripts pkg/ cmd/operator/ # Cobra start + flags @@ -124,16 +125,16 @@ Use `pkg/controller/common` (`ReconcileError`, `FromClientError`, `HandleReconci | Integration | Mechanism | |-------------|-----------| -| Proxy | `withProxyEnv` + CSV `proxy-aware: true`; see `docs/proxy.md` | +| Proxy | `withProxyEnv` + CSV `proxy-aware: true`; see `../../../docs/proxy.md` | | Trusted CA | `--trusted-ca-configmap` mounts admin-created CM at **`/etc/pki/tls/certs/cert-manager-tls-ca-bundle.crt`** (`subPath: ca-bundle.crt`). Missing CM → library-go sets **Degraded=True and retries** (not a permanent fail). TrustManager separately watches CNO CM `cert-manager-operator-trusted-ca-bundle` — **not** the CertManager flag. | | TLS profile | Registered when **Infrastructure** informer is `Applicable()` (APIServer shares that factory; not separately discovered). Applies only when `APIServer.spec.tlsAdherence` is `StrictAllComponents`. Nil profile → **Intermediate**. TLS 1.3 → strip cipher args via `StripArgsByKeys(..., CertManagerCipherSuiteArgKeys)`. Hook **before** `withUnsupportedArgsOverrideHook`. Missing `APIServer/cluster` object → Degraded+retry (not silent). | -| Cloud credentials | Mount **existing** Secret into **controller** Deployment only — **never** create CredentialsRequest. AWS: `/.aws` + `AWS_SDK_LOAD_CONFIG=1`. GCP: `service_account.json` → `/.config/gcloud/application_default_credentials.json`. Other platforms → hard error. Missing secret → **Degraded=True + retry** (library-go). See `docs/cloud_credentials.md`. | +| Cloud credentials | Mount **existing** Secret into **controller** Deployment only — **never** create CredentialsRequest. AWS: `/.aws` + `AWS_SDK_LOAD_CONFIG=1`. GCP: `service_account.json` → `/.config/gcloud/application_default_credentials.json`. Other platforms → hard error. Missing secret → **Degraded=True + retry** (library-go). See `../../../docs/cloud_credentials.md`. | | Optional APIs | Discover Infrastructure first (`InitInformerIfAvailable` / `Applicable()`). NotFound ≠ error; skip cloud-cred + TLS hooks when absent. | -| Monitoring | CSV `operatorframework.io/cluster-monitoring: "true"`; operand Service labels in bindata; no operator-owned ServiceMonitor. See `docs/operand_metrics.md`. | -| FIPS | `hack/go-fips.sh` WARN branch = **local-only**. `go.mod` replace → `openshift/jetstack-cert-manager` should stay lockstep with `CERT_MANAGER_VERSION`. Don’t retarget upstream or silence WARN. Flip CSV `fips-compliant` only with a real guarantee change. See `docs/fips-guidelines.md`. | +| Monitoring | CSV `operatorframework.io/cluster-monitoring: "true"`; operand Service labels in bindata; no operator-owned ServiceMonitor. See `../../../docs/operand_metrics.md`. | +| FIPS | `hack/go-fips.sh` WARN branch = **local-only**. `go.mod` replace → `openshift/jetstack-cert-manager` should stay lockstep with `CERT_MANAGER_VERSION`. Don’t retarget upstream or silence WARN. Flip CSV `fips-compliant` only with a real guarantee change. See `fips-guidelines.md`. | | OLM | `replaces` / `skipRange`; uninstall requires manual operand cleanup; CSV `tls-profiles: "false"` despite runtime TLS hooks. | -Detail playbooks: `docs/{integration,security,fips,operator-controllers}-guidelines.md`. +Detail playbooks: `{integration,security,fips,operator-controllers}-guidelines.md` in harness-docs. ## Cache Constraints (`setup_manager.go`) diff --git a/ai-docs/decisions/adr-0001-dual-controller-frameworks.md b/harness-evals/harness-docs/decisions/adr-0001-dual-controller-frameworks.md similarity index 100% rename from ai-docs/decisions/adr-0001-dual-controller-frameworks.md rename to harness-evals/harness-docs/decisions/adr-0001-dual-controller-frameworks.md diff --git a/ai-docs/decisions/adr-0002-apply-strategies.md b/harness-evals/harness-docs/decisions/adr-0002-apply-strategies.md similarity index 100% rename from ai-docs/decisions/adr-0002-apply-strategies.md rename to harness-evals/harness-docs/decisions/adr-0002-apply-strategies.md diff --git a/ai-docs/decisions/adr-0003-feature-gates.md b/harness-evals/harness-docs/decisions/adr-0003-feature-gates.md similarity index 100% rename from ai-docs/decisions/adr-0003-feature-gates.md rename to harness-evals/harness-docs/decisions/adr-0003-feature-gates.md diff --git a/ai-docs/decisions/adr-template.md b/harness-evals/harness-docs/decisions/adr-template.md similarity index 100% rename from ai-docs/decisions/adr-template.md rename to harness-evals/harness-docs/decisions/adr-template.md diff --git a/ai-docs/domain/certmanager.md b/harness-evals/harness-docs/domain/certmanager.md similarity index 100% rename from ai-docs/domain/certmanager.md rename to harness-evals/harness-docs/domain/certmanager.md diff --git a/ai-docs/domain/istiocsr.md b/harness-evals/harness-docs/domain/istiocsr.md similarity index 100% rename from ai-docs/domain/istiocsr.md rename to harness-evals/harness-docs/domain/istiocsr.md diff --git a/ai-docs/domain/trustmanager.md b/harness-evals/harness-docs/domain/trustmanager.md similarity index 100% rename from ai-docs/domain/trustmanager.md rename to harness-evals/harness-docs/domain/trustmanager.md diff --git a/docs/error-handling-guidelines.md b/harness-evals/harness-docs/error-handling-guidelines.md similarity index 99% rename from docs/error-handling-guidelines.md rename to harness-evals/harness-docs/error-handling-guidelines.md index d5e95fe82..86c3d6352 100644 --- a/docs/error-handling-guidelines.md +++ b/harness-evals/harness-docs/error-handling-guidelines.md @@ -158,7 +158,7 @@ classify it. ## 8. Adding a new ctrl-runtime operand -Per [ADR-0002](../ai-docs/decisions/adr-0002-apply-strategies.md) and the repo's greenfield rule, +Per [ADR-0002](./decisions/adr-0002-apply-strategies.md) and the repo's greenfield rule, copy **TrustManager**, not IstioCSR, for both apply strategy and error handling — IstioCSR's Create+Update path and its `updateCondition` aggregation quirk (§6) are legacy, not the target pattern. At minimum: wrap every client call in `FromClientError`, classify programmatic/validation diff --git a/ai-docs/exec-plans/README.md b/harness-evals/harness-docs/exec-plans/README.md similarity index 100% rename from ai-docs/exec-plans/README.md rename to harness-evals/harness-docs/exec-plans/README.md diff --git a/docs/fips-guidelines.md b/harness-evals/harness-docs/fips-guidelines.md similarity index 99% rename from docs/fips-guidelines.md rename to harness-evals/harness-docs/fips-guidelines.md index e4fdd1dd3..a71c42355 100644 --- a/docs/fips-guidelines.md +++ b/harness-evals/harness-docs/fips-guidelines.md @@ -170,4 +170,4 @@ proceeding — do not treat it as routine build-config cleanup. | Wiring into build | `Makefile` → `build-operator` | | Operand crypto fork | `go.mod` → `replace github.com/cert-manager/cert-manager => github.com/openshift/jetstack-cert-manager ...` | | Compliance claim | `bundle/manifests/cert-manager-operator.clusterserviceversion.yaml` → `features.operators.openshift.io/fips-compliant` | -| Human-readable overview | `ai-docs/architecture/components.md` (FIPS row) | +| Human-readable overview | `architecture/components.md` (FIPS row) | diff --git a/docs/integration-guidelines.md b/harness-evals/harness-docs/integration-guidelines.md similarity index 97% rename from docs/integration-guidelines.md rename to harness-evals/harness-docs/integration-guidelines.md index 74328e9d0..9d3a9bb57 100644 --- a/docs/integration-guidelines.md +++ b/harness-evals/harness-docs/integration-guidelines.md @@ -6,7 +6,7 @@ Rules for integrating cert-manager-operator with cluster-provided OpenShift serv - **Rule**: Never hardcode proxy env vars. OLM injects `HTTP_PROXY`/`HTTPS_PROXY`/`NO_PROXY` into the operator Deployment when a cluster-wide egress proxy exists (CSV must declare `proxy-aware: true`). - The operator propagates these to operands via `withProxyEnv` (`pkg/controller/certmanager/deployment_overrides.go`), which reads `operator-lib/proxy.ReadProxyVarsFromEnv()` and merges into container env for every operand deployment. Do not add a per-deployment opt-out. -- See [docs/proxy.md](proxy.md). +- See [docs/proxy.md](../../docs/proxy.md). ## 2. Trusted Certificate Authority @@ -15,7 +15,7 @@ Rules for integrating cert-manager-operator with cluster-provided OpenShift serv - Implementation: `withCAConfigMap` (`deployment_overrides.go`) looks up the ConfigMap via the target-namespace `ConfigMapInformer` lister; if not found it returns a retryable error (`"(Retrying) trusted CA config map %q doesn't exist"`). The library-go `DeploymentController` treats every hook error the same way: it sets `Degraded=True` **and** keeps retrying via the factory's rate limiter — there is no non-degrading "just requeue" state on this stack (see [error-handling-guidelines.md](error-handling-guidelines.md) §7). It never becomes a permanent, un-retried failure, but it does surface as Degraded until the ConfigMap appears. - Mount contract is fixed and must not change: volume name `trusted-ca`, mount path `/etc/pki/tls/certs/cert-manager-tls-ca-bundle.crt`, `subPath: ca-bundle.crt` (constants `trustedCAVolumeName`, `trustedCAPath`, `defaultCABundleKey`). - If `trustedCAConfigmapName` is empty, `withCAConfigMap` is a no-op — do not add a default configmap name. -- See [docs/proxy.md](proxy.md) (Trusted CA section). +- See [docs/proxy.md](../../docs/proxy.md) (Trusted CA section). ## 3. TLS Security Profile @@ -28,7 +28,7 @@ Rules for integrating cert-manager-operator with cluster-provided OpenShift serv ## 4. Cloud Credentials (ambient credentials) -- **Rule**: The operator only **mounts an existing Secret** into the `cert-manager` controller deployment for ACME DNS-01 ambient credentials (AWS Route53 / GCP Cloud DNS). It **must never create, own, or reconcile a `CredentialsRequest`** object — that is a cluster-admin/`ccoctl` responsibility documented for humans in [docs/cloud_credentials.md](cloud_credentials.md), not operator code. +- **Rule**: The operator only **mounts an existing Secret** into the `cert-manager` controller deployment for ACME DNS-01 ambient credentials (AWS Route53 / GCP Cloud DNS). It **must never create, own, or reconcile a `CredentialsRequest`** object — that is a cluster-admin/`ccoctl` responsibility documented for humans in [docs/cloud_credentials.md](../../docs/cloud_credentials.md), not operator code. - Flag `--cloud-credentials-secret` (`operator.CloudCredentialSecret`) names a Secret that **must already exist** in the `cert-manager` namespace before it is referenced. - Implementation: `withCloudCredentials` (`pkg/controller/certmanager/credentials_request.go`): - No-op for every deployment except `certmanagerControllerDeployment` — never mount on webhook/cainjector. @@ -47,7 +47,7 @@ Rules for integrating cert-manager-operator with cluster-provided OpenShift serv - Operands expose Prometheus metrics on port `9402` at `/metrics` for all three components (controller, webhook, cainjector). - Admins must enable OpenShift user-workload monitoring (`enableUserWorkload: true` in `cluster-monitoring-config`) and apply a `ServiceMonitor` selecting `cert-manager` namespace services by the labels above. - Metrics TLS (when the cluster TLS profile requires it) is layered on via `tlsprofile.CertManagerOperandMetricsTLSArgs`, not via this doc's ServiceMonitor step — do not conflate the two; a metrics TLS listener still needs `insecureSkipVerify`/TLS config on the scraping side if enabled. -- See [docs/operand_metrics.md](operand_metrics.md) for full scrape/query walkthrough. +- See [docs/operand_metrics.md](../../docs/operand_metrics.md) for full scrape/query walkthrough. ## 6. Optional APIs (Infrastructure / APIServer discovery) diff --git a/docs/olm-packaging-guidelines.md b/harness-evals/harness-docs/olm-packaging-guidelines.md similarity index 100% rename from docs/olm-packaging-guidelines.md rename to harness-evals/harness-docs/olm-packaging-guidelines.md diff --git a/docs/operator-controllers-guidelines.md b/harness-evals/harness-docs/operator-controllers-guidelines.md similarity index 98% rename from docs/operator-controllers-guidelines.md rename to harness-evals/harness-docs/operator-controllers-guidelines.md index 51cd7f6d9..6c91dc279 100644 --- a/docs/operator-controllers-guidelines.md +++ b/harness-evals/harness-docs/operator-controllers-guidelines.md @@ -2,8 +2,8 @@ Contributor guide for `pkg/controller/{certmanager,istiocsr,trustmanager,common}`. This repo runs **two controller frameworks side by side** — read this before adding or -modifying reconciliation logic. See `ai-docs/architecture/components.md` and -`ai-docs/decisions/adr-0001-dual-controller-frameworks.md` / `adr-0002-apply-strategies.md` +modifying reconciliation logic. See `architecture/components.md` and +`decisions/adr-0001-dual-controller-frameworks.md` / `adr-0002-apply-strategies.md` for the full rationale; this doc is the actionable "how to work in this code" summary. ## 1. Dual Framework — know which one you're in diff --git a/ai-docs/references/ecosystem.md b/harness-evals/harness-docs/references/ecosystem.md similarity index 70% rename from ai-docs/references/ecosystem.md rename to harness-evals/harness-docs/references/ecosystem.md index 4400a2ce3..431a40584 100644 --- a/ai-docs/references/ecosystem.md +++ b/harness-evals/harness-docs/references/ecosystem.md @@ -1,6 +1,6 @@ # Platform Ecosystem References -Links to generic OpenShift/Kubernetes patterns in the Platform hub ([openshift/enhancements/ai-docs](https://github.com/openshift/enhancements/tree/master/ai-docs)). Component-specific patterns live in this repository’s `ai-docs/`. +Links to generic OpenShift/Kubernetes patterns in the Platform hub ([openshift/enhancements/ai-docs](https://github.com/openshift/enhancements/tree/master/ai-docs)). Component-specific patterns live in this repository’s `harness-evals/harness-docs/`. ## Operator Patterns @@ -11,13 +11,13 @@ Links to generic OpenShift/Kubernetes patterns in the Platform hub ([openshift/e **Component usage**: - CertManager embeds `operatorv1.OperatorStatus` via library-go `OperatorClient`. - IstioCSR/TrustManager use custom `ConditionalStatus` (`Ready`/`Degraded`) via `common.HandleReconcileResult` — outcome table in [architecture/components.md](../architecture/components.md). -- Deep rules: [docs/operator-controllers-guidelines.md](../../docs/operator-controllers-guidelines.md), [docs/error-handling-guidelines.md](../../docs/error-handling-guidelines.md). +- Deep rules: [operator-controllers-guidelines.md](../operator-controllers-guidelines.md), [error-handling-guidelines.md](../error-handling-guidelines.md). ## Testing Practices **Location**: Platform practices tree is still sparse in the hub; prefer component guides. -**Component usage**: [CERT_MANAGER_OPERATOR_TESTING.md](../CERT_MANAGER_OPERATOR_TESTING.md) + [docs/testing-guidelines.md](../../docs/testing-guidelines.md) (apply-path assertions, Ginkgo label quoting). +**Component usage**: [CERT_MANAGER_OPERATOR_TESTING.md](../CERT_MANAGER_OPERATOR_TESTING.md) + [testing-guidelines.md](../testing-guidelines.md) (apply-path assertions, Ginkgo label quoting). ## Security Practices @@ -25,21 +25,21 @@ Links to generic OpenShift/Kubernetes patterns in the Platform hub ([openshift/e **Component usage**: - NetworkPolicy CEL: `defaultNetworkPolicy` ratchet; user `componentName` Enum = **CoreController only**. -- Trusted CA / TLS 1.3 strip / cloud-cred mount contracts — [docs/security-guidelines.md](../../docs/security-guidelines.md), [docs/integration-guidelines.md](../../docs/integration-guidelines.md). -- FIPS — [docs/fips-guidelines.md](../../docs/fips-guidelines.md). +- Trusted CA / TLS 1.3 strip / cloud-cred mount contracts — [security-guidelines.md](../security-guidelines.md), [integration-guidelines.md](../integration-guidelines.md). +- FIPS — [fips-guidelines.md](../fips-guidelines.md). ## Agent Guideline Index (repo) | Domain | File | |--------|------| -| Controllers / apply / cache | [operator-controllers-guidelines.md](../../docs/operator-controllers-guidelines.md) | -| Errors / status | [error-handling-guidelines.md](../../docs/error-handling-guidelines.md) | -| API / CEL | [api-contracts-guidelines.md](../../docs/api-contracts-guidelines.md) | -| OLM packaging | [olm-packaging-guidelines.md](../../docs/olm-packaging-guidelines.md) | -| Integration hooks | [integration-guidelines.md](../../docs/integration-guidelines.md) | -| Security | [security-guidelines.md](../../docs/security-guidelines.md) | -| Testing | [testing-guidelines.md](../../docs/testing-guidelines.md) | -| FIPS | [fips-guidelines.md](../../docs/fips-guidelines.md) | +| Controllers / apply / cache | [operator-controllers-guidelines.md](../operator-controllers-guidelines.md) | +| Errors / status | [error-handling-guidelines.md](../error-handling-guidelines.md) | +| API / CEL | [api-contracts-guidelines.md](../api-contracts-guidelines.md) | +| OLM packaging | [olm-packaging-guidelines.md](../olm-packaging-guidelines.md) | +| Integration hooks | [integration-guidelines.md](../integration-guidelines.md) | +| Security | [security-guidelines.md](../security-guidelines.md) | +| Testing | [testing-guidelines.md](../testing-guidelines.md) | +| FIPS | [fips-guidelines.md](../fips-guidelines.md) | ## Reliability / Observability @@ -69,7 +69,7 @@ Links to generic OpenShift/Kubernetes patterns in the Platform hub ([openshift/e **Location**: Platform `ai-docs/decisions/` may be incomplete; use hub [DESIGN_PHILOSOPHY.md](https://github.com/openshift/enhancements/blob/master/ai-docs/DESIGN_PHILOSOPHY.md) and [KNOWLEDGE_GRAPH.md](https://github.com/openshift/enhancements/blob/master/ai-docs/KNOWLEDGE_GRAPH.md). -**Component-specific ADRs**: [ai-docs/decisions/](../decisions/) +**Component-specific ADRs**: [decisions/](../decisions/) --- diff --git a/ai-docs/references/enhancements.md b/harness-evals/harness-docs/references/enhancements.md similarity index 91% rename from ai-docs/references/enhancements.md rename to harness-evals/harness-docs/references/enhancements.md index 88f8b6471..9b35e5cf6 100644 --- a/ai-docs/references/enhancements.md +++ b/harness-evals/harness-docs/references/enhancements.md @@ -23,5 +23,5 @@ Related diagrams (IstioCSR): `istio-csr-create.puml` / `istio-csr-delete.puml` u ## Notes -- ADRs in `ai-docs/decisions/` capture **component architectural** choices (framework split, apply strategy, feature gates). Do not duplicate enhancement prose here. +- ADRs in `decisions/` capture **component architectural** choices (framework split, apply strategy, feature gates). Do not duplicate enhancement prose here. - Cross-component platform proposals mentioning cert-manager (OLM platform operators, Route external certs, etc.) are out of scope for this index unless they change this operator’s API or controllers. diff --git a/docs/security-guidelines.md b/harness-evals/harness-docs/security-guidelines.md similarity index 95% rename from docs/security-guidelines.md rename to harness-evals/harness-docs/security-guidelines.md index 67e0b2682..885398754 100644 --- a/docs/security-guidelines.md +++ b/harness-evals/harness-docs/security-guidelines.md @@ -1,7 +1,7 @@ # Security Guidelines Repo-specific security conventions for the cert-manager-operator. This is not a generic -Kubernetes/OpenShift security primer — see [AGENTS.md](../AGENTS.md) and `ai-docs/` for +Kubernetes/OpenShift security primer — see [AGENTS.md](../../AGENTS.md) and `harness-evals/harness-docs/` for architecture. Rules below are enforced by code, CRD validation, or CI unless noted. ## NetworkPolicies @@ -38,7 +38,7 @@ architecture. Rules below are enforced by code, CRD validation, or CI unless not resolves to `Intermediate`, not "no TLS enforcement" — don't treat missing config as "TLS profile disabled". - CSV declares `tls-profiles: "false"` despite the runtime hook existing — this is a known - inconsistency (see `ai-docs/architecture/components.md`); don't assume the annotation reflects actual + inconsistency (see `architecture/components.md`); don't assume the annotation reflects actual behavior when auditing. ```go @@ -89,7 +89,7 @@ if effective.MinTLSVersion == configv1.VersionTLS13 { ## Cloud Credentials - The operator **mounts an existing Secret**; it does not create `CredentialsRequest` objects - (`docs/cloud_credentials.md`, `credentials_request.go` despite its filename). Do not add + (`../../docs/cloud_credentials.md`, `credentials_request.go` despite its filename). Do not add CredentialsRequest-creation logic here — that belongs to `ccoctl`/cloud-credential-operator flows documented for cluster admins. - `--cloud-credentials-secret` / `CLOUD_CREDENTIALS_SECRET_NAME` only mounts into the **CertManager @@ -107,7 +107,7 @@ if effective.MinTLSVersion == configv1.VersionTLS13 { - `TRUSTED_CA_CONFIGMAP_NAME` / `--trusted-ca-configmap` mounts a ConfigMap the **cluster admin** creates and labels with `config.openshift.io/inject-trusted-cabundle=true` — the operator never - creates or labels this ConfigMap itself (`docs/proxy.md`). If a bundle isn't found yet, the hook + creates or labels this ConfigMap itself (`../../docs/proxy.md`). If a bundle isn't found yet, the hook returns a retryable error (`(Retrying) trusted CA config map %q doesn't exist`); don't convert this to a fatal/Degraded condition. - The CA bundle is always mounted at the fixed path @@ -147,7 +147,7 @@ if effective.MinTLSVersion == configv1.VersionTLS13 { so a broken scan doesn't silently report success. - FIPS: `hack/go-fips.sh` sets `GOEXPERIMENT=strictfipsruntime` and `-tags=strictfipsruntime,openssl` when available, and **warns loudly** (not an error) when the local toolchain lacks FIPS support. Per - `ai-docs/architecture/components.md` this warning means the resulting binary must not ship to + `architecture/components.md` this warning means the resulting binary must not ship to CI/production — don't treat a successful non-FIPS local build as CI-equivalent. ## Generated Artifacts — Don't Hand-Edit Security-Relevant Files diff --git a/docs/testing-guidelines.md b/harness-evals/harness-docs/testing-guidelines.md similarity index 97% rename from docs/testing-guidelines.md rename to harness-evals/harness-docs/testing-guidelines.md index bbc7bdc61..95c0539f3 100644 --- a/docs/testing-guidelines.md +++ b/harness-evals/harness-docs/testing-guidelines.md @@ -1,6 +1,6 @@ # Testing Guidelines -Repo-specific testing reference for `cert-manager-operator`. For generic testing philosophy, see the platform docs linked from `ai-docs/CERT_MANAGER_OPERATOR_TESTING.md`. +Repo-specific testing reference for `cert-manager-operator`. For generic testing philosophy, see the platform docs linked from `CERT_MANAGER_OPERATOR_TESTING.md`. ## Test Suites at a Glance @@ -152,10 +152,10 @@ make test-e2e E2E_GINKGO_LABEL_FILTER='Feature:TrustManager && !TechPreview:Inve ## What Not to Duplicate Here -Generic envtest bootstrap theory and OpenShift e2e framework tutorials belong in the platform `ai-docs`, not this file or `ai-docs/CERT_MANAGER_OPERATOR_TESTING.md`. +Generic envtest bootstrap theory and OpenShift e2e framework tutorials belong in the platform `ai-docs`, not this file or `CERT_MANAGER_OPERATOR_TESTING.md`. ## See Also -- `ai-docs/CERT_MANAGER_OPERATOR_TESTING.md` -- `ai-docs/architecture/components.md` (apply-strategy table) -- `ai-docs/decisions/adr-0002-apply-strategies.md` +- `CERT_MANAGER_OPERATOR_TESTING.md` +- `architecture/components.md` (apply-strategy table) +- `decisions/adr-0002-apply-strategies.md` From 3ba4b59c28666d924b75fc28d34904e4473f2aa5 Mon Sep 17 00:00:00 2001 From: Arun Maurya Date: Wed, 5 Aug 2026 15:36:13 +0530 Subject: [PATCH 3/3] Resolved coderabbit PR review comments --- Makefile | 2 +- .../harness-docs/api-contracts-guidelines.md | 29 ++++++++++++------- .../harness-docs/architecture/components.md | 2 +- .../adr-0001-dual-controller-frameworks.md | 2 +- .../decisions/adr-0003-feature-gates.md | 2 +- .../harness-docs/olm-packaging-guidelines.md | 24 ++++++++++----- .../harness-docs/security-guidelines.md | 5 ++-- .../harness-docs/testing-guidelines.md | 14 +++++++-- 8 files changed, 53 insertions(+), 27 deletions(-) diff --git a/Makefile b/Makefile index 101accaa9..3a035573d 100644 --- a/Makefile +++ b/Makefile @@ -273,7 +273,7 @@ test: manifests generate vet test-apis test-unit ## Run tests. .PHONY: test-unit test-unit: ## Run unit tests. - go test $$(go list ./... | grep -vE 'test/[e2e|apis|utils]') -coverprofile cover.out + go test $$(go list ./... | grep -vE 'test/(e2e|apis|utils)(/|$$)') -coverprofile cover.out # Utilize controller-runtime provided envtest for API integration test .PHONY: test-apis ## Run only the api integration tests. diff --git a/harness-evals/harness-docs/api-contracts-guidelines.md b/harness-evals/harness-docs/api-contracts-guidelines.md index bd778fc79..3cb8c5289 100644 --- a/harness-evals/harness-docs/api-contracts-guidelines.md +++ b/harness-evals/harness-docs/api-contracts-guidelines.md @@ -26,9 +26,12 @@ Rules: vendored, not owned here. - Never edit `config.openshift.io_certmanagers.yaml`. It is dead/unused; a common mistake is assuming it's the operator API. -- Adding a new operand CRD (e.g. a new `Bundle`-like resource) means adding a YAML base - under `config/crd/bases/` and wiring it into `config/crd/kustomization.yaml`, **not** - adding a Go type in `api/operator/v1alpha1/`. +- Do **not** hand-author CRD YAML under `config/crd/bases/`. Operand CRDs + (`*-crd.yaml`, `customresourcedefinition_*.yml`) are produced there by + `make update-manifests` (`hack/update-{cert-manager,trust-manager}-manifests.sh` + from upstream releases). After regeneration, wire the new file into + `config/crd/kustomization.yaml` — do **not** add a Go type in + `api/operator/v1alpha1/`. ## 2. Spec/Status Conventions @@ -145,19 +148,23 @@ Never hand-edit generated output. After changing any file in `api/operator/v1alp 1. `make generate` — regenerates deepcopy (`zz_generated.deepcopy.go`) via `controller-gen object:...` and client-gen artifacts (`hack/update-clientgen.sh`). -2. `make manifests` — regenerates CRD YAML under `config/crd/bases/` and RBAC under - `config/rbac/` via `controller-gen rbac:... crd webhook`. +2. `make manifests` — regenerates **operator** CRDs (`operator.openshift.io_*.yaml`) + under `config/crd/bases/` and RBAC under `config/rbac/` via + `controller-gen rbac:... crd webhook` from Go types in `api/operator/v1alpha1/`. 3. If operand versions/manifests changed (not API types), use - `make update-manifests` (`hack/update-{cert-manager,istio-csr,trust-manager}-manifests.sh`), - which is separate from `make generate`/`manifests`. + `make update-manifests` (`hack/update-{cert-manager,istio-csr,trust-manager}-manifests.sh`) + — refreshes **operand** CRDs in `config/crd/bases/` (`*-crd.yaml`, Bundle YAML) plus + bindata; separate from `make generate`/`manifests`. 4. `make update` runs generate + update-manifests + update-bindata together; CI's `verify-scripts` (`verify-bindata`, `verify-deepcopy.sh`, `verify-clientgen.sh`, `verify-bundle.sh`) will fail the build if generated output is stale or hand-edited. -Never manually edit: `zz_generated.deepcopy.go`, files under `config/crd/bases/`, -`pkg/operator/assets/bindata.go`, generated clientset/informers/listers/ -applyconfigurations, or `bindata/` YAML. Edit the Go types (or the source manifests for -operand bindata) and regenerate. +Never manually edit CRD **content** under `config/crd/bases/` (or +`zz_generated.deepcopy.go`, `pkg/operator/assets/bindata.go`, generated +clientset/informers/listers/applyconfigurations, or `bindata/` YAML). Edit Go +types and run `make manifests` for operator CRDs; bump upstream versions and run +`make update-manifests` for operand CRDs. You may edit +`config/crd/kustomization.yaml` to wire a newly generated CRD file. ## 7. What Not to Invent About Fields diff --git a/harness-evals/harness-docs/architecture/components.md b/harness-evals/harness-docs/architecture/components.md index c3a7e4bcd..526005a5b 100644 --- a/harness-evals/harness-docs/architecture/components.md +++ b/harness-evals/harness-docs/architecture/components.md @@ -37,7 +37,7 @@ test/{apis,e2e}/ # envtest CRD suites + Ginkgo e2e 2. **Construct** `NewCertManagerControllerSet` + `NewDefaultCertManagerController` (controllers not running yet) 3. **Start informers** (`informer.Start`, including optional infra factory when `Applicable()`) 4. **Run** each library-go controller (`go controller.Run`) -5. `setupFeatureGates` from `--unsupported-addon-features` + optional `featuregates/cluster` read (fail-closed retries; does not abort operator on persistent FeatureGate errors) +5. `setupFeatureGates` from `--unsupported-addon-features` + optional `featuregates/cluster` read (retries discovery failures; does not abort operator on persistent FeatureGate errors; optional controllers follow internal flags only) 6. If IstioCSR and/or TrustManager enabled → `NewControllerManager` → Start in goroutine 7. Block on `ctx.Done()` diff --git a/harness-evals/harness-docs/decisions/adr-0001-dual-controller-frameworks.md b/harness-evals/harness-docs/decisions/adr-0001-dual-controller-frameworks.md index 0fd20018e..50539fc87 100644 --- a/harness-evals/harness-docs/decisions/adr-0001-dual-controller-frameworks.md +++ b/harness-evals/harness-docs/decisions/adr-0001-dual-controller-frameworks.md @@ -12,7 +12,7 @@ Migrating CertManager off library-go was not required for shipping optional oper ## Decision -Keep **library-go** for CertManager (always-on). Use **controller-runtime** for IstioCSR and TrustManager, started only when `--unsupported-addon-features` enables the corresponding gates. +Keep **library-go** for CertManager (always-on). Use **controller-runtime** for IstioCSR and TrustManager, started when each gate's **resolved** value is enabled (`--unsupported-addon-features` / defaults — IstioCSR GA default true; TrustManager TechPreview default false). ## Consequences diff --git a/harness-evals/harness-docs/decisions/adr-0003-feature-gates.md b/harness-evals/harness-docs/decisions/adr-0003-feature-gates.md index 73c5aac7b..ff05c9629 100644 --- a/harness-evals/harness-docs/decisions/adr-0003-feature-gates.md +++ b/harness-evals/harness-docs/decisions/adr-0003-feature-gates.md @@ -15,7 +15,7 @@ TrustManager was TechPreview and previously interacted with cluster `featuregate - Expose operator-local gates: `IstioCSR` (GA, default true), `TrustManager` (TechPreview, default false). - Wire enablement through `--unsupported-addon-features` (and CSV/deployment args in OLM). - Do **not** require cluster FeatureSet for TrustManager after CM-1141. -- Fail-closed on transient FeatureGate discovery errors without aborting the rest of operator startup (`starter.go` retries). +- Retry FeatureGate discovery failures in `setupFeatureGates`; on persistent failure keep the operator running and gate optional controllers from `--unsupported-addon-features` only (cluster FeatureGate errors do not disable them). - CRDs always install; only controllers are gated. **Five-touchpoint wiring for any new gate**: diff --git a/harness-evals/harness-docs/olm-packaging-guidelines.md b/harness-evals/harness-docs/olm-packaging-guidelines.md index 53a84c3bb..fd2572231 100644 --- a/harness-evals/harness-docs/olm-packaging-guidelines.md +++ b/harness-evals/harness-docs/olm-packaging-guidelines.md @@ -55,9 +55,11 @@ spec: ``` - `replaces` must point at the exact previous CSV name shipped in the same channel (upgrade edge). -- `olm.skipRange` should cover all previously-published z-streams so upgrades from any older - installed version resolve correctly; bump the lower bound as old minors are dropped and the - upper bound to the new `` being released. +- `olm.skipRange` should cover all previously-published versions so upgrades from any older + installed version resolve correctly; bump the lower bound as old minors are dropped. + Set the exclusive upper bound to the **candidate version** being released: `` for a + z-stream (e.g. `>=1.20.0 <1.20.1` when shipping `1.20.1`), or `` for a new minor + (e.g. `>=1.19.0 <1.20.0` when shipping `1.20.0`). - Update **both** when cutting a new minor/z-stream; keep `bundle/metadata/annotations.yaml` channel labels consistent with the release train. @@ -83,8 +85,11 @@ Make variables change, or `relatedImages` will silently drift from the intended disconnected/digest-pinned overrides and must be bumped by hand alongside the image references. Adding a **new** operand image requires: a new `RELATED_IMAGE_*` env in `config/manager/manager.yaml`, an entry in the `imageEnvMap` (or equivalent constants file) that -reads it, and re-running `make bundle` so the corresponding `relatedImages` entry is generated — -missing any of these will make `operator-sdk bundle validate` / disconnected mirroring fail. +reads it, and re-running `make bundle` so the corresponding `relatedImages` entry is generated. +`operator-sdk bundle validate` checks generated bundle/`relatedImages` metadata only — it does +**not** inspect Go `imageEnvMap`/constants. Confirm the runtime mapping with source review or a +unit test; a missing map entry can pass bundle validate and still break disconnected operand +image resolution. ## RBAC changes @@ -138,10 +143,11 @@ affected operand, then `make update-bindata`. | Script | Checks | Wired into `make verify-scripts`? | |---|---|---| +| `make verify-bindata` | Regenerated `pkg/operator/assets/bindata.go` matches `bindata/` sources. | Yes (Make dependency) | | `hack/verify-bundle.sh` | Runs `make bundle` and fails if the tree has uncommitted diffs. | Yes | | `hack/verify-deepcopy.sh` | Generated `zz_generated.deepcopy.go` is current. | Yes | | `hack/verify-clientgen.sh` | Generated clientset/informers/listers/applyconfigurations are current. | Yes | -| `hack/verify-crds.sh` / `verify-crds-version-upgrade.sh` | CRD manifests match `config/crd/bases` and upgrades are compatible. | No — run directly | +| `hack/verify-crds.sh` / `hack/verify-crds-version-upgrade.sh` | CRD manifests match `config/crd/bases` and upgrades are compatible. | No — run directly | Run `make verify` (or `make verify-scripts`) locally before sending a PR that touches `api/`, `config/`, or `bindata/`; also run `hack/verify-crds*.sh` directly if operand CRDs changed, @@ -157,7 +163,7 @@ since they are not invoked by any Makefile target in this repo. - [ ] `make bundle` — regenerates the bundle CSV/`relatedImages` from the updated `manager.yaml`; diff the result to confirm the versions actually changed. - [ ] Update the operand version link in the CSV `description` field (`config/manifests/bases/...csv.yaml`). -- [ ] `hack/verify-crds.sh` / `verify-crds-version-upgrade.sh` pass if operand CRDs changed (run +- [ ] `hack/verify-crds.sh` / `hack/verify-crds-version-upgrade.sh` pass if operand CRDs changed (run directly; not part of `make verify-scripts`). ## Checklist: bumping the operator version @@ -172,7 +178,9 @@ since they are not invoked by any Makefile target in this repo. - [ ] Env var: add to `config/manager/manager.yaml` container `env`; if operand-image-related, wire it through the matching `pkg/controller/*/constants.go`, then rerun `make bundle` — for a `RELATED_IMAGE_*` var this auto-populates the `relatedImages` entry; never hand-add one to the generated bundle CSV. - [ ] RBAC: add `+kubebuilder:rbac` marker on the controller; `make manifests`; `make bundle`; diff `config/rbac/role.yaml` and the CSV `clusterPermissions`. -- [ ] relatedImage: must have a paired `RELATED_IMAGE_*` env and an override in the corresponding controller's image-resolution map/constants — validated by `operator-sdk bundle validate`. +- [ ] relatedImage: paired `RELATED_IMAGE_*` env in `manager.yaml` + `make bundle` (bundle + validate covers `relatedImages`); also wire `imageEnvMap`/controller constants and cover + with source review or a unit test — bundle validate does **not** check the Go mapping. - [ ] Regenerate bindata if the change also touches an operand manifest template. ## Checklist: manual operand cleanup on uninstall diff --git a/harness-evals/harness-docs/security-guidelines.md b/harness-evals/harness-docs/security-guidelines.md index 885398754..7275ea06f 100644 --- a/harness-evals/harness-docs/security-guidelines.md +++ b/harness-evals/harness-docs/security-guidelines.md @@ -108,8 +108,9 @@ if effective.MinTLSVersion == configv1.VersionTLS13 { - `TRUSTED_CA_CONFIGMAP_NAME` / `--trusted-ca-configmap` mounts a ConfigMap the **cluster admin** creates and labels with `config.openshift.io/inject-trusted-cabundle=true` — the operator never creates or labels this ConfigMap itself (`../../docs/proxy.md`). If a bundle isn't found yet, the hook - returns a retryable error (`(Retrying) trusted CA config map %q doesn't exist`); don't convert this - to a fatal/Degraded condition. + returns a retryable error (`(Retrying) trusted CA config map %q doesn't exist`). The library-go + `DeploymentController` sets `Degraded=True` while retrying via the factory rate limiter; this is + transient and clears when the ConfigMap appears — not a permanent/unretried failure. - The CA bundle is always mounted at the fixed path `/etc/pki/tls/certs/cert-manager-tls-ca-bundle.crt` with `subPath: ca-bundle.crt` (`deployment_overrides.go`) — this matches Go's default root cert lookup path; changing it silently diff --git a/harness-evals/harness-docs/testing-guidelines.md b/harness-evals/harness-docs/testing-guidelines.md index 95c0539f3..df4ea8ba9 100644 --- a/harness-evals/harness-docs/testing-guidelines.md +++ b/harness-evals/harness-docs/testing-guidelines.md @@ -45,7 +45,7 @@ Focused e2e runs use the `TEST` variable (passed to `-run`) plus the label filte make test-e2e TEST='TestE2E' E2E_GINKGO_LABEL_FILTER='Feature:TrustManager' ``` -`test-unit` filters packages with `grep -vE 'test/[e2e|apis|utils]'` — new top-level test helper packages under `test/` should follow the existing `test/apis`, `test/e2e`, `test/utils` naming so they're auto-excluded from unit runs. +`test-unit` filters packages with `grep -vE 'test/(e2e|apis|utils)(/|$)'` — new top-level test helper packages under `test/` should follow the existing `test/apis`, `test/e2e`, `test/utils` naming so they're auto-excluded from unit runs. ## Unit Test Patterns Per Controller @@ -81,7 +81,17 @@ CertManager tests are library-go-oriented and live beside the controllers (`*_te ```go //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -generate //counterfeiter:generate -o fakes . CtrlClient -type CtrlClient interface { Get, List, StatusUpdate, Update, UpdateWithRetry, Create, Delete, Patch, Exists } +type CtrlClient interface { + Get(context.Context, client.ObjectKey, client.Object) error + List(context.Context, client.ObjectList, ...client.ListOption) error + StatusUpdate(context.Context, client.Object, ...client.SubResourceUpdateOption) error + Update(context.Context, client.Object, ...client.UpdateOption) error + UpdateWithRetry(context.Context, client.Object, ...client.UpdateOption) error + 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 + Exists(context.Context, client.ObjectKey, client.Object) (bool, error) +} ``` - Regenerate after changing the interface: `make generate-fakes` (wraps `go generate ./...`).