From 7a76e16a66d2953bc9467a19e952c5b4b27ee1d9 Mon Sep 17 00:00:00 2001 From: Cecile Robert-Michon Date: Thu, 2 Sep 2021 17:05:14 -0400 Subject: [PATCH] Make VNet and NSGs reconcile/delete async --- api/v1alpha4/types.go | 5 - azure/interfaces.go | 2 +- azure/mock_azure/azure_mock.go | 10 +- azure/scope/cluster.go | 51 +- azure/scope/managedcontrolplane.go | 29 +- .../mocks_bastionhosts/bastionhosts_mock.go | 5 +- .../mock_loadbalancers/loadbalancers_mock.go | 5 +- .../mock_natgateways/natgateways_mock.go | 5 +- azure/services/natgateways/natgateways.go | 10 +- .../services/natgateways/natgateways_test.go | 124 +---- .../mock_routetables/routetables_mock.go | 5 +- azure/services/routetables/routetables.go | 10 +- .../services/routetables/routetables_test.go | 112 +--- azure/services/securitygroups/client.go | 106 +++- .../mock_securitygroups/client_mock.go | 51 +- .../securitygroups_mock.go | 312 ++---------- .../services/securitygroups/securitygroups.go | 143 ++---- .../securitygroups/securitygroups_test.go | 477 +++++++++--------- azure/services/securitygroups/spec.go | 117 +++++ .../subnets/mock_subnets/subnets_mock.go | 5 +- azure/services/subnets/subnets.go | 25 +- azure/services/subnets/subnets_test.go | 26 +- azure/services/virtualnetworks/client.go | 108 +++- .../mock_virtualnetworks/client_mock.go | 51 +- .../virtualnetworks_mock.go | 137 ++--- azure/services/virtualnetworks/spec.go | 72 +++ .../virtualnetworks/virtualnetworks.go | 135 ++--- .../virtualnetworks/virtualnetworks_test.go | 423 ++++++---------- azure/types.go | 13 - 29 files changed, 1234 insertions(+), 1340 deletions(-) create mode 100644 azure/services/securitygroups/spec.go create mode 100644 azure/services/virtualnetworks/spec.go diff --git a/api/v1alpha4/types.go b/api/v1alpha4/types.go index 1ea9430e7d45..ff7de9319f4c 100644 --- a/api/v1alpha4/types.go +++ b/api/v1alpha4/types.go @@ -111,11 +111,6 @@ type VnetSpec struct { Tags Tags `json:"tags,omitempty"` } -// IsManaged returns true if the vnet is managed. -func (v *VnetSpec) IsManaged(clusterName string) bool { - return v.ID == "" || v.Tags.HasOwned(clusterName) -} - // Subnets is a slice of Subnet. type Subnets []SubnetSpec diff --git a/azure/interfaces.go b/azure/interfaces.go index 7b7ceea28ba5..d59696879330 100644 --- a/azure/interfaces.go +++ b/azure/interfaces.go @@ -54,7 +54,7 @@ type Authorizer interface { // NetworkDescriber is an interface which can get common Azure Cluster Networking information. type NetworkDescriber interface { Vnet() *infrav1.VnetSpec - IsVnetManaged() bool + IsVnetManaged() (bool, error) ControlPlaneSubnet() infrav1.SubnetSpec Subnets() infrav1.Subnets Subnet(string) infrav1.SubnetSpec diff --git a/azure/mock_azure/azure_mock.go b/azure/mock_azure/azure_mock.go index 408e24f4b7ec..295f552f6920 100644 --- a/azure/mock_azure/azure_mock.go +++ b/azure/mock_azure/azure_mock.go @@ -404,11 +404,12 @@ func (mr *MockNetworkDescriberMockRecorder) IsIPv6Enabled() *gomock.Call { } // IsVnetManaged mocks base method. -func (m *MockNetworkDescriber) IsVnetManaged() bool { +func (m *MockNetworkDescriber) IsVnetManaged() (bool, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "IsVnetManaged") ret0, _ := ret[0].(bool) - return ret0 + ret1, _ := ret[1].(error) + return ret0, ret1 } // IsVnetManaged indicates an expected call of IsVnetManaged. @@ -1091,11 +1092,12 @@ func (mr *MockClusterScoperMockRecorder) IsIPv6Enabled() *gomock.Call { } // IsVnetManaged mocks base method. -func (m *MockClusterScoper) IsVnetManaged() bool { +func (m *MockClusterScoper) IsVnetManaged() (bool, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "IsVnetManaged") ret0, _ := ret[0].(bool) - return ret0 + ret1, _ := ret[1].(error) + return ret0, ret1 } // IsVnetManaged indicates an expected call of IsVnetManaged. diff --git a/azure/scope/cluster.go b/azure/scope/cluster.go index 56b6fce782aa..c5ede06ca928 100644 --- a/azure/scope/cluster.go +++ b/azure/scope/cluster.go @@ -36,6 +36,8 @@ import ( infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1alpha4" "sigs.k8s.io/cluster-api-provider-azure/azure" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/securitygroups" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/virtualnetworks" "sigs.k8s.io/cluster-api-provider-azure/util/futures" ) @@ -90,6 +92,7 @@ func NewClusterScope(ctx context.Context, params ClusterScopeParams) (*ClusterSc Cluster: params.Cluster, AzureCluster: params.AzureCluster, patchHelper: helper, + cache: &clusterCache{}, }, nil } @@ -102,6 +105,12 @@ type ClusterScope struct { AzureClients Cluster *clusterv1.Cluster AzureCluster *infrav1.AzureCluster + cache *clusterCache +} + +// clusterCache stores common cluster information so we don't have to hit the API multiple times within the same reconcile loop. +type clusterCache struct { + IsVnetManaged *bool } // BaseURI returns the Azure ResourceManagerEndpoint. @@ -243,12 +252,14 @@ func (s *ClusterScope) NatGatewaySpecs() []azure.NatGatewaySpec { } // NSGSpecs returns the security group specs. -func (s *ClusterScope) NSGSpecs() []azure.NSGSpec { - nsgspecs := []azure.NSGSpec{} +func (s *ClusterScope) NSGSpecs() []azure.ResourceSpecGetter { + nsgspecs := []azure.ResourceSpecGetter{} for _, subnet := range s.AzureCluster.Spec.NetworkSpec.Subnets { - nsgspecs = append(nsgspecs, azure.NSGSpec{ + nsgspecs = append(nsgspecs, &securitygroups.NSGSpec{ Name: subnet.SecurityGroup.Name, SecurityRules: subnet.SecurityGroup.SecurityRules, + ResourceGroup: s.ResourceGroup(), + Location: s.Location(), }) } @@ -287,11 +298,14 @@ func (s *ClusterScope) SubnetSpecs() []azure.SubnetSpec { } // VNetSpec returns the virtual network spec. -func (s *ClusterScope) VNetSpec() azure.VNetSpec { - return azure.VNetSpec{ - ResourceGroup: s.Vnet().ResourceGroup, - Name: s.Vnet().Name, - CIDRs: s.Vnet().CIDRBlocks, +func (s *ClusterScope) VNetSpec() azure.ResourceSpecGetter { + return &virtualnetworks.VNetSpec{ + ResourceGroup: s.Vnet().ResourceGroup, + Name: s.Vnet().Name, + CIDRs: s.Vnet().CIDRBlocks, + Location: s.Location(), + ClusterName: s.ClusterName(), + AdditionalTags: s.AdditionalTags(), } } @@ -336,8 +350,16 @@ func (s *ClusterScope) Vnet() *infrav1.VnetSpec { } // IsVnetManaged returns true if the vnet is managed. -func (s *ClusterScope) IsVnetManaged() bool { - return s.Vnet().ID == "" || s.Vnet().Tags.HasOwned(s.ClusterName()) +func (s *ClusterScope) IsVnetManaged() (bool, error) { + if s.cache.IsVnetManaged != nil { + return to.Bool(s.cache.IsVnetManaged), nil + } + return false, errors.New("could not determine if vnet is managed") +} + +// SetVnetManagedCache stores the value of VNet management in the cluster cache so it can be accessed later in the reconcile. +func (s *ClusterScope) SetVnetManagedCache(managed bool) { + s.cache.IsVnetManaged = &managed } // IsIPv6Enabled returns true if IPv6 is enabled. @@ -539,13 +561,20 @@ func (s *ClusterScope) ListOptionsLabelSelector() client.ListOption { // PatchObject persists the cluster configuration and status. func (s *ClusterScope) PatchObject(ctx context.Context) error { - conditions.SetSummary(s.AzureCluster) + conditions.SetSummary(s.AzureCluster, + conditions.WithConditions( + infrav1.VNetReadyCondition, + infrav1.SecurityGroupsReadyCondition, + ), + ) return s.patchHelper.Patch( ctx, s.AzureCluster, patch.WithOwnedConditions{Conditions: []clusterv1.ConditionType{ clusterv1.ReadyCondition, + infrav1.VNetReadyCondition, + infrav1.SecurityGroupsReadyCondition, }}) } diff --git a/azure/scope/managedcontrolplane.go b/azure/scope/managedcontrolplane.go index 13b93bdf62bd..960570f09a73 100644 --- a/azure/scope/managedcontrolplane.go +++ b/azure/scope/managedcontrolplane.go @@ -24,6 +24,7 @@ import ( "strings" "github.com/Azure/go-autorest/autorest" + "github.com/Azure/go-autorest/autorest/to" "github.com/go-logr/logr" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" @@ -38,6 +39,7 @@ import ( infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1alpha4" "sigs.k8s.io/cluster-api-provider-azure/azure" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/virtualnetworks" infrav1exp "sigs.k8s.io/cluster-api-provider-azure/exp/api/v1alpha4" "sigs.k8s.io/cluster-api-provider-azure/util/futures" ) @@ -100,6 +102,7 @@ func NewManagedControlPlaneScope(ctx context.Context, params ManagedControlPlane InfraMachinePool: params.InfraMachinePool, PatchTarget: params.PatchTarget, patchHelper: helper, + cache: &clusterCache{}, }, nil } @@ -116,6 +119,7 @@ type ManagedControlPlaneScope struct { ControlPlane *infrav1exp.AzureManagedControlPlane InfraMachinePool *infrav1exp.AzureManagedMachinePool PatchTarget client.Object + cache *clusterCache SystemNodePools []infrav1exp.AzureManagedMachinePool } @@ -198,11 +202,14 @@ func (s *ManagedControlPlaneScope) Vnet() *infrav1.VnetSpec { } // VNetSpec returns the virtual network spec. -func (s *ManagedControlPlaneScope) VNetSpec() azure.VNetSpec { - return azure.VNetSpec{ - ResourceGroup: s.Vnet().ResourceGroup, - Name: s.Vnet().Name, - CIDRs: s.Vnet().CIDRBlocks, +func (s *ManagedControlPlaneScope) VNetSpec() azure.ResourceSpecGetter { + return &virtualnetworks.VNetSpec{ + ResourceGroup: s.Vnet().ResourceGroup, + Name: s.Vnet().Name, + CIDRs: s.Vnet().CIDRBlocks, + Location: s.Location(), + ClusterName: s.ClusterName(), + AdditionalTags: s.AdditionalTags(), } } @@ -284,8 +291,16 @@ func (s *ManagedControlPlaneScope) IsIPv6Enabled() bool { } // IsVnetManaged returns true if the vnet is managed. -func (s *ManagedControlPlaneScope) IsVnetManaged() bool { - return true +func (s *ManagedControlPlaneScope) IsVnetManaged() (bool, error) { + if s.cache.IsVnetManaged != nil { + return to.Bool(s.cache.IsVnetManaged), nil + } + return false, errors.New("could not determine if vnet is managed") +} + +// SetVnetManagedCache stores the value of VNet management in the cluster cache so it can be accessed later in the reconcile. +func (s *ManagedControlPlaneScope) SetVnetManagedCache(managed bool) { + s.cache.IsVnetManaged = &managed } // APIServerLBName returns the API Server LB name. diff --git a/azure/services/bastionhosts/mocks_bastionhosts/bastionhosts_mock.go b/azure/services/bastionhosts/mocks_bastionhosts/bastionhosts_mock.go index 0683f524cfc6..894a681ec103 100644 --- a/azure/services/bastionhosts/mocks_bastionhosts/bastionhosts_mock.go +++ b/azure/services/bastionhosts/mocks_bastionhosts/bastionhosts_mock.go @@ -354,11 +354,12 @@ func (mr *MockBastionScopeMockRecorder) IsIPv6Enabled() *gomock.Call { } // IsVnetManaged mocks base method. -func (m *MockBastionScope) IsVnetManaged() bool { +func (m *MockBastionScope) IsVnetManaged() (bool, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "IsVnetManaged") ret0, _ := ret[0].(bool) - return ret0 + ret1, _ := ret[1].(error) + return ret0, ret1 } // IsVnetManaged indicates an expected call of IsVnetManaged. diff --git a/azure/services/loadbalancers/mock_loadbalancers/loadbalancers_mock.go b/azure/services/loadbalancers/mock_loadbalancers/loadbalancers_mock.go index afa1ab5312d6..7d7e96573da7 100644 --- a/azure/services/loadbalancers/mock_loadbalancers/loadbalancers_mock.go +++ b/azure/services/loadbalancers/mock_loadbalancers/loadbalancers_mock.go @@ -340,11 +340,12 @@ func (mr *MockLBScopeMockRecorder) IsIPv6Enabled() *gomock.Call { } // IsVnetManaged mocks base method. -func (m *MockLBScope) IsVnetManaged() bool { +func (m *MockLBScope) IsVnetManaged() (bool, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "IsVnetManaged") ret0, _ := ret[0].(bool) - return ret0 + ret1, _ := ret[1].(error) + return ret0, ret1 } // IsVnetManaged indicates an expected call of IsVnetManaged. diff --git a/azure/services/natgateways/mock_natgateways/natgateways_mock.go b/azure/services/natgateways/mock_natgateways/natgateways_mock.go index 41ca0506173f..35c63bae11b5 100644 --- a/azure/services/natgateways/mock_natgateways/natgateways_mock.go +++ b/azure/services/natgateways/mock_natgateways/natgateways_mock.go @@ -340,11 +340,12 @@ func (mr *MockNatGatewayScopeMockRecorder) IsIPv6Enabled() *gomock.Call { } // IsVnetManaged mocks base method. -func (m *MockNatGatewayScope) IsVnetManaged() bool { +func (m *MockNatGatewayScope) IsVnetManaged() (bool, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "IsVnetManaged") ret0, _ := ret[0].(bool) - return ret0 + ret1, _ := ret[1].(error) + return ret0, ret1 } // IsVnetManaged indicates an expected call of IsVnetManaged. diff --git a/azure/services/natgateways/natgateways.go b/azure/services/natgateways/natgateways.go index 925f4d841962..a9aeefb0ea61 100644 --- a/azure/services/natgateways/natgateways.go +++ b/azure/services/natgateways/natgateways.go @@ -58,7 +58,10 @@ func (s *Service) Reconcile(ctx context.Context) error { ctx, span := tele.Tracer().Start(ctx, "natgateways.Service.Reconcile") defer span.End() - if !s.Scope.Vnet().IsManaged(s.Scope.ClusterName()) { + managed, err := s.Scope.IsVnetManaged() + if err != nil { + return errors.Wrap(err, "failed to check if vnet is managed") + } else if !managed { s.Scope.V(4).Info("Skipping nat gateways reconcile in custom vnet mode") return nil } @@ -152,7 +155,10 @@ func (s *Service) Delete(ctx context.Context) error { ctx, span := tele.Tracer().Start(ctx, "natgateways.Service.Delete") defer span.End() - if !s.Scope.Vnet().IsManaged(s.Scope.ClusterName()) { + managed, err := s.Scope.IsVnetManaged() + if err != nil { + return errors.Wrap(err, "failed to check if vnet is managed") + } else if !managed { s.Scope.V(4).Info("Skipping nat gateway deletion in custom vnet mode") return nil } diff --git a/azure/services/natgateways/natgateways_test.go b/azure/services/natgateways/natgateways_test.go index d811c4a7e5a7..9cd784b5d7ab 100644 --- a/azure/services/natgateways/natgateways_test.go +++ b/azure/services/natgateways/natgateways_test.go @@ -43,41 +43,23 @@ func init() { func TestReconcileNatGateways(t *testing.T) { testcases := []struct { name string - tags infrav1.Tags expectedError string expect func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) }{ { - name: "nat gateways in custom vnet mode", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "shared", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "nat gateways in custom vnet mode", expectedError: "", expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - ID: "1234", - Name: "my-vnet", - }) + s.IsVnetManaged().Return(false, nil) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName() }, }, { - name: "nat gateway create successfully", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "nat gateway create successfully", expectedError: "", expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) + s.IsVnetManaged().Return(true, nil) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName() s.NatGatewaySpecs().Return([]azure.NatGatewaySpec{ { Name: "my-node-natgateway", @@ -108,19 +90,11 @@ func TestReconcileNatGateways(t *testing.T) { }, }, { - name: "update nat gateway if actual state does not match desired state", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "update nat gateway if actual state does not match desired state", expectedError: "", expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) + s.IsVnetManaged().Return(true, nil) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName() s.NatGatewaySpecs().Return([]azure.NatGatewaySpec{ { Name: "my-node-natgateway", @@ -159,19 +133,11 @@ func TestReconcileNatGateways(t *testing.T) { }, }, { - name: "nat gateway is not updated if it's up to date", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "nat gateway is not updated if it's up to date", expectedError: "", expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) + s.IsVnetManaged().Return(true, nil) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName() s.NatGatewaySpecs().Return([]azure.NatGatewaySpec{ { Name: "my-node-natgateway", @@ -212,19 +178,11 @@ func TestReconcileNatGateways(t *testing.T) { }, }, { - name: "fail when getting existing nat gateway", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "fail when getting existing nat gateway", expectedError: "failed to get nat gateway my-node-natgateway in my-rg: #: Internal Server Error: StatusCode=500", expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) + s.IsVnetManaged().Return(true, nil) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName() s.NatGatewaySpecs().Return([]azure.NatGatewaySpec{ { Name: "my-node-natgateway", @@ -240,19 +198,11 @@ func TestReconcileNatGateways(t *testing.T) { }, }, { - name: "fail to create a nat gateway", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "fail to create a nat gateway", expectedError: "failed to create nat gateway my-node-natgateway in resource group my-rg: #: Internal Server Error: StatusCode=500", expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) + s.IsVnetManaged().Return(true, nil) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName() s.NatGatewaySpecs().Return([]azure.NatGatewaySpec{ { Name: "my-node-natgateway", @@ -302,41 +252,23 @@ func TestReconcileNatGateways(t *testing.T) { func TestDeleteNatGateway(t *testing.T) { testcases := []struct { name string - tags infrav1.Tags expectedError string expect func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) }{ { - name: "nat gateways in custom vnet mode", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "shared", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "nat gateways in custom vnet mode", expectedError: "", expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - ID: "1234", - Name: "my-vnet", - }) + s.IsVnetManaged().Return(false, nil) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName() }, }, { - name: "nat gateway deleted successfully", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "nat gateway deleted successfully", expectedError: "", expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) + s.IsVnetManaged().Return(true, nil) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName() s.NatGatewaySpecs().Return([]azure.NatGatewaySpec{ { Name: "my-node-natgateway", @@ -351,19 +283,11 @@ func TestDeleteNatGateway(t *testing.T) { }, }, { - name: "nat gateway already deleted", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "nat gateway already deleted", expectedError: "", expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) + s.IsVnetManaged().Return(true, nil) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName() s.NatGatewaySpecs().Return([]azure.NatGatewaySpec{ { Name: "my-node-natgateway", @@ -380,19 +304,11 @@ func TestDeleteNatGateway(t *testing.T) { }, }, { - name: "nat gateway deletion fails", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "nat gateway deletion fails", expectedError: "failed to delete nat gateway my-node-natgateway in resource group my-rg: #: Internal Server Error: StatusCode=500", expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, m *mock_natgateways.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) + s.IsVnetManaged().Return(true, nil) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName() s.NatGatewaySpecs().Return([]azure.NatGatewaySpec{ { Name: "my-node-natgateway", diff --git a/azure/services/routetables/mock_routetables/routetables_mock.go b/azure/services/routetables/mock_routetables/routetables_mock.go index 4ebc9949f099..40735a47475c 100644 --- a/azure/services/routetables/mock_routetables/routetables_mock.go +++ b/azure/services/routetables/mock_routetables/routetables_mock.go @@ -340,11 +340,12 @@ func (mr *MockRouteTableScopeMockRecorder) IsIPv6Enabled() *gomock.Call { } // IsVnetManaged mocks base method. -func (m *MockRouteTableScope) IsVnetManaged() bool { +func (m *MockRouteTableScope) IsVnetManaged() (bool, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "IsVnetManaged") ret0, _ := ret[0].(bool) - return ret0 + ret1, _ := ret[1].(error) + return ret0, ret1 } // IsVnetManaged indicates an expected call of IsVnetManaged. diff --git a/azure/services/routetables/routetables.go b/azure/services/routetables/routetables.go index 60917b3beef2..b6c9874c1af0 100644 --- a/azure/services/routetables/routetables.go +++ b/azure/services/routetables/routetables.go @@ -56,7 +56,10 @@ func (s *Service) Reconcile(ctx context.Context) error { ctx, span := tele.Tracer().Start(ctx, "routetables.Service.Reconcile") defer span.End() - if !s.Scope.Vnet().IsManaged(s.Scope.ClusterName()) { + managed, err := s.Scope.IsVnetManaged() + if err != nil { + return errors.Wrap(err, "failed to check if vnet is managed") + } else if !managed { s.Scope.V(4).Info("Skipping route tables reconcile in custom vnet mode") return nil } @@ -100,7 +103,10 @@ func (s *Service) Delete(ctx context.Context) error { ctx, span := tele.Tracer().Start(ctx, "routetables.Service.Delete") defer span.End() - if !s.Scope.Vnet().IsManaged(s.Scope.ClusterName()) { + managed, err := s.Scope.IsVnetManaged() + if err != nil { + return errors.Wrap(err, "failed to check if vnet is managed") + } else if !managed { s.Scope.V(4).Info("Skipping route table deletion in custom vnet mode") return nil } diff --git a/azure/services/routetables/routetables_test.go b/azure/services/routetables/routetables_test.go index 7afdad7c4f9e..b80a37f1fda4 100644 --- a/azure/services/routetables/routetables_test.go +++ b/azure/services/routetables/routetables_test.go @@ -43,41 +43,23 @@ func init() { func TestReconcileRouteTables(t *testing.T) { testcases := []struct { name string - tags infrav1.Tags expectedError string expect func(s *mock_routetables.MockRouteTableScopeMockRecorder, m *mock_routetables.MockclientMockRecorder) }{ { - name: "route tables in custom vnet mode", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "shared", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "route tables in custom vnet mode", expectedError: "", expect: func(s *mock_routetables.MockRouteTableScopeMockRecorder, m *mock_routetables.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - ID: "1234", - Name: "my-vnet", - }) + s.IsVnetManaged().Return(false, nil) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName() }, }, { - name: "route table create successfully", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "route table create successfully", expectedError: "", expect: func(s *mock_routetables.MockRouteTableScopeMockRecorder, m *mock_routetables.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) + s.IsVnetManaged().Return(true, nil) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName() s.RouteTableSpecs().Return([]azure.RouteTableSpec{ { Name: "my-cp-routetable", @@ -105,19 +87,11 @@ func TestReconcileRouteTables(t *testing.T) { }, }, { - name: "do not create route table if already exists", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "do not create route table if already exists", expectedError: "", expect: func(s *mock_routetables.MockRouteTableScopeMockRecorder, m *mock_routetables.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) + s.IsVnetManaged().Return(true, nil) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName() s.RouteTableSpecs().AnyTimes().Return([]azure.RouteTableSpec{ { Name: "my-cp-routetable", @@ -165,19 +139,11 @@ func TestReconcileRouteTables(t *testing.T) { }, }, { - name: "fail when getting existing route table", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "fail when getting existing route table", expectedError: "failed to get route table my-cp-routetable in my-rg: #: Internal Server Error: StatusCode=500", expect: func(s *mock_routetables.MockRouteTableScopeMockRecorder, m *mock_routetables.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) + s.IsVnetManaged().Return(true, nil) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName() s.RouteTableSpecs().Return([]azure.RouteTableSpec{{ Name: "my-cp-routetable", Subnet: infrav1.SubnetSpec{ @@ -193,19 +159,11 @@ func TestReconcileRouteTables(t *testing.T) { }, }, { - name: "fail to create a route table", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "fail to create a route table", expectedError: "failed to create route table my-cp-routetable in resource group my-rg: #: Internal Server Error: StatusCode=500", expect: func(s *mock_routetables.MockRouteTableScopeMockRecorder, m *mock_routetables.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) + s.IsVnetManaged().Return(true, nil) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName() s.RouteTableSpecs().Return([]azure.RouteTableSpec{{ Name: "my-cp-routetable", Subnet: infrav1.SubnetSpec{ @@ -254,41 +212,23 @@ func TestReconcileRouteTables(t *testing.T) { func TestDeleteRouteTable(t *testing.T) { testcases := []struct { name string - tags infrav1.Tags expectedError string expect func(s *mock_routetables.MockRouteTableScopeMockRecorder, m *mock_routetables.MockclientMockRecorder) }{ { - name: "route tables in custom vnet mode", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "shared", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "route tables in custom vnet mode", expectedError: "", expect: func(s *mock_routetables.MockRouteTableScopeMockRecorder, m *mock_routetables.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - ID: "1234", - Name: "my-vnet", - }) + s.IsVnetManaged().Return(false, nil) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName() }, }, { - name: "route table deleted successfully", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "route table deleted successfully", expectedError: "", expect: func(s *mock_routetables.MockRouteTableScopeMockRecorder, m *mock_routetables.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) + s.IsVnetManaged().Return(true, nil) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName() s.RouteTableSpecs().Return([]azure.RouteTableSpec{ { Name: "my-cp-routetable", @@ -313,19 +253,11 @@ func TestDeleteRouteTable(t *testing.T) { }, }, { - name: "route table already deleted", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "route table already deleted", expectedError: "", expect: func(s *mock_routetables.MockRouteTableScopeMockRecorder, m *mock_routetables.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) + s.IsVnetManaged().Return(true, nil) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName() s.RouteTableSpecs().Return([]azure.RouteTableSpec{ { Name: "my-cp-routetable", @@ -350,19 +282,11 @@ func TestDeleteRouteTable(t *testing.T) { }, }, { - name: "route table deletion fails", - tags: infrav1.Tags{ - "Name": "my-vnet", - "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }, + name: "route table deletion fails", expectedError: "failed to delete route table my-cp-routetable in resource group my-rg: #: Internal Server Error: StatusCode=500", expect: func(s *mock_routetables.MockRouteTableScopeMockRecorder, m *mock_routetables.MockclientMockRecorder) { - s.Vnet().Return(&infrav1.VnetSpec{ - Name: "my-vnet", - }) + s.IsVnetManaged().Return(true, nil) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName() s.RouteTableSpecs().Return([]azure.RouteTableSpec{{ Name: "my-cp-routetable", Subnet: infrav1.SubnetSpec{ diff --git a/azure/services/securitygroups/client.go b/azure/services/securitygroups/client.go index e52d7447ac25..fe6ec5ce8a9c 100644 --- a/azure/services/securitygroups/client.go +++ b/azure/services/securitygroups/client.go @@ -21,16 +21,20 @@ import ( "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network" "github.com/Azure/go-autorest/autorest" + azureautorest "github.com/Azure/go-autorest/autorest/azure" + "github.com/pkg/errors" "sigs.k8s.io/cluster-api-provider-azure/azure" + "sigs.k8s.io/cluster-api-provider-azure/util/reconciler" "sigs.k8s.io/cluster-api-provider-azure/util/tele" ) // client wraps go-sdk. type client interface { Get(context.Context, string, string) (network.SecurityGroup, error) - CreateOrUpdate(context.Context, string, string, network.SecurityGroup) error - Delete(context.Context, string, string) error + CreateOrUpdateAsync(context.Context, azure.ResourceSpecGetter) (azureautorest.FutureAPI, error) + DeleteAsync(context.Context, azure.ResourceSpecGetter) (azureautorest.FutureAPI, error) + IsDone(context.Context, azureautorest.FutureAPI) (bool, error) } // azureClient contains the Azure go-sdk Client. @@ -61,19 +65,29 @@ func (ac *azureClient) Get(ctx context.Context, resourceGroupName, sgName string return ac.securitygroups.Get(ctx, resourceGroupName, sgName, "") } -// CreateOrUpdate creates or updates a network security group in the specified resource group. -func (ac *azureClient) CreateOrUpdate(ctx context.Context, resourceGroupName string, sgName string, sg network.SecurityGroup) error { +// CreateOrUpdateAsync creates or updates a network security group in the specified resource group. +// 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) (azureautorest.FutureAPI, error) { ctx, span := tele.Tracer().Start(ctx, "securitygroups.AzureClient.CreateOrUpdate") defer span.End() + sg, err := ac.SecurityGroupParams(ctx, spec) + if err != nil { + return nil, err + } else if sg == nil { + // nothing to do here. + return nil, nil + } + var etag string if sg.Etag != nil { etag = *sg.Etag } - req, err := ac.securitygroups.CreateOrUpdatePreparer(ctx, resourceGroupName, sgName, sg) + req, err := ac.securitygroups.CreateOrUpdatePreparer(ctx, spec.ResourceGroupName(), spec.ResourceName(), *sg) if err != nil { err = autorest.NewErrorWithError(err, "network.SecurityGroupsClient", "CreateOrUpdate", nil, "Failure preparing request") - return err + return nil, err } if etag != "" { req.Header.Add("If-Match", etag) @@ -82,30 +96,90 @@ func (ac *azureClient) CreateOrUpdate(ctx context.Context, resourceGroupName str future, err := ac.securitygroups.CreateOrUpdateSender(req) if err != nil { err = autorest.NewErrorWithError(err, "network.SecurityGroupsClient", "CreateOrUpdate", future.Response(), "Failure sending request") - return err + return nil, err } + ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureCallTimeout) + defer cancel() + err = future.WaitForCompletionRef(ctx, ac.securitygroups.Client) if err != nil { - return err + // if an error occurs, return the future. + // this means the long-running operation didn't finish in the specified timeout. + return &future, err } _, err = future.Result(ac.securitygroups) - return err + // if the operation completed, return a nil future. + return nil, err } -// Delete deletes the specified network security group. -func (ac *azureClient) Delete(ctx context.Context, resourceGroupName, sgName string) error { - ctx, span := tele.Tracer().Start(ctx, "securitygroups.AzureClient.Delete") +// Delete deletes the specified network security group. DeleteAsync sends a DELETE +// request to Azure and if accepted without error, the func will return a Future which can be used to track the ongoing +// progress of the operation. +func (ac *azureClient) DeleteAsync(ctx context.Context, spec azure.ResourceSpecGetter) (azureautorest.FutureAPI, error) { + ctx, span := tele.Tracer().Start(ctx, "securitygroups.AzureClient.DeleteAsync") defer span.End() - future, err := ac.securitygroups.Delete(ctx, resourceGroupName, sgName) + future, err := ac.securitygroups.Delete(ctx, spec.ResourceGroupName(), spec.ResourceName()) if err != nil { - return err + return nil, err } + + ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureCallTimeout) + defer cancel() + err = future.WaitForCompletionRef(ctx, ac.securitygroups.Client) if err != nil { - return err + // if an error occurs, return the future. + // this means the long-running operation didn't finish in the specified timeout. + return &future, err } _, err = future.Result(ac.securitygroups) - return err + // if the operation completed, return a nil future. + return nil, err +} + +// IsDone returns true if the long-running operation has completed. +func (ac *azureClient) IsDone(ctx context.Context, future azureautorest.FutureAPI) (bool, error) { + ctx, span := tele.Tracer().Start(ctx, "securitygroups.AzureClient.IsDone") + defer span.End() + + done, err := future.DoneWithContext(ctx, ac.securitygroups) + if err != nil { + return false, errors.Wrap(err, "failed checking if the operation was complete") + } + + return done, nil +} + +func (ac *azureClient) SecurityGroupParams(ctx context.Context, spec azure.ResourceSpecGetter) (*network.SecurityGroup, error) { + var params interface{} + + existingNSG, err := ac.Get(ctx, spec.ResourceGroupName(), spec.ResourceName()) + if azure.ResourceNotFound(err) { + // NSG doesn't exist, create it from scratch. + params, err = spec.Parameters(nil) + if err != nil { + return nil, errors.Wrapf(err, "failed to get desired parameters for nsg %s", spec.ResourceName()) + } + } else if err != nil { + return nil, errors.Wrapf(err, "failed to get NSG %s in %s", spec.ResourceName(), spec.ResourceGroupName()) + } else { + // NSG already exists + params, err = spec.Parameters(existingNSG) + if err != nil { + return nil, err + } + } + + sg, ok := params.(network.SecurityGroup) + if !ok { + if params == nil { + // nothing to do here. + return nil, nil + } + return nil, errors.Errorf("%T is not a network.SecurityGroup", params) + } + + return &sg, nil } diff --git a/azure/services/securitygroups/mock_securitygroups/client_mock.go b/azure/services/securitygroups/mock_securitygroups/client_mock.go index d36127423066..bcf0b788ed38 100644 --- a/azure/services/securitygroups/mock_securitygroups/client_mock.go +++ b/azure/services/securitygroups/mock_securitygroups/client_mock.go @@ -25,7 +25,9 @@ import ( 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. @@ -51,32 +53,34 @@ func (m *Mockclient) EXPECT() *MockclientMockRecorder { return m.recorder } -// CreateOrUpdate mocks base method. -func (m *Mockclient) CreateOrUpdate(arg0 context.Context, arg1, arg2 string, arg3 network.SecurityGroup) error { +// CreateOrUpdateAsync mocks base method. +func (m *Mockclient) CreateOrUpdateAsync(arg0 context.Context, arg1 azure0.ResourceSpecGetter) (azure.FutureAPI, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateOrUpdate", arg0, arg1, arg2, arg3) - ret0, _ := ret[0].(error) - return ret0 + ret := m.ctrl.Call(m, "CreateOrUpdateAsync", arg0, arg1) + ret0, _ := ret[0].(azure.FutureAPI) + ret1, _ := ret[1].(error) + return ret0, ret1 } -// CreateOrUpdate indicates an expected call of CreateOrUpdate. -func (mr *MockclientMockRecorder) CreateOrUpdate(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { +// 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, "CreateOrUpdate", reflect.TypeOf((*Mockclient)(nil).CreateOrUpdate), arg0, arg1, arg2, arg3) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdateAsync", reflect.TypeOf((*Mockclient)(nil).CreateOrUpdateAsync), arg0, arg1) } -// Delete mocks base method. -func (m *Mockclient) Delete(arg0 context.Context, arg1, arg2 string) error { +// 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, "Delete", arg0, arg1, arg2) - ret0, _ := ret[0].(error) - return ret0 + ret := m.ctrl.Call(m, "DeleteAsync", arg0, arg1) + ret0, _ := ret[0].(azure.FutureAPI) + ret1, _ := ret[1].(error) + return ret0, ret1 } -// Delete indicates an expected call of Delete. -func (mr *MockclientMockRecorder) Delete(arg0, arg1, arg2 interface{}) *gomock.Call { +// 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, "Delete", reflect.TypeOf((*Mockclient)(nil).Delete), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAsync", reflect.TypeOf((*Mockclient)(nil).DeleteAsync), arg0, arg1) } // Get mocks base method. @@ -93,3 +97,18 @@ func (mr *MockclientMockRecorder) Get(arg0, arg1, arg2 interface{}) *gomock.Call mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*Mockclient)(nil).Get), arg0, arg1, arg2) } + +// IsDone mocks base method. +func (m *Mockclient) IsDone(arg0 context.Context, arg1 azure.FutureAPI) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "IsDone", arg0, arg1) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// IsDone indicates an expected call of IsDone. +func (mr *MockclientMockRecorder) IsDone(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsDone", reflect.TypeOf((*Mockclient)(nil).IsDone), arg0, arg1) +} diff --git a/azure/services/securitygroups/mock_securitygroups/securitygroups_mock.go b/azure/services/securitygroups/mock_securitygroups/securitygroups_mock.go index 07c323e1b97e..85e9f86c2916 100644 --- a/azure/services/securitygroups/mock_securitygroups/securitygroups_mock.go +++ b/azure/services/securitygroups/mock_securitygroups/securitygroups_mock.go @@ -28,6 +28,7 @@ import ( gomock "github.com/golang/mock/gomock" v1alpha4 "sigs.k8s.io/cluster-api-provider-azure/api/v1alpha4" azure "sigs.k8s.io/cluster-api-provider-azure/azure" + v1alpha40 "sigs.k8s.io/cluster-api/api/v1alpha4" ) // MockNSGScope is a mock of NSGScope interface. @@ -53,48 +54,6 @@ func (m *MockNSGScope) EXPECT() *MockNSGScopeMockRecorder { return m.recorder } -// APIServerLBName mocks base method. -func (m *MockNSGScope) APIServerLBName() string { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "APIServerLBName") - ret0, _ := ret[0].(string) - return ret0 -} - -// APIServerLBName indicates an expected call of APIServerLBName. -func (mr *MockNSGScopeMockRecorder) APIServerLBName() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "APIServerLBName", reflect.TypeOf((*MockNSGScope)(nil).APIServerLBName)) -} - -// APIServerLBPoolName mocks base method. -func (m *MockNSGScope) APIServerLBPoolName(arg0 string) string { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "APIServerLBPoolName", arg0) - ret0, _ := ret[0].(string) - return ret0 -} - -// APIServerLBPoolName indicates an expected call of APIServerLBPoolName. -func (mr *MockNSGScopeMockRecorder) APIServerLBPoolName(arg0 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "APIServerLBPoolName", reflect.TypeOf((*MockNSGScope)(nil).APIServerLBPoolName), arg0) -} - -// AdditionalTags mocks base method. -func (m *MockNSGScope) AdditionalTags() v1alpha4.Tags { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AdditionalTags") - ret0, _ := ret[0].(v1alpha4.Tags) - return ret0 -} - -// AdditionalTags indicates an expected call of AdditionalTags. -func (mr *MockNSGScopeMockRecorder) AdditionalTags() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AdditionalTags", reflect.TypeOf((*MockNSGScope)(nil).AdditionalTags)) -} - // Authorizer mocks base method. func (m *MockNSGScope) Authorizer() autorest.Authorizer { m.ctrl.T.Helper() @@ -109,20 +68,6 @@ func (mr *MockNSGScopeMockRecorder) Authorizer() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Authorizer", reflect.TypeOf((*MockNSGScope)(nil).Authorizer)) } -// AvailabilitySetEnabled mocks base method. -func (m *MockNSGScope) AvailabilitySetEnabled() bool { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AvailabilitySetEnabled") - ret0, _ := ret[0].(bool) - return ret0 -} - -// AvailabilitySetEnabled indicates an expected call of AvailabilitySetEnabled. -func (mr *MockNSGScopeMockRecorder) AvailabilitySetEnabled() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AvailabilitySetEnabled", reflect.TypeOf((*MockNSGScope)(nil).AvailabilitySetEnabled)) -} - // BaseURI mocks base method. func (m *MockNSGScope) BaseURI() string { m.ctrl.T.Helper() @@ -179,60 +124,16 @@ func (mr *MockNSGScopeMockRecorder) CloudEnvironment() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CloudEnvironment", reflect.TypeOf((*MockNSGScope)(nil).CloudEnvironment)) } -// CloudProviderConfigOverrides mocks base method. -func (m *MockNSGScope) CloudProviderConfigOverrides() *v1alpha4.CloudProviderConfigOverrides { +// DeleteLongRunningOperationState mocks base method. +func (m *MockNSGScope) DeleteLongRunningOperationState(arg0, arg1 string) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CloudProviderConfigOverrides") - ret0, _ := ret[0].(*v1alpha4.CloudProviderConfigOverrides) - return ret0 + m.ctrl.Call(m, "DeleteLongRunningOperationState", arg0, arg1) } -// CloudProviderConfigOverrides indicates an expected call of CloudProviderConfigOverrides. -func (mr *MockNSGScopeMockRecorder) CloudProviderConfigOverrides() *gomock.Call { +// DeleteLongRunningOperationState indicates an expected call of DeleteLongRunningOperationState. +func (mr *MockNSGScopeMockRecorder) DeleteLongRunningOperationState(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CloudProviderConfigOverrides", reflect.TypeOf((*MockNSGScope)(nil).CloudProviderConfigOverrides)) -} - -// ClusterName mocks base method. -func (m *MockNSGScope) ClusterName() string { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ClusterName") - ret0, _ := ret[0].(string) - return ret0 -} - -// ClusterName indicates an expected call of ClusterName. -func (mr *MockNSGScopeMockRecorder) ClusterName() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ClusterName", reflect.TypeOf((*MockNSGScope)(nil).ClusterName)) -} - -// ControlPlaneRouteTable mocks base method. -func (m *MockNSGScope) ControlPlaneRouteTable() v1alpha4.RouteTable { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ControlPlaneRouteTable") - ret0, _ := ret[0].(v1alpha4.RouteTable) - return ret0 -} - -// ControlPlaneRouteTable indicates an expected call of ControlPlaneRouteTable. -func (mr *MockNSGScopeMockRecorder) ControlPlaneRouteTable() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ControlPlaneRouteTable", reflect.TypeOf((*MockNSGScope)(nil).ControlPlaneRouteTable)) -} - -// ControlPlaneSubnet mocks base method. -func (m *MockNSGScope) ControlPlaneSubnet() v1alpha4.SubnetSpec { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ControlPlaneSubnet") - ret0, _ := ret[0].(v1alpha4.SubnetSpec) - return ret0 -} - -// ControlPlaneSubnet indicates an expected call of ControlPlaneSubnet. -func (mr *MockNSGScopeMockRecorder) ControlPlaneSubnet() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ControlPlaneSubnet", reflect.TypeOf((*MockNSGScope)(nil).ControlPlaneSubnet)) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteLongRunningOperationState", reflect.TypeOf((*MockNSGScope)(nil).DeleteLongRunningOperationState), arg0, arg1) } // Enabled mocks base method. @@ -266,18 +167,18 @@ func (mr *MockNSGScopeMockRecorder) Error(err, msg interface{}, keysAndValues .. return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Error", reflect.TypeOf((*MockNSGScope)(nil).Error), varargs...) } -// GetPrivateDNSZoneName mocks base method. -func (m *MockNSGScope) GetPrivateDNSZoneName() string { +// GetLongRunningOperationState mocks base method. +func (m *MockNSGScope) GetLongRunningOperationState(arg0, arg1 string) *v1alpha4.Future { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetPrivateDNSZoneName") - ret0, _ := ret[0].(string) + ret := m.ctrl.Call(m, "GetLongRunningOperationState", arg0, arg1) + ret0, _ := ret[0].(*v1alpha4.Future) return ret0 } -// GetPrivateDNSZoneName indicates an expected call of GetPrivateDNSZoneName. -func (mr *MockNSGScopeMockRecorder) GetPrivateDNSZoneName() *gomock.Call { +// GetLongRunningOperationState indicates an expected call of GetLongRunningOperationState. +func (mr *MockNSGScopeMockRecorder) GetLongRunningOperationState(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetPrivateDNSZoneName", reflect.TypeOf((*MockNSGScope)(nil).GetPrivateDNSZoneName)) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetLongRunningOperationState", reflect.TypeOf((*MockNSGScope)(nil).GetLongRunningOperationState), arg0, arg1) } // HashKey mocks base method. @@ -311,40 +212,13 @@ func (mr *MockNSGScopeMockRecorder) Info(msg interface{}, keysAndValues ...inter return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Info", reflect.TypeOf((*MockNSGScope)(nil).Info), varargs...) } -// IsAPIServerPrivate mocks base method. -func (m *MockNSGScope) IsAPIServerPrivate() bool { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "IsAPIServerPrivate") - ret0, _ := ret[0].(bool) - return ret0 -} - -// IsAPIServerPrivate indicates an expected call of IsAPIServerPrivate. -func (mr *MockNSGScopeMockRecorder) IsAPIServerPrivate() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsAPIServerPrivate", reflect.TypeOf((*MockNSGScope)(nil).IsAPIServerPrivate)) -} - -// IsIPv6Enabled mocks base method. -func (m *MockNSGScope) IsIPv6Enabled() bool { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "IsIPv6Enabled") - ret0, _ := ret[0].(bool) - return ret0 -} - -// IsIPv6Enabled indicates an expected call of IsIPv6Enabled. -func (mr *MockNSGScopeMockRecorder) IsIPv6Enabled() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsIPv6Enabled", reflect.TypeOf((*MockNSGScope)(nil).IsIPv6Enabled)) -} - // IsVnetManaged mocks base method. -func (m *MockNSGScope) IsVnetManaged() bool { +func (m *MockNSGScope) IsVnetManaged() (bool, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "IsVnetManaged") ret0, _ := ret[0].(bool) - return ret0 + ret1, _ := ret[1].(error) + return ret0, ret1 } // IsVnetManaged indicates an expected call of IsVnetManaged. @@ -353,25 +227,11 @@ func (mr *MockNSGScopeMockRecorder) IsVnetManaged() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsVnetManaged", reflect.TypeOf((*MockNSGScope)(nil).IsVnetManaged)) } -// Location mocks base method. -func (m *MockNSGScope) Location() string { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Location") - ret0, _ := ret[0].(string) - return ret0 -} - -// Location indicates an expected call of Location. -func (mr *MockNSGScopeMockRecorder) Location() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Location", reflect.TypeOf((*MockNSGScope)(nil).Location)) -} - // NSGSpecs mocks base method. -func (m *MockNSGScope) NSGSpecs() []azure.NSGSpec { +func (m *MockNSGScope) NSGSpecs() []azure.ResourceSpecGetter { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "NSGSpecs") - ret0, _ := ret[0].([]azure.NSGSpec) + ret0, _ := ret[0].([]azure.ResourceSpecGetter) return ret0 } @@ -381,128 +241,80 @@ func (mr *MockNSGScopeMockRecorder) NSGSpecs() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NSGSpecs", reflect.TypeOf((*MockNSGScope)(nil).NSGSpecs)) } -// NodeSubnets mocks base method. -func (m *MockNSGScope) NodeSubnets() []v1alpha4.SubnetSpec { +// SetLongRunningOperationState mocks base method. +func (m *MockNSGScope) SetLongRunningOperationState(arg0 *v1alpha4.Future) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "NodeSubnets") - ret0, _ := ret[0].([]v1alpha4.SubnetSpec) - return ret0 + m.ctrl.Call(m, "SetLongRunningOperationState", arg0) } -// NodeSubnets indicates an expected call of NodeSubnets. -func (mr *MockNSGScopeMockRecorder) NodeSubnets() *gomock.Call { +// SetLongRunningOperationState indicates an expected call of SetLongRunningOperationState. +func (mr *MockNSGScopeMockRecorder) SetLongRunningOperationState(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NodeSubnets", reflect.TypeOf((*MockNSGScope)(nil).NodeSubnets)) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetLongRunningOperationState", reflect.TypeOf((*MockNSGScope)(nil).SetLongRunningOperationState), arg0) } -// OutboundLBName mocks base method. -func (m *MockNSGScope) OutboundLBName(arg0 string) string { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "OutboundLBName", arg0) - ret0, _ := ret[0].(string) - return ret0 -} - -// OutboundLBName indicates an expected call of OutboundLBName. -func (mr *MockNSGScopeMockRecorder) OutboundLBName(arg0 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "OutboundLBName", reflect.TypeOf((*MockNSGScope)(nil).OutboundLBName), arg0) -} - -// OutboundPoolName mocks base method. -func (m *MockNSGScope) OutboundPoolName(arg0 string) string { +// SubscriptionID mocks base method. +func (m *MockNSGScope) SubscriptionID() string { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "OutboundPoolName", arg0) + ret := m.ctrl.Call(m, "SubscriptionID") ret0, _ := ret[0].(string) return ret0 } -// OutboundPoolName indicates an expected call of OutboundPoolName. -func (mr *MockNSGScopeMockRecorder) OutboundPoolName(arg0 interface{}) *gomock.Call { +// SubscriptionID indicates an expected call of SubscriptionID. +func (mr *MockNSGScopeMockRecorder) SubscriptionID() *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "OutboundPoolName", reflect.TypeOf((*MockNSGScope)(nil).OutboundPoolName), arg0) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SubscriptionID", reflect.TypeOf((*MockNSGScope)(nil).SubscriptionID)) } -// ResourceGroup mocks base method. -func (m *MockNSGScope) ResourceGroup() string { +// TenantID mocks base method. +func (m *MockNSGScope) TenantID() string { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ResourceGroup") + ret := m.ctrl.Call(m, "TenantID") ret0, _ := ret[0].(string) return ret0 } -// ResourceGroup indicates an expected call of ResourceGroup. -func (mr *MockNSGScopeMockRecorder) ResourceGroup() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResourceGroup", reflect.TypeOf((*MockNSGScope)(nil).ResourceGroup)) -} - -// SetSubnet mocks base method. -func (m *MockNSGScope) SetSubnet(arg0 v1alpha4.SubnetSpec) { - m.ctrl.T.Helper() - m.ctrl.Call(m, "SetSubnet", arg0) -} - -// SetSubnet indicates an expected call of SetSubnet. -func (mr *MockNSGScopeMockRecorder) SetSubnet(arg0 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetSubnet", reflect.TypeOf((*MockNSGScope)(nil).SetSubnet), arg0) -} - -// Subnet mocks base method. -func (m *MockNSGScope) Subnet(arg0 string) v1alpha4.SubnetSpec { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Subnet", arg0) - ret0, _ := ret[0].(v1alpha4.SubnetSpec) - return ret0 -} - -// Subnet indicates an expected call of Subnet. -func (mr *MockNSGScopeMockRecorder) Subnet(arg0 interface{}) *gomock.Call { +// TenantID indicates an expected call of TenantID. +func (mr *MockNSGScopeMockRecorder) TenantID() *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Subnet", reflect.TypeOf((*MockNSGScope)(nil).Subnet), arg0) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "TenantID", reflect.TypeOf((*MockNSGScope)(nil).TenantID)) } -// Subnets mocks base method. -func (m *MockNSGScope) Subnets() v1alpha4.Subnets { +// UpdateDeleteStatus mocks base method. +func (m *MockNSGScope) UpdateDeleteStatus(arg0 v1alpha40.ConditionType, arg1 string, arg2 error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Subnets") - ret0, _ := ret[0].(v1alpha4.Subnets) - return ret0 + m.ctrl.Call(m, "UpdateDeleteStatus", arg0, arg1, arg2) } -// Subnets indicates an expected call of Subnets. -func (mr *MockNSGScopeMockRecorder) Subnets() *gomock.Call { +// UpdateDeleteStatus indicates an expected call of UpdateDeleteStatus. +func (mr *MockNSGScopeMockRecorder) UpdateDeleteStatus(arg0, arg1, arg2 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Subnets", reflect.TypeOf((*MockNSGScope)(nil).Subnets)) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateDeleteStatus", reflect.TypeOf((*MockNSGScope)(nil).UpdateDeleteStatus), arg0, arg1, arg2) } -// SubscriptionID mocks base method. -func (m *MockNSGScope) SubscriptionID() string { +// UpdatePatchStatus mocks base method. +func (m *MockNSGScope) UpdatePatchStatus(arg0 v1alpha40.ConditionType, arg1 string, arg2 error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "SubscriptionID") - ret0, _ := ret[0].(string) - return ret0 + m.ctrl.Call(m, "UpdatePatchStatus", arg0, arg1, arg2) } -// SubscriptionID indicates an expected call of SubscriptionID. -func (mr *MockNSGScopeMockRecorder) SubscriptionID() *gomock.Call { +// UpdatePatchStatus indicates an expected call of UpdatePatchStatus. +func (mr *MockNSGScopeMockRecorder) UpdatePatchStatus(arg0, arg1, arg2 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SubscriptionID", reflect.TypeOf((*MockNSGScope)(nil).SubscriptionID)) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdatePatchStatus", reflect.TypeOf((*MockNSGScope)(nil).UpdatePatchStatus), arg0, arg1, arg2) } -// TenantID mocks base method. -func (m *MockNSGScope) TenantID() string { +// UpdatePutStatus mocks base method. +func (m *MockNSGScope) UpdatePutStatus(arg0 v1alpha40.ConditionType, arg1 string, arg2 error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "TenantID") - ret0, _ := ret[0].(string) - return ret0 + m.ctrl.Call(m, "UpdatePutStatus", arg0, arg1, arg2) } -// TenantID indicates an expected call of TenantID. -func (mr *MockNSGScopeMockRecorder) TenantID() *gomock.Call { +// UpdatePutStatus indicates an expected call of UpdatePutStatus. +func (mr *MockNSGScopeMockRecorder) UpdatePutStatus(arg0, arg1, arg2 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "TenantID", reflect.TypeOf((*MockNSGScope)(nil).TenantID)) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdatePutStatus", reflect.TypeOf((*MockNSGScope)(nil).UpdatePutStatus), arg0, arg1, arg2) } // V mocks base method. @@ -519,20 +331,6 @@ func (mr *MockNSGScopeMockRecorder) V(level interface{}) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "V", reflect.TypeOf((*MockNSGScope)(nil).V), level) } -// Vnet mocks base method. -func (m *MockNSGScope) Vnet() *v1alpha4.VnetSpec { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Vnet") - ret0, _ := ret[0].(*v1alpha4.VnetSpec) - return ret0 -} - -// Vnet indicates an expected call of Vnet. -func (mr *MockNSGScopeMockRecorder) Vnet() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Vnet", reflect.TypeOf((*MockNSGScope)(nil).Vnet)) -} - // WithName mocks base method. func (m *MockNSGScope) WithName(name string) logr.Logger { m.ctrl.T.Helper() diff --git a/azure/services/securitygroups/securitygroups.go b/azure/services/securitygroups/securitygroups.go index 684e44b8e868..c7f7c6dd91b1 100644 --- a/azure/services/securitygroups/securitygroups.go +++ b/azure/services/securitygroups/securitygroups.go @@ -18,24 +18,26 @@ package securitygroups import ( "context" - "strings" - "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network" - "github.com/Azure/go-autorest/autorest/to" "github.com/go-logr/logr" "github.com/pkg/errors" + infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1alpha4" "sigs.k8s.io/cluster-api-provider-azure/azure" - "sigs.k8s.io/cluster-api-provider-azure/azure/converters" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/async" + "sigs.k8s.io/cluster-api-provider-azure/util/reconciler" "sigs.k8s.io/cluster-api-provider-azure/util/tele" ) +const serviceName = "securitygroups" + // NSGScope defines the scope interface for a security groups service. type NSGScope interface { logr.Logger - azure.ClusterDescriber - azure.NetworkDescriber - NSGSpecs() []azure.NSGSpec + azure.Authorizer + azure.AsyncStatusUpdater + NSGSpecs() []azure.ResourceSpecGetter + IsVnetManaged() (bool, error) } // Service provides operations on Azure resources. @@ -52,111 +54,72 @@ func New(scope NSGScope) *Service { } } -// Reconcile gets/creates/updates a network security group. +// Reconcile gets/creates/updates network security groups. func (s *Service) Reconcile(ctx context.Context) error { ctx, span := tele.Tracer().Start(ctx, "securitygroups.Service.Reconcile") defer span.End() - if !s.Scope.IsVnetManaged() { - s.Scope.V(4).Info("Skipping network security group reconcile in custom VNet mode") + ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout) + defer cancel() + + // Only create the NSGs if their lifecycle is managed by this controller. + // NSGs are managed if and only if the vnet is managed. + managed, err := s.Scope.IsVnetManaged() + if err != nil { + return errors.Wrap(err, "failed to determine if network security groups are managed") + } else if !managed { + s.Scope.V(4).Info("Skipping network security groups reconcile in custom VNet mode") return nil } + // We go through the list of NSGSpecs to reconcile each one, independently of the result of the previous one. + // If multiple erros occur, we return the most pressing one + // order of precedence is: error creating -> creating in progress -> created (no error) + var result error + for _, nsgSpec := range s.Scope.NSGSpecs() { - securityRules := make([]network.SecurityRule, 0) - var etag *string - - existingNSG, err := s.client.Get(ctx, s.Scope.ResourceGroup(), nsgSpec.Name) - switch { - case err != nil && !azure.ResourceNotFound(err): - return errors.Wrapf(err, "failed to get NSG %s in %s", nsgSpec.Name, s.Scope.ResourceGroup()) - case err == nil: - // security group already exists - // We append the existing NSG etag to the header to ensure we only apply the updates if the NSG has not been modified. - etag = existingNSG.Etag - // Check if the expected rules are present - update := false - securityRules = *existingNSG.SecurityRules - for _, rule := range nsgSpec.SecurityRules { - sdkRule := converters.SecurityRuleToSDK(rule) - if !ruleExists(securityRules, sdkRule) { - update = true - securityRules = append(securityRules, sdkRule) - } - } - if !update { - // Skip update for NSG as the required default rules are present - s.Scope.V(2).Info("security group exists and no default rules are missing, skipping update", "security group", nsgSpec.Name) - continue - } - default: - s.Scope.V(2).Info("creating security group", "security group", nsgSpec.Name) - for _, rule := range nsgSpec.SecurityRules { - securityRules = append(securityRules, converters.SecurityRuleToSDK(rule)) + if err := async.CreateResource(ctx, s.Scope, s.client, nsgSpec, serviceName); err != nil { + if !azure.IsOperationNotDoneError(err) || result == nil { + result = err } } - sg := network.SecurityGroup{ - Location: to.StringPtr(s.Scope.Location()), - SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{ - SecurityRules: &securityRules, - }, - Etag: etag, - } - err = s.client.CreateOrUpdate(ctx, s.Scope.ResourceGroup(), nsgSpec.Name, sg) - if err != nil { - return errors.Wrapf(err, "failed to create or update security group %s in resource group %s", nsgSpec.Name, s.Scope.ResourceGroup()) - } - - s.Scope.V(2).Info("successfully created or updated security group", "security group", nsgSpec.Name) } - return nil -} -func ruleExists(rules []network.SecurityRule, rule network.SecurityRule) bool { - for _, existingRule := range rules { - if !strings.EqualFold(to.String(existingRule.Name), to.String(rule.Name)) { - continue - } - if !strings.EqualFold(to.String(existingRule.DestinationPortRange), to.String(rule.DestinationPortRange)) { - continue - } - if existingRule.Protocol != network.SecurityRuleProtocolTCP && - existingRule.Access != network.SecurityRuleAccessAllow && - existingRule.Direction != network.SecurityRuleDirectionInbound { - continue - } - if !strings.EqualFold(to.String(existingRule.SourcePortRange), "*") && - !strings.EqualFold(to.String(existingRule.SourceAddressPrefix), "*") && - !strings.EqualFold(to.String(existingRule.DestinationAddressPrefix), "*") { - continue - } - return true - } - return false + s.Scope.UpdatePutStatus(infrav1.SecurityGroupsReadyCondition, serviceName, result) + return result } -// Delete deletes the network security group with the provided name. +// Delete deletes network security groups. func (s *Service) Delete(ctx context.Context) error { ctx, span := tele.Tracer().Start(ctx, "securitygroups.Service.Delete") defer span.End() - if !s.Scope.IsVnetManaged() { - s.Scope.V(4).Info("Skipping network security group delete in custom VNet mode") + ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout) + defer cancel() + + // Only delete the NSG if its lifecycle is managed by this controller. + // NSGs are managed if and only if the vnet is managed. + managed, err := s.Scope.IsVnetManaged() + if err != nil { + return errors.Wrap(err, "failed to determine if network security groups are managed") + } else if !managed { + s.Scope.V(4).Info("Skipping network security groups delete in custom VNet mode") return nil } + var result error + + // We go through the list of NSGSpecs to delete each one, independently of the result of the previous one. + // If multiple erros occur, we return the most pressing one + // order of precedence is: error deleting -> deleting in progress -> deleted (no error) for _, nsgSpec := range s.Scope.NSGSpecs() { - s.Scope.V(2).Info("deleting security group", "security group", nsgSpec.Name) - err := s.client.Delete(ctx, s.Scope.ResourceGroup(), nsgSpec.Name) - if err != nil && azure.ResourceNotFound(err) { - // already deleted - continue - } - if err != nil { - return errors.Wrapf(err, "failed to delete security group %s in resource group %s", nsgSpec.Name, s.Scope.ResourceGroup()) + if err := async.DeleteResource(ctx, s.Scope, s.client, nsgSpec, serviceName); err != nil { + if !azure.IsOperationNotDoneError(err) || result == nil { + result = err + } } - - s.Scope.V(2).Info("successfully deleted security group", "security group", nsgSpec.Name) } - return nil + + s.Scope.UpdateDeleteStatus(infrav1.SecurityGroupsReadyCondition, serviceName, result) + return result } diff --git a/azure/services/securitygroups/securitygroups_test.go b/azure/services/securitygroups/securitygroups_test.go index f3fdb8ce2a7a..81927b7780f6 100644 --- a/azure/services/securitygroups/securitygroups_test.go +++ b/azure/services/securitygroups/securitygroups_test.go @@ -22,213 +22,131 @@ import ( "testing" "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network" - "github.com/Azure/go-autorest/autorest" + azureautorest "github.com/Azure/go-autorest/autorest/azure" + "github.com/pkg/errors" + "github.com/Azure/go-autorest/autorest/to" "github.com/golang/mock/gomock" . "github.com/onsi/gomega" "k8s.io/klog/v2/klogr" - infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1alpha4" "sigs.k8s.io/cluster-api-provider-azure/azure" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/securitygroups/mock_securitygroups" gomockinternal "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock" ) +var ( + fakeNSG = NSGSpec{ + Name: "test-nsg", + Location: "test-location", + SecurityRules: infrav1.SecurityRules{ + { + Name: "allow_ssh", + Description: "Allow SSH", + Priority: 2200, + Protocol: infrav1.SecurityGroupProtocolTCP, + Direction: infrav1.SecurityRuleDirectionInbound, + Source: to.StringPtr("*"), + SourcePorts: to.StringPtr("*"), + Destination: to.StringPtr("*"), + DestinationPorts: to.StringPtr("22"), + }, + { + Name: "other_rule", + Description: "Test Rule", + Priority: 500, + Protocol: infrav1.SecurityGroupProtocolTCP, + Direction: infrav1.SecurityRuleDirectionInbound, + Source: to.StringPtr("*"), + SourcePorts: to.StringPtr("*"), + Destination: to.StringPtr("*"), + DestinationPorts: to.StringPtr("80"), + }, + }, + ResourceGroup: "test-group", + } + fakeNSG2 = NSGSpec{ + Name: "test-nsg-2", + Location: "test-location", + SecurityRules: infrav1.SecurityRules{}, + ResourceGroup: "test-group", + } + fakeNSGSpecs = []azure.ResourceSpecGetter{&fakeNSG, &fakeNSG2} + 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") +) + func TestReconcileSecurityGroups(t *testing.T) { testcases := []struct { - name string - expect func(s *mock_securitygroups.MockNSGScopeMockRecorder, m *mock_securitygroups.MockclientMockRecorder) + name string + expectedError string + expect func(s *mock_securitygroups.MockNSGScopeMockRecorder, m *mock_securitygroups.MockclientMockRecorder) }{ { - name: "security groups do not exist", + name: "create multiple security groups succeeds", + expectedError: "", + expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, m *mock_securitygroups.MockclientMockRecorder) { + s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) + s.IsVnetManaged().Return(true, nil) + s.NSGSpecs().Return(fakeNSGSpecs) + s.GetLongRunningOperationState("test-nsg", serviceName).Return(nil) + m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakeNSG).Return(nil, nil) + s.GetLongRunningOperationState("test-nsg-2", serviceName).Return(nil) + m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakeNSG2).Return(nil, nil) + s.UpdatePutStatus(infrav1.SecurityGroupsReadyCondition, serviceName, nil) + }, + }, + { + name: "first security groups create fails", + expectedError: "failed to create resource test-group/test-nsg (service: securitygroups): this is an error", expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, m *mock_securitygroups.MockclientMockRecorder) { - s.NSGSpecs().Return([]azure.NSGSpec{ - { - Name: "nsg-one", - SecurityRules: infrav1.SecurityRules{ - { - Name: "first-rule", - Description: "a test rule", - Protocol: infrav1.SecurityGroupProtocolAll, - Priority: 400, - SourcePorts: to.StringPtr("*"), - DestinationPorts: to.StringPtr("*"), - Source: to.StringPtr("*"), - Destination: to.StringPtr("*"), - Direction: infrav1.SecurityRuleDirectionInbound, - }, - { - Name: "second-rule", - Description: "another test rule", - Protocol: infrav1.SecurityGroupProtocolAll, - Priority: 450, - SourcePorts: to.StringPtr("*"), - DestinationPorts: to.StringPtr("*"), - Source: to.StringPtr("*"), - Destination: to.StringPtr("*"), - Direction: infrav1.SecurityRuleDirectionInbound, - }, - }, - }, - { - Name: "nsg-two", - SecurityRules: infrav1.SecurityRules{}, - }, - }) - s.IsVnetManaged().Return(true) - s.ResourceGroup().AnyTimes().Return("my-rg") - s.Location().AnyTimes().Return("test-location") s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - m.Get(gomockinternal.AContext(), "my-rg", "nsg-one").Return(network.SecurityGroup{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found")) - m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "nsg-one", gomockinternal.DiffEq(network.SecurityGroup{ - SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{ - SecurityRules: &[]network.SecurityRule{ - { - SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ - Description: to.StringPtr("a test rule"), - SourcePortRange: to.StringPtr("*"), - DestinationPortRange: to.StringPtr("*"), - SourceAddressPrefix: to.StringPtr("*"), - DestinationAddressPrefix: to.StringPtr("*"), - Protocol: "*", - Direction: "Inbound", - Access: "Allow", - Priority: to.Int32Ptr(400), - }, - Name: to.StringPtr("first-rule"), - }, - { - SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ - Description: to.StringPtr("another test rule"), - SourcePortRange: to.StringPtr("*"), - DestinationPortRange: to.StringPtr("*"), - SourceAddressPrefix: to.StringPtr("*"), - DestinationAddressPrefix: to.StringPtr("*"), - Protocol: "*", - Direction: "Inbound", - Access: "Allow", - Priority: to.Int32Ptr(450), - }, - Name: to.StringPtr("second-rule"), - }, - }, - }, - Etag: nil, - Location: to.StringPtr("test-location"), - })) - m.Get(gomockinternal.AContext(), "my-rg", "nsg-two").Return(network.SecurityGroup{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found")) - m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "nsg-two", gomockinternal.DiffEq(network.SecurityGroup{ - SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{ - SecurityRules: &[]network.SecurityRule{}, - }, - Etag: nil, - Location: to.StringPtr("test-location"), - })) + s.IsVnetManaged().Return(true, nil) + s.NSGSpecs().Return(fakeNSGSpecs) + s.GetLongRunningOperationState("test-nsg", serviceName).Return(nil) + m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakeNSG).Return(nil, errFake) + s.GetLongRunningOperationState("test-nsg-2", serviceName).Return(nil) + m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakeNSG2).Return(nil, nil) + s.UpdatePutStatus(infrav1.SecurityGroupsReadyCondition, serviceName, gomockinternal.ErrStrEq("failed to create resource test-group/test-nsg (service: securitygroups): this is an error")) }, - }, { - name: "security group exists", + }, + { + name: "second security groups create not done", + expectedError: "failed to create resource test-group/test-nsg (service: securitygroups): this is an error", + expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, m *mock_securitygroups.MockclientMockRecorder) { + s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) + s.IsVnetManaged().Return(true, nil) + s.NSGSpecs().Return(fakeNSGSpecs) + s.GetLongRunningOperationState("test-nsg", serviceName).Return(nil) + m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakeNSG).Return(nil, errFake) + s.GetLongRunningOperationState("test-nsg-2", serviceName).Return(nil) + m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakeNSG2).Return(&fakeFuture, errFoo) + s.SetLongRunningOperationState(gomock.AssignableToTypeOf(&infrav1.Future{})) + s.UpdatePutStatus(infrav1.SecurityGroupsReadyCondition, serviceName, gomockinternal.ErrStrEq("failed to create resource test-group/test-nsg (service: securitygroups): this is an error")) + }, + }, + { + name: "vnet is not managed", + expectedError: "", expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, m *mock_securitygroups.MockclientMockRecorder) { - s.NSGSpecs().Return([]azure.NSGSpec{ - { - Name: "nsg-one", - SecurityRules: infrav1.SecurityRules{ - { - Name: "first-rule", - Description: "a test rule", - Protocol: "*", - Priority: 400, - SourcePorts: to.StringPtr("*"), - DestinationPorts: to.StringPtr("*"), - Source: to.StringPtr("*"), - Destination: to.StringPtr("*"), - Direction: infrav1.SecurityRuleDirectionOutbound, - }, - }, - }, - { - Name: "nsg-two", - SecurityRules: infrav1.SecurityRules{}, - }, - }) - s.IsVnetManaged().AnyTimes().Return(true) - s.ResourceGroup().AnyTimes().Return("my-rg") - s.Location().AnyTimes().Return("test-location") s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - m.Get(gomockinternal.AContext(), "my-rg", "nsg-one").Return(network.SecurityGroup{ - Response: autorest.Response{}, - SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{ - SecurityRules: &[]network.SecurityRule{ - { - SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ - Description: to.StringPtr("a different rule"), - Protocol: "*", - SourcePortRange: to.StringPtr("*"), - DestinationPortRange: to.StringPtr("*"), - SourceAddressPrefix: to.StringPtr("*"), - DestinationAddressPrefix: to.StringPtr("*"), - Priority: to.Int32Ptr(300), - Access: network.SecurityRuleAccessDeny, - Direction: network.SecurityRuleDirectionOutbound, - }, - Name: to.StringPtr("foo-rule"), - }, - }, - }, - Etag: to.StringPtr("test-etag"), - ID: to.StringPtr("fake/nsg/id"), - Name: to.StringPtr("nsg-one"), - }, nil) - m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "nsg-one", gomockinternal.DiffEq(network.SecurityGroup{ - SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{ - SecurityRules: &[]network.SecurityRule{ - { - SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ - Description: to.StringPtr("a different rule"), - SourcePortRange: to.StringPtr("*"), - DestinationPortRange: to.StringPtr("*"), - SourceAddressPrefix: to.StringPtr("*"), - DestinationAddressPrefix: to.StringPtr("*"), - Protocol: "*", - Direction: "Outbound", - Access: "Deny", - Priority: to.Int32Ptr(300), - }, - Name: to.StringPtr("foo-rule"), - }, - { - SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ - Description: to.StringPtr("a test rule"), - SourcePortRange: to.StringPtr("*"), - DestinationPortRange: to.StringPtr("*"), - SourceAddressPrefix: to.StringPtr("*"), - DestinationAddressPrefix: to.StringPtr("*"), - Protocol: "*", - Direction: "Outbound", - Access: "Allow", - Priority: to.Int32Ptr(400), - }, - Name: to.StringPtr("first-rule"), - }, - }, - }, - Etag: to.StringPtr("test-etag"), - Location: to.StringPtr("test-location"), - })) - m.Get(gomockinternal.AContext(), "my-rg", "nsg-two").Return(network.SecurityGroup{ - Response: autorest.Response{}, - SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{ - SecurityRules: &[]network.SecurityRule{}, - }, - Etag: to.StringPtr("test-etag"), - ID: to.StringPtr("fake/nsg/two/id"), - Name: to.StringPtr("nsg-two"), - }, nil) + s.IsVnetManaged().Return(false, nil) }, - }, { - name: "skipping network security group reconcile in custom VNet mode", + }, + { + name: "fail to check if vnet is managed", + expectedError: "failed to determine if network security groups are managed: this is an error", expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, m *mock_securitygroups.MockclientMockRecorder) { - s.IsVnetManaged().Return(false) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) + s.IsVnetManaged().Return(false, errFake) }, }, } @@ -250,74 +168,80 @@ func TestReconcileSecurityGroups(t *testing.T) { client: clientMock, } - g.Expect(s.Reconcile(context.TODO())).To(Succeed()) + err := s.Reconcile(context.TODO()) + if tc.expectedError != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tc.expectedError)) + } else { + g.Expect(err).NotTo(HaveOccurred()) + } }) } } func TestDeleteSecurityGroups(t *testing.T) { testcases := []struct { - name string - expect func(s *mock_securitygroups.MockNSGScopeMockRecorder, m *mock_securitygroups.MockclientMockRecorder) + name string + expectedError string + expect func(s *mock_securitygroups.MockNSGScopeMockRecorder, m *mock_securitygroups.MockclientMockRecorder) }{ { - name: "security groups exist", + name: "delete multiple security groups succeeds", + expectedError: "", + expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, m *mock_securitygroups.MockclientMockRecorder) { + s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) + s.IsVnetManaged().Return(true, nil) + s.NSGSpecs().Return(fakeNSGSpecs) + s.GetLongRunningOperationState("test-nsg", serviceName).Return(nil) + m.DeleteAsync(gomockinternal.AContext(), &fakeNSG).Return(nil, nil) + s.GetLongRunningOperationState("test-nsg-2", serviceName).Return(nil) + m.DeleteAsync(gomockinternal.AContext(), &fakeNSG2).Return(nil, nil) + s.UpdateDeleteStatus(infrav1.SecurityGroupsReadyCondition, serviceName, nil) + }, + }, + { + name: "first security groups delete fails", + expectedError: "failed to delete resource test-group/test-nsg (service: securitygroups): this is an error", expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, m *mock_securitygroups.MockclientMockRecorder) { - s.NSGSpecs().Return([]azure.NSGSpec{ - { - Name: "nsg-one", - SecurityRules: infrav1.SecurityRules{ - { - Name: "first-rule", - Description: "a test rule", - Protocol: "all", - Priority: 400, - SourcePorts: to.StringPtr("*"), - DestinationPorts: to.StringPtr("*"), - Source: to.StringPtr("*"), - Destination: to.StringPtr("*"), - Direction: infrav1.SecurityRuleDirectionInbound, - }, - }, - }, - { - Name: "nsg-two", - SecurityRules: infrav1.SecurityRules{}, - }, - }) - s.ResourceGroup().AnyTimes().Return("my-rg") s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.IsVnetManaged().Return(true) - m.Delete(gomockinternal.AContext(), "my-rg", "nsg-one") - m.Delete(gomockinternal.AContext(), "my-rg", "nsg-two") + s.IsVnetManaged().Return(true, nil) + s.NSGSpecs().Return(fakeNSGSpecs) + s.GetLongRunningOperationState("test-nsg", serviceName).Return(nil) + m.DeleteAsync(gomockinternal.AContext(), &fakeNSG).Return(nil, errFake) + s.GetLongRunningOperationState("test-nsg-2", serviceName).Return(nil) + m.DeleteAsync(gomockinternal.AContext(), &fakeNSG2).Return(nil, nil) + s.UpdateDeleteStatus(infrav1.SecurityGroupsReadyCondition, serviceName, gomockinternal.ErrStrEq("failed to delete resource test-group/test-nsg (service: securitygroups): this is an error")) }, }, { - name: "security group already deleted", + name: "second security groups create not done", + expectedError: "failed to delete resource test-group/test-nsg (service: securitygroups): this is an error", expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, m *mock_securitygroups.MockclientMockRecorder) { - s.NSGSpecs().Return([]azure.NSGSpec{ - { - Name: "nsg-one", - SecurityRules: infrav1.SecurityRules{}, - }, - { - Name: "nsg-two", - SecurityRules: infrav1.SecurityRules{}, - }, - }) - s.ResourceGroup().AnyTimes().Return("my-rg") s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.IsVnetManaged().Return(true) - m.Delete(gomockinternal.AContext(), "my-rg", "nsg-one"). - Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found")) - m.Delete(gomockinternal.AContext(), "my-rg", "nsg-two") + s.IsVnetManaged().Return(true, nil) + s.NSGSpecs().Return(fakeNSGSpecs) + s.GetLongRunningOperationState("test-nsg", serviceName).Return(nil) + m.DeleteAsync(gomockinternal.AContext(), &fakeNSG).Return(nil, errFake) + s.GetLongRunningOperationState("test-nsg-2", serviceName).Return(nil) + m.DeleteAsync(gomockinternal.AContext(), &fakeNSG2).Return(&fakeFuture, errFoo) + s.SetLongRunningOperationState(gomock.AssignableToTypeOf(&infrav1.Future{})) + s.UpdateDeleteStatus(infrav1.SecurityGroupsReadyCondition, serviceName, gomockinternal.ErrStrEq("failed to delete resource test-group/test-nsg (service: securitygroups): this is an error")) }, }, { - name: "skipping network security group delete in custom VNet mode", + name: "vnet is not managed", + expectedError: "", expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, m *mock_securitygroups.MockclientMockRecorder) { - s.IsVnetManaged().Return(false) s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) + s.IsVnetManaged().Return(false, nil) + }, + }, + { + name: "fail to check if vnet is managed", + expectedError: "failed to determine if network security groups are managed: this is an error", + expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, m *mock_securitygroups.MockclientMockRecorder) { + s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) + s.IsVnetManaged().Return(false, errFake) }, }, } @@ -339,7 +263,92 @@ func TestDeleteSecurityGroups(t *testing.T) { client: clientMock, } - g.Expect(s.Delete(context.TODO())).To(Succeed()) + err := s.Delete(context.TODO()) + if tc.expectedError != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err).To(MatchError(tc.expectedError)) + } else { + g.Expect(err).NotTo(HaveOccurred()) + } + }) + } +} + +var ( + ruleA = network.SecurityRule{ + Name: to.StringPtr("A"), + SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ + Description: to.StringPtr("this is rule A"), + Protocol: network.SecurityRuleProtocolTCP, + DestinationPortRange: to.StringPtr("*"), + SourcePortRange: to.StringPtr("*"), + DestinationAddressPrefix: to.StringPtr("*"), + SourceAddressPrefix: to.StringPtr("*"), + Priority: to.Int32Ptr(100), + Direction: network.SecurityRuleDirectionInbound, + }, + } + ruleB = network.SecurityRule{ + Name: to.StringPtr("B"), + SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ + Description: to.StringPtr("this is rule B"), + Protocol: network.SecurityRuleProtocolTCP, + DestinationPortRange: to.StringPtr("*"), + SourcePortRange: to.StringPtr("*"), + DestinationAddressPrefix: to.StringPtr("*"), + SourceAddressPrefix: to.StringPtr("*"), + Priority: to.Int32Ptr(100), + Direction: network.SecurityRuleDirectionOutbound, + }, + } + ruleBModified = network.SecurityRule{ + Name: to.StringPtr("B"), + SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ + Description: to.StringPtr("this is rule B"), + Protocol: network.SecurityRuleProtocolTCP, + DestinationPortRange: to.StringPtr("80"), + SourcePortRange: to.StringPtr("*"), + DestinationAddressPrefix: to.StringPtr("*"), + SourceAddressPrefix: to.StringPtr("*"), + Priority: to.Int32Ptr(100), + Direction: network.SecurityRuleDirectionOutbound, + }, + } +) + +func TestRuleExists(t *testing.T) { + testcases := []struct { + name string + rules []network.SecurityRule + rule network.SecurityRule + expected bool + }{ + { + name: "rule doesn't exitst", + rules: []network.SecurityRule{ruleA}, + rule: ruleB, + expected: false, + }, + { + name: "rule exists", + rules: []network.SecurityRule{ruleA, ruleB}, + rule: ruleB, + expected: true, + }, + { + name: "rule exists but has been modified", + rules: []network.SecurityRule{ruleA, ruleB}, + rule: ruleBModified, + expected: false, + }, + } + for _, tc := range testcases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + t.Parallel() + result := ruleExists(tc.rules, tc.rule) + g.Expect(result).To(Equal(tc.expected)) }) } } diff --git a/azure/services/securitygroups/spec.go b/azure/services/securitygroups/spec.go new file mode 100644 index 000000000000..69908afaf12c --- /dev/null +++ b/azure/services/securitygroups/spec.go @@ -0,0 +1,117 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package securitygroups + +import ( + "strings" + + "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network" + "github.com/Azure/go-autorest/autorest/to" + "github.com/pkg/errors" + infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1alpha4" + "sigs.k8s.io/cluster-api-provider-azure/azure/converters" +) + +// NSGSpec defines the specification for a Security Group. +type NSGSpec struct { + Name string + SecurityRules infrav1.SecurityRules + Location string + ResourceGroup string +} + +// ResourceName returns the name of the security group. +func (s *NSGSpec) ResourceName() string { + return s.Name +} + +// ResourceGroupName returns the name of the resource group. +func (s *NSGSpec) ResourceGroupName() string { + return s.ResourceGroup +} + +// OwnerResourceName is a no-op for security groups. +func (s *NSGSpec) OwnerResourceName() string { + return "" +} + +// Parameters returns the parameters for the security group. +func (s *NSGSpec) Parameters(existing interface{}) (interface{}, error) { + securityRules := make([]network.SecurityRule, 0) + var etag *string + + if existing != nil { + existingNSG, ok := existing.(network.SecurityGroup) + if !ok { + return nil, errors.Errorf("%T is not a network.SecurityGroup", existing) + } + // security group already exists + // We append the existing NSG etag to the header to ensure we only apply the updates if the NSG has not been modified. + etag = existingNSG.Etag + // Check if the expected rules are present + update := false + securityRules = *existingNSG.SecurityRules + for _, rule := range s.SecurityRules { + sdkRule := converters.SecurityRuleToSDK(rule) + if !ruleExists(securityRules, sdkRule) { + update = true + securityRules = append(securityRules, sdkRule) + } + } + if !update { + // Skip update for NSG as the required default rules are present + return nil, nil + } + } else { + // new security group + for _, rule := range s.SecurityRules { + securityRules = append(securityRules, converters.SecurityRuleToSDK(rule)) + } + } + + return network.SecurityGroup{ + Location: to.StringPtr(s.Location), + SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{ + SecurityRules: &securityRules, + }, + Etag: etag, + }, nil +} + +// TODO: review this logic and make sure it is what we want. It seems incorrect to skip rules that don't have a certain protocol, etc. +func ruleExists(rules []network.SecurityRule, rule network.SecurityRule) bool { + for _, existingRule := range rules { + if !strings.EqualFold(to.String(existingRule.Name), to.String(rule.Name)) { + continue + } + if !strings.EqualFold(to.String(existingRule.DestinationPortRange), to.String(rule.DestinationPortRange)) { + continue + } + if existingRule.Protocol != network.SecurityRuleProtocolTCP && + existingRule.Access != network.SecurityRuleAccessAllow && + existingRule.Direction != network.SecurityRuleDirectionInbound { + continue + } + if !strings.EqualFold(to.String(existingRule.SourcePortRange), "*") && + !strings.EqualFold(to.String(existingRule.SourceAddressPrefix), "*") && + !strings.EqualFold(to.String(existingRule.DestinationAddressPrefix), "*") { + continue + } + return true + } + return false +} diff --git a/azure/services/subnets/mock_subnets/subnets_mock.go b/azure/services/subnets/mock_subnets/subnets_mock.go index 43f6e78565c0..2e280e6a561f 100644 --- a/azure/services/subnets/mock_subnets/subnets_mock.go +++ b/azure/services/subnets/mock_subnets/subnets_mock.go @@ -340,11 +340,12 @@ func (mr *MockSubnetScopeMockRecorder) IsIPv6Enabled() *gomock.Call { } // IsVnetManaged mocks base method. -func (m *MockSubnetScope) IsVnetManaged() bool { +func (m *MockSubnetScope) IsVnetManaged() (bool, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "IsVnetManaged") ret0, _ := ret[0].(bool) - return ret0 + ret1, _ := ret[1].(error) + return ret0, ret1 } // IsVnetManaged indicates an expected call of IsVnetManaged. diff --git a/azure/services/subnets/subnets.go b/azure/services/subnets/subnets.go index 001dfc9b8928..cbaf7b3ce774 100644 --- a/azure/services/subnets/subnets.go +++ b/azure/services/subnets/subnets.go @@ -58,6 +58,7 @@ func (s *Service) Reconcile(ctx context.Context) error { for _, subnetSpec := range s.Scope.SubnetSpecs() { existingSubnet, err := s.getExisting(ctx, s.Scope.Vnet().ResourceGroup, subnetSpec) + switch { case err != nil && !azure.ResourceNotFound(err): return errors.Wrapf(err, "failed to get subnet %s", subnetSpec.Name) @@ -65,11 +66,13 @@ func (s *Service) Reconcile(ctx context.Context) error { // subnet already exists, update the spec and skip creation s.Scope.SetSubnet(*existingSubnet) continue - - case !s.Scope.IsVnetManaged(): - return fmt.Errorf("vnet was provided but subnet %s is missing", subnetSpec.Name) - default: + managed, err := s.Scope.IsVnetManaged() + if err != nil { + return errors.Wrap(err, "failed to check if vnet is managed") + } else if !managed { + return fmt.Errorf("vnet was provided but subnet %s is missing", subnetSpec.Name) + } subnetProperties := network.SubnetPropertiesFormat{ AddressPrefixes: &subnetSpec.CIDRs, @@ -125,13 +128,17 @@ func (s *Service) Delete(ctx context.Context) error { ctx, span := tele.Tracer().Start(ctx, "subnets.Service.Delete") defer span.End() + managed, err := s.Scope.IsVnetManaged() + if err != nil { + return errors.Wrap(err, "failed to check if vnet is managed") + } else if !managed { + s.Scope.V(4).Info("Skipping subnets deletion in custom vnet mode") + return nil + } + for _, subnetSpec := range s.Scope.SubnetSpecs() { - if !s.Scope.Vnet().IsManaged(s.Scope.ClusterName()) { - s.Scope.V(4).Info("Skipping subnets deletion in custom vnet mode") - continue - } s.Scope.V(2).Info("deleting subnet in vnet", "subnet", subnetSpec.Name, "vnet", subnetSpec.VNetName) - err := s.Client.Delete(ctx, s.Scope.Vnet().ResourceGroup, subnetSpec.VNetName, subnetSpec.Name) + err = s.Client.Delete(ctx, s.Scope.Vnet().ResourceGroup, subnetSpec.VNetName, subnetSpec.Name) if err != nil && azure.ResourceNotFound(err) { // already deleted continue diff --git a/azure/services/subnets/subnets_test.go b/azure/services/subnets/subnets_test.go index ec317fa2c7ec..2175d6479e46 100644 --- a/azure/services/subnets/subnets_test.go +++ b/azure/services/subnets/subnets_test.go @@ -62,7 +62,7 @@ func TestReconcileSubnets(t *testing.T) { s.SubscriptionID().AnyTimes().Return("123") s.ResourceGroup().AnyTimes().Return("my-rg") s.IsIPv6Enabled().AnyTimes().Return(false) - s.IsVnetManaged().Return(true) + s.IsVnetManaged().Return(true, nil) m.Get(gomockinternal.AContext(), "", "my-vnet", "my-subnet"). Return(network.Subnet{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found")) m.CreateOrUpdate(gomockinternal.AContext(), "", "my-vnet", "my-subnet", gomockinternal.DiffEq(network.Subnet{ @@ -93,7 +93,7 @@ func TestReconcileSubnets(t *testing.T) { s.ClusterName().AnyTimes().Return("fake-cluster") s.ResourceGroup().AnyTimes().Return("my-rg") s.SubscriptionID().AnyTimes().Return("123") - s.IsVnetManaged().Return(true) + s.IsVnetManaged().Return(true, nil) m.Get(gomockinternal.AContext(), "my-rg", "my-vnet", "my-ipv6-subnet"). Return(network.Subnet{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found")) m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "my-vnet", "my-ipv6-subnet", gomockinternal.DiffEq(network.Subnet{ @@ -128,7 +128,7 @@ func TestReconcileSubnets(t *testing.T) { s.SubscriptionID().AnyTimes().Return("123") s.ResourceGroup().AnyTimes().Return("my-rg") s.IsIPv6Enabled().AnyTimes().Return(false) - s.IsVnetManaged().Return(true) + s.IsVnetManaged().Return(true, nil) m.Get(gomockinternal.AContext(), "", "my-vnet", "my-subnet"). Return(network.Subnet{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found")) m.CreateOrUpdate(gomockinternal.AContext(), "", "my-vnet", "my-subnet", gomock.AssignableToTypeOf(network.Subnet{})).Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")) @@ -180,7 +180,7 @@ func TestReconcileSubnets(t *testing.T) { s.ClusterName().AnyTimes().Return("fake-cluster") s.SubscriptionID().AnyTimes().Return("123") s.ResourceGroup().AnyTimes().Return("custom-vnet-rg") - s.IsVnetManaged().Return(false) + s.IsVnetManaged().Return(false, nil) m.Get(gomockinternal.AContext(), "custom-vnet-rg", "custom-vnet", "my-subnet"). Return(network.Subnet{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found")) }, @@ -537,6 +537,7 @@ func TestDeleteSubnets(t *testing.T) { Role: infrav1.SubnetControlPlane, }, }) + s.IsVnetManaged().Return(true, nil) s.Vnet().AnyTimes().Return(&infrav1.VnetSpec{Name: "my-vnet"}) s.ClusterName().AnyTimes().Return("fake-cluster") s.ResourceGroup().AnyTimes().Return("my-rg") @@ -559,6 +560,7 @@ func TestDeleteSubnets(t *testing.T) { Role: infrav1.SubnetNode, }, }) + s.IsVnetManaged().Return(true, nil) s.Vnet().AnyTimes().Return(&infrav1.VnetSpec{Name: "my-vnet"}) s.ClusterName().AnyTimes().Return("fake-cluster") s.ResourceGroup().AnyTimes().Return("my-rg") @@ -589,6 +591,7 @@ func TestDeleteSubnets(t *testing.T) { Role: infrav1.SubnetControlPlane, }, }) + s.IsVnetManaged().Return(true, nil) s.Vnet().AnyTimes().Return(&infrav1.VnetSpec{Name: "my-vnet"}) s.ClusterName().AnyTimes().Return("fake-cluster") s.ResourceGroup().AnyTimes().Return("my-rg") @@ -602,19 +605,7 @@ func TestDeleteSubnets(t *testing.T) { expectedError: "", expect: func(s *mock_subnets.MockSubnetScopeMockRecorder, m *mock_subnets.MockClientMockRecorder) { s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.SubnetSpecs().Return([]azure.SubnetSpec{ - { - Name: "my-subnet", - CIDRs: []string{"10.0.0.0/16"}, - VNetName: "custom-vnet", - RouteTableName: "my-subnet_route_table", - SecurityGroupName: "my-sg", - Role: infrav1.SubnetNode, - }, - }) - s.Vnet().AnyTimes().Return(&infrav1.VnetSpec{ResourceGroup: "custom-vnet-rg", Name: "custom-vnet", ID: "id1"}) - s.ClusterName().AnyTimes().Return("fake-cluster") - s.ResourceGroup().AnyTimes().Return("my-rg") + s.IsVnetManaged().Return(false, nil) }, }, { @@ -632,6 +623,7 @@ func TestDeleteSubnets(t *testing.T) { Role: infrav1.SubnetNode, }, }) + s.IsVnetManaged().Return(true, nil) s.Vnet().AnyTimes().Return(&infrav1.VnetSpec{Name: "my-vnet", ResourceGroup: "my-rg"}) s.ClusterName().AnyTimes().Return("fake-cluster") s.ResourceGroup().AnyTimes().Return("my-rg") diff --git a/azure/services/virtualnetworks/client.go b/azure/services/virtualnetworks/client.go index 06e3a17df6b7..6a9f1bd90a8c 100644 --- a/azure/services/virtualnetworks/client.go +++ b/azure/services/virtualnetworks/client.go @@ -21,17 +21,21 @@ import ( "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network" "github.com/Azure/go-autorest/autorest" + azureautorest "github.com/Azure/go-autorest/autorest/azure" + "github.com/pkg/errors" "sigs.k8s.io/cluster-api-provider-azure/azure" + "sigs.k8s.io/cluster-api-provider-azure/util/reconciler" "sigs.k8s.io/cluster-api-provider-azure/util/tele" ) // Client wraps go-sdk. type Client interface { Get(context.Context, string, string) (network.VirtualNetwork, error) - CreateOrUpdate(context.Context, string, string, network.VirtualNetwork) error - Delete(context.Context, string, string) error + CreateOrUpdateAsync(context.Context, azure.ResourceSpecGetter) (azureautorest.FutureAPI, error) + DeleteAsync(context.Context, azure.ResourceSpecGetter) (azureautorest.FutureAPI, error) CheckIPAddressAvailability(context.Context, string, string, string) (network.IPAddressAvailabilityResult, error) + IsDone(context.Context, azureautorest.FutureAPI) (bool, error) } // AzureClient contains the Azure go-sdk Client. @@ -64,38 +68,64 @@ func (ac *AzureClient) Get(ctx context.Context, resourceGroupName, vnetName stri return ac.virtualnetworks.Get(ctx, resourceGroupName, vnetName, "") } -// CreateOrUpdate creates or updates a virtual network in the specified resource group. -func (ac *AzureClient) CreateOrUpdate(ctx context.Context, resourceGroupName, vnetName string, vn network.VirtualNetwork) error { - ctx, span := tele.Tracer().Start(ctx, "virtualnetworks.AzureClient.CreateOrUpdate") +// CreateOrUpdateAsync creates or updates a virtual network in the specified resource group 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) (azureautorest.FutureAPI, error) { + ctx, span := tele.Tracer().Start(ctx, "virtualnetworks.AzureClient.CreateOrUpdateAsync") defer span.End() - future, err := ac.virtualnetworks.CreateOrUpdate(ctx, resourceGroupName, vnetName, vn) + vn, err := ac.VirtualNetworkParams(ctx, spec) if err != nil { - return err + return nil, errors.Wrapf(err, "failed to get desired parameters for vnet %s", spec.ResourceName()) + } else if vn == nil { + // nothing to do here + return nil, nil } + + future, err := ac.virtualnetworks.CreateOrUpdate(ctx, spec.ResourceGroupName(), spec.ResourceName(), *vn) + if err != nil { + return nil, err + } + + ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureCallTimeout) + defer cancel() + err = future.WaitForCompletionRef(ctx, ac.virtualnetworks.Client) if err != nil { - return err + // if an error occurs, return the future. + // this means the long-running operation didn't finish in the specified timeout. + return &future, err } _, err = future.Result(ac.virtualnetworks) - return err + // if the operation completed, return a nil future. + return nil, err } -// Delete deletes the specified virtual network. -func (ac *AzureClient) Delete(ctx context.Context, resourceGroupName, vnetName string) error { - ctx, span := tele.Tracer().Start(ctx, "virtualnetworks.AzureClient.Delete") +// DeleteAsync deletes a virtual network asynchronously. DeleteAsync sends a DELETE +// request to Azure and if accepted without error, the func will return a Future which can be used to track the ongoing +// progress of the operation. +func (ac *AzureClient) DeleteAsync(ctx context.Context, spec azure.ResourceSpecGetter) (azureautorest.FutureAPI, error) { + ctx, span := tele.Tracer().Start(ctx, "virtualnetworks.AzureClient.DeleteAsync") defer span.End() - future, err := ac.virtualnetworks.Delete(ctx, resourceGroupName, vnetName) + future, err := ac.virtualnetworks.Delete(ctx, spec.ResourceGroupName(), spec.ResourceName()) if err != nil { - return err + return nil, err } + + ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureCallTimeout) + defer cancel() + err = future.WaitForCompletionRef(ctx, ac.virtualnetworks.Client) if err != nil { - return err + // if an error occurs, return the future. + // this means the long-running operation didn't finish in the specified timeout. + return &future, err } _, err = future.Result(ac.virtualnetworks) - return err + // if the operation completed, return a nil future. + return nil, err } // CheckIPAddressAvailability checks whether a private IP address is available for use. @@ -105,3 +135,49 @@ func (ac *AzureClient) CheckIPAddressAvailability(ctx context.Context, resourceG return ac.virtualnetworks.CheckIPAddressAvailability(ctx, resourceGroupName, vnetName, ip) } + +// 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, "virtualnetworks.AzureClient.IsDone") + defer span.End() + + done, err := future.DoneWithContext(ctx, ac.virtualnetworks) + if err != nil { + return false, errors.Wrap(err, "failed checking if the operation was complete") + } + + return done, nil +} + +// VirtualNetworkParams creates a VirtualNetworkParams object from the given spec. +func (ac *AzureClient) VirtualNetworkParams(ctx context.Context, spec azure.ResourceSpecGetter) (*network.VirtualNetwork, error) { + var params interface{} + + existingVnet, err := ac.Get(ctx, spec.ResourceGroupName(), spec.ResourceName()) + if azure.ResourceNotFound(err) { + // vnet 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 VNet %s in %s", spec.ResourceName(), spec.ResourceGroupName()) + } else { + // vnet already exists + params, err = spec.Parameters(existingVnet) + if err != nil { + return nil, err + } + } + + vn, ok := params.(network.VirtualNetwork) + if !ok { + if params == nil { + // nothing to do here. + return nil, nil + } + return nil, errors.Errorf("%T is not a network.VirtualNetwork", params) + } + + return &vn, nil +} diff --git a/azure/services/virtualnetworks/mock_virtualnetworks/client_mock.go b/azure/services/virtualnetworks/mock_virtualnetworks/client_mock.go index b2f48294cc62..3ee6ee1f2d5e 100644 --- a/azure/services/virtualnetworks/mock_virtualnetworks/client_mock.go +++ b/azure/services/virtualnetworks/mock_virtualnetworks/client_mock.go @@ -25,7 +25,9 @@ import ( 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. @@ -66,32 +68,34 @@ func (mr *MockClientMockRecorder) CheckIPAddressAvailability(arg0, arg1, arg2, a return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CheckIPAddressAvailability", reflect.TypeOf((*MockClient)(nil).CheckIPAddressAvailability), arg0, arg1, arg2, arg3) } -// CreateOrUpdate mocks base method. -func (m *MockClient) CreateOrUpdate(arg0 context.Context, arg1, arg2 string, arg3 network.VirtualNetwork) error { +// CreateOrUpdateAsync mocks base method. +func (m *MockClient) CreateOrUpdateAsync(arg0 context.Context, arg1 azure0.ResourceSpecGetter) (azure.FutureAPI, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateOrUpdate", arg0, arg1, arg2, arg3) - ret0, _ := ret[0].(error) - return ret0 + ret := m.ctrl.Call(m, "CreateOrUpdateAsync", arg0, arg1) + ret0, _ := ret[0].(azure.FutureAPI) + ret1, _ := ret[1].(error) + return ret0, ret1 } -// CreateOrUpdate indicates an expected call of CreateOrUpdate. -func (mr *MockClientMockRecorder) CreateOrUpdate(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { +// 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, "CreateOrUpdate", reflect.TypeOf((*MockClient)(nil).CreateOrUpdate), arg0, arg1, arg2, arg3) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdateAsync", reflect.TypeOf((*MockClient)(nil).CreateOrUpdateAsync), arg0, arg1) } -// Delete mocks base method. -func (m *MockClient) Delete(arg0 context.Context, arg1, arg2 string) error { +// 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, "Delete", arg0, arg1, arg2) - ret0, _ := ret[0].(error) - return ret0 + ret := m.ctrl.Call(m, "DeleteAsync", arg0, arg1) + ret0, _ := ret[0].(azure.FutureAPI) + ret1, _ := ret[1].(error) + return ret0, ret1 } -// Delete indicates an expected call of Delete. -func (mr *MockClientMockRecorder) Delete(arg0, arg1, arg2 interface{}) *gomock.Call { +// 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, "Delete", reflect.TypeOf((*MockClient)(nil).Delete), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAsync", reflect.TypeOf((*MockClient)(nil).DeleteAsync), arg0, arg1) } // Get mocks base method. @@ -108,3 +112,18 @@ func (mr *MockClientMockRecorder) Get(arg0, arg1, arg2 interface{}) *gomock.Call mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockClient)(nil).Get), arg0, arg1, arg2) } + +// IsDone mocks base method. +func (m *MockClient) IsDone(arg0 context.Context, arg1 azure.FutureAPI) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "IsDone", arg0, arg1) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// IsDone indicates an expected call of IsDone. +func (mr *MockClientMockRecorder) IsDone(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsDone", reflect.TypeOf((*MockClient)(nil).IsDone), arg0, arg1) +} diff --git a/azure/services/virtualnetworks/mock_virtualnetworks/virtualnetworks_mock.go b/azure/services/virtualnetworks/mock_virtualnetworks/virtualnetworks_mock.go index 325437d6749d..ba4987800f58 100644 --- a/azure/services/virtualnetworks/mock_virtualnetworks/virtualnetworks_mock.go +++ b/azure/services/virtualnetworks/mock_virtualnetworks/virtualnetworks_mock.go @@ -28,6 +28,7 @@ import ( gomock "github.com/golang/mock/gomock" v1alpha4 "sigs.k8s.io/cluster-api-provider-azure/api/v1alpha4" azure "sigs.k8s.io/cluster-api-provider-azure/azure" + v1alpha40 "sigs.k8s.io/cluster-api/api/v1alpha4" ) // MockVNetScope is a mock of VNetScope interface. @@ -53,20 +54,6 @@ func (m *MockVNetScope) EXPECT() *MockVNetScopeMockRecorder { return m.recorder } -// AdditionalTags mocks base method. -func (m *MockVNetScope) AdditionalTags() v1alpha4.Tags { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AdditionalTags") - ret0, _ := ret[0].(v1alpha4.Tags) - return ret0 -} - -// AdditionalTags indicates an expected call of AdditionalTags. -func (mr *MockVNetScopeMockRecorder) AdditionalTags() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AdditionalTags", reflect.TypeOf((*MockVNetScope)(nil).AdditionalTags)) -} - // Authorizer mocks base method. func (m *MockVNetScope) Authorizer() autorest.Authorizer { m.ctrl.T.Helper() @@ -81,20 +68,6 @@ func (mr *MockVNetScopeMockRecorder) Authorizer() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Authorizer", reflect.TypeOf((*MockVNetScope)(nil).Authorizer)) } -// AvailabilitySetEnabled mocks base method. -func (m *MockVNetScope) AvailabilitySetEnabled() bool { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AvailabilitySetEnabled") - ret0, _ := ret[0].(bool) - return ret0 -} - -// AvailabilitySetEnabled indicates an expected call of AvailabilitySetEnabled. -func (mr *MockVNetScopeMockRecorder) AvailabilitySetEnabled() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AvailabilitySetEnabled", reflect.TypeOf((*MockVNetScope)(nil).AvailabilitySetEnabled)) -} - // BaseURI mocks base method. func (m *MockVNetScope) BaseURI() string { m.ctrl.T.Helper() @@ -151,20 +124,6 @@ func (mr *MockVNetScopeMockRecorder) CloudEnvironment() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CloudEnvironment", reflect.TypeOf((*MockVNetScope)(nil).CloudEnvironment)) } -// CloudProviderConfigOverrides mocks base method. -func (m *MockVNetScope) CloudProviderConfigOverrides() *v1alpha4.CloudProviderConfigOverrides { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CloudProviderConfigOverrides") - ret0, _ := ret[0].(*v1alpha4.CloudProviderConfigOverrides) - return ret0 -} - -// CloudProviderConfigOverrides indicates an expected call of CloudProviderConfigOverrides. -func (mr *MockVNetScopeMockRecorder) CloudProviderConfigOverrides() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CloudProviderConfigOverrides", reflect.TypeOf((*MockVNetScope)(nil).CloudProviderConfigOverrides)) -} - // ClusterName mocks base method. func (m *MockVNetScope) ClusterName() string { m.ctrl.T.Helper() @@ -179,6 +138,18 @@ func (mr *MockVNetScopeMockRecorder) ClusterName() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ClusterName", reflect.TypeOf((*MockVNetScope)(nil).ClusterName)) } +// DeleteLongRunningOperationState mocks base method. +func (m *MockVNetScope) DeleteLongRunningOperationState(arg0, arg1 string) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "DeleteLongRunningOperationState", arg0, arg1) +} + +// DeleteLongRunningOperationState indicates an expected call of DeleteLongRunningOperationState. +func (mr *MockVNetScopeMockRecorder) DeleteLongRunningOperationState(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteLongRunningOperationState", reflect.TypeOf((*MockVNetScope)(nil).DeleteLongRunningOperationState), arg0, arg1) +} + // Enabled mocks base method. func (m *MockVNetScope) Enabled() bool { m.ctrl.T.Helper() @@ -210,6 +181,20 @@ func (mr *MockVNetScopeMockRecorder) Error(err, msg interface{}, keysAndValues . return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Error", reflect.TypeOf((*MockVNetScope)(nil).Error), varargs...) } +// GetLongRunningOperationState mocks base method. +func (m *MockVNetScope) GetLongRunningOperationState(arg0, arg1 string) *v1alpha4.Future { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetLongRunningOperationState", arg0, arg1) + ret0, _ := ret[0].(*v1alpha4.Future) + return ret0 +} + +// GetLongRunningOperationState indicates an expected call of GetLongRunningOperationState. +func (mr *MockVNetScopeMockRecorder) GetLongRunningOperationState(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetLongRunningOperationState", reflect.TypeOf((*MockVNetScope)(nil).GetLongRunningOperationState), arg0, arg1) +} + // HashKey mocks base method. func (m *MockVNetScope) HashKey() string { m.ctrl.T.Helper() @@ -241,32 +226,28 @@ func (mr *MockVNetScopeMockRecorder) Info(msg interface{}, keysAndValues ...inte return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Info", reflect.TypeOf((*MockVNetScope)(nil).Info), varargs...) } -// Location mocks base method. -func (m *MockVNetScope) Location() string { +// SetLongRunningOperationState mocks base method. +func (m *MockVNetScope) SetLongRunningOperationState(arg0 *v1alpha4.Future) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Location") - ret0, _ := ret[0].(string) - return ret0 + m.ctrl.Call(m, "SetLongRunningOperationState", arg0) } -// Location indicates an expected call of Location. -func (mr *MockVNetScopeMockRecorder) Location() *gomock.Call { +// SetLongRunningOperationState indicates an expected call of SetLongRunningOperationState. +func (mr *MockVNetScopeMockRecorder) SetLongRunningOperationState(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Location", reflect.TypeOf((*MockVNetScope)(nil).Location)) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetLongRunningOperationState", reflect.TypeOf((*MockVNetScope)(nil).SetLongRunningOperationState), arg0) } -// ResourceGroup mocks base method. -func (m *MockVNetScope) ResourceGroup() string { +// SetVnetManagedCache mocks base method. +func (m *MockVNetScope) SetVnetManagedCache(arg0 bool) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ResourceGroup") - ret0, _ := ret[0].(string) - return ret0 + m.ctrl.Call(m, "SetVnetManagedCache", arg0) } -// ResourceGroup indicates an expected call of ResourceGroup. -func (mr *MockVNetScopeMockRecorder) ResourceGroup() *gomock.Call { +// SetVnetManagedCache indicates an expected call of SetVnetManagedCache. +func (mr *MockVNetScopeMockRecorder) SetVnetManagedCache(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResourceGroup", reflect.TypeOf((*MockVNetScope)(nil).ResourceGroup)) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetVnetManagedCache", reflect.TypeOf((*MockVNetScope)(nil).SetVnetManagedCache), arg0) } // SubscriptionID mocks base method. @@ -297,6 +278,42 @@ func (mr *MockVNetScopeMockRecorder) TenantID() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "TenantID", reflect.TypeOf((*MockVNetScope)(nil).TenantID)) } +// UpdateDeleteStatus mocks base method. +func (m *MockVNetScope) UpdateDeleteStatus(arg0 v1alpha40.ConditionType, arg1 string, arg2 error) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "UpdateDeleteStatus", arg0, arg1, arg2) +} + +// UpdateDeleteStatus indicates an expected call of UpdateDeleteStatus. +func (mr *MockVNetScopeMockRecorder) UpdateDeleteStatus(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateDeleteStatus", reflect.TypeOf((*MockVNetScope)(nil).UpdateDeleteStatus), arg0, arg1, arg2) +} + +// UpdatePatchStatus mocks base method. +func (m *MockVNetScope) UpdatePatchStatus(arg0 v1alpha40.ConditionType, arg1 string, arg2 error) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "UpdatePatchStatus", arg0, arg1, arg2) +} + +// UpdatePatchStatus indicates an expected call of UpdatePatchStatus. +func (mr *MockVNetScopeMockRecorder) UpdatePatchStatus(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdatePatchStatus", reflect.TypeOf((*MockVNetScope)(nil).UpdatePatchStatus), arg0, arg1, arg2) +} + +// UpdatePutStatus mocks base method. +func (m *MockVNetScope) UpdatePutStatus(arg0 v1alpha40.ConditionType, arg1 string, arg2 error) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "UpdatePutStatus", arg0, arg1, arg2) +} + +// UpdatePutStatus indicates an expected call of UpdatePutStatus. +func (mr *MockVNetScopeMockRecorder) UpdatePutStatus(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdatePutStatus", reflect.TypeOf((*MockVNetScope)(nil).UpdatePutStatus), arg0, arg1, arg2) +} + // V mocks base method. func (m *MockVNetScope) V(level int) logr.Logger { m.ctrl.T.Helper() @@ -312,10 +329,10 @@ func (mr *MockVNetScopeMockRecorder) V(level interface{}) *gomock.Call { } // VNetSpec mocks base method. -func (m *MockVNetScope) VNetSpec() azure.VNetSpec { +func (m *MockVNetScope) VNetSpec() azure.ResourceSpecGetter { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "VNetSpec") - ret0, _ := ret[0].(azure.VNetSpec) + ret0, _ := ret[0].(azure.ResourceSpecGetter) return ret0 } diff --git a/azure/services/virtualnetworks/spec.go b/azure/services/virtualnetworks/spec.go new file mode 100644 index 000000000000..f03a859820a2 --- /dev/null +++ b/azure/services/virtualnetworks/spec.go @@ -0,0 +1,72 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package virtualnetworks + +import ( + "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network" + "github.com/Azure/go-autorest/autorest/to" + infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1alpha4" + "sigs.k8s.io/cluster-api-provider-azure/azure/converters" +) + +// VNetSpec defines the specification for a Virtual Network. +type VNetSpec struct { + ResourceGroup string + Name string + CIDRs []string + Location string + ClusterName string + AdditionalTags infrav1.Tags +} + +// ResourceName returns the name of the vnet. +func (s *VNetSpec) ResourceName() string { + return s.Name +} + +// ResourceGroupName returns the name of the resource group. +func (s *VNetSpec) ResourceGroupName() string { + return s.ResourceGroup +} + +// OwnerResourceName is a no-op for vnets. +func (s *VNetSpec) OwnerResourceName() string { + return "" +} + +// Parameters returns the parameters for the vnet. +func (s *VNetSpec) Parameters(existing interface{}) (interface{}, error) { + if existing != nil { + // vnet already exists, nothing to update. + return nil, nil + } + return network.VirtualNetwork{ + Tags: converters.TagsToMap(infrav1.Build(infrav1.BuildParams{ + ClusterName: s.ClusterName, + Lifecycle: infrav1.ResourceLifecycleOwned, + Name: to.StringPtr(s.Name), + Role: to.StringPtr(infrav1.CommonRole), + Additional: s.AdditionalTags, + })), + Location: to.StringPtr(s.Location), + VirtualNetworkPropertiesFormat: &network.VirtualNetworkPropertiesFormat{ + AddressSpace: &network.AddressSpace{ + AddressPrefixes: &s.CIDRs, + }, + }, + }, nil +} diff --git a/azure/services/virtualnetworks/virtualnetworks.go b/azure/services/virtualnetworks/virtualnetworks.go index 55762a5acad2..335dda179a14 100644 --- a/azure/services/virtualnetworks/virtualnetworks.go +++ b/azure/services/virtualnetworks/virtualnetworks.go @@ -19,23 +19,28 @@ package virtualnetworks import ( "context" - "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network" - "github.com/Azure/go-autorest/autorest/to" "github.com/go-logr/logr" "github.com/pkg/errors" infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1alpha4" "sigs.k8s.io/cluster-api-provider-azure/azure" "sigs.k8s.io/cluster-api-provider-azure/azure/converters" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/async" + "sigs.k8s.io/cluster-api-provider-azure/util/reconciler" "sigs.k8s.io/cluster-api-provider-azure/util/tele" ) +const serviceName = "virtualnetwork" + // VNetScope defines the scope interface for a virtual network service. type VNetScope interface { logr.Logger - azure.ClusterDescriber + azure.Authorizer + azure.AsyncStatusUpdater Vnet() *infrav1.VnetSpec - VNetSpec() azure.VNetSpec + VNetSpec() azure.ResourceSpecGetter + ClusterName() string + SetVnetManagedCache(bool) } // Service provides operations on Azure resources. @@ -53,113 +58,69 @@ func New(scope VNetScope) *Service { } // Reconcile gets/creates/updates a virtual network. -func (s *Service) Reconcile(ctx context.Context) error { +func (s *Service) Reconcile(ctx context.Context) (err error) { ctx, span := tele.Tracer().Start(ctx, "virtualnetworks.Service.Reconcile") defer span.End() - // Following should be created upstream and provided as an input to NewService - // A VNet has following dependencies - // * VNet Cidr - // * Control Plane Subnet Cidr - // * Node Subnet Cidr - // * Control Plane NSG - // * Node NSG - // * Node Route Table - vnetSpec := s.Scope.VNetSpec() - existingVnet, err := s.getExisting(ctx, vnetSpec) + ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout) + defer cancel() - switch { - case err != nil && !azure.ResourceNotFound(err): - return errors.Wrapf(err, "failed to get VNet %s", vnetSpec.Name) + vnetSpec := s.Scope.VNetSpec() - case err == nil: - // vnet already exists, cannot update since it's immutable - if !existingVnet.IsManaged(s.Scope.ClusterName()) { - s.Scope.V(2).Info("Working on custom VNet", "vnet-id", existingVnet.ID) - } - existingVnet.DeepCopyInto(s.Scope.Vnet()) - - default: - s.Scope.V(2).Info("creating VNet", "VNet", vnetSpec.Name) - - vnetProperties := network.VirtualNetwork{ - Tags: converters.TagsToMap(infrav1.Build(infrav1.BuildParams{ - ClusterName: s.Scope.ClusterName(), - Lifecycle: infrav1.ResourceLifecycleOwned, - Name: to.StringPtr(vnetSpec.Name), - Role: to.StringPtr(infrav1.CommonRole), - Additional: s.Scope.AdditionalTags(), - })), - Location: to.StringPtr(s.Scope.Location()), - VirtualNetworkPropertiesFormat: &network.VirtualNetworkPropertiesFormat{ - AddressSpace: &network.AddressSpace{ - AddressPrefixes: &vnetSpec.CIDRs, - }, - }, - } - err = s.Client.CreateOrUpdate(ctx, vnetSpec.ResourceGroup, vnetSpec.Name, vnetProperties) - if err != nil { - return errors.Wrapf(err, "failed to create virtual network %s", vnetSpec.Name) - } - s.Scope.V(2).Info("successfully created VNet", "VNet", vnetSpec.Name) + err = async.CreateResource(ctx, s.Scope, s.Client, vnetSpec, serviceName) + s.Scope.UpdatePutStatus(infrav1.VNetReadyCondition, serviceName, err) + if err != nil { + return err } - - return nil + // check and store whether the vnet is BYO so other services can use the cached information. + _, err = s.IsVnetManaged(ctx) + return err } -// Delete deletes the virtual network with the provided name. +// Delete deletes the virtual network if it is managed by us. func (s *Service) Delete(ctx context.Context) error { ctx, span := tele.Tracer().Start(ctx, "virtualnetworks.Service.Delete") defer span.End() - vnetSpec := s.Scope.VNetSpec() - existingVnet, err := s.getExisting(ctx, vnetSpec) - if azure.ResourceNotFound(err) { - // vnet does not exist, there is nothing to delete - return nil - } + ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout) + defer cancel() - if !existingVnet.IsManaged(s.Scope.ClusterName()) { - s.Scope.V(4).Info("Skipping VNet deletion in custom vnet mode") - return nil - } + vnetSpec := s.Scope.VNetSpec() - s.Scope.V(2).Info("deleting VNet", "VNet", vnetSpec.Name) - err = s.Client.Delete(ctx, vnetSpec.ResourceGroup, vnetSpec.Name) + // Check that the vnet is not BYO. + managed, err := s.IsVnetManaged(ctx) if err != nil { - if azure.ResourceGroupNotFound(err) || azure.ResourceNotFound(err) { + if azure.ResourceNotFound(err) { + // already deleted or doesn't exist, cleanup status and return. + s.Scope.DeleteLongRunningOperationState(vnetSpec.ResourceName(), serviceName) + s.Scope.UpdateDeleteStatus(infrav1.VNetReadyCondition, serviceName, nil) return nil } + return errors.Wrap(err, "could not get VNet management state") } - if err != nil { - return errors.Wrapf(err, "failed to delete VNet %s in resource group %s", vnetSpec.Name, vnetSpec.ResourceGroup) + if !managed { + s.Scope.V(4).Info("Skipping VNet deletion in custom vnet mode") + return nil } - s.Scope.V(2).Info("successfully deleted VNet", "VNet", vnetSpec.Name) - return nil + err = async.DeleteResource(ctx, s.Scope, s.Client, vnetSpec, serviceName) + s.Scope.UpdateDeleteStatus(infrav1.VNetReadyCondition, serviceName, err) + return err } -// getExisting provides information about an existing virtual network. -func (s *Service) getExisting(ctx context.Context, spec azure.VNetSpec) (*infrav1.VnetSpec, error) { - ctx, span := tele.Tracer().Start(ctx, "virtualnetworks.Service.getExisting") +// IsVnetManaged returns true if the virtual network has an owned tag with the cluster name as value, +// meaning that the vnet's lifecycle is managed, and caches the result in scope so that other services that depend on the vnet can check if it is managed. +func (s *Service) IsVnetManaged(ctx context.Context) (bool, error) { + ctx, span := tele.Tracer().Start(ctx, "virtualnetworks.Service.IsVnetManaged") defer span.End() - vnet, err := s.Client.Get(ctx, spec.ResourceGroup, spec.Name) + vnetSpec := s.Scope.VNetSpec() + vnet, err := s.Client.Get(ctx, vnetSpec.ResourceGroupName(), vnetSpec.ResourceName()) if err != nil { - if azure.ResourceNotFound(err) { - return nil, err - } - return nil, errors.Wrapf(err, "failed to get VNet %s", spec.Name) - } - var prefixes []string - if vnet.VirtualNetworkPropertiesFormat != nil && vnet.VirtualNetworkPropertiesFormat.AddressSpace != nil { - prefixes = to.StringSlice(vnet.VirtualNetworkPropertiesFormat.AddressSpace.AddressPrefixes) + return false, err } - return &infrav1.VnetSpec{ - ResourceGroup: spec.ResourceGroup, - ID: to.String(vnet.ID), - Name: to.String(vnet.Name), - CIDRBlocks: prefixes, - Tags: converters.MapToTags(vnet.Tags), - }, nil + tags := converters.MapToTags(vnet.Tags) + managed := tags.HasOwned(s.Scope.ClusterName()) + s.Scope.SetVnetManagedCache(managed) + return managed, nil } diff --git a/azure/services/virtualnetworks/virtualnetworks_test.go b/azure/services/virtualnetworks/virtualnetworks_test.go index a8dc273bbbea..55178379b1bc 100644 --- a/azure/services/virtualnetworks/virtualnetworks_test.go +++ b/azure/services/virtualnetworks/virtualnetworks_test.go @@ -18,22 +18,59 @@ package virtualnetworks import ( "context" + "fmt" "net/http" "testing" "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" "k8s.io/klog/v2/klogr" infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1alpha4" - "sigs.k8s.io/cluster-api-provider-azure/azure" "sigs.k8s.io/cluster-api-provider-azure/azure/services/virtualnetworks/mock_virtualnetworks" gomockinternal "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock" ) +var ( + fakeVNetSpec = VNetSpec{ + ResourceGroup: "test-group", + Name: "test-vnet", + CIDRs: []string{"10.0.0.0/8"}, + Location: "test-location", + ClusterName: "test-cluster", + AdditionalTags: map[string]string{"foo": "bar"}, + } + fakeCreateFuture = infrav1.Future{ + Type: infrav1.PutFuture, + ServiceName: serviceName, + Name: "test-vnet", + ResourceGroup: "test-group", + Data: "eyJtZXRob2QiOiJQVVQiLCJwb2xsaW5nTWV0aG9kIjoiTG9jYXRpb24iLCJscm9TdGF0ZSI6IkluUHJvZ3Jlc3MifQ==", + } + managedVnet = network.VirtualNetwork{ + ID: to.StringPtr("/subscriptions/subscription/resourceGroups/test-group/providers/Microsoft.Network/virtualNetworks/test-vnet"), + Name: to.StringPtr("test-vnet"), + Tags: map[string]*string{ + "foo": to.StringPtr("bar"), + "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": to.StringPtr("owned"), + }, + } + customVnet = network.VirtualNetwork{ + ID: to.StringPtr("/subscriptions/subscription/resourceGroups/test-group/providers/Microsoft.Network/virtualNetworks/test-vnet"), + Name: to.StringPtr("test-vnet"), + Tags: map[string]*string{ + "foo": to.StringPtr("bar"), + "something": to.StringPtr("else"), + }, + } + internalError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error") + notFoundError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not Found") +) + func TestReconcileVnet(t *testing.T) { testcases := []struct { name string @@ -41,206 +78,143 @@ func TestReconcileVnet(t *testing.T) { expect func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_virtualnetworks.MockClientMockRecorder) }{ { - name: "managed vnet exists", + name: "create vnet succeeds", expectedError: "", expect: func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_virtualnetworks.MockClientMockRecorder) { s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName().AnyTimes().Return("fake-cluster") - s.Vnet().AnyTimes().Return(&infrav1.VnetSpec{Name: "vnet-exists"}) - s.VNetSpec().Return(azure.VNetSpec{ - ResourceGroup: "my-rg", - Name: "vnet-exists", - CIDRs: []string{"10.0.0.0/8"}, - }) - m.Get(gomockinternal.AContext(), "my-rg", "vnet-exists"). - Return(network.VirtualNetwork{ - ID: to.StringPtr("azure/fake/id"), - Name: to.StringPtr("vnet-exists"), - VirtualNetworkPropertiesFormat: &network.VirtualNetworkPropertiesFormat{ - AddressSpace: &network.AddressSpace{ - AddressPrefixes: to.StringSlicePtr([]string{"10.0.0.0/8"}), - }, - }, - Tags: map[string]*string{ - "Name": to.StringPtr("vnet-exists"), - "sigs.k8s.io_cluster-api-provider-azure_cluster_fake-cluster": to.StringPtr("owned"), - "sigs.k8s.io_cluster-api-provider-azure_role": to.StringPtr("common"), - }, - }, nil) + s.VNetSpec().Times(2).Return(&fakeVNetSpec) + s.GetLongRunningOperationState("test-vnet", serviceName).Return(nil) + m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakeVNetSpec).Return(nil, nil) + s.UpdatePutStatus(infrav1.VNetReadyCondition, serviceName, nil) + m.Get(gomockinternal.AContext(), "test-group", "test-vnet").Return(managedVnet, nil) + s.ClusterName().Return("test-cluster") + s.SetVnetManagedCache(true) }, }, { - name: "managed ipv6 vnet exists", - expectedError: "", + name: "create vnet fails", + expectedError: "failed to create resource test-group/test-vnet (service: virtualnetwork): #: Internal Server Error: StatusCode=500", expect: func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_virtualnetworks.MockClientMockRecorder) { s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName().AnyTimes().Return("fake-cluster") - s.Vnet().AnyTimes().Return(&infrav1.VnetSpec{Name: "vnet-exists"}) - s.VNetSpec().Return(azure.VNetSpec{ - ResourceGroup: "my-rg", - Name: "ipv6-vnet-exists", - CIDRs: []string{"10.0.0.0/8", "2001:1234:5678:9a00::/56"}, - }) - m.Get(gomockinternal.AContext(), "my-rg", "ipv6-vnet-exists"). - Return(network.VirtualNetwork{ - ID: to.StringPtr("azure/fake/id"), - Name: to.StringPtr("ipv6-vnet-exists"), - VirtualNetworkPropertiesFormat: &network.VirtualNetworkPropertiesFormat{ - AddressSpace: &network.AddressSpace{ - AddressPrefixes: to.StringSlicePtr([]string{ - "10.0.0.0/8", - "2001:1234:5678:9a00::/56", - }), - }, - }, - Tags: map[string]*string{ - "Name": to.StringPtr("ipv6-vnet-exists"), - "sigs.k8s.io_cluster-api-provider-azure_cluster_fake-cluster": to.StringPtr("owned"), - "sigs.k8s.io_cluster-api-provider-azure_role": to.StringPtr("common"), - }, - }, nil) + s.VNetSpec().Return(&fakeVNetSpec) + s.GetLongRunningOperationState("test-vnet", serviceName).Return(nil) + m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakeVNetSpec).Return(nil, internalError) + s.UpdatePutStatus(infrav1.VNetReadyCondition, serviceName, gomockinternal.ErrStrEq(fmt.Sprintf("failed to create resource test-group/test-vnet (service: virtualnetwork): %s", internalError.Error()))) }, }, { - name: "vnet created successufuly", - expectedError: "", + name: "create vnet operation is already in progress", + expectedError: "transient reconcile error occurred: operation type PUT on Azure resource test-group/test-vnet is not done. Object will be requeued after 15s", expect: func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_virtualnetworks.MockClientMockRecorder) { s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName().AnyTimes().Return("fake-cluster") - s.Location().AnyTimes().Return("fake-location") - s.AdditionalTags().AnyTimes().Return(infrav1.Tags{}) - s.Vnet().AnyTimes().Return(&infrav1.VnetSpec{Name: "vnet-new"}) - s.VNetSpec().Return(azure.VNetSpec{ - ResourceGroup: "my-rg", - Name: "vnet-new", - CIDRs: []string{"10.0.0.0/8"}, - }) - m.Get(gomockinternal.AContext(), "my-rg", "vnet-new"). - Return(network.VirtualNetwork{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found")) - - m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "vnet-new", gomock.AssignableToTypeOf(network.VirtualNetwork{})) + s.VNetSpec().Return(&fakeVNetSpec) + s.GetLongRunningOperationState("test-vnet", serviceName).Times(2).Return(&fakeCreateFuture) + m.IsDone(gomockinternal.AContext(), gomock.AssignableToTypeOf(&azureautorest.Future{})).Return(false, nil) + s.UpdatePutStatus(infrav1.VNetReadyCondition, serviceName, gomockinternal.ErrStrEq("transient reconcile error occurred: operation type PUT on Azure resource test-group/test-vnet is not done. Object will be requeued after 15s")) }, }, + } + + for _, tc := range testcases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + + t.Parallel() + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + scopeMock := mock_virtualnetworks.NewMockVNetScope(mockCtrl) + clientMock := mock_virtualnetworks.NewMockClient(mockCtrl) + + tc.expect(scopeMock.EXPECT(), clientMock.EXPECT()) + + s := &Service{ + Scope: scopeMock, + Client: clientMock, + } + + err := s.Reconcile(context.TODO()) + if tc.expectedError != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tc.expectedError)) + } else { + g.Expect(err).NotTo(HaveOccurred()) + } + }) + } +} + +func TestDeleteVnet(t *testing.T) { + testcases := []struct { + name string + expectedError string + expect func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_virtualnetworks.MockClientMockRecorder) + }{ { - name: "ipv6 vnet created successufuly", + name: "delete vnet succeeds", expectedError: "", expect: func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_virtualnetworks.MockClientMockRecorder) { s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName().AnyTimes().Return("fake-cluster") - s.Location().AnyTimes().Return("fake-location") - s.AdditionalTags().AnyTimes().Return(infrav1.Tags{}) - s.Vnet().AnyTimes().Return(&infrav1.VnetSpec{Name: "vnet-new"}) - s.VNetSpec().Return(azure.VNetSpec{ - ResourceGroup: "my-rg", - Name: "vnet-ipv6-new", - CIDRs: []string{"10.0.0.0/8", "2001:1234:5678:9a00::/56"}, - }) - m.Get(gomockinternal.AContext(), "my-rg", "vnet-ipv6-new"). - Return(network.VirtualNetwork{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found")) - - m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "vnet-ipv6-new", gomockinternal.DiffEq(network.VirtualNetwork{ - Tags: map[string]*string{ - "Name": to.StringPtr("vnet-ipv6-new"), - "sigs.k8s.io_cluster-api-provider-azure_cluster_fake-cluster": to.StringPtr(string(infrav1.ResourceLifecycleOwned)), - "sigs.k8s.io_cluster-api-provider-azure_role": to.StringPtr(infrav1.CommonRole), - }, - Location: to.StringPtr("fake-location"), - VirtualNetworkPropertiesFormat: &network.VirtualNetworkPropertiesFormat{ - AddressSpace: &network.AddressSpace{ - AddressPrefixes: to.StringSlicePtr([]string{ - "10.0.0.0/8", - "2001:1234:5678:9a00::/56", - }), - }, - }, - })) + s.VNetSpec().Times(2).Return(&fakeVNetSpec) + m.Get(gomockinternal.AContext(), "test-group", "test-vnet").Return(managedVnet, nil) + s.ClusterName().Return("test-cluster") + s.SetVnetManagedCache(true) + s.GetLongRunningOperationState("test-vnet", serviceName).Return(nil) + m.DeleteAsync(gomockinternal.AContext(), &fakeVNetSpec).Return(nil, nil) + s.UpdateDeleteStatus(infrav1.VNetReadyCondition, serviceName, nil) }, }, { - name: "unmanaged vnet exists", - expectedError: "", + name: "delete vnet fails", + expectedError: "failed to delete resource test-group/test-vnet (service: virtualnetwork): #: Internal Server Error: StatusCode=500", expect: func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_virtualnetworks.MockClientMockRecorder) { s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName().AnyTimes().Return("fake-cluster") - s.Location().AnyTimes().Return("fake-location") - s.Vnet().AnyTimes().Return(&infrav1.VnetSpec{Name: "custom-vnet"}) - s.VNetSpec().Return(azure.VNetSpec{ - ResourceGroup: "custom-vnet-rg", - Name: "custom-vnet", - CIDRs: []string{"10.0.0.0/16"}, - }) - m.Get(gomockinternal.AContext(), "custom-vnet-rg", "custom-vnet"). - Return(network.VirtualNetwork{ - ID: to.StringPtr("azure/custom-vnet/id"), - Name: to.StringPtr("custom-vnet"), - VirtualNetworkPropertiesFormat: &network.VirtualNetworkPropertiesFormat{ - AddressSpace: &network.AddressSpace{ - AddressPrefixes: to.StringSlicePtr([]string{"10.0.0.0/16"}), - }, - }, - Tags: map[string]*string{ - "Name": to.StringPtr("my-custom-vnet"), - }, - }, nil) + s.VNetSpec().Times(2).Return(&fakeVNetSpec) + m.Get(gomockinternal.AContext(), "test-group", "test-vnet").Return(managedVnet, nil) + s.ClusterName().Return("test-cluster") + s.SetVnetManagedCache(true) + s.GetLongRunningOperationState("test-vnet", serviceName).Return(nil) + m.DeleteAsync(gomockinternal.AContext(), &fakeVNetSpec).Return(nil, internalError) + s.UpdateDeleteStatus(infrav1.VNetReadyCondition, serviceName, gomockinternal.ErrStrEq(fmt.Sprintf("failed to delete resource test-group/test-vnet (service: virtualnetwork): %s", internalError.Error()))) }, }, { - name: "custom vnet not found", - expectedError: "", + name: "delete vnet operation is already in progress", + expectedError: "transient reconcile error occurred: operation type PUT on Azure resource test-group/test-vnet is not done. Object will be requeued after 15s", expect: func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_virtualnetworks.MockClientMockRecorder) { s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName().AnyTimes().Return("fake-cluster") - s.Location().AnyTimes().Return("fake-location") - s.AdditionalTags().AnyTimes().Return(infrav1.Tags{}) - s.Vnet().AnyTimes().Return(&infrav1.VnetSpec{Name: "custom-vnet"}) - s.VNetSpec().Return(azure.VNetSpec{ - ResourceGroup: "custom-vnet-rg", - Name: "custom-vnet", - CIDRs: []string{"10.0.0.0/16"}, - }) - m.Get(gomockinternal.AContext(), "custom-vnet-rg", "custom-vnet"). - Return(network.VirtualNetwork{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found")) - - m.CreateOrUpdate(gomockinternal.AContext(), "custom-vnet-rg", "custom-vnet", gomock.AssignableToTypeOf(network.VirtualNetwork{})) + s.VNetSpec().Times(2).Return(&fakeVNetSpec) + m.Get(gomockinternal.AContext(), "test-group", "test-vnet").Return(managedVnet, nil) + s.ClusterName().Return("test-cluster") + s.SetVnetManagedCache(true) + s.GetLongRunningOperationState("test-vnet", serviceName).Times(2).Return(&fakeCreateFuture) + m.IsDone(gomockinternal.AContext(), gomock.AssignableToTypeOf(&azureautorest.Future{})).Return(false, nil) + s.UpdateDeleteStatus(infrav1.VNetReadyCondition, serviceName, gomockinternal.ErrStrEq("transient reconcile error occurred: operation type PUT on Azure resource test-group/test-vnet is not done. Object will be requeued after 15s")) }, }, { - name: "failed to fetch vnet", - expectedError: "failed to get VNet custom-vnet: failed to get VNet custom-vnet: #: Internal Server Error: StatusCode=500", + name: "vnet already deleted", + expectedError: "", expect: func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_virtualnetworks.MockClientMockRecorder) { s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName().AnyTimes().Return("fake-cluster") - s.VNetSpec().Return(azure.VNetSpec{ - ResourceGroup: "custom-vnet-rg", - Name: "custom-vnet", - CIDRs: []string{"10.0.0.0/16"}, - }) - m.Get(gomockinternal.AContext(), "custom-vnet-rg", "custom-vnet"). - Return(network.VirtualNetwork{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")) + s.VNetSpec().Times(2).Return(&fakeVNetSpec) + m.Get(gomockinternal.AContext(), "test-group", "test-vnet").Return(network.VirtualNetwork{}, notFoundError) + s.DeleteLongRunningOperationState("test-vnet", serviceName) + s.UpdateDeleteStatus(infrav1.VNetReadyCondition, serviceName, nil) }, }, { - name: "fail to create vnet", - expectedError: "failed to create virtual network custom-vnet: #: Internal Server Honk: StatusCode=500", + name: "vnet is not managed", + expectedError: "", expect: func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_virtualnetworks.MockClientMockRecorder) { s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName().AnyTimes().Return("fake-cluster") - s.Location().AnyTimes().Return("fake-location") - s.AdditionalTags().AnyTimes().Return(infrav1.Tags{}) - s.Vnet().AnyTimes().Return(&infrav1.VnetSpec{Name: "custom-vnet"}) - s.VNetSpec().Return(azure.VNetSpec{ - ResourceGroup: "custom-vnet-rg", - Name: "custom-vnet", - CIDRs: []string{"10.0.0.0/16"}, - }) - m.Get(gomockinternal.AContext(), "custom-vnet-rg", "custom-vnet"). - Return(network.VirtualNetwork{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found")) - - m.CreateOrUpdate(gomockinternal.AContext(), "custom-vnet-rg", "custom-vnet", gomock.AssignableToTypeOf(network.VirtualNetwork{})).Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Honk")) + s.VNetSpec().Times(2).Return(&fakeVNetSpec) + m.Get(gomockinternal.AContext(), "test-group", "test-vnet").Return(customVnet, nil) + s.ClusterName().Return("test-cluster") + s.SetVnetManagedCache(false) }, }, } - for _, tc := range testcases { tc := tc t.Run(tc.name, func(t *testing.T) { @@ -259,7 +233,7 @@ func TestReconcileVnet(t *testing.T) { Client: clientMock, } - err := s.Reconcile(context.TODO()) + err := s.Delete(context.TODO()) if tc.expectedError != "" { g.Expect(err).To(HaveOccurred()) g.Expect(err).To(MatchError(tc.expectedError)) @@ -270,134 +244,44 @@ func TestReconcileVnet(t *testing.T) { } } -func TestDeleteVnet(t *testing.T) { +func TestIsVnetManaged(t *testing.T) { testcases := []struct { name string expectedError string + result bool expect func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_virtualnetworks.MockClientMockRecorder) }{ { - name: "managed vnet exists", - expectedError: "", - expect: func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_virtualnetworks.MockClientMockRecorder) { - s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName().AnyTimes().Return("fake-cluster") - s.Location().AnyTimes().Return("fake-location") - s.AdditionalTags().AnyTimes().Return(infrav1.Tags{ - "Name": "vnet-exists", - "sigs.k8s.io_cluster-api-provider-azure_cluster_fake-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }) - s.Vnet().AnyTimes().Return(&infrav1.VnetSpec{Name: "vnet-exists"}) - s.VNetSpec().Return(azure.VNetSpec{ - ResourceGroup: "my-rg", - Name: "vnet-exists", - CIDRs: []string{"10.0.0.0/16"}, - }) - m.Get(gomockinternal.AContext(), "my-rg", "vnet-exists"). - Return(network.VirtualNetwork{ - ID: to.StringPtr("azure/fake/id"), - Name: to.StringPtr("vnet-exists"), - VirtualNetworkPropertiesFormat: &network.VirtualNetworkPropertiesFormat{ - AddressSpace: &network.AddressSpace{ - AddressPrefixes: to.StringSlicePtr([]string{"10.0.0.0/8"}), - }, - }, - Tags: map[string]*string{ - "Name": to.StringPtr("vnet-exists"), - "sigs.k8s.io_cluster-api-provider-azure_cluster_fake-cluster": to.StringPtr("owned"), - "sigs.k8s.io_cluster-api-provider-azure_role": to.StringPtr("common"), - }, - }, nil) - m.Delete(gomockinternal.AContext(), "my-rg", "vnet-exists") - }, - }, - { - name: "managed vnet already deleted", + name: "managed vnet returns true", + result: true, expectedError: "", expect: func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_virtualnetworks.MockClientMockRecorder) { s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName().AnyTimes().Return("fake-cluster") - s.Location().AnyTimes().Return("fake-location") - s.AdditionalTags().AnyTimes().Return(infrav1.Tags{ - "Name": "vnet-exists", - "sigs.k8s.io_cluster-api-provider-azure_cluster_fake-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }) - s.Vnet().AnyTimes().Return(&infrav1.VnetSpec{Name: "vnet-exists"}) - s.VNetSpec().Return(azure.VNetSpec{ - ResourceGroup: "my-rg", - Name: "vnet-exists", - CIDRs: []string{"10.0.0.0/16"}, - }) - m.Get(gomockinternal.AContext(), "my-rg", "vnet-exists"). - Return(network.VirtualNetwork{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found")) + s.VNetSpec().Return(&fakeVNetSpec) + m.Get(gomockinternal.AContext(), "test-group", "test-vnet").Return(managedVnet, nil) + s.ClusterName().Return("test-cluster") + s.SetVnetManagedCache(true) }, }, { - name: "unmanaged vnet", + name: "custom vnet returns false", + result: false, expectedError: "", expect: func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_virtualnetworks.MockClientMockRecorder) { s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName().AnyTimes().Return("fake-cluster") - s.Location().AnyTimes().Return("fake-location") - s.AdditionalTags().AnyTimes().Return(infrav1.Tags{}) - s.Vnet().AnyTimes().Return(&infrav1.VnetSpec{ResourceGroup: "my-rg", Name: "my-vnet", ID: "azure/custom-vnet/id"}) - s.VNetSpec().Return(azure.VNetSpec{ - ResourceGroup: "my-rg", - Name: "my-vnet", - CIDRs: []string{"10.0.0.0/16"}, - }) - m.Get(gomockinternal.AContext(), "my-rg", "my-vnet"). - Return(network.VirtualNetwork{ - ID: to.StringPtr("azure/custom-vnet/id"), - Name: to.StringPtr("my-vnet"), - VirtualNetworkPropertiesFormat: &network.VirtualNetworkPropertiesFormat{ - AddressSpace: &network.AddressSpace{ - AddressPrefixes: to.StringSlicePtr([]string{"10.0.0.0/16"}), - }, - }, - Tags: map[string]*string{ - "Name": to.StringPtr("my-vnet"), - }, - }, nil) + s.VNetSpec().Return(&fakeVNetSpec) + m.Get(gomockinternal.AContext(), "test-group", "test-vnet").Return(customVnet, nil) + s.ClusterName().Return("test-cluster") + s.SetVnetManagedCache(false) }, }, { - name: "fail to delete vnet", - expectedError: "failed to delete VNet vnet-exists in resource group my-rg: #: Internal Honk Server: StatusCode=500", + name: "fail to check if vnet is managed", + expectedError: internalError.Error(), expect: func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_virtualnetworks.MockClientMockRecorder) { s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.ClusterName().AnyTimes().Return("fake-cluster") - s.Location().AnyTimes().Return("fake-location") - s.AdditionalTags().AnyTimes().Return(infrav1.Tags{ - "Name": "vnet-exists", - "sigs.k8s.io_cluster-api-provider-azure_cluster_fake-cluster": "owned", - "sigs.k8s.io_cluster-api-provider-azure_role": "common", - }) - s.Vnet().AnyTimes().Return(&infrav1.VnetSpec{Name: "vnet-exists"}) - s.VNetSpec().Return(azure.VNetSpec{ - ResourceGroup: "my-rg", - Name: "vnet-exists", - CIDRs: []string{"10.0.0.0/16"}, - }) - m.Get(gomockinternal.AContext(), "my-rg", "vnet-exists"). - Return(network.VirtualNetwork{ - ID: to.StringPtr("azure/fake/id"), - Name: to.StringPtr("vnet-exists"), - VirtualNetworkPropertiesFormat: &network.VirtualNetworkPropertiesFormat{ - AddressSpace: &network.AddressSpace{ - AddressPrefixes: to.StringSlicePtr([]string{"10.0.0.0/8"}), - }, - }, - Tags: map[string]*string{ - "Name": to.StringPtr("vnet-exists"), - "sigs.k8s.io_cluster-api-provider-azure_cluster_fake-cluster": to.StringPtr("owned"), - "sigs.k8s.io_cluster-api-provider-azure_role": to.StringPtr("common"), - }, - }, nil) - m.Delete(gomockinternal.AContext(), "my-rg", "vnet-exists"). - Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Honk Server")) + s.VNetSpec().Return(&fakeVNetSpec) + m.Get(gomockinternal.AContext(), "test-group", "test-vnet").Return(network.VirtualNetwork{}, internalError) }, }, } @@ -419,7 +303,8 @@ func TestDeleteVnet(t *testing.T) { Client: clientMock, } - err := s.Delete(context.TODO()) + result, err := s.IsVnetManaged(context.TODO()) + g.Expect(result).To(Equal(tc.result)) if tc.expectedError != "" { g.Expect(err).To(HaveOccurred()) g.Expect(err).To(MatchError(tc.expectedError)) diff --git a/azure/types.go b/azure/types.go index 8d659eb85bf5..8840bea3ae6b 100644 --- a/azure/types.go +++ b/azure/types.go @@ -102,13 +102,6 @@ type SubnetSpec struct { NatGatewayName string } -// VNetSpec defines the specification for a Virtual Network. -type VNetSpec struct { - ResourceGroup string - Name string - CIDRs []string -} - // RoleAssignmentSpec defines the specification for a Role Assignment. type RoleAssignmentSpec struct { MachineName string @@ -129,12 +122,6 @@ const ( VirtualMachineScaleSet = "VirtualMachineScaleSet" ) -// NSGSpec defines the specification for a Security Group. -type NSGSpec struct { - Name string - SecurityRules infrav1.SecurityRules -} - // VMSpec defines the specification for a Virtual Machine. type VMSpec struct { Name string