diff --git a/spinifex/network/reconcile/apply.go b/spinifex/network/reconcile/apply.go index d72097ce9..43fa44770 100644 --- a/spinifex/network/reconcile/apply.go +++ b/spinifex/network/reconcile/apply.go @@ -2,6 +2,7 @@ package reconcile import ( "context" + "errors" "fmt" "log/slog" "maps" @@ -271,6 +272,30 @@ func (r *reconciler) applySGs(ctx context.Context, intent IntentState, actual Ac } } +// desiredPortGroups maps an ENI's SGs to OVN port group names, erroring unless +// every one of them exists. An empty result is never valid: a port in no port +// group matches no SG ACL, including the per-group default-denies, so it would +// come up unrestricted. +func desiredPortGroups(spec topology.PortSpec, actual ActualState) ([]string, error) { + if len(spec.SGIDs) == 0 { + return nil, errors.New("ENI has no security groups") + } + pgNames := make([]string, 0, len(spec.SGIDs)) + var missing []string + for _, sgID := range spec.SGIDs { + pgName := topology.SecurityGroupPortGroup(sgID) + if _, ok := actual.PortGroups[pgName]; !ok { + missing = append(missing, pgName) + continue + } + pgNames = append(pgNames, pgName) + } + if len(missing) > 0 { + return nil, fmt.Errorf("security group port groups missing in OVN: %s", strings.Join(missing, ", ")) + } + return pgNames, nil +} + // applyPorts ensures each intent ENI has an LSP with PG memberships matching its // SGIDs. Existing ports use diff-based UpdatePortGroupMemberships to avoid gaps. // When pruneOrphans is true, ENI LSPs with no matching intent ENI are torn down; @@ -279,15 +304,13 @@ func (r *reconciler) applyPorts(ctx context.Context, intent IntentState, actual for portID, spec := range intent.Ports { portName := topology.Port(portID) switchName := topology.SubnetSwitch(spec.SubnetID) - desiredPGs := make([]string, 0, len(spec.SGIDs)) - for _, sgID := range spec.SGIDs { - pgName := topology.SecurityGroupPortGroup(sgID) - if _, ok := actual.PortGroups[pgName]; !ok { - slog.Warn("reconcile/apply: skipping port SG membership — port group missing in OVN", - "port", portName, "sg", sgID, "pg", pgName) - continue - } - desiredPGs = append(desiredPGs, pgName) + desiredPGs, err := desiredPortGroups(spec, actual) + if err != nil { + // Fail closed. Every SG ACL, including the default-denies, hangs off + // a port group, so a port in none of them is unrestricted. + slog.Error("reconcile/apply: refusing port with unprogrammable policy", "port", portName, "err", err) + res.fail(classPort, portName, err) + continue } if _, err := r.ovn.GetLogicalSwitchPort(ctx, portName); err != nil { diff --git a/spinifex/network/reconcile/apply_test.go b/spinifex/network/reconcile/apply_test.go index ff18a0767..1f413c416 100644 --- a/spinifex/network/reconcile/apply_test.go +++ b/spinifex/network/reconcile/apply_test.go @@ -90,7 +90,7 @@ func TestReconcile_PortSuppressDHCP(t *testing.T) { intent.Ports["eni-static"] = topology.PortSpec{ PortID: "eni-static", SubnetID: "subnet-a", VPCID: "vpc-a", PrivateIP: netip.MustParseAddr("10.0.1.11"), MAC: mac, - SuppressDHCP: true, + SGIDs: []string{"sg-a"}, SuppressDHCP: true, } if err := rec.Reconcile(ctx, intent); err != nil { @@ -486,6 +486,125 @@ func TestReconcile_PortMembershipDriftCorrected(t *testing.T) { } } +// Every SG ACL, including the priority 900/800 default-denies, hangs off a port +// group, so a port in none of them is unrestricted. When a port's policy cannot +// be programmed in full, applyPorts must refuse the port rather than create it. +func TestReconcile_UnprogrammablePolicyDoesNotCreatePort(t *testing.T) { + ctx := context.Background() + + tests := []struct { + name string + mutate func(*IntentState) + }{ + { + // The OVN-recreate / cold-chassis window: applySGs has not yet + // built the port group this pass. + name: "every port group missing", + mutate: func(in *IntentState) { + delete(in.SGs, "sg-a") + }, + }, + { + // A partial miss must not land the port in the subset that exists: + // the absent SG's rules would silently not apply. + name: "one port group missing", + mutate: func(in *IntentState) { + port := in.Ports["eni-a"] + port.SGIDs = append(port.SGIDs, "sg-b") + in.Ports["eni-a"] = port + }, + }, + { + // A legacy ENI record written before SG defaulting: the field is + // omitempty and unmarshals to nil with no migration to backfill it. + name: "no security groups at all", + mutate: func(in *IntentState) { + port := in.Ports["eni-a"] + port.SGIDs = nil + in.Ports["eni-a"] = port + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + rec, m := newTestReconciler(t) + intent := freshIntent(t) + tc.mutate(&intent) + + err := rec.Reconcile(ctx, intent) + if !errors.Is(err, ErrPassIncomplete) { + t.Errorf("Reconcile err = %v, want ErrPassIncomplete: the port must be reported unconverged", err) + } + if _, ok := m.Ports["port-eni-a"]; ok { + t.Error("port created with an unprogrammable policy — it would be unrestricted") + } + }) + } +} + +// The same rule on the update path: an existing port whose desired groups have +// gone missing keeps the memberships it has. Applying the diff would strip a +// live guest bare, which is worse than leaving it on a stale policy. +func TestReconcile_UnprogrammablePolicyKeepsExistingMemberships(t *testing.T) { + ctx := context.Background() + rec, m := newTestReconciler(t) + + intent := freshIntent(t) + if err := rec.Reconcile(ctx, intent); err != nil { + t.Fatalf("Reconcile #1: %v", err) + } + if m.Ports["port-eni-a"] == nil { + t.Fatal("ENI port not created") + } + + // A pass whose EnsureSGPortGroup failed sees the port group as absent while + // the LSP is still joined to it in OVN. + actual := newActualState() + var res passResult + rec.applyPorts(ctx, intent, actual, false, &res) + if len(res.failures) == 0 { + t.Error("applyPorts reported no failure for an unprogrammable policy") + } + + names, err := m.ListPortGroupsForPort(ctx, "port-eni-a") + if err != nil { + t.Fatalf("ListPortGroupsForPort: %v", err) + } + if len(names) == 0 { + t.Error("live port stripped of every port group — it is now unrestricted") + } +} + +// Once applySGs catches up, the next pass creates the port and joins it: the +// refusal is a window that heals, not a permanent block. +func TestReconcile_RefusedPortHealsOncePortGroupExists(t *testing.T) { + ctx := context.Background() + rec, m := newTestReconciler(t) + + intent := freshIntent(t) + withoutSG := intent + withoutSG.SGs = map[string]policy.SGSpec{} + if err := rec.Reconcile(ctx, withoutSG); !errors.Is(err, ErrPassIncomplete) { + t.Fatalf("Reconcile #1 err = %v, want ErrPassIncomplete", err) + } + if _, ok := m.Ports["port-eni-a"]; ok { + t.Fatal("port created before its port group existed") + } + + if err := rec.Reconcile(ctx, intent); err != nil { + t.Fatalf("Reconcile #2: %v", err) + } + storedPort := m.Ports["port-eni-a"] + if storedPort == nil { + t.Fatal("port not created once its port group existed") + } + pg := m.PortGroups[topology.SecurityGroupPortGroup("sg-a")] + if pg == nil || !slices.Contains(pg.Ports, storedPort.UUID) { + t.Error("healed port not joined to its SG port group") + } +} + // TestReconcile_PublicInstanceExemptFromDropGate locks the post-reboot regression: a // reconcile that drop-gates an IGW-attached subnet with no 0.0.0.0/0 route must also // install the /32 reroute above the gate for every public-IP instance in that subnet, diff --git a/spinifex/network/reconcile/invariants_test.go b/spinifex/network/reconcile/invariants_test.go index 02be17343..b33c2fd1e 100644 --- a/spinifex/network/reconcile/invariants_test.go +++ b/spinifex/network/reconcile/invariants_test.go @@ -31,6 +31,7 @@ func TestI1_GuestLSPMustHavePortSecurity(t *testing.T) { port := topology.PortSpec{ PortID: "eni-a", SubnetID: "subnet-a", VPCID: "vpc-a", PrivateIP: netip.MustParseAddr("10.0.1.5"), MAC: mac, + SGIDs: []string{"sg-a"}, } if err := mgr.EnsureVPC(ctx, vpc); err != nil { t.Fatalf("EnsureVPC: %v", err) @@ -38,6 +39,9 @@ func TestI1_GuestLSPMustHavePortSecurity(t *testing.T) { if err := mgr.EnsureSubnet(ctx, sub); err != nil { t.Fatalf("EnsureSubnet: %v", err) } + if err := mgr.EnsureSGPortGroup(ctx, "sg-a"); err != nil { + t.Fatalf("EnsureSGPortGroup: %v", err) + } if err := mgr.EnsurePort(ctx, port); err != nil { t.Fatalf("EnsurePort: %v", err) } diff --git a/spinifex/network/subscribers/subscribers_test.go b/spinifex/network/subscribers/subscribers_test.go index 0fe8e7a1a..19b420b1f 100644 --- a/spinifex/network/subscribers/subscribers_test.go +++ b/spinifex/network/subscribers/subscribers_test.go @@ -655,10 +655,12 @@ func TestHandlePort_FlushesStaleMACBinding(t *testing.T) { require.NoError(t, topo.EnsureVPC(ctx, topology.VPCSpec{VPCID: "vpc-a", CIDR: netip.MustParsePrefix("172.31.0.0/16")})) require.NoError(t, topo.EnsureSubnet(ctx, topology.SubnetSpec{SubnetID: "subnet-a", VPCID: "vpc-a", CIDR: netip.MustParsePrefix("172.31.0.0/24")})) + require.NoError(t, topo.EnsureSGPortGroup(ctx, "sg-a")) evt := PortEvent{ NetworkInterfaceId: "eni-new", SubnetId: "subnet-a", VpcId: "vpc-a", PrivateIpAddress: "172.31.0.4", MacAddress: "02:00:00:00:00:aa", + SecurityGroupIds: []string{"sg-a"}, } sub.handleCreatePort(&nats.Msg{Data: mustJSON(t, evt)}) diff --git a/spinifex/network/topology/manager_live.go b/spinifex/network/topology/manager_live.go index 4ed1de147..0453ce837 100644 --- a/spinifex/network/topology/manager_live.go +++ b/spinifex/network/topology/manager_live.go @@ -301,6 +301,11 @@ func (m *liveManager) EnsurePort(ctx context.Context, spec PortSpec) error { if spec.PortID == "" || spec.SubnetID == "" { return fmt.Errorf("EnsurePort: PortID/SubnetID required") } + // A port in no port group matches no SG ACL, including the per-group + // default-denies, so it would come up unrestricted. + if len(spec.SGIDs) == 0 { + return fmt.Errorf("EnsurePort: %q has no security groups", spec.PortID) + } portName := Port(spec.PortID) switchName := SubnetSwitch(spec.SubnetID) @@ -437,6 +442,11 @@ func (m *liveManager) SetPortSecurityGroups(ctx context.Context, portID string, if m.ovn == nil { return fmt.Errorf("OVN client not connected") } + // Stripping the last group would leave a live port unrestricted; teardown + // goes through DeletePort, which clears memberships and removes the LSP. + if len(sgIDs) == 0 { + return fmt.Errorf("SetPortSecurityGroups: %q would be left in no port group", portID) + } portName := Port(portID) if _, err := m.reconcilePortSGs(ctx, portName, sgIDs); err != nil { return fmt.Errorf("reconcile SGs for port %q: %w", portName, err) diff --git a/spinifex/network/topology/manager_live_test.go b/spinifex/network/topology/manager_live_test.go index 3d029bb96..056c58847 100644 --- a/spinifex/network/topology/manager_live_test.go +++ b/spinifex/network/topology/manager_live_test.go @@ -16,6 +16,17 @@ func newLiveManagerForTest(t *testing.T) (Manager, *mock.Client) { return NewLiveManager(m), m } +// seedSG creates the SG's port group and returns the ID as a PortSpec.SGIDs +// value. Every ENI carries at least one SG, and EnsurePort now refuses a port +// that would land in no port group. +func seedSG(ctx context.Context, t *testing.T, mgr Manager, sgID string) []string { + t.Helper() + if err := mgr.EnsureSGPortGroup(ctx, sgID); err != nil { + t.Fatalf("EnsureSGPortGroup(%s): %v", sgID, err) + } + return []string{sgID} +} + func TestLiveManager_EnsureVPC(t *testing.T) { mgr, mockClient := newLiveManagerForTest(t) ctx := context.Background() @@ -94,6 +105,7 @@ func TestLiveManager_EnsurePort(t *testing.T) { VPCID: vpc.VPCID, PrivateIP: netip.MustParseAddr("10.2.1.10"), MAC: mac, + SGIDs: seedSG(ctx, t, mgr, "sg-port"), } if err := mgr.EnsurePort(ctx, port); err != nil { t.Fatalf("EnsurePort: %v", err) @@ -130,10 +142,12 @@ func TestLiveManager_EnsurePort_SuppressDHCP(t *testing.T) { t.Fatalf("EnsureSubnet: %v", err) } + sgIDs := seedSG(ctx, t, mgr, "sg-nodhcp") dhcpMAC, _ := net.ParseMAC("02:00:00:00:02:01") dhcpPort := PortSpec{ PortID: "eni-dhcp", SubnetID: sub.SubnetID, VPCID: vpc.VPCID, PrivateIP: netip.MustParseAddr("10.7.1.10"), MAC: dhcpMAC, + SGIDs: sgIDs, } if err := mgr.EnsurePort(ctx, dhcpPort); err != nil { t.Fatalf("EnsurePort(dhcp): %v", err) @@ -150,7 +164,7 @@ func TestLiveManager_EnsurePort_SuppressDHCP(t *testing.T) { staticPort := PortSpec{ PortID: "eni-static", SubnetID: sub.SubnetID, VPCID: vpc.VPCID, PrivateIP: netip.MustParseAddr("10.7.1.11"), MAC: staticMAC, - SuppressDHCP: true, + SGIDs: sgIDs, SuppressDHCP: true, } if err := mgr.EnsurePort(ctx, staticPort); err != nil { t.Fatalf("EnsurePort(static): %v", err) @@ -175,6 +189,7 @@ func TestLiveManager_DeletePort(t *testing.T) { port := PortSpec{ PortID: "eni-DP", SubnetID: sub.SubnetID, VPCID: vpc.VPCID, PrivateIP: netip.MustParseAddr("10.3.1.5"), MAC: mac, + SGIDs: seedSG(ctx, t, mgr, "sg-dp"), } if err := mgr.EnsurePort(ctx, port); err != nil { t.Fatalf("EnsurePort: %v", err) @@ -209,6 +224,7 @@ func TestLiveManager_DeletePort_AlreadyAbsentIsNoop(t *testing.T) { port2 := PortSpec{ PortID: "eni-DP2", SubnetID: sub.SubnetID, VPCID: vpc.VPCID, PrivateIP: netip.MustParseAddr("10.5.1.5"), MAC: mac, + SGIDs: seedSG(ctx, t, mgr, "sg-dp2"), } if err := mgr.EnsurePort(ctx, port2); err != nil { t.Fatalf("EnsurePort: %v", err) @@ -233,10 +249,12 @@ func TestLiveManager_DeletePort_ThenRecreateSameIP_NoDuplicateLSP(t *testing.T) _ = mgr.EnsureVPC(ctx, vpc) _ = mgr.EnsureSubnet(ctx, sub) + sgIDs := seedSG(ctx, t, mgr, "sg-reuse") 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, + SGIDs: sgIDs, } if err := mgr.EnsurePort(ctx, oldPort); err != nil { t.Fatalf("EnsurePort(old): %v", err) @@ -249,6 +267,7 @@ func TestLiveManager_DeletePort_ThenRecreateSameIP_NoDuplicateLSP(t *testing.T) newPort := PortSpec{ PortID: "eni-new", SubnetID: sub.SubnetID, VPCID: vpc.VPCID, PrivateIP: netip.MustParseAddr("172.31.0.4"), MAC: newMAC, + SGIDs: sgIDs, } if err := mgr.EnsurePort(ctx, newPort); err != nil { t.Fatalf("EnsurePort(new): %v", err) @@ -404,6 +423,49 @@ func TestLiveManager_SetPortSecurityGroups(t *testing.T) { } } +// Every SG ACL, including the priority 900/800 default-denies, is attached to a +// port group, so a port in none of them is unrestricted. EnsurePort must refuse +// to create one and SetPortSecurityGroups must refuse to strip a live one bare. +func TestLiveManager_NoSecurityGroupsIsRefused(t *testing.T) { + mgr, mockClient := newLiveManagerForTest(t) + ctx := context.Background() + vpc := VPCSpec{VPCID: "vpc-nosg", CIDR: netip.MustParsePrefix("10.9.0.0/16")} + sub := SubnetSpec{SubnetID: "subnet-nosg", VPCID: vpc.VPCID, CIDR: netip.MustParsePrefix("10.9.1.0/24")} + _ = mgr.EnsureVPC(ctx, vpc) + _ = mgr.EnsureSubnet(ctx, sub) + + mac, _ := net.ParseMAC("02:00:00:00:09:01") + bare := PortSpec{ + PortID: "eni-bare", SubnetID: sub.SubnetID, VPCID: vpc.VPCID, + PrivateIP: netip.MustParseAddr("10.9.1.5"), MAC: mac, + } + if err := mgr.EnsurePort(ctx, bare); err == nil { + t.Error("EnsurePort with no SGs must fail: the LSP would match no SG ACL") + } + if _, err := mockClient.GetLogicalSwitchPort(ctx, Port(bare.PortID)); err == nil { + t.Error("refused EnsurePort must leave no LSP behind") + } + + live := PortSpec{ + PortID: "eni-live", SubnetID: sub.SubnetID, VPCID: vpc.VPCID, + PrivateIP: netip.MustParseAddr("10.9.1.6"), MAC: mac, + SGIDs: seedSG(ctx, t, mgr, "sg-nosg"), + } + if err := mgr.EnsurePort(ctx, live); err != nil { + t.Fatalf("EnsurePort: %v", err) + } + if err := mgr.SetPortSecurityGroups(ctx, live.PortID, nil); err == nil { + t.Error("SetPortSecurityGroups with no SGs must fail: it would strip the port bare") + } + names, err := mockClient.ListPortGroupsForPort(ctx, Port(live.PortID)) + if err != nil { + t.Fatalf("ListPortGroupsForPort: %v", err) + } + if len(names) != 1 || names[0] != SecurityGroupPortGroup("sg-nosg") { + t.Errorf("live port must keep its membership, got %v", names) + } +} + func TestLiveManager_WithDNSServer(t *testing.T) { mockClient := mock.New() _ = mockClient.Connect(context.Background())