diff --git a/internal/cli/checkpoint.go b/internal/cli/checkpoint.go index d3d7fdf7..7edf2c63 100644 --- a/internal/cli/checkpoint.go +++ b/internal/cli/checkpoint.go @@ -165,25 +165,16 @@ func resetCommand(args []string, stdout, stderr io.Writer) int { } endpoint := "http://" + *addr - recorded, err := readRecordedCheckpoint(*addr) + // The name chooses the record, and an empty one means the last taken. + // Before the audit this read one file per instance, so a second + // checkpoint made the first unresettable. + recorded, err := readRecordedCheckpoint(*addr, asked) if err != nil { fmt.Fprintf(stderr, "feint reset: %v\n", err) fmt.Fprintf(stderr, " Take one first: feint checkpoint --addr %s\n", *addr) return exitError } name := recorded.Name - if asked != "" { - name = asked - } - if name != recorded.Name { - // Refused rather than guessed. The recorded state is what a verdict - // compares against, and comparing one checkpoint's emulator state to - // another's observation would report differences that are not the - // reset's fault. - fmt.Fprintf(stderr, "feint reset: this instance recorded %q, not %q, so a verdict on %q would compare the wrong states\n", - recorded.Name, name, name) - return exitError - } answer, err := callCheckpoint(http.MethodPost, endpoint+"/_feint/reset/"+name) if err != nil { @@ -306,18 +297,45 @@ func callCheckpoint(method, url string) (map[string]any, error) { return body, nil } -// checkpointPath is where the observed state lives: beside the instance -// record, because it belongs to that emulator and goes when it does. -func checkpointPath(addr string) (string, error) { +// checkpointPath is where one checkpoint's observed state lives: beside the +// instance record, because it belongs to that emulator and goes when it does. +// +// Indexed by NAME, which the first version was not. An external audit caught +// it: the emulator holds any number of named checkpoints and the CLI wrote a +// single checkpoint.json, so `checkpoint A` followed by `checkpoint B` made +// `reset A` impossible — the observation it would have been judged against had +// been overwritten. The refusal that fired was honest and the feature was +// still missing. +// +// The name has already been through checkpointArgName or is a session +// identity this command built, so it carries no separator. It is cleaned here +// anyway, because a path built from a name is a path whoever reads this file +// next will assume was checked. +// +// TestTwoNamedCheckpointsDoNotOverwriteEachOther fails without this. +func checkpointPath(addr, name string) (string, error) { + dir, err := instanceDir(addr) + if err != nil { + return "", err + } + if name == "" || name == "." || name == ".." || strings.ContainsAny(name, `/\`) { + return "", fmt.Errorf("%q is not a checkpoint name", name) + } + return filepath.Join(dir, "checkpoints", filepath.Base(name)+".json"), nil +} + +// lastCheckpointPath records which name was taken most recently, so that +// `feint reset` with no argument has something to mean. +func lastCheckpointPath(addr string) (string, error) { dir, err := instanceDir(addr) if err != nil { return "", err } - return filepath.Join(dir, "checkpoint.json"), nil + return filepath.Join(dir, "checkpoints", "last"), nil } func writeRecordedCheckpoint(addr string, recorded recordedCheckpoint) error { - path, err := checkpointPath(addr) + path, err := checkpointPath(addr, recorded.Name) if err != nil { return err } @@ -328,17 +346,37 @@ func writeRecordedCheckpoint(addr string, recorded recordedCheckpoint) error { if err != nil { return err } - return os.WriteFile(path, append(body, '\n'), 0o600) + if err := os.WriteFile(path, append(body, '\n'), 0o600); err != nil { + return err + } + // Written after the record, so a failure above leaves `last` pointing at + // a checkpoint that exists. + last, err := lastCheckpointPath(addr) + if err != nil { + return err + } + return os.WriteFile(last, []byte(recorded.Name+"\n"), 0o600) } -func readRecordedCheckpoint(addr string) (recordedCheckpoint, error) { - path, err := checkpointPath(addr) +func readRecordedCheckpoint(addr, name string) (recordedCheckpoint, error) { + if name == "" { + last, err := lastCheckpointPath(addr) + if err != nil { + return recordedCheckpoint{}, err + } + body, err := os.ReadFile(filepath.Clean(last)) + if err != nil { + return recordedCheckpoint{}, fmt.Errorf("no checkpoint recorded for %s", addr) + } + name = strings.TrimSpace(string(body)) + } + path, err := checkpointPath(addr, name) if err != nil { return recordedCheckpoint{}, err } body, err := os.ReadFile(filepath.Clean(path)) if err != nil { - return recordedCheckpoint{}, fmt.Errorf("no checkpoint recorded for %s", addr) + return recordedCheckpoint{}, fmt.Errorf("no checkpoint named %q recorded for %s", name, addr) } var recorded recordedCheckpoint if err := json.Unmarshal(body, &recorded); err != nil { @@ -386,7 +424,7 @@ func takeSessionCheckpoint(endpoint, session, addr string) error { // // TestAFailedResetAfterIsReportedAndNotRaised fails without this. func resetToSession(endpoint, session, addr string, stderr io.Writer) (string, []string) { - recorded, err := readRecordedCheckpoint(addr) + recorded, err := readRecordedCheckpoint(addr, session) if err != nil || recorded.Name != session { fmt.Fprintf(stderr, "feint run: --reset-after has no session checkpoint to return to: %v\n", err) return "not attempted", nil diff --git a/internal/cli/checkpoint_test.go b/internal/cli/checkpoint_test.go index 48d21217..ae8f5814 100644 --- a/internal/cli/checkpoint_test.go +++ b/internal/cli/checkpoint_test.go @@ -55,15 +55,16 @@ func TestACheckpointNameIsTakenBeforeTheFlags(t *testing.T) { } } -// TestAResetRefusesToJudgeAgainstAnotherCheckpointsState is the other half of -// the verdict's honesty. +// TestAResetNamesTheCheckpointItCouldNotFind replaces a refusal that lost its +// subject to the audit's second finding. // -// The recorded observable state belongs to ONE checkpoint. Comparing the -// emulator's restore of one against the observation of another would report -// differences that are not the reset's fault, and an operator would chase -// them. So a name that disagrees with the record is refused rather than -// guessed. -func TestAResetRefusesToJudgeAgainstAnotherCheckpointsState(t *testing.T) { +// Before, the CLI kept one observation per instance, so asking to reset a +// name it had not recorded last meant comparing one checkpoint's restore to +// another's observation — and the command refused, honestly, while the +// feature was missing. Observations are indexed by name now, so an unknown +// name is simply unknown, and what matters is that the refusal says WHICH +// name and how to make one. +func TestAResetNamesTheCheckpointItCouldNotFind(t *testing.T) { t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) const addr = "127.0.0.1:4599" if err := writeRecordedCheckpoint(addr, recordedCheckpoint{ @@ -74,9 +75,9 @@ func TestAResetRefusesToJudgeAgainstAnotherCheckpointsState(t *testing.T) { var stdout, stderr bytes.Buffer if code := resetCommand([]string{"something-else", "--addr", addr}, &stdout, &stderr); code != exitError { - t.Fatalf("a reset against another checkpoint's state was allowed (%d)", code) + t.Fatalf("a reset of a name nobody recorded was allowed (%d)", code) } - for _, want := range []string{"baseline", "something-else", "compare the wrong states"} { + for _, want := range []string{"something-else", "feint checkpoint"} { if !strings.Contains(stderr.String(), want) { t.Errorf("the refusal does not mention %q: %s", want, stderr.String()) } @@ -94,6 +95,51 @@ func TestAResetRefusesToJudgeAgainstAnotherCheckpointsState(t *testing.T) { } } +// TestTwoNamedCheckpointsDoNotOverwriteEachOther is cited by checkpointPath, +// and it is the audit's second finding. +// +// The emulator holds any number of named checkpoints; the CLI wrote a single +// file per instance. So `checkpoint A` then `checkpoint B` made `reset A` +// impossible, because the observation A would have been judged against had +// been overwritten by B's. +func TestTwoNamedCheckpointsDoNotOverwriteEachOther(t *testing.T) { + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + const addr = "127.0.0.1:4599" + + for _, name := range []string{"alpha", "beta"} { + if err := writeRecordedCheckpoint(addr, recordedCheckpoint{ + SchemaVersion: checkpointStateVersion, Name: name, + State: observed{Machines: []string{"feint-scw-" + name}}, + }); err != nil { + t.Fatalf("record %s: %v", name, err) + } + } + + // Both are still readable BY NAME, which is what the single file broke. + for _, name := range []string{"alpha", "beta"} { + back, err := readRecordedCheckpoint(addr, name) + if err != nil { + t.Fatalf("read %s back: %v", name, err) + } + if back.Name != name { + t.Errorf("reading %q answered %q", name, back.Name) + } + if len(back.State.Machines) != 1 || back.State.Machines[0] != "feint-scw-"+name { + t.Errorf("%s carries the wrong observation: %v", name, back.State.Machines) + } + } + + // And an empty name means the last taken, so `feint reset` with no + // argument has something to mean. + last, err := readRecordedCheckpoint(addr, "") + if err != nil { + t.Fatalf("read the last: %v", err) + } + if last.Name != "beta" { + t.Errorf("the last taken reads as %q", last.Name) + } +} + // TestARecordedCheckpointRefusesAVersionItCannotRead is the forward // compatibility half. // @@ -109,7 +155,7 @@ func TestARecordedCheckpointRefusesAVersionItCannotRead(t *testing.T) { }); err != nil { t.Fatalf("record a checkpoint: %v", err) } - if _, err := readRecordedCheckpoint(addr); err == nil { + if _, err := readRecordedCheckpoint(addr, ""); err == nil { t.Fatal("a record from a later version was read anyway") } else if !strings.Contains(err.Error(), "version") { t.Errorf("the refusal does not say it is a version: %v", err) @@ -123,7 +169,7 @@ func TestARecordedCheckpointRefusesAVersionItCannotRead(t *testing.T) { }); err != nil { t.Fatalf("record a checkpoint: %v", err) } - back, err := readRecordedCheckpoint(addr) + back, err := readRecordedCheckpoint(addr, "") if err != nil { t.Fatalf("read it back: %v", err) } diff --git a/internal/cli/exit_contract_test.go b/internal/cli/exit_contract_test.go new file mode 100644 index 00000000..4c03e1a2 --- /dev/null +++ b/internal/cli/exit_contract_test.go @@ -0,0 +1,64 @@ +package cli + +import ( + "testing" +) + +// TestAGuaranteeThatWasNotDeliveredFailsTheRun is cited by runCommand's exit +// code, and it closes a contract error an external audit found in merged code. +// +// The first version failed the run only on "did not converge". So this was a +// possible CI run: +// +// terraform test success +// feint reset failed +// feint run exit 0 +// +// and the same held for a lab this command owned and could not stop: the +// receipt said `cleanup: failed` and the exit code stayed at the child's. +// +// The table is written as the full cross product on purpose. Two of the +// verdict words — "unverified" and "not attempted" — are the ones the first +// version let through, and a test that only covered the word it already +// handled would have agreed with the defect. +func TestAGuaranteeThatWasNotDeliveredFailsTheRun(t *testing.T) { + for _, probe := range []struct { + name string + reset string + cleanup string + delivered bool + }{ + // No reset was asked for, so there is nothing to have failed. + {"nothing asked, clean teardown", "", "clean", true}, + {"nothing asked, lab not ours", "", "not owned", true}, + {"nothing asked, kept on purpose", "", "kept", true}, + + // A reset that was asked for and delivered. + {"reset converged", "converged", "clean", true}, + + // Every way a reset can fail to deliver. The first version of this + // guard treated the last three as success. + {"reset did not converge", "did not converge", "clean", false}, + {"the emulator refused the reset", "failed", "clean", false}, + {"the reset could not be read back", "unverified", "clean", false}, + {"there was no checkpoint to return to", "not attempted", "clean", false}, + + // A teardown this command owed and did not perform. The receipt said + // so and the exit code did not. + {"the lab could not be stopped", "", "failed", false}, + {"converged, and the lab leaked anyway", "converged", "failed", false}, + + // A word nobody has written yet must not default to success: that is + // the whole reason the function lists the failures rather than the + // successes. + {"an unknown verdict word", "something-new", "clean", false}, + {"an unknown cleanup word", "", "something-new", false}, + } { + t.Run(probe.name, func(t *testing.T) { + if got := deliveredWhatWasAsked(probe.reset, probe.cleanup); got != probe.delivered { + t.Errorf("deliveredWhatWasAsked(%q, %q) = %v, want %v", + probe.reset, probe.cleanup, got, probe.delivered) + } + }) + } +} diff --git a/internal/cli/observable.go b/internal/cli/observable.go index 7c607f62..02be169d 100644 --- a/internal/cli/observable.go +++ b/internal/cli/observable.go @@ -2,6 +2,8 @@ package cli import ( "context" + "crypto/sha256" + "encoding/hex" "encoding/json" "encoding/xml" "fmt" @@ -36,6 +38,31 @@ import ( // restoring the store and the host leaves bytes from a later test in a bucket // the checkpoint had empty. // +// ## Why the digest is computed here and not read from the listing +// +// The first version of this file took the digest from ListObjectsV2's ETag, +// and `internal/s3/list.go` emits no ETag at all: the field was always empty, +// so two different contents under one key compared EQUAL and a reset could +// report convergence over bytes that had changed. The commit message claimed +// objects were compared by digest. They were not. +// +// It survived review and a passing test because the test built observedObject +// values by hand, with digests in them. A comparison is only worth the +// observation under it, and an observation nobody made is a field with a zero +// value. Found by an external audit of the merged code, not here. +// +// So each object is FETCHED and hashed by the observer. Not the store's own +// recorded digest and not the checksum header the door offers: what is being +// judged is whether a client gets the same bytes back, and a digest supplied +// by the subject cannot answer that. The size travels with it as a second, +// independent signal and is part of the comparison key, which the first +// version also dropped. +// +// It costs one GET per object. That is acceptable for a verdict on a local +// store with a byte budget, and the alternative measured nothing. +// +// TestTwoObjectsOfOneSizeAndDifferentBytesDoNotConverge fails without this. +// // ## Why the bytes are read through the door rather than from the store // // The authoritative view of an object is the one a client gets, and this @@ -88,8 +115,16 @@ type observedObject struct { Door string `json:"door"` Bucket string `json:"bucket"` Key string `json:"key"` + // Digest is a SHA-256 this observer computed over the bytes a GET + // delivered. Never the store's own record of it, and never the door's + // checksum header: a digest supplied by the subject cannot answer whether + // the subject serves the bytes it claims. Digest string `json:"digest"` - Size int64 `json:"size"` + // Size is what the GET actually delivered, and Listed is what the + // inventory said. They are compared separately so an inventory that + // disagrees with the bytes is a difference rather than a silent one. + Size int64 `json:"size"` + Listed int64 `json:"listed"` } // observeLab reads the three halves and normalises them. @@ -234,9 +269,18 @@ func observeObjects(ctx context.Context, endpoint string) ([]observedObject, err return nil, fmt.Errorf("%s%s: %w", door.Path, bucket, err) } for _, object := range objects { + digest, size, err := fetchDigest(ctx, client, base+bucket+"/"+object.Key) + if err != nil { + return nil, fmt.Errorf("%s%s/%s: %w", door.Path, bucket, object.Key, err) + } + // The listing's size travels too, and a disagreement between + // it and what the GET delivered is itself a difference: a + // door whose inventory and bytes disagree has not converged + // whatever the digests say. out = append(out, observedObject{ Door: strings.Trim(door.Path, "/"), Bucket: bucket, - Key: object.Key, Digest: object.ETag, Size: object.Size, + Key: object.Key, Digest: digest, Size: size, + Listed: object.Size, }) } } @@ -255,7 +299,6 @@ type bucketListing struct { type objectListing struct { Contents []struct { Key string `xml:"Key"` - ETag string `xml:"ETag"` Size int64 `xml:"Size"` } `xml:"Contents"` IsTruncated bool `xml:"IsTruncated"` @@ -277,13 +320,11 @@ func listBuckets(ctx context.Context, client *http.Client, base string) ([]strin func listObjects(ctx context.Context, client *http.Client, base, bucket string) ([]struct { Key string `xml:"Key"` - ETag string `xml:"ETag"` Size int64 `xml:"Size"` }, error, ) { var all []struct { Key string `xml:"Key"` - ETag string `xml:"ETag"` Size int64 `xml:"Size"` } // Paginated, because a comparison that read the first page only would @@ -337,7 +378,11 @@ func (o observed) diff(other observed) []string { return r.Kind + " " + r.ID + " state=" + r.State + " backing=[" + r.Backing + "]" } objectKey := func(ob observedObject) string { - return ob.Door + "/" + ob.Bucket + "/" + ob.Key + " digest=" + ob.Digest + // Every field, and the size twice. The first version compared the + // door, the bucket, the key and a digest that was always empty, so it + // compared nothing about the content at all. + return fmt.Sprintf("%s/%s/%s digest=%s bytes=%d listed=%d", + ob.Door, ob.Bucket, ob.Key, ob.Digest, ob.Size, ob.Listed) } out = append(out, compareSets("resource", mapAll(o.Resources, resourceKey), mapAll(other.Resources, resourceKey))...) @@ -371,3 +416,37 @@ func compareSets(kind string, want, got []string) []string { } return out } + +// fetchDigest reads an object through its door and answers what arrived. +// +// The hash is computed over the response body as it streams, so nothing the +// door says about the bytes is taken on trust, and the byte count comes from +// the hash's own writer rather than from a header. +func fetchDigest(ctx context.Context, client *http.Client, url string) (string, int64, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return "", 0, err + } + res, err := client.Do(req) + if err != nil { + return "", 0, err + } + defer func() { _ = res.Body.Close() }() + if res.StatusCode != http.StatusOK { + return "", 0, fmt.Errorf("the door answered %d", res.StatusCode) + } + sum := sha256.New() + size, err := io.Copy(sum, io.LimitReader(res.Body, maxObservedObjectBytes)) + if err != nil { + return "", 0, err + } + return hex.EncodeToString(sum.Sum(nil)), size, nil +} + +// maxObservedObjectBytes bounds one object a verdict will read. +// +// Generous rather than tight: the store's own per-object limit is 256 MiB by +// default, and a verdict that silently truncated at a smaller bound would +// compare the first N bytes of two objects and call them equal — the same +// class of defect as the empty digest this replaces. +const maxObservedObjectBytes = 512 << 20 diff --git a/internal/cli/observable_bytes_test.go b/internal/cli/observable_bytes_test.go new file mode 100644 index 00000000..4d20eca9 --- /dev/null +++ b/internal/cli/observable_bytes_test.go @@ -0,0 +1,191 @@ +package cli + +import ( + "encoding/json" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stephrobert/feint/internal/core/machine" + "github.com/stephrobert/feint/internal/core/objectstore" + "github.com/stephrobert/feint/internal/s3" +) + +// The defect an external audit found in merged code, and the test that would +// have stopped it. +// +// The observable state took an object's digest from ListObjectsV2's ETag, and +// internal/s3/list.go emits no ETag: the field was always empty, so two +// different contents under one key compared EQUAL. A reset could report +// convergence over bytes that had changed, and the commit message claimed +// objects were compared by digest. +// +// What let it through is the shape of the test beside it: +// TestADifferenceIsNamedWhereverItIs built observedObject values BY HAND, with +// digests in them, so it exercised the comparison and never the observation. +// A fake that emitted an ETag would have hidden it just as well. +// +// So this one mounts a REAL door over a REAL object store. It is the only +// arrangement in which the absent field is visible at all. + +// realDoorLab serves one scaleway/fr-par door over a byte store, plus the +// three endpoints observeLab reads. Nothing here is canned: the listing, the +// bytes and the report all come from internal/s3. +func realDoorLab(t *testing.T) (*httptest.Server, *objectstore.Store) { + t.Helper() + + root := t.TempDir() + if err := os.Chmod(root, 0o700); err != nil { + t.Fatal(err) + } + store, err := objectstore.OpenWithLimit(t.Context(), filepath.Join(root, "objects"), 2<<20) + if err != nil { + t.Fatalf("open the byte store: %v", err) + } + t.Cleanup(func() { + if err := store.Close(); err != nil { + t.Error(err) + } + }) + + control := http.NewServeMux() + control.HandleFunc("/_feint/resources", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"count":0,"resources":[]}`)) + }) + control.HandleFunc("/_feint/health", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"machines":"none","capabilities":null,"enforced":{}}`)) + }) + + doors, err := s3.Mount(control, store, []s3.Profile{ + // The profile the Scaleway pack actually declares, not a minimal one: + // a door without BucketConfiguration answers 501 to the bucket listing, + // and the reader would then observe nothing at all. Copied from the + // pack so this test measures the door an operator gets. + {Provider: "scaleway", Region: "fr-par", MissingBucket: s3.ScalewayMissingBucket, + BucketConfiguration: true, ObjectDiscovery: true, ListObjectsV1: true, + PrivateACLWrite: true}, + }, 1<<20, slog.New(slog.NewTextHandler(io.Discard, nil))) + if err != nil { + t.Fatalf("mount the door: %v", err) + } + // The report the reader validates, served from the doors' own snapshot so + // the test cannot disagree with the code about what is mounted. + mounted := http.NewServeMux() + mounted.HandleFunc(s3.ReportPath, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + writeReportJSON(t, w, doors) + }) + mounted.Handle("/", doors) + + server := httptest.NewServer(mounted) + t.Cleanup(server.Close) + return server, store +} + +func writeReportJSON(t *testing.T, w http.ResponseWriter, doors *s3.Doors) { + t.Helper() + if err := json.NewEncoder(w).Encode(doors.Snapshot()); err != nil { + t.Errorf("serve the S3 report: %v", err) + } +} + +// TestTwoObjectsOfOneSizeAndDifferentBytesDoNotConverge is cited by the +// observable state's doc, and it is the audit's own decisive case: two objects +// of the same key and the same size, with different bytes, must produce a +// different verdict. +func TestTwoObjectsOfOneSizeAndDifferentBytesDoNotConverge(t *testing.T) { + lab, _ := realDoorLab(t) + const bucket, key = "images", "disk.img" + base := lab.URL + s3.DoorPath("scaleway", "fr-par") + + // Everything goes THROUGH the door, which is what a client does and what + // this test is about. It also earned its place: a bucket created behind + // the door, straight on the store, carries no creation metadata and the + // door then refuses to list it with a 501 — "legacy bucket listing has no + // measured creation metadata". The first version of this test did that + // and spent three runs blaming the reader. + doorPut(t, base+bucket, "") + + // Same length, different content. A comparison on size alone, or on an + // absent digest, calls these two states equal. + const before, after = "AAAAAAAAAAAAAAAA", "BBBBBBBBBBBBBBBB" + if len(before) != len(after) { + t.Fatal("the two payloads must be the same length or this test proves nothing") + } + + put := func(body string) { + t.Helper() + doorPut(t, base+bucket+"/"+key, body) + } + + put(before) + checkpoint, err := observeLab(t.Context(), lab.URL, machine.Runtime{}) + if err != nil { + t.Fatalf("observe the checkpoint: %v", err) + } + if len(checkpoint.Objects) != 1 { + t.Fatalf("the observation found %d object(s); the door or the reader is not wired", + len(checkpoint.Objects)) + } + // The digest must be REAL. An empty one is how the defect looked. + if len(checkpoint.Objects[0].Digest) != 64 { + t.Fatalf("the observation carries digest %q; an empty or short digest compares everything equal", + checkpoint.Objects[0].Digest) + } + if checkpoint.Objects[0].Size != int64(len(before)) { + t.Errorf("the observation read %d bytes", checkpoint.Objects[0].Size) + } + + put(after) + changed, err := observeLab(t.Context(), lab.URL, machine.Runtime{}) + if err != nil { + t.Fatalf("observe the change: %v", err) + } + + differences := checkpoint.diff(changed) + if len(differences) == 0 { + t.Fatal("two objects of one key and one size with different bytes compared EQUAL; " + + "a reset would report convergence over content that changed") + } + joined := strings.Join(differences, "\n") + if !strings.Contains(joined, key) { + t.Errorf("the verdict does not name the object that changed:\n%s", joined) + } + + // And the same bytes twice compare equal, so this cannot be satisfied by + // a comparison that always differs — which would make every reset fail + // and teach an operator to ignore the verdict. + put(before) + back, err := observeLab(t.Context(), lab.URL, machine.Runtime{}) + if err != nil { + t.Fatalf("observe the restoration: %v", err) + } + if differences := checkpoint.diff(back); len(differences) != 0 { + t.Errorf("the same bytes compared differently:\n%s", strings.Join(differences, "\n")) + } +} + +// doorPut writes through the door, so that what the listing and the GET see +// is what a client would have made. +func doorPut(t *testing.T, url, body string) { + t.Helper() + req, err := http.NewRequestWithContext(t.Context(), http.MethodPut, url, strings.NewReader(body)) + if err != nil { + t.Fatal(err) + } + req.ContentLength = int64(len(body)) + res, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("PUT %s: %v", url, err) + } + defer func() { _ = res.Body.Close() }() + if res.StatusCode != http.StatusOK { + answer, _ := io.ReadAll(io.LimitReader(res.Body, 1<<12)) + t.Fatalf("PUT %s answered %d: %s", url, res.StatusCode, answer) + } +} diff --git a/internal/cli/reconcile_rules_test.go b/internal/cli/reconcile_rules_test.go new file mode 100644 index 00000000..9d79f71d --- /dev/null +++ b/internal/cli/reconcile_rules_test.go @@ -0,0 +1,51 @@ +package cli + +import ( + "testing" + + "github.com/stephrobert/feint/internal/core/emulator" +) + +// TestAPackThatEnforcesRulesReappliesThemOnReset is cited by +// emulator.RuleReconciler. +// +// A pack that pushes its rules onto the runtime owns a second promise it is +// easy not to notice: bringing them BACK. A reset restores the control plane +// and asks the runtime what it holds, and Survey answers names. A rule set +// whose content changed after the checkpoint keeps its name, so the +// observation sees no difference, the reset reports convergence, and the +// runtime goes on enforcing the rules of a state nobody asked to return to. +// The network suite measured that with a TCP connection before this interface +// existed. +// +// The subject is read from the pack — "does it declare that it enforces" — +// rather than from a list of provider names, so a fourth pack inherits the +// obligation without anyone editing this file. That is rule 5 applied to a +// test, and the same shape as its neighbour +// TestAPackThatDrivesMachinesReconcilesBothWays. +func TestAPackThatEnforcesRulesReappliesThemOnReset(t *testing.T) { + srv, _, err := newServer(nil) + if err != nil { + t.Fatalf("build the emulator: %v", err) + } + + examined := 0 + for _, pack := range srv.Packs() { + enforcer, declares := pack.(emulator.FirewallEnforcer) + if !declares || !enforcer.EnforcesFirewall() { + // A pack whose rules stay metadata has nothing to put back. + continue + } + examined++ + if _, reapplies := pack.(emulator.RuleReconciler); !reapplies { + t.Errorf("%s pushes its rules onto the runtime and cannot put them back: a reset "+ + "would report convergence while the runtime still enforces the rules of a "+ + "state the checkpoint does not describe", pack.Name()) + } + } + + // A denominator, because a loop over nothing passes silently. + if examined < 3 { + t.Fatalf("only %d pack(s) declare that they enforce rules; this build has three", examined) + } +} diff --git a/internal/cli/reset_after_test.go b/internal/cli/reset_after_test.go index 309c0ba4..8523fa51 100644 --- a/internal/cli/reset_after_test.go +++ b/internal/cli/reset_after_test.go @@ -103,8 +103,15 @@ func TestAFailedResetAfterIsReportedAndNotRaised(t *testing.T) { t.Run("a session that is not this run's is not reset", func(t *testing.T) { // Two runs against one shared emulator must not reset each other's - // state. The recorded name decides, and a mismatch is "not attempted" - // rather than a reset of somebody else's checkpoint. + // state. Since the audit, records are indexed by name, so the + // isolation is the lookup itself: a run asks for ITS session and + // finds nothing when only another run's was recorded. + // + // A fresh runtime directory, because the subtests above recorded + // their own sessions here and the first version of this one found + // one of them — the test passing for the wrong reason, which is what + // the per-name index changed underneath it. + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) if err := writeRecordedCheckpoint(addr, recordedCheckpoint{ SchemaVersion: checkpointStateVersion, Name: "feint-lab-OTHER", }); err != nil { @@ -115,6 +122,11 @@ func TestAFailedResetAfterIsReportedAndNotRaised(t *testing.T) { if verdict != "not attempted" { t.Errorf("verdict %q for another run's session", verdict) } + // And the other run's record is untouched: this one did not reset it + // and did not delete it either. + if back, err := readRecordedCheckpoint(addr, "feint-lab-OTHER"); err != nil || back.Name != "feint-lab-OTHER" { + t.Errorf("another run's record was disturbed: %+v %v", back, err) + } }) } @@ -136,7 +148,7 @@ func TestASessionCheckpointIsNamedAfterItsRun(t *testing.T) { if len(asked) != 1 || asked[0] != "/_feint/checkpoint/feint-lab-4021" { t.Fatalf("the emulator was asked for %v", asked) } - recorded, err := readRecordedCheckpoint(addr) + recorded, err := readRecordedCheckpoint(addr, "") if err != nil { t.Fatalf("read the record back: %v", err) } diff --git a/internal/cli/run.go b/internal/cli/run.go index 8e41a8ed..0a06db49 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -278,11 +278,29 @@ func runCommand(args []string, stdout io.Writer, stderr io.Writer) int { return exitError } } - // A reset that did not converge is this command's failure even when the - // child passed: the caller asked for the lab to be left as found, and it - // was not. Reported separately from the child's code so a reader can tell - // a failing test from a leaking lab. - if resetVerdict == "did not converge" && code == exitOK { + // A guarantee this command was ASKED for and did not deliver is this + // command's failure, even when the child passed. + // + // The first version only failed on "did not converge", which an external + // audit read correctly as a contract error: a reset that could not be + // attempted, that the emulator refused, or that could not be read back + // left the exit code at the child's, so + // + // terraform test success + // feint reset failed + // feint run exit 0 + // + // was a possible CI run. The same held for a lab this command owned and + // could not stop: `cleanup = "failed"` reached the receipt and nothing + // else, so a leaked port and a leaked directory reported success. + // + // So the rule is the general one, and it is worth stating because it is + // what a wrapper is for: the child's code travels untouched when the + // wrapper kept its promises, and the wrapper fails when it did not. Which + // of the two happened is in the receipt, because "the test failed" and + // "the lab leaked" send a reader to different places. + // TestAGuaranteeThatWasNotDeliveredFailsTheRun fails without this. + if code == exitOK && !deliveredWhatWasAsked(resetVerdict, cleanup) { return exitError } // The child's code, not this command's: a wrapper that swallowed a @@ -307,6 +325,34 @@ func keepsTheLab(code int, runErr error, asked bool) bool { return code != exitOK || runErr != nil } +// deliveredWhatWasAsked reports whether this command kept the promises it was +// given, which is a different question from whether the child passed. +// +// Written as a list of the answers that are NOT a success rather than the ones +// that are: a verdict word added later would otherwise default to "fine", and +// the one thing this function exists to prevent is a new way of failing that +// nobody notices because the switch did not name it. +func deliveredWhatWasAsked(resetVerdict, cleanup string) bool { + switch resetVerdict { + case "", "converged": + // Empty means --reset-after was never asked for, so there is nothing + // to have failed. + default: + // "did not converge", "failed", "unverified" and "not attempted" are + // all a reset the caller asked for and did not get. + return false + } + switch cleanup { + case "clean", "kept", "not owned": + // Kept is a success: --keep-on-failure asked for exactly that. + // Not owned is this command declining to claim a teardown it did not + // perform. + default: + return false + } + return true +} + // labSession names an owned lab, and deliberately nothing for an attached // one: its identity belongs to whoever started it, and inventing a second name // for it would put two names on one emulator. diff --git a/internal/core/emulator/atomic_checkpoint_test.go b/internal/core/emulator/atomic_checkpoint_test.go new file mode 100644 index 00000000..95828e1a --- /dev/null +++ b/internal/core/emulator/atomic_checkpoint_test.go @@ -0,0 +1,110 @@ +package emulator_test + +import ( + "io" + "net/http" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/stephrobert/feint/internal/core/objectstore" + "github.com/stephrobert/feint/internal/core/resource" +) + +// TestAFailedControlPlaneHalfLeavesTheEarlierBytesWhole is cited by +// handleCheckpointSave, and it is the audit's finding about my own comment. +// +// The first version saved the bytes first and the control plane second while +// promising "both halves or neither". A failure in the second step then left +// an earlier checkpoint of that name holding its OLD store beside the NEW +// object references, and a later reset restored one against the other. +// +// ## Why this test looks the way it does +// +// A first attempt made the failure by closing the byte store, and it measured +// NOTHING: with the bytes failing first, both orders refuse at the same step +// and the earlier checkpoint survives either way. The mutation that reverses +// the order stayed green, which is the harness catching the author. +// +// The scenario that separates the two orders is the one the audit actually +// described: the byte half SUCCEEDS and the control-plane half fails. So the +// store is made unmarshallable — a resource carrying a value encoding/json +// cannot write — and the assertion is on the BYTES a later reset brings back. +// +// Under the shipped order: the snapshot fails first, the bytes are never +// touched, and the reset returns the original content. Under the reversed +// order: the new bytes are published, the snapshot then fails, and the reset +// returns content that was never checkpointed. +func TestAFailedControlPlaneHalfLeavesTheEarlierBytesWhole(t *testing.T) { + root := t.TempDir() + if err := os.Chmod(root, 0o700); err != nil { + t.Fatal(err) + } + store, err := objectstore.OpenWithLimit(t.Context(), filepath.Join(root, "objects"), 2<<20) + if err != nil { + t.Fatalf("open the byte store: %v", err) + } + t.Cleanup(func() { + if err := store.Close(); err != nil { + t.Error(err) + } + }) + server, held := labServerWithBytes(t, store, stubPack{name: "acme"}) + + const bucket, key = "acme/images", "disk.img" + const original, later = "the bytes the checkpoint holds", "bytes nobody checkpointed" + if err := store.CreateBucket(t.Context(), bucket); err != nil { + t.Fatalf("create the bucket: %v", err) + } + write := func(body string) { + t.Helper() + if _, err := store.PutObject(t.Context(), bucket, key, + strings.NewReader(body), objectstore.PutOptions{Size: int64(len(body))}); err != nil { + t.Fatalf("put: %v", err) + } + } + + write(original) + if code, body := checkpointPost(t, server.URL+"/_feint/checkpoint/baseline"); code != http.StatusOK { + t.Fatalf("the first checkpoint: %d %v", code, body) + } + + // The lab moves on, and the control-plane half is then made to fail while + // the byte half would succeed. + write(later) + unmarshallable := resource.New("broken", "instance/server", + resource.Tenant{Provider: "acme", Project: "p", Zone: "z"}, "running", + time.Unix(1700000000, 0).UTC()) + unmarshallable.Attrs = map[string]any{"cannot": make(chan int)} + held.Put(unmarshallable) + + code, body := checkpointPost(t, server.URL+"/_feint/checkpoint/baseline") + if code == http.StatusOK { + t.Fatalf("a checkpoint whose control-plane half could not be taken reported success: %v", body) + } + if message, _ := body["error"].(string); !strings.Contains(message, "control plane") { + t.Errorf("the refusal does not name the half that failed: %v", body["error"]) + } + + // The earlier checkpoint's BYTES are intact. This is the assertion the + // reversed order fails: it would have published `later` under the name + // `baseline` before refusing. + if err := store.ResetCheckpoint(t.Context(), "baseline"); err != nil { + t.Fatalf("reset the byte checkpoint: %v", err) + } + _, reader, err := store.GetObject(t.Context(), bucket, key) + if err != nil { + t.Fatalf("read the object back: %v", err) + } + defer func() { _ = reader.Close() }() + back, err := io.ReadAll(reader) + if err != nil { + t.Fatalf("read: %v", err) + } + if string(back) != original { + t.Errorf("the checkpoint's bytes are %q, not the %q it held: a half-taken checkpoint "+ + "published bytes the control plane never saw", back, original) + } +} diff --git a/internal/core/emulator/checkpoint.go b/internal/core/emulator/checkpoint.go index b149039a..090e4213 100644 --- a/internal/core/emulator/checkpoint.go +++ b/internal/core/emulator/checkpoint.go @@ -83,6 +83,28 @@ type MissingReconciler interface { ReconcileMissing(ctx context.Context, records []*resource.Resource, held []string) machine.Reconciled } +// RuleReconciler is the third optional half, and separate from the other two +// for the reason MissingReconciler spells out: a pack gains one when it is +// ready, and the ones it already had keep working. Merging them would turn +// every pack into a non-implementer in silence. +// +// It exists because a reset that only restores the store reports convergence +// over a firewall the runtime is still enforcing from before the checkpoint. +// Survey answers names, a modified rule set keeps its name, so nothing in the +// observation can see the difference — and machine.GroupSync.Reapply carries +// the measurement that turned that deduction into a defect. +// +// tools/conformance/scaleway/network.sh and internal/cli's +// TestAPackThatEnforcesRulesReappliesThemOnReset fail without it. +type RuleReconciler interface { + // ReconcileRules writes this pack's restored rule sets to the runtime + // again. It takes the pack's own records, because which of them is a + // group is the pack's vocabulary — "instance/security-group", + // "securitygroup" and "security-group" for the three — and the core + // guessing at that is the mistake ReconcileMissing documents. + ReconcileRules(ctx context.Context, records []*resource.Resource) machine.Reconciled +} + // checkpoints holds the control-plane half of each named checkpoint. // // In memory, and that is the scope #853 describes: "returns to the session's @@ -120,13 +142,28 @@ func (c *checkpoints) drop(name string) bool { // handleCheckpointSave records the control plane and the bytes under one name. // -// Both halves or neither: a checkpoint whose store was taken and whose bytes -// were not is a checkpoint a reset cannot honour, and it would fail later, -// somewhere else, with no mention of this call. So the byte half goes first — -// it is the one that can fail on a disk — and the store half follows only -// when it succeeded. +// Both halves or neither, and the ORDER is what makes that true rather than a +// sentence. An external audit caught the first version claiming it: the bytes +// were saved first, so a failure in the store snapshot left a previously taken +// checkpoint of the same name holding its OLD control plane beside the NEW +// object references. A later reset would have restored one against the other, +// and the comment promising otherwise is the defect this repository names most +// often. +// +// The order that delivers it: +// +// 1. take the control-plane snapshot into memory. Nothing durable is touched, +// and a failure here leaves the previous checkpoint whole; +// 2. save the bytes, which objectstore publishes by renaming a temporary +// file: it either replaces the old checkpoint or leaves it, never a +// half of one; +// 3. publish the control-plane half only now. // -// TestACheckpointTakesBothHalvesOrNeither fails without this. +// So a failure at any step leaves the previous checkpoint consistent with +// itself, which is the only promise worth making. +// +// TestACheckpointTakesBothHalvesOrNeither and +// TestAFailedControlPlaneHalfLeavesTheEarlierBytesWhole fail without this. func (s *Server) handleCheckpointSave(w http.ResponseWriter, r *http.Request) { name := r.PathValue("name") if !validCheckpointName(name) { @@ -135,6 +172,13 @@ func (s *Server) handleCheckpointSave(w http.ResponseWriter, r *http.Request) { }) return } + var state bytes.Buffer + if err := s.env.Store.Snapshot(&state); err != nil { + writeJSON(w, http.StatusInternalServerError, map[string]any{ + "error": "the control plane could not be checkpointed: " + err.Error(), + }) + return + } if s.env.Objects != nil { if err := s.env.Objects.SaveCheckpoint(r.Context(), name); err != nil { writeJSON(w, http.StatusInternalServerError, map[string]any{ @@ -143,13 +187,9 @@ func (s *Server) handleCheckpointSave(w http.ResponseWriter, r *http.Request) { return } } - var state bytes.Buffer - if err := s.env.Store.Snapshot(&state); err != nil { - writeJSON(w, http.StatusInternalServerError, map[string]any{ - "error": "the control plane could not be checkpointed: " + err.Error(), - }) - return - } + // Published last, so that a byte half that failed above leaves any earlier + // checkpoint of this name holding its own control plane rather than a + // newer one. s.checkpoints.put(name, state.Bytes()) writeJSON(w, http.StatusOK, map[string]any{ "schema_version": CheckpointSchemaVersion, @@ -236,11 +276,15 @@ func (s *Server) handleReset(w http.ResponseWriter, r *http.Request) { // asked would start containers beside ones already running. held, surveyed := heldOnHost(r.Context(), s.env) reconciled := map[string]any{} - var destroyed, skipped, recreated, failed []string + var destroyed, skipped, recreated, failed, reapplied []string for _, pack := range s.packs { mine := ownedBy(gone, pack.Name()) records := ownedBy(restored, pack.Name()) - if len(mine) == 0 && (!surveyed || len(records) == 0) { + // Records alone are enough to have work to do, with no survey: rule + // sets live on the runtime whether or not a machine wears one, and an + // earlier form of this guard skipped the whole pack when nothing could + // be surveyed, which would have skipped the firewall half with it. + if len(mine) == 0 && len(records) == 0 { continue } done := machine.Reconciled{} @@ -261,15 +305,26 @@ func (s *Server) handleReset(w http.ResponseWriter, r *http.Request) { s.env.Store.Put(res) } } + // Last of the three, and the order is the point: a set is attached to + // the interfaces of the machines that wear it, so re-applying before + // ReconcileMissing has put those machines back would attach nothing. + if rules, ok := pack.(RuleReconciler); ok && len(records) != 0 { + back := rules.ReconcileRules(r.Context(), records) + done.Reapplied = append(done.Reapplied, back.Reapplied...) + done.Skipped = append(done.Skipped, back.Skipped...) + } destroyed = append(destroyed, done.Destroyed...) + reapplied = append(reapplied, done.Reapplied...) skipped = append(skipped, done.Skipped...) recreated = append(recreated, done.Recreated...) failed = append(failed, done.Failed...) reconciled[pack.Name()] = map[string]any{ "destroyed": done.Destroyed, "skipped": done.Skipped, "recreated": done.Recreated, "failed": done.Failed, + "reapplied": done.Reapplied, } } + sort.Strings(reapplied) sort.Strings(destroyed) sort.Strings(skipped) sort.Strings(recreated) @@ -282,6 +337,11 @@ func (s *Server) handleReset(w http.ResponseWriter, r *http.Request) { "destroyed": destroyed, "skipped": skipped, "recreated": recreated, + // Named in the receipt rather than folded into recreated: nothing was + // started, a rule set the runtime already held was overwritten with + // the restored rules. An operator reading "recreated" would look for + // a machine that never moved. + "reapplied": reapplied, // Named apart from skipped: a start that was attempted and refused and // a record nobody could act on need different answers from whoever // reads this. diff --git a/internal/core/emulator/reapply_test.go b/internal/core/emulator/reapply_test.go new file mode 100644 index 00000000..f6882943 --- /dev/null +++ b/internal/core/emulator/reapply_test.go @@ -0,0 +1,91 @@ +package emulator_test + +import ( + "context" + "net/http" + "testing" + + "github.com/stephrobert/feint/internal/core/machine" + "github.com/stephrobert/feint/internal/core/resource" +) + +// reapplyingPack implements the third half and records what it was handed. +type reapplyingPack struct { + stubPack + asked [][]string +} + +func (p *reapplyingPack) ReconcileRules(_ context.Context, records []*resource.Resource) machine.Reconciled { + var groups []string + for _, res := range records { + if res.Kind == "instance/security-group" { + groups = append(groups, res.ID) + } + } + p.asked = append(p.asked, groups) + return machine.Reconciled{Reapplied: groups} +} + +// TestAResetPutsTheRuleBackOnTheRuntimeAndNotOnlyInTheStore is cited by +// emulator.RuleReconciler and by machine.GroupSync.Reapply. +// +// ## The defect it closes +// +// An external audit deduced it and tools/conformance/scaleway/network.sh then +// measured it under --vm incus, with the only instrument whose verdict cannot +// be argued with: +// +// - and a reset puts the rule back on the NIC, not only in the store +// FAIL: the reset reported convergence and port 80 is still refused: +// the rule came back in the store and not on the NIC +// +// A reset restores the control plane and asks the runtime what it HOLDS, and +// Survey answers NAMES. The rule set of a group whose rules changed after the +// checkpoint keeps its name, so the observation sees no difference at all: the +// reset reports convergence while the runtime goes on enforcing a state the +// checkpoint does not describe. +// +// ## The two properties this asserts, and why each would have been missed +// +// 1. the group is handed over although it is UNCHANGED by the restore. It is +// not in `gone` and its machine is not missing, so both existing halves +// skip it — and the whole point is that the record is identical and the +// runtime is not; +// 2. it is handed over with NO surveyable runtime. Rule sets live on the +// runtime whether or not any machine wears one, and the handler's guard +// used to skip a pack entirely when nothing could be surveyed, which would +// have taken this half with it. +func TestAResetPutsTheRuleBackOnTheRuntimeAndNotOnlyInTheStore(t *testing.T) { + acme := &reapplyingPack{stubPack: stubPack{name: "acme"}} + // The zero runtime, deliberately: see property 2. + server, held := labServer(t, acme) + + put(t, held, "acme", "instance/security-group", "open-80") + put(t, held, "acme", "instance/server", "web") + if code, body := checkpointPost(t, server.URL+"/_feint/checkpoint/baseline"); code != http.StatusOK { + t.Fatalf("checkpoint: %d %v", code, body) + } + + // Nothing is added or removed between the checkpoint and the reset: the + // rules of the group changed, which the store round-trips and the runtime + // does not. + code, body := checkpointPost(t, server.URL+"/_feint/reset/baseline") + if code != http.StatusOK { + t.Fatalf("reset: %d %v", code, body) + } + if len(acme.asked) != 1 { + t.Fatalf("the pack was asked to re-apply its rules %d time(s); a reset must ask exactly once", + len(acme.asked)) + } + if len(acme.asked[0]) != 1 || acme.asked[0][0] != "open-80" { + t.Fatalf("the pack was handed %v; the restored group is what its rule set is translated from", + acme.asked[0]) + } + // And the receipt says so, rather than leaving a reader to infer it from a + // quiet success. Named apart from recreated: nothing was started. + back, _ := body["reapplied"].([]any) + if len(back) != 1 || back[0] != "open-80" { + t.Errorf("the receipt reports reapplied=%v, so an operator cannot tell the rules were put back", + body["reapplied"]) + } +} diff --git a/internal/core/machine/groupsync.go b/internal/core/machine/groupsync.go index 902cab96..7af27ae6 100644 --- a/internal/core/machine/groupsync.go +++ b/internal/core/machine/groupsync.go @@ -264,12 +264,23 @@ func (s GroupSync) spec(group, fresh *resource.Resource) FirewallSpec { // fresh, when non-nil, wins over the store's copy of the same resource, in the // wearer replay as in the expansion — SpecOf says why. func (s GroupSync) SyncGroup(ctx context.Context, group, fresh *resource.Resource) { + _ = s.syncGroup(ctx, group, fresh) +} + +// syncGroup is SyncGroup with its verdict kept. +// +// The bool exists for Reapply alone: every other caller acts on a change it +// just made and has nothing to report, while a reconciliation answers an +// operator who asked whether the lab came back. false means the runtime was +// not wired or refused the set — in both cases the rules are metadata again, +// and saying so is the point. +func (s GroupSync) syncGroup(ctx context.Context, group, fresh *resource.Resource) bool { if !s.wired() { - return + return false } fw := s.enforcer() if !s.Binding.SyncRuleSet(ctx, fw, s.spec(group, fresh)) { - return + return false } for _, wearer := range s.Wearers(group) { if fresh != nil && wearer.ID == fresh.ID { @@ -277,6 +288,68 @@ func (s GroupSync) SyncGroup(ctx context.Context, group, fresh *resource.Resourc } s.applyMachine(ctx, wearer, fresh) } + return true +} + +// Reapply writes every one of these groups to the runtime again and replays +// each onto its wearers. +// +// ## Why a reset needs this at all +// +// An external audit deduced it from the code and this repository then measured +// it, which is the order that makes it a defect rather than a worry. A reset +// restores the control plane and asks the runtime what it HOLDS, and Survey +// answers NAMES. A rule revoked after the checkpoint leaves its rule set in +// place under the same name, carrying the revoked content: nothing in the +// observation differs, the reset reports convergence, and the port stays +// closed. The suite reproduced exactly that under --vm incus, with a TCP +// connection as the instrument: +// +// FAIL: the reset reported convergence and port 80 is still refused: +// the rule came back in the store and not on the NIC +// +// Destroying and recreating the set would be the wrong repair: it is a +// reconfiguration, not a resurrection, and SyncRuleSet already replaces a set +// of the same name with the restored translation. +// +// ## Why it is shared and not three copies +// +// The sequence is identical for the three packs — translate the restored +// group, write the set, replay it onto the wearers — and only the KIND of +// record a pack calls a group differs. That is the repository's own test: a +// line that could be written identically for another provider belongs here. +// The firewall reconciliation was written once in a pack before, and the two +// others ran with zero ACLs while their API described a closed port (#475). +// +// tools/conformance/scaleway/network.sh fails without this. +func (s GroupSync) Reapply(ctx context.Context, groups []*resource.Resource) Reconciled { + var done Reconciled + for _, group := range groups { + // Only a group somebody WEARS. A reset puts back what the checkpoint + // held; it does not furnish the host with objects the checkpoint never + // had, and a rule set attached to no interface enforces nothing by + // definition. The first version wrote every restored group, and the + // runtime leg caught it at once: two rule sets left over from earlier + // suites, whose machines were long swept, appeared on the host and the + // verdict reported them as differences the reset had created. + // + // This is also why the audit's case is still covered: the group whose + // rule was revoked is worn by the two servers the suite started, so it + // is precisely the one re-applied. + if len(s.Wearers(group)) == 0 { + done.Skipped = append(done.Skipped, group.ID) + continue + } + if s.syncGroup(ctx, group, nil) { + done.Reapplied = append(done.Reapplied, group.ID) + continue + } + // Not Failed: a runtime that enforces nothing is the documented + // degraded mode, not an error, and the caller already knows whether + // one was asked. Skipped is the answer for "nobody could act on it". + done.Skipped = append(done.Skipped, group.ID) + } + return done } // ApplyMachine writes every set one resource wears and attaches them to its diff --git a/internal/core/machine/groupsync_reapply_test.go b/internal/core/machine/groupsync_reapply_test.go new file mode 100644 index 00000000..b3414c8b --- /dev/null +++ b/internal/core/machine/groupsync_reapply_test.go @@ -0,0 +1,85 @@ +package machine + +import ( + "slices" + "testing" + + "github.com/stephrobert/feint/internal/core/resource" +) + +// TestReapplyWritesTheWornSetsAndOnlyThose is cited by GroupSync.Reapply. +// +// ## What Reapply is for +// +// A reset restores the control plane and asks the runtime what it HOLDS, and +// Survey answers NAMES. A rule set whose content changed after the checkpoint +// keeps its name, so the observation cannot see the difference: the reset +// reports convergence while the runtime goes on enforcing rules the checkpoint +// does not describe. tools/conformance/scaleway/network.sh measured exactly +// that with a TCP connection, and this is the shared gesture that answers it. +// +// ## Why the wearer filter is asserted, and in the same test +// +// The first version wrote every restored group, and the runtime leg caught it +// in one pass: rule sets left over from earlier suites, whose machines were +// long swept, appeared on the host and the verdict reported them as +// differences the reset had created. A reset puts back what the checkpoint +// held; it does not furnish the host with objects the checkpoint never had, +// and a set attached to no interface enforces nothing by definition. +// +// The two halves are asserted together on purpose. A guard that refused every +// group would pass the second half alone, and would silently turn the whole +// re-application off — which is the defect this file exists to close. +func TestReapplyWritesTheWornSetsAndOnlyThose(t *testing.T) { + b := newGroupSyncBench() + worn := b.group("worn", "") + orphan := b.group("orphan", "") + b.machine("m-web", "10.42.0.9", "worn") + + done := b.sync().Reapply(t.Context(), []*resource.Resource{worn, orphan}) + + // The worn set reached the runtime, with its rules. + spec := ensuredSpec(t, b.rec, FirewallName("bench", "worn")) + if len(spec.Rules) == 0 { + t.Errorf("the re-applied set carries no rule: %+v", spec) + } + if !slices.Equal(done.Reapplied, []string{"worn"}) { + t.Errorf("reapplied %v; the group a machine wears is the one to put back", done.Reapplied) + } + + // And the orphan set did not, which is the half the host pays for. + for _, e := range b.rec.Events() { + if e.Kind == "EnsureFirewall" && e.Resource == FirewallName("bench", "orphan") { + t.Fatalf("a reset created a rule set nobody wears: the checkpoint never held it, "+ + "and the verdict above reports it as a difference the reset itself made (%v)", e) + } + } + if !slices.Equal(done.Skipped, []string{"orphan"}) { + t.Errorf("skipped %v; a group nobody could act on is reported, never passed over in silence", + done.Skipped) + } +} + +// TestReapplyOnNoRuntimeSaysSoRatherThanClaimingSuccess holds the degraded +// mode, which is the DEFAULT here: --vm off is what an ordinary run uses. +// +// Reporting these as Reapplied would have a reset tell an operator that rules +// are enforced again on a runtime that enforces nothing, which is the exact +// lie this emulator exists to avoid. Failed would be wrong too: nothing +// refused anything, and docs/limits.md documents the mode. +func TestReapplyOnNoRuntimeSaysSoRatherThanClaimingSuccess(t *testing.T) { + b := newGroupSyncBench() + group := b.group("worn", "") + b.machine("m-web", "10.42.0.9", "worn") + + sync := b.sync() + sync.Binding = Binding{Provider: "bench", Prefix: "feint-bench-", RuntimeKey: "machine"} + + done := sync.Reapply(t.Context(), []*resource.Resource{group}) + if len(done.Reapplied) != 0 || len(done.Failed) != 0 { + t.Errorf("a binding with no runtime reported %+v", done) + } + if !slices.Equal(done.Skipped, []string{"worn"}) { + t.Errorf("skipped %v", done.Skipped) + } +} diff --git a/internal/core/machine/reconcile.go b/internal/core/machine/reconcile.go index 38049bec..968d94bd 100644 --- a/internal/core/machine/reconcile.go +++ b/internal/core/machine/reconcile.go @@ -51,6 +51,11 @@ type Reconciled struct { // and a record nobody could act on need different answers from the // operator. Failed []string + // Reapplied names the rule sets written to the runtime again because the + // restore brought back the records they are translated from. Separate + // from Recreated: nothing was started, a set that the runtime already + // held under the same name was overwritten with the restored rules. + Reapplied []string // Skipped names records whose machine this binding does not own, or which // name no machine at all. Reported rather than silently passed over: a // record nobody could act on is what an operator needs to hear about. @@ -168,6 +173,21 @@ func (b Binding) ReconcileMissing(ctx context.Context, records []*resource.Resou if name == "" || slices.Contains(held, name) { continue } + // And the checkpoint must describe it as UP. A record keeps its + // machine name after the machine is gone — that is how ReconcileGone + // finds what to destroy — so "has a name and the host does not hold + // it" is also, word for word, the description of a server the + // checkpoint says is stopped. The runtime leg measured it: a reset + // returned a terminated VM to running and started a container for it, + // and the verdict then named the resource it had itself changed. + // + // The word is the pack's and the comparison is shared, which is the + // field this binding already carries for PowerOn to write. + // + // TestAResetLeavesAStoppedRecordStopped fails without this. + if res.State != b.RunningState { + continue + } if b.driver == nil { // Metadata only: PowerOn would answer running without starting // anything, which is the documented degraded mode and not diff --git a/internal/core/machine/reconcile_stopped_test.go b/internal/core/machine/reconcile_stopped_test.go new file mode 100644 index 00000000..17ae2f91 --- /dev/null +++ b/internal/core/machine/reconcile_stopped_test.go @@ -0,0 +1,73 @@ +package machine + +import ( + "slices" + "testing" + + "github.com/stephrobert/feint/internal/core/resource" +) + +// TestAResetLeavesAStoppedRecordStopped is cited by ReconcileMissing. +// +// ## The defect it closes, as it was measured +// +// A record keeps its machine name after the machine is gone — that is how +// ReconcileGone finds what to destroy — so "names a machine and the host does +// not hold it" is also, word for word, the description of a server the +// checkpoint says is stopped. The first version of this half read only those +// two facts, so a reset started a container for a terminated VM and then wrote +// `running` over the state the checkpoint held. +// +// The runtime leg said it plainly, and the shape of the complaint is worth +// keeping: the verdict reported, as differences the lab had failed to +// reconcile, the very resource the reset had itself changed. +// +// resource present and not in the checkpoint: vm i-a3f536ec state=running +// resource in the checkpoint and not present: vm i-a3f536ec state=terminated +// machine present and not in the checkpoint: feint-osc-i-a3f536ec +// +// ## Why the comparison is here and not in the packs +// +// Each cloud spells a running machine its own way, and the Binding already +// carries that word as a field because PowerOn writes it. So the word varies +// and the gesture does not, which is this repository's own test for where a +// line belongs. Written in each pack, the next pack would have been the one +// that forgot. +// +// The accepting half is asserted too: a guard that refused every record would +// pass the failing case above and silently turn the whole recreating half off. +func TestAResetLeavesAStoppedRecordStopped(t *testing.T) { + boot := func(res *resource.Resource) Boot { + return Boot{Image: "img", User: "root", Hostname: res.ID} + } + + stopped := absent("a") + stopped.State = "terminated" + running := absent("b") + + d := &startingDriver{} + done := bootBinding(d).ReconcileMissing(t.Context(), + []*resource.Resource{stopped, running}, nil, boot) + + // The refusing half: nothing was started for the stopped record, which is + // the assertion the host pays for. + if slices.Contains(d.started, "feint-acme-a") { + t.Errorf("a reset started a container for a record the checkpoint describes as stopped: %v", + d.started) + } + if stopped.State != "terminated" { + t.Errorf("the record says %q; a reset that rewrites the state it was asked to return to "+ + "then reports its own change as a difference", stopped.State) + } + if slices.Contains(done.Recreated, "feint-acme-a") { + t.Errorf("a stopped record was reported recreated: %+v", done) + } + + // The accepting half, in the same pass so neither can be satisfied alone. + if !slices.Contains(d.started, "feint-acme-b") { + t.Errorf("the running record was not started again: %v", d.started) + } + if !slices.Contains(done.Recreated, "feint-acme-b") { + t.Errorf("recreated %v", done.Recreated) + } +} diff --git a/internal/core/machine/surface.go b/internal/core/machine/surface.go index 3c09b261..d586a06f 100644 --- a/internal/core/machine/surface.go +++ b/internal/core/machine/surface.go @@ -184,6 +184,7 @@ func PackSurface() map[string]string { "GroupSync": "the firewall orchestrator, built from the pack's own translation of its groups", "GroupSync.SyncGroup": "write one group's rule set and replay it onto every wearer", "GroupSync.ApplyMachine": "write every set one resource wears and attach them in one call", + "GroupSync.Reapply": "write a restored group's rule set to the runtime again, so a reset puts the rule back on the NIC", "GroupSync.SyncReferrers": "re-expand the groups whose rules name the groups this resource wears", "GroupSync.Drop": "remove a deleted group's rule set from the runtime", "GroupSync.PlanOf": "declare the plan the scope of the groups is read from, the pack's own", diff --git a/internal/providers/exoscale/firewall.go b/internal/providers/exoscale/firewall.go index e78ecb5f..618ee834 100644 --- a/internal/providers/exoscale/firewall.go +++ b/internal/providers/exoscale/firewall.go @@ -257,3 +257,19 @@ func (p *Pack) removeFirewall(ctx context.Context, group *resource.Resource) { // internal/cli's TestEveryPackThatWiresTheFirewallSaysSo is what notices if // it does not. func (p *Pack) EnforcesFirewall() bool { return true } + +// ReconcileRules implements emulator.RuleReconciler: after a reset restores +// the control plane, this pack's groups are written to the runtime again. +// +// One line of its own, and the rest is machine.GroupSync.Reapply, shared with +// the two other packs. What varies is this: which restored record this +// provider calls a group. +func (p *Pack) ReconcileRules(ctx context.Context, records []*resource.Resource) machine.Reconciled { + groups := make([]*resource.Resource, 0, len(records)) + for _, res := range records { + if res.Kind == kindSecurityGroup { + groups = append(groups, res) + } + } + return p.groupSync().Reapply(ctx, groups) +} diff --git a/internal/providers/outscale/firewall.go b/internal/providers/outscale/firewall.go index 37b38809..06546a3d 100644 --- a/internal/providers/outscale/firewall.go +++ b/internal/providers/outscale/firewall.go @@ -246,3 +246,19 @@ func (p *Pack) removeFirewall(ctx context.Context, group *resource.Resource) { // internal/cli's TestEveryPackThatWiresTheFirewallSaysSo is what notices if // it does not. func (p *Pack) EnforcesFirewall() bool { return true } + +// ReconcileRules implements emulator.RuleReconciler: after a reset restores +// the control plane, this pack's groups are written to the runtime again. +// +// One line of its own, and the rest is machine.GroupSync.Reapply, shared with +// the two other packs. What varies is this: which restored record this +// provider calls a group. +func (p *Pack) ReconcileRules(ctx context.Context, records []*resource.Resource) machine.Reconciled { + groups := make([]*resource.Resource, 0, len(records)) + for _, res := range records { + if res.Kind == kindSecurityGroup { + groups = append(groups, res) + } + } + return p.groupSync().Reapply(ctx, groups) +} diff --git a/internal/providers/scaleway/firewall.go b/internal/providers/scaleway/firewall.go index bb5ebfb7..78370c5e 100644 --- a/internal/providers/scaleway/firewall.go +++ b/internal/providers/scaleway/firewall.go @@ -243,3 +243,19 @@ func (p *Pack) removeFirewall(ctx context.Context, group *resource.Resource) { // The day this pack stops handing rules over, this line is what has to change, // and TestEnforcementNamesOnlyThePacksThatWireIt is what notices if it does not. func (p *Pack) EnforcesFirewall() bool { return true } + +// ReconcileRules implements emulator.RuleReconciler: after a reset restores +// the control plane, this pack's groups are written to the runtime again. +// +// One line of its own, and the rest is machine.GroupSync.Reapply, shared with +// the two other packs. What varies is this: which restored record this +// provider calls a group. +func (p *Pack) ReconcileRules(ctx context.Context, records []*resource.Resource) machine.Reconciled { + groups := make([]*resource.Resource, 0, len(records)) + for _, res := range records { + if res.Kind == kindSecurityGroup { + groups = append(groups, res) + } + } + return p.groupSync().Reapply(ctx, groups) +} diff --git a/tools/conformance/scaleway/network.sh b/tools/conformance/scaleway/network.sh index 63ae6777..e7476009 100755 --- a/tools/conformance/scaleway/network.sh +++ b/tools/conformance/scaleway/network.sh @@ -272,6 +272,28 @@ rule_id="$(api -X POST "$ENDPOINT/instance/v1/zones/$ZONE/security_groups/$sg_id wait_until 30 reach || fail "port 80 is still refused after the rule was authorised" ok "port 80 reachable" +# ---- The audit's third P0, reproduced before being believed ----------------- +# An external audit deduced from the code that a reset could report convergence +# while a firewall rule was still missing from the runtime: `Survey` answers +# machine, network and rule-set NAMES, and a rule modified in place keeps its +# rule set's name, so nothing in the observation would see the difference. +# +# It said so as a hypothesis and had not reproduced it. Neither had anybody +# here. This measures it, with the only instrument whose verdict cannot be +# argued with: a TCP connection. +# +# The port is open at this point, so the checkpoint is taken with the rule in +# place. The suite then revokes the rule below and proves the port closes; the +# reset step after that asks whether the rule comes BACK. +fw_checkpoint=0 +if command -v "${FEINT_BIN:-./feint}" >/dev/null 2>&1; then + if "${FEINT_BIN:-./feint}" checkpoint firewall-reset --addr "${ENDPOINT#http://}" >/dev/null; then + fw_checkpoint=1 + else + skip "the checkpoint was refused; the reset step below is skipped" + fi +fi + echo "- revoking it closes it again" api -X DELETE "$ENDPOINT/instance/v1/zones/$ZONE/security_groups/$sg_id/rules/$rule_id" >/dev/null # waits on silence: the verdict below is drawn from a connection that does NOT @@ -283,6 +305,34 @@ sleep 3 if reach; then fail "port 80 is still open after the rule was revoked"; fi ok "port 80 refused again" +echo "- and a reset puts the rule back on the NIC, not only in the store" +if [ "${fw_checkpoint:-0}" = 1 ]; then + "${FEINT_BIN:-./feint}" reset firewall-reset --addr "${ENDPOINT#http://}" >/dev/null \ + || fail "the reset reported that the lab did not converge" + # The store has the rule again by construction: it was restored verbatim. + # What this asks is whether the PACKETS flow, which is the question `Survey` + # cannot answer and the one an operator has. + if wait_until 30 reach; then + ok "port 80 reachable again after the reset" + # The reset put the AUTHORISING rule back, which is exactly what it was + # asked for. Everything below measures a group that DROPS inbound, so the + # rule is revoked once more and the suite goes on from the state it + # expected. Without this the flexible-IP section reads the reset's own + # success as a firewall gap. + api -X DELETE "$ENDPOINT/instance/v1/zones/$ZONE/security_groups/$sg_id/rules/$rule_id" >/dev/null + # waits on silence: the verdict below is drawn from a connection that + # does NOT open, exactly as above the first revocation, and polling + # until the port closes would end at the first failed connection — + # which is also what a port that has not reopened yet looks like. + sleep 3 + if reach; then fail "port 80 is still open after the restored rule was revoked again"; fi + else + fail "the reset reported convergence and port 80 is still refused: the rule came back in the store and not on the NIC" + fi +else + skip "no checkpoint was taken, so the reset step measures nothing" +fi + # ---- Flexible IP ------------------------------------------------------------ # A public address a cloud hands out is routed to the machine holding it. Here it # comes from 203.0.113.0/24, which RFC 5737 reserves for documentation: it reads diff --git a/tools/falsify/specs/a-reset-is-not-a-restore.json b/tools/falsify/specs/a-reset-is-not-a-restore.json index 69347c3b..de0c462c 100644 --- a/tools/falsify/specs/a-reset-is-not-a-restore.json +++ b/tools/falsify/specs/a-reset-is-not-a-restore.json @@ -1,63 +1,116 @@ { - "package": "./internal/core/emulator/", - "note": "The subject is #853's own line: \"reset is not successful because JSON was restored\". `feint snapshot load` and `PUT /_feint/state` restore the control plane verbatim and touch nothing else, so a resource that disappears leaves its container running while the API no longer mentions it. The issue's last closing criterion asks for exactly this spec: \"a falsification that restores only the store but not runtime makes the test fail\".\n\nThe mutations are therefore all of the same family: each one turns the reset back into a restore, by a different route. The route matters, because each is a shape that would look reasonable in review — skipping the reconciliation loop, computing the difference by count instead of identity, handing a pack the resources that SURVIVED, or taking the \"before\" picture after the restore, when it is already gone.\n\nThe last one is the one worth reading twice. Reading the store after the restore instead of before is a one-line reordering that compiles, runs, reports a clean reset, and reconciles nothing at all — because by then there is no difference left to see.", - "mutations": [ - { - "label": "the destroying half stops being called, so a reset restores the control plane and leaves every machine the checkpoint does not hold running. This is the gap #853 names, reproduced exactly: the API no longer mentions the container and nobody takes it down", - "file": "internal/core/emulator/checkpoint.go", - "find": "\t\tif reconciler, ok := pack.(Reconciler); ok && len(mine) != 0 {", - "replace": "\t\tif reconciler, ok := pack.(Reconciler); ok && len(mine) != 0 && false {", - "test": "TestAResetDestroysWhatTheCheckpointDoesNotHold" - }, - { - "label": "the before picture is taken AFTER the restore instead of before, so there is no difference left to compute and the reset reconciles nothing while reporting a clean pass. A one-line reordering that compiles, runs and lies", - "file": "internal/core/emulator/checkpoint.go", - "find": "\tbefore := s.env.Store.All()\n\tif err := s.env.Store.Restore(bytes.NewReader(state)); err != nil {", - "replace": "\tbefore := []*resource.Resource(nil)\n\tif err := s.env.Store.Restore(bytes.NewReader(state)); err != nil {", - "test": "TestAResetDestroysWhatTheCheckpointDoesNotHold" - }, - { - "label": "each pack is handed ANOTHER pack's resources, so a reset destroys exactly the machines the checkpoint expects to keep and leaves the ones it does not. The opposite error of the first mutation and the expensive one: it takes down what the operator asked to come back", - "file": "internal/core/emulator/checkpoint.go", - "find": "\t\tif res.Tenant.Provider == provider {\n\t\t\tout = append(out, res)\n\t\t}", - "replace": "\t\tif res.Tenant.Provider != provider {\n\t\t\tout = append(out, res)\n\t\t}", - "test": "TestAResetDestroysWhatTheCheckpointDoesNotHold" - }, - { - "label": "the difference is computed by comparing identifiers only, dropping the kind and the provider, so two resources that share an id across products are taken for one another. Written as a narrowed key rather than a count because the count version does not compile, and the narrowed one is what a reviewer would wave through", - "file": "internal/core/emulator/checkpoint.go", - "find": "\treturn res.Tenant.Provider + \"\\x00\" + res.Kind + \"\\x00\" + res.ID", - "replace": "\treturn res.ID", - "test": "TestAResetDestroysWhatTheCheckpointDoesNotHold" - }, - { - "label": "the byte half of a checkpoint is skipped, so the store is named and the bytes are not. A later reset restores the control plane, finds no byte checkpoint, and an operator reads a clean reset over a store whose objects are whatever the last test left", - "file": "internal/core/emulator/checkpoint.go", - "find": "\tif s.env.Objects != nil {\n\t\tif err := s.env.Objects.SaveCheckpoint(r.Context(), name); err != nil {", - "replace": "\tif s.env.Objects != nil && false {\n\t\tif err := s.env.Objects.SaveCheckpoint(r.Context(), name); err != nil {", - "test": "TestACheckpointTakesBothHalvesOrNeither" - }, - { - "label": "a checkpoint name stops being validated, so a name that is a path component before it is a name reaches the byte store, which writes a directory per checkpoint. This repository's order is a refusal at intake rather than escaping later, and `..` is the whole reason", - "file": "internal/core/emulator/checkpoint.go", - "find": "\tif !validCheckpointName(name) {", - "replace": "\tif !validCheckpointName(name) && false {", - "test": "TestACheckpointTakesBothHalvesOrNeither" - }, - { - "label": "a host nobody can survey is taken for an empty one, so a reset offers every record for recreation and starts a second container beside each one already running. The defect is trusting the store about a question only the runtime can answer: the store is exactly what just changed", - "file": "internal/core/emulator/checkpoint.go", - "find": "\tleft, asked, err := env.machines.Survey(ctx)\n\tif !asked || err != nil {\n\t\treturn nil, false\n\t}", - "replace": "\tleft, asked, err := env.machines.Survey(ctx)\n\tif (!asked || err != nil) && false {\n\t\treturn nil, false\n\t}", - "test": "TestAResetAsksNothingOfAHostNobodyCanSurvey" - }, - { - "label": "the pack is handed the records and asked to start them without being told what the host holds, so every record looks missing. This is the version of the code that shipped for an hour: the core picked the missing records itself with a \"feint-\" prefix, which put a provider's vocabulary in the neutral layer and matched a test's own bookkeeping stamp, which it then tried to boot", - "file": "internal/core/machine/reconcile.go", - "find": "\t\tname := res.Runtime[b.RuntimeKey]\n\t\tif name == \"\" || slices.Contains(held, name) {\n\t\t\tcontinue\n\t\t}", - "replace": "\t\tname := res.Runtime[b.RuntimeKey]\n\t\tif name == \"\" || (slices.Contains(held, name) && false) {\n\t\t\tcontinue\n\t\t}", - "test": "TestReconcileStartsOnlyTheMachinesThatAreMissing", - "package": "./internal/core/machine/" - } - ] + "package": "./internal/core/emulator/", + "note": "The subject is #853's own line: \"reset is not successful because JSON was restored\". `feint snapshot load` and `PUT /_feint/state` restore the control plane verbatim and touch nothing else, so a resource that disappears leaves its container running while the API no longer mentions it. The issue's last closing criterion asks for exactly this spec: \"a falsification that restores only the store but not runtime makes the test fail\".\n\nThe mutations are therefore all of the same family: each one turns the reset back into a restore, by a different route. The route matters, because each is a shape that would look reasonable in review — skipping the reconciliation loop, computing the difference by count instead of identity, handing a pack the resources that SURVIVED, or taking the \"before\" picture after the restore, when it is already gone.\n\nThe last one is the one worth reading twice. Reading the store after the restore instead of before is a one-line reordering that compiles, runs, reports a clean reset, and reconciles nothing at all — because by then there is no difference left to see.\n\nSince an external audit of the merged code: the verdict's object comparison is in here too. It took the digest from ListObjectsV2's ETag, which internal/s3/list.go does not emit, so the field was always empty and two different contents under one key compared equal. The mutation that reproduces it is below, and the test that bites mounts a REAL door over a REAL store — the test that let the defect through built observedObject values by hand, with digests in them, so it exercised the comparison and never the observation.\n\nThird round, same audit: its P0-3, the firewall. The audit deduced from the code that a reset could report convergence while a rule was still missing from the runtime, and said so as a hypothesis it had not reproduced. The network suite reproduced it under --vm incus, with a TCP connection as the instrument, before a line of the fix was written: \"the reset reported convergence and port 80 is still refused: the rule came back in the store and not on the NIC\". Survey answers NAMES, and a rule set whose content changed keeps its name, so nothing in the observation can see the difference.\n\nThe first fix then over-corrected, and the same leg caught that too: writing every restored group put rule sets nobody wears back on the host, and the verdict reported them as differences the reset itself had made. Both directions are mutated below, because a guard that refuses everything passes the attack and breaks the product.\n\nThe fourth mutation is a defect the leg surfaced on the way, older than the audit: a record keeps its machine name after the machine is gone, so \"names a machine the host does not hold\" also describes a server the checkpoint says is stopped. A reset was starting containers for terminated VMs and then writing `running` over the state it had been asked to return to.", + "mutations": [ + { + "label": "the destroying half stops being called, so a reset restores the control plane and leaves every machine the checkpoint does not hold running. This is the gap #853 names, reproduced exactly: the API no longer mentions the container and nobody takes it down", + "file": "internal/core/emulator/checkpoint.go", + "find": "\t\tif reconciler, ok := pack.(Reconciler); ok && len(mine) != 0 {", + "replace": "\t\tif reconciler, ok := pack.(Reconciler); ok && len(mine) != 0 && false {", + "test": "TestAResetDestroysWhatTheCheckpointDoesNotHold" + }, + { + "label": "the before picture is taken AFTER the restore instead of before, so there is no difference left to compute and the reset reconciles nothing while reporting a clean pass. A one-line reordering that compiles, runs and lies", + "file": "internal/core/emulator/checkpoint.go", + "find": "\tbefore := s.env.Store.All()\n\tif err := s.env.Store.Restore(bytes.NewReader(state)); err != nil {", + "replace": "\tbefore := []*resource.Resource(nil)\n\tif err := s.env.Store.Restore(bytes.NewReader(state)); err != nil {", + "test": "TestAResetDestroysWhatTheCheckpointDoesNotHold" + }, + { + "label": "each pack is handed ANOTHER pack's resources, so a reset destroys exactly the machines the checkpoint expects to keep and leaves the ones it does not. The opposite error of the first mutation and the expensive one: it takes down what the operator asked to come back", + "file": "internal/core/emulator/checkpoint.go", + "find": "\t\tif res.Tenant.Provider == provider {\n\t\t\tout = append(out, res)\n\t\t}", + "replace": "\t\tif res.Tenant.Provider != provider {\n\t\t\tout = append(out, res)\n\t\t}", + "test": "TestAResetDestroysWhatTheCheckpointDoesNotHold" + }, + { + "label": "the difference is computed by comparing identifiers only, dropping the kind and the provider, so two resources that share an id across products are taken for one another. Written as a narrowed key rather than a count because the count version does not compile, and the narrowed one is what a reviewer would wave through", + "file": "internal/core/emulator/checkpoint.go", + "find": "\treturn res.Tenant.Provider + \"\\x00\" + res.Kind + \"\\x00\" + res.ID", + "replace": "\treturn res.ID", + "test": "TestAResetDestroysWhatTheCheckpointDoesNotHold" + }, + { + "label": "the byte half of a checkpoint is skipped, so the store is named and the bytes are not. A later reset restores the control plane, finds no byte checkpoint, and an operator reads a clean reset over a store whose objects are whatever the last test left", + "file": "internal/core/emulator/checkpoint.go", + "find": "\tif s.env.Objects != nil {\n\t\tif err := s.env.Objects.SaveCheckpoint(r.Context(), name); err != nil {", + "replace": "\tif s.env.Objects != nil && false {\n\t\tif err := s.env.Objects.SaveCheckpoint(r.Context(), name); err != nil {", + "test": "TestACheckpointTakesBothHalvesOrNeither" + }, + { + "label": "a checkpoint name stops being validated, so a name that is a path component before it is a name reaches the byte store, which writes a directory per checkpoint. This repository's order is a refusal at intake rather than escaping later, and `..` is the whole reason", + "file": "internal/core/emulator/checkpoint.go", + "find": "\tif !validCheckpointName(name) {", + "replace": "\tif !validCheckpointName(name) && false {", + "test": "TestACheckpointTakesBothHalvesOrNeither" + }, + { + "label": "a host nobody can survey is taken for an empty one, so a reset offers every record for recreation and starts a second container beside each one already running. The defect is trusting the store about a question only the runtime can answer: the store is exactly what just changed", + "file": "internal/core/emulator/checkpoint.go", + "find": "\tleft, asked, err := env.machines.Survey(ctx)\n\tif !asked || err != nil {\n\t\treturn nil, false\n\t}", + "replace": "\tleft, asked, err := env.machines.Survey(ctx)\n\tif (!asked || err != nil) && false {\n\t\treturn nil, false\n\t}", + "test": "TestAResetAsksNothingOfAHostNobodyCanSurvey" + }, + { + "label": "the pack is handed the records and asked to start them without being told what the host holds, so every record looks missing. This is the version of the code that shipped for an hour: the core picked the missing records itself with a \"feint-\" prefix, which put a provider's vocabulary in the neutral layer and matched a test's own bookkeeping stamp, which it then tried to boot", + "file": "internal/core/machine/reconcile.go", + "find": "\t\tname := res.Runtime[b.RuntimeKey]\n\t\tif name == \"\" || slices.Contains(held, name) {\n\t\t\tcontinue\n\t\t}", + "replace": "\t\tname := res.Runtime[b.RuntimeKey]\n\t\tif name == \"\" || (slices.Contains(held, name) && false) {\n\t\t\tcontinue\n\t\t}", + "test": "TestReconcileStartsOnlyTheMachinesThatAreMissing", + "package": "./internal/core/machine/" + }, + { + "label": "the object comparison goes back to the door, the bucket and the key, dropping the digest and both sizes. This IS the defect an audit found in merged code: with no content in the key, a reset reports convergence over bytes that changed, and the commit message claimed objects were compared by digest", + "file": "internal/cli/observable.go", + "find": "\t\treturn fmt.Sprintf(\"%s/%s/%s digest=%s bytes=%d listed=%d\",\n\t\t\tob.Door, ob.Bucket, ob.Key, ob.Digest, ob.Size, ob.Listed)", + "replace": "\t\treturn fmt.Sprintf(\"%s/%s/%s\",\n\t\t\tob.Door, ob.Bucket, ob.Key) + fmt.Sprint(ob.Digest, ob.Size, ob.Listed)[:0]", + "test": "TestTwoObjectsOfOneSizeAndDifferentBytesDoNotConverge", + "package": "./internal/cli/" + }, + { + "label": "the digest stops being computed over the bytes that arrived and becomes the empty string, which is exactly what reading an absent ETag produced. A verdict whose digest is constant compares every content equal, and the run reports a clean reset", + "file": "internal/cli/observable.go", + "find": "\treturn hex.EncodeToString(sum.Sum(nil)), size, nil", + "replace": "\treturn hex.EncodeToString(sum.Sum(nil))[:0], size, nil", + "test": "TestTwoObjectsOfOneSizeAndDifferentBytesDoNotConverge", + "package": "./internal/cli/" + }, + { + "label": "the two halves are taken in the order an audit caught: the bytes first, the control plane second. A failure in the second step then leaves an earlier checkpoint of that name holding its OLD store beside the NEW object references, and a later reset restores one against the other — while the comment above promises both halves or neither. The order IS the guard here, so the mutation reorders rather than neutralises", + "file": "internal/core/emulator/checkpoint.go", + "find": "\tvar state bytes.Buffer\n\tif err := s.env.Store.Snapshot(&state); err != nil {\n\t\twriteJSON(w, http.StatusInternalServerError, map[string]any{\n\t\t\t\"error\": \"the control plane could not be checkpointed: \" + err.Error(),\n\t\t})\n\t\treturn\n\t}\n\tif s.env.Objects != nil {\n\t\tif err := s.env.Objects.SaveCheckpoint(r.Context(), name); err != nil {\n\t\t\twriteJSON(w, http.StatusInternalServerError, map[string]any{\n\t\t\t\t\"error\": \"the bytes could not be checkpointed: \" + err.Error(),\n\t\t\t})\n\t\t\treturn\n\t\t}\n\t}", + "replace": "\tif s.env.Objects != nil {\n\t\tif err := s.env.Objects.SaveCheckpoint(r.Context(), name); err != nil {\n\t\t\twriteJSON(w, http.StatusInternalServerError, map[string]any{\n\t\t\t\t\"error\": \"the bytes could not be checkpointed: \" + err.Error(),\n\t\t\t})\n\t\t\treturn\n\t\t}\n\t}\n\tvar state bytes.Buffer\n\tif err := s.env.Store.Snapshot(&state); err != nil {\n\t\twriteJSON(w, http.StatusInternalServerError, map[string]any{\n\t\t\t\"error\": \"the control plane could not be checkpointed: \" + err.Error(),\n\t\t})\n\t\treturn\n\t}", + "test": "TestAFailedControlPlaneHalfLeavesTheEarlierBytesWhole" + }, + { + "label": "the firewall half stops being called, which is the audit's third P0 exactly: the store gets its rule back, the runtime keeps enforcing the one from after the checkpoint, and the verdict sees no difference because Survey answers names", + "file": "internal/core/emulator/checkpoint.go", + "find": "\t\tif rules, ok := pack.(RuleReconciler); ok && len(records) != 0 {", + "replace": "\t\tif rules, ok := pack.(RuleReconciler); ok && len(records) != 0 && false {", + "test": "TestAResetPutsTheRuleBackOnTheRuntimeAndNotOnlyInTheStore" + }, + { + "label": "the pack is only offered its records when something could be surveyed, which is the guard as it stood before the firewall half existed. Rule sets live on the runtime whether or not any machine wears one, so this skips the whole third half on a host nobody can survey", + "file": "internal/core/emulator/checkpoint.go", + "find": "\t\tif len(mine) == 0 && len(records) == 0 {", + "replace": "\t\tif len(mine) == 0 && (!surveyed || len(records) == 0) {", + "test": "TestAResetPutsTheRuleBackOnTheRuntimeAndNotOnlyInTheStore" + }, + { + "label": "the wearer filter goes, and a reset furnishes the host with rule sets the checkpoint never held. The runtime leg reported them as differences the reset had itself created, which is the over-correction half of the same fix", + "package": "./internal/core/machine/", + "file": "internal/core/machine/groupsync.go", + "find": "\t\tif len(s.Wearers(group)) == 0 {", + "replace": "\t\tif len(s.Wearers(group)) == 0 && false {", + "test": "TestReapplyWritesTheWornSetsAndOnlyThose" + }, + { + "label": "a reset starts again the machine of a record the checkpoint describes as stopped, then writes `running` over the state it was asked to return to. The verdict that follows names, as a difference it failed to reconcile, the very resource it changed", + "package": "./internal/core/machine/", + "file": "internal/core/machine/reconcile.go", + "find": "\t\tif res.State != b.RunningState {", + "replace": "\t\tif res.State != b.RunningState && false {", + "test": "TestAResetLeavesAStoppedRecordStopped" + } + ] } diff --git a/tools/falsify/specs/a-run-owns-its-lab.json b/tools/falsify/specs/a-run-owns-its-lab.json index eff81685..9b9d1131 100644 --- a/tools/falsify/specs/a-run-owns-its-lab.json +++ b/tools/falsify/specs/a-run-owns-its-lab.json @@ -96,9 +96,23 @@ { "label": "a reset that did not converge stops failing the command, so a shared emulator is left dirty by a run that reported success. The caller asked for the lab to be left as found; a quiet 0 is how the next test inherits the mess and blames itself", "file": "internal/cli/run.go", - "find": "\tif resetVerdict == \"did not converge\" && code == exitOK {", - "replace": "\tif resetVerdict == \"did not converge\" && code == exitOK && false {", + "find": "\tif code == exitOK && !deliveredWhatWasAsked(resetVerdict, cleanup) {", + "replace": "\tif code == exitOK && !deliveredWhatWasAsked(resetVerdict, cleanup) && false {", "test": "TestARunWhoseResetDidNotConvergeFails" + }, + { + "label": "only a non-converging reset fails the run, which is the contract error an audit found: a reset the emulator refused, one that could not be read back, and one with no checkpoint to return to all left the exit code at the child's, so a CI run could read `terraform test success / feint reset failed / feint run exit 0`", + "file": "internal/cli/run.go", + "find": "\tcase \"\", \"converged\":", + "replace": "\tcase \"\", \"converged\", \"failed\", \"unverified\", \"not attempted\":", + "test": "TestAGuaranteeThatWasNotDeliveredFailsTheRun" + }, + { + "label": "a lab this command owned and could not stop stops failing the run, so a leaked port and a leaked directory report success with `cleanup: failed` in a receipt nobody reads. The receipt said it and the exit code did not, which is the half that reaches CI", + "file": "internal/cli/run.go", + "find": "\tswitch cleanup {\n\tcase \"clean\", \"kept\", \"not owned\":", + "replace": "\tswitch cleanup {\n\tcase \"clean\", \"kept\", \"not owned\", \"failed\":", + "test": "TestAGuaranteeThatWasNotDeliveredFailsTheRun" } ] }