Allow default subnets to be optional (#204)

Author: Harvey Lowndes

pkg/oci/config_validate.go

@@ -63,19 +63,18 @@ func validateAuthConfig(c *AuthConfig, fldPath *field.Path) field.ErrorList {
 
 // validateLoadBalancerConfig provides basic validation of LoadBalancerConfig
 // instances.
-func validateLoadBalancerConfig(c *LoadBalancerConfig, fldPath *field.Path) field.ErrorList {
+func validateLoadBalancerConfig(c *Config, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
-	if c == nil {
+	lbConfig := c.LoadBalancer
+	if &lbConfig == nil {
 		return append(allErrs, field.Required(fldPath, ""))
 	}
-	if c.Subnet1 == "" {
-		allErrs = append(allErrs, field.Required(fldPath.Child("subnet1"), ""))
-	}
-	if c.Subnet2 == "" {
-		allErrs = append(allErrs, field.Required(fldPath.Child("subnet2"), ""))
+	if lbConfig.Subnet1 == "" && c.VCNID == "" {
+		allErrs = append(allErrs, field.Required(field.NewPath("vcn"), "VCNID configuration must be provided if configuration for subnet1 is not provided"))
 	}
-	if !IsValidSecurityListManagementMode(c.SecurityListManagementMode) {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("securityListManagementMode"), c.SecurityListManagementMode, "invalid security list management mode"))
+	if !IsValidSecurityListManagementMode(lbConfig.SecurityListManagementMode) {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("securityListManagementMode"),
+			lbConfig.SecurityListManagementMode, "invalid security list management mode"))
 	}
 	return allErrs
 }
@@ -84,6 +83,6 @@ func validateLoadBalancerConfig(c *LoadBalancerConfig, fldPath *field.Path) fiel
 func ValidateConfig(c *Config) field.ErrorList {
 	allErrs := field.ErrorList{}
 	allErrs = append(allErrs, validateAuthConfig(&c.Auth, field.NewPath("auth"))...)
-	allErrs = append(allErrs, validateLoadBalancerConfig(&c.LoadBalancer, field.NewPath("loadBalancer"))...)
+	allErrs = append(allErrs, validateLoadBalancerConfig(c, field.NewPath("loadBalancer"))...)
 	return allErrs
 }

pkg/oci/config_validate_test.go

@@ -205,7 +205,7 @@ func TestValidateConfig(t *testing.T) {
 				&field.Error{Type: field.ErrorTypeRequired, Field: "auth.fingerprint", BadValue: ""},
 			},
 		}, {
-			name: "missing_subnet1",
+			name: "missing_vcnid",
 			in: &Config{
 				Auth: AuthConfig{
 					Region:        "us-phoenix-1",
@@ -215,30 +215,10 @@ func TestValidateConfig(t *testing.T) {
 					PrivateKey:    "-----BEGIN RSA PRIVATE KEY----- (etc)",
 					Fingerprint:   "8c:bf:17:7b:5f:e0:7d:13:75:11:d6:39:0d:e2:84:74",
 				},
-				LoadBalancer: LoadBalancerConfig{
-					Subnet2: "ocid1.subnet.oc1.phx.aaaaaaaahuxrgvs65iwdz7ekwgg3l5gyah7ww5klkwjcso74u3e4i64hvtvq",
-				},
-			},
-			errs: field.ErrorList{
-				&field.Error{Type: field.ErrorTypeRequired, Field: "loadBalancer.subnet1", BadValue: ""},
-			},
-		}, {
-			name: "missing_subnet2",
-			in: &Config{
-				Auth: AuthConfig{
-					Region:        "us-phoenix-1",
-					TenancyID:     "ocid1.tenancy.oc1..aaaaaaaatyn7scrtwtqedvgrxgr2xunzeo6uanvyhzxqblctwkrpisvke4kq",
-					CompartmentID: "ocid1.compartment.oc1..aaaaaaaa3um2atybwhder4qttfhgon4j3hcxgmsvnyvx4flfjyewkkwfzwnq",
-					UserID:        "ocid1.user.oc1..aaaaaaaai77mql2xerv7cn6wu3nhxang3y4jk56vo5bn5l5lysl34avnui3q",
-					PrivateKey:    "-----BEGIN RSA PRIVATE KEY----- (etc)",
-					Fingerprint:   "8c:bf:17:7b:5f:e0:7d:13:75:11:d6:39:0d:e2:84:74",
-				},
-				LoadBalancer: LoadBalancerConfig{
-					Subnet1: "ocid1.tenancy.oc1..aaaaaaaatyn7scrtwtqedvgrxgr2xunzeo6uanvyhzxqblctwkrpisvke4kq",
-				},
+				LoadBalancer: LoadBalancerConfig{},
 			},
 			errs: field.ErrorList{
-				&field.Error{Type: field.ErrorTypeRequired, Field: "loadBalancer.subnet2", BadValue: ""},
+				&field.Error{Type: field.ErrorTypeRequired, Field: "vcn", BadValue: "", Detail: "VCNID configuration must be provided if configuration for subnet1 is not provided"},
 			},
 		}, {
 			name: "invalid_security_list_management_mode",

pkg/oci/load_balancer_spec.go

@@ -115,10 +115,18 @@ func NewLBSpec(svc *v1.Service, nodes []*v1.Node, defaultSubnets []string, sslCf
 	if s, ok := svc.Annotations[ServiceAnnotationLoadBalancerSubnet2]; ok {
 		subnets[1] = s
 	}
+
 	if internal {
 		// Only public load balancers need two subnets.  Internal load
 		// balancers will always use the first subnet.
+		if subnets[0] == "" {
+			return nil, errors.Errorf("a configuration for subnet1 must be specified for an internal load balancer")
+		}
 		subnets = subnets[:1]
+	} else {
+		if subnets[0] == "" || subnets[1] == "" {
+			return nil, errors.Errorf("a configuration for both subnets must be specified")
+		}
 	}
 
 	listeners, err := getListeners(svc, sslCfg)

pkg/oci/load_balancer_spec_test.go

@@ -366,6 +366,8 @@ func TestNewLBSpecFailure(t *testing.T) {
 			expectedErrMsg: "invalid service: OCI only supports SessionAffinity \"None\" currently",
 		},
 		"invalid idle connection timeout": {
+			defaultSubnetOne: "one",
+			defaultSubnetTwo: "two",
 			service: &v1.Service{
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "kube-system",
@@ -384,6 +386,39 @@ func TestNewLBSpecFailure(t *testing.T) {
 			},
 			expectedErrMsg: "error parsing service annotation: service.beta.kubernetes.io/oci-load-balancer-connection-idle-timeout=whoops",
 		},
+		"missing subnet defaults and annotations": {
+			service: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace:   "kube-system",
+					Name:        "testservice",
+					UID:         "test-uid",
+					Annotations: map[string]string{},
+				},
+				Spec: v1.ServiceSpec{
+					SessionAffinity: v1.ServiceAffinityNone,
+					Ports:           []v1.ServicePort{},
+				},
+			},
+			expectedErrMsg: "a configuration for both subnets must be specified",
+		},
+		"internal lb missing subnet1": {
+			defaultSubnetTwo: "two",
+			service: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "kube-system",
+					Name:      "testservice",
+					UID:       "test-uid",
+					Annotations: map[string]string{
+						ServiceAnnotationLoadBalancerInternal: "",
+					},
+				},
+				Spec: v1.ServiceSpec{
+					SessionAffinity: v1.ServiceAffinityNone,
+					Ports:           []v1.ServicePort{},
+				},
+			},
+			expectedErrMsg: "a configuration for subnet1 must be specified for an internal load balancer",
+		},
 	}
 
 	for name, tc := range testCases {