Refactor spec and fix naming convention for NAT gateway

api/v1alpha4/types.go

@@ -133,7 +133,7 @@ type RouteTable struct {
 	Name string `json:"name,omitempty"`
 }
 
-// NatGateway defines an Azure Nat Gateway.
+// NatGateway defines an Azure NAT gateway.
 // NAT gateway resources are part of Vnet NAT and provide outbound Internet connectivity for subnets of a virtual network.
 type NatGateway struct {
 	ID           string       `json:"id,omitempty"`
@@ -545,7 +545,7 @@ func (n *NetworkSpec) UpdateNodeSubnet(subnet SubnetSpec) {
 	}
 }
 
-// IsNatGatewayEnabled returns whether or not a Nat Gateway is enabled on the subnet.
+// IsNatGatewayEnabled returns whether or not a NAT gateway is enabled on the subnet.
 func (s SubnetSpec) IsNatGatewayEnabled() bool {
 	return s.NatGateway.Name != ""
 }

api/v1beta1/azurecluster_default.go

@@ -219,7 +219,7 @@ func (c *AzureCluster) setNodeOutboundLBDefaults() {
 			}
 		}
 
-		// If we don't default the outbound LB when there are some subnets with nat gateway,
+		// 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 !needsOutboundLB {
@@ -401,7 +401,7 @@ func generateControlPlaneOutboundIPName(clusterName string) string {
 	return fmt.Sprintf("pip-%s-controlplane-outbound", clusterName)
 }
 
-// generateNatGatewayIPName generates a nat gateway IP name.
+// generateNatGatewayIPName generates a NAT gateway IP name.
 func generateNatGatewayIPName(clusterName, subnetName string) string {
 	return fmt.Sprintf("pip-%s-%s-natgw", clusterName, subnetName)
 }

api/v1beta1/azurecluster_default_test.go

@@ -978,7 +978,7 @@ func TestNodeOutboundLBDefaults(t *testing.T) {
 			},
 		},
 		{
-			name: "NAT Gateway enabled - no LB",
+			name: "NAT gateway enabled - no LB",
 			cluster: &AzureCluster{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "cluster-test",
@@ -1037,7 +1037,7 @@ func TestNodeOutboundLBDefaults(t *testing.T) {
 			},
 		},
 		{
-			name: "NAT Gateway enabled on 1 of 2 node subnets",
+			name: "NAT gateway enabled on 1 of 2 node subnets",
 			cluster: &AzureCluster{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "cluster-test",
@@ -1121,7 +1121,7 @@ func TestNodeOutboundLBDefaults(t *testing.T) {
 			},
 		},
 		{
-			name: "multiple node subnets, NAT Gateway not enabled in any of them",
+			name: "multiple node subnets, NAT gateway not enabled in any of them",
 			cluster: &AzureCluster{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "cluster-test",
@@ -1211,7 +1211,7 @@ func TestNodeOutboundLBDefaults(t *testing.T) {
 			},
 		},
 		{
-			name: "multiple node subnets, NAT Gateway enabled on all of them",
+			name: "multiple node subnets, NAT gateway enabled on all of them",
 			cluster: &AzureCluster{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "cluster-test",

api/v1beta1/types.go

@@ -160,10 +160,10 @@ type RouteTable struct {
 	Name string `json:"name"`
 }
 
-// NatGateway defines an Azure Nat Gateway.
+// NatGateway defines an Azure NAT gateway.
 // NAT gateway resources are part of Vnet NAT and provide outbound Internet connectivity for subnets of a virtual network.
 type NatGateway struct {
-	// ID is the Azure resource ID of the nat gateway.
+	// ID is the Azure resource ID of the NAT gateway.
 	// READ-ONLY
 	// +optional
 	ID   string `json:"id,omitempty"`
@@ -574,7 +574,7 @@ func (n *NetworkSpec) UpdateNodeSubnet(subnet SubnetSpec) {
 	}
 }
 
-// IsNatGatewayEnabled returns whether or not a Nat Gateway is enabled on the subnet.
+// IsNatGatewayEnabled returns whether or not a NAT gateway is enabled on the subnet.
 func (s SubnetSpec) IsNatGatewayEnabled() bool {
 	return s.NatGateway.Name != ""
 }

azure/defaults.go

@@ -109,7 +109,7 @@ func GenerateFrontendIPConfigName(lbName string) string {
 	return fmt.Sprintf("%s-%s", lbName, "frontEnd")
 }
 
-// GenerateNatGatewayIPName generates a nat gateway IP name.
+// GenerateNatGatewayIPName generates a NAT gateway IP name.
 func GenerateNatGatewayIPName(clusterName, subnetName string) string {
 	return fmt.Sprintf("pip-%s-%s-natgw", clusterName, subnetName)
 }
@@ -222,7 +222,7 @@ func SecurityGroupID(subscriptionID, resourceGroup, nsgName string) string {
 	return fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/networkSecurityGroups/%s", subscriptionID, resourceGroup, nsgName)
 }
 
-// NatGatewayID returns the azure resource ID for a given nat gateway.
+// NatGatewayID returns the azure resource ID for a given NAT gateway.
 func NatGatewayID(subscriptionID, resourceGroup, natgatewayName string) string {
 	return fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/natGateways/%s", subscriptionID, resourceGroup, natgatewayName)
 }

azure/scope/cluster.go

@@ -148,7 +148,7 @@ func (s *ClusterScope) PublicIPSpecs() []azure.PublicIPSpec {
 		publicIPSpecs = append(publicIPSpecs, nodeOutboundIPSpecs...)
 	}
 
-	// Public IP specs for node nat gateways
+	// Public IP specs for node NAT gateways
 	var nodeNatGatewayIPSpecs []azure.PublicIPSpec
 	for _, subnet := range s.NodeSubnets() {
 		if subnet.IsNatGatewayEnabled() {
@@ -230,11 +230,11 @@ func (s *ClusterScope) RouteTableSpecs() []azure.RouteTableSpec {
 	return routetables
 }
 
-// NatGatewaySpecs returns the node nat gateway.
+// NatGatewaySpecs returns the node NAT gateway.
 func (s *ClusterScope) NatGatewaySpecs() []azure.ResourceSpecGetter {
 	natGateways := []azure.ResourceSpecGetter{}
 
-	// We ignore the control plane nat gateway, as we will always use a LB to enable egress on the control plane.
+	// We ignore the control plane NAT gateway, as we will always use a LB to enable egress on the control plane.
 	// TODO: do we need to use setSubnet() in natgateways.go, since it doesn't add/remove any NodeSubnets() or change IsNatGatewayEnabled()
 	for _, subnet := range s.NodeSubnets() {
 		if subnet.IsNatGatewayEnabled() {
@@ -246,7 +246,6 @@ func (s *ClusterScope) NatGatewaySpecs() []azure.ResourceSpecGetter {
 				NatGatewayIP: infrav1.PublicIPSpec{
 					Name: subnet.NatGateway.NatGatewayIP.Name,
 				},
-				SubnetName: subnet.Name,
 			})
 		}
 	}

azure/scope/machine.go

@@ -229,7 +229,7 @@ func (m *MachineScope) NICSpecs() []azure.NICSpec {
 		}
 	}
 
-	// If Nat Gateway is not enabled and node has no public IP, then the NIC needs to reference the LB to get outbound traffic.
+	// If NAT gateway is not enabled and node has no public IP, then the NIC needs to reference the LB to get outbound traffic.
 	if m.Role() == infrav1.Node && !m.Subnet().IsNatGatewayEnabled() && !m.AzureMachine.Spec.AllocatePublicIP {
 		spec.PublicLBName = m.OutboundLBName(m.Role())
 		spec.PublicLBAddressPoolName = m.OutboundPoolName(m.OutboundLBName(m.Role()))

azure/scope/machine_test.go

@@ -1313,7 +1313,7 @@ func TestMachineScope_NICSpecs(t *testing.T) {
 		want         []azure.NICSpec
 	}{
 		{
-			name: "Node Machine with no nat gateway and no public IP address",
+			name: "Node Machine with no NAT gateway and no public IP address",
 			machineScope: MachineScope{
 				ClusterScoper: &ClusterScope{
 					Cluster: &clusterv1.Cluster{
@@ -1392,7 +1392,7 @@ func TestMachineScope_NICSpecs(t *testing.T) {
 			},
 		},
 		{
-			name: "Node Machine with nat gateway",
+			name: "Node Machine with NAT gateway",
 			machineScope: MachineScope{
 				ClusterScoper: &ClusterScope{
 					Cluster: &clusterv1.Cluster{

azure/scope/managedcontrolplane.go

@@ -228,7 +228,7 @@ func (s *ManagedControlPlaneScope) NodeRouteTable() infrav1.RouteTable {
 	return infrav1.RouteTable{}
 }
 
-// NodeNatGateway returns the cluster node nat gateway.
+// NodeNatGateway returns the cluster node NAT gateway.
 func (s *ManagedControlPlaneScope) NodeNatGateway() infrav1.NatGateway {
 	return infrav1.NatGateway{}
 }

azure/services/natgateways/client.go

@@ -53,39 +53,39 @@ func newClient(auth azure.Authorizer) *azureClient {
 	return &azureClient{c}
 }
 
-// netNatGatewaysClient creates a new nat gateways client from subscription ID.
+// netNatGatewaysClient creates a new NAT gateways client from subscription ID.
 func netNatGatewaysClient(subscriptionID string, baseURI string, authorizer autorest.Authorizer) network.NatGatewaysClient {
 	natGatewaysClient := network.NewNatGatewaysClientWithBaseURI(baseURI, subscriptionID)
 	azure.SetAutoRestClientDefaults(&natGatewaysClient.Client, authorizer)
 	return natGatewaysClient
 }
 
-// Get gets the specified nat gateway.
+// Get gets the specified NAT gateway.
 func (ac *azureClient) Get(ctx context.Context, resourceGroupName, natGatewayName string) (network.NatGateway, error) {
 	ctx, _, done := tele.StartSpanWithLogger(ctx, "natgateways.azureClient.Get")
 	defer done()
 
 	return ac.natgateways.Get(ctx, resourceGroupName, natGatewayName, "")
 }
 
-// CreateOrUpdateAsync creates or updates a Nat Gateway asynchronously.
+// CreateOrUpdateAsync creates or updates a NAT gateway asynchronously.
 // It sends a PUT request to Azure and if accepted without error, the func will return a Future which can be used to track the ongoing
 // progress of the operation.
 func (ac *azureClient) CreateOrUpdateAsync(ctx context.Context, spec azure.ResourceSpecGetter) (interface{}, azureautorest.FutureAPI, error) {
-	ctx, span := tele.Tracer().Start(ctx, "natgateways.azureClient.CreateOrUpdateAsync")
-	defer span.End()
+	ctx, _, done := tele.StartSpanWithLogger(ctx, "natgateways.Service.CreateOrUpdateAsync")
+	defer done()
 
 	var existingNatGateway interface{}
 
 	if existing, err := ac.Get(ctx, spec.ResourceGroupName(), spec.ResourceName()); err != nil && !azure.ResourceNotFound(err) {
-		return nil, nil, errors.Wrapf(err, "failed to get Nat Gateway %s for %s in %s", spec.ResourceName(), spec.OwnerResourceName(), spec.ResourceGroupName())
+		return nil, nil, errors.Wrapf(err, "failed to get NAT gateway %s for %s in %s", spec.ResourceName(), spec.OwnerResourceName(), spec.ResourceGroupName())
 	} else if err == nil {
 		existingNatGateway = existing
 	}
 
 	params, err := spec.Parameters(existingNatGateway)
 	if err != nil {
-		return nil, nil, errors.Wrapf(err, "failed to get desired parameters for Nat Gateway %s", spec.ResourceName())
+		return nil, nil, errors.Wrapf(err, "failed to get desired parameters for NAT gateway %s", spec.ResourceName())
 	}
 
 	natGateway, ok := params.(network.NatGateway)
@@ -118,12 +118,12 @@ func (ac *azureClient) CreateOrUpdateAsync(ctx context.Context, spec azure.Resou
 	return result, nil, err
 }
 
-// DeleteAsync deletes a Nat Gateway asynchronously. DeleteAsync sends a DELETE
+// DeleteAsync deletes a NAT gateway asynchronously. DeleteAsync sends a DELETE
 // request to Azure and if accepted without error, the func will return a Future which can be used to track the ongoing
 // progress of the operation.
 func (ac *azureClient) DeleteAsync(ctx context.Context, spec azure.ResourceSpecGetter) (azureautorest.FutureAPI, error) {
-	ctx, span := tele.Tracer().Start(ctx, "natgateways.azureClient.Delete")
-	defer span.End()
+	ctx, _, done := tele.StartSpanWithLogger(ctx, "natgateways.Service.DeleteAsync")
+	defer done()
 
 	future, err := ac.natgateways.Delete(ctx, spec.ResourceGroupName(), spec.ResourceName())
 	if err != nil {
@@ -146,8 +146,8 @@ func (ac *azureClient) DeleteAsync(ctx context.Context, spec azure.ResourceSpecG
 
 // IsDone returns true if the long-running operation has completed.
 func (ac *azureClient) IsDone(ctx context.Context, future azureautorest.FutureAPI) (bool, error) {
-	ctx, span := tele.Tracer().Start(ctx, "natgateways.azureClient.IsDone")
-	defer span.End()
+	ctx, _, done := tele.StartSpanWithLogger(ctx, "natgateways.Service.DeleteAsync")
+	defer done()
 
 	done, err := future.DoneWithContext(ctx, ac.natgateways)
 	if err != nil {
@@ -177,7 +177,7 @@ func (ac *azureClient) Result(ctx context.Context, futureData azureautorest.Futu
 		result = (*future).Result
 
 	case infrav1.DeleteFuture:
-		// Delete does not return a result Nat Gateway
+		// Delete does not return a result NAT gateway
 		return nil, nil
 
 	default:

azure/services/natgateways/natgateways.go

@@ -32,7 +32,7 @@ import (
 
 const serviceName = "natgateways"
 
-// NatGatewayScope defines the scope interface for nat gateway service.
+// NatGatewayScope defines the scope interface for NAT gateway service.
 type NatGatewayScope interface {
 	logr.Logger
 	azure.ClusterScoper
@@ -54,56 +54,56 @@ func New(scope NatGatewayScope) *Service {
 	}
 }
 
-// Reconcile gets/creates/updates a nat gateway.
-// Only when the Nat Gateway 'Name' property is defined we create the Nat Gateway: it's opt-in.
+// Reconcile gets/creates/updates a NAT gateway.
+// Only when the NAT gateway 'Name' property is defined we create the NAT gateway: it's opt-in.
 func (s *Service) Reconcile(ctx context.Context) error {
-	ctx, span := tele.Tracer().Start(ctx, "natgateways.Service.Reconcile")
-	defer span.End()
+	ctx, _, done := tele.StartSpanWithLogger(ctx, "natgateways.Service.Reconcile")
+	defer done()
 
 	ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout)
 	defer cancel()
 
-	// We go through the list of NatGatewaySpecs to reconcile each one, independently of the result of the previous one.
+	// We go through the list of NatGatewaySpecs to reconcile each one, independently of the resultingErr of the previous one.
 	// If multiple errors occur, we return the most pressing one
 	// order of precedence is: error creating -> creating in progress -> created (no error)
-	var result error
+	var resultingErr error
 	for _, natGatewaySpec := range s.Scope.NatGatewaySpecs() {
 		natGateway, err := async.CreateResource(ctx, s.Scope, s.client, natGatewaySpec, serviceName)
 		if err != nil {
-			if !azure.IsOperationNotDoneError(err) || result == nil {
-				result = err
+			if !azure.IsOperationNotDoneError(err) || resultingErr == nil {
+				resultingErr = err
 			}
 		}
-		if err == nil && result != nil {
+		if err == nil && resultingErr != nil {
 			// TODO: consider making OwnerResourceName() a no-op
 			subnetSpec := s.Scope.Subnet(natGatewaySpec.OwnerResourceName())
 			networkNatGateway, ok := natGateway.(network.NatGateway)
 			if !ok {
-				// TODO: maybe this error shouldn't take priority
-				result = errors.Errorf("created resource %T is not a network.NatGateway", natGateway)
+				// Return out of loop since this would be an unexpcted fatal error
+				return errors.Errorf("created resource %T is not a network.NatGateway", natGateway)
 			}
 			// TODO: is it necessary to set the spec since it doesn't appear to change any behavior
 			subnetSpec.NatGateway.ID = *networkNatGateway.ID
 			s.Scope.SetSubnet(subnetSpec)
 		}
 	}
 
-	s.Scope.UpdatePutStatus(infrav1.NATGatewaysReadyCondition, serviceName, result)
-	return result
+	s.Scope.UpdatePutStatus(infrav1.NATGatewaysReadyCondition, serviceName, resultingErr)
+	return resultingErr
 }
 
-// Delete deletes the nat gateway with the provided name.
+// Delete deletes the NAT gateway with the provided name.
 func (s *Service) Delete(ctx context.Context) error {
-	ctx, span := tele.Tracer().Start(ctx, "natgateways.Service.Delete")
-	defer span.End()
+	ctx, _, done := tele.StartSpanWithLogger(ctx, "natgateways.Service.Delete")
+	defer done()
 
 	ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout)
 	defer cancel()
 
 	var result error
 
 	// We go through the list of NatGatewaySpecs to delete each one, independently of the result of the previous one.
-	// If multiple erros occur, we return the most pressing one
+	// If multiple errors occur, we return the most pressing one
 	// order of precedence is: error deleting -> deleting in progress -> deleted (no error)
 	for _, natGatewaySpec := range s.Scope.NatGatewaySpecs() {
 		if err := async.DeleteResource(ctx, s.Scope, s.client, natGatewaySpec, serviceName); err != nil {
@@ -115,3 +115,13 @@ func (s *Service) Delete(ctx context.Context) error {
 	s.Scope.UpdateDeleteStatus(infrav1.NATGatewaysReadyCondition, serviceName, result)
 	return result
 }
+
+func (s *Service) SetNatGatewayID(name string, id string) {
+	for _, subnet := range s.Scope.Subnets() {
+		if subnet.NatGateway.Name == name {
+			subnet.NatGateway.ID = id
+			s.Scope.SetSubnet(subnet)
+			// s.AzureCluster.Spec.NetworkSpec.Subnets[i] = id
+		}
+	}
+}