diff --git a/spinifex/domains/ec2/instance/state/record_codec.go b/spinifex/domains/ec2/instance/state/record_codec.go index 57b922c03..288d11cf1 100644 --- a/spinifex/domains/ec2/instance/state/record_codec.go +++ b/spinifex/domains/ec2/instance/state/record_codec.go @@ -9,8 +9,10 @@ import ( "errors" "fmt" "io" + "maps" "math" "reflect" + "slices" "strconv" "strings" "sync" @@ -43,6 +45,7 @@ var ( ErrRecordFormatOutOfRange = errors.New("instance record format is below 1") ErrRecordFormatAhead = errors.New("instance record format is newer than this binary can write") ErrRecordSectionsNeedFormat = errors.New("assignment and observation need record format 2") + ErrRecordExtensionConflict = errors.New("instance record change would drop an unknown member inside an array") ) // AssignmentReason says why an assignment changed. @@ -190,7 +193,7 @@ func DecodeTransitionRecord(data []byte) (*TransitionRecord, error) { // Encode writes only the typed paths that differ from the decoded snapshot // over the input, keeping unknown members. A changed array is replaced whole, -// losing unknown members in its elements. It never adds a format marker. +// and refused if its read elements carry unknown members. It adds no marker. func (r *TransitionRecord) Encode() ([]byte, error) { if r.readOnly { return nil, fmt.Errorf("%w: format %d", ErrRecordFormatAhead, r.format) @@ -206,7 +209,11 @@ func (r *TransitionRecord) Encode() ([]byte, error) { if raw == nil { raw, snapshot = map[string]any{}, map[string]any{} } - return json.Marshal(mergeChanged(transitionSchema(), raw, snapshot, current)) + merged, err := mergeChanged(transitionSchema(), raw, snapshot, current, "$") + if err != nil { + return nil, err + } + return json.Marshal(merged) } // typedTree re-encodes the typed fields that the record's format covers. @@ -238,15 +245,20 @@ func (r *TransitionRecord) typedTree() (map[string]any, error) { // mergeChanged applies the paths where current differs from snapshot onto // raw. A struct merges per member and keeps members it does not know; a map // merges per entry; any other differing value is replaced by current's. -func mergeChanged(s *fieldSchema, raw, snapshot, current any) any { +func mergeChanged(s *fieldSchema, raw, snapshot, current any, path string) (any, error) { if reflect.DeepEqual(snapshot, current) { - return raw + return raw, nil + } + if rawArr, ok := raw.([]any); ok && s != nil { + if at, found := unknownMember(s, rawArr, path); found { + return nil, fmt.Errorf("%w: changed array %s would drop unknown member %s", ErrRecordExtensionConflict, path, at) + } } rawObj, rawIsObj := raw.(map[string]any) snapObj, snapIsObj := snapshot.(map[string]any) curObj, curIsObj := current.(map[string]any) if s == nil || (s.fields == nil && !s.isMap) || !rawIsObj || !snapIsObj || !curIsObj { - return current + return current, nil } out := make(map[string]any, len(rawObj)) for k, v := range rawObj { @@ -268,14 +280,49 @@ func mergeChanged(s *fieldSchema, raw, snapshot, current any) any { } switch { case inSnap && inRaw: - out[k] = mergeChanged(child, rawV, snapV, curV) + merged, err := mergeChanged(child, rawV, snapV, curV, path+"."+k) + if err != nil { + return nil, err + } + out[k] = merged case inSnap && !s.isMap && reflect.DeepEqual(snapV, curV): // A zero member the input left out stays out. default: out[k] = curV } } - return out + return out, nil +} + +// unknownMember finds the first member of v, at any depth, that schema s does +// not know. An opaque value (nil schema) is not inspected. +func unknownMember(s *fieldSchema, v any, path string) (string, bool) { + if s == nil { + return "", false + } + switch v := v.(type) { + case []any: + for i, e := range v { + if at, found := unknownMember(s.elem, e, path+"["+strconv.Itoa(i)+"]"); found { + return at, true + } + } + case map[string]any: + for _, k := range slices.Sorted(maps.Keys(v)) { + child := s.elem + if !s.isMap { + known, ok := s.fields[k] + if !ok { + return path + "." + k, true + } + child = known + } + if at, found := unknownMember(child, v[k], path+"."+k); found { + return at, true + } + } + } + return "", false } // recordFormat reads the marker: absent is 1, an integer above 2 is returned diff --git a/spinifex/domains/ec2/instance/state/record_codec_test.go b/spinifex/domains/ec2/instance/state/record_codec_test.go index 575826b39..c1823d21d 100644 --- a/spinifex/domains/ec2/instance/state/record_codec_test.go +++ b/spinifex/domains/ec2/instance/state/record_codec_test.go @@ -230,8 +230,8 @@ func TestTransitionRecord_ExplicitMutationShadowsRawForm(t *testing.T) { } // An unchanged array is written back as read, unknown element members and -// all. A changed array is the writer's value: unknown members inside its -// elements are lost. That is a documented limitation, pinned here. +// all. A changed array is replaced whole, unless a read element carries an +// unknown member at any depth: then Encode refuses rather than drop it. func TestTransitionRecord_Arrays(t *testing.T) { t.Run("unchanged array keeps unknown element members", func(t *testing.T) { r := decode(t, goldenV1) @@ -240,13 +240,59 @@ func TestTransitionRecord_Arrays(t *testing.T) { refs, _ := member(t, out, "metadata", "owner_refs") assert.JSONEq(t, `[{"kind":"cluster","name":"c-1","future_ref":"r-1"}]`, string(refs)) }) - t.Run("changed array is replaced and loses unknown element members", func(t *testing.T) { - r := decode(t, goldenV1) + t.Run("changed array with known members only is replaced whole", func(t *testing.T) { + doc := `{"metadata":{"name":"i-1","owner_refs":[{"kind":"cluster","name":"c-1"}]},"future":[{"x":1}]}` + r := decode(t, doc) r.Record.Metadata.OwnerRefs = append(r.Record.Metadata.OwnerRefs, resource.OwnerRef{Kind: "asg", Name: "a-1"}) - out := encode(t, r) - refs, _ := member(t, out, "metadata", "owner_refs") - assert.JSONEq(t, `[{"kind":"cluster","name":"c-1"},{"kind":"asg","name":"a-1"}]`, string(refs)) + want := edit(t, doc, map[string]any{"metadata.owner_refs": []any{ + map[string]any{"kind": "cluster", "name": "c-1"}, + map[string]any{"kind": "asg", "name": "a-1"}, + }}) + assert.Equal(t, want, canonical(t, encode(t, r))) }) + t.Run("changed array of scalars is replaced whole", func(t *testing.T) { + doc := `{"metadata":{"name":"i-1","finalizers":["eni","volume"]}}` + r := decode(t, doc) + r.Record.Metadata.Finalizers = []string{"volume"} + want := edit(t, doc, map[string]any{"metadata.finalizers": []any{"volume"}}) + assert.Equal(t, want, canonical(t, encode(t, r))) + }) + + conflicts := []struct { + name, doc, array, member string + mutate func(*state.TransitionRecord) + }{ + { + "unknown member in an element", + goldenV1, + "$.metadata.owner_refs", + "$.metadata.owner_refs[0].future_ref", + func(r *state.TransitionRecord) { + r.Record.Metadata.OwnerRefs = append(r.Record.Metadata.OwnerRefs, resource.OwnerRef{Kind: "asg", Name: "a-1"}) + }, + }, + { + "unknown member one level deeper in an element", + `{"status":{"instance":{"NetworkInterfaces":[{"NetworkInterfaceId":"eni-1",` + + `"Attachment":{"DeviceIndex":0,"FutureAttach":"x"}}]}}}`, + "$.status.instance.NetworkInterfaces", + "$.status.instance.NetworkInterfaces[0].Attachment.FutureAttach", + func(r *state.TransitionRecord) { + r.Record.Status.Instance.NetworkInterfaces[0].NetworkInterfaceId = aws.String("eni-2") + }, + }, + } + for _, tc := range conflicts { + t.Run("changed array refused: "+tc.name, func(t *testing.T) { + r := decode(t, tc.doc) + tc.mutate(r) + out, err := r.Encode() + require.ErrorIs(t, err, state.ErrRecordExtensionConflict) + assert.Contains(t, err.Error(), "changed array "+tc.array+" ") + assert.Contains(t, err.Error(), tc.member) + assert.Nil(t, out) + }) + } } func TestTransitionRecord_V2DecodesSectionsAndKeepsLegacyMirror(t *testing.T) { @@ -451,6 +497,8 @@ func TestTransitionRecord_CapableWriterPreservesWhatPlainTypedRoundTripStrips(t } } +// An unmutated record cannot raise an extension conflict: that needs a +// changed array, so every accepted, writable input must round-trip. func FuzzTransitionRecordRoundTrip(f *testing.F) { for _, seed := range []string{ goldenV1, goldenV2,