diff --git a/cmd/galactic-nat/nat.go b/cmd/galactic-nat/nat.go index e27560c5..33d91f79 100644 --- a/cmd/galactic-nat/nat.go +++ b/cmd/galactic-nat/nat.go @@ -7,13 +7,13 @@ package main import ( "fmt" "log/slog" - "net/netip" - "sync/atomic" + "sync" "github.com/cilium/ebpf/link" "github.com/prometheus/client_golang/prometheus" "go.datum.net/galactic/internal/config" + "go.datum.net/galactic/internal/controller" "go.datum.net/galactic/internal/plumbing/ebpf/natattach" "go.datum.net/galactic/internal/plumbing/ebpf/natmap" "go.datum.net/galactic/internal/plumbing/ebpf/natprog" @@ -36,111 +36,143 @@ var natDatapathKeepAlive struct { links []link.Link } -// natDatapathStatus reports whether setup has completed a successful load, -// attach, and configure pass. The flag is set true once, after every step -// succeeds, and never cleared: there is no runtime detach detection, the -// datapath being expected to survive the process's whole lifetime. That makes -// an atomic sufficient, it being written once and read concurrently by every -// later reconcile. -type natDatapathStatus struct { - attached atomic.Bool +// natDatapath is this node's attached egress translation datapath, as +// controller.EgressShardReconciler drives it (controller.EgressDatapath). +// +// Attachment happens once, at startup, and needs no identity: until Program +// writes one, shard_config_table holds a row serving no family and the +// datapath claims no packet. Program and Clear are called from the reconciler +// only, but Attached and Programmed are read concurrently with them, hence the +// mutex. +type natDatapath struct { + uplinks []string + shardConfig *natmap.ShardConfigTable + + mu sync.Mutex + attached bool } -func (s *natDatapathStatus) Attached() bool { return s.attached.Load() } +// Attached reports whether startup completed its load and attach pass. It is +// set once and never cleared: there is no runtime detach detection, the +// datapath being expected to survive the process's whole lifetime. +func (d *natDatapath) Attached() bool { + d.mu.Lock() + defer d.mu.Unlock() + return d.attached +} -// shardConfigFromFlags translates validated process configuration into the row -// the datapath reads. Config validation has already rejected a half-configured -// family, so an unset address here means that family is genuinely off rather -// than missing. -func shardConfigFromFlags(cfg *config.NATConfig) (natmap.ShardConfig, error) { - sidAddr, err := netip.ParseAddr(cfg.ShardSID) - if err != nil { - return natmap.ShardConfig{}, fmt.Errorf("parse shard SID %q: %w", cfg.ShardSID, err) +// Program writes identity into shard_config_table. A blind overwrite: see +// natmap.ShardConfigTable.Set. +func (d *natDatapath) Program(identity controller.EgressShardIdentity) error { + d.mu.Lock() + defer d.mu.Unlock() + + // The IPv4 counterpart of the forwarding sysctls startup sets, needed for + // the same reason on the one leg that leaves this datapath as IPv4: a + // NAT64 forward packet resolves its next hop with an IPv4 FIB lookup, + // which a kernel with IPv4 forwarding off refuses. The datapath counts + // that refusal, but a shard whose every IPv4 flow dies at the last + // instruction is a shard that does not work. Set only once a shard + // actually serves NAT64, so a node that never does keeps IPv4 + // forwarding as it was. + if identity.ShardAddressIPv4.IsValid() { + for _, iface := range d.uplinks { + if err := sysctl.ConfigureFIBLookupUplinkSysctlsIPv4(iface); err != nil { + return fmt.Errorf("configure IPv4 forwarding on uplink interface %q: %w", iface, err) + } + } } - shardCfg := natmap.ShardConfig{ - ShardSID: sidAddr, + cfg := natmap.ShardConfig{ + ShardSID: identity.ShardSID, + ShardPubAddr6: identity.ShardAddressIPv6, + ShardPubAddr4: identity.ShardAddressIPv4, + NAT64Prefix: identity.NAT64Prefix, } - - if cfg.ServesNAT66() { - pubAddr, err := netip.ParseAddr(cfg.ShardPubAddr6) - if err != nil { - return natmap.ShardConfig{}, fmt.Errorf("parse shard public address %q: %w", cfg.ShardPubAddr6, err) - } - shardCfg.ShardPubAddr6 = pubAddr + if current, ok, err := d.shardConfig.Get(); err == nil && ok && current == cfg { + return nil + } + if err := d.shardConfig.Set(cfg); err != nil { + return err } + slog.Info("Egress translation datapath programmed", + "shardSID", identity.ShardSID, + "nat66", identity.ShardAddressIPv6.IsValid(), + "nat64", identity.ShardAddressIPv4.IsValid(), + ) + return nil +} - if cfg.ServesNAT64() { - pubAddr4, err := netip.ParseAddr(cfg.ShardPubAddr4) - if err != nil { - return natmap.ShardConfig{}, fmt.Errorf( - "parse shard public IPv4 address %q: %w", cfg.ShardPubAddr4, err) - } - prefix, err := netip.ParsePrefix(cfg.NAT64Prefix) - if err != nil { - return natmap.ShardConfig{}, fmt.Errorf("parse NAT64 prefix %q: %w", cfg.NAT64Prefix, err) - } - shardCfg.ShardPubAddr4 = pubAddr4 - shardCfg.NAT64Prefix = prefix +// Clear removes the programmed identity, so the datapath claims no packet. +func (d *natDatapath) Clear() error { + d.mu.Lock() + defer d.mu.Unlock() + + if _, ok, err := d.shardConfig.Get(); err == nil && !ok { + return nil } + if err := d.shardConfig.Clear(); err != nil { + return err + } + slog.Info("Egress translation datapath cleared; claiming no packets") + return nil +} - return shardCfg, nil +// Programmed reads back the identity shard_config_table holds. A read failure +// reports no identity, which publishes as an empty status rather than as one +// the datapath might not be translating with. +func (d *natDatapath) Programmed() (controller.EgressShardIdentity, bool) { + d.mu.Lock() + defer d.mu.Unlock() + + cfg, ok, err := d.shardConfig.Get() + if err != nil || !ok { + return controller.EgressShardIdentity{}, false + } + return controller.EgressShardIdentity{ + ShardSID: cfg.ShardSID, + ShardAddressIPv6: cfg.ShardPubAddr6, + ShardAddressIPv4: cfg.ShardPubAddr4, + NAT64Prefix: cfg.NAT64Prefix, + }, true } // setupNatDatapath loads and attaches the egress translation datapath to every -// one of this shard's uplinks, writes its identity into the config map, and -// registers this shard's metrics. It returns the health reporter the reconciler -// uses for its Ready condition. +// one of this node's uplinks and registers this shard's metrics. It returns +// the datapath the reconciler programs from the node's EgressShard. // -// Every configured uplink is attached, not only the one this node's traffic -// uses today: a packet arriving on an uplink with no program reaches no -// translation at all and leaves untranslated and uncounted, so a multi-homed -// shard node that attached to one uplink would lose the shard role the moment -// routing moved. Attachment is all-or-nothing -- natattach.Attach unwinds its -// own partial work -- so a shard that cannot claim every uplink fails to start -// rather than running with a hole in its coverage. +// Uplinks come from natattach.ResolveUplinks: the override when one is +// configured, otherwise the same auto-detection the CNI's SRv6 datapath uses. +// Every one is attached, not only the one this node's traffic uses today: a +// packet arriving on an uplink with no program reaches no translation at all +// and leaves untranslated and uncounted, so a multi-homed shard node that +// attached to one uplink would lose the shard role the moment routing moved. +// Attachment is all-or-nothing -- natattach.Attach unwinds its own partial +// work -- so a shard that cannot claim every uplink fails to start rather than +// running with a hole in its coverage. // // The loaded objects and every returned link are stashed in natDatapathKeepAlive // rather than closed here: they, and the attachment itself, must survive for the // life of this process. -func setupNatDatapath(cfg *config.NATConfig, metricsReg prometheus.Registerer) (*natDatapathStatus, error) { - shardCfg, err := shardConfigFromFlags(cfg) - if err != nil { - return nil, err - } - - // A bonding master is attached through its slaves, so every step below - // works on the resolved targets rather than the configured names. - targets, err := natattach.ResolveTargets(cfg.UplinkInterfaces) +func setupNatDatapath(cfg *config.NATConfig, metricsReg prometheus.Registerer) (*natDatapath, error) { + uplinks, err := natattach.ResolveUplinks(cfg.UplinkInterfaces) if err != nil { - return nil, fmt.Errorf("resolve uplink interfaces %v: %w", cfg.UplinkInterfaces, err) + return nil, fmt.Errorf("resolve uplink interfaces: %w", err) } // Required for the FIB lookup in both the forward and return paths to // succeed on the interface the program actually runs on. The lookup uses - // the ingress interface, meaning whichever resolved target the packet - // arrived on, so this must be applied to every one of them rather than to - // a primary: once an uplink is a bond, its slaves rather than the master - // are what the kernel reports as ingress. Best-effort and non-fatal, - // matching how the gateway binary configures the same sysctls. - for _, iface := range targets { + // the ingress interface, meaning whichever uplink the packet arrived on, so + // this must be applied to every one of them rather than to a primary: + // once an uplink is a bond, its slaves rather than the master are what the + // kernel reports as ingress, and uplinks already holds the slaves. + // Best-effort and non-fatal, matching how the gateway binary configures the + // same sysctls. + for _, iface := range uplinks { if err := sysctl.ConfigureFIBLookupUplinkSysctls(iface); err != nil { return nil, fmt.Errorf("configure IPv6 forwarding on uplink interface %q: %w", iface, err) } } - // The IPv4 counterpart, needed for the same reason on the one leg that - // leaves this datapath as IPv4: a NAT64 forward packet resolves its next - // hop with an IPv4 FIB lookup, which a kernel with IPv4 forwarding off - // refuses. The datapath counts that refusal, but a shard whose every IPv4 - // flow dies at the last instruction is a shard that does not work. - if cfg.ServesNAT64() { - for _, iface := range targets { - if err := sysctl.ConfigureFIBLookupUplinkSysctlsIPv4(iface); err != nil { - return nil, fmt.Errorf( - "configure IPv4 forwarding on uplink interface %q: %w", iface, err) - } - } - } objs, err := natattach.Load(natattach.PinDir) if err != nil { @@ -154,17 +186,16 @@ func setupNatDatapath(cfg *config.NATConfig, metricsReg prometheus.Registerer) ( return nil, fmt.Errorf("populate egress translation tail-call array: %w", err) } - shardConfigTable := natmap.NewShardConfigTable(natmap.KernelTable{Map: objs.ShardConfigTable}) - if err := shardConfigTable.Set(shardCfg); err != nil { - _ = objs.Close() - return nil, fmt.Errorf("write shard_config_table: %w", err) - } + // shard_config_table is deliberately not written here. Its pinned row is + // whatever this node's previous process programmed, so a restart keeps + // translating established flows until the first reconcile re-derives it + // from the EgressShard spec -- or clears it, if that shard is gone. + shardConfig := natmap.NewShardConfigTable(natmap.KernelTable{Map: objs.ShardConfigTable}) - xdpLinks, err := natattach.Attach(objs.NatIngress, targets) + xdpLinks, err := natattach.Attach(objs.NatIngress, uplinks) if err != nil { _ = objs.Close() - return nil, fmt.Errorf("attach egress translation datapath to uplink interfaces %v: %w", - cfg.UplinkInterfaces, err) + return nil, fmt.Errorf("attach egress translation datapath to uplink interfaces %v: %w", uplinks, err) } collector := newNatCollector(objs) @@ -178,16 +209,11 @@ func setupNatDatapath(cfg *config.NATConfig, metricsReg prometheus.Registerer) ( natDatapathKeepAlive.links = xdpLinks slog.Info("Egress translation datapath attached", - "interfaces", cfg.UplinkInterfaces, - "targets", targets, - "shardSID", cfg.ShardSID, - "nat66", cfg.ServesNAT66(), - "nat64", cfg.ServesNAT64(), + "interfaces", uplinks, + "autoDetected", len(cfg.UplinkInterfaces) == 0, ) - status := &natDatapathStatus{} - status.attached.Store(true) - return status, nil + return &natDatapath{uplinks: uplinks, shardConfig: shardConfig, attached: true}, nil } // closeAll best-effort closes every link, to unwind a partially set-up datapath diff --git a/cmd/galactic-nat/root.go b/cmd/galactic-nat/root.go index 1cd99bff..a72cb58d 100644 --- a/cmd/galactic-nat/root.go +++ b/cmd/galactic-nat/root.go @@ -37,9 +37,10 @@ const ( Find more information at: https://www.datum.net/docs` ) -// runCmd is the application startup: it loads and attaches this shard's NAT66 -// egress datapath to its fabric-facing uplink and registers the reconciler that -// publishes this shard's identity and health. +// runCmd is the application startup: it loads and attaches this node's egress +// translation datapath to its fabric-facing uplinks and registers the +// reconciler that programs it from this node's EgressShard and publishes the +// result. func runCmd(cfg *config.NATConfig) error { nodeName := cfg.NodeName metricsPort := cfg.MetricsPort @@ -111,28 +112,26 @@ func runCmd(cfg *config.NATConfig) error { // Pre-flight RBAC check. checkWatchPermissions(mgr) - // Load and attach the egress translation datapath. Always a real datapath: - // configuration validation rejects an empty uplink or SID, and a shard - // serving neither address family, before this is reached -- this binary - // exists only to run a shard. - datapathHealth, err := setupNatDatapath(cfg, ctrlmetrics.Registry) + // Load and attach the egress translation datapath. It needs no identity to + // attach: that comes from this node's EgressShard spec, which the + // reconciler below programs on its first reconcile. + datapath, err := setupNatDatapath(cfg, ctrlmetrics.Registry) if err != nil { return fmt.Errorf("setup egress translation eBPF datapath: %w", err) } // Only now is the datapath attached. Report serving from here on, not from - // process start. + // process start. Serving does not wait for an identity to be programmed: + // a node whose shard has not been assigned one yet would otherwise never + // turn ready and would block the DaemonSet's rollout. The EgressShard's + // Programmed condition reports that instead. healthSrv.SetServingStatus("", grpc_health_v1.HealthCheckResponse_SERVING) // Register EgressShard controller. if err := (&controller.EgressShardReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - NodeName: nodeName, - ShardSID: cfg.ShardSID, - ShardAddressIPv6: cfg.ShardPubAddr6, - ShardAddressIPv4: cfg.ShardPubAddr4, - NAT64Prefix: cfg.NAT64Prefix, - Datapath: datapathHealth, + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + NodeName: nodeName, + Datapath: datapath, }).SetupWithManager(mgr); err != nil { return fmt.Errorf("setup EgressShard controller: %w", err) } @@ -184,17 +183,8 @@ func newRootCommand() *cobra.Command { config.DefaultNATGRPCHealthPort, "gRPC health check port") cmd.Flags().StringP("nat-uplink-interfaces", "", "", - "Comma-separated fabric-facing uplink interfaces this shard's XDP datapath attaches to; "+ - "name every fabric uplink, not just the primary (required)") - cmd.Flags().StringP("nat-shard-sid", "", "", - "This shard's own SRv6 uSID, encapsulation target for tenant egress traffic (required)") - cmd.Flags().StringP("nat-shard-pub-addr6", "", "", - "This shard's own publicly-routable IPv6 masquerade source address, enabling NAT66") - cmd.Flags().StringP("nat-shard-pub-addr4", "", "", - "This shard's own publicly-routable IPv4 masquerade source address, enabling NAT64 "+ - "together with --nat64-prefix") - cmd.Flags().StringP("nat64-prefix", "", "", - "Fabric-wide NAT64 /96 this shard translates for; must match what DNS64 synthesizes into") + "Comma-separated fabric-facing uplink interfaces this shard's XDP datapath attaches to, "+ + "overriding auto-detection; name every fabric uplink, not just the primary") cmd.Flags().Bool("build-info", false, "Print build information and exit") cmd.Flags().BoolP("version", "V", false, "Print version and exit") return cmd diff --git a/config/galactic-nat/base/daemonset.yaml b/config/galactic-nat/base/daemonset.yaml index 86455bd2..5f780eb0 100644 --- a/config/galactic-nat/base/daemonset.yaml +++ b/config/galactic-nat/base/daemonset.yaml @@ -30,10 +30,8 @@ spec: # config/galactic-router/overlays/router/daemonset-patch.yaml) -- # not via this label, since those two aren't compute-specific # anymore. A shard's own SID/public addresses must still be - # unique per node (see ../kustomization.yaml), which is why this - # base is meant to be instantiated once per shard node rather than - # applied as-is -- that per-node uniqueness is handled by the - # per-node overlay's env vars, not by the scheduling gate here. + # unique per node, but they live in each node's EgressShard spec, + # not in this DaemonSet, so no per-node overlay is needed for them. # Excludes control-plane outright for the same reason the other # roles do. # @@ -88,54 +86,38 @@ spec: # Set explicitly here anyway so it can't silently drift. - name: GALACTIC_NAT_GRPC_HEALTH_PORT value: "5182" - # GALACTIC_NAT_UPLINK_INTERFACES and _SHARD_SID are - # intentionally left unset here: the interface names are - # deployment-specific and the shard SID must be unique per - # shard node (no in-cluster mechanism yet derives them - # automatically -- see internal/config/nat.go's EnvNATShardSID - # doc comment, the same gap GALACTIC_GATEWAY_SRV6_ADDRESS has - # today). + # Nothing about this shard's identity is set here. Its SID, + # masquerade addresses and NAT64 prefix come from the + # EgressShard whose spec.targetRef names this node, which the + # process programs into its datapath on reconcile. Until one + # exists and assigns them, the datapath is attached but claims + # no packet, and the EgressShard's Programmed condition says + # why. # - # GALACTIC_NAT_UPLINK_INTERFACES is a comma-separated list, and - # a multi-homed shard node must name every fabric uplink rather - # than only the one its traffic uses today: the datapath claims - # a packet only on an interface it is attached to, so an uplink - # left out is one whose tenant traffic is forwarded untranslated - # and uncounted the moment routing moves to it. - # - # So is every address that selects which families this shard - # serves. config.NATConfig.Validate requires the uplink, the - # SID, and at least one family, so applying this base as-is - # produces a crash-looping container. The deployer's per-node - # overlay must set: - # - # GALACTIC_NAT_SHARD_PUB_ADDR6 IPv6 masquerade source, - # enabling NAT66 - # GALACTIC_NAT_SHARD_PUB_ADDR4 IPv4 masquerade source, and - # GALACTIC_NAT_NAT64_PREFIX the fabric-wide /96, - # together enabling NAT64 - # - # The NAT64 pair is all-or-nothing and the prefix must match - # what DNS64 synthesizes into, or the shard translates for a - # prefix nothing resolves to. + # GALACTIC_NAT_UPLINK_INTERFACES is optional: unset, the + # uplinks are auto-detected the way galactic-cni detects its + # own. Set it, comma-separated, on a node where that is + # ambiguous -- typically one whose management NIC carries the + # IPv6 default route -- and name every fabric uplink rather + # than only the one its traffic uses today: the datapath + # claims a packet only on an interface it is attached to, so + # an uplink left out is one whose tenant traffic is forwarded + # untranslated and uncounted the moment routing moves to it. # # Note that nothing in this repo makes either masquerade # address reachable from outside the fabric. A reply comes back # from a host that is not on the fabric, so the underlay or an # upstream announcement has to attract each address to this - # node -- EgressShardReconciler's EVPN advertisement of - # _PUB_ADDR6 reaches other fabric nodes and nothing beyond - # them, and _PUB_ADDR4 is advertised by nothing at all. - # Setting either without that in place yields a shard that + # node -- EgressShardReconciler's EVPN advertisement of the + # IPv6 address reaches other fabric nodes and nothing beyond + # them, and the IPv4 address is advertised by nothing at all. + # Assigning either without that in place yields a shard that # translates outbound traffic and never sees a reply (#549), # with every forward-path counter still green. The origination # has to be for this node specifically: the address must reach # the node whose datapath holds that flow's connection state. # deploy/containerlab/resources/fabric-router/*/frr.conf.*-worker # is a worked example. - # - # See deploy/containerlab/resources/galactic-gateway/ for a - # worked example of the equivalent per-node overlay pattern. ports: - name: metrics containerPort: 9182 diff --git a/config/galactic-nat/base/kustomization.yaml b/config/galactic-nat/base/kustomization.yaml index e607d4c5..c6865bc4 100644 --- a/config/galactic-nat/base/kustomization.yaml +++ b/config/galactic-nat/base/kustomization.yaml @@ -1,10 +1,9 @@ # The single-container egress translation shard pod's shared base -- see -# ../kustomization.yaml for why this isn't applied as-is: this is meant to -# be instantiated once per shard node by a further overlay that pins it to -# one node (kubernetes.io/hostname) and sets that node's own -# GALACTIC_NAT_UPLINK_INTERFACES, _SHARD_SID, and per-family masquerade -# address values, -# mirroring config/galactic-gateway/base/kustomization.yaml's identical +# ../kustomization.yaml for why this isn't applied as-is: a deployer whose +# nodes' uplinks auto-detection cannot pick out applies it through an +# overlay setting GALACTIC_NAT_UPLINK_INTERFACES. The shard's identity is +# never set here; it comes from each node's EgressShard spec. The overlay +# shape mirrors config/galactic-gateway/base/kustomization.yaml's identical # per-node overlay pattern (see # deploy/containerlab/resources/galactic-gateway/ for a worked example of # that pattern applied to a sibling component). diff --git a/config/galactic-nat/kustomization.yaml b/config/galactic-nat/kustomization.yaml index 1573f9e7..b7aa30f8 100644 --- a/config/galactic-nat/kustomization.yaml +++ b/config/galactic-nat/kustomization.yaml @@ -1,14 +1,14 @@ # The sharded egress translation datapath control plane. Unlike # config/galactic-router/'s equivalent kustomization.yaml, this -# deliberately does NOT include base/: base/daemonset.yaml's -# galactic-nat container has no generic default for -# GALACTIC_NAT_UPLINK_INTERFACES or _SHARD_SID (both required, and the -# shard SID must be unique per shard node), nor for the per-family -# addresses that decide whether a shard serves NAT66, NAT64, or both -- -# config.NATConfig.Validate requires at least one family. Applying it as -# shipped here would produce a crash-looping container. -# config/galactic-gateway/ and config/fabric-router/ have the same -# exemption for the same reason. +# deliberately does NOT include base/. A shard's identity -- its SID and +# per-family masquerade addresses -- comes from its EgressShard's spec, and +# its uplinks are auto-detected, so base/daemonset.yaml no longer needs any +# per-node value to start. But auto-detection attaches native XDP to every +# interface carrying the IPv6 default route or a BGP-learned route, and on a +# node where that includes a management NIC the deployer has to name the +# fabric uplinks with GALACTIC_NAT_UPLINK_INTERFACES instead, which only +# they know. config/galactic-gateway/ and config/fabric-router/ have the +# same exemption. # # serviceaccount.yaml/rbac.yaml have no such per-node uniqueness problem -- # they're safe and idempotent to apply cluster-wide regardless of how many diff --git a/deploy/containerlab/README.md b/deploy/containerlab/README.md index 3ece9e01..d37ecb5c 100644 --- a/deploy/containerlab/README.md +++ b/deploy/containerlab/README.md @@ -297,7 +297,7 @@ tenant ingress before the TC decap hook ever ran, silently. | Service | Node-ID range | Identity | |-------------|-------------------|------------------------------------------------| | `0x1` | `0x1000`–`0x1FFF` | Tenant delivery — every node's `BGPRouter` | -| `0x2` | `0x2000`–`0x2FFF` | NAT egress shard (`GALACTIC_NAT_SHARD_SID`) | +| `0x2` | `0x2000`–`0x2FFF` | NAT egress shard (`EgressShard.spec.shardSID`) | | `0x3` | `0x3000`–`0x3FFF` | Edge gateway (`GALACTIC_GATEWAY_SRV6_ADDRESS`) | | `0x4`–`0xD` | — | Unallocated | @@ -401,7 +401,8 @@ site-local `ns60` backend — one anycast service, three sites, four gateways. `verify:nat-datapath` is deliberately kept out of the `verify` chain. - **A shard attaches to its uplinks once, at process startup.** `GALACTIC_NAT_UPLINK_INTERFACES` is a list and every interface in it gets - the shard XDP program, so a dual-homed node like `dfw-worker` keeps + the shard XDP program (set explicitly in this lab: auto-detection would + also claim Kind's `eth0`, which carries each node's IPv6 default route), so a dual-homed node like `dfw-worker` keeps translating across an uplink failure. Nothing watches for link changes afterwards, though: an interface that appears after the process started gets no program until it restarts. diff --git a/deploy/containerlab/Taskfile.yaml b/deploy/containerlab/Taskfile.yaml index 665abea5..42155ef6 100644 --- a/deploy/containerlab/Taskfile.yaml +++ b/deploy/containerlab/Taskfile.yaml @@ -729,9 +729,8 @@ tasks: cp=${site}-control-plane shard6=$(docker exec ${cp} kubectl -n galactic-system get egressshard \ -o jsonpath='{.items[0].status.shardAddressIPv6}') - shard4=$(docker exec ${cp} kubectl -n galactic-system get ds galactic-nat \ - -o jsonpath='{range .spec.template.spec.containers[*].env[*]}{.name}={.value}{"\n"}{end}' \ - | awk -F= '/GALACTIC_NAT_SHARD_PUB_ADDR4/ {print $2}') + shard4=$(docker exec ${cp} kubectl -n galactic-system get egressshard \ + -o jsonpath='{.items[0].status.shardAddressIPv4}') echo "--- ${site} (${node}): ${shard6} / ${shard4} ---" if [ -z "${shard6}" ] || [ -z "${shard4}" ]; then echo " FAIL: shard publishes no masquerade address pair" >&2 @@ -841,11 +840,10 @@ tasks: pod=$(docker exec ${cp} kubectl -n ns10 get pods -o jsonpath='{.items[0].metadata.name}') shard6=$(docker exec {{.SHARD_SITE}}-control-plane kubectl -n galactic-system \ get egressshard -o jsonpath='{.items[0].status.shardAddressIPv6}') - shard4=$(docker exec {{.SHARD_SITE}}-control-plane kubectl -n galactic-system get ds galactic-nat \ - -o jsonpath='{range .spec.template.spec.containers[*].env[*]}{.name}={.value}{"\n"}{end}' \ - | awk -F= '/GALACTIC_NAT_SHARD_PUB_ADDR4/ {print $2}') + shard4=$(docker exec {{.SHARD_SITE}}-control-plane kubectl -n galactic-system \ + get egressshard -o jsonpath='{.items[0].status.shardAddressIPv4}') if [ -z "${shard6}" ]; then echo "FAIL: shard publishes no IPv6 masquerade address" >&2; exit 1; fi - if [ -z "${shard4}" ]; then echo "FAIL: shard has no IPv4 masquerade address configured" >&2; exit 1; fi + if [ -z "${shard4}" ]; then echo "FAIL: shard publishes no IPv4 masquerade address" >&2; exit 1; fi echo "sender ${pod} (${cp}) shard ${shard6} / ${shard4}" rc=0 diff --git a/deploy/containerlab/resources/fabric-router/dfw/frr.conf.dfw-worker b/deploy/containerlab/resources/fabric-router/dfw/frr.conf.dfw-worker index 1c7e31c9..5cc772a0 100644 --- a/deploy/containerlab/resources/fabric-router/dfw/frr.conf.dfw-worker +++ b/deploy/containerlab/resources/fabric-router/dfw/frr.conf.dfw-worker @@ -21,8 +21,8 @@ ipv6 route 2001:db8:ff01::/48 Null0 ipv6 route fc00:0:2::/48 Null0 # This node's egress shard's two masquerade addresses -# (GALACTIC_NAT_SHARD_PUB_ADDR6 and _ADDR4, resources/galactic-nat/dfw/ -# node-patch.yaml), originated into the underlay so a reply from outside the +# (spec.shardAddressIPv6 and spec.shardAddressIPv4, +# resources/galactic-nat/dfw/egressshard.yaml), originated into the underlay so a reply from outside the # fabric can reach the shard that sent the request (#549). # # galactic-nat's EgressShardReconciler advertises the IPv6 address as an EVPN diff --git a/deploy/containerlab/resources/fabric-router/iad/frr.conf.iad-worker b/deploy/containerlab/resources/fabric-router/iad/frr.conf.iad-worker index 95e90c43..1b653861 100644 --- a/deploy/containerlab/resources/fabric-router/iad/frr.conf.iad-worker +++ b/deploy/containerlab/resources/fabric-router/iad/frr.conf.iad-worker @@ -16,8 +16,8 @@ ipv6 route 2001:db8:ff03::/48 Null0 ipv6 route fc00:0:4::/48 Null0 # This node's egress shard's two masquerade addresses -# (GALACTIC_NAT_SHARD_PUB_ADDR6 and _ADDR4, resources/galactic-nat/iad/ -# node-patch.yaml), originated into the underlay so a reply from outside the +# (spec.shardAddressIPv6 and spec.shardAddressIPv4, +# resources/galactic-nat/iad/egressshard.yaml), originated into the underlay so a reply from outside the # fabric can reach the shard that sent the request (#549). galactic-nat # advertises the IPv6 one as an EVPN Type 5 path, which the plain-IPv6- # unicast transit never sees, and the IPv4 one is advertised by nothing at diff --git a/deploy/containerlab/resources/fabric-router/sjc/frr.conf.sjc-worker b/deploy/containerlab/resources/fabric-router/sjc/frr.conf.sjc-worker index ec2f3e7a..fcf94f8f 100644 --- a/deploy/containerlab/resources/fabric-router/sjc/frr.conf.sjc-worker +++ b/deploy/containerlab/resources/fabric-router/sjc/frr.conf.sjc-worker @@ -16,8 +16,8 @@ ipv6 route 2001:db8:ff02::/48 Null0 ipv6 route fc00:0:3::/48 Null0 # This node's egress shard's two masquerade addresses -# (GALACTIC_NAT_SHARD_PUB_ADDR6 and _ADDR4, resources/galactic-nat/sjc/ -# node-patch.yaml), originated into the underlay so a reply from outside the +# (spec.shardAddressIPv6 and spec.shardAddressIPv4, +# resources/galactic-nat/sjc/egressshard.yaml), originated into the underlay so a reply from outside the # fabric can reach the shard that sent the request (#549). galactic-nat # advertises the IPv6 one as an EVPN Type 5 path, which the plain-IPv6- # unicast transit never sees, and the IPv4 one is advertised by nothing at diff --git a/deploy/containerlab/resources/galactic-nat/README.md b/deploy/containerlab/resources/galactic-nat/README.md index 31c1b1a5..77b4d9b7 100644 --- a/deploy/containerlab/resources/galactic-nat/README.md +++ b/deploy/containerlab/resources/galactic-nat/README.md @@ -7,27 +7,22 @@ built on the same base/per-node-overlay shape - `base/` — the lab's patch onto `config/galactic-nat/base` (image override for Kind's locally-built images; see `base/kustomization.yaml` - and `base/nat-lab-patch.yaml`). Not applied directly, same reason - `config/galactic-nat/base` itself isn't (`GALACTIC_NAT_UPLINK_INTERFACES` - and `_SHARD_SID` are required and must be unique per shard node, and at - least one address family has to be turned on). + and `base/nat-lab-patch.yaml`). Not applied directly: each site's + uplinks have to be named, since auto-detection would also claim Kind's + `eth0`, which carries every node's IPv6 default route. - `dfw/`, `sjc/`, `iad/` — one per-site overlay each, per the redesign plan's own §8 suggestion to reuse the three existing site workers (`dfw-worker`, `iad-worker`, `sjc-worker`) as a 3-shard DaemonSet rather - than inventing new lab topology. Each pins the DaemonSet to that site's - own worker via `kubernetes.io/hostname` (`node-patch.yaml`, mirroring - `resources/galactic-gateway//`'s per-node-pin pattern) and - sets that shard's own `GALACTIC_NAT_UPLINK_INTERFACES` and - `GALACTIC_NAT_SHARD_SID`/`_SHARD_PUB_ADDR6` — `dfw` names both of its + than inventing new lab topology. Each site's `node-patch.yaml` sets + only `GALACTIC_NAT_UPLINK_INTERFACES` — `dfw` names both of its dual-homed compute node's uplinks (`bond0,bond1`, one LACP bond to each edge node), `sjc` and `iad` their single `bond0` — each a bond - the shard attaches to through its members — - see each `node-patch.yaml`'s own comments for the exact uFMT 48+16 - encoding and address choices, including the note on why the NAT66 - shards use `Argument=1` rather than reusing the gateway nodes' - `Argument=0` on iad's shared locator. Each site directory also carries - a sample `EgressShard` object (`egressshard.yaml`) targeting that site's - worker node. + the shard attaches to through its members. The shard's identity lives + in that site's `EgressShard` (`egressshard.yaml`), whose spec assigns + the SID, both masquerade addresses and the NAT64 prefix; see its + comments for the exact uFMT 48+16 encoding and address choices. The + `galactic-nat` process on the target worker programs its datapath from + that spec. What this reuses: the three existing site workers as shard nodes (no new lab topology, no new containerlab nodes), and the exact @@ -51,7 +46,7 @@ own, and `EgressShard`/`BGPVRFInstance` (the latter for NPTv6's own `nptv6` field) are installed from the local `../network` checkout by that same script — see its own comments for why. -Each shard's `Status.ShardSID` is advertised as a plain, RT-less +Each shard's `status.shardSID` is advertised as a plain, RT-less BGPAdvertisement by `EgressShardReconciler` (`internal/controller/egressshard_controller.go`) — the same shape `NetworkGatewayReconciler` uses for its own ingress VIP — so every other @@ -68,56 +63,35 @@ necessarily fail loudly here: each site originates its locator as a `/48` into the underlay (`resources/fabric-router/*/frr.conf.*-worker`), so an Argument-bearing SID can still resolve on the sending node and be discarded by that aggregate's `Null0` at the far site instead. The shard -address (`Status.ShardAddressIPv6`) stays a `/128` — it is an ordinary +address (`status.shardAddressIPv6`) stays a `/128` — it is an ordinary masquerade source, not a uSID. `GALACTIC_CNI_EGRESS_SHARD_SIDS` (set identically on every site's -`galactic-cni` DaemonSet, `resources/galactic-cni/daemonset-patch.yaml`) +`galactic-cni` DaemonSet, `resources/galactic-cni/shared/daemonset-patch.yaml`) carries the fabric-wide membership list every compute node needs to build its own default route — operator-supplied in this phase, not learned in-cluster; see that env var's own doc comment (`internal/config/cni.go`) for why. -`task verify:nat66-sharding` checks each site's `EgressShard` status and +`task verify:nat-sharding` checks each site's `EgressShard` status and `BGPAdvertisement` list. ## NAT64 in this lab -Every shard here is configured for NAT66 only: `GALACTIC_NAT_SHARD_PUB_ADDR6` -is set, and `GALACTIC_NAT_SHARD_PUB_ADDR4`/`GALACTIC_NAT_NAT64_PREFIX` are -not. That is a limitation of the topology, not a default worth copying. - -This lab's transit mesh is IPv6-only and has no IPv4 upstream, so there is -nothing for a translated packet to reach and nothing to send a reply back. -Turning NAT64 on here would produce shards that translate outbound traffic -correctly and then blackhole it — which looks identical, from every counter -this component exposes, to a NAT64 deployment that is simply broken. Leaving -it off keeps the lab's NAT66 signal trustworthy. - -To enable it on a site once a real IPv4 upstream exists, add to that site's -`node-patch.yaml`: - -```yaml -- name: GALACTIC_NAT_SHARD_PUB_ADDR4 - value: "" -- name: GALACTIC_NAT_NAT64_PREFIX - value: "" -``` - -and set `GALACTIC_CNI_NAT64_PREFIX` to the same prefix on every site's -`galactic-cni` DaemonSet (`resources/galactic-cni/daemonset-patch.yaml`), -so each tenant VRF gets a route toward it. Three things have to agree or the -path is a silent blackhole: the prefix the shards translate for, the prefix -the CNI installs a route for, and the prefix DNS64 synthesizes into. - -Unlike `GALACTIC_NAT_SHARD_PUB_ADDR6`, the IPv4 address is not advertised -into the fabric by anything in this repo — a NAT64 reply arrives from the -IPv4 internet, so the underlay or an upstream announcement has to attract -it to that node. - -What this lab therefore does **not** validate: the NAT64 forward and return -legs end to end. Those are covered at the datapath level instead, in -`internal/plumbing/ebpf/natprog/nat64_test.go`, which runs real packets -through the loaded programs and verifies both translated checksums against -independent full recomputes rather than against the datapath's own -arithmetic. +Every shard here serves both families: its `EgressShard` spec assigns +`shardAddressIPv6` (NAT66) and the `shardAddressIPv4`/`nat64Prefix` pair +(NAT64). The pair is all-or-nothing; leave both out of a shard's spec to +make it NAT66-only. + +Three things have to agree or the NAT64 path is a silent blackhole: the +`nat64Prefix` every shard translates for, `GALACTIC_CNI_NAT64_PREFIX` on +every site's `galactic-cni` DaemonSet +(`resources/galactic-cni/shared/daemonset-patch.yaml`), which gives each +tenant VRF a route toward it, and the prefix DNS64 synthesizes into. + +Unlike `shardAddressIPv6`, the IPv4 address is not advertised into the +fabric by anything in this repo — a NAT64 reply arrives over the IPv4 +underlay, which carries no EVPN, so each shard node's +`resources/fabric-router/*/frr.conf.*-worker` originates its own `/32`. +`task verify:nat-datapath` drives both families end to end to the +off-fabric host. diff --git a/deploy/containerlab/resources/galactic-nat/dfw/egressshard.yaml b/deploy/containerlab/resources/galactic-nat/dfw/egressshard.yaml index f00e5300..0821b35f 100644 --- a/deploy/containerlab/resources/galactic-nat/dfw/egressshard.yaml +++ b/deploy/containerlab/resources/galactic-nat/dfw/egressshard.yaml @@ -7,3 +7,13 @@ spec: targetRef: kind: Node name: dfw-worker + # uFMT uSID over dfw's locator (2001:db8:ff01::/48), nodeID=0x2001 + # (service 2 = NAT shard, node index 001). Write-once, like every + # identity field below. + shardSID: "2001:db8:ff01:2001:e001::" + # NAT66 masquerade source address, docs-range 2001:db8:9966::/32 block + shardAddressIPv6: "2001:db8:9966:1::1" + # NAT64 masquerade source address (RFC 5737 TEST-NET-1) and the + # fabric-wide /96 it translates for + shardAddressIPv4: "192.0.2.1" + nat64Prefix: "2001:db8:64::/96" diff --git a/deploy/containerlab/resources/galactic-nat/dfw/node-patch.yaml b/deploy/containerlab/resources/galactic-nat/dfw/node-patch.yaml index 2a3a8d3d..61d46efd 100644 --- a/deploy/containerlab/resources/galactic-nat/dfw/node-patch.yaml +++ b/deploy/containerlab/resources/galactic-nat/dfw/node-patch.yaml @@ -9,18 +9,10 @@ spec: - name: galactic-nat env: # dual-homed node: two uplinks, one LACP bond to each edge node; - # the shard attaches to every member of both + # the shard attaches to every member of both. Named explicitly + # rather than auto-detected: Kind's eth0 carries this node's IPv6 + # default route, so detection would also claim the management + # bridge. The same override galactic-cni's + # GALACTIC_CNI_EBPF_INTERFACES carries here. - name: GALACTIC_NAT_UPLINK_INTERFACES value: bond0,bond1 - # shard SID: uFMT uSID over dfw's locator (2001:db8:ff01::/48), - # nodeID=0x2001 (service 2 = NAT shard, node index 001) - - name: GALACTIC_NAT_SHARD_SID - value: "2001:db8:ff01:2001:e001::" - # NAT66 masquerade source address, docs-range 2001:db8:9966::/32 block - - name: GALACTIC_NAT_SHARD_PUB_ADDR6 - value: "2001:db8:9966:1::1" - # NAT64 masquerade source address (RFC 5737 TEST-NET-1) - - name: GALACTIC_NAT_SHARD_PUB_ADDR4 - value: "192.0.2.1" - - name: GALACTIC_NAT_NAT64_PREFIX - value: "2001:db8:64::/96" diff --git a/deploy/containerlab/resources/galactic-nat/iad/egressshard.yaml b/deploy/containerlab/resources/galactic-nat/iad/egressshard.yaml index 0e3d7159..7e0f2c96 100644 --- a/deploy/containerlab/resources/galactic-nat/iad/egressshard.yaml +++ b/deploy/containerlab/resources/galactic-nat/iad/egressshard.yaml @@ -7,3 +7,13 @@ spec: targetRef: kind: Node name: iad-worker + # uFMT uSID over iad's locator (2001:db8:ff03::/48), nodeID=0x2001 + # (service 2 = NAT shard, node index 001). Write-once, like every + # identity field below. + shardSID: "2001:db8:ff03:2001:e001::" + # NAT66 masquerade source address, docs-range 2001:db8:9966::/32 block + shardAddressIPv6: "2001:db8:9966:2::1" + # NAT64 masquerade source address (RFC 5737 TEST-NET-1) and the + # fabric-wide /96 it translates for + shardAddressIPv4: "192.0.2.2" + nat64Prefix: "2001:db8:64::/96" diff --git a/deploy/containerlab/resources/galactic-nat/iad/node-patch.yaml b/deploy/containerlab/resources/galactic-nat/iad/node-patch.yaml index a63d112c..caeca3b7 100644 --- a/deploy/containerlab/resources/galactic-nat/iad/node-patch.yaml +++ b/deploy/containerlab/resources/galactic-nat/iad/node-patch.yaml @@ -9,18 +9,10 @@ spec: - name: galactic-nat env: # single-homed node: one uplink, an LACP bond the shard attaches - # to through its members + # to through its members. Named explicitly rather than + # auto-detected: Kind's eth0 carries this node's IPv6 default + # route, so detection would also claim the management bridge. The + # same override galactic-cni's GALACTIC_CNI_EBPF_INTERFACES + # carries here. - name: GALACTIC_NAT_UPLINK_INTERFACES value: bond0 - # shard SID: uFMT uSID over iad's locator (2001:db8:ff03::/48), - # nodeID=0x2001 (service 2 = NAT shard, node index 001) - - name: GALACTIC_NAT_SHARD_SID - value: "2001:db8:ff03:2001:e001::" - # NAT66 masquerade source address, docs-range 2001:db8:9966::/32 block - - name: GALACTIC_NAT_SHARD_PUB_ADDR6 - value: "2001:db8:9966:2::1" - # NAT64 masquerade source address (RFC 5737 TEST-NET-1) - - name: GALACTIC_NAT_SHARD_PUB_ADDR4 - value: "192.0.2.2" - - name: GALACTIC_NAT_NAT64_PREFIX - value: "2001:db8:64::/96" diff --git a/deploy/containerlab/resources/galactic-nat/sjc/egressshard.yaml b/deploy/containerlab/resources/galactic-nat/sjc/egressshard.yaml index 17398cc2..c11d3da5 100644 --- a/deploy/containerlab/resources/galactic-nat/sjc/egressshard.yaml +++ b/deploy/containerlab/resources/galactic-nat/sjc/egressshard.yaml @@ -7,3 +7,13 @@ spec: targetRef: kind: Node name: sjc-worker + # uFMT uSID over sjc's locator (2001:db8:ff02::/48), nodeID=0x2001 + # (service 2 = NAT shard, node index 001). Write-once, like every + # identity field below. + shardSID: "2001:db8:ff02:2001:e001::" + # NAT66 masquerade source address, docs-range 2001:db8:9966::/32 block + shardAddressIPv6: "2001:db8:9966:3::1" + # NAT64 masquerade source address (RFC 5737 TEST-NET-1) and the + # fabric-wide /96 it translates for + shardAddressIPv4: "192.0.2.3" + nat64Prefix: "2001:db8:64::/96" diff --git a/deploy/containerlab/resources/galactic-nat/sjc/node-patch.yaml b/deploy/containerlab/resources/galactic-nat/sjc/node-patch.yaml index 7e95aced..caeca3b7 100644 --- a/deploy/containerlab/resources/galactic-nat/sjc/node-patch.yaml +++ b/deploy/containerlab/resources/galactic-nat/sjc/node-patch.yaml @@ -9,18 +9,10 @@ spec: - name: galactic-nat env: # single-homed node: one uplink, an LACP bond the shard attaches - # to through its members + # to through its members. Named explicitly rather than + # auto-detected: Kind's eth0 carries this node's IPv6 default + # route, so detection would also claim the management bridge. The + # same override galactic-cni's GALACTIC_CNI_EBPF_INTERFACES + # carries here. - name: GALACTIC_NAT_UPLINK_INTERFACES value: bond0 - # shard SID: uFMT uSID over sjc's locator (2001:db8:ff02::/48), - # nodeID=0x2001 (service 2 = NAT shard, node index 001) - - name: GALACTIC_NAT_SHARD_SID - value: "2001:db8:ff02:2001:e001::" - # NAT66 masquerade source address, docs-range 2001:db8:9966::/32 block - - name: GALACTIC_NAT_SHARD_PUB_ADDR6 - value: "2001:db8:9966:3::1" - # NAT64 masquerade source address (RFC 5737 TEST-NET-1) - - name: GALACTIC_NAT_SHARD_PUB_ADDR4 - value: "192.0.2.3" - - name: GALACTIC_NAT_NAT64_PREFIX - value: "2001:db8:64::/96" diff --git a/docs/nat/configuration.md b/docs/nat/configuration.md index 6c376987..d368fe21 100644 --- a/docs/nat/configuration.md +++ b/docs/nat/configuration.md @@ -9,7 +9,7 @@ of standing this up from nothing, see [docs/nat/getting-started.md](getting-started.md); this document only covers the "what", not the "why" or the step-by-step. -> Last verified: 2026-09-11 against the current working tree of +> Last verified: 2026-09-23 against the current working tree of > `internal/config/nat.go`, `internal/config/cni.go`, > `cmd/galactic-nat/`, `config/galactic-nat/`, > `internal/controller/egressshard_controller.go`, and @@ -23,44 +23,41 @@ flags, or a combination of both (CLI flags take precedence), with the `galactic-router` and `galactic-gateway` use (see [docs/router/configuration.md](../router/configuration.md)). -| Option | Environment Variable | CLI Flag | Default | Required | -| ------------------- | --------------------------------- | -------------------------- | ------- | -------------- | -| Node name | `GALACTIC_NAT_NODE_NAME` | `--node-name` | — | Yes | -| Uplink interfaces | `GALACTIC_NAT_UPLINK_INTERFACES` | `--nat-uplink-interfaces` | — | Yes | -| Shard SID | `GALACTIC_NAT_SHARD_SID` | `--nat-shard-sid` | — | Yes | -| IPv6 masquerade src | `GALACTIC_NAT_SHARD_PUB_ADDR6` | `--nat-shard-pub-addr6` | — | Enables NAT66 | -| IPv4 masquerade src | `GALACTIC_NAT_SHARD_PUB_ADDR4` | `--nat-shard-pub-addr4` | — | Enables NAT64 | -| NAT64 prefix | `GALACTIC_NAT_NAT64_PREFIX` | `--nat64-prefix` | — | With the above | -| Metrics port | `GALACTIC_NAT_METRICS_PORT` | `--metrics-port` | `9182` | No | -| gRPC health port | `GALACTIC_NAT_GRPC_HEALTH_PORT` | `--grpc-health-port` | `5182` | No | - -`NATConfig.Validate` enforces all of this at startup — a shard node -deployed wrong crash-loops immediately with an actionable message rather -than running degraded. Specifically: - -- The node name, at least one uplink, and the shard SID are always required. -- **At least one address family must be turned on.** A shard serving - neither loads a datapath that claims no packet at all, which presents as - a silent blackhole rather than as the misconfiguration it is. -- **The NAT64 pair is all-or-nothing.** An IPv4 address with no prefix has - nothing to translate for; a prefix with no IPv4 address has nothing to - translate into. Either alone would drop every NAT64 packet sent to it, so - both are rejected up front rather than at the first packet. - -A shard may serve NAT66 only (the shape every shard had before NAT64 -existed), NAT64 only, or both. `9182`/`5182` are -chosen to avoid every other `hostNetwork: true` galactic process already -running on a compute node (`fabric-router`'s `179`, `galactic-router`'s -`9179`/`5179`, `galactic-cni`'s `9180`/`5180`, `galactic-gateway`'s -`8081`/`5181`). +| Option | Environment Variable | CLI Flag | Default | Required | +| ----------------- | -------------------------------- | ------------------------- | ------------- | -------- | +| Node name | `GALACTIC_NAT_NODE_NAME` | `--node-name` | — | Yes | +| Uplink interfaces | `GALACTIC_NAT_UPLINK_INTERFACES` | `--nat-uplink-interfaces` | auto-detected | No | +| Metrics port | `GALACTIC_NAT_METRICS_PORT` | `--metrics-port` | `9182` | No | +| gRPC health port | `GALACTIC_NAT_GRPC_HEALTH_PORT` | `--grpc-health-port` | `5182` | No | + +That is the whole process configuration. The shard's identity — its SID, +masquerade addresses and NAT64 prefix — is not process configuration: it +comes from the spec of the `EgressShard` targeting this node (see +[below](#egressshard-crd-networkdatumapiscomv1alpha1)). The process attaches +its datapath at startup with no identity and programs one on reconcile, so a +node without an `EgressShard` runs attached but claims no packet rather than +crash-looping. + +`9182`/`5182` are chosen to avoid every other `hostNetwork: true` galactic +process already running on a compute node (`fabric-router`'s `179`, +`galactic-router`'s `9179`/`5179`, `galactic-cni`'s `9180`/`5180`, +`galactic-gateway`'s `8081`/`5181`). ### Option details **`--nat-uplink-interfaces` / `GALACTIC_NAT_UPLINK_INTERFACES`** -Comma-separated names of this shard's fabric-facing uplink interfaces — +Comma-separated override for this shard's fabric-facing uplink interfaces — `internal/plumbing/ebpf/natprog`'s XDP program attaches to every one of -them. Required: `galactic-nat` only ever runs as a dedicated shard, so -there's no "not this role, skip the datapath" case to fall back to. +them. Unset, they are auto-detected with the same derivation +`galactic-cni`'s SRv6 datapath uses: every interface carrying the IPv6 +default route or a BGP-learned route, skipping tunnels, VRF slaves and +loopback (`attach.DetectUplinks`). Either way a bonding master resolves to +its slaves, never to itself, since native XDP on a master is unreliable +(`natattach.ResolveUplinks`). + +Set it on a node where detection is ambiguous — typically one whose +management NIC carries the IPv6 default route, as Kind's `eth0` does in the +containerlab lab. Detection would attach there too. > **Name every fabric uplink, not just the primary.** The datapath claims > a packet only on an interface its program is attached to. An @@ -80,63 +77,10 @@ driver drops carrier to attach a native XDP program, every member of the bond bounces at once when the shard starts. Attachment is all-or-nothing: a shard that cannot attach to every -interface in the list fails to start, rather than coming up with a hole in +resolved interface fails to start, rather than coming up with a hole in its coverage. It happens once, at process startup, so an interface that appears later is not picked up until the process restarts. -**`--nat-shard-sid` / `GALACTIC_NAT_SHARD_SID`** -This shard's own SRv6 uSID (`EgressShardStatus.ShardSID`) — the outer -destination a tenant's egress packet is encapsulated toward. One SID -serves both address families: the datapath decides which translation a -packet gets from its inner destination, so enabling NAT64 needs no second -SID and no second route on any tenant VRF. Must be a -native IPv6 address (`NATConfig.Validate` rejects IPv4 and 4-in-6). -Operator-supplied today; nothing in this repo derives it automatically — -the same gap `BGPRouter.Spec.SRv6Locator`/`NodeID` assignment and -`GALACTIC_GATEWAY_SRV6_ADDRESS` both have. - -> **Node-ID collision hazard.** The datapath's `locator_matches` check -> (`internal/plumbing/ebpf/natprog/nat.c`) only compares the top 64 -> bits (Block + Node-ID) of a packet's outer destination against this -> value — it does **not** check that the Node-ID is actually reserved for -> the shard. Reusing the physical node's own real `BGPRouter.Spec.NodeID` -> here means the shard's XDP program hijacks that node's own ordinary -> tenant ingress traffic before `usid_ingress` ever gets to it. Reserve a -> distinct Node-ID on the shard's locator for this purpose alone — see -> `deploy/containerlab/resources/galactic-nat/dfw/node-patch.yaml`'s -> comment for the exact encoding the lab uses. - -**`--nat-shard-pub-addr6` / `GALACTIC_NAT_SHARD_PUB_ADDR6`** -This shard's own dedicated, publicly-routable IPv6 address -(`EgressShardStatus.ShardAddressIPv6`) — every flow this shard NATs is SNAT'd -to an address:port within it. Must also be a native IPv6 address. Must be -unique per shard: a flow's reply is routed back to the correct shard by -ordinary unicast routing on this address alone, with no hashing on the -return path — two shards sharing an address would make replies -undeliverable or misdelivered. - -> **The underlay has to carry both masquerade addresses, and nothing in -> this repo puts them there.** `EgressShardReconciler` advertises this -> address as an EVPN Type 5 path, which makes it reachable from other -> nodes on the fabric and from nowhere else — the plain-unicast underlay -> carries no EVPN, and a masqueraded flow's reply comes back from a host -> that is not on the fabric at all. Unless the underlay holds a route -> attracting this address to *this* node, that reply is forwarded on -> whatever default the first router holding no route for it has, and the -> flow is one-way while every forward-path counter stays green (#549). -> `--nat-shard-pub-addr4` below needs the same thing over IPv4, where -> there is no advertisement of any kind. -> -> Origination has to come from the shard's own node, not from an -> aggregate elsewhere: the address must reach the node whose datapath -> holds that flow's connection state. The lab does it by having each -> shard node's `fabric-router` originate its own two addresses (a `/64` -> and a `/32`) — see -> `deploy/containerlab/resources/fabric-router/dfw/frr.conf.dfw-worker`, -> and `task -d deploy/containerlab verify:nat-return-route` for the check -> that proves it. Where those addresses come from in the first place is -> #409. - ### Capabilities and host requirements `galactic-nat` runs `hostNetwork: true` and needs: @@ -154,24 +98,64 @@ under `/sys/fs/bpf/galactic-nat`. ## `EgressShard` CRD (`network.datumapis.com/v1alpha1`) -One object per shard node, in the `galactic-system` namespace. - -| Field | Required | Type | Description | -| --------------------- | -------- | -------- | --------------------------------------------------------------------------------------------- | -| `spec.targetRef.name` | Yes | `string` | Kubernetes node name this shard's `galactic-nat` process runs on. | -| `status.shardSID` | — | `string` | This shard's uSID, published by the reconciler from `GALACTIC_NAT_SHARD_SID`; not user-set. | -| `status.shardAddressIPv6` | — | `string` | This shard's IPv6 masquerade address, from `GALACTIC_NAT_SHARD_PUB_ADDR6`. Empty means no NAT66. | -| `status.shardAddressIPv4` | — | `string` | This shard's IPv4 masquerade address, from `GALACTIC_NAT_SHARD_PUB_ADDR4`. Empty means no NAT64. | -| `status.nat64Prefix` | — | `string` | The `/96` this shard translates for, from `GALACTIC_NAT_NAT64_PREFIX`. | -| `status.conditions` | — | — | `Ready` condition, reason `DatapathAttached` or `DatapathNotAttached`. | - -Leave `status` empty when creating the object — `EgressShardReconciler` -(running inside that node's own `galactic-nat` pod) fills it in from the -pod's own resolved config at startup and publishes a `/128` -`BGPAdvertisement` for each of `shardSID`/`shardAddressIPv6` (the same -RT-less, no-`VRFID`/`Function` shape `NetworkGatewayReconciler` uses for -its own VIP advertisements), so every other node in the mesh learns a -real kernel route to it. +One object per shard node, in the `galactic-system` namespace. The spec +assigns the shard's identity; the `galactic-nat` process on the target node +programs its datapath from it and reports what it is actually programmed +with in status. + +| Field | Required | Type | Description | +| ------------------------- | -------- | -------- | ------------------------------------------------------------------------------------------------------ | +| `spec.targetRef.name` | Yes | `string` | Kubernetes node name this shard's `galactic-nat` process runs on. | +| `spec.shardSID` | No | `string` | This shard's SRv6 uSID. Write-once. | +| `spec.shardAddressIPv6` | No | `string` | IPv6 masquerade source. Setting it enables NAT66. Write-once. | +| `spec.shardAddressIPv4` | No | `string` | IPv4 masquerade source. Setting it, with `nat64Prefix`, enables NAT64. Write-once. | +| `spec.nat64Prefix` | No | `string` | The fabric-wide `/96` this shard translates to IPv4. Set together with `shardAddressIPv4`. Write-once. | +| `status.shardSID` | — | `string` | The SID the datapath is programmed with. | +| `status.shardAddressIPv6` | — | `string` | The IPv6 masquerade source the datapath is programmed with. Empty means no NAT66. | +| `status.shardAddressIPv4` | — | `string` | The IPv4 masquerade source the datapath is programmed with. Empty means no NAT64. | +| `status.nat64Prefix` | — | `string` | The `/96` the datapath is programmed to translate. | +| `status.conditions` | — | — | `Ready` (datapath attached) and `Programmed` (datapath translating with the spec's identity). | + +Every identity field is optional and write-once. A shard can exist before +its identity is assigned, and gains it later with a spec update; once +assigned, a value cannot change or be cleared, because the datapath claims +return traffic by exact match on it and a change strands every established +flow. A shard holding the wrong identity is deleted and recreated instead. + +The datapath needs a SID and at least one family before it translates +anything: + +| `Programmed` reason | Meaning | +| --------------------- | ---------------------------------------------------------------------------------------------------------------- | +| `AddressesProgrammed` | The datapath translates with the identity the spec assigns. | +| `AddressUnassigned` | The spec assigns no SID, or no masquerade address for either family. The datapath is cleared and claims nothing. | +| `ProgrammingFailed` | The datapath rejected the identity, for example a NAT64 prefix that is not a `/96`. The message says why. | +| `ShardConflict` | More than one `EgressShard` targets this node. None is programmed until only one does. | + +`EgressShardReconciler` (running inside that node's own `galactic-nat` pod) +publishes one `BGPAdvertisement` per shard, built from status rather than +spec so the fabric only learns an identity the node actually translates +with: the SID's covering `/64` and `shardAddressIPv6` as a `/128`, in the +same RT-less, no-`VRFID`/`Function` shape `NetworkGatewayReconciler` uses +for its own VIP advertisements. Every other node in the mesh learns a real +kernel route to both. Deleting the shard, or leaving it with no usable +identity, clears the datapath and withdraws the advertisement. + +> **Node-ID collision hazard.** The datapath's `locator_matches` check +> (`internal/plumbing/ebpf/natprog/nat.c`) only compares the top 64 +> bits (Block + Node-ID) of a packet's outer destination against +> `shardSID` — it does **not** check that the Node-ID is actually reserved +> for the shard. Reusing the physical node's own real `BGPRouter.Spec.NodeID` +> here means the shard's XDP program hijacks that node's own ordinary +> tenant ingress traffic before `usid_ingress` ever gets to it. Reserve a +> distinct Node-ID on the shard's locator for this purpose alone — see +> `deploy/containerlab/resources/galactic-nat/dfw/egressshard.yaml` for +> the exact encoding the lab uses. + +`shardAddressIPv6` must be unique per shard: a flow's reply is routed back +to the correct shard by ordinary unicast routing on this address alone, with +no hashing on the return path — two shards sharing an address would make +replies undeliverable or misdelivered. `shardAddressIPv4` is deliberately **not** advertised. A NAT64 reply arrives from the IPv4 internet rather than across this fabric, so a @@ -182,10 +166,29 @@ makes that prerequisite checkable rather than implicit; a shard whose `shardAddressIPv4` is set but unreachable translates outbound traffic correctly and never sees a single reply. -`nat64Prefix` is echoed here for the same class of reason: it is the one -value DNS64 synthesis has to agree with, and a shard translating for a -different prefix than the resolver hands out is a blackhole with no -symptom on either side. +> **The underlay has to carry both masquerade addresses, and nothing in +> this repo puts them there.** The EVPN Type 5 path for +> `shardAddressIPv6` makes it reachable from other nodes on the fabric and +> from nowhere else — the plain-unicast underlay carries no EVPN, and a +> masqueraded flow's reply comes back from a host that is not on the +> fabric at all. Unless the underlay holds a route attracting each address +> to *this* node, that reply is forwarded on whatever default the first +> router holding no route for it has, and the flow is one-way while every +> forward-path counter stays green (#549). +> +> Origination has to come from the shard's own node, not from an +> aggregate elsewhere: the address must reach the node whose datapath +> holds that flow's connection state. The lab does it by having each +> shard node's `fabric-router` originate its own two addresses (a `/64` +> and a `/32`) — see +> `deploy/containerlab/resources/fabric-router/dfw/frr.conf.dfw-worker`, +> and `task -d deploy/containerlab verify:nat-return-route` for the check +> that proves it. Where those addresses come from in the first place is +> #409. + +`nat64Prefix` has to agree with DNS64 synthesis: a shard translating for a +different prefix than the resolver hands out is a blackhole with no symptom +on either side. Example: @@ -199,6 +202,10 @@ spec: targetRef: kind: Node name: dfw-worker + shardSID: "2001:db8:ff01:2001:e001::" + shardAddressIPv6: "2001:db8:9966:1::1" + shardAddressIPv4: "192.0.2.1" + nat64Prefix: "2001:db8:64::/96" ``` ### RBAC @@ -235,7 +242,7 @@ fabric may offer NAT64 without NAT66, and then there is no default for that traffic to fall into. `GALACTIC_CNI_EGRESS_SHARD_SIDS` is a comma-separated list of every live -shard's `Status.ShardSID`, resolved with env > conflist > default +shard's `status.shardSID`, resolved with env > conflist > default precedence by `internal/config.CNIConfig`, written into the static conflist by `internal/installer.Bootstrap`, and read back by `internal/cnibgp` on every CNI ADD. An empty value means "no NAT66 @@ -295,9 +302,9 @@ kubectl exec -n galactic-system -- \ wget -qO- http://localhost:9182/metrics | grep galactic_nat_ ``` -Confirm the eBPF program is actually attached — `Ready`'s condition -reason on the `EgressShard` object should read `DatapathAttached`, not -`DatapathNotAttached`: +Confirm the eBPF program is attached and translating — on the +`EgressShard` object, `Ready` should read `DatapathAttached` and +`Programmed` should read `AddressesProgrammed`: ```sh kubectl get egressshard -n galactic-system -o jsonpath='{.status.conditions}' @@ -340,10 +347,13 @@ knowing before you rely on this component in production: kernel's full output path — so `ip rule` policy routing is not consulted — and no ICMP error is generated on the shard's behalf. Each of those surfaces as a named drop counter instead. -- **`ShardSID`/`ShardPubAddr6` are entirely operator-chosen.** There is no - in-cluster allocator for either value, and no automatic check that a - chosen SID's Node-ID doesn't collide with a real node's own — see the - "Node-ID collision hazard" callout above. +- **A shard's identity is entirely operator-chosen.** Nothing in this repo + allocates `spec.shardSID` or the masquerade addresses, and nothing checks + that a chosen SID's Node-ID doesn't collide with a real node's own — see + the "Node-ID collision hazard" callout above. +- **The CNI's shard list is a second copy of every shard SID.** + `GALACTIC_CNI_EGRESS_SHARD_SIDS` is set by hand and is not derived from + `EgressShard` status, so the two can disagree. ## See also diff --git a/go.mod b/go.mod index f45eb1be..c430e0e2 100644 --- a/go.mod +++ b/go.mod @@ -19,7 +19,7 @@ require ( github.com/spf13/pflag v1.0.10 github.com/spf13/viper v1.21.0 github.com/vishvananda/netlink v1.3.2-0.20260831221819-dcee5577542a - go.datum.net/network v0.1.0 + go.datum.net/network v0.1.1-0.20260923215140-1ed44c853eea golang.org/x/sys v0.48.0 golang.org/x/term v0.46.0 google.golang.org/grpc v1.84.0 diff --git a/go.sum b/go.sum index fbc05ed9..a0eaf312 100644 --- a/go.sum +++ b/go.sum @@ -188,6 +188,8 @@ github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM= github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg= go.datum.net/network v0.1.0 h1:AmYSwxUWOk26UnK6S6NA7OuucGJniKo/CWqjs+VcSCs= go.datum.net/network v0.1.0/go.mod h1:dqzM8WZczbiZ9bCvsxjkoI10GJqQ24NVWnc9boXgOkE= +go.datum.net/network v0.1.1-0.20260923215140-1ed44c853eea h1:E1nxOKynuFjWuJuHyXsPyXWN98/H0WNcsyD03eqpKdk= +go.datum.net/network v0.1.1-0.20260923215140-1ed44c853eea/go.mod h1:dqzM8WZczbiZ9bCvsxjkoI10GJqQ24NVWnc9boXgOkE= go.opentelemetry.io/auto/sdk v1.2.1 h1:jXsnJ4Lmnqd11kwkBV2LgLoFMZKizbCi5fNZ/ipaZ64= go.opentelemetry.io/auto/sdk v1.2.1/go.mod h1:KRTj+aOaElaLi+wW1kO/DZRXwkF4C5xPbEe3ZiIhN7Y= go.opentelemetry.io/otel v1.44.0 h1:JjwHmHpA4iZ3wBxluu2fbbE7j4kqlE8jXyAyPXH7HqU= diff --git a/internal/config/nat.go b/internal/config/nat.go index 6bc7414c..a8074f5c 100644 --- a/internal/config/nat.go +++ b/internal/config/nat.go @@ -7,7 +7,6 @@ package config import ( "errors" "fmt" - "net/netip" "github.com/spf13/pflag" "github.com/spf13/viper" @@ -23,10 +22,6 @@ const ( // these avoid every value already claimed rather than assume no overlap. DefaultNATMetricsPort = 9182 DefaultNATGRPCHealthPort = 5182 - - // DefaultNAT64PrefixLen is the only NAT64 prefix length the datapath - // supports; see natmap's own constant for why a /96 specifically. - DefaultNAT64PrefixLen = 96 ) // --- NAT environment variable keys ------------------------------------ @@ -36,60 +31,27 @@ const ( EnvNATMetricsPort = "GALACTIC_NAT_METRICS_PORT" EnvNATGRPCHealthPort = "GALACTIC_NAT_GRPC_HEALTH_PORT" - // EnvNATUplinkInterfaces names this shard's fabric-facing uplinks, comma- - // separated -- every interface the XDP datapath attaches to. Required: this - // binary only ever runs as a dedicated shard, so there is no "not this - // role, skip the datapath" case. + // EnvNATUplinkInterfaces overrides the fabric-facing uplinks the XDP + // datapath attaches to, comma-separated. Optional: unset, the uplinks are + // auto-detected the way the CNI's SRv6 datapath detects its own (see + // natattach.ResolveUplinks), so the shard and the CNI on one node converge + // on the same interfaces with no per-node configuration. // - // Every fabric uplink belongs here, not just the one a node's traffic - // happens to use today. A shard claims a packet only on an interface its - // program is attached to; an encapsulated tenant packet arriving anywhere - // else reaches no translation program at all and is forwarded untranslated - // and uncounted, with nothing on either side reporting a fault. Naming one - // uplink on a multi-homed node therefore makes the shard role survive only - // as long as that uplink does, which is what this taking a list rather than - // a single name exists to prevent. + // Set it only on a multi-homed node where that detection cannot be + // confident, and then name every fabric uplink, not just the one a node's + // traffic happens to use today. A shard claims a packet only on an + // interface its program is attached to; an encapsulated tenant packet + // arriving anywhere else reaches no translation program at all and is + // forwarded untranslated and uncounted, with nothing on either side + // reporting a fault. // // A bonding master may be named in place of its members: it is expanded // to its slaves at startup and the program attached to each, never to the // master itself. - EnvNATUplinkInterfaces = "GALACTIC_NAT_UPLINK_INTERFACES" - - // EnvNATShardSID is this shard's own SRv6 uSID, the outer destination a - // tenant's egress packet is encapsulated toward. Required, and - // operator-supplied: no in-cluster mechanism derives it yet, the same gap - // the router locator and the gateway address both have. - // - // One SID serves both address families. Which translation a packet gets is - // decided from its inner destination, so enabling NAT64 on a shard needs no - // second SID and no second route on any tenant VRF. - EnvNATShardSID = "GALACTIC_NAT_SHARD_SID" - - // EnvNATShardPubAddr6 is this shard's publicly routable IPv6 masquerade - // source. Every NAT66 flow this shard translates is given an address and - // port within it. Optional: a shard may serve NAT64 alone. - EnvNATShardPubAddr6 = "GALACTIC_NAT_SHARD_PUB_ADDR6" - - // EnvNATShardPubAddr4 is this shard's publicly routable IPv4 masquerade - // source, the address an IPv4-only destination sees. Setting it, together - // with a NAT64 prefix, is what turns NAT64 on for this shard. - // - // Unlike the IPv6 address, nothing in this repo makes it reachable: a NAT64 - // reply arrives from the IPv4 internet, so the underlay or an upstream - // announcement has to attract this address to this node. Setting it here - // without that in place produces a shard that translates outbound traffic - // and never sees a single reply. - EnvNATShardPubAddr4 = "GALACTIC_NAT_SHARD_PUB_ADDR4" - - // EnvNAT64Prefix is the IPv6 /96 whose synthesized addresses this shard - // translates to IPv4 -- one Datum-operated Network-Specific Prefix, shared - // fabric-wide, never per-tenant. Required whenever the IPv4 address is set. // - // It must be the same value DNS64 synthesizes into. A shard translating for - // a different prefix than the resolver hands out is a blackhole with no - // symptom on either side, which is why this value is echoed into - // EgressShard status rather than living only here. - EnvNAT64Prefix = "GALACTIC_NAT_NAT64_PREFIX" + // The shard's identity -- its SID, masquerade addresses and NAT64 prefix -- + // is not process configuration: it comes from its EgressShard's spec. + EnvNATUplinkInterfaces = "GALACTIC_NAT_UPLINK_INTERFACES" ) // --- NATConfig --------------------------------------------------------- @@ -107,18 +69,9 @@ type NATConfig struct { MetricsPort int GRPCHealthPort int - // UplinkInterfaces and ShardSID configure the shard's identity and are - // always required. UplinkInterfaces is parsed from the comma-separated - // EnvNATUplinkInterfaces and carries every fabric uplink the datapath - // attaches to, never only the primary -- see that variable's own comment. + // UplinkInterfaces is the optional override parsed from the + // comma-separated EnvNATUplinkInterfaces. Empty means auto-detect. UplinkInterfaces []string - ShardSID string - - // ShardPubAddr6 enables NAT66; ShardPubAddr4 with NAT64Prefix enables NAT64. - // Validate requires at least one family, and rejects half of either. - ShardPubAddr6 string - ShardPubAddr4 string - NAT64Prefix string } // NewNATConfig creates a config resolver reading the GALACTIC_NAT @@ -133,10 +86,6 @@ func NewNATConfig() *NATConfig { v.SetDefault(KeyMetricsPort, DefaultNATMetricsPort) v.SetDefault(KeyGRPCHealthPort, DefaultNATGRPCHealthPort) v.SetDefault("uplink_interfaces", "") - v.SetDefault("shard_sid", "") - v.SetDefault("shard_pub_addr6", "") - v.SetDefault("shard_pub_addr4", "") - v.SetDefault("nat64_prefix", "") cfg := &NATConfig{ v: v, @@ -157,10 +106,6 @@ func (c *NATConfig) BindFlags(flags *pflag.FlagSet) { {FlagMetricsPort, KeyMetricsPort}, {FlagGRPCHealthPort, KeyGRPCHealthPort}, {"nat-uplink-interfaces", "uplink_interfaces"}, - {"nat-shard-sid", "shard_sid"}, - {"nat-shard-pub-addr6", "shard_pub_addr6"}, - {"nat-shard-pub-addr4", "shard_pub_addr4"}, - {"nat64-prefix", "nat64_prefix"}, } for _, b := range bindings { if flags.Changed(b.flag) { @@ -179,78 +124,6 @@ func (c *NATConfig) readFields() { c.MetricsPort = c.v.GetInt(KeyMetricsPort) c.GRPCHealthPort = c.v.GetInt(KeyGRPCHealthPort) c.UplinkInterfaces = splitInterfaceList(c.v.GetString("uplink_interfaces")) - c.ShardSID = c.v.GetString("shard_sid") - c.ShardPubAddr6 = c.v.GetString("shard_pub_addr6") - c.ShardPubAddr4 = c.v.GetString("shard_pub_addr4") - c.NAT64Prefix = c.v.GetString("nat64_prefix") -} - -// ServesNAT66 and ServesNAT64 report which families this shard's configuration -// turns on. They are what the binary and the EgressShard reconciler both read, -// so "does this shard do NAT64" has one answer rather than each caller -// re-deriving it from which fields happen to be non-empty. -func (c *NATConfig) ServesNAT66() bool { return c.ShardPubAddr6 != "" } -func (c *NATConfig) ServesNAT64() bool { return c.ShardPubAddr4 != "" } - -// validateShardAddr parses and range-checks a shard identity address, rejecting -// anything that is not a native IPv6 address. -// -// The map layer catches the same thing eventually, but only after the datapath -// has been loaded and attached. Rejecting it at startup names the actual field -// instead of surfacing as a deeper kernel-datapath error. -func validateShardAddr(field, value string) error { - addr, err := netip.ParseAddr(value) - if err != nil { - return fmt.Errorf("%s %q is not a valid IP address: %w", field, value, err) - } - if !addr.Is6() || addr.Is4In6() { - return fmt.Errorf("%s %q must be a native IPv6 address, not IPv4", field, value) - } - return nil -} - -// validateNAT64 checks the IPv4 half of the configuration, which is all-or- -// nothing: an IPv4 address with no prefix has nothing to translate for, and a -// prefix with no address has nothing to translate into. Either half alone -// produces a shard that drops every NAT64 packet it is sent, so both are -// rejected at startup rather than at the first packet. -func (c *NATConfig) validateNAT64() error { - if c.ShardPubAddr4 == "" && c.NAT64Prefix == "" { - return nil - } - if c.ShardPubAddr4 == "" { - return fmt.Errorf( - "NAT64 prefix is set but shard public IPv4 address is not (use --nat-shard-pub-addr4 flag or %s env var)", - EnvNATShardPubAddr4) - } - if c.NAT64Prefix == "" { - return fmt.Errorf( - "shard public IPv4 address is set but NAT64 prefix is not (use --nat64-prefix flag or %s env var)", - EnvNAT64Prefix) - } - - addr4, err := netip.ParseAddr(c.ShardPubAddr4) - if err != nil { - return fmt.Errorf("shard public IPv4 address %q is not a valid IP address: %w", c.ShardPubAddr4, err) - } - if !addr4.Is4() { - return fmt.Errorf("shard public IPv4 address %q must be an IPv4 address", c.ShardPubAddr4) - } - - prefix, err := netip.ParsePrefix(c.NAT64Prefix) - if err != nil { - return fmt.Errorf("NAT64 prefix %q is not a valid CIDR: %w", c.NAT64Prefix, err) - } - if !prefix.Addr().Is6() || prefix.Addr().Is4In6() { - return fmt.Errorf("NAT64 prefix %q must be an IPv6 prefix", c.NAT64Prefix) - } - if prefix.Bits() != DefaultNAT64PrefixLen { - return fmt.Errorf("NAT64 prefix %q must be a /%d", c.NAT64Prefix, DefaultNAT64PrefixLen) - } - if prefix.Masked() != prefix { - return fmt.Errorf("NAT64 prefix %q has bits set below its prefix length", c.NAT64Prefix) - } - return nil } // Validate checks that the required configuration fields are set. @@ -258,34 +131,6 @@ func (c *NATConfig) Validate() error { if c.NodeName == "" { return fmt.Errorf("node name is required (use --node-name flag or %s env var)", EnvNATNodeName) } - if len(c.UplinkInterfaces) == 0 { - return fmt.Errorf( - "at least one uplink interface is required (use --nat-uplink-interfaces flag or %s env var)", - EnvNATUplinkInterfaces) - } - if c.ShardSID == "" { - return fmt.Errorf( - "shard SID is required (use --nat-shard-sid flag or %s env var)", EnvNATShardSID) - } - if err := validateShardAddr("shard SID", c.ShardSID); err != nil { - return err - } - if c.ShardPubAddr6 != "" { - if err := validateShardAddr("shard public address", c.ShardPubAddr6); err != nil { - return err - } - } - if err := c.validateNAT64(); err != nil { - return err - } - // A shard serving neither family loads a datapath that claims no packet at - // all, which presents as a silent blackhole rather than as the - // misconfiguration it is. - if !c.ServesNAT66() && !c.ServesNAT64() { - return fmt.Errorf( - "a shard must serve at least one address family: set %s for NAT66, or %s and %s for NAT64", - EnvNATShardPubAddr6, EnvNATShardPubAddr4, EnvNAT64Prefix) - } if c.MetricsPort < 1 || c.MetricsPort > 65535 { return errors.New("metrics port must be between 1 and 65535") } diff --git a/internal/config/nat_test.go b/internal/config/nat_test.go index c80eb5f2..0094da3a 100644 --- a/internal/config/nat_test.go +++ b/internal/config/nat_test.go @@ -12,13 +12,9 @@ import ( ) const ( - testNATNodeName = "test-nat-node" - testNATIface = "eth1" - testNATIface2 = "eth2" - testNATShardSID = "fc00:1:2::1" - testNATShardPub4 = "192.0.2.10" - testNAT64Prefix = "2001:db8:64::/96" - testNATShardPub = "2001:db8:9999::1" + testNATNodeName = "test-nat-node" + testNATIface = "eth1" + testNATIface2 = "eth2" ) func TestNATConfigDefaults(t *testing.T) { @@ -36,12 +32,6 @@ func TestNATConfigDefaults(t *testing.T) { if len(cfg.UplinkInterfaces) != 0 { t.Errorf("UplinkInterfaces = %q, want empty", cfg.UplinkInterfaces) } - if cfg.ShardSID != "" { - t.Errorf("ShardSID = %q, want empty", cfg.ShardSID) - } - if cfg.ShardPubAddr6 != "" { - t.Errorf("ShardPubAddr6 = %q, want empty", cfg.ShardPubAddr6) - } } func TestNATConfigEnvOverride(t *testing.T) { @@ -49,8 +39,6 @@ func TestNATConfigEnvOverride(t *testing.T) { t.Setenv(EnvNATMetricsPort, "9090") t.Setenv(EnvNATGRPCHealthPort, "9091") t.Setenv(EnvNATUplinkInterfaces, testNATIface) - t.Setenv(EnvNATShardSID, testNATShardSID) - t.Setenv(EnvNATShardPubAddr6, testNATShardPub) cfg := NewNATConfig() @@ -66,12 +54,6 @@ func TestNATConfigEnvOverride(t *testing.T) { if len(cfg.UplinkInterfaces) != 1 || cfg.UplinkInterfaces[0] != testNATIface { t.Errorf("UplinkInterfaces = %q, want [%q]", cfg.UplinkInterfaces, testNATIface) } - if cfg.ShardSID != testNATShardSID { - t.Errorf("ShardSID = %q, want %q", cfg.ShardSID, testNATShardSID) - } - if cfg.ShardPubAddr6 != testNATShardPub { - t.Errorf("ShardPubAddr6 = %q, want %q", cfg.ShardPubAddr6, testNATShardPub) - } } func TestNATConfigValidate(t *testing.T) { @@ -81,160 +63,32 @@ func TestNATConfigValidate(t *testing.T) { wantErr string }{ { - name: testCaseMissingNodeName, - envVars: map[string]string{ - EnvNATUplinkInterfaces: testNATIface, - EnvNATShardSID: testNATShardSID, - EnvNATShardPubAddr6: testNATShardPub, - }, + name: testCaseMissingNodeName, + envVars: map[string]string{EnvNATUplinkInterfaces: testNATIface}, wantErr: testErrNodeNameRequired, }, - { - name: "missing uplink interface", - envVars: map[string]string{ - EnvNATNodeName: testNATNodeName, - EnvNATShardSID: testNATShardSID, - EnvNATShardPubAddr6: testNATShardPub, - }, - wantErr: "uplink interface is required", - }, - { - name: "missing shard SID", - envVars: map[string]string{ - EnvNATNodeName: testNATNodeName, - EnvNATUplinkInterfaces: testNATIface, - EnvNATShardPubAddr6: testNATShardPub, - }, - wantErr: "shard SID is required", - }, - { - name: "unparseable shard SID", - envVars: map[string]string{ - EnvNATNodeName: testNATNodeName, - EnvNATUplinkInterfaces: testNATIface, - EnvNATShardSID: "not-an-ip-address", - EnvNATShardPubAddr6: testNATShardPub, - }, - wantErr: "is not a valid IP address", - }, - { - name: "ipv4 shard SID is wrong family", - envVars: map[string]string{ - EnvNATNodeName: testNATNodeName, - EnvNATUplinkInterfaces: testNATIface, - EnvNATShardSID: testIPv4Addr, - EnvNATShardPubAddr6: testNATShardPub, - }, - wantErr: testErrMustBeNativeIPv6, - }, - { - // No longer "the IPv6 address is required" -- a shard may serve - // NAT64 alone. What is still required is that it serve something. - name: "serving neither address family", - envVars: map[string]string{ - EnvNATNodeName: testNATNodeName, - EnvNATUplinkInterfaces: testNATIface, - EnvNATShardSID: testNATShardSID, - }, - wantErr: "must serve at least one address family", - }, - { - name: "NAT64 prefix without a public IPv4 address", - envVars: map[string]string{ - EnvNATNodeName: testNATNodeName, - EnvNATUplinkInterfaces: testNATIface, - EnvNATShardSID: testNATShardSID, - EnvNATShardPubAddr6: testNATShardPub, - EnvNAT64Prefix: testNAT64Prefix, - }, - wantErr: "shard public IPv4 address is not", - }, - { - name: "public IPv4 address without a NAT64 prefix", - envVars: map[string]string{ - EnvNATNodeName: testNATNodeName, - EnvNATUplinkInterfaces: testNATIface, - EnvNATShardSID: testNATShardSID, - EnvNATShardPubAddr6: testNATShardPub, - EnvNATShardPubAddr4: testNATShardPub4, - }, - wantErr: "NAT64 prefix is not", - }, - { - name: "ipv6 shard public IPv4 address is wrong family", - envVars: map[string]string{ - EnvNATNodeName: testNATNodeName, - EnvNATUplinkInterfaces: testNATIface, - EnvNATShardSID: testNATShardSID, - EnvNATShardPubAddr4: testNATShardPub, - EnvNAT64Prefix: testNAT64Prefix, - }, - wantErr: "must be an IPv4 address", - }, - { - // The datapath extracts the embedded IPv4 address from one aligned - // four-byte run, which only a /96 guarantees. - name: "NAT64 prefix of the wrong length", - envVars: map[string]string{ - EnvNATNodeName: testNATNodeName, - EnvNATUplinkInterfaces: testNATIface, - EnvNATShardSID: testNATShardSID, - EnvNATShardPubAddr4: testNATShardPub4, - EnvNAT64Prefix: "2001:db8:64::/64", - }, - wantErr: "must be a /96", - }, - { - name: "NAT64 prefix with bits below its length", - envVars: map[string]string{ - EnvNATNodeName: testNATNodeName, - EnvNATUplinkInterfaces: testNATIface, - EnvNATShardSID: testNATShardSID, - EnvNATShardPubAddr4: testNATShardPub4, - EnvNAT64Prefix: "2001:db8:64::1/96", - }, - wantErr: "bits set below its prefix length", - }, - { - name: "ipv4 shard public address is wrong family", - envVars: map[string]string{ - EnvNATNodeName: testNATNodeName, - EnvNATUplinkInterfaces: testNATIface, - EnvNATShardSID: testNATShardSID, - EnvNATShardPubAddr6: testIPv4Addr, - }, - wantErr: testErrMustBeNativeIPv6, - }, { name: testCaseInvalidMetricsPort, envVars: map[string]string{ - EnvNATNodeName: testNATNodeName, - EnvNATUplinkInterfaces: testNATIface, - EnvNATShardSID: testNATShardSID, - EnvNATShardPubAddr6: testNATShardPub, - EnvNATMetricsPort: "0", + EnvNATNodeName: testNATNodeName, + EnvNATMetricsPort: "0", }, wantErr: testErrMetricsPortRange, }, { name: testCaseInvalidGRPCHealthPort, envVars: map[string]string{ - EnvNATNodeName: testNATNodeName, - EnvNATUplinkInterfaces: testNATIface, - EnvNATShardSID: testNATShardSID, - EnvNATShardPubAddr6: testNATShardPub, - EnvNATGRPCHealthPort: "0", + EnvNATNodeName: testNATNodeName, + EnvNATGRPCHealthPort: "0", }, wantErr: testErrGRPCHealthPortRange, }, { - name: testCaseValidConfig, - envVars: map[string]string{ - EnvNATNodeName: testNATNodeName, - EnvNATUplinkInterfaces: testNATIface, - EnvNATShardSID: testNATShardSID, - EnvNATShardPubAddr6: testNATShardPub, - }, + // The node name is the only required value. Uplinks are + // auto-detected when unset, and the shard's identity comes from + // its EgressShard's spec, not from process configuration. + name: testCaseValidConfig, + envVars: map[string]string{EnvNATNodeName: testNATNodeName}, wantErr: "", }, } @@ -263,75 +117,11 @@ func TestNATConfigValidate(t *testing.T) { } } -// TestNATConfigValidateAcceptsEachFamilyCombination pins the three shard shapes -// the generalization is for: NAT66 alone (what every shard was before this), -// NAT64 alone, and both at once. The negative table above proves half a NAT64 -// configuration is rejected; this proves a whole one is not. -func TestNATConfigValidateAcceptsEachFamilyCombination(t *testing.T) { - tests := []struct { - name string - envVars map[string]string - wantNAT66 bool - wantNAT64 bool - description string - }{ - { - name: "NAT66 only", - envVars: map[string]string{ - EnvNATShardPubAddr6: testNATShardPub, - }, - wantNAT66: true, - description: "the shape every shard had before NAT64 existed", - }, - { - name: "NAT64 only", - envVars: map[string]string{ - EnvNATShardPubAddr4: testNATShardPub4, - EnvNAT64Prefix: testNAT64Prefix, - }, - wantNAT64: true, - description: "a shard dedicated to IPv4 reachability", - }, - { - name: "both families", - envVars: map[string]string{ - EnvNATShardPubAddr6: testNATShardPub, - EnvNATShardPubAddr4: testNATShardPub4, - EnvNAT64Prefix: testNAT64Prefix, - }, - wantNAT66: true, - wantNAT64: true, - description: "one shard, one session table, both families", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Setenv(EnvNATNodeName, testNATNodeName) - t.Setenv(EnvNATUplinkInterfaces, testNATIface) - t.Setenv(EnvNATShardSID, testNATShardSID) - for k, v := range tt.envVars { - t.Setenv(k, v) - } - - cfg := NewNATConfig() - if err := cfg.Validate(); err != nil { - t.Fatalf("Validate() = %v, want nil (%s)", err, tt.description) - } - if got := cfg.ServesNAT66(); got != tt.wantNAT66 { - t.Errorf("ServesNAT66() = %v, want %v", got, tt.wantNAT66) - } - if got := cfg.ServesNAT64(); got != tt.wantNAT64 { - t.Errorf("ServesNAT64() = %v, want %v", got, tt.wantNAT64) - } - }) - } -} - // TestNATConfigUplinkInterfaces is the regression test for a shard that could -// only ever attach to one uplink (#545). A multi-homed shard node needs every -// fabric uplink named here: one an operator cannot express is one the datapath -// never claims, and traffic arriving there leaves untranslated and uncounted. +// only ever attach to one uplink (#545). The override must be able to name +// every fabric uplink of a multi-homed node: one an operator cannot express is +// one the datapath never claims, and traffic arriving there leaves untranslated +// and uncounted. // // Before the field became a list, the comma-separated form below resolved to a // single interface literally named "eth1,eth2", which Validate accepted and no @@ -366,8 +156,6 @@ func TestNATConfigUplinkInterfaces(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Setenv(EnvNATNodeName, testNATNodeName) t.Setenv(EnvNATUplinkInterfaces, tt.value) - t.Setenv(EnvNATShardSID, testNATShardSID) - t.Setenv(EnvNATShardPubAddr6, testNATShardPub) cfg := NewNATConfig() if err := cfg.Validate(); err != nil { @@ -380,24 +168,22 @@ func TestNATConfigUplinkInterfaces(t *testing.T) { } } -// TestNATConfigUplinkInterfacesRequired covers the other half: a shard with no -// usable uplink must fail at startup naming the field, not load a datapath it -// then attaches nowhere. -func TestNATConfigUplinkInterfacesRequired(t *testing.T) { +// TestNATConfigUplinkInterfacesOptional covers the other half: an override +// that is unset or names nothing usable means auto-detect, not an error. +// Treating it as required is what forced a hand-written per-node patch onto +// every shard (#581). +func TestNATConfigUplinkInterfacesOptional(t *testing.T) { for _, value := range []string{"", " ", ",", " , "} { t.Run(fmt.Sprintf("%q", value), func(t *testing.T) { t.Setenv(EnvNATNodeName, testNATNodeName) t.Setenv(EnvNATUplinkInterfaces, value) - t.Setenv(EnvNATShardSID, testNATShardSID) - t.Setenv(EnvNATShardPubAddr6, testNATShardPub) cfg := NewNATConfig() - err := cfg.Validate() - if err == nil { - t.Fatal("Validate() = nil, want an error for a shard with no uplink") + if err := cfg.Validate(); err != nil { + t.Fatalf("Validate() = %v, want nil (an empty override means auto-detect)", err) } - if !strings.Contains(err.Error(), EnvNATUplinkInterfaces) { - t.Errorf("Validate() = %v, want an error naming %s", err, EnvNATUplinkInterfaces) + if len(cfg.UplinkInterfaces) != 0 { + t.Errorf("UplinkInterfaces = %q, want empty", cfg.UplinkInterfaces) } }) } diff --git a/internal/controller/egressshard_controller.go b/internal/controller/egressshard_controller.go index 53a7a4a1..a731423f 100644 --- a/internal/controller/egressshard_controller.go +++ b/internal/controller/egressshard_controller.go @@ -6,8 +6,10 @@ package controller import ( "context" + "errors" "fmt" "net/netip" + "sync" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -17,18 +19,45 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/manager" ctrlreconcile "sigs.k8s.io/controller-runtime/pkg/reconcile" "go.datum.net/galactic/internal/plumbing/ebpf/uformat" bgpv1alpha1 "go.datum.net/network/api/v1alpha1" ) -// EgressDatapathHealth reports whether this node's egress translation XDP datapath is -// attached and serving traffic. EgressShardReconciler uses it to decide whether -// to set its Ready condition, and it is an interface so tests can fake it. -type EgressDatapathHealth interface { +// EgressShardIdentity is the identity an egress translation datapath is +// programmed with: the shard SID tenant egress routes encapsulate toward, and +// the masquerade address of each family it serves. ShardAddressIPv4 and +// NAT64Prefix are valid together or not at all. +// +// It is this package's own type rather than the datapath's map row so the +// controller package, which every binary links, does not pull in the egress +// translation eBPF objects. +type EgressShardIdentity struct { + ShardSID netip.Addr + ShardAddressIPv6 netip.Addr + ShardAddressIPv4 netip.Addr + NAT64Prefix netip.Prefix +} + +// EgressDatapath is this node's egress translation XDP datapath, as +// EgressShardReconciler drives it. It is an interface so tests can fake it. +type EgressDatapath interface { // Attached reports whether the datapath is loaded and attached. Attached() bool + + // Program replaces the identity the datapath translates with. It rejects + // an identity the datapath cannot translate with, leaving the previous one + // in place. + Program(EgressShardIdentity) error + + // Clear removes the identity, so the datapath claims no packet. + Clear() error + + // Programmed reports the identity the datapath currently translates with, + // and false when it has none. + Programmed() (EgressShardIdentity, bool) } const ( @@ -39,72 +68,74 @@ const ( // reasonEgressDatapathNotAttached is the Ready condition reason while // the datapath is not yet (or no longer) attached. reasonEgressDatapathNotAttached = "DatapathNotAttached" + + // reasonEgressShardConflict is the Programmed condition reason on every + // EgressShard targeting a node that more than one targets. The datapath + // holds one identity, so none of them is programmed until the conflict is + // resolved. + reasonEgressShardConflict = "ShardConflict" ) -// EgressShardReconciler reconciles the single EgressShard object whose -// spec.targetRef.name is this node. It publishes the shard address and SID this -// node's datapath process was started with, echoing the operator-configured -// values rather than deriving them, sets Ready once the datapath is confirmed -// attached, and maintains one BGPAdvertisement carrying a /128 for each. +// EgressShardReconciler programs this node's egress translation datapath from +// the spec of the single EgressShard whose spec.targetRef.name is this node, +// and publishes what the datapath is actually programmed with in its status. +// Ready reports that the datapath is attached; Programmed reports that it is +// translating with the identity the spec assigns. +// +// It also maintains one BGPAdvertisement per shard carrying the shard SID's +// locator and the IPv6 masquerade address, built from status rather than spec +// so the fabric only learns an identity this node actually translates with. // -// That advertisement is what makes both addresses reachable across the fabric, -// in the same route-target-less, VRFID-less shape the gateway's VIP -// advertisements use. Without the SID's route, a tenant VRF default egress -// route points at a SID no node ever learns a kernel route to, so no forward -// traffic reaches this shard. Without the address's route, forward traffic -// arrives and is correctly translated, but the reply has no route back from -// anywhere else on the fabric, so a TCP connection never completes even while -// every forward-path counter looks healthy. +// That advertisement is what makes both reachable across the fabric, in the +// same route-target-less, VRFID-less shape the gateway's VIP advertisements +// use. Without the SID's route, a tenant VRF default egress route points at a +// SID no node ever learns a kernel route to, so no forward traffic reaches this +// shard. Without the address's route, forward traffic arrives and is correctly +// translated, but the reply has no route back from anywhere else on the fabric, +// so a TCP connection never completes even while every forward-path counter +// looks healthy. type EgressShardReconciler struct { client.Client Scheme *runtime.Scheme NodeName string - // ShardSID and the ShardAddress fields are this node's operator-configured - // shard identity, the same values the running datapath was configured with. - // This reconciler publishes them; it does not compute them. - // - // ShardAddressIPv4 and NAT64Prefix are set together or not at all, and only - // on a shard performing NAT64. - ShardSID string - ShardAddressIPv6 string - ShardAddressIPv4 string - NAT64Prefix string - - // Datapath reports whether this node's egress translation datapath is - // currently attached -- see EgressDatapathHealth's doc comment. - Datapath EgressDatapathHealth + // Datapath is this node's egress translation datapath -- see + // EgressDatapath's doc comment. + Datapath EgressDatapath + + // syncMu serializes syncNode between the controller's reconciles and the + // startup sync SetupWithManager registers, which run concurrently. + syncMu sync.Mutex } // Reconcile reconciles a single EgressShard. +// +// The datapath holds one identity per node, so which shard it is programmed +// from is a property of every EgressShard targeting this node together, not of +// the one named in req. Every request therefore ends in syncNode, which +// re-derives the datapath's state from all of them. That is also what clears +// the datapath when this node's shard is deleted: by then the object is gone +// and could not say which node it targeted. func (r *EgressShardReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { logger := log.FromContext(ctx) shard := &bgpv1alpha1.EgressShard{} - if err := r.Get(ctx, req.NamespacedName, shard); err != nil { - if apierrors.IsNotFound(err) { - // EgressShard carries no finalizer, so by the time this Get sees - // NotFound the object is gone everywhere, on whichever node's - // process handled the event and not necessarily the shard's own. - // Withdraw keyed on req.Name, which the advertisement name is - // derived from, rather than on the unreadable deleted object. - if err := withdrawShardAdvertisement(ctx, r.Client, req.Namespace, req.Name); err != nil { - logger.Error(err, "withdraw BGPAdvertisement for deleted EgressShard", "egressShard", req.NamespacedName) - return ctrl.Result{}, err - } - return ctrl.Result{}, nil + err := r.Get(ctx, req.NamespacedName, shard) + switch { + case apierrors.IsNotFound(err): + // EgressShard carries no finalizer, so by the time this Get sees + // NotFound the object is gone everywhere, on whichever node's + // process handled the event and not necessarily the shard's own. + // Withdraw keyed on req.Name, which the advertisement name is + // derived from, rather than on the unreadable deleted object. + if err := withdrawShardAdvertisement(ctx, r.Client, req.Namespace, req.Name); err != nil { + logger.Error(err, "withdraw BGPAdvertisement for deleted EgressShard", "egressShard", req.NamespacedName) + return ctrl.Result{}, err } + case err != nil: return ctrl.Result{}, fmt.Errorf("get EgressShard %s: %w", req.NamespacedName, err) - } - - // Node check: skip shards that don't target this node, mirroring - // NetworkGatewayReconciler's identical targetRef.Name check. - if shard.Spec.TargetRef.Name != r.NodeName { - return ctrl.Result{}, nil - } - - if !shard.DeletionTimestamp.IsZero() { + case !shard.DeletionTimestamp.IsZero() && shard.Spec.TargetRef.Name == r.NodeName: // Not known to be reachable while there is no finalizer, the NotFound // branch above being the one a real delete takes, but kept correct in // case that changes. @@ -112,36 +143,197 @@ func (r *EgressShardReconciler) Reconcile(ctx context.Context, req ctrl.Request) logger.Error(err, "withdraw BGPAdvertisement for terminating EgressShard", "egressShard", req.NamespacedName) return ctrl.Result{}, err } - return ctrl.Result{}, nil } - shardCopy := shard.DeepCopy() - shardCopy.Status.ObservedGeneration = shard.Generation - if r.ShardSID != "" { - shardCopy.Status.ShardSID = r.ShardSID + if err := r.syncNode(ctx); err != nil { + logger.Error(err, "sync egress translation datapath", "node", r.NodeName) + return ctrl.Result{}, err + } + return ctrl.Result{}, nil +} + +// syncNode drives the datapath from every live EgressShard targeting this node +// and publishes the result on each of them: +// +// - none: the datapath is cleared. Nothing assigns this node an identity, so +// it must not keep translating with one a deleted shard left behind. +// - one: the datapath is programmed from its spec. +// - more than one: the datapath is cleared and every one of them reports the +// conflict. Picking one would make which shard translates depend on list +// order, and the other would publish an identity nothing translates with. +func (r *EgressShardReconciler) syncNode(ctx context.Context) error { + if r.Datapath == nil { + return errors.New("no egress translation datapath configured") + } + r.syncMu.Lock() + defer r.syncMu.Unlock() + + shardList := &bgpv1alpha1.EgressShardList{} + if err := r.List(ctx, shardList); err != nil { + return fmt.Errorf("list EgressShards: %w", err) + } + var mine []*bgpv1alpha1.EgressShard + for i := range shardList.Items { + s := &shardList.Items[i] + if s.Spec.TargetRef.Name == r.NodeName && s.DeletionTimestamp.IsZero() { + mine = append(mine, s) + } + } + + var programmed metav1.Condition + switch len(mine) { + case 0: + if err := r.Datapath.Clear(); err != nil { + return fmt.Errorf("clear egress translation datapath: %w", err) + } + return nil + case 1: + var err error + if programmed, err = r.program(mine[0]); err != nil { + // Publish why before returning the error for a retry. + if pubErr := r.publish(ctx, mine[0], programmed); pubErr != nil { + return errors.Join(err, pubErr) + } + return err + } + default: + if err := r.Datapath.Clear(); err != nil { + return fmt.Errorf("clear egress translation datapath: %w", err) + } + programmed = metav1.Condition{ + Type: bgpv1alpha1.ConditionTypeProgrammed, + Status: metav1.ConditionFalse, + Reason: reasonEgressShardConflict, + Message: fmt.Sprintf("%d EgressShards target node %s; exactly one may", len(mine), r.NodeName), + } + } + + var errs []error + for _, s := range mine { + errs = append(errs, r.publish(ctx, s, programmed)) } - if r.ShardAddressIPv6 != "" { - shardCopy.Status.ShardAddressIPv6 = r.ShardAddressIPv6 + return errors.Join(errs...) +} + +// program writes shard's spec into the datapath and returns the resulting +// Programmed condition. A spec that assigns no usable identity yet clears the +// datapath rather than leaving a previous one in place, which is not an error: +// the assignment arrives later as a spec update. +func (r *EgressShardReconciler) program(shard *bgpv1alpha1.EgressShard) (metav1.Condition, error) { + cond := metav1.Condition{Type: bgpv1alpha1.ConditionTypeProgrammed, Status: metav1.ConditionFalse} + + identity, missing, err := identityFromSpec(shard.Spec) + if err != nil { + cond.Reason = bgpv1alpha1.ProgrammedReasonProgrammingFailed + cond.Message = err.Error() + return cond, nil + } + if missing != "" { + if err := r.Datapath.Clear(); err != nil { + return cond, fmt.Errorf("clear egress translation datapath: %w", err) + } + cond.Reason = bgpv1alpha1.ProgrammedReasonAddressUnassigned + cond.Message = missing + return cond, nil + } + if err := r.Datapath.Program(identity); err != nil { + cond.Reason = bgpv1alpha1.ProgrammedReasonProgrammingFailed + cond.Message = err.Error() + return cond, fmt.Errorf("program egress translation datapath from EgressShard %s/%s: %w", + shard.Namespace, shard.Name, err) + } + + cond.Status = metav1.ConditionTrue + cond.Reason = bgpv1alpha1.ProgrammedReasonAddressesProgrammed + cond.Message = "Egress translation datapath is translating with the assigned identity" + return cond, nil +} + +// identityFromSpec parses spec's identity fields. missing is non-empty, with +// the error nil, when the spec does not yet assign enough to translate with: a +// SID and at least one family. err is a value that does not parse, which the +// CRD's own validation should already have rejected. +func identityFromSpec(spec bgpv1alpha1.EgressShardSpec) (identity EgressShardIdentity, missing string, err error) { + parseAddr := func(field, raw string) (netip.Addr, error) { + if raw == "" { + return netip.Addr{}, nil + } + addr, err := netip.ParseAddr(raw) + if err != nil { + return netip.Addr{}, fmt.Errorf("spec.%s: %w", field, err) + } + return addr, nil + } + + if identity.ShardSID, err = parseAddr("shardSID", spec.ShardSID); err != nil { + return identity, "", err + } + if identity.ShardAddressIPv6, err = parseAddr("shardAddressIPv6", spec.ShardAddressIPv6); err != nil { + return identity, "", err + } + if identity.ShardAddressIPv4, err = parseAddr("shardAddressIPv4", spec.ShardAddressIPv4); err != nil { + return identity, "", err + } + if spec.NAT64Prefix != "" { + if identity.NAT64Prefix, err = netip.ParsePrefix(spec.NAT64Prefix); err != nil { + return identity, "", fmt.Errorf("spec.nat64Prefix: %w", err) + } } - if r.ShardAddressIPv4 != "" { - shardCopy.Status.ShardAddressIPv4 = r.ShardAddressIPv4 + + switch { + case !identity.ShardSID.IsValid(): + return identity, "No shard SID is assigned", nil + case !identity.ShardAddressIPv6.IsValid() && !identity.ShardAddressIPv4.IsValid(): + return identity, "No masquerade address is assigned for either family", nil } - if r.NAT64Prefix != "" { - shardCopy.Status.NAT64Prefix = r.NAT64Prefix + return identity, "", nil +} + +// publish writes shard's status from what the datapath is programmed with -- +// not from its spec, so a shard that has not converged shows it -- together +// with the Ready and Programmed conditions, then reconciles its advertisement +// to match. +func (r *EgressShardReconciler) publish(ctx context.Context, shard *bgpv1alpha1.EgressShard, + programmed metav1.Condition, +) error { + shardCopy := shard.DeepCopy() + shardCopy.Status.ObservedGeneration = shard.Generation + shardCopy.Status.ShardSID = "" + shardCopy.Status.ShardAddressIPv6 = "" + shardCopy.Status.ShardAddressIPv4 = "" + shardCopy.Status.NAT64Prefix = "" + // A conflicting shard publishes no identity even while the datapath holds + // one, which it cannot here: syncNode clears it first. Checking the + // condition rather than relying on that keeps the two from drifting. + if programmed.Reason != reasonEgressShardConflict { + if identity, ok := r.Datapath.Programmed(); ok { + shardCopy.Status.ShardSID = addrString(identity.ShardSID) + shardCopy.Status.ShardAddressIPv6 = addrString(identity.ShardAddressIPv6) + shardCopy.Status.ShardAddressIPv4 = addrString(identity.ShardAddressIPv4) + if identity.NAT64Prefix.IsValid() { + shardCopy.Status.NAT64Prefix = identity.NAT64Prefix.String() + } + } } setEgressShardCondition(shardCopy, r.readyCondition()) + setEgressShardCondition(shardCopy, programmed) if err := r.Status().Update(ctx, shardCopy); err != nil { - logger.Error(err, "update EgressShard status", "egressShard", req.NamespacedName) - return ctrl.Result{}, fmt.Errorf("update EgressShard %s status: %w", req.NamespacedName, err) + return fmt.Errorf("update EgressShard %s/%s status: %w", shard.Namespace, shard.Name, err) } - if err := r.applyShardAdvertisement(ctx, shardCopy); err != nil { - logger.Error(err, "apply BGPAdvertisement for EgressShard", "egressShard", req.NamespacedName) - return ctrl.Result{}, err + return fmt.Errorf("apply BGPAdvertisement for EgressShard %s/%s: %w", shard.Namespace, shard.Name, err) } + return nil +} - return ctrl.Result{}, nil +// addrString is addr's string form, or empty for the zero Addr rather than +// netip's "invalid IP". +func addrString(addr netip.Addr) string { + if !addr.IsValid() { + return "" + } + return addr.String() } // shardAdvertisementName derives the deterministic BGPAdvertisement name for a @@ -183,10 +375,10 @@ func shardAdvertisementName(shardName string) string { // the underlay or an upstream announcement has to attract it to this node. See // EgressShard's own field documentation. // -// Either prefix may be independently unset, by an operator who has not finished -// configuring this node's identity, in which case it is omitted rather than -// failing the whole advertisement. Callers treat an empty result as nothing to -// advertise yet, not an error. +// Either prefix may be independently unset -- a NAT64-only shard has no IPv6 +// address, and a shard its node is not translating for has neither -- in which +// case it is omitted rather than failing the whole advertisement. Callers treat +// an empty result as nothing to advertise, not an error. func shardAdvertisementPrefixes(shard *bgpv1alpha1.EgressShard) ([]bgpv1alpha1.Prefix, error) { var prefixes []bgpv1alpha1.Prefix @@ -217,16 +409,17 @@ func shardAdvertisementPrefixes(shard *bgpv1alpha1.EgressShard) ([]bgpv1alpha1.P // covering both prefixes, in the route-target-less, VRFID-less plain // reachability shape the gateway's VIP advertisements use. // -// A no-op rather than an error when neither address is set yet, or when no -// BGPRouter targets this node yet: the BGPRouter watch retries once one -// appears. +// With neither prefix set, which is a shard its node is not translating for, +// any advertisement left from an earlier identity is withdrawn. With no +// BGPRouter targeting this node yet it is a no-op rather than an error: the +// BGPRouter watch retries once one appears. func (r *EgressShardReconciler) applyShardAdvertisement(ctx context.Context, shard *bgpv1alpha1.EgressShard) error { prefixes, err := shardAdvertisementPrefixes(shard) if err != nil { return fmt.Errorf("build advertised prefixes: %w", err) } if len(prefixes) == 0 { - return nil + return withdrawShardAdvertisement(ctx, r.Client, shard.Namespace, shard.Name) } routerName, err := routerNameForNode(ctx, r.Client, shard.Namespace, r.NodeName) @@ -309,7 +502,26 @@ func (r *EgressShardReconciler) readyCondition() metav1.Condition { // node's BGPRouter does not exist yet at first reconcile fails its router // lookup once and gets no second chance until an unrelated event triggers a // fresh reconcile. +// +// The startup runnable closes another. The datapath's maps are pinned, so a +// restarted process inherits whatever identity its predecessor programmed. If +// this node's shard was deleted while no process was running and no other +// EgressShard exists, no event ever arrives to clear it, and the node would go +// on translating for a shard that no longer exists. func (r *EgressShardReconciler) SetupWithManager(mgr ctrl.Manager) error { + if err := mgr.Add(manager.RunnableFunc(func(ctx context.Context) error { + if !mgr.GetCache().WaitForCacheSync(ctx) { + return nil + } + if err := r.syncNode(ctx); err != nil { + // Not fatal: the next EgressShard event syncs again. + log.FromContext(ctx).Error(err, "initial egress translation datapath sync", "node", r.NodeName) + } + return nil + })); err != nil { + return fmt.Errorf("add initial EgressShard sync: %w", err) + } + return ctrl.NewControllerManagedBy(mgr). For(&bgpv1alpha1.EgressShard{}). Watches(&bgpv1alpha1.BGPRouter{}, handler.EnqueueRequestsFromMapFunc( diff --git a/internal/controller/egressshard_controller_test.go b/internal/controller/egressshard_controller_test.go index 91545cf9..878e3142 100644 --- a/internal/controller/egressshard_controller_test.go +++ b/internal/controller/egressshard_controller_test.go @@ -6,6 +6,8 @@ package controller import ( "context" + "errors" + "net/netip" "testing" "k8s.io/apimachinery/pkg/api/meta" @@ -24,6 +26,8 @@ const ( testNAT66NodeB = "node-b" testNAT66ShardName = "node-a" testNAT66ShardAddr = "2001:db8:9999::1" + testNAT64ShardAddr = "192.0.2.10" + testNAT64Prefix = "2001:db8:64::/96" // A well-formed uFMT 48+16 uSID: Block fc00:0001:0002, Node-ID 9, // Function 0xE (uEnd.DT46), Argument 0x001. The shape matters now that the @@ -36,13 +40,40 @@ const ( testNAT66ShardSIDLocator = "fc00:1:2:9::/64" ) -// fakeDatapathHealth is a EgressDatapathHealth test double whose Attached -// return value is directly settable. -type fakeDatapathHealth struct { - attached bool +// fakeEgressDatapath is an EgressDatapath test double holding its programmed +// identity in memory, with settable attachment and Program failure. +type fakeEgressDatapath struct { + attached bool + programErr error + + identity *EgressShardIdentity + programs int + clears int } -func (f *fakeDatapathHealth) Attached() bool { return f.attached } +func (f *fakeEgressDatapath) Attached() bool { return f.attached } + +func (f *fakeEgressDatapath) Program(identity EgressShardIdentity) error { + f.programs++ + if f.programErr != nil { + return f.programErr + } + f.identity = &identity + return nil +} + +func (f *fakeEgressDatapath) Clear() error { + f.clears++ + f.identity = nil + return nil +} + +func (f *fakeEgressDatapath) Programmed() (EgressShardIdentity, bool) { + if f.identity == nil { + return EgressShardIdentity{}, false + } + return *f.identity, true +} func nat66TestScheme(t *testing.T) *runtime.Scheme { t.Helper() @@ -53,35 +84,24 @@ func nat66TestScheme(t *testing.T) *runtime.Scheme { // newEgressShard builds the fixture EgressShard object (always named // testNAT66ShardName, matching every reconcileReq call below) targeting -// nodeName. +// nodeName, with a NAT66 identity assigned in its spec. func newEgressShard(nodeName string) *bgpv1alpha1.EgressShard { return &bgpv1alpha1.EgressShard{ ObjectMeta: metav1.ObjectMeta{Namespace: testNAT66Namespace, Name: testNAT66ShardName}, Spec: bgpv1alpha1.EgressShardSpec{ - TargetRef: bgpv1alpha1.TargetRef{Kind: "Node", Name: nodeName}, + TargetRef: bgpv1alpha1.TargetRef{Kind: "Node", Name: nodeName}, + ShardSID: testNAT66ShardSIDVal, + ShardAddressIPv6: testNAT66ShardAddr, }, } } -// nat66ReconcilerParams bundles newNAT66Reconciler's arguments so the -// per-test call sites don't overflow the line-length limit. -type nat66ReconcilerParams struct { - client client.Client - scheme *runtime.Scheme - nodeName string - addr string - sid string - datapath EgressDatapathHealth -} - -func newNAT66Reconciler(p nat66ReconcilerParams) *EgressShardReconciler { +func newNAT66Reconciler(c client.Client, scheme *runtime.Scheme, datapath EgressDatapath) *EgressShardReconciler { return &EgressShardReconciler{ - Client: p.client, - Scheme: p.scheme, - NodeName: p.nodeName, - ShardAddressIPv6: p.addr, - ShardSID: p.sid, - Datapath: p.datapath, + Client: c, + Scheme: scheme, + NodeName: testNAT66NodeA, + Datapath: datapath, } } @@ -89,162 +109,253 @@ func reconcileReq(name string) ctrl.Request { return ctrl.Request{NamespacedName: client.ObjectKey{Namespace: testNAT66Namespace, Name: name}} } -func TestEgressShardReconciler_NotFoundIsANoop(t *testing.T) { +// reconcileShard reconciles testNAT66ShardName and returns the shard as stored +// afterwards. +func reconcileShard(t *testing.T, r *EgressShardReconciler, c client.Client) (*bgpv1alpha1.EgressShard, error) { + t.Helper() + _, reconcileErr := r.Reconcile(context.Background(), reconcileReq(testNAT66ShardName)) + got := &bgpv1alpha1.EgressShard{} + key := client.ObjectKey{Namespace: testNAT66Namespace, Name: testNAT66ShardName} + if err := c.Get(context.Background(), key, got); err != nil { + t.Fatalf("get shard after reconcile: %v", err) + } + return got, reconcileErr +} + +func assertCondition(t *testing.T, shard *bgpv1alpha1.EgressShard, condType string, + wantStatus metav1.ConditionStatus, wantReason string, +) { + t.Helper() + cond := meta.FindStatusCondition(shard.Status.Conditions, condType) + if cond == nil { + t.Fatalf("%s condition not set", condType) + } + if cond.Status != wantStatus || cond.Reason != wantReason { + t.Errorf("%s condition = %v/%q, want %v/%q", condType, cond.Status, cond.Reason, wantStatus, wantReason) + } +} + +func TestEgressShardReconciler_NotFoundClearsTheDatapath(t *testing.T) { scheme := nat66TestScheme(t) c := newIndexedClientBuilder(scheme).Build() - r := newNAT66Reconciler(nat66ReconcilerParams{ - client: c, scheme: scheme, nodeName: testNAT66NodeA, - addr: testNAT66ShardAddr, sid: testNAT66ShardSIDVal, datapath: &fakeDatapathHealth{attached: true}, - }) + datapath := &fakeEgressDatapath{attached: true, identity: &EgressShardIdentity{}} + r := newNAT66Reconciler(c, scheme, datapath) if _, err := r.Reconcile(context.Background(), reconcileReq("does-not-exist")); err != nil { t.Fatalf("Reconcile() error = %v, want nil for a NotFound object", err) } + // No shard targets this node, so an identity inherited from a previous + // process's pinned map must not survive. + if _, ok := datapath.Programmed(); ok { + t.Error("datapath still programmed with no EgressShard targeting this node") + } } func TestEgressShardReconciler_SkipsShardForAnotherNode(t *testing.T) { scheme := nat66TestScheme(t) shard := newEgressShard(testNAT66NodeB) c := newIndexedClientBuilder(scheme).WithObjects(shard).WithStatusSubresource(shard).Build() - r := newNAT66Reconciler(nat66ReconcilerParams{ - client: c, scheme: scheme, nodeName: testNAT66NodeA, - addr: testNAT66ShardAddr, sid: testNAT66ShardSIDVal, datapath: &fakeDatapathHealth{attached: true}, - }) + datapath := &fakeEgressDatapath{attached: true} + r := newNAT66Reconciler(c, scheme, datapath) - if _, err := r.Reconcile(context.Background(), reconcileReq(testNAT66ShardName)); err != nil { + got, err := reconcileShard(t, r, c) + if err != nil { t.Fatalf("Reconcile() error = %v", err) } - - got := &bgpv1alpha1.EgressShard{} - if err := c.Get(context.Background(), client.ObjectKeyFromObject(shard), got); err != nil { - t.Fatalf("get shard after reconcile: %v", err) + if datapath.programs != 0 { + t.Errorf("datapath programmed %d times from a shard targeting another node", datapath.programs) } - if got.Status.ShardAddressIPv6 != "" { - t.Errorf("Status.ShardAddressIPv6 = %q, want untouched empty string (shard targets another node)", - got.Status.ShardAddressIPv6) - } - if len(got.Status.Conditions) != 0 { - t.Errorf("Status.Conditions = %+v, want untouched empty slice (shard targets another node)", - got.Status.Conditions) + if got.Status.ShardAddressIPv6 != "" || len(got.Status.Conditions) != 0 { + t.Errorf("Status = %+v, want untouched (shard targets another node)", got.Status) } } -func TestEgressShardReconciler_PublishesStatusWhenAttached(t *testing.T) { +func TestEgressShardReconciler_ProgramsDatapathFromSpec(t *testing.T) { scheme := nat66TestScheme(t) shard := newEgressShard(testNAT66NodeA) + shard.Spec.ShardAddressIPv4 = testNAT64ShardAddr + shard.Spec.NAT64Prefix = testNAT64Prefix c := newIndexedClientBuilder(scheme).WithObjects(shard).WithStatusSubresource(shard).Build() - r := newNAT66Reconciler(nat66ReconcilerParams{ - client: c, scheme: scheme, nodeName: testNAT66NodeA, - addr: testNAT66ShardAddr, sid: testNAT66ShardSIDVal, datapath: &fakeDatapathHealth{attached: true}, - }) + datapath := &fakeEgressDatapath{attached: true} + r := newNAT66Reconciler(c, scheme, datapath) - if _, err := r.Reconcile(context.Background(), reconcileReq(testNAT66ShardName)); err != nil { + got, err := reconcileShard(t, r, c) + if err != nil { t.Fatalf("Reconcile() error = %v", err) } - got := &bgpv1alpha1.EgressShard{} - if err := c.Get(context.Background(), client.ObjectKeyFromObject(shard), got); err != nil { - t.Fatalf("get shard after reconcile: %v", err) + want := EgressShardIdentity{ + ShardSID: netip.MustParseAddr(testNAT66ShardSIDVal), + ShardAddressIPv6: netip.MustParseAddr(testNAT66ShardAddr), + ShardAddressIPv4: netip.MustParseAddr(testNAT64ShardAddr), + NAT64Prefix: netip.MustParsePrefix(testNAT64Prefix), } - if got.Status.ShardAddressIPv6 != testNAT66ShardAddr { - t.Errorf("Status.ShardAddressIPv6 = %q, want %q", got.Status.ShardAddressIPv6, testNAT66ShardAddr) + if identity, ok := datapath.Programmed(); !ok || identity != want { + t.Errorf("datapath identity = %+v (programmed %v), want %+v", identity, ok, want) } + if got.Status.ShardSID != testNAT66ShardSIDVal { t.Errorf("Status.ShardSID = %q, want %q", got.Status.ShardSID, testNAT66ShardSIDVal) } - if got.Status.ObservedGeneration != shard.Generation { - t.Errorf("Status.ObservedGeneration = %d, want %d", got.Status.ObservedGeneration, shard.Generation) + if got.Status.ShardAddressIPv6 != testNAT66ShardAddr { + t.Errorf("Status.ShardAddressIPv6 = %q, want %q", got.Status.ShardAddressIPv6, testNAT66ShardAddr) } - - cond := meta.FindStatusCondition(got.Status.Conditions, bgpv1alpha1.ConditionTypeReady) - if cond == nil { - t.Fatal("Ready condition not set") + if got.Status.ShardAddressIPv4 != testNAT64ShardAddr { + t.Errorf("Status.ShardAddressIPv4 = %q, want %q", got.Status.ShardAddressIPv4, testNAT64ShardAddr) } - if cond.Status != metav1.ConditionTrue { - t.Errorf("Ready condition status = %v, want True", cond.Status) + if got.Status.NAT64Prefix != testNAT64Prefix { + t.Errorf("Status.NAT64Prefix = %q, want %q", got.Status.NAT64Prefix, testNAT64Prefix) } - if cond.Reason != reasonEgressDatapathAttached { - t.Errorf("Ready condition reason = %q, want %q", cond.Reason, reasonEgressDatapathAttached) + if got.Status.ObservedGeneration != shard.Generation { + t.Errorf("Status.ObservedGeneration = %d, want %d", got.Status.ObservedGeneration, shard.Generation) } + assertCondition(t, got, bgpv1alpha1.ConditionTypeReady, metav1.ConditionTrue, reasonEgressDatapathAttached) + assertCondition(t, got, bgpv1alpha1.ConditionTypeProgrammed, metav1.ConditionTrue, + bgpv1alpha1.ProgrammedReasonAddressesProgrammed) } func TestEgressShardReconciler_ReadyFalseWhenNotAttached(t *testing.T) { scheme := nat66TestScheme(t) shard := newEgressShard(testNAT66NodeA) c := newIndexedClientBuilder(scheme).WithObjects(shard).WithStatusSubresource(shard).Build() - r := newNAT66Reconciler(nat66ReconcilerParams{ - client: c, scheme: scheme, nodeName: testNAT66NodeA, - addr: testNAT66ShardAddr, sid: testNAT66ShardSIDVal, datapath: &fakeDatapathHealth{attached: false}, - }) + r := newNAT66Reconciler(c, scheme, &fakeEgressDatapath{attached: false}) - if _, err := r.Reconcile(context.Background(), reconcileReq(testNAT66ShardName)); err != nil { + got, err := reconcileShard(t, r, c) + if err != nil { t.Fatalf("Reconcile() error = %v", err) } + assertCondition(t, got, bgpv1alpha1.ConditionTypeReady, metav1.ConditionFalse, reasonEgressDatapathNotAttached) +} - got := &bgpv1alpha1.EgressShard{} - if err := c.Get(context.Background(), client.ObjectKeyFromObject(shard), got); err != nil { - t.Fatalf("get shard after reconcile: %v", err) - } +func TestEgressShardReconciler_NilDatapathIsAnError(t *testing.T) { + scheme := nat66TestScheme(t) + shard := newEgressShard(testNAT66NodeA) + c := newIndexedClientBuilder(scheme).WithObjects(shard).WithStatusSubresource(shard).Build() + r := newNAT66Reconciler(c, scheme, nil) - cond := meta.FindStatusCondition(got.Status.Conditions, bgpv1alpha1.ConditionTypeReady) - if cond == nil { - t.Fatal("Ready condition not set") - } - if cond.Status != metav1.ConditionFalse { - t.Errorf("Ready condition status = %v, want False", cond.Status) + if _, err := reconcileShard(t, r, c); err == nil { + t.Fatal("Reconcile() error = nil, want an error with no datapath to program") } - if cond.Reason != reasonEgressDatapathNotAttached { - t.Errorf("Ready condition reason = %q, want %q", cond.Reason, reasonEgressDatapathNotAttached) +} + +// TestEgressShardReconciler_UnassignedIdentityClearsTheDatapath covers a shard +// whose spec does not yet assign enough to translate with. It exists from the +// moment its node is labelled and gains an identity afterwards, so this is a +// normal state rather than an error, and the datapath must not keep one the +// spec does not assign. +func TestEgressShardReconciler_UnassignedIdentityClearsTheDatapath(t *testing.T) { + tests := []struct { + name string + mutate func(*bgpv1alpha1.EgressShardSpec) + }{ + {name: "no SID", mutate: func(s *bgpv1alpha1.EgressShardSpec) { s.ShardSID = "" }}, + {name: "no address for either family", mutate: func(s *bgpv1alpha1.EgressShardSpec) { s.ShardAddressIPv6 = "" }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + scheme := nat66TestScheme(t) + shard := newEgressShard(testNAT66NodeA) + tt.mutate(&shard.Spec) + c := newIndexedClientBuilder(scheme).WithObjects(shard, newTestNAT66Router()). + WithStatusSubresource(shard).Build() + datapath := &fakeEgressDatapath{attached: true, identity: &EgressShardIdentity{}} + r := newNAT66Reconciler(c, scheme, datapath) + + got, err := reconcileShard(t, r, c) + if err != nil { + t.Fatalf("Reconcile() error = %v", err) + } + if _, ok := datapath.Programmed(); ok || datapath.programs != 0 { + t.Errorf("datapath programmed from a spec with no usable identity") + } + if got.Status.ShardSID != "" || got.Status.ShardAddressIPv6 != "" { + t.Errorf("Status = %+v, want no identity published", got.Status) + } + assertCondition(t, got, bgpv1alpha1.ConditionTypeProgrammed, metav1.ConditionFalse, + bgpv1alpha1.ProgrammedReasonAddressUnassigned) + assertNoAdvertisement(t, c) + }) } } -func TestEgressShardReconciler_NilDatapathIsNotAttached(t *testing.T) { +// TestEgressShardReconciler_StatusReportsTheDatapathNotTheSpec covers the +// status contract: when programming fails, status must keep reporting what +// the datapath still translates with, not the spec it failed to apply. +func TestEgressShardReconciler_StatusReportsTheDatapathNotTheSpec(t *testing.T) { scheme := nat66TestScheme(t) shard := newEgressShard(testNAT66NodeA) c := newIndexedClientBuilder(scheme).WithObjects(shard).WithStatusSubresource(shard).Build() - r := newNAT66Reconciler(nat66ReconcilerParams{ - client: c, scheme: scheme, nodeName: testNAT66NodeA, - addr: testNAT66ShardAddr, sid: testNAT66ShardSIDVal, datapath: nil, - }) + previous := EgressShardIdentity{ + ShardSID: netip.MustParseAddr("fc00:1:2:8::"), + ShardAddressIPv6: netip.MustParseAddr("2001:db8:8888::1"), + } + datapath := &fakeEgressDatapath{attached: true, identity: &previous, programErr: errors.New("map write failed")} + r := newNAT66Reconciler(c, scheme, datapath) - if _, err := r.Reconcile(context.Background(), reconcileReq(testNAT66ShardName)); err != nil { - t.Fatalf("Reconcile() error = %v", err) + got, err := reconcileShard(t, r, c) + if err == nil { + t.Fatal("Reconcile() error = nil, want the programming failure returned for a retry") + } + if got.Status.ShardAddressIPv6 != "2001:db8:8888::1" { + t.Errorf("Status.ShardAddressIPv6 = %q, want the still-programmed %q", got.Status.ShardAddressIPv6, + "2001:db8:8888::1") } + assertCondition(t, got, bgpv1alpha1.ConditionTypeProgrammed, metav1.ConditionFalse, + bgpv1alpha1.ProgrammedReasonProgrammingFailed) +} - got := &bgpv1alpha1.EgressShard{} - if err := c.Get(context.Background(), client.ObjectKeyFromObject(shard), got); err != nil { - t.Fatalf("get shard after reconcile: %v", err) +// TestEgressShardReconciler_ConflictingShardsProgramNone covers two +// EgressShards targeting one node. The datapath holds one identity, and +// picking one would make which shard translates depend on list order. +func TestEgressShardReconciler_ConflictingShardsProgramNone(t *testing.T) { + scheme := nat66TestScheme(t) + shard := newEgressShard(testNAT66NodeA) + other := newEgressShard(testNAT66NodeA) + other.Name = "node-a-duplicate" + other.Spec.ShardSID = "fc00:1:2:a::" + c := newIndexedClientBuilder(scheme).WithObjects(shard, other, newTestNAT66Router()). + WithStatusSubresource(shard, other).Build() + datapath := &fakeEgressDatapath{attached: true} + r := newNAT66Reconciler(c, scheme, datapath) + + got, err := reconcileShard(t, r, c) + if err != nil { + t.Fatalf("Reconcile() error = %v", err) } - cond := meta.FindStatusCondition(got.Status.Conditions, bgpv1alpha1.ConditionTypeReady) - if cond == nil { - t.Fatal("Ready condition not set") + if _, ok := datapath.Programmed(); ok || datapath.programs != 0 { + t.Error("datapath programmed while two EgressShards target this node") + } + + gotOther := &bgpv1alpha1.EgressShard{} + if err := c.Get(context.Background(), client.ObjectKeyFromObject(other), gotOther); err != nil { + t.Fatalf("get other shard: %v", err) } - if cond.Status != metav1.ConditionFalse { - t.Errorf("Ready condition status = %v, want False for a nil Datapath", cond.Status) + for _, s := range []*bgpv1alpha1.EgressShard{got, gotOther} { + assertCondition(t, s, bgpv1alpha1.ConditionTypeProgrammed, metav1.ConditionFalse, reasonEgressShardConflict) + if s.Status.ShardSID != "" { + t.Errorf("%s Status.ShardSID = %q, want none published while in conflict", s.Name, s.Status.ShardSID) + } } + assertNoAdvertisement(t, c) } -func TestEgressShardReconciler_DeletingShardIsANoop(t *testing.T) { +func TestEgressShardReconciler_TerminatingShardClearsTheDatapath(t *testing.T) { scheme := nat66TestScheme(t) shard := newEgressShard(testNAT66NodeA) shard.Finalizers = []string{"test.datum.net/keep"} // required for the fake client to accept a deletion timestamp c := newIndexedClientBuilder(scheme).WithObjects(shard).WithStatusSubresource(shard).Build() - if err := c.Delete(context.Background(), shard); err != nil { t.Fatalf("delete shard: %v", err) } + datapath := &fakeEgressDatapath{attached: true, identity: &EgressShardIdentity{}} + r := newNAT66Reconciler(c, scheme, datapath) - r := newNAT66Reconciler(nat66ReconcilerParams{ - client: c, scheme: scheme, nodeName: testNAT66NodeA, - addr: testNAT66ShardAddr, sid: testNAT66ShardSIDVal, datapath: &fakeDatapathHealth{attached: true}, - }) - if _, err := r.Reconcile(context.Background(), reconcileReq(testNAT66ShardName)); err != nil { + got, err := reconcileShard(t, r, c) + if err != nil { t.Fatalf("Reconcile() error = %v", err) } - - got := &bgpv1alpha1.EgressShard{} - if err := c.Get(context.Background(), client.ObjectKeyFromObject(shard), got); err != nil { - t.Fatalf("get shard after reconcile: %v", err) + if _, ok := datapath.Programmed(); ok { + t.Error("datapath still programmed for a terminating EgressShard") } if got.Status.ShardAddressIPv6 != "" { t.Errorf("Status.ShardAddressIPv6 = %q, want untouched empty string for a terminating shard", @@ -252,34 +363,13 @@ func TestEgressShardReconciler_DeletingShardIsANoop(t *testing.T) { } } -func TestEgressShardReconciler_EmptyConfiguredValuesLeaveStatusUntouched(t *testing.T) { - scheme := nat66TestScheme(t) - shard := newEgressShard(testNAT66NodeA) - shard.Status.ShardAddressIPv6 = testNAT66ShardAddr - shard.Status.ShardSID = testNAT66ShardSIDVal - c := newIndexedClientBuilder(scheme).WithObjects(shard).WithStatusSubresource(shard).Build() - - // A reconciler started with empty ShardAddress/ShardSID (shouldn't - // happen in production -- config.NAT66Config.Validate requires both -- - // but must not clobber a previously-published value with an empty - // string if it somehow does). - r := newNAT66Reconciler(nat66ReconcilerParams{ - client: c, scheme: scheme, nodeName: testNAT66NodeA, - addr: "", sid: "", datapath: &fakeDatapathHealth{attached: true}, - }) - if _, err := r.Reconcile(context.Background(), reconcileReq(testNAT66ShardName)); err != nil { - t.Fatalf("Reconcile() error = %v", err) - } - - got := &bgpv1alpha1.EgressShard{} - if err := c.Get(context.Background(), client.ObjectKeyFromObject(shard), got); err != nil { - t.Fatalf("get shard after reconcile: %v", err) - } - if got.Status.ShardAddressIPv6 != testNAT66ShardAddr { - t.Errorf("Status.ShardAddressIPv6 = %q, want untouched %q", got.Status.ShardAddressIPv6, testNAT66ShardAddr) - } - if got.Status.ShardSID != testNAT66ShardSIDVal { - t.Errorf("Status.ShardSID = %q, want untouched %q", got.Status.ShardSID, testNAT66ShardSIDVal) +// assertNoAdvertisement fails if testNAT66ShardName's BGPAdvertisement exists. +func assertNoAdvertisement(t *testing.T, c client.Client) { + t.Helper() + adv := &bgpv1alpha1.BGPAdvertisement{} + advKey := client.ObjectKey{Namespace: testNAT66Namespace, Name: shardAdvertisementName(testNAT66ShardName)} + if err := c.Get(context.Background(), advKey, adv); err == nil { + t.Errorf("BGPAdvertisement %v exists, want none", advKey) } } @@ -291,11 +381,8 @@ const testNAT66RouterName = "node-a-router" // newTestNAT66Router returns a BGPRouter named testNAT66RouterName, // targeting testNAT66NodeA, resolved through the BGPRouterByTargetName // index newIndexedClientBuilder (shared with -// networkgateway_controller_test.go) registers. Every call site below -// reconciles against testNAT66NodeA, so unlike newEgressShard/ -// newNAT66Reconciler (which do vary their own node-related argument -// across tests, e.g. TestEgressShardReconciler_SkipsShardForAnotherNode), -// this takes no arguments at all. +// networkgateway_controller_test.go) registers. Every reconciler here runs +// as testNAT66NodeA, so this takes no arguments at all. func newTestNAT66Router() *bgpv1alpha1.BGPRouter { return &bgpv1alpha1.BGPRouter{ ObjectMeta: metav1.ObjectMeta{Namespace: testNAT66Namespace, Name: testNAT66RouterName}, @@ -303,52 +390,6 @@ func newTestNAT66Router() *bgpv1alpha1.BGPRouter { } } -// TestEgressShardReconciler_CreatesAdvertisementForBothSIDAndAddress covers -// the 2026-08-19 fix: the shard's advertisement must carry ShardAddress -// (the return leg -- see shardAdvertisementPrefixes' own doc comment) as -// well as ShardSID (the forward leg), not just the latter -- a real TCP -// connection through NAT66 never completed while only ShardSID was -// advertised, since no route back to ShardAddress existed anywhere else -// on the fabric. -func TestEgressShardReconciler_CreatesAdvertisementForBothSIDAndAddress(t *testing.T) { - scheme := nat66TestScheme(t) - shard := newEgressShard(testNAT66NodeA) - router := newTestNAT66Router() - c := newIndexedClientBuilder(scheme).WithObjects(shard, router).WithStatusSubresource(shard).Build() - r := newNAT66Reconciler(nat66ReconcilerParams{ - client: c, scheme: scheme, nodeName: testNAT66NodeA, - addr: testNAT66ShardAddr, sid: testNAT66ShardSIDVal, datapath: &fakeDatapathHealth{attached: true}, - }) - - if _, err := r.Reconcile(context.Background(), reconcileReq(testNAT66ShardName)); err != nil { - t.Fatalf("Reconcile() error = %v", err) - } - - adv := &bgpv1alpha1.BGPAdvertisement{} - advKey := client.ObjectKey{Namespace: testNAT66Namespace, Name: shardAdvertisementName(testNAT66ShardName)} - if err := c.Get(context.Background(), advKey, adv); err != nil { - t.Fatalf("get shard BGPAdvertisement: %v", err) - } - if adv.Spec.RouterRef.Name != router.Name { - t.Errorf("Spec.RouterRef.Name = %q, want %q", adv.Spec.RouterRef.Name, router.Name) - } - if adv.Spec.AddressFamily.AFI != bgpv1alpha1.AFIL2VPN || adv.Spec.AddressFamily.SAFI != bgpv1alpha1.SAFIEVPN { - t.Errorf("Spec.AddressFamily = %+v, want l2vpn/evpn", adv.Spec.AddressFamily) - } - wantPrefixes := []bgpv1alpha1.Prefix{ - bgpv1alpha1.Prefix(testNAT66ShardSIDLocator), - bgpv1alpha1.Prefix(testNAT66ShardAddr + "/128"), - } - if len(adv.Spec.Prefixes) != len(wantPrefixes) { - t.Fatalf("Spec.Prefixes = %+v, want %+v", adv.Spec.Prefixes, wantPrefixes) - } - for i, want := range wantPrefixes { - if adv.Spec.Prefixes[i] != want { - t.Errorf("Spec.Prefixes[%d] = %s, want %s", i, adv.Spec.Prefixes[i], want) - } - } -} - // TestShardAdvertisementPrefixes_AdvertisesTheSIDsCoveringLocator is the // control-plane half of #538. A tenant VRF's egress route encapsulates toward // this shard with that tenant's own Argument written into the SID, so the @@ -429,24 +470,20 @@ func TestShardAdvertisementPrefixes_ShardAddressStaysAHostRoute(t *testing.T) { } } -// TestEgressShardReconciler_AdvertisesShardAddressAloneWhenSIDUnset covers -// shardAdvertisementPrefixes' "either may be independently unset" claim -// from the other direction: with no ShardSID configured at all (an -// operator mid-rollout, or a shard that only participates in the return -// leg), ShardAddress alone must still be advertised -- the old -// implementation's `if shard.Status.ShardSID == "" { return nil }` guard -// would have skipped this entirely. -func TestEgressShardReconciler_AdvertisesShardAddressAloneWhenSIDUnset(t *testing.T) { +// TestEgressShardReconciler_CreatesAdvertisementForBothSIDAndAddress covers +// the 2026-08-19 fix: the shard's advertisement must carry the IPv6 shard +// address (the return leg -- see shardAdvertisementPrefixes' own doc comment) +// as well as the SID (the forward leg), not just the latter -- a real TCP +// connection through NAT66 never completed while only the SID was advertised, +// since no route back to the address existed anywhere else on the fabric. +func TestEgressShardReconciler_CreatesAdvertisementForBothSIDAndAddress(t *testing.T) { scheme := nat66TestScheme(t) shard := newEgressShard(testNAT66NodeA) router := newTestNAT66Router() c := newIndexedClientBuilder(scheme).WithObjects(shard, router).WithStatusSubresource(shard).Build() - r := newNAT66Reconciler(nat66ReconcilerParams{ - client: c, scheme: scheme, nodeName: testNAT66NodeA, - addr: testNAT66ShardAddr, sid: "", datapath: &fakeDatapathHealth{attached: true}, - }) + r := newNAT66Reconciler(c, scheme, &fakeEgressDatapath{attached: true}) - if _, err := r.Reconcile(context.Background(), reconcileReq(testNAT66ShardName)); err != nil { + if _, err := reconcileShard(t, r, c); err != nil { t.Fatalf("Reconcile() error = %v", err) } @@ -455,123 +492,134 @@ func TestEgressShardReconciler_AdvertisesShardAddressAloneWhenSIDUnset(t *testin if err := c.Get(context.Background(), advKey, adv); err != nil { t.Fatalf("get shard BGPAdvertisement: %v", err) } - wantPrefix := bgpv1alpha1.Prefix(testNAT66ShardAddr + "/128") - if len(adv.Spec.Prefixes) != 1 || adv.Spec.Prefixes[0] != wantPrefix { - t.Errorf("Spec.Prefixes = %+v, want [%s]", adv.Spec.Prefixes, wantPrefix) + if adv.Spec.RouterRef.Name != router.Name { + t.Errorf("Spec.RouterRef.Name = %q, want %q", adv.Spec.RouterRef.Name, router.Name) + } + if adv.Spec.AddressFamily.AFI != bgpv1alpha1.AFIL2VPN || adv.Spec.AddressFamily.SAFI != bgpv1alpha1.SAFIEVPN { + t.Errorf("Spec.AddressFamily = %+v, want l2vpn/evpn", adv.Spec.AddressFamily) + } + wantPrefixes := []bgpv1alpha1.Prefix{ + bgpv1alpha1.Prefix(testNAT66ShardSIDLocator), + bgpv1alpha1.Prefix(testNAT66ShardAddr + "/128"), + } + if len(adv.Spec.Prefixes) != len(wantPrefixes) { + t.Fatalf("Spec.Prefixes = %+v, want %+v", adv.Spec.Prefixes, wantPrefixes) + } + for i, want := range wantPrefixes { + if adv.Spec.Prefixes[i] != want { + t.Errorf("Spec.Prefixes[%d] = %s, want %s", i, adv.Spec.Prefixes[i], want) + } } } -// TestEgressShardReconciler_SkipsAdvertisementWhenNeitherSIDNorAddressSet -// covers shardAdvertisementPrefixes' all-empty case: no advertisement at -// all, not an error, when an operator hasn't configured either value yet. -func TestEgressShardReconciler_SkipsAdvertisementWhenNeitherSIDNorAddressSet(t *testing.T) { +// TestEgressShardReconciler_NAT64OnlyShardAdvertisesTheSIDAlone covers +// shardAdvertisementPrefixes' "either may be independently unset" claim: a +// shard serving only NAT64 has no IPv6 masquerade address, and its SID must +// still be advertised. The IPv4 address never is -- see +// shardAdvertisementPrefixes' doc comment. +func TestEgressShardReconciler_NAT64OnlyShardAdvertisesTheSIDAlone(t *testing.T) { scheme := nat66TestScheme(t) shard := newEgressShard(testNAT66NodeA) - router := newTestNAT66Router() - c := newIndexedClientBuilder(scheme).WithObjects(shard, router).WithStatusSubresource(shard).Build() - r := newNAT66Reconciler(nat66ReconcilerParams{ - client: c, scheme: scheme, nodeName: testNAT66NodeA, - addr: "", sid: "", datapath: &fakeDatapathHealth{attached: true}, - }) + shard.Spec.ShardAddressIPv6 = "" + shard.Spec.ShardAddressIPv4 = testNAT64ShardAddr + shard.Spec.NAT64Prefix = testNAT64Prefix + c := newIndexedClientBuilder(scheme).WithObjects(shard, newTestNAT66Router()).WithStatusSubresource(shard).Build() + r := newNAT66Reconciler(c, scheme, &fakeEgressDatapath{attached: true}) - if _, err := r.Reconcile(context.Background(), reconcileReq(testNAT66ShardName)); err != nil { + if _, err := reconcileShard(t, r, c); err != nil { t.Fatalf("Reconcile() error = %v", err) } adv := &bgpv1alpha1.BGPAdvertisement{} advKey := client.ObjectKey{Namespace: testNAT66Namespace, Name: shardAdvertisementName(testNAT66ShardName)} - if err := c.Get(context.Background(), advKey, adv); err == nil { - t.Fatalf("BGPAdvertisement %v unexpectedly created with neither ShardSID nor ShardAddress configured", advKey) + if err := c.Get(context.Background(), advKey, adv); err != nil { + t.Fatalf("get shard BGPAdvertisement: %v", err) + } + wantPrefix := bgpv1alpha1.Prefix(testNAT66ShardSIDLocator) + if len(adv.Spec.Prefixes) != 1 || adv.Spec.Prefixes[0] != wantPrefix { + t.Errorf("Spec.Prefixes = %+v, want [%s]", adv.Spec.Prefixes, wantPrefix) } } +// TestEgressShardReconciler_WithdrawsStaleAdvertisementWhenUnprogrammed +// covers the advertisement following status: a shard its node no longer +// translates for must not keep attracting traffic to that node. +func TestEgressShardReconciler_WithdrawsStaleAdvertisementWhenUnprogrammed(t *testing.T) { + scheme := nat66TestScheme(t) + shard := newEgressShard(testNAT66NodeA) + shard.Spec.ShardSID = "" + c := newIndexedClientBuilder(scheme).WithObjects(shard, newTestNAT66Router(), staleShardAdvertisement()). + WithStatusSubresource(shard).Build() + r := newNAT66Reconciler(c, scheme, &fakeEgressDatapath{attached: true}) + + if _, err := reconcileShard(t, r, c); err != nil { + t.Fatalf("Reconcile() error = %v", err) + } + assertNoAdvertisement(t, c) +} + func TestEgressShardReconciler_SkipsAdvertisementWithoutRouter(t *testing.T) { scheme := nat66TestScheme(t) shard := newEgressShard(testNAT66NodeA) c := newIndexedClientBuilder(scheme).WithObjects(shard).WithStatusSubresource(shard).Build() - r := newNAT66Reconciler(nat66ReconcilerParams{ - client: c, scheme: scheme, nodeName: testNAT66NodeA, - addr: testNAT66ShardAddr, sid: testNAT66ShardSIDVal, datapath: &fakeDatapathHealth{attached: true}, - }) + r := newNAT66Reconciler(c, scheme, &fakeEgressDatapath{attached: true}) - if _, err := r.Reconcile(context.Background(), reconcileReq(testNAT66ShardName)); err != nil { + if _, err := reconcileShard(t, r, c); err != nil { t.Fatalf("Reconcile() error = %v, want nil even with no BGPRouter for this node yet", err) } - - adv := &bgpv1alpha1.BGPAdvertisement{} - advKey := client.ObjectKey{Namespace: testNAT66Namespace, Name: shardAdvertisementName(testNAT66ShardName)} - if err := c.Get(context.Background(), advKey, adv); err == nil { - t.Fatalf("BGPAdvertisement %v unexpectedly created with no BGPRouter for this node", advKey) - } + assertNoAdvertisement(t, c) } -func TestEgressShardReconciler_WithdrawsAdvertisementOnDelete(t *testing.T) { - scheme := nat66TestScheme(t) - shard := newEgressShard(testNAT66NodeA) - shard.Finalizers = []string{"test.datum.net/keep"} // required for the fake client to accept a deletion timestamp - router := newTestNAT66Router() - adv := &bgpv1alpha1.BGPAdvertisement{ +// staleShardAdvertisement is the BGPAdvertisement a previous reconcile of +// testNAT66ShardName left behind. +func staleShardAdvertisement() *bgpv1alpha1.BGPAdvertisement { + return &bgpv1alpha1.BGPAdvertisement{ ObjectMeta: metav1.ObjectMeta{ Namespace: testNAT66Namespace, Name: shardAdvertisementName(testNAT66ShardName), }, Spec: bgpv1alpha1.BGPAdvertisementSpec{ - RouterRef: bgpv1alpha1.RouterRef{Name: router.Name}, + RouterRef: bgpv1alpha1.RouterRef{Name: testNAT66RouterName}, AddressFamily: bgpv1alpha1.AddressFamily{AFI: bgpv1alpha1.AFIL2VPN, SAFI: bgpv1alpha1.SAFIEVPN}, - Prefixes: []bgpv1alpha1.Prefix{bgpv1alpha1.Prefix(testNAT66ShardSIDVal + "/128")}, + Prefixes: []bgpv1alpha1.Prefix{bgpv1alpha1.Prefix(testNAT66ShardSIDLocator)}, }, } - c := newIndexedClientBuilder(scheme).WithObjects(shard, router, adv).WithStatusSubresource(shard).Build() +} +func TestEgressShardReconciler_WithdrawsAdvertisementOnDelete(t *testing.T) { + scheme := nat66TestScheme(t) + shard := newEgressShard(testNAT66NodeA) + shard.Finalizers = []string{"test.datum.net/keep"} // required for the fake client to accept a deletion timestamp + c := newIndexedClientBuilder(scheme).WithObjects(shard, newTestNAT66Router(), staleShardAdvertisement()). + WithStatusSubresource(shard).Build() if err := c.Delete(context.Background(), shard); err != nil { t.Fatalf("delete shard: %v", err) } + r := newNAT66Reconciler(c, scheme, &fakeEgressDatapath{attached: true}) - r := newNAT66Reconciler(nat66ReconcilerParams{ - client: c, scheme: scheme, nodeName: testNAT66NodeA, - addr: testNAT66ShardAddr, sid: testNAT66ShardSIDVal, datapath: &fakeDatapathHealth{attached: true}, - }) - if _, err := r.Reconcile(context.Background(), reconcileReq(testNAT66ShardName)); err != nil { + if _, err := reconcileShard(t, r, c); err != nil { t.Fatalf("Reconcile() error = %v", err) } - - got := &bgpv1alpha1.BGPAdvertisement{} - if err := c.Get(context.Background(), client.ObjectKeyFromObject(adv), got); err == nil { - t.Fatalf("BGPAdvertisement %v still exists after deleting its EgressShard", client.ObjectKeyFromObject(adv)) - } + assertNoAdvertisement(t, c) } func TestEgressShardReconciler_WithdrawsAdvertisementWhenShardObjectAlreadyGone(t *testing.T) { // Mirrors NetworkGatewayReconciler's own req.Name-keyed withdrawal in // its NotFound branch: this reconciler never observes a live shard // object at all here, only the deletion event's req.Name, and must - // still withdraw the advertisement it created (see the Reconcile - // NotFound branch's own doc comment for why no finalizer is used). + // still withdraw the advertisement it created and stop translating + // (see the Reconcile NotFound branch's own doc comment for why no + // finalizer is used). scheme := nat66TestScheme(t) - router := newTestNAT66Router() - adv := &bgpv1alpha1.BGPAdvertisement{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: testNAT66Namespace, - Name: shardAdvertisementName(testNAT66ShardName), - }, - Spec: bgpv1alpha1.BGPAdvertisementSpec{ - RouterRef: bgpv1alpha1.RouterRef{Name: router.Name}, - AddressFamily: bgpv1alpha1.AddressFamily{AFI: bgpv1alpha1.AFIL2VPN, SAFI: bgpv1alpha1.SAFIEVPN}, - Prefixes: []bgpv1alpha1.Prefix{bgpv1alpha1.Prefix(testNAT66ShardSIDVal + "/128")}, - }, - } - c := newIndexedClientBuilder(scheme).WithObjects(router, adv).Build() - r := newNAT66Reconciler(nat66ReconcilerParams{ - client: c, scheme: scheme, nodeName: testNAT66NodeA, - addr: testNAT66ShardAddr, sid: testNAT66ShardSIDVal, datapath: &fakeDatapathHealth{attached: true}, - }) + c := newIndexedClientBuilder(scheme).WithObjects(newTestNAT66Router(), staleShardAdvertisement()).Build() + datapath := &fakeEgressDatapath{attached: true, identity: &EgressShardIdentity{}} + r := newNAT66Reconciler(c, scheme, datapath) if _, err := r.Reconcile(context.Background(), reconcileReq(testNAT66ShardName)); err != nil { t.Fatalf("Reconcile() error = %v, want nil for a NotFound shard object", err) } - - got := &bgpv1alpha1.BGPAdvertisement{} - if err := c.Get(context.Background(), client.ObjectKeyFromObject(adv), got); err == nil { - t.Fatalf("BGPAdvertisement %v still exists after its EgressShard object disappeared", client.ObjectKeyFromObject(adv)) + assertNoAdvertisement(t, c) + if _, ok := datapath.Programmed(); ok { + t.Error("datapath still programmed after its EgressShard object disappeared") } } diff --git a/internal/controller/status.go b/internal/controller/status.go index 3179ff31..12eee085 100644 --- a/internal/controller/status.go +++ b/internal/controller/status.go @@ -132,8 +132,9 @@ func setRuleCondition(rule *bgpv1alpha1.NetworkRule, condition metav1.Condition) meta.SetStatusCondition(&rule.Status.Conditions, condition) } -// setEgressShardCondition sets or updates a condition on an EgressShard. Ready is -// the only type it currently uses. +// setEgressShardCondition sets or updates a condition on an EgressShard. It uses +// two types: Ready, from the datapath's attachment, and Programmed, from whether +// the datapath translates with the identity the spec assigns. func setEgressShardCondition(shard *bgpv1alpha1.EgressShard, condition metav1.Condition) { condition.ObservedGeneration = shard.Generation meta.SetStatusCondition(&shard.Status.Conditions, condition) diff --git a/internal/plumbing/bond/bond.go b/internal/plumbing/bond/bond.go index ad3838a4..b0201008 100644 --- a/internal/plumbing/bond/bond.go +++ b/internal/plumbing/bond/bond.go @@ -16,7 +16,11 @@ // test-only indirection. package bond -import "github.com/vishvananda/netlink" +import ( + "fmt" + + "github.com/vishvananda/netlink" +) // LinkType is the vishvananda/netlink Link.Type() value reported for a // Linux bonding master. @@ -40,3 +44,23 @@ func SlaveNames(master netlink.Link, links []netlink.Link) []string { } return slaves } + +// XDPTargets returns the interface names a native XDP program should attach to +// in place of link: link itself when it is not a bonding master, and its slaves +// in links, never the master, when it is. See the package doc comment for why +// the master is excluded. +// +// A bonding master with no slaves is an error. Attaching nothing leaves the +// datapath with no ingress on that uplink, and attaching the master risks the +// failure excluding it avoids. +func XDPTargets(link netlink.Link, links []netlink.Link) ([]string, error) { + if !IsMaster(link) { + return []string{link.Attrs().Name}, nil + } + slaves := SlaveNames(link, links) + if len(slaves) == 0 { + return nil, fmt.Errorf( + "bonding master %q has no slave interfaces to attach the XDP program to", link.Attrs().Name) + } + return slaves, nil +} diff --git a/internal/plumbing/bond/bond_test.go b/internal/plumbing/bond/bond_test.go index 685cb0e8..1f0f3b36 100644 --- a/internal/plumbing/bond/bond_test.go +++ b/internal/plumbing/bond/bond_test.go @@ -25,11 +25,14 @@ func (f *fakeLink) Type() string { return f.linkType } -const testBondName = "bond0" +const ( + testBondName = "bond0" + testNonBondName = "eth0" +) func TestIsMaster(t *testing.T) { bondLink := &fakeLink{attrs: netlink.LinkAttrs{Name: testBondName}, linkType: LinkType} - nonBondLink := &fakeLink{attrs: netlink.LinkAttrs{Name: "eth0"}} + nonBondLink := &fakeLink{attrs: netlink.LinkAttrs{Name: testNonBondName}} if !IsMaster(bondLink) { t.Error("IsMaster(bond link) = false, want true") @@ -71,3 +74,42 @@ func TestSlaveNames_NoSlaves(t *testing.T) { t.Errorf("SlaveNames() = %v, want nil", got) } } + +func TestXDPTargets(t *testing.T) { + bondLink := &fakeLink{attrs: netlink.LinkAttrs{Name: testBondName, Index: 10}, linkType: LinkType} + nonBondLink := &fakeLink{attrs: netlink.LinkAttrs{Name: testNonBondName, Index: 2}} + links := []netlink.Link{ + bondLink, + nonBondLink, + &fakeLink{attrs: netlink.LinkAttrs{Name: "eth1", Index: 3, MasterIndex: 10}}, + &fakeLink{attrs: netlink.LinkAttrs{Name: "eth2", Index: 4, MasterIndex: 10}}, + } + + tests := []struct { + name string + link netlink.Link + links []netlink.Link + want []string + wantErr bool + }{ + {name: "non-bond resolves to itself", link: nonBondLink, links: links, want: []string{testNonBondName}}, + {name: "bond resolves to its slaves only", link: bondLink, links: links, want: []string{"eth1", "eth2"}}, + {name: "bond with no slaves is an error", link: bondLink, links: links[:2], wantErr: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := XDPTargets(tt.link, tt.links) + if (err != nil) != tt.wantErr { + t.Fatalf("XDPTargets() error = %v, wantErr %v", err, tt.wantErr) + } + if len(got) != len(tt.want) { + t.Fatalf("XDPTargets() = %v, want %v", got, tt.want) + } + for i := range got { + if got[i] != tt.want[i] { + t.Fatalf("XDPTargets() = %v, want %v", got, tt.want) + } + } + }) + } +} diff --git a/internal/plumbing/ebpf/attach/interfaces.go b/internal/plumbing/ebpf/attach/interfaces.go index 4f26b568..a32f8c29 100644 --- a/internal/plumbing/ebpf/attach/interfaces.go +++ b/internal/plumbing/ebpf/attach/interfaces.go @@ -5,6 +5,7 @@ package attach import ( + "errors" "fmt" "log/slog" "net" @@ -73,12 +74,22 @@ func ResolveInterfaces() ([]string, error) { } else { names, err = autoDetectInterfaces() if err != nil { - return nil, err + return nil, fmt.Errorf("%w; set %s to override", err, config.EnvCNIEBPFInterfaces) } } return expandBondSlaves(names) } +// DetectUplinks returns the interfaces carrying the IPv6 default route or a +// BGP-learned route, in the order and with the exclusions ResolveInterfaces' +// auto-detection applies. Unlike ResolveInterfaces it reads no override and +// expands no bond, so a caller attaching some other hook applies its own: the +// egress translation datapath attaches native XDP, which must land on a bond's +// slaves and never on its master, where the TC-BPF path attaches to both. +func DetectUplinks() ([]string, error) { + return autoDetectInterfaces() +} + // parseInterfaceList splits a comma-separated interface list, trimming // whitespace and removing duplicate/empty entries while preserving order. func parseInterfaceList(v string) []string { @@ -163,9 +174,9 @@ func autoDetectInterfaces() ([]string, error) { collect(isFabricPeerRoute) if len(names) == 0 { - return nil, fmt.Errorf( - "attach: no default or BGP-learned IPv6 route found to auto-detect the "+ - "SRv6/underlay-facing interface; set %s to override", config.EnvCNIEBPFInterfaces) + return nil, errors.New( + "attach: no default or BGP-learned IPv6 route found to auto-detect the " + + "SRv6/underlay-facing interface") } return names, nil } diff --git a/internal/plumbing/ebpf/edgeattach/attach.go b/internal/plumbing/ebpf/edgeattach/attach.go index 7794d8e0..1d328223 100644 --- a/internal/plumbing/ebpf/edgeattach/attach.go +++ b/internal/plumbing/ebpf/edgeattach/attach.go @@ -139,12 +139,11 @@ func ResolveTargets(ifaceName string) ([]string, error) { if err != nil { return nil, fmt.Errorf("edgeattach: enumerate slaves of bonding master %q: %w", ifaceName, err) } - slaves := bond.SlaveNames(iface, links) - if len(slaves) == 0 { - return nil, fmt.Errorf( - "edgeattach: bonding master %q has no slave interfaces to attach the XDP program to", ifaceName) + targets, err := bond.XDPTargets(iface, links) + if err != nil { + return nil, fmt.Errorf("edgeattach: %w", err) } - return slaves, nil + return targets, nil } // Attach attaches program to the XDP hook of every interface in ifaceNames, in diff --git a/internal/plumbing/ebpf/natattach/attach_test.go b/internal/plumbing/ebpf/natattach/attach_test.go index 87007f10..221bdd32 100644 --- a/internal/plumbing/ebpf/natattach/attach_test.go +++ b/internal/plumbing/ebpf/natattach/attach_test.go @@ -31,7 +31,7 @@ func requireRoot(t *testing.T) { // TestAttach_NilProgramIsError covers Attach's defensive nil-program // guard directly, without needing root. func TestAttach_NilProgramIsError(t *testing.T) { - if _, err := Attach(nil, []string{"eth0"}); err == nil { + if _, err := Attach(nil, []string{testUplink}); err == nil { t.Error("Attach(nil program, ...) error = nil, want an error") } } diff --git a/internal/plumbing/ebpf/natattach/doc.go b/internal/plumbing/ebpf/natattach/doc.go index 4992a26f..cd67ec35 100644 --- a/internal/plumbing/ebpf/natattach/doc.go +++ b/internal/plumbing/ebpf/natattach/doc.go @@ -21,12 +21,12 @@ // // # No re-attachment // -// Every uplink in the operator-configured list is attached at process startup, -// so a multi-homed shard node translates on all of them and losing one uplink -// does not stop translation on the rest. A single uplink is simply a -// one-element list, and one naming a bonding master is expanded to that bond's -// slaves by ResolveTargets, native XDP against a bonding master being -// unreliable. +// Every uplink ResolveUplinks returns -- the operator's override, or the +// auto-detected set -- is attached at process startup, so a multi-homed shard +// node translates on all of them and losing one uplink does not stop +// translation on the rest. A single uplink is simply a one-element list, and +// one naming a bonding master is expanded to that bond's slaves by +// ResolveTargets, native XDP against a bonding master being unreliable. // // Unlike the edge attach package, slaves are attached back to back, without // waiting for each to rejoin its aggregate first. On a NIC whose driver drops diff --git a/internal/plumbing/ebpf/natattach/uplinks.go b/internal/plumbing/ebpf/natattach/uplinks.go new file mode 100644 index 00000000..bd4a9e22 --- /dev/null +++ b/internal/plumbing/ebpf/natattach/uplinks.go @@ -0,0 +1,54 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package natattach + +import ( + "errors" + "fmt" + + "go.datum.net/galactic/internal/plumbing/ebpf/attach" +) + +// detectUplinksFn is an override point so ResolveUplinks' tests can substitute +// a fake route view without touching the host network stack. The netlink +// override points ResolveTargets uses (linkByNameFn, linkListFn) live in +// attach.go. +var detectUplinksFn = attach.DetectUplinks + +// ResolveUplinks returns the interfaces Attach should attach the egress +// translation program to. +// +// A non-empty override is used as given. Otherwise the uplinks are +// auto-detected with the same derivation the CNI's SRv6 datapath uses +// (attach.DetectUplinks), so the shard and the CNI on one node converge on the +// same physical uplinks with no per-node configuration. The override exists for +// a multi-homed node where that derivation cannot be confident. +// +// Either way, every name then passes through ResolveTargets: a bonding master +// resolves to its slaves and never to itself, native XDP on a master being +// unreliable. +// +// An empty result is an error. The datapath claims a packet only on an +// interface it is attached to, so too few uplinks is a silent blackhole rather +// than a degraded mode. +func ResolveUplinks(override []string) ([]string, error) { + names := override + if len(names) == 0 { + detected, err := detectUplinksFn() + if err != nil { + return nil, fmt.Errorf("natattach: auto-detect uplinks: %w", err) + } + names = detected + } + + targets, err := ResolveTargets(names) + if err != nil { + return nil, err + } + if len(targets) == 0 { + return nil, errors.New("natattach: no uplink interfaces resolved") + } + return targets, nil +} diff --git a/internal/plumbing/ebpf/natattach/uplinks_test.go b/internal/plumbing/ebpf/natattach/uplinks_test.go new file mode 100644 index 00000000..2a215a60 --- /dev/null +++ b/internal/plumbing/ebpf/natattach/uplinks_test.go @@ -0,0 +1,90 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package natattach + +import ( + "errors" + "slices" + "testing" + + "github.com/vishvananda/netlink" + + "go.datum.net/galactic/internal/plumbing/bond" +) + +const ( + testUplink = "eth0" + testBond = "bond0" + testBondSlave1 = "eth1" + testBondSlave2 = "eth2" +) + +// fakeUplinkHost installs, using attach_test.go's fakeLink fixture, a netlink view holding eth0, and bond0 with slaves +// eth1 and eth2, with detection reporting detected. +func fakeUplinkHost(t *testing.T, detected []string, detectErr error) { + t.Helper() + origDetect, origByName, origList := detectUplinksFn, linkByNameFn, linkListFn + t.Cleanup(func() { detectUplinksFn, linkByNameFn, linkListFn = origDetect, origByName, origList }) + + links := []netlink.Link{ + &fakeLink{attrs: netlink.LinkAttrs{Name: testUplink, Index: 2}}, + &fakeLink{attrs: netlink.LinkAttrs{Name: testBond, Index: 10}, linkType: bond.LinkType}, + &fakeLink{attrs: netlink.LinkAttrs{Name: testBondSlave1, Index: 3, MasterIndex: 10}}, + &fakeLink{attrs: netlink.LinkAttrs{Name: testBondSlave2, Index: 4, MasterIndex: 10}}, + } + detectUplinksFn = func() ([]string, error) { return detected, detectErr } + linkByNameFn = func(name string) (netlink.Link, error) { + for _, l := range links { + if l.Attrs().Name == name { + return l, nil + } + } + return nil, errors.New("link not found") + } + linkListFn = func() ([]netlink.Link, error) { return links, nil } +} + +func TestResolveUplinks(t *testing.T) { + tests := []struct { + name string + override []string + detected []string + detectErr error + want []string + wantErr bool + }{ + {name: "auto-detected plain uplink", detected: []string{testUplink}, want: []string{testUplink}}, + { + name: "auto-detected bond resolves to slaves, never the master", + detected: []string{testUplink, testBond}, + want: []string{testUplink, testBondSlave1, testBondSlave2}, + }, + { + name: "override wins over detection", + override: []string{testBond}, + detected: []string{testUplink}, + want: []string{testBondSlave1, testBondSlave2}, + }, + {name: "override skips detection entirely", override: []string{testUplink}, detectErr: errors.New("boom"), + want: []string{testUplink}}, + {name: "a slave named alongside its bond is not attached twice", override: []string{testBond, testBondSlave1}, + want: []string{testBondSlave1, testBondSlave2}}, + {name: "detection failure is an error", detectErr: errors.New("no routes"), wantErr: true}, + {name: "detection finding nothing is an error", detected: []string{}, wantErr: true}, + {name: "an unknown interface is an error", override: []string{"eth9"}, wantErr: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fakeUplinkHost(t, tt.detected, tt.detectErr) + got, err := ResolveUplinks(tt.override) + if (err != nil) != tt.wantErr { + t.Fatalf("ResolveUplinks(%v) error = %v, wantErr %v", tt.override, err, tt.wantErr) + } + if !slices.Equal(got, tt.want) { + t.Errorf("ResolveUplinks(%v) = %v, want %v", tt.override, got, tt.want) + } + }) + } +} diff --git a/internal/plumbing/ebpf/natmap/shardconfig.go b/internal/plumbing/ebpf/natmap/shardconfig.go index 071f60eb..f787d876 100644 --- a/internal/plumbing/ebpf/natmap/shardconfig.go +++ b/internal/plumbing/ebpf/natmap/shardconfig.go @@ -157,8 +157,21 @@ func (c ShardConfig) toWire() (natprog.NatShardConfig, error) { return value, nil } -// Get reads the single entry and reports whether it has been written yet. Until -// it has, the datapath fails open and claims no packets. +// Clear overwrites the single entry with the all-zero row, which serves no +// family and so is what the datapath reads as unconfigured: it fails open and +// claims no packets. An array map has no entry to delete, so this is how a +// shard that no longer has an identity stops translating. +func (t *ShardConfigTable) Clear() error { + if err := t.table.Put(shardConfigKey, natprog.NatShardConfig{}); err != nil { + return fmt.Errorf("natmap: shard_config_table: clear: %w", err) + } + return nil +} + +// Get reads the single entry and reports whether it holds a configuration. +// Until one is written, and after Clear, the datapath fails open and claims no +// packets. A row serving no family counts as unconfigured: the kernel's array +// map always returns a row, zeroed until first written. func (t *ShardConfigTable) Get() (ShardConfig, bool, error) { var value natprog.NatShardConfig if err := t.table.Lookup(shardConfigKey, &value); err != nil { @@ -167,6 +180,9 @@ func (t *ShardConfigTable) Get() (ShardConfig, bool, error) { } return ShardConfig{}, false, fmt.Errorf("natmap: shard_config_table: get: %w", err) } + if value.ServesV6 == 0 && value.ServesV4 == 0 { + return ShardConfig{}, false, nil + } cfg := ShardConfig{ ShardSID: netip.AddrFrom16(value.ShardSid), } diff --git a/internal/plumbing/ebpf/natmap/shardconfig_test.go b/internal/plumbing/ebpf/natmap/shardconfig_test.go index 9fc6bfe9..4d0421d4 100644 --- a/internal/plumbing/ebpf/natmap/shardconfig_test.go +++ b/internal/plumbing/ebpf/natmap/shardconfig_test.go @@ -45,6 +45,31 @@ func TestShardConfigTable_GetBeforeSet(t *testing.T) { } } +// TestShardConfigTable_ClearReadsAsUnconfigured covers the zero row Clear +// writes, which is also what a real array map holds before its first write: +// Get must report it as unconfigured, not as a shard with an all-zero SID. +func TestShardConfigTable_ClearReadsAsUnconfigured(t *testing.T) { + table := NewShardConfigTable(newFakeTable()) + + if err := table.Set(ShardConfig{ + ShardSID: netip.MustParseAddr("fc00:1:2::1"), + ShardPubAddr6: netip.MustParseAddr("2001:db8:9999::1"), + }); err != nil { + t.Fatalf("Set() error = %v", err) + } + if err := table.Clear(); err != nil { + t.Fatalf("Clear() error = %v", err) + } + + got, ok, err := table.Get() + if err != nil { + t.Fatalf("Get() error = %v", err) + } + if ok { + t.Errorf("Get() ok = true after Clear, want false; got %+v", got) + } +} + func TestShardConfigTable_SetOverwrites(t *testing.T) { table := NewShardConfigTable(newFakeTable()) diff --git a/internal/plumbing/ebpf/natprog/nat.c b/internal/plumbing/ebpf/natprog/nat.c index bd3996fd..8668e42c 100644 --- a/internal/plumbing/ebpf/natprog/nat.c +++ b/internal/plumbing/ebpf/natprog/nat.c @@ -75,8 +75,9 @@ // // Dispatch, on the outer header: // -// 1. Not IPv6 and not IPv4: XDP_PASS. Not configured yet: XDP_PASS, this -// shard claiming nothing until it knows its own identity. +// 1. Not IPv6 and not IPv4: XDP_PASS. Not configured yet, serving neither +// family: XDP_PASS, this shard claiming nothing until it knows its own +// identity. // 2. IPv6 destination equal to shard_pub_addr6 is a reply from the IPv6 // internet addressed to a masquerade source this shard allocated -> // nat66_return. @@ -1407,6 +1408,12 @@ int nat_ingress(struct xdp_md *ctx) __u32 cfg_key = 0; struct shard_config *cfg = bpf_map_lookup_elem(&shard_config_table, &cfg_key); if (!cfg) + return XDP_PASS; + // An array map's lookup never misses, so an unconfigured shard reads its + // all-zero row rather than NULL. Serving no family is what "not configured" + // looks like, and it has to be tested here: a zero shard_sid still matches + // every destination whose top 64 bits are zero. + if (!cfg->serves_v6 && !cfg->serves_v4) return XDP_PASS; // not yet configured -- fail open, not claimed if (eth->h_proto == __builtin_bswap16(NAT_ETH_P_IPV6)) { diff --git a/internal/plumbing/ebpf/natprog/nat_bpfeb.o b/internal/plumbing/ebpf/natprog/nat_bpfeb.o index d66f4702..f2d8373c 100644 Binary files a/internal/plumbing/ebpf/natprog/nat_bpfeb.o and b/internal/plumbing/ebpf/natprog/nat_bpfeb.o differ diff --git a/internal/plumbing/ebpf/natprog/nat_bpfel.o b/internal/plumbing/ebpf/natprog/nat_bpfel.o index 8b117838..f1007b20 100644 Binary files a/internal/plumbing/ebpf/natprog/nat_bpfel.o and b/internal/plumbing/ebpf/natprog/nat_bpfel.o differ diff --git a/internal/plumbing/ebpf/natprog/nat_test.go b/internal/plumbing/ebpf/natprog/nat_test.go index 1b49be5f..0e712c24 100644 --- a/internal/plumbing/ebpf/natprog/nat_test.go +++ b/internal/plumbing/ebpf/natprog/nat_test.go @@ -265,6 +265,30 @@ func TestNatIngress_UnclaimedTrafficPassesThrough(t *testing.T) { } } +// TestNatIngress_UnconfiguredClaimsNothing covers the all-zero row an array +// map holds before the shard is programmed, or after it is cleared. Its zero +// shard_sid matches any outer destination whose top 64 bits are zero, so the +// dispatcher must reject the row for serving no family before comparing it. +func TestNatIngress_UnconfiguredClaimsNothing(t *testing.T) { + requireRoot(t) + objs := loadObjects(t) + populateProgArray(t, objs) + + pkt := buildEncappedUDPPacket(t, netip.MustParseAddr("::1:0:0:1"), + netip.MustParseAddr("fc00:9::1"), netip.MustParseAddr("fd00::10"), + netip.MustParseAddr("2001:db8::9999"), []byte("hi")) + ret, out, err := objs.NatIngress.Test(pkt) + if err != nil { + t.Fatalf("program test-run: %v", err) + } + if ret != xdpPass { + t.Errorf("verdict = %d, want XDP_PASS (%d) from an unconfigured shard", ret, xdpPass) + } + if string(out) != string(pkt) { + t.Errorf("unconfigured shard mutated a packet:\n in: % x\nout: % x", pkt, out) + } +} + // TestNatIngress_ForwardSNATsAndPreservesChecksum covers handle_forward: // a tenant's own SRv6-encapsulated egress packet must be decapsulated, // SNAT'd to shard_pub_addr6 with an allocated port, and passed through