From 491dd65160ca2403019560de60e6328785a0c859 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 16 Oct 2026 13:14:07 +0100 Subject: [PATCH 01/13] fix(component): keep other writers' status fields on a FlushStatus conflict 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 --- docs/component.md | 67 ++++- docs/guidelines.md | 5 + pkg/component/component.go | 105 +++++--- pkg/component/conditions_test.go | 235 ++++++++++++++++++ pkg/component/status_fields.go | 230 +++++++++++++++++ pkg/component/zz_mock_crd_test.go | 21 ++ plugin/skills/building-components/SKILL.md | 25 +- .../references/component.md | 67 ++++- plugin/skills/structuring-operators/SKILL.md | 3 + .../references/guidelines.md | 6 + 10 files changed, 713 insertions(+), 51 deletions(-) create mode 100644 pkg/component/status_fields.go diff --git a/docs/component.md b/docs/component.md index e6af17b4..5b1f84a1 100644 --- a/docs/component.md +++ b/docs/component.md @@ -818,19 +818,27 @@ reconcile, so the fields nobody staged still hold current server state. An owner writes stale values back over newer ones. On a 409 Conflict, for example when another writer updated the owner between the controller's `Get` and this call, -`FlushStatus` keeps the staged owner as the object it writes. It fetches the server's copy into a **separate** object, -takes that copy's `resourceVersion` so the retry targets the live object, restores from it only the conditions whose -type this flush does not own, and retries. The staged owner is never replaced. +`FlushStatus` keeps the staged owner as the object it writes. It fetches the server's copy into a **separate** object +and retries. The staged owner is never replaced. From the server's copy, the retry takes: + +- the `resourceVersion`, so the retry targets the live object; +- every condition whose type this flush does not own; +- with [`WithOwnedStatusFields`](#sharing-the-owners-status-with-another-controller), every non-condition status field + outside the declared paths. + +Everything else keeps the staged value. That fetch goes through `ReconcileContext.APIReader` when it is set, and through `Client` otherwise. Set it from the manager's `GetAPIReader()`. The manager's default `Client` serves reads from the informer cache, which can lag the very write that caused the conflict and hand back the same stale `resourceVersion`, so a retry through the cache may conflict again until the cache catches up. A direct read sees the server as it is. -Two things follow from that. Non-condition status fields staged during the reconcile survive a conflict, because the -object holding them is the object that gets written. And a condition type this flush does not own keeps the server's -value rather than the copy the controller happens to be holding, so a concurrent update by another writer is not rolled -back. +Two things follow from that. A condition type this flush does not own keeps the server's value rather than the copy the +controller happens to be holding, so a concurrent update by another writer is not rolled back. And without +`WithOwnedStatusFields`, every non-condition status field keeps the staged value: a field the controller staged during +the reconcile survives a conflict, and so does the stale copy of a field that another controller owns. When another +controller writes non-condition fields of the same status, declare the fields this flush owns, as described in +[Sharing the owner's status with another controller](#sharing-the-owners-status-with-another-controller). Which types count as owned comes from the components passed to `FlushStatus`: @@ -917,6 +925,47 @@ Server-Side Apply is for managed resources, never for the owner's status subreso depends on which call runs last: the `Update` can overwrite the patched condition, or the patch can overwrite what the `Update` wrote. Keep every owner-status write on the `meta.SetStatusCondition` plus `FlushStatus` path. +### Sharing the owner's status with another controller + +Sometimes two controllers write the status of one CR. For example, a status-only controller owns an `Accepted` +condition, and a second controller owns a `Ready` condition and `status.observedGeneration`. Each controller still +writes through `FlushStatus`. Condition ownership comes from the components passed to it. For non-condition fields, pass +`component.WithOwnedStatusFields` with the fields that this controller writes: + +```go +// In the status-only controller, which writes status.phase and no other non-condition field: +flushErr := component.FlushStatus(ctx, recCtx, comps, component.WithOwnedStatusFields("phase")) + +// In the other controller, which writes status.observedGeneration: +flushErr := component.FlushStatus(ctx, recCtx, comps, component.WithOwnedStatusFields("observedGeneration")) +``` + +The declaration changes only the conflict retry. The first write sends the whole staged status, as without the option. +When nothing changed on the server since the controller read the owner, that write succeeds, and the fields the +controller did not stage still hold the values it read. On a retry: + +- A declared field keeps the staged value. If the staged owner does not carry the field, the retry removes it. +- Every other non-condition status field takes the server's value. If the server does not carry the field, the retry + removes it. This is the same rule as for an unowned condition. +- Conditions follow component ownership, as described above. + +Name each field by its JSON name under `status`, with dots between nested fields: `"observedGeneration"`, +`"summary.ready"`. Do not include the `status.` prefix: `"status.phase"` is an error unless status has a field named +`status`. `WithOwnedStatusFields()` with no paths declares that the flush owns no non-condition field, which fits a +controller that writes only conditions. Paths from several options are combined. + +`FlushStatus` validates each path before it writes and returns an error, with no write, when a path: + +- is empty or has an empty segment, such as `"summary..ready"`; +- names `conditions` or a field below it, because conditions are owned through components; +- does not resolve to a field of the owner's Go type, such as a misspelled name or a path below a scalar field. Below a + map field, any key is accepted. + +Without the option, `FlushStatus` keeps the staged value of every non-condition status field on a retry. That default +keeps a controller's own `observedGeneration` write safe when it is the only writer of the status. Once you declare +fields, a staged field that you did not declare follows the server on a retry. So declare every field that the +controller writes, including `observedGeneration` when it is this controller's field. + ### Where the owner's observedGeneration is set The framework sets `ObservedGeneration` on every **component condition** it writes, from `owner.GetGeneration()`. It @@ -925,7 +974,9 @@ never writes `status.observedGeneration` on the owner itself. `OperatorCRD` requ An owner-level `status.observedGeneration` is the controller's own field. Assign it on the owner before `FlushStatus` runs, in the same staging step that writes the owner's own condition, and the single status write persists both. This -applies to a status-only controller too, as in the validation-only example above. +applies to a status-only controller too, as in the validation-only example above. When the status is +[shared with another controller](#sharing-the-owners-status-with-another-controller), only the controller that owns +`observedGeneration` assigns it, and that controller includes it in `WithOwnedStatusFields`. ```go app.Status.ObservedGeneration = app.Generation diff --git a/docs/guidelines.md b/docs/guidelines.md index 13b07842..b42aa23d 100644 --- a/docs/guidelines.md +++ b/docs/guidelines.md @@ -190,6 +190,11 @@ returns an error. If you do not want condition metrics, leave `ReconcileContext.Metrics` as `nil`; `FlushStatus` tolerates a nil recorder and skips metric emission. +When two controllers write the status of one CR, give each status field one owner, as with conditions. Each controller +passes the components that own its conditions and declares its non-condition fields with +[`WithOwnedStatusFields`](component.md#sharing-the-owners-status-with-another-controller). Without the declaration, a +conflict retry writes the controller's stale copy of the other controller's fields back over newer values. + Building the component set from a pure resolver `(spec, version) -> []*component.Component` keeps the loop stable: enabling an optional feature changes which components the resolver returns without touching the reconcile loop. diff --git a/pkg/component/component.go b/pkg/component/component.go index 1f9af1eb..5e11cee7 100644 --- a/pkg/component/component.go +++ b/pkg/component/component.go @@ -691,21 +691,25 @@ func fail(rec ReconcileContext, conditionType ConditionType, err error) error { // On a 409 Conflict (for example if an external writer updated the owner between // the controller fetching it and this call) FlushStatus keeps the staged owner as // the object being written. It fetches the server's copy into a separate object -// through rec.APIReader when set (rec.Client otherwise), takes that object's -// resourceVersion so the retry targets the live object, and restores from it -// only the conditions whose type this flush does not own. It then retries. +// through rec.APIReader when set (rec.Client otherwise) and retries. From the +// server's copy a retry takes: +// +// - the resourceVersion, so the retry targets the live object; +// - every condition whose type this flush does not own; +// - when [WithOwnedStatusFields] is passed, every non-condition status field +// outside the declared paths. +// +// Everything else keeps the staged value. Without [WithOwnedStatusFields] that +// includes every non-condition status field, so a field the controller staged +// survives a conflict, and so does a stale field another controller owns. On an +// owner whose status is shared with another writer, declare the fields this +// flush owns, or the retry rolls back that writer's newer values. // // Set rec.APIReader from the manager's GetAPIReader. The manager's default // Client serves reads from the informer cache, which can lag the very write // that caused the conflict and return the same stale resourceVersion, so a // retry through it may conflict again until the cache catches up. // -// Two consequences follow from never replacing the staged owner. Non-condition -// status fields staged during the reconcile survive a conflict. And a condition -// type this flush does not own keeps the server's value rather than the possibly -// stale copy the controller happens to be holding, so a concurrent update by -// another writer is not rolled back. -// // For unowned types the server is the source of truth, absence included. An // unowned condition the server no longer carries is dropped from the staged // owner rather than written back, so a condition another writer removed is not @@ -714,12 +718,29 @@ func fail(rec ReconcileContext, conditionType ConditionType, err error) error { // the next reconcile. // // rec.Client and rec.Owner must be populated, and every component in comps must -// be non-nil; a nil component is a programming error. If rec.Metrics is nil, -// metric recording is skipped. -func FlushStatus(ctx context.Context, rec ReconcileContext, comps []*Component) error { - owned := ownedConditionTypes(comps) +// be non-nil; a nil component is a programming error. FlushStatus returns an +// error without writing when a path given to [WithOwnedStatusFields] is +// invalid. If rec.Metrics is nil, metric recording is skipped. +func FlushStatus(ctx context.Context, rec ReconcileContext, comps []*Component, opts ...FlushOption) error { + var options flushOptions + for _, opt := range opts { + opt(&options) + } + + ownedFields, err := parseOwnedStatusFields(rec.Owner, options.ownedFields) + if err != nil { + return fmt.Errorf( + "flushing status of %s %s: %w", rec.Owner.GetKind(), client.ObjectKeyFromObject(rec.Owner), err, + ) + } + + refresh := conflictRefresh{ + ownedConditions: ownedConditionTypes(comps), + fieldsDeclared: options.fieldsDeclared, + ownedFieldPaths: ownedFields, + } - err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { updateErr := rec.Client.Status().Update(ctx, rec.Owner) if updateErr == nil { return nil @@ -727,7 +748,7 @@ func FlushStatus(ctx context.Context, rec ReconcileContext, comps []*Component) if !apierrors.IsConflict(updateErr) { return updateErr } - if refreshErr := refreshUnownedConditions(ctx, rec, owned); refreshErr != nil { + if refreshErr := refresh.apply(ctx, rec); refreshErr != nil { return refreshErr } return updateErr @@ -759,16 +780,22 @@ func ownedConditionTypes(comps []*Component) map[string]struct{} { return owned } -// refreshUnownedConditions prepares the staged owner for a retry after a -// conflict, without ever replacing it. -// -// The staged owner remains the object that will be written, so every field the -// controller staged during this reconcile survives, conditions and non-condition -// status fields alike. Only two things are taken from the server: the current -// resourceVersion, so the retry targets the live object, and the value of every -// condition type this flush does not own, so a concurrent update by another -// writer is not overwritten by the copy this controller happens to be holding. -func refreshUnownedConditions(ctx context.Context, rec ReconcileContext, owned map[string]struct{}) error { +// conflictRefresh prepares the staged owner for a retry after a conflict, +// without ever replacing it. +// +// The staged owner remains the object that will be written. From the server it +// takes the current resourceVersion, so the retry targets the live object, and +// the value of every condition type this flush does not own, so a concurrent +// update by another writer is not overwritten by the copy this controller +// happens to be holding. When the controller declared its status fields, it +// also takes every non-condition status field outside those paths. +type conflictRefresh struct { + ownedConditions map[string]struct{} + fieldsDeclared bool + ownedFieldPaths [][]string +} + +func (r conflictRefresh) apply(ctx context.Context, rec ReconcileContext) error { server, ok := rec.Owner.DeepCopyObject().(OperatorCRD) if !ok { return fmt.Errorf("failed to deep copy owner of type %T", rec.Owner) @@ -776,8 +803,15 @@ func refreshUnownedConditions(ctx context.Context, rec ReconcileContext, owned m // Clear the copy's conditions before the fetch. Decoding a response into a // non-empty struct does not remove fields the response omits, so an owner // with no conditions on the server would otherwise leave the staged - // conditions in place here and they would read as the server's. + // conditions in place here and they would read as the server's. With + // declared fields every status field is read from the server, so the whole + // status is cleared for the same reason. *server.GetStatusConditions() = nil + if r.fieldsDeclared { + if err := zeroStatus(server); err != nil { + return err + } + } // Read through APIReader when one is set: the manager's Client serves reads // from the informer cache, which can lag the write that caused the conflict @@ -790,8 +824,20 @@ func refreshUnownedConditions(ctx context.Context, rec ReconcileContext, owned m return err } + if r.fieldsDeclared { + if err := applyOwnedStatusFields(rec.Owner, server, r.ownedFieldPaths); err != nil { + return err + } + } + + refreshUnownedConditions(rec.Owner, server, r.ownedConditions) rec.Owner.SetResourceVersion(server.GetResourceVersion()) + return nil +} +// refreshUnownedConditions sets on staged the server's value of every condition +// type this flush does not own. +func refreshUnownedConditions(staged, server OperatorCRD, owned map[string]struct{}) { serverConds := *server.GetStatusConditions() onServer := make(map[string]struct{}, len(serverConds)) for _, cond := range serverConds { @@ -801,8 +847,8 @@ func refreshUnownedConditions(ctx context.Context, rec ReconcileContext, owned m // Absence is server state too. An unowned condition the server no longer // carries has been removed by whoever owns it, so drop the local copy // instead of writing it back and resurrecting it. - staged := rec.Owner.GetStatusConditions() - *staged = slices.DeleteFunc(*staged, func(cond metav1.Condition) bool { + conditions := staged.GetStatusConditions() + *conditions = slices.DeleteFunc(*conditions, func(cond metav1.Condition) bool { if _, isOwned := owned[cond.Type]; isOwned { return false } @@ -814,9 +860,8 @@ func refreshUnownedConditions(ctx context.Context, rec ReconcileContext, owned m if _, isOwned := owned[cond.Type]; isOwned { continue } - replaceStatusCondition(staged, cond) + replaceStatusCondition(conditions, cond) } - return nil } // replaceStatusCondition sets cond by type, taking its value verbatim. diff --git a/pkg/component/conditions_test.go b/pkg/component/conditions_test.go index 9801491e..76ceacc6 100644 --- a/pkg/component/conditions_test.go +++ b/pkg/component/conditions_test.go @@ -2,6 +2,7 @@ package component import ( "context" + "encoding/json" "errors" "fmt" "testing" @@ -796,3 +797,237 @@ func TestFlushStatusConflictOwnership(t *testing.T) { assert.Zero(t, counting.gets, "the non-conflict path must not fetch the owner") }) } + +// TestFlushStatusOwnedStatusFields covers a status shared between controllers. +// Each case reads the owner, lets a second writer update the status, and then +// flushes, so the first update is a real 409 from a stale resourceVersion. +func TestFlushStatusOwnedStatusFields(t *testing.T) { + ctx := context.Background() + scheme := runtime.NewScheme() + require.NoError(t, AddToScheme(scheme)) + + infraComponent := func(t *testing.T) *Component { + t.Helper() + comp, err := NewComponentBuilder().WithName("infra").WithConditionType("InfraReady").Build() + require.NoError(t, err) + return comp + } + + // readThenRace creates the owner on a fake server, reads it as the + // controller would at the top of a reconcile, and then applies the second + // writer's status update on the server. The returned owner is stale. + readThenRace := func( + t *testing.T, initial *MockOperatorCRD, secondWriter func(*MockOperatorCRD), + ) (client.Client, *MockOperatorCRD) { + t.Helper() + initial.ObjectMeta = metav1.ObjectMeta{Name: "test-owner", Namespace: "default", Generation: 2} + k8sClient := fake.NewClientBuilder(). + WithScheme(scheme).WithStatusSubresource(initial).WithObjects(initial).Build() + + owner := &MockOperatorCRD{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), owner)) + + other := &MockOperatorCRD{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), other)) + secondWriter(other) + require.NoError(t, k8sClient.Status().Update(ctx, other)) + return k8sClient, owner + } + + persistedOf := func(t *testing.T, c client.Client, owner *MockOperatorCRD) *MockOperatorCRD { + t.Helper() + persisted := &MockOperatorCRD{} + require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(owner), persisted)) + return persisted + } + + t.Run("keeps a second writer's non-condition field across the conflict retry", func(t *testing.T) { + k8sClient, owner := readThenRace(t, &MockOperatorCRD{Status: MockStatus{ObservedGeneration: 1}}, + func(o *MockOperatorCRD) { o.Status.ObservedGeneration = 2 }) + + // This controller owns phase and InfraReady; observedGeneration belongs + // to the second writer and is still 1 on the stale owner. + owner.Status.Phase = "Accepted" + applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ + Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", + }) + + require.NoError(t, FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, []*Component{infraComponent(t)}, + WithOwnedStatusFields("phase"), + )) + + persisted := persistedOf(t, k8sClient, owner) + assert.Equal(t, int64(2), persisted.Status.ObservedGeneration, + "the retry must not roll back a field this flush does not own") + assert.Equal(t, "Accepted", persisted.Status.Phase, "an owned field keeps the staged value") + assert.NotNil(t, meta.FindStatusCondition(persisted.Status.Conditions, "InfraReady"), + "an owned condition keeps the staged value") + assert.Equal(t, int64(2), owner.Status.ObservedGeneration, + "the in-memory owner reflects what was written") + }) + + t.Run("owns no non-condition field when declared with no paths", func(t *testing.T) { + k8sClient, owner := readThenRace(t, &MockOperatorCRD{Status: MockStatus{Phase: "Pending"}}, + func(o *MockOperatorCRD) { + o.Status.ObservedGeneration = 2 + o.Status.Phase = "Running" + }) + + owner.Status.ObservedGeneration = 99 // a stray write this flush does not own + applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ + Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", + }) + + require.NoError(t, FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, []*Component{infraComponent(t)}, + WithOwnedStatusFields(), + )) + + persisted := persistedOf(t, k8sClient, owner) + assert.Equal(t, int64(2), persisted.Status.ObservedGeneration) + assert.Equal(t, "Running", persisted.Status.Phase) + assert.Equal(t, "Healthy", meta.FindStatusCondition(persisted.Status.Conditions, "InfraReady").Reason) + }) + + t.Run("keeps every staged field when no fields are declared", func(t *testing.T) { + // Without the option the controller's own staged fields win, so an + // existing caller that stages observedGeneration does not lose it. + k8sClient, owner := readThenRace(t, &MockOperatorCRD{}, + func(o *MockOperatorCRD) { o.Status.Phase = "Running" }) + + owner.Status.ObservedGeneration = 2 + + require.NoError(t, FlushStatus(ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil)) + + persisted := persistedOf(t, k8sClient, owner) + assert.Equal(t, int64(2), persisted.Status.ObservedGeneration) + assert.Empty(t, persisted.Status.Phase, "with nothing declared the staged status is written as a whole") + }) + + t.Run("removes an owned field the staged owner cleared", func(t *testing.T) { + k8sClient, owner := readThenRace(t, &MockOperatorCRD{Status: MockStatus{Phase: "Pending"}}, + func(o *MockOperatorCRD) { o.Status.ObservedGeneration = 2 }) + + owner.Status.Phase = "" + + require.NoError(t, FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("phase"), + )) + + persisted := persistedOf(t, k8sClient, owner) + assert.Empty(t, persisted.Status.Phase, "the staged owner is the source of truth for an owned field") + assert.Equal(t, int64(2), persisted.Status.ObservedGeneration) + }) + + t.Run("drops an unowned field the server no longer carries", func(t *testing.T) { + k8sClient, owner := readThenRace(t, &MockOperatorCRD{Status: MockStatus{ObservedGeneration: 1}}, + func(o *MockOperatorCRD) { o.Status.ObservedGeneration = 0 }) + + owner.Status.Phase = "Accepted" + + // The REST client decodes a response into the object it is given, so a + // field the response omits keeps whatever that object held. The fake + // client replaces the object instead and would hide a stale value. + require.NoError(t, FlushStatus( + ctx, + ReconcileContext{Client: k8sClient, APIReader: decodingReader{Reader: k8sClient}, Owner: owner}, + nil, + WithOwnedStatusFields("phase"), + )) + + persisted := persistedOf(t, k8sClient, owner) + assert.Zero(t, persisted.Status.ObservedGeneration, + "the server is the source of truth for an unowned field, absence included") + assert.Equal(t, "Accepted", persisted.Status.Phase) + }) + + t.Run("owns a nested field by its dotted path", func(t *testing.T) { + k8sClient, owner := readThenRace(t, + &MockOperatorCRD{Status: MockStatus{Summary: &MockStatusSummary{Ready: 1, Total: 3}}}, + func(o *MockOperatorCRD) { o.Status.Summary.Total = 4 }) + + owner.Status.Summary.Ready = 2 + + require.NoError(t, FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("summary.ready"), + )) + + persisted := persistedOf(t, k8sClient, owner) + require.NotNil(t, persisted.Status.Summary) + assert.Equal(t, MockStatusSummary{Ready: 2, Total: 4}, *persisted.Status.Summary) + }) + + t.Run("combines the paths of several options", func(t *testing.T) { + k8sClient, owner := readThenRace(t, &MockOperatorCRD{}, + func(o *MockOperatorCRD) { o.Status.Summary = &MockStatusSummary{Total: 4} }) + + owner.Status.Phase = "Accepted" + owner.Status.ObservedGeneration = 2 + + require.NoError(t, FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, + WithOwnedStatusFields("phase"), WithOwnedStatusFields("observedGeneration"), + )) + + persisted := persistedOf(t, k8sClient, owner) + assert.Equal(t, "Accepted", persisted.Status.Phase) + assert.Equal(t, int64(2), persisted.Status.ObservedGeneration) + require.NotNil(t, persisted.Status.Summary) + assert.Equal(t, int32(4), persisted.Status.Summary.Total) + }) + + t.Run("rejects an invalid path before writing", func(t *testing.T) { + for name, path := range map[string]string{ + "empty": "", + "empty segment": "summary..ready", + "conditions": "conditions", + "inside conditions": "conditions.type", + "unknown field": "observedGenration", + "prefixed with status": "status.phase", + "below a scalar": "phase.value", + "unknown nested field": "summary.unknown", + "trailing separator": "phase.", + "leading separator": ".phase", + } { + t.Run(name, func(t *testing.T) { + owner := &MockOperatorCRD{ + ObjectMeta: metav1.ObjectMeta{Name: "test-owner", Namespace: "default"}, + } + k8sClient := fake.NewClientBuilder(). + WithScheme(scheme).WithStatusSubresource(owner).WithObjects(owner).Build() + owner.Status.Phase = "Accepted" + + err := FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields(path), + ) + + require.Error(t, err) + assert.Contains(t, err.Error(), fmt.Sprintf("%q", path)) + assert.Empty(t, persistedOf(t, k8sClient, owner).Status.Phase, "nothing is written") + }) + } + }) +} + +// decodingReader reads like the REST client: it decodes the server's JSON into +// the object passed in, so fields the JSON omits are left as they were. +type decodingReader struct { + client.Reader +} + +func (r decodingReader) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + fresh, ok := obj.DeepCopyObject().(client.Object) + if !ok { + return fmt.Errorf("deep copy of %T is not a client.Object", obj) + } + if err := r.Reader.Get(ctx, key, fresh, opts...); err != nil { + return err + } + + data, err := json.Marshal(fresh) + if err != nil { + return err + } + return json.Unmarshal(data, obj) +} diff --git a/pkg/component/status_fields.go b/pkg/component/status_fields.go new file mode 100644 index 00000000..87ab3e3e --- /dev/null +++ b/pkg/component/status_fields.go @@ -0,0 +1,230 @@ +package component + +import ( + "errors" + "fmt" + "reflect" + "strings" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" +) + +// FlushOption configures a [FlushStatus] call. +type FlushOption func(*flushOptions) + +type flushOptions struct { + // fieldsDeclared is set by any WithOwnedStatusFields option, including one + // with no paths: declaring that a flush owns no field differs from not + // declaring at all. + fieldsDeclared bool + ownedFields []string +} + +// WithOwnedStatusFields declares the non-condition status fields that this +// flush owns, for an owner whose status is shared with other writers. +// +// Each path names a field under status by its JSON name, with nested fields +// separated by dots, for example "observedGeneration" or "summary.ready". +// Paths are relative to status: write "phase", not "status.phase". A path must +// not name "conditions" or a field below it: condition ownership comes from the +// components passed to [FlushStatus]. For a typed owner every path must resolve +// to a field of the owner's Go type, so a misspelled path is an error rather +// than a silent loss of ownership. FlushStatus validates the paths before it +// writes and returns an error for an invalid one. +// +// The declaration takes effect only on a 409 conflict retry. An owned field +// keeps the staged value, and a field the staged owner does not carry is +// removed. Every other non-condition status field takes the server's value, and +// a field the server does not carry is removed. A field the controller staged +// but did not declare is therefore lost on a retry: declare every non-condition +// field the controller writes. +// +// Calling it with no paths declares that the flush owns no non-condition +// status field, as for a controller that writes only conditions. Paths from +// several options are combined. +func WithOwnedStatusFields(paths ...string) FlushOption { + return func(o *flushOptions) { + o.fieldsDeclared = true + o.ownedFields = append(o.ownedFields, paths...) + } +} + +// parseOwnedStatusFields splits each path into its segments and rejects a path +// the owner's type cannot carry. +func parseOwnedStatusFields(owner OperatorCRD, paths []string) ([][]string, error) { + parsed := make([][]string, 0, len(paths)) + for _, path := range paths { + segments := strings.Split(path, ".") + if err := validateStatusFieldPath(owner, segments); err != nil { + return nil, fmt.Errorf("owned status field %q: %w", path, err) + } + parsed = append(parsed, segments) + } + return parsed, nil +} + +func validateStatusFieldPath(owner OperatorCRD, segments []string) error { + for _, segment := range segments { + if segment == "" { + return errors.New("path has an empty segment") + } + } + + if segments[0] == "conditions" { + return errors.New("conditions are owned through the components passed to FlushStatus") + } + + // An unstructured owner has no Go type to check the path against. + if _, ok := owner.(runtime.Unstructured); ok { + return nil + } + + current, ok := jsonField(reflect.TypeOf(owner), "status") + if !ok { + return fmt.Errorf("owner type %T has no status field", owner) + } + + for _, segment := range segments { + current = indirectType(current) + // Below a map or an interface the schema is open, so any key is valid. + if current.Kind() == reflect.Map || current.Kind() == reflect.Interface { + return nil + } + if current.Kind() != reflect.Struct { + return fmt.Errorf("cannot address %q below a field of type %s", segment, current) + } + + next, found := jsonField(current, segment) + if !found { + return fmt.Errorf("type %s has no field with JSON name %q", current, segment) + } + current = next + } + return nil +} + +// jsonField returns the type of the struct field that encodes under name, +// following inlined and embedded structs the way encoding/json does. +func jsonField(t reflect.Type, name string) (reflect.Type, bool) { + t = indirectType(t) + if t.Kind() != reflect.Struct { + return nil, false + } + + for i := range t.NumField() { + field := t.Field(i) + tag := field.Tag.Get("json") + if tag == "-" { + continue + } + + fieldName, _, _ := strings.Cut(tag, ",") + if fieldName == "" && field.Anonymous { + if found, ok := jsonField(field.Type, name); ok { + return found, true + } + continue + } + if fieldName == "" { + fieldName = field.Name + } + if fieldName == name { + return field.Type, true + } + } + return nil, false +} + +func indirectType(t reflect.Type) reflect.Type { + for t.Kind() == reflect.Pointer { + t = t.Elem() + } + return t +} + +func zeroStatus(obj OperatorCRD) error { + content, err := toUnstructuredContent(obj) + if err != nil { + return err + } + delete(content, "status") + return fromUnstructuredContent(content, obj) +} + +// applyOwnedStatusFields sets on staged the non-condition status of server, +// except for the owned paths, which keep the staged value. Conditions are left +// as staged. +func applyOwnedStatusFields(staged, server OperatorCRD, owned [][]string) error { + stagedContent, err := toUnstructuredContent(staged) + if err != nil { + return err + } + + serverContent, err := toUnstructuredContent(server) + if err != nil { + return err + } + + stagedStatus, _, err := unstructured.NestedMap(stagedContent, "status") + if err != nil { + return fmt.Errorf("reading staged status: %w", err) + } + + merged, _, err := unstructured.NestedMap(serverContent, "status") + if err != nil { + return fmt.Errorf("reading server status: %w", err) + } + if merged == nil { + merged = map[string]any{} + } + delete(merged, "conditions") + + for _, path := range owned { + value, found, err := unstructured.NestedFieldNoCopy(stagedStatus, path...) + if err != nil { + return fmt.Errorf("reading staged status field %q: %w", strings.Join(path, "."), err) + } + if !found { + unstructured.RemoveNestedField(merged, path...) + continue + } + if err := unstructured.SetNestedField(merged, value, path...); err != nil { + return fmt.Errorf("setting status field %q: %w", strings.Join(path, "."), err) + } + } + + if conditions, ok := stagedStatus["conditions"]; ok { + merged["conditions"] = conditions + } + stagedContent["status"] = merged + return fromUnstructuredContent(stagedContent, staged) +} + +// toUnstructuredContent returns a copy of obj's content that the caller may +// modify. The converter hands back an unstructured object's own map. +func toUnstructuredContent(obj OperatorCRD) (map[string]any, error) { + if u, ok := obj.(runtime.Unstructured); ok { + return runtime.DeepCopyJSON(u.UnstructuredContent()), nil + } + + content, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj) + if err != nil { + return nil, fmt.Errorf("converting %T to unstructured: %w", obj, err) + } + return content, nil +} + +// fromUnstructuredContent replaces obj's content. Fields the content does not +// carry are zeroed. +func fromUnstructuredContent(content map[string]any, obj OperatorCRD) error { + if u, ok := obj.(runtime.Unstructured); ok { + u.SetUnstructuredContent(content) + return nil + } + + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(content, obj); err != nil { + return fmt.Errorf("converting unstructured to %T: %w", obj, err) + } + return nil +} diff --git a/pkg/component/zz_mock_crd_test.go b/pkg/component/zz_mock_crd_test.go index 2e0173ee..380f8f41 100644 --- a/pkg/component/zz_mock_crd_test.go +++ b/pkg/component/zz_mock_crd_test.go @@ -37,6 +37,15 @@ type MockStatus struct { // prove that FlushStatus preserves staged status fields the framework does // not manage, including across a conflict retry. ObservedGeneration int64 `json:"observedGeneration,omitempty"` + // Phase and Summary are further non-condition status fields, so tests can + // split status ownership between controllers by field and by nested path. + Phase string `json:"phase,omitempty"` + Summary *MockStatusSummary `json:"summary,omitempty"` +} + +type MockStatusSummary struct { + Ready int32 `json:"ready,omitempty"` + Total int32 `json:"total,omitempty"` } func (m *MockOperatorCRD) GetStatusConditions() *[]metav1.Condition { @@ -90,6 +99,14 @@ var ( Type: "object", Properties: map[string]apiextensionsv1.JSONSchemaProps{ "observedGeneration": {Type: "integer", Format: "int64"}, + "phase": {Type: "string"}, + "summary": { + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "ready": {Type: "integer", Format: "int32"}, + "total": {Type: "integer", Format: "int32"}, + }, + }, "conditions": { Type: "array", Items: &apiextensionsv1.JSONSchemaPropsOrArray{ @@ -130,6 +147,10 @@ func (m *MockOperatorCRD) DeepCopyInto(out *MockOperatorCRD) { *out = make([]metav1.Condition, len(*in)) copy(*out, *in) } + if m.Status.Summary != nil { + summary := *m.Status.Summary + out.Status.Summary = &summary + } } func (m *MockOperatorCRD) DeepCopy() *MockOperatorCRD { diff --git a/plugin/skills/building-components/SKILL.md b/plugin/skills/building-components/SKILL.md index d4d42c51..999860fb 100644 --- a/plugin/skills/building-components/SKILL.md +++ b/plugin/skills/building-components/SKILL.md @@ -249,11 +249,11 @@ old wider ownership, making a correctness fix opt-in and invisible. On a 409 conflict `FlushStatus` keeps the staged owner as the object it writes. It fetches the server's copy into a separate object through `ReconcileContext.APIReader` (set it from the manager's `GetAPIReader()`; the default `Client` -reads from the informer cache, which can lag the conflicting write and return the same stale `resourceVersion`), takes -that copy's `resourceVersion`, restores from it only the conditions whose type this flush does not own, and retries. Two -consequences: non-condition status fields staged during the reconcile survive, and a type this flush does not own keeps -the server's value instead of the possibly stale copy the controller is holding, so another writer's concurrent update -is not rolled back. +reads from the informer cache, which can lag the conflicting write and return the same stale `resourceVersion`) and +retries. From the server's copy the retry takes the `resourceVersion`, every condition whose type this flush does not +own, and, only with `component.WithOwnedStatusFields`, every non-condition status field outside the declared paths. +Everything else keeps the staged value. So without the option, every staged non-condition field survives a conflict, +including the stale copy of a field another controller owns. For unowned types the server is the source of truth, absence included. An unowned condition the server no longer carries is dropped rather than written back, so a condition another writer removed is not resurrected, and one the server has @@ -295,6 +295,21 @@ return reconcile.Result{}, component.FlushStatus(ctx, component.ReconcileContext }, nil) // nil: owns no component conditions; on a conflict this follows the server ``` +**A status shared with another controller needs `WithOwnedStatusFields`.** When a second controller writes non-condition +fields of the same status, for example `observedGeneration`, a conflict retry without the option writes this +controller's stale copy of those fields back. Declare the fields this flush writes, by JSON name under `status` with +dots for nested fields: + +```go +component.FlushStatus(ctx, recCtx, comps, component.WithOwnedStatusFields("phase")) +``` + +On a retry a declared field keeps the staged value, and every other non-condition field takes the server's value, +absence included. `WithOwnedStatusFields()` with no paths owns no non-condition field. Once declared, an undeclared +staged field follows the server, so declare every field the controller writes, including `observedGeneration` when it is +this controller's. `FlushStatus` returns an error without writing for an empty segment, a `conditions` path, or a path +that is not a field of the owner's Go type. + **Server-side apply is for managed resources, never for the owner's status subresource.** An SSA status patch alongside `FlushStatus` puts two field managers on one conditions list, and because `FlushStatus` writes the full status the outcome depends on which call runs last: the `Update` can clobber the patched condition, or the patch can clobber what diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index 185ef7be..17e4af3a 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -821,19 +821,27 @@ reconcile, so the fields nobody staged still hold current server state. An owner writes stale values back over newer ones. On a 409 Conflict, for example when another writer updated the owner between the controller's `Get` and this call, -`FlushStatus` keeps the staged owner as the object it writes. It fetches the server's copy into a **separate** object, -takes that copy's `resourceVersion` so the retry targets the live object, restores from it only the conditions whose -type this flush does not own, and retries. The staged owner is never replaced. +`FlushStatus` keeps the staged owner as the object it writes. It fetches the server's copy into a **separate** object +and retries. The staged owner is never replaced. From the server's copy, the retry takes: + +- the `resourceVersion`, so the retry targets the live object; +- every condition whose type this flush does not own; +- with [`WithOwnedStatusFields`](#sharing-the-owners-status-with-another-controller), every non-condition status field + outside the declared paths. + +Everything else keeps the staged value. That fetch goes through `ReconcileContext.APIReader` when it is set, and through `Client` otherwise. Set it from the manager's `GetAPIReader()`. The manager's default `Client` serves reads from the informer cache, which can lag the very write that caused the conflict and hand back the same stale `resourceVersion`, so a retry through the cache may conflict again until the cache catches up. A direct read sees the server as it is. -Two things follow from that. Non-condition status fields staged during the reconcile survive a conflict, because the -object holding them is the object that gets written. And a condition type this flush does not own keeps the server's -value rather than the copy the controller happens to be holding, so a concurrent update by another writer is not rolled -back. +Two things follow from that. A condition type this flush does not own keeps the server's value rather than the copy the +controller happens to be holding, so a concurrent update by another writer is not rolled back. And without +`WithOwnedStatusFields`, every non-condition status field keeps the staged value: a field the controller staged during +the reconcile survives a conflict, and so does the stale copy of a field that another controller owns. When another +controller writes non-condition fields of the same status, declare the fields this flush owns, as described in +[Sharing the owner's status with another controller](#sharing-the-owners-status-with-another-controller). Which types count as owned comes from the components passed to `FlushStatus`: @@ -920,6 +928,47 @@ Server-Side Apply is for managed resources, never for the owner's status subreso depends on which call runs last: the `Update` can overwrite the patched condition, or the patch can overwrite what the `Update` wrote. Keep every owner-status write on the `meta.SetStatusCondition` plus `FlushStatus` path. +### Sharing the owner's status with another controller + +Sometimes two controllers write the status of one CR. For example, a status-only controller owns an `Accepted` +condition, and a second controller owns a `Ready` condition and `status.observedGeneration`. Each controller still +writes through `FlushStatus`. Condition ownership comes from the components passed to it. For non-condition fields, pass +`component.WithOwnedStatusFields` with the fields that this controller writes: + +```go +// In the status-only controller, which writes status.phase and no other non-condition field: +flushErr := component.FlushStatus(ctx, recCtx, comps, component.WithOwnedStatusFields("phase")) + +// In the other controller, which writes status.observedGeneration: +flushErr := component.FlushStatus(ctx, recCtx, comps, component.WithOwnedStatusFields("observedGeneration")) +``` + +The declaration changes only the conflict retry. The first write sends the whole staged status, as without the option. +When nothing changed on the server since the controller read the owner, that write succeeds, and the fields the +controller did not stage still hold the values it read. On a retry: + +- A declared field keeps the staged value. If the staged owner does not carry the field, the retry removes it. +- Every other non-condition status field takes the server's value. If the server does not carry the field, the retry + removes it. This is the same rule as for an unowned condition. +- Conditions follow component ownership, as described above. + +Name each field by its JSON name under `status`, with dots between nested fields: `"observedGeneration"`, +`"summary.ready"`. Do not include the `status.` prefix: `"status.phase"` is an error unless status has a field named +`status`. `WithOwnedStatusFields()` with no paths declares that the flush owns no non-condition field, which fits a +controller that writes only conditions. Paths from several options are combined. + +`FlushStatus` validates each path before it writes and returns an error, with no write, when a path: + +- is empty or has an empty segment, such as `"summary..ready"`; +- names `conditions` or a field below it, because conditions are owned through components; +- does not resolve to a field of the owner's Go type, such as a misspelled name or a path below a scalar field. Below a + map field, any key is accepted. + +Without the option, `FlushStatus` keeps the staged value of every non-condition status field on a retry. That default +keeps a controller's own `observedGeneration` write safe when it is the only writer of the status. Once you declare +fields, a staged field that you did not declare follows the server on a retry. So declare every field that the +controller writes, including `observedGeneration` when it is this controller's field. + ### Where the owner's observedGeneration is set The framework sets `ObservedGeneration` on every **component condition** it writes, from `owner.GetGeneration()`. It @@ -928,7 +977,9 @@ never writes `status.observedGeneration` on the owner itself. `OperatorCRD` requ An owner-level `status.observedGeneration` is the controller's own field. Assign it on the owner before `FlushStatus` runs, in the same staging step that writes the owner's own condition, and the single status write persists both. This -applies to a status-only controller too, as in the validation-only example above. +applies to a status-only controller too, as in the validation-only example above. When the status is +[shared with another controller](#sharing-the-owners-status-with-another-controller), only the controller that owns +`observedGeneration` assigns it, and that controller includes it in `WithOwnedStatusFields`. ```go app.Status.ObservedGeneration = app.Generation diff --git a/plugin/skills/structuring-operators/SKILL.md b/plugin/skills/structuring-operators/SKILL.md index cd4b82e5..7696d627 100644 --- a/plugin/skills/structuring-operators/SKILL.md +++ b/plugin/skills/structuring-operators/SKILL.md @@ -101,6 +101,9 @@ Consequences worth holding on to: them with `meta.RemoveStatusCondition` when a component is retired. - **Set the owner's `status.observedGeneration` yourself.** The framework stamps `ObservedGeneration` on component conditions only. +- **A status shared by two controllers gives each field one owner.** Each controller declares its non-condition fields + with `FlushStatus(..., component.WithOwnedStatusFields("phase"))`, or a conflict retry rolls back the other + controller's newer values. Only the owner of `observedGeneration` sets it. Full behavior table and reference: `ocf:building-components`, and `references/guidelines.md`, "Derive the Owner's Aggregate Condition from Component Conditions", for the practice framing. diff --git a/plugin/skills/structuring-operators/references/guidelines.md b/plugin/skills/structuring-operators/references/guidelines.md index 8f7e9440..921a499f 100644 --- a/plugin/skills/structuring-operators/references/guidelines.md +++ b/plugin/skills/structuring-operators/references/guidelines.md @@ -190,6 +190,12 @@ returns an error. If you do not want condition metrics, leave `ReconcileContext.Metrics` as `nil`; `FlushStatus` tolerates a nil recorder and skips metric emission. +When two controllers write the status of one CR, give each status field one owner, as with conditions. Each controller +passes the components that own its conditions and declares its non-condition fields with +[`WithOwnedStatusFields`](../../building-components/references/component.md#sharing-the-owners-status-with-another-controller). +Without the declaration, a conflict retry writes the controller's stale copy of the other controller's fields back over +newer values. + Building the component set from a pure resolver `(spec, version) -> []*component.Component` keeps the loop stable: enabling an optional feature changes which components the resolver returns without touching the reconcile loop. From 824f0a3dc9c5f24fac8cd9827450cc6d5e32c86a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 16 Oct 2026 14:29:21 +0100 Subject: [PATCH 02/13] fix(component): reject ambiguous owned status paths and handle null parents 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 --- docs/component.md | 4 +- pkg/component/conditions_test.go | 100 ++++++++++++++++++ pkg/component/status_fields.go | 89 +++++++++++----- pkg/component/zz_mock_crd_test.go | 14 +++ plugin/skills/building-components/SKILL.md | 2 +- .../references/component.md | 4 +- 6 files changed, 183 insertions(+), 30 deletions(-) diff --git a/docs/component.md b/docs/component.md index 5b1f84a1..b11cf36d 100644 --- a/docs/component.md +++ b/docs/component.md @@ -959,7 +959,9 @@ controller that writes only conditions. Paths from several options are combined. - is empty or has an empty segment, such as `"summary..ready"`; - names `conditions` or a field below it, because conditions are owned through components; - does not resolve to a field of the owner's Go type, such as a misspelled name or a path below a scalar field. Below a - map field, any key is accepted. + map field, any key is accepted; +- is ambiguous: a segment matches more than one field, because a struct declares the JSON name and also gets it from an + embedded struct, or gets it from two embedded structs. Without the option, `FlushStatus` keeps the staged value of every non-condition status field on a retry. That default keeps a controller's own `observedGeneration` write safe when it is the only writer of the status. Once you declare diff --git a/pkg/component/conditions_test.go b/pkg/component/conditions_test.go index 76ceacc6..b608c6e0 100644 --- a/pkg/component/conditions_test.go +++ b/pkg/component/conditions_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "reflect" "testing" "time" @@ -958,6 +959,40 @@ func TestFlushStatusOwnedStatusFields(t *testing.T) { assert.Equal(t, MockStatusSummary{Ready: 2, Total: 4}, *persisted.Status.Summary) }) + t.Run("sets an owned nested field below a parent the server holds as null", func(t *testing.T) { + k8sClient, owner := readThenRace(t, &MockOperatorCRD{}, + func(o *MockOperatorCRD) { o.Status.ObservedGeneration = 2 }) + + owner.Status.Details = &MockStatusSummary{Ready: 1} + + require.NoError(t, FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("details.ready"), + )) + + persisted := persistedOf(t, k8sClient, owner) + require.NotNil(t, persisted.Status.Details) + assert.Equal(t, int32(1), persisted.Status.Details.Ready) + assert.Equal(t, int64(2), persisted.Status.ObservedGeneration) + }) + + t.Run("removes an owned nested field below a parent the staged owner holds as null", func(t *testing.T) { + k8sClient, owner := readThenRace(t, + &MockOperatorCRD{Status: MockStatus{Details: &MockStatusSummary{Ready: 1}}}, + func(o *MockOperatorCRD) { o.Status.ObservedGeneration = 2 }) + + owner.Status.Details = nil + + require.NoError(t, FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("details.ready"), + )) + + persisted := persistedOf(t, k8sClient, owner) + if persisted.Status.Details != nil { + assert.Zero(t, persisted.Status.Details.Ready) + } + assert.Equal(t, int64(2), persisted.Status.ObservedGeneration) + }) + t.Run("combines the paths of several options", func(t *testing.T) { k8sClient, owner := readThenRace(t, &MockOperatorCRD{}, func(o *MockOperatorCRD) { o.Status.Summary = &MockStatusSummary{Total: 4} }) @@ -1010,6 +1045,71 @@ func TestFlushStatusOwnedStatusFields(t *testing.T) { }) } +// TestJSONFields covers how path validation finds a field through embedded +// structs. More than one candidate makes the path ambiguous, which validation +// rejects rather than guessing which field encoding/json encodes. +func TestJSONFields(t *testing.T) { + type base struct { + Phase string `json:"phase"` + } + type otherBase struct { + Phase string `json:"phase"` + } + type phaseObject struct { + Ready bool `json:"ready"` + } + + t.Run("finds a field of an embedded struct", func(t *testing.T) { + type status struct { + base + } + + assert.Equal(t, []reflect.Type{reflect.TypeOf("")}, jsonFields(reflect.TypeOf(status{}), "phase")) + }) + + t.Run("finds a field of a struct embedded by pointer", func(t *testing.T) { + type status struct { + *base + } + + assert.Equal(t, []reflect.Type{reflect.TypeOf("")}, jsonFields(reflect.TypeOf(status{}), "phase")) + }) + + // go vet rejects a struct literal that repeats a JSON name, so the + // ambiguous types are built at run time. + baseType := reflect.TypeOf(base{}) + embed := func(name string, typ reflect.Type) reflect.StructField { + return reflect.StructField{Name: name, Type: typ, Anonymous: true} + } + + t.Run("finds both a declared field and the embedded field it shadows", func(t *testing.T) { + status := reflect.StructOf([]reflect.StructField{ + embed("Base", baseType), + {Name: "Phase", Type: reflect.TypeOf(phaseObject{}), Tag: `json:"phase"`}, + }) + + assert.Len(t, jsonFields(status, "phase"), 2) + }) + + t.Run("finds the field of each embedded struct that provides the name", func(t *testing.T) { + status := reflect.StructOf([]reflect.StructField{ + embed("Base", baseType), + embed("OtherBase", reflect.TypeOf(otherBase{})), + }) + + assert.Len(t, jsonFields(status, "phase"), 2) + }) + + t.Run("skips a field tagged to be ignored", func(t *testing.T) { + type status struct { + Phase string `json:"-"` + Hidden string `json:"phase"` + } + + assert.Equal(t, []reflect.Type{reflect.TypeOf("")}, jsonFields(reflect.TypeOf(status{}), "phase")) + }) +} + // decodingReader reads like the REST client: it decodes the server's JSON into // the object passed in, so fields the JSON omits are left as they were. type decodingReader struct { diff --git a/pkg/component/status_fields.go b/pkg/component/status_fields.go index 87ab3e3e..fd416b85 100644 --- a/pkg/component/status_fields.go +++ b/pkg/component/status_fields.go @@ -29,9 +29,9 @@ type flushOptions struct { // Paths are relative to status: write "phase", not "status.phase". A path must // not name "conditions" or a field below it: condition ownership comes from the // components passed to [FlushStatus]. For a typed owner every path must resolve -// to a field of the owner's Go type, so a misspelled path is an error rather -// than a silent loss of ownership. FlushStatus validates the paths before it -// writes and returns an error for an invalid one. +// to exactly one field of the owner's Go type, so a misspelled or ambiguous path +// is an error rather than a silent loss of ownership. FlushStatus validates the +// paths before it writes and returns an error for an invalid one. // // The declaration takes effect only on a 409 conflict retry. An owned field // keeps the staged value, and a field the staged owner does not carry is @@ -80,12 +80,8 @@ func validateStatusFieldPath(owner OperatorCRD, segments []string) error { return nil } - current, ok := jsonField(reflect.TypeOf(owner), "status") - if !ok { - return fmt.Errorf("owner type %T has no status field", owner) - } - - for _, segment := range segments { + current := reflect.TypeOf(owner) + for _, segment := range append([]string{"status"}, segments...) { current = indirectType(current) // Below a map or an interface the schema is open, so any key is valid. if current.Kind() == reflect.Map || current.Kind() == reflect.Interface { @@ -95,23 +91,27 @@ func validateStatusFieldPath(owner OperatorCRD, segments []string) error { return fmt.Errorf("cannot address %q below a field of type %s", segment, current) } - next, found := jsonField(current, segment) - if !found { + candidates := jsonFields(current, segment) + switch len(candidates) { + case 0: return fmt.Errorf("type %s has no field with JSON name %q", current, segment) + case 1: + current = candidates[0] + default: + // encoding/json picks one of several such fields by tag and depth + // rules. Rejecting the path keeps ownership from landing on a + // field other than the one that is encoded. + return fmt.Errorf("type %s has %d fields with JSON name %q, directly or through embedded structs", + current, len(candidates), segment) } - current = next } return nil } -// jsonField returns the type of the struct field that encodes under name, -// following inlined and embedded structs the way encoding/json does. -func jsonField(t reflect.Type, name string) (reflect.Type, bool) { - t = indirectType(t) - if t.Kind() != reflect.Struct { - return nil, false - } - +// jsonFields returns the type of every field with the JSON name name, declared +// on t or on a struct that t embeds at any depth. +func jsonFields(t reflect.Type, name string) []reflect.Type { + var found []reflect.Type for i := range t.NumField() { field := t.Field(i) tag := field.Tag.Get("json") @@ -120,20 +120,19 @@ func jsonField(t reflect.Type, name string) (reflect.Type, bool) { } fieldName, _, _ := strings.Cut(tag, ",") - if fieldName == "" && field.Anonymous { - if found, ok := jsonField(field.Type, name); ok { - return found, true - } + embedded := indirectType(field.Type) + if fieldName == "" && field.Anonymous && embedded.Kind() == reflect.Struct { + found = append(found, jsonFields(embedded, name)...) continue } if fieldName == "" { fieldName = field.Name } if fieldName == name { - return field.Type, true + found = append(found, field.Type) } } - return nil, false + return found } func indirectType(t reflect.Type) reflect.Type { @@ -181,7 +180,7 @@ func applyOwnedStatusFields(staged, server OperatorCRD, owned [][]string) error delete(merged, "conditions") for _, path := range owned { - value, found, err := unstructured.NestedFieldNoCopy(stagedStatus, path...) + value, found, err := lookupStatusField(stagedStatus, path) if err != nil { return fmt.Errorf("reading staged status field %q: %w", strings.Join(path, "."), err) } @@ -189,6 +188,10 @@ func applyOwnedStatusFields(staged, server OperatorCRD, owned [][]string) error unstructured.RemoveNestedField(merged, path...) continue } + + // A nullable parent without omitempty encodes as null, and + // SetNestedField cannot descend through a null. + removeNullParent(merged, path) if err := unstructured.SetNestedField(merged, value, path...); err != nil { return fmt.Errorf("setting status field %q: %w", strings.Join(path, "."), err) } @@ -201,6 +204,38 @@ func applyOwnedStatusFields(staged, server OperatorCRD, owned [][]string) error return fromUnstructuredContent(stagedContent, staged) } +// lookupStatusField returns the value at path. A null parent counts as +// absent, the same as a missing one. +func lookupStatusField(status map[string]any, path []string) (any, bool, error) { + var current any = status + for i, segment := range path { + if current == nil { + return nil, false, nil + } + + object, ok := current.(map[string]any) + if !ok { + return nil, false, fmt.Errorf("%q is not an object", strings.Join(path[:i], ".")) + } + + current, ok = object[segment] + if !ok { + return nil, false, nil + } + } + return current, true, nil +} + +func removeNullParent(status map[string]any, path []string) { + for i := 1; i < len(path); i++ { + value, found, _ := lookupStatusField(status, path[:i]) + if found && value == nil { + unstructured.RemoveNestedField(status, path[:i]...) + return + } + } +} + // toUnstructuredContent returns a copy of obj's content that the caller may // modify. The converter hands back an unstructured object's own map. func toUnstructuredContent(obj OperatorCRD) (map[string]any, error) { diff --git a/pkg/component/zz_mock_crd_test.go b/pkg/component/zz_mock_crd_test.go index 380f8f41..dfec022e 100644 --- a/pkg/component/zz_mock_crd_test.go +++ b/pkg/component/zz_mock_crd_test.go @@ -41,6 +41,8 @@ type MockStatus struct { // split status ownership between controllers by field and by nested path. Phase string `json:"phase,omitempty"` Summary *MockStatusSummary `json:"summary,omitempty"` + // Details lacks omitempty so tests can cover a parent the server holds as null. + Details *MockStatusSummary `json:"details"` } type MockStatusSummary struct { @@ -107,6 +109,14 @@ var ( "total": {Type: "integer", Format: "int32"}, }, }, + "details": { + Type: "object", + Nullable: true, + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "ready": {Type: "integer", Format: "int32"}, + "total": {Type: "integer", Format: "int32"}, + }, + }, "conditions": { Type: "array", Items: &apiextensionsv1.JSONSchemaPropsOrArray{ @@ -151,6 +161,10 @@ func (m *MockOperatorCRD) DeepCopyInto(out *MockOperatorCRD) { summary := *m.Status.Summary out.Status.Summary = &summary } + if m.Status.Details != nil { + details := *m.Status.Details + out.Status.Details = &details + } } func (m *MockOperatorCRD) DeepCopy() *MockOperatorCRD { diff --git a/plugin/skills/building-components/SKILL.md b/plugin/skills/building-components/SKILL.md index 999860fb..f503884c 100644 --- a/plugin/skills/building-components/SKILL.md +++ b/plugin/skills/building-components/SKILL.md @@ -308,7 +308,7 @@ On a retry a declared field keeps the staged value, and every other non-conditio absence included. `WithOwnedStatusFields()` with no paths owns no non-condition field. Once declared, an undeclared staged field follows the server, so declare every field the controller writes, including `observedGeneration` when it is this controller's. `FlushStatus` returns an error without writing for an empty segment, a `conditions` path, or a path -that is not a field of the owner's Go type. +that does not resolve to exactly one field of the owner's Go type. **Server-side apply is for managed resources, never for the owner's status subresource.** An SSA status patch alongside `FlushStatus` puts two field managers on one conditions list, and because `FlushStatus` writes the full status the diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index 17e4af3a..0b0ccd04 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -962,7 +962,9 @@ controller that writes only conditions. Paths from several options are combined. - is empty or has an empty segment, such as `"summary..ready"`; - names `conditions` or a field below it, because conditions are owned through components; - does not resolve to a field of the owner's Go type, such as a misspelled name or a path below a scalar field. Below a - map field, any key is accepted. + map field, any key is accepted; +- is ambiguous: a segment matches more than one field, because a struct declares the JSON name and also gets it from an + embedded struct, or gets it from two embedded structs. Without the option, `FlushStatus` keeps the staged value of every non-condition status field on a retry. That default keeps a controller's own `observedGeneration` write safe when it is the only writer of the status. Once you declare From 2aa366630439c8186b17d9160d99a8d69fe750c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 16 Oct 2026 14:37:25 +0100 Subject: [PATCH 03/13] fix(component): keep owned conditions under any JSON name on a retry 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 --- pkg/component/conditions_test.go | 86 ++++++++++++++++++++++++++++++-- pkg/component/status_fields.go | 39 +++++++++++---- 2 files changed, 111 insertions(+), 14 deletions(-) diff --git a/pkg/component/conditions_test.go b/pkg/component/conditions_test.go index b608c6e0..f0b8948c 100644 --- a/pkg/component/conditions_test.go +++ b/pkg/component/conditions_test.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "reflect" + "slices" "testing" "time" @@ -1012,6 +1013,40 @@ func TestFlushStatusOwnedStatusFields(t *testing.T) { assert.Equal(t, int32(4), persisted.Status.Summary.Total) }) + t.Run("keeps owned conditions stored under a JSON name other than conditions", func(t *testing.T) { + // OperatorCRD reaches the conditions through GetStatusConditions, so + // their JSON name is the owner type's choice. + checksScheme := runtime.NewScheme() + checksScheme.AddKnownTypes(GroupVersion, &checksOwner{}) + initial := &checksOwner{ObjectMeta: metav1.ObjectMeta{Name: "test-owner", Namespace: "default"}} + k8sClient := fake.NewClientBuilder(). + WithScheme(checksScheme).WithStatusSubresource(initial).WithObjects(initial).Build() + + owner := &checksOwner{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), owner)) + + other := &checksOwner{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), other)) + other.Status.Phase = "Running" + require.NoError(t, k8sClient.Status().Update(ctx, other)) + + applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ + Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", + }) + + require.NoError(t, FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, []*Component{infraComponent(t)}, + WithOwnedStatusFields(), + )) + + persisted := &checksOwner{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), persisted)) + found := meta.FindStatusCondition(persisted.Status.Checks, "InfraReady") + require.NotNil(t, found, "an owned condition must keep the staged value") + assert.Equal(t, "Healthy", found.Reason) + assert.Equal(t, "Running", persisted.Status.Phase) + }) + t.Run("rejects an invalid path before writing", func(t *testing.T) { for name, path := range map[string]string{ "empty": "", @@ -1064,7 +1099,7 @@ func TestJSONFields(t *testing.T) { base } - assert.Equal(t, []reflect.Type{reflect.TypeOf("")}, jsonFields(reflect.TypeOf(status{}), "phase")) + assert.Equal(t, []reflect.Type{reflect.TypeOf("")}, jsonFields(reflect.TypeOf(status{}), "phase", nil)) }) t.Run("finds a field of a struct embedded by pointer", func(t *testing.T) { @@ -1072,7 +1107,7 @@ func TestJSONFields(t *testing.T) { *base } - assert.Equal(t, []reflect.Type{reflect.TypeOf("")}, jsonFields(reflect.TypeOf(status{}), "phase")) + assert.Equal(t, []reflect.Type{reflect.TypeOf("")}, jsonFields(reflect.TypeOf(status{}), "phase", nil)) }) // go vet rejects a struct literal that repeats a JSON name, so the @@ -1088,7 +1123,7 @@ func TestJSONFields(t *testing.T) { {Name: "Phase", Type: reflect.TypeOf(phaseObject{}), Tag: `json:"phase"`}, }) - assert.Len(t, jsonFields(status, "phase"), 2) + assert.Len(t, jsonFields(status, "phase", nil), 2) }) t.Run("finds the field of each embedded struct that provides the name", func(t *testing.T) { @@ -1097,7 +1132,13 @@ func TestJSONFields(t *testing.T) { embed("OtherBase", reflect.TypeOf(otherBase{})), }) - assert.Len(t, jsonFields(status, "phase"), 2) + assert.Len(t, jsonFields(status, "phase", nil), 2) + }) + + t.Run("does not recurse forever through a struct that embeds itself", func(t *testing.T) { + found := jsonFields(reflect.TypeOf(selfEmbedding{}), "phase", nil) + + assert.Equal(t, []reflect.Type{reflect.TypeOf("")}, found) }) t.Run("skips a field tagged to be ignored", func(t *testing.T) { @@ -1106,10 +1147,45 @@ func TestJSONFields(t *testing.T) { Hidden string `json:"phase"` } - assert.Equal(t, []reflect.Type{reflect.TypeOf("")}, jsonFields(reflect.TypeOf(status{}), "phase")) + assert.Equal(t, []reflect.Type{reflect.TypeOf("")}, jsonFields(reflect.TypeOf(status{}), "phase", nil)) }) } +// selfEmbedding is valid Go and valid encoding/json input: the embedded +// pointer can stay nil while Phase is encoded. +type selfEmbedding struct { + *selfEmbedding //nolint:unused // read only by reflection + Phase string `json:"phase"` +} + +// checksOwner is an OperatorCRD whose conditions encode as "checks". +type checksOwner struct { + metav1.TypeMeta `json:",inline"` + metav1.ObjectMeta `json:"metadata,omitempty"` + + Status checksStatus `json:"status,omitempty"` +} + +type checksStatus struct { + Checks []metav1.Condition `json:"checks,omitempty"` + Phase string `json:"phase,omitempty"` +} + +func (o *checksOwner) GetStatusConditions() *[]metav1.Condition { + return &o.Status.Checks +} + +func (o *checksOwner) GetKind() string { + return "checksOwner" +} + +func (o *checksOwner) DeepCopyObject() runtime.Object { + out := *o + o.ObjectMeta.DeepCopyInto(&out.ObjectMeta) //nolint:staticcheck // naming ObjectMeta keeps the copy readable + out.Status.Checks = slices.Clone(o.Status.Checks) + return &out +} + // decodingReader reads like the REST client: it decodes the server's JSON into // the object passed in, so fields the JSON omits are left as they were. type decodingReader struct { diff --git a/pkg/component/status_fields.go b/pkg/component/status_fields.go index fd416b85..f23e8798 100644 --- a/pkg/component/status_fields.go +++ b/pkg/component/status_fields.go @@ -3,7 +3,9 @@ package component import ( "errors" "fmt" + "maps" "reflect" + "slices" "strings" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -91,7 +93,7 @@ func validateStatusFieldPath(owner OperatorCRD, segments []string) error { return fmt.Errorf("cannot address %q below a field of type %s", segment, current) } - candidates := jsonFields(current, segment) + candidates := jsonFields(current, segment, nil) switch len(candidates) { case 0: return fmt.Errorf("type %s has no field with JSON name %q", current, segment) @@ -109,8 +111,21 @@ func validateStatusFieldPath(owner OperatorCRD, segments []string) error { } // jsonFields returns the type of every field with the JSON name name, declared -// on t or on a struct that t embeds at any depth. -func jsonFields(t reflect.Type, name string) []reflect.Type { +// on t or on a struct that t embeds at any depth. Pass nil for visiting. +func jsonFields(t reflect.Type, name string, visiting map[reflect.Type]bool) []reflect.Type { + // A struct can embed itself through a pointer. visiting holds the structs + // on the current embedding chain, so such a chain ends instead of looping. + // It is copied per chain, so the same struct embedded twice side by side + // still counts twice toward ambiguity. + if visiting[t] { + return nil + } + visiting = maps.Clone(visiting) + if visiting == nil { + visiting = map[reflect.Type]bool{} + } + visiting[t] = true + var found []reflect.Type for i := range t.NumField() { field := t.Field(i) @@ -122,7 +137,7 @@ func jsonFields(t reflect.Type, name string) []reflect.Type { fieldName, _, _ := strings.Cut(tag, ",") embedded := indirectType(field.Type) if fieldName == "" && field.Anonymous && embedded.Kind() == reflect.Struct { - found = append(found, jsonFields(embedded, name)...) + found = append(found, jsonFields(embedded, name, visiting)...) continue } if fieldName == "" { @@ -155,6 +170,11 @@ func zeroStatus(obj OperatorCRD) error { // except for the owned paths, which keep the staged value. Conditions are left // as staged. func applyOwnedStatusFields(staged, server OperatorCRD, owned [][]string) error { + // GetStatusConditions decides where the conditions live, and their JSON + // name can be anything, so they are carried across the conversion through + // the accessor rather than by key. + conditions := slices.Clone(*staged.GetStatusConditions()) + stagedContent, err := toUnstructuredContent(staged) if err != nil { return err @@ -177,7 +197,6 @@ func applyOwnedStatusFields(staged, server OperatorCRD, owned [][]string) error if merged == nil { merged = map[string]any{} } - delete(merged, "conditions") for _, path := range owned { value, found, err := lookupStatusField(stagedStatus, path) @@ -197,11 +216,13 @@ func applyOwnedStatusFields(staged, server OperatorCRD, owned [][]string) error } } - if conditions, ok := stagedStatus["conditions"]; ok { - merged["conditions"] = conditions - } stagedContent["status"] = merged - return fromUnstructuredContent(stagedContent, staged) + if err := fromUnstructuredContent(stagedContent, staged); err != nil { + return err + } + + *staged.GetStatusConditions() = conditions + return nil } // lookupStatusField returns the value at path. A null parent counts as From 175fac6b907964f6af2f3dbe4988fd3b7e289d37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 16 Oct 2026 14:46:48 +0100 Subject: [PATCH 04/13] fix(component): validate owned paths against the status JSON 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 --- docs/component.md | 5 +- pkg/component/conditions_test.go | 84 ++++++++++++++++++- pkg/component/status_fields.go | 74 ++++++++++++---- .../references/component.md | 5 +- 4 files changed, 148 insertions(+), 20 deletions(-) diff --git a/docs/component.md b/docs/component.md index b11cf36d..30732f01 100644 --- a/docs/component.md +++ b/docs/component.md @@ -958,8 +958,9 @@ controller that writes only conditions. Paths from several options are combined. - is empty or has an empty segment, such as `"summary..ready"`; - names `conditions` or a field below it, because conditions are owned through components; -- does not resolve to a field of the owner's Go type, such as a misspelled name or a path below a scalar field. Below a - map field, any key is accepted; +- does not resolve to a field that the owner's Go type encodes in its JSON, such as a misspelled name, an unexported + field, a path below a scalar field, or a path below a type that encodes itself, such as `metav1.Time`. Below a map + field, any key is accepted; - is ambiguous: a segment matches more than one field, because a struct declares the JSON name and also gets it from an embedded struct, or gets it from two embedded structs. diff --git a/pkg/component/conditions_test.go b/pkg/component/conditions_test.go index f0b8948c..4d3c09e4 100644 --- a/pkg/component/conditions_test.go +++ b/pkg/component/conditions_test.go @@ -1047,6 +1047,53 @@ func TestFlushStatusOwnedStatusFields(t *testing.T) { assert.Equal(t, "Running", persisted.Status.Phase) }) + t.Run("sets an owned field when the server holds the whole status as null", func(t *testing.T) { + nullScheme := runtime.NewScheme() + nullScheme.AddKnownTypes(GroupVersion, &nullableStatusOwner{}) + initial := &nullableStatusOwner{ObjectMeta: metav1.ObjectMeta{Name: "test-owner", Namespace: "default"}} + k8sClient := fake.NewClientBuilder(). + WithScheme(nullScheme).WithStatusSubresource(initial).WithObjects(initial).Build() + + owner := &nullableStatusOwner{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), owner)) + + other := &nullableStatusOwner{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), other)) + other.Labels = map[string]string{"touched": "true"} + require.NoError(t, k8sClient.Update(ctx, other)) + + owner.Status = &checksStatus{Phase: "Accepted"} + + require.NoError(t, FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("phase"), + )) + + persisted := &nullableStatusOwner{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), persisted)) + require.NotNil(t, persisted.Status) + assert.Equal(t, "Accepted", persisted.Status.Phase) + }) + + t.Run("rejects a path that the status JSON does not contain", func(t *testing.T) { + for name, path := range map[string]string{ + "unexported field": "internal", + "below a type that encodes itself": "lastSync.Time", + "unexported field of such a type": "lastSync.wall", + } { + t.Run(name, func(t *testing.T) { + owner := &checksOwner{ObjectMeta: metav1.ObjectMeta{Name: "test-owner", Namespace: "default"}} + k8sClient := fake.NewClientBuilder().WithScheme(scheme).Build() + + err := FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields(path), + ) + + require.Error(t, err) + assert.Contains(t, err.Error(), fmt.Sprintf("%q", path)) + }) + } + }) + t.Run("rejects an invalid path before writing", func(t *testing.T) { for name, path := range map[string]string{ "empty": "", @@ -1167,8 +1214,10 @@ type checksOwner struct { } type checksStatus struct { - Checks []metav1.Condition `json:"checks,omitempty"` - Phase string `json:"phase,omitempty"` + Checks []metav1.Condition `json:"checks,omitempty"` + Phase string `json:"phase,omitempty"` + LastSync metav1.Time `json:"lastSync,omitzero"` + internal string //nolint:unused // read only by reflection } func (o *checksOwner) GetStatusConditions() *[]metav1.Condition { @@ -1186,6 +1235,37 @@ func (o *checksOwner) DeepCopyObject() runtime.Object { return &out } +// nullableStatusOwner encodes a nil status as null, because the field has no +// omitempty. +type nullableStatusOwner struct { + metav1.TypeMeta `json:",inline"` + metav1.ObjectMeta `json:"metadata,omitempty"` + + Status *checksStatus `json:"status"` +} + +func (o *nullableStatusOwner) GetStatusConditions() *[]metav1.Condition { + if o.Status == nil { + o.Status = &checksStatus{} + } + return &o.Status.Checks +} + +func (o *nullableStatusOwner) GetKind() string { + return "nullableStatusOwner" +} + +func (o *nullableStatusOwner) DeepCopyObject() runtime.Object { + out := *o + o.ObjectMeta.DeepCopyInto(&out.ObjectMeta) //nolint:staticcheck // naming ObjectMeta keeps the copy readable + if o.Status != nil { + status := *o.Status + status.Checks = slices.Clone(o.Status.Checks) + out.Status = &status + } + return &out +} + // decodingReader reads like the REST client: it decodes the server's JSON into // the object passed in, so fields the JSON omits are left as they were. type decodingReader struct { diff --git a/pkg/component/status_fields.go b/pkg/component/status_fields.go index f23e8798..af0a8ca6 100644 --- a/pkg/component/status_fields.go +++ b/pkg/component/status_fields.go @@ -1,6 +1,8 @@ package component import ( + "bytes" + "encoding/json" "errors" "fmt" "maps" @@ -12,6 +14,8 @@ import ( "k8s.io/apimachinery/pkg/runtime" ) +var jsonMarshaler = reflect.TypeFor[json.Marshaler]() + // FlushOption configures a [FlushStatus] call. type FlushOption func(*flushOptions) @@ -31,8 +35,8 @@ type flushOptions struct { // Paths are relative to status: write "phase", not "status.phase". A path must // not name "conditions" or a field below it: condition ownership comes from the // components passed to [FlushStatus]. For a typed owner every path must resolve -// to exactly one field of the owner's Go type, so a misspelled or ambiguous path -// is an error rather than a silent loss of ownership. FlushStatus validates the +// to exactly one field in the JSON of the owner's Go type, so a misspelled or +// ambiguous path is an error rather than a silent loss of ownership. FlushStatus validates the // paths before it writes and returns an error for an invalid one. // // The declaration takes effect only on a 409 conflict retry. An owned field @@ -83,8 +87,14 @@ func validateStatusFieldPath(owner OperatorCRD, segments []string) error { } current := reflect.TypeOf(owner) - for _, segment := range append([]string{"status"}, segments...) { + for i, segment := range append([]string{"status"}, segments...) { current = indirectType(current) + // A type such as metav1.Time encodes itself, often as a string, so its + // Go fields say nothing about its JSON shape. The owner type itself is + // exempt: only its status is addressed. + if i > 0 && reflect.PointerTo(current).Implements(jsonMarshaler) { + return fmt.Errorf("cannot address %q below a field of type %s, which encodes itself", segment, current) + } // Below a map or an interface the schema is open, so any key is valid. if current.Kind() == reflect.Map || current.Kind() == reflect.Interface { return nil @@ -134,6 +144,12 @@ func jsonFields(t reflect.Type, name string, visiting map[reflect.Type]bool) []r continue } + // encoding/json skips unexported fields, except that the exported + // fields of an unexported embedded struct are promoted. + if !field.IsExported() && !field.Anonymous { + continue + } + fieldName, _, _ := strings.Cut(tag, ",") embedded := indirectType(field.Type) if fieldName == "" && field.Anonymous && embedded.Kind() == reflect.Struct { @@ -185,18 +201,15 @@ func applyOwnedStatusFields(staged, server OperatorCRD, owned [][]string) error return err } - stagedStatus, _, err := unstructured.NestedMap(stagedContent, "status") + stagedStatus, err := statusOf(stagedContent) if err != nil { return fmt.Errorf("reading staged status: %w", err) } - merged, _, err := unstructured.NestedMap(serverContent, "status") + merged, err := statusOf(serverContent) if err != nil { return fmt.Errorf("reading server status: %w", err) } - if merged == nil { - merged = map[string]any{} - } for _, path := range owned { value, found, err := lookupStatusField(stagedStatus, path) @@ -257,30 +270,63 @@ func removeNullParent(status map[string]any, path []string) { } } +// statusOf returns the status map of content, which the caller may modify. A +// status that is absent or null, as a nullable status without omitempty +// encodes, is an empty map. +func statusOf(content map[string]any) (map[string]any, error) { + status, ok := content["status"] + if !ok || status == nil { + return map[string]any{}, nil + } + + object, ok := status.(map[string]any) + if !ok { + return nil, fmt.Errorf("status is a %T, not an object", status) + } + return runtime.DeepCopyJSON(object), nil +} + // toUnstructuredContent returns a copy of obj's content that the caller may -// modify. The converter hands back an unstructured object's own map. +// modify. UnstructuredContent returns the object's own map, not a copy. func toUnstructuredContent(obj OperatorCRD) (map[string]any, error) { if u, ok := obj.(runtime.Unstructured); ok { return runtime.DeepCopyJSON(u.UnstructuredContent()), nil } - content, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj) + // encoding/json, not runtime.DefaultUnstructuredConverter: the converter + // panics on a struct with an unexported field when it converts back. + // json.Number keeps int64 values exact. + data, err := json.Marshal(obj) if err != nil { - return nil, fmt.Errorf("converting %T to unstructured: %w", obj, err) + return nil, fmt.Errorf("encoding %T: %w", obj, err) + } + + decoder := json.NewDecoder(bytes.NewReader(data)) + decoder.UseNumber() + var content map[string]any + if err := decoder.Decode(&content); err != nil { + return nil, fmt.Errorf("decoding %T: %w", obj, err) } return content, nil } // fromUnstructuredContent replaces obj's content. Fields the content does not -// carry are zeroed. +// carry are zeroed, unexported fields included. func fromUnstructuredContent(content map[string]any, obj OperatorCRD) error { if u, ok := obj.(runtime.Unstructured); ok { u.SetUnstructuredContent(content) return nil } - if err := runtime.DefaultUnstructuredConverter.FromUnstructured(content, obj); err != nil { - return fmt.Errorf("converting unstructured to %T: %w", obj, err) + data, err := json.Marshal(content) + if err != nil { + return fmt.Errorf("encoding content for %T: %w", obj, err) + } + + // Decoding into a non-empty object keeps the fields the content omits. + reflect.ValueOf(obj).Elem().SetZero() + if err := json.Unmarshal(data, obj); err != nil { + return fmt.Errorf("decoding into %T: %w", obj, err) } return nil } diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index 0b0ccd04..95104c3a 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -961,8 +961,9 @@ controller that writes only conditions. Paths from several options are combined. - is empty or has an empty segment, such as `"summary..ready"`; - names `conditions` or a field below it, because conditions are owned through components; -- does not resolve to a field of the owner's Go type, such as a misspelled name or a path below a scalar field. Below a - map field, any key is accepted; +- does not resolve to a field that the owner's Go type encodes in its JSON, such as a misspelled name, an unexported + field, a path below a scalar field, or a path below a type that encodes itself, such as `metav1.Time`. Below a map + field, any key is accepted; - is ambiguous: a segment matches more than one field, because a struct declares the JSON name and also gets it from an embedded struct, or gets it from two embedded structs. From 20d4fadf08a1be949365baf3030abdfa3a697b37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 16 Oct 2026 14:55:31 +0100 Subject: [PATCH 05/13] fix(component): keep integers exact and reject text-encoded parents on 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 --- go.mod | 2 +- pkg/component/conditions_test.go | 59 ++++++++++++++++++++++++++++++-- pkg/component/status_fields.go | 27 +++++++++------ 3 files changed, 74 insertions(+), 14 deletions(-) diff --git a/go.mod b/go.mod index 60899b52..09b5effe 100644 --- a/go.mod +++ b/go.mod @@ -18,6 +18,7 @@ require ( k8s.io/client-go v0.37.1 k8s.io/utils v0.0.0-20260707023825-cf1189d6abe3 sigs.k8s.io/controller-runtime v0.25.2 + sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 sigs.k8s.io/yaml v1.6.0 ) @@ -84,7 +85,6 @@ require ( gopkg.in/yaml.v3 v3.0.1 // indirect k8s.io/klog/v2 v2.140.0 // indirect k8s.io/kube-openapi v0.0.0-20260721132016-d427ff9ee9ad // indirect - sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect sigs.k8s.io/randfill v1.0.0 // indirect sigs.k8s.io/structured-merge-diff/v6 v6.4.2 // indirect ) diff --git a/pkg/component/conditions_test.go b/pkg/component/conditions_test.go index 4d3c09e4..d20c65d9 100644 --- a/pkg/component/conditions_test.go +++ b/pkg/component/conditions_test.go @@ -1074,11 +1074,41 @@ func TestFlushStatusOwnedStatusFields(t *testing.T) { assert.Equal(t, "Accepted", persisted.Status.Phase) }) + t.Run("keeps a large integer in a dynamic status field exact across the retry", func(t *testing.T) { + const big = int64(1<<53 + 1) // the first integer a float64 cannot hold + + checksScheme := runtime.NewScheme() + checksScheme.AddKnownTypes(GroupVersion, &checksOwner{}) + initial := &checksOwner{ObjectMeta: metav1.ObjectMeta{Name: "test-owner", Namespace: "default"}} + k8sClient := fake.NewClientBuilder(). + WithScheme(checksScheme).WithStatusSubresource(initial).WithObjects(initial).Build() + + owner := &checksOwner{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), owner)) + + other := &checksOwner{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), other)) + other.Status.Extra = map[string]any{"big": big} + require.NoError(t, k8sClient.Status().Update(ctx, other)) + + owner.Status.Phase = "Accepted" + + require.NoError(t, FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("phase"), + )) + + persisted := &checksOwner{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), persisted)) + assert.Equal(t, big, persisted.Status.Extra["big"]) + assert.Equal(t, "Accepted", persisted.Status.Phase) + }) + t.Run("rejects a path that the status JSON does not contain", func(t *testing.T) { for name, path := range map[string]string{ - "unexported field": "internal", - "below a type that encodes itself": "lastSync.Time", - "unexported field of such a type": "lastSync.wall", + "below a type that encodes itself as text": "level.value", + "unexported field": "internal", + "below a type that encodes itself": "lastSync.Time", + "unexported field of such a type": "lastSync.wall", } { t.Run(name, func(t *testing.T) { owner := &checksOwner{ObjectMeta: metav1.ObjectMeta{Name: "test-owner", Namespace: "default"}} @@ -1217,9 +1247,26 @@ type checksStatus struct { Checks []metav1.Condition `json:"checks,omitempty"` Phase string `json:"phase,omitempty"` LastSync metav1.Time `json:"lastSync,omitzero"` + Level textLevel `json:"level,omitzero"` + Extra map[string]any `json:"extra,omitempty"` internal string //nolint:unused // read only by reflection } +// textLevel encodes as a JSON string through MarshalText, so its Value field +// never appears in the JSON. +type textLevel struct { + Value string `json:"value"` +} + +func (l textLevel) MarshalText() ([]byte, error) { + return []byte(l.Value), nil +} + +func (l *textLevel) UnmarshalText(text []byte) error { + l.Value = string(text) + return nil +} + func (o *checksOwner) GetStatusConditions() *[]metav1.Condition { return &o.Status.Checks } @@ -1232,6 +1279,9 @@ func (o *checksOwner) DeepCopyObject() runtime.Object { out := *o o.ObjectMeta.DeepCopyInto(&out.ObjectMeta) //nolint:staticcheck // naming ObjectMeta keeps the copy readable out.Status.Checks = slices.Clone(o.Status.Checks) + if o.Status.Extra != nil { + out.Status.Extra = runtime.DeepCopyJSON(o.Status.Extra) + } return &out } @@ -1261,6 +1311,9 @@ func (o *nullableStatusOwner) DeepCopyObject() runtime.Object { if o.Status != nil { status := *o.Status status.Checks = slices.Clone(o.Status.Checks) + if o.Status.Extra != nil { + status.Extra = runtime.DeepCopyJSON(o.Status.Extra) + } out.Status = &status } return &out diff --git a/pkg/component/status_fields.go b/pkg/component/status_fields.go index af0a8ca6..b9f11280 100644 --- a/pkg/component/status_fields.go +++ b/pkg/component/status_fields.go @@ -1,7 +1,7 @@ package component import ( - "bytes" + "encoding" "encoding/json" "errors" "fmt" @@ -12,9 +12,13 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + kjson "sigs.k8s.io/json" ) -var jsonMarshaler = reflect.TypeFor[json.Marshaler]() +var ( + jsonMarshaler = reflect.TypeFor[json.Marshaler]() + textMarshaler = reflect.TypeFor[encoding.TextMarshaler]() +) // FlushOption configures a [FlushStatus] call. type FlushOption func(*flushOptions) @@ -92,7 +96,7 @@ func validateStatusFieldPath(owner OperatorCRD, segments []string) error { // A type such as metav1.Time encodes itself, often as a string, so its // Go fields say nothing about its JSON shape. The owner type itself is // exempt: only its status is addressed. - if i > 0 && reflect.PointerTo(current).Implements(jsonMarshaler) { + if i > 0 && encodesItself(current) { return fmt.Errorf("cannot address %q below a field of type %s, which encodes itself", segment, current) } // Below a map or an interface the schema is open, so any key is valid. @@ -120,6 +124,11 @@ func validateStatusFieldPath(owner OperatorCRD, segments []string) error { return nil } +func encodesItself(t reflect.Type) bool { + pointer := reflect.PointerTo(t) + return pointer.Implements(jsonMarshaler) || pointer.Implements(textMarshaler) +} + // jsonFields returns the type of every field with the JSON name name, declared // on t or on a struct that t embeds at any depth. Pass nil for visiting. func jsonFields(t reflect.Type, name string, visiting map[reflect.Type]bool) []reflect.Type { @@ -293,18 +302,16 @@ func toUnstructuredContent(obj OperatorCRD) (map[string]any, error) { return runtime.DeepCopyJSON(u.UnstructuredContent()), nil } - // encoding/json, not runtime.DefaultUnstructuredConverter: the converter - // panics on a struct with an unexported field when it converts back. - // json.Number keeps int64 values exact. + // JSON, not runtime.DefaultUnstructuredConverter: the converter panics on + // a struct with an unexported field when it converts back. The decoder is + // the API machinery's own, which keeps integers as exact int64 values. data, err := json.Marshal(obj) if err != nil { return nil, fmt.Errorf("encoding %T: %w", obj, err) } - decoder := json.NewDecoder(bytes.NewReader(data)) - decoder.UseNumber() var content map[string]any - if err := decoder.Decode(&content); err != nil { + if err := kjson.UnmarshalCaseSensitivePreserveInts(data, &content); err != nil { return nil, fmt.Errorf("decoding %T: %w", obj, err) } return content, nil @@ -325,7 +332,7 @@ func fromUnstructuredContent(content map[string]any, obj OperatorCRD) error { // Decoding into a non-empty object keeps the fields the content omits. reflect.ValueOf(obj).Elem().SetZero() - if err := json.Unmarshal(data, obj); err != nil { + if err := kjson.UnmarshalCaseSensitivePreserveInts(data, obj); err != nil { return fmt.Errorf("decoding into %T: %w", obj, err) } return nil From 5554abbee20a208064b15ad206d8f88c09d0a0b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 16 Oct 2026 15:04:54 +0100 Subject: [PATCH 06/13] fix(component): replace only the status on a retry with owned fields 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 --- pkg/component/conditions_test.go | 127 +++++++++++++++++++++++++++++-- pkg/component/status_fields.go | 93 +++++++++++++++------- 2 files changed, 187 insertions(+), 33 deletions(-) diff --git a/pkg/component/conditions_test.go b/pkg/component/conditions_test.go index d20c65d9..31cf4767 100644 --- a/pkg/component/conditions_test.go +++ b/pkg/component/conditions_test.go @@ -1103,8 +1103,49 @@ func TestFlushStatusOwnedStatusFields(t *testing.T) { assert.Equal(t, "Accepted", persisted.Status.Phase) }) + t.Run("replaces only the status on a retry, keeping the owner's private state", func(t *testing.T) { + // OperatorCRD does not forbid private state, such as a cached pointer + // behind GetStatusConditions. decodingClient decodes into the object as + // the REST client does, where the fake client would zero it. + adapterScheme := runtime.NewScheme() + adapterScheme.AddKnownTypes(GroupVersion, &adapterOwner{}) + initial := newAdapterOwner() + initial.ObjectMeta = metav1.ObjectMeta{Name: "test-owner", Namespace: "default"} + inner := fake.NewClientBuilder(). + WithScheme(adapterScheme).WithStatusSubresource(initial).WithObjects(initial).Build() + k8sClient := decodingClient{Client: inner} + + owner := newAdapterOwner() + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), owner)) + + other := newAdapterOwner() + require.NoError(t, inner.Get(ctx, client.ObjectKeyFromObject(initial), other)) + other.Status.Phase = "Running" + require.NoError(t, inner.Status().Update(ctx, other)) + + applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ + Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", + }) + + require.NoError(t, FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, []*Component{infraComponent(t)}, + WithOwnedStatusFields(), + )) + + assert.Equal(t, "AdapterOwner", owner.GetKind()) + assert.Same(t, &owner.Status.Checks, owner.GetStatusConditions()) + + persisted := newAdapterOwner() + require.NoError(t, inner.Get(ctx, client.ObjectKeyFromObject(initial), persisted)) + assert.Equal(t, "Running", persisted.Status.Phase) + found := meta.FindStatusCondition(persisted.Status.Checks, "InfraReady") + require.NotNil(t, found) + assert.Equal(t, "Healthy", found.Reason) + }) + t.Run("rejects a path that the status JSON does not contain", func(t *testing.T) { for name, path := range map[string]string{ + "unexported embedded scalar": "code", "below a type that encodes itself as text": "level.value", "unexported field": "internal", "below a type that encodes itself": "lastSync.Time", @@ -1244,14 +1285,17 @@ type checksOwner struct { } type checksStatus struct { - Checks []metav1.Condition `json:"checks,omitempty"` - Phase string `json:"phase,omitempty"` - LastSync metav1.Time `json:"lastSync,omitzero"` - Level textLevel `json:"level,omitzero"` - Extra map[string]any `json:"extra,omitempty"` - internal string //nolint:unused // read only by reflection + Checks []metav1.Condition `json:"checks,omitempty"` + Phase string `json:"phase,omitempty"` + LastSync metav1.Time `json:"lastSync,omitzero"` + Level textLevel `json:"level,omitzero"` + Extra map[string]any `json:"extra,omitempty"` + internal string //nolint:unused // read only by reflection + secretCode `json:"code"` //nolint:unused // read only by reflection } +type secretCode string //nolint:unused // embedded only to be found by reflection + // textLevel encodes as a JSON string through MarshalText, so its Value field // never appears in the JSON. type textLevel struct { @@ -1285,6 +1329,77 @@ func (o *checksOwner) DeepCopyObject() runtime.Object { return &out } +// adapterOwner keeps private state that JSON cannot restore: the kind and a +// cached pointer to its conditions. +type adapterOwner struct { + metav1.TypeMeta `json:",inline"` + metav1.ObjectMeta `json:"metadata,omitempty"` + + Status checksStatus `json:"status,omitempty"` + + kind string + conditions *[]metav1.Condition +} + +func newAdapterOwner() *adapterOwner { + o := &adapterOwner{kind: "AdapterOwner"} + o.conditions = &o.Status.Checks + return o +} + +func (o *adapterOwner) GetStatusConditions() *[]metav1.Condition { + return o.conditions +} + +func (o *adapterOwner) GetKind() string { + return o.kind +} + +func (o *adapterOwner) DeepCopyObject() runtime.Object { + out := *o + o.ObjectMeta.DeepCopyInto(&out.ObjectMeta) //nolint:staticcheck // naming ObjectMeta keeps the copy readable + out.Status.Checks = slices.Clone(o.Status.Checks) + if o.Status.Extra != nil { + out.Status.Extra = runtime.DeepCopyJSON(o.Status.Extra) + } + out.conditions = &out.Status.Checks + return &out +} + +// decodingClient reads and writes like the REST client: results decode into +// the object passed in, which keeps fields the JSON does not carry. +type decodingClient struct { + client.Client +} + +func (c decodingClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + return decodingReader{Reader: c.Client}.Get(ctx, key, obj, opts...) +} + +func (c decodingClient) Status() client.SubResourceWriter { + return decodingStatusWriter{SubResourceWriter: c.Client.Status()} +} + +type decodingStatusWriter struct { + client.SubResourceWriter +} + +func (w decodingStatusWriter) Update(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { + fresh, ok := obj.DeepCopyObject().(client.Object) + if !ok { + return fmt.Errorf("deep copy of %T is not a client.Object", obj) + } + if err := w.SubResourceWriter.Update(ctx, fresh, opts...); err != nil { + return err + } + + data, err := json.Marshal(fresh) + if err != nil { + return err + } + return json.Unmarshal(data, obj) +} + // nullableStatusOwner encodes a nil status as null, because the field has no // omitempty. type nullableStatusOwner struct { diff --git a/pkg/component/status_fields.go b/pkg/component/status_fields.go index b9f11280..8a526ead 100644 --- a/pkg/component/status_fields.go +++ b/pkg/component/status_fields.go @@ -90,13 +90,18 @@ func validateStatusFieldPath(owner OperatorCRD, segments []string) error { return nil } - current := reflect.TypeOf(owner) - for i, segment := range append([]string{"status"}, segments...) { + ownerType := indirectType(reflect.TypeOf(owner)) + index, ok := statusFieldIndex(ownerType) + if !ok { + return fmt.Errorf("type %s has no exported field with JSON name \"status\"", ownerType) + } + + current := ownerType.Field(index).Type + for _, segment := range segments { current = indirectType(current) // A type such as metav1.Time encodes itself, often as a string, so its - // Go fields say nothing about its JSON shape. The owner type itself is - // exempt: only its status is addressed. - if i > 0 && encodesItself(current) { + // Go fields say nothing about its JSON shape. + if encodesItself(current) { return fmt.Errorf("cannot address %q below a field of type %s, which encodes itself", segment, current) } // Below a map or an interface the schema is open, so any key is valid. @@ -155,12 +160,12 @@ func jsonFields(t reflect.Type, name string, visiting map[reflect.Type]bool) []r // encoding/json skips unexported fields, except that the exported // fields of an unexported embedded struct are promoted. - if !field.IsExported() && !field.Anonymous { + embedded := indirectType(field.Type) + if !field.IsExported() && (!field.Anonymous || embedded.Kind() != reflect.Struct) { continue } fieldName, _, _ := strings.Cut(tag, ",") - embedded := indirectType(field.Type) if fieldName == "" && field.Anonymous && embedded.Kind() == reflect.Struct { found = append(found, jsonFields(embedded, name, visiting)...) continue @@ -183,12 +188,7 @@ func indirectType(t reflect.Type) reflect.Type { } func zeroStatus(obj OperatorCRD) error { - content, err := toUnstructuredContent(obj) - if err != nil { - return err - } - delete(content, "status") - return fromUnstructuredContent(content, obj) + return setStatus(obj, nil) } // applyOwnedStatusFields sets on staged the non-condition status of server, @@ -238,8 +238,7 @@ func applyOwnedStatusFields(staged, server OperatorCRD, owned [][]string) error } } - stagedContent["status"] = merged - if err := fromUnstructuredContent(stagedContent, staged); err != nil { + if err := setStatus(staged, merged); err != nil { return err } @@ -302,9 +301,8 @@ func toUnstructuredContent(obj OperatorCRD) (map[string]any, error) { return runtime.DeepCopyJSON(u.UnstructuredContent()), nil } - // JSON, not runtime.DefaultUnstructuredConverter: the converter panics on - // a struct with an unexported field when it converts back. The decoder is - // the API machinery's own, which keeps integers as exact int64 values. + // JSON is what the API server receives. The decoder is the API + // machinery's own, which keeps integers as exact int64 values. data, err := json.Marshal(obj) if err != nil { return nil, fmt.Errorf("encoding %T: %w", obj, err) @@ -317,23 +315,64 @@ func toUnstructuredContent(obj OperatorCRD) (map[string]any, error) { return content, nil } -// fromUnstructuredContent replaces obj's content. Fields the content does not -// carry are zeroed, unexported fields included. -func fromUnstructuredContent(content map[string]any, obj OperatorCRD) error { +// setStatus replaces the status of obj with status, or clears it when status +// is nil. Nothing outside the status changes, so state that JSON cannot carry, +// such as a cached pointer behind GetStatusConditions, survives. +func setStatus(obj OperatorCRD, status map[string]any) error { if u, ok := obj.(runtime.Unstructured); ok { + content := runtime.DeepCopyJSON(u.UnstructuredContent()) + if status == nil { + delete(content, "status") + } else { + content["status"] = status + } u.SetUnstructuredContent(content) return nil } - data, err := json.Marshal(content) + owner := reflect.ValueOf(obj).Elem() + index, ok := statusFieldIndex(owner.Type()) + if !ok { + return fmt.Errorf("type %s has no exported field with JSON name \"status\"", owner.Type()) + } + + field := owner.Field(index) + if status == nil { + field.SetZero() + return nil + } + + data, err := json.Marshal(status) if err != nil { - return fmt.Errorf("encoding content for %T: %w", obj, err) + return fmt.Errorf("encoding status for %T: %w", obj, err) } - // Decoding into a non-empty object keeps the fields the content omits. - reflect.ValueOf(obj).Elem().SetZero() - if err := kjson.UnmarshalCaseSensitivePreserveInts(data, obj); err != nil { - return fmt.Errorf("decoding into %T: %w", obj, err) + fresh := reflect.New(field.Type()) + if err := kjson.UnmarshalCaseSensitivePreserveInts(data, fresh.Interface()); err != nil { + return fmt.Errorf("decoding status into %T: %w", obj, err) } + field.Set(fresh.Elem()) return nil } + +func statusFieldIndex(t reflect.Type) (int, bool) { + if t.Kind() != reflect.Struct { + return 0, false + } + + for i := range t.NumField() { + field := t.Field(i) + if !field.IsExported() { + continue + } + + name, _, _ := strings.Cut(field.Tag.Get("json"), ",") + if name == "" && !field.Anonymous { + name = field.Name + } + if name == "status" { + return i, true + } + } + return 0, false +} From 2a9d0bd351078f687a26b035e81e3d36167ff6d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:24:17 +0100 Subject: [PATCH 07/13] fix(component): write a pointer status in place on a retry 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 --- pkg/component/conditions_test.go | 82 ++++++++++++++++++++++++++++++++ pkg/component/status_fields.go | 21 ++++++-- 2 files changed, 99 insertions(+), 4 deletions(-) diff --git a/pkg/component/conditions_test.go b/pkg/component/conditions_test.go index 31cf4767..713b1d80 100644 --- a/pkg/component/conditions_test.go +++ b/pkg/component/conditions_test.go @@ -1143,6 +1143,50 @@ func TestFlushStatusOwnedStatusFields(t *testing.T) { assert.Equal(t, "Healthy", found.Reason) }) + t.Run("keeps a cached conditions pointer inside a pointer status valid", func(t *testing.T) { + adapterScheme := runtime.NewScheme() + adapterScheme.AddKnownTypes(GroupVersion, &pointerAdapterOwner{}) + initial := newPointerAdapterOwner() + initial.ObjectMeta = metav1.ObjectMeta{Name: "test-owner", Namespace: "default"} + inner := fake.NewClientBuilder(). + WithScheme(adapterScheme).WithStatusSubresource(initial).WithObjects(initial).Build() + k8sClient := decodingClient{Client: inner} + + owner := newPointerAdapterOwner() + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), owner)) + + other := newPointerAdapterOwner() + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), other)) + meta.SetStatusCondition(other.GetStatusConditions(), metav1.Condition{ + Type: "ExternalReady", Status: metav1.ConditionTrue, Reason: "WrittenByOtherController", + }) + other.Status.Phase = "Running" + require.NoError(t, k8sClient.Status().Update(ctx, other)) + + applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ + Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", + }) + + require.NoError(t, FlushStatus( + ctx, + ReconcileContext{Client: k8sClient, APIReader: decodingReader{Reader: inner}, Owner: owner}, + []*Component{infraComponent(t)}, + WithOwnedStatusFields(), + )) + + assert.Same(t, &owner.Status.Checks, owner.GetStatusConditions()) + + persisted := newPointerAdapterOwner() + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), persisted)) + assert.Equal(t, "Running", persisted.Status.Phase) + infra := meta.FindStatusCondition(persisted.Status.Checks, "InfraReady") + require.NotNil(t, infra, "an owned condition must keep the staged value") + assert.Equal(t, "Healthy", infra.Reason) + external := meta.FindStatusCondition(persisted.Status.Checks, "ExternalReady") + require.NotNil(t, external, "an unowned condition must follow the server") + assert.Equal(t, "WrittenByOtherController", external.Reason) + }) + t.Run("rejects a path that the status JSON does not contain", func(t *testing.T) { for name, path := range map[string]string{ "unexported embedded scalar": "code", @@ -1366,6 +1410,44 @@ func (o *adapterOwner) DeepCopyObject() runtime.Object { return &out } +// pointerAdapterOwner is adapterOwner with a pointer status: its cached +// pointer reaches into the status struct the Status field points to. +type pointerAdapterOwner struct { + metav1.TypeMeta `json:",inline"` + metav1.ObjectMeta `json:"metadata,omitempty"` + + Status *checksStatus `json:"status,omitempty"` + + conditions *[]metav1.Condition +} + +func newPointerAdapterOwner() *pointerAdapterOwner { + o := &pointerAdapterOwner{Status: &checksStatus{}} + o.conditions = &o.Status.Checks + return o +} + +func (o *pointerAdapterOwner) GetStatusConditions() *[]metav1.Condition { + return o.conditions +} + +func (o *pointerAdapterOwner) GetKind() string { + return "pointerAdapterOwner" +} + +func (o *pointerAdapterOwner) DeepCopyObject() runtime.Object { + out := *o + o.ObjectMeta.DeepCopyInto(&out.ObjectMeta) //nolint:staticcheck // naming ObjectMeta keeps the copy readable + status := *o.Status + status.Checks = slices.Clone(o.Status.Checks) + if o.Status.Extra != nil { + status.Extra = runtime.DeepCopyJSON(o.Status.Extra) + } + out.Status = &status + out.conditions = &out.Status.Checks + return &out +} + // decodingClient reads and writes like the REST client: results decode into // the object passed in, which keeps fields the JSON does not carry. type decodingClient struct { diff --git a/pkg/component/status_fields.go b/pkg/component/status_fields.go index 8a526ead..7997799e 100644 --- a/pkg/component/status_fields.go +++ b/pkg/component/status_fields.go @@ -336,9 +336,22 @@ func setStatus(obj OperatorCRD, status map[string]any) error { return fmt.Errorf("type %s has no exported field with JSON name \"status\"", owner.Type()) } - field := owner.Field(index) + // Write through a non-nil status pointer instead of replacing it: a cached + // pointer into the status struct must keep reaching the struct that gets + // encoded. + target := owner.Field(index) + for target.Kind() == reflect.Pointer { + if target.IsNil() { + if status == nil { + return nil + } + target.Set(reflect.New(target.Type().Elem())) + } + target = target.Elem() + } + if status == nil { - field.SetZero() + target.SetZero() return nil } @@ -347,11 +360,11 @@ func setStatus(obj OperatorCRD, status map[string]any) error { return fmt.Errorf("encoding status for %T: %w", obj, err) } - fresh := reflect.New(field.Type()) + fresh := reflect.New(target.Type()) if err := kjson.UnmarshalCaseSensitivePreserveInts(data, fresh.Interface()); err != nil { return fmt.Errorf("decoding status into %T: %w", obj, err) } - field.Set(fresh.Elem()) + target.Set(fresh.Elem()) return nil } From dd682ce869bbe0284aa5492e189726a64904cbeb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 16 Oct 2026 17:03:43 +0100 Subject: [PATCH 08/13] refactor(component): write owned status fields with a merge patch 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 --- docs/component.md | 77 +- docs/guidelines.md | 5 +- go.mod | 2 +- pkg/component/component.go | 147 ++-- pkg/component/conditions_test.go | 741 ------------------ pkg/component/flush_owned_fields_test.go | 336 ++++++++ pkg/component/status_fields.go | 391 --------- pkg/component/status_patch.go | 257 ++++++ pkg/component/zz_mock_crd_test.go | 14 - plugin/skills/building-components/SKILL.md | 26 +- .../references/component.md | 77 +- plugin/skills/structuring-operators/SKILL.md | 4 +- .../references/guidelines.md | 6 +- 13 files changed, 760 insertions(+), 1323 deletions(-) create mode 100644 pkg/component/flush_owned_fields_test.go delete mode 100644 pkg/component/status_fields.go create mode 100644 pkg/component/status_patch.go diff --git a/docs/component.md b/docs/component.md index 30732f01..bd8c9b21 100644 --- a/docs/component.md +++ b/docs/component.md @@ -815,29 +815,27 @@ func (r *WebAppReconciler) Reconcile(ctx context.Context, req reconcile.Request) status subresource**, not only the conditions. Every status field the controller staged in memory is sent, and every field it did not stage is sent exactly as it stands on the in-memory object. Fetch the owner fresh at the top of each reconcile, so the fields nobody staged still hold current server state. An owner carried over from an earlier pass -writes stale values back over newer ones. +writes stale values back over newer ones. With +[`WithOwnedStatusFields`](#sharing-the-owners-status-with-another-controller), the write is a merge patch of the +declared fields and the conditions instead. On a 409 Conflict, for example when another writer updated the owner between the controller's `Get` and this call, -`FlushStatus` keeps the staged owner as the object it writes. It fetches the server's copy into a **separate** object -and retries. The staged owner is never replaced. From the server's copy, the retry takes: - -- the `resourceVersion`, so the retry targets the live object; -- every condition whose type this flush does not own; -- with [`WithOwnedStatusFields`](#sharing-the-owners-status-with-another-controller), every non-condition status field - outside the declared paths. - -Everything else keeps the staged value. +`FlushStatus` keeps the staged owner as the object it writes. It fetches the server's copy into a **separate** object, +takes that copy's `resourceVersion` so the retry targets the live object, restores from it only the conditions whose +type this flush does not own, and retries. The staged owner is never replaced, and no other status field is taken from +the server. That fetch goes through `ReconcileContext.APIReader` when it is set, and through `Client` otherwise. Set it from the manager's `GetAPIReader()`. The manager's default `Client` serves reads from the informer cache, which can lag the very write that caused the conflict and hand back the same stale `resourceVersion`, so a retry through the cache may conflict again until the cache catches up. A direct read sees the server as it is. -Two things follow from that. A condition type this flush does not own keeps the server's value rather than the copy the -controller happens to be holding, so a concurrent update by another writer is not rolled back. And without -`WithOwnedStatusFields`, every non-condition status field keeps the staged value: a field the controller staged during -the reconcile survives a conflict, and so does the stale copy of a field that another controller owns. When another -controller writes non-condition fields of the same status, declare the fields this flush owns, as described in +Two things follow from that. A status field that the flush writes keeps its staged value across a conflict, because the +object holding it is the object that gets written. And a condition type this flush does not own keeps the server's value +rather than the copy the controller happens to be holding, so a concurrent update by another writer is not rolled back. +Without `WithOwnedStatusFields`, the flush writes every non-condition status field, so a retry also writes back the +stale copy of a field that another controller changed after the fetch. When another controller writes non-condition +fields of the same status, see [Sharing the owner's status with another controller](#sharing-the-owners-status-with-another-controller). Which types count as owned comes from the components passed to `FlushStatus`: @@ -940,34 +938,39 @@ flushErr := component.FlushStatus(ctx, recCtx, comps, component.WithOwnedStatusF flushErr := component.FlushStatus(ctx, recCtx, comps, component.WithOwnedStatusFields("observedGeneration")) ``` -The declaration changes only the conflict retry. The first write sends the whole staged status, as without the option. -When nothing changed on the server since the controller read the owner, that write succeeds, and the fields the -controller did not stage still hold the values it read. On a retry: +With the option, every attempt of `FlushStatus` is a JSON merge patch of the status instead of a `Status().Update`. The +patch holds only: + +- the staged owner's `metadata.resourceVersion`, as a precondition: the API server rejects the patch with a 409 when the + owner changed after the controller read it; +- each declared field with the staged owner's value, or `null` when the staged owner does not carry the field, so the + server removes it; +- the conditions list, with the owned conditions as staged and the unowned conditions as the server holds them. -- A declared field keeps the staged value. If the staged owner does not carry the field, the retry removes it. -- Every other non-condition status field takes the server's value. If the server does not carry the field, the retry - removes it. This is the same rule as for an unowned condition. -- Conditions follow component ownership, as described above. +A field that the controller did not declare is never sent, on the first attempt or on a retry, so it cannot overwrite +another writer's value. On a 409, `FlushStatus` refreshes the `resourceVersion` and the unowned conditions from the +server, as described above, builds the patch again, and retries. After a successful write, the controller-runtime client +decodes the server's response into the owner, so a value staged in a field that you did not declare does not survive in +memory. Name each field by its JSON name under `status`, with dots between nested fields: `"observedGeneration"`, -`"summary.ready"`. Do not include the `status.` prefix: `"status.phase"` is an error unless status has a field named -`status`. `WithOwnedStatusFields()` with no paths declares that the flush owns no non-condition field, which fits a -controller that writes only conditions. Paths from several options are combined. +`"summary.ready"`. Do not include the `status.` prefix. `WithOwnedStatusFields()` with no paths declares that the flush +owns no non-condition field, which fits a controller that writes only conditions. Paths from several options are +combined. Declare every non-condition field that the controller writes, including `observedGeneration` when it is this +controller's field: a field that you stage but do not declare is not written. -`FlushStatus` validates each path before it writes and returns an error, with no write, when a path: +`FlushStatus` returns an error, with no write, when a path: - is empty or has an empty segment, such as `"summary..ready"`; -- names `conditions` or a field below it, because conditions are owned through components; -- does not resolve to a field that the owner's Go type encodes in its JSON, such as a misspelled name, an unexported - field, a path below a scalar field, or a path below a type that encodes itself, such as `metav1.Time`. Below a map - field, any key is accepted; -- is ambiguous: a segment matches more than one field, because a struct declares the JSON name and also gets it from an - embedded struct, or gets it from two embedded structs. - -Without the option, `FlushStatus` keeps the staged value of every non-condition status field on a retry. That default -keeps a controller's own `observedGeneration` write safe when it is the only writer of the status. Once you declare -fields, a staged field that you did not declare follows the server on a retry. So declare every field that the -controller writes, including `observedGeneration` when it is this controller's field. +- names the conditions, contains them, or lies below them, because conditions are owned through components. + `FlushStatus` finds the JSON path of the list that `GetStatusConditions` returns, so this holds whatever that field is + called; +- runs below a field that the staged owner holds as a value that is not an object, such as `"phase.value"` when `phase` + is a string. + +The framework does not check a path against the CRD schema. A misspelled path, such as `"phsae"`, sends a field that the +schema does not declare. With a structural schema the API server prunes that field, so that part of the write does +nothing and `FlushStatus` returns no error. Check each path against the CRD type when you add it. ### Where the owner's observedGeneration is set diff --git a/docs/guidelines.md b/docs/guidelines.md index b42aa23d..b0c28a6d 100644 --- a/docs/guidelines.md +++ b/docs/guidelines.md @@ -192,8 +192,9 @@ and skips metric emission. When two controllers write the status of one CR, give each status field one owner, as with conditions. Each controller passes the components that own its conditions and declares its non-condition fields with -[`WithOwnedStatusFields`](component.md#sharing-the-owners-status-with-another-controller). Without the declaration, a -conflict retry writes the controller's stale copy of the other controller's fields back over newer values. +[`WithOwnedStatusFields`](component.md#sharing-the-owners-status-with-another-controller), so its flush patches only +those fields and the conditions. Without the declaration, the flush writes the whole status, and a conflict retry writes +the controller's stale copy of the other controller's fields back over newer values. Building the component set from a pure resolver `(spec, version) -> []*component.Component` keeps the loop stable: enabling an optional feature changes which components the resolver returns without touching the reconcile loop. diff --git a/go.mod b/go.mod index 09b5effe..60899b52 100644 --- a/go.mod +++ b/go.mod @@ -18,7 +18,6 @@ require ( k8s.io/client-go v0.37.1 k8s.io/utils v0.0.0-20260707023825-cf1189d6abe3 sigs.k8s.io/controller-runtime v0.25.2 - sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 sigs.k8s.io/yaml v1.6.0 ) @@ -85,6 +84,7 @@ require ( gopkg.in/yaml.v3 v3.0.1 // indirect k8s.io/klog/v2 v2.140.0 // indirect k8s.io/kube-openapi v0.0.0-20260721132016-d427ff9ee9ad // indirect + sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect sigs.k8s.io/randfill v1.0.0 // indirect sigs.k8s.io/structured-merge-diff/v6 v6.4.2 // indirect ) diff --git a/pkg/component/component.go b/pkg/component/component.go index 5e11cee7..4cf8e90e 100644 --- a/pkg/component/component.go +++ b/pkg/component/component.go @@ -655,11 +655,21 @@ func fail(rec ReconcileContext, conditionType ConditionType, err error) error { // validation-only CRD reports, it is unowned. On a conflict an unowned condition // follows the server, and the next reconcile stages it again. // -// FlushStatus issues a single Status().Update, which writes the entire status -// subresource, not only the conditions. Fields the controller never staged are -// sent exactly as they stand on the in-memory owner. The owner must therefore be -// fetched fresh at the top of each reconcile: an owner carried over from an -// earlier pass writes stale status values back over newer ones. +// By default FlushStatus issues a single Status().Update, which writes the +// entire status subresource, not only the conditions. Fields the controller +// never staged are sent exactly as they stand on the in-memory owner. The owner +// must therefore be fetched fresh at the top of each reconcile: an owner +// carried over from an earlier pass writes stale status values back over newer +// ones. +// +// With [WithOwnedStatusFields], every attempt is instead a JSON merge patch of +// the status that holds only the declared fields and the conditions, with the +// staged resourceVersion as a precondition when the owner has one. A field the +// controller did not declare is never sent, so it cannot overwrite another +// writer's value. Use it when another controller writes non-condition fields of +// the same status. After a successful write rec.Client decodes the server's +// response into rec.Owner; with the controller-runtime client a staged value of +// an undeclared field does not survive in memory. // // Controllers must call FlushStatus exactly once per reconciliation, typically // via defer so that conditions set on error paths are still persisted. Declare @@ -691,25 +701,24 @@ func fail(rec ReconcileContext, conditionType ConditionType, err error) error { // On a 409 Conflict (for example if an external writer updated the owner between // the controller fetching it and this call) FlushStatus keeps the staged owner as // the object being written. It fetches the server's copy into a separate object -// through rec.APIReader when set (rec.Client otherwise) and retries. From the -// server's copy a retry takes: -// -// - the resourceVersion, so the retry targets the live object; -// - every condition whose type this flush does not own; -// - when [WithOwnedStatusFields] is passed, every non-condition status field -// outside the declared paths. -// -// Everything else keeps the staged value. Without [WithOwnedStatusFields] that -// includes every non-condition status field, so a field the controller staged -// survives a conflict, and so does a stale field another controller owns. On an -// owner whose status is shared with another writer, declare the fields this -// flush owns, or the retry rolls back that writer's newer values. +// through rec.APIReader when set (rec.Client otherwise), takes that object's +// resourceVersion so the retry targets the live object, and restores from it +// only the conditions whose type this flush does not own. It then retries. No +// other status field is taken from the server. Without [WithOwnedStatusFields] +// the retry therefore writes every staged non-condition field again, including +// a stale copy of a field that another writer has changed since the fetch. // // Set rec.APIReader from the manager's GetAPIReader. The manager's default // Client serves reads from the informer cache, which can lag the very write // that caused the conflict and return the same stale resourceVersion, so a // retry through it may conflict again until the cache catches up. // +// Two consequences follow from never replacing the staged owner. Non-condition +// status fields staged during the reconcile survive a conflict. And a condition +// type this flush does not own keeps the server's value rather than the possibly +// stale copy the controller happens to be holding, so a concurrent update by +// another writer is not rolled back. +// // For unowned types the server is the source of truth, absence included. An // unowned condition the server no longer carries is dropped from the staged // owner rather than written back, so a condition another writer removed is not @@ -718,41 +727,35 @@ func fail(rec ReconcileContext, conditionType ConditionType, err error) error { // the next reconcile. // // rec.Client and rec.Owner must be populated, and every component in comps must -// be non-nil; a nil component is a programming error. FlushStatus returns an -// error without writing when a path given to [WithOwnedStatusFields] is -// invalid. If rec.Metrics is nil, metric recording is skipped. +// be non-nil; a nil component is a programming error. With +// [WithOwnedStatusFields], FlushStatus returns an error without writing for an +// invalid path. If rec.Metrics is nil, metric recording is skipped. func FlushStatus(ctx context.Context, rec ReconcileContext, comps []*Component, opts ...FlushOption) error { var options flushOptions for _, opt := range opts { opt(&options) } - ownedFields, err := parseOwnedStatusFields(rec.Owner, options.ownedFields) - if err != nil { - return fmt.Errorf( - "flushing status of %s %s: %w", rec.Owner.GetKind(), client.ObjectKeyFromObject(rec.Owner), err, - ) - } - - refresh := conflictRefresh{ - ownedConditions: ownedConditionTypes(comps), - fieldsDeclared: options.fieldsDeclared, - ownedFieldPaths: ownedFields, - } + owned := ownedConditionTypes(comps) - err = retry.RetryOnConflict(retry.DefaultRetry, func() error { - updateErr := rec.Client.Status().Update(ctx, rec.Owner) - if updateErr == nil { - return nil - } - if !apierrors.IsConflict(updateErr) { + var err error + if options.fieldsDeclared { + err = patchOwnedStatus(ctx, rec, owned, options.ownedFields) + } else { + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + updateErr := rec.Client.Status().Update(ctx, rec.Owner) + if updateErr == nil { + return nil + } + if !apierrors.IsConflict(updateErr) { + return updateErr + } + if refreshErr := refreshUnownedConditions(ctx, rec, owned); refreshErr != nil { + return refreshErr + } return updateErr - } - if refreshErr := refresh.apply(ctx, rec); refreshErr != nil { - return refreshErr - } - return updateErr - }) + }) + } if err != nil { return err } @@ -780,22 +783,16 @@ func ownedConditionTypes(comps []*Component) map[string]struct{} { return owned } -// conflictRefresh prepares the staged owner for a retry after a conflict, -// without ever replacing it. -// -// The staged owner remains the object that will be written. From the server it -// takes the current resourceVersion, so the retry targets the live object, and -// the value of every condition type this flush does not own, so a concurrent -// update by another writer is not overwritten by the copy this controller -// happens to be holding. When the controller declared its status fields, it -// also takes every non-condition status field outside those paths. -type conflictRefresh struct { - ownedConditions map[string]struct{} - fieldsDeclared bool - ownedFieldPaths [][]string -} - -func (r conflictRefresh) apply(ctx context.Context, rec ReconcileContext) error { +// refreshUnownedConditions prepares the staged owner for a retry after a +// conflict, without ever replacing it. +// +// The staged owner remains the object that will be written, so every field the +// controller staged during this reconcile survives, conditions and non-condition +// status fields alike. Only two things are taken from the server: the current +// resourceVersion, so the retry targets the live object, and the value of every +// condition type this flush does not own, so a concurrent update by another +// writer is not overwritten by the copy this controller happens to be holding. +func refreshUnownedConditions(ctx context.Context, rec ReconcileContext, owned map[string]struct{}) error { server, ok := rec.Owner.DeepCopyObject().(OperatorCRD) if !ok { return fmt.Errorf("failed to deep copy owner of type %T", rec.Owner) @@ -803,15 +800,8 @@ func (r conflictRefresh) apply(ctx context.Context, rec ReconcileContext) error // Clear the copy's conditions before the fetch. Decoding a response into a // non-empty struct does not remove fields the response omits, so an owner // with no conditions on the server would otherwise leave the staged - // conditions in place here and they would read as the server's. With - // declared fields every status field is read from the server, so the whole - // status is cleared for the same reason. + // conditions in place here and they would read as the server's. *server.GetStatusConditions() = nil - if r.fieldsDeclared { - if err := zeroStatus(server); err != nil { - return err - } - } // Read through APIReader when one is set: the manager's Client serves reads // from the informer cache, which can lag the write that caused the conflict @@ -824,20 +814,8 @@ func (r conflictRefresh) apply(ctx context.Context, rec ReconcileContext) error return err } - if r.fieldsDeclared { - if err := applyOwnedStatusFields(rec.Owner, server, r.ownedFieldPaths); err != nil { - return err - } - } - - refreshUnownedConditions(rec.Owner, server, r.ownedConditions) rec.Owner.SetResourceVersion(server.GetResourceVersion()) - return nil -} -// refreshUnownedConditions sets on staged the server's value of every condition -// type this flush does not own. -func refreshUnownedConditions(staged, server OperatorCRD, owned map[string]struct{}) { serverConds := *server.GetStatusConditions() onServer := make(map[string]struct{}, len(serverConds)) for _, cond := range serverConds { @@ -847,8 +825,8 @@ func refreshUnownedConditions(staged, server OperatorCRD, owned map[string]struc // Absence is server state too. An unowned condition the server no longer // carries has been removed by whoever owns it, so drop the local copy // instead of writing it back and resurrecting it. - conditions := staged.GetStatusConditions() - *conditions = slices.DeleteFunc(*conditions, func(cond metav1.Condition) bool { + staged := rec.Owner.GetStatusConditions() + *staged = slices.DeleteFunc(*staged, func(cond metav1.Condition) bool { if _, isOwned := owned[cond.Type]; isOwned { return false } @@ -860,8 +838,9 @@ func refreshUnownedConditions(staged, server OperatorCRD, owned map[string]struc if _, isOwned := owned[cond.Type]; isOwned { continue } - replaceStatusCondition(conditions, cond) + replaceStatusCondition(staged, cond) } + return nil } // replaceStatusCondition sets cond by type, taking its value verbatim. diff --git a/pkg/component/conditions_test.go b/pkg/component/conditions_test.go index 713b1d80..9801491e 100644 --- a/pkg/component/conditions_test.go +++ b/pkg/component/conditions_test.go @@ -2,11 +2,8 @@ package component import ( "context" - "encoding/json" "errors" "fmt" - "reflect" - "slices" "testing" "time" @@ -799,741 +796,3 @@ func TestFlushStatusConflictOwnership(t *testing.T) { assert.Zero(t, counting.gets, "the non-conflict path must not fetch the owner") }) } - -// TestFlushStatusOwnedStatusFields covers a status shared between controllers. -// Each case reads the owner, lets a second writer update the status, and then -// flushes, so the first update is a real 409 from a stale resourceVersion. -func TestFlushStatusOwnedStatusFields(t *testing.T) { - ctx := context.Background() - scheme := runtime.NewScheme() - require.NoError(t, AddToScheme(scheme)) - - infraComponent := func(t *testing.T) *Component { - t.Helper() - comp, err := NewComponentBuilder().WithName("infra").WithConditionType("InfraReady").Build() - require.NoError(t, err) - return comp - } - - // readThenRace creates the owner on a fake server, reads it as the - // controller would at the top of a reconcile, and then applies the second - // writer's status update on the server. The returned owner is stale. - readThenRace := func( - t *testing.T, initial *MockOperatorCRD, secondWriter func(*MockOperatorCRD), - ) (client.Client, *MockOperatorCRD) { - t.Helper() - initial.ObjectMeta = metav1.ObjectMeta{Name: "test-owner", Namespace: "default", Generation: 2} - k8sClient := fake.NewClientBuilder(). - WithScheme(scheme).WithStatusSubresource(initial).WithObjects(initial).Build() - - owner := &MockOperatorCRD{} - require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), owner)) - - other := &MockOperatorCRD{} - require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), other)) - secondWriter(other) - require.NoError(t, k8sClient.Status().Update(ctx, other)) - return k8sClient, owner - } - - persistedOf := func(t *testing.T, c client.Client, owner *MockOperatorCRD) *MockOperatorCRD { - t.Helper() - persisted := &MockOperatorCRD{} - require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(owner), persisted)) - return persisted - } - - t.Run("keeps a second writer's non-condition field across the conflict retry", func(t *testing.T) { - k8sClient, owner := readThenRace(t, &MockOperatorCRD{Status: MockStatus{ObservedGeneration: 1}}, - func(o *MockOperatorCRD) { o.Status.ObservedGeneration = 2 }) - - // This controller owns phase and InfraReady; observedGeneration belongs - // to the second writer and is still 1 on the stale owner. - owner.Status.Phase = "Accepted" - applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ - Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", - }) - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, []*Component{infraComponent(t)}, - WithOwnedStatusFields("phase"), - )) - - persisted := persistedOf(t, k8sClient, owner) - assert.Equal(t, int64(2), persisted.Status.ObservedGeneration, - "the retry must not roll back a field this flush does not own") - assert.Equal(t, "Accepted", persisted.Status.Phase, "an owned field keeps the staged value") - assert.NotNil(t, meta.FindStatusCondition(persisted.Status.Conditions, "InfraReady"), - "an owned condition keeps the staged value") - assert.Equal(t, int64(2), owner.Status.ObservedGeneration, - "the in-memory owner reflects what was written") - }) - - t.Run("owns no non-condition field when declared with no paths", func(t *testing.T) { - k8sClient, owner := readThenRace(t, &MockOperatorCRD{Status: MockStatus{Phase: "Pending"}}, - func(o *MockOperatorCRD) { - o.Status.ObservedGeneration = 2 - o.Status.Phase = "Running" - }) - - owner.Status.ObservedGeneration = 99 // a stray write this flush does not own - applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ - Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", - }) - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, []*Component{infraComponent(t)}, - WithOwnedStatusFields(), - )) - - persisted := persistedOf(t, k8sClient, owner) - assert.Equal(t, int64(2), persisted.Status.ObservedGeneration) - assert.Equal(t, "Running", persisted.Status.Phase) - assert.Equal(t, "Healthy", meta.FindStatusCondition(persisted.Status.Conditions, "InfraReady").Reason) - }) - - t.Run("keeps every staged field when no fields are declared", func(t *testing.T) { - // Without the option the controller's own staged fields win, so an - // existing caller that stages observedGeneration does not lose it. - k8sClient, owner := readThenRace(t, &MockOperatorCRD{}, - func(o *MockOperatorCRD) { o.Status.Phase = "Running" }) - - owner.Status.ObservedGeneration = 2 - - require.NoError(t, FlushStatus(ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil)) - - persisted := persistedOf(t, k8sClient, owner) - assert.Equal(t, int64(2), persisted.Status.ObservedGeneration) - assert.Empty(t, persisted.Status.Phase, "with nothing declared the staged status is written as a whole") - }) - - t.Run("removes an owned field the staged owner cleared", func(t *testing.T) { - k8sClient, owner := readThenRace(t, &MockOperatorCRD{Status: MockStatus{Phase: "Pending"}}, - func(o *MockOperatorCRD) { o.Status.ObservedGeneration = 2 }) - - owner.Status.Phase = "" - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("phase"), - )) - - persisted := persistedOf(t, k8sClient, owner) - assert.Empty(t, persisted.Status.Phase, "the staged owner is the source of truth for an owned field") - assert.Equal(t, int64(2), persisted.Status.ObservedGeneration) - }) - - t.Run("drops an unowned field the server no longer carries", func(t *testing.T) { - k8sClient, owner := readThenRace(t, &MockOperatorCRD{Status: MockStatus{ObservedGeneration: 1}}, - func(o *MockOperatorCRD) { o.Status.ObservedGeneration = 0 }) - - owner.Status.Phase = "Accepted" - - // The REST client decodes a response into the object it is given, so a - // field the response omits keeps whatever that object held. The fake - // client replaces the object instead and would hide a stale value. - require.NoError(t, FlushStatus( - ctx, - ReconcileContext{Client: k8sClient, APIReader: decodingReader{Reader: k8sClient}, Owner: owner}, - nil, - WithOwnedStatusFields("phase"), - )) - - persisted := persistedOf(t, k8sClient, owner) - assert.Zero(t, persisted.Status.ObservedGeneration, - "the server is the source of truth for an unowned field, absence included") - assert.Equal(t, "Accepted", persisted.Status.Phase) - }) - - t.Run("owns a nested field by its dotted path", func(t *testing.T) { - k8sClient, owner := readThenRace(t, - &MockOperatorCRD{Status: MockStatus{Summary: &MockStatusSummary{Ready: 1, Total: 3}}}, - func(o *MockOperatorCRD) { o.Status.Summary.Total = 4 }) - - owner.Status.Summary.Ready = 2 - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("summary.ready"), - )) - - persisted := persistedOf(t, k8sClient, owner) - require.NotNil(t, persisted.Status.Summary) - assert.Equal(t, MockStatusSummary{Ready: 2, Total: 4}, *persisted.Status.Summary) - }) - - t.Run("sets an owned nested field below a parent the server holds as null", func(t *testing.T) { - k8sClient, owner := readThenRace(t, &MockOperatorCRD{}, - func(o *MockOperatorCRD) { o.Status.ObservedGeneration = 2 }) - - owner.Status.Details = &MockStatusSummary{Ready: 1} - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("details.ready"), - )) - - persisted := persistedOf(t, k8sClient, owner) - require.NotNil(t, persisted.Status.Details) - assert.Equal(t, int32(1), persisted.Status.Details.Ready) - assert.Equal(t, int64(2), persisted.Status.ObservedGeneration) - }) - - t.Run("removes an owned nested field below a parent the staged owner holds as null", func(t *testing.T) { - k8sClient, owner := readThenRace(t, - &MockOperatorCRD{Status: MockStatus{Details: &MockStatusSummary{Ready: 1}}}, - func(o *MockOperatorCRD) { o.Status.ObservedGeneration = 2 }) - - owner.Status.Details = nil - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("details.ready"), - )) - - persisted := persistedOf(t, k8sClient, owner) - if persisted.Status.Details != nil { - assert.Zero(t, persisted.Status.Details.Ready) - } - assert.Equal(t, int64(2), persisted.Status.ObservedGeneration) - }) - - t.Run("combines the paths of several options", func(t *testing.T) { - k8sClient, owner := readThenRace(t, &MockOperatorCRD{}, - func(o *MockOperatorCRD) { o.Status.Summary = &MockStatusSummary{Total: 4} }) - - owner.Status.Phase = "Accepted" - owner.Status.ObservedGeneration = 2 - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, - WithOwnedStatusFields("phase"), WithOwnedStatusFields("observedGeneration"), - )) - - persisted := persistedOf(t, k8sClient, owner) - assert.Equal(t, "Accepted", persisted.Status.Phase) - assert.Equal(t, int64(2), persisted.Status.ObservedGeneration) - require.NotNil(t, persisted.Status.Summary) - assert.Equal(t, int32(4), persisted.Status.Summary.Total) - }) - - t.Run("keeps owned conditions stored under a JSON name other than conditions", func(t *testing.T) { - // OperatorCRD reaches the conditions through GetStatusConditions, so - // their JSON name is the owner type's choice. - checksScheme := runtime.NewScheme() - checksScheme.AddKnownTypes(GroupVersion, &checksOwner{}) - initial := &checksOwner{ObjectMeta: metav1.ObjectMeta{Name: "test-owner", Namespace: "default"}} - k8sClient := fake.NewClientBuilder(). - WithScheme(checksScheme).WithStatusSubresource(initial).WithObjects(initial).Build() - - owner := &checksOwner{} - require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), owner)) - - other := &checksOwner{} - require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), other)) - other.Status.Phase = "Running" - require.NoError(t, k8sClient.Status().Update(ctx, other)) - - applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ - Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", - }) - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, []*Component{infraComponent(t)}, - WithOwnedStatusFields(), - )) - - persisted := &checksOwner{} - require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), persisted)) - found := meta.FindStatusCondition(persisted.Status.Checks, "InfraReady") - require.NotNil(t, found, "an owned condition must keep the staged value") - assert.Equal(t, "Healthy", found.Reason) - assert.Equal(t, "Running", persisted.Status.Phase) - }) - - t.Run("sets an owned field when the server holds the whole status as null", func(t *testing.T) { - nullScheme := runtime.NewScheme() - nullScheme.AddKnownTypes(GroupVersion, &nullableStatusOwner{}) - initial := &nullableStatusOwner{ObjectMeta: metav1.ObjectMeta{Name: "test-owner", Namespace: "default"}} - k8sClient := fake.NewClientBuilder(). - WithScheme(nullScheme).WithStatusSubresource(initial).WithObjects(initial).Build() - - owner := &nullableStatusOwner{} - require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), owner)) - - other := &nullableStatusOwner{} - require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), other)) - other.Labels = map[string]string{"touched": "true"} - require.NoError(t, k8sClient.Update(ctx, other)) - - owner.Status = &checksStatus{Phase: "Accepted"} - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("phase"), - )) - - persisted := &nullableStatusOwner{} - require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), persisted)) - require.NotNil(t, persisted.Status) - assert.Equal(t, "Accepted", persisted.Status.Phase) - }) - - t.Run("keeps a large integer in a dynamic status field exact across the retry", func(t *testing.T) { - const big = int64(1<<53 + 1) // the first integer a float64 cannot hold - - checksScheme := runtime.NewScheme() - checksScheme.AddKnownTypes(GroupVersion, &checksOwner{}) - initial := &checksOwner{ObjectMeta: metav1.ObjectMeta{Name: "test-owner", Namespace: "default"}} - k8sClient := fake.NewClientBuilder(). - WithScheme(checksScheme).WithStatusSubresource(initial).WithObjects(initial).Build() - - owner := &checksOwner{} - require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), owner)) - - other := &checksOwner{} - require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), other)) - other.Status.Extra = map[string]any{"big": big} - require.NoError(t, k8sClient.Status().Update(ctx, other)) - - owner.Status.Phase = "Accepted" - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("phase"), - )) - - persisted := &checksOwner{} - require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), persisted)) - assert.Equal(t, big, persisted.Status.Extra["big"]) - assert.Equal(t, "Accepted", persisted.Status.Phase) - }) - - t.Run("replaces only the status on a retry, keeping the owner's private state", func(t *testing.T) { - // OperatorCRD does not forbid private state, such as a cached pointer - // behind GetStatusConditions. decodingClient decodes into the object as - // the REST client does, where the fake client would zero it. - adapterScheme := runtime.NewScheme() - adapterScheme.AddKnownTypes(GroupVersion, &adapterOwner{}) - initial := newAdapterOwner() - initial.ObjectMeta = metav1.ObjectMeta{Name: "test-owner", Namespace: "default"} - inner := fake.NewClientBuilder(). - WithScheme(adapterScheme).WithStatusSubresource(initial).WithObjects(initial).Build() - k8sClient := decodingClient{Client: inner} - - owner := newAdapterOwner() - require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), owner)) - - other := newAdapterOwner() - require.NoError(t, inner.Get(ctx, client.ObjectKeyFromObject(initial), other)) - other.Status.Phase = "Running" - require.NoError(t, inner.Status().Update(ctx, other)) - - applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ - Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", - }) - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, []*Component{infraComponent(t)}, - WithOwnedStatusFields(), - )) - - assert.Equal(t, "AdapterOwner", owner.GetKind()) - assert.Same(t, &owner.Status.Checks, owner.GetStatusConditions()) - - persisted := newAdapterOwner() - require.NoError(t, inner.Get(ctx, client.ObjectKeyFromObject(initial), persisted)) - assert.Equal(t, "Running", persisted.Status.Phase) - found := meta.FindStatusCondition(persisted.Status.Checks, "InfraReady") - require.NotNil(t, found) - assert.Equal(t, "Healthy", found.Reason) - }) - - t.Run("keeps a cached conditions pointer inside a pointer status valid", func(t *testing.T) { - adapterScheme := runtime.NewScheme() - adapterScheme.AddKnownTypes(GroupVersion, &pointerAdapterOwner{}) - initial := newPointerAdapterOwner() - initial.ObjectMeta = metav1.ObjectMeta{Name: "test-owner", Namespace: "default"} - inner := fake.NewClientBuilder(). - WithScheme(adapterScheme).WithStatusSubresource(initial).WithObjects(initial).Build() - k8sClient := decodingClient{Client: inner} - - owner := newPointerAdapterOwner() - require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), owner)) - - other := newPointerAdapterOwner() - require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), other)) - meta.SetStatusCondition(other.GetStatusConditions(), metav1.Condition{ - Type: "ExternalReady", Status: metav1.ConditionTrue, Reason: "WrittenByOtherController", - }) - other.Status.Phase = "Running" - require.NoError(t, k8sClient.Status().Update(ctx, other)) - - applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ - Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", - }) - - require.NoError(t, FlushStatus( - ctx, - ReconcileContext{Client: k8sClient, APIReader: decodingReader{Reader: inner}, Owner: owner}, - []*Component{infraComponent(t)}, - WithOwnedStatusFields(), - )) - - assert.Same(t, &owner.Status.Checks, owner.GetStatusConditions()) - - persisted := newPointerAdapterOwner() - require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(initial), persisted)) - assert.Equal(t, "Running", persisted.Status.Phase) - infra := meta.FindStatusCondition(persisted.Status.Checks, "InfraReady") - require.NotNil(t, infra, "an owned condition must keep the staged value") - assert.Equal(t, "Healthy", infra.Reason) - external := meta.FindStatusCondition(persisted.Status.Checks, "ExternalReady") - require.NotNil(t, external, "an unowned condition must follow the server") - assert.Equal(t, "WrittenByOtherController", external.Reason) - }) - - t.Run("rejects a path that the status JSON does not contain", func(t *testing.T) { - for name, path := range map[string]string{ - "unexported embedded scalar": "code", - "below a type that encodes itself as text": "level.value", - "unexported field": "internal", - "below a type that encodes itself": "lastSync.Time", - "unexported field of such a type": "lastSync.wall", - } { - t.Run(name, func(t *testing.T) { - owner := &checksOwner{ObjectMeta: metav1.ObjectMeta{Name: "test-owner", Namespace: "default"}} - k8sClient := fake.NewClientBuilder().WithScheme(scheme).Build() - - err := FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields(path), - ) - - require.Error(t, err) - assert.Contains(t, err.Error(), fmt.Sprintf("%q", path)) - }) - } - }) - - t.Run("rejects an invalid path before writing", func(t *testing.T) { - for name, path := range map[string]string{ - "empty": "", - "empty segment": "summary..ready", - "conditions": "conditions", - "inside conditions": "conditions.type", - "unknown field": "observedGenration", - "prefixed with status": "status.phase", - "below a scalar": "phase.value", - "unknown nested field": "summary.unknown", - "trailing separator": "phase.", - "leading separator": ".phase", - } { - t.Run(name, func(t *testing.T) { - owner := &MockOperatorCRD{ - ObjectMeta: metav1.ObjectMeta{Name: "test-owner", Namespace: "default"}, - } - k8sClient := fake.NewClientBuilder(). - WithScheme(scheme).WithStatusSubresource(owner).WithObjects(owner).Build() - owner.Status.Phase = "Accepted" - - err := FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields(path), - ) - - require.Error(t, err) - assert.Contains(t, err.Error(), fmt.Sprintf("%q", path)) - assert.Empty(t, persistedOf(t, k8sClient, owner).Status.Phase, "nothing is written") - }) - } - }) -} - -// TestJSONFields covers how path validation finds a field through embedded -// structs. More than one candidate makes the path ambiguous, which validation -// rejects rather than guessing which field encoding/json encodes. -func TestJSONFields(t *testing.T) { - type base struct { - Phase string `json:"phase"` - } - type otherBase struct { - Phase string `json:"phase"` - } - type phaseObject struct { - Ready bool `json:"ready"` - } - - t.Run("finds a field of an embedded struct", func(t *testing.T) { - type status struct { - base - } - - assert.Equal(t, []reflect.Type{reflect.TypeOf("")}, jsonFields(reflect.TypeOf(status{}), "phase", nil)) - }) - - t.Run("finds a field of a struct embedded by pointer", func(t *testing.T) { - type status struct { - *base - } - - assert.Equal(t, []reflect.Type{reflect.TypeOf("")}, jsonFields(reflect.TypeOf(status{}), "phase", nil)) - }) - - // go vet rejects a struct literal that repeats a JSON name, so the - // ambiguous types are built at run time. - baseType := reflect.TypeOf(base{}) - embed := func(name string, typ reflect.Type) reflect.StructField { - return reflect.StructField{Name: name, Type: typ, Anonymous: true} - } - - t.Run("finds both a declared field and the embedded field it shadows", func(t *testing.T) { - status := reflect.StructOf([]reflect.StructField{ - embed("Base", baseType), - {Name: "Phase", Type: reflect.TypeOf(phaseObject{}), Tag: `json:"phase"`}, - }) - - assert.Len(t, jsonFields(status, "phase", nil), 2) - }) - - t.Run("finds the field of each embedded struct that provides the name", func(t *testing.T) { - status := reflect.StructOf([]reflect.StructField{ - embed("Base", baseType), - embed("OtherBase", reflect.TypeOf(otherBase{})), - }) - - assert.Len(t, jsonFields(status, "phase", nil), 2) - }) - - t.Run("does not recurse forever through a struct that embeds itself", func(t *testing.T) { - found := jsonFields(reflect.TypeOf(selfEmbedding{}), "phase", nil) - - assert.Equal(t, []reflect.Type{reflect.TypeOf("")}, found) - }) - - t.Run("skips a field tagged to be ignored", func(t *testing.T) { - type status struct { - Phase string `json:"-"` - Hidden string `json:"phase"` - } - - assert.Equal(t, []reflect.Type{reflect.TypeOf("")}, jsonFields(reflect.TypeOf(status{}), "phase", nil)) - }) -} - -// selfEmbedding is valid Go and valid encoding/json input: the embedded -// pointer can stay nil while Phase is encoded. -type selfEmbedding struct { - *selfEmbedding //nolint:unused // read only by reflection - Phase string `json:"phase"` -} - -// checksOwner is an OperatorCRD whose conditions encode as "checks". -type checksOwner struct { - metav1.TypeMeta `json:",inline"` - metav1.ObjectMeta `json:"metadata,omitempty"` - - Status checksStatus `json:"status,omitempty"` -} - -type checksStatus struct { - Checks []metav1.Condition `json:"checks,omitempty"` - Phase string `json:"phase,omitempty"` - LastSync metav1.Time `json:"lastSync,omitzero"` - Level textLevel `json:"level,omitzero"` - Extra map[string]any `json:"extra,omitempty"` - internal string //nolint:unused // read only by reflection - secretCode `json:"code"` //nolint:unused // read only by reflection -} - -type secretCode string //nolint:unused // embedded only to be found by reflection - -// textLevel encodes as a JSON string through MarshalText, so its Value field -// never appears in the JSON. -type textLevel struct { - Value string `json:"value"` -} - -func (l textLevel) MarshalText() ([]byte, error) { - return []byte(l.Value), nil -} - -func (l *textLevel) UnmarshalText(text []byte) error { - l.Value = string(text) - return nil -} - -func (o *checksOwner) GetStatusConditions() *[]metav1.Condition { - return &o.Status.Checks -} - -func (o *checksOwner) GetKind() string { - return "checksOwner" -} - -func (o *checksOwner) DeepCopyObject() runtime.Object { - out := *o - o.ObjectMeta.DeepCopyInto(&out.ObjectMeta) //nolint:staticcheck // naming ObjectMeta keeps the copy readable - out.Status.Checks = slices.Clone(o.Status.Checks) - if o.Status.Extra != nil { - out.Status.Extra = runtime.DeepCopyJSON(o.Status.Extra) - } - return &out -} - -// adapterOwner keeps private state that JSON cannot restore: the kind and a -// cached pointer to its conditions. -type adapterOwner struct { - metav1.TypeMeta `json:",inline"` - metav1.ObjectMeta `json:"metadata,omitempty"` - - Status checksStatus `json:"status,omitempty"` - - kind string - conditions *[]metav1.Condition -} - -func newAdapterOwner() *adapterOwner { - o := &adapterOwner{kind: "AdapterOwner"} - o.conditions = &o.Status.Checks - return o -} - -func (o *adapterOwner) GetStatusConditions() *[]metav1.Condition { - return o.conditions -} - -func (o *adapterOwner) GetKind() string { - return o.kind -} - -func (o *adapterOwner) DeepCopyObject() runtime.Object { - out := *o - o.ObjectMeta.DeepCopyInto(&out.ObjectMeta) //nolint:staticcheck // naming ObjectMeta keeps the copy readable - out.Status.Checks = slices.Clone(o.Status.Checks) - if o.Status.Extra != nil { - out.Status.Extra = runtime.DeepCopyJSON(o.Status.Extra) - } - out.conditions = &out.Status.Checks - return &out -} - -// pointerAdapterOwner is adapterOwner with a pointer status: its cached -// pointer reaches into the status struct the Status field points to. -type pointerAdapterOwner struct { - metav1.TypeMeta `json:",inline"` - metav1.ObjectMeta `json:"metadata,omitempty"` - - Status *checksStatus `json:"status,omitempty"` - - conditions *[]metav1.Condition -} - -func newPointerAdapterOwner() *pointerAdapterOwner { - o := &pointerAdapterOwner{Status: &checksStatus{}} - o.conditions = &o.Status.Checks - return o -} - -func (o *pointerAdapterOwner) GetStatusConditions() *[]metav1.Condition { - return o.conditions -} - -func (o *pointerAdapterOwner) GetKind() string { - return "pointerAdapterOwner" -} - -func (o *pointerAdapterOwner) DeepCopyObject() runtime.Object { - out := *o - o.ObjectMeta.DeepCopyInto(&out.ObjectMeta) //nolint:staticcheck // naming ObjectMeta keeps the copy readable - status := *o.Status - status.Checks = slices.Clone(o.Status.Checks) - if o.Status.Extra != nil { - status.Extra = runtime.DeepCopyJSON(o.Status.Extra) - } - out.Status = &status - out.conditions = &out.Status.Checks - return &out -} - -// decodingClient reads and writes like the REST client: results decode into -// the object passed in, which keeps fields the JSON does not carry. -type decodingClient struct { - client.Client -} - -func (c decodingClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { - return decodingReader{Reader: c.Client}.Get(ctx, key, obj, opts...) -} - -func (c decodingClient) Status() client.SubResourceWriter { - return decodingStatusWriter{SubResourceWriter: c.Client.Status()} -} - -type decodingStatusWriter struct { - client.SubResourceWriter -} - -func (w decodingStatusWriter) Update(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { - fresh, ok := obj.DeepCopyObject().(client.Object) - if !ok { - return fmt.Errorf("deep copy of %T is not a client.Object", obj) - } - if err := w.SubResourceWriter.Update(ctx, fresh, opts...); err != nil { - return err - } - - data, err := json.Marshal(fresh) - if err != nil { - return err - } - return json.Unmarshal(data, obj) -} - -// nullableStatusOwner encodes a nil status as null, because the field has no -// omitempty. -type nullableStatusOwner struct { - metav1.TypeMeta `json:",inline"` - metav1.ObjectMeta `json:"metadata,omitempty"` - - Status *checksStatus `json:"status"` -} - -func (o *nullableStatusOwner) GetStatusConditions() *[]metav1.Condition { - if o.Status == nil { - o.Status = &checksStatus{} - } - return &o.Status.Checks -} - -func (o *nullableStatusOwner) GetKind() string { - return "nullableStatusOwner" -} - -func (o *nullableStatusOwner) DeepCopyObject() runtime.Object { - out := *o - o.ObjectMeta.DeepCopyInto(&out.ObjectMeta) //nolint:staticcheck // naming ObjectMeta keeps the copy readable - if o.Status != nil { - status := *o.Status - status.Checks = slices.Clone(o.Status.Checks) - if o.Status.Extra != nil { - status.Extra = runtime.DeepCopyJSON(o.Status.Extra) - } - out.Status = &status - } - return &out -} - -// decodingReader reads like the REST client: it decodes the server's JSON into -// the object passed in, so fields the JSON omits are left as they were. -type decodingReader struct { - client.Reader -} - -func (r decodingReader) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { - fresh, ok := obj.DeepCopyObject().(client.Object) - if !ok { - return fmt.Errorf("deep copy of %T is not a client.Object", obj) - } - if err := r.Reader.Get(ctx, key, fresh, opts...); err != nil { - return err - } - - data, err := json.Marshal(fresh) - if err != nil { - return err - } - return json.Unmarshal(data, obj) -} diff --git a/pkg/component/flush_owned_fields_test.go b/pkg/component/flush_owned_fields_test.go new file mode 100644 index 00000000..8e8bb706 --- /dev/null +++ b/pkg/component/flush_owned_fields_test.go @@ -0,0 +1,336 @@ +package component + +import ( + "context" + "encoding/json" + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// These specs run against envtest because they depend on what the API server +// does with a merge patch: null removes a field, a resourceVersion in the body +// is a precondition, and a structural schema prunes unknown fields. +var _ = Describe("FlushStatus with WithOwnedStatusFields", func() { + var ( + ctx = context.Background() + namespace string + ) + + infraComponent := func() *Component { + GinkgoHelper() + comp, err := NewComponentBuilder().WithName("infra").WithConditionType("InfraReady").Build() + Expect(err).NotTo(HaveOccurred()) + return comp + } + + // readThenRace creates the owner with the given status and reads it as a + // controller does at the top of a reconcile. It then applies the second + // writer's change on the server, so the returned owner is stale and its + // first write conflicts. + readThenRace := func(status MockStatus, secondWriter func(*MockOperatorCRD)) *MockOperatorCRD { + GinkgoHelper() + created := &MockOperatorCRD{ObjectMeta: metav1.ObjectMeta{Name: "owner", Namespace: namespace}} + Expect(k8sClient.Create(ctx, created)).To(Succeed()) + created.Status = status + Expect(k8sClient.Status().Update(ctx, created)).To(Succeed()) + + owner := &MockOperatorCRD{} + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(created), owner)).To(Succeed()) + + if secondWriter != nil { + other := &MockOperatorCRD{} + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(created), other)).To(Succeed()) + secondWriter(other) + Expect(k8sClient.Status().Update(ctx, other)).To(Succeed()) + } + return owner + } + + persisted := func(owner *MockOperatorCRD) *MockOperatorCRD { + GinkgoHelper() + live := &MockOperatorCRD{} + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(owner), live)).To(Succeed()) + return live + } + + condition := func(ctype, reason string) metav1.Condition { + return metav1.Condition{ + Type: ctype, Status: metav1.ConditionTrue, Reason: reason, LastTransitionTime: metav1.Now(), + } + } + + BeforeEach(func() { + namespace = createNamespace(ctx, "flush-owned-fields-") + }) + + It("keeps a second writer's observedGeneration across the conflict retry", func() { + owner := readThenRace(MockStatus{ObservedGeneration: 1}, func(other *MockOperatorCRD) { + other.Status.ObservedGeneration = 2 + }) + recording := &patchRecordingClient{Client: k8sClient} + + owner.Status.Phase = "Accepted" + applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ + Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", + }) + + Expect(FlushStatus( + ctx, ReconcileContext{Client: recording, Owner: owner}, []*Component{infraComponent()}, + WithOwnedStatusFields("phase"), + )).To(Succeed()) + + Expect(recording.bodies).To(HaveLen(2), "the first patch must conflict and the retry must succeed") + live := persisted(owner) + Expect(live.Status.ObservedGeneration).To(Equal(int64(2))) + Expect(live.Status.Phase).To(Equal("Accepted")) + Expect(meta.FindStatusCondition(live.Status.Conditions, "InfraReady")).NotTo(BeNil()) + }) + + It("sends only the declared fields and the conditions, even without a conflict", func() { + owner := readThenRace(MockStatus{ObservedGeneration: 1}, nil) + recording := &patchRecordingClient{Client: k8sClient} + + owner.Status.ObservedGeneration = 99 // staged but not declared + owner.Status.Phase = "Accepted" + readVersion := owner.GetResourceVersion() + + Expect(FlushStatus( + ctx, ReconcileContext{Client: recording, Owner: owner}, nil, WithOwnedStatusFields("phase"), + )).To(Succeed()) + + Expect(recording.bodies).To(HaveLen(1)) + var body map[string]any + Expect(json.Unmarshal(recording.bodies[0], &body)).To(Succeed()) + Expect(body).To(HaveKey("metadata")) + Expect(body["metadata"]).To(Equal(map[string]any{"resourceVersion": readVersion})) + Expect(body["status"]).To(HaveKeyWithValue("phase", "Accepted")) + Expect(body["status"]).NotTo(HaveKey("observedGeneration")) + + live := persisted(owner) + Expect(live.Status.ObservedGeneration).To(Equal(int64(1)), "an undeclared field is never written") + Expect(live.Status.Phase).To(Equal("Accepted")) + }) + + It("removes a declared field that the staged owner cleared", func() { + owner := readThenRace(MockStatus{Phase: "Pending", ObservedGeneration: 1}, func(other *MockOperatorCRD) { + other.Status.ObservedGeneration = 2 + }) + + owner.Status.Phase = "" + + Expect(FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("phase"), + )).To(Succeed()) + + live := persisted(owner) + Expect(live.Status.Phase).To(BeEmpty()) + Expect(live.Status.ObservedGeneration).To(Equal(int64(2))) + }) + + It("writes a nested declared field and leaves its siblings to the server", func() { + owner := readThenRace( + MockStatus{Summary: &MockStatusSummary{Ready: 1, Total: 3}}, + func(other *MockOperatorCRD) { other.Status.Summary.Total = 4 }, + ) + + owner.Status.Summary.Ready = 2 + + Expect(FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("summary.ready"), + )).To(Succeed()) + + Expect(persisted(owner).Status.Summary).To(Equal(&MockStatusSummary{Ready: 2, Total: 4})) + }) + + It("lets owned conditions win and unowned conditions follow the server on a retry", func() { + owner := readThenRace( + MockStatus{Conditions: []metav1.Condition{ + condition("InfraReady", "StaleServerValue"), + condition("ExternalReady", "BeforeOtherWrite"), + }}, + func(other *MockOperatorCRD) { + meta.SetStatusCondition(&other.Status.Conditions, metav1.Condition{ + Type: "ExternalReady", Status: metav1.ConditionFalse, Reason: "WrittenByOtherController", + }) + meta.SetStatusCondition(&other.Status.Conditions, condition("AddedByOther", "BrandNew")) + }, + ) + + applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ + Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", + }) + + Expect(FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, []*Component{infraComponent()}, + WithOwnedStatusFields(), + )).To(Succeed()) + + live := persisted(owner) + Expect(meta.FindStatusCondition(live.Status.Conditions, "InfraReady").Reason).To(Equal("Healthy")) + Expect(meta.FindStatusCondition(live.Status.Conditions, "ExternalReady").Reason). + To(Equal("WrittenByOtherController")) + Expect(meta.FindStatusCondition(live.Status.Conditions, "AddedByOther")).NotTo(BeNil()) + }) + + It("does nothing for a misspelled path, because the schema prunes the unknown field", func() { + owner := readThenRace(MockStatus{Phase: "Pending"}, nil) + + owner.Status.Phase = "Accepted" + + Expect(FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("phsae"), + )).To(Succeed()) + + Expect(persisted(owner).Status.Phase).To(Equal("Pending"), "the declared path names no field, so nothing is written") + }) + + It("fails without writing for a declared path below a scalar the staged owner holds", func() { + owner := readThenRace(MockStatus{Phase: "Pending"}, nil) + recording := &patchRecordingClient{Client: k8sClient} + + err := FlushStatus( + ctx, ReconcileContext{Client: recording, Owner: owner}, nil, WithOwnedStatusFields("phase.value"), + ) + + Expect(err).To(MatchError(ContainSubstring(`"phase.value"`))) + Expect(recording.bodies).To(BeEmpty()) + Expect(persisted(owner).Status.Phase).To(Equal("Pending")) + }) + + DescribeTable("rejects invalid path syntax before any write", + func(path string) { + owner := readThenRace(MockStatus{}, nil) + recording := &patchRecordingClient{Client: k8sClient} + + err := FlushStatus( + ctx, ReconcileContext{Client: recording, Owner: owner}, nil, WithOwnedStatusFields(path), + ) + + Expect(err).To(MatchError(ContainSubstring(`"` + path + `"`))) + Expect(recording.bodies).To(BeEmpty()) + }, + Entry("empty", ""), + Entry("empty segment", "summary..ready"), + Entry("leading separator", ".phase"), + Entry("trailing separator", "phase."), + Entry("the conditions", "conditions"), + Entry("below the conditions", "conditions.type"), + ) +}) + +func TestConditionsJSONPath(t *testing.T) { + t.Run("finds the conditions directly under status", func(t *testing.T) { + path, err := conditionsJSONPath(&MockOperatorCRD{}) + + require.NoError(t, err) + assert.Equal(t, []string{"conditions"}, path) + }) + + t.Run("finds conditions nested below status under any JSON name", func(t *testing.T) { + path, err := conditionsJSONPath(&nestedConditionsOwner{}) + + require.NoError(t, err) + assert.Equal(t, []string{"health", "checks"}, path) + }) + + t.Run("leaves the staged conditions as they were", func(t *testing.T) { + staged := []metav1.Condition{{Type: "InfraReady", Reason: "Healthy"}} + owner := &MockOperatorCRD{Status: MockStatus{Conditions: staged}} + + _, err := conditionsJSONPath(owner) + + require.NoError(t, err) + assert.Equal(t, staged, owner.Status.Conditions) + }) +} + +func TestParseOwnedStatusPaths(t *testing.T) { + conditions := []string{"health", "checks"} + + t.Run("splits a valid path into its segments", func(t *testing.T) { + paths, err := parseOwnedStatusPaths([]string{"phase", "health.score"}, conditions) + + require.NoError(t, err) + assert.Equal(t, [][]string{{"phase"}, {"health", "score"}}, paths) + }) + + for name, path := range map[string]string{ + "the conditions": "health.checks", + "a path that contains them": "health", + "a path below them": "health.checks.type", + } { + t.Run("rejects "+name, func(t *testing.T) { + _, err := parseOwnedStatusPaths([]string{path}, conditions) + + require.Error(t, err) + assert.Contains(t, err.Error(), `"`+path+`"`) + }) + } +} + +// patchRecordingClient records the body of every status patch it forwards. +type patchRecordingClient struct { + client.Client + bodies [][]byte +} + +func (c *patchRecordingClient) Status() client.SubResourceWriter { + return &patchRecordingStatusWriter{SubResourceWriter: c.Client.Status(), parent: c} +} + +type patchRecordingStatusWriter struct { + client.SubResourceWriter + parent *patchRecordingClient +} + +func (w *patchRecordingStatusWriter) Patch( + ctx context.Context, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption, +) error { + body, err := patch.Data(obj) + if err != nil { + return err + } + w.parent.bodies = append(w.parent.bodies, body) + return w.SubResourceWriter.Patch(ctx, obj, patch, opts...) +} + +// nestedConditionsOwner keeps its conditions below status.health.checks, so +// the conditions JSON path has more than one segment. +type nestedConditionsOwner struct { + metav1.TypeMeta `json:",inline"` + metav1.ObjectMeta `json:"metadata,omitempty"` + + Status nestedConditionsStatus `json:"status,omitempty"` +} + +type nestedConditionsStatus struct { + Phase string `json:"phase,omitempty"` + Health healthStatus `json:"health,omitzero"` +} + +type healthStatus struct { + Checks []metav1.Condition `json:"checks,omitempty"` +} + +func (o *nestedConditionsOwner) GetStatusConditions() *[]metav1.Condition { + return &o.Status.Health.Checks +} + +func (o *nestedConditionsOwner) GetKind() string { + return "nestedConditionsOwner" +} + +func (o *nestedConditionsOwner) DeepCopyObject() runtime.Object { + out := *o + o.ObjectMeta.DeepCopyInto(&out.ObjectMeta) //nolint:staticcheck // naming ObjectMeta keeps the copy readable + out.Status.Health.Checks = append([]metav1.Condition(nil), o.Status.Health.Checks...) + return &out +} diff --git a/pkg/component/status_fields.go b/pkg/component/status_fields.go deleted file mode 100644 index 7997799e..00000000 --- a/pkg/component/status_fields.go +++ /dev/null @@ -1,391 +0,0 @@ -package component - -import ( - "encoding" - "encoding/json" - "errors" - "fmt" - "maps" - "reflect" - "slices" - "strings" - - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime" - kjson "sigs.k8s.io/json" -) - -var ( - jsonMarshaler = reflect.TypeFor[json.Marshaler]() - textMarshaler = reflect.TypeFor[encoding.TextMarshaler]() -) - -// FlushOption configures a [FlushStatus] call. -type FlushOption func(*flushOptions) - -type flushOptions struct { - // fieldsDeclared is set by any WithOwnedStatusFields option, including one - // with no paths: declaring that a flush owns no field differs from not - // declaring at all. - fieldsDeclared bool - ownedFields []string -} - -// WithOwnedStatusFields declares the non-condition status fields that this -// flush owns, for an owner whose status is shared with other writers. -// -// Each path names a field under status by its JSON name, with nested fields -// separated by dots, for example "observedGeneration" or "summary.ready". -// Paths are relative to status: write "phase", not "status.phase". A path must -// not name "conditions" or a field below it: condition ownership comes from the -// components passed to [FlushStatus]. For a typed owner every path must resolve -// to exactly one field in the JSON of the owner's Go type, so a misspelled or -// ambiguous path is an error rather than a silent loss of ownership. FlushStatus validates the -// paths before it writes and returns an error for an invalid one. -// -// The declaration takes effect only on a 409 conflict retry. An owned field -// keeps the staged value, and a field the staged owner does not carry is -// removed. Every other non-condition status field takes the server's value, and -// a field the server does not carry is removed. A field the controller staged -// but did not declare is therefore lost on a retry: declare every non-condition -// field the controller writes. -// -// Calling it with no paths declares that the flush owns no non-condition -// status field, as for a controller that writes only conditions. Paths from -// several options are combined. -func WithOwnedStatusFields(paths ...string) FlushOption { - return func(o *flushOptions) { - o.fieldsDeclared = true - o.ownedFields = append(o.ownedFields, paths...) - } -} - -// parseOwnedStatusFields splits each path into its segments and rejects a path -// the owner's type cannot carry. -func parseOwnedStatusFields(owner OperatorCRD, paths []string) ([][]string, error) { - parsed := make([][]string, 0, len(paths)) - for _, path := range paths { - segments := strings.Split(path, ".") - if err := validateStatusFieldPath(owner, segments); err != nil { - return nil, fmt.Errorf("owned status field %q: %w", path, err) - } - parsed = append(parsed, segments) - } - return parsed, nil -} - -func validateStatusFieldPath(owner OperatorCRD, segments []string) error { - for _, segment := range segments { - if segment == "" { - return errors.New("path has an empty segment") - } - } - - if segments[0] == "conditions" { - return errors.New("conditions are owned through the components passed to FlushStatus") - } - - // An unstructured owner has no Go type to check the path against. - if _, ok := owner.(runtime.Unstructured); ok { - return nil - } - - ownerType := indirectType(reflect.TypeOf(owner)) - index, ok := statusFieldIndex(ownerType) - if !ok { - return fmt.Errorf("type %s has no exported field with JSON name \"status\"", ownerType) - } - - current := ownerType.Field(index).Type - for _, segment := range segments { - current = indirectType(current) - // A type such as metav1.Time encodes itself, often as a string, so its - // Go fields say nothing about its JSON shape. - if encodesItself(current) { - return fmt.Errorf("cannot address %q below a field of type %s, which encodes itself", segment, current) - } - // Below a map or an interface the schema is open, so any key is valid. - if current.Kind() == reflect.Map || current.Kind() == reflect.Interface { - return nil - } - if current.Kind() != reflect.Struct { - return fmt.Errorf("cannot address %q below a field of type %s", segment, current) - } - - candidates := jsonFields(current, segment, nil) - switch len(candidates) { - case 0: - return fmt.Errorf("type %s has no field with JSON name %q", current, segment) - case 1: - current = candidates[0] - default: - // encoding/json picks one of several such fields by tag and depth - // rules. Rejecting the path keeps ownership from landing on a - // field other than the one that is encoded. - return fmt.Errorf("type %s has %d fields with JSON name %q, directly or through embedded structs", - current, len(candidates), segment) - } - } - return nil -} - -func encodesItself(t reflect.Type) bool { - pointer := reflect.PointerTo(t) - return pointer.Implements(jsonMarshaler) || pointer.Implements(textMarshaler) -} - -// jsonFields returns the type of every field with the JSON name name, declared -// on t or on a struct that t embeds at any depth. Pass nil for visiting. -func jsonFields(t reflect.Type, name string, visiting map[reflect.Type]bool) []reflect.Type { - // A struct can embed itself through a pointer. visiting holds the structs - // on the current embedding chain, so such a chain ends instead of looping. - // It is copied per chain, so the same struct embedded twice side by side - // still counts twice toward ambiguity. - if visiting[t] { - return nil - } - visiting = maps.Clone(visiting) - if visiting == nil { - visiting = map[reflect.Type]bool{} - } - visiting[t] = true - - var found []reflect.Type - for i := range t.NumField() { - field := t.Field(i) - tag := field.Tag.Get("json") - if tag == "-" { - continue - } - - // encoding/json skips unexported fields, except that the exported - // fields of an unexported embedded struct are promoted. - embedded := indirectType(field.Type) - if !field.IsExported() && (!field.Anonymous || embedded.Kind() != reflect.Struct) { - continue - } - - fieldName, _, _ := strings.Cut(tag, ",") - if fieldName == "" && field.Anonymous && embedded.Kind() == reflect.Struct { - found = append(found, jsonFields(embedded, name, visiting)...) - continue - } - if fieldName == "" { - fieldName = field.Name - } - if fieldName == name { - found = append(found, field.Type) - } - } - return found -} - -func indirectType(t reflect.Type) reflect.Type { - for t.Kind() == reflect.Pointer { - t = t.Elem() - } - return t -} - -func zeroStatus(obj OperatorCRD) error { - return setStatus(obj, nil) -} - -// applyOwnedStatusFields sets on staged the non-condition status of server, -// except for the owned paths, which keep the staged value. Conditions are left -// as staged. -func applyOwnedStatusFields(staged, server OperatorCRD, owned [][]string) error { - // GetStatusConditions decides where the conditions live, and their JSON - // name can be anything, so they are carried across the conversion through - // the accessor rather than by key. - conditions := slices.Clone(*staged.GetStatusConditions()) - - stagedContent, err := toUnstructuredContent(staged) - if err != nil { - return err - } - - serverContent, err := toUnstructuredContent(server) - if err != nil { - return err - } - - stagedStatus, err := statusOf(stagedContent) - if err != nil { - return fmt.Errorf("reading staged status: %w", err) - } - - merged, err := statusOf(serverContent) - if err != nil { - return fmt.Errorf("reading server status: %w", err) - } - - for _, path := range owned { - value, found, err := lookupStatusField(stagedStatus, path) - if err != nil { - return fmt.Errorf("reading staged status field %q: %w", strings.Join(path, "."), err) - } - if !found { - unstructured.RemoveNestedField(merged, path...) - continue - } - - // A nullable parent without omitempty encodes as null, and - // SetNestedField cannot descend through a null. - removeNullParent(merged, path) - if err := unstructured.SetNestedField(merged, value, path...); err != nil { - return fmt.Errorf("setting status field %q: %w", strings.Join(path, "."), err) - } - } - - if err := setStatus(staged, merged); err != nil { - return err - } - - *staged.GetStatusConditions() = conditions - return nil -} - -// lookupStatusField returns the value at path. A null parent counts as -// absent, the same as a missing one. -func lookupStatusField(status map[string]any, path []string) (any, bool, error) { - var current any = status - for i, segment := range path { - if current == nil { - return nil, false, nil - } - - object, ok := current.(map[string]any) - if !ok { - return nil, false, fmt.Errorf("%q is not an object", strings.Join(path[:i], ".")) - } - - current, ok = object[segment] - if !ok { - return nil, false, nil - } - } - return current, true, nil -} - -func removeNullParent(status map[string]any, path []string) { - for i := 1; i < len(path); i++ { - value, found, _ := lookupStatusField(status, path[:i]) - if found && value == nil { - unstructured.RemoveNestedField(status, path[:i]...) - return - } - } -} - -// statusOf returns the status map of content, which the caller may modify. A -// status that is absent or null, as a nullable status without omitempty -// encodes, is an empty map. -func statusOf(content map[string]any) (map[string]any, error) { - status, ok := content["status"] - if !ok || status == nil { - return map[string]any{}, nil - } - - object, ok := status.(map[string]any) - if !ok { - return nil, fmt.Errorf("status is a %T, not an object", status) - } - return runtime.DeepCopyJSON(object), nil -} - -// toUnstructuredContent returns a copy of obj's content that the caller may -// modify. UnstructuredContent returns the object's own map, not a copy. -func toUnstructuredContent(obj OperatorCRD) (map[string]any, error) { - if u, ok := obj.(runtime.Unstructured); ok { - return runtime.DeepCopyJSON(u.UnstructuredContent()), nil - } - - // JSON is what the API server receives. The decoder is the API - // machinery's own, which keeps integers as exact int64 values. - data, err := json.Marshal(obj) - if err != nil { - return nil, fmt.Errorf("encoding %T: %w", obj, err) - } - - var content map[string]any - if err := kjson.UnmarshalCaseSensitivePreserveInts(data, &content); err != nil { - return nil, fmt.Errorf("decoding %T: %w", obj, err) - } - return content, nil -} - -// setStatus replaces the status of obj with status, or clears it when status -// is nil. Nothing outside the status changes, so state that JSON cannot carry, -// such as a cached pointer behind GetStatusConditions, survives. -func setStatus(obj OperatorCRD, status map[string]any) error { - if u, ok := obj.(runtime.Unstructured); ok { - content := runtime.DeepCopyJSON(u.UnstructuredContent()) - if status == nil { - delete(content, "status") - } else { - content["status"] = status - } - u.SetUnstructuredContent(content) - return nil - } - - owner := reflect.ValueOf(obj).Elem() - index, ok := statusFieldIndex(owner.Type()) - if !ok { - return fmt.Errorf("type %s has no exported field with JSON name \"status\"", owner.Type()) - } - - // Write through a non-nil status pointer instead of replacing it: a cached - // pointer into the status struct must keep reaching the struct that gets - // encoded. - target := owner.Field(index) - for target.Kind() == reflect.Pointer { - if target.IsNil() { - if status == nil { - return nil - } - target.Set(reflect.New(target.Type().Elem())) - } - target = target.Elem() - } - - if status == nil { - target.SetZero() - return nil - } - - data, err := json.Marshal(status) - if err != nil { - return fmt.Errorf("encoding status for %T: %w", obj, err) - } - - fresh := reflect.New(target.Type()) - if err := kjson.UnmarshalCaseSensitivePreserveInts(data, fresh.Interface()); err != nil { - return fmt.Errorf("decoding status into %T: %w", obj, err) - } - target.Set(fresh.Elem()) - return nil -} - -func statusFieldIndex(t reflect.Type) (int, bool) { - if t.Kind() != reflect.Struct { - return 0, false - } - - for i := range t.NumField() { - field := t.Field(i) - if !field.IsExported() { - continue - } - - name, _, _ := strings.Cut(field.Tag.Get("json"), ",") - if name == "" && !field.Anonymous { - name = field.Name - } - if name == "status" { - return i, true - } - } - return 0, false -} diff --git a/pkg/component/status_patch.go b/pkg/component/status_patch.go new file mode 100644 index 00000000..f6628c80 --- /dev/null +++ b/pkg/component/status_patch.go @@ -0,0 +1,257 @@ +package component + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "slices" + "strings" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/retry" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// FlushOption configures a [FlushStatus] call. +type FlushOption func(*flushOptions) + +type flushOptions struct { + // fieldsDeclared is set by any WithOwnedStatusFields option, including one + // with no paths: declaring that a flush owns no field differs from not + // declaring at all. + fieldsDeclared bool + ownedFields []string +} + +// conditionsMarker is the condition type staged on the owner while +// conditionsJSONPath looks for it in the owner's JSON. +const conditionsMarker = "ocf.sourcehawk.io/flush-status-conditions-marker" + +// WithOwnedStatusFields declares the non-condition status fields that this +// flush owns, for an owner whose status is shared with other writers. It turns +// every write of [FlushStatus] into a JSON merge patch of the status that holds +// only the declared fields and the conditions, so a field that another writer +// owns is never sent, on the first attempt or on a retry. +// +// Each path names a field under status by its JSON name, with nested fields +// separated by dots, for example "observedGeneration" or "summary.ready". +// Paths are relative to status: write "phase", not "status.phase". A declared +// field takes the staged owner's value, and a declared field that the staged +// owner does not carry is removed on the server. Calling it with no paths +// declares that the flush owns no non-condition status field, as for a +// controller that writes only conditions. Paths from several options are +// combined. +// +// FlushStatus returns an error without writing when a path has an empty +// segment, names the conditions, contains them, or lies below them; condition +// ownership comes from the components passed to FlushStatus. It also fails +// when a declared path runs below a field that the staged owner holds as a +// non-object value. The framework does not check a path against the CRD +// schema: with a structural schema the API server prunes a misspelled field, +// so that part of the write does nothing. +func WithOwnedStatusFields(paths ...string) FlushOption { + return func(o *flushOptions) { + o.fieldsDeclared = true + o.ownedFields = append(o.ownedFields, paths...) + } +} + +// patchOwnedStatus writes the declared fields and the conditions of rec.Owner +// with a merge patch guarded by the staged resourceVersion, and retries on a +// conflict after refreshing the staged owner's resourceVersion and unowned +// conditions from the server. +func patchOwnedStatus( + ctx context.Context, rec ReconcileContext, ownedConditions map[string]struct{}, rawPaths []string, +) error { + invalid := func(err error) error { + return fmt.Errorf( + "flushing status of %s %s: %w", rec.Owner.GetKind(), client.ObjectKeyFromObject(rec.Owner), err, + ) + } + + conditionsPath, err := conditionsJSONPath(rec.Owner) + if err != nil { + return invalid(err) + } + + paths, err := parseOwnedStatusPaths(rawPaths, conditionsPath) + if err != nil { + return invalid(err) + } + + // Build the first body before any write, so a path that cannot be read + // from the staged owner fails without touching the server. + body, err := ownedStatusPatch(rec.Owner, paths, conditionsPath) + if err != nil { + return invalid(err) + } + + return retry.RetryOnConflict(retry.DefaultRetry, func() error { + patchErr := rec.Client.Status().Patch(ctx, rec.Owner, client.RawPatch(types.MergePatchType, body)) + if patchErr == nil || !apierrors.IsConflict(patchErr) { + return patchErr + } + + if refreshErr := refreshUnownedConditions(ctx, rec, ownedConditions); refreshErr != nil { + return refreshErr + } + + var buildErr error + if body, buildErr = ownedStatusPatch(rec.Owner, paths, conditionsPath); buildErr != nil { + return buildErr + } + return patchErr + }) +} + +// conditionsJSONPath returns the JSON path under status of the list that +// GetStatusConditions returns. OperatorCRD does not fix that list's JSON name +// or depth, so a marker condition is staged for a moment and found in the +// owner's JSON. +func conditionsJSONPath(owner OperatorCRD) ([]string, error) { + conditions := owner.GetStatusConditions() + staged := *conditions + *conditions = []metav1.Condition{{Type: conditionsMarker}} + data, err := json.Marshal(owner) + *conditions = staged + if err != nil { + return nil, fmt.Errorf("encoding %T: %w", owner, err) + } + + var content map[string]any + if err := json.Unmarshal(data, &content); err != nil { + return nil, fmt.Errorf("decoding %T: %w", owner, err) + } + + status, _ := content["status"].(map[string]any) + path, found := findConditionsMarker(status) + if !found { + return nil, fmt.Errorf("%T does not encode its status conditions under status", owner) + } + return path, nil +} + +func findConditionsMarker(object map[string]any) ([]string, bool) { + for key, value := range object { + switch typed := value.(type) { + case []any: + if len(typed) != 1 { + continue + } + if item, ok := typed[0].(map[string]any); ok && item["type"] == conditionsMarker { + return []string{key}, true + } + case map[string]any: + if path, found := findConditionsMarker(typed); found { + return append([]string{key}, path...), true + } + } + } + return nil, false +} + +func parseOwnedStatusPaths(paths []string, conditionsPath []string) ([][]string, error) { + parsed := make([][]string, 0, len(paths)) + for _, path := range paths { + segments := strings.Split(path, ".") + if slices.Contains(segments, "") { + return nil, fmt.Errorf("owned status field %q: path has an empty segment", path) + } + + // The patch writes the conditions under their own path, so a declared + // path that overlaps it would collide with that write. + if isPrefix(segments, conditionsPath) || isPrefix(conditionsPath, segments) { + return nil, fmt.Errorf( + "owned status field %q: overlaps the conditions at %q, which are owned through the components passed to FlushStatus", + path, strings.Join(conditionsPath, "."), + ) + } + parsed = append(parsed, segments) + } + return parsed, nil +} + +func isPrefix(prefix, path []string) bool { + return len(prefix) <= len(path) && slices.Equal(prefix, path[:len(prefix)]) +} + +// ownedStatusPatch returns the merge patch body for the staged owner: its +// resourceVersion as a precondition, each declared path with the staged value +// or null, and the staged conditions. +func ownedStatusPatch(owner OperatorCRD, paths [][]string, conditionsPath []string) ([]byte, error) { + data, err := json.Marshal(owner) + if err != nil { + return nil, fmt.Errorf("encoding %T: %w", owner, err) + } + + // json.Number re-encodes a number exactly as it was, so an int64 is not + // rounded through float64. + decoder := json.NewDecoder(bytes.NewReader(data)) + decoder.UseNumber() + var content map[string]any + if err := decoder.Decode(&content); err != nil { + return nil, fmt.Errorf("decoding %T: %w", owner, err) + } + staged, _ := content["status"].(map[string]any) + + status := map[string]any{} + for _, path := range append(slices.Clone(paths), conditionsPath) { + value, err := stagedValue(staged, path) + if err != nil { + return nil, fmt.Errorf("owned status field %q: %w", strings.Join(path, "."), err) + } + setPatchValue(status, path, value) + } + + patch := map[string]any{"status": status} + if resourceVersion := owner.GetResourceVersion(); resourceVersion != "" { + patch["metadata"] = map[string]any{"resourceVersion": resourceVersion} + } + return json.Marshal(patch) +} + +// stagedValue returns the value at path in the staged status, or nil when the +// staged owner does not carry it, which the patch sends as null to remove the +// field. An absent or null parent counts as absent. +func stagedValue(status map[string]any, path []string) (any, error) { + var current any = status + for i, segment := range path { + if current == nil { + return nil, nil + } + + object, ok := current.(map[string]any) + if !ok { + return nil, errors.New(strings.Join(path[:i], ".") + " is not an object in the staged status") + } + current = object[segment] + } + return current, nil +} + +// setPatchValue sets value at path in the patch, creating parent objects. A +// parent already set to a non-object value by a shorter declared path covers +// the longer one, so the longer path is skipped. +func setPatchValue(patch map[string]any, path []string, value any) { + current := patch + for _, segment := range path[:len(path)-1] { + next, exists := current[segment] + if !exists { + child := map[string]any{} + current[segment] = child + current = child + continue + } + + child, ok := next.(map[string]any) + if !ok { + return + } + current = child + } + current[path[len(path)-1]] = value +} diff --git a/pkg/component/zz_mock_crd_test.go b/pkg/component/zz_mock_crd_test.go index dfec022e..380f8f41 100644 --- a/pkg/component/zz_mock_crd_test.go +++ b/pkg/component/zz_mock_crd_test.go @@ -41,8 +41,6 @@ type MockStatus struct { // split status ownership between controllers by field and by nested path. Phase string `json:"phase,omitempty"` Summary *MockStatusSummary `json:"summary,omitempty"` - // Details lacks omitempty so tests can cover a parent the server holds as null. - Details *MockStatusSummary `json:"details"` } type MockStatusSummary struct { @@ -109,14 +107,6 @@ var ( "total": {Type: "integer", Format: "int32"}, }, }, - "details": { - Type: "object", - Nullable: true, - Properties: map[string]apiextensionsv1.JSONSchemaProps{ - "ready": {Type: "integer", Format: "int32"}, - "total": {Type: "integer", Format: "int32"}, - }, - }, "conditions": { Type: "array", Items: &apiextensionsv1.JSONSchemaPropsOrArray{ @@ -161,10 +151,6 @@ func (m *MockOperatorCRD) DeepCopyInto(out *MockOperatorCRD) { summary := *m.Status.Summary out.Status.Summary = &summary } - if m.Status.Details != nil { - details := *m.Status.Details - out.Status.Details = &details - } } func (m *MockOperatorCRD) DeepCopy() *MockOperatorCRD { diff --git a/plugin/skills/building-components/SKILL.md b/plugin/skills/building-components/SKILL.md index f503884c..2f19b3aa 100644 --- a/plugin/skills/building-components/SKILL.md +++ b/plugin/skills/building-components/SKILL.md @@ -233,7 +233,8 @@ the list. This split lets a controller with several components stage several con them in one write, instead of racing a write per component. That `Status().Update` writes the **whole status subresource**, not only the conditions, so fetch the owner fresh at the -top of every reconcile: an owner carried over from an earlier pass writes stale values back over newer ones. +top of every reconcile: an owner carried over from an earlier pass writes stale values back over newer ones. With +`WithOwnedStatusFields` the write is a merge patch of the declared fields and the conditions instead (see below). **Pass the components whose conditions this flush owns:** `component.FlushStatus(ctx, recCtx, comps)`. Their condition types are the owned set, so it cannot drift from what the components actually write. @@ -249,11 +250,12 @@ old wider ownership, making a correctness fix opt-in and invisible. On a 409 conflict `FlushStatus` keeps the staged owner as the object it writes. It fetches the server's copy into a separate object through `ReconcileContext.APIReader` (set it from the manager's `GetAPIReader()`; the default `Client` -reads from the informer cache, which can lag the conflicting write and return the same stale `resourceVersion`) and -retries. From the server's copy the retry takes the `resourceVersion`, every condition whose type this flush does not -own, and, only with `component.WithOwnedStatusFields`, every non-condition status field outside the declared paths. -Everything else keeps the staged value. So without the option, every staged non-condition field survives a conflict, -including the stale copy of a field another controller owns. +reads from the informer cache, which can lag the conflicting write and return the same stale `resourceVersion`), takes +that copy's `resourceVersion`, restores from it only the conditions whose type this flush does not own, and retries. No +other status field is taken from the server. Two consequences: the status fields the flush writes keep their staged +values, and a type this flush does not own keeps the server's value instead of the possibly stale copy the controller is +holding, so another writer's concurrent update is not rolled back. Without `WithOwnedStatusFields` the flush writes +every non-condition field, so a retry also writes back a stale copy of another controller's field. For unowned types the server is the source of truth, absence included. An unowned condition the server no longer carries is dropped rather than written back, so a condition another writer removed is not resurrected, and one the server has @@ -304,11 +306,13 @@ dots for nested fields: component.FlushStatus(ctx, recCtx, comps, component.WithOwnedStatusFields("phase")) ``` -On a retry a declared field keeps the staged value, and every other non-condition field takes the server's value, -absence included. `WithOwnedStatusFields()` with no paths owns no non-condition field. Once declared, an undeclared -staged field follows the server, so declare every field the controller writes, including `observedGeneration` when it is -this controller's. `FlushStatus` returns an error without writing for an empty segment, a `conditions` path, or a path -that does not resolve to exactly one field of the owner's Go type. +With the option, every attempt is a JSON merge patch of the status that holds only the staged `resourceVersion` (as a +precondition), the declared fields (a field the staged owner does not carry is sent as `null` and removed), and the +conditions. An undeclared field is never sent, so declare every field the controller writes, including +`observedGeneration` when it is this controller's. `WithOwnedStatusFields()` with no paths owns no non-condition field. +`FlushStatus` returns an error without writing for an empty segment, a path that overlaps the conditions, or a path +below a non-object value. A misspelled path is not caught: a structural CRD schema prunes the unknown field, so that +part of the write does nothing. **Server-side apply is for managed resources, never for the owner's status subresource.** An SSA status patch alongside `FlushStatus` puts two field managers on one conditions list, and because `FlushStatus` writes the full status the diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index 95104c3a..9331f1ec 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -818,29 +818,27 @@ func (r *WebAppReconciler) Reconcile(ctx context.Context, req reconcile.Request) status subresource**, not only the conditions. Every status field the controller staged in memory is sent, and every field it did not stage is sent exactly as it stands on the in-memory object. Fetch the owner fresh at the top of each reconcile, so the fields nobody staged still hold current server state. An owner carried over from an earlier pass -writes stale values back over newer ones. +writes stale values back over newer ones. With +[`WithOwnedStatusFields`](#sharing-the-owners-status-with-another-controller), the write is a merge patch of the +declared fields and the conditions instead. On a 409 Conflict, for example when another writer updated the owner between the controller's `Get` and this call, -`FlushStatus` keeps the staged owner as the object it writes. It fetches the server's copy into a **separate** object -and retries. The staged owner is never replaced. From the server's copy, the retry takes: - -- the `resourceVersion`, so the retry targets the live object; -- every condition whose type this flush does not own; -- with [`WithOwnedStatusFields`](#sharing-the-owners-status-with-another-controller), every non-condition status field - outside the declared paths. - -Everything else keeps the staged value. +`FlushStatus` keeps the staged owner as the object it writes. It fetches the server's copy into a **separate** object, +takes that copy's `resourceVersion` so the retry targets the live object, restores from it only the conditions whose +type this flush does not own, and retries. The staged owner is never replaced, and no other status field is taken from +the server. That fetch goes through `ReconcileContext.APIReader` when it is set, and through `Client` otherwise. Set it from the manager's `GetAPIReader()`. The manager's default `Client` serves reads from the informer cache, which can lag the very write that caused the conflict and hand back the same stale `resourceVersion`, so a retry through the cache may conflict again until the cache catches up. A direct read sees the server as it is. -Two things follow from that. A condition type this flush does not own keeps the server's value rather than the copy the -controller happens to be holding, so a concurrent update by another writer is not rolled back. And without -`WithOwnedStatusFields`, every non-condition status field keeps the staged value: a field the controller staged during -the reconcile survives a conflict, and so does the stale copy of a field that another controller owns. When another -controller writes non-condition fields of the same status, declare the fields this flush owns, as described in +Two things follow from that. A status field that the flush writes keeps its staged value across a conflict, because the +object holding it is the object that gets written. And a condition type this flush does not own keeps the server's value +rather than the copy the controller happens to be holding, so a concurrent update by another writer is not rolled back. +Without `WithOwnedStatusFields`, the flush writes every non-condition status field, so a retry also writes back the +stale copy of a field that another controller changed after the fetch. When another controller writes non-condition +fields of the same status, see [Sharing the owner's status with another controller](#sharing-the-owners-status-with-another-controller). Which types count as owned comes from the components passed to `FlushStatus`: @@ -943,34 +941,39 @@ flushErr := component.FlushStatus(ctx, recCtx, comps, component.WithOwnedStatusF flushErr := component.FlushStatus(ctx, recCtx, comps, component.WithOwnedStatusFields("observedGeneration")) ``` -The declaration changes only the conflict retry. The first write sends the whole staged status, as without the option. -When nothing changed on the server since the controller read the owner, that write succeeds, and the fields the -controller did not stage still hold the values it read. On a retry: +With the option, every attempt of `FlushStatus` is a JSON merge patch of the status instead of a `Status().Update`. The +patch holds only: + +- the staged owner's `metadata.resourceVersion`, as a precondition: the API server rejects the patch with a 409 when the + owner changed after the controller read it; +- each declared field with the staged owner's value, or `null` when the staged owner does not carry the field, so the + server removes it; +- the conditions list, with the owned conditions as staged and the unowned conditions as the server holds them. -- A declared field keeps the staged value. If the staged owner does not carry the field, the retry removes it. -- Every other non-condition status field takes the server's value. If the server does not carry the field, the retry - removes it. This is the same rule as for an unowned condition. -- Conditions follow component ownership, as described above. +A field that the controller did not declare is never sent, on the first attempt or on a retry, so it cannot overwrite +another writer's value. On a 409, `FlushStatus` refreshes the `resourceVersion` and the unowned conditions from the +server, as described above, builds the patch again, and retries. After a successful write, the controller-runtime client +decodes the server's response into the owner, so a value staged in a field that you did not declare does not survive in +memory. Name each field by its JSON name under `status`, with dots between nested fields: `"observedGeneration"`, -`"summary.ready"`. Do not include the `status.` prefix: `"status.phase"` is an error unless status has a field named -`status`. `WithOwnedStatusFields()` with no paths declares that the flush owns no non-condition field, which fits a -controller that writes only conditions. Paths from several options are combined. +`"summary.ready"`. Do not include the `status.` prefix. `WithOwnedStatusFields()` with no paths declares that the flush +owns no non-condition field, which fits a controller that writes only conditions. Paths from several options are +combined. Declare every non-condition field that the controller writes, including `observedGeneration` when it is this +controller's field: a field that you stage but do not declare is not written. -`FlushStatus` validates each path before it writes and returns an error, with no write, when a path: +`FlushStatus` returns an error, with no write, when a path: - is empty or has an empty segment, such as `"summary..ready"`; -- names `conditions` or a field below it, because conditions are owned through components; -- does not resolve to a field that the owner's Go type encodes in its JSON, such as a misspelled name, an unexported - field, a path below a scalar field, or a path below a type that encodes itself, such as `metav1.Time`. Below a map - field, any key is accepted; -- is ambiguous: a segment matches more than one field, because a struct declares the JSON name and also gets it from an - embedded struct, or gets it from two embedded structs. - -Without the option, `FlushStatus` keeps the staged value of every non-condition status field on a retry. That default -keeps a controller's own `observedGeneration` write safe when it is the only writer of the status. Once you declare -fields, a staged field that you did not declare follows the server on a retry. So declare every field that the -controller writes, including `observedGeneration` when it is this controller's field. +- names the conditions, contains them, or lies below them, because conditions are owned through components. + `FlushStatus` finds the JSON path of the list that `GetStatusConditions` returns, so this holds whatever that field is + called; +- runs below a field that the staged owner holds as a value that is not an object, such as `"phase.value"` when `phase` + is a string. + +The framework does not check a path against the CRD schema. A misspelled path, such as `"phsae"`, sends a field that the +schema does not declare. With a structural schema the API server prunes that field, so that part of the write does +nothing and `FlushStatus` returns no error. Check each path against the CRD type when you add it. ### Where the owner's observedGeneration is set diff --git a/plugin/skills/structuring-operators/SKILL.md b/plugin/skills/structuring-operators/SKILL.md index 7696d627..e462aad3 100644 --- a/plugin/skills/structuring-operators/SKILL.md +++ b/plugin/skills/structuring-operators/SKILL.md @@ -102,8 +102,8 @@ Consequences worth holding on to: - **Set the owner's `status.observedGeneration` yourself.** The framework stamps `ObservedGeneration` on component conditions only. - **A status shared by two controllers gives each field one owner.** Each controller declares its non-condition fields - with `FlushStatus(..., component.WithOwnedStatusFields("phase"))`, or a conflict retry rolls back the other - controller's newer values. Only the owner of `observedGeneration` sets it. + with `FlushStatus(..., component.WithOwnedStatusFields("phase"))`, so the flush patches only those fields and the + conditions and never writes the other controller's fields. Only the owner of `observedGeneration` sets it. Full behavior table and reference: `ocf:building-components`, and `references/guidelines.md`, "Derive the Owner's Aggregate Condition from Component Conditions", for the practice framing. diff --git a/plugin/skills/structuring-operators/references/guidelines.md b/plugin/skills/structuring-operators/references/guidelines.md index 921a499f..de1e049f 100644 --- a/plugin/skills/structuring-operators/references/guidelines.md +++ b/plugin/skills/structuring-operators/references/guidelines.md @@ -192,9 +192,9 @@ and skips metric emission. When two controllers write the status of one CR, give each status field one owner, as with conditions. Each controller passes the components that own its conditions and declares its non-condition fields with -[`WithOwnedStatusFields`](../../building-components/references/component.md#sharing-the-owners-status-with-another-controller). -Without the declaration, a conflict retry writes the controller's stale copy of the other controller's fields back over -newer values. +[`WithOwnedStatusFields`](../../building-components/references/component.md#sharing-the-owners-status-with-another-controller), +so its flush patches only those fields and the conditions. Without the declaration, the flush writes the whole status, +and a conflict retry writes the controller's stale copy of the other controller's fields back over newer values. Building the component set from a pure resolver `(spec, version) -> []*component.Component` keeps the loop stable: enabling an optional feature changes which components the resolver returns without touching the reconcile loop. From 699d96841a2999038cd2adfe406257c6104eb7c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 16 Oct 2026 17:13:24 +0100 Subject: [PATCH 09/13] fix(component): remove dropped children of a declared status object 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 --- docs/component.md | 3 +- pkg/component/component.go | 13 +- pkg/component/flush_owned_fields_test.go | 44 ++++++ pkg/component/status_patch.go | 128 ++++++++++++++---- plugin/skills/building-components/SKILL.md | 12 +- .../references/component.md | 3 +- 6 files changed, 164 insertions(+), 39 deletions(-) diff --git a/docs/component.md b/docs/component.md index bd8c9b21..7a24a334 100644 --- a/docs/component.md +++ b/docs/component.md @@ -944,7 +944,8 @@ patch holds only: - the staged owner's `metadata.resourceVersion`, as a precondition: the API server rejects the patch with a 409 when the owner changed after the controller read it; - each declared field with the staged owner's value, or `null` when the staged owner does not carry the field, so the - server removes it; + server removes it. A declared object is owned whole: `FlushStatus` reads the server's version of it and sends `null` + for each child that the staged owner does not carry, so a child that `omitempty` dropped is removed too; - the conditions list, with the owned conditions as staged and the unowned conditions as the server holds them. A field that the controller did not declare is never sent, on the first attempt or on a retry, so it cannot overwrite diff --git a/pkg/component/component.go b/pkg/component/component.go index 4cf8e90e..dafaf562 100644 --- a/pkg/component/component.go +++ b/pkg/component/component.go @@ -750,7 +750,7 @@ func FlushStatus(ctx context.Context, rec ReconcileContext, comps []*Component, if !apierrors.IsConflict(updateErr) { return updateErr } - if refreshErr := refreshUnownedConditions(ctx, rec, owned); refreshErr != nil { + if _, refreshErr := refreshUnownedConditions(ctx, rec, owned); refreshErr != nil { return refreshErr } return updateErr @@ -792,10 +792,13 @@ func ownedConditionTypes(comps []*Component) map[string]struct{} { // resourceVersion, so the retry targets the live object, and the value of every // condition type this flush does not own, so a concurrent update by another // writer is not overwritten by the copy this controller happens to be holding. -func refreshUnownedConditions(ctx context.Context, rec ReconcileContext, owned map[string]struct{}) error { +// It returns the server's copy it read. +func refreshUnownedConditions( + ctx context.Context, rec ReconcileContext, owned map[string]struct{}, +) (OperatorCRD, error) { server, ok := rec.Owner.DeepCopyObject().(OperatorCRD) if !ok { - return fmt.Errorf("failed to deep copy owner of type %T", rec.Owner) + return nil, fmt.Errorf("failed to deep copy owner of type %T", rec.Owner) } // Clear the copy's conditions before the fetch. Decoding a response into a // non-empty struct does not remove fields the response omits, so an owner @@ -811,7 +814,7 @@ func refreshUnownedConditions(ctx context.Context, rec ReconcileContext, owned m reader = rec.APIReader } if err := reader.Get(ctx, client.ObjectKeyFromObject(rec.Owner), server); err != nil { - return err + return nil, err } rec.Owner.SetResourceVersion(server.GetResourceVersion()) @@ -840,7 +843,7 @@ func refreshUnownedConditions(ctx context.Context, rec ReconcileContext, owned m } replaceStatusCondition(staged, cond) } - return nil + return server, nil } // replaceStatusCondition sets cond by type, taking its value verbatim. diff --git a/pkg/component/flush_owned_fields_test.go b/pkg/component/flush_owned_fields_test.go index 8e8bb706..d2caa61a 100644 --- a/pkg/component/flush_owned_fields_test.go +++ b/pkg/component/flush_owned_fields_test.go @@ -119,6 +119,50 @@ var _ = Describe("FlushStatus with WithOwnedStatusFields", func() { Expect(live.Status.Phase).To(Equal("Accepted")) }) + It("leaves the server's state in memory, not undeclared staged values", func() { + owner := readThenRace(MockStatus{}, nil) + + owner.Status.ObservedGeneration = 99 // undeclared, and absent on the server + owner.Status.Summary = &MockStatusSummary{Ready: 5} + owner.Status.Phase = "Accepted" + + Expect(FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("phase"), + )).To(Succeed()) + + Expect(owner.Status.ObservedGeneration).To(BeZero()) + Expect(owner.Status.Summary).To(BeNil()) + Expect(owner.Status.Phase).To(Equal("Accepted")) + }) + + It("removes a child that the staged owner dropped from a wholly declared object", func() { + owner := readThenRace(MockStatus{Summary: &MockStatusSummary{Ready: 1, Total: 3}}, nil) + + owner.Status.Summary = &MockStatusSummary{Ready: 2} // total cleared, so omitempty drops it + + Expect(FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("summary"), + )).To(Succeed()) + + Expect(persisted(owner).Status.Summary).To(Equal(&MockStatusSummary{Ready: 2})) + }) + + It("removes a dropped child against the server's version after a conflict", func() { + owner := readThenRace( + MockStatus{Summary: &MockStatusSummary{Ready: 1}}, + func(other *MockOperatorCRD) { other.Status.Summary.Total = 4 }, + ) + + owner.Status.Summary = &MockStatusSummary{Ready: 2} + + Expect(FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("summary"), + )).To(Succeed()) + + Expect(persisted(owner).Status.Summary).To(Equal(&MockStatusSummary{Ready: 2}), + "the declared object is the staged one, so the other writer's total is removed") + }) + It("removes a declared field that the staged owner cleared", func() { owner := readThenRace(MockStatus{Phase: "Pending", ObservedGeneration: 1}, func(other *MockOperatorCRD) { other.Status.ObservedGeneration = 2 diff --git a/pkg/component/status_patch.go b/pkg/component/status_patch.go index f6628c80..775cb0ed 100644 --- a/pkg/component/status_patch.go +++ b/pkg/component/status_patch.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "maps" "slices" "strings" @@ -41,10 +42,12 @@ const conditionsMarker = "ocf.sourcehawk.io/flush-status-conditions-marker" // separated by dots, for example "observedGeneration" or "summary.ready". // Paths are relative to status: write "phase", not "status.phase". A declared // field takes the staged owner's value, and a declared field that the staged -// owner does not carry is removed on the server. Calling it with no paths -// declares that the flush owns no non-condition status field, as for a -// controller that writes only conditions. Paths from several options are -// combined. +// owner does not carry is removed on the server. A declared object is owned +// whole: a child that the staged owner does not carry is removed too. +// +// Calling it with no paths declares that the flush owns no non-condition status +// field, as for a controller that writes only conditions. Paths from several +// options are combined. // // FlushStatus returns an error without writing when a path has an empty // segment, names the conditions, contains them, or lies below them; condition @@ -83,9 +86,34 @@ func patchOwnedStatus( return invalid(err) } - // Build the first body before any write, so a path that cannot be read - // from the staged owner fails without touching the server. - body, err := ownedStatusPatch(rec.Owner, paths, conditionsPath) + staged, err := statusContent(rec.Owner) + if err != nil { + return invalid(err) + } + + // Read every declared path before any request, so a path that cannot be + // read from the staged owner fails without touching the server. + declaresObject := false + for _, path := range paths { + value, err := valueAt(staged, path) + if err != nil { + return invalid(fmt.Errorf("owned status field %q: %w", strings.Join(path, "."), err)) + } + if _, ok := value.(map[string]any); ok { + declaresObject = true + } + } + + // A merge patch merges objects, so a declared object needs the server's + // version of it to send null for the children the staged owner dropped. + var server OperatorCRD + if declaresObject { + if server, err = refreshUnownedConditions(ctx, rec, ownedConditions); err != nil { + return err + } + } + + body, err := ownedStatusPatch(rec.Owner, server, paths, conditionsPath) if err != nil { return invalid(err) } @@ -96,12 +124,13 @@ func patchOwnedStatus( return patchErr } - if refreshErr := refreshUnownedConditions(ctx, rec, ownedConditions); refreshErr != nil { + var refreshErr error + if server, refreshErr = refreshUnownedConditions(ctx, rec, ownedConditions); refreshErr != nil { return refreshErr } var buildErr error - if body, buildErr = ownedStatusPatch(rec.Owner, paths, conditionsPath); buildErr != nil { + if body, buildErr = ownedStatusPatch(rec.Owner, server, paths, conditionsPath); buildErr != nil { return buildErr } return patchErr @@ -181,29 +210,33 @@ func isPrefix(prefix, path []string) bool { // ownedStatusPatch returns the merge patch body for the staged owner: its // resourceVersion as a precondition, each declared path with the staged value -// or null, and the staged conditions. -func ownedStatusPatch(owner OperatorCRD, paths [][]string, conditionsPath []string) ([]byte, error) { - data, err := json.Marshal(owner) +// or null, and the staged conditions. Pass the server's copy when one is known. +func ownedStatusPatch(owner, server OperatorCRD, paths [][]string, conditionsPath []string) ([]byte, error) { + staged, err := statusContent(owner) if err != nil { - return nil, fmt.Errorf("encoding %T: %w", owner, err) + return nil, err } - // json.Number re-encodes a number exactly as it was, so an int64 is not - // rounded through float64. - decoder := json.NewDecoder(bytes.NewReader(data)) - decoder.UseNumber() - var content map[string]any - if err := decoder.Decode(&content); err != nil { - return nil, fmt.Errorf("decoding %T: %w", owner, err) + var serverStatus map[string]any + if server != nil { + if serverStatus, err = statusContent(server); err != nil { + return nil, err + } } - staged, _ := content["status"].(map[string]any) status := map[string]any{} for _, path := range append(slices.Clone(paths), conditionsPath) { - value, err := stagedValue(staged, path) + value, err := valueAt(staged, path) if err != nil { return nil, fmt.Errorf("owned status field %q: %w", strings.Join(path, "."), err) } + + if object, ok := value.(map[string]any); ok && serverStatus != nil { + // A server value that is not an object is replaced whole, so + // there is nothing to remove. + current, _ := valueAt(serverStatus, path) + value = withRemovals(object, current) + } setPatchValue(status, path, value) } @@ -214,10 +247,31 @@ func ownedStatusPatch(owner OperatorCRD, paths [][]string, conditionsPath []stri return json.Marshal(patch) } -// stagedValue returns the value at path in the staged status, or nil when the -// staged owner does not carry it, which the patch sends as null to remove the -// field. An absent or null parent counts as absent. -func stagedValue(status map[string]any, path []string) (any, error) { +// statusContent returns the status of obj as JSON values, or nil when obj has +// no status. +func statusContent(obj OperatorCRD) (map[string]any, error) { + data, err := json.Marshal(obj) + if err != nil { + return nil, fmt.Errorf("encoding %T: %w", obj, err) + } + + // json.Number re-encodes a number exactly as it was, so an int64 is not + // rounded through float64. + decoder := json.NewDecoder(bytes.NewReader(data)) + decoder.UseNumber() + var content map[string]any + if err := decoder.Decode(&content); err != nil { + return nil, fmt.Errorf("decoding %T: %w", obj, err) + } + + status, _ := content["status"].(map[string]any) + return status, nil +} + +// valueAt returns the value at path in status, or nil when status does not +// carry it, which the patch sends as null to remove the field. An absent or +// null parent counts as absent. +func valueAt(status map[string]any, path []string) (any, error) { var current any = status for i, segment := range path { if current == nil { @@ -233,6 +287,28 @@ func stagedValue(status map[string]any, path []string) (any, error) { return current, nil } +// withRemovals returns staged with null for each key that server holds and +// staged does not, at any depth, so the merge patch removes it. +func withRemovals(staged map[string]any, server any) map[string]any { + serverObject, ok := server.(map[string]any) + if !ok { + return staged + } + + result := maps.Clone(staged) + for key, serverValue := range serverObject { + value, present := result[key] + if !present { + result[key] = nil + continue + } + if child, ok := value.(map[string]any); ok { + result[key] = withRemovals(child, serverValue) + } + } + return result +} + // setPatchValue sets value at path in the patch, creating parent objects. A // parent already set to a non-object value by a shorter declared path covers // the longer one, so the longer path is skipped. diff --git a/plugin/skills/building-components/SKILL.md b/plugin/skills/building-components/SKILL.md index 2f19b3aa..74109f18 100644 --- a/plugin/skills/building-components/SKILL.md +++ b/plugin/skills/building-components/SKILL.md @@ -307,12 +307,12 @@ component.FlushStatus(ctx, recCtx, comps, component.WithOwnedStatusFields("phase ``` With the option, every attempt is a JSON merge patch of the status that holds only the staged `resourceVersion` (as a -precondition), the declared fields (a field the staged owner does not carry is sent as `null` and removed), and the -conditions. An undeclared field is never sent, so declare every field the controller writes, including -`observedGeneration` when it is this controller's. `WithOwnedStatusFields()` with no paths owns no non-condition field. -`FlushStatus` returns an error without writing for an empty segment, a path that overlaps the conditions, or a path -below a non-object value. A misspelled path is not caught: a structural CRD schema prunes the unknown field, so that -part of the write does nothing. +precondition), the declared fields (a field the staged owner does not carry is sent as `null` and removed, and a +declared object is owned whole, so its dropped children are removed too), and the conditions. An undeclared field is +never sent, so declare every field the controller writes, including `observedGeneration` when it is this controller's. +`WithOwnedStatusFields()` with no paths owns no non-condition field. `FlushStatus` returns an error without writing for +an empty segment, a path that overlaps the conditions, or a path below a non-object value. A misspelled path is not +caught: a structural CRD schema prunes the unknown field, so that part of the write does nothing. **Server-side apply is for managed resources, never for the owner's status subresource.** An SSA status patch alongside `FlushStatus` puts two field managers on one conditions list, and because `FlushStatus` writes the full status the diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index 9331f1ec..17d80d27 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -947,7 +947,8 @@ patch holds only: - the staged owner's `metadata.resourceVersion`, as a precondition: the API server rejects the patch with a 409 when the owner changed after the controller read it; - each declared field with the staged owner's value, or `null` when the staged owner does not carry the field, so the - server removes it; + server removes it. A declared object is owned whole: `FlushStatus` reads the server's version of it and sends `null` + for each child that the staged owner does not carry, so a child that `omitempty` dropped is removed too; - the conditions list, with the owned conditions as staged and the unowned conditions as the server holds them. A field that the controller did not declare is never sent, on the first attempt or on a retry, so it cannot overwrite From b9bf47fe3c02cf00126136112b89a96333eef6bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 16 Oct 2026 17:21:27 +0100 Subject: [PATCH 10/13] fix(component): read the server copy without refreshing before the first 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 --- pkg/component/component.go | 46 ++++++++++++++---------- pkg/component/flush_owned_fields_test.go | 39 +++++++++++++++++--- pkg/component/status_patch.go | 4 ++- 3 files changed, 65 insertions(+), 24 deletions(-) diff --git a/pkg/component/component.go b/pkg/component/component.go index dafaf562..9059f4b7 100644 --- a/pkg/component/component.go +++ b/pkg/component/component.go @@ -796,24 +796,8 @@ func ownedConditionTypes(comps []*Component) map[string]struct{} { func refreshUnownedConditions( ctx context.Context, rec ReconcileContext, owned map[string]struct{}, ) (OperatorCRD, error) { - server, ok := rec.Owner.DeepCopyObject().(OperatorCRD) - if !ok { - return nil, fmt.Errorf("failed to deep copy owner of type %T", rec.Owner) - } - // Clear the copy's conditions before the fetch. Decoding a response into a - // non-empty struct does not remove fields the response omits, so an owner - // with no conditions on the server would otherwise leave the staged - // conditions in place here and they would read as the server's. - *server.GetStatusConditions() = nil - - // Read through APIReader when one is set: the manager's Client serves reads - // from the informer cache, which can lag the write that caused the conflict - // and return the same stale resourceVersion, defeating the retry. - reader := client.Reader(rec.Client) - if rec.APIReader != nil { - reader = rec.APIReader - } - if err := reader.Get(ctx, client.ObjectKeyFromObject(rec.Owner), server); err != nil { + server, err := readServerOwner(ctx, rec) + if err != nil { return nil, err } @@ -846,6 +830,32 @@ func refreshUnownedConditions( return server, nil } +// readServerOwner returns the owner as the server holds it, read into a +// separate object so the staged owner is left untouched. +func readServerOwner(ctx context.Context, rec ReconcileContext) (OperatorCRD, error) { + server, ok := rec.Owner.DeepCopyObject().(OperatorCRD) + if !ok { + return nil, fmt.Errorf("failed to deep copy owner of type %T", rec.Owner) + } + // Clear the copy's conditions before the fetch. Decoding a response into a + // non-empty struct does not remove fields the response omits, so an owner + // with no conditions on the server would otherwise leave the staged + // conditions in place here and they would read as the server's. + *server.GetStatusConditions() = nil + + // Read through APIReader when one is set: the manager's Client serves reads + // from the informer cache, which can lag the write that caused the conflict + // and return the same stale resourceVersion, defeating the retry. + reader := client.Reader(rec.Client) + if rec.APIReader != nil { + reader = rec.APIReader + } + if err := reader.Get(ctx, client.ObjectKeyFromObject(rec.Owner), server); err != nil { + return nil, err + } + return server, nil +} + // replaceStatusCondition sets cond by type, taking its value verbatim. // // meta.SetStatusCondition is deliberately not used here: it preserves the diff --git a/pkg/component/flush_owned_fields_test.go b/pkg/component/flush_owned_fields_test.go index d2caa61a..cacf4682 100644 --- a/pkg/component/flush_owned_fields_test.go +++ b/pkg/component/flush_owned_fields_test.go @@ -148,21 +148,45 @@ var _ = Describe("FlushStatus with WithOwnedStatusFields", func() { }) It("removes a dropped child against the server's version after a conflict", func() { - owner := readThenRace( - MockStatus{Summary: &MockStatusSummary{Ready: 1}}, - func(other *MockOperatorCRD) { other.Status.Summary.Total = 4 }, - ) + owner := readThenRace(MockStatus{Summary: &MockStatusSummary{Ready: 1}}, nil) + // The competing write lands after FlushStatus read the server's copy + // and before its first patch, so that patch conflicts and the retry + // must compute the removals against the newer copy. + recording := &patchRecordingClient{Client: k8sClient, beforeFirstPatch: func() { + other := &MockOperatorCRD{} + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(owner), other)).To(Succeed()) + other.Status.Summary.Total = 4 + Expect(k8sClient.Status().Update(ctx, other)).To(Succeed()) + }} owner.Status.Summary = &MockStatusSummary{Ready: 2} Expect(FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("summary"), + ctx, ReconcileContext{Client: recording, Owner: owner}, nil, WithOwnedStatusFields("summary"), )).To(Succeed()) + Expect(recording.bodies).To(HaveLen(2), "the first patch must conflict and the retry must succeed") Expect(persisted(owner).Status.Summary).To(Equal(&MockStatusSummary{Ready: 2}), "the declared object is the staged one, so the other writer's total is removed") }) + It("keeps a staged unowned condition when a declared object needs the server's copy", func() { + owner := readThenRace(MockStatus{Summary: &MockStatusSummary{Ready: 1}}, nil) + + owner.Status.Summary = &MockStatusSummary{Ready: 2} + // An owner-level aggregate, owned by no component and not yet on the server. + meta.SetStatusCondition(&owner.Status.Conditions, metav1.Condition{ + Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllReady", + }) + + Expect(FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("summary"), + )).To(Succeed()) + + Expect(meta.FindStatusCondition(persisted(owner).Status.Conditions, "Ready")).NotTo(BeNil(), + "without a conflict the staged conditions are written as staged") + }) + It("removes a declared field that the staged owner cleared", func() { owner := readThenRace(MockStatus{Phase: "Pending", ObservedGeneration: 1}, func(other *MockOperatorCRD) { other.Status.ObservedGeneration = 2 @@ -324,6 +348,8 @@ func TestParseOwnedStatusPaths(t *testing.T) { type patchRecordingClient struct { client.Client bodies [][]byte + // beforeFirstPatch, when set, runs once before the first patch is forwarded. + beforeFirstPatch func() } func (c *patchRecordingClient) Status() client.SubResourceWriter { @@ -342,6 +368,9 @@ func (w *patchRecordingStatusWriter) Patch( if err != nil { return err } + if w.parent.beforeFirstPatch != nil && len(w.parent.bodies) == 0 { + w.parent.beforeFirstPatch() + } w.parent.bodies = append(w.parent.bodies, body) return w.SubResourceWriter.Patch(ctx, obj, patch, opts...) } diff --git a/pkg/component/status_patch.go b/pkg/component/status_patch.go index 775cb0ed..90038d78 100644 --- a/pkg/component/status_patch.go +++ b/pkg/component/status_patch.go @@ -106,9 +106,11 @@ func patchOwnedStatus( // A merge patch merges objects, so a declared object needs the server's // version of it to send null for the children the staged owner dropped. + // Only read it here: refreshing would revert staged unowned conditions + // although nothing conflicted. var server OperatorCRD if declaresObject { - if server, err = refreshUnownedConditions(ctx, rec, ownedConditions); err != nil { + if server, err = readServerOwner(ctx, rec); err != nil { return err } } From 6f15e269d4e015edec41f282e2903611a4e4d770 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 16 Oct 2026 17:31:59 +0100 Subject: [PATCH 11/13] fix(component): build the owned status patch against the stored status 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 --- docs/component.md | 17 +-- pkg/component/component.go | 53 ++++---- pkg/component/flush_owned_fields_test.go | 85 +++++++++++++ pkg/component/status_patch.go | 116 ++++++++++++------ plugin/skills/building-components/SKILL.md | 15 +-- .../references/component.md | 17 +-- 6 files changed, 214 insertions(+), 89 deletions(-) diff --git a/docs/component.md b/docs/component.md index 7a24a334..b4e0427d 100644 --- a/docs/component.md +++ b/docs/component.md @@ -943,16 +943,17 @@ patch holds only: - the staged owner's `metadata.resourceVersion`, as a precondition: the API server rejects the patch with a 409 when the owner changed after the controller read it; -- each declared field with the staged owner's value, or `null` when the staged owner does not carry the field, so the - server removes it. A declared object is owned whole: `FlushStatus` reads the server's version of it and sends `null` - for each child that the staged owner does not carry, so a child that `omitempty` dropped is removed too; +- each declared field with the staged owner's value, or `null` when the staged owner does not carry the field but the + server does, so the server removes it. A declared object is owned whole: the patch sends `null` for each child that + the server stores and the staged owner does not carry, so a child that `omitempty` dropped is removed too; - the conditions list, with the owned conditions as staged and the unowned conditions as the server holds them. -A field that the controller did not declare is never sent, on the first attempt or on a retry, so it cannot overwrite -another writer's value. On a 409, `FlushStatus` refreshes the `resourceVersion` and the unowned conditions from the -server, as described above, builds the patch again, and retries. After a successful write, the controller-runtime client -decodes the server's response into the owner, so a value staged in a field that you did not declare does not survive in -memory. +To build the patch, each attempt first reads the owner's status through `APIReader` when it is set, and through `Client` +otherwise. A cache-backed `Client` can return a stale status, so set `APIReader`. A field that the controller did not +declare is never sent, on the first attempt or on a retry, so it cannot overwrite another writer's value. On a 409, +`FlushStatus` refreshes the `resourceVersion` and the unowned conditions from the server, as described above, builds the +patch again, and retries. After a successful write, the controller-runtime client decodes the server's response into the +owner, so a value staged in a field that you did not declare does not survive in memory. Name each field by its JSON name under `status`, with dots between nested fields: `"observedGeneration"`, `"summary.ready"`. Do not include the `status.` prefix. `WithOwnedStatusFields()` with no paths declares that the flush diff --git a/pkg/component/component.go b/pkg/component/component.go index 9059f4b7..7537a3b0 100644 --- a/pkg/component/component.go +++ b/pkg/component/component.go @@ -750,7 +750,7 @@ func FlushStatus(ctx context.Context, rec ReconcileContext, comps []*Component, if !apierrors.IsConflict(updateErr) { return updateErr } - if _, refreshErr := refreshUnownedConditions(ctx, rec, owned); refreshErr != nil { + if refreshErr := refreshUnownedConditions(ctx, rec, owned); refreshErr != nil { return refreshErr } return updateErr @@ -792,13 +792,19 @@ func ownedConditionTypes(comps []*Component) map[string]struct{} { // resourceVersion, so the retry targets the live object, and the value of every // condition type this flush does not own, so a concurrent update by another // writer is not overwritten by the copy this controller happens to be holding. -// It returns the server's copy it read. -func refreshUnownedConditions( - ctx context.Context, rec ReconcileContext, owned map[string]struct{}, -) (OperatorCRD, error) { - server, err := readServerOwner(ctx, rec) - if err != nil { - return nil, err +func refreshUnownedConditions(ctx context.Context, rec ReconcileContext, owned map[string]struct{}) error { + server, ok := rec.Owner.DeepCopyObject().(OperatorCRD) + if !ok { + return fmt.Errorf("failed to deep copy owner of type %T", rec.Owner) + } + // Clear the copy's conditions before the fetch. Decoding a response into a + // non-empty struct does not remove fields the response omits, so an owner + // with no conditions on the server would otherwise leave the staged + // conditions in place here and they would read as the server's. + *server.GetStatusConditions() = nil + + if err := liveReader(rec).Get(ctx, client.ObjectKeyFromObject(rec.Owner), server); err != nil { + return err } rec.Owner.SetResourceVersion(server.GetResourceVersion()) @@ -827,33 +833,18 @@ func refreshUnownedConditions( } replaceStatusCondition(staged, cond) } - return server, nil + return nil } -// readServerOwner returns the owner as the server holds it, read into a -// separate object so the staged owner is left untouched. -func readServerOwner(ctx context.Context, rec ReconcileContext) (OperatorCRD, error) { - server, ok := rec.Owner.DeepCopyObject().(OperatorCRD) - if !ok { - return nil, fmt.Errorf("failed to deep copy owner of type %T", rec.Owner) - } - // Clear the copy's conditions before the fetch. Decoding a response into a - // non-empty struct does not remove fields the response omits, so an owner - // with no conditions on the server would otherwise leave the staged - // conditions in place here and they would read as the server's. - *server.GetStatusConditions() = nil - - // Read through APIReader when one is set: the manager's Client serves reads - // from the informer cache, which can lag the write that caused the conflict - // and return the same stale resourceVersion, defeating the retry. - reader := client.Reader(rec.Client) +// liveReader returns rec.APIReader when one is set, and rec.Client otherwise. +// The manager's Client serves reads from the informer cache, which can lag the +// write that caused a conflict and return the same stale resourceVersion, +// defeating the retry. +func liveReader(rec ReconcileContext) client.Reader { if rec.APIReader != nil { - reader = rec.APIReader - } - if err := reader.Get(ctx, client.ObjectKeyFromObject(rec.Owner), server); err != nil { - return nil, err + return rec.APIReader } - return server, nil + return rec.Client } // replaceStatusCondition sets cond by type, taking its value verbatim. diff --git a/pkg/component/flush_owned_fields_test.go b/pkg/component/flush_owned_fields_test.go index cacf4682..07c8f68e 100644 --- a/pkg/component/flush_owned_fields_test.go +++ b/pkg/component/flush_owned_fields_test.go @@ -11,7 +11,9 @@ import ( "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -218,6 +220,33 @@ var _ = Describe("FlushStatus with WithOwnedStatusFields", func() { Expect(persisted(owner).Status.Summary).To(Equal(&MockStatusSummary{Ready: 2, Total: 4})) }) + It("removes an explicitly stored zero child of a wholly declared object", func() { + owner := readThenRace(MockStatus{Summary: &MockStatusSummary{Ready: 1}}, nil) + // A typed write drops total: 0 through omitempty, so store it raw. + Expect(k8sClient.Status().Patch(ctx, owner, client.RawPatch( + types.MergePatchType, []byte(`{"status":{"summary":{"ready":1,"total":0}}}`), + ))).To(Succeed()) + Expect(rawStatus(ctx, owner)["summary"]).To(HaveKey("total")) + + owner.Status.Summary = &MockStatusSummary{Ready: 2} + + Expect(FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("summary"), + )).To(Succeed()) + + Expect(rawStatus(ctx, owner)["summary"]).To(Equal(map[string]any{"ready": int64(2)})) + }) + + It("leaves an absent parent absent when a declared child has nothing to remove", func() { + owner := readThenRace(MockStatus{Phase: "Pending"}, nil) + + Expect(FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("summary.ready"), + )).To(Succeed()) + + Expect(rawStatus(ctx, owner)).NotTo(HaveKey("summary")) + }) + It("lets owned conditions win and unowned conditions follow the server on a retry", func() { owner := readThenRace( MockStatus{Conditions: []metav1.Condition{ @@ -344,6 +373,62 @@ func TestParseOwnedStatusPaths(t *testing.T) { } } +// rawStatus reads the owner's status as the server stores it, where a typed +// read cannot tell a stored zero from an absent field. +func rawStatus(ctx context.Context, owner *MockOperatorCRD) map[string]any { + GinkgoHelper() + live := &unstructured.Unstructured{} + live.SetGroupVersionKind(GroupVersion.WithKind("MockOperatorCRD")) + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(owner), live)).To(Succeed()) + status, _ := live.Object["status"].(map[string]any) + return status +} + +func TestOwnedStatusPatch(t *testing.T) { + owner := &MockOperatorCRD{Status: MockStatus{Summary: &MockStatusSummary{Ready: 2}}} + conditionsPath := []string{"conditions"} + + statusOf := func(t *testing.T, body []byte) map[string]any { + t.Helper() + var patch map[string]any + require.NoError(t, json.Unmarshal(body, &patch)) + status, _ := patch["status"].(map[string]any) + return status + } + + t.Run("does not depend on the order of overlapping declarations", func(t *testing.T) { + server := map[string]any{"summary": map[string]any{"ready": int64(1)}} + forward, err := parseOwnedStatusPaths([]string{"summary", "summary.details.ready"}, conditionsPath) + require.NoError(t, err) + backward, err := parseOwnedStatusPaths([]string{"summary.details.ready", "summary"}, conditionsPath) + require.NoError(t, err) + + forwardBody, err := ownedStatusPatch(owner, server, forward, conditionsPath) + require.NoError(t, err) + backwardBody, err := ownedStatusPatch(owner, server, backward, conditionsPath) + require.NoError(t, err) + + assert.Equal(t, statusOf(t, forwardBody), statusOf(t, backwardBody)) + assert.Equal(t, map[string]any{"ready": float64(2)}, statusOf(t, forwardBody)["summary"]) + }) + + for name, server := range map[string]map[string]any{ + "absent": {}, + "null": {"summary": nil}, + } { + t.Run("sends nothing to remove below a parent the server holds as "+name, func(t *testing.T) { + stagedWithout := &MockOperatorCRD{} + paths, err := parseOwnedStatusPaths([]string{"summary.ready"}, conditionsPath) + require.NoError(t, err) + + body, err := ownedStatusPatch(stagedWithout, server, paths, conditionsPath) + require.NoError(t, err) + + assert.NotContains(t, statusOf(t, body), "summary") + }) + } +} + // patchRecordingClient records the body of every status patch it forwards. type patchRecordingClient struct { client.Client diff --git a/pkg/component/status_patch.go b/pkg/component/status_patch.go index 90038d78..80d032c1 100644 --- a/pkg/component/status_patch.go +++ b/pkg/component/status_patch.go @@ -12,9 +12,11 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/apiutil" ) // FlushOption configures a [FlushStatus] call. @@ -36,7 +38,9 @@ const conditionsMarker = "ocf.sourcehawk.io/flush-status-conditions-marker" // flush owns, for an owner whose status is shared with other writers. It turns // every write of [FlushStatus] into a JSON merge patch of the status that holds // only the declared fields and the conditions, so a field that another writer -// owns is never sent, on the first attempt or on a retry. +// owns is never sent, on the first attempt or on a retry. 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. // // Each path names a field under status by its JSON name, with nested fields // separated by dots, for example "observedGeneration" or "summary.ready". @@ -93,26 +97,15 @@ func patchOwnedStatus( // Read every declared path before any request, so a path that cannot be // read from the staged owner fails without touching the server. - declaresObject := false for _, path := range paths { - value, err := valueAt(staged, path) - if err != nil { + if _, err := valueAt(staged, path); err != nil { return invalid(fmt.Errorf("owned status field %q: %w", strings.Join(path, "."), err)) } - if _, ok := value.(map[string]any); ok { - declaresObject = true - } } - // A merge patch merges objects, so a declared object needs the server's - // version of it to send null for the children the staged owner dropped. - // Only read it here: refreshing would revert staged unowned conditions - // although nothing conflicted. - var server OperatorCRD - if declaresObject { - if server, err = readServerOwner(ctx, rec); err != nil { - return err - } + server, err := readServerStatus(ctx, rec) + if err != nil { + return err } body, err := ownedStatusPatch(rec.Owner, server, paths, conditionsPath) @@ -126,11 +119,17 @@ func patchOwnedStatus( return patchErr } - var refreshErr error - if server, refreshErr = refreshUnownedConditions(ctx, rec, ownedConditions); refreshErr != nil { + if refreshErr := refreshUnownedConditions(ctx, rec, ownedConditions); refreshErr != nil { return refreshErr } + // Read the status after the refresh, so it is at least as new as the + // resourceVersion the patch is guarded by. + var readErr error + if server, readErr = readServerStatus(ctx, rec); readErr != nil { + return readErr + } + var buildErr error if body, buildErr = ownedStatusPatch(rec.Owner, server, paths, conditionsPath); buildErr != nil { return buildErr @@ -139,6 +138,25 @@ func patchOwnedStatus( }) } +// readServerStatus returns the status that the server stores for the owner. +// It reads into an unstructured object, because a typed read drops a stored +// zero value that omitempty skips, and the patch must still remove it. +func readServerStatus(ctx context.Context, rec ReconcileContext) (map[string]any, error) { + gvk, err := apiutil.GVKForObject(rec.Owner, rec.Client.Scheme()) + if err != nil { + return nil, err + } + + live := &unstructured.Unstructured{} + live.SetGroupVersionKind(gvk) + if err := liveReader(rec).Get(ctx, client.ObjectKeyFromObject(rec.Owner), live); err != nil { + return nil, err + } + + status, _ := live.Object["status"].(map[string]any) + return status, nil +} + // conditionsJSONPath returns the JSON path under status of the list that // GetStatusConditions returns. OperatorCRD does not fix that list's JSON name // or depth, so a marker condition is staged for a moment and found in the @@ -203,29 +221,35 @@ func parseOwnedStatusPaths(paths []string, conditionsPath []string) ([][]string, } parsed = append(parsed, segments) } - return parsed, nil + + // A declared ancestor writes its whole subtree. A descendant written into + // it as well would make the patch depend on the order of the paths. + var kept [][]string + for i, path := range parsed { + covered := slices.ContainsFunc(parsed[:i], func(other []string) bool { return isPrefix(other, path) }) || + slices.ContainsFunc(parsed[i+1:], func(other []string) bool { + return len(other) < len(path) && isPrefix(other, path) + }) + if !covered { + kept = append(kept, path) + } + } + return kept, nil } func isPrefix(prefix, path []string) bool { return len(prefix) <= len(path) && slices.Equal(prefix, path[:len(prefix)]) } -// ownedStatusPatch returns the merge patch body for the staged owner: its -// resourceVersion as a precondition, each declared path with the staged value -// or null, and the staged conditions. Pass the server's copy when one is known. -func ownedStatusPatch(owner, server OperatorCRD, paths [][]string, conditionsPath []string) ([]byte, error) { +// ownedStatusPatch returns the merge patch body that takes each declared path +// and the conditions from the server's status to the staged owner's, with the +// staged resourceVersion as a precondition. +func ownedStatusPatch(owner OperatorCRD, server map[string]any, paths [][]string, conditionsPath []string) ([]byte, error) { staged, err := statusContent(owner) if err != nil { return nil, err } - var serverStatus map[string]any - if server != nil { - if serverStatus, err = statusContent(server); err != nil { - return nil, err - } - } - status := map[string]any{} for _, path := range append(slices.Clone(paths), conditionsPath) { value, err := valueAt(staged, path) @@ -233,11 +257,17 @@ func ownedStatusPatch(owner, server OperatorCRD, paths [][]string, conditionsPat return nil, fmt.Errorf("owned status field %q: %w", strings.Join(path, "."), err) } - if object, ok := value.(map[string]any); ok && serverStatus != nil { - // A server value that is not an object is replaced whole, so - // there is nothing to remove. - current, _ := valueAt(serverStatus, path) - value = withRemovals(object, current) + // A server value below a non-object parent counts as absent. + current, _ := valueAt(server, path) + switch typed := value.(type) { + case nil: + // A null below a parent the server does not hold as an object + // would create that parent, so remove only what is there. + if !holds(server, path) { + continue + } + case map[string]any: + value = withRemovals(typed, current) } setPatchValue(status, path, value) } @@ -249,6 +279,22 @@ func ownedStatusPatch(owner, server OperatorCRD, paths [][]string, conditionsPat return json.Marshal(patch) } +// holds reports whether status has a key at path, below parents that are all +// objects. +func holds(status map[string]any, path []string) bool { + parent, err := valueAt(status, path[:len(path)-1]) + if err != nil { + return false + } + + object, ok := parent.(map[string]any) + if !ok { + return false + } + _, present := object[path[len(path)-1]] + return present +} + // statusContent returns the status of obj as JSON values, or nil when obj has // no status. func statusContent(obj OperatorCRD) (map[string]any, error) { diff --git a/plugin/skills/building-components/SKILL.md b/plugin/skills/building-components/SKILL.md index 74109f18..e7cd44c6 100644 --- a/plugin/skills/building-components/SKILL.md +++ b/plugin/skills/building-components/SKILL.md @@ -306,13 +306,14 @@ dots for nested fields: component.FlushStatus(ctx, recCtx, comps, component.WithOwnedStatusFields("phase")) ``` -With the option, every attempt is a JSON merge patch of the status that holds only the staged `resourceVersion` (as a -precondition), the declared fields (a field the staged owner does not carry is sent as `null` and removed, and a -declared object is owned whole, so its dropped children are removed too), and the conditions. An undeclared field is -never sent, so declare every field the controller writes, including `observedGeneration` when it is this controller's. -`WithOwnedStatusFields()` with no paths owns no non-condition field. `FlushStatus` returns an error without writing for -an empty segment, a path that overlaps the conditions, or a path below a non-object value. A misspelled path is not -caught: a structural CRD schema prunes the unknown field, so that part of the write does nothing. +With the option, every attempt reads the stored status and then sends a JSON merge patch of the status that holds only +the staged `resourceVersion` (as a precondition), the declared fields (a field the staged owner does not carry is sent +as `null` and removed, and a declared object is owned whole, so its dropped children are removed too), and the +conditions. An undeclared field is never sent, so declare every field the controller writes, including +`observedGeneration` when it is this controller's. `WithOwnedStatusFields()` with no paths owns no non-condition field. +`FlushStatus` returns an error without writing for an empty segment, a path that overlaps the conditions, or a path +below a non-object value. A misspelled path is not caught: a structural CRD schema prunes the unknown field, so that +part of the write does nothing. **Server-side apply is for managed resources, never for the owner's status subresource.** An SSA status patch alongside `FlushStatus` puts two field managers on one conditions list, and because `FlushStatus` writes the full status the diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index 17d80d27..9c7a09bb 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -946,16 +946,17 @@ patch holds only: - the staged owner's `metadata.resourceVersion`, as a precondition: the API server rejects the patch with a 409 when the owner changed after the controller read it; -- each declared field with the staged owner's value, or `null` when the staged owner does not carry the field, so the - server removes it. A declared object is owned whole: `FlushStatus` reads the server's version of it and sends `null` - for each child that the staged owner does not carry, so a child that `omitempty` dropped is removed too; +- each declared field with the staged owner's value, or `null` when the staged owner does not carry the field but the + server does, so the server removes it. A declared object is owned whole: the patch sends `null` for each child that + the server stores and the staged owner does not carry, so a child that `omitempty` dropped is removed too; - the conditions list, with the owned conditions as staged and the unowned conditions as the server holds them. -A field that the controller did not declare is never sent, on the first attempt or on a retry, so it cannot overwrite -another writer's value. On a 409, `FlushStatus` refreshes the `resourceVersion` and the unowned conditions from the -server, as described above, builds the patch again, and retries. After a successful write, the controller-runtime client -decodes the server's response into the owner, so a value staged in a field that you did not declare does not survive in -memory. +To build the patch, each attempt first reads the owner's status through `APIReader` when it is set, and through `Client` +otherwise. A cache-backed `Client` can return a stale status, so set `APIReader`. A field that the controller did not +declare is never sent, on the first attempt or on a retry, so it cannot overwrite another writer's value. On a 409, +`FlushStatus` refreshes the `resourceVersion` and the unowned conditions from the server, as described above, builds the +patch again, and retries. After a successful write, the controller-runtime client decodes the server's response into the +owner, so a value staged in a field that you did not declare does not survive in memory. Name each field by its JSON name under `status`, with dots between nested fields: `"observedGeneration"`, `"summary.ready"`. Do not include the `status.` prefix. `WithOwnedStatusFields()` with no paths declares that the flush From fdcf22987ea17717bf6fba140b5a6954172bb58d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 16 Oct 2026 17:37:07 +0100 Subject: [PATCH 12/13] docs(component): say which conditions the first owned status patch sends 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 --- docs/component.md | 4 +++- plugin/skills/building-components/references/component.md | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/component.md b/docs/component.md index b4e0427d..1250fbbf 100644 --- a/docs/component.md +++ b/docs/component.md @@ -946,7 +946,9 @@ patch holds only: - each declared field with the staged owner's value, or `null` when the staged owner does not carry the field but the server does, so the server removes it. A declared object is owned whole: the patch sends `null` for each child that the server stores and the staged owner does not carry, so a child that `omitempty` dropped is removed too; -- the conditions list, with the owned conditions as staged and the unowned conditions as the server holds them. +- the conditions list as staged. On the first attempt that includes any unowned condition the controller staged by hand, + such as an owner-level aggregate; the precondition makes the patch fail if the server changed after the read. After a + 409, the unowned conditions are first refreshed from the server. To build the patch, each attempt first reads the owner's status through `APIReader` when it is set, and through `Client` otherwise. A cache-backed `Client` can return a stale status, so set `APIReader`. A field that the controller did not diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index 9c7a09bb..37ad6abf 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -949,7 +949,9 @@ patch holds only: - each declared field with the staged owner's value, or `null` when the staged owner does not carry the field but the server does, so the server removes it. A declared object is owned whole: the patch sends `null` for each child that the server stores and the staged owner does not carry, so a child that `omitempty` dropped is removed too; -- the conditions list, with the owned conditions as staged and the unowned conditions as the server holds them. +- the conditions list as staged. On the first attempt that includes any unowned condition the controller staged by hand, + such as an owner-level aggregate; the precondition makes the patch fail if the server changed after the read. After a + 409, the unowned conditions are first refreshed from the server. To build the patch, each attempt first reads the owner's status through `APIReader` when it is set, and through `Client` otherwise. A cache-backed `Client` can return a stale status, so set `APIReader`. A field that the controller did not From f971adfc8b864de87cb7e5cb836213683a4edf29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 16 Oct 2026 17:44:57 +0100 Subject: [PATCH 13/13] fix(component): refuse to patch an owner recreated under the same name 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 --- docs/component.md | 4 +++- pkg/component/flush_owned_fields_test.go | 19 +++++++++++++++++++ pkg/component/status_patch.go | 10 ++++++++++ .../references/component.md | 4 +++- 4 files changed, 35 insertions(+), 2 deletions(-) diff --git a/docs/component.md b/docs/component.md index 1250fbbf..402b8aff 100644 --- a/docs/component.md +++ b/docs/component.md @@ -951,7 +951,9 @@ patch holds only: 409, the unowned conditions are first refreshed from the server. To build the patch, each attempt first reads the owner's status through `APIReader` when it is set, and through `Client` -otherwise. A cache-backed `Client` can return a stale status, so set `APIReader`. A field that the controller did not +otherwise. A cache-backed `Client` can return a stale status, so set `APIReader`. When that read finds an owner with +another UID, because the owner was deleted and recreated under the same name after the controller read it, `FlushStatus` +returns an error and does not write, as `Status().Update` would refuse the write. A field that the controller did not declare is never sent, on the first attempt or on a retry, so it cannot overwrite another writer's value. On a 409, `FlushStatus` refreshes the `resourceVersion` and the unowned conditions from the server, as described above, builds the patch again, and retries. After a successful write, the controller-runtime client decodes the server's response into the diff --git a/pkg/component/flush_owned_fields_test.go b/pkg/component/flush_owned_fields_test.go index 07c8f68e..692315bc 100644 --- a/pkg/component/flush_owned_fields_test.go +++ b/pkg/component/flush_owned_fields_test.go @@ -247,6 +247,25 @@ var _ = Describe("FlushStatus with WithOwnedStatusFields", func() { Expect(rawStatus(ctx, owner)).NotTo(HaveKey("summary")) }) + It("does not write into a replacement created under the same name", func() { + owner := readThenRace(MockStatus{Phase: "Pending"}, nil) + + Expect(k8sClient.Delete(ctx, owner.DeepCopy())).To(Succeed()) + replacement := &MockOperatorCRD{ + ObjectMeta: metav1.ObjectMeta{Name: owner.Name, Namespace: owner.Namespace}, + } + Expect(k8sClient.Create(ctx, replacement)).To(Succeed()) + Expect(replacement.UID).NotTo(Equal(owner.UID)) + + owner.Status.Phase = "Accepted" + + Expect(FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("phase"), + )).To(MatchError(ContainSubstring("was replaced"))) + + Expect(persisted(owner).Status.Phase).To(BeEmpty(), "the replacement must stay as it was created") + }) + It("lets owned conditions win and unowned conditions follow the server on a retry", func() { owner := readThenRace( MockStatus{Conditions: []metav1.Condition{ diff --git a/pkg/component/status_patch.go b/pkg/component/status_patch.go index 80d032c1..8369d26b 100644 --- a/pkg/component/status_patch.go +++ b/pkg/component/status_patch.go @@ -153,6 +153,16 @@ func readServerStatus(ctx context.Context, rec ReconcileContext) (map[string]any return nil, err } + // Status().Update sends the uid and the API server rejects a mismatch. A + // merge patch does not, so a retry would patch an owner that was deleted + // and recreated under the same name, taking its resourceVersion. + if staged := rec.Owner.GetUID(); staged != "" && live.GetUID() != staged { + return nil, fmt.Errorf( + "owner %s was replaced: the server holds uid %s, the staged owner has uid %s", + client.ObjectKeyFromObject(rec.Owner), live.GetUID(), staged, + ) + } + status, _ := live.Object["status"].(map[string]any) return status, nil } diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index 37ad6abf..0c1de1f1 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -954,7 +954,9 @@ patch holds only: 409, the unowned conditions are first refreshed from the server. To build the patch, each attempt first reads the owner's status through `APIReader` when it is set, and through `Client` -otherwise. A cache-backed `Client` can return a stale status, so set `APIReader`. A field that the controller did not +otherwise. A cache-backed `Client` can return a stale status, so set `APIReader`. When that read finds an owner with +another UID, because the owner was deleted and recreated under the same name after the controller read it, `FlushStatus` +returns an error and does not write, as `Status().Update` would refuse the write. A field that the controller did not declare is never sent, on the first attempt or on a retry, so it cannot overwrite another writer's value. On a 409, `FlushStatus` refreshes the `resourceVersion` and the unowned conditions from the server, as described above, builds the patch again, and retries. After a successful write, the controller-runtime client decodes the server's response into the