Don't default NAT Gateway for existing node subnet

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)
 			}

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 {

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,32 @@ 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.
+	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})
+	}
+
+	diff := cmp.Diff(newServiceEndpoints, existingServiceEndpoints)
+	return diff != ""
+}

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)
+			}
+		})
+	}
+}