From 11adba6dd536726197dfe06cc60f3330101142a4 Mon Sep 17 00:00:00 2001 From: Cecile Robert-Michon Date: Thu, 6 Jul 2023 23:16:25 +0000 Subject: [PATCH] Don't default NAT Gateway for existing node subnet --- api/v1beta1/azurecluster_default.go | 7 +- api/v1beta1/azurecluster_default_test.go | 67 ++++++++++++ azure/services/subnets/spec.go | 54 ++++++---- azure/services/subnets/spec_test.go | 124 +++++++++++++++++++++++ 4 files changed, 228 insertions(+), 24 deletions(-) diff --git a/api/v1beta1/azurecluster_default.go b/api/v1beta1/azurecluster_default.go index a9a0526cf67..a96a5409a89 100644 --- a/api/v1beta1/azurecluster_default.go +++ b/api/v1beta1/azurecluster_default.go @@ -125,9 +125,10 @@ func (c *AzureCluster) setSubnetDefaults() { subnet.RouteTable.Name = generateNodeRouteTableName(c.ObjectMeta.Name) } - if !subnet.IsIPv6Enabled() { - // NAT gateway supports the use of IPv4 public IP addresses for outbound connectivity. - // So default use the NAT gateway for outbound traffic in IPv4 cluster instead of loadbalancer. + // NAT gateway only supports the use of IPv4 public IP addresses for outbound connectivity. + // So default use the NAT gateway for outbound traffic in IPv4 cluster instead of loadbalancer. + // We assume that if the ID is set, the subnet already exists so we shouldn't add a NAT gateway. + if !subnet.IsIPv6Enabled() && subnet.ID == "" { if subnet.NatGateway.Name == "" { subnet.NatGateway.Name = withIndex(generateNatGatewayName(c.ObjectMeta.Name), nodeSubnetCounter) } diff --git a/api/v1beta1/azurecluster_default_test.go b/api/v1beta1/azurecluster_default_test.go index 6504ae240e9..ef6be61f701 100644 --- a/api/v1beta1/azurecluster_default_test.go +++ b/api/v1beta1/azurecluster_default_test.go @@ -724,6 +724,73 @@ func TestSubnetDefaults(t *testing.T) { }, }, }, + { + name: "don't default NAT Gateway if subnet already exists", + cluster: &AzureCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-test", + }, + Spec: AzureClusterSpec{ + NetworkSpec: NetworkSpec{ + Subnets: Subnets{ + { + SubnetClassSpec: SubnetClassSpec{ + Role: SubnetControlPlane, + Name: "cluster-test-controlplane-subnet", + }, + ID: "my-subnet-id", + }, + { + SubnetClassSpec: SubnetClassSpec{ + Role: SubnetNode, + Name: "cluster-test-node-subnet", + }, + ID: "my-subnet-id-2", + }, + }, + }, + }, + }, + output: &AzureCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-test", + }, + Spec: AzureClusterSpec{ + NetworkSpec: NetworkSpec{ + Subnets: Subnets{ + { + SubnetClassSpec: SubnetClassSpec{ + Role: SubnetControlPlane, + CIDRBlocks: []string{DefaultControlPlaneSubnetCIDR}, + Name: "cluster-test-controlplane-subnet", + }, + ID: "my-subnet-id", + SecurityGroup: SecurityGroup{Name: "cluster-test-controlplane-nsg"}, + RouteTable: RouteTable{}, + }, + { + SubnetClassSpec: SubnetClassSpec{ + Role: SubnetNode, + CIDRBlocks: []string{DefaultNodeSubnetCIDR}, + Name: "cluster-test-node-subnet", + }, + ID: "my-subnet-id-2", + SecurityGroup: SecurityGroup{Name: "cluster-test-node-nsg"}, + RouteTable: RouteTable{Name: "cluster-test-node-routetable"}, + NatGateway: NatGateway{ + NatGatewayClassSpec: NatGatewayClassSpec{ + Name: "", + }, + NatGatewayIP: PublicIPSpec{ + Name: "", + }, + }, + }, + }, + }, + }, + }, + }, } for _, c := range cases { diff --git a/azure/services/subnets/spec.go b/azure/services/subnets/spec.go index 5da1300cdc7..cb2a645b038 100644 --- a/azure/services/subnets/spec.go +++ b/azure/services/subnets/spec.go @@ -66,27 +66,7 @@ func (s *SubnetSpec) Parameters(ctx context.Context, existing interface{}) (para return nil, errors.Errorf("%T is not a network.Subnet", existing) } - // No modifications for non-managed subnets - if !s.IsVNetManaged { - return nil, nil - } - - var existingServiceEndpoints []network.ServiceEndpointPropertiesFormat - if existingSubnet.ServiceEndpoints != nil { - for _, se := range *existingSubnet.ServiceEndpoints { - existingServiceEndpoints = append(existingServiceEndpoints, network.ServiceEndpointPropertiesFormat{Service: se.Service, Locations: se.Locations}) - } - } - var newServiceEndpoints []network.ServiceEndpointPropertiesFormat - for _, se := range s.ServiceEndpoints { - se := se - newServiceEndpoints = append(newServiceEndpoints, network.ServiceEndpointPropertiesFormat{Service: pointer.String(se.Service), Locations: &se.Locations}) - } - - // Right now only serviceEndpoints are allowed to be updated. More to come later - diff := cmp.Diff(newServiceEndpoints, existingServiceEndpoints) - if diff == "" { - // up to date, nothing to do + if !s.shouldUpdate(existingSubnet) { return nil, nil } } @@ -135,3 +115,35 @@ func (s *SubnetSpec) Parameters(ctx context.Context, existing interface{}) (para SubnetPropertiesFormat: &subnetProperties, }, nil } + +// shouldUpdate returns true if an existing subnet should be updated. +func (s *SubnetSpec) shouldUpdate(existingSubnet network.Subnet) bool { + // No modifications for non-managed subnets + if !s.IsVNetManaged { + return false + } + + // Update the subnet a NAT Gateway was added for backwards compatibility. + if s.NatGatewayName != "" && existingSubnet.SubnetPropertiesFormat.NatGateway == nil { + return true + } + + // Update the subnet if the service endpoints changed. + if existingSubnet.ServiceEndpoints != nil || len(s.ServiceEndpoints) > 0 { + var existingServiceEndpoints []network.ServiceEndpointPropertiesFormat + if existingSubnet.ServiceEndpoints != nil { + for _, se := range *existingSubnet.ServiceEndpoints { + existingServiceEndpoints = append(existingServiceEndpoints, network.ServiceEndpointPropertiesFormat{Service: se.Service, Locations: se.Locations}) + } + } + newServiceEndpoints := make([]network.ServiceEndpointPropertiesFormat, len(s.ServiceEndpoints)) + for _, se := range s.ServiceEndpoints { + se := se + newServiceEndpoints = append(newServiceEndpoints, network.ServiceEndpointPropertiesFormat{Service: pointer.String(se.Service), Locations: &se.Locations}) + } + + diff := cmp.Diff(newServiceEndpoints, existingServiceEndpoints) + return diff != "" + } + return false +} diff --git a/azure/services/subnets/spec_test.go b/azure/services/subnets/spec_test.go index f0be79babd7..fdfa1b24af6 100644 --- a/azure/services/subnets/spec_test.go +++ b/azure/services/subnets/spec_test.go @@ -186,3 +186,127 @@ func TestParameters(t *testing.T) { }) } } + +func TestSubnetSpec_shouldUpdate(t *testing.T) { + type fields struct { + Name string + ResourceGroup string + SubscriptionID string + CIDRs []string + VNetName string + VNetResourceGroup string + IsVNetManaged bool + RouteTableName string + SecurityGroupName string + Role infrav1.SubnetRole + NatGatewayName string + ServiceEndpoints infrav1.ServiceEndpoints + } + type args struct { + existingSubnet network.Subnet + } + tests := []struct { + name string + fields fields + args args + want bool + }{ + { + name: "subnet should not be updated when the VNet is not managed", + fields: fields{ + Name: "my-subnet", + ResourceGroup: "my-rg", + SubscriptionID: "123", + IsVNetManaged: false, + }, + args: args{ + existingSubnet: network.Subnet{ + Name: pointer.String("my-subnet"), + }, + }, + want: false, + }, + { + name: "subnet should be updated when NAT Gateway gets added", + fields: fields{ + Name: "my-subnet", + ResourceGroup: "my-rg", + SubscriptionID: "123", + IsVNetManaged: true, + NatGatewayName: "my-nat-gateway", + }, + args: args{ + existingSubnet: network.Subnet{ + Name: pointer.String("my-subnet"), + SubnetPropertiesFormat: &network.SubnetPropertiesFormat{ + NatGateway: nil, + }, + }, + }, + want: true, + }, + { + name: "subnet should be updated if service endpoints changed", + fields: fields{ + Name: "my-subnet", + ResourceGroup: "my-rg", + SubscriptionID: "123", + IsVNetManaged: true, + ServiceEndpoints: infrav1.ServiceEndpoints{ + { + Service: "Microsoft.Storage", + }, + }, + }, + args: args{ + existingSubnet: network.Subnet{ + Name: pointer.String("my-subnet"), + SubnetPropertiesFormat: &network.SubnetPropertiesFormat{ + ServiceEndpoints: nil, + }, + }, + }, + want: true, + }, + { + name: "subnet should not be updated if other properties change", + fields: fields{ + Name: "my-subnet", + ResourceGroup: "my-rg", + SubscriptionID: "123", + IsVNetManaged: true, + CIDRs: []string{"10.1.0.0/16"}, + }, + args: args{ + existingSubnet: network.Subnet{ + Name: pointer.String("my-subnet"), + SubnetPropertiesFormat: &network.SubnetPropertiesFormat{ + AddressPrefixes: &[]string{"10.1.0.0/8"}, + }, + }, + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := &SubnetSpec{ + Name: tt.fields.Name, + ResourceGroup: tt.fields.ResourceGroup, + SubscriptionID: tt.fields.SubscriptionID, + CIDRs: tt.fields.CIDRs, + VNetName: tt.fields.VNetName, + VNetResourceGroup: tt.fields.VNetResourceGroup, + IsVNetManaged: tt.fields.IsVNetManaged, + RouteTableName: tt.fields.RouteTableName, + SecurityGroupName: tt.fields.SecurityGroupName, + Role: tt.fields.Role, + NatGatewayName: tt.fields.NatGatewayName, + ServiceEndpoints: tt.fields.ServiceEndpoints, + } + if got := s.shouldUpdate(tt.args.existingSubnet); got != tt.want { + t.Errorf("SubnetSpec.shouldUpdate() = %v, want %v", got, tt.want) + } + }) + } +}