diff --git a/deploy/containerlab/resources/tenants/ns10/dfw/nad.yaml b/deploy/containerlab/resources/tenants/ns10/dfw/nad.yaml index 8a350fa1..eff1b109 100644 --- a/deploy/containerlab/resources/tenants/ns10/dfw/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns10/dfw/nad.yaml @@ -25,7 +25,8 @@ spec: "type": "galactic-bgp", "vpc": "10", "vpcattachment": "10", - "namespace": "galactic-system" + "namespace": "galactic-system", + "egress": { "internet": { "mode": "Enabled" } } } ] } diff --git a/deploy/containerlab/resources/tenants/ns10/iad/nad.yaml b/deploy/containerlab/resources/tenants/ns10/iad/nad.yaml index 6544d533..465a6e29 100644 --- a/deploy/containerlab/resources/tenants/ns10/iad/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns10/iad/nad.yaml @@ -25,7 +25,8 @@ spec: "type": "galactic-bgp", "vpc": "10", "vpcattachment": "10", - "namespace": "galactic-system" + "namespace": "galactic-system", + "egress": { "internet": { "mode": "Enabled" } } } ] } diff --git a/deploy/containerlab/resources/tenants/ns10/sjc/nad.yaml b/deploy/containerlab/resources/tenants/ns10/sjc/nad.yaml index a2055ec6..31092e0f 100644 --- a/deploy/containerlab/resources/tenants/ns10/sjc/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns10/sjc/nad.yaml @@ -25,7 +25,8 @@ spec: "type": "galactic-bgp", "vpc": "10", "vpcattachment": "10", - "namespace": "galactic-system" + "namespace": "galactic-system", + "egress": { "internet": { "mode": "Enabled" } } } ] } diff --git a/deploy/containerlab/resources/tenants/ns20/dfw/nad.yaml b/deploy/containerlab/resources/tenants/ns20/dfw/nad.yaml index bcd4108b..5d721200 100644 --- a/deploy/containerlab/resources/tenants/ns20/dfw/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns20/dfw/nad.yaml @@ -26,7 +26,8 @@ spec: "type": "galactic-bgp", "vpc": "20", "vpcattachment": "20", - "namespace": "galactic-system" + "namespace": "galactic-system", + "egress": { "internet": { "mode": "Enabled" } } } ] } diff --git a/deploy/containerlab/resources/tenants/ns20/iad/nad.yaml b/deploy/containerlab/resources/tenants/ns20/iad/nad.yaml index 9412d755..af745ce3 100644 --- a/deploy/containerlab/resources/tenants/ns20/iad/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns20/iad/nad.yaml @@ -26,7 +26,8 @@ spec: "type": "galactic-bgp", "vpc": "20", "vpcattachment": "20", - "namespace": "galactic-system" + "namespace": "galactic-system", + "egress": { "internet": { "mode": "Enabled" } } } ] } diff --git a/deploy/containerlab/resources/tenants/ns20/sjc/nad.yaml b/deploy/containerlab/resources/tenants/ns20/sjc/nad.yaml index 2062071f..a64ce2e5 100644 --- a/deploy/containerlab/resources/tenants/ns20/sjc/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns20/sjc/nad.yaml @@ -26,7 +26,8 @@ spec: "type": "galactic-bgp", "vpc": "20", "vpcattachment": "20", - "namespace": "galactic-system" + "namespace": "galactic-system", + "egress": { "internet": { "mode": "Enabled" } } } ] } diff --git a/deploy/containerlab/resources/tenants/ns30/dfw/nad-b.yaml b/deploy/containerlab/resources/tenants/ns30/dfw/nad-b.yaml index b9dbeb47..f9f98caa 100644 --- a/deploy/containerlab/resources/tenants/ns30/dfw/nad-b.yaml +++ b/deploy/containerlab/resources/tenants/ns30/dfw/nad-b.yaml @@ -25,7 +25,8 @@ spec: "type": "galactic-bgp", "vpc": "30", "vpcattachment": "31", - "namespace": "galactic-system" + "namespace": "galactic-system", + "egress": { "internet": { "mode": "Enabled" } } } ] } diff --git a/deploy/containerlab/resources/tenants/ns30/dfw/nad.yaml b/deploy/containerlab/resources/tenants/ns30/dfw/nad.yaml index acc1bf88..2a6e12b4 100644 --- a/deploy/containerlab/resources/tenants/ns30/dfw/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns30/dfw/nad.yaml @@ -25,7 +25,8 @@ spec: "type": "galactic-bgp", "vpc": "30", "vpcattachment": "30", - "namespace": "galactic-system" + "namespace": "galactic-system", + "egress": { "internet": { "mode": "Enabled" } } } ] } diff --git a/deploy/containerlab/resources/tenants/ns40/iad/nad-b.yaml b/deploy/containerlab/resources/tenants/ns40/iad/nad-b.yaml index ffb2c90e..8fdc0355 100644 --- a/deploy/containerlab/resources/tenants/ns40/iad/nad-b.yaml +++ b/deploy/containerlab/resources/tenants/ns40/iad/nad-b.yaml @@ -25,7 +25,8 @@ spec: "type": "galactic-bgp", "vpc": "40", "vpcattachment": "41", - "namespace": "galactic-system" + "namespace": "galactic-system", + "egress": { "internet": { "mode": "Enabled" } } } ] } diff --git a/deploy/containerlab/resources/tenants/ns40/iad/nad.yaml b/deploy/containerlab/resources/tenants/ns40/iad/nad.yaml index 11e74f64..fd57fdd6 100644 --- a/deploy/containerlab/resources/tenants/ns40/iad/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns40/iad/nad.yaml @@ -25,7 +25,8 @@ spec: "type": "galactic-bgp", "vpc": "40", "vpcattachment": "40", - "namespace": "galactic-system" + "namespace": "galactic-system", + "egress": { "internet": { "mode": "Enabled" } } } ] } diff --git a/deploy/containerlab/resources/tenants/ns60/dfw/nad.yaml b/deploy/containerlab/resources/tenants/ns60/dfw/nad.yaml index ec246305..5e1caf57 100644 --- a/deploy/containerlab/resources/tenants/ns60/dfw/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns60/dfw/nad.yaml @@ -25,7 +25,8 @@ spec: "type": "galactic-bgp", "vpc": "60", "vpcattachment": "60", - "namespace": "galactic-system" + "namespace": "galactic-system", + "egress": { "internet": { "mode": "Enabled" } } } ] } diff --git a/deploy/containerlab/resources/tenants/ns60/iad/nad.yaml b/deploy/containerlab/resources/tenants/ns60/iad/nad.yaml index ce088bbf..b0317e1d 100644 --- a/deploy/containerlab/resources/tenants/ns60/iad/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns60/iad/nad.yaml @@ -25,7 +25,8 @@ spec: "type": "galactic-bgp", "vpc": "60", "vpcattachment": "60", - "namespace": "galactic-system" + "namespace": "galactic-system", + "egress": { "internet": { "mode": "Enabled" } } } ] } diff --git a/deploy/containerlab/resources/tenants/ns60/sjc/nad.yaml b/deploy/containerlab/resources/tenants/ns60/sjc/nad.yaml index 5501bc3d..140b9890 100644 --- a/deploy/containerlab/resources/tenants/ns60/sjc/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns60/sjc/nad.yaml @@ -25,7 +25,8 @@ spec: "type": "galactic-bgp", "vpc": "60", "vpcattachment": "60", - "namespace": "galactic-system" + "namespace": "galactic-system", + "egress": { "internet": { "mode": "Enabled" } } } ] } diff --git a/deploy/containerlab/resources/tenants/ns70/iad/nad.yaml b/deploy/containerlab/resources/tenants/ns70/iad/nad.yaml index 69aa4441..55d79509 100644 --- a/deploy/containerlab/resources/tenants/ns70/iad/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns70/iad/nad.yaml @@ -24,7 +24,8 @@ spec: "type": "galactic-bgp", "vpc": "70", "vpcattachment": "70", - "namespace": "galactic-system" + "namespace": "galactic-system", + "egress": { "internet": { "mode": "Enabled" } } } ] } diff --git a/deploy/containerlab/resources/tenants/ns70/sjc/nad.yaml b/deploy/containerlab/resources/tenants/ns70/sjc/nad.yaml index 69aa4441..55d79509 100644 --- a/deploy/containerlab/resources/tenants/ns70/sjc/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns70/sjc/nad.yaml @@ -24,7 +24,8 @@ spec: "type": "galactic-bgp", "vpc": "70", "vpcattachment": "70", - "namespace": "galactic-system" + "namespace": "galactic-system", + "egress": { "internet": { "mode": "Enabled" } } } ] } diff --git a/deploy/containerlab/resources/tenants/ns71/sjc/nad-b.yaml b/deploy/containerlab/resources/tenants/ns71/sjc/nad-b.yaml index b859a46f..a605e730 100644 --- a/deploy/containerlab/resources/tenants/ns71/sjc/nad-b.yaml +++ b/deploy/containerlab/resources/tenants/ns71/sjc/nad-b.yaml @@ -29,7 +29,8 @@ spec: "type": "galactic-bgp", "vpc": "72", "vpcattachment": "72", - "namespace": "galactic-system" + "namespace": "galactic-system", + "egress": { "internet": { "mode": "Enabled" } } } ] } diff --git a/deploy/containerlab/resources/tenants/ns71/sjc/nad.yaml b/deploy/containerlab/resources/tenants/ns71/sjc/nad.yaml index bf75bbcd..51c9c182 100644 --- a/deploy/containerlab/resources/tenants/ns71/sjc/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns71/sjc/nad.yaml @@ -29,7 +29,8 @@ spec: "type": "galactic-bgp", "vpc": "71", "vpcattachment": "71", - "namespace": "galactic-system" + "namespace": "galactic-system", + "egress": { "internet": { "mode": "Enabled" } } } ] } diff --git a/docs/cni/conflist-reference.md b/docs/cni/conflist-reference.md index b4cffcb8..64a324dc 100644 --- a/docs/cni/conflist-reference.md +++ b/docs/cni/conflist-reference.md @@ -244,12 +244,28 @@ on the same attachment, so cleanup is left to `galactic-router`'s GC controller. ## BGP Publish Fields (`galactic-bgp`) -`galactic-bgp`'s own stanza carries only `vpc`, `vpcattachment`, and -`namespace` — nothing else. It learns which interface kind was created and -what addresses were allocated entirely from `prevResult` (the accumulated +`galactic-bgp`'s own stanza carries `vpc`, `vpcattachment`, `namespace`, and +an optional `egress` declaration. It learns which interface kind was created +and what addresses were allocated entirely from `prevResult` (the accumulated result of every preceding plugin in the chain), never from its own config or a kernel call. +### Egress declaration + +```json +"egress": { "internet": { "mode": "Enabled" } } +``` + +The declaration says whether this network reaches the internet. It names no +shard: the node routes toward the shard its own configuration names (see +[`GALACTIC_CNI_EGRESS_SHARD_SIDS`](environment-variables.md#galactic_cni_egress_shard_sids)), +so the declaration decides only whether this VRF gets that route. + +`Enabled` installs the VRF's egress routes, and fails the ADD if the node +names no shard or none of its shards resolves. Anything else, including an +absent `egress` key, installs no route and withdraws one that is there, so a +network that declared no egress gets none even on a node that has shards. + ### EndpointSlice publish (HTTP ingress backend discovery) Alongside the `BGPVRFInstance`/`BGPAdvertisement` CRDs, `galactic-bgp` diff --git a/docs/cni/environment-variables.md b/docs/cni/environment-variables.md index ea94ef18..e8318613 100644 --- a/docs/cni/environment-variables.md +++ b/docs/cni/environment-variables.md @@ -139,6 +139,12 @@ field; as of this writing it is still env-only. ## `GALACTIC_CNI_EGRESS_SHARD_SIDS` +> **This list no longer decides which networks get egress.** It names the +> shards a node may route toward. Whether a given VRF gets that route is the +> `egress` declaration in the attachment's own `galactic-bgp` stanza (see the +> [conflist reference](conflist-reference.md#egress-declaration)), so a node +> holding this list installs a route only for a network that declared one. + Comma-separated list of every live `galactic-nat` shard's `Status.ShardSID` — the fabric-wide membership list `internal/plumbing/srv6.EgressDefaultRouteAdd` needs to install a tenant diff --git a/docs/nat/configuration.md b/docs/nat/configuration.md index 6c376987..3815a540 100644 --- a/docs/nat/configuration.md +++ b/docs/nat/configuration.md @@ -212,6 +212,12 @@ to an earlier, superseded design this sharded egress tier replaced. ## Shard membership (`galactic-cni` side) +The list below is where a node may route toward. Whether a given network gets +that route is the `egress` declaration in the attachment's own `galactic-bgp` +conflist stanza, rendered from what the network declared; a network that +declares none gets no route however the node is configured. See the +[conflist reference](../cni/conflist-reference.md#egress-declaration). + A tenant's compute node needs to know the fabric-wide list of live shard SIDs to install its own tenant VRFs' egress routes, and the NAT64 prefix to install a route toward IPv4 reachability. Both are separate from anything diff --git a/internal/cnibgp/bgp.go b/internal/cnibgp/bgp.go index c8514a79..2bfc017e 100644 --- a/internal/cnibgp/bgp.go +++ b/internal/cnibgp/bgp.go @@ -76,6 +76,7 @@ type publishConfig struct { // ifaceType selects the attachment's egress kind, veth or tap. Inferred from // prevResult, never a config field. ifaceType string + egress *Egress } // publishResult records what publishBGPState created, so cmdAdd can fold it @@ -447,7 +448,7 @@ func publishBGPState( // Not tracked for rollback: the vrf_table entry is shared by every // attachment on this VPC and node, like the BGPVRFInstance above. if _, err := registerEBPFDatapath( - bgp, cfg.vpc, cfg.vpcAttachment, cfg.ifaceType, uint16(vrfID), ebpfPinDir, prefixes, + bgp, cfg.vpc, cfg.vpcAttachment, cfg.ifaceType, uint16(vrfID), ebpfPinDir, prefixes, cfg.egress, ); err != nil { return fmt.Errorf("register eBPF uSID datapath: %w", err) } @@ -522,6 +523,7 @@ func publishBGPState( // nothing. func registerEBPFDatapath( bgp bgpConfig, vpc, vpcAttachment, ifaceType string, argument uint16, pinDir string, prefixes []string, + egress *Egress, ) (registered bool, err error) { if bgp.srv6Locator == "" || bgp.nodeID == 0 { return false, nil @@ -551,12 +553,12 @@ func registerEBPFDatapath( return false, fmt.Errorf("look up VRF table id for eBPF registration: %w", err) } - // Installs or refreshes this VRF's NAT66 default egress route. The - // optional routing plugin in this chain may be absent from a given - // conflist, and this route must exist wherever a shard is configured, so - // it is written here. - if err := installEgressRoutes(vrfTableID, argument); err != nil { - return false, fmt.Errorf("install NAT66 default egress route: %w", err) + // Installs, refreshes, or withdraws this VRF's egress routes. The optional + // routing plugin in this chain may be absent from a given conflist, and + // these routes must exist wherever this network declares egress, so they + // are written here. + if err := installEgressRoutes(vrfTableID, argument, egress); err != nil { + return false, fmt.Errorf("install egress routes: %w", err) } if err := registerLocalEgressRoutes(pinDir, vrfTableID, prefixes); err != nil { @@ -761,11 +763,20 @@ func registerLocalEgressRoutes(pinDir string, vrfTableID uint32, prefixes []stri // may offer NAT64 without NAT66, and then no default route exists for this // traffic to fall into. // -// No shard configured is not an error: the shard list parses to an empty slice -// and srv6.EgressDefaultRouteAdd no-ops. A shard SID that is invalid, or that -// has no reachable route yet, fails this attachment's ADD rather than leaving -// the VRF with no egress at all. -func installEgressRoutes(vrfTableID uint32, argument uint16) error { +// egress is this network's own declaration, from its own conflist stanza. A +// network that declares no egress gets no route, and loses one it has, which is +// what makes a declaration of no egress mean anything: the node-wide list on +// its own handed a default route out to every network on the node. +// +// A network that declares egress on a node naming no shard fails this +// attachment's ADD. A node without a shard is an operator error, and failing +// the first instance surfaces it where an attach that succeeded without egress +// would hide it. A shard SID that is invalid, or that has no reachable route +// yet, fails the ADD for the same reason. +func installEgressRoutes(vrfTableID uint32, argument uint16, egress *Egress) error { + if !egress.Enabled() { + return withdrawEgressRoutes(vrfTableID) + } // cniConfig is nil until InitCNIConfig runs, which several unit tests // calling registerEBPFDatapath directly never do. Treated as "no shard // configured" rather than a panic. @@ -777,7 +788,8 @@ func installEgressRoutes(vrfTableID uint32, argument uint16) error { return fmt.Errorf("parse %s: %w", config.EnvCNIEgressShardSIDs, err) } if len(shardSIDs) == 0 { - return nil + return fmt.Errorf("this network declares internet egress and this node names no egress shard (%s is empty)", + config.EnvCNIEgressShardSIDs) } tenantSIDs, err := shardSIDsForTenant(shardSIDs, argument) if err != nil { @@ -897,3 +909,24 @@ func egressKindForInterfaceType(ifaceType string) (uint32, error) { return 0, fmt.Errorf("unknown interface type %q", ifaceType) } } + +// withdrawEgressRoutes removes vrfTableID's egress routes, so a network whose +// declaration withdrew egress, or never declared it, carries none however the +// node is configured. Idempotent, and a no-op on a node whose datapath has not +// loaded, so an attachment there never fails its ADD over a route it never had. +func withdrawEgressRoutes(vrfTableID uint32) error { + if err := srv6.EgressDefaultRouteWithdraw(vrfTableID); err != nil { + return fmt.Errorf("withdraw default egress route: %w", err) + } + if cniConfig == nil || cniConfig.NAT64Prefix == "" { + return nil + } + _, prefix, err := net.ParseCIDR(cniConfig.NAT64Prefix) + if err != nil { + return fmt.Errorf("parse %s %q: %w", config.EnvCNINAT64Prefix, cniConfig.NAT64Prefix, err) + } + if err := srv6.EgressPrefixRouteWithdraw(vrfTableID, prefix); err != nil { + return fmt.Errorf("withdraw NAT64 egress route: %w", err) + } + return nil +} diff --git a/internal/cnibgp/bgp_ebpf_test.go b/internal/cnibgp/bgp_ebpf_test.go index a3a138a3..3f8fd148 100644 --- a/internal/cnibgp/bgp_ebpf_test.go +++ b/internal/cnibgp/bgp_ebpf_test.go @@ -52,7 +52,7 @@ func blockFromLocator(locator string) (uint64, error) { func TestRegisterEBPFDatapath_NotConfiguredIsNoOp(t *testing.T) { cfg := bgpConfig{srv6Locator: "", nodeID: 0} registered, err := registerEBPFDatapath( - cfg, testVPC, testAttachment, ifaceTypeVeth, 42, "/sys/fs/bpf/galactic-does-not-exist", nil) + cfg, testVPC, testAttachment, ifaceTypeVeth, 42, "/sys/fs/bpf/galactic-does-not-exist", nil, enabledEgress()) if err != nil { t.Errorf("registerEBPFDatapath with unconfigured BGPRouter = %v, want nil (no-op)", err) } @@ -69,7 +69,7 @@ func TestRegisterEBPFDatapath_NotConfiguredIsNoOp(t *testing.T) { func TestRegisterEBPFDatapath_RejectsOutOfRangeNodeID(t *testing.T) { cfg := bgpConfig{srv6Locator: "2001:db8:1::/48", nodeID: 0x10001} // wraps to uint16(1) if narrowed unchecked registered, err := registerEBPFDatapath( - cfg, testVPC, testAttachment, ifaceTypeVeth, 42, "/sys/fs/bpf/galactic-does-not-exist", nil) + cfg, testVPC, testAttachment, ifaceTypeVeth, 42, "/sys/fs/bpf/galactic-does-not-exist", nil, enabledEgress()) if err == nil { t.Fatal("registerEBPFDatapath with nodeID=0x10001 = nil error, want an out-of-range rejection") } @@ -119,7 +119,8 @@ func TestRegisterEBPFDatapath_RegistersAllThreeTables(t *testing.T) { t.Cleanup(func() { _ = loaderObjs.Close() }) cfg := bgpConfig{srv6Locator: locator, nodeID: nodeID} - registered, err := registerEBPFDatapath(cfg, vpc, testAttachment, ifaceTypeVeth, uint16(vrfID), pinDir, nil) + registered, err := registerEBPFDatapath( + cfg, vpc, testAttachment, ifaceTypeVeth, uint16(vrfID), pinDir, nil, enabledEgress()) if err != nil { t.Fatalf("registerEBPFDatapath: %v", err) } @@ -268,14 +269,14 @@ func TestRegisterEBPFDatapath_SecondAttachmentSharesEntry(t *testing.T) { cfg := bgpConfig{srv6Locator: locator, nodeID: nodeID} if _, err := registerEBPFDatapath( - cfg, vpc, firstAttachment, ifaceTypeVeth, uint16(vrfID), pinDir, []string{firstPrefix}, + cfg, vpc, firstAttachment, ifaceTypeVeth, uint16(vrfID), pinDir, []string{firstPrefix}, enabledEgress(), ); err != nil { t.Fatalf("first attachment's registerEBPFDatapath: %v", err) } // A second attachment on the same VPC/node resolves the same Argument // (allocateArgument's idempotent lookup) and re-registers the same key. registered, err := registerEBPFDatapath( - cfg, vpc, secondAttachment, ifaceTypeVeth, uint16(vrfID), pinDir, []string{secondPrefix}, + cfg, vpc, secondAttachment, ifaceTypeVeth, uint16(vrfID), pinDir, []string{secondPrefix}, enabledEgress(), ) if err != nil { t.Fatalf("second attachment's registerEBPFDatapath: %v", err) @@ -384,7 +385,8 @@ func TestRegisterEBPFDatapath_MixedInterfaceTypesKeepOwnEgressKind(t *testing.T) cfg := bgpConfig{srv6Locator: locator, nodeID: nodeID} for _, a := range tt.order { - if _, err := registerEBPFDatapath(cfg, vpc, a.name, a.ifaceType, uint16(vrfID), pinDir, nil); err != nil { + if _, err := registerEBPFDatapath( + cfg, vpc, a.name, a.ifaceType, uint16(vrfID), pinDir, nil, enabledEgress()); err != nil { t.Fatalf("registerEBPFDatapath(%s): %v", a.name, err) } } diff --git a/internal/cnibgp/bgp_test.go b/internal/cnibgp/bgp_test.go index c5585421..0bdce3f9 100644 --- a/internal/cnibgp/bgp_test.go +++ b/internal/cnibgp/bgp_test.go @@ -1116,8 +1116,8 @@ func TestInstallNAT66EgressRoute_NilCNIConfigIsANoop(t *testing.T) { cniConfig = nil defer func() { cniConfig = original }() - if err := installEgressRoutes(1, 0x005); err != nil { - t.Errorf("installEgressRoutes(1, 0x005) = %v, want nil with cniConfig == nil", err) + if err := installEgressRoutes(1, 0x005, enabledEgress()); err != nil { + t.Errorf("installEgressRoutes(1, 0x005, enabled) = %v, want nil with cniConfig == nil", err) } } @@ -1240,3 +1240,67 @@ func TestShardSIDsForTenant_RejectsAMalformedSID(t *testing.T) { } } } + +// enabledEgress is the declaration a network that reaches the internet +// carries in its stanza. +func enabledEgress() *Egress { + return &Egress{Internet: &InternetEgress{Mode: InternetEgressEnabled}} +} + +// TestInstallEgressRoutes_NoDeclarationTouchesNoBPFFS pins the behaviour that +// makes a network with no egress safe on any node: it must behave exactly as +// the path it replaces did before egress existed, which touched no pinned map +// at all. These tests run with no loaded datapath, so a withdrawal that opened +// bpffs unconditionally would fail here, and would fail an ADD on every node +// whose datapath has not loaded yet. +func TestInstallEgressRoutes_NoDeclarationTouchesNoBPFFS(t *testing.T) { + original := cniConfig + cniConfig = &config.CNIConfig{EgressShardSIDs: "2001:db8:ff01:2001:e001::"} + defer func() { cniConfig = original }() + + for name, egress := range map[string]*Egress{ + "absent": nil, + "empty": {}, + "no mode": {Internet: &InternetEgress{}}, + "disabled": {Internet: &InternetEgress{Mode: "Disabled"}}, + } { + if err := installEgressRoutes(1, 0x005, egress); err != nil { + t.Errorf("installEgressRoutes(%s) = %v, want nil with no loaded datapath", name, err) + } + } +} + +// TestInstallEgressRoutes_EnabledWithNoShardFails is the other half of the +// declaration: a network that asked for egress on a node naming no shard is an +// operator error, and it fails the first attach rather than succeeding +// without egress. +func TestInstallEgressRoutes_EnabledWithNoShardFails(t *testing.T) { + original := cniConfig + cniConfig = &config.CNIConfig{} + defer func() { cniConfig = original }() + + err := installEgressRoutes(1, 0x005, enabledEgress()) + if err == nil { + t.Fatal("installEgressRoutes(enabled) = nil, want an error on a node naming no shard") + } + if !strings.Contains(err.Error(), config.EnvCNIEgressShardSIDs) { + t.Errorf("installEgressRoutes(enabled) error = %q, want it to name %s", err, config.EnvCNIEgressShardSIDs) + } +} + +// TestEgressEnabled pins which declarations a node acts on: exactly one. +func TestEgressEnabled(t *testing.T) { + for name, tt := range map[string]struct { + egress *Egress + want bool + }{ + "nil": {nil, false}, + "empty": {&Egress{}, false}, + "disabled": {&Egress{Internet: &InternetEgress{Mode: "Disabled"}}, false}, + "enabled": {enabledEgress(), true}, + } { + if got := tt.egress.Enabled(); got != tt.want { + t.Errorf("%s: Enabled() = %v, want %v", name, got, tt.want) + } + } +} diff --git a/internal/cnibgp/ops_add.go b/internal/cnibgp/ops_add.go index d93a5981..2f50f431 100644 --- a/internal/cnibgp/ops_add.go +++ b/internal/cnibgp/ops_add.go @@ -76,7 +76,8 @@ func cmdAdd(args *skel.CmdArgs) (err error) { return fmt.Errorf("decode VPC: %w", err) } - cfg := publishConfig{vpc: pluginConf.VPC, vpcAttachment: pluginConf.VPCAttachment, ifaceType: ifaceType} + cfg := publishConfig{vpc: pluginConf.VPC, vpcAttachment: pluginConf.VPCAttachment, ifaceType: ifaceType, + egress: pluginConf.Egress} result, err := publishBGPState(args, cfg, nodeName, namespace, ipamResult, vpcHex, k8sClient) tracker.publishResult = result if err != nil { diff --git a/internal/cnibgp/types.go b/internal/cnibgp/types.go index 51c1af39..50c6d5f9 100644 --- a/internal/cnibgp/types.go +++ b/internal/cnibgp/types.go @@ -19,7 +19,39 @@ type PluginConf struct { VPC string `json:"vpc"` VPCAttachment string `json:"vpcattachment"` Namespace string `json:"namespace,omitempty"` + + // Egress is this network's internet egress declaration, written into the + // stanza by whoever renders it from the network's declared intent. Absent, + // or anything but Enabled, installs no egress route for the VRF and + // withdraws one that is there. + Egress *Egress `json:"egress,omitempty"` } // HostConf holds node-local settings read from /etc/cni/net.d/10-galactic.conflist. type HostConf = hostconf.HostConf + +// Egress is the per-network internet egress declaration. It names no shard: +// the node routes toward the shard its own configuration names, so the +// declaration decides only whether this VRF gets that route. +// +// It is per network because egress is per network. The node-wide shard list +// on its own gives every network on the node a default route out, including +// every network that declared no egress, so the list cannot express a network +// that opted out. The declaration can. +type Egress struct { + Internet *InternetEgress `json:"internet,omitempty"` +} + +// InternetEgressEnabled is the one mode a node acts on. +const InternetEgressEnabled = "Enabled" + +// InternetEgress is whether this network reaches the internet. +type InternetEgress struct { + Mode string `json:"mode"` +} + +// Enabled reports whether a declaration asks for an egress route. A nil +// declaration, or any mode but Enabled, does not. +func (e *Egress) Enabled() bool { + return e != nil && e.Internet != nil && e.Internet.Mode == InternetEgressEnabled +} diff --git a/internal/plumbing/srv6/egress.go b/internal/plumbing/srv6/egress.go index ec77fa6a..ff3c78bd 100644 --- a/internal/plumbing/srv6/egress.go +++ b/internal/plumbing/srv6/egress.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "net" + "os" "github.com/vishvananda/netlink" @@ -248,3 +249,36 @@ func ResolvePublicUplink() (linkIndex int, dmac, smac net.HardwareAddr, err erro "no resolved neighbor found on %s (the SRv6/underlay-facing interface) -- the underlay hasn't converged yet", names[0]) } + +// EgressDefaultRouteWithdraw removes egress_route_table's default (::/0) entry +// for Linux VRF table tableID, for a VRF whose network declares no egress. +// +// A node with no loaded datapath has no pinned map to withdraw from. That is +// nothing to do rather than a failure: it is the state every node is in before +// the datapath loads, and a network that declares no egress must behave there +// exactly as it did when no egress existed anywhere, which is to touch no bpffs +// at all rather than to fail an attachment's ADD. +func EgressDefaultRouteWithdraw(tableID uint32) error { + if !egressDatapathLoaded() { + return nil + } + return EgressDefaultRouteDel(tableID) +} + +// EgressPrefixRouteWithdraw removes egress_route_table's entry for prefix on +// Linux VRF table tableID, the counterpart to EgressPrefixRouteAdd. It skips a +// node with no loaded datapath for the same reason. +func EgressPrefixRouteWithdraw(tableID uint32, prefix *net.IPNet) error { + if !egressDatapathLoaded() { + return nil + } + return RouteEgressDel(prefix, tableID) +} + +// egressDatapathLoaded reports whether this node has a pin directory to open a +// map from. It reads the same package var the helpers here resolve their +// pinned maps through, so a test redirecting that var redirects this too. +func egressDatapathLoaded() bool { + _, err := os.Stat(pinDir) + return err == nil +} diff --git a/internal/plumbing/srv6/egress_test.go b/internal/plumbing/srv6/egress_test.go index f3c7cb75..083a1ef4 100644 --- a/internal/plumbing/srv6/egress_test.go +++ b/internal/plumbing/srv6/egress_test.go @@ -8,6 +8,7 @@ import ( "fmt" "net" "os" + "path/filepath" "testing" "github.com/containernetworking/plugins/pkg/ns" @@ -720,3 +721,90 @@ func TestResolvePublicUplink_NoResolvedNeighborFailsLoudly(t *testing.T) { t.Error("ResolvePublicUplink() = nil error with no resolved neighbor, want an error") } } + +// TestEgressDefaultRouteWithdraw_NoDatapathIsANoop covers the guard that keeps +// a network with no egress behaving exactly as it did before egress existed. +// A node whose datapath has not loaded has no pinned map to open, and an +// attachment there must not fail its ADD over a route it never had. +func TestEgressDefaultRouteWithdraw_NoDatapathIsANoop(t *testing.T) { + prevPinDir := pinDir + pinDir = filepath.Join(t.TempDir(), "no-such-datapath") + t.Cleanup(func() { pinDir = prevPinDir }) + + if err := EgressDefaultRouteWithdraw(1); err != nil { + t.Errorf("EgressDefaultRouteWithdraw(1) = %v, want nil with no loaded datapath", err) + } + _, prefix, err := net.ParseCIDR("2001:db8:64::/96") + if err != nil { + t.Fatalf("parse test prefix: %v", err) + } + if err := EgressPrefixRouteWithdraw(1, prefix); err != nil { + t.Errorf("EgressPrefixRouteWithdraw(1, %s) = %v, want nil with no loaded datapath", prefix, err) + } +} + +// TestEgressDefaultRouteWithdraw_RemovesBothPrefixes is the withdrawal half of +// a per-network egress declaration: a network that stops declaring egress must +// lose both routes, the ::/0 default and the more-specific NAT64 prefix. +// Leaving the second behind would withdraw a network's IPv6 egress and leave +// its IPv4 egress running. +// +// It also covers the idempotence the attach path relies on: withdrawing twice, +// and withdrawing an entry that was never installed, are both successes. +func TestEgressDefaultRouteWithdraw_RemovesBothPrefixes(t *testing.T) { + requireRoot(t) + testPinDir := setUpTestPinDir(t) + + const ( + ifaceName = "srv6egtest5" + ifaceAddr = "2001:db8:7::1/64" + shardSID = "2001:db8:ff01:1:e001::" + table = 77 + ) + nsObj := setUpResolvableSID(t, ifaceName, ifaceAddr, shardSID) + shardSIDs := []net.IP{net.ParseIP(shardSID)} + _, nat64Prefix, err := net.ParseCIDR("2001:db8:64::/96") + if err != nil { + t.Fatalf("parse NAT64 prefix: %v", err) + } + + if err := nsObj.Do(func(_ ns.NetNS) error { + if addErr := EgressDefaultRouteAdd(table, shardSIDs); addErr != nil { + return addErr + } + return EgressPrefixRouteAdd(table, nat64Prefix, shardSIDs) + }); err != nil { + t.Fatalf("install egress routes: %v", err) + } + + entryTable, closer, err := egressroutemap.OpenPinnedEgressRouteTable(testPinDir) + if err != nil { + t.Fatalf("open pinned egress_route_table: %v", err) + } + defer func() { _ = closer.Close() }() + + for _, prefix := range []*net.IPNet{egressroutemap.DefaultPrefix, nat64Prefix} { + if _, ok, lookupErr := entryTable.Lookup(table, prefix); lookupErr != nil { + t.Fatalf("lookup %s before withdrawal: %v", prefix, lookupErr) + } else if !ok { + t.Fatalf("no entry installed for %s", prefix) + } + } + + for range 2 { + if err := EgressDefaultRouteWithdraw(table); err != nil { + t.Fatalf("EgressDefaultRouteWithdraw(%d) = %v, want success", table, err) + } + if err := EgressPrefixRouteWithdraw(table, nat64Prefix); err != nil { + t.Fatalf("EgressPrefixRouteWithdraw(%d, %s) = %v, want success", table, nat64Prefix, err) + } + } + + for _, prefix := range []*net.IPNet{egressroutemap.DefaultPrefix, nat64Prefix} { + if _, ok, lookupErr := entryTable.Lookup(table, prefix); lookupErr != nil { + t.Fatalf("lookup %s after withdrawal: %v", prefix, lookupErr) + } else if ok { + t.Errorf("entry for %s still present after withdrawal", prefix) + } + } +}