diff --git a/api/v1alpha3/gittarget_types.go b/api/v1alpha3/gittarget_types.go index 86dd19c4..676552a3 100644 --- a/api/v1alpha3/gittarget_types.go +++ b/api/v1alpha3/gittarget_types.go @@ -130,6 +130,20 @@ type GitTargetSpec struct { // objects have no namespace. Full resolution table: docs/configuration.md. // +optional AllowedSourceNamespaces *NamespaceMatcher `json:"allowedSourceNamespaces,omitempty"` + + // Design rationale, kept out of the generated CRD description by the blank line below. + // + // Deliberately MUTABLE, unlike the destination fields above. The whole point of the safe + // default is that a target keeps its documents while a scope mistake is diagnosed; turning + // convergence back on afterwards must not require deleting and recreating the GitTarget, which + // would be the one operation guaranteed to lose the folder's history. + + // Prune controls which deletion paths may remove documents from this target's folder: an + // explicit source DELETE event, and the resync mark-and-sweep that infers a deletion from a + // desired snapshot. Omitted, it is `mode: OnEvent` — observed deletes are mirrored, inferred + // ones are not — for a stored GitTarget as well as a new one. + // +optional + Prune *PrunePolicy `json:"prune,omitempty"` } // GitTargetPlacementSpec declares where NEW resources are written when no document @@ -193,6 +207,22 @@ type GitTargetStatus struct { // Counts, never a per-type list, so it stays bounded however many types are watched. // +optional Streams *GitTargetStreamsStatus `json:"streams,omitempty"` + + // Design rationale, kept out of the generated CRD description by the blank line below. + // + // An observation, not a condition. A sweep suppressed by spec.prune.mode is the configured + // outcome and a healthy reconciliation, so no condition may go False for it — doing so would + // train operators to ignore the conditions that mean the mirror is genuinely broken. The + // distinction this field rests on: a condition asserts health, an observation reports a fact. + // status.streams is the precedent for the second kind. + + // Retention reports documents a resync kept because this target's spec.prune.mode suppressed + // the mark-and-sweep. It covers the INFERRED deletion path only: under `never`, a suppressed + // source DELETE is not counted here, so a `never` target can report zero while still declining + // to mirror deletes. It is informational either way — retention is the configured behavior, + // never a fault, and no condition changes state because of it. + // +optional + Retention *GitTargetRetentionStatus `json:"retention,omitempty"` } // GitTargetStreamsStatus is a bounded roll-up of the stream readiness state for the @@ -219,6 +249,37 @@ type GitTargetStreamsStatus struct { ObservedTime *metav1.Time `json:"observedTime,omitempty"` } +// Design rationale, kept out of the generated CRD description by the blank line below. +// +// Counts, never a per-document list, for the same reason GitTargetStreamsStatus is counts: the +// field must stay bounded however many documents are retained. An operator who needs to know WHICH +// documents reads the retention log line or the folder; status answers "how many, under what +// policy, as of when". +// +// The projection is pull-based — the GitTarget controller reads it from the watch manager on each +// reconcile — so it is only as fresh as the last reconcile, exactly like GitPathAccepted. That is +// acceptable for an observation and would not be for a gate, which is a further reason this must +// not become a condition. + +// GitTargetRetentionStatus is a bounded roll-up of what this GitTarget's prune policy kept. +type GitTargetRetentionStatus struct { + // Mode is the EFFECTIVE spec.prune.mode this roll-up was produced under. It is reported here + // rather than left to be read from the spec because a GitTarget that predates spec.prune has + // no stored value at all, so the spec alone cannot explain why documents are being kept. + // +optional + Mode PruneMode `json:"mode,omitempty"` + + // RetainedDocuments is how many managed documents the policy kept that a converged mirror + // would not hold. Zero means a resync ran and found nothing to retain — the mirror is + // converged. An ABSENT retention block means something different: no resync has reported yet. + RetainedDocuments int32 `json:"retainedDocuments"` + + // ObservedTime is when this roll-up was last computed. A retention that begins just after a + // reconcile is not visible until the next one, so read this before treating a zero as live. + // +optional + ObservedTime *metav1.Time `json:"observedTime,omitempty"` +} + // +kubebuilder:object:root=true // +kubebuilder:subresource:status // +kubebuilder:printcolumn:name="Provider",type=string,JSONPath=`.spec.providerRef.name` diff --git a/api/v1alpha3/prune_policy.go b/api/v1alpha3/prune_policy.go new file mode 100644 index 00000000..a8b4588d --- /dev/null +++ b/api/v1alpha3/prune_policy.go @@ -0,0 +1,142 @@ +// SPDX-License-Identifier: Apache-2.0 + +package v1alpha3 + +// PruneMode enumerates which of the two deletion paths may remove a managed document from a +// GitTarget's folder. +// +// | source DELETE event | resync mark-and-sweep +// Never | suppressed | suppressed +// OnEvent | applied | suppressed +// Always | applied | applied +type PruneMode string + +const ( + // PruneNever removes nothing: neither an explicit source DELETE nor an inferred sweep drop. + // The folder becomes an archive that only ever gains and updates documents. + PruneNever PruneMode = "Never" + // PruneOnEvent mirrors an observed source DELETE but never infers a deletion from a desired + // snapshot. It is the effective default. + PruneOnEvent PruneMode = "OnEvent" + // PruneAlways enables both paths: full desired-state convergence, including removing a Git + // document whose resource is absent from the snapshot. + PruneAlways PruneMode = "Always" +) + +// Design rationale, kept out of the generated CRD description by the blank line below. +// +// An object rather than a bare enum field on GitTargetSpec, so a later volume guard (for example +// maxDeletesPerCommit) can be added as a sibling field. Shipping the enum as a scalar would force +// a scalar-to-object change later, which is breaking; shipping the object costs one nesting level +// now and nothing afterwards. + +// PrunePolicy declares which deletion paths may remove documents from a GitTarget's folder. +type PrunePolicy struct { + // Design rationale, kept out of the generated CRD description by the blank line below. + // + // The kubebuilder default writes OnEvent into a NEWLY created object, which is useful but is + // deliberately NOT the compatibility mechanism: a GitTarget stored before this field existed + // carries no value at all, and Kubernetes does not retro-default stored objects. Every reader + // must therefore go through EffectivePruneMode, which maps both an absent policy and an empty + // mode to OnEvent — so an old GitTarget becomes safe without first being edited. + + // Mode selects which deletion paths are enabled. `Never` removes nothing; `OnEvent` mirrors an + // observed source DELETE but never infers a deletion from a resync snapshot; `Always` enables + // both, restoring full desired-state convergence. Omitted, it is `OnEvent`. + // +optional + // +kubebuilder:validation:Enum=Never;OnEvent;Always + // +kubebuilder:default=OnEvent + Mode PruneMode `json:"mode,omitempty"` +} + +// EffectiveMode resolves the declared mode to the one the controller acts on. A nil policy (the +// field was never written) and an empty mode (written without a mode, or stored before the schema +// default existed) both resolve to PruneOnEvent, so an unedited legacy GitTarget is safe. +// +// The nil receiver is deliberate: it makes the omitted case answerable without every call site +// repeating a nil check, which is where a "safe unless someone forgot" default goes wrong. +func (p *PrunePolicy) EffectiveMode() PruneMode { + if p == nil { + return PruneOnEvent + } + return p.Mode.OrDefault() +} + +// OrDefault resolves the EMPTY mode — unset, which is not a mode — to the documented default. +// +// It exists because the empty string is the one value that must not be read literally: both +// predicates below answer false for it, which is `Never`'s behaviour, not `OnEvent`'s. Any value +// that has travelled through a struct literal, a retained pending write, or a stored object +// written before the schema default therefore passes through here first. An UNRECOGNIZED value is +// deliberately left alone — see SweepsOrphans. +func (m PruneMode) OrDefault() PruneMode { + if m == "" { + return PruneOnEvent + } + return m +} + +// AppliesEventDeletes reports whether an explicit source DELETE event may remove its managed +// document. True for OnEvent and Always. Call OrDefault first if the value may be unset. +func (m PruneMode) AppliesEventDeletes() bool { + return m == PruneOnEvent || m == PruneAlways +} + +// SweepsOrphans reports whether a resync may drop a managed document that its desired snapshot did +// not contain — the inferred mark-and-sweep deletion. True only for Always. +// +// An unrecognized value (an object stored under a schema that allowed more than this build does) +// reads as false here and as false in AppliesEventDeletes: an unknown policy retains everything, +// because the failure mode of guessing wrong in the other direction is deleting a tenant's +// manifests. The empty string is NOT such a value — it is unset, and OrDefault resolves it. +func (m PruneMode) SweepsOrphans() bool { + return m == PruneAlways +} + +// EffectivePruneMode is the mode this GitTarget's writes are subject to, with the omitted-field +// default applied. It is the only supported way to read the policy: reading spec.prune.mode +// directly would treat a legacy GitTarget as if it had no mode rather than OnEvent. +func (g *GitTarget) EffectivePruneMode() PruneMode { + return g.Spec.Prune.EffectiveMode() +} + +// The modes ordered by how much deletion they authorize. An unrecognized value ranks with Never, +// matching what both predicates already do with it — an unknown policy retains on both paths. +const ( + pruneRankNever = iota + pruneRankOnEvent + pruneRankAlways +) + +func (m PruneMode) restrictiveness() int { + switch m.OrDefault() { + case PruneAlways: + return pruneRankAlways + case PruneOnEvent: + return pruneRankOnEvent + case PruneNever: + return pruneRankNever + default: + return pruneRankNever + } +} + +// MoreRestrictiveOf returns whichever of the two modes authorizes less deletion. +// +// It exists for one situation: a write that was planned under one policy and is applied — or +// replayed after a rebase — under another. Taking the minimum makes the two directions behave the +// way an operator means them, and they are NOT symmetric: +// +// - LOOSENING must not escalate an already-planned write. A resync planned under `OnEvent` chose +// to keep its orphans against a desired snapshot that is now stale; someone declaring `Always` +// afterwards must not turn that stale plan into deletions. The new policy applies to the next +// resync, which gathers a fresh snapshot. +// - TIGHTENING must apply immediately. `Always` -> `OnEvent` is what an operator reaches for to +// stop deletions that have not landed yet; a policy change that queued work could outrun would +// not be a stop button at all. +func (m PruneMode) MoreRestrictiveOf(other PruneMode) PruneMode { + if other.restrictiveness() < m.restrictiveness() { + return other.OrDefault() + } + return m.OrDefault() +} diff --git a/api/v1alpha3/prune_policy_test.go b/api/v1alpha3/prune_policy_test.go new file mode 100644 index 00000000..ac2c452d --- /dev/null +++ b/api/v1alpha3/prune_policy_test.go @@ -0,0 +1,85 @@ +// SPDX-License-Identifier: Apache-2.0 + +package v1alpha3 + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestEffectivePruneMode_LegacyGitTargetIsOnEvent is the compatibility contract PR 5 rests on. +// +// The CRD default only writes onEvent into a NEWLY created object; Kubernetes does not retro-fill +// stored objects, so a GitTarget written before this field existed reaches the controller with a +// nil spec.prune forever. If that read as anything other than OnEvent, every existing target +// would change behaviour on upgrade without anyone editing it — which is exactly the class of +// surprise this release exists to prevent. +func TestEffectivePruneMode_LegacyGitTargetIsOnEvent(t *testing.T) { + legacy := &GitTarget{} + assert.Equal(t, PruneOnEvent, legacy.EffectivePruneMode(), + "a GitTarget that never declared spec.prune must be OnEvent, not unset") + + declaredEmpty := &GitTarget{Spec: GitTargetSpec{Prune: &PrunePolicy{}}} + assert.Equal(t, PruneOnEvent, declaredEmpty.EffectivePruneMode(), + "a prune object written without a mode must also be OnEvent") +} + +// TestEffectivePruneMode_HonoursDeclaredMode covers the three declared values end to end through +// the GitTarget accessor, so the helper and the field cannot drift. +func TestEffectivePruneMode_HonoursDeclaredMode(t *testing.T) { + for _, mode := range []PruneMode{PruneNever, PruneOnEvent, PruneAlways} { + target := &GitTarget{Spec: GitTargetSpec{Prune: &PrunePolicy{Mode: mode}}} + assert.Equal(t, mode, target.EffectivePruneMode(), "declared mode %q must survive the accessor", mode) + } +} + +// TestPruneMode_PathsMatchTheDocumentedTable pins the two-path table from +// docs/design/watchrule-source-namespace/pr5-gittarget-deletion-safety.md. The two predicates are +// deliberately independent: `onEvent` differs from `always` on ONE of them, and a change that +// collapsed them into a single boolean would silently turn the safe default into full +// convergence. +func TestPruneMode_PathsMatchTheDocumentedTable(t *testing.T) { + for _, tc := range []struct { + mode PruneMode + eventDelete bool + sweep bool + }{ + {PruneNever, false, false}, + {PruneOnEvent, true, false}, + {PruneAlways, true, true}, + } { + assert.Equal(t, tc.eventDelete, tc.mode.AppliesEventDeletes(), + "%q: explicit source DELETE", tc.mode) + assert.Equal(t, tc.sweep, tc.mode.SweepsOrphans(), + "%q: resync mark-and-sweep", tc.mode) + } +} + +// TestPruneMode_UnsetIsNotNever guards the trap that makes the zero value dangerous: read +// literally, "" answers false to BOTH predicates, which is `never` — a mode that silently stops +// mirroring deletes. Every internal carrier of the value normalizes through OrDefault first. +func TestPruneMode_UnsetIsNotNever(t *testing.T) { + var unset PruneMode + + assert.False(t, unset.AppliesEventDeletes(), + "read literally the empty mode looks like never — this is why nothing may read it literally") + assert.Equal(t, PruneOnEvent, unset.OrDefault(), + "OrDefault is the one place that turns unset into the documented default") + assert.True(t, unset.OrDefault().AppliesEventDeletes(), + "a normalized unset mode mirrors an explicit source DELETE") + assert.False(t, unset.OrDefault().SweepsOrphans(), + "a normalized unset mode never infers a deletion") +} + +// TestPruneMode_UnrecognizedValueRetains covers the downgrade case: an object stored by a newer +// build under a mode this build's enum does not have. It must fail closed toward RETENTION on +// both paths — never toward deleting content whose policy it cannot understand. +func TestPruneMode_UnrecognizedValueRetains(t *testing.T) { + future := PruneMode("onEventWithApproval") + + assert.Equal(t, future, future.OrDefault(), + "an unrecognized value is not unset; OrDefault must leave it alone rather than reinterpret it") + assert.False(t, future.AppliesEventDeletes()) + assert.False(t, future.SweepsOrphans()) +} diff --git a/api/v1alpha3/zz_generated.deepcopy.go b/api/v1alpha3/zz_generated.deepcopy.go index ca596b1d..21592aec 100644 --- a/api/v1alpha3/zz_generated.deepcopy.go +++ b/api/v1alpha3/zz_generated.deepcopy.go @@ -734,6 +734,25 @@ 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 *GitTargetRetentionStatus) DeepCopyInto(out *GitTargetRetentionStatus) { + *out = *in + if in.ObservedTime != nil { + in, out := &in.ObservedTime, &out.ObservedTime + *out = (*in).DeepCopy() + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GitTargetRetentionStatus. +func (in *GitTargetRetentionStatus) DeepCopy() *GitTargetRetentionStatus { + if in == nil { + return nil + } + out := new(GitTargetRetentionStatus) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *GitTargetSpec) DeepCopyInto(out *GitTargetSpec) { *out = *in @@ -758,6 +777,11 @@ func (in *GitTargetSpec) DeepCopyInto(out *GitTargetSpec) { *out = new(NamespaceMatcher) (*in).DeepCopyInto(*out) } + if in.Prune != nil { + in, out := &in.Prune, &out.Prune + *out = new(PrunePolicy) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GitTargetSpec. @@ -790,6 +814,11 @@ func (in *GitTargetStatus) DeepCopyInto(out *GitTargetStatus) { *out = new(GitTargetStreamsStatus) (*in).DeepCopyInto(*out) } + if in.Retention != nil { + in, out := &in.Retention, &out.Retention + *out = new(GitTargetRetentionStatus) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GitTargetStatus. @@ -906,6 +935,21 @@ func (in *NamespacedTargetReference) DeepCopy() *NamespacedTargetReference { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *PrunePolicy) DeepCopyInto(out *PrunePolicy) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PrunePolicy. +func (in *PrunePolicy) DeepCopy() *PrunePolicy { + if in == nil { + return nil + } + out := new(PrunePolicy) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PushStrategy) DeepCopyInto(out *PushStrategy) { *out = *in diff --git a/cmd/manifest-analyzer/main.go b/cmd/manifest-analyzer/main.go index 68aeb2fc..8f9a5109 100644 --- a/cmd/manifest-analyzer/main.go +++ b/cmd/manifest-analyzer/main.go @@ -260,6 +260,7 @@ func runScanFolder(dir, format, policy string, stdout, stderr io.Writer) int { scanPolicy := manifestanalyzer.ScanPolicy{ Acceptance: manifestanalyzer.AcceptancePolicy{Allowlist: manifestanalyzer.DefaultAllowlist()}, + Plan: manifestanalyzer.FolderScanPlanPolicy(), } result, err := manifestanalyzer.ScanDir(context.Background(), dir, nil, nil, scanPolicy) if err != nil { diff --git a/config/crd/bases/configbutler.ai_gittargets.yaml b/config/crd/bases/configbutler.ai_gittargets.yaml index 8a1bf299..25f1f875 100644 --- a/config/crd/bases/configbutler.ai_gittargets.yaml +++ b/config/crd/bases/configbutler.ai_gittargets.yaml @@ -338,6 +338,25 @@ spec: required: - name type: object + prune: + description: |- + Prune controls which deletion paths may remove documents from this target's folder: an + explicit source DELETE event, and the resync mark-and-sweep that infers a deletion from a + desired snapshot. Omitted, it is `mode: OnEvent` — observed deletes are mirrored, inferred + ones are not — for a stored GitTarget as well as a new one. + properties: + mode: + default: OnEvent + description: |- + Mode selects which deletion paths are enabled. `Never` removes nothing; `OnEvent` mirrors an + observed source DELETE but never infers a deletion from a resync snapshot; `Always` enables + both, restoring full desired-state convergence. Omitted, it is `OnEvent`. + enum: + - Never + - OnEvent + - Always + type: string + type: object required: - branch - path @@ -435,6 +454,36 @@ spec: by the controller. format: int64 type: integer + retention: + description: |- + Retention reports documents a resync kept because this target's spec.prune.mode suppressed + the mark-and-sweep. It covers the INFERRED deletion path only: under `never`, a suppressed + source DELETE is not counted here, so a `never` target can report zero while still declining + to mirror deletes. It is informational either way — retention is the configured behavior, + never a fault, and no condition changes state because of it. + properties: + mode: + description: |- + Mode is the EFFECTIVE spec.prune.mode this roll-up was produced under. It is reported here + rather than left to be read from the spec because a GitTarget that predates spec.prune has + no stored value at all, so the spec alone cannot explain why documents are being kept. + type: string + observedTime: + description: |- + ObservedTime is when this roll-up was last computed. A retention that begins just after a + reconcile is not visible until the next one, so read this before treating a zero as live. + format: date-time + type: string + retainedDocuments: + description: |- + RetainedDocuments is how many managed documents the policy kept that a converged mirror + would not hold. Zero means a resync ran and found nothing to retain — the mirror is + converged. An ABSENT retention block means something different: no resync has reported yet. + format: int32 + type: integer + required: + - retainedDocuments + type: object streams: description: |- Streams is the bounded data-plane roll-up over this GitTarget's tracked types. diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 2fa299fa..49c3c111 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,62 @@ 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. +## Unreleased — a resync no longer deletes Git documents by default (next minor; behavior change) + +**`GitTarget` gained `spec.prune.mode`, and its effective default changes what a resync does.** +Previously every resync mark-and-swept: a managed document whose resource was absent from the +desired snapshot was deleted. That is now opt-in. + +| Mode | Explicit source DELETE | Resync mark-and-sweep | +|---|---|---| +| `Never` | kept | kept | +| `OnEvent` — the effective default | mirrored | kept | +| `Always` — the previous behavior | mirrored | swept | + +**Deleting a resource in the cluster still deletes its file.** Only the *inferred* deletion changes: +the operator no longer concludes "Git has a document, the snapshot does not list it, therefore delete +it". That inference is only as good as the snapshot's scope — a watch rule narrower than you +intended, or version skew against a controller that does not understand a newer scope field, both +produce a complete-looking snapshot that is smaller than reality. (A snapshot the operator could not +*finish* is already handled: a failed list or watch enqueues no resync, so an outage stops a sweep +rather than shrinking one.) + +### What you have to do + +**Nothing, to be safe.** An existing `GitTarget` has no `spec.prune` and resolves to `OnEvent` +without being edited — Kubernetes does not retro-fill defaults into stored objects, so the operator +applies the default itself. + +**To keep the old behavior**, declare it on each target that needs full convergence: + +```yaml +spec: + prune: + mode: Always +``` + +The field is mutable, so you can switch a target to `Always` after confirming its watch scope +without recreating it. The switch re-lists that target's watched scopes, so the documents a resync +had been keeping are swept on the edit rather than at some later replay. + +### How to tell whether it affects you + +```console +$ kubectl get gittarget acme -o jsonpath='{.status.retention}' +{"mode":"OnEvent","retainedDocuments":3,"observedTime":"2026-07-21T13:20:00Z"} +``` + +A non-zero `retainedDocuments` means the mirror holds documents a converged one would not — the +configured outcome, not a fault, so no condition goes `False` for it. `0` means a resync ran and +found nothing to retain; an absent `retention` block means none has reported yet. The same event +logs a throttled line naming the target and increments +`gitopsreverser_prune_retained_documents_total`, labelled by GitTarget and mode. See +[configuration.md](configuration.md#seeing-what-was-kept). + +This ships in the **same release** as the rule-kind scope change below, and is what makes that +migration non-destructive: a converted `WatchRule` that resolves to a narrower set of namespaces than +you intended leaves the affected documents in Git instead of deleting them. + ## Unreleased — scope is now carried by the rule kind (next minor; breaking) **Scope moved from a per-rule field onto the rule KIND.** `WatchRule` is the namespaced surface and @@ -88,7 +144,7 @@ For each one: A narrowing that slips through is visible rather than silent — `SourceNamespaceAuthorized=False`, `Stalled=True`, streams stopped, and a message naming the failing item — but the documents already in Git are governed by `GitTarget.spec.prune.mode`, which ships in the same release and defaults to -`onEvent`: prior documents are **left in place** rather than swept. (The two changes are never +`OnEvent`: prior documents are **left in place** rather than swept. (The two changes are never released apart.) Verify with `kubectl get watchrules -o wide`, whose `SourceAuthorized` column carries the verdict. diff --git a/docs/configuration.md b/docs/configuration.md index a80c5a1e..1f0d6ec3 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -472,6 +472,8 @@ The important fields are: - `spec.placement`: optional policy for where **new** resources are written (see [Where new resources are written](#where-new-resources-are-written-specplacement)); omit it to follow the repository's existing layout +- `spec.prune`: which deletion paths may remove documents from this target's folder (see + [Deletion policy](#deletion-policy-specprunemode)); omit it for the safe default Example: @@ -519,9 +521,81 @@ The most useful status fields are: - `StreamsRunning`: true when the source watches are past initial replay and routing live events. - `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. Use conditions for automation. +### Deletion policy (`spec.prune.mode`) + +A target removes a document from Git for one of two very different reasons, and `spec.prune.mode` +controls them separately: + +- an **explicit source DELETE event** — the source cluster told the operator the resource is gone; +- a **resync mark-and-sweep** — a snapshot taken when a watch stream starts or restarts did not + contain a resource that Git still has a document for, so its absence is *inferred*. + +The second is only as trustworthy as the snapshot's **scope**. A snapshot the operator could not +finish is not the risk: a failed list or watch blocks the stream and enqueues no resync at all, and a +replay cut short before its initial-events bookmark enqueues nothing — so a source-cluster outage or +a revoked RBAC grant currently stops a sweep rather than shrinking one. + +The risk is a snapshot that is *complete* but gathered against the wrong scope: a watch rule narrower +than you intended, version skew, or an older controller that does not understand a newer scope field. +That snapshot is smaller than reality and indistinguishable from a converged one, and a sweep turns +it into deleted manifests. `OnEvent` is the defence, and it also covers the outage case in depth — +failing closed there is a property of how the gather works today, not a guarantee the API makes. + +| Mode | Explicit source DELETE | Resync mark-and-sweep | Use it for | +|---|---|---|---| +| `Never` | kept | kept | an archive or tombstone mirror that only ever gains documents | +| `OnEvent` (default) | mirrored | kept | mirroring observed deletes without ever inferring one | +| `Always` | mirrored | swept | full desired-state convergence, including cleaning up stale documents | + +```yaml +spec: + prune: + mode: Always +``` + +Omitting `spec.prune` means `OnEvent`. That applies to a `GitTarget` created before this field +existed as well: an upgrade never changes an existing target to a more destructive policy, and you +do not have to edit anything to be safe. + +Choose `Always` when the folder is meant to be a faithful, converged mirror and you accept that a +bad watch scope can delete manifests. Choose `Never` when the folder is an audit trail. + +#### Seeing what was kept + +A retained document is invisible in Git by design — nothing is written, so a retaining mirror and a +converged one look identical in the folder and in `git log`. Three signals report it instead, and +none of them is a failure: retention is the configured outcome, so no condition goes `False` for it. + +```console +$ kubectl get gittarget acme -o jsonpath='{.status.retention}' +{"mode":"OnEvent","retainedDocuments":3,"observedTime":"2026-07-21T13:20:00Z"} +``` + +- `status.retention.retainedDocuments` is how many managed documents a converged mirror would not + hold. `0` means a resync ran and found nothing to retain; an **absent** `retention` block means no + resync has reported yet, which is not the same thing. `mode` is the *effective* mode the count was + produced under — the only place a `GitTarget` that predates `spec.prune` shows one at all. +- A throttled log line names the target, its path, and the scope (one per target folder per 10 + minutes; the full detail is at `-v1`). +- `gitopsreverser_prune_retained_documents_total`, labelled by `gittarget_namespace`, + `gittarget_name`, and `prune_mode`. + +`status.retention` covers the resync sweep only. Under `Never` a suppressed source DELETE is not +counted, so a `Never` target can report `0` while still declining to mirror deletes. + +The count is refreshed when a resync runs, so it lags a change in the cluster until the next one — +read `observedTime` before treating a `0` as live. + +`spec.prune` is mutable — unlike `providerRef`, `branch`, and `path` — so a target can be moved to +`Always` once its watch scope is confirmed, without recreating it. Widening it to `Always` re-lists +the target's watched scopes, so the cleanup runs on the edit instead of waiting for the next replay. +Tightening it applies to the next write and leaves the streams alone, which is what makes it usable +as a stop button. + ### Where new resources are written (`spec.placement`) Placement decides the file path for a resource that has **no document in Git yet**. Once a document diff --git a/docs/design/watchrule-source-namespace/README.md b/docs/design/watchrule-source-namespace/README.md index 184cd7a5..f2cc53a2 100644 --- a/docs/design/watchrule-source-namespace/README.md +++ b/docs/design/watchrule-source-namespace/README.md @@ -4,7 +4,8 @@ > scope-by-kind design. **PR 5** is the deletion-safety change, implemented in the PR immediately > after it. **No release may be cut between the two merges** — the first release containing PR 4 also > contains PR 5. Review findings still open against PR 4 are tracked in -> [PR 4 review follow-ups](pr4-review-followups.md). Index: [INDEX.md](../../INDEX.md). +> [PR 4 review follow-ups](pr4-review-followups.md), and against PR 5 in +> [PR 5 review follow-ups](pr5-review-followups.md). Index: [INDEX.md](../../INDEX.md). ## Decision @@ -37,7 +38,7 @@ WatchRule ──uses──> GitTarget ──uses──> ClusterProvider WatchRule source namespaces. A declared policy has no self-namespace exception. - `ClusterProvider.spec.allowSourceNamespaceOverride` is the platform-admin delegation that permits an admitted GitTarget to authorize a WatchRule outside its own namespace. -- `GitTarget.spec.prune.mode` controls deletion: `never`, `onEvent` (default), or `always`. +- `GitTarget.spec.prune.mode` controls deletion: `Never`, `OnEvent` (default), or `Always`. Cluster-scoped objects have no namespace. A ClusterWatchRule therefore receives every selected cluster-scoped object its source credential can read; tenant isolation for such objects requires @@ -72,7 +73,9 @@ namespace and target policy, or recognize a ClusterWatchRule as intentionally cl | 2 | [Stream-scope collapse](pr2-stream-scope-collapse.md) | A cluster-wide stream cannot silently widen a co-resident named stream. | landed | | 3 | [ClusterWatchRule target admission](pr3-clusterwatchrule-target-admission.md) | A ClusterWatchRule cannot attach to a GitTarget its ClusterProvider does not admit. | landed | | 4 | [Scope by kind](pr4-cluster-scope-only.md) | Rework the unshipped source-namespace work for `rules[].sourceNamespace`, narrow ClusterWatchRule to cluster scope, refuse stored namespaced rules, and document the cross-kind migration. | current branch; breaking | -| 5 | [GitTarget deletion safety](pr5-gittarget-deletion-safety.md) | Add `prune.mode` and make resync sweep opt-in (`always`). | next PR; ships in the same release as PR 4 | +| 5 | [GitTarget deletion safety](pr5-gittarget-deletion-safety.md) | Add `prune.mode` and make resync sweep opt-in (`Always`). | implemented; ships in the same release as PR 4 | +| 5b | [Retention visibility](pr5-retention-visibility.md) | Report what PR 5 decided to keep, so a retention is observable rather than silent. | implemented; same PR as 5 | +| 5c | [PR 5 review follow-ups](pr5-review-followups.md) | Make a `prune.mode` change take effect: converge on a loosening, apply a tightening immediately. Plus two doc corrections. | implemented; same PR as 5 | The discarded top-level `sourceNamespace` plan is retained as a [historical implementation baseline](historical-top-level-source-namespace-baseline.md). Its gate, @@ -101,9 +104,9 @@ the affected WatchRules first. ## Deletion safety The product has two deletion paths: explicit source DELETE events and inferred mark-and-sweep drops -during resync. Scope mistakes threaten only the latter. PR 5 defaults targets to `prune.mode: onEvent`: +during resync. Scope mistakes threaten only the latter. PR 5 defaults targets to `prune.mode: OnEvent`: explicit source DELETE events remain mirrored, while a narrowed or incorrect desired set leaves prior -Git documents untouched. `always` restores full desired-state convergence; `never` creates an archive +Git documents untouched. `Always` restores full desired-state convergence; `Never` creates an archive that does not remove documents. PR 5 is intentionally narrow. It does not add a deletion-count or percentage guard. If experience diff --git a/docs/design/watchrule-source-namespace/pr4-cluster-scope-only.md b/docs/design/watchrule-source-namespace/pr4-cluster-scope-only.md index 36a9beb1..4cd1bd5f 100644 --- a/docs/design/watchrule-source-namespace/pr4-cluster-scope-only.md +++ b/docs/design/watchrule-source-namespace/pr4-cluster-scope-only.md @@ -551,7 +551,7 @@ Instead: 3. the warning that a target with no policy admits **only** a WatchRule whose every item watches its own namespace — every wildcard or cross-namespace item a conversion produces is denied — so converting without also declaring `allowedSourceNamespaces` narrows production data; and that - under PR 5's `onEvent` default the narrowing leaves stale documents in Git rather than deleting + under PR 5's `OnEvent` default the narrowing leaves stale documents in Git rather than deleting them; 4. a `kubectl get clusterwatchrules -o json | jq …` one-liner that lists every affected object and its target. @@ -680,7 +680,7 @@ Fixture conversions worth calling out, because they change what the test proves: proof, kept. - A target whose policy admits `repo-config` never receives an object from `tenant-zen`, asserted against a real commit. -- Narrowing a policy under PR 5's `onEvent` default leaves the prior documents in Git rather than +- Narrowing a policy under PR 5's `OnEvent` default leaves the prior documents in Git rather than sweeping them. ## Done when diff --git a/docs/design/watchrule-source-namespace/pr5-gittarget-deletion-safety.md b/docs/design/watchrule-source-namespace/pr5-gittarget-deletion-safety.md index c17fefdf..33e9081d 100644 --- a/docs/design/watchrule-source-namespace/pr5-gittarget-deletion-safety.md +++ b/docs/design/watchrule-source-namespace/pr5-gittarget-deletion-safety.md @@ -5,8 +5,10 @@ > [PR 4](pr4-cluster-scope-only.md), and **no release may be cut between the two merges** — the first > release containing PR 4's breaking API changes also contains this one. > -> **Status: proposal, not started.** Depends on [PR 1](pr1-namespace-scoped-resync.md) and +> **Status: implemented.** Depends on [PR 1](pr1-namespace-scoped-resync.md) and > [PR 2](pr2-stream-scope-collapse.md), which identify the resync sweep boundary this PR controls. +> Implementation notes that were decided during the build — and are not restatements of this +> design — are recorded in [Implementation notes](#implementation-notes) at the end. ## Purpose @@ -16,9 +18,13 @@ GitOps Reverser has two distinct deletion paths: - A resync compares a desired snapshot with Git and drops documents that are absent from that snapshot (mark-and-sweep). -The first is source-cluster evidence. The second is an inference, and is unsafe when the snapshot -is narrowed by a bad scope, a temporary outage, or a controller that does not understand a newer -scope field. This PR lets each GitTarget decide which paths may remove Git documents. +The first is source-cluster evidence. The second is an inference, and is unsafe when the snapshot is +narrowed by a bad scope or by a controller that does not understand a newer scope field — a snapshot +that is complete but gathered against the wrong boundary. An *incomplete* gather is not the exposure: +a failed list or watch enqueues no resync, and a replay cut short before its initial-events bookmark +enqueues nothing, so an outage stops a sweep rather than shrinking one. That is a property of today's +gather rather than a guarantee, which is why `OnEvent` covers it in depth. This PR lets each GitTarget +decide which paths may remove Git documents. ## API @@ -36,31 +42,31 @@ spec: branch: main path: tenants/acme prune: - mode: onEvent + mode: OnEvent ~~~ -`GitTarget.spec.prune.mode` is an enum with an effective default of `onEvent`: +`GitTarget.spec.prune.mode` is an enum with an effective default of `OnEvent`: | Mode | Explicit source DELETE event | Resync mark-and-sweep | Intended use | |---|---:|---:|---| -| `never` | suppressed | suppressed | archive/tombstone mirror | -| `onEvent` | applied | suppressed | safe default; mirror observed deletes but never infer them | -| `always` | applied | applied | full convergence, including cleanup of stale Git documents | +| `Never` | suppressed | suppressed | archive/tombstone mirror | +| `OnEvent` | applied | suppressed | safe default; mirror observed deletes but never infer them | +| `Always` | applied | applied | full convergence, including cleanup of stale Git documents | -`onEvent` means a DELETE event, not every watch event. `always` enables both deletion paths. +`OnEvent` means a DELETE event, not every watch event. `Always` enables both deletion paths. The CRD default is useful for newly written objects, but it is not the compatibility mechanism. -The controller must use `EffectivePruneMode()` and treat an omitted stored value as `onEvent`; old +The controller must use `EffectivePruneMode()` and treat an omitted stored value as `OnEvent`; old GitTargets must become safe without first being edited. -## Why `onEvent` is the default +## Why `OnEvent` is the default A scope collapse must stop updates outside the resulting scope, but it must not erase their existing -Git documents. With `onEvent`, a resync emits no managed-drop actions, so the documents remain until -an explicit source DELETE is observed or the target owner deliberately selects `always`. +Git documents. With `OnEvent`, a resync emits no managed-drop actions, so the documents remain until +an explicit source DELETE is observed or the target owner deliberately selects `Always`. This is intentionally a behavior change from today's implicit sweep behavior. A target that needs -full desired-state convergence opts in with `prune.mode: always`. +full desired-state convergence opts in with `prune.mode: Always`. This PR does **not** attempt to limit a large cascade of genuine DELETE events. A future PR may add for example `maxDeletesPerCommit` to this object if experience shows that control is needed. An @@ -69,31 +75,31 @@ than presented as one in this first safety release. ## Implementation -1. **API and effective default.** Add `PrunePolicy` and `PruneMode` (`never`, `onEvent`, `always`) to +1. **API and effective default.** Add `PrunePolicy` and `PruneMode` (`Never`, `OnEvent`, `Always`) to `GitTargetSpec`, with schema enum/default and an `EffectivePruneMode()` helper. Regenerate deepcopy code and CRDs. 2. **Suppress sweep actions at the planner.** Thread the effective mode into the resync planning - policy. `BuildScopedPlan` must not emit `PlanDropOrphan` when the mode is `never` or `onEvent`. + policy. `BuildScopedPlan` must not emit `PlanDropOrphan` when the mode is `Never` or `OnEvent`. Do not filter the action at apply time: a suppressed drop must not enter the plan, plan hash, or commit path. 3. **Gate explicit deletes.** The steady-state delete writer applies delete-document actions only - for `onEvent` and `always`; `never` leaves the managed document unchanged. + for `OnEvent` and `Always`; `Never` leaves the managed document unchanged. 4. **Surface configured retention.** A sweep suppressed by policy is healthy, not a failed reconciliation. Emit a rate-limited informational log and make the mode visible in API documentation; do not add a failure condition merely because a stale document remains. The zero value of the internal planner policy must be explicit in every caller. Production resync code passes the target's effective mode; dry-run and unit-test callers choose deliberately whether -they want to model `always` or `onEvent`. +they want to model `Always` or `OnEvent`. ## Tests -- A legacy GitTarget that omits `prune` has effective mode `onEvent`. -- `onEvent` retains a document when a resync desired set narrows to empty; the generated plan has no +- A legacy GitTarget that omits `prune` has effective mode `OnEvent`. +- `OnEvent` retains a document when a resync desired set narrows to empty; the generated plan has no managed-drop action. -- `onEvent` still mirrors one explicit DELETE event. -- `never` suppresses both paths. -- `always` reproduces today's mark-and-sweep behavior byte-for-byte. +- `OnEvent` still mirrors one explicit DELETE event. +- `Never` suppresses both paths. +- `Always` reproduces today's mark-and-sweep behavior byte-for-byte. - Full and namespace-scoped resyncs both honor the mode; no alternate sweep path bypasses it. ## Release and rollback @@ -106,11 +112,74 @@ Do not claim safety for a rollback past that release once PR-4 manifests have be controller neither understands `prune` nor the rule-item source-namespace field, so it resolves a narrower desired set *and* sweeps it. Remove or narrow the affected WatchRules first. -Inside the shipping release, `onEvent` is still what makes a scope mistake non-destructive — it is +Inside the shipping release, `OnEvent` is still what makes a scope mistake non-destructive — it is just no longer a separately released prerequisite. ## Done when -- `prune.mode` defaults effectively to `onEvent` for both new and existing GitTargets. +- `prune.mode` defaults effectively to `OnEvent` for both new and existing GitTargets. - Explicit deletes and inferred sweep drops are independently controlled exactly as in the table. - `task lint`, `task test`, and `task test-e2e` pass, closing the do-not-release window PR 4 opened. + +## Reporting what was kept + +This page covers the decision — which deletions happen. Making a retention *observable* is covered +separately in [retention visibility](pr5-retention-visibility.md), which landed in the same pull +request: a suppressed drop produces no action, no commit, and no stat, so on its own it is +discoverable only from a throttled log line. That page adds the `status.retention` roll-up, and is +bound by the rule above — an observation, never a condition. + +Two defects in the *transitions* between modes were found by review after this page was written and +fixed in the same PR: widening to `Always` did not converge a quiet target, and tightening away from +it could be outrun by a write already committed locally. Both are recorded in +[PR 5 review follow-ups](pr5-review-followups.md); neither changes the decision above. + +## Implementation notes + +Decisions taken while building this that the design above did not settle. They are recorded because +each one has a failure mode that is invisible if the reasoning is lost. + +### The empty string is unset, not `Never` + +Read literally, `PruneMode("")` answers false to both `AppliesEventDeletes` and `SweepsOrphans` — +which is `Never`, the mode that stops mirroring deletes. But an omitted field means `OnEvent`. Every +internal carrier of the value (the writer's `ResolvedTargetMetadata`, the per-base lookup, the +write batch) therefore normalizes through `PruneMode.OrDefault` before asking either predicate. An +*unrecognized* value is treated differently and deliberately: it is left alone and retains on both +paths, because a policy this build cannot understand must not authorize deletion. + +### The planner models only the inferred path + +`manifestanalyzer.Policy` gains a `SweepMode`, not a `PruneMode`: the planner never sees an explicit +DELETE event, so `Never` and `OnEvent` are the same instruction to it. Keeping the API enum out of +`manifestanalyzer` also preserves that package's freedom from Kubernetes API types, the same rule +`PlacementPolicy` already follows. The translation happens once, in `resyncPlanPolicy`. + +`SweepMode`'s zero value **retains**, and every production caller sets it explicitly anyway. The +offline folder scan (`FolderScanPlanPolicy`) deliberately converges: it has no GitTarget to read a +policy from, it writes nothing, and a report that silently omitted a folder's orphans would render a +stale folder identically to a converged one. + +### A suppressed drop is counted, not just skipped + +`Plan.RetainedOrphans` exists because a suppressed drop leaves no other trace — no action, no +commit, no `ResyncStats` entry. Without the count, nothing downstream could distinguish "this mirror +is converged" from "this mirror is deliberately keeping stale documents". It feeds a throttled +default-verbosity log (one per target folder per 10 minutes, since retention is a steady state and +resyncs fire per type and namespace) and `gitopsreverser_prune_retained_documents_total`. + +### Retention is not an empty scope + +A retaining policy could have been implemented by passing a scope predicate that matches nothing, +and every drop-suppression test would still pass. It is not, because the two answer different +questions: `inScope` is "is this document any of my business", `Sweep` is "may I delete the ones +that are". Collapsing them would erase the distinction between a document this plan never owned and +one it owned, considered, and kept — which is exactly the fact the operator needs. + +### Drift heal is gated too, by construction + +The heal resync (`ResyncRequest.Heal`) runs through the same planner, so under `OnEvent` the +operator no longer removes a stray managed document a human added to the folder by hand. This +follows from the design rather than extending it, but it is the consequence most likely to surprise: +"the reverser stopped cleaning up junk I put in the folder" is the same mechanism as "the reverser +did not delete my tenant's manifests when the scope collapsed". `Always` restores both. diff --git a/docs/design/watchrule-source-namespace/pr5-retention-visibility.md b/docs/design/watchrule-source-namespace/pr5-retention-visibility.md new file mode 100644 index 00000000..e6a90b72 --- /dev/null +++ b/docs/design/watchrule-source-namespace/pr5-retention-visibility.md @@ -0,0 +1,314 @@ +# PR 5, part 2 — make retention visible on the GitTarget + +> The reporting half of [PR 5](pr5-gittarget-deletion-safety.md), landing in the **same pull request** +> ([#260](https://github.com/ConfigButler/gitops-reverser/pull/260)). Part 1 decides what to keep; +> this decides how an operator finds out. No change to the write path. +> +> **Status: implemented.** The [open questions](#open-questions-for-review) are answered at the end +> of that section, and [what this costs the PR](#what-this-costs-the-open-pr) was accepted rather +> than deferred: the field ships in the same release as `spec.prune.mode`. +> +> A review of #260 found the *log* half of the same defect — it did not name the GitTarget either, +> though two user-facing docs said it did. It is folded into +> [Step 0](#step-0-make-both-retention-signals-name-the-gittarget) rather than tracked separately; +> the rest of that review is in [PR 5 review follow-ups](pr5-review-followups.md). + +## The problem + +Part 1 made a suppressed sweep the **default**, and a suppressed sweep is by construction invisible. +It produces no plan action, no commit, and no `ResyncStats` entry — that is deliberate, because a +retention must be indistinguishable from the event never arriving. The consequence is that an +operator comparing their cluster to the mirror has nothing to read that distinguishes: + +- **converged** — the folder matches the cluster; and +- **deliberately retaining** — the folder holds documents a converged mirror would not, because the + target's policy kept them. + +Those two states look identical from `kubectl get gittarget`, from `git log`, and from the folder +itself. That is a bad property for a safety feature whose entire premise is *"we kept something on +purpose."* A safety mechanism the operator cannot observe is one they cannot trust — and, more +practically, one they cannot audit before flipping a target to `Always`. + +### What part 1 shipped, and why it is not enough + +The state this plan started from. Every "what it cannot" below is now closed — the log line and the +metric name the target, and the count reaches status — but the table is kept as written because it +is the argument for the shape, not a description of the code. + +| Signal | Where | What it answers | What it cannot | +|---|---|---|---| +| Throttled log line | [`resync_flush.go`](../../../internal/git/resync_flush.go), `reportRetainedOrphans` | which folder and scope, how many | **which GitTarget** — it logs `path`, and two targets in different namespaces can share one; needs log access; throttled to one per folder per 10 min; nothing to query | +| `gitopsreverser_prune_retained_documents_total` | [`telemetry/exporter.go`](../../../internal/telemetry/exporter.go) | is *anything* retaining, under which mode | **which GitTarget** — it is labelled by `prune_mode` only | +| `Plan.RetainedOrphans` | [`manifestanalyzer/plan.go`](../../../internal/manifestanalyzer/plan.go) | the count, per resync | never leaves the writer; not carried on `ResyncStats` | + +So today the only way to answer *"is target X retaining anything?"* is to grep controller logs and +match the `path` field — which is not even an identifier, since two GitTargets in different +namespaces can write the same `spec.path` on different branches of one repository. For a change that +altered the default deletion behaviour of every existing GitTarget, that is too weak. + +Neither signal naming the target is the cheapest defect here, and it is worth fixing whatever else is +decided — see [Step 0](#step-0-make-both-retention-signals-name-the-gittarget). It is also a +correctness issue in the docs, not only a gap: both +[configuration.md](../../configuration.md) and [UPGRADING.md](../../UPGRADING.md) already promise a +log line "naming the target". + +## What this is deliberately NOT + +**Not a condition.** [Part 1](pr5-gittarget-deletion-safety.md#implementation) is explicit: a sweep +suppressed by policy is healthy, not a failed reconciliation, and no failure condition may be raised +merely because a stale document remains. That still holds. Raising any `False` condition for the +configured behaviour would train operators to ignore the conditions that mean the mirror is genuinely +broken — strictly worse than the invisibility being fixed. + +The distinction this plan rests on: **a condition asserts health; an observation reports a fact.** +`status.streams` is already the precedent for the second kind — a bounded roll-up that no condition +reads. Retention belongs in that category. + +**Not a per-document list.** The count stays bounded however many documents are retained, for the +same reason `status.streams` is counts and never a per-type list. An operator who needs to know +*which* documents reads the log line or scans the folder; status answers "how many, and under what +policy". + +## Proposed API + +```yaml +status: + retention: + mode: OnEvent # the EFFECTIVE mode, resolved — answers "why" without a second lookup + retainedDocuments: 3 # documents a converged mirror would not hold + observedTime: "2026-07-21T13:20:00Z" +``` + +`mode` is duplicated from `spec.prune.mode` rather than left for the reader to correlate, because +the interesting value is the **effective** one: a legacy GitTarget has no `spec.prune` at all, so +`spec` alone cannot explain why documents are being kept. This is the one place the omitted-field +default becomes visible without reading the source. + +`retainedDocuments: 0` and an **absent** `retention` block mean different things, and both are +needed: + +- absent — no resync has reported yet (the target has not replayed, or predates the field); +- `0` — a resync ran and found nothing to retain. This is the "converged" signal, and it is why zero + must be recorded as actively as a non-zero count. + +## How the number gets there + +The projection is **pull-based**, which the codebase already establishes: the GitTarget controller +reads data-plane state from the watch `Manager` on each reconcile +([`gittarget_controller.go`](../../../internal/controller/gittarget_controller.go)): + +~~~go +streams = r.EventRouter.WatchManager.StreamSummaryForGitTarget(gitDest) +gitPath = r.EventRouter.WatchManager.GitPathAcceptanceForGitTarget(gitDest) +renderFidelity = r.EventRouter.WatchManager.RenderFidelityForGitTarget(gitDest) +~~~ + +Retention becomes a fourth reader beside them. The full path: + +1. **Carry the count out of the writer.** `ResyncStats` gains `Retained int`, set from + `Plan.RetainedOrphans` in `applyResyncPlan`. It rides the existing `ResyncResult` reply channel — + no new transport. +2. **Record it per scope.** [`drainScopedResync`](../../../internal/watch/event_router.go) already + receives the result, the `targetWatchKey` (GVR + namespace) and the render-fidelity epoch, and + already calls `MarkTargetGitPathAccepted` and `MarkTargetRenderFidelityScopeClean` there. A + `MarkTargetRetention(gitDest, key, epoch, stats.Retained)` sits beside them. +3. **Roll up per target.** The `Manager` keeps per-(target, scope) counts and sums them; + `RetentionForGitTarget(gitDest)` returns the roll-up. +4. **Project onto status.** The controller writes `status.retention` next to `status.streams`. + +### The eviction problem, and why it is already solved + +A per-scope map raises the obvious question: when a type stops being watched, or a namespace leaves +the target's admitted set, its retained count must **disappear** — otherwise the roll-up only ever +grows and becomes a lie. Pruning that map correctly against a changing watch plan is exactly the kind +of bookkeeping that rots. + +It does not need writing. [`RenderFidelityGate`](../../../internal/watch/render_fidelity_gate.go) +solves the identical problem for an identical key, and it does so with an **epoch** rather than with +eviction: records carry the watch epoch they were produced under, the epoch bumps when the watch plan +is reinstalled, and records from an older epoch are ignored — "a stale cancellation tail is ignored by +the gate and cannot reopen a failed target." Retention should reuse `RenderFidelityEpochForGitTarget` +verbatim. A scope that vanishes from the plan takes its count with it at the next epoch, with no +per-key deletion logic to get wrong. + +This is the most important reuse decision in the plan: writing a second, independent scope-lifecycle +tracker next to the one that already exists is how the two drift apart. + +### The staleness property, stated rather than hidden + +Because the projection is pull-based, `status.retention` is only as fresh as the last GitTarget +reconcile. A data-plane fact does not by itself enqueue the GitTarget — the same property that makes +`GitPathAccepted` lag, and the documented cause of a past CI flake in that seam. A retention that +begins just after a reconcile can take until the next periodic requeue (up to ~10 min) to appear. + +That is acceptable for an observation and unacceptable for a gate, which is a further reason it must +not become a condition. It should be **documented in the field's own godoc**, so a reader does not +mistake a stale zero for a live one — and it is why `observedTime` is in the shape rather than being +inferred from `lastReconcileTime`. + +Optionally, a `0 → n` transition could enqueue the target so the first appearance is prompt while +later updates ride the requeue. That is a refinement, not a requirement, and should be decided +against the flake history rather than added reflexively. + +## Step 0: make both retention signals name the GitTarget + +Small, independent of the status work, and worth doing first. Both halves live in +`reportRetainedOrphans`, which already receives everything except the target reference — +`executeResyncPendingWrite` holds the resolved target and passes its `PruneMode` down the same call, +so this is one extra parameter, not new plumbing. + +**The metric:** + +~~~go +telemetry.PruneRetainedDocumentsTotal.Add(ctx, int64(retained), metric.WithAttributes( + attribute.String("prune_mode", string(mode)), + attribute.String("gittarget_namespace", ns), // new + attribute.String("gittarget_name", name), // new +)) +~~~ + +Cardinality is bounded by the number of GitTargets, not by resources, and the label names follow the +convention `TargetReconcileCompletedTotal` already sets — `gittarget_namespace` / `gittarget_name` +rather than the reserved `namespace` / `name`, because a pod scrape with `honor_labels=false` +overwrites a metric's `namespace` attribute with the scraping pod's own, silently breaking any +per-target selector. + +**The log line** gains the same identity as a `gitTarget` field. It logs `retained`, `pruneMode`, +`path`, and `scope` today, and `path` is the one thing an operator cannot map back to an object. The +[#260 review](pr5-review-followups.md#r3--the-retention-log-does-not-name-the-gittarget) asked for +the log field while asking to leave the metric unlabelled for cardinality; that half is declined +here, for the reason above — per-target labels are already this codebase's convention and are what +makes the counter actionable. What stays off the metric is per-path, per-scope, or per-document +labels, which are unbounded and are exactly what the log line is for. + +Together these make "which target is retaining" answerable from metrics *and* from logs, even before +status lands — and they make the existing sentence in +[configuration.md](../../configuration.md) and [UPGRADING.md](../../UPGRADING.md) true. + +## What this costs the open PR + +Worth weighing explicitly, since this is going into #260 rather than following it: + +- **The suite must be re-run.** #260 is currently green end to end (65 e2e specs, 0 failures). This + adds an API field, a controller status write, a new `Manager` surface, and e2e coverage — so + `task lint`, `task test`, and `task test-e2e` all run again from scratch. +- **The do-not-release window stays open longer.** `main` currently holds PR 4's breaking scope + rework *without* the deletion safety that makes it non-destructive, and + [release-please #256](https://github.com/ConfigButler/gitops-reverser/pull/256) is open and would + ship exactly that state. Every day #260 stays open is a day that window is open. +- **It grows a reviewed PR after review.** #260 has already been read once. Additive status work is + low-risk, but it is new surface arriving after the fact. + +Against that: the field is API surface, and adding `status.retention` in the same release as +`spec.prune.mode` avoids a second status-shape change one release later. If it does not land here it +should land before the release, not after — a released `prune.mode` whose retention cannot be +observed is the version operators will form their first impression on. + +**Decided: it landed here.** The suite was re-run in full and is green (75 e2e specs passed, 22 +skipped, 0 failures; unit coverage 77.8%, unchanged against the baseline). + +## Tests + +- A resync that retains N documents surfaces `retainedDocuments: N` and the effective `mode`, for a + target declaring no `spec.prune` — so the roll-up reports the *effective* mode, not the absent + stored one. +- A later resync that retains nothing drives it back to `0`. This is the likeliest regression, since + "record zero as actively as non-zero" is easy to lose. +- A scope that leaves the watch plan drops its contribution at the next epoch, without recreating the + target. +- Counts from a stale epoch are ignored — the property inherited from `RenderFidelityGate`. +- Unscoped and namespace-scoped resyncs both contribute, mirroring part 1's + `TestPrune_RetentionIsIdenticalUnderEveryResyncShape`. +- e2e: the `Always` target reports `0` while the co-resident default target reports non-zero for the + same seeded orphan — reusing part 1's barrier structure, since the `Always` sweep is what proves a + resync ran at all. + +All of the above landed in [`retention_rollup_test.go`](../../../internal/watch/retention_rollup_test.go), +[`gittarget_status_test.go`](../../../internal/controller/gittarget_status_test.go) (the +absent-versus-zero projection), and the e2e spec *"reports retained documents, and convergence, on +GitTarget status"*. Two were added while building: + +- **The enqueue is on a change only** — first report and every transition enqueue, an unchanged + report does not. Both halves are asserted, because the second is what stops a deliberately + retaining target from re-reconciling on every resync of every scope forever. +- **The e2e reads an absent block as a failure, not as zero.** `retainedDocumentsOf` fails when the + jsonpath is empty rather than defaulting to `0`; without that, the convergence assertion would + pass before any resync had reported. + +## Open questions for review + +1. **Should `Never` also report its suppressed explicit deletes?** Under `Never` the DELETE gate + simply returns and counts nothing, so `retainedDocuments` would cover only the sweep half. A + `Never` target could therefore report `0` while actively declining to mirror deletes — arguably + the more surprising retention of the two. Options: leave it sweep-only and say so in the godoc; + add a second counter; or make the field mean "documents kept by policy" across both paths, which + is more honest but needs counting in the event writer too. +2. **Is `0` vs absent worth the extra state?** It costs a pointer field and the "record zero actively" + requirement. A plain count where zero and unknown collapse is simpler, but then status can never + say "converged" — which is half the value. +3. **Enqueue on `0 → n`, or accept the requeue lag?** Given the flake history in this projection + class, accepting the lag and documenting it is the conservative default. +4. **Step 0 only, if the window matters more?** The metric labels are a few lines and close the + operational question; the status field is the audit question. Shipping only Step 0 in #260 and the + status field immediately after is a legitimate split if closing the release window is the priority. + +### How they were answered + +1. **Sweep-only, and the godoc says so.** `retainedDocuments` counts the inferred path alone, so a + `Never` target can report zero while still declining to mirror deletes. Counting the two together + would need the event writer to count as well, and — more importantly — would merge a number + derived from a *snapshot* with one derived from *events*, which respond to different failures. The + field's `kubectl explain` text carries the caveat rather than leaving it to be discovered. +2. **Yes — a pointer field, with zero recorded as actively as any other count.** Absent and zero are + the two things an operator most needs told apart, and a plain count cannot say "converged". + `TestRetentionRollup_ZeroIsRecordedAsActivelyAsAnyOtherCount` exists because that is the half most + easily lost in a later refactor. +3. **Enqueue on change — the opposite of the conservative default this plan proposed.** Waiting out + the steady requeue (5 minutes) for the *first appearance* of a retention is too long for a signal + an operator consults before flipping a target to `Always`, and it would leave the e2e assertion + with nothing better than a long sleep. The flake history argues against a projection that does + *not* enqueue, which is the opposite failure. It enqueues on a change of the count or the mode, + never on an unchanged report, so a steadily retaining target does not re-reconcile forever. +4. **Not split — everything shipped in #260.** See the status note at the top. + +## Done when + +- `status.retention` reports the effective mode and a bounded retained count for both new and legacy + GitTargets, and returns to `0` when a resync finds nothing to retain. +- The roll-up is epoch-based and shares the scope lifecycle `RenderFidelityGate` already owns, rather + than maintaining a second one. +- Both retention signals — the metric and the throttled log line — identify the GitTarget. +- No condition changes state because of retention. +- `task lint`, `task test`, and `task test-e2e` pass. + +## Implementation notes + +Decisions taken while building this that the plan above did not settle. + +### The mode travels with the count, not with the spec + +The plan said `mode` is duplicated onto status so a legacy GitTarget's behaviour is explainable +without a second lookup. Building it surfaced a second reason, and it changed where the value comes +from: the controller could have read `EffectivePruneMode()` off the object it is already reconciling, +but then a target patched to `Always` would publish the new mode beside a count the *old* one +produced, until the next resync reported. So `ResyncStats` carries `PruneMode` alongside `Retained`, +and the pair is written and read together. The mode on status is the mode that produced the number. + +### Retention is not the render-fidelity gate, only its epoch + +The plan's most important reuse decision was to take the epoch from `RenderFidelityGate` rather than +write a second scope-lifecycle tracker, and that held: `MarkTargetRetention` takes the epoch +`enqueueReplayResync` already computes, a newer epoch replaces the whole per-scope map, and an older +one is dropped. What it does **not** do is live inside the gate. The gate decides whether a target +may be written to; retention decides nothing at all. Sharing the epoch is reuse; sharing the +structure would have put an observation inside a gate, which is exactly the confusion this plan +exists to avoid. + +### `Retained` is a plan view, and has to be + +Every other `ResyncStats` field is counted from what the apply *did* — deliberately, because a +sensitive resource is `PlanSkip` in the plan while `applyUpsert` really does rewrite it, so +plan-derived stats would report a real commit as skipped. `Retained` is the exception and cannot be +anything else: it counts drops the planner did not emit, so there is no action to observe. The +comment at the assignment says so, because "count from the apply, not the plan" is otherwise the +rule in that function. diff --git a/docs/design/watchrule-source-namespace/pr5-review-followups.md b/docs/design/watchrule-source-namespace/pr5-review-followups.md new file mode 100644 index 00000000..01d590a2 --- /dev/null +++ b/docs/design/watchrule-source-namespace/pr5-review-followups.md @@ -0,0 +1,292 @@ +# PR 5 review follow-ups + +> Work list from a review of PR 5 ([#260](https://github.com/ConfigButler/gitops-reverser/pull/260)): +> two behavioral gaps and two documentation corrections. **Every claim below was re-verified against +> the code before being accepted**, and all four hold. Nothing here reopens the +> [deletion-safety decision](pr5-gittarget-deletion-safety.md) — `OnEvent` stays the default, and the +> review reached that conclusion independently. +> +> **Status: all four landed on this branch.** All gates were green when the review ran (`task lint`, +> `task test`, and `task test-e2e` at 73 passed / 22 skipped / 0 failures), so nothing below was +> caught by a gate — which is the point of writing it down. Each section keeps the analysis that +> justified the fix; the **Landed** note at its end records what was actually done and which test +> holds it. After the fixes: 75 passed / 22 skipped / 0 failures, unit coverage 77.8%. + +## The two blockers are one defect seen from both sides + +PR 5 applies `prune.mode` correctly everywhere it is *read*. Both high findings are about what +happens when the value **changes**: it is captured at a moment and never re-read. + +| Operator does | Intent | What should happen | What happens | +|---|---|---|---| +| `OnEvent` → `Always` | "converge this mirror" | the orphans get swept | nothing, until a replay happens for an unrelated reason (**R1**) | +| `Always` → `OnEvent`/`Never` | "stop deleting, now" | queued deletions stop | a locally committed, unpushed write can still replay under `Always` (**R2**) | + +So R1 and R2 are opposite halves of one missing rule: **the effective mode must be re-read at the +moment it is acted on, and a change in it must be an event.** The two fixes are deliberately +asymmetric, because the risk is asymmetric — a loosening may take its time but must be triggered; a +tightening must take effect immediately and must trigger nothing. + +## Summary + +| # | Item | Verdict | Status | +|---|---|---|---| +| R1 | Declaring `Always` does not converge a quiet target | Confirmed | **Fixed** | +| R2 | A tightening is outrun by an already-retained write on rebase replay | Confirmed, **exposure narrowed** | **Fixed** | +| R3 | The retention log does not name the GitTarget the docs promise it names | Confirmed | **Fixed** — with [retention visibility](pr5-retention-visibility.md) Step 0 | +| R4 | The docs claim a failure mode the code does not have | Confirmed | **Fixed** | +| — | Keep the retention metric target-unlabelled for cardinality | **Declined** | Not taken | + +## R1 — declaring `Always` does not converge a quiet target + +[UPGRADING.md](../../UPGRADING.md) tells an operator that declaring `Always` keeps the old behavior, +and [configuration.md](../../configuration.md) that it gives a "faithful, converged mirror". A +`prune.mode` edit does neither on its own: + +- the GitTarget controller's only force flag is `gitPathWasRefused` + ([`gittarget_controller.go`](../../../internal/controller/gittarget_controller.go)); +- an unchanged watch-spec set returns early from `prepareTargetWatchSetReplacementLocked` + ([`target_watch.go`](../../../internal/watch/target_watch.go)) — `equalTargetWatchSpecs` compares + GVR, namespace, and operation filter, and the prune mode is (correctly) not among them; +- the **only** production resync enqueue is `enqueueReplayResync`, reached from a completed initial + replay or the LIST fallback. Nothing else in the operator enqueues a resync. + +The part that makes this worse than "converges eventually" is the reconnect path. After the first +session `runTargetWatch` sets `resumeFromCursor = true`, and a cursor resume streams live events +**without** enqueuing anything. So the practical trigger set for a sweep is: a controller restart, a +WatchRule edit that changes this target's watch set, or a cursor expiry. A healthy target with stable +rules whose cursors keep resuming can sit under `Always` indefinitely without ever sweeping the +orphans the operator declared `Always` to remove. + +The e2e suite already contains the workaround, which is the strongest evidence the gap is real — +the sweep spec has to churn the rule to make a resync happen at all: + +~~~go +By("toggling ConfigMaps off and back on to force a scoped replay resync") +~~~ +— [`prune_mode_e2e_test.go`](../../../test/e2e/prune_mode_e2e_test.go) + +**Fix.** Edge-triggered force. Remember the last-declared effective mode per `gitDest` beside the +source cluster id (`rememberGitTargetCluster` in +[`materialization.go`](../../../internal/watch/materialization.go) is the shape), and force the watch +set when it has changed *to* a sweeping mode: + +~~~go +DeclareForGitTarget(ctx, gitDest, target.SourceCluster(), gitPathWasRefused || pruneModeBecameSweeping) +~~~ + +`force` already does the right thing end to end: it cancels the prior set, and the first session of +the replacement replays rather than resuming, which is exactly what enqueues the resync. + +Two things this must get right, both silent if wrong: + +- **Edge, not level.** Forcing whenever the mode *is* `Always` forces on every steady requeue, which + is a permanent replay loop. Only a *change* may force. +- **Only the loosening direction.** `always → never` needs no replay, and forcing one would tear down + every stream at the exact moment an operator is trying to stop something from happening. The + remembered mode must also be dropped in `ForgetGitTargetDeclaration` with the rest of the + per-target state. + +**Considered and not chosen:** putting the mode into the watch-set identity (`equalTargetWatchSpecs`). +It makes both directions do the same expensive thing, including the one that must not churn, and it +puts a policy value into a structure that otherwise describes only *what is being watched* — two +concerns that would then have to be kept in sync forever. + +**Worth noting for later:** `ResyncRequest.Heal` exists for precisely this shape — its doc comment +says "a periodic checkpoint re-anchor or a removed-type sweep" +([`types.go`](../../../internal/git/types.go)) — and it currently has **no production producer**; +every call site passes `heal: false`. A heal-shaped "re-list this target's scopes and enqueue a +converging resync" would avoid stream churn altogether. That is a larger change than this PR should +carry; the force flag is the right size now, and the heal path is the better long-term home. + +**Test.** e2e: seed an orphan under the default mode, patch `spec.prune.mode: Always`, and observe +the sweep **without touching the WatchRule**. That is the test that would have caught this, and it +also lets the existing sweep spec drop its toggle workaround. Unit: a declare whose mode changed to +`Always` replaces the watch set; an unchanged mode does not; and `always → onEvent` does not. + +**Landed.** [`prune_declaration.go`](../../../internal/watch/prune_declaration.go) holds the whole +seam — `pruneModeRequiresReplay` / `rememberGitTargetPruneMode` / `forgetGitTargetPruneMode` — and +`DeclareForGitTarget` now takes the effective mode beside the source cluster. Three details worth +recording: + +- The mode is remembered **only after `EnsureGitTargetWatches` succeeds**. A failed declare must + leave the pending force standing for the next reconcile rather than consuming it on an attempt + that never reached the data plane, so the value means "what the running watches were built for". +- The `forgetGitTargetPruneMode` rationale in the first draft of this document was wrong, and the + code comment says so instead: a recreated GitTarget has no watch set to replace, so its first + declare replays regardless. Forgetting is lifecycle hygiene — an entry per deleted target, and a + claim about state that no longer exists — not a correctness fix. +- `TestReplaceGitTargetWatches_ForceReplaysAnUnchangedSet` asserts the mechanism itself with an + explicit negative control (the same specs without the flag reopen nothing), because every other + test in this area would pass whether or not the flag reached the watch layer. + +Held by [`prune_declaration_test.go`](../../../internal/watch/prune_declaration_test.go) — a truth +table over all nine transitions, the two `false` rows carrying the reasons — and by the e2e spec +*"converges an existing orphan when prune.mode is widened, without touching the WatchRule"*, which +seeds a second orphan whose only possible trigger is the patch. + +## R2 — a tightening is outrun by an already-retained write + +`ResolvedTargetMetadata.PruneMode` is captured at plan time, and its doc comment states outright that +a rebase replay applies the policy in force when the write was planned rather than the current one +([`types.go`](../../../internal/git/types.go)). `executeResyncPendingWrite` passes that stored value +into the planner ([`resync_flush.go`](../../../internal/git/resync_flush.go)). + +**The exposure is the retained-write window, not the queue** — worth stating precisely, because it +narrows the finding without dismissing it. A *queued resync request* is safe: `buildResyncPendingWrite` +calls `resolveTargetMetadata` at apply time, so it reads the current spec. What is not safe is a +`PendingWrite`: committed locally, retained until push (`PushCooldown` is 5s, and much longer while +pushes keep failing), and **re-executed** on a push conflict via `pushPendingCommits` → +`rebuildPendingWrites` → `executePendingWrites` +([`branch_worker.go`](../../../internal/git/branch_worker.go)). The replay re-plans against the newly +rebased worktree, so it can compute drops the first apply never made — against a remote that changed +in the meantime — under the superseded policy. + +The existing doc comment is half right, and the half it gets right is worth keeping. Its reasoning +holds for the **loosening** direction: a write planned under `OnEvent` must not start sweeping merely +because someone set `Always` afterwards, since that snapshot's retention decision was already taken +against a desired set that is now stale. It does not hold for the **tightening** direction, where +stopping deletions that have not yet landed is the entire purpose of the edit. + +**Fix.** Order the modes (`Never` < `OnEvent` < `Always`) and apply the **minimum** of the stored and +the current value at execution time. Minimum delivers both halves at once: a loosening cannot +escalate an already-planned write (what the doc comment wanted), and a tightening applies immediately +(what it missed). + +The read-failure case needs a deliberate answer rather than a fallback: if the GitTarget cannot be +read at replay time — deleted, or a cold cache — treat it as the most restrictive. A missed sweep is +redone by the next resync; a sweep authorized by a policy the process could not read is not undoable. +This is the same rule the implementation already adopted for an unrecognized enum value, recorded in +[part 1's implementation notes](pr5-gittarget-deletion-safety.md#the-empty-string-is-unset-not-never): +a policy this build cannot resolve does not authorize deletion. + +**The event path has the same shape.** Retained live-event windows carry the same metadata and +`pruneModeForBase` reads the stored value +([`pending_writes.go`](../../../internal/git/pending_writes.go)). Severity is lower — those deletes +are observed evidence, not inference — but `Never` means "do not mirror deletes", and a window that +has not pushed has not mirrored anything yet. Fix both; the resync path is the blocker. + +**Test.** A replay-seam regression: retain a resync write planned under `Always`, tighten the +GitTarget to `OnEvent`, force a push conflict so the write replays, and assert the replay retains. +Plus a unit test on the min-of-two helper covering the unreadable-target case, which is the branch +most likely to be written the wrong way round. + +**Landed.** `PruneMode.MoreRestrictiveOf` in +[`prune_policy.go`](../../../api/v1alpha3/prune_policy.go) orders the modes; +`tightenPendingPruneModes` in [`branch_worker.go`](../../../internal/git/branch_worker.go) applies +it in `rebuildPendingWrites`, immediately before the replay re-plans. Three decisions the design +above did not settle: + +- **The tightening mutates the shared `Targets` map**, so one pass covers both deletion paths: the + sweep reads its mode through `PendingWrite.Target`, the DELETE writer through `pruneModeForBase`, + and both read the same map. It also means the tightening survives every subsequent push attempt, + which is right — a policy decrease is not undone by a later increase. +- **A failed read returns an error rather than guessing**, which the design's "use the more + restrictive" rule did not cover. Guessing the captured mode could apply a revoked deletion; + guessing the strictest could silently drop a legitimate one, and under `OnEvent` no later resync + re-derives an event delete. Returning the error leaves the pending writes retained and the push + cycle retries them, so neither happens. A **deleted** GitTarget is different — that is a definite + answer, no policy exists, and it replays under `Never`. +- **One read per GitTarget, not per retained write.** A conflicting push can be replaying many + windows for one busy target. + +Held by [`prune_replay_test.go`](../../../internal/git/prune_replay_test.go): both directions, the +deleted and unreadable targets, the both-paths assertion, the read-count bound, and the ordering +table including the unrecognized value. + +## R3 — the retention log does not name the GitTarget + +`reportRetainedOrphans` logs `retained`, `pruneMode`, `path`, and `scope` +([`resync_flush.go`](../../../internal/git/resync_flush.go)) — no GitTarget namespace or name. Both +user-facing docs promise otherwise: "logs a throttled line naming the target" +([UPGRADING.md](../../UPGRADING.md)) and "the operator logs a line naming the target" +([configuration.md](../../configuration.md)). `path` is not an identifier: two GitTargets in +different namespaces can write the same `spec.path` on different branches of the same repository. + +The caller already holds what is needed — `executeResyncPendingWrite` has the resolved target and +already passes its `PruneMode` down the same call. + +This is the same defect as the metric's missing labels, so it is tracked once, in +[retention visibility → Step 0](pr5-retention-visibility.md#step-0-make-both-retention-signals-name-the-gittarget), +rather than in two places. + +**One disagreement recorded.** The review asks to leave the metric target-unlabelled for cardinality. +Declined: `gittarget_namespace` / `gittarget_name` is the established label pair for this codebase's +per-target counters (`TargetReconcileCompletedTotal` and its neighbours in +[`telemetry/exporter.go`](../../../internal/telemetry/exporter.go)), cardinality is bounded by the +number of GitTargets rather than by resources, and "which target is retaining" is the operational +question the counter exists to answer. What must stay off the metric is per-path, per-scope, or +per-document labels — that is unbounded, and it is what the log line is for. + +**Landed.** Both signals in [`resync_flush.go`](../../../internal/git/resync_flush.go) now carry the +target. Two things came out of it that the finding did not ask for: + +- **The throttle key was the defect too.** It was the path alone, so two co-resident targets sharing + a `spec.path` would have had one silently suppress the other's line for ten minutes. It is now the + GitTarget plus the path. +- **`applyResyncToWorktree` takes the `ResolvedTargetMetadata` instead of four unpacked fields** + (cluster id, placement, prune mode, and now the identity). That is what made the target reference + available without an eighth parameter, and it moved the `OrDefault` normalization to a single + entry point rather than leaving each reader to remember it. + +Held by [`retention_report_test.go`](../../../internal/git/retention_report_test.go), including the +legacy target routed through the real apply so the assertion covers the normalization seam and not +just the log call. + +## R4 — the docs claim a failure mode the code does not have + +[configuration.md](../../configuration.md) and [UPGRADING.md](../../UPGRADING.md) both say that a +source-cluster outage or a narrowed RBAC grant produces a snapshot smaller than reality. The gather +is fail-closed against both, in [`target_watch.go`](../../../internal/watch/target_watch.go): + +- a failed watch open or LIST marks the stream `Blocked` and returns an error — no resync is + enqueued, so no sweep is planned from a partial snapshot; +- a streaming replay enqueues only after the `initial-events-end` bookmark is folded + (`foldTargetReplayEvent`). A replay cut off mid-stream enqueues nothing at all. + +RBAC also cannot narrow *within* a stream: a denied list/watch fails the whole scope rather than +returning a subset, so there is no smaller-but-accepted snapshot to sweep from. + +The honest rationale is the one part 1 already leads with — a snapshot that is **complete but +computed against the wrong scope**: a bad rule scope, version skew, a controller that does not +understand a newer scope field. Outages and authorization failures should be described as the case +the controller currently declines to sweep on, with `OnEvent` as defence in depth for the day that +changes: fail-closed is a property of today's gather, not a guarantee the API makes. + +The same overstatement is in part 1's own Purpose section ("narrowed by a bad scope, a temporary +outage, or a controller that does not understand a newer scope field"), so the correction is needed +in three places, not the two the review names. + +Docs-only, no code dependency — this can land ahead of R1 and R2. + +**Landed.** All three corrected, in the shape the review recommended: the scope rationale keeps the +lead, and the outage/RBAC cases are described as what the controller currently declines to sweep on, +with `OnEvent` as defence in depth because failing closed there is a property of today's gather +rather than a guarantee. Two adjacent inaccuracies went with them — the resync is not "periodic" +(it happens when a stream starts or restarts), and the mutability paragraphs now say what a +`prune.mode` edit actually does in each direction, which is only true because of R1. + +## What the review confirmed + +Recorded because these are the properties most likely to be broken by a later change, and each is now +a claim a reviewer has checked independently: legacy and empty policy values resolve to `OnEvent`; an +unrecognized value fails closed toward retention; suppression happens during planning, so a retained +deletion never enters actions, ordering, stats, or commits; explicit DELETEs stay functional under the +default and `Never` is genuinely archival; the offline analyzer deliberately keeps its full-convergence +reporting; and the e2e retention assertion is guarded by a positive barrier — the `Always` target's +sweep proves a resync ran — rather than by a bare wait. + +## Sequencing + +All four landed in [#260](https://github.com/ConfigButler/gitops-reverser/pull/260) itself, together +with the [retention visibility](pr5-retention-visibility.md) work: R1 and R2 are policy-*transition* +defects in the feature the PR exists to add, one of them undermines the migration instruction the +release ships with, and both are small. + +The [do-not-release window](pr5-gittarget-deletion-safety.md#release-and-rollback) is unchanged by +anything here: `main` holds PR 4's breaking scope rework without PR 5's deletion safety until #260 +merges, so these items were on the critical path rather than beside it. + +Validated with the standard sequence — `task lint` (0 issues), `task test` (unit coverage 77.8%, at +the baseline, so `.coverage-baseline` is unchanged), and `task test-e2e` (75 passed, 22 skipped, 0 +failures), the e2e legs run sequentially. diff --git a/docs/future/flux-maintainer-review-status-and-config-model.md b/docs/future/flux-maintainer-review-status-and-config-model.md new file mode 100644 index 00000000..4b570b95 --- /dev/null +++ b/docs/future/flux-maintainer-review-status-and-config-model.md @@ -0,0 +1,652 @@ +# Review: the configuration model and status implementation, read as a Flux maintainer + +> Status: external review — findings open, nothing here binds until scheduled. +> Date: 2026-07-21 +> Reviewed at: branch `feat/gittarget-prune-mode-pr5`, commit `f37a7ba`. +> +> **Scheduled and done so far:** F12's *enum casing* only — `PruneMode` is now +> `Never`/`OnEvent`/`Always`, taken before the release because it was the last moment it was free. +> Every other finding, including the rest of F12, is still open. +> Stance: reviewed as if this API were proposed for the GitOps Toolkit, with Flux's own +> source (`external-sources/flux/`) and kstatus (`sigs.k8s.io/cli-utils/pkg/kstatus`) as ground +> truth rather than recollection. + +**What was read** + +- The API surface: `api/v1alpha3/*.go` (all six kinds plus `NamespaceMatcher`, `PrunePolicy`). +- The status implementations: `internal/controller/{gittarget,watchrule,clusterwatchrule,gitprovider,clusterprovider,commitrequest}_controller.go`, `condition_helper.go`, `stream_status.go`, `gittarget_dependency_status.go`, `gittarget_source_cluster.go`, `internal/watch/stream_readiness.go`. +- `docs/configuration.md`, `docs/spec/status-conditions-guide.md`, `docs/spec/where-validation-lives.md`, `docs/design/reconcile-triggering.md`. +- Flux as ground truth: `external-sources/flux/pkg/apis/meta`, `external-sources/flux/pkg/runtime/{conditions,patch}`, `external-sources/flux/pkg/apis/acl`, `external-sources/flux/flux2/rfcs/`, `external-sources/flux/flux-operator/api/v1`. +- kstatus itself: `sigs.k8s.io/cli-utils/pkg/kstatus/status` from the module cache. + +Read-only review: no builds, no tests, no edits to the tree. + +--- + +## 1. The standard + +It is **kstatus** — `sigs.k8s.io/cli-utils/pkg/kstatus`, from Kubernetes SIG-CLI. It is what +`kubectl apply --wait`, kpt, Config Sync, Argo's health model (in spirit), and the Flux CLI all +lean on. Its upstream is the **Kubernetes API conventions**, section *"typical status properties"*, +which defines the abnormal-true polarity rule. Flux codifies both in +`fluxcd/pkg/apis/meta` — `ReadyCondition` / `StalledCondition` / `ReconcilingCondition` +(`external-sources/flux/pkg/apis/meta/conditions.go:43-59`) — and the flux-operator boils +readiness down to one CEL expression +(`external-sources/flux/flux-operator/api/v1/common_types.go:23`): + +``` +status.conditions.filter(c, c.type == 'Ready').all(c, c.status == 'True' && c.observedGeneration == metadata.generation) +``` + +**The precise rules kstatus enforces** (verified by reading +`cli-utils/pkg/kstatus/status/generic.go`, not from memory): + +1. `metadata.deletionTimestamp` set → `Terminating`. +2. `status.observedGeneration != metadata.generation` → `InProgress`. +3. any condition `Reconciling` **with status True** → `InProgress`. +4. any condition `Stalled` **with status True** → `Failed`. +5. otherwise → `Current`. + +Two consequences that matter enormously here, and that are easy to get wrong: + +- kstatus **never reads `Ready`** for a custom resource. `Ready=False` with `Reconciling` and + `Stalled` both False reads as **`Current`** — "done, healthy". The `Ready` condition is for + `kubectl wait` and humans; the trio is for machines. They must never disagree. +- kstatus only reacts to those conditions **when True**. That is why the convention says they + MUST NOT be present when False. + +### Verdict on compatibility + +**This project is on the standard, deliberately and knowledgeably — closer than most projects at +this stage.** `docs/spec/status-conditions-guide.md` states the contract correctly, and +`internal/controller/gittarget_kstatus_test.go` asserts it against the *real* kstatus library +rather than a hand-rolled reimplementation. That is mergeable work. + +What is not yet mergeable is the gap between that stated contract and what the reconcilers +actually emit. Three defects (F1–F3) make the trio lie in states production will reach, and the +tests do not catch them because they assert hand-built condition sets rather than the output of a +reconcile. + +| Area | Grade | Note | +|---|---|---| +| Condition types (`Ready`/`Reconciling`/`Stalled`) | **A** | Correct vocabulary, correct meanings, documented. | +| `observedGeneration` on object **and** per-condition | **A** | Passes the flux-operator CEL health expr as written. | +| `+listType=map` / `+listMapKey=type` on conditions | **A** | Correct on all six kinds. SSA-safe. | +| kstatus **conformance tests** | **A–** | Real library, real `Compute()`. Fixtures are synthetic (see F1/F2). | +| kstatus **behaviour under real reconciles** | **D** | F1 masks `Failed`; F2 never reaches `Current`. | +| Abnormal-true polarity (MUST NOT be present when False) | **C** | Always written as `False`. Tolerated by kstatus, violates the contract. | +| Status write discipline (no-op suppression) | **D** | Unconditional writes + always-moving timestamps (F3). | +| Reason vocabulary | **C** | Ad-hoc; `Reason == Type` in several places. | +| Flux object contract (`suspend`, `interval`, `requestedAt`, Events) — see layer 3 below | **F** | None of it present. Known — `docs/design/reconcile-triggering.md`. | +| `no phase string` discipline | **A** | Conditions only, everywhere. Correct. | + +### Three different things get conflated below — only one of them is a standard + +This review keeps saying "the GitOps Toolkit object contract". That is shorthand, not an official +name, and it is worth separating the three layers because they carry very different weight. + +**1. The Kubernetes API conventions — normative, upstream.** +`kubernetes/community`, `contributors/devel/sig-architecture/api-conventions.md`, section *"typical +status properties"*. This is where abnormal-true polarity comes from, where "conditions are a map +keyed by type" comes from, and where the UpperCamelCase enum rule (F12) comes from. Anything that +calls itself a Kubernetes API is measured against this. Flux's own condition docs link to it by +URL (`external-sources/flux/pkg/apis/meta/conditions.go:49-50, 57-58`). + +**2. kstatus — a real, named, vendor-neutral convention.** +`sigs.k8s.io/cli-utils/pkg/kstatus`, owned by Kubernetes SIG-CLI. It is a *library plus a +convention*: implement the trio the way it expects and any kstatus consumer can compute your +object's status without knowing what your CRD is. Consumers include cli-utils' own applier, +`kpt live apply`, and the Config Sync / Nomos family built on cli-utils. Flux designs to it +explicitly and says so in the comment above its condition constants. + +Worth knowing what it is *not*: **Argo CD does not use kstatus.** Its health model +(`gitops-engine`, built-in checks plus Lua) is a separate, older implementation of the same idea. +So conforming here buys interoperability with the Flux/kpt/cli-utils side of the ecosystem, not +with Argo. That is still the larger side for anything condition-driven, and it is the side this +project's users already live on. + +**3. The Flux object contract — not a published cross-vendor standard, but real, importable code.** +There is no RFC and no spec document for it. What exists instead is +**`github.com/fluxcd/pkg/apis/meta`** — its own Go module, Apache-2.0, versioned independently of +the controllers, which this repo *already pins at `v1.31.0`* (`go.mod:9`, for +`KubeConfigReference`). It is written in RFC-2119 language and is deliberately the shared artifact +rather than a doc: + +| Codified in `pkg/apis/meta` | Where | +|---|---| +| `ReadyCondition` / `StalledCondition` / `ReconcilingCondition` / `HealthyCondition` | `conditions.go:43-64` | +| Generic reasons: `Succeeded`, `Failed`, `Progressing`, `ProgressingWithRetry`, `Suspended`, `DependencyNotReady`, `InvalidPath`, `InvalidURL` | `conditions.go:80-115` | +| `ReconcileRequestAnnotation = "reconcile.fluxcd.io/requestedAt"` — *"any change in value SHOULD trigger a reconciliation"* | `annotations.go:23` | +| `ForceRequestAnnotation = "reconcile.fluxcd.io/forceAt"` — explicitly *"used to standardize the mechanism across controllers"* | `annotations.go:32` | +| `ReconcileRequestStatus` — an **embeddable struct** carrying `lastHandledReconcileAt`, plus `StatusWithHandledReconcileRequest` / `ObjectWithAnnotationRequests` interfaces | `annotations.go:49-133` | +| `LocalObjectReference`, `NamespacedObjectReference`, `NamespacedObjectKindReference`, `SecretKeyReference`, `KubeConfigReference` | `reference_types.go` | +| `AccessDeniedCondition` / `AccessDeniedReason`, `AccessFrom` | sibling module `pkg/apis/acl` | + +**What is deliberately *not* in that module: `spec.suspend` and `spec.interval`.** `meta` ships +`SuspendedReason` but no `Suspend` field — the fields themselves are hand-repeated in every fluxcd +controller's own API types. So "adopt the contract" means *import the type* for conditions, +reasons, annotations and references, but *copy the field shape* for suspend/interval. F6 is +therefore a convention-matching argument, not a dependency argument. + +**Who adheres.** Verifiable from this checkout: every fluxcd controller (source-, kustomize-, +helm-, notification-, image-\*) via `flux2`; and ControlPlane's **flux-operator** — a different +vendor under a different licence (AGPL) — which adheres and then extends it with its own +`FluxObject` interface and a CEL readiness expression +(`external-sources/flux/flux-operator/api/v1/common_types.go:23, 44-64`). That second one is the +interesting data point: an independent project chose to implement the same contract rather than +invent one, because it is what makes `flux`-shaped tooling work against non-Flux kinds. + +**Why this matters for F6/F7 specifically.** The argument is interop, not compliance. Nobody will +fail an audit for lacking `spec.suspend`. But a platform team that already runs Flux has muscle +memory — `suspend` to pause, annotate `requestedAt` to force, `kubectl describe` to see what +happened, notification-controller to route failures — and every one of those reflexes currently +returns nothing here. Meanwhile the marginal cost is close to zero: the module is already a +dependency, so adopting the condition types, reason constants and `ReconcileRequestStatus` is an +import and a struct embed, not a new supply-chain decision. + +--- + +## 2. Findings + +### F1 — `downgradeReady` erases a terminal `Stalled`, downgrading kstatus `Failed` → `InProgress` (High) + +`internal/controller/gittarget_controller.go:240` runs `applyDataPlaneConditions`, which for a +refused Git path sets the correct terminal trio (`Ready=False`, `Reconciling=False`, +`Stalled=True`, reason `UnsupportedContent`) — `gittarget_controller.go:526-543`. + +Then `internal/controller/gittarget_controller.go:258` runs `projectSourceAndProvider`, which on +any source/provider imperfection calls `downgradeReady` +(`internal/controller/gittarget_source_cluster.go:224-233`): + +```go +r.setCondition(target, GitTargetConditionReady, readyStatus, reason, message) +r.setCondition(target, GitTargetConditionReconciling, metav1.ConditionTrue, reason, message) +r.setCondition(target, GitTargetConditionStalled, metav1.ConditionFalse, ReasonProgressing, ...) +``` + +It unconditionally stamps `Stalled=False, Reconciling=True`, wiping the stall set 18 lines +earlier. The doc comment says it "only ever DOWNGRADES Ready" — true of `Ready`, but for +**kstatus** it *upgrades* the object from `Failed` to `InProgress`. + +**Trigger, most likely path:** a remote-source GitTarget before first discovery has +`SourceClusterReachable=Unknown` (`gittarget_controller.go:249-255`), which hits the +`reachStatus == metav1.ConditionUnknown` branch (`gittarget_source_cluster.go:212`). Any +GitProvider blip does it too. So: a GitTarget with unsupported kustomize content in its folder, or +`RenderMatchesLive=False`, reports "still working on it, please wait" forever to every kstatus +consumer, instead of failing fast. Nothing in the folder is being written, and nothing will be. +`GitPathAccepted=False` is still there for a human who goes looking; `kubectl wait` and any +kstatus-driven CI gate hangs to timeout. + +This is exactly why Flux computes the summary **once, at the end, from a declared precedence +order** rather than by successive mutation — the `summarize` pattern over `conditions.Set` with +`patch.WithOwnedConditions` (`external-sources/flux/pkg/runtime/patch/options.go:67-72`). + +**Fix.** Collect `(status, reason, message)` candidates from every gate into a list, then derive +the trio once at the end with stated precedence: `Stalled=True` wins over `Reconciling=True` wins +over `Ready=True`. `docs/spec/status-conditions-guide.md:69-73` already states the canonical +reads — make one function the single writer of the trio, and make every gate a *contributor*, not +a *setter*. Add a table-driven test that runs the actual reconcile and feeds the resulting object +to `kstatus.Compute`, including the "path refused **and** provider unready" cell. + +--- + +### F2 — A GitTarget with no WatchRules is never `Current`, and requeues every 10 s forever (High) + +`internal/watch/stream_readiness.go:75-77`: + +```go +func (s StreamSummary) StreamsRunning() bool { return s.Total > 0 && s.Ready == s.Total } +``` + +Zero tracked types → `StreamsRunning()==false` → the `!streams.StreamsRunning()` branch at +`gittarget_controller.go:562` → `Ready=False`, `Reconciling=True`, reason `NoResolvedTypes`. +Permanently, because nothing will ever resolve. And because `streamsSettling` is true +(`gittarget_controller.go:232`), the reconcile returns `RequeueAfter: RequeueStreamSettleInterval` += **10 s** (`constants.go:117`), forever. + +That state is not exotic — it is **step 3 of the documented setup flow** +(`docs/configuration.md:29-34`: create GitTarget, *then* create WatchRules), and it is the steady +state of any target whose rules were deleted. A user following the docs has an object that +`kubectl wait --for=condition=Ready` never returns on, and that burns a reconcile plus a status +write every 10 seconds indefinitely. + +Empty is not in-progress. Nothing is pending. "I have nothing to mirror" is a **converged** +state — Flux's Kustomization with an empty path is `Ready=True` with a "no objects" message, not +InProgress. + +**Fix.** Split "nothing resolved" from "resolving". `Total == 0` should be `Ready=True`, +`Reconciling=False`, `Stalled=False` with reason `NoWatchRules` (or `NoResolvedTypes`) and a +message saying so — kstatus `Current`, honest, and it drops back to the 5 min cadence. Keep +`status.streams.summary: "0/0"` so the zero stays visible. If that feels like hiding a +misconfiguration, that is what an Event or a `Ready` *reason* is for, not a permanent +`Reconciling=True`. + +--- + +### F3 — Unconditional status writes with always-moving timestamps, plus no self-predicate, create a self-triggering reconcile edge (High) + +Three things compose badly. + +1. **Every reconcile stamps a fresh timestamp.** + `gittarget_controller.go:129` — `target.Status.LastReconcileTime = metav1.Now()`, unconditional. + `internal/watch/stream_readiness.go:244` — `StreamSummary{..., ObservedTime: metav1.Now()}`, + surfaced into `status.streams.observedTime` for GitTarget *and* both rule kinds + (`gittarget_controller.go:1065-1078`, `stream_status.go:26-40`). + +2. **The status write is unconditional and full-object.** `updateStatusWithRetry` + (`gittarget_controller.go:1081-1113`, and the near-identical copies in + `watchrule_controller.go:405`, `clusterprovider_controller.go:283`, + `gitprovider_controller.go:403`) does `latest.Status = target.Status; r.Status().Update(...)`. + There is no "did anything change?" check — and the timestamps guarantee something always did. + +3. **`For()` carries no predicate on GitTarget, WatchRule, or ClusterWatchRule** + (`gittarget_controller.go:1117-1118`, `watchrule_controller.go:463-464`, + `clusterwatchrule_controller.go:547`). Status-subresource writes bump `resourceVersion` and + fire an Update watch event, which `handler.EnqueueRequestForObject` puts straight back on the + queue — un-rate-limited. + +So each reconcile enqueues itself. It is not an unbounded spin — `metav1.Time` serialises at +RFC3339 second precision, so a follow-up reconcile that lands inside the same wall-clock second +produces a byte-identical status and the apiserver no-ops it. The practical shape is therefore: +**every reconcile costs roughly two reconciles and at least one etcd write, degenerating into a +sustained self-sustaining loop whenever a reconcile takes ≥1 s** — plausible here, since +`checkForConflicts` does a cluster-wide `List` of every GitTarget on every pass +(`gittarget_controller.go:793`) on top of several `Get`s and `DeclareForGitTarget`. + +Combine with F2 and an idle GitTarget writes to etcd at ~0.1 Hz forever, per object, and wakes +every controller watching GitTargets each time. + +`GitProvider` and `ClusterProvider` **do** have self-predicates +(`gitprovider_controller.go:462-465`, `clusterprovider_controller.go:332-335`), so this is an +inconsistency, not a house style. `docs/design/reconcile-triggering.md:44-50` inventories +per-controller predicates but records only the *dependency* edges — the missing `For()` predicate +on GitTarget/WatchRule is not in that table. + +**Fix, in order of value:** + +- Adopt `fluxcd/pkg/runtime/patch.Helper`. It computes a merge patch of *what actually changed* + and sets `observedGeneration` "only if there is a change" + (`external-sources/flux/pkg/runtime/patch/options.go:33-35`). No change → no request → no watch + event → no loop. A drop-in: `fluxcd/pkg/apis/meta` is already a dependency. +- Failing that: `if !equality.Semantic.DeepEqual(latest.Status, target.Status) { update }`, and + drop `LastReconcileTime` / `observedTime` from the comparison — or drop the fields. +- Ask whether `status.lastReconcileTime` and `status.streams.observedTime` earn their keep at all. + Flux deliberately does not carry a "last reconcile attempt" timestamp; a condition's + `lastTransitionTime` plus `controller_runtime_reconcile_total` answer the same question without + making every object mutable-on-read. `LastPushTime` is genuinely useful (it records a real + event) — keep that one. +- Add a `For()` predicate on the three controllers that lack one, e.g. + `predicate.Or(GenerationChangedPredicate{}, )`. + +--- + +### F4 — Abnormal-true polarity: `Reconciling`/`Stalled` are always present, including when False (Medium) + +`external-sources/flux/pkg/apis/meta/conditions.go:47-59` states it twice: + +> The Condition adheres to an "abnormal-true" polarity pattern, and **MUST only be present on the +> resource if the Condition is True**. + +Every write path here emits both unconditionally: `setStalledConditions` +(`gittarget_controller.go:450-457`), `downgradeReady`, `setRuleProgressing` +(`stream_status.go:114-123`), `setReadyConditions` / `setProgressingConditions` on both providers. + +kstatus tolerates it — it only tests for `== True` — so this is not a correctness bug. But it is a +contract violation with real costs: + +- `kubectl get gittarget -o yaml` carries six condition entries where three would do. +- Any tool that treats *presence* as signal (a fair reading of the convention) misreads it. +- `Reconciling=False, reason=UnsupportedContent, message="Reconciliation is stalled"` + (`gittarget_controller.go:538-539`) is a condition that says nothing true about reconciling. It + exists only to be overwritten. + +`docs/spec/status-conditions-guide.md:31-32` names the polarity rule correctly and the code does +the opposite. Pick one. Preferred: `DeleteCondition` on the way to False — +`conditions.Delete(obj, meta.StalledCondition)` in Flux terms — and let `Ready` carry the positive +summary. + +--- + +### F5 — `upsertCondition` reorders the condition list on every touch (Medium) + +`internal/controller/condition_helper.go:26-44` rebuilds the slice with the target type *removed* +and then appends it at the end. Touch `Ready` and it migrates to the tail; touch it again next +pass and everything else has shuffled. + +Effects: gratuitous diffs in `kubectl get -o yaml` and in any GitOps repo that mirrors these +objects (which, given what this product does, is not hypothetical — a GitTarget mirroring +GitTargets would commit condition-reordering noise); a byte-level change even when nothing +semantically changed, which feeds F3; and unstable ordering for humans. + +The `LastTransitionTime` handling is *correct* — preserved when `Status` is unchanged +(`condition_helper.go:39-41`), matching `apimeta.SetStatusCondition` and the API conventions. +Flux is actually stricter than the convention here, resetting on Reason/Message change too +(`external-sources/flux/pkg/runtime/conditions/setter.go:44-46`); this repo's is the more +conventional reading and worth keeping. + +**Fix.** Use `k8s.io/apimachinery/pkg/api/meta.SetStatusCondition`, which updates in place. Or +adopt Flux's approach and sort deterministically, with `Stalled`, `Reconciling`, `Ready` weighted +to the front for `kubectl` legibility +(`external-sources/flux/pkg/runtime/conditions/setter.go:89-92, 196-218`) — a nice touch worth +stealing regardless. + +--- + +### F6 — No `spec.suspend`, no `spec.interval`, no reconcile-request annotation (Medium) + +Nothing in `api/v1alpha3` has `suspend` or `interval` — zero hits. The cadence is a compile-time +constant: `RequeueSteadyInterval = 5 * time.Minute` (`constants.go:113`). + +Every Flux object implements all three +(`external-sources/flux/flux-operator/api/v1/common_types.go:44-64`): `GetInterval()`, +`IsDisabled()`, `SetLastHandledReconcileAt()`. They are not decoration: + +- **`spec.suspend`** is the only way to say "stop touching this while I fix the repo by hand" + without deleting the object. For a controller that *writes to a Git repository*, the absence of + a pause button is the single most surprising gap in this API. Today the only way to stop a + GitTarget writing is to delete it or its rules — and per the `GitTargetSpec` doc comment, that + is irreversible for path/branch/provider. +- **`spec.interval`** — `GitProvider` does a real network `ls-remote` against the git host on + every pass (`gitprovider_controller.go:220`, `checkRemoteConnectivity`), hardcoded at 5 min, + with no jitter. N providers against github.com from one operator, all re-synchronised into a + thundering herd after each restart. Flux ships `fluxcd/pkg/runtime/jitter` precisely for this. + At minimum: jitter the requeue. Better: `spec.interval` per object, since a GitProvider pointing + at a rate-limited enterprise host and one pointing at a local Gitea do not deserve the same + cadence. +- **`reconcile./requestedAt` + `status.lastHandledReconcileAt`** is the universal + "reconcile now" idiom (`flux reconcile`, `kubectl annotate`, webhook receivers). This is the one + piece of F6 that is *shared code* rather than a copied field shape: embed + `meta.ReconcileRequestStatus` and call `meta.ReconcileAnnotationValue` + (`external-sources/flux/pkg/apis/meta/annotations.go:23-64`) and the semantics — including the + "any change in value SHOULD trigger" token comparison — come with it. Already identified as F1 + in `docs/design/reconcile-triggering.md`; still unbuilt. + +--- + +### F7 — Zero Kubernetes Events (Medium) + +There is no `EventRecorder` anywhere in `internal/controller` or `cmd/main.go` — no `Recorder`, +`Eventf`, or `Event(` call sites. + +Consequences: `kubectl describe gittarget` shows no history; a transient push failure that +resolves before anyone looks is invisible; and there is **no integration path with +notification-controller** or any Event-driven alerting, because there is no Event to route. +Metrics say a counter moved; they cannot say *which* GitTarget failed to push and why. + +`docs/design/reconcile-triggering.md:222-227` already prescribes the fix ("F4. Conditions **and** +Events, every loop") and cites `fluxcd/pkg/runtime/events`. This ranks above the webhook-receiver +work in the same doc: Events are cheap, and a controller that writes to Git without emitting an +Event on write failure is hard to operate. + +--- + +### F8 — Reason vocabulary is ad-hoc, and in several places `Reason == Type` (Medium) + +`gitprovider_controller.go:325`, `clusterprovider_controller.go:226`, `stream_status.go:45`: + +```go +r.setCondition(gitProvider, ConditionTypeReady, metav1.ConditionTrue, ConditionTypeReady, message) +// ^^ type ^^ reason == "Ready" +const ruleReadyReason = "Ready" +``` + +`Ready=True, reason=Ready` conveys nothing. A reason answers *why*. Flux's generic set +(`external-sources/flux/pkg/apis/meta/conditions.go:80-115`) is `Succeeded`, `Failed`, +`Progressing`, `ProgressingWithRetry`, `Suspended`, `DependencyNotReady`, `InvalidPath`, +`InvalidURL`, `AccessDenied` (the last from `pkg/apis/acl`) — and it is a *shared vocabulary*, so +one alerting rule works across every kind. + +Here it is `OK`, `Ready`, `Checking`, `Resolved`, `Progressing`, `Stalled`, `Validated` scattered +across `constants.go:95-145` and four controllers. `Progressing` and `Stalled` already match Flux. +Suggested: + +- Alias the generic ones to `meta.SucceededReason` / `meta.ProgressingReason` / + `meta.FailedReason` / `meta.DependencyNotReadyReason` — `github.com/fluxcd/pkg/apis/meta` is + already imported (`clusterprovider_types.go:6`), so it is free. +- Replace `OK` and reason-equals-type with `Succeeded`. +- Keep the excellent domain-specific reasons (`UnsupportedContent`, `IgnoreShadowsManagedPath`, + `WriteBoundaryRefused`, `NoAdmittedSourceNamespaces`) — those are exactly what the Flux docs + mean by "declaration of domain common Condition reasons in the API specification is + RECOMMENDED". Consider promoting them from `internal/controller` constants to exported constants + in `api/v1alpha3`, so consumers can compile against them. + +`GitTargetReasonProviderNotFound` should probably be `meta.DependencyNotReadyReason` or at least +`DependencyNotFound`, for the same cross-kind-alerting reason. + +--- + +### F9 — A stored `ClusterWatchRule` with `scope: Namespaced` may be unable to report its own refusal (Medium — needs verification) + +`api/v1alpha3/clusterwatchrule_types.go:130-133` narrows the enum to `Cluster` only, deliberately +keeping the field so a re-apply *fails*. The reasoning in the comment above it is sound. +`DeclaresNamespacedScope()` (`:144`) then refuses a stored value at compile time. + +The concern: for CRDs, the apiserver validates the **whole object** against the OpenAPI schema on +**status-subresource** updates too, not just spec updates. If that holds here, the controller +cannot write `Stalled=True` onto an object whose stored `spec.rules[].scope` is `Namespaced` — the +status update is rejected 422, and the one object that most needs to explain itself is the one +that cannot. + +**Mitigating factor:** CRD Validation Ratcheting (beta and default-on in 1.30, GA in 1.33) skips +re-validation of *unchanged* fields, which would make this a non-issue on modern clusters. So the +exposure is older clusters, or a cluster with the feature gate off. + +Not confirmed by execution. **Worth one envtest:** create a ClusterWatchRule with +`scope: Namespaced` via a client that bypasses the enum (or against an older CRD), then attempt a +status update, on the minimum supported Kubernetes version. If it fails, the fallback is to widen +the enum back and rely solely on the compile-path refusal plus a loud `Stalled` condition — +refusing at admission is nice, but not at the cost of being unable to report the refusal. + +--- + +### F10 — CommitRequest objects accumulate forever, and the controller cannot delete them (Medium) + +`CommitRequest` is a one-shot imperative object with an immutable spec +(`commitrequest_types.go:14`). Every "save now" leaves an object in etcd. There is no TTL, no +`ownerReference`, no GC path — and the reconciler's RBAC is +`commitrequests, verbs=get;list;watch` (`commitrequest_controller.go:108`), so it could not delete +them even if it wanted to. + +A team using this as an interactive save button generates hundreds per namespace per week. +Nothing reaps them. + +The broader Flux-maintainer objection is that **this should be an annotation, not a kind**. +`reconcile./requestedAt` is the established idiom for "act now", it is free to issue, it +self-GCs by being overwritten, and it needs no CRD. The counter-argument here is accepted: a +CommitRequest carries a verbatim commit message, a collect-delay, and (via the admission webhook) +the submitter's identity, and it reports back a SHA. That is genuinely more than a trigger, and +identity capture at admission is the textbook justification made well in +`docs/spec/where-validation-lives.md:50-56`. + +But if it stays a kind, it needs a lifecycle: + +- `spec.ttlSecondsAfterFinished` (the Job precedent) or a controller-side "delete terminal + requests older than N", plus the `delete` verb. +- Or an `ownerReference` to the GitTarget so it is at least cascade-deleted. +- At minimum, document the retention expectation and ship a + `kubectl delete commitrequests --field-selector` recipe. + +--- + +### F11 — `observedGeneration` can record a generation that was never observed (Low) + +`updateStatusWithRetry` re-`Get`s the object and then does `latest.Status = target.Status` +(`gittarget_controller.go:1102`). `target.Status.ObservedGeneration` was set from the generation +read at the *top* of the reconcile (`:128`). If the spec changed in between, the new object is +stamped with an `observedGeneration` equal to a generation never actually processed — kstatus then +reports `Current` for a spec nobody looked at, until the next pass corrects it. + +Narrow window, self-correcting, low severity. Flux avoids it structurally by patching with +optimistic concurrency and setting `observedGeneration` from the object being patched +(`patch.WithStatusObservedGeneration`). Another thing that comes free with the patch helper (F3). + +--- + +### F12 — Small API-conventions nits (Low) + +- **Enum casing — DONE.** `PruneMode` values were `never` / `onEvent` / `always` + (`prune_policy.go:17-23`). Kubernetes API conventions call for UpperCamelCase enum values — + compare `imagePullPolicy: Always|Never|IfNotPresent`, `persistentVolumeReclaimPolicy: + Retain|Delete`. Flux is itself inconsistent here (`driftDetection.mode: enabled|warn|disabled`), + so this was in company, but `Never`/`OnEvent`/`Always` is the conventional spelling. This was the + moment to decide — the field was brand new and unreleased on this branch, and the branch already + carried two `feat(api)!` commits, so the rename rode along in a release that was breaking anyway; + after that release it would have been a second breaking change for a cosmetic gain. + **Resolved on `feat/gittarget-prune-mode-pr5`: the values are now `Never` / `OnEvent` / + `Always`.** The typed constants (`PruneNever`, `PruneOnEvent`, `PruneAlways`) are unchanged, so + no Go call site moved; the wire values, the CRD enum and default, and the docs did. + (Conversely, `OperationType`'s `CREATE`/`UPDATE`/`DELETE` **is** right, because it matches + `admissionregistration.k8s.io` `rules[].operations` verbatim.) +- **Duplicated representation.** `status.streams.summary` is `fmt.Sprintf("%d/%d", Ready, Total)` + (`stream_readiness.go:70-72`) over fields that are right there in the same struct. + `status-conditions-guide.md:37-38` says don't do this. It exists only to feed a printer column — + a legitimate reason, but worth naming as such in the field doc so the next reader doesn't + "clean it up". +- **Printer-column sprawl.** GitTarget declares 13 columns, 7 of them default-priority + (`gittarget_types.go:238-252`). `kubectl get gittargets` will wrap on any normal terminal. Flux + ships 3–4 (Age, Ready, Status). Push `Provider`, `Branch`, `Path` to `priority=1` and keep + `Ready`, `Reason`, `Streams`, `Age`. +- **Reference types.** Six near-identical shapes — `GitProviderReference`, + `ClusterProviderReference`, `LocalTargetReference`, `NamespacedTargetReference`, + `LocalSecretReference`, `KnownHostsReference` — while already depending on + `fluxcd/pkg/apis/meta`, which offers `LocalObjectReference`, `NamespacedObjectReference`, + `NamespacedObjectKindReference`, `SecretKeyReference`. Reusing `meta.KubeConfigReference` + (`clusterprovider_types.go:69`) clearly paid off. The `Group`+`Kind` enum-with-default pattern is + defensible (it documents what is accepted and leaves room to widen), but consider embedding + `meta.LocalObjectReference` for the name half so downstream Go consumers get interoperable types. +- **`metav1.ObjectMeta` json tags are inconsistent** — `omitempty,omitzero` on GitTarget / + GitProvider / ClusterProvider / CommitRequest, plain `omitempty` on WatchRule + (`watchrule_types.go:277`) and ClusterWatchRule (`clusterwatchrule_types.go:203`). +- **`GitProviderStatus.Conditions` lacks `+patchStrategy=merge` / `+patchMergeKey=type`** + (`gitprovider_types.go:143-146`) while the other five kinds have them. Harmless with + `listType=map`, but inconsistent. + +--- + +## 3. The configuration model itself + +Setting status aside — this is the part worth defending as-is, and that deserves saying before the +criticism. + +**What is genuinely strong:** + +- **`docs/spec/where-validation-lives.md` is better than what Flux has written down.** The + schema → CEL → reconciler ladder, and specifically the argument that *reconcile-time is the + stronger gate because admission cannot see a policy tightened after creation*, is correct and is + the thing most projects get backwards. Flux arrived at the same place empirically; here it is a + stated rule. The one webhook shipped is justified on exactly the right grounds (identity exists + only in the `AdmissionRequest`). +- **`NamespaceMatcher`'s absent-vs-declared-vs-empty trichotomy** (`namespace_matcher.go:14-34, + 106-123`). Flux's `acl.AccessFrom` (`external-sources/flux/pkg/apis/acl/`) is a flat + `namespaceSelectors` list with no way to express "declared and empty", and Flux has been bitten + by exactly that ambiguity. The `selector: {}` = everything / `{}` = nothing / absent = legacy + distinction, anchored to `LabelSelectorAsSelector`'s own `Nothing()`/`Everything()` asymmetry, is + more precise than the incumbent. Rejecting `names: ["*"]` because Kubernetes treats it as a + literal name is the kind of detail that only comes from having been burned. +- **The two-key delegation for cross-namespace source watching** — + `ClusterProvider.spec.allowSourceNamespaceOverride` (platform admin) AND + `GitTarget.spec.allowedSourceNamespaces` (destination owner) — is a materially better answer than + Flux's. RFC-0001 (`external-sources/flux/flux2/rfcs/0001-authorization/README.md:97-101`) + documents that Flux controllers simply **do not respect namespace isolation** when dereferencing + cross-namespace refs, a long-standing multi-tenancy sore spot. Deny-by-default and two-party is + the right shape. +- **Immutability where identity is at stake.** `providerRef`/`branch`/`path`/`clusterProviderRef` + immutable via CEL (`gittarget_types.go:45-53`), with the reasoning in the type doc: a folder's + meaning is constituted by those four. `spec.prune` deliberately mutable + (`gittarget_types.go:134-139`) because forcing a delete-and-recreate to re-enable convergence + would destroy the one thing that cannot be rebuilt. Well drawn. +- **The `prune.mode` design.** Separating "the source told me it was deleted" from "a snapshot + didn't mention it" is the correct decomposition, and `onEvent` as the effective default — + resolved in code via `EffectiveMode()` rather than relying on CRD defaulting, so a *stored* + pre-field object is also safe (`prune_policy.go:52-63`) — is careful in the right way. Flux's + `spec.prune` is a single bool and cannot express the middle mode. +- **`no phase string`, anywhere.** Six kinds, zero `.status.phase`. Rarer than it should be. + +**Where to push back on the model:** + +- **The missing pause button** (F6) is the biggest hole. This writes to Git. +- **`GitProvider` is doing three jobs**: remote+credentials (Flux: `GitRepository` + `Secret`), + commit identity/templates/signing (Flux: `ImageUpdateAutomation.spec.git.commit`), and push + batching policy. Defensible cohesion — they are all "how this repo gets written" — but note the + consequence: `spec.push.commitWindow` and `spec.commit.message.*` are properties of a *workload*, + yet they live on the *connection*, so two GitTargets sharing a repo cannot have different + batching or message templates. If that ever needs to differ per target, the field has to move, + and moving it is breaking. Worth writing down as a known constraint now. +- **The `GitProvider` (namespaced) / `ClusterProvider` (cluster) asymmetry** is justified well in + `docs/configuration.md:39-63` and the reasoning holds: a Git destination is a team's write + boundary, a source cluster is a shared physical identity. It *will* still surprise people, and + the doc already anticipates the follow-up ("if a platform later needs a shared, platform-owned + Git destination, that should be a separate cluster-scoped concept"). Keep that paragraph. +- **`clusterProviderRef` defaults to `{name: "default"}` for an object the operator never + creates.** A GitTarget applied to a fresh cluster is unready with `ClusterProviderNotFound` until + someone creates it. Deliberate and well defended (`docs/configuration.md:419-436`; the chart + renders it by default), and the substance is right — silently defaulting to in-cluster + credentials would bypass the authorization model. One ask: make the `ProviderNotFound` message + for the literal name `default` say *"the ClusterProvider named 'default' does not exist; the + operator never creates one — see `clusterProvider.createDefault` in the chart, or commit the + object"*. A generic "provider not found" for the **defaulted** value is the single most likely + first-run support ticket. +- **`ClusterWatchRule` is unbounded by `allowedSourceNamespaces` by construction** — correct + (cluster-scoped objects have no namespace) and clearly documented + (`clusterwatchrule_types.go:172-177`, `docs/configuration.md:935-938`). The mitigation "give each + tenant its own ClusterProvider and credential" is the right answer and matches how Flux tells + people to do hard multi-tenancy. Fine as-is; make sure the security model doc says it in one + place. +- **`sourceNamespace: "*"` fan-out.** One watch stream per (type × admitted namespace) + (`watchrule_types.go:132-134`), a cost flagged honestly in the type doc. On a broad policy across + a big cluster this is the scalability cliff — worth a hard cap with a `Stalled` reason + (`TooManyStreams`) rather than discovering it as apiserver watch pressure. + +--- + +## 4. Suggested order + +**Before the next release** (observable behaviour, cheap): + +1. **F1** — one function owns the trio; every gate contributes a candidate, precedence stated once. + Add a reconcile-output-driven `kstatus.Compute` test covering "refused path + unready provider". +2. **F2** — `Total == 0` is `Ready=True` / `Current`, not perpetual `Reconciling`. +3. **F3** — adopt `fluxcd/pkg/runtime/patch.Helper` (or a `DeepEqual` guard), drop or exclude the + always-moving timestamps, add `For()` predicates to the three controllers missing one. + +**Next** (contract alignment, low risk, high interop value): + +4. **F5** — `apimeta.SetStatusCondition`, or Flux's sorted `Set`. +5. **F8** — alias generic reasons to `fluxcd/pkg/apis/meta`; kill `reason == type`; export the + domain reasons from `api/v1alpha3`. +6. **F4** — delete `Reconciling`/`Stalled` rather than writing them False. +7. **F7** — wire an `EventRecorder`; emit on every terminal outcome and every push failure. + +**Then** (API surface — do the breaking ones while still `v1alpha3`): + +8. **F6** — `spec.suspend` on GitTarget/WatchRule/ClusterWatchRule/GitProvider; `spec.interval` on + GitProvider at minimum; jitter the requeue; `reconcile.configbutler.ai/requestedAt` + + `status.lastHandledReconcileAt`. +9. **F12** — ~~decide `PruneMode` casing **now**~~ (done, pre-release); trim printer columns; unify + ObjectMeta tags. +10. **F10** — CommitRequest lifecycle (TTL or ownerRef) and the `delete` verb. +11. **F9** — verify the `scope: Namespaced` status-write path on the minimum supported Kubernetes + version. + +--- + +## 5. Bottom line + +The question asked was whether the status implementation is *compatible enough* with the open +standard. + +**The model is compatible. The implementation is compatible in three of the four states it can be +in.** The right conditions, the right per-condition `observedGeneration`, the right list semantics, +no phase string, a written contract, and conformance tests against the real kstatus library — more +than most projects have at v1alpha3, and the reason the two real defects are worth fixing rather +than redesigning around. F1 makes a `Failed` object look `InProgress`; F2 makes an idle object +never reach `Current`. Both are localized. Fix those and this passes a kstatus conformance review. + +The larger gap is not kstatus at all — it is the **Flux object contract** (§1, layer 3): `suspend`, +`interval`, reconcile-on-annotation, and Events. That one is not a formal standard and nobody fails +an audit for missing it; it is codified only as an importable Go module, +`github.com/fluxcd/pkg/apis/meta`, which this repo already depends on. But it is what makes an +object *operable* by the reflexes a platform team already has from Flux, and right now none of +those reflexes return anything here. Most of it is already designed in +`docs/design/reconcile-triggering.md`; it needs building. `spec.suspend` first, because this +controller writes to Git and there is currently no way to make it stop. diff --git a/internal/controller/gittarget_controller.go b/internal/controller/gittarget_controller.go index 32c7740c..f8e7c7c5 100644 --- a/internal/controller/gittarget_controller.go +++ b/internal/controller/gittarget_controller.go @@ -219,6 +219,7 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( ctx, gitDest, target.SourceCluster(), + target.EffectivePruneMode(), gitPathWasRefused, ); declareErr != nil { log.V(1).Info("stream declaration skipped; surface not observable", @@ -229,6 +230,10 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( gitPath = r.EventRouter.WatchManager.GitPathAcceptanceForGitTarget(gitDest) renderFidelity = r.EventRouter.WatchManager.RenderFidelityForGitTarget(gitDest) target.Status.Streams = gitTargetStreamsStatus(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( + r.EventRouter.WatchManager.RetentionForGitTarget(gitDest)) streamsSettling = streamsSettling || !streams.StreamsRunning() || !gitPath.Accepted || renderFidelity.State == "Unknown" } else { @@ -1077,6 +1082,28 @@ func gitTargetStreamsStatus(streams watch.StreamSummary) *configbutleraiv1alpha3 } } +// gitTargetRetentionStatus projects the data-plane retention roll-up. +// +// A summary that has never been reported projects to NIL rather than to a zero count, and the +// distinction is load-bearing: absent means "no resync has reported yet" (the target has not +// replayed, or predates the field), while zero means "a resync ran and found nothing to retain" — +// the converged signal. Collapsing them would make status unable to say a mirror is converged, +// which is half the reason the field exists. +func gitTargetRetentionStatus(summary watch.RetentionSummary) *configbutleraiv1alpha3.GitTargetRetentionStatus { + if !summary.Reported { + return nil + } + observed := metav1.NewTime(summary.ObservedTime) + if summary.ObservedTime.IsZero() { + observed = metav1.Now() + } + return &configbutleraiv1alpha3.GitTargetRetentionStatus{ + Mode: summary.Mode, + RetainedDocuments: clampIntToInt32(summary.RetainedDocuments), + ObservedTime: &observed, + } +} + // updateStatusWithRetry updates the status with retry logic to handle race conditions. func (r *GitTargetReconciler) updateStatusWithRetry( ctx context.Context, diff --git a/internal/controller/gittarget_source_cluster_test.go b/internal/controller/gittarget_source_cluster_test.go index 512d29ae..aa6ec1c6 100644 --- a/internal/controller/gittarget_source_cluster_test.go +++ b/internal/controller/gittarget_source_cluster_test.go @@ -383,7 +383,8 @@ func TestReconcile_UnauthorizedNamespaceStartsNoWatch(t *testing.T) { // before it opens any watch, so it records even though opening watches fails here (no discovery // client is wired) — which is exactly the capture a refused GitTarget must not produce. other := types.NewResourceReference("authorized", ns).WithUID("other-uid") - _ = watchManager.DeclareForGitTarget(context.Background(), other, providerName) + _ = watchManager.DeclareForGitTarget( + context.Background(), other, providerName, configbutleraiv1alpha3.PruneOnEvent) id, declaredOther := watchManager.DeclaredSourceCluster(other) require.True(t, declaredOther, "the positive control must declare, or the assertion above proves nothing") assert.Equal(t, providerName, id) diff --git a/internal/controller/gittarget_status_test.go b/internal/controller/gittarget_status_test.go index 589eedc3..a168e91b 100644 --- a/internal/controller/gittarget_status_test.go +++ b/internal/controller/gittarget_status_test.go @@ -4,6 +4,7 @@ package controller import ( "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -170,3 +171,36 @@ func TestDeriveGitTargetDataPlaneStatusWithRenderFidelity(t *testing.T) { assert.Equal(t, metav1.ConditionTrue, diverged.StalledStatus) assert.Equal(t, GitTargetReasonRenderDoesNotMatchLive, diverged.StalledReason) } + +// TestGitTargetRetentionStatus_AbsentAndZeroMeanDifferentThings is the whole point of the field +// being a pointer. Absent is "no resync has reported yet — the target has not replayed, or predates +// this field"; zero is "a resync ran and found nothing to retain", which is the converged signal. +// Collapsing them would leave status unable to say a mirror is converged. +func TestGitTargetRetentionStatus_AbsentAndZeroMeanDifferentThings(t *testing.T) { + assert.Nil(t, gitTargetRetentionStatus(watch.RetentionSummary{}), + "a target that has never reported must not publish a count of zero") + + converged := gitTargetRetentionStatus(watch.RetentionSummary{ + Reported: true, Mode: configbutleraiv1alpha3.PruneAlways, + }) + require.NotNil(t, converged, "a reported zero is a report") + assert.Zero(t, converged.RetainedDocuments) + assert.Equal(t, configbutleraiv1alpha3.PruneAlways, converged.Mode) + require.NotNil(t, converged.ObservedTime, "a reading with no timestamp cannot be judged stale") + assert.False(t, converged.ObservedTime.IsZero()) +} + +// TestGitTargetRetentionStatus_ReportsTheEffectiveMode covers the legacy GitTarget: it stores no +// spec.prune at all, so status is the only place the mode keeping its documents is visible. +func TestGitTargetRetentionStatus_ReportsTheEffectiveMode(t *testing.T) { + observed := time.Date(2026, 7, 21, 13, 20, 0, 0, time.UTC) + + projected := gitTargetRetentionStatus(watch.RetentionSummary{ + Reported: true, Mode: configbutleraiv1alpha3.PruneOnEvent, RetainedDocuments: 3, ObservedTime: observed, + }) + + require.NotNil(t, projected) + assert.Equal(t, int32(3), projected.RetainedDocuments) + assert.Equal(t, configbutleraiv1alpha3.PruneOnEvent, projected.Mode) + assert.Equal(t, observed, projected.ObservedTime.Time) +} diff --git a/internal/git/acceptance_gate_test.go b/internal/git/acceptance_gate_test.go index 0b9d895a..3ed6911e 100644 --- a/internal/git/acceptance_gate_test.go +++ b/internal/git/acceptance_gate_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" "github.com/ConfigButler/gitops-reverser/internal/types" ) @@ -42,7 +43,7 @@ func TestPlanFlush_RefusesUnsupportedKustomizeFolder(t *testing.T) { w := &BranchWorker{contentWriter: writer} event := cmEvent("CREATE", "fresh", "green") - _, err := w.flushEventsToWorktree(context.Background(), worktree, "", []Event{event}, nil) + _, err := w.flushEventsToWorktree(context.Background(), worktree, "", []Event{event}, nil, v1alpha3.PruneOnEvent) var refused *manifestanalyzer.AcceptanceRefusedError require.ErrorAs(t, err, &refused, "flush must refuse with *AcceptanceRefusedError") @@ -67,7 +68,7 @@ func TestPlanFlush_AcceptsPlainKustomizeFolder(t *testing.T) { w := &BranchWorker{contentWriter: writer} create := []Event{cmEvent("CREATE", "fresh", "green")} - changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", create, nil) + changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", create, nil, v1alpha3.PruneOnEvent) require.NoError(t, err, "a plain kustomization must not be refused") assert.True(t, changed, "the ConfigMap must be written beside the retained kustomization") } @@ -82,7 +83,7 @@ func TestPlanFlush_DoesNotRefuseOwnSopsConfig(t *testing.T) { w := &BranchWorker{contentWriter: writer} create := []Event{cmEvent("CREATE", "fresh", "green")} - changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", create, nil) + changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", create, nil, v1alpha3.PruneOnEvent) require.NoError(t, err, ".sops.yaml is the operator's own config and must not be refused") assert.True(t, changed, "the ConfigMap must still be written beside .sops.yaml") } diff --git a/internal/git/branch_worker.go b/internal/git/branch_worker.go index 326bbe54..eb97f2de 100644 --- a/internal/git/branch_worker.go +++ b/internal/git/branch_worker.go @@ -20,6 +20,7 @@ import ( "github.com/go-logr/logr" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" @@ -140,6 +141,12 @@ type BranchWorker struct { // firsts surfaces the first successful commit and push at default verbosity. firsts branchWorkerLogFirsts + // retentionLoggedAt throttles the default-verbosity "prune policy retained documents" + // line to one per GitTarget subtree per retentionLogInterval. Retaining is a steady + // state, so the log must not scale with the reconcile rate. Owned by the event loop + // goroutine, like firsts, so it carries no lock. + retentionLoggedAt map[string]time.Time + // hasUnpushedWork mirrors whether the event loop is currently holding a live // open window or any committed-but-not-yet-pushed pending writes. The event // loop is the only writer and, via syncQueueDepthMetric, the only reader; the @@ -1241,6 +1248,13 @@ func (w *BranchWorker) rebuildPendingWrites( return "", plumbing.ZeroHash, err } + // A replay re-PLANS against the rebased worktree, so it can decide deletions the first apply + // never made — under whatever policy was in force when the write was planned. Re-check the + // policy before that happens, so an operator who tightened it in the meantime is obeyed. + if err := w.tightenPendingPruneModes(w.ctx, pendingWrites); err != nil { + return "", plumbing.ZeroHash, err + } + if _, err := w.executePendingWrites(w.ctx, repo, pendingWrites); err != nil { return "", plumbing.ZeroHash, fmt.Errorf("execute replay pending writes: %w", err) } @@ -1248,6 +1262,55 @@ func (w *BranchWorker) rebuildPendingWrites( return baseBranch, baseHash, nil } +// tightenPendingPruneModes lowers every retained write's captured prune mode to the more +// restrictive of (captured, current) before the write is replayed. Tightening only: see +// PruneMode.MoreRestrictiveOf for why the loosening direction must NOT propagate here. +// +// It mutates the Targets map in place, which is the point — the map is shared with the retained +// PendingWrite, so one pass covers both deletion paths (the resync sweep reads it through +// PendingWrite.Target, the steady-state DELETE writer through pruneModeForBase) and the tightening +// survives every subsequent push attempt. +// +// The two failure modes are answered differently on purpose: +// +// - the GitTarget is GONE — a definite answer, and no policy exists to authorize anything, so +// the write replays under the most restrictive mode. Retrying could not produce a better one. +// - the read FAILED — no answer. Returning the error leaves the pending writes retained and the +// push cycle retries them, so neither a legitimate delete is dropped nor an unauthorized one +// applied. Guessing either way here would do one or the other. +func (w *BranchWorker) tightenPendingPruneModes(ctx context.Context, pendingWrites []PendingWrite) error { + current := map[pendingTargetKey]configv1alpha3.PruneMode{} + // Ranged by value on purpose: Targets is a map, so writing through this copy still updates the + // retained write's own map — which is exactly the sharing this relies on. + for _, pendingWrite := range pendingWrites { + for key, md := range pendingWrite.Targets { + mode, cached := current[key] + if !cached { + target, err := w.getGitTarget(ctx, key.Name, key.Namespace) + switch { + case apierrors.IsNotFound(err): + mode = configv1alpha3.PruneNever + case err != nil: + return fmt.Errorf("re-read prune policy for %s/%s before replay: %w", key.Namespace, key.Name, err) + default: + mode = target.EffectivePruneMode() + } + current[key] = mode + } + tightened := md.PruneMode.MoreRestrictiveOf(mode) + if tightened == md.PruneMode.OrDefault() { + continue + } + w.Log.Info("prune policy tightened since this write was planned; replaying under the stricter mode", + "gitTarget", key.Namespace+"/"+key.Name, + "planned", string(md.PruneMode.OrDefault()), "replaying", string(tightened)) + md.PruneMode = tightened + pendingWrite.Targets[key] = md + } + } + return nil +} + // estimateEventSize approximates the serialized YAML size for an event's object. func (w *BranchWorker) estimateEventSize(ev Event) int64 { if ev.Object == nil { diff --git a/internal/git/branch_worker_split_test.go b/internal/git/branch_worker_split_test.go index e8c1c441..9078be99 100644 --- a/internal/git/branch_worker_split_test.go +++ b/internal/git/branch_worker_split_test.go @@ -67,8 +67,27 @@ func configMapTargetEvent(name, username, target string) Event { return event } +// createPlainGitTarget creates a GitTarget that declares NO prune policy — the legacy shape, and +// the one an existing cluster holds after an upgrade. Its effective mode is therefore onEvent, so +// a resync through it must not sweep. Tests that want convergence say so with +// createGitTargetWithPruneMode. func createPlainGitTarget(t *testing.T, worker *BranchWorker, name, path string) { t.Helper() + createGitTarget(t, worker, name, path, nil) +} + +// createGitTargetWithPruneMode creates a GitTarget declaring an explicit spec.prune.mode. +func createGitTargetWithPruneMode( + t *testing.T, worker *BranchWorker, name, path string, mode configv1alpha3.PruneMode, +) { + t.Helper() + createGitTarget(t, worker, name, path, &configv1alpha3.PrunePolicy{Mode: mode}) +} + +func createGitTarget( + t *testing.T, worker *BranchWorker, name, path string, prune *configv1alpha3.PrunePolicy, +) { + t.Helper() require.NoError(t, worker.Client.Create(worker.ctx, &configv1alpha3.GitTarget{ ObjectMeta: metav1.ObjectMeta{ @@ -81,6 +100,7 @@ func createPlainGitTarget(t *testing.T, worker *BranchWorker, name, path string) }, Branch: worker.Branch, Path: path, + Prune: prune, }, })) } @@ -1042,7 +1062,7 @@ func TestEventLoop_AtomicPushFailure_DoesNotAdvanceCooldownOrLosePendingWrite(t func TestResync_WorkerAppliesMarkAndSweepAndCommits(t *testing.T) { worker, serverRepo, _ := setupCommitPushSplitWorker(t) worker.mapper = configMapMapper() - createPlainGitTarget(t, worker, "target-a", "live") + createGitTargetWithPruneMode(t, worker, "target-a", "live", configv1alpha3.PruneAlways) initialRef, err := serverRepo.Reference(plumbing.NewBranchReferenceName("main"), true) require.NoError(t, err) @@ -1120,7 +1140,7 @@ func TestResync_WorkerNoopDoesNotRetainOrPush(t *testing.T) { func TestResync_WorkerEmptyDesiredSweepsManagedResource(t *testing.T) { worker, _, _ := setupCommitPushSplitWorker(t) worker.mapper = configMapMapper() - createPlainGitTarget(t, worker, "target-a", "live") + createGitTargetWithPruneMode(t, worker, "target-a", "live", configv1alpha3.PruneAlways) loop := newBranchWorkerEventLoop(worker, 0) loop.lastPushAt = time.Now() diff --git a/internal/git/commit_executor.go b/internal/git/commit_executor.go index 4af0f2fb..ba8eb9a7 100644 --- a/internal/git/commit_executor.go +++ b/internal/git/commit_executor.go @@ -185,6 +185,7 @@ func (w *BranchWorker) applyPendingWriteEvents( base, byBase[base], placementPolicyForBase(targets, base), + pruneModeForBase(targets, base), ) if err != nil { return false, err diff --git a/internal/git/fieldpatch_flush_test.go b/internal/git/fieldpatch_flush_test.go index 6508d827..7183b572 100644 --- a/internal/git/fieldpatch_flush_test.go +++ b/internal/git/fieldpatch_flush_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/runtime/schema" + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" "github.com/ConfigButler/gitops-reverser/internal/types" "github.com/ConfigButler/gitops-reverser/internal/typeset" @@ -54,7 +55,7 @@ func deploymentsMapper() typeset.Lookup { func applyScalePatch(t *testing.T, writer *contentWriter, worktree *gogit.Worktree, events ...Event) bool { t.Helper() w := &BranchWorker{contentWriter: writer, mapper: deploymentsMapper()} - changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", events, nil) + changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", events, nil, v1alpha3.PruneOnEvent) require.NoError(t, err) return changed } diff --git a/internal/git/inplace_edit_test.go b/internal/git/inplace_edit_test.go index dddfb1c1..3a693553 100644 --- a/internal/git/inplace_edit_test.go +++ b/internal/git/inplace_edit_test.go @@ -14,6 +14,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/types" "github.com/ConfigButler/gitops-reverser/internal/typeset" ) @@ -52,7 +53,7 @@ func newWorktreeForTest(t *testing.T) *gogit.Worktree { func applyEventsViaPlanFlush(t *testing.T, writer *contentWriter, worktree *gogit.Worktree, events ...Event) bool { t.Helper() w := &BranchWorker{contentWriter: writer} - changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", events, nil) + changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", events, nil, v1alpha3.PruneOnEvent) require.NoError(t, err) return changed } @@ -66,7 +67,7 @@ func applyEventsViaPlanFlushWithMapper( ) bool { t.Helper() w := &BranchWorker{contentWriter: writer, mapper: mapper} - changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", events, nil) + changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", events, nil, v1alpha3.PruneOnEvent) require.NoError(t, err) return changed } diff --git a/internal/git/kustomize_oracle_test.go b/internal/git/kustomize_oracle_test.go index 00e15183..94d8a720 100644 --- a/internal/git/kustomize_oracle_test.go +++ b/internal/git/kustomize_oracle_test.go @@ -15,6 +15,7 @@ import ( "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" "github.com/ConfigButler/gitops-reverser/internal/types" "github.com/ConfigButler/gitops-reverser/internal/typeset" @@ -32,7 +33,7 @@ func flushEventsForTest( ) (bool, error) { t.Helper() w := &BranchWorker{contentWriter: writer, mapper: mapper} - return w.flushEventsToWorktree(context.Background(), worktree, "", events, nil) + return w.flushEventsToWorktree(context.Background(), worktree, "", events, nil, v1alpha3.PruneOnEvent) } // Before a kustomize-governed write is committed, the repository is re-rendered WITH it diff --git a/internal/git/pending_writes.go b/internal/git/pending_writes.go index f607e2bf..e4a484e7 100644 --- a/internal/git/pending_writes.go +++ b/internal/git/pending_writes.go @@ -168,10 +168,29 @@ func (w *BranchWorker) resolveTargetMetadata( BootstrapOptions: buildBootstrapOptions(encryptionConfig), EncryptionConfig: encryptionConfig, Placement: resolvePlacementPolicy(target.Spec.Placement), + PruneMode: target.EffectivePruneMode(), SourceCluster: target.SourceCluster(), }, nil } +// pruneModeForBase finds the effective prune mode for the GitTarget that owns base among +// targets, matching exactly as placementPolicyForBase does (see its comment for why both +// sides must be sanitized, and why at most one target can match). +// +// A base with no matching target — an event whose target metadata could not be resolved — +// falls back to the omitted-field default rather than to the zero value of the type. The +// zero value is the empty string, which is not a mode at all: it would report false for +// both deletion paths and silently upgrade an unresolvable target to `never`. onEvent is +// what an unset policy means everywhere else, so it is what it means here too. +func pruneModeForBase(targets map[pendingTargetKey]ResolvedTargetMetadata, base string) v1alpha3.PruneMode { + for _, md := range targets { + if sanitizePath(md.Path) == base { + return md.PruneMode.OrDefault() + } + } + return v1alpha3.PruneOnEvent +} + // resolvePlacementPolicy converts the CRD's declared placement spec into the // package-local shape manifestanalyzer.LocateNew consumes. Kept as a plain field- // for-field copy (not a shared type) so manifestanalyzer stays free of any diff --git a/internal/git/placement_test.go b/internal/git/placement_test.go index 1935744e..1de8dbf8 100644 --- a/internal/git/placement_test.go +++ b/internal/git/placement_test.go @@ -14,6 +14,7 @@ import ( "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" "github.com/ConfigButler/gitops-reverser/internal/types" ) @@ -41,7 +42,7 @@ func applyEventsWithPolicy( ) bool { t.Helper() w := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} - changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", events, policy) + changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", events, policy, v1alpha3.PruneOnEvent) require.NoError(t, err) return changed } @@ -143,7 +144,14 @@ func TestPlacement_SensitiveCollision_SkipsWithoutCrashing(t *testing.T) { Operation: "CREATE", } w := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{})} - changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", []Event{event}, policy) + changed, err := w.flushEventsToWorktree( + context.Background(), + worktree, + "", + []Event{event}, + policy, + v1alpha3.PruneOnEvent, + ) require.NoError(t, err, "a placement conflict must be skipped, not returned as a batch error") assert.False(t, changed, "no file should be written when placement cannot be resolved safely") @@ -236,7 +244,7 @@ func TestPlacement_UndecodableKustomization_RefusesTheFlush(t *testing.T) { w := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} _, err := w.flushEventsToWorktree( - context.Background(), worktree, "", []Event{newConfigMapEvent("cache", "app")}, nil, + context.Background(), worktree, "", []Event{newConfigMapEvent("cache", "app")}, nil, v1alpha3.PruneOnEvent, ) require.Error(t, err, "a kustomization kustomize cannot build must refuse the folder, not be written into") } @@ -265,7 +273,7 @@ func TestPlacement_ExternalBaseOverlay_NewObject(t *testing.T) { w := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} changed, err := w.flushEventsToWorktree( context.Background(), worktree, "overlays/test", - []Event{newConfigMapEvent("cache", "podinfo-test")}, nil, + []Event{newConfigMapEvent("cache", "podinfo-test")}, nil, v1alpha3.PruneOnEvent, ) require.NoError(t, err, "the overlay new-object flush must pass the render oracle") require.True(t, changed) @@ -329,7 +337,14 @@ func liveDeployment(image string, replicas int64) Event { func flushOverlayDeployment(t *testing.T, worktree *gogit.Worktree, event Event) error { t.Helper() w := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: deploymentMapper()} - _, err := w.flushEventsToWorktree(context.Background(), worktree, "overlays/test", []Event{event}, nil) + _, err := w.flushEventsToWorktree( + context.Background(), + worktree, + "overlays/test", + []Event{event}, + nil, + v1alpha3.PruneOnEvent, + ) return err } @@ -411,7 +426,14 @@ func TestOverlayAuthors_DeletePatch_ForInheritedObject(t *testing.T) { Operation: "DELETE", } w := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} - _, err := w.flushEventsToWorktree(context.Background(), worktree, "overlays/test", []Event{del}, nil) + _, err := w.flushEventsToWorktree( + context.Background(), + worktree, + "overlays/test", + []Event{del}, + nil, + v1alpha3.PruneOnEvent, + ) require.NoError(t, err, "deleting an inherited object must author a $patch: delete, not refuse") patch, err := os.ReadFile(filepath.Join(root, "overlays/test/configmap-shared-delete.yaml")) @@ -452,7 +474,14 @@ func TestOverlayAuthors_DeletePatch_SkipsOnPathCollision(t *testing.T) { Operation: "DELETE", } w := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} - _, err := w.flushEventsToWorktree(context.Background(), worktree, "overlays/test", []Event{del}, nil) + _, err := w.flushEventsToWorktree( + context.Background(), + worktree, + "overlays/test", + []Event{del}, + nil, + v1alpha3.PruneOnEvent, + ) require.NoError(t, err, "a patch-path collision must be skipped, not error") got, err := os.ReadFile(filepath.Join(root, "overlays/test/configmap-shared-delete.yaml")) @@ -592,6 +621,7 @@ func TestPlacement_ColdBundleCollision_SensitiveNeverMerged(t *testing.T) { changed, err := w.flushEventsToWorktree( context.Background(), worktree, "", []Event{newSecretEvent("first"), newSecretEvent("second")}, policy, + v1alpha3.PruneOnEvent, ) require.NoError(t, err) @@ -635,7 +665,7 @@ func TestPlacement_ColdBundleCollision_SensitiveAndPlaintextNeverMix(t *testing. secretFirst := newWorktreeForTest(t) wsf := &BranchWorker{contentWriter: newWriter()} _, err := wsf.flushEventsToWorktree( - context.Background(), secretFirst, "", []Event{secretEvent, configMapEvent}, policy, + context.Background(), secretFirst, "", []Event{secretEvent, configMapEvent}, policy, v1alpha3.PruneOnEvent, ) require.NoError(t, err) secretFirstBody, readErr := os.ReadFile(filepath.Join(secretFirst.Filesystem.Root(), "all.yaml")) @@ -648,7 +678,7 @@ func TestPlacement_ColdBundleCollision_SensitiveAndPlaintextNeverMix(t *testing. configMapFirst := newWorktreeForTest(t) wcf := &BranchWorker{contentWriter: newWriter()} _, err = wcf.flushEventsToWorktree( - context.Background(), configMapFirst, "", []Event{configMapEvent, secretEvent}, policy, + context.Background(), configMapFirst, "", []Event{configMapEvent, secretEvent}, policy, v1alpha3.PruneOnEvent, ) require.NoError(t, err) configMapFirstBody, readErr := os.ReadFile(filepath.Join(configMapFirst.Filesystem.Root(), "all.yaml")) @@ -682,7 +712,14 @@ func TestPlacement_ColdBundleCollision_ViaResync(t *testing.T) { } w := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} - _, changed, err := w.applyResyncToWorktree(context.Background(), worktree, "", "", desired, nil, policy) + _, changed, err := w.applyResyncToWorktree( + context.Background(), + worktree, + "", + ResolvedTargetMetadata{Placement: policy, PruneMode: v1alpha3.PruneAlways}, + desired, + nil, + ) require.NoError(t, err) assert.True(t, changed) diff --git a/internal/git/plan_flush.go b/internal/git/plan_flush.go index 04c9818f..2bf53413 100644 --- a/internal/git/plan_flush.go +++ b/internal/git/plan_flush.go @@ -19,6 +19,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" sigsyaml "sigs.k8s.io/yaml" + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" "github.com/ConfigButler/gitops-reverser/internal/manifestreport" @@ -70,6 +71,7 @@ func (w *BranchWorker) flushEventsToWorktree( base string, events []Event, policy *manifestanalyzer.PlacementPolicy, + pruneMode v1alpha3.PruneMode, ) (bool, error) { root := worktree.Filesystem.Root() scoped, err := scanRenderScope(root, base) @@ -81,6 +83,7 @@ func (w *BranchWorker) flushEventsToWorktree( // one source cluster; resolve this subtree's GVK->GVR against that cluster's registry. mapper := w.mapperForCluster(clusterIDForEvents(events)) batch := newWriteBatch(ctx, w.contentWriter, mapper, scoped.scan, policy, scoped.writeSubdir) + batch.pruneMode = pruneMode if err := batch.refusal(); err != nil { return false, err } @@ -121,6 +124,16 @@ type writeBatch struct { // only for a resource with no existing document. nil means no declared policy — // placement falls through to sibling inference and then the canonical path. policy *manifestanalyzer.PlacementPolicy + // pruneMode is the GitTarget's effective spec.prune.mode, gating the EXPLICIT delete + // path only (applyDelete). The inferred mark-and-sweep is gated a layer up, in the + // planner, so a suppressed drop never becomes an action in the first place. + // + // Set only on the live-event batch, because that is the only batch that folds DELETE + // events; the resync batch drops documents through the plan instead and leaves this + // zero. It is therefore always read through OrDefault: the zero value is unset, not + // `never`, and reading it literally would make a batch that simply never set it stop + // mirroring deletes. + pruneMode v1alpha3.PruneMode // writeSubdir is spec.path expressed relative to the render anchor (renderBase) — the // write jail. It is "" for a self-contained subtree (renderBase == spec.path), where // every scanned path is writable; it is non-empty only when the scan reached past @@ -1137,7 +1150,17 @@ func (wb *writeBatch) writeWholeFile(ctx context.Context, event Event, rel strin // the same batch that shifted a multi-document file does not misdirect this one. // Removing the last document in a file marks it for deletion; otherwise the surviving // documents are kept byte-for-byte. +// +// The target's spec.prune.mode gates this whole path: under `never` the managed document is +// left exactly as Git holds it. The check is FIRST, before the document is even located, so a +// suppressed delete touches no buffer, records no write intent, and cannot turn the kustomize +// oracle on — a retention must be indistinguishable from the event never having arrived. func (wb *writeBatch) applyDelete(ctx context.Context, event Event) { + if !wb.pruneMode.OrDefault().AppliesEventDeletes() { + log.FromContext(ctx).V(1).Info("source DELETE not mirrored (spec.prune.mode)", + "pruneMode", string(wb.pruneMode.OrDefault()), "resource", event.Identifier.Key()) + return + } target, found := wb.resolveDelete(event) if !found { return diff --git a/internal/git/plan_flush_test.go b/internal/git/plan_flush_test.go index 78626411..1671d11d 100644 --- a/internal/git/plan_flush_test.go +++ b/internal/git/plan_flush_test.go @@ -13,6 +13,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/types" "github.com/ConfigButler/gitops-reverser/internal/typeset" ) @@ -119,7 +120,14 @@ func TestPlanFlush_DeleteByGVROnlyFollowsMovedManifestViaMapper(t *testing.T) { }, Operation: "DELETE", } - changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", []Event{del}, nil) + changed, err := w.flushEventsToWorktree( + context.Background(), + worktree, + "", + []Event{del}, + nil, + v1alpha3.PruneOnEvent, + ) require.NoError(t, err) assert.True(t, changed, "the moved manifest must be deleted via the resolved resource identity") _, statErr := os.Stat(placedFull) diff --git a/internal/git/prune_mode_test.go b/internal/git/prune_mode_test.go new file mode 100644 index 00000000..26575f4f --- /dev/null +++ b/internal/git/prune_mode_test.go @@ -0,0 +1,232 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "context" + "io/fs" + "os" + "path/filepath" + "strings" + "testing" + + gogit "github.com/go-git/go-git/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// The two deletion paths are exercised separately here because spec.prune.mode is the only place +// in the product where they are controlled independently. `onEvent` — the effective default — +// differs from `always` on exactly one of them, so a test that only covered "does a delete +// happen" could not tell the two modes apart at all. + +// resyncUnder folds desired over the worktree under one prune mode and returns the stats. +func resyncUnder( + t *testing.T, + worktree *gogit.Worktree, + mode v1alpha3.PruneMode, + desired ...manifestanalyzer.DesiredResource, +) ResyncStats { + t.Helper() + w := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} + stats, _, err := w.applyResyncToWorktree( + context.Background(), worktree, "", ResolvedTargetMetadata{PruneMode: mode}, desired, nil) + require.NoError(t, err) + return stats +} + +// deleteEventFor builds the object-less DELETE event the steady-state writer receives when a +// watched resource is removed from the source cluster. +func deleteEventFor(name string) Event { + return Event{ + Operation: "DELETE", + Identifier: types.ResourceIdentifier{ + Group: "", Version: "v1", Resource: "configmaps", Namespace: "default", Name: name, + }, + } +} + +// deleteUnder folds one explicit DELETE event through the steady-state writer under one prune +// mode and reports whether anything changed. +func deleteUnder(t *testing.T, worktree *gogit.Worktree, mode v1alpha3.PruneMode, name string) bool { + t.Helper() + w := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} + changed, err := w.flushEventsToWorktree( + context.Background(), worktree, "", []Event{deleteEventFor(name)}, nil, mode) + require.NoError(t, err) + return changed +} + +// TestPrune_OnEventRetainsWhenTheDesiredSetNarrowsToEmpty is the safety property PR 5 exists for. +// +// An empty desired set is exactly what a scope collapse, a source-cluster outage, or an older +// controller that does not understand a newer scope field produces. Under the default the mirror +// keeps its documents: nothing is deleted, nothing is counted as deleted, and the file is still +// on disk byte-for-byte. +func TestPrune_OnEventRetainsWhenTheDesiredSetNarrowsToEmpty(t *testing.T) { + worktree := newWorktreeForTest(t) + seeded := cmManifest("orphan", "blue") + full := seedPlacedManifest(t, worktree, "apps/orphan.yaml", seeded) + + stats := resyncUnder(t, worktree, v1alpha3.PruneOnEvent) + + assert.Zero(t, stats.Deleted, "onEvent must never infer a deletion from a desired snapshot") + got, err := os.ReadFile(full) + require.NoError(t, err, "the retained document must still exist") + assert.Equal(t, seeded, string(got), "a retained document must be untouched, not rewritten") +} + +// TestPrune_OnEventStillMirrorsAnExplicitDelete is the other half of the default: retention is +// about INFERENCE, not about deletion. Source-cluster evidence still removes the document, which +// is what keeps `onEvent` a usable default rather than a slow-growing archive. +func TestPrune_OnEventStillMirrorsAnExplicitDelete(t *testing.T) { + worktree := newWorktreeForTest(t) + full := seedPlacedManifest(t, worktree, "apps/app.yaml", cmManifest("app", "blue")) + + assert.True(t, deleteUnder(t, worktree, v1alpha3.PruneOnEvent, "app"), + "an observed source DELETE must still be mirrored under onEvent") + _, statErr := os.Stat(full) + assert.True(t, os.IsNotExist(statErr), "the document must be removed from Git") +} + +// TestPrune_NeverSuppressesBothPaths pins the archive mode. `never` is the only mode under which +// an explicit DELETE leaves the document in place, so this is what distinguishes it from the +// default — a test that only checked the sweep would pass for both. +func TestPrune_NeverSuppressesBothPaths(t *testing.T) { + t.Run("explicit delete", func(t *testing.T) { + worktree := newWorktreeForTest(t) + seeded := cmManifest("app", "blue") + full := seedPlacedManifest(t, worktree, "apps/app.yaml", seeded) + + assert.False(t, deleteUnder(t, worktree, v1alpha3.PruneNever, "app"), + "never must not mirror a source DELETE, so the flush changes nothing") + got, err := os.ReadFile(full) + require.NoError(t, err) + assert.Equal(t, seeded, string(got), "an archived document keeps its bytes") + }) + + t.Run("resync sweep", func(t *testing.T) { + worktree := newWorktreeForTest(t) + full := seedPlacedManifest(t, worktree, "apps/orphan.yaml", cmManifest("orphan", "blue")) + + stats := resyncUnder(t, worktree, v1alpha3.PruneNever) + + assert.Zero(t, stats.Deleted) + _, statErr := os.Stat(full) + assert.NoError(t, statErr, "never must not sweep either") + }) +} + +// TestPrune_AlwaysReproducesMarkAndSweep proves `always` is the opt-in back to the pre-PR-5 +// behaviour: the orphan is swept, and the resource the snapshot DOES name is still upserted in +// the same pass. Sweeping without upserting would be a different, much worse bug. +func TestPrune_AlwaysReproducesMarkAndSweep(t *testing.T) { + worktree := newWorktreeForTest(t) + root := worktree.Filesystem.Root() + orphan := seedPlacedManifest(t, worktree, "apps/orphan.yaml", cmManifest("orphan", "blue")) + + stats := resyncUnder(t, worktree, v1alpha3.PruneAlways, desiredCM("keep", "green")) + + assert.Equal(t, 1, stats.Deleted, "always restores the mark-and-sweep") + assert.Equal(t, 1, stats.Created, "the desired resource is still created in the same pass") + _, statErr := os.Stat(orphan) + assert.True(t, os.IsNotExist(statErr), "the orphan is removed from Git") + // Asserted by content rather than by path: where a new document lands is placement's + // decision (here, sibling inference off the orphan's own directory), and this test is about + // the sweep not swallowing the upsert — not about where the upsert went. + assert.True(t, worktreeHoldsDocumentNamed(t, root, "keep"), + "the resource present in the cluster is mirrored") +} + +// worktreeHoldsDocumentNamed reports whether any YAML file under root names a metadata.name. +func worktreeHoldsDocumentNamed(t *testing.T, root, name string) bool { + t.Helper() + found := false + require.NoError(t, filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() || filepath.Ext(path) != ".yaml" { + return err + } + body, readErr := os.ReadFile(path) + if readErr != nil { + return readErr + } + if strings.Contains(string(body), "name: "+name) { + found = true + } + return nil + })) + return found +} + +// TestPrune_RetentionIsIdenticalUnderEveryResyncShape closes the "no alternate sweep path" gap. +// A whole-target resync (nil scope, BuildPlan) and a namespace-scoped per-type resync (non-nil +// scope, BuildScopedPlan) reach the planner through two different functions; gating only one +// would leave a live deletion path uncontrolled. Production only ever issues the scoped shape, +// which is precisely why the unscoped one is the easy one to forget. +func TestPrune_RetentionIsIdenticalUnderEveryResyncShape(t *testing.T) { + scope := &ResyncScope{GVR: configmapsGVRForScope, Namespace: "default"} + + for _, tc := range []struct { + name string + scope *ResyncScope + }{ + {"whole target", nil}, + {"namespace-scoped per-type", scope}, + } { + t.Run(tc.name, func(t *testing.T) { + worktree := newWorktreeForTest(t) + full := seedPlacedManifest(t, worktree, "apps/orphan.yaml", cmManifest("orphan", "blue")) + w := &BranchWorker{ + contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), + mapper: configMapMapper(), + } + + stats, _, err := w.applyResyncToWorktree( + context.Background(), worktree, "", + ResolvedTargetMetadata{PruneMode: v1alpha3.PruneOnEvent}, nil, tc.scope) + require.NoError(t, err) + + assert.Zero(t, stats.Deleted) + _, statErr := os.Stat(full) + assert.NoError(t, statErr, "no resync shape may bypass the prune policy") + }) + } +} + +// TestPruneModeForBase_UnresolvableTargetFallsBackToOnEvent guards the lookup's failure mode. A +// base with no matching target (an event whose GitTarget metadata could not be resolved) must not +// pick up the zero value of the type: "" answers false to both predicates, so it would silently +// promote the target to `never` and stop mirroring deletes. +func TestPruneModeForBase_UnresolvableTargetFallsBackToOnEvent(t *testing.T) { + targets := map[pendingTargetKey]ResolvedTargetMetadata{ + {Name: "known", Namespace: "default"}: {Path: "live", PruneMode: v1alpha3.PruneAlways}, + // Written by a path that predates the field, or by a struct literal in a test. + {Name: "unset", Namespace: "default"}: {Path: "legacy"}, + } + + assert.Equal(t, v1alpha3.PruneAlways, pruneModeForBase(targets, "live")) + assert.Equal(t, v1alpha3.PruneOnEvent, pruneModeForBase(targets, "legacy"), + "a metadata entry with no mode is unset, which means onEvent") + assert.Equal(t, v1alpha3.PruneOnEvent, pruneModeForBase(targets, "no-such-base"), + "an unresolvable target must mirror deletes, not silently archive") +} + +// TestShouldLogRetention_ThrottlesPerTarget pins the throttle. Retaining is a steady state and a +// resync fires per watched type and namespace, so an unthrottled default-verbosity line would +// scale with the reconcile rate — which is how a useful signal becomes noise nobody reads. +func TestShouldLogRetention_ThrottlesPerTarget(t *testing.T) { + w := &BranchWorker{} + + assert.True(t, w.shouldLogRetention("tenant-a"), "the first retention for a target is reported") + assert.False(t, w.shouldLogRetention("tenant-a"), "an immediate repeat is throttled") + assert.True(t, w.shouldLogRetention("tenant-b"), "a different target is throttled independently") + + // Age the stamp past the interval: the next one reports again, so a long-lived retention does + // not go permanently silent after its first line. + w.retentionLoggedAt["tenant-a"] = w.retentionLoggedAt["tenant-a"].Add(-2 * retentionLogInterval) + assert.True(t, w.shouldLogRetention("tenant-a")) +} diff --git a/internal/git/prune_replay_test.go b/internal/git/prune_replay_test.go new file mode 100644 index 00000000..16ec63b0 --- /dev/null +++ b/internal/git/prune_replay_test.go @@ -0,0 +1,235 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "context" + "errors" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + configv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" +) + +// A retained PendingWrite is committed locally but not yet pushed, and a push conflict REPLAYS it +// against the rebased worktree — re-planning, so it can decide deletions the first apply never +// made. These tests pin which policy that replay runs under. + +const ( + replayTargetName = "acme" + replayTargetNamespace = "tenant-acme" +) + +func replayScheme(t *testing.T) *runtime.Scheme { + t.Helper() + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, configv1alpha3.AddToScheme(scheme)) + return scheme +} + +// gitTargetWithMode is the CURRENT stored object — what an operator's `kubectl patch` produced +// after the pending write below was planned. +func gitTargetWithMode(mode configv1alpha3.PruneMode) *configv1alpha3.GitTarget { + return &configv1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{Name: replayTargetName, Namespace: replayTargetNamespace}, + Spec: configv1alpha3.GitTargetSpec{ + Path: "tenants/acme", + Prune: &configv1alpha3.PrunePolicy{Mode: mode}, + }, + } +} + +// retainedWriteUnder is a resync write already committed locally under plannedMode, waiting for a +// push that has not succeeded yet. +func retainedWriteUnder(plannedMode configv1alpha3.PruneMode) PendingWrite { + key := pendingTargetKey{Name: replayTargetName, Namespace: replayTargetNamespace} + return PendingWrite{ + Kind: PendingWriteResync, + GitTargetName: replayTargetName, + GitTargetNamespace: replayTargetNamespace, + Targets: map[pendingTargetKey]ResolvedTargetMetadata{ + key: { + Name: replayTargetName, + Namespace: replayTargetNamespace, + Path: "tenants/acme", + PruneMode: plannedMode, + }, + }, + } +} + +func replayWorker(t *testing.T, objects []client.Object, fns *interceptor.Funcs) *BranchWorker { + t.Helper() + builder := fake.NewClientBuilder().WithScheme(replayScheme(t)).WithObjects(objects...) + if fns != nil { + builder = builder.WithInterceptorFuncs(*fns) + } + worker := NewBranchWorker(builder.Build(), logr.Discard(), "provider", "default", "main", nil, 0) + worker.ctx = context.Background() + return worker +} + +func modeOfWrite(t *testing.T, pw PendingWrite) configv1alpha3.PruneMode { + t.Helper() + return pw.Target().PruneMode +} + +// TestTightenPendingPruneModes_TighteningReachesAWriteThatHasNotPushed is the emergency stop. +// +// A resync planned under `always` is committed locally and waiting on a push that keeps +// conflicting. The operator patches the GitTarget to `onEvent` to stop the deletions. Without this, +// the replay re-plans under the captured `always` and can sweep documents against a remote that has +// changed since — deletions the operator has already revoked. +func TestTightenPendingPruneModes_TighteningReachesAWriteThatHasNotPushed(t *testing.T) { + worker := replayWorker(t, []client.Object{gitTargetWithMode(configv1alpha3.PruneOnEvent)}, nil) + writes := []PendingWrite{retainedWriteUnder(configv1alpha3.PruneAlways)} + + require.NoError(t, worker.tightenPendingPruneModes(worker.ctx, writes)) + + assert.Equal(t, configv1alpha3.PruneOnEvent, modeOfWrite(t, writes[0]), + "a policy tightened after the write was planned must apply to the replay") +} + +// TestTightenPendingPruneModes_LooseningNeverEscalatesAPlannedWrite is the opposite direction, and +// it must NOT be symmetric. The retained write chose to keep its orphans against a desired snapshot +// that is now stale; declaring `always` afterwards applies to the next resync — which gathers a +// fresh snapshot — not to this one. +func TestTightenPendingPruneModes_LooseningNeverEscalatesAPlannedWrite(t *testing.T) { + worker := replayWorker(t, []client.Object{gitTargetWithMode(configv1alpha3.PruneAlways)}, nil) + writes := []PendingWrite{retainedWriteUnder(configv1alpha3.PruneOnEvent)} + + require.NoError(t, worker.tightenPendingPruneModes(worker.ctx, writes)) + + assert.Equal(t, configv1alpha3.PruneOnEvent, modeOfWrite(t, writes[0]), + "a widened policy must not turn a stale retention decision into deletions") +} + +// TestTightenPendingPruneModes_DeletedGitTargetReplaysUnderNever covers the definite no-answer: the +// GitTarget is gone, so no policy authorizes anything. Retrying could not produce a better answer, +// so the replay proceeds under the most restrictive mode rather than the captured one. +func TestTightenPendingPruneModes_DeletedGitTargetReplaysUnderNever(t *testing.T) { + worker := replayWorker(t, nil, nil) + writes := []PendingWrite{retainedWriteUnder(configv1alpha3.PruneAlways)} + + require.NoError(t, worker.tightenPendingPruneModes(worker.ctx, writes)) + + assert.Equal(t, configv1alpha3.PruneNever, modeOfWrite(t, writes[0]), + "a deleted GitTarget must not keep authorizing deletions through a retained write") +} + +// TestTightenPendingPruneModes_UnreadablePolicyRetriesInsteadOfGuessing covers the case with no +// answer at all. Guessing the captured mode could apply a revoked deletion; guessing the strictest +// mode could silently drop a legitimate one, which under `onEvent` no later resync re-derives. +// Returning the error does neither: the push cycle leaves the writes retained and tries again. +func TestTightenPendingPruneModes_UnreadablePolicyRetriesInsteadOfGuessing(t *testing.T) { + unavailable := apierrors.NewServiceUnavailable("cache not synced") + worker := replayWorker(t, []client.Object{gitTargetWithMode(configv1alpha3.PruneOnEvent)}, &interceptor.Funcs{ + Get: func( + _ context.Context, _ client.WithWatch, _ client.ObjectKey, obj client.Object, _ ...client.GetOption, + ) error { + if _, isTarget := obj.(*configv1alpha3.GitTarget); isTarget { + return unavailable + } + return nil + }, + }) + writes := []PendingWrite{retainedWriteUnder(configv1alpha3.PruneAlways)} + + err := worker.tightenPendingPruneModes(worker.ctx, writes) + + require.Error(t, err, "an unreadable policy must fail the replay so the push retries it") + assert.True(t, errors.Is(err, unavailable) || apierrors.IsServiceUnavailable(err), + "the underlying read failure must survive wrapping, so the retry is diagnosable") + assert.Equal(t, configv1alpha3.PruneAlways, modeOfWrite(t, writes[0]), + "nothing is decided when the policy could not be read") +} + +// TestTightenPendingPruneModes_CoversBothDeletionPaths is why the tightening mutates the shared +// Targets map rather than a local copy: the resync sweep reads its mode through PendingWrite.Target +// and the steady-state DELETE writer reads it through pruneModeForBase. One pass has to serve both, +// or `never` would stop half of what an operator just asked it to stop. +func TestTightenPendingPruneModes_CoversBothDeletionPaths(t *testing.T) { + worker := replayWorker(t, []client.Object{gitTargetWithMode(configv1alpha3.PruneNever)}, nil) + writes := []PendingWrite{retainedWriteUnder(configv1alpha3.PruneAlways)} + + require.NoError(t, worker.tightenPendingPruneModes(worker.ctx, writes)) + + assert.Equal(t, configv1alpha3.PruneNever, modeOfWrite(t, writes[0]), + "the resync sweep reads the mode here") + assert.Equal(t, configv1alpha3.PruneNever, pruneModeForBase(writes[0].Targets, "tenants/acme"), + "the steady-state DELETE writer reads it here, off the same map") +} + +// TestTightenPendingPruneModes_ReadsEachGitTargetOnce keeps the replay's cost proportional to the +// targets involved rather than to the retained writes: a conflicting push can be replaying many +// windows for one busy target. +func TestTightenPendingPruneModes_ReadsEachGitTargetOnce(t *testing.T) { + var gets int + worker := replayWorker(t, []client.Object{gitTargetWithMode(configv1alpha3.PruneOnEvent)}, &interceptor.Funcs{ + Get: func( + ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption, + ) error { + if _, isTarget := obj.(*configv1alpha3.GitTarget); isTarget { + gets++ + } + return c.Get(ctx, key, obj, opts...) + }, + }) + writes := []PendingWrite{ + retainedWriteUnder(configv1alpha3.PruneAlways), + retainedWriteUnder(configv1alpha3.PruneAlways), + retainedWriteUnder(configv1alpha3.PruneAlways), + } + + require.NoError(t, worker.tightenPendingPruneModes(worker.ctx, writes)) + + assert.Equal(t, 1, gets, "one policy read per GitTarget, not per retained write") + for i := range writes { + assert.Equal(t, configv1alpha3.PruneOnEvent, modeOfWrite(t, writes[i])) + } +} + +// TestMoreRestrictiveOf_OrdersTheModes pins the ordering the replay depends on, including the two +// values that are not modes: the empty string (unset, which means onEvent) and an unrecognized one +// (a policy this build cannot interpret, which must authorize nothing). +func TestMoreRestrictiveOf_OrdersTheModes(t *testing.T) { + for _, tc := range []struct { + name string + left, right configv1alpha3.PruneMode + want configv1alpha3.PruneMode + }{ + {"always over onEvent", configv1alpha3.PruneAlways, configv1alpha3.PruneOnEvent, configv1alpha3.PruneOnEvent}, + {"always over never", configv1alpha3.PruneAlways, configv1alpha3.PruneNever, configv1alpha3.PruneNever}, + {"onEvent over never", configv1alpha3.PruneOnEvent, configv1alpha3.PruneNever, configv1alpha3.PruneNever}, + {"never under always", configv1alpha3.PruneNever, configv1alpha3.PruneAlways, configv1alpha3.PruneNever}, + {"identical", configv1alpha3.PruneAlways, configv1alpha3.PruneAlways, configv1alpha3.PruneAlways}, + {"unset resolves to onEvent", "", configv1alpha3.PruneAlways, configv1alpha3.PruneOnEvent}, + {"unset against never", "", configv1alpha3.PruneNever, configv1alpha3.PruneNever}, + {"unrecognized authorizes nothing", configv1alpha3.PruneAlways, "someFutureMode", "someFutureMode"}, + } { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, tc.left.MoreRestrictiveOf(tc.right)) + }) + } +} + +// TestMoreRestrictiveOf_UnrecognizedResultStillAuthorizesNothing guards the one place returning the +// raw unrecognized value could go wrong: it is only safe because both predicates already read it as +// false. If that ever changes, an unknown policy would start deleting. +func TestMoreRestrictiveOf_UnrecognizedResultStillAuthorizesNothing(t *testing.T) { + result := configv1alpha3.PruneAlways.MoreRestrictiveOf("someFutureMode") + + assert.False(t, result.SweepsOrphans()) + assert.False(t, result.AppliesEventDeletes()) +} diff --git a/internal/git/render_fidelity_test.go b/internal/git/render_fidelity_test.go index b489e27f..db012b85 100644 --- a/internal/git/render_fidelity_test.go +++ b/internal/git/render_fidelity_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" "github.com/ConfigButler/gitops-reverser/internal/types" ) @@ -59,7 +60,7 @@ func TestRenderFidelityRefusal_BlocksLiveAndResyncWrites(t *testing.T) { name: "live event", run: func(worker *BranchWorker, worktree *gogit.Worktree) error { _, err := worker.flushEventsToWorktree( - context.Background(), worktree, "", []Event{postBuildTokenEvent()}, nil) + context.Background(), worktree, "", []Event{postBuildTokenEvent()}, nil, v1alpha3.PruneOnEvent) return err }, }, @@ -67,11 +68,12 @@ func TestRenderFidelityRefusal_BlocksLiveAndResyncWrites(t *testing.T) { name: "scoped resync", run: func(worker *BranchWorker, worktree *gogit.Worktree) error { _, _, err := worker.applyResyncToWorktree( - context.Background(), worktree, "", "", + context.Background(), worktree, "", + ResolvedTargetMetadata{PruneMode: v1alpha3.PruneAlways}, []manifestanalyzer.DesiredResource{{ Resource: postBuildTokenEvent().Identifier, Object: postBuildTokenEvent().Object, - }}, nil, nil) + }}, nil) return err }, }, diff --git a/internal/git/render_scope_test.go b/internal/git/render_scope_test.go index f6437c24..843484db 100644 --- a/internal/git/render_scope_test.go +++ b/internal/git/render_scope_test.go @@ -15,6 +15,7 @@ import ( gogit "github.com/go-git/go-git/v5" + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" "github.com/ConfigButler/gitops-reverser/internal/types" "github.com/ConfigButler/gitops-reverser/internal/typeset" @@ -130,7 +131,7 @@ func flushAtBase( ) (bool, error) { t.Helper() w := &BranchWorker{contentWriter: writer, mapper: mapper} - return w.flushEventsToWorktree(context.Background(), worktree, base, events, nil) + return w.flushEventsToWorktree(context.Background(), worktree, base, events, nil, v1alpha3.PruneOnEvent) } // The read scope of a pure overlay re-roots at the base's parent, keeps every scanned path diff --git a/internal/git/resync_flush.go b/internal/git/resync_flush.go index aaab9988..98ce92f4 100644 --- a/internal/git/resync_flush.go +++ b/internal/git/resync_flush.go @@ -7,12 +7,14 @@ import ( "errors" "fmt" "path/filepath" + "time" gogit "github.com/go-git/go-git/v5" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" "sigs.k8s.io/controller-runtime/pkg/log" + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" "github.com/ConfigButler/gitops-reverser/internal/manifestreport" @@ -239,7 +241,7 @@ func (w *BranchWorker) executeResyncPendingWrite( } stats, anyChanges, err := w.applyResyncToWorktree( - ctx, worktree, base, target.SourceCluster, pendingWrite.Desired, pendingWrite.Scope, target.Placement, + ctx, worktree, base, target, pendingWrite.Desired, pendingWrite.Scope, ) if err != nil { return 0, err @@ -315,21 +317,39 @@ func (w *BranchWorker) refuseUnsafeWorktree( // is swept clean to match. Nothing is flushed until every action applies cleanly, so a // mid-resync error (e.g. an encryption failure) commits nothing rather than a partial // sweep. +// +// The sweep half is additionally gated by the GitTarget's effective spec.prune.mode: under +// `never` and `onEvent` the planner emits no managed drop at all, so a resync whose desired +// set is narrowed by a bad scope, an outage, or a controller that does not understand a newer +// scope field updates and creates but removes nothing. `always` is the opt-in that restores +// the full convergence described above. func (w *BranchWorker) applyResyncToWorktree( ctx context.Context, worktree *gogit.Worktree, - base, clusterID string, + base string, + target ResolvedTargetMetadata, desired []manifestanalyzer.DesiredResource, scope *ResyncScope, - policy *manifestanalyzer.PlacementPolicy, ) (ResyncStats, bool, error) { + // Normalize the prune mode ONCE, here, on this function's own copy of the metadata: everything + // below asks the mode a question (may I sweep? what do I report?), and the empty string answers + // "Never" to all of them while meaning "OnEvent". Doing it at the single entry point is why no + // individual reader has to remember. + target.PruneMode = target.PruneMode.OrDefault() root := worktree.Filesystem.Root() scoped, err := scanRenderScope(root, base) if err != nil { return ResyncStats{}, false, err } - batch := newWriteBatch(ctx, w.contentWriter, w.mapperForCluster(clusterID), scoped.scan, policy, scoped.writeSubdir) + batch := newWriteBatch( + ctx, + w.contentWriter, + w.mapperForCluster(target.SourceCluster), + scoped.scan, + target.Placement, + scoped.writeSubdir, + ) // First materialization is the adoption gate: refuse a subtree that holds content the // operator cannot safely manage (unsupported kustomization, duplicate identity, impure // or non-KRM files, foreign content, a catastrophic .gittargetignore) and commit nothing, @@ -342,12 +362,14 @@ func (w *BranchWorker) applyResyncToWorktree( // see identical bytes. The planner is the authoritative mark-and-sweep over the resolved // resource-identity index; the upserts reuse the steady-state writer. A scoped resync // (M12 per-type) restricts the sweep to one type so no sibling document is dropped. - plan := resyncPlan(batch.store, scoped.scan.YAMLFiles, desired, scope) + plan := resyncPlan(batch.store, scoped.scan.YAMLFiles, desired, scope, target.PruneMode) + w.reportRetainedOrphans(ctx, plan, target, base, scope) stats, err := batch.applyResyncPlan(ctx, desired, plan) if err != nil { return ResyncStats{}, false, err } + stats.PruneMode = target.PruneMode // Anchored at renderBase; the write jail (writeSubdir) is enforced inside the flush. changed, err := batch.flush(ctx, worktree, root, scoped.renderBase) return stats, changed, err @@ -403,9 +425,96 @@ func (wb *writeBatch) applyResyncPlan( // Skipped stays a plan view (documents present but not editable in place); it is // informational only and not part of the GitTarget status. stats.Skipped = plan.Counts()[manifestanalyzer.PlanSkip] + // Retained is a plan view too, and necessarily so: it counts drops the planner did NOT emit, + // so there is no action to observe here. Carrying it on the stats is what lets it leave the + // writer at all. + stats.Retained = plan.RetainedOrphans return stats, nil } +// retentionLogInterval bounds how often ONE GitTarget subtree's suppressed sweep is reported at +// default verbosity. A resync fires per watched type, and per namespace within a type, so an +// unthrottled line would repeat for every one of them on every reconcile of a target that is +// deliberately retaining — a steady state, not an incident. V(1) is never throttled. +const retentionLogInterval = 10 * time.Minute + +// reportRetainedOrphans surfaces a mark-and-sweep the target's prune policy suppressed. +// +// Retention is the CONFIGURED outcome, so every signal here is informational: no error, no +// GitTarget condition, no background-failure count. A stale Git document under `onEvent` is the +// feature working, and raising a failure for it would train operators to ignore the one condition +// that means their mirror is actually broken. +// +// It is worth reporting at all because a suppressed drop is otherwise INVISIBLE: it produces no +// plan action, no commit, and no ResyncStats entry, so an operator comparing the folder to the +// cluster has nothing to distinguish "converged" from "deliberately retaining stale documents". +// The GitTarget is named on BOTH signals, because neither is actionable without it and `path` +// cannot stand in: two GitTargets in different namespaces may write the same spec.path on +// different branches of one repository, so a folder does not identify a target. +func (w *BranchWorker) reportRetainedOrphans( + ctx context.Context, + plan manifestanalyzer.Plan, + target ResolvedTargetMetadata, + base string, + scope *ResyncScope, +) { + if plan.RetainedOrphans == 0 { + return + } + gitTarget := target.Namespace + "/" + target.Name + recordPruneRetention(ctx, target, plan.RetainedOrphans) + logger := log.FromContext(ctx).WithValues( + "retained", plan.RetainedOrphans, "pruneMode", string(target.PruneMode), + "gitTarget", gitTarget, "path", base, "scope", scope.String()) + logger.V(1).Info("resync retained managed documents (spec.prune.mode)") + if !w.shouldLogRetention(gitTarget + "@" + base) { + return + } + logger.Info("resync retained managed documents absent from the cluster; " + + "set spec.prune.mode: Always on the GitTarget to remove them") +} + +// shouldLogRetention reports whether one target subtree's retention may be logged at default +// verbosity now, stamping the moment when it may. The key is the GitTarget plus its path rather +// than the path alone: co-resident targets writing the same path on different branches share a +// worker only by accident, but when they do, one throttling the other's line is a silent loss. +// +// The event loop is the only caller and it is single-goroutine (handleQueueItem, and the +// rebase-replay that re-executes retained writes, both run on it), so the map needs no lock — the +// same ownership branchWorkerLogFirsts relies on. +func (w *BranchWorker) shouldLogRetention(key string) bool { + now := time.Now() + if last, seen := w.retentionLoggedAt[key]; seen && now.Sub(last) < retentionLogInterval { + return false + } + if w.retentionLoggedAt == nil { + w.retentionLoggedAt = make(map[string]time.Time) + } + w.retentionLoggedAt[key] = now + return true +} + +// recordPruneRetention counts documents a prune policy kept, labelled by the GitTarget that kept +// them and the mode it kept them under — "which target is retaining, and why" is the operational +// question, and a counter that cannot name the target only answers it for a single-target +// deployment. It is the retention twin of ResyncSweepDeletesTotal. +// +// Cardinality is bounded by the number of GitTargets, not by resources: the per-path, per-scope and +// per-document detail deliberately stays in the log line. The label names follow the convention +// TargetReconcileCompletedTotal already sets — gittarget_namespace / gittarget_name rather than the +// reserved namespace / name, because a pod scrape with honor_labels=false overwrites a metric's +// `namespace` attribute with the scraping pod's own and silently breaks any per-target selector. +func recordPruneRetention(ctx context.Context, target ResolvedTargetMetadata, retained int) { + if telemetry.PruneRetainedDocumentsTotal == nil { + return + } + telemetry.PruneRetainedDocumentsTotal.Add(ctx, int64(retained), metric.WithAttributes( + attribute.String("prune_mode", string(target.PruneMode)), + attribute.String("gittarget_namespace", target.Namespace), + attribute.String("gittarget_name", target.Name), + )) +} + func recordResyncSweepDelete(ctx context.Context, resource types.ResourceIdentifier) { if telemetry.ResyncSweepDeletesTotal == nil { return @@ -467,19 +576,31 @@ func resyncPlan( files []manifestedit.FileContent, desired []manifestanalyzer.DesiredResource, scope *ResyncScope, + pruneMode v1alpha3.PruneMode, ) manifestanalyzer.Plan { + policy := resyncPlanPolicy(pruneMode) if scope == nil { - return manifestanalyzer.BuildPlan(store, files, desired, resyncPlanPolicy()) + return manifestanalyzer.BuildPlan(store, files, desired, policy) } - return manifestanalyzer.BuildScopedPlan(store, files, desired, resyncPlanPolicy(), scope.Matches) + return manifestanalyzer.BuildScopedPlan(store, files, desired, policy, scope.Matches) } // resyncPlanPolicy is the planning policy for a resync: the same sanitized projection // and edit options the steady-state writer uses, so a resync and a live event reach -// the same patch/replace/skip decision for the same resource. -func resyncPlanPolicy() manifestanalyzer.Policy { +// the same patch/replace/skip decision for the same resource — plus the target's prune +// policy, which is the only input that differs between the two. +// +// The mode is translated to a SweepMode here rather than passed down, because the planner +// models only the INFERRED deletion path: `never` and `onEvent` are the same instruction to +// it (retain), and they diverge only at the writer, which the planner never reaches. +func resyncPlanPolicy(pruneMode v1alpha3.PruneMode) manifestanalyzer.Policy { + sweep := manifestanalyzer.SweepRetainOrphans + if pruneMode.SweepsOrphans() { + sweep = manifestanalyzer.SweepDropOrphans + } return manifestanalyzer.Policy{ Project: manifestreport.Project, EditOptions: manifestreport.EditOptions(), + Sweep: sweep, } } diff --git a/internal/git/resync_flush_test.go b/internal/git/resync_flush_test.go index 59e23048..10c4a013 100644 --- a/internal/git/resync_flush_test.go +++ b/internal/git/resync_flush_test.go @@ -14,6 +14,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" "github.com/ConfigButler/gitops-reverser/internal/telemetry" "github.com/ConfigButler/gitops-reverser/internal/types" @@ -65,7 +66,14 @@ func applyResyncViaWorktree( ) (ResyncStats, bool) { t.Helper() w := &BranchWorker{contentWriter: writer, mapper: mapper} - stats, changed, err := w.applyResyncToWorktree(context.Background(), worktree, "", "", desired, nil, nil) + stats, changed, err := w.applyResyncToWorktree( + context.Background(), + worktree, + "", + ResolvedTargetMetadata{PruneMode: v1alpha3.PruneAlways}, + desired, + nil, + ) require.NoError(t, err) return stats, changed } @@ -222,7 +230,14 @@ func TestResync_ScopedSweepDropsOnlyTargetType(t *testing.T) { w := &BranchWorker{contentWriter: writer, mapper: twoTypeMapper()} scope := &ResyncScope{GVR: schema.GroupVersionResource{Group: "", Version: "v1", Resource: "configmaps"}} - stats, changed, err := w.applyResyncToWorktree(context.Background(), worktree, "", "", nil, scope, nil) + stats, changed, err := w.applyResyncToWorktree( + context.Background(), + worktree, + "", + ResolvedTargetMetadata{PruneMode: v1alpha3.PruneAlways}, + nil, + scope, + ) require.NoError(t, err) require.True(t, changed, "the removed type's document is swept") assert.Equal(t, 1, stats.Deleted, "exactly the configmap is swept, not the secret") @@ -314,7 +329,14 @@ func TestResync_UnsafePlacementCountsAsPlacementSkipped(t *testing.T) { } w := &BranchWorker{contentWriter: writer, mapper: twoTypeMapper()} - stats, _, err := w.applyResyncToWorktree(context.Background(), worktree, "", "", desired, nil, policy) + stats, _, err := w.applyResyncToWorktree( + context.Background(), + worktree, + "", + ResolvedTargetMetadata{Placement: policy, PruneMode: v1alpha3.PruneAlways}, + desired, + nil, + ) require.NoError(t, err) assert.Equal(t, 1, stats.PlacementSkipped, @@ -363,10 +385,9 @@ func TestResync_SensitiveUpdateCountsAsUpdatedNotSkipped(t *testing.T) { context.Background(), worktree, "", - "", + ResolvedTargetMetadata{PruneMode: v1alpha3.PruneAlways}, []manifestanalyzer.DesiredResource{desired}, nil, - nil, ) require.NoError(t, err) require.True(t, changed, "the secret is re-encrypted") diff --git a/internal/git/resync_scope_test.go b/internal/git/resync_scope_test.go index 4f632256..afda495d 100644 --- a/internal/git/resync_scope_test.go +++ b/internal/git/resync_scope_test.go @@ -12,6 +12,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" "github.com/ConfigButler/gitops-reverser/internal/types" ) @@ -136,7 +137,14 @@ func TestResync_NamespaceScopedSweepLeavesSiblingNamespacesAlone(t *testing.T) { // team-a replays and finds nothing: its namespace is empty at the pinned revision. w := &BranchWorker{contentWriter: writer, mapper: configMapMapper()} scope := &ResyncScope{GVR: configmapsGVRForScope, Namespace: "team-a"} - stats, changed, err := w.applyResyncToWorktree(context.Background(), worktree, "", "", nil, scope, nil) + stats, changed, err := w.applyResyncToWorktree( + context.Background(), + worktree, + "", + ResolvedTargetMetadata{PruneMode: v1alpha3.PruneAlways}, + nil, + scope, + ) require.NoError(t, err) require.True(t, changed, "team-a's orphaned document is swept") assert.Equal(t, 1, stats.Deleted, "exactly team-a's document is swept, not team-b's") @@ -160,9 +168,11 @@ func TestResync_NamespaceScopedSweepStillDropsOrphansInItsOwnNamespace(t *testin w := &BranchWorker{contentWriter: writer, mapper: configMapMapper()} scope := &ResyncScope{GVR: configmapsGVRForScope, Namespace: "team-a"} stats, changed, err := w.applyResyncToWorktree( - context.Background(), worktree, "", "", []manifestanalyzer.DesiredResource{ + context.Background(), worktree, "", + ResolvedTargetMetadata{PruneMode: v1alpha3.PruneAlways}, + []manifestanalyzer.DesiredResource{ desiredCMIn("kept", "team-a", "blue"), - }, scope, nil) + }, scope) require.NoError(t, err) require.True(t, changed) assert.Equal(t, 1, stats.Deleted, "the orphan inside the scoped namespace is still swept") @@ -184,7 +194,14 @@ func TestResync_ClusterWideScopeStillSweepsEveryNamespace(t *testing.T) { w := &BranchWorker{contentWriter: writer, mapper: configMapMapper()} scope := &ResyncScope{GVR: configmapsGVRForScope} // no namespace: all-namespaces - stats, changed, err := w.applyResyncToWorktree(context.Background(), worktree, "", "", nil, scope, nil) + stats, changed, err := w.applyResyncToWorktree( + context.Background(), + worktree, + "", + ResolvedTargetMetadata{PruneMode: v1alpha3.PruneAlways}, + nil, + scope, + ) require.NoError(t, err) require.True(t, changed) assert.Equal(t, 2, stats.Deleted, "an all-namespaces scope sweeps the type in every namespace") diff --git a/internal/git/retention_report_test.go b/internal/git/retention_report_test.go new file mode 100644 index 00000000..853bf3b6 --- /dev/null +++ b/internal/git/retention_report_test.go @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "context" + "testing" + + "github.com/go-logr/logr/funcr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sigs.k8s.io/controller-runtime/pkg/log" + + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// retentionLogCapture returns a context whose logger records every default-verbosity line, plus +// the slice the lines land in. V(1) is dropped so the assertions see exactly what an operator +// running at default verbosity sees. +func retentionLogCapture() (context.Context, *[]string) { + var lines []string + logger := funcr.New(func(_, args string) { + lines = append(lines, args) + }, funcr.Options{}) + return log.IntoContext(context.Background(), logger), &lines +} + +func retainingTarget(namespace, name, path string) ResolvedTargetMetadata { + return ResolvedTargetMetadata{ + Name: name, + Namespace: namespace, + Path: path, + PruneMode: v1alpha3.PruneOnEvent, + } +} + +// TestReportRetainedOrphans_NamesTheGitTarget holds the promise both configuration.md and +// UPGRADING.md make to operators: the retention line names the target, so a non-zero retention is +// actionable without correlating a folder back to an object. The folder alone cannot do that — +// see the co-resident case below. +func TestReportRetainedOrphans_NamesTheGitTarget(t *testing.T) { + ctx, lines := retentionLogCapture() + w := &BranchWorker{} + + w.reportRetainedOrphans( + ctx, + manifestanalyzer.Plan{RetainedOrphans: 3}, + retainingTarget("tenant-acme", "acme", "tenants/acme"), + "tenants/acme", + nil, + ) + + require.Len(t, *lines, 1, "a retention is reported once at default verbosity") + assert.Contains(t, (*lines)[0], `"gitTarget"="tenant-acme/acme"`, + "the retention line must name the GitTarget, not only its path") + assert.Contains(t, (*lines)[0], `"retained"=3`) + assert.Contains(t, (*lines)[0], `"pruneMode"="OnEvent"`) +} + +// TestReportRetainedOrphans_ThrottlesPerTargetNotPerPath is why the throttle key is the GitTarget +// plus its path rather than the path alone. Two targets in different namespaces may write the same +// spec.path on different branches; if they ever share a worker, keying on the path alone means the +// first one to report silences the second for ten minutes — a retention that is real, configured, +// and invisible. +func TestReportRetainedOrphans_ThrottlesPerTargetNotPerPath(t *testing.T) { + ctx, lines := retentionLogCapture() + w := &BranchWorker{} + plan := manifestanalyzer.Plan{RetainedOrphans: 1} + + w.reportRetainedOrphans(ctx, plan, retainingTarget("tenant-a", "mirror", "shared"), "shared", nil) + w.reportRetainedOrphans(ctx, plan, retainingTarget("tenant-b", "mirror", "shared"), "shared", nil) + // The same target again inside the interval is the case the throttle exists for. + w.reportRetainedOrphans(ctx, plan, retainingTarget("tenant-a", "mirror", "shared"), "shared", nil) + + require.Len(t, *lines, 2, "each target reports once; the repeat is throttled") + assert.Contains(t, (*lines)[0], `"gitTarget"="tenant-a/mirror"`) + assert.Contains(t, (*lines)[1], `"gitTarget"="tenant-b/mirror"`) +} + +// TestReportRetainedOrphans_SilentWhenNothingIsRetained keeps the signal meaningful: a converged +// mirror under any mode must not log or count. Losing this turns the line into noise operators +// filter out, which defeats the whole point of reporting a retention at all. +func TestReportRetainedOrphans_SilentWhenNothingIsRetained(t *testing.T) { + ctx, lines := retentionLogCapture() + w := &BranchWorker{} + + w.reportRetainedOrphans(ctx, manifestanalyzer.Plan{}, retainingTarget("ns", "target", "p"), "p", nil) + + assert.Empty(t, *lines) +} + +// TestReportRetainedOrphans_ReportsTheEffectiveModeForALegacyTarget covers the GitTarget created +// before spec.prune existed: it stores no mode at all. Routed through the real apply, because +// that is where the single normalization lives — reporting the raw "" would name a mode that does +// not exist, and one that answers "false" to both deletion predicates while meaning onEvent. +func TestReportRetainedOrphans_ReportsTheEffectiveModeForALegacyTarget(t *testing.T) { + ctx, lines := retentionLogCapture() + worktree := newWorktreeForTest(t) + seedPlacedManifest(t, worktree, "apps/orphan.yaml", cmManifest("orphan", "blue")) + w := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} + + _, _, err := w.applyResyncToWorktree( + ctx, worktree, "", + ResolvedTargetMetadata{Name: "acme", Namespace: "tenant-acme"}, // no PruneMode: a legacy target + nil, nil, + ) + require.NoError(t, err) + + require.Len(t, *lines, 1, "the orphan is retained under the effective default, and reported") + assert.Contains(t, (*lines)[0], `"pruneMode"="OnEvent"`) + assert.Contains(t, (*lines)[0], `"gitTarget"="tenant-acme/acme"`) +} diff --git a/internal/git/types.go b/internal/git/types.go index 6135b030..0a11b42c 100644 --- a/internal/git/types.go +++ b/internal/git/types.go @@ -242,6 +242,20 @@ type ResolvedTargetMetadata struct { // from spec.placement. Nil when the GitTarget declares none, in which case new // resources are placed by sibling inference and then the canonical path. Placement *manifestanalyzer.PlacementPolicy + // PruneMode is the GitTarget's EFFECTIVE spec.prune.mode — always a concrete value, + // because it is resolved through EffectivePruneMode and an omitted policy is onEvent. + // It gates both deletion paths: the resync mark-and-sweep (through the planner's + // SweepMode) and the steady-state DELETE-event writer. + // + // Retained on the pending write with the rest of the target's metadata, so a write replayed + // after a rebase is not re-planned under a LOOSER policy than the one it was planned against: + // its retention decisions were taken over a desired snapshot that is now stale, and a later + // `always` applies to the next resync, which gathers a fresh one. + // + // It is not frozen, though. tightenPendingPruneModes lowers it before a replay when the + // GitTarget's current policy is stricter, because the whole point of tightening a deletion + // policy is to stop deletions that have not landed yet. + PruneMode v1alpha3.PruneMode // SourceCluster is the NAME of the source cluster the GitTarget mirrors from — // (api/v1alpha3).GitTarget.SourceCluster(), the referenced ClusterProvider's name // ("default" for the in-cluster provider). The resync mark-and-sweep resolves this subtree's @@ -414,6 +428,17 @@ type ResyncStats struct { Deleted int Skipped int PlacementSkipped int + // Retained is how many managed documents this resync's prune policy kept that a converged + // mirror would have dropped. It is the ONE count here that does not describe something the + // resync did: a suppressed drop produces no action, no commit, and no other stat, so without + // it nothing downstream can tell a converged mirror from a deliberately retaining one. It + // rides the reply channel to the drain, which rolls it up onto GitTarget status. + Retained int + // PruneMode stamps Retained with the effective policy that produced it, so the count and the + // reason for it travel together. Reading the mode from the spec at projection time instead + // would let a target that has just been switched publish a new mode beside a count the old + // one produced. + PruneMode v1alpha3.PruneMode } // reply delivers a result on the request's buffered channel without blocking, so a diff --git a/internal/git/write_boundary_precondition_test.go b/internal/git/write_boundary_precondition_test.go index fff9cd1c..b5f093c2 100644 --- a/internal/git/write_boundary_precondition_test.go +++ b/internal/git/write_boundary_precondition_test.go @@ -147,7 +147,7 @@ func TestFanInPrecondition_RefusesAmbiguousOverrideWriteThrough(t *testing.T) { w := &BranchWorker{contentWriter: writer, mapper: deploymentMapper()} _, err := w.flushEventsToWorktree(context.Background(), worktree, "", - []Event{overridesDeploymentEvent("ghcr.io/example/podinfo:9.9.9", 3)}, nil) + []Event{overridesDeploymentEvent("ghcr.io/example/podinfo:9.9.9", 3)}, nil, configv1alpha3.PruneOnEvent) assert.Contains(t, refusalIssueKinds(t, err), manifestanalyzer.IssueWriteFanIn, "an ambiguous-override write-through must be refused, not written through") diff --git a/internal/manifestanalyzer/plan.go b/internal/manifestanalyzer/plan.go index 645d367b..63af4b2d 100644 --- a/internal/manifestanalyzer/plan.go +++ b/internal/manifestanalyzer/plan.go @@ -30,6 +30,14 @@ type Plan struct { // Diagnostics are planning-level problems (e.g. a touched file whose bytes were // not provided for hydration). Store-level diagnostics stay on the ManifestStore. Diagnostics []manifestedit.Diagnostic + // RetainedOrphans counts the managed drops the sweep policy SUPPRESSED: documents + // that would have been PlanDropOrphan under SweepDropOrphans and produced no action + // at all under SweepRetainOrphans. It exists because a suppressed drop leaves no other + // trace — the whole point is that it is absent from Actions, so nothing downstream + // could otherwise tell "the mirror is converged" from "the mirror has stale documents + // the policy is deliberately keeping". Purely informational: it is a count, not an + // action, and it never reaches the commit path. + RetainedOrphans int } // PlanActionKind enumerates what a single action does. The seven kinds are the @@ -138,6 +146,39 @@ type DesiredResource struct { Object *unstructured.Unstructured } +// SweepMode decides whether the Git-only mark-and-sweep may turn an unmatched managed +// document into a managed drop. It is the planner-local shape of a GitTarget's +// spec.prune.mode, kept as its own type (like PlacementPolicy) so manifestanalyzer stays +// free of any Kubernetes API type dependency. +// +// Only the INFERRED deletion path is modelled here. An explicit source DELETE event +// never reaches this planner — it is resolved by PlanDelete and gated at the writer — so +// PruneNever and PruneOnEvent both map to SweepRetainOrphans. The two differ only on the +// path this type knows nothing about. +type SweepMode string + +const ( + // SweepUnspecified is the ZERO VALUE, and it RETAINS. + // + // The direction is not arbitrary. A caller that forgets this field gets the outcome + // whose failure mode is a stale document; the other default's failure mode is deleting + // a tenant's manifests from a snapshot that was never authoritative. Every production + // caller sets the field explicitly regardless — this is the backstop for the one that + // is added later, not a default anyone should rely on. + SweepUnspecified SweepMode = "" + // SweepRetainOrphans emits no managed drop. An unmatched document produces no action + // at all, so it is absent from the plan, the plan's action ordering, and the commit — + // it is not a filtered-out action but an action that was never planned. + SweepRetainOrphans SweepMode = "retain" + // SweepDropOrphans emits PlanDropOrphan for every unmatched, followable managed + // document in scope: full desired-state convergence. + SweepDropOrphans SweepMode = "drop" +) + +// DropsOrphans reports whether this mode may emit a managed drop. Only SweepDropOrphans +// does; every other value, including an unrecognized one, retains. +func (m SweepMode) DropsOrphans() bool { return m == SweepDropOrphans } + // Policy is the injected planning policy. The planner stays a pure function and // pulls every cluster-shaped or rendering-shaped decision out into this struct, so // the production wiring (manifestreport.Project / EditOptions) lives at the call @@ -149,6 +190,9 @@ type Policy struct { // EditOptions are the manifestedit options (canonical renderer, list-match) used // when Decide must compare and choose patch vs. whole-replace. EditOptions manifestedit.EditOptions + // Sweep decides whether the Git-only mark-and-sweep may emit managed drops. Its zero + // value retains, so every caller that wants convergence must say so — see SweepMode. + Sweep SweepMode } // BuildPlan computes the Plan from the byte-free ManifestStore, the file bytes @@ -166,7 +210,8 @@ type Policy struct { // The store is expected to have been built with the same mapper whose watched set // produced desired; under a structure-only store (no resolved mappings) no managed // drop is ever emitted, preserving the no-cluster promise even if a desired set is -// passed by mistake. +// passed by mistake. policy.Sweep is the second, independent gate on the same +// deletions — the caller's declared prune policy — and its zero value retains. func BuildPlan( store *ManifestStore, files []manifestedit.FileContent, @@ -214,6 +259,7 @@ func BuildScopedPlan( collided: collidedIdentities(store), matched: map[*DocumentModel]bool{}, inScope: inScope, + sweep: policy.Sweep, } // Desired side: create / patch / replace / skip for every cluster object, and @@ -241,7 +287,7 @@ func BuildScopedPlan( } sortActions(b.actions) - return Plan{Actions: b.actions, Diagnostics: b.diags} + return Plan{Actions: b.actions, Diagnostics: b.diags, RetainedOrphans: b.retained} } // planBuilder accumulates a plan's actions and diagnostics while BuildPlan walks @@ -263,6 +309,13 @@ type planBuilder struct { // whole-folder BuildPlan it is allInScope (always true); for a per-type reconcile/sweep // it matches one type's (group, resource), so out-of-scope documents are never dropped. inScope func(types.ResourceIdentifier) bool + // sweep decides whether an in-scope, unmatched document becomes a managed drop at all. + // It is orthogonal to inScope: inScope answers "is this document any of my business", + // sweep answers "may I delete the ones that are". + sweep SweepMode + // retained counts the in-scope managed drops sweep suppressed, surfaced as + // Plan.RetainedOrphans. + retained int } // planDesired classifies one desired resource against the store and appends its @@ -386,12 +439,20 @@ func (b *planBuilder) planGitOnly(dm *DocumentModel) { // one the registry resolved to a served, policy-allowed GVR — is dropped. // Not-followable KRM and no-source documents produce no action: they are refused // at acceptance, never pruned. - if dm.Mapping == MappingFollowable { - b.actions = append(b.actions, PlanAction{ - Kind: PlanDropOrphan, Ref: ref, Identity: dm.ManifestIdentity, Resource: resourceOf(dm), - Reason: "watched resource absent from the cluster: managed drop", - }) + if dm.Mapping != MappingFollowable { + return } + if !b.sweep.DropsOrphans() { + // The target's prune policy keeps inferred deletions off. The drop is not planned + // at all — not planned and then filtered — so it cannot reach the plan's action + // list, its ordering, or the commit. Counting it is the only trace it leaves. + b.retained++ + return + } + b.actions = append(b.actions, PlanAction{ + Kind: PlanDropOrphan, Ref: ref, Identity: dm.ManifestIdentity, Resource: resourceOf(dm), + Reason: "watched resource absent from the cluster: managed drop", + }) } // actionFromDecision maps a manifestedit decision intent to a plan action kind. The diff --git a/internal/manifestanalyzer/plan_test.go b/internal/manifestanalyzer/plan_test.go index 8e334519..f1f1cbf9 100644 --- a/internal/manifestanalyzer/plan_test.go +++ b/internal/manifestanalyzer/plan_test.go @@ -116,7 +116,7 @@ func TestBuildPlan_Patch(t *testing.T) { store := planStore(t) // ConfigMaps in sync, Deployment differs. desired := []DesiredResource{desiredConfigMap("a"), desiredConfigMap("b"), desiredDeployWeb(3)} - plan := BuildPlan(store, planFiles(), desired, Policy{}) + plan := BuildPlan(store, planFiles(), desired, convergingPolicy()) if len(plan.Actions) != 1 { t.Fatalf("want exactly one patch, got %+v", plan.Actions) @@ -141,7 +141,7 @@ func TestBuildPlan_Patch(t *testing.T) { func TestBuildPlan_Create(t *testing.T) { store := planStore(t) desired := append(inSync(), desiredConfigMap("c")) // c is brand new - plan := BuildPlan(store, planFiles(), desired, Policy{}) + plan := BuildPlan(store, planFiles(), desired, convergingPolicy()) if len(plan.Actions) != 1 { t.Fatalf("actions = %+v, want exactly one create", plan.Actions) @@ -168,7 +168,7 @@ func TestBuildPlan_Create(t *testing.T) { // TestBuildPlan_NoOp: every desired resource matching Git yields no actions at all. func TestBuildPlan_NoOp(t *testing.T) { store := planStore(t) - plan := BuildPlan(store, planFiles(), inSync(), Policy{}) + plan := BuildPlan(store, planFiles(), inSync(), convergingPolicy()) if len(plan.Actions) != 0 { t.Fatalf("in-sync plan should have no actions, got %+v", plan.Actions) } @@ -178,7 +178,7 @@ func TestBuildPlan_NoOp(t *testing.T) { // document is a managed drop, while the disallowed Secret is left untouched. func TestBuildPlan_DropOrphans(t *testing.T) { store := planStore(t) - plan := BuildPlan(store, planFiles(), nil, Policy{}) + plan := BuildPlan(store, planFiles(), nil, convergingPolicy()) counts := plan.Counts() if counts[PlanDropOrphan] != 3 || len(plan.Actions) != 3 { @@ -201,7 +201,7 @@ func TestBuildPlan_DropOrphans(t *testing.T) { func TestBuildPlan_SkipEncrypted(t *testing.T) { store := planStore(t) desired := append(inSync(), desiredSecret()) - plan := BuildPlan(store, planFiles(), desired, Policy{}) + plan := BuildPlan(store, planFiles(), desired, convergingPolicy()) skip := findAction(t, plan, "secret.sops.yaml") if skip.Kind != PlanSkip { @@ -231,12 +231,19 @@ func TestBuildPlan_DuplicateSuppressed(t *testing.T) { // A desired update to the collided identity is suppressed: the winner is not // patched. - if plan := BuildPlan(store, files, []DesiredResource{desiredDeployWeb(3)}, Policy{}); len(plan.Actions) != 0 { + if plan := BuildPlan( + store, + files, + []DesiredResource{desiredDeployWeb(3)}, + convergingPolicy(), + ); len( + plan.Actions, + ) != 0 { t.Errorf("collided identity should produce no action on update, got %+v", plan.Actions) } // An empty desired set does not drop the collided identity either. - if plan := BuildPlan(store, files, nil, Policy{}); len(plan.Actions) != 0 { + if plan := BuildPlan(store, files, nil, convergingPolicy()); len(plan.Actions) != 0 { t.Errorf("collided identity should produce no drop, got %+v", plan.Actions) } } @@ -248,11 +255,11 @@ func TestBuildPlan_DuplicateSuppressed(t *testing.T) { func TestBuildPlan_StructureOnlyNeverDrops(t *testing.T) { store := BuildStore(context.Background(), planFS(), nil) - if plan := BuildPlan(store, planFiles(), nil, Policy{}); len(plan.Actions) != 0 { + if plan := BuildPlan(store, planFiles(), nil, convergingPolicy()); len(plan.Actions) != 0 { t.Fatalf("structure-only plan should never drop, got %+v", plan.Actions) } - plan := BuildPlan(store, planFiles(), []DesiredResource{desiredDeployWeb(5)}, Policy{}) + plan := BuildPlan(store, planFiles(), []DesiredResource{desiredDeployWeb(5)}, convergingPolicy()) if len(plan.Actions) != 1 || plan.Actions[0].Kind != PlanPatch { t.Fatalf("structure-only differing object should patch, got %+v", plan.Actions) } @@ -273,7 +280,7 @@ func TestBuildPlan_NonEditableConstructSkips(t *testing.T) { t.Fatalf("anchor/alias document should not claim its identity") } - plan := BuildPlan(store, files, []DesiredResource{desiredConfigMap("anchored")}, Policy{}) + plan := BuildPlan(store, files, []DesiredResource{desiredConfigMap("anchored")}, convergingPolicy()) if len(plan.Actions) != 1 || plan.Actions[0].Kind != PlanSkip { t.Fatalf("non-editable construct should yield one skip, got %+v", plan.Actions) } @@ -284,7 +291,7 @@ func TestBuildPlan_NonEditableConstructSkips(t *testing.T) { // a no-op. func TestBuildPlan_ProjectPolicy(t *testing.T) { store := planStore(t) - policy := Policy{Project: func(_ *unstructured.Unstructured) *unstructured.Unstructured { + policy := Policy{Sweep: SweepDropOrphans, Project: func(_ *unstructured.Unstructured) *unstructured.Unstructured { return deployWeb(1) // normalize back to the Git value }} // Live says 9, the projection normalizes back to 1. @@ -310,7 +317,7 @@ func TestBuildPlan_MissingHydration(t *testing.T) { } } desired := []DesiredResource{desiredConfigMap("a"), desiredConfigMap("b"), desiredDeployWeb(3)} - plan := BuildPlan(store, files, desired, Policy{}) + plan := BuildPlan(store, files, desired, convergingPolicy()) skip := findAction(t, plan, "deploy.yaml") if skip.Kind != PlanSkip { @@ -326,7 +333,7 @@ func TestBuildPlan_MissingHydration(t *testing.T) { func TestBuildPlan_TwoCreatesSortByIdentity(t *testing.T) { store := planStore(t) desired := append(inSync(), desiredConfigMap("z"), desiredConfigMap("m")) - plan := BuildPlan(store, planFiles(), desired, Policy{}) + plan := BuildPlan(store, planFiles(), desired, convergingPolicy()) if len(plan.Actions) != 2 { t.Fatalf("want two creates, got %+v", plan.Actions) @@ -344,7 +351,7 @@ func TestBuildPlan_NilObjectGhostInert(t *testing.T) { store := planStore(t) desired := append(inSync(), DesiredResource{Resource: types.NewResourceIdentifier("", "v1", "configmaps", "default", "ghost")}) - plan := BuildPlan(store, planFiles(), desired, Policy{}) + plan := BuildPlan(store, planFiles(), desired, convergingPolicy()) if len(plan.Actions) != 0 { t.Fatalf("a nil-Object entry must produce no action, got %+v", plan.Actions) @@ -364,7 +371,7 @@ func TestBuildPlan_NilObjectProtectsExistingResource(t *testing.T) { desiredConfigMap("a"), desiredConfigMap("b"), {Resource: types.NewResourceIdentifier("apps", "v1", "deployments", "default", "web")}, } - plan := BuildPlan(store, planFiles(), desired, Policy{}) + plan := BuildPlan(store, planFiles(), desired, convergingPolicy()) for _, a := range plan.Actions { if a.Kind == PlanDropOrphan { @@ -390,7 +397,7 @@ func TestBuildPlan_ImpureFileTrueIndices(t *testing.T) { files := []manifestedit.FileContent{{Path: "app.yaml", Content: []byte(impure)}} store := BuildStore(context.Background(), fsys, typeset.NewSnapshotRegistry(sampleClusterSnapshot())) - plan := BuildPlan(store, files, nil, Policy{}) + plan := BuildPlan(store, files, nil, convergingPolicy()) idxByKind := map[string]int{} for _, a := range plan.Actions { diff --git a/internal/manifestanalyzer/scan.go b/internal/manifestanalyzer/scan.go index 96a68acc..3ac2e5a8 100644 --- a/internal/manifestanalyzer/scan.go +++ b/internal/manifestanalyzer/scan.go @@ -65,10 +65,27 @@ type ScanPolicy struct { // Acceptance configures the adoption gate (allowlist + scope). Its allowlist also // drives store construction, so allowlisted documents are retained, not planned. Acceptance AcceptancePolicy - // Plan configures the planner (projection + edit options). + // Plan configures the planner (projection + edit options). A dry-run has no GitTarget + // and therefore no prune policy to read, so a caller that wants the folder's orphans + // listed must set Plan.Sweep to SweepDropOrphans deliberately — see FolderScanPlanPolicy. Plan Policy } +// FolderScanPlanPolicy is the planning policy for an offline folder scan: it DROPS orphans. +// +// A scan is a report, not a write, and it is run against a folder rather than against a +// GitTarget — there is no spec.prune to consult. Reporting the full convergence view is the +// analysis the tool exists to produce; suppressing it would make a folder full of stale +// documents render identically to a converged one, with nothing on screen to distinguish them. +// Nothing here can delete anything: Scan writes nothing, and the live writer builds its own +// policy from the target. +// +// In practice a CLI scan also passes a nil mapper, so no document resolves to MappingFollowable +// and no drop is emitted regardless. This is the deliberate choice for the day that changes. +func FolderScanPlanPolicy() Policy { + return Policy{Sweep: SweepDropOrphans} +} + // ScanResult is the dry-run outcome: the built store, the acceptance decision, and // the full plan. It carries everything needed to render the human, JSON, and status // views without recomputation. diff --git a/internal/manifestanalyzer/scoped_plan_test.go b/internal/manifestanalyzer/scoped_plan_test.go index cd40edb2..ad2a4e84 100644 --- a/internal/manifestanalyzer/scoped_plan_test.go +++ b/internal/manifestanalyzer/scoped_plan_test.go @@ -21,7 +21,7 @@ func inScopeGroupResource(group, resource string) func(types.ResourceIdentifier) // left untouched even though they too have no desired counterpart. func TestBuildScopedPlan_SweepsOnlyTargetType(t *testing.T) { store := planStore(t) - plan := BuildScopedPlan(store, planFiles(), nil, Policy{}, inScopeGroupResource("apps", "deployments")) + plan := BuildScopedPlan(store, planFiles(), nil, convergingPolicy(), inScopeGroupResource("apps", "deployments")) counts := plan.Counts() if counts[PlanDropOrphan] != 1 || len(plan.Actions) != 1 { @@ -46,7 +46,7 @@ func TestBuildScopedPlan_SweepsOnlyTargetType(t *testing.T) { func TestBuildScopedPlan_ReconcileDropsInScopeOrphanKeepsSiblings(t *testing.T) { store := planStore(t) desired := []DesiredResource{desiredConfigMap("a")} - plan := BuildScopedPlan(store, planFiles(), desired, Policy{}, inScopeGroupResource("", "configmaps")) + plan := BuildScopedPlan(store, planFiles(), desired, convergingPolicy(), inScopeGroupResource("", "configmaps")) if got := plan.Counts()[PlanDropOrphan]; got != 1 { t.Fatalf("want exactly one drop (ConfigMap b), got %d (actions=%+v)", got, plan.Actions) @@ -66,8 +66,8 @@ func TestBuildScopedPlan_ReconcileDropsInScopeOrphanKeepsSiblings(t *testing.T) // whole-folder mark-and-sweep), so both share one set of safety guarantees. func TestBuildScopedPlan_AllInScopeEqualsBuildPlan(t *testing.T) { store := planStore(t) - full := BuildPlan(store, planFiles(), nil, Policy{}) - scoped := BuildScopedPlan(planStore(t), planFiles(), nil, Policy{}, allInScope) + full := BuildPlan(store, planFiles(), nil, convergingPolicy()) + scoped := BuildScopedPlan(planStore(t), planFiles(), nil, convergingPolicy(), allInScope) if len(full.Actions) != len(scoped.Actions) { t.Fatalf("action count differs: BuildPlan=%d allInScope=%d", len(full.Actions), len(scoped.Actions)) diff --git a/internal/manifestanalyzer/sweep_policy_test.go b/internal/manifestanalyzer/sweep_policy_test.go new file mode 100644 index 00000000..cb3b74cd --- /dev/null +++ b/internal/manifestanalyzer/sweep_policy_test.go @@ -0,0 +1,145 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "testing" + + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// convergingPolicy is the planning policy that MODELS FULL CONVERGENCE: it drops orphans. +// +// Every pre-existing planner test uses it, because the mark-and-sweep is what they were written +// to pin. It is named rather than inlined so those tests state which prune policy they model — +// the planner's own default retains, and a test that silently inherited that default would keep +// passing while asserting nothing about the sweep. +func convergingPolicy() Policy { return Policy{Sweep: SweepDropOrphans} } + +// TestSweepMode_OnlyDropExplicitlyDrops pins the direction of the zero value. It is the whole +// safety property in one assertion: a caller that forgets the field retains, and so does a value +// this build does not recognize. +func TestSweepMode_OnlyDropExplicitlyDrops(t *testing.T) { + for _, tc := range []struct { + mode SweepMode + want bool + }{ + {SweepDropOrphans, true}, + {SweepRetainOrphans, false}, + {SweepUnspecified, false}, + {SweepMode("shred-everything"), false}, + } { + if got := tc.mode.DropsOrphans(); got != tc.want { + t.Errorf("SweepMode(%q).DropsOrphans() = %v, want %v", tc.mode, got, tc.want) + } + } +} + +// TestBuildPlan_RetainDoesNotPlanTheDrop is the planner half of PR 5: with a retaining policy, +// the managed drops the same store produces under convergence are not merely filtered out of the +// applied actions — they are never planned. The assertion is deliberately on the WHOLE plan, not +// on a count of drop actions: a suppressed drop must not reach the action list, so it cannot +// reach the plan hash or the commit path either. +func TestBuildPlan_RetainDoesNotPlanTheDrop(t *testing.T) { + store := planStore(t) + + converging := BuildPlan(store, planFiles(), nil, convergingPolicy()) + if converging.Counts()[PlanDropOrphan] != 3 { + t.Fatalf("fixture drift: want 3 drops under convergence, got %v", converging.Counts()) + } + + retaining := BuildPlan(store, planFiles(), nil, Policy{Sweep: SweepRetainOrphans}) + if got := retaining.Counts()[PlanDropOrphan]; got != 0 { + t.Errorf("drop-orphan actions under a retaining policy = %d, want 0", got) + } + if len(retaining.Actions) != 0 { + t.Errorf("a retaining plan over an empty desired set must be empty, got %+v", retaining.Actions) + } + if retaining.RetainedOrphans != 3 { + t.Errorf("RetainedOrphans = %d, want 3 — the suppressed drops must still be counted", + retaining.RetainedOrphans) + } +} + +// TestBuildScopedPlan_RetainKeepsTheInScopeOrphan proves the gate applies to the SCOPED planner +// too, which is the one production actually reaches: every resync the watch layer issues carries +// a non-nil scope. Without this, gating only BuildPlan would leave the live sweep untouched. +func TestBuildScopedPlan_RetainKeepsTheInScopeOrphan(t *testing.T) { + store := planStore(t) + inScope := inScopeGroupResource("apps", "deployments") + + converging := BuildScopedPlan(store, planFiles(), nil, convergingPolicy(), inScope) + if converging.Counts()[PlanDropOrphan] != 1 { + t.Fatalf("fixture drift: want the Deployment dropped under convergence, got %v", converging.Counts()) + } + + retaining := BuildScopedPlan(store, planFiles(), nil, Policy{Sweep: SweepRetainOrphans}, inScope) + if len(retaining.Actions) != 0 { + t.Errorf("a retaining scoped plan must plan nothing, got %+v", retaining.Actions) + } + if retaining.RetainedOrphans != 1 { + t.Errorf("RetainedOrphans = %d, want 1", retaining.RetainedOrphans) + } +} + +// TestBuildPlan_RetainStillUpsertsAndSkips proves retention is scoped to DELETIONS and nothing +// else. A retaining policy that also suppressed creates or patches would quietly stop mirroring, +// which is a far worse failure than the stale document it is trying to avoid — and it would be +// invisible, because both look like "no commit". +func TestBuildPlan_RetainStillUpsertsAndSkips(t *testing.T) { + store := planStore(t) + // One in-sync resource, one drifted, one absent from Git, and the encrypted Secret. + desired := []DesiredResource{desiredConfigMap("a"), desiredDeployWeb(9), desiredSecret()} + + plan := BuildPlan(store, planFiles(), desired, Policy{Sweep: SweepRetainOrphans}) + + counts := plan.Counts() + if counts[PlanDropOrphan] != 0 { + t.Errorf("drop-orphan actions = %d, want 0 under retention", counts[PlanDropOrphan]) + } + if counts[PlanPatch]+counts[PlanReplace] == 0 { + t.Errorf("a drifted resource must still be edited under retention: %v", counts) + } + if counts[PlanSkip] == 0 { + t.Errorf("an encrypted document must still be reported as a skip under retention: %v", counts) + } + // The one ConfigMap the desired set does not name is retained, not dropped. + if plan.RetainedOrphans != 1 { + t.Errorf("RetainedOrphans = %d, want 1 (configmap b)", plan.RetainedOrphans) + } +} + +// TestFolderScanPlanPolicy_Converges pins the dry-run's deliberate choice. A scan has no +// GitTarget and therefore no prune policy to read; reporting the folder's orphans is the +// analysis the tool exists to produce, and nothing it does can delete anything. +func TestFolderScanPlanPolicy_Converges(t *testing.T) { + if !FolderScanPlanPolicy().Sweep.DropsOrphans() { + t.Error("an offline folder scan must report managed drops, not silently omit them") + } +} + +// TestBuildScopedPlan_RetainIsNotAnEmptyScope distinguishes the two gates that both end up +// suppressing a drop. inScope answers "is this document any of my business"; Sweep answers "may I +// delete the ones that are". Implementing retention as an empty scope predicate would be tempting +// and would pass the drop assertions above — but it erases the distinction, and with it the +// operator's only signal: an out-of-scope document is not this plan's concern and is counted +// nowhere, while a RETAINED one is a document this plan owns, considered, and deliberately kept. +func TestBuildScopedPlan_RetainIsNotAnEmptyScope(t *testing.T) { + store := planStore(t) + all := func(types.ResourceIdentifier) bool { return true } + + retaining := BuildScopedPlan(store, planFiles(), nil, Policy{Sweep: SweepRetainOrphans}, all) + if retaining.RetainedOrphans == 0 { + t.Error("a document in scope and deliberately kept must be counted as retained") + } + + outOfScope := BuildScopedPlan(store, planFiles(), nil, convergingPolicy(), + func(types.ResourceIdentifier) bool { return false }) + if len(outOfScope.Actions) != 0 { + t.Errorf("an empty scope reports nothing at all, got %+v", outOfScope.Actions) + } + if outOfScope.RetainedOrphans != 0 { + t.Errorf("an out-of-scope document is not a retained orphan; RetainedOrphans = %d", + outOfScope.RetainedOrphans) + } +} diff --git a/internal/telemetry/exporter.go b/internal/telemetry/exporter.go index d13a8212..2d36a743 100644 --- a/internal/telemetry/exporter.go +++ b/internal/telemetry/exporter.go @@ -35,6 +35,13 @@ var ( // ResyncSweepDeletesTotal counts managed documents deleted by mark-and-sweep // resyncs, labelled by the swept resource {group, version, resource}. ResyncSweepDeletesTotal metric.Int64Counter + // PruneRetainedDocumentsTotal counts managed documents a GitTarget's spec.prune.mode + // KEPT that a mark-and-sweep would otherwise have deleted, labelled by + // {prune_mode, gittarget_namespace, gittarget_name}. It is the retention twin of + // ResyncSweepDeletesTotal and the only numeric trace a suppressed drop leaves: such a + // drop produces no plan action, no commit, and no ResyncStats entry. A non-zero value + // is the configured behaviour, never a fault. + PruneRetainedDocumentsTotal metric.Int64Counter // TargetReconcileCompletedTotal counts completed watch recovery passes per // GitTarget: each increment marks either a streaming-snapshot resync applied on @@ -209,6 +216,7 @@ func registerCounters() error { {"gitopsreverser_objects_written_total", &ObjectsWrittenTotal}, {"gitopsreverser_commits_total", &CommitsTotal}, {"gitopsreverser_resync_sweep_deletes_total", &ResyncSweepDeletesTotal}, + {"gitopsreverser_prune_retained_documents_total", &PruneRetainedDocumentsTotal}, {"gitopsreverser_target_reconcile_completed_total", &TargetReconcileCompletedTotal}, {"gitopsreverser_resync_background_failures_total", &ResyncBackgroundFailuresTotal}, {"gitopsreverser_audit_events_total", &AuditEventsTotal}, diff --git a/internal/watch/event_router.go b/internal/watch/event_router.go index f48829bb..3c13a715 100644 --- a/internal/watch/event_router.go +++ b/internal/watch/event_router.go @@ -235,6 +235,11 @@ func (r *EventRouter) drainScopedResync( if r.WatchManager != nil { r.WatchManager.MarkTargetGitPathAccepted(gitDest) r.WatchManager.MarkTargetRenderFidelityScopeClean(gitDest, renderFidelityEpoch, key) + // Recorded for every applied resync, including the ones that retained nothing: zero + // is the converged signal and is only meaningful if it is published as actively as a + // non-zero count. + r.WatchManager.MarkTargetRetention( + gitDest, key, renderFidelityEpoch, result.Stats.PruneMode, result.Stats.Retained) } // Count an applied per-type RECONCILE as a completed GitTarget reconcile so the // per-pod counter advances after a restart — the drain signal the restart-reconcile diff --git a/internal/watch/manager.go b/internal/watch/manager.go index e42be4d2..f93f7040 100644 --- a/internal/watch/manager.go +++ b/internal/watch/manager.go @@ -24,6 +24,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/event" + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/git" "github.com/ConfigButler/gitops-reverser/internal/rulestore" "github.com/ConfigButler/gitops-reverser/internal/telemetry" @@ -200,6 +201,20 @@ type Manager struct { gitTargetUIDsMu sync.Mutex gitTargetUIDs map[string]string + // gitTargetPruneModes maps a GitTarget key to the effective spec.prune.mode of its LAST + // successful Declare. Unlike the UID and the source cluster this value is mutable, and it is + // remembered for exactly one reason: to detect the edge where an operator widens the policy to + // one that sweeps, which must force a fresh replay or the newly authorized cleanup never runs. + // See prune_declaration.go. Guarded by gitTargetPruneModesMu. + gitTargetPruneModesMu sync.Mutex + gitTargetPruneModes map[string]v1alpha3.PruneMode + + // targetRetention holds each GitTarget's per-scope retained-document counts, epoch-keyed so a + // scope that leaves the watch plan takes its count with it. Projected onto status.retention. + // See retention_rollup.go. Guarded by targetRetentionMu. + targetRetentionMu sync.Mutex + targetRetention map[string]targetRetentionState + // declaredGVRsMu guards declaredGVRs: the type-set each GitTarget last Declared. The watch-first // data plane reads it to drive the per-(GitTarget, type) watch set; re-declaring is idempotent. declaredGVRsMu sync.Mutex diff --git a/internal/watch/materialization.go b/internal/watch/materialization.go index e4142332..0523d3c8 100644 --- a/internal/watch/materialization.go +++ b/internal/watch/materialization.go @@ -5,6 +5,7 @@ package watch import ( "context" + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/types" ) @@ -14,10 +15,15 @@ import ( // captured here, the same capture-on-Declare pattern as the UID: because spec.clusterProviderRef // is immutable it is learned once and never changes, so there is no per-rule propagation and no // cross-rule disagreement window. +// +// pruneMode is (api/v1alpha3).GitTarget.EffectivePruneMode(). Unlike the other two it is mutable, +// and widening it to a sweeping mode forces a fresh replay — see prune_declaration.go for why the +// edge, and only that edge, has to be the trigger. func (m *Manager) DeclareForGitTarget( ctx context.Context, gitDest types.ResourceReference, clusterID string, + pruneMode v1alpha3.PruneMode, forceRecheck ...bool, ) error { // Capture the UID and the source cluster before starting watches: the data plane keys its @@ -25,12 +31,15 @@ func (m *Manager) DeclareForGitTarget( // cluster's context — neither of which the rule-derived watch tables carry. m.rememberGitTargetUID(gitDest) m.rememberGitTargetCluster(gitDest, clusterID) - force := len(forceRecheck) > 0 && forceRecheck[0] + force := (len(forceRecheck) > 0 && forceRecheck[0]) || m.pruneModeRequiresReplay(gitDest, pruneMode) if err := m.EnsureGitTargetWatches(ctx, gitDest, force); err != nil { m.Log.Info("watch-first declare skipped; surface not observable", "gitDest", gitDest.String(), "clusterID", describeCluster(clusterID), "err", err.Error()) return err } + // Only once the watches are actually in place: a failed declare must leave the pending force + // standing for the next reconcile rather than consuming it. + m.rememberGitTargetPruneMode(gitDest, pruneMode) return nil } @@ -40,6 +49,7 @@ func (m *Manager) ForgetGitTargetDeclaration(gitDest types.ResourceReference) { m.forgetGitTargetWatches(gitDest) m.forgetGitTargetUID(gitDest) m.forgetGitTargetCluster(gitDest) + m.forgetGitTargetPruneMode(gitDest) m.declaredGVRsMu.Lock() defer m.declaredGVRsMu.Unlock() delete(m.declaredGVRs, gitDest.String()) diff --git a/internal/watch/prune_declaration.go b/internal/watch/prune_declaration.go new file mode 100644 index 00000000..0e8dff83 --- /dev/null +++ b/internal/watch/prune_declaration.go @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: Apache-2.0 + +package watch + +import ( + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// A GitTarget's prune policy is the one piece of its write-relevant identity that is MUTABLE, and +// that mutability is why it is tracked here at all. +// +// Widening the policy to `always` means "converge this mirror", but nothing in the data plane +// notices on its own: the watch set is keyed by what is being watched (GVR, namespace, operations) +// and a prune edit changes none of it, so the watches are left alone. The only production path +// that enqueues a resync is a fresh replay, and a reconnect resumes from its durable cursor rather +// than replaying — so a healthy, quiet target could sit under `always` indefinitely without ever +// sweeping the orphans the operator declared `always` to remove. Declaring the mode here turns the +// change itself into the trigger. +// +// Deliberately one-directional. Only a change INTO a sweeping mode forces the replay: +// +// - `onEvent`/`never` -> `always` needs a snapshot to sweep against, and has none; +// - `always` -> `onEvent`/`never` needs nothing. It takes effect on the next write by itself, and +// forcing a replay would tear down every one of the target's streams at the exact moment an +// operator is trying to STOP something from happening. Tightening a deletion policy is what an +// operator reaches for during an incident; it must be the cheap, quiet direction. +// +// It is also edge-triggered, not level-triggered: the GitTarget controller re-declares on every +// steady requeue, so forcing whenever the mode *is* `always` would rebuild the watch set forever. + +// pruneModeRequiresReplay reports whether declaring mode for gitDest must force a fresh replay, +// which is the only thing that enqueues the resync the new policy authorizes. +// +// A target with no remembered mode — first declare, or the first after a restart — never forces: +// there is no watch set to replace, and the declare that follows opens one whose first session +// replays anyway. +func (m *Manager) pruneModeRequiresReplay(gitDest types.ResourceReference, mode v1alpha3.PruneMode) bool { + m.gitTargetPruneModesMu.Lock() + defer m.gitTargetPruneModesMu.Unlock() + previous, known := m.gitTargetPruneModes[gitDest.Key()] + if !known { + return false + } + return previous != mode.OrDefault() && mode.SweepsOrphans() +} + +// rememberGitTargetPruneMode records the mode a Declare succeeded under. +// +// Called only AFTER the watches are in place. A declare that fails leaves the previous value +// standing, so the pending force survives to the next reconcile instead of being consumed by an +// attempt that never reached the data plane — the mode is remembered as "what the running watches +// were built for", not as "what was last requested". +func (m *Manager) rememberGitTargetPruneMode(gitDest types.ResourceReference, mode v1alpha3.PruneMode) { + m.gitTargetPruneModesMu.Lock() + defer m.gitTargetPruneModesMu.Unlock() + if m.gitTargetPruneModes == nil { + m.gitTargetPruneModes = map[string]v1alpha3.PruneMode{} + } + m.gitTargetPruneModes[gitDest.Key()] = mode.OrDefault() +} + +// forgetGitTargetPruneMode drops a deleted GitTarget's declared mode. The value describes the +// watch set the mode was declared for, so once that set is torn down it describes nothing: keeping +// it leaks an entry per deleted GitTarget, and makes a same-name recreation compute its force flag +// against a predecessor's policy. That is harmless today — a recreation has no watch set to +// replace, so its first declare replays regardless — but it is a claim about state that is gone. +func (m *Manager) forgetGitTargetPruneMode(gitDest types.ResourceReference) { + m.gitTargetPruneModesMu.Lock() + defer m.gitTargetPruneModesMu.Unlock() + delete(m.gitTargetPruneModes, gitDest.Key()) +} diff --git a/internal/watch/prune_declaration_test.go b/internal/watch/prune_declaration_test.go new file mode 100644 index 00000000..e782ac1a --- /dev/null +++ b/internal/watch/prune_declaration_test.go @@ -0,0 +1,173 @@ +// SPDX-License-Identifier: Apache-2.0 + +package watch + +import ( + "context" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/watch" + + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// TestPruneModeRequiresReplay_OnlyOnTheEdgeIntoASweepingMode is the whole contract in one table. +// +// The two rows that matter most are the ones that must stay FALSE: `always` re-declared unchanged +// (a level trigger there rebuilds the watch set on every steady requeue, forever), and `always` +// tightened to `onEvent` (that direction needs no snapshot, and tearing down a target's streams +// while an operator is trying to stop deletions is the opposite of what they asked for). +func TestPruneModeRequiresReplay_OnlyOnTheEdgeIntoASweepingMode(t *testing.T) { + gitDest := types.NewResourceReference("target", "tenant") + + for _, tc := range []struct { + name string + previous *v1alpha3.PruneMode + declared v1alpha3.PruneMode + want bool + why string + }{ + { + name: "first declare of a sweeping target", previous: nil, + declared: v1alpha3.PruneAlways, want: false, + why: "there is no watch set to replace; the declare that follows replays anyway", + }, + { + name: "widened from the default", previous: mode(v1alpha3.PruneOnEvent), + declared: v1alpha3.PruneAlways, want: true, + why: "the newly authorized sweep needs a snapshot, and only a replay produces one", + }, + { + name: "widened from never", previous: mode(v1alpha3.PruneNever), + declared: v1alpha3.PruneAlways, want: true, + }, + { + name: "widened from a legacy target that stored nothing", previous: mode(""), + declared: v1alpha3.PruneAlways, want: true, + why: "an omitted policy is onEvent, so this is the same edge as widening from the default", + }, + { + name: "unchanged sweeping mode", previous: mode(v1alpha3.PruneAlways), + declared: v1alpha3.PruneAlways, want: false, + why: "level-triggering here rebuilds the watch set on every steady requeue", + }, + { + name: "tightened to the default", previous: mode(v1alpha3.PruneAlways), + declared: v1alpha3.PruneOnEvent, want: false, + why: "a tightening applies at the next write by itself and must not churn streams", + }, + { + name: "tightened to never", previous: mode(v1alpha3.PruneAlways), + declared: v1alpha3.PruneNever, want: false, + }, + { + name: "changed between two retaining modes", previous: mode(v1alpha3.PruneNever), + declared: v1alpha3.PruneOnEvent, want: false, + why: "neither mode sweeps, so no resync is newly authorized", + }, + { + name: "an omitted policy declared over the default", previous: mode(v1alpha3.PruneOnEvent), + declared: "", want: false, + why: "the effective modes are equal; the raw values are not", + }, + } { + t.Run(tc.name, func(t *testing.T) { + m := &Manager{} + if tc.previous != nil { + m.rememberGitTargetPruneMode(gitDest, *tc.previous) + } + + assert.Equal(t, tc.want, m.pruneModeRequiresReplay(gitDest, tc.declared), tc.why) + }) + } +} + +// TestPruneModeDeclaration_IsPerGitTarget guards the obvious sharing bug: one target widening its +// policy must not force a replay of an unrelated target that never changed. +func TestPruneModeDeclaration_IsPerGitTarget(t *testing.T) { + m := &Manager{} + widened := types.NewResourceReference("widened", "tenant-a") + untouched := types.NewResourceReference("untouched", "tenant-b") + + m.rememberGitTargetPruneMode(widened, v1alpha3.PruneOnEvent) + m.rememberGitTargetPruneMode(untouched, v1alpha3.PruneOnEvent) + + assert.True(t, m.pruneModeRequiresReplay(widened, v1alpha3.PruneAlways)) + assert.False(t, m.pruneModeRequiresReplay(untouched, v1alpha3.PruneOnEvent)) +} + +// TestForgetGitTargetPruneMode_DropsTheDeclaration keeps the map bounded by live GitTargets. The +// remembered mode describes a running watch set; once ForgetGitTargetDeclaration tears that set +// down, an entry left behind is a claim about state that no longer exists. +func TestForgetGitTargetPruneMode_DropsTheDeclaration(t *testing.T) { + m := &Manager{} + gitDest := types.NewResourceReference("target", "tenant") + m.rememberGitTargetPruneMode(gitDest, v1alpha3.PruneOnEvent) + require.True(t, m.pruneModeRequiresReplay(gitDest, v1alpha3.PruneAlways), + "precondition: the declaration is remembered") + + m.forgetGitTargetPruneMode(gitDest) + + assert.False(t, m.pruneModeRequiresReplay(gitDest, v1alpha3.PruneAlways), + "a forgotten target has no previous mode, so nothing to compare against") + assert.Empty(t, m.gitTargetPruneModes) +} + +// TestReplaceGitTargetWatches_ForceReplaysAnUnchangedSet is the mechanism R1 relies on, asserted +// end to end at the watch layer: a widened prune policy leaves the watch SPECS identical (they +// describe what is watched, not what may be deleted), so without the force flag the replacement is +// a no-op and no resync is ever enqueued. With it, the scope replays — and a replay is the only +// production path that enqueues the sweep the new policy authorizes. +func TestReplaceGitTargetWatches_ForceReplaysAnUnchangedSet(t *testing.T) { + gitDest := types.NewResourceReference("target", "default") + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + opened := make(chan openedWatch, 4) + manager := &Manager{ + Log: logr.Discard(), + WatchCursorStore: &fakeWatchCursorStore{rv: "41", ok: true}, + targetWatchOpen: func( + _ context.Context, + _ schema.GroupVersionResource, + namespace string, + opts metav1.ListOptions, + ) (watch.Interface, error) { + fw := watch.NewFake() + opened <- openedWatch{namespace: namespace, opts: opts, watch: fw} + return fw, nil + }, + } + manager.rememberGitTargetUID(gitDest.WithUID("uid-1")) + + table := WatchedTypeTable{ + GitDest: gitDest, + Types: []WatchedType{{ + GVR: configmapsGVR, + NamespaceOps: map[string]OperationSet{"apps": {"CREATE": struct{}{}}}, + }}, + } + require.NoError(t, manager.replaceGitTargetWatches(ctx, table)) + receiveOpenedWatch(t, opened) + + // The negative control: re-declaring the same specs without the force flag changes nothing, + // which is exactly why a prune-mode edit needs one. + require.NoError(t, manager.replaceGitTargetWatches(ctx, table)) + assertNoOpenedWatch(t, opened) + + require.NoError(t, manager.replaceGitTargetWatches(ctx, table, true)) + + forced := receiveOpenedWatch(t, opened) + assert.True(t, *forced.opts.SendInitialEvents, + "a forced replacement must replay, or the widened policy has no snapshot to sweep against") + assert.Empty(t, forced.opts.ResourceVersion, + "a forced replay must not resume from the durable cursor, which would enqueue no resync") +} + +func mode(m v1alpha3.PruneMode) *v1alpha3.PruneMode { return &m } diff --git a/internal/watch/retention_rollup.go b/internal/watch/retention_rollup.go new file mode 100644 index 00000000..36753312 --- /dev/null +++ b/internal/watch/retention_rollup.go @@ -0,0 +1,123 @@ +// SPDX-License-Identifier: Apache-2.0 + +package watch + +import ( + "time" + + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// A suppressed sweep is invisible by construction — no plan action, no commit, no other stat — +// which is exactly right for the write path and leaves an operator unable to tell a CONVERGED +// mirror from one that is deliberately retaining. This roll-up is the answer: each scope's resync +// reports what its policy kept, the counts are summed per GitTarget, and the controller projects +// the sum onto status.retention. +// +// It is an observation, never a gate. Nothing here may fail a reconciliation or move a condition. + +// RetentionSummary is the per-GitTarget roll-up the controller projects onto status. +type RetentionSummary struct { + // Reported distinguishes "no resync has reported yet" from "a resync reported zero". Both are + // legitimate states and they mean opposite things: the first is unknown, the second is the + // converged signal, which is half the value of publishing this at all. + Reported bool + // Mode is the effective spec.prune.mode the most recent contributing resync ran under. It + // travels WITH the count rather than being read from the spec at projection time, so the two + // always describe the same observation — a target switched to `always` does not briefly + // publish `always` beside a count that a retaining policy produced. + Mode v1alpha3.PruneMode + // RetainedDocuments is the sum over the target's currently tracked scopes. + RetainedDocuments int + // ObservedTime is when the most recent contributing resync reported. + ObservedTime time.Time +} + +// targetRetentionState is one GitTarget's per-scope counts, valid for a single watch epoch. +type targetRetentionState struct { + epoch uint64 + scopes map[targetWatchKey]int + mode v1alpha3.PruneMode + observed time.Time +} + +func (s targetRetentionState) total() int { + sum := 0 + for _, retained := range s.scopes { + sum += retained + } + return sum +} + +// MarkTargetRetention records what one scope's resync retained. +// +// Scope lifecycle is handled by the EPOCH rather than by eviction, reusing the watch epoch +// RenderFidelityGate already defines: records carry the epoch they were produced under, a new +// epoch replaces the whole per-scope map, and a record from an older epoch is dropped. A scope +// that leaves the watch plan therefore takes its count with it at the next declaration, with no +// per-key deletion logic to get wrong — and a stale in-flight reply from a cancelled watch cannot +// resurrect a count for a scope this target no longer has. +// +// Zero is recorded as actively as any other number: it is the converged signal. +func (m *Manager) MarkTargetRetention( + gitDest types.ResourceReference, + key targetWatchKey, + epoch uint64, + mode v1alpha3.PruneMode, + retained int, +) { + m.targetRetentionMu.Lock() + if m.targetRetention == nil { + m.targetRetention = map[string]targetRetentionState{} + } + state, had := m.targetRetention[gitDest.Key()] + if had && epoch < state.epoch { + m.targetRetentionMu.Unlock() + return + } + // Captured BEFORE the epoch reset below, so "changed" compares what an operator would see on + // status, not what the internal map did. A new epoch that re-reports the same total is not a + // change to them, and enqueueing for it would make every watch-set replacement reconcile twice. + priorTotal, priorMode := state.total(), state.mode + if !had || epoch > state.epoch { + state = targetRetentionState{epoch: epoch, scopes: map[targetWatchKey]int{}} + } + state.scopes[key] = retained + state.mode = mode.OrDefault() + state.observed = time.Now() + m.targetRetention[gitDest.Key()] = state + changed := !had || state.total() != priorTotal || state.mode != priorMode + m.targetRetentionMu.Unlock() + + // Prompt a status refresh on a CHANGE only. Without it the first appearance of a retention + // would wait for the steady requeue (minutes), which is too long for a signal an operator + // consults before flipping a target to `always`; with it on every report, a steadily retaining + // target would enqueue on every resync of every scope forever. + if changed { + m.enqueueGitPathChange(gitDest) + } +} + +// RetentionForGitTarget returns the roll-up across the target's currently tracked scopes. +func (m *Manager) RetentionForGitTarget(gitDest types.ResourceReference) RetentionSummary { + m.targetRetentionMu.Lock() + defer m.targetRetentionMu.Unlock() + state, had := m.targetRetention[gitDest.Key()] + if !had { + return RetentionSummary{} + } + return RetentionSummary{ + Reported: true, + Mode: state.mode, + RetainedDocuments: state.total(), + ObservedTime: state.observed, + } +} + +// forgetTargetRetention drops a deleted GitTarget's roll-up. +func (m *Manager) forgetTargetRetention(gitDest types.ResourceReference) { + m.targetRetentionMu.Lock() + defer m.targetRetentionMu.Unlock() + delete(m.targetRetention, gitDest.Key()) +} diff --git a/internal/watch/retention_rollup_test.go b/internal/watch/retention_rollup_test.go new file mode 100644 index 00000000..d6dadf21 --- /dev/null +++ b/internal/watch/retention_rollup_test.go @@ -0,0 +1,158 @@ +// SPDX-License-Identifier: Apache-2.0 + +package watch + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/runtime/schema" + + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +var ( + retentionCMScope = targetWatchKey{GVR: schema.GroupVersionResource{Version: "v1", Resource: "configmaps"}} + retentionSecretScope = targetWatchKey{GVR: schema.GroupVersionResource{Version: "v1", Resource: "secrets"}} +) + +// TestRetentionRollup_SumsEveryScope: a resync fires per type and per namespace within a type, so +// the number an operator needs is the target-wide total, not whichever scope reported last. +func TestRetentionRollup_SumsEveryScope(t *testing.T) { + m := &Manager{} + gitDest := types.NewResourceReference("acme", "tenant-acme") + + m.MarkTargetRetention(gitDest, retentionCMScope, 1, v1alpha3.PruneOnEvent, 2) + m.MarkTargetRetention(gitDest, retentionSecretScope, 1, v1alpha3.PruneOnEvent, 3) + + summary := m.RetentionForGitTarget(gitDest) + assert.True(t, summary.Reported) + assert.Equal(t, 5, summary.RetainedDocuments) + assert.Equal(t, v1alpha3.PruneOnEvent, summary.Mode) + assert.False(t, summary.ObservedTime.IsZero()) +} + +// TestRetentionRollup_ZeroIsRecordedAsActivelyAsAnyOtherCount is the likeliest regression in this +// whole projection. "Converged" and "retaining" are the two states the field exists to separate, +// and only publishing non-zero counts would make the first indistinguishable from a stale reading. +func TestRetentionRollup_ZeroIsRecordedAsActivelyAsAnyOtherCount(t *testing.T) { + m := &Manager{} + gitDest := types.NewResourceReference("acme", "tenant-acme") + m.MarkTargetRetention(gitDest, retentionCMScope, 1, v1alpha3.PruneOnEvent, 4) + require.Equal(t, 4, m.RetentionForGitTarget(gitDest).RetainedDocuments) + + // The operator removed the stale documents by hand; the next resync finds nothing to retain. + m.MarkTargetRetention(gitDest, retentionCMScope, 1, v1alpha3.PruneOnEvent, 0) + + summary := m.RetentionForGitTarget(gitDest) + assert.True(t, summary.Reported, "a reported zero is still a report") + assert.Zero(t, summary.RetainedDocuments) +} + +// TestRetentionRollup_UnreportedIsNotZero keeps "nobody has told us yet" distinguishable from "a +// resync ran and found nothing". The controller projects the first as an absent status block. +func TestRetentionRollup_UnreportedIsNotZero(t *testing.T) { + m := &Manager{} + + summary := m.RetentionForGitTarget(types.NewResourceReference("acme", "tenant-acme")) + + assert.False(t, summary.Reported) + assert.Zero(t, summary.RetainedDocuments) +} + +// TestRetentionRollup_ANewEpochDropsScopesThatLeftThePlan is the eviction property, and the reason +// this reuses the watch epoch instead of maintaining its own scope lifecycle: when a type stops +// being watched, its count has to disappear, or the roll-up only ever grows and becomes a lie. +func TestRetentionRollup_ANewEpochDropsScopesThatLeftThePlan(t *testing.T) { + m := &Manager{} + gitDest := types.NewResourceReference("acme", "tenant-acme") + m.MarkTargetRetention(gitDest, retentionCMScope, 1, v1alpha3.PruneOnEvent, 2) + m.MarkTargetRetention(gitDest, retentionSecretScope, 1, v1alpha3.PruneOnEvent, 3) + require.Equal(t, 5, m.RetentionForGitTarget(gitDest).RetainedDocuments) + + // Secrets left the watch plan; the new declaration replays only ConfigMaps. + m.MarkTargetRetention(gitDest, retentionCMScope, 2, v1alpha3.PruneOnEvent, 2) + + assert.Equal(t, 2, m.RetentionForGitTarget(gitDest).RetainedDocuments, + "a scope that left the plan must take its count with it") +} + +// TestRetentionRollup_StaleEpochIsIgnored is the property inherited from RenderFidelityGate: a +// cancelled watch's in-flight reply arrives after the new declaration and must not resurrect a +// count for a scope this target no longer has. +func TestRetentionRollup_StaleEpochIsIgnored(t *testing.T) { + m := &Manager{} + gitDest := types.NewResourceReference("acme", "tenant-acme") + m.MarkTargetRetention(gitDest, retentionCMScope, 2, v1alpha3.PruneOnEvent, 1) + + m.MarkTargetRetention(gitDest, retentionSecretScope, 1, v1alpha3.PruneOnEvent, 99) + + assert.Equal(t, 1, m.RetentionForGitTarget(gitDest).RetainedDocuments, + "a record from a superseded epoch must not contribute") +} + +// TestRetentionRollup_ReportsTheModeTheCountWasProducedUnder keeps the pair self-consistent. The +// mode travels with the count precisely so status cannot show a freshly declared `always` beside a +// number that a retaining policy produced. +func TestRetentionRollup_ReportsTheModeTheCountWasProducedUnder(t *testing.T) { + m := &Manager{} + gitDest := types.NewResourceReference("acme", "tenant-acme") + + // A legacy GitTarget stores no mode at all; the roll-up must report the effective one. + m.MarkTargetRetention(gitDest, retentionCMScope, 1, "", 2) + + assert.Equal(t, v1alpha3.PruneOnEvent, m.RetentionForGitTarget(gitDest).Mode) +} + +// TestRetentionRollup_IsPerGitTarget guards the sharing bug a single map invites. +func TestRetentionRollup_IsPerGitTarget(t *testing.T) { + m := &Manager{} + acme := types.NewResourceReference("acme", "tenant-acme") + other := types.NewResourceReference("other", "tenant-other") + + m.MarkTargetRetention(acme, retentionCMScope, 1, v1alpha3.PruneOnEvent, 7) + + assert.Equal(t, 7, m.RetentionForGitTarget(acme).RetainedDocuments) + assert.False(t, m.RetentionForGitTarget(other).Reported) +} + +// TestRetentionRollup_ForgottenTargetReportsNothing: a deleted GitTarget's roll-up must go with it, +// so a recreated target under the same name starts from "not reported" rather than inheriting a +// predecessor's count. +func TestRetentionRollup_ForgottenTargetReportsNothing(t *testing.T) { + m := &Manager{} + gitDest := types.NewResourceReference("acme", "tenant-acme") + m.MarkTargetRetention(gitDest, retentionCMScope, 1, v1alpha3.PruneOnEvent, 3) + + m.forgetTargetRetention(gitDest) + + assert.False(t, m.RetentionForGitTarget(gitDest).Reported) +} + +// TestRetentionRollup_EnqueuesOnChangeOnly: the first appearance of a retention must not wait out +// the steady requeue — an operator consults this before flipping a target to `always`. A steady +// state must not enqueue at all, or a target that is deliberately retaining would re-reconcile on +// every resync of every scope, forever. +func TestRetentionRollup_EnqueuesOnChangeOnly(t *testing.T) { + m := &Manager{} + events := m.GitPathEvents() + gitDest := types.NewResourceReference("acme", "tenant-acme") + + m.MarkTargetRetention(gitDest, retentionCMScope, 1, v1alpha3.PruneOnEvent, 2) + require.Len(t, events, 1, "the first report is a change: nothing was known before") + + m.MarkTargetRetention(gitDest, retentionCMScope, 1, v1alpha3.PruneOnEvent, 2) + assert.Len(t, events, 1, "an unchanged roll-up must not enqueue") + + m.MarkTargetRetention(gitDest, retentionCMScope, 1, v1alpha3.PruneOnEvent, 0) + assert.Len(t, events, 2, "returning to converged is a change an operator is waiting for") + + m.MarkTargetRetention(gitDest, retentionCMScope, 1, v1alpha3.PruneAlways, 0) + assert.Len(t, events, 3, "the mode changing is a change even when the count does not") + + m.MarkTargetRetention(gitDest, retentionCMScope, 2, v1alpha3.PruneAlways, 0) + assert.Len(t, events, 3, + "a new epoch that re-reports the same roll-up is not a change an operator can see") +} diff --git a/internal/watch/target_watch.go b/internal/watch/target_watch.go index 40006c05..ea5d54bb 100644 --- a/internal/watch/target_watch.go +++ b/internal/watch/target_watch.go @@ -225,6 +225,7 @@ func (m *Manager) forgetGitTargetWatches(gitDest types.ResourceReference) { m.dropTargetStreamStateLocked(gitDest) m.dropTargetGitPathAcceptanceLocked(gitDest) m.dropTargetRenderFidelityLocked(gitDest) + m.forgetTargetRetention(gitDest) } func targetWatchSpecs(table WatchedTypeTable) map[targetWatchKey]string { diff --git a/pkg/manifestanalyzer/folder.go b/pkg/manifestanalyzer/folder.go index 3db251e9..527d30a3 100644 --- a/pkg/manifestanalyzer/folder.go +++ b/pkg/manifestanalyzer/folder.go @@ -150,6 +150,7 @@ func (r FolderReport) WriteJSON(w io.Writer) error { func folderScanPolicy() internalanalyzer.ScanPolicy { return internalanalyzer.ScanPolicy{ Acceptance: internalanalyzer.AcceptancePolicy{Allowlist: internalanalyzer.DefaultAllowlist()}, + Plan: internalanalyzer.FolderScanPlanPolicy(), } } diff --git a/test/e2e/prune_mode_e2e_test.go b/test/e2e/prune_mode_e2e_test.go new file mode 100644 index 00000000..aa223b40 --- /dev/null +++ b/test/e2e/prune_mode_e2e_test.go @@ -0,0 +1,417 @@ +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "fmt" + "os" + "path" + "path/filepath" + "strconv" + "strings" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// This spec is the end-to-end proof for GitTarget.spec.prune.mode (see +// docs/design/watchrule-source-namespace/pr5-gittarget-deletion-safety.md). It exercises the two +// deletion paths SEPARATELY, because the whole design rests on them being independently +// controlled — `onEvent`, the effective default, differs from `always` on exactly one of them. +// +// Every assertion here is paired with a BARRIER: a negative claim ("the file is still there") is +// worthless on its own, since it also passes when the pipeline is simply asleep. So each retention +// assertion is made only after a co-resident GitTarget, fed by the SAME cluster event or the SAME +// resync trigger, has been observed to act. That is what makes "it did not delete" mean "it +// decided not to delete" rather than "nothing happened yet". +var _ = Describe("Manager GitTarget prune policy", Label("manager"), Ordered, func() { + const ( + providerName = "gitprovider-prune" + + defaultTarget = "prune-default-target" + neverTarget = "prune-never-target" + alwaysTarget = "prune-always-target" + + defaultPath = "e2e/prune-default" + neverPath = "e2e/prune-never" + alwaysPath = "e2e/prune-always" + + defaultRule = "prune-default-rule" + neverRule = "prune-never-rule" + alwaysRule = "prune-always-rule" + + // Seeded by the sweep spec and RETAINED by the default target, which is what the two specs + // after it observe: first on status, then being swept once the policy is widened. Shared + // here rather than repeated, so the coupling between those specs is visible. + orphanName = "prune-orphan" + ) + + var ( + testNs string + pruneRepo *RepoArtifacts + ) + + BeforeAll(func() { + By("creating the prune-policy test namespace") + testNs = testNamespaceFor("manager-prune") + _, _ = kubectlRun("create", "namespace", testNs) // idempotent; ignore AlreadyExists + + By("setting up the Gitea repo and credentials") + pruneRepo = SetupRepo( + resolveE2EContext(), + testNs, + fmt.Sprintf("e2e-manager-prune-%d", GinkgoRandomSeed()), + ) + _, err := kubectlRunInNamespace(testNs, "apply", "-f", pruneRepo.SecretsYAML) + Expect(err).NotTo(HaveOccurred(), "failed to apply git secrets to test namespace") + + createReadyGitProvider(providerName, testNs, pruneRepo.GitSecretHTTP, pruneRepo.RepoURLHTTP) + + By("creating three GitTargets in one repo, one per prune policy") + // The default target declares NO prune block at all — the shape an existing cluster + // holds after upgrading into this release. Its behaviour must come from + // EffectivePruneMode, not from a CRD default it never received. + applyPruneGitTarget(defaultTarget, testNs, providerName, defaultPath, "") + applyPruneGitTarget(neverTarget, testNs, providerName, neverPath, "Never") + applyPruneGitTarget(alwaysTarget, testNs, providerName, alwaysPath, "Always") + for _, name := range []string{defaultTarget, neverTarget, alwaysTarget} { + verifyResourceCondition("gittarget", name, testNs, "Validated", "True", "OK", "") + } + + By("each target watches ConfigMaps in this namespace") + applyIsolationWatchRule(defaultRule, testNs, defaultTarget, `"configmaps"`) + applyIsolationWatchRule(neverRule, testNs, neverTarget, `"configmaps"`) + applyIsolationWatchRule(alwaysRule, testNs, alwaysTarget, `"configmaps"`) + for _, name := range []string{defaultRule, neverRule, alwaysRule} { + verifyResourceStatus("watchrule", name, testNs, "True", "Ready", "") + } + + By("waiting for every target's ConfigMap stream to be live before any event is created") + for _, name := range []string{defaultTarget, neverTarget, alwaysTarget} { + waitForStreamsRunning(name, testNs) + } + }) + + AfterAll(func() { + cleanupNamespace(testNs) + }) + + SetDefaultEventuallyTimeout(60 * time.Second) + SetDefaultEventuallyPollingInterval(2 * time.Second) + + // The API contract, checked against the live apiserver rather than against the Go types: the + // schema must default a declared-but-empty prune block, must leave an omitted one absent + // (which is exactly why EffectivePruneMode exists), and must reject a value outside the enum. + It("defaults, omits, and validates spec.prune.mode at the API server", func() { + By("an omitted prune block stays omitted — Kubernetes does not default an absent object") + Expect(pruneModeOf(defaultTarget, testNs)).To(BeEmpty(), + "a GitTarget with no prune block must persist without one, so the omitted case is real") + + By("a declared-but-empty prune block is defaulted to OnEvent by the schema") + declaredEmpty := "prune-defaulted-target" + Expect(applyRawGitTarget(declaredEmpty, testNs, providerName, "e2e/prune-defaulted", " prune: {}")). + To(Succeed(), "a GitTarget declaring an empty prune block must be accepted") + Expect(pruneModeOf(declaredEmpty, testNs)).To(Equal("OnEvent"), + "the CRD default must write onEvent into a newly created object") + + By("a mode outside the enum is rejected") + err := applyRawGitTarget("prune-bogus-target", testNs, providerName, "e2e/prune-bogus", + " prune:\n mode: sometimes") + Expect(err).To(HaveOccurred(), "an unsupported prune mode must be rejected by the schema") + Expect(err.Error()).To(ContainSubstring("mode"), + "the rejection must name the offending field") + }) + + // PATH 1 — the explicit source DELETE. `never` is the only mode that suppresses it, so this + // is the assertion that distinguishes `never` from the default; a test that only covered the + // sweep would pass for both. + It("mirrors an observed DELETE under the default and retains it under never", func() { + const cmName = "prune-delete-me" + + By("creating a watched ConfigMap and waiting for both targets to mirror it") + applyIsolationConfigMap(cmName, testNs) + defaultFile := pruneConfigMapPath(defaultPath, testNs, cmName) + neverFile := pruneConfigMapPath(neverPath, testNs, cmName) + waitForPruneFile(pruneRepo, defaultFile, true) + waitForPruneFile(pruneRepo, neverFile, true) + + By("deleting the ConfigMap from the cluster") + _, err := kubectlRunInNamespace(testNs, "delete", "configmap", cmName) + Expect(err).NotTo(HaveOccurred(), "deleting the watched ConfigMap should succeed") + + // The barrier: the default target consumes the same DELETE event from the same watch and + // removes its copy. Once that is observed, the event has demonstrably reached the writer, + // so the never target's surviving copy is a decision rather than a pending write. + By("the default target (effective mode onEvent) removes its copy") + waitForPruneFile(pruneRepo, defaultFile, false) + + By("the never target keeps its copy, and keeps it") + Consistently(func(g Gomega) { + pullLatestRepoState(g, pruneRepo.CheckoutDir) + _, statErr := os.Stat(filepath.Join(pruneRepo.CheckoutDir, neverFile)) + g.Expect(statErr).NotTo(HaveOccurred(), + "prune.mode: Never must not mirror a source DELETE") + }, 15*time.Second, 3*time.Second).Should(Succeed()) + }) + + // PATH 2 — the inferred mark-and-sweep. This is the path PR 5 exists for: a document in Git + // that the cluster has no counterpart for is a DELETION ONLY IF the desired snapshot is + // trusted, and a narrowed snapshot is exactly what a scope mistake produces. + // + // The orphan is seeded by pushing straight into the repo, because that is the only way to + // manufacture "Git has a managed document the cluster does not" without also making the + // cluster emit an event about it. + It("sweeps an orphaned document only when prune.mode is Always", func() { + By("seeding an orphaned ConfigMap manifest into both the always and default folders") + alwaysOrphan := pruneConfigMapPath(alwaysPath, testNs, orphanName) + defaultOrphan := pruneConfigMapPath(defaultPath, testNs, orphanName) + seedOrphanManifests(pruneRepo, testNs, map[string]string{ + alwaysOrphan: orphanConfigMapYAML(orphanName, testNs), + defaultOrphan: orphanConfigMapYAML(orphanName, testNs), + }) + + // Toggling ConfigMaps OFF and back ON is what makes this deterministic: it tears the + // configmaps stream down and re-establishes it, and a re-established stream replays and + // issues a resync scoped to exactly (configmaps, this namespace) — the scope the seeded + // orphan lives in. Merely ADDING an unrelated type would churn the rule without + // guaranteeing the configmaps stream restarts, and the sweep would never be attempted. + By("toggling ConfigMaps off and back on to force a scoped replay resync") + applyIsolationWatchRule(alwaysRule, testNs, alwaysTarget, `"services"`) + applyIsolationWatchRule(defaultRule, testNs, defaultTarget, `"services"`) + applyIsolationWatchRule(alwaysRule, testNs, alwaysTarget, `"configmaps"`) + applyIsolationWatchRule(defaultRule, testNs, defaultTarget, `"configmaps"`) + waitForStreamsRunning(alwaysTarget, testNs) + waitForStreamsRunning(defaultTarget, testNs) + + // The barrier: the always target's resync reaches the same folder with the same desired + // snapshot as the default target's. Observing its sweep proves a resync ran and that the + // seeded document was in its scope — without which "still present" would prove nothing. + By("the always target sweeps the orphan") + waitForPruneFile(pruneRepo, alwaysOrphan, false) + + By("the default target (effective mode onEvent) keeps it") + Consistently(func(g Gomega) { + pullLatestRepoState(g, pruneRepo.CheckoutDir) + _, statErr := os.Stat(filepath.Join(pruneRepo.CheckoutDir, defaultOrphan)) + g.Expect(statErr).NotTo(HaveOccurred(), + "the default prune mode must never infer a deletion from a desired snapshot") + }, 15*time.Second, 3*time.Second).Should(Succeed()) + + By("and the default target is still mirroring — retention is not a stalled pipeline") + const proofName = "prune-still-live" + applyIsolationConfigMap(proofName, testNs) + waitForPruneFile(pruneRepo, pruneConfigMapPath(defaultPath, testNs, proofName), true) + }) + + // A suppressed sweep leaves no action, no commit, and no stat — deliberately, so a retention is + // indistinguishable from the event never arriving. status.retention is the one place it becomes + // visible, and this asserts BOTH of its states from the same seeded orphan: the default target + // reports what it kept, while the co-resident always target reports zero. Zero is the converged + // signal, and it only means anything if it is published as actively as a non-zero count. + It("reports retained documents, and convergence, on GitTarget status", func() { + By("the default target reports the documents its policy kept") + Eventually(func(g Gomega) { + g.Expect(retainedDocumentsOf(g, defaultTarget, testNs)).To(BeNumerically(">", 0), + "the orphan the previous spec retained must be visible on status") + g.Expect(retentionModeOf(g, defaultTarget, testNs)).To(Equal("OnEvent"), + "status must report the EFFECTIVE mode; this target stores no prune block at all") + }).Should(Succeed()) + + By("the always target reports zero — it converged rather than never having reported") + Eventually(func(g Gomega) { + g.Expect(retainedDocumentsOf(g, alwaysTarget, testNs)).To(Equal(0)) + g.Expect(retentionModeOf(g, alwaysTarget, testNs)).To(Equal("Always")) + }).Should(Succeed()) + + By("no condition went False for a retention — it is the configured outcome, not a fault") + verifyResourceCondition("gittarget", defaultTarget, testNs, "Ready", "True", "", "") + }) + + // The migration instruction this release ships with is "declare always to keep the old + // behaviour". That is only true if the edit itself converges the mirror: the watch specs + // describe what is watched, not what may be deleted, so a prune edit changes none of them, and + // a reconnect resumes from its cursor rather than replaying. Without the widening being its own + // trigger, a quiet target could sit under always indefinitely and never sweep. + // + // Deliberately NO WatchRule change here — that is the whole point. The previous sweep spec had + // to toggle the rule to force a resync; if this spec ever needs the same crutch, the fix has + // regressed. + It("converges an existing orphan when prune.mode is widened, without touching the WatchRule", func() { + const lateOrphanName = "prune-late-orphan" + + By("seeding a second orphan that no cluster event will ever mention") + lateOrphan := pruneConfigMapPath(defaultPath, testNs, lateOrphanName) + seedOrphanManifests(pruneRepo, testNs, map[string]string{ + lateOrphan: orphanConfigMapYAML(lateOrphanName, testNs), + }) + waitForPruneFile(pruneRepo, lateOrphan, true) + + By("widening the default target's policy to always — the only action this spec takes") + _, err := kubectlRunInNamespace(testNs, "patch", "gittarget", defaultTarget, + "--type=merge", "-p", `{"spec":{"prune":{"mode":"Always"}}}`) + Expect(err).NotTo(HaveOccurred(), "spec.prune is mutable and the patch must be accepted") + + By("the newly authorized sweep removes both retained orphans") + waitForPruneFile(pruneRepo, lateOrphan, false) + waitForPruneFile(pruneRepo, pruneConfigMapPath(defaultPath, testNs, orphanName), false) + + By("and status follows the sweep back to a converged zero under the new mode") + waitForStreamsRunning(defaultTarget, testNs) + Eventually(func(g Gomega) { + g.Expect(retainedDocumentsOf(g, defaultTarget, testNs)).To(Equal(0), + "a resync that retains nothing must drive the count back to zero, not leave it stale") + g.Expect(retentionModeOf(g, defaultTarget, testNs)).To(Equal("Always")) + }).Should(Succeed()) + + By("the target still mirrors live events after the forced replay") + const proofName = "prune-post-widen" + applyIsolationConfigMap(proofName, testNs) + waitForPruneFile(pruneRepo, pruneConfigMapPath(defaultPath, testNs, proofName), true) + }) +}) + +// retainedDocumentsOf reads status.retention.retainedDocuments. An ABSENT retention block fails the +// read rather than reporting zero: the two mean different things (nothing reported yet vs. a resync +// found nothing), and collapsing them here would let a spec pass before any resync had run. +func retainedDocumentsOf(g Gomega, name, namespace string) int { + out, err := kubectlRunInNamespace(namespace, "get", "gittarget", name, + "-o", "jsonpath={.status.retention.retainedDocuments}") + g.Expect(err).NotTo(HaveOccurred(), "failed to read status.retention of %q", name) + value := strings.TrimSpace(out) + g.Expect(value).NotTo(BeEmpty(), "%q has not reported a retention roll-up yet", name) + count, convErr := strconv.Atoi(value) + g.Expect(convErr).NotTo(HaveOccurred(), "retainedDocuments %q is not a number", value) + return count +} + +// retentionModeOf reads status.retention.mode — the effective prune mode the count was produced +// under, which for a legacy GitTarget is the only place that mode is visible at all. +func retentionModeOf(g Gomega, name, namespace string) string { + out, err := kubectlRunInNamespace(namespace, "get", "gittarget", name, + "-o", "jsonpath={.status.retention.mode}") + g.Expect(err).NotTo(HaveOccurred(), "failed to read status.retention.mode of %q", name) + return strings.TrimSpace(out) +} + +// applyPruneGitTarget creates a GitTarget with the given prune mode. An empty mode omits the +// prune block entirely — the legacy shape, which must resolve to OnEvent without being edited. +func applyPruneGitTarget(name, namespace, providerName, targetPath, mode string) { + GinkgoHelper() + data := struct { + Name string + Namespace string + ProviderName string + Branch string + Path string + PruneMode string + }{ + Name: name, + Namespace: namespace, + ProviderName: providerName, + Branch: "main", + Path: targetPath, + PruneMode: mode, + } + Expect(applyFromTemplate("test/e2e/templates/manager/gittarget-prune.tmpl", data, namespace)). + To(Succeed(), "failed to apply GitTarget %q with prune mode %q", name, mode) +} + +// applyRawGitTarget applies a GitTarget whose prune block is supplied verbatim, so a spec can +// send the API server a body the typed template cannot express (an empty object, or an invalid +// enum value). It returns the apply error rather than asserting, since rejection is the point. +func applyRawGitTarget(name, namespace, providerName, targetPath, pruneBlock string) error { + 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 +%s +`, name, namespace, providerName, targetPath, pruneBlock) + out, err := kubectlRunWithStdin(namespace, manifest, "apply", "-f", "-") + if err != nil { + return fmt.Errorf("%w: %s", err, out) + } + return nil +} + +// pruneModeOf reads a GitTarget's stored spec.prune.mode, empty when the field is absent. +func pruneModeOf(name, namespace string) string { + GinkgoHelper() + out, err := kubectlRunInNamespace(namespace, "get", "gittarget", name, + "-o", "jsonpath={.spec.prune.mode}") + Expect(err).NotTo(HaveOccurred(), "failed to read spec.prune.mode of %q", name) + return strings.TrimSpace(out) +} + +// pruneConfigMapPath is the canonical mirror path for a ConfigMap under a GitTarget folder. +func pruneConfigMapPath(basePath, ns, name string) string { + return path.Join(basePath, fmt.Sprintf("%s/configmaps/%s.yaml", ns, name)) +} + +// waitForPruneFile waits until a repo-relative path is present (or absent), pulling fresh state +// on each attempt. +func waitForPruneFile(repo *RepoArtifacts, relPath string, wantPresent bool) { + GinkgoHelper() + Eventually(func(g Gomega) { + pullLatestRepoState(g, repo.CheckoutDir) + _, statErr := os.Stat(filepath.Join(repo.CheckoutDir, relPath)) + if wantPresent { + g.Expect(statErr).NotTo(HaveOccurred(), "%s should exist", relPath) + return + } + g.Expect(os.IsNotExist(statErr)).To(BeTrue(), "%s should be gone", relPath) + }).Should(Succeed()) +} + +// orphanConfigMapYAML renders a ConfigMap manifest for a resource that does NOT exist in the +// cluster — a managed document with no desired counterpart, which is precisely what a +// mark-and-sweep resync classifies as a managed drop. +func orphanConfigMapYAML(name, namespace string) string { + return fmt.Sprintf(`apiVersion: v1 +kind: ConfigMap +metadata: + name: %s + namespace: %s +data: + seeded-by: e2e-prune-spec +`, name, namespace) +} + +// seedOrphanManifests commits files straight into the repo's main branch, bypassing the operator. +// It mirrors the push-with-rebase-retry shape the in-place edit spec uses, because the operator +// pushes to the same branch concurrently and a lost race must not fail the spec. +func seedOrphanManifests(repo *RepoArtifacts, namespace string, filesByRelPath map[string]string) { + GinkgoHelper() + + configureRepoOriginWithCredentials(repo, namespace) + mustGit := func(args ...string) { + out, gitErr := gitRun(repo.CheckoutDir, args...) + Expect(gitErr).NotTo(HaveOccurred(), fmt.Sprintf("git %s: %s", strings.Join(args, " "), out)) + } + + mustGit("fetch", "origin", "main") + mustGit("checkout", "-B", "main", "origin/main") + mustGit("reset", "--hard", "origin/main") + + for relPath, body := range filesByRelPath { + full := filepath.Join(repo.CheckoutDir, relPath) + Expect(os.MkdirAll(filepath.Dir(full), 0o750)).To(Succeed()) + Expect(os.WriteFile(full, []byte(body), 0o600)).To(Succeed()) + mustGit("add", relPath) + } + mustGit("commit", "-m", "e2e: seed an orphaned managed document") + if _, pushErr := gitRun(repo.CheckoutDir, "push", "origin", "HEAD:main"); pushErr != nil { + // Lost a race with the operator's own push: rebase onto the new tip and retry. + mustGit("fetch", "origin", "main") + mustGit("rebase", "origin/main") + mustGit("push", "origin", "HEAD:main") + } +} diff --git a/test/e2e/templates/manager/gittarget-prune.tmpl b/test/e2e/templates/manager/gittarget-prune.tmpl new file mode 100644 index 00000000..9fc398c7 --- /dev/null +++ b/test/e2e/templates/manager/gittarget-prune.tmpl @@ -0,0 +1,15 @@ +apiVersion: configbutler.ai/v1alpha3 +kind: GitTarget +metadata: + name: {{ .Name }} + namespace: {{ .Namespace }} +spec: + providerRef: + kind: GitProvider + name: {{ .ProviderName }} + branch: {{ .Branch }} + path: {{ .Path }} +{{- if .PruneMode }} + prune: + mode: {{ .PruneMode }} +{{- end }}