diff --git a/spinifex/network/policy/acl.go b/spinifex/network/policy/acl.go new file mode 100644 index 000000000..b2ebc2054 --- /dev/null +++ b/spinifex/network/policy/acl.go @@ -0,0 +1,226 @@ +package policy + +import ( + "fmt" + "strings" + + "github.com/mulgadc/spinifex/spinifex/network/ovn" + "github.com/mulgadc/spinifex/spinifex/network/topology" +) + +// ACL priority table. Higher priority wins. Tenants only ever populate +// priority 1000; the 1100/1050/900/800 bands are platform-reserved. +// +// See docs/development/feature/spinifex-network-redesign.md §8.1 for the +// authoritative table. +const ( + // ACLPriorityAllowIntraSG allows unconditional traffic between members + // of the same port group. Reserved for future use; not yet emitted by + // InfrastructureACLs. + ACLPriorityAllowIntraSG = 1100 + + // ACLPriorityAllowDHCP sits above tenant rules so a narrow-egress SG + // (e.g. only-VPC-CIDR) doesn't silently drop the guest's DHCPDISCOVER + // against the priority-900 default-deny. AWS users can't write a rule + // for 255.255.255.255, so the platform must. + ACLPriorityAllowDHCP = 1050 + + // ACLPriorityTenantAllow is the band tenant ingress/egress allow rules + // emit at. Allows are not logged — high volume, low signal on a private + // network. + ACLPriorityTenantAllow = 1000 + + // ACLPriorityDefaultDenyIngress is the logged drop for unmatched + // ingress. Severity = denyACLSeverity. CMMC SC.L1-3.13.1. + ACLPriorityDefaultDenyIngress = 900 + + // ACLPriorityDefaultDenyEgress mirrors the ingress default-deny on the + // egress direction. + ACLPriorityDefaultDenyEgress = 800 +) + +// denyACLSeverity is the syslog severity OVN logs against a default-deny ACL +// hit. "info" is loud enough for a syslog forwarder to capture, quiet enough +// that a port scan doesn't page anyone. Operators can promote it at the log +// collector if they want higher-priority alerts. +const denyACLSeverity = "info" + +// Rule is the policy-layer representation of an AWS-style security group +// rule. The handler layer constructs Rules from the AWS API payload after +// validating CidrIp / SourceSG — this builder interpolates the values +// verbatim, so untrusted input must not reach here. +type Rule struct { + IPProtocol string // "tcp", "udp", "icmp", or "-1" (all) + FromPort int64 + ToPort int64 + CIDR string + SourceSG string +} + +// InfrastructureACLs returns the platform ACLs that every port group carries +// regardless of tenant rules: the priority-900/800 default-deny pair (logged +// for CMMC SC.L1-3.13.1 boundary monitoring) and the priority-1050 DHCPv4 +// client/server allows. +// +// OVN's built-in DHCP responder runs inside the LS pipeline after the +// port-group ACL stage, so without these allows a narrow-egress tenant SG +// silently drops the guest's DHCPDISCOVER (dst=255.255.255.255) against the +// default-deny and the VM never gets a lease. Matches AWS's implicit allow. +func InfrastructureACLs(portGroupName string) []ovn.ACLSpec { + return []ovn.ACLSpec{ + denyIngressACL(portGroupName), + denyEgressACL(portGroupName), + dhcpEgressACL(portGroupName), + dhcpIngressACL(portGroupName), + } +} + +// RuleACLSpecs builds the priority-1000 allow ACL specs for the given +// tenant rules. Action is "allow-related" so reply traffic for established +// flows passes without requiring an explicit reverse-direction rule. +func RuleACLSpecs(portGroupName string, ingress, egress []Rule) []ovn.ACLSpec { + specs := make([]ovn.ACLSpec, 0, len(ingress)+len(egress)) + for _, rule := range ingress { + specs = append(specs, ovn.ACLSpec{ + Direction: "to-lport", + Priority: ACLPriorityTenantAllow, + Match: BuildIngressACLMatch(portGroupName, rule), + Action: "allow-related", + }) + } + for _, rule := range egress { + specs = append(specs, ovn.ACLSpec{ + Direction: "from-lport", + Priority: ACLPriorityTenantAllow, + Match: BuildEgressACLMatch(portGroupName, rule), + Action: "allow-related", + }) + } + return specs +} + +// BuildIngressACLMatch generates an OVN ACL match expression for an ingress +// rule. Ingress rules use "outport == @{pgName}" because OVN ACLs with +// direction "to-lport" match on the destination port (outport from the +// pipeline's perspective). +// +// Tenant-supplied CIDR/SourceSG values MUST be validated by the handler +// (see handlers/ec2/vpc/security_group.go) before reaching this builder; +// values are interpolated verbatim. +func BuildIngressACLMatch(portGroupName string, rule Rule) string { + parts := []string{fmt.Sprintf("outport == @%s", portGroupName), "ip4"} + parts = appendProtocolMatch(parts, rule) + if rule.CIDR != "" && rule.CIDR != "0.0.0.0/0" { + parts = append(parts, fmt.Sprintf("ip4.src == %s", rule.CIDR)) + } + if rule.SourceSG != "" { + parts = append(parts, fmt.Sprintf("ip4.src == $%s", addressSetName(topology.SecurityGroupPortGroup(rule.SourceSG)))) + } + return strings.Join(parts, " && ") +} + +// BuildEgressACLMatch generates an OVN ACL match expression for an egress +// rule. Egress rules use "inport == @{pgName}" because OVN ACLs with +// direction "from-lport" match on the source port (inport from the +// pipeline's perspective). +func BuildEgressACLMatch(portGroupName string, rule Rule) string { + parts := []string{fmt.Sprintf("inport == @%s", portGroupName), "ip4"} + parts = appendProtocolMatch(parts, rule) + if rule.CIDR != "" && rule.CIDR != "0.0.0.0/0" { + parts = append(parts, fmt.Sprintf("ip4.dst == %s", rule.CIDR)) + } + if rule.SourceSG != "" { + parts = append(parts, fmt.Sprintf("ip4.dst == $%s", addressSetName(topology.SecurityGroupPortGroup(rule.SourceSG)))) + } + return strings.Join(parts, " && ") +} + +// addressSetName returns the SB Address_Set name that ovn-northd +// auto-derives for the given port group's IPv4 addresses. SG-to-SG ACL +// matches reference it via "ip4.src == $" / "ip4.dst == $". +// Lifecycle is owned by ovn-northd — Spinifex must NOT create explicit NB +// Address_Set rows with these names; a duplicate sync to SB wedges northd +// in an "OVNSB commit failed" retry loop. +func addressSetName(portGroupName string) string { + return portGroupName + "_ip4" +} + +func appendProtocolMatch(parts []string, rule Rule) []string { + switch rule.IPProtocol { + case "tcp": + parts = appendPortMatch(parts, "tcp", rule.FromPort, rule.ToPort) + case "udp": + parts = appendPortMatch(parts, "udp", rule.FromPort, rule.ToPort) + case "icmp": + parts = append(parts, "icmp4") + case "-1", "": + // All protocols — no additional filter beyond "ip4". + default: + // Unknown protocol — treat as all traffic. + } + return parts +} + +func appendPortMatch(parts []string, proto string, fromPort, toPort int64) []string { + if fromPort == 0 && toPort == 0 { + return append(parts, proto) + } + if fromPort == toPort { + return append(parts, fmt.Sprintf("%s.dst == %d", proto, fromPort)) + } + parts = append(parts, fmt.Sprintf("%s.dst >= %d", proto, fromPort)) + parts = append(parts, fmt.Sprintf("%s.dst <= %d", proto, toPort)) + return parts +} + +func denyIngressACL(portGroupName string) ovn.ACLSpec { + return ovn.ACLSpec{ + Direction: "to-lport", + Priority: ACLPriorityDefaultDenyIngress, + Match: fmt.Sprintf("outport == @%s && ip4", portGroupName), + Action: "drop", + Name: portGroupName + "-deny-ingress", + Log: true, + Severity: denyACLSeverity, + } +} + +func denyEgressACL(portGroupName string) ovn.ACLSpec { + return ovn.ACLSpec{ + Direction: "from-lport", + Priority: ACLPriorityDefaultDenyEgress, + Match: fmt.Sprintf("inport == @%s && ip4", portGroupName), + Action: "drop", + Name: portGroupName + "-deny-egress", + Log: true, + Severity: denyACLSeverity, + } +} + +// dhcpEgressACL allows the guest's DHCP client traffic (DHCPDISCOVER, +// DHCPREQUEST: udp.src=68, udp.dst=67) out of the port. Action is "allow" +// rather than "allow-related" — DHCP is single-shot request/reply and +// allow-related on broadcast UDP interacts oddly with OVN's CT zones. +func dhcpEgressACL(portGroupName string) ovn.ACLSpec { + return ovn.ACLSpec{ + Direction: "from-lport", + Priority: ACLPriorityAllowDHCP, + Match: fmt.Sprintf("inport == @%s && udp && udp.src == 68 && udp.dst == 67", portGroupName), + Action: "allow", + Name: portGroupName + "-allow-dhcp-egress", + } +} + +// dhcpIngressACL allows the OVN-generated DHCP server reply (DHCPOFFER, +// DHCPACK: udp.src=67, udp.dst=68) back to the guest. OVN's DHCP responder +// emits the reply from inside the LS pipeline, so without this matching +// to-lport allow the reply hits the priority-900 ingress default-deny. +func dhcpIngressACL(portGroupName string) ovn.ACLSpec { + return ovn.ACLSpec{ + Direction: "to-lport", + Priority: ACLPriorityAllowDHCP, + Match: fmt.Sprintf("outport == @%s && udp && udp.src == 67 && udp.dst == 68", portGroupName), + Action: "allow", + Name: portGroupName + "-allow-dhcp-ingress", + } +} diff --git a/spinifex/network/policy/acl_test.go b/spinifex/network/policy/acl_test.go new file mode 100644 index 000000000..bb7f773ab --- /dev/null +++ b/spinifex/network/policy/acl_test.go @@ -0,0 +1,136 @@ +package policy + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestACL_TCPPortFromCIDR(t *testing.T) { + match := BuildIngressACLMatch("sg_test", Rule{ + IPProtocol: "tcp", + FromPort: 22, + ToPort: 22, + CIDR: "10.0.0.0/8", + }) + assert.Contains(t, match, "tcp.dst == 22") + assert.Contains(t, match, "ip4.src == 10.0.0.0/8") + assert.Contains(t, match, "outport == @sg_test") + assert.Contains(t, match, "ip4") +} + +func TestACL_AllTrafficFromSG(t *testing.T) { + match := BuildIngressACLMatch("sg_test", Rule{ + IPProtocol: "-1", + SourceSG: "sg-abc123", + }) + assert.Contains(t, match, "ip4.src == $sg_abc123_ip4") + assert.Contains(t, match, "outport == @sg_test") + assert.Contains(t, match, "ip4") +} + +func TestACL_PortRange(t *testing.T) { + match := BuildIngressACLMatch("sg_test", Rule{ + IPProtocol: "udp", + FromPort: 1024, + ToPort: 65535, + }) + assert.Contains(t, match, "udp.dst >= 1024") + assert.Contains(t, match, "udp.dst <= 65535") +} + +func TestACL_ICMP(t *testing.T) { + match := BuildIngressACLMatch("sg_test", Rule{ + IPProtocol: "icmp", + CIDR: "0.0.0.0/0", + }) + assert.Contains(t, match, "icmp4") + assert.NotContains(t, match, "tcp.dst") + assert.NotContains(t, match, "udp.dst") +} + +func TestACL_AllProtocols(t *testing.T) { + match := BuildIngressACLMatch("sg_test", Rule{ + IPProtocol: "-1", + CIDR: "10.0.0.0/16", + }) + assert.Contains(t, match, "ip4") + assert.Contains(t, match, "ip4.src == 10.0.0.0/16") + assert.NotContains(t, match, "tcp") + assert.NotContains(t, match, "udp") + assert.NotContains(t, match, "icmp") +} + +func TestACL_EgressAll(t *testing.T) { + match := BuildEgressACLMatch("sg_test", Rule{ + IPProtocol: "-1", + CIDR: "0.0.0.0/0", + }) + assert.Contains(t, match, "inport == @sg_test") + assert.NotContains(t, match, "outport") + assert.Contains(t, match, "ip4") +} + +func TestACL_TCPSinglePort(t *testing.T) { + match := BuildIngressACLMatch("sg_test", Rule{ + IPProtocol: "tcp", + FromPort: 443, + ToPort: 443, + CIDR: "10.0.0.0/8", + }) + assert.Contains(t, match, "tcp.dst == 443") + assert.NotContains(t, match, "tcp.dst >=") + assert.NotContains(t, match, "tcp.dst <=") +} + +func TestACL_NoSource(t *testing.T) { + match := BuildIngressACLMatch("sg_test", Rule{ + IPProtocol: "tcp", + FromPort: 80, + ToPort: 80, + CIDR: "0.0.0.0/0", + }) + assert.Contains(t, match, "tcp.dst == 80") + assert.NotContains(t, match, "ip4.src") +} + +func TestACL_EgressFromSGToSG(t *testing.T) { + match := BuildEgressACLMatch("sg_test", Rule{ + IPProtocol: "tcp", + FromPort: 3306, + ToPort: 3306, + SourceSG: "sg-db-tier", + }) + assert.Contains(t, match, "inport == @sg_test") + assert.Contains(t, match, "tcp.dst == 3306") + assert.Contains(t, match, "ip4.dst == $sg_db_tier_ip4") +} + +func TestInfrastructureACLs_Shape(t *testing.T) { + specs := InfrastructureACLs("sg_test") + if assert.Len(t, specs, 4) { + // Priorities: deny-ingress 900, deny-egress 800, dhcp-egress 1050, dhcp-ingress 1050 + assert.Equal(t, ACLPriorityDefaultDenyIngress, specs[0].Priority) + assert.Equal(t, "drop", specs[0].Action) + assert.True(t, specs[0].Log) + assert.Equal(t, ACLPriorityDefaultDenyEgress, specs[1].Priority) + assert.True(t, specs[1].Log) + assert.Equal(t, ACLPriorityAllowDHCP, specs[2].Priority) + assert.Equal(t, "allow", specs[2].Action) + assert.Equal(t, ACLPriorityAllowDHCP, specs[3].Priority) + } +} + +func TestRuleACLSpecs_PriorityAndAction(t *testing.T) { + specs := RuleACLSpecs("sg_test", + []Rule{{IPProtocol: "tcp", FromPort: 80, ToPort: 80, CIDR: "0.0.0.0/0"}}, + []Rule{{IPProtocol: "-1", CIDR: "0.0.0.0/0"}}, + ) + if assert.Len(t, specs, 2) { + assert.Equal(t, ACLPriorityTenantAllow, specs[0].Priority) + assert.Equal(t, "to-lport", specs[0].Direction) + assert.Equal(t, "allow-related", specs[0].Action) + assert.Equal(t, "from-lport", specs[1].Direction) + assert.Equal(t, "allow-related", specs[1].Action) + } +} diff --git a/spinifex/network/policy/nat.go b/spinifex/network/policy/nat.go new file mode 100644 index 000000000..a83df22a8 --- /dev/null +++ b/spinifex/network/policy/nat.go @@ -0,0 +1,196 @@ +package policy + +import ( + "context" + "errors" + "fmt" + "log/slog" + + "github.com/mulgadc/spinifex/spinifex/network/ovn" + "github.com/mulgadc/spinifex/spinifex/network/topology" + "github.com/mulgadc/spinifex/spinifex/services/vpcd/nbdb" +) + +// EIPSpec is a 1:1 Elastic IP NAT (dnat_and_snat) attaching ExternalIP to +// LogicalIP on the VPC's router. In NATModeDistributed, PortName and MAC +// MUST be set so OVN can install per-chassis flows; in NATModeCentralized +// they are ignored. Callers that target distributed mode without supplying +// both fields fall back to the centralised shape — the NAT still works but +// hairpins through the gateway chassis. +type EIPSpec struct { + VPCID string + ExternalIP string + LogicalIP string + PortName string // OVN LSP name backing the VM + MAC string // external MAC for distributed NAT +} + +// NATGWSpec is a NAT Gateway SNAT rule: every packet leaving the private +// subnet whose source is SubnetCIDR is rewritten to PublicIP. The OVN rule +// keys on (snat, SubnetCIDR) on the VPC's router. +type NATGWSpec struct { + VPCID string + NATGatewayID string + PublicIP string + SubnetCIDR string +} + +// NATManager owns the OVN NAT rule lifecycle for one cluster. It does not +// create the LogicalRouter — L2 (topology.Manager) owns that — and does +// not allocate public IPs (handlers/ec2/eip owns pool allocation). +// +// NAT mode is fixed at construction time from the host's UplinkMode and +// never changes at runtime. +type NATManager interface { + // AddEIP installs a dnat_and_snat rule. In NATModeDistributed, PortName + // and MAC are written to logical_port / external_mac so the rule fires + // on the VM's own chassis. Stale dnat_and_snat rules referencing the + // same ExternalIP on any router are removed first — an EIP can change + // VPC association in our pool model and a leftover rule on the old + // router would silently steal the public IP. + AddEIP(ctx context.Context, eip EIPSpec) error + + // DeleteEIP removes the dnat_and_snat rule with the given LogicalIP on + // the VPC's router. Returns nil when the rule is already absent + // (matching the idempotent semantics required by the multiple cleanup + // paths that publish vpc.delete-nat). + DeleteEIP(ctx context.Context, vpcID, logicalIP string) error + + // AddNATGateway installs the snat rule for a NAT Gateway's private + // subnet. The OVN key is (snat, SubnetCIDR); AddNATGateway therefore + // rejects two NAT GWs on overlapping subnet CIDRs. + AddNATGateway(ctx context.Context, gw NATGWSpec) error + + // DeleteNATGateway removes the snat rule keyed by SubnetCIDR on the + // VPC's router. Idempotent. + DeleteNATGateway(ctx context.Context, vpcID, subnetCIDR string) error + + // AddSNAT installs the IGW default-outbound snat rule rewriting the + // VPC CIDR to ExternalIP. AWS-parity behaviour (only EIP-backed VMs + // can egress) means callers in the IGW attach path typically skip + // this — it exists for future deployments that opt into blanket SNAT. + AddSNAT(ctx context.Context, vpcID, vpcCIDR, externalIP string) error + + // DeleteSNAT removes the IGW default-outbound snat rule for vpcCIDR. + // Idempotent. + DeleteSNAT(ctx context.Context, vpcID, vpcCIDR string) error +} + +type natManager struct { + ovn ovn.Client + mode NATMode +} + +var _ NATManager = (*natManager)(nil) + +// NewNATManager constructs a NATManager bound to one NAT mode. mode is +// resolved at startup from host.Wiring.UplinkMode() via NATModeFromUplinkMode +// and must not be NATModeUnknown. +func NewNATManager(client ovn.Client, mode NATMode) (NATManager, error) { + if mode == NATModeUnknown { + return nil, fmt.Errorf("NAT mode is unknown; resolve from host.Wiring.UplinkMode()") + } + return &natManager{ovn: client, mode: mode}, nil +} + +func (m *natManager) AddEIP(ctx context.Context, eip EIPSpec) error { + router := topology.VPCRouter(eip.VPCID) + + natRule := &nbdb.NAT{ + Type: "dnat_and_snat", + ExternalIP: eip.ExternalIP, + LogicalIP: eip.LogicalIP, + ExternalIDs: map[string]string{ + "spinifex:vpc_id": eip.VPCID, + "spinifex:public_ip": eip.ExternalIP, + }, + } + if m.mode == NATModeDistributed && eip.PortName != "" && eip.MAC != "" { + mac := eip.MAC + port := eip.PortName + natRule.ExternalMAC = &mac + natRule.LogicalPort = &port + } + + // Search every router, not just the target — stale rules may exist on + // a different VPC's router when vpc.delete-nat (fire-and-forget) hasn't + // been processed before the IP was reused by a new VPC. + if removed, err := m.ovn.DeleteAllNATsByExternalIP(ctx, "dnat_and_snat", eip.ExternalIP); err != nil { + slog.Warn("policy: stale NAT cleanup failed before AddEIP", "external_ip", eip.ExternalIP, "err", err) + } else if removed > 0 { + slog.Info("policy: cleaned stale dnat_and_snat rules before AddEIP", "external_ip", eip.ExternalIP, "removed", removed) + } + + if err := m.ovn.AddNAT(ctx, router, natRule); err != nil { + return fmt.Errorf("add dnat_and_snat %s -> %s on %s: %w", eip.LogicalIP, eip.ExternalIP, router, err) + } + return nil +} + +func (m *natManager) DeleteEIP(ctx context.Context, vpcID, logicalIP string) error { + router := topology.VPCRouter(vpcID) + if err := m.ovn.DeleteNAT(ctx, router, "dnat_and_snat", logicalIP); err != nil { + if errors.Is(err, ovn.ErrNATNotFound) { + return nil + } + return fmt.Errorf("delete dnat_and_snat %s on %s: %w", logicalIP, router, err) + } + return nil +} + +func (m *natManager) AddNATGateway(ctx context.Context, gw NATGWSpec) error { + router := topology.VPCRouter(gw.VPCID) + + snatRule := &nbdb.NAT{ + Type: "snat", + ExternalIP: gw.PublicIP, + LogicalIP: gw.SubnetCIDR, + ExternalIDs: map[string]string{ + "spinifex:vpc_id": gw.VPCID, + "spinifex:nat_gateway_id": gw.NATGatewayID, + }, + } + if err := m.ovn.AddNAT(ctx, router, snatRule); err != nil { + return fmt.Errorf("add NAT GW snat %s -> %s on %s: %w", gw.SubnetCIDR, gw.PublicIP, router, err) + } + return nil +} + +func (m *natManager) DeleteNATGateway(ctx context.Context, vpcID, subnetCIDR string) error { + router := topology.VPCRouter(vpcID) + if err := m.ovn.DeleteNAT(ctx, router, "snat", subnetCIDR); err != nil { + if errors.Is(err, ovn.ErrNATNotFound) { + return nil + } + return fmt.Errorf("delete NAT GW snat %s on %s: %w", subnetCIDR, router, err) + } + return nil +} + +func (m *natManager) AddSNAT(ctx context.Context, vpcID, vpcCIDR, externalIP string) error { + router := topology.VPCRouter(vpcID) + snatRule := &nbdb.NAT{ + Type: "snat", + ExternalIP: externalIP, + LogicalIP: vpcCIDR, + ExternalIDs: map[string]string{ + "spinifex:vpc_id": vpcID, + "spinifex:role": "igw-default-snat", + }, + } + if err := m.ovn.AddNAT(ctx, router, snatRule); err != nil { + return fmt.Errorf("add IGW snat %s -> %s on %s: %w", vpcCIDR, externalIP, router, err) + } + return nil +} + +func (m *natManager) DeleteSNAT(ctx context.Context, vpcID, vpcCIDR string) error { + router := topology.VPCRouter(vpcID) + if err := m.ovn.DeleteNAT(ctx, router, "snat", vpcCIDR); err != nil { + if errors.Is(err, ovn.ErrNATNotFound) { + return nil + } + return fmt.Errorf("delete IGW snat %s on %s: %w", vpcCIDR, router, err) + } + return nil +} diff --git a/spinifex/network/policy/nat_test.go b/spinifex/network/policy/nat_test.go new file mode 100644 index 000000000..032db44c5 --- /dev/null +++ b/spinifex/network/policy/nat_test.go @@ -0,0 +1,170 @@ +package policy + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/mulgadc/spinifex/spinifex/network/host" + "github.com/mulgadc/spinifex/spinifex/network/ovn/mock" + "github.com/mulgadc/spinifex/spinifex/network/topology" + "github.com/mulgadc/spinifex/spinifex/services/vpcd/nbdb" +) + +func seedRouter(t *testing.T, m *mock.Client, vpcID string) string { + t.Helper() + name := topology.VPCRouter(vpcID) + require.NoError(t, m.CreateLogicalRouter(context.Background(), &nbdb.LogicalRouter{Name: name})) + return name +} + +func findNAT(m *mock.Client, natType, logicalIP string) *nbdb.NAT { + for _, n := range m.NATs { + if n.Type == natType && n.LogicalIP == logicalIP { + return n + } + } + return nil +} + +func TestNATModeFromUplinkMode(t *testing.T) { + assert.Equal(t, NATModeDistributed, NATModeFromUplinkMode(host.UplinkModePhysical)) + assert.Equal(t, NATModeCentralized, NATModeFromUplinkMode(host.UplinkModeVeth)) + assert.Equal(t, NATModeUnknown, NATModeFromUplinkMode(host.UplinkModeUnknown)) +} + +func TestNewNATManager_RejectsUnknownMode(t *testing.T) { + _, err := NewNATManager(mock.New(), NATModeUnknown) + require.Error(t, err) +} + +func TestNATManager_AddEIP_Distributed_SetsPortAndMAC(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + nm, err := NewNATManager(m, NATModeDistributed) + require.NoError(t, err) + + require.NoError(t, nm.AddEIP(ctx, EIPSpec{ + VPCID: "vpc-1", + ExternalIP: "1.2.3.4", + LogicalIP: "10.0.0.5", + PortName: "port-eni-abc", + MAC: "aa:bb:cc:dd:ee:ff", + })) + + got := findNAT(m, "dnat_and_snat", "10.0.0.5") + require.NotNil(t, got) + require.NotNil(t, got.LogicalPort) + require.NotNil(t, got.ExternalMAC) + assert.Equal(t, "port-eni-abc", *got.LogicalPort) + assert.Equal(t, "aa:bb:cc:dd:ee:ff", *got.ExternalMAC) +} + +func TestNATManager_AddEIP_Centralized_LeavesPortAndMACUnset(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + nm, err := NewNATManager(m, NATModeCentralized) + require.NoError(t, err) + + require.NoError(t, nm.AddEIP(ctx, EIPSpec{ + VPCID: "vpc-1", + ExternalIP: "1.2.3.4", + LogicalIP: "10.0.0.5", + PortName: "port-eni-abc", + MAC: "aa:bb:cc:dd:ee:ff", + })) + + got := findNAT(m, "dnat_and_snat", "10.0.0.5") + require.NotNil(t, got) + assert.Nil(t, got.LogicalPort) + assert.Nil(t, got.ExternalMAC) +} + +func TestNATManager_AddEIP_RemovesStaleRuleOnOtherRouter(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-old") + seedRouter(t, m, "vpc-new") + nm, err := NewNATManager(m, NATModeDistributed) + require.NoError(t, err) + + // Stale rule still attached to vpc-old's router for the same external IP. + require.NoError(t, nm.AddEIP(ctx, EIPSpec{ + VPCID: "vpc-old", + ExternalIP: "1.2.3.4", + LogicalIP: "10.0.0.5", + PortName: "port-a", MAC: "aa:aa:aa:aa:aa:aa", + })) + // Reuse the EIP in vpc-new. + require.NoError(t, nm.AddEIP(ctx, EIPSpec{ + VPCID: "vpc-new", + ExternalIP: "1.2.3.4", + LogicalIP: "10.1.0.7", + PortName: "port-b", MAC: "bb:bb:bb:bb:bb:bb", + })) + + // Only the new rule survives. + var matching []*nbdb.NAT + for _, n := range m.NATs { + if n.Type == "dnat_and_snat" && n.ExternalIP == "1.2.3.4" { + matching = append(matching, n) + } + } + require.Len(t, matching, 1) + assert.Equal(t, "10.1.0.7", matching[0].LogicalIP) +} + +func TestNATManager_DeleteEIP_IdempotentOnMissing(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + nm, _ := NewNATManager(m, NATModeDistributed) + + // No prior AddEIP — delete should still succeed. + require.NoError(t, nm.DeleteEIP(ctx, "vpc-1", "10.0.0.5")) +} + +func TestNATManager_AddNATGateway_AndDelete(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + nm, _ := NewNATManager(m, NATModeCentralized) + + require.NoError(t, nm.AddNATGateway(ctx, NATGWSpec{ + VPCID: "vpc-1", + NATGatewayID: "nat-xyz", + PublicIP: "9.9.9.9", + SubnetCIDR: "10.0.1.0/24", + })) + got := findNAT(m, "snat", "10.0.1.0/24") + require.NotNil(t, got) + assert.Equal(t, "9.9.9.9", got.ExternalIP) + assert.Equal(t, "nat-xyz", got.ExternalIDs["spinifex:nat_gateway_id"]) + + require.NoError(t, nm.DeleteNATGateway(ctx, "vpc-1", "10.0.1.0/24")) + assert.Nil(t, findNAT(m, "snat", "10.0.1.0/24")) + + // Idempotent re-delete. + require.NoError(t, nm.DeleteNATGateway(ctx, "vpc-1", "10.0.1.0/24")) +} + +func TestNATManager_AddSNAT_AndDelete(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + nm, _ := NewNATManager(m, NATModeDistributed) + + require.NoError(t, nm.AddSNAT(ctx, "vpc-1", "10.0.0.0/16", "1.2.3.4")) + got := findNAT(m, "snat", "10.0.0.0/16") + require.NotNil(t, got) + assert.Equal(t, "1.2.3.4", got.ExternalIP) + assert.Equal(t, "igw-default-snat", got.ExternalIDs["spinifex:role"]) + + require.NoError(t, nm.DeleteSNAT(ctx, "vpc-1", "10.0.0.0/16")) + assert.Nil(t, findNAT(m, "snat", "10.0.0.0/16")) + require.NoError(t, nm.DeleteSNAT(ctx, "vpc-1", "10.0.0.0/16")) +} diff --git a/spinifex/network/policy/policy.go b/spinifex/network/policy/policy.go new file mode 100644 index 000000000..6105cf2cd --- /dev/null +++ b/spinifex/network/policy/policy.go @@ -0,0 +1,69 @@ +// Package policy is L3 of the spinifex network stack: data-plane policy +// attached to the logical objects owned by L2 (network/topology). It exposes +// three managers — SecurityGroupManager (ACLs against SG port groups), +// NATManager (dnat_and_snat for EIPs, snat for NAT gateways and IGW default +// outbound), and RouteManager (static + default routes) — and calls L1 +// (network/ovn) for the OVSDB transactions. +// +// L3 does not create or delete L2 objects. Port groups, logical routers, and +// logical switches must already exist before the L3 managers can attach +// policy to them; layering breaks are loud (L1 returns "not found" and the +// L3 method propagates the error). +// +// See docs/development/feature/spinifex-network-redesign.md §8 and +// docs/development/feature/spinifex-network-redesign-phase2.md §2.1 for the +// full contract. +package policy + +import ( + "github.com/mulgadc/spinifex/spinifex/network/host" +) + +// NATMode tells NATManager whether the cluster runs distributed or +// centralised NAT. The choice is fixed at L0 by the host's uplink mode and +// is passed in at NATManager construction time — there is no runtime +// reselection. ADR-0006 S3. +type NATMode int + +const ( + // NATModeUnknown is the zero value; never returned by a configured + // NATManager. + NATModeUnknown NATMode = iota + + // NATModeDistributed sets ExternalMAC + LogicalPort on every dnat_and_snat + // rule so OVN processes DNAT on the VM's own chassis without hairpinning + // through the gateway chassis. Required by UplinkModePhysical. + NATModeDistributed + + // NATModeCentralized leaves ExternalMAC + LogicalPort unset so the + // gateway chassis owns SNAT/DNAT. Required by UplinkModeVeth because the + // Linux bridge intermediary breaks distributed-NAT hairpin routing. + NATModeCentralized +) + +// String returns the canonical name used in logs. +func (m NATMode) String() string { + switch m { + case NATModeDistributed: + return "distributed" + case NATModeCentralized: + return "centralized" + default: + return "unknown" + } +} + +// NATModeFromUplinkMode resolves the L0 uplink mode into the L3 NAT mode. +// The mapping is fixed: physical uplinks support per-chassis NAT, veth +// uplinks do not. UplinkModeUnknown returns NATModeUnknown so misconfiguration +// surfaces loudly at NATManager construction. +func NATModeFromUplinkMode(m host.UplinkMode) NATMode { + switch m { + case host.UplinkModePhysical: + return NATModeDistributed + case host.UplinkModeVeth: + return NATModeCentralized + default: + return NATModeUnknown + } +} diff --git a/spinifex/network/policy/route.go b/spinifex/network/policy/route.go new file mode 100644 index 000000000..d6c401156 --- /dev/null +++ b/spinifex/network/policy/route.go @@ -0,0 +1,138 @@ +package policy + +import ( + "context" + "fmt" + "net/netip" + + "github.com/mulgadc/spinifex/spinifex/network/ovn" + "github.com/mulgadc/spinifex/spinifex/network/topology" + "github.com/mulgadc/spinifex/spinifex/services/vpcd/nbdb" +) + +// defaultRoutePrefix is the IPv4 default-route prefix used by AddDefaultRoute +// / DeleteDefaultRoute. Held as a constant so the magic string isn't +// scattered across the package. +const defaultRoutePrefix = "0.0.0.0/0" + +// RouteSpec is a single static route on a VPC's LogicalRouter. OutputPort is +// only required when Nexthop is not on a directly-connected subnet — e.g. the +// IGW default route uses 169.254.0.1/30 on the gateway LRP but the WAN +// nexthop sits on the uplink subnet, so OVN northd silently drops the route +// from southbound unless OutputPort pins it to the gateway LRP. The IGW +// attach path always sets it. +type RouteSpec struct { + Prefix netip.Prefix + Nexthop string + OutputPort string +} + +// RouteManager owns static routes on VPC LogicalRouters. The router itself +// is L2's responsibility; RouteManager only adds/removes route rows. +// +// Add operations are idempotent — a re-add with identical Nexthop+OutputPort +// is a no-op, a re-add with drifted fields deletes-then-adds. The historical +// vpcd path was non-idempotent (every retry left a duplicate row); the new +// API closes that bug at the contract level so the reconciler can replay +// freely. +type RouteManager interface { + AddDefaultRoute(ctx context.Context, vpcID, nexthop, outputPort string) error + DeleteDefaultRoute(ctx context.Context, vpcID string) error + AddStaticRoute(ctx context.Context, vpcID string, route RouteSpec) error + DeleteStaticRoute(ctx context.Context, vpcID string, prefix netip.Prefix) error +} + +type routeManager struct { + ovn ovn.Client +} + +var _ RouteManager = (*routeManager)(nil) + +// NewRouteManager constructs a RouteManager backed by the given OVN client. +func NewRouteManager(client ovn.Client) RouteManager { + return &routeManager{ovn: client} +} + +func (m *routeManager) AddDefaultRoute(ctx context.Context, vpcID, nexthop, outputPort string) error { + prefix, err := netip.ParsePrefix(defaultRoutePrefix) + if err != nil { + return fmt.Errorf("parse default route prefix: %w", err) + } + return m.AddStaticRoute(ctx, vpcID, RouteSpec{ + Prefix: prefix, + Nexthop: nexthop, + OutputPort: outputPort, + }) +} + +func (m *routeManager) DeleteDefaultRoute(ctx context.Context, vpcID string) error { + return m.deleteByPrefixString(ctx, vpcID, defaultRoutePrefix) +} + +func (m *routeManager) AddStaticRoute(ctx context.Context, vpcID string, route RouteSpec) error { + router := topology.VPCRouter(vpcID) + prefixStr := route.Prefix.String() + + existing, err := m.ovn.FindStaticRoute(ctx, router, prefixStr) + if err != nil { + return fmt.Errorf("find static route %s on %s: %w", prefixStr, router, err) + } + if existing != nil { + if routeMatches(existing, route) { + return nil + } + if err := m.ovn.DeleteStaticRoute(ctx, router, prefixStr); err != nil { + return fmt.Errorf("delete drifted static route %s on %s: %w", prefixStr, router, err) + } + } + + row := &nbdb.LogicalRouterStaticRoute{ + IPPrefix: prefixStr, + Nexthop: route.Nexthop, + } + if route.OutputPort != "" { + op := route.OutputPort + row.OutputPort = &op + } + if err := m.ovn.AddStaticRoute(ctx, router, row); err != nil { + return fmt.Errorf("add static route %s -> %s on %s: %w", prefixStr, route.Nexthop, router, err) + } + return nil +} + +func (m *routeManager) DeleteStaticRoute(ctx context.Context, vpcID string, prefix netip.Prefix) error { + return m.deleteByPrefixString(ctx, vpcID, prefix.String()) +} + +// deleteByPrefixString centralises the "delete and treat not-found as +// success" idempotency required by every Delete* entrypoint. Returns nil +// when the route is already absent — the only stable contract for callers +// driven by fire-and-forget events (IGW detach, NAT GW delete). +func (m *routeManager) deleteByPrefixString(ctx context.Context, vpcID, prefix string) error { + router := topology.VPCRouter(vpcID) + existing, err := m.ovn.FindStaticRoute(ctx, router, prefix) + if err != nil { + return fmt.Errorf("find static route %s on %s: %w", prefix, router, err) + } + if existing == nil { + return nil + } + if err := m.ovn.DeleteStaticRoute(ctx, router, prefix); err != nil { + return fmt.Errorf("delete static route %s on %s: %w", prefix, router, err) + } + return nil +} + +func routeMatches(existing *nbdb.LogicalRouterStaticRoute, want RouteSpec) bool { + if existing.Nexthop != want.Nexthop { + return false + } + switch { + case existing.OutputPort == nil && want.OutputPort == "": + return true + case existing.OutputPort == nil || want.OutputPort == "": + return false + default: + return *existing.OutputPort == want.OutputPort + } +} diff --git a/spinifex/network/policy/route_test.go b/spinifex/network/policy/route_test.go new file mode 100644 index 000000000..5e830e5c4 --- /dev/null +++ b/spinifex/network/policy/route_test.go @@ -0,0 +1,104 @@ +package policy + +import ( + "context" + "net/netip" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/mulgadc/spinifex/spinifex/network/ovn/mock" + "github.com/mulgadc/spinifex/spinifex/network/topology" + "github.com/mulgadc/spinifex/spinifex/services/vpcd/nbdb" +) + +func findRoute(m *mock.Client, prefix string) *nbdb.LogicalRouterStaticRoute { + for _, r := range m.StaticRoutes { + if r.IPPrefix == prefix { + return r + } + } + return nil +} + +func countRoutes(m *mock.Client, prefix string) int { + n := 0 + for _, r := range m.StaticRoutes { + if r.IPPrefix == prefix { + n++ + } + } + return n +} + +func TestRouteManager_AddDefaultRoute_WritesOutputPort(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + require.NoError(t, rm.AddDefaultRoute(ctx, "vpc-1", "192.168.1.1", topology.GatewayRouterPort("vpc-1"))) + + got := findRoute(m, "0.0.0.0/0") + require.NotNil(t, got) + assert.Equal(t, "192.168.1.1", got.Nexthop) + require.NotNil(t, got.OutputPort) + assert.Equal(t, "gw-vpc-1", *got.OutputPort) +} + +func TestRouteManager_AddStaticRoute_IsIdempotent(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + spec := RouteSpec{ + Prefix: netip.MustParsePrefix("10.42.0.0/24"), + Nexthop: "10.0.0.254", + } + require.NoError(t, rm.AddStaticRoute(ctx, "vpc-1", spec)) + require.NoError(t, rm.AddStaticRoute(ctx, "vpc-1", spec)) + require.NoError(t, rm.AddStaticRoute(ctx, "vpc-1", spec)) + + assert.Equal(t, 1, countRoutes(m, "10.42.0.0/24")) +} + +func TestRouteManager_AddStaticRoute_DriftReplacesRow(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + require.NoError(t, rm.AddDefaultRoute(ctx, "vpc-1", "192.168.1.1", "gw-vpc-1")) + // Drift the nexthop. + require.NoError(t, rm.AddDefaultRoute(ctx, "vpc-1", "192.168.1.254", "gw-vpc-1")) + + assert.Equal(t, 1, countRoutes(m, "0.0.0.0/0")) + got := findRoute(m, "0.0.0.0/0") + require.NotNil(t, got) + assert.Equal(t, "192.168.1.254", got.Nexthop) +} + +func TestRouteManager_DeleteDefaultRoute_IdempotentOnMissing(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + require.NoError(t, rm.DeleteDefaultRoute(ctx, "vpc-1")) +} + +func TestRouteManager_DeleteStaticRoute_RemovesRow(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + prefix := netip.MustParsePrefix("10.42.0.0/24") + require.NoError(t, rm.AddStaticRoute(ctx, "vpc-1", RouteSpec{Prefix: prefix, Nexthop: "10.0.0.254"})) + require.NotNil(t, findRoute(m, "10.42.0.0/24")) + + require.NoError(t, rm.DeleteStaticRoute(ctx, "vpc-1", prefix)) + assert.Nil(t, findRoute(m, "10.42.0.0/24")) +} diff --git a/spinifex/network/policy/sg.go b/spinifex/network/policy/sg.go new file mode 100644 index 000000000..c6908af94 --- /dev/null +++ b/spinifex/network/policy/sg.go @@ -0,0 +1,91 @@ +package policy + +import ( + "context" + "fmt" + + "github.com/mulgadc/spinifex/spinifex/network/ovn" + "github.com/mulgadc/spinifex/spinifex/network/topology" +) + +// SGSpec is the policy-layer view of a security group: identifier plus +// ingress/egress rule sets. The OVN port group named by +// topology.SecurityGroupPortGroup(GroupID) must already exist (L2 owns +// its lifecycle); SecurityGroupManager only attaches and clears ACLs. +type SGSpec struct { + GroupID string + VPCID string + IngressRules []Rule + EgressRules []Rule +} + +// SecurityGroupManager attaches ACL policy to a security group's OVN port +// group. EnsureSG and UpdateSG share semantics — both atomically clear and +// re-add the full ACL set (infrastructure + tenant). The replace-all model +// avoids per-rule diff complexity: OVN treats the AddACLs transaction as +// atomic, so callers never observe a partial rule set on the wire. +// +// Port group lifecycle (create + delete) is owned by topology.Manager. If +// the port group is absent, EnsureSG / UpdateSG / DeleteSG return the +// underlying L1 "port group not found" error. +type SecurityGroupManager interface { + // EnsureSG sets the full ACL set on the SG's port group. Idempotent: + // repeated calls with the same spec converge to the same OVN state. + EnsureSG(ctx context.Context, sg SGSpec) error + + // UpdateSG replaces the SG's ACL set. Identical to EnsureSG; kept as a + // distinct method to mirror the parent plan's interface (§8.1) and + // document caller intent. + UpdateSG(ctx context.Context, sg SGSpec) error + + // DeleteSG clears every ACL on the SG's port group. The port group + // itself is not deleted — topology.Manager owns that. + DeleteSG(ctx context.Context, groupID string) error +} + +type sgManager struct { + ovn ovn.Client +} + +var _ SecurityGroupManager = (*sgManager)(nil) + +// NewSecurityGroupManager constructs a SecurityGroupManager backed by the +// given OVN client. +func NewSecurityGroupManager(client ovn.Client) SecurityGroupManager { + return &sgManager{ovn: client} +} + +func (m *sgManager) EnsureSG(ctx context.Context, sg SGSpec) error { + return m.applyACLs(ctx, sg) +} + +func (m *sgManager) UpdateSG(ctx context.Context, sg SGSpec) error { + return m.applyACLs(ctx, sg) +} + +func (m *sgManager) DeleteSG(ctx context.Context, groupID string) error { + pg := topology.SecurityGroupPortGroup(groupID) + if err := m.ovn.ClearACLs(ctx, pg); err != nil { + return fmt.Errorf("clear ACLs on %s: %w", pg, err) + } + return nil +} + +// applyACLs clears the port group's current ACL set and re-adds the +// infrastructure + tenant ACLs. ClearACLs followed by AddACLs is not +// transactional at the OVSDB level, but every chassis re-derives flows +// from the new ACL set in one ovn-northd compile cycle — the observable +// gap is bounded by southbound flow-install latency, the same window that +// applies to any ACL change. Callers requiring stronger atomicity must +// gate updates via leader election. +func (m *sgManager) applyACLs(ctx context.Context, sg SGSpec) error { + pg := topology.SecurityGroupPortGroup(sg.GroupID) + if err := m.ovn.ClearACLs(ctx, pg); err != nil { + return fmt.Errorf("clear ACLs on %s: %w", pg, err) + } + specs := append(InfrastructureACLs(pg), RuleACLSpecs(pg, sg.IngressRules, sg.EgressRules)...) + if err := m.ovn.AddACLs(ctx, pg, specs); err != nil { + return fmt.Errorf("add ACLs on %s: %w", pg, err) + } + return nil +} diff --git a/spinifex/network/policy/sg_test.go b/spinifex/network/policy/sg_test.go new file mode 100644 index 000000000..0f3595f65 --- /dev/null +++ b/spinifex/network/policy/sg_test.go @@ -0,0 +1,111 @@ +package policy + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/mulgadc/spinifex/spinifex/network/ovn/mock" + "github.com/mulgadc/spinifex/spinifex/network/topology" +) + +func newSGMockWithPG(t *testing.T, sgID string) (*mock.Client, string) { + t.Helper() + m := mock.New() + pg := topology.SecurityGroupPortGroup(sgID) + require.NoError(t, m.CreatePortGroup(context.Background(), pg, nil)) + return m, pg +} + +func TestSGManager_EnsureSG_AppliesInfraPlusTenantACLs(t *testing.T) { + ctx := context.Background() + m, pg := newSGMockWithPG(t, "sg-web") + sg := NewSecurityGroupManager(m) + + spec := SGSpec{ + GroupID: "sg-web", + VPCID: "vpc-1", + IngressRules: []Rule{ + {IPProtocol: "tcp", FromPort: 80, ToPort: 80, CIDR: "0.0.0.0/0"}, + {IPProtocol: "tcp", FromPort: 443, ToPort: 443, CIDR: "0.0.0.0/0"}, + }, + EgressRules: []Rule{ + {IPProtocol: "-1", CIDR: "0.0.0.0/0"}, + }, + } + require.NoError(t, sg.EnsureSG(ctx, spec)) + + storedPG, ok := m.PortGroups[pg] + require.True(t, ok) + // 4 infra + 2 ingress + 1 egress = 7 ACLs. + assert.Len(t, storedPG.ACLs, 7) +} + +func TestSGManager_UpdateSG_ReplacesACLSet(t *testing.T) { + ctx := context.Background() + m, pg := newSGMockWithPG(t, "sg-web") + sg := NewSecurityGroupManager(m) + + require.NoError(t, sg.EnsureSG(ctx, SGSpec{ + GroupID: "sg-web", + IngressRules: []Rule{{IPProtocol: "tcp", FromPort: 22, ToPort: 22, CIDR: "10.0.0.0/8"}}, + })) + firstCount := len(m.PortGroups[pg].ACLs) + assert.Equal(t, 5, firstCount) // 4 infra + 1 tenant + + // Replace with a different set — wider ingress, no egress. + require.NoError(t, sg.UpdateSG(ctx, SGSpec{ + GroupID: "sg-web", + IngressRules: []Rule{ + {IPProtocol: "tcp", FromPort: 80, ToPort: 80, CIDR: "0.0.0.0/0"}, + {IPProtocol: "tcp", FromPort: 443, ToPort: 443, CIDR: "0.0.0.0/0"}, + {IPProtocol: "icmp", CIDR: "0.0.0.0/0"}, + }, + })) + assert.Len(t, m.PortGroups[pg].ACLs, 7) // 4 infra + 3 tenant +} + +func TestSGManager_DeleteSG_ClearsACLsKeepsPortGroup(t *testing.T) { + ctx := context.Background() + m, pg := newSGMockWithPG(t, "sg-web") + sg := NewSecurityGroupManager(m) + + require.NoError(t, sg.EnsureSG(ctx, SGSpec{ + GroupID: "sg-web", + IngressRules: []Rule{{IPProtocol: "-1", CIDR: "0.0.0.0/0"}}, + })) + assert.NotEmpty(t, m.PortGroups[pg].ACLs) + + require.NoError(t, sg.DeleteSG(ctx, "sg-web")) + assert.Empty(t, m.PortGroups[pg].ACLs) + // Port group still present — L2 owns lifecycle. + _, exists := m.PortGroups[pg] + assert.True(t, exists) +} + +func TestSGManager_EnsureSG_MissingPortGroup_ReturnsError(t *testing.T) { + ctx := context.Background() + m := mock.New() + sg := NewSecurityGroupManager(m) + + err := sg.EnsureSG(ctx, SGSpec{GroupID: "sg-missing"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "port group") +} + +func TestSGManager_EnsureSG_Idempotent(t *testing.T) { + ctx := context.Background() + m, pg := newSGMockWithPG(t, "sg-web") + sg := NewSecurityGroupManager(m) + + spec := SGSpec{ + GroupID: "sg-web", + IngressRules: []Rule{{IPProtocol: "tcp", FromPort: 22, ToPort: 22, CIDR: "10.0.0.0/8"}}, + } + require.NoError(t, sg.EnsureSG(ctx, spec)) + first := len(m.PortGroups[pg].ACLs) + require.NoError(t, sg.EnsureSG(ctx, spec)) + assert.Equal(t, first, len(m.PortGroups[pg].ACLs)) +}