From 9f99f183a7a29139aeeccf9f7c4654a642fb5de0 Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Tue, 16 Dec 2025 16:29:01 +0100 Subject: [PATCH] vminitd: correctly support dual-stack network args vminitd was not correctly parsing -network args when both an IPv4 and an IPv6 address were provided. Only the latest 'addr' field was kept, overriding the previous one. Fix that by storing the IPv4 and IPv6 addrs separately, and introduce a unit test to verify this works correctly. Signed-off-by: Albin Kerouanton --- Makefile | 3 + cmd/vminitd/networking.go | 25 ++++-- cmd/vminitd/networking_test.go | 88 ++++++++++++++++++++ go.mod | 4 + go.sum | 2 + internal/shim/task/networking_unix.go | 8 +- internal/vminit/vmnetworking/vmnetworking.go | 62 +++++++++----- 7 files changed, 159 insertions(+), 33 deletions(-) create mode 100644 cmd/vminitd/networking_test.go diff --git a/Makefile b/Makefile index 9673f8f..f875037 100644 --- a/Makefile +++ b/Makefile @@ -165,3 +165,6 @@ verify-vendor: ## verify if all the go.mod/go.sum files are up-to-date @(cd ${TMPDIR}/nerdbox && ${GO} mod verify) diff -r -u ${ROOTDIR} ${TMPDIR}/nerdbox @rm -rf ${TMPDIR} + +test-unit: + go test -count=1 $(shell go list ./... | grep -v /integration) diff --git a/cmd/vminitd/networking.go b/cmd/vminitd/networking.go index e5452d0..1f88e35 100644 --- a/cmd/vminitd/networking.go +++ b/cmd/vminitd/networking.go @@ -33,19 +33,24 @@ type networks []vmnetworking.Network func (n *networks) String() string { ss := make([]string, 0, len(*n)) for _, nw := range *n { - ss = append(ss, fmt.Sprintf("mac=%s,addr=%s,dhcp=%t", nw.MAC, nw.Addr, nw.DHCP)) + fields := []string{"mac=" + nw.MAC.String()} + if nw.DHCP { + fields = append(fields, "dhcp=true") + } + if nw.Addr4.IsValid() { + fields = append(fields, "addr="+nw.Addr4.String()) + } + if nw.Addr6.IsValid() { + fields = append(fields, "addr="+nw.Addr6.String()) + } + ss = append(ss, strings.Join(fields, ",")) } return strings.Join(ss, " ") } func (n *networks) Set(value string) error { - kvs := strings.Split(value, ",") - if len(kvs) != 2 { - return fmt.Errorf("invalid network %q: expected format: mac=,[addr=|dhcp=]", value) - } - var nw vmnetworking.Network - for _, kv := range kvs { + for _, kv := range strings.Split(value, ",") { parts := strings.Split(kv, "=") if len(parts) != 2 { return fmt.Errorf("invalid network %q: expected format: mac=,[addr=|dhcp=]", value) @@ -62,7 +67,11 @@ func (n *networks) Set(value string) error { if err != nil { return fmt.Errorf("invalid IP address: %w", err) } - nw.Addr = addr + if addr.Addr().Is4() { + nw.Addr4 = addr + } else { + nw.Addr6 = addr + } case "dhcp": dhcp, err := strconv.ParseBool(parts[1]) if err != nil { diff --git a/cmd/vminitd/networking_test.go b/cmd/vminitd/networking_test.go new file mode 100644 index 0000000..a49779b --- /dev/null +++ b/cmd/vminitd/networking_test.go @@ -0,0 +1,88 @@ +//go:build linux + +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package main + +import ( + "net" + "net/netip" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestParseNetwork(t *testing.T) { + testscases := []struct { + name string + input string + want networks + }{ + { + name: "ipv4 only", + input: "mac=72:e2:03:9b:d8:0d,addr=172.17.0.2/16", + want: networks{ + { + MAC: net.HardwareAddr{0x72, 0xe2, 0x03, 0x9b, 0xd8, 0x0d}, + Addr4: netip.MustParsePrefix("172.17.0.2/16"), + }, + }, + }, + { + name: "ipv6 only", + input: "mac=72:e2:03:9b:d8:0d,addr=fd06:322:d419::2/64", + want: networks{ + { + MAC: net.HardwareAddr{0x72, 0xe2, 0x03, 0x9b, 0xd8, 0x0d}, + Addr6: netip.MustParsePrefix("fd06:322:d419::2/64"), + }, + }, + }, + { + name: "dual stack", + input: "mac=72:e2:03:9b:d8:0d,addr=172.17.0.2/16,addr=fd06:322:d419::2/64", + want: networks{ + { + MAC: net.HardwareAddr{0x72, 0xe2, 0x03, 0x9b, 0xd8, 0x0d}, + Addr4: netip.MustParsePrefix("172.17.0.2/16"), + Addr6: netip.MustParsePrefix("fd06:322:d419::2/64"), + }, + }, + }, + { + name: "with dhcp", + input: "mac=72:e2:03:9b:d8:0d,dhcp=true", + want: networks{ + { + MAC: net.HardwareAddr{0x72, 0xe2, 0x03, 0x9b, 0xd8, 0x0d}, + DHCP: true, + }, + }, + }, + } + + for _, tc := range testscases { + t.Run(tc.name, func(t *testing.T) { + var n networks + err := n.Set(tc.input) + assert.NoError(t, err) + assert.Equal(t, tc.want, n) + // Try to convert back the parsed struct into a string to check if it matches the input. + assert.Equal(t, tc.input, n.String()) + }) + } +} diff --git a/go.mod b/go.mod index c8905b6..49c284b 100644 --- a/go.mod +++ b/go.mod @@ -24,6 +24,7 @@ require ( github.com/mdlayher/vsock v1.2.1 github.com/moby/sys/userns v0.1.0 github.com/opencontainers/runtime-spec v1.3.0 + github.com/stretchr/testify v1.11.1 github.com/vishvananda/netlink v1.3.1 github.com/vishvananda/netns v0.0.5 golang.org/x/sync v0.18.0 @@ -37,6 +38,7 @@ require ( github.com/cilium/ebpf v0.16.0 // indirect github.com/containerd/continuity v0.4.5 // indirect github.com/coreos/go-systemd/v22 v22.6.0 // indirect + github.com/davecgh/go-spew v1.1.1 // indirect github.com/docker/go-units v0.5.0 // indirect github.com/go-logr/logr v1.4.3 // indirect github.com/go-logr/stdr v1.2.2 // indirect @@ -52,6 +54,7 @@ require ( github.com/opencontainers/image-spec v1.1.1 // indirect github.com/pierrec/lz4/v4 v4.1.14 // indirect github.com/pkg/errors v0.9.1 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect github.com/sirupsen/logrus v1.9.3 // indirect github.com/u-root/uio v0.0.0-20230220225925-ffce2a382923 // indirect go.opencensus.io v0.24.0 // indirect @@ -64,6 +67,7 @@ require ( golang.org/x/text v0.28.0 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20250707201910-8d1bb00bc6a7 // indirect google.golang.org/grpc v1.75.1 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect ) replace github.com/containerd/containerd/v2 => github.com/dmcgowan/containerd/v2 v2.2.0-beta.0-erofs-darwin.3 diff --git a/go.sum b/go.sum index 143f909..0cf6e98 100644 --- a/go.sum +++ b/go.sum @@ -255,6 +255,8 @@ google.golang.org/protobuf v1.25.0/go.mod h1:9JNX74DMeImyA3h4bdi1ymwjUzf21/xIlba google.golang.org/protobuf v1.36.10 h1:AYd7cD/uASjIL6Q9LiTjz8JLcrh/88q5UObnmY3aOOE= google.golang.org/protobuf v1.36.10/go.mod h1:HTf+CrKn2C3g5S8VImy6tdcUvCska2kB7j23XfzDpco= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk= +gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/internal/shim/task/networking_unix.go b/internal/shim/task/networking_unix.go index f99666d..e894837 100644 --- a/internal/shim/task/networking_unix.go +++ b/internal/shim/task/networking_unix.go @@ -195,15 +195,17 @@ func (p *networksProvider) SetupVM(ctx context.Context, vmi vm.Instance) error { func (p *networksProvider) InitArgs() []string { args := make([]string, 0, len(p.nws)) for _, nw := range p.nws { + fields := []string{"mac=" + nw.mac.String()} if nw.dhcp { - args = append(args, fmt.Sprintf("-network=mac=%s,dhcp=true", nw.mac)) + fields = append(fields, "dhcp=true") } if nw.addr4.IsValid() { - args = append(args, fmt.Sprintf("-network=mac=%s,addr=%s", nw.mac, nw.addr4)) + fields = append(fields, "addr="+nw.addr4.String()) } if nw.addr6.IsValid() { - args = append(args, fmt.Sprintf("-network=mac=%s,addr=%s", nw.mac, nw.addr6)) + fields = append(fields, "addr="+nw.addr6.String()) } + args = append(args, "-network="+strings.Join(fields, ",")) } return args } diff --git a/internal/vminit/vmnetworking/vmnetworking.go b/internal/vminit/vmnetworking/vmnetworking.go index d6a317f..c75829c 100644 --- a/internal/vminit/vmnetworking/vmnetworking.go +++ b/internal/vminit/vmnetworking/vmnetworking.go @@ -38,16 +38,17 @@ import ( ) type Network struct { - MAC net.HardwareAddr - Addr netip.Prefix - DHCP bool + MAC net.HardwareAddr + Addr4 netip.Prefix + Addr6 netip.Prefix + DHCP bool } func (nw Network) Validate() error { - if nw.MAC == nil || (!nw.Addr.IsValid() && !nw.DHCP) { - return errors.New("must specify mac and either addr or dhcp") + if nw.MAC == nil || (!nw.Addr4.IsValid() && !nw.Addr6.IsValid() && !nw.DHCP) { + return errors.New("must specify either addr or dhcp") } - if nw.Addr.IsValid() && nw.DHCP { + if (nw.Addr4.IsValid() || nw.Addr6.IsValid()) && nw.DHCP { return errors.New("cannot specify both addr and dhcp") } return nil @@ -126,11 +127,18 @@ func SetupVM(ctx context.Context, nws []Network, debug bool) (func(context.Conte }) } else { eg.Go(func() error { - ctx := log.WithLogger(ctx, log.G(ctx).WithField("addr", nw.Addr.String())) + ctx := log.WithLogger(ctx, log.G(ctx).WithFields(log.Fields{ + "addr4": nw.Addr4.String(), + "addr6": nw.Addr6.String(), + })) // Consider that the 1st assignable IP address in the subnet is // the gateway and select that as a potential default gateway. - gws[i] = nw.Addr.Masked().Addr().Next() + if nw.Addr4.IsValid() { + gws[i] = nw.Addr4.Masked().Addr().Next() + } else { + gws[i] = nw.Addr6.Masked().Addr().Next() + } return configureStatic(ctx, iface, nw) }) @@ -200,20 +208,30 @@ func listVirtioIfaces() (map[string]netlink.Link, error) { // configureStatic configures an interface with a static IP address. func configureStatic(ctx context.Context, iface netlink.Link, nw Network) error { - if err := netlink.AddrAdd(iface, &netlink.Addr{ - IPNet: &net.IPNet{ - IP: nw.Addr.Addr().AsSlice(), - Mask: net.CIDRMask(nw.Addr.Bits(), nw.Addr.Addr().BitLen()), - }, - // Disable DAD to avoid random delays until the IP address is ready - // and the VM gets external connectivity. - // The VMM, and its network provider, need to ensure that there's no - // conflicting IP addresses assigned to multiple VMs on the same - // network. - Flags: unix.IFA_F_PERMANENT | unix.IFA_F_NODAD, - }); err != nil { - log.G(ctx).WithError(err).Error("failed to add IP address to virtio interface") - return err + for _, prefix := range []netip.Prefix{nw.Addr4, nw.Addr6} { + if !prefix.IsValid() { + continue + } + + if err := netlink.AddrAdd(iface, &netlink.Addr{ + IPNet: &net.IPNet{ + IP: prefix.Addr().AsSlice(), + Mask: net.CIDRMask(prefix.Bits(), prefix.Addr().BitLen()), + }, + // Disable DAD to avoid random delays until the IP address is ready + // and the VM gets external connectivity. + // The VMM, and its network provider, need to ensure that there's no + // conflicting IP addresses assigned to multiple VMs on the same + // network. + Flags: unix.IFA_F_PERMANENT | unix.IFA_F_NODAD, + }); err != nil { + log.G(ctx).WithFields(log.Fields{ + "error": err, + "addr": prefix.String(), + "iface": iface.Attrs().Name, + }).Error("failed to add IP address to virtio interface") + return err + } } if err := netlink.LinkSetUp(iface); err != nil {