diff --git a/spinifex/daemon/dns_reconcile.go b/spinifex/daemon/dns_reconcile.go index bc46bfd9f..f3f90ba30 100644 --- a/spinifex/daemon/dns_reconcile.go +++ b/spinifex/daemon/dns_reconcile.go @@ -2,6 +2,7 @@ package daemon import ( "context" + "log/slog" "github.com/aws/aws-sdk-go/aws" handlers_dns "github.com/mulgadc/spinifex/spinifex/handlers/dns" @@ -10,7 +11,6 @@ import ( handlers_rds "github.com/mulgadc/spinifex/spinifex/handlers/rds" "github.com/mulgadc/spinifex/spinifex/kvstore" "github.com/mulgadc/spinifex/spinifex/reconciler" - "github.com/mulgadc/spinifex/spinifex/vm" ) // dnsWatchSources names the buckets whose changes should wake the DNS @@ -81,7 +81,10 @@ func (d *Daemon) rdsWatchBuckets(ctx context.Context) ([]*kvstore.Bucket, error) // records — the reconcile only ever repairs, never over-prunes, on a partial view. func (d *Daemon) dnsDesiredSet() handlers_dns.DesiredSet { ds := handlers_dns.DesiredSet{} - ds.Changes = append(ds.Changes, d.desiredEC2DNSChanges()...) + if ch, ok := d.desiredEC2DNSChanges(); ok { + ds.Changes = append(ds.Changes, ch...) + ds.Prunable.EC2 = true + } if d.elbv2Service != nil { if ch, ok := d.elbv2Service.DesiredDNSChanges(); ok { @@ -104,29 +107,46 @@ func (d *Daemon) dnsDesiredSet() handlers_dns.DesiredSet { return ds } -// desiredEC2DNSChanges returns UPSERTs for this node's running instances. EC2 -// records are node-local — vmMgr holds only this node's VMs — so they are never -// pruned by the reconcile; the terminate hook owns EC2 record removal. The -// domains mirror the lifecycle publish so re-asserting is a no-op when in sync. -func (d *Daemon) desiredEC2DNSChanges() []handlers_dns.Change { +// desiredEC2DNSChanges returns UPSERTs for every running instance in the +// cluster, and whether that view was complete. The instance record key space +// spans all nodes, unlike the vmMgr map it replaces here, which is what lets +// the reconcile prune a record as well as repair one. The domains mirror the +// lifecycle publish so re-asserting is a no-op when in sync. +func (d *Daemon) desiredEC2DNSChanges() ([]handlers_dns.Change, bool) { + if d.jsManager == nil { + return nil, false + } + records, err := d.jsManager.ListInstanceRecords() + if err != nil { + // Reported without changes: a partial instance view is exactly the case + // pruning must not run on, and repairing from it would be repairing from + // the same partial view. + slog.Warn("dns reconcile: instance records unavailable, EC2 records left alone this cycle", + "error", err) + return nil, false + } + var changes []handlers_dns.Change - d.vmMgr.View(func(vms map[string]*vm.VM) { - for _, v := range vms { - if v == nil || v.Status != vm.StateRunning { - continue - } - privateIP := "" - if v.Instance != nil { - privateIP = aws.StringValue(v.Instance.PrivateIpAddress) - } - if v.PublicIP == "" && privateIP == "" { - continue - } - changes = append(changes, handlers_dns.EC2Changes( - handlers_dns.ActionUpsert, d.config.Region, - d.dnsBaseDomain, d.dnsInternalDomain, v.PublicIP, privateIP, - )...) + for _, record := range records { + // Stopped instances keep their addresses and their names, which is why + // the state test is the same one the lifecycle publish withdraws on. + if record == nil || !handlers_dns.InstanceRetainsRecords(record.Status.Status) { + continue } - }) - return changes + if record.Metadata.DeletionTimestamp != nil { + continue + } + privateIP := "" + if record.Status.Instance != nil { + privateIP = aws.StringValue(record.Status.Instance.PrivateIpAddress) + } + if record.Status.PublicIP == "" && privateIP == "" { + continue + } + changes = append(changes, handlers_dns.EC2Changes( + handlers_dns.ActionUpsert, d.config.Region, + d.dnsBaseDomain, d.dnsInternalDomain, record.Status.PublicIP, privateIP, + )...) + } + return changes, true } diff --git a/spinifex/daemon/dns_reconcile_ec2_test.go b/spinifex/daemon/dns_reconcile_ec2_test.go new file mode 100644 index 000000000..12c41055e --- /dev/null +++ b/spinifex/daemon/dns_reconcile_ec2_test.go @@ -0,0 +1,170 @@ +//test:in-package — these build *Daemon and *JetStreamManager literals to drive +//the desired-set builder against a seeded record store, which is only possible +//from inside the package. + +package daemon + +import ( + "testing" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/config" + handlers_dns "github.com/mulgadc/spinifex/spinifex/handlers/dns" + "github.com/mulgadc/spinifex/spinifex/kvstore" + "github.com/mulgadc/spinifex/spinifex/testutil" + "github.com/mulgadc/spinifex/spinifex/vm" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// dnsTestDaemon returns a daemon whose instance records live in a real KV +// bucket, plus a writer for seeding it. vmMgr is empty on purpose: the desired +// set must not read it. +func dnsTestDaemon(t *testing.T) (*Daemon, func(id, publicIP, privateIP string, state vm.InstanceState)) { + t.Helper() + _, _, js := testutil.StartTestJetStream(t) + store := kvstore.New[vm.InstanceRecord](js, kvstore.Config{ + Name: InstanceStateBucket, History: 1, Replicas: 1, + }) + d := &Daemon{ + jsManager: &JetStreamManager{js: js, records: store, replicas: 1}, + vmMgr: vm.NewManager(), + config: &config.Config{Region: "ap-southeast-2"}, + dnsBaseDomain: "spx3.net", + dnsInternalDomain: "compute.internal", + } + seed := func(id, publicIP, privateIP string, state vm.InstanceState) { + t.Helper() + record := &vm.InstanceRecord{} + record.Metadata.Name = id + record.Status.Status = state + record.Status.PublicIP = publicIP + if privateIP != "" { + record.Status.Instance = &ec2.Instance{PrivateIpAddress: aws.String(privateIP)} + } + require.NoError(t, d.jsManager.WriteInstanceRecord(id, record)) + } + return d, seed +} + +func changeNames(changes []handlers_dns.Change) []string { + out := make([]string, 0, len(changes)) + for _, c := range changes { + out = append(out, c.Name) + } + return out +} + +// The desired set has to span every node, because that is the whole basis on +// which the reconcile is allowed to delete an instance record. A record written +// by another node is indistinguishable here from one written locally, which is +// the point. +func TestDesiredEC2DNSChanges_SpansEveryNode(t *testing.T) { + d, seed := dnsTestDaemon(t) + seed("i-local", "203.0.113.10", "172.31.0.10", vm.StateRunning) + seed("i-remote", "203.0.113.20", "172.31.0.20", vm.StateRunning) + + changes, ok := d.desiredEC2DNSChanges() + require.True(t, ok, "a successful list is a cluster-wide view and carries prune authority") + assert.ElementsMatch(t, []string{ + "ec2-203-0-113-10.ap-southeast-2.compute.spx3.net", + "ip-172-31-0-10.ap-southeast-2.compute.internal", + "ec2-203-0-113-20.ap-southeast-2.compute.spx3.net", + "ip-172-31-0-20.ap-southeast-2.compute.internal", + }, changeNames(changes)) +} + +// The mutation check for the change itself: a VM present only in this node's +// in-memory manager must contribute nothing. Reading vmMgr is what made the +// view node-local and pruning unsafe, so a fallback to it would quietly restore +// the old behaviour while the prune authority claimed otherwise. +func TestDesiredEC2DNSChanges_IgnoresTheLocalVMManager(t *testing.T) { + d, _ := dnsTestDaemon(t) + d.vmMgr.Replace(map[string]*vm.VM{ + "i-inmemory": { + ID: "i-inmemory", + Status: vm.StateRunning, + PublicIP: "203.0.113.99", + Instance: &ec2.Instance{PrivateIpAddress: aws.String("172.31.0.99")}, + }, + }) + + changes, ok := d.desiredEC2DNSChanges() + require.True(t, ok) + assert.Empty(t, changes, "an instance with no record is not part of the cluster-wide view") +} + +// A stopped instance keeps its addresses, so it keeps its names: the lifecycle +// publish withdraws records on terminate and deliberately not on stop. Now that +// absence from this set is a deletion, the two have to agree — a stopped +// instance missing here would have its name pruned out from under it. +func TestDesiredEC2DNSChanges_KeepsStoppedInstancesAndDropsTerminated(t *testing.T) { + d, seed := dnsTestDaemon(t) + seed("i-up", "203.0.113.10", "172.31.0.10", vm.StateRunning) + seed("i-stopping", "203.0.113.11", "172.31.0.11", vm.StateStopping) + seed("i-stopped", "203.0.113.12", "172.31.0.12", vm.StateStopped) + seed("i-gone", "203.0.113.13", "172.31.0.13", vm.StateTerminated) + + changes, ok := d.desiredEC2DNSChanges() + require.True(t, ok) + names := changeNames(changes) + assert.ElementsMatch(t, []string{ + "ec2-203-0-113-10.ap-southeast-2.compute.spx3.net", + "ip-172-31-0-10.ap-southeast-2.compute.internal", + "ec2-203-0-113-11.ap-southeast-2.compute.spx3.net", + "ip-172-31-0-11.ap-southeast-2.compute.internal", + "ec2-203-0-113-12.ap-southeast-2.compute.spx3.net", + "ip-172-31-0-12.ap-southeast-2.compute.internal", + }, names) + assert.NotContains(t, names, "ec2-203-0-113-13.ap-southeast-2.compute.spx3.net") +} + +// An instance on its way out is not desired state, even while its record is +// still readable: re-upserting it would race the withdrawal the terminate path +// already published. +func TestDesiredEC2DNSChanges_SkipsARecordMarkedForDeletion(t *testing.T) { + d, seed := dnsTestDaemon(t) + seed("i-doomed", "203.0.113.10", "172.31.0.10", vm.StateRunning) + _, err := d.jsManager.UpdateInstanceRecord("i-doomed", func(r *vm.InstanceRecord) { + now := time.Now() + r.Metadata.DeletionTimestamp = &now + }) + require.NoError(t, err) + + changes, ok := d.desiredEC2DNSChanges() + require.True(t, ok) + assert.Empty(t, changes) +} + +// A view that could not be read is the case pruning exists to be protected +// from. It must report no authority, and contribute no records either: repairing +// from a partial view is repairing from the same partial view. +func TestDesiredEC2DNSChanges_UnreadableStoreYieldsNoAuthority(t *testing.T) { + for name, d := range map[string]*Daemon{ + "no jsManager": {}, + "no record store": {jsManager: &JetStreamManager{}}, + } { + t.Run(name, func(t *testing.T) { + changes, ok := d.desiredEC2DNSChanges() + assert.False(t, ok, "an unread instance view must not grant prune authority") + assert.Empty(t, changes) + }) + } +} + +// The flag and the records travel together: a desired set carrying EC2 changes +// without EC2 authority would repair but never prune, and one carrying the flag +// without the changes would prune everything. +func TestDNSDesiredSet_EC2AuthorityFollowsTheRecords(t *testing.T) { + d, seed := dnsTestDaemon(t) + seed("i-up", "203.0.113.10", "172.31.0.10", vm.StateRunning) + + ds := d.dnsDesiredSet() + assert.True(t, ds.Prunable.EC2) + assert.NotEmpty(t, ds.Changes) + + assert.False(t, (&Daemon{}).dnsDesiredSet().Prunable.EC2, + "a daemon that could not read any instance record claims no authority") +} diff --git a/spinifex/handlers/dns/naming.go b/spinifex/handlers/dns/naming.go index 17dbf5323..5b9da2591 100644 --- a/spinifex/handlers/dns/naming.go +++ b/spinifex/handlers/dns/naming.go @@ -3,24 +3,42 @@ package dns import ( "fmt" "strings" + + "github.com/mulgadc/spinifex/spinifex/vm" ) +// InstanceRetainsRecords reports whether an instance in this state still owns +// its A records. Stop and start retain the addresses, so they retain the names; +// anything past stopped no longer resolves and its records are withdrawn. +func InstanceRetainsRecords(status vm.InstanceState) bool { + return status == vm.StateRunning || status == vm.StateStopping || status == vm.StateStopped +} + // dashIP renders an IP for AWS-style hostnames (1.2.3.4 → 1-2-3-4). func dashIP(ip string) string { return strings.ReplaceAll(ip, ".", "-") } +const ( + // ec2PublicLabelPrefix and ec2PrivateLabelPrefix open every instance record + // name. The reconcile matches on them to recognise a record as an instance + // record, so they are defined alongside the names they build. + ec2PublicLabelPrefix = "ec2-" + ec2PrivateLabelPrefix = "ip-" +) + // EC2PublicName is the public AWS-shaped name for an instance: // ec2-{dashed-public-ip}.{region}.compute.{baseDomain}. func EC2PublicName(publicIP, region, baseDomain string) string { - return fmt.Sprintf("ec2-%s.%s.compute.%s", dashIP(publicIP), region, baseDomain) + return fmt.Sprintf("%s%s.%s.compute.%s", ec2PublicLabelPrefix, dashIP(publicIP), region, baseDomain) } // EC2PrivateName is the private AWS-parity name for an instance: // ip-{dashed-private-ip}.{region}.{internalDomain} (IMDS synthHostname). The // internal domain defaults to PrivateZone when empty. func EC2PrivateName(privateIP, region, internalDomain string) string { - return fmt.Sprintf("ip-%s.%s.%s", dashIP(privateIP), region, privateZoneOrDefault(internalDomain)) + return fmt.Sprintf("%s%s.%s.%s", ec2PrivateLabelPrefix, dashIP(privateIP), region, + privateZoneOrDefault(internalDomain)) } // EC2DNSNames returns the public and private AWS-shaped DNS names for an diff --git a/spinifex/handlers/dns/naming_test.go b/spinifex/handlers/dns/naming_test.go index 4b0a78938..af5f06f43 100644 --- a/spinifex/handlers/dns/naming_test.go +++ b/spinifex/handlers/dns/naming_test.go @@ -3,10 +3,34 @@ package dns import ( "testing" + "github.com/mulgadc/spinifex/spinifex/vm" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +// Two callers read this predicate in opposite directions: the terminate hook +// withdraws when it is false, and the reconcile's desired set omits — and so +// prunes — when it is false. They have to agree on every state. +func TestInstanceRetainsRecords(t *testing.T) { + tests := []struct { + name string + status vm.InstanceState + want bool + }{ + {name: "running", status: vm.StateRunning, want: true}, + {name: "stopping", status: vm.StateStopping, want: true}, + {name: "stopped", status: vm.StateStopped, want: true}, + {name: "shutting down", status: vm.StateShuttingDown, want: false}, + {name: "terminated", status: vm.StateTerminated, want: false}, + {name: "error", status: vm.StateError, want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, InstanceRetainsRecords(tt.status)) + }) + } +} + func TestEC2Names(t *testing.T) { assert.Equal(t, "ec2-1-2-3-4.ap-southeast-2.compute.spx3.net", EC2PublicName("1.2.3.4", "ap-southeast-2", "spx3.net")) diff --git a/spinifex/handlers/dns/reconcile.go b/spinifex/handlers/dns/reconcile.go index 4a182f366..ec320f766 100644 --- a/spinifex/handlers/dns/reconcile.go +++ b/spinifex/handlers/dns/reconcile.go @@ -57,6 +57,11 @@ type PruneScope struct { ELB bool EKS bool RDS bool + + // EC2 is set only when the instance view spanned every node. A node's own + // VM manager does not qualify: pruning against it would delete the records + // of every instance running elsewhere. + EC2 bool } // Reconciler converges the zone toward the live inventory. On each pass it @@ -70,21 +75,24 @@ type PruneScope struct { // the queue group load-balances rather than serialises, an idle cluster raced // its own zone object once per cycle. // -// Only cluster-wide-enumerable records (load balancers, EKS clusters) are -// pruned: any node sees the full ELB/EKS set from KV. EC2 records are never -// pruned here because a node's vmMgr holds only its own instances — an -// incomplete view would delete another node's records. EC2 removal stays with -// the terminate hook; the reconcile only repairs missing/incorrect EC2 records. +// Only cluster-wide-enumerable records are pruned, and only for the classes a +// cycle enumerated in full. Every class the desired set carries is readable +// from KV across the whole cluster, so a pass that reads them all can prune +// them all; one that could not read a class repairs it without pruning it. type Reconciler struct { enabled bool s3cfg *nsconfig.S3Config baseDomain string - nc *nats.Conn - desired DesiredFunc - interval time.Duration - accountID string - holder string - sources []reconciler.Source + // internalDomain is the private zone instance records land in. It is read + // as well as the base domain because pruning one zone and not the other + // leaves every stale private record behind. + internalDomain string + nc *nats.Conn + desired DesiredFunc + interval time.Duration + accountID string + holder string + sources []reconciler.Source } // NewReconciler builds the drift backstop. It is disabled (a no-op) when @@ -110,6 +118,7 @@ func NewReconciler(cfg *config.Config, nc *nats.Conn, desired DesiredFunc, sourc r.enabled = true r.s3cfg = zoneCfg.s3 r.baseDomain = strings.TrimSpace(zoneCfg.server.DefaultDomain) + r.internalDomain = ResolveInternalDomain(cfg) return r } @@ -280,31 +289,50 @@ func splitReconcileChanges(changes []Change, payloadLimit int) ([][]Change, erro return batches, nil } -// computeBatch reads the base zone (the only zone holding prunable ELB/EKS -// records) and converges the desired set against it. +// computeBatch reads every zone holding prunable records and converges the +// desired set against them. That is the base domain, plus the private zone once +// instance records are prunable — a stale private record is as wrong as a stale +// public one, and only this zone holds it. func (r *Reconciler) computeBatch() ([]Change, error) { ds := r.desired() existing := map[string][]zoneRecord{} - recs, ok, err := r.readZone(r.baseDomain) - if err != nil { - return nil, err + zones := []string{r.baseDomain} + if ds.Prunable.EC2 { + zones = append(zones, privateZoneOrDefault(r.internalDomain)) } - if ok { - existing[r.baseDomain] = recs + for _, zone := range zones { + if _, seen := existing[zone]; seen || zone == "" { + continue + } + recs, ok, err := r.readZone(zone) + if err != nil { + return nil, err + } + if ok { + existing[zone] = recs + } } return computeConverge(ds.Changes, existing, r.prunable(ds.Prunable)) } // prunable returns the predicate deciding whether a (zone, label) record may be -// deleted when absent from the desired set: load-balancer, EKS and RDS records -// in the base domain, but only for the classes this cycle enumerated authoritatively -// across all tenants. EC2 (`.compute.`) records are never pruned (a node sees -// only its own instances); structural (apex/NS/glue) records never match. +// deleted when absent from the desired set, for the classes this cycle +// enumerated authoritatively across all tenants. Structural (apex/NS/glue) +// records carry none of these prefixes, so they never match. func (r *Reconciler) prunable(scope PruneScope) func(zone, label string) bool { + private := privateZoneOrDefault(r.internalDomain) return func(zone, label string) bool { + // The private zone holds instance records and nothing else: every other + // producer writes the base domain. + if zone == private { + return scope.EC2 && strings.HasPrefix(label, ec2PrivateLabelPrefix) + } if zone != r.baseDomain { return false } + if scope.EC2 && strings.HasPrefix(label, ec2PublicLabelPrefix) { + return true + } if scope.ELB && strings.Contains(label, ".elb.") { return true } diff --git a/spinifex/handlers/dns/reconcile_ec2_prune_test.go b/spinifex/handlers/dns/reconcile_ec2_prune_test.go new file mode 100644 index 000000000..43f51193c --- /dev/null +++ b/spinifex/handlers/dns/reconcile_ec2_prune_test.go @@ -0,0 +1,151 @@ +//test:in-package — prunable, computeBatch, zoneRecord and the label prefixes are +//all unexported, and what these tests pin is which records a pass is willing to +//delete rather than anything the package exports. + +package dns + +import ( + "fmt" + "testing" + + nsconfig "github.com/mulgadc/northstar/pkg/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const testPrivate = "compute.internal" + +// prunableForZones mirrors prunableFor with a private zone configured, which is +// what makes instance records reachable at all. +func prunableForZones(scope PruneScope) func(zone, label string) bool { + r := &Reconciler{baseDomain: testBase, internalDomain: testPrivate} + return r.prunable(scope) +} + +// zoneTOML renders a minimal parseable zone carrying an apex NS plus the given +// A records, so a test can seed one into the fake S3 and read it back. +func zoneTOML(domain string, records map[string]string) string { + out := fmt.Sprintf("version = 1.0\n[domain]\ndomain = %q\nactive = true\nsoa = \"ns1.%s.\"\n"+ + "[defaults]\nttl = 300\ntype = 1\nclass = 1\n"+ + "[[records]]\ndomain = \"\"\ntype = 2\naddress = \"ns1.%s.\"\n", domain, domain, domain) + for label, addr := range records { + out += fmt.Sprintf("[[records]]\ndomain = %q\ntype = 1\naddress = %q\n", label, addr) + } + return out +} + +// An instance record is prunable in both of the zones it lands in, and only +// with the authority that says the instance view spanned every node. Without +// that authority pruning would delete the records of every instance running +// somewhere else, which is what the vmMgr-backed desired set could not avoid. +func TestPrunable_InstanceRecordsNeedClusterWideAuthority(t *testing.T) { + public := "ec2-3-3-3-3.ap-southeast-2.compute" + private := "ip-172-31-0-6.ap-southeast-2" + + withAuthority := prunableForZones(PruneScope{EC2: true}) + assert.True(t, withAuthority(testBase, public), "a public instance record in the base zone") + assert.True(t, withAuthority(testPrivate, private), "a private instance record in the private zone") + + without := prunableForZones(PruneScope{ELB: true, EKS: true, RDS: true}) + assert.False(t, without(testBase, public), + "without EC2 authority the instance view may be one node's, so nothing may be pruned") + assert.False(t, without(testPrivate, private), + "authority for the other classes does not extend to instances") +} + +// The private zone holds instance records and structural records, nothing else. +// Pruning it on "absent from the desired set" alone would take the NS set with +// it, which is a zone outage rather than a stale address. +func TestPrunable_PrivateZoneStructuralRecordsSurvive(t *testing.T) { + prune := prunableForZones(PruneScope{EC2: true, ELB: true, EKS: true, RDS: true}) + for _, label := range []string{"", "ns1", "soa"} { + assert.False(t, prune(testPrivate, label), + "structural record %q in the private zone must never be pruned", label) + } +} + +// An address that no running instance holds is stale in both zones at once: the +// record name is derived from the IP, so a reallocated address otherwise keeps +// resolving to whatever held it before. +func TestComputeConverge_PrunesAStaleInstanceRecordInBothZones(t *testing.T) { + desired := []Change{ + upsert("ec2-3-3-3-3.ap-southeast-2.compute."+testBase, "3.3.3.3"), + {Action: ActionUpsert, Zone: testPrivate, + Name: "ip-172-31-0-6.ap-southeast-2." + testPrivate, Type: "A", Value: "172.31.0.6"}, + } + // Stored labels are zone-relative and keep their trailing dot, which is what + // relativeLabel produces and therefore what a desired record is matched on. + existing := map[string][]zoneRecord{ + testBase: { + existingA("ec2-3-3-3-3.ap-southeast-2.compute.", "3.3.3.3"), + existingA("ec2-9-9-9-9.ap-southeast-2.compute.", "9.9.9.9"), + {label: "", rtype: nsconfig.TypeNS, value: "ns1." + testBase + "."}, + }, + testPrivate: { + existingA("ip-172-31-0-6.ap-southeast-2.", "172.31.0.6"), + existingA("ip-172-31-0-99.ap-southeast-2.", "172.31.0.99"), + {label: "", rtype: nsconfig.TypeNS, value: "ns1." + testPrivate + "."}, + }, + } + + batch, err := computeConverge(desired, existing, prunableForZones(PruneScope{EC2: true})) + require.NoError(t, err) + + deletes := deletesOf(batch) + names := make([]string, 0, len(deletes)) + for _, d := range deletes { + names = append(names, d.Name) + } + assert.ElementsMatch(t, []string{ + "ec2-9-9-9-9.ap-southeast-2.compute." + testBase, + "ip-172-31-0-99.ap-southeast-2." + testPrivate, + }, names, "both zones prune the address no running instance holds, and nothing else") +} + +// End to end through the real zone read: the private zone has to be one of the +// zones a pass reads, or its stale records are never even considered. Whether +// it is read when no pass could act on it is an S3 GET, not a behaviour, so it +// is not asserted here. +func TestComputeBatch_PrunesBothZonesUnderEC2Authority(t *testing.T) { + endpoint, objects := fakeS3(t, "northstar") + objects[testBase+".toml"] = zoneTOML(testBase, map[string]string{ + "ec2-3-3-3-3.ap-southeast-2.compute.": "3.3.3.3", + "ec2-9-9-9-9.ap-southeast-2.compute.": "9.9.9.9", + }) + objects[testPrivate+".toml"] = zoneTOML(testPrivate, map[string]string{ + "ip-172-31-0-6.ap-southeast-2.": "172.31.0.6", + "ip-172-31-0-99.ap-southeast-2.": "172.31.0.99", + }) + + // One instance is still running, so its two records must survive both passes. + live := []Change{ + upsert("ec2-3-3-3-3.ap-southeast-2.compute."+testBase, "3.3.3.3"), + {Action: ActionUpsert, Zone: testPrivate, + Name: "ip-172-31-0-6.ap-southeast-2." + testPrivate, Type: "A", Value: "172.31.0.6"}, + } + scope := PruneScope{} + r := &Reconciler{ + enabled: true, + baseDomain: testBase, + internalDomain: testPrivate, + s3cfg: &nsconfig.S3Config{ + Endpoint: endpoint, Bucket: "northstar", Region: "us-east-1", + AccessKey: "SYSTEM", SecretKey: "SYSTEMSECRET", + }, + desired: func() DesiredSet { return DesiredSet{Changes: live, Prunable: scope} }, + } + + batch, err := r.computeBatch() + require.NoError(t, err) + assert.Empty(t, deletesOf(batch), "no EC2 authority prunes nothing in either zone") + + scope = PruneScope{EC2: true} + batch, err = r.computeBatch() + require.NoError(t, err) + deletes := deletesOf(batch) + require.Len(t, deletes, 2, "with authority both zones are read and both stale records prune") + assert.ElementsMatch(t, []string{ + "ec2-9-9-9-9.ap-southeast-2.compute." + testBase, + "ip-172-31-0-99.ap-southeast-2." + testPrivate, + }, []string{deletes[0].Name, deletes[1].Name}) +} diff --git a/spinifex/handlers/dns/reconcile_test.go b/spinifex/handlers/dns/reconcile_test.go index e4e841a01..32b39fa05 100644 --- a/spinifex/handlers/dns/reconcile_test.go +++ b/spinifex/handlers/dns/reconcile_test.go @@ -46,7 +46,7 @@ func TestComputeConvergeUpsertsPassThroughAndPruneStale(t *testing.T) { existingA("app-old-xyz.ap-southeast-2.elb.", "9.9.9.9"), // A live EKS record still desired → kept (dedup by RRset, not re-deleted). existingA("prod.ap-southeast-2.eks.", "2.2.2.2"), - // An EC2 record absent from this node's desired view → must NOT be pruned. + // An EC2 record, with no EC2 authority this cycle → must NOT be pruned. existingA("ec2-4-4-4-4.ap-southeast-2.compute.", "4.4.4.4"), // Structural apex NS → never pruned. {label: "", rtype: nsconfig.TypeNS, value: "ns1.spx3.net."}, @@ -65,7 +65,7 @@ func TestComputeConvergeUpsertsPassThroughAndPruneStale(t *testing.T) { // EC2 and structural records survive. for _, d := range deletes { - assert.NotContains(t, d.Name, ".compute.", "EC2 records are never pruned") + assert.NotContains(t, d.Name, ".compute.", "EC2 pruning is suppressed when not enumerated") assert.NotEmpty(t, d.Name, "structural apex records are never pruned") } } @@ -126,7 +126,7 @@ func TestComputeConvergePrunesRDSOnlyWhenEnumerated(t *testing.T) { assert.Equal(t, "dropped-db.111122223333.ap-southeast-2.rds.spx3.net", deletes[0].Name) for _, d := range deletes { assert.NotContains(t, d.Name, ".elb.", "RDS authority does not grant ELB pruning") - assert.NotContains(t, d.Name, ".compute.", "EC2 records are never pruned") + assert.NotContains(t, d.Name, ".compute.", "EC2 pruning is suppressed when not enumerated") } } diff --git a/spinifex/handlers/ec2/instance/dns_hook_test.go b/spinifex/handlers/ec2/instance/dns_hook_test.go index 91769aca9..eb30b9b59 100644 --- a/spinifex/handlers/ec2/instance/dns_hook_test.go +++ b/spinifex/handlers/ec2/instance/dns_hook_test.go @@ -37,26 +37,6 @@ func TestPublishDNS(t *testing.T) { enabled.publishDNS("123456789012", handlers_dns.ActionUpsert, nil) } -func TestNeedsDNSWithdrawal(t *testing.T) { - tests := []struct { - name string - status vm.InstanceState - want bool - }{ - {name: "running", status: vm.StateRunning, want: false}, - {name: "stopping", status: vm.StateStopping, want: false}, - {name: "stopped", status: vm.StateStopped, want: false}, - {name: "shutting down", status: vm.StateShuttingDown, want: true}, - {name: "terminated", status: vm.StateTerminated, want: true}, - {name: "error", status: vm.StateError, want: true}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - require.Equal(t, tt.want, needsDNSWithdrawal(tt.status)) - }) - } -} - // The race the check exists for: terminate lands while the deferred launch is // still publishing the UPSERT, so Status has not caught up and only the // deletion mark says the record is going away. diff --git a/spinifex/handlers/ec2/instance/service_impl.go b/spinifex/handlers/ec2/instance/service_impl.go index 3b97c635c..ee8e86d91 100644 --- a/spinifex/handlers/ec2/instance/service_impl.go +++ b/spinifex/handlers/ec2/instance/service_impl.go @@ -1024,17 +1024,13 @@ func (s *InstanceServiceImpl) LaunchRunInstances(ctx context.Context, instances slog.InfoContext(ctx, "LaunchRunInstances: completed", "requested", len(instances), "launched", successCount) } -func needsDNSWithdrawal(status vm.InstanceState) bool { - return status != vm.StateRunning && status != vm.StateStopping && status != vm.StateStopped -} - // terminateRacedLaunch reports whether a terminate raced the deferred launch // UPSERT: it reads the terminate flag stamped under the lock (plus status) since // the async state transition may not have landed, which a status-only check misses. func (s *InstanceServiceImpl) terminateRacedLaunch(instance *vm.VM) bool { withdraw := false s.vmMgr.Inspect(instance, func(v *vm.VM) { - withdraw = v.DeletionTimestamp != nil || needsDNSWithdrawal(v.Status) + withdraw = v.DeletionTimestamp != nil || !handlers_dns.InstanceRetainsRecords(v.Status) }) return withdraw }