Bug 1973421: [4.8] improve dual-stack install-config validation

pkg/types/validation/installconfig.go

@@ -12,6 +12,7 @@ import (
 	dockerref "github.com/containers/image/docker/reference"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
+	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 
@@ -126,40 +127,53 @@ func ValidateInstallConfig(c *types.InstallConfig) field.ErrorList {
 	return allErrs
 }
 
-// ipAddressTypeByField is a map of field path to whether they request IPv4 or IPv6.
-type ipAddressTypeByField map[string]struct{ IPv4, IPv6 bool }
+// ipAddressType indicates the address types provided for a given field
+type ipAddressType struct {
+	IPv4    bool
+	IPv6    bool
+	Primary corev1.IPFamily
+}
+
+// ipAddressTypeByField is a map of field path to ipAddressType
+type ipAddressTypeByField map[string]ipAddressType
 
-// ipByField is a map of field path to the net.IPs in sorted order.
-type ipByField map[string][]net.IP
+// ipNetByField is a map of field path to the IPNets
+type ipNetByField map[string][]ipnet.IPNet
 
 // inferIPVersionFromInstallConfig infers the user's desired ip version from the networking config.
 // Presence field names match the field path of the struct within the Networking type. This function
 // assumes a valid install config.
-func inferIPVersionFromInstallConfig(n *types.Networking) (hasIPv4, hasIPv6 bool, presence ipAddressTypeByField, addresses ipByField) {
+func inferIPVersionFromInstallConfig(n *types.Networking) (hasIPv4, hasIPv6 bool, presence ipAddressTypeByField, addresses ipNetByField) {
 	if n == nil {
 		return
 	}
-	addresses = make(ipByField)
+	addresses = make(ipNetByField)
 	for _, network := range n.MachineNetwork {
-		addresses["machineNetwork"] = append(addresses["machineNetwork"], network.CIDR.IP)
+		addresses["machineNetwork"] = append(addresses["machineNetwork"], network.CIDR)
 	}
 	for _, network := range n.ServiceNetwork {
-		addresses["serviceNetwork"] = append(addresses["serviceNetwork"], network.IP)
+		addresses["serviceNetwork"] = append(addresses["serviceNetwork"], network)
 	}
 	for _, network := range n.ClusterNetwork {
-		addresses["clusterNetwork"] = append(addresses["clusterNetwork"], network.CIDR.IP)
+		addresses["clusterNetwork"] = append(addresses["clusterNetwork"], network.CIDR)
 	}
 	presence = make(ipAddressTypeByField)
-	for k, ips := range addresses {
-		for _, ip := range ips {
+	for k, ipnets := range addresses {
+		for i, ipnet := range ipnets {
 			has := presence[k]
-			if ip.To4() != nil {
+			if ipnet.IP.To4() != nil {
 				has.IPv4 = true
+				if i == 0 {
+					has.Primary = corev1.IPv4Protocol
+				}
 				if k == "serviceNetwork" {
 					hasIPv4 = true
 				}
 			} else {
 				has.IPv6 = true
+				if i == 0 {
+					has.Primary = corev1.IPv6Protocol
+				}
 				if k == "serviceNetwork" {
 					hasIPv6 = true
 				}
@@ -170,20 +184,11 @@ func inferIPVersionFromInstallConfig(n *types.Networking) (hasIPv4, hasIPv6 bool
 	return
 }
 
-func ipSliceToStrings(ips []net.IP) []string {
-	var s []string
-	for _, ip := range ips {
-		s = append(s, ip.String())
-	}
-	return s
-}
-
 func ipnetworksToStrings(networks []ipnet.IPNet) []string {
 	var diag []string
 	for _, sn := range networks {
 		diag = append(diag, sn.String())
 	}
-	sort.Strings(diag)
 	return diag
 }
 
@@ -215,12 +220,17 @@ func validateNetworkingIPVersion(n *types.Networking, p *types.Platform) field.E
 		}
 		for k, v := range presence {
 			switch {
-			case k == "machineNetwork" && p.AWS != nil:
-				// AWS can default an ipv6 subnet
 			case v.IPv4 && !v.IPv6:
-				allErrs = append(allErrs, field.Invalid(field.NewPath("networking", k), strings.Join(ipSliceToStrings(addresses[k]), ", "), "dual-stack IPv4/IPv6 requires an IPv6 address in this list"))
+				allErrs = append(allErrs, field.Invalid(field.NewPath("networking", k), strings.Join(ipnetworksToStrings(addresses[k]), ", "), "dual-stack IPv4/IPv6 requires an IPv6 network in this list"))
 			case !v.IPv4 && v.IPv6:
-				allErrs = append(allErrs, field.Invalid(field.NewPath("networking", k), strings.Join(ipSliceToStrings(addresses[k]), ", "), "dual-stack IPv4/IPv6 requires an IPv4 address in this list"))
+				allErrs = append(allErrs, field.Invalid(field.NewPath("networking", k), strings.Join(ipnetworksToStrings(addresses[k]), ", "), "dual-stack IPv4/IPv6 requires an IPv4 network in this list"))
+			}
+
+			// FIXME: we should allow either all-networks-IPv4Primary or
+			// all-networks-IPv6Primary, but the latter currently causes
+			// confusing install failures, so block it.
+			if v.IPv4 && v.IPv6 && v.Primary != corev1.IPv4Protocol {
+				allErrs = append(allErrs, field.Invalid(field.NewPath("networking", k), strings.Join(ipnetworksToStrings(addresses[k]), ", "), "IPv4 addresses must be listed before IPv6 addresses"))
 			}
 		}
 
@@ -270,7 +280,7 @@ func validateNetworking(n *types.Networking, fldPath *field.Path) field.ErrorLis
 	}
 
 	for i, sn := range n.ServiceNetwork {
-		if err := validate.SubnetCIDR(&sn.IPNet); err != nil {
+		if err := validate.ServiceSubnetCIDR(&sn.IPNet); err != nil {
 			allErrs = append(allErrs, field.Invalid(fldPath.Child("serviceNetwork").Index(i), sn.String(), err.Error()))
 		}
 		for _, network := range n.MachineNetwork {

pkg/types/validation/installconfig_test.go

@@ -155,7 +155,7 @@ func validIPv6NetworkingConfig() *types.Networking {
 			},
 		},
 		ServiceNetwork: []ipnet.IPNet{
-			*ipnet.MustParseCIDR("ffd1::/48"),
+			*ipnet.MustParseCIDR("ffd1::/112"),
 		},
 		ClusterNetwork: []types.ClusterNetworkEntry{
 			{
@@ -171,25 +171,25 @@ func validDualStackNetworkingConfig() *types.Networking {
 		NetworkType: "OVNKubernetes",
 		MachineNetwork: []types.MachineNetworkEntry{
 			{
-				CIDR: *ipnet.MustParseCIDR("ffd0::/48"),
+				CIDR: *ipnet.MustParseCIDR("10.0.0.0/16"),
 			},
 			{
-				CIDR: *ipnet.MustParseCIDR("10.0.0.0/16"),
+				CIDR: *ipnet.MustParseCIDR("ffd0::/48"),
 			},
 		},
 		ServiceNetwork: []ipnet.IPNet{
-			*ipnet.MustParseCIDR("ffd1::/48"),
 			*ipnet.MustParseCIDR("172.30.0.0/16"),
+			*ipnet.MustParseCIDR("ffd1::/112"),
 		},
 		ClusterNetwork: []types.ClusterNetworkEntry{
-			{
-				CIDR:       *ipnet.MustParseCIDR("ffd2::/48"),
-				HostPrefix: 64,
-			},
 			{
 				CIDR:       *ipnet.MustParseCIDR("192.168.1.0/24"),
 				HostPrefix: 28,
 			},
+			{
+				CIDR:       *ipnet.MustParseCIDR("ffd2::/48"),
+				HostPrefix: 64,
+			},
 		},
 	}
 }
@@ -1107,20 +1107,40 @@ func TestValidateInstallConfig(t *testing.T) {
 				c := validInstallConfig()
 				c.Platform = types.Platform{None: &none.Platform{}}
 				c.Networking = validDualStackNetworkingConfig()
-				c.Networking.MachineNetwork = c.Networking.MachineNetwork[1:]
+				c.Networking.MachineNetwork = c.Networking.MachineNetwork[:1]
 				return c
 			}(),
-			expectedError: `Invalid value: "10.0.0.0": dual-stack IPv4/IPv6 requires an IPv6 address in this list`,
+			expectedError: `Invalid value: "10.0.0.0/16": dual-stack IPv4/IPv6 requires an IPv6 network in this list`,
 		},
 		{
-			name: "valid dual-stack configuration, machine has no IPv6 but is on AWS",
+			name: "invalid dual-stack configuration, IPv6-primary",
 			installConfig: func() *types.InstallConfig {
 				c := validInstallConfig()
+				c.Platform = types.Platform{None: &none.Platform{}}
 				c.Networking = validDualStackNetworkingConfig()
-				c.Networking.MachineNetwork = c.Networking.MachineNetwork[1:]
+				c.Networking.ServiceNetwork = []ipnet.IPNet{
+					c.Networking.ServiceNetwork[1],
+					c.Networking.ServiceNetwork[0],
+				}
+				return c
+			}(),
+			expectedError: `Invalid value: "ffd1::/112, 172.30.0.0/16": IPv4 addresses must be listed before IPv6 addresses`,
+		},
+		{
+			name: "valid dual-stack configuration with mixed-order clusterNetworks",
+			installConfig: func() *types.InstallConfig {
+				c := validInstallConfig()
+				c.Platform = types.Platform{None: &none.Platform{}}
+				c.Networking = validDualStackNetworkingConfig()
+				c.Networking.ClusterNetwork = append(c.Networking.ClusterNetwork,
+					types.ClusterNetworkEntry{
+						CIDR:       *ipnet.MustParseCIDR("192.168.2.0/24"),
+						HostPrefix: 28,
+					},
+				)
+				// ClusterNetwork is now "IPv4, IPv6, IPv4", which is allowed
 				return c
 			}(),
-			expectedError: `Invalid value: "DualStack": dual-stack IPv4/IPv6 is not supported for this platform, specify only one type of address`,
 		},
 		{
 			name: "invalid IPv6 hostprefix",
@@ -1133,6 +1153,17 @@ func TestValidateInstallConfig(t *testing.T) {
 			}(),
 			expectedError: `Invalid value: 72: cluster network host subnetwork prefix must be 64 for IPv6 networks`,
 		},
+		{
+			name: "invalid IPv6 service network size",
+			installConfig: func() *types.InstallConfig {
+				c := validInstallConfig()
+				c.Platform = types.Platform{None: &none.Platform{}}
+				c.Networking = validIPv6NetworkingConfig()
+				c.Networking.ServiceNetwork[0] = *ipnet.MustParseCIDR("ffd1::/48")
+				return c
+			}(),
+			expectedError: `Invalid value: "ffd1::/48": subnet size for IPv6 service network should be /112`,
+		},
 
 		{
 			name: "valid ovirt platform",

pkg/validate/validate.go

@@ -159,6 +159,27 @@ func SubnetCIDR(cidr *net.IPNet) error {
 	return nil
 }
 
+// ServiceSubnetCIDR checks if the given IP net is a valid CIDR for the Kubernetes service network
+func ServiceSubnetCIDR(cidr *net.IPNet) error {
+	if cidr.IP.IsUnspecified() {
+		return errors.New("address must be specified")
+	}
+	nip := cidr.IP.Mask(cidr.Mask)
+	if nip.String() != cidr.IP.String() {
+		return fmt.Errorf("invalid network address. got %s, expecting %s", cidr.String(), (&net.IPNet{IP: nip, Mask: cidr.Mask}).String())
+	}
+	maskLen, addrLen := cidr.Mask.Size()
+	if addrLen == 32 && maskLen < 12 {
+		return fmt.Errorf("subnet size for IPv4 service network must be /12 or greater (/16 is recommended)")
+	} else if addrLen == 128 && maskLen < 108 {
+		// Kubernetes allows any length greater than 108 (and so do we, for
+		// backward compat), but for various reasons there is no point in
+		// using any value other than 112.
+		return fmt.Errorf("subnet size for IPv6 service network should be /112")
+	}
+	return nil
+}
+
 // DoCIDRsOverlap returns true if one of the CIDRs is a subset of the other.
 func DoCIDRsOverlap(acidr, bcidr *net.IPNet) bool {
 	return acidr.Contains(bcidr.IP) || bcidr.Contains(acidr.IP)