addressing review comments

thaJeztah · thaJeztah · commit cfe34c86824f · 2025-10-10T18:27:37.000+02:00
Signed-off-by: Sebastiaan van Stijn &lt;github@gone.nl&gt;
diff --git a/cli/command/completion/functions_test.go b/cli/command/completion/functions_test.go
@@ -6,7 +6,6 @@ import (
 	"sort"
 	"testing"
 
-	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/moby/moby/api/types/container"
 	"github.com/moby/moby/api/types/image"
 	"github.com/moby/moby/api/types/network"
@@ -155,7 +154,7 @@ func TestCompleteContainerNames(t *testing.T) {
 			}
 			comp := ContainerNames(fakeCLI{&fakeClient{
 				containerListFunc: func(opts client.ContainerListOptions) ([]container.Summary, error) {
-					assert.Check(t, is.DeepEqual(opts, tc.expOpts, cmpopts.IgnoreUnexported(client.ContainerListOptions{})))
+					assert.Check(t, is.DeepEqual(opts, tc.expOpts))
 					if tc.expDirective == cobra.ShellCompDirectiveError {
 						return nil, errors.New("some error occurred")
 					}
diff --git a/cli/command/container/opts.go b/cli/command/container/opts.go
@@ -1158,13 +1158,16 @@ func validateAttach(val string) (string, error) {
 }
 
 func toNetipAddrSlice(ips []string) []netip.Addr {
-	netips := make([]netip.Addr, 0, len(ips))
+	if len(ips) == 0 {
+		return nil
+	}
+	netIPs := make([]netip.Addr, 0, len(ips))
 	for _, ip := range ips {
 		addr, err := netip.ParseAddr(ip)
 		if err != nil {
 			continue
 		}
-		netips = append(netips, addr)
+		netIPs = append(netIPs, addr)
 	}
-	return netips
+	return netIPs
 }
diff --git a/cli/command/container/opts_test.go b/cli/command/container/opts_test.go
@@ -755,7 +755,7 @@ func TestParseNetworkConfig(t *testing.T) {
 			assert.NilError(t, err)
 			assert.DeepEqual(t, config.MacAddress, tc.expectedCfg.MacAddress) //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44.
 			assert.DeepEqual(t, hConfig.NetworkMode, tc.expectedHostCfg.NetworkMode)
-			assert.DeepEqual(t, nwConfig.EndpointsConfig, tc.expected, cmpopts.IgnoreUnexported(netip.Addr{}))
+			assert.DeepEqual(t, nwConfig.EndpointsConfig, tc.expected, cmpopts.EquateComparable(netip.Addr{}))
 		})
 	}
 }
diff --git a/cli/command/network/connect.go b/cli/command/network/connect.go
@@ -97,46 +97,39 @@ func convertDriverOpt(options []string) (map[string]string, error) {
 }
 
 func toNetipAddrSlice(ips []net.IP) []netip.Addr {
-	netips := make([]netip.Addr, 0, len(ips))
+	if len(ips) == 0 {
+		return nil
+	}
+	netIPs := make([]netip.Addr, 0, len(ips))
 	for _, ip := range ips {
-		netips = append(netips, toNetipAddr(ip))
+		netIPs = append(netIPs, toNetipAddr(ip))
 	}
-	return netips
+	return netIPs
 }
 
 func toNetipAddr(ip net.IP) netip.Addr {
-	if len(ip) == 0 {
-		return netip.Addr{}
-	}
-	if ip4 := ip.To4(); ip4 != nil {
-		a, _ := netip.AddrFromSlice(ip4)
-		return a
-	}
-	if ip16 := ip.To16(); ip16 != nil {
-		a, _ := netip.AddrFromSlice(ip16)
-		return a
-	}
-	return netip.Addr{}
+	a, _ := netip.AddrFromSlice(ip)
+	return a.Unmap()
 }
 
-func ipNetToPrefix(n net.IPNet) netip.Prefix {
-	if n.IP == nil {
-		return netip.Prefix{}
+// toPrefix converts n into a netip.Prefix. If n is not a valid IPv4 or IPV6
+// address, ToPrefix returns netip.Prefix{}, false.
+//
+// TODO(thaJeztah): create internal package similar to https://github.com/moby/moby/blob/0769fe708773892d6ac399ee137e71a777b35de7/daemon/internal/netiputil/netiputil.go#L21-L42
+func toPrefix(n net.IPNet) (netip.Prefix, bool) {
+	if ll := len(n.Mask); ll != net.IPv4len && ll != net.IPv6len {
+		return netip.Prefix{}, false
 	}
 
-	ip := n.IP.To4()
-	if ip == nil {
-		ip = n.IP.To16()
-	}
-	if ip == nil {
-		return netip.Prefix{}
+	addr, ok := netip.AddrFromSlice(n.IP)
+	if !ok {
+		return netip.Prefix{}, false
 	}
 
-	addr, ok := netip.AddrFromSlice(ip)
-	if !ok {
-		return netip.Prefix{}
+	ones, bits := n.Mask.Size()
+	if ones == 0 && bits == 0 {
+		return netip.Prefix{}, false
 	}
 
-	ones, _ := n.Mask.Size()
-	return netip.PrefixFrom(addr, ones)
+	return netip.PrefixFrom(addr.Unmap(), ones), true
 }
diff --git a/cli/command/network/connect_test.go b/cli/command/network/connect_test.go
@@ -62,7 +62,7 @@ func TestNetworkConnectWithFlags(t *testing.T) {
 	}
 	cli := test.NewFakeCli(&fakeClient{
 		networkConnectFunc: func(ctx context.Context, networkID, container string, config *network.EndpointSettings) error {
-			assert.Check(t, is.DeepEqual(expectedConfig, config, cmpopts.IgnoreUnexported(netip.Addr{})))
+			assert.Check(t, is.DeepEqual(expectedConfig, config, cmpopts.EquateComparable(netip.Addr{})))
 			return nil
 		},
 	})
diff --git a/cli/command/network/create.go b/cli/command/network/create.go
@@ -150,7 +150,6 @@ func createIPAMConfig(options ipamOptions) (*network.IPAM, error) {
 
 	// Populate non-overlapping subnets into consolidation map
 	for _, s := range options.subnets {
-		// TODO(thaJeztah): is all this validation needed on the CLI-side?
 		for k := range iData {
 			ok1, err := subnetMatches(s, k)
 			if err != nil {
@@ -164,7 +163,7 @@ func createIPAMConfig(options ipamOptions) (*network.IPAM, error) {
 				return nil, errors.New("multiple overlapping subnet configuration is not supported")
 			}
 		}
-		sn, err := parsePrefixOrAddr(s)
+		sn, err := netip.ParsePrefix(s)
 		if err != nil {
 			return nil, err
 		}
@@ -173,7 +172,6 @@ func createIPAMConfig(options ipamOptions) (*network.IPAM, error) {
 
 	// Validate and add valid ip ranges
 	for _, r := range options.ipRanges {
-		// TODO(thaJeztah): is all this validation needed on the CLI-side?
 		match := false
 		for _, s := range options.subnets {
 			ok, err := subnetMatches(s, r.String())
@@ -188,9 +186,10 @@ func createIPAMConfig(options ipamOptions) (*network.IPAM, error) {
 			if iData[s].IPRange.IsValid() {
 				return nil, fmt.Errorf("cannot configure multiple ranges (%s, %s) on the same subnet (%s)", r.String(), iData[s].IPRange.String(), s)
 			}
-			d := iData[s]
-			d.IPRange = ipNetToPrefix(r)
-			match = true
+			if ipRange, ok := toPrefix(r); ok {
+				iData[s].IPRange = ipRange
+				match = true
+			}
 		}
 		if !match {
 			return nil, fmt.Errorf("no matching subnet for range %s", r.String())
@@ -201,7 +200,6 @@ func createIPAMConfig(options ipamOptions) (*network.IPAM, error) {
 	for _, g := range options.gateways {
 		match := false
 		for _, s := range options.subnets {
-			// TODO(thaJeztah): is all this validation needed on the CLI-side?
 			ok, err := subnetMatches(s, g.String())
 			if err != nil {
 				return nil, err
@@ -278,20 +276,3 @@ func subnetMatches(subnet, data string) (bool, error) {
 
 	return s.Contains(ip), nil
 }
-
-// parsePrefixOrAddr parses s as a subnet in CIDR notation (e.g. "10.0.0.0/24").
-// If s does not include a prefix length, it is interpreted as a single-address
-// subnet using the full address width (/32 for IPv4 or /128 for IPv6).
-//
-// It returns the resulting netip.Prefix or an error if the input is invalid.
-func parsePrefixOrAddr(s string) (netip.Prefix, error) {
-	pfx, err := netip.ParsePrefix(s)
-	if err != nil {
-		addr, err := netip.ParseAddr(s)
-		if err != nil {
-			return netip.Prefix{}, fmt.Errorf("invalid address: %w", err)
-		}
-		pfx = netip.PrefixFrom(addr, addr.BitLen())
-	}
-	return pfx, nil
-}
diff --git a/cli/command/network/create_test.go b/cli/command/network/create_test.go
@@ -40,7 +40,7 @@ func TestNetworkCreateErrors(t *testing.T) {
 				"gateway":  "255.0.255.0", // FIXME(thaJeztah): this used to accept a CIDR ("255.0.255.0/24")
 				"subnet":   "10.1.2.0.30.50",
 			},
-			expectedError: `invalid address: ParseAddr("10.1.2.0.30.50")`,
+			expectedError: `netip.ParsePrefix("10.1.2.0.30.50"): no '/'`,
 		},
 		{
 			args: []string{"toto"},
@@ -168,7 +168,7 @@ func TestNetworkCreateWithFlags(t *testing.T) {
 	cli := test.NewFakeCli(&fakeClient{
 		networkCreateFunc: func(ctx context.Context, name string, options client.NetworkCreateOptions) (network.CreateResponse, error) {
 			assert.Check(t, is.Equal(expectedDriver, options.Driver), "not expected driver error")
-			assert.Check(t, is.DeepEqual(expectedOpts, options.IPAM.Config, cmpopts.IgnoreUnexported(netip.Addr{}, netip.Prefix{})), "not expected driver error")
+			assert.Check(t, is.DeepEqual(expectedOpts, options.IPAM.Config, cmpopts.EquateComparable(netip.Addr{}, netip.Prefix{})), "not expected driver error")
 			return network.CreateResponse{
 				ID: name,
 			}, nil
diff --git a/cli/command/service/opts.go b/cli/command/service/opts.go
@@ -1076,13 +1076,16 @@ const (
 )
 
 func toNetipAddrSlice(ips []string) []netip.Addr {
-	netips := make([]netip.Addr, 0, len(ips))
+	if len(ips) == 0 {
+		return nil
+	}
+	netIPs := make([]netip.Addr, 0, len(ips))
 	for _, ip := range ips {
 		addr, err := netip.ParseAddr(ip)
 		if err != nil {
 			continue
 		}
-		netips = append(netips, addr)
+		netIPs = append(netIPs, addr)
 	}
-	return netips
+	return netIPs
 }
diff --git a/cli/command/swarm/init.go b/cli/command/swarm/init.go
@@ -68,7 +68,7 @@ func newInitCommand(dockerCLI command.Cli) *cobra.Command {
 func runInit(ctx context.Context, dockerCLI command.Cli, flags *pflag.FlagSet, opts initOptions) error {
 	apiClient := dockerCLI.Client()
 
-	// TODO(thaJeztah): should we change opts.defaultAddrPools to be the right type? What formats does it accept?
+	// TODO(thaJeztah): change opts.defaultAddrPools a []netip.Prefix; see https://github.com/docker/cli/pull/6545#discussion_r2420361609
 	defaultAddrPool := make([]netip.Prefix, 0, len(opts.defaultAddrPools))
 	for _, p := range opts.defaultAddrPools {
 		if len(p.IP) == 0 {
diff --git a/cli/compose/convert/compose.go b/cli/compose/convert/compose.go
@@ -1,7 +1,6 @@
 package convert
 
 import (
-	"fmt"
 	"net/netip"
 	"os"
 	"strings"
@@ -86,7 +85,7 @@ func Networks(namespace Namespace, networks networkMap, servicesNetworks map[str
 				Driver: nw.Ipam.Driver,
 			}
 			for _, ipamConfig := range nw.Ipam.Config {
-				sn, _ := parsePrefixOrAddr(ipamConfig.Subnet) // TODO(thaJeztah): change Subnet field to netip.Prefix (but this would break "address only" formats.
+				sn, _ := netip.ParsePrefix(ipamConfig.Subnet)
 				createOpts.IPAM.Config = append(createOpts.IPAM.Config, network.IPAMConfig{
 					Subnet: sn,
 				})
@@ -202,20 +201,3 @@ func fileObjectConfig(namespace Namespace, name string, obj composetypes.FileObj
 		Data: data,
 	}, nil
 }
-
-// parsePrefixOrAddr parses s as a subnet in CIDR notation (e.g. "10.0.0.0/24").
-// If s does not include a prefix length, it is interpreted as a single-address
-// subnet using the full address width (/32 for IPv4 or /128 for IPv6).
-//
-// It returns the resulting netip.Prefix or an error if the input is invalid.
-func parsePrefixOrAddr(s string) (netip.Prefix, error) {
-	pfx, err := netip.ParsePrefix(s)
-	if err != nil {
-		addr, err := netip.ParseAddr(s)
-		if err != nil {
-			return netip.Prefix{}, fmt.Errorf("invalid address: %w", err)
-		}
-		pfx = netip.PrefixFrom(addr, addr.BitLen())
-	}
-	return pfx, nil
-}
diff --git a/cli/compose/convert/compose_test.go b/cli/compose/convert/compose_test.go
@@ -59,7 +59,7 @@ func TestNetworks(t *testing.T) {
 				Driver: "driver",
 				Config: []*composetypes.IPAMPool{
 					{
-						Subnet: "10.0.0.0",
+						Subnet: "10.0.0.0/32",
 					},
 				},
 			},
@@ -116,7 +116,7 @@ func TestNetworks(t *testing.T) {
 	}
 
 	networks, externals := Networks(namespace, source, serviceNetworks)
-	assert.DeepEqual(t, expected, networks, cmpopts.IgnoreUnexported(netip.Addr{}, netip.Prefix{}))
+	assert.DeepEqual(t, expected, networks, cmpopts.EquateComparable(netip.Addr{}, netip.Prefix{}))
 	assert.DeepEqual(t, []string{"special"}, externals)
 }
 
diff --git a/cli/compose/convert/service.go b/cli/compose/convert/service.go
@@ -653,7 +653,10 @@ func convertDNSConfig(dns []string, dnsSearch []string) *swarm.DNSConfig {
 }
 
 func toNetipAddrSlice(ips []string) []netip.Addr {
-	var netIPs []netip.Addr //nolint:prealloc // keeping old behavior
+	if len(ips) == 0 {
+		return nil
+	}
+	netIPs := make([]netip.Addr, 0, len(ips))
 	for _, ip := range ips {
 		addr, err := netip.ParseAddr(ip)
 		if err != nil {
diff --git a/cli/compose/convert/service_test.go b/cli/compose/convert/service_test.go
@@ -311,23 +311,23 @@ func TestConvertDNSConfigAll(t *testing.T) {
 	assert.Check(t, is.DeepEqual(&swarm.DNSConfig{
 		Nameservers: toNetipAddrSlice(nameservers),
 		Search:      search,
-	}, dnsConfig, cmpopts.IgnoreUnexported(netip.Addr{})))
+	}, dnsConfig, cmpopts.EquateComparable(netip.Addr{})))
 }
 
 func TestConvertDNSConfigNameservers(t *testing.T) {
 	dnsConfig := convertDNSConfig(nameservers, nil)
 	assert.Check(t, is.DeepEqual(&swarm.DNSConfig{
 		Nameservers: toNetipAddrSlice(nameservers),
 		Search:      nil,
-	}, dnsConfig, cmpopts.IgnoreUnexported(netip.Addr{})))
+	}, dnsConfig, cmpopts.EquateComparable(netip.Addr{})))
 }
 
 func TestConvertDNSConfigSearch(t *testing.T) {
 	dnsConfig := convertDNSConfig(nil, search)
 	assert.Check(t, is.DeepEqual(&swarm.DNSConfig{
 		Nameservers: nil,
 		Search:      search,
-	}, dnsConfig, cmpopts.IgnoreUnexported(netip.Addr{})))
+	}, dnsConfig, cmpopts.EquateComparable(netip.Addr{})))
 }
 
 func TestConvertCredentialSpec(t *testing.T) {
diff --git a/opts/network_test.go b/opts/network_test.go
@@ -26,7 +26,7 @@ func TestNetworkOptLegacySyntax(t *testing.T) {
 	for _, tc := range testCases {
 		var network NetworkOpt
 		assert.NilError(t, network.Set(tc.value))
-		assert.Check(t, is.DeepEqual(tc.expected, network.Value(), cmpopts.EquateComparable(), cmpopts.IgnoreUnexported(netip.Addr{})))
+		assert.Check(t, is.DeepEqual(tc.expected, network.Value(), cmpopts.EquateComparable(netip.Addr{})))
 	}
 }
 
@@ -129,7 +129,7 @@ func TestNetworkOptAdvancedSyntax(t *testing.T) {
 		t.Run(tc.value, func(t *testing.T) {
 			var network NetworkOpt
 			assert.NilError(t, network.Set(tc.value))
-			assert.Check(t, is.DeepEqual(tc.expected, network.Value(), cmpopts.EquateComparable(), cmpopts.IgnoreUnexported(netip.Addr{})))
+			assert.Check(t, is.DeepEqual(tc.expected, network.Value(), cmpopts.EquateComparable(netip.Addr{})))
 		})
 	}
 }
diff --git a/opts/swarmopts/port.go b/opts/swarmopts/port.go
@@ -155,14 +155,11 @@ func ConvertPortToPortConfig(
 	portProto network.Port,
 	portBindings map[nat.Port][]nat.PortBinding,
 ) ([]swarm.PortConfig, error) {
-	portProto.Num()
-	proto, portInt := portProto.Proto(), portProto.Num()
-
 	ports := make([]swarm.PortConfig, 0, len(portBindings))
 	for _, binding := range portBindings[nat.Port(portProto.String())] {
 		if p := net.ParseIP(binding.HostIP); p != nil && !p.IsUnspecified() {
 			// TODO(thaJeztah): use context-logger, so that this output can be suppressed (in tests).
-			logrus.Warnf("ignoring IP-address (%s:%s) service will listen on '0.0.0.0'", net.JoinHostPort(binding.HostIP, binding.HostPort), portProto)
+			logrus.Warnf("ignoring IP-address (%s:%s) service will listen on '0.0.0.0'", net.JoinHostPort(binding.HostIP, binding.HostPort), portProto.Proto())
 		}
 
 		startHostPort, endHostPort, err := nat.ParsePortRange(binding.HostPort)
@@ -174,8 +171,8 @@ func ConvertPortToPortConfig(
 		for i := startHostPort; i <= endHostPort; i++ {
 			ports = append(ports, swarm.PortConfig{
 				// TODO Name: ?
-				Protocol:      proto,
-				TargetPort:    uint32(portInt),
+				Protocol:      portProto.Proto(),
+				TargetPort:    uint32(portProto.Num()),
 				PublishedPort: uint32(i),
 				PublishMode:   swarm.PortConfigPublishModeIngress,
 			})

Original file line number	Diff line number	Diff line change
`@@ -1158,13 +1158,16 @@ func validateAttach(val string) (string, error) {`
`1158`	`1158`	`}`
`1159`	`1159`
`1160`	`1160`	`func toNetipAddrSlice(ips []string) []netip.Addr {`
`1161`		`- netips := make([]netip.Addr, 0, len(ips))`
	`1161`	`+ if len(ips) == 0 {`
	`1162`	`+ return nil`
	`1163`	`+ }`
	`1164`	`+ netIPs := make([]netip.Addr, 0, len(ips))`
`1162`	`1165`	`for _, ip := range ips {`
`1163`	`1166`	`addr, err := netip.ParseAddr(ip)`
`1164`	`1167`	`if err != nil {`
`1165`	`1168`	`continue`
`1166`	`1169`	`}`
`1167`		`- netips = append(netips, addr)`
	`1170`	`+ netIPs = append(netIPs, addr)`
`1168`	`1171`	`}`
`1169`		`- return netips`
	`1172`	`+ return netIPs`
`1170`	`1173`	`}`
Original file line number	Diff line number	Diff line change
`@@ -755,7 +755,7 @@ func TestParseNetworkConfig(t *testing.T) {`
`755`	`755`	`assert.NilError(t, err)`
`756`	`756`	`assert.DeepEqual(t, config.MacAddress, tc.expectedCfg.MacAddress) //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44.`
`757`	`757`	`assert.DeepEqual(t, hConfig.NetworkMode, tc.expectedHostCfg.NetworkMode)`
`758`		`- assert.DeepEqual(t, nwConfig.EndpointsConfig, tc.expected, cmpopts.IgnoreUnexported(netip.Addr{}))`
	`758`	`+ assert.DeepEqual(t, nwConfig.EndpointsConfig, tc.expected, cmpopts.EquateComparable(netip.Addr{}))`
`759`	`759`	`})`
`760`	`760`	`}`
`761`	`761`	`}`
Original file line number	Diff line number	Diff line change
`@@ -62,7 +62,7 @@ func TestNetworkConnectWithFlags(t *testing.T) {`
`62`	`62`	`}`
`63`	`63`	`cli := test.NewFakeCli(&fakeClient{`
`64`	`64`	`networkConnectFunc: func(ctx context.Context, networkID, container string, config *network.EndpointSettings) error {`
`65`		`- assert.Check(t, is.DeepEqual(expectedConfig, config, cmpopts.IgnoreUnexported(netip.Addr{})))`
	`65`	`+ assert.Check(t, is.DeepEqual(expectedConfig, config, cmpopts.EquateComparable(netip.Addr{})))`
`66`	`66`	`return nil`
`67`	`67`	`},`
`68`	`68`	`})`
Original file line number	Diff line number	Diff line change
`@@ -1076,13 +1076,16 @@ const (`
`1076`	`1076`	`)`
`1077`	`1077`
`1078`	`1078`	`func toNetipAddrSlice(ips []string) []netip.Addr {`
`1079`		`- netips := make([]netip.Addr, 0, len(ips))`
	`1079`	`+ if len(ips) == 0 {`
	`1080`	`+ return nil`
	`1081`	`+ }`
	`1082`	`+ netIPs := make([]netip.Addr, 0, len(ips))`
`1080`	`1083`	`for _, ip := range ips {`
`1081`	`1084`	`addr, err := netip.ParseAddr(ip)`
`1082`	`1085`	`if err != nil {`
`1083`	`1086`	`continue`
`1084`	`1087`	`}`
`1085`		`- netips = append(netips, addr)`
	`1088`	`+ netIPs = append(netIPs, addr)`
`1086`	`1089`	`}`
`1087`		`- return netips`
	`1090`	`+ return netIPs`
`1088`	`1091`	`}`