diff --git a/cmd/spinifex/cmd/admin.go b/cmd/spinifex/cmd/admin.go index d50b068d3..4009ebb55 100644 --- a/cmd/spinifex/cmd/admin.go +++ b/cmd/spinifex/cmd/admin.go @@ -34,6 +34,7 @@ import ( "github.com/mulgadc/spinifex/spinifex/gpu" handlers_ec2_vpc "github.com/mulgadc/spinifex/spinifex/handlers/ec2/vpc" handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" + "github.com/mulgadc/spinifex/spinifex/network/host" "github.com/mulgadc/spinifex/spinifex/objectstore" "github.com/mulgadc/spinifex/spinifex/utils" "github.com/mulgadc/viperblock/viperblock" @@ -325,7 +326,7 @@ func init() { adminInitCmd.Flags().StringSlice("services", nil, "Services this node runs (default: all). Valid: nats,predastore,viperblock,daemon,awsgw,ui") // External networking flags - adminInitCmd.Flags().String("external-mode", "", "External network mode: 'pool' (default when WAN detected) or '' (disabled)") + adminInitCmd.Flags().String("external-mode", "", "External network mode: 'pool' (default when WAN detected), 'nat' (routed; non-bridgeable uplinks; add --external-pool or --external-source=dhcp for public IPs), or '' (disabled)") adminInitCmd.Flags().String("external-iface", "", "WAN NIC for br-external (auto-detected from default route)") adminInitCmd.Flags().String("external-source", "", "Pool IP source: 'dhcp' (default when no --external-pool) or 'static' (uses --external-pool range)") adminInitCmd.Flags().String("external-bind-bridge", "", "Linux bridge for upstream DHCP DORA (default 'br-wan' when --external-source=dhcp)") @@ -1090,6 +1091,13 @@ func runAdminInit(cmd *cobra.Command, args []string) { // --external-pool is omitted the validator below will error with // a SuggestPoolRange hint. if externalMode == "" && !cmd.Flags().Changed("external-mode") { + if isNonBridgeableUplink(detected.WAN.Name) { + fmt.Fprintf(os.Stderr, "\n❌ Detected WAN interface %s cannot be bridged (WiFi/cellular/PPP).\n", detected.WAN.Name) + fmt.Fprintf(os.Stderr, " Use routed NAT mode instead (outbound-only VM networking):\n") + fmt.Fprintf(os.Stderr, " ./scripts/setup-ovn.sh --management --nat-uplink\n") + fmt.Fprintf(os.Stderr, " spx admin init --external-mode=nat\n") + os.Exit(1) + } externalMode = "pool" } } @@ -1097,60 +1105,60 @@ func runAdminInit(cmd *cobra.Command, args []string) { } // Validate external networking flags - if externalMode != "" && externalMode != "pool" { - fmt.Fprintf(os.Stderr, "❌ Error: --external-mode must be 'pool' or empty, got: %s\n", externalMode) + if externalMode != "" && externalMode != "pool" && externalMode != "nat" { + fmt.Fprintf(os.Stderr, "❌ Error: --external-mode must be 'pool', 'nat', or empty, got: %s\n", externalMode) os.Exit(1) } - if externalMode == "pool" { - // Default source: dhcp when no --external-pool, else static. - if externalSource == "" { - if externalPool == "" { - externalSource = "dhcp" - } else { - externalSource = "static" - } - } - switch externalSource { - case "dhcp": - if externalPool != "" { - fmt.Fprintf(os.Stderr, "❌ Error: --external-pool not allowed with --external-source=dhcp (addresses come from upstream DHCP server)\n") - os.Exit(1) - } - if externalBindBridge == "" { - externalBindBridge = "br-wan" - } - case "static": - if externalBindBridge != "" { - fmt.Fprintf(os.Stderr, "❌ Error: --external-bind-bridge only valid with --external-source=dhcp\n") - os.Exit(1) - } - if externalPool == "" { - fmt.Fprintf(os.Stderr, "❌ Error: --external-pool is required with --external-source=static (e.g., 192.168.1.150-192.168.1.250)\n") - if detectedNet != nil && detectedNet.WAN != nil { - sugStart, sugEnd := admin.SuggestPoolRange(detectedNet.WAN) - fmt.Fprintf(os.Stderr, " Suggested: --external-pool=%s-%s\n", sugStart, sugEnd) - } - os.Exit(1) - } - if externalGateway == "" { - fmt.Fprintf(os.Stderr, "❌ Error: --external-gateway is required with --external-source=static\n") - os.Exit(1) - } - parts := strings.SplitN(externalPool, "-", 2) - if len(parts) != 2 || net.ParseIP(parts[0]) == nil || net.ParseIP(parts[1]) == nil { - fmt.Fprintf(os.Stderr, "❌ Error: --external-pool must be start-end IPs (e.g., 192.168.1.150-192.168.1.250), got: %s\n", externalPool) - os.Exit(1) - } - poolStart, poolEnd = parts[0], parts[1] - default: - fmt.Fprintf(os.Stderr, "❌ Error: --external-source must be 'static' or 'dhcp', got: %s\n", externalSource) + // A public pool alongside nat's transit pool restores EIP / public-subnet + // parity where the operator has spare LAN IPs; without these flags nat + // stays Tier-1-only (host-jumpbox access, no public IPs). + natPublicPool := externalMode == "nat" && (externalPool != "" || externalSource != "") + // natPublicGateway keeps the upstream gateway for the public pool before + // the transit segment claims externalGateway below. + natPublicGateway := externalGateway + if externalMode == "nat" { + if nodes >= 2 { + fmt.Fprintf(os.Stderr, "❌ Error: --external-mode=nat is single-node only (v1); use --nodes=1\n") os.Exit(1) } + if !natPublicPool && (externalBindBridge != "" || gatewayIP != "") { + fmt.Fprintf(os.Stderr, "❌ Error: --external-bind-bridge/--gateway-ip require --external-pool or --external-source in --external-mode=nat\n") + os.Exit(1) + } + if natPublicPool { + // DHCP DORA in nat mode binds the uplink interface itself — there + // is no br-wan (nothing is bridged in routed mode). + src, start, end, bb, err := resolvePublicPoolFlags(externalSource, externalPool, externalBindBridge, natPublicGateway, externalIface) + if err != nil { + fmt.Fprintf(os.Stderr, "❌ Error: %v\n", err) + os.Exit(1) + } + externalSource, poolStart, poolEnd, externalBindBridge = src, start, end, bb + } + // The transit segment is fixed: the host veth owns the gateway IP and + // masquerades the /24. + externalGateway = host.NATTransitGatewayIP + } + if externalMode == "pool" { + src, start, end, bb, err := resolvePublicPoolFlags(externalSource, externalPool, externalBindBridge, externalGateway, "br-wan") + if err != nil { + fmt.Fprintf(os.Stderr, "❌ Error: %v\n", err) + if strings.Contains(err.Error(), "--external-pool is required") && detectedNet != nil && detectedNet.WAN != nil { + sugStart, sugEnd := admin.SuggestPoolRange(detectedNet.WAN) + fmt.Fprintf(os.Stderr, " Suggested: --external-pool=%s-%s\n", sugStart, sugEnd) + } + os.Exit(1) + } + externalSource, poolStart, poolEnd, externalBindBridge = src, start, end, bb } if externalGateway != "" && net.ParseIP(externalGateway) == nil { fmt.Fprintf(os.Stderr, "❌ Error: --external-gateway is not a valid IP: %s\n", externalGateway) os.Exit(1) } + if natPublicGateway != "" && net.ParseIP(natPublicGateway) == nil { + fmt.Fprintf(os.Stderr, "❌ Error: --external-gateway is not a valid IP: %s\n", natPublicGateway) + os.Exit(1) + } if gatewayIP != "" && net.ParseIP(gatewayIP) == nil { fmt.Fprintf(os.Stderr, "❌ Error: --gateway-ip is not a valid IP: %s\n", gatewayIP) os.Exit(1) @@ -1165,6 +1173,35 @@ func runAdminInit(cmd *cobra.Command, args []string) { } } + // Assemble the external pool blocks rendered into spinifex.toml. + var externalPools []admin.PoolData + switch externalMode { + case "nat": + externalPools = append(externalPools, admin.PoolData{ + Name: host.NATTransitPoolName, Gateway: host.NATTransitGatewayIP, + PrefixLen: 24, DNSServers: dnsServers, + }) + if natPublicPool { + // WiFi/WWAN uplinks drop frames with foreign source MACs, so DHCP + // leases must go out with the interface's own MAC. + dhcpMAC := "" + if externalSource == "dhcp" && isNonBridgeableUplink(externalBindBridge) { + dhcpMAC = "interface" + } + externalPools = append(externalPools, admin.PoolData{ + Name: "wan", Source: externalSource, BindBridge: externalBindBridge, + DHCPMAC: dhcpMAC, Start: poolStart, End: poolEnd, Gateway: natPublicGateway, + GatewayIP: gatewayIP, PrefixLen: externalPrefixLen, DNSServers: dnsServers, + }) + } + case "pool": + externalPools = append(externalPools, admin.PoolData{ + Name: "wan", Source: externalSource, BindBridge: externalBindBridge, + Start: poolStart, End: poolEnd, Gateway: externalGateway, + GatewayIP: gatewayIP, PrefixLen: externalPrefixLen, DNSServers: dnsServers, + }) + } + // Validate IP address format if net.ParseIP(bindIP) == nil { fmt.Fprintf(os.Stderr, "❌ Error: Invalid IP address for --bind: %s\n", bindIP) @@ -1450,17 +1487,10 @@ func runAdminInit(cmd *cobra.Command, args []string) { OVNNBAddr: "tcp:127.0.0.1:6641", OVNSBAddr: "tcp:127.0.0.1:6642", - ExternalMode: externalMode, - ExternalIface: externalIface, - PoolName: "wan", - PoolSource: externalSource, - PoolBindBridge: externalBindBridge, - PoolStart: poolStart, - PoolEnd: poolEnd, - PoolGateway: externalGateway, - PoolGatewayIP: gatewayIP, - PoolPrefixLen: externalPrefixLen, - PoolDNSServers: dnsServers, + ExternalMode: externalMode, + ExternalIface: externalIface, + BridgeMode: bridgeModeFor(externalMode), + Pools: externalPools, OperatorEmail: email, BootstrapAccountId: admin.DefaultAccountID(), @@ -1477,7 +1507,20 @@ func runAdminInit(cmd *cobra.Command, args []string) { } // Print external networking summary - if externalMode != "" { + if externalMode == "nat" { + if natPublicPool { + fmt.Printf("\n📡 External networking: nat (routed) with public pool — EIPs enabled\n") + if externalSource == "static" { + fmt.Printf(" Public pool: %s - %s (source: static)\n", poolStart, poolEnd) + } else { + fmt.Printf(" Public pool: dhcp via %s\n", externalBindBridge) + } + } else { + fmt.Printf("\n📡 External networking: nat (routed, outbound-only — no public IPs/EIPs)\n") + } + fmt.Printf(" Transit: %s via %s (host masquerades out any uplink)\n", host.NATTransitCIDR, host.NATTransitHostEnd) + fmt.Printf(" Host setup: ./scripts/setup-ovn.sh --nat-uplink (run before starting services)\n") + } else if externalMode != "" { fmt.Printf("\n📡 External networking: %s\n", externalMode) fmt.Printf(" WAN interface: %s\n", externalIface) switch externalSource { @@ -2547,15 +2590,19 @@ func finalizeNodeSetup(dataDir, certPath, adminAccessKey, adminSecretKey, region func applyNetworkConfig(settings *admin.ConfigSettings, nc *formation.NetworkConfig) { settings.IPSecEnabled = nc.IPSecEnabled settings.ExternalMode = nc.ExternalMode - settings.PoolName = nc.PoolName - settings.PoolSource = nc.PoolSource - settings.PoolBindBridge = nc.PoolBindBridge - settings.PoolStart = nc.PoolStart - settings.PoolEnd = nc.PoolEnd - settings.PoolGateway = nc.PoolGateway - settings.PoolGatewayIP = nc.PoolGatewayIP - settings.PoolPrefixLen = nc.PoolPrefixLen - settings.PoolDNSServers = nc.PoolDNSServers + if nc.ExternalMode != "" { + settings.Pools = []admin.PoolData{{ + Name: nc.PoolName, + Source: nc.PoolSource, + BindBridge: nc.PoolBindBridge, + Start: nc.PoolStart, + End: nc.PoolEnd, + Gateway: nc.PoolGateway, + GatewayIP: nc.PoolGatewayIP, + PrefixLen: nc.PoolPrefixLen, + DNSServers: nc.PoolDNSServers, + }} + } settings.BootstrapAccountId = nc.BootstrapAccountId settings.BootstrapVpcId = nc.BootstrapVpcId @@ -2713,6 +2760,66 @@ func writeBootstrapFilesWithAdmin(configDir, bootstrapDir string, masterKey []by return handlers_iam.SaveBootstrapData(filepath.Join(bootstrapDir, "bootstrap.json"), bd) } +// isNonBridgeableUplink reports whether the interface name indicates an uplink +// that cannot be enslaved to an L2 bridge: WiFi (wl*), cellular (ww*), PPP. +func isNonBridgeableUplink(name string) bool { + return strings.HasPrefix(name, "wl") || strings.HasPrefix(name, "ww") || strings.HasPrefix(name, "ppp") +} + +// resolvePublicPoolFlags validates the public-pool flag set shared by pool +// mode and nat-with-public-pool. Returns the resolved source, static range +// start/end, and bind bridge. Source defaults to dhcp when no range is given; +// defaultBindBridge fills --external-bind-bridge for dhcp (br-wan in pool +// mode, the uplink interface in nat mode). +func resolvePublicPoolFlags(source, poolRange, bindBridge, gateway, defaultBindBridge string) (resolvedSource, start, end, resolvedBindBridge string, err error) { + if source == "" { + if poolRange == "" { + source = "dhcp" + } else { + source = "static" + } + } + switch source { + case "dhcp": + if poolRange != "" { + return "", "", "", "", fmt.Errorf("--external-pool not allowed with --external-source=dhcp (addresses come from upstream DHCP server)") + } + if bindBridge == "" { + if defaultBindBridge == "" { + return "", "", "", "", fmt.Errorf("--external-bind-bridge is required with --external-source=dhcp (no uplink interface detected)") + } + bindBridge = defaultBindBridge + } + case "static": + if bindBridge != "" { + return "", "", "", "", fmt.Errorf("--external-bind-bridge only valid with --external-source=dhcp") + } + if poolRange == "" { + return "", "", "", "", fmt.Errorf("--external-pool is required with --external-source=static (e.g., 192.168.1.150-192.168.1.250)") + } + if gateway == "" { + return "", "", "", "", fmt.Errorf("--external-gateway is required with --external-source=static") + } + parts := strings.SplitN(poolRange, "-", 2) + if len(parts) != 2 || net.ParseIP(parts[0]) == nil || net.ParseIP(parts[1]) == nil { + return "", "", "", "", fmt.Errorf("--external-pool must be start-end IPs (e.g., 192.168.1.150-192.168.1.250), got: %s", poolRange) + } + start, end = parts[0], parts[1] + default: + return "", "", "", "", fmt.Errorf("--external-source must be 'static' or 'dhcp', got: %s", source) + } + return source, start, end, bindBridge, nil +} + +// bridgeModeFor returns the vpcd bridge_mode admin init persists for the +// external mode: only nat mode is pinned; bridged modes stay auto-detected. +func bridgeModeFor(externalMode string) string { + if externalMode == "nat" { + return "nat" + } + return "" +} + // detectDNSServers auto-detects DNS servers from the host for the specified // interface. Uses resolvectl (systemd-resolved) first, then falls back to // /etc/resolv.conf. Returns up to 3 servers. Falls back to public DNS if none found. diff --git a/cmd/spinifex/cmd/admin_nat_test.go b/cmd/spinifex/cmd/admin_nat_test.go new file mode 100644 index 000000000..d7541731d --- /dev/null +++ b/cmd/spinifex/cmd/admin_nat_test.go @@ -0,0 +1,198 @@ +package cmd + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/mulgadc/spinifex/spinifex/admin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSpinifexTomlTemplate_NATMode(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "spinifex.toml") + settings := admin.ConfigSettings{ + Node: "node1", + Az: "ap-southeast-2a", + Port: "4432", + Region: "ap-southeast-2", + BindIP: "10.11.12.1", + AccessKey: "AKIATEST", + SecretKey: "SECRET", + AccountID: "123456789012", + NatsToken: "token", + ConfigDir: dir, + OVNNBAddr: "tcp:127.0.0.1:6641", + OVNSBAddr: "tcp:127.0.0.1:6642", + + ExternalMode: "nat", + BridgeMode: "nat", + Pools: []admin.PoolData{{ + Name: "nat-transit", Gateway: "100.127.0.1", PrefixLen: 24, + }}, + } + require.NoError(t, admin.GenerateConfigFile(path, spinifexTomlTemplate, settings)) + + data, err := os.ReadFile(path) + require.NoError(t, err) + content := string(data) + assert.Contains(t, content, `external_mode = "nat"`) + assert.Contains(t, content, `bridge_mode = "nat"`) + assert.Contains(t, content, `name = "nat-transit"`) + assert.Contains(t, content, `gateway = "100.127.0.1"`) + assert.NotContains(t, content, "range_start", "nat mode has no public IP range") +} + +func TestSpinifexTomlTemplate_PoolModeOmitsBridgeMode(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "spinifex.toml") + settings := admin.ConfigSettings{ + Node: "node1", + Az: "ap-southeast-2a", + Port: "4432", + Region: "ap-southeast-2", + BindIP: "10.11.12.1", + AccessKey: "AKIATEST", + SecretKey: "SECRET", + AccountID: "123456789012", + NatsToken: "token", + ConfigDir: dir, + OVNNBAddr: "tcp:127.0.0.1:6641", + OVNSBAddr: "tcp:127.0.0.1:6642", + + ExternalMode: "pool", + ExternalIface: "enp0s3", + Pools: []admin.PoolData{{ + Name: "wan", Source: "static", + Start: "192.168.1.150", End: "192.168.1.250", + Gateway: "192.168.1.1", PrefixLen: 24, + }}, + } + require.NoError(t, admin.GenerateConfigFile(path, spinifexTomlTemplate, settings)) + + data, err := os.ReadFile(path) + require.NoError(t, err) + assert.NotContains(t, string(data), "bridge_mode", "bridged modes keep runtime auto-detect") +} + +func TestIsNonBridgeableUplink(t *testing.T) { + for _, name := range []string{"wlan0", "wlp3s0", "wlx001122334455", "wwan0", "wwp0s20f0u6", "ppp0"} { + assert.True(t, isNonBridgeableUplink(name), name) + } + for _, name := range []string{"eth0", "enp0s3", "eno1", "br-wan", "veth-wan-br"} { + assert.False(t, isNonBridgeableUplink(name), name) + } +} + +func TestBridgeModeFor(t *testing.T) { + assert.Equal(t, "nat", bridgeModeFor("nat")) + assert.Equal(t, "", bridgeModeFor("pool")) + assert.Equal(t, "", bridgeModeFor("")) +} + +func TestSpinifexTomlTemplate_NATModeWithPublicPool(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "spinifex.toml") + settings := admin.ConfigSettings{ + Node: "node1", + Az: "ap-southeast-2a", + Port: "4432", + Region: "ap-southeast-2", + BindIP: "10.11.12.1", + AccessKey: "AKIATEST", + SecretKey: "SECRET", + AccountID: "123456789012", + NatsToken: "token", + ConfigDir: dir, + OVNNBAddr: "tcp:127.0.0.1:6641", + OVNSBAddr: "tcp:127.0.0.1:6642", + + ExternalMode: "nat", + BridgeMode: "nat", + Pools: []admin.PoolData{ + {Name: "nat-transit", Gateway: "100.127.0.1", PrefixLen: 24}, + { + Name: "wan", Source: "dhcp", BindBridge: "wlan0", + DHCPMAC: "interface", PrefixLen: 24, + }, + }, + } + require.NoError(t, admin.GenerateConfigFile(path, spinifexTomlTemplate, settings)) + + data, err := os.ReadFile(path) + require.NoError(t, err) + content := string(data) + assert.Contains(t, content, `name = "nat-transit"`) + assert.Contains(t, content, `name = "wan"`) + assert.Contains(t, content, `source = "dhcp"`) + assert.Contains(t, content, `bind_bridge = "wlan0"`) + assert.Contains(t, content, `dhcp_mac = "interface"`) + transitIdx := strings.Index(content, `name = "nat-transit"`) + wanIdx := strings.Index(content, `name = "wan"`) + assert.Less(t, transitIdx, wanIdx, "transit pool must render first (IGW allocator picks it)") +} + +func TestResolvePublicPoolFlags(t *testing.T) { + tests := []struct { + name string + source, poolRange string + bindBridge, gw string + defaultBridge string + wantSource string + wantStart string + wantEnd string + wantBridge string + wantErr string + }{ + { + name: "static pool range", source: "", poolRange: "192.168.1.150-192.168.1.250", gw: "192.168.1.1", + wantSource: "static", wantStart: "192.168.1.150", wantEnd: "192.168.1.250", + }, + { + name: "dhcp defaults bind bridge", source: "dhcp", defaultBridge: "wlan0", + wantSource: "dhcp", wantBridge: "wlan0", + }, + { + name: "dhcp explicit bind bridge", source: "dhcp", bindBridge: "br-wan", defaultBridge: "wlan0", + wantSource: "dhcp", wantBridge: "br-wan", + }, + { + name: "dhcp rejects pool range", source: "dhcp", poolRange: "192.168.1.150-192.168.1.250", + wantErr: "--external-pool not allowed", + }, + { + name: "static requires gateway", poolRange: "192.168.1.150-192.168.1.250", + wantErr: "--external-gateway is required", + }, + { + name: "static rejects bind bridge", poolRange: "192.168.1.150-192.168.1.250", gw: "192.168.1.1", bindBridge: "br-wan", + wantErr: "--external-bind-bridge only valid", + }, + { + name: "static requires range", source: "static", gw: "192.168.1.1", + wantErr: "--external-pool is required", + }, + { + name: "bad range", poolRange: "not-a-range", gw: "192.168.1.1", + wantErr: "start-end IPs", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + src, start, end, bb, err := resolvePublicPoolFlags(tt.source, tt.poolRange, tt.bindBridge, tt.gw, tt.defaultBridge) + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tt.wantSource, src) + assert.Equal(t, tt.wantStart, start) + assert.Equal(t, tt.wantEnd, end) + assert.Equal(t, tt.wantBridge, bb) + }) + } +} diff --git a/cmd/spinifex/cmd/admin_test.go b/cmd/spinifex/cmd/admin_test.go index 48f3ab25c..60f513873 100644 --- a/cmd/spinifex/cmd/admin_test.go +++ b/cmd/spinifex/cmd/admin_test.go @@ -382,20 +382,19 @@ func TestSpinifexTomlTemplate_ExternalPoolDHCPSource(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "spinifex.toml") settings := admin.ConfigSettings{ - Node: "node1", - Az: "ap-southeast-2a", - Port: "4432", - Region: "ap-southeast-2", - BindIP: "10.11.12.1", - ConfigDir: dir, - OVNNBAddr: "tcp:127.0.0.1:6641", - OVNSBAddr: "tcp:127.0.0.1:6642", - ExternalMode: "pool", - ExternalIface: "eth0", - PoolName: "wan", - PoolSource: "dhcp", - PoolBindBridge: "br-wan", - PoolPrefixLen: 24, + Node: "node1", + Az: "ap-southeast-2a", + Port: "4432", + Region: "ap-southeast-2", + BindIP: "10.11.12.1", + ConfigDir: dir, + OVNNBAddr: "tcp:127.0.0.1:6641", + OVNSBAddr: "tcp:127.0.0.1:6642", + ExternalMode: "pool", + ExternalIface: "eth0", + Pools: []admin.PoolData{{ + Name: "wan", Source: "dhcp", BindBridge: "br-wan", PrefixLen: 24, + }}, } require.NoError(t, admin.GenerateConfigFile(path, spinifexTomlTemplate, settings)) @@ -424,12 +423,11 @@ func TestSpinifexTomlTemplate_ExternalPoolStaticSource(t *testing.T) { OVNSBAddr: "tcp:127.0.0.1:6642", ExternalMode: "pool", ExternalIface: "eth0", - PoolName: "wan", - PoolSource: "static", - PoolStart: "192.168.1.150", - PoolEnd: "192.168.1.250", - PoolGateway: "192.168.1.1", - PoolPrefixLen: 24, + Pools: []admin.PoolData{{ + Name: "wan", Source: "static", + Start: "192.168.1.150", End: "192.168.1.250", + Gateway: "192.168.1.1", PrefixLen: 24, + }}, } require.NoError(t, admin.GenerateConfigFile(path, spinifexTomlTemplate, settings)) @@ -454,8 +452,9 @@ func TestApplyNetworkConfig_PropagatesPoolBindBridge(t *testing.T) { PoolPrefixLen: 24, } applyNetworkConfig(settings, nc) - assert.Equal(t, "dhcp", settings.PoolSource) - assert.Equal(t, "br-wan", settings.PoolBindBridge) + require.Len(t, settings.Pools, 1) + assert.Equal(t, "dhcp", settings.Pools[0].Source) + assert.Equal(t, "br-wan", settings.Pools[0].BindBridge) } func renderSingleNodePredastore(t *testing.T, settings admin.ConfigSettings) string { diff --git a/cmd/spinifex/cmd/service.go b/cmd/spinifex/cmd/service.go index a21156dbf..e79a981af 100644 --- a/cmd/spinifex/cmd/service.go +++ b/cmd/spinifex/cmd/service.go @@ -758,6 +758,7 @@ var vpcdStartCmd = &cobra.Command{ Name: p.Name, Source: p.Source, BindBridge: p.BindBridge, + DHCPMAC: p.DHCPMAC, RangeStart: p.RangeStart, RangeEnd: p.RangeEnd, Gateway: p.Gateway, @@ -805,6 +806,7 @@ var vpcdStartCmd = &cobra.Command{ ExternalInterface: nodeConfig.VPCD.ExternalInterface, BridgeMode: nodeConfig.VPCD.BridgeMode, AZ: nodeConfig.AZ, + NATExemptCIDRs: clusterConfig.Network.NATExemptCIDRs, }) if err != nil { fmt.Println("Error starting vpcd service:", err) diff --git a/cmd/spinifex/cmd/templates/spinifex.toml b/cmd/spinifex/cmd/templates/spinifex.toml index 7062e2879..11899d17c 100644 --- a/cmd/spinifex/cmd/templates/spinifex.toml +++ b/cmd/spinifex/cmd/templates/spinifex.toml @@ -16,28 +16,33 @@ internal_suffix = "spinifex.internal" ipsec_enabled = {{.IPSecEnabled}} {{- if .ExternalMode}} external_mode = "{{.ExternalMode}}" +{{- range .Pools}} [[network.external_pools]] -name = "{{.PoolName}}" -{{- if .PoolSource}} -source = "{{.PoolSource}}" +name = "{{.Name}}" +{{- if .Source}} +source = "{{.Source}}" {{- end}} -{{- if .PoolBindBridge}} -bind_bridge = "{{.PoolBindBridge}}" +{{- if .BindBridge}} +bind_bridge = "{{.BindBridge}}" {{- end}} -{{- if .PoolStart}} -range_start = "{{.PoolStart}}" -range_end = "{{.PoolEnd}}" +{{- if .DHCPMAC}} +dhcp_mac = "{{.DHCPMAC}}" {{- end}} -{{- if .PoolGateway}} -gateway = "{{.PoolGateway}}" +{{- if .Start}} +range_start = "{{.Start}}" +range_end = "{{.End}}" {{- end}} -{{- if .PoolGatewayIP}} -gateway_ip = "{{.PoolGatewayIP}}" +{{- if .Gateway}} +gateway = "{{.Gateway}}" +{{- end}} +{{- if .GatewayIP}} +gateway_ip = "{{.GatewayIP}}" +{{- end}} +prefix_len = {{if .PrefixLen}}{{.PrefixLen}}{{else}}24{{end}} +{{- if .DNSServers}} +dns_servers = [{{range $i, $s := .DNSServers}}{{if $i}}, {{end}}"{{$s}}"{{end}}] {{- end}} -prefix_len = {{if .PoolPrefixLen}}{{.PoolPrefixLen}}{{else}}24{{end}} -{{- if .PoolDNSServers}} -dns_servers = [{{range $i, $s := .PoolDNSServers}}{{if $i}}, {{end}}"{{$s}}"{{end}}] {{- end}} {{- end}} {{- if .BootstrapVpcId}} @@ -107,6 +112,9 @@ ovn_sb_addr = "{{.OVNSBAddr}}" {{- if .ExternalIface}} external_interface = "{{.ExternalIface}}" {{- end}} +{{- if .BridgeMode}} +bridge_mode = "{{.BridgeMode}}" +{{- end}} [nodes.{{.Node}}.predastore] host = "{{.BindIP}}:8443" diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index 80c68c54d..25cff2e28 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -54,7 +54,7 @@ Operational commands for inspecting cluster state. These fan out NATS requests t | Command | Flags | Description | |---------|-------|-------------| -| `spx admin init` | `--nodes`, `--node`, `--bind`, `--port`, `--region`, `--az`, `--cluster-name`, `--cluster-bind`, `--cluster-routes`, `--predastore-nodes`, `--services`, `--formation-timeout`, `--token-ttl`, `--force` | Generates root IAM credentials (AKIA-prefixed access key + secret) → creates master.key (AES-256, 32 bytes, 0600) → writes bootstrap.json (consumed on first start) → generates CA + server TLS certificates → generates join token (written to `join-token` file, displayed in join command) → creates NATS config with auth token → writes spinifex.toml, awsgw.toml, predastore.toml → configures AWS CLI `spx` profile → creates directory structure under `~/spinifex/` | +| `spx admin init` | `--nodes`, `--node`, `--bind`, `--port`, `--region`, `--az`, `--cluster-name`, `--cluster-bind`, `--cluster-routes`, `--predastore-nodes`, `--services`, `--formation-timeout`, `--token-ttl`, `--force`, `--external-mode` (`pool` \| `nat` routed NAT for non-bridgeable uplinks, single-node; pair with `setup-ovn.sh --nat-uplink`; add `--external-pool start-end --external-gateway ` or `--external-source=dhcp [--external-bind-bridge ]` for a public pool with full EIP support), `--no-external` | Generates root IAM credentials (AKIA-prefixed access key + secret) → creates master.key (AES-256, 32 bytes, 0600) → writes bootstrap.json (consumed on first start) → generates CA + server TLS certificates → generates join token (written to `join-token` file, displayed in join command) → creates NATS config with auth token → writes spinifex.toml, awsgw.toml, predastore.toml → configures AWS CLI `spx` profile → creates directory structure under `~/spinifex/` | | `spx admin join` | `--host` (required), `--node` (required), `--token` (required), `--bind`, `--port`, `--region`, `--az`, `--cluster-bind`, `--cluster-routes`, `--data-dir`, `--services` | Connects to leader node with join token (Authorization: Bearer header) → retrieves cluster configuration → configures local node to join cluster and participate in distributed operations | ### Version @@ -385,7 +385,7 @@ termination. Standalone attach/detach API is internal-only. ### EC2 — Elastic IP -EIP commands are only registered when an external IPAM pool is configured. +EIP handlers are always registered. Without a public IPAM pool (external mode disabled, or `nat` without a public pool), `describe-addresses` returns an empty list and mutating commands return `UnsupportedOperation`. In `nat` mode a public pool (`--external-pool` or `--external-source=dhcp` at init) enables the full EIP surface with host-delivered ingress. | Command | Implemented Flags | Missing Flags | Status | |---------|-------------------|---------------|--------| diff --git a/docs/compute/vpc-networking/README.md b/docs/compute/vpc-networking/README.md index 49926239e..a644969ea 100644 --- a/docs/compute/vpc-networking/README.md +++ b/docs/compute/vpc-networking/README.md @@ -191,11 +191,12 @@ is identical: Both support the same AWS features: public subnets, Elastic IPs, security groups, DescribeInstances showing public IPs. -## `nat` — Outbound Only (Simple) +## `nat` — Shared SNAT (Simple) -All VMs share a single external IP for outbound SNAT. No public IPs, no Elastic -IPs, no inbound from WAN. All subnets behave as private subnets with internet -access. +All VMs share a single external IP for outbound SNAT. By default there are no +public IPs, no Elastic IPs, and no inbound from WAN — all subnets behave as +private subnets with internet access. On routed-NAT nodes, adding a public pool +restores full public IP parity (see below). The `gateway_ip` is the IP that OVN uses for SNAT. You can set it statically or use `setup-ovn.sh --dhcp` to obtain one from the router. This is the router's @@ -215,6 +216,90 @@ gateway_ip = "192.168.1.100" # Single IP for all VM outbound SNAT prefix_len = 24 ``` +### Host access to instances (jumpbox pattern) + +The spinifex host automatically reaches every instance's **private IP**: IGW +attach installs a host route into OVN (` via +dev spx-nat-host`) and exempts the transit net from SNAT, so replies to +host-initiated connections come back un-NATted. No per-instance setup. + +Security groups still apply and the default SG is closed to the host, same as +AWS — open SSH/ICMP from the transit net first: + +```bash +aws ec2 authorize-security-group-ingress --group-id $SG \ + --protocol tcp --port 22 --cidr 100.127.0.0/24 +``` + +Then use the host as a jumpbox for remote access: + +```bash +ssh -J admin@ ubuntu@ +``` + +Extra networks that must reach instances without SNAT (e.g. a management LAN) +can be added via `[network] nat_exempt_cidrs = ["192.168.50.0/24"]`. + +### Public IPs in NAT mode (public pool) + +A routed-NAT node (`setup-ovn.sh --nat-uplink` + `spx admin init +--external-mode=nat`) can carry a public pool alongside the internal +`nat-transit` pool. With one configured, nat mode behaves like pool mode for +public IPs: `MapPublicIpOnLaunch` on the default subnet, auto-assigned public +IPs, and Elastic IPs all work. Spinifex delivers each public IP at the host — +a `/32` route steers it into OVN and a proxy-ARP neighbor entry answers for it +on the uplink (L3 only, same MAC, so it works on WiFi and other non-bridgeable +uplinks). + +Static range carved out of the router's DHCP scope: + +```bash +spx admin init --external-mode=nat \ + --external-pool 192.168.1.150-192.168.1.250 \ + --external-gateway 192.168.1.1 +``` + +Or lease public IPs from the upstream router's DHCP: + +```bash +spx admin init --external-mode=nat --external-source=dhcp \ + --external-bind-bridge wlan0 +``` + +On WiFi/WWAN uplinks the leases are requested with the interface's own MAC +(`dhcp_mac = "interface"`, written automatically) and distinguished by DHCP +client-id. Some routers key leases by MAC and ignore the client-id — Spinifex +detects this (the router hands the same IP to two client-ids) and fails the +allocation with advice to switch to a static range. + +Resulting config: + +```toml +[network] +external_mode = "nat" +bridge_mode = "nat" + +[[network.external_pools]] +name = "nat-transit" # internal transit net (auto-generated) +gateway = "100.127.0.1" +prefix_len = 24 + +[[network.external_pools]] +name = "wan" # public pool +range_start = "192.168.1.150" +range_end = "192.168.1.250" +gateway = "192.168.1.1" +prefix_len = 24 +``` + +**Caveat — reaching an EIP from the spinifex host itself.** Host-sourced +traffic enters OVN from the transit net, which is exempt from NAT (that is what +makes the jumpbox pattern work) — so a host connection to an EIP that carries +the transit source IP would skip DNAT. Spinifex stamps the EIP route with the +uplink's LAN IP as source to avoid this, but if no uplink address can be +determined, connect to the instance's **private IP** from the host instead. +Other machines on the LAN are unaffected. + ## Disabled (Empty/Omitted) VPC networking is overlay-only. No external connectivity. Instances can only @@ -222,19 +307,21 @@ communicate within their VPC. ## Mode Comparison -| Capability | `pool` (static) | `pool` (dhcp) | `nat` | Disabled | -| --------------------------------- | --------------- | ------------- | -------- | -------- | -| Outbound internet | Yes | Yes | Yes | No | -| Inbound from WAN | Yes (1:1 NAT) | Yes (1:1 NAT) | No | No | -| Public subnets | Yes | Yes | No | No | -| Auto-assign public IPs | Yes | Yes | No | No | -| Elastic IPs | Yes | Yes | No | No | -| DescribeInstances shows public IP | Yes | Yes | No | No | -| Admin must reserve IP range | Yes | No | No | No | -| Needs router DHCP | No | Yes | Optional | No | +| Capability | `pool` (static) | `pool` (dhcp) | `nat` | Disabled | +| --------------------------------- | --------------- | ------------- | ----------------- | -------- | +| Outbound internet | Yes | Yes | Yes | No | +| Host reaches instance private IPs | No | No | Yes (routed) | No | +| Inbound from WAN | Yes (1:1 NAT) | Yes (1:1 NAT) | With public pool | No | +| Public subnets | Yes | Yes | With public pool | No | +| Auto-assign public IPs | Yes | Yes | With public pool | No | +| Elastic IPs | Yes | Yes | With public pool | No | +| DescribeInstances shows public IP | Yes | Yes | With public pool | No | +| Admin must reserve IP range | Yes | No | Only static pool | No | +| Needs router DHCP | No | Yes | Optional | No | -If you start with `nat` and later need public subnets, switch to `pool` and -define a range (or use `source = "dhcp"`) — no data migration needed. +If you start with `nat` and later need public subnets: on a bridgeable uplink +switch to `pool` and define a range (or use `source = "dhcp"`); on a routed-NAT +node just add a public pool alongside `nat-transit` — no data migration needed. ## Bridge Setup — Physical Network Wiring diff --git a/docs/service-interfaces.yaml b/docs/service-interfaces.yaml index 4ee224877..c7131da67 100644 --- a/docs/service-interfaces.yaml +++ b/docs/service-interfaces.yaml @@ -332,6 +332,12 @@ suites: # e2e-bm: # path: spinifex/tests/e2e/baremetal # covers: [awsgw, spinifex_daemon, vpcd, viperblockd, predastore] + # + # e2e-natuplink activates once CI can provision an external_mode=nat + # node (setup-ovn.sh --nat-uplink + spx admin init --external-mode=nat). + # e2e-natuplink: + # path: spinifex/tests/e2e/natuplink + # covers: [awsgw, spinifex_daemon, vpcd, viperblockd, predastore] # Special-case rules consumed by the selector (Bead 4). Hard-coded in diff --git a/scripts/setup-ovn.sh b/scripts/setup-ovn.sh index 3c84eba16..766801efc 100755 --- a/scripts/setup-ovn.sh +++ b/scripts/setup-ovn.sh @@ -18,6 +18,11 @@ # --wan-bridge=NAME OVS bridge for WAN traffic (default: auto-detect from default route) # --wan-iface=NAME Physical NIC to add to the WAN bridge (use with --wan-bridge) # --dhcp Obtain gateway IP via DHCP on the WAN bridge interface +# --nat-uplink Routed NAT mode: no WAN NIC is bridged. Creates br-ext +# with a transit veth (spx-nat-host 100.127.0.1/24) and +# host masquerade rules; VMs get outbound-only WAN over +# any uplink (ethernet, WiFi, cellular, PPP). Pair with +# `spx admin init --external-mode=nat`. # --mgmt-bridge=NAME OVS bridge for system-instance control plane (default: br-mgmt) # --mgmt-cidr=CIDR IPv4 CIDR to assign on the mgmt bridge (default: 10.15.8.1/24) # --mgmt-iface=NAME Physical/virtual NIC to enslave to the mgmt bridge (multi-node only) @@ -62,6 +67,9 @@ # # # No WAN bridge (overlay-only, no public subnet): # ./scripts/setup-ovn.sh --management --encap-ip=10.0.0.1 +# +# # Non-bridgeable uplink (WiFi/cellular) — routed NAT, outbound-only VMs: +# ./scripts/setup-ovn.sh --management --nat-uplink set -e @@ -70,6 +78,7 @@ MANAGEMENT=false WAN_BRIDGE="" WAN_IFACE="" EXTERNAL_DHCP=false +NAT_UPLINK=false MGMT_BRIDGE_ENABLED=true MGMT_BRIDGE="br-mgmt" MGMT_CIDR="10.15.8.1/24" @@ -95,6 +104,7 @@ for arg in "$@"; do case "$arg" in --management) MANAGEMENT=true ;; --dhcp) EXTERNAL_DHCP=true ;; + --nat-uplink) NAT_UPLINK=true ;; --wan-bridge=*) WAN_BRIDGE="${arg#*=}" ;; --wan-iface=*) WAN_IFACE="${arg#*=}" ;; --mgmt-bridge=*) MGMT_BRIDGE="${arg#*=}" ;; @@ -204,12 +214,28 @@ detect_wan_bridge() { echo "" echo " 3. No external networking (overlay-only):" echo " ./scripts/setup-ovn.sh --management --encap-ip=$wan_ip" + echo "" + echo " 4. Non-bridgeable uplink (WiFi/cellular/PPP) — routed NAT mode" + echo " (VMs get outbound-only internet, no public IPs):" + echo " ./scripts/setup-ovn.sh --management --nat-uplink" + echo " then: spx admin init --external-mode=nat" echo "============================================================" echo "" exit 1 } -detect_wan_bridge +if [ "$NAT_UPLINK" = true ]; then + # Routed NAT: nothing is bridged, so the uplink type is irrelevant. + if [ -n "$WAN_BRIDGE" ] || [ -n "$WAN_IFACE" ]; then + echo "ERROR: --nat-uplink does not take --wan-bridge/--wan-iface (no WAN NIC is bridged)" + exit 1 + fi + WAN_BRIDGE="br-ext" + WAN_BRIDGE_MODE="nat" + echo " Routed NAT uplink: br-ext + transit veth, host masquerade (no WAN bridge)" +else + detect_wan_bridge +fi # Auto-detect encap IP if not specified if [ -z "$ENCAP_IP" ]; then @@ -413,6 +439,16 @@ if [ -n "$WAN_BRIDGE" ]; then sudo ip link del veth-wan-br 2>/dev/null || true fi + # Same ripdown for stale routed-NAT plumbing when switching away from nat. + if [ "$WAN_BRIDGE_MODE" != "nat" ]; then + sudo rm -f /etc/systemd/network/17-spinifex-nat.netdev \ + /etc/systemd/network/17-spinifex-nat.network \ + /etc/systemd/network/18-spinifex-nat-ovs.network + sudo networkctl reload 2>/dev/null || true + sudo ovs-vsctl --if-exists del-port spx-nat-ovs 2>/dev/null || true + sudo ip link del spx-nat-host 2>/dev/null || true + fi + case "$WAN_BRIDGE_MODE" in existing) # Already an OVS bridge (from a previous run or explicit --wan-bridge). @@ -560,10 +596,101 @@ NETWORK echo " $WAN_BRIDGE: direct bridge on $WAN_IFACE" echo " NOTE: $WAN_IFACE is now an OVS port — no host IP on this NIC" ;; + + nat) + # Routed NAT: br-ext carries no WAN NIC. A transit veth pair links + # it to the host stack (spx-nat-host owns 100.127.0.1/24); the host + # forwards and masquerades the transit /24 out whatever uplink it + # has. OVN SNATs each VPC CIDR to its gateway LRP transit IP, so + # the masquerade rule below is the only host-side NAT state. + NAT_TRANSIT_CIDR="100.127.0.0/24" + NAT_TRANSIT_GW_CIDR="100.127.0.1/24" + + if ! sudo ovs-vsctl br-exists "$WAN_BRIDGE" 2>/dev/null; then + sudo ovs-vsctl --may-exist add-br "$WAN_BRIDGE" + echo " created OVS bridge: $WAN_BRIDGE" + fi + sudo ip link set "$WAN_BRIDGE" up + + # Create transit veth pair (idempotent) + if ! ip link show spx-nat-host >/dev/null 2>&1; then + sudo ip link add spx-nat-host type veth peer name spx-nat-ovs + echo " created veth pair: spx-nat-host ↔ spx-nat-ovs" + else + echo " veth pair already exists: spx-nat-host ↔ spx-nat-ovs" + fi + sudo ip addr replace "$NAT_TRANSIT_GW_CIDR" dev spx-nat-host + + # Add the OVS end to br-ext + if ! sudo ovs-vsctl port-to-br spx-nat-ovs >/dev/null 2>&1; then + sudo ovs-vsctl --may-exist add-port "$WAN_BRIDGE" spx-nat-ovs + echo " spx-nat-ovs → $WAN_BRIDGE (OVS bridge)" + fi + sudo ip link set spx-nat-host up + sudo ip link set spx-nat-ovs up + echo " host (spx-nat-host $NAT_TRANSIT_GW_CIDR) ↔ veth pair ↔ $WAN_BRIDGE (OVS)" + + # Persist the veth pair + transit IP across reboot (veths are + # kernel-only; same rationale as veth mode above). + NAT_NETDEV="/etc/systemd/network/17-spinifex-nat.netdev" + NAT_NETWORK="/etc/systemd/network/17-spinifex-nat.network" + NAT_OVS_NETWORK="/etc/systemd/network/18-spinifex-nat-ovs.network" + sudo tee "$NAT_NETDEV" >/dev/null </dev/null </dev/null </dev/null || true + echo " wrote $NAT_NETDEV + $NAT_NETWORK + $NAT_OVS_NETWORK (transit veth persists on reboot)" + + # Kernel egress: masquerade the transit /24 out any uplink and + # accept forwarded transit traffic even under FORWARD-policy DROP. + # vpcd re-ensures these on every start; installing here too means + # the wiring is testable before services run. + sudo iptables -t nat -C POSTROUTING -s "$NAT_TRANSIT_CIDR" ! -d "$NAT_TRANSIT_CIDR" \ + -m comment --comment "spinifex-nat-egress" -j MASQUERADE 2>/dev/null || \ + sudo iptables -t nat -A POSTROUTING -s "$NAT_TRANSIT_CIDR" ! -d "$NAT_TRANSIT_CIDR" \ + -m comment --comment "spinifex-nat-egress" -j MASQUERADE + sudo iptables -C FORWARD -i spx-nat-host -s "$NAT_TRANSIT_CIDR" \ + -m comment --comment "spinifex-nat-egress" -j ACCEPT 2>/dev/null || \ + sudo iptables -A FORWARD -i spx-nat-host -s "$NAT_TRANSIT_CIDR" \ + -m comment --comment "spinifex-nat-egress" -j ACCEPT + sudo iptables -C FORWARD -o spx-nat-host -m conntrack --ctstate RELATED,ESTABLISHED \ + -m comment --comment "spinifex-nat-egress" -j ACCEPT 2>/dev/null || \ + sudo iptables -A FORWARD -o spx-nat-host -m conntrack --ctstate RELATED,ESTABLISHED \ + -m comment --comment "spinifex-nat-egress" -j ACCEPT + echo " installed masquerade + forward rules for $NAT_TRANSIT_CIDR (comment: spinifex-nat-egress)" + ;; esac # --- DHCP: obtain gateway IP for OVN SNAT --- - if [ "$EXTERNAL_DHCP" = true ]; then + if [ "$EXTERNAL_DHCP" = true ] && [ "$WAN_BRIDGE_MODE" = "nat" ]; then + echo "" + echo " Skipping --dhcp: routed NAT mode has a fixed transit gateway (100.127.0.1)" + elif [ "$EXTERNAL_DHCP" = true ]; then echo "" echo "Step 3c: Obtaining external gateway IP via DHCP..." diff --git a/scripts/setup.sh b/scripts/setup.sh index ee22dc403..b177c0afc 100755 --- a/scripts/setup.sh +++ b/scripts/setup.sh @@ -198,6 +198,11 @@ spinifex-vpcd ALL=(root) NOPASSWD: /usr/bin/ovs-vsctl, /usr/bin/ovs-appctl spinifex-vpcd ALL=(root) NOPASSWD: /usr/bin/ovn-nbctl, /usr/bin/ovn-sbctl, /usr/bin/ovn-appctl spinifex-vpcd ALL=(root) NOPASSWD: /usr/bin/systemctl is-active --quiet ovn-controller spinifex-vpcd ALL=(root) NOPASSWD: /sbin/ip, /usr/sbin/ip +# Routed-NAT mode: transit masquerade + per-EIP FORWARD accepts, proxy-ARP +# delay tune, and gratuitous ARP announcements for host-delivered EIPs. +spinifex-vpcd ALL=(root) NOPASSWD: /usr/sbin/iptables, /sbin/iptables +spinifex-vpcd ALL=(root) NOPASSWD: /usr/sbin/sysctl -w net.ipv4.neigh.* +spinifex-vpcd ALL=(root) NOPASSWD: /usr/sbin/arping, /usr/bin/arping SUDOERS $SUDO chmod 0440 /etc/sudoers.d/spinifex-network $SUDO visudo -cf /etc/sudoers.d/spinifex-network || fatal "Invalid sudoers syntax in spinifex-network" diff --git a/spinifex/admin/admin.go b/spinifex/admin/admin.go index 1b0ce0d48..7aa5422ff 100644 --- a/spinifex/admin/admin.go +++ b/spinifex/admin/admin.go @@ -73,17 +73,10 @@ type ConfigSettings struct { OVNSBAddr string // External networking for public subnets - ExternalMode string // "pool" or "" (disabled) - ExternalIface string // WAN NIC name (e.g., "eth0", "eth1") - PoolName string // External pool name (e.g., "wan") - PoolSource string // IP source: "static" or "dhcp" - PoolBindBridge string // Linux bridge for upstream DORA (source=dhcp only) - PoolStart string // First IP in external pool range (static only) - PoolEnd string // Last IP in external pool range (static only) - PoolGateway string // WAN gateway IP - PoolGatewayIP string // Explicit SNAT IP (overrides default of first IP in range) - PoolPrefixLen int // Subnet prefix length (default 24) - PoolDNSServers []string // DNS servers for VM DHCP (auto-detected from host) + ExternalMode string // "pool", "nat" (routed), or "" (disabled) + ExternalIface string // WAN NIC name (e.g., "eth0", "eth1") + BridgeMode string // vpcd bridge_mode; only written for "nat" (bridged modes auto-detect) + Pools []PoolData // External pools rendered in order (nat mode: transit first, then optional public pool) // OperatorEmail is the address collected at install time. Written under [operator] // in spinifex.toml so it survives wipes. Empty means no identity was supplied. @@ -116,6 +109,20 @@ type ConfigSettings struct { EncryptionKeyFile string } +// PoolData is one [[network.external_pools]] block rendered into spinifex.toml. +type PoolData struct { + Name string // Pool name (e.g., "wan", "nat-transit") + Source string // IP source: "static" or "dhcp" + BindBridge string // Linux bridge / interface for upstream DORA (source=dhcp only) + Start string // First IP in range (static only) + End string // Last IP in range (static only) + Gateway string // WAN gateway IP + GatewayIP string // Explicit SNAT IP (overrides default of first IP in range) + PrefixLen int // Subnet prefix length (default 24) + DNSServers []string // DNS servers for VM DHCP (auto-detected from host) + DHCPMAC string // DHCP client MAC strategy: "derived" (default) or "interface" +} + // PredastoreNodeConfig describes a single Predastore node for multi-node config generation. type PredastoreNodeConfig struct { ID int diff --git a/spinifex/config/config.go b/spinifex/config/config.go index 43085bdd9..4fc9796c4 100644 --- a/spinifex/config/config.go +++ b/spinifex/config/config.go @@ -40,6 +40,7 @@ type ExternalPool struct { Name string `mapstructure:"name"` // Pool identifier (e.g., "wan", "dc1-primary") Source string `mapstructure:"source"` // IP source: "static" (default) or "dhcp" BindBridge string `mapstructure:"bind_bridge"` // Linux bridge for DHCP DORA (source=dhcp only) + DHCPMAC string `mapstructure:"dhcp_mac"` // DHCP client MAC strategy: "derived" (default) or "interface" (source=dhcp only) RangeStart string `mapstructure:"range_start"` // First IP in range (static source only) RangeEnd string `mapstructure:"range_end"` // Last IP in range (static source only) Gateway string `mapstructure:"gateway"` // WAN default gateway (next hop for 0.0.0.0/0) @@ -60,6 +61,9 @@ type NetworkConfig struct { ExternalPools []ExternalPool `mapstructure:"external_pools"` // One or more IP pools // IPSecEnabled toggles OVN native IPsec (AES-256-GCM) on every node. Default true; disable only for trusted lab topologies. IPSecEnabled bool `mapstructure:"ipsec_enabled"` + // NATExemptCIDRs are extra destinations that skip routed-mode SNAT (added + // to the transit /24 in the spinifex_nat_exempt set). nat mode only. + NATExemptCIDRs []string `mapstructure:"nat_exempt_cidrs"` } // BootstrapConfig holds the default VPC infrastructure IDs written by admin init. @@ -300,6 +304,15 @@ func validateClusterConfig(cc *ClusterConfig) error { } } + if len(cc.Network.NATExemptCIDRs) > 0 && cc.Network.ExternalMode != "nat" { + return fmt.Errorf("config: [network] nat_exempt_cidrs requires external_mode = \"nat\"") + } + for _, c := range cc.Network.NATExemptCIDRs { + if _, err := netip.ParsePrefix(c); err != nil { + return fmt.Errorf("config: [network] nat_exempt_cidrs entry %q: %w", c, err) + } + } + type poolRange struct { name string start netip.Addr @@ -307,11 +320,19 @@ func validateClusterConfig(cc *ClusterConfig) error { } var ranges []poolRange for _, p := range cc.Network.ExternalPools { + switch p.DHCPMAC { + case "", "derived", "interface": + default: + return fmt.Errorf("config: [[network.external_pools]] %q: dhcp_mac=%q unsupported; use \"derived\" or \"interface\"", p.Name, p.DHCPMAC) + } switch p.Source { case "", "static": if p.BindBridge != "" { return fmt.Errorf("config: [[network.external_pools]] %q: bind_bridge is only valid with source=\"dhcp\"", p.Name) } + if p.DHCPMAC != "" { + return fmt.Errorf("config: [[network.external_pools]] %q: dhcp_mac is only valid with source=\"dhcp\"", p.Name) + } case "dhcp": if p.BindBridge == "" { return fmt.Errorf("config: [[network.external_pools]] %q: source=\"dhcp\" requires bind_bridge (Linux bridge for DHCP DORA)", p.Name) diff --git a/spinifex/config/config_test.go b/spinifex/config/config_test.go index fc2f1d558..e5dfe712b 100644 --- a/spinifex/config/config_test.go +++ b/spinifex/config/config_test.go @@ -1,6 +1,7 @@ package config import ( + "fmt" "os" "path/filepath" "testing" @@ -804,3 +805,98 @@ func TestParseEndpoints(t *testing.T) { }) } } + +func TestLoadConfig_NATExemptCIDRs(t *testing.T) { + base := ` +node = "n1" + +[network] +external_mode = %q +nat_exempt_cidrs = [%s] + +[nodes.n1] +region = "us-east-1" + +[nodes.n1.vpcd] +ovn_nb_addr = "tcp:127.0.0.1:6641" +` + write := func(t *testing.T, mode, cidrs string) string { + t.Helper() + resetViper(t) + path := filepath.Join(t.TempDir(), "spinifex.toml") + require.NoError(t, os.WriteFile(path, fmt.Appendf(nil, base, mode, cidrs), 0600)) + return path + } + + t.Run("valid in nat mode", func(t *testing.T) { + cfg, err := LoadConfig(write(t, "nat", `"192.168.1.0/24", "172.16.0.0/12"`)) + require.NoError(t, err) + assert.Equal(t, []string{"192.168.1.0/24", "172.16.0.0/12"}, cfg.Network.NATExemptCIDRs) + }) + + t.Run("rejected outside nat mode", func(t *testing.T) { + _, err := LoadConfig(write(t, "pool", `"192.168.1.0/24"`)) + require.Error(t, err) + assert.Contains(t, err.Error(), "nat_exempt_cidrs") + }) + + t.Run("invalid CIDR rejected", func(t *testing.T) { + _, err := LoadConfig(write(t, "nat", `"not-a-cidr"`)) + require.Error(t, err) + assert.Contains(t, err.Error(), "not-a-cidr") + }) +} + +func TestLoadConfig_DHCPMAC(t *testing.T) { + base := ` +node = "n1" + +[network] +external_mode = "pool" + +[[network.external_pools]] +name = "wan" +source = %q +%s +%s + +[nodes.n1] +region = "us-east-1" + +[nodes.n1.vpcd] +ovn_nb_addr = "tcp:127.0.0.1:6641" +` + write := func(t *testing.T, source, bindBridge, dhcpMAC string) string { + t.Helper() + resetViper(t) + path := filepath.Join(t.TempDir(), "spinifex.toml") + bb := "" + if bindBridge != "" { + bb = fmt.Sprintf("bind_bridge = %q", bindBridge) + } + dm := "" + if dhcpMAC != "" { + dm = fmt.Sprintf("dhcp_mac = %q", dhcpMAC) + } + require.NoError(t, os.WriteFile(path, fmt.Appendf(nil, base, source, bb, dm), 0600)) + return path + } + + t.Run("interface on dhcp pool", func(t *testing.T) { + cfg, err := LoadConfig(write(t, "dhcp", "wlan0", "interface")) + require.NoError(t, err) + assert.Equal(t, "interface", cfg.Network.ExternalPools[0].DHCPMAC) + }) + + t.Run("unknown value rejected", func(t *testing.T) { + _, err := LoadConfig(write(t, "dhcp", "wlan0", "random")) + require.Error(t, err) + assert.Contains(t, err.Error(), "dhcp_mac") + }) + + t.Run("rejected on static pool", func(t *testing.T) { + _, err := LoadConfig(write(t, "static", "", "interface")) + require.Error(t, err) + assert.Contains(t, err.Error(), `dhcp_mac is only valid with source="dhcp"`) + }) +} diff --git a/spinifex/daemon/daemon.go b/spinifex/daemon/daemon.go index f4f97c327..43d8e446b 100644 --- a/spinifex/daemon/daemon.go +++ b/spinifex/daemon/daemon.go @@ -134,7 +134,7 @@ type Daemon struct { placementGroupService *handlers_ec2_placementgroup.PlacementGroupServiceImpl spotInstanceService *handlers_ec2_spotinstance.SpotInstanceServiceImpl vpcService *handlers_ec2_vpc.VPCServiceImpl - eipService *handlers_ec2_eip.EIPServiceImpl + eipService handlers_ec2_eip.EIPService elbv2Service *handlers_elbv2.ELBv2ServiceImpl eksService *handlers_eks.EKSServiceImpl ecsService *handlers_ecs.Service @@ -1043,18 +1043,17 @@ func (d *Daemon) subscribeAll() error { ) } - // EIP operations require external IPAM (pool mode). Only subscribe when available; - // without a subscriber the gateway returns a NATS timeout → clean error to the client. - if d.eipService != nil { - subs = append(subs, - natsSub{"ec2.AllocateAddress", d.handleEC2AllocateAddress, "spinifex-workers"}, - natsSub{"ec2.ReleaseAddress", d.handleEC2ReleaseAddress, "spinifex-workers"}, - natsSub{"ec2.AssociateAddress", d.handleEC2AssociateAddress, "spinifex-workers"}, - natsSub{"ec2.DisassociateAddress", d.handleEC2DisassociateAddress, "spinifex-workers"}, - natsSub{"ec2.DescribeAddresses", d.handleEC2DescribeAddresses, "spinifex-workers"}, - natsSub{"ec2.DescribeAddressesAttribute", d.handleEC2DescribeAddressesAttribute, "spinifex-workers"}, - ) - } + // EIP handlers are always registered: without external IPAM d.eipService is + // the disabled stub, so reads return empty lists and mutations a clean + // UnsupportedOperation instead of a NATS timeout. + subs = append(subs, + natsSub{"ec2.AllocateAddress", d.handleEC2AllocateAddress, "spinifex-workers"}, + natsSub{"ec2.ReleaseAddress", d.handleEC2ReleaseAddress, "spinifex-workers"}, + natsSub{"ec2.AssociateAddress", d.handleEC2AssociateAddress, "spinifex-workers"}, + natsSub{"ec2.DisassociateAddress", d.handleEC2DisassociateAddress, "spinifex-workers"}, + natsSub{"ec2.DescribeAddresses", d.handleEC2DescribeAddresses, "spinifex-workers"}, + natsSub{"ec2.DescribeAddressesAttribute", d.handleEC2DescribeAddressesAttribute, "spinifex-workers"}, + ) for _, s := range subs { var sub *nats.Subscription @@ -1162,6 +1161,34 @@ func (d *Daemon) startLocal() error { return nil } +// publicExternalPools filters out the routed-NAT transit pool: it carries +// gateway plumbing addresses, never allocatable public IPs. +func publicExternalPools(pools []config.ExternalPool) []config.ExternalPool { + var public []config.ExternalPool + for _, p := range pools { + if p.Name == host.NATTransitPoolName { + continue + } + public = append(public, p) + } + return public +} + +// hasPublicIPPools reports whether the cluster can allocate routable public +// IPs: pool mode always, nat mode only with a public pool beside the transit. +func (d *Daemon) hasPublicIPPools() bool { + if d.clusterConfig == nil { + return false + } + switch d.clusterConfig.Network.ExternalMode { + case "pool": + return true + case "nat": + return len(publicExternalPools(d.clusterConfig.Network.ExternalPools)) > 0 + } + return false +} + // assertNoClusterServicesInitialised enforces the DDIL §1e-audit invariant: // no NATS-dependent handle may exist at the end of startLocal. func (d *Daemon) assertNoClusterServicesInitialised() error { @@ -1310,6 +1337,10 @@ func (d *Daemon) startCluster() error { if err != nil { return fmt.Errorf("failed to initialize VPC service: %w", err) } + // Default subnets request a public IP on launch only when the cluster has + // pools that hand out routable public IPs (pool mode always; nat mode only + // when a public pool rides alongside the transit segment). + d.vpcService.SetDefaultPublicIPMapping(d.hasPublicIPPools()) d.routeTableService, err = initServiceWithRetry("RouteTable service", func() (*handlers_ec2_routetable.RouteTableServiceImpl, error) { return handlers_ec2_routetable.NewRouteTableServiceImplWithNATS(d.config, d.natsConn) @@ -1328,19 +1359,22 @@ func (d *Daemon) startCluster() error { return fmt.Errorf("failed to initialize NatGateway service: %w", err) } - // Initialize external IPAM for pool mode (per-VM public IPs). - if d.clusterConfig != nil && d.clusterConfig.Network.ExternalMode == "pool" { + // Initialize external IPAM when public IP pools exist (pool mode, or nat + // mode with a public pool alongside the transit segment). The transit pool + // never enters IPAM — its addresses are gateway-LRP plumbing, not EIPs. + if d.hasPublicIPPools() { js, jsErr := d.natsConn.JetStream() if jsErr != nil { slog.Warn("Failed to get JetStream for external IPAM", "err", jsErr) } else { var pools []external.ExternalPoolConfig anyDHCP := false - for _, p := range d.clusterConfig.Network.ExternalPools { + for _, p := range publicExternalPools(d.clusterConfig.Network.ExternalPools) { pools = append(pools, external.ExternalPoolConfig{ Name: p.Name, Source: p.Source, BindBridge: p.BindBridge, + DHCPMAC: p.DHCPMAC, RangeStart: p.RangeStart, RangeEnd: p.RangeEnd, Gateway: p.Gateway, @@ -1395,6 +1429,13 @@ func (d *Daemon) startCluster() error { } } + // Without external IPAM (nat mode or external disabled) serve EIP requests + // from the disabled stub so the API surface stays registered. + if d.eipService == nil { + d.eipService = handlers_ec2_eip.NewDisabledEIPService() + slog.Info("EIP service disabled — no external IPAM; serving empty/unsupported responses") + } + d.instanceService.SetTerminationDeps(d.volumeService, d.vpcService, d.externalIPAM, d.tagsService) d.instanceService.SetRunInstancesDeps(d.imageService, d.keyService, &daemonENICreator{d: d}, d.externalIPAM) diff --git a/spinifex/daemon/daemon_handlers_tag.go b/spinifex/daemon/daemon_handlers_tag.go index 36aa5cb36..0b7974748 100644 --- a/spinifex/daemon/daemon_handlers_tag.go +++ b/spinifex/daemon/daemon_handlers_tag.go @@ -66,8 +66,8 @@ func (d *Daemon) recordTagMirrors() []recordTagMirror { if d.eigwService != nil { mirrors = append(mirrors, d.eigwService) } - if d.eipService != nil { - mirrors = append(mirrors, d.eipService) + if m, ok := d.eipService.(recordTagMirror); ok { + mirrors = append(mirrors, m) } if d.natGatewayService != nil { mirrors = append(mirrors, d.natGatewayService) diff --git a/spinifex/daemon/daemon_system_instance.go b/spinifex/daemon/daemon_system_instance.go index 69ceb3bc0..04b510a16 100644 --- a/spinifex/daemon/daemon_system_instance.go +++ b/spinifex/daemon/daemon_system_instance.go @@ -575,10 +575,11 @@ func (d *Daemon) TerminateSystemInstance(instanceID string) error { // the authoritative EIP KV, independent of the cached fields on the VM record. // Idempotent and nil-safe; logs on failure. func (d *Daemon) reclaimSystemInstanceEIP(instanceID string) { - if d.eipService == nil { + releaser, ok := d.eipService.(interface{ ReleaseAddressByInstanceID(string) error }) + if !ok { return } - if err := d.eipService.ReleaseAddressByInstanceID(instanceID); err != nil { + if err := releaser.ReleaseAddressByInstanceID(instanceID); err != nil { slog.Warn("reclaimSystemInstanceEIP: backstop release failed", "instanceId", instanceID, "err", err) } } diff --git a/spinifex/daemon/public_pools_test.go b/spinifex/daemon/public_pools_test.go new file mode 100644 index 000000000..abcdd5737 --- /dev/null +++ b/spinifex/daemon/public_pools_test.go @@ -0,0 +1,52 @@ +package daemon + +import ( + "testing" + + "github.com/mulgadc/spinifex/spinifex/config" + "github.com/mulgadc/spinifex/spinifex/network/host" + "github.com/stretchr/testify/assert" +) + +func TestHasPublicIPPools(t *testing.T) { + transit := config.ExternalPool{Name: host.NATTransitPoolName, Gateway: host.NATTransitGatewayIP, PrefixLen: 24} + wanStatic := config.ExternalPool{Name: "wan", RangeStart: "192.168.1.150", RangeEnd: "192.168.1.250", Gateway: "192.168.1.1", PrefixLen: 24} + wanDHCP := config.ExternalPool{Name: "wan", Source: "dhcp", BindBridge: "wlan0"} + + tests := []struct { + name string + mode string + pools []config.ExternalPool + want bool + }{ + {name: "pool mode", mode: "pool", pools: []config.ExternalPool{wanStatic}, want: true}, + {name: "nat transit only", mode: "nat", pools: []config.ExternalPool{transit}, want: false}, + {name: "nat with static wan", mode: "nat", pools: []config.ExternalPool{transit, wanStatic}, want: true}, + {name: "nat with dhcp wan", mode: "nat", pools: []config.ExternalPool{transit, wanDHCP}, want: true}, + {name: "nat no pools", mode: "nat", pools: nil, want: false}, + {name: "external disabled", mode: "", pools: nil, want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + d := &Daemon{clusterConfig: &config.ClusterConfig{}} + d.clusterConfig.Network.ExternalMode = tt.mode + d.clusterConfig.Network.ExternalPools = tt.pools + assert.Equal(t, tt.want, d.hasPublicIPPools()) + }) + } + + t.Run("nil cluster config", func(t *testing.T) { + d := &Daemon{} + assert.False(t, d.hasPublicIPPools()) + }) +} + +func TestPublicExternalPools_FiltersTransit(t *testing.T) { + pools := []config.ExternalPool{ + {Name: host.NATTransitPoolName}, + {Name: "wan", Source: "dhcp", BindBridge: "wlan0"}, + } + public := publicExternalPools(pools) + assert.Len(t, public, 1) + assert.Equal(t, "wan", public[0].Name) +} diff --git a/spinifex/daemon/vm_adapters.go b/spinifex/daemon/vm_adapters.go index 32d6c33a0..99fc397ee 100644 --- a/spinifex/daemon/vm_adapters.go +++ b/spinifex/daemon/vm_adapters.go @@ -465,8 +465,10 @@ func (d *Daemon) onInstanceUpHook() func(*vm.VM) error { // never re-announces the EIP's NAT and the VM loses connectivity. if d.natsConn != nil && instance.ENIId != "" && instance.Instance != nil { publicIP := instance.PublicIP - if publicIP == "" && d.eipService != nil { - if eipIP, ok := d.eipService.AssociatedPublicIPForInstance(context.Background(), instance.AccountID, instance.ID); ok { + if resolver, ok := d.eipService.(interface { + AssociatedPublicIPForInstance(context.Context, string, string) (string, bool) + }); ok && publicIP == "" { + if eipIP, ok := resolver.AssociatedPublicIPForInstance(context.Background(), instance.AccountID, instance.ID); ok { publicIP = eipIP } } diff --git a/spinifex/handlers/ec2/eip/disabled.go b/spinifex/handlers/ec2/eip/disabled.go new file mode 100644 index 000000000..e107a5986 --- /dev/null +++ b/spinifex/handlers/ec2/eip/disabled.go @@ -0,0 +1,42 @@ +package handlers_ec2_eip + +import ( + "context" + "errors" + + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" +) + +// DisabledEIPService serves EIP requests when the cluster has no external +// IPAM (external_mode "nat" or disabled): reads return AWS-faithful empty +// lists, mutations return UnsupportedOperation. +type DisabledEIPService struct{} + +var _ EIPService = (*DisabledEIPService)(nil) + +func NewDisabledEIPService() *DisabledEIPService { return &DisabledEIPService{} } + +func (s *DisabledEIPService) AllocateAddress(_ context.Context, _ *ec2.AllocateAddressInput, _ string) (*ec2.AllocateAddressOutput, error) { + return nil, errors.New(awserrors.ErrorUnsupportedOperation) +} + +func (s *DisabledEIPService) ReleaseAddress(_ context.Context, _ *ec2.ReleaseAddressInput, _ string) (*ec2.ReleaseAddressOutput, error) { + return nil, errors.New(awserrors.ErrorUnsupportedOperation) +} + +func (s *DisabledEIPService) AssociateAddress(_ context.Context, _ *ec2.AssociateAddressInput, _ string) (*ec2.AssociateAddressOutput, error) { + return nil, errors.New(awserrors.ErrorUnsupportedOperation) +} + +func (s *DisabledEIPService) DisassociateAddress(_ context.Context, _ *ec2.DisassociateAddressInput, _ string) (*ec2.DisassociateAddressOutput, error) { + return nil, errors.New(awserrors.ErrorUnsupportedOperation) +} + +func (s *DisabledEIPService) DescribeAddresses(_ context.Context, _ *ec2.DescribeAddressesInput, _ string) (*ec2.DescribeAddressesOutput, error) { + return &ec2.DescribeAddressesOutput{Addresses: []*ec2.Address{}}, nil +} + +func (s *DisabledEIPService) DescribeAddressesAttribute(_ context.Context, _ *ec2.DescribeAddressesAttributeInput, _ string) (*ec2.DescribeAddressesAttributeOutput, error) { + return &ec2.DescribeAddressesAttributeOutput{Addresses: []*ec2.AddressAttribute{}}, nil +} diff --git a/spinifex/handlers/ec2/eip/disabled_test.go b/spinifex/handlers/ec2/eip/disabled_test.go new file mode 100644 index 000000000..a050dbdb7 --- /dev/null +++ b/spinifex/handlers/ec2/eip/disabled_test.go @@ -0,0 +1,38 @@ +package handlers_ec2_eip + +import ( + "context" + "testing" + + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestDisabledEIPService_DescribeReturnsEmpty(t *testing.T) { + s := NewDisabledEIPService() + + out, err := s.DescribeAddresses(context.Background(), &ec2.DescribeAddressesInput{}, "123456789012") + require.NoError(t, err) + require.NotNil(t, out.Addresses) + assert.Empty(t, out.Addresses) + + attrOut, err := s.DescribeAddressesAttribute(context.Background(), &ec2.DescribeAddressesAttributeInput{}, "123456789012") + require.NoError(t, err) + require.NotNil(t, attrOut.Addresses) + assert.Empty(t, attrOut.Addresses) +} + +func TestDisabledEIPService_MutationsUnsupported(t *testing.T) { + s := NewDisabledEIPService() + + _, err := s.AllocateAddress(context.Background(), &ec2.AllocateAddressInput{}, "123456789012") + assert.EqualError(t, err, awserrors.ErrorUnsupportedOperation) + _, err = s.ReleaseAddress(context.Background(), &ec2.ReleaseAddressInput{}, "123456789012") + assert.EqualError(t, err, awserrors.ErrorUnsupportedOperation) + _, err = s.AssociateAddress(context.Background(), &ec2.AssociateAddressInput{}, "123456789012") + assert.EqualError(t, err, awserrors.ErrorUnsupportedOperation) + _, err = s.DisassociateAddress(context.Background(), &ec2.DisassociateAddressInput{}, "123456789012") + assert.EqualError(t, err, awserrors.ErrorUnsupportedOperation) +} diff --git a/spinifex/handlers/ec2/vpc/default_subnet_mapping_test.go b/spinifex/handlers/ec2/vpc/default_subnet_mapping_test.go new file mode 100644 index 000000000..be15ce33b --- /dev/null +++ b/spinifex/handlers/ec2/vpc/default_subnet_mapping_test.go @@ -0,0 +1,36 @@ +package handlers_ec2_vpc + +import ( + "context" + "testing" + + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestEnsureDefaultVPC_PublicIPMappingDisabled(t *testing.T) { + svc := setupTestVPCService(t) + svc.SetDefaultPublicIPMapping(false) + + _, err := svc.EnsureDefaultVPC(testAccountID) + require.NoError(t, err) + + subDesc, err := svc.DescribeSubnets(context.Background(), &ec2.DescribeSubnetsInput{}, testAccountID) + require.NoError(t, err) + require.Len(t, subDesc.Subnets, 1) + assert.False(t, *subDesc.Subnets[0].MapPublicIpOnLaunch, + "non-pool external modes must not seed public-IP mapping") +} + +func TestEnsureDefaultVPC_PublicIPMappingDefaultOn(t *testing.T) { + svc := setupTestVPCService(t) + + _, err := svc.EnsureDefaultVPC(testAccountID) + require.NoError(t, err) + + subDesc, err := svc.DescribeSubnets(context.Background(), &ec2.DescribeSubnetsInput{}, testAccountID) + require.NoError(t, err) + require.Len(t, subDesc.Subnets, 1) + assert.True(t, *subDesc.Subnets[0].MapPublicIpOnLaunch) +} diff --git a/spinifex/handlers/ec2/vpc/service_impl.go b/spinifex/handlers/ec2/vpc/service_impl.go index ef011c616..5c9e88adb 100644 --- a/spinifex/handlers/ec2/vpc/service_impl.go +++ b/spinifex/handlers/ec2/vpc/service_impl.go @@ -85,6 +85,17 @@ type VPCServiceImpl struct { // Optional: injected after construction for public IP cleanup in DeleteNetworkInterface. externalIPAM *ExternalIPAM eipKV nats.KeyValue + + // disableDefaultPublicIP seeds default subnets with MapPublicIpOnLaunch=false + // (non-pool external modes have no public IPs to assign). Zero value keeps + // the AWS-faithful default of true. + disableDefaultPublicIP bool +} + +// SetDefaultPublicIPMapping controls whether default subnets are seeded with +// MapPublicIpOnLaunch. Call with false when the cluster has no external IPAM. +func (s *VPCServiceImpl) SetDefaultPublicIPMapping(enabled bool) { + s.disableDefaultPublicIP = !enabled } // SetExternalIPAM injects external IPAM and EIP KV store references so that @@ -1224,7 +1235,7 @@ func (s *VPCServiceImpl) EnsureDefaultVPC(accountID string, bootstrap ...Bootstr AvailabilityZone: az, State: "available", IsDefault: true, - MapPublicIpOnLaunch: true, // AWS default subnets auto-assign public IPs + MapPublicIpOnLaunch: !s.disableDefaultPublicIP, // AWS default subnets auto-assign public IPs (unless external mode has none) Tags: map[string]string{"Name": "default"}, CreatedAt: time.Now(), } diff --git a/spinifex/network/external/dhcp/client.go b/spinifex/network/external/dhcp/client.go index 46f3b7b9a..0419447ab 100644 --- a/spinifex/network/external/dhcp/client.go +++ b/spinifex/network/external/dhcp/client.go @@ -25,6 +25,10 @@ type AcquireRequest struct { Hostname string // option 12 — host-name VendorClass string // option 60 — vendor-class-identifier HWAddr net.HardwareAddr // chaddr in the DHCP payload + // UseIfaceMAC leases with the bind interface's own MAC instead of a + // derived one — for uplinks that drop foreign source MACs (WiFi/WWAN). + // Option 61 stays the discriminator between leases. + UseIfaceMAC bool } // Lease is a server-bound DHCPv4 lease. Carries enough state for Renew @@ -36,6 +40,7 @@ type Lease struct { Hostname string VendorClass string HWAddr net.HardwareAddr + UseIfaceMAC bool IP net.IP SubnetMask net.IPMask diff --git a/spinifex/network/external/dhcp/fake.go b/spinifex/network/external/dhcp/fake.go index 14e1512cd..866b21aa2 100644 --- a/spinifex/network/external/dhcp/fake.go +++ b/spinifex/network/external/dhcp/fake.go @@ -103,6 +103,7 @@ func (f *Fake) Acquire(_ context.Context, req AcquireRequest) (*Lease, error) { Hostname: req.Hostname, VendorClass: req.VendorClass, HWAddr: append(net.HardwareAddr(nil), req.HWAddr...), + UseIfaceMAC: req.UseIfaceMAC, IP: tmpl.IP, SubnetMask: tmpl.SubnetMask, Routers: tmpl.Routers, diff --git a/spinifex/network/external/dhcp/iface_mac_test.go b/spinifex/network/external/dhcp/iface_mac_test.go new file mode 100644 index 000000000..a215fb431 --- /dev/null +++ b/spinifex/network/external/dhcp/iface_mac_test.go @@ -0,0 +1,167 @@ +package dhcp_test + +import ( + "context" + "net" + "testing" + "time" + + "github.com/mulgadc/spinifex/spinifex/network/external" + "github.com/mulgadc/spinifex/spinifex/network/external/dhcp" + "github.com/mulgadc/spinifex/spinifex/testutil" + "github.com/nats-io/nats.go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newIfaceMACManager(t *testing.T, fake *dhcp.Fake, ifaceIPs func(string) ([]net.IP, error)) (*dhcp.Manager, *dhcp.Store, *nats.Conn) { + t.Helper() + _, nc, js := testutil.StartTestJetStream(t) + store, err := dhcp.NewStore(js, "az1") + require.NoError(t, err) + mgr, err := dhcp.NewManager(dhcp.ManagerConfig{Client: fake, Store: store, IfaceIPs: ifaceIPs}) + require.NoError(t, err) + t.Cleanup(mgr.Stop) + require.NoError(t, mgr.Start(context.Background())) + subs, err := mgr.Subscribe(nc) + require.NoError(t, err) + t.Cleanup(func() { + for _, s := range subs { + _ = s.Unsubscribe() + } + }) + return mgr, store, nc +} + +func noIfaceIPs(string) ([]net.IP, error) { return nil, nil } + +// UseIfaceMAC must survive the NATS wire, reach the Client request, and +// persist through the KV entry so re-acquire after restart repeats it. +func TestAcquireUseIfaceMACThreadedAndPersisted(t *testing.T) { + fake := dhcp.NewFake() + var gotReq dhcp.AcquireRequest + fake.AcquireHook = func(req dhcp.AcquireRequest) (*dhcp.Lease, error) { + gotReq = req + return &dhcp.Lease{ + Bridge: req.Bridge, ClientID: req.ClientID, UseIfaceMAC: req.UseIfaceMAC, + IP: net.IPv4(192, 0, 2, 100), AcquiredAt: time.Now(), LeaseDuration: time.Hour, + }, nil + } + _, store, nc := newIfaceMACManager(t, fake, noIfaceIPs) + + client := dhcp.NewNATSClient(nc, 2*time.Second) + lease, err := client.RequestAcquire(context.Background(), dhcp.AcquireParams{ + Bridge: "wlan0", ClientID: "eipalloc-A", UseIfaceMAC: true, PoolName: "wan", + }) + require.NoError(t, err) + assert.True(t, gotReq.UseIfaceMAC, "flag must reach the DHCP client") + assert.True(t, lease.UseIfaceMAC, "flag must reflect back on the lease") + + got, err := store.Get("eipalloc-A") + require.NoError(t, err) + assert.True(t, got.Lease.UseIfaceMAC, "flag must round-trip through KV") +} + +// Derived-MAC acquires (the default) must not set the flag anywhere. +func TestAcquireDerivedMACDefault(t *testing.T) { + fake := dhcp.NewFake() + _, store, nc := newIfaceMACManager(t, fake, noIfaceIPs) + + client := dhcp.NewNATSClient(nc, 2*time.Second) + lease, err := client.RequestAcquire(context.Background(), dhcp.AcquireParams{ + Bridge: "br-wan", ClientID: "eipalloc-B", PoolName: "wan", + }) + require.NoError(t, err) + assert.False(t, lease.UseIfaceMAC) + got, err := store.Get("eipalloc-B") + require.NoError(t, err) + assert.False(t, got.Lease.UseIfaceMAC) +} + +// A MAC-keyed upstream router ACKs the interface's own IP back to an +// interface-MAC client. That lease is unusable — hard error, lease released. +func TestAcquireIfaceMACCollisionWithOwnIP(t *testing.T) { + fake := dhcp.NewFake() + tmpl := dhcp.DefaultLeaseTemplate() // leases 192.0.2.100 + fake.SetDefaultLease(tmpl) + ownIPs := func(iface string) ([]net.IP, error) { + return []net.IP{net.IPv4(192, 0, 2, 100)}, nil + } + _, store, nc := newIfaceMACManager(t, fake, ownIPs) + + client := dhcp.NewNATSClient(nc, 2*time.Second) + _, err := client.RequestAcquire(context.Background(), dhcp.AcquireParams{ + Bridge: "wlan0", ClientID: "eipalloc-C", UseIfaceMAC: true, PoolName: "wan", + }) + require.Error(t, err) + assert.Contains(t, err.Error(), `use source="static"`) + assert.Equal(t, 1, fake.ReleaseCount(), "colliding lease must be released") + _, err = store.Get("eipalloc-C") + assert.ErrorIs(t, err, nats.ErrKeyNotFound, "colliding lease must not persist") +} + +// Two client-ids ACKing the same IP means the router ignores option 61. +func TestAcquireIfaceMACCollisionWithExistingLease(t *testing.T) { + fake := dhcp.NewFake() // template always leases 192.0.2.100 + _, _, nc := newIfaceMACManager(t, fake, noIfaceIPs) + + client := dhcp.NewNATSClient(nc, 2*time.Second) + _, err := client.RequestAcquire(context.Background(), dhcp.AcquireParams{ + Bridge: "wlan0", ClientID: "eipalloc-D", UseIfaceMAC: true, PoolName: "wan", + }) + require.NoError(t, err, "first lease is fine") + + _, err = client.RequestAcquire(context.Background(), dhcp.AcquireParams{ + Bridge: "wlan0", ClientID: "eipalloc-E", UseIfaceMAC: true, PoolName: "wan", + }) + require.Error(t, err, "second client-id got the same IP — MAC-keyed router") + assert.Contains(t, err.Error(), `use source="static"`) + assert.Contains(t, err.Error(), "eipalloc-D") +} + +// A dhcp_mac="interface" pool must set UseIfaceMAC on every acquire. +func TestDHCPPoolAllocatorInterfaceMACPool(t *testing.T) { + fake := dhcp.NewFake() + var gotReq dhcp.AcquireRequest + fake.AcquireHook = func(req dhcp.AcquireRequest) (*dhcp.Lease, error) { + gotReq = req + return &dhcp.Lease{ + Bridge: req.Bridge, ClientID: req.ClientID, UseIfaceMAC: req.UseIfaceMAC, + IP: net.IPv4(192, 0, 2, 100), AcquiredAt: time.Now(), LeaseDuration: time.Hour, + }, nil + } + pool := external.ExternalPoolConfig{ + Name: "wan", Source: external.SourceDHCP, BindBridge: "wlan0", + DHCPMAC: external.DHCPMACInterface, + } + allocator, _ := newPoolAllocator(t, fake, pool) + + _, err := allocator.Allocate(context.Background(), external.AllocateRequest{ + PoolName: "wan", AllocationID: "eipalloc-H", + }) + require.NoError(t, err) + assert.True(t, gotReq.UseIfaceMAC) +} + +// Distinct IPs per client-id (a sane server) must not trip the detector. +func TestAcquireIfaceMACDistinctIPsOK(t *testing.T) { + fake := dhcp.NewFake() + next := byte(10) + fake.AcquireHook = func(req dhcp.AcquireRequest) (*dhcp.Lease, error) { + ip := net.IPv4(192, 0, 2, next) + next++ + return &dhcp.Lease{ + Bridge: req.Bridge, ClientID: req.ClientID, UseIfaceMAC: req.UseIfaceMAC, + IP: ip, AcquiredAt: time.Now(), LeaseDuration: time.Hour, + }, nil + } + _, _, nc := newIfaceMACManager(t, fake, noIfaceIPs) + + client := dhcp.NewNATSClient(nc, 2*time.Second) + for _, id := range []string{"eipalloc-F", "eipalloc-G"} { + _, err := client.RequestAcquire(context.Background(), dhcp.AcquireParams{ + Bridge: "wlan0", ClientID: id, UseIfaceMAC: true, PoolName: "wan", + }) + require.NoError(t, err, id) + } +} diff --git a/spinifex/network/external/dhcp/kv.go b/spinifex/network/external/dhcp/kv.go index 8f545cb8b..fd21d317b 100644 --- a/spinifex/network/external/dhcp/kv.go +++ b/spinifex/network/external/dhcp/kv.go @@ -180,6 +180,7 @@ type wireEntry struct { DNS []string `json:"dns,omitempty"` ServerID string `json:"server_id,omitempty"` HWAddr string `json:"hwaddr,omitempty"` + UseIfaceMAC bool `json:"use_iface_mac,omitempty"` Bridge string `json:"bridge,omitempty"` Hostname string `json:"hostname,omitempty"` VendorClass string `json:"vendor_class,omitempty"` @@ -204,6 +205,7 @@ func toWireEntry(e Entry) wireEntry { DNS: ipsToStrings(l.DNS), ServerID: ipToString(l.ServerID), HWAddr: hwToString(l.HWAddr), + UseIfaceMAC: l.UseIfaceMAC, Bridge: l.Bridge, Hostname: l.Hostname, VendorClass: l.VendorClass, @@ -223,6 +225,7 @@ func fromWireEntry(w wireEntry) (Entry, error) { Bridge: w.Bridge, Hostname: w.Hostname, VendorClass: w.VendorClass, + UseIfaceMAC: w.UseIfaceMAC, AcquiredAt: w.Acquired, RawOffer: w.RawOffer, RawACK: w.RawACK, diff --git a/spinifex/network/external/dhcp/manager.go b/spinifex/network/external/dhcp/manager.go index 2e39a921c..9e647cac1 100644 --- a/spinifex/network/external/dhcp/manager.go +++ b/spinifex/network/external/dhcp/manager.go @@ -45,6 +45,9 @@ type ManagerConfig struct { Now func() time.Time AcquireSchedule []time.Duration AcquireBudget time.Duration + // IfaceIPs lists the IPs bound to a named interface; tests override. + // Used to detect MAC-keyed upstream routers on interface-MAC pools. + IfaceIPs func(iface string) ([]net.IP, error) } // Manager owns active DHCP leases in vpcd: one renewal goroutine per @@ -56,6 +59,7 @@ type Manager struct { now func() time.Time acquireSchedule []time.Duration acquireBudget time.Duration + ifaceIPs func(iface string) ([]net.IP, error) sf singleflight.Group @@ -97,12 +101,17 @@ func NewManager(cfg ManagerConfig) (*Manager, error) { if budget <= 0 { budget = defaultAcquireBudget } + ifaceIPs := cfg.IfaceIPs + if ifaceIPs == nil { + ifaceIPs = defaultIfaceIPs + } return &Manager{ client: cfg.Client, store: cfg.Store, now: now, acquireSchedule: schedule, acquireBudget: budget, + ifaceIPs: ifaceIPs, loops: map[string]*leaseLoop{}, }, nil } @@ -300,6 +309,7 @@ func (m *Manager) doAcquire(ctx context.Context, e *Entry) error { Hostname: e.Lease.Hostname, VendorClass: e.Lease.VendorClass, HWAddr: e.Lease.HWAddr, + UseIfaceMAC: e.Lease.UseIfaceMAC, }) if err != nil { return err @@ -467,10 +477,19 @@ func (m *Manager) acquireLocked(ctx context.Context, req acquireWireRequest) (*E Hostname: req.Hostname, VendorClass: req.VendorClass, HWAddr: hw, + UseIfaceMAC: req.UseIfaceMAC, }) if err != nil { return nil, err } + if req.UseIfaceMAC { + if cerr := m.checkIfaceMACCollision(req.Bridge, req.ClientID, lease.IP); cerr != nil { + if relErr := m.client.Release(ctx, lease); relErr != nil { + slog.Warn("dhcp manager: release of colliding lease failed", "client_id", req.ClientID, "err", relErr) + } + return nil, cerr + } + } entry := Entry{Purpose: req.Purpose, PoolName: req.PoolName, VPCID: req.VPCID, Lease: lease} if err := m.store.Put(entry); err != nil { return nil, fmt.Errorf("persist new lease: %w", err) @@ -479,6 +498,57 @@ func (m *Manager) acquireLocked(ctx context.Context, req acquireWireRequest) (*E return &entry, nil } +// checkIfaceMACCollision detects upstream routers that key leases on MAC and +// ignore option 61: every interface-MAC client then ACKs the same IP. The +// ACK IP matching the interface's own address or another client-id's live +// lease is a hard error — the operator must switch the pool to a static range. +func (m *Manager) checkIfaceMACCollision(iface, clientID string, ip net.IP) error { + if ip == nil { + return nil + } + const advice = "the upstream router keys leases on MAC and ignores client-id; use source=\"static\" with a range outside its DHCP scope" + if ips, err := m.ifaceIPs(iface); err == nil { + for _, own := range ips { + if own.Equal(ip) { + return fmt.Errorf("dhcp manager: upstream leased %s to client %q but %s already owns that IP — %s", ip, clientID, iface, advice) + } + } + } + entries, err := m.store.List() + if err != nil { + slog.Warn("dhcp manager: lease list for MAC-collision check failed", "err", err) + return nil + } + for _, e := range entries { + if e.Lease == nil || e.Lease.ClientID == clientID || e.Lease.IP == nil { + continue + } + if e.Lease.IP.Equal(ip) && e.Lease.ExpiresAt().After(m.now()) { + return fmt.Errorf("dhcp manager: upstream leased %s to client %q but client %q already holds it — %s", ip, clientID, e.Lease.ClientID, advice) + } + } + return nil +} + +// defaultIfaceIPs lists the addresses bound to a named interface. +func defaultIfaceIPs(name string) ([]net.IP, error) { + iface, err := net.InterfaceByName(name) + if err != nil { + return nil, err + } + addrs, err := iface.Addrs() + if err != nil { + return nil, err + } + var ips []net.IP + for _, a := range addrs { + if ipn, ok := a.(*net.IPNet); ok { + ips = append(ips, ipn.IP) + } + } + return ips, nil +} + // DrainAll DHCPRELEASEs every lease in this vpcd's per-AZ store and deletes // the KV records. Best-effort (per-lease failures logged, not fatal). Used on // destructive teardown; Stop() preserves leases for adopt-on-reboot. diff --git a/spinifex/network/external/dhcp/nats_client.go b/spinifex/network/external/dhcp/nats_client.go index 1ec215f21..d9617e363 100644 --- a/spinifex/network/external/dhcp/nats_client.go +++ b/spinifex/network/external/dhcp/nats_client.go @@ -36,6 +36,7 @@ type AcquireParams struct { Hostname string VendorClass string HWAddr net.HardwareAddr + UseIfaceMAC bool Purpose string PoolName string VPCID string @@ -57,6 +58,7 @@ func (c *NATSClient) RequestAcquire(ctx context.Context, p AcquireParams) (*Leas Hostname: p.Hostname, VendorClass: p.VendorClass, HWAddr: hwToString(p.HWAddr), + UseIfaceMAC: p.UseIfaceMAC, Purpose: p.Purpose, PoolName: p.PoolName, VPCID: p.VPCID, diff --git a/spinifex/network/external/dhcp/nclient4.go b/spinifex/network/external/dhcp/nclient4.go index 8706ec526..4a17d1d0f 100644 --- a/spinifex/network/external/dhcp/nclient4.go +++ b/spinifex/network/external/dhcp/nclient4.go @@ -37,7 +37,19 @@ func (c *NClient4Client) Acquire(ctx context.Context, req AcquireRequest) (*Leas if req.Bridge == "" { return nil, fmt.Errorf("dhcp acquire: bridge is required") } - if len(req.HWAddr) == 0 { + switch { + case req.UseIfaceMAC: + // The uplink drops foreign source MACs (WiFi/WWAN), so lease with the + // interface's own MAC; option 61 keeps leases apart on sane servers. + iface, err := net.InterfaceByName(req.Bridge) + if err != nil { + return nil, fmt.Errorf("dhcp acquire: interface MAC for %s: %w", req.Bridge, err) + } + if len(iface.HardwareAddr) == 0 { + return nil, fmt.Errorf("dhcp acquire: interface %s has no MAC", req.Bridge) + } + req.HWAddr = iface.HardwareAddr + case len(req.HWAddr) == 0: if req.ClientID == "" { return nil, fmt.Errorf("dhcp acquire: client_id or hw_addr is required") } @@ -121,6 +133,7 @@ func (c *NClient4Client) Renew(ctx context.Context, lease *Lease) (*Lease, error Hostname: lease.Hostname, VendorClass: lease.VendorClass, HWAddr: lease.HWAddr, + UseIfaceMAC: lease.UseIfaceMAC, }, renewed), nil } @@ -197,6 +210,7 @@ func leaseFromNClient4(req AcquireRequest, in *nclient4.Lease) *Lease { Hostname: req.Hostname, VendorClass: req.VendorClass, HWAddr: req.HWAddr, + UseIfaceMAC: req.UseIfaceMAC, IP: ack.YourIPAddr, SubnetMask: ack.SubnetMask(), Routers: ack.Router(), diff --git a/spinifex/network/external/dhcp/pool_allocator.go b/spinifex/network/external/dhcp/pool_allocator.go index 68e07a80f..e39e86382 100644 --- a/spinifex/network/external/dhcp/pool_allocator.go +++ b/spinifex/network/external/dhcp/pool_allocator.go @@ -59,11 +59,12 @@ func (a *DHCPPoolAllocator) Allocate(ctx context.Context, req external.AllocateR } lease, err := a.client.RequestAcquire(ctx, AcquireParams{ - Bridge: a.pool.BindBridge, - ClientID: clientID, - HWAddr: a.hwAddrFor(clientID), - Purpose: poolPurpose(req), - PoolName: a.pool.Name, + Bridge: a.pool.BindBridge, + ClientID: clientID, + HWAddr: a.hwAddrFor(clientID), + UseIfaceMAC: a.pool.UsesIfaceMAC(), + Purpose: poolPurpose(req), + PoolName: a.pool.Name, }) if err != nil { return netip.Addr{}, fmt.Errorf("dhcp pool allocate: %w", err) diff --git a/spinifex/network/external/dhcp/wire.go b/spinifex/network/external/dhcp/wire.go index 029d7d923..ccee4ef3b 100644 --- a/spinifex/network/external/dhcp/wire.go +++ b/spinifex/network/external/dhcp/wire.go @@ -26,6 +26,7 @@ type acquireWireRequest struct { Hostname string `json:"hostname,omitempty"` VendorClass string `json:"vendor_class,omitempty"` HWAddr string `json:"hwaddr,omitempty"` + UseIfaceMAC bool `json:"use_iface_mac,omitempty"` Purpose string `json:"purpose,omitempty"` PoolName string `json:"pool_name,omitempty"` VPCID string `json:"vpc_id,omitempty"` @@ -68,6 +69,7 @@ type wireLease struct { Hostname string `json:"hostname,omitempty"` VendorClass string `json:"vendor_class,omitempty"` HWAddr string `json:"hwaddr,omitempty"` + UseIfaceMAC bool `json:"use_iface_mac,omitempty"` IP string `json:"ip,omitempty"` SubnetMask string `json:"subnet_mask,omitempty"` Routers []string `json:"routers,omitempty"` @@ -91,6 +93,7 @@ func toWireLease(l *Lease) *wireLease { Hostname: l.Hostname, VendorClass: l.VendorClass, HWAddr: hwToString(l.HWAddr), + UseIfaceMAC: l.UseIfaceMAC, IP: ipToString(l.IP), SubnetMask: maskToString(l.SubnetMask), Routers: ipsToStrings(l.Routers), @@ -114,6 +117,7 @@ func fromWireLease(w *wireLease) (*Lease, error) { ClientID: w.ClientID, Hostname: w.Hostname, VendorClass: w.VendorClass, + UseIfaceMAC: w.UseIfaceMAC, AcquiredAt: w.AcquiredAt, LeaseDuration: w.LeaseDuration, T1: w.T1, diff --git a/spinifex/network/external/igw.go b/spinifex/network/external/igw.go index 95593a710..550cfbe60 100644 --- a/spinifex/network/external/igw.go +++ b/spinifex/network/external/igw.go @@ -50,28 +50,39 @@ type IGWManager interface { EnsureEIPInstanceEgress(ctx context.Context, vpcID, subnetID, instanceIP string) error } +// RoutedIngressHooks deliver host-side ingress plumbing in routed-NAT mode: +// Ensure installs the host route sending VPC-CIDR traffic into OVN via the +// gateway LRP transit IP; Remove tears it down on detach. Both idempotent. +// Nil hooks (all other modes) are never called. +type RoutedIngressHooks struct { + Ensure func(ctx context.Context, vpcID, vpcCIDR, gwLrpIP string) error + Remove func(ctx context.Context, vpcID, vpcCIDR, gwLrpIP string) error +} + // IGWManagerConfig is the construction-time bag for igwManager. // FlowsBarrier defaults to a no-op when nil. type IGWManagerConfig struct { - OVN ovn.Client - Routes policy.RouteManager - NAT policy.NATManager - Pool *ExternalPoolConfig - Allocator GatewayIPAllocator - Chassis []string - NATMode policy.NATMode - FlowsBarrier FlowsBarrier + OVN ovn.Client + Routes policy.RouteManager + NAT policy.NATManager + Pool *ExternalPoolConfig + Allocator GatewayIPAllocator + Chassis []string + NATMode policy.NATMode + FlowsBarrier FlowsBarrier + RoutedIngress *RoutedIngressHooks } type igwManager struct { - ovn ovn.Client - routes policy.RouteManager - nat policy.NATManager - pool *ExternalPoolConfig - allocator GatewayIPAllocator - chassis []string - natMode policy.NATMode - barrier FlowsBarrier + ovn ovn.Client + routes policy.RouteManager + nat policy.NATManager + pool *ExternalPoolConfig + allocator GatewayIPAllocator + chassis []string + natMode policy.NATMode + barrier FlowsBarrier + routedIngress *RoutedIngressHooks } var _ IGWManager = (*igwManager)(nil) @@ -95,17 +106,25 @@ func NewIGWManager(cfg IGWManagerConfig) (IGWManager, error) { barrier = func() error { return nil } } return &igwManager{ - ovn: cfg.OVN, - routes: cfg.Routes, - nat: cfg.NAT, - pool: cfg.Pool, - allocator: cfg.Allocator, - chassis: cfg.Chassis, - natMode: cfg.NATMode, - barrier: barrier, + ovn: cfg.OVN, + routes: cfg.Routes, + nat: cfg.NAT, + pool: cfg.Pool, + allocator: cfg.Allocator, + chassis: cfg.Chassis, + natMode: cfg.NATMode, + barrier: barrier, + routedIngress: cfg.RoutedIngress, }, nil } +// gatewayOwnsNAT reports whether the gateway chassis owns NAT for VM egress — +// true for centralized and routed modes, where the gateway LRP gets a real IP +// from the pool and the localnet advertises router NAT addresses. +func (m *igwManager) gatewayOwnsNAT() bool { + return m.natMode == policy.NATModeCentralized || m.natMode == policy.NATModeRouted +} + // AttachIGW wires external connectivity for spec.VPCID onto the shared external // switch: it ensures the singleton external switch + localnet, attaches the VPC // gateway LRP via its own router-type switch port, installs the default route, @@ -129,6 +148,13 @@ func (m *igwManager) AttachIGW(ctx context.Context, spec IGWSpec) error { // The gateway switch port on the shared switch is the per-VPC attach marker. if _, err := m.ovn.GetLogicalSwitchPort(ctx, switchGWPortName); err == nil { + // OVN state survives reboots but host routes do not: re-run the routed + // leg so reconcile's replayed attach re-ensures ingress + SNAT. + if m.natMode == policy.NATModeRouted { + if err := m.ensureRoutedLeg(ctx, spec.VPCID, m.gatewayLRPIP(ctx, spec.VPCID)); err != nil { + return err + } + } slog.Debug("external: IGW already attached for VPC, skipping", "vpc_id", spec.VPCID, "gw_port", switchGWPortName) return nil @@ -183,6 +209,13 @@ func (m *igwManager) AttachIGW(ctx context.Context, spec IGWSpec) error { return fmt.Errorf("add default route on %s: %w", routerName, err) } + // Routed mode: egress SNATs the VPC CIDR to the gateway LRP transit IP so + // the host only ever masquerades the transit /24, and the host gets an + // ingress route to the VPC CIDR. DetachIGW reverses both. + if err := m.ensureRoutedLeg(ctx, spec.VPCID, gwLrpIP); err != nil { + return err + } + if len(m.chassis) == 0 { slog.Warn("external: no chassis configured — gateway port has no chassis binding, external traffic will not flow", "vpc_id", spec.VPCID, "gw_port", gwPortName) @@ -209,6 +242,52 @@ func (m *igwManager) AttachIGW(ctx context.Context, spec IGWSpec) error { return nil } +// ensureRoutedLeg installs the routed-mode datapath legs for a VPC: the OVN +// SNAT (VPC CIDR → gateway LRP transit IP) and the host ingress route into +// it. No-op outside routed mode or without a gateway IP. Idempotent — the +// reconcile loop replays AttachIGW, re-ensuring host state lost on reboot. +func (m *igwManager) ensureRoutedLeg(ctx context.Context, vpcID, gwLrpIP string) error { + if m.natMode != policy.NATModeRouted || gwLrpIP == "" { + return nil + } + routerName := topology.VPCRouter(vpcID) + router, err := m.ovn.GetLogicalRouter(ctx, routerName) + if err != nil { + return fmt.Errorf("get router %s for routed SNAT: %w", routerName, err) + } + vpcCIDR := router.ExternalIDs["spinifex:cidr"] + if vpcCIDR == "" { + return fmt.Errorf("routed NAT: router %s has no spinifex:cidr external_id", routerName) + } + if err := m.nat.AddSNAT(ctx, vpcID, vpcCIDR, gwLrpIP); err != nil { + return fmt.Errorf("install routed SNAT %s -> %s: %w", vpcCIDR, gwLrpIP, err) + } + if m.routedIngress != nil && m.routedIngress.Ensure != nil { + if err := m.routedIngress.Ensure(ctx, vpcID, vpcCIDR, gwLrpIP); err != nil { + return fmt.Errorf("install routed ingress route %s via %s: %w", vpcCIDR, gwLrpIP, err) + } + } + return nil +} + +// gatewayLRPIP returns the transit IP of the VPC's gateway LRP — the stamped +// external_id, falling back to the host part of Networks[0]. Empty on miss. +func (m *igwManager) gatewayLRPIP(ctx context.Context, vpcID string) string { + lrp, err := m.ovn.GetLogicalRouterPort(ctx, topology.GatewayRouterPort(vpcID)) + if err != nil || lrp == nil { + return "" + } + if ip := lrp.ExternalIDs[gatewayIPExtIDKey]; ip != "" { + return ip + } + if len(lrp.Networks) > 0 { + if pfx, err := netip.ParsePrefix(lrp.Networks[0]); err == nil { + return pfx.Addr().String() + } + } + return "" +} + // ensureSharedExternal idempotently creates the singleton shared external switch // and its single localnet port. The localnet advertises router NAT addresses in // centralized mode so gateways behind it are reachable from the uplink. @@ -224,7 +303,7 @@ func (m *igwManager) ensureSharedExternal(ctx context.Context, switchName, portN return nil } localnetOpts := map[string]string{"network_name": "external"} - if m.natMode == policy.NATModeCentralized { + if m.gatewayOwnsNAT() { localnetOpts["nat-addresses"] = "router" } if err := m.ovn.CreateLogicalSwitchPort(ctx, switchName, &nbdb.LogicalSwitchPort{ @@ -262,6 +341,14 @@ func (m *igwManager) DetachIGW(ctx context.Context, vpcID string) error { if err := m.nat.DeleteSNAT(ctx, vpcID, vpcCIDR); err != nil { slog.Warn("external: delete IGW SNAT failed", "router", routerName, "cidr", vpcCIDR, "err", err) } + if m.routedIngress != nil && m.routedIngress.Remove != nil { + // gwLrpIP resolved before the LRP is deleted below — Remove + // needs it to prove this VPC owns the host route it deletes. + gwLrpIP := m.gatewayLRPIP(ctx, vpcID) + if err := m.routedIngress.Remove(ctx, vpcID, vpcCIDR, gwLrpIP); err != nil { + slog.Warn("external: remove routed ingress route failed", "cidr", vpcCIDR, "err", err) + } + } } } else { slog.Warn("external: get router for NAT cleanup failed", "router", routerName, "err", err) @@ -496,7 +583,7 @@ func (m *igwManager) resolveGatewayNetwork(ctx context.Context, vpcID string) (n nexthop = m.pool.Gateway } - if m.natMode != policy.NATModeCentralized || m.pool == nil { + if !m.gatewayOwnsNAT() || m.pool == nil { return network, nexthop, "", nil } diff --git a/spinifex/network/external/igw_routed_test.go b/spinifex/network/external/igw_routed_test.go new file mode 100644 index 000000000..6ac5a3664 --- /dev/null +++ b/spinifex/network/external/igw_routed_test.go @@ -0,0 +1,192 @@ +package external + +import ( + "context" + "testing" + + "github.com/mulgadc/spinifex/spinifex/network/ovn/mock" + "github.com/mulgadc/spinifex/spinifex/network/ovn/nbdb" + "github.com/mulgadc/spinifex/spinifex/network/policy" + "github.com/mulgadc/spinifex/spinifex/network/topology" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func routedTestPool() *ExternalPoolConfig { + return &ExternalPoolConfig{ + Name: "nat-transit", Gateway: "100.127.0.1", PrefixLen: 24, + GwLrpRangeStart: "100.127.0.240", GwLrpRangeEnd: "100.127.0.243", + } +} + +func findSNAT(m *mock.Client, logicalIP string) *nbdb.NAT { + for _, n := range m.NATs { + if n.Type == "snat" && n.LogicalIP == logicalIP { + return n + } + } + return nil +} + +func TestAttachIGW_Routed_AllocatesTransitIPAndInstallsSNAT(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedVPCRouter(t, m, "vpc-1", "10.0.0.0/16") + mgr, _ := newTestIGWManager(t, m, policy.NATModeRouted, routedTestPool(), NewStaticRangeAllocator(m), []string{"chassis-a"}) + + require.NoError(t, mgr.AttachIGW(ctx, IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) + + localnet, err := m.GetLogicalSwitchPort(ctx, topology.ExternalLocalnetPortShared()) + require.NoError(t, err) + assert.Equal(t, "router", localnet.Options["nat-addresses"], "routed mode must set nat-addresses=router") + + lrp, err := m.GetLogicalRouterPort(ctx, topology.GatewayRouterPort("vpc-1")) + require.NoError(t, err) + assert.Equal(t, []string{"100.127.0.240/24"}, lrp.Networks, "gateway LRP must get a transit pool IP") + + snat := findSNAT(m, "10.0.0.0/16") + require.NotNil(t, snat, "routed mode must install snat VPC CIDR -> gateway LRP IP") + assert.Equal(t, "100.127.0.240", snat.ExternalIP) + assert.Equal(t, "igw-default-snat", snat.ExternalIDs["spinifex:role"]) +} + +func TestAttachIGW_Routed_MissingVPCCIDRFails(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedVPCRouter(t, m, "vpc-1", "") + mgr, _ := newTestIGWManager(t, m, policy.NATModeRouted, routedTestPool(), NewStaticRangeAllocator(m), []string{"chassis-a"}) + + err := mgr.AttachIGW(ctx, IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "spinifex:cidr") +} + +func TestDetachIGW_Routed_RemovesSNAT(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedVPCRouter(t, m, "vpc-1", "10.0.0.0/16") + mgr, _ := newTestIGWManager(t, m, policy.NATModeRouted, routedTestPool(), NewStaticRangeAllocator(m), []string{"chassis-a"}) + + require.NoError(t, mgr.AttachIGW(ctx, IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) + require.NotNil(t, findSNAT(m, "10.0.0.0/16")) + + require.NoError(t, mgr.DetachIGW(ctx, "vpc-1")) + assert.Nil(t, findSNAT(m, "10.0.0.0/16"), "detach must remove the routed SNAT") +} + +func TestNewManagers_AcceptRoutedMode(t *testing.T) { + m := mock.New() + nm, err := policy.NewNATManager(m, policy.NATModeRouted) + require.NoError(t, err) + _, err = NewIGWManager(IGWManagerConfig{ + OVN: m, Routes: policy.NewRouteManager(m), NAT: nm, + Allocator: NewStaticRangeAllocator(m), NATMode: policy.NATModeRouted, + }) + require.NoError(t, err) +} + +type recordedIngress struct { + ensures []string // "vpcID cidr via gwLrpIP" + removes []string // "vpcID cidr via gwLrpIP" +} + +func (r *recordedIngress) hooks() *RoutedIngressHooks { + return &RoutedIngressHooks{ + Ensure: func(_ context.Context, vpcID, vpcCIDR, gwLrpIP string) error { + r.ensures = append(r.ensures, vpcID+" "+vpcCIDR+" via "+gwLrpIP) + return nil + }, + Remove: func(_ context.Context, vpcID, vpcCIDR, gwLrpIP string) error { + r.removes = append(r.removes, vpcID+" "+vpcCIDR+" via "+gwLrpIP) + return nil + }, + } +} + +func newRoutedIngressIGWManager(t *testing.T, m *mock.Client, mode policy.NATMode) (IGWManager, *recordedIngress) { + t.Helper() + nm, err := policy.NewNATManager(m, mode, + policy.WithSNATExemptSet(policy.NATExemptSetName, []string{"100.127.0.0/24"})) + require.NoError(t, err) + rec := &recordedIngress{} + mgr, err := NewIGWManager(IGWManagerConfig{ + OVN: m, Routes: policy.NewRouteManager(m), NAT: nm, + Pool: routedTestPool(), Allocator: NewStaticRangeAllocator(m), + Chassis: []string{"chassis-a"}, NATMode: mode, + RoutedIngress: rec.hooks(), + }) + require.NoError(t, err) + return mgr, rec +} + +func TestAttachIGW_Routed_InstallsIngressRoute(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedVPCRouter(t, m, "vpc-1", "10.0.0.0/16") + mgr, rec := newRoutedIngressIGWManager(t, m, policy.NATModeRouted) + + require.NoError(t, mgr.AttachIGW(ctx, IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) + require.Equal(t, []string{"vpc-1 10.0.0.0/16 via 100.127.0.240"}, rec.ensures) + + snat := findSNAT(m, "10.0.0.0/16") + require.NotNil(t, snat) + require.NotNil(t, snat.ExemptedExtIps, "routed SNAT must carry the exempt set ref") +} + +func TestAttachIGW_Routed_AlreadyAttachedReEnsuresIngress(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedVPCRouter(t, m, "vpc-1", "10.0.0.0/16") + mgr, rec := newRoutedIngressIGWManager(t, m, policy.NATModeRouted) + + require.NoError(t, mgr.AttachIGW(ctx, IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) + // Simulate reboot: OVN state survives, host route lost, SNAT loses its ref + // (legacy row shape). The replayed attach hits the already-attached marker. + require.NoError(t, m.SetNATExemptedExtIPs(ctx, topology.VPCRouter("vpc-1"), "snat", "10.0.0.0/16", nil)) + + require.NoError(t, mgr.AttachIGW(ctx, IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) + require.Equal(t, []string{ + "vpc-1 10.0.0.0/16 via 100.127.0.240", + "vpc-1 10.0.0.0/16 via 100.127.0.240", + }, rec.ensures, "already-attached path must re-invoke the ingress hook") + + snat := findSNAT(m, "10.0.0.0/16") + require.NotNil(t, snat.ExemptedExtIps, "already-attached path must patch the legacy SNAT ref") +} + +func TestDetachIGW_Routed_RemovesIngressRoute(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedVPCRouter(t, m, "vpc-1", "10.0.0.0/16") + mgr, rec := newRoutedIngressIGWManager(t, m, policy.NATModeRouted) + + require.NoError(t, mgr.AttachIGW(ctx, IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) + require.NoError(t, mgr.DetachIGW(ctx, "vpc-1")) + require.Equal(t, []string{"vpc-1 10.0.0.0/16 via 100.127.0.240"}, rec.removes, + "detach must pass the VPC's own gateway IP so ownership checks work") +} + +func TestAttachIGW_NonRouted_NeverCallsIngressHooks(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedVPCRouter(t, m, "vpc-1", "10.0.0.0/16") + + nm, err := policy.NewNATManager(m, policy.NATModeCentralized) + require.NoError(t, err) + rec := &recordedIngress{} + mgr, err := NewIGWManager(IGWManagerConfig{ + OVN: m, Routes: policy.NewRouteManager(m), NAT: nm, + Pool: &ExternalPoolConfig{ + Name: "wan", Gateway: "192.168.1.1", PrefixLen: 24, + GwLrpRangeStart: "192.168.1.240", GwLrpRangeEnd: "192.168.1.243", + }, + Allocator: NewStaticRangeAllocator(m), + Chassis: []string{"chassis-a"}, NATMode: policy.NATModeCentralized, + RoutedIngress: rec.hooks(), + }) + require.NoError(t, err) + + require.NoError(t, mgr.AttachIGW(ctx, IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) + require.NoError(t, mgr.AttachIGW(ctx, IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) + assert.Empty(t, rec.ensures, "non-routed modes must never install host ingress routes") +} diff --git a/spinifex/network/external/pool.go b/spinifex/network/external/pool.go index 2c8ce2c6e..f53849be1 100644 --- a/spinifex/network/external/pool.go +++ b/spinifex/network/external/pool.go @@ -7,6 +7,7 @@ import ( "net" "github.com/mulgadc/spinifex/spinifex/network/ovn" + "github.com/mulgadc/spinifex/spinifex/network/policy" "github.com/mulgadc/spinifex/spinifex/network/topology" ) @@ -17,7 +18,7 @@ const linkLocalGatewayNetwork = "169.254.0.1/30" const linkLocalGatewayNexthop = "169.254.0.2" // gatewayIPExtIDKey is the LRP external_ids key for the allocated gateway IP. -const gatewayIPExtIDKey = "spinifex:gateway_ip" +const gatewayIPExtIDKey = policy.GatewayIPExtIDKey // FindPool returns the first matching pool: AZ-scoped → region-scoped → unscoped. func FindPool(pools []ExternalPoolConfig, region, az string) *ExternalPoolConfig { diff --git a/spinifex/network/external/types.go b/spinifex/network/external/types.go index 9eb44568f..c8e36f28c 100644 --- a/spinifex/network/external/types.go +++ b/spinifex/network/external/types.go @@ -9,7 +9,11 @@ type ExternalPoolConfig struct { Source string // BindBridge is the Linux bridge the DHCP client runs against // (required when Source="dhcp"). Empty for static pools. - BindBridge string + BindBridge string + // DHCPMAC picks the DHCP client MAC strategy: "derived" (default, + // per-client-id 02:xx MACs) or "interface" (the bind interface's own + // MAC, for uplinks that drop foreign source MACs — WiFi/WWAN). + DHCPMAC string RangeStart string RangeEnd string Gateway string @@ -32,8 +36,20 @@ const ( SourceStatic = "static" // SourceDHCP delegates allocation to vpcd's DHCPManager. SourceDHCP = "dhcp" + + // DHCPMACDerived leases with deterministic per-client-id 02:xx MACs. + DHCPMACDerived = "derived" + // DHCPMACInterface leases with the bind interface's own MAC; the + // upstream router keys on MAC, so client-id is the only discriminator. + DHCPMACInterface = "interface" ) +// UsesIfaceMAC reports whether DHCP leases go out with the bind +// interface's own MAC instead of derived per-client-id MACs. +func (p *ExternalPoolConfig) UsesIfaceMAC() bool { + return p != nil && p.DHCPMAC == DHCPMACInterface +} + // IGWSpec is the L5 input for IGWManager.AttachIGW / DetachIGW. // InternetGatewayID is propagated into OVN external_ids for reconcile correlation. type IGWSpec struct { diff --git a/spinifex/network/host/eip_ingress.go b/spinifex/network/host/eip_ingress.go new file mode 100644 index 000000000..09ad9c84c --- /dev/null +++ b/spinifex/network/host/eip_ingress.go @@ -0,0 +1,160 @@ +package host + +import ( + "context" + "fmt" + "log/slog" + "strings" +) + +const eipIngressComment = "spinifex-eip-ingress" + +// DetectUplinkFor resolves which interface (and local source IP) the kernel +// would use to reach gatewayIP by parsing `ip route get`. Re-run per ensure: +// WiFi/cellular uplinks change interface and address across reconnects. +func DetectUplinkFor(ctx context.Context, r Runner, gatewayIP string) (iface, srcIP string, err error) { + if gatewayIP == "" { + return "", "", fmt.Errorf("DetectUplinkFor: gatewayIP required") + } + out, err := r.Run(ctx, "ip", "route", "get", gatewayIP) + if err != nil { + return "", "", fmt.Errorf("route lookup for %s: %s: %w", gatewayIP, string(out), err) + } + fields := strings.Fields(strings.SplitN(string(out), "\n", 2)[0]) + for i := 0; i < len(fields)-1; i++ { + switch fields[i] { + case "dev": + iface = fields[i+1] + case "src": + srcIP = fields[i+1] + } + } + if iface == "" { + return "", "", fmt.Errorf("no dev in route to %s: %q", gatewayIP, strings.TrimSpace(string(out))) + } + return iface, srcIP, nil +} + +// uplinkAddr returns the first global IPv4 on iface, or "" when none parses. +// Best-effort: the src hint on the EIP route is an optimization, not plumbing. +func uplinkAddr(ctx context.Context, r Runner, iface string) string { + out, err := r.Run(ctx, "ip", "-4", "-o", "addr", "show", "dev", iface, "scope", "global") + if err != nil { + return "" + } + fields := strings.Fields(string(out)) + for i := 0; i < len(fields)-1; i++ { + if fields[i] == "inet" { + return strings.SplitN(fields[i+1], "/", 2)[0] + } + } + return "" +} + +// resolveUplink picks the LAN-facing interface for EIP proxy-ARP: routed +// toward poolGateway when known (static pools), else the configured hint +// (dhcp pools DORA on the uplink itself). Probing the EIP would be wrong — +// once the /32 route lands, the kernel routes it to the transit veth. +func resolveUplink(ctx context.Context, r Runner, poolGateway, uplinkHint string) (iface, srcIP string, err error) { + if poolGateway != "" { + return DetectUplinkFor(ctx, r, poolGateway) + } + if uplinkHint != "" { + return uplinkHint, uplinkAddr(ctx, r, uplinkHint), nil + } + return "", "", nil +} + +func eipForwardRules(eip string) [][]string { + return [][]string{ + {"-i", NATTransitHostEnd, "-s", eip + "/32", + "-m", "comment", "--comment", eipIngressComment, "-j", "ACCEPT"}, + {"-o", NATTransitHostEnd, "-d", eip + "/32", + "-m", "comment", "--comment", eipIngressComment, "-j", "ACCEPT"}, + } +} + +// EnsureEIPIngress makes an EIP reachable from the uplink LAN in routed-NAT +// mode: a /32 host route steers the EIP into OVN via the VPC gateway LRP's +// transit IP, a proxy-ARP neighbor entry answers ARP for the EIP on the +// uplink (L3 only — same MAC, so WiFi-safe), and per-EIP FORWARD accepts +// cover DROP-policy hosts. The route carries `src ` so host-local +// traffic to the EIP still DNATs (host source would match the exempt set). +// Idempotent; call on every EIP bind and reconcile pass. +func EnsureEIPIngress(ctx context.Context, r Runner, eip, gwLrpIP, poolGateway, uplinkHint string) error { + if eip == "" { + return fmt.Errorf("EnsureEIPIngress: eip required") + } + if gwLrpIP == "" { + return fmt.Errorf("EnsureEIPIngress: gwLrpIP required") + } + + uplink, srcIP, err := resolveUplink(ctx, r, poolGateway, uplinkHint) + if err != nil { + return fmt.Errorf("EnsureEIPIngress %s: %w", eip, err) + } + + routeArgs := []string{"route", "replace", eip + "/32", "via", gwLrpIP, "dev", NATTransitHostEnd} + if srcIP != "" { + routeArgs = append(routeArgs, "src", srcIP) + } + if out, err := r.Run(ctx, "ip", routeArgs...); err != nil { + return fmt.Errorf("install EIP route %s via %s: %s: %w", eip, gwLrpIP, string(out), err) + } + + if uplink != "" { + if out, err := r.Run(ctx, "ip", "neigh", "replace", "proxy", eip, "dev", uplink); err != nil { + return fmt.Errorf("install proxy-ARP for %s on %s: %s: %w", eip, uplink, string(out), err) + } + // Answer proxied ARP immediately instead of the ~800ms default delay. + if out, err := r.Run(ctx, "sysctl", "-w", "net.ipv4.neigh."+uplink+".proxy_delay=0"); err != nil { + slog.Debug("host: proxy_delay tune failed (non-fatal)", "uplink", uplink, "out", string(out), "err", err) + } + // Gratuitous ARP so LAN peers refresh stale entries for reused EIPs. + if out, err := r.Run(ctx, "arping", "-U", "-c", "2", "-I", uplink, eip); err != nil { + slog.Debug("host: gratuitous ARP failed (non-fatal)", "eip", eip, "uplink", uplink, "out", string(out), "err", err) + } + } + + for _, spec := range eipForwardRules(eip) { + if _, err := r.Run(ctx, "iptables", natRuleArgs("-C", "filter", "FORWARD", spec)...); err == nil { + continue + } + if out, err := r.Run(ctx, "iptables", natRuleArgs("-A", "filter", "FORWARD", spec)...); err != nil { + return fmt.Errorf("install EIP FORWARD rule for %s: %s: %w", eip, string(out), err) + } + } + + slog.Info("EIP ingress installed", "eip", eip, "gw_lrp_ip", gwLrpIP, "uplink", uplink, "src", srcIP) + return nil +} + +// RemoveEIPIngress tears down the host state for an EIP on disassociate or +// release. Missing pieces are not errors (idempotent teardown); the proxy +// neighbor is removed from whatever uplink currently faces the pool. +func RemoveEIPIngress(ctx context.Context, r Runner, eip, poolGateway, uplinkHint string) error { + if eip == "" { + return fmt.Errorf("RemoveEIPIngress: eip required") + } + + if _, err := r.Run(ctx, "ip", "route", "del", eip+"/32", "dev", NATTransitHostEnd); err != nil { + slog.Debug("host: EIP route already absent", "eip", eip, "err", err) + } + + if uplink, _, err := resolveUplink(ctx, r, poolGateway, uplinkHint); err != nil { + slog.Debug("host: uplink detection failed on EIP teardown", "eip", eip, "err", err) + } else if uplink != "" { + if _, err := r.Run(ctx, "ip", "neigh", "del", "proxy", eip, "dev", uplink); err != nil { + slog.Debug("host: proxy-ARP entry already absent", "eip", eip, "uplink", uplink, "err", err) + } + } + + for _, spec := range eipForwardRules(eip) { + if _, err := r.Run(ctx, "iptables", natRuleArgs("-D", "filter", "FORWARD", spec)...); err != nil { + slog.Debug("host: EIP FORWARD rule not present on delete", "eip", eip, "err", err) + } + } + + slog.Info("EIP ingress removed", "eip", eip) + return nil +} diff --git a/spinifex/network/host/eip_ingress_test.go b/spinifex/network/host/eip_ingress_test.go new file mode 100644 index 000000000..4a01cbfa4 --- /dev/null +++ b/spinifex/network/host/eip_ingress_test.go @@ -0,0 +1,197 @@ +package host + +import ( + "context" + "fmt" + "strings" + "testing" +) + +const ( + wiredRouteGet = "192.168.1.1 dev eth0 src 192.168.1.42 uid 0\n cache\n" + wifiRouteGet = "192.168.1.1 dev wlan0 proto dhcp src 192.168.1.87 metric 600\n cache\n" +) + +func TestDetectUplinkFor(t *testing.T) { + tests := []struct { + name string + out string + wantIface string + wantSrc string + wantErr bool + }{ + {name: "wired", out: wiredRouteGet, wantIface: "eth0", wantSrc: "192.168.1.42"}, + {name: "wifi with metric", out: wifiRouteGet, wantIface: "wlan0", wantSrc: "192.168.1.87"}, + {name: "no dev", out: "unreachable 192.168.1.1\n", wantErr: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := newStubRunner() + r.expect("ip route get", []byte(tt.out), nil) + iface, src, err := DetectUplinkFor(context.Background(), r, "192.168.1.1") + if tt.wantErr { + if err == nil { + t.Fatal("expected error") + } + return + } + if err != nil { + t.Fatalf("DetectUplinkFor: %v", err) + } + if iface != tt.wantIface || src != tt.wantSrc { + t.Errorf("got (%q, %q), want (%q, %q)", iface, src, tt.wantIface, tt.wantSrc) + } + }) + } +} + +func TestDetectUplinkFor_LookupFailure(t *testing.T) { + r := newStubRunner() + r.expect("ip route get", []byte("RTNETLINK answers: Network is unreachable"), fmt.Errorf("exit 2")) + if _, _, err := DetectUplinkFor(context.Background(), r, "192.168.1.1"); err == nil || !strings.Contains(err.Error(), "unreachable") { + t.Fatalf("expected lookup error with output, got: %v", err) + } +} + +func newEIPStubRunner(routeGet string) *stubRunner { + r := newStubRunner() + r.expect("ip route get", []byte(routeGet), nil) + r.expect("ip route replace", nil, nil) + r.expect("ip neigh replace proxy", nil, nil) + r.expect("sysctl -w", nil, nil) + r.expect("arping", nil, nil) + r.expect("iptables -t filter -C", []byte("iptables: No chain/target/match by that name."), fmt.Errorf("exit 1")) + r.expect("iptables -t filter -A", nil, nil) + return r +} + +func TestEnsureEIPIngress_FullPlumbing(t *testing.T) { + r := newEIPStubRunner(wiredRouteGet) + if err := EnsureEIPIngress(context.Background(), r, "192.168.1.200", "100.127.0.10", "192.168.1.1", ""); err != nil { + t.Fatalf("EnsureEIPIngress: %v", err) + } + for _, want := range []string{ + "ip route replace 192.168.1.200/32 via 100.127.0.10 dev " + NATTransitHostEnd + " src 192.168.1.42", + "ip neigh replace proxy 192.168.1.200 dev eth0", + "sysctl -w net.ipv4.neigh.eth0.proxy_delay=0", + "arping -U -c 2 -I eth0 192.168.1.200", + "iptables -t filter -A FORWARD -i " + NATTransitHostEnd + " -s 192.168.1.200/32", + "iptables -t filter -A FORWARD -o " + NATTransitHostEnd + " -d 192.168.1.200/32", + } { + if !r.called(want) { + t.Errorf("missing call:\n want %q\n got %v", want, r.calls) + } + } +} + +func TestEnsureEIPIngress_WiFiUplink(t *testing.T) { + r := newEIPStubRunner(wifiRouteGet) + if err := EnsureEIPIngress(context.Background(), r, "192.168.1.200", "100.127.0.10", "192.168.1.1", ""); err != nil { + t.Fatalf("EnsureEIPIngress: %v", err) + } + if !r.called("ip neigh replace proxy 192.168.1.200 dev wlan0") { + t.Errorf("proxy-ARP must land on detected wifi uplink: %v", r.calls) + } + if !r.called("ip route replace 192.168.1.200/32 via 100.127.0.10 dev " + NATTransitHostEnd + " src 192.168.1.87") { + t.Errorf("route must carry wifi src IP: %v", r.calls) + } +} + +func TestEnsureEIPIngress_IdempotentSkipsExistingRules(t *testing.T) { + r := newEIPStubRunner(wiredRouteGet) + r.expect("iptables -t filter -C", nil, nil) + if err := EnsureEIPIngress(context.Background(), r, "192.168.1.200", "100.127.0.10", "192.168.1.1", ""); err != nil { + t.Fatalf("EnsureEIPIngress: %v", err) + } + if r.called("iptables -t filter -A") { + t.Errorf("existing FORWARD rules must not be re-appended: %v", r.calls) + } +} + +func TestEnsureEIPIngress_ArpingFailureNonFatal(t *testing.T) { + r := newEIPStubRunner(wiredRouteGet) + r.expect("arping", []byte("arping: command not found"), fmt.Errorf("exit 127")) + r.expect("sysctl -w", []byte("permission denied"), fmt.Errorf("exit 1")) + if err := EnsureEIPIngress(context.Background(), r, "192.168.1.200", "100.127.0.10", "192.168.1.1", ""); err != nil { + t.Fatalf("arping/sysctl failures must be non-fatal: %v", err) + } +} + +func TestEnsureEIPIngress_NoGatewaySkipsUplinkPlumbing(t *testing.T) { + r := newStubRunner() + r.expect("ip route replace", nil, nil) + r.expect("iptables -t filter -C", nil, nil) + if err := EnsureEIPIngress(context.Background(), r, "192.168.1.200", "100.127.0.10", "", ""); err != nil { + t.Fatalf("EnsureEIPIngress: %v", err) + } + if !r.called("ip route replace 192.168.1.200/32 via 100.127.0.10 dev " + NATTransitHostEnd) { + t.Errorf("route must install without gateway: %v", r.calls) + } + if r.called("ip neigh") || r.called("ip route get") { + t.Errorf("no proxy-ARP or uplink detection without gateway: %v", r.calls) + } + if strings.Contains(strings.Join(r.calls, " "), " src ") { + t.Errorf("no src hint without uplink detection: %v", r.calls) + } +} + +func TestEnsureEIPIngress_UplinkHintForDHCPPool(t *testing.T) { + r := newEIPStubRunner("") + r.expect("ip -4 -o addr show", []byte("3: wlan0 inet 192.168.1.87/24 brd 192.168.1.255 scope global dynamic wlan0\n"), nil) + if err := EnsureEIPIngress(context.Background(), r, "192.168.1.200", "100.127.0.10", "", "wlan0"); err != nil { + t.Fatalf("EnsureEIPIngress: %v", err) + } + if r.called("ip route get") { + t.Errorf("hint must bypass gateway route lookup: %v", r.calls) + } + if !r.called("ip neigh replace proxy 192.168.1.200 dev wlan0") { + t.Errorf("proxy-ARP must land on hinted uplink: %v", r.calls) + } + if !r.called("ip route replace 192.168.1.200/32 via 100.127.0.10 dev " + NATTransitHostEnd + " src 192.168.1.87") { + t.Errorf("route must carry uplink addr as src: %v", r.calls) + } +} + +func TestEnsureEIPIngress_ValidatesArgs(t *testing.T) { + r := newStubRunner() + if err := EnsureEIPIngress(context.Background(), r, "", "100.127.0.10", "", ""); err == nil { + t.Error("empty eip: expected error") + } + if err := EnsureEIPIngress(context.Background(), r, "192.168.1.200", "", "", ""); err == nil { + t.Error("empty gwLrpIP: expected error") + } + if len(r.calls) != 0 { + t.Errorf("validation failures must not run commands: %v", r.calls) + } +} + +func TestRemoveEIPIngress_TearsDownAll(t *testing.T) { + r := newStubRunner() + r.expect("ip route get", []byte(wiredRouteGet), nil) + r.expect("ip route del", nil, nil) + r.expect("ip neigh del proxy", nil, nil) + r.expect("iptables -t filter -D", nil, nil) + if err := RemoveEIPIngress(context.Background(), r, "192.168.1.200", "192.168.1.1", ""); err != nil { + t.Fatalf("RemoveEIPIngress: %v", err) + } + for _, want := range []string{ + "ip route del 192.168.1.200/32 dev " + NATTransitHostEnd, + "ip neigh del proxy 192.168.1.200 dev eth0", + "iptables -t filter -D FORWARD -i " + NATTransitHostEnd + " -s 192.168.1.200/32", + "iptables -t filter -D FORWARD -o " + NATTransitHostEnd + " -d 192.168.1.200/32", + } { + if !r.called(want) { + t.Errorf("missing teardown call:\n want %q\n got %v", want, r.calls) + } + } +} + +func TestRemoveEIPIngress_MissingStateNotError(t *testing.T) { + r := newStubRunner() + r.expect("ip route get", []byte("RTNETLINK answers: Network is unreachable"), fmt.Errorf("exit 2")) + r.expect("ip route del", []byte("RTNETLINK answers: No such process"), fmt.Errorf("exit 2")) + r.expect("iptables -t filter -D", []byte("Bad rule"), fmt.Errorf("exit 1")) + if err := RemoveEIPIngress(context.Background(), r, "192.168.1.200", "192.168.1.1", ""); err != nil { + t.Fatalf("missing state must not be an error, got: %v", err) + } +} diff --git a/spinifex/network/host/host.go b/spinifex/network/host/host.go index 620cde9d7..cf39ad1aa 100644 --- a/spinifex/network/host/host.go +++ b/spinifex/network/host/host.go @@ -21,6 +21,10 @@ const ( // UplinkModeVeth bridges a Linux bridge to br-ext via a veth pair (centralised NAT). UplinkModeVeth + + // UplinkModeRouted leaves br-ext with no WAN NIC; a veth pair links it to + // the host stack, which forwards + masquerades the transit /24 (routed NAT). + UplinkModeRouted ) func (m UplinkMode) String() string { @@ -29,6 +33,8 @@ func (m UplinkMode) String() string { return "physical" case UplinkModeVeth: return "veth" + case UplinkModeRouted: + return "routed" default: return "unknown" } diff --git a/spinifex/network/host/host_test.go b/spinifex/network/host/host_test.go index ee6def29c..26f6a389b 100644 --- a/spinifex/network/host/host_test.go +++ b/spinifex/network/host/host_test.go @@ -113,6 +113,7 @@ func TestUplinkModeString(t *testing.T) { }{ {UplinkModePhysical, "physical"}, {UplinkModeVeth, "veth"}, + {UplinkModeRouted, "routed"}, {UplinkModeUnknown, "unknown"}, {UplinkMode(99), "unknown"}, } diff --git a/spinifex/network/host/natrules.go b/spinifex/network/host/natrules.go new file mode 100644 index 000000000..d0d37f8a6 --- /dev/null +++ b/spinifex/network/host/natrules.go @@ -0,0 +1,59 @@ +package host + +import ( + "context" + "fmt" + "log/slog" +) + +const natEgressComment = "spinifex-nat-egress" + +// natEgressRules are the kernel rules giving routed-NAT VMs outbound WAN: +// masquerade the transit /24 out any uplink, and accept forwarded transit +// traffic even when the FORWARD policy is DROP (Docker/firewalld hosts). +var natEgressRules = []struct { + table string + chain string + spec []string +}{ + {"nat", "POSTROUTING", []string{ + "-s", NATTransitCIDR, "!", "-d", NATTransitCIDR, + "-m", "comment", "--comment", natEgressComment, "-j", "MASQUERADE"}}, + {"filter", "FORWARD", []string{ + "-i", NATTransitHostEnd, "-s", NATTransitCIDR, + "-m", "comment", "--comment", natEgressComment, "-j", "ACCEPT"}}, + {"filter", "FORWARD", []string{ + "-o", NATTransitHostEnd, "-m", "conntrack", "--ctstate", "RELATED,ESTABLISHED", + "-m", "comment", "--comment", natEgressComment, "-j", "ACCEPT"}}, +} + +func natRuleArgs(op, table, chain string, spec []string) []string { + args := []string{"-t", table, op, chain} + return append(args, spec...) +} + +// EnsureNATEgressRules idempotently installs the routed-NAT egress rules: +// probe with -C, append with -A only when missing. vpcd calls this on every +// start, so rules survive reboots without iptables-persistent. +func EnsureNATEgressRules(ctx context.Context, r Runner) error { + for _, rule := range natEgressRules { + if _, err := r.Run(ctx, "iptables", natRuleArgs("-C", rule.table, rule.chain, rule.spec)...); err == nil { + continue + } + if out, err := r.Run(ctx, "iptables", natRuleArgs("-A", rule.table, rule.chain, rule.spec)...); err != nil { + return fmt.Errorf("install NAT egress rule (%s %s): %s: %w", rule.table, rule.chain, string(out), err) + } + slog.Info("host: installed NAT egress rule", "table", rule.table, "chain", rule.chain) + } + return nil +} + +// RemoveNATEgressRules deletes the routed-NAT egress rules; missing rules are +// not an error (teardown is idempotent). +func RemoveNATEgressRules(ctx context.Context, r Runner) { + for _, rule := range natEgressRules { + if _, err := r.Run(ctx, "iptables", natRuleArgs("-D", rule.table, rule.chain, rule.spec)...); err != nil { + slog.Debug("host: NAT egress rule not present on delete", "table", rule.table, "chain", rule.chain, "err", err) + } + } +} diff --git a/spinifex/network/host/natrules_test.go b/spinifex/network/host/natrules_test.go new file mode 100644 index 000000000..1e3e9f8ed --- /dev/null +++ b/spinifex/network/host/natrules_test.go @@ -0,0 +1,70 @@ +package host + +import ( + "context" + "fmt" + "strings" + "testing" +) + +func TestEnsureNATEgressRules_InstallsAllWhenMissing(t *testing.T) { + r := newStubRunner() + r.expect("iptables -t nat -C", nil, fmt.Errorf("no match")) + r.expect("iptables -t filter -C", nil, fmt.Errorf("no match")) + r.expect("iptables -t nat -A", nil, nil) + r.expect("iptables -t filter -A", nil, nil) + + if err := EnsureNATEgressRules(context.Background(), r); err != nil { + t.Fatalf("EnsureNATEgressRules: %v", err) + } + wantAppends := []string{ + "iptables -t nat -A POSTROUTING -s " + NATTransitCIDR + " ! -d " + NATTransitCIDR + + " -m comment --comment spinifex-nat-egress -j MASQUERADE", + "iptables -t filter -A FORWARD -i " + NATTransitHostEnd + " -s " + NATTransitCIDR + + " -m comment --comment spinifex-nat-egress -j ACCEPT", + "iptables -t filter -A FORWARD -o " + NATTransitHostEnd + " -m conntrack --ctstate RELATED,ESTABLISHED" + + " -m comment --comment spinifex-nat-egress -j ACCEPT", + } + for _, want := range wantAppends { + if !r.called(want) { + t.Errorf("missing append call:\n want %q\n got %v", want, r.calls) + } + } +} + +func TestEnsureNATEgressRules_SkipsWhenPresent(t *testing.T) { + r := newStubRunner() + r.expect("iptables -t nat -C", nil, nil) + r.expect("iptables -t filter -C", nil, nil) + + if err := EnsureNATEgressRules(context.Background(), r); err != nil { + t.Fatalf("EnsureNATEgressRules: %v", err) + } + for _, c := range r.calls { + if strings.Contains(c, " -A ") { + t.Errorf("unexpected append when rule present: %q", c) + } + } +} + +func TestEnsureNATEgressRules_AppendFailure(t *testing.T) { + r := newStubRunner() + r.expect("iptables -t nat -C", nil, fmt.Errorf("no match")) + r.expect("iptables -t nat -A", []byte("iptables: permission denied"), fmt.Errorf("exit 4")) + + err := EnsureNATEgressRules(context.Background(), r) + if err == nil || !strings.Contains(err.Error(), "permission denied") { + t.Fatalf("expected append failure error, got: %v", err) + } +} + +func TestRemoveNATEgressRules_IgnoresMissing(t *testing.T) { + r := newStubRunner() + r.expect("iptables -t nat -D", nil, fmt.Errorf("no match")) + r.expect("iptables -t filter -D", nil, nil) + + RemoveNATEgressRules(context.Background(), r) + if !r.called("iptables -t nat -D POSTROUTING") { + t.Errorf("expected POSTROUTING delete attempt, got %v", r.calls) + } +} diff --git a/spinifex/network/host/routed.go b/spinifex/network/host/routed.go new file mode 100644 index 000000000..a83726b1e --- /dev/null +++ b/spinifex/network/host/routed.go @@ -0,0 +1,104 @@ +package host + +import ( + "context" + "fmt" + "log/slog" + "net" + "net/netip" + "strings" +) + +// Transit segment for routed-NAT mode. RFC 6598 CGN space so it cannot +// collide with user VPC CIDRs (RFC 1918) or the operator's LAN. +const ( + NATTransitCIDR = "100.127.0.0/24" + NATTransitGatewayIP = "100.127.0.1" + NATTransitGatewayCIDR = "100.127.0.1/24" + + // NATTransitPoolName is the auto-generated external pool for the transit + // segment; nat mode may carry a public pool alongside it. + NATTransitPoolName = "nat-transit" + + // NATTransitHostEnd carries the transit gateway IP in the host stack. + NATTransitHostEnd = "spx-nat-host" + + // NATTransitOVSEnd is the veth peer attached to the OVS uplink bridge. + NATTransitOVSEnd = "spx-nat-ovs" +) + +// Routed implements Wiring for the routed-NAT model: no WAN NIC is bridged; +// br-ext reaches the host via the spx-nat veth pair and the host kernel +// forwards + masquerades the transit /24 out whatever uplink it has. +type Routed struct { + // UplinkBridge is the OVS bridge ovn-bridge-mappings targets (conventionally "br-ext"). + UplinkBridge string + + Runner Runner + Reader InterfaceReader +} + +var _ Wiring = (*Routed)(nil) + +func (r *Routed) runner() Runner { + if r.Runner != nil { + return r.Runner + } + return NewExecRunner() +} + +func (r *Routed) reader() InterfaceReader { + if r.Reader != nil { + return r.Reader + } + return NewKernelReader() +} + +// EnsureBridges ensures br-int and UplinkBridge exist. The transit veth pair +// is provisioned by setup-ovn.sh --nat-uplink. +func (r *Routed) EnsureBridges(ctx context.Context) error { + return ensureBridges(ctx, r.runner(), r.UplinkBridge) +} + +// EnsureUplinkPort verifies the transit veth wiring — OVS end on UplinkBridge, +// host end carrying the transit gateway IP — and returns the OVS-side MAC. +func (r *Routed) EnsureUplinkPort(ctx context.Context) (net.HardwareAddr, error) { + if r.UplinkBridge == "" { + return nil, fmt.Errorf("host.Routed: UplinkBridge required") + } + + out, err := r.runner().Run(ctx, "ovs-vsctl", "port-to-br", NATTransitOVSEnd) + if err != nil { + return nil, fmt.Errorf("port-to-br %q: not on OVS — run setup-ovn.sh --nat-uplink: %w", NATTransitOVSEnd, err) + } + br := strings.TrimSpace(string(out)) + if br != r.UplinkBridge { + return nil, fmt.Errorf("host.Routed: %q on bridge %q, expected %q", NATTransitOVSEnd, br, r.UplinkBridge) + } + + cidr, err := r.reader().BridgeCIDR(NATTransitHostEnd) + if err != nil { + return nil, fmt.Errorf("host.Routed: read transit CIDR on %q: %w", NATTransitHostEnd, err) + } + want := netip.MustParsePrefix(NATTransitGatewayCIDR) + if cidr != want { + return nil, fmt.Errorf("host.Routed: %q has %s, expected %s", NATTransitHostEnd, cidr, want) + } + + mac, err := r.reader().LinkMAC(NATTransitOVSEnd) + if err != nil { + return nil, fmt.Errorf("read MAC for %q: %w", NATTransitOVSEnd, err) + } + slog.Info("host.Routed: transit uplink verified", + "ovs_end", NATTransitOVSEnd, "host_end", NATTransitHostEnd, + "uplink_bridge", r.UplinkBridge, "transit", NATTransitGatewayCIDR, "mac", mac.String()) + return mac, nil +} + +// UplinkMode returns UplinkModeRouted (routed NAT). +func (r *Routed) UplinkMode() UplinkMode { return UplinkModeRouted } + +// ExternalCIDR returns the transit prefix on the host-side veth end. +func (r *Routed) ExternalCIDR(_ context.Context) (netip.Prefix, error) { + return r.reader().BridgeCIDR(NATTransitHostEnd) +} diff --git a/spinifex/network/host/routed_ingress.go b/spinifex/network/host/routed_ingress.go new file mode 100644 index 000000000..ad27d70c9 --- /dev/null +++ b/spinifex/network/host/routed_ingress.go @@ -0,0 +1,61 @@ +package host + +import ( + "context" + "fmt" + "log/slog" + "strings" +) + +// EnsureVPCIngressRoute installs the host route that makes a VPC's private IPs +// reachable in routed-NAT mode: traffic to the VPC CIDR enters OVN via the +// gateway LRP's transit IP on the spx-nat veth. Idempotent (route replace). +func EnsureVPCIngressRoute(ctx context.Context, r Runner, vpcCIDR, gwLrpIP string) error { + if vpcCIDR == "" { + return fmt.Errorf("EnsureVPCIngressRoute: vpcCIDR required") + } + if gwLrpIP == "" { + return fmt.Errorf("EnsureVPCIngressRoute: gwLrpIP required") + } + if out, err := r.Run(ctx, "ip", "route", "replace", vpcCIDR, + "via", gwLrpIP, "dev", NATTransitHostEnd); err != nil { + return fmt.Errorf("install VPC ingress route %s via %s: %s: %w", vpcCIDR, gwLrpIP, string(out), err) + } + slog.Info("VPC ingress route installed", "vpc_cidr", vpcCIDR, "gw_lrp_ip", gwLrpIP) + return nil +} + +// VPCIngressRouteVia returns the gateway IP of the existing transit-veth host +// route for a VPC CIDR, or "" when none exists. Lets callers detect a +// duplicate-CIDR collision (two VPCs sharing e.g. 172.31.0.0/16) before +// replacing or deleting a route another VPC installed. +func VPCIngressRouteVia(ctx context.Context, r Runner, vpcCIDR string) (string, error) { + if vpcCIDR == "" { + return "", fmt.Errorf("VPCIngressRouteVia: vpcCIDR required") + } + out, err := r.Run(ctx, "ip", "route", "show", vpcCIDR, "dev", NATTransitHostEnd) + if err != nil { + return "", fmt.Errorf("show VPC ingress route %s: %s: %w", vpcCIDR, string(out), err) + } + fields := strings.Fields(string(out)) + for i, f := range fields { + if f == "via" && i+1 < len(fields) { + return fields[i+1], nil + } + } + return "", nil +} + +// RemoveVPCIngressRoute deletes the host route for a VPC CIDR on IGW detach. +// A missing route is not an error (idempotent teardown). +func RemoveVPCIngressRoute(ctx context.Context, r Runner, vpcCIDR string) error { + if vpcCIDR == "" { + return fmt.Errorf("RemoveVPCIngressRoute: vpcCIDR required") + } + if _, err := r.Run(ctx, "ip", "route", "del", vpcCIDR, "dev", NATTransitHostEnd); err != nil { + slog.Debug("VPC ingress route already absent", "vpc_cidr", vpcCIDR, "err", err) + return nil + } + slog.Info("VPC ingress route removed", "vpc_cidr", vpcCIDR) + return nil +} diff --git a/spinifex/network/host/routed_ingress_test.go b/spinifex/network/host/routed_ingress_test.go new file mode 100644 index 000000000..cba947dfe --- /dev/null +++ b/spinifex/network/host/routed_ingress_test.go @@ -0,0 +1,106 @@ +package host + +import ( + "context" + "fmt" + "strings" + "testing" +) + +func TestEnsureVPCIngressRoute_InstallsRoute(t *testing.T) { + r := newStubRunner() + r.expect("ip route replace", nil, nil) + + if err := EnsureVPCIngressRoute(context.Background(), r, "10.0.0.0/16", "100.127.0.10"); err != nil { + t.Fatalf("EnsureVPCIngressRoute: %v", err) + } + want := "ip route replace 10.0.0.0/16 via 100.127.0.10 dev " + NATTransitHostEnd + if !r.called(want) { + t.Errorf("missing route call:\n want %q\n got %v", want, r.calls) + } +} + +func TestEnsureVPCIngressRoute_ValidatesArgs(t *testing.T) { + r := newStubRunner() + if err := EnsureVPCIngressRoute(context.Background(), r, "", "100.127.0.10"); err == nil { + t.Error("empty vpcCIDR: expected error") + } + if err := EnsureVPCIngressRoute(context.Background(), r, "10.0.0.0/16", ""); err == nil { + t.Error("empty gwLrpIP: expected error") + } + if len(r.calls) != 0 { + t.Errorf("validation failures must not run commands: %v", r.calls) + } +} + +func TestEnsureVPCIngressRoute_RouteFailure(t *testing.T) { + r := newStubRunner() + r.expect("ip route replace", []byte("RTNETLINK answers: no such device"), fmt.Errorf("exit 2")) + + err := EnsureVPCIngressRoute(context.Background(), r, "10.0.0.0/16", "100.127.0.10") + if err == nil || !strings.Contains(err.Error(), "no such device") { + t.Fatalf("expected route failure error, got: %v", err) + } +} + +func TestVPCIngressRouteVia_ParsesHolder(t *testing.T) { + r := newStubRunner() + r.expect("ip route show", []byte("172.31.0.0/16 via 100.127.0.239 dev spx-nat-host \n"), nil) + + via, err := VPCIngressRouteVia(context.Background(), r, "172.31.0.0/16") + if err != nil { + t.Fatalf("VPCIngressRouteVia: %v", err) + } + if via != "100.127.0.239" { + t.Errorf("via = %q, want 100.127.0.239", via) + } + want := "ip route show 172.31.0.0/16 dev " + NATTransitHostEnd + if !r.called(want) { + t.Errorf("missing route show call:\n want %q\n got %v", want, r.calls) + } +} + +func TestVPCIngressRouteVia_NoRoute(t *testing.T) { + r := newStubRunner() + r.expect("ip route show", nil, nil) + + via, err := VPCIngressRouteVia(context.Background(), r, "172.31.0.0/16") + if err != nil { + t.Fatalf("VPCIngressRouteVia: %v", err) + } + if via != "" { + t.Errorf("via = %q, want empty for absent route", via) + } +} + +func TestVPCIngressRouteVia_ValidatesArgs(t *testing.T) { + r := newStubRunner() + if _, err := VPCIngressRouteVia(context.Background(), r, ""); err == nil { + t.Error("empty vpcCIDR: expected error") + } + if len(r.calls) != 0 { + t.Errorf("validation failures must not run commands: %v", r.calls) + } +} + +func TestRemoveVPCIngressRoute_DeletesRoute(t *testing.T) { + r := newStubRunner() + r.expect("ip route del", nil, nil) + + if err := RemoveVPCIngressRoute(context.Background(), r, "10.0.0.0/16"); err != nil { + t.Fatalf("RemoveVPCIngressRoute: %v", err) + } + want := "ip route del 10.0.0.0/16 dev " + NATTransitHostEnd + if !r.called(want) { + t.Errorf("missing route del call:\n want %q\n got %v", want, r.calls) + } +} + +func TestRemoveVPCIngressRoute_MissingRouteNotError(t *testing.T) { + r := newStubRunner() + r.expect("ip route del", []byte("RTNETLINK answers: No such process"), fmt.Errorf("exit 2")) + + if err := RemoveVPCIngressRoute(context.Background(), r, "10.0.0.0/16"); err != nil { + t.Fatalf("missing route must not be an error, got: %v", err) + } +} diff --git a/spinifex/network/host/routed_test.go b/spinifex/network/host/routed_test.go new file mode 100644 index 000000000..a65761e8d --- /dev/null +++ b/spinifex/network/host/routed_test.go @@ -0,0 +1,74 @@ +package host + +import ( + "context" + "net/netip" + "strings" + "testing" +) + +func TestRouted_EnsureUplinkPort(t *testing.T) { + r := newStubRunner() + r.expect("ovs-vsctl port-to-br spx-nat-ovs", []byte("br-ext\n"), nil) + rd := newStubReader() + rd.macs[NATTransitOVSEnd] = mustMAC(t, "02:aa:bb:cc:dd:ee") + rd.cidrs[NATTransitHostEnd] = netip.MustParsePrefix(NATTransitGatewayCIDR) + + w := &Routed{UplinkBridge: "br-ext", Runner: r, Reader: rd} + mac, err := w.EnsureUplinkPort(context.Background()) + if err != nil { + t.Fatalf("EnsureUplinkPort: %v", err) + } + if mac.String() != "02:aa:bb:cc:dd:ee" { + t.Errorf("got MAC %q, want 02:aa:bb:cc:dd:ee", mac) + } + if w.UplinkMode() != UplinkModeRouted { + t.Errorf("UplinkMode = %v, want routed", w.UplinkMode()) + } +} + +func TestRouted_EnsureUplinkPort_WrongBridge(t *testing.T) { + r := newStubRunner() + r.expect("ovs-vsctl port-to-br spx-nat-ovs", []byte("br-other\n"), nil) + w := &Routed{UplinkBridge: "br-ext", Runner: r, Reader: newStubReader()} + _, err := w.EnsureUplinkPort(context.Background()) + if err == nil || !strings.Contains(err.Error(), "br-other") { + t.Fatalf("expected bridge mismatch error, got: %v", err) + } +} + +func TestRouted_EnsureUplinkPort_WrongTransitIP(t *testing.T) { + r := newStubRunner() + r.expect("ovs-vsctl port-to-br spx-nat-ovs", []byte("br-ext\n"), nil) + rd := newStubReader() + rd.cidrs[NATTransitHostEnd] = netip.MustParsePrefix("192.0.2.1/24") + w := &Routed{UplinkBridge: "br-ext", Runner: r, Reader: rd} + _, err := w.EnsureUplinkPort(context.Background()) + if err == nil || !strings.Contains(err.Error(), NATTransitGatewayCIDR) { + t.Fatalf("expected transit CIDR mismatch error, got: %v", err) + } +} + +func TestRouted_EnsureUplinkPort_MissingHostEnd(t *testing.T) { + r := newStubRunner() + r.expect("ovs-vsctl port-to-br spx-nat-ovs", []byte("br-ext\n"), nil) + w := &Routed{UplinkBridge: "br-ext", Runner: r, Reader: newStubReader()} + _, err := w.EnsureUplinkPort(context.Background()) + if err == nil || !strings.Contains(err.Error(), NATTransitHostEnd) { + t.Fatalf("expected missing host end error, got: %v", err) + } +} + +func TestRouted_ExternalCIDR(t *testing.T) { + rd := newStubReader() + want := netip.MustParsePrefix(NATTransitGatewayCIDR) + rd.cidrs[NATTransitHostEnd] = want + w := &Routed{UplinkBridge: "br-ext", Reader: rd} + got, err := w.ExternalCIDR(context.Background()) + if err != nil { + t.Fatalf("ExternalCIDR: %v", err) + } + if got != want { + t.Errorf("got %v, want %v", got, want) + } +} diff --git a/spinifex/network/ovn/client.go b/spinifex/network/ovn/client.go index a7433c816..955be4cae 100644 --- a/spinifex/network/ovn/client.go +++ b/spinifex/network/ovn/client.go @@ -16,6 +16,9 @@ var ErrNATNotFound = errors.New("NAT not found") // ErrPortGroupNotFound is returned when a port group is absent (use errors.Is for idempotency). var ErrPortGroupNotFound = errors.New("port group not found") +// ErrAddressSetNotFound is returned when an address set is absent (use errors.Is for idempotency). +var ErrAddressSetNotFound = errors.New("address set not found") + // ACLSpec describes an OVN ACL rule for attachment to a port group. type ACLSpec struct { Direction string // "to-lport" or "from-lport" @@ -95,6 +98,17 @@ type Client interface { DeleteAllNATsByExternalIP(ctx context.Context, natType, externalIP string) (int, error) FindNATByExternalIP(ctx context.Context, natType, externalIP string) (*nbdb.NAT, error) FindNATByLogicalIP(ctx context.Context, routerName, natType, logicalIP string) (*nbdb.NAT, error) + // SetNATExemptedExtIPs updates exempted_ext_ips in place on the NAT rule + // matching (natType, logicalIP) on routerName — no delete/re-add flow gap. + // nil clears the ref. Returns ErrNATNotFound when the rule is absent. + SetNATExemptedExtIPs(ctx context.Context, routerName, natType, logicalIP string, addressSetUUID *string) error + + // Address Sets (referenced by NAT exempted_ext_ips and ACL match exprs) + // EnsureAddressSet atomically creates the named address set or converges the + // addresses on the existing row (see EnsureLogicalRouter for the wait-op + // rationale). Returns the persisted row UUID. + EnsureAddressSet(ctx context.Context, name string, addresses []string) (uuid string, err error) + GetAddressSet(ctx context.Context, name string) (*nbdb.AddressSet, error) // Static routes AddStaticRoute(ctx context.Context, routerName string, route *nbdb.LogicalRouterStaticRoute) error diff --git a/spinifex/network/ovn/live.go b/spinifex/network/ovn/live.go index 6f11dae40..942bd3ba2 100644 --- a/spinifex/network/ovn/live.go +++ b/spinifex/network/ovn/live.go @@ -1059,6 +1059,27 @@ func (c *LiveClient) FindNATByLogicalIP(ctx context.Context, routerName, natType return &nats[0], nil } +// SetNATExemptedExtIPs updates exempted_ext_ips in place on the NAT rule +// matching (natType, logicalIP) on routerName. nil clears the ref. +func (c *LiveClient) SetNATExemptedExtIPs(ctx context.Context, routerName, natType, logicalIP string, addressSetUUID *string) error { + nat, err := c.FindNATByLogicalIP(ctx, routerName, natType, logicalIP) + if err != nil { + return fmt.Errorf("set NAT exempted_ext_ips lookup: %w", err) + } + if nat == nil { + return fmt.Errorf("NAT %s %s on %s: %w", natType, logicalIP, routerName, ErrNATNotFound) + } + nat.ExemptedExtIps = addressSetUUID + ops, err := c.client.Where(nat).Update(nat, &nat.ExemptedExtIps) + if err != nil { + return fmt.Errorf("set NAT exempted_ext_ips ops: %w", err) + } + if err := c.transactOps(ctx, ops); err != nil { + return fmt.Errorf("set NAT exempted_ext_ips transact: %w", err) + } + return nil +} + func (c *LiveClient) AddStaticRoute(ctx context.Context, routerName string, route *nbdb.LogicalRouterStaticRoute) error { if route.UUID == "" { route.UUID = namedUUID("route_", route.IPPrefix) @@ -1119,9 +1140,23 @@ func (c *LiveClient) FindStaticRoute(ctx context.Context, routerName, ipPrefix s } func (c *LiveClient) DeleteStaticRoute(ctx context.Context, routerName string, ipPrefix string) error { + lr, err := c.GetLogicalRouter(ctx, routerName) + if err != nil { + return fmt.Errorf("get logical router for route delete: %w", err) + } + // Scope the prefix match to this router's rows — every VPC router carries + // 0.0.0.0/0, so an unscoped cache match can grab another router's route. + owned := make(map[string]struct{}, len(lr.StaticRoutes)) + for _, u := range lr.StaticRoutes { + owned[u] = struct{}{} + } var routes []nbdb.LogicalRouterStaticRoute - err := c.client.WhereCache(func(r *nbdb.LogicalRouterStaticRoute) bool { - return r.IPPrefix == ipPrefix + err = c.client.WhereCache(func(r *nbdb.LogicalRouterStaticRoute) bool { + if r.IPPrefix != ipPrefix { + return false + } + _, ok := owned[r.UUID] + return ok }).List(ctx, &routes) if err != nil { return fmt.Errorf("find static route: %w", err) @@ -1131,10 +1166,6 @@ func (c *LiveClient) DeleteStaticRoute(ctx context.Context, routerName string, i } route := &routes[0] - lr, err := c.GetLogicalRouter(ctx, routerName) - if err != nil { - return fmt.Errorf("get logical router for route delete: %w", err) - } mutateOps, err := c.client.Where(lr).Mutate(lr, model.Mutation{ Field: &lr.StaticRoutes, @@ -1433,6 +1464,97 @@ func (c *LiveClient) ListPortGroups(ctx context.Context) ([]nbdb.PortGroup, erro return pgs, nil } +// EnsureAddressSet atomically creates the named address set or converges the +// addresses on the existing row. Wait-op serialises concurrent writers (NB has +// no unique-Name constraint). Returns the persisted row UUID. +func (c *LiveClient) EnsureAddressSet(ctx context.Context, name string, addresses []string) (string, error) { + if existing, err := c.getAddressSet(ctx, name); err == nil { + return existing.UUID, c.convergeAddressSet(ctx, existing, addresses) + } + as := &nbdb.AddressSet{ + UUID: namedUUID("as_", name), + Name: name, + Addresses: addresses, + ExternalIDs: map[string]string{}, + } + ops, err := c.ensureNamedRowOps("Address_Set", name, as) + if err != nil { + return "", err + } + if err := c.transactOps(ctx, ops); err != nil { + if existing, refErr := c.waitForCachedAddressSet(ctx, name); refErr == nil { + slog.Info("ovn: EnsureAddressSet lost race, reusing existing", + "name", name, "existing_uuid", existing.UUID) + return existing.UUID, c.convergeAddressSet(ctx, existing, addresses) + } + return "", fmt.Errorf("ensure address set %q: %w", name, err) + } + created, refErr := c.waitForCachedAddressSet(ctx, name) + if refErr != nil { + return "", fmt.Errorf("resolve address set %q uuid after insert: %w", name, refErr) + } + return created.UUID, nil +} + +// convergeAddressSet updates the row's addresses when they differ from want. +func (c *LiveClient) convergeAddressSet(ctx context.Context, existing *nbdb.AddressSet, want []string) error { + have := slices.Clone(existing.Addresses) + wantSorted := slices.Clone(want) + slices.Sort(have) + slices.Sort(wantSorted) + if slices.Equal(have, wantSorted) { + return nil + } + existing.Addresses = want + ops, err := c.client.Where(existing).Update(existing, &existing.Addresses) + if err != nil { + return fmt.Errorf("converge address set %q ops: %w", existing.Name, err) + } + if err := c.transactOps(ctx, ops); err != nil { + return fmt.Errorf("converge address set %q transact: %w", existing.Name, err) + } + return nil +} + +func (c *LiveClient) waitForCachedAddressSet(ctx context.Context, name string) (*nbdb.AddressSet, error) { + deadline := time.Now().Add(ensureRefetchTimeout) + var lastErr error + for { + v, err := c.getAddressSet(ctx, name) + if err == nil { + return v, nil + } + lastErr = err + if time.Now().After(deadline) { + return nil, lastErr + } + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-time.After(ensureRefetchInterval): + } + } +} + +// GetAddressSet returns the named address set. +func (c *LiveClient) GetAddressSet(ctx context.Context, name string) (*nbdb.AddressSet, error) { + return c.getAddressSet(ctx, name) +} + +func (c *LiveClient) getAddressSet(ctx context.Context, name string) (*nbdb.AddressSet, error) { + var sets []nbdb.AddressSet + err := c.client.WhereCache(func(as *nbdb.AddressSet) bool { + return as.Name == name + }).List(ctx, &sets) + if err != nil { + return nil, fmt.Errorf("get address set: %w", err) + } + if len(sets) == 0 { + return nil, fmt.Errorf("%w: %q", ErrAddressSetNotFound, name) + } + return &sets[0], nil +} + // ListPortGroupsForPort returns port group names containing the LSP. Empty (not error) when none. func (c *LiveClient) ListPortGroupsForPort(ctx context.Context, lspName string) ([]string, error) { lsp, err := c.GetLogicalSwitchPort(ctx, lspName) diff --git a/spinifex/network/ovn/live_ensure_test.go b/spinifex/network/ovn/live_ensure_test.go index 136342cbd..85565721f 100644 --- a/spinifex/network/ovn/live_ensure_test.go +++ b/spinifex/network/ovn/live_ensure_test.go @@ -2,6 +2,7 @@ package ovn_test import ( "context" + "errors" "testing" "time" @@ -136,3 +137,80 @@ func equalStringSet(a, b map[string]struct{}) bool { } return true } + +// EnsureAddressSet must return the persisted UUID on insert, keep it stable on +// re-ensure, and converge the address list. +func TestEnsureAddressSet_InsertAndConverge(t *testing.T) { + cli, ctx := liveClient(t) + + uuid1, err := cli.EnsureAddressSet(ctx, "spinifex_nat_exempt", []string{"100.127.0.0/24"}) + if err != nil { + t.Fatalf("EnsureAddressSet #1: %v", err) + } + if uuid1 == "" || uuid1 == "as_spinifex_nat_exempt" { + t.Fatalf("insert returned non-persisted UUID %q", uuid1) + } + + uuid2, err := cli.EnsureAddressSet(ctx, "spinifex_nat_exempt", []string{"100.127.0.0/24", "192.168.1.0/24"}) + if err != nil { + t.Fatalf("EnsureAddressSet #2: %v", err) + } + if uuid2 != uuid1 { + t.Fatalf("UUID changed on re-ensure: %q → %q", uuid1, uuid2) + } + + as, err := cli.GetAddressSet(ctx, "spinifex_nat_exempt") + if err != nil { + t.Fatalf("GetAddressSet: %v", err) + } + if len(as.Addresses) != 2 { + t.Fatalf("addresses not converged: %v", as.Addresses) + } + + if _, err := cli.GetAddressSet(ctx, "no-such-set"); !errors.Is(err, ovn.ErrAddressSetNotFound) { + t.Fatalf("missing set: err = %v, want ErrAddressSetNotFound", err) + } +} + +// SetNATExemptedExtIPs must stamp/clear the strong Address_Set ref in place on +// the matching NAT row and return ErrNATNotFound for absent rules. +func TestSetNATExemptedExtIPs_Live(t *testing.T) { + cli, ctx := liveClient(t) + + if _, _, err := cli.EnsureLogicalRouter(ctx, &nbdb.LogicalRouter{Name: "vpc-r1"}); err != nil { + t.Fatalf("EnsureLogicalRouter: %v", err) + } + if err := cli.AddNAT(ctx, "vpc-r1", &nbdb.NAT{ + Type: "snat", ExternalIP: "100.127.0.10", LogicalIP: "10.0.0.0/16", + }); err != nil { + t.Fatalf("AddNAT: %v", err) + } + setUUID, err := cli.EnsureAddressSet(ctx, "spinifex_nat_exempt", []string{"100.127.0.0/24"}) + if err != nil { + t.Fatalf("EnsureAddressSet: %v", err) + } + + if err := cli.SetNATExemptedExtIPs(ctx, "vpc-r1", "snat", "10.0.0.0/16", &setUUID); err != nil { + t.Fatalf("SetNATExemptedExtIPs: %v", err) + } + nat, err := cli.FindNATByLogicalIP(ctx, "vpc-r1", "snat", "10.0.0.0/16") + if err != nil || nat == nil { + t.Fatalf("FindNATByLogicalIP: nat=%v err=%v", nat, err) + } + if nat.ExemptedExtIps == nil || *nat.ExemptedExtIps != setUUID { + t.Fatalf("ExemptedExtIps = %v, want %q", nat.ExemptedExtIps, setUUID) + } + + if err := cli.SetNATExemptedExtIPs(ctx, "vpc-r1", "snat", "10.0.0.0/16", nil); err != nil { + t.Fatalf("clear: %v", err) + } + nat, _ = cli.FindNATByLogicalIP(ctx, "vpc-r1", "snat", "10.0.0.0/16") + if nat.ExemptedExtIps != nil { + t.Fatalf("ExemptedExtIps not cleared: %v", *nat.ExemptedExtIps) + } + + err = cli.SetNATExemptedExtIPs(ctx, "vpc-r1", "snat", "10.99.0.0/16", &setUUID) + if !errors.Is(err, ovn.ErrNATNotFound) { + t.Fatalf("missing NAT: err = %v, want ErrNATNotFound", err) + } +} diff --git a/spinifex/network/ovn/mock/mock.go b/spinifex/network/ovn/mock/mock.go index d4a2829f7..dcda803e3 100644 --- a/spinifex/network/ovn/mock/mock.go +++ b/spinifex/network/ovn/mock/mock.go @@ -29,6 +29,7 @@ type Client struct { PortGroups map[string]*nbdb.PortGroup // keyed by name ACLs map[string]*nbdb.ACL // keyed by UUID GatewayChassis map[string]*nbdb.GatewayChassis // keyed by UUID + AddressSets map[string]*nbdb.AddressSet // keyed by name // Call counters so tests can assert idempotent / drift-only write paths. UpdateLogicalSwitchPortCalls int @@ -62,6 +63,7 @@ func New() *Client { PortGroups: make(map[string]*nbdb.PortGroup), ACLs: make(map[string]*nbdb.ACL), GatewayChassis: make(map[string]*nbdb.GatewayChassis), + AddressSets: make(map[string]*nbdb.AddressSet), } } @@ -622,6 +624,27 @@ func (m *Client) FindNATByLogicalIP(_ context.Context, routerName, natType, logi return nil, nil } +// SetNATExemptedExtIPs updates exempted_ext_ips in place on the matching NAT row. +func (m *Client) SetNATExemptedExtIPs(_ context.Context, routerName, natType, logicalIP string, addressSetUUID *string) error { + m.Mu.Lock() + defer m.Mu.Unlock() + lr, exists := m.Routers[routerName] + if !exists { + return fmt.Errorf("logical router %q not found", routerName) + } + for _, uuid := range lr.NAT { + n, ok := m.NATs[uuid] + if !ok { + continue + } + if n.Type == natType && n.LogicalIP == logicalIP { + n.ExemptedExtIps = addressSetUUID + return nil + } + } + return fmt.Errorf("NAT %s %s on %s: %w", natType, logicalIP, routerName, ovn.ErrNATNotFound) +} + // Static Routes func (m *Client) AddStaticRoute(_ context.Context, routerName string, route *nbdb.LogicalRouterStaticRoute) error { @@ -667,9 +690,11 @@ func (m *Client) DeleteStaticRoute(_ context.Context, routerName string, ipPrefi if !exists { return fmt.Errorf("logical router %q not found", routerName) } + // Scope the prefix match to this router's rows — every VPC router carries + // 0.0.0.0/0, so an unscoped match can grab another router's route. var foundUUID string - for uuid, r := range m.StaticRoutes { - if r.IPPrefix == ipPrefix { + for _, uuid := range lr.StaticRoutes { + if r, ok := m.StaticRoutes[uuid]; ok && r.IPPrefix == ipPrefix { foundUUID = uuid break } @@ -895,6 +920,36 @@ func (m *Client) ListPortGroups(_ context.Context) ([]nbdb.PortGroup, error) { return out, nil } +// EnsureAddressSet mirrors the live client's wait-op semantics: creates the +// named set or converges the addresses on the existing row. +func (m *Client) EnsureAddressSet(_ context.Context, name string, addresses []string) (string, error) { + m.Mu.Lock() + defer m.Mu.Unlock() + if existing, ok := m.AddressSets[name]; ok { + existing.Addresses = slices.Clone(addresses) + return existing.UUID, nil + } + as := &nbdb.AddressSet{ + UUID: utils.GenerateResourceID("as"), + Name: name, + Addresses: slices.Clone(addresses), + } + m.AddressSets[name] = as + return as.UUID, nil +} + +// GetAddressSet returns the address set, or ovn.ErrAddressSetNotFound. +func (m *Client) GetAddressSet(_ context.Context, name string) (*nbdb.AddressSet, error) { + m.Mu.Lock() + defer m.Mu.Unlock() + as, exists := m.AddressSets[name] + if !exists { + return nil, fmt.Errorf("%w: %q", ovn.ErrAddressSetNotFound, name) + } + result := *as + return &result, nil +} + // ACLs func (m *Client) AddACLs(_ context.Context, portGroupName string, specs []ovn.ACLSpec) error { diff --git a/spinifex/network/ovn/mock/mock_test.go b/spinifex/network/ovn/mock/mock_test.go index 058fdd50e..046176845 100644 --- a/spinifex/network/ovn/mock/mock_test.go +++ b/spinifex/network/ovn/mock/mock_test.go @@ -2,6 +2,7 @@ package mock import ( "context" + "errors" "sync" "testing" @@ -469,3 +470,87 @@ func TestLogicalRouterPolicies_DanglingUUIDsIgnored(t *testing.T) { t.Fatalf("expected 0 policies, got %d", len(policies)) } } + +// EnsureAddressSet must create on first call, keep the UUID stable, and +// converge addresses on subsequent calls (mirrors LiveClient). +func TestEnsureAddressSet_IdempotentConverge(t *testing.T) { + m := New() + _ = m.Connect(context.Background()) + ctx := context.Background() + + uuid1, err := m.EnsureAddressSet(ctx, "spinifex_nat_exempt", []string{"100.127.0.0/24"}) + if err != nil { + t.Fatalf("EnsureAddressSet #1: %v", err) + } + if uuid1 == "" { + t.Fatal("EnsureAddressSet returned empty UUID") + } + + uuid2, err := m.EnsureAddressSet(ctx, "spinifex_nat_exempt", []string{"100.127.0.0/24", "192.168.1.0/24"}) + if err != nil { + t.Fatalf("EnsureAddressSet #2: %v", err) + } + if uuid2 != uuid1 { + t.Fatalf("UUID changed on re-ensure: %q → %q", uuid1, uuid2) + } + + as, err := m.GetAddressSet(ctx, "spinifex_nat_exempt") + if err != nil { + t.Fatalf("GetAddressSet: %v", err) + } + if len(as.Addresses) != 2 || as.Addresses[0] != "100.127.0.0/24" || as.Addresses[1] != "192.168.1.0/24" { + t.Fatalf("addresses not converged: %v", as.Addresses) + } +} + +func TestGetAddressSet_NotFound(t *testing.T) { + m := New() + _ = m.Connect(context.Background()) + + _, err := m.GetAddressSet(context.Background(), "no-such-set") + if !errors.Is(err, ovn.ErrAddressSetNotFound) { + t.Fatalf("err = %v, want ErrAddressSetNotFound", err) + } +} + +// SetNATExemptedExtIPs must mutate the stored NAT row in place, clear on nil, +// and return ErrNATNotFound when no rule matches. +func TestSetNATExemptedExtIPs(t *testing.T) { + m := New() + _ = m.Connect(context.Background()) + ctx := context.Background() + + if err := m.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{Name: "vpc-r1"}); err != nil { + t.Fatalf("CreateLogicalRouter: %v", err) + } + if err := m.AddNAT(ctx, "vpc-r1", &nbdb.NAT{ + Type: "snat", ExternalIP: "100.127.0.10", LogicalIP: "10.0.0.0/16", + }); err != nil { + t.Fatalf("AddNAT: %v", err) + } + + setUUID := "as-1234" + if err := m.SetNATExemptedExtIPs(ctx, "vpc-r1", "snat", "10.0.0.0/16", &setUUID); err != nil { + t.Fatalf("SetNATExemptedExtIPs: %v", err) + } + nat, err := m.FindNATByLogicalIP(ctx, "vpc-r1", "snat", "10.0.0.0/16") + if err != nil || nat == nil { + t.Fatalf("FindNATByLogicalIP: nat=%v err=%v", nat, err) + } + if nat.ExemptedExtIps == nil || *nat.ExemptedExtIps != setUUID { + t.Fatalf("ExemptedExtIps = %v, want %q", nat.ExemptedExtIps, setUUID) + } + + if err := m.SetNATExemptedExtIPs(ctx, "vpc-r1", "snat", "10.0.0.0/16", nil); err != nil { + t.Fatalf("clear: %v", err) + } + nat, _ = m.FindNATByLogicalIP(ctx, "vpc-r1", "snat", "10.0.0.0/16") + if nat.ExemptedExtIps != nil { + t.Fatalf("ExemptedExtIps not cleared: %v", *nat.ExemptedExtIps) + } + + err = m.SetNATExemptedExtIPs(ctx, "vpc-r1", "snat", "10.99.0.0/16", &setUUID) + if !errors.Is(err, ovn.ErrNATNotFound) { + t.Fatalf("missing NAT: err = %v, want ErrNATNotFound", err) + } +} diff --git a/spinifex/network/ovn/nbdb/model.go b/spinifex/network/ovn/nbdb/model.go index 9a922c283..b1baa50fa 100644 --- a/spinifex/network/ovn/nbdb/model.go +++ b/spinifex/network/ovn/nbdb/model.go @@ -63,14 +63,17 @@ type DHCPOptions struct { // NAT represents an OVN NAT rule on a Logical_Router. type NAT struct { - UUID string `ovsdb:"_uuid"` - Type string `ovsdb:"type"` // "snat", "dnat", "dnat_and_snat" - ExternalIP string `ovsdb:"external_ip"` - LogicalIP string `ovsdb:"logical_ip"` - LogicalPort *string `ovsdb:"logical_port"` - ExternalMAC *string `ovsdb:"external_mac"` - ExternalIDs map[string]string `ovsdb:"external_ids"` - Options map[string]string `ovsdb:"options"` + UUID string `ovsdb:"_uuid"` + Type string `ovsdb:"type"` // "snat", "dnat", "dnat_and_snat" + ExternalIP string `ovsdb:"external_ip"` + LogicalIP string `ovsdb:"logical_ip"` + LogicalPort *string `ovsdb:"logical_port"` + ExternalMAC *string `ovsdb:"external_mac"` + // ExemptedExtIps references an Address_Set (strong ref); packets whose + // destination (snat) / source (dnat) matches the set skip this NAT rule. + ExemptedExtIps *string `ovsdb:"exempted_ext_ips"` + ExternalIDs map[string]string `ovsdb:"external_ids"` + Options map[string]string `ovsdb:"options"` } // LogicalRouterStaticRoute represents an OVN Logical_Router_Static_Route. diff --git a/spinifex/network/ovn/ovntest/ovn-nb.ovsschema b/spinifex/network/ovn/ovntest/ovn-nb.ovsschema index d48e517cf..b7ecb144c 100644 --- a/spinifex/network/ovn/ovntest/ovn-nb.ovsschema +++ b/spinifex/network/ovn/ovntest/ovn-nb.ovsschema @@ -71,6 +71,7 @@ "logical_ip": {"type": "string"}, "logical_port": {"type": {"key": "string", "min": 0, "max": 1}}, "external_mac": {"type": {"key": "string", "min": 0, "max": 1}}, + "exempted_ext_ips": {"type": {"key": {"type": "uuid", "refTable": "Address_Set", "refType": "strong"}, "min": 0, "max": 1}}, "external_ids": {"type": {"key": "string", "value": "string", "min": 0, "max": "unlimited"}}, "options": {"type": {"key": "string", "value": "string", "min": 0, "max": "unlimited"}} } diff --git a/spinifex/network/policy/nat.go b/spinifex/network/policy/nat.go index e5f5938e8..8904d5907 100644 --- a/spinifex/network/policy/nat.go +++ b/spinifex/network/policy/nat.go @@ -5,12 +5,17 @@ import ( "errors" "fmt" "log/slog" + "net/netip" "github.com/mulgadc/spinifex/spinifex/network/ovn" "github.com/mulgadc/spinifex/spinifex/network/ovn/nbdb" "github.com/mulgadc/spinifex/spinifex/network/topology" ) +// GatewayIPExtIDKey is the gateway LRP external_ids key carrying the IP +// allocated at IGW attach; the routed-mode next hop for host EIP routes. +const GatewayIPExtIDKey = "spinifex:gateway_ip" + // EIPSpec is a 1:1 EIP NAT (dnat_and_snat). In NATModeDistributed PortName // and MAC MUST be set for per-chassis flows; missing values fall back to // centralised shape (hairpins via gateway chassis). @@ -112,12 +117,51 @@ func WithNeighPrimer(p NeighPrimer) Option { } } +// HostEIPBinder plumbs routed-mode host state for an EIP: the /32 route into +// OVN via the gateway LRP and proxy-ARP on the uplink. Bind fires on every +// AddEIP (fresh and idempotent) so reconcile re-ensures host state after +// reboot; Unbind fires on DeleteEIP. +type HostEIPBinder struct { + Bind func(eip EIPSpec, gwLrpIP string) error + Unbind func(externalIP string) error +} + +// WithHostEIPBinder injects the routed-mode host plumbing hooks fired on EIP +// attach/detach. Only consulted in NATModeRouted. +func WithHostEIPBinder(b HostEIPBinder) Option { + return func(m *natManager) { + if b.Bind != nil && b.Unbind != nil { + m.hostBinder = &b + } + } +} + +// NATExemptSetName is the singleton Address_Set holding destinations that +// skip routed-mode NAT (transit /24 plus operator extras). +const NATExemptSetName = "spinifex_nat_exempt" + +// WithSNATExemptSet names an Address_Set whose CIDRs skip NAT (stamped as +// exempted_ext_ips on SNAT/EIP rows). Routed mode uses it so VM replies to +// host-initiated flows keep their private source. The set is never deleted: +// it is a singleton strongly referenced by every routed NAT row. +func WithSNATExemptSet(setName string, cidrs []string) Option { + return func(m *natManager) { + if setName != "" { + m.exemptSetName = setName + m.exemptCIDRs = cidrs + } + } +} + type natManager struct { - ovn ovn.Client - mode NATMode - barrier FlowsBarrier - neigh NeighFlusher - neighPrime NeighPrimer + ovn ovn.Client + mode NATMode + barrier FlowsBarrier + neigh NeighFlusher + neighPrime NeighPrimer + exemptSetName string + exemptCIDRs []string + hostBinder *HostEIPBinder } var _ NATManager = (*natManager)(nil) @@ -141,9 +185,28 @@ func NewNATManager(client ovn.Client, mode NATMode, opts ...Option) (NATManager, return m, nil } +// exemptSetUUID ensures the configured exempt Address_Set exists and returns +// its UUID for stamping as exempted_ext_ips. Nil when the option is unset or +// the mode is not routed (only routed SNAT breaks host-initiated return paths). +func (m *natManager) exemptSetUUID(ctx context.Context) (*string, error) { + if m.mode != NATModeRouted || m.exemptSetName == "" { + return nil, nil + } + uuid, err := m.ovn.EnsureAddressSet(ctx, m.exemptSetName, m.exemptCIDRs) + if err != nil { + return nil, fmt.Errorf("ensure NAT exempt set %q: %w", m.exemptSetName, err) + } + return &uuid, nil +} + func (m *natManager) AddEIP(ctx context.Context, eip EIPSpec) error { router := topology.VPCRouter(eip.VPCID) + exemptUUID, err := m.exemptSetUUID(ctx) + if err != nil { + return err + } + natRule := &nbdb.NAT{ Type: "dnat_and_snat", ExternalIP: eip.ExternalIP, @@ -159,6 +222,7 @@ func (m *natManager) AddEIP(ctx context.Context, eip EIPSpec) error { if eip.PortName != "" { natRule.ExternalIDs["spinifex:logical_port"] = eip.PortName } + natRule.ExemptedExtIps = exemptUUID distributed := m.mode == NATModeDistributed && eip.PortName != "" && eip.MAC != "" if distributed { mac := eip.MAC @@ -176,12 +240,23 @@ func (m *natManager) AddEIP(ctx context.Context, eip EIPSpec) error { (!distributed || (existing.ExternalMAC != nil && *existing.ExternalMAC == eip.MAC && existing.LogicalPort != nil && *existing.LogicalPort == eip.PortName)) { + // Upgrade path: stamp the exempt ref in place on rows minted before the + // option existed (or after a set re-create). + if exemptUUID != nil && (existing.ExemptedExtIps == nil || *existing.ExemptedExtIps != *exemptUUID) { + if err := m.ovn.SetNATExemptedExtIPs(ctx, router, "dnat_and_snat", eip.LogicalIP, exemptUUID); err != nil { + return fmt.Errorf("patch exempt set on dnat_and_snat %s on %s: %w", eip.ExternalIP, router, err) + } + slog.Info("policy: AddEIP patched exempt set on existing rule", + "router", router, "external_ip", eip.ExternalIP, "logical_ip", eip.LogicalIP) + } slog.Info("policy: AddEIP idempotent skip — rule current, re-priming reachability", "router", router, "external_ip", eip.ExternalIP, "logical_ip", eip.LogicalIP) // Skip row churn but still re-prime: stop->start re-attaches the same EIP // and the host neigh stays dark until ARP times out without a fresh prime. m.primeReachability(ctx, eip, distributed) - return nil + // Host state (routes, proxy-ARP) is volatile even when the OVN row + // survives — reconcile lands here after a reboot, so re-bind. + return m.bindHostEIP(ctx, eip) } // Search every router for stale rules — vpc.delete-nat is fire-and-forget. @@ -198,6 +273,23 @@ func (m *natManager) AddEIP(ctx context.Context, eip EIPSpec) error { slog.Warn("policy: AddEIP flows barrier failed", "external_ip", eip.ExternalIP, "logical_ip", eip.LogicalIP, "err", err) } m.primeReachability(ctx, eip, distributed) + return m.bindHostEIP(ctx, eip) +} + +// bindHostEIP fires the routed-mode host plumbing hook for an EIP. No-op in +// other modes or when no binder is configured. Errors are returned so a +// half-plumbed EIP surfaces to the caller (reconcile retries the bind). +func (m *natManager) bindHostEIP(ctx context.Context, eip EIPSpec) error { + if m.mode != NATModeRouted || m.hostBinder == nil { + return nil + } + gwLrpIP := m.gatewayPortIP(ctx, eip.VPCID) + if gwLrpIP == "" { + return fmt.Errorf("bind host EIP %s: gateway LRP IP unknown for %s (IGW attached?)", eip.ExternalIP, eip.VPCID) + } + if err := m.hostBinder.Bind(eip, gwLrpIP); err != nil { + return fmt.Errorf("bind host EIP %s via %s: %w", eip.ExternalIP, gwLrpIP, err) + } return nil } @@ -233,6 +325,25 @@ func (m *natManager) gatewayPortMAC(ctx context.Context, vpcID string) string { return lrp.MAC } +// gatewayPortIP returns the IP of the VPC gateway router's external port — the +// transit-side next hop for routed-mode host EIP routes. Prefers the IP stamped +// at IGW attach; falls back to the LRP network address. Empty on lookup miss. +func (m *natManager) gatewayPortIP(ctx context.Context, vpcID string) string { + lrp, err := m.ovn.GetLogicalRouterPort(ctx, topology.GatewayRouterPort(vpcID)) + if err != nil || lrp == nil { + return "" + } + if ip := lrp.ExternalIDs[GatewayIPExtIDKey]; ip != "" { + return ip + } + if len(lrp.Networks) > 0 { + if pfx, err := netip.ParsePrefix(lrp.Networks[0]); err == nil { + return pfx.Addr().String() + } + } + return "" +} + func (m *natManager) DeleteEIP(ctx context.Context, vpcID, externalIP, logicalIP, portName string) error { router := topology.VPCRouter(vpcID) // An empty external IP means there is no EIP and therefore no dnat_and_snat @@ -277,6 +388,13 @@ func (m *natManager) DeleteEIP(ctx context.Context, vpcID, externalIP, logicalIP if err := m.neigh(externalIP); err != nil { slog.Warn("policy: DeleteEIP neighbour flush failed", "external_ip", externalIP, "logical_ip", logicalIP, "err", err) } + // Tear down routed-mode host plumbing. Best-effort: the row is already gone + // and stale host state is re-converged by the next bind of the same IP. + if m.mode == NATModeRouted && m.hostBinder != nil { + if err := m.hostBinder.Unbind(externalIP); err != nil { + slog.Warn("policy: DeleteEIP host unbind failed", "external_ip", externalIP, "err", err) + } + } return nil } @@ -336,10 +454,45 @@ func (m *natManager) DeleteNATGateway(ctx context.Context, vpcID, subnetCIDR str func (m *natManager) AddSNAT(ctx context.Context, vpcID, vpcCIDR, externalIP string) error { router := topology.VPCRouter(vpcID) + + exemptUUID, err := m.exemptSetUUID(ctx) + if err != nil { + return err + } + + // Reconcile replays IGW attach every pass; key on (router, vpcCIDR) so the + // re-publish is a no-op instead of minting duplicate snat rows. + if existing, err := m.ovn.FindNATByLogicalIP(ctx, router, "snat", vpcCIDR); err != nil { + slog.Warn("policy: AddSNAT idempotency lookup failed", "vpc_cidr", vpcCIDR, "err", err) + } else if existing != nil && existing.ExternalIP == externalIP { + // Upgrade path: stamp the exempt ref in place on rows minted before the + // option existed (or after a set re-create). + if exemptUUID != nil && (existing.ExemptedExtIps == nil || *existing.ExemptedExtIps != *exemptUUID) { + if err := m.ovn.SetNATExemptedExtIPs(ctx, router, "snat", vpcCIDR, exemptUUID); err != nil { + return fmt.Errorf("patch exempt set on IGW snat %s on %s: %w", vpcCIDR, router, err) + } + slog.Info("policy: AddSNAT patched exempt set on existing rule", + "router", router, "vpc_cidr", vpcCIDR, "external_ip", externalIP) + return nil + } + slog.Info("policy: AddSNAT idempotent skip — rule already current", + "router", router, "vpc_cidr", vpcCIDR, "external_ip", externalIP) + return nil + } else if existing != nil { + // Same VPC CIDR, different external IP (gateway transit IP changed). + // Scrub the stale row so egress does not leak via the old IP. + slog.Info("policy: AddSNAT replacing stale snat — external IP changed", + "router", router, "old_ip", existing.ExternalIP, "new_ip", externalIP, "vpc_cidr", vpcCIDR) + if err := m.ovn.DeleteNAT(ctx, router, "snat", vpcCIDR); err != nil && !errors.Is(err, ovn.ErrNATNotFound) { + return fmt.Errorf("replace stale IGW snat %s on %s: %w", vpcCIDR, router, err) + } + } + snatRule := &nbdb.NAT{ - Type: "snat", - ExternalIP: externalIP, - LogicalIP: vpcCIDR, + Type: "snat", + ExternalIP: externalIP, + LogicalIP: vpcCIDR, + ExemptedExtIps: exemptUUID, ExternalIDs: map[string]string{ "spinifex:vpc_id": vpcID, "spinifex:role": "igw-default-snat", diff --git a/spinifex/network/policy/nat_binder_test.go b/spinifex/network/policy/nat_binder_test.go new file mode 100644 index 000000000..7f5d1e49e --- /dev/null +++ b/spinifex/network/policy/nat_binder_test.go @@ -0,0 +1,155 @@ +package policy + +import ( + "context" + "fmt" + "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/ovn/nbdb" + "github.com/mulgadc/spinifex/spinifex/network/topology" +) + +type recordedBinder struct { + binds []string + unbinds []string + bindErr error +} + +func (b *recordedBinder) hooks() HostEIPBinder { + return HostEIPBinder{ + Bind: func(eip EIPSpec, gwLrpIP string) error { + b.binds = append(b.binds, eip.ExternalIP+" via "+gwLrpIP) + return b.bindErr + }, + Unbind: func(externalIP string) error { + b.unbinds = append(b.unbinds, externalIP) + return nil + }, + } +} + +func seedGatewayPortIP(t *testing.T, m *mock.Client, vpcID, gwIP string, networks []string) { + t.Helper() + extIDs := map[string]string{} + if gwIP != "" { + extIDs[GatewayIPExtIDKey] = gwIP + } + require.NoError(t, m.CreateLogicalRouterPort(context.Background(), + topology.VPCRouter(vpcID), + &nbdb.LogicalRouterPort{ + Name: topology.GatewayRouterPort(vpcID), + MAC: "02:00:00:00:00:01", + Networks: networks, + ExternalIDs: extIDs, + })) +} + +func TestNATManager_AddEIP_Routed_BindsHostEIP(t *testing.T) { + m := mock.New() + seedRouter(t, m, "vpc-1") + seedGatewayPortIP(t, m, "vpc-1", "100.127.0.10", nil) + b := &recordedBinder{} + mgr, err := NewNATManager(m, NATModeRouted, WithHostEIPBinder(b.hooks())) + require.NoError(t, err) + + require.NoError(t, mgr.AddEIP(context.Background(), EIPSpec{ + VPCID: "vpc-1", ExternalIP: "192.168.1.200", LogicalIP: "10.0.1.5", + })) + require.Len(t, b.binds, 1) + assert.Equal(t, "192.168.1.200 via 100.127.0.10", b.binds[0]) +} + +func TestNATManager_AddEIP_Routed_BindsOnIdempotentSkip(t *testing.T) { + m := mock.New() + seedRouter(t, m, "vpc-1") + seedGatewayPortIP(t, m, "vpc-1", "100.127.0.10", nil) + b := &recordedBinder{} + mgr, err := NewNATManager(m, NATModeRouted, WithHostEIPBinder(b.hooks())) + require.NoError(t, err) + + eip := EIPSpec{VPCID: "vpc-1", ExternalIP: "192.168.1.200", LogicalIP: "10.0.1.5"} + require.NoError(t, mgr.AddEIP(context.Background(), eip)) + require.NoError(t, mgr.AddEIP(context.Background(), eip)) + assert.Equal(t, 1, countNAT(m, "dnat_and_snat", "10.0.1.5"), "second add must not duplicate the row") + assert.Len(t, b.binds, 2, "host state is volatile; the skip path must re-bind") +} + +func TestNATManager_AddEIP_Routed_GatewayIPFallsBackToNetworks(t *testing.T) { + m := mock.New() + seedRouter(t, m, "vpc-1") + seedGatewayPortIP(t, m, "vpc-1", "", []string{"100.127.0.7/24"}) + b := &recordedBinder{} + mgr, err := NewNATManager(m, NATModeRouted, WithHostEIPBinder(b.hooks())) + require.NoError(t, err) + + require.NoError(t, mgr.AddEIP(context.Background(), EIPSpec{ + VPCID: "vpc-1", ExternalIP: "192.168.1.200", LogicalIP: "10.0.1.5", + })) + require.Len(t, b.binds, 1) + assert.Equal(t, "192.168.1.200 via 100.127.0.7", b.binds[0]) +} + +func TestNATManager_AddEIP_Routed_NoGatewayLRPFailsBind(t *testing.T) { + m := mock.New() + seedRouter(t, m, "vpc-1") + b := &recordedBinder{} + mgr, err := NewNATManager(m, NATModeRouted, WithHostEIPBinder(b.hooks())) + require.NoError(t, err) + + err = mgr.AddEIP(context.Background(), EIPSpec{ + VPCID: "vpc-1", ExternalIP: "192.168.1.200", LogicalIP: "10.0.1.5", + }) + require.Error(t, err, "no gateway LRP means the /32 route has no next hop") + assert.Empty(t, b.binds) +} + +func TestNATManager_AddEIP_Routed_BindFailureSurfaces(t *testing.T) { + m := mock.New() + seedRouter(t, m, "vpc-1") + seedGatewayPortIP(t, m, "vpc-1", "100.127.0.10", nil) + b := &recordedBinder{bindErr: fmt.Errorf("ip route replace: exit 2")} + mgr, err := NewNATManager(m, NATModeRouted, WithHostEIPBinder(b.hooks())) + require.NoError(t, err) + + err = mgr.AddEIP(context.Background(), EIPSpec{ + VPCID: "vpc-1", ExternalIP: "192.168.1.200", LogicalIP: "10.0.1.5", + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "bind host EIP 192.168.1.200") +} + +func TestNATManager_DeleteEIP_Routed_UnbindsHostEIP(t *testing.T) { + m := mock.New() + seedRouter(t, m, "vpc-1") + seedGatewayPortIP(t, m, "vpc-1", "100.127.0.10", nil) + b := &recordedBinder{} + mgr, err := NewNATManager(m, NATModeRouted, WithHostEIPBinder(b.hooks())) + require.NoError(t, err) + + eip := EIPSpec{VPCID: "vpc-1", ExternalIP: "192.168.1.200", LogicalIP: "10.0.1.5"} + require.NoError(t, mgr.AddEIP(context.Background(), eip)) + require.NoError(t, mgr.DeleteEIP(context.Background(), "vpc-1", "192.168.1.200", "10.0.1.5", "")) + assert.Equal(t, []string{"192.168.1.200"}, b.unbinds) +} + +func TestNATManager_AddEIP_NonRoutedNeverBinds(t *testing.T) { + for _, mode := range []NATMode{NATModeDistributed, NATModeCentralized} { + m := mock.New() + seedRouter(t, m, "vpc-1") + seedGatewayPortIP(t, m, "vpc-1", "192.168.1.240", nil) + b := &recordedBinder{} + mgr, err := NewNATManager(m, mode, WithHostEIPBinder(b.hooks())) + require.NoError(t, err) + + require.NoError(t, mgr.AddEIP(context.Background(), EIPSpec{ + VPCID: "vpc-1", ExternalIP: "192.168.1.200", LogicalIP: "10.0.1.5", + })) + require.NoError(t, mgr.DeleteEIP(context.Background(), "vpc-1", "192.168.1.200", "10.0.1.5", "")) + assert.Empty(t, b.binds, "mode %v must not touch host EIP plumbing", mode) + assert.Empty(t, b.unbinds, "mode %v must not touch host EIP plumbing", mode) + } +} diff --git a/spinifex/network/policy/nat_test.go b/spinifex/network/policy/nat_test.go index 88fb239e3..1445535d2 100644 --- a/spinifex/network/policy/nat_test.go +++ b/spinifex/network/policy/nat_test.go @@ -49,6 +49,7 @@ func countNAT(m *mock.Client, natType, logicalIP string) int { func TestNATModeFromUplinkMode(t *testing.T) { assert.Equal(t, NATModeDistributed, NATModeFromUplinkMode(host.UplinkModePhysical)) assert.Equal(t, NATModeCentralized, NATModeFromUplinkMode(host.UplinkModeVeth)) + assert.Equal(t, NATModeRouted, NATModeFromUplinkMode(host.UplinkModeRouted)) assert.Equal(t, NATModeUnknown, NATModeFromUplinkMode(host.UplinkModeUnknown)) } @@ -671,3 +672,132 @@ func TestNATManager_AddSystemInstanceSNAT_AndDelete(t *testing.T) { // Idempotent on a missing rule. require.NoError(t, nm.DeleteSystemInstanceSNAT(ctx, "vpc-1", "172.31.4.10/32")) } + +func TestNATManager_AddSNAT_ExemptSetAndIdempotency(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + nm, err := NewNATManager(m, NATModeRouted, + WithSNATExemptSet("spinifex_nat_exempt", []string{"100.127.0.0/24", "192.168.50.0/24"})) + require.NoError(t, err) + + // Fresh add: row created with exempt ref, set contains configured CIDRs. + require.NoError(t, nm.AddSNAT(ctx, "vpc-1", "10.0.0.0/16", "100.127.0.10")) + nat := findNAT(m, "snat", "10.0.0.0/16") + require.NotNil(t, nat) + require.NotNil(t, nat.ExemptedExtIps) + as, err := m.GetAddressSet(ctx, "spinifex_nat_exempt") + require.NoError(t, err) + assert.Equal(t, as.UUID, *nat.ExemptedExtIps) + assert.Equal(t, []string{"100.127.0.0/24", "192.168.50.0/24"}, as.Addresses) + + // Double add: idempotent skip, exactly one row, same UUID. + firstUUID := nat.UUID + require.NoError(t, nm.AddSNAT(ctx, "vpc-1", "10.0.0.0/16", "100.127.0.10")) + assert.Equal(t, 1, countNAT(m, "snat", "10.0.0.0/16")) + assert.Equal(t, firstUUID, findNAT(m, "snat", "10.0.0.0/16").UUID) + + // External IP change: stale row replaced, still one row. + require.NoError(t, nm.AddSNAT(ctx, "vpc-1", "10.0.0.0/16", "100.127.0.20")) + assert.Equal(t, 1, countNAT(m, "snat", "10.0.0.0/16")) + assert.Equal(t, "100.127.0.20", findNAT(m, "snat", "10.0.0.0/16").ExternalIP) +} + +func TestNATManager_AddSNAT_PatchesLegacyRow(t *testing.T) { + ctx := context.Background() + m := mock.New() + router := seedRouter(t, m, "vpc-1") + + // Legacy row minted before the exempt option existed (no ref). + require.NoError(t, m.AddNAT(ctx, router, &nbdb.NAT{ + Type: "snat", ExternalIP: "100.127.0.10", LogicalIP: "10.0.0.0/16", + })) + + nm, err := NewNATManager(m, NATModeRouted, + WithSNATExemptSet("spinifex_nat_exempt", []string{"100.127.0.0/24"})) + require.NoError(t, err) + require.NoError(t, nm.AddSNAT(ctx, "vpc-1", "10.0.0.0/16", "100.127.0.10")) + + nat := findNAT(m, "snat", "10.0.0.0/16") + require.NotNil(t, nat.ExemptedExtIps, "legacy row must be patched in place") + as, err := m.GetAddressSet(ctx, "spinifex_nat_exempt") + require.NoError(t, err) + assert.Equal(t, as.UUID, *nat.ExemptedExtIps) + assert.Equal(t, 1, countNAT(m, "snat", "10.0.0.0/16"), "patch must not mint a second row") +} + +func TestNATManager_AddSNAT_NoExemptOutsideRoutedMode(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + + // Option set but mode centralised: no set created, no ref stamped. + nm, err := NewNATManager(m, NATModeCentralized, + WithSNATExemptSet("spinifex_nat_exempt", []string{"100.127.0.0/24"})) + require.NoError(t, err) + require.NoError(t, nm.AddSNAT(ctx, "vpc-1", "10.0.0.0/16", "100.127.0.10")) + + nat := findNAT(m, "snat", "10.0.0.0/16") + require.NotNil(t, nat) + assert.Nil(t, nat.ExemptedExtIps) + assert.Empty(t, m.AddressSets) +} + +func TestNATManager_AddEIP_RoutedStampsExemptSet(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + nm, err := NewNATManager(m, NATModeRouted, + WithSNATExemptSet("spinifex_nat_exempt", []string{"100.127.0.0/24"})) + require.NoError(t, err) + + spec := EIPSpec{VPCID: "vpc-1", ExternalIP: "192.168.50.80", LogicalIP: "10.0.0.5"} + require.NoError(t, nm.AddEIP(ctx, spec)) + + nat := findNAT(m, "dnat_and_snat", "10.0.0.5") + require.NotNil(t, nat) + require.NotNil(t, nat.ExemptedExtIps) + as, err := m.GetAddressSet(ctx, "spinifex_nat_exempt") + require.NoError(t, err) + assert.Equal(t, as.UUID, *nat.ExemptedExtIps) +} + +func TestNATManager_AddEIP_IdempotentSkipPatchesExempt(t *testing.T) { + ctx := context.Background() + m := mock.New() + router := seedRouter(t, m, "vpc-1") + + // Pre-existing routed EIP row without the ref (pre-upgrade). + require.NoError(t, m.AddNAT(ctx, router, &nbdb.NAT{ + Type: "dnat_and_snat", ExternalIP: "192.168.50.80", LogicalIP: "10.0.0.5", + ExternalIDs: map[string]string{"spinifex:vpc_id": "vpc-1", "spinifex:public_ip": "192.168.50.80"}, + })) + + nm, err := NewNATManager(m, NATModeRouted, + WithSNATExemptSet("spinifex_nat_exempt", []string{"100.127.0.0/24"})) + require.NoError(t, err) + spec := EIPSpec{VPCID: "vpc-1", ExternalIP: "192.168.50.80", LogicalIP: "10.0.0.5"} + require.NoError(t, nm.AddEIP(ctx, spec)) + + nat := findNAT(m, "dnat_and_snat", "10.0.0.5") + require.NotNil(t, nat.ExemptedExtIps, "skip path must patch the exempt ref in place") + assert.Equal(t, 1, countNAT(m, "dnat_and_snat", "10.0.0.5")) +} + +func TestNATManager_AddEIP_NoExemptInDistributedMode(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + nm, err := NewNATManager(m, NATModeDistributed, + WithSNATExemptSet("spinifex_nat_exempt", []string{"100.127.0.0/24"})) + 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", + })) + nat := findNAT(m, "dnat_and_snat", "10.0.0.5") + require.NotNil(t, nat) + assert.Nil(t, nat.ExemptedExtIps) + assert.Empty(t, m.AddressSets) +} diff --git a/spinifex/network/policy/policy.go b/spinifex/network/policy/policy.go index 5fca12355..e22867220 100644 --- a/spinifex/network/policy/policy.go +++ b/spinifex/network/policy/policy.go @@ -21,6 +21,11 @@ const ( // NATModeCentralized leaves ExternalMAC/LogicalPort unset; gateway chassis // owns SNAT/DNAT. Required by UplinkModeVeth. NATModeCentralized + + // NATModeRouted is centralised-shaped: gateway chassis SNATs the VPC CIDR + // to its transit LRP IP; the host masquerades egress. Required by + // UplinkModeRouted. Outbound-only — no EIP/public-IP support. + NATModeRouted ) func (m NATMode) String() string { @@ -29,6 +34,8 @@ func (m NATMode) String() string { return "distributed" case NATModeCentralized: return "centralized" + case NATModeRouted: + return "routed" default: return "unknown" } @@ -42,6 +49,8 @@ func NATModeFromUplinkMode(m host.UplinkMode) NATMode { return NATModeDistributed case host.UplinkModeVeth: return NATModeCentralized + case host.UplinkModeRouted: + return NATModeRouted default: return NATModeUnknown } diff --git a/spinifex/network/policy/route_test.go b/spinifex/network/policy/route_test.go index 0ba25dfe0..d74418223 100644 --- a/spinifex/network/policy/route_test.go +++ b/spinifex/network/policy/route_test.go @@ -103,6 +103,33 @@ func TestRouteManager_DeleteStaticRoute_RemovesRow(t *testing.T) { assert.Nil(t, findRoute(m, "10.42.0.0/24")) } +// Every VPC router carries 0.0.0.0/0 — a drift replace on one router must +// never delete another router's default route (regression: unscoped prefix +// match grabbed the first 0.0.0.0/0 row DB-wide and hit a referential +// integrity violation, wedging IGW attach on multi-VPC nodes). +func TestRouteManager_DriftReplace_ScopedToRouter(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + seedRouter(t, m, "vpc-2") + rm := NewRouteManager(m) + + require.NoError(t, rm.AddDefaultRoute(ctx, "vpc-1", "192.168.1.1", "gw-vpc-1")) + require.NoError(t, rm.AddDefaultRoute(ctx, "vpc-2", "192.168.1.1", "gw-vpc-2")) + + // Drift vpc-2's nexthop (the nat-mode transit migration path). + require.NoError(t, rm.AddDefaultRoute(ctx, "vpc-2", "100.127.0.1", "gw-vpc-2")) + + assert.Equal(t, 2, countRoutes(m, "0.0.0.0/0")) + r1 := m.Routers[topology.VPCRouter("vpc-1")] + require.Len(t, r1.StaticRoutes, 1) + assert.Equal(t, "192.168.1.1", m.StaticRoutes[r1.StaticRoutes[0]].Nexthop, + "vpc-1 default route must survive vpc-2's drift replace") + r2 := m.Routers[topology.VPCRouter("vpc-2")] + require.Len(t, r2.StaticRoutes, 1) + assert.Equal(t, "100.127.0.1", m.StaticRoutes[r2.StaticRoutes[0]].Nexthop) +} + func TestRouteManager_AddSubnetEgress_InstallsScopedPolicy(t *testing.T) { ctx := context.Background() m := mock.New() diff --git a/spinifex/network/reconcile/apply.go b/spinifex/network/reconcile/apply.go index 7663f10b8..b0ba43f7a 100644 --- a/spinifex/network/reconcile/apply.go +++ b/spinifex/network/reconcile/apply.go @@ -270,16 +270,16 @@ func (r *reconciler) pruneOrphanPorts(ctx context.Context, intent IntentState) { } // applyIGWs ensures every intent IGW has OVN topology and rebinds chassis on -// existing IGWs. AttachIGW is idempotent. +// existing IGWs. AttachIGW is idempotent and must run even when the gateway +// switch port already exists: its already-attached path re-ensures host state +// (routed-NAT ingress routes) that survives in OVN but not across reboots. func (r *reconciler) applyIGWs(ctx context.Context, intent IntentState, actual ActualState) { for vpcID, spec := range intent.IGWs { - if _, ok := actual.ExternalSwch[vpcID]; !ok { - if err := r.igw.AttachIGW(ctx, spec); err != nil { - slog.Error("reconcile/apply: AttachIGW failed", "vpc_id", vpcID, "err", err) - continue - } - actual.ExternalSwch[vpcID] = struct{}{} + if err := r.igw.AttachIGW(ctx, spec); err != nil { + slog.Error("reconcile/apply: AttachIGW failed", "vpc_id", vpcID, "err", err) + continue } + actual.ExternalSwch[vpcID] = struct{}{} r.rebindGatewayChassis(ctx, vpcID, eipProbeIP(intent, vpcID)) } } diff --git a/spinifex/vpcd/pool_select_test.go b/spinifex/vpcd/pool_select_test.go new file mode 100644 index 000000000..2cbbc00d5 --- /dev/null +++ b/spinifex/vpcd/pool_select_test.go @@ -0,0 +1,44 @@ +package vpcd + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/mulgadc/spinifex/spinifex/network/external" + "github.com/mulgadc/spinifex/spinifex/network/host" +) + +func TestSelectExternalPools(t *testing.T) { + transit := external.ExternalPoolConfig{Name: host.NATTransitPoolName, Gateway: host.NATTransitGatewayIP} + wan := external.ExternalPoolConfig{Name: "wan", Gateway: "192.168.1.1"} + + t.Run("nat picks transit by name regardless of order", func(t *testing.T) { + igw, public := selectExternalPools("nat", []external.ExternalPoolConfig{wan, transit}) + require.NotNil(t, igw) + require.NotNil(t, public) + assert.Equal(t, host.NATTransitPoolName, igw.Name) + assert.Equal(t, "wan", public.Name) + }) + + t.Run("nat transit only", func(t *testing.T) { + igw, public := selectExternalPools("nat", []external.ExternalPoolConfig{transit}) + require.NotNil(t, igw) + assert.Equal(t, host.NATTransitPoolName, igw.Name) + assert.Nil(t, public) + }) + + t.Run("pool mode keeps first pool, no public split", func(t *testing.T) { + igw, public := selectExternalPools("pool", []external.ExternalPoolConfig{wan}) + require.NotNil(t, igw) + assert.Equal(t, "wan", igw.Name) + assert.Nil(t, public) + }) + + t.Run("no pools", func(t *testing.T) { + igw, public := selectExternalPools("", nil) + assert.Nil(t, igw) + assert.Nil(t, public) + }) +} diff --git a/spinifex/vpcd/vpcd.go b/spinifex/vpcd/vpcd.go index ef1ab1225..805bbb747 100644 --- a/spinifex/vpcd/vpcd.go +++ b/spinifex/vpcd/vpcd.go @@ -32,9 +32,11 @@ import ( // Bridge mode selects how the WAN NIC reaches the OVS bridge. // Direct: WAN NIC added to OVS (distributed NAT, not safe on mgmt NIC). // Veth: veth pair links a Linux bridge to OVS (requires centralized NAT). +// NAT: no WAN NIC bridged; transit veth + host masquerade (routed NAT). const ( BridgeModeDirect = "direct" BridgeModeVeth = "veth" + BridgeModeNAT = "nat" // OvnExternalBridge is the OVS bridge targeted by ovn-bridge-mappings for the "external" localnet. OvnExternalBridge = "br-ext" ) @@ -100,7 +102,7 @@ type Config struct { BaseDir string // Debug enables debug logging. Debug bool - // ExternalMode is "pool" or "" (disabled). + // ExternalMode is "pool", "nat" (routed, outbound-only), or "" (disabled). ExternalMode string // ExternalPools holds the cluster-wide external IP pool configs. ExternalPools []external.ExternalPoolConfig @@ -117,6 +119,9 @@ type Config struct { // to scope its IntentState scan to local-AZ KV records; new VPC records // are stamped with this value at create time. AZ string + // NATExemptCIDRs are extra destinations that skip routed-mode SNAT, + // appended to the transit /24 in the spinifex_nat_exempt set. nat mode only. + NATExemptCIDRs []string } // Service implements the Spinifex service interface for vpcd. @@ -391,6 +396,22 @@ func launchService(cfg *Config) error { return err } + if bridgeMode == BridgeModeNAT { + // Re-ensure kernel egress rules on every start so they survive reboots + // and firewall flushes without iptables-persistent. + if err := host.EnsureNATEgressRules(ctx, host.NewExecRunner()); err != nil { + slog.Error("vpcd: NAT egress rule install failed", "err", err) + return err + } + if cfg.ExternalMode == "nat" && len(cfg.ExternalPools) == 0 { + slog.Warn("vpcd: nat mode with no external pool; synthesizing default transit pool", + "name", host.NATTransitPoolName, "gateway", host.NATTransitGatewayIP) + cfg.ExternalPools = append(cfg.ExternalPools, external.ExternalPoolConfig{ + Name: host.NATTransitPoolName, Gateway: host.NATTransitGatewayIP, PrefixLen: 24, + }) + } + } + if err := ensureExternalCIDRReady(ctx, cfg.ExternalMode, wanBridge); err != nil { return err } @@ -409,8 +430,11 @@ func launchService(cfg *Config) error { slog.Info("vpcd: gateway chassis discovered", "chassis", chassisNames) uplinkMode := host.UplinkModePhysical - if bridgeMode == BridgeModeVeth { + switch bridgeMode { + case BridgeModeVeth: uplinkMode = host.UplinkModeVeth + case BridgeModeNAT: + uplinkMode = host.UplinkModeRouted } natMode := policy.NATModeFromUplinkMode(uplinkMode) @@ -420,23 +444,27 @@ func launchService(cfg *Config) error { } topoMgr := topology.NewLiveManager(liveClient, topoOpts...) + igwPool, publicPool := selectExternalPools(cfg.ExternalMode, cfg.ExternalPools) + sgMgr := policy.NewSecurityGroupManager(liveClient) - natMgr, err := policy.NewNATManager(liveClient, natMode, + natOpts := []policy.Option{ policy.WithFlowsBarrier(waitForFlowsHV), policy.WithNeighFlusher(neighFlusher(wanBridge)), policy.WithNeighPrimer(neighPrimer(wanBridge)), - ) + } + if natMode == policy.NATModeRouted { + natOpts = append(natOpts, policy.WithSNATExemptSet(policy.NATExemptSetName, + append([]string{host.NATTransitCIDR}, cfg.NATExemptCIDRs...))) + } + if natMode == policy.NATModeRouted && publicPool != nil { + natOpts = append(natOpts, policy.WithHostEIPBinder(hostEIPBinder(publicPool))) + } + natMgr, err := policy.NewNATManager(liveClient, natMode, natOpts...) if err != nil { return fmt.Errorf("construct NAT manager: %w", err) } routeMgr := policy.NewRouteManager(liveClient) - var igwPool *external.ExternalPoolConfig - if cfg.ExternalMode != "" && len(cfg.ExternalPools) > 0 { - p := cfg.ExternalPools[0] - igwPool = &p - } - js, err := nc.JetStream() if err != nil { return fmt.Errorf("get JetStream context: %w", err) @@ -488,16 +516,56 @@ func launchService(cfg *Config) error { } }() + // Host ingress plumbing only exists in routed-NAT mode; other modes keep + // nil hooks so the IGW manager never touches host routes. + var routedIngress *external.RoutedIngressHooks + if bridgeMode == BridgeModeNAT { + ingressRunner := host.NewExecRunner() + // Duplicate VPC CIDRs (every account's default VPC is 172.31.0.0/16) + // collide on the single host route table: the bootstrap VPC always + // wins; other VPCs only install when the CIDR is free and only remove + // a route their own gateway IP holds. + preferredVPC := "" + if cfg.Bootstrap != nil { + preferredVPC = cfg.Bootstrap.VpcId + } + routedIngress = &external.RoutedIngressHooks{ + Ensure: func(ctx context.Context, vpcID, vpcCIDR, gwLrpIP string) error { + if vpcID != preferredVPC { + holder, err := host.VPCIngressRouteVia(ctx, ingressRunner, vpcCIDR) + if err == nil && holder != "" && holder != gwLrpIP { + slog.Warn("vpcd: host ingress route conflict, keeping existing holder", + "vpc_id", vpcID, "vpc_cidr", vpcCIDR, "holder_gw", holder, "skipped_gw", gwLrpIP) + return nil + } + } + return host.EnsureVPCIngressRoute(ctx, ingressRunner, vpcCIDR, gwLrpIP) + }, + Remove: func(ctx context.Context, vpcID, vpcCIDR, gwLrpIP string) error { + if gwLrpIP != "" { + holder, err := host.VPCIngressRouteVia(ctx, ingressRunner, vpcCIDR) + if err == nil && holder != "" && holder != gwLrpIP { + slog.Info("vpcd: leaving host ingress route held by another VPC", + "vpc_id", vpcID, "vpc_cidr", vpcCIDR, "holder_gw", holder) + return nil + } + } + return host.RemoveVPCIngressRoute(ctx, ingressRunner, vpcCIDR) + }, + } + } + gwAllocator := pickGatewayAllocator(igwPool, liveClient, dhcpMgr) igwMgr, err := external.NewIGWManager(external.IGWManagerConfig{ - OVN: liveClient, - Routes: routeMgr, - NAT: natMgr, - Pool: igwPool, - Allocator: gwAllocator, - Chassis: chassisNames, - NATMode: natMode, - FlowsBarrier: waitForFlowsHV, + OVN: liveClient, + Routes: routeMgr, + NAT: natMgr, + Pool: igwPool, + Allocator: gwAllocator, + Chassis: chassisNames, + NATMode: natMode, + FlowsBarrier: waitForFlowsHV, + RoutedIngress: routedIngress, }) if err != nil { return fmt.Errorf("construct IGW manager: %w", err) @@ -648,15 +716,59 @@ func pickGatewayAllocator(pool *external.ExternalPoolConfig, ovnClient ovn.Clien return external.NewStaticRangeAllocator(ovnClient) } -// resolveBridgeConfig picks bridge mode (auto-detecting when unset) and always uses "br-wan" as the WAN bridge. +// resolveBridgeConfig picks bridge mode (auto-detecting when unset) and the WAN +// bridge: "br-wan" for bridged modes, the transit veth host end for nat mode. func resolveBridgeConfig(cfgBridgeMode, externalIface string) (string, string) { bridgeMode := cfgBridgeMode if bridgeMode == "" && externalIface != "" { bridgeMode = detectBridgeMode(externalIface) } + if bridgeMode == BridgeModeNAT { + return bridgeMode, host.NATTransitHostEnd + } return bridgeMode, "br-wan" } +// selectExternalPools splits configured pools by role. In nat mode the IGW +// gateway-LRP allocator draws from the transit pool (matched by name, never +// by index); any other pool carries public EIPs delivered via host plumbing. +// Other modes keep the first pool for the IGW and have no public split. +func selectExternalPools(externalMode string, pools []external.ExternalPoolConfig) (igwPool, publicPool *external.ExternalPoolConfig) { + for i := range pools { + p := pools[i] + if externalMode == "nat" && p.Name != host.NATTransitPoolName { + if publicPool == nil { + publicPool = &p + } + continue + } + if igwPool == nil { + igwPool = &p + } + } + return igwPool, publicPool +} + +// hostEIPBinder builds the routed-mode host plumbing hooks for EIPs on the +// public pool: /32 route into OVN plus proxy-ARP on the uplink. Static pools +// locate the uplink via the pool gateway; dhcp pools via their bind bridge. +func hostEIPBinder(pool *external.ExternalPoolConfig) policy.HostEIPBinder { + runner := host.NewExecRunner() + gateway, uplinkHint := pool.Gateway, pool.BindBridge + return policy.HostEIPBinder{ + Bind: func(eip policy.EIPSpec, gwLrpIP string) error { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + return host.EnsureEIPIngress(ctx, runner, eip.ExternalIP, gwLrpIP, gateway, uplinkHint) + }, + Unbind: func(externalIP string) error { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + return host.RemoveEIPIngress(ctx, runner, externalIP, gateway, uplinkHint) + }, + } +} + // neighFlusher builds the ARP-flush hook for AddEIP/DeleteEIP so recycled IPs re-resolve L2 immediately. // No-op when wanBridge is unset. func neighFlusher(wanBridge string) policy.NeighFlusher { @@ -688,9 +800,14 @@ var ifaceExists = func(name string) bool { return exec.Command("ip", "link", "show", name).Run() == nil } -// detectBridgeMode infers bridge mode: veth when veth-wan-ovs exists, direct otherwise. +// detectBridgeMode infers bridge mode: nat when spx-nat-ovs exists, veth when +// veth-wan-ovs exists, direct otherwise. // Each branch logs at Info/Warn so `journalctl | grep bridge` shows the full detection trail. func detectBridgeMode(externalIface string) string { + if ifaceExists(host.NATTransitOVSEnd) { + slog.Info("vpcd: detected routed-NAT transit veth", "mode", BridgeModeNAT) + return BridgeModeNAT + } if ifaceExists("veth-wan-ovs") { slog.Info("vpcd: detected veth pair linking Linux bridge to OVS", "mode", BridgeModeVeth) return BridgeModeVeth @@ -766,8 +883,23 @@ func verifyBridgeMode(mode, externalIface, wanBridge string) error { master, wanBridge) } return nil + case BridgeModeNAT: + br, err := portToBr(host.NATTransitOVSEnd) + if err != nil { + return fmt.Errorf("vpcd: nat bridge mode: %s not on OVS — run setup-ovn.sh --nat-uplink: %w", + host.NATTransitOVSEnd, err) + } + if br != OvnExternalBridge { + return fmt.Errorf("vpcd: nat bridge mode: %s is on OVS bridge %q, expected %q", + host.NATTransitOVSEnd, br, OvnExternalBridge) + } + if !ifaceExists(host.NATTransitHostEnd) { + return fmt.Errorf("vpcd: nat bridge mode: %s link missing — run setup-ovn.sh --nat-uplink", + host.NATTransitHostEnd) + } + return nil default: - return fmt.Errorf("vpcd: unknown bridge_mode %q — supported values: %q, %q", - mode, BridgeModeDirect, BridgeModeVeth) + return fmt.Errorf("vpcd: unknown bridge_mode %q — supported values: %q, %q, %q", + mode, BridgeModeDirect, BridgeModeVeth, BridgeModeNAT) } } diff --git a/spinifex/vpcd/vpcd_nat_test.go b/spinifex/vpcd/vpcd_nat_test.go new file mode 100644 index 000000000..3b885533b --- /dev/null +++ b/spinifex/vpcd/vpcd_nat_test.go @@ -0,0 +1,64 @@ +package vpcd + +import ( + "strings" + "testing" + + "github.com/mulgadc/spinifex/spinifex/network/host" +) + +func TestVerifyBridgeMode_NATOK(t *testing.T) { + stubBridgeProbes(t, map[string]string{host.NATTransitOVSEnd: OvnExternalBridge}, nil) + stubDetectProbes(t, []string{host.NATTransitHostEnd}) + if err := verifyBridgeMode(BridgeModeNAT, "", host.NATTransitHostEnd); err != nil { + t.Fatalf("unexpected: %v", err) + } +} + +func TestVerifyBridgeMode_NATMissingOvsPort(t *testing.T) { + stubBridgeProbes(t, map[string]string{}, nil) + stubDetectProbes(t, []string{host.NATTransitHostEnd}) + err := verifyBridgeMode(BridgeModeNAT, "", host.NATTransitHostEnd) + if err == nil || !strings.Contains(err.Error(), "--nat-uplink") { + t.Fatalf("expected setup-ovn.sh hint, got: %v", err) + } +} + +func TestVerifyBridgeMode_NATWrongOvsBridge(t *testing.T) { + stubBridgeProbes(t, map[string]string{host.NATTransitOVSEnd: "br-wan"}, nil) + stubDetectProbes(t, []string{host.NATTransitHostEnd}) + err := verifyBridgeMode(BridgeModeNAT, "", host.NATTransitHostEnd) + if err == nil || !strings.Contains(err.Error(), OvnExternalBridge) { + t.Fatalf("expected br-ext mismatch, got: %v", err) + } +} + +func TestVerifyBridgeMode_NATMissingHostEnd(t *testing.T) { + stubBridgeProbes(t, map[string]string{host.NATTransitOVSEnd: OvnExternalBridge}, nil) + stubDetectProbes(t, nil) + err := verifyBridgeMode(BridgeModeNAT, "", host.NATTransitHostEnd) + if err == nil || !strings.Contains(err.Error(), host.NATTransitHostEnd) { + t.Fatalf("expected missing host end error, got: %v", err) + } +} + +func TestVerifyBridgeMode_UnknownModeListsNAT(t *testing.T) { + err := verifyBridgeMode("bogus", "", "") + if err == nil || !strings.Contains(err.Error(), BridgeModeNAT) { + t.Fatalf("expected error listing nat mode, got: %v", err) + } +} + +func TestDetectBridgeMode_NATWinsOverVeth(t *testing.T) { + stubDetectProbes(t, []string{host.NATTransitOVSEnd, "veth-wan-ovs"}) + if got := detectBridgeMode("enp0s3"); got != BridgeModeNAT { + t.Errorf("want %q, got %q", BridgeModeNAT, got) + } +} + +func TestResolveBridgeConfig_NATUsesTransitHostEnd(t *testing.T) { + mode, br := resolveBridgeConfig(BridgeModeNAT, "") + if mode != BridgeModeNAT || br != host.NATTransitHostEnd { + t.Errorf("got (%q,%q), want (%q,%q)", mode, br, BridgeModeNAT, host.NATTransitHostEnd) + } +} diff --git a/tests/e2e/Makefile b/tests/e2e/Makefile index c7202ba14..58faa0a2e 100644 --- a/tests/e2e/Makefile +++ b/tests/e2e/Makefile @@ -11,9 +11,9 @@ SUITE_TAG ?= $(notdir $(patsubst %/...,%,$(firstword $(PKG)))) # named suite into BIN_DIR/.test so CI can ship binaries (not source + # go toolchain) to the remote VM. BIN_DIR ?= $(CURDIR)/_bin -SUITES_BUILD ?= cert lb eks ecs single iam ecr gpu +SUITES_BUILD ?= cert lb eks ecs single iam ecr gpu natuplink -.PHONY: e2e e2e-cert e2e-lb e2e-eks e2e-ecs e2e-single e2e-iam e2e-ecr e2e-multinode e2e-bm e2e-reboot e2e-tofu e2e-gpu e2e-all clean e2e-build +.PHONY: e2e e2e-cert e2e-lb e2e-eks e2e-ecs e2e-single e2e-iam e2e-ecr e2e-multinode e2e-bm e2e-reboot e2e-tofu e2e-gpu e2e-natuplink e2e-all clean e2e-build ifeq ($(GOTESTSUM),1) E2E_CMD = gotestsum \ @@ -44,6 +44,7 @@ e2e-bm: ; $(MAKE) e2e PKG=./baremetal/... MODE=baremetal e2e-reboot: ; $(MAKE) e2e PKG=./reboot/... e2e-tofu: ; $(MAKE) e2e PKG=./tofu/... e2e-gpu: ; $(MAKE) e2e PKG=./gpu/... +e2e-natuplink: ; $(MAKE) e2e PKG=./natuplink/... e2e-all: ; $(MAKE) e2e e2e-build: diff --git a/tests/e2e/natuplink/natuplink_test.go b/tests/e2e/natuplink/natuplink_test.go new file mode 100644 index 000000000..54ec184df --- /dev/null +++ b/tests/e2e/natuplink/natuplink_test.go @@ -0,0 +1,793 @@ +//go:build e2e + +// Package natuplink is the routed-NAT (external_mode = "nat") single-node +// suite. It runs ON the spinifex node itself (like the single suite): the +// host-wiring and OVN phases shell out to ip/iptables/ovn-nbctl locally, +// and instance egress is proven via the serial console because nat mode +// has no inbound path to VMs (outbound-only by design). +// +// The node must be provisioned with `setup-ovn.sh --management --nat-uplink` +// followed by `spx admin init --external-mode=nat`; the suite skips unless +// spinifex.toml carries external_mode = "nat". +package natuplink + +import ( + "bufio" + "encoding/base64" + "fmt" + "io" + "net" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/tests/e2e/harness" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + transitCIDR = "100.127.0.0/24" + transitGatewayIP = "100.127.0.1" + transitHostEnd = "spx-nat-host" + transitOVSEnd = "spx-nat-ovs" + uplinkBridge = "br-ext" + + egressOKMarker = "NAT-E2E-EGRESS-OK" + egressFailMarker = "NAT-E2E-EGRESS-FAIL" + + secondVPCCIDR = "10.213.0.0/16" + natExemptSetName = "spinifex_nat_exempt" +) + +// egressUserData probes outbound WAN reachability from inside the guest and +// reports the verdict on the serial console — the only channel back to the +// test in nat mode. ping 8.8.8.8 is the DNS-free fallback; the curl targets +// additionally prove DHCP-delivered DNS works through the masquerade. +const egressUserData = `#!/bin/bash +for i in $(seq 1 36); do + if curl -fsS -m 10 -o /dev/null http://connectivity-check.ubuntu.com/ \ + || curl -fsS -m 10 -o /dev/null http://www.google.com/generate_204 \ + || ping -c 1 -W 3 8.8.8.8 >/dev/null 2>&1; then + echo "NAT-E2E-EGRESS-OK" | tee /dev/console + exit 0 + fi + sleep 5 +done +echo "NAT-E2E-EGRESS-FAIL" | tee /dev/console +` + +type fixture struct { + env *harness.Env + aws *harness.AWSClient + harness *harness.Fixture + artifacts string + configTOML string + // publicPool is true when spinifex.toml carries a non-transit pool — + // the Tier 2 (EIP / public IP) lane is exercised only then. + publicPool bool +} + +// TestNATUplink runs the routed-NAT lane end to end. Phases are sequential: +// wiring must hold before config, config before API behaviour, and the OVN +// phases read state the earlier phases prove exists. +func TestNATUplink(t *testing.T) { + env := harness.LoadEnv(t) + if env.Mode != harness.ModeSingle { + t.Skipf("natuplink suite is single-node only (mode=%s)", env.Mode) + } + cfgPath := configPath(env) + if mode := readExternalMode(t, cfgPath); mode != "nat" { + t.Skipf("natuplink suite requires external_mode=nat (got %q in %s)", mode, cfgPath) + } + harness.SkipIfNoOVN(t) + + awsCli := harness.NewAWSClient(t, env) + hfx, err := harness.NewProcessFixture(awsCli) + require.NoError(t, err, "harness fixture init") + t.Cleanup(hfx.Close) + + fix := &fixture{ + env: env, + aws: awsCli, + harness: hfx, + artifacts: harness.ArtifactDir(t, env), + configTOML: cfgPath, + publicPool: hasPublicPool(t, cfgPath), + } + + harness.Phase(t, "NAT Uplink — Phase 1: host wiring") + phaseHostWiring(t) + + harness.Phase(t, "NAT Uplink — Phase 2: config") + phaseConfig(t, fix) + + if fix.publicPool { + harness.Phase(t, "NAT Uplink — Phase 3: EIP surface enabled (Tier 2)") + phaseEIPEnabled(t, fix) + } else { + harness.Phase(t, "NAT Uplink — Phase 3: EIP surface disabled") + phaseEIPDisabled(t, fix) + } + + harness.Phase(t, "NAT Uplink — Phase 4: default subnet public IP mapping") + def := phaseDefaultSubnet(t, fix) + + harness.Phase(t, "NAT Uplink — Phase 5: OVN gateway + SNAT on transit net") + defaultGwIP := phaseOVNGateway(t, fix, def.VPCID) + + harness.Phase(t, "NAT Uplink — Phase 6: instance boots and reaches WAN outbound") + probe := phaseInstanceEgress(t, fix, def) + + harness.Phase(t, "NAT Uplink — Phase 7: host reaches instance private IP (Tier 1 ingress)") + phaseHostIngress(t, fix, def, probe) + + harness.Phase(t, "NAT Uplink — Phase 8: second VPC gets a unique transit gateway IP") + phaseUniqueTransitIP(t, fix, defaultGwIP) + + if fix.publicPool { + harness.Phase(t, "NAT Uplink — Phase 9: EIP ingress lifecycle (Tier 2)") + phaseEIPIngress(t, fix, def, probe) + } else { + t.Log("Phase 9 (Tier 2 EIP ingress) skipped: no public pool in config") + } +} + +// --- Phase 1: host wiring -------------------------------------------------- + +func phaseHostWiring(t *testing.T) { + t.Helper() + + harness.Step(t, "transit veth host end %s carries %s", transitHostEnd, transitGatewayIP) + addrOut := hostCmd(t, "ip", "-o", "addr", "show", "dev", transitHostEnd) + assert.Containsf(t, addrOut, transitGatewayIP+"/24", + "%s must carry %s/24\n%s", transitHostEnd, transitGatewayIP, addrOut) + + harness.Step(t, "transit veth OVS end %s attached to %s", transitOVSEnd, uplinkBridge) + br := harness.OvsVsctl(t, "port-to-br", transitOVSEnd) + assert.Equalf(t, uplinkBridge, br, "%s must be a port on %s", transitOVSEnd, uplinkBridge) + + harness.Step(t, "ip_forward enabled") + fwd, err := os.ReadFile("/proc/sys/net/ipv4/ip_forward") + require.NoError(t, err, "read ip_forward") + assert.Equal(t, "1", strings.TrimSpace(string(fwd)), "net.ipv4.ip_forward must be 1") + + harness.Step(t, "NAT egress iptables rules present") + checks := [][]string{ + {"-t", "nat", "-C", "POSTROUTING", "-s", transitCIDR, "!", "-d", transitCIDR, + "-m", "comment", "--comment", "spinifex-nat-egress", "-j", "MASQUERADE"}, + {"-t", "filter", "-C", "FORWARD", "-i", transitHostEnd, "-s", transitCIDR, + "-m", "comment", "--comment", "spinifex-nat-egress", "-j", "ACCEPT"}, + {"-t", "filter", "-C", "FORWARD", "-o", transitHostEnd, "-m", "conntrack", + "--ctstate", "RELATED,ESTABLISHED", + "-m", "comment", "--comment", "spinifex-nat-egress", "-j", "ACCEPT"}, + } + for _, args := range checks { + out, err := exec.Command("sudo", append([]string{"-n", "iptables"}, args...)...).CombinedOutput() + assert.NoErrorf(t, err, "iptables %s missing: %s", strings.Join(args, " "), string(out)) + } +} + +// --- Phase 2: config --------------------------------------------------------- + +func phaseConfig(t *testing.T, fix *fixture) { + t.Helper() + data, err := os.ReadFile(fix.configTOML) + require.NoError(t, err, "read %s", fix.configTOML) + content := string(data) + + assert.Contains(t, content, `external_mode = "nat"`) + assert.Contains(t, content, `bridge_mode = "nat"`) + assert.Containsf(t, content, "nat-transit", "transit pool block missing in %s", fix.configTOML) + assert.Containsf(t, content, transitGatewayIP, "transit gateway IP missing in %s", fix.configTOML) + if fix.publicPool { + harness.Detail(t, "tier2", "public pool present — EIP lane active") + } else { + assert.NotContains(t, content, "range_start", "Tier-1-only nat mode must not carry a public IP range") + } +} + +// --- Phase 3: EIP surface ---------------------------------------------------- + +func phaseEIPDisabled(t *testing.T, fix *fixture) { + t.Helper() + + harness.Step(t, "describe-addresses returns empty") + out, err := fix.aws.EC2.DescribeAddresses(&ec2.DescribeAddressesInput{}) + require.NoError(t, err, "describe-addresses must succeed (empty), not error") + assert.Emptyf(t, out.Addresses, "nat mode must report zero addresses, got %d", len(out.Addresses)) + + harness.Step(t, "allocate-address returns UnsupportedOperation") + harness.ExpectError(t, "UnsupportedOperation", func() error { + // e2e:allow-create — negative-path call; nat mode must reject it, nothing is created. + _, aerr := fix.aws.EC2.AllocateAddress(&ec2.AllocateAddressInput{Domain: aws.String("vpc")}) + return aerr + }) +} + +// phaseEIPEnabled proves nat mode with a public pool exposes the same EIP +// surface as pool mode: allocate works, the address shows in describe, and +// release returns it to the pool. The full ingress lifecycle runs in Phase 9. +func phaseEIPEnabled(t *testing.T, fix *fixture) { + t.Helper() + + harness.Step(t, "allocate-address succeeds from public pool") + // e2e:allow-create — scratch EIP, released at the end of this phase. + alloc, err := fix.aws.EC2.AllocateAddress(&ec2.AllocateAddressInput{Domain: aws.String("vpc")}) + require.NoError(t, err, "allocate-address must succeed with a public pool configured") + allocID := aws.StringValue(alloc.AllocationId) + eip := aws.StringValue(alloc.PublicIp) + require.NotEmpty(t, allocID, "allocate-address returned no AllocationId") + require.NotEmpty(t, eip, "allocate-address returned no PublicIp") + harness.Detail(t, "allocation", allocID, "eip", eip) + + harness.Step(t, "describe-addresses lists the allocation") + out, err := fix.aws.EC2.DescribeAddresses(&ec2.DescribeAddressesInput{ + AllocationIds: []*string{aws.String(allocID)}, + }) + require.NoError(t, err, "describe-addresses %s", allocID) + require.Len(t, out.Addresses, 1, "allocation %s missing from describe-addresses", allocID) + assert.Equal(t, eip, aws.StringValue(out.Addresses[0].PublicIp)) + + harness.Step(t, "release-address returns it to the pool") + _, err = fix.aws.EC2.ReleaseAddress(&ec2.ReleaseAddressInput{AllocationId: aws.String(allocID)}) + require.NoError(t, err, "release-address %s", allocID) +} + +// --- Phase 4: default subnet ------------------------------------------------- + +func phaseDefaultSubnet(t *testing.T, fix *fixture) harness.VPCInfo { + t.Helper() + def := harness.EnsureDefaultVPC(t, fix.harness) + harness.Detail(t, "vpc", def.VPCID, "subnet", def.SubnetID) + + out, err := fix.aws.EC2.DescribeSubnets(&ec2.DescribeSubnetsInput{ + SubnetIds: []*string{aws.String(def.SubnetID)}, + }) + require.NoError(t, err, "describe-subnets %s", def.SubnetID) + require.NotEmpty(t, out.Subnets, "default subnet %s not found", def.SubnetID) + if fix.publicPool { + assert.Truef(t, aws.BoolValue(out.Subnets[0].MapPublicIpOnLaunch), + "default subnet %s must have MapPublicIpOnLaunch=true with a public pool (bridge parity)", def.SubnetID) + } else { + assert.Falsef(t, aws.BoolValue(out.Subnets[0].MapPublicIpOnLaunch), + "default subnet %s must have MapPublicIpOnLaunch=false in Tier-1-only nat mode", def.SubnetID) + } + return def +} + +// --- Phase 5: OVN gateway + SNAT ---------------------------------------------- + +// phaseOVNGateway asserts the default VPC's gateway LRP sits on the transit +// /24 and the router SNATs the VPC CIDR to that LRP IP. Returns the gateway +// LRP IP for the uniqueness check in Phase 7. +func phaseOVNGateway(t *testing.T, fix *fixture, vpcID string) string { + t.Helper() + + gwIP := gatewayLRPIP(t, vpcID) + require.NotEmptyf(t, gwIP, "gateway LRP gw-%s has no transit IP", vpcID) + harness.Detail(t, "gw_lrp_ip", gwIP) + assert.Truef(t, strings.HasPrefix(gwIP, "100.127.0."), + "gateway LRP IP %s must be on %s", gwIP, transitCIDR) + assert.NotEqualf(t, transitGatewayIP, gwIP, + "gateway LRP must not squat the host transit gateway IP") + + vpcCIDR := describeVPCCIDR(t, fix, vpcID) + natList := harness.OvnNbctl(t, "lr-nat-list", "vpc-"+vpcID) + harness.Detail(t, "lr_nat_list", natList) + assert.Containsf(t, natList, "snat", "router vpc-%s missing snat rule\n%s", vpcID, natList) + assert.Containsf(t, natList, gwIP, "snat external IP should be gateway LRP IP %s\n%s", gwIP, natList) + assert.Containsf(t, natList, vpcCIDR, "snat logical IP should be VPC CIDR %s\n%s", vpcCIDR, natList) + return gwIP +} + +// --- Phase 6: instance egress --------------------------------------------------- + +// egressProbe carries the phase-6 instance into the ingress phases. +type egressProbe struct { + instanceID string + privateIP string + publicIP string + sgID string +} + +func phaseInstanceEgress(t *testing.T, fix *fixture, def harness.VPCInfo) egressProbe { + t.Helper() + + instType, arch := harness.DiscoverNanoInstanceType(t, fix.harness) + amiID := harness.DiscoverUbuntuAMI(t, fix.harness, arch) + keyName, _ := harness.EnsureKeyPair(t, fix.harness, fix.artifacts) + harness.Detail(t, "type", instType, "ami", amiID, "key", keyName) + + harness.Step(t, "run-instances (default subnet, egress-probe user-data)") + // e2e:allow-create — throwaway probe VM; the egress-probe user-data makes it unshareable. + runOut, err := fix.aws.EC2.RunInstances(&ec2.RunInstancesInput{ + ImageId: aws.String(amiID), + InstanceType: aws.String(instType), + KeyName: aws.String(keyName), + SubnetId: aws.String(def.SubnetID), + MinCount: aws.Int64(1), + MaxCount: aws.Int64(1), + UserData: aws.String(base64.StdEncoding.EncodeToString([]byte(egressUserData))), + }) + require.NoError(t, err, "run-instances") + require.NotEmpty(t, runOut.Instances, "run-instances returned no Instances") + instanceID := aws.StringValue(runOut.Instances[0].InstanceId) + t.Cleanup(func() { + _, _ = fix.aws.EC2.TerminateInstances(&ec2.TerminateInstancesInput{ + InstanceIds: []*string{aws.String(instanceID)}, + }) + harness.WaitForInstanceTerminated(t, fix.aws, []string{instanceID}, 5*time.Minute) + }) + harness.Detail(t, "instance", instanceID) + + inst := harness.WaitForInstanceState(t, fix.aws, instanceID, "running") + + if fix.publicPool { + harness.Step(t, "instance auto-assigned a public IP (MapPublicIpOnLaunch)") + assert.NotEmptyf(t, aws.StringValue(inst.PublicIpAddress), + "instance %s must auto-assign a public IP with a public pool configured", instanceID) + harness.Detail(t, "public_ip", aws.StringValue(inst.PublicIpAddress)) + } else { + harness.Step(t, "instance has no public IP") + assert.Emptyf(t, aws.StringValue(inst.PublicIpAddress), + "Tier-1-only nat mode instance %s must not get a public IP (got %q)", + instanceID, aws.StringValue(inst.PublicIpAddress)) + } + assert.NotEmptyf(t, aws.StringValue(inst.PrivateIpAddress), + "instance %s missing private IP", instanceID) + + harness.Step(t, "wait for egress verdict on serial console") + var console string + harness.EventuallyErr(t, func() error { + console = consoleOutput(t, fix, instanceID) + if strings.Contains(console, egressOKMarker) || strings.Contains(console, egressFailMarker) { + return nil + } + return fmt.Errorf("no egress marker on console yet (%d bytes)", len(console)) + }, 8*time.Minute, 10*time.Second) + + if strings.Contains(console, egressFailMarker) { + harness.DumpFile(t, fix.artifacts, "egress-console.log", []byte(console)) + t.Fatalf("guest reported %s — outbound WAN unreachable through routed NAT (console saved to artifacts)", egressFailMarker) + } + harness.Step(t, "guest reported %s", egressOKMarker) + + var sgID string + if len(inst.SecurityGroups) > 0 { + sgID = aws.StringValue(inst.SecurityGroups[0].GroupId) + } + return egressProbe{ + instanceID: instanceID, + privateIP: aws.StringValue(inst.PrivateIpAddress), + publicIP: aws.StringValue(inst.PublicIpAddress), + sgID: sgID, + } +} + +// --- Phase 7: Tier 1 host ingress ----------------------------------------------- + +// phaseHostIngress proves the automatic routed-NAT ingress tier: the host has +// a route to the VPC CIDR via the gateway LRP transit IP, the SNAT rule +// carries the exempt Address_Set ref (so VM replies to host-initiated flows +// are not SNATted), and — after opening the SG like on AWS — the host can +// ping the instance's private IP and complete a TCP handshake to sshd. +func phaseHostIngress(t *testing.T, fix *fixture, def harness.VPCInfo, probe egressProbe) { + t.Helper() + + gwIP := gatewayLRPIP(t, def.VPCID) + require.NotEmpty(t, gwIP, "gateway LRP IP") + vpcCIDR := describeVPCCIDR(t, fix, def.VPCID) + + harness.Step(t, "host route %s via %s dev %s", vpcCIDR, gwIP, transitHostEnd) + routeOut := hostCmd(t, "ip", "route", "show", vpcCIDR) + assert.Containsf(t, routeOut, "via "+gwIP, "VPC ingress route must go via gateway LRP IP\n%s", routeOut) + assert.Containsf(t, routeOut, "dev "+transitHostEnd, "VPC ingress route must use %s\n%s", transitHostEnd, routeOut) + + harness.Step(t, "SNAT rule carries exempted_ext_ips -> %s", natExemptSetName) + exemptRef := strings.TrimSpace(harness.OvnNbctl(t, "--bare", "--columns=exempted_ext_ips", + "find", "nat", "type=snat", "logical_ip="+vpcCIDR)) + require.NotEmptyf(t, exemptRef, "snat rule for %s has no exempted_ext_ips ref", vpcCIDR) + setAddrs := harness.OvnNbctl(t, "--bare", "--columns=addresses", + "find", "address_set", "name="+natExemptSetName) + assert.Containsf(t, setAddrs, transitCIDR, + "%s must contain the transit CIDR %s\n%s", natExemptSetName, transitCIDR, setAddrs) + + harness.Step(t, "open SG for ICMP + SSH from transit net (default-closed, AWS parity)") + require.NotEmpty(t, probe.sgID, "probe instance has no security group") + perms := []*ec2.IpPermission{ + { + IpProtocol: aws.String("icmp"), FromPort: aws.Int64(-1), ToPort: aws.Int64(-1), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String(transitCIDR)}}, + }, + { + IpProtocol: aws.String("tcp"), FromPort: aws.Int64(22), ToPort: aws.Int64(22), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String(transitCIDR)}}, + }, + } + _, err := fix.aws.EC2.AuthorizeSecurityGroupIngress(&ec2.AuthorizeSecurityGroupIngressInput{ + GroupId: aws.String(probe.sgID), IpPermissions: perms, + }) + require.NoError(t, err, "authorize-security-group-ingress") + t.Cleanup(func() { + _, _ = fix.aws.EC2.RevokeSecurityGroupIngress(&ec2.RevokeSecurityGroupIngressInput{ + GroupId: aws.String(probe.sgID), IpPermissions: perms, + }) + }) + + harness.Step(t, "ping instance private IP %s from host", probe.privateIP) + harness.EventuallyErr(t, func() error { + out, perr := exec.Command("ping", "-c", "1", "-W", "3", probe.privateIP).CombinedOutput() + if perr != nil { + return fmt.Errorf("ping %s: %v: %s", probe.privateIP, perr, string(out)) + } + return nil + }, 2*time.Minute, 5*time.Second) + + harness.Step(t, "TCP handshake to sshd on %s:22 from host", probe.privateIP) + harness.EventuallyErr(t, func() error { + return sshHandshake(probe.privateIP) + }, 2*time.Minute, 5*time.Second) + harness.Step(t, "host->instance return path works without SNAT mangling") +} + +// --- Phase 7: unique transit IP per VPC ----------------------------------------- + +// phaseUniqueTransitIP creates a second VPC and attaches an IGW, then asserts +// its gateway LRP gets a transit IP distinct from the default VPC's — the +// regression guard for the duplicate-gateway-IP failure in field report #471. +func phaseUniqueTransitIP(t *testing.T, fix *fixture, defaultGwIP string) { + t.Helper() + + harness.Step(t, "create-vpc %s", secondVPCCIDR) + // e2e:allow-create — scratch VPC owned end to end by this uniqueness check. + vpcOut, err := fix.aws.EC2.CreateVpc(&ec2.CreateVpcInput{CidrBlock: aws.String(secondVPCCIDR)}) + require.NoError(t, err, "create-vpc") + vpcID := aws.StringValue(vpcOut.Vpc.VpcId) + t.Cleanup(func() { + _, _ = fix.aws.EC2.DeleteVpc(&ec2.DeleteVpcInput{VpcId: aws.String(vpcID)}) + }) + + harness.Step(t, "create + attach internet gateway") + // e2e:allow-create — scratch IGW owned by this uniqueness check. + igwOut, err := fix.aws.EC2.CreateInternetGateway(&ec2.CreateInternetGatewayInput{}) + require.NoError(t, err, "create-internet-gateway") + igwID := aws.StringValue(igwOut.InternetGateway.InternetGatewayId) + t.Cleanup(func() { + _, _ = fix.aws.EC2.DetachInternetGateway(&ec2.DetachInternetGatewayInput{ + InternetGatewayId: aws.String(igwID), VpcId: aws.String(vpcID), + }) + _, _ = fix.aws.EC2.DeleteInternetGateway(&ec2.DeleteInternetGatewayInput{ + InternetGatewayId: aws.String(igwID), + }) + }) + _, err = fix.aws.EC2.AttachInternetGateway(&ec2.AttachInternetGatewayInput{ + InternetGatewayId: aws.String(igwID), VpcId: aws.String(vpcID), + }) + require.NoError(t, err, "attach-internet-gateway") + + harness.Step(t, "wait for gateway LRP on transit net") + var gwIP string + harness.EventuallyErr(t, func() error { + gwIP = gatewayLRPIP(t, vpcID) + if gwIP == "" { + return fmt.Errorf("gw-%s has no networks yet", vpcID) + } + return nil + }, 2*time.Minute, 3*time.Second) + + harness.Detail(t, "second_gw_lrp_ip", gwIP, "default_gw_lrp_ip", defaultGwIP) + assert.Truef(t, strings.HasPrefix(gwIP, "100.127.0."), + "second VPC gateway LRP IP %s must be on %s", gwIP, transitCIDR) + assert.NotEqualf(t, defaultGwIP, gwIP, + "each VPC gateway LRP must get a unique transit IP (both got %s)", gwIP) + + natList := harness.OvnNbctl(t, "lr-nat-list", "vpc-"+vpcID) + assert.Containsf(t, natList, secondVPCCIDR, + "second VPC router missing snat for %s\n%s", secondVPCCIDR, natList) + + harness.Step(t, "host ingress route for second VPC installed on attach") + harness.EventuallyErr(t, func() error { + out, _ := exec.Command("ip", "route", "show", secondVPCCIDR).CombinedOutput() + if !strings.Contains(string(out), "via "+gwIP) { + return fmt.Errorf("route %s via %s not present yet: %s", secondVPCCIDR, gwIP, string(out)) + } + return nil + }, 2*time.Minute, 3*time.Second) +} + +// --- Phase 9: Tier 2 EIP ingress lifecycle ----------------------------------- + +// phaseEIPIngress proves EIP parity with pool mode on a routed-NAT node: the +// auto-assigned public IP and a freshly associated EIP both get the host +// delivery trio (/32 route into OVN, proxy-ARP on the uplink, per-EIP FORWARD +// accepts) plus an exempt-stamped dnat_and_snat row; a TCP handshake to the +// EIP proves the DNAT path end to end; a vpcd restart must replay the host +// bindings; disassociating must tear them down. +func phaseEIPIngress(t *testing.T, fix *fixture, def harness.VPCInfo, probe egressProbe) { + t.Helper() + + gwIP := gatewayLRPIP(t, def.VPCID) + require.NotEmpty(t, gwIP, "gateway LRP IP") + + harness.Step(t, "auto-assigned public IP %s has host delivery plumbing", probe.publicIP) + require.NotEmpty(t, probe.publicIP, "phase 6 probe carries no public IP") + harness.EventuallyErr(t, func() error { + return eipHostPlumbing(probe.publicIP, gwIP) + }, 2*time.Minute, 3*time.Second) + assertDNATExempt(t, probe.publicIP) + + harness.Step(t, "allocate + associate a fresh EIP") + // e2e:allow-create — scratch EIP owned end to end by this phase. + alloc, err := fix.aws.EC2.AllocateAddress(&ec2.AllocateAddressInput{Domain: aws.String("vpc")}) + require.NoError(t, err, "allocate-address") + allocID := aws.StringValue(alloc.AllocationId) + eip := aws.StringValue(alloc.PublicIp) + require.NotEmpty(t, eip, "allocate-address returned no PublicIp") + t.Cleanup(func() { + _, _ = fix.aws.EC2.ReleaseAddress(&ec2.ReleaseAddressInput{AllocationId: aws.String(allocID)}) + }) + harness.Detail(t, "allocation", allocID, "eip", eip) + + assocOut, err := fix.aws.EC2.AssociateAddress(&ec2.AssociateAddressInput{ + AllocationId: aws.String(allocID), + InstanceId: aws.String(probe.instanceID), + }) + require.NoError(t, err, "associate-address %s -> %s", allocID, probe.instanceID) + assocID := aws.StringValue(assocOut.AssociationId) + + harness.Step(t, "host delivery plumbing lands for %s", eip) + harness.EventuallyErr(t, func() error { + return eipHostPlumbing(eip, gwIP) + }, 2*time.Minute, 3*time.Second) + assertDNATExempt(t, eip) + + harness.Step(t, "open SG for SSH from anywhere, TCP handshake to EIP %s:22", eip) + perms := []*ec2.IpPermission{{ + IpProtocol: aws.String("tcp"), FromPort: aws.Int64(22), ToPort: aws.Int64(22), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String("0.0.0.0/0")}}, + }} + _, err = fix.aws.EC2.AuthorizeSecurityGroupIngress(&ec2.AuthorizeSecurityGroupIngressInput{ + GroupId: aws.String(probe.sgID), IpPermissions: perms, + }) + require.NoError(t, err, "authorize-security-group-ingress") + t.Cleanup(func() { + _, _ = fix.aws.EC2.RevokeSecurityGroupIngress(&ec2.RevokeSecurityGroupIngressInput{ + GroupId: aws.String(probe.sgID), IpPermissions: perms, + }) + }) + harness.EventuallyErr(t, func() error { + return sshHandshake(eip) + }, 2*time.Minute, 5*time.Second) + + harness.Step(t, "vpcd restart replays host EIP bindings (reconcile)") + if out, rerr := exec.Command("sudo", "-n", "ip", "route", "del", eip+"/32", + "dev", transitHostEnd).CombinedOutput(); rerr != nil { + t.Logf("route del before restart failed (continuing): %s", string(out)) + } + if out, rerr := exec.Command("sudo", "-n", "systemctl", "restart", + "spinifex-vpcd").CombinedOutput(); rerr != nil { + t.Logf("skipping reconcile-replay check — cannot restart spinifex-vpcd: %s", string(out)) + } else { + harness.EventuallyErr(t, func() error { + return eipHostPlumbing(eip, gwIP) + }, 3*time.Minute, 5*time.Second) + } + + harness.Step(t, "disassociate tears down host delivery for %s", eip) + _, err = fix.aws.EC2.DisassociateAddress(&ec2.DisassociateAddressInput{ + AssociationId: aws.String(assocID), + }) + require.NoError(t, err, "disassociate-address %s", assocID) + harness.EventuallyErr(t, func() error { + return eipHostPlumbingGone(eip) + }, 2*time.Minute, 3*time.Second) + + harness.Step(t, "auto-assigned public IP %s still plumbed after EIP teardown", probe.publicIP) + require.NoError(t, eipHostPlumbing(probe.publicIP, gwIP)) +} + +// eipHostPlumbing returns nil when the full Tier 2 host state for eip is in +// place: the /32 route into OVN via the gateway LRP, a proxy-ARP neighbor on +// the uplink, and both per-EIP FORWARD accepts. +func eipHostPlumbing(eip, gwIP string) error { + out, _ := exec.Command("ip", "route", "show", eip+"/32").CombinedOutput() + route := string(out) + if !strings.Contains(route, "via "+gwIP) || !strings.Contains(route, "dev "+transitHostEnd) { + return fmt.Errorf("EIP route for %s missing (want via %s dev %s): %q", + eip, gwIP, transitHostEnd, strings.TrimSpace(route)) + } + out, _ = exec.Command("ip", "neigh", "show", "proxy").CombinedOutput() + if !strings.Contains(string(out), eip) { + return fmt.Errorf("proxy-ARP entry for %s missing:\n%s", eip, string(out)) + } + for _, args := range eipForwardChecks(eip) { + if out, err := exec.Command("sudo", append([]string{"-n", "iptables"}, args...)...).CombinedOutput(); err != nil { + return fmt.Errorf("iptables %s missing: %s", strings.Join(args, " "), string(out)) + } + } + return nil +} + +// eipHostPlumbingGone returns nil when no host delivery state remains for eip. +func eipHostPlumbingGone(eip string) error { + out, _ := exec.Command("ip", "route", "show", eip+"/32").CombinedOutput() + if strings.TrimSpace(string(out)) != "" { + return fmt.Errorf("EIP route for %s still present: %q", eip, strings.TrimSpace(string(out))) + } + out, _ = exec.Command("ip", "neigh", "show", "proxy").CombinedOutput() + if strings.Contains(string(out), eip) { + return fmt.Errorf("proxy-ARP entry for %s still present", eip) + } + for _, args := range eipForwardChecks(eip) { + if _, err := exec.Command("sudo", append([]string{"-n", "iptables"}, args...)...).CombinedOutput(); err == nil { + return fmt.Errorf("iptables rule still present: %s", strings.Join(args, " ")) + } + } + return nil +} + +func eipForwardChecks(eip string) [][]string { + return [][]string{ + {"-C", "FORWARD", "-i", transitHostEnd, "-s", eip + "/32", + "-m", "comment", "--comment", "spinifex-eip-ingress", "-j", "ACCEPT"}, + {"-C", "FORWARD", "-o", transitHostEnd, "-d", eip + "/32", + "-m", "comment", "--comment", "spinifex-eip-ingress", "-j", "ACCEPT"}, + } +} + +// assertDNATExempt checks the dnat_and_snat row for eip exists and carries +// the exempt Address_Set ref (so Tier 1 host->private-IP flows keep working +// for EIP-holding instances). +func assertDNATExempt(t *testing.T, eip string) { + t.Helper() + exemptRef := strings.TrimSpace(harness.OvnNbctl(t, "--bare", "--columns=exempted_ext_ips", + "find", "nat", "type=dnat_and_snat", "external_ip="+eip)) + require.NotEmptyf(t, exemptRef, "dnat_and_snat for %s missing or has no exempted_ext_ips ref", eip) +} + +// sshHandshake dials hostPort:22 and reads the SSH banner prefix. +func sshHandshake(host string) error { + conn, err := net.DialTimeout("tcp", net.JoinHostPort(host, "22"), 5*time.Second) + if err != nil { + return fmt.Errorf("dial %s:22: %w", host, err) + } + defer conn.Close() + _ = conn.SetReadDeadline(time.Now().Add(5 * time.Second)) + banner := make([]byte, 4) + if _, err := io.ReadFull(conn, banner); err != nil { + return fmt.Errorf("read ssh banner: %w", err) + } + if string(banner) != "SSH-" { + return fmt.Errorf("unexpected banner prefix %q", string(banner)) + } + return nil +} + +// --- Helpers --------------------------------------------------------------- + +// hostCmd runs a local (non-sudo) command and fails the test on error. +func hostCmd(t *testing.T, name string, args ...string) string { + t.Helper() + out, err := exec.Command(name, args...).CombinedOutput() + require.NoErrorf(t, err, "%s %s: %s", name, strings.Join(args, " "), string(out)) + return string(out) +} + +func configPath(env *harness.Env) string { + if env.ConfigDir != "" { + return filepath.Join(env.ConfigDir, "spinifex.toml") + } + return os.ExpandEnv("$HOME/spinifex/config/spinifex.toml") +} + +// readExternalMode extracts external_mode from the [network] block of +// spinifex.toml. Returns "" when the file or key is absent. +func readExternalMode(t *testing.T, path string) string { + t.Helper() + f, err := os.Open(path) + if err != nil { + return "" + } + defer f.Close() + + scanner := bufio.NewScanner(f) + inNetwork := false + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + if strings.HasPrefix(line, "[") { + inNetwork = line == "[network]" + continue + } + if !inNetwork || !strings.HasPrefix(line, "external_mode") { + continue + } + if i := strings.IndexByte(line, '='); i >= 0 { + return strings.Trim(strings.TrimSpace(line[i+1:]), "\"'") + } + } + return "" +} + +// hasPublicPool reports whether spinifex.toml carries an external pool other +// than the transit pool — the marker that the Tier 2 (EIP / public IP) lane +// is configured on this node. +func hasPublicPool(t *testing.T, path string) bool { + t.Helper() + f, err := os.Open(path) + if err != nil { + return false + } + defer f.Close() + + scanner := bufio.NewScanner(f) + inPool := false + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + if strings.HasPrefix(line, "[") { + inPool = line == "[[network.external_pools]]" + continue + } + if !inPool || !strings.HasPrefix(line, "name") { + continue + } + if i := strings.IndexByte(line, '='); i >= 0 { + name := strings.Trim(strings.TrimSpace(line[i+1:]), "\"'") + if name != "nat-transit" { + return true + } + } + } + return false +} + +// gatewayLRPIP returns the IP (sans prefix) of gw-'s first network, +// or "" when the LRP does not exist yet. +func gatewayLRPIP(t *testing.T, vpcID string) string { + t.Helper() + out := harness.OvnNbctl(t, "--bare", "--columns=networks", + "find", "logical_router_port", "name=gw-"+vpcID) + fields := strings.Fields(out) + if len(fields) == 0 { + return "" + } + ip, _, ok := strings.Cut(fields[0], "/") + if !ok { + return fields[0] + } + return ip +} + +func describeVPCCIDR(t *testing.T, fix *fixture, vpcID string) string { + t.Helper() + out, err := fix.aws.EC2.DescribeVpcs(&ec2.DescribeVpcsInput{ + VpcIds: []*string{aws.String(vpcID)}, + }) + require.NoError(t, err, "describe-vpcs %s", vpcID) + require.NotEmpty(t, out.Vpcs, "vpc %s not found", vpcID) + return aws.StringValue(out.Vpcs[0].CidrBlock) +} + +func consoleOutput(t *testing.T, fix *fixture, instanceID string) string { + t.Helper() + out, err := fix.aws.EC2.GetConsoleOutput(&ec2.GetConsoleOutputInput{ + InstanceId: aws.String(instanceID), + }) + if err != nil { + return "" + } + encoded := aws.StringValue(out.Output) + if encoded == "" { + return "" + } + raw, derr := base64.StdEncoding.DecodeString(encoded) + if derr != nil { + return encoded + } + return string(raw) +}