diff --git a/.gitignore b/.gitignore index 1bfc08ab..e78e17ee 100644 --- a/.gitignore +++ b/.gitignore @@ -15,6 +15,10 @@ coverage.html /galactic /galactic-router /galactic-cni +/galactic-tap-cni +/galactic-ipam +/galactic-bgp +/galactic-route # Go workspace go.work diff --git a/Taskfile.yaml b/Taskfile.yaml index 597f77f9..a368dec1 100644 --- a/Taskfile.yaml +++ b/Taskfile.yaml @@ -134,6 +134,7 @@ tasks: cmds: - go build -ldflags "{{.LDFLAGS}}" -o bin/galactic-cni ./cmd/galactic-cni - go build -ldflags "{{.LDFLAGS}}" -o bin/galactic-tap-cni ./cmd/galactic-tap-cni + - go build -ldflags "{{.LDFLAGS}}" -o bin/galactic-ipam ./cmd/galactic-ipam - go build -ldflags "{{.LDFLAGS}}" -o bin/galactic-router ./cmd/galactic-router - go build -ldflags "{{.LDFLAGS}}" -o bin/vmtap-cni ./cmd/vmtap-cni - GOBIN={{.LOCALBIN}} go install github.com/containernetworking/plugins/plugins/main/host-device@v1.9.1 diff --git a/cmd/galactic-ipam/main.go b/cmd/galactic-ipam/main.go new file mode 100644 index 00000000..0d61a691 --- /dev/null +++ b/cmd/galactic-ipam/main.go @@ -0,0 +1,79 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package main + +import ( + "fmt" + "log" + "os" + "strings" + + "github.com/containernetworking/cni/pkg/version" + "github.com/spf13/cobra" + "golang.org/x/term" + + "go.datum.net/galactic/internal/cniipam" + "go.datum.net/galactic/internal/metadata" +) + +const ( + appName = "galactic-ipam" + + appDesc = `Galactic IPAM CNI Plugin + + The delegated CNI IPAM plugin in the galactic CNI chain — invoked by + galactic-cni/galactic-tap-cni's own "ipam" block via the CNI IPAM + delegation protocol (github.com/containernetworking/cni/pkg/ipam), never + run directly from a conflist. Has no Kubernetes dependency at all: + allocation state persists in on-disk marker files under this node's own + filesystem. + + Find more information at: https://www.datum.net/docs` +) + +func newRootCommand() *cobra.Command { + cmd := &cobra.Command{ + Use: appName, + Short: strings.Split(appDesc, "\n")[0], + Long: appDesc, + RunE: func(cmd *cobra.Command, _ []string) error { + if ok, _ := cmd.Flags().GetBool("build-info"); ok { + fmt.Println(metadata.BuildInfo(appName)) + return nil + } + if ok, _ := cmd.Flags().GetBool("version"); ok { + fmt.Printf("%s version %s\n", appName, metadata.Version) + return nil + } + if os.Getenv("CNI_COMMAND") == "VERSION" { + return version.All.Encode(os.Stdout) + } + + // Real CNI runtimes (via IPAM delegation's ExecAdd/ExecDel/ + // ExecCheck) always pipe the netconf JSON on stdin and close + // it. If stdin is an interactive terminal instead, no config + // will ever arrive and skel's blocking stdin read would hang + // forever — print version info instead. + if term.IsTerminal(int(os.Stdin.Fd())) { + fmt.Printf("%s version %s\n", appName, metadata.Version) + fmt.Printf("CNI protocol versions supported: %s\n", strings.Join(version.All.SupportedVersions(), ", ")) + return nil + } + + cniipam.RunPlugin() + return nil + }, + } + + cmd.Flags().Bool("build-info", false, "Print build information and exit") + cmd.Flags().BoolP("version", "V", false, "Print version and exit") + return cmd +} + +func main() { + if err := newRootCommand().Execute(); err != nil { + log.Fatalf("error: %v", err) + } +} diff --git a/containers/galactic-cni/Dockerfile b/containers/galactic-cni/Dockerfile index ec09b1ab..74baa0b4 100644 --- a/containers/galactic-cni/Dockerfile +++ b/containers/galactic-cni/Dockerfile @@ -62,6 +62,19 @@ RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build \ -X go.datum.net/galactic/internal/metadata.GitURL=${GIT_URL}" \ -o galactic-tap-cni cmd/galactic-tap-cni/main.go +# Build galactic-ipam, the delegated CNI IPAM plugin in the galactic CNI +# chain. Ships in this same image/binary set for the same reason +# galactic-tap-cni does — see its own comment above. +RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build \ + -ldflags "-s -w \ + -X go.datum.net/galactic/internal/metadata.Version=${VERSION} \ + -X go.datum.net/galactic/internal/metadata.GitCommit=${GIT_COMMIT} \ + -X go.datum.net/galactic/internal/metadata.GitTreeState=${GIT_TREE_STATE} \ + -X go.datum.net/galactic/internal/metadata.BuildDate=${BUILD_DATE} \ + -X go.datum.net/galactic/internal/metadata.SPDXLicense=${SPDX_LICENSE} \ + -X go.datum.net/galactic/internal/metadata.GitURL=${GIT_URL}" \ + -o galactic-ipam cmd/galactic-ipam/main.go + # Build vmtap-cni. It ships in this image rather than one of its own so the # vmtap DaemonSet (config/vmtap/) can reference the same published # ghcr.io/datum-cloud/galactic-cni image instead of a second, separately @@ -97,6 +110,7 @@ FROM gcr.io/distroless/static:nonroot AS production # Copy binaries from builder into the distroless production image COPY --from=builder /workspace/galactic-cni /galactic-cni COPY --from=builder /workspace/galactic-tap-cni /galactic-tap-cni +COPY --from=builder /workspace/galactic-ipam /galactic-ipam COPY --from=builder /workspace/vmtap-cni /vmtap-cni COPY --from=builder /workspace/host-device /host-device COPY --from=builder /var/run/galactic-cni /var/run/galactic-cni @@ -110,6 +124,7 @@ FROM docker.io/library/alpine:latest # Copy binaries from the production (distroless) image COPY --from=production /galactic-cni /galactic-cni COPY --from=production /galactic-tap-cni /galactic-tap-cni +COPY --from=production /galactic-ipam /galactic-ipam COPY --from=production /vmtap-cni /vmtap-cni COPY --from=production /host-device /host-device COPY --from=production /var/run/galactic-cni /var/run/galactic-cni diff --git a/deploy/containerlab/resources/tenants/ns10/dfw/nad.yaml b/deploy/containerlab/resources/tenants/ns10/dfw/nad.yaml index 5cdd682b..d36c7be4 100644 --- a/deploy/containerlab/resources/tenants/ns10/dfw/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns10/dfw/nad.yaml @@ -13,6 +13,9 @@ spec: "vpc": "10", "vpcattachment": "10", "namespace": "galactic-system", - "ipv6_subnet": "fd20:10:ff01::/48", - "address_families": ["ipv6"] + "ipam": { + "type": "galactic-ipam", + "ipv6_subnet": "fd20:10:ff01::/48", + "address_families": ["ipv6"] + } } diff --git a/deploy/containerlab/resources/tenants/ns10/iad/nad.yaml b/deploy/containerlab/resources/tenants/ns10/iad/nad.yaml index 9955b1ec..ea2b59e5 100644 --- a/deploy/containerlab/resources/tenants/ns10/iad/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns10/iad/nad.yaml @@ -13,6 +13,9 @@ spec: "vpc": "10", "vpcattachment": "10", "namespace": "galactic-system", - "ipv6_subnet": "fd20:10:ff03::/48", - "address_families": ["ipv6"] + "ipam": { + "type": "galactic-ipam", + "ipv6_subnet": "fd20:10:ff03::/48", + "address_families": ["ipv6"] + } } diff --git a/deploy/containerlab/resources/tenants/ns10/sjc/nad.yaml b/deploy/containerlab/resources/tenants/ns10/sjc/nad.yaml index fdd6439b..aa48fa98 100644 --- a/deploy/containerlab/resources/tenants/ns10/sjc/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns10/sjc/nad.yaml @@ -13,6 +13,9 @@ spec: "vpc": "10", "vpcattachment": "10", "namespace": "galactic-system", - "ipv6_subnet": "fd20:10:ff02::/48", - "address_families": ["ipv6"] + "ipam": { + "type": "galactic-ipam", + "ipv6_subnet": "fd20:10:ff02::/48", + "address_families": ["ipv6"] + } } diff --git a/deploy/containerlab/resources/tenants/ns20/dfw/nad.yaml b/deploy/containerlab/resources/tenants/ns20/dfw/nad.yaml index 0dec53aa..6d9f565e 100644 --- a/deploy/containerlab/resources/tenants/ns20/dfw/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns20/dfw/nad.yaml @@ -13,7 +13,10 @@ spec: "vpc": "20", "vpcattachment": "20", "namespace": "galactic-system", - "ipv6_subnet": "fd20:20:ff01::/48", - "ipv4_subnet": "172.21.1.0/24", - "address_families": ["ipv6", "ipv4"] + "ipam": { + "type": "galactic-ipam", + "ipv6_subnet": "fd20:20:ff01::/48", + "ipv4_subnet": "172.21.1.0/24", + "address_families": ["ipv6", "ipv4"] + } } diff --git a/deploy/containerlab/resources/tenants/ns20/iad/nad.yaml b/deploy/containerlab/resources/tenants/ns20/iad/nad.yaml index bfa8f449..852c95fa 100644 --- a/deploy/containerlab/resources/tenants/ns20/iad/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns20/iad/nad.yaml @@ -13,7 +13,10 @@ spec: "vpc": "20", "vpcattachment": "20", "namespace": "galactic-system", - "ipv6_subnet": "fd20:20:ff03::/48", - "ipv4_subnet": "172.21.10.0/24", - "address_families": ["ipv6", "ipv4"] + "ipam": { + "type": "galactic-ipam", + "ipv6_subnet": "fd20:20:ff03::/48", + "ipv4_subnet": "172.21.10.0/24", + "address_families": ["ipv6", "ipv4"] + } } diff --git a/deploy/containerlab/resources/tenants/ns20/sjc/nad.yaml b/deploy/containerlab/resources/tenants/ns20/sjc/nad.yaml index 13e93bcd..c82099db 100644 --- a/deploy/containerlab/resources/tenants/ns20/sjc/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns20/sjc/nad.yaml @@ -13,7 +13,10 @@ spec: "vpc": "20", "vpcattachment": "20", "namespace": "galactic-system", - "ipv6_subnet": "fd20:20:ff02::/48", - "ipv4_subnet": "172.21.20.0/24", - "address_families": ["ipv6", "ipv4"] + "ipam": { + "type": "galactic-ipam", + "ipv6_subnet": "fd20:20:ff02::/48", + "ipv4_subnet": "172.21.20.0/24", + "address_families": ["ipv6", "ipv4"] + } } diff --git a/deploy/containerlab/resources/tenants/ns30/dfw/nad.yaml b/deploy/containerlab/resources/tenants/ns30/dfw/nad.yaml index 0f0bb879..56b05256 100644 --- a/deploy/containerlab/resources/tenants/ns30/dfw/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns30/dfw/nad.yaml @@ -13,6 +13,9 @@ spec: "vpc": "30", "vpcattachment": "30", "namespace": "galactic-system", - "ipv6_subnet": "fd20:30:ff01::/48", - "address_families": ["ipv6"] + "ipam": { + "type": "galactic-ipam", + "ipv6_subnet": "fd20:30:ff01::/48", + "address_families": ["ipv6"] + } } diff --git a/deploy/containerlab/resources/tenants/ns40/iad/nad.yaml b/deploy/containerlab/resources/tenants/ns40/iad/nad.yaml index 2b341ee3..425c2292 100644 --- a/deploy/containerlab/resources/tenants/ns40/iad/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns40/iad/nad.yaml @@ -13,6 +13,9 @@ spec: "vpc": "40", "vpcattachment": "40", "namespace": "galactic-system", - "ipv4_subnet": "172.40.10.0/24", - "address_families": ["ipv4"] + "ipam": { + "type": "galactic-ipam", + "ipv4_subnet": "172.40.10.0/24", + "address_families": ["ipv4"] + } } diff --git a/deploy/containerlab/resources/tenants/ns50/dfw/nad.yaml b/deploy/containerlab/resources/tenants/ns50/dfw/nad.yaml index 949a90da..574a03bb 100644 --- a/deploy/containerlab/resources/tenants/ns50/dfw/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns50/dfw/nad.yaml @@ -13,8 +13,11 @@ spec: "vpc": "50", "vpcattachment": "50", "namespace": "galactic-system", - "ipv4_subnet": "172.20.1.0/24", - "address_families": ["ipv4"] + "ipam": { + "type": "galactic-ipam", + "ipv4_subnet": "172.20.1.0/24", + "address_families": ["ipv4"] + } } --- diff --git a/deploy/containerlab/resources/tenants/ns50/iad/nad.yaml b/deploy/containerlab/resources/tenants/ns50/iad/nad.yaml index ad08218b..8d197bfa 100644 --- a/deploy/containerlab/resources/tenants/ns50/iad/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns50/iad/nad.yaml @@ -13,6 +13,9 @@ spec: "vpc": "50", "vpcattachment": "50", "namespace": "galactic-system", - "ipv4_subnet": "172.20.10.0/24", - "address_families": ["ipv4"] + "ipam": { + "type": "galactic-ipam", + "ipv4_subnet": "172.20.10.0/24", + "address_families": ["ipv4"] + } } diff --git a/deploy/containerlab/resources/tenants/ns50/sjc/nad.yaml b/deploy/containerlab/resources/tenants/ns50/sjc/nad.yaml index 25ea8b37..8fd60ebd 100644 --- a/deploy/containerlab/resources/tenants/ns50/sjc/nad.yaml +++ b/deploy/containerlab/resources/tenants/ns50/sjc/nad.yaml @@ -13,6 +13,9 @@ spec: "vpc": "50", "vpcattachment": "50", "namespace": "galactic-system", - "ipv4_subnet": "172.20.20.0/24", - "address_families": ["ipv4"] + "ipam": { + "type": "galactic-ipam", + "ipv4_subnet": "172.20.20.0/24", + "address_families": ["ipv4"] + } } diff --git a/go.mod b/go.mod index 3b678efb..d2e95759 100644 --- a/go.mod +++ b/go.mod @@ -59,9 +59,11 @@ require ( github.com/onsi/gomega v1.39.1 // indirect github.com/orcaman/concurrent-map/v2 v2.0.1 // indirect github.com/pelletier/go-toml/v2 v2.2.4 // indirect + github.com/pkg/errors v0.9.1 // indirect github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/prometheus/common v0.67.5 // indirect github.com/prometheus/procfs v0.19.2 // indirect + github.com/safchain/ethtool v0.6.2 // indirect github.com/sagikazarmark/locafero v0.11.0 // indirect github.com/segmentio/fasthash v1.0.3 // indirect github.com/sourcegraph/conc v0.3.1-0.20240121214520-5f936abd7ae8 // indirect @@ -96,6 +98,7 @@ require ( k8s.io/kube-openapi v0.0.0-20260317180543-43fb72c5454a // indirect k8s.io/utils v0.0.0-20260210185600-b8788abfbbc2 // indirect sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect + sigs.k8s.io/knftables v0.0.18 // indirect sigs.k8s.io/randfill v1.0.0 // indirect sigs.k8s.io/structured-merge-diff/v6 v6.3.3 // indirect sigs.k8s.io/yaml v1.6.0 // indirect diff --git a/go.sum b/go.sum index eb1c522b..3967c824 100644 --- a/go.sum +++ b/go.sum @@ -134,6 +134,8 @@ github.com/prometheus/procfs v0.19.2/go.mod h1:M0aotyiemPhBCM0z5w87kL22CxfcH05Zp github.com/rogpeppe/go-internal v1.14.1 h1:UQB4HGPB6osV0SQTLymcB4TgvyWu6ZyliaW0tI/otEQ= github.com/rogpeppe/go-internal v1.14.1/go.mod h1:MaRKkUm5W0goXpeCfT7UZI6fk/L7L7so1lCWt35ZSgc= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= +github.com/safchain/ethtool v0.6.2 h1:O3ZPFAKEUEfbtE6J/feEe2Ft7dIJ2Sy8t4SdMRiIMHY= +github.com/safchain/ethtool v0.6.2/go.mod h1:VS7cn+bP3Px3rIq55xImBiZGHVLNyBh5dqG6dDQy8+I= github.com/sagikazarmark/locafero v0.11.0 h1:1iurJgmM9G3PA/I+wWYIOw/5SyBtxapeHDcg+AAIFXc= github.com/sagikazarmark/locafero v0.11.0/go.mod h1:nVIGvgyzw595SUSUE6tvCp3YYTeHs15MvlmU87WwIik= github.com/segmentio/fasthash v1.0.3 h1:EI9+KE1EwvMLBWwjpRDc+fEM+prwxDYbslddQGtrmhM= @@ -291,6 +293,8 @@ sigs.k8s.io/controller-runtime v0.24.1 h1:miPEwrmirImAvgME1L9qebGHrOnGJoVmVdtOU9 sigs.k8s.io/controller-runtime v0.24.1/go.mod h1:vFkfY5fGt5xAC/sKb8IBFKgWPNKG9OUG29dR8Y2wImw= sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 h1:IpInykpT6ceI+QxKBbEflcR5EXP7sU1kvOlxwZh5txg= sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730/go.mod h1:mdzfpAEoE6DHQEN0uh9ZbOCuHbLK5wOm7dK4ctXE9Tg= +sigs.k8s.io/knftables v0.0.18 h1:6Duvmu0s/HwGifKrtl6G3AyAPYlWiZqTgS8bkVMiyaE= +sigs.k8s.io/knftables v0.0.18/go.mod h1:f/5ZLKYEUPUhVjUCg6l80ACdL7CIIyeL0DxfgojGRTk= sigs.k8s.io/randfill v1.0.0 h1:JfjMILfT8A6RbawdsK2JXGBR5AQVfd+9TbzrlneTyrU= sigs.k8s.io/randfill v1.0.0/go.mod h1:XeLlZ/jmk4i1HRopwe7/aU3H5n1zNUcX6TM94b3QxOY= sigs.k8s.io/structured-merge-diff/v6 v6.3.2 h1:kwVWMx5yS1CrnFWA/2QHyRVJ8jM6dBA80uLmm0wJkk8= diff --git a/internal/cni/cni.go b/internal/cni/cni.go index 762f27ad..b70d1183 100644 --- a/internal/cni/cni.go +++ b/internal/cni/cni.go @@ -10,19 +10,11 @@ import ( "github.com/containernetworking/cni/pkg/skel" "github.com/containernetworking/cni/pkg/version" - "go.datum.net/galactic/internal/cniipam" "go.datum.net/galactic/internal/metadata" ) const cniTimeout = 10 * time.Second -// SetEnableLocalIPAM sets the local IPAM flag from the CLI. Kept here as a -// thin forwarder so cmd/galactic-cni/main.go doesn't need to know that IPAM -// allocation itself now lives in internal/cniipam. -func SetEnableLocalIPAM(v bool) { - cniipam.SetEnableLocalIPAM(v) -} - // RunPlugin starts the CNI plugin, handling ADD, DEL, CHECK, and STATUS operations. func RunPlugin() { skel.PluginMainFuncs( diff --git a/internal/cni/cni_test.go b/internal/cni/cni_test.go index 39bd9360..3734151b 100644 --- a/internal/cni/cni_test.go +++ b/internal/cni/cni_test.go @@ -12,7 +12,6 @@ import ( "net" "os" "path/filepath" - "reflect" "strings" "testing" @@ -68,12 +67,11 @@ func assertCNIError(t *testing.T, err error, wantCode uint, wantMsg string) { func TestParseConf(t *testing.T) { tests := []struct { - name string - input string - wantVPC string - wantAddressFamilies []string // nil means "don't check" - wantErr string - wantCode uint // CNI error code; 0 means "don't check" + name string + input string + wantVPC string + wantErr string + wantCode uint // CNI error code; 0 means "don't check" }{ { name: "valid config", @@ -191,131 +189,16 @@ func TestParseConf(t *testing.T) { wantVPC: testVPC, }, - // ---- dual-stack addressing fields (ipv6_subnet, ipv4_subnet, address_families) ---- - { - name: "dual-stack fields omitted parses successfully", + name: "ipam block present is accepted, delegated per its own contract", input: fmt.Sprintf( `{"cniVersion":"1.0.0","name":"test",`+ `"type":"galactic-cni","vpc":"%s",`+ - `"vpcattachment":"%s"}`, - testVPC, testAttachment, - ), - wantVPC: testVPC, - wantAddressFamilies: []string{addressFamilyIPv6}, - }, - { - name: "valid ipv6_subnet accepted", - input: fmt.Sprintf( - `{"cniVersion":"1.0.0","name":"test",`+ - `"type":"galactic-cni","vpc":"%s",`+ - `"vpcattachment":"%s","ipv6_subnet":"fd00:10:ff01::/48"}`, + `"vpcattachment":"%s","ipam":{"type":"galactic-ipam","ipv6_subnet":"fd00:10:ff01::/48"}}`, testVPC, testAttachment, ), wantVPC: testVPC, }, - { - name: "invalid ipv6_subnet CIDR rejected", - input: fmt.Sprintf( - `{"cniVersion":"1.0.0","name":"test",`+ - `"type":"galactic-cni","vpc":"%s",`+ - `"vpcattachment":"%s","ipv6_subnet":"not-a-cidr"}`, - testVPC, testAttachment, - ), - wantErr: "invalid CIDR value for field 'ipv6_subnet'", - wantCode: 7, - }, - { - name: "ipv4 CIDR given where ipv6_subnet expected rejected", - input: fmt.Sprintf( - `{"cniVersion":"1.0.0","name":"test",`+ - `"type":"galactic-cni","vpc":"%s",`+ - `"vpcattachment":"%s","ipv6_subnet":"10.0.0.0/24"}`, - testVPC, testAttachment, - ), - wantErr: "ipv6_subnet must be an IPv6 CIDR, got IPv4", - wantCode: 7, - }, - { - name: "ipv6_subnet prefix length over 96 rejected", - input: fmt.Sprintf( - `{"cniVersion":"1.0.0","name":"test",`+ - `"type":"galactic-cni","vpc":"%s",`+ - `"vpcattachment":"%s","ipv6_subnet":"fd00:10:ff01::/112"}`, - testVPC, testAttachment, - ), - wantErr: "ipv6_subnet prefix length 112 exceeds maximum of 96", - wantCode: 7, - }, - { - name: "valid ipv4_subnet accepted", - input: fmt.Sprintf( - `{"cniVersion":"1.0.0","name":"test",`+ - `"type":"galactic-cni","vpc":"%s",`+ - `"vpcattachment":"%s","ipv4_subnet":"10.0.0.0/20"}`, - testVPC, testAttachment, - ), - wantVPC: testVPC, - }, - { - // A standard dotted-decimal CIDR can never carry a mask longer - // than /32 (net.ParseCIDR itself rejects e.g. "10.0.0.0/33"), so - // this exercises the prefix-length guard via an IPv4-mapped IPv6 - // literal, which Go parses with a 128-bit mask space. - name: "ipv4_subnet prefix length over 32 rejected", - input: fmt.Sprintf( - `{"cniVersion":"1.0.0","name":"test",`+ - `"type":"galactic-cni","vpc":"%s",`+ - `"vpcattachment":"%s","ipv4_subnet":"::ffff:10.0.0.0/40"}`, - testVPC, testAttachment, - ), - wantErr: "ipv4_subnet prefix length 40 exceeds maximum of 32", - wantCode: 7, - }, - { - name: "ipv6 CIDR given where ipv4_subnet expected rejected", - input: fmt.Sprintf( - `{"cniVersion":"1.0.0","name":"test",`+ - `"type":"galactic-cni","vpc":"%s",`+ - `"vpcattachment":"%s","ipv4_subnet":"2001:db8::/64"}`, - testVPC, testAttachment, - ), - wantErr: "ipv4_subnet must be an IPv4 CIDR, got IPv6", - wantCode: 7, - }, - { - name: "address_families defaults to ipv6 when omitted", - input: fmt.Sprintf( - `{"cniVersion":"1.0.0","name":"test",`+ - `"type":"galactic-cni","vpc":"%s",`+ - `"vpcattachment":"%s"}`, - testVPC, testAttachment, - ), - wantVPC: testVPC, - wantAddressFamilies: []string{addressFamilyIPv6}, - }, - { - name: "address_families explicit dual-stack accepted", - input: fmt.Sprintf( - `{"cniVersion":"1.0.0","name":"test",`+ - `"type":"galactic-cni","vpc":"%s",`+ - `"vpcattachment":"%s","address_families":["ipv6","ipv4"]}`, - testVPC, testAttachment, - ), - wantVPC: testVPC, - wantAddressFamilies: []string{addressFamilyIPv6, addressFamilyIPv4}, - }, - { - name: "invalid address_families entry rejected", - input: fmt.Sprintf( - `{"cniVersion":"1.0.0","name":"test",`+ - `"type":"galactic-cni","vpc":"%s",`+ - `"vpcattachment":"%s","address_families":["ipv6","bogus"]}`, - testVPC, testAttachment, - ), - wantErr: `invalid address_families entry "bogus": must be "ipv6" or "ipv4"`, - wantCode: 7, - }, } for _, tt := range tests { @@ -339,9 +222,6 @@ func TestParseConf(t *testing.T) { if conf.VPC != tt.wantVPC { t.Errorf("VPC = %q, want %q", conf.VPC, tt.wantVPC) } - if tt.wantAddressFamilies != nil && !reflect.DeepEqual(conf.AddressFamilies, tt.wantAddressFamilies) { - t.Errorf("AddressFamilies = %v, want %v", conf.AddressFamilies, tt.wantAddressFamilies) - } }) } } @@ -1186,38 +1066,48 @@ func TestLoadHostConf(t *testing.T) { } } -// ---- enableLocalIPAM required check ----------------------------------------- - -func TestEnableLocalIPAMRequired(t *testing.T) { - // Set local IPAM enabled - t.Setenv("GALACTIC_CNI_ENABLE_LOCAL_IPAM", "true") +// ---- explicit IPAM delegation contract ------------------------------------- + +// TestIPAMBlockPresenceIsTheOnlyTrigger is the regression test for the +// explicit contract internal/cniipam's doc comment describes: whether this +// plugin delegates to IPAM at all is decided solely by whether "ipam" is +// present in its own config — no environment variable can manufacture (or +// suppress) that block. The historical GALACTIC_CNI_ENABLE_LOCAL_IPAM +// trigger no longer exists at all (that flag, renamed +// GALACTIC_IPAM_ENABLE_LOCAL_IPAM, now lives entirely inside +// internal/cniipam as a default-filler for an already-present ipam block). +func TestIPAMBlockPresenceIsTheOnlyTrigger(t *testing.T) { t.Setenv("GALACTIC_CNI_NODE_NAME", "test-node") - // Missing IPAM block should cause a hard error. + // Missing ipam block: no error, no delegation signal — conf.IPAM stays nil. inputNoIPAM := fmt.Sprintf( `{"cniVersion":"1.0.0","name":"test","type":"galactic-cni","vpc":"%s","vpcattachment":"%s"}`, testVPC, testAttachment, ) - _, err := parseConf([]byte(inputNoIPAM)) - if err == nil { - t.Fatal("expected error for missing ipam block when local IPAM is enabled, got nil") + conf, err := parseConf([]byte(inputNoIPAM)) + if err != nil { + t.Fatalf("unexpected error for missing ipam block: %v", err) } - if !strings.Contains(err.Error(), "no 'ipam' block is present") { - t.Fatalf("expected error containing 'no 'ipam' block', got: %v", err) + if conf.IPAM != nil { + t.Fatalf("IPAM = %+v, want nil (absent block must never be manufactured)", conf.IPAM) } - // Present IPAM block should succeed. + // Present ipam block: conf.IPAM is populated, ready for delegation. inputWithIPAM := fmt.Sprintf( - `{"cniVersion":"1.0.0","name":"test","type":"galactic-cni","vpc":"%s","vpcattachment":"%s","ipam":{"type":"pool"}}`, + `{"cniVersion":"1.0.0","name":"test","type":"galactic-cni","vpc":"%s","vpcattachment":"%s",`+ + `"ipam":{"type":"galactic-ipam"}}`, testVPC, testAttachment, ) - conf, err := parseConf([]byte(inputWithIPAM)) + conf, err = parseConf([]byte(inputWithIPAM)) if err != nil { t.Fatalf("unexpected error with present ipam block: %v", err) } if conf.IPAM == nil { t.Fatal("expected IPAM block to be non-nil") } + if conf.IPAM.Type != "galactic-ipam" { + t.Errorf("IPAM.Type = %q, want %q", conf.IPAM.Type, "galactic-ipam") + } } // ---- logging setup ---------------------------------------------------------- diff --git a/internal/cni/config.go b/internal/cni/config.go index 3eda19e7..cfcc1d0d 100644 --- a/internal/cni/config.go +++ b/internal/cni/config.go @@ -9,7 +9,6 @@ import ( "errors" "fmt" "log/slog" - "net" "os" "path/filepath" "strings" @@ -18,7 +17,6 @@ import ( type100 "github.com/containernetworking/cni/pkg/types/100" "go.datum.net/galactic/internal/cni/hostconf" - "go.datum.net/galactic/internal/cniipam" "go.datum.net/galactic/internal/config" ) @@ -47,26 +45,6 @@ const ( errVPCAttachmentRequired = "vpcattachment is required and must be a non-empty base62 string" ) -const ( - // maxIPv6SubnetPrefixLen is the maximum (longest) prefix length allowed - // for ipv6_subnet. It matches ipam.PoolAllocator's constraint that the - // pool prefix must be no longer than the per-allocation subnet length; - // dual-stack tenant addressing allocates /96 endpoints from this subnet, - // so the subnet itself must be a /96 or shorter. - maxIPv6SubnetPrefixLen = 96 - - // maxIPv4SubnetPrefixLen is the maximum (longest) prefix length allowed - // for ipv4_subnet: a full IPv4 host route. - maxIPv4SubnetPrefixLen = 32 -) - -// addressFamilyIPv6 and addressFamilyIPv4 are the only valid entries for -// the address_families config field. -const ( - addressFamilyIPv6 = "ipv6" - addressFamilyIPv4 = "ipv4" -) - // isValidBase62 reports whether s contains only valid base62 characters // ([0-9a-zA-Z]) and is non-empty. VPC and VPCAttachment identifiers are // base62-encoded and used throughout the ADD path (interface naming, @@ -317,77 +295,15 @@ func parseConf(data []byte) (*PluginConf, error) { setupLogging(cniConfig.LogFile, cniConfig.LogLevel) slog.Debug("CNI config received", "stdin", string(data)) - // Resolve local IPAM flag and propagate it to internal/cniipam, which - // owns wantsIPAM/allocateIPAM. (This env-var-as-trigger shape is - // slated to be replaced by an explicit ipam-block-presence contract - // once internal/cniipam becomes its own delegated plugin.) - localIPAM := config.CNIGetEnableLocalIPAM() - cniipam.SetEnableLocalIPAM(localIPAM) - - // Enforce required IPAM block if local IPAM is enabled - if localIPAM && conf.IPAM == nil { - return nil, &types.Error{Code: 7, Msg: "local IPAM is enabled, but no 'ipam' block is present in the configuration"} - } - - // Validate dual-stack addressing fields (ipv6_subnet, ipv4_subnet, - // address_families). Both subnet fields stay optional at the parseConf - // level: whether one is actually required depends on which IPAM path a - // given ADD takes (static, local-IPAM fallback, or pool), which is - // resolved in internal/cniipam.Allocate — see WantsIPAM/Allocate there. - // When present, both are validated for CIDR shape so misconfigurations - // are caught early regardless of which path runs. - if conf.IPv6Subnet != "" { - ip, mask, err := net.ParseCIDR(conf.IPv6Subnet) - if err != nil { - return nil, &types.Error{Code: 7, Msg: fmt.Sprintf( - "invalid CIDR value for field 'ipv6_subnet': %q", sanitizeForError(conf.IPv6Subnet)), - } - } - if ip.To4() != nil { - return nil, &types.Error{Code: 7, Msg: fmt.Sprintf( - "ipv6_subnet must be an IPv6 CIDR, got IPv4: %q", sanitizeForError(conf.IPv6Subnet)), - } - } - if prefixLen, _ := mask.Mask.Size(); prefixLen > maxIPv6SubnetPrefixLen { - return nil, &types.Error{Code: 7, Msg: fmt.Sprintf( - "ipv6_subnet prefix length %d exceeds maximum of %d: %q", - prefixLen, maxIPv6SubnetPrefixLen, sanitizeForError(conf.IPv6Subnet)), - } - } - } - if conf.IPv4Subnet != "" { - ip, mask, err := net.ParseCIDR(conf.IPv4Subnet) - if err != nil { - return nil, &types.Error{Code: 7, Msg: fmt.Sprintf( - "invalid CIDR value for field 'ipv4_subnet': %q", sanitizeForError(conf.IPv4Subnet)), - } - } - if ip.To4() == nil { - return nil, &types.Error{Code: 7, Msg: fmt.Sprintf( - "ipv4_subnet must be an IPv4 CIDR, got IPv6: %q", sanitizeForError(conf.IPv4Subnet)), - } - } - if prefixLen, _ := mask.Mask.Size(); prefixLen > maxIPv4SubnetPrefixLen { - return nil, &types.Error{Code: 7, Msg: fmt.Sprintf( - "ipv4_subnet prefix length %d exceeds maximum of %d: %q", - prefixLen, maxIPv4SubnetPrefixLen, sanitizeForError(conf.IPv4Subnet)), - } - } - } - if len(conf.AddressFamilies) == 0 { - conf.AddressFamilies = []string{addressFamilyIPv6} - } else { - for _, af := range conf.AddressFamilies { - switch af { - case addressFamilyIPv6, addressFamilyIPv4: - default: - return nil, &types.Error{Code: 7, Msg: fmt.Sprintf( - "invalid address_families entry %q: must be %q or %q", - sanitizeForError(af), addressFamilyIPv6, addressFamilyIPv4), - } - } - } - } + // Whether IPAM runs at all is decided entirely by whether "ipam" is + // present — no environment variable or sibling field can trigger or + // suppress that. Addressing fields (ipv6_subnet, ipv4_subnet, + // address_families, static_ip) and their own default-filling/CIDR + // validation live inside internal/cniipam, since they're only ever + // read by whichever binary "ipam.type" names — this plugin passes its + // own StdinData straight through unmodified when it delegates + // (ops_add.go/ops_del.go), so validating them here too would just be + // redundant work on the same bytes. if conf.PrevResult != nil { if err := validatePrevResult(conf.PrevResult); err != nil { diff --git a/internal/cni/ipam/dualstack.go b/internal/cni/ipam/dualstack.go index 9a0c687b..bf9edfad 100644 --- a/internal/cni/ipam/dualstack.go +++ b/internal/cni/ipam/dualstack.go @@ -31,16 +31,17 @@ type DualStackResult struct { // the resulting allocator only allocates IPv4 addresses (IPv6 fields in // DualStackResult are left nil). If ipv4Pool is empty, the resulting // allocator only allocates IPv6 addresses (IPv4 fields in DualStackResult -// are left nil) and ipv4LockDir is ignored. ipv4LockDir is passed straight -// through to NewIPv4PoolAllocator (see DefaultIPv4LockDir for the production -// path). +// are left nil). lockDir is passed straight through to both +// NewPoolAllocator and NewIPv4PoolAllocator (see DefaultLockDir for the +// production path) — one shared root serves both families, since each +// pool's own CIDR namespaces its state into a distinct subdirectory. func NewDualStackAllocator( - ipv6Pool, ipv6Gateway, ipv4Pool, ipv4Gateway, ipv4LockDir string, + ipv6Pool, ipv6Gateway, ipv4Pool, ipv4Gateway, lockDir string, ) (*DualStackAllocator, error) { a := &DualStackAllocator{} if ipv6Pool != "" { - ipv6, err := NewPoolAllocator(ipv6Pool, ipv6Gateway, DefaultSubnetLen) + ipv6, err := NewPoolAllocator(ipv6Pool, ipv6Gateway, DefaultSubnetLen, lockDir) if err != nil { return nil, err } @@ -48,7 +49,7 @@ func NewDualStackAllocator( } if ipv4Pool != "" { - ipv4, err := NewIPv4PoolAllocator(ipv4Pool, ipv4Gateway, ipv4LockDir) + ipv4, err := NewIPv4PoolAllocator(ipv4Pool, ipv4Gateway, lockDir) if err != nil { return nil, err } diff --git a/internal/cni/ipam/dualstack_test.go b/internal/cni/ipam/dualstack_test.go index bc72e67a..ab231993 100644 --- a/internal/cni/ipam/dualstack_test.go +++ b/internal/cni/ipam/dualstack_test.go @@ -4,7 +4,10 @@ package ipam -import "testing" +import ( + "fmt" + "testing" +) func TestNewDualStackAllocator(t *testing.T) { tests := []struct { @@ -167,8 +170,12 @@ func TestDualStackAllocatorAllocate(t *testing.T) { t.Fatalf("unexpected error: %v", err) } + // Each iteration uses a distinct container ID -- Allocate is + // idempotent per containerID, so reusing one ID would only ever + // consume a single IPv4 address instead of exhausting the pool. for i := range 4 { - if _, err := a.Allocate("container"); err != nil { + containerID := fmt.Sprintf("container-%d", i) + if _, err := a.Allocate(containerID); err != nil { t.Fatalf("unexpected error on allocation %d: %v", i, err) } } diff --git a/internal/cni/ipam/ipam.go b/internal/cni/ipam/ipam.go index b4a90eab..f78b99d0 100644 --- a/internal/cni/ipam/ipam.go +++ b/internal/cni/ipam/ipam.go @@ -2,17 +2,25 @@ // // SPDX-License-Identifier: AGPL-3.0-or-later -// Package ipam provides IPv6 subnet allocation for the Galactic CNI. -// Each allocation returns a subnet (default /96) from a larger CIDR pool -// (e.g. a /64 region subnet). Allocations are kept ephemeral in memory; -// separate CNI plugin processes (each invocation is a separate process) rely -// on the BGPAdvertisement CRD annotation to look up the allocated subnet -// during teardown. +// Package ipam provides IPv6 subnet allocation for the Galactic CNI. Each +// allocation returns a subnet (default /96) from a larger CIDR pool (e.g. a +// /64 region subnet). +// +// Allocations persist as an on-disk marker file per allocated subnet, keyed +// by the pool CIDR (mirroring IPv4PoolAllocator's own scheme) — required +// because each CNI ADD/DEL is a separate OS process: an in-memory-only +// record (as this package used before) is discarded the moment the ADD +// process that created it exits, leaving DEL with nothing to look up. package ipam import ( + "errors" "fmt" "net" + "os" + "path/filepath" + "strconv" + "strings" "sync" ) @@ -23,23 +31,41 @@ const ( // DefaultSubnetLen is the default prefix length returned per allocation. // A /96 gives 2^32 addresses per pod subnet. DefaultSubnetLen = 96 + + // poolLockFileName is the flock target within each pool's state + // directory; every other entry in that directory is an allocation + // marker file named after the subnet it reserves. + poolLockFileName = "lock" ) -// PoolAllocator allocates IPv6 subnets from a CIDR pool, tracking -// allocations by subnet CIDR string in memory. All bindings are ephemeral. +// DefaultLockDir is the well-known parent directory for the node-local +// on-disk lock and allocation state both PoolAllocator (IPv6) and +// IPv4PoolAllocator use to stay correct across separate CNI plugin +// invocations. Both families share one root — each pool's own CIDR +// namespaces its state into a distinct subdirectory (sanitizePoolDirName), +// so an IPv6 pool and an IPv4 pool never collide here. +const DefaultLockDir = "/var/lib/cni/galactic-ipam" + +// PoolAllocator allocates IPv6 subnets from a CIDR pool, persisting each +// allocation as a marker file under a lock directory, guarded by a +// cross-process flock — see IPv4PoolAllocator's own doc comment for why +// this is required rather than optional. type PoolAllocator struct { - pool *net.IPNet // the master pool (e.g. a /64 region subnet) - subnetLen int // prefix length per allocation (e.g. 96) - gateway net.IP // gateway IP address - poolIP net.IP // immutable copy of pool.IP for boundary checks - reserved string // subnet CIDR string containing the gateway; never allocated - allocations sync.Map // allocated subnet CIDR string -> struct{}{} - mu sync.Mutex // serializes Allocate calls + pool *net.IPNet // the master pool (e.g. a /64 region subnet) + subnetLen int // prefix length per allocation (e.g. 96) + gateway net.IP // gateway IP address + poolIP net.IP // immutable copy of pool.IP for boundary checks + reserved string // subnet CIDR string containing the gateway; never allocated + mu sync.Mutex // serializes Allocate/Deallocate within this process + stateDir string // directory holding the lock file and one allocation marker file per allocated subnet + state lockedState } -// NewPoolAllocator creates a new pool allocator from an IPv6 CIDR pool, -// an optional gateway address, and a subnet prefix length. The pool must be -// an IPv6 prefix with a length of subnetLen or fewer bits (e.g. a /64 region +// NewPoolAllocator creates a new pool allocator from an IPv6 CIDR pool, an +// optional gateway address, a subnet prefix length, and a parent directory +// for this pool's on-disk lock and allocation state (see DefaultLockDir for +// the production path; lockDir must not be empty). The pool must be an +// IPv6 prefix with a length of subnetLen or fewer bits (e.g. a /64 region // subnet when subnetLen is the default /96, though any pool length <= // subnetLen is accepted). If gateway is empty, the first address in the pool // (host bits = 1) is used as the gateway. If subnetLen is 0, DefaultSubnetLen @@ -48,7 +74,7 @@ type PoolAllocator struct { // self-assign the gateway's own address to one of its secondary/pod // addresses, colliding with the address every other endpoint in the pool // routes its default route through. -func NewPoolAllocator(poolCIDR, gateway string, subnetLen int) (*PoolAllocator, error) { +func NewPoolAllocator(poolCIDR, gateway string, subnetLen int, lockDir string) (*PoolAllocator, error) { _, pool, err := net.ParseCIDR(poolCIDR) if err != nil { return nil, fmt.Errorf("parse pool CIDR %q: %w", poolCIDR, err) @@ -66,11 +92,14 @@ func NewPoolAllocator(poolCIDR, gateway string, subnetLen int) (*PoolAllocator, return nil, fmt.Errorf("pool prefix length %d is longer than subnet length %d", mask, subnetLen) } + if lockDir == "" { + return nil, errors.New("lockDir must not be empty") + } + pa := &PoolAllocator{ - pool: pool, - subnetLen: subnetLen, - poolIP: make(net.IP, ipv6Bits/8), - allocations: sync.Map{}, + pool: pool, + subnetLen: subnetLen, + poolIP: make(net.IP, ipv6Bits/8), } copy(pa.poolIP, pool.IP) @@ -97,65 +126,164 @@ func NewPoolAllocator(poolCIDR, gateway string, subnetLen int) (*PoolAllocator, } pa.reserved = reservedSubnet.String() + stateDir := filepath.Join(lockDir, sanitizePoolDirName(pool.String())) + if err := os.MkdirAll(stateDir, 0o700); err != nil { + return nil, fmt.Errorf("create pool state dir %q: %w", stateDir, err) + } + pa.stateDir = stateDir + pa.state = lockedState{stateDir: stateDir, lockFileName: poolLockFileName} + return pa, nil } // Allocate assigns the next available IPv6 subnet from the pool for the // given container ID, skipping the subnet that contains the pool's gateway -// address. Returns the allocated subnet CIDR or an error if the pool is -// exhausted. Thread-safe. -func (a *PoolAllocator) Allocate(_ string) (*net.IPNet, error) { +// address and any subnet another allocation already holds (per the on-disk +// marker files, so this is correct across separate CNI plugin invocations +// on the same pool). If containerID already holds an allocation in this +// pool, that same subnet is returned rather than a fresh one being handed +// out — the CNI spec permits a runtime to retry ADD for the same container +// after a transient failure, and without this check each retry would leak +// the marker file from the previous attempt (findContainerMarker only ever +// returns the first match, so only one of the leaked markers would ever be +// recoverable via DEL). Returns the allocated subnet CIDR or an error if the +// pool is exhausted. Thread-safe. +func (a *PoolAllocator) Allocate(containerID string) (*net.IPNet, error) { a.mu.Lock() defer a.mu.Unlock() - // Collect currently allocated subnets for fast lookup. - used := make(map[string]struct{}) - a.allocations.Range(func(key, _ any) bool { - used[key.(string)] = struct{}{} - return true - }) - - // Iterate subnet boundaries within the pool. - subnetStart := make(net.IP, ipv6Bits/8) - copy(subnetStart, a.poolIP) - - for ; a.pool.Contains(subnetStart); subnetStart = incSubnet(subnetStart, a.subnetLen) { - // Build the subnet CIDR for this boundary. - subnet := &net.IPNet{ - IP: make(net.IP, ipv6Bits/8), - Mask: net.CIDRMask(a.subnetLen, ipv6Bits), + var result *net.IPNet + err := a.state.withLock(func() error { + if name, ok := a.state.findContainerMarkerLocked(containerID); ok { + existing, err := parseAllocatedSubnet(desanitizeMarkerName(name)) + if err != nil { + return fmt.Errorf("parse existing allocation marker %q: %w", name, err) + } + result = existing + return nil } - copy(subnet.IP, subnetStart) - subnetStr := subnet.String() - // Skip the subnet reserved for the gateway. - if subnetStr == a.reserved { - continue + used, err := a.usedSubnets() + if err != nil { + return err } - // Skip already allocated. - if _, ok := used[subnetStr]; ok { - continue + // Iterate subnet boundaries within the pool. + subnetStart := make(net.IP, ipv6Bits/8) + copy(subnetStart, a.poolIP) + + for ; a.pool.Contains(subnetStart); subnetStart = incSubnet(subnetStart, a.subnetLen) { + subnet := &net.IPNet{ + IP: make(net.IP, ipv6Bits/8), + Mask: net.CIDRMask(a.subnetLen, ipv6Bits), + } + copy(subnet.IP, subnetStart) + subnetStr := subnet.String() + + if subnetStr == a.reserved { + continue + } + if _, ok := used[subnetStr]; ok { + continue + } + + markerPath := filepath.Join(a.stateDir, sanitizePoolDirName(subnetStr)) + if err := os.WriteFile(markerPath, []byte(containerID), 0o600); err != nil { + return fmt.Errorf("write allocation marker %q: %w", markerPath, err) + } + result = subnet + return nil } - // Allocate. - a.allocations.Store(subnetStr, struct{}{}) - return subnet, nil + return fmt.Errorf("pool %s exhausted (subnet /%d)", a.pool.String(), a.subnetLen) + }) + if err != nil { + return nil, err } + return result, nil +} - return nil, fmt.Errorf("pool %s exhausted (subnet /%d)", a.pool.String(), a.subnetLen) +// usedSubnets reads the pool's state directory and returns the set of +// subnet CIDR strings currently marked allocated. Callers must hold both mu +// and the pool's flock. +func (a *PoolAllocator) usedSubnets() (map[string]struct{}, error) { + entries, err := a.state.entries() + if err != nil { + return nil, err + } + used := make(map[string]struct{}, len(entries)) + for _, e := range entries { + used[desanitizeMarkerName(e.Name())] = struct{}{} + } + return used, nil } // Deallocate removes the allocation for the given subnet CIDR string. -// Silently ignores unknown subnets. +// Silently ignores unknown subnets. Serialized the same way as Allocate. +// Callers that only know the containerID (not the allocated value) should +// use DeallocateContainer instead. func (a *PoolAllocator) Deallocate(subnetCIDR string) { - a.allocations.Delete(subnetCIDR) + a.mu.Lock() + defer a.mu.Unlock() + + _ = a.state.withLock(func() error { + return os.Remove(filepath.Join(a.stateDir, sanitizePoolDirName(subnetCIDR))) + }) +} + +// LookupContainer reports the subnet CIDR, if any, allocated to +// containerID, without removing it — used by CHECK to confirm an +// allocation is still in place. Returns ("", false) if none is found. +func (a *PoolAllocator) LookupContainer(containerID string) (string, bool) { + a.mu.Lock() + defer a.mu.Unlock() + + var name string + var ok bool + _ = a.state.withLock(func() error { + name, ok = a.state.findContainerMarkerLocked(containerID) + return nil + }) + if !ok { + return "", false + } + return desanitizeMarkerName(name), true +} + +// DeallocateContainer removes the allocation, if any, held by containerID, +// without the caller needing to already know the allocated subnet — the +// on-disk marker file records which containerID holds each subnet, so this +// is a direct scan of this pool's own state, no external lookup (e.g. a +// CRD read) required. The scan and the removal happen under a single flock +// acquisition (via lockedState.withLock) so a concurrent process sharing +// this pool can never interleave between them. Returns the deallocated +// subnet CIDR and true if one was found; ("", false) otherwise. +func (a *PoolAllocator) DeallocateContainer(containerID string) (string, bool) { + a.mu.Lock() + defer a.mu.Unlock() + + var subnet string + var ok bool + _ = a.state.withLock(func() error { + name, found := a.state.findContainerMarkerLocked(containerID) + if !found { + return nil + } + subnet = desanitizeMarkerName(name) + ok = true + return os.Remove(filepath.Join(a.stateDir, name)) + }) + if !ok { + return "", false + } + return subnet, true } -// IsAllocated reports whether the given subnet CIDR string is actively allocated. +// IsAllocated reports whether the given subnet CIDR string is actively +// allocated, by checking for its on-disk marker file. func (a *PoolAllocator) IsAllocated(subnetCIDR string) bool { - _, ok := a.allocations.Load(subnetCIDR) - return ok + _, err := os.Stat(filepath.Join(a.stateDir, sanitizePoolDirName(subnetCIDR))) + return err == nil } // Gateway returns the gateway IP for the pool. @@ -209,3 +337,40 @@ func incSubnet(ip net.IP, subnetLen int) net.IP { } return ip } + +// desanitizeMarkerName reverses sanitizePoolDirName's "/" -> "-" replacement +// for a subnet CIDR marker filename. A CIDR string carries exactly one "/", +// so replacing the first "-" back is unambiguous (subnet strings otherwise +// contain only hex digits and ":"). +func desanitizeMarkerName(name string) string { + before, after, found := strings.Cut(name, "-") + if !found { + return name + } + return before + "/" + after +} + +// parseAllocatedSubnet parses a subnet CIDR string previously produced by +// Allocate (via (*net.IPNet).String()) back into a *net.IPNet, preserving +// its IP exactly as allocated. net.ParseCIDR is deliberately not used here: +// it returns the *masked* network address, which would zero out incSubnet's +// per-subnet counter byte — the byte incSubnet advances sits inside what a +// strict /subnetLen mask treats as host bits (see incSubnet's own doc +// comment), so re-masking a stored subnet string would silently collapse +// every allocated subnet in the pool back down to the same reserved-subnet +// address. +func parseAllocatedSubnet(cidr string) (*net.IPNet, error) { + ipStr, prefixStr, found := strings.Cut(cidr, "/") + if !found { + return nil, fmt.Errorf("missing '/' in subnet CIDR %q", cidr) + } + ip := net.ParseIP(ipStr) + if ip == nil { + return nil, fmt.Errorf("invalid IP in subnet CIDR %q", cidr) + } + prefixLen, err := strconv.Atoi(prefixStr) + if err != nil { + return nil, fmt.Errorf("invalid prefix length in subnet CIDR %q: %w", cidr, err) + } + return &net.IPNet{IP: ip.To16(), Mask: net.CIDRMask(prefixLen, ipv6Bits)}, nil +} diff --git a/internal/cni/ipam/ipam_test.go b/internal/cni/ipam/ipam_test.go index 3c23bfeb..2a588ab9 100644 --- a/internal/cni/ipam/ipam_test.go +++ b/internal/cni/ipam/ipam_test.go @@ -93,7 +93,7 @@ func TestNewPoolAllocator(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - pa, err := NewPoolAllocator(tt.poolCIDR, tt.gateway, tt.subnetLen) + pa, err := NewPoolAllocator(tt.poolCIDR, tt.gateway, tt.subnetLen, t.TempDir()) if tt.wantErr { if err == nil { t.Fatal("expected error, got nil") @@ -115,7 +115,7 @@ func TestNewPoolAllocator(t *testing.T) { } func TestPoolAllocatorAllocate(t *testing.T) { - pa, err := NewPoolAllocator(testPoolCIDR, testPoolGw, testSubnetLen) + pa, err := NewPoolAllocator(testPoolCIDR, testPoolGw, testSubnetLen, t.TempDir()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -160,8 +160,40 @@ func TestPoolAllocatorAllocate(t *testing.T) { } } +func TestPoolAllocatorAllocateIdempotentPerContainer(t *testing.T) { + pa, err := NewPoolAllocator(testPoolCIDR, testPoolGw, testSubnetLen, t.TempDir()) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + first, err := pa.Allocate("container-1") + if err != nil { + t.Fatalf("unexpected error on first allocation: %v", err) + } + + // A CNI runtime may retry ADD for the same container after a transient + // failure; a retry must get back the same subnet, not a fresh one (and + // must not leak the first attempt's marker file). + second, err := pa.Allocate("container-1") + if err != nil { + t.Fatalf("unexpected error on retry allocation: %v", err) + } + if second.String() != first.String() { + t.Errorf("retry Allocate() = %q, want %q (same as first allocation)", second.String(), first.String()) + } + + // A different container must still get a distinct subnet. + other, err := pa.Allocate("container-2") + if err != nil { + t.Fatalf("unexpected error on other container's allocation: %v", err) + } + if other.String() == first.String() { + t.Errorf("other container's Allocate() = %q, want distinct from %q", other.String(), first.String()) + } +} + func TestPoolAllocatorSkipsAllocatedSubnets(t *testing.T) { - pa, err := NewPoolAllocator(testPoolCIDR, testPoolGw, testSubnetLen) + pa, err := NewPoolAllocator(testPoolCIDR, testPoolGw, testSubnetLen, t.TempDir()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -187,7 +219,7 @@ func TestPoolAllocatorSkipsAllocatedSubnets(t *testing.T) { } func TestPoolAllocatorReservesGatewaySubnet(t *testing.T) { - pa, err := NewPoolAllocator(testPoolCIDR, testPoolGw, testSubnetLen) + pa, err := NewPoolAllocator(testPoolCIDR, testPoolGw, testSubnetLen, t.TempDir()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -216,7 +248,7 @@ func TestPoolAllocatorReservesGatewaySubnet(t *testing.T) { } func TestPoolAllocatorDeallocate(t *testing.T) { - pa, err := NewPoolAllocator(testPoolCIDR, testPoolGw, testSubnetLen) + pa, err := NewPoolAllocator(testPoolCIDR, testPoolGw, testSubnetLen, t.TempDir()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -241,7 +273,7 @@ func TestPoolAllocatorDeallocate(t *testing.T) { } func TestPoolAllocatorDeallocateUnknown(t *testing.T) { - pa, err := NewPoolAllocator(testPoolCIDR, testPoolGw, testSubnetLen) + pa, err := NewPoolAllocator(testPoolCIDR, testPoolGw, testSubnetLen, t.TempDir()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -250,8 +282,62 @@ func TestPoolAllocatorDeallocateUnknown(t *testing.T) { pa.Deallocate("fd00:dead::/80") } +func TestPoolAllocatorRejectsEmptyLockDir(t *testing.T) { + if _, err := NewPoolAllocator(testPoolCIDR, testPoolGw, testSubnetLen, ""); err == nil { + t.Fatal("expected error for empty lockDir, got nil") + } +} + +// TestPoolAllocatorPersistsAcrossInstances is the regression test for the +// bug this package's on-disk persistence fixes: each CNI ADD/DEL is a +// separate OS process, so a fresh *PoolAllocator constructed by DEL must +// still see the allocation ADD's own (now-exited) *PoolAllocator made. +func TestPoolAllocatorPersistsAcrossInstances(t *testing.T) { + lockDir := t.TempDir() + + addPA, err := NewPoolAllocator(testPoolCIDR, testPoolGw, testSubnetLen, lockDir) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + subnet, err := addPA.Allocate("container-a") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // A brand new instance, as DEL's own process would construct, must see + // the allocation the (conceptually already-exited) ADD process made. + delPA, err := NewPoolAllocator(testPoolCIDR, testPoolGw, testSubnetLen, lockDir) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !delPA.IsAllocated(subnet.String()) { + t.Fatalf("IsAllocated(%q) = false on a fresh instance, want true (persisted)", subnet) + } + + gotSubnet, ok := delPA.DeallocateContainer("container-a") + if !ok { + t.Fatal("DeallocateContainer(\"container-a\") = false, want true") + } + if gotSubnet != subnet.String() { + t.Errorf("DeallocateContainer returned %q, want %q", gotSubnet, subnet.String()) + } + if delPA.IsAllocated(subnet.String()) { + t.Error("IsAllocated after DeallocateContainer = true, want false") + } +} + +func TestPoolAllocatorDeallocateContainerUnknown(t *testing.T) { + pa, err := NewPoolAllocator(testPoolCIDR, testPoolGw, testSubnetLen, t.TempDir()) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if _, ok := pa.DeallocateContainer("no-such-container"); ok { + t.Error("DeallocateContainer for unknown container = true, want false") + } +} + func TestPoolAllocatorIsAllocated(t *testing.T) { - pa, err := NewPoolAllocator(testPoolCIDR, testPoolGw, testSubnetLen) + pa, err := NewPoolAllocator(testPoolCIDR, testPoolGw, testSubnetLen, t.TempDir()) if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/internal/cni/ipam/ipv4.go b/internal/cni/ipam/ipv4.go index 9d8d1093..c439ae56 100644 --- a/internal/cni/ipam/ipv4.go +++ b/internal/cni/ipam/ipv4.go @@ -18,12 +18,6 @@ const ( // ipv4Bits is the number of bits in an IPv4 address. ipv4Bits = 32 - // DefaultIPv4LockDir is the well-known parent directory for the - // node-local on-disk lock and allocation state IPv4PoolAllocator uses to - // stay correct across separate CNI plugin invocations (each ADD/DEL is - // its own OS process) sharing the same site-wide IPv4 pool. - DefaultIPv4LockDir = "/var/lib/cni/galactic-ipv4" - // ipv4LockFileName is the flock target within each pool's state // directory; every other entry in that directory is an allocation // marker file named after the address it reserves. @@ -49,13 +43,14 @@ type IPv4PoolAllocator struct { gateway net.IP // gateway IP address mu sync.Mutex // serializes Allocate/Deallocate within this process stateDir string // directory holding the lock file and one allocation marker file per allocated address + state lockedState } // NewIPv4PoolAllocator creates a new IPv4 pool allocator from a CIDR pool and // an optional gateway address. The pool must be an IPv4 prefix. If gateway is // empty, the network address plus 1 is used as the gateway. lockDir is the // parent directory for this pool's on-disk lock and allocation state (see -// DefaultIPv4LockDir for the production path); it must not be empty, and a +// DefaultLockDir for the production path); it must not be empty, and a // pool-scoped subdirectory under it is created if it doesn't already exist. func NewIPv4PoolAllocator(poolCIDR, gateway, lockDir string) (*IPv4PoolAllocator, error) { _, pool, err := net.ParseCIDR(poolCIDR) @@ -98,61 +93,68 @@ func NewIPv4PoolAllocator(poolCIDR, gateway, lockDir string) (*IPv4PoolAllocator return nil, fmt.Errorf("create pool state dir %q: %w", stateDir, err) } a.stateDir = stateDir + a.state = lockedState{stateDir: stateDir, lockFileName: ipv4LockFileName} return a, nil } // Allocate assigns the next available IPv4 /32 address from the pool for the // given container ID, skipping reserved addresses (the network address, the -// gateway, the second-to-last address, and the last address of the pool). -// Returns an error if the pool is exhausted. The read-modify-write against -// the on-disk allocation state is serialized both within this process (via -// mu) and across processes sharing the same pool (via a flock on the pool's -// lock file), so concurrent ADDs from different VPCs on the same node never -// return the same address. +// gateway, the second-to-last address, and the last address of the pool). If +// containerID already holds an allocation in this pool, that same address is +// returned rather than a fresh one being handed out — see +// PoolAllocator.Allocate's doc comment (IPv6) for why this idempotency check +// matters for CNI ADD retries. Returns an error if the pool is exhausted. +// The read-modify-write against the on-disk allocation state is serialized +// both within this process (via mu) and across processes sharing the same +// pool (via a flock on the pool's lock file), so concurrent ADDs from +// different VPCs on the same node never return the same address. func (a *IPv4PoolAllocator) Allocate(containerID string) (net.IP, error) { a.mu.Lock() defer a.mu.Unlock() - lock, err := newFileLock(filepath.Join(a.stateDir, ipv4LockFileName)) - if err != nil { - return nil, fmt.Errorf("open lock for pool %s: %w", a.pool.String(), err) - } - defer func() { _ = lock.close() }() + var result net.IP + err := a.state.withLock(func() error { + if addrStr, ok := a.state.findContainerMarkerLocked(containerID); ok { + result = net.ParseIP(addrStr).To4() + return nil + } - if err := lock.lock(); err != nil { - return nil, fmt.Errorf("lock pool %s: %w", a.pool.String(), err) - } + used, err := a.usedAddresses() + if err != nil { + return err + } - used, err := a.usedAddresses() - if err != nil { - return nil, err - } + reserved := a.reservedAddresses() - reserved := a.reservedAddresses() + ones, bits := a.pool.Mask.Size() + total := uint64(1) << uint(bits-ones) - ones, bits := a.pool.Mask.Size() - total := uint64(1) << uint(bits-ones) + for i := range total { + addr := offsetIP4(a.pool.IP, i) + addrStr := addr.String() - for i := range total { - addr := offsetIP4(a.pool.IP, i) - addrStr := addr.String() + if _, ok := reserved[addrStr]; ok { + continue + } + if _, ok := used[addrStr]; ok { + continue + } - if _, ok := reserved[addrStr]; ok { - continue - } - if _, ok := used[addrStr]; ok { - continue + markerPath := filepath.Join(a.stateDir, addrStr) + if err := os.WriteFile(markerPath, []byte(containerID), 0o600); err != nil { + return fmt.Errorf("write allocation marker %q: %w", markerPath, err) + } + result = addr + return nil } - markerPath := filepath.Join(a.stateDir, addrStr) - if err := os.WriteFile(markerPath, []byte(containerID), 0o600); err != nil { - return nil, fmt.Errorf("write allocation marker %q: %w", markerPath, err) - } - return addr, nil + return fmt.Errorf("pool %s exhausted", a.pool.String()) + }) + if err != nil { + return nil, err } - - return nil, fmt.Errorf("pool %s exhausted", a.pool.String()) + return result, nil } // Deallocate removes the allocation for the given address string. Silently @@ -161,17 +163,52 @@ func (a *IPv4PoolAllocator) Deallocate(addr string) { a.mu.Lock() defer a.mu.Unlock() - lock, err := newFileLock(filepath.Join(a.stateDir, ipv4LockFileName)) - if err != nil { - return - } - defer func() { _ = lock.close() }() + _ = a.state.withLock(func() error { + return os.Remove(filepath.Join(a.stateDir, addr)) + }) +} - if err := lock.lock(); err != nil { - return - } +// LookupContainer reports the address, if any, allocated to containerID, +// without removing it — used by CHECK to confirm an allocation is still in +// place. Returns ("", false) if none is found. +func (a *IPv4PoolAllocator) LookupContainer(containerID string) (string, bool) { + a.mu.Lock() + defer a.mu.Unlock() - _ = os.Remove(filepath.Join(a.stateDir, addr)) + var addr string + var ok bool + _ = a.state.withLock(func() error { + addr, ok = a.state.findContainerMarkerLocked(containerID) + return nil + }) + return addr, ok +} + +// DeallocateContainer removes the allocation, if any, held by containerID, +// without the caller needing to already know the allocated address — +// mirrors PoolAllocator.DeallocateContainer (IPv6); see its doc comment for +// why the scan and the removal must happen under a single flock acquisition. +// Returns the deallocated address and true if one was found; ("", false) +// otherwise. +func (a *IPv4PoolAllocator) DeallocateContainer(containerID string) (string, bool) { + a.mu.Lock() + defer a.mu.Unlock() + + var addr string + var ok bool + _ = a.state.withLock(func() error { + found, wasFound := a.state.findContainerMarkerLocked(containerID) + if !wasFound { + return nil + } + addr = found + ok = true + return os.Remove(filepath.Join(a.stateDir, found)) + }) + if !ok { + return "", false + } + return addr, true } // IsAllocated reports whether the given address string is actively @@ -185,16 +222,13 @@ func (a *IPv4PoolAllocator) IsAllocated(addr string) bool { // addresses currently marked allocated by any process sharing this pool. // Callers must hold both mu and the pool's flock. func (a *IPv4PoolAllocator) usedAddresses() (map[string]struct{}, error) { - entries, err := os.ReadDir(a.stateDir) + entries, err := a.state.entries() if err != nil { - return nil, fmt.Errorf("read pool state dir %q: %w", a.stateDir, err) + return nil, err } used := make(map[string]struct{}, len(entries)) for _, e := range entries { - if e.Name() == ipv4LockFileName { - continue - } used[e.Name()] = struct{}{} } return used, nil diff --git a/internal/cni/ipam/ipv4_test.go b/internal/cni/ipam/ipv4_test.go index 5585f2be..4d357402 100644 --- a/internal/cni/ipam/ipv4_test.go +++ b/internal/cni/ipam/ipv4_test.go @@ -4,7 +4,10 @@ package ipam -import "testing" +import ( + "fmt" + "testing" +) const ( // testIPv4PoolCIDR is a /29 (8 addresses) so tests can exercise @@ -117,6 +120,34 @@ func TestIPv4PoolAllocatorAllocate(t *testing.T) { } } +func TestIPv4PoolAllocatorAllocateIdempotentPerContainer(t *testing.T) { + a, err := NewIPv4PoolAllocator(testIPv4PoolCIDR, testIPv4Gw, t.TempDir()) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + first, err := a.Allocate("container-1") + if err != nil { + t.Fatalf("unexpected error on first allocation: %v", err) + } + + second, err := a.Allocate("container-1") + if err != nil { + t.Fatalf("unexpected error on retry allocation: %v", err) + } + if !second.Equal(first) { + t.Errorf("retry Allocate() = %q, want %q (same as first allocation)", second, first) + } + + other, err := a.Allocate("container-2") + if err != nil { + t.Fatalf("unexpected error on other container's allocation: %v", err) + } + if other.Equal(first) { + t.Errorf("other container's Allocate() = %q, want distinct from %q", other, first) + } +} + func TestIPv4PoolAllocatorSkipsReservedAddresses(t *testing.T) { a, err := NewIPv4PoolAllocator(testIPv4PoolCIDR, testIPv4Gw, t.TempDir()) if err != nil { @@ -150,9 +181,13 @@ func TestIPv4PoolAllocatorExhaustion(t *testing.T) { } // The /29 has exactly 4 usable addresses (.2-.5); the 5th allocation - // must fail with an exhaustion error. + // must fail with an exhaustion error. Each iteration uses a distinct + // container ID -- Allocate is idempotent per containerID (a CNI ADD + // retry must get back the same address, not a fresh one), so reusing + // one ID here would only ever consume a single address. for i := range 4 { - if _, err := a.Allocate("container"); err != nil { + containerID := fmt.Sprintf("container-%d", i) + if _, err := a.Allocate(containerID); err != nil { t.Fatalf("unexpected error on allocation %d: %v", i, err) } } @@ -184,6 +219,46 @@ func TestIPv4PoolAllocatorDeallocate(t *testing.T) { } } +func TestIPv4PoolAllocatorDeallocateContainer(t *testing.T) { + lockDir := t.TempDir() + + addA, err := NewIPv4PoolAllocator(testIPv4PoolCIDR, testIPv4Gw, lockDir) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + addr, err := addA.Allocate("container-a") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // A fresh instance, as DEL's own process would construct, must see the + // allocation the (conceptually already-exited) ADD process made. + delA, err := NewIPv4PoolAllocator(testIPv4PoolCIDR, testIPv4Gw, lockDir) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + gotAddr, ok := delA.DeallocateContainer("container-a") + if !ok { + t.Fatal("DeallocateContainer(\"container-a\") = false, want true") + } + if gotAddr != addr.String() { + t.Errorf("DeallocateContainer returned %q, want %q", gotAddr, addr.String()) + } + if delA.IsAllocated(addr.String()) { + t.Error("IsAllocated after DeallocateContainer = true, want false") + } +} + +func TestIPv4PoolAllocatorDeallocateContainerUnknown(t *testing.T) { + a, err := NewIPv4PoolAllocator(testIPv4PoolCIDR, testIPv4Gw, t.TempDir()) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if _, ok := a.DeallocateContainer("no-such-container"); ok { + t.Error("DeallocateContainer for unknown container = true, want false") + } +} + func TestIPv4PoolAllocatorDeallocateUnknown(t *testing.T) { a, err := NewIPv4PoolAllocator(testIPv4PoolCIDR, testIPv4Gw, t.TempDir()) if err != nil { diff --git a/internal/cni/ipam/lockedstate.go b/internal/cni/ipam/lockedstate.go new file mode 100644 index 00000000..a4e21b74 --- /dev/null +++ b/internal/cni/ipam/lockedstate.go @@ -0,0 +1,79 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package ipam + +import ( + "fmt" + "os" + "path/filepath" +) + +// lockedState wraps the on-disk state directory and cross-process flock +// pattern shared by PoolAllocator (IPv6) and IPv4PoolAllocator: a directory +// holding a "lock" file plus one allocation marker file per allocated +// subnet/address, guarded by a flock so separate CNI plugin invocations +// (each ADD/DEL/CHECK is its own OS process) never race against each other +// over the same pool. +type lockedState struct { + stateDir string + lockFileName string +} + +// withLock acquires the pool's cross-process flock, runs fn, and releases +// the lock unconditionally before returning. Every read-modify-write against +// stateDir must go through this — in particular, a scan (to find which +// marker file, if any, belongs to a given containerID) and any removal that +// follows it must happen inside the *same* withLock call. Splitting them +// across two separate lock/unlock cycles, as an earlier version of +// findContainerMarker plus its callers did, leaves a window between the scan +// and the removal where a concurrent process sharing this pool can allocate +// or deallocate the very entry being acted on. +func (s lockedState) withLock(fn func() error) error { + lock, err := newFileLock(filepath.Join(s.stateDir, s.lockFileName)) + if err != nil { + return fmt.Errorf("open lock for %q: %w", s.stateDir, err) + } + defer func() { _ = lock.close() }() + + if err := lock.lock(); err != nil { + return fmt.Errorf("lock %q: %w", s.stateDir, err) + } + return fn() +} + +// entries reads stateDir and returns every marker filename except the lock +// file itself. Callers must already hold the flock (call from inside +// withLock). +func (s lockedState) entries() ([]os.DirEntry, error) { + all, err := os.ReadDir(s.stateDir) + if err != nil { + return nil, fmt.Errorf("read pool state dir %q: %w", s.stateDir, err) + } + markers := make([]os.DirEntry, 0, len(all)) + for _, e := range all { + if e.Name() == s.lockFileName { + continue + } + markers = append(markers, e) + } + return markers, nil +} + +// findContainerMarkerLocked scans stateDir for the marker file whose +// content matches containerID, returning its filename. Callers must already +// hold the flock (call from inside withLock). +func (s lockedState) findContainerMarkerLocked(containerID string) (string, bool) { + entries, err := s.entries() + if err != nil { + return "", false + } + for _, e := range entries { + content, err := os.ReadFile(filepath.Join(s.stateDir, e.Name())) + if err == nil && string(content) == containerID { + return e.Name(), true + } + } + return "", false +} diff --git a/internal/cni/ops_add.go b/internal/cni/ops_add.go index 50d8a6ce..7a5fc761 100644 --- a/internal/cni/ops_add.go +++ b/internal/cni/ops_add.go @@ -64,6 +64,16 @@ func cmdAdd(args *skel.CmdArgs) (err error) { vpcAttachment: pluginConf.VPCAttachment, namespace: namespace, } + // Record IPAM delegation intent up front, before configureIPAM (called + // from buildVethResult below) ever runs — see resourceTracker's + // ipamDelegated doc comment for why rollback needs this set + // unconditionally on "ipam" block presence, not just after a + // successful ExecAdd. + if pluginConf.IPAM != nil { + tracker.ipamDelegated = true + tracker.ipamType = pluginConf.IPAM.Type + tracker.ipamStdin = args.StdinData + } // Selective rollback: clean up only resources that were created. // We need a context for k8s operations in rollback; the k8s client diff --git a/internal/cni/ops_check.go b/internal/cni/ops_check.go index adebc672..b325baec 100644 --- a/internal/cni/ops_check.go +++ b/internal/cni/ops_check.go @@ -18,6 +18,7 @@ import ( "github.com/containernetworking/cni/pkg/skel" "github.com/containernetworking/cni/pkg/types" type100 "github.com/containernetworking/cni/pkg/types/100" + "github.com/containernetworking/plugins/pkg/ipam" "github.com/containernetworking/plugins/pkg/ns" "github.com/vishvananda/netlink" "k8s.io/client-go/rest" @@ -64,6 +65,14 @@ func cmdCheck(args *skel.CmdArgs) error { } } + // Delegate CHECK to the IPAM plugin so a lost or corrupted allocation + // marker file is caught here too, not just at ADD/DEL time. + if pluginConf.IPAM != nil { + if err := ipam.ExecCheck(pluginConf.IPAM.Type, args.StdinData); err != nil { + errs = append(errs, fmt.Errorf("IPAM CHECK: %w", err)) + } + } + if len(errs) > 0 { err := fmt.Errorf("CHECK failed: %w", errors.Join(errs...)) slog.Error("CHECK: failed", "err", err, "containerID", args.ContainerID, diff --git a/internal/cni/ops_del.go b/internal/cni/ops_del.go index 17d719c0..054fafbf 100644 --- a/internal/cni/ops_del.go +++ b/internal/cni/ops_del.go @@ -10,8 +10,7 @@ import ( "github.com/containernetworking/cni/pkg/skel" "github.com/containernetworking/cni/pkg/types" type100 "github.com/containernetworking/cni/pkg/types/100" - - "go.datum.net/galactic/internal/cniipam" + "github.com/containernetworking/plugins/pkg/ipam" ) func cmdDel(args *skel.CmdArgs) error { @@ -32,13 +31,13 @@ func cmdDel(args *skel.CmdArgs) error { vpc, vpcAtt := pluginConf.VPC, pluginConf.VPCAttachment // Deallocate the pod's IPAM subnet. This is pod-specific and safe to - // release immediately. - cfg := allocConfig(pluginConf) - if cniipam.WantsIPAM(cfg) { - if k8s, err := newK8sClient(); err == nil { - cniipam.Deallocate(args, cfg, k8s) - } else { - slog.Warn("DEL: failed to create k8s client, skipping IPAM deallocation", "err", err, + // release immediately. Delegating at all (or not) is entirely + // pluginConf.IPAM's own presence — no k8s client needed here at all + // now that galactic-ipam's own DEL looks its allocation up locally + // (see internal/cniipam's doc comment). + if pluginConf.IPAM != nil { + if err := ipam.ExecDel(pluginConf.IPAM.Type, args.StdinData); err != nil { + slog.Warn("DEL: IPAM delegation failed, allocation may not have been released", "err", err, "containerID", args.ContainerID) } } diff --git a/internal/cni/resource.go b/internal/cni/resource.go index 57cc430c..710bf1a0 100644 --- a/internal/cni/resource.go +++ b/internal/cni/resource.go @@ -9,6 +9,7 @@ import ( "fmt" "log/slog" + "github.com/containernetworking/plugins/pkg/ipam" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" @@ -67,6 +68,21 @@ type resourceTracker struct { ebpfRegistered bool ebpfBlock uint64 ebpfArgument uint16 + + // ipamDelegated, ipamType, and ipamStdin record enough to release the + // IPAM allocation during rollback. Set as soon as pluginConf.IPAM != nil + // is known (before configureIPAM/ipam.ExecAdd is even attempted) rather + // than only after a successful ExecAdd: ipam.ExecDel is idempotent per + // the CNI IPAM delegation protocol (galactic-ipam's own cmdDel no-ops + // when it finds no allocation for the containerID), so calling it + // unconditionally whenever an "ipam" block was configured is safe and + // covers every failure path, including ones where ExecAdd itself never + // ran. Without this, a failed ADD that got past IPAM permanently burns + // an address/subnet out of the pool — the on-disk marker file has no + // implicit teardown the way the old in-memory-only allocator did. + ipamDelegated bool + ipamType string + ipamStdin []byte } // cleanup rolls back all tracked resources in reverse creation order. @@ -124,7 +140,18 @@ func (rt *resourceTracker) cleanup(ctx context.Context) { } } - // 4. Delete host veth + // 4. Release the IPAM allocation (if pluginConf carried an "ipam" block + // at all — see the ipamDelegated field doc comment for why this fires + // unconditionally on that alone, not just after a confirmed ExecAdd). + if rt.ipamDelegated { + if err := ipam.ExecDel(rt.ipamType, rt.ipamStdin); err != nil { + slog.Error("Rollback: failed to release IPAM allocation", "err", err, "ipamType", rt.ipamType) + } else { + slog.Debug("Rollback: released IPAM allocation", "ipamType", rt.ipamType) + } + } + + // 5. Delete host veth if err := veth.Delete(rt.vpc, rt.vpcAttachment); err != nil { slog.Error("Rollback: failed to delete veth", "err", err, "vpc", rt.vpc, "vpcAttachment", rt.vpcAttachment) @@ -132,7 +159,7 @@ func (rt *resourceTracker) cleanup(ctx context.Context) { slog.Debug("Rollback: deleted veth", "vpc", rt.vpc, "vpcAttachment", rt.vpcAttachment) } - // 5. Delete VRF (flushes all routes, removes VRF interface) + // 6. Delete VRF (flushes all routes, removes VRF interface) if err := vrf.Delete(rt.vpc, rt.vpcAttachment); err != nil { slog.Error("Rollback: failed to delete VRF", "err", err, "vpc", rt.vpc, "vpcAttachment", rt.vpcAttachment) diff --git a/internal/cni/result.go b/internal/cni/result.go index 23a80968..46f5eb60 100644 --- a/internal/cni/result.go +++ b/internal/cni/result.go @@ -11,6 +11,7 @@ import ( "github.com/containernetworking/cni/pkg/skel" "github.com/containernetworking/cni/pkg/types" type100 "github.com/containernetworking/cni/pkg/types/100" + "github.com/containernetworking/plugins/pkg/ipam" "github.com/vishvananda/netlink" "go.datum.net/galactic/internal/cniipam" @@ -103,10 +104,12 @@ func buildVethResult( } // Configure IP address on the guest interface inside the container netns. - cfg := allocConfig(pluginConf) + // Delegating at all is this plugin's own call, decided solely by "ipam" + // block presence — no config field or env var elsewhere can override + // that (see internal/cniipam's doc comment for the explicit contract). var ipamResult *cniipam.IPAMResult - if cniipam.WantsIPAM(cfg) { - result, err := configureIPAM(args, cfg, args.IfName) + if pluginConf.IPAM != nil { + result, err := configureIPAM(args, pluginConf, args.IfName) if err != nil { return nil, nil, fmt.Errorf("configure IPAM: %w", err) } @@ -130,15 +133,22 @@ func buildVethResult( return ipamResult, guestHWAddr, nil } -// configureIPAM allocates addresses and configures the guest interface inside -// the container network namespace with both families (when dual-stack). -func configureIPAM(args *skel.CmdArgs, cfg cniipam.AllocConfig, guestName string) (*cniipam.IPAMResult, error) { - ipamResult, err := cniipam.Allocate(args, cfg) +// configureIPAM delegates IPAM allocation to whatever binary pluginConf's +// own "ipam.type" names (per the CNI IPAM delegation protocol — see +// github.com/containernetworking/plugins/pkg/ipam.ExecAdd), then applies +// the returned addresses to the guest interface inside the container +// network namespace with both families (when dual-stack). args.StdinData +// is passed straight through as the delegate's own netconf: it already +// contains the "ipam" block (plus everything else in this plugin's own +// config, which the delegate simply ignores). +func configureIPAM(args *skel.CmdArgs, pluginConf *PluginConf, guestName string) (*cniipam.IPAMResult, error) { + cniResult, err := ipam.ExecAdd(pluginConf.IPAM.Type, args.StdinData) if err != nil { - return nil, err + return nil, fmt.Errorf("delegate to %s ADD: %w", pluginConf.IPAM.Type, err) } - if ipamResult == nil { - return nil, nil + ipamResult, err := cniipam.ResultToIPAMResult(cniResult) + if err != nil { + return nil, fmt.Errorf("convert IPAM result: %w", err) } var ipv4Net *net.IPNet diff --git a/internal/cni/types.go b/internal/cni/types.go index 9f33cf6f..2857b017 100644 --- a/internal/cni/types.go +++ b/internal/cni/types.go @@ -21,21 +21,19 @@ type Termination struct { // PluginConf is the CNI plugin configuration passed via stdin on each // invocation of galactic-cni, the veth master plugin. // -// IPv6Subnet, IPv4Subnet, and AddressFamilies feed the dual-stack IPAM -// allocators (internal/cni/ipam, IPv4PoolAllocator/DualStackAllocator); as of -// this change allocateIPAM does not yet consume them, and format/requiredness -// validation in parseConf lands in a later phase. +// IPAM addressing fields (ipv6_subnet, ipv4_subnet, address_families, +// static_ip) live entirely inside the "ipam" block now — see +// go.datum.net/galactic/internal/cniipam's doc comment for the explicit +// delegation contract: this struct only decides *whether* to delegate +// (IPAM != nil), never anything about how allocation itself works. type PluginConf struct { types.PluginConf - VPC string `json:"vpc"` - VPCAttachment string `json:"vpcattachment"` - MTU int `json:"mtu,omitempty"` - Terminations []Termination `json:"terminations,omitempty"` - IPAM *cniipam.IPAM `json:"ipam"` - Namespace string `json:"namespace,omitempty"` - IPv6Subnet string `json:"ipv6_subnet,omitempty"` // region IPv6 pool CIDR; endpoints alloc /96 - IPv4Subnet string `json:"ipv4_subnet,omitempty"` // optional site IPv4 pool CIDR; endpoints alloc /32 - AddressFamilies []string `json:"address_families,omitempty"` // families to allocate; default ["ipv6"] + VPC string `json:"vpc"` + VPCAttachment string `json:"vpcattachment"` + MTU int `json:"mtu,omitempty"` + Terminations []Termination `json:"terminations,omitempty"` + IPAM *cniipam.IPAM `json:"ipam"` + Namespace string `json:"namespace,omitempty"` } // HostConf holds node-local settings read from /etc/cni/net.d/10-galactic.conflist. @@ -47,19 +45,3 @@ type HostDevicePluginConf struct { types.PluginConf Device string `json:"device"` } - -// allocConfig adapts pluginConf's fields into cniipam.AllocConfig, the shape -// internal/cniipam actually needs — see that package's doc comment for why -// it takes its own minimal config shape rather than this package's full -// PluginConf. -func allocConfig(pluginConf *PluginConf) cniipam.AllocConfig { - return cniipam.AllocConfig{ - VPC: pluginConf.VPC, - VPCAttachment: pluginConf.VPCAttachment, - Namespace: pluginConf.Namespace, - IPAM: pluginConf.IPAM, - IPv6Subnet: pluginConf.IPv6Subnet, - IPv4Subnet: pluginConf.IPv4Subnet, - AddressFamilies: pluginConf.AddressFamilies, - } -} diff --git a/internal/cniipam/allocate.go b/internal/cniipam/allocate.go new file mode 100644 index 00000000..0f4d68e2 --- /dev/null +++ b/internal/cniipam/allocate.go @@ -0,0 +1,177 @@ +// Copyright 2025 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cniipam + +import ( + "errors" + "fmt" + "log/slog" + "net" + + "github.com/containernetworking/cni/pkg/skel" + + "go.datum.net/galactic/internal/cni/ipam" +) + +// localIPAMDefaultPool is the IPv6 CIDR pool used when local IPAM is enabled +// but neither static_ip nor ipv6_subnet/ipv4_subnet is set in the ipam +// block. Allocations from it use ipam.DefaultSubnetLen (/96). +const localIPAMDefaultPool = "fd00:10:ff01::/64" + +// lockDir is the on-disk allocation-state root both PoolAllocator and +// IPv4PoolAllocator persist to. Overridable in tests so unit tests never +// touch the real production path. +var lockDir = ipam.DefaultLockDir + +// allocate allocates addresses for the given container according to conf's +// mode: presence of StaticIP selects the static path; otherwise the pool +// path (either family alone, or both). +func allocate(args *skel.CmdArgs, conf *IPAM) (*IPAMResult, error) { + if conf.StaticIP != "" { + return allocateStatic(args, conf) + } + return allocatePool(args, conf) +} + +// allocateStatic validates and returns the pre-assigned static IPv6 address +// from static_ip. No IPv4 address is ever allocated for static IPAM — it is +// a single fixed address, not a dual-stack pool. +func allocateStatic(args *skel.CmdArgs, conf *IPAM) (*IPAMResult, error) { + alloc := ipam.NewStaticAllocator() + allocIP, err := alloc.Allocate(args.ContainerID, conf.StaticIP) + if err != nil { + return nil, fmt.Errorf("allocate static IP: %w", err) + } + subnet := &net.IPNet{ + IP: allocIP, + Mask: net.CIDRMask(64, 128), + } + slog.Debug("IPAM: allocated static", "containerID", args.ContainerID, "subnet", subnet) + return &IPAMResult{IPv6Subnet: subnet}, nil +} + +// allocatePool allocates a dual-stack, IPv6-only, or IPv4-only pool-based +// endpoint address for the given container, via ipam.DualStackAllocator. +// IPv6Subnet and IPv4Subnet each independently supply a pool CIDR for their +// family; at least one must be set (falling back to localIPAMDefaultPool +// for IPv6 when GALACTIC_IPAM_ENABLE_LOCAL_IPAM is set and both are unset — +// see parseConf, which fills that default in before this ever runs). +func allocatePool(args *skel.CmdArgs, conf *IPAM) (*IPAMResult, error) { + if conf.IPv6Subnet == "" && conf.IPv4Subnet == "" { + return nil, errors.New("ipam.ipv6_subnet or ipam.ipv4_subnet is required (or enable GALACTIC_IPAM_ENABLE_LOCAL_IPAM)") + } + + alloc, err := ipam.NewDualStackAllocator(conf.IPv6Subnet, "", conf.IPv4Subnet, "", lockDir) + if err != nil { + return nil, fmt.Errorf("create dual-stack allocator: %w", err) + } + + res, err := alloc.Allocate(args.ContainerID) + if err != nil { + return nil, fmt.Errorf("allocate dual-stack addresses: %w", err) + } + + var routes []*net.IPNet + if res.IPv6Subnet != nil { + routes = append(routes, &net.IPNet{IP: net.IPv6zero, Mask: net.CIDRMask(0, 128)}) + } + if res.IPv4Address != nil { + routes = append(routes, &net.IPNet{IP: net.IPv4zero, Mask: net.CIDRMask(0, 32)}) + } + + slog.Debug("IPAM: allocated", "containerID", args.ContainerID, + "ipv6Subnet", res.IPv6Subnet, "ipv6Gateway", res.IPv6Gateway, + "ipv4Address", res.IPv4Address, "ipv4Gateway", res.IPv4Gateway) + + return &IPAMResult{ + IPv6Subnet: res.IPv6Subnet, + IPv6Gateway: res.IPv6Gateway, + IPv4Address: res.IPv4Address, + IPv4Gateway: res.IPv4Gateway, + Routes: routes, + }, nil +} + +// effectiveIPv6Subnet returns conf.IPv6Subnet if either family's subnet was +// ever explicitly set. Otherwise — neither ipv6_subnet nor ipv4_subnet is +// set — the only pool an allocation could possibly have come from is +// parseConf's default-filler pool, so that's returned directly instead of +// re-deriving it from GALACTIC_IPAM_ENABLE_LOCAL_IPAM. deallocate/ +// checkAllocation must not depend on that env var still agreeing at DEL/ +// CHECK time with whatever it resolved to at ADD time: if it flips in +// between, re-checking it here would see an empty subnet and silently skip +// cleanup/verification, leaking the allocation instead of releasing it. +func effectiveIPv6Subnet(conf *IPAM) string { + if conf.IPv6Subnet != "" || conf.IPv4Subnet != "" { + return conf.IPv6Subnet + } + return localIPAMDefaultPool +} + +// deallocate releases whatever allocation containerID holds against conf's +// pools — entirely local: each family's own on-disk marker file is looked +// up directly by containerID (internal/cni/ipam's DeallocateContainer), no +// external state (a CRD read, a Kubernetes client) required. A missing +// allocation for one family (e.g. a v6-only pod, or a partial ADD failure +// that never reached IPv4 allocation) does not prevent cleanup of the +// other — each call is independent and silently no-ops if nothing is +// found. +func deallocate(containerID string, conf *IPAM) { + if conf.StaticIP != "" { + // Static allocations don't need deallocation. + return + } + + if ipv6Subnet := effectiveIPv6Subnet(conf); ipv6Subnet != "" { + pa, err := ipam.NewPoolAllocator(ipv6Subnet, "", 0, lockDir) + if err != nil { + slog.Warn("IPAM: failed to build IPv6 pool allocator for deallocation, skipping", "err", err, + "containerID", containerID) + } else if subnet, ok := pa.DeallocateContainer(containerID); ok { + slog.Debug("IPAM: deallocated IPv6", "containerID", containerID, "subnet", subnet) + } + } + + if conf.IPv4Subnet != "" { + pa, err := ipam.NewIPv4PoolAllocator(conf.IPv4Subnet, "", lockDir) + if err != nil { + slog.Warn("IPAM: failed to build IPv4 pool allocator for deallocation, skipping", "err", err, + "containerID", containerID) + } else if addr, ok := pa.DeallocateContainer(containerID); ok { + slog.Debug("IPAM: deallocated IPv4", "containerID", containerID, "address", addr) + } + } +} + +// checkAllocation verifies that containerID still holds an allocation +// against every family conf configures — used by CHECK. A static +// allocation has nothing persisted to check (it's validated once, at ADD, +// and never stored), so it always passes. Returns one error per +// missing/unreachable family; nil means every configured family checked +// out. +func checkAllocation(containerID string, conf *IPAM) []error { + if conf.StaticIP != "" { + return nil + } + + var errs []error + if ipv6Subnet := effectiveIPv6Subnet(conf); ipv6Subnet != "" { + pa, err := ipam.NewPoolAllocator(ipv6Subnet, "", 0, lockDir) + if err != nil { + errs = append(errs, fmt.Errorf("open IPv6 pool: %w", err)) + } else if _, ok := pa.LookupContainer(containerID); !ok { + errs = append(errs, errors.New("no IPv6 allocation found for container")) + } + } + if conf.IPv4Subnet != "" { + pa, err := ipam.NewIPv4PoolAllocator(conf.IPv4Subnet, "", lockDir) + if err != nil { + errs = append(errs, fmt.Errorf("open IPv4 pool: %w", err)) + } else if _, ok := pa.LookupContainer(containerID); !ok { + errs = append(errs, errors.New("no IPv4 allocation found for container")) + } + } + return errs +} diff --git a/internal/cniipam/allocate_test.go b/internal/cniipam/allocate_test.go new file mode 100644 index 00000000..298fe3aa --- /dev/null +++ b/internal/cniipam/allocate_test.go @@ -0,0 +1,129 @@ +// Copyright 2025 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cniipam + +import ( + "net" + "testing" + + "github.com/containernetworking/cni/pkg/skel" +) + +const ( + testContainerID = "test-container" + testIPv6PoolDefault = "fd00:10:ff01::/64" + testIPv4Subnet = "10.128.0.0/20" +) + +func withTempLockDir(t *testing.T) { + t.Helper() + original := lockDir + lockDir = t.TempDir() + t.Cleanup(func() { lockDir = original }) +} + +func TestAllocateStatic(t *testing.T) { + args := &skel.CmdArgs{ContainerID: testContainerID} + conf := &IPAM{Type: testIPAMType, StaticIP: "fd00:10:ff01::1234"} + + res, err := allocate(args, conf) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if res.IPv6Subnet == nil || !res.IPv6Subnet.IP.Equal(net.ParseIP("fd00:10:ff01::1234")) { + t.Errorf("IPv6Subnet = %v, want fd00:10:ff01::1234", res.IPv6Subnet) + } + if res.IPv4Address != nil { + t.Errorf("IPv4Address = %v, want nil for static IPAM", res.IPv4Address) + } +} + +func TestAllocatePoolDualStack(t *testing.T) { + withTempLockDir(t) + args := &skel.CmdArgs{ContainerID: testContainerID} + conf := &IPAM{Type: testIPAMType, IPv6Subnet: testIPv6PoolDefault, IPv4Subnet: testIPv4Subnet} + + res, err := allocate(args, conf) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if res.IPv6Subnet == nil { + t.Error("IPv6Subnet = nil, want an allocated /96") + } + if res.IPv4Address == nil { + t.Fatal("IPv4Address = nil, want an allocated /32") + } + if len(res.Routes) != 2 { + t.Errorf("Routes = %v, want one default route per family", res.Routes) + } +} + +func TestAllocatePoolMissingBothSubnetsErrors(t *testing.T) { + withTempLockDir(t) + args := &skel.CmdArgs{ContainerID: testContainerID} + if _, err := allocate(args, &IPAM{Type: testIPAMType}); err == nil { + t.Fatal("expected error when neither subnet is set, got nil") + } +} + +func TestDeallocateStaticNoop(t *testing.T) { + // Static allocations persist nothing; deallocate must be a pure no-op + // (this asserts it doesn't panic touching pool state that was never + // created). + deallocate(testContainerID, &IPAM{Type: testIPAMType, StaticIP: "fd00::1"}) +} + +func TestAllocateDeallocateRoundTripIPv6(t *testing.T) { + withTempLockDir(t) + args := &skel.CmdArgs{ContainerID: testContainerID} + conf := &IPAM{Type: testIPAMType, IPv6Subnet: testIPv6PoolDefault} + + res, err := allocate(args, conf) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // A fresh "DEL process" must be able to look the allocation up and + // deallocate it, without ever being told the allocated subnet. + deallocate(testContainerID, conf) + + if errs := checkAllocation(testContainerID, conf); len(errs) == 0 { + t.Errorf("checkAllocation after deallocate = no errors, want a not-found error (subnet %v)", res.IPv6Subnet) + } +} + +func TestAllocateDeallocateRoundTripIPv4(t *testing.T) { + withTempLockDir(t) + args := &skel.CmdArgs{ContainerID: testContainerID} + conf := &IPAM{Type: testIPAMType, IPv4Subnet: testIPv4Subnet} + + if _, err := allocate(args, conf); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if errs := checkAllocation(testContainerID, conf); len(errs) != 0 { + t.Fatalf("checkAllocation before deallocate = %v, want no errors", errs) + } + + deallocate(testContainerID, conf) + + if errs := checkAllocation(testContainerID, conf); len(errs) == 0 { + t.Error("checkAllocation after deallocate = no errors, want a not-found error") + } +} + +func TestCheckAllocationStaticAlwaysPasses(t *testing.T) { + if errs := checkAllocation(testContainerID, &IPAM{Type: testIPAMType, StaticIP: "fd00::1"}); errs != nil { + t.Errorf("checkAllocation for static IPAM = %v, want nil (nothing persisted to check)", errs) + } +} + +func TestCheckAllocationUnknownContainer(t *testing.T) { + withTempLockDir(t) + conf := &IPAM{Type: testIPAMType, IPv6Subnet: testIPv6PoolDefault, IPv4Subnet: testIPv4Subnet} + errs := checkAllocation("no-such-container", conf) + if len(errs) != 2 { + t.Fatalf("checkAllocation for unknown container = %v, want 2 errors (one per family)", errs) + } +} diff --git a/internal/cniipam/cniipam.go b/internal/cniipam/cniipam.go new file mode 100644 index 00000000..654e9992 --- /dev/null +++ b/internal/cniipam/cniipam.go @@ -0,0 +1,27 @@ +// Copyright 2025 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cniipam + +import ( + "github.com/containernetworking/cni/pkg/skel" + "github.com/containernetworking/cni/pkg/version" + + "go.datum.net/galactic/internal/metadata" +) + +// RunPlugin starts galactic-ipam, handling the CNI IPAM delegation +// protocol's ADD, DEL, CHECK, and STATUS operations. +func RunPlugin() { + skel.PluginMainFuncs( + skel.CNIFuncs{ + Add: cmdAdd, + Check: cmdCheck, + Del: cmdDel, + Status: cmdStatus, + }, + version.All, + "CNI galactic-ipam plugin "+metadata.Version, + ) +} diff --git a/internal/cniipam/config.go b/internal/cniipam/config.go new file mode 100644 index 00000000..4c78165e --- /dev/null +++ b/internal/cniipam/config.go @@ -0,0 +1,156 @@ +// Copyright 2025 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cniipam + +import ( + "encoding/json" + "fmt" + "net" + + "github.com/containernetworking/cni/pkg/types" + + "go.datum.net/galactic/internal/config" +) + +const errInvalidCNIConfig = "invalid CNI config" + +const ( + // maxIPv6SubnetPrefixLen mirrors internal/cni's own constraint: pool + // prefix must be no longer than the per-allocation subnet length, and + // dual-stack tenant addressing allocates /96 endpoints. + maxIPv6SubnetPrefixLen = 96 + maxIPv4SubnetPrefixLen = 32 +) + +const ( + addressFamilyIPv6 = "ipv6" + addressFamilyIPv4 = "ipv4" +) + +// sanitizeForErrorBinary is substituted for a config value that fails +// sanitizeForError's printable-ASCII check. +const sanitizeForErrorBinary = "" + +// parseConf unmarshals the full CNI config document (the same one the +// master plugin itself received) and validates/normalizes the "ipam" +// block. Returns a *types.Error (CNI error code 7) for anything a real IPAM +// invocation should never see, since a master plugin only ever delegates +// here when its own "ipam" block is present. +func parseConf(data []byte) (*pluginConf, error) { + conf := &pluginConf{} + if err := json.Unmarshal(data, conf); err != nil { + return nil, &types.Error{Code: 7, Msg: errInvalidCNIConfig, Details: err.Error()} + } + if conf.IPAM == nil { + return nil, &types.Error{Code: 7, Msg: "ipam block is required"} + } + + if conf.IPAM.IPv6Subnet != "" { + if err := validateIPv6Subnet(conf.IPAM.IPv6Subnet); err != nil { + return nil, err + } + } + if conf.IPAM.IPv4Subnet != "" { + if err := validateIPv4Subnet(conf.IPAM.IPv4Subnet); err != nil { + return nil, err + } + } + + // Default-filler: only when the ipam block is present but specifies + // neither a static address nor a pool CIDR for either family. Cannot + // manufacture an ipam block out of thin air — that decision already + // happened in the master plugin, before this process was even execed. + if conf.IPAM.StaticIP == "" && conf.IPAM.IPv6Subnet == "" && conf.IPAM.IPv4Subnet == "" { + if config.IPAMGetEnableLocalIPAM() { + conf.IPAM.IPv6Subnet = localIPAMDefaultPool + } + } + + if len(conf.IPAM.AddressFamilies) == 0 { + conf.IPAM.AddressFamilies = []string{addressFamilyIPv6} + } else { + for _, af := range conf.IPAM.AddressFamilies { + switch af { + case addressFamilyIPv6, addressFamilyIPv4: + default: + return nil, &types.Error{Code: 7, Msg: fmt.Sprintf( + "invalid ipam.address_families entry %q: must be %q or %q", + sanitizeForError(af), addressFamilyIPv6, addressFamilyIPv4), + } + } + } + } + + return conf, nil +} + +func validateIPv6Subnet(subnet string) error { + ip, mask, err := net.ParseCIDR(subnet) + if err != nil { + return &types.Error{Code: 7, Msg: fmt.Sprintf( + "invalid CIDR value for field 'ipam.ipv6_subnet': %q", sanitizeForError(subnet)), + } + } + if ip.To4() != nil { + return &types.Error{Code: 7, Msg: fmt.Sprintf( + "ipam.ipv6_subnet must be an IPv6 CIDR, got IPv4: %q", sanitizeForError(subnet)), + } + } + if prefixLen, _ := mask.Mask.Size(); prefixLen > maxIPv6SubnetPrefixLen { + return &types.Error{Code: 7, Msg: fmt.Sprintf( + "ipam.ipv6_subnet prefix length %d exceeds maximum of %d: %q", + prefixLen, maxIPv6SubnetPrefixLen, sanitizeForError(subnet)), + } + } + return nil +} + +func validateIPv4Subnet(subnet string) error { + ip, mask, err := net.ParseCIDR(subnet) + if err != nil { + return &types.Error{Code: 7, Msg: fmt.Sprintf( + "invalid CIDR value for field 'ipam.ipv4_subnet': %q", sanitizeForError(subnet)), + } + } + if ip.To4() == nil { + return &types.Error{Code: 7, Msg: fmt.Sprintf( + "ipam.ipv4_subnet must be an IPv4 CIDR, got IPv6: %q", sanitizeForError(subnet)), + } + } + if prefixLen, _ := mask.Mask.Size(); prefixLen > maxIPv4SubnetPrefixLen { + return &types.Error{Code: 7, Msg: fmt.Sprintf( + "ipam.ipv4_subnet prefix length %d exceeds maximum of %d: %q", + prefixLen, maxIPv4SubnetPrefixLen, sanitizeForError(subnet)), + } + } + return nil +} + +// sanitizeForError returns s unchanged if it contains only printable ASCII +// characters; otherwise returns "" to avoid corrupting log output. +func sanitizeForError(s string) string { + for _, c := range s { + if c < 0x20 || c > 0x7e { + return sanitizeForErrorBinary + } + } + return s +} + +// parseStatusConf validates that STATUS's config is minimally parseable. +// galactic-ipam has no attachment-specific or API-server state to check — +// STATUS just confirms the binary can parse a well-formed CNI config. +func parseStatusConf(data []byte) error { + var sc struct { + CNIVersion string `json:"cniVersion"` + } + if err := json.Unmarshal(data, &sc); err != nil { + return &types.Error{Code: 7, Msg: errInvalidCNIConfig, Details: err.Error()} + } + if sc.CNIVersion == "" { + return &types.Error{Code: 7, Msg: "cniVersion is required"} + } + return nil +} diff --git a/internal/cniipam/config_test.go b/internal/cniipam/config_test.go new file mode 100644 index 00000000..7206bc93 --- /dev/null +++ b/internal/cniipam/config_test.go @@ -0,0 +1,149 @@ +// Copyright 2025 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cniipam + +import ( + "fmt" + "strings" + "testing" +) + +const ( + testCNIVersion = "1.0.0" + testIPAMType = "galactic-ipam" +) + +// confJSON builds a minimal CNI config document carrying the given "ipam" +// block body (already-JSON-encoded, e.g. `"type":"galactic-ipam"`), keeping +// every test case below short enough to stay under the project's line +// length limit. +func confJSON(ipamBody string) string { + return fmt.Sprintf(`{"cniVersion":"%s","name":"test","ipam":{%s}}`, testCNIVersion, ipamBody) +} + +func TestParseConf(t *testing.T) { + tests := []struct { + name string + input string + wantErr string + wantType string + }{ + { + name: "MissingIPAMBlockRejected", + input: fmt.Sprintf(`{"cniVersion":"%s","name":"test","type":"%s"}`, testCNIVersion, testIPAMType), + wantErr: "ipam block is required", + }, + { + name: "StaticIPAccepted", + input: confJSON(fmt.Sprintf(`"type":%q,"static_ip":"fd00::1234"`, testIPAMType)), + wantType: testIPAMType, + }, + { + name: "InvalidIPv6SubnetCIDRRejected", + input: confJSON(fmt.Sprintf(`"type":%q,"ipv6_subnet":"not-a-cidr"`, testIPAMType)), + wantErr: "invalid CIDR value for field 'ipam.ipv6_subnet'", + }, + { + name: "IPv4GivenWhereIPv6SubnetExpectedRejected", + input: confJSON(fmt.Sprintf(`"type":%q,"ipv6_subnet":"10.0.0.0/24"`, testIPAMType)), + wantErr: "ipam.ipv6_subnet must be an IPv6 CIDR, got IPv4", + }, + { + name: "IPv6SubnetPrefixLengthOver96Rejected", + input: confJSON(fmt.Sprintf(`"type":%q,"ipv6_subnet":"fd00:10:ff01::/112"`, testIPAMType)), + wantErr: "ipam.ipv6_subnet prefix length 112 exceeds maximum of 96", + }, + { + name: "InvalidIPv4SubnetCIDRRejected", + input: confJSON(fmt.Sprintf(`"type":%q,"ipv4_subnet":"not-a-cidr"`, testIPAMType)), + wantErr: "invalid CIDR value for field 'ipam.ipv4_subnet'", + }, + { + name: "IPv6GivenWhereIPv4SubnetExpectedRejected", + input: confJSON(fmt.Sprintf(`"type":%q,"ipv4_subnet":"2001:db8::/64"`, testIPAMType)), + wantErr: "ipam.ipv4_subnet must be an IPv4 CIDR, got IPv6", + }, + { + name: "IPv4SubnetPrefixLengthOver32Rejected", + input: confJSON(fmt.Sprintf(`"type":%q,"ipv4_subnet":"::ffff:10.0.0.0/40"`, testIPAMType)), + wantErr: "ipam.ipv4_subnet prefix length 40 exceeds maximum of 32", + }, + { + name: "InvalidAddressFamiliesEntryRejected", + input: confJSON(fmt.Sprintf( + `"type":%q,"ipv6_subnet":"fd00::/64","address_families":["ipv6","bogus"]`, testIPAMType)), + wantErr: `invalid ipam.address_families entry "bogus"`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + conf, err := parseConf([]byte(tt.input)) + if tt.wantErr != "" { + if err == nil { + t.Fatalf("expected error containing %q, got nil", tt.wantErr) + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("error %q does not contain %q", err, tt.wantErr) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if conf.IPAM.Type != tt.wantType { + t.Errorf("IPAM.Type = %q, want %q", conf.IPAM.Type, tt.wantType) + } + }) + } +} + +func TestParseConfDefaultFillerOnlyWhenUnderspecified(t *testing.T) { + t.Setenv("GALACTIC_IPAM_ENABLE_LOCAL_IPAM", "true") + + // ipam present but specifies neither static_ip nor a subnet: filled in. + conf, err := parseConf([]byte(confJSON(fmt.Sprintf(`"type":%q`, testIPAMType)))) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if conf.IPAM.IPv6Subnet != localIPAMDefaultPool { + t.Errorf("IPv6Subnet = %q, want default-filled %q", conf.IPAM.IPv6Subnet, localIPAMDefaultPool) + } + + // ipam present and already specifies a subnet: default-filler must not + // override it. + conf, err = parseConf([]byte(confJSON(fmt.Sprintf(`"type":%q,"ipv4_subnet":"10.0.0.0/24"`, testIPAMType)))) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if conf.IPAM.IPv6Subnet != "" { + t.Errorf("IPv6Subnet = %q, want empty (ipv4_subnet already specified)", conf.IPAM.IPv6Subnet) + } +} + +func TestParseConfDefaultFillerRequiresEnvVar(t *testing.T) { + t.Setenv("GALACTIC_IPAM_ENABLE_LOCAL_IPAM", "false") + + conf, err := parseConf([]byte(confJSON(fmt.Sprintf(`"type":%q`, testIPAMType)))) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if conf.IPAM.IPv6Subnet != "" || conf.IPAM.IPv4Subnet != "" { + t.Errorf("IPv6Subnet/IPv4Subnet = %q/%q, want both empty (default-filler disabled)", + conf.IPAM.IPv6Subnet, conf.IPAM.IPv4Subnet) + } +} + +func TestParseStatusConf(t *testing.T) { + if err := parseStatusConf([]byte(`{"cniVersion":"1.0.0"}`)); err != nil { + t.Errorf("unexpected error: %v", err) + } + if err := parseStatusConf([]byte(`not json`)); err == nil { + t.Error("expected error for invalid JSON, got nil") + } + if err := parseStatusConf([]byte(`{}`)); err == nil { + t.Error("expected error for missing cniVersion, got nil") + } +} diff --git a/internal/cniipam/ipam.go b/internal/cniipam/ipam.go deleted file mode 100644 index 46310173..00000000 --- a/internal/cniipam/ipam.go +++ /dev/null @@ -1,276 +0,0 @@ -// Copyright 2025 Datum Cloud, Inc. -// -// SPDX-License-Identifier: AGPL-3.0-or-later - -// Package cniipam holds the IPAM allocation/deallocation logic shared by -// every master plugin in the galactic CNI chain (galactic-cni, veth; -// galactic-tap-cni, tap) — interface-agnostic, since neither kernel state -// nor network namespaces are touched here (galactic-cni's own -// configureInterfaceInNetns applies the result to the guest veth; tap mode -// applies nothing, the VM manages its own interface). -// -// This package is a plain library today, imported directly by the master -// plugins, not yet a delegated CNI IPAM plugin of its own — that lands in a -// follow-up step (cmd/galactic-ipam, the CNI IPAM delegation protocol, and -// dropping the getAllocatedSubnetsFromCRD dependency below in favor of local -// marker-file persistence for the IPv6 pool allocator, mirroring -// IPv4PoolAllocator's). Landing this as its own package now — instead of -// duplicating it between galactic-cni and galactic-tap-cni — means that -// follow-up step only has to add the delegation wiring, not first -// de-duplicate two drifted copies. -package cniipam - -import ( - "context" - "errors" - "fmt" - "log/slog" - "net" - "time" - - "github.com/containernetworking/cni/pkg/skel" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" - - "go.datum.net/galactic/internal/cni/crdnames" - "go.datum.net/galactic/internal/cni/ipam" - bgpv1alpha1 "go.datum.net/network/api/v1alpha1" -) - -// cniTimeout bounds the k8s API call getAllocatedSubnetsFromCRD makes while -// deallocating. Mirrors internal/cni's own cniTimeout — small enough that a -// duplicated constant beats a shared package for this alone. -const cniTimeout = 10 * time.Second - -// TypeStatic is the ipam.type value for a single pre-assigned static -// address. Any other (or empty) value takes the pool-based dual-stack path -// — see WantsIPAM/Allocate. -const TypeStatic = "static" - -// localIPAMDefaultPool is the IPv6 CIDR pool used when local IPAM is enabled -// but IPv6Subnet is unset in the CNI config. Allocations from it use -// ipam.DefaultSubnetLen (/96). -const localIPAMDefaultPool = "fd00:10:ff01::/64" - -// IPAM holds IP address management configuration passed in the CNI config's -// "ipam" block. -type IPAM struct { - Type string `json:"type"` // "pool" (default) or "static" - StaticIP string `json:"static_ip,omitempty"` // used when type="static" - Routes []Route `json:"routes,omitempty"` - Addresses []Address `json:"addresses,omitempty"` -} - -// Route describes a static route to install. -type Route struct { - Dst string `json:"dst"` - GW string `json:"gw,omitempty"` -} - -// Address describes a static IP address assignment. -type Address struct { - Address string `json:"address"` -} - -// IPAMResult holds the IPAM allocation details a caller uses to build its -// own CNI result and, for veth, to configure the guest interface. -// IPv4Address/IPv4Gateway are nil when the attachment is IPv6-only. -type IPAMResult struct { - IPv6Subnet *net.IPNet - IPv6Gateway net.IP - IPv4Address net.IP - IPv4Gateway net.IP - Routes []*net.IPNet -} - -// AllocConfig carries the subset of a caller's own CNI config that IPAM -// allocation needs. Each master plugin passes its own values in — this -// package doesn't know, or need to know, about the rest of that config's -// shape. -type AllocConfig struct { - VPC string - VPCAttachment string - Namespace string - IPAM *IPAM - IPv6Subnet string - IPv4Subnet string - AddressFamilies []string -} - -// ipv4LockDir is the IPv4PoolAllocator lock/state directory used by -// allocatePoolIPAM/Deallocate. Overridable in tests so unit tests never -// touch the real production path. -var ipv4LockDir = ipam.DefaultIPv4LockDir - -// enableLocalIPAM controls whether allocation proceeds when no explicit -// "ipam" block is present in the CNI config. Defaults to false. -var enableLocalIPAM bool - -// SetEnableLocalIPAM sets the local IPAM flag from the CLI/env. -func SetEnableLocalIPAM(v bool) { - enableLocalIPAM = v -} - -// WantsIPAM reports whether cfg should trigger IPAM allocation at all. Four -// independent signals opt in: an explicit "static" IPAM type, a configured -// IPv6Subnet or IPv4Subnet (the NAD-driven pool-IPAM path, either family -// alone or both), or the enable-local-ipam dev fallback. A config with none -// of these (e.g. a tap workload that manages its own addressing) allocates -// nothing, matching the CNI plugin's own longstanding behavior of skipping -// IPAM entirely rather than erroring. -func WantsIPAM(cfg AllocConfig) bool { - if cfg.IPAM != nil && cfg.IPAM.Type == TypeStatic { - return true - } - return cfg.IPv6Subnet != "" || cfg.IPv4Subnet != "" || enableLocalIPAM -} - -// Allocate allocates addresses for the given container. This is -// interface-agnostic — it does not touch any kernel state or network -// namespaces. Returns (nil, nil) when WantsIPAM reports no allocation is -// requested. When enableLocalIPAM is true and IPv6Subnet is unset, falls -// back to a built-in default IPv6 pool CIDR. -func Allocate(args *skel.CmdArgs, cfg AllocConfig) (*IPAMResult, error) { - if !WantsIPAM(cfg) { - return nil, nil - } - - if cfg.IPAM != nil && cfg.IPAM.Type == TypeStatic { - return allocateStatic(args, cfg.IPAM) - } - - return allocatePool(args, cfg) -} - -// allocateStatic validates and returns the pre-assigned static IPv6 address -// from the "static" IPAM block. No IPv4 address is ever allocated for -// static IPAM — it is a single fixed address, not a dual-stack pool. -func allocateStatic(args *skel.CmdArgs, ipamConf *IPAM) (*IPAMResult, error) { - alloc := ipam.NewStaticAllocator() - allocIP, err := alloc.Allocate(args.ContainerID, ipamConf.StaticIP) - if err != nil { - return nil, fmt.Errorf("allocate static IP: %w", err) - } - subnet := &net.IPNet{ - IP: allocIP, - Mask: net.CIDRMask(64, 128), - } - slog.Debug("IPAM: allocated static", "containerID", args.ContainerID, "subnet", subnet) - return &IPAMResult{IPv6Subnet: subnet}, nil -} - -// allocatePool allocates a dual-stack, IPv6-only, or IPv4-only pool-based -// endpoint address for the given container, via ipam.DualStackAllocator. -// IPv6Subnet and IPv4Subnet each independently supply a pool CIDR for their -// family; at least one must be set (falling back to localIPAMDefaultPool for -// IPv6 when enableLocalIPAM and both are unset). -func allocatePool(args *skel.CmdArgs, cfg AllocConfig) (*IPAMResult, error) { - ipv6Pool := cfg.IPv6Subnet - if ipv6Pool == "" && cfg.IPv4Subnet == "" { - if !enableLocalIPAM { - return nil, errors.New("ipv6_subnet or ipv4_subnet is required (or enable local IPAM)") - } - ipv6Pool = localIPAMDefaultPool - } - - alloc, err := ipam.NewDualStackAllocator(ipv6Pool, "", cfg.IPv4Subnet, "", ipv4LockDir) - if err != nil { - return nil, fmt.Errorf("create dual-stack allocator: %w", err) - } - - res, err := alloc.Allocate(args.ContainerID) - if err != nil { - return nil, fmt.Errorf("allocate dual-stack addresses: %w", err) - } - - var routes []*net.IPNet - if res.IPv6Subnet != nil { - routes = append(routes, &net.IPNet{IP: net.IPv6zero, Mask: net.CIDRMask(0, 128)}) - } - if res.IPv4Address != nil { - routes = append(routes, &net.IPNet{IP: net.IPv4zero, Mask: net.CIDRMask(0, 32)}) - } - - slog.Debug("IPAM: allocated", "containerID", args.ContainerID, - "ipv6Subnet", res.IPv6Subnet, "ipv6Gateway", res.IPv6Gateway, - "ipv4Address", res.IPv4Address, "ipv4Gateway", res.IPv4Gateway) - - return &IPAMResult{ - IPv6Subnet: res.IPv6Subnet, - IPv6Gateway: res.IPv6Gateway, - IPv4Address: res.IPv4Address, - IPv4Gateway: res.IPv4Gateway, - Routes: routes, - }, nil -} - -// Deallocate releases the IPAM allocation for the given container. Reads the -// allocated IPv6 subnet and (if present) IPv4 address from the -// BGPAdvertisement CRD annotations galactic-bgp wrote, then deallocates each -// independently and non-fatally: a missing annotation for one family (e.g. a -// pre-existing v6-only pod, or a partial ADD failure that never reached IPv4 -// allocation) must not prevent cleanup of the other. -func Deallocate(args *skel.CmdArgs, cfg AllocConfig, k8s client.Client) { - if cfg.IPAM != nil && cfg.IPAM.Type == TypeStatic { - // Static allocations don't need deallocation. - return - } - - ipv6Subnet, ipv4Addr := getAllocatedSubnetsFromCRD(args.ContainerID, cfg, k8s) - if ipv6Subnet == "" && ipv4Addr == "" { - // No allocation found — either allocation was never completed, - // or the advertisement was already deleted. Nothing to clean up. - slog.Debug("IPAM: no allocation found to deallocate", "containerID", args.ContainerID) - return - } - - if ipv6Subnet != "" { - ipv6Pool := cfg.IPv6Subnet - if ipv6Pool == "" && enableLocalIPAM { - ipv6Pool = localIPAMDefaultPool - } - pa, err := ipam.NewPoolAllocator(ipv6Pool, "", 0) - if err != nil { - slog.Warn("IPAM: failed to build IPv6 pool allocator for deallocation, skipping", "err", err, - "containerID", args.ContainerID, "subnet", ipv6Subnet) - } else { - pa.Deallocate(ipv6Subnet) - slog.Debug("IPAM: deallocated IPv6", "containerID", args.ContainerID, "subnet", ipv6Subnet) - } - } - - if ipv4Addr != "" { - if cfg.IPv4Subnet == "" { - slog.Warn("IPAM: found allocated IPv4 address but no ipv4_subnet in config, skipping deallocation", - "containerID", args.ContainerID, "address", ipv4Addr) - } else if pa, err := ipam.NewIPv4PoolAllocator(cfg.IPv4Subnet, "", ipv4LockDir); err != nil { - slog.Warn("IPAM: failed to build IPv4 pool allocator for deallocation, skipping", "err", err, - "containerID", args.ContainerID, "address", ipv4Addr) - } else { - pa.Deallocate(ipv4Addr) - slog.Debug("IPAM: deallocated IPv4", "containerID", args.ContainerID, "address", ipv4Addr) - } - } -} - -// getAllocatedSubnetsFromCRD reads the allocated IPv6 subnet and (if -// present) IPv4 address for the given container from the BGPAdvertisement -// CRD annotations. Either return value is empty when not found. -func getAllocatedSubnetsFromCRD( - containerID string, cfg AllocConfig, k8s client.Client, -) (ipv6Subnet, ipv4Addr string) { - ctx, cancel := context.WithTimeout(context.Background(), cniTimeout) - defer cancel() - - adv := &bgpv1alpha1.BGPAdvertisement{ - ObjectMeta: metav1.ObjectMeta{ - Name: crdnames.BGPAdvertisementName(cfg.VPC, cfg.VPCAttachment), - Namespace: cfg.Namespace, - }, - } - if err := k8s.Get(ctx, client.ObjectKeyFromObject(adv), adv); err != nil { - return "", "" - } - - return adv.Annotations[crdnames.SubnetKeyIPv6(containerID)], adv.Annotations[crdnames.SubnetKeyIPv4(containerID)] -} diff --git a/internal/cniipam/ipam_test.go b/internal/cniipam/ipam_test.go deleted file mode 100644 index c8d2ee22..00000000 --- a/internal/cniipam/ipam_test.go +++ /dev/null @@ -1,397 +0,0 @@ -// Copyright 2025 Datum Cloud, Inc. -// -// SPDX-License-Identifier: AGPL-3.0-or-later - -package cniipam - -import ( - "net" - "testing" - - "github.com/containernetworking/cni/pkg/skel" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - utilruntime "k8s.io/apimachinery/pkg/util/runtime" - clientgoscheme "k8s.io/client-go/kubernetes/scheme" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/fake" - - "go.datum.net/galactic/internal/cni/crdnames" - "go.datum.net/galactic/internal/cni/ipam" - bgpv1alpha1 "go.datum.net/network/api/v1alpha1" -) - -const ( - testVPC = "abc" - testAttachment = "def" - testContainerID = "test-container" - testNamespace = "galactic-system" - testIPv4Subnet = "10.128.0.0/20" -) - -var testScheme = func() *runtime.Scheme { - s := runtime.NewScheme() - utilruntime.Must(clientgoscheme.AddToScheme(s)) - utilruntime.Must(bgpv1alpha1.AddToScheme(s)) - return s -}() - -func fakeClient(objs ...client.Object) client.Client { - return fake.NewClientBuilder().WithScheme(testScheme).WithObjects(objs...).Build() -} - -// ---- WantsIPAM ------------------------------------------------------------- - -func TestWantsIPAM(t *testing.T) { - original := enableLocalIPAM - defer func() { enableLocalIPAM = original }() - - tests := []struct { - name string - cfg AllocConfig - enableLocalIPA bool - want bool - }{ - { - name: "no ipam block, no ipv6_subnet, local IPAM disabled", - cfg: AllocConfig{}, - want: false, - }, - { - name: "static ipam type opts in regardless of other fields", - cfg: AllocConfig{IPAM: &IPAM{Type: TypeStatic}}, - want: true, - }, - { - name: "ipv6_subnet set opts in", - cfg: AllocConfig{IPv6Subnet: localIPAMDefaultPool}, - want: true, - }, - { - name: "ipv4_subnet set opts in", - cfg: AllocConfig{IPv4Subnet: testIPv4Subnet}, - want: true, - }, - { - name: "local IPAM enabled opts in even without ipv6_subnet", - cfg: AllocConfig{}, - enableLocalIPA: true, - want: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - enableLocalIPAM = tt.enableLocalIPA - if got := WantsIPAM(tt.cfg); got != tt.want { - t.Errorf("WantsIPAM(%+v) = %v, want %v", tt.cfg, got, tt.want) - } - }) - } -} - -func TestSetEnableLocalIPAM(t *testing.T) { - original := enableLocalIPAM - defer func() { enableLocalIPAM = original }() - - enableLocalIPAM = false - if enableLocalIPAM { - t.Error("enableLocalIPAM default = true, want false") - } - - SetEnableLocalIPAM(true) - if !enableLocalIPAM { - t.Error("enableLocalIPAM after SetEnableLocalIPAM(true) = false, want true") - } - - SetEnableLocalIPAM(false) - if enableLocalIPAM { - t.Error("enableLocalIPAM after SetEnableLocalIPAM(false) = true, want false") - } -} - -// ---- Allocate --------------------------------------------------------- - -func TestAllocateNoAllocation(t *testing.T) { - original := enableLocalIPAM - defer func() { enableLocalIPAM = original }() - enableLocalIPAM = false - - args := &skel.CmdArgs{ContainerID: testContainerID} - res, err := Allocate(args, AllocConfig{}) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if res != nil { - t.Errorf("Allocate() = %+v, want nil (WantsIPAM should have been false)", res) - } -} - -func TestAllocateStatic(t *testing.T) { - args := &skel.CmdArgs{ContainerID: testContainerID} - cfg := AllocConfig{IPAM: &IPAM{Type: TypeStatic, StaticIP: "fd00:10:ff01::1234"}} - - res, err := Allocate(args, cfg) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if res == nil { - t.Fatal("Allocate() = nil, want a result") - } - if res.IPv6Subnet == nil || !res.IPv6Subnet.IP.Equal(net.ParseIP("fd00:10:ff01::1234")) { - t.Errorf("IPv6Subnet = %v, want fd00:10:ff01::1234", res.IPv6Subnet) - } - if res.IPv4Address != nil { - t.Errorf("IPv4Address = %v, want nil for static IPAM", res.IPv4Address) - } -} - -func TestAllocatePoolIPv6Only(t *testing.T) { - args := &skel.CmdArgs{ContainerID: testContainerID} - cfg := AllocConfig{IPv6Subnet: localIPAMDefaultPool} - - res, err := Allocate(args, cfg) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if res == nil { - t.Fatal("Allocate() = nil, want a result") - } - if res.IPv6Subnet == nil { - t.Fatal("IPv6Subnet = nil, want an allocated /96") - } - if ones, bits := res.IPv6Subnet.Mask.Size(); ones != 96 || bits != 128 { - t.Errorf("IPv6Subnet mask = /%d, want /96", ones) - } - if res.IPv6Gateway == nil { - t.Error("IPv6Gateway = nil, want the pool's default gateway (::1 of the /64)") - } - if res.IPv4Address != nil { - t.Errorf("IPv4Address = %v, want nil (no ipv4_subnet configured)", res.IPv4Address) - } - if len(res.Routes) != 1 { - t.Errorf("Routes = %v, want exactly one default IPv6 route", res.Routes) - } -} - -func TestAllocatePoolIPv4Only(t *testing.T) { - origLockDir := ipv4LockDir - ipv4LockDir = t.TempDir() - defer func() { ipv4LockDir = origLockDir }() - - args := &skel.CmdArgs{ContainerID: testContainerID} - cfg := AllocConfig{IPv4Subnet: testIPv4Subnet} - - res, err := Allocate(args, cfg) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if res == nil { - t.Fatal("Allocate() = nil, want a result") - } - if res.IPv6Subnet != nil { - t.Errorf("IPv6Subnet = %v, want nil (no ipv6_subnet configured)", res.IPv6Subnet) - } - if res.IPv4Address == nil { - t.Fatal("IPv4Address = nil, want an allocated /32") - } - if res.IPv4Gateway == nil { - t.Error("IPv4Gateway = nil, want the pool's default gateway") - } - if len(res.Routes) != 1 { - t.Errorf("Routes = %v, want exactly one default IPv4 route", res.Routes) - } -} - -func TestAllocatePoolDualStack(t *testing.T) { - origLockDir := ipv4LockDir - ipv4LockDir = t.TempDir() - defer func() { ipv4LockDir = origLockDir }() - - args := &skel.CmdArgs{ContainerID: testContainerID} - cfg := AllocConfig{ - IPv6Subnet: localIPAMDefaultPool, - IPv4Subnet: testIPv4Subnet, - } - - res, err := Allocate(args, cfg) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if res == nil { - t.Fatal("Allocate() = nil, want a result") - } - if res.IPv6Subnet == nil { - t.Error("IPv6Subnet = nil, want an allocated /96") - } - if res.IPv4Address == nil { - t.Fatal("IPv4Address = nil, want an allocated /32") - } - if res.IPv4Gateway == nil { - t.Error("IPv4Gateway = nil, want the pool's default gateway") - } - if len(res.Routes) != 2 { - t.Errorf("Routes = %v, want one default route per family", res.Routes) - } -} - -func TestAllocatePoolMissingBothSubnetsErrors(t *testing.T) { - original := enableLocalIPAM - defer func() { enableLocalIPAM = original }() - enableLocalIPAM = false - - args := &skel.CmdArgs{ContainerID: testContainerID} - _, err := allocatePool(args, AllocConfig{}) - if err == nil { - t.Fatal("expected error when both ipv6_subnet and ipv4_subnet are unset and local IPAM is disabled, got nil") - } -} - -// ---- Deallocate --------------------------------------------------------- - -func TestDeallocateStaticNoop(t *testing.T) { - // Static IPAM never wrote a CRD annotation for DEL to look up, and - // Deallocate must return immediately without attempting a k8s lookup. - cfg := AllocConfig{ - VPC: testVPC, VPCAttachment: testAttachment, - IPAM: &IPAM{Type: TypeStatic}, - } - args := &skel.CmdArgs{ContainerID: testContainerID} - // A nil client would panic if Deallocate tried to use it; passing nil - // here asserts the static-type early return happens first. - Deallocate(args, cfg, nil) -} - -func TestDeallocateDualStack(t *testing.T) { - origLockDir := ipv4LockDir - ipv4LockDir = t.TempDir() - defer func() { ipv4LockDir = origLockDir }() - - cfg := AllocConfig{ - VPC: testVPC, VPCAttachment: testAttachment, - Namespace: testNamespace, - IPv6Subnet: localIPAMDefaultPool, - IPv4Subnet: testIPv4Subnet, - } - args := &skel.CmdArgs{ContainerID: testContainerID} - - alloc, err := ipam.NewDualStackAllocator(cfg.IPv6Subnet, "", cfg.IPv4Subnet, "", ipv4LockDir) - if err != nil { - t.Fatalf("NewDualStackAllocator: %v", err) - } - dsRes, err := alloc.Allocate(args.ContainerID) - if err != nil { - t.Fatalf("Allocate: %v", err) - } - - ipv4Pool, err := ipam.NewIPv4PoolAllocator(cfg.IPv4Subnet, "", ipv4LockDir) - if err != nil { - t.Fatalf("NewIPv4PoolAllocator: %v", err) - } - if !ipv4Pool.IsAllocated(dsRes.IPv4Address.String()) { - t.Fatalf("setup: IPv4 address %s not marked allocated", dsRes.IPv4Address) - } - - adv := &bgpv1alpha1.BGPAdvertisement{ - ObjectMeta: metav1.ObjectMeta{ - Name: crdnames.BGPAdvertisementName(cfg.VPC, cfg.VPCAttachment), - Namespace: testNamespace, - Annotations: map[string]string{ - crdnames.SubnetKeyIPv6(args.ContainerID): dsRes.IPv6Subnet.String(), - crdnames.SubnetKeyIPv4(args.ContainerID): dsRes.IPv4Address.String(), - }, - }, - } - k8s := fakeClient(adv) - - Deallocate(args, cfg, k8s) - - if ipv4Pool.IsAllocated(dsRes.IPv4Address.String()) { - t.Errorf("IPv4 address %s still marked allocated after Deallocate", dsRes.IPv4Address) - } -} - -func TestDeallocateIPv4Only(t *testing.T) { - origLockDir := ipv4LockDir - ipv4LockDir = t.TempDir() - defer func() { ipv4LockDir = origLockDir }() - - cfg := AllocConfig{ - VPC: testVPC, VPCAttachment: testAttachment, - Namespace: testNamespace, - IPv4Subnet: testIPv4Subnet, - } - args := &skel.CmdArgs{ContainerID: testContainerID} - - ipv4Pool, err := ipam.NewIPv4PoolAllocator(cfg.IPv4Subnet, "", ipv4LockDir) - if err != nil { - t.Fatalf("NewIPv4PoolAllocator: %v", err) - } - ipv4Addr, err := ipv4Pool.Allocate(args.ContainerID) - if err != nil { - t.Fatalf("Allocate: %v", err) - } - if !ipv4Pool.IsAllocated(ipv4Addr.String()) { - t.Fatalf("setup: IPv4 address %s not marked allocated", ipv4Addr) - } - - adv := &bgpv1alpha1.BGPAdvertisement{ - ObjectMeta: metav1.ObjectMeta{ - Name: crdnames.BGPAdvertisementName(cfg.VPC, cfg.VPCAttachment), - Namespace: testNamespace, - Annotations: map[string]string{ - // No IPv6 annotation — this is an IPv4-only allocation. - crdnames.SubnetKeyIPv4(args.ContainerID): ipv4Addr.String(), - }, - }, - } - k8s := fakeClient(adv) - - // Must not panic despite no ipv6_subnet in config, and must deallocate - // the IPv4 address. - Deallocate(args, cfg, k8s) - - if ipv4Pool.IsAllocated(ipv4Addr.String()) { - t.Errorf("IPv4 address %s still marked allocated after Deallocate", ipv4Addr) - } -} - -func TestDeallocatePartialAllocationNonFatal(t *testing.T) { - // A v6-only pod (no IPv4 annotation, e.g. pre-existing or a partial ADD - // failure) must still have its IPv6 side cleaned up without erroring, - // and must not attempt to touch a nonexistent IPv4 pool. - cfg := AllocConfig{ - VPC: testVPC, VPCAttachment: testAttachment, - Namespace: testNamespace, - IPv6Subnet: localIPAMDefaultPool, - // IPv4Subnet intentionally unset. - } - args := &skel.CmdArgs{ContainerID: testContainerID} - - adv := &bgpv1alpha1.BGPAdvertisement{ - ObjectMeta: metav1.ObjectMeta{ - Name: crdnames.BGPAdvertisementName(cfg.VPC, cfg.VPCAttachment), - Namespace: testNamespace, - Annotations: map[string]string{ - crdnames.SubnetKeyIPv6(args.ContainerID): "fd00:10:ff01::1234/96", - }, - }, - } - k8s := fakeClient(adv) - - // Must not panic despite no ipv4_subnet in config. - Deallocate(args, cfg, k8s) -} - -func TestDeallocateNoAllocationFound(t *testing.T) { - cfg := AllocConfig{ - VPC: testVPC, VPCAttachment: testAttachment, - Namespace: testNamespace, - } - args := &skel.CmdArgs{ContainerID: testContainerID} - // No BGPAdvertisement exists at all. - k8s := fakeClient() - - // Must return cleanly with nothing to deallocate. - Deallocate(args, cfg, k8s) -} diff --git a/internal/cniipam/ops.go b/internal/cniipam/ops.go new file mode 100644 index 00000000..3c906579 --- /dev/null +++ b/internal/cniipam/ops.go @@ -0,0 +1,85 @@ +// Copyright 2025 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cniipam + +import ( + "errors" + "fmt" + "log/slog" + + "github.com/containernetworking/cni/pkg/skel" + "github.com/containernetworking/cni/pkg/types" +) + +// cmdAdd implements the CNI IPAM delegation ADD path. Being invoked at all +// is the master plugin's own signal that its "ipam" block was present — +// this function always allocates, it never re-checks whether it should. +func cmdAdd(args *skel.CmdArgs) error { + conf, err := parseConf(args.StdinData) + if err != nil { + return err + } + + slog.Info("ADD: starting", "containerID", args.ContainerID, "type", conf.IPAM.Type) + + result, err := allocate(args, conf.IPAM) + if err != nil { + return fmt.Errorf("allocate: %w", err) + } + + cniResult := BuildCNIResult(conf.CNIVersion, result) + if err := types.PrintResult(cniResult, conf.CNIVersion); err != nil { + return fmt.Errorf("print CNI result: %w", err) + } + slog.Info("ADD: allocated", "containerID", args.ContainerID, + "ipv6Subnet", result.IPv6Subnet, "ipv4Address", result.IPv4Address) + return nil +} + +// cmdDel implements the CNI IPAM delegation DEL path. Per the CNI spec, +// DEL is idempotent: a config parse failure or a missing allocation is not +// an error, since there may be nothing left to clean up. +func cmdDel(args *skel.CmdArgs) error { + slog.Info("DEL: starting", "containerID", args.ContainerID) + + conf, err := parseConf(args.StdinData) + if err != nil { + slog.Warn("DEL: failed to parse CNI config, skipping deallocation", "err", err, + "containerID", args.ContainerID) + return nil + } + + deallocate(args.ContainerID, conf.IPAM) + return nil +} + +// cmdCheck implements the CNI IPAM delegation CHECK path: confirm the +// containerID's allocation, if any, is still present in each family conf +// configures. +func cmdCheck(args *skel.CmdArgs) error { + conf, err := parseConf(args.StdinData) + if err != nil { + return err + } + + if errs := checkAllocation(args.ContainerID, conf.IPAM); len(errs) > 0 { + err := fmt.Errorf("CHECK failed: %w", errors.Join(errs...)) + slog.Error("CHECK: failed", "err", err, "containerID", args.ContainerID) + return err + } + slog.Info("CHECK: passed", "containerID", args.ContainerID) + return nil +} + +// cmdStatus implements the CNI spec STATUS operation. galactic-ipam has no +// API server or attachment-specific state to probe — it either parses a +// well-formed config or it doesn't. +func cmdStatus(args *skel.CmdArgs) error { + if err := parseStatusConf(args.StdinData); err != nil { + return err + } + slog.Info("STATUS: ready") + return nil +} diff --git a/internal/cniipam/ops_test.go b/internal/cniipam/ops_test.go new file mode 100644 index 00000000..4c80cdd9 --- /dev/null +++ b/internal/cniipam/ops_test.go @@ -0,0 +1,81 @@ +// Copyright 2025 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cniipam + +import ( + "fmt" + "strings" + "testing" + + "github.com/containernetworking/cni/pkg/skel" +) + +func TestCmdAddCmdDelCmdCheckRoundTrip(t *testing.T) { + withTempLockDir(t) + + conf := confJSON(fmt.Sprintf(`"type":%q,"ipv6_subnet":%q`, testIPAMType, testIPv6PoolDefault)) + args := &skel.CmdArgs{ContainerID: testContainerID, StdinData: []byte(conf)} + + if err := cmdAdd(args); err != nil { + t.Fatalf("cmdAdd: unexpected error: %v", err) + } + if err := cmdCheck(args); err != nil { + t.Fatalf("cmdCheck after cmdAdd: unexpected error: %v", err) + } + if err := cmdDel(args); err != nil { + t.Fatalf("cmdDel: unexpected error: %v", err) + } + if err := cmdCheck(args); err == nil { + t.Fatal("cmdCheck after cmdDel: expected error (allocation released), got nil") + } +} + +func TestCmdAddMissingIPAMBlock(t *testing.T) { + args := &skel.CmdArgs{ + ContainerID: testContainerID, + StdinData: []byte(fmt.Sprintf(`{"cniVersion":"%s","name":"test","type":%q}`, testCNIVersion, testIPAMType)), + } + err := cmdAdd(args) + if err == nil || !strings.Contains(err.Error(), "ipam block is required") { + t.Fatalf("expected 'ipam block is required' error, got: %v", err) + } +} + +func TestCmdDelIdempotentOnUnparseableConfig(t *testing.T) { + args := &skel.CmdArgs{ContainerID: testContainerID, StdinData: []byte("not valid json")} + if err := cmdDel(args); err != nil { + t.Fatalf("cmdDel with invalid config returned error = %v, want nil (idempotent)", err) + } +} + +func TestCmdDelStaticIsNoop(t *testing.T) { + conf := confJSON(fmt.Sprintf(`"type":%q,"static_ip":"fd00::1234"`, testIPAMType)) + args := &skel.CmdArgs{ContainerID: testContainerID, StdinData: []byte(conf)} + if err := cmdDel(args); err != nil { + t.Fatalf("cmdDel for static IPAM returned error = %v, want nil", err) + } +} + +func TestCmdStatusValid(t *testing.T) { + args := &skel.CmdArgs{StdinData: []byte(fmt.Sprintf(`{"cniVersion":"%s"}`, testCNIVersion))} + if err := cmdStatus(args); err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestCmdStatusInvalidConfig(t *testing.T) { + args := &skel.CmdArgs{StdinData: []byte("not valid json")} + if err := cmdStatus(args); err == nil { + t.Fatal("expected error for invalid config, got nil") + } +} + +func TestCmdCheckStaticAlwaysPasses(t *testing.T) { + conf := confJSON(fmt.Sprintf(`"type":%q,"static_ip":"fd00::1234"`, testIPAMType)) + args := &skel.CmdArgs{ContainerID: testContainerID, StdinData: []byte(conf)} + if err := cmdCheck(args); err != nil { + t.Fatalf("unexpected error: %v", err) + } +} diff --git a/internal/cniipam/result.go b/internal/cniipam/result.go new file mode 100644 index 00000000..9991d77c --- /dev/null +++ b/internal/cniipam/result.go @@ -0,0 +1,81 @@ +// Copyright 2025 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cniipam + +import ( + "encoding/json" + "fmt" + "net" + + "github.com/containernetworking/cni/pkg/types" + type100 "github.com/containernetworking/cni/pkg/types/100" +) + +// BuildCNIResult constructs the type100.Result IPAM delegation returns — +// ips/routes only, no interfaces. The master plugin owns interface +// creation entirely; keeping interfaces out of this result is exactly what +// delegation exists to enforce (see the package doc comment). +func BuildCNIResult(cniVersion string, res *IPAMResult) *type100.Result { + result := &type100.Result{CNIVersion: cniVersion} + if res == nil { + return result + } + if res.IPv6Subnet != nil { + result.IPs = append(result.IPs, &type100.IPConfig{ + Address: *res.IPv6Subnet, + Gateway: res.IPv6Gateway, + }) + } + if res.IPv4Address != nil { + result.IPs = append(result.IPs, &type100.IPConfig{ + Address: net.IPNet{IP: res.IPv4Address, Mask: net.CIDRMask(32, 32)}, + Gateway: res.IPv4Gateway, + }) + } + for _, r := range res.Routes { + result.Routes = append(result.Routes, &types.Route{Dst: *r}) + } + return result +} + +// ResultToIPAMResult converts a CNI result — as returned by +// github.com/containernetworking/cni/pkg/ipam.ExecAdd back to the master +// plugin that just delegated an ADD — into the local shape callers apply +// directly (configureInterfaceInNetns for veth, or read straight into a +// tap/BGP-advertisement result). Marshals and re-parses via type100 rather +// than a direct type assertion, since the concrete type returned by +// ExecAdd depends on CNI version negotiation (mirrors the same pattern +// internal/cni's own prevResult validation already uses). +func ResultToIPAMResult(res types.Result) (*IPAMResult, error) { + jsonBytes, err := json.Marshal(res) + if err != nil { + return nil, fmt.Errorf("marshal IPAM result: %w", err) + } + parsed, err := type100.NewResult(jsonBytes) + if err != nil { + return nil, fmt.Errorf("parse IPAM result: %w", err) + } + versioned, err := type100.GetResult(parsed) + if err != nil { + return nil, fmt.Errorf("get IPAM result: %w", err) + } + + r := &IPAMResult{} + for _, ipConf := range versioned.IPs { + if ipConf.Address.IP.To4() != nil { + r.IPv4Address = ipConf.Address.IP + r.IPv4Gateway = ipConf.Gateway + continue + } + subnet := ipConf.Address + r.IPv6Subnet = &subnet + r.IPv6Gateway = ipConf.Gateway + } + for _, rt := range versioned.Routes { + dst := rt.Dst + r.Routes = append(r.Routes, &dst) + } + return r, nil +} diff --git a/internal/cniipam/result_test.go b/internal/cniipam/result_test.go new file mode 100644 index 00000000..2d3aee1c --- /dev/null +++ b/internal/cniipam/result_test.go @@ -0,0 +1,111 @@ +// Copyright 2025 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cniipam + +import ( + "net" + "testing" + + type100 "github.com/containernetworking/cni/pkg/types/100" +) + +func mustParseCIDR(t *testing.T, cidr string) *net.IPNet { + t.Helper() + _, ipnet, err := net.ParseCIDR(cidr) + if err != nil { + t.Fatalf("parse CIDR %q: %v", cidr, err) + } + return ipnet +} + +func TestBuildCNIResultNil(t *testing.T) { + result := BuildCNIResult(testCNIVersion, nil) + if result.CNIVersion != testCNIVersion { + t.Errorf("CNIVersion = %q, want %q", result.CNIVersion, testCNIVersion) + } + if len(result.IPs) != 0 { + t.Errorf("IPs = %v, want empty", result.IPs) + } +} + +func TestBuildCNIResultDualStack(t *testing.T) { + subnet := mustParseCIDR(t, "fd00:10:ff01::1234/96") + gw6 := net.ParseIP("fd00:10:ff01::1") + addr4 := net.ParseIP("10.128.0.5") + gw4 := net.ParseIP("10.128.0.1") + route6 := mustParseCIDR(t, "::/0") + + result := BuildCNIResult(testCNIVersion, &IPAMResult{ + IPv6Subnet: subnet, IPv6Gateway: gw6, + IPv4Address: addr4, IPv4Gateway: gw4, + Routes: []*net.IPNet{route6}, + }) + + if len(result.IPs) != 2 { + t.Fatalf("IPs count = %d, want 2", len(result.IPs)) + } + if result.IPs[0].Address.String() != subnet.String() { + t.Errorf("IPs[0].Address = %v, want %v", result.IPs[0].Address, subnet) + } + if !result.IPs[0].Gateway.Equal(gw6) { + t.Errorf("IPs[0].Gateway = %v, want %v", result.IPs[0].Gateway, gw6) + } + wantMask := net.CIDRMask(32, 32).String() + if result.IPs[1].Address.IP.String() != addr4.String() || result.IPs[1].Address.Mask.String() != wantMask { + t.Errorf("IPs[1].Address = %v, want %s/32", result.IPs[1].Address, addr4) + } + if len(result.Routes) != 1 { + t.Errorf("Routes count = %d, want 1", len(result.Routes)) + } + // No interfaces should ever be set — that's the master plugin's job. + if len(result.Interfaces) != 0 { + t.Errorf("Interfaces = %v, want empty (IPAM delegation never returns interfaces)", result.Interfaces) + } +} + +func TestResultToIPAMResultRoundTrip(t *testing.T) { + subnet := mustParseCIDR(t, "fd00:10:ff01::1234/96") + gw6 := net.ParseIP("fd00:10:ff01::1") + addr4 := net.ParseIP("10.128.0.5") + gw4 := net.ParseIP("10.128.0.1") + route6 := mustParseCIDR(t, "::/0") + + built := BuildCNIResult(testCNIVersion, &IPAMResult{ + IPv6Subnet: subnet, IPv6Gateway: gw6, + IPv4Address: addr4, IPv4Gateway: gw4, + Routes: []*net.IPNet{route6}, + }) + + roundTripped, err := ResultToIPAMResult(built) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if roundTripped.IPv6Subnet == nil || roundTripped.IPv6Subnet.String() != subnet.String() { + t.Errorf("IPv6Subnet = %v, want %v", roundTripped.IPv6Subnet, subnet) + } + if !roundTripped.IPv6Gateway.Equal(gw6) { + t.Errorf("IPv6Gateway = %v, want %v", roundTripped.IPv6Gateway, gw6) + } + if roundTripped.IPv4Address == nil || !roundTripped.IPv4Address.Equal(addr4) { + t.Errorf("IPv4Address = %v, want %v", roundTripped.IPv4Address, addr4) + } + if !roundTripped.IPv4Gateway.Equal(gw4) { + t.Errorf("IPv4Gateway = %v, want %v", roundTripped.IPv4Gateway, gw4) + } + if len(roundTripped.Routes) != 1 { + t.Fatalf("Routes count = %d, want 1", len(roundTripped.Routes)) + } +} + +func TestResultToIPAMResultEmpty(t *testing.T) { + empty := &type100.Result{CNIVersion: testCNIVersion} + res, err := ResultToIPAMResult(empty) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if res.IPv6Subnet != nil || res.IPv4Address != nil { + t.Errorf("res = %+v, want all-nil fields", res) + } +} diff --git a/internal/cniipam/types.go b/internal/cniipam/types.go new file mode 100644 index 00000000..3fb8e686 --- /dev/null +++ b/internal/cniipam/types.go @@ -0,0 +1,84 @@ +// Copyright 2025 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +// Package cniipam implements galactic-ipam, the delegated CNI IPAM plugin +// in the galactic CNI chain (see github.com/containernetworking/cni/pkg/ipam +// for the delegation protocol both master plugins, galactic-cni and +// galactic-tap-cni, invoke this through via ExecAdd/ExecDel/ExecCheck). +// +// Explicit contract: a master plugin delegates here if and only if its own +// "ipam" block is present at all — no environment variable or sibling +// config field can trigger or suppress that decision (that's the master's +// own call, made before this package is ever invoked). Once delegated to, +// mode selection is entirely this package's own: presence of +// ipam.static_ip selects the static single-address path; otherwise +// ipam.ipv6_subnet/ipv4_subnet (either family alone, or both) select the +// pool path. GALACTIC_IPAM_ENABLE_LOCAL_IPAM only fills in a default IPv6 +// pool CIDR when the ipam block is present but specifies neither +// static_ip nor a subnet — it can no longer manufacture an ipam block out +// of thin air the way its GALACTIC_CNI_ENABLE_LOCAL_IPAM predecessor did. +// +// Allocation state persists in on-disk marker files (internal/cni/ipam), +// keyed by containerID, so this package never needs a Kubernetes client +// at all: DEL looks its own allocation up locally instead of reading it +// back from a BGPAdvertisement CRD annotation galactic-bgp wrote. +package cniipam + +import ( + "net" + + "github.com/containernetworking/cni/pkg/types" +) + +// IPAM is the JSON shape of a CNI config's "ipam" block, as galactic-ipam +// itself parses it (the master plugins each embed the same shape as +// *IPAM in their own PluginConf, since the full netconf — including this +// block — is what gets passed through to the delegate unmodified). +type IPAM struct { + // Type names the delegated binary (e.g. "galactic-ipam") — a CNI IPAM + // delegation implementation detail (github.com/containernetworking/cni/ + // pkg/ipam.ExecAdd/ExecDel read this to know which binary to exec), not + // a mode selector. Mode is decided from which of the fields below are + // present instead — see the package doc comment. + Type string `json:"type"` + StaticIP string `json:"static_ip,omitempty"` + IPv6Subnet string `json:"ipv6_subnet,omitempty"` + IPv4Subnet string `json:"ipv4_subnet,omitempty"` + AddressFamilies []string `json:"address_families,omitempty"` + Routes []Route `json:"routes,omitempty"` + Addresses []Address `json:"addresses,omitempty"` +} + +// Route describes a static route to install. +type Route struct { + Dst string `json:"dst"` + GW string `json:"gw,omitempty"` +} + +// Address describes a static IP address assignment. +type Address struct { + Address string `json:"address"` +} + +// IPAMResult holds the allocation details a master plugin uses to build +// its own CNI result and, for veth, to configure the guest interface. +// IPv4Address/IPv4Gateway are nil when the attachment is IPv6-only. +type IPAMResult struct { + IPv6Subnet *net.IPNet + IPv6Gateway net.IP + IPv4Address net.IP + IPv4Gateway net.IP + Routes []*net.IPNet +} + +// pluginConf is the full CNI config document galactic-ipam receives as +// args.StdinData — the same document the master plugin itself parsed, +// passed through unmodified per the IPAM delegation protocol. Only the +// "ipam" key (plus cniVersion, for result versioning) is ever read; the +// master-plugin-specific fields (vpc, vpcattachment, terminations, ...) +// are present in the JSON but simply ignored here. +type pluginConf struct { + types.PluginConf + IPAM *IPAM `json:"ipam"` +} diff --git a/internal/cnitap/config.go b/internal/cnitap/config.go index e399a502..88463497 100644 --- a/internal/cnitap/config.go +++ b/internal/cnitap/config.go @@ -9,7 +9,6 @@ import ( "errors" "fmt" "log/slog" - "net" "os" "path/filepath" "strings" @@ -18,7 +17,6 @@ import ( type100 "github.com/containernetworking/cni/pkg/types/100" "go.datum.net/galactic/internal/cni/hostconf" - "go.datum.net/galactic/internal/cniipam" "go.datum.net/galactic/internal/config" ) @@ -44,16 +42,6 @@ const ( errVPCAttachmentRequired = "vpcattachment is required and must be a non-empty base62 string" ) -const ( - maxIPv6SubnetPrefixLen = 96 - maxIPv4SubnetPrefixLen = 32 -) - -const ( - addressFamilyIPv6 = "ipv6" - addressFamilyIPv4 = "ipv4" -) - // isValidBase62 reports whether s contains only valid base62 characters // ([0-9a-zA-Z]) and is non-empty. func isValidBase62(s string) bool { @@ -268,64 +256,11 @@ func parseConf(data []byte) (*PluginConf, error) { setupLogging(cniConfig.LogFile, cniConfig.LogLevel) slog.Debug("CNI config received", "stdin", string(data)) - localIPAM := config.CNIGetEnableLocalIPAM() - cniipam.SetEnableLocalIPAM(localIPAM) - if localIPAM && conf.IPAM == nil { - return nil, &types.Error{Code: 7, Msg: "local IPAM is enabled, but no 'ipam' block is present in the configuration"} - } - - if conf.IPv6Subnet != "" { - ip, mask, err := net.ParseCIDR(conf.IPv6Subnet) - if err != nil { - return nil, &types.Error{Code: 7, Msg: fmt.Sprintf( - "invalid CIDR value for field 'ipv6_subnet': %q", sanitizeForError(conf.IPv6Subnet)), - } - } - if ip.To4() != nil { - return nil, &types.Error{Code: 7, Msg: fmt.Sprintf( - "ipv6_subnet must be an IPv6 CIDR, got IPv4: %q", sanitizeForError(conf.IPv6Subnet)), - } - } - if prefixLen, _ := mask.Mask.Size(); prefixLen > maxIPv6SubnetPrefixLen { - return nil, &types.Error{Code: 7, Msg: fmt.Sprintf( - "ipv6_subnet prefix length %d exceeds maximum of %d: %q", - prefixLen, maxIPv6SubnetPrefixLen, sanitizeForError(conf.IPv6Subnet)), - } - } - } - if conf.IPv4Subnet != "" { - ip, mask, err := net.ParseCIDR(conf.IPv4Subnet) - if err != nil { - return nil, &types.Error{Code: 7, Msg: fmt.Sprintf( - "invalid CIDR value for field 'ipv4_subnet': %q", sanitizeForError(conf.IPv4Subnet)), - } - } - if ip.To4() == nil { - return nil, &types.Error{Code: 7, Msg: fmt.Sprintf( - "ipv4_subnet must be an IPv4 CIDR, got IPv6: %q", sanitizeForError(conf.IPv4Subnet)), - } - } - if prefixLen, _ := mask.Mask.Size(); prefixLen > maxIPv4SubnetPrefixLen { - return nil, &types.Error{Code: 7, Msg: fmt.Sprintf( - "ipv4_subnet prefix length %d exceeds maximum of %d: %q", - prefixLen, maxIPv4SubnetPrefixLen, sanitizeForError(conf.IPv4Subnet)), - } - } - } - if len(conf.AddressFamilies) == 0 { - conf.AddressFamilies = []string{addressFamilyIPv6} - } else { - for _, af := range conf.AddressFamilies { - switch af { - case addressFamilyIPv6, addressFamilyIPv4: - default: - return nil, &types.Error{Code: 7, Msg: fmt.Sprintf( - "invalid address_families entry %q: must be %q or %q", - sanitizeForError(af), addressFamilyIPv6, addressFamilyIPv4), - } - } - } - } + // Whether IPAM runs at all is decided entirely by "ipam" block + // presence — see internal/cni's own parseConf for the full reasoning, + // identical here. Addressing fields and their validation live inside + // internal/cniipam, invoked via delegation with this plugin's own + // StdinData passed straight through unmodified. if conf.PrevResult != nil { if err := validatePrevResult(conf.PrevResult); err != nil { diff --git a/internal/cnitap/ops_add.go b/internal/cnitap/ops_add.go index b8af4533..f352441d 100644 --- a/internal/cnitap/ops_add.go +++ b/internal/cnitap/ops_add.go @@ -13,6 +13,7 @@ import ( "github.com/containernetworking/cni/pkg/skel" "github.com/containernetworking/cni/pkg/types" + "github.com/containernetworking/plugins/pkg/ipam" "github.com/vishvananda/netlink" "go.datum.net/galactic/internal/cni/nadpatch" @@ -58,6 +59,13 @@ func cmdAdd(args *skel.CmdArgs) (err error) { vpcAttachment: pluginConf.VPCAttachment, namespace: namespace, } + // Record IPAM delegation intent up front, before the ExecAdd call + // below ever runs — see resourceTracker's ipamDelegated doc comment. + if pluginConf.IPAM != nil { + tracker.ipamDelegated = true + tracker.ipamType = pluginConf.IPAM.Type + tracker.ipamStdin = args.StdinData + } rollbackCtx, rollbackCancel := context.WithTimeout(context.Background(), cniTimeout) defer func() { @@ -109,11 +117,20 @@ func cmdAdd(args *skel.CmdArgs) (err error) { slog.Debug("ADD: termination routes installed", "count", tracker.routesCreated, "dev", dev) } - // Allocate IPAM for the tap interface. The VM manages its own guest - // interface; the CNI only configures the host side. - ipamResult, err := cniipam.Allocate(args, allocConfig(pluginConf)) - if err != nil { - return fmt.Errorf("allocate IPAM: %w", err) + // Allocate IPAM for the tap interface via delegation (only if pluginConf + // carries an "ipam" block at all — see internal/cniipam's doc comment + // for the explicit contract). The VM manages its own guest interface; + // this plugin only configures the host side. + var ipamResult *cniipam.IPAMResult + if pluginConf.IPAM != nil { + cniResult, err := ipam.ExecAdd(pluginConf.IPAM.Type, args.StdinData) + if err != nil { + return fmt.Errorf("delegate to %s ADD: %w", pluginConf.IPAM.Type, err) + } + ipamResult, err = cniipam.ResultToIPAMResult(cniResult) + if err != nil { + return fmt.Errorf("convert IPAM result: %w", err) + } } if ipamResult != nil { slog.Debug("ADD: IPAM allocated", "containerID", args.ContainerID, diff --git a/internal/cnitap/ops_check.go b/internal/cnitap/ops_check.go index 7e89229a..df5a874a 100644 --- a/internal/cnitap/ops_check.go +++ b/internal/cnitap/ops_check.go @@ -18,6 +18,7 @@ import ( "github.com/containernetworking/cni/pkg/skel" "github.com/containernetworking/cni/pkg/types" type100 "github.com/containernetworking/cni/pkg/types/100" + "github.com/containernetworking/plugins/pkg/ipam" "github.com/vishvananda/netlink" "k8s.io/client-go/rest" ctrl "sigs.k8s.io/controller-runtime" @@ -54,6 +55,14 @@ func cmdCheck(args *skel.CmdArgs) error { } } + // Delegate CHECK to the IPAM plugin so a lost or corrupted allocation + // marker file is caught here too, not just at ADD/DEL time. + if pluginConf.IPAM != nil { + if err := ipam.ExecCheck(pluginConf.IPAM.Type, args.StdinData); err != nil { + errs = append(errs, fmt.Errorf("IPAM CHECK: %w", err)) + } + } + if len(errs) > 0 { err := fmt.Errorf("CHECK failed: %w", errors.Join(errs...)) slog.Error("CHECK: failed", "err", err, "containerID", args.ContainerID, diff --git a/internal/cnitap/ops_del.go b/internal/cnitap/ops_del.go index d9e05cfb..0ff894c7 100644 --- a/internal/cnitap/ops_del.go +++ b/internal/cnitap/ops_del.go @@ -10,8 +10,7 @@ import ( "github.com/containernetworking/cni/pkg/skel" "github.com/containernetworking/cni/pkg/types" type100 "github.com/containernetworking/cni/pkg/types/100" - - "go.datum.net/galactic/internal/cniipam" + "github.com/containernetworking/plugins/pkg/ipam" ) // cmdDel mirrors internal/cni's own cmdDel, minus everything guest-netns @@ -31,12 +30,9 @@ func cmdDel(args *skel.CmdArgs) error { } vpc, vpcAtt := pluginConf.VPC, pluginConf.VPCAttachment - cfg := allocConfig(pluginConf) - if cniipam.WantsIPAM(cfg) { - if k8s, err := newK8sClient(); err == nil { - cniipam.Deallocate(args, cfg, k8s) - } else { - slog.Warn("DEL: failed to create k8s client, skipping IPAM deallocation", "err", err, + if pluginConf.IPAM != nil { + if err := ipam.ExecDel(pluginConf.IPAM.Type, args.StdinData); err != nil { + slog.Warn("DEL: IPAM delegation failed, allocation may not have been released", "err", err, "containerID", args.ContainerID) } } diff --git a/internal/cnitap/resource.go b/internal/cnitap/resource.go index 28bcae04..9e3d0790 100644 --- a/internal/cnitap/resource.go +++ b/internal/cnitap/resource.go @@ -9,6 +9,7 @@ import ( "fmt" "log/slog" + "github.com/containernetworking/plugins/pkg/ipam" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" @@ -63,6 +64,15 @@ type resourceTracker struct { ebpfRegistered bool ebpfBlock uint64 ebpfArgument uint16 + + // ipamDelegated, ipamType, and ipamStdin record enough to release the + // IPAM allocation during rollback — see internal/cni's own + // resourceTracker for the full doc comment on why this fires + // unconditionally on "ipam" block presence rather than only after a + // confirmed ExecAdd. + ipamDelegated bool + ipamType string + ipamStdin []byte } func (rt *resourceTracker) cleanup(ctx context.Context) { @@ -112,6 +122,14 @@ func (rt *resourceTracker) cleanup(ctx context.Context) { } } + if rt.ipamDelegated { + if err := ipam.ExecDel(rt.ipamType, rt.ipamStdin); err != nil { + slog.Error("Rollback: failed to release IPAM allocation", "err", err, "ipamType", rt.ipamType) + } else { + slog.Debug("Rollback: released IPAM allocation", "ipamType", rt.ipamType) + } + } + if err := tap.Delete(rt.vpc, rt.vpcAttachment); err != nil { slog.Error("Rollback: failed to delete tap", "err", err, "vpc", rt.vpc, "vpcAttachment", rt.vpcAttachment) diff --git a/internal/cnitap/types.go b/internal/cnitap/types.go index 17d72fb8..189a7c8a 100644 --- a/internal/cnitap/types.go +++ b/internal/cnitap/types.go @@ -25,31 +25,19 @@ type Termination struct { // PluginConf is the CNI plugin configuration passed via stdin on each // invocation of galactic-tap-cni. +// +// IPAM addressing fields live entirely inside the "ipam" block — see +// go.datum.net/galactic/internal/cniipam's doc comment for the explicit +// delegation contract. type PluginConf struct { types.PluginConf - VPC string `json:"vpc"` - VPCAttachment string `json:"vpcattachment"` - MTU int `json:"mtu,omitempty"` - Terminations []Termination `json:"terminations,omitempty"` - IPAM *cniipam.IPAM `json:"ipam"` - Namespace string `json:"namespace,omitempty"` - IPv6Subnet string `json:"ipv6_subnet,omitempty"` - IPv4Subnet string `json:"ipv4_subnet,omitempty"` - AddressFamilies []string `json:"address_families,omitempty"` + VPC string `json:"vpc"` + VPCAttachment string `json:"vpcattachment"` + MTU int `json:"mtu,omitempty"` + Terminations []Termination `json:"terminations,omitempty"` + IPAM *cniipam.IPAM `json:"ipam"` + Namespace string `json:"namespace,omitempty"` } // HostConf holds node-local settings read from /etc/cni/net.d/10-galactic.conflist. type HostConf = hostconf.HostConf - -// allocConfig adapts pluginConf's fields into cniipam.AllocConfig. -func allocConfig(pluginConf *PluginConf) cniipam.AllocConfig { - return cniipam.AllocConfig{ - VPC: pluginConf.VPC, - VPCAttachment: pluginConf.VPCAttachment, - Namespace: pluginConf.Namespace, - IPAM: pluginConf.IPAM, - IPv6Subnet: pluginConf.IPv6Subnet, - IPv4Subnet: pluginConf.IPv4Subnet, - AddressFamilies: pluginConf.AddressFamilies, - } -} diff --git a/internal/config/ipam.go b/internal/config/ipam.go new file mode 100644 index 00000000..7c755b1c --- /dev/null +++ b/internal/config/ipam.go @@ -0,0 +1,31 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package config + +import ( + "os" + "strings" +) + +// EnvIPAMEnableLocalIPAM is galactic-ipam's own local-IPAM default-filler +// flag, read fresh on every invocation (mirrors CNIConfig's env-only +// resolution — galactic-ipam has no conflist/kubeconfig to read a +// middle-tier value from, since it has no k8s dependency at all). +// +// Renamed from the historical GALACTIC_CNI_ENABLE_LOCAL_IPAM now that the +// flag belongs entirely to galactic-ipam: it's no longer a trigger deciding +// whether IPAM runs at all (that's the "ipam" block's presence, decided by +// the master plugin before it ever delegates) — only a default-filler for +// when the ipam block is present but under-specified. See +// go.datum.net/galactic/internal/cniipam's own doc comment. +const EnvIPAMEnableLocalIPAM = "GALACTIC_IPAM_ENABLE_LOCAL_IPAM" + +// IPAMGetEnableLocalIPAM reports whether galactic-ipam's local-IPAM +// default-filler is enabled via environment variable. Returns false if the +// variable is unset or not "true". +func IPAMGetEnableLocalIPAM() bool { + val := os.Getenv(EnvIPAMEnableLocalIPAM) + return strings.EqualFold(val, "true") +} diff --git a/internal/installer/installer.go b/internal/installer/installer.go index 4a9c7d4b..011d7c11 100644 --- a/internal/installer/installer.go +++ b/internal/installer/installer.go @@ -67,6 +67,7 @@ var ( SADir = "/var/run/secrets/kubernetes.io/serviceaccount" SourceCNIBinary = "/galactic-cni" SourceTapCNIBinary = "/galactic-tap-cni" + SourceIPAMBinary = "/galactic-ipam" SourceHostDeviceBinary = "/host-device" ) @@ -268,6 +269,9 @@ func Bootstrap(ctx context.Context, nodeName string) error { if err := atomicCopyFile(SourceTapCNIBinary, filepath.Join(HostBinDir, "galactic-tap-cni"), 0755); err != nil { return fmt.Errorf("copy galactic-tap-cni binary: %w", err) } + if err := atomicCopyFile(SourceIPAMBinary, filepath.Join(HostBinDir, "galactic-ipam"), 0755); err != nil { + return fmt.Errorf("copy galactic-ipam binary: %w", err) + } if err := atomicCopyFile(SourceHostDeviceBinary, filepath.Join(HostBinDir, "host-device"), 0755); err != nil { return fmt.Errorf("copy host-device binary: %w", err) } diff --git a/internal/installer/installer_test.go b/internal/installer/installer_test.go index cd3ef050..2041da33 100644 --- a/internal/installer/installer_test.go +++ b/internal/installer/installer_test.go @@ -104,6 +104,7 @@ func TestBootstrap(t *testing.T) { // Create mock CNI source binary files SourceCNIBinary = filepath.Join(tmpDir, "source-galactic-cni") SourceTapCNIBinary = filepath.Join(tmpDir, "source-galactic-tap-cni") + SourceIPAMBinary = filepath.Join(tmpDir, "source-galactic-ipam") SourceHostDeviceBinary = filepath.Join(tmpDir, "source-host-device") if err := os.WriteFile(SourceCNIBinary, []byte("cni-content"), 0755); err != nil { t.Fatalf("write SourceCNIBinary: %v", err) @@ -111,6 +112,9 @@ func TestBootstrap(t *testing.T) { if err := os.WriteFile(SourceTapCNIBinary, []byte("tap-cni-content"), 0755); err != nil { t.Fatalf("write SourceTapCNIBinary: %v", err) } + if err := os.WriteFile(SourceIPAMBinary, []byte("ipam-content"), 0755); err != nil { + t.Fatalf("write SourceIPAMBinary: %v", err) + } if err := os.WriteFile(SourceHostDeviceBinary, []byte("host-device-content"), 0755); err != nil { t.Fatalf("write SourceHostDeviceBinary: %v", err) } @@ -168,6 +172,7 @@ func TestBootstrap(t *testing.T) { // Verify binaries copied assertBinaryCopied(t, filepath.Join(HostBinDir, "galactic-cni"), "cni-content") assertBinaryCopied(t, filepath.Join(HostBinDir, "galactic-tap-cni"), "tap-cni-content") + assertBinaryCopied(t, filepath.Join(HostBinDir, "galactic-ipam"), "ipam-content") // Verify conflist written conflist, err := loadHostConf(HostConflist) diff --git a/tests/e2e/e2e_test.go b/tests/e2e/e2e_test.go index 9a9ec417..f1df4ead 100644 --- a/tests/e2e/e2e_test.go +++ b/tests/e2e/e2e_test.go @@ -226,6 +226,14 @@ func TestCNITapInterface(t *testing.T) { // Write the CNI config to a file inside the pod, then run the plugin // with the config piped via stdin. The plugin reads config from stdin // (the CNI protocol) and CNI_NETNS from the environment. + // + // The "ipam" block's "type" now names the delegated binary + // (galactic-ipam), not a pool-vs-static mode selector -- this step + // rewired IPAM from an in-process call into real CNI IPAM delegation + // (github.com/containernetworking/plugins/pkg/ipam.ExecAdd), so + // "pool" is no longer a valid type value; presence of ipv6_subnet + // alone opts this config into pool IPAM (see internal/cniipam's doc + // comment and docs/cni/configuration.md). cniConf := `{ "cniVersion": "1.0.0", "name": "galactic", @@ -233,23 +241,25 @@ func TestCNITapInterface(t *testing.T) { "vpc": "1", "vpcattachment": "1", "ipam": { - "type": "pool" + "type": "galactic-ipam", + "ipv6_subnet": "fd00:e2e::/48" } }` // Step 1: write the CNI config and a wrapper script into the pod. - // Tap mode now runs IPAM allocation unconditionally (matching veth mode). - // GALACTIC_CNI_ENABLE_LOCAL_IPAM fills in default pool/subnet_len when - // omitted, but parseConf still requires an explicit "ipam" block to be - // present in the config (see docs/cni/configuration.md). + // CNI_PATH=/ lets IPAM delegation (galactic-tap-cni execs galactic-ipam + // via ipam.ExecAdd) find the delegate binary: every binary in the + // chain is copied to the image root by containers/galactic-cni/ + // Dockerfile (not /opt/cni/bin -- that path only exists on the real + // host once installer.Bootstrap's init container stages it there, + // which this test's pod never runs). script := `#!/bin/sh ip netns add e2e-tap-ns CNI_NETNS=/var/run/netns/e2e-tap-ns \ CNI_COMMAND=ADD \ CNI_CONTAINERID=e2e-tap-001 \ CNI_IFNAME=eth0 \ -CNI_PATH=/opt/cni/bin \ +CNI_PATH=/ \ NODE_NAME=` + nodeName() + ` \ -GALACTIC_CNI_ENABLE_LOCAL_IPAM=true \ /galactic-tap-cni < /tmp/cni.json ` _, err = kubectl(t.Context(), "exec", name, "--",