diff --git a/api/v1alpha3/gitprovider_types.go b/api/v1alpha3/gitprovider_types.go index b8f77fe4..45b40923 100644 --- a/api/v1alpha3/gitprovider_types.go +++ b/api/v1alpha3/gitprovider_types.go @@ -179,7 +179,10 @@ type CommitMessageSpec struct { // LiveTemplate formats every live window, including one resource and a 0s window. // Fields: Author (raw username, possibly empty), GitTarget, Count, Operations, Resources. // Count is retained input entries after coalescing, before comparison with Git. - // Resources exposes Operation, Group, Version, Resource, Namespace, Name, and APIVersion. + // Resources exposes Operation, Group, Version, Resource, Namespace, Name, APIVersion, + // ResourceVersion — the version of the state this commit wrote, which lags the object's + // current one and is empty for a producer that observed none — and Generation, the desired + // state's counter, which is 0 for a kind without a spec. Guard both with {{with}}. // A literal CommitRequest message overrides this template without changing authorship. // +optional LiveTemplate string `json:"liveTemplate,omitempty"` @@ -207,9 +210,11 @@ type CommitMessageSpec struct { RequestTemplate string `json:"requestTemplate,omitempty"` // ReconcileTemplate formats atomic snapshots and resyncs. - // Fields: Count, GitTarget, Group, Version, Resource, APIVersion, Namespace, Revision. - // Type and Namespace fields are empty for whole-target snapshots. Revision can be empty. + // Fields: Count, GitTarget, Group, Version, Resource, APIVersion, Namespace, ResourceVersion. + // ResourceVersion is the snapshot LIST's version, not any one object's. Type and Namespace + // fields are empty for whole-target snapshots, and ResourceVersion can be empty. // Guard optional fields so the message remains meaningful for every snapshot scope. + // Revision was renamed to ResourceVersion in v0.48.0; a template still naming it is rejected. // +optional ReconcileTemplate string `json:"reconcileTemplate,omitempty"` diff --git a/config/crd/bases/configbutler.ai_gittargets.yaml b/config/crd/bases/configbutler.ai_gittargets.yaml index b34584ac..d0044b10 100644 --- a/config/crd/bases/configbutler.ai_gittargets.yaml +++ b/config/crd/bases/configbutler.ai_gittargets.yaml @@ -162,15 +162,20 @@ spec: LiveTemplate formats every live window, including one resource and a 0s window. Fields: Author (raw username, possibly empty), GitTarget, Count, Operations, Resources. Count is retained input entries after coalescing, before comparison with Git. - Resources exposes Operation, Group, Version, Resource, Namespace, Name, and APIVersion. + Resources exposes Operation, Group, Version, Resource, Namespace, Name, APIVersion, + ResourceVersion — the version of the state this commit wrote, which lags the object's + current one and is empty for a producer that observed none — and Generation, the desired + state's counter, which is 0 for a kind without a spec. Guard both with {{with}}. A literal CommitRequest message overrides this template without changing authorship. type: string reconcileTemplate: description: |- ReconcileTemplate formats atomic snapshots and resyncs. - Fields: Count, GitTarget, Group, Version, Resource, APIVersion, Namespace, Revision. - Type and Namespace fields are empty for whole-target snapshots. Revision can be empty. + Fields: Count, GitTarget, Group, Version, Resource, APIVersion, Namespace, ResourceVersion. + ResourceVersion is the snapshot LIST's version, not any one object's. Type and Namespace + fields are empty for whole-target snapshots, and ResourceVersion can be empty. Guard optional fields so the message remains meaningful for every snapshot scope. + Revision was renamed to ResourceVersion in v0.48.0; a template still naming it is rejected. type: string requestTemplate: description: |- diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 0afc16f3..612b5ecf 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,55 @@ 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. +## `reconcileTemplate`'s `Revision` is now `ResourceVersion` + +**Breaking, and only for a custom `reconcileTemplate`.** The reconcile commit-message field +`{{.Revision}}` is renamed to `{{.ResourceVersion}}`. A template still naming the old spelling is +**rejected**, not quietly pruned: the `GitTarget` reports `Validated=False` with reason +`InvalidConfig` and a message naming both spellings, and it stops gating its rules until the +template is fixed. + +Nothing changes if you never set `reconcileTemplate`. The default template is renamed with the +field and its output is byte-identical — still +`chore: reconcile 4 configmaps in team-a (last resourceVersion: 1331)`. + +### Why + +`Revision` meant three different things in this project: a Git commit (`LayoutReport.Revision`, the +`GitTarget`'s `resolvedAtRevision`), an internal watch-plan counter, and — only in this one +user-facing place — a Kubernetes `resourceVersion`. A commit message about Git is the last place +"revision" should mean something other than a commit. + +The rename lands in the same release as the new per-resource +[`Resources[i].ResourceVersion`](configuration.md#naming-the-state-a-commit-wrote) on +`liveTemplate`, so the vocabulary changes once: **`Revision` is a Git commit, `ResourceVersion` is a +Kubernetes version.** + +### What to change + +Rename the field in any `reconcileTemplate` that uses it: + +```yaml +spec: + commit: + message: + # before + reconcileTemplate: "chore: reconcile {{.Count}}{{if .Revision}} at {{.Revision}}{{end}}" + # after + reconcileTemplate: "chore: reconcile {{.Count}}{{with .ResourceVersion}} at {{.}}{{end}}" +``` + +Find them before upgrading: + +```bash +kubectl get gittargets -A \ + -o jsonpath='{range .items[?(@.spec.commit.message.reconcileTemplate)]}{.metadata.namespace}{"/"}{.metadata.name}{"\t"}{.spec.commit.message.reconcileTemplate}{"\n"}{end}' \ + | grep -F '.Revision' +``` + +The meaning is unchanged: it is the snapshot `LIST`'s `resourceVersion`, one value for the whole +reconcile, and empty for a pure sweep. + ## `CommitRequest.spec.closeDelaySeconds` defaults to 2 seconds **Breaking.** A `CommitRequest` that omits `closeDelaySeconds` now sets its finalize deadline two diff --git a/docs/configuration.md b/docs/configuration.md index f398ab18..722159b3 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -598,15 +598,15 @@ spec: {{range .Resources -}} - [{{.Operation}}] {{.APIVersion}}/{{.Resource}}/{{if .Namespace}}{{.Namespace}}/{{end}}{{.Name}} {{end -}} - reconcileTemplate: "chore: reconcile {{.Count}} {{if .Resource}}{{.Resource}}{{else}}resources{{end}}{{if .Namespace}} in {{.Namespace}}{{end}}{{if .Revision}} (last resourceVersion: {{.Revision}}){{end}}" + reconcileTemplate: "chore: reconcile {{.Count}} {{if .Resource}}{{.Resource}}{{else}}resources{{end}}{{if .Namespace}} in {{.Namespace}}{{end}}{{if .ResourceVersion}} (last resourceVersion: {{.ResourceVersion}}){{end}}" ``` | Template | Fields | |---|---| | `liveTemplate` | `Author`, `GitTarget`, `Count`, `Operations`, `Resources`, and the `LabelValues` / `LabelValue` accessors | | `requestTemplate` | the same fields, plus `RequestMessage` | -| Each `Resources` entry | `Operation`, `Group`, `Version`, `Resource`, `Kind`, `Namespace`, `Name`, `APIVersion`, `Labels`, and the `Label` accessor | -| `reconcileTemplate` | `Count`, `GitTarget`, `Group`, `Version`, `Resource`, `APIVersion`, `Namespace`, `Revision` | +| Each `Resources` entry | `Operation`, `Group`, `Version`, `Resource`, `Kind`, `Namespace`, `Name`, `APIVersion`, `ResourceVersion`, `Generation`, `Labels`, and the `Label` accessor | +| `reconcileTemplate` | `Count`, `GitTarget`, `Group`, `Version`, `Resource`, `APIVersion`, `Namespace`, `ResourceVersion` | Live `Count` counts retained entries after window coalescing, before the writer compares them with Git. Repeated edits to one resource collapse to one entry, with the last operation retained. @@ -699,12 +699,65 @@ Three things to know: nothing, so a commit holding one labeled and one unlabeled resource is named after neither. - **A `DELETE` carries no object**, because the resource is already gone from the cluster, so `Kind` and `Labels` are empty for one. The identity fields (`Name`, `Namespace`, `Resource`, …) - are unaffected. A commit containing a `DELETE` therefore has no agreed `LabelValue`: what the - deleted resource was labeled is not something the window still knows. + are unaffected, and so are `ResourceVersion` and `Generation`: the watch delivers the final + object, so the last state that existed is still knowable even though the object is not. A commit containing a + `DELETE` therefore has no agreed `LabelValue`: what the deleted resource was labeled is not + something the window still knows. + +##### Naming the state a commit wrote + +Two counters describe the observed state each entry was at. Neither is committed to the file +(`resourceVersion` and `generation` are stripped from every manifest on purpose, because a counter +inside a manifest would make every observation a byte change); both travel beside the object, in +the message only. Both are off by default, and both are guarded with `{{with}}`: + +```yaml +liveTemplate: |- + chore: sync {{.Count}} resource{{if ne .Count 1}}s{{end}} + + {{range .Resources -}} + - [{{.Operation}}] {{.APIVersion}}/{{.Resource}}/{{.Namespace}}/{{.Name}}{{with .ResourceVersion}}@{{.}}{{end}}{{with .Generation}} gen{{.}}{{end}} + {{end -}} +``` + +| | `{{.ResourceVersion}}` | `{{.Generation}}` | +|---|---|---| +| moves when | **anything** is written, `/status` included | only the **desired state** changes | +| present on | every resource the operator observed | only kinds with a spec: **not** a ConfigMap or a Secret | +| absent value | `""` | `0` | +| safe to compare | for **equality** only | yes: per object, starts at `1`, one step per spec write | + +**Turn `ResourceVersion` on to make a commit joinable**: to an audit log entry, to a +`kubectl get -o yaml` taken at the time, to another operator's logs. Equal to the object's current +version means nothing is pending. Do not subtract two of them: `resourceVersion` is opaque by API +contract and is the cluster-wide store revision in practice, so two consecutive commits of one +ConfigMap can read `@1331` then `@8402` with nothing skipped, because every other object's writes +moved the same counter. + +**Turn `Generation` on to see what a commit missed.** It is the counter a gap is meaningful in: a +commit at `gen3` following one at `gen6` means three spec changes that were not committed +separately. That is the question `ResourceVersion` cannot answer. + +Read them together, because each is blind where the other sees: + +- **`ResourceVersion` lags, deliberately.** An update whose committed content would be identical (a + `/status`-only write) is never routed, so the version in a commit can be older than the object's + current one. A resource re-edited inside one window contributes the last routed version, and an + entry that already matched Git still contributes one. +- **`Generation` misses metadata.** A label- or annotation-only edit changes what gets committed + without moving it, so an unchanged `Generation` across two commits does not mean an unchanged + commit. And it is `0` for every spec-less kind, which is much of what a typical target mirrors. +- **Neither counts skips.** To count what the operator deliberately did not route, read + `watch_events_total{outcome="unchanged"}`, which is exactly that census. + +A producer that observed no object renders nothing for either: reconcile, resync, and bootstrap +writes all leave both empty. `reconcileTemplate` gets none of these. It describes a *type* being reconciled rather than a list -of resources, so there are no labels to read, and its `Namespace` keeps the plain meaning it always -had: the namespace a namespace-scoped reconcile covered, empty for a whole-target or all-namespaces +of resources, so there are no labels to read, and its own `{{.ResourceVersion}}` is the snapshot +`LIST`'s version: one value for the whole run, not any single object's. Live names n resources, so +it names each one's version; reconcile names a type, so it names the snapshot's. Its `Namespace` +keeps the plain meaning it always had: the namespace a namespace-scoped reconcile covered, empty for a whole-target or all-namespaces one. That emptiness means "every namespace", not "cluster-scoped", so the `_cluster` sentinel would be a lie there rather than a convenience. @@ -724,6 +777,13 @@ The nouns are the same in both, and only the spelling follows each host language | name | `{name}` | `.Name` | | API version | `{apiVersion}` | `.APIVersion` | | one label | `{label:team}` | `.Label "team"` | +| the observed version | (none) | `.ResourceVersion` | +| the desired-state counter | (none) | `.Generation` | + +The two counters are the first nouns that legitimately exist on only one side. A path keyed on a +counter would write a new file whenever it moved, which is the opposite of what placement is for: a +path has to be stable and statically checkable. A message describes one moment, so it can name +one. The capitals are Go's, not a style choice: `text/template` can only reach exported struct fields, which must begin with one. The braces are lower-case because a placement template reads like the @@ -731,8 +791,12 @@ manifest it is filing (`metadata.namespace`). A label needs a method call rather the commit side because a Go template field name cannot contain a `:` or a `/`. Reconcile type fields name the synced type; `Namespace` names a namespace-scoped snapshot. -Whole-target snapshots leave those fields empty. `Revision` is the snapshot's resourceVersion and -can be absent, including a pure sweep. Guard optional values as in the example. +Whole-target snapshots leave those fields empty. `ResourceVersion` is the snapshot's resourceVersion +and can be absent, including a pure sweep. Guard optional values as in the example. + +`ResourceVersion` was called `Revision` before `v0.48.0`. A `reconcileTemplate` still naming +`{{.Revision}}` is rejected, and the `GitTarget` says so on its `Validated` condition. See +[the upgrade note](UPGRADING.md#reconciletemplates-revision-is-now-resourceversion). A reconcile runs per *cell* (a (type, namespace) pair) rather than per target, so a namespace-scoped run covers exactly one namespace and the default subject names it: diff --git a/internal/controller/gittarget_commit_validation_test.go b/internal/controller/gittarget_commit_validation_test.go index 31dfc623..ba242756 100644 --- a/internal/controller/gittarget_commit_validation_test.go +++ b/internal/controller/gittarget_commit_validation_test.go @@ -78,6 +78,19 @@ func TestValidateCommitConfig(t *testing.T) { }, saysA: "spec.commit.message", }, + { + // The renamed reconcile field reaches an operator HERE, on the Validated condition, + // so the message has to name the new spelling. Reaching it through the controller + // (not just ValidateCommitConfig) is the point: this is the text someone upgrading + // into a stored template actually reads. + name: "a reconcile template still naming the retired Revision", + spec: &configbutleraiv1alpha3.GitTargetCommitSpec{ + Message: &configbutleraiv1alpha3.CommitMessageSpec{ + ReconcileTemplate: "chore: reconcile {{.Count}} at {{.Revision}}", + }, + }, + saysA: "{{.ResourceVersion}}", + }, } { t.Run(tc.name, func(t *testing.T) { target := &configbutleraiv1alpha3.GitTarget{} diff --git a/internal/git/branch_worker_split_test.go b/internal/git/branch_worker_split_test.go index cd26f63d..b00b6d2c 100644 --- a/internal/git/branch_worker_split_test.go +++ b/internal/git/branch_worker_split_test.go @@ -1140,7 +1140,7 @@ func TestResync_WorkerAppliesMarkAndSweepAndCommits(t *testing.T) { // Resync: the cluster now has "keep" and no longer has "drop-me". req := &ResyncRequest{ Desired: []manifestanalyzer.DesiredResource{desiredCM("keep", "blue")}, - Revision: "42", + ResourceVersion: "42", GitTargetName: "target-a", GitTargetNamespace: "default", Result: make(chan ResyncResult, 1), diff --git a/internal/git/commit.go b/internal/git/commit.go index 3d12c3de..e3aa2c96 100644 --- a/internal/git/commit.go +++ b/internal/git/commit.go @@ -24,8 +24,8 @@ import ( // renderReconcileCommitMessageFromEvents renders the reconcile commit message for the // events-based atomic path from the provider's ReconcileTemplate. It carries no single -// type or revision, so those template fields stay empty (the default guards them). Literal -// overrides are resolved by the caller. +// type and no snapshot version, so those template fields stay empty (the default guards them). +// Literal overrides are resolved by the caller. func renderReconcileCommitMessageFromEvents( events []Event, gitTarget string, @@ -44,21 +44,21 @@ func renderReconcileCommitMessageFromEvents( // renderReconcileCommitMessage renders the reconcile commit message for a resync from the // provider's ReconcileTemplate, so a resync honours a custom reconcile template. count is the // number of resources the reconcile changed; scopeGVR names the synced type for a per-type -// reconcile, and a nil scopeGVR (whole-target reconcile) leaves the type fields empty; revision -// is the cluster resourceVersion the desired set was pinned to (empty for a pure sweep). The -// default template guards the type and revision fields so it still renders cleanly when either -// is absent. +// reconcile, and a nil scopeGVR (whole-target reconcile) leaves the type fields empty; +// resourceVersion is the cluster resourceVersion the desired set was pinned to (empty for a +// pure sweep). The default template guards the type and version fields so it still renders +// cleanly when either is absent. func renderReconcileCommitMessage( count int, gitTarget string, scope *ResyncScope, - revision string, + resourceVersion string, config CommitConfig, ) (string, error) { data := ReconcileCommitMessageData{ - Count: count, - GitTarget: gitTarget, - Revision: revision, + Count: count, + GitTarget: gitTarget, + ResourceVersion: resourceVersion, } if scope != nil { data.Group = scope.Cell.Group @@ -133,7 +133,8 @@ func ValidateCommitConfig(config CommitConfig) error { // Validate the per-type reconcile path with the type and revision fields populated, // so a custom reconcile template that names its synced type ({{.Resource}} / {{.APIVersion}}) - // or pins the {{.Revision}} is exercised at admission exactly as a per-type reconcile renders it. + // or pins the {{.ResourceVersion}} is exercised at admission exactly as a per-type reconcile + // renders it. // The sample scope names a namespace so a template referencing {{.Namespace}} — populated // only by a namespace-scoped reconcile — is validated here too. sampleScope := ResyncScopeFor( @@ -173,6 +174,11 @@ func ValidateCommitConfig(config CommitConfig) error { // It is shared so liveTemplate and requestTemplate cannot drift into being checked against // different worlds. They face identical windows at runtime; checking one more thoroughly than the // other just moves which template fails months later instead of at admission. +// +// The UPDATE sample deliberately carries NO resourceVersion and NO generation. A live watch +// stamps a version on every event, but reconcile and bootstrap do not, and a generation is absent +// for every kind without a spec (a ConfigMap, a Secret) however it was produced — so the shape a +// template must survive is the mixed one. func liveValidationSamples(sampleEvent Event) [][]Event { var samples [][]Event for _, author := range []string{"template-validator", ""} { @@ -182,6 +188,8 @@ func liveValidationSamples(sampleEvent Event) [][]Event { event.UserInfo.Username = author event.Operation = operation event.Identifier.Name = operation + event.ResourceVersion = sampleResourceVersion(operation) + event.Generation = sampleGeneration(operation) if operation == "CREATE" { event.Object = sampleLabeledObject() } @@ -192,6 +200,38 @@ func liveValidationSamples(sampleEvent Event) [][]Event { return samples } +// sampleResourceVersion gives the validation samples both states of ResourceVersion: present on a +// CREATE and on a DELETE — which carries one although it carries no object — and absent on the +// UPDATE, standing in for every producer that observed no version. +func sampleResourceVersion(operation string) string { + switch operation { + case "CREATE": + return "20001" + case "DELETE": + return "20003" + default: + return "" + } +} + +// sampleGeneration mirrors sampleResourceVersion for the desired-state counter, so a template +// naming {{.Generation}} is validated against a resource that has one and a resource that does +// not — the second being every spec-less kind this operator mirrors. +func sampleGeneration(operation string) int64 { + const ( + sampleCreateGeneration = 3 + sampleDeleteGeneration = 4 + ) + switch operation { + case "CREATE": + return sampleCreateGeneration + case "DELETE": + return sampleDeleteGeneration + default: + return 0 + } +} + // newRequestTemplateProbe mints the sentinel that requestTemplate validation renders as the // request's message. // diff --git a/internal/git/commit_resource_version_test.go b/internal/git/commit_resource_version_test.go new file mode 100644 index 00000000..53c7187c --- /dev/null +++ b/internal/git/commit_resource_version_test.go @@ -0,0 +1,200 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "strings" + "testing" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/sanitize" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// observedEvent is a live event as the watch seam builds one: the object sanitized, the observed +// counters carried BESIDE it. generation 0 stands in for a kind that has none. +func observedEvent(name, operation, resourceVersion string, generation int64) Event { + event := Event{ + Identifier: types.NewResourceIdentifier("apps", "v1", "deployments", "prod", name), + Operation: operation, + ResourceVersion: resourceVersion, + Generation: generation, + } + if operation != "DELETE" { + event.Object = &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]any{"name": name, "namespace": "prod"}, + }} + } + return event +} + +func TestBuildLiveCommitMessageData_CarriesResourceVersionPerResource(t *testing.T) { + data := buildLiveCommitMessageData("someone", "target", "", []Event{ + observedEvent("api", "CREATE", "20001", 3), + // A DELETE carries no object, and so no Kind and no labels — but it DOES carry a + // version: the watch Deleted frame delivers the final object, so the last version that + // existed is knowable even though the object is not. + observedEvent("web", "DELETE", "20003", 4), + // A reconcile-sourced event observed nothing, so it names no version. + {Identifier: types.NewResourceIdentifier("", "v1", "configmaps", "prod", "cm"), Operation: "RECONCILE"}, + }) + + want := []string{"20001", "20003", ""} + for i, w := range want { + if got := data.Resources[i].ResourceVersion; got != w { + t.Errorf("Resources[%d].ResourceVersion = %q, want %q", i, got, w) + } + } + wantGenerations := []int64{3, 4, 0} + for i, w := range wantGenerations { + if got := data.Resources[i].Generation; got != w { + t.Errorf("Resources[%d].Generation = %d, want %d", i, got, w) + } + } + if kind := data.Resources[1].Kind; kind != "" { + t.Errorf("a DELETE must still carry no Kind, got %q", kind) + } +} + +// A resource re-edited inside one window collapses to its last routed event, so the version the +// message names is the state the commit actually writes — not the one that opened the window. +// +// It goes through openWindow, which is where coalescing actually happens: add() is +// last-write-wins per windowPathKey and orderedEvents() yields one event per path. +// buildLiveCommitMessageData does NOT deduplicate — it appends one ResourceRef per event it is +// handed — so feeding it both events directly and reading the last element would assert nothing +// but slice indexing, and would keep passing if coalescing stopped selecting the final event. +// Hence the length assertion first: one entry is the property, the counters on it are the payload. +func TestOpenWindow_ResourceVersionIsTheLastObserved(t *testing.T) { + first := observedEvent("api", "CREATE", "20001", 3) + second := observedEvent("api", "UPDATE", "20009", 4) + + window := newOpenWindow(first, newContentWriter(types.SensitiveResourcePolicy{})) + window.add(first) + window.add(second) + + data := buildLiveCommitMessageData( + window.Author, window.GitTarget, "", window.orderedEvents()) + + if len(data.Resources) != 1 { + t.Fatalf("the window collapsed to %d resources, want 1 — both events write the same path", + len(data.Resources)) + } + surviving := data.Resources[0] + if surviving.ResourceVersion != "20009" { + t.Errorf("the surviving ref names %q, want the newest observed version 20009", + surviving.ResourceVersion) + } + if surviving.Generation != 4 { + t.Errorf("the surviving ref names generation %d, want the newest observed 4", + surviving.Generation) + } +} + +func TestRenderLiveCommitMessage_RendersResourceVersion(t *testing.T) { + config := CommitConfig{Message: CommitMessageConfig{ + LiveTemplate: `chore: sync {{.Count}}` + "\n" + + `{{range .Resources}}- {{.Name}}{{with .ResourceVersion}}@{{.}}{{end}}` + "\n" + `{{end}}`, + }} + write := PendingWrite{Kind: PendingWriteCommit, Events: []Event{ + observedEvent("api", "CREATE", "20001", 3), + {Identifier: types.NewResourceIdentifier("", "v1", "configmaps", "prod", "cm"), Operation: "RECONCILE"}, + }} + + got, err := renderLiveCommitMessage(write, config) + if err != nil { + t.Fatalf("renderLiveCommitMessage: %v", err) + } + if !strings.Contains(got, "- api@20001") { + t.Errorf("message %q must name the observed version", got) + } + // The {{with}} guard is what keeps a mixed window renderable, so the unversioned entry has + // to come out clean rather than as a trailing "@". + if !strings.Contains(got, "- cm\n") { + t.Errorf("message %q must render an unversioned resource without a dangling separator", got) + } +} + +// The invariant this whole feature rests on: the version reaches the MESSAGE and never the +// CONTENT. A version inside a committed manifest would make every observation a byte change, +// which is exactly what sanitize strips it to prevent — so anyone tempted to "fix" the empty +// field by leaving resourceVersion on the object fails here. +func TestObservedResourceVersionNeverReachesCommittedContent(t *testing.T) { + live := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]any{ + "name": "api", "namespace": "prod", "resourceVersion": "20001", "generation": int64(3), + }, + }} + + event := Event{ + Identifier: types.NewResourceIdentifier("apps", "v1", "deployments", "prod", "api"), + Operation: "CREATE", + Object: sanitize.Sanitize(live), + ResourceVersion: live.GetResourceVersion(), + Generation: live.GetGeneration(), + } + + if event.ResourceVersion != "20001" || event.Generation != 3 { + t.Fatalf("the event must carry the observed counters, got %q/%d", + event.ResourceVersion, event.Generation) + } + if got := event.Object.GetResourceVersion(); got != "" { + t.Errorf("the committed object still carries resourceVersion %q: it must never reach content", got) + } + if got := event.Object.GetGeneration(); got != 0 { + t.Errorf("the committed object still carries generation %d: it must never reach content", got) + } + for _, field := range []string{"resourceVersion", "generation"} { + if _, found, _ := unstructured.NestedFieldNoCopy(event.Object.Object, "metadata", field); found { + t.Errorf("metadata.%s is present in the object the writer commits", field) + } + } + // And the source object is untouched, so stamping the event cannot have consumed it. + if live.GetResourceVersion() != "20001" || live.GetGeneration() != 3 { + t.Error("sanitizing must not strip the counters from the observed object") + } +} + +func TestValidateCommitConfig_AcceptsResourceVersionTemplates(t *testing.T) { + config := ResolveCommitConfig(nil).WithTargetMessage(&v1alpha3.CommitMessageSpec{ + LiveTemplate: `chore: sync {{.Count}}{{range .Resources}} {{.Name}}` + + `{{with .ResourceVersion}}@{{.}}{{end}}{{with .Generation}} gen{{.}}{{end}}{{end}}`, + ReconcileTemplate: `chore: reconcile {{.Count}}{{with .ResourceVersion}} at {{.}}{{end}}`, + }) + + if err := ValidateCommitConfig(config); err != nil { + t.Errorf("a template naming ResourceVersion must validate: %v", err) + } +} + +// The validation samples must contain BOTH states, or a template that only renders cleanly for a +// versioned resource passes admission and fails months later in a mixed window. +func TestLiveValidationSamples_CoverVersionedAndUnversioned(t *testing.T) { + var versioned, unversioned, generated, ungenerated bool + for _, events := range liveValidationSamples(Event{Operation: "CREATE"}) { + for _, event := range events { + if event.ResourceVersion == "" { + unversioned = true + } else { + versioned = true + } + if event.Generation == 0 { + ungenerated = true + } else { + generated = true + } + } + } + if !versioned || !unversioned { + t.Errorf("samples cover versioned=%v unversioned=%v, want both", versioned, unversioned) + } + if !generated || !ungenerated { + t.Errorf("samples cover generation present=%v absent=%v, want both", generated, ungenerated) + } +} diff --git a/internal/git/commit_test.go b/internal/git/commit_test.go index 63f80d18..0ac0adba 100644 --- a/internal/git/commit_test.go +++ b/internal/git/commit_test.go @@ -202,7 +202,7 @@ func TestRenderReconcileCommitMessage_CustomTemplateUsesTypeAndRevisionFields(t "1331", ResolveCommitConfig(nil).WithTargetMessage(&v1alpha3.CommitMessageSpec{ ReconcileTemplate: "e2e-snapshot: synced {{.Count}} {{.APIVersion}}/{{.Resource}}" + - "@{{.Revision}} to {{.GitTarget}}", + "@{{.ResourceVersion}} to {{.GitTarget}}", }), ) require.NoError(t, err) @@ -211,7 +211,7 @@ func TestRenderReconcileCommitMessage_CustomTemplateUsesTypeAndRevisionFields(t func TestValidateCommitConfig_CustomReconcileTemplateReferencingTypeAndRevision(t *testing.T) { config := ResolveCommitConfig(nil).WithTargetMessage(&v1alpha3.CommitMessageSpec{ - ReconcileTemplate: "reconcile: {{.Count}} {{.APIVersion}}/{{.Resource}}@{{.Revision}} on {{.GitTarget}}", + ReconcileTemplate: "reconcile: {{.Count}} {{.APIVersion}}/{{.Resource}}@{{.ResourceVersion}} on {{.GitTarget}}", }) require.NoError(t, ValidateCommitConfig(config)) diff --git a/internal/git/open_window.go b/internal/git/open_window.go index b32813c8..694ab8f5 100644 --- a/internal/git/open_window.go +++ b/internal/git/open_window.go @@ -102,7 +102,9 @@ func windowPathKey(e Event, writer eventContentWriter) string { // buildLiveCommitMessageData produces the template context for a grouped // commit unit. Operations are counted by Operation tag; Resources is the -// deduplicated list of resource refs in arrival order. +// deduplicated list of resource refs in arrival order — so a resource re-edited inside the +// window contributes the LAST routed event's resourceVersion, which is the state the commit +// actually writes. // // requestMessage is the attached CommitRequest's message, or empty when no request attached. It is // carried through unaltered — never parsed, never trimmed — because the request's literal bytes @@ -133,6 +135,12 @@ func buildLiveCommitMessageData( Namespace: e.Identifier.NamespaceOrCluster(), Name: e.Identifier.Name, Labels: labels, + // Read off the EVENT, not off Object: sanitize strips resourceVersion and + // generation from the object precisely so neither reaches a committed manifest. + // That is also why a DELETE carries them here while Kind and Labels above stay + // empty for it. + ResourceVersion: e.ResourceVersion, + Generation: e.Generation, }) } return LiveCommitMessageData{ diff --git a/internal/git/reconcile_revision_rename_test.go b/internal/git/reconcile_revision_rename_test.go new file mode 100644 index 00000000..a86cd0c7 --- /dev/null +++ b/internal/git/reconcile_revision_rename_test.go @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "strings" + "testing" + + "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/ConfigButler/gitops-reverser/api/v1alpha3" +) + +// A stored reconcileTemplate still naming {{.Revision}} must be refused with a SENTENCE. Without +// the tombstone method the render fails too, but with "can't evaluate field Revision in type +// git.ReconcileCommitMessageData" — which lands on a GitTarget's Validated condition and tells its +// owner nothing about what to write instead. The message is the whole point of the tombstone, so +// the assertion is on the message. +func TestValidateCommitConfig_RejectsRetiredRevisionWithBothSpellings(t *testing.T) { + config := ResolveCommitConfig(nil).WithTargetMessage(&v1alpha3.CommitMessageSpec{ + ReconcileTemplate: "chore: reconcile {{.Count}} at {{.Revision}}", + }) + + err := ValidateCommitConfig(config) + if err == nil { + t.Fatal("a reconcileTemplate naming the retired {{.Revision}} must be rejected") + } + for _, want := range []string{"{{.Revision}}", "{{.ResourceVersion}}"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q must name %s so the fix is readable from the condition", err, want) + } + } +} + +// A scan for the literal "{{.Revision}}" would miss every indirect spelling. The tombstone is a +// method, so the render fails wherever the field is reached from. +func TestValidateCommitConfig_RejectsRetiredRevisionIndirectly(t *testing.T) { + for name, template := range map[string]string{ + "with": "chore: reconcile {{.Count}}{{with .Revision}} at {{.}}{{end}}", + "pipeline": `chore: reconcile {{.Count}} at {{.Revision | printf "%s"}}`, + "variable": "{{$rv := .Revision}}chore: reconcile {{.Count}} at {{$rv}}", + "if": "chore: reconcile {{.Count}}{{if .Revision}} pinned{{end}}", + } { + t.Run(name, func(t *testing.T) { + config := ResolveCommitConfig(nil).WithTargetMessage(&v1alpha3.CommitMessageSpec{ + ReconcileTemplate: template, + }) + if err := ValidateCommitConfig(config); err == nil { + t.Errorf("%q reaches the retired field and must be rejected", template) + } + }) + } +} + +// The tombstone must not become a compatibility shim: honouring the old name would keep the wrong +// word alive indefinitely, which is the "accepted, and quietly carried on" upgrade failure the +// rejection exists to avoid. +func TestReconcileCommitMessageDataRevision_NeverReturnsTheValue(t *testing.T) { + value, err := ReconcileCommitMessageData{ResourceVersion: "1331"}.Revision() + if err == nil { + t.Fatal("the Revision tombstone must always error") + } + if value != "" { + t.Errorf("the tombstone returned %q: it must never carry the value under the old name", value) + } +} + +// The rename is a rename: the default subject's output is unchanged, so a target on the default +// template crosses the release without noticing. +func TestDefaultReconcileTemplate_SubjectUnchangedByTheRename(t *testing.T) { + scope := ResyncScopeFor( + schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, "team-a") + message, err := renderReconcileCommitMessage(4, "dest", &scope, "1331", ResolveCommitConfig(nil)) + if err != nil { + t.Fatalf("renderReconcileCommitMessage: %v", err) + } + const want = "chore: reconcile 4 deployments in team-a (last resourceVersion: 1331)" + if message != want { + t.Errorf("subject = %q, want %q", message, want) + } +} diff --git a/internal/git/resync_flush.go b/internal/git/resync_flush.go index 28002c45..3fe96269 100644 --- a/internal/git/resync_flush.go +++ b/internal/git/resync_flush.go @@ -86,7 +86,7 @@ func resyncHealKey(req *ResyncRequest) healKey { func (l *branchWorkerEventLoop) applyResync(req *ResyncRequest) { l.w.Log.Info("Handling resync request", "resources", len(req.Desired), - "revision", req.Revision, + "resourceVersion", req.ResourceVersion, "scope", req.Scope.String(), "heal", req.Heal, "gitTarget", req.GitTargetNamespace+"/"+req.GitTargetName, @@ -193,7 +193,7 @@ func (w *BranchWorker) buildResyncPendingWrite( return &PendingWrite{ Kind: PendingWriteResync, Desired: req.Desired, - Revision: req.Revision, + ResourceVersion: req.ResourceVersion, Scope: req.Scope, ResyncStats: stats, CommitConfig: ResolveCommitConfig(provider.Spec.Commit).WithTargetMessage(targetMetadata.CommitMessage), @@ -296,7 +296,7 @@ func (w *BranchWorker) executeResyncPendingWrite( // commitMetadata through the verbatim path. changed := stats.Created + stats.Updated + stats.Deleted rendered, err := renderReconcileCommitMessage( - changed, target.Name, pendingWrite.Scope, pendingWrite.Revision, pendingWrite.CommitConfig) + changed, target.Name, pendingWrite.Scope, pendingWrite.ResourceVersion, pendingWrite.CommitConfig) if err != nil { return 0, err } @@ -314,7 +314,7 @@ func (w *BranchWorker) executeResyncPendingWrite( log.FromContext(ctx).Info("git resync commit created", "created", stats.Created, "updated", stats.Updated, "deleted", stats.Deleted, "skipped", stats.Skipped, - "placementSkipped", stats.PlacementSkipped, "revision", pendingWrite.Revision) + "placementSkipped", stats.PlacementSkipped, "resourceVersion", pendingWrite.ResourceVersion) return 1, nil } diff --git a/internal/git/resync_push_test.go b/internal/git/resync_push_test.go index 60559ae8..7b69c1ea 100644 --- a/internal/git/resync_push_test.go +++ b/internal/git/resync_push_test.go @@ -62,7 +62,7 @@ func TestEnqueueResync_CoalescesSameScope(t *testing.T) { superseded := make(chan ResyncResult, 1) require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "1", + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "1", RefreshRemote: true, Result: superseded, })) @@ -71,7 +71,7 @@ func TestEnqueueResync_CoalescesSameScope(t *testing.T) { // accepted: it replaces the queued one rather than being dropped. newest := make(chan ResyncResult, 1) require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "2", + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "2", Result: newest, }), "a resync for an already-queued scope coalesces instead of being dropped") @@ -88,12 +88,12 @@ func TestEnqueueResync_CoalescesSameScope(t *testing.T) { item := <-w.eventQueue require.NotNil(t, item.Resync) current := w.takePendingResync(item.Resync) - assert.Equal(t, "2", current.Revision, "the marker runs the newest request for its scope") + assert.Equal(t, "2", current.ResourceVersion, "the marker runs the newest request for its scope") assert.True(t, current.RefreshRemote, "coalescing must preserve a pending remote refresh") // The key is cleared, so the next resync for that scope queues a fresh marker. require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "3", + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "3", Result: make(chan ResyncResult, 1), })) assert.Len(t, w.eventQueue, 1, "a scope taken off the queue can be queued again") @@ -108,18 +108,18 @@ func TestEnqueueResync_CoalescingCarriesTheSurvivingRequest(t *testing.T) { scope := ResyncScopeFor(configmapsGVRForScope, "team-a") require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "1", Scope: &scope, + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "1", Scope: &scope, SourceCell: cell, Result: make(chan ResyncResult, 1), })) require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "2", Scope: &scope, + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "2", Scope: &scope, SourceCell: cell, Result: make(chan ResyncResult, 1), })) item := <-w.eventQueue require.NotNil(t, item.Resync) current := w.takePendingResync(item.Resync) - assert.Equal(t, "2", current.Revision, + assert.Equal(t, "2", current.ResourceVersion, "the newer snapshot is the one that runs, at the marker's position") assert.Equal(t, cell, current.SourceCell, "and it still names the cell that gathered it") } @@ -285,7 +285,7 @@ func TestEnqueueResync_DoesNotCoalescePastQueuedWrites(t *testing.T) { first := make(chan ResyncResult, 1) require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "100", + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "100", Scope: scope, Result: first, })) @@ -293,7 +293,7 @@ func TestEnqueueResync_DoesNotCoalescePastQueuedWrites(t *testing.T) { require.True(t, w.Enqueue(liveEvent("target", "app"))) require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "103", + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "103", Scope: scope, Result: make(chan ResyncResult, 1), })) @@ -309,12 +309,12 @@ func TestEnqueueResync_DoesNotCoalescePastQueuedWrites(t *testing.T) { require.Len(t, w.eventQueue, 3, "the later resync takes its own slot rather than coalescing") firstMarker := <-w.eventQueue require.NotNil(t, firstMarker.Resync) - assert.Equal(t, "100", w.takePendingResync(firstMarker.Resync).Revision, + assert.Equal(t, "100", w.takePendingResync(firstMarker.Resync).ResourceVersion, "the earlier marker runs the snapshot it carried, not the newer one") assert.NotNil(t, (<-w.eventQueue).Request, "the write keeps its position between the snapshots") lastMarker := <-w.eventQueue require.NotNil(t, lastMarker.Resync) - assert.Equal(t, "103", w.takePendingResync(lastMarker.Resync).Revision) + assert.Equal(t, "103", w.takePendingResync(lastMarker.Resync).ResourceVersion) } // TestEnqueueResync_DoesNotCoalescePastQueuedAttach pins the same fence for a @@ -325,14 +325,14 @@ func TestEnqueueResync_DoesNotCoalescePastQueuedAttach(t *testing.T) { first := make(chan ResyncResult, 1) require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "100", + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "100", Result: first, })) w.EnqueueAttach(&AttachCommitRequest{ Namespace: "ns", Name: "cr", GitTargetNamespace: "ns", GitTargetName: "target", }) require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "103", + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "103", Result: make(chan ResyncResult, 1), })) @@ -356,7 +356,7 @@ func TestEnqueueResync_CoalescesPastUnrelatedWrites(t *testing.T) { superseded := make(chan ResyncResult, 1) require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "100", + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "100", Scope: scope, Result: superseded, })) @@ -365,7 +365,7 @@ func TestEnqueueResync_CoalescesPastUnrelatedWrites(t *testing.T) { require.True(t, w.Enqueue(liveEvent("elsewhere", "app"))) require.True(t, w.EnqueueResync(&ResyncRequest{ - GitTargetNamespace: "ns", GitTargetName: "target", Revision: "103", + GitTargetNamespace: "ns", GitTargetName: "target", ResourceVersion: "103", Scope: scope, Result: make(chan ResyncResult, 1), })) @@ -378,7 +378,7 @@ func TestEnqueueResync_CoalescesPastUnrelatedWrites(t *testing.T) { require.Len(t, w.eventQueue, 3, "coalescing still costs no extra FIFO slot") marker := <-w.eventQueue require.NotNil(t, marker.Resync) - assert.Equal(t, "103", w.takePendingResync(marker.Resync).Revision) + assert.Equal(t, "103", w.takePendingResync(marker.Resync).ResourceVersion) } // TestEnqueueResync_FenceHoldsUnderConcurrentWrites drives writes and resyncs for one @@ -464,7 +464,7 @@ func raceWritesAndResyncsOnOneScope(t *testing.T) (fifoTally, int, int) { defer wg.Done() if w.EnqueueResync(&ResyncRequest{ GitTargetNamespace: "ns", GitTargetName: "target", - Revision: strconv.Itoa(i), Scope: scope, Result: replies[i], + ResourceVersion: strconv.Itoa(i), Scope: scope, Result: replies[i], }) { resyncs.Add(1) } diff --git a/internal/git/types.go b/internal/git/types.go index 789c4cfb..14f4892e 100644 --- a/internal/git/types.go +++ b/internal/git/types.go @@ -3,6 +3,7 @@ package git import ( + "errors" "fmt" "sort" "strings" @@ -105,7 +106,7 @@ const ( DefaultReconcileCommitMessageTemplate = "chore: reconcile {{.Count}} " + "{{if .Resource}}{{.Resource}}{{else}}resources{{end}}" + "{{if .Namespace}} in {{.Namespace}}{{end}}" + - "{{if .Revision}} (last resourceVersion: {{.Revision}}){{end}}" + "{{if .ResourceVersion}} (last resourceVersion: {{.ResourceVersion}}){{end}}" // DefaultLiveCommitMessageTemplate describes retained input resources. DefaultLiveCommitMessageTemplate = "chore: sync {{.Count}} resource{{if ne .Count 1}}s{{end}}\n\n" + "{{range .Resources -}}" + @@ -289,9 +290,12 @@ type PendingWrite struct { // sweep), and no sibling type's — nor, for a namespace-scoped resync, any sibling // namespace's — document is ever dropped. Nil is the whole-GitTarget resync. Scope *ResyncScope - // Revision is the cluster snapshot resourceVersion the desired set is pinned to - // (the joined streaming-watch bookmark). Carried for diagnostics and logging. - Revision string + // ResourceVersion is the cluster snapshot's resourceVersion the desired set is pinned to + // (the joined streaming-watch bookmark) — the COLLECTION's version, not any one object's. + // Carried for diagnostics, logging, and the reconcile commit message. + // + // Not "Revision": in this codebase a revision is a Git commit (see LayoutReport.Revision). + ResourceVersion string // ResyncStats, when non-nil, is populated during apply with the plan's // create/update/delete/skip counts so a synchronous caller can report them. ResyncStats *ResyncStats @@ -385,14 +389,14 @@ func (s *ResyncScope) Matches(ri types.ResourceIdentifier) bool { } // ResyncRequest is a synchronous resync of one GitTarget against a complete, -// revision-pinned desired snapshot (M8). It rides the worker queue so the single +// version-pinned desired snapshot (M8). It rides the worker queue so the single // git-mutating goroutine applies it in order with live events, and replies on // Result once the local commit is created. The desired set is the whole watched -// resource state at Revision; the worker's content-derived mark-and-sweep drops +// resource state at ResourceVersion; the worker's content-derived mark-and-sweep drops // any managed document the snapshot did not contain. type ResyncRequest struct { Desired []manifestanalyzer.DesiredResource - Revision string + ResourceVersion string GitTargetName string GitTargetNamespace string // Scope, when set, makes this a per-type (M12) reconcile/sweep: the mark-and-sweep is @@ -513,6 +517,28 @@ type Event struct { // Identifier contains resource identification information. Identifier types.ResourceIdentifier + // ResourceVersion is the metadata.resourceVersion of the observed state this event + // describes. It is PROVENANCE, never content: sanitize strips resourceVersion from Object + // on purpose (a version in a committed manifest makes every observation a byte change), so + // this field carries the fact BESIDE the object rather than inside it. Never write it back + // onto Object. + // + // Stamped where the object is observed — the live watch holds the unsanitized object there + // and nowhere downstream does. Producers with no observed object (reconcile, bootstrap) + // leave it empty. + // + // Unlike the object-derived fields, a DELETE CAN carry one: the watch Deleted frame + // delivers the final object, so this names the last version that existed. + ResourceVersion string + + // Generation is the metadata.generation of the observed state — the DESIRED state's counter, + // where ResourceVersion counts every write. Same provenance-not-content rule: sanitize strips + // generation from Object one line after resourceVersion, and for the same reason. + // + // 0 means "no generation", which covers both "this producer observed nothing" and "this kind + // has none": a ConfigMap or a Secret never carries one, only types with a spec do. + Generation int64 + // Operation is the admission operation (CREATE, UPDATE, DELETE). Operation string @@ -602,7 +628,7 @@ type CommitMessageConfig struct { // ReconcileCommitMessageData is the template context for reconcile commit messages. // // Group, Version, Resource and APIVersion describe the reconciled type, and are -// populated only for a per-type reconcile. Revision is the resourceVersion the desired set was +// populated only for a per-type reconcile. ResourceVersion is the version the desired set was // pinned to. Any template referencing these must render cleanly when absent; the default guards // both with {{if}}. type ReconcileCommitMessageData struct { @@ -612,12 +638,41 @@ type ReconcileCommitMessageData struct { Version string Resource string APIVersion string - Revision string + // ResourceVersion is THE COLLECTION'S metadata.resourceVersion — the LIST the desired set + // was folded from, not any one object's. A reconcile describes a type, so the version it + // can name is the snapshot's. (The per-object counterpart is + // LiveCommitMessageData.Resources[i].ResourceVersion, which a live commit carries because + // it names n resources.) + // + // Empty for a pure sweep. It was called Revision until v0.48.0; see the Revision tombstone + // below for why the word moved. + ResourceVersion string // Namespace is the single source namespace a namespace-scoped reconcile covered, and // is empty for a whole-target or all-namespaces reconcile. Namespace string } +// Revision is the retired name of ResourceVersion, kept ONLY so a stored template that still +// says {{.Revision}} is refused with a sentence instead of a text/template internal +// ("can't evaluate field Revision in type git.ReconcileCommitMessageData"). +// +// It is a method rather than a scan of the template source because a method catches every +// spelling — {{with .Revision}}, {{.Revision | printf "%s"}}, assignment to a variable — which a +// search for the literal string would not. Same argument validateRequestTemplate makes for +// probing behaviour rather than matching template text. +// +// It deliberately does NOT return the value. Silently honouring the old name would keep the +// wrong word alive indefinitely, and "accepted, and quietly carried on" is the upgrade failure +// mode docs/UPGRADING.md is written against. Reject, do not prune. +// +// Delete one minor release after v0.48.0. By then {{.Revision}} can go back to failing as a +// plain unknown field, which is all an unknown field deserves. +func (ReconcileCommitMessageData) Revision() (string, error) { + return "", errors.New( + "reconcileTemplate: {{.Revision}} was renamed to {{.ResourceVersion}} in v0.48.0 " + + "(it is the snapshot's resourceVersion; \"revision\" now only ever means a Git commit)") +} + // ResourceRef is the lightweight resource identifier emitted to grouped commit // templates via LiveCommitMessageData.Resources. // @@ -650,6 +705,32 @@ type ResourceRef struct { // // Read it with .Label, not with .Labels.key — see Label. Labels map[string]string + // ResourceVersion is the metadata.resourceVersion of the observed state this commit wrote, + // and "" for a producer that observed none (reconcile, bootstrap). Guard it with + // {{with .ResourceVersion}} — empty here has one meaning, "this producer observed no + // version", and no sentinel states that better than silence. + // + // It is NOT the object's current version, and the gap is deliberate: an update whose + // git-writable content is unchanged (a /status-only write) is never routed, so this value + // lags the cluster's. Compare it for EQUALITY — equal means nothing is pending — never by + // subtraction: resourceVersion is opaque by contract and is the global store revision in + // practice, so a gap measures other objects' writes, not our own drops. The census + // counts those: watch_events_total{outcome="unchanged"}. + // + // Unlike Kind and Labels, a DELETE DOES carry one — see Event.ResourceVersion. + ResourceVersion string + // Generation is the metadata.generation of the state this commit wrote, and 0 when there is + // none. It moves only when the DESIRED state changes, so unlike ResourceVersion it is worth + // comparing: it is per object, starts at 1, and advances once per spec write, so a gap + // between two commits really does mean spec changes that were not committed separately. + // + // Two blind spots keep it from replacing ResourceVersion. A ConfigMap, a Secret, and any + // other kind without a spec never carry one, so this stays 0 for much of what a target + // mirrors. And a label- or annotation-only edit changes what gets committed WITHOUT moving + // it, so an unchanged Generation does not mean an unchanged commit. + // + // Guard it with {{with .Generation}}, which renders nothing for 0. + Generation int64 } // Label is the value of one label on this resource, and "" when it does not carry the label. diff --git a/internal/watch/event_router.go b/internal/watch/event_router.go index 390c81f1..c32f5626 100644 --- a/internal/watch/event_router.go +++ b/internal/watch/event_router.go @@ -194,13 +194,14 @@ func (r *EventRouter) resolveWorkerForGitDest( // worker defers while a commit window is open. enqueued is false when the worker's queue was full // and dropped the request (its failure is still delivered on resultCh for the drain to record). // refreshRemote asks the worker to fetch the latest remote tip before it inspects the folder. +// resourceVersion is the LIST's collection version the desired set is pinned to. func (r *EventRouter) enqueueScopedResync( ctx context.Context, gitDest types.ResourceReference, scope git.ResyncScope, sourceCell types.CellKey, desired []manifestanalyzer.DesiredResource, - revision string, + resourceVersion string, heal bool, refreshRemote bool, ) (chan git.ResyncResult, bool, error) { @@ -211,7 +212,7 @@ func (r *EventRouter) enqueueScopedResync( resultCh := make(chan git.ResyncResult, 1) enqueued := worker.EnqueueResync(&git.ResyncRequest{ Desired: desired, - Revision: revision, + ResourceVersion: resourceVersion, GitTargetName: gitDest.Name, GitTargetNamespace: gitDest.Namespace, Scope: &scope, diff --git a/internal/watch/target_watch.go b/internal/watch/target_watch.go index ed8efb25..695d6600 100644 --- a/internal/watch/target_watch.go +++ b/internal/watch/target_watch.go @@ -810,11 +810,11 @@ func (m *Manager) targetWatchListAndStream( return fmt.Errorf("list target watch snapshot %s/%q: %w", stream.key.GVR.String(), stream.key.Namespace, err) } desired := desiredFromList(stream.key.GVR, list) - revision := list.GetResourceVersion() - if err := m.enqueueReplayResync(ctx, log, gitDest, stream, desired, revision); err != nil { + resourceVersion := list.GetResourceVersion() + if err := m.enqueueReplayResync(ctx, log, gitDest, stream, desired, resourceVersion); err != nil { return err } - if err := m.recordTargetWatchCursor(ctx, gitDest, stream.key, revision); err != nil { + if err := m.recordTargetWatchCursor(ctx, gitDest, stream.key, resourceVersion); err != nil { return err } // Recorded HERE, not where the fallback was chosen. watch_recovery_total counts recoveries that @@ -824,7 +824,7 @@ func (m *Manager) targetWatchListAndStream( m.recordWatchRecovery(gitDest, stream.key.GVR.Group, stream.key.GVR.Resource, recoveryModeListFallback) log.Info("target watch list fallback complete", "gitDest", gitDest.String(), "gvr", stream.key.GVR.String(), "namespace", stream.key.Namespace, - "count", len(desired), "resourceVersion", revision) + "count", len(desired), "resourceVersion", resourceVersion) m.markTargetStreamState( gitDest, stream.key.Cell(), @@ -832,7 +832,7 @@ func (m *Manager) targetWatchListAndStream( StreamReasonAllStreamsReady, "target watch list fallback complete", ) - return m.streamLiveTargetWatchEvents(ctx, log, gitDest, stream, buffered, revision) + return m.streamLiveTargetWatchEvents(ctx, log, gitDest, stream, buffered, resourceVersion) } func (m *Manager) handleTargetWatchSessionEvent( @@ -916,7 +916,7 @@ func (m *Manager) enqueueReplayResync( gitDest types.ResourceReference, stream targetWatchStream, desired []manifestanalyzer.DesiredResource, - revision string, + resourceVersion string, ) error { if m.EventRouter == nil { return nil @@ -930,13 +930,14 @@ func (m *Manager) enqueueReplayResync( return nil default: } - // The revision is the one this stream was STARTED with, never the cell's current revision. - // A cancelled stream can still be in flight with a replay result, and reading the revision - // here would let it report a scope clean under a revision it never replayed for — reopening - // writes on the strength of a snapshot the new plan never gathered. The gate already - // ignores a superseded revision; capturing it at start is what makes it stale. + // The stream's PLAN revision (stream.revision, a render-fidelity generation counter — not the + // resourceVersion this snapshot is pinned to) is the one this stream was STARTED with, never + // the cell's current one. A cancelled stream can still be in flight with a replay result, and + // reading that revision here would let it report a scope clean under a revision it never + // replayed for — reopening writes on the strength of a snapshot the new plan never gathered. + // The gate already ignores a superseded revision; capturing it at start is what makes it stale. resultCh, enqueued, err := m.EventRouter.enqueueScopedResync( - ctx, gitDest, resyncScopeForWatchKey(stream.key), stream.sourceCell(), desired, revision, false, + ctx, gitDest, resyncScopeForWatchKey(stream.key), stream.sourceCell(), desired, resourceVersion, false, stream.refreshRemote) if err != nil { return err @@ -958,7 +959,8 @@ func (m *Manager) enqueueReplayResync( stream.key.GVR.String(), gitDest.String(), git.ErrFinalizeQueueFull) } log.V(1).Info("target replay resync enqueued", - "gitDest", gitDest.String(), "gvr", stream.key.GVR.String(), "revision", revision, "count", len(desired)) + "gitDest", gitDest.String(), "gvr", stream.key.GVR.String(), + "resourceVersion", resourceVersion, "count", len(desired)) return nil } @@ -1251,10 +1253,24 @@ func sanitizedContentHash(event *git.Event) (string, bool) { return string(sum[:]), true } +// targetWatchGitEvent adapts one observed object into the Event the pipeline carries. +// +// The resourceVersion and generation are stamped here BECAUSE this is where the unsanitized +// object is: sanitize strips both from the object below (a version inside a committed manifest +// makes every observation a byte change), and nothing downstream can recover them — by the time +// the event reaches a branch worker the object is out of the process and the cluster has moved +// on. They are capture-at-the-seam facts, like the author. +// +// They are stamped for a DELETE too, where Object deliberately is not: the watch Deleted frame +// delivers the final object, so the last state that existed is knowable even though the object +// no longer is. func targetWatchGitEvent(gvr schema.GroupVersionResource, u *unstructured.Unstructured, op string) git.Event { event := git.Event{ - Identifier: types.NewResourceIdentifier(gvr.Group, gvr.Version, gvr.Resource, u.GetNamespace(), u.GetName()), - Operation: op, + Identifier: types.NewResourceIdentifier( + gvr.Group, gvr.Version, gvr.Resource, u.GetNamespace(), u.GetName()), + Operation: op, + ResourceVersion: u.GetResourceVersion(), + Generation: u.GetGeneration(), } if op != string(configv1alpha3.OperationDelete) { event.Object = sanitize.Sanitize(u) diff --git a/internal/watch/target_watch_resource_version_test.go b/internal/watch/target_watch_resource_version_test.go new file mode 100644 index 00000000..cf978b0e --- /dev/null +++ b/internal/watch/target_watch_resource_version_test.go @@ -0,0 +1,104 @@ +// SPDX-License-Identifier: Apache-2.0 + +package watch + +import ( + "testing" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +func observedDeployment(resourceVersion string, generation int64) *unstructured.Unstructured { + return &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]any{ + "name": "api", "namespace": "prod", "resourceVersion": resourceVersion, + "generation": generation, + "uid": "2f1c8f1e-0000-4000-8000-000000000001", + }, + "spec": map[string]any{"replicas": int64(2)}, + }} +} + +func deploymentsGVR() schema.GroupVersionResource { + return schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"} +} + +// The watch seam is the only place the unsanitized object exists — by the time the event reaches a +// branch worker the object is out of the process and the cluster has moved on. So the counters are +// stamped here or they are lost, exactly like the author. +func TestTargetWatchGitEvent_StampsObservedResourceVersion(t *testing.T) { + event := targetWatchGitEvent(deploymentsGVR(), observedDeployment("20001", 3), "UPDATE") + + if event.ResourceVersion != "20001" { + t.Errorf("event.ResourceVersion = %q, want the observed 20001", event.ResourceVersion) + } + if event.Generation != 3 { + t.Errorf("event.Generation = %d, want the observed 3", event.Generation) + } + if event.Object == nil { + t.Fatal("an UPDATE must carry the sanitized object") + } + if got := event.Object.GetResourceVersion(); got != "" { + t.Errorf("the object the writer commits still carries resourceVersion %q", got) + } + if got := event.Object.GetGeneration(); got != 0 { + t.Errorf("the object the writer commits still carries generation %d", got) + } +} + +// A DELETE carries no object — the resource is gone — but the Deleted frame still delivers the +// final one, so the last version that existed IS knowable. This is where the version parts company +// with Kind and Labels, which stay empty for a delete. +func TestTargetWatchGitEvent_DeleteCarriesTheFinalResourceVersion(t *testing.T) { + event := targetWatchGitEvent(deploymentsGVR(), observedDeployment("20003", 4), "DELETE") + + if event.Object != nil { + t.Error("a DELETE must carry no object") + } + if event.ResourceVersion != "20003" { + t.Errorf("event.ResourceVersion = %q, want the final 20003", event.ResourceVersion) + } + if event.Generation != 4 { + t.Errorf("event.Generation = %d, want the final 4", event.Generation) + } +} + +// A ConfigMap is the case that matters for Generation: the API server gives spec-less kinds none, +// and this operator mirrors them constantly. The field stays 0 rather than inventing a value, and +// templates guard it with {{with}}. The resourceVersion is still there, which is the whole reason +// the two counters are carried separately. +func TestTargetWatchGitEvent_SpeclessKindCarriesNoGeneration(t *testing.T) { + configMap := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]any{"name": "cm", "namespace": "prod", "resourceVersion": "20005"}, + "data": map[string]any{"key": "value"}, + }} + gvr := schema.GroupVersionResource{Version: "v1", Resource: "configmaps"} + + event := targetWatchGitEvent(gvr, configMap, "CREATE") + if event.Generation != 0 { + t.Errorf("Generation = %d, want 0 for a kind that has none", event.Generation) + } + if event.ResourceVersion != "20005" { + t.Errorf("ResourceVersion = %q, want it present even without a generation", event.ResourceVersion) + } +} + +// An object the API server gave neither counter leaves both at their zero value. +func TestTargetWatchGitEvent_NothingObservedStaysEmpty(t *testing.T) { + bare := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]any{"name": "api", "namespace": "prod"}, + }} + + event := targetWatchGitEvent(deploymentsGVR(), bare, "CREATE") + if event.ResourceVersion != "" || event.Generation != 0 { + t.Errorf("counters = %q/%d, want both empty when nothing was observed", + event.ResourceVersion, event.Generation) + } +} diff --git a/test/e2e/signing_e2e_test.go b/test/e2e/signing_e2e_test.go index 4a811523..294dd510 100644 --- a/test/e2e/signing_e2e_test.go +++ b/test/e2e/signing_e2e_test.go @@ -394,10 +394,11 @@ var _ = Describe("Commit Signing", Label("signing"), Ordered, func() { watchRuleName := providerName + "-wr" commitPath := "e2e/signing-reconcile" // The reconcile template names the synced type ({{.APIVersion}}/{{.Resource}}) and pins the - // {{.Revision}} so the per-type reconcile commits are self-describing — the §9 "name the synced - // type" improvement in docs/finished/signing-snapshot-tail-replay-failure-investigation.md. + // {{.ResourceVersion}} so the per-type reconcile commits are self-describing — the §9 "name the + // synced type" improvement in docs/finished/signing-snapshot-tail-replay-failure-investigation.md + // (which predates the rename and still writes it as {{.Revision}}). customReconcileTemplate := "e2e-reconcile: synced {{.Count}} {{.APIVersion}}/{{.Resource}}" + - "@{{.Revision}} to {{.GitTarget}}" + "@{{.ResourceVersion}} to {{.GitTarget}}" DeferCleanup(func() { if skipCleanupBecauseResourcesArePreserved( @@ -452,7 +453,17 @@ var _ = Describe("Commit Signing", Label("signing"), Ordered, func() { By("recreating the GitTarget now that the WatchRule is active to force a fresh reconcile batch") cleanupGitTarget(destName, testNs) - createValidatedGitTarget(destName, testNs, providerName, commitPath) + // The recreated target must carry the SAME reconcile template. It is the batch commit this + // spec asserts on, and the recreation exists only to re-trigger the reconcile that produces + // it — recreating with the default template instead makes the two targets differ in the one + // way the assertion reads. Today the spec still passes, because the fresh reconcile finds + // the batch already committed byte-identically and writes nothing, so the commit under + // assertion is the pre-recreation one. That is a trap, not a design: the moment the + // recreated reconcile has anything to write, the subject becomes "chore: reconcile ..." and + // the HavePrefix("e2e-reconcile:") assertion below fails for a reason that has nothing to + // do with what this spec tests. + createValidatedGitTargetWithCommitMessage(destName, testNs, providerName, commitPath, + gitTargetCommitOptions{ReconcileTemplate: customReconcileTemplate}) By("waiting for the batch commit and verifying its message uses the batch template") Eventually(func(g Gomega) { @@ -553,7 +564,7 @@ var _ = Describe("Commit Signing", Label("signing"), Ordered, func() { By("creating target A and its WatchRule, then waiting for A to reconcile the seed band") createValidatedGitTargetWithCommitMessage(destNameA, testNs, providerName, commitPathA, gitTargetCommitOptions{ReconcileTemplate: "e2e-reconcile: synced {{.Count}} " + - "{{.APIVersion}}/{{.Resource}}@{{.Revision}} to {{.GitTarget}}"}) + "{{.APIVersion}}/{{.Resource}}@{{.ResourceVersion}} to {{.GitTarget}}"}) Expect(applyFromTemplate("test/e2e/templates/watchrule.tmpl", struct { Name, Namespace, DestinationName string }{watchRuleNameA, testNs, destNameA}, testNs)).To(Succeed()) @@ -580,7 +591,7 @@ var _ = Describe("Commit Signing", Label("signing"), Ordered, func() { Expect(err).NotTo(HaveOccurred(), "failed to create overlap-b configmaps") createGitTargetWithCommitMessage(destNameB, testNs, providerName, commitPathB, "main", gitTargetCommitOptions{ReconcileTemplate: "e2e-reconcile: synced {{.Count}} " + - "{{.APIVersion}}/{{.Resource}}@{{.Revision}} to {{.GitTarget}}"}) + "{{.APIVersion}}/{{.Resource}}@{{.ResourceVersion}} to {{.GitTarget}}"}) Expect(applyFromTemplate("test/e2e/templates/watchrule.tmpl", struct { Name, Namespace, DestinationName string }{watchRuleNameB, testNs, destNameB}, testNs)).To(Succeed())