Move validation of ClusterSubnetSpec into pkg/apis/kops/validation

pkg/apis/kops/validation/aws.go

@@ -17,6 +17,8 @@ limitations under the License.
 package validation
 
 import (
+	"fmt"
+	"net"
 	"strconv"
 	"strings"
 
@@ -35,6 +37,7 @@ func awsValidateCluster(c *kops.Cluster) field.ErrorList {
 		if c.Spec.API.LoadBalancer != nil {
 			allErrs = append(allErrs, awsValidateAdditionalSecurityGroups(field.NewPath("spec", "api", "loadBalancer", "additionalSecurityGroups"), c.Spec.API.LoadBalancer.AdditionalSecurityGroups)...)
 			allErrs = append(allErrs, awsValidateSSLPolicy(field.NewPath("spec", "api", "loadBalancer", "sslPolicy"), c.Spec.API.LoadBalancer)...)
+			allErrs = append(allErrs, awsValidateLoadBalancerSubnets(field.NewPath("spec", "api", "loadBalancer", "subnets"), c.Spec)...)
 		}
 	}
 
@@ -196,3 +199,49 @@ func awsValidateSSLPolicy(fieldPath *field.Path, spec *kops.LoadBalancerAccessSp
 
 	return allErrs
 }
+
+func awsValidateLoadBalancerSubnets(fieldPath *field.Path, spec kops.ClusterSpec) field.ErrorList {
+	allErrs := field.ErrorList{}
+
+	lbSpec := spec.API.LoadBalancer
+
+	for i, subnet := range lbSpec.Subnets {
+		var clusterSubnet *kops.ClusterSubnetSpec
+		if subnet.Name == "" {
+			allErrs = append(allErrs, field.Invalid(fieldPath.Index(i).Child("name"), subnet, "subnet name can't be empty"))
+		} else {
+			for _, cs := range spec.Subnets {
+				if subnet.Name == cs.Name {
+					clusterSubnet = &cs
+					break
+				}
+			}
+			if clusterSubnet == nil {
+				allErrs = append(allErrs, field.Invalid(fieldPath.Index(i).Child("name"), subnet, fmt.Sprintf("subnet %q not found in cluster subnets", subnet.Name)))
+			}
+		}
+
+		if subnet.PrivateIPv4Address != nil {
+			if *subnet.PrivateIPv4Address == "" {
+				allErrs = append(allErrs, field.Invalid(fieldPath.Index(i).Child("privateIPv4Address"), subnet, "privateIPv4Address can't be empty"))
+			}
+			ip := net.ParseIP(*subnet.PrivateIPv4Address)
+			if ip == nil {
+				allErrs = append(allErrs, field.Invalid(fieldPath.Index(i).Child("privateIPv4Address"), subnet, "privateIPv4Address is not a valid IPv4 address"))
+			} else if clusterSubnet != nil {
+				_, ipNet, err := net.ParseCIDR(clusterSubnet.CIDR)
+				if err == nil { // we assume that the cidr is actually valid
+					if !ipNet.Contains(ip) {
+						allErrs = append(allErrs, field.Invalid(fieldPath.Index(i).Child("privateIPv4Address"), subnet, "privateIPv4Address is not part of the subnet CIDR"))
+					}
+				}
+
+			}
+			if lbSpec.Class != kops.LoadBalancerClassNetwork || lbSpec.Type != kops.LoadBalancerTypeInternal {
+				allErrs = append(allErrs, field.Invalid(fieldPath.Index(i).Child("privateIPv4Address"), subnet, "privateIPv4Address only allowed for internal NLBs"))
+			}
+		}
+	}
+
+	return allErrs
+}

pkg/apis/kops/validation/aws_test.go

@@ -239,3 +239,143 @@ func TestInstanceMetadataOptions(t *testing.T) {
 		testErrors(t, test.ig.ObjectMeta.Name, errs, test.expected)
 	}
 }
+
+func TestLoadBalancerSubnets(t *testing.T) {
+	cidr := "10.0.0.0/24"
+	tests := []struct {
+		lbType         *string
+		class          *string
+		clusterSubnets []string
+		lbSubnets      []kops.LoadBalancerSubnetSpec
+		expected       []string
+	}{
+		{ // valid (no privateIPv4Address)
+			clusterSubnets: []string{"a", "b", "c"},
+			lbSubnets: []kops.LoadBalancerSubnetSpec{
+				{
+					Name:               "a",
+					PrivateIPv4Address: nil,
+				},
+				{
+					Name:               "b",
+					PrivateIPv4Address: nil,
+				},
+			},
+		},
+		{ // valid (with privateIPv4Address)
+			clusterSubnets: []string{"a", "b", "c"},
+			lbSubnets: []kops.LoadBalancerSubnetSpec{
+				{
+					Name:               "a",
+					PrivateIPv4Address: fi.String("10.0.0.10"),
+				},
+				{
+					Name:               "b",
+					PrivateIPv4Address: nil,
+				},
+			},
+		},
+		{ // empty subnet name
+			clusterSubnets: []string{"a", "b", "c"},
+			lbSubnets: []kops.LoadBalancerSubnetSpec{
+				{
+					Name:               "",
+					PrivateIPv4Address: nil,
+				},
+			},
+			expected: []string{"Invalid value::spec.api.loadBalancer.subnets[0].name"},
+		},
+		{ // subnet not found
+			clusterSubnets: []string{"a", "b", "c"},
+			lbSubnets: []kops.LoadBalancerSubnetSpec{
+				{
+					Name:               "d",
+					PrivateIPv4Address: nil,
+				},
+			},
+			expected: []string{"Invalid value::spec.api.loadBalancer.subnets[0].name"},
+		},
+		{ // empty privateIPv4Address
+			clusterSubnets: []string{"a", "b", "c"},
+			lbSubnets: []kops.LoadBalancerSubnetSpec{
+				{
+					Name:               "a",
+					PrivateIPv4Address: fi.String(""),
+				},
+			},
+			expected: []string{"Invalid value::spec.api.loadBalancer.subnets[0].privateIPv4Address"},
+		},
+		{ // invalid privateIPv4Address
+			clusterSubnets: []string{"a", "b", "c"},
+			lbSubnets: []kops.LoadBalancerSubnetSpec{
+				{
+					Name:               "a",
+					PrivateIPv4Address: fi.String("invalidip"),
+				},
+			},
+			expected: []string{"Invalid value::spec.api.loadBalancer.subnets[0].privateIPv4Address"},
+		},
+		{ // privateIPv4Address not matching subnet cidr
+			clusterSubnets: []string{"a", "b", "c"},
+			lbSubnets: []kops.LoadBalancerSubnetSpec{
+				{
+					Name:               "a",
+					PrivateIPv4Address: fi.String("11.0.0.10"),
+				},
+			},
+			expected: []string{"Invalid value::spec.api.loadBalancer.subnets[0].privateIPv4Address"},
+		},
+		{ // invalid class
+			class:          fi.String(string(kops.LoadBalancerClassClassic)),
+			clusterSubnets: []string{"a", "b", "c"},
+			lbSubnets: []kops.LoadBalancerSubnetSpec{
+				{
+					Name:               "a",
+					PrivateIPv4Address: fi.String("10.0.0.10"),
+				},
+			},
+			expected: []string{"Invalid value::spec.api.loadBalancer.subnets[0].privateIPv4Address"},
+		},
+		{ // invalid type
+			lbType:         fi.String(string(kops.LoadBalancerTypePublic)),
+			clusterSubnets: []string{"a", "b", "c"},
+			lbSubnets: []kops.LoadBalancerSubnetSpec{
+				{
+					Name:               "a",
+					PrivateIPv4Address: fi.String("10.0.0.10"),
+				},
+			},
+			expected: []string{"Invalid value::spec.api.loadBalancer.subnets[0].privateIPv4Address"},
+		},
+	}
+
+	for _, test := range tests {
+		cluster := kops.Cluster{
+			Spec: kops.ClusterSpec{
+				API: &kops.AccessSpec{
+					LoadBalancer: &kops.LoadBalancerAccessSpec{
+						Class: kops.LoadBalancerClassNetwork,
+						Type:  kops.LoadBalancerTypeInternal,
+					},
+				},
+			},
+		}
+		if test.class != nil {
+			cluster.Spec.API.LoadBalancer.Class = kops.LoadBalancerClass(*test.class)
+		}
+		if test.lbType != nil {
+			cluster.Spec.API.LoadBalancer.Type = kops.LoadBalancerType(*test.lbType)
+		}
+		for _, s := range test.clusterSubnets {
+			cluster.Spec.Subnets = append(cluster.Spec.Subnets, kops.ClusterSubnetSpec{
+				Name: s,
+				CIDR: cidr,
+			})
+		}
+		for _, s := range test.lbSubnets {
+			cluster.Spec.API.LoadBalancer.Subnets = append(cluster.Spec.API.LoadBalancer.Subnets, s)
+		}
+		errs := awsValidateCluster(&cluster)
+		testErrors(t, test, errs, test.expected)
+	}
+}

pkg/model/awsmodel/api_loadbalancer.go

@@ -68,31 +68,19 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 	if len(lbSpec.Subnets) != 0 {
 		// Subnets have been explicitly set
 		for _, subnet := range lbSpec.Subnets {
-			found := false
 			for _, clusterSubnet := range b.Cluster.Spec.Subnets {
 				if subnet.Name == clusterSubnet.Name {
-					found = true
 					elbSubnet := b.LinkToSubnet(&clusterSubnet)
 					elbSubnets = append(elbSubnets, elbSubnet)
 					nlbSubnetMappings = append(nlbSubnetMappings, &awstasks.SubnetMapping{
 						Subnet: elbSubnet,
 					})
 					if subnet.PrivateIPv4Address != nil {
-						if *subnet.PrivateIPv4Address == "" {
-							return fmt.Errorf("privateIPv4Address can't be empty")
-						}
-						if lbSpec.Class != kops.LoadBalancerClassNetwork || lbSpec.Type != kops.LoadBalancerTypeInternal {
-							return fmt.Errorf("privateIPv4Address only allowed for internal NLBs")
-						}
-
 						nlbSubnetMappings[len(nlbSubnetMappings)-1].PrivateIPv4Address = subnet.PrivateIPv4Address
 					}
 					break
 				}
 			}
-			if !found {
-				return fmt.Errorf("subnet %q not found in cluster subnets", subnet.Name)
-			}
 		}
 	} else {
 		// Compute the subnets - only one per zone, and then break ties based on chooseBestSubnetForELB