Refactor logic with updating subnet spec

azure/scope/cluster.go

@@ -235,6 +235,7 @@ 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.
+	// 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() {
 			natGateways = append(natGateways, &natgateways.NatGatewaySpec{
@@ -245,7 +246,7 @@ func (s *ClusterScope) NatGatewaySpecs() []azure.ResourceSpecGetter {
 				NatGatewayIP: infrav1.PublicIPSpec{
 					Name: subnet.NatGateway.NatGatewayIP.Name,
 				},
-				Subnet: subnet,
+				SubnetName: subnet.Name,
 			})
 		}
 	}

azure/services/natgateways/natgateways.go

@@ -64,7 +64,7 @@ func (s *Service) Reconcile(ctx context.Context) error {
 	defer cancel()
 
 	// We go through the list of NatGatewaySpecs to reconcile 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 creating -> creating in progress -> created (no error)
 	var result error
 	for _, natGatewaySpec := range s.Scope.NatGatewaySpecs() {
@@ -75,18 +75,15 @@ func (s *Service) Reconcile(ctx context.Context) error {
 			}
 		}
 		if err == nil && result != nil {
-			// TODO: is there a better way to do this?
-			// natGatewaySpec.Subnet.NatGateway = natGateway
+			// TODO: consider making OwnerResourceName() a no-op
 			subnetSpec := s.Scope.Subnet(natGatewaySpec.OwnerResourceName())
 			networkNatGateway, ok := natGateway.(network.NatGateway)
 			if !ok {
-				result = errors.Errorf("%T is not a network.NatGateway", natGateway)
-			}
-			natGatewayInfraV1Spec, err := ParseExistingNatGateway(networkNatGateway)
-			if err != nil {
-				result = err
+				// TODO: maybe this error shouldn't take priority
+				result = errors.Errorf("created resource %T is not a network.NatGateway", natGateway)
 			}
-			subnetSpec.NatGateway = *natGatewayInfraV1Spec
+			// 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)
 		}
 	}

azure/services/natgateways/spec.go

@@ -33,8 +33,8 @@ type NatGatewaySpec struct {
 	ResourceGroup  string
 	SubscriptionID string
 	Location       string
+	SubnetName     string
 	NatGatewayIP   infrav1.PublicIPSpec
-	Subnet         infrav1.SubnetSpec
 }
 
 // ResourceName returns the name of the Nat Gateway.
@@ -49,25 +49,23 @@ func (s *NatGatewaySpec) ResourceGroupName() string {
 
 // OwnerResourceName returns the name of the associated subnet.
 func (s *NatGatewaySpec) OwnerResourceName() string {
-	return s.Subnet.Name
+	return s.SubnetName
 }
 
 // Parameters returns the parameters for the Nat Gateway.
 func (s *NatGatewaySpec) Parameters(existing interface{}) (interface{}, error) {
 	if existing != nil {
-		if _, ok := existing.(network.NatGateway); !ok {
+		existingNatGateway, ok := existing.(network.NatGateway)
+		if !ok {
 			return nil, errors.Errorf("%T is not a network.NatGateway", existing)
 		}
 		// Nat Gateway already exists, parse existing spec
-		existingNatGateway, err := ParseExistingNatGateway(existing.(network.NatGateway))
+		publicIP, err := GetPublicIP(existingNatGateway)
 		if err != nil {
 			return nil, err
 		}
-		s.Subnet.NatGateway.ID = existingNatGateway.ID
-		if existingNatGateway.NatGatewayIP.Name == s.NatGatewayIP.Name {
+		if s.NatGatewayIP.Name == publicIP {
 			// Skip update for Nat Gateway as it exists with expected values
-			// TODO: consider refactoring this assignment since it's not apparent that this function should mutate the spec
-			s.Subnet.NatGateway = *existingNatGateway
 			return nil, nil
 		}
 	}
@@ -87,27 +85,21 @@ func (s *NatGatewaySpec) Parameters(existing interface{}) (interface{}, error) {
 	return natGatewayToCreate, nil
 }
 
-func ParseExistingNatGateway(existingNatGateway network.NatGateway) (*infrav1.NatGateway, error) {
+func GetPublicIP(natGateway network.NatGateway) (string, error) {
 	// We must have a non-nil, non-"empty" PublicIPAddresses
-	if !(existingNatGateway.PublicIPAddresses != nil && len(*existingNatGateway.PublicIPAddresses) > 0) {
-		return nil, errors.New("failed to parse PublicIPAddresses")
+	if !(natGateway.PublicIPAddresses != nil && len(*natGateway.PublicIPAddresses) > 0) {
+		return "", errors.New("failed to parse PublicIPAddresses")
 	}
 	// TODO: do we need to handle NatGateway resources w/ more than one public IP address?
-	publicIPAddressID := to.String((*existingNatGateway.PublicIPAddresses)[0].ID)
+	publicIPAddressID := to.String((*natGateway.PublicIPAddresses)[0].ID)
 	resource, err := autorest.ParseResourceID(publicIPAddressID)
 	if err != nil {
-		return nil, errors.Wrap(err, "failed to parse Resource ID from PublicIPAddresses ID")
+		return "", 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 "", 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: resource.ResourceName,
-		},
-	}, nil
+	return resource.ResourceName, nil
 }