From 494e77d5d884530cfaceaf279f58142e65e15965 Mon Sep 17 00:00:00 2001 From: Jonathan Tong Date: Wed, 1 Dec 2021 15:48:26 -0500 Subject: [PATCH] Update service to use refactor --- azure/scope/cluster.go | 9 + azure/services/natgateways/client.go | 39 +- .../mock_natgateways/client_mock.go | 130 ------ .../natgateways/mock_natgateways/doc.go | 2 - .../mock_natgateways/natgateways_mock.go | 12 + azure/services/natgateways/natgateways.go | 55 +-- .../services/natgateways/natgateways_test.go | 383 ++++-------------- azure/services/natgateways/spec.go | 1 + 8 files changed, 141 insertions(+), 490 deletions(-) delete mode 100644 azure/services/natgateways/mock_natgateways/client_mock.go diff --git a/azure/scope/cluster.go b/azure/scope/cluster.go index 176a4b4eb52d..7a14e338acf1 100644 --- a/azure/scope/cluster.go +++ b/azure/scope/cluster.go @@ -454,6 +454,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() diff --git a/azure/services/natgateways/client.go b/azure/services/natgateways/client.go index b245b32b2e7f..a8dc60f17c97 100644 --- a/azure/services/natgateways/client.go +++ b/azure/services/natgateways/client.go @@ -31,22 +31,11 @@ import ( "sigs.k8s.io/cluster-api-provider-azure/util/tele" ) -// client wraps go-sdk. -type client interface { - Get(context.Context, string, string) (network.NatGateway, 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. 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()) @@ -61,40 +50,23 @@ 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) { +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(), "") } // 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) (interface{}, azureautorest.FutureAPI, error) { +func (ac *azureClient) CreateOrUpdateAsync(ctx context.Context, spec azure.ResourceSpecGetter, parameters interface{}) (interface{}, azureautorest.FutureAPI, error) { ctx, _, done := tele.StartSpanWithLogger(ctx, "natgateways.Service.CreateOrUpdateAsync") defer done() - var existingNatGateway interface{} - - if existing, err := ac.Get(ctx, spec.ResourceGroupName(), spec.ResourceName()); err != nil && !azure.ResourceNotFound(err) { - return nil, nil, errors.Wrapf(err, "failed to get Nat Gateway %s for %s in %s", spec.ResourceName(), spec.OwnerResourceName(), spec.ResourceGroupName()) - } else if err == nil { - existingNatGateway = existing - } - - params, err := spec.Parameters(existingNatGateway) - if err != nil { - return nil, nil, errors.Wrapf(err, "failed to get desired parameters for Nat Gateway %s", spec.ResourceName()) - } - - natGateway, ok := params.(network.NatGateway) + natGateway, ok := parameters.(network.NatGateway) if !ok { - if params == nil { - // nothing to do here. - return existingNatGateway, nil, nil - } - return nil, nil, errors.Errorf("%T is not a network.NatGateway", params) + return nil, nil, errors.Errorf("%T is not a network.NatGateway", parameters) } future, err := ac.natgateways.CreateOrUpdate(ctx, spec.ResourceGroupName(), spec.ResourceName(), natGateway) @@ -113,7 +85,6 @@ func (ac *azureClient) CreateOrUpdateAsync(ctx context.Context, spec azure.Resou } result, err := future.Result(ac.natgateways) - // if the operation completed, return a nil future return result, nil, err } 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 629fe686b84a..000000000000 --- a/azure/services/natgateways/mock_natgateways/client_mock.go +++ /dev/null @@ -1,130 +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/2021-02-01/network" - azure "github.com/Azure/go-autorest/autorest/azure" - gomock "github.com/golang/mock/gomock" - azure0 "sigs.k8s.io/cluster-api-provider-azure/azure" -) - -// 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 -} - -// CreateOrUpdateAsync mocks base method. -func (m *Mockclient) CreateOrUpdateAsync(arg0 context.Context, arg1 azure0.ResourceSpecGetter) (interface{}, azure.FutureAPI, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateOrUpdateAsync", arg0, arg1) - ret0, _ := ret[0].(interface{}) - ret1, _ := ret[1].(azure.FutureAPI) - ret2, _ := ret[2].(error) - return ret0, ret1, ret2 -} - -// CreateOrUpdateAsync indicates an expected call of CreateOrUpdateAsync. -func (mr *MockclientMockRecorder) CreateOrUpdateAsync(arg0, arg1 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdateAsync", reflect.TypeOf((*Mockclient)(nil).CreateOrUpdateAsync), arg0, arg1) -} - -// DeleteAsync mocks base method. -func (m *Mockclient) DeleteAsync(arg0 context.Context, arg1 azure0.ResourceSpecGetter) (azure.FutureAPI, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteAsync", arg0, arg1) - ret0, _ := ret[0].(azure.FutureAPI) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// DeleteAsync indicates an expected call of DeleteAsync. -func (mr *MockclientMockRecorder) DeleteAsync(arg0, arg1 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAsync", reflect.TypeOf((*Mockclient)(nil).DeleteAsync), arg0, arg1) -} - -// 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) -} - -// IsDone mocks base method. -func (m *Mockclient) IsDone(arg0 context.Context, arg1 azure.FutureAPI) (bool, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "IsDone", arg0, arg1) - ret0, _ := ret[0].(bool) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// IsDone indicates an expected call of IsDone. -func (mr *MockclientMockRecorder) IsDone(arg0, arg1 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsDone", reflect.TypeOf((*Mockclient)(nil).IsDone), arg0, arg1) -} - -// Result mocks base method. -func (m *Mockclient) Result(arg0 context.Context, arg1 azure.FutureAPI, arg2 string) (interface{}, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Result", arg0, arg1, arg2) - ret0, _ := ret[0].(interface{}) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// Result indicates an expected call of Result. -func (mr *MockclientMockRecorder) Result(arg0, arg1, arg2 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Result", reflect.TypeOf((*Mockclient)(nil).Result), arg0, arg1, arg2) -} diff --git a/azure/services/natgateways/mock_natgateways/doc.go b/azure/services/natgateways/mock_natgateways/doc.go index 6c25bb36c5e9..b58fcf8699e7 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 295415601707..14720b7fd347 100644 --- a/azure/services/natgateways/mock_natgateways/natgateways_mock.go +++ b/azure/services/natgateways/mock_natgateways/natgateways_mock.go @@ -490,6 +490,18 @@ func (mr *MockNatGatewayScopeMockRecorder) SetLongRunningOperationState(arg0 int 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() diff --git a/azure/services/natgateways/natgateways.go b/azure/services/natgateways/natgateways.go index 64b1775d54b1..953bfb05b243 100644 --- a/azure/services/natgateways/natgateways.go +++ b/azure/services/natgateways/natgateways.go @@ -37,20 +37,22 @@ type NatGatewayScope interface { logr.Logger azure.ClusterScoper 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), } } @@ -63,26 +65,33 @@ func (s *Service) Reconcile(ctx context.Context) error { ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout) defer cancel() + if !s.Scope.Vnet().IsManaged(s.Scope.ClusterName()) { + s.Scope.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() { - natGateway, err := async.CreateResource(ctx, s.Scope, s.client, natGatewaySpec, serviceName) + result, err := s.CreateResource(ctx, natGatewaySpec, serviceName) if err != nil { if !azure.IsOperationNotDoneError(err) || resultingErr == nil { resultingErr = err } } - if err == nil && resultingErr != nil { - networkNatGateway, ok := natGateway.(network.NatGateway) + if err == nil { + natGateway, ok := result.(network.NatGateway) if !ok { // Return out of loop since this would be an unexpcted fatal error - return errors.Errorf("created resource %T is not a network.NatGateway", natGateway) + return errors.Errorf("created resource %T is not a network.NatGateway", result) } // TODO: is it necessary to set the spec since it doesn't appear to change any behavior - s.SetNatGatewayIDInSubnets(natGatewaySpec.ResourceName(), *networkNatGateway.ID) + s.Scope.SetNatGatewayIDInSubnets(natGatewaySpec.ResourceName(), *natGateway.ID) } } @@ -98,27 +107,25 @@ func (s *Service) Delete(ctx context.Context) error { ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout) defer cancel() - var result error + if !s.Scope.Vnet().IsManaged(s.Scope.ClusterName()) { + s.Scope.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 result of the previous one. + // 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() { - if err := async.DeleteResource(ctx, s.Scope, s.client, natGatewaySpec, serviceName); err != nil { - if !azure.IsOperationNotDoneError(err) || result == nil { - result = err + if err := s.DeleteResource(ctx, natGatewaySpec, serviceName); err != nil { + if !azure.IsOperationNotDoneError(err) || resultingErr == nil { + resultingErr = err } } } - s.Scope.UpdateDeleteStatus(infrav1.NATGatewaysReadyCondition, serviceName, result) - return result -} - -func (s *Service) SetNatGatewayIDInSubnets(name string, id string) { - for _, subnet := range s.Scope.Subnets() { - if subnet.NatGateway.Name == name { - subnet.NatGateway.ID = id - s.Scope.SetSubnet(subnet) - } - } + 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 f83e87320c6a..0c1a95d93444 100644 --- a/azure/services/natgateways/natgateways_test.go +++ b/azure/services/natgateways/natgateways_test.go @@ -32,6 +32,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/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" ) @@ -40,233 +41,80 @@ 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.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - 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.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - 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.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) 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", - }) - s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - 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) - }, - }, - { - 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", - }) + expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.Vnet().Return(&ownedVNetSpec) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) 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(natGateway1, nil) + s.SetNatGatewayIDInSubnets(natGatewaySpec1.Name, *natGateway1.ID) + s.UpdatePutStatus(infrav1.NATGatewaysReadyCondition, serviceName, nil) }, }, { - 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: "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.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) 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(nil, internalError) + s.UpdatePutStatus(infrav1.NATGatewaysReadyCondition, serviceName, internalError) }, }, } @@ -279,13 +127,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()) @@ -304,108 +152,43 @@ 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.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) 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.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) 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.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - 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.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) 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) }, }, } @@ -418,13 +201,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 index c7c97618536a..f67707882475 100644 --- a/azure/services/natgateways/spec.go +++ b/azure/services/natgateways/spec.go @@ -70,6 +70,7 @@ func (s *NatGatewaySpec) Parameters(existing interface{}) (interface{}, error) { } natGatewayToCreate := network.NatGateway{ + Name: to.StringPtr(s.Name), Location: to.StringPtr(s.Location), Sku: &network.NatGatewaySku{Name: network.NatGatewaySkuNameStandard}, NatGatewayPropertiesFormat: &network.NatGatewayPropertiesFormat{