From 1481f7e7e2e4d0a309a01402a426e82012502072 Mon Sep 17 00:00:00 2001 From: Niklas Voss Date: Mon, 21 Sep 2026 12:08:46 +0200 Subject: [PATCH] feat(apiserver): opt-in metadata.generation semantics in DefaultStrategy DefaultStrategy never touched metadata.generation, and neither does the upstream machinery around it: BeforeCreate sets no generation at all, and BeforeUpdate only copies the stored value forward so clients cannot forge it. Upstream leaves the increment to each resource's own strategy, so a resource served through this kit stayed at generation 0 for its whole life. That is quietly destructive for controllers, because it makes every `status.observedGeneration == metadata.generation` comparison trivially true: an idempotence short-circuit written against it latches after the first reconcile and the resource is never reconciled for a spec edit again. It is also invisible to envtest, which serves real CRDs through the real apiserver and does increment generation, so the behaviour only diverges once a resource is served from an aggregated server built with this kit. A resource opts in by implementing GenerationTracker, which answers the one question a generic strategy cannot: whether its spec changed. Implementations compare their own typed spec, so there is no reflection and no guessing at which field is the "spec". Opt-in rather than automatic because turning it on changes what every controller watching a resource observes. Only a spec change increments: metadata-only edits must not make controllers re-reconcile. Status updates never reach this path, because the status subresource installs a strategy that calls only its own override. --- apiserver/rest/interface.go | 30 +++++++++++++ apiserver/rest/strategy.go | 20 +++++++++ apiserver/rest/strategy_test.go | 74 +++++++++++++++++++++++++++++++++ 3 files changed, 124 insertions(+) diff --git a/apiserver/rest/interface.go b/apiserver/rest/interface.go index f39ccc0..744fa3d 100644 --- a/apiserver/rest/interface.go +++ b/apiserver/rest/interface.go @@ -70,6 +70,36 @@ type PrepareForUpdater interface { PrepareForUpdate(ctx context.Context, old runtime.Object) } +// GenerationTracker opts a resource into Kubernetes generation semantics: DefaultStrategy sets +// metadata.generation to 1 on create and increments it on every update that changes the spec. +// +// This is opt-in rather than automatic because a generic strategy cannot know which part of an +// arbitrary runtime.Object is its "spec", and because turning it on changes what every controller +// watching the resource observes. Upstream Kubernetes does the same work per resource: BeforeCreate +// sets no generation at all, and BeforeUpdate only copies the stored value forward so clients +// cannot forge it, leaving the increment to each resource's own strategy. A resource that does not +// implement this interface therefore stays at generation 0 for its whole life — which silently +// makes every `status.observedGeneration == metadata.generation` comparison a controller performs +// trivially true, so idempotence short-circuits latch and spec edits are never acted on. +// +// Implementations compare their own typed spec, which is exact and needs no reflection: +// +// func (o *Widget) SpecChanged(old runtime.Object) bool { +// p, ok := old.(*Widget) +// +// return !ok || !apiequality.Semantic.DeepEqual(o.Spec, p.Spec) +// } +// +// Only the spec counts. Metadata-only edits (labels, annotations) must not make controllers +// re-reconcile, and status updates never reach this path: the status subresource installs a +// strategy that calls only its own override. +type GenerationTracker interface { + // SpecChanged reports whether this object's spec differs from old's. It must report true + // when old is not of the same type, so an unexpected pairing is treated as a change rather + // than silently skipping the increment. + SpecChanged(old runtime.Object) bool +} + // TableConverter implements an adapted version of rest.TableConverter // it can be used by objects to override DefaultStrategy behaviour. type TableConverter interface { diff --git a/apiserver/rest/strategy.go b/apiserver/rest/strategy.go index 6524d62..ace6115 100644 --- a/apiserver/rest/strategy.go +++ b/apiserver/rest/strategy.go @@ -82,15 +82,24 @@ func (d DefaultStrategy) NamespaceScoped() bool { } // PrepareForCreate normalizes the object before creation, delegating to PrepareForCreater if implemented. +// A resource that implements GenerationTracker also starts at metadata.generation 1. func (DefaultStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { if v, ok := obj.(PrepareForCreater); ok { v.PrepareForCreate(ctx) } + // BeforeCreate sets no generation, so a resource that tracks it starts here. See + // GenerationTracker for why this is opt-in. + if _, ok := obj.(GenerationTracker); ok { + if m, err := meta.Accessor(obj); err == nil { + m.SetGeneration(1) + } + } } // PrepareForUpdate normalizes the object before update. // If the object has a status subresource, status is copied from old to new. // If PrepareForUpdater is implemented, it is called to further normalize. +// If GenerationTracker is implemented, metadata.generation is incremented when the spec changed. func (DefaultStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { if v, ok := obj.(resource.ObjectWithStatusSubResource); ok { // Copy status from old to new to avoid spec-only updates modifying status. @@ -99,6 +108,17 @@ func (DefaultStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Ob if v, ok := obj.(PrepareForUpdater); ok { v.PrepareForUpdate(ctx, old) } + // BeforeUpdate has already copied the stored generation onto the incoming object, and it + // calls this before persisting, so an increment here is what lands. Only a spec change + // counts; status updates never reach this method, because the status subresource installs a + // strategy that calls only its own override. + if v, ok := obj.(GenerationTracker); ok && v.SpecChanged(old) { + newMeta, newErr := meta.Accessor(obj) + oldMeta, oldErr := meta.Accessor(old) + if newErr == nil && oldErr == nil { + newMeta.SetGeneration(oldMeta.GetGeneration() + 1) + } + } } // Validate delegates to the object's Validater interface if present, otherwise returns no errors. diff --git a/apiserver/rest/strategy_test.go b/apiserver/rest/strategy_test.go index 1d648d7..30358cc 100644 --- a/apiserver/rest/strategy_test.go +++ b/apiserver/rest/strategy_test.go @@ -295,3 +295,77 @@ var _ = Describe("PrepareForUpdaterStrategy", func() { Expect(func() { s.PrepareForUpdate(context.Background(), obj, old) }).ToNot(Panic()) }) }) + +// genObj opts into generation semantics. It deliberately does NOT implement +// resource.ObjectWithStatusSubResource, so these specs exercise the generation path alone. +type genObj struct { + metav1.TypeMeta + metav1.ObjectMeta + Spec string +} + +func (g *genObj) DeepCopyObject() runtime.Object { + if g == nil { + return nil + } + clone := *g + + return &clone +} + +func (g *genObj) GetObjectMeta() *metav1.ObjectMeta { return &g.ObjectMeta } + +// SpecChanged implements GenerationTracker. +func (g *genObj) SpecChanged(old runtime.Object) bool { + o, ok := old.(*genObj) + + return !ok || g.Spec != o.Spec +} + +var _ = Describe("DefaultStrategy generation semantics", func() { + // Upstream BeforeCreate never sets a generation and BeforeUpdate only copies the stored one + // forward, so incrementing it is each strategy's job. Without this a resource sits at + // generation 0 forever and every status.observedGeneration == metadata.generation test a + // controller makes is trivially true. + It("starts an opted-in resource at generation 1 on create", func() { + obj := &genObj{Spec: "a"} + DefaultStrategy{}.PrepareForCreate(context.Background(), obj) + Expect(obj.GetGeneration()).To(Equal(int64(1))) + }) + + It("leaves a resource that has not opted in at generation 0", func() { + obj := &testObj{} + DefaultStrategy{}.PrepareForCreate(context.Background(), obj) + Expect(obj.GetGeneration()).To(Equal(int64(0))) + }) + + It("increments the generation when the spec changes", func() { + old := &genObj{Spec: "a"} + old.SetGeneration(7) + // BeforeUpdate has already copied the stored generation onto the incoming object. + obj := &genObj{Spec: "b"} + obj.SetGeneration(7) + DefaultStrategy{}.PrepareForUpdate(context.Background(), obj, old) + Expect(obj.GetGeneration()).To(Equal(int64(8))) + }) + + // A label or annotation edit must not make every controller re-reconcile. + It("leaves the generation alone when the spec is unchanged", func() { + old := &genObj{Spec: "a"} + old.SetGeneration(7) + obj := &genObj{Spec: "a"} + obj.SetGeneration(7) + obj.SetLabels(map[string]string{"k": "v"}) + DefaultStrategy{}.PrepareForUpdate(context.Background(), obj, old) + Expect(obj.GetGeneration()).To(Equal(int64(7))) + }) + + It("leaves a resource that has not opted in alone on update", func() { + obj := &testObj{Status: "new"} + old := &testObj{Status: "old"} + old.SetGeneration(3) + obj.SetGeneration(3) + DefaultStrategy{}.PrepareForUpdate(context.Background(), obj, old) + Expect(obj.GetGeneration()).To(Equal(int64(3))) + }) +})