diff --git a/.devcontainer/Dockerfile b/.devcontainer/Dockerfile index 6713debf..c1a02631 100644 --- a/.devcontainer/Dockerfile +++ b/.devcontainer/Dockerfile @@ -63,6 +63,7 @@ RUN apt-get update \ # NODE_MAJOR -> https://github.com/nodejs/node/releases (track a current LTS line) # MARKDOWNLINT_CLI2_VERSION -> https://www.npmjs.com/package/markdownlint-cli2?activeTab=versions # VALE_VERSION -> https://github.com/vale-cli/vale/releases +# TRIVY_VERSION -> https://github.com/aquasecurity/trivy/releases ENV PATH="/go/bin:/usr/local/go/bin:${PATH}" \ KUBECTL_VERSION=v1.36.4 \ @@ -81,7 +82,8 @@ ENV PATH="/go/bin:/usr/local/go/bin:${PATH}" \ ORAS_VERSION=1.3.3 \ NODE_MAJOR=24 \ MARKDOWNLINT_CLI2_VERSION=0.23.2 \ - VALE_VERSION=3.18.0 + VALE_VERSION=3.18.0 \ + TRIVY_VERSION=0.70.0 # Fail early on unsupported architectures instead of producing a partial image. RUN test "$(dpkg --print-architecture)" = "amd64" \ @@ -149,6 +151,23 @@ RUN asset="actionlint_${ACTIONLINT_VERSION}_linux_amd64.tar.gz" \ && install -m 0755 actionlint /usr/local/bin/actionlint \ && rm -rf "${tmpdir}" +# Install trivy (container image vulnerability scanner). It backs `task scan-image`, +# which is the same command CI runs to gate the built image — so a maintainer can +# reproduce a scan failure locally instead of pushing to find out what it said. +# +# The vulnerability DB is deliberately NOT pre-seeded here: it unpacks to 1.3GB, goes +# stale in 24h, and ten CI jobs pull this image while only two scan. CI caches it instead. +RUN asset="trivy_${TRIVY_VERSION}_Linux-64bit.tar.gz" \ + && base="https://github.com/aquasecurity/trivy/releases/download/v${TRIVY_VERSION}" \ + && tmpdir="$(mktemp -d)" \ + && curl -fsSL "${base}/${asset}" -o "${tmpdir}/${asset}" \ + && curl -fsSL "${base}/trivy_${TRIVY_VERSION}_checksums.txt" -o "${tmpdir}/checksums.txt" \ + && cd "${tmpdir}" \ + && grep " ${asset}$" checksums.txt | sha256sum -c - \ + && tar -xzf "${asset}" trivy \ + && install -m 0755 trivy /usr/local/bin/trivy \ + && rm -rf "${tmpdir}" + # Install hadolint (static linter for Dockerfiles). Since 2.15.0 the release no # longer ships a per-asset .sha256 file, only one combined checksums.sha256 in # `sha256sum *filename` format (space-asterisk, not the two-space format the diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7d38cf63..b49a9a44 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -648,7 +648,7 @@ jobs: name: Scan project image runs-on: ubuntu-latest timeout-minutes: 20 - needs: build + needs: [build, ci-container] # PRs only: scans the instrumented image the PR built (delivered as an # artifact). On main the shipped bytes are the release-grade digests, which # image-scan-release scans instead — so this scan reports to the job log @@ -659,6 +659,11 @@ jobs: packages: read env: PROJECT_IMAGE: ${{ needs.build.outputs.image }} + CI_CONTAINER: ${{ needs.ci-container.outputs.image }} + # Inside the container this is a workspace path, so the runner-side cache step + # below can persist it between runs. Without a cache Trivy re-downloads its whole + # vulnerability DB on every job, which is slow and rate-limited at the source. + TRIVY_CACHE_DIR: ${{ github.workspace }}/.stamps/trivy-cache steps: - name: Checkout code uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -671,27 +676,46 @@ jobs: name: project-image path: . - - name: Load project image - run: docker load -i project-image.tar && rm -f project-image.tar - - - name: Scan image with Trivy (report) - uses: aquasecurity/trivy-action@ed142fd0673e97e23eac54620cfb913e5ce36c25 # v0.36.0 + - name: Download CI container image + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: - image-ref: ${{ env.PROJECT_IMAGE }} - format: table - severity: CRITICAL,HIGH,MEDIUM - exit-code: "0" - - - name: Gate on critical fixable vulnerabilities - # Fails the job only for CRITICAL vulnerabilities that have a fix - # available — actionable signal, not noise from unfixed CVEs. - uses: aquasecurity/trivy-action@ed142fd0673e97e23eac54620cfb913e5ce36c25 # v0.36.0 + name: ci-container-image + path: . + + - name: Load CI container image + # rm after load: the tarball is multi-GB and otherwise occupies the runner + # for the rest of the job. + run: docker load -i ci-container-image.tar && rm -f ci-container-image.tar + + - name: Restore Trivy vulnerability DB + uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 # v4.3.0 with: - image-ref: ${{ env.PROJECT_IMAGE }} - format: table - severity: CRITICAL - ignore-unfixed: true - exit-code: "1" + path: .stamps/trivy-cache + key: trivy-db-${{ github.run_id }} + restore-keys: trivy-db- + + - name: Scan project image + # The project image is scanned straight from its artifact tarball, so this + # needs neither a `docker load` of it nor a registry pull. What the task does + # — report everything, then gate on CRITICAL-with-a-fix — is documented where + # it is defined; keeping it there is what lets a maintainer reproduce this + # exact gate with `task scan-image SCAN_ARCHIVE=project-image.tar`. + run: | + docker run --rm \ + -v "${GITHUB_WORKSPACE}:${{ env.CI_WORKDIR }}" \ + -w "${{ env.CI_WORKDIR }}" \ + -e TRIVY_CACHE_DIR="${{ env.CI_WORKDIR }}/.stamps/trivy-cache" \ + "${CI_CONTAINER}" \ + bash -lc ' + set -e + git config --global --add safe.directory "$PWD" + task scan-image SCAN_ARCHIVE=project-image.tar + ' + + - name: Make the Trivy cache readable by the runner + # The container writes it as root; actions/cache runs as the runner user. + if: always() + run: sudo chown -R "$(id -u):$(id -g)" .stamps/trivy-cache || true image-scan-release: name: Scan release image (${{ matrix.arch }}) @@ -701,11 +725,14 @@ jobs: # dispatch) this skips transitively — no skip-tolerant `if:` needed. The # release ships amd64 *and* arm64, so both digests are scanned; Trivy pulls # each by digest and analyzes the filesystem cross-arch. - needs: [build-release-amd64, build-release-arm64] + needs: [ci-container, build-release-amd64, build-release-arm64] permissions: contents: read packages: read security-events: write + env: + CI_CONTAINER: ${{ needs.ci-container.outputs.image }} + TRIVY_CACHE_DIR: ${{ github.workspace }}/.stamps/trivy-cache strategy: fail-fast: false matrix: @@ -715,6 +742,13 @@ jobs: - arch: arm64 digest: ${{ needs.build-release-arm64.outputs.digest }} steps: + - name: Checkout code + # This job scans a REMOTE image by digest and needs nothing from the repository + # except the Taskfile and .trivyignore.yaml the scan below reads. Trivy treats a + # missing --ignorefile as a fatal error, so a lost checkout fails loudly here + # rather than quietly scanning without the suppressions. + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - name: Log in to registry # Each build-release-* job pushes by digest, and buildx wraps the # single-platform image in an OCI *index* (it attaches a provenance @@ -727,19 +761,54 @@ jobs: run: | echo "${{ secrets.GITHUB_TOKEN }}" | docker login ${{ env.REGISTRY }} -u ${{ github.actor }} --password-stdin - - name: Scan release image with Trivy (report) - uses: aquasecurity/trivy-action@ed142fd0673e97e23eac54620cfb913e5ce36c25 # v0.36.0 - env: - # The pushed digest is an OCI index; pin the child to scan (see login step). - TRIVY_PLATFORM: linux/${{ matrix.arch }} + - name: Restore Trivy vulnerability DB + uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 # v4.3.0 with: - image-ref: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}@${{ matrix.digest }} - format: sarif - output: trivy-image.sarif - severity: CRITICAL,HIGH,MEDIUM - exit-code: "0" + path: .stamps/trivy-cache + key: trivy-db-${{ github.run_id }} + restore-keys: trivy-db- + + - name: Scan release image + # The same `task scan-image` the PR gate runs, so the release and the PR are + # held to one definition of what blocks. It writes SARIF for code scanning and + # then gates; the upload below runs even when the gate fails, so a blocking + # finding is still reported rather than only logged. + # + # Trivy pulls the image with its OWN registry client rather than through the + # docker daemon, so the runner's `docker login` above does not reach it: the + # credentials live in the runner's home, and this container sees only the + # workspace. Mounting that config read-only and pointing DOCKER_CONFIG at it is + # what keeps this working if the package ever stops being public. Read-only, and + # outside the workspace, so the credential is never somewhere an artifact upload + # could pick it up. DOCKER_CONFIG rather than a mount onto ~/.docker because it + # does not care which user the image runs as. + run: | + docker run --rm \ + -v "${GITHUB_WORKSPACE}:${{ env.CI_WORKDIR }}" \ + -v "${HOME}/.docker:/tmp/docker-config:ro" \ + -w "${{ env.CI_WORKDIR }}" \ + -e DOCKER_CONFIG=/tmp/docker-config \ + -e TRIVY_CACHE_DIR="${{ env.CI_WORKDIR }}/.stamps/trivy-cache" \ + -e TRIVY_PLATFORM="linux/${{ matrix.arch }}" \ + "${CI_CONTAINER}" \ + bash -lc ' + set -e + git config --global --add safe.directory "$PWD" + task scan-image \ + SCAN_IMAGE=${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}@${{ matrix.digest }} \ + SCAN_SARIF=trivy-image.sarif + ' + + - name: Make scan output readable by the runner + # The container writes both as root; the upload action and actions/cache run + # as the runner user. + if: always() + run: sudo chown -R "$(id -u):$(id -g)" .stamps/trivy-cache trivy-image.sarif || true - name: Upload scan results to GitHub code scanning + # always(): the SARIF is written by the report pass BEFORE the gate runs, so a + # gate failure must not swallow the report that explains it. + if: always() uses: github/codeql-action/upload-sarif@db488ddef3bf6cb639b32c2e9a7c0a7ea8271d28 # v4.37.8 with: sarif_file: trivy-image.sarif @@ -747,20 +816,6 @@ jobs: # other's code-scanning results. category: trivy-release-${{ matrix.arch }} - - name: Gate on critical fixable vulnerabilities - # Fails the job only for CRITICAL vulnerabilities that have a fix - # available — actionable signal, not noise from unfixed CVEs. - uses: aquasecurity/trivy-action@ed142fd0673e97e23eac54620cfb913e5ce36c25 # v0.36.0 - env: - # The pushed digest is an OCI index; pin the child to scan (see login step). - TRIVY_PLATFORM: linux/${{ matrix.arch }} - with: - image-ref: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}@${{ matrix.digest }} - format: table - severity: CRITICAL - ignore-unfixed: true - exit-code: "1" - e2e: name: E2E (${{ matrix.name }}) runs-on: ubuntu-latest diff --git a/.trivyignore.yaml b/.trivyignore.yaml new file mode 100644 index 00000000..0a2e5aed --- /dev/null +++ b/.trivyignore.yaml @@ -0,0 +1,31 @@ +# Vulnerabilities the image scan may skip, each with the reason it is not reachable here and a +# date it must be reconsidered. An entry without both is a suppression nobody can audit. +# +# The gate this feeds is `task scan-image`, which fails on CRITICAL vulnerabilities that HAVE a +# fix — so everything listed here is by definition something a reader would expect to be fixed by +# upgrading. Say why we have not. Reproduce the gate locally with that task before editing this. +vulnerabilities: + - id: CVE-2026-56854 + # golang.org/x/crypto/ssh: authentication bypass because a server does not enforce the + # source-address restrictions in an authorized_keys `from=` criterion. + # + # It reaches the image through the SOPS release binary, which getsops builds against + # x/crypto v0.54.0. Our own manager is unaffected and Trivy reports it clean: this repository + # requires x/crypto v0.55.0, which carries the fix. + # + # The vulnerable code is an SSH SERVER path (ssh.ServerConfig). SOPS is a CLI we exec to + # encrypt and decrypt files with age; it listens on nothing and accepts no SSH connection, so + # the affected path cannot be entered in this image. Suppressed rather than fixed because + # v3.13.3 is the latest SOPS release: there is no rebuilt binary to bump to, and building SOPS + # ourselves would trade an upstream signed release for one of ours, which is a bigger change + # to the supply chain than the finding warrants. + # + # Revisit when getsops ships a release built against x/crypto >= 0.55.0, and delete this entry + # then. The expiry is what forces that check to happen even if nobody is watching upstream. + paths: + - "usr/local/bin/sops" + statement: >- + Not reachable: the vulnerable path is x/crypto/ssh's SERVER-side authorized_keys handling, + and the SOPS CLI in this image runs no SSH server. Our own binary already uses the fixed + x/crypto v0.55.0. No rebuilt SOPS release exists yet (v3.13.3 is latest). + expired_at: 2026-12-01 diff --git a/AGENTS.md b/AGENTS.md index 0737dc9d..39b529d2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -36,6 +36,19 @@ workflow or Dockerfile change is covered by the normal lint gate; you can also r `task lint-actions` or `task lint-dockerfiles` directly. `actionlint`, `hadolint`, and `golangci-lint` all ship in the devcontainer image. +`trivy` ships there too, behind `task scan-image` — the same command CI runs to gate the +built image, so a scan failure can be reproduced locally instead of by pushing: + +```bash +task scan-image SCAN_ARCHIVE=project-image.tar # a docker-archive tarball +task scan-image SCAN_IMAGE=ghcr.io/example/img:tag # or an image reference +``` + +It is not part of `task lint`: it needs an image to scan, which a lint run has no reason +to build. Suppressions live in [`.trivyignore.yaml`](./.trivyignore.yaml), each with a +justification and an expiry date. A devcontainer built before `trivy` was added to the +image does not have it — rebuild the container if the task reports it missing. + It also runs the documentation checks via `task lint-docs`, which is three tasks: `lint-doc-links` (`hack/doccheck`, every tracked file), `lint-markdown` (markdownlint-cli2, every tracked file), and `lint-prose` (Vale, against diff --git a/Taskfile-build.yml b/Taskfile-build.yml index eb72a9de..7c8259b4 100644 --- a/Taskfile-build.yml +++ b/Taskfile-build.yml @@ -345,6 +345,45 @@ tasks: cmds: - actionlint + scan-image: + desc: Scan a container image for vulnerabilities — full report, then the CI gate + # This task IS the gate CI runs. Both live here rather than in the workflow so the + # severities, the ignore file and the exit codes have one definition: they were + # duplicated across four workflow steps in two jobs, which is four places to keep in + # agreement and no way to run any of them before pushing. + # + # It scans in two passes on purpose. The first reports everything a human should see + # and never fails. The second is the gate, and it fails ONLY on CRITICAL findings that + # have a fix — actionable signal, not noise from unfixed CVEs. Only the gate reads + # .trivyignore.yaml, so a suppressed finding still appears in the report above it: it + # stops blocking without disappearing. Trivy does not read that file from the working + # directory on its own, which is why --ignorefile is passed explicitly. + # + # A missing .trivyignore.yaml is a FATAL error rather than a silent no-op, which is + # the behaviour to want from a security control: the gate cannot quietly stop + # honouring its own suppressions. It is a relative path, so run this from the + # repository root (CI mounts the workspace and sets -w). + # + # Point it at either a docker-archive tarball or an image reference: + # task scan-image SCAN_ARCHIVE=project-image.tar + # task scan-image SCAN_IMAGE=ghcr.io/configbutler/gitops-reverser@sha256:... + # A tarball needs no docker daemon and no registry pull, so CI prefers it for the + # image it just built. SCAN_SARIF= makes the report pass write SARIF for GitHub + # code scanning instead of a table; the gate is unaffected either way. + vars: + SCAN_ARCHIVE: '{{.SCAN_ARCHIVE | default ""}}' + SCAN_IMAGE: '{{.SCAN_IMAGE | default ""}}' + SCAN_SARIF: '{{.SCAN_SARIF | default ""}}' + SCAN_TARGET: '{{if .SCAN_ARCHIVE}}--input {{.SCAN_ARCHIVE}}{{else}}{{.SCAN_IMAGE}}{{end}}' + SCAN_REPORT_FORMAT: '{{if .SCAN_SARIF}}--format sarif --output {{.SCAN_SARIF}}{{else}}--format table{{end}}' + preconditions: + - sh: '[ -n "{{.SCAN_ARCHIVE}}{{.SCAN_IMAGE}}" ]' + msg: "set SCAN_ARCHIVE= or SCAN_IMAGE= — there is nothing to scan otherwise" + cmds: + - trivy image {{.SCAN_TARGET}} {{.SCAN_REPORT_FORMAT}} --severity CRITICAL,HIGH,MEDIUM --exit-code 0 + - trivy image {{.SCAN_TARGET}} --format table --severity CRITICAL --ignore-unfixed + --ignorefile .trivyignore.yaml --exit-code 1 + lint-dockerfiles: desc: Lint the project Dockerfiles with hadolint # Only re-lint when a Dockerfile or the hadolint config changes; Task diff --git a/api/v1alpha3/gittarget_types.go b/api/v1alpha3/gittarget_types.go index dfc6125a..f9d70f39 100644 --- a/api/v1alpha3/gittarget_types.go +++ b/api/v1alpha3/gittarget_types.go @@ -144,6 +144,50 @@ type GitTargetSpec struct { // ones are not — for a stored GitTarget as well as a new one. // +optional Prune *PrunePolicy `json:"prune,omitempty"` + + // Design rationale, kept out of the generated CRD description by the blank line below. + // + // Suspend is a PANIC KNOB: one field that stops this target writing, reachable without + // deleting anything and without unpicking the watch configuration that would have to be + // rebuilt afterwards. That is the whole justification, and it is enough on its own. + // + // It is deliberately NOT a preview mechanism. A target that writes nothing has nothing to + // show, and the honest way to see what a target would do is to point one at a scratch branch + // and read the commits — real bytes, real registrations, real deletes, diffable. The + // manifest-analyzer CLI is the other half of that answer. Neither is something status should + // grow a second, worse copy of; see docs/layout/model.md § "Previewing a target: point it at a + // scratch branch". + // + // The scan is deliberately NOT suspended with the write, and the reason is incident response + // rather than preview: a stopped valve that also stopped looking would freeze status.placement + // at whatever the folder looked like the moment someone panicked, which is exactly when a + // stale answer costs the most. + // + // Deliberately MUTABLE and deliberately not a fault: a suspended target reports Ready=True with + // reason Suspended, because not writing is the configured outcome. The precedent is + // status.retention — a condition asserts health, and suppressing a write on request is not ill + // health. + + // Suspend stops this target from writing to Git, without deleting it. It is the knob to turn + // when something is wrong and the writes have to stop now: watches keep running, events keep + // arriving, the folder keeps being scanned and status.placement keeps being maintained, and no + // new commit is planned while it is set. + // + // It takes effect at the next planning boundary, not instantly. Work already committed + // locally when suspend is set is still pushed, so a target suspended during a push cooldown + // can publish one more commit seconds later. That is deliberate: a local commit that is never + // pushed would sit in the worker's checkout indefinitely and surface later, out of order, on + // resume. Suspend is a valve on new work, not an undo — it stops the next write, it does not + // revert the last one. + // + // While it is set, status.retention is not published at all: nothing is swept, so nothing is + // measured, and reporting zero retained documents would read as "converged" when nothing was + // counted. + // + // Omitted, it is false. Clearing it resumes writing from the current cluster state on the next + // resync; the writes suppressed while suspended are not replayed. + // +optional + Suspend bool `json:"suspend,omitempty"` } // GitTargetPlacementSpec declares where NEW resources are written when no document @@ -224,8 +268,114 @@ type GitTargetStatus struct { // never a fault, and no condition changes state because of it. // +optional Retention *GitTargetRetentionStatus `json:"retention,omitempty"` + + // Placement is what the last scan resolved about this folder's layout: whether a kustomize + // render root governs new documents, which one, and whether the folder renders a base it may + // not write to. It is an observation, like streams and retention, and the LayoutResolved + // condition carries the verdict. + // +optional + Placement *GitTargetPlacementStatus `json:"placement,omitempty"` +} + +// Design rationale, kept out of the generated CRD description by the blank line below. +// +// This stanza answers ONE question: why did a write take the shape it did, or why was it refused. +// It is deliberately not a preview of what the target would do, and three fields were removed for +// trying to be one — examples (a fabricated object at a fabricated path), byTypeEntries (a count +// of a spec map the same GET already returns), and serializeNamespace (a copy of a spec field). +// +// The rule that kept them out is worth stating, because it is what to hold new fields against: a +// field earns its place only if a reader cannot get it from the spec, AND it varies with this +// folder. The write behaviours that follow from Mode — registration into resources:, the +// deregistration a delete performs, the $patch: delete an inherited object needs — are constants +// of the mode rather than facts about the folder, so they are documented on Mode and not +// enumerated here. +// +// To PREVIEW what a target would do, point one at a scratch branch and read the commits it makes. +// That is complete, reviewable, and real, where any status stanza is a summary; see +// docs/layout/model.md § "Previewing a target: point it at a scratch branch". +// +// The resolution REASON is a condition reason (LayoutResolved), not a field, because every +// consumer in this ecosystem already reads reasons from conditions. +// +// There are NO counters. placedResources, overriddenTypes and refusedResources are metrics, and +// placements_total carries them with better labels; a counter in status is a status write per +// event, which re-creates the self-triggering reconcile edge the status work already fixed once. +// +// Nothing here may depend on a placement having HAPPENED. Every field is a fact about the folder +// from the last scan, so the whole stanza is available before the target has ever written a byte. + +// GitTargetPlacementStatus is what the last scan resolved about a GitTarget folder's layout. +type GitTargetPlacementStatus struct { + // Mode is how this folder is written, and it is the field that predicts the surprises. + // + // `Plain` — no kustomization governs the folder. A new document is written and nothing else + // is touched; a delete removes it and nothing else is touched. + // + // `KustomizeRoot` — exactly one kustomization governs the folder and the folder is + // self-contained. A new document is also registered in that root's `resources:`; a delete + // also drops its entry; and every write is proved by re-rendering, so kustomize itself can + // refuse one. + // + // `KustomizeOverlay` — as KustomizeRoot, and the folder additionally renders a base outside + // it (ReadOnlyBases). The base is read-only input: an edit to a field the base owns is + // authored into the overlay when it is expressible there (`images:`, `replicas:`) and refused + // with WriteBoundaryRefused otherwise, and DELETING an object the overlay inherits authors a + // `$patch: delete` into the overlay rather than removing anything. + // + // Empty when the folder covers more than one render root, which is LayoutResolved=Ambiguous: + // there is no single answer, and no new document is placed until the target is pointed at one + // of them. + // +optional + // +kubebuilder:validation:Enum=Plain;KustomizeRoot;KustomizeOverlay + Mode PlacementMode `json:"mode,omitempty"` + + // RenderRoot is the kustomization directory that governs new documents in this folder, + // relative to spec.path; "." is the folder itself. Empty for Plain (there is no root) and + // under LayoutResolved=Ambiguous (there is no single one). + // +optional + RenderRoot string `json:"renderRoot,omitempty"` + + // ReadOnlyBases are the kustomization directories this folder renders but may never write to, + // relative to spec.path — so they lead with "../". Non-empty exactly when Mode is + // KustomizeOverlay, and it is what makes a WriteBoundaryRefused message predictable instead of + // surprising: an edit that lands on a document under one of these paths cannot be written + // where it lives. + // +optional + ReadOnlyBases []string `json:"readOnlyBases,omitempty"` + + // ResolvedAtRevision is the Git revision this resolution was first observed at. It is not + // re-stamped on every scan: a resolution that has not changed is not republished, because + // doing so would write status once per commit to the branch, whichever target caused the + // commit. So it dates the RESOLUTION, not the last scan — a revision older than the branch + // head means the folder's layout has not changed since, not that nothing has looked. + // + // It is empty when the branch had no commit at the time (a folder nothing has written to yet) + // and is filled in by the first scan that finds one. + // +optional + ResolvedAtRevision string `json:"resolvedAtRevision,omitempty"` + + // ResolvedAt is when this resolution was computed. Like ResolvedAtRevision it dates the + // resolution rather than the last scan, so a timestamp well in the past means the folder's + // shape has been stable, not that scanning stopped. + // +optional + ResolvedAt *metav1.Time `json:"resolvedAt,omitempty"` } +// PlacementMode is how a GitTarget folder is written: as plain files, as a kustomize root, or as +// an overlay over a base it may not write to. +type PlacementMode string + +const ( + // PlacementModePlain is a folder no kustomization governs. + PlacementModePlain PlacementMode = "Plain" + // PlacementModeKustomizeRoot is a self-contained folder governed by exactly one kustomization. + PlacementModeKustomizeRoot PlacementMode = "KustomizeRoot" + // PlacementModeKustomizeOverlay is a folder governed by one kustomization that renders a base + // outside the write scope. + PlacementModeKustomizeOverlay PlacementMode = "KustomizeOverlay" +) + // GitTargetStreamsStatus is a bounded roll-up of the stream readiness state for the // types this GitTarget tracks. type GitTargetStreamsStatus struct { @@ -288,6 +438,10 @@ type GitTargetRetentionStatus struct { // +kubebuilder:printcolumn:name="Ready",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].status` // +kubebuilder:printcolumn:name="Reason",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].reason` // +kubebuilder:printcolumn:name="Streams",type=string,JSONPath=`.status.streams.summary` +// +kubebuilder:printcolumn:name="Suspended",type=boolean,JSONPath=`.spec.suspend`,priority=1 +// +kubebuilder:printcolumn:name="Layout",type=string,JSONPath=`.status.placement.mode`,priority=1 +// +kubebuilder:printcolumn:name="RenderRoot",type=string,JSONPath=`.status.placement.renderRoot`,priority=1 +// +kubebuilder:printcolumn:name="LayoutResolved",type=string,JSONPath=`.status.conditions[?(@.type=="LayoutResolved")].reason`,priority=1 // +kubebuilder:printcolumn:name="GitPathAccepted",type=string,JSONPath=`.status.conditions[?(@.type=="GitPathAccepted")].status`,priority=1 // +kubebuilder:printcolumn:name="RenderMatchesLive",type=string,JSONPath=`.status.conditions[?(@.type=="RenderMatchesLive")].status`,priority=1 // +kubebuilder:printcolumn:name="StreamsRunning",type=string,JSONPath=`.status.conditions[?(@.type=="StreamsRunning")].status`,priority=1 diff --git a/api/v1alpha3/zz_generated.deepcopy.go b/api/v1alpha3/zz_generated.deepcopy.go index 057a273d..4a43f048 100644 --- a/api/v1alpha3/zz_generated.deepcopy.go +++ b/api/v1alpha3/zz_generated.deepcopy.go @@ -754,6 +754,30 @@ func (in *GitTargetPlacementSpec) DeepCopy() *GitTargetPlacementSpec { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *GitTargetPlacementStatus) DeepCopyInto(out *GitTargetPlacementStatus) { + *out = *in + if in.ReadOnlyBases != nil { + in, out := &in.ReadOnlyBases, &out.ReadOnlyBases + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.ResolvedAt != nil { + in, out := &in.ResolvedAt, &out.ResolvedAt + *out = (*in).DeepCopy() + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GitTargetPlacementStatus. +func (in *GitTargetPlacementStatus) DeepCopy() *GitTargetPlacementStatus { + if in == nil { + return nil + } + out := new(GitTargetPlacementStatus) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *GitTargetRetentionStatus) DeepCopyInto(out *GitTargetRetentionStatus) { *out = *in @@ -838,6 +862,11 @@ func (in *GitTargetStatus) DeepCopyInto(out *GitTargetStatus) { *out = new(GitTargetRetentionStatus) (*in).DeepCopyInto(*out) } + if in.Placement != nil { + in, out := &in.Placement, &out.Placement + *out = new(GitTargetPlacementStatus) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GitTargetStatus. diff --git a/cmd/main.go b/cmd/main.go index a0f2a361..085893d5 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -180,6 +180,7 @@ func main() { // write-boundary precondition) would abort the commit and leave the GitTarget looking // healthy; the resync path already reports its own refusals through the router. workerManager.SetPathRefusalReporter(watchMgr.ReportGitPathRefusal) + workerManager.SetLayoutReporter(watchMgr.ReportLayoutResolved) // WatchRule controller (with WatchManager reference for dynamic reconciliation) fatalIfErr((&controller.WatchRuleReconciler{ diff --git a/config/crd/bases/configbutler.ai_gittargets.yaml b/config/crd/bases/configbutler.ai_gittargets.yaml index d26b385e..1df8f7e7 100644 --- a/config/crd/bases/configbutler.ai_gittargets.yaml +++ b/config/crd/bases/configbutler.ai_gittargets.yaml @@ -24,6 +24,22 @@ spec: - jsonPath: .status.streams.summary name: Streams type: string + - jsonPath: .spec.suspend + name: Suspended + priority: 1 + type: boolean + - jsonPath: .status.placement.mode + name: Layout + priority: 1 + type: string + - jsonPath: .status.placement.renderRoot + name: RenderRoot + priority: 1 + type: string + - jsonPath: .status.conditions[?(@.type=="LayoutResolved")].reason + name: LayoutResolved + priority: 1 + type: string - jsonPath: .status.conditions[?(@.type=="GitPathAccepted")].status name: GitPathAccepted priority: 1 @@ -361,6 +377,27 @@ spec: - Always type: string type: object + suspend: + description: |- + Suspend stops this target from writing to Git, without deleting it. It is the knob to turn + when something is wrong and the writes have to stop now: watches keep running, events keep + arriving, the folder keeps being scanned and status.placement keeps being maintained, and no + new commit is planned while it is set. + + It takes effect at the next planning boundary, not instantly. Work already committed + locally when suspend is set is still pushed, so a target suspended during a push cooldown + can publish one more commit seconds later. That is deliberate: a local commit that is never + pushed would sit in the worker's checkout indefinitely and surface later, out of order, on + resume. Suspend is a valve on new work, not an undo — it stops the next write, it does not + revert the last one. + + While it is set, status.retention is not published at all: nothing is swept, so nothing is + measured, and reporting zero retained documents would read as "converged" when nothing was + counted. + + Omitted, it is false. Clearing it resumes writing from the current cluster state on the next + resync; the writes suppressed while suspended are not replayed. + type: boolean required: - branch - path @@ -453,6 +490,74 @@ spec: by the controller. format: int64 type: integer + placement: + description: |- + Placement is what the last scan resolved about this folder's layout: whether a kustomize + render root governs new documents, which one, and whether the folder renders a base it may + not write to. It is an observation, like streams and retention, and the LayoutResolved + condition carries the verdict. + properties: + mode: + description: |- + Mode is how this folder is written, and it is the field that predicts the surprises. + + `Plain` — no kustomization governs the folder. A new document is written and nothing else + is touched; a delete removes it and nothing else is touched. + + `KustomizeRoot` — exactly one kustomization governs the folder and the folder is + self-contained. A new document is also registered in that root's `resources:`; a delete + also drops its entry; and every write is proved by re-rendering, so kustomize itself can + refuse one. + + `KustomizeOverlay` — as KustomizeRoot, and the folder additionally renders a base outside + it (ReadOnlyBases). The base is read-only input: an edit to a field the base owns is + authored into the overlay when it is expressible there (`images:`, `replicas:`) and refused + with WriteBoundaryRefused otherwise, and DELETING an object the overlay inherits authors a + `$patch: delete` into the overlay rather than removing anything. + + Empty when the folder covers more than one render root, which is LayoutResolved=Ambiguous: + there is no single answer, and no new document is placed until the target is pointed at one + of them. + enum: + - Plain + - KustomizeRoot + - KustomizeOverlay + type: string + readOnlyBases: + description: |- + ReadOnlyBases are the kustomization directories this folder renders but may never write to, + relative to spec.path — so they lead with "../". Non-empty exactly when Mode is + KustomizeOverlay, and it is what makes a WriteBoundaryRefused message predictable instead of + surprising: an edit that lands on a document under one of these paths cannot be written + where it lives. + items: + type: string + type: array + renderRoot: + description: |- + RenderRoot is the kustomization directory that governs new documents in this folder, + relative to spec.path; "." is the folder itself. Empty for Plain (there is no root) and + under LayoutResolved=Ambiguous (there is no single one). + type: string + resolvedAt: + description: |- + ResolvedAt is when this resolution was computed. Like ResolvedAtRevision it dates the + resolution rather than the last scan, so a timestamp well in the past means the folder's + shape has been stable, not that scanning stopped. + format: date-time + type: string + resolvedAtRevision: + description: |- + ResolvedAtRevision is the Git revision this resolution was first observed at. It is not + re-stamped on every scan: a resolution that has not changed is not republished, because + doing so would write status once per commit to the branch, whichever target caused the + commit. So it dates the RESOLUTION, not the last scan — a revision older than the branch + head means the folder's layout has not changed since, not that nothing has looked. + + It is empty when the branch had no commit at the time (a folder nothing has written to yet) + and is filled in by the first scan that finds one. + type: string + type: object retention: description: |- Retention reports documents a resync kept because this target's spec.prune.mode suppressed diff --git a/docs/INDEX.md b/docs/INDEX.md index 0f6f6928..adff1e47 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -133,7 +133,7 @@ by path from Go source. |---|---|---| | [`contextual-namespace.md`](layout/contextual-namespace.md) | **spec** | kustomize namespace inference; the supported subset | | [`new-file-placement-rules.md`](layout/new-file-placement-rules.md) | **spec** | where a new resource's file goes: declared, the folder's one kustomize root, canonical. Sibling inference is removed, and kept as history | -| [`model.md`](layout/model.md) | **design** | **reversed, and much smaller than it was.** The earlier thesis wanted `spec.placement` replaced by a `spec.layout` discriminated union; three of its five arguments were retired by [#319](https://github.com/ConfigButler/gitops-reverser/pull/319), which made registration an invariant. So the template **stays** and gains two optional booleans: **`spec.placement.useKustomize`** (create and maintain the folder's root; registering into a root that already exists is an invariant, not a setting) and **`spec.serializeNamespace`** (a `*bool`, because unset must keep meaning "infer" — no plain default preserves today's behavior), which sits one level up because it governs the bytes of every write and the identity a managed document is found by, not just new files. Carries four kustomize facts **measured** against v5.8.1, three of which contradict the earlier model; the `status.placement` stanza and the post-scan pass; and the build order. The headline is what it deletes — `spec.layout`, `kind`, `scope`, `kustomize.create`, the `LayoutProfile` question, and the migration — so the largest breaking change in the queue stops being breaking at all | +| [`model.md`](layout/model.md) | **design** | **reversed, and much smaller than it was.** The earlier thesis wanted `spec.placement` replaced by a `spec.layout` discriminated union; three of its five arguments were retired by [#319](https://github.com/ConfigButler/gitops-reverser/pull/319), which made registration an invariant. So the template **stays** and gains two optional booleans: **`spec.placement.useKustomize`** (create and maintain the folder's root; registering into a root that already exists is an invariant, not a setting) and **`spec.serializeNamespace`** (a `*bool`, because unset must keep meaning "infer" — no plain default preserves today's behavior), which sits one level up because it governs the bytes of every write and the identity a managed document is found by, not just new files. Carries four kustomize facts **measured** against v5.8.1, three of which contradict the earlier model; the `status.placement` stanza and the post-scan pass; and the build order. The headline is what it deletes — `spec.layout`, `kind`, `scope`, `kustomize.create`, the `LayoutProfile` question, the migration, **the post-scan supplier guard** (the supplier of a namespace-free folder lives in another cluster and may not even be single, so the check fires on the correct configuration), and **the dry-run framing of `spec.suspend`** (a scratch branch is a better preview and needs nothing built, so `suspend` is a panic knob and `status.placement` explains writes rather than previewing them) — so the largest breaking change in the queue stops being breaking at all | [`shapes/`](layout/shapes/README.md) is the specification by example: the cross-product of folder shapes, one live object written into all of them, so the only difference between two folders is the diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index b1c3a9f1..1e5c0753 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,50 @@ guidance that the changelog's breaking-change entries link to. We are pre-1.0, so breaking changes bump the **minor** version (release-please is configured with `bump-minor-pre-major`) rather than the major. Read the relevant entry before upgrading across it. +## A GitTarget must cover exactly one kustomize render root + +A `GitTarget` whose `spec.path` covers more than one kustomize render root — an app root above a +`base/` and several `overlays/`, rather than one leaf overlay — no longer places new documents. It +reports `LayoutResolved=False` with reason `Ambiguous`, naming the roots it covers, and refuses the +write with `GitPathAccepted=False`, reason `AmbiguousLayout`. + +Before this, such a target wrote the new document to the built-in canonical path inside the folder +it covered, where it belonged to no render root and no deployer would ever apply it. + +**Who is affected.** Only a target pointed at a folder with several kustomizations under it. A +folder with one kustomization, or none at all, is untouched. **Existing documents are untouched +either way**: a resource that already has a document in Git is edited where it lives, whatever the +folder covers, so nothing stops being mirrored and nothing moves. + +**Find them before you upgrade.** For each `GitTarget`, count the `kustomization.yaml` files under +its `spec.path` in the branch it writes to: + +```bash +kubectl get gittargets -A \ + -o custom-columns=NS:.metadata.namespace,NAME:.metadata.name,PATH:.spec.path +# then, in a checkout of each target's branch: +find -name kustomization.yaml +``` + +Two or more, and that target is affected — unless all but one of them are referenced by another +(a `base/` that every overlay lists is not itself a root). + +**What to do about it.** Point the target at one leaf, and declare the other environments as their +own `GitTarget` objects: + +```yaml +spec: + path: apps/checkout/overlays/prod # not apps/checkout +``` + +One target is one environment is one write partition, which is what makes authorization, audit and +review line up with the environment boundary. The reasoning is in +[`layout/shapes/README.md`](layout/shapes/README.md#why-only-a-leaf-can-be-a-kustomize-target). + +`status.placement.mode` and `status.placement.renderRoot` report what the scan resolved, and both +are published before a target has written anything — so a target you have just declared already +says whether it found one root, none, or several. + ## New resources land where you declare, not where the folder's other documents live Sibling inference is gone. A resource with no document in Git yet is placed by the first of three diff --git a/docs/configuration.md b/docs/configuration.md index 2c64a7be..fb657f04 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -530,9 +530,132 @@ The most useful status fields are: - `GitPathAccepted`: true when the target Git path is safe to materialize. - `status.streams`: bounded counts for tracked, running, replaying, and blocked streams. - `status.retention`: how many documents `spec.prune.mode` is keeping, and under which mode. +- `LayoutResolved`: what the last scan resolved about the folder's shape, with + `status.placement` carrying the detail. See below. Use conditions for automation. +### Seeing what a target will do, before it does it + +**Point one at a scratch branch.** + +```yaml +spec: + providerRef: {name: homelab} + branch: gitops-preview # not main + path: apps/checkout +``` + +It commits, and you read the commits: the real files, the real `resources:` registrations, the real +deletes, in a diff you can review. Nothing is simulated, so nothing can be wrong about it. When you +are happy, delete the preview target and declare the real one. `spec.branch` is immutable, so the +two are always separate objects, and deleting the preview cannot disturb the real folder. + +For inspecting a repository without a cluster at all, use the `manifest-analyzer` CLI. + +`spec.suspend` is **not** this. It is the next section. + +### Stopping a target from writing (`spec.suspend`) + +The panic knob. One field that stops a target writing, without deleting it and without unpicking +the `WatchRule` objects you would have to rebuild afterwards: + +```yaml +spec: + path: apps/checkout + suspend: true +``` + +A suspended target keeps its watches, keeps receiving events, and keeps scanning its folder. A valve +that also stopped looking would leave `status.placement` frozen at whatever the folder looked like +the moment you turned it, which is exactly when a stale answer costs the most. It plans no new +commit. `Ready` stays `True` with reason `Suspended`, because not writing is the configured outcome +rather than a fault; every other gate still applies, so a suspended target with a broken +`GitProvider` is still not ready. + +`status.retention` is not published while a target is suspended: no resync sweeps, so nothing is +counted, and a published zero would read as "converged" when it means "not measured". + +**It takes effect at the next planning boundary, not instantly.** Work already committed locally +when you set it is still pushed, so a target suspended during a push cooldown can publish one more +commit seconds later. That is deliberate: a local commit that is never pushed would sit in the +operator's checkout and surface later, out of order, when you resume. Suspend is a valve on new +work, not an undo. + +Clearing `suspend` resumes from the cluster's current state on the next resync. The writes +suppressed while it was set are not replayed, so what lands is what the cluster holds then, not a +backlog of the values it passed through. + +To re-read the folder now rather than on the periodic cadence, stamp the reconcile-request +annotation with any value that changes: + +```bash +kubectl annotate gittarget checkout \ + reconcile.configbutler.ai/requestedAt="$(date -u +%FT%TZ)" --overwrite +``` + +The spelling is Flux's `reconcile.fluxcd.io/requestedAt` with our own group; the value carries no +meaning, only its change does. + +### What the folder resolved to (`status.placement`) + +`status.placement` is what the last scan learned about the **folder**, and it is available before +the target has ever written. It answers one question: *why did a write take the shape it did, or why +was it refused?* It deliberately restates nothing the spec already carries: + +```yaml +status: + conditions: + - type: LayoutResolved + status: "True" + reason: SingleKustomization # SingleKustomization | Ambiguous | None + message: 'render root "." governs new files; it renders ../../base, which is read-only input' + placement: + mode: KustomizeOverlay # Plain | KustomizeRoot | KustomizeOverlay + renderRoot: . + readOnlyBases: ["../../base"] + resolvedAtRevision: 9f3c1ab + resolvedAt: "2026-07-30T09:14:22Z" +``` + +- `mode` is **how this folder is written**, and it is the field that predicts the surprises: + + | | `Plain` | `KustomizeRoot` | `KustomizeOverlay` | + |---|---|---|---| + | a new document | written | written **and registered** in `resources:` | same | + | a delete | file removed | file removed **and its `resources:` entry dropped** | same, unless the object is inherited | + | deleting an object the folder inherits from its base | n/a | n/a | a **`$patch: delete` is authored into the overlay**; nothing is removed | + | editing a field the base owns | n/a | n/a | authored into the overlay for `images:`/`replicas:`, **refused** otherwise | + | can kustomize refuse the write | no | yes, by re-rendering | yes | + + It is absent when the folder covers several render roots, along with `renderRoot`: there is no + single answer. +- `renderRoot` is the kustomization directory that governs new documents, relative to `spec.path`; + `.` is the folder itself. Empty for `Plain`, and when the folder has several roots. +- `readOnlyBases` are directories the folder renders but may never write to, spelled the way the + overlay's own `resources:` spells them. Non-empty exactly when `mode` is `KustomizeOverlay`, and + an edit landing on a document under one of them is what a `WriteBoundaryRefused` refusal is about. +- `resolvedAtRevision` and `resolvedAt` date **the resolution**, not the last scan. They advance + when the resolution changes, not on every scan of an unchanged folder, so a timestamp well in the + past means the folder's shape has been stable, rather than that scanning stopped. + +The stanza carries no counters, no `examples`, and no copy of `spec.serializeNamespace` or of the +`byType` map. The rule is that a status field earns its place only if a reader cannot get it from +the spec in the same GET, **and** it varies with this folder. To preview what a target would write, +use a scratch branch (above) rather than a status field. + +`LayoutResolved` reports the verdict. `None` (no kustomization governs the folder) is `True` and +perfectly healthy; it is the ordinary case. Only `Ambiguous` is `False`: + +**A target must cover exactly one kustomize render root.** A `GitTarget` at `apps/checkout` in a +base-plus-overlays repository covers the base and every overlay, so a new document has no single +root to be placed into, and picking one would hand it to an environment nobody named. Such a target +reports `LayoutResolved=False` with reason `Ambiguous`, naming the roots it covers, and refuses to +place new documents (`GitPathAccepted=False`, reason `AmbiguousLayout`). Existing documents are +unaffected: they are edited where they already live. The fix is to point the target at one leaf, +`apps/checkout/overlays/prod`, and declare the other environments as their own `GitTarget` objects: +one target is one environment is one write partition. + ### Deletion policy (`spec.prune.mode`) A target removes a document from Git for one of two very different reasons, and `spec.prune.mode` diff --git a/docs/design/build-order.md b/docs/design/build-order.md index 692f7b85..697c0993 100644 --- a/docs/design/build-order.md +++ b/docs/design/build-order.md @@ -18,7 +18,7 @@ couplings that are real. flowchart TB subgraph A["Track A — additive placement (no consumer bump)"] direction LR - A1["PR 1
corpus + suspend
+ status.placement
+ the Ambiguous rule"] --> A2["PR 2
useKustomize + serializeNamespace
+ the supplier rule
+ one-source-namespace refusal"] + A1["PR 1
corpus + suspend
+ status.placement
+ the Ambiguous rule"] --> A2["PR 2
useKustomize + serializeNamespace
+ one-source-namespace refusal"] end subgraph B["Track B — the breaking wave (one coordinated bump)"] direction LR @@ -39,8 +39,8 @@ tracks are the *independence* argument — why the order is free — and the PR | PR | Contains | Breaking | Done when | |---|---|---|---| -| **1 — see it before it writes** | the corpus wired up, `spec.suspend` + the reconcile-request annotation, `status.placement` + `LayoutResolved`, and the post-scan pass's **`Ambiguous` rule** | no | a suspended target reports what it resolved, and every corpus scenario either passes or is skipped naming PR 2 | -| **2 — the two booleans** | `spec.serializeNamespace`, `placement.useKustomize`, the post-scan pass's **supplier rule**, the one-source-namespace refusal, creating a `kustomization.yaml` | no | the last skip is gone | +| **1 — explain what it did** | the corpus wired up, `spec.suspend` + the reconcile-request annotation, `status.placement` + `LayoutResolved`, and the post-scan pass (one rule: **`Ambiguous`**) | no | a refused or surprising write is explainable from status, and every corpus scenario either passes or is skipped naming PR 2 | +| **2 — the two booleans** | `spec.serializeNamespace`, `placement.useKustomize`, the one-source-namespace refusal, creating a `kustomization.yaml` | no | every corpus skip naming PR 2 is gone | | **3 — the breaking wave** | delete `allowedSourceNamespaces`, redefine `sourceNamespace: "*"`, the `commit.window` / `commit.message` moves and their riders | **yes**, one bump | the wave's own migration note is satisfied | PRs 1 and 2 are specified in @@ -50,18 +50,38 @@ with its co-members in [`gittarget-api-wave.md`](gittarget-api-wave.md); track C [`support-boundary/patch-authoring.md` § Delivery sequence](support-boundary/patch-authoring.md#delivery-sequence). Those pages are the authority on *what*; this one only on *when*. -**PR 1 is one feature with four parts, not four features.** `suspend` with no status shows you -nothing; `status.placement` with no `suspend` arrives after the first write, which is too late to act -on; and the corpus is what proves either of them behaves as written. Their real common property is -what makes them one review: **nothing in PR 1 changes what the operator writes.** That is the -reviewability the old four-way split was buying, and merging them keeps it at PR granularity rather -than spending three PRs to get it. - -**The post-scan pass splits by rule, and that is a new seam this cut introduces.** Its two rules do -not have the same inputs: *a folder covering two render roots is `Ambiguous`* reads only the scan and -ships in PR 1, while *`serializeNamespace: false` needs a supplier* reads a field that does not exist -until PR 2. So the pass is not "done" in PR 1 — it exists, with one rule in it. Worth stating, -because the old plan had the pass landing whole. +**PR 1 is one feature with four parts, not four features.** The parts are each small, and the corpus +is what proves any of them behaves as written. Their common property is what makes them one review: +**PR 1 changes what the operator writes in exactly one case, and that case is the `Ambiguous` +rule.** Everything else in it is a report. + +They are independent of each other, and that is worth stating because the grouping invites the +opposite reading. `suspend` is a panic knob — a way to stop the writes that is not deleting the +object — and needs no status to be useful. `status.placement` answers "why did that write take that +shape" and needs no `suspend` to be useful. Neither is a preview: previewing a target means pointing +one at a scratch branch and reading the commits +([`../layout/model.md`](../layout/model.md#previewing-a-target-point-it-at-a-scratch-branch)). They +ship together because they are small and adjacent. + +**The one case, stated plainly**, because a rule that gates is a write-behavior change however +additive the rest of the PR is: a GitTarget covering more than one kustomize render root — an app +root rather than a leaf overlay — stops placing new documents. Before +PR 1 it placed them at the canonical path inside whichever folder it covered. The refusal is raised +at the placement site and surfaces as `GitPathAccepted=False`, reason `AmbiguousLayout`, with +`LayoutResolved=False` naming the roots the folder covers. An existing document is unaffected: it is +edited where it already lives, whatever the folder covers. + +**It gates at the write rather than on `Validated`, and the difference is recoverability.** +`Validated` is evaluated before the data plane exists, so a target failing it never registers a +worker, never scans, and could therefore never observe that the folder had been fixed — and a target +that had never scanned could never trip the rule in the first place. Refusing at the placement site +keeps the target declared and scanning, so narrowing it to a leaf clears the refusal the way fixing +any other unsupported content does. + +**The post-scan pass lands whole in PR 1**, and it is one rule: *a folder covering two render roots +is `Ambiguous`*, which reads only the scan. It has no second rule — `serializeNamespace: false` is +not checked against the folder, because the namespace supplier lives outside the repository +([`../layout/model.md`](../layout/model.md#why-false-needs-no-guard)). **Order between them is free, and the numbering is a recommendation.** PR 3 does not block PR 1 or 2 and neither blocks it — see [the couplings that do not @@ -77,15 +97,17 @@ a file nobody asked for by name, and keep **the write-plan precondition ahead of check**, because the precondition is the correctness layer and admission is only feedback. But be honest about what that does not buy: -- **Bisect and revert granularity is the PR.** Merging the old PRs 1–3 means a regression in - `suspend`, in `status.placement`, or in the corpus is one commit on `main`, and reverting any of - them reverts all three. The mitigation is that PR 1 changes no write behavior and nothing depends - on it yet, so a revert is cheap — not that the granularity survives. +- **Bisect and revert granularity is the PR.** A regression in `suspend`, in `status.placement`, or + in the corpus is one commit on `main`, and reverting any of them reverts all three. The mitigation + is that PR 1's only write-behavior change is the `Ambiguous` rule and nothing depends on the rest + of it yet, so a revert is cheap — not that the granularity survives. - **The changelog entry is the PR title.** release-please reads the squashed commit, so PR 1's title has to cover four things honestly rather than name the most interesting one. - **One property is untouched by the merge:** scenarios for unbuilt behavior are written in PR 1 and - skipped, naming PR 2 in the skip message, so PR 2 is still finished when the last skip is gone. - That is enforced by the test suite rather than by history, which is why squashing cannot erode it. + skipped, each naming the track that unskips it, so PR 2 is still finished when every skip naming + PR 2 is gone. Not all of them do: shape 8's `images:` authoring names track C and outlives PR 2, + which is why the rule is "PR 2's own skips" rather than "the last skip". Either way it is enforced + by the test suite rather than by history, which is why squashing cannot erode it. **Track C is not one of the three, on purpose.** It is one and a half to two weeks of engineering that blocks nothing; folding it into any of the three would make that PR unreviewable and would tie a @@ -168,15 +190,16 @@ So PR 1 is assembly, not construction: seed a worktree from `repository/`, build **Three rules for the corpus, each of which has already been learned the hard way here:** -- **Scenarios for unbuilt behavior are written now and skipped**, with the PR that unskips them named - in the skip message. PR 2 is finished when the last skip is gone. +- **Scenarios for unbuilt behavior are written now and skipped**, with the track that unskips them + named in the skip message. PR 2 is finished when every skip naming PR 2 is gone — shape 8's + `images:` authoring names track C and outlives it. - **`config/gittarget.yaml` parses into a harness-local struct** until PR 2 deletes that mapping — which is itself a check that the API the examples describe is the API that got built. -- **Refusals are fixtures too.** Every scenario that only ever succeeds is advertising rather than - specification. The set needs at least: `serializeNamespace: false` with no supplier, a second - source namespace against an explicit `false`, a folder covering two render roots, and a - base-owned field edit — each asserting an `expected-status.yaml` rather than a patch. Only the - two-roots one asserts a rule PR 1 ships; the rest are written in PR 1 and skipped until PR 2. +- **Refusals are fixtures too.** A set in which every scenario succeeds is advertising rather than + specification. Three: a second source namespace against an explicit `serializeNamespace: false`, a + folder covering two render roots, and a base-owned field edit — each asserting an + `expected-status.yaml` rather than a patch. Only the two-roots one asserts a rule PR 1 ships; the + second-namespace one is written in PR 1 and skipped until PR 2. ### The behavior reference this leaves missing diff --git a/docs/design/gittarget-api-wave.md b/docs/design/gittarget-api-wave.md index f2dcc1d5..141c0bef 100644 --- a/docs/design/gittarget-api-wave.md +++ b/docs/design/gittarget-api-wave.md @@ -79,21 +79,26 @@ can do none of the three it is probably not a `GitTarget` field. These are the reasons to combine, as opposed to merely batch. Each one changes what gets built. -### 1. Adoption is a dry run, and `spec.suspend` is enough to give it - -Placement only ever affects *new* documents, so there is nothing to preview by inspection: you find -out where files go by letting one be written. - -A suspended target plus `status.placement` is that preview. The operator scans, resolves the render -root, publishes what it *would* do, and writes nothing; clear `suspend` when the status says what you -expected. That needs no second field, which is why **`spec.mode: Observe|Write` (B1) was dropped**: -it bought only the difference between a temporary pause and a declared permanent read-only posture, -which is a distinction in intent, not in behavior. - -The cost is stated rather than hidden: **`suspend` must keep observing.** Flux's `suspend` stops -reconciliation altogether; ours stops *writes* and keeps scanning, so the status a user is waiting on -stays fresh while they wait. That deviation belongs in the field's documentation, in one sentence, -because it is the only place we differ from a convention a Flux user brings with them. +### 1. `spec.suspend` is a panic knob, and adoption is a scratch branch + +The review's central complaint is that *this controller writes to a Git repository and there is no +way to make it stop that is not deleting the object.* One field that stops the writes without +deleting the target or unpicking its `WatchRule` objects answers that, and that is the whole +justification — which is why **`spec.mode: Observe|Write` (B1) was dropped**: mode bought only the +difference between a temporary pause and a declared permanent read-only posture, a distinction in +intent rather than in behavior. + +`suspend` is not how a user previews a target. That is done by pointing a `GitTarget` at a scratch +branch and reading the commits it makes — real bytes, real registrations, real deletes, in a +reviewable diff +([`../layout/model.md`](../layout/model.md#previewing-a-target-point-it-at-a-scratch-branch)). + +The one deviation is stated rather than hidden: **`suspend` keeps observing.** Flux's `suspend` stops +reconciliation altogether; ours stops *writes* and keeps scanning, because a valve that stopped +looking as well as writing would freeze `status.placement` at whatever the folder looked like the +moment someone panicked — exactly when a stale answer costs the most. That belongs in the field's +documentation, in one sentence, because it is the only place we differ from a convention a Flux user +brings with them. **Re-open trigger for `mode`**: someone who needs a target that can never write, as a property of the object rather than a switch a colleague can flip. @@ -113,7 +118,7 @@ deletion; [`event_router.go`](../../internal/watch/event_router.go), What is left uncovered is one case: a repository whose folder was changed **by someone else** while our target wrote nothing. The reconcile-request annotation refreshes that on demand. A stale -`observedRevision` on an idle target is a legible cost; a periodic scan on every target is not. +`resolvedAtRevision` on an idle target is a legible cost; a periodic scan on every target is not. **Re-open trigger**: a user who needs an idle target's `status.placement` to track a repository other people edit, for whom the annotation is not enough. Then it is a scan cadence, named for scanning. @@ -211,9 +216,9 @@ status: message: "render root '.' governs new files" observedGeneration: 4 placement: + mode: KustomizeRoot renderRoot: . - serializeNamespace: false - observedRevision: 9f3c1ab + resolvedAtRevision: 9f3c1ab lastHandledReconcileAt: "2026-07-30T09:14:22Z" ``` @@ -275,9 +280,9 @@ Dependencies first, then the things that only need the object to be breaking. 1. **The `scope: Namespaced` envtest**, above. Not an API change; its answer constrains the enum work. Do it before planning. 2. **`spec.suspend`.** Precondition for anything that creates files, and independently the review's - highest-value gap. -3. **`status.placement`** plus the post-scan validation pass. A dry run with nothing to read previews - nothing. + highest-value gap: a way to stop the writes that is not deleting the object. +3. **`status.placement`** plus the post-scan validation pass. Independent of step 2 — it explains a + write that already happened, rather than previewing one that has not. 4. **`requestedAt` + `lastHandledReconcileAt`.** On-demand refresh of step 3. 5. **Events on a changed resolution**, over the existing recorder. 6. **B4**, as `spec.commit`. Last of the principle items, and the one that makes the object coherent. diff --git a/docs/design/open-asks-priority.md b/docs/design/open-asks-priority.md index 7dae3c0c..85876a02 100644 --- a/docs/design/open-asks-priority.md +++ b/docs/design/open-asks-priority.md @@ -243,7 +243,7 @@ and is not independently schedulable. | F6 | `spec.suspend`, `GitProvider.spec.interval`, `requestedAt` (no `interval` on `GitTarget`, see [`gittarget-api-wave.md`](gittarget-api-wave.md)) | maintainer review | **2** | wave | | 5 | `CommitRequest.spec.author`, SAR-guarded | gitops-api (#220) | **2** | wave | | B4 | `commitWindow` / `commit.message` move to GitTarget | config surface | **2** | wave | -| ~~B1~~ | ~~`GitTarget.spec.mode: Observe\|Write`~~ **dropped**: `suspend` on a still-scanning target is the same dry run with one field | config surface | — | [`gittarget-api-wave.md`](gittarget-api-wave.md) | +| ~~B1~~ | ~~`GitTarget.spec.mode: Observe\|Write`~~ **dropped**: `suspend` already stops the writes, and `mode` buys only a declared posture over a pause | config surface | — | [`gittarget-api-wave.md`](gittarget-api-wave.md) | | 6 | Movable destination via `status.observedDestination` | gitops-api (#220) | **2** | wave | | F10 | CommitRequest TTL / ownerRef + the `delete` verb | maintainer review | **2** | wave | | n/a | The blocking resolve is head-of-line on the shard goroutine | [`../spec/attribution.md`](../spec/attribution.md#the-wait) | **2** | — | @@ -507,9 +507,9 @@ carry, and an aggregated-API create is logged with no name and no response body `#220` shape — honored only against an admission record carrying an authorized verdict, fail-closed independent of the webhook's `failurePolicy` — remains the right one, on the first argument alone. -**B4, #6, F10** as written in their source documents. **B1 has left the wave**: a suspended -`GitTarget` that keeps scanning is the same dry run with one field instead of two, so `mode` buys -only the difference between a pause and a declared posture. The re-open trigger is in the wave +**B4, #6, F10** as written in their source documents. **B1 has left the wave**: `suspend` already +stops a target writing without deleting it, so `mode` buys only the difference between a pause and a +declared permanent posture — a distinction in intent, not in behavior. The re-open trigger is in the wave document. #6 is explicitly a lower priority than when it was filed: the consumer downgraded it themselves, because branch and folder are now chosen once per repository on an object that exists because the user picked that repository. diff --git a/docs/design/placement-visibility-and-declared-defaults.md b/docs/design/placement-visibility-and-declared-defaults.md index 6f1da6ba..ef2b19ba 100644 --- a/docs/design/placement-visibility-and-declared-defaults.md +++ b/docs/design/placement-visibility-and-declared-defaults.md @@ -256,9 +256,16 @@ nothing built now is wasted. ## `status.layout`, renamed `status.placement` -> **The field is `status.placement` now.** [`model.md`](../layout/model.md) renamed it when -> `spec.layout` stopped existing, and specifies it under that name. The shape below is unchanged -> and this page is still where it is argued; read every `status.layout` here as `status.placement`. +> **The field is `status.placement` now, and it is much smaller than the shape below.** +> [`model.md`](../layout/model.md) renamed it when `spec.layout` stopped existing, and is the +> specification of record. What it kept from this page is the *principle* — an observation, not a +> condition, bounded rather than per-type. What it dropped is most of the shape: the historical half +> (`placedResources`, `canonicalTypes`, `refusedResources`) went to metrics, `examples` went because +> a fabricated object at a fabricated path is not an answer, `declaredTypes` went because it counts a +> spec map the same GET returns, and `renderRootReason` became the `LayoutResolved` condition's +> reason. `status.placement` now publishes `mode`, `renderRoot`, `readOnlyBases`, +> `resolvedAtRevision` and `resolvedAt`, and nothing else. Read the shape below as the argument that +> produced it, not as the field. An **observation, not a condition**, in the sense [`GitTargetStatus.Retention`](../../api/v1alpha3/gittarget_types.go) already establishes: nothing diff --git a/docs/layout/model.md b/docs/layout/model.md index 1a5a3c47..dbcfd0d0 100644 --- a/docs/layout/model.md +++ b/docs/layout/model.md @@ -190,36 +190,42 @@ resolves each subtree correctly without anyone declaring anything. So the uniform claim is what an explicit setting is *for*, and the non-uniform folder is what unset is for. That is also why unset cannot be spelled `false`. -### The guard on `false`, and why it is a post-scan check - -`false` where nothing supplies the namespace hands the object to whatever namespace the applier -happens to be pointed at, which is a different object with the same name. It is honest only when -something guarantees the namespace, and where the guarantee is a `kustomization.yaml` **the user -owns**, they can delete one line from their own file and every subsequent document silently -relocates. - -That precondition is a property of the observed folder, not of the spec, so no CEL rule can check it. -It is one post-scan rule, on the scan that already runs, setting `Validated=False` with a message -naming the field and what the folder actually contains. With `useKustomize: true` the rule is -satisfied by construction, because the operator wrote the supplier. - -**Nothing here is the last line of defence, which is why the guard can be a report rather than a -refusal.** The render check at the write path already refuses a write whose document does not render -to the live object, and it holds for both shapes where the store's view and kustomize's disagree: a -document naming a namespace the governing transformer overrides, and a folder whose nested roots both -assign one. Both are measured, not assumed, and both are pinned by +### Why `false` needs no guard + +`serializeNamespace: false` is not checked against the folder, and it cannot be: **the supplier of +the namespace lives outside the repository, and there may not be a single one.** + +For a raw namespace-free folder — [shape 2](shapes/2-flat-namespace-free/README.md) and +[shape 4](shapes/4-tree-namespace-free/README.md) — the supplier is a Flux +`Kustomization.spec.targetNamespace` or an Argo `Application.spec.destination.namespace`, in a +different cluster from the repository. Being unbound that way **is the point of the shape**: anyone +may point a deployer at that folder and land it wherever they choose, and two deployers may +correctly land the same folder in two different namespaces. A rule demanding that something in the +folder supply the namespace would therefore report a fault on a folder doing exactly what it was +built to do, and a field naming the supplier would ask the user to promise something that is not +theirs to promise. Neither exists. + +Nothing is lost by that, because none of it was ever the last line of defence. The render check at +the write path already refuses a write whose document does not render to the live object, and it +holds for both shapes where the store's view and kustomize's disagree: a document naming a namespace +the governing transformer overrides, and a folder whose nested roots both assign one. Both are +measured, not assumed, and both are pinned by [`namespace_context_refusal_test.go`](../../internal/git/namespace_context_refusal_test.go) with the -read-side halves in the contextual-namespace corpus. What the post-scan rule adds is not safety but -legibility: today those failures surface as an opaque render error rather than as the one fixable -thing that is wrong. +read-side halves in the contextual-namespace corpus. + +The division this draws runs through the whole model: **guard what is inside the folder, say nothing +about what happens after it leaves.** The one-source-namespace rule below is on the inside of that +line — two namespaces collapsing onto one namespace-free document is a loss the operator can see, in +the folder it owns — and it refuses. It also reaches status, which publishes no supplier and no +`serializeNamespace`: see [`status.placement`](#statusplacement-and-the-post-scan-pass). ### The second guard: one source namespace, and this one refuses **A `GitTarget` with an explicit `serializeNamespace: false` admits exactly one source namespace. The second is refused.** -The guard above asks whether a supplier exists. This one asks how many namespaces that supplier is -being asked to speak for, and the answer can only ever be one: a document with no +The question is not whether a supplier exists — that is unanswerable, above — but how many namespaces +one supplier can be asked to speak for, and the answer can only ever be one: a document with no `metadata.namespace` takes its namespace from a single supplier, so two source namespaces reaching the folder is a contradiction in the setting itself. What follows is not a collision but a **match** — `shop/config` and `billing/config` both resolve to a `config.yaml` whose bytes carry no namespace, so @@ -227,12 +233,10 @@ their manifest identities are equal, the bundling rule never fires, and each wri between two live objects. Everywhere else in this model, losing a distinction produces a refusal or a bundle; only here does it produce a match, which is why this guard refuses where the other reports. -It is **derived from one setting, not inferred from two.** An earlier draft proposed a separate -`enforceSingleNamespace` boolean, on the objection that a refusal derived from other fields is the -kind of inference that got deleted from placement. That objection applied to a rule keyed on -`serializeNamespace: false` *plus* a template with no `{namespace}`. Drop the template half — the -path is irrelevant, because a deployer applies bytes rather than filenames — and what is left is the -field's own meaning rather than a correlation between two fields. So there is no new field. +It is **derived from one setting, not inferred from two**, which is why it needs no +`enforceSingleNamespace` boolean of its own. The path plays no part: a deployer applies bytes rather +than filenames, so the rule keys on the field's own meaning rather than on a correlation between +`serializeNamespace` and a template that happens to omit `{namespace}`. **Explicit `false` only. Inference is never constrained by it.** That asymmetry is what makes the refusal safe next to kustomize, and it is the same line [`Is a folder-wide claim @@ -249,9 +253,9 @@ operator can write no `namespace:` at all, so it creates a root that supplies no namespace-less documents beneath it — the operator actively constructing the silent-mislabel folder. This is the one place refusing is not merely permissible but the only defensible behavior. -**It needs no scan and no repository state**, which is what separates it from the guard above. The -set of source namespaces reaching a target is `{the target's own namespace} ∪ {the explicit -rules[].sourceNamespace names of every WatchRule pointing at it}`, all of it in the config cluster. +**It needs no scan and no repository state.** The set of source namespaces reaching a target is +`{the target's own namespace} ∪ {the explicit rules[].sourceNamespace names of every WatchRule +pointing at it}`, all of it in the config cluster. A `sourceNamespace: "*"` item is refused outright and statically, with no enumeration, and that holds under **either** reading of `*` — the shipped one or the one the wave replaces it with ([definition of record](../design/source-scope-simplification.md#sourcenamespace--needs-its-own-decision)). @@ -320,12 +324,41 @@ change at all. that answered rather than a resolved layout kind, so nothing here breaks a label. - **`{kindLower}` and the versionless identity fix** are template features and stay queued. +## Previewing a target: point it at a scratch branch + +Placement only ever affects *new* documents, so there is nothing to preview by inspecting the +folder. The way to see what a target would do is to let one do it, somewhere harmless: + +```yaml +spec: + providerRef: {name: homelab} + branch: gitops-preview # not main + path: apps/checkout +``` + +It commits. You read the commits. That is the actual bytes, the actual `resources:` registrations, +the actual `$patch: delete` files, in a diff you can review and hand to someone else. It costs one +field value, needs nothing built, and the branch is disposable. + +`spec.branch` is immutable ([the destination fields are](#what-it-leaves-standing)), so the shape of +this is *declare a preview target, look, delete it, declare the real one* — not flip a branch on a +live target. That is a feature: the preview target and the real one are different objects, and +deleting the preview cannot disturb the real folder. For inspecting a repository with no cluster at +all, the manifest-analyzer CLI is the other half of the answer. + +Two things follow, and they are what keeps the rest of this proposal small: + +- **`spec.suspend` is a panic knob.** One field that stops a target writing without deleting it or + unpicking the watch configuration that would have to be rebuilt afterwards. It is not a preview + mechanism: a target that writes nothing has nothing to show. It still **scans** while suspended, + for a different reason — a valve that stopped looking as well as writing would freeze + `status.placement` at whatever the folder looked like the moment someone panicked, which is + exactly when a stale answer costs the most. +- **`status.placement` explains rather than predicts.** See below. + ## `status.placement`, and the post-scan pass -The legibility gap is the one surviving argument from the earlier thesis that no field answers, and -it is worth building **before** either flag: placement only ever affects *new* documents, so there is -nothing to preview by inspection, and a suspended target plus this stanza is what turns adoption from -declare-and-hope into a dry run. +The stanza has one job: **explain why a write took the shape it did, or why it was refused.** ```yaml status: @@ -334,45 +367,68 @@ status: - type: LayoutResolved status: "True" reason: SingleKustomization # SingleKustomization | Ambiguous | None - message: "render root '.' governs new files" + message: 'render root "." governs new files; it renders ../../base, which is read-only input' observedGeneration: 4 placement: + mode: KustomizeOverlay # Plain | KustomizeRoot | KustomizeOverlay renderRoot: . - serializeNamespace: false # what it resolved to for this folder - byTypeEntries: 1 - observedRevision: 9f3c1ab - observedTime: "2026-07-30T09:14:22Z" - examples: [] # capped at three, illustrative, not a tally + readOnlyBases: ["../../base"] # non-empty exactly when mode is KustomizeOverlay + resolvedAtRevision: 9f3c1ab + resolvedAt: "2026-07-30T09:14:22Z" ``` -Three decisions are taken here rather than deferred, because each is cheaper to take before the field -exists than after: +**The rule that decides what belongs here**, and the one to hold a proposed field against: a status +field earns its place only if a reader **cannot get it from the spec** in the same GET, **and** it +**varies with this folder**. A copy of `spec.serializeNamespace`, a count of the `byType` map, and a +list of illustrative destinations for a fabricated object all fail it, and none of them is here. + +**`mode` is the field a reader cannot work out for themselves.** Whether a folder is written as +plain files, as a self-contained kustomize root, or as an overlay over a base it may not write to is +a fact about the *repository*, and it predicts every behaviour that surprises people: + +| | `Plain` | `KustomizeRoot` | `KustomizeOverlay` | +|---|---|---|---| +| a new document | written | written **and registered** in `resources:` | same | +| a delete | file removed | file removed **and its `resources:` entry dropped** | same, unless the object is inherited | +| deleting an object the folder inherits | n/a | n/a | a **`$patch: delete` is authored into the overlay**; nothing is removed | +| editing a field the base owns | n/a | n/a | authored into the overlay for `images:`/`replicas:`, **refused** otherwise | +| can kustomize itself refuse the write | no | yes, via the re-render oracle | yes | + +Those behaviours are **constants of the mode**, so they are documented on the field and not +enumerated per folder in status. `readOnlyBases` is the exception that is genuinely per-folder: it +names the directories a `WriteBoundaryRefused` will fire on, which turns that refusal from a +surprise into something a reader could have predicted. `mode` is absent under `Ambiguous`, along +with `renderRoot`: there is no single answer, and naming one of several roots would be the guess +that verdict exists to refuse. + +Three decisions are taken here rather than deferred, because each is cheaper to take before the +field ships: - **The resolution reason is a condition reason, not a field.** `renderRootReason` would have been a reason enum in a bespoke field, and every consumer in this ecosystem already reads reasons from `conditions`. -- **No accumulating counters.** `placedResources`, `overriddenTypes` and `refusedResources` are - metrics; `placements_total` carries them with better labels. A monotonic counter in status is a - status write per event, which re-creates the self-triggering reconcile edge the status work already - fixed once. `examples` stays, capped and fixed-size, because "show me where a Secret would land" is - not a metric. -- **`conditions` and `observedGeneration` are in the stanza**, because every scenario README already - asserts `Ready=True`. - -The current half must never depend on a placement having happened: `renderRoot` is a fact about the -folder from the last scan, available before anything is ever written. - -**The post-scan validation pass ships with it**, because it is the same scan. Two rules today, whose -precondition is a property of the observed folder rather than of the spec, so no CEL rule can reach -them: `serializeNamespace: false` requires a namespace supplier, and a folder covering two roots is -`Ambiguous` rather than silently picking one. One pass, one condition shape, `Validated=False` naming -the offending field and what the folder actually contains. +- **No counters.** `placedResources`, `overriddenTypes` and `refusedResources` are metrics; + `placements_total` carries them with better labels. A counter in status is a status write per + event, which re-creates the self-triggering reconcile edge the status work already fixed once. +- **`resolvedAtRevision`/`resolvedAt` date the RESOLUTION, not the last scan.** An unchanged + resolution is not republished, so a timestamp well in the past means the folder's shape has been + stable rather than that scanning stopped. The names say so, because `observed*` would read as + "last looked". + +**The post-scan validation pass ships with the stanza**, because it is the same scan, and it is one +rule: a folder covering two render roots is `Ambiguous` rather than silently picking one. There is +no supplier rule — [`false` needs no guard](#why-false-needs-no-guard). The [one-source-namespace rule](#the-second-guard-one-source-namespace-and-this-one-refuses) is **not** part of this pass, though it guards the same field. Its input is the set of `WatchRule` objects naming the target, which is in the config cluster and needs no scan, and its outcome is a refusal rather than a report. It ships with the field, in PR 2. +While a target is suspended, `status.retention` is **not published at all**. The resync stops before +the mark-and-sweep, so nothing is swept and nothing is counted, and a published zero would read as +"converged" when it means "not measured". Absent already means "no resync has reported", which is +exactly the truth. + ## How it gets built `LocateNew` is not rewritten. The four-rung ladder is a single function, @@ -390,12 +446,20 @@ authoring. Nothing below waits for either — see | PR | Content | Breaking | |---|---|---| | 1 | The worked examples as an executable corpus, `spec.suspend` and the reconcile-request annotation, `status.placement`, and the post-scan pass's `Ambiguous` rule | no | -| 2 | `useKustomize` and `serializeNamespace`, with the post-scan pass's supplier rule and the one-source-namespace refusal ([#322](https://github.com/ConfigButler/gitops-reverser/issues/322)) | no | +| 2 | `useKustomize` and `serializeNamespace`, with the one-source-namespace refusal ([#322](https://github.com/ConfigButler/gitops-reverser/issues/322)) | no | + +PR 1's four parts are one review because the corpus is what proves the other three: `spec.suspend`, +`status.placement` and the reconcile-request annotation are each small, and each is only credible +against a worked example that pins what the folder actually does. The exception is the `Ambiguous` +rule, which **gates**: a folder covering several render roots stops placing new documents, where +before it placed them at the canonical path inside whichever folder it covered. Existing documents +are untouched, and the refusal is raised at the +write rather than on `Validated` so the target keeps scanning and can observe the folder being +fixed. [`../design/build-order.md`](../design/build-order.md#the-plan-as-three-prs) carries the +before-and-after. -PR 1's four parts are one review, because none of them changes what the operator writes: a suspended -target that publishes what it resolved is the whole feature, and the corpus is what proves it. The -post-scan pass is the one thing that does **not** land whole — its `Ambiguous` rule reads only the -scan, while its supplier rule reads a field PR 2 introduces. +The post-scan pass lands whole in PR 1: it is the `Ambiguous` rule and nothing else, and that rule +reads only the scan. Neither PR is breaking, so neither waits for a coordinated consumer bump. What is breaking on `GitTarget` is unrelated to placement and is sequenced in @@ -411,7 +475,9 @@ scenario: seed a worktree from `repository/`, build the write event from `input/ policy from `config/gittarget.yaml`, flush, and compare the normalized diff with `expected-*.patch`. Blob hashes and index lines are noise; a `-update` flag that rewrites the patches keeps the corpus cheap to extend. Scenarios describing behavior PR 2 introduces are written now and skipped with the -PR that unskips them named in the skip message, so **PR 2 is finished when the last skip is gone.** +PR that unskips them named in the skip message, so **PR 2 is finished when its own skips are +gone.** Not every skip is PR 2's: shape 8's `images:` authoring belongs to track C and is skipped +naming it, so it stays after PR 2 lands and is not a defect in that PR's completion. `config/gittarget.yaml` uses fields that do not exist yet, so it parses into a harness-local struct until PR 2 deletes that mapping — which is itself a check that the API the examples describe is the API that got built. @@ -430,18 +496,14 @@ feedback half and can follow in the same PR. It also decides the `useKustomize` created root's `namespace:` is written when the folder is single-namespace, and under this rule an explicit `serializeNamespace: false` guarantees it is. -Two gaps the corpus should fill in PR 1: **a refusal scenario** (every example is a happy path, and -the post-scan pass has the least coverage — a `serializeNamespace: false` with no supplier, a -`serializeNamespace: false` target that a second source namespace reaches, and a -folder covering two roots, each asserting `expected-status.yaml` instead of a patch), and **the -missing `ClusterProvider`** that `empty-repo-bootstrap` references as `clusterProviderRef: app-intent` -without a specimen existing anywhere. +**Refusals are fixtures too**, and they are part of PR 1 even where the rule is not: a set of worked +examples in which every write succeeds is advertising rather than specification. Three assert an +`expected-*-status.yaml` instead of a patch — a `serializeNamespace: false` target a second source +namespace reaches, a folder covering two roots, and the base-owned field edit. Only the two-roots +one asserts a rule PR 1 ships; the second-namespace one is written and skipped naming PR 2. ## Open questions -- Does `serializeNamespace: false` need to **name** its supplier (`KustomizeRoot`, - `FluxTargetNamespace`, `Asserted`) so the post-scan pass can check the guarantee rather than infer - which one was meant? - Should a `useKustomize: true` folder create a **nested** root per directory the template writes into, each carrying its own `namespace:`? Fact 2 proves it works. Deferred, and now more firmly: it was the thing that would have made `serializeNamespace: false` safe in a multi-namespace tree, @@ -451,7 +513,8 @@ without a specimen existing anywhere. whose documents omit their namespaces, and for whom leaving the field unset is not enough. - Should the operator ever **refuse** a write when a root that used to govern the path is gone, rather than reporting `LayoutResolved: None` and carrying on? Report first; escalate if someone - says the status was not enough. + says the status was not enough. Such a folder is also a `mode` transition (`KustomizeRoot` to + `Plain`), which republishes, so the change is already visible without a refusal. - Should `placement.default` gain a CRD default now that a defaulted template no longer produces unrendered files? The remaining objection is legibility, not correctness. - Namespace-local `GitProvider`: the homelab examples put a `GitTarget` in `argocd`, `flux-system` diff --git a/docs/layout/shapes/1-flat-serialized/README.md b/docs/layout/shapes/1-flat-serialized/README.md index 193d2323..7f101515 100644 --- a/docs/layout/shapes/1-flat-serialized/README.md +++ b/docs/layout/shapes/1-flat-serialized/README.md @@ -6,7 +6,8 @@ set that needs no `-R` and no deployer to be correct. ## Starting repository -[`repository/`](repository/) **is** `mirror/prod`, holding two namespaces already: +[`repository/`](repository/) is the repository root. The target's path, `mirror/prod`, already +holds two namespaces: ```text mirror/prod/ @@ -35,8 +36,7 @@ being self-describing. The flag makes that a folder property rather than a conse - Starting repository: [`repository/`](repository/). - Live input: [`input/checkout-config.yaml`](input/checkout-config.yaml). -- Expected Git change: [`expected-checkout-config.patch`](expected-checkout-config.patch), after a - reviewer clears `suspend`. +- Expected Git change: [`expected-checkout-config.patch`](expected-checkout-config.patch). - Expected status: `Ready=True`, `LayoutResolved` reason `None` — there is no render root, and the message says so rather than leaving the field unexplained. diff --git a/docs/layout/shapes/1-flat-serialized/config/gittarget.yaml b/docs/layout/shapes/1-flat-serialized/config/gittarget.yaml index 80cae737..db66bd81 100644 --- a/docs/layout/shapes/1-flat-serialized/config/gittarget.yaml +++ b/docs/layout/shapes/1-flat-serialized/config/gittarget.yaml @@ -10,7 +10,6 @@ spec: name: prod branch: main path: mirror/prod - suspend: true # adoption dry run: scans and publishes, writes nothing # The WatchRule beside this file names source namespaces other than its own, so # the folder owner must say which ones may arrive here. Without it the rule is # refused (SourceNamespaceAuthorized=False) — the deny-by-default posture. diff --git a/docs/layout/shapes/1-flat-serialized/expected-checkout-config.patch b/docs/layout/shapes/1-flat-serialized/expected-checkout-config.patch index 76d3683f..40642364 100644 --- a/docs/layout/shapes/1-flat-serialized/expected-checkout-config.patch +++ b/docs/layout/shapes/1-flat-serialized/expected-checkout-config.patch @@ -9,4 +9,4 @@ new file mode 100644 + name: checkout-config + namespace: shop +data: -+ timeout: "15m" ++ timeout: 15m diff --git a/docs/layout/shapes/1-flat-serialized/repository/billing-invoices.yaml b/docs/layout/shapes/1-flat-serialized/repository/mirror/prod/billing-invoices.yaml similarity index 100% rename from docs/layout/shapes/1-flat-serialized/repository/billing-invoices.yaml rename to docs/layout/shapes/1-flat-serialized/repository/mirror/prod/billing-invoices.yaml diff --git a/docs/layout/shapes/1-flat-serialized/repository/shop-web.yaml b/docs/layout/shapes/1-flat-serialized/repository/mirror/prod/shop-web.yaml similarity index 100% rename from docs/layout/shapes/1-flat-serialized/repository/shop-web.yaml rename to docs/layout/shapes/1-flat-serialized/repository/mirror/prod/shop-web.yaml diff --git a/docs/layout/shapes/2-flat-namespace-free/README.md b/docs/layout/shapes/2-flat-namespace-free/README.md index d2db34a5..a591120a 100644 --- a/docs/layout/shapes/2-flat-namespace-free/README.md +++ b/docs/layout/shapes/2-flat-namespace-free/README.md @@ -31,20 +31,26 @@ supplies what the documents omit. - Live input: [`input/checkout-config.yaml`](input/checkout-config.yaml) — a `ConfigMap` in `shop`. - Expected Git change: [`expected-checkout-config.patch`](expected-checkout-config.patch). The input's `namespace: shop` does not appear in it. That subtraction is the assertion. -- Expected status: `Ready=True`, and see the guard below. +- Expected status: `Ready=True`, and nothing is reported about the missing supplier. Why not is the + next section. -## The guard has nothing to check +## There is no guard, because there is nothing to check -`serializeNamespace: false` is honest only when something guarantees the namespace, and the post-scan -pass re-checks that on every scan by looking at the folder. Here the guarantee is a `Kustomization` -in the deploying cluster, which the operator cannot see, so a guard that only accepts a -`kustomization.yaml` reports `Validated=False` against a perfectly correct folder. +`serializeNamespace: false` is not checked against this folder, and it cannot be. The guarantee is a +`Kustomization` in the deploying cluster, which the operator cannot see — so a rule requiring +*something in the folder* to supply the namespace would report a fault against a perfectly correct +folder. -That is the strongest argument for -[naming the supplier](../../model.md#open-questions): a `false` that says *"external, asserted"* -moves the responsibility to the user explicitly, instead of leaving the operator to choose between a -false alarm and no check at all. Until that exists, this shape is the one where the guard has to -stay a report rather than a refusal. +Naming the supplier instead (a `false` that also declares *"external, asserted"*) does not help +either, and for a stronger reason: **there is often no single supplier to name.** A raw +namespace-free folder can be consumed by two deployers into two different namespaces, both +correctly. That portability is what the shape is *for*. An assertion field would ask the user to +promise something that is not theirs to promise, and buy nothing, since nothing could check it +either way. + +What is left is a division the rest of the model runs along: **guard what is inside the folder, say +nothing about what happens after it leaves.** The next section is a rule on the inside of that line, +and it is enforced. ## What if two namespaces reach this target? @@ -74,6 +80,13 @@ with the field, in PR 2. Unlike the supplier question above, it is answerable en cluster: the set of source namespaces reaching a target comes from the rules that name it, not from the folder — so this shape gets a real fence even though its *supplier* stays unverifiable. +It is a fixture rather than only an argument. +[`config/gittarget-second-namespace.yaml`](config/gittarget-second-namespace.yaml) and +[`config/watchrule-second-namespace.yaml`](config/watchrule-second-namespace.yaml) are this folder +with the mistake made, and +[`expected-second-namespace-status.yaml`](expected-second-namespace-status.yaml) is the refusal. +The corpus runs it and skips it, naming PR 2. + ## Empty folder **This is the case the refactor exists for.** An empty `apps/checkout` supplies nothing to infer diff --git a/docs/layout/shapes/2-flat-namespace-free/config/gittarget-second-namespace.yaml b/docs/layout/shapes/2-flat-namespace-free/config/gittarget-second-namespace.yaml new file mode 100644 index 00000000..c6910dc2 --- /dev/null +++ b/docs/layout/shapes/2-flat-namespace-free/config/gittarget-second-namespace.yaml @@ -0,0 +1,28 @@ +# The fence around "one namespace", as a fixture. Same folder and same flag as +# gittarget.yaml; what differs is that TWO WatchRules reach it, from two source namespaces. +# +# An explicit `serializeNamespace: false` admits exactly one source namespace, and the second is +# refused. The reason is not a collision but a MATCH: shop/checkout-config and +# billing/checkout-config both resolve to a checkout-config.yaml whose bytes carry no namespace, +# so their manifest identities are equal, the bundling rule never fires, and each write flips one +# document between two live objects. See ../../model.md, "The second guard". +apiVersion: configbutler.ai/v1alpha3 +kind: GitTarget +metadata: + name: checkout-artifact + namespace: shop +spec: + providerRef: + name: artifacts-repository + clusterProviderRef: + name: apps + branch: main + path: apps/checkout + allowedSourceNamespaces: + names: [shop, billing] + placement: + default: "{name}.yaml" + # The declaration the rule binds to. Unset would be untouched by it: inference resolves each + # document against the root governing its own path, so a legitimately multi-namespace folder + # must use unset rather than false, and the refusal is what pushes it there. + serializeNamespace: false diff --git a/docs/layout/shapes/2-flat-namespace-free/config/gittarget.yaml b/docs/layout/shapes/2-flat-namespace-free/config/gittarget.yaml index 33d87753..1e8a61a3 100644 --- a/docs/layout/shapes/2-flat-namespace-free/config/gittarget.yaml +++ b/docs/layout/shapes/2-flat-namespace-free/config/gittarget.yaml @@ -8,7 +8,6 @@ spec: name: artifacts-repository branch: main path: apps/checkout - suspend: true # adoption dry run: scans and publishes, writes nothing # One file per object, flat, and no {namespace} in the template: the folder is # single-namespace by construction. placement: diff --git a/docs/layout/shapes/2-flat-namespace-free/config/watchrule-second-namespace.yaml b/docs/layout/shapes/2-flat-namespace-free/config/watchrule-second-namespace.yaml new file mode 100644 index 00000000..226dd1db --- /dev/null +++ b/docs/layout/shapes/2-flat-namespace-free/config/watchrule-second-namespace.yaml @@ -0,0 +1,15 @@ +# The second rule, and the mistake. Its sourceNamespace names a namespace other than the target's +# own, so a second namespace's ConfigMaps arrive in a folder whose documents carry no namespace. +apiVersion: configbutler.ai/v1alpha3 +kind: WatchRule +metadata: + name: checkout-content-billing + namespace: shop +spec: + targetRef: + name: checkout-artifact + rules: + - apiGroups: [""] + apiVersions: ["v1"] + resources: ["configmaps"] + sourceNamespace: billing diff --git a/docs/layout/shapes/2-flat-namespace-free/expected-second-namespace-status.yaml b/docs/layout/shapes/2-flat-namespace-free/expected-second-namespace-status.yaml new file mode 100644 index 00000000..c4ee582b --- /dev/null +++ b/docs/layout/shapes/2-flat-namespace-free/expected-second-namespace-status.yaml @@ -0,0 +1,19 @@ +# No patch: the write is refused before a byte moves, because the document it would place cannot +# be told apart from the one the other source namespace already owns. +# +# The refusal has two subjects, and both are deliberate: an admission check on the second +# WatchRule is the feedback at the moment the mistake is made, and this write-plan precondition is +# the correctness layer, because admission is one-shot and cannot see a serializeNamespace flipped +# to false after the rules were created. +status: + conditions: + - type: GitPathAccepted + status: "False" + reason: MultipleSourceNamespaces + message: >- + spec.serializeNamespace is false, which admits exactly one source namespace, but + [billing shop] reach this target; set serializeNamespace to unset so each document takes + the namespace of the root governing it, or split the target + - type: Stalled + status: "True" + reason: MultipleSourceNamespaces diff --git a/docs/layout/shapes/2-flat-namespace-free/repository/web.yaml b/docs/layout/shapes/2-flat-namespace-free/repository/apps/checkout/web.yaml similarity index 100% rename from docs/layout/shapes/2-flat-namespace-free/repository/web.yaml rename to docs/layout/shapes/2-flat-namespace-free/repository/apps/checkout/web.yaml diff --git a/docs/layout/shapes/3-tree-serialized/README.md b/docs/layout/shapes/3-tree-serialized/README.md index 1c87d9ff..842789cd 100644 --- a/docs/layout/shapes/3-tree-serialized/README.md +++ b/docs/layout/shapes/3-tree-serialized/README.md @@ -10,7 +10,7 @@ what is in a cluster. ## Starting repository -[`repository/`](repository/) **is** `clusters/home`: +[`repository/`](repository/) is the repository root, and the target's path is `clusters/home`: ```text clusters/home/ diff --git a/docs/layout/shapes/3-tree-serialized/config/gittarget.yaml b/docs/layout/shapes/3-tree-serialized/config/gittarget.yaml index 9cdce49c..1ce0c4f8 100644 --- a/docs/layout/shapes/3-tree-serialized/config/gittarget.yaml +++ b/docs/layout/shapes/3-tree-serialized/config/gittarget.yaml @@ -10,7 +10,6 @@ spec: name: home branch: main path: clusters/home - suspend: true # adoption dry run: scans and publishes, writes nothing # The WatchRule beside this file names source namespaces other than its own, so # the folder owner must say which ones may arrive here. Without it the rule is # refused (SourceNamespaceAuthorized=False) — the deny-by-default posture. diff --git a/docs/layout/shapes/3-tree-serialized/expected-checkout-config.patch b/docs/layout/shapes/3-tree-serialized/expected-checkout-config.patch index e2d63bfb..c0266047 100644 --- a/docs/layout/shapes/3-tree-serialized/expected-checkout-config.patch +++ b/docs/layout/shapes/3-tree-serialized/expected-checkout-config.patch @@ -9,4 +9,4 @@ new file mode 100644 + name: checkout-config + namespace: shop +data: -+ timeout: "15m" ++ timeout: 15m diff --git a/docs/layout/shapes/3-tree-serialized/repository/_cluster/rbac.authorization.k8s.io/clusterroles/homelab-viewer.yaml b/docs/layout/shapes/3-tree-serialized/repository/clusters/home/_cluster/rbac.authorization.k8s.io/clusterroles/homelab-viewer.yaml similarity index 100% rename from docs/layout/shapes/3-tree-serialized/repository/_cluster/rbac.authorization.k8s.io/clusterroles/homelab-viewer.yaml rename to docs/layout/shapes/3-tree-serialized/repository/clusters/home/_cluster/rbac.authorization.k8s.io/clusterroles/homelab-viewer.yaml diff --git a/docs/layout/shapes/3-tree-serialized/repository/billing/configmaps/invoices.yaml b/docs/layout/shapes/3-tree-serialized/repository/clusters/home/billing/configmaps/invoices.yaml similarity index 100% rename from docs/layout/shapes/3-tree-serialized/repository/billing/configmaps/invoices.yaml rename to docs/layout/shapes/3-tree-serialized/repository/clusters/home/billing/configmaps/invoices.yaml diff --git a/docs/layout/shapes/3-tree-serialized/repository/shop/apps/deployments/web.yaml b/docs/layout/shapes/3-tree-serialized/repository/clusters/home/shop/apps/deployments/web.yaml similarity index 100% rename from docs/layout/shapes/3-tree-serialized/repository/shop/apps/deployments/web.yaml rename to docs/layout/shapes/3-tree-serialized/repository/clusters/home/shop/apps/deployments/web.yaml diff --git a/docs/layout/shapes/4-tree-namespace-free/README.md b/docs/layout/shapes/4-tree-namespace-free/README.md index 185edf6f..676e65d5 100644 --- a/docs/layout/shapes/4-tree-namespace-free/README.md +++ b/docs/layout/shapes/4-tree-namespace-free/README.md @@ -54,11 +54,24 @@ Two ways out, and this scenario takes the first: - Live input: [`input/checkout-config.yaml`](input/checkout-config.yaml). - Expected Git change: [`expected-checkout-config.patch`](expected-checkout-config.patch) — a new file at `configmaps/checkout-config.yaml` with no namespace in it. -- Expected status: `Ready=True`, with the same unverifiable-supplier caveat as - [shape 2](../2-flat-namespace-free/README.md). +- Expected status: `Ready=True`. Nothing complains about the missing supplier, and that is the + decided behaviour rather than a gap — see below. + +**The supplier is unknowable, and the operator says nothing about it.** This folder is *correct*, +and the configuration that supplies its namespace is a Flux `targetNamespace` or an Argo +`destination.namespace` in another cluster. Two deployers may point at this folder and land it in +two different namespaces, both correctly — being unbound is what the shape is for, so nothing here +reports on it. See +[`model.md`](../../model.md#why-false-needs-no-guard). + +The rule that *is* enforced is on the inside of the folder: +[one source namespace](../../model.md#the-second-guard-one-source-namespace-and-this-one-refuses), +where two namespaces would collapse onto one namespace-free document. That loss is visible in the +folder the operator owns, so it is refused. ## Empty folder **Same gap as shape 2, and the same answer.** Nothing to infer from, so `serializeNamespace: false` has to be declared, and no scan of the folder can confirm it was right. The supplier is in another -cluster. +cluster — and since no scan could confirm it in a *populated* folder either, the empty folder is not +a special case here. It is the general one. diff --git a/docs/layout/shapes/4-tree-namespace-free/config/gittarget.yaml b/docs/layout/shapes/4-tree-namespace-free/config/gittarget.yaml index bc9f7ce3..94acd828 100644 --- a/docs/layout/shapes/4-tree-namespace-free/config/gittarget.yaml +++ b/docs/layout/shapes/4-tree-namespace-free/config/gittarget.yaml @@ -8,7 +8,6 @@ spec: name: artifacts-repository branch: main path: apps/checkout - suspend: true # adoption dry run: scans and publishes, writes nothing # A tree WITHOUT {namespace}: the subfolders group by type, not by namespace, # so the folder stays single-namespace. That is the precondition for the flag # below; see the README for what a MULTI-namespace tree does to it. diff --git a/docs/layout/shapes/4-tree-namespace-free/repository/configmaps/web.yaml b/docs/layout/shapes/4-tree-namespace-free/repository/apps/checkout/configmaps/web.yaml similarity index 100% rename from docs/layout/shapes/4-tree-namespace-free/repository/configmaps/web.yaml rename to docs/layout/shapes/4-tree-namespace-free/repository/apps/checkout/configmaps/web.yaml diff --git a/docs/layout/shapes/4-tree-namespace-free/repository/deployments/web.yaml b/docs/layout/shapes/4-tree-namespace-free/repository/apps/checkout/deployments/web.yaml similarity index 100% rename from docs/layout/shapes/4-tree-namespace-free/repository/deployments/web.yaml rename to docs/layout/shapes/4-tree-namespace-free/repository/apps/checkout/deployments/web.yaml diff --git a/docs/layout/shapes/5-kustomize-single-folder/README.md b/docs/layout/shapes/5-kustomize-single-folder/README.md index 88b79ba9..de722d78 100644 --- a/docs/layout/shapes/5-kustomize-single-folder/README.md +++ b/docs/layout/shapes/5-kustomize-single-folder/README.md @@ -6,7 +6,8 @@ creation differ by two lines of spec. ## Adopting an existing folder -[`repository/`](repository/) is `apps/checkout` with a root that already supplies `namespace: shop`: +[`repository/`](repository/) is the repository root. The target's path, `apps/checkout`, already has +a root that supplies `namespace: shop`: ```text apps/checkout/ diff --git a/docs/layout/shapes/5-kustomize-single-folder/config/gittarget.yaml b/docs/layout/shapes/5-kustomize-single-folder/config/gittarget.yaml index 4386e33b..c90be2df 100644 --- a/docs/layout/shapes/5-kustomize-single-folder/config/gittarget.yaml +++ b/docs/layout/shapes/5-kustomize-single-folder/config/gittarget.yaml @@ -8,7 +8,6 @@ spec: name: app-repository branch: main path: apps/checkout - suspend: true # adoption dry run: scans and publishes, writes nothing # Nothing else is declared, and that is the shape: the folder already has one # kustomization.yaml, so a new file is placed beside it, registered in its # resources:, and the root's namespace: shop is what lets the document omit its diff --git a/docs/layout/shapes/5-kustomize-single-folder/expected-checkout-config.patch b/docs/layout/shapes/5-kustomize-single-folder/expected-checkout-config.patch index 39039d00..ae0a32eb 100644 --- a/docs/layout/shapes/5-kustomize-single-folder/expected-checkout-config.patch +++ b/docs/layout/shapes/5-kustomize-single-folder/expected-checkout-config.patch @@ -1,12 +1,3 @@ -diff --git a/apps/checkout/kustomization.yaml b/apps/checkout/kustomization.yaml ---- a/apps/checkout/kustomization.yaml -+++ b/apps/checkout/kustomization.yaml -@@ -2,4 +2,5 @@ apiVersion: kustomize.config.k8s.io/v1beta1 - kind: Kustomization - namespace: shop - resources: - - web.yaml -+ - checkout-config.yaml diff --git a/apps/checkout/checkout-config.yaml b/apps/checkout/checkout-config.yaml new file mode 100644 --- /dev/null @@ -17,4 +8,12 @@ new file mode 100644 +metadata: + name: checkout-config +data: -+ timeout: "15m" ++ timeout: 15m +diff --git a/apps/checkout/kustomization.yaml b/apps/checkout/kustomization.yaml +--- a/apps/checkout/kustomization.yaml ++++ b/apps/checkout/kustomization.yaml +@@ -3,3 +3,4 @@ kind: Kustomization + namespace: shop + resources: + - web.yaml ++ - checkout-config.yaml diff --git a/docs/layout/shapes/5-kustomize-single-folder/repository/kustomization.yaml b/docs/layout/shapes/5-kustomize-single-folder/repository/apps/checkout/kustomization.yaml similarity index 100% rename from docs/layout/shapes/5-kustomize-single-folder/repository/kustomization.yaml rename to docs/layout/shapes/5-kustomize-single-folder/repository/apps/checkout/kustomization.yaml diff --git a/docs/layout/shapes/5-kustomize-single-folder/repository/web.yaml b/docs/layout/shapes/5-kustomize-single-folder/repository/apps/checkout/web.yaml similarity index 100% rename from docs/layout/shapes/5-kustomize-single-folder/repository/web.yaml rename to docs/layout/shapes/5-kustomize-single-folder/repository/apps/checkout/web.yaml diff --git a/docs/layout/shapes/6-kustomize-base-and-overlays/README.md b/docs/layout/shapes/6-kustomize-base-and-overlays/README.md index 8b46d261..49a2721d 100644 --- a/docs/layout/shapes/6-kustomize-base-and-overlays/README.md +++ b/docs/layout/shapes/6-kustomize-base-and-overlays/README.md @@ -20,6 +20,12 @@ apps/checkout/ ## One target per leaf overlay Three environments are **three `GitTarget` objects**, each rooted at a leaf: +[`config/gittarget-app-root.yaml`](config/gittarget-app-root.yaml) is the same repository pointed at +`apps/checkout` instead of a leaf, which covers four render roots at once: +[`expected-app-root-status.yaml`](expected-app-root-status.yaml) is the `LayoutResolved=Ambiguous` +it earns and the refusal that comes with it. It is the one refusal in this set the operator enforces +today. + [`config/gittarget-prod.yaml`](config/gittarget-prod.yaml) and [`config/gittarget-test.yaml`](config/gittarget-test.yaml) are the same object with the environment swapped, and neither declares any layout configuration at all. diff --git a/docs/layout/shapes/6-kustomize-base-and-overlays/config/gittarget-app-root.yaml b/docs/layout/shapes/6-kustomize-base-and-overlays/config/gittarget-app-root.yaml new file mode 100644 index 00000000..3e7fd164 --- /dev/null +++ b/docs/layout/shapes/6-kustomize-base-and-overlays/config/gittarget-app-root.yaml @@ -0,0 +1,19 @@ +# The MISTAKE this shape exists to name, kept as a fixture because a set where every write +# succeeds is advertising rather than specification. Same repository as gittarget-prod.yaml, +# one field different: the path is the app root rather than a leaf overlay. +# +# apps/checkout covers four kustomize render roots — base and three overlays — so there is no +# single one for a new document to go into, and picking one would hand it to an environment +# nobody named. The target resolves LayoutResolved=Ambiguous and refuses to place, rather than +# guessing. The fix is one edit: point it at apps/checkout/overlays/prod, as gittarget-prod.yaml +# does, and declare the other environments as their own GitTargets. +apiVersion: configbutler.ai/v1alpha3 +kind: GitTarget +metadata: + name: checkout-app + namespace: shop-prod +spec: + providerRef: + name: app-repository + branch: main + path: apps/checkout diff --git a/docs/layout/shapes/6-kustomize-base-and-overlays/config/gittarget-prod.yaml b/docs/layout/shapes/6-kustomize-base-and-overlays/config/gittarget-prod.yaml index 139e10cd..df2b9f36 100644 --- a/docs/layout/shapes/6-kustomize-base-and-overlays/config/gittarget-prod.yaml +++ b/docs/layout/shapes/6-kustomize-base-and-overlays/config/gittarget-prod.yaml @@ -11,6 +11,5 @@ spec: # one write partition. apps/checkout would cover four roots at once, which is # LayoutResolved=Ambiguous, and would make the shared base writable from prod. path: apps/checkout/overlays/prod - suspend: true # adoption dry run: scans and publishes, writes nothing # Nothing declared: the overlay's own kustomization.yaml is the folder's one root, # it supplies namespace: shop-prod, and inference omits metadata.namespace. diff --git a/docs/layout/shapes/6-kustomize-base-and-overlays/config/gittarget-test.yaml b/docs/layout/shapes/6-kustomize-base-and-overlays/config/gittarget-test.yaml index 1330ba6e..1cb16c80 100644 --- a/docs/layout/shapes/6-kustomize-base-and-overlays/config/gittarget-test.yaml +++ b/docs/layout/shapes/6-kustomize-base-and-overlays/config/gittarget-test.yaml @@ -10,4 +10,3 @@ spec: name: app-repository branch: main path: apps/checkout/overlays/test - suspend: true diff --git a/docs/layout/shapes/6-kustomize-base-and-overlays/expected-app-root-status.yaml b/docs/layout/shapes/6-kustomize-base-and-overlays/expected-app-root-status.yaml new file mode 100644 index 00000000..116412a5 --- /dev/null +++ b/docs/layout/shapes/6-kustomize-base-and-overlays/expected-app-root-status.yaml @@ -0,0 +1,30 @@ +# No patch, because no document is placed: the folder covers four render roots, so placement has +# no single one to resolve to and the write is refused before a byte moves. +# +# GitPathAccepted carries the refusal verbatim — its message is the writer's own, asserted by the +# corpus — and is the one condition to automate on: it answers "may this folder be written to" +# whatever the cause. LayoutResolved carries the verdict and the roots; Stalled follows +# GitPathAccepted, as it does for every other content refusal. +status: + conditions: + - type: GitPathAccepted + status: "False" + reason: AmbiguousLayout + message: >- + the GitTarget path covers 4 kustomize render roots (base, overlays/acceptance, + overlays/prod, overlays/test), so there is no single one to place new documents into; + point the GitTarget at one of them instead + - type: LayoutResolved + status: "False" + reason: Ambiguous + message: >- + the GitTarget path covers 4 kustomize render roots (base, overlays/acceptance, + overlays/prod, overlays/test); point it at one of them, so one target is one write + partition + - type: Stalled + status: "True" + reason: AmbiguousLayout + placement: + renderRoot: "" # deliberately empty: no single root, and no arbitrary pick + # mode is ABSENT too: with four roots there is no single way this folder is written, and + # naming one of them would be the guess the Ambiguous verdict exists to refuse. diff --git a/docs/layout/shapes/6-kustomize-base-and-overlays/expected-checkout-config.patch b/docs/layout/shapes/6-kustomize-base-and-overlays/expected-checkout-config.patch index f4d358c3..002d0bcf 100644 --- a/docs/layout/shapes/6-kustomize-base-and-overlays/expected-checkout-config.patch +++ b/docs/layout/shapes/6-kustomize-base-and-overlays/expected-checkout-config.patch @@ -1,12 +1,3 @@ -diff --git a/apps/checkout/overlays/prod/kustomization.yaml b/apps/checkout/overlays/prod/kustomization.yaml ---- a/apps/checkout/overlays/prod/kustomization.yaml -+++ b/apps/checkout/overlays/prod/kustomization.yaml -@@ -2,4 +2,5 @@ apiVersion: kustomize.config.k8s.io/v1beta1 - kind: Kustomization - namespace: shop-prod - resources: - - ../../base -+ - checkout-config.yaml diff --git a/apps/checkout/overlays/prod/checkout-config.yaml b/apps/checkout/overlays/prod/checkout-config.yaml new file mode 100644 --- /dev/null @@ -17,4 +8,12 @@ new file mode 100644 +metadata: + name: checkout-config +data: -+ timeout: "15m" ++ timeout: 15m +diff --git a/apps/checkout/overlays/prod/kustomization.yaml b/apps/checkout/overlays/prod/kustomization.yaml +--- a/apps/checkout/overlays/prod/kustomization.yaml ++++ b/apps/checkout/overlays/prod/kustomization.yaml +@@ -3,3 +3,4 @@ kind: Kustomization + namespace: shop-prod + resources: + - ../../base ++ - checkout-config.yaml diff --git a/docs/layout/shapes/7-kustomize-layered/README.md b/docs/layout/shapes/7-kustomize-layered/README.md index 49a1e99a..0f689e0a 100644 --- a/docs/layout/shapes/7-kustomize-layered/README.md +++ b/docs/layout/shapes/7-kustomize-layered/README.md @@ -78,5 +78,6 @@ mistake, and the deeper one reaches more environments. **Not bootstrappable, for the same reason as shape 6 and one more.** `useKustomize: true` cannot invent `resources: [../../layers/observability]`, and it certainly cannot invent the layer. A layered -repository is scaffolded by a template or by hand, and GitOps Reverser adopts it afterwards — which -[`spec.suspend`](../../model.md) exists to make a dry run rather than a leap. +repository is scaffolded by a template or by hand, and GitOps Reverser adopts it afterwards. To see +what that adoption would write before committing to it, point a `GitTarget` at a scratch branch and +read the commits ([`model.md`](../../model.md#previewing-a-target-point-it-at-a-scratch-branch)). diff --git a/docs/layout/shapes/7-kustomize-layered/config/gittarget-prod.yaml b/docs/layout/shapes/7-kustomize-layered/config/gittarget-prod.yaml index 1fc0ca1d..e13b21c7 100644 --- a/docs/layout/shapes/7-kustomize-layered/config/gittarget-prod.yaml +++ b/docs/layout/shapes/7-kustomize-layered/config/gittarget-prod.yaml @@ -12,4 +12,3 @@ spec: # refused as a write target (L2). The leaf is the only folder in this tree with a # fan-in of one, which is what makes it the write partition. path: apps/checkout/envs/prod - suspend: true # adoption dry run: scans and publishes, writes nothing diff --git a/docs/layout/shapes/7-kustomize-layered/expected-checkout-config.patch b/docs/layout/shapes/7-kustomize-layered/expected-checkout-config.patch index 68e786f2..5efac8b5 100644 --- a/docs/layout/shapes/7-kustomize-layered/expected-checkout-config.patch +++ b/docs/layout/shapes/7-kustomize-layered/expected-checkout-config.patch @@ -1,12 +1,3 @@ -diff --git a/apps/checkout/envs/prod/kustomization.yaml b/apps/checkout/envs/prod/kustomization.yaml ---- a/apps/checkout/envs/prod/kustomization.yaml -+++ b/apps/checkout/envs/prod/kustomization.yaml -@@ -2,4 +2,5 @@ apiVersion: kustomize.config.k8s.io/v1beta1 - kind: Kustomization - namespace: shop-prod - resources: - - ../../layers/observability -+ - checkout-config.yaml diff --git a/apps/checkout/envs/prod/checkout-config.yaml b/apps/checkout/envs/prod/checkout-config.yaml new file mode 100644 --- /dev/null @@ -17,4 +8,12 @@ new file mode 100644 +metadata: + name: checkout-config +data: -+ timeout: "15m" ++ timeout: 15m +diff --git a/apps/checkout/envs/prod/kustomization.yaml b/apps/checkout/envs/prod/kustomization.yaml +--- a/apps/checkout/envs/prod/kustomization.yaml ++++ b/apps/checkout/envs/prod/kustomization.yaml +@@ -3,3 +3,4 @@ kind: Kustomization + namespace: shop-prod + resources: + - ../../layers/observability ++ - checkout-config.yaml diff --git a/docs/layout/shapes/8-base-owned-field-edit/expected-env-change-status.yaml b/docs/layout/shapes/8-base-owned-field-edit/expected-env-change-status.yaml index 84317645..f0961750 100644 --- a/docs/layout/shapes/8-base-owned-field-edit/expected-env-change-status.yaml +++ b/docs/layout/shapes/8-base-owned-field-edit/expected-env-change-status.yaml @@ -1,13 +1,17 @@ -# The second half. No patch, because no file is written: the whole flush is refused -# before a byte moves, and the result is recorded once on the GitTarget. +# The second half. No patch, because no file is written: the whole flush is refused before a byte +# moves, and the result is recorded once on the GitTarget. +# +# GitPathAccepted's message is the writer's own and is asserted by the corpus. Stalled is the +# controller's summary of it. status: conditions: - type: GitPathAccepted status: "False" reason: WriteBoundaryRefused message: >- - planned write to "base/deployment.yaml" is outside this target's write scope - (apps/checkout/overlays/prod): Deployment/checkout + planned write path "base/deployment.yaml" escapes the GitTarget write scope: the operator + only ever writes inside spec.path (reads may reach shared context such as ../../base, + writes never leave it) - type: Stalled status: "True" reason: WriteBoundaryRefused diff --git a/docs/layout/shapes/README.md b/docs/layout/shapes/README.md index 1a60c010..0470936d 100644 --- a/docs/layout/shapes/README.md +++ b/docs/layout/shapes/README.md @@ -2,8 +2,7 @@ > **design**: a specification by example for the layout model proposed in > [`../model.md`](../model.md). The two booleans shown here — `spec.serializeNamespace` and -> `spec.placement.useKustomize` — do not exist in the current release, and neither does -> `spec.suspend`. +> `spec.placement.useKustomize` — do not exist in the current release. > Date: 2026-08-31. > Index: [`../../INDEX.md`](../../INDEX.md) @@ -18,7 +17,7 @@ Every shape receives the same input, [`checkout-config.yaml`](1-flat-serialized/ a `ConfigMap` named `checkout-config` in namespace `shop`. Each folder holds `config/`, `repository/` (the starting state), `input/`, and `expected-*.patch` — the same conventions as [`../specific-examples/README.md`](../specific-examples/README.md), including patches without -`index` lines. +`index` lines and a `repository/` rooted at the repository root rather than at `spec.path`. | # | Shape | `useKustomize` | `serializeNamespace` | Placement template | |---|---|---|---|---| @@ -117,7 +116,7 @@ so every shape that relied on inference has to be *declared* instead. | Shape | Empty folder, today | What makes it work | |---|---|---| | 1 — flat, serialized | **Works.** No root is needed; inference writes the namespace anyway | nothing — but declare `serializeNamespace: true` to pin it | -| 2 — flat, namespace-free | **Broken.** Nothing supplies the namespace and nothing proves it will | `serializeNamespace: false` plus an out-of-band supplier the operator cannot verify. See below | +| 2 — flat, namespace-free | **Works, unverifiably.** Nothing in the repository supplies the namespace, and nothing ever will | `serializeNamespace: false` plus an out-of-band supplier the operator cannot see. Not a fault. See below | | 3 — tree, serialized | **Works.** The canonical path needs no context | nothing | | 4 — tree, namespace-free | **Broken**, same reason as 2 | same as 2 | | 5 — one kustomize folder | **Broken today**: no root exists, so nothing is registered and nothing renders | `placement.useKustomize: true` — the operator writes the root, `namespace:` included | @@ -133,15 +132,19 @@ legitimately leaves `metadata.namespace` out of every document it places. Nothin [`5-kustomize-single-folder`](5-kustomize-single-folder/README.md) shows both halves — the same folder adopted and created — and they differ by two lines of spec. -**Shapes 2 and 4 have no such proof, and that is a real gap.** Their supplier is a Flux -`Kustomization.spec.targetNamespace` or an Argo `Application.spec.destination.namespace` living in a -different cluster from the repository. It is a perfectly ordinary way to run GitOps — it is what -makes a folder portable — but the operator cannot see it, so the post-scan guard has nothing to -check. It can only report `Validated=False` on a folder whose documents omit a namespace no -kustomization supplies, which is a **false alarm** for exactly this shape. That is the case for -answering [`model.md`'s first open question](../model.md#open-questions) with a named supplier — -something like `serializeNamespace: false` plus an assertion that the guarantee is external, so the -user takes the responsibility explicitly rather than the operator guessing whether to complain. +**Shapes 2 and 4 have no such proof, and that is not a gap — it is the shape.** Their supplier is a +Flux `Kustomization.spec.targetNamespace` or an Argo `Application.spec.destination.namespace` living +in a different cluster from the repository. It is a perfectly ordinary way to run GitOps — it is +what makes a folder portable, and two deployers may point at the same folder and land it in two +different namespaces, both correctly. + +So there is **no post-scan supplier guard**, and no field naming the supplier either: a rule keyed on +the folder's own contents would fire on the intended use, and an assertion field would ask the user +to promise something that is not theirs to promise and that nothing could check. The line the model +draws instead is **guard what is inside the folder, say nothing about what happens after it +leaves** — which is why the one-source-namespace rule below is enforced and this one does not exist. +See +[`model.md`](../model.md#why-false-needs-no-guard). **Shapes 6 and 7 cannot be bootstrapped from empty, and the reason is not a missing flag.** An overlay is not a root plus a namespace; it is a root whose `resources:` names a *relative path to a @@ -259,13 +262,57 @@ limitation. ### Two more, carried in the sections above -- **Should `serializeNamespace: false` name its supplier?** Shapes 2 and 4 have a guarantee the - operator cannot see, so the post-scan guard can only produce a false alarm. See +- **`serializeNamespace: false` does not name its supplier, and nothing checks one.** Shapes 2 and 4 + have a guarantee the operator cannot see and that may not even be single, so any such check could + only produce a false alarm on the intended use. See [Pointing each shape at an empty folder](#pointing-each-shape-at-an-empty-folder). - **Creating an overlay is scaffolding, not placement.** `useKustomize` cannot invent a base reference, and the folder it would create renders green while being the wrong folder. See [shape 6](6-kustomize-base-and-overlays/README.md). +## What each shape reports, and what a delete does to it + +`status.placement.mode` is the one field that separates the raw shapes from the kustomize ones, and +it is worth reading as the deletion table it really is. **Every shape above answers "where does a +new file go"; this answers "and what happens when it goes away".** + +| Shape | `mode` | `renderRoot` | `readOnlyBases` | A delete removes… | +|---|---|---|---|---| +| [1](1-flat-serialized/README.md), [2](2-flat-namespace-free/README.md) | `Plain` | — | — | the file. Nothing else is touched | +| [3](3-tree-serialized/README.md), [4](4-tree-namespace-free/README.md) | `Plain` | — | — | the file. Empty directories are left behind | +| [5](5-kustomize-single-folder/README.md) | `KustomizeRoot` | `.` | — | the file **and** its `resources:` entry, in one commit | +| [6](6-kustomize-base-and-overlays/README.md), [7](7-kustomize-layered/README.md) at the leaf | `KustomizeOverlay` | `.` | `../../base` | the file and its entry — **unless the object is inherited**, see below | +| [6](6-kustomize-base-and-overlays/README.md), [7](7-kustomize-layered/README.md) at the wide path | *absent* | *absent* | — | nothing: the folder is `Ambiguous` and places nothing | +| [8](8-base-owned-field-edit/README.md) | `KustomizeOverlay` | `.` | `../../base` | as 6/7 — the scenario is an edit, not a delete | + +Three things follow, and only the first is obvious: + +- **In a `Plain` folder a delete is a file removal and nothing more.** No index to maintain, no + render to re-prove. This is the shape where what you see in Git is all there is. +- **In a `KustomizeRoot` folder deleting the manifest is only half the delete.** An entry still + naming a file that does not exist makes `kustomize build` fail, so the `resources:` entry goes in + the same commit + ([`dropKustomizationResource`](../../../internal/git/plan_flush.go)). If the entry cannot be removed, + nothing is committed: the re-render precondition rebuilds the tree and refuses rather than + pushing a folder that does not build. +- **In a `KustomizeOverlay`, deleting an object the overlay INHERITS deletes nothing.** The document + lives in the base, which is outside the write scope and shared with the other environments, so + removing it would delete the object from every environment at once. Instead the operator authors + a `$patch: delete` file into the overlay and names it in the overlay's `patches:`, and the + re-render oracle proves the object leaves *this* overlay's render. The base is never touched. + This is the single most surprising behaviour in the model, which is why `mode` is published: it + is the field that tells you to expect it. + +`spec.prune.mode` sits on top of all of this and decides whether a delete is attempted at all — +`Never` removes nothing, `OnEvent` (the default) mirrors an observed DELETE, `Always` additionally +lets a resync infer one. The table describes what happens once a delete is allowed through. + +**To watch any of this happen before you commit to it, point a `GitTarget` at a scratch branch** and +read the commits it makes: the file removals, the `resources:` edits and the `$patch: delete` files +are all right there in a diff. That is the preview, and it is why neither `status.placement` nor +`spec.suspend` tries to be one — see +[`model.md`](../model.md#previewing-a-target-point-it-at-a-scratch-branch). + ## What this set does not cover - **Secrets and encryption.** `{sensitiveSuffix}`, the SOPS naming convention, and the rule that a diff --git a/docs/layout/specific-examples/README.md b/docs/layout/specific-examples/README.md index 93fb5daa..d9b8a7cc 100644 --- a/docs/layout/specific-examples/README.md +++ b/docs/layout/specific-examples/README.md @@ -1,8 +1,8 @@ # Specific examples: two ecosystems, and the shared prerequisites > **design**: worked scenarios for the layout model in [`../model.md`](../model.md). The -> `GitTarget` files use `spec.serializeNamespace`, `spec.placement.useKustomize` and `spec.suspend`, -> none of which exist in the current release. +> `GitTarget` files use `spec.serializeNamespace` and `spec.placement.useKustomize`, neither of +> which exists in the current release. > Date: 2026-08-31. > Index: [`../../INDEX.md`](../../INDEX.md) @@ -45,8 +45,8 @@ the refusal halves included: The same as `shapes/`, so one harness reads both: -- `repository/` — the relevant repository subtree, with each scenario stating whether it is the - starting state or the state after the illustrated change. +- `repository/` — the starting state, always rooted at the **repository root** rather than at a + target's `spec.path`, so the folder shows where the target sits as well as what it holds. - `config/` — the `GitTarget` and watcher objects that describe the target. - `input/` — one live object **as the operator receives it from the API server**, not as it is written to Git. diff --git a/docs/layout/specific-examples/homelab-argocd/README.md b/docs/layout/specific-examples/homelab-argocd/README.md index 374d6ab1..2f3db8aa 100644 --- a/docs/layout/specific-examples/homelab-argocd/README.md +++ b/docs/layout/specific-examples/homelab-argocd/README.md @@ -10,7 +10,8 @@ Application created in the Argo CD UI receives a sibling file such as `paperless and an entry in the root's `resources:` list. The tree below shows the target's path in the real repository; -[`repository/`](repository/) in this folder **is** `bootstrap/argocd-applications/`. +[`repository/`](repository/) in this folder is the repository root, and the target's path is +`bootstrap/argocd-applications/`. ```text bootstrap/argocd-applications/ @@ -31,7 +32,7 @@ objects. The folder's kustomize root supplies the namespace, so the Application `metadata.namespace`. **This is the scenario the post-scan guard exists for.** The root that makes the omission safe is -[`repository/kustomization.yaml`](repository/kustomization.yaml) — a file the *repository owner* +[`kustomization.yaml`](repository/bootstrap/argocd-applications/kustomization.yaml) — a file the *repository owner* controls, not the operator. Delete it, or delete its `namespace: argocd` line, and every subsequent document would land in whatever namespace the applier happens to be pointed at, which is a different object with the same name. Because `serializeNamespace: false` is an explicit override of a @@ -73,13 +74,11 @@ because the rest of that prefix is user data. See reason: SingleKustomization message: "render root '.' governs new files" placement: + mode: KustomizeRoot renderRoot: . - serializeNamespace: false ``` -- Expected Git change: - [`expected-paperless.patch`](expected-paperless.patch), after a reviewer - clears `suspend`. +- Expected Git change: [`expected-paperless.patch`](expected-paperless.patch). - Expected status: `Ready=True` after the root renders the added Application. - Boundary: only `Application` declarations in `argocd` are eligible. An Argo-created workload has no writable home in this target. diff --git a/docs/layout/specific-examples/homelab-argocd/config/gittarget.yaml b/docs/layout/specific-examples/homelab-argocd/config/gittarget.yaml index 36026330..3c2b03cb 100644 --- a/docs/layout/specific-examples/homelab-argocd/config/gittarget.yaml +++ b/docs/layout/specific-examples/homelab-argocd/config/gittarget.yaml @@ -8,7 +8,6 @@ spec: name: homelab-repository branch: main path: bootstrap/argocd-applications - suspend: true # adoption dry run: scans and publishes, writes nothing # Safe here only because the folder's kustomization.yaml supplies # namespace: argocd — and that file belongs to the repository owner, not to us. # The post-scan pass checks that supplier on every scan, so deleting it turns diff --git a/docs/layout/specific-examples/homelab-argocd/expected-paperless.patch b/docs/layout/specific-examples/homelab-argocd/expected-paperless.patch index 572e7f2e..bb0c0a31 100644 --- a/docs/layout/specific-examples/homelab-argocd/expected-paperless.patch +++ b/docs/layout/specific-examples/homelab-argocd/expected-paperless.patch @@ -1,7 +1,7 @@ diff --git a/bootstrap/argocd-applications/kustomization.yaml b/bootstrap/argocd-applications/kustomization.yaml --- a/bootstrap/argocd-applications/kustomization.yaml +++ b/bootstrap/argocd-applications/kustomization.yaml -@@ -4,3 +4,4 @@ kind: Kustomization +@@ -4,3 +4,4 @@ namespace: argocd resources: - application-jellyfin.yaml - application-nextcloud.yaml @@ -16,14 +16,14 @@ new file mode 100644 +metadata: + name: paperless +spec: ++ destination: ++ namespace: paperless ++ server: https://kubernetes.default.svc + project: default + source: -+ repoURL: https://github.com/example/homelab.git + path: apps/paperless ++ repoURL: https://github.com/example/homelab.git + targetRevision: main -+ destination: -+ server: https://kubernetes.default.svc -+ namespace: paperless + syncPolicy: + automated: + prune: true diff --git a/docs/layout/specific-examples/homelab-argocd/repository/application-jellyfin.yaml b/docs/layout/specific-examples/homelab-argocd/repository/bootstrap/argocd-applications/application-jellyfin.yaml similarity index 100% rename from docs/layout/specific-examples/homelab-argocd/repository/application-jellyfin.yaml rename to docs/layout/specific-examples/homelab-argocd/repository/bootstrap/argocd-applications/application-jellyfin.yaml diff --git a/docs/layout/specific-examples/homelab-argocd/repository/application-nextcloud.yaml b/docs/layout/specific-examples/homelab-argocd/repository/bootstrap/argocd-applications/application-nextcloud.yaml similarity index 100% rename from docs/layout/specific-examples/homelab-argocd/repository/application-nextcloud.yaml rename to docs/layout/specific-examples/homelab-argocd/repository/bootstrap/argocd-applications/application-nextcloud.yaml diff --git a/docs/layout/specific-examples/homelab-argocd/repository/kustomization.yaml b/docs/layout/specific-examples/homelab-argocd/repository/bootstrap/argocd-applications/kustomization.yaml similarity index 100% rename from docs/layout/specific-examples/homelab-argocd/repository/kustomization.yaml rename to docs/layout/specific-examples/homelab-argocd/repository/bootstrap/argocd-applications/kustomization.yaml diff --git a/docs/layout/specific-examples/homelab-flux/README.md b/docs/layout/specific-examples/homelab-flux/README.md index c73523ab..43a89a73 100644 --- a/docs/layout/specific-examples/homelab-flux/README.md +++ b/docs/layout/specific-examples/homelab-flux/README.md @@ -20,8 +20,8 @@ alone. ## Repository folder -[`repository/`](repository/) is rooted at the repository root rather than at one target's path, -because this scenario has two targets on two layers: +[`repository/`](repository/) is rooted at the repository root, as every scenario's is. Here that +matters twice over, because this scenario has two targets on two layers: ```text clusters/home/flux-system/ # flux bootstrap owns this. GitOps Reverser never writes here. @@ -65,9 +65,9 @@ Two targets, one per layer: | [`config/gittarget.yaml`](config/gittarget.yaml) | `infrastructure/home/sources` | `GitRepository`, `HelmRepository` ([`config/watchrule.yaml`](config/watchrule.yaml)) | | [`config/gittarget-media.yaml`](config/gittarget-media.yaml) | `apps/home/media` | `HelmRelease` ([`config/watchrule-media.yaml`](config/watchrule-media.yaml)) | -Both declare a one-root Kustomize folder with one source namespace, and both open suspended. Each -folder's `namespace: flux-system` transformer supplies the namespace for every -document in it, so the committed documents omit `metadata.namespace`. +Both declare a one-root Kustomize folder with one source namespace. Each folder's +`namespace: flux-system` transformer supplies the namespace for every document in it, so the +committed documents omit `metadata.namespace`. The `HelmRelease` object living in `flux-system` does not put jellyfin there: [`helmrelease-jellyfin.yaml`](repository/apps/home/media/helmrelease-jellyfin.yaml) carries @@ -103,13 +103,11 @@ not adopted yet, so deleting a recreated object would block on a controller that reason: SingleKustomization message: "render root '.' governs new files" placement: + mode: KustomizeRoot renderRoot: . - serializeNamespace: false ``` -- Expected Git change: - [`expected-bitnami.patch`](expected-bitnami.patch), after a reviewer clears `suspend` on - `flux-sources`. +- Expected Git change: [`expected-bitnami.patch`](expected-bitnami.patch). - Expected status: `Ready=True` after the root renders the new declaration. - Boundary: a rendered object has no writable home; only selected Flux declarations can produce a Git change. diff --git a/docs/layout/specific-examples/homelab-flux/config/gittarget-media.yaml b/docs/layout/specific-examples/homelab-flux/config/gittarget-media.yaml index c21739d2..103d689d 100644 --- a/docs/layout/specific-examples/homelab-flux/config/gittarget-media.yaml +++ b/docs/layout/specific-examples/homelab-flux/config/gittarget-media.yaml @@ -12,5 +12,4 @@ spec: name: homelab-repository branch: main path: apps/home/media - suspend: true # adoption dry run: scans and publishes, writes nothing serializeNamespace: false # the folder's own kustomization.yaml supplies it diff --git a/docs/layout/specific-examples/homelab-flux/config/gittarget.yaml b/docs/layout/specific-examples/homelab-flux/config/gittarget.yaml index b4170f58..c0e0eea3 100644 --- a/docs/layout/specific-examples/homelab-flux/config/gittarget.yaml +++ b/docs/layout/specific-examples/homelab-flux/config/gittarget.yaml @@ -10,5 +10,4 @@ spec: # Not clusters/home/flux-system. That directory belongs to `flux bootstrap`, and a # GitTarget must not point at a path another controller writes. path: infrastructure/home/sources - suspend: true # adoption dry run: scans and publishes, writes nothing serializeNamespace: false # the folder's own kustomization.yaml supplies it diff --git a/docs/layout/specific-examples/homelab-flux/expected-bitnami.patch b/docs/layout/specific-examples/homelab-flux/expected-bitnami.patch index 0c05e0a4..43722353 100644 --- a/docs/layout/specific-examples/homelab-flux/expected-bitnami.patch +++ b/docs/layout/specific-examples/homelab-flux/expected-bitnami.patch @@ -1,12 +1,3 @@ -diff --git a/infrastructure/home/sources/kustomization.yaml b/infrastructure/home/sources/kustomization.yaml ---- a/infrastructure/home/sources/kustomization.yaml -+++ b/infrastructure/home/sources/kustomization.yaml -@@ -3,4 +3,5 @@ kind: Kustomization - namespace: flux-system - resources: - - gitrepository-homelab.yaml - - helmrepository-jellyfin.yaml -+ - bitnami.yaml diff --git a/infrastructure/home/sources/bitnami.yaml b/infrastructure/home/sources/bitnami.yaml new file mode 100644 --- /dev/null @@ -19,3 +10,11 @@ new file mode 100644 +spec: + interval: 1h + url: https://charts.bitnami.com/bitnami +diff --git a/infrastructure/home/sources/kustomization.yaml b/infrastructure/home/sources/kustomization.yaml +--- a/infrastructure/home/sources/kustomization.yaml ++++ b/infrastructure/home/sources/kustomization.yaml +@@ -4,3 +4,4 @@ namespace: flux-system + resources: + - gitrepository-homelab.yaml + - helmrepository-jellyfin.yaml ++ - bitnami.yaml diff --git a/docs/spec/status-conditions-guide.md b/docs/spec/status-conditions-guide.md index d3ee6917..2ec4c410 100644 --- a/docs/spec/status-conditions-guide.md +++ b/docs/spec/status-conditions-guide.md @@ -125,6 +125,7 @@ const ( TypeStreamsRunning = "StreamsRunning" TypeGitPathAccepted = "GitPathAccepted" TypeGitTargetReady = "GitTargetReady" + TypeLayoutResolved = "LayoutResolved" // WatchRule only: whether every rule item's RESOLVED source-namespace scope is authorized. TypeSourceNamespaceAuthorized = "SourceNamespaceAuthorized" @@ -143,6 +144,19 @@ Canonical reads: - refused Git path, invalid provider, RBAC denial, or broken encryption: `Ready=False`, `Reconciling=False`, `Stalled=True` - Git path refusal details live on `GitPathAccepted=False` and `Stalled=True`, reason `UnsupportedContent` +- **suspended**: `Ready=True` with reason `Suspended`. Not writing on request is a configured + outcome, so nothing goes False for it. `status.placement` keeps updating, because a suspended + target still scans — a valve that also stopped looking would freeze it at whatever the folder + looked like when someone panicked. `status.retention` goes ABSENT instead of zero: no resync + sweeps while suspended, so nothing is counted, and a zero would read as converged +- `LayoutResolved` reports what the last scan resolved about the folder's shape, with + `status.placement` carrying the detail — `mode` (`Plain`, `KustomizeRoot`, `KustomizeOverlay`), + the governing `renderRoot`, and the `readOnlyBases` the folder renders but may not write to. It + is an OBSERVATION and writes no part of the trio. + `SingleKustomization` and `None` are both `True`: a folder with no kustomization is the ordinary + case, and reporting the ordinary case as `False` is how a condition gets trained out of a + reader's attention. Only `Ambiguous` is `False`, and the write refusal it implies is carried by + `GitPathAccepted=False` with reason `AmbiguousLayout` rather than by a second gate - WatchRule and ClusterWatchRule carry target dependency health in `GitTargetReady` - WatchRule carries source-namespace authorization in `SourceNamespaceAuthorized`, a positive state-style condition set even for legacy own-namespace rules (reason `LegacySourceNamespace`), so diff --git a/docs/tasks-overview.md b/docs/tasks-overview.md index 449cc39c..af066a6a 100644 --- a/docs/tasks-overview.md +++ b/docs/tasks-overview.md @@ -64,6 +64,7 @@ Run `task` (or `task help`) to list everything. | `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 scan-image` | Scans a container image with Trivy: a full report, then the gate that fails on CRITICAL findings that have a fix. Takes `SCAN_ARCHIVE=` or `SCAN_IMAGE=`. | When the CI image scan fails, to reproduce it locally. | `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 @@ -151,6 +152,22 @@ plus `.hadolint.yaml`; `lint-actions` the `.github/workflows/*` glob (it runs `a 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. +`task scan-image` is deliberately **not** one of them, and is not in the DAG above. It scans a +container image, so it needs an image — which a lint run has no reason to build, and which CI +already has in hand by the time it scans. It is the same command both CI scan jobs run, inside the +same container, so the gate can be reproduced locally instead of by pushing: + +```bash +task scan-image SCAN_ARCHIVE=project-image.tar # the artifact CI builds +task scan-image SCAN_IMAGE=ghcr.io/example/img@sha256:… # or any reference +``` + +Suppressions live in [`.trivyignore.yaml`](../.trivyignore.yaml), each with a justification and an +expiry date, and only the gate reads them — a suppressed finding still appears in the report above +it. Trivy treats a missing ignore file as a fatal error rather than carrying on without it. The +binary ships in the CI/dev container, so a devcontainer built before it was added needs rebuilding +before this task runs. + `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 diff --git a/internal/controller/gittarget_controller.go b/internal/controller/gittarget_controller.go index d2720f4b..47cb57ce 100644 --- a/internal/controller/gittarget_controller.go +++ b/internal/controller/gittarget_controller.go @@ -100,6 +100,10 @@ const ( GitTargetReasonWatchPlanFailing = "WatchPlanFailing" GitTargetStreamsRunningReasonNotReady = "NotReady" + + // GitTargetReasonSuspended is Ready=True on a target whose spec.suspend stops it writing. It + // is True on purpose: suppressing writes on request is a configured outcome, not ill health. + GitTargetReasonSuspended = "Suspended" ) const ( @@ -118,6 +122,11 @@ type GitTargetReconciler struct { // Recorder emits a Kubernetes Event on every persisted Ready transition. It may be nil in // tests, in which case no Event is recorded and nothing else changes. Recorder record.EventRecorder + + // reconcileRequests remembers which reconcile-request annotation values have been acted on, + // so a standing annotation forces one re-read rather than one per reconcile forever. Its zero + // value is usable. + reconcileRequests reconcileRequestTracker } // +kubebuilder:rbac:groups=configbutler.ai,resources=gittargets,verbs=get;list;watch;create;update;patch;delete @@ -145,6 +154,13 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( target.Status.ObservedGeneration = target.Generation gitPathWasRefused := conditionIsFalse(target.Status.Conditions, GitTargetConditionGitPathAccepted) + // Ahead of every gate, so a target held unready still shows what its folder resolved to. That + // ordering is the point rather than a convenience: the stanza's job is to explain a refused or + // surprising write, and a projection that ran only on the happy path would be missing exactly + // when it is wanted. + layout, scanned := r.observeLayout(&target) + publishLayout(st, &target, layout, scanned) + providerNS := target.Namespace validated, validationMsg, validationResult, validationErr := r.evaluateValidatedGate(ctx, st, &target, providerNS) if validationErr != nil { @@ -201,7 +217,12 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{}, sourceProviderErr } - observed := r.observeDataPlane(&target, sourceProvider, gitPathWasRefused, log) + // A standing reconcile request forces the same re-check a refused Git path does: the watch + // plane re-anchors the target's streams, which is what makes it re-read the folder rather than + // wait for the periodic pass. Taken once per distinct annotation value. + forceRecheck := gitPathWasRefused || r.reconcileRequests.take( + types.NewResourceReference(target.Name, target.Namespace), reconcileRequestedAt(&target)) + observed := r.observeDataPlane(&target, sourceProvider, forceRecheck, log) st.setValue(GitTargetConditionStreamsRunning, observed.axes.Streams) st.setValue(GitTargetConditionGitPathAccepted, observed.axes.GitPath) st.setValue(GitTargetConditionRenderMatchesLive, observed.axes.Render) @@ -217,6 +238,19 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( st.setValue(GitTargetConditionClusterProviderReady, clusterProvider) rd := newGitTargetReadiness() + // A suspended target is healthy, not faulty: not writing is the configured outcome, and no + // condition may go False for one — that is what trains operators to ignore the conditions + // that mean the mirror is genuinely broken. status.retention is the precedent. So Ready stays + // True and only its reason changes, which is what makes the state legible without making it + // look like a fault. Every real gate below still applies: a suspended target with a broken + // provider is still not Ready. + if target.Spec.Suspend { + rd.convergesAs(conditionValue{ + Status: metav1.ConditionTrue, + Reason: GitTargetReasonSuspended, + Message: "GitTarget is suspended: it scans and publishes what it resolved, and writes nothing", + }) + } gitTargetReadinessGates(rd, observed, provider, clusterProvider, sourceReach) st.applyReadiness(rd) @@ -488,7 +522,7 @@ type dataPlaneObservation struct { func (r *GitTargetReconciler) observeDataPlane( target *configbutleraiv1alpha3.GitTarget, sourceProvider *configbutleraiv1alpha3.ClusterProvider, - gitPathWasRefused bool, + forceRecheck bool, log logr.Logger, ) dataPlaneObservation { if r.EventRouter == nil || r.EventRouter.WatchManager == nil { @@ -524,7 +558,7 @@ func (r *GitTargetReconciler) observeDataPlane( target.SourceCluster(), sourceProvider.AuditRoute(), target.EffectivePruneMode(), - gitPathWasRefused, + forceRecheck, ) observation := dataPlaneObservation{declare: manager.DeclareStatusForGitTarget(gitDest)} @@ -543,7 +577,16 @@ func (r *GitTargetReconciler) observeDataPlane( target.Status.Streams = gitTargetStreamsStatus(observation.streams) // Retention is read beside the others and projected the same way, but it feeds NO condition: a // document kept by policy is the configured outcome, not a degraded target. - target.Status.Retention = gitTargetRetentionStatus(manager.RetentionForGitTarget(gitDest)) + // + // A SUSPENDED target publishes none of it. The resync stops before the mark-and-sweep, so + // nothing is swept and nothing is counted — and a published zero would read as "converged" + // when it means "not measured". Absent already means "no resync has reported", which is + // exactly the truth here. + if target.Spec.Suspend { + target.Status.Retention = nil + } else { + target.Status.Retention = gitTargetRetentionStatus(manager.RetentionForGitTarget(gitDest)) + } return observation } @@ -1022,12 +1065,15 @@ func (r *GitTargetReconciler) cleanupDeletedGitTarget( namespacedName k8stypes.NamespacedName, log logr.Logger, ) { + gitDest := types.NewResourceReference(namespacedName.Name, namespacedName.Namespace) + // Unconditionally, and before the EventRouter check below: the tracker is the reconciler's own + // memory, so it must be released for a deleted target whether or not a data plane is wired. + r.reconcileRequests.forget(gitDest) + if r.EventRouter == nil { return } - gitDest := types.NewResourceReference(namespacedName.Name, namespacedName.Namespace) - r.EventRouter.UnregisterGitTargetEventStream(gitDest) // Forget the diff-wake's last-Declared cache so a GitTarget recreated with the same name is a @@ -1091,7 +1137,9 @@ func (r *GitTargetReconciler) SetupWithManager(mgr ctrl.Manager) error { // turns straight back into a queued request, un-rate-limited. reconcileStatus.commit() // already suppresses no-op writes, so the loop has no fuel; this makes it structural, and // matches what GitProvider and ClusterProvider already do. - For(&configbutleraiv1alpha3.GitTarget{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). + // It additionally admits a change to the reconcile-request annotation, which does not bump + // metadata.generation and would otherwise be filtered out along with the status writes. + For(&configbutleraiv1alpha3.GitTarget{}, builder.WithPredicates(reconcileRequestedOrSpecChanged())). // No control-plane Secret watch. Reacting to age-key Secret changes with a // full-object Secret watch made the process retain every Secret value in the // cluster. Generated-age-Secret recovery and out-of-band age-key updates are diff --git a/internal/controller/gittarget_layout.go b/internal/controller/gittarget_layout.go new file mode 100644 index 00000000..5614e6ca --- /dev/null +++ b/internal/controller/gittarget_layout.go @@ -0,0 +1,119 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "fmt" + "strings" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/git" + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// GitTargetConditionLayoutResolved reports what the last scan resolved about the folder's shape: +// which kustomize render root governs new documents, or that there is none, or that there are +// several. It is an OBSERVATION and writes no part of the kstatus trio — a folder with no +// kustomization at all is a perfectly healthy folder, and most of them are. +const GitTargetConditionLayoutResolved = "LayoutResolved" + +const ( + // GitTargetReasonAmbiguousLayout is a GitTarget path covering more than one kustomize render + // root. The string must stay in sync with the watch package's gitPathRefusalReason, which + // maps the corresponding write refusal onto GitPathAccepted. + GitTargetReasonAmbiguousLayout = "AmbiguousLayout" + // GitTargetReasonLayoutNotScanned is the pre-scan state: the target has not read its folder + // yet, so nothing is known about its layout. Distinct from a folder that resolved to nothing. + GitTargetReasonLayoutNotScanned = "NotScanned" +) + +// observeLayout reads the layout the data plane last resolved for this GitTarget. +// +// Absent means no scan has reported yet, which is genuinely different from a folder that +// resolved to no root: the first is Unknown and the second is a definite answer with reason +// None, and collapsing them would make a target that has never read its folder indistinguishable +// from one that read it and found a plain directory. +func (r *GitTargetReconciler) observeLayout( + target *configbutleraiv1alpha3.GitTarget, +) (git.LayoutReport, bool) { + if r.EventRouter == nil || r.EventRouter.WatchManager == nil { + return git.LayoutReport{}, false + } + return r.EventRouter.WatchManager.LayoutForGitTarget( + types.NewResourceReference(target.Name, target.Namespace)) +} + +// publishLayout writes status.placement and the LayoutResolved condition. +// +// It runs before every gate, so a target held unready still shows what its folder resolved to. +// That ordering is the point rather than a convenience: the stanza exists to explain a refused or +// surprising write, and a projection that only ran on the happy path would be missing exactly when +// it is wanted. +func publishLayout( + st *reconcileStatus, + target *configbutleraiv1alpha3.GitTarget, + report git.LayoutReport, + scanned bool, +) { + if !scanned { + st.set(GitTargetConditionLayoutResolved, metav1.ConditionUnknown, + GitTargetReasonLayoutNotScanned, + "the GitTarget folder has not been scanned yet; layout is unknown") + return + } + + target.Status.Placement = placementStatus(report) + value, message := layoutCondition(report) + st.set(GitTargetConditionLayoutResolved, value, string(report.Reason), message) +} + +// layoutCondition maps a resolution onto the condition's status and message. +// +// Only Ambiguous is False. None is a definite, healthy answer — the folder has no kustomization +// and new documents take a declared template or the canonical path — and reporting it as False +// would train operators to ignore the condition on the majority of folders, which is the same +// mistake status.retention was designed not to make. +func layoutCondition(report git.LayoutReport) (metav1.ConditionStatus, string) { + switch report.Reason { + case manifestanalyzer.LayoutSingleKustomization: + if len(report.ReadOnlyBases) > 0 { + return metav1.ConditionTrue, fmt.Sprintf( + "render root %q governs new files; it renders %s, which is read-only input", + report.RenderRoot, strings.Join(report.ReadOnlyBases, ", ")) + } + return metav1.ConditionTrue, + fmt.Sprintf("render root %q governs new files", report.RenderRoot) + case manifestanalyzer.LayoutNone: + return metav1.ConditionTrue, + "no kustomization governs this folder; new files take the declared or canonical path" + case manifestanalyzer.LayoutAmbiguous: + return metav1.ConditionFalse, fmt.Sprintf( + "the GitTarget path covers %d kustomize render roots (%s); point it at one of them, so "+ + "one target is one write partition", + len(report.RenderRoots), strings.Join(report.RenderRoots, ", ")) + default: + return metav1.ConditionUnknown, "the folder's layout could not be resolved" + } +} + +// placementStatus projects a report onto the status stanza. +// +// The stanza is deliberately small: everything a reader could get from the spec in the same GET +// was left out, and so was anything that tried to preview what the target would do. See the +// rationale block on GitTargetPlacementStatus. +func placementStatus(report git.LayoutReport) *configbutleraiv1alpha3.GitTargetPlacementStatus { + status := &configbutleraiv1alpha3.GitTargetPlacementStatus{ + Mode: configbutleraiv1alpha3.PlacementMode(report.Mode), + RenderRoot: report.RenderRoot, + ReadOnlyBases: report.ReadOnlyBases, + ResolvedAtRevision: report.Revision, + } + if !report.ResolvedAt.IsZero() { + resolved := metav1.NewTime(report.ResolvedAt) + status.ResolvedAt = &resolved + } + return status +} diff --git a/internal/controller/gittarget_layout_test.go b/internal/controller/gittarget_layout_test.go new file mode 100644 index 00000000..a861de30 --- /dev/null +++ b/internal/controller/gittarget_layout_test.go @@ -0,0 +1,279 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "encoding/json" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/event" + + configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/git" + "github.com/ConfigButler/gitops-reverser/internal/layoutfixture" + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +func layoutTestTarget() *configbutleraiv1alpha3.GitTarget { + return &configbutleraiv1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{Name: "checkout", Namespace: "shop", Generation: 4}, + } +} + +func publishForTest(t *testing.T, report git.LayoutReport, scanned bool) *configbutleraiv1alpha3.GitTarget { + t.Helper() + target := layoutTestTarget() + st := beginStatus(nil, nil, target, &target.Status.Conditions) + publishLayout(st, target, report, scanned) + return target +} + +func layoutConditionOf(t *testing.T, target *configbutleraiv1alpha3.GitTarget) metav1.Condition { + t.Helper() + for _, c := range target.Status.Conditions { + if c.Type == GitTargetConditionLayoutResolved { + return c + } + } + t.Fatalf("LayoutResolved is not published") + return metav1.Condition{} +} + +// The stanza is a fact about the folder, so a target that has never written still carries it. +// This is the property status.placement rests on: renderRoot must not wait for a placement. +func TestPublishLayout_SingleKustomization(t *testing.T) { + resolved := time.Date(2026, 7, 30, 9, 14, 22, 0, time.UTC) + target := publishForTest(t, git.LayoutReport{ + LayoutResolution: manifestanalyzer.LayoutResolution{ + Reason: manifestanalyzer.LayoutSingleKustomization, + Mode: manifestanalyzer.LayoutModeKustomizeRoot, + RenderRoot: ".", + }, + Revision: "9f3c1ab", + ResolvedAt: resolved, + }, true) + + require.NotNil(t, target.Status.Placement) + assert.Equal(t, configbutleraiv1alpha3.PlacementModeKustomizeRoot, target.Status.Placement.Mode) + assert.Equal(t, ".", target.Status.Placement.RenderRoot) + assert.Empty(t, target.Status.Placement.ReadOnlyBases) + assert.Equal(t, "9f3c1ab", target.Status.Placement.ResolvedAtRevision) + require.NotNil(t, target.Status.Placement.ResolvedAt) + assert.Equal(t, resolved, target.Status.Placement.ResolvedAt.Time.UTC()) + + condition := layoutConditionOf(t, target) + assert.Equal(t, metav1.ConditionTrue, condition.Status) + assert.Equal(t, "SingleKustomization", condition.Reason) + assert.Equal(t, int64(4), condition.ObservedGeneration) +} + +// An overlay is the shape whose write behaviour surprises people — an inherited object is +// deleted by an authored $patch: delete, and an edit to a base-owned field is refused — so the +// mode says which shape it is, and the bases it may not write to are named in the condition +// message as well as in the stanza. +func TestPublishLayout_OverlayNamesTheBasesItMayNotWrite(t *testing.T) { + target := publishForTest(t, git.LayoutReport{ + LayoutResolution: manifestanalyzer.LayoutResolution{ + Reason: manifestanalyzer.LayoutSingleKustomization, + Mode: manifestanalyzer.LayoutModeKustomizeOverlay, + RenderRoot: ".", + ReadOnlyBases: []string{"../../base"}, + }, + }, true) + + require.NotNil(t, target.Status.Placement) + assert.Equal(t, configbutleraiv1alpha3.PlacementModeKustomizeOverlay, target.Status.Placement.Mode) + assert.Equal(t, []string{"../../base"}, target.Status.Placement.ReadOnlyBases) + + condition := layoutConditionOf(t, target) + assert.Equal(t, metav1.ConditionTrue, condition.Status) + assert.Contains(t, condition.Message, "../../base") + assert.Contains(t, condition.Message, "read-only") +} + +// The stanza restates nothing the spec already carries. This is the rule that kept +// serializeNamespace, byTypeEntries and examples out of it, and it is worth a test because the +// pressure to add "just one convenient copy" is what the rule exists to resist. +func TestPublishLayout_StanzaRestatesNothingFromTheSpec(t *testing.T) { + target := publishForTest(t, git.LayoutReport{ + LayoutResolution: manifestanalyzer.LayoutResolution{ + Reason: manifestanalyzer.LayoutNone, + Mode: manifestanalyzer.LayoutModePlain, + }, + }, true) + + require.NotNil(t, target.Status.Placement) + published, err := json.Marshal(target.Status.Placement) + require.NoError(t, err) + + var keys map[string]any + require.NoError(t, json.Unmarshal(published, &keys)) + for key := range keys { + assert.Contains(t, []string{"mode", "renderRoot", "readOnlyBases", "resolvedAtRevision", "resolvedAt"}, + key, "an unexpected key reached the stanza; hold it against the rule on GitTargetPlacementStatus") + } +} + +// None is True, not False. A folder with no kustomization is the ordinary case, and reporting the +// ordinary case as False is how a condition gets trained out of a reader's attention. +func TestPublishLayout_NoKustomizationIsHealthy(t *testing.T) { + target := publishForTest(t, git.LayoutReport{ + LayoutResolution: manifestanalyzer.LayoutResolution{Reason: manifestanalyzer.LayoutNone}, + }, true) + + condition := layoutConditionOf(t, target) + assert.Equal(t, metav1.ConditionTrue, condition.Status) + assert.Equal(t, "None", condition.Reason) +} + +// The rule PR 1 ships, as a user reads it: False, named Ambiguous, and the message says which +// folders the target actually covers rather than only how many. +func TestPublishLayout_AmbiguousNamesTheRoots(t *testing.T) { + target := publishForTest(t, git.LayoutReport{ + LayoutResolution: manifestanalyzer.LayoutResolution{ + Reason: manifestanalyzer.LayoutAmbiguous, + RenderRoots: []string{"overlays/prod", "overlays/test"}, + }, + }, true) + + condition := layoutConditionOf(t, target) + assert.Equal(t, metav1.ConditionFalse, condition.Status) + assert.Equal(t, "Ambiguous", condition.Reason) + assert.Contains(t, condition.Message, "overlays/prod, overlays/test") + require.NotNil(t, target.Status.Placement) + assert.Empty(t, target.Status.Placement.RenderRoot, "no arbitrary pick reaches status") + assert.Empty(t, target.Status.Placement.Mode, + "with several roots there is no single way the folder is written") +} + +// The controller's half of docs/layout/shapes/6-kustomize-base-and-overlays. The corpus asserts +// that fixture's GitPathAccepted condition, which is the writer's own; LayoutResolved is projected +// here, from data no write-path test can produce, so without this the fixture could claim anything +// about it and stay green. +func TestPublishLayout_AmbiguousMatchesTheCorpusFixture(t *testing.T) { + want, err := layoutfixture.ReadCondition( + layoutfixture.Path("shapes", "6-kustomize-base-and-overlays", "expected-app-root-status.yaml"), + GitTargetConditionLayoutResolved) + require.NoError(t, err) + + // The roots the fixture's repository actually holds, in the order a scan reports them. + target := publishForTest(t, git.LayoutReport{ + LayoutResolution: manifestanalyzer.LayoutResolution{ + Reason: manifestanalyzer.LayoutAmbiguous, + RenderRoots: []string{ + "base", "overlays/acceptance", "overlays/prod", "overlays/test", + }, + }, + }, true) + + got := layoutConditionOf(t, target) + assert.Equal(t, want.Status, string(got.Status)) + assert.Equal(t, want.Reason, got.Reason) + assert.Equal(t, want.Message, got.Message, + "the message the controller publishes and the one the fixture claims disagree") +} + +// The third condition those fixtures carry. A refused Git path is terminal until a human changes +// the folder, so it must reach the kstatus trio as Stalled=True under its own reason rather than +// as a transient — otherwise a target that will never converge reads as one that is still trying. +func TestGitTargetReadiness_StalledFollowsGitPathAccepted(t *testing.T) { + for _, fixture := range []struct{ dir, file string }{ + {"6-kustomize-base-and-overlays", "expected-app-root-status.yaml"}, + {"8-base-owned-field-edit", "expected-env-change-status.yaml"}, + } { + t.Run(fixture.dir, func(t *testing.T) { + path := layoutfixture.Path("shapes", fixture.dir, fixture.file) + gitPath, err := layoutfixture.ReadCondition(path, GitTargetConditionGitPathAccepted) + require.NoError(t, err) + wantStalled, err := layoutfixture.ReadCondition(path, ConditionTypeStalled) + require.NoError(t, err) + + rd := newGitTargetReadiness() + gitTargetReadinessGates(rd, dataPlaneObservation{ + axes: gitTargetAxes{ + Streams: conditionValue{Status: metav1.ConditionTrue}, + GitPath: conditionValue{ + Status: metav1.ConditionFalse, + Reason: gitPath.Reason, + Message: gitPath.Message, + }, + Render: conditionValue{Status: metav1.ConditionTrue}, + }, + }, healthyDependency(), healthyDependency(), healthyDependency()) + + trio := rd.trio() + assert.Equal(t, wantStalled.Status, string(trio.Stalled.Status)) + assert.Equal(t, wantStalled.Reason, trio.Stalled.Reason, + "Stalled must carry the refusal's own reason, not a generic one") + assert.Equal(t, metav1.ConditionFalse, trio.Ready.Status, + "a refused Git path is not a converged target") + }) + } +} + +func healthyDependency() conditionValue { + return conditionValue{Status: metav1.ConditionTrue, Reason: ReasonSucceeded} +} + +// Not-yet-scanned is Unknown and writes no stanza. It is a different state from a folder that +// resolved to nothing, and collapsing the two would make a target that has never read its folder +// indistinguishable from one that read it and found a plain directory. +func TestPublishLayout_UnscannedIsUnknownAndWritesNoStanza(t *testing.T) { + target := publishForTest(t, git.LayoutReport{}, false) + + condition := layoutConditionOf(t, target) + assert.Equal(t, metav1.ConditionUnknown, condition.Status) + assert.Equal(t, GitTargetReasonLayoutNotScanned, condition.Reason) + assert.Nil(t, target.Status.Placement) +} + +// A standing annotation forces exactly one re-read. Without the tracker every reconcile after the +// annotation was set would force one, which turns a one-shot request into a permanent re-anchor. +func TestReconcileRequestTracker_TakesEachValueOnce(t *testing.T) { + var tracker reconcileRequestTracker + ref := types.NewResourceReference("checkout", "shop") + + assert.False(t, tracker.take(ref, ""), "no annotation is not a request") + assert.True(t, tracker.take(ref, "2026-08-31T10:00:00Z"), "a new request is taken") + assert.False(t, tracker.take(ref, "2026-08-31T10:00:00Z"), "the same request is not taken twice") + assert.True(t, tracker.take(ref, "2026-08-31T10:05:00Z"), "a changed value is a new request") + + // A different target's request is independent of this one's. + assert.True(t, tracker.take(types.NewResourceReference("other", "shop"), "2026-08-31T10:05:00Z")) + + // Forgetting a deleted target releases its record, so a recreate is a fresh request. + tracker.forget(ref) + assert.True(t, tracker.take(ref, "2026-08-31T10:05:00Z")) +} + +// The predicate has to admit the annotation change specifically: it does not bump +// metadata.generation, so GenerationChangedPredicate alone would filter the request out along +// with the controller's own status writes. +func TestReconcileRequestedOrSpecChanged(t *testing.T) { + annotated := func(value string, generation int64) *configbutleraiv1alpha3.GitTarget { + target := layoutTestTarget() + target.Generation = generation + if value != "" { + target.Annotations = map[string]string{ReconcileRequestAnnotation: value} + } + return target + } + p := reconcileRequestedOrSpecChanged() + + assert.True(t, p.Update(event.UpdateEvent{ + ObjectOld: annotated("", 1), ObjectNew: annotated("now", 1), + }), "a reconcile request must pass the predicate") + + assert.True(t, p.Update(event.UpdateEvent{ + ObjectOld: annotated("", 1), ObjectNew: annotated("", 2), + }), "a spec change must still pass") + + assert.False(t, p.Update(event.UpdateEvent{ + ObjectOld: annotated("now", 1), ObjectNew: annotated("now", 1), + }), "a status-only write must still be filtered") +} diff --git a/internal/controller/gittarget_reconcile_request.go b/internal/controller/gittarget_reconcile_request.go new file mode 100644 index 00000000..7b3a26ff --- /dev/null +++ b/internal/controller/gittarget_reconcile_request.go @@ -0,0 +1,98 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "sync" + + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/predicate" + + configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// ReconcileRequestAnnotation asks a GitTarget to be reconciled at once, and to re-read its Git +// folder rather than wait for the periodic pass. Set it to any value that changes — a timestamp +// is the convention — and the change is what triggers; the value itself carries no meaning. +// +// The spelling is Flux's, deliberately: `reconcile.fluxcd.io/requestedAt` is what this ecosystem +// already types, and `flux reconcile` is the muscle memory a user brings. Nothing else in this +// repository had a reconcile-request convention to be consistent with instead. +// +// It matters most for a folder someone else edits. The resolution only refreshes when this +// target scans, and a resolution that only refreshes on the periodic cadence is one you cannot iterate +// with: edit the folder, request a reconcile, read status.placement. +const ReconcileRequestAnnotation = "reconcile.configbutler.ai/requestedAt" + +// reconcileRequestTracker remembers the last reconcile-request value acted on per object, so a +// standing annotation forces exactly one re-read rather than one on every reconcile after it. +// +// It is deliberately in MEMORY rather than in status. The alternative — a +// status.lastHandledReconcileAt echo — is more surface on an API this PR is otherwise only adding +// two things to, and the cost of not having it is bounded and small: after a controller restart a +// target carrying an old annotation is force-rechecked once. A re-check re-reads Git and writes +// nothing that was not going to be written anyway, so paying it once per restart is cheaper than +// a field every consumer then has to understand. +type reconcileRequestTracker struct { + mu sync.Mutex + handled map[string]string + initOnce sync.Once +} + +// take reports whether the object carries a reconcile request that has not been acted on yet, and +// records it as handled. An absent annotation is never a request. +func (t *reconcileRequestTracker) take(ref types.ResourceReference, requestedAt string) bool { + if requestedAt == "" { + return false + } + t.initOnce.Do(func() { t.handled = map[string]string{} }) + t.mu.Lock() + defer t.mu.Unlock() + if t.handled == nil { + t.handled = map[string]string{} + } + if prior, had := t.handled[ref.Key()]; had && prior == requestedAt { + return false + } + t.handled[ref.Key()] = requestedAt + return true +} + +// forget drops a deleted object's record so the map cannot grow without bound across the +// lifetime of the process. +func (t *reconcileRequestTracker) forget(ref types.ResourceReference) { + t.mu.Lock() + defer t.mu.Unlock() + delete(t.handled, ref.Key()) +} + +// reconcileRequestedOrSpecChanged is the For() predicate a reconcile request needs. +// +// GenerationChangedPredicate alone would filter the request out: an annotation edit does not bump +// metadata.generation, which is exactly why that predicate is safe against the controller's own +// status writes. So the request is admitted as an explicit exception — the annotation's VALUE +// changing — and everything else keeps the old behaviour. +func reconcileRequestedOrSpecChanged() predicate.Predicate { + generation := predicate.GenerationChangedPredicate{} + return predicate.Funcs{ + CreateFunc: generation.Create, + DeleteFunc: generation.Delete, + GenericFunc: generation.Generic, + UpdateFunc: func(e event.UpdateEvent) bool { + if generation.Update(e) { + return true + } + if e.ObjectOld == nil || e.ObjectNew == nil { + return false + } + return e.ObjectOld.GetAnnotations()[ReconcileRequestAnnotation] != + e.ObjectNew.GetAnnotations()[ReconcileRequestAnnotation] + }, + } +} + +// reconcileRequestedAt reads the annotation off a GitTarget. +func reconcileRequestedAt(target *configbutleraiv1alpha3.GitTarget) string { + return target.GetAnnotations()[ReconcileRequestAnnotation] +} diff --git a/internal/controller/readiness.go b/internal/controller/readiness.go index 75fed6a8..4ae6daf5 100644 --- a/internal/controller/readiness.go +++ b/internal/controller/readiness.go @@ -64,6 +64,14 @@ func newReadiness(message, notStalledMessage string) *readiness { } } +// convergesAs replaces what Ready reports when no gate objects. It is for a configured state +// that changes the MEANING of converged without being a fault — a suspended object is fully +// reconciled and deliberately idle — and it is deliberately not a contribution: a gate that +// objects still wins, so this can never mask one. +func (r *readiness) convergesAs(value conditionValue) { + r.whenConverged = value +} + // stalled contributes a terminal gate: this object is not converging and will not, until someone // changes something. func (r *readiness) stalled(reason, message string) { diff --git a/internal/git/branch_worker.go b/internal/git/branch_worker.go index 294d5700..06917fbd 100644 --- a/internal/git/branch_worker.go +++ b/internal/git/branch_worker.go @@ -104,6 +104,11 @@ type BranchWorker struct { // Set by WorkerManager before Start, alongside pathRefusal. renderFidelityGate *RenderFidelityGate + // layoutReporter publishes what each scan resolved about a target's folder, projected as + // status.placement and the LayoutResolved condition. Set by the WorkerManager before Start, + // alongside pathRefusal; a nil reporter only drops the projection. + layoutReporter LayoutReporter + // Event processing eventQueue chan WorkItem ctx context.Context diff --git a/internal/git/commit_executor.go b/internal/git/commit_executor.go index c3f39867..c792f43d 100644 --- a/internal/git/commit_executor.go +++ b/internal/git/commit_executor.go @@ -164,14 +164,6 @@ func (w *BranchWorker) applyPendingWriteEvents( events []Event, targets map[pendingTargetKey]ResolvedTargetMetadata, ) (bool, error) { - // Stage path-scoped bootstrap files first, before any resource write, exactly as - // the per-event path did. - for _, event := range events { - if err := ensureBootstrapTemplateInPath(repo, sanitizePath(event.Path), event.BootstrapOptions); err != nil { - return false, err - } - } - // Plan-then-flush each GitTarget subtree once: build the structure model, resolve // every event to a single-identity action, apply to hydrated file buffers, and // flush dirty/deleted files. A grouped window is single-target, so this is usually @@ -179,6 +171,35 @@ func (w *BranchWorker) applyPendingWriteEvents( byBase := groupEventsByBase(events) anyChanges := false for _, base := range sortedBaseKeys(byBase) { + // A suspended target scans and publishes what it resolved, and writes nothing. The scan + // is not skipped with the write: dropping it would leave status.placement frozen at + // whatever the folder looked like when suspension began, which is exactly when a stale + // answer costs the most. Its events are dropped rather than deferred — resuming replays + // the cluster's current state on the next resync, not a backlog of stale intermediate + // ones. + if md, ok := targetForBase(targets, base); ok && md.Suspend { + if err := w.refuseUnsafeWorktree(ctx, worktree, base, md); err != nil { + return false, err + } + log.FromContext(ctx).V(1).Info("live write suppressed: GitTarget is suspended", + "gitTarget", md.Namespace+"/"+md.Name, "path", base, "events", len(byBase[base])) + continue + } + + // Stage this path's bootstrap files before any resource write into it. + // + // It is INSIDE the loop, and after the suspend gate, because both matter. Bootstrap + // staging writes .gittargetignore (and .sops.yaml) into the path and adds them to the + // index, and the index is shared by every target on this branch — so staging for a + // suspended path meant the next active target's commit carried those files into the + // suspended target's folder. A suspended target must leave no trace in a commit, + // including one it did not author. The resync path already had this ordering. + for _, event := range byBase[base] { + if err := ensureBootstrapTemplateInPath(repo, base, event.BootstrapOptions); err != nil { + return false, err + } + } + changed, err := w.flushEventsToWorktree( ctx, worktree, diff --git a/internal/git/layout_corpus_test.go b/internal/git/layout_corpus_test.go new file mode 100644 index 00000000..5820ea40 --- /dev/null +++ b/internal/git/layout_corpus_test.go @@ -0,0 +1,592 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "flag" + "os" + "path/filepath" + "sort" + "strings" + "testing" + + gogit "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing/format/diff" + "github.com/go-git/go-git/v6/plumbing/object" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/yaml" + + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/layoutfixture" + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + "github.com/ConfigButler/gitops-reverser/internal/types" + "github.com/ConfigButler/gitops-reverser/internal/typeset" +) + +// The layout corpus executes the worked examples under docs/layout/. Until this file +// existed those folders were read by nothing but a human, so every claim in them was +// prose: the READMEs said where a document lands and what the commit looks like, and +// nothing failed when the writer disagreed. Each scenario now seeds a worktree from +// `repository/`, folds `input/` through the real plan-then-flush path with the flush +// policy derived from `config/gittarget.yaml`, and compares a normalized diff against +// `expected-*.patch`. +// +// Three conventions here are load-bearing and are stated in +// docs/layout/shapes/README.md as well: +// +// - A scenario describing behavior that is not built yet is written NOW and skipped, naming the +// track that unskips it. The corpus is the definition of done for that track: PR 2 is +// finished when every skip naming PR 2 is gone. Not every skip is PR 2's — shape 8's +// `images:` authoring belongs to track C and outlives it — so the rule is deliberately "its +// own skips" rather than "the last skip". +// - `config/gittarget.yaml` parses into corpusGitTarget below, a HARNESS-LOCAL struct, +// for exactly as long as it names fields the API does not have. Every field it holds +// that v1alpha3.GitTargetSpec also holds is asserted against the real type by +// TestLayoutCorpus_ConfigParsesAgainstTheRealAPI, so the examples cannot quietly +// describe an API nobody built. +// - Refusals are fixtures too. A scenario set where every write succeeds is +// advertising rather than specification, so the refusal halves assert an +// `expected-status.yaml` instead of a patch. +// +// Run with -update to rewrite the expected patches from the observed diff. + +var updateLayoutCorpus = flag.Bool("update", false, + "rewrite docs/layout expected-*.patch fixtures from the observed diff") + +// layoutCorpusRoot is docs/layout/ as reached from this package's directory. The fixtures are read +// in place rather than copied into testdata/: a copy would drift from the documents it +// illustrates, and the drift would be invisible in review. +const layoutCorpusRoot = layoutfixture.Root + +// corpusGitTarget is the harness-local reading of a scenario's config/gittarget.yaml. +// +// It is deliberately NOT v1alpha3.GitTarget. The examples use spec.serializeNamespace +// and spec.placement.useKustomize, which PR 2 introduces, so decoding into the real type +// would either fail or silently drop them. The mapping is temporary by design and PR 2 +// deletes it — the pointer-typed booleans below are the fields that keep it alive, and +// when they move to the real spec this struct has nothing left to hold. +type corpusGitTarget struct { + Metadata struct { + Name string `json:"name"` + Namespace string `json:"namespace"` + } `json:"metadata"` + Spec struct { + Path string `json:"path"` + Branch string `json:"branch"` + // SerializeNamespace is PR 2's spec.serializeNamespace: unset means infer. + SerializeNamespace *bool `json:"serializeNamespace"` + Placement *struct { + ByType map[string]string `json:"byType"` + Default string `json:"default"` + // UseKustomize is PR 2's placement.useKustomize. + UseKustomize *bool `json:"useKustomize"` + } `json:"placement"` + } `json:"spec"` +} + +// policy projects the parsed config onto the flush policy the write path actually takes +// today. Only the two shipped rungs — byType and default — cross over; the two booleans +// have no consumer until PR 2, which is precisely why the scenarios that depend on them +// are skipped rather than asserted. +func (c corpusGitTarget) policy() *manifestanalyzer.PlacementPolicy { + if c.Spec.Placement == nil { + return nil + } + if len(c.Spec.Placement.ByType) == 0 && c.Spec.Placement.Default == "" { + return nil + } + return &manifestanalyzer.PlacementPolicy{ + ByType: c.Spec.Placement.ByType, + Default: c.Spec.Placement.Default, + } +} + +// corpusScenario is one executable row of the corpus: a fixture folder, which config in +// it drives the write, which input object arrives, and what Git is expected to look like +// afterwards. +type corpusScenario struct { + // dir is the fixture folder, relative to docs/layout. + dir string + // config names the GitTarget under config/; folders with one target may omit it. + config string + // input names the live object under input/. + input string + // patch names the expected diff under the fixture folder. Empty means the scenario asserts a + // refusal instead, through status. + patch string + // status names the expected-*-status.yaml a refusal scenario asserts, instead of a patch. + // The harness reads its GitPathAccepted condition WHOLE — status, reason and message — and + // requires the flush to refuse with exactly that: the status is the refusal, the reason is + // what manifestanalyzer.GitPathRefusalReason maps the refusal's issue kinds to, and the + // message is the writer's own text. Asserting all three is what keeps the fixture a + // specification rather than prose; asserting only the message let the reason drift. + // + // LayoutResolved and Stalled in the same file are the CONTROLLER's projection of this + // refusal, which no write-path test can produce. They are asserted against the same fixture + // by internal/controller (TestPublishLayout_AmbiguousMatchesTheCorpusFixture and + // TestGitTargetReadiness_StalledFollowsGitPathAccepted), so the whole file is covered even + // though no single test covers all of it. + status string + // skip names the PR that unskips this scenario, and is the whole reason the row is + // written before the behavior exists. An empty skip is a scenario that runs today. + skip string +} + +func (s corpusScenario) configFile() string { + if s.config != "" { + return s.config + } + return "gittarget.yaml" +} + +// name is the subtest name: the folder plus what the scenario expects. It is keyed on the +// expectation rather than on the config, because two scenarios in one folder can share a config +// and differ only in their input — shape 8's image bump and env change do — and naming those by +// config produces "gittarget-prod" twice. +func (s corpusScenario) name() string { + expectation := s.patch + if expectation == "" { + expectation = s.status + } + expectation = strings.TrimPrefix(expectation, "expected-") + expectation = strings.TrimSuffix(strings.TrimSuffix(expectation, ".patch"), ".yaml") + return s.dir + "/" + expectation +} + +// layoutCorpus is the whole corpus. The eight folder shapes are the cross-product of +// docs/layout/shapes/README.md; the two specific examples are the Argo CD and Flux +// repositories of docs/layout/specific-examples/README.md. +func layoutCorpus() []corpusScenario { + return []corpusScenario{ + { + dir: "shapes/1-flat-serialized", + input: "checkout-config.yaml", + patch: "expected-checkout-config.patch", + }, + { + dir: "shapes/2-flat-namespace-free", + input: "checkout-config.yaml", + patch: "expected-checkout-config.patch", + skip: "PR 2: needs spec.serializeNamespace: false. Today inference writes " + + "metadata.namespace because no kustomization in the folder supplies it", + }, + { + // The fence around "one namespace": an explicit serializeNamespace: false admits + // exactly one source namespace, and the second is refused because the two documents + // would be indistinguishable rather than merely colliding. + dir: "shapes/2-flat-namespace-free", + config: "gittarget-second-namespace.yaml", + input: "checkout-config.yaml", + status: "expected-second-namespace-status.yaml", + skip: "PR 2: the one-source-namespace refusal ships with spec.serializeNamespace " + + "(the write-plan precondition first, then the WatchRule admission check)", + }, + { + dir: "shapes/3-tree-serialized", + input: "checkout-config.yaml", + patch: "expected-checkout-config.patch", + }, + { + dir: "shapes/4-tree-namespace-free", + input: "checkout-config.yaml", + patch: "expected-checkout-config.patch", + skip: "PR 2: needs spec.serializeNamespace: false. Today inference writes " + + "metadata.namespace because no kustomization in the folder supplies it", + }, + { + dir: "shapes/5-kustomize-single-folder", + input: "checkout-config.yaml", + patch: "expected-checkout-config.patch", + }, + { + dir: "shapes/5-kustomize-single-folder", + config: "gittarget-empty-folder.yaml", + input: "checkout-config.yaml", + patch: "expected-empty-folder-first-write.patch", + skip: "PR 2: needs placement.useKustomize to create the kustomization.yaml this " + + "empty folder has none of", + }, + { + dir: "shapes/6-kustomize-base-and-overlays", + config: "gittarget-prod.yaml", + input: "checkout-config.yaml", + patch: "expected-checkout-config.patch", + }, + { + // The one refusal in the corpus that asserts a rule this PR ships: a target at the + // app root covers four render roots, so a new document has no single one to be + // placed into. expected-app-root-status.yaml is the pair of conditions a user reads. + dir: "shapes/6-kustomize-base-and-overlays", + config: "gittarget-app-root.yaml", + input: "checkout-config.yaml", + status: "expected-app-root-status.yaml", + }, + { + dir: "shapes/7-kustomize-layered", + config: "gittarget-prod.yaml", + input: "checkout-config.yaml", + patch: "expected-checkout-config.patch", + }, + { + dir: "shapes/8-base-owned-field-edit", + config: "gittarget-prod.yaml", + input: "deployment-image-bumped.yaml", + patch: "expected-image-bump.patch", + skip: "patch authoring (track C of docs/design/build-order.md): writing an " + + "images: declaration into the overlay is not built, and is not PR 2 either", + }, + { + // The refusal half of shape 8, and the reason the shape is in the set at all: the + // changed field (a container env var) is expressed only in the base, which this + // target reads and never writes. expected-env-change-status.yaml is the condition + // a user reads; the refusal below is what produces it. + dir: "shapes/8-base-owned-field-edit", + config: "gittarget-prod.yaml", + input: "deployment-env-changed.yaml", + status: "expected-env-change-status.yaml", + }, + { + dir: "specific-examples/homelab-argocd", + input: "paperless.yaml", + patch: "expected-paperless.patch", + }, + { + dir: "specific-examples/homelab-flux", + input: "bitnami.yaml", + patch: "expected-bitnami.patch", + }, + } +} + +// TestLayoutCorpus runs every scenario in docs/layout/ against the real write path. +func TestLayoutCorpus(t *testing.T) { + for _, sc := range layoutCorpus() { + t.Run(sc.name(), func(t *testing.T) { + if sc.skip != "" { + t.Skip(sc.skip) + } + runCorpusScenario(t, sc) + }) + } +} + +func runCorpusScenario(t *testing.T, sc corpusScenario) { + t.Helper() + folder := filepath.Join(layoutCorpusRoot, sc.dir) + + target := readCorpusGitTarget(t, filepath.Join(folder, "config", sc.configFile())) + obj := readCorpusInput(t, filepath.Join(folder, "input", sc.input)) + + worktree, seeded := seedCorpusWorktree(t, filepath.Join(folder, "repository")) + event := corpusEvent(t, obj, target) + + worker := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: corpusMapper()} + _, err := worker.flushEventsToWorktree( + t.Context(), worktree, sanitizePath(target.Spec.Path), + []Event{event}, target.policy(), v1alpha3.PruneOnEvent) + + if sc.patch == "" { + requireCorpusRefusal(t, err, filepath.Join(folder, sc.status), worktree, seeded) + return + } + require.NoError(t, err, "%s: the scenario expects a commit, not a refusal", sc.name()) + + got := corpusDiff(t, worktree, seeded) + assertCorpusPatch(t, filepath.Join(folder, sc.patch), got) +} + +// requireCorpusRefusal asserts a scenario that must not write: the flush refuses with the message +// the fixture's GitPathAccepted condition carries, and the worktree comes back exactly as it was +// seeded. The second half is not a formality — a refusal that leaves bytes behind is a partial +// commit, which is the failure mode the write-plan preconditions exist to prevent. +func requireCorpusRefusal( + t *testing.T, + err error, + statusPath string, + worktree *gogit.Worktree, + seeded *object.Commit, +) { + t.Helper() + require.Error(t, err, "the scenario expects a refusal") + + want, err2 := layoutfixture.ReadCondition(statusPath, "GitPathAccepted") + require.NoError(t, err2) + require.Equal(t, "False", want.Status, "%s: a refusal is GitPathAccepted=False", statusPath) + + var refused *manifestanalyzer.AcceptanceRefusedError + require.ErrorAs(t, err, &refused, + "the refusal must be an acceptance refusal, or it reaches status as an unexplained write fault") + require.Equal(t, want.Reason, manifestanalyzer.GitPathRefusalReason(refused), + "the reason this refusal publishes and the reason %s claims disagree", statusPath) + require.Contains(t, refused.Error(), want.Message, + "the refusal and %s disagree", statusPath) + + require.Empty(t, corpusDiff(t, worktree, seeded), "a refused flush must leave the folder untouched") +} + +// assertCorpusPatch compares the observed diff with the fixture, rewriting the fixture +// instead when -update is given. +func assertCorpusPatch(t *testing.T, path, got string) { + t.Helper() + if *updateLayoutCorpus { + require.NoError(t, os.WriteFile(path, []byte(got), 0o600)) + return + } + want, err := os.ReadFile(path) + require.NoError(t, err) + require.Equal(t, string(want), got, + "the write path and %s disagree; re-run with -update if the new diff is the intended one", path) +} + +// readCorpusGitTarget decodes one scenario's GitTarget through the harness-local struct. +func readCorpusGitTarget(t *testing.T, path string) corpusGitTarget { + t.Helper() + raw, err := os.ReadFile(path) + require.NoError(t, err) + var target corpusGitTarget + require.NoError(t, yaml.Unmarshal(raw, &target), "parsing %s", path) + require.NotEmpty(t, target.Spec.Path, "%s: spec.path is what the corpus writes into", path) + return target +} + +// readCorpusInput decodes the live object a scenario receives. It is deliberately the +// object as the API server serves it — uid, resourceVersion, managedFields and all — +// because the difference between it and the expected patch IS the sanitization assertion. +func readCorpusInput(t *testing.T, path string) *unstructured.Unstructured { + t.Helper() + raw, err := os.ReadFile(path) + require.NoError(t, err) + obj := &unstructured.Unstructured{} + require.NoError(t, yaml.Unmarshal(raw, &obj.Object), "parsing %s", path) + return obj +} + +// corpusEvent builds the write event for a scenario's live object. +func corpusEvent(t *testing.T, obj *unstructured.Unstructured, target corpusGitTarget) Event { + t.Helper() + gvk := obj.GroupVersionKind() + entry, ok := corpusTypes[gvk] + require.True(t, ok, "corpusTypes has no entry for %s; add it beside the type it serves", gvk) + return Event{ + Object: obj, + Identifier: types.NewResourceIdentifier( + gvk.Group, gvk.Version, entry.Resource, obj.GetNamespace(), obj.GetName()), + Operation: "CREATE", + Path: target.Spec.Path, + GitTargetName: target.Metadata.Name, + GitTargetNamespace: target.Metadata.Namespace, + } +} + +// seedCorpusWorktree materialises a scenario's `repository/` into a fresh worktree and +// commits it, returning the worktree and the commit the diff is taken against. +// +// `repository/` is always rooted at the REPOSITORY root, never at spec.path, so a fixture +// shows where the target sits as well as what it holds. Shapes 6 to 8 depend on that: their +// target is a leaf overlay whose base lives outside spec.path but inside the render scope. +func seedCorpusWorktree(t *testing.T, repositoryDir string) (*gogit.Worktree, *object.Commit) { + t.Helper() + worktree := newWorktreeForTest(t) + root := worktree.Filesystem().Root() + + require.NoError(t, filepath.WalkDir(repositoryDir, func(path string, entry os.DirEntry, err error) error { + if err != nil || entry.IsDir() { + return err + } + rel, relErr := filepath.Rel(repositoryDir, path) + if relErr != nil { + return relErr + } + body, readErr := os.ReadFile(path) + if readErr != nil { + return readErr + } + seedFile(t, root, rel, string(body)) + return nil + }), "seeding %s", repositoryDir) + + return worktree, commitCorpusWorktree(t, worktree, "seed the fixture repository") +} + +func commitCorpusWorktree(t *testing.T, worktree *gogit.Worktree, message string) *object.Commit { + t.Helper() + require.NoError(t, worktree.AddWithOptions(&gogit.AddOptions{All: true})) + hash, err := worktree.Commit(message, &gogit.CommitOptions{ + Author: corpusSignature(), Committer: corpusSignature(), AllowEmptyCommits: true, + }) + require.NoError(t, err) + repo, err := gogit.PlainOpen(worktree.Filesystem().Root()) + require.NoError(t, err) + commit, err := repo.CommitObject(hash) + require.NoError(t, err) + return commit +} + +// corpusDiff renders what the flush did to the worktree as a unified diff in the +// fixtures' format: no `index` lines, because a blob hash is noise a reader cannot check +// and a rename or a whitespace change would churn for no reason. +func corpusDiff(t *testing.T, worktree *gogit.Worktree, base *object.Commit) string { + t.Helper() + after := commitCorpusWorktree(t, worktree, "the flush under test") + + baseTree, err := base.Tree() + require.NoError(t, err) + afterTree, err := after.Tree() + require.NoError(t, err) + changes, err := object.DiffTree(baseTree, afterTree) + require.NoError(t, err) + sort.Sort(changes) + + patch, err := changes.Patch() + require.NoError(t, err) + var rendered strings.Builder + require.NoError(t, diff.NewUnifiedEncoder(&rendered, diff.DefaultContextLines).Encode(patch)) + return stripIndexLines(rendered.String()) +} + +// stripIndexLines removes the `index ..` header line from every file patch. +func stripIndexLines(patch string) string { + lines := strings.Split(patch, "\n") + kept := lines[:0] + for _, line := range lines { + if strings.HasPrefix(line, "index ") { + continue + } + kept = append(kept, line) + } + return strings.Join(kept, "\n") +} + +func corpusSignature() *object.Signature { + return &object.Signature{Name: "layout corpus", Email: "corpus@example.com"} +} + +// corpusTypes is the corpus's type registry: every kind any scenario's input carries. It +// is written out rather than discovered so a new fixture that forgets its type fails +// loudly in corpusEvent instead of resolving to an empty resource. +var corpusTypes = map[schema.GroupVersionKind]schema.GroupVersionResource{ + {Group: "", Version: "v1", Kind: "ConfigMap"}: { + Group: "", Version: "v1", Resource: "configmaps"}, + {Group: "apps", Version: "v1", Kind: "Deployment"}: { + Group: "apps", Version: "v1", Resource: "deployments"}, + {Group: "argoproj.io", Version: "v1alpha1", Kind: "Application"}: { + Group: "argoproj.io", Version: "v1alpha1", Resource: "applications"}, + {Group: "source.toolkit.fluxcd.io", Version: "v1", Kind: "HelmRepository"}: { + Group: "source.toolkit.fluxcd.io", Version: "v1", Resource: "helmrepositories"}, + {Group: "source.toolkit.fluxcd.io", Version: "v1", Kind: "GitRepository"}: { + Group: "source.toolkit.fluxcd.io", Version: "v1", Resource: "gitrepositories"}, + {Group: "helm.toolkit.fluxcd.io", Version: "v2", Kind: "HelmRelease"}: { + Group: "helm.toolkit.fluxcd.io", Version: "v2", Resource: "helmreleases"}, + {Group: "rbac.authorization.k8s.io", Version: "v1", Kind: "ClusterRole"}: { + Group: "rbac.authorization.k8s.io", Version: "v1", Resource: "clusterroles"}, +} + +// corpusMapper serves every type in corpusTypes as followable, which is what namespace +// context resolution needs to run at all. +func corpusMapper() typeset.Lookup { + entries := make([]typeset.Entry, 0, len(corpusTypes)) + for gvk, gvr := range corpusTypes { + entries = append(entries, typeset.Entry{ + GVK: gvk, + GVR: gvr, + Namespaced: gvk.Kind != "ClusterRole", + Allowed: true, + }) + } + sort.Slice(entries, func(i, j int) bool { return entries[i].GVK.String() < entries[j].GVK.String() }) + return typeset.NewSnapshotRegistry(typeset.Snapshot{Entries: entries}) +} + +// TestLayoutCorpus_ConfigParsesAgainstTheRealAPI is the check the harness-local struct +// buys. corpusGitTarget exists because the examples name fields the API does not have +// yet; the risk that creates is the opposite one — a field the examples and the API BOTH +// have, spelled differently — so every shipped field a scenario config sets is decoded +// into the real v1alpha3.GitTarget too, and must survive the round trip. +// +// PR 2 deletes corpusGitTarget and this test with it: once serializeNamespace and +// useKustomize are real fields, the scenarios decode into v1alpha3.GitTarget directly and +// there is nothing left to keep honest. +func TestLayoutCorpus_ConfigParsesAgainstTheRealAPI(t *testing.T) { + for _, sc := range layoutCorpus() { + t.Run(sc.name(), func(t *testing.T) { + path := filepath.Join(layoutCorpusRoot, sc.dir, "config", sc.configFile()) + harness := readCorpusGitTarget(t, path) + + raw, err := os.ReadFile(path) + require.NoError(t, err) + // The unbuilt fields are stripped, not tolerated: decoding them into the real + // type is exactly the thing that must start working in PR 2, and letting the + // decoder ignore them here would hide the day it does. + var shipped v1alpha3.GitTarget + require.NoError(t, yaml.Unmarshal(withoutUnbuiltFields(t, raw), &shipped), "parsing %s", path) + + require.Equal(t, harness.Spec.Path, shipped.Spec.Path, "spec.path") + require.Equal(t, harness.Metadata.Name, shipped.Name, "metadata.name") + require.Equal(t, harness.Metadata.Namespace, shipped.Namespace, "metadata.namespace") + require.Equal(t, harness.Spec.Branch, shipped.Spec.Branch, "spec.branch") + if policy := harness.policy(); policy != nil { + require.NotNil(t, shipped.Spec.Placement, "spec.placement") + require.Equal(t, policy.ByType, shipped.Spec.Placement.ByType, "spec.placement.byType") + require.Equal(t, policy.Default, shipped.Spec.Placement.Default, "spec.placement.default") + } + }) + } +} + +// withoutUnbuiltFields removes the fields PR 2 introduces from a scenario config, so what +// is left is the API as it stands today. `suspend:` was on this list and is not any more: it +// ships in PR 1, and no worked example sets it — an example exists to show what gets WRITTEN, +// and previewing that is a scratch branch rather than a suspended target. It is a line filter rather than a re-marshal +// because the configs are commented documents and the comments are half of what they say. +func withoutUnbuiltFields(t *testing.T, raw []byte) []byte { + t.Helper() + unbuilt := []string{"serializeNamespace:", "useKustomize:"} + var kept []string + for _, line := range strings.Split(string(raw), "\n") { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "#") { + continue + } + if slicesContainsPrefix(trimmed, unbuilt) { + continue + } + kept = append(kept, line) + } + return []byte(strings.Join(kept, "\n")) +} + +func slicesContainsPrefix(s string, prefixes []string) bool { + for _, p := range prefixes { + if strings.HasPrefix(s, p) { + return true + } + } + return false +} + +// TestLayoutCorpus_EveryFixtureFolderIsExecuted closes the corpus over the filesystem: a +// scenario folder that nothing in layoutCorpus() names is a fixture nobody runs, which is +// the state this whole file exists to leave behind. +func TestLayoutCorpus_EveryFixtureFolderIsExecuted(t *testing.T) { + executed := map[string]bool{} + for _, sc := range layoutCorpus() { + executed[sc.dir] = true + } + for _, parent := range []string{"shapes", "specific-examples"} { + entries, err := os.ReadDir(filepath.Join(layoutCorpusRoot, parent)) + require.NoError(t, err) + for _, entry := range entries { + if !entry.IsDir() { + continue + } + dir := parent + "/" + entry.Name() + if _, err := os.Stat(filepath.Join(layoutCorpusRoot, dir, "input")); os.IsNotExist(err) { + continue // a folder with no input/ illustrates prerequisites, not a write + } + require.True(t, executed[dir], + "%s has an input/ but no scenario in layoutCorpus() runs it", dir) + } + } +} diff --git a/internal/git/layout_report.go b/internal/git/layout_report.go new file mode 100644 index 00000000..6f6da7d1 --- /dev/null +++ b/internal/git/layout_report.go @@ -0,0 +1,93 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "context" + "time" + + gogit "github.com/go-git/go-git/v6" + "sigs.k8s.io/controller-runtime/pkg/log" + + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + itypes "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// LayoutReport is one scan's answer to "what does this folder's shape imply about where new +// documents go" — the resolution plus the revision and moment it was read at, so a consumer +// can tell a fresh answer from one taken before the folder changed. +type LayoutReport struct { + manifestanalyzer.LayoutResolution + + // Revision is the Git revision the scan read; ResolvedAt is when it ran. + Revision string + ResolvedAt time.Time +} + +// LayoutReporter publishes a folder's resolved layout to the layer that owns GitTarget status. +// +// It is the twin of PathRefusalReporter, and it exists for the same structural reason: the +// scan happens on a branch-worker goroutine with no result channel back to the controller, so +// without a hook the resolution would be computed and dropped. The watch Manager supplies it +// (WorkerManager.SetLayoutReporter), which is where the projection onto status lives. +// +// Every scan reports, including one that changes nothing and one on a SUSPENDED target: a +// stopped valve that also stopped looking would freeze status.placement at whatever the folder +// looked like when someone panicked, which is exactly when a stale answer costs the most. That +// is only possible because the report is independent of anything being written. The consumer is +// responsible for republishing only on a transition. +type LayoutReporter func(target itypes.ResourceReference, report LayoutReport) + +// reportLayout resolves the layout over a batch's store and publishes it. +// +// The store is the same one the batch resolves placements against, so the reported layout is +// the one this write actually used rather than a second opinion computed from a second scan. +// An unattributable batch (either half of the target reference empty — the CLI, and tests) +// resolves nothing and publishes nothing: the projection is keyed by "namespace/name", so an +// empty half would file the report under a key no GitTarget reads. +func (w *BranchWorker) reportLayout(ctx context.Context, batch *writeBatch, revision string) { + if w.layoutReporter == nil || batch.target.name == "" || batch.target.namespace == "" { + return + } + resolution := manifestanalyzer.ResolveLayout(batch.store, batch.writeSubdir) + log.FromContext(ctx).V(1).Info("GitTarget layout resolved", + "gitTarget", batch.target.namespace+"/"+batch.target.name, + "reason", resolution.Reason, "mode", resolution.Mode, + "renderRoot", resolution.RenderRoot, "revision", revision) + w.layoutReporter( + itypes.NewResourceReference(batch.target.name, batch.target.namespace), + LayoutReport{ + LayoutResolution: resolution, + Revision: revision, + ResolvedAt: time.Now(), + }) +} + +// scanLayout resolves a folder's layout, publishes it, and arms the batch with the verdict. +// +// It never refuses on its own. Ambiguity is a PLACEMENT problem — a new document has no single +// root to go into — and an existing document is edited where it already lives, whatever the +// folder covers. Refusing the whole flush here would also pre-empt the file-level write-boundary +// preconditions (L1 and L2), whose messages name the offending file rather than the folder, so +// the specific answer would be replaced by a general one. The refusal is raised in createNew, +// where the problem actually is. +func (w *BranchWorker) scanLayout(ctx context.Context, batch *writeBatch, worktree *gogit.Worktree) { + w.reportLayout(ctx, batch, worktreeRevision(worktree)) +} + +// worktreeRevision is the commit the scan read, or "" when the branch has no commit yet (a +// freshly bootstrapped folder). It is read from the worktree rather than from the worker's +// cached remote metadata, because the cached SHA describes the REMOTE branch and the scan +// describes what is on disk — and a placement stanza that claims a revision it did not read is +// worse than one that claims none. +func worktreeRevision(worktree *gogit.Worktree) string { + repo, err := gogit.PlainOpen(worktree.Filesystem().Root()) + if err != nil { + return "" + } + head, err := repo.Head() + if err != nil { + return "" + } + return head.Hash().String() +} diff --git a/internal/git/pending_writes.go b/internal/git/pending_writes.go index d22a4056..42249a00 100644 --- a/internal/git/pending_writes.go +++ b/internal/git/pending_writes.go @@ -170,6 +170,7 @@ func (w *BranchWorker) resolveTargetMetadata( Placement: resolvePlacementPolicy(target.Spec.Placement), PruneMode: target.EffectivePruneMode(), SourceCluster: target.SourceCluster(), + Suspend: target.Spec.Suspend, }, nil } @@ -227,6 +228,22 @@ func placementPolicyForBase( return nil } +// targetForBase returns the resolved metadata of the GitTarget writing at base, and whether +// one was found. The three per-base lookups above answer one question each; this one exists for +// the callers that need the whole record — the layout report names the target, and the suspend +// gate reads a field with no safe default (a missing target must not read as suspended). +func targetForBase( + targets map[pendingTargetKey]ResolvedTargetMetadata, + base string, +) (ResolvedTargetMetadata, bool) { + for _, md := range targets { + if sanitizePath(md.Path) == base { + return md, true + } + } + return ResolvedTargetMetadata{}, false +} + // MessageKind is derived from the pending write's shape. func (p PendingWrite) MessageKind() CommitMessageKind { if p.Kind == PendingWriteAtomic || p.Kind == PendingWriteResync { diff --git a/internal/git/plan_flush.go b/internal/git/plan_flush.go index eb9e80f8..71812c24 100644 --- a/internal/git/plan_flush.go +++ b/internal/git/plan_flush.go @@ -88,6 +88,12 @@ func (w *BranchWorker) flushEventsToWorktree( if err := batch.refusal(); err != nil { return false, err } + // Publish what this folder's shape resolved to before folding anything in. It is a fact about + // the scan, so it must not depend on the events applying — or on their being applied at all, + // which is what makes it available to a suspended target. It runs AFTER the acceptance gate + // above on purpose: a folder the operator has refused to manage is one whose layout it should + // not be making claims about, and GitPathAccepted=False already says why. + w.scanLayout(ctx, batch, worktree) for _, event := range events { if err := batch.applyEvent(ctx, event); err != nil { return false, err @@ -147,6 +153,10 @@ type writeBatch struct { // stay within it. The store and every path in it are keyed relative to renderBase, so a // writable path is one under writeSubdir. See internal/git/render_scope.go. writeSubdir string + // layout is what the scan resolved about this folder's shape. It is published as + // status.placement, and createNew reads it: a folder covering several render roots has no + // single one to place a new document into, so placing one is refused rather than guessed. + layout manifestanalyzer.LayoutResolution // coldBundles tracks, per path, the new resources this batch has placed at a // path that held no document before the batch started (keyed the same as // buffers). It exists so several new resources that render to the same @@ -201,7 +211,7 @@ func newWriteBatch( for _, f := range scan.YAMLFiles { contentByPath[f.Path] = f.Content } - return &writeBatch{ + batch := &writeBatch{ writer: writer, mapper: mapper, store: store, @@ -211,6 +221,12 @@ func newWriteBatch( policy: policy, writeSubdir: writeSubdir, } + // Resolved with the store rather than by each caller, so no write path can reach createNew + // with an unresolved layout and place a new document into a folder that has no single root + // to place it in. Publishing it is a separate step (scanLayout), because only the paths that + // know which GitTarget they serve can publish. + batch.layout = manifestanalyzer.ResolveLayout(store, writeSubdir) + return batch } // refusal runs the structure-only acceptance gate over the batch's store and returns a @@ -351,6 +367,13 @@ func (wb *writeBatch) createNew(ctx context.Context, event Event) (upsertOutcome if event.Object != nil { kind = event.Object.GetKind() } + // A folder covering several render roots has no single one for a NEW document, and picking + // one would hand it to an environment nobody named. An existing document is unaffected: it + // is edited where it lives, and this branch is not reached for it. + if issues := manifestanalyzer.AmbiguousLayoutRefusal(wb.layout, wb.writeSubdir); len(issues) > 0 { + return upsertNoChange, manifestanalyzer.RefusalError( + manifestanalyzer.Acceptance{Accepted: false, Issues: issues}) + } sensitive := wb.writer.isSensitiveIdentifier(event.Identifier) // WriteScope tells placement the write jail: when render-root scoping re-rooted the scan // past spec.path, a declared/canonical path is rebased under the jail rather than escaping diff --git a/internal/git/resync_flush.go b/internal/git/resync_flush.go index 002c30e7..dbee1e53 100644 --- a/internal/git/resync_flush.go +++ b/internal/git/resync_flush.go @@ -223,10 +223,27 @@ func (w *BranchWorker) executeResyncPendingWrite( target := pendingWrite.Target() base := sanitizePath(target.Path) - if err := w.refuseUnsafeWorktree(ctx, worktree, base, target.SourceCluster); err != nil { + if err := w.refuseUnsafeWorktree(ctx, worktree, base, target); err != nil { return 0, err } + // Suspend stops the write and nothing before it: the scan above ran, and the layout it + // resolved has already been published, so a stopped target still says what it is looking at. + // Returning zero commits leaves the pending write unretained and unpushed. + // + // It returns SUCCESS, so the drain marks this scope render-fidelity clean and reports zero + // retained documents without either having been measured. Both are accurate enough to leave + // alone and neither is load-bearing while writes are off: the fidelity gate only gates + // writes, and a target that wrote nothing has nothing that could have diverged. Resuming + // re-measures both on the first real resync. What would NOT be acceptable is returning an + // error here — a suspended target is not a failing one, and saying so would put it on the + // background-failure path and hold it unready. + if target.Suspend { + log.FromContext(ctx).V(1).Info("resync suppressed: GitTarget is suspended", + "gitTarget", target.Namespace+"/"+target.Name, "path", base) + return 0, nil + } + // Stage the path's bootstrap template (its directory and any .sops.yaml) before // applying, exactly as the per-event path does via ensureBootstrapTemplateInPath. // Without it a first resync into a fresh subtree has no directory for SOPS to chdir @@ -285,16 +302,29 @@ func (w *BranchWorker) executeResyncPendingWrite( func (w *BranchWorker) refuseUnsafeWorktree( ctx context.Context, worktree *gogit.Worktree, - base, clusterID string, + base string, + target ResolvedTargetMetadata, ) error { root := worktree.Filesystem().Root() scoped, err := scanRenderScope(root, base) if err != nil { return err } - // The acceptance gate never places a resource, so no placement policy is needed here. - batch := newWriteBatch(ctx, w.contentWriter, w.mapperForCluster(clusterID), scoped.scan, nil, scoped.writeSubdir) - return batch.refusal() + // The acceptance gate never places a resource, but the layout report resolved from the same + // scan does, so the target's declared policy is carried in rather than passed as nil. + batch := newWriteBatch( + ctx, w.contentWriter, w.mapperForCluster(target.SourceCluster), + scoped.scan, target.Placement, scoped.writeSubdir) + batch.target = placementTarget{namespace: target.Namespace, name: target.Name} + if err := batch.refusal(); err != nil { + return err + } + // This is the scan every target gets, events or not: the periodic resync runs it whether or + // not anything changed, and a SUSPENDED target reaches here and stops just after. It is + // therefore the reason status.placement is populated on a target that has never written, and + // the reason an ambiguous folder can stop being ambiguous. + w.scanLayout(ctx, batch, worktree) + return nil } // applyResyncToWorktree is the streaming mark-and-sweep resync apply (M8), described diff --git a/internal/git/suspend_test.go b/internal/git/suspend_test.go new file mode 100644 index 00000000..b6c15526 --- /dev/null +++ b/internal/git/suspend_test.go @@ -0,0 +1,287 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "os" + "path/filepath" + "testing" + "time" + + gogit "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing" + "github.com/go-git/go-git/v6/plumbing/object" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// spec.suspend is a write gate and nothing more: the scan in front of it still runs, and the +// layout that scan resolves is still published. Both halves are load-bearing, and the second +// one is the half that would rot silently — a suspend that also stopped scanning would leave +// status.placement frozen at whatever the folder looked like when suspension began, so the dry +// run would show a stale answer with no way to tell. + +// suspendedTarget is a resolved target metadata record with suspend set, writing at base. +func suspendedTarget(suspend bool) ResolvedTargetMetadata { + return ResolvedTargetMetadata{ + Name: "checkout", + Namespace: "shop", + Suspend: suspend, + } +} + +// captureLayout installs a reporter that records every published report. +func captureLayout(w *BranchWorker) *[]LayoutReport { + var reports []LayoutReport + w.layoutReporter = func(_ types.ResourceReference, report LayoutReport) { + reports = append(reports, report) + } + return &reports +} + +// A suspended target folds no event into the worktree: the file the event would have created +// is not there, and the flush reports no change, so nothing is committed and nothing is pushed. +func TestSuspend_LiveWriteCreatesNothing(t *testing.T) { + worktree := newWorktreeForTest(t) + root := worktree.Filesystem().Root() + worker := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} + + event := newConfigMapEvent("cache", "app") + targets := map[pendingTargetKey]ResolvedTargetMetadata{ + {}: suspendedTarget(true), + } + + changed, err := worker.applyPendingWriteEvents(t.Context(), repoFor(t, worktree), worktree, []Event{event}, targets) + + require.NoError(t, err) + assert.False(t, changed, "a suspended target must report no change, so no commit is created") + _, statErr := os.Stat(filepath.Join(root, "app", "configmaps", "cache.yaml")) + assert.True(t, os.IsNotExist(statErr), "a suspended target must write no file") +} + +// The same event against the same target, not suspended, does create the file. Without this +// the test above would pass just as well if placement were broken. +func TestSuspend_UnsuspendedTargetStillWrites(t *testing.T) { + worktree := newWorktreeForTest(t) + root := worktree.Filesystem().Root() + worker := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} + + targets := map[pendingTargetKey]ResolvedTargetMetadata{ + {}: suspendedTarget(false), + } + changed, err := worker.applyPendingWriteEvents( + t.Context(), repoFor(t, worktree), worktree, []Event{newConfigMapEvent("cache", "app")}, targets) + + require.NoError(t, err) + assert.True(t, changed) + _, statErr := os.Stat(filepath.Join(root, "app", "configmaps", "cache.yaml")) + assert.NoError(t, statErr, "an active target writes the file the suspended one withheld") +} + +// Suspend's cutover, pinned. The gate reads the value CAPTURED when the write was planned, not +// the live GitTarget, so a suspension arriving after planning does not retract the write — and a +// write already committed locally is still pushed by the retained-writes path, which never +// consults suspend at all. +// +// That is the contract rather than a gap in it: a local commit that is never pushed would sit in +// the worker's checkout indefinitely and surface later, out of order, on resume. Suspend is a +// valve on new work. This test exists so that reading is a decision the suite defends rather than +// an accident of where the flag happens to be read. +func TestSuspend_IsTheValueCapturedWhenTheWriteWasPlanned(t *testing.T) { + worktree := newWorktreeForTest(t) + root := worktree.Filesystem().Root() + worker := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} + + // The metadata this write was planned under: not suspended. The GitTarget in the cluster may + // have been suspended since, and this write does not care. + planned := suspendedTarget(false) + targets := map[pendingTargetKey]ResolvedTargetMetadata{{}: planned} + + changed, err := worker.applyPendingWriteEvents( + t.Context(), repoFor(t, worktree), worktree, []Event{newConfigMapEvent("cache", "app")}, targets) + + require.NoError(t, err) + assert.True(t, changed, "a write planned before suspension still lands") + _, statErr := os.Stat(filepath.Join(root, "app", "configmaps", "cache.yaml")) + assert.NoError(t, statErr) +} + +// The half that keeps a stopped target's status fresh rather than frozen: the suspended target still +// scans, so it still publishes what its folder resolved to: a valve that stopped looking as well +// as writing would freeze status.placement at whatever the folder looked like when someone +// panicked, which is exactly when a stale answer costs the most. +func TestSuspend_StillPublishesTheResolvedLayout(t *testing.T) { + worktree := newWorktreeForTest(t) + root := worktree.Filesystem().Root() + seedFile(t, root, "kustomization.yaml", + "apiVersion: kustomize.config.k8s.io/v1beta1\nkind: Kustomization\n"+ + "namespace: app\nresources:\n- web.yaml\n") + seedFile(t, root, "web.yaml", + "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: web\ndata:\n k: v\n") + + worker := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} + reports := captureLayout(worker) + targets := map[pendingTargetKey]ResolvedTargetMetadata{ + {}: suspendedTarget(true), + } + + _, err := worker.applyPendingWriteEvents( + t.Context(), repoFor(t, worktree), worktree, []Event{newConfigMapEvent("cache", "app")}, targets) + require.NoError(t, err) + + require.Len(t, *reports, 1, "a suspended target must still publish what it resolved") + report := (*reports)[0] + assert.Equal(t, manifestanalyzer.LayoutSingleKustomization, report.Reason) + assert.Equal(t, ".", report.RenderRoot) + assert.Equal(t, manifestanalyzer.LayoutModeKustomizeRoot, report.Mode) +} + +// The report is a fact about the folder, so it does not wait for a placement to happen: it is +// published by the scan that precedes the write, which is the property status.placement rests +// on and the one a later refactor is most likely to break. +func TestLayoutReport_PublishedBeforeAnythingIsWritten(t *testing.T) { + worktree := newWorktreeForTest(t) + worker := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} + reports := captureLayout(worker) + + // An empty folder: no kustomization, no documents, and no write has ever happened here. + err := worker.refuseUnsafeWorktree(t.Context(), worktree, "", suspendedTarget(true)) + require.NoError(t, err) + + require.Len(t, *reports, 1) + assert.Equal(t, manifestanalyzer.LayoutNone, (*reports)[0].Reason, + "an empty folder resolves to the canonical ladder, and says so before it is written to") +} + +// An unattributable batch publishes nothing rather than filing a report under a key no +// GitTarget reads. The CLI and most unit tests are exactly this case. +func TestLayoutReport_UnattributableBatchPublishesNothing(t *testing.T) { + worktree := newWorktreeForTest(t) + worker := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} + reports := captureLayout(worker) + + err := worker.refuseUnsafeWorktree(t.Context(), worktree, "", ResolvedTargetMetadata{Path: ""}) + require.NoError(t, err) + + assert.Empty(t, *reports, "a batch that names no GitTarget must publish no report") +} + +// The report is a fact about the FOLDER and about nothing else. The target's declared placement +// policy does not reach it: a reader who wants to know what was declared reads the spec in the +// same GET, and a status field that copied it would be a second place to look that can disagree +// with the first. +func TestLayoutReport_DoesNotRestateTheDeclaredPolicy(t *testing.T) { + worktree := newWorktreeForTest(t) + worker := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} + reports := captureLayout(worker) + + target := suspendedTarget(true) + target.Placement = &manifestanalyzer.PlacementPolicy{ + ByType: map[string]string{"v1/secrets": "secrets/{name}.yaml"}, + } + require.NoError(t, worker.refuseUnsafeWorktree(t.Context(), worktree, "", target)) + + require.Len(t, *reports, 1) + report := (*reports)[0] + assert.Equal(t, manifestanalyzer.LayoutNone, report.Reason) + assert.Equal(t, manifestanalyzer.LayoutModePlain, report.Mode, + "an empty folder is written as plain files whatever the target declares") + assert.Empty(t, report.RenderRoot) + assert.Empty(t, report.ReadOnlyBases) +} + +func repoFor(t *testing.T, worktree *gogit.Worktree) *gogit.Repository { + t.Helper() + repo, err := gogit.PlainOpen(worktree.Filesystem().Root()) + require.NoError(t, err) + return repo +} + +// bootstrapEnabledTarget is a target at base that stages its path's bootstrap files — +// .gittargetignore always, .sops.yaml because the recipient makes the SOPS half renderable. +func bootstrapEnabledTarget(name, base string, suspend bool) ResolvedTargetMetadata { + return ResolvedTargetMetadata{ + Name: name, + Namespace: "shop", + Path: base, + Suspend: suspend, + BootstrapOptions: pathBootstrapOptions{ + Enabled: true, + IncludeSOPSConfig: true, + TemplateData: bootstrapTemplateData{AgeRecipients: []string{"age1exampleexampleexample"}}, + }, + } +} + +func bootstrapEventFor(md ResolvedTargetMetadata, name string) Event { + event := newConfigMapEvent(name, "app") + event.Path = md.Path + event.BootstrapOptions = md.BootstrapOptions + return event +} + +// A suspended target leaves NOTHING behind, including files it did not author as content. +// +// Bootstrap staging writes .gittargetignore and .sops.yaml into the target's path and adds +// them to the index — and the index belongs to the branch, not to one target. Staging it for a +// suspended path therefore used to smuggle those files into the next ACTIVE target's commit, +// so a target that was supposed to be writing nothing appeared in history with two files in +// its folder. This asserts both halves: nothing on disk, and nothing in the commit. +func TestSuspend_StagesNoBootstrapFilesIntoTheNextCommit(t *testing.T) { + worktree := newWorktreeForTest(t) + root := worktree.Filesystem().Root() + repo := repoFor(t, worktree) + worker := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} + + suspended := bootstrapEnabledTarget("suspended", "suspended", true) + active := bootstrapEnabledTarget("active", "active", false) + targets := map[pendingTargetKey]ResolvedTargetMetadata{ + {Name: suspended.Name, Namespace: suspended.Namespace}: suspended, + {Name: active.Name, Namespace: active.Namespace}: active, + } + + changed, err := worker.applyPendingWriteEvents(t.Context(), repo, worktree, []Event{ + bootstrapEventFor(suspended, "cache"), + bootstrapEventFor(active, "cache"), + }, targets) + require.NoError(t, err) + require.True(t, changed, "the active target wrote, so this window commits") + + // The active target commits. Anything the suspended path left in the index rides along. + hash, err := worktree.Commit("active target write", &gogit.CommitOptions{ + Author: &object.Signature{Name: "t", Email: "t@example.com", When: time.Now()}, + }) + require.NoError(t, err) + + for _, name := range []string{gitTargetIgnoreFileName, sopsConfigFileName} { + _, statErr := os.Stat(filepath.Join(root, "suspended", name)) + assert.True(t, os.IsNotExist(statErr), + "a suspended target must not have %s written into its folder", name) + assert.False(t, commitHoldsPath(t, repo, hash, "suspended/"+name), + "a suspended target's %s must not reach a commit", name) + } + + // The control: the active target got both, so the assertions above are about suspend + // rather than about bootstrap staging having quietly stopped working. + for _, name := range []string{gitTargetIgnoreFileName, sopsConfigFileName} { + assert.True(t, commitHoldsPath(t, repo, hash, "active/"+name), + "the active target's %s must still be committed", name) + } +} + +// commitHoldsPath reports whether a commit's tree holds a path. +func commitHoldsPath(t *testing.T, repo *gogit.Repository, hash plumbing.Hash, path string) bool { + t.Helper() + commit, err := repo.CommitObject(hash) + require.NoError(t, err) + tree, err := commit.Tree() + require.NoError(t, err) + if _, err := tree.File(path); err != nil { + require.ErrorIs(t, err, object.ErrFileNotFound) + return false + } + return true +} diff --git a/internal/git/types.go b/internal/git/types.go index 03effb19..def5b4b5 100644 --- a/internal/git/types.go +++ b/internal/git/types.go @@ -263,6 +263,18 @@ type ResolvedTargetMetadata struct { // documents' GVK->GVR against that cluster's registry, so a folder mirroring a remote is swept // against the right cluster's mapping. SourceCluster string + + // Suspend is the GitTarget's spec.suspend, captured with the rest of its metadata so a write + // replayed after a rebase honours the policy it was planned under. It suppresses the write + // only: the scan that precedes it still runs, and the layout that scan resolves is still + // published, which is what keeps a suspended target's status fresh while it is stopped. + // + // Being CAPTURED is what defines suspend's cutover: it is the value as of planning, so a + // suspension that arrives after this write was planned does not retract it, and a write + // already committed locally is still pushed. Reading the live GitTarget at push time instead + // would strand that commit in the worker's checkout, to surface later and out of order on + // resume. See the field's doc on GitTargetSpec. + Suspend bool } // PendingWrite is the unit retained until a push succeeds. diff --git a/internal/git/worker_manager.go b/internal/git/worker_manager.go index 738da226..d6215d51 100644 --- a/internal/git/worker_manager.go +++ b/internal/git/worker_manager.go @@ -53,6 +53,11 @@ type WorkerManager struct { // CLI and in tests that do not assert on the status transition. pathRefusal PathRefusalReporter + // layoutReporter publishes each scan's resolved folder layout to the GitTarget status + // surface. Set once at startup (SetLayoutReporter) before any worker is created; nil in the + // CLI and in tests that do not assert on status.placement. + layoutReporter LayoutReporter + // renderFidelityGate is shared by every worker and the watch manager. It is created with the // manager so a target's state survives workers being recreated for the same branch. renderFidelityGate *RenderFidelityGate @@ -123,6 +128,16 @@ func (m *WorkerManager) SetPathRefusalReporter(reporter PathRefusalReporter) { m.pathRefusal = reporter } +// SetLayoutReporter injects the hook every worker calls after a scan resolves a target's +// folder layout, so status.placement and LayoutResolved reflect the folder rather than being +// computed and dropped. Like SetPathRefusalReporter, it is called once at startup before any +// worker is created. +func (m *WorkerManager) SetLayoutReporter(reporter LayoutReporter) { + m.mu.Lock() + defer m.mu.Unlock() + m.layoutReporter = reporter +} + // RegisterTarget ensures a worker exists for the target's (provider, branch) // and registers the target with that worker. // This is called by GitTarget controller when a target becomes Ready. @@ -182,6 +197,7 @@ func (m *WorkerManager) EnsureWorker( worker.clusterMapper = m.clusterMapper worker.sshHostKeys = m.sshHostKeys worker.pathRefusal = m.pathRefusal + worker.layoutReporter = m.layoutReporter worker.renderFidelityGate = m.renderFidelityGate if err := worker.Start(m.ctx); err != nil { diff --git a/internal/layoutfixture/layoutfixture.go b/internal/layoutfixture/layoutfixture.go new file mode 100644 index 00000000..3f768cbf --- /dev/null +++ b/internal/layoutfixture/layoutfixture.go @@ -0,0 +1,63 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Package layoutfixture reads the expected-*-status.yaml fixtures under docs/layout. +// +// It exists because those fixtures are asserted from two packages that cannot share a test +// helper: internal/git pins the half a refusal produces (GitPathAccepted), and +// internal/controller pins the half the controller projects from it (LayoutResolved, Stalled). +// A fixture parser copied into both would be two things that must agree about a file format, +// which is the drift the fixtures exist to prevent. +package layoutfixture + +import ( + "fmt" + "os" + "path/filepath" + + "sigs.k8s.io/yaml" +) + +// Root is docs/layout as reached from a package directory two levels below the repository root +// (internal/git, internal/controller). The fixtures are read in place rather than copied into a +// testdata directory: a copy would drift from the documents it illustrates, and the drift would +// be invisible in review. +const Root = "../../docs/layout" + +// Condition is one expected condition in a fixture. +type Condition struct { + Type string `json:"type"` + Status string `json:"status"` + Reason string `json:"reason"` + Message string `json:"message"` +} + +type statusFixture struct { + Status struct { + Conditions []Condition `json:"conditions"` + } `json:"status"` +} + +// Path joins a fixture path onto Root. +func Path(elem ...string) string { + return filepath.Join(append([]string{Root}, elem...)...) +} + +// ReadCondition returns the named condition from a status fixture. A missing condition is an +// error rather than a zero value: a test asking for one is asserting that the fixture makes that +// claim, and a silently absent claim is exactly what would let the fixture rot. +func ReadCondition(path, conditionType string) (Condition, error) { + raw, err := os.ReadFile(path) + if err != nil { + return Condition{}, err + } + var fixture statusFixture + if err := yaml.Unmarshal(raw, &fixture); err != nil { + return Condition{}, fmt.Errorf("parsing %s: %w", path, err) + } + for _, condition := range fixture.Status.Conditions { + if condition.Type == conditionType { + return condition, nil + } + } + return Condition{}, fmt.Errorf("%s has no %s condition, so nothing pins it", path, conditionType) +} diff --git a/internal/manifestanalyzer/acceptance_refusal.go b/internal/manifestanalyzer/acceptance_refusal.go index f7125596..d33b658e 100644 --- a/internal/manifestanalyzer/acceptance_refusal.go +++ b/internal/manifestanalyzer/acceptance_refusal.go @@ -54,6 +54,37 @@ func (e *AcceptanceRefusedError) AllIssuesOfKinds(kinds ...IssueKind) bool { return true } +// GitPathRefusalReason maps a refusal onto the GitPathAccepted condition reason it is published +// under. A refusal made up PURELY of one recognised shape gets that shape's own reason; any mix +// falls back to the umbrella UnsupportedContent, because a mixed refusal has no single answer and +// naming one of its halves would send the reader to the wrong fix. +// +// It lives here, beside the IssueKind constants it reads, rather than in the watch package that +// publishes it. Both the projection and the corpus that pins the fixtures need the same answer, +// and the corpus cannot import watch (watch imports git). Before this the mapping was in watch +// with three comments elsewhere asking callers to keep their strings in sync with it by hand. +// +// The strings mirror the controller's GitTargetReason* constants — neither package can import +// controller without a cycle — and every one of them is a member of the controller's +// stalled-reason set, so a refusal surfaces as Stalled=True / kstatus Failed whichever it is. +func GitPathRefusalReason(refused *AcceptanceRefusedError) string { + switch { + case refused.AllIssuesOfKinds(IssueIgnoreShadowsManaged): + return "IgnoreShadowsManagedPath" + case refused.AllIssuesOfKinds(IssueAmbiguousLayout): + return "AmbiguousLayout" + case refused.AllIssuesOfKinds( + IssueWriteEscapesScope, + IssueWriteFanIn, + IssueRenderRefused, + IssueUnplaceableEdit, + ): + return "WriteBoundaryRefused" + default: + return "UnsupportedContent" + } +} + // RefusalError returns an *AcceptanceRefusedError when the acceptance was not accepted, or // nil when the folder is clean. The writer calls this immediately after running the gate, so // a refusal aborts the commit before any file is touched. diff --git a/internal/manifestanalyzer/analyzer_test.go b/internal/manifestanalyzer/analyzer_test.go index d9d52b97..2965dc45 100644 --- a/internal/manifestanalyzer/analyzer_test.go +++ b/internal/manifestanalyzer/analyzer_test.go @@ -114,6 +114,7 @@ func TestAnalyze_Issues(t *testing.T) { IssueOutOfScope: 0, IssueUnsupportedKustomize: 0, IssueRenderDoesNotMatchLive: 0, + IssueAmbiguousLayout: 0, // Foreign-content, ignore-shadow, and the write-boundary refusals are // acceptance-gate / write-plan facts, not part of the structure-only Analyze report, // so they never surface here. IssueRenderRefused is the strongest case of that: it is diff --git a/internal/manifestanalyzer/layout.go b/internal/manifestanalyzer/layout.go new file mode 100644 index 00000000..0179fc3e --- /dev/null +++ b/internal/manifestanalyzer/layout.go @@ -0,0 +1,241 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "fmt" + "sort" + "strings" +) + +// This file is the post-scan layout resolution: what the folder's shape implies about where +// new documents go and whether they carry their own namespace, computed from a scan and from +// nothing else. +// +// It reports the ladder rung placement WILL take rather than the one it took, so a refusal or a +// surprising destination can be explained from status instead of from the logs. It is not a +// preview of the target's output: to see that, point a target at a scratch branch and read the +// commits (docs/layout/model.md § "Previewing a target: point it at a scratch branch"). +// +// Nothing here depends on a placement having happened. See +// docs/layout/model.md § "status.placement, and the post-scan pass". + +// LayoutReason is the resolved shape of a GitTarget folder, and it is a condition reason on +// LayoutResolved rather than a status field: every consumer in this ecosystem already reads +// reasons from conditions, and a bespoke field would be a second place to look. +type LayoutReason string + +const ( + // LayoutSingleKustomization is a folder governed by exactly one supported, writable + // kustomization. A new document lands beside it and joins its resources:. + LayoutSingleKustomization LayoutReason = "SingleKustomization" + // LayoutAmbiguous is a folder covering more than one render root. Placement declines to + // pick one rather than guessing, so the folder is not a write partition: point the target + // at a leaf instead. See docs/layout/shapes/README.md § "Why only a leaf can be a + // kustomize target". + LayoutAmbiguous LayoutReason = "Ambiguous" + // LayoutNone is a folder with no supported kustomization at all. New documents land at a + // declared template's path, or at the built-in canonical path. + LayoutNone LayoutReason = "None" +) + +// LayoutResolution is what one scan resolved about a folder's layout. +type LayoutResolution struct { + // Reason is the verdict, projected onto the LayoutResolved condition. + Reason LayoutReason + // Mode is how the folder is written — plain files, a self-contained kustomize root, or an + // overlay over a base outside the write scope. Empty under Ambiguous, which has no single + // answer. + Mode LayoutMode + // RenderRoot is the governing kustomization's directory relative to the write scope, "." + // for the scope itself. Empty for every reason but LayoutSingleKustomization. + RenderRoot string + // RenderRoots is every writable render root the scan found, sorted. It is what makes an + // Ambiguous message able to name the folders it actually covers instead of only counting + // them. + RenderRoots []string + // ReadOnlyBases is every kustomization the scan holds that lies OUTSIDE the write scope, + // relative to it, sorted. Non-empty exactly when Mode is LayoutModeKustomizeOverlay, and it + // is what a WriteBoundaryRefused message is predictable from. + ReadOnlyBases []string +} + +// LayoutMode is how a folder is written. It mirrors v1alpha3.PlacementMode, which is the +// published form; the duplication is the usual one-way dependency rule — the analyzer does not +// import the API types. +type LayoutMode string + +const ( + // LayoutModePlain is a folder no kustomization governs. + LayoutModePlain LayoutMode = "Plain" + // LayoutModeKustomizeRoot is a self-contained folder governed by exactly one kustomization. + LayoutModeKustomizeRoot LayoutMode = "KustomizeRoot" + // LayoutModeKustomizeOverlay is a folder governed by one kustomization that renders a base + // outside the write scope. + LayoutModeKustomizeOverlay LayoutMode = "KustomizeOverlay" +) + +// ResolveLayout resolves a folder's layout from a scan. +// +// writeScope is the write jail relative to the scanned root — empty for a self-contained +// subtree, non-empty only when render-root scoping anchored the scan past spec.path into a base +// the folder renders. Read scope is wider than write scope, always, so a base above the jail is +// read to render the folder and is never a candidate root for it. +func ResolveLayout(store *ManifestStore, writeScope string) LayoutResolution { + roots := writableRenderRoots(store, writeScope) + bases := readOnlyBases(store, writeScope) + resolution := LayoutResolution{RenderRoots: roots, ReadOnlyBases: bases} + + switch { + case len(roots) == 0: + resolution.Reason = LayoutNone + resolution.Mode = LayoutModePlain + case len(roots) == 1: + resolution.Reason = LayoutSingleKustomization + resolution.RenderRoot = relativeToScope(roots[0], writeScope) + // The presence of a base OUTSIDE the write scope is what separates an overlay from a + // self-contained root, and it is the same condition every write-boundary refusal turns + // on — so the two cannot disagree about which folder is which. + if len(bases) > 0 { + resolution.Mode = LayoutModeKustomizeOverlay + } else { + resolution.Mode = LayoutModeKustomizeRoot + } + default: + resolution.Reason = LayoutAmbiguous + // Mode is deliberately empty: with several roots there is no single answer, and asserting + // a folder-wide one would be the guess the Ambiguous verdict exists to refuse. + } + + return resolution +} + +// readOnlyBases is every supported kustomization the scan holds that lies outside the write +// jail, relative to it and sorted. +// +// It is the complement of writableRenderRoots over the same store, which is what makes +// "non-empty exactly when the folder is an overlay" true by construction rather than by +// agreement between two rules. +func readOnlyBases(store *ManifestStore, writeScope string) []string { + if writeScope == "" { + return nil // nothing was scanned above the jail, so nothing can be outside it + } + var bases []string + for dir, k := range store.Kustomizations { + if k.Unsupported || pathWithin(slashDir(k.Path), writeScope) { + continue + } + bases = append(bases, relativeFromScope(dir, writeScope)) + } + sort.Strings(bases) + return bases +} + +// writableRenderRoots is the predicate resolveKustomizeRoot resolves against, lifted out so the +// reported layout and the taken layout cannot drift: LayoutResolved must describe the rung +// placement WILL take, and the only way to guarantee that is for both to ask one function. +// +// A root is a supported kustomization inside the write jail. Under render-root scoping the scan +// also holds the read-only bases the folder renders; those are not candidates, which is what +// lets an overlay resolve to its own single root instead of counting its base as a second one. +func writableRenderRoots(store *ManifestStore, writeScope string) []string { + roots := make([]string, 0, len(store.Kustomizations)) + for dir, k := range store.Kustomizations { + if k.Unsupported { + continue + } + if writeScope != "" && !pathWithin(slashDir(k.Path), writeScope) { + continue + } + roots = append(roots, dir) + } + sort.Strings(roots) + return roots +} + +// relativeToScope expresses a scanned-root-relative directory relative to the write jail, so a +// reported renderRoot is relative to spec.path exactly as placement paths are documented to be. +func relativeToScope(dir, writeScope string) string { + if writeScope == "" { + return orDot(dir) + } + if dir == writeScope { + return "." + } + return orDot(trimScopePrefix(dir, writeScope)) +} + +func trimScopePrefix(dir, writeScope string) string { + prefix := writeScope + "/" + if len(dir) > len(prefix) && dir[:len(prefix)] == prefix { + return dir[len(prefix):] + } + return dir +} + +// relativeFromScope expresses a directory OUTSIDE the write jail relative to it, so a base the +// folder renders reads as "../../base" — the same way it is spelled in the overlay's own +// resources:, and the same way the write-boundary refusal names it. A base is always above the +// jail, never beside it, because the jail is what the scan was anchored past to reach it. +func relativeFromScope(dir, writeScope string) string { + scopeParts := strings.Split(writeScope, "/") + dirParts := strings.Split(dir, "/") + common := 0 + for common < len(scopeParts) && common < len(dirParts) && scopeParts[common] == dirParts[common] { + common++ + } + up := strings.Repeat("../", len(scopeParts)-common) + return orDot(up + strings.Join(dirParts[common:], "/")) +} + +func orDot(dir string) string { + if dir == "" { + return "." + } + return dir +} + +// IssueAmbiguousLayout marks a GitTarget folder covering more than one render root. It is a +// property of the OBSERVED folder rather than of the spec, so no CEL rule and no admission +// check can reach it — the folder is only ambiguous once the operator has read it. +const IssueAmbiguousLayout IssueKind = "ambiguous-layout" + +// AmbiguousLayoutRefusal is the write-path form of the Ambiguous verdict: a folder covering +// several render roots is refused rather than silently placed into whichever one an arbitrary +// rule picks. +// +// It refuses at the WRITE rather than at the GitTarget's Validated gate, and the difference is +// recoverability. Validated is evaluated before the data plane exists, so a target failing it +// never registers a worker, never scans, and could therefore never observe that the folder had +// been fixed — the refusal would be permanent, and for a target that had never scanned it could +// never fire in the first place. Refusing here keeps the target declared and scanning: the +// periodic resync re-reads the folder, so splitting the target down to a leaf overlay clears the +// refusal the same way fixing any other unsupported content does. +// +// It returns no issue for a folder that is not ambiguous, so the caller can raise it +// unconditionally. +func AmbiguousLayoutRefusal(resolution LayoutResolution, specPath string) []AcceptanceIssue { + if resolution.Reason != LayoutAmbiguous { + return nil + } + // Every path in a refusal is relative to the write jail, so the folder's own name is "." — + // which reads as nothing at all in a message. Say what "." IS instead; the roots below are + // relative to it and are what makes the message actionable. + return []AcceptanceIssue{{ + Kind: IssueAmbiguousLayout, + Path: orDot(specPath), + Message: fmt.Sprintf( + "the GitTarget path covers %d kustomize render roots (%s), so there is no single one "+ + "to place new documents into; point the GitTarget at one of them instead", + len(resolution.RenderRoots), strings.Join(resolution.RenderRoots, ", ")), + // The PLATFORM OPERATOR fixes it, because the remedy is a GitTarget edit rather than a + // repository one: the folder is a perfectly good base-plus-overlays tree, and what is + // wrong is the scope pointed at it. That is this actor's definition — the GitTarget's + // scope and path — and misfiling it would send the one actionable instruction we have + // ("point the GitTarget at one of them") to someone who does not own the object it + // names. It is solvable and it is not a support boundary. See + // docs/layout/shapes/README.md, "Why only a leaf can be a kustomize target". + Solvable: true, + Actor: ActorPlatformOperator, + }} +} diff --git a/internal/manifestanalyzer/layout_test.go b/internal/manifestanalyzer/layout_test.go new file mode 100644 index 00000000..c2b956ce --- /dev/null +++ b/internal/manifestanalyzer/layout_test.go @@ -0,0 +1,120 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "context" + "testing" + "testing/fstest" + + "github.com/stretchr/testify/assert" + + "github.com/ConfigButler/gitops-reverser/internal/typeset" +) + +// layoutStore builds a store over an in-memory folder, using the same registry the rest of +// this package's tests resolve types against. +func layoutStore(t *testing.T, files map[string]string) *ManifestStore { + t.Helper() + fsys := fstest.MapFS{} + for name, body := range files { + fsys[name] = &fstest.MapFile{Data: []byte(body)} + } + return BuildStore(context.Background(), fsys, typeset.NewSnapshotRegistry(sampleClusterSnapshot())) +} + +const layoutRootWithNamespace = "apiVersion: kustomize.config.k8s.io/v1beta1\n" + + "kind: Kustomization\nnamespace: shop\nresources:\n - web.yaml\n" + +const layoutWebDoc = "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: web\ndata:\n k: v\n" + +// A self-contained folder with one supported kustomization resolves to that root, and to +// KustomizeRoot rather than KustomizeOverlay: it renders nothing it may not write to. +func TestResolveLayout_SingleKustomizationRoot(t *testing.T) { + store := layoutStore(t, map[string]string{ + "kustomization.yaml": layoutRootWithNamespace, + "web.yaml": layoutWebDoc, + }) + + got := ResolveLayout(store, "") + + assert.Equal(t, LayoutSingleKustomization, got.Reason) + assert.Equal(t, LayoutModeKustomizeRoot, got.Mode) + assert.Equal(t, ".", got.RenderRoot) + assert.Empty(t, got.ReadOnlyBases, "nothing outside the write scope was scanned") +} + +// A folder with no kustomization at all resolves to None and to Plain: the ladder falls +// through to a declared template or the canonical path, and no file is registered anywhere. +func TestResolveLayout_NoKustomization(t *testing.T) { + store := layoutStore(t, map[string]string{"web.yaml": layoutWebDoc}) + + got := ResolveLayout(store, "") + + assert.Equal(t, LayoutNone, got.Reason) + assert.Equal(t, LayoutModePlain, got.Mode) + assert.Empty(t, got.RenderRoot) + assert.Empty(t, got.ReadOnlyBases) +} + +// The rule this PR ships. A target covering two overlays covers two render roots, so there is +// no single answer: renderRoot is empty rather than an arbitrary pick, and mode is empty +// rather than one of the two roots' answers. +func TestResolveLayout_TwoRenderRootsIsAmbiguous(t *testing.T) { + store := layoutStore(t, map[string]string{ + "overlays/prod/kustomization.yaml": "apiVersion: kustomize.config.k8s.io/v1beta1\n" + + "kind: Kustomization\nnamespace: shop-prod\nresources:\n - cm.yaml\n", + "overlays/prod/cm.yaml": layoutWebDoc, + "overlays/test/kustomization.yaml": "apiVersion: kustomize.config.k8s.io/v1beta1\n" + + "kind: Kustomization\nnamespace: shop-test\nresources:\n - cm.yaml\n", + "overlays/test/cm.yaml": layoutWebDoc, + }) + + got := ResolveLayout(store, "") + + assert.Equal(t, LayoutAmbiguous, got.Reason) + assert.Empty(t, got.RenderRoot, "an ambiguous folder must not report one of its roots as THE root") + assert.Empty(t, got.Mode, "with two roots there is no single way the folder is written") + assert.Equal(t, []string{"overlays/prod", "overlays/test"}, got.RenderRoots, + "the roots are named so the message can say what the folder actually covers") +} + +// Render-root scoping: a leaf overlay that reads a base outside spec.path is scanned from the +// common ancestor, so the store holds both kustomizations. Only the one inside the write jail +// is a candidate — read scope is wider than write scope — so the leaf resolves to its own +// single root, and the base it renders is reported as read-only rather than as a second root. +// +// The base is spelled the way the overlay's own resources: spells it, which is also how the +// write-boundary refusal names it. +func TestResolveLayout_BaseOutsideTheWriteJailIsReadOnly(t *testing.T) { + store := layoutStore(t, map[string]string{ + "base/kustomization.yaml": "apiVersion: kustomize.config.k8s.io/v1beta1\n" + + "kind: Kustomization\nresources:\n - deployment.yaml\n", + "base/deployment.yaml": layoutWebDoc, + "overlays/prod/kustomization.yaml": "apiVersion: kustomize.config.k8s.io/v1beta1\n" + + "kind: Kustomization\nnamespace: shop-prod\nresources:\n - ../../base\n", + }) + + got := ResolveLayout(store, "overlays/prod") + + assert.Equal(t, LayoutSingleKustomization, got.Reason) + assert.Equal(t, LayoutModeKustomizeOverlay, got.Mode) + assert.Equal(t, ".", got.RenderRoot, "the leaf's own root, expressed relative to spec.path") + assert.Equal(t, []string{"../../base"}, got.ReadOnlyBases) +} + +// Mode separates the two kustomize shapes on exactly the condition every write-boundary +// refusal turns on, so the two cannot disagree about which folder is which: an overlay is a +// root that renders something outside its write scope, and nothing else. +func TestResolveLayout_ModeSeparatesOverlayFromSelfContainedRoot(t *testing.T) { + files := map[string]string{ + "apps/checkout/kustomization.yaml": layoutRootWithNamespace, + "apps/checkout/web.yaml": layoutWebDoc, + } + + scoped := ResolveLayout(layoutStore(t, files), "apps/checkout") + + assert.Equal(t, LayoutModeKustomizeRoot, scoped.Mode, + "a scan anchored at the folder itself holds nothing above it, so nothing is read-only") + assert.Empty(t, scoped.ReadOnlyBases) +} diff --git a/internal/manifestanalyzer/placement.go b/internal/manifestanalyzer/placement.go index 868d0119..03cca4c8 100644 --- a/internal/manifestanalyzer/placement.go +++ b/internal/manifestanalyzer/placement.go @@ -211,27 +211,17 @@ func LocateNew(store *ManifestStore, policy *PlacementPolicy, req PlacementReque // than one supported kustomization under the scanned root is ambiguous and declines // rather than guessing. That is why this survived the Option C deletion — deleting it // would reintroduce the unreachable-file bug it was added to fix. +// +// The "exactly one writable supported kustomization" predicate lives in writableRenderRoots +// (layout.go), because status.placement reports the rung this function will take and the two +// must not be able to drift: a LayoutResolved that says SingleKustomization while placement +// declines here would be worse than no report at all. func resolveKustomizeRoot(store *ManifestStore, req PlacementRequest) (string, bool) { - var only *KustomizationInfo - for _, k := range store.Kustomizations { - if k.Unsupported { - continue - } - // Under render-root scoping the scan also holds the read-only base kustomizations, so - // "one supported kustomization" must mean one WRITABLE one. Skipping the out-of-jail - // bases lets an overlay resolve to its own single root (and a new object land beside it, - // governed) instead of declining as ambiguous because the base counts as a second root. - if req.WriteScope != "" && !pathWithin(slashDir(k.Path), req.WriteScope) { - continue - } - if only != nil { - return "", false - } - only = k - } - if only == nil { + roots := writableRenderRoots(store, req.WriteScope) + if len(roots) != 1 { return "", false } + only := store.Kustomizations[roots[0]] name := req.Identifier.Name + ".yaml" if req.Sensitive { name = req.Identifier.Name + ".sops.yaml" diff --git a/internal/manifestanalyzer/solvable.go b/internal/manifestanalyzer/solvable.go index df9a3df3..2c56642b 100644 --- a/internal/manifestanalyzer/solvable.go +++ b/internal/manifestanalyzer/solvable.go @@ -20,10 +20,13 @@ import ( // CLUSTER-AWARENESS is the gate, and it is structural rather than incidental. A // STRUCTURE-ONLY scan — one whose [typeset.Lookup] is not ready, which is every // ScanFolder and ScanRepo — reports only [ActorUnknown] or [ActorRepositoryAuthor]. It -// cannot reach [ActorPlatformOperator] through either of that value's two acceptance -// sites: [IssueOutOfScope] needs a declared AcceptancePolicy.InScope, and +// cannot reach [ActorPlatformOperator] through any of that value's three acceptance +// sites: [IssueOutOfScope] needs a declared AcceptancePolicy.InScope, // [IssueUnresolvedKRM] needs MappingNotFollowable, which a not-ready registry never -// produces because it resolves every document to MappingNoSource instead. +// produces because it resolves every document to MappingNoSource instead, and +// [IssueAmbiguousLayout] is raised only by the live write path ([AmbiguousLayoutRefusal] +// has one caller, in the flush planner) — a scan resolves the layout but never refuses on +// it. // // So a consumer of a structure-only report has a platform-operator branch that never // fires, and that is by design: a scan that cannot see the cluster cannot know a CRD is diff --git a/internal/manifestanalyzer/solvable_test.go b/internal/manifestanalyzer/solvable_test.go index c8126c30..f229ad71 100644 --- a/internal/manifestanalyzer/solvable_test.go +++ b/internal/manifestanalyzer/solvable_test.go @@ -56,6 +56,7 @@ var classificationByKind = map[IssueKind][]Classification{ IssueForeignSubmodule: {{Solvable: true, Actor: ActorRepositoryAuthor}}, IssueOutOfScope: {{Solvable: true, Actor: ActorPlatformOperator}}, IssueWriteEscapesScope: {{Solvable: true, Actor: ActorPlatformOperator}}, + IssueAmbiguousLayout: {{Solvable: true, Actor: ActorPlatformOperator}}, IssueRenderDoesNotMatchLive: {{Solvable: true, Actor: ActorPlatformOperator}}, IssueWriteFanIn: {{Solvable: false}}, IssueUnplaceableEdit: {{Solvable: false}}, diff --git a/internal/watch/event_router.go b/internal/watch/event_router.go index 5e9a6bde..5908ba1e 100644 --- a/internal/watch/event_router.go +++ b/internal/watch/event_router.go @@ -346,19 +346,7 @@ func renderFidelityDivergence(refused *manifestanalyzer.AcceptanceRefusedError) // controller without a cycle), and all three are members of the controller's stalled-reason // set, so every refusal surfaces as Stalled=True / kstatus Failed. func gitPathRefusalReason(refused *manifestanalyzer.AcceptanceRefusedError) string { - switch { - case refused.AllIssuesOfKinds(manifestanalyzer.IssueIgnoreShadowsManaged): - return "IgnoreShadowsManagedPath" - case refused.AllIssuesOfKinds( - manifestanalyzer.IssueWriteEscapesScope, - manifestanalyzer.IssueWriteFanIn, - manifestanalyzer.IssueRenderRefused, - manifestanalyzer.IssueUnplaceableEdit, - ): - return "WriteBoundaryRefused" - default: - return "UnsupportedContent" - } + return manifestanalyzer.GitPathRefusalReason(refused) } // RegisterGitTargetEventStream registers a GitTargetEventStream with the router. diff --git a/internal/watch/layout_resolution.go b/internal/watch/layout_resolution.go new file mode 100644 index 00000000..0e2200e0 --- /dev/null +++ b/internal/watch/layout_resolution.go @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: Apache-2.0 + +package watch + +import ( + "github.com/ConfigButler/gitops-reverser/internal/git" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// ReportLayoutResolved records what a scan resolved about a GitTarget's folder. It is installed +// on the WorkerManager (git.LayoutReporter) at startup, and it is the only writer of the +// layouts projection. +// +// Every scan calls it, including the ones that resolve exactly what the last one did — the +// branch worker has no memory across scans and should not grow one. The transition test lives +// here, so a steady-state target republishes nothing and enqueues no reconcile, and a folder +// whose shape actually changed does both. +func (m *Manager) ReportLayoutResolved(gitDest types.ResourceReference, report git.LayoutReport) { + changed := m.mutateWatchPlane(func(s *watchPlaneState) bool { + if prior, had := s.layouts[gitDest.Key()]; had && sameLayout(prior, report) { + return false + } + s.layouts[gitDest.Key()] = report + return true + }) + if changed { + m.enqueueGitTargetReconcile(gitDest) + } +} + +// LayoutForGitTarget returns the most recently resolved layout for a GitTarget, and whether one +// has been resolved at all. Absent means no scan has reported yet — which is different from a +// folder that resolved to nothing, and the controller has to distinguish the two: the first +// leaves LayoutResolved Unknown, the second sets it with reason None. +func (m *Manager) LayoutForGitTarget(gitDest types.ResourceReference) (git.LayoutReport, bool) { + report, ok := m.watchPlane().layouts[gitDest.Key()] + return report, ok +} + +// sameLayout compares two reports by what a reader would see in status. +// +// The resolution time is deliberately not compared: it advances on every scan of an unchanged +// folder, so comparing it would republish the whole immutable snapshot and enqueue a reconcile +// once per resync per target, forever, to advance a clock nobody reads a decision from. It is the +// same trap targetPassStatus avoided by dropping its timestamps. +// +// The REVISION is not compared either, for the same reason — every commit to the branch +// moves it, whichever target caused the commit — with one exception, below: a report that has one +// where the last had none is always a change. Without that exception the field would be written +// once, at the first scan of a branch that usually has no commit yet, and then never advance, +// which would leave it permanently empty on exactly the targets it is meant to inform. What it +// therefore means is the revision this resolution was FIRST observed at, not the latest scanned. +func sameLayout(a, b git.LayoutReport) bool { + if a.Revision == "" && b.Revision != "" { + return false + } + if a.Reason != b.Reason || a.Mode != b.Mode || a.RenderRoot != b.RenderRoot { + return false + } + return sameStrings(a.RenderRoots, b.RenderRoots) && sameStrings(a.ReadOnlyBases, b.ReadOnlyBases) +} + +func sameStrings(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} diff --git a/internal/watch/layout_resolution_test.go b/internal/watch/layout_resolution_test.go new file mode 100644 index 00000000..1e2ce411 --- /dev/null +++ b/internal/watch/layout_resolution_test.go @@ -0,0 +1,86 @@ +// SPDX-License-Identifier: Apache-2.0 + +package watch + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + + "github.com/ConfigButler/gitops-reverser/internal/git" + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" +) + +func layoutReportFor(reason manifestanalyzer.LayoutReason, root, revision string) git.LayoutReport { + return git.LayoutReport{ + LayoutResolution: manifestanalyzer.LayoutResolution{Reason: reason, RenderRoot: root}, + Revision: revision, + ResolvedAt: time.Now(), + } +} + +// The steady state must be quiet. Every scan reports — the branch worker has no memory across +// scans and should not grow one — so the transition test here is the only thing standing between +// a healthy folder and one status write per resync, forever. +func TestSameLayout_UnchangedResolutionIsNotRepublished(t *testing.T) { + prior := layoutReportFor(manifestanalyzer.LayoutSingleKustomization, ".", "9f3c1ab") + next := layoutReportFor(manifestanalyzer.LayoutSingleKustomization, ".", "9f3c1ab") + next.ResolvedAt = prior.ResolvedAt.Add(time.Hour) + + assert.True(t, sameLayout(prior, next), "a later scan of an unchanged folder must not republish") +} + +// A later commit to the branch moves the revision without changing the layout, and that must not +// republish either: every target on the branch would write status once per commit, whichever +// target caused it. +func TestSameLayout_ALaterRevisionAloneIsNotAChange(t *testing.T) { + prior := layoutReportFor(manifestanalyzer.LayoutSingleKustomization, ".", "9f3c1ab") + next := layoutReportFor(manifestanalyzer.LayoutSingleKustomization, ".", "44de91c") + + assert.True(t, sameLayout(prior, next)) +} + +// The one exception, and the reason it exists: the first scan of a branch that has no commit yet +// reports no revision, so without this the field would be written empty once and never advance — +// permanently blank on exactly the targets it is meant to inform. +func TestSameLayout_FirstRevisionIsAChange(t *testing.T) { + prior := layoutReportFor(manifestanalyzer.LayoutNone, "", "") + next := layoutReportFor(manifestanalyzer.LayoutNone, "", "9f3c1ab") + + assert.False(t, sameLayout(prior, next), "gaining a revision must reach status") +} + +// The verdict itself is what the condition reads, so a folder that becomes ambiguous republishes. +func TestSameLayout_ChangedVerdictIsAChange(t *testing.T) { + prior := layoutReportFor(manifestanalyzer.LayoutSingleKustomization, ".", "9f3c1ab") + next := layoutReportFor(manifestanalyzer.LayoutAmbiguous, "", "9f3c1ab") + + assert.False(t, sameLayout(prior, next)) +} + +// Mode is what a reader learns the folder's write behaviour from, and it can move without the +// verdict moving: an overlay whose base is removed becomes a self-contained root, still +// SingleKustomization, and the difference is exactly the one a reader needs. +func TestSameLayout_ChangedModeIsAChange(t *testing.T) { + prior := layoutReportFor(manifestanalyzer.LayoutSingleKustomization, ".", "9f3c1ab") + prior.Mode = manifestanalyzer.LayoutModeKustomizeOverlay + prior.ReadOnlyBases = []string{"../../base"} + next := layoutReportFor(manifestanalyzer.LayoutSingleKustomization, ".", "9f3c1ab") + next.Mode = manifestanalyzer.LayoutModeKustomizeRoot + + assert.False(t, sameLayout(prior, next)) +} + +// The bases are published, so a base appearing or moving republishes even when the verdict and +// the mode do not. +func TestSameLayout_ChangedReadOnlyBasesAreAChange(t *testing.T) { + prior := layoutReportFor(manifestanalyzer.LayoutSingleKustomization, ".", "9f3c1ab") + prior.Mode = manifestanalyzer.LayoutModeKustomizeOverlay + prior.ReadOnlyBases = []string{"../../base"} + next := layoutReportFor(manifestanalyzer.LayoutSingleKustomization, ".", "9f3c1ab") + next.Mode = manifestanalyzer.LayoutModeKustomizeOverlay + next.ReadOnlyBases = []string{"../../base", "../../common"} + + assert.False(t, sameLayout(prior, next)) +} diff --git a/internal/watch/materialization.go b/internal/watch/materialization.go index 6070a0f3..5ea1aa8d 100644 --- a/internal/watch/materialization.go +++ b/internal/watch/materialization.go @@ -56,10 +56,13 @@ func (m *Manager) tearDownGitTarget(gitDest types.ResourceReference) { m.forgetGitTargetCluster(gitDest) m.forgetGitTargetPruneMode(gitDest) m.mutateWatchPlane(func(s *watchPlaneState) bool { - if _, had := s.passes[gitDest.Key()]; !had { + _, hadPass := s.passes[gitDest.Key()] + _, hadLayout := s.layouts[gitDest.Key()] + if !hadPass && !hadLayout { return false } delete(s.passes, gitDest.Key()) + delete(s.layouts, gitDest.Key()) return true }) } diff --git a/internal/watch/watch_plane_state.go b/internal/watch/watch_plane_state.go index 9ba69586..d9c90a93 100644 --- a/internal/watch/watch_plane_state.go +++ b/internal/watch/watch_plane_state.go @@ -46,6 +46,10 @@ type watchPlaneState struct { // passes records how each target's most recent plan pass ended, so a target whose passes keep // failing is visible on its own status instead of only in a log line. passes map[string]targetPassStatus + // layouts is each GitTarget's most recently resolved folder layout, published as + // status.placement and the LayoutResolved condition. It is a report from a scan, not from a + // write, so it is present for a target that has never written and for a suspended one. + layouts map[string]git.LayoutReport } // targetPassStatus is how one GitTarget's most recent plan pass ended. @@ -101,6 +105,7 @@ func newWatchPlaneState() *watchPlaneState { auditRoutes: map[string]string{}, pruneModes: map[string]v1alpha3.PruneMode{}, passes: map[string]targetPassStatus{}, + layouts: map[string]git.LayoutReport{}, } } @@ -118,6 +123,7 @@ func (s *watchPlaneState) clone() *watchPlaneState { auditRoutes: copyMap(s.auditRoutes), pruneModes: copyMap(s.pruneModes), passes: copyMap(s.passes), + layouts: copyMap(s.layouts), } for key, cells := range s.streams { out.streams[key] = copyMap(cells) diff --git a/pkg/manifestanalyzer/folder.go b/pkg/manifestanalyzer/folder.go index fece860b..9ee6677c 100644 --- a/pkg/manifestanalyzer/folder.go +++ b/pkg/manifestanalyzer/folder.go @@ -54,6 +54,11 @@ const ( IssueIgnoreShadowsManaged IssueKind = "ignore-shadows-managed" // IssueWriteEscapesScope marks a planned write that would leave the GitTarget's path. IssueWriteEscapesScope IssueKind = "write-escapes-scope" + // IssueAmbiguousLayout marks a GitTarget path covering more than one kustomize render root, + // so a new document has no single root to be placed into. It is a property of the observed + // folder rather than of the spec, so nothing can reject it before the folder is read; the + // fix is to point the GitTarget at one of the roots it covers. + IssueAmbiguousLayout IssueKind = "ambiguous-layout" // IssueWriteFanIn marks an in-place edit of a source file that more than one kustomize // render root reaches. IssueWriteFanIn IssueKind = "write-fan-in" diff --git a/test/e2e/suspend_e2e_test.go b/test/e2e/suspend_e2e_test.go new file mode 100644 index 00000000..d57ca317 --- /dev/null +++ b/test/e2e/suspend_e2e_test.go @@ -0,0 +1,290 @@ +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "encoding/json" + "fmt" + "os" + "path" + "path/filepath" + "strings" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +// This spec is the end-to-end proof for GitTarget.spec.suspend and status.placement +// (docs/layout/model.md, "status.placement, and the post-scan pass"). It is here rather than +// only in the unit suite because the claim it makes is a wiring claim, and the unit tests cannot +// reach the wiring: a suspended target has to still DECLARE, still start its watches, still +// resync, and still have the resulting layout report reach the GitTarget's status through the +// watch plane — while writing nothing. Every one of those hops is real-cluster machinery. +// +// The negative half ("nothing was committed") is paired with a BARRIER, because a negative claim +// is worthless on its own: it passes just as well when the pipeline is asleep. Here the barrier is +// a co-resident ACTIVE GitTarget in the same repository, fed by the same ConfigMap events. Once +// the active target's file has landed, the pipeline has demonstrably run, so the suspended +// target's empty folder means it decided not to write rather than that nothing happened yet. +var _ = Describe("Manager GitTarget suspend", Label("manager", "suspend"), Ordered, func() { + const ( + providerName = "gitprovider-suspend" + + suspendedTarget = "suspend-suspended-target" + activeTarget = "suspend-active-target" + + suspendedPath = "e2e/suspend-suspended" + activePath = "e2e/suspend-active" + + suspendedRule = "suspend-suspended-rule" + activeRule = "suspend-active-rule" + + configMapName = "suspend-probe" + ) + + var ( + testNs string + suspendRepo *RepoArtifacts + ) + + BeforeAll(func() { + By("creating the suspend test namespace") + testNs = testNamespaceFor("manager-suspend") + _, _ = kubectlRun("create", "namespace", testNs) // idempotent; ignore AlreadyExists + + By("setting up the Gitea repo and credentials") + suspendRepo = SetupRepo( + resolveE2EContext(), + testNs, + fmt.Sprintf("e2e-manager-suspend-%d", GinkgoRandomSeed()), + ) + _, err := kubectlRunInNamespace(testNs, "apply", "-f", suspendRepo.SecretsYAML) + Expect(err).NotTo(HaveOccurred(), "failed to apply git secrets to test namespace") + + createReadyGitProvider(providerName, testNs, suspendRepo.GitSecretHTTP, suspendRepo.RepoURLHTTP) + + By("creating one suspended and one active GitTarget in the same repository") + applySuspendGitTarget(suspendedTarget, testNs, providerName, suspendedPath, true) + applySuspendGitTarget(activeTarget, testNs, providerName, activePath, false) + for _, name := range []string{suspendedTarget, activeTarget} { + verifyResourceCondition("gittarget", name, testNs, "Validated", "True", "Succeeded", "") + } + + By("both targets watch ConfigMaps in this namespace") + applyIsolationWatchRule(suspendedRule, testNs, suspendedTarget, `"configmaps"`) + applyIsolationWatchRule(activeRule, testNs, activeTarget, `"configmaps"`) + for _, name := range []string{suspendedRule, activeRule} { + verifyResourceStatus("watchrule", name, testNs, "True", "Succeeded", "") + } + + By("waiting for both targets' ConfigMap streams to be live before any event is created") + for _, name := range []string{suspendedTarget, activeTarget} { + waitForStreamsRunning(name, testNs) + } + }) + + AfterAll(func() { + cleanupNamespace(testNs) + }) + + SetDefaultEventuallyTimeout(90 * time.Second) + SetDefaultEventuallyPollingInterval(2 * time.Second) + + // The half a unit test cannot make: a suspended target keeps its watches and keeps scanning, + // so the layout it resolved reaches its status through the real watch plane. + It("publishes status.placement for a target that has never written", func() { + By("the suspended target resolves its folder and says so") + verifyResourceCondition("gittarget", suspendedTarget, testNs, + "LayoutResolved", "True", "None", + "no kustomization governs this folder", "150s") + + By("the stanza carries the scan's own facts, not a placement's") + // resolvedAtRevision is deliberately NOT asserted here. The repository's branch has no + // commit yet at this point — nothing has written to it — so the scan honestly read the + // folder at no revision, and reporting an empty one is the correct answer rather than a + // missing one. It is asserted below, once the active target has produced a commit. + Eventually(func(g Gomega) { + placement := placementStatusOf(g, suspendedTarget, testNs) + g.Expect(placement).NotTo(BeNil(), "status.placement must be published") + g.Expect(placement).To(HaveKeyWithValue("resolvedAt", Not(BeEmpty()))) + g.Expect(placement).To(HaveKeyWithValue("mode", "Plain"), + "no kustomization governs this folder, so it is written as plain files") + }).Should(Succeed()) + + By("and restates nothing the spec already carries") + // The rule the stanza is held to: a status field earns its place only if a reader cannot + // get it from the spec in the same GET. This asserts the removals stay removed, which + // prose in the API doc cannot. + Eventually(func(g Gomega) { + placement := placementStatusOf(g, suspendedTarget, testNs) + for _, key := range []string{"serializeNamespace", "byTypeEntries", "examples"} { + g.Expect(placement).NotTo(HaveKey(key), + "status.placement must not restate the spec") + } + }).Should(Succeed()) + }) + + // Ready=True with reason Suspended. Not writing is the configured outcome, so no condition + // goes False for it — that is what keeps the conditions that mean a broken mirror meaningful. + It("reports a suspended target as Ready with reason Suspended", func() { + verifyResourceCondition("gittarget", suspendedTarget, testNs, + "Ready", "True", "Suspended", "writes nothing", "150s") + }) + + // The write gate, with its barrier. + It("writes nothing while the active target in the same repo writes", func() { + By("creating a ConfigMap both targets watch") + applySuspendConfigMap(configMapName, testNs, "15m") + + By("BARRIER: the active target commits it, so the pipeline has demonstrably run") + waitForPruneFile(suspendRepo, suspendConfigMapPath(activePath, testNs, configMapName), true) + + By("the suspended target now dates its resolution to a real revision") + // The branch has a commit now, so the scan has a revision to name. Before the barrier it + // did not, which is why this assertion lives here rather than with the rest of the stanza. + // The reconcile request is what makes this prompt rather than a wait on the periodic pass, + // and asserting through it is the point: it is how an operator re-reads a folder someone + // else changed without waiting for the periodic cadence. + requestReconcile(suspendedTarget, testNs) + Eventually(func(g Gomega) { + placement := placementStatusOf(g, suspendedTarget, testNs) + g.Expect(placement).To(HaveKeyWithValue("resolvedAtRevision", Not(BeEmpty())), + "the scan names the revision it read") + }).Should(Succeed()) + + By("and publishes no retention while suspended") + // Nothing sweeps while writes are off, so nothing is measured. A published zero would + // read as "converged" when it means "not counted", so the stanza is absent instead. + Consistently(func(g Gomega) { + g.Expect(statusStanzaOf(g, suspendedTarget, testNs, "retention")).To(BeNil(), + "a suspended target measures no retention, so it reports none") + }, "20s", "4s").Should(Succeed()) + + By("and the suspended target's folder is still empty") + Consistently(func(g Gomega) { + pullLatestRepoState(g, suspendRepo.CheckoutDir) + _, statErr := os.Stat(filepath.Join( + suspendRepo.CheckoutDir, suspendConfigMapPath(suspendedPath, testNs, configMapName))) + g.Expect(os.IsNotExist(statErr)).To(BeTrue(), + "a suspended target must not write, even for an event it observed") + }, 20*time.Second, 4*time.Second).Should(Succeed()) + }) + + // Clearing suspend resumes from the CURRENT cluster state rather than replaying the events + // suppressed while suspended, which is why the file that appears is the ConfigMap's latest + // value and not the one it had when it was created. + It("resumes writing when suspend is cleared", func() { + By("changing the ConfigMap while the target is still suspended") + applySuspendConfigMap(configMapName, testNs, "30m") + + By("clearing spec.suspend") + _, err := kubectlRunInNamespace(testNs, "patch", "gittarget", suspendedTarget, + "--type", "merge", "-p", `{"spec":{"suspend":false}}`) + Expect(err).NotTo(HaveOccurred(), "failed to clear spec.suspend") + + By("requesting a reconcile so the resume does not wait for the periodic pass") + requestReconcile(suspendedTarget, testNs) + + By("the document lands, carrying the value the cluster holds NOW") + relPath := suspendConfigMapPath(suspendedPath, testNs, configMapName) + waitForPruneFile(suspendRepo, relPath, true) + Eventually(func(g Gomega) { + pullLatestRepoState(g, suspendRepo.CheckoutDir) + g.Expect(readRepoFile(g, filepath.Join(suspendRepo.CheckoutDir, relPath))). + To(ContainSubstring("30m"), + "resuming replays the current state, not the events suppressed while suspended") + }).Should(Succeed()) + + By("and Ready is no longer reported as Suspended") + Eventually(func(g Gomega) { + g.Expect(readyReasonOf(g, suspendedTarget, testNs)).NotTo(Equal("Suspended")) + }).Should(Succeed()) + }) +}) + +// applySuspendGitTarget applies a GitTarget with spec.suspend set explicitly. +func applySuspendGitTarget(name, namespace, providerName, targetPath string, suspend bool) { + GinkgoHelper() + manifest := fmt.Sprintf(`apiVersion: configbutler.ai/v1alpha3 +kind: GitTarget +metadata: + name: %s + namespace: %s +spec: + providerRef: + kind: GitProvider + name: %s + branch: main + path: %s + suspend: %t +`, name, namespace, providerName, targetPath, suspend) + out, err := kubectlRunWithStdin(namespace, manifest, "apply", "-f", "-") + Expect(err).NotTo(HaveOccurred(), + "failed to apply GitTarget %q with suspend %t: %s", name, suspend, out) +} + +// applySuspendConfigMap applies the ConfigMap both targets watch, with a value the spec can +// distinguish between writes. +func applySuspendConfigMap(name, namespace, timeout string) { + GinkgoHelper() + manifest := fmt.Sprintf(`apiVersion: v1 +kind: ConfigMap +metadata: + name: %s + namespace: %s +data: + timeout: %q +`, name, namespace, timeout) + out, err := kubectlRunWithStdin(namespace, manifest, "apply", "-f", "-") + Expect(err).NotTo(HaveOccurred(), "failed to apply ConfigMap %s/%s: %s", namespace, name, out) +} + +// requestReconcile stamps the reconcile-request annotation with a fresh value, which is what makes +// the controller re-read the folder now instead of on the periodic cadence. +func requestReconcile(name, namespace string) { + GinkgoHelper() + patch := fmt.Sprintf(`{"metadata":{"annotations":{"reconcile.configbutler.ai/requestedAt":%q}}}`, + time.Now().UTC().Format(time.RFC3339Nano)) + _, err := kubectlRunInNamespace(namespace, "patch", "gittarget", name, "--type", "merge", "-p", patch) + Expect(err).NotTo(HaveOccurred(), "failed to request a reconcile of %q", name) +} + +// suspendConfigMapPath is the canonical mirror path for a ConfigMap under a GitTarget folder. +func suspendConfigMapPath(basePath, ns, name string) string { + return path.Join(basePath, fmt.Sprintf("%s/configmaps/%s.yaml", ns, name)) +} + +// statusStanzaOf reads one named stanza under a GitTarget's status, nil when it has none — +// which is a meaningful answer for both callers: an unpublished stanza and a zeroed one say +// different things. +func statusStanzaOf(g Gomega, name, namespace, stanza string) map[string]interface{} { + GinkgoHelper() + out, err := kubectlRunInNamespace(namespace, "get", "gittarget", name, "-o", "json") + g.Expect(err).NotTo(HaveOccurred(), "failed to read GitTarget %q", name) + + var obj unstructured.Unstructured + g.Expect(json.Unmarshal([]byte(out), &obj.Object)).To(Succeed()) + value, found, err := unstructured.NestedMap(obj.Object, "status", stanza) + g.Expect(err).NotTo(HaveOccurred()) + if !found { + return nil + } + return value +} + +// placementStatusOf reads a GitTarget's status.placement stanza, nil when it has none. +func placementStatusOf(g Gomega, name, namespace string) map[string]interface{} { + GinkgoHelper() + return statusStanzaOf(g, name, namespace, "placement") +} + +// readyReasonOf reads the reason on a GitTarget's Ready condition. +func readyReasonOf(g Gomega, name, namespace string) string { + GinkgoHelper() + out, err := kubectlRunInNamespace(namespace, "get", "gittarget", name, + "-o", `jsonpath={.status.conditions[?(@.type=="Ready")].reason}`) + g.Expect(err).NotTo(HaveOccurred(), "failed to read Ready reason of %q", name) + return strings.TrimSpace(out) +}