Refactor to remove intermediate converter type

azure/scope/machine.go

@@ -38,7 +38,6 @@ import (
 
 	infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1"
 	"sigs.k8s.io/cluster-api-provider-azure/azure"
-	"sigs.k8s.io/cluster-api-provider-azure/azure/converters"
 	"sigs.k8s.io/cluster-api-provider-azure/azure/services/availabilitysets"
 	"sigs.k8s.io/cluster-api-provider-azure/azure/services/disks"
 	"sigs.k8s.io/cluster-api-provider-azure/azure/services/inboundnatrules"
@@ -195,15 +194,15 @@ func (m *MachineScope) PublicIPSpecs() []azure.PublicIPSpec {
 }
 
 // InboundNatSpecs returns the inbound NAT specs.
-func (m *MachineScope) InboundNatSpecs(existingRules []converters.ExistingInboundNatSpec) []azure.ResourceSpecGetter {
+func (m *MachineScope) InboundNatSpecs(portsInUse map[int32]struct{}) []azure.ResourceSpecGetter {
 	// The existing inbound NAT rules are needed in order to find an available SSH port for each new inbound NAT rule.
 	if m.Role() == infrav1.ControlPlane {
 		spec := &inboundnatrules.InboundNatSpec{
 			Name:                      m.Name(),
 			ResourceGroup:             m.ResourceGroup(),
 			LoadBalancerName:          m.APIServerLBName(),
 			FrontendIPConfigurationID: nil,
-			ExistingRules:             existingRules,
+			PortsInUse:                portsInUse,
 		}
 		if frontEndIPs := m.APIServerLB().FrontendIPs; len(frontEndIPs) > 0 {
 			ipConfig := frontEndIPs[0].Name

azure/scope/machine_test.go

@@ -32,7 +32,6 @@ import (
 
 	infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1"
 	"sigs.k8s.io/cluster-api-provider-azure/azure"
-	"sigs.k8s.io/cluster-api-provider-azure/azure/converters"
 	"sigs.k8s.io/cluster-api-provider-azure/azure/services/disks"
 	"sigs.k8s.io/cluster-api-provider-azure/azure/services/inboundnatrules"
 )
@@ -347,7 +346,7 @@ func TestMachineScope_InboundNatSpecs(t *testing.T) {
 					LoadBalancerName:          "foo-loadbalancer",
 					ResourceGroup:             "my-rg",
 					FrontendIPConfigurationID: to.StringPtr(azure.FrontendIPConfigID("123", "my-rg", "foo-loadbalancer", "foo-frontend-ip")),
-					ExistingRules:             []converters.ExistingInboundNatSpec{},
+					PortsInUse:                make(map[int32]struct{}),
 				},
 			},
 		},
@@ -356,7 +355,7 @@ func TestMachineScope_InboundNatSpecs(t *testing.T) {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			if got := tt.machineScope.InboundNatSpecs([]converters.ExistingInboundNatSpec{}); !reflect.DeepEqual(got, tt.want) {
+			if got := tt.machineScope.InboundNatSpecs(make(map[int32]struct{})); !reflect.DeepEqual(got, tt.want) {
 				gotArray := "[ "
 				for _, spec := range got {
 					gotArray += fmt.Sprintf("%+v ", spec)

azure/services/inboundnatrules/inboundnatrules.go

@@ -23,7 +23,6 @@ import (
 
 	infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1"
 	"sigs.k8s.io/cluster-api-provider-azure/azure"
-	"sigs.k8s.io/cluster-api-provider-azure/azure/converters"
 	"sigs.k8s.io/cluster-api-provider-azure/azure/services/async"
 	"sigs.k8s.io/cluster-api-provider-azure/util/reconciler"
 	"sigs.k8s.io/cluster-api-provider-azure/util/tele"
@@ -36,7 +35,7 @@ type InboundNatScope interface {
 	azure.ClusterDescriber
 	azure.AsyncStatusUpdater
 	APIServerLBName() string
-	InboundNatSpecs([]converters.ExistingInboundNatSpec) []azure.ResourceSpecGetter
+	InboundNatSpecs(map[int32]struct{}) []azure.ResourceSpecGetter
 }
 
 // Service provides operations on Azure resources.
@@ -71,16 +70,19 @@ func (s *Service) Reconcile(ctx context.Context) error {
 		return result
 	}
 
-	existingRuleSpecs := make([]converters.ExistingInboundNatSpec, len(existingRules))
-	for i, rule := range existingRules {
-		existingRuleSpecs[i] = converters.InboundNatRuleToExistingInboundNatSpec(rule)
+	portsInUse := make(map[int32]struct{})
+	for _, rule := range existingRules {
+		portsInUse[*rule.InboundNatRulePropertiesFormat.FrontendPort] = struct{}{} // Mark frontend port as in use
 	}
 
 	// We go through the list of InboundNatSpecs to reconcile each one, independently of the result of the previous one.
 	// If multiple errors occur, we return the most pressing one.
 	//  Order of precedence (highest -> lowest) is: error that is not an operationNotDoneError (i.e. error creating) -> operationNotDoneError (i.e. creating in progress) -> no error (i.e. created)
 	var result error
-	for _, natRule := range s.Scope.InboundNatSpecs(existingRuleSpecs) {
+	for _, natRule := range s.Scope.InboundNatSpecs(portsInUse) {
+		// If we are creating multiple inbound NAT rules, we could have a collision in finding an available frontend port since the newly created rule takes an available port, and we do not update portsInUse in the specs.
+		// It doesn't matter in this case since we only create one rule per machine, but for multiple rules, we could end up restarting the Reconcile function each time to get the updated available ports.
+		// TODO: We can update the available ports and recompute the specs each time, or alternatively, we could deterministically calculate the ports we plan on using to avoid collisions, i.e. rule #1 uses the first available port, rule #2 uses the second available port, etc.
 		if _, err := s.CreateResource(ctx, natRule, serviceName); err != nil {
 			if !azure.IsOperationNotDoneError(err) || result == nil {
 				result = err
@@ -105,7 +107,7 @@ func (s *Service) Delete(ctx context.Context) error {
 	// We go through the list of InboundNatSpecs to delete each one, independently of the result of the previous one.
 	// If multiple errors occur, we return the most pressing one.
 	//  Order of precedence (highest -> lowest) is: error that is not an operationNotDoneError (i.e. error deleting) -> operationNotDoneError (i.e. deleting in progress) -> no error (i.e. deleted)
-	for _, natRule := range s.Scope.InboundNatSpecs(nil) {
+	for _, natRule := range s.Scope.InboundNatSpecs(make(map[int32]struct{})) {
 		if err := s.DeleteResource(ctx, natRule, serviceName); err != nil {
 			if !azure.IsOperationNotDoneError(err) || result == nil {
 				result = err

azure/services/inboundnatrules/inboundnatrules_test.go

@@ -30,7 +30,6 @@ import (
 
 	infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1"
 	"sigs.k8s.io/cluster-api-provider-azure/azure"
-	"sigs.k8s.io/cluster-api-provider-azure/azure/converters"
 	"sigs.k8s.io/cluster-api-provider-azure/azure/services/async/mock_async"
 	"sigs.k8s.io/cluster-api-provider-azure/azure/services/inboundnatrules/mock_inboundnatrules"
 	gomockinternal "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock"
@@ -40,9 +39,9 @@ var (
 	fakeLBName    = "my-lb-1"
 	fakeGroupName = "my-rg"
 
-	noExistingRuleSpecs = []converters.ExistingInboundNatSpec{}
-	noExistingRules     = []network.InboundNatRule{}
-	fakeExistingRules   = []network.InboundNatRule{
+	noPortsInUse      = getFakeExistingPortsInUse([]int{})
+	noExistingRules   = []network.InboundNatRule{}
+	fakeExistingRules = []network.InboundNatRule{
 		{
 			Name: pointer.StringPtr("other-machine-nat-rule"),
 			ID:   pointer.StringPtr("some-natrules-id"),
@@ -58,34 +57,34 @@ var (
 			},
 		},
 	}
-	fakeExistingRuleSpecs = []converters.ExistingInboundNatSpec{
-		{
-			Name:         "other-machine-nat-rule",
-			FrontendPort: 22,
-		},
-		{
-			Name:         "other-machine-nat-rule-2",
-			FrontendPort: 2201,
-		},
-	}
+	somePortsInUse = getFakeExistingPortsInUse([]int{22, 2201})
 
 	fakeNatSpecWithNoExisting = InboundNatSpec{
 		Name:                      "my-machine-1",
 		LoadBalancerName:          "my-lb-1",
 		ResourceGroup:             fakeGroupName,
 		FrontendIPConfigurationID: to.StringPtr("frontend-ip-config-id-2"),
-		ExistingRules:             noExistingRuleSpecs,
+		PortsInUse:                noPortsInUse,
 	}
 	fakeNatSpec = InboundNatSpec{
 		Name:                      "my-machine-2",
 		LoadBalancerName:          "my-lb-2",
 		ResourceGroup:             fakeGroupName,
 		FrontendIPConfigurationID: to.StringPtr("frontend-ip-config-id-2"),
-		ExistingRules:             fakeExistingRuleSpecs,
+		PortsInUse:                somePortsInUse,
 	}
 	internalError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")
 )
 
+func getFakeExistingPortsInUse(usedPorts []int) map[int32]struct{} {
+	portsInUse := make(map[int32]struct{})
+	for _, port := range usedPorts {
+		portsInUse[int32(port)] = struct{}{}
+	}
+
+	return portsInUse
+}
+
 func TestReconcileInboundNATRule(t *testing.T) {
 	testcases := []struct {
 		name          string
@@ -103,7 +102,7 @@ func TestReconcileInboundNATRule(t *testing.T) {
 				s.ResourceGroup().AnyTimes().Return(fakeGroupName)
 				s.APIServerLBName().AnyTimes().Return(fakeLBName)
 				m.List(gomockinternal.AContext(), fakeGroupName, fakeLBName).Return(noExistingRules, nil)
-				s.InboundNatSpecs(noExistingRuleSpecs).Return([]azure.ResourceSpecGetter{&fakeNatSpecWithNoExisting})
+				s.InboundNatSpecs(noPortsInUse).Return([]azure.ResourceSpecGetter{&fakeNatSpecWithNoExisting})
 				gomock.InOrder(
 					r.CreateResource(gomockinternal.AContext(), &fakeNatSpecWithNoExisting, serviceName).Return(nil, nil),
 					s.UpdatePutStatus(infrav1.InboundNATRulesReadyCondition, serviceName, nil),
@@ -119,7 +118,7 @@ func TestReconcileInboundNATRule(t *testing.T) {
 				s.ResourceGroup().AnyTimes().Return(fakeGroupName)
 				s.APIServerLBName().AnyTimes().Return("my-lb")
 				m.List(gomockinternal.AContext(), fakeGroupName, "my-lb").Return(fakeExistingRules, nil)
-				s.InboundNatSpecs(fakeExistingRuleSpecs).Return([]azure.ResourceSpecGetter{&fakeNatSpec})
+				s.InboundNatSpecs(somePortsInUse).Return([]azure.ResourceSpecGetter{&fakeNatSpec})
 				gomock.InOrder(
 					r.CreateResource(gomockinternal.AContext(), &fakeNatSpec, serviceName).Return(nil, nil),
 					s.UpdatePutStatus(infrav1.InboundNATRulesReadyCondition, serviceName, nil),
@@ -147,7 +146,7 @@ func TestReconcileInboundNATRule(t *testing.T) {
 				s.ResourceGroup().AnyTimes().Return(fakeGroupName)
 				s.APIServerLBName().AnyTimes().Return("my-lb")
 				m.List(gomockinternal.AContext(), fakeGroupName, "my-lb").Return(fakeExistingRules, nil)
-				s.InboundNatSpecs(fakeExistingRuleSpecs).Return([]azure.ResourceSpecGetter{&fakeNatSpec})
+				s.InboundNatSpecs(somePortsInUse).Return([]azure.ResourceSpecGetter{&fakeNatSpec})
 				gomock.InOrder(
 					r.CreateResource(gomockinternal.AContext(), &fakeNatSpec, serviceName).Return(nil, internalError),
 					s.UpdatePutStatus(infrav1.InboundNATRulesReadyCondition, serviceName, internalError),
@@ -198,7 +197,7 @@ func TestDeleteNetworkInterface(t *testing.T) {
 			expectedError: "",
 			expect: func(s *mock_inboundnatrules.MockInboundNatScopeMockRecorder,
 				m *mock_inboundnatrules.MockclientMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
-				s.InboundNatSpecs(nil).Return([]azure.ResourceSpecGetter{&fakeNatSpecWithNoExisting})
+				s.InboundNatSpecs(noPortsInUse).Return([]azure.ResourceSpecGetter{&fakeNatSpecWithNoExisting})
 				s.ResourceGroup().AnyTimes().Return(fakeGroupName)
 				s.APIServerLBName().AnyTimes().Return(fakeLBName)
 				gomock.InOrder(
@@ -212,7 +211,7 @@ func TestDeleteNetworkInterface(t *testing.T) {
 			expectedError: "#: Internal Server Error: StatusCode=500",
 			expect: func(s *mock_inboundnatrules.MockInboundNatScopeMockRecorder,
 				m *mock_inboundnatrules.MockclientMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
-				s.InboundNatSpecs(nil).Return([]azure.ResourceSpecGetter{&fakeNatSpecWithNoExisting})
+				s.InboundNatSpecs(noPortsInUse).Return([]azure.ResourceSpecGetter{&fakeNatSpecWithNoExisting})
 				s.ResourceGroup().AnyTimes().Return(fakeGroupName)
 				s.APIServerLBName().AnyTimes().Return(fakeLBName)
 				gomock.InOrder(

azure/services/inboundnatrules/spec.go

@@ -20,7 +20,6 @@ import (
 	"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network"
 	"github.com/Azure/go-autorest/autorest/to"
 	"github.com/pkg/errors"
-	"sigs.k8s.io/cluster-api-provider-azure/azure/converters"
 )
 
 // InboundNatSpec defines the specification for an inbound NAT rule.
@@ -29,7 +28,7 @@ type InboundNatSpec struct {
 	LoadBalancerName          string
 	ResourceGroup             string
 	FrontendIPConfigurationID *string
-	ExistingRules             []converters.ExistingInboundNatSpec
+	PortsInUse                map[int32]struct{}
 }
 
 // ResourceName returns the name of the inbound NAT rule.
@@ -61,12 +60,7 @@ func (s *InboundNatSpec) Parameters(existing interface{}) (parameters interface{
 		return nil, errors.Errorf("FrontendIPConfigurationID is not set")
 	}
 
-	portsInUse := make(map[int32]struct{})
-	for _, v := range s.ExistingRules {
-		portsInUse[v.FrontendPort] = struct{}{}
-	}
-
-	sshFrontendPort, err := getAvailablePort(portsInUse)
+	sshFrontendPort, err := getAvailablePort(s.PortsInUse)
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to find available SSH Frontend port for NAT Rule %s in load balancer %s", s.ResourceName(), s.OwnerResourceName())
 	}