strict validations for nodeoutbound lb

kubernetes-sigs · Apr 5, 2021 · 12a776a · 12a776a
1 parent eb1e4b9
commit 12a776a
Show file tree

Hide file tree

Showing 4 changed files with 87 additions and 184 deletions.
diff --git a/api/v1alpha4/azurecluster_default.go b/api/v1alpha4/azurecluster_default.go
@@ -160,46 +160,35 @@ func (c *AzureCluster) setNodeOutboundLBDefaults() {
 	}
 
 	lb := c.Spec.NetworkSpec.NodeOutboundLB
-	if lb.Type == "" {
-		lb.Type = Public
-	}
-	if lb.SKU == "" {
-		lb.SKU = SKUStandard
-	}
-
-	if lb.Name == "" {
-		lb.Name = c.ObjectMeta.Name
-	}
-	if len(lb.FrontendIPs) == 0 {
-
-		if lb.FrontendIPsCount == nil {
-			lb.FrontendIPsCount = pointer.Int32Ptr(1)
+	lb.Type = Public
+	lb.SKU = SKUStandard
+	lb.Name = c.ObjectMeta.Name
+
+	if lb.FrontendIPsCount == nil {
+		lb.FrontendIPsCount = pointer.Int32Ptr(1)
+	}
+
+	switch *lb.FrontendIPsCount {
+	case 0: // do nothing
+	case 1:
+		lb.FrontendIPs = []FrontendIP{
+			{
+				Name: generateFrontendIPConfigName(lb.Name),
+				PublicIP: &PublicIPSpec{
+					Name: generateNodeOutboundIPName(c.ObjectMeta.Name),
+				},
+			},
 		}
-
-		switch *lb.FrontendIPsCount {
-		case 0: // do nothing
-		case 1:
-			lb.FrontendIPs = []FrontendIP{
-				{
-					Name: generateFrontendIPConfigName(lb.Name),
-					PublicIP: &PublicIPSpec{
-						Name: generateNodeOutboundIPName(c.ObjectMeta.Name),
-					},
+	default:
+		for i := 0; i < int(*lb.FrontendIPsCount); i++ {
+			lb.FrontendIPs = append(lb.FrontendIPs, FrontendIP{
+				Name: withIndex(generateFrontendIPConfigName(lb.Name), i+1),
+				PublicIP: &PublicIPSpec{
+					Name: withIndex(generateNodeOutboundIPName(c.ObjectMeta.Name), i+1),
 				},
-			}
-		default:
-			for i := 0; i < int(*lb.FrontendIPsCount); i++ {
-				lb.FrontendIPs = append(lb.FrontendIPs, FrontendIP{
-					Name: withIndex(generateFrontendIPConfigName(lb.Name), i+1),
-					PublicIP: &PublicIPSpec{
-						Name: withIndex(generateNodeOutboundIPName(c.ObjectMeta.Name), i+1),
-					},
-				})
-			}
-
+			})
 		}
-	} else {
-		lb.FrontendIPsCount = pointer.Int32Ptr(int32(len(lb.FrontendIPs)))
+
 	}
 }
 

diff --git a/api/v1alpha4/azurecluster_default_test.go b/api/v1alpha4/azurecluster_default_test.go
@@ -131,18 +131,7 @@ func TestVnetDefaults(t *testing.T) {
 							Type: Public,
 						},
 						NodeOutboundLB: &LoadBalancerSpec{
-							Name: "my-node-outbound-lb",
-							SKU:  SKUStandard,
-							FrontendIPs: []FrontendIP{
-								{
-									Name: "ip-config",
-									PublicIP: &PublicIPSpec{
-										Name:    "public-ip",
-										DNSName: "myfqdn.azure.com",
-									},
-								},
-							},
-							Type: Public,
+							FrontendIPsCount: pointer.Int32Ptr(1),
 						},
 					},
 				},
@@ -832,63 +821,6 @@ func TestNodeOutboundLBDefaults(t *testing.T) {
 				},
 			},
 		},
-		{
-			name: "when frontend ips are configured",
-			cluster: &AzureCluster{
-				ObjectMeta: v1.ObjectMeta{
-					Name: "cluster-test",
-				},
-				Spec: AzureClusterSpec{
-					NetworkSpec: NetworkSpec{
-						APIServerLB: LoadBalancerSpec{Type: Public},
-						NodeOutboundLB: &LoadBalancerSpec{FrontendIPs: []FrontendIP{
-							{
-								Name: "fip-1",
-								PublicIP: &PublicIPSpec{
-									Name: "pip-1",
-								},
-							},
-							{
-								Name: "fip-2",
-								PublicIP: &PublicIPSpec{
-									Name: "pip-2",
-								},
-							},
-						}},
-					},
-				},
-			},
-			output: &AzureCluster{
-				ObjectMeta: v1.ObjectMeta{
-					Name: "cluster-test",
-				},
-				Spec: AzureClusterSpec{
-					NetworkSpec: NetworkSpec{
-						APIServerLB: LoadBalancerSpec{Type: Public},
-						NodeOutboundLB: &LoadBalancerSpec{
-							Name: "cluster-test",
-							Type: Public,
-							SKU:  SKUStandard,
-							FrontendIPs: []FrontendIP{
-								{
-									Name: "fip-1",
-									PublicIP: &PublicIPSpec{
-										Name: "pip-1",
-									},
-								},
-								{
-									Name: "fip-2",
-									PublicIP: &PublicIPSpec{
-										Name: "pip-2",
-									},
-								},
-							},
-							FrontendIPsCount: pointer.Int32Ptr(2),
-						},
-					},
-				},
-			},
-		},
 	}
 
 	for _, c := range cases {

diff --git a/api/v1alpha4/azurecluster_validation.go b/api/v1alpha4/azurecluster_validation.go
@@ -283,36 +283,32 @@ func validateNodeOutboundLB(lb *LoadBalancerSpec, old *LoadBalancerSpec, apiserv
 		return allErrs
 	}
 
-	// SKU should be Standard and is immutable.
-	if lb.SKU != SKUStandard {
-		allErrs = append(allErrs, field.NotSupported(fldPath.Child("sku"), lb.SKU, []string{string(SKUStandard)}))
-	}
-	if old != nil && old.SKU != "" && old.SKU != lb.SKU {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("sku"), lb.SKU, "Node outbound load balancer SKU should not be modified after AzureCluster creation."))
+	if old != nil && old.ID != lb.ID {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("id"), lb.ID, "Node outbound load balancer ID should not be modified after AzureCluster creation."))
 	}
 
-	// Type should be Public.
-	if lb.Type != Public {
-		allErrs = append(allErrs, field.NotSupported(fldPath.Child("type"), lb.Type, []string{string(Public)}))
+	if old != nil && old.Name != lb.Name {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("name"), lb.Name, "Node outbound load balancer Name should not be modified after AzureCluster creation."))
 	}
-	if old != nil && old.Type != "" && old.Type != lb.Type {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("type"), lb.Type, "Node outbound load balancer type should not be modified after AzureCluster creation."))
+
+	if old != nil && old.SKU != lb.SKU {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("sku"), lb.SKU, "Node outbound load balancer SKU should not be modified after AzureCluster creation."))
 	}
 
-	// Name should be valid.
-	if err := validateLoadBalancerName(lb.Name, fldPath.Child("name")); err != nil {
-		allErrs = append(allErrs, err)
+	if old != nil && len(lb.FrontendIPs) != len(old.FrontendIPs) {
+		allErrs = append(allErrs, field.Forbidden(fldPath.Child("frontendIPs"), "Node outbound load balancer FrontendIPs is not allowed to be modified after AzureCluster creation."))
 	}
-	if old != nil && old.Name != "" && old.Name != lb.Name {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("name"), lb.Type, "Node outbound load balancer name should not be modified after AzureCluster creation."))
+	if old != nil && len(lb.FrontendIPs) == len(old.FrontendIPs) {
+		for i, frontEndIps := range lb.FrontendIPs {
+			if frontEndIps != old.FrontendIPs[i] {
+				allErrs = append(allErrs, field.Invalid(fldPath.Child("frontendIPs").Index(i), frontEndIps,
+					"Node outbound load balancer FrontendIPs is not allowed to be modified after AzureCluster creation."))
+			}
+		}
 	}
 
-	for i, frontEndIps := range lb.FrontendIPs {
-
-		if frontEndIps.PrivateIPAddress != "" {
-			allErrs = append(allErrs, field.Forbidden(fldPath.Child("frontendIPConfigs").Index(i).Child("privateIP"),
-				"Public Load Balancers cannot have a Private IP"))
-		}
+	if old != nil && old.Type != lb.Type {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("type"), lb.SKU, "Node outbound load balancer Type should not be modified after AzureCluster creation."))
 	}
 
 	if lb.FrontendIPsCount != nil && *lb.FrontendIPsCount > MaxLoadBalancerOutboundIPs {

diff --git a/api/v1alpha4/azurecluster_validation_test.go b/api/v1alpha4/azurecluster_validation_test.go
@@ -790,81 +790,78 @@ func TestValidateNodeOutboundLB(t *testing.T) {
 			wantErr:     false,
 		},
 		{
-			name: "invalid SKU",
+			name: "invalid ID update",
 			lb: &LoadBalancerSpec{
-				Name: "my-awesome-lb",
-				SKU:  "Awesome",
-				FrontendIPs: []FrontendIP{
-					{
-						Name: "ip-config",
-					},
-				},
-				Type: Public,
+				ID: "some-id",
+			},
+			old: &LoadBalancerSpec{
+				ID: "old-id",
 			},
 			wantErr: true,
 			expectedErr: field.Error{
-				Type:     "FieldValueNotSupported",
-				Field:    "nodeOutboundLB.sku",
-				BadValue: "Awesome",
-				Detail:   "supported values: \"Standard\"",
+				Type:     "FieldValueInvalid",
+				Field:    "nodeOutboundLB.id",
+				BadValue: "some-id",
+				Detail:   "Node outbound load balancer ID should not be modified after AzureCluster creation.",
 			},
 		},
 		{
-			name: "invalid Type",
+			name: "invalid Name update",
 			lb: &LoadBalancerSpec{
-				Type: "Foo",
+				Name: "some-name",
+			},
+			old: &LoadBalancerSpec{
+				Name: "old-name",
 			},
 			wantErr: true,
 			expectedErr: field.Error{
-				Type:     "FieldValueNotSupported",
-				Field:    "nodeOutboundLB.type",
-				BadValue: "Foo",
-				Detail:   "supported values: \"Public\"",
+				Type:     "FieldValueInvalid",
+				Field:    "nodeOutboundLB.name",
+				BadValue: "some-name",
+				Detail:   "Node outbound load balancer Name should not be modified after AzureCluster creation.",
 			},
 		},
 		{
-			name: "invalid Name",
+			name: "invalid SKU update",
 			lb: &LoadBalancerSpec{
-				Name: "***",
+				SKU: "some-sku",
+			},
+			old: &LoadBalancerSpec{
+				SKU: "old-sku",
 			},
 			wantErr: true,
 			expectedErr: field.Error{
 				Type:     "FieldValueInvalid",
-				Field:    "nodeOutboundLB.name",
-				BadValue: "***",
-				Detail:   "name of load balancer doesn't match regex ^[-\\w\\._]+$",
+				Field:    "nodeOutboundLB.sku",
+				BadValue: "some-sku",
+				Detail:   "Node outbound load balancer SKU should not be modified after AzureCluster creation.",
 			},
 		},
 		{
-			name: "public LB with private IP",
+			name: "invalid FrontendIps update",
 			lb: &LoadBalancerSpec{
-				Type: Public,
-				FrontendIPs: []FrontendIP{
-					{
-						Name: "ip-1",
-					},
-					{
-						Name:             "ip-2",
-						PrivateIPAddress: "10.0.0.4",
-					},
-				},
+				FrontendIPs: []FrontendIP{{
+					Name: "some-frontend-ip",
+				}},
+			},
+			old: &LoadBalancerSpec{
+				FrontendIPs: []FrontendIP{{
+					Name: "old-frontend-ip",
+				}},
 			},
 			wantErr: true,
 			expectedErr: field.Error{
-				Type:   "FieldValueForbidden",
-				Field:  "nodeOutboundLB.frontendIPConfigs[1].privateIP",
-				Detail: "Public Load Balancers cannot have a Private IP",
+				Type:  "FieldValueInvalid",
+				Field: "nodeOutboundLB.frontendIPs[0]",
+				BadValue: FrontendIP{
+					Name: "some-frontend-ip",
+				},
+				Detail: "Node outbound load balancer FrontendIPs is not allowed to be modified after AzureCluster creation.",
 			},
 		},
 		{
 			name: "frontend ips count exceeds max value",
 			lb: &LoadBalancerSpec{
-				Type: Public,
-				FrontendIPs: []FrontendIP{
-					{
-						Name: "ip-1",
-					},
-				},
 				FrontendIPsCount: pointer.Int32Ptr(100),
 			},
 			wantErr: true,
@@ -953,17 +950,6 @@ func createValidAPIServerLB() LoadBalancerSpec {
 
 func createValidNodeOutboundLB() *LoadBalancerSpec {
 	return &LoadBalancerSpec{
-		Name: "my-node-outbound-lb",
-		SKU:  SKUStandard,
-		FrontendIPs: []FrontendIP{
-			{
-				Name: "ip-config",
-				PublicIP: &PublicIPSpec{
-					Name:    "public-ip",
-					DNSName: "myfqdn.azure.com",
-				},
-			},
-		},
-		Type: Public,
+		FrontendIPsCount: pointer.Int32Ptr(1),
 	}
 }