Merge pull request #451 from CecileRobertMichon/natrulesfix-2

🐛 make NAT Rules creation and deletion owned by network interfaces

Author: k8s-ci-robot

api/v1alpha2/types.go

@@ -262,7 +262,7 @@ type LoadBalancer struct {
 		Probes *[]Probe `json:"probes,omitempty"`
 		// InboundNatRules - Collection of inbound NAT Rules used by a load balancer. Defining inbound NAT rules on your load balancer is mutually exclusive with defining an inbound NAT pool. Inbound NAT pools are referenced from virtual machine scale sets. NICs that are associated with individual virtual machines cannot reference an Inbound NAT pool. They have to reference individual inbound NAT rules.
 		InboundNatRules *[]InboundNatRule `json:"inboundNatRules,omitempty"`
-		// InboundNatPools - Defines an external port range for inbound NAT to a single backend port on NICs associated with a load balancer. Inbound NAT rules are created automatically for each NIC associated with the Load Balancer using an external port from this range. Defining an Inbound NAT pool on your Load Balancer is mutually exclusive with defining inbound Nat rules. Inbound NAT pools are referenced from virtual machine scale sets. NICs that are associated with individual virtual machines cannot reference an inbound NAT pool. They have to reference individual inbound NAT rules.
+		// InboundNatPools - Defines an external port range for inbound NAT to a single backend port on NICs associated with a load balancer. Inbound NAT rules are created automatically for each NIC associated with the Load Balancer using an external port from this range. Defining an Inbound NAT pool on your Load Balancer is mutually exclusive with defining inbound NAT rules. Inbound NAT pools are referenced from virtual machine scale sets. NICs that are associated with individual virtual machines cannot reference an inbound NAT pool. They have to reference individual inbound NAT rules.
 		InboundNatPools *[]InboundNatPool `json:"inboundNatPools,omitempty"`
 		// OutboundRules - The outbound rules.
 		OutboundRules *[]OutboundRule `json:"outboundRules,omitempty"`

api/v1alpha3/types.go

@@ -262,7 +262,7 @@ type LoadBalancer struct {
 		Probes *[]Probe `json:"probes,omitempty"`
 		// InboundNatRules - Collection of inbound NAT Rules used by a load balancer. Defining inbound NAT rules on your load balancer is mutually exclusive with defining an inbound NAT pool. Inbound NAT pools are referenced from virtual machine scale sets. NICs that are associated with individual virtual machines cannot reference an Inbound NAT pool. They have to reference individual inbound NAT rules.
 		InboundNatRules *[]InboundNatRule `json:"inboundNatRules,omitempty"`
-		// InboundNatPools - Defines an external port range for inbound NAT to a single backend port on NICs associated with a load balancer. Inbound NAT rules are created automatically for each NIC associated with the Load Balancer using an external port from this range. Defining an Inbound NAT pool on your Load Balancer is mutually exclusive with defining inbound Nat rules. Inbound NAT pools are referenced from virtual machine scale sets. NICs that are associated with individual virtual machines cannot reference an inbound NAT pool. They have to reference individual inbound NAT rules.
+		// InboundNatPools - Defines an external port range for inbound NAT to a single backend port on NICs associated with a load balancer. Inbound NAT rules are created automatically for each NIC associated with the Load Balancer using an external port from this range. Defining an Inbound NAT pool on your Load Balancer is mutually exclusive with defining inbound NAT rules. Inbound NAT pools are referenced from virtual machine scale sets. NICs that are associated with individual virtual machines cannot reference an inbound NAT pool. They have to reference individual inbound NAT rules.
 		InboundNatPools *[]InboundNatPool `json:"inboundNatPools,omitempty"`
 		// OutboundRules - The outbound rules.
 		OutboundRules *[]OutboundRule `json:"outboundRules,omitempty"`

cloud/services/inboundnatrules/client.go

@@ -0,0 +1,86 @@
+/*
+Copyright 2019 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 inboundnatrules
+
+import (
+	"context"
+
+	"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-06-01/network"
+	"github.com/Azure/go-autorest/autorest"
+	azure "sigs.k8s.io/cluster-api-provider-azure/cloud"
+)
+
+// Client wraps go-sdk
+type Client interface {
+	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
+}
+
+// AzureClient contains the Azure go-sdk Client
+type AzureClient struct {
+	inboundnatrules network.InboundNatRulesClient
+}
+
+var _ Client = &AzureClient{}
+
+// NewClient creates a new inbound NAT rules client from subscription ID.
+func NewClient(subscriptionID string, authorizer autorest.Authorizer) *AzureClient {
+	c := newInboundNatRulesClient(subscriptionID, authorizer)
+	return &AzureClient{c}
+}
+
+// newLoadbalancersClient creates a new inbound NAT rules client from subscription ID.
+func newInboundNatRulesClient(subscriptionID string, authorizer autorest.Authorizer) network.InboundNatRulesClient {
+	inboundNatRulesClient := network.NewInboundNatRulesClient(subscriptionID)
+	inboundNatRulesClient.Authorizer = authorizer
+	inboundNatRulesClient.AddToUserAgent(azure.UserAgent)
+	return inboundNatRulesClient
+}
+
+// Get gets the specified inbound NAT rules.
+func (ac *AzureClient) Get(ctx context.Context, resourceGroupName, lbName, inboundNatRuleName string) (network.InboundNatRule, error) {
+	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 {
+	future, err := ac.inboundnatrules.CreateOrUpdate(ctx, resourceGroupName, lbName, inboundNatRuleName, inboundNatRuleParameters)
+	if err != nil {
+		return err
+	}
+	err = future.WaitForCompletionRef(ctx, ac.inboundnatrules.Client)
+	if err != nil {
+		return err
+	}
+	_, err = future.Result(ac.inboundnatrules)
+	return err
+}
+
+// Delete deletes the specified inbound NAT rules.
+func (ac *AzureClient) Delete(ctx context.Context, resourceGroupName, lbName, inboundNatRuleName string) error {
+	future, err := ac.inboundnatrules.Delete(ctx, resourceGroupName, lbName, inboundNatRuleName)
+	if err != nil {
+		return err
+	}
+	err = future.WaitForCompletionRef(ctx, ac.inboundnatrules.Client)
+	if err != nil {
+		return err
+	}
+	_, err = future.Result(ac.inboundnatrules)
+	return err
+}

cloud/services/inboundnatrules/mock_inboundnatrules/doc.go

@@ -0,0 +1,20 @@
+/*
+Copyright 2019 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.
+*/
+
+// Run go generate to regenerate this mock.
+//go:generate ../../../../hack/tools/bin/mockgen -destination inboundnatrules_mock.go -package mock_inboundnatrules -source ../client.go Client
+//go:generate /usr/bin/env bash -c "cat ../../../../hack/boilerplate/boilerplate.generatego.txt inboundnatrules_mock.go > _inboundnatrules_mock.go && mv _inboundnatrules_mock.go inboundnatrules_mock.go"
+package mock_inboundnatrules //nolint

cloud/services/inboundnatrules/mock_inboundnatrules/inboundnatrules_mock.go

@@ -38,7 +38,7 @@ type AzureClient struct {
 
 var _ Client = &AzureClient{}
 
-// NewClient creates a new VM client from subscription ID.
+// NewClient creates a new load balancer client from subscription ID.
 func NewClient(subscriptionID string, authorizer autorest.Authorizer) *AzureClient {
 	c := newLoadBalancersClient(subscriptionID, authorizer)
 	return &AzureClient{c}

cloud/services/internalloadbalancers/client.go

@@ -18,6 +18,7 @@ package networkinterfaces
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-06-01/network"
 	"github.com/Azure/go-autorest/autorest/to"
@@ -35,7 +36,6 @@ type Spec struct {
 	PublicLoadBalancerName   string
 	InternalLoadBalancerName string
 	PublicIPName             string
-	NatRule                  int
 }
 
 // Get provides information about a network interface.
@@ -63,7 +63,7 @@ func (s *Service) Reconcile(ctx context.Context, spec interface{}) error {
 
 	subnet, err := s.SubnetsClient.Get(ctx, s.Scope.Vnet().ResourceGroup, nicSpec.VnetName, nicSpec.SubnetName)
 	if err != nil {
-		return errors.Wrap(err, "failed to get subNets")
+		return errors.Wrap(err, "failed to get subnets")
 	}
 
 	nicConfig.Subnet = &network.Subnet{ID: subnet.ID}
@@ -75,7 +75,8 @@ func (s *Service) Reconcile(ctx context.Context, spec interface{}) error {
 
 	backendAddressPools := []network.BackendAddressPool{}
 	if nicSpec.PublicLoadBalancerName != "" {
-		lb, lberr := s.LoadBalancersClient.Get(ctx, s.Scope.ResourceGroup(), nicSpec.PublicLoadBalancerName)
+		// only control planes have an attached public LB
+		lb, lberr := s.PublicLoadBalancersClient.Get(ctx, s.Scope.ResourceGroup(), nicSpec.PublicLoadBalancerName)
 		if lberr != nil {
 			return errors.Wrap(lberr, "failed to get publicLB")
 		}
@@ -85,14 +86,21 @@ func (s *Service) Reconcile(ctx context.Context, spec interface{}) error {
 				ID: (*lb.BackendAddressPools)[0].ID,
 			})
 
+		ruleName := s.MachineScope.Name()
+		naterr := s.createInboundNatRule(ctx, lb, ruleName)
+		if naterr != nil {
+			return errors.Wrap(naterr, "failed to create NAT rule")
+		}
+
 		nicConfig.LoadBalancerInboundNatRules = &[]network.InboundNatRule{
 			{
-				ID: (*lb.InboundNatRules)[nicSpec.NatRule].ID,
+				ID: to.StringPtr(fmt.Sprintf("%s/inboundNatRules/%s", to.String(lb.ID), ruleName)),
 			},
 		}
 	}
 	if nicSpec.InternalLoadBalancerName != "" {
-		internalLB, ilberr := s.LoadBalancersClient.Get(ctx, s.Scope.ResourceGroup(), nicSpec.InternalLoadBalancerName)
+		// only control planes have an attached internal LB
+		internalLB, ilberr := s.InternalLoadBalancersClient.Get(ctx, s.Scope.ResourceGroup(), nicSpec.InternalLoadBalancerName)
 		if ilberr != nil {
 			return errors.Wrap(ilberr, "failed to get internalLB")
 		}
@@ -143,14 +151,59 @@ func (s *Service) Delete(ctx context.Context, spec interface{}) error {
 	}
 	klog.V(2).Infof("deleting nic %s", nicSpec.Name)
 	err := s.Client.Delete(ctx, s.Scope.ResourceGroup(), nicSpec.Name)
-	if err != nil && azure.ResourceNotFound(err) {
-		// already deleted
-		return nil
-	}
-	if err != nil {
+	if err != nil && !azure.ResourceNotFound(err) {
 		return errors.Wrapf(err, "failed to delete network interface %s in resource group %s", nicSpec.Name, s.Scope.ResourceGroup())
 	}
-
-	klog.V(2).Infof("successfully deleted nic %s", nicSpec.Name)
+	NATRuleName := s.MachineScope.Name()
+	err = s.InboundNATRulesClient.Delete(ctx, s.Scope.ResourceGroup(), nicSpec.PublicLoadBalancerName, NATRuleName)
+	if err != nil && !azure.ResourceNotFound(err) {
+		return errors.Wrapf(err, "failed to delete inbound NAT rule %s in load balancer %s", NATRuleName, nicSpec.PublicLoadBalancerName)
+	}
+	klog.V(2).Infof("successfully deleted nic %s and NAT rule %s", nicSpec.Name, NATRuleName)
 	return nil
 }
+
+func (s *Service) createInboundNatRule(ctx context.Context, lb network.LoadBalancer, ruleName string) error {
+	var sshFrontendPort int32 = 22
+	ports := make(map[int32]struct{})
+	if lb.LoadBalancerPropertiesFormat == nil || lb.InboundNatRules == nil {
+		return errors.Errorf("Could not get existing inbound NAT rules from load balancer %s properties", to.String(lb.Name))
+	}
+	for _, v := range *lb.InboundNatRules {
+		if to.String(v.Name) == ruleName {
+			// Inbound NAT Rule already exists, nothing to do here.
+			klog.Infof("NAT rule %s already exists", ruleName)
+			return nil
+		}
+		ports[*v.InboundNatRulePropertiesFormat.FrontendPort] = struct{}{}
+	}
+	if _, ok := ports[22]; ok {
+		var i int32
+		found := false
+		for i = 2201; i < 2220; i++ {
+			if _, ok := ports[i]; !ok {
+				sshFrontendPort = i
+				found = true
+				break
+			}
+		}
+		if !found {
+			return errors.Errorf("Failed to find available SSH Frontend port for NAT Rule in load balancer %s for AzureMachine %s", to.String(lb.Name), ruleName)
+		}
+	}
+	rule := network.InboundNatRule{
+		Name: to.StringPtr(ruleName),
+		InboundNatRulePropertiesFormat: &network.InboundNatRulePropertiesFormat{
+			BackendPort:          to.Int32Ptr(22),
+			EnableFloatingIP:     to.BoolPtr(false),
+			IdleTimeoutInMinutes: to.Int32Ptr(4),
+			FrontendIPConfiguration: &network.SubResource{
+				ID: (*lb.FrontendIPConfigurations)[0].ID,
+			},
+			Protocol:     network.TransportProtocolTCP,
+			FrontendPort: &sshFrontendPort,
+		},
+	}
+	klog.V(3).Infof("Creating rule %s using port %d", ruleName, sshFrontendPort)
+	return s.InboundNATRulesClient.CreateOrUpdate(ctx, s.Scope.ResourceGroup(), to.String(lb.Name), ruleName, rule)
+}