You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(component): stop FlushStatus conflict retries rolling back shared status - #240
When two controllers share the status of one CR, a 409 retry of FlushStatus writes the stale copy of the other controller's non-condition fields back, for example status.observedGeneration. This PR adds component.WithOwnedStatusFields(paths...). With it, every write of FlushStatus is a JSON merge patch of the status that holds only the declared fields and the conditions, guarded by the staged resourceVersion, so another writer's fields are never sent. Without the option, FlushStatus behaves exactly as before. The framework does not check a declared path against the CRD schema: with a structural schema, the API server prunes a misspelled field, so that part of the write does nothing.
Changes
FlushStatus(ctx, rec, comps, opts ...FlushOption): new variadic options. Existing calls compile and keep the Status().Update path unchanged.
WithOwnedStatusFields(paths ...string): names the non-condition status fields that the flush owns, by JSON name under status with dots for nested fields ("observedGeneration", "summary.ready"). With no paths, the flush owns no non-condition field. Paths from several options are combined.
With the option, each attempt first reads the stored status (an unstructured read through APIReader when set), then sends client.RawPatch(types.MergePatchType, body) to the status subresource. The body holds:
metadata.resourceVersion of the staged owner, as the optimistic lock;
each declared path with the value from the staged owner's JSON, or null when the staged owner does not carry it and the server does. A declared object is owned whole: it carries null for every stored child that the staged owner lacks, including a stored zero that a typed read would drop;
the conditions list under its JSON path, as staged. On the first attempt the resourceVersion precondition ensures that the list read at the top of the reconcile is still current. After a 409, the unowned conditions are refreshed from the server first.
A declared path below another declared path is dropped, so the patch does not depend on the order of the paths.
The read before each attempt also compares the stored uid with the staged owner's. A merge patch has no uid precondition (the API server ignores a uid in the body), so this check stops a retry from patching an owner that was deleted and recreated under the same name.
On a 409, the existing refreshUnownedConditions updates the staged resourceVersion and unowned conditions. The stored status is read again, the patch is rebuilt, and the write retries under retry.RetryOnConflict.
The JSON path of the conditions comes from the owner itself: GetStatusConditions decides where they live. FlushStatus stages a marker condition for a moment, encodes the owner, and finds the marker in the JSON. This needs no reflection and works whatever the field is called.
Path validation checks syntax only. It rejects an empty segment and a path that is, contains, or lies below the conditions. A declared path that runs below a non-object value in the staged owner also fails before any write.
After a successful write, the controller-runtime client decodes the server's response into rec.Owner, replacing the object. A staged value of an undeclared field does not survive in memory. An envtest run confirmed that no stale field remains.
The FlushStatus GoDoc states which status fields a conflict retry takes from the server: the resourceVersion and the unowned conditions, nothing else.
Docs: docs/component.md gets a new section, "Sharing the owner's status with another controller", plus corrected conflict and write descriptions. docs/guidelines.md and the building-components and structuring-operators plugin skills get the shared-status guidance. The plugin references are synced.
Challenges
A merge patch replaces a list as a whole, so the conditions cannot be patched by type. The patch carries the full conditions list. The resourceVersion precondition is what makes that safe: the list matches the server's at the moment the patch applies, except for the owned types, and a concurrent change makes the patch fail with a 409 and refresh. Server-side apply would avoid the full list, but a CRD's conditions list is atomic unless the schema marks it +listType=map. On most CRDs an apply would replace the other writers' conditions.
Testing
New envtest specs in pkg/component/flush_owned_fields_test.go. They run against a real API server, because merge-patch null removal, the resourceVersion precondition and schema pruning are server behavior. They cover:
the issue's scenario: a second writer changes observedGeneration between the read and the write. The first patch conflicts, the retry succeeds, and the server keeps the second writer's value;
the first attempt sends only metadata.resourceVersion, the declared field and the conditions, even with no conflict, and an undeclared staged field is not written;
a declared field that the staged owner cleared is removed on the server;
a nested path, with its sibling left to the server;
a wholly declared object whose child the staged owner dropped (with and without a real conflict, and for a stored zero), and a staged unowned condition that is written as staged without a conflict;
a declared child whose parent the server does not hold leaves the parent absent;
an owner deleted and recreated under the same name is not written, and the flush returns an error;
the in-memory owner after the write holds the server's state, not undeclared staged values;
owned conditions win, unowned conditions changed or added by another writer survive the retry;
a misspelled path does nothing, because the structural schema prunes it;
a path below a scalar fails before any write;
invalid path syntax (empty, empty segment, leading or trailing dot, the conditions, below the conditions) is rejected before any write.
Unit tests cover the conditions path lookup (including conditions nested under another JSON name), the overlap rejection, order independence of overlapping declarations, and absent or null parents. The existing FlushStatus tests cover the default path, unchanged. I confirmed that the envtest specs fail when the option is ignored, and that the removal spec fails when null is not sent. make all passes.
…nflict retry
A 409 retry of FlushStatus wrote the staged owner again. It took only the
resourceVersion and the unowned conditions from the server, so the stale
copy of a non-condition field that another controller owns, such as
status.observedGeneration, was written back over the newer value.
Add the WithOwnedStatusFields option. It names the non-condition status
fields that the flush owns, by JSON path under status. With the option, a
retry keeps the staged value only for those fields and takes every other
non-condition field from the server, absence included. FlushStatus checks
each path against the owner's Go type before it writes.
Without the option the behavior does not change, so a controller that
stages its own observedGeneration does not lose it on a conflict.
Fixes#238
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Adds opt-in status-field ownership to FlushStatus, protecting other controllers’ non-condition fields during conflict retries while preserving existing default behavior.
Changes:
Adds WithOwnedStatusFields with nested-path validation and ownership-aware retries.
Adds conflict and validation tests.
Updates shared-status documentation and plugin guidance.
…arents
Path validation took the field of an embedded struct even when the
struct declared its own field with the same JSON name, so it could
check a path against the wrong type. Validation now collects every field
with the segment's JSON name, directly or through embedded structs, and
rejects the path when more than one matches, instead of copying the
encoding/json tie-break rules.
A nullable parent without omitempty encodes as null, and SetNestedField
cannot descend through it. A conflict retry with an owned path below
such a parent failed. A null parent now counts as absent when the
staged value is read and is replaced before the owned value is set.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review round 1 (Copilot, effort Balanced, review of 491dd65). Fixes are in 824f0a3.
Finding
Verdict
Action
Embedded-field shadowing in path validation (status_fields.go:125)
Correct
Validation now rejects a path segment that matches more than one field, directly or through embedded structs. It does not copy the encoding/json tie-break rules. Covered by TestJSONFields.
Null intermediate parent on a conflict retry (status_fields.go:194)
Correct
A null parent now counts as absent, and it is removed before the owned value is set. Two new conflict-retry tests cover it.
OperatorCRD reaches the conditions through GetStatusConditions, so their
JSON name need not be "conditions". The owned-fields merge carried them
across the unstructured conversion by that key. An owner with another
name, such as "checks", got the server's conditions, and the refresh
that follows does not restore owned types, so their staged values were
lost. The merge now carries the conditions through the accessor.
Path validation also recursed forever on a struct that embeds itself
through a pointer. It now tracks the structs on the current embedding
chain and ends the chain on a revisit.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reject unexported fields that JSON does not serialize
pkg/component/status_fields.go:132
This loop counts unexported non-anonymous fields even though JSON does not serialize them. For example, deadline.wall passes validation when deadline is a time.Time, but the encoded deadline is a string, so reading that path fails only on a conflict retry. This violates the documented rejection of invalid paths before writing. Skip unexported fields except anonymous structs whose exported fields can be promoted, and add a validation regression test.
Handle null top-level status as an absent map
pkg/component/status_fields.go:193
NestedMap treats an explicit status: null as a type error, not an absent map. An owner with a nullable top-level status can therefore fail conflict refresh while initializing its status, rather than retrying with its staged fields. The merged == nil fallback below cannot handle this because the error has already returned. Normalize null root statuses before both map reads, and add a conflict-retry test using a nullable top-level status.
Path validation counted unexported fields and descended into types that
encode themselves, such as metav1.Time, so a path that the status JSON
does not contain passed validation and failed only on a conflict retry.
It now skips unexported fields and rejects a path below a json.Marshaler.
A nullable status without omitempty encodes as null, which NestedMap
rejects, so the retry failed while the status was first written. A null
status now reads as an empty map.
The merge now converts through encoding/json instead of
runtime.DefaultUnstructuredConverter, which panics on a status struct
with an unexported field.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review round 3 (Copilot, effort Balanced, review of 2aa3666). The review posted no comments. It listed two "previously missed" findings in its body, which have no thread. Their dispositions are below. The fixes are in 175fac6.
Finding
Verdict
Action
Reject unexported fields that JSON does not serialize (status_fields.go:132)
Correct
Validation skips unexported fields, except for an unexported embedded struct, whose exported fields are promoted. It also rejects a path below a type that implements json.Marshaler, such as metav1.Time (lastSync.Time and lastSync.wall are now errors). New test: "rejects a path that the status JSON does not contain".
Handle null top-level status as an absent map (status_fields.go:193)
Correct
A new statusOf reads an absent or null status as an empty map. New conflict-retry test with an owner whose *Status has no omitempty.
The regression test for the first finding exposed a second problem: runtime.DefaultUnstructuredConverter.FromUnstructured panics on a status struct with an unexported field. The merge now converts through encoding/json, which is also what the API server receives. Numbers decode as json.Number, so int64 values stay exact.
Comment check (review base 491dd65): after the edits below, the gate has 0 flags.
Declaration
Lines (base, now)
Content
Kind and rule
Judge
WithOwnedStatusFields GoDoc
22, 22
"exactly one field in the JSON of the owner's Go type"
CHANGED, caller fact: which paths are rejected
self
toUnstructuredContent doc
2, 2
corrected the "converter" sentence, which no longer described the code
A first draft grew the WithOwnedStatusFields and toUnstructuredContent doc comments (gate flags). I moved the reason to the line it constrains instead of sending the drafts to the evaluator.
Reject TextMarshaler parents in status field validation
pkg/component/status_fields.go:95
This guard misses encoding.TextMarshaler, which encoding/json also uses to encode values as strings. A summary struct with an exported Ready field and text marshaling/unmarshaling methods passes validation for summary.ready, although its JSON has no such nested field. The initial flush can succeed, but a conflict retry then fails in lookupStatusField. Reject parents implementing encoding.TextMarshaler as well, and add a validation test asserting that this declaration fails before any write.
…n retry
The merge decoded the owner back with encoding/json, which turns a
number in a map[string]any or interface status field into float64, so an
int64 above 2^53 was written back rounded. Both directions now use
sigs.k8s.io/json UnmarshalCaseSensitivePreserveInts, the decoder of the
API machinery, which keeps integers as int64.
Path validation now also rejects a path below a type that implements
encoding.TextMarshaler, which encoding/json encodes as a string.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review round 4 (Copilot, effort Balanced, review of 175fac6). Fixes are in 20d4fad.
Finding
Verdict
Action
Preserve JSON numbers during conflict-retry decoding (thread, status_fields.go:328)
Correct
Both directions use sigs.k8s.io/jsonUnmarshalCaseSensitivePreserveInts. New large-integer conflict-retry test.
Reject TextMarshaler parents in validation ("previously missed", body only, status_fields.go:95)
Correct
Validation rejects a path below a type that implements encoding.TextMarshaler as well as json.Marshaler. New case in "rejects a path that the status JSON does not contain".
Comment check (review base 491dd65): the gate has 0 flags. The json.Marshal comment changed to name the API machinery decoder (CHANGED, 3 lines to 3 lines, an external constraint). The textLevel fixture comment was added (test intent). go.mod has no comments and is not checked.
Unexported anonymous scalar fields incorrectly pass status validation
pkg/component/status_fields.go:158
This guard includes unexported anonymous scalar fields, but encoding/json omits them even when tagged. For example, embedding a private type phase string with json:"phase" lets WithOwnedStatusFields("phase") pass validation although JSON contains no such field. The flush writes instead of returning the documented invalid-path error. Skip unexported anonymous fields unless their underlying type is a struct, and add a regression case for an embedded private scalar.
The owned-fields merge decoded the whole owner back from JSON after
zeroing it. That erased private state that JSON cannot restore, such as
a cached pointer behind GetStatusConditions, which then panicked. The
merge now replaces only the owner's top-level status field, and leaves
everything else on the object as it was.
Path validation also counted an unexported embedded scalar, which
encoding/json omits. Only an unexported embedded struct contributes
fields now.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Pointer-valued status replacement loses conditions on conflict retry
pkg/component/status_fields.go:354
With WithOwnedStatusFields, a conflict retry still loses conditions when a pointer-valued status has a cached accessor such as &owner.Status.Checks. This assignment replaces the status pointer, so restoring staged conditions through GetStatusConditions() writes into the old status while serialization sends the new status. The clearing branch at line 341 also detaches the server copy's accessor, which can make unowned conditions appear absent. The new adapter test covers only value-valued status. Preserve accessor-backed storage across clearing, fetching, and replacement, and add a conflict-retry regression combining pointer-valued status with a cached accessor.
setStatus replaced a pointer-valued status field with a new pointer. An
accessor that caches a pointer into the status struct, such as
&owner.Status.Checks, then pointed at the old struct, so the restored
conditions were not the ones encoded and were lost. setStatus now writes
through a non-nil status pointer, and clearing zeroes the struct it
points to.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WithOwnedStatusFields kept Status().Update and, on a conflict, merged
the server's non-condition fields into the typed owner. Each review
round found another gap in that merge or in the reflection-based path
validation it needed.
With the option, every attempt is now a JSON merge patch of the status.
It holds the staged resourceVersion as the precondition, each declared
path with its value from the staged owner's JSON (null when absent, so
the server removes it), and the conditions list. A field the controller
did not declare is never sent. On a 409 the existing
refreshUnownedConditions runs, the patch is built again and retried.
The JSON path of the conditions comes from a marker condition staged
for a moment and found in the owner's JSON, so no reflection is needed.
Paths are checked for syntax only. A misspelled path is no longer
caught; a structural CRD schema prunes the unknown field.
The typed merge, the reflection validation and their tests and
fixtures are removed. sigs.k8s.io/json is indirect again. The specs now
run against envtest, because null removal, the resourceVersion
precondition and schema pruning are server behavior.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Design change: WithOwnedStatusFields now writes a JSON merge patch.
The earlier version kept Status().Update and, on a 409, merged the server's non-condition status fields into the typed owner. Review rounds 2 to 6 each found a gap in that merge or in the path validation it needed:
recursive embedded types;
conditions under another JSON name;
unexported and self-encoding fields;
int64 precision;
private adapter state;
pointer status fields.
All of these came from two things: reflection over the owner's Go type, and decoding server data back into the typed owner.
With the option, every attempt, the first included, is now a merge patch of the status. The patch holds only:
the staged resourceVersion, as the precondition;
the declared fields, taken from the staged owner's JSON, with null for a declared field that the staged owner does not carry;
the conditions list: owned conditions as staged, and unowned conditions as the server holds them.
On a 409, the existing refreshUnownedConditions runs, the patch is built again, and the write retries. An undeclared field is never sent, and nothing is decoded into the typed owner except the response that controller-runtime decodes after a successful write. That removes the class of bugs above, because the code no longer has to reason about Go types at all.
The cost: the framework no longer catches a misspelled path before the write. With a structural CRD schema, the API server prunes the unknown field, so that part of the write does nothing. Path validation now checks syntax only (empty segments, overlap with the conditions), plus a path below a non-object value in the staged owner.
Server-side apply was considered and rejected. A CRD's conditions list is atomic unless the schema marks it +listType=map, so on most CRDs an apply would replace the conditions of every other writer.
The review history stays in the comments above. The PR body now describes the final diff.
Comment check on the rework (review base 491dd65): the gate flagged the FlushStatus GoDoc. A fresh evaluator judged it, and I applied its verdicts. The precondition sentence now says "when the owner has one". The base sentence "Non-condition status fields staged during the reconcile survive a conflict" is restored. The in-memory claim is scoped to the controller-runtime client, which an envtest run measured (self-evaluated after the scoping). The doc on parseOwnedStatusPaths was cut, and its reason moved inline to the overlap check. Not applied: the evaluator's proposal to cut the base "comps is a required parameter" paragraph, which is text from before this PR.
A merge patch merges objects. Declaring an object-valued field such as
"summary" sent only the children the staged owner encodes, so a child
that omitempty dropped stayed on the server. When a declared value is an
object, FlushStatus now reads the server's copy, as it does on a
conflict, and sends null for each child the server holds and the staged
owner does not. A retry recomputes the nulls against the copy it reads.
refreshUnownedConditions now returns the server's copy it read.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Envtest shows that the controller-runtime client decodes the response into a cleared object. Added the in-memory regression that the finding asked for. It passes without a code change.
Object field deletions are not persisted in merge patches (status_patch.go:207)
Correct
A declared object is owned whole. FlushStatus reads the server's copy and sends null for the dropped children. Two new envtest specs.
Comment check (new review base dd682ce): one flag, the WithOwnedStatusFields GoDoc, which gained one sentence. A fresh evaluator gave it KEEP: "A declared object is owned whole: a child that the staged owner does not carry is removed too." The caller decision is to stage every child of a declared object, or to declare narrower paths. I trimmed the internal ownedStatusPatch doc so it does not describe the body. New internal docs on statusContent, valueAt and withRemovals state behavior that a reader would not expect.
…rst patch
Declaring an object-valued field read the server's copy through
refreshUnownedConditions before the first patch. That also reverted
hand-staged unowned conditions, such as an owner-level aggregate, on
every flush, with no conflict at all. The read is now a separate
readServerOwner that leaves the staged owner untouched. The refresh
stays reserved for a 409.
The spec for removals after a conflict now makes the competing write
after that read, so the first patch really conflicts.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review round 9 (Copilot, effort Balanced, review of 699d968). Fixes are in b9bf47f.
Finding
Verdict
Action
The read before the first patch removes staged conditions without a conflict (status_patch.go:114)
Correct
A new read-only readServerOwner; the refresh only runs after a 409. New conflict-free spec with a staged aggregate.
Test child removal after a real conflict retry (flush_owned_fields_test.go:160)
Correct
The competing write now runs between the read and the first patch, through a beforeFirstPatch hook. The spec asserts two attempts.
Comment check (review base dd682ce): one flag, the WithOwnedStatusFields GoDoc. Its only added sentence got KEEP in round 8, and this round only split the paragraph. Added: the inline reason at the read ("refreshing would revert staged unowned conditions although nothing conflicted"), and the readServerOwner doc. The existing clearing and reader comments moved with their code into readServerOwner. I also restored two specs that an edit in this round had removed by mistake: "removes a declared field that the staged owner cleared" and "writes a nested declared field".
Preserve explicit zero-valued keys during owned field removal
pkg/component/status_patch.go:224
Whole-object removal still misses explicitly stored zero-valued children. For example, server JSON summary: {ready: 1, total: 0} decodes into MockStatusSummary, but statusContent(server) drops total because of omitempty. Owning summary and staging {ready: 2} therefore sends no total: null, leaving the field present despite the documented removal contract. Preserve the raw server keys when calculating removals, including after conflicts. Add a regression that seeds an explicit zero and checks field absence through an unstructured read; a typed assertion cannot distinguish zero from absence.
Skip descendant declarations covered by an owned ancestor
pkg/component/status_patch.go:231
Overlapping declarations make the patch depend on path order. With WithOwnedStatusFields("summary", "summary.details.ready"), when staged and server summary are objects that both omit details, the descendant adds details: {ready: null} to the parent patch. Merge patch creates details: {} even though the wholly owned staged object omits it. Reversing the declarations leaves it absent. After validating all paths, skip descendants covered by a declared ancestor, and test both orders, including paths combined from separate options.
Avoid creating parent objects for absent or null owned paths
pkg/component/status_patch.go:324
For WithOwnedStatusFields("summary.ready"), if the staged owner and server both omit summary, this builds {"summary":{"ready":null}}. JSON merge patch creates summary: {} instead of leaving it absent; a server-side null parent also becomes an object. If that optional object has required children, validation rejects the entire flush, including its conditions. Build deletion-only paths against the server's parent state so absent/null parents remain unchanged without deleting unowned siblings. Refresh that state after conflicts and add regressions for absent and null parents.
Three gaps in building the patch:
- A typed read of the server's copy drops a stored zero value that
omitempty skips, so a declared object kept that child.
- A null for a nested field below a parent the server does not hold as
an object created that parent as {}.
- Overlapping declarations made the patch depend on path order.
Each attempt now reads the stored status into an unstructured object,
through APIReader when set, and builds the patch against it. A null is
sent only for a key the server holds. A declared object gets null for
every stored child the staged owner lacks. A path below a declared
ancestor is dropped. The typed preflight read and the server copy
returned from refreshUnownedConditions are gone again. The reader
selection moves into liveReader.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review round 10 (Copilot, effort Balanced, review of b9bf47f). The review posted no comments. It listed three "previously missed" findings in its body, which have no threads. The fixes are in 6f15e26.
Finding
Verdict
Action
Preserve explicit zero-valued keys during owned field removal (status_patch.go:224)
Correct
Each attempt reads the stored status into an unstructured object, so a stored total: 0 is seen and removed. New envtest spec that seeds the zero with a raw patch and checks absence through an unstructured read.
Skip descendant declarations covered by an owned ancestor (status_patch.go:231)
Correct
parseOwnedStatusPaths drops a path below a declared ancestor, and duplicates. A unit test shows both orders give the same patch.
Avoid creating parent objects for absent or null owned paths (status_patch.go:324)
Correct
A null is sent only for a key that the server holds below object parents. An envtest spec covers an absent parent, and unit tests cover absent and null parents.
Each new test was shown to fail without its fix.
With this, the patch is built against the stored status on every attempt (one live read per attempt). The typed read before the first attempt from round 9 is gone, and refreshUnownedConditions is back to main's signature. The reader selection moved into a small liveReader helper, which both reads share.
Comment check (review base dd682ce): one flag, the WithOwnedStatusFields GoDoc, which gained a sentence about the read. A fresh evaluator kept the first part and cut the purpose clause, because a cache-backed Client can return a stale status. Applied as: "Each attempt first reads the owner's status through rec.APIReader when set, and through rec.Client otherwise; a cache-backed rec.Client can return a stale status." docs/component.md now says the same and tells readers to set APIReader. The reader comment moved from the refresh into the liveReader doc.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Concurrent status writes and API-server patch semantics require final human validation.
0 open findings
Previously missed (1)
In code that hasn't changed since last review
Exclude unowned staged conditions on the first patch attempt
docs/component.md:949
The first patch sends all conditions as staged; only a 409 refreshes unowned types from the server. The regression in pkg/component/flush_owned_fields_test.go:175–189 explicitly preserves staged unowned conditions without a conflict. Distinguish the first attempt from retries so callers do not assume unowned staged changes are excluded, then run make sync-plugin to update the same statement in plugin/skills/building-components/references/component.md.
The shared-status section said the patch carries the unowned conditions
as the server holds them. That holds after a 409. The first attempt
sends the conditions as staged, including a hand-staged unowned
condition, guarded by the resourceVersion precondition.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review round 11 (Copilot, effort Balanced, review of 6f15e26). The review posted no comments. It listed one "previously missed" finding in its body, which has no thread.
Finding
Verdict
Action
Exclude unowned staged conditions on the first patch attempt (docs/component.md:949)
Correct, docs only
The section now says that the first attempt sends the conditions as staged, including a hand-staged unowned condition, under the resourceVersion precondition, and that unowned conditions are refreshed from the server only after a 409. The plugin reference is synced, and the PR body is updated.
Status().Update sends the owner's uid and the API server rejects a
mismatch. A merge patch carries no uid precondition: the server ignores
a uid in the body. So after a conflict, a retry took the resourceVersion
of an owner that was deleted and recreated under the same name and
patched it with the old owner's status. The read before each attempt
now compares the stored uid with the staged owner's and returns an
error without writing when they differ.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review round 12 (Copilot, effort Balanced, review of fdcf229). The fix is in f971adf.
Finding
Verdict
Action
Reject deleted-and-recreated CRs with a mismatched UID (status_patch.go:157)
Correct
The read before each attempt compares uids and fails without writing. A uid in the patch body does not work: the API server ignores it, as envtest showed. New delete-and-recreate envtest spec.
Comment check (review base dd682ce): the only flag is the WithOwnedStatusFields GoDoc, unchanged since round 10's verdict. One inline comment was added at the uid check, stating a constraint from outside the file (a merge patch has no uid precondition).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #238
When two controllers share the status of one CR, a 409 retry of
FlushStatuswrites the stale copy of the other controller's non-condition fields back, for examplestatus.observedGeneration. This PR addscomponent.WithOwnedStatusFields(paths...). With it, every write ofFlushStatusis a JSON merge patch of the status that holds only the declared fields and the conditions, guarded by the stagedresourceVersion, so another writer's fields are never sent. Without the option,FlushStatusbehaves exactly as before. The framework does not check a declared path against the CRD schema: with a structural schema, the API server prunes a misspelled field, so that part of the write does nothing.Changes
FlushStatus(ctx, rec, comps, opts ...FlushOption): new variadic options. Existing calls compile and keep theStatus().Updatepath unchanged.WithOwnedStatusFields(paths ...string): names the non-condition status fields that the flush owns, by JSON name understatuswith dots for nested fields ("observedGeneration","summary.ready"). With no paths, the flush owns no non-condition field. Paths from several options are combined.APIReaderwhen set), then sendsclient.RawPatch(types.MergePatchType, body)to the status subresource. The body holds:metadata.resourceVersionof the staged owner, as the optimistic lock;nullwhen the staged owner does not carry it and the server does. A declared object is owned whole: it carriesnullfor every stored child that the staged owner lacks, including a stored zero that a typed read would drop;resourceVersionprecondition ensures that the list read at the top of the reconcile is still current. After a 409, the unowned conditions are refreshed from the server first.refreshUnownedConditionsupdates the stagedresourceVersionand unowned conditions. The stored status is read again, the patch is rebuilt, and the write retries underretry.RetryOnConflict.GetStatusConditionsdecides where they live.FlushStatusstages a marker condition for a moment, encodes the owner, and finds the marker in the JSON. This needs no reflection and works whatever the field is called.rec.Owner, replacing the object. A staged value of an undeclared field does not survive in memory. An envtest run confirmed that no stale field remains.FlushStatusGoDoc states which status fields a conflict retry takes from the server: theresourceVersionand the unowned conditions, nothing else.docs/component.mdgets a new section, "Sharing the owner's status with another controller", plus corrected conflict and write descriptions.docs/guidelines.mdand thebuilding-componentsandstructuring-operatorsplugin skills get the shared-status guidance. The plugin references are synced.Challenges
A merge patch replaces a list as a whole, so the conditions cannot be patched by type. The patch carries the full conditions list. The
resourceVersionprecondition is what makes that safe: the list matches the server's at the moment the patch applies, except for the owned types, and a concurrent change makes the patch fail with a 409 and refresh. Server-side apply would avoid the full list, but a CRD's conditions list is atomic unless the schema marks it+listType=map. On most CRDs an apply would replace the other writers' conditions.Testing
New envtest specs in
pkg/component/flush_owned_fields_test.go. They run against a real API server, because merge-patch null removal, theresourceVersionprecondition and schema pruning are server behavior. They cover:observedGenerationbetween the read and the write. The first patch conflicts, the retry succeeds, and the server keeps the second writer's value;metadata.resourceVersion, the declared field and the conditions, even with no conflict, and an undeclared staged field is not written;Unit tests cover the conditions path lookup (including conditions nested under another JSON name), the overlap rejection, order independence of overlapping declarations, and absent or null parents. The existing
FlushStatustests cover the default path, unchanged. I confirmed that the envtest specs fail when the option is ignored, and that the removal spec fails whennullis not sent.make allpasses.🤖 Generated with Claude Code