diff --git a/.devcontainer/Dockerfile b/.devcontainer/Dockerfile index cbc8b671..a8491d31 100644 --- a/.devcontainer/Dockerfile +++ b/.devcontainer/Dockerfile @@ -5,6 +5,11 @@ FROM golang:1.26.4-bookworm AS ci # Avoid warnings by switching to noninteractive ENV DEBIAN_FRONTEND=noninteractive +# Run RUN steps under bash with pipefail so a failure anywhere in a +# `curl ... | tar` download pipeline aborts the build instead of silently +# installing a truncated or empty binary. +SHELL ["/bin/bash", "-o", "pipefail", "-c"] + # Install essential packages + Docker CLI (for DooD via socket mount in CI) RUN apt-get update \ && apt-get -y install --no-install-recommends \ @@ -48,27 +53,24 @@ RUN apt-get update \ # FLUX_VERSION -> https://github.com/fluxcd/flux2 # FLUX_OPERATOR_VERSION -> https://github.com/controlplaneio-fluxcd/flux-operator # TASK_VERSION -> https://github.com/go-task/task/releases -# TILT_VERSION -> https://github.com/tilt-dev/tilt/releases # ACTIONLINT_VERSION -> https://github.com/rhysd/actionlint/releases +# HADOLINT_VERSION -> https://github.com/hadolint/hadolint/releases # VALKEY_VERSION -> https://github.com/valkey-io/valkey/releases ENV PATH="/go/bin:/usr/local/go/bin:${PATH}" \ - KUBECTL_VERSION=v1.36.1 \ + KUBECTL_VERSION=v1.36.2 \ KUSTOMIZE_VERSION=5.8.1 \ - KUBEBUILDER_VERSION=4.14.1 \ + KUBEBUILDER_VERSION=4.15.0 \ GOLANGCI_LINT_VERSION=v2.12.2 \ HELM_VERSION=v4.2.0 \ K3D_VERSION=v5.9.0 \ - FLUX_VERSION=2.8.8 \ - FLUX_OPERATOR_VERSION=0.50.0 \ + FLUX_VERSION=2.9.0 \ + FLUX_OPERATOR_VERSION=0.53.0 \ TASK_VERSION=v3.51.1 \ - TILT_VERSION=v0.37.4 \ ACTIONLINT_VERSION=1.7.12 \ + HADOLINT_VERSION=2.14.0 \ VALKEY_VERSION=9.1.0 -# https://github.com/fluxcd/flux2/releases -# https://fluxoperator.dev/ - # Fail early on unsupported architectures instead of producing a partial image. RUN test "$(dpkg --print-architecture)" = "amd64" \ || (echo "This devcontainer currently supports amd64 only." && exit 1) @@ -135,6 +137,17 @@ RUN asset="actionlint_${ACTIONLINT_VERSION}_linux_amd64.tar.gz" \ && install -m 0755 actionlint /usr/local/bin/actionlint \ && rm -rf "${tmpdir}" +# Install hadolint (static linter for Dockerfiles) +RUN asset="hadolint-linux-x86_64" \ + && base="https://github.com/hadolint/hadolint/releases/download/v${HADOLINT_VERSION}" \ + && tmpdir="$(mktemp -d)" \ + && curl -fsSL "${base}/${asset}" -o "${tmpdir}/${asset}" \ + && curl -fsSL "${base}/${asset}.sha256" -o "${tmpdir}/${asset}.sha256" \ + && cd "${tmpdir}" \ + && sha256sum -c "${asset}.sha256" \ + && install -m 0755 "${asset}" /usr/local/bin/hadolint \ + && rm -rf "${tmpdir}" + # Install valkey-cli # Valkey only ships prebuilt binaries for Ubuntu jammy/noble; the jammy build # (glibc 2.35) is compatible with this bookworm image (glibc 2.36). Extract only @@ -170,13 +183,16 @@ RUN go install sigs.k8s.io/controller-tools/cmd/controller-gen@v0.19.0 \ # This downloads linter dependencies without needing source code RUN mkdir -p /tmp/golangci-init && cd /tmp/golangci-init \ && go mod init example.com/init \ - && echo 'package main\n\nfunc main() {}' > main.go \ + && printf 'package main\n\nfunc main() {}\n' > main.go \ && golangci-lint run --timeout=5m || true \ && cd / && rm -rf /tmp/golangci-init # Pre-download Go modules for caching - placed AFTER tool installation # This layer will be cached and only rebuilt when go.mod/go.sum changes # Moving this down prevents tool reinstallation when dependencies change +# The single-quoted lines below are shell written verbatim into the profile +# script; build-time expansion must NOT happen, so single quotes are intended. +# hadolint ignore=SC2016 RUN printf '%s\n' \ '# Silently load the optional repo-root .env into login shells.' \ 'workspace_dir="${PROJECT_PATH:-}"' \ @@ -197,7 +213,7 @@ ENV DEBIAN_FRONTEND=dialog # Default command CMD ["/bin/bash"] -# Stage 2: Development container with Kind and debugging tools +# Stage 2: Development container, used for local development (https://github.com/devcontainers/spec) FROM ci AS dev USER root @@ -205,6 +221,25 @@ USER root # Switch to noninteractive for package installation ENV DEBIAN_FRONTEND=noninteractive +# Same pipefail hardening as the ci stage (SHELL is not inherited across stages). +SHELL ["/bin/bash", "-o", "pipefail", "-c"] + +# Dev-only tool versions - centralized for easy updates. +# These tools are only used inside the devcontainer (local development, IDE +# tooling, debugging), never in CI, so they live in the dev stage. +# +# Finding the latest versions: +# NODE_MAJOR -> https://github.com/nodejs/node/releases (track a current LTS line) +# TILT_VERSION -> https://github.com/tilt-dev/tilt/releases +# DLV_VERSION -> https://github.com/go-delve/delve/releases +# GOPLS_VERSION -> https://github.com/golang/tools/releases +# STATICCHECK_VERSION -> https://github.com/dominikh/go-tools/releases +ENV NODE_MAJOR=22 \ + TILT_VERSION=v0.37.4 \ + DLV_VERSION=v1.27.0 \ + GOPLS_VERSION=v0.22.0 \ + STATICCHECK_VERSION=v0.7.0 + # Kind is already installed in the ci stage above. RUN apt-get update \ && apt-get -y install --no-install-recommends bash-completion \ @@ -213,6 +248,8 @@ RUN apt-get update \ && rm -rf /var/lib/apt/lists/* # Enable bash completion and auto-load Task completions for interactive shells in the dev image only. +# Single-quoted lines are written verbatim into bash.bashrc; no build-time expansion intended. +# hadolint ignore=SC2016 RUN printf '%s\n' \ '' \ '# Enable bash completion and Task completions in interactive shells.' \ @@ -229,12 +266,10 @@ RUN printf '%s\n' \ >> /etc/bash.bashrc # Install Node.js (provides npm + npx, e.g. for installing Claude Code skills). -# Dev stage only; CI does not need a JS runtime. -# NODE_MAJOR -> https://github.com/nodejs/node/releases (track a current LTS line) RUN curl -fsSL https://deb.nodesource.com/gpgkey/nodesource-repo.gpg.key \ | gpg --dearmor -o /etc/apt/keyrings/nodesource.gpg \ && chmod a+r /etc/apt/keyrings/nodesource.gpg \ - && echo "deb [arch=$(dpkg --print-architecture) signed-by=/etc/apt/keyrings/nodesource.gpg] https://deb.nodesource.com/node_22.x nodistro main" \ + && echo "deb [arch=$(dpkg --print-architecture) signed-by=/etc/apt/keyrings/nodesource.gpg] https://deb.nodesource.com/node_${NODE_MAJOR}.x nodistro main" \ > /etc/apt/sources.list.d/nodesource.list \ && apt-get update \ && apt-get -y install --no-install-recommends nodejs \ @@ -259,11 +294,15 @@ RUN arch="$(dpkg --print-architecture)" && \ # Install Delve debugger for Go debugging in VSCode # ACLs from CI stage should be preserved, so no need to re-apply -RUN go install github.com/go-delve/delve/cmd/dlv@v1.26.1 - -# Install VSCode Go extension tools (gopls and staticcheck) -RUN go install golang.org/x/tools/gopls@v0.21.1 \ - && go install honnef.co/go/tools/cmd/staticcheck@v0.7.0 +RUN go install github.com/go-delve/delve/cmd/dlv@${DLV_VERSION} + +# Install VSCode Go extension tools (gopls and staticcheck). Both are editor-only: +# gopls is the language server, and this standalone staticcheck powers the editor's +# live (as-you-type) diagnostics. Neither is a lint step -- `task lint` already runs +# staticcheck's analyzers via golangci-lint's bundled staticcheck linter (configured in +# .golangci.yml), so this binary is for the IDE, not a second lint pass. +RUN go install golang.org/x/tools/gopls@${GOPLS_VERSION} \ + && go install honnef.co/go/tools/cmd/staticcheck@${STATICCHECK_VERSION} # Create vscode user for non-root development and add to godev group RUN groupadd --gid 1000 vscode \ diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e8c9e393..4a0f87bf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -135,7 +135,7 @@ jobs: " lint-helm: - name: Lint and build Helm Chart (and generate single-file installer) + name: Build Helm chart and installer bundle runs-on: ubuntu-latest needs: build-ci-container container: @@ -153,9 +153,6 @@ jobs: - name: Copy generated things from /config run: task helm-sync - - name: Helm lint - run: helm lint charts/gitops-reverser - - name: Helm template (dry-run) run: | helm template gitops-reverser charts/gitops-reverser \ @@ -181,7 +178,7 @@ jobs: if-no-files-found: error lint: - name: Lint Go Code + name: Lint runs-on: ubuntu-latest needs: build-ci-container container: @@ -196,19 +193,48 @@ jobs: - name: Configure Git safe directory (for now needed as workarround https://github.com/actions/checkout/issues/2031) run: git config --global --add safe.directory /__w/gitops-reverser/gitops-reverser - - name: golangci-lint - uses: golangci/golangci-lint-action@v9 - with: - install-mode: none - skip-cache: false - skip-save-cache: false - only-new-issues: ${{ github.event_name == 'pull_request' }} - args: --timeout=5m --concurrency=4 + # Runs golangci-lint, hadolint, actionlint, and helm lint (see `task lint`). + # No PR annotations (unlike golangci-lint-action) -- run `task lint` locally. + - name: task lint + run: task lint + + - name: task lint cache check + # Container jobs default to `sh`, which rejects `set -o pipefail` below + # ("Illegal option -o pipefail"). Force bash for this bashism-using step. + shell: bash + run: | + set -euo pipefail + cache_log="$(mktemp)" + task --dry --verbose lint 2>&1 | tee "${cache_log}" + + for task_name in \ + generate \ + manifests \ + helm-sync \ + lint-golang \ + lint-dockerfiles \ + lint-actions \ + lint-helm + do + if ! grep -Fq "Task \"${task_name}\" is up to date" "${cache_log}"; then + echo "Expected ${task_name} to be cached after task lint" >&2 + exit 1 + fi + done + + if grep -Eq '^task: \[[^]]+\]' "${cache_log}"; then + echo "Expected second task lint dry-run to be fully cached, but a command would run" >&2 + exit 1 + fi test: name: Unit tests runs-on: ubuntu-latest needs: build-ci-container + permissions: + contents: read # checkout + packages: read # pull the CI base container from GHCR + id-token: write # OIDC token for tokenless Codecov uploads (public repo) container: image: ${{ needs.build-ci-container.outputs.image }} credentials: @@ -229,7 +255,7 @@ jobs: with: files: cover.out flags: unit - token: ${{ secrets.CODECOV_TOKEN }} + use_oidc: true fail_ci_if_error: false docker-build: @@ -271,6 +297,10 @@ jobs: name: E2E (${{ matrix.name }}) runs-on: ubuntu-latest needs: [build-ci-container, docker-build, lint-helm] + permissions: + contents: read # checkout + packages: read # pull the CI container + project image from GHCR + id-token: write # OIDC token for tokenless Codecov uploads (public repo) strategy: matrix: include: @@ -404,7 +434,7 @@ jobs: with: files: e2e-cover.out flags: e2e - token: ${{ secrets.CODECOV_TOKEN }} + use_oidc: true fail_ci_if_error: false - name: Report disk usage (peak + final) diff --git a/.golangci.yml b/.golangci.yml index 8c1764a8..fb09cd9f 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -390,11 +390,3 @@ linters: # Relax godot for test helpers and utility functions - path: '(test/|helpers\.go)' linters: [godot, godoclint] - # Allow utils package name (standard pattern) - - text: "var-naming: avoid meaningless package names" - path: 'test/utils/.*\.go' - linters: [revive] - # Allow types package name (common Go pattern for shared types) - - text: "var-naming: avoid meaningless package names" - path: 'internal/types/.*\.go' - linters: [revive] diff --git a/.hadolint.yaml b/.hadolint.yaml new file mode 100644 index 00000000..832d4dfd --- /dev/null +++ b/.hadolint.yaml @@ -0,0 +1,20 @@ +# hadolint configuration - https://github.com/hadolint/hadolint#configure +# +# Rules disabled below are deliberate patterns in this repo's tooling/build +# Dockerfiles (.devcontainer/Dockerfile and ./Dockerfile), not defects. Every +# other check stays active. Lint locally with `task lint-dockerfiles`. +ignored: + # Distro package versions are intentionally unpinned in tooling/build images; + # we pin the tool versions we care about via ENV blocks instead. + - DL3008 # apt-get install without a pinned version + - DL3018 # apk add without a pinned version + # Installers `cd` into an ephemeral `mktemp -d` and remove it in the same + # RUN; WORKDIR would persist a path that no longer exists. + - DL3003 # use WORKDIR instead of `cd` + +# SC2016 (single-quoted lines that intentionally aren't expanded) is suppressed +# inline with `# hadolint ignore=SC2016` at the two profile-script RUN blocks. + +# Fail the build on warnings and errors, but let info/style suggestions through +# so they surface without blocking `task lint`. +failure-threshold: warning diff --git a/AGENTS.md b/AGENTS.md index ef54828c..9d0c87e6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,9 +24,11 @@ task test # Must pass all unit tests + the coverage ratchet (see TESTING RE task test-e2e # Must pass end-to-end tests ``` -If you change a GitHub Actions workflow, also run `task lint-actions`, which lints -`.github/workflows/ci.yml` with `actionlint`. Both `actionlint` and `golangci-lint` -ship in the devcontainer image. +`task lint` also runs `actionlint` on every workflow under `.github/workflows/` (via the +`lint-actions` task) and `hadolint` on the Dockerfiles (via `lint-dockerfiles`), so a +workflow or Dockerfile change is covered by the normal lint gate; you can also run +`task lint-actions` or `task lint-dockerfiles` directly. `actionlint`, `hadolint`, and +`golangci-lint` all ship in the devcontainer image. ## PRE-IMPLEMENTATION BEHAVIOR diff --git a/Taskfile-build.yml b/Taskfile-build.yml index a4cd4b5e..3a654991 100644 --- a/Taskfile-build.yml +++ b/Taskfile-build.yml @@ -20,7 +20,7 @@ vars: tasks: manifests: - desc: Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects + desc: Generate ClusterRole and CustomResourceDefinition objects deps: - generate sources: @@ -36,11 +36,10 @@ tasks: generates: - config/crd/bases/configbutler.ai*.yaml - config/rbac/role.yaml - - config/webhook/manifests.yaml cmds: - | set -e - mkdir -p config/crd/bases config/rbac config/webhook + mkdir -p config/crd/bases config/rbac # Write CRDs straight into the bases dir, overwriting in place. The # marker predates the run, so afterwards any base controller-gen did # not (re)write — i.e. a renamed/removed type — is older than it and @@ -51,11 +50,9 @@ tasks: {{.CONTROLLER_GEN}} \ rbac:roleName=gitops-reverser \ crd \ - webhook \ paths=./api/... \ paths=./internal/controller/... \ paths=./internal/watch/... \ - paths=./internal/webhook/... \ paths=./cmd/... \ output:crd:artifacts:config=config/crd/bases find config/crd/bases -maxdepth 1 -name '*.yaml' ! -newer "$marker" -delete @@ -161,14 +158,71 @@ tasks: fi lint: + desc: Run all linters (Go, Dockerfiles, workflows, Helm chart); deps run in parallel where safe + deps: + - lint-golang + - lint-dockerfiles + - lint-actions + - lint-helm + + lint-golang: desc: Run golangci-lint + deps: + # The Helm lint branch also reaches manifests -> generate via helm-sync. + # Wait for generated Go/CRD files to settle before golangci-lint scans api/**/*.go. + - manifests + # golangci-lint's real inputs: the module's Go sources (including tests, which it + # lints), the lint config, and the dependency graph. The external-sources/* trees + # are separate modules and are not linted, so they are deliberately not listed. + sources: + - api/**/*.go + - cmd/**/*.go + - internal/**/*.go + - test/**/*.go + - .golangci.yml + - go.mod + - go.sum cmds: - '{{.GOLANGCI_LINT}} run' lint-actions: desc: Lint GitHub Actions workflows with actionlint + # Re-lint whenever any workflow changes (glob covers new pipelines too). + sources: + - .github/workflows/*.yml + - .github/workflows/*.yaml + # No path arg: actionlint auto-discovers every workflow under + # .github/workflows/, so newly added pipelines are checked automatically. + cmds: + - actionlint + + lint-dockerfiles: + desc: Lint the project Dockerfiles with hadolint + # Only re-lint when a Dockerfile or the hadolint config changes; Task + # fingerprints these (checksum) under .task/ and skips the run otherwise. + sources: + - .devcontainer/Dockerfile + - Dockerfile + - .hadolint.yaml + cmds: + - hadolint .devcontainer/Dockerfile Dockerfile + + lint-helm: + desc: Lint the Helm chart with helm lint + # Depends on helm-sync so the chart's generated CRDs/role are present and current + # before linting -- a complete check that also works on a fresh checkout (e.g. CI), + # where those files are gitignored and absent. This pulls in manifests/controller-gen, + # which re-runs on every invocation, so `task lint` is not a strict no-op; that is an + # accepted trade for a complete check. The generated CRDs are gitignored, so the + # resync never produces a stray git diff. Fingerprint every chart YAML/template so an + # edit to a template *or* a regenerated CRD re-triggers the lint. + deps: + - helm-sync + sources: + - charts/gitops-reverser/**/*.yaml + - charts/gitops-reverser/**/*.tpl cmds: - - actionlint .github/workflows/ci.yml + - helm lint charts/gitops-reverser lint-fix: desc: Run golangci-lint and perform fixes diff --git a/docs/design/audit-ingestion-decision-record.md b/docs/design/audit-ingestion-decision-record.md deleted file mode 100644 index 8bc8f40d..00000000 --- a/docs/design/audit-ingestion-decision-record.md +++ /dev/null @@ -1,153 +0,0 @@ -# Audit Ingestion Decision Record - -This file replaces the older "current state" snapshot for the audit pipeline. - -The durable conclusion is simple: - -- the Kubernetes audit webhook is the only reliable source for author-attributed live mutations -- watch-based reconciliation is still useful, but it is not a complete substitute for audit -- a simpler watch-only mode is still possible in the future, but it should be framed as a reduced - capability mode that does not know the real actor - -## Why this matters - -GitOps Reverser is trying to turn live Kubernetes activity into Git history that is both: - -- operationally correct -- attributable to the real user when possible - -Those goals are stricter than "notice that something changed eventually." - -## Approaches we tried - -### 1. Watch-only live routing - -This is the simplest model mechanically: - -- watch the resulting object state -- sanitize it -- write it to Git - -That can work for a simpler product mode, but it has two hard limits: - -- the watch path does not reliably know the real request user -- the watch path only sees resulting object state, not the exact write intent or request context - -So watch-only is acceptable only if we are willing to give up real author attribution and accept a -bot-style committer identity. - -### 2. Request-time webhook or correlation-based enrichment - -We also tried preserving request-time identity through separate webhook-style enrichment and then -joining that information back to the later observed object state. - -This looks attractive at first, but it is fundamentally awkward: - -- request-time signals happen before you can be sure the final object state is what will persist -- retries, updates, defaulting, later mutations, and failed writes make the join fuzzy -- syncing "who asked for a change" with "what actually ended up stored" is much harder than it - sounds - -In practice this became a correlation problem with edge cases at every seam. It was hard to make -reliable and even harder to make obviously correct. - -That is the key lesson: request-time webhook data is not a trustworthy standalone source for final -Git history. - -### 3. Mixed watch and audit live sources - -We also had periods where both watch and audit could route the same logical live mutation. - -That created exactly the kind of race you would expect: - -- watch saw the object and wrote first -- audit arrived later with the real user attribution -- the later audit event often became a no-op because the Git content already matched - -That meant the lower-fidelity source could suppress the higher-fidelity source. The result was -wrong authorship, duplicate-source complexity, and a lot of fragile source-precedence logic. - -## Why audit webhook is the authoritative live path - -The audit webhook is the best fit for the high-fidelity mode because it gives us: - -- the real Kubernetes user information -- a single source of truth for live mutating activity -- a path that is conceptually tied to actual API operations, not only later observed state - -It is still not magic, but it is the least-wrong authority for "who changed what" in live mode. - -That is why the accepted architecture is: - -- audit for live mutation authority -- watch for snapshot and reconcile behavior - -Not: - -- watch and audit competing for the same live write path -- webhook/correlation data trying to patch over a watch-only model - -## Why watch is still needed - -Watch-based logic still has a clear role: - -- initial snapshot -- rule changes -- discovery changes -- retry loops for unavailable GVRs -- any future simplified mode that intentionally gives up author attribution - -So the conclusion is not "watch is bad." - -The conclusion is: - -- watch is good for state reconciliation -- watch is not enough for high-fidelity author-attributed live history - -## The viable simpler mode - -A simpler mode without kube-apiserver audit integration is still a reasonable future idea. - -That mode would be: - -- watch/reconcile based -- simpler to install -- able to write useful Git state -- unable to reliably name the real end-user who made each change - -That trade-off can be acceptable, but it should be described honestly. It is not equivalent to the -audit-backed mode. - -## Remaining architectural debt - -Two durable concerns remain even with audit authority: - -### 1. Queue payload sensitivity - -The current audit queue persists raw `payload_json`, which can include Secret material before Git-side -SOPS encryption happens. - -That is why the queue must be treated as a security boundary and why payload minimization or -redaction is still worth doing. - -### 2. Audit and watch are still separate systems - -The design is intentionally split: - -- audit handles live mutations -- watch handles reconciliation and snapshots - -That split is correct, but bugs tend to appear at the seams: - -- deduplication -- stale state -- source precedence -- rule-change timing - -## Bottom line - -If we want correct end-user attribution for live changes, the audit webhook is the only reliable -mechanism we have found. - -If we want a simpler installation path, we can still add a watch-based mode later, but it should be -treated as a reduced-fidelity mode rather than as an equivalent replacement. diff --git a/docs/design/best-practices-webhook-ingress.md b/docs/design/best-practices-webhook-ingress.md deleted file mode 100644 index de387415..00000000 --- a/docs/design/best-practices-webhook-ingress.md +++ /dev/null @@ -1,213 +0,0 @@ -1) Mutating webhook from a Kubernetes Service: minimal settings to support -A. Listener + routing - -listenAddress / port (default 8443) - -path (e.g. /mutate), and optionally multiple paths if you’ll have multiple webhooks - -readTimeout / writeTimeout / idleTimeout - -maxRequestBodyBytes (defensive; AdmissionReview can be big with certain objects) - -B. TLS (this is non-negotiable in real clusters) - -Kubernetes expects HTTPS for webhooks (service or URL). Minimally support: - -Provide TLS cert + key - -Either via: tls.secretName (mounted secret) - -Or direct file paths (less “Kubernetes-y”, but useful for dev) - -Provide CA bundle for the webhook configuration - -In practice you’ll set caBundle on the MutatingWebhookConfiguration (or let cert-manager inject it) - -Best practice: integrate with cert-manager and expose: - -certManager.enabled (bool) - -certManager.issuerRef (name/kind/group) - -dnsNames (at least service.namespace.svc and service.namespace.svc.cluster.local) - -rotation: rely on cert-manager renewal; your pod must reload certs (or restart on secret change) - -C. Webhook registration (what you control via config/helm values) - -Even if you generate the MutatingWebhookConfiguration from code/helm, you want these as configurable knobs: - -Per webhook: - -failurePolicy: Fail vs Ignore - -Default recommendation: Fail for security/consistency webhooks; Ignore only if mutation is “nice to have” - -timeoutSeconds: keep low (1–5s). Default 2–3s. - -sideEffects: usually None (and mean it) - -admissionReviewVersions: support v1 (and accept v1beta1 only if you must) - -matchPolicy: typically Equivalent - -reinvocationPolicy: consider IfNeeded if you mutate fields other mutators might touch - -Selectors - -namespaceSelector (exclude system namespaces by default) - -objectSelector (optional but great for opt-in via label) - -Rules - -resources + operations you mutate (keep tight) - -scope: cluster vs namespaced where relevant - -D. Runtime safety knobs - -Expose: - -concurrency (max in-flight) - -rateLimit (optional but helpful under thundering herd) - -metrics (Prometheus) + request duration histogram - -pprof optional (dev only) - -logLevel with request IDs and admission UID - -E. Leader election (only if you have shared mutable state) - -For pure stateless mutation, you can run multiple replicas with no leader election. -If you rely on a single writer (e.g., CRD-backed shared cache warmup, or you do coordinated external writes), support: - -leaderElection.enabled - -lease namespace/name - -2) Mutating webhook best practices (the stuff that prevents outages) - -Correctness & determinism - -Make patches deterministic (same input → same output). - -Be idempotent (if called twice, you don’t double-apply). - -Respect dryRun (don’t create external side effects). - -Don’t depend on “live GET” calls in the hot path unless cached; API calls add latency and can deadlock during API stress. - -Performance - -Keep p99 latency low; webhooks are on the API request path. - -Prefer fast local validation/mutation + cached lookups. - -Set tight timeoutSeconds and tune server timeouts accordingly. - -Safety - -Default namespaceSelector to exclude kube-system, kube-public, kube-node-lease, and your own operator namespace until you explicitly need them. - -Use objectSelector to allow opt-in (label) for risky mutations. - -Use failurePolicy=Fail only when you’re confident in HA + readiness + rollout strategy. - -Rollout strategy - -Run at least 2 replicas (or more, depending on API QPS). - -Use a PodDisruptionBudget. - -Ensure readinessProbe only goes ready when: - -certs are loaded - -any required caches are warm (if you depend on them) - -Prefer “versioned” webhook names/paths when doing breaking changes. - -Observability - -Log: admission UID, kind, namespace/name, userInfo, decision, latency - -Metrics: requests, rejections, patch size, errors, timeouts - -3) Should you “support the same settings” for audit webhook handling? - -Some overlap, yes (TLS/HA/observability), but don’t treat them as the same product surface. Audit has very different operational requirements. - -What overlaps (you should support in both) - -HTTPS listener, cert management, rotation - -AuthN (ideally mTLS) and authorization/allowlisting - -Timeouts + max body size - -Concurrency limits and metrics - -What’s different (audit needs extra settings) - -Audit webhook backends can get a lot of traffic and the API server will retry under some failure modes, but you still need to assume: - -bursts - -duplicates - -out-of-order delivery - -occasional loss depending on audit config and backpressure - -So minimally for audit ingestion, add: - -queue.enabled + queue.size - -batching (optional, but very useful downstream) - -durability choice: - -memory queue (simple, lossy on restart) - -persistent queue (disk/DB/Kafka/etc.) - -Backpressure behavior - -what happens when full: drop / block / shed by priority - -Deduplication keying (best-effort): use audit event IDs if present - -Separate endpoint / separate Deployment strongly recommended - -Auth for audit - -For the audit webhook backend, the API server can be configured with a kubeconfig to talk to your endpoint, which makes mTLS client cert auth a clean approach. If you already have a public wildcard cert, that helps with server identity, but client auth is what prevents random in-cluster callers from spamming your audit ingest. - -Recommendation: - -Admission webhook: rely on in-cluster service + TLS + CA bundle (standard) - -Audit webhook: mTLS (client certs) and strict allowlisting/rate limits - -Practical recommendation on architecture - -Keep admission and audit as separate handlers, ideally separate deployments. - -Admission: optimized for latency + correctness - -Audit: optimized for throughput + buffering + durability - -Share libraries (TLS, metrics, logging), but do not share the same scaling knobs or failure modes. - -If you want a simple “minimal config surface” that still scales, expose two top-level blocks: - -admissionWebhooks: (tls, selectors, failurePolicy, timeouts, concurrency) - -auditIngest: (tls, authn, queue/durability, backpressure, concurrency) - -That’s the line where you stay sane when traffic grows. - -If you want, paste your current helm values / flags structure and I’ll suggest a clean config schema (what should be values vs generated defaults) without blowing up the number of knobs. \ No newline at end of file diff --git a/docs/task-migration-plan.md b/docs/task-migration-plan.md deleted file mode 100644 index 0470c85e..00000000 --- a/docs/task-migration-plan.md +++ /dev/null @@ -1,161 +0,0 @@ -# Why We Switched From Make To Task - -This file used to be the migration plan. The migration is done now, so the useful question is no -longer "how should we move?" but "why was the move worth it?" - -Short version: switching from `Makefile` to Task was a very good choice for this repository. - -## The main win - -The old `Makefile` had grown into three things at once: - -- developer commands -- e2e orchestration -- incremental state management through `.stamps` - -Make can do all of that, but the price was high. The file mixed shell, dependency graph tricks, -dynamic variables, grouped outputs, and recursive `$(MAKE)` calls in one place. That gave us a lot -of freedom, but it also made the execution model harder to read and safer changes harder to make. - -Task gave us a better split: - -- Taskfiles own orchestration and task discovery -- `.stamps` still own the explicit runtime state we actually care about -- helper scripts still do the detailed cluster and install work - -That separation fits this repo much better. - -## What got better - -### 1. The command surface became clearer - -The current root [Taskfile.yml](/workspaces/gitops-reverser/Taskfile.yml) is intentionally small: - -- it includes build tasks -- it includes e2e tasks -- it exposes one flat command surface - -That is easier to reason about than one large Makefile trying to be both user interface and -execution engine. - -### 2. Build tasks became more explicit - -The current [Taskfile-build.yml](/workspaces/gitops-reverser/Taskfile-build.yml) makes the important -parts visible: - -- `desc` -- `sources` -- `generates` -- `deps` -- `cmds` - -For example, `manifests` now reads like a declaration of intent: what changes trigger it, what it -produces, and what it runs. - -In the old [Makefile.oldway](/workspaces/gitops-reverser/Makefile.oldway), the same area depended on -Make-specific constructs like grouped targets: - -```make -manifests: $(MANIFEST_OUTPUTS) -$(MANIFEST_OUTPUTS) &: $$(MANIFEST_INPUTS) -``` - -That is powerful, but it asks every future maintainer to keep a fairly large chunk of GNU Make in -their head before they can safely edit a routine build step. - -### 3. E2E orchestration became much easier to read - -The e2e flow is where Task helped most. - -In the current [test/e2e/Taskfile.yml](/workspaces/gitops-reverser/test/e2e/Taskfile.yml), -`prepare-e2e` is spelled out step by step: - -```yaml -prepare-e2e: - cmds: - - task: install - - task: _project-image-ready - - task: _image-loaded - - task: _controller-deployed - - task: _age-key -``` - -That is boring in a good way. The order is visible immediately. - -In `Makefile.oldway`, the same behavior was spread across file targets, prerequisites, and shell -recipes: - -```make -prepare-e2e: $(CS)/$(NAMESPACE)/prepare-e2e.ready portforward-ensure -$(CS)/$(NAMESPACE)/prepare-e2e.ready: ... -``` - -Again, Make can express this, but the flow is much less obvious to someone reading it fresh. You -have to jump between definitions and expand variables mentally before the real sequence becomes -clear. - -### 4. We kept the good part: explicit stamp state - -One important lesson from the migration is that we did not actually want to replace everything. - -The `.stamps` model was still useful. It captures runtime facts and readiness boundaries that Task's -own cache should not own for us. Keeping `.stamps` while moving orchestration to Task turned out to -be the right split. - -That is an important hindsight point: - -- switching away from Make was good -- throwing away explicit state tracking would not have been good - -## Why the old freedom was costly - -`Makefile.oldway` shows how much raw power Make gives you: - -- special forms like `.ONESHELL`, `.SECONDEXPANSION`, and grouped targets -- recursive `$(MAKE)` calls inside recipes -- target names that are also file paths and readiness markers -- a lot of behavior encoded indirectly through prerequisite relationships - -That flexibility is real, but it pushes complexity onto every maintainer. - -Task is more structured and more limited, and that has been an advantage here. The YAML shape makes -it harder to be too clever. In this repo, that constraint improved readability and maintainability. - -## Why Task fits this repo better - -This repository benefits from a tool that makes these things obvious: - -- what the public commands are -- which tasks are build tasks versus e2e orchestration -- which inputs and outputs matter -- where ordering is intentional -- which state is externalized in `.stamps` - -Task does that well enough without pretending our cluster runtime can be reduced to a pure checksum -graph. - -## Current file layout - -The current arrangement is a good outcome: - -- [Taskfile.yml](/workspaces/gitops-reverser/Taskfile.yml): small root entrypoint -- [Taskfile-build.yml](/workspaces/gitops-reverser/Taskfile-build.yml): build and local artifact tasks -- [test/e2e/Taskfile.yml](/workspaces/gitops-reverser/test/e2e/Taskfile.yml): e2e orchestration -- [Makefile.oldway](/workspaces/gitops-reverser/Makefile.oldway): historical reference only - -That split is much easier to work in than the old single-file Make model. - -## Bottom line - -In hindsight, moving from Make to Task was not just neutral cleanup. It improved the repo. - -The biggest reasons are: - -- the public command surface is clearer -- the e2e flow is easier to follow -- build tasks are more declarative -- `.stamps` stayed where explicit runtime state still matters -- maintainers no longer need as much Make-specific knowledge to change routine automation safely - -So this document should no longer be read as a migration checklist. It is now the rationale for why -the current Task-based structure is the better long-term home for this repository. diff --git a/docs/tasks-overview.md b/docs/tasks-overview.md new file mode 100644 index 00000000..5550a0f4 --- /dev/null +++ b/docs/tasks-overview.md @@ -0,0 +1,373 @@ +# Tasks Overview + +This repository drives codegen, build, unit tests, and the full e2e bring-up through +[Task](https://taskfile.dev) (`task`) instead of a `Makefile`. + +The Taskfiles encode a dependency graph (DAG). Every step from source files to a working +build, and then to a running controller under e2e, is expressed as a task with explicit +`sources`, `generates`, and `deps`. Task only re-runs the steps whose inputs changed, so a +routine edit usually waits for one small rebuild instead of a six-minute cold cluster. + +> **New to Task?** Start with the official docs at [taskfile.dev](https://taskfile.dev/docs) +> for syntax and concepts. This repo also follows the upstream +> [Task styleguide](https://taskfile.dev/docs/styleguide); the [Best practices](#best-practices) +> below are our project-specific additions on top of it. + +## Why not a Makefile + +Make could express this, but for me it became too much hassle every time. In the end I learned +about things like `.ONESHELL`, `.SECONDEXPANSION`, grouped targets, and recursive `$(MAKE)` calls. +It's all very powerful, but for me it's all a bit too much. + +Task keeps the behavior easier to follow: + +- **Clean YAML.** `desc`/`sources`/`generates`/`deps` are declared up front, it's boring in the good way. +- **A clear DAG.** [Dependencies](https://taskfile.dev/docs/guide#task-dependencies) are explicit and visible, so the e2e flow reads top-to-bottom + instead of being reconstructed from prerequisite tricks. +- **[Fingerprinting support](https://taskfile.dev/docs/guide#prevent-unnecessary-work), kept on purpose.** Task orchestrates; `.stamps` hold the runtime + facts as files (cluster ready, image loaded) that a checksum cache shouldn't own. Helper scripts under + `hack/e2e/` still do the detailed work. + +## File layout + +- [Taskfile.yml](../Taskfile.yml): small root entrypoint. It `includes` the other two with + `flatten: true` so everything shares one flat command surface, and sets `run: once` so the + e2e DAG can fan out through `deps:` without a shared node starting twice. +- [Taskfile-build.yml](../Taskfile-build.yml): build, codegen, lint, and unit-test tasks. +- [test/e2e/Taskfile.yml](../test/e2e/Taskfile.yml): the e2e bring-up DAG. + +Run `task` (or `task help`) to list everything. + +## Most important tasks + +| Task | What it does | When to run it | +| --- | --- | --- | +| `task test` | Regenerates manifests + deepcopy, runs `go fmt`/`go vet`, sets up envtest, runs all non-e2e packages, then the coverage ratchet (`cover-check`). | Before every commit. | +| `task lint` | Aggregates `lint-golang` (golangci-lint), `lint-dockerfiles` (hadolint), `lint-actions` (actionlint), and `lint-helm` (helm lint). Dockerfile/action lint can run immediately; Go and Helm lint both wait for generated files to settle first. | Before every commit. | +| **`task test-e2e`** | Builds the controller image, brings up k3d + Flux + services, installs and deploys the controller, then runs the Ginkgo suite against it. The suite's before-hook invokes `task prepare-e2e`, so this one command walks the entire DAG below. | Before every commit that changes behavior. Needs Docker running. | +| `task prepare-e2e` | Runs the bring-up/deploy half of the DAG, without specs. | Rarely by hand; it's what Tilt and the suite call. | +| **`task clean-cluster`** | Deletes the k3d cluster and removes its stamps (`.stamps/cluster//`). Forces the entire cluster subtree to rebuild cold (~5–6 min) next run. | **Only when your cluster is broken**, or when you deliberately want a cold, slow, from-scratch run. Not part of the normal loop. | +| `task clean` | Removes `bin/`, `cover.out`, `dist/`, and **all** of `.stamps/` (including the image and envtest caches). | A full local reset. | +| `task manifests` / `task generate` | Regenerate CRDs/RBAC and deepcopy code. | Usually automatic; other tasks depend on them. | +| `task build` | Compile `bin/manager`. | When you want the local binary. | + +`task clean-cluster` is the one to reach for when the e2e cluster is wedged. It only wipes +`.stamps/cluster//`, so the **controller image cache (`.stamps/image/`) survives**. The +next run rebuilds the cluster but reuses the image unless the Go sources changed. + +## Focus areas + +The graphs below use two different arrow meanings: + +- In the DAG graphs, arrows point from prerequisite/input to the task it can trigger. Read them + bottom-up: change something near the bottom, follow the arrows upward, and see what re-runs. +- In the entrypoint graph, arrows show command flow: the thing a public task calls or prepares. + +### Build, unit, and packaging + +This is the local build side: codegen feeds unit tests, binary builds, Helm chart sync, and the +rendered install bundle. + +```mermaid +flowchart BT + classDef src fill:#e8ecff,stroke:#5566aa,color:#000; + classDef key fill:#e6f7e6,stroke:#33aa33,stroke-width:3px,color:#000; + + GO["api / internal / cmd/*.go"]:::src + TESTGO["*_test.go"]:::src + GOMOD["go.mod / go.sum"]:::src + CHART["charts/gitops-reverser/**"]:::src + BOILER["hack/boilerplate.go.txt"]:::src + + GO --> generate + BOILER --> generate + generate --> manifests + GO --> manifests + manifests --> test["test
(go test)"]:::key + TESTGO --> test + GOMOD --> ENVTEST["setup-envtest"] + ENVTEST --> test + fmt --> test + vet --> test + test --> COVER["cover-check"]:::key + + manifests --> build + manifests --> HELM["helm-sync"] + CHART --> DIST["dist-install"] + HELM --> DIST +``` + +### Lint + +Linting is intentionally its own focus area. The root task is a pure aggregator; the individual +linters own their inputs. + +```mermaid +flowchart BT + classDef src fill:#e8ecff,stroke:#5566aa,color:#000; + classDef key fill:#e6f7e6,stroke:#33aa33,stroke-width:3px,color:#000; + + GO["api / cmd / internal / test *.go
+ .golangci.yml + go.mod"]:::src + DOCKER["Dockerfile, .devcontainer/Dockerfile
+ .hadolint.yaml"]:::src + WF[".github/workflows/*.yml"]:::src + CHART["charts/gitops-reverser/**"]:::src + API["api / internal / cmd/*.go"]:::src + + GO --> LGO + DOCKER --> DLINT + WF --> ALINT + API --> manifests --> SYNC["helm-sync
(regens chart CRDs)"] + manifests --> LGO + CHART --> HLINT + SYNC --> HLINT["lint-helm
(helm lint)"] + LGO["lint-golang
(golangci-lint)"] --> LINT["lint
(aggregator)"]:::key + DLINT["lint-dockerfiles
(hadolint)"] --> LINT + ALINT["lint-actions
(actionlint)"] --> LINT + HLINT --> LINT +``` + +`task lint` is a pure aggregator: it lists `lint-golang`, `lint-dockerfiles`, `lint-actions`, and +`lint-helm` as `deps:`. Dockerfile and workflow lint can run immediately. `lint-golang` waits on +`manifests`, and `lint-helm` waits on `helm-sync`, so neither scans files while codegen is rewriting +generated Go or chart inputs. CI runs this same `task lint` (in the `lint` job), so the local gate +and the CI gate are identical — the four linters can't drift apart. Each sub-task's `sources:` is +what makes the skip precise: `lint-golang` fingerprints the module's Go files (`api`, `cmd`, +`internal`, `test`), `.golangci.yml`, and `go.mod`/`go.sum`; `lint-dockerfiles` the two Dockerfiles +plus `.hadolint.yaml`; `lint-actions` the `.github/workflows/*` glob (it runs `actionlint` with no +path argument, so new workflows are auto-discovered); `lint-helm` the chart's YAML and templates. +Each fingerprint includes that tool's config, so changing a lint rule re-triggers just that linter. + +`lint-helm` is the one that is not a pure skip: it depends on `helm-sync` so the chart's generated +CRDs/role are present and current before `helm lint` runs — a complete check that also works on a +fresh CI checkout, where those files are gitignored and absent. `helm-sync` chains into +`manifests`/`controller-gen`, which re-runs on every invocation, so a `task lint` with nothing +changed still does that codegen (the generated CRDs are gitignored, so it never leaves a stray git +diff). That is the accepted cost of a complete Helm check; drop the `helm-sync` dep from +`lint-helm` to get the pure no-op back. `rm -rf .task` forces a clean re-lint of the fingerprinted +tasks if ever needed. + +### Install modes + +The public `install` task dispatches to exactly one install mode based on `INSTALL_MODE` +(`config-dir` by default for e2e). All modes require the cluster services and cleanup gate, but +they get their rendered YAML differently. + +```mermaid +flowchart BT + classDef src fill:#e8ecff,stroke:#5566aa,color:#000; + classDef key fill:#e6f7e6,stroke:#33aa33,stroke-width:3px,color:#000; + + SERVICES["_services-ready"]:::src + CLEAN["_install-cleanup"]:::src + GO["api / internal / cmd/*.go"]:::src + CHART["charts/gitops-reverser/**"]:::src + + GO --> manifests + manifests --> CONFIG["install-config-dir
(default e2e mode)"] + manifests --> HELMSYNC["helm-sync"] + HELMSYNC --> HELM["install-helm"] + HELMSYNC --> DIST["dist-install"] + CHART --> HELM + CHART --> DIST + DIST --> PLAIN["install-plain-manifests-file"] + + SERVICES --> CONFIG + SERVICES --> HELM + SERVICES --> PLAIN + CLEAN --> CONFIG + CLEAN --> HELM + CLEAN --> PLAIN + + CONFIG --> INSTALLED["selected install output
.stamps/cluster/<ctx>/<ns>/<mode>/install.yaml"]:::key + HELM --> INSTALLED + PLAIN --> INSTALLED +``` + +### E2E prepare and deploy + +`prepare-e2e` is the e2e bring-up target. It is the graph that gets the cluster, image, install, +controller, webhook TLS, SOPS key, and aggregated API server into a ready state. + +```mermaid +flowchart BT + classDef src fill:#e8ecff,stroke:#5566aa,color:#000; + classDef slow fill:#ffe6e6,stroke:#aa5555,color:#000; + classDef key fill:#e6f7e6,stroke:#33aa33,stroke-width:3px,color:#000; + + GO["api / internal / cmd/*.go
+ Dockerfile"]:::src + CLUSTERFILES["cluster/start-cluster.sh
+ audit policy/webhook config"]:::src + FLUXFILES["Flux operator + services manifests"]:::src + INSTALLFILES["selected install-mode output"]:::src + + GHCR["_ghcr-preflight"] --> CREADY["_cluster-ready"]:::slow + CLUSTERFILES --> CREADY + CREADY --> FLUX["_flux-installed"]:::slow + FLUXFILES --> FLUX + FLUX --> FSETUP["_flux-setup-ready"] + FSETUP --> SVC["_services-ready"] + SVC --> AGG["_aggregated-api-ready"] + + GO --> IMGID["_controller-image-id
(docker build)"]:::slow + IMGID --> PROJ["_project-image-ready"] + CREADY --> LOADED["_image-loaded"] + PROJ --> LOADED + + CREADY --> CLEAN["_install-cleanup"] + SVC --> INSTALLFILES + CLEAN --> INSTALLFILES + INSTALLFILES --> DEPLOYED["_controller-deployed"] + LOADED --> DEPLOYED + + DEPLOYED --> TLS["_webhook-tls-ready"] + AGE["_age-key"] --> SY["_sops-secret-yaml"] + CLEAN --> SY + SY --> SA["_sops-secret-applied"] + INSTALLFILES --> SA + + DEPLOYED --> READY["_prepare-e2e-ready"] + TLS --> READY + SA --> READY + AGG --> READY + SVC --> PF["portforward-ensure"] + READY --> PREP["prepare-e2e"]:::key + PF --> PREP +``` + +Some details in the e2e graph matter for day-to-day work: + +- **Two branches start from the same Go sources and run in parallel.** Editing a `.go` file + invalidates both `manifests`/install work and `_controller-image-id` at once; they rejoin at + deploy. +- **The cluster spine is deliberately linear and slow.** `_cluster-ready` → + `_flux-installed` → `_flux-setup-ready` → `_services-ready` is the expensive part. Everything + downstream reuses it as long as its inputs do not change. +- **`_prepare-e2e-ready` is a barrier.** It waits on the deployed controller, webhook TLS, the + applied SOPS secret, and the aggregated API server. +- **`task test-e2e` is the entrypoint, not a Task dependency of `prepare-e2e`.** The Ginkgo + `SynchronizedBeforeSuite` shells out to `task prepare-e2e` + ([e2e_suite_test.go](../test/e2e/e2e_suite_test.go)), exactly once, before specs execute. + +### E2E entrypoints and tools + +These are command-flow arrows, not Task dependency arrows. + +```mermaid +flowchart LR + classDef key fill:#e6f7e6,stroke:#33aa33,stroke-width:3px,color:#000; + + TE["task test-e2e"]:::key -. "BeforeSuite calls" .-> PREP["task prepare-e2e"]:::key + PREP --> FULL["standard Ginkgo suite"] + + PREP --> AGG["test-e2e-aggregated-api"] + PREP --> IMG["test-image-refresh"] + PREP --> QH["test-e2e-quickstart-helm"] + PREP --> QM["test-e2e-quickstart-manifest"] + + PREP --> DEMOPREP["prepare-e2e-demo"] + DEMOPREP --> DEMO["test-e2e-demo"] + DEMO --> LOAD["loadtest"] + + FULL --> ALLURE["allure-e2e-results/report/open"] + FULL --> COV["e2e-coverage-collect
(when E2E_COVERAGE=1)"] + PREP --> LAB["lab-e2e / lab-corpus-update
(serial opt-in)"] +``` + +## Why the DAG pays off: only what changed re-runs + +Because each task declares its real inputs and outputs, Task skips any step already up to +date. The practical effect, edit by edit: + +| You change | What re-runs | What stays warm | +| --- | --- | --- | +| A controller `.go` file (`internal/…`) | codegen check + **image rebuild** → reload → redeploy → suite | cluster, Flux, services, age key, install-cleanup | +| An API type (`api/v1alpha3/…`) | `generate` → `manifests` → `helm-sync`, **and** image rebuild → redeploy | the whole cluster/Flux spine | +| Only a `*_test.go` file | just the `go test` / Ginkgo run; test files are `exclude`d from image and manifest `sources` | image, deploy, cluster; no rebuild at all | +| `cluster/start-cluster.sh` or the audit policy | `_cluster-ready` invalidates → **everything downstream** re-runs (cold, ~5–6 min) | nothing; this is the expensive case | +| `go.mod` | `setup-envtest` (new envtest assets) + image rebuild | cluster/Flux | +| *Nothing* (re-run `task test-e2e`) | just the specs; every stamp is up to date, so the cluster is ready in seconds | everything | + +That bottom row is the main benefit: a warm re-run of the full e2e suite skips the entire +bring-up graph. The row to respect is the cluster-script change; it is equivalent to a +`clean-cluster` in cost, because it invalidates the root of the spine. + +## The `.stamps` model + +Task's own checksum cache can't observe whether the k3d cluster is actually alive, so we keep +explicit stamp files for the runtime facts, and let Task's `sources`/`generates` handle the +file-derived ones. The layout: + +- `.stamps/cluster//`: cluster-scoped readiness: `ready`, `flux.installed`, + `flux-setup.ready`, `services.ready`, `aggregated-api.ready`, `image.loaded`, + `ghcr-preflight.ok`, `age-key.txt`, and a per-namespace `/` subdir with + `install.yaml`, `controller.deployed`, `webhook-tls.ready`, `sops-secret.applied`, and the + final `prepare-e2e.ready`. **`task clean-cluster` removes this tree.** +- `.stamps/image/`: the controller image cache: `controller.id`, `controller.cover`, + `project-image.ready`. Survives `clean-cluster`; removed by `task clean`. +- `.stamps/envtest-.ready`: the downloaded envtest binaries marker. + +Many stamp tasks also carry a `status:` block that probes the live cluster (e.g. "does the +`fluxinstance` still exist?"), so a stamp left over from a deleted cluster is correctly +treated as out of date rather than trusted blindly. + +## Best practices + +Start from the official [Task styleguide](https://taskfile.dev/docs/styleguide). This repo +follows it: two-space indent, `UPPERCASE` variable names, `kebab-case` task names, and complex +logic pushed into external scripts (ours live under `hack/e2e/`) rather than long inline `cmds:`. + +On top of that, four project-specific rules keep the graph fast and readable, each already +visible throughout the Taskfiles: + +### 1. Prefer `deps:` over calling tasks by hand + +Declare what a task needs in `deps:` and let Task order and dedupe the work. With `run: once` +set at the [root](../Taskfile.yml), a shared node like `_cluster-ready` runs exactly once even +when it's reached through the image branch *and* the install branch. Dependencies are also +visible in the graph and can run in parallel; an imperative `task:` call buried in `cmds:` is +neither. So `deps:` is the default. + +Reach for a sequential `cmds:` → `task:` call only when the ordering itself is the point. For +example, `prepare-e2e` runs `portforward-ensure` *after* the ready barrier because a webhook +TLS step restarts the k3d server and would kill port-forwards started concurrently. `install` +also dispatches to `install-{{.INSTALL_MODE}}` by variable. Those are the exceptions, not the +pattern. + +### 2. Keep tasks small + +A small task has a precise up-to-date check, so an edit invalidates the minimum. When a step +is shared, give it its own node instead of inlining it. `_install-cleanup` was lifted out of +the top of every `_install-` task into a standalone node precisely so it could be a +single prerequisite for both the active install *and* `_sops-secret-yaml`, +which writes into the same namespace dir. Small nodes cache better and compose better. + +### 3. Let every cache-worthy task deliver a file + +Task decides whether to skip work by comparing `sources` against `generates`, so give each +such task a concrete output: + +- a real artifact where one exists: `manifests` generates `config/crd/bases/*.yaml`, + `generate` produces `zz_generated.deepcopy.go`; +- an explicit `.stamps/…` marker where the "output" is really a runtime fact: + `_cluster-ready` touches `.stamps/cluster//ready`, `_image-loaded` touches + `image.loaded`. + +Without a delivered file a task can neither be skipped nor gate a downstream `sources:` edge. +When the file stands in for live state, back it with a `status:` probe (e.g. "does the +`fluxinstance` still exist?") so a stamp left over from a deleted cluster is correctly treated +as stale rather than trusted. + +### 4. Prefix internal tasks with `_` + +Public, day-to-day commands stay unprefixed: `test`, `lint`, `test-e2e`, `clean-cluster`. +Anything you only expect to be reached *through* the graph gets a leading underscore: +`_cluster-ready`, `_image-loaded`, `_prepare-e2e-ready`. It keeps `task --list` focused on the +commands people actually type, and the `_` is a clear signal: "you normally get here via a +dependency, not by running it directly." If you're unsure whether a task is day-to-day, prefix +it; promoting it later is cheaper than a cluttered command surface. + +## What this gives us + +Task gives the repo a clearer command surface, a readable e2e flow, and declarative build +tasks while keeping explicit `.stamps` state where it still matters. Accurate +`sources`/`generates`/`deps` mean routine edits trigger minimal, correct rebuilds, which keeps +the local development loop fast. diff --git a/test/e2e/Taskfile.yml b/test/e2e/Taskfile.yml index 2686a7ae..d11ac638 100644 --- a/test/e2e/Taskfile.yml +++ b/test/e2e/Taskfile.yml @@ -100,9 +100,21 @@ vars: FLUX_OPERATOR_DIR: '{{.FLUX_OPERATOR_DIR | default "test/e2e/setup/flux-operator"}}' FLUX_WAIT_TIMEOUT: '{{.FLUX_WAIT_TIMEOUT | default "500s"}}' FLUX_SERVICES_WAIT_TIMEOUT: '{{.FLUX_SERVICES_WAIT_TIMEOUT | default "120s"}}' + # In-cluster Flux versions are pinned (no moving `:latest` / patch range) and + # come straight from the FLUX_VERSION / FLUX_OPERATOR_VERSION env vars baked into + # the CI + dev container by .devcontainer/Dockerfile — the single source of + # truth, so a bump there is the only edit needed. e2e always runs inside that + # container (local dev and every ci.yml e2e step), so these are always set. The + # distribution version is consumed as ${FLUX_VERSION} directly when + # _flux-installed renders the FluxInstance with envsubst. + # + # Operator manifests OCI artifact tag = the operator version with a 'v' prefix + # (ghcr publishes v0.53.0, not 0.53.0). + FLUX_OPERATOR_MANIFESTS_VERSION: 'v{{.FLUX_OPERATOR_VERSION}}' # Preflight (see _ghcr-preflight): the registry artifact `_flux-installed` pulls, # and how long a successful reachability check stays valid before we re-probe. - GHCR_PREFLIGHT_REF: '{{.GHCR_PREFLIGHT_REF | default "ghcr.io/controlplaneio-fluxcd/flux-operator-manifests:latest"}}' + # Probe the exact pinned artifact (not `:latest`) so the check matches the pull. + GHCR_PREFLIGHT_REF: '{{.GHCR_PREFLIGHT_REF | default (printf "ghcr.io/controlplaneio-fluxcd/flux-operator-manifests:%s" .FLUX_OPERATOR_MANIFESTS_VERSION)}}' GHCR_PREFLIGHT_MAX_AGE_MIN: '{{.GHCR_PREFLIGHT_MAX_AGE_MIN | default "5"}}' E2E_GO_TEST_TIMEOUT: '{{.E2E_GO_TEST_TIMEOUT | default "15m"}}' ALLURE_RESULTS_DIR: '{{.ALLURE_RESULTS_DIR | default ".stamps/allure-results"}}' @@ -642,6 +654,30 @@ tasks: {{.K3D}} cluster delete "{{.CLUSTER_NAME}}" - rm -rf "{{.CS}}" + cleanup-stamp-repos: + desc: Delete accumulated per-spec Git checkouts under .stamps/repos (reclaim disk) + cmds: + # Every e2e spec clones its Gitea repo into .stamps/repos/ and keeps it + # on purpose: the checkout has a working remote + credentials, so you can push, + # pull, or read the reversed Git history of a past run to investigate. Nothing + # ever prunes them, so across many runs this grows without bound (multiple GB). + # This task reclaims that space; the checkout helper re-creates .stamps/repos + # (mkdir -p) on the next run, so deleting it wholesale is safe. The paired + # generated GitProvider Secret manifests in .stamps/e2e-repo-artifacts belong + # to those same repos, so they are cleared in the same sweep. + - | + for dir in .stamps/repos .stamps/e2e-repo-artifacts; do + if [ -d "${dir}" ]; then + count="$(find "${dir}" -maxdepth 1 -mindepth 1 | wc -l | tr -d ' ')" + size="$(du -sh "${dir}" 2>/dev/null | cut -f1)" + echo "Removing ${dir} (${count} entries, ${size})" + rm -rf "${dir}" + else + echo "${dir} already clean" + fi + done + echo "Done." + _ghcr-preflight: # Fail fast — BEFORE we spend ~30s creating a k3d cluster — when ghcr.io cannot # serve the Flux operator manifests that _flux-installed pulls. Otherwise an @@ -714,12 +750,25 @@ tasks: get fluxinstance "{{.FLUX_INSTANCE_NAME}}" >/dev/null cmds: - | + # The FluxInstance pins its controller distribution to ${FLUX_VERSION} + # (the flux version from the container env — see the vars block). Render + # the operator dir through envsubst — only ${FLUX_VERSION} is substituted, + # every other line is copied verbatim — into the cluster stamp dir + # (cleaned with the cluster), then install/apply the rendered copies. The + # raw file holds a placeholder and must not be applied directly. + rendered="{{.CS}}/flux-operator" + mkdir -p "$rendered" + for f in "{{.FLUX_OPERATOR_DIR}}"/*.yaml; do + envsubst '${FLUX_VERSION}' <"$f" >"$rendered/$(basename "$f")" + done {{.FLUX_OPERATOR}} install \ --kube-context "{{.CTX}}" \ --namespace "{{.FLUX_NAMESPACE}}" \ --timeout "{{.FLUX_WAIT_TIMEOUT}}" \ - -f "{{.FLUX_INSTANCE_FILE}}" - kubectl --context "{{.CTX}}" apply -f "{{.FLUX_OPERATOR_DIR}}" + --instance-distribution-artifact "oci://ghcr.io/controlplaneio-fluxcd/flux-operator-manifests:{{.FLUX_OPERATOR_MANIFESTS_VERSION}}" \ + --auto-update=false \ + -f "$rendered/$(basename "{{.FLUX_INSTANCE_FILE}}")" + kubectl --context "{{.CTX}}" apply -f "$rendered" {{.FLUX_OPERATOR}} \ --kube-context "{{.CTX}}" \ -n "{{.FLUX_NAMESPACE}}" \ diff --git a/test/e2e/setup/flux-operator/flux-instance.yaml b/test/e2e/setup/flux-operator/flux-instance.yaml index 44bd979f..dffa5ecf 100644 --- a/test/e2e/setup/flux-operator/flux-instance.yaml +++ b/test/e2e/setup/flux-operator/flux-instance.yaml @@ -5,7 +5,7 @@ metadata: namespace: flux-system spec: distribution: - version: "2.8.x" + version: "${FLUX_VERSION}" # Rendered by the e2e `_flux-installed` task via envsubst from ${FLUX_VERSION} (defined in .devcontainer/Dockerfile) registry: ghcr.io/fluxcd components: - source-controller