diff --git a/scripts/images/common/mulga-mgmt-net.sh b/scripts/images/common/mulga-mgmt-net.sh index 1193b7125..a08afba1b 100644 --- a/scripts/images/common/mulga-mgmt-net.sh +++ b/scripts/images/common/mulga-mgmt-net.sh @@ -8,11 +8,14 @@ # - NIC_DHCP=1: the primary data ENI. OVN serves DHCP; we lease it (retrying # one-shots on a budget until the cross-host datapath is up) so the Ec2 IMDS # datasource can reach 169.254.169.254, and pin a /32 to it so a link-local -# 169.254.0.0/16 route on another NIC cannot hijack IMDS. A DHCP NIC with -# NIC_DEFAULT=0 (an RDS DB VM's customer ENI) instead loses its leased -# default route and gets a source-based policy table for its replies. -# - NIC_CIDR: a static NIC (mgmt0 on br-mgmt, which has no DHCP). Applied -# address-only; NIC_DEFAULT is 0, so mgmt0 is never the default route. +# 169.254.0.0/16 route on another NIC cannot hijack IMDS. +# - NIC_CIDR (+ optional NIC_GW): a static NIC — mgmt0 on br-mgmt, and +# an RDS DB VM's customer ENI. The customer ENI is static rather than DHCP +# so it never depends on OVN's 3600s lease being renewed, which nothing +# does. NIC_DEFAULT is 0 for both, so neither is ever the default +# route; when NIC_GW is set, the NIC also gets the source-based +# return-path policy table a non-default DHCP NIC gets, so cross-subnet +# replies still egress correctly. # # The blob is shell KEY=value, matching daemon.buildNetcfgBlob and # build/microvm/init.sh; interfaces are matched by MAC. No-op when the blob is @@ -128,6 +131,7 @@ cidr_network() { install_return_policy() { iface="$1" tid=$(( 100 + $2 )) + gw="${3:-}" addr=$(ip -4 addr show dev "$iface" 2>/dev/null | awk '$1 == "inet" { print $2; exit }') if [ -z "$addr" ]; then @@ -136,9 +140,13 @@ install_return_policy() { fi ip4="${addr%%/*}" net=$(cidr_network "$addr") - # The lease's gateway is OVN's per-subnet router IP, which reaches every - # subnet of the VPC. Read it before the default route is dropped from main. - gw=$(ip -4 route show default dev "$iface" 2>/dev/null | awk '{print $3; exit}') + # OVN's per-subnet router IP reaches every subnet of the VPC. A static NIC + # has no lease to read it from, so the caller passes it explicitly + # (NIC_GW); a DHCP NIC reads it from its own default route instead, + # before that route is dropped below. + if [ -z "$gw" ]; then + gw=$(ip -4 route show default dev "$iface" 2>/dev/null | awk '{print $3; exit}') + fi # Both passes rewrite the table, not just the rule: a rule survives an # interface bounce but the routes go down with the device, and a rule whose @@ -174,6 +182,7 @@ for n in 0 1 2 3 4 5; do eval "dhcp=\${NIC${n}_DHCP:-}" eval "cidr=\${NIC${n}_CIDR:-}" + eval "gw=\${NIC${n}_GW:-}" eval "isdefault=\${NIC${n}_DEFAULT:-}" ip link set "$iface" up @@ -243,9 +252,17 @@ for n in 0 1 2 3 4 5; do # NIC_DEFAULT=0 means this NIC must never carry the default route: the # mgmt NIC reaches the gateway on-link and a default via it would blackhole - # egress and (with a link-local /16) hijack IMDS. Enforce it — a DHCP client - # racing this NIC may have added one before we set it static. + # egress and (with a link-local /16) hijack IMDS. Enforce it on every pass, + # not just setup: nothing else re-adds a route to a static NIC, but the + # policy table itself goes down with the device on a stop/start re-attach. if [ "$isdefault" != "1" ]; then + # A static NIC with a resolved gateway (an RDS DB VM's customer ENI) is + # cross-subnet reachable, so its replies need the same source-based + # return path a non-default DHCP NIC gets. mgmt0 has no NIC_GW and + # is skipped — it only ever reaches the gateway on-link. + if [ -n "$gw" ]; then + install_return_policy "$iface" "$n" "$gw" + fi drop_default_route "$iface" fi done diff --git a/spinifex/daemon/daemon_system_instance.go b/spinifex/daemon/daemon_system_instance.go index 0b86df369..d8a65b4dc 100644 --- a/spinifex/daemon/daemon_system_instance.go +++ b/spinifex/daemon/daemon_system_instance.go @@ -187,10 +187,12 @@ func (d *Daemon) LaunchSystemInstance(input *handlers_elbv2.SystemInstanceInput) d.recordENIInstanceOwner(extraAccount, extra.ENIID, accountID) } instance.ExtraENIs = append(instance.ExtraENIs, vm.ExtraENI{ - ENIID: extra.ENIID, - ENIMac: extra.ENIMac, - ENIIP: extra.ENIIP, - SubnetID: extra.SubnetID, + ENIID: extra.ENIID, + ENIMac: extra.ENIMac, + ENIIP: extra.ENIIP, + SubnetID: extra.SubnetID, + ENICIDRPrefix: extra.ENICIDRPrefix, + Gateway: extra.Gateway, }) } } else if input.SubnetID != "" && d.vpcService != nil { @@ -514,10 +516,12 @@ func (d *Daemon) launchAMISystemInstance(input *sysinstance.SystemInstanceInput) } } inst.ExtraENIs = append(inst.ExtraENIs, vm.ExtraENI{ - ENIID: extra.ENIID, - ENIMac: extra.ENIMac, - ENIIP: extra.ENIIP, - SubnetID: extra.SubnetID, + ENIID: extra.ENIID, + ENIMac: extra.ENIMac, + ENIIP: extra.ENIIP, + SubnetID: extra.SubnetID, + ENICIDRPrefix: extra.ENICIDRPrefix, + Gateway: extra.Gateway, }) } diff --git a/spinifex/daemon/vm_adapters.go b/spinifex/daemon/vm_adapters.go index ae0b0b6ee..71aea1ee5 100644 --- a/spinifex/daemon/vm_adapters.go +++ b/spinifex/daemon/vm_adapters.go @@ -14,6 +14,7 @@ import ( handlers_ec2_placementgroup "github.com/mulgadc/spinifex/spinifex/handlers/ec2/placementgroup" "github.com/mulgadc/spinifex/spinifex/network/external/dhcp" "github.com/mulgadc/spinifex/spinifex/network/topology" + "github.com/mulgadc/spinifex/spinifex/objectstore" "github.com/mulgadc/spinifex/spinifex/otelsetup" "github.com/mulgadc/spinifex/spinifex/tags" "github.com/mulgadc/spinifex/spinifex/types" @@ -707,7 +708,7 @@ func (a *instanceCleanerAdapter) DeleteVolumes(instance *vm.VM) error { "name", ebsRequest.Name, "id", instance.ID) continue } - if err := a.d.volumeService.DetachVolumeOnTerminate(context.Background(), ebsRequest.Name, instance.AccountID); err != nil && !awserrors.IsNotFound(err) { + if err := a.d.volumeService.DetachVolumeOnTerminate(context.Background(), ebsRequest.Name, instance.AccountID); err != nil && !awserrors.IsNotFound(err) && !objectstore.IsNoSuchKeyError(err) { slog.Error("Failed to detach volume on termination", "name", ebsRequest.Name, "id", instance.ID, "err", err) firstErr = cmp.Or(firstErr, err) @@ -730,7 +731,7 @@ func (a *instanceCleanerAdapter) DeleteVolumes(instance *vm.VM) error { // (above, via terminateCleanup) deliberately never clears a Boot // volume's attachment, so without this the root volume still hits // DeleteVolume's in-use guard here. - if err := a.d.volumeService.DeleteVolumeOnTerminate(context.Background(), ebsRequest.Name, instance.AccountID); err != nil && !awserrors.IsNotFound(err) { + if err := a.d.volumeService.DeleteVolumeOnTerminate(context.Background(), ebsRequest.Name, instance.AccountID); err != nil && !awserrors.IsNotFound(err) && !objectstore.IsNoSuchKeyError(err) { slog.Error("Failed to delete volume on termination", "name", ebsRequest.Name, "id", instance.ID, "err", err) firstErr = cmp.Or(firstErr, err) diff --git a/spinifex/handlers/ec2/volume/service_impl.go b/spinifex/handlers/ec2/volume/service_impl.go index 1e20ef401..e098a853e 100644 --- a/spinifex/handlers/ec2/volume/service_impl.go +++ b/spinifex/handlers/ec2/volume/service_impl.go @@ -1007,6 +1007,11 @@ func (s *VolumeServiceImpl) ModifyVolume(ctx context.Context, input *ec2.ModifyV // are returned, not swallowed. func (s *VolumeServiceImpl) DeleteVolumeOnTerminate(ctx context.Context, volumeID, accountID string) error { if err := s.UpdateVolumeState(volumeID, "available", "", ""); err != nil { + // No metadata doc means the volume is already gone, per GetVolume's own + // contract, so absence here is success, not a teardown failure. + if objectstore.IsNoSuchKeyError(err) { + return nil + } return fmt.Errorf("clear attachment before terminate delete: %w", err) } _, err := s.DeleteVolume(ctx, &ec2.DeleteVolumeInput{VolumeId: &volumeID}, accountID) @@ -1020,7 +1025,15 @@ func (s *VolumeServiceImpl) DeleteVolumeOnTerminate(ctx context.Context, volumeI // as DeleteVolumeOnTerminate: there is no live QEMU to hot-unplug on either // terminate path. func (s *VolumeServiceImpl) DetachVolumeOnTerminate(_ context.Context, volumeID, _ string) error { - return s.UpdateVolumeState(volumeID, "available", "", "") + if err := s.UpdateVolumeState(volumeID, "available", "", ""); err != nil { + // No metadata doc means the volume is already gone, per GetVolume's own + // contract, so absence here is success, not a teardown failure. + if objectstore.IsNoSuchKeyError(err) { + return nil + } + return err + } + return nil } // ForceDetachVolume clears a volume's attachment in the control plane without diff --git a/spinifex/handlers/ec2/volume/service_impl_test.go b/spinifex/handlers/ec2/volume/service_impl_test.go index 8c76129b0..e5f6debda 100644 --- a/spinifex/handlers/ec2/volume/service_impl_test.go +++ b/spinifex/handlers/ec2/volume/service_impl_test.go @@ -1773,6 +1773,34 @@ func TestDeleteVolumeOnTerminate_ClearsAttachmentThenDeletes(t *testing.T) { assert.Contains(t, err.Error(), awserrors.ErrorInvalidVolumeNotFound) } +// TestDeleteVolumeOnTerminate_AlreadyGoneMetadataIsSuccess verifies GC +// re-driving the volumes dependent on an instance whose volume metadata +// document is already deleted is treated as success, not a hard failure that +// pins the reaper backoff at its cap forever. +func TestDeleteVolumeOnTerminate_AlreadyGoneMetadataIsSuccess(t *testing.T) { + kv := setupTestVolumeKV(t) + store := objectstore.NewMemoryObjectStore() + svc := newTestVolumeServiceWithStore("ap-southeast-2a", store) + svc.snapshotKV = kv + + // No metadata doc is ever written for this volume ID. + err := svc.DeleteVolumeOnTerminate(context.Background(), "vol-already-gone", "") + require.NoError(t, err, "a volume with no metadata document is already gone, not a teardown failure") +} + +// TestDetachVolumeOnTerminate_AlreadyGoneMetadataIsSuccess mirrors +// TestDeleteVolumeOnTerminate_AlreadyGoneMetadataIsSuccess for the +// DeleteOnTermination=false path. +func TestDetachVolumeOnTerminate_AlreadyGoneMetadataIsSuccess(t *testing.T) { + kv := setupTestVolumeKV(t) + store := objectstore.NewMemoryObjectStore() + svc := newTestVolumeServiceWithStore("ap-southeast-2a", store) + svc.snapshotKV = kv + + err := svc.DetachVolumeOnTerminate(context.Background(), "vol-already-gone", "") + require.NoError(t, err, "a volume with no metadata document is already gone, not a teardown failure") +} + // TestDeleteVolumeOnTerminate_SurfacesDeleteFailure verifies a DeleteVolume // failure downstream of the attachment clear is returned, not swallowed — the // caller (deleteInstanceVolumes / instanceCleanerAdapter.DeleteVolumes) relies diff --git a/spinifex/handlers/ec2/vpc/eni.go b/spinifex/handlers/ec2/vpc/eni.go index be72caf12..a33ca7f99 100644 --- a/spinifex/handlers/ec2/vpc/eni.go +++ b/spinifex/handlers/ec2/vpc/eni.go @@ -968,18 +968,14 @@ type portEventPayload struct { SecurityGroupIds []string `json:"security_group_ids,omitempty"` } -// publishPortEvent publishes a port lifecycle event fire-and-forget. Used for -// vpc.delete-port — failure on delete is harmless (the port is going away -// anyway and the reconciler converges any leftover OVN state). +// publishPortEvent sends vpc.delete-port via request-reply so vpcd confirms +// the LSP is gone before a same-IP recreate can collide with a stale one. +// Non-fatal: the ENI KV row is already gone, so a failure is only logged. func (s *VPCServiceImpl) publishPortEvent(topic, eniId, subnetId, vpcId, privateIP, macAddr string, sgIds []string) { - utils.PublishEvent(s.natsConn, topic, portEventPayload{ - NetworkInterfaceId: eniId, - SubnetId: subnetId, - VpcId: vpcId, - PrivateIpAddress: privateIP, - MacAddress: macAddr, - SecurityGroupIds: sgIds, - }) + if err := s.requestPortEvent(topic, eniId, subnetId, vpcId, privateIP, macAddr, sgIds); err != nil { + slog.Warn("vpc: delete-port request failed, relying on reconciler orphan prune", + "topic", topic, "eniId", eniId, "err", err) + } } // requestPortEvent sends a port lifecycle event via request-reply so vpcd diff --git a/spinifex/handlers/ec2/vpc/eni_test.go b/spinifex/handlers/ec2/vpc/eni_test.go index 6e71e7b01..39452efa5 100644 --- a/spinifex/handlers/ec2/vpc/eni_test.go +++ b/spinifex/handlers/ec2/vpc/eni_test.go @@ -582,6 +582,31 @@ func TestDeleteNetworkInterface_PublishesEvent(t *testing.T) { } } +// TestDeleteNetworkInterface_VpcdDeletePortError_NonFatal proves a vpcd-side +// OVN LSP delete failure surfaces as a log, not an API error: the ENI KV row +// is already gone by the time this runs, so a failed teardown must still let +// the caller (instance terminate, appliance teardown) converge. +func TestDeleteNetworkInterface_VpcdDeletePortError_NonFatal(t *testing.T) { + svc, nc := setupTestVPCServiceWithNC(t) + vpcId := createTestVPC(t, svc, "10.0.0.0/16") + subnetId := createTestSubnet(t, svc, vpcId, "10.0.1.0/24") + eniId := createTestENI(t, svc, subnetId) + + testutil.OverrideVpcdStubResponse(nc, "vpc.delete-port", + []byte(`{"success":false,"error":"forced-delete-port-error"}`)) + + _, err := svc.DeleteNetworkInterface(context.Background(), &ec2.DeleteNetworkInterfaceInput{ + NetworkInterfaceId: aws.String(eniId), + }, testAccountID) + require.NoError(t, err, "a vpcd delete-port failure must not fail DeleteNetworkInterface") + + // The ENI record itself is gone regardless of the OVN-side failure. + _, err = svc.DescribeNetworkInterfaces(context.Background(), &ec2.DescribeNetworkInterfacesInput{ + NetworkInterfaceIds: []*string{aws.String(eniId)}, + }, testAccountID) + assert.Error(t, err) +} + func TestModifyNetworkInterfaceAttribute_SecurityGroups(t *testing.T) { svc := setupTestVPCService(t) vpcId := createTestVPC(t, svc, "10.0.0.0/16") diff --git a/spinifex/handlers/ochrevector/hostport.go b/spinifex/handlers/ochrevector/hostport.go index 4ebee7b2d..b1f67c0b2 100644 --- a/spinifex/handlers/ochrevector/hostport.go +++ b/spinifex/handlers/ochrevector/hostport.go @@ -39,6 +39,7 @@ type vpcProvisioner interface { CreateNetworkInterface(ctx context.Context, input *ec2.CreateNetworkInterfaceInput, accountID string) (*ec2.CreateNetworkInterfaceOutput, error) DescribeNetworkInterfaces(ctx context.Context, input *ec2.DescribeNetworkInterfacesInput, accountID string) (*ec2.DescribeNetworkInterfacesOutput, error) DescribeSubnets(ctx context.Context, input *ec2.DescribeSubnetsInput, accountID string) (*ec2.DescribeSubnetsOutput, error) + ModifyNetworkInterfaceAttribute(ctx context.Context, input *ec2.ModifyNetworkInterfaceAttributeInput, accountID string) (*ec2.ModifyNetworkInterfaceAttributeOutput, error) } // HostPortDeps bundles the collaborators the daemon's appliance-VPC host @@ -73,7 +74,7 @@ func daemonPortDescription(nodeID string) string { // handlers/bedrock's helper of the same shape: description-filtered // describe first, create only on a miss, so a restart reuses the same // address rather than leaking one per launch. -func ensureDaemonENI(ctx context.Context, vpcSvc vpcProvisioner, subnetID, nodeID string) (*daemonENI, error) { +func ensureDaemonENI(ctx context.Context, vpcSvc vpcProvisioner, subnetID, nodeID string, groupIDs []string) (*daemonENI, error) { desc := daemonPortDescription(nodeID) out, err := vpcSvc.DescribeNetworkInterfaces(ctx, &ec2.DescribeNetworkInterfacesInput{ Filters: []*ec2.Filter{ @@ -94,6 +95,12 @@ func ensureDaemonENI(ctx context.Context, vpcSvc vpcProvisioner, subnetID, nodeI // A record missing either field cannot back a host port, so fall // through and mint a usable one rather than fail the connect. if eni.id != "" && eni.ip != "" && eni.mac != "" { + // A reused ENI predates this node's SG membership, so re-assert + // it: without the appliance's groups the self-referencing SG + // drops the host port's traffic. + if err := ensureENIGroups(ctx, vpcSvc, ni, groupIDs); err != nil { + return nil, err + } return eni, nil } slog.WarnContext(ctx, "ochrevector: ignoring incomplete daemon ENI record", @@ -101,9 +108,14 @@ func ensureDaemonENI(ctx context.Context, vpcSvc vpcProvisioner, subnetID, nodeI } } + var groups []*string + if len(groupIDs) > 0 { + groups = aws.StringSlice(groupIDs) + } created, err := vpcSvc.CreateNetworkInterface(ctx, &ec2.CreateNetworkInterfaceInput{ SubnetId: aws.String(subnetID), Description: aws.String(desc), + Groups: groups, TagSpecifications: []*ec2.TagSpecification{{ ResourceType: aws.String("network-interface"), Tags: []*ec2.Tag{ @@ -130,6 +142,41 @@ func ensureDaemonENI(ctx context.Context, vpcSvc vpcProvisioner, subnetID, nodeI return eni, nil } +// ensureENIGroups makes the daemon ENI a member of the appliance's security +// groups, so the customer ENI's self-referencing SG authorizes the host port +// rather than dropping it. A no-op when the ENI already carries exactly those +// groups, or when the appliance exposed none. +func ensureENIGroups(ctx context.Context, vpcSvc vpcProvisioner, ni *ec2.NetworkInterface, groupIDs []string) error { + if len(groupIDs) == 0 || sameGroupSet(ni.Groups, groupIDs) { + return nil + } + _, err := vpcSvc.ModifyNetworkInterfaceAttribute(ctx, &ec2.ModifyNetworkInterfaceAttributeInput{ + NetworkInterfaceId: ni.NetworkInterfaceId, + Groups: aws.StringSlice(groupIDs), + }, utils.GlobalAccountID) + if err != nil { + return fmt.Errorf("ochrevector: set daemon ENI %s security groups: %w", aws.StringValue(ni.NetworkInterfaceId), err) + } + return nil +} + +// sameGroupSet reports whether current holds exactly the desired group IDs. +func sameGroupSet(current []*ec2.GroupIdentifier, desired []string) bool { + if len(current) != len(desired) { + return false + } + have := make(map[string]struct{}, len(current)) + for _, g := range current { + have[aws.StringValue(g.GroupId)] = struct{}{} + } + for _, id := range desired { + if _, ok := have[id]; !ok { + return false + } + } + return true +} + // applianceInstanceTagKey mirrors the unexported rdsInstanceTagKey in // handlers/rds/launch.go, which stamps it on the appliance's customer ENI at // launch. Keep this literal in sync with that const; the e2e harness @@ -145,9 +192,9 @@ const endpointENIDescriptionPrefix = "RDS endpoint ENI for " // handlers/rds stamps on it at launch, returning the real private IP to dial // and the subnet it lives in -- never the vanity endpoint hostname, which is // unresolvable and unroutable from the daemon host. -func resolveApplianceTarget(ctx context.Context, deps HostPortDeps, identifier string) (dialIP, subnetID, subnetCIDR string, err error) { +func resolveApplianceTarget(ctx context.Context, deps HostPortDeps, identifier string) (dialIP, subnetID, subnetCIDR string, groupIDs []string, err error) { if deps.VPC == nil { - return "", "", "", errors.New("ochrevector: no VPC provider configured; cannot resolve the appliance's ENI") + return "", "", "", nil, errors.New("ochrevector: no VPC provider configured; cannot resolve the appliance's ENI") } out, err := deps.VPC.DescribeNetworkInterfaces(ctx, &ec2.DescribeNetworkInterfacesInput{ @@ -156,7 +203,7 @@ func resolveApplianceTarget(ctx context.Context, deps HostPortDeps, identifier s }, }, utils.GlobalAccountID) if err != nil { - return "", "", "", fmt.Errorf("ochrevector: describe appliance ENI for %s: %w", identifier, err) + return "", "", "", nil, fmt.Errorf("ochrevector: describe appliance ENI for %s: %w", identifier, err) } // Both the endpoint ENI and the management NIC carry the rds-db-instance // tag, but only the endpoint ENI (in the customer VPC) is reachable from the @@ -177,22 +224,29 @@ func resolveApplianceTarget(ctx context.Context, deps HostPortDeps, identifier s } } if ni == nil { - return "", "", "", fmt.Errorf("ochrevector: no customer ENI found for appliance %s", identifier) + return "", "", "", nil, fmt.Errorf("ochrevector: no customer ENI found for appliance %s", identifier) } dialIP = aws.StringValue(ni.PrivateIpAddress) subnetID = aws.StringValue(ni.SubnetId) + // The daemon's host-port ENI must join these groups, or the customer ENI's + // self-referencing SG drops its traffic to the appliance. + for _, g := range ni.Groups { + if id := aws.StringValue(g.GroupId); id != "" { + groupIDs = append(groupIDs, id) + } + } subOut, err := deps.VPC.DescribeSubnets(ctx, &ec2.DescribeSubnetsInput{ Filters: []*ec2.Filter{{Name: aws.String("subnet-id"), Values: aws.StringSlice([]string{subnetID})}}, }, utils.GlobalAccountID) if err != nil { - return "", "", "", fmt.Errorf("ochrevector: describe subnet %s: %w", subnetID, err) + return "", "", "", nil, fmt.Errorf("ochrevector: describe subnet %s: %w", subnetID, err) } if subOut == nil || len(subOut.Subnets) == 0 || aws.StringValue(subOut.Subnets[0].CidrBlock) == "" { - return "", "", "", fmt.Errorf("ochrevector: subnet %s has no cidr block", subnetID) + return "", "", "", nil, fmt.Errorf("ochrevector: subnet %s has no cidr block", subnetID) } subnetCIDR = aws.StringValue(subOut.Subnets[0].CidrBlock) - return dialIP, subnetID, subnetCIDR, nil + return dialIP, subnetID, subnetCIDR, groupIDs, nil } // ensureApplianceHostPort gives this daemon a routed presence in the subnet @@ -213,7 +267,7 @@ func ensureApplianceHostPort(ctx context.Context, deps HostPortDeps, identifier return "", "", errors.New("ochrevector: no VPC provider configured; cannot mint the daemon's appliance-VPC port") } - dialIP, subnetID, subnetCIDR, err := resolveApplianceTarget(ctx, deps, identifier) + dialIP, subnetID, subnetCIDR, groupIDs, err := resolveApplianceTarget(ctx, deps, identifier) if err != nil { return "", "", err } @@ -222,7 +276,7 @@ func ensureApplianceHostPort(ctx context.Context, deps HostPortDeps, identifier return "", "", fmt.Errorf("ochrevector: parse appliance subnet CIDR %q: %w", subnetCIDR, err) } - eni, err := ensureDaemonENI(ctx, deps.VPC, subnetID, deps.NodeID) + eni, err := ensureDaemonENI(ctx, deps.VPC, subnetID, deps.NodeID, groupIDs) if err != nil { return "", "", err } diff --git a/spinifex/handlers/ochrevector/hostport_test.go b/spinifex/handlers/ochrevector/hostport_test.go index 1db498ae1..2610bb7ff 100644 --- a/spinifex/handlers/ochrevector/hostport_test.go +++ b/spinifex/handlers/ochrevector/hostport_test.go @@ -24,6 +24,7 @@ const ( testApplianceSubnet = "subnet-appliance-0001" testApplianceEndpoint = "10.244.1.9" testApplianceENIID = "eni-appliance-0001" + testApplianceSG = "sg-appliance-0001" ) // fakeVPC is an in-memory vpcProvisioner: every CreateNetworkInterface call @@ -32,11 +33,12 @@ const ( // lookup and the description-filtered daemon-ENI describe-or-create. // subnets backs DescribeSubnets. type fakeVPC struct { - mu sync.Mutex - nextID int - created []string - records map[string]*ec2.NetworkInterface - subnets []*ec2.Subnet + mu sync.Mutex + nextID int + created []string + sgModifies []string + records map[string]*ec2.NetworkInterface + subnets []*ec2.Subnet } var _ vpcProvisioner = (*fakeVPC)(nil) @@ -54,6 +56,7 @@ func (f *fakeVPC) CreateNetworkInterface(_ context.Context, in *ec2.CreateNetwor MacAddress: aws.String("02:00:00:00:00:01"), SubnetId: in.SubnetId, Description: in.Description, + Groups: groupIdentifiers(in.Groups), } if f.records == nil { f.records = map[string]*ec2.NetworkInterface{} @@ -62,6 +65,32 @@ func (f *fakeVPC) CreateNetworkInterface(_ context.Context, in *ec2.CreateNetwor return &ec2.CreateNetworkInterfaceOutput{NetworkInterface: ni}, nil } +// ModifyNetworkInterfaceAttribute records the call and re-associates the +// stored ENI's security groups, mirroring the in-place SG replace the real +// service does. +func (f *fakeVPC) ModifyNetworkInterfaceAttribute(_ context.Context, in *ec2.ModifyNetworkInterfaceAttributeInput, _ string) (*ec2.ModifyNetworkInterfaceAttributeOutput, error) { + f.mu.Lock() + defer f.mu.Unlock() + id := aws.StringValue(in.NetworkInterfaceId) + f.sgModifies = append(f.sgModifies, id) + if ni, ok := f.records[id]; ok && in.Groups != nil { + ni.Groups = groupIdentifiers(in.Groups) + } + return &ec2.ModifyNetworkInterfaceAttributeOutput{}, nil +} + +// groupIdentifiers maps SG IDs to the describe-shaped GroupIdentifier slice. +func groupIdentifiers(ids []*string) []*ec2.GroupIdentifier { + if len(ids) == 0 { + return nil + } + out := make([]*ec2.GroupIdentifier, 0, len(ids)) + for _, id := range ids { + out = append(out, &ec2.GroupIdentifier{GroupId: id}) + } + return out +} + // putRecord seeds an ENI the fake did not mint, so a test can present the // lookup with a record it would not otherwise produce. func (f *fakeVPC) putRecord(eniID, subnetID, description, ip, mac string) { @@ -92,6 +121,7 @@ func (f *fakeVPC) putApplianceENI(eniID, identifier, subnetID, ip string) { NetworkInterfaceId: aws.String(eniID), SubnetId: aws.String(subnetID), PrivateIpAddress: aws.String(ip), + Groups: []*ec2.GroupIdentifier{{GroupId: aws.String(testApplianceSG)}}, TagSet: []*ec2.Tag{ {Key: aws.String(applianceInstanceTagKey), Value: aws.String(identifier)}, }, @@ -112,6 +142,7 @@ func (f *fakeVPC) putTaggedENIWithDescription(eniID, identifier, subnetID, ip, d SubnetId: aws.String(subnetID), PrivateIpAddress: aws.String(ip), Description: aws.String(description), + Groups: []*ec2.GroupIdentifier{{GroupId: aws.String(testApplianceSG)}}, TagSet: []*ec2.Tag{ {Key: aws.String(applianceInstanceTagKey), Value: aws.String(identifier)}, }, @@ -291,6 +322,49 @@ func TestEnsureApplianceHostPort_CreatesENIAndHostPort(t *testing.T) { assert.Equal(t, "02:00:00:00:00:01", calls[0].mac) } +// The daemon's host-port ENI must join the appliance's security group, or the +// customer ENI's self-referencing SG silently drops its traffic to pg. +func TestEnsureApplianceHostPort_DaemonENIJoinsApplianceSG(t *testing.T) { + h := newHostPortHarness() + h.putSubnet(testApplianceSubnet, "10.244.1.0/24") + h.putApplianceENI(testApplianceIdentifier, testApplianceSubnet, testApplianceEndpoint) + + _, eniID, err := ensureApplianceHostPort(t.Context(), h.deps(), testApplianceIdentifier) + require.NoError(t, err) + + h.vpc.mu.Lock() + defer h.vpc.mu.Unlock() + require.Contains(t, h.vpc.records, eniID) + got := []string{} + for _, g := range h.vpc.records[eniID].Groups { + got = append(got, aws.StringValue(g.GroupId)) + } + assert.Equal(t, []string{testApplianceSG}, got, "the created daemon ENI must carry the appliance SG") +} + +// A daemon ENI that predates SG inheritance (reused by description, no groups) +// must be re-associated in place rather than left unauthorized. +func TestEnsureApplianceHostPort_ReusedENIGetsApplianceSG(t *testing.T) { + h := newHostPortHarness() + h.putSubnet(testApplianceSubnet, "10.244.1.0/24") + h.putApplianceENI(testApplianceIdentifier, testApplianceSubnet, testApplianceEndpoint) + h.vpc.putRecord("eni-legacy-daemon", testApplianceSubnet, daemonPortDescription(testNodeID), "10.244.1.50", "02:00:00:00:00:09") + + _, eniID, err := ensureApplianceHostPort(t.Context(), h.deps(), testApplianceIdentifier) + require.NoError(t, err) + assert.Equal(t, "eni-legacy-daemon", eniID, "the existing daemon ENI should be reused") + assert.Empty(t, h.vpc.created, "reuse must not mint a new ENI") + + h.vpc.mu.Lock() + defer h.vpc.mu.Unlock() + assert.Contains(t, h.vpc.sgModifies, "eni-legacy-daemon", "the reused ENI must be re-associated to the appliance SG") + got := []string{} + for _, g := range h.vpc.records["eni-legacy-daemon"].Groups { + got = append(got, aws.StringValue(g.GroupId)) + } + assert.Equal(t, []string{testApplianceSG}, got) +} + // The port must carry the ENI address at the SUBNET's prefix length: a /32 // addresses the port and still leaves the appliance unreachable, which is // the entire reason the port exists. @@ -417,11 +491,12 @@ func TestResolveApplianceTarget_FindsTheTaggedApplianceENI(t *testing.T) { h.putSubnet(testApplianceSubnet, "10.244.1.0/24") h.putApplianceENI(testApplianceIdentifier, testApplianceSubnet, testApplianceEndpoint) - dialIP, subnetID, cidr, err := resolveApplianceTarget(t.Context(), h.deps(), testApplianceIdentifier) + dialIP, subnetID, cidr, groupIDs, err := resolveApplianceTarget(t.Context(), h.deps(), testApplianceIdentifier) require.NoError(t, err) assert.Equal(t, testApplianceEndpoint, dialIP) assert.Equal(t, testApplianceSubnet, subnetID) assert.Equal(t, "10.244.1.0/24", cidr) + assert.Equal(t, []string{testApplianceSG}, groupIDs, "the appliance SG must be surfaced so the daemon ENI can join it") } // The endpoint ENI and the management NIC share the rds-db-instance tag, but @@ -441,7 +516,7 @@ func TestResolveApplianceTarget_PrefersEndpointENIOverManagementNIC(t *testing.T h.vpc.putTaggedENIWithDescription("eni-endpoint", testApplianceIdentifier, testApplianceSubnet, testApplianceEndpoint, endpointENIDescriptionPrefix+testApplianceIdentifier) - dialIP, subnetID, cidr, err := resolveApplianceTarget(t.Context(), h.deps(), testApplianceIdentifier) + dialIP, subnetID, cidr, _, err := resolveApplianceTarget(t.Context(), h.deps(), testApplianceIdentifier) require.NoError(t, err) require.Equal(t, testApplianceEndpoint, dialIP, "must dial the endpoint ENI, never the management NIC") require.Equal(t, testApplianceSubnet, subnetID) @@ -455,7 +530,7 @@ func TestResolveApplianceTarget_IgnoresOtherIdentifiersTags(t *testing.T) { h.putSubnet(testApplianceSubnet, "10.244.1.0/24") h.putApplianceENI("some-other-db", testApplianceSubnet, testApplianceEndpoint) - _, _, _, err := resolveApplianceTarget(t.Context(), h.deps(), testApplianceIdentifier) + _, _, _, _, err := resolveApplianceTarget(t.Context(), h.deps(), testApplianceIdentifier) require.Error(t, err) assert.Contains(t, err.Error(), "no customer ENI found") } @@ -464,7 +539,7 @@ func TestResolveApplianceTarget_ErrorsWhenSubnetHasNoCIDR(t *testing.T) { h := newHostPortHarness() h.putApplianceENI(testApplianceIdentifier, testApplianceSubnet, testApplianceEndpoint) - _, _, _, err := resolveApplianceTarget(t.Context(), h.deps(), testApplianceIdentifier) + _, _, _, _, err := resolveApplianceTarget(t.Context(), h.deps(), testApplianceIdentifier) require.Error(t, err) } @@ -474,7 +549,7 @@ func TestResolveApplianceTarget_PropagatesDescribeFailure(t *testing.T) { deps := h.deps() deps.VPC = failing - _, _, _, err := resolveApplianceTarget(t.Context(), deps, testApplianceIdentifier) + _, _, _, _, err := resolveApplianceTarget(t.Context(), deps, testApplianceIdentifier) require.Error(t, err) assert.Contains(t, err.Error(), "nats timeout") } @@ -483,7 +558,7 @@ func TestResolveApplianceTarget_RequiresVPC(t *testing.T) { h := newHostPortHarness() noVPC := h.deps() noVPC.VPC = nil - _, _, _, err := resolveApplianceTarget(t.Context(), noVPC, testApplianceIdentifier) + _, _, _, _, err := resolveApplianceTarget(t.Context(), noVPC, testApplianceIdentifier) require.Error(t, err) assert.Contains(t, err.Error(), "VPC provider") } diff --git a/spinifex/handlers/rds/launch.go b/spinifex/handlers/rds/launch.go index 797ee373e..6daaf4f20 100644 --- a/spinifex/handlers/rds/launch.go +++ b/spinifex/handlers/rds/launch.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "log/slog" + "net" "slices" "time" @@ -84,6 +85,10 @@ type launchVPCProvisioner interface { // start, so a deployment whose system VPC was rebuilt still gets one. CreateSecurityGroup(ctx context.Context, input *ec2.CreateSecurityGroupInput, accountID string) (*ec2.CreateSecurityGroupOutput, error) DescribeSecurityGroups(ctx context.Context, input *ec2.DescribeSecurityGroupsInput, accountID string) (*ec2.DescribeSecurityGroupsOutput, error) + // Read to configure the customer ENI statically: it must not depend on the + // OVN DHCP lease being renewed, so its prefix and gateway are resolved once + // at launch time instead. + DescribeSubnets(ctx context.Context, input *ec2.DescribeSubnetsInput, accountID string) (*ec2.DescribeSubnetsOutput, error) } // System-managed VMs get a mgmt-bridge NIC alongside their VPC NICs, which is @@ -239,6 +244,15 @@ func LaunchDBInstanceVM(ctx context.Context, deps LaunchDeps, in LaunchInput) (o }) } + // The customer ENI is configured statically, not via DHCP: OVN's 3600s + // lease is never renewed by the guest's one-shot udhcpc, so a DHCP customer + // ENI goes address-less an hour after boot. Resolve its prefix and gateway + // once here instead of depending on the lease surviving the VM's lifetime. + customerPrefix, customerGateway, err := customerENISubnetGateway(ctx, deps.VPC, in.AccountID, in.SubnetID) + if err != nil { + return nil, fmt.Errorf("rds: resolve customer subnet network for %s: %w", in.DBInstanceIdentifier, err) + } + sysOut, err := deps.Instance.LaunchSystemInstance(&sysinstance.SystemInstanceInput{ BootMode: sysinstance.BootAMI, ManagedBy: tags.ManagedByRDS, @@ -252,11 +266,13 @@ func LaunchDBInstanceVM(ctx context.Context, deps LaunchDeps, in LaunchInput) (o ENIMac: systemENI.mac, ENIIP: systemENI.ip, ExtraENIs: []sysinstance.ExtraENIInput{{ - ENIID: customerENI.id, - ENIMac: customerENI.mac, - ENIIP: customerENI.ip, - SubnetID: in.SubnetID, - AccountID: in.AccountID, + ENIID: customerENI.id, + ENIMac: customerENI.mac, + ENIIP: customerENI.ip, + SubnetID: in.SubnetID, + AccountID: in.AccountID, + ENICIDRPrefix: customerPrefix, + Gateway: customerGateway, // The endpoint is the ENI's address, so it has to survive the // terminate half of a replace the way the datadir volume does. // A DeleteDBInstance deletes it explicitly once the VM is gone. @@ -445,6 +461,42 @@ func resolveCustomerENI(ctx context.Context, vpcSvc launchVPCProvisioner, in Lau return nil, fmt.Errorf("rds: endpoint ENI %s no longer exists", in.ExistingCustomerENI) } +// customerENISubnetGateway resolves the customer subnet's prefix length and +// gateway IP (network address +1), matching OVN's per-subnet router +// placement (see network/topology.SubnetGatewayCIDR). Both are needed to +// configure the customer ENI statically instead of via DHCP. +func customerENISubnetGateway(ctx context.Context, vpcSvc launchVPCProvisioner, accountID, subnetID string) (prefixBits int, gateway string, err error) { + out, err := vpcSvc.DescribeSubnets(ctx, &ec2.DescribeSubnetsInput{ + SubnetIds: aws.StringSlice([]string{subnetID}), + }, accountID) + if err != nil { + return 0, "", fmt.Errorf("rds: describe subnet %s: %w", subnetID, err) + } + if out == nil { + return 0, "", fmt.Errorf("rds: subnet %s not found", subnetID) + } + for _, subnet := range out.Subnets { + if aws.StringValue(subnet.SubnetId) != subnetID { + continue + } + _, ipNet, perr := net.ParseCIDR(aws.StringValue(subnet.CidrBlock)) + if perr != nil { + return 0, "", fmt.Errorf("rds: subnet %s has an unparseable CIDR %q: %w", + subnetID, aws.StringValue(subnet.CidrBlock), perr) + } + ones, _ := ipNet.Mask.Size() + gw := ipNet.IP.To4() + if gw == nil { + return 0, "", fmt.Errorf("rds: subnet %s CIDR %q is not IPv4", subnetID, ipNet.String()) + } + gwCopy := make(net.IP, len(gw)) + copy(gwCopy, gw) + gwCopy[3]++ + return ones, gwCopy.String(), nil + } + return 0, "", fmt.Errorf("rds: subnet %s not found", subnetID) +} + // Detach comes first because a terminated VM can leave the attachment record // behind, which a plain delete rejects as InUse. func deleteLaunchENI(ctx context.Context, vpcSvc launchVPCProvisioner, accountID, eniID string) { diff --git a/spinifex/handlers/rds/launch_test.go b/spinifex/handlers/rds/launch_test.go index 107a3db06..f86bd96a8 100644 --- a/spinifex/handlers/rds/launch_test.go +++ b/spinifex/handlers/rds/launch_test.go @@ -26,7 +26,11 @@ import ( const ( testCustomerAccount = "123456789012" testDBSubnet = "subnet-customer-db" - testEngineAMI = "ami-rds-postgres-18" + // The customer subnet's CIDR, resolved so the launcher can configure the + // customer ENI statically. Gives a gateway of 10.0.0.1, matching the fake + // customer ENI IPs CreateNetworkInterface hands out. + testDBSubnetCIDR = "10.0.0.0/24" + testEngineAMI = "ami-rds-postgres-18" // The persisted endpoint a replace re-attaches: its address is the DNS // target and its MAC is what the replacement VM's NIC has to come up with. @@ -266,6 +270,16 @@ func (f *fakeENIs) ModifyNetworkInterfaceAttribute(_ context.Context, in *ec2.Mo return &ec2.ModifyNetworkInterfaceAttributeOutput{}, nil } +// DescribeSubnets answers the customer subnet's CIDR, which the launcher +// resolves into the prefix and gateway it configures the customer ENI with. +func (f *fakeENIs) DescribeSubnets(_ context.Context, in *ec2.DescribeSubnetsInput, _ string) (*ec2.DescribeSubnetsOutput, error) { + var subnets []*ec2.Subnet + for _, id := range aws.StringValueSlice(in.SubnetIds) { + subnets = append(subnets, &ec2.Subnet{SubnetId: aws.String(id), CidrBlock: aws.String(testDBSubnetCIDR)}) + } + return &ec2.DescribeSubnetsOutput{Subnets: subnets}, nil +} + func (f *fakeENIs) DescribeSecurityGroups(_ context.Context, in *ec2.DescribeSecurityGroupsInput, _ string) (*ec2.DescribeSecurityGroupsOutput, error) { f.sgDescribed = append(f.sgDescribed, in) if f.sgExistingGroups != nil { @@ -601,6 +615,10 @@ func TestLaunchDBInstanceVMWiresBothNICs(t *testing.T) { // The endpoint address lives on this NIC, so it has to survive the // terminate half of a replace the way the datadir volume does. DeleteOnTermination: aws.Bool(false), + // Resolved from the customer subnet's CIDR so the guest configures this + // NIC statically instead of depending on the OVN DHCP lease. + ENICIDRPrefix: 24, + Gateway: "10.0.0.1", }, in.ExtraENIs[0], "the extra NIC must carry the customer account, or the daemon updates the wrong ENI record") assert.Equal(t, "#cloud-config\n", in.UserData) assert.Equal(t, "arn:aws:iam::000000000000:instance-profile/rdsInstanceRole", in.IamInstanceProfileArn) diff --git a/spinifex/handlers/sysinstance/launcher.go b/spinifex/handlers/sysinstance/launcher.go index f3cfaa63e..aef988c76 100644 --- a/spinifex/handlers/sysinstance/launcher.go +++ b/spinifex/handlers/sysinstance/launcher.go @@ -105,6 +105,12 @@ type ExtraENIInput struct { // ENI across a class-change replace). Nil keeps the attach-time default, // which deletes the ENI with the VM. DeleteOnTermination *bool `json:"delete_on_termination,omitempty"` + + // ENICIDRPrefix and Gateway configure this ENI statically instead of via + // DHCP (an RDS DB VM's customer ENI, so it never depends on the OVN lease + // being renewed). Zero/empty keeps the DHCP path. + ENICIDRPrefix int `json:"eni_cidr_prefix,omitempty"` + Gateway string `json:"gateway,omitempty"` } // NICConfig describes a single network interface for a BootDirect microVM. diff --git a/spinifex/network/topology/manager_live.go b/spinifex/network/topology/manager_live.go index 792cba677..4e92fbd0d 100644 --- a/spinifex/network/topology/manager_live.go +++ b/spinifex/network/topology/manager_live.go @@ -347,6 +347,9 @@ func (m *liveManager) EnsurePort(ctx context.Context, spec PortSpec) error { } // DeletePort clears the ENI's port-group memberships and removes the LSP. +// Idempotent: an already-absent LSP (a retry, or a race with the reconciler's +// orphan prune) is a no-op success, not an error — mirrors ForceDeleteInstanceENI +// tolerating an already-gone ENI. func (m *liveManager) DeletePort(ctx context.Context, spec PortSpec) error { if m.ovn == nil { return fmt.Errorf("OVN client not connected") @@ -354,17 +357,25 @@ func (m *liveManager) DeletePort(ctx context.Context, spec PortSpec) error { portName := Port(spec.PortID) switchName := SubnetSwitch(spec.SubnetID) - if _, err := m.reconcilePortSGs(ctx, portName, nil); err != nil { - return fmt.Errorf("clear port group memberships for %q: %w", portName, err) + // Mirror EnsurePort's existence check: a Get that returns no error means + // the port is present, so delete it; any error means it is already gone + // (a retry, or a race with the reconciler's orphan prune) and we no-op. + if _, err := m.ovn.GetLogicalSwitchPort(ctx, portName); err == nil { + if _, err := m.reconcilePortSGs(ctx, portName, nil); err != nil { + return fmt.Errorf("clear port group memberships for %q: %w", portName, err) + } + if err := m.ovn.DeleteLogicalSwitchPort(ctx, switchName, portName); err != nil { + return fmt.Errorf("delete logical switch port %q on %q: %w", portName, switchName, err) + } + slog.Info("topology: DeletePort removed LSP", + "port", portName, + "switch", switchName, + "eni_id", spec.PortID, + ) + return nil } - if err := m.ovn.DeleteLogicalSwitchPort(ctx, switchName, portName); err != nil { - return fmt.Errorf("delete logical switch port %q on %q: %w", portName, switchName, err) - } - slog.Info("topology: DeletePort removed LSP", - "port", portName, - "switch", switchName, - "eni_id", spec.PortID, - ) + + slog.Info("topology: DeletePort found no LSP, already gone", "port", portName, "eni_id", spec.PortID) return nil } diff --git a/spinifex/network/topology/manager_live_test.go b/spinifex/network/topology/manager_live_test.go index 21658019e..10e0f4210 100644 --- a/spinifex/network/topology/manager_live_test.go +++ b/spinifex/network/topology/manager_live_test.go @@ -137,6 +137,96 @@ func TestLiveManager_DeletePort(t *testing.T) { } } +// TestLiveManager_DeletePort_AlreadyAbsentIsNoop proves DeletePort is +// idempotent: a retry (or a race with the reconciler's orphan prune) against +// an already-gone LSP must not error, matching how the codebase treats +// already-gone resources elsewhere (e.g. ForceDeleteInstanceENI). +func TestLiveManager_DeletePort_AlreadyAbsentIsNoop(t *testing.T) { + mgr, _ := newLiveManagerForTest(t) + ctx := context.Background() + vpc := VPCSpec{VPCID: "vpc-dp2", CIDR: netip.MustParsePrefix("10.5.0.0/16")} + sub := SubnetSpec{SubnetID: "subnet-DP2", VPCID: vpc.VPCID, CIDR: netip.MustParsePrefix("10.5.1.0/24")} + _ = mgr.EnsureVPC(ctx, vpc) + _ = mgr.EnsureSubnet(ctx, sub) + port := PortSpec{PortID: "eni-never-created", SubnetID: sub.SubnetID, VPCID: vpc.VPCID} + + if err := mgr.DeletePort(ctx, port); err != nil { + t.Fatalf("DeletePort on an absent LSP must be a no-op, got: %v", err) + } + + // A second delete of a port that WAS created, once it's gone, is also a no-op. + mac, _ := net.ParseMAC("02:00:00:00:00:03") + port2 := PortSpec{ + PortID: "eni-DP2", SubnetID: sub.SubnetID, VPCID: vpc.VPCID, + PrivateIP: netip.MustParseAddr("10.5.1.5"), MAC: mac, + } + if err := mgr.EnsurePort(ctx, port2); err != nil { + t.Fatalf("EnsurePort: %v", err) + } + if err := mgr.DeletePort(ctx, port2); err != nil { + t.Fatalf("first DeletePort: %v", err) + } + if err := mgr.DeletePort(ctx, port2); err != nil { + t.Fatalf("repeat DeletePort on an already-deleted LSP must be a no-op, got: %v", err) + } +} + +// TestLiveManager_DeletePort_ThenRecreateSameIP_NoDuplicateLSP covers the +// teardown+recreate-on-the-same-IP scenario: after DeletePort removes the +// old ENI's LSP, a new ENI can EnsurePort the same private IP and OVN NB +// ends up with exactly one LSP carrying that address, not two. +func TestLiveManager_DeletePort_ThenRecreateSameIP_NoDuplicateLSP(t *testing.T) { + mgr, mockClient := newLiveManagerForTest(t) + ctx := context.Background() + vpc := VPCSpec{VPCID: "vpc-reuse", CIDR: netip.MustParsePrefix("172.31.0.0/16")} + sub := SubnetSpec{SubnetID: "subnet-reuse", VPCID: vpc.VPCID, CIDR: netip.MustParsePrefix("172.31.0.0/24")} + _ = mgr.EnsureVPC(ctx, vpc) + _ = mgr.EnsureSubnet(ctx, sub) + + oldMAC, _ := net.ParseMAC("02:00:00:00:01:01") + oldPort := PortSpec{ + PortID: "eni-old", SubnetID: sub.SubnetID, VPCID: vpc.VPCID, + PrivateIP: netip.MustParseAddr("172.31.0.4"), MAC: oldMAC, + } + if err := mgr.EnsurePort(ctx, oldPort); err != nil { + t.Fatalf("EnsurePort(old): %v", err) + } + if err := mgr.DeletePort(ctx, oldPort); err != nil { + t.Fatalf("DeletePort(old): %v", err) + } + + newMAC, _ := net.ParseMAC("02:00:00:00:01:02") + newPort := PortSpec{ + PortID: "eni-new", SubnetID: sub.SubnetID, VPCID: vpc.VPCID, + PrivateIP: netip.MustParseAddr("172.31.0.4"), MAC: newMAC, + } + if err := mgr.EnsurePort(ctx, newPort); err != nil { + t.Fatalf("EnsurePort(new): %v", err) + } + + lsps, err := mockClient.ListLogicalSwitchPorts(ctx) + if err != nil { + t.Fatalf("ListLogicalSwitchPorts: %v", err) + } + matching := 0 + for _, lsp := range lsps { + for _, addr := range lsp.Addresses { + if addr == newMAC.String()+" 172.31.0.4" || addr == oldMAC.String()+" 172.31.0.4" { + matching++ + } + } + } + if matching != 1 { + t.Fatalf("expected exactly one LSP carrying 172.31.0.4, found %d", matching) + } + if _, err := mockClient.GetLogicalSwitchPort(ctx, Port(oldPort.PortID)); err == nil { + t.Fatal("old ENI's LSP must be gone after DeletePort") + } + if _, err := mockClient.GetLogicalSwitchPort(ctx, Port(newPort.PortID)); err != nil { + t.Fatalf("new ENI's LSP must exist: %v", err) + } +} + func TestLiveManager_DeleteSubnetAndVPC(t *testing.T) { mgr, mockClient := newLiveManagerForTest(t) ctx := context.Background() diff --git a/spinifex/testutil/nats.go b/spinifex/testutil/nats.go index 160d974d7..42ad5aa86 100644 --- a/spinifex/testutil/nats.go +++ b/spinifex/testutil/nats.go @@ -94,6 +94,7 @@ var vpcdStubTopics = []string{ "vpc.update-sg", "vpc.update-port-sgs", "vpc.create-port", + "vpc.delete-port", } // vpcdStubRegistry holds the per-conn response map. Per-conn so concurrent tests with separate conns don't interfere. diff --git a/spinifex/vm/lifecycle.go b/spinifex/vm/lifecycle.go index c5e96b985..1c5d97c1e 100644 --- a/spinifex/vm/lifecycle.go +++ b/spinifex/vm/lifecycle.go @@ -1129,14 +1129,17 @@ const EBSHotPlugSlotCount = 11 // which. The primary data ENI is marked DHCP (OVN serves it; it carries the // default route, and bringing it up before cloud-init's network stage is what // lets the Ec2 datasource reach IMDS — without it cloud-init brings up the mgmt -// NIC instead and falls to DataSourceNone). Every extra ENI is DHCP too, but -// never the default route. mgmt0 lives on br-mgmt with no DHCP, so its static -// address is delivered here; it is never the default route either. The blob -// must name every NIC QEMU is given: the guest configures by MAC, so a NIC the -// blob omits is left down and address-less, unreachable at its own VPC IP. The -// blob key format matches daemon.buildNetcfgBlob and build/microvm/init.sh. -// No-op without a mgmt NIC, so single-NIC guests are untouched — cloud-init -// brings their one NIC up. +// NIC instead and falls to DataSourceNone). Every extra ENI is static when a +// prefix and gateway were resolved at launch (an RDS DB VM's customer ENI is +// the case): DHCP would depend on the OVN lease being renewed, which nothing +// does, and the NIC goes address-less an hour after boot. mgmt0 lives on +// br-mgmt with no DHCP, so its static address is delivered here too; neither +// it nor an extra ENI is ever the default route. The blob must name every NIC +// QEMU is given: the guest configures by MAC, so a NIC the blob omits is left +// down and address-less, unreachable at its own VPC IP. The blob key format +// matches daemon.buildNetcfgBlob and build/microvm/init.sh. No-op without a +// mgmt NIC, so single-NIC guests are untouched — cloud-init brings their one +// NIC up. func (m *Manager) appendSystemNetcfgFwCfg(instance *VM) error { if instance.MgmtMAC == "" || instance.MgmtIP == "" { return nil @@ -1148,15 +1151,22 @@ func (m *Manager) appendSystemNetcfgFwCfg(instance *VM) error { fmt.Fprintf(&b, "NIC%d_MAC=%s\nNIC%d_DHCP=1\nNIC%d_DEFAULT=1\n", n, instance.ENIMac, n, n) n++ } - // Additional VPC ENIs: an RDS DB VM's customer-VPC NIC is the case. OVN - // serves DHCP on each, which is also what supplies their subnet routes. - // None may take the default route — that stays with the primary ENI, - // which carries the IMDS path. + // Additional VPC ENIs: an RDS DB VM's customer-VPC NIC is the case. Static + // when the launcher resolved a prefix and gateway, so it never depends on + // the OVN lease being renewed. Falls back to DHCP for an extra ENI that + // predates this field or whose subnet could not be resolved. None may take + // the default route — that stays with the primary ENI, which carries the + // IMDS path. for _, extra := range instance.ExtraENIs { if extra.ENIMac == "" { continue } - fmt.Fprintf(&b, "NIC%d_MAC=%s\nNIC%d_DHCP=1\nNIC%d_DEFAULT=0\n", n, extra.ENIMac, n, n) + if extra.ENICIDRPrefix > 0 && extra.Gateway != "" { + fmt.Fprintf(&b, "NIC%d_MAC=%s\nNIC%d_CIDR=%s/%d\nNIC%d_GW=%s\nNIC%d_DEFAULT=0\n", + n, extra.ENIMac, n, extra.ENIIP, extra.ENICIDRPrefix, n, extra.Gateway, n) + } else { + fmt.Fprintf(&b, "NIC%d_MAC=%s\nNIC%d_DHCP=1\nNIC%d_DEFAULT=0\n", n, extra.ENIMac, n, n) + } n++ } // Management NIC: static, off br-mgmt, never the default route. diff --git a/spinifex/vm/mgmt_netcfg_test.go b/spinifex/vm/mgmt_netcfg_test.go index e99599e07..3e3348819 100644 --- a/spinifex/vm/mgmt_netcfg_test.go +++ b/spinifex/vm/mgmt_netcfg_test.go @@ -33,14 +33,52 @@ func TestAppendSystemNetcfgFwCfg(t *testing.T) { // The guest configures by MAC, so a NIC the blob omits is left down and // address-less. An RDS DB VM spans three: the system-VPC ENI, the customer - // ENI it is actually reached on, and mgmt0. - t.Run("names every NIC the VM is given, extras DHCP but never the default route", func(t *testing.T) { + // ENI it is actually reached on, and mgmt0. The customer ENI is static + // (NIC1_CIDR + NIC1_GW), not DHCP: OVN's lease is never renewed by the + // guest's one-shot udhcpc, so a DHCP customer ENI goes address-less an + // hour after boot. + t.Run("names every NIC the VM is given, customer ENI static and never the default route", func(t *testing.T) { t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) inst := &VM{ ID: "i-rds01", ENIMac: "02:3c:8f:54:bd:c9", MgmtMAC: "02:aa:bb:cc:dd:ee", MgmtIP: "10.20.0.5", + ExtraENIs: []ExtraENI{ + { + ENIID: "eni-customer", + ENIMac: "02:31:ed:a2:ed:27", + ENIIP: "172.31.0.4", + ENICIDRPrefix: 20, + Gateway: "172.31.0.1", + }, + }, + } + + require.NoError(t, m.appendSystemNetcfgFwCfg(inst)) + + require.Len(t, inst.Config.FwCfg, 1) + data, err := os.ReadFile(inst.Config.FwCfg[0].File) + require.NoError(t, err) + blob := string(data) + require.Equal(t, + "NIC0_MAC=02:3c:8f:54:bd:c9\nNIC0_DHCP=1\nNIC0_DEFAULT=1\n"+ + "NIC1_MAC=02:31:ed:a2:ed:27\nNIC1_CIDR=172.31.0.4/20\nNIC1_GW=172.31.0.1\nNIC1_DEFAULT=0\n"+ + "NIC2_MAC=02:aa:bb:cc:dd:ee\nNIC2_CIDR=10.20.0.5/24\nNIC2_DEFAULT=0\n", + blob) + require.NotContains(t, blob, "NIC1_DHCP", "the customer ENI must not depend on the OVN DHCP lease being renewed") + }) + + // An extra ENI without a resolved prefix/gateway (e.g. persisted state from + // before this field existed) falls back to DHCP rather than emitting a + // bogus static config. + t.Run("extra ENI without a resolved gateway falls back to DHCP", func(t *testing.T) { + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + inst := &VM{ + ID: "i-rds02", + ENIMac: "02:3c:8f:54:bd:c9", + MgmtMAC: "02:aa:bb:cc:dd:ee", + MgmtIP: "10.20.0.5", ExtraENIs: []ExtraENI{ {ENIID: "eni-customer", ENIMac: "02:31:ed:a2:ed:27"}, }, diff --git a/spinifex/vm/shutdown.go b/spinifex/vm/shutdown.go index ccc4d4700..d32719a7e 100644 --- a/spinifex/vm/shutdown.go +++ b/spinifex/vm/shutdown.go @@ -430,8 +430,9 @@ func (m *Manager) terminateCleanup(instance *VM) { } } - // ENI KV delete is sync; vpc.delete-port (OVN LSP) is fire-and-forget, so - // the OVN port removal is recorded pending (reconcile LSP prune reaps it). + // ENI KV delete is sync; vpc.delete-port (OVN LSP) is request-reply but + // non-fatal on failure, so the OVN port removal is recorded pending + // (reconcile LSP prune reaps anything the request-reply missed). eniErr := m.deps.InstanceCleaner.DetachAndDeleteENI(instance) if instance.ENIId != "" { m.markTeardownResult(instance, TeardownENI, eniErr) diff --git a/spinifex/vm/vm.go b/spinifex/vm/vm.go index e40583112..f94fc381e 100644 --- a/spinifex/vm/vm.go +++ b/spinifex/vm/vm.go @@ -33,12 +33,19 @@ type InstanceHealthState struct { } // ExtraENI describes an additional VPC network interface attached to a VM -// beyond the primary ENI. Only system VMs (ALBs) use multiple ENIs today. +// beyond the primary ENI. Only system VMs (ALBs, RDS DB VMs) use multiple +// ENIs today. type ExtraENI struct { ENIID string `json:"eni_id"` ENIMac string `json:"eni_mac"` ENIIP string `json:"eni_ip"` SubnetID string `json:"subnet_id,omitempty"` + + // ENICIDRPrefix and Gateway let this ENI be configured statically instead + // of via DHCP, so it never depends on the OVN lease being renewed (an RDS + // DB VM's customer ENI is the case). Zero/empty falls back to DHCP. + ENICIDRPrefix int `json:"eni_cidr_prefix,omitempty"` + Gateway string `json:"gateway,omitempty"` } type VM struct {