Skip to content

Commit

Permalink
Merge pull request #5005 from danwinship/ipv6-validation
Browse files Browse the repository at this point in the history 
Bug 1972776: improve dual-stack install-config validation
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot committed Jul 28, 2021
2 parents f643583 + cc1d3e9 commit 17398dd
Show file tree
Hide file tree
Showing 3 changed files with 101 additions and 39 deletions.
62 changes: 36 additions & 26 deletions pkg/types/validation/installconfig.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
"golang.org/x/crypto/ssh"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/validation/field"

Expand Down Expand Up @@ -136,40 +137,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
}
Expand All @@ -180,20 +194,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
}

Expand Down Expand Up @@ -225,12 +230,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"))
}
}

Expand Down Expand Up @@ -282,7 +292,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 {
Expand Down
57 changes: 44 additions & 13 deletions pkg/types/validation/installconfig_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ func validIPv6NetworkingConfig() *types.Networking {
},
},
ServiceNetwork: []ipnet.IPNet{
*ipnet.MustParseCIDR("ffd1::/48"),
*ipnet.MustParseCIDR("ffd1::/112"),
},
ClusterNetwork: []types.ClusterNetworkEntry{
{
Expand All @@ -178,25 +178,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,
},
},
}
}
Expand Down Expand Up @@ -1135,20 +1135,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",
Expand All @@ -1161,6 +1181,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",
Expand Down
21 changes: 21 additions & 0 deletions pkg/validate/validate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 17398dd

Please sign in to comment.