From 1257cbab663f6b146bd8f81380abfe008eba542b Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Fri, 18 Sep 2026 16:22:51 -0500 Subject: [PATCH] fix(webhook): stop attaching the default network twice The pod webhook set v1.multus-cni.io/default-network to the first interface and also listed it in k8s.v1.cni.cncf.io/networks, so Multus attached the same NAD as eth0 and net1. Only the remaining interfaces are now listed, and the networks annotation is omitted when there are none. Closes #28 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SYmkahHLMsqf3f8DYcsRaQ --- README.md | 2 +- internal/webhook/pod_webhook.go | 26 +++++++++++++++----- internal/webhook/pod_webhook_test.go | 36 +++++++++++++++++++++++++++- 3 files changed, 56 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index e73efdd..7792d85 100644 --- a/README.md +++ b/README.md @@ -14,7 +14,7 @@ Services defines Kubernetes Custom Resource Definitions for virtual tenant netwo The controller runs in a POP cell beside network-services-operator, compute and the workload providers. It turns a `NetworkContext` into a `VPC` identity; when a `NetworkInterface` claim is fulfilled it creates the `VPCAttachment` and the `NetworkAttachmentDefinition`, allocates the attachment identifier, and publishes the annotations a workload must carry; and it projects what the data plane reported back onto `VPCAttachment` and `NetworkInterface` status. -It also serves a mutating admission webhook that injects the Multus annotations into Pods labelled `networking.datumapis.com/inject-interfaces: "true"`, so Multus knowledge stays inside the one component that writes NetworkAttachmentDefinitions. It names the instance's interfaces in `k8s.v1.cni.cncf.io/networks`, and names the first of them in `v1.multus-cni.io/default-network` so the Pod's default network is the tenant's rather than the cluster CNI's. +It also serves a mutating admission webhook that injects the Multus annotations into Pods labelled `networking.datumapis.com/inject-interfaces: "true"`, so Multus knowledge stays inside the one component that writes NetworkAttachmentDefinitions. It names the instance's first interface in `v1.multus-cni.io/default-network`, so the Pod's default network is the tenant's rather than the cluster CNI's, and names the remaining interfaces in `k8s.v1.cni.cncf.io/networks`. The first interface is not repeated there, or Multus would attach it twice. It requires `--attachment-mode` (`Netns` or `Hypervisor`) — how guests in the cell consume an interface. There is no default, because defaulting would hand a microVM an interface it cannot use. diff --git a/internal/webhook/pod_webhook.go b/internal/webhook/pod_webhook.go index 919b279..791cb26 100644 --- a/internal/webhook/pod_webhook.go +++ b/internal/webhook/pod_webhook.go @@ -112,8 +112,7 @@ func (i *PodInterfaceInjector) Handle(ctx context.Context, req admission.Request if pod.Annotations == nil { pod.Annotations = map[string]string{} } - pod.Annotations[MultusNetworksAnnotation] = mergeNetworks(pod.Annotations[MultusNetworksAnnotation], networks) - pod.Annotations[MultusDefaultNetworkAnnotation] = networks[0] + injectNetworks(pod.Annotations, networks) pod.Annotations[InjectedInterfacesAnnotation] = strings.Join(injected, ",") patched, err := json.Marshal(pod) @@ -186,12 +185,27 @@ func instanceOwner(pod *corev1.Pod) (string, bool) { return "", false } +// injectNetworks makes the first network the Pod's default and lists the rest as +// additional networks. Multus attaches the default network as eth0 on its own, so +// listing it again would attach the same definition a second time as net1. +func injectNetworks(annotations map[string]string, networks []string) { + defaultNetwork := networks[0] + annotations[MultusDefaultNetworkAnnotation] = defaultNetwork + + merged := mergeNetworks(annotations[MultusNetworksAnnotation], networks[1:], defaultNetwork) + if merged == "" { + delete(annotations, MultusNetworksAnnotation) + return + } + annotations[MultusNetworksAnnotation] = merged +} + // mergeNetworks appends the resolved networks to whatever the Pod already asked -// for, without duplicating an entry. -func mergeNetworks(existing string, networks []string) string { +// for, without duplicating an entry or repeating the default network. +func mergeNetworks(existing string, networks []string, defaultNetwork string) string { merged := []string{} - for _, entry := range strings.Split(existing, ",") { - if entry = strings.TrimSpace(entry); entry != "" { + for entry := range strings.SplitSeq(existing, ",") { + if entry = strings.TrimSpace(entry); entry != "" && entry != defaultNetwork { merged = append(merged, entry) } } diff --git a/internal/webhook/pod_webhook_test.go b/internal/webhook/pod_webhook_test.go index 86b31af..b79327b 100644 --- a/internal/webhook/pod_webhook_test.go +++ b/internal/webhook/pod_webhook_test.go @@ -85,16 +85,50 @@ func TestMergeNetworks(t *testing.T) { want string }{ {"empty", "", []string{"ns/a"}, "ns/a"}, + {"nothing to add", "", nil, ""}, {"appends in order", "", []string{"ns/a", "ns/b"}, "ns/a,ns/b"}, {"preserves what the pod asked for", "ns/other", []string{"ns/a"}, "ns/other,ns/a"}, {"does not duplicate", "ns/a", []string{"ns/a"}, "ns/a"}, {"tolerates whitespace", " ns/other , ", []string{"ns/a"}, "ns/other,ns/a"}, + {"drops the default network", "ns/default,ns/other", []string{"ns/a"}, "ns/other,ns/a"}, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - if got := mergeNetworks(test.existing, test.networks); got != test.want { + if got := mergeNetworks(test.existing, test.networks, "ns/default"); got != test.want { t.Errorf("got %q, want %q", got, test.want) } }) } } + +func TestInjectNetworks(t *testing.T) { + tests := []struct { + name string + existing map[string]string + networks []string + wantDefault string + wantList string + wantListSet bool + }{ + {"single interface is only the default", map[string]string{}, + []string{"ns/a"}, "ns/a", "", false}, + {"additional interfaces exclude the default", map[string]string{}, + []string{"ns/a", "ns/b", "ns/c"}, "ns/a", "ns/b,ns/c", true}, + {"preserves what the pod asked for", map[string]string{MultusNetworksAnnotation: "ns/other"}, + []string{"ns/a"}, "ns/a", "ns/other", true}, + {"drops a default the pod already listed", map[string]string{MultusNetworksAnnotation: "ns/a"}, + []string{"ns/a", "ns/b"}, "ns/a", "ns/b", true}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + injectNetworks(test.existing, test.networks) + if got := test.existing[MultusDefaultNetworkAnnotation]; got != test.wantDefault { + t.Errorf("default network: got %q, want %q", got, test.wantDefault) + } + got, set := test.existing[MultusNetworksAnnotation] + if got != test.wantList || set != test.wantListSet { + t.Errorf("networks: got (%q, %v), want (%q, %v)", got, set, test.wantList, test.wantListSet) + } + }) + } +}