diff --git a/api/v1beta1/azurecluster_default.go b/api/v1beta1/azurecluster_default.go index f4374e3329d..6d4f8211a43 100644 --- a/api/v1beta1/azurecluster_default.go +++ b/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) } diff --git a/api/v1beta1/azurecluster_default_test.go b/api/v1beta1/azurecluster_default_test.go index 4e5c03749e1..6a30b28abc9 100644 --- a/api/v1beta1/azurecluster_default_test.go +++ b/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", diff --git a/api/v1beta1/types.go b/api/v1beta1/types.go index fe04ad2c466..eb952d06436 100644 --- a/api/v1beta1/types.go +++ b/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 != "" } diff --git a/azure/defaults.go b/azure/defaults.go index 7a019fd6102..94fe37491a7 100644 --- a/azure/defaults.go +++ b/azure/defaults.go @@ -107,7 +107,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) } @@ -220,7 +220,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) } diff --git a/azure/scope/cluster.go b/azure/scope/cluster.go index 637d7150397..90fff59c7eb 100644 --- a/azure/scope/cluster.go +++ b/azure/scope/cluster.go @@ -36,6 +36,7 @@ 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/services/groups" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/natgateways" "sigs.k8s.io/cluster-api-provider-azure/azure/services/vnetpeerings" "sigs.k8s.io/cluster-api-provider-azure/util/futures" "sigs.k8s.io/cluster-api-provider-azure/util/tele" @@ -138,7 +139,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() { @@ -220,20 +221,26 @@ func (s *ClusterScope) RouteTableSpecs() []azure.RouteTableSpec { return routetables } -// NatGatewaySpecs returns the node nat gateway. -func (s *ClusterScope) NatGatewaySpecs() []azure.NatGatewaySpec { - var natGateways []azure.NatGatewaySpec +// NatGatewaySpecs returns the node NAT gateway. +func (s *ClusterScope) NatGatewaySpecs() []azure.ResourceSpecGetter { + natGatewaySet := make(map[string]struct{}) + var 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. for _, subnet := range s.NodeSubnets() { if subnet.IsNatGatewayEnabled() { - natGateways = append(natGateways, azure.NatGatewaySpec{ - Name: subnet.NatGateway.Name, - NatGatewayIP: infrav1.PublicIPSpec{ - Name: subnet.NatGateway.NatGatewayIP.Name, - }, - Subnet: subnet, - }) + if _, ok := natGatewaySet[subnet.NatGateway.Name]; !ok { + natGatewaySet[subnet.NatGateway.Name] = struct{}{} // empty struct to represent hash set + natGateways = append(natGateways, &natgateways.NatGatewaySpec{ + Name: subnet.NatGateway.Name, + ResourceGroup: s.ResourceGroup(), + SubscriptionID: s.SubscriptionID(), + Location: s.Location(), + NatGatewayIP: infrav1.PublicIPSpec{ + Name: subnet.NatGateway.NatGatewayIP.Name, + }, + }) + } } } @@ -446,6 +453,15 @@ func (s *ClusterScope) SetSubnet(subnetSpec infrav1.SubnetSpec) { } } +func (s *ClusterScope) SetNatGatewayIDInSubnets(name string, id string) { + for _, subnet := range s.Subnets() { + if subnet.NatGateway.Name == name { + subnet.NatGateway.ID = id + s.SetSubnet(subnet) + } + } +} + // ControlPlaneRouteTable returns the cluster controlplane routetable. func (s *ClusterScope) ControlPlaneRouteTable() infrav1.RouteTable { subnet, _ := s.AzureCluster.Spec.NetworkSpec.GetControlPlaneSubnet() @@ -597,6 +613,7 @@ func (s *ClusterScope) PatchObject(ctx context.Context) error { infrav1.NetworkInfrastructureReadyCondition, infrav1.VnetPeeringReadyCondition, infrav1.DisksReadyCondition, + infrav1.NATGatewaysReadyCondition, ), ) @@ -609,6 +626,7 @@ func (s *ClusterScope) PatchObject(ctx context.Context) error { infrav1.NetworkInfrastructureReadyCondition, infrav1.VnetPeeringReadyCondition, infrav1.DisksReadyCondition, + infrav1.NATGatewaysReadyCondition, }}) } diff --git a/azure/scope/machine.go b/azure/scope/machine.go index 230e2ec5a8b..d253544a1a5 100644 --- a/azure/scope/machine.go +++ b/azure/scope/machine.go @@ -231,7 +231,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())) diff --git a/azure/scope/machine_test.go b/azure/scope/machine_test.go index 2a137a1410e..d2f4c232eb1 100644 --- a/azure/scope/machine_test.go +++ b/azure/scope/machine_test.go @@ -1306,7 +1306,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{ @@ -1385,7 +1385,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{ diff --git a/azure/scope/managedcontrolplane.go b/azure/scope/managedcontrolplane.go index 8651b94c9ac..ce69dd2cb8b 100644 --- a/azure/scope/managedcontrolplane.go +++ b/azure/scope/managedcontrolplane.go @@ -230,7 +230,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{} } diff --git a/azure/services/disks/client.go b/azure/services/disks/client.go index 85ae1c0fbd1..26623eb96f3 100644 --- a/azure/services/disks/client.go +++ b/azure/services/disks/client.go @@ -51,7 +51,7 @@ func NewDisksClient(subscriptionID string, baseURI string, authorizer autorest.A // 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, "disks.Service.DeleteAsync") + ctx, _, done := tele.StartSpanWithLogger(ctx, "disks.azureClient.DeleteAsync") defer done() future, err := ac.disks.Delete(ctx, spec.ResourceGroupName(), spec.ResourceName()) @@ -80,7 +80,7 @@ func (ac *azureClient) Result(ctx context.Context, futureData azureautorest.Futu // 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, "natgateways.Service.IsDone") + ctx, _, done := tele.StartSpanWithLogger(ctx, "disks.azureClient.IsDone") defer done() isDone, err := future.DoneWithContext(ctx, ac.disks) diff --git a/azure/services/natgateways/client.go b/azure/services/natgateways/client.go index db832862167..1490bba326d 100644 --- a/azure/services/natgateways/client.go +++ b/azure/services/natgateways/client.go @@ -18,28 +18,24 @@ package natgateways import ( "context" + "encoding/json" - "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-06-01/network" + "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 { - Get(context.Context, string, string) (network.NatGateway, error) - CreateOrUpdate(context.Context, string, string, network.NatGateway) error - Delete(context.Context, string, string) error -} - // azureClient contains the Azure go-sdk Client. type azureClient struct { natgateways network.NatGatewaysClient } -var _ client = (*azureClient)(nil) - // newClient creates a new VM client from subscription ID. func newClient(auth azure.Authorizer) *azureClient { c := netNatGatewaysClient(auth.SubscriptionID(), auth.BaseURI(), auth.Authorizer()) @@ -54,43 +50,113 @@ func netNatGatewaysClient(subscriptionID string, baseURI string, authorizer auto } // 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") +func (ac *azureClient) Get(ctx context.Context, spec azure.ResourceSpecGetter) (interface{}, error) { + ctx, _, done := tele.StartSpanWithLogger(ctx, "natgateways.azureClient.Get") defer done() - return ac.natgateways.Get(ctx, resourceGroupName, natGatewayName, "") + return ac.natgateways.Get(ctx, spec.ResourceGroupName(), spec.ResourceName(), "") } -// CreateOrUpdate create or updates a nat gateway in a specified resource group. -func (ac *azureClient) CreateOrUpdate(ctx context.Context, resourceGroupName string, natGatewayName string, natGateway network.NatGateway) error { - ctx, _, done := tele.StartSpanWithLogger(ctx, "natgateways.AzureClient.CreateOrUpdate") +// 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, parameters interface{}) (interface{}, azureautorest.FutureAPI, error) { + ctx, _, done := tele.StartSpanWithLogger(ctx, "natgateways.azureClient.CreateOrUpdateAsync") defer done() - future, err := ac.natgateways.CreateOrUpdate(ctx, resourceGroupName, natGatewayName, natGateway) + natGateway, ok := parameters.(network.NatGateway) + if !ok { + return nil, nil, errors.Errorf("%T is not a network.NatGateway", parameters) + } + + future, err := ac.natgateways.CreateOrUpdate(ctx, spec.ResourceGroupName(), spec.ResourceName(), natGateway) if err != nil { - return err + return nil, nil, err } + + ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureCallTimeout) + defer cancel() + err = future.WaitForCompletionRef(ctx, ac.natgateways.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.natgateways) - return err + + result, err := future.Result(ac.natgateways) + // if the operation completed, return a nil future + return result, nil, err } -// Delete deletes the specified nat gateway. -func (ac *azureClient) Delete(ctx context.Context, resourceGroupName, natGatewayName string) error { - ctx, _, done := tele.StartSpanWithLogger(ctx, "natgateways.AzureClient.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, _, done := tele.StartSpanWithLogger(ctx, "natgateways.azureClient.DeleteAsync") defer done() - future, err := ac.natgateways.Delete(ctx, resourceGroupName, natGatewayName) + future, err := ac.natgateways.Delete(ctx, spec.ResourceGroupName(), spec.ResourceName()) if err != nil { - return err + return nil, err } + + ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureCallTimeout) + defer cancel() + err = future.WaitForCompletionRef(ctx, ac.natgateways.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.natgateways) - 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, "natgateways.azureClient.IsDone") + defer done() + + isDone, err := future.DoneWithContext(ctx, ac.natgateways) + 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, "natgateways.azureClient.Result") + defer done() + + if futureData == nil { + return nil, errors.Errorf("cannot get result from nil future") + } + var result func(client network.NatGatewaysClient) (natGateway network.NatGateway, err error) + + switch futureType { + case infrav1.PutFuture: + var future *network.NatGatewaysCreateOrUpdateFuture + 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 NAT gateway + return nil, nil + + default: + return nil, errors.Errorf("unknown future type %q", futureType) + } + + return result(ac.natgateways) } diff --git a/azure/services/natgateways/mock_natgateways/client_mock.go b/azure/services/natgateways/mock_natgateways/client_mock.go deleted file mode 100644 index 0fff8a51f20..00000000000 --- a/azure/services/natgateways/mock_natgateways/client_mock.go +++ /dev/null @@ -1,95 +0,0 @@ -/* -Copyright 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. -*/ - -// Code generated by MockGen. DO NOT EDIT. -// Source: ../client.go - -// Package mock_natgateways is a generated GoMock package. -package mock_natgateways - -import ( - context "context" - reflect "reflect" - - network "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-06-01/network" - gomock "github.com/golang/mock/gomock" -) - -// Mockclient is a mock of client interface. -type Mockclient struct { - ctrl *gomock.Controller - recorder *MockclientMockRecorder -} - -// MockclientMockRecorder is the mock recorder for Mockclient. -type MockclientMockRecorder struct { - mock *Mockclient -} - -// NewMockclient creates a new mock instance. -func NewMockclient(ctrl *gomock.Controller) *Mockclient { - mock := &Mockclient{ctrl: ctrl} - mock.recorder = &MockclientMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *Mockclient) EXPECT() *MockclientMockRecorder { - return m.recorder -} - -// CreateOrUpdate mocks base method. -func (m *Mockclient) CreateOrUpdate(arg0 context.Context, arg1, arg2 string, arg3 network.NatGateway) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateOrUpdate", arg0, arg1, arg2, arg3) - ret0, _ := ret[0].(error) - return ret0 -} - -// CreateOrUpdate indicates an expected call of CreateOrUpdate. -func (mr *MockclientMockRecorder) CreateOrUpdate(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdate", reflect.TypeOf((*Mockclient)(nil).CreateOrUpdate), arg0, arg1, arg2, arg3) -} - -// Delete mocks base method. -func (m *Mockclient) Delete(arg0 context.Context, arg1, arg2 string) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Delete", arg0, arg1, arg2) - ret0, _ := ret[0].(error) - return ret0 -} - -// Delete indicates an expected call of Delete. -func (mr *MockclientMockRecorder) Delete(arg0, arg1, arg2 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delete", reflect.TypeOf((*Mockclient)(nil).Delete), arg0, arg1, arg2) -} - -// Get mocks base method. -func (m *Mockclient) Get(arg0 context.Context, arg1, arg2 string) (network.NatGateway, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Get", arg0, arg1, arg2) - ret0, _ := ret[0].(network.NatGateway) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// Get indicates an expected call of Get. -func (mr *MockclientMockRecorder) Get(arg0, arg1, arg2 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*Mockclient)(nil).Get), arg0, arg1, arg2) -} diff --git a/azure/services/natgateways/mock_natgateways/doc.go b/azure/services/natgateways/mock_natgateways/doc.go index 6c25bb36c5e..b58fcf8699e 100644 --- a/azure/services/natgateways/mock_natgateways/doc.go +++ b/azure/services/natgateways/mock_natgateways/doc.go @@ -15,8 +15,6 @@ limitations under the License. */ // Run go generate to regenerate this mock. -//go:generate ../../../../hack/tools/bin/mockgen -destination client_mock.go -package mock_natgateways -source ../client.go Client //go:generate ../../../../hack/tools/bin/mockgen -destination natgateways_mock.go -package mock_natgateways -source ../natgateways.go NatGatewayScope -//go:generate /usr/bin/env bash -c "cat ../../../../hack/boilerplate/boilerplate.generatego.txt client_mock.go > _client_mock.go && mv _client_mock.go client_mock.go" //go:generate /usr/bin/env bash -c "cat ../../../../hack/boilerplate/boilerplate.generatego.txt natgateways_mock.go > _natgateways_mock.go && mv _natgateways_mock.go natgateways_mock.go" package mock_natgateways //nolint diff --git a/azure/services/natgateways/mock_natgateways/natgateways_mock.go b/azure/services/natgateways/mock_natgateways/natgateways_mock.go index c7f1878ab87..53c415e5530 100644 --- a/azure/services/natgateways/mock_natgateways/natgateways_mock.go +++ b/azure/services/natgateways/mock_natgateways/natgateways_mock.go @@ -27,6 +27,7 @@ import ( gomock "github.com/golang/mock/gomock" v1beta1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" azure "sigs.k8s.io/cluster-api-provider-azure/azure" + v1beta10 "sigs.k8s.io/cluster-api/api/v1beta1" ) // MockNatGatewayScope is a mock of NatGatewayScope interface. @@ -234,6 +235,18 @@ func (mr *MockNatGatewayScopeMockRecorder) ControlPlaneSubnet() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ControlPlaneSubnet", reflect.TypeOf((*MockNatGatewayScope)(nil).ControlPlaneSubnet)) } +// DeleteLongRunningOperationState mocks base method. +func (m *MockNatGatewayScope) DeleteLongRunningOperationState(arg0, arg1 string) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "DeleteLongRunningOperationState", arg0, arg1) +} + +// DeleteLongRunningOperationState indicates an expected call of DeleteLongRunningOperationState. +func (mr *MockNatGatewayScopeMockRecorder) DeleteLongRunningOperationState(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteLongRunningOperationState", reflect.TypeOf((*MockNatGatewayScope)(nil).DeleteLongRunningOperationState), arg0, arg1) +} + // FailureDomains mocks base method. func (m *MockNatGatewayScope) FailureDomains() []string { m.ctrl.T.Helper() @@ -248,6 +261,20 @@ func (mr *MockNatGatewayScopeMockRecorder) FailureDomains() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FailureDomains", reflect.TypeOf((*MockNatGatewayScope)(nil).FailureDomains)) } +// GetLongRunningOperationState mocks base method. +func (m *MockNatGatewayScope) GetLongRunningOperationState(arg0, arg1 string) *v1beta1.Future { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetLongRunningOperationState", arg0, arg1) + ret0, _ := ret[0].(*v1beta1.Future) + return ret0 +} + +// GetLongRunningOperationState indicates an expected call of GetLongRunningOperationState. +func (mr *MockNatGatewayScopeMockRecorder) GetLongRunningOperationState(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetLongRunningOperationState", reflect.TypeOf((*MockNatGatewayScope)(nil).GetLongRunningOperationState), arg0, arg1) +} + // GetPrivateDNSZoneName mocks base method. func (m *MockNatGatewayScope) GetPrivateDNSZoneName() string { m.ctrl.T.Helper() @@ -333,10 +360,10 @@ func (mr *MockNatGatewayScopeMockRecorder) Location() *gomock.Call { } // NatGatewaySpecs mocks base method. -func (m *MockNatGatewayScope) NatGatewaySpecs() []azure.NatGatewaySpec { +func (m *MockNatGatewayScope) NatGatewaySpecs() []azure.ResourceSpecGetter { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "NatGatewaySpecs") - ret0, _ := ret[0].([]azure.NatGatewaySpec) + ret0, _ := ret[0].([]azure.ResourceSpecGetter) return ret0 } @@ -402,6 +429,30 @@ func (mr *MockNatGatewayScopeMockRecorder) ResourceGroup() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResourceGroup", reflect.TypeOf((*MockNatGatewayScope)(nil).ResourceGroup)) } +// SetLongRunningOperationState mocks base method. +func (m *MockNatGatewayScope) SetLongRunningOperationState(arg0 *v1beta1.Future) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "SetLongRunningOperationState", arg0) +} + +// SetLongRunningOperationState indicates an expected call of SetLongRunningOperationState. +func (mr *MockNatGatewayScopeMockRecorder) SetLongRunningOperationState(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetLongRunningOperationState", reflect.TypeOf((*MockNatGatewayScope)(nil).SetLongRunningOperationState), arg0) +} + +// SetNatGatewayIDInSubnets mocks base method. +func (m *MockNatGatewayScope) SetNatGatewayIDInSubnets(natGatewayName, natGatewayID string) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "SetNatGatewayIDInSubnets", natGatewayName, natGatewayID) +} + +// SetNatGatewayIDInSubnets indicates an expected call of SetNatGatewayIDInSubnets. +func (mr *MockNatGatewayScopeMockRecorder) SetNatGatewayIDInSubnets(natGatewayName, natGatewayID interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetNatGatewayIDInSubnets", reflect.TypeOf((*MockNatGatewayScope)(nil).SetNatGatewayIDInSubnets), natGatewayName, natGatewayID) +} + // SetSubnet mocks base method. func (m *MockNatGatewayScope) SetSubnet(arg0 v1beta1.SubnetSpec) { m.ctrl.T.Helper() @@ -470,6 +521,42 @@ func (mr *MockNatGatewayScopeMockRecorder) TenantID() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "TenantID", reflect.TypeOf((*MockNatGatewayScope)(nil).TenantID)) } +// UpdateDeleteStatus mocks base method. +func (m *MockNatGatewayScope) UpdateDeleteStatus(arg0 v1beta10.ConditionType, arg1 string, arg2 error) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "UpdateDeleteStatus", arg0, arg1, arg2) +} + +// UpdateDeleteStatus indicates an expected call of UpdateDeleteStatus. +func (mr *MockNatGatewayScopeMockRecorder) UpdateDeleteStatus(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateDeleteStatus", reflect.TypeOf((*MockNatGatewayScope)(nil).UpdateDeleteStatus), arg0, arg1, arg2) +} + +// UpdatePatchStatus mocks base method. +func (m *MockNatGatewayScope) UpdatePatchStatus(arg0 v1beta10.ConditionType, arg1 string, arg2 error) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "UpdatePatchStatus", arg0, arg1, arg2) +} + +// UpdatePatchStatus indicates an expected call of UpdatePatchStatus. +func (mr *MockNatGatewayScopeMockRecorder) UpdatePatchStatus(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdatePatchStatus", reflect.TypeOf((*MockNatGatewayScope)(nil).UpdatePatchStatus), arg0, arg1, arg2) +} + +// UpdatePutStatus mocks base method. +func (m *MockNatGatewayScope) UpdatePutStatus(arg0 v1beta10.ConditionType, arg1 string, arg2 error) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "UpdatePutStatus", arg0, arg1, arg2) +} + +// UpdatePutStatus indicates an expected call of UpdatePutStatus. +func (mr *MockNatGatewayScopeMockRecorder) UpdatePutStatus(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdatePutStatus", reflect.TypeOf((*MockNatGatewayScope)(nil).UpdatePutStatus), arg0, arg1, arg2) +} + // Vnet mocks base method. func (m *MockNatGatewayScope) Vnet() *v1beta1.VnetSpec { m.ctrl.T.Helper() diff --git a/azure/services/natgateways/natgateways.go b/azure/services/natgateways/natgateways.go index 8e18950a364..7e2b1b30a64 100644 --- a/azure/services/natgateways/natgateways.go +++ b/azure/services/natgateways/natgateways.go @@ -18,156 +18,112 @@ package natgateways import ( "context" - "fmt" - "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-06-01/network" - autorest "github.com/Azure/go-autorest/autorest/azure" - "github.com/Azure/go-autorest/autorest/to" + "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network" "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/azure/services/async" + "sigs.k8s.io/cluster-api-provider-azure/util/reconciler" "sigs.k8s.io/cluster-api-provider-azure/util/tele" ) -// NatGatewayScope defines the scope interface for nat gateway service. +const serviceName = "natgateways" + +// NatGatewayScope defines the scope interface for NAT gateway service. type NatGatewayScope interface { azure.ClusterScoper - NatGatewaySpecs() []azure.NatGatewaySpec + azure.AsyncStatusUpdater + SetNatGatewayIDInSubnets(natGatewayName string, natGatewayID string) + NatGatewaySpecs() []azure.ResourceSpecGetter } // Service provides operations on azure resources. type Service struct { Scope NatGatewayScope - client + async.Reconciler } // New creates a new service. func New(scope NatGatewayScope) *Service { + client := newClient(scope) return &Service{ - Scope: scope, - client: newClient(scope), + Scope: scope, + Reconciler: async.New(scope, client, client), } } -// 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, log, done := tele.StartSpanWithLogger(ctx, "natgateways.Service.Reconcile") defer done() + ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout) + defer cancel() + if !s.Scope.Vnet().IsManaged(s.Scope.ClusterName()) { log.V(4).Info("Skipping nat gateways reconcile in custom vnet mode") + + s.Scope.UpdatePutStatus(infrav1.NATGatewaysReadyCondition, serviceName, nil) return nil } + // 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 resultingErr error for _, natGatewaySpec := range s.Scope.NatGatewaySpecs() { - existingNatGateway, err := s.getExisting(ctx, natGatewaySpec) - - switch { - case err != nil && !azure.ResourceNotFound(err): - return errors.Wrapf(err, "failed to get nat gateway %s in %s", natGatewaySpec.Name, s.Scope.ResourceGroup()) - case err == nil: - // nat gateway already exists - log.V(4).Info("nat gateway already exists", "nat gateway", natGatewaySpec.Name) - natGatewaySpec.Subnet.NatGateway.ID = existingNatGateway.ID - - if existingNatGateway.NatGatewayIP.Name == natGatewaySpec.NatGatewayIP.Name { - // Skip update for Nat Gateway as it exists with expected values - log.V(4).Info("Nat Gateway exists with expected values, skipping update", "nat gateway", natGatewaySpec.Name) - natGatewaySpec.Subnet.NatGateway = *existingNatGateway - s.Scope.SetSubnet(natGatewaySpec.Subnet) - continue - } else { - log.V(2).Info("updating NAT gateway IP name to match the spec", "old name", existingNatGateway.NatGatewayIP.Name, "desired name", natGatewaySpec.NatGatewayIP.Name) + result, err := s.CreateResource(ctx, natGatewaySpec, serviceName) + if err != nil { + if !azure.IsOperationNotDoneError(err) || resultingErr == nil { + resultingErr = err } - default: - // nat gateway doesn't exist but its name was specified in the subnet, let's create it - log.V(2).Info("nat gateway doesn't exist yet, creating it", "nat gateway", natGatewaySpec.Name) } + if err == nil { + natGateway, ok := result.(network.NatGateway) + if !ok { + // Return out of loop since this would be an unexepcted fatal error + resultingErr = errors.Errorf("created resource %T is not a network.NatGateway", result) + break + } - natGatewayToCreate := network.NatGateway{ - Location: to.StringPtr(s.Scope.Location()), - Sku: &network.NatGatewaySku{Name: network.Standard}, - NatGatewayPropertiesFormat: &network.NatGatewayPropertiesFormat{ - PublicIPAddresses: &[]network.SubResource{ - { - ID: to.StringPtr(azure.PublicIPID(s.Scope.SubscriptionID(), s.Scope.ResourceGroup(), natGatewaySpec.NatGatewayIP.Name)), - }, - }, - }, - } - err = s.client.CreateOrUpdate(ctx, s.Scope.ResourceGroup(), natGatewaySpec.Name, natGatewayToCreate) - if err != nil { - return errors.Wrapf(err, "failed to create nat gateway %s in resource group %s", natGatewaySpec.Name, s.Scope.ResourceGroup()) - } - log.V(2).Info("successfully created nat gateway", "nat gateway", natGatewaySpec.Name) - natGateway := infrav1.NatGateway{ - ID: azure.NatGatewayID(s.Scope.SubscriptionID(), s.Scope.ResourceGroup(), natGatewaySpec.Name), - Name: natGatewaySpec.Name, - NatGatewayIP: infrav1.PublicIPSpec{ - Name: natGatewaySpec.NatGatewayIP.Name, - }, + // TODO: ideally we wouldn't need to set the subnet spec based on the result of the create operation + s.Scope.SetNatGatewayIDInSubnets(natGatewaySpec.ResourceName(), *natGateway.ID) } - natGatewaySpec.Subnet.NatGateway = natGateway - s.Scope.SetSubnet(natGatewaySpec.Subnet) - } - return nil -} - -func (s *Service) getExisting(ctx context.Context, spec azure.NatGatewaySpec) (*infrav1.NatGateway, error) { - ctx, _, done := tele.StartSpanWithLogger(ctx, "natgateways.Service.getExisting") - defer done() - - existingNatGateway, err := s.Get(ctx, s.Scope.ResourceGroup(), spec.Name) - if err != nil { - return nil, err - } - // We must have a non-nil, non-"empty" PublicIPAddresses - if !(existingNatGateway.PublicIPAddresses != nil && len(*existingNatGateway.PublicIPAddresses) > 0) { - return nil, errors.Wrap(err, "failed to parse PublicIPAddresses") - } - // TODO do we want to eventually handle NatGateway resources w/ more than one public IP address? - // For now we assume the first one is the significant one - publicIPAddressID := to.String((*existingNatGateway.PublicIPAddresses)[0].ID) - resource, err := autorest.ParseResourceID(publicIPAddressID) - if err != nil { - return nil, 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 &infrav1.NatGateway{ - ID: to.String(existingNatGateway.ID), - Name: to.String(existingNatGateway.Name), - NatGatewayIP: infrav1.PublicIPSpec{ - Name: resource.ResourceName, - }, - }, nil + 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, log, done := tele.StartSpanWithLogger(ctx, "natgateways.Service.Delete") defer done() + ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout) + defer cancel() + if !s.Scope.Vnet().IsManaged(s.Scope.ClusterName()) { log.V(4).Info("Skipping nat gateway deletion in custom vnet mode") + + s.Scope.UpdateDeleteStatus(infrav1.NATGatewaysReadyCondition, serviceName, nil) return nil } + + var resultingErr error + + // We go through the list of NatGatewaySpecs to delete 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 deleting -> deleting in progress -> deleted (no error) for _, natGatewaySpec := range s.Scope.NatGatewaySpecs() { - log.V(2).Info("deleting nat gateway", "nat gateway", natGatewaySpec.Name) - err := s.client.Delete(ctx, s.Scope.ResourceGroup(), natGatewaySpec.Name) - if err != nil && azure.ResourceNotFound(err) { - // already deleted - continue - } - if err != nil { - return errors.Wrapf(err, "failed to delete nat gateway %s in resource group %s", natGatewaySpec.Name, s.Scope.ResourceGroup()) + if err := s.DeleteResource(ctx, natGatewaySpec, serviceName); err != nil { + if !azure.IsOperationNotDoneError(err) || resultingErr == nil { + resultingErr = err + } } - - log.V(2).Info("successfully deleted nat gateway", "nat gateway", natGatewaySpec.Name) } - return nil + s.Scope.UpdateDeleteStatus(infrav1.NATGatewaysReadyCondition, serviceName, resultingErr) + return resultingErr } diff --git a/azure/services/natgateways/natgateways_test.go b/azure/services/natgateways/natgateways_test.go index 16db094c4ba..7cfc809c060 100644 --- a/azure/services/natgateways/natgateways_test.go +++ b/azure/services/natgateways/natgateways_test.go @@ -21,7 +21,7 @@ import ( "net/http" "testing" - "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-06-01/network" + "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network" "github.com/Azure/go-autorest/autorest" "github.com/Azure/go-autorest/autorest/to" "github.com/golang/mock/gomock" @@ -29,6 +29,7 @@ import ( "k8s.io/client-go/kubernetes/scheme" 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/services/async/mock_async" "sigs.k8s.io/cluster-api-provider-azure/azure/services/natgateways/mock_natgateways" gomockinternal "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock" clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3" @@ -38,227 +39,89 @@ func init() { _ = clusterv1.AddToScheme(scheme.Scheme) } +var ( + customVNetSpec = infrav1.VnetSpec{ + ID: "1234", + Name: "my-vnet", + } + ownedVNetSpec = infrav1.VnetSpec{ + Name: "my-vnet", + } + natGatewaySpec1 = NatGatewaySpec{ + Name: "my-node-natgateway-1", + ResourceGroup: "my-rg", + SubscriptionID: "my-sub", + Location: "westus", + NatGatewayIP: infrav1.PublicIPSpec{Name: "pip-node-subnet"}, + } + natGateway1 = network.NatGateway{ + ID: to.StringPtr("/subscriptions/my-sub/resourceGroups/my-rg/providers/Microsoft.Network/natGateways/my-node-natgateway-1"), + } + customVNetTags = infrav1.Tags{ + "Name": "my-vnet", + "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "shared", + "sigs.k8s.io_cluster-api-provider-azure_role": "common", + } + ownedVNetTags = infrav1.Tags{ + "Name": "my-vnet", + "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", + "sigs.k8s.io_cluster-api-provider-azure_role": "common", + } + internalError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error") +) + func TestReconcileNatGateways(t *testing.T) { testcases := []struct { name string tags infrav1.Tags expectedError string - expect func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) + expect func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ { - name: "nat gateways in custom vnet mode", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "shared", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, - expectedError: "", - expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - ID: "1234", - Name: "my-vnet", - }) - s.ClusterName() - }, - }, - { - name: "nat gateway create successfully", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, - expectedError: "", - expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) - s.ClusterName() - s.NatGatewaySpecs().Return([]azure.NatGatewaySpec{ - { - Name: "my-node-natgateway", - Subnet: infrav1.SubnetSpec{ - Name: "node-subnet", - Role: infrav1.SubnetNode, - }, - NatGatewayIP: infrav1.PublicIPSpec{Name: "pip-node-subnet"}, - }, - }) - - s.SubscriptionID().AnyTimes().Return("123") - s.ResourceGroup().AnyTimes().Return("my-rg") - m.Get(gomockinternal.AContext(), "my-rg", "my-node-natgateway").Return(network.NatGateway{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found")).Times(1) - s.Location().Return("westus") - s.SetSubnet(infrav1.SubnetSpec{ - Role: infrav1.SubnetNode, - Name: "node-subnet", - NatGateway: infrav1.NatGateway{ - ID: "/subscriptions/123/resourceGroups/my-rg/providers/Microsoft.Network/natGateways/my-node-natgateway", - Name: "my-node-natgateway", - NatGatewayIP: infrav1.PublicIPSpec{ - Name: "pip-node-subnet", - }, - }, - }) - m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "my-node-natgateway", gomock.AssignableToTypeOf(network.NatGateway{})).Times(1) - }, - }, - { - name: "update nat gateway if actual state does not match desired state", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "NAT gateways in custom vnet mode", + tags: customVNetTags, expectedError: "", - expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) + expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.Vnet().Return(&customVNetSpec) s.ClusterName() - s.NatGatewaySpecs().Return([]azure.NatGatewaySpec{ - { - Name: "my-node-natgateway", - Subnet: infrav1.SubnetSpec{ - Name: "node-subnet", - Role: infrav1.SubnetNode, - }, - NatGatewayIP: infrav1.PublicIPSpec{ - Name: "different-pip-name", - }, - }, - }) - - s.SubscriptionID().AnyTimes().Return("123") - s.ResourceGroup().Return("my-rg").AnyTimes() - m.Get(gomockinternal.AContext(), "my-rg", "my-node-natgateway").Times(1).Return(network.NatGateway{ - Name: to.StringPtr("my-node-natgateway"), - ID: to.StringPtr("/subscriptions/123/resourceGroups/my-rg/providers/Microsoft.Network/natGateways/my-node-natgateway"), - NatGatewayPropertiesFormat: &network.NatGatewayPropertiesFormat{PublicIPAddresses: &[]network.SubResource{ - {ID: to.StringPtr("/subscriptions/123/resourceGroups/my-rg/providers/Microsoft.Network/publicIPAddresses/pip-my-node-natgateway-node-subnet-natgw")}, - }}, - }, nil) - s.SetSubnet(infrav1.SubnetSpec{ - Role: infrav1.SubnetNode, - Name: "node-subnet", - NatGateway: infrav1.NatGateway{ - ID: "/subscriptions/123/resourceGroups/my-rg/providers/Microsoft.Network/natGateways/my-node-natgateway", - Name: "my-node-natgateway", - NatGatewayIP: infrav1.PublicIPSpec{ - Name: "different-pip-name", - }, - }, - }) - s.Location().Return("westus") - m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "my-node-natgateway", gomock.AssignableToTypeOf(network.NatGateway{})) + s.UpdatePutStatus(infrav1.NATGatewaysReadyCondition, serviceName, nil) }, }, { - name: "nat gateway is not updated if it's up to date", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "NAT gateway create successfully", + tags: ownedVNetTags, expectedError: "", - expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) + expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.Vnet().Return(&ownedVNetSpec) s.ClusterName() - s.NatGatewaySpecs().Return([]azure.NatGatewaySpec{ - { - Name: "my-node-natgateway", - Subnet: infrav1.SubnetSpec{ - Name: "node-subnet", - Role: infrav1.SubnetNode, - }, - NatGatewayIP: infrav1.PublicIPSpec{ - Name: "pip-my-node-natgateway-node-subnet-natgw", - }, - }, - }) - - s.SubscriptionID().AnyTimes().Return("123") - s.ResourceGroup().Return("my-rg").AnyTimes() - m.Get(gomockinternal.AContext(), "my-rg", "my-node-natgateway").Times(1).Return(network.NatGateway{ - Name: to.StringPtr("my-node-natgateway"), - ID: to.StringPtr("/subscriptions/123/resourceGroups/my-rg/providers/Microsoft.Network/natGateways/my-node-natgateway"), - NatGatewayPropertiesFormat: &network.NatGatewayPropertiesFormat{PublicIPAddresses: &[]network.SubResource{ - { - ID: to.StringPtr("/subscriptions/123/resourceGroups/my-rg/providers/Microsoft.Network/publicIPAddresses/pip-my-node-natgateway-node-subnet-natgw"), - }, - }}, - }, nil) - s.SetSubnet(infrav1.SubnetSpec{ - Role: infrav1.SubnetNode, - Name: "node-subnet", - NatGateway: infrav1.NatGateway{ - ID: "/subscriptions/123/resourceGroups/my-rg/providers/Microsoft.Network/natGateways/my-node-natgateway", - Name: "my-node-natgateway", - NatGatewayIP: infrav1.PublicIPSpec{ - Name: "pip-my-node-natgateway-node-subnet-natgw", - }, - }, - }) - s.Location().Return("westus").Times(0) - m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "my-node-natgateway", gomock.AssignableToTypeOf(network.NatGateway{})).Times(0) + s.NatGatewaySpecs().Return([]azure.ResourceSpecGetter{&natGatewaySpec1}) + r.CreateResource(gomockinternal.AContext(), &natGatewaySpec1, serviceName).Return(natGateway1, nil) + s.SetNatGatewayIDInSubnets(natGatewaySpec1.Name, *natGateway1.ID) + s.UpdatePutStatus(infrav1.NATGatewaysReadyCondition, serviceName, nil) }, }, { - name: "fail when getting existing nat gateway", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, - expectedError: "failed to get nat gateway my-node-natgateway in my-rg: #: Internal Server Error: StatusCode=500", - expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) + name: "fail to create a NAT gateway", + tags: ownedVNetTags, + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.Vnet().Return(&ownedVNetSpec) s.ClusterName() - s.NatGatewaySpecs().Return([]azure.NatGatewaySpec{ - { - Name: "my-node-natgateway", - Subnet: infrav1.SubnetSpec{ - Name: "node-subnet", - Role: infrav1.SubnetNode, - }, - }, - }) - s.ResourceGroup().AnyTimes().Return("my-rg") - m.Get(gomockinternal.AContext(), "my-rg", "my-node-natgateway").Return(network.NatGateway{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")) - m.CreateOrUpdate(gomockinternal.AContext(), gomock.Any(), gomock.Any(), gomock.AssignableToTypeOf(network.NatGateway{})).Times(0) + s.NatGatewaySpecs().Return([]azure.ResourceSpecGetter{&natGatewaySpec1}) + r.CreateResource(gomockinternal.AContext(), &natGatewaySpec1, serviceName).Return(nil, internalError) + s.UpdatePutStatus(infrav1.NATGatewaysReadyCondition, serviceName, internalError) }, }, { - name: "fail to create a nat gateway", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, - expectedError: "failed to create nat gateway my-node-natgateway in resource group my-rg: #: Internal Server Error: StatusCode=500", - expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) + name: "result is not a NAT gateway", + tags: ownedVNetTags, + expectedError: "created resource string is not a network.NatGateway", + expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.Vnet().Return(&ownedVNetSpec) s.ClusterName() - s.NatGatewaySpecs().Return([]azure.NatGatewaySpec{ - { - Name: "my-node-natgateway", - Subnet: infrav1.SubnetSpec{ - Name: "node-subnet", - Role: infrav1.SubnetNode, - }, - }, - }) - s.SubscriptionID().AnyTimes().Return("123") - s.ResourceGroup().AnyTimes().Return("my-rg") - m.Get(gomockinternal.AContext(), "my-rg", "my-node-natgateway").Return(network.NatGateway{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found")) - s.Location().Return("westus") - m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "my-node-natgateway", gomock.AssignableToTypeOf(network.NatGateway{})).Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")) + s.NatGatewaySpecs().Return([]azure.ResourceSpecGetter{&natGatewaySpec1}) + r.CreateResource(gomockinternal.AContext(), &natGatewaySpec1, serviceName).Return("not a nat gateway", nil) + s.UpdatePutStatus(infrav1.NATGatewaysReadyCondition, serviceName, gomockinternal.ErrStrEq("created resource string is not a network.NatGateway")) }, }, } @@ -271,13 +134,13 @@ func TestReconcileNatGateways(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() scopeMock := mock_natgateways.NewMockNatGatewayScope(mockCtrl) - clientMock := mock_natgateways.NewMockclient(mockCtrl) + asyncMock := mock_async.NewMockReconciler(mockCtrl) - tc.expect(scopeMock.EXPECT(), clientMock.EXPECT()) + tc.expect(scopeMock.EXPECT(), asyncMock.EXPECT()) s := &Service{ - Scope: scopeMock, - client: clientMock, + Scope: scopeMock, + Reconciler: asyncMock, } err := s.Reconcile(context.TODO()) @@ -296,104 +159,40 @@ func TestDeleteNatGateway(t *testing.T) { name string tags infrav1.Tags expectedError string - expect func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) + expect func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ { - name: "nat gateways in custom vnet mode", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "shared", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "NAT gateways in custom vnet mode", + tags: customVNetTags, expectedError: "", - expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - ID: "1234", - Name: "my-vnet", - }) + expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.Vnet().Return(&customVNetSpec) s.ClusterName() + s.UpdateDeleteStatus(infrav1.NATGatewaysReadyCondition, serviceName, nil) }, }, { - name: "nat gateway deleted successfully", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "NAT gateway deleted successfully", + tags: ownedVNetTags, expectedError: "", - expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) + expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.Vnet().Return(&ownedVNetSpec) s.ClusterName() - s.NatGatewaySpecs().Return([]azure.NatGatewaySpec{ - { - Name: "my-node-natgateway", - Subnet: infrav1.SubnetSpec{ - Name: "node-subnet", - Role: infrav1.SubnetNode, - }, - }, - }) - s.ResourceGroup().Return("my-rg") - m.Delete(gomockinternal.AContext(), "my-rg", "my-node-natgateway") + s.NatGatewaySpecs().Return([]azure.ResourceSpecGetter{&natGatewaySpec1}) + r.DeleteResource(gomockinternal.AContext(), &natGatewaySpec1, serviceName).Return(nil) + s.UpdateDeleteStatus(infrav1.NATGatewaysReadyCondition, serviceName, nil) }, }, { - name: "nat gateway already deleted", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, - expectedError: "", - expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) - s.ClusterName() - s.NatGatewaySpecs().Return([]azure.NatGatewaySpec{ - { - Name: "my-node-natgateway", - Subnet: infrav1.SubnetSpec{ - Name: "node-subnet", - Role: infrav1.SubnetNode, - }, - }, - }) - s.ResourceGroup().Return("my-rg") - m.Delete(gomockinternal.AContext(), "my-rg", "my-node-natgateway").Return(autorest.NewErrorWithResponse("", "", &http.Response{ - StatusCode: 404, - }, "Not Found")) - }, - }, - { - name: "nat gateway deletion fails", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, - expectedError: "failed to delete nat gateway my-node-natgateway in resource group my-rg: #: Internal Server Error: StatusCode=500", - expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) + name: "NAT gateway deletion fails", + tags: ownedVNetTags, + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.Vnet().Return(&ownedVNetSpec) s.ClusterName() - s.NatGatewaySpecs().Return([]azure.NatGatewaySpec{ - { - Name: "my-node-natgateway", - Subnet: infrav1.SubnetSpec{ - Name: "node-subnet", - Role: infrav1.SubnetNode, - }, - }, - }) - s.ResourceGroup().AnyTimes().Return("my-rg") - m.Delete(gomockinternal.AContext(), "my-rg", "my-node-natgateway").Return(autorest.NewErrorWithResponse("", "", &http.Response{ - StatusCode: 500, - }, "Internal Server Error")) + s.NatGatewaySpecs().Return([]azure.ResourceSpecGetter{&natGatewaySpec1}) + r.DeleteResource(gomockinternal.AContext(), &natGatewaySpec1, serviceName).Return(internalError) + s.UpdateDeleteStatus(infrav1.NATGatewaysReadyCondition, serviceName, internalError) }, }, } @@ -406,13 +205,13 @@ func TestDeleteNatGateway(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() scopeMock := mock_natgateways.NewMockNatGatewayScope(mockCtrl) - clientMock := mock_natgateways.NewMockclient(mockCtrl) + asyncMock := mock_async.NewMockReconciler(mockCtrl) - tc.expect(scopeMock.EXPECT(), clientMock.EXPECT()) + tc.expect(scopeMock.EXPECT(), asyncMock.EXPECT()) s := &Service{ - Scope: scopeMock, - client: clientMock, + Scope: scopeMock, + Reconciler: asyncMock, } err := s.Delete(context.TODO()) diff --git a/azure/services/natgateways/spec.go b/azure/services/natgateways/spec.go new file mode 100644 index 00000000000..6999d48b4bb --- /dev/null +++ b/azure/services/natgateways/spec.go @@ -0,0 +1,98 @@ +/* +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 natgateways + +import ( + "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network" + autorest "github.com/Azure/go-autorest/autorest/azure" + "github.com/Azure/go-autorest/autorest/to" + "github.com/pkg/errors" + infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" + "sigs.k8s.io/cluster-api-provider-azure/azure" +) + +// NatGatewaySpec defines the specification for a NAT gateway. +type NatGatewaySpec struct { + Name string + ResourceGroup string + SubscriptionID string + Location string + NatGatewayIP infrav1.PublicIPSpec +} + +// ResourceName returns the name of the NAT gateway. +func (s *NatGatewaySpec) ResourceName() string { + return s.Name +} + +// ResourceGroupName returns the name of the resource group. +func (s *NatGatewaySpec) ResourceGroupName() string { + return s.ResourceGroup +} + +// OwnerResourceName is a no-op for NAT gateways. +func (s *NatGatewaySpec) OwnerResourceName() string { + return "" +} + +// Parameters returns the parameters for the NAT gateway. +func (s *NatGatewaySpec) Parameters(existing interface{}) (interface{}, error) { + if existing != nil { + existingNatGateway, ok := existing.(network.NatGateway) + if !ok { + return nil, errors.Errorf("%T is not a network.NatGateway", existing) + } + + if hasPublicIP(existingNatGateway, s.NatGatewayIP.Name) { + // Skip update for NAT gateway as it exists with expected values + return nil, nil + } + } + + natGatewayToCreate := network.NatGateway{ + Name: to.StringPtr(s.Name), + Location: to.StringPtr(s.Location), + Sku: &network.NatGatewaySku{Name: network.NatGatewaySkuNameStandard}, + NatGatewayPropertiesFormat: &network.NatGatewayPropertiesFormat{ + PublicIPAddresses: &[]network.SubResource{ + { + ID: to.StringPtr(azure.PublicIPID(s.SubscriptionID, s.ResourceGroupName(), s.NatGatewayIP.Name)), + }, + }, + }, + } + + return natGatewayToCreate, nil +} + +func hasPublicIP(natGateway network.NatGateway, publicIPName string) bool { + // We must have a non-nil, non-"empty" PublicIPAddresses + if !(natGateway.PublicIPAddresses != nil && len(*natGateway.PublicIPAddresses) > 0) { + return false + } + + for _, publicIP := range *natGateway.PublicIPAddresses { + resource, err := autorest.ParseResourceID(*publicIP.ID) + if err != nil { + continue + } + if resource.ResourceName == publicIPName { + return true + } + } + return false +} diff --git a/azure/services/subnets/subnets_test.go b/azure/services/subnets/subnets_test.go index 381f7adae50..744d181f5ca 100644 --- a/azure/services/subnets/subnets_test.go +++ b/azure/services/subnets/subnets_test.go @@ -352,7 +352,7 @@ func TestReconcileSubnets(t *testing.T) { }, }, { - name: "doesn't overwrite existing NAT Gateway", + name: "doesn't overwrite existing NAT gateway", expectedError: "", expect: func(s *mock_subnets.MockSubnetScopeMockRecorder, m *mock_subnets.MockClientMockRecorder) { s.Subnet("my-subnet").AnyTimes().Return(infrav1.SubnetSpec{ diff --git a/azure/types.go b/azure/types.go index 14c92d3668f..351ffc24841 100644 --- a/azure/types.go +++ b/azure/types.go @@ -73,13 +73,6 @@ type RouteTableSpec struct { Subnet infrav1.SubnetSpec } -// NatGatewaySpec defines the specification for a Nat Gateway. -type NatGatewaySpec struct { - NatGatewayIP infrav1.PublicIPSpec - Name string - Subnet infrav1.SubnetSpec -} - // InboundNatSpec defines the specification for an inbound NAT rule. type InboundNatSpec struct { Name string diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_azureclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_azureclusters.yaml index 9c1b1281ce3..7280e7a7fb2 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_azureclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_azureclusters.yaml @@ -1346,7 +1346,7 @@ spec: description: NatGateway associated with this subnet. properties: id: - description: ID is the Azure resource ID of the nat + description: ID is the Azure resource ID of the NAT gateway. READ-ONLY type: string ip: @@ -1816,7 +1816,7 @@ spec: description: NatGateway associated with this subnet. properties: id: - description: ID is the Azure resource ID of the nat + description: ID is the Azure resource ID of the NAT gateway. READ-ONLY type: string ip: diff --git a/controllers/azurecluster_reconciler.go b/controllers/azurecluster_reconciler.go index 7f4c2a7e77e..26e6d5b5063 100644 --- a/controllers/azurecluster_reconciler.go +++ b/controllers/azurecluster_reconciler.go @@ -117,7 +117,7 @@ func (s *azureClusterService) Reconcile(ctx context.Context) error { } if err := s.natGatewaySvc.Reconcile(ctx); err != nil { - return errors.Wrapf(err, "failed to reconcile nat gateway") + return errors.Wrapf(err, "failed to reconcile NAT gateway") } if err := s.subnetsSvc.Reconcile(ctx); err != nil { @@ -175,7 +175,7 @@ func (s *azureClusterService) Delete(ctx context.Context) error { } if err := s.natGatewaySvc.Delete(ctx); err != nil { - return errors.Wrapf(err, "failed to delete nat gateway") + return errors.Wrapf(err, "failed to delete NAT gateway") } if err := s.publicIPSvc.Delete(ctx); err != nil { diff --git a/docs/book/src/topics/node-outbound-lb.md b/docs/book/src/topics/node-outbound-lb.md index 8e80f3cceea..082671c893b 100644 --- a/docs/book/src/topics/node-outbound-lb.md +++ b/docs/book/src/topics/node-outbound-lb.md @@ -69,10 +69,10 @@ spec: frontendIPsCount: 1 ``` -## Node Outbound Nat Gateway +## Node Outbound NAT gateway -You can configure a [Nat Gateway](https://docs.microsoft.com/en-us/azure/virtual-network/nat-gateway-resource) in a subnet to enable outbound traffic in the cluster nodes by setting the Nat Gateway's name in the subnet configuration. -A Public IP will also be created for the Nat Gateway. +You can configure a [NAT gateway](https://docs.microsoft.com/en-us/azure/virtual-network/nat-gateway-resource) in a subnet to enable outbound traffic in the cluster nodes by setting the NAT gateway's name in the subnet configuration. +A Public IP will also be created for the NAT gateway. Using this configuration, [a Load Balancer for the nodes outbound traffic](./node-outbound-lb.md) won't be created. @@ -80,7 +80,7 @@ Using this configuration, [a Load Balancer for the nodes outbound traffic](./nod