From 12a776a24ac030f6bd34053c10da2bcbebae266b Mon Sep 17 00:00:00 2001 From: shysank Date: Wed, 24 Mar 2021 00:31:21 +0530 Subject: [PATCH] strict validations for nodeoutbound lb --- api/v1alpha4/azurecluster_default.go | 63 ++++++------- api/v1alpha4/azurecluster_default_test.go | 70 +------------- api/v1alpha4/azurecluster_validation.go | 40 ++++---- api/v1alpha4/azurecluster_validation_test.go | 98 +++++++++----------- 4 files changed, 87 insertions(+), 184 deletions(-) diff --git a/api/v1alpha4/azurecluster_default.go b/api/v1alpha4/azurecluster_default.go index 89087d0c227..8603a6e4659 100644 --- 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 index 2621e6815a6..464193ebd8d 100644 --- 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 index d7dfea1f5b5..b1a7b34edba 100644 --- 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 index c3ca1c85dab..e23b63f8726 100644 --- 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), } }