fix: don't create outbound LB if using NatGateway

Author: jackfrancis

api/v1alpha4/azurecluster_default.go

@@ -202,18 +202,18 @@ func (c *AzureCluster) setNodeOutboundLBDefaults() {
 			return
 		}
 
-		var oneSubnetWithoutNatGateway bool
+		var needsOutboundLB bool
 		for _, subnet := range c.Spec.NetworkSpec.Subnets {
 			if subnet.Role == SubnetNode && !subnet.IsNatGatewayEnabled() {
-				oneSubnetWithoutNatGateway = true
+				needsOutboundLB = true
 				break
 			}
 		}
 
 		// If we don't default the outbound LB when there are some subnets with nat gateway,
 		// and some without, those without wouldn't have outbound traffic. So taking the
 		// safer route, we configure the outbound LB in that scenario.
-		if len(c.Spec.NetworkSpec.Subnets) > 0 && !oneSubnetWithoutNatGateway {
+		if !needsOutboundLB {
 			return
 		}
 

azure/services/natgateways/natgateways.go

@@ -18,8 +18,10 @@ package natgateways
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-06-01/network"
+	autorest "github.com/Azure/go-autorest/autorest/azure"
 	"github.com/Azure/go-autorest/autorest/to"
 	"github.com/go-logr/logr"
 	"github.com/pkg/errors"
@@ -78,6 +80,8 @@ func (s *Service) Reconcile(ctx context.Context) error {
 				natGatewaySpec.Subnet.NatGateway = *existingNatGateway
 				s.Scope.SetSubnet(natGatewaySpec.Subnet)
 				continue
+			} else {
+				s.Scope.Error(errors.New("unexpected NatGateway name mismatch"), "NatGateway spec Name did not match actual NatGateway resource name", "actual NatGatewayIP.Name", existingNatGateway.NatGatewayIP.Name, "spec NatGatewayIP.Name", natGatewaySpec.NatGatewayIP.Name)
 			}
 		default:
 			// nat gateway doesn't exist but its name was specified in the subnet, let's create it
@@ -118,12 +122,26 @@ func (s *Service) getExisting(ctx context.Context, spec azure.NatGatewaySpec) (*
 	if err != nil {
 		return nil, err
 	}
+	// We must have a non-nil PublicIPAddresses, and the underlying SubResource slice type must have a length of 1
+	// TODO do we want to eventually handle NatGateway resources w/ more than one public IP address?
+	if !(existingNatGateway.PublicIPAddresses != nil && len(*existingNatGateway.PublicIPAddresses) == 1) {
+		return nil, errors.Wrap(err, "failed to parse PublicIPAddresses")
+	}
+	publicIPAddressID := to.String((*existingNatGateway.PublicIPAddresses)[0].ID)
+	resource, err := autorest.ParseResourceID(publicIPAddressID)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to parse Resource ID from PublicIPAddresses ID")
+	}
+	// We depend upon a non-empty ResourceName string
+	if resource.ResourceName == "" {
+		return nil, errors.Wrap(err, fmt.Sprintf("got unexpected ResourceName value from NatGateway PublicIpAddress, ResourceName=%s", resource.ResourceName))
+	}
 
 	return &infrav1.NatGateway{
 		ID:   to.String(existingNatGateway.ID),
 		Name: to.String(existingNatGateway.Name),
 		NatGatewayIP: infrav1.PublicIPSpec{
-			Name: to.String((*existingNatGateway.PublicIPAddresses)[0].ID),
+			Name: resource.ResourceName,
 		},
 	}, nil
 }

azure/services/subnets/subnets.go

@@ -63,13 +63,8 @@ func (s *Service) Reconcile(ctx context.Context) error {
 			return errors.Wrapf(err, "failed to get subnet %s", subnetSpec.Name)
 		case err == nil:
 			// subnet already exists, update the spec and skip creation
-			var subnet infrav1.SubnetSpec
-			subnet.ID = existingSubnet.ID
-			subnet.Name = existingSubnet.Name
-			subnet.Role = existingSubnet.Role
-			subnet.CIDRBlocks = existingSubnet.CIDRBlocks
-
-			s.Scope.SetSubnet(subnet)
+			s.Scope.SetSubnet(*existingSubnet)
+			continue
 
 		case !s.Scope.IsVnetManaged():
 			return fmt.Errorf("vnet was provided but subnet %s is missing", subnetSpec.Name)
@@ -167,12 +162,9 @@ func (s *Service) getExisting(ctx context.Context, rgName string, spec azure.Sub
 		addresses = to.StringSlice(subnet.SubnetPropertiesFormat.AddressPrefixes)
 	}
 
-	subnetSpec := &infrav1.SubnetSpec{
-		Role:       spec.Role,
-		Name:       to.String(subnet.Name),
-		ID:         to.String(subnet.ID),
-		CIDRBlocks: addresses,
-	}
+	subnetSpec := s.Scope.Subnet(spec.Name)
+	subnetSpec.ID = to.String(subnet.ID)
+	subnetSpec.CIDRBlocks = addresses
 
-	return subnetSpec, nil
+	return &subnetSpec, nil
 }