diff --git a/azure/services/async/async.go b/azure/services/async/async.go index d7a374a336c..1314f5f0ac7 100644 --- a/azure/services/async/async.go +++ b/azure/services/async/async.go @@ -30,6 +30,22 @@ import ( "sigs.k8s.io/cluster-api-provider-azure/util/tele" ) +// Service is an implementation of the Reconciler interface. It handles asynchronous creation and deletion of resources. +type Service struct { + Scope FutureScope + Creator + Deleter +} + +// New creates a new async service. +func New(scope FutureScope, createClient Creator, deleteClient Deleter) *Service { + return &Service{ + Scope: scope, + Creator: createClient, + Deleter: deleteClient, + } +} + // processOngoingOperation is a helper function that will process an ongoing operation to check if it is done. // If it is not done, it will return a transient error. func processOngoingOperation(ctx context.Context, scope FutureScope, client FutureHandler, resourceName string, serviceName string) (interface{}, error) { @@ -71,7 +87,7 @@ func processOngoingOperation(ctx context.Context, scope FutureScope, client Futu } // CreateResource implements the logic for creating a resource Asynchronously. -func CreateResource(ctx context.Context, scope FutureScope, client Creator, spec azure.ResourceSpecGetter, serviceName string) (interface{}, error) { +func (s *Service) CreateResource(ctx context.Context, spec azure.ResourceSpecGetter, serviceName string) (interface{}, error) { ctx, log, done := tele.StartSpanWithLogger(ctx, "async.Service.CreateResource") defer done() @@ -79,20 +95,39 @@ func CreateResource(ctx context.Context, scope FutureScope, client Creator, spec rgName := spec.ResourceGroupName() // Check if there is an ongoing long running operation. - future := scope.GetLongRunningOperationState(resourceName, serviceName) + future := s.Scope.GetLongRunningOperationState(resourceName, serviceName) if future != nil { - return processOngoingOperation(ctx, scope, client, resourceName, serviceName) + return processOngoingOperation(ctx, s.Scope, s.Creator, resourceName, serviceName) } - // No long running operation is active, so create the resource. + // Get the resource if it already exists, and use it to construct the desired resource parameters. + var existingResource interface{} + if existing, err := s.Creator.Get(ctx, spec); err != nil && !azure.ResourceNotFound(err) { + return nil, errors.Wrapf(err, "failed to get existing resource %s/%s (service: %s)", rgName, resourceName, serviceName) + } else if err == nil { + existingResource = existing + log.V(2).Info("successfully got existing resource", "service", serviceName, "resource", resourceName, "resourceGroup", rgName) + } + + // Construct parameters using the resource spec and information from the existing resource, if there is one. + parameters, err := spec.Parameters(existingResource) + if err != nil { + return nil, errors.Wrapf(err, "failed to get desired parameters for resource %s/%s (service: %s)", rgName, resourceName, serviceName) + } else if parameters == nil { + // Nothing to do, don't create or update the resource and return the existing resource. + log.V(2).Info("resource up to date", "service", serviceName, "resource", resourceName, "resourceGroup", rgName) + return existingResource, nil + } + + // Create or update the resource with the desired parameters. log.V(2).Info("creating resource", "service", serviceName, "resource", resourceName, "resourceGroup", rgName) - result, sdkFuture, err := client.CreateOrUpdateAsync(ctx, spec) + result, sdkFuture, err := s.Creator.CreateOrUpdateAsync(ctx, spec, parameters) if sdkFuture != nil { future, err := converters.SDKToFuture(sdkFuture, infrav1.PutFuture, serviceName, resourceName, rgName) if err != nil { return nil, errors.Wrapf(err, "failed to create resource %s/%s (service: %s)", rgName, resourceName, serviceName) } - scope.SetLongRunningOperationState(future) + s.Scope.SetLongRunningOperationState(future) return nil, azure.WithTransientError(azure.NewOperationNotDoneError(future), retryAfter(sdkFuture)) } else if err != nil { return nil, errors.Wrapf(err, "failed to create resource %s/%s (service: %s)", rgName, resourceName, serviceName) @@ -103,7 +138,7 @@ func CreateResource(ctx context.Context, scope FutureScope, client Creator, spec } // DeleteResource implements the logic for deleting a resource Asynchronously. -func DeleteResource(ctx context.Context, scope FutureScope, client Deleter, spec azure.ResourceSpecGetter, serviceName string) error { +func (s *Service) DeleteResource(ctx context.Context, spec azure.ResourceSpecGetter, serviceName string) error { ctx, log, done := tele.StartSpanWithLogger(ctx, "async.Service.DeleteResource") defer done() @@ -111,21 +146,21 @@ func DeleteResource(ctx context.Context, scope FutureScope, client Deleter, spec rgName := spec.ResourceGroupName() // Check if there is an ongoing long running operation. - future := scope.GetLongRunningOperationState(resourceName, serviceName) + future := s.Scope.GetLongRunningOperationState(resourceName, serviceName) if future != nil { - _, err := processOngoingOperation(ctx, scope, client, resourceName, serviceName) + _, err := processOngoingOperation(ctx, s.Scope, s.Deleter, resourceName, serviceName) return err } // No long running operation is active, so delete the resource. log.V(2).Info("deleting resource", "service", serviceName, "resource", resourceName, "resourceGroup", rgName) - sdkFuture, err := client.DeleteAsync(ctx, spec) + sdkFuture, err := s.Deleter.DeleteAsync(ctx, spec) if sdkFuture != nil { future, err := converters.SDKToFuture(sdkFuture, infrav1.DeleteFuture, serviceName, resourceName, rgName) if err != nil { return errors.Wrapf(err, "failed to delete resource %s/%s (service: %s)", rgName, resourceName, serviceName) } - scope.SetLongRunningOperationState(future) + s.Scope.SetLongRunningOperationState(future) return azure.WithTransientError(azure.NewOperationNotDoneError(future), retryAfter(sdkFuture)) } else if err != nil { if azure.ResourceNotFound(err) { diff --git a/azure/services/async/async_test.go b/azure/services/async/async_test.go index 4a7166b0f49..87b26fadb30 100644 --- a/azure/services/async/async_test.go +++ b/azure/services/async/async_test.go @@ -25,7 +25,6 @@ import ( "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2019-05-01/resources" "github.com/Azure/go-autorest/autorest" azureautorest "github.com/Azure/go-autorest/autorest/azure" - "github.com/Azure/go-autorest/autorest/to" "github.com/golang/mock/gomock" . "github.com/onsi/gomega" infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" @@ -56,8 +55,11 @@ var ( ResourceGroup: "test-group", Data: "ZmFrZSBiNjQgZnV0dXJlIGRhdGEK", } - fakeError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error") - errCtxExceeded = errors.New("ctx exceeded") + fakeExistingResource = resources.GenericResource{} + fakeResourceParameters = resources.GenericResource{} + fakeInternalError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error") + fakeNotFoundError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not Found") + errCtxExceeded = errors.New("ctx exceeded") ) // TestProcessOngoingOperation tests the processOngoingOperation function. @@ -96,7 +98,7 @@ func TestProcessOngoingOperation(t *testing.T) { serviceName: "test-service", expect: func(s *mock_async.MockFutureScopeMockRecorder, c *mock_async.MockFutureHandlerMockRecorder) { s.GetLongRunningOperationState("test-resource", "test-service").Return(&validDeleteFuture) - c.IsDone(gomockinternal.AContext(), gomock.AssignableToTypeOf(&azureautorest.Future{})).Return(false, fakeError) + c.IsDone(gomockinternal.AContext(), gomock.AssignableToTypeOf(&azureautorest.Future{})).Return(false, fakeInternalError) }, }, { @@ -112,14 +114,14 @@ func TestProcessOngoingOperation(t *testing.T) { { name: "operation is done", expectedError: "", - expectedResult: resources.Group{Name: to.StringPtr("test-resource")}, + expectedResult: &fakeExistingResource, resourceName: "test-resource", serviceName: "test-service", expect: func(s *mock_async.MockFutureScopeMockRecorder, c *mock_async.MockFutureHandlerMockRecorder) { s.GetLongRunningOperationState("test-resource", "test-service").Return(&validDeleteFuture) c.IsDone(gomockinternal.AContext(), gomock.AssignableToTypeOf(&azureautorest.Future{})).Return(true, nil) s.DeleteLongRunningOperationState("test-resource", "test-service") - c.Result(gomockinternal.AContext(), gomock.AssignableToTypeOf(&azureautorest.Future{}), infrav1.DeleteFuture).Return(resources.Group{Name: to.StringPtr("test-resource")}, nil) + c.Result(gomockinternal.AContext(), gomock.AssignableToTypeOf(&azureautorest.Future{}), infrav1.DeleteFuture).Return(&fakeExistingResource, nil) }, }, } @@ -182,7 +184,59 @@ func TestCreateResource(t *testing.T) { r.ResourceName().Return("test-resource") r.ResourceGroupName().Return("test-group") s.GetLongRunningOperationState("test-resource", "test-service").Return(nil) - c.CreateOrUpdateAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(&mock_azure.MockResourceSpecGetter{})).Return("test-resource", nil, nil) + c.Get(gomockinternal.AContext(), gomock.AssignableToTypeOf(&mock_azure.MockResourceSpecGetter{})).Return(&fakeExistingResource, nil) + r.Parameters(&fakeExistingResource).Return(&fakeResourceParameters, nil) + c.CreateOrUpdateAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(&mock_azure.MockResourceSpecGetter{}), &fakeResourceParameters).Return("test-resource", nil, nil) + }, + }, + { + name: "error occurs while running async get", + expectedError: "failed to get existing resource test-group/test-resource (service: test-service)", + serviceName: "test-service", + expect: func(s *mock_async.MockFutureScopeMockRecorder, c *mock_async.MockCreatorMockRecorder, r *mock_azure.MockResourceSpecGetterMockRecorder) { + r.ResourceName().Return("test-resource") + r.ResourceGroupName().Return("test-group") + s.GetLongRunningOperationState("test-resource", "test-service").Return(nil) + c.Get(gomockinternal.AContext(), gomock.AssignableToTypeOf(&mock_azure.MockResourceSpecGetter{})).Return(nil, fakeInternalError) + }, + }, + { + name: "async get returns not found", + expectedError: "", + serviceName: "test-service", + expectedResult: &fakeExistingResource, + expect: func(s *mock_async.MockFutureScopeMockRecorder, c *mock_async.MockCreatorMockRecorder, r *mock_azure.MockResourceSpecGetterMockRecorder) { + r.ResourceName().Return("test-resource") + r.ResourceGroupName().Return("test-group") + s.GetLongRunningOperationState("test-resource", "test-service").Return(nil) + c.Get(gomockinternal.AContext(), gomock.AssignableToTypeOf(&mock_azure.MockResourceSpecGetter{})).Return(nil, fakeNotFoundError) + r.Parameters(nil).Return(&fakeResourceParameters, nil) + c.CreateOrUpdateAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(&mock_azure.MockResourceSpecGetter{}), &fakeResourceParameters).Return(&fakeExistingResource, nil, nil) + }, + }, + { + name: "error occurs while running async spec parameters", + expectedError: "failed to get desired parameters for resource test-group/test-resource (service: test-service)", + serviceName: "test-service", + expect: func(s *mock_async.MockFutureScopeMockRecorder, c *mock_async.MockCreatorMockRecorder, r *mock_azure.MockResourceSpecGetterMockRecorder) { + r.ResourceName().Return("test-resource") + r.ResourceGroupName().Return("test-group") + s.GetLongRunningOperationState("test-resource", "test-service").Return(nil) + c.Get(gomockinternal.AContext(), gomock.AssignableToTypeOf(&mock_azure.MockResourceSpecGetter{})).Return(&fakeExistingResource, nil) + r.Parameters(&fakeExistingResource).Return(nil, fakeInternalError) + }, + }, + { + name: "async spec parameters returns nil", + expectedError: "", + serviceName: "test-service", + expectedResult: &fakeExistingResource, + expect: func(s *mock_async.MockFutureScopeMockRecorder, c *mock_async.MockCreatorMockRecorder, r *mock_azure.MockResourceSpecGetterMockRecorder) { + r.ResourceName().Return("test-resource") + r.ResourceGroupName().Return("test-group") + s.GetLongRunningOperationState("test-resource", "test-service").Return(nil) + c.Get(gomockinternal.AContext(), gomock.AssignableToTypeOf(&mock_azure.MockResourceSpecGetter{})).Return(&fakeExistingResource, nil) + r.Parameters(&fakeExistingResource).Return(nil, nil) }, }, { @@ -193,7 +247,9 @@ func TestCreateResource(t *testing.T) { r.ResourceName().Return("test-resource") r.ResourceGroupName().Return("test-group") s.GetLongRunningOperationState("test-resource", "test-service").Return(nil) - c.CreateOrUpdateAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(&mock_azure.MockResourceSpecGetter{})).Return(nil, nil, fakeError) + c.Get(gomockinternal.AContext(), gomock.AssignableToTypeOf(&mock_azure.MockResourceSpecGetter{})).Return(&fakeExistingResource, nil) + r.Parameters(&fakeExistingResource).Return(&fakeResourceParameters, nil) + c.CreateOrUpdateAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(&mock_azure.MockResourceSpecGetter{}), &fakeResourceParameters).Return(nil, nil, fakeInternalError) }, }, { @@ -204,7 +260,9 @@ func TestCreateResource(t *testing.T) { r.ResourceName().Return("test-resource") r.ResourceGroupName().Return("test-group") s.GetLongRunningOperationState("test-resource", "test-service").Return(nil) - c.CreateOrUpdateAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(&mock_azure.MockResourceSpecGetter{})).Return(nil, &azureautorest.Future{}, errCtxExceeded) + c.Get(gomockinternal.AContext(), gomock.AssignableToTypeOf(&mock_azure.MockResourceSpecGetter{})).Return(&fakeExistingResource, nil) + r.Parameters(&fakeExistingResource).Return(&fakeResourceParameters, nil) + c.CreateOrUpdateAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(&mock_azure.MockResourceSpecGetter{}), &fakeResourceParameters).Return(nil, &azureautorest.Future{}, errCtxExceeded) s.SetLongRunningOperationState(gomock.AssignableToTypeOf(&infrav1.Future{})) }, }, @@ -219,12 +277,13 @@ func TestCreateResource(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() scopeMock := mock_async.NewMockFutureScope(mockCtrl) - clientMock := mock_async.NewMockCreator(mockCtrl) + creatorMock := mock_async.NewMockCreator(mockCtrl) specMock := mock_azure.NewMockResourceSpecGetter(mockCtrl) - tc.expect(scopeMock.EXPECT(), clientMock.EXPECT(), specMock.EXPECT()) + tc.expect(scopeMock.EXPECT(), creatorMock.EXPECT(), specMock.EXPECT()) - result, err := CreateResource(context.TODO(), scopeMock, clientMock, specMock, tc.serviceName) + s := New(scopeMock, creatorMock, nil) + result, err := s.CreateResource(context.TODO(), specMock, tc.serviceName) if tc.expectedError != "" { g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring(tc.expectedError)) @@ -266,6 +325,17 @@ func TestDeleteResource(t *testing.T) { c.DeleteAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(&mock_azure.MockResourceSpecGetter{})).Return(nil, nil) }, }, + { + name: "delete async returns not found", + expectedError: "", + serviceName: "test-service", + expect: func(s *mock_async.MockFutureScopeMockRecorder, c *mock_async.MockDeleterMockRecorder, r *mock_azure.MockResourceSpecGetterMockRecorder) { + r.ResourceName().Return("test-resource") + r.ResourceGroupName().Return("test-group") + s.GetLongRunningOperationState("test-resource", "test-service").Return(nil) + c.DeleteAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(&mock_azure.MockResourceSpecGetter{})).Return(nil, fakeNotFoundError) + }, + }, { name: "error occurs while running async delete", expectedError: "failed to delete resource test-group/test-resource (service: test-service)", @@ -274,7 +344,7 @@ func TestDeleteResource(t *testing.T) { r.ResourceName().Return("test-resource") r.ResourceGroupName().Return("test-group") s.GetLongRunningOperationState("test-resource", "test-service").Return(nil) - c.DeleteAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(&mock_azure.MockResourceSpecGetter{})).Return(nil, fakeError) + c.DeleteAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(&mock_azure.MockResourceSpecGetter{})).Return(nil, fakeInternalError) }, }, { @@ -300,12 +370,13 @@ func TestDeleteResource(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() scopeMock := mock_async.NewMockFutureScope(mockCtrl) - clientMock := mock_async.NewMockDeleter(mockCtrl) + deleterMock := mock_async.NewMockDeleter(mockCtrl) specMock := mock_azure.NewMockResourceSpecGetter(mockCtrl) - tc.expect(scopeMock.EXPECT(), clientMock.EXPECT(), specMock.EXPECT()) + tc.expect(scopeMock.EXPECT(), deleterMock.EXPECT(), specMock.EXPECT()) - err := DeleteResource(context.TODO(), scopeMock, clientMock, specMock, tc.serviceName) + s := New(scopeMock, nil, deleterMock) + err := s.DeleteResource(context.TODO(), specMock, tc.serviceName) if tc.expectedError != "" { g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring(tc.expectedError)) diff --git a/azure/services/async/interfaces.go b/azure/services/async/interfaces.go index 21fb893b85a..c66ec3a0d57 100644 --- a/azure/services/async/interfaces.go +++ b/azure/services/async/interfaces.go @@ -39,7 +39,8 @@ type FutureHandler interface { // Creator is a client that can create or update a resource asynchronously. type Creator interface { FutureHandler - CreateOrUpdateAsync(ctx context.Context, spec azure.ResourceSpecGetter) (interface{}, azureautorest.FutureAPI, error) + CreateOrUpdateAsync(ctx context.Context, spec azure.ResourceSpecGetter, existingResource interface{}) (interface{}, azureautorest.FutureAPI, error) + Get(ctx context.Context, spec azure.ResourceSpecGetter) (interface{}, error) } // Deleter is a client that can delete a resource asynchronously. @@ -47,3 +48,9 @@ type Deleter interface { FutureHandler DeleteAsync(ctx context.Context, spec azure.ResourceSpecGetter) (azureautorest.FutureAPI, error) } + +// Reconciler is a generic interface used to perform asynchronous reconciliation of Azure resources. +type Reconciler interface { + CreateResource(ctx context.Context, spec azure.ResourceSpecGetter, serviceName string) (interface{}, error) + DeleteResource(ctx context.Context, spec azure.ResourceSpecGetter, serviceName string) error +} diff --git a/azure/services/async/mock_async/async_mock.go b/azure/services/async/mock_async/async_mock.go index dd5595034f3..4c4c1da09a1 100644 --- a/azure/services/async/mock_async/async_mock.go +++ b/azure/services/async/mock_async/async_mock.go @@ -205,9 +205,9 @@ func (m *MockCreator) EXPECT() *MockCreatorMockRecorder { } // CreateOrUpdateAsync mocks base method. -func (m *MockCreator) CreateOrUpdateAsync(ctx context.Context, spec azure0.ResourceSpecGetter) (interface{}, azure.FutureAPI, error) { +func (m *MockCreator) CreateOrUpdateAsync(ctx context.Context, spec azure0.ResourceSpecGetter, existingResource interface{}) (interface{}, azure.FutureAPI, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateOrUpdateAsync", ctx, spec) + ret := m.ctrl.Call(m, "CreateOrUpdateAsync", ctx, spec, existingResource) ret0, _ := ret[0].(interface{}) ret1, _ := ret[1].(azure.FutureAPI) ret2, _ := ret[2].(error) @@ -215,9 +215,24 @@ func (m *MockCreator) CreateOrUpdateAsync(ctx context.Context, spec azure0.Resou } // CreateOrUpdateAsync indicates an expected call of CreateOrUpdateAsync. -func (mr *MockCreatorMockRecorder) CreateOrUpdateAsync(ctx, spec interface{}) *gomock.Call { +func (mr *MockCreatorMockRecorder) CreateOrUpdateAsync(ctx, spec, existingResource interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdateAsync", reflect.TypeOf((*MockCreator)(nil).CreateOrUpdateAsync), ctx, spec) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdateAsync", reflect.TypeOf((*MockCreator)(nil).CreateOrUpdateAsync), ctx, spec, existingResource) +} + +// Get mocks base method. +func (m *MockCreator) Get(ctx context.Context, spec azure0.ResourceSpecGetter) (interface{}, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Get", ctx, spec) + ret0, _ := ret[0].(interface{}) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Get indicates an expected call of Get. +func (mr *MockCreatorMockRecorder) Get(ctx, spec interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockCreator)(nil).Get), ctx, spec) } // IsDone mocks base method. @@ -317,3 +332,55 @@ func (mr *MockDeleterMockRecorder) Result(ctx, future, futureType interface{}) * mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Result", reflect.TypeOf((*MockDeleter)(nil).Result), ctx, future, futureType) } + +// MockReconciler is a mock of Reconciler interface. +type MockReconciler struct { + ctrl *gomock.Controller + recorder *MockReconcilerMockRecorder +} + +// MockReconcilerMockRecorder is the mock recorder for MockReconciler. +type MockReconcilerMockRecorder struct { + mock *MockReconciler +} + +// NewMockReconciler creates a new mock instance. +func NewMockReconciler(ctrl *gomock.Controller) *MockReconciler { + mock := &MockReconciler{ctrl: ctrl} + mock.recorder = &MockReconcilerMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockReconciler) EXPECT() *MockReconcilerMockRecorder { + return m.recorder +} + +// CreateResource mocks base method. +func (m *MockReconciler) CreateResource(ctx context.Context, spec azure0.ResourceSpecGetter, serviceName string) (interface{}, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CreateResource", ctx, spec, serviceName) + ret0, _ := ret[0].(interface{}) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// CreateResource indicates an expected call of CreateResource. +func (mr *MockReconcilerMockRecorder) CreateResource(ctx, spec, serviceName interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateResource", reflect.TypeOf((*MockReconciler)(nil).CreateResource), ctx, spec, serviceName) +} + +// DeleteResource mocks base method. +func (m *MockReconciler) DeleteResource(ctx context.Context, spec azure0.ResourceSpecGetter, serviceName string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeleteResource", ctx, spec, serviceName) + ret0, _ := ret[0].(error) + return ret0 +} + +// DeleteResource indicates an expected call of DeleteResource. +func (mr *MockReconcilerMockRecorder) DeleteResource(ctx, spec, serviceName interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteResource", reflect.TypeOf((*MockReconciler)(nil).DeleteResource), ctx, spec, serviceName) +} diff --git a/azure/services/disks/client.go b/azure/services/disks/client.go index de210aedea7..85ae1c0fbd1 100644 --- a/azure/services/disks/client.go +++ b/azure/services/disks/client.go @@ -29,21 +29,12 @@ import ( "sigs.k8s.io/cluster-api-provider-azure/util/tele" ) -// client wraps go-sdk. -type client interface { - DeleteAsync(context.Context, azure.ResourceSpecGetter) (azureautorest.FutureAPI, error) - Result(context.Context, azureautorest.FutureAPI, string) (interface{}, error) - IsDone(context.Context, azureautorest.FutureAPI) (bool, error) -} - // azureClient contains the Azure go-sdk Client. type azureClient struct { disks compute.DisksClient } -var _ client = (*azureClient)(nil) - -// newClient creates a new VM Client from subscription ID. +// newClient creates a new disk Client from subscription ID. func newClient(auth azure.Authorizer) *azureClient { c := NewDisksClient(auth.SubscriptionID(), auth.BaseURI(), auth.Authorizer()) return &azureClient{c} diff --git a/azure/services/disks/disks.go b/azure/services/disks/disks.go index a37b4e1653d..516f8030c79 100644 --- a/azure/services/disks/disks.go +++ b/azure/services/disks/disks.go @@ -38,14 +38,15 @@ type DiskScope interface { // Service provides operations on Azure resources. type Service struct { Scope DiskScope - client + async.Reconciler } // New creates a new disks service. func New(scope DiskScope) *Service { + client := newClient(scope) return &Service{ - Scope: scope, - client: newClient(scope), + Scope: scope, + Reconciler: async.New(scope, nil, client), } } @@ -72,7 +73,7 @@ func (s *Service) Delete(ctx context.Context) error { // If multiple errors occur, we return the most pressing one // order of precedence is: error deleting -> deleting in progress -> deleted (no error) for _, diskSpec := range s.Scope.DiskSpecs() { - if err := async.DeleteResource(ctx, s.Scope, s.client, diskSpec, serviceName); err != nil { + if err := s.DeleteResource(ctx, diskSpec, serviceName); err != nil { if !azure.IsOperationNotDoneError(err) || result == nil { result = err } diff --git a/azure/services/disks/disks_test.go b/azure/services/disks/disks_test.go index 2e170aa5cea..a695b058b9d 100644 --- a/azure/services/disks/disks_test.go +++ b/azure/services/disks/disks_test.go @@ -18,7 +18,6 @@ package disks import ( "context" - "fmt" "net/http" "testing" @@ -28,6 +27,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/disks/mock_disks" gomockinternal "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock" ) @@ -49,25 +49,22 @@ var ( } internalError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error") - notFoundError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not Found") ) func TestDeleteDisk(t *testing.T) { testcases := []struct { name string expectedError string - expect func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockclientMockRecorder) + expect func(s *mock_disks.MockDiskScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ { name: "delete the disk", expectedError: "", - expect: func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockclientMockRecorder) { + expect: func(s *mock_disks.MockDiskScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.DiskSpecs().Return(fakeDiskSpecs) gomock.InOrder( - s.GetLongRunningOperationState("my-disk-1", serviceName), - m.DeleteAsync(gomockinternal.AContext(), &diskSpec1).Return(nil, nil), - s.GetLongRunningOperationState("my-disk-2", serviceName), - m.DeleteAsync(gomockinternal.AContext(), &diskSpec2).Return(nil, nil), + r.DeleteResource(gomockinternal.AContext(), &diskSpec1, serviceName).Return(nil), + r.DeleteResource(gomockinternal.AContext(), &diskSpec2, serviceName).Return(nil), s.UpdateDeleteStatus(infrav1.DisksReadyCondition, serviceName, nil), ) }, @@ -75,28 +72,24 @@ func TestDeleteDisk(t *testing.T) { { name: "disk already deleted", expectedError: "", - expect: func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockclientMockRecorder) { + expect: func(s *mock_disks.MockDiskScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.DiskSpecs().Return(fakeDiskSpecs) gomock.InOrder( - s.GetLongRunningOperationState("my-disk-1", serviceName), - m.DeleteAsync(gomockinternal.AContext(), &diskSpec1).Return(nil, notFoundError), - s.GetLongRunningOperationState("my-disk-2", serviceName), - m.DeleteAsync(gomockinternal.AContext(), &diskSpec2).Return(nil, nil), + r.DeleteResource(gomockinternal.AContext(), &diskSpec1, serviceName).Return(nil), + r.DeleteResource(gomockinternal.AContext(), &diskSpec2, serviceName).Return(nil), s.UpdateDeleteStatus(infrav1.DisksReadyCondition, serviceName, nil), ) }, }, { name: "error while trying to delete the disk", - expectedError: "failed to delete resource my-group/my-disk-1 (service: disks): #: Internal Server Error: StatusCode=500", - expect: func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockclientMockRecorder) { + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(s *mock_disks.MockDiskScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.DiskSpecs().Return(fakeDiskSpecs) gomock.InOrder( - s.GetLongRunningOperationState("my-disk-1", serviceName), - m.DeleteAsync(gomockinternal.AContext(), &diskSpec1).Return(nil, internalError), - s.GetLongRunningOperationState("my-disk-2", serviceName), - m.DeleteAsync(gomockinternal.AContext(), &diskSpec2).Return(nil, nil), - s.UpdateDeleteStatus(infrav1.DisksReadyCondition, serviceName, gomockinternal.ErrStrEq(fmt.Sprintf("failed to delete resource my-group/my-disk-1 (service: disks): %s", internalError.Error()))), + r.DeleteResource(gomockinternal.AContext(), &diskSpec1, serviceName).Return(internalError), + r.DeleteResource(gomockinternal.AContext(), &diskSpec2, serviceName).Return(nil), + s.UpdateDeleteStatus(infrav1.DisksReadyCondition, serviceName, internalError), ) }, }, @@ -111,13 +104,13 @@ func TestDeleteDisk(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() scopeMock := mock_disks.NewMockDiskScope(mockCtrl) - clientMock := mock_disks.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/disks/mock_disks/client_mock.go b/azure/services/disks/mock_disks/client_mock.go deleted file mode 100644 index c896a6f4e20..00000000000 --- a/azure/services/disks/mock_disks/client_mock.go +++ /dev/null @@ -1,98 +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_disks is a generated GoMock package. -package mock_disks - -import ( - context "context" - reflect "reflect" - - 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 -} - -// 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) -} - -// 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/disks/mock_disks/doc.go b/azure/services/disks/mock_disks/doc.go index 61ae82ad0b5..a6140f92e14 100644 --- a/azure/services/disks/mock_disks/doc.go +++ b/azure/services/disks/mock_disks/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_disks -source ../client.go client //go:generate ../../../../hack/tools/bin/mockgen -destination disks_mock.go -package mock_disks -source ../disks.go DiskScope -//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 disks_mock.go > _disks_mock.go && mv _disks_mock.go disks_mock.go" package mock_disks //nolint diff --git a/azure/services/groups/client.go b/azure/services/groups/client.go index 1ac851eeb8b..447537776da 100644 --- a/azure/services/groups/client.go +++ b/azure/services/groups/client.go @@ -31,8 +31,8 @@ import ( // client wraps go-sdk. type client interface { - Get(context.Context, string) (resources.Group, error) - CreateOrUpdateAsync(context.Context, azure.ResourceSpecGetter) (interface{}, azureautorest.FutureAPI, error) + Get(context.Context, azure.ResourceSpecGetter) (interface{}, error) + CreateOrUpdateAsync(context.Context, azure.ResourceSpecGetter, interface{}) (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) @@ -61,28 +61,25 @@ func newGroupsClient(subscriptionID string, baseURI string, authorizer autorest. } // Get gets a resource group. -func (ac *azureClient) Get(ctx context.Context, name string) (resources.Group, error) { +func (ac *azureClient) Get(ctx context.Context, spec azure.ResourceSpecGetter) (interface{}, error) { ctx, _, done := tele.StartSpanWithLogger(ctx, "groups.AzureClient.Get") defer done() - return ac.groups.Get(ctx, name) + return ac.groups.Get(ctx, spec.ResourceName()) } // CreateOrUpdateAsync creates or updates a resource group. // Creating a resource group is not a long running operation, so we don't ever return a future. -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, "groups.AzureClient.CreateOrUpdate") defer done() - group, err := ac.resourceGroupParams(ctx, spec) - if err != nil { - return nil, nil, errors.Wrapf(err, "failed to get desired parameters for group %s", spec.ResourceName()) - } else if group == nil { - // nothing to do here - return nil, nil, nil + group, ok := parameters.(resources.Group) + if !ok { + return nil, nil, errors.Errorf("%T is not a resources.Group", parameters) } - result, err := ac.groups.CreateOrUpdate(ctx, spec.ResourceName(), *group) + result, err := ac.groups.CreateOrUpdate(ctx, spec.ResourceName(), group) return result, nil, err } @@ -132,39 +129,3 @@ func (ac *azureClient) Result(ctx context.Context, futureData azureautorest.Futu // Result is a no-op for resource groups as only Delete operations return a future. return nil, nil } - -// resourceGroupParams returns the desired resource group parameters from the given spec. -func (ac *azureClient) resourceGroupParams(ctx context.Context, spec azure.ResourceSpecGetter) (*resources.Group, error) { - ctx, _, done := tele.StartSpanWithLogger(ctx, "groups.AzureClient.resourceGroupParams") - defer done() - - var params interface{} - - existingRG, err := ac.Get(ctx, spec.ResourceName()) - if azure.ResourceNotFound(err) { - // rg doesn't exist, create it from scratch. - params, err = spec.Parameters(nil) - if err != nil { - return nil, err - } - } else if err != nil { - return nil, errors.Wrapf(err, "failed to get RG %s", spec.ResourceName()) - } else { - // rg already exists - params, err = spec.Parameters(existingRG) - if err != nil { - return nil, err - } - } - - rg, ok := params.(resources.Group) - if !ok { - if params == nil { - // nothing to do here. - return nil, nil - } - return nil, errors.Errorf("%T is not a resources.Group", params) - } - - return &rg, nil -} diff --git a/azure/services/groups/groups.go b/azure/services/groups/groups.go index 7f3d33deb95..b89ee308b9e 100644 --- a/azure/services/groups/groups.go +++ b/azure/services/groups/groups.go @@ -19,6 +19,7 @@ package groups import ( "context" + "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2019-05-01/resources" "github.com/pkg/errors" infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" "sigs.k8s.io/cluster-api-provider-azure/azure" @@ -33,6 +34,7 @@ const serviceName = "group" // Service provides operations on Azure resources. type Service struct { Scope GroupScope + async.Reconciler client } @@ -46,9 +48,11 @@ type GroupScope interface { // New creates a new service. func New(scope GroupScope) *Service { + client := newClient(scope) return &Service{ - Scope: scope, - client: newClient(scope), + Scope: scope, + client: client, + Reconciler: async.New(scope, client, client), } } @@ -62,7 +66,7 @@ func (s *Service) Reconcile(ctx context.Context) error { groupSpec := s.Scope.GroupSpec() - _, err := async.CreateResource(ctx, s.Scope, s.client, groupSpec, serviceName) + _, err := s.CreateResource(ctx, groupSpec, serviceName) s.Scope.UpdatePutStatus(infrav1.ResourceGroupReadyCondition, serviceName, err) return err } @@ -93,7 +97,7 @@ func (s *Service) Delete(ctx context.Context) error { return azure.ErrNotOwned } - err = async.DeleteResource(ctx, s.Scope, s.client, groupSpec, serviceName) + err = s.DeleteResource(ctx, groupSpec, serviceName) s.Scope.UpdateDeleteStatus(infrav1.ResourceGroupReadyCondition, serviceName, err) return err } @@ -105,10 +109,15 @@ func (s *Service) IsGroupManaged(ctx context.Context) (bool, error) { defer done() groupSpec := s.Scope.GroupSpec() - group, err := s.client.Get(ctx, groupSpec.ResourceName()) + groupIface, err := s.client.Get(ctx, groupSpec) if err != nil { return false, err } + group, ok := groupIface.(resources.Group) + if !ok { + return false, errors.Errorf("%T is not a resources.Group", groupIface) + } + tags := converters.MapToTags(group.Tags) return tags.HasOwned(s.Scope.ClusterName()), nil } diff --git a/azure/services/groups/groups_test.go b/azure/services/groups/groups_test.go index 2cd47e02d88..30f8d5d8a52 100644 --- a/azure/services/groups/groups_test.go +++ b/azure/services/groups/groups_test.go @@ -18,19 +18,18 @@ package groups import ( "context" - "errors" - "fmt" + "net/http" "testing" "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2019-05-01/resources" "github.com/Azure/go-autorest/autorest" - azureautorest "github.com/Azure/go-autorest/autorest/azure" "github.com/Azure/go-autorest/autorest/to" "github.com/golang/mock/gomock" . "github.com/onsi/gomega" 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/groups/mock_groups" gomockinternal "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock" ) @@ -42,16 +41,8 @@ var ( ClusterName: "test-cluster", AdditionalTags: map[string]string{"foo": "bar"}, } - internalError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error") - notFoundError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not Found") - errCtxExceeded = errors.New("ctx exceeded") - fakeFuture = infrav1.Future{ - Type: infrav1.DeleteFuture, - ServiceName: serviceName, - Name: "test-group", - ResourceGroup: "test-group", - Data: "eyJtZXRob2QiOiJERUxFVEUiLCJwb2xsaW5nTWV0aG9kIjoiTG9jYXRpb24iLCJscm9TdGF0ZSI6IkluUHJvZ3Jlc3MifQ==", - } + internalError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error") + notFoundError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not Found") sampleManagedGroup = resources.Group{ Name: to.StringPtr("test-group"), Location: to.StringPtr("test-location"), @@ -70,26 +61,24 @@ func TestReconcileGroups(t *testing.T) { testcases := []struct { name string expectedError string - expect func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder) + expect func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ { name: "create group succeeds", expectedError: "", - expect: func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder) { + expect: func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.GroupSpec().Return(&fakeGroupSpec) - s.GetLongRunningOperationState("test-group", serviceName) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakeGroupSpec).Return(nil, nil, nil) + r.CreateResource(gomockinternal.AContext(), &fakeGroupSpec, serviceName).Return(nil, nil) s.UpdatePutStatus(infrav1.ResourceGroupReadyCondition, serviceName, nil) }, }, { name: "create resource group fails", - expectedError: "failed to create resource test-group/test-group (service: group): #: Internal Server Error: StatusCode=500", - expect: func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder) { + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.GroupSpec().Return(&fakeGroupSpec) - s.GetLongRunningOperationState("test-group", serviceName) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakeGroupSpec).Return(nil, nil, internalError) - s.UpdatePutStatus(infrav1.ResourceGroupReadyCondition, serviceName, gomockinternal.ErrStrEq(fmt.Sprintf("failed to create resource test-group/test-group (service: group): %s", internalError.Error()))) + r.CreateResource(gomockinternal.AContext(), &fakeGroupSpec, serviceName).Return(nil, internalError) + s.UpdatePutStatus(infrav1.ResourceGroupReadyCondition, serviceName, internalError) }, }, } @@ -104,12 +93,14 @@ func TestReconcileGroups(t *testing.T) { defer mockCtrl.Finish() scopeMock := mock_groups.NewMockGroupScope(mockCtrl) clientMock := mock_groups.NewMockclient(mockCtrl) + asyncMock := mock_async.NewMockReconciler(mockCtrl) - tc.expect(scopeMock.EXPECT(), clientMock.EXPECT()) + tc.expect(scopeMock.EXPECT(), clientMock.EXPECT(), asyncMock.EXPECT()) s := &Service{ - Scope: scopeMock, - client: clientMock, + Scope: scopeMock, + client: clientMock, + Reconciler: asyncMock, } err := s.Reconcile(context.TODO()) @@ -127,96 +118,55 @@ func TestDeleteGroups(t *testing.T) { testcases := []struct { name string expectedError string - expect func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder) + expect func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ { - name: "long running delete operation is done", + name: "delete operation is successful for managed resource group", expectedError: "", - expect: func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder) { + expect: func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.GroupSpec().AnyTimes().Return(&fakeGroupSpec) - m.Get(gomockinternal.AContext(), "test-group").Return(sampleManagedGroup, nil) + m.Get(gomockinternal.AContext(), &fakeGroupSpec).Return(sampleManagedGroup, nil) s.ClusterName().Return("test-cluster") - s.GetLongRunningOperationState("test-group", serviceName).Times(2).Return(&fakeFuture) - m.IsDone(gomockinternal.AContext(), gomock.AssignableToTypeOf(&azureautorest.Future{})).Return(true, nil) - m.Result(gomockinternal.AContext(), gomock.AssignableToTypeOf(&azureautorest.Future{}), infrav1.DeleteFuture).Return(nil, nil) - s.DeleteLongRunningOperationState("test-group", serviceName) + r.DeleteResource(gomockinternal.AContext(), &fakeGroupSpec, serviceName).Return(nil) s.UpdateDeleteStatus(infrav1.ResourceGroupReadyCondition, serviceName, nil) }, }, - { - name: "long running delete operation is not done", - expectedError: "operation type DELETE on Azure resource test-group/test-group is not done. Object will be requeued after 15s", - expect: func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder) { - s.GroupSpec().AnyTimes().Return(&fakeGroupSpec) - m.Get(gomockinternal.AContext(), "test-group").Return(sampleManagedGroup, nil) - s.ClusterName().Return("test-cluster") - s.GetLongRunningOperationState("test-group", serviceName).Times(2).Return(&fakeFuture) - m.IsDone(gomockinternal.AContext(), gomock.AssignableToTypeOf(&azureautorest.Future{})).Return(false, nil) - s.UpdateDeleteStatus(infrav1.ResourceGroupReadyCondition, serviceName, gomockinternal.ErrStrEq("operation type DELETE on Azure resource test-group/test-group is not done. Object will be requeued after 15s")) - }, - }, { name: "resource group is not managed by capz", expectedError: azure.ErrNotOwned.Error(), - expect: func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder) { + expect: func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.GroupSpec().AnyTimes().Return(&fakeGroupSpec) - m.Get(gomockinternal.AContext(), "test-group").Return(sampleBYOGroup, nil) + m.Get(gomockinternal.AContext(), &fakeGroupSpec).Return(sampleBYOGroup, nil) s.ClusterName().Return("test-cluster") }, }, { name: "fail to check if resource group is managed", expectedError: "could not get resource group management state", - expect: func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder) { + expect: func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.GroupSpec().AnyTimes().Return(&fakeGroupSpec) - m.Get(gomockinternal.AContext(), "test-group").Return(resources.Group{}, internalError) + m.Get(gomockinternal.AContext(), &fakeGroupSpec).Return(resources.Group{}, internalError) }, }, { name: "resource group doesn't exist", expectedError: "", - expect: func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder) { + expect: func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.GroupSpec().AnyTimes().Return(&fakeGroupSpec) - m.Get(gomockinternal.AContext(), "test-group").Return(resources.Group{}, notFoundError) + m.Get(gomockinternal.AContext(), &fakeGroupSpec).Return(resources.Group{}, notFoundError) s.DeleteLongRunningOperationState("test-group", serviceName) s.UpdateDeleteStatus(infrav1.ResourceGroupReadyCondition, serviceName, nil) }, }, { name: "error occurs when deleting resource group", - expectedError: "failed to delete resource test-group/test-group (service: group): #: Internal Server Error: StatusCode=500", - expect: func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder) { - s.GroupSpec().AnyTimes().Return(&fakeGroupSpec) - s.GetLongRunningOperationState("test-group", serviceName).Return(nil) - m.Get(gomockinternal.AContext(), "test-group").Return(sampleManagedGroup, nil) - s.ClusterName().Return("test-cluster") - m.DeleteAsync(gomockinternal.AContext(), &fakeGroupSpec).Return(nil, internalError) - s.UpdateDeleteStatus(infrav1.ResourceGroupReadyCondition, serviceName, gomockinternal.ErrStrEq("failed to delete resource test-group/test-group (service: group): #: Internal Server Error: StatusCode=500")) - }, - }, - { - name: "context deadline exceeded while deleting resource group", - expectedError: "operation type DELETE on Azure resource test-group/test-group is not done. Object will be requeued after 15s", - expect: func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder) { - s.GroupSpec().AnyTimes().Return(&fakeGroupSpec) - s.GetLongRunningOperationState("test-group", serviceName).Return(nil) - m.Get(gomockinternal.AContext(), "test-group").Return(sampleManagedGroup, nil) - s.ClusterName().Return("test-cluster") - m.DeleteAsync(gomockinternal.AContext(), &fakeGroupSpec).Return(&azureautorest.Future{}, errCtxExceeded) - s.SetLongRunningOperationState(gomock.AssignableToTypeOf(&infrav1.Future{})) - s.UpdateDeleteStatus(infrav1.ResourceGroupReadyCondition, serviceName, gomockinternal.ErrStrEq("operation type DELETE on Azure resource test-group/test-group is not done. Object will be requeued after 15s")) - }, - }, - { - name: "delete the resource group successfully", - expectedError: "", - expect: func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder) { + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.GroupSpec().AnyTimes().Return(&fakeGroupSpec) - s.GetLongRunningOperationState("test-group", serviceName).Return(nil) - m.Get(gomockinternal.AContext(), "test-group").Return(sampleManagedGroup, nil) + m.Get(gomockinternal.AContext(), &fakeGroupSpec).Return(sampleManagedGroup, nil) s.ClusterName().Return("test-cluster") - m.DeleteAsync(gomockinternal.AContext(), &fakeGroupSpec).Return(nil, nil) - s.UpdateDeleteStatus(infrav1.ResourceGroupReadyCondition, serviceName, nil) + r.DeleteResource(gomockinternal.AContext(), &fakeGroupSpec, serviceName).Return(internalError) + s.UpdateDeleteStatus(infrav1.ResourceGroupReadyCondition, serviceName, gomockinternal.ErrStrEq("#: Internal Server Error: StatusCode=500")) }, }, } @@ -231,12 +181,14 @@ func TestDeleteGroups(t *testing.T) { defer mockCtrl.Finish() scopeMock := mock_groups.NewMockGroupScope(mockCtrl) clientMock := mock_groups.NewMockclient(mockCtrl) + asyncMock := mock_async.NewMockReconciler(mockCtrl) - tc.expect(scopeMock.EXPECT(), clientMock.EXPECT()) + tc.expect(scopeMock.EXPECT(), clientMock.EXPECT(), asyncMock.EXPECT()) s := &Service{ - Scope: scopeMock, - client: clientMock, + Scope: scopeMock, + client: clientMock, + Reconciler: asyncMock, } err := s.Delete(context.TODO()) diff --git a/azure/services/groups/mock_groups/client_mock.go b/azure/services/groups/mock_groups/client_mock.go index f18a9c84c35..77b530db5e3 100644 --- a/azure/services/groups/mock_groups/client_mock.go +++ b/azure/services/groups/mock_groups/client_mock.go @@ -24,7 +24,6 @@ import ( context "context" reflect "reflect" - resources "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2019-05-01/resources" azure "github.com/Azure/go-autorest/autorest/azure" gomock "github.com/golang/mock/gomock" azure0 "sigs.k8s.io/cluster-api-provider-azure/azure" @@ -54,9 +53,9 @@ func (m *Mockclient) EXPECT() *MockclientMockRecorder { } // CreateOrUpdateAsync mocks base method. -func (m *Mockclient) CreateOrUpdateAsync(arg0 context.Context, arg1 azure0.ResourceSpecGetter) (interface{}, azure.FutureAPI, error) { +func (m *Mockclient) CreateOrUpdateAsync(arg0 context.Context, arg1 azure0.ResourceSpecGetter, arg2 interface{}) (interface{}, azure.FutureAPI, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateOrUpdateAsync", arg0, arg1) + ret := m.ctrl.Call(m, "CreateOrUpdateAsync", arg0, arg1, arg2) ret0, _ := ret[0].(interface{}) ret1, _ := ret[1].(azure.FutureAPI) ret2, _ := ret[2].(error) @@ -64,9 +63,9 @@ func (m *Mockclient) CreateOrUpdateAsync(arg0 context.Context, arg1 azure0.Resou } // CreateOrUpdateAsync indicates an expected call of CreateOrUpdateAsync. -func (mr *MockclientMockRecorder) CreateOrUpdateAsync(arg0, arg1 interface{}) *gomock.Call { +func (mr *MockclientMockRecorder) CreateOrUpdateAsync(arg0, arg1, arg2 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdateAsync", reflect.TypeOf((*Mockclient)(nil).CreateOrUpdateAsync), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdateAsync", reflect.TypeOf((*Mockclient)(nil).CreateOrUpdateAsync), arg0, arg1, arg2) } // DeleteAsync mocks base method. @@ -85,10 +84,10 @@ func (mr *MockclientMockRecorder) DeleteAsync(arg0, arg1 interface{}) *gomock.Ca } // Get mocks base method. -func (m *Mockclient) Get(arg0 context.Context, arg1 string) (resources.Group, error) { +func (m *Mockclient) Get(arg0 context.Context, arg1 azure0.ResourceSpecGetter) (interface{}, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Get", arg0, arg1) - ret0, _ := ret[0].(resources.Group) + ret0, _ := ret[0].(interface{}) ret1, _ := ret[1].(error) return ret0, ret1 } diff --git a/azure/services/roleassignments/roleassignments.go b/azure/services/roleassignments/roleassignments.go index 5514c70b298..2744b0fe452 100644 --- a/azure/services/roleassignments/roleassignments.go +++ b/azure/services/roleassignments/roleassignments.go @@ -21,6 +21,7 @@ import ( "fmt" "github.com/Azure/azure-sdk-for-go/profiles/2019-03-01/authorization/mgmt/authorization" + "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-04-01/compute" "github.com/Azure/go-autorest/autorest/to" "github.com/pkg/errors" @@ -79,10 +80,19 @@ func (s *Service) reconcileVM(ctx context.Context, roleSpec azure.RoleAssignment ctx, log, done := tele.StartSpanWithLogger(ctx, "roleassignments.Service.reconcileVM") defer done() - resultVM, err := s.virtualMachinesClient.Get(ctx, s.Scope.ResourceGroup(), roleSpec.MachineName) + spec := &virtualmachines.VMSpec{ + Name: roleSpec.MachineName, + ResourceGroup: s.Scope.ResourceGroup(), + } + + resultVMIface, err := s.virtualMachinesClient.Get(ctx, spec) if err != nil { return errors.Wrap(err, "cannot get VM to assign role to system assigned identity") } + resultVM, ok := resultVMIface.(compute.VirtualMachine) + if !ok { + return errors.Errorf("%T is not a compute.VirtualMachine", resultVMIface) + } err = s.assignRole(ctx, roleSpec.Name, resultVM.Identity.PrincipalID) if err != nil { diff --git a/azure/services/roleassignments/roleassignments_test.go b/azure/services/roleassignments/roleassignments_test.go index 82e14f6b246..2b223ef9fee 100644 --- a/azure/services/roleassignments/roleassignments_test.go +++ b/azure/services/roleassignments/roleassignments_test.go @@ -30,10 +30,18 @@ import ( "sigs.k8s.io/cluster-api-provider-azure/azure" "sigs.k8s.io/cluster-api-provider-azure/azure/services/roleassignments/mock_roleassignments" "sigs.k8s.io/cluster-api-provider-azure/azure/services/scalesets/mock_scalesets" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/virtualmachines" "sigs.k8s.io/cluster-api-provider-azure/azure/services/virtualmachines/mock_virtualmachines" gomockinternal "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock" ) +var ( + fakeVMSpec = virtualmachines.VMSpec{ + Name: "test-vm", + ResourceGroup: "my-rg", + } +) + func TestReconcileRoleAssignmentsVM(t *testing.T) { testcases := []struct { name string @@ -52,7 +60,7 @@ func TestReconcileRoleAssignmentsVM(t *testing.T) { ResourceType: azure.VirtualMachine, }, }) - v.Get(gomockinternal.AContext(), "my-rg", "test-vm").Return(compute.VirtualMachine{ + v.Get(gomockinternal.AContext(), &fakeVMSpec).Return(compute.VirtualMachine{ Identity: &compute.VirtualMachineIdentity{ PrincipalID: to.StringPtr("000"), }, @@ -77,7 +85,7 @@ func TestReconcileRoleAssignmentsVM(t *testing.T) { ResourceType: azure.VirtualMachine, }, }) - v.Get(gomockinternal.AContext(), "my-rg", "test-vm").Return(compute.VirtualMachine{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")) + v.Get(gomockinternal.AContext(), &fakeVMSpec).Return(compute.VirtualMachine{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")) }, }, { @@ -92,7 +100,7 @@ func TestReconcileRoleAssignmentsVM(t *testing.T) { ResourceType: azure.VirtualMachine, }, }) - v.Get(gomockinternal.AContext(), "my-rg", "test-vm").Return(compute.VirtualMachine{ + v.Get(gomockinternal.AContext(), &fakeVMSpec).Return(compute.VirtualMachine{ Identity: &compute.VirtualMachineIdentity{ PrincipalID: to.StringPtr("000"), }, diff --git a/azure/services/virtualmachines/client.go b/azure/services/virtualmachines/client.go index d4400f02c4c..f4f77618c0f 100644 --- a/azure/services/virtualmachines/client.go +++ b/azure/services/virtualmachines/client.go @@ -35,8 +35,8 @@ import ( // Client wraps go-sdk. type ( Client interface { - Get(context.Context, string, string) (compute.VirtualMachine, error) - CreateOrUpdateAsync(context.Context, azure.ResourceSpecGetter) (interface{}, azureautorest.FutureAPI, error) + Get(context.Context, azure.ResourceSpecGetter) (interface{}, error) + CreateOrUpdateAsync(context.Context, azure.ResourceSpecGetter, interface{}) (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) @@ -64,40 +64,23 @@ func newVirtualMachinesClient(subscriptionID string, baseURI string, authorizer } // Get retrieves information about the model view or the instance view of a virtual machine. -func (ac *AzureClient) Get(ctx context.Context, resourceGroupName, vmName string) (compute.VirtualMachine, error) { +func (ac *AzureClient) Get(ctx context.Context, spec azure.ResourceSpecGetter) (interface{}, error) { ctx, _, done := tele.StartSpanWithLogger(ctx, "virtualmachines.AzureClient.Get") defer done() - return ac.virtualmachines.Get(ctx, resourceGroupName, vmName, "") + return ac.virtualmachines.Get(ctx, spec.ResourceGroupName(), spec.ResourceName(), "") } // CreateOrUpdateAsync creates or updates a virtual machine 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, "virtualmachines.AzureClient.CreateOrUpdate") defer done() - var existingVM 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 virtual machine %s in %s", spec.ResourceName(), spec.ResourceGroupName()) - } else if err == nil { - existingVM = existing - } - - params, err := spec.Parameters(existingVM) - if err != nil { - return nil, nil, errors.Wrapf(err, "failed to get desired parameters for virtual machine %s", spec.ResourceName()) - } - - vm, ok := params.(compute.VirtualMachine) + vm, ok := parameters.(compute.VirtualMachine) if !ok { - if params == nil { - // nothing to do here. - return existingVM, nil, nil - } - return nil, nil, errors.Errorf("%T is not a compute.VirtualMachine", params) + return nil, nil, errors.Errorf("%T is not a compute.VirtualMachine", parameters) } future, err := ac.virtualmachines.CreateOrUpdate(ctx, spec.ResourceGroupName(), spec.ResourceName(), vm) @@ -149,19 +132,22 @@ func (ac *AzureClient) DeleteAsync(ctx context.Context, spec azure.ResourceSpecG // IsDone returns true if the long-running operation has completed. func (ac *AzureClient) IsDone(ctx context.Context, future azureautorest.FutureAPI) (bool, error) { - ctx, span := tele.Tracer().Start(ctx, "virtualmachines.AzureClient.IsDone") - defer span.End() + ctx, _, done := tele.StartSpanWithLogger(ctx, "virtualmachines.AzureClient.IsDone") + defer done() - done, err := future.DoneWithContext(ctx, ac.virtualmachines) + isDone, err := future.DoneWithContext(ctx, ac.virtualmachines) if err != nil { return false, errors.Wrap(err, "failed checking if the operation was complete") } - return done, nil + 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, "virtualmachines.AzureClient.Result") + defer done() + if futureData == nil { return nil, errors.Errorf("cannot get result from nil future") } diff --git a/azure/services/virtualmachines/mock_virtualmachines/client_mock.go b/azure/services/virtualmachines/mock_virtualmachines/client_mock.go index 2410527fb14..8236de64ea2 100644 --- a/azure/services/virtualmachines/mock_virtualmachines/client_mock.go +++ b/azure/services/virtualmachines/mock_virtualmachines/client_mock.go @@ -24,7 +24,6 @@ import ( context "context" reflect "reflect" - compute "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-04-01/compute" azure "github.com/Azure/go-autorest/autorest/azure" gomock "github.com/golang/mock/gomock" azure0 "sigs.k8s.io/cluster-api-provider-azure/azure" @@ -54,9 +53,9 @@ func (m *MockClient) EXPECT() *MockClientMockRecorder { } // CreateOrUpdateAsync mocks base method. -func (m *MockClient) CreateOrUpdateAsync(arg0 context.Context, arg1 azure0.ResourceSpecGetter) (interface{}, azure.FutureAPI, error) { +func (m *MockClient) CreateOrUpdateAsync(arg0 context.Context, arg1 azure0.ResourceSpecGetter, arg2 interface{}) (interface{}, azure.FutureAPI, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateOrUpdateAsync", arg0, arg1) + ret := m.ctrl.Call(m, "CreateOrUpdateAsync", arg0, arg1, arg2) ret0, _ := ret[0].(interface{}) ret1, _ := ret[1].(azure.FutureAPI) ret2, _ := ret[2].(error) @@ -64,9 +63,9 @@ func (m *MockClient) CreateOrUpdateAsync(arg0 context.Context, arg1 azure0.Resou } // CreateOrUpdateAsync indicates an expected call of CreateOrUpdateAsync. -func (mr *MockClientMockRecorder) CreateOrUpdateAsync(arg0, arg1 interface{}) *gomock.Call { +func (mr *MockClientMockRecorder) CreateOrUpdateAsync(arg0, arg1, arg2 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdateAsync", reflect.TypeOf((*MockClient)(nil).CreateOrUpdateAsync), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdateAsync", reflect.TypeOf((*MockClient)(nil).CreateOrUpdateAsync), arg0, arg1, arg2) } // DeleteAsync mocks base method. @@ -85,18 +84,18 @@ func (mr *MockClientMockRecorder) DeleteAsync(arg0, arg1 interface{}) *gomock.Ca } // Get mocks base method. -func (m *MockClient) Get(arg0 context.Context, arg1, arg2 string) (compute.VirtualMachine, error) { +func (m *MockClient) Get(arg0 context.Context, arg1 azure0.ResourceSpecGetter) (interface{}, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Get", arg0, arg1, arg2) - ret0, _ := ret[0].(compute.VirtualMachine) + ret := m.ctrl.Call(m, "Get", arg0, arg1) + ret0, _ := ret[0].(interface{}) ret1, _ := ret[1].(error) return ret0, ret1 } // Get indicates an expected call of Get. -func (mr *MockClientMockRecorder) Get(arg0, arg1, arg2 interface{}) *gomock.Call { +func (mr *MockClientMockRecorder) Get(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockClient)(nil).Get), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockClient)(nil).Get), arg0, arg1) } // IsDone mocks base method. diff --git a/azure/services/virtualmachines/virtualmachines.go b/azure/services/virtualmachines/virtualmachines.go index 4b6452a4825..906bcb01b3f 100644 --- a/azure/services/virtualmachines/virtualmachines.go +++ b/azure/services/virtualmachines/virtualmachines.go @@ -52,7 +52,7 @@ type VMScope interface { // Service provides operations on Azure resources. type Service struct { Scope VMScope - Client + async.Reconciler interfacesClient networkinterfaces.Client publicIPsClient publicips.Client availabilitySetsClient availabilitysets.Client @@ -60,12 +60,13 @@ type Service struct { // New creates a new service. func New(scope VMScope) *Service { + Client := NewClient(scope) return &Service{ Scope: scope, - Client: NewClient(scope), interfacesClient: networkinterfaces.NewClient(scope), publicIPsClient: publicips.NewClient(scope), availabilitySetsClient: availabilitysets.NewClient(scope), + Reconciler: async.New(scope, Client, Client), } } @@ -78,7 +79,8 @@ func (s *Service) Reconcile(ctx context.Context) error { defer cancel() vmSpec := s.Scope.VMSpec() - result, err := async.CreateResource(ctx, s.Scope, s.Client, vmSpec, serviceName) + + result, err := s.CreateResource(ctx, vmSpec, serviceName) s.Scope.UpdatePutStatus(infrav1.VMRunningCondition, serviceName, err) // Set the DiskReady condition here since the disk gets created with the VM. s.Scope.UpdatePutStatus(infrav1.DisksReadyCondition, serviceName, err) @@ -114,7 +116,8 @@ func (s *Service) Delete(ctx context.Context) error { defer cancel() vmSpec := s.Scope.VMSpec() - err := async.DeleteResource(ctx, s.Scope, s.Client, vmSpec, serviceName) + + err := s.DeleteResource(ctx, vmSpec, serviceName) if err != nil { s.Scope.SetVMState(infrav1.Deleting) } else { diff --git a/azure/services/virtualmachines/virtualmachines_test.go b/azure/services/virtualmachines/virtualmachines_test.go index 2dc1f38e849..998b8dc1a70 100644 --- a/azure/services/virtualmachines/virtualmachines_test.go +++ b/azure/services/virtualmachines/virtualmachines_test.go @@ -18,20 +18,18 @@ package virtualmachines import ( "context" - "fmt" "net/http" "testing" "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-04-01/compute" "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/Azure/go-autorest/autorest/to" "github.com/golang/mock/gomock" . "github.com/onsi/gomega" - "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/async/mock_async" "sigs.k8s.io/cluster-api-provider-azure/azure/services/availabilitysets/mock_availabilitysets" "sigs.k8s.io/cluster-api-provider-azure/azure/services/networkinterfaces/mock_networkinterfaces" "sigs.k8s.io/cluster-api-provider-azure/azure/services/publicips/mock_publicips" @@ -55,88 +53,108 @@ var ( Image: &infrav1.Image{ID: to.StringPtr("fake-image-id")}, BootstrapData: "fake data", } - fakeFuture = infrav1.Future{ - Type: infrav1.DeleteFuture, - ServiceName: serviceName, - Name: "test-vm", - ResourceGroup: "test-group", - Data: "eyJtZXRob2QiOiJERUxFVEUiLCJwb2xsaW5nTWV0aG9kIjoiTG9jYXRpb24iLCJscm9TdGF0ZSI6IkluUHJvZ3Jlc3MifQ==", + fakeExistingVM = compute.VirtualMachine{ + ID: to.StringPtr("test-vm-id"), + VirtualMachineProperties: &compute.VirtualMachineProperties{ + ProvisioningState: to.StringPtr("Succeeded"), + NetworkProfile: &compute.NetworkProfile{ + NetworkInterfaces: &[]compute.NetworkInterfaceReference{ + { + ID: to.StringPtr("/subscriptions/123/resourceGroups/test-rg/providers/Microsoft.Network/networkInterfaces/nic-1"), + }, + }, + }, + }, } - internalError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error") - notFoundError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not Found") - errCtxExceeded = errors.New("ctx exceeded") + fakeNetworkInterface = network.Interface{ + InterfacePropertiesFormat: &network.InterfacePropertiesFormat{ + IPConfigurations: &[]network.InterfaceIPConfiguration{ + { + InterfaceIPConfigurationPropertiesFormat: &network.InterfaceIPConfigurationPropertiesFormat{ + PrivateIPAddress: to.StringPtr("10.0.0.5"), + PublicIPAddress: &network.PublicIPAddress{ + ID: to.StringPtr("/subscriptions/123/resourceGroups/test-rg/providers/Microsoft.Network/publicIPAddresses/pip-1"), + }, + }, + }, + }, + }, + } + fakePublicIPs = network.PublicIPAddress{ + PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ + IPAddress: to.StringPtr("10.0.0.6"), + }, + } + fakeNodeAddresses = []corev1.NodeAddress{ + { + Type: corev1.NodeInternalIP, + Address: "10.0.0.5", + }, + { + Type: corev1.NodeExternalIP, + Address: "10.0.0.6", + }, + } + internalError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error") ) func TestReconcileVM(t *testing.T) { testcases := []struct { name string expectedError string - expect func(s *mock_virtualmachines.MockVMScopeMockRecorder, m *mock_virtualmachines.MockClientMockRecorder, mnic *mock_networkinterfaces.MockClientMockRecorder, mpip *mock_publicips.MockClientMockRecorder) + expect func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_networkinterfaces.MockClientMockRecorder, mpip *mock_publicips.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ { name: "create vm succeeds", expectedError: "", - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, m *mock_virtualmachines.MockClientMockRecorder, mnic *mock_networkinterfaces.MockClientMockRecorder, mpip *mock_publicips.MockClientMockRecorder) { + expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_networkinterfaces.MockClientMockRecorder, mpip *mock_publicips.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.VMSpec().Return(&fakeVMSpec) - s.GetLongRunningOperationState("test-vm", serviceName) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakeVMSpec).Return(compute.VirtualMachine{ - ID: to.StringPtr("test-vm-id"), - VirtualMachineProperties: &compute.VirtualMachineProperties{ - ProvisioningState: to.StringPtr("Succeeded"), - NetworkProfile: &compute.NetworkProfile{ - NetworkInterfaces: &[]compute.NetworkInterfaceReference{ - { - ID: to.StringPtr("/subscriptions/123/resourceGroups/test-rg/providers/Microsoft.Network/networkInterfaces/nic-1"), - }, - }, - }, - }, - }, nil, nil) + r.CreateResource(gomockinternal.AContext(), &fakeVMSpec, serviceName).Return(fakeExistingVM, nil) s.UpdatePutStatus(infrav1.VMRunningCondition, serviceName, nil) s.UpdatePutStatus(infrav1.DisksReadyCondition, serviceName, nil) s.SetProviderID("azure://test-vm-id") s.SetAnnotation("cluster-api-provider-azure", "true") - mnic.Get(gomockinternal.AContext(), "test-group", "nic-1").Return(network.Interface{ - InterfacePropertiesFormat: &network.InterfacePropertiesFormat{ - IPConfigurations: &[]network.InterfaceIPConfiguration{ - { - InterfaceIPConfigurationPropertiesFormat: &network.InterfaceIPConfigurationPropertiesFormat{ - PrivateIPAddress: to.StringPtr("10.0.0.5"), - PublicIPAddress: &network.PublicIPAddress{ - ID: to.StringPtr("/subscriptions/123/resourceGroups/test-rg/providers/Microsoft.Network/publicIPAddresses/pip-1"), - }, - }, - }, - }, - }, - }, nil) - mpip.Get(gomockinternal.AContext(), "test-group", "pip-1").Return(network.PublicIPAddress{ - PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ - IPAddress: to.StringPtr("10.0.0.6"), - }, - }, nil) - s.SetAddresses([]corev1.NodeAddress{ - { - Type: corev1.NodeInternalIP, - Address: "10.0.0.5", - }, - { - Type: corev1.NodeExternalIP, - Address: "10.0.0.6", - }, - }) + mnic.Get(gomockinternal.AContext(), "test-group", "nic-1").Return(fakeNetworkInterface, nil) + mpip.Get(gomockinternal.AContext(), "test-group", "pip-1").Return(fakePublicIPs, nil) + s.SetAddresses(fakeNodeAddresses) s.SetVMState(infrav1.Succeeded) }, }, { - name: "create vm fails", - expectedError: "failed to create resource test-group/test-vm (service: virtualmachine): #: Internal Server Error: StatusCode=500", - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, m *mock_virtualmachines.MockClientMockRecorder, mnic *mock_networkinterfaces.MockClientMockRecorder, mpip *mock_publicips.MockClientMockRecorder) { + name: "creating vm fails", + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_networkinterfaces.MockClientMockRecorder, mpip *mock_publicips.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.VMSpec().Return(&fakeVMSpec) + r.CreateResource(gomockinternal.AContext(), &fakeVMSpec, serviceName).Return(nil, internalError) + s.UpdatePutStatus(infrav1.VMRunningCondition, serviceName, internalError) + s.UpdatePutStatus(infrav1.DisksReadyCondition, serviceName, internalError) + }, + }, + { + name: "create vm succeeds but failed to get network interfaces", + expectedError: "failed to fetch VM addresses: #: Internal Server Error: StatusCode=500", + expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_networkinterfaces.MockClientMockRecorder, mpip *mock_publicips.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.VMSpec().Return(&fakeVMSpec) - s.GetLongRunningOperationState("test-vm", serviceName) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakeVMSpec).Return(nil, nil, internalError) - s.UpdatePutStatus(infrav1.VMRunningCondition, serviceName, gomockinternal.ErrStrEq(fmt.Sprintf("failed to create resource test-group/test-vm (service: virtualmachine): %s", internalError.Error()))) - s.UpdatePutStatus(infrav1.DisksReadyCondition, serviceName, gomockinternal.ErrStrEq(fmt.Sprintf("failed to create resource test-group/test-vm (service: virtualmachine): %s", internalError.Error()))) + r.CreateResource(gomockinternal.AContext(), &fakeVMSpec, serviceName).Return(fakeExistingVM, nil) + s.UpdatePutStatus(infrav1.VMRunningCondition, serviceName, nil) + s.UpdatePutStatus(infrav1.DisksReadyCondition, serviceName, nil) + s.SetProviderID("azure://test-vm-id") + s.SetAnnotation("cluster-api-provider-azure", "true") + mnic.Get(gomockinternal.AContext(), "test-group", "nic-1").Return(network.Interface{}, internalError) + }, + }, + { + name: "create vm succeeds but failed to get public IPs", + expectedError: "failed to fetch VM addresses: #: Internal Server Error: StatusCode=500", + expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_networkinterfaces.MockClientMockRecorder, mpip *mock_publicips.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.VMSpec().Return(&fakeVMSpec) + r.CreateResource(gomockinternal.AContext(), &fakeVMSpec, serviceName).Return(fakeExistingVM, nil) + s.UpdatePutStatus(infrav1.VMRunningCondition, serviceName, nil) + s.UpdatePutStatus(infrav1.DisksReadyCondition, serviceName, nil) + s.SetProviderID("azure://test-vm-id") + s.SetAnnotation("cluster-api-provider-azure", "true") + mnic.Get(gomockinternal.AContext(), "test-group", "nic-1").Return(fakeNetworkInterface, nil) + mpip.Get(gomockinternal.AContext(), "test-group", "pip-1").Return(network.PublicIPAddress{}, internalError) }, }, } @@ -150,19 +168,19 @@ func TestReconcileVM(t *testing.T) { defer mockCtrl.Finish() scopeMock := mock_virtualmachines.NewMockVMScope(mockCtrl) - clientMock := mock_virtualmachines.NewMockClient(mockCtrl) interfaceMock := mock_networkinterfaces.NewMockClient(mockCtrl) publicIPMock := mock_publicips.NewMockClient(mockCtrl) availabilitySetsMock := mock_availabilitysets.NewMockClient(mockCtrl) + asyncMock := mock_async.NewMockReconciler(mockCtrl) - tc.expect(scopeMock.EXPECT(), clientMock.EXPECT(), interfaceMock.EXPECT(), publicIPMock.EXPECT()) + tc.expect(scopeMock.EXPECT(), interfaceMock.EXPECT(), publicIPMock.EXPECT(), asyncMock.EXPECT()) s := &Service{ Scope: scopeMock, - Client: clientMock, interfacesClient: interfaceMock, publicIPsClient: publicIPMock, availabilitySetsClient: availabilitySetsMock, + Reconciler: asyncMock, } err := s.Reconcile(context.TODO()) @@ -180,73 +198,34 @@ func TestDeleteVM(t *testing.T) { testcases := []struct { name string expectedError string - expect func(s *mock_virtualmachines.MockVMScopeMockRecorder, m *mock_virtualmachines.MockClientMockRecorder) + expect func(s *mock_virtualmachines.MockVMScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ - { - name: "long running delete operation is done", - expectedError: "", - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, m *mock_virtualmachines.MockClientMockRecorder) { - s.VMSpec().AnyTimes().Return(&fakeVMSpec) - s.GetLongRunningOperationState("test-vm", serviceName).Times(2).Return(&fakeFuture) - m.IsDone(gomockinternal.AContext(), gomock.AssignableToTypeOf(&azureautorest.Future{})).Return(true, nil) - m.Result(gomockinternal.AContext(), gomock.AssignableToTypeOf(&azureautorest.Future{}), infrav1.DeleteFuture).Return(nil, nil) - s.DeleteLongRunningOperationState("test-vm", serviceName) - s.SetVMState(infrav1.Deleted) - s.UpdateDeleteStatus(infrav1.VMRunningCondition, serviceName, nil) - }, - }, - { - name: "long running delete operation is not done", - expectedError: "operation type DELETE on Azure resource test-group/test-vm is not done. Object will be requeued after 15s", - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, m *mock_virtualmachines.MockClientMockRecorder) { - s.VMSpec().AnyTimes().Return(&fakeVMSpec) - s.GetLongRunningOperationState("test-vm", serviceName).Times(2).Return(&fakeFuture) - m.IsDone(gomockinternal.AContext(), gomock.AssignableToTypeOf(&azureautorest.Future{})).Return(false, nil) - s.SetVMState(infrav1.Deleting) - s.UpdateDeleteStatus(infrav1.VMRunningCondition, serviceName, gomockinternal.ErrStrEq("operation type DELETE on Azure resource test-group/test-vm is not done. Object will be requeued after 15s")) - }, - }, { name: "vm doesn't exist", expectedError: "", - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, m *mock_virtualmachines.MockClientMockRecorder) { + expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.VMSpec().AnyTimes().Return(&fakeVMSpec) - s.GetLongRunningOperationState("test-vm", serviceName) - m.DeleteAsync(gomockinternal.AContext(), &fakeVMSpec).Return(nil, notFoundError) + r.DeleteResource(gomockinternal.AContext(), &fakeVMSpec, serviceName).Return(nil) s.SetVMState(infrav1.Deleted) s.UpdateDeleteStatus(infrav1.VMRunningCondition, serviceName, nil) }, }, { name: "error occurs when deleting vm", - expectedError: "failed to delete resource test-group/test-vm (service: virtualmachine): #: Internal Server Error: StatusCode=500", - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, m *mock_virtualmachines.MockClientMockRecorder) { - s.VMSpec().AnyTimes().Return(&fakeVMSpec) - s.GetLongRunningOperationState("test-vm", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakeVMSpec).Return(nil, internalError) - s.SetVMState(infrav1.Deleting) - s.UpdateDeleteStatus(infrav1.VMRunningCondition, serviceName, gomockinternal.ErrStrEq("failed to delete resource test-group/test-vm (service: virtualmachine): #: Internal Server Error: StatusCode=500")) - }, - }, - { - name: "context deadline exceeded while deleting vm", - expectedError: "operation type DELETE on Azure resource test-group/test-vm is not done. Object will be requeued after 15s", - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, m *mock_virtualmachines.MockClientMockRecorder) { + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.VMSpec().AnyTimes().Return(&fakeVMSpec) - s.GetLongRunningOperationState("test-vm", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakeVMSpec).Return(&azureautorest.Future{}, errCtxExceeded) - s.SetLongRunningOperationState(gomock.AssignableToTypeOf(&infrav1.Future{})) + r.DeleteResource(gomockinternal.AContext(), &fakeVMSpec, serviceName).Return(internalError) s.SetVMState(infrav1.Deleting) - s.UpdateDeleteStatus(infrav1.VMRunningCondition, serviceName, gomockinternal.ErrStrEq("operation type DELETE on Azure resource test-group/test-vm is not done. Object will be requeued after 15s")) + s.UpdateDeleteStatus(infrav1.VMRunningCondition, serviceName, internalError) }, }, { name: "delete the vm successfully", expectedError: "", - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, m *mock_virtualmachines.MockClientMockRecorder) { + expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.VMSpec().AnyTimes().Return(&fakeVMSpec) - s.GetLongRunningOperationState("test-vm", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakeVMSpec).Return(nil, nil) + r.DeleteResource(gomockinternal.AContext(), &fakeVMSpec, serviceName).Return(nil) s.SetVMState(infrav1.Deleted) s.UpdateDeleteStatus(infrav1.VMRunningCondition, serviceName, nil) }, @@ -261,13 +240,13 @@ func TestDeleteVM(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() scopeMock := mock_virtualmachines.NewMockVMScope(mockCtrl) - clientMock := mock_virtualmachines.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/vnetpeerings/client.go b/azure/services/vnetpeerings/client.go index 0674cc7f541..b16e89faea9 100644 --- a/azure/services/vnetpeerings/client.go +++ b/azure/services/vnetpeerings/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, string) (network.VirtualNetworkPeering, 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 { peerings network.VirtualNetworkPeeringsClient } -var _ Client = &AzureClient{} - // NewClient creates a new virtual network peerings client from subscription ID. func NewClient(auth azure.Authorizer) *AzureClient { c := newPeeringsClient(auth.SubscriptionID(), auth.BaseURI(), auth.Authorizer()) @@ -61,40 +50,23 @@ func newPeeringsClient(subscriptionID string, baseURI string, authorizer autores } // Get gets the specified virtual network peering by the peering name, virtual network, and resource group. -func (ac *AzureClient) Get(ctx context.Context, resourceGroupName, vnetName, peeringName string) (network.VirtualNetworkPeering, error) { - ctx, span := tele.Tracer().Start(ctx, "vnetpeerings.AzureClient.Get") - defer span.End() +func (ac *AzureClient) Get(ctx context.Context, spec azure.ResourceSpecGetter) (interface{}, error) { + ctx, _, done := tele.StartSpanWithLogger(ctx, "vnetpeerings.AzureClient.Get") + defer done() - return ac.peerings.Get(ctx, resourceGroupName, vnetName, peeringName) + return ac.peerings.Get(ctx, spec.ResourceGroupName(), spec.OwnerResourceName(), spec.ResourceName()) } // CreateOrUpdateAsync creates or updates a virtual network peering asynchronously. // It sends a PUT request to Azure and if accepted without error, the func will return a Future which can be used to track the ongoing // progress of the operation. -func (ac *AzureClient) CreateOrUpdateAsync(ctx context.Context, spec azure.ResourceSpecGetter) (interface{}, azureautorest.FutureAPI, error) { - ctx, span := tele.Tracer().Start(ctx, "vnetpeerings.AzureClient.CreateOrUpdateAsync") - defer span.End() - - var existingPeering interface{} - - if existing, err := ac.Get(ctx, spec.ResourceGroupName(), spec.OwnerResourceName(), spec.ResourceName()); err != nil && !azure.ResourceNotFound(err) { - return nil, nil, errors.Wrapf(err, "failed to get virtual network peering %s for %s in %s", spec.ResourceName(), spec.OwnerResourceName(), spec.ResourceGroupName()) - } else if err == nil { - existingPeering = existing - } +func (ac *AzureClient) CreateOrUpdateAsync(ctx context.Context, spec azure.ResourceSpecGetter, parameters interface{}) (interface{}, azureautorest.FutureAPI, error) { + ctx, _, done := tele.StartSpanWithLogger(ctx, "vnetpeerings.AzureClient.CreateOrUpdateAsync") + defer done() - params, err := spec.Parameters(existingPeering) - if err != nil { - return nil, nil, errors.Wrapf(err, "failed to get desired parameters for virtual network peering %s", spec.ResourceName()) - } - - peering, ok := params.(network.VirtualNetworkPeering) + peering, ok := parameters.(network.VirtualNetworkPeering) if !ok { - if params == nil { - // nothing to do here. - return existingPeering, nil, nil - } - return nil, nil, errors.Errorf("%T is not a network.VirtualNetworkPeering", params) + return nil, nil, errors.Errorf("%T is not a network.VirtualNetworkPeering", parameters) } future, err := ac.peerings.CreateOrUpdate(ctx, spec.ResourceGroupName(), spec.OwnerResourceName(), spec.ResourceName(), peering, network.SyncRemoteAddressSpaceTrue) @@ -121,8 +93,8 @@ func (ac *AzureClient) CreateOrUpdateAsync(ctx context.Context, spec azure.Resou // 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, span := tele.Tracer().Start(ctx, "vnetpeerings.AzureClient.Delete") - defer span.End() + ctx, _, done := tele.StartSpanWithLogger(ctx, "vnetpeerings.AzureClient.Delete") + defer done() future, err := ac.peerings.Delete(ctx, spec.ResourceGroupName(), spec.OwnerResourceName(), spec.ResourceName()) if err != nil { @@ -145,19 +117,22 @@ func (ac *AzureClient) DeleteAsync(ctx context.Context, spec azure.ResourceSpecG // IsDone returns true if the long-running operation has completed. func (ac *AzureClient) IsDone(ctx context.Context, future azureautorest.FutureAPI) (bool, error) { - ctx, span := tele.Tracer().Start(ctx, "vnetpeerings.AzureClient.IsDone") - defer span.End() + ctx, _, done := tele.StartSpanWithLogger(ctx, "vnetpeerings.AzureClient.IsDone") + defer done() - done, err := future.DoneWithContext(ctx, ac.peerings) + isDone, err := future.DoneWithContext(ctx, ac.peerings) if err != nil { return false, errors.Wrap(err, "failed checking if the operation was complete") } - return done, nil + 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, "vnetpeerings.AzureClient.Result") + defer done() + if futureData == nil { return nil, errors.Errorf("cannot get result from nil future") } diff --git a/azure/services/vnetpeerings/mock_vnetpeerings/client_mock.go b/azure/services/vnetpeerings/mock_vnetpeerings/client_mock.go deleted file mode 100644 index ecd68a5d0fd..00000000000 --- a/azure/services/vnetpeerings/mock_vnetpeerings/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_vnetpeerings is a generated GoMock package. -package mock_vnetpeerings - -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, arg3 string) (network.VirtualNetworkPeering, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Get", arg0, arg1, arg2, arg3) - ret0, _ := ret[0].(network.VirtualNetworkPeering) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// Get indicates an expected call of Get. -func (mr *MockClientMockRecorder) Get(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockClient)(nil).Get), arg0, arg1, arg2, arg3) -} - -// 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/vnetpeerings/mock_vnetpeerings/doc.go b/azure/services/vnetpeerings/mock_vnetpeerings/doc.go index 8bd3fe24d27..a61540300e8 100644 --- a/azure/services/vnetpeerings/mock_vnetpeerings/doc.go +++ b/azure/services/vnetpeerings/mock_vnetpeerings/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_vnetpeerings -source ../client.go Client //go:generate ../../../../hack/tools/bin/mockgen -destination vnetpeerings_mock.go -package mock_vnetpeerings -source ../vnetpeerings.go VnetPeeringScope -//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 vnetpeerings_mock.go > _vnetpeerings_mock.go && mv _vnetpeerings_mock.go vnetpeerings_mock.go" package mock_vnetpeerings //nolint diff --git a/azure/services/vnetpeerings/vnetpeerings.go b/azure/services/vnetpeerings/vnetpeerings.go index 4c9dda94343..6ff3bd95c56 100644 --- a/azure/services/vnetpeerings/vnetpeerings.go +++ b/azure/services/vnetpeerings/vnetpeerings.go @@ -38,14 +38,15 @@ type VnetPeeringScope interface { // Service provides operations on Azure resources. type Service struct { Scope VnetPeeringScope - Client + async.Reconciler } // New creates a new service. func New(scope VnetPeeringScope) *Service { + Client := NewClient(scope) return &Service{ - Scope: scope, - Client: NewClient(scope), + Scope: scope, + Reconciler: async.New(scope, Client, Client), } } @@ -62,7 +63,7 @@ func (s *Service) Reconcile(ctx context.Context) error { // order of precedence is: error creating -> creating in progress -> created (no error) var result error for _, peeringSpec := range s.Scope.VnetPeeringSpecs() { - if _, err := async.CreateResource(ctx, s.Scope, s.Client, peeringSpec, serviceName); err != nil { + if _, err := s.CreateResource(ctx, peeringSpec, serviceName); err != nil { if !azure.IsOperationNotDoneError(err) || result == nil { result = err } @@ -87,7 +88,7 @@ func (s *Service) Delete(ctx context.Context) error { // If multiple errors occur, we return the most pressing one // order of precedence is: error deleting -> deleting in progress -> deleted (no error) for _, peeringSpec := range s.Scope.VnetPeeringSpecs() { - if err := async.DeleteResource(ctx, s.Scope, s.Client, peeringSpec, serviceName); err != nil { + if err := s.DeleteResource(ctx, peeringSpec, serviceName); err != nil { if !azure.IsOperationNotDoneError(err) || result == nil { result = err } diff --git a/azure/services/vnetpeerings/vnetpeerings_test.go b/azure/services/vnetpeerings/vnetpeerings_test.go index c210f7afcb9..db70ac2933f 100644 --- a/azure/services/vnetpeerings/vnetpeerings_test.go +++ b/azure/services/vnetpeerings/vnetpeerings_test.go @@ -18,16 +18,16 @@ package vnetpeerings import ( "context" - "errors" "fmt" "net/http" "testing" - azureautorest "github.com/Azure/go-autorest/autorest/azure" + "github.com/Azure/go-autorest/autorest" "github.com/golang/mock/gomock" . "github.com/onsi/gomega" 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/vnetpeerings/mock_vnetpeerings" gomockinternal "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock" ) @@ -75,37 +75,29 @@ var ( } fakePeeringSpecs = []azure.ResourceSpecGetter{&fakePeering1To2, &fakePeering2To1, &fakePeering1To3, &fakePeering3To1} fakePeeringExtraSpecs = []azure.ResourceSpecGetter{&fakePeering1To2, &fakePeering2To1, &fakePeeringExtra} - fakeFuture, _ = azureautorest.NewFutureFromResponse(&http.Response{ - Status: "200 OK", - StatusCode: 200, - Request: &http.Request{ - Method: http.MethodDelete, - }, - }) - errFake = errors.New("this is an error") - errFoo = errors.New("foo") + internalError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error") + notDoneError = azure.NewOperationNotDoneError(&infrav1.Future{}) ) func TestReconcileVnetPeerings(t *testing.T) { testcases := []struct { name string expectedError string - expect func(s *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, m *mock_vnetpeerings.MockClientMockRecorder) + expect func(s *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ { name: "create one peering", expectedError: "", - expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, m *mock_vnetpeerings.MockClientMockRecorder) { + expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { p.VnetPeeringSpecs().Return(fakePeeringSpecs[:1]) - p.GetLongRunningOperationState("vnet1-to-vnet2", serviceName).Return(nil) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakePeering1To2).Return(nil, nil, nil) + r.CreateResource(gomockinternal.AContext(), &fakePeering1To2, serviceName).Return(&fakePeering1To2, nil) p.UpdatePutStatus(infrav1.VnetPeeringReadyCondition, serviceName, nil) }, }, { name: "create no peerings", expectedError: "", - expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, m *mock_vnetpeerings.MockClientMockRecorder) { + expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { p.VnetPeeringSpecs().Return(fakePeeringSpecs[:0]) p.UpdatePutStatus(infrav1.VnetPeeringReadyCondition, serviceName, nil) }, @@ -113,91 +105,94 @@ func TestReconcileVnetPeerings(t *testing.T) { { name: "create even number of peerings", expectedError: "", - expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, m *mock_vnetpeerings.MockClientMockRecorder) { + expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { p.VnetPeeringSpecs().Return(fakePeeringSpecs[:2]) - p.GetLongRunningOperationState("vnet1-to-vnet2", serviceName).Return(nil) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakePeering1To2).Return(nil, nil, nil) - p.GetLongRunningOperationState("vnet2-to-vnet1", serviceName).Return(nil) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakePeering2To1).Return(nil, nil, nil) + r.CreateResource(gomockinternal.AContext(), &fakePeering1To2, serviceName).Return(&fakePeering1To2, nil) + r.CreateResource(gomockinternal.AContext(), &fakePeering2To1, serviceName).Return(&fakePeering2To1, nil) p.UpdatePutStatus(infrav1.VnetPeeringReadyCondition, serviceName, nil) }, }, { name: "create odd number of peerings", expectedError: "", - expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, m *mock_vnetpeerings.MockClientMockRecorder) { + expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { p.VnetPeeringSpecs().Return(fakePeeringExtraSpecs) - p.GetLongRunningOperationState("vnet1-to-vnet2", serviceName).Return(nil) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakePeering1To2).Return(nil, nil, nil) - - p.GetLongRunningOperationState("vnet2-to-vnet1", serviceName).Return(nil) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakePeering2To1).Return(nil, nil, nil) - - p.GetLongRunningOperationState("extra-peering", serviceName).Return(nil) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakePeeringExtra).Return(nil, nil, nil) - + r.CreateResource(gomockinternal.AContext(), &fakePeering1To2, serviceName).Return(&fakePeering1To2, nil) + r.CreateResource(gomockinternal.AContext(), &fakePeering2To1, serviceName).Return(&fakePeering2To1, nil) + r.CreateResource(gomockinternal.AContext(), &fakePeeringExtra, serviceName).Return(&fakePeeringExtra, nil) p.UpdatePutStatus(infrav1.VnetPeeringReadyCondition, serviceName, nil) }, }, { name: "create multiple peerings on one vnet", expectedError: "", - expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, m *mock_vnetpeerings.MockClientMockRecorder) { + expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { p.VnetPeeringSpecs().Return(fakePeeringSpecs) - p.GetLongRunningOperationState("vnet1-to-vnet2", serviceName).Return(nil) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakePeering1To2).Return(nil, nil, nil) - - p.GetLongRunningOperationState("vnet2-to-vnet1", serviceName).Return(nil) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakePeering2To1).Return(nil, nil, nil) - - p.GetLongRunningOperationState("vnet1-to-vnet3", serviceName).Return(nil) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakePeering1To3).Return(nil, nil, nil) - - p.GetLongRunningOperationState("vnet3-to-vnet1", serviceName).Return(nil) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakePeering3To1).Return(nil, nil, nil) - + r.CreateResource(gomockinternal.AContext(), &fakePeering1To2, serviceName).Return(&fakePeering1To2, nil) + r.CreateResource(gomockinternal.AContext(), &fakePeering2To1, serviceName).Return(&fakePeering2To1, nil) + r.CreateResource(gomockinternal.AContext(), &fakePeering1To3, serviceName).Return(&fakePeering1To3, nil) + r.CreateResource(gomockinternal.AContext(), &fakePeering3To1, serviceName).Return(&fakePeering3To1, nil) p.UpdatePutStatus(infrav1.VnetPeeringReadyCondition, serviceName, nil) }, }, { name: "error in creating peering", - expectedError: "failed to create resource group1/vnet1-to-vnet3 (service: vnetpeerings): this is an error", - expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, m *mock_vnetpeerings.MockClientMockRecorder) { + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { p.VnetPeeringSpecs().Return(fakePeeringSpecs) - p.GetLongRunningOperationState("vnet1-to-vnet2", serviceName).Return(nil) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakePeering1To2).Return(nil, nil, nil) - - p.GetLongRunningOperationState("vnet2-to-vnet1", serviceName).Return(nil) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakePeering2To1).Return(nil, nil, nil) - - p.GetLongRunningOperationState("vnet1-to-vnet3", serviceName).Return(nil) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakePeering1To3).Return(nil, nil, errFake) - - p.GetLongRunningOperationState("vnet3-to-vnet1", serviceName).Return(nil) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakePeering3To1).Return(nil, nil, nil) - - p.UpdatePutStatus(infrav1.VnetPeeringReadyCondition, serviceName, gomockinternal.ErrStrEq("failed to create resource group1/vnet1-to-vnet3 (service: vnetpeerings): this is an error")) + r.CreateResource(gomockinternal.AContext(), &fakePeering1To2, serviceName).Return(&fakePeering1To2, nil) + r.CreateResource(gomockinternal.AContext(), &fakePeering2To1, serviceName).Return(&fakePeering2To1, nil) + r.CreateResource(gomockinternal.AContext(), &fakePeering1To3, serviceName).Return(nil, internalError) + r.CreateResource(gomockinternal.AContext(), &fakePeering3To1, serviceName).Return(&fakePeering3To1, nil) + p.UpdatePutStatus(infrav1.VnetPeeringReadyCondition, serviceName, internalError) }, }, { - name: "error in creating peering which is not done", - expectedError: "failed to create resource group1/vnet1-to-vnet3 (service: vnetpeerings): this is an error", - expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, m *mock_vnetpeerings.MockClientMockRecorder) { + name: "error in creating peering", + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { p.VnetPeeringSpecs().Return(fakePeeringSpecs) - p.GetLongRunningOperationState("vnet1-to-vnet2", serviceName).Return(nil) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakePeering1To2).Return(nil, nil, nil) - - p.GetLongRunningOperationState("vnet2-to-vnet1", serviceName).Return(nil) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakePeering2To1).Return(nil, nil, nil) - - p.GetLongRunningOperationState("vnet1-to-vnet3", serviceName).Return(nil) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakePeering1To3).Return(nil, nil, errFake) - - p.GetLongRunningOperationState("vnet3-to-vnet1", serviceName).Return(nil) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakePeering3To1).Return(nil, &fakeFuture, errFoo) - p.SetLongRunningOperationState(gomock.AssignableToTypeOf(&infrav1.Future{})) - - p.UpdatePutStatus(infrav1.VnetPeeringReadyCondition, serviceName, gomockinternal.ErrStrEq("failed to create resource group1/vnet1-to-vnet3 (service: vnetpeerings): this is an error")) + r.CreateResource(gomockinternal.AContext(), &fakePeering1To2, serviceName).Return(&fakePeering1To2, nil) + r.CreateResource(gomockinternal.AContext(), &fakePeering2To1, serviceName).Return(&fakePeering2To1, nil) + r.CreateResource(gomockinternal.AContext(), &fakePeering1To3, serviceName).Return(nil, internalError) + r.CreateResource(gomockinternal.AContext(), &fakePeering3To1, serviceName).Return(&fakePeering3To1, nil) + p.UpdatePutStatus(infrav1.VnetPeeringReadyCondition, serviceName, internalError) + }, + }, + { + name: "not done error in creating is ignored", + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + p.VnetPeeringSpecs().Return(fakePeeringSpecs) + r.CreateResource(gomockinternal.AContext(), &fakePeering1To2, serviceName).Return(&fakePeering1To2, nil) + r.CreateResource(gomockinternal.AContext(), &fakePeering2To1, serviceName).Return(nil, internalError) + r.CreateResource(gomockinternal.AContext(), &fakePeering1To3, serviceName).Return(nil, notDoneError) + r.CreateResource(gomockinternal.AContext(), &fakePeering3To1, serviceName).Return(&fakePeering3To1, nil) + p.UpdatePutStatus(infrav1.VnetPeeringReadyCondition, serviceName, internalError) + }, + }, + { + name: "not done error in creating is overwritten", + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + p.VnetPeeringSpecs().Return(fakePeeringSpecs) + r.CreateResource(gomockinternal.AContext(), &fakePeering1To2, serviceName).Return(&fakePeering1To2, nil) + r.CreateResource(gomockinternal.AContext(), &fakePeering2To1, serviceName).Return(&fakePeering2To1, nil) + r.CreateResource(gomockinternal.AContext(), &fakePeering1To3, serviceName).Return(nil, notDoneError) + r.CreateResource(gomockinternal.AContext(), &fakePeering3To1, serviceName).Return(nil, internalError) + p.UpdatePutStatus(infrav1.VnetPeeringReadyCondition, serviceName, internalError) + }, + }, + { + name: "not done error in creating remains", + expectedError: "operation type on Azure resource / is not done", + expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + p.VnetPeeringSpecs().Return(fakePeeringSpecs) + r.CreateResource(gomockinternal.AContext(), &fakePeering1To2, serviceName).Return(&fakePeering1To2, nil) + r.CreateResource(gomockinternal.AContext(), &fakePeering2To1, serviceName).Return(&fakePeering2To1, nil) + r.CreateResource(gomockinternal.AContext(), &fakePeering1To3, serviceName).Return(nil, notDoneError) + r.CreateResource(gomockinternal.AContext(), &fakePeering3To1, serviceName).Return(&fakePeering3To1, nil) + p.UpdatePutStatus(infrav1.VnetPeeringReadyCondition, serviceName, notDoneError) }, }, } @@ -211,13 +206,13 @@ func TestReconcileVnetPeerings(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() scopeMock := mock_vnetpeerings.NewMockVnetPeeringScope(mockCtrl) - clientMock := mock_vnetpeerings.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()) @@ -235,23 +230,21 @@ func TestDeleteVnetPeerings(t *testing.T) { testcases := []struct { name string expectedError string - expect func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, m *mock_vnetpeerings.MockClientMockRecorder) + expect func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ { name: "delete one peering", expectedError: "", - expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, m *mock_vnetpeerings.MockClientMockRecorder) { + expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { p.VnetPeeringSpecs().Return(fakePeeringSpecs[:1]) - p.GetLongRunningOperationState("vnet1-to-vnet2", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakePeering1To2).Return(nil, nil) - + r.DeleteResource(gomockinternal.AContext(), &fakePeering1To2, serviceName).Return(nil) p.UpdateDeleteStatus(infrav1.VnetPeeringReadyCondition, serviceName, nil) }, }, { name: "delete no peerings", expectedError: "", - expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, m *mock_vnetpeerings.MockClientMockRecorder) { + expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { p.VnetPeeringSpecs().Return(fakePeeringSpecs[:0]) p.UpdateDeleteStatus(infrav1.VnetPeeringReadyCondition, serviceName, nil) }, @@ -259,93 +252,82 @@ func TestDeleteVnetPeerings(t *testing.T) { { name: "delete even number of peerings", expectedError: "", - expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, m *mock_vnetpeerings.MockClientMockRecorder) { + expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { p.VnetPeeringSpecs().Return(fakePeeringSpecs[:2]) - p.GetLongRunningOperationState("vnet1-to-vnet2", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakePeering1To2).Return(nil, nil) - - p.GetLongRunningOperationState("vnet2-to-vnet1", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakePeering2To1).Return(nil, nil) - + r.DeleteResource(gomockinternal.AContext(), &fakePeering1To2, serviceName).Return(nil) + r.DeleteResource(gomockinternal.AContext(), &fakePeering2To1, serviceName).Return(nil) p.UpdateDeleteStatus(infrav1.VnetPeeringReadyCondition, serviceName, nil) }, }, { name: "delete odd number of peerings", expectedError: "", - expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, m *mock_vnetpeerings.MockClientMockRecorder) { + expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { p.VnetPeeringSpecs().Return(fakePeeringExtraSpecs) - p.GetLongRunningOperationState("vnet1-to-vnet2", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakePeering1To2).Return(nil, nil) - - p.GetLongRunningOperationState("vnet2-to-vnet1", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakePeering2To1).Return(nil, nil) - - p.GetLongRunningOperationState("extra-peering", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakePeeringExtra).Return(nil, nil) - + r.DeleteResource(gomockinternal.AContext(), &fakePeering1To2, serviceName).Return(nil) + r.DeleteResource(gomockinternal.AContext(), &fakePeering2To1, serviceName).Return(nil) + r.DeleteResource(gomockinternal.AContext(), &fakePeeringExtra, serviceName).Return(nil) p.UpdateDeleteStatus(infrav1.VnetPeeringReadyCondition, serviceName, nil) }, }, { name: "delete multiple peerings on one vnet", expectedError: "", - expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, m *mock_vnetpeerings.MockClientMockRecorder) { + expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { p.VnetPeeringSpecs().Return(fakePeeringSpecs) - p.GetLongRunningOperationState("vnet1-to-vnet2", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakePeering1To2).Return(nil, nil) - - p.GetLongRunningOperationState("vnet2-to-vnet1", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakePeering2To1).Return(nil, nil) - - p.GetLongRunningOperationState("vnet1-to-vnet3", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakePeering1To3).Return(nil, nil) - - p.GetLongRunningOperationState("vnet3-to-vnet1", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakePeering3To1).Return(nil, nil) - + r.DeleteResource(gomockinternal.AContext(), &fakePeering1To2, serviceName).Return(nil) + r.DeleteResource(gomockinternal.AContext(), &fakePeering2To1, serviceName).Return(nil) + r.DeleteResource(gomockinternal.AContext(), &fakePeering1To3, serviceName).Return(nil) + r.DeleteResource(gomockinternal.AContext(), &fakePeering3To1, serviceName).Return(nil) p.UpdateDeleteStatus(infrav1.VnetPeeringReadyCondition, serviceName, nil) }, }, { name: "error in deleting peering", - expectedError: "failed to delete resource group1/vnet1-to-vnet3 (service: vnetpeerings): this is an error", - expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, m *mock_vnetpeerings.MockClientMockRecorder) { + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { p.VnetPeeringSpecs().Return(fakePeeringSpecs) - p.GetLongRunningOperationState("vnet1-to-vnet2", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakePeering1To2).Return(nil, nil) - - p.GetLongRunningOperationState("vnet2-to-vnet1", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakePeering2To1).Return(nil, nil) - - p.GetLongRunningOperationState("vnet1-to-vnet3", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakePeering1To3).Return(nil, errFake) - - p.GetLongRunningOperationState("vnet3-to-vnet1", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakePeering3To1).Return(nil, nil) - - p.UpdateDeleteStatus(infrav1.VnetPeeringReadyCondition, serviceName, gomockinternal.ErrStrEq("failed to delete resource group1/vnet1-to-vnet3 (service: vnetpeerings): this is an error")) + r.DeleteResource(gomockinternal.AContext(), &fakePeering1To2, serviceName).Return(nil) + r.DeleteResource(gomockinternal.AContext(), &fakePeering2To1, serviceName).Return(nil) + r.DeleteResource(gomockinternal.AContext(), &fakePeering1To3, serviceName).Return(internalError) + r.DeleteResource(gomockinternal.AContext(), &fakePeering3To1, serviceName).Return(nil) + p.UpdateDeleteStatus(infrav1.VnetPeeringReadyCondition, serviceName, internalError) }, }, { - name: "error in deleting peering which is not done", - expectedError: "failed to delete resource group1/vnet1-to-vnet3 (service: vnetpeerings): this is an error", - expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, m *mock_vnetpeerings.MockClientMockRecorder) { + name: "not done error in deleting is ignored", + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { p.VnetPeeringSpecs().Return(fakePeeringSpecs) - p.GetLongRunningOperationState("vnet1-to-vnet2", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakePeering1To2).Return(nil, nil) - - p.GetLongRunningOperationState("vnet2-to-vnet1", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakePeering2To1).Return(nil, nil) - - p.GetLongRunningOperationState("vnet1-to-vnet3", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakePeering1To3).Return(nil, errFake) - - p.GetLongRunningOperationState("vnet3-to-vnet1", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakePeering3To1).Return(&fakeFuture, errFoo) - p.SetLongRunningOperationState(gomock.AssignableToTypeOf(&infrav1.Future{})) - - p.UpdateDeleteStatus(infrav1.VnetPeeringReadyCondition, serviceName, gomockinternal.ErrStrEq("failed to delete resource group1/vnet1-to-vnet3 (service: vnetpeerings): this is an error")) + r.DeleteResource(gomockinternal.AContext(), &fakePeering1To2, serviceName).Return(nil) + r.DeleteResource(gomockinternal.AContext(), &fakePeering2To1, serviceName).Return(internalError) + r.DeleteResource(gomockinternal.AContext(), &fakePeering1To3, serviceName).Return(notDoneError) + r.DeleteResource(gomockinternal.AContext(), &fakePeering3To1, serviceName).Return(nil) + p.UpdateDeleteStatus(infrav1.VnetPeeringReadyCondition, serviceName, internalError) + }, + }, + { + name: "not done error in deleting is overwritten", + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + p.VnetPeeringSpecs().Return(fakePeeringSpecs) + r.DeleteResource(gomockinternal.AContext(), &fakePeering1To2, serviceName).Return(nil) + r.DeleteResource(gomockinternal.AContext(), &fakePeering2To1, serviceName).Return(nil) + r.DeleteResource(gomockinternal.AContext(), &fakePeering1To3, serviceName).Return(notDoneError) + r.DeleteResource(gomockinternal.AContext(), &fakePeering3To1, serviceName).Return(internalError) + p.UpdateDeleteStatus(infrav1.VnetPeeringReadyCondition, serviceName, internalError) + }, + }, + { + name: "not done error in deleting remains", + expectedError: "operation type on Azure resource / is not done", + expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + p.VnetPeeringSpecs().Return(fakePeeringSpecs) + r.DeleteResource(gomockinternal.AContext(), &fakePeering1To2, serviceName).Return(nil) + r.DeleteResource(gomockinternal.AContext(), &fakePeering2To1, serviceName).Return(nil) + r.DeleteResource(gomockinternal.AContext(), &fakePeering1To3, serviceName).Return(notDoneError) + r.DeleteResource(gomockinternal.AContext(), &fakePeering3To1, serviceName).Return(nil) + p.UpdateDeleteStatus(infrav1.VnetPeeringReadyCondition, serviceName, notDoneError) }, }, } @@ -359,13 +341,13 @@ func TestDeleteVnetPeerings(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() scopeMock := mock_vnetpeerings.NewMockVnetPeeringScope(mockCtrl) - clientMock := mock_vnetpeerings.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())