amadan
boxvolumes: discharge refuses foreign or odd records, detaching volumes, interior gaps and times that do not fit
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 files changed,
+79
−11
internal/boxvolumes/volumes.go+27 −11internal/boxvolumes/volumes_test.go+52 −0
diff --git a/internal/boxvolumes/volumes.go b/internal/boxvolumes/volumes.go| index ec05e0d18..5941e53ce 100644 |
| --- a/internal/boxvolumes/volumes.go |
| +++ b/internal/boxvolumes/volumes.go |
| @@ -289,6 +289,10 @@ func incomplete(r Record) string { |
| if d.VolumeID == "" || d.Device == "" { |
| return "lists a volume with no id or device" |
| } |
| + // A volume that is detaching or detached may outlive the box. |
| + if d.Status != "attached" && d.Status != "attaching" { |
| + return fmt.Sprintf("lists volume %s with status %q", d.VolumeID, d.Status) |
| + } |
| } |
| return "" |
| } |
| @@ -304,32 +308,38 @@ type Verdict struct { |
| // volume e's instance had attached, from the first record taken within |
| // EnrollmentWindow of enrolledAt to the last, taken within LastRecordWindow |
| // of terminatedAt (the fence's observed_at), was DeleteOnTermination, with |
| -// none ever removed (spec §5.9, H19). Records for another enrollment or |
| -// another instance are ignored; a record of this enrollment's that is |
| -// incomplete, carries no mapping, or has a version this binary does not |
| -// read keeps the chores open. Anything short of that keeps the chores |
| -// open for an operator, which costs cleanup and never recovery. |
| +// none ever removed (spec §5.9, H19). Every record is checked, none |
| +// skipped: one with another version, enrollment or cloud identity, one |
| +// that is incomplete or carries no mapping, a volume not attached or |
| +// attaching, a record outside enrolledAt to terminatedAt, or a gap between |
| +// records over LastRecordWindow keeps the chores open, as do missing or |
| +// misordered times. Anything short of that keeps the chores open for an |
| +// operator, which costs cleanup and never recovery. |
| func Discharge(records []Record, e Entry, enrolledAt, terminatedAt time.Time) Verdict { |
| no := func(format string, a ...any) Verdict { return Verdict{Reason: fmt.Sprintf(format, a...)} } |
| if !e.Cloud.Complete() || e.Enrollment == "" { |
| return no("the roster entry has no proven cloud") |
| } |
| + if enrolledAt.IsZero() || terminatedAt.IsZero() || terminatedAt.Before(enrolledAt) { |
| + return no("the enrollment and termination times are missing or out of order") |
| + } |
| type stamped struct { |
| at time.Time |
| rec Record |
| } |
| var mine []stamped |
| for _, r := range records { |
| - if r.Enrollment != e.Enrollment { |
| - continue |
| - } |
| - // A version this binary does not read could hold the adverse |
| - // record; it is never skipped. |
| + // Every record listed under this enrollment's prefix is checked, |
| + // none skipped: one that is not this enrollment's, this instance's |
| + // or a version this binary reads could be the adverse record. |
| if r.V != 1 { |
| return no("a record for %s has version %d", e.Enrollment, r.V) |
| } |
| + if r.Enrollment != e.Enrollment { |
| + return no("a record listed for %s names enrollment %q", e.Enrollment, r.Enrollment) |
| + } |
| if r.Cloud != e.Cloud { |
| - continue // another instance's (spec §5.9: ignored) |
| + return no("a record for %s names another cloud identity", e.Enrollment) |
| } |
| if reason := incomplete(r); reason != "" { |
| return no("the record observed at %q %s", r.ObservedAt, reason) |
| @@ -351,6 +361,12 @@ func Discharge(records []Record, e Entry, enrolledAt, terminatedAt time.Time) Ve |
| return no("the newest record (%s) is more than %s before the termination", last.Format(time.RFC3339), LastRecordWindow) |
| } |
| for i, s := range mine { |
| + if s.at.Before(enrolledAt) || s.at.After(terminatedAt) { |
| + return no("the record observed at %s lies outside enrollment to termination", s.rec.ObservedAt) |
| + } |
| + if i > 0 && s.at.Sub(mine[i-1].at) > LastRecordWindow { |
| + return no("no record between %s and %s, more than %s", mine[i-1].rec.ObservedAt, s.rec.ObservedAt, LastRecordWindow) |
| + } |
| for _, d := range s.rec.BlockDevices { |
| if !d.DeleteOnTermination { |
| return no("volume %s was not DeleteOnTermination at %s", d.VolumeID, s.rec.ObservedAt) |
diff --git a/internal/boxvolumes/volumes_test.go b/internal/boxvolumes/volumes_test.go| index b3dd7678d..d3e264ab3 100644 |
| --- a/internal/boxvolumes/volumes_test.go |
| +++ b/internal/boxvolumes/volumes_test.go |
| @@ -3,6 +3,7 @@ package boxvolumes |
| import ( |
| "context" |
| "errors" |
| + "strings" |
| "testing" |
| "time" |
| @@ -238,6 +239,21 @@ func TestDischargeRefusesStoredRecordsThatAreNotEvidence(t *testing.T) { |
| "a terminated read with its root mapping": {`{"v":1,` + after + `,"instance_state":"terminated","block_devices":[{"device":"/dev/xvda","volume_id":"vol-root","delete_on_termination":true,"status":"attached"}],"removed_volumes":[]}`, 5 * time.Minute}, |
| "a volume with no device": {`{"v":1,` + after + `,"instance_state":"running","block_devices":[{"device":"/dev/xvda","volume_id":"vol-root","delete_on_termination":true,"status":"attached"},{"device":"","volume_id":"vol-data","delete_on_termination":true,"status":"attached"}],"removed_volumes":[]}`, 5 * time.Minute}, |
| } |
| + const goodAfter = `{"v":1,` + after + `,"instance_state":"running","block_devices":[{"device":"/dev/xvda","volume_id":"vol-root","delete_on_termination":true,"status":"attached"}],"removed_volumes":[]}` |
| + swap := func(old, new string) string { |
| + if !strings.Contains(goodAfter, old) { |
| + t.Fatalf("no %q in the template", old) |
| + } |
| + return strings.Replace(goodAfter, old, new, 1) |
| + } |
| + cases["a detaching volume"] = bad{swap(`"status":"attached"`, `"status":"detaching"`), 5 * time.Minute} |
| + cases["a detached volume"] = bad{swap(`"status":"attached"`, `"status":"detached"`), 5 * time.Minute} |
| + cases["no status"] = bad{swap(`"status":"attached"`, `"status":""`), 5 * time.Minute} |
| + cases["no enrollment"] = bad{swap(`"enrollment":"enr-annie-1"`, `"enrollment":""`), 5 * time.Minute} |
| + cases["another enrollment"] = bad{swap(`"enrollment":"enr-annie-1"`, `"enrollment":"enr-other"`), 5 * time.Minute} |
| + cases["another instance"] = bad{swap(`"instance_id":"i-0abc"`, `"instance_id":"i-0zzz"`), 5 * time.Minute} |
| + cases["a region in another case"] = bad{swap(`"region":"eu-west-1"`, `"region":"EU-WEST-1"`), 5 * time.Minute} |
| + cases["an unlabelled version 2 record"] = bad{`{"v":2}`, 5 * time.Minute} |
| for name, tc := range cases { |
| t.Run(name, func(t *testing.T) { |
| st := release.NewDirStore(t.TempDir()) |
| @@ -269,3 +285,39 @@ func TestDischargeRefusesStoredRecordsThatAreNotEvidence(t *testing.T) { |
| t.Fatalf("a stored good record: %+v", v) |
| } |
| } |
| + |
| +// A gap longer than LastRecordWindow between two records is a stretch the |
| +// recorder did not cover. |
| +func TestDischargeRefusesAnInteriorGap(t *testing.T) { |
| + a, _ := Next(nil, entry, writer, observe(t, reply("111122223333", types.InstanceStateNameRunning, root)), t0) |
| + late := t0.Add(30 * 24 * time.Hour) |
| + b, _ := Next(&a, entry, writer, observe(t, reply("111122223333", types.InstanceStateNameRunning, root)), late) |
| + if v := Discharge([]Record{a, b}, entry, t0, late.Add(time.Hour)); v.Discharge || v.Reason == "" { |
| + t.Fatalf("a 30 day gap discharged: %+v", v) |
| + } |
| + c, _ := Next(&a, entry, writer, observe(t, reply("111122223333", types.InstanceStateNameRunning, root)), t0.Add(LastRecordWindow)) |
| + if v := Discharge([]Record{a, c}, entry, t0, t0.Add(LastRecordWindow)); !v.Discharge { |
| + t.Fatalf("a gap of exactly the window: %+v", v) |
| + } |
| +} |
| + |
| +// Records must lie between enrollment and termination, and both times must |
| +// be real and in order. |
| +func TestDischargeRefusesTimesThatDoNotFit(t *testing.T) { |
| + recs := records(t, []vol{root}, []vol{root}) // t0, t0+10m |
| + var zero time.Time |
| + cases := map[string]struct{ enrolled, terminated time.Time }{ |
| + "zero termination": {t0, zero}, |
| + "zero enrollment": {zero, t0.Add(time.Hour)}, |
| + "terminated before enrolled": {t0.Add(time.Hour), t0}, |
| + "a record before enrollment": {t0.Add(time.Minute), t0.Add(time.Hour)}, |
| + "a record after termination": {t0, t0.Add(5 * time.Minute)}, |
| + } |
| + for name, tc := range cases { |
| + t.Run(name, func(t *testing.T) { |
| + if v := Discharge(recs, entry, tc.enrolled, tc.terminated); v.Discharge || v.Reason == "" { |
| + t.Fatalf("verdict = %+v", v) |
| + } |
| + }) |
| + } |
| +} |