From 701783900cdb5326889cc2f7612689e3418fd76c Mon Sep 17 00:00:00 2001 From: Bharath B Date: Fri, 7 Aug 2026 20:30:30 +0530 Subject: [PATCH 1/2] OAPE-877: Standardize AGENTS.md and agent harness docs for ESO Signed-off-by: Bharath B --- .coderabbit.yaml | 54 ++++ .github/ISSUE_TEMPLATE/bug_report.md | 52 ++++ .github/ISSUE_TEMPLATE/feature_request.md | 25 ++ .github/PULL_REQUEST_TEMPLATE.md | 37 +++ AGENTS.md | 117 ++++++++ CLAUDE.md | 100 +++++++ CONTRIBUTING.md | 142 ++++++++++ README.md | 239 +++++++++------- SECURITY.md | 38 +++ docs/anti-patterns/DUAL_CACHE_FIX.md | 27 +- docs/anti-patterns/DUAL_CACHE_FIX_SUMMARY.md | 24 +- docs/anti-patterns/README.md | 9 +- harness-evals/harness-docs/ESO_DEVELOPMENT.md | 167 +++++++++++ harness-evals/harness-docs/ESO_TESTING.md | 173 +++++++++++ .../harness-docs/api-contracts-guidelines.md | 185 ++++++++++++ .../harness-docs/architecture/components.md | 268 ++++++++++++++++++ .../decisions/adr-0001-bindata-over-helm.md | 56 ++++ .../adr-0002-update-with-retry-over-ssa.md | 50 ++++ .../adr-0003-network-policy-naming-scheme.md | 48 ++++ .../harness-docs/decisions/adr-template.md | 43 +++ .../domain/external-secrets-config.md | 167 +++++++++++ .../domain/external-secrets-manager.md | 95 +++++++ .../harness-docs/error-handling-guidelines.md | 128 +++++++++ .../harness-docs/exec-plans/README.md | 22 ++ .../harness-docs/integration-guidelines.md | 135 +++++++++ .../harness-docs/performance-guidelines.md | 100 +++++++ .../harness-docs/references/ecosystem.md | 84 ++++++ .../harness-docs/references/enhancements.md | 23 ++ .../harness-docs/security-guidelines.md | 118 ++++++++ .../harness-docs/testing-guidelines.md | 136 +++++++++ test/e2e/README.md | 4 +- 31 files changed, 2757 insertions(+), 109 deletions(-) create mode 100644 .coderabbit.yaml create mode 100644 .github/ISSUE_TEMPLATE/bug_report.md create mode 100644 .github/ISSUE_TEMPLATE/feature_request.md create mode 100644 .github/PULL_REQUEST_TEMPLATE.md create mode 100644 AGENTS.md create mode 100644 CLAUDE.md create mode 100644 CONTRIBUTING.md create mode 100644 SECURITY.md create mode 100644 harness-evals/harness-docs/ESO_DEVELOPMENT.md create mode 100644 harness-evals/harness-docs/ESO_TESTING.md create mode 100644 harness-evals/harness-docs/api-contracts-guidelines.md create mode 100644 harness-evals/harness-docs/architecture/components.md create mode 100644 harness-evals/harness-docs/decisions/adr-0001-bindata-over-helm.md create mode 100644 harness-evals/harness-docs/decisions/adr-0002-update-with-retry-over-ssa.md create mode 100644 harness-evals/harness-docs/decisions/adr-0003-network-policy-naming-scheme.md create mode 100644 harness-evals/harness-docs/decisions/adr-template.md create mode 100644 harness-evals/harness-docs/domain/external-secrets-config.md create mode 100644 harness-evals/harness-docs/domain/external-secrets-manager.md create mode 100644 harness-evals/harness-docs/error-handling-guidelines.md create mode 100644 harness-evals/harness-docs/exec-plans/README.md create mode 100644 harness-evals/harness-docs/integration-guidelines.md create mode 100644 harness-evals/harness-docs/performance-guidelines.md create mode 100644 harness-evals/harness-docs/references/ecosystem.md create mode 100644 harness-evals/harness-docs/references/enhancements.md create mode 100644 harness-evals/harness-docs/security-guidelines.md create mode 100644 harness-evals/harness-docs/testing-guidelines.md diff --git a/.coderabbit.yaml b/.coderabbit.yaml new file mode 100644 index 000000000..666948f3d --- /dev/null +++ b/.coderabbit.yaml @@ -0,0 +1,54 @@ +# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json +# See https://docs.coderabbit.ai/reference/configuration for all fields and default values +knowledge_base: + code_guidelines: + filePatterns: + - "harness-evals/harness-docs/*-guidelines.md" + - "AGENTS.md" + - "CONTRIBUTING.md" +reviews: + high_level_summary: true + high_level_summary_in_walkthrough: true + poem: false + review_status: true + commit_status: true + collapse_walkthrough: true + sequence_diagrams: false + path_instructions: + - path: "api/v1alpha1/**" + instructions: | + Review CRD type changes for CEL validation rules, immutability markers, + bounds constraints, and corresponding .testsuite.yaml test coverage. + See harness-evals/harness-docs/api-contracts-guidelines.md. + - path: "pkg/controller/**" + instructions: | + Check error classification (IrrecoverableError/RetryRequiredError/UserConfigurationError), + requeue behavior, and that new managed resources are registered in + controllerManagedResources and HasObjectChanged. See harness-evals/harness-docs/error-handling-guidelines.md + and harness-evals/harness-docs/performance-guidelines.md. + tools: + github-checks: + timeout_ms: 120000 +chat: + auto_reply: true +code_generation: + docstrings: false +instructions: + review: | + IMPORTANT: The following PR metadata checks are mandatory requirements, + not suggestions. Report violations as actionable review comments with + severity "important" or "warning", NOT as nitpicks. + + **Commit messages** (REQUIRED): Every commit MUST follow the format + `: short imperative description` (e.g., `ESO-142: add proxy egress + network policy`). The Jira project can be any valid project (ESO, OAPE, etc.). + Reject commits with generic messages like "fix bug", "update code", + "address review comments", or commits missing a Jira ticket number. + + **PR title** (REQUIRED): MUST be concise (<70 chars), imperative, and + include the Jira ticket number (e.g., "ESO-142: Add proxy egress network + policy"). Reject titles that are vague or missing the ticket number. + + **PR description** (REQUIRED): MUST explain what changed, why, and how. + Reject PRs with empty or minimal descriptions that don't help reviewers + understand the motivation. diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md new file mode 100644 index 000000000..db5aa1fcb --- /dev/null +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -0,0 +1,52 @@ +--- +name: Bug Report +about: Report a bug in the External Secrets Operator +labels: kind/bug +--- + +## Describe the Bug + + + +## Steps to Reproduce + +1. +2. +3. + +## Expected Behavior + + + +## Actual Behavior + + + +## Environment + +- OpenShift / Kubernetes version: +- Operator version (`oc get csv -n external-secrets-operator`): +- External Secrets operand version: +- Secret provider (AWS, Vault, Azure, GCP, etc.): + +## Relevant Resources + +> **Redact** secret values, credentials, tokens, private keys, and personal data before posting resources or logs in a public issue. + +```yaml +# oc get esc cluster -o yaml +``` + +```yaml +# oc get esm cluster -o yaml +``` + +## Operator Logs + +```text +# oc logs -n external-secrets-operator deployment/external-secrets-operator-controller-manager +``` + +## Additional Context + + diff --git a/.github/ISSUE_TEMPLATE/feature_request.md b/.github/ISSUE_TEMPLATE/feature_request.md new file mode 100644 index 000000000..9a6fd7a90 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/feature_request.md @@ -0,0 +1,25 @@ +--- +name: Feature Request +about: Suggest a new feature or enhancement +labels: kind/feature +--- + +## Summary + + + +## Motivation + + + +## Proposed Solution + + + +## Alternatives Considered + + + +## Additional Context + + diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md new file mode 100644 index 000000000..162f1a93c --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -0,0 +1,37 @@ +## Description + +### What changed? + + +### Why? + + +### How? + + +## Type of Change + +- [ ] Bug fix +- [ ] New feature +- [ ] CRD / API change +- [ ] Refactoring (no functional change) +- [ ] Documentation +- [ ] CI / build + +## Checklist + +- [ ] `make verify` passes (vet, fmt, deps, bindata, generated files, govulncheck, git diff) +- [ ] `make test` passes (unit + API integration tests) +- [ ] `make lint` passes +- [ ] New/changed CRD fields have appropriate CEL validation; add `.testsuite.yaml` tests for new CEL rules +- [ ] New managed resources added to `controllerManagedResources`, `buildCacheObjectList()`, `HasObjectChanged`, and the ordered install sequence +- [ ] No hand-edits to generated files (`bindata.go`, `zz_generated.deepcopy.go`, CRD YAML, fakes) +- [ ] Error paths use the correct error type (`IrrecoverableError` / `RetryRequiredError` / `UserConfigurationError`) + +## Testing + + + +## Additional Context + + diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 000000000..10fa46623 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,117 @@ +# External Secrets Operator — Agentic Documentation + +**Component**: External Secrets Operator for Red Hat OpenShift +**Repository**: openshift/external-secrets-operator + +> **Platform Patterns**: See [openshift/enhancements/ai-docs/](https://github.com/openshift/enhancements/tree/master/ai-docs) for operator patterns, testing practices, security guidelines, and cross-repo ADRs. + +## What is ESO? + +Manages the lifecycle of the upstream [external-secrets](https://github.com/external-secrets/external-secrets) project on OpenShift. Deploys and configures the operand via static YAML manifests embedded as bindata — it is not a fork. + +**Key Principle**: The operator owns the operand deployment; users configure via two singleton CRs (`ExternalSecretsConfig`, `ExternalSecretsManager`), both named `cluster`. + +## Core Components + +| Component | Purpose | Location | +|-----------|---------|----------| +| ExternalSecrets Controller | Operand lifecycle (install, update, delete) | `pkg/controller/external_secrets/` | +| ESM Controller | Status aggregation, default ESM creation | `pkg/controller/external_secrets_manager/` | +| CRD Annotator | cert-manager CA injection on CRDs (conditional) | `pkg/controller/crd_annotator/` | + +**Quick Start**: `oc get esc cluster -o yaml` | `oc get esm cluster -o yaml` | `oc get pods -n external-secrets` + +## Critical Patterns + +1. **NOT Server-Side Apply** — all updates use `UpdateWithRetry` (Get → set ResourceVersion → Update). Co-managed resources (Secret, ConfigMap) use `patchResourceMetadata` for metadata-only JSON Patch. Never introduce SSA. +2. **Bindata pipeline** — operand manifests are pre-rendered from upstream Helm charts at build time (`hack/update-external-secrets-manifests.sh`), embedded via `openshift/build-machinery-go`. `pkg/operator/assets/bindata.go` is generated — **never hand-edit**. +3. **Immutable cert-manager fields** — `mode`, `injectAnnotations`, `issuerRef` in CertManagerConfig are immutable via CEL `self == oldSelf`. Network policy entries (name+componentName) also cannot be removed once added. + +## Domain Guidelines + +Detailed rules for each domain are in `harness-evals/harness-docs/`. Read the relevant file before modifying that area. + +| Guideline | Scope | +|-----------|-------| +| [Security](harness-evals/harness-docs/security-guidelines.md) | CEL validation, annotation/label restrictions, container hardening, RBAC, network policies, TLS | +| [Performance](harness-evals/harness-docs/performance-guidelines.md) | Label-filtered caches, change detection, event predicates, requeue strategy, concurrency | +| [Error Handling](harness-evals/harness-docs/error-handling-guidelines.md) | Error classification (Irrecoverable/Retry/UserConfig), status conditions, requeue matrix, events | +| [API Contracts](harness-evals/harness-docs/api-contracts-guidelines.md) | Singleton enforcement, field immutability, CEL rules, list map keys, `.testsuite.yaml` patterns | +| [Testing](harness-evals/harness-docs/testing-guidelines.md) | Unit tests, API integration tests (envtest), E2E with Ginkgo labels, make targets | +| [Integration](harness-evals/harness-docs/integration-guidelines.md) | cert-manager, OLM, proxy, CNO trusted CA, console, metrics, multi-arch, webhooks | + +## Cross-Cutting Conventions + +- **Generated files**: Never hand-edit `bindata.go`, `fake_ctrl_client.go`, `zz_generated.deepcopy.go`, or CRD YAML in `config/crd/bases/`. Regenerate with `make manifests generate update-bindata` or `go generate`. +- **Go style**: stdlib `testing` only (no Ginkgo for unit tests, no testify except E2E utils). Table-driven tests with `t.Run`. Call `t.Parallel()` on outer function and each subtest. Use `t.Setenv()` instead of `os.Setenv`. +- **Constants**: All string constants (asset names, label keys, env var names) live in `constants.go`. Do not scatter literals across source files. +- **New managed resources**: Must be added to `controllerManagedResources`, `buildCacheObjectList()`, `HasObjectChanged` type-switch, and the ordered install sequence. See `harness-evals/harness-docs/ESO_DEVELOPMENT.md` section 2. +- **Commit messages**: Always include the Jira ticket number and a clear imperative description. Format: `: short description` (e.g., `ESO-142: add proxy egress network policy`). The Jira project can be any valid project (ESO, OAPE, etc.). If no Jira ticket exists, use a descriptive imperative summary. Never use generic messages like "fix bug" or "update code". +- **PR checklist**: Run `make verify` (vet, fmt, deps, bindata, generated files, govulncheck, markdownlint, git diff), `make test`, and `make lint` before submitting. `make verify` is the single gate that CI enforces. + +## Common Pitfalls + +1. **Never return both `RequeueAfter` and a non-nil error** from `Reconcile` — return one or the other. +2. **Use the cached client for managed resources** (`app=external-secrets`). Use `UncachedClient` only for objects outside the cache (cert-manager Issuers, user-provided Secrets). +3. **`Decode*ObjBytes` helpers panic on failure** — intentional for build-time-constant assets; do not wrap them in error handling. +4. **Operator RBAC markers** (`+kubebuilder:rbac`) go in controller Go files; operand RBAC lives in static YAML under `bindata/`. +5. More contributor pitfalls: `harness-evals/harness-docs/ESO_DEVELOPMENT.md` → Common Mistakes. + +## Documentation Structure + +```text +harness-evals/harness-docs/ +├── *-guidelines.md # Enforceable domain guidelines (security, testing, API, …) +├── domain/ # ExternalSecretsConfig, ExternalSecretsManager API docs +├── architecture/ # Controller internals, resource management, bindata pipeline +│ └── components.md +├── decisions/ # Component-specific ADRs (bindata, update strategy, NP naming) +├── exec-plans/ # Feature planning +├── references/ +│ ├── ecosystem.md # Links to Platform patterns +│ └── enhancements.md # Enhancement proposals catalog +├── ESO_DEVELOPMENT.md # Development workflows, build targets, common tasks +└── ESO_TESTING.md # Test suites, patterns, E2E labels +``` + +**AI Agent Path**: `harness-evals/harness-docs/*-guidelines.md` (as needed) → `domain/` → `architecture/` → `decisions/` → `ESO_DEVELOPMENT.md` + +## Namespaces & Image Resolution + +| Namespace | Purpose | +|-----------|---------| +| `external-secrets-operator` | Operator deployment (OLM-managed) | +| `external-secrets` | Operand namespace (operator-created) | + +| Env Var | Purpose | +|---------|---------| +| `RELATED_IMAGE_EXTERNAL_SECRETS` | Operand image (OLM disconnected convention) | +| `RELATED_IMAGE_BITWARDEN_SDK_SERVER` | Bitwarden image | + +## Error Classification + +| Type | Requeue | Example | +|------|---------|---------| +| `IrrecoverableError` | No | Missing RELATED_IMAGE_* env var | +| `RetryRequiredError` | 30s | Transient API server error | +| `UserConfigurationError` | Only NotFound | Invalid cert-manager issuer ref | + +## Conditional Deployments + +| Operand Component | Condition | +|-------------------|-----------| +| `external-secrets` (core) | Always | +| `external-secrets-webhook` | Always | +| `external-secrets-cert-controller` | cert-manager **disabled** | +| `bitwarden-sdk-server` | Bitwarden plugin **enabled** | + +## Key References + +- [Enhancement: ESO on OpenShift](https://github.com/openshift/enhancements/blob/master/enhancements/external-secrets-operator/external-secrets-operator.md) +- [Enhancement: Network Policies](https://github.com/openshift/enhancements/blob/master/enhancements/external-secrets-operator/external-secrets-network-policy.md) +- [Enhancement: Component Config](https://github.com/openshift/enhancements/blob/master/enhancements/external-secrets-operator/external-secrets-component-config.md) +- [Upstream external-secrets](https://github.com/external-secrets/external-secrets) | [OpenShift Docs](https://docs.openshift.com/) + +--- + +**Platform Documentation**: openshift/enhancements/ai-docs/ diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 000000000..af75d8fa3 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,100 @@ +@AGENTS.md + +## Build & Test Commands + +### Build + +```bash +make build # Build operator with manifests, generate, fmt, vet +make build-operator # Build operator binary only (no checks) +make image-build # Build container image +``` + +### Test + +```bash +make test # Run all non-e2e tests (test-apis + test-unit) +make test-unit # Run unit tests only +make test-apis # Run API integration tests (envtest + Ginkgo) +make test-e2e # Run e2e tests (requires live cluster) +``` + +### Lint & Verify + +```bash +make lint # Run golangci-lint +make lint-fix # Run golangci-lint with auto-fix +make lint-markdown # Run markdownlint-cli2 on docs +make lint-markdown-fix # Auto-fix markdownlint findings where possible +make verify # Run vet, fmt, deps, bindata, generated files, govulncheck, markdownlint, git diff +make fmt # Run go fmt +make vet # Run go vet +``` + +### Code Generation + +```bash +make manifests # Generate CRDs and RBAC +make generate # Generate deepcopy methods +make update-bindata # Regenerate bindata.go from bindata/ +make update # Run generate, manifests, update-operand-manifests, update-bindata, bundle, docs +``` + +### Dependency Management + +```bash +make update-vendor # Update vendor directory for all workspace modules +make update-dep PKG=... # Update a dependency across all modules +make verify-deps # Verify go.mod dependencies +``` + +## Claude Code Behavioral Preferences + +### Commit Messages + +Always include the Jira ticket number and a clear imperative description. Format: + +```text +: short description of the change +``` + +Example: `ESO-142: add proxy egress network policy`. The Jira project can be any valid project (ESO, OAPE, etc.). If no Jira ticket exists, ask the user for context or use a descriptive imperative summary. Never use generic messages like "fix bug", "update code", or "address review comments". + +### Pre-Commit Workflow + +Always run `make verify` before committing. This is the single CI gate that catches: +- Generated file drift (bindata, deepcopy, CRDs) +- Dependency inconsistencies +- Go formatting and vet issues +- Vulnerability scan failures + +### Go Workspace Mode + +This repository uses Go workspaces (`go.work`). Commands that need workspace mode (`fmt`, `vet`, `test`, `test-unit`, `test-e2e`, `run`, `update-vendor`, `update-dep`) automatically unset `GOFLAGS` to avoid conflicts with `-mod=vendor`. + +### Generated Files — Never Hand-Edit + +- `pkg/operator/assets/bindata.go` (regenerate with `make update-bindata`) +- `pkg/controller/client/fakes/fake_ctrl_client.go` (regenerate with `go generate ./pkg/controller/client/...`) +- `zz_generated.deepcopy.go` files (regenerate with `make generate`) +- CRD YAML in `config/crd/bases/` (regenerate with `make manifests`) + +### Container Tool + +Default is `podman`. Override with `CONTAINER_TOOL=docker` if needed. + +### E2E Test Filtering + +Default filter excludes Proxy, Upgrade, and Bitwarden tests. Override with: + +```bash +make test-e2e E2E_GINKGO_LABEL_FILTER='Provider:Vault' +``` + +## File Exclusions + +When reading or editing code, skip these auto-generated files: +- `pkg/operator/assets/bindata.go` +- `pkg/controller/client/fakes/` +- `**/zz_generated.deepcopy.go` +- `config/crd/bases/*.yaml` diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 000000000..95fecc1f0 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,142 @@ +# Contributing to External Secrets Operator + +Thank you for your interest in contributing to the External Secrets Operator for Red Hat OpenShift. This guide covers the contribution workflow. For architecture details, see [README.md](README.md). For detailed domain rules, see the guideline files linked below. + +## Development Environment Setup + +### Prerequisites + +- Go 1.26+ +- podman (or docker -- set `CONTAINER_TOOL=docker` to override) +- kubectl v1.32.1+ or oc +- Access to a Kubernetes v1.32.1+ or OpenShift cluster (required only for E2E tests) + +### Building + +```sh +make build # Full build: codegen, fmt, vet, compile +make build-operator # Compile binary only (skip codegen/checks) +``` + +All build tooling (controller-gen, golangci-lint, envtest, etc.) is vendored and built on demand -- no manual tool installation required. + +### Repository Layout + +This project uses Go workspaces (`go.work`). Targets that need workspace mode (`fmt`, `vet`, `test`, `test-unit`, `test-e2e`, `run`, `update-vendor`, `update-dep`) automatically unset `GOFLAGS` to avoid conflicts with `-mod=vendor`. Run `make help` for all available targets. + +## Code Style and Conventions + +The project enforces style through `golangci-lint` (configured in `.golangci.yml`) and `go vet`. A few key conventions: + +- **Import ordering**: standard, third-party, project-local (`github.com/openshift/external-secrets-operator`). Enforced by the `gci` formatter in `.golangci.yml`. +- **String constants**: All string constants (asset names, label keys, env var names) belong in `constants.go`. Do not scatter literals. +- **Update strategy**: All resource updates use `UpdateWithRetry` (Get, set ResourceVersion, Update). Do not use Server-Side Apply. See [AGENTS.md](AGENTS.md) for details. +- **Generated files**: Never hand-edit `bindata.go`, `fake_ctrl_client.go`, `zz_generated.deepcopy.go`, or CRD YAML. Regenerate with `make manifests generate update-bindata`. + +For domain-specific rules, see: + +| Guideline | When to Read | +|-----------|-------------| +| [Security](harness-evals/harness-docs/security-guidelines.md) | Touching RBAC, CEL validation, container specs, network policies, TLS | +| [API Contracts](harness-evals/harness-docs/api-contracts-guidelines.md) | Modifying CRD fields, adding CEL rules, writing `.testsuite.yaml` | +| [Testing](harness-evals/harness-docs/testing-guidelines.md) | Writing unit tests, API integration tests, or E2E tests | +| [Error Handling](harness-evals/harness-docs/error-handling-guidelines.md) | Adding error paths, status conditions, or requeue logic | +| [Performance](harness-evals/harness-docs/performance-guidelines.md) | Working with caches, predicates, or reconciler concurrency | +| [Integration](harness-evals/harness-docs/integration-guidelines.md) | Touching cert-manager, OLM, proxy, or webhook integration | + +## Submitting Changes + +### Branch Naming + +Use the Jira ticket ID as a prefix (e.g., `eso-142`, `oape-481-fix-predicates`). The Jira project can be any valid project (ESO, OAPE, etc.). + +### Commit Messages + +Follow the pattern used in the repository: + +```text +: Short imperative description of the change +``` + +Example: `ESO-142: add proxy egress network policy` + +The Jira project can be any valid project (ESO, OAPE, etc.). For changes without a Jira ticket, use a descriptive imperative summary (e.g., `fix make verify`, `update owners list`). Avoid generic messages like "fix bug" or "update code". + +### Pull Request Process + +1. Fork the repository and create a branch from `main`. +2. Make your changes. Run all checks (see next section). +3. Submit a Pull Request describing what changed and why. +4. Address review feedback. Push additional commits rather than force-pushing. +5. A maintainer will merge once CI passes and the review is approved. + +## Pre-Submission Checks + +Run these before every PR. `make verify` is the single gate CI enforces. + +```sh +make verify # Runs vet, fmt, deps, bindata, generated files, govulncheck, markdownlint, git diff +make test # All non-E2E tests (unit + API integration) +make lint # golangci-lint +make lint-markdown # markdownlint-cli2 (also part of make verify) +``` + +If you changed CRD types, run the full regeneration first: + +```sh +make manifests generate update-bindata +``` + +If `make verify` reports a git diff, it means generated files are out of date. Regenerate and commit the results. + +## API and Design Changes + +Significant API or behavioral changes need design review before implementation. + +- For user-visible API changes, new configuration surfaces, or cross-cutting behavior, open (or update) an enhancement proposal under [`openshift/enhancements/enhancements/external-secrets-operator/`](https://github.com/openshift/enhancements/tree/master/enhancements/external-secrets-operator). +- Discuss the design with maintainers (Jira + enhancement PR) and get agreement before landing CRD/API changes in this repository. +- Small additive fields that follow an existing, approved pattern may not need a full enhancement — ask maintainers if unsure. +- Component-local implementation decisions that do not warrant a cross-repo enhancement can be captured as ADRs in [`harness-evals/harness-docs/decisions/`](harness-evals/harness-docs/decisions/). See the catalog in [`harness-evals/harness-docs/references/enhancements.md`](harness-evals/harness-docs/references/enhancements.md). + +## Adding New Features + +### New CRD Fields + +1. Confirm design review / enhancement status (see [API and Design Changes](#api-and-design-changes)). +2. Edit types in `api/v1alpha1/`. +3. Add CEL validation rules as needed (see [API Contracts](harness-evals/harness-docs/api-contracts-guidelines.md)). +4. Run `make manifests generate`. +5. Add `.testsuite.yaml` test cases for new CEL rules. +6. Update controller logic and add unit tests. + +### New Managed Resources (Operand Manifests) + +1. Add the manifest to `bindata/`. +2. Register it in `controllerManagedResources` and the `HasObjectChanged` type-switch. +3. Add it to the ordered install sequence. +4. Run `make update-bindata`. +5. See `harness-evals/harness-docs/ESO_DEVELOPMENT.md` section 2 for the full checklist. + +### New Controllers + +Follow the existing controller structure under `pkg/controller/`. Each controller should have its own package with dedicated unit tests. See the existing controllers (`external_secrets`, `external_secrets_manager`, `crd_annotator`) as reference. + +## Testing Expectations + +Follow [harness-evals/harness-docs/testing-guidelines.md](harness-evals/harness-docs/testing-guidelines.md). In short: stdlib `testing` for unit tests (table-driven, `t.Parallel()`), envtest + Ginkgo for API tests (`make test-apis`), and labeled Ginkgo E2E against a live cluster (`make test-e2e`; see [test/e2e/README.md](test/e2e/README.md)). + +## Review Process + +- All PRs require at least one maintainer approval. +- CI must pass (`make verify` is the primary gate). +- Reviewers will check for adherence to the domain guidelines linked above. +- For API changes, expect additional scrutiny on backward compatibility, CEL validation, and whether an [enhancement proposal](#api-and-design-changes) was completed. +- Generated file drift is a common rejection reason -- always run `make verify` locally. + +## For AI Agents + +Start with [AGENTS.md](AGENTS.md). Claude Code–specific notes are in [CLAUDE.md](CLAUDE.md). + +## License + +By contributing, you agree that your contributions will be licensed under the Apache License 2.0. diff --git a/README.md b/README.md index c5d9c828f..5c5a689df 100644 --- a/README.md +++ b/README.md @@ -1,16 +1,8 @@ -# external-secrets-operator for Red Hat OpenShift -This repository contains External Secrets Operator for Red Hat OpenShift. The operator runs in `external-secrets-operator` namespace. -The External Secrets Operator provides the ability to deploy [`external-secrets`](https://github.com/openshift/external-secrets) using different configurations +# External Secrets Operator for Red Hat OpenShift -The External Secrets Operator for Red Hat OpenShift operates as a cluster-wide service to deploy and manage the external-secrets -application. The external-secrets application integrates with external secrets management systems and performs secret fetching, -refreshing, and provisioning within the cluster. +This repository contains the External Secrets Operator for Red Hat OpenShift. The operator runs in the `external-secrets-operator` namespace and deploys and manages the upstream [external-secrets](https://github.com/external-secrets/external-secrets) application on OpenShift clusters using static YAML manifests embedded as bindata. -## Description -Use the External Secrets Operator for Red Hat OpenShift to integrate external-secrets application with the -OpenShift Container Platform cluster. The external-secrets application fetches secrets stored in the external providers such as -AWS Secrets Manager, HashiCorp Vault, Google Secrets Manager, Azure Key Vault, IBM Cloud Secrets Manager, -AWS Systems Manager Parameter Store and integrates them with Kubernetes in a secure manner. +The External Secrets Operator operates as a cluster-wide service that integrates external secrets management systems -- such as AWS Secrets Manager, HashiCorp Vault, Google Secrets Manager, Azure Key Vault, IBM Cloud Secrets Manager, and AWS Systems Manager Parameter Store -- with the OpenShift Container Platform, performing secret fetching, refreshing, and provisioning within the cluster. Using the External Secrets Operator ensures the following: - Decouples applications from the secret-lifecycle management. @@ -19,147 +11,203 @@ Using the External Secrets Operator ensures the following: - Supports multi-cloud secret sourcing with fine-grained access control. - Centralizes and audits access control. -The External Secrets Operator for Red Hat OpenShift uses the [`external-secrets`](https://github.com/openshift/external-secrets) helm charts -to install application. The operator has three controllers to achieve the same: -- `external_secrets_manager` controller: This is responsible for - * reconciling the `externalsecretsmanagers.operator.openshift.io` resource. - * providing the status of other controllers. -- `external_secrets` controller: This is responsible for - * reconciling the `externalsecretsconfigs.operator.openshift.io` resource. - * installing and managing the `external-secrets` application based on the user defined configurations in `externalsecretsconfigs.operator.openshift.io` resource. - * reconciling the `externalsecretsmanagers.operator.openshift.io` resource for the global configurations and updates the `external-secrets` deployment accordingly. -- `crd_annotator` controller: - * This is responsible for adding `cert-manager.io/inject-ca-from` annotation in the `external-secrets` provided CRDs. - * This is an optional controller, which will be activated only when [`cert-manager`](https://cert-manager.io/) is installed. - * When `cert-manager` is installed after External Secrets Operator installation, `external-secrets-operator-controller-manager` deployment must be restarted to activate the controller. - -The operator automatically creates a cluster-scoped `externalsecretsmanagers.operator.openshift.io` object named `cluster`. - -For more information about -- `external-secrets-operator for Red Hat OpenShift`, refer to the [link](https://docs.redhat.com/en/documentation/openshift_container_platform/latest/html/security_and_compliance/external-secrets-operator-for-red-hat-openshift) -- `external-secrets` application, refer to the [link](https://external-secrets.io/latest/). -- `cert-manager Operator for Red Hat OpenShift`, refer to the [link](https://docs.redhat.com/en/documentation/openshift_container_platform/latest/html/security_and_compliance/cert-manager-operator-for-red-hat-openshift) +## Architecture + +The operator uses two singleton custom resources (both named `cluster`) for configuration: + +| Custom Resource | Purpose | +|-----------------|---------| +| `ExternalSecretsConfig` (`esc`) | Operand installation and configuration | +| `ExternalSecretsManager` (`esm`) | Status aggregation and global settings | + +Three controllers handle the operator lifecycle: + +| Controller | Responsibility | +|------------|----------------| +| `external_secrets` | Installs and manages the external-secrets application based on user-defined configuration in the `ExternalSecretsConfig` CR. | +| `external_secrets_manager` | Reconciles the `ExternalSecretsManager` CR, provides aggregated status from other controllers, and auto-creates the default `cluster` CR. | +| `crd_annotator` | Adds `cert-manager.io/inject-ca-from` annotations to external-secrets CRDs. Activates only when [cert-manager](https://cert-manager.io/) is installed. | + +The operand manifests are pre-rendered from upstream Helm charts at build time (`hack/update-external-secrets-manifests.sh`) and embedded via `openshift/build-machinery-go` into `pkg/operator/assets/bindata.go`. + +## Tech Stack + +| Component | Version | +|-----------|---------| +| Go | 1.26 (workspace mode via `go.work`) | +| Kubernetes libraries | v0.35.6 | +| controller-runtime | v0.23.3 | +| cert-manager | v1.18.5 | +| External Secrets (operand) | v2.5.0 | +| Container tool | podman (default; override with `CONTAINER_TOOL=docker`) | + +## Project Structure + +```text +api/ API type definitions (ExternalSecretsConfig, ExternalSecretsManager) +cmd/ Operator entry point +pkg/ + controller/ Controller implementations (external_secrets, external_secrets_manager, crd_annotator) + operator/ Operand lifecycle, asset management, bindata +config/ Kustomize overlays, CRD bases, RBAC, samples +bindata/ Embedded operand manifests (source for bindata.go) +test/ Test suites (e2e/, apis/, utils/) +hack/ Build and codegen scripts +docs/ Product/user docs, anti-patterns, OpenSpec notes +harness-evals/harness-docs/ Domain guidelines, architecture, ADRs, development workflows +bundle/ OLM bundle manifests +tools/ Build tooling module +images/ Container image definitions +vendor/ Vendored Go dependencies +``` ## Getting Started ### Prerequisites -- go version 1.23.6+ -- docker version 17.03+. -- kubectl version v1.32.1+. -- Access to a Kubernetes v1.32.1+ cluster. - -### To Deploy on the cluster -**Build and push your image to the location specified by `IMG`:** -```sh -make docker-build docker-push IMG=/external-secrets-operator: -``` +- Go 1.26+ +- podman (or docker) 17.03+ +- kubectl v1.32.1+ / oc +- Access to a Kubernetes v1.32.1+ / OpenShift cluster -> **NOTE:** This image ought to be published in the personal registry you specified. -And it is required to have access to pull the image from the working environment. -Make sure you have the proper permission to the registry if the above commands don’t work. - -**Install the CRDs into the cluster:** +### Building ```sh -make install +make build # Full build: manifests, generate, fmt, vet, then compile +make build-operator # Compile the operator binary only (no codegen or checks) +make image-build # Build the container image with podman ``` -**Deploy the Manager to the cluster with the image specified by `IMG`:** +To build and push a custom image: ```sh -make deploy IMG=/external-secrets-operator: +make image-build image-push IMG=/external-secrets-operator: ``` -> **NOTE:** If you encounter RBAC errors, you may need to grant yourself cluster-admin -privileges or be logged in as admin. +### Deploying to a Cluster -**Create instances of your solution** -You can apply the samples (examples) from the config/sample: +Install CRDs and deploy the operator: ```sh -kubectl apply -k config/samples/ +make install # Install CRDs +make deploy IMG=/external-secrets-operator: # Deploy the operator +kubectl apply -k config/samples/ # Create sample CRs ``` -> **NOTE:** Ensure that the samples has default values to test it out. - -### To Uninstall -**Delete the instances (CRs) from the cluster:** +To uninstall: ```sh kubectl delete -k config/samples/ +make uninstall +make undeploy ``` -**Delete the APIs(CRDs) from the cluster:** +### Generating a Standalone Installer ```sh -make uninstall +make build-installer IMG=/external-secrets-operator: +# Produces dist/install.yaml containing all resources +kubectl apply -f dist/install.yaml ``` -**UnDeploy the controller from the cluster:** +## Testing + +| Make Target | Description | +|-------------|-------------| +| `make test` | Run all non-e2e tests (`test-apis` + `test-unit`); no cluster required. | +| `make test-unit` | Run unit tests (excludes `test/e2e`, `test/apis`, `test/utils`). | +| `make test-apis` | Run API integration tests (Ginkgo + envtest). | +| `make test-e2e` | Run end-to-end tests against a live cluster. | + +E2E tests support label filtering for provider-specific or scenario-specific runs: ```sh -make undeploy +make test-e2e E2E_GINKGO_LABEL_FILTER="Provider:Vault" ``` -## Project Distribution +For full E2E details including prerequisites, suite-specific commands, and cross-platform labels, see [test/e2e/README.md](test/e2e/README.md). -Following are the steps to build the installer and distribute this project to users. +Detailed testing conventions (unit test style, table-driven patterns, envtest setup) are documented in [harness-evals/harness-docs/testing-guidelines.md](harness-evals/harness-docs/testing-guidelines.md). -1. Build the installer for the image built and published in the registry: +## Verification and Linting + +Before submitting a PR, run the CI gate check: ```sh -make build-installer IMG=/external-secrets-operator:tag +make verify # Runs vet, fmt, deps, bindata, generated files, govulncheck, markdownlint, and git diff +make lint # Run golangci-lint +make lint-fix # Run golangci-lint with auto-fix +make lint-markdown # Run markdownlint-cli2 on docs (also invoked by make verify) ``` -> **NOTE:** The makefile target mentioned above generates an 'install.yaml' -file in the dist directory. This file contains all the resources built -with Kustomize, which are necessary to install this project without -its dependencies. +`make verify` is the single gate that CI enforces. It catches generated-file drift, dependency inconsistencies, formatting issues, markdown style problems, and vulnerability scan failures. + +## Code Generation -2. Using the installer +Several files in this repository are generated and must not be hand-edited: -Users can just run kubectl apply -f to install the project, i.e.: +| File | Regenerate With | +|------|-----------------| +| `pkg/operator/assets/bindata.go` | `make update-bindata` | +| `pkg/controller/client/fakes/fake_ctrl_client.go` | `go generate ./pkg/controller/client/...` | +| `**/zz_generated.deepcopy.go` | `make generate` | +| CRD YAML in `config/crd/bases/` | `make manifests` | + +To regenerate everything at once: ```sh -kubectl apply -f https://raw.githubusercontent.com//external-secrets-operator//dist/install.yaml +make update # generate + manifests + update-operand-manifests + update-bindata + bundle + docs ``` -> **NOTE:** Run `make help` for more information on all potential `make` targets +## Further Documentation -More information can be found via the [Kubebuilder Documentation](https://book.kubebuilder.io/introduction.html) +### Domain Guidelines and Agentic Documentation -## Testing +Detailed rules, architecture deep-dives, ADRs, and development workflows are in `harness-evals/harness-docs/`: -From the repo root you can run: +| Guideline | Scope | +|-----------|-------| +| [Security](harness-evals/harness-docs/security-guidelines.md) | CEL validation, annotation/label restrictions, container hardening, RBAC, network policies, TLS | +| [Performance](harness-evals/harness-docs/performance-guidelines.md) | Label-filtered caches, change detection, event predicates, requeue strategy, concurrency | +| [Error Handling](harness-evals/harness-docs/error-handling-guidelines.md) | Error classification, status conditions, requeue matrix, events | +| [API Contracts](harness-evals/harness-docs/api-contracts-guidelines.md) | Singleton enforcement, field immutability, CEL rules, `.testsuite.yaml` patterns | +| [Testing](harness-evals/harness-docs/testing-guidelines.md) | Unit tests, API integration tests, E2E with Ginkgo labels | +| [Integration](harness-evals/harness-docs/integration-guidelines.md) | cert-manager, OLM, proxy, trusted CA, console, metrics, multi-arch, webhooks | -| Target | Description | -|--------|-------------| -| `make test-unit` | Run unit tests (excluding test/e2e, test/apis, test/utils). | -| `make test-apis` | Run API integration tests (Ginkgo tests in `test/apis` using envtest). | -| `make test` | Run `test-apis` and `test-unit` (no cluster required). | -| `make test-e2e` | Run end-to-end tests against a live cluster. | +- `harness-evals/harness-docs/ESO_DEVELOPMENT.md` -- Development workflows, build targets, common tasks +- `harness-evals/harness-docs/ESO_TESTING.md` -- Test suites, patterns, E2E labels +- `harness-evals/harness-docs/architecture/` -- Controller internals, resource management, bindata pipeline +- `harness-evals/harness-docs/domain/` -- API documentation for ExternalSecretsConfig and ExternalSecretsManager +- `harness-evals/harness-docs/decisions/` -- Component-specific architectural decision records +- `harness-evals/harness-docs/references/` -- Enhancement proposals catalog and ecosystem links -For e2e tests, including prerequisites and suite-specific commands (e.g. label filters for AWS, Bitwarden, cross-platform), see [test/e2e/README.md](test/e2e/README.md). Example: +## For AI Agents -```sh -make test-e2e E2E_GINKGO_LABEL_FILTER="" -``` +If you are an AI agent or LLM-based tool working on this repository, start with [AGENTS.md](AGENTS.md). It indexes critical patterns and deeper documentation in `harness-evals/harness-docs/`. Claude Code–specific build commands and behavioral preferences are in [CLAUDE.md](CLAUDE.md). + +Recommended reading order: `AGENTS.md` → relevant `harness-evals/harness-docs/*-guidelines.md` → `domain/` → `architecture/` → `decisions/` → `ESO_DEVELOPMENT.md` ## Contributing -We welcome contributions from the community! To contribute: -- Fork this repository and create a new branch. -- Make your changes and test them thoroughly. -- Run make targets to verify the behavior. -- Submit a Pull Request describing your changes and the motivation behind them. -- Run make help to view all available development targets. +We welcome contributions from the community! See [CONTRIBUTING.md](CONTRIBUTING.md) for the full workflow (setup, conventions, PR checklist, and testing expectations). + +In short: fork, branch from `main`, run `make verify` / `make test` / `make lint`, and open a PR that explains what changed and why. + +## Security -We appreciate issues, bug reports, feature requests, and feedback! +To report a vulnerability, see [SECURITY.md](SECURITY.md). Do not open a public GitHub issue for security reports. + +## External References + +- [External Secrets Operator on OpenShift (Red Hat docs)](https://docs.redhat.com/en/documentation/openshift_container_platform/latest/html/security_and_compliance/external-secrets-operator-for-red-hat-openshift) +- [external-secrets upstream project](https://external-secrets.io/latest/) +- [cert-manager Operator for Red Hat OpenShift](https://docs.redhat.com/en/documentation/openshift_container_platform/latest/html/security_and_compliance/cert-manager-operator-for-red-hat-openshift) +- [Enhancement: ESO on OpenShift](https://github.com/openshift/enhancements/blob/master/enhancements/external-secrets-operator/external-secrets-operator.md) +- [Kubebuilder Documentation](https://book.kubebuilder.io/introduction.html) ## License -Copyright 2025. +Copyright 2025-2026. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -172,4 +220,3 @@ distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. - diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 000000000..dd562bb9e --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,38 @@ +# Security Policy + +## Reporting a Vulnerability + +If you discover a security vulnerability in the External Secrets Operator for Red Hat OpenShift, please report it responsibly. **Do not open a public GitHub issue for security vulnerabilities.** + +### Contact + +Email: **external-secrets-oape@redhat.com** + +### What to Include + +- Description of the vulnerability +- Steps to reproduce +- Affected versions (operator and/or operand) +- Potential impact +- Any suggested fix or mitigation + +### Response + +- You will receive an acknowledgement within 3 business days. +- We will work with you to understand the issue and coordinate a fix. +- We will provide credit in the advisory unless you prefer to remain anonymous. + +## Supported Versions + +Security fixes are applied to the latest release branch. Refer to the [Red Hat External Secrets Operator documentation](https://docs.redhat.com/en/documentation/openshift_container_platform/latest/html/security_and_compliance/external-secrets-operator-for-red-hat-openshift) for supported OpenShift versions. + +## Security Practices + +This operator enforces several security controls documented in [harness-evals/harness-docs/security-guidelines.md](harness-evals/harness-docs/security-guidelines.md), including: + +- Hardened container security contexts (non-root, read-only filesystem, dropped capabilities) +- CEL validation on CRD fields to prevent misconfiguration +- Deny-all-first network policy model for the operand namespace +- RBAC least-privilege with auto-generated ClusterRoles +- PEM validation rejecting private keys and non-CA certificates +- Reserved annotation/label/env-var domain blocking diff --git a/docs/anti-patterns/DUAL_CACHE_FIX.md b/docs/anti-patterns/DUAL_CACHE_FIX.md index 20ef68b6f..f1ad51e74 100644 --- a/docs/anti-patterns/DUAL_CACHE_FIX.md +++ b/docs/anti-patterns/DUAL_CACHE_FIX.md @@ -11,12 +11,14 @@ The external-secrets-operator controller was using **two separate caches**: 2. **Custom cache** - For reading objects during reconciliation This created a race condition: -``` + +```text OLD (Race Condition): Manager cache syncs → triggers reconcile → reads from different custom cache → might not be synced yet ``` ### Consequences + - Potential "object not found" errors despite object existing - Race condition during startup - Unnecessary memory and network overhead (both caches watching same resources) @@ -26,18 +28,19 @@ Manager cache syncs → triggers reconcile → reads from different custom cache Replaced the dual-cache pattern with a **single unified cache**: -``` +```text NEW (No Race): Manager cache syncs → triggers reconcile → reads from SAME manager cache → guaranteed synced ``` ### Changes Made -####1. Configure Manager Cache with Label Selectors +#### 1. Configure Manager Cache with Label Selectors **File:** `cmd/external-secrets-operator/main.go` Added `NewCache` option to manager configuration: + ```go mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ // ... existing options ... @@ -71,6 +74,7 @@ func NewCacheBuilder() cache.NewCacheFunc { #### 3. Simplified Client Creation **Before:** + ```go func NewClient(m manager.Manager, r *Reconciler) (operatorclient.CtrlClient, error) { c, err := BuildCustomClient(m, r) // Created separate custom cache @@ -82,6 +86,7 @@ func NewClient(m manager.Manager, r *Reconciler) (operatorclient.CtrlClient, err ``` **After:** + ```go func NewClient(m manager.Manager, r *Reconciler) (operatorclient.CtrlClient, error) { // Use the manager's client directly - it reads from the manager's cache @@ -123,22 +128,26 @@ Deleted the entire `BuildCustomClient()` function and its associated logic (~100 ## Benefits ### 1. Eliminates Race Condition ✅ + - Controller-runtime guarantees cache sync before reconciliation starts - No more potential for reading from an unsynced cache - Deterministic behavior ### 2. Reduces Resource Usage ✅ + - **Before:** 2 caches × N resources = 2N watch connections + 2N cached objects - **After:** 1 cache × N resources = N watch connections + N cached objects - **Memory saved:** ~50% - **Network traffic saved:** ~50% ### 3. Simplifies Code ✅ + - Removed ~100 lines of custom cache management code - Clearer control flow: one cache, one source of truth - Easier to understand and maintain ### 4. Follows Best Practices ✅ + - Uses standard controller-runtime pattern - Same solution as cert-manager-operator ([PR #324](https://github.com/openshift/cert-manager-operator/pull/324)) - Leverages controller-runtime's built-in cache synchronization guarantees @@ -146,7 +155,8 @@ Deleted the entire `BuildCustomClient()` function and its associated logic (~100 ## Architecture Comparison ### Before (Dual Cache) -``` + +```text ┌──────────────────────────────────────────┐ │ Kubernetes API Server │ └────────────┬─────────────────┬───────────┘ @@ -171,7 +181,8 @@ Deleted the entire `BuildCustomClient()` function and its associated logic (~100 ``` ### After (Unified Cache) -``` + +```text ┌──────────────────────────────────────────┐ │ Kubernetes API Server │ └────────────┬─────────────────────────────┘ @@ -200,11 +211,13 @@ Deleted the entire `BuildCustomClient()` function and its associated logic (~100 ### Verification Steps 1. **Build the operator:** + ```bash make build ``` 2. **Deploy and observe:** + ```bash # Check cache initialization logs kubectl logs -n external-secrets-operator deployment/external-secrets-operator-controller-manager | grep "cache-setup" @@ -214,6 +227,7 @@ Deleted the entire `BuildCustomClient()` function and its associated logic (~100 ``` 3. **Test reconciliation:** + ```bash # Create/update ExternalSecretsConfig kubectl apply -f config/samples/operator_v1alpha1_externalsecretsconfig.yaml @@ -232,9 +246,11 @@ Deleted the entire `BuildCustomClient()` function and its associated logic (~100 ## Migration Notes ### Breaking Changes + **None.** This is an internal implementation change with no API changes. ### Rollback + If issues occur, revert this commit to restore the dual-cache implementation. ## References @@ -247,4 +263,3 @@ If issues occur, revert this commit to restore the dual-cache implementation. ## Credits Solution inspired by the fix implemented in cert-manager-operator by the OpenShift cert-manager team for a similar race condition in the istio-csr controller. - diff --git a/docs/anti-patterns/DUAL_CACHE_FIX_SUMMARY.md b/docs/anti-patterns/DUAL_CACHE_FIX_SUMMARY.md index a18b61517..1e27cc876 100644 --- a/docs/anti-patterns/DUAL_CACHE_FIX_SUMMARY.md +++ b/docs/anti-patterns/DUAL_CACHE_FIX_SUMMARY.md @@ -16,6 +16,7 @@ Fixed the **dual cache race condition** by replacing two separate caches with a ### Key Changes #### 1. Manager Configuration (`main.go`) + ```diff mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ Scheme: scheme, @@ -27,6 +28,7 @@ mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ ``` #### 2. New Cache Builder (`controller.go`) + ```go // NEW: Configure manager's cache with label selectors func NewCacheBuilder() cache.NewCacheFunc { @@ -45,6 +47,7 @@ func buildCacheObjectList() map[client.Object]cache.ByObject { ``` #### 3. Simplified Client Creation + ```diff func NewClient(m manager.Manager, r *Reconciler) (operatorclient.CtrlClient, error) { - c, err := BuildCustomClient(m, r) // OLD: Created separate custom cache @@ -60,6 +63,7 @@ func NewClient(m manager.Manager, r *Reconciler) (operatorclient.CtrlClient, err ``` #### 4. Removed + - ❌ `BuildCustomClient()` function (~100 lines) - ❌ Custom cache creation logic - ❌ Custom client configuration @@ -68,7 +72,8 @@ func NewClient(m manager.Manager, r *Reconciler) (operatorclient.CtrlClient, err ## Before vs After ### Before (❌ Race Condition) -``` + +```text Kubernetes API ├── Manager Cache (watches) → Triggers Reconciliation └── Custom Cache (reads) → Might not be synced! @@ -77,7 +82,8 @@ Problem: Reconciler might read from unsynced custom cache ``` ### After (✅ No Race) -``` + +```text Kubernetes API └── Unified Manager Cache → Both watches AND reads @@ -97,6 +103,7 @@ Solution: Same cache for everything, guaranteed synced ## Testing ### Build Status + ```bash $ make build ✅ SUCCESS - No compilation errors @@ -105,21 +112,25 @@ $ make build ### Verification Steps 1. **Deploy the operator:** + ```bash make deploy ``` 2. **Check logs for unified cache:** + ```bash kubectl logs -n external-secrets-operator deployment/external-secrets-operator-controller-manager | grep "cache-setup" ``` 3. **Create test resource:** + ```bash kubectl apply -f config/samples/operator_v1alpha1_externalsecretsconfig.yaml ``` 4. **Verify immediate reconciliation:** + ```bash kubectl get externalsecretsconfig cluster -w # Should show READY immediately, no delays @@ -128,16 +139,20 @@ $ make build ## Migration Path ### For Developers + No action needed - internal implementation change only. ### For Operators + 1. Rebuild operator image 2. Deploy new version 3. Observe reduced memory usage 4. Verify no "not found" errors ### Rollback Plan + If issues occur: + ```bash git revert make build && make deploy @@ -146,12 +161,14 @@ make build && make deploy ## Performance Impact ### Expected Improvements + - ✅ **50% reduction** in watch connections to API server - ✅ **50% reduction** in memory for cached objects - ✅ **No more race conditions** during startup/reconciliation - ✅ **Faster reconciliation** (no cache sync delays) ### Monitoring + Watch these metrics after deployment: - Memory usage of operator pod (should decrease) - API server watch connection count (should decrease) @@ -160,6 +177,7 @@ Watch these metrics after deployment: ## Related Work ### Inspiration + This fix is based on the solution implemented in cert-manager-operator: - **PR:** https://github.com/openshift/cert-manager-operator/pull/324 - **Issue:** https://issues.redhat.com/browse/CM-735 @@ -167,6 +185,7 @@ This fix is based on the solution implemented in cert-manager-operator: - **Solution:** Unified cache approach (same as applied here) ### References + - [Controller-Runtime Cache Documentation](https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/cache) - [Kubebuilder Book - Caching](https://book.kubebuilder.io/reference/watching-resources.html) - [Original AI Dialogue](https://gist.github.com/lunarwhite/8928d1dc8e35d0d23e6cc7a364985215) @@ -199,4 +218,3 @@ Additional documentation created: **Build:** ✅ Passing **Tests:** ⏭️ Pending E2E validation **Risk Level:** 🟡 Medium (internal refactoring, well-tested pattern) - diff --git a/docs/anti-patterns/README.md b/docs/anti-patterns/README.md index dcdbf68a4..71502da86 100644 --- a/docs/anti-patterns/README.md +++ b/docs/anti-patterns/README.md @@ -37,23 +37,27 @@ Documenting anti-patterns helps: Based on common Kubernetes operator issues, watch for: ### Controller Patterns + - [ ] **Unbounded Reconciliation** - Missing rate limiting or exponential backoff - [ ] **Status Update Loops** - Status updates triggering unnecessary reconciliations - [ ] **Missing Finalizers** - Resources not properly cleaned up on deletion - [ ] **Blocking Reconciliation** - Long-running operations without context timeouts ### Cache & Client Patterns + - [ ] **Cache Stampede** - All controllers resyncing simultaneously - [ ] **Over-caching** - Watching resources not actually used - [ ] **Direct API Calls** - Bypassing cache unnecessarily (use `UncachedClient` intentionally) - [ ] **Stale Reads** - Not handling cache sync properly ### Resource Management + - [ ] **Resource Leaks** - Not cleaning up created resources - [ ] **Owner Reference Missing** - Manual cleanup instead of garbage collection - [ ] **Unbounded Resource Creation** - No limits on child resources ### Error Handling + - [ ] **Silent Failures** - Errors not surfaced to status or events - [ ] **Panic in Reconciliation** - Unhandled panics crashing controller - [ ] **Error Shadowing** - Generic errors hiding root cause @@ -63,7 +67,8 @@ Based on common Kubernetes operator issues, watch for: When you discover an anti-pattern: 1. **Create a new document** following this structure: - ``` + + ```markdown # Anti-Pattern: [Name] ## Problem @@ -98,6 +103,7 @@ When you discover an anti-pattern: ## References ### Best Practices + - [Controller-Runtime Best Practices](https://pkg.go.dev/sigs.k8s.io/controller-runtime) - [Kubernetes Operator Patterns](https://kubernetes.io/docs/concepts/extend-kubernetes/operator/) - [Kubebuilder Book](https://book.kubebuilder.io/) @@ -115,4 +121,3 @@ Found an anti-pattern? Document it here! Include: **Maintained by:** External Secrets Operator Team **Last Updated:** 2025-10-19 - diff --git a/harness-evals/harness-docs/ESO_DEVELOPMENT.md b/harness-evals/harness-docs/ESO_DEVELOPMENT.md new file mode 100644 index 000000000..574666a35 --- /dev/null +++ b/harness-evals/harness-docs/ESO_DEVELOPMENT.md @@ -0,0 +1,167 @@ +# External Secrets Operator — Development Guide + +> **Generic Development Practices**: See [Platform Development Practices](https://github.com/openshift/enhancements/tree/master/ai-docs) for Go standards, controller-runtime patterns, and CI/CD workflows. + +This guide covers **ESO-specific** development practices. + +## Quick Start + +### Prerequisites + +- Go 1.26.0+ (from go.mod) +- Access to OpenShift cluster with `KUBECONFIG` set +- Podman (default container tool; Docker supported via `CONTAINER_TOOL=docker`) +- `yq` (installed via `make ensure-yq`) + +### Build + +```bash +make build-operator # Build operator binary +make build # Build all binaries +make image-build # Build container image (podman) +``` + +**Binary output**: `bin/external-secrets-operator` +**Version ldflags**: commitFromGit, versionFromGit, majorFromGit, minorFromGit, buildDate injected at build time. + +## Key Makefile Targets + +| Target | Purpose | +|--------|---------| +| `make build-operator` | Build operator binary | +| `make test` | Run all non-e2e tests (manifests + generate + fmt + vet + test-apis + test-unit) | +| `make test-unit` | Run unit tests only (excludes e2e, apis, utils) | +| `make test-apis` | Run envtest-based API validation tests | +| `make manifests` | Generate CRDs and RBAC via controller-gen | +| `make generate` | Generate deepcopy methods | +| `make update-operand-manifests` | Re-render upstream Helm → bindata manifests | +| `make bundle` | Generate OLM bundle | +| `make verify` | Run all verification checks | +| `make lint` | Run golangci-lint | +| `make fmt` | Format Go code | +| `make vet` | Run go vet | +| `make govulncheck` | Run Go vulnerability scanner | +| `make verify-deps` | Verify dependency integrity | + +## Common Tasks + +### 1. Bump Upstream External-Secrets Version + +```bash +# 1. Update EXTERNAL_SECRETS_VERSION in Makefile +# 2. Re-render manifests from upstream Helm charts +make update-operand-manifests + +# 3. Regenerate bindata +make update-bindata + +# 4. Update IMG_VERSION in Makefile if needed +# 5. Review generated diffs in bindata/ and config/crd/bases/ +# 6. Run tests +make test verify +``` + +The `hack/update-external-secrets-manifests.sh` script: +- Downloads upstream Helm charts +- Renders templates (cert-manager enabled + disabled variants) +- Strips Helm labels, relabels `managed-by` +- Customizes core deployment (disables leader election, cluster-store/push-secret reconcilers) +- Splits into individual YAML files in `bindata/external-secrets/` + +### 2. Add a New Managed Resource Type + +1. Add bindata YAML to `bindata/external-secrets/resources/` +2. Run `make update-bindata` so `pkg/operator/assets/bindata.go` picks up the new asset +3. Add asset name constant in `pkg/controller/external_secrets/constants.go` +4. Add `Decode*ObjBytes` function in `pkg/controller/common/utils.go` +5. Add creation/update logic in a new file under `pkg/controller/external_secrets/` +6. Add to `controllerManagedResources` in `controller.go` if the resource should be cleaned on delete +7. Add the type to `buildCacheObjectList()` when it should be watched via the label-filtered cache +8. Add the resource type to `HasObjectChanged` type-switch in `common/utils.go` +9. Wire into the installation order in `install_external_secrets.go` +10. Add unit tests +11. Run `make verify` before opening the PR + +### 3. Add a New Feature Toggle + +1. Add constant to `FeatureName` enum in `api/v1alpha1/meta.go` +2. Add kubebuilder enum validation marker +3. Map feature to container arg in `featureContainerArgs` in `constants.go` +4. If feature affects only specific deployments, add to that deployment's `supportedFeatures` slice +5. Run `make manifests generate` to regenerate CRDs and deepcopy + +### 4. Modify CRD API Types + +```bash +# 1. Edit types in api/v1alpha1/ +# 2. Regenerate +make manifests generate + +# 3. If CEL validation added, add test cases in test/apis/ +make test-apis + +# 4. Verify no git diff in generated files +make verify +``` + +### 5. Update RBAC Permissions + +```bash +# 1. Edit RBAC markers on controller methods (//+kubebuilder:rbac:...) +# 2. Regenerate +make manifests + +# 3. Review config/rbac/role.yaml changes +``` + +## Go Workspace + +This repo uses Go workspaces (`go.work`) with 4 modules: +- `.` (root) +- `cmd/external-secrets-operator` +- `test` +- `tools` + +`GOFLAGS` is overridden in the Makefile for workspace mode. + +## Key Environment Variables + +| Variable | Purpose | Set By | +|----------|---------|--------| +| `RELATED_IMAGE_EXTERNAL_SECRETS` | Operand image | `config/manager/manager.yaml`, OLM | +| `RELATED_IMAGE_BITWARDEN_SDK_SERVER` | Bitwarden image | `config/manager/manager.yaml`, OLM | +| `OPERAND_EXTERNAL_SECRETS_IMAGE_VERSION` | Version tracking | Makefile | +| `BITWARDEN_SDK_SERVER_IMAGE_VERSION` | Version tracking | Makefile | +| `OPERATOR_IMAGE_VERSION` | Operator version | `config/manager/manager.yaml` | + +## Common Mistakes + +1. **DO NOT hand-edit `pkg/operator/assets/bindata.go`** — it is generated by `make update-bindata` +2. **DO NOT hand-edit `pkg/controller/client/fakes/fake_ctrl_client.go`** — it is generated by counterfeiter +3. **DO NOT introduce Server-Side Apply** — the codebase uses `UpdateWithRetry`; SSA would conflict with cert-manager/CNO field ownership on co-managed resources +4. **DO NOT skip `make verify` before PRs** — it catches generated file drift +5. **DO NOT assume all deployments exist** — cert-controller is conditional (only when cert-manager disabled), bitwarden is conditional (only when enabled) +6. **DO NOT use reserved annotation domains** — `kubernetes.io/`, `openshift.io/`, `cert-manager.io/`, `k8s.io/` are blocked by CEL validation +7. **DO NOT modify cert-manager config fields after creation** — `mode`, `injectAnnotations`, `issuerRef` are immutable via CEL +8. **DO NOT return both `RequeueAfter` and a non-nil error** from `Reconcile` — return one or the other +9. **DO NOT forget `buildCacheObjectList()`** when adding watched/managed resource types — also update `controllerManagedResources` and `HasObjectChanged` +10. **DO NOT wrap `Decode*ObjBytes` in error handling** — those helpers panic on failure by design for build-time-constant assets +11. **DO NOT put operand RBAC in `+kubebuilder:rbac` markers** — operator permissions go in controller Go markers; operand RBAC is static YAML under `bindata/` + +## FIPS + +`make build-operator` / `make build` source `hack/go-fips.sh`, which enables `GOEXPERIMENT=strictfipsruntime` and build tags `strictfipsruntime,openssl` when the Go compiler supports it. + +Container image builds (`make image-build` / `Dockerfile`) currently run `CGO_ENABLED=0 go build` **without** the FIPS script or FIPS tags. Treat the root `Dockerfile` path as a non-FIPS local/dev image build unless/until it is wired through the FIPS build path (CI operand images under `images/ci/` use a separate FIPS-oriented Dockerfile). + +## See Also + +- [Testing Guide](./ESO_TESTING.md) +- [Architecture](./architecture/components.md) +- [Platform Development Practices](https://github.com/openshift/enhancements/tree/master/ai-docs) + +## SME Review Recommended + +- Detailed steps for adding a new operand component end-to-end (deployment + service + RBAC + network policy + bindata) +- CI/CD pipeline specifics beyond `.ci-operator.yaml` +- Release process for operator version bumps diff --git a/harness-evals/harness-docs/ESO_TESTING.md b/harness-evals/harness-docs/ESO_TESTING.md new file mode 100644 index 000000000..bc0d2ebbd --- /dev/null +++ b/harness-evals/harness-docs/ESO_TESTING.md @@ -0,0 +1,173 @@ +# External Secrets Operator — Testing Guide + +This guide covers **ESO-specific** test suites and patterns. + +## Test Organization + +```text +pkg/controller/external_secrets/*_test.go # Unit tests (controller logic) +test/apis/ # API integration tests (CEL validation via envtest) +test/e2e/ # E2E tests (real OpenShift cluster) +test/utils/ # Shared test utilities +``` + +## Unit Tests + +### Running Unit Tests + +```bash +make test # All non-e2e tests (manifests, generate, fmt, vet, test-apis, test-unit) +make test-unit # Unit tests only +go test -v ./pkg/... # Specific package +go test -count=1 ./pkg/... # Disable cache +go test -cover ./pkg/... # With coverage +``` + +### Patterns + +**Client mocking**: Uses [counterfeiter](https://github.com/maxbrunsfeld/counterfeiter)-generated fakes at `pkg/controller/client/fakes/fake_ctrl_client.go`. + +```go +// Regenerate fakes after CtrlClient interface changes: +//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -generate +``` + +**Controller tests** (`pkg/controller/external_secrets/*_test.go`): +- Test individual reconciliation functions (deployment creation, RBAC, network policies, etc.) +- Use fake clients to simulate cluster state +- Verify resource field values, label/annotation application, error classification + +## API Integration Tests (envtest) + +### Running API Tests + +```bash +make test-apis # Via Makefile +go test -v ./test/apis/... -ginkgo.v # Direct +``` + +### Pattern + +Uses controller-runtime's `envtest.Environment` to run a real API server with CRDs installed. Tests are **data-driven** via `.testsuite.yaml` files: + +```go +// test/apis/suite_test.go +testEnv = &envtest.Environment{ + CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")}, +} +``` + +**Purpose**: Validates CEL rules, default values, and field validation. Requires Kubernetes 1.25+ for CEL support. + +**Test data**: `.testsuite.yaml` files live under `api/v1alpha1/tests/./` (e.g., `externalsecretsconfig.operator.openshift.io/`), loaded by `LoadTestSuiteSpecs` and executed by `GenerateTestSuite`. + +## E2E Tests + +### Running E2E Tests + +```bash +# Requires real OpenShift cluster with KUBECONFIG set +make test-e2e + +# With specific labels +E2E_GINKGO_LABEL_FILTER="Platform:Generic && Feature:NetworkPolicy" make test-e2e + +# Default filter excludes: Proxy, Upgrade, Bitwarden tests +``` + +**Build tag**: `//go:build e2e` — E2E tests are excluded from `make test`. + +### Ginkgo Label System + +| Label | Description | +|-------|-------------| +| `Platform:AWS` | Requires AWS cluster | +| `Platform:GCP` | Requires GCP cluster | +| `Platform:Generic` | Any cluster | +| `Provider:AWS` | Tests AWS Secrets Manager integration | +| `Provider:Vault` | Tests HashiCorp Vault integration | +| `Feature:NetworkPolicy` | Network policy lifecycle | +| `Feature:Proxy` | Proxy egress policy lifecycle | +| `Feature:OverrideEnv` | Per-component env var overrides | +| `Feature:RevisionHistoryLimit` | Deployment history configuration | +| `Feature:UnsafeAllowGenericTargets` | ESM feature toggle | +| `Feature:CustomAnnotations` | Annotation lifecycle and restoration | +| `Feature:CustomLabels` | Label lifecycle and restoration | +| `Skipped:Disconnected` | Excluded in disconnected environments | + +### Test Suite Setup + +```go +// test/e2e/e2e_suite_test.go +BeforeSuite: creates test namespace, initializes 3 clients (clientset, dynamic, runtime) +AfterSuite: dumps artifacts to ARTIFACT_DIR on failure, cleans up +``` + +### E2E Test Scenarios + +**Secret Store lifecycle** (`e2e_test.go`): +- AWS: SecretStore → ExternalSecret → verify Secret data → PushSecret → ClusterSecretStore +- Vault: SecretStore with token auth → ExternalSecret → verify sync + +**Drift detection** (`e2e_test.go`): +- Modify managed resource annotations/labels → verify operator restores them +- Per resource type: SA, Role, RoleBinding, ClusterRole, ClusterRoleBinding, Service, NetworkPolicy +- Deployment intentionally excluded from annotation restoration due to revision annotation churn + +**Feature toggles** (`e2e_test.go`): +- Enable/disable `UnsafeAllowGenericTargets` → verify container args on core deployment + +**Bitwarden** (`bitwarden_*.go`): +- Plugin lifecycle with cert-manager integration +- API-based bitwarden tests with secret sync verification + +**Trusted CA Bundle** (`trusted_ca_bundle_test.go`): +- User-specified CA bundle ConfigMap → verify volume mount and SSL_CERT_DIR env var + +### Test Utilities (`test/utils/`) + +| Utility | Purpose | +|---------|---------| +| `conditions.go` | `WaitForExternalSecretsConfigReady`, pod readiness polling | +| `external_secrets_config.go` | Cert-manager detection, operand pod prefix computation | +| `dynamic_resources.go` | Load YAML with pattern replacement | +| `aws_resources.go` | AWS credential fetching, secret management | +| `bitwarden_resources.go` | Bitwarden test resource management | +| `artifact_dump.go` | Failure artifact collection to `ARTIFACT_DIR` | +| `cleanup.go` | Test namespace and resource cleanup | +| `kube_client.go` | Pod exec, architecture detection, vault image selection | + +### Embedded Test Data + +```go +//go:embed testdata +var testData embed.FS +``` + +Test YAML manifests (SecretStores, ExternalSecrets, etc.) are embedded and loaded via `dynamic_resources.go`. + +## CI Configuration + +- `.ci-operator.yaml` — CI operator configuration +- Unit tests and API tests run on every PR +- E2E tests run on target clusters with appropriate labels + +## Debugging Test Failures + +```bash +# E2E artifacts +ls $ARTIFACT_DIR/ + +# Operator logs +oc logs -n external-secrets-operator deployment/external-secrets-operator-controller-manager + +# Operand status +oc get esc cluster -o yaml +oc get esm cluster -o yaml +oc get pods -n external-secrets +``` + +## See Also + +- [Development Guide](./ESO_DEVELOPMENT.md) +- [Architecture](./architecture/components.md) diff --git a/harness-evals/harness-docs/api-contracts-guidelines.md b/harness-evals/harness-docs/api-contracts-guidelines.md new file mode 100644 index 000000000..23285e8cb --- /dev/null +++ b/harness-evals/harness-docs/api-contracts-guidelines.md @@ -0,0 +1,185 @@ +# API Contracts Guidelines + +Guidelines for defining and validating CRD types in the external-secrets-operator. All rules are drawn from existing conventions in `api/v1alpha1/`. + +## General Rules + +- **No functions in the API package.** `api/v1alpha1/` must contain only type definitions, constants, kubebuilder markers, and generated code (deepcopy). Business logic, helpers, and utility functions belong in `pkg/controller/` or `pkg/operator/`. Exception: standard kubebuilder scaffolding (`init()` / `SchemeBuilder.Register()`, and `Resource()` in `groupversion_info.go`) is allowed. +- **Godoc is user-facing.** Field comments are extracted into API reference docs (`make docs`). Write them for cluster administrators, not only for operator developers. See [Godoc Requirements](#13-godoc-requirements). + +## 1. Singleton Enforcement + +Both CRDs are cluster-scoped singletons. Enforce with a CEL rule on the top-level type: + +```go +// +kubebuilder:validation:XValidation:rule="self.metadata.name == 'cluster'",message=" is a singleton, .metadata.name must be 'cluster'" +``` + +Test both the happy path (`resourceName: cluster`) and a rejection case in the `.testsuite.yaml`. + +## 2. Field Immutability + +Use `self == oldSelf` on the field itself, not on the parent struct: + +```go +// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="mode is immutable once set" +``` + +When applied to an object field, the entire sub-tree becomes immutable. Always add a corresponding `onUpdate` test that asserts a stable error substring (see [Test Patterns](#14-test-patterns-testsuiteyaml)). + +## 3. List Map Keys and `listType=map` + +Ordered lists that must be merge-patched by a unique key use: + +```go +// +listType=map +// +listMapKey= +``` + +Composite keys are supported by repeating `+listMapKey`. Write a test that submits duplicates and asserts the `Duplicate value` error. When a list is not merge-patched (e.g., tolerations), use `+listType=atomic`. + +## 4. `nolint:kubeapilinter` for listMapKey Fields + +Fields that serve as `listMapKey` must NOT carry `omitempty` in their JSON tag. Suppress the kube-api-linter warning with a reason: + +```go +//nolint:kubeapilinter // Name is a listMapKey and must not have omitempty for proper patch identification +Name string `json:"name"` +``` + +## 5. CEL Cross-Field Validation + +Place rules on the lowest struct that contains all referenced fields. Guard every path segment with `has()`: + +```go +// +kubebuilder:validation:XValidation:rule="self.mode != 'Enabled' || has(self.issuerRef)",message="issuerRef must be provided when mode is set to Enabled." +``` + +Test both failing and passing paths. + +## 6. List Key Immutability via CEL + +To make list map keys immutable while allowing other field changes: + +```go +// +kubebuilder:validation:XValidation:rule="oldSelf.all(op, self.exists(p, p.name == op.name && p.componentName == op.componentName))",message="name and componentName fields in networkPolicies are immutable" +``` + +New entries are allowed; removal or key changes are rejected. + +## 7. Reserved Domain Blocking + +Annotations use layered CEL rules: key format regex, prefix length <= 253, name part <= 63, reserved domain block. Each reserved domain group gets its own rule and message. Test subdomains, non-matching lookalikes, and max-length boundaries. + +## 8. Reserved Environment Variable Names + +Block reserved names/prefixes with a single CEL expression on the list: + +```go +// +kubebuilder:validation:XValidation:rule="self.all(e, !['KUBERNETES_', 'EXTERNAL_SECRETS_'].exists(p, e.name.startsWith(p)) && e.name != 'HOSTNAME' && e.name != 'SSL_CERT_DIR' && e.name != 'SSL_CERT_FILE')" +``` + +Test that exact matches are blocked but superstrings are allowed (e.g., `HOSTNAME_SUFFIX` is valid). + +## 9. Enum Validation + +Mode/state fields use explicit enum markers: + +```go +// +kubebuilder:validation:Enum:=Enabled;Disabled +``` + +Note: The codebase has one case using `Enum=` instead of `Enum:=` for `ManagementState`. Define a named Go type with `const` values. Use case-insensitive CEL (`lowerAscii()`) only when the API must accept mixed-case input. + +## 10. Bounds + +| Constraint | Typical values | +|---|---| +| Kubernetes names | MinLength=1, MaxLength=253 | +| Namespace names | MinLength=1, MaxLength=63 | +| Proxy URLs | MinLength=0, MaxLength=2048 | +| Labels/annotations maps | MinProperties=0, MaxProperties=20 | +| Tolerations | MinItems=0, MaxItems=50 | +| NetworkPolicies | MinItems=0, MaxItems=50 | +| ComponentConfigs | MinItems=0, MaxItems=4 | +| logLevel | Min=1, Max=5 | +| revisionHistoryLimit | Min=1, Max=50 | + +## 11. Defaults + +**Going forward:** prefer defaulting within the controller, not in the CRD schema. Per OpenShift API conventions: *"With configuration APIs, we typically default fields within the controller and not within the API. This means that the platform has the ability to make changes to the defaults over time."* + +New fields should omit `+kubebuilder:default` and apply defaults at reconcile time so defaults can evolve across releases without CRD schema migrations. + +Existing fields that already use `+kubebuilder:default` are legacy — do not add more: + +| Field | Default | +|---|---| +| `logLevel` | `1` | +| `mode` | `Disabled` | +| `injectAnnotations` | `"false"` | +| `certificateCheckInterval` | `"5m"` | +| `certificateDuration` | `"8760h"` | +| `certificateRenewBefore` | `"30m"` | +| `revisionHistoryLimit` | `10` | +| `networkPolicyProvisioning` | `Managed` | +| `key` (`ConfigMapKeyReference`) | `"ca-bundle.crt"` | + +## 12. Kubebuilder Markers Reference + +| Marker | Placement | +|---|---| +| `+kubebuilder:object:root=true` | Top-level CRD type and List type | +| `+kubebuilder:subresource:status` | Top-level CRD type | +| `+kubebuilder:resource:path=...,scope=Cluster` | Top-level CRD type | +| `+kubebuilder:validation:XValidation` | Type or field for CEL rules | +| `+kubebuilder:validation:Enum` | Fields with closed set of values | +| `+listType=map/atomic` | Slice fields | +| `+listMapKey=` | Slice fields with `listType=map` | +| `+mapType=granular/atomic` | Map fields | +| `+optional` / `+required` | Every exported field | + +## 13. Godoc Requirements + +Every exported field in `api/v1alpha1/` types must have a Godoc comment that: + +- Explains the field's **purpose** clearly enough for an end user unfamiliar with the implementation +- Documents **interactions** with other fields (e.g., "Only relevant when `certManager.mode` is `Enabled`") +- States **limitations** or constraints (e.g., max length, immutability, allowed values) +- Describes **default behavior** when the field is omitted or zero-valued + +Godoc is the primary user-facing API documentation and is extracted into generated API reference docs via `make docs`. Write for a cluster administrator audience. + +## 14. Test Patterns (`.testsuite.yaml`) + +Tests live in `api//tests/./` (singular kind, e.g. `externalsecretsconfig.operator.openshift.io/`) and use the declarative YAML format consumed by `test/apis/generator.go`. Inside the suite YAML, `crdName` is the **plural** Kubernetes CRD name (e.g. `externalsecretsconfigs.operator.openshift.io`). + +```yaml +name: "ExternalSecretsConfig" +# Directory: api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/ +# crdName: plural CRD name (not the directory segment) +crdName: externalsecretsconfigs.operator.openshift.io +tests: + onCreate: + - name: "Should be able to create a minimal instance" + resourceName: cluster + initial: | + + expected: | + + expectedError: "" + onUpdate: + - name: "Should reject immutable field change" + ... + expectedError: "field is immutable" +``` + +Coverage requirements per validation rule: +1. Happy-path create with defaults in `expected` +2. Rejection case with exact error substring in `expectedError` +3. Boundary tests (max length, max items) +4. For immutability: `onUpdate` test changing the immutable field +5. For cross-field rules: all passing and failing combinations +6. For list map keys: duplicate entries, valid multi-entry lists, key immutability + +Test names start with "Should" and describe the outcome. diff --git a/harness-evals/harness-docs/architecture/components.md b/harness-evals/harness-docs/architecture/components.md new file mode 100644 index 000000000..37e933bdd --- /dev/null +++ b/harness-evals/harness-docs/architecture/components.md @@ -0,0 +1,268 @@ +# External Secrets Operator — Architecture + +## Overview + +The OpenShift External Secrets Operator (ESO) manages the lifecycle of the upstream [external-secrets](https://github.com/external-secrets/external-secrets) project on OpenShift. It is **not** a fork — it deploys and configures the upstream operand via static YAML manifests embedded as bindata. + +**Framework**: controller-runtime v0.23.3 (no library-go, no operator-sdk Go libraries) +**Reconciliation**: Standard Update with `RetryOnConflict` — **NOT Server-Side Apply** +**Operand version**: external-secrets v2.5.0 + +## Repository Layout + +```text +api/v1alpha1/ # CRD types: ExternalSecretsConfig, ExternalSecretsManager +cmd/external-secrets-operator/ # Entrypoint: scheme registration, leader election, metrics +pkg/ +├── controller/ +│ ├── external_secrets/ # PRIMARY CONTROLLER — operand lifecycle +│ │ ├── controller.go # Reconciler, watches, error classification +│ │ ├── constants.go # All constants, asset names, label maps +│ │ ├── install_external_secrets.go # Ordered resource installation (11 steps) +│ │ ├── deployments.go # Deployment creation, image resolution, container security +│ │ ├── certificate.go # cert-manager Certificate resources +│ │ ├── configmap.go # Trusted CA bundle ConfigMap +│ │ ├── networkpolicy.go # Static (eso-sys-*) and user (eso-user-*) policies +│ │ ├── rbacs.go # ClusterRole, ClusterRoleBinding, Role, RoleBinding +│ │ ├── secret.go # Webhook TLS secret +│ │ └── validatingwebhook.go # Webhook configurations +│ ├── common/ # Shared utilities, error types, decode functions +│ ├── external_secrets_manager/ # ESM CONTROLLER — status aggregation +│ ├── crd_annotator/ # CRD ANNOTATOR — cert-manager CA injection (conditional) +│ └── client/ # CtrlClient interface with UpdateWithRetry, Exists +│ └── fakes/ # Counterfeiter-generated test fakes +pkg/operator/ +│ ├── setup_manager.go # Controller registration, default ESM creation +│ └── assets/bindata.go # Generated — DO NOT EDIT +pkg/version/ # Build-time ldflags (commit, version, date) +bindata/external-secrets/ # Source YAML manifests for operand resources +config/ # CRDs, RBAC, manager deployment, samples, console +bundle/ # OLM bundle (CRDs, metadata, console quickstarts) +hack/ # Build/update scripts +test/ +├── e2e/ # Ginkgo E2E tests (//go:build e2e) +├── apis/ # envtest-based API/CEL validation tests +└── utils/ # Test utilities (conditions, cleanup, artifact dump) +``` + +## Controllers + +### 1. ExternalSecrets Controller (primary) + +**File**: `pkg/controller/external_secrets/controller.go` +**Watches**: ExternalSecretsConfig (primary), all managed resources via label `app=external-secrets`, ExternalSecretsManager (spec changes) +**Singleton**: Always reconciles ExternalSecretsConfig named `cluster` + +#### Reconciliation Flow + +```text +Fetch ESC + ├─ if deleting: cleanup managed resources → remove finalizer → return + └─ else: ensure finalizer → Fetch ESM → processReconcileRequest +``` + +#### Resource Installation Order (`install_external_secrets.go`) + +Resources are applied in strict dependency order: + +1. Namespace (`external-secrets`) +2. Network Policies (static `eso-sys-*` + user `eso-user-*`) +3. Service Accounts +4. Certificates (when cert-manager enabled) +5. Secrets (webhook TLS) +6. Trusted CA Bundle ConfigMap (when proxy configured) +7. RBAC (ClusterRoles, ClusterRoleBindings, Roles, RoleBindings) +8. Services +9. Deployments (core, webhook, cert-controller, bitwarden — conditional) +10. Validating Webhooks +11. CR annotation tracking (MergePatch — only after all resources reconciled) + +#### Error Classification (`common/errors.go`) + +| Error Type | Effect | Requeue | +|-----------|--------|---------| +| `IrrecoverableError` | Degraded=True, Ready=False | No | +| `RetryRequiredError` | Degraded=False (Reason=Ready), Ready=False (Reason=Progressing) | Yes (30s) | +| `UserConfigurationError` | Degraded=True, Ready=False | Only if NotFound | + +#### Cache Configuration + +- Label-filtered cache scoped to `app=external-secrets` +- ConfigMaps cached by namespace (OperandDefaultNamespace only) +- Certificate informer conditionally registered (only when cert-manager CRD detected at startup via discovery API) +- `createWithFallback`: handles AlreadyExists from label-filtered cache misses + +#### Two Clients + +The ExternalSecrets reconciler maintains two Kubernetes clients: + +| Client | Field | Use | +|--------|-------|-----| +| Cached | `r.CtrlClient` | Normal access to managed operand resources in the label-filtered cache (`NewCacheBuilder` / `buildCacheObjectList`) | +| Uncached | `r.UncachedClient` | Objects not tracked by the cache (cert-manager Issuers, user-provided Secrets such as Bitwarden `secretRef`); also cache-miss fallbacks via `createWithFallback` / `createWithMetadataFallback` | + +**Invariant**: use `r.CtrlClient` for normal reads/writes of `controllerManagedResources`. Use `r.UncachedClient` for those same types only as a fallback when the label-filtered cache misses (for example, after an external actor strips the managed label and `Create` returns `AlreadyExists`). + +### 2. ExternalSecretsManager Controller + +**File**: `pkg/controller/external_secrets_manager/controller.go` +**Purpose**: Status aggregation — copies ESC conditions into ESM ControllerStatuses +**Auto-creation**: Creates default ESM CR at startup with retry + +### 3. CRD Annotator Controller (conditional) + +**File**: `pkg/controller/crd_annotator/controller.go` +**Purpose**: Adds `cert-manager.io/inject-ca-from` annotation to external-secrets CRDs +**Condition**: Only registered when cert-manager inject annotations enabled in ESC +**Cache**: Label-filtered by `external-secrets.io/component=controller` +**Update method**: MergePatch for CRD annotation updates + +## Resource Management Patterns + +### Update Strategy: NOT Server-Side Apply + +There is **no SSA usage** in this codebase. Full-resource updates use `UpdateWithRetry` (fresh Get, set ResourceVersion, Update). Metadata-only and annotation updates use JSON Patch or MergePatch instead. + +| Pattern | Used For | Code Reference | +|---------|----------|---------------| +| `UpdateWithRetry` | Full resource updates | `pkg/controller/client/client.go` | +| `createWithFallback` | Create with AlreadyExists handling | `pkg/controller/external_secrets/controller.go` | +| `patchResourceMetadata` | JSON Patch for metadata-only updates on co-managed resources (Secret, ConfigMap) | `pkg/controller/external_secrets/controller.go` | +| MergePatch | CR annotation tracking, CRD annotator | `install_external_secrets.go`, `crd_annotator/controller.go` | + +### Change Detection + +| Function | Purpose | Used For | +|----------|---------|----------| +| `HasObjectChanged` | Type-switch field-level comparison | Most resources | +| `ObjectMetadataModified` | Labels + managed annotation keys only | Secrets, ConfigMaps (Data managed externally) | +| `deploymentSpecModified` | Extensive field-by-field including order-insensitive env vars | Deployments | + +## Image Resolution + +Images are resolved from environment variables (OLM disconnected/mirror convention): + +| Env Var | Purpose | +|---------|---------| +| `RELATED_IMAGE_EXTERNAL_SECRETS` | External-secrets operand image | +| `RELATED_IMAGE_BITWARDEN_SDK_SERVER` | Bitwarden SDK server image | +| `OPERAND_EXTERNAL_SECRETS_IMAGE_VERSION` | Version tracking | +| `BITWARDEN_SDK_SERVER_IMAGE_VERSION` | Version tracking | + +Defaults are set in `config/manager/manager.yaml`. Missing image env var returns `IrrecoverableError` (no retry, no fallback). + +## Bindata Pipeline + +Operand manifests are sourced from upstream Helm charts, processed by `hack/update-external-secrets-manifests.sh`: + +```text +upstream Helm chart → helm template (cert-manager enabled + disabled variants) + → strip Helm labels → relabel managed-by → split into individual YAML files + → bindata/external-secrets/ → openshift/build-machinery-go add-bindata + → pkg/operator/assets/bindata.go (DO NOT EDIT) +``` + +Customizations applied during rendering: leader election disabled, cluster-store and push-secret reconcilers disabled in core deployment. + +**Decoding**: `runtime.Decode` with codec factory via `Decode*ObjBytes` functions in `common/utils.go`. + +## Conditional Components + +| Component | Condition | Deployment | +|-----------|-----------|------------| +| Core controller | Always | `external-secrets` | +| Webhook | Always | `external-secrets-webhook` | +| Cert-controller | When cert-manager **disabled** | `external-secrets-cert-controller` | +| Bitwarden SDK server | When bitwarden plugin **enabled** | `bitwarden-sdk-server` | +| Proxy CA ConfigMap | When proxy configured | `external-secrets-trusted-ca-bundle` | +| Proxy NetworkPolicy | When proxy configured + Managed | `eso-sys-allow-proxy-egress` | +| CRD Annotator controller | When cert-manager inject annotations enabled | (runs in operator) | + +## Network Policy Architecture + +**Static policies** (operator-managed, `eso-sys-*` prefix): +- `eso-sys-deny-all-traffic` — default deny +- `eso-sys-allow-api-server-egress-for-main-controller` — core controller to API server +- `eso-sys-allow-api-server-egress-for-webhook` — webhook traffic (ingress + egress) +- `eso-sys-allow-to-dns` — DNS resolution +- `eso-sys-allow-proxy-egress` — proxy egress (conditional on proxy + Managed) + +**User policies** (`eso-user-*` prefix): Built from `spec.controllerConfig.networkPolicies`. Only egress rules; operator auto-handles ingress. + +**Migration**: Unprefixed policies from versions < 1.2.0 are cleaned up once, tracked via `skipNPCleanupAnnotation`. TODO: Remove in v1.5.0. + +## Label Architecture + +**Label priority** (lowest to highest): ESM GlobalConfig labels < ESC ControllerConfig labels < `controllerDefaultResourceLabels` + +**Disallowed user labels** (regex): `^app.kubernetes.io/`, `^external-secrets.io/`, `^rbac.authorization.k8s.io/`, `^servicebinding.io/controller$`, `^app$` + +**Key labels**: +- `app=external-secrets` — ManagedResourceLabelKey, used for cache filtering and secondary watches +- `app.kubernetes.io/managed-by=external-secrets-operator` — ownership marker +- `externalsecretsconfig.operator.openshift.io/watching=true` — marks user-provided referenced resources + +## Managed Annotation Tracking + +Annotations from ESC spec are tracked via base64-encoded JSON in the `ManagedAnnotationsKey` annotation on the CR. When annotations are removed from spec, they appear in `DeletedAnnotationKeys` and are removed from all child resources. MergePatch is used for CR annotation update. + +## Proxy Configuration + +Layered resolution (highest priority first): ESC `spec.appConfig.proxy` > ESM `spec.globalConfig.proxy` > OLM-injected env vars (`HTTP_PROXY`, `HTTPS_PROXY`, `NO_PROXY`). + +Both uppercase and lowercase proxy env vars are set on operand containers. Trusted CA bundle injected via CNO-labeled ConfigMap with `config.openshift.io/inject-trusted-cabundle=true`. + +## Feature Gates + +| Feature | CR | Effect | +|---------|--------|--------| +| `UnsafeAllowGenericTargets` | ESM `.spec.features` | Passes `--unsafe-allow-generic-targets=true` to core controller | +| cert-manager integration | ESC `.spec.controllerConfig.certProvider.certManager` | Detected at startup via discovery API; conditionally registers CRD annotator + certificate informer | +| Bitwarden plugin | ESC `.spec.plugins.bitwardenSecretManagerProvider` | Deploys bitwarden-sdk-server | + +## Shared Utilities (`pkg/controller/common/`) + +| Symbol | Purpose | +|--------|---------| +| `HasObjectChanged` | Type-switch field-level comparison for resource drift | +| `ObjectMetadataModified` | Metadata-only comparison (labels + managed annotations) | +| `deploymentSpecModified` | Deployment-specific field comparison (env order-insensitive) | +| `ReconcileError` / `FromClientError` | Error classification (Irrecoverable, RetryRequired, UserConfiguration) | +| `EvalMode` / `ParseBool` | Mode and bool evaluation helpers | +| `IsFeatureEnabled` | Check feature toggle state from ESM | +| `AddFinalizer` / `RemoveFinalizer` | Finalizer management | +| `RemoveObsoleteAnnotations` | Annotation cleanup | +| `AddManagedMetadataAnnotation` / `GetPreviouslyAppliedAnnotationKeys` | Annotation tracking | +| `Decode*ObjBytes` | Typed bindata decoders (one per resource type) | +| `DefaultRequeueTime` | 30 seconds | + +## Container Security + +All operand containers enforce: +- `AllowPrivilegeEscalation: false` +- `Capabilities: drop ALL` +- `ReadOnlyRootFilesystem: true` +- `RunAsNonRoot: true` +- `SeccompProfile: RuntimeDefault` + +## OpenShift Integrations + +- **Trusted CA Bundle**: CNO injects cluster CA via labeled ConfigMap +- **Proxy**: Resolves from OLM-injected env vars as fallback +- **Console**: QuickStart content in `config/console/` +- **Metrics**: Secure metrics with OpenShift service CA +- **Multi-arch**: NodeAffinity for amd64, arm64, ppc64le, s390x + +## Anti-Patterns and TODOs + +1. `controller.go:349` — cert-manager CRD detection is startup-only; no runtime watch. TODO: Add dynamic CRD watch +2. `controller.go:584` — TODO: For GA, handle cleanup of operand resources on operator removal +3. `configmap.go:25` — TODO: ConfigMap removal when proxy config is removed (deferred) +4. `constants.go:133-138` — TODO: Remove NP migration cleanup in v1.5.0 +5. `common/validation_helpers_duplication.go` — Duplicated private k8s.io/kubernetes validation functions. TODO: Remove when upstream makes them public + +## SME Review Recommended + +- Recipes for adding a new operand component (deployment + service + RBAC + network policy wiring) +- Rationale behind startup-only cert-manager detection vs runtime watch +- Institutional knowledge around bindata update process when bumping upstream external-secrets version diff --git a/harness-evals/harness-docs/decisions/adr-0001-bindata-over-helm.md b/harness-evals/harness-docs/decisions/adr-0001-bindata-over-helm.md new file mode 100644 index 000000000..af2f1b6d3 --- /dev/null +++ b/harness-evals/harness-docs/decisions/adr-0001-bindata-over-helm.md @@ -0,0 +1,56 @@ +# ADR-0001: Bindata-Embedded Manifests Over Direct Helm Usage + +**Status**: Accepted +**Date**: 2025-05-15 +**Deciders**: ESO team +**Component**: External Secrets Operator + +## Context + +The operator needs to deploy the upstream external-secrets project. The upstream project distributes its manifests via Helm charts. The operator could either use Helm at runtime or pre-render and embed the manifests. + +## Decision + +Pre-render upstream Helm charts at build time via `hack/update-external-secrets-manifests.sh`, embed them as Go bindata using `openshift/build-machinery-go`, and decode them at runtime with `runtime.Decode`. + +## Rationale + +1. **No runtime Helm dependency** — the operator binary is self-contained with no need for Helm libraries or tiller +2. **Deterministic deployments** — manifests are version-pinned and reviewed in PRs +3. **OpenShift customization** — the rendering step strips Helm labels, relabels managed-by, disables leader election and cluster-store/push-secret reconcilers +4. **Consistency with OpenShift patterns** — other OpenShift operators (MCO, cluster-authentication-operator) use bindata embedding + +## Consequences + +### Positive + +- Operator image contains all manifests — works in disconnected environments +- Changes to operand manifests are visible in code review +- No runtime dependency on Helm chart repositories + +### Negative + +- Upstream version bump requires running the update script and reviewing generated diffs +- Customizations to the Helm rendering must be maintained in `hack/update-external-secrets-manifests.sh` + +### Neutral + +- Generated `pkg/operator/assets/bindata.go` must never be hand-edited + +## Alternatives Considered + +### Direct Helm Library Usage + +**Description**: Use the Helm Go SDK to render charts at runtime. +**Rejected because**: Adds a large dependency, makes deployments non-deterministic, and complicates disconnected environment support. + +### Kustomize Overlays + +**Description**: Use kustomize to customize upstream manifests. +**Rejected because**: Upstream distributes via Helm, not kustomize bases. Would require maintaining a separate kustomize layer on top of rendered Helm output without clear benefit over bindata. + +## References + +- `hack/update-external-secrets-manifests.sh` — manifest rendering pipeline +- `pkg/operator/assets/bindata.go` — generated bindata (DO NOT EDIT) +- [openshift/build-machinery-go](https://github.com/openshift/build-machinery-go) — bindata embedding tool diff --git a/harness-evals/harness-docs/decisions/adr-0002-update-with-retry-over-ssa.md b/harness-evals/harness-docs/decisions/adr-0002-update-with-retry-over-ssa.md new file mode 100644 index 000000000..7b54ae70e --- /dev/null +++ b/harness-evals/harness-docs/decisions/adr-0002-update-with-retry-over-ssa.md @@ -0,0 +1,50 @@ +# ADR-0002: UpdateWithRetry Over Server-Side Apply + +**Status**: Accepted +**Date**: 2025-05-15 +**Deciders**: ESO team +**Component**: External Secrets Operator + +## Context + +The operator must create and update Kubernetes resources for the operand deployment. Two primary patterns exist: Server-Side Apply (SSA) and traditional Update with conflict retry. Some resources (Secret, ConfigMap) have fields managed by external controllers (cert-manager, CNO), requiring careful handling. + +## Decision + +Use `UpdateWithRetry` (Get → set ResourceVersion → Update) as the primary update pattern. Use `patchResourceMetadata` (JSON Patch) for metadata-only updates on co-managed resources. Use MergePatch for CR annotation tracking. + +## Rationale + +1. **Simplicity** — `UpdateWithRetry` is straightforward and well-understood within the team +2. **Co-managed resources** — Secrets (Data managed by cert-controller/cert-manager) and ConfigMaps (Data managed by CNO) require metadata-only patches to avoid overwriting externally managed fields +3. **Field-level change detection** — Custom `HasObjectChanged` / `deploymentSpecModified` functions provide precise drift detection without SSA's field ownership complexity + +## Consequences + +### Positive + +- No field ownership conflicts with external controllers +- `ObjectMetadataModified` avoids unnecessary updates to Secrets and ConfigMaps whose Data is externally managed +- Precise change detection via type-specific comparison functions + +### Negative + +- Must maintain type-switch comparison functions (`HasObjectChanged`) for each resource type +- `createWithFallback` needed to handle AlreadyExists from label-filtered cache misses + +### Neutral + +- `RetryOnConflict` handles concurrent modification gracefully + +## Alternatives Considered + +### Server-Side Apply + +**Description**: Use SSA with field managers for all resource updates. +**Rejected because**: Would conflict with cert-manager and CNO field ownership on Secrets and ConfigMaps. SSA's "apply configurations" would require generating typed apply configs for every resource type. + +## References + +- `pkg/controller/client/client.go` — `UpdateWithRetry` implementation +- `pkg/controller/common/utils.go` — `HasObjectChanged`, `ObjectMetadataModified`, `deploymentSpecModified` +- `pkg/controller/external_secrets/secret.go` — `createWithMetadataFallback` pattern diff --git a/harness-evals/harness-docs/decisions/adr-0003-network-policy-naming-scheme.md b/harness-evals/harness-docs/decisions/adr-0003-network-policy-naming-scheme.md new file mode 100644 index 000000000..e0fbdeffc --- /dev/null +++ b/harness-evals/harness-docs/decisions/adr-0003-network-policy-naming-scheme.md @@ -0,0 +1,48 @@ +# ADR-0003: Network Policy Naming Scheme and Migration + +**Status**: Accepted +**Date**: 2025-09-12 +**Deciders**: ESO team +**Component**: External Secrets Operator + +## Context + +Operator v1.0.0-v1.1.x created network policies with unprefixed names. As user-defined network policies were introduced, a clear naming convention was needed to distinguish operator-managed from user-managed policies. Upgrading clusters need to migrate from unprefixed to prefixed names without disrupting connectivity. + +## Decision + +Adopt a two-prefix naming scheme: +- `eso-sys-*` for operator-managed static policies (deny-all, API server egress, DNS, proxy) +- `eso-user-*` for user-defined policies from `spec.controllerConfig.networkPolicies` + +Implement a one-time migration cleanup of unprefixed policies, tracked via the `skipNPCleanupAnnotation` on the ESC CR. + +## Rationale + +1. **Clear ownership** — prefix immediately identifies whether a policy is operator-managed or user-defined +2. **Safe migration** — one-time cleanup prevents orphaned unprefixed policies while the annotation prevents repeated cleanup attempts +3. **Future extensibility** — user policies can be added/modified via CR spec without naming collisions + +## Consequences + +### Positive + +- Users can identify policy ownership at a glance +- No naming collisions between operator and user policies +- Migration is idempotent (runs once per cluster) + +### Negative + +- Migration code and `skipNPCleanupAnnotation` must be maintained until v1.5.0 +- Network policy entries in the CR spec cannot be removed once added (CEL immutability constraint). The controller recreates a missing `eso-user-*` NetworkPolicy from the CR, so deleting the Kubernetes object alone does not revoke access. +- **Supported removal path today**: tighten or empty the policy's egress rules in place (same `name` + `componentName`), or leave the entry unused. Removing a list entry requires an API/CEL change via an [enhancement proposal](https://github.com/openshift/enhancements/tree/master/enhancements/external-secrets-operator) before implementation. + +### Neutral + +- User policy names are limited to 243 characters (`+kubebuilder:validation:MaxLength:=243`) so the `eso-user-` prefix fits within Kubernetes' 253-character name limit + +## References + +- `pkg/controller/external_secrets/networkpolicy.go` — policy creation and migration +- `pkg/controller/external_secrets/constants.go:133-138` — cleanup annotation and TODO +- Enhancement: [external-secrets-network-policy.md](https://github.com/openshift/enhancements/blob/master/enhancements/external-secrets-operator/external-secrets-network-policy.md) diff --git a/harness-evals/harness-docs/decisions/adr-template.md b/harness-evals/harness-docs/decisions/adr-template.md new file mode 100644 index 000000000..e8be3f723 --- /dev/null +++ b/harness-evals/harness-docs/decisions/adr-template.md @@ -0,0 +1,43 @@ +# ADR-NNNN: Title + +**Status**: Proposed | Accepted | Deprecated | Superseded +**Date**: YYYY-MM-DD +**Deciders**: Team or individuals +**Component**: External Secrets Operator + +## Context + +What is the component-specific issue or situation we're addressing? + +## Decision + +What component-specific decision did we make? + +## Rationale + +Why did we choose this approach for this component? + +## Consequences + +### Positive + +- Benefit 1 (component-specific) + +### Negative + +- Trade-off 1 (component-specific) + +### Neutral + +- Implication 1 + +## Alternatives Considered + +### Alternative 1 + +**Description**: ... +**Rejected because**: ... + +## References + +- Related component docs diff --git a/harness-evals/harness-docs/domain/external-secrets-config.md b/harness-evals/harness-docs/domain/external-secrets-config.md new file mode 100644 index 000000000..5b574c40c --- /dev/null +++ b/harness-evals/harness-docs/domain/external-secrets-config.md @@ -0,0 +1,167 @@ +# ExternalSecretsConfig + +**API Group**: `operator.openshift.io/v1alpha1` +**Kind**: `ExternalSecretsConfig` +**Scope**: Cluster (singleton, name must be `cluster`) +**Short Names**: `esc`, `externalsecretsconfig`, `esconfig` + +**API Definition**: [`api/v1alpha1/external_secrets_config_types.go`](../../api/v1alpha1/external_secrets_config_types.go) + +## Purpose + +Primary CR that triggers installation and configuration of the external-secrets operand. Creating this resource causes the operator to deploy all external-secrets components into the `external-secrets` namespace. + +**Key Principle**: Singleton pattern enforced via CEL rule — only one instance named `cluster` is permitted per cluster. + +## Spec Structure + +```go +type ExternalSecretsConfigSpec struct { + ApplicationConfig ApplicationConfig // Operand behavior: logLevel, resources, affinity, tolerations, nodeSelector, proxy, operatingNamespace, webhookConfig + Plugins PluginsConfig // Optional provider plugins (BitwardenSecretManagerProvider) + ControllerConfig ControllerConfig // Deployment config: certProvider, labels, annotations, networkPolicies, componentConfigs, trustedCABundle +} +``` + +### ApplicationConfig + +| Field | Type | Description | +|-------|------|-------------| +| `logLevel` | `int32` (1-5) | Kubernetes logging level, default 1 | +| `resources` | `ResourceRequirements` | CPU/memory requests and limits (immutable) | +| `affinity` | `Affinity` | Scheduling affinity rules | +| `tolerations` | `[]Toleration` (max 50) | Pod tolerations | +| `nodeSelector` | `map[string]string` (max 50) | Node label selectors | +| `proxy` | `ProxyConfig` | Proxy settings (httpProxy, httpsProxy, noProxy, networkPolicyProvisioning) | +| `operatingNamespace` | `string` (1-63 chars) | Restricts ESO to single namespace; disables ClusterSecretStore and ClusterExternalSecret | +| `webhookConfig` | `*WebhookConfig` | Webhook-specific settings (certificateCheckInterval, default "5m") | + +### ControllerConfig + +| Field | Type | Description | +|-------|------|-------------| +| `certProvider` | `*CertProvidersConfig` | Certificate management: cert-manager integration (mode, issuerRef, injectAnnotations) — **immutable once set** | +| `labels` | `map[string]string` (max 20) | Custom labels for all operand resources | +| `annotations` | `map[string]string` (max 20) | Custom annotations; reserved domains (`kubernetes.io/`, `openshift.io/`, `cert-manager.io/`, `k8s.io/`) blocked via CEL | +| `networkPolicies` | `[]NetworkPolicy` (max 50) | Custom egress rules per component; name+componentName immutable once added; operator prepends `eso-user-` prefix | +| `componentConfigs` | `[]ComponentConfig` (max 4) | Per-component overrides: `overrideEnv`, `revisionHistoryLimit` | +| `trustedCABundle` | `*ConfigMapKeyReference` | User CA bundle ConfigMap for outbound TLS; must exist in operand namespace | + +### ComponentConfig + +Per-component deployment-level overrides. Components: `ExternalSecretsCoreController`, `Webhook`, `CertController`, `BitwardenSDKServer`. + +| Field | Type | Description | +|-------|------|-------------| +| `componentName` | `ComponentName` (enum) | Target component | +| `overrideEnv` | `[]EnvVar` (max 50) | Custom env vars; reserved prefixes (`KUBERNETES_`, `EXTERNAL_SECRETS_`) and names (`HOSTNAME`, `SSL_CERT_DIR`, `SSL_CERT_FILE`) blocked | +| `deploymentConfigs.revisionHistoryLimit` | `*int32` (1-50) | ReplicaSet history limit, default 10 | + +### PluginsConfig + +| Field | Type | Description | +|-------|------|-------------| +| `bitwardenSecretManagerProvider.mode` | `Mode` (Enabled/Disabled) | Plugin state, default Disabled | +| `bitwardenSecretManagerProvider.secretRef` | `*SecretReference` | TLS secret for bitwarden server; required when Bitwarden is Enabled and cert-manager `mode` is not `Enabled` | + +**CEL Validation**: When Bitwarden `mode` is `Enabled`, either `secretRef` must be set **or** `controllerConfig.certProvider.certManager.mode` must be `Enabled` (a present cert-manager config with `mode: Disabled` does not satisfy this rule). + +## Status + +```go +type ExternalSecretsConfigStatus struct { + ConditionalStatus // Embeds []metav1.Condition (patchMergeKey=type) + ExternalSecretsImage string // Deployed external-secrets image + BitwardenSDKServerImage string // Deployed bitwarden image (if applicable) +} +``` + +### Conditions + +| Type | Status | Reason | Meaning | +|------|--------|--------|---------| +| `Ready` | True | `Ready` | Operand deployed and healthy | +| `Ready` | False | `Progressing` | Deployment in progress | +| `Ready` | False | `Failed` | Deployment failed | +| `Degraded` | True | `Failed` | Irrecoverable error (e.g., missing image env var) | +| `Degraded` | False | `Ready` | Operating normally | +| `UpdateAnnotation` | True/False | `Completed`/`Failed` | Annotation tracking status | + +## Lifecycle + +1. **Creation**: Controller installs operand namespace, network policies, RBAC, services, deployments, webhooks in strict dependency order +2. **Update**: Controller diffs each managed resource field-by-field (not SSA) and applies changes via `UpdateWithRetry` +3. **Deletion**: Finalizer-protected cleanup of operator-managed resources — namespace-scoped objects in `external-secrets`, plus cluster-scoped managed objects (ClusterRoles/Bindings, ValidatingWebhookConfigurations). Operand CRDs themselves are not deleted. + +## Example: Minimal Installation + +```yaml +apiVersion: operator.openshift.io/v1alpha1 +kind: ExternalSecretsConfig +metadata: + name: cluster +spec: {} +``` + +## Example: Full Configuration + +```yaml +apiVersion: operator.openshift.io/v1alpha1 +kind: ExternalSecretsConfig +metadata: + name: cluster +spec: + appConfig: + logLevel: 2 + proxy: + httpProxy: "http://proxy.example.com:3128" + httpsProxy: "https://proxy.example.com:3128" + noProxy: ".cluster.local,.svc,10.0.0.0/8" + networkPolicyProvisioning: Managed + controllerConfig: + certProvider: + certManager: + mode: Enabled + issuerRef: + name: my-issuer + kind: ClusterIssuer + group: cert-manager.io + injectAnnotations: "true" + labels: + team: platform + annotations: + custom.example.com/owner: "team-secrets" + networkPolicies: + - name: allow-vault-egress + componentName: ExternalSecretsCoreController + egress: + - to: + - ipBlock: + cidr: 10.0.1.0/24 + ports: + - port: 8200 + protocol: TCP + componentConfigs: + - componentName: ExternalSecretsCoreController + overrideEnv: + - name: MY_CUSTOM_VAR + value: "custom-value" + deploymentConfigs: + revisionHistoryLimit: 5 + plugins: + bitwardenSecretManagerProvider: + mode: Enabled +``` + +## Common Mistakes + +1. **Name must be `cluster`** — CEL validation rejects any other name +2. **cert-manager fields are immutable** — `mode`, `injectAnnotations`, `issuerRef` cannot be changed after initial set; delete and recreate the CR to change +3. **Network policy entries cannot be removed** — CEL rule `oldSelf.all(op, self.exists(...))` prevents removal of existing entries +4. **Reserved annotation domains** — `kubernetes.io/`, `openshift.io/`, `cert-manager.io/`, `k8s.io/` prefixes are rejected +5. **Reserved env var names** — `KUBERNETES_*`, `EXTERNAL_SECRETS_*`, `HOSTNAME`, `SSL_CERT_DIR`, `SSL_CERT_FILE` are blocked in `overrideEnv` +6. **Bitwarden requires TLS** — When enabling bitwarden without cert-manager, `secretRef` is mandatory + +## Related Concepts + +- [ExternalSecretsManager](./external-secrets-manager.md) — Global config and feature toggles diff --git a/harness-evals/harness-docs/domain/external-secrets-manager.md b/harness-evals/harness-docs/domain/external-secrets-manager.md new file mode 100644 index 000000000..39cce04b6 --- /dev/null +++ b/harness-evals/harness-docs/domain/external-secrets-manager.md @@ -0,0 +1,95 @@ +# ExternalSecretsManager + +**API Group**: `operator.openshift.io/v1alpha1` +**Kind**: `ExternalSecretsManager` +**Scope**: Cluster (singleton, name must be `cluster`) +**Short Names**: `esm`, `externalsecretsmanager`, `esmanager` + +**API Definition**: [`api/v1alpha1/external_secrets_manager_types.go`](../../api/v1alpha1/external_secrets_manager_types.go) + +## Purpose + +Global configuration and feature toggle CR. Auto-created by the operator during startup. Provides cluster-wide settings and aggregates status from all operator controllers. + +**Key Principle**: ESM is a centralized config that the operator creates and manages. Users modify it for global settings; status is read-only and aggregated from ESC conditions. + +## Spec Structure + +```go +type ExternalSecretsManagerSpec struct { + GlobalConfig *GlobalConfig // Cluster-wide common configs + labels + Features []Feature // Optional feature toggles (max 1 entry) +} +``` + +### GlobalConfig + +Inherits `CommonConfigs` (logLevel, resources, affinity, tolerations, nodeSelector, proxy) plus: + +| Field | Type | Description | +|-------|------|-------------| +| `labels` | `map[string]string` (max 20) | Labels applied to all operator-created resources | + +**Label priority** (lowest to highest): ESM GlobalConfig < ESC ControllerConfig < controller default labels. + +**Proxy resolution** (highest priority first): ESC spec > ESM GlobalConfig > OLM-injected env vars. + +### Features + +| Field | Type | Description | +|-------|------|-------------| +| `name` | `FeatureName` (enum) | Feature identifier; currently only `UnsafeAllowGenericTargets` | +| `mode` | `Mode` (Enabled/Disabled) | Feature state, default Disabled | + +**UnsafeAllowGenericTargets**: When enabled, passes `--unsafe-allow-generic-targets=true` to the core controller, allowing ExternalSecret resources to sync into non-Secret Kubernetes resources (ConfigMaps, custom resources). The operator-managed `external-secrets-controller` ClusterRole/Binding does **not** grant write access to arbitrary target resource types; administrators must create additional RBAC for the `external-secrets` ServiceAccount when using this feature. + +**Lifecycle note**: `CreateDefaultESMResource` runs at operator startup. If the ESM CR is deleted at runtime, the controller removes its finalizer but does not recreate the object until the operator process is restarted. + +## Status + +```go +type ExternalSecretsManagerStatus struct { + ControllerStatuses []ControllerStatus // Aggregated from ESC conditions + LastTransitionTime metav1.Time // Last condition change +} +``` + +**Note**: ESM uses a custom `Condition` type (Type, Status, Message) — NOT the full `metav1.Condition`. This is intentional per API linter comment. + +### ControllerStatuses + +Each entry represents a controller with its name, conditions, and `observedGeneration`. + +## Lifecycle + +1. **Creation**: Auto-created by operator at startup via `CreateDefaultESMResource` with retry logic (stops on AlreadyExists, Conflict, Invalid, BadRequest, Unauthorized, Forbidden, TooManyRequests) +2. **Update**: User modifies spec for global settings; controller updates status by aggregating ESC conditions +3. **Deletion**: Not user-managed — operator recreates on startup + +## Example: Enable Generic Targets + +```yaml +apiVersion: operator.openshift.io/v1alpha1 +kind: ExternalSecretsManager +metadata: + name: cluster +spec: + features: + - name: UnsafeAllowGenericTargets + mode: Enabled + globalConfig: + logLevel: 2 + labels: + environment: production +``` + +## Common Mistakes + +1. **Name must be `cluster`** — singleton enforced via CEL +2. **Max 1 feature entry** — `MaxItems:=1` on the features list +3. **Don't delete ESM** — the operator recreates it; configure via spec instead +4. **UnsafeAllowGenericTargets needs RBAC** — enabling without granting the operand additional permissions causes reconciliation failures + +## Related Concepts + +- [ExternalSecretsConfig](./external-secrets-config.md) — Per-deployment configuration and installation trigger diff --git a/harness-evals/harness-docs/error-handling-guidelines.md b/harness-evals/harness-docs/error-handling-guidelines.md new file mode 100644 index 000000000..f1f65439e --- /dev/null +++ b/harness-evals/harness-docs/error-handling-guidelines.md @@ -0,0 +1,128 @@ +# Error Handling Guidelines + +## Error Type Classification + +This operator uses three `ReconcileError` categories defined in `pkg/controller/common/errors.go`. Every error returned from reconciliation sub-functions must be wrapped in one of these. + +### IrrecoverableError + +Permanent failures that no retry can fix. The reconciler does **not** requeue. + +Use when: +- Required environment variables are missing (e.g., operand image env vars) +- An optional CRD is referenced in spec but not installed on the cluster +- The Kubernetes API returns Unauthorized, Forbidden, Invalid, or BadRequest + +```go +common.NewIrrecoverableError( + fmt.Errorf("%s environment variable not set", envVar), + "failed to update image in %s deployment object", name) +``` + +### RetryRequiredError + +Transient failures that may self-resolve. The reconciler requeues after `DefaultRequeueTime` (30s). + +Use when: +- Network timeouts, API server unavailability, resource conflicts +- Any Kubernetes API error not classified as irrecoverable by `FromClientError` + +### UserConfigurationError + +The user's CR spec references something invalid or missing. Sets `Degraded=True`. + +Use when: +- A referenced ConfigMap/Secret does not exist or lacks the expected key +- PEM data is malformed or contains non-CA certificates +- Proxy URL is invalid + +Requeue behavior depends on the root cause: +- **NotFound** (`IsUserConfigurationNotFound`): requeue after 30s (the missing object has no watch yet) +- **All other user config errors**: do not requeue; recovery is driven by watches + +## FromClientError Auto-Classification + +For Kubernetes API client errors, use `common.FromClientError` instead of manually choosing a category: + +| API Error | Mapped Reason | +|---|---| +| Unauthorized, Forbidden, Invalid, BadRequest | `IrrecoverableError` | +| NotFound, Conflict, Timeout, ServiceUnavailable, all others | `RetryRequiredError` | + +**When to override**: If a NotFound error reflects user misconfiguration (not infrastructure), wrap it as `NewUserConfigurationError` instead of calling `FromClientError`. + +## Status Condition Patterns + +| Condition | When True | When False | +|---|---|---| +| `Ready` | Operand deployed and healthy | Reconciliation failed or in progress | +| `Degraded` | Irrecoverable or user-config error | Normal operation | +| `UpdateAnnotation` | CRD annotations updated | Annotation update failed | + +Rules: +- On **IrrecoverableError** or **UserConfigurationError**: `Degraded=True`, `Ready=False`, `Reason=Failed` +- On **RetryRequiredError**: `Degraded=False` (Reason=Ready), `Ready=False` (Reason=Progressing) +- On **success**: `Degraded=False`, `Ready=True`, both `Reason=Ready` +- Always set `ObservedGeneration` from the CR's generation +- Only call `StatusUpdate` when a condition actually changed + +## Requeue Decision Matrix + +| Error Type | Requeue? | Returned Result | +|---|---|---| +| `IrrecoverableError` | No | `ctrl.Result{}, errUpdate` | +| `UserConfigurationError` (NotFound) | Yes, 30s | `ctrl.Result{RequeueAfter: DefaultRequeueTime}, nil` | +| `UserConfigurationError` (other) | No | `ctrl.Result{}, nil` | +| `RetryRequiredError` | Yes, 30s | `ctrl.Result{RequeueAfter: DefaultRequeueTime}, nil` | +| Success | No | `ctrl.Result{}, nil` | +| Status update failure | Yes (backoff) | `ctrl.Result{}, errUpdate` | + +## Error Wrapping Conventions + +1. **Prefer `%w` in `fmt.Errorf`** to preserve the error chain for `errors.Is`/`errors.As`. Simple error creation without wrapping is acceptable when no underlying error needs to be preserved. +2. **ReconcileError constructors wrap automatically** — do not double-wrap. +3. **Nil-safe constructors**: All `New*Error` and `FromClientError` return nil when input error is nil. +4. **Aggregate errors** for status update failures using `utilerrors.NewAggregate`. + +## RetryOnConflict Patterns + +### Status Updates + +Use `retry.RetryOnConflict` with `retry.DefaultRetry`. Re-fetch the latest object, deep-copy desired status, then call `StatusUpdate`. + +### Resource Updates (`UpdateWithRetry`) + +Re-fetches the object for the latest `resourceVersion` before each update attempt. + +### ESM Resource Creation + +`CreateDefaultESMResource` uses `retry.OnError` with `shouldRetryOnError` that stops on: AlreadyExists, Conflict, Invalid, BadRequest, Unauthorized, Forbidden, TooManyRequests. + +## Event Recording + +Events are recorded on the `ExternalSecretsConfig` CR using `r.eventRecorder.Eventf`. + +| Scenario | EventType | Reason | +|---|---|---| +| Resource created or updated | `Normal` | `Reconciled` | +| Resource already exists (adopted) | `Warning` | `ResourceAlreadyExists` | +| CR marked for deletion | `Warning` | `RemoveDeployment` | +| Trusted CA validation failure | `Warning` | Specific reason | + +### Throttled Validation Events + +`trusted_ca_bundle.go` uses `r.now.Do(f)` to emit at most one warning per degraded period. Resets on successful validation or when `trustedCABundle` is cleared. + +## Sub-Function Error Return + +Sub-functions in `install_external_secrets.go` return typed `ReconcileError` values. The top-level `reconcileExternalSecretsDeployment` propagates the first error and stops. When a sub-function encounters a user-config NotFound that should not block the entire deployment, it returns the deployment alongside the error for partial spec application. + +## Testing Error Classification + +Test with `Is*` helpers, not string-matching: + +```go +if !common.IsUserConfigurationError(err) { + t.Fatal("expected UserConfigurationError") +} +``` diff --git a/harness-evals/harness-docs/exec-plans/README.md b/harness-evals/harness-docs/exec-plans/README.md new file mode 100644 index 000000000..8e4dc8230 --- /dev/null +++ b/harness-evals/harness-docs/exec-plans/README.md @@ -0,0 +1,22 @@ +# Execution Plans + +Exec-plans are optional, time-bounded working notes for larger features. They are not a second architecture doc tree — keep durable decisions in [`../decisions/`](../decisions/) and enforceable rules in [`../*-guidelines.md`](../). + +## When to create one + +Create an exec-plan when a change spans multiple packages/PRs, needs an ordered rollout, or must track open questions before coding. Skip them for small, single-PR fixes. + +## Where they live + +```text +exec-plans/ +└── active/ # Feature-specific plans while work is in flight +``` + +Use a short descriptive filename (for example `active/network-policy-egress-plan.md`). There is no required template in this repository yet; a concise checklist with goal, steps, risks, and open questions is enough. Prefer linking an OpenShift enhancement under [`../references/enhancements.md`](../references/enhancements.md) when the work needs cross-repo design review. + +## How to use / complete + +1. Draft the plan in `active/` and link it from the Jira issue or PR. +2. Update the plan as assumptions change; keep it short. +3. When the feature lands, delete the plan from `active/` and capture lasting decisions as an ADR in [`../decisions/`](../decisions/). diff --git a/harness-evals/harness-docs/integration-guidelines.md b/harness-evals/harness-docs/integration-guidelines.md new file mode 100644 index 000000000..ebf77a047 --- /dev/null +++ b/harness-evals/harness-docs/integration-guidelines.md @@ -0,0 +1,135 @@ +# Integration Guidelines + +Rules and conventions for integrating the external-secrets-operator with OpenShift and Kubernetes subsystems. + +## 1. cert-manager Integration + +### Startup Detection + +The operator probes for `certificates.cert-manager.io/v1` at startup using the discovery API (`isCRDInstalled`). The result is cached for the process lifetime in `optionalResourcesList`. No dynamic re-detection after startup. + +Rules: +- Never add a `Watches()` call for Certificate without guarding with `optionalResourcesList[certificateCRDGKV]`. +- If cert-manager is enabled in ESC spec but the CRD was not detected at startup, return `NewIrrecoverableError`. +- If cert-manager is installed **after** the operator has already started, restart the operator deployment so discovery runs again (the cached negative result does not self-heal). + +### CRD Annotator Controller + +When cert-manager is installed **and** `injectAnnotations: "true"` is set, the `crd-annotator` controller patches `cert-manager.io/inject-ca-from` on CRDs labeled `external-secrets.io/component=controller`. Only registered when `IsCertManagerInstalled()` returns true. + +### Certificate Resources + +- Webhook Certificate secret name is `external-secrets-webhook-cm` (avoids collision with cert-controller secret) +- Bitwarden Certificate only created when plugin enabled +- `IssuerRef` validated at reconcile time via uncached client +- DNSNames rewritten to match the operand namespace + +### Cert-Controller Exclusion + +When cert-manager is enabled, the in-tree cert-controller Deployment and its metrics Service are not created. + +## 2. OpenShift CNO / Trusted CA Bundle + +### Proxy CA Bundle ConfigMap + +When proxy is configured, the operator creates ConfigMap `external-secrets-trusted-ca-bundle` with label `config.openshift.io/inject-trusted-cabundle: "true"`. CNO injects the cluster CA bundle. The operator **never writes Data** — use `patchResourceMetadata` for metadata-only updates. + +### User trustedCABundle + +Rules: +- Mounted only on the core controller as volume `user-ca-bundle` at `/etc/pki/tls/user-certs` +- Sets `SSL_CERT_DIR=/etc/pki/tls/user-certs:/etc/pki/tls/certs:/etc/ssl/certs` +- Skipped when ConfigMap has CNO inject label and proxy is enabled +- PEM validated: must contain CA certificates, no private keys, no trailing non-PEM data +- ConfigMap labeled with `WatchedResourceLabelKey` for change detection + +## 3. OLM Integration + +### RELATED_IMAGE Convention + +Operand images read from env vars at runtime: +- `RELATED_IMAGE_EXTERNAL_SECRETS` — external-secrets image +- `RELATED_IMAGE_BITWARDEN_SDK_SERVER` — bitwarden image + +Set in `config/manager/manager.yaml` and CSV deployment spec. OLM uses the `RELATED_IMAGE_` prefix for disconnected/mirrored registries. Missing env var returns `IrrecoverableError`. + +### Bundle Structure + +```text +bundle/ + manifests/ # CSV, CRDs, console quickstarts, YAML samples + metadata/ # annotations.yaml (package, channel, layout) +``` + +CSV declares `installModes: AllNamespaces` only. + +## 4. Proxy Configuration (Layered Resolution) + +Precedence (first non-empty wins per field): +1. `ExternalSecretsConfig.spec.appConfig.proxy` (ESC) +2. `ExternalSecretsManager.spec.globalConfig.proxy` (ESM) +3. OLM environment variables (`HTTP_PROXY`, `HTTPS_PROXY`, `NO_PROXY`) + +Both uppercase and lowercase env vars set on all containers. URLs validated for scheme, host, port range 1-65535. + +## 5. OpenShift Console Integration + +- `ConsoleQuickStart` in `config/console/` with guided tasks for SecretStore/ExternalSecret creation +- Two `ConsoleYAMLSample` resources for ExternalSecret and Vault SecretStore + +## 6. Metrics / Monitoring Integration + +Operator metrics use OpenShift service-CA for TLS: +- Service annotation `service.beta.openshift.io/serving-cert-secret-name: metrics-serving-cert` +- Mounted at `/etc/metrics-certs` with `--metrics-cert-dir=/etc/metrics-certs --metrics-secure=true` + +Operand metrics Services (port 8080) are plain ClusterIP without TLS. + +## 7. Multi-Architecture Support + +Operator Deployment declares `nodeAffinity` for `amd64`, `arm64`, `ppc64le`, `s390x` on `linux`. When adding arch support, update both `config/manager/manager.yaml` and the CSV deployment spec. + +Operand deployments (from bindata) do not carry node affinity by default; users configure via `spec.appConfig.affinity` or `spec.appConfig.nodeSelector`. + +## 8. Webhook Integration + +Two `ValidatingWebhookConfiguration` resources: `externalsecret-validate` and `secretstore-validate`. When cert-manager `injectAnnotations` is enabled, `cert-manager.io/inject-ca-from` annotation is added. Webhook volume switches from cert-controller secret to `external-secrets-webhook-cm`. + +## 9. Controller-to-Controller Communication (ESM <-> ESC) + +### Data Flow + +- **ESC reads ESM**: fetches ESM for `globalConfig` (labels, proxy, resources, etc.) and `features` as defaults. ESC-level config takes precedence. +- **ESM watches ESC status**: copies ESC conditions into `esm.status.controllerStatuses[]` +- **ESC watches ESM spec**: `GenerationChangedPredicate`, enqueues ESC singleton on ESM spec changes + +### Feature Flag Propagation + +ESM `features` mapped to container args via `featureContainerArgs`. Applied only if feature is enabled **and** the deployment declares support via `updateOptionalFeatures`. + +### Default ESM Creation + +Auto-created at startup by `CreateDefaultESMResource` with standard labels and empty spec. + +## 10. Bindata / Asset Management + +Static manifests in `bindata/external-secrets/`, compiled into `pkg/operator/assets/bindata.go`. + +Rules: +- Decode with typed helpers (`DecodeDeploymentObjBytes`, etc.) +- Always call `updateNamespace(obj, esc)` after decoding +- Always call `ApplyResourceMetadata(obj, resourceMetadata)` for labels/annotations +- Use `createWithFallback` for fully-owned resources; `createWithMetadataFallback` for co-managed +- Network policy prefixes: `eso-sys-` (operator), `eso-user-` (user) + +## 11. Resource Labeling Conventions + +| Label/Annotation | Purpose | +|---|---| +| `app=external-secrets` | Cache filter for managed operand resources | +| `externalsecretsconfig.operator.openshift.io/watching=true` | Watch trigger for user-referenced resources | +| `config.openshift.io/inject-trusted-cabundle=true` | CNO CA bundle injection | +| `external-secrets.io/component=controller` | CRD annotator target | +| `cert-manager.io/inject-ca-from` | cert-manager CA injection | + +Disallowed user labels: `^app.kubernetes.io/`, `^external-secrets.io/`, `^rbac.authorization.k8s.io/`, `^servicebinding.io/controller$`, `^app$` diff --git a/harness-evals/harness-docs/performance-guidelines.md b/harness-evals/harness-docs/performance-guidelines.md new file mode 100644 index 000000000..1378a371a --- /dev/null +++ b/harness-evals/harness-docs/performance-guidelines.md @@ -0,0 +1,100 @@ +# Performance Guidelines + +Conventions specific to the external-secrets-operator codebase for keeping the operator fast and stable at scale. + +## 1. Cache Configuration + +### 1.1 Label-filtered cache (managed resources) + +The manager cache is configured in `NewCacheBuilder()` with a per-type label selector (`app=external-secrets`) for label-filtered operand resources listed in `controllerManagedResources`. ConfigMaps are the exception and use namespace scoping (see below). Only matching objects are stored in the informer cache for label-filtered types. + +Rules: +- Every new operand resource type MUST be added to `controllerManagedResources` with the `app=external-secrets` label applied via `ApplyResourceMetadata` (unless it follows the ConfigMap namespace-scoped pattern). +- Never create an unfiltered cache entry for a type with unbounded cluster-wide cardinality (Secrets, ConfigMaps, ClusterRoles). +- Own CRs (`ExternalSecretsConfig`, `ExternalSecretsManager`) are cached without a label filter. + +### 1.2 Namespace-scoped ConfigMap cache + +ConfigMaps use namespace scoping (`OperandDefaultNamespace`) instead of a label selector because user-provided ConfigMaps only receive the watch label during reconciliation. Do not add a label selector to the ConfigMap cache entry. + +### 1.3 AlreadyExists handling (`createWithFallback`) + +A label-filtered cache can miss objects whose managed label was externally removed. `createWithFallback` handles the resulting `AlreadyExists` by falling back to `UncachedClient.UpdateWithRetry`. For externally-managed data (Secrets, ConfigMaps), use `createWithMetadataFallback` or `patchResourceMetadata` to touch only labels and annotations. + +## 2. Change Detection + +### 2.1 Field-level comparison (`HasObjectChanged`) + +`HasObjectChanged` in `pkg/controller/common/utils.go` compares only operator-managed fields per resource type. When adding a new managed resource type, add a case to the switch with a type-specific comparator. Never compare `metadata.resourceVersion`, `status`, or `metadata.managedFields`. + +### 2.2 Order-insensitive comparisons + +Environment variables and volume mounts are compared order-insensitively using `slicesEqualUnordered` (clone, sort, then `DeepEqual`). Any new slice field whose ordering is not semantically significant must use this pattern. + +### 2.3 Managed-key annotation comparison + +`ObjectMetadataModified` and `annotationMapsModified` compare only operator-managed annotation keys. Never use `reflect.DeepEqual` on annotation maps — always pass through `ObjectMetadataModified` to avoid infinite reconcile loops. + +## 3. Event Predicates and Watch Filtering + +| Resource Type | Predicate | Reason | +|---|---|---| +| ExternalSecretsConfig (primary) | `GenerationChangedPredicate` | Skip status-only updates | +| Deployments | `GenerationChangedPredicate` OR `LabelChangedPredicate` | Filter pod rollout updates, catch label removals | +| Secrets | `WatchesMetadata` + `LabelChangedPredicate` | Avoid caching Secret data | +| ConfigMaps | `ResourceVersionChangedPredicate` | ConfigMaps lack `.metadata.generation` | + +The `mapFunc` in `SetupWithManager` checks `hasManagedOrWatchLabel` before returning a reconcile request — objects without the managed or watch label produce an empty request slice. + +Rule: Always pick the narrowest predicate. Use `GenerationChangedPredicate` for spec-bearing resources, `LabelChangedPredicate` for metadata-only watches, `ResourceVersionChangedPredicate` only when generation is unavailable. + +## 4. Requeue Strategy + +`DefaultRequeueTime` is 30 seconds (defined in `pkg/controller/common/constants.go`). + +| Error Type | Requeue? | Rationale | +|---|---|---| +| `IrrecoverableError` | No | Permanent failure, no point spinning | +| `RetryRequiredError` | 30s | Transient, may self-resolve | +| `UserConfigurationError` (NotFound) | 30s | No watch events for nonexistent objects | +| `UserConfigurationError` (other) | No | Recovery is watch-driven | +| Success | No | Done | + +`FromClientError` in `pkg/controller/common/errors.go` classifies API errors automatically: Unauthorized, Forbidden, Invalid, BadRequest become `IrrecoverableError`; all other client errors (including `NotFound`) become `RetryRequiredError`. Call sites that treat a missing user-referenced object as bad configuration should wrap with `NewUserConfigurationError` instead (see [`error-handling-guidelines.md`](error-handling-guidelines.md) for status/requeue behavior). + +## 5. Concurrency Patterns + +### 5.1 Resettable sync.Once (`common.Now`) + +`Now` uses double-checked locking (`atomic.Uint32` + `sync.Mutex`) to call a function at most once per degraded period. Use `r.now.Do(f)` for events that should fire at most once per error period. Call `r.now.Reset()` when the error condition clears. + +### 5.2 UpdateWithRetry / RetryOnConflict + +`UpdateWithRetry` in `pkg/controller/client/client.go` wraps `retry.RetryOnConflict(retry.DefaultRetry, ...)`, re-fetching the object for the latest `resourceVersion` before each attempt. + +Rules: +- Use `UpdateWithRetry` for all metadata/spec updates on shared objects. +- Prefer `Patch` over `Update` when touching a small number of fields. + +### 5.3 Cache fallback reads + +`getWithCacheFallback` reads from the manager cache first and falls back to `UncachedClient` on `IsNotFound`. Use only for resources that may not yet be in the label-filtered cache. + +## 6. Status Update Efficiency + +### 6.1 Skip unchanged conditions + +Both `reconcileDeploymentSuccessResult` and `updateStatusConditionsOnFailure` check whether conditions actually changed before issuing a status update API call. Always gate on condition-change booleans — unconditional status writes generate unnecessary API traffic. + +### 6.2 CR annotation patching + +`updateCRAnnotationsIfNeeded` uses `MergePatch` for annotation-only updates. Never use full `Update` on the CR just to change an annotation. + +## 7. Anti-Patterns to Avoid + +1. **Full-object DeepEqual for change detection** — use `HasObjectChanged` or `ObjectMetadataModified` +2. **Bare Create without AlreadyExists fallback** — use `createWithFallback` or `createWithMetadataFallback` +3. **Unfiltered cache for cluster-scoped types** — add label or namespace filters +4. **Requeuing irrecoverable errors** — check `IsIrrecoverableError` and return without requeue +5. **Unconditional status writes** — gate on condition-change booleans +6. **Full Update on externally-managed resources** — use metadata-only patches for Secrets and ConfigMaps whose data is owned by other controllers diff --git a/harness-evals/harness-docs/references/ecosystem.md b/harness-evals/harness-docs/references/ecosystem.md new file mode 100644 index 000000000..848ff1f46 --- /dev/null +++ b/harness-evals/harness-docs/references/ecosystem.md @@ -0,0 +1,84 @@ +# Platform Ecosystem References + +This document links to generic OpenShift/Kubernetes patterns in the Platform ecosystem hub. The component inherits these platform-wide patterns and practices. + +## Operator Patterns + +**Location**: [openshift/enhancements/ai-docs/platform/operator-patterns/](https://github.com/openshift/enhancements/tree/master/ai-docs/platform/operator-patterns) + +- **Controller Runtime**: Reconciliation loops, event handling, client patterns +- **Status Conditions**: Available, Progressing, Degraded condition semantics +- **Webhooks**: Validation and mutation patterns +- **Finalizers**: Resource cleanup patterns +- **RBAC**: Service account and permissions + +**Component Usage**: +- Uses controller-runtime v0.23.3 (not library-go) +- Custom error classification: IrrecoverableError, RetryRequiredError, UserConfigurationError +- Standard `metav1.Condition` on ESC; custom lightweight `Condition` on ESM +- Finalizer-protected deletion on ExternalSecretsConfig + +## Testing Practices + +- **Test Pyramid**: Unit > Integration > E2E +- **E2E Framework**: OpenShift E2E test patterns + +**Component Usage**: +- See `ESO_TESTING.md` for component-specific test suites +- Unit tests use counterfeiter fakes for client mocking +- API tests use envtest for CEL validation +- E2E tests use Ginkgo v2 with label-based filtering + +## Security Practices + +- **STRIDE Threat Model**: Threat modeling framework +- **RBAC Guidelines**: Role and ClusterRole design + +**Component Usage**: +- All operand containers enforce restricted security context (drop ALL, read-only root, non-root, seccomp RuntimeDefault) +- Network policies isolate operand namespace with deny-all default +- HTTP/2 disabled on operator for security +- Reserved annotation/env var domains blocked via CEL + +## Reliability Practices + +- **SLO Framework**: Service Level Objectives and error budgets +- **Observability**: Metrics, logging, tracing patterns + +**Component Usage**: +- Prometheus metrics endpoint with OpenShift service CA +- Error classification drives requeue behavior (30s for retryable, none for irrecoverable) +- Event recording for reconciliation outcomes + +## Kubernetes Fundamentals + +**Location**: [openshift/enhancements/ai-docs/domain/kubernetes/](https://github.com/openshift/enhancements/tree/master/ai-docs/domain/kubernetes) + +- **Pod**: Pod lifecycle, container specs +- **CRDs**: CustomResourceDefinition patterns + +**Component Usage**: +- Two cluster-scoped CRDs: ExternalSecretsConfig, ExternalSecretsManager +- Upstream external-secrets CRDs distributed via OLM bundle +- CEL validation rules for singleton enforcement and field immutability + +## OpenShift Integrations + +**Location**: [openshift/enhancements/ai-docs/domain/openshift/](https://github.com/openshift/enhancements/tree/master/ai-docs/domain/openshift) + +**Component Usage**: +- **Trusted CA Bundle**: CNO injects cluster CA via `config.openshift.io/inject-trusted-cabundle` label +- **Proxy**: Falls back to OLM-injected HTTP_PROXY/HTTPS_PROXY/NO_PROXY env vars +- **Console**: QuickStart content for operator setup +- **Multi-arch**: NodeAffinity for amd64, arm64, ppc64le, s390x +- **OLM**: Operator distributed via OLM bundle; uses RELATED_IMAGE_* convention for disconnected support + +## Cross-Repository ADRs + +**Component-Specific ADRs**: See [`../decisions/`](../decisions/) for component-specific decisions. + +--- + +**Note**: These links point to Platform (ecosystem hub) documentation. Component-specific patterns are documented under `harness-evals/harness-docs/` in this repository. + +**Last Updated**: 2026-07-31 diff --git a/harness-evals/harness-docs/references/enhancements.md b/harness-evals/harness-docs/references/enhancements.md new file mode 100644 index 000000000..3b261694f --- /dev/null +++ b/harness-evals/harness-docs/references/enhancements.md @@ -0,0 +1,23 @@ +# Enhancement Proposals & Design Documents + +Catalog of design documentation for the External Secrets Operator. + +## OpenShift Enhancement Proposals + +| Title | Status | Location | Tracking | +|-------|--------|----------|----------| +| External Secrets Operator for Red Hat OpenShift | Implemented | [enhancements/external-secrets-operator/external-secrets-operator.md](https://github.com/openshift/enhancements/blob/master/enhancements/external-secrets-operator/external-secrets-operator.md) | OCPSTRAT-1539, OCPSTRAT-1637, ESO-2, ESO-13, ESO-155 | +| Network Policies for external-secrets Operator and Operands | Implemented | [enhancements/external-secrets-operator/external-secrets-network-policy.md](https://github.com/openshift/enhancements/blob/master/enhancements/external-secrets-operator/external-secrets-network-policy.md) | ESO-165, ESO-70, ESO-418 | +| Component Configuration for external-secrets Operator | Implemented | [enhancements/external-secrets-operator/external-secrets-component-config.md](https://github.com/openshift/enhancements/blob/master/enhancements/external-secrets-operator/external-secrets-component-config.md) | OCPSTRAT-2419, RFE-7842, RFE-8685, ESO-266, ESO-417 | + +## Local Design Documents + +| Title | Location | Description | +|-------|----------|-------------| +| Dual Cache Fix | [docs/anti-patterns/DUAL_CACHE_FIX.md](../../../docs/anti-patterns/DUAL_CACHE_FIX.md) | Anti-pattern documentation for cache issues | + +## Notes + +- Enhancement proposals are the source of truth for feature design decisions +- Enhancement proposals are cross-component feature designs; component-specific ADRs are in [`../decisions/`](../decisions/) +- For the latest proposal status, check the [openshift/enhancements](https://github.com/openshift/enhancements/tree/master/enhancements/external-secrets-operator) repository diff --git a/harness-evals/harness-docs/security-guidelines.md b/harness-evals/harness-docs/security-guidelines.md new file mode 100644 index 000000000..4edfb54d6 --- /dev/null +++ b/harness-evals/harness-docs/security-guidelines.md @@ -0,0 +1,118 @@ +# Security Guidelines + +Security conventions enforced by the external-secrets-operator codebase. These rules govern contributions to the operator controller, CRD types, bindata manifests, and container images. + +## 1. CRD Singleton and CEL Validation + +Both `ExternalSecretsConfig` and `ExternalSecretsManager` are cluster-scoped singletons. The CRD enforces `metadata.name == 'cluster'` via a CEL `XValidation` rule on the type itself (`api/v1alpha1/external_secrets_config_types.go`, `api/v1alpha1/external_secrets_manager_types.go`). + +New API fields must include CEL validation rules for: +- **Immutability**: Fields that cannot change after creation use `rule="self == oldSelf"`. Applied to `certManager.mode`, `issuerRef`, and `injectAnnotations`. Network policy `name` and `componentName` use a list-level CEL rule: `oldSelf.all(op, self.exists(p, p.name == op.name && p.componentName == op.componentName))`. +- **Cross-field dependencies**: The spec-level CEL rule on `ExternalSecretsConfigSpec` ensures that when Bitwarden is enabled, either `secretRef` or `certManager` is configured. +- **Bounded cardinality**: Labels, annotations, tolerations, componentConfigs, networkPolicies, and overrideEnv all enforce `MaxItems`/`MaxProperties` (typically 20 or 50). + +All CEL rules must have corresponding test cases in `api/v1alpha1/tests/` test suite YAML files. + +## 2. Annotation Domain Restrictions + +User-supplied annotations in `controllerConfig.annotations` are blocked from reserved Kubernetes ecosystem domains via CEL rules: +- `kubernetes.io/` and subdomains (`*.kubernetes.io/`) +- `openshift.io/` and subdomains +- `k8s.io/` and subdomains +- `cert-manager.io/` (exact domain only, not subdomains) + +The regex validates key format (alphanumeric start/end, optional DNS prefix), prefix length (max 253), and name part length (max 63). New annotation restrictions must be added as CEL rules on the `annotations` map field, not in controller-side Go code. + +## 3. Label Domain Restrictions + +Labels are restricted at the controller level via `disallowedLabelMatcher` regex in `install_external_secrets.go`: + +```regex +^app.kubernetes.io\/|^external-secrets.io\/|^rbac.authorization.k8s.io\/|^servicebinding.io\/controller$|^app$ +``` + +Matching **user-supplied** labels are silently skipped. The operator still sets `app: external-secrets` (and other defaults) via `controllerDefaultResourceLabels` in `constants.go` so `app`, `app.kubernetes.io/version`, `app.kubernetes.io/managed-by`, and `app.kubernetes.io/part-of` remain operator-controlled. + +## 4. Environment Variable Reservation + +The `overrideEnv` field on `ComponentConfig` uses a CEL rule to reject: +- **Prefix reservations**: Names starting with `KUBERNETES_` or `EXTERNAL_SECRETS_` +- **Exact name reservations**: `HOSTNAME`, `SSL_CERT_DIR`, `SSL_CERT_FILE` + +`SSL_CERT_DIR` is managed by the operator when `trustedCABundle` is configured. Proxy env vars (`HTTP_PROXY`, `HTTPS_PROXY`, `NO_PROXY` and lowercase variants) are managed by the proxy reconciliation logic and must not be added to the reservation list since they are set/removed programmatically. + +## 5. Container Security Context + +Every operand container gets a hardened security context applied in `updateContainerSecurityContext()` (`deployments.go`): + +```go +AllowPrivilegeEscalation: false +Capabilities.Drop: ["ALL"] +ReadOnlyRootFilesystem: true +RunAsNonRoot: true +RunAsUser: nil // defers to the image or PSA +SeccompProfile.Type: RuntimeDefault +``` + +This function is called for every deployment container. The bindata manifests also declare these settings, but the controller overwrites them on every reconcile to prevent drift. New deployments must call `updateContainerSecurityContext()` on each container. + +## 6. Container Image Security + +The Dockerfile runs as UID `65534:65534` (nobody). Operand images are resolved exclusively from `RELATED_IMAGE_*` environment variables (`RELATED_IMAGE_EXTERNAL_SECRETS`, `RELATED_IMAGE_BITWARDEN_SDK_SERVER`), which are set by OLM during installation. The controller treats a missing `RELATED_IMAGE_*` variable as an irrecoverable error. Never hardcode image references in Go code; always read from environment variables following the `RELATED_IMAGE_` convention. + +## 7. HTTP/2 Disabled by Default + +In `cmd/external-secrets-operator/main.go`, HTTP/2 is disabled for both metrics and webhook servers to mitigate known HTTP/2 vulnerabilities. The `--enable-http2` flag defaults to `false` and sets `c.NextProtos = []string{"http/1.1"}` on the TLS config. Do not change this default. + +## 8. Network Policy Architecture + +The operator enforces a deny-all-first network model for the operand namespace. + +**Naming prefixes** (defined in `constants.go`): +- `eso-sys-` — operator-managed static policies from bindata manifests +- `eso-user-` — user-defined policies from `controllerConfig.networkPolicies` + +**Static policies** (always applied): +- `eso-sys-deny-all-traffic` — blanket deny on all pods +- `eso-sys-allow-api-server-egress-for-main-controller` — egress to port 6443 +- `eso-sys-allow-api-server-egress-for-webhook` — egress to 6443, ingress on 10250 and 8080 +- `eso-sys-allow-to-dns` — egress to OpenShift DNS pods on ports 53/5353 + +**Conditional policies**: +- `eso-sys-allow-api-server-egress-for-cert-controller` — only when cert-manager disabled +- `eso-sys-allow-api-server-egress-for-bitwarden-server` — only when Bitwarden enabled +- `eso-sys-allow-proxy-egress` — when proxy configured and `networkPolicyProvisioning` is `Managed` + +**User custom policies** only support `egress` rules and only target `ExternalSecretsCoreController` or `BitwardenSDKServer` components. + +## 9. TLS Certificate Management + +Two mutually exclusive certificate strategies: + +**Built-in cert-controller** (default): Deployed as `external-secrets-cert-controller` when `certManager.mode` is not `Enabled`. TLS secret is `external-secrets-webhook`. + +**cert-manager integration**: When `certManager.mode: Enabled`, the cert-controller is skipped. Webhook TLS secret becomes `external-secrets-webhook-cm` to avoid clash. The `mode`, `issuerRef`, and `injectAnnotations` fields are all immutable after creation. + +## 10. Trusted CA Bundle Validation + +The `trustedCABundle` ConfigMap undergoes strict PEM validation in `trusted_ca_bundle.go`: +- Must contain at least one PEM-encoded X.509 CA certificate +- Private key PEM blocks are rejected (RSA, EC, DSA, ENCRYPTED, generic) +- Leaf certificates (non-CA) are rejected +- Trailing non-PEM data is rejected +- When the ConfigMap carries the CNO inject label and proxy is enabled, the user CA mount is skipped + +## 11. Proxy Configuration Security + +Proxy settings are resolved by layering three sources (highest to lowest priority): ESC spec, ESM globalConfig, OLM environment variables. URL validation requires a valid scheme and host; explicit ports must be in TCP range 1-65535. Both uppercase and lowercase proxy env vars are set on all containers. + +## 12. RBAC Least-Privilege + +The operator's ClusterRole (`config/rbac/role.yaml`) is auto-generated from `+kubebuilder:rbac` markers and requests only needed verbs per resource. Key constraints: +- `serviceaccounts/token` only gets `create` +- Deployments do not get `delete` +- Namespaces do not get `delete` + +## 13. Reconciliation Drift Protection + +The controller uses label-filtered informer caches (`app=external-secrets`) and detects external modifications via `HasObjectChanged()`. `createWithFallback()` handles label-stripped resources by using an uncached client to restore desired state when `Create` returns `AlreadyExists`. diff --git a/harness-evals/harness-docs/testing-guidelines.md b/harness-evals/harness-docs/testing-guidelines.md new file mode 100644 index 000000000..cb88a0d11 --- /dev/null +++ b/harness-evals/harness-docs/testing-guidelines.md @@ -0,0 +1,136 @@ +# Testing Guidelines + +## Make Targets + +| Target | What it runs | Notes | +|---|---|---| +| `make test` | manifests + generate + fmt + vet + `test-apis` + `test-unit` | All non-e2e tests | +| `make test-unit` | Go tests excluding E2E, API, and utility packages | Standard Go tests | +| `make test-apis` | `hack/test-apis.sh` via Ginkgo + envtest | CEL/CRD validation | +| `make test-e2e` | `go test -C test -tags e2e ./e2e` (via Makefile) | Requires live cluster; prefer `make test-e2e` | + +## 1. Unit Tests (pkg/controller/) + +### Counterfeiter Fakes + +Uses counterfeiter for `CtrlClient` interface in `pkg/controller/client/client.go`. Fakes in `pkg/controller/client/fakes/`. Regenerate with: + +```bash +go generate ./pkg/controller/client/... +``` + +Never edit `fakes/fake_ctrl_client.go` by hand. + +### Shared Test Utilities + +- `pkg/controller/commontest/utils.go` — `TestExternalSecretsConfig()`, `TestExternalSecretsManager()`, `ErrTestClient` (test error variable for client failure scenarios). Use these instead of ad-hoc CR fixtures. +- `pkg/controller/external_secrets/test_utils.go` — `testReconciler(t)`, `testDeployment(name string)`, `testResourceMetadata(esc)`, typed helpers for each resource type. + +### Writing a Unit Test + +1. Use **stdlib `testing`**, not Ginkgo. +2. Use **table-driven tests** with `t.Run(tt.name, ...)`. +3. Call `t.Parallel()` on the outer function and each subtest. +4. Create reconciler with `testReconciler(t)`, wire `&fakes.FakeCtrlClient{}`. +5. Use `t.Setenv()` for environment variables, never `os.Setenv`. +6. Assert with `t.Fatalf` / `t.Errorf`, not testify (testify is used only in E2E `test/utils/`). + +### Capturing Created/Updated Objects + +```go +var capturedDeployment *appsv1.Deployment +mock.CreateCalls(func(_ context.Context, obj client.Object, _ ...client.CreateOption) error { + if dep, ok := obj.(*appsv1.Deployment); ok { + capturedDeployment = dep.DeepCopy() + } + return nil +}) +``` + +## 2. API Integration Tests (test/apis/) + +### Data-Driven Test Suites + +CRD validation tests use `.testsuite.yaml` files under `api//tests/./` (e.g., `externalsecretsconfig.operator.openshift.io`). The generator (`test/apis/generator.go`) auto-discovers files and generates Ginkgo `DescribeTable` entries. No Go code changes needed to add test cases. + +### envtest Details + +- CRDs from `config/crd/bases/` +- Each `Describe` installs/uninstalls its CRD per `Ordered` group +- Requires Kube API >= 1.25 for CEL validation +- Run with `make test-apis` (sets `KUBEBUILDER_ASSETS` automatically) + +## 3. E2E Tests (test/e2e/) + +### Build Tag + +All E2E files require `//go:build e2e`. `make test-e2e` passes `-tags e2e`. + +### Ginkgo v2 Label System + +Every `Context`/`Describe` must carry `Label()` decorators: + +| Dimension | Values | +|---|---| +| `Platform:` | `AWS`, `GCP`, `Generic` | +| `Provider:` | `AWS`, `Vault`, `Bitwarden` | +| `Feature:` | `Proxy`, `Upgrade`, `OverrideEnv`, `NetworkPolicy`, `CustomAnnotations`, `CustomLabels`, `TrustedCABundle`, `RevisionHistoryLimit`, `UnsafeAllowGenericTargets` | +| `Skipped:` | `Disconnected` | + +Default filter: `Platform: isSubsetOf {AWS,Generic} && !(Feature: containsAny {Proxy, Upgrade}) && !(Provider: containsAny Bitwarden)` + +Override: `make test-e2e E2E_GINKGO_LABEL_FILTER='Provider:Vault'` + +> **Note:** If/when the e2e suite is restructured, update this label taxonomy and the default `E2E_GINKGO_LABEL_FILTER` to match the new suite layout. + +### Embedded Testdata + +Test manifests in `test/e2e/testdata/` embedded with `//go:embed testdata/*`. Use `testassets.ReadFile` to load. For template substitution use `utils.ReplacePatternInAsset("${PLACEHOLDER}", value)`. + +### Condition Utilities + +- `utils.VerifyPodsReadyByPrefix(ctx, clientset, ns, prefixes)` — poll for pods to reach Ready +- `utils.VerifyOperandPodsReady(ctx, clientset, ns, esc)` — verify all expected operand pods +- `utils.WaitForESOResourceReady(ctx, dynamicClient, gvr, ns, name, timeout)` — poll for `Ready=True` +- `utils.WaitForExternalSecretsConfigReady(ctx, dynamicClient, name, timeout)` — checks both `Ready=True` and `Degraded=False` + +### Artifact Dumping + +`AfterEach` calls `utils.DumpE2EArtifacts(...)` on failure. Artifacts written to `$ARTIFACT_DIR/e2e-artifacts/failure-/` include pod logs, events, and ESO custom resources. + +### Test Structure Conventions + +- Use `Ordered` on blocks with `BeforeAll`/`AfterAll` +- Use `BeforeAll` for expensive setup (credential checks, CR creation) +- Clean up with `defer loader.DeleteFromFile(...)` immediately after creation +- Use `retry.RetryOnConflict` when updating shared CRs +- Use `Eventually(...).Should(Succeed())` with explicit timeout and poll interval +- Use `Consistently(...)` to verify something does NOT happen + +## 4. What to Test vs. What Not to Test + +### Do Test + +- Reconciler logic for each resource type +- Error classification (irrecoverable vs. retryable vs. user-configuration) +- Status condition generation and requeue behavior +- User-configurable fields (affinity, tolerations, env overrides, etc.) +- CRD validation rules (CEL) via `.testsuite.yaml` +- Feature flag propagation + +### Do Not Test + +- Counterfeiter-generated fake code +- Upstream controller-runtime / client-go behavior +- Static asset YAML content (test the Go code that processes it) + +## 5. Naming Conventions + +| Category | Convention | +|---|---| +| Unit test files | `_test.go` in same package | +| Unit test functions | `TestFunctionName` with descriptive subtests | +| Shared test helpers | `test_utils.go`, `commontest/` for cross-package | +| E2E test files | `_test.go` with `//go:build e2e` | +| E2E helpers | `helpers_test.go` for internal, `test/utils/*.go` for shared | +| API test suites | `.testsuite.yaml` under `api//tests/./` (singular CRD kind) | diff --git a/test/e2e/README.md b/test/e2e/README.md index 0524987fa..ab6a72d8f 100644 --- a/test/e2e/README.md +++ b/test/e2e/README.md @@ -41,7 +41,7 @@ make test-e2e E2E_GINKGO_LABEL_FILTER="" `make test-e2e` uses: -``` +```text Platform: isSubsetOf {AWS,Generic} && !(Feature: containsAny {Proxy, Upgrade}) && !(Provider: containsAny Bitwarden) ``` @@ -187,7 +187,7 @@ Files written on every run: When a spec in the main e2e describe fails, a snapshot is also written to: -``` +```text _output/e2e-artifacts/failure-/ ├── pods/ # last 500 log lines and describe YAML per pod (operator, operand, test namespaces) ├── events/ # recent events per namespace From e4a78f64d357b52320a32f631612839426ebb61d Mon Sep 17 00:00:00 2001 From: Bharath B Date: Fri, 7 Aug 2026 20:34:43 +0530 Subject: [PATCH 2/2] OAPE-877: Add markdown linter Signed-off-by: Bharath B --- .markdownlint-cli2.yaml | 35 ++++++++++++++++++++ .markdownlint.yaml | 3 -- Makefile | 62 +++++++++++++++++++++++++++++++++++- hack/Dockerfile.markdownlint | 15 +++++++++ hack/install-markdownlint.sh | 40 +++++++++++++++++++++++ hack/markdownlint.sh | 32 +++++++++++++++++++ 6 files changed, 183 insertions(+), 4 deletions(-) create mode 100644 .markdownlint-cli2.yaml delete mode 100644 .markdownlint.yaml create mode 100644 hack/Dockerfile.markdownlint create mode 100755 hack/install-markdownlint.sh create mode 100755 hack/markdownlint.sh diff --git a/.markdownlint-cli2.yaml b/.markdownlint-cli2.yaml new file mode 100644 index 000000000..b8b1e6e99 --- /dev/null +++ b/.markdownlint-cli2.yaml @@ -0,0 +1,35 @@ +# markdownlint-cli2 config. +# globs: files to lint (allowlist). +# References: +# - https://github.com/DavidAnson/markdownlint-cli2 +# - https://github.com/DavidAnson/markdownlint + +globs: + - "*.md" + - "harness-evals/harness-docs/**/*.md" + - "docs/**/*.md" + - ".github/**/*.md" + - "test/**/*.md" + +# Generated; regenerated by `make docs`. +ignores: + - "docs/api_reference.md" + +config: + # Disabled rules. + MD013: false # line-length + MD034: false # no-bare-urls + MD033: false # no-inline-html + MD010: false # no-hard-tabs + MD036: false # no-emphasis-as-heading + MD009: false # no-trailing-spaces + MD041: false # first-line-heading + MD046: false # code-block-style + MD032: false # blanks-around-lists + MD024: # no-duplicate-heading + siblings_only: true + + # Enabled by default (called out for discoverability): + # MD022 blanks-around-headings + # MD031 blanks-around-fences + # MD040 fenced-code-language diff --git a/.markdownlint.yaml b/.markdownlint.yaml deleted file mode 100644 index 2b5a52380..000000000 --- a/.markdownlint.yaml +++ /dev/null @@ -1,3 +0,0 @@ -MD034: - exclude: - - docs/api_reference.md diff --git a/Makefile b/Makefile index c0f29671f..62426199f 100644 --- a/Makefile +++ b/Makefile @@ -92,6 +92,14 @@ endif # tools. (i.e. podman) CONTAINER_TOOL ?= podman +# Map the invoking host user into a container so bind-mount writes are owned +# correctly (podman: keep-id; docker: numeric --user). +ifeq ($(CONTAINER_TOOL),podman) +CONTAINER_USER_FLAGS ?= --userns=keep-id +else +CONTAINER_USER_FLAGS ?= --user $(shell id -u):$(shell id -g) +endif + # GO_PACKAGE is the Go module path (used for ldflags to embed version info). GO_PACKAGE ?= github.com/openshift/external-secrets-operator @@ -139,6 +147,10 @@ OPERATOR_SDK_VERSION ?= v1.39.0 YQ_VERSION = v4.50.1 HELM_VERSION ?= v3.17.3 +# Image tag produced by markdownlint-image; base image for that Dockerfile. +MARKDOWNLINT_IMAGE ?= external-secrets-operator-markdownlint:latest +MARKDOWNLINT_BASE_IMAGE ?= mirror.gcr.io/library/node@sha256:76789712cd1ae89a1225eac9077010d68987a423588042dac30446f502f1858c + # Include the library makefiles only when vendored (so e.g. `make update-vendor` works on a clean tree). BUILD_MACHINERY_GO_MAKE := $(PROJECT_ROOT)/vendor/github.com/openshift/build-machinery-go/make @@ -242,6 +254,24 @@ lint-fix: $(GOLANGCI_LINT) ## Run golangci-lint linter and perform fixes. @echo "Running go linter with auto-fix..." @$(GOLANGCI_LINT) run --verbose --fix --config .golangci.yml +.PHONY: markdownlint-image +markdownlint-image: ## Build MARKDOWNLINT_IMAGE from hack/Dockerfile.markdownlint. + @echo "Building markdownlint image $(MARKDOWNLINT_IMAGE)..." + @$(CONTAINER_TOOL) build \ + --build-arg MARKDOWNLINT_BASE_IMAGE=$(MARKDOWNLINT_BASE_IMAGE) \ + -f hack/Dockerfile.markdownlint \ + -t $(MARKDOWNLINT_IMAGE) . + +.PHONY: lint-markdown +lint-markdown: ## Run markdownlint-cli2 (config: .markdownlint-cli2.yaml). + @echo "Running markdownlint..." + @$(call run-markdownlint,) + +.PHONY: lint-markdown-fix +lint-markdown-fix: ## Run markdownlint-cli2 --fix. + @echo "Running markdownlint with auto-fix..." + @$(call run-markdownlint,--fix) + ##@ Build .PHONY: build-operator @@ -370,6 +400,36 @@ go build -mod=vendor -o $${bin_path} $${package}; \ } endef +# run-markdownlint $(1) +# $(1): args forwarded to markdownlint-cli2 (e.g. --fix). +# Resolution order: +# 1. markdownlint-cli2 on PATH -> hack/markdownlint.sh +# 2. OPENSHIFT_CI=true and missing binary -> error +# 3. else -> build MARKDOWNLINT_IMAGE and podman/docker run +# --fix uses a writable mount and CONTAINER_USER_FLAGS; lint-only mounts :ro,Z. +define run-markdownlint +if command -v markdownlint-cli2 >/dev/null 2>&1; then \ + ./hack/markdownlint.sh $(1); \ +elif [ "$${OPENSHIFT_CI:-}" = "true" ]; then \ + echo "markdownlint-cli2 not found on PATH (OPENSHIFT_CI=true)." >&2; \ + exit 1; \ +else \ + $(MAKE) markdownlint-image; \ + if [ "$(1)" = "--fix" ]; then \ + $(CONTAINER_TOOL) run --rm \ + $(CONTAINER_USER_FLAGS) \ + -v $(PROJECT_ROOT):/workdir:Z \ + -w /workdir \ + $(MARKDOWNLINT_IMAGE) $(1); \ + else \ + $(CONTAINER_TOOL) run --rm \ + -v $(PROJECT_ROOT):/workdir:ro,Z \ + -w /workdir \ + $(MARKDOWNLINT_IMAGE) $(1); \ + fi; \ +fi +endef + $(OPERATOR_SDK): ## Download operator-sdk locally if necessary. ifeq (,$(wildcard $(OPERATOR_SDK))) ifeq (,$(shell which operator-sdk 2>/dev/null)) @@ -460,7 +520,7 @@ catalog-push: ## Push a catalog image. ##@ Verification .PHONY: verify -verify: vet fmt verify-deps verify-bindata verify-bindata-assets verify-generated govulncheck check-git-diff ## Verify the changes are working as expected. +verify: vet fmt verify-deps verify-bindata verify-bindata-assets verify-generated govulncheck lint-markdown check-git-diff ## Verify the changes are working as expected. .PHONY: check-git-diff check-git-diff: update ## Check for any uncommitted changes including untracked files. diff --git a/hack/Dockerfile.markdownlint b/hack/Dockerfile.markdownlint new file mode 100644 index 000000000..8eb972ea3 --- /dev/null +++ b/hack/Dockerfile.markdownlint @@ -0,0 +1,15 @@ +# Container image for markdownlint-cli2. +# Built by `make markdownlint-image`; entrypoint is hack/markdownlint.sh. +# +# Override base: +# make markdownlint-image MARKDOWNLINT_BASE_IMAGE=mirror.gcr.io/library/node@sha256: +ARG MARKDOWNLINT_BASE_IMAGE=mirror.gcr.io/library/node@sha256:76789712cd1ae89a1225eac9077010d68987a423588042dac30446f502f1858c +FROM ${MARKDOWNLINT_BASE_IMAGE} + +WORKDIR /workdir +COPY hack/install-markdownlint.sh /tmp/install-markdownlint.sh +RUN chmod +x /tmp/install-markdownlint.sh && /tmp/install-markdownlint.sh + +COPY hack/markdownlint.sh /usr/local/bin/markdownlint.sh +RUN chmod +x /usr/local/bin/markdownlint.sh +ENTRYPOINT ["/usr/local/bin/markdownlint.sh"] diff --git a/hack/install-markdownlint.sh b/hack/install-markdownlint.sh new file mode 100755 index 000000000..e8c9ecafa --- /dev/null +++ b/hack/install-markdownlint.sh @@ -0,0 +1,40 @@ +#!/bin/sh +# Install markdownlint-cli2 globally via npm. +# If npm is missing, install Node.js/npm with dnf or yum (requires root). +# +# Invoked by: +# - openshift/release test_binary_build_commands +# - hack/Dockerfile.markdownlint +# +# Override version: MARKDOWNLINT_CLI2_VERSION= ./hack/install-markdownlint.sh +set -eu + +MARKDOWNLINT_CLI2_VERSION="${MARKDOWNLINT_CLI2_VERSION:-0.18.1}" + +install_nodejs() { + if command -v npm >/dev/null 2>&1; then + return 0 + fi + if [ "$(id -u)" -ne 0 ]; then + echo "npm is not installed; re-run as root or install Node.js/npm first." >&2 + exit 1 + fi + if command -v dnf >/dev/null 2>&1; then + dnf -y module reset nodejs || true + # module streams differ by distro; try 22 then 20, then default packages. + dnf -y module enable nodejs:22 || dnf -y module enable nodejs:20 || true + dnf -y install nodejs npm + return 0 + fi + if command -v yum >/dev/null 2>&1; then + yum -y install nodejs npm + return 0 + fi + echo "Unable to install Node.js/npm: no dnf/yum package manager found." >&2 + exit 1 +} + +install_nodejs +npm install -g "markdownlint-cli2@${MARKDOWNLINT_CLI2_VERSION}" +command -v markdownlint-cli2 +npm list -g --depth=0 "markdownlint-cli2@${MARKDOWNLINT_CLI2_VERSION}" diff --git a/hack/markdownlint.sh b/hack/markdownlint.sh new file mode 100755 index 000000000..b673f3c6e --- /dev/null +++ b/hack/markdownlint.sh @@ -0,0 +1,32 @@ +#!/bin/sh +# Wrapper around markdownlint-cli2. +# Reads .markdownlint-cli2.yaml from the current working directory. +# Forwards all CLI args (e.g. --fix). +set -eu + +handle_exit() { + if [ "$1" != "0" ]; then + cat <<'EOF' + +Markdown lint failed. From the repository root, run: + + make lint-markdown + +To auto-fix many style issues: + + make lint-markdown-fix + +EOF + fi +} + +trap 'handle_exit $?' EXIT + +if ! command -v markdownlint-cli2 >/dev/null 2>&1; then + echo "markdownlint-cli2 not found on PATH." >&2 + echo "Install: ./hack/install-markdownlint.sh" >&2 + echo "Or: make markdownlint-image && make lint-markdown" >&2 + exit 1 +fi + +markdownlint-cli2 "$@"