Make inbound NAT rules reconcile/delete async

azure/converters/inboundnatrules.go

@@ -0,0 +1,34 @@
+/*
+Copyright 2021 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package converters
+
+import (
+	"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network"
+)
+
+type ExistingInboundNatSpec struct {
+	Name         string
+	FrontendPort int32
+}
+
+// SKUtoSDK converts infrav1.SKU into a network.LoadBalancerSkuName.
+func InboundNatRuleToExistingInboundNatSpec(rule network.InboundNatRule) ExistingInboundNatSpec {
+	return ExistingInboundNatSpec{
+		Name:         *rule.Name,
+		FrontendPort: *rule.InboundNatRulePropertiesFormat.FrontendPort,
+	}
+}

azure/interfaces.go

@@ -62,6 +62,7 @@ type NetworkDescriber interface {
 	SetSubnet(infrav1.SubnetSpec)
 	IsIPv6Enabled() bool
 	ControlPlaneRouteTable() infrav1.RouteTable
+	APIServerLB() *infrav1.LoadBalancerSpec
 	APIServerLBName() string
 	APIServerLBPoolName(string) string
 	IsAPIServerPrivate() bool

azure/scope/machine.go

@@ -39,6 +39,8 @@ 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/inboundnatrules"
 	"sigs.k8s.io/cluster-api-provider-azure/azure/services/resourceskus"
 	"sigs.k8s.io/cluster-api-provider-azure/azure/services/virtualmachines"
 	"sigs.k8s.io/cluster-api-provider-azure/util/futures"
@@ -191,16 +193,22 @@ func (m *MachineScope) PublicIPSpecs() []azure.PublicIPSpec {
 }
 
 // InboundNatSpecs returns the inbound NAT specs.
-func (m *MachineScope) InboundNatSpecs() []azure.InboundNatSpec {
+// The existing inbound nat rules are needed in order to find an availalbe SSH port for each inbound NAT rule
+func (m *MachineScope) InboundNatSpecs(existingRules []converters.ExistingInboundNatSpec) []azure.ResourceSpecGetter {
 	if m.Role() == infrav1.ControlPlane {
-		return []azure.InboundNatSpec{
-			{
-				Name:             m.Name(),
-				LoadBalancerName: m.APIServerLBName(),
+		ipConfig := m.APIServerLB().FrontendIPs[0].Name
+		id := azure.FrontendIPConfigID(m.SubscriptionID(), m.ResourceGroup(), m.APIServerLBName(), ipConfig)
+
+		return []azure.ResourceSpecGetter{
+			&inboundnatrules.InboundNatSpec{
+				Name:                      m.Name(),
+				LoadBalancerName:          m.APIServerLBName(),
+				FrontendIPConfigurationID: id,
+				ExistingRules:             existingRules,
 			},
 		}
 	}
-	return []azure.InboundNatSpec{}
+	return []azure.ResourceSpecGetter{}
 }
 
 // NICSpecs returns the network interface specs.

azure/scope/managedcontrolplane.go

@@ -300,6 +300,11 @@ func (s *ManagedControlPlaneScope) IsVnetManaged() bool {
 	return true
 }
 
+// APIServerLBName returns the API Server LB spec
+func (s *ManagedControlPlaneScope) APIServerLB() *infrav1.LoadBalancerSpec {
+	return nil // does not apply for AKS
+}
+
 // APIServerLBName returns the API Server LB name.
 func (s *ManagedControlPlaneScope) APIServerLBName() string {
 	return "" // does not apply for AKS

azure/services/inboundnatrules/client.go

@@ -18,19 +18,28 @@ package inboundnatrules
 
 import (
 	"context"
+	"encoding/json"
+	"fmt"
 
 	"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network"
 	"github.com/Azure/go-autorest/autorest"
+	azureautorest "github.com/Azure/go-autorest/autorest/azure"
+	"github.com/pkg/errors"
 
+	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/util/reconciler"
 	"sigs.k8s.io/cluster-api-provider-azure/util/tele"
 )
 
 // client wraps go-sdk.
 type client interface {
+	List(context.Context, string, string) ([]network.InboundNatRule, error)
 	Get(context.Context, string, string, string) (network.InboundNatRule, error)
-	CreateOrUpdate(context.Context, string, string, string, network.InboundNatRule) error
-	Delete(context.Context, string, string, string) error
+	CreateOrUpdateAsync(context.Context, azure.ResourceSpecGetter) (interface{}, azureautorest.FutureAPI, error)
+	DeleteAsync(context.Context, azure.ResourceSpecGetter) (azureautorest.FutureAPI, error)
+	IsDone(context.Context, azureautorest.FutureAPI) (bool, error)
+	Result(context.Context, azureautorest.FutureAPI, string) (interface{}, error)
 }
 
 // azureClient contains the Azure go-sdk Client.
@@ -42,8 +51,10 @@ var _ client = (*azureClient)(nil)
 
 // newClient creates a new inbound NAT rules client from subscription ID.
 func newClient(auth azure.Authorizer) *azureClient {
-	c := newInboundNatRulesClient(auth.SubscriptionID(), auth.BaseURI(), auth.Authorizer())
-	return &azureClient{c}
+	inboundNatRulesClient := newInboundNatRulesClient(auth.SubscriptionID(), auth.BaseURI(), auth.Authorizer())
+	return &azureClient{
+		inboundnatrules: inboundNatRulesClient,
+	}
 }
 
 // newLoadbalancersClient creates a new inbound NAT rules client from subscription ID.
@@ -55,42 +66,150 @@ func newInboundNatRulesClient(subscriptionID string, baseURI string, authorizer
 
 // Get gets the specified inbound NAT rules.
 func (ac *azureClient) Get(ctx context.Context, resourceGroupName, lbName, inboundNatRuleName string) (network.InboundNatRule, error) {
-	ctx, _, done := tele.StartSpanWithLogger(ctx, "inboundnatrules.AzureClient.Get")
+	ctx, _, done := tele.StartSpanWithLogger(ctx, "inboundnatrules.azureClient.Get")
 	defer done()
 
 	return ac.inboundnatrules.Get(ctx, resourceGroupName, lbName, inboundNatRuleName, "")
 }
 
-// CreateOrUpdate creates or updates a inbound NAT rules.
-func (ac *azureClient) CreateOrUpdate(ctx context.Context, resourceGroupName string, lbName string, inboundNatRuleName string, inboundNatRuleParameters network.InboundNatRule) error {
-	ctx, _, done := tele.StartSpanWithLogger(ctx, "inboundnatrules.AzureClient.CreateOrUpdate")
+// List returns all inbound NAT rules on a load balancer
+func (ac *azureClient) List(ctx context.Context, resourceGroupName, lbName string) ([]network.InboundNatRule, error) {
+	ctx, _, done := tele.StartSpanWithLogger(ctx, "inboundnatrules.azureClient.List")
 	defer done()
 
-	future, err := ac.inboundnatrules.CreateOrUpdate(ctx, resourceGroupName, lbName, inboundNatRuleName, inboundNatRuleParameters)
+	iter, err := ac.inboundnatrules.ListComplete(ctx, resourceGroupName, lbName)
 	if err != nil {
-		return err
+		return nil, errors.Wrap(err, fmt.Sprintf("could not list inbound NAT rules for load balancer %s", lbName))
+	}
+
+	var natRules []network.InboundNatRule
+	for iter.NotDone() {
+		natRules = append(natRules, iter.Value())
+		if err := iter.NextWithContext(ctx); err != nil {
+			return natRules, errors.Wrap(err, "could not iterate inbound NAT rules")
+		}
+	}
+
+	return natRules, nil
+}
+
+// CreateOrUpdateAsync creates or updates a inbound NAT rule 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, _, done := tele.StartSpanWithLogger(ctx, "inboundnatrules.azureClient.CreateOrUpdateAsync")
+	defer done()
+
+	var existingNatRule interface{}
+
+	if existing, err := ac.Get(ctx, spec.ResourceGroupName(), spec.OwnerResourceName(), spec.ResourceName()); err != nil && !azure.ResourceNotFound(err) {
+		return nil, nil, errors.Wrapf(err, "failed to get inbound NAT rule %s for %s in %s", spec.ResourceName(), spec.OwnerResourceName(), spec.ResourceGroupName())
+	} else if err == nil {
+		existingNatRule = existing
 	}
+
+	params, err := spec.Parameters(existingNatRule)
+	if err != nil {
+		return nil, nil, errors.Wrapf(err, "failed to get desired parameters for inbound NAT rule %s", spec.ResourceName())
+	}
+
+	natRule, ok := params.(network.InboundNatRule)
+	if !ok {
+		if params == nil {
+			// nothing to do here.
+			return existingNatRule, nil, nil
+		}
+		return nil, nil, errors.Errorf("%T is not a network.InboundNatRule", params)
+	}
+
+	future, err := ac.inboundnatrules.CreateOrUpdate(ctx, spec.ResourceGroupName(), spec.OwnerResourceName(), spec.ResourceName(), natRule)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureCallTimeout)
+	defer cancel()
+
 	err = future.WaitForCompletionRef(ctx, ac.inboundnatrules.Client)
 	if err != nil {
-		return err
+		// if an error occurs, return the future.
+		// this means the long-running operation didn't finish in the specified timeout.
+		return nil, &future, err
 	}
-	_, err = future.Result(ac.inboundnatrules)
-	return err
+
+	result, err := future.Result(ac.inboundnatrules)
+	// if the operation completed, return a nil future
+	return result, nil, err
 }
 
-// Delete deletes the specified inbound NAT rules.
-func (ac *azureClient) Delete(ctx context.Context, resourceGroupName, lbName, inboundNatRuleName string) error {
-	ctx, _, done := tele.StartSpanWithLogger(ctx, "inboundnatrules.AzureClient.Delete")
+// DeleteAsync deletes a inbound NAT rule 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, _, done := tele.StartSpanWithLogger(ctx, "inboundnatrules.azureClient.DeleteAsync")
 	defer done()
 
-	future, err := ac.inboundnatrules.Delete(ctx, resourceGroupName, lbName, inboundNatRuleName)
+	future, err := ac.inboundnatrules.Delete(ctx, spec.ResourceGroupName(), spec.OwnerResourceName(), spec.ResourceName())
 	if err != nil {
-		return err
+		return nil, err
 	}
+
+	ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureCallTimeout)
+	defer cancel()
+
 	err = future.WaitForCompletionRef(ctx, ac.inboundnatrules.Client)
 	if err != nil {
-		return err
+		// if an error occurs, return the future.
+		// this means the long-running operation didn't finish in the specified timeout.
+		return &future, err
 	}
 	_, err = future.Result(ac.inboundnatrules)
-	return err
+	// if the operation completed, return a nil future.
+	return nil, err
+}
+
+// IsDone returns true if the long-running operation has completed.
+func (ac *azureClient) IsDone(ctx context.Context, future azureautorest.FutureAPI) (bool, error) {
+	ctx, _, done := tele.StartSpanWithLogger(ctx, "inboundnatrules.azureClient.IsDone")
+	defer done()
+
+	isDone, err := future.DoneWithContext(ctx, ac.inboundnatrules)
+	if err != nil {
+		return false, errors.Wrap(err, "failed checking if the operation was complete")
+	}
+
+	return isDone, nil
+}
+
+// Result fetches the result of a long-running operation future.
+func (ac *azureClient) Result(ctx context.Context, futureData azureautorest.FutureAPI, futureType string) (interface{}, error) {
+	_, _, done := tele.StartSpanWithLogger(ctx, "inboundnatrules.azureClient.Result")
+	defer done()
+
+	if futureData == nil {
+		return nil, errors.Errorf("cannot get result from nil future")
+	}
+	var result func(client network.InboundNatRulesClient) (natRule network.InboundNatRule, err error)
+
+	switch futureType {
+	case infrav1.PutFuture:
+		var future *network.InboundNatRulesCreateOrUpdateFuture
+		jsonData, err := futureData.MarshalJSON()
+		if err != nil {
+			return nil, errors.Wrap(err, "failed to marshal future")
+		}
+		if err := json.Unmarshal(jsonData, &future); err != nil {
+			return nil, errors.Wrap(err, "failed to unmarshal future data")
+		}
+		result = (*future).Result
+
+	case infrav1.DeleteFuture:
+		// Delete does not return a result inbound NAT rule
+		return nil, nil
+
+	default:
+		return nil, errors.Errorf("unknown future type %q", futureType)
+	}
+
+	return result(ac.inboundnatrules)
 }