From 1278516765c1875025a48ab112320cb2d0bfdc62 Mon Sep 17 00:00:00 2001 From: Jonathan Tong Date: Tue, 15 Feb 2022 16:22:20 -0500 Subject: [PATCH] Only show applicable conditions for AzureClusters and AzureMachines --- azure/scope/cluster.go | 15 +--- azure/scope/machine.go | 8 +- .../availabilitysets/availabilitysets.go | 33 +++---- .../availabilitysets/availabilitysets_test.go | 4 +- azure/services/bastionhosts/bastionhosts.go | 4 + .../bastionhosts/bastionhosts_test.go | 2 - azure/services/disks/disks.go | 10 ++- azure/services/disks/disks_test.go | 7 ++ azure/services/groups/groups.go | 6 ++ azure/services/groups/groups_test.go | 14 +++ .../inboundnatrules/inboundnatrules.go | 21 +++-- .../inboundnatrules/inboundnatrules_test.go | 21 ++++- azure/services/loadbalancers/loadbalancers.go | 16 +++- .../loadbalancers/loadbalancers_test.go | 14 +++ azure/services/natgateways/natgateways.go | 15 +++- .../services/natgateways/natgateways_test.go | 20 +++++ .../networkinterfaces/networkinterfaces.go | 16 +++- .../networkinterfaces_test.go | 14 +++ azure/services/routetables/routetables.go | 35 +++++--- .../services/routetables/routetables_test.go | 21 ++++- azure/services/subnets/spec_test.go | 28 ------ azure/services/subnets/subnets.go | 24 ++++-- azure/services/subnets/subnets_test.go | 85 +++++++++++++++++-- .../virtualmachines/virtualmachines.go | 6 ++ .../virtualmachines/virtualmachines_test.go | 14 +++ .../virtualnetworks_mock.go | 14 +++ .../virtualnetworks/virtualnetworks.go | 13 ++- .../virtualnetworks/virtualnetworks_test.go | 25 ++++++ azure/services/vnetpeerings/vnetpeerings.go | 15 +++- .../vnetpeerings/vnetpeerings_test.go | 10 +-- controllers/azurecluster_controller.go | 1 + 31 files changed, 405 insertions(+), 126 deletions(-) diff --git a/azure/scope/cluster.go b/azure/scope/cluster.go index 30415c88dc9..0eb7c6725a2 100644 --- a/azure/scope/cluster.go +++ b/azure/scope/cluster.go @@ -679,20 +679,7 @@ func (s *ClusterScope) PatchObject(ctx context.Context) error { ctx, _, done := tele.StartSpanWithLogger(ctx, "scope.ClusterScope.PatchObject") defer done() - conditions.SetSummary(s.AzureCluster, - conditions.WithConditions( - infrav1.ResourceGroupReadyCondition, - infrav1.RouteTablesReadyCondition, - infrav1.NetworkInfrastructureReadyCondition, - infrav1.VnetPeeringReadyCondition, - infrav1.DisksReadyCondition, - infrav1.NATGatewaysReadyCondition, - infrav1.LoadBalancersReadyCondition, - infrav1.BastionHostReadyCondition, - infrav1.VNetReadyCondition, - infrav1.SubnetsReadyCondition, - ), - ) + conditions.SetSummary(s.AzureCluster) return s.patchHelper.Patch( ctx, diff --git a/azure/scope/machine.go b/azure/scope/machine.go index fcad6e9dd99..a21fad7a4b2 100644 --- a/azure/scope/machine.go +++ b/azure/scope/machine.go @@ -547,13 +547,7 @@ func (m *MachineScope) SetAddresses(addrs []corev1.NodeAddress) { // PatchObject persists the machine spec and status. func (m *MachineScope) PatchObject(ctx context.Context) error { - conditions.SetSummary(m.AzureMachine, - conditions.WithConditions( - infrav1.VMRunningCondition, - infrav1.AvailabilitySetReadyCondition, - infrav1.NetworkInterfaceReadyCondition, - ), - ) + conditions.SetSummary(m.AzureMachine) return m.patchHelper.Patch( ctx, diff --git a/azure/services/availabilitysets/availabilitysets.go b/azure/services/availabilitysets/availabilitysets.go index dccde1b9b87..280e8958714 100644 --- a/azure/services/availabilitysets/availabilitysets.go +++ b/azure/services/availabilitysets/availabilitysets.go @@ -70,6 +70,7 @@ func (s *Service) Reconcile(ctx context.Context) error { _, err = s.CreateResource(ctx, setSpec, serviceName) } else { log.V(2).Info("skip creation when no availability set spec is found") + return nil } s.Scope.UpdatePutStatus(infrav1.AvailabilitySetReadyCondition, serviceName, err) @@ -85,25 +86,27 @@ func (s *Service) Delete(ctx context.Context) error { defer cancel() var resultingErr error - if setSpec := s.Scope.AvailabilitySetSpec(); setSpec == nil { + setSpec := s.Scope.AvailabilitySetSpec() + if setSpec == nil { log.V(2).Info("skip deletion when no availability set spec is found") + return nil + } + + existingSet, err := s.Get(ctx, setSpec) + if err != nil { + if !azure.ResourceNotFound(err) { + resultingErr = errors.Wrapf(err, "failed to get availability set %s in resource group %s", setSpec.ResourceName(), setSpec.ResourceGroupName()) + } } else { - existingSet, err := s.Get(ctx, setSpec) - if err != nil { - if !azure.ResourceNotFound(err) { - resultingErr = errors.Wrapf(err, "failed to get availability set %s in resource group %s", setSpec.ResourceName(), setSpec.ResourceGroupName()) - } + availabilitySet, ok := existingSet.(compute.AvailabilitySet) + if !ok { + resultingErr = errors.Errorf("%T is not a compute.AvailabilitySet", existingSet) } else { - availabilitySet, ok := existingSet.(compute.AvailabilitySet) - if !ok { - resultingErr = errors.Errorf("%T is not a compute.AvailabilitySet", existingSet) + // only delete when the availability set does not have any vms + if availabilitySet.AvailabilitySetProperties != nil && availabilitySet.VirtualMachines != nil && len(*availabilitySet.VirtualMachines) > 0 { + log.V(2).Info("skip deleting availability set with VMs", "availability set", setSpec.ResourceName()) } else { - // only delete when the availability set does not have any vms - if availabilitySet.AvailabilitySetProperties != nil && availabilitySet.VirtualMachines != nil && len(*availabilitySet.VirtualMachines) > 0 { - log.V(2).Info("skip deleting availability set with VMs", "availability set", setSpec.ResourceName()) - } else { - resultingErr = s.DeleteResource(ctx, setSpec, serviceName) - } + resultingErr = s.DeleteResource(ctx, setSpec, serviceName) } } } diff --git a/azure/services/availabilitysets/availabilitysets_test.go b/azure/services/availabilitysets/availabilitysets_test.go index fb4c648f1fe..da9d0192aca 100644 --- a/azure/services/availabilitysets/availabilitysets_test.go +++ b/azure/services/availabilitysets/availabilitysets_test.go @@ -89,11 +89,10 @@ func TestReconcileAvailabilitySets(t *testing.T) { }, }, { - name: "noop if no availability set spec is found", + name: "noop if no availability set spec returns nil", expectedError: "", expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.AvailabilitySetSpec().Return(nil) - s.UpdatePutStatus(infrav1.AvailabilitySetReadyCondition, serviceName, nil) }, }, { @@ -167,7 +166,6 @@ func TestDeleteAvailabilitySets(t *testing.T) { expectedError: "", expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.AvailabilitySetSpec().Return(nil) - s.UpdateDeleteStatus(infrav1.AvailabilitySetReadyCondition, serviceName, nil) }, }, { diff --git a/azure/services/bastionhosts/bastionhosts.go b/azure/services/bastionhosts/bastionhosts.go index 2922f4f0c80..396a663f67e 100644 --- a/azure/services/bastionhosts/bastionhosts.go +++ b/azure/services/bastionhosts/bastionhosts.go @@ -61,6 +61,8 @@ func (s *Service) Reconcile(ctx context.Context) error { var resultingErr error if bastionSpec := s.Scope.AzureBastionSpec(); bastionSpec != nil { _, resultingErr = s.CreateResource(ctx, bastionSpec, serviceName) + } else { + return nil } s.Scope.UpdatePutStatus(infrav1.BastionHostReadyCondition, serviceName, resultingErr) @@ -78,6 +80,8 @@ func (s *Service) Delete(ctx context.Context) error { var resultingErr error if bastionSpec := s.Scope.AzureBastionSpec(); bastionSpec != nil { resultingErr = s.DeleteResource(ctx, bastionSpec, serviceName) + } else { + return nil } s.Scope.UpdateDeleteStatus(infrav1.BastionHostReadyCondition, serviceName, resultingErr) diff --git a/azure/services/bastionhosts/bastionhosts_test.go b/azure/services/bastionhosts/bastionhosts_test.go index 24111b5d9b1..369fdc9a22d 100644 --- a/azure/services/bastionhosts/bastionhosts_test.go +++ b/azure/services/bastionhosts/bastionhosts_test.go @@ -69,7 +69,6 @@ func TestReconcileBastionHosts(t *testing.T) { expectedError: "", expect: func(s *mock_bastionhosts.MockBastionScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.AzureBastionSpec().Return(nil) - s.UpdatePutStatus(infrav1.BastionHostReadyCondition, serviceName, nil) }, }, { @@ -140,7 +139,6 @@ func TestDeleteBastionHost(t *testing.T) { expectedError: "", expect: func(s *mock_bastionhosts.MockBastionScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.AzureBastionSpec().Return(nil) - s.UpdateDeleteStatus(infrav1.BastionHostReadyCondition, serviceName, nil) }, }, } diff --git a/azure/services/disks/disks.go b/azure/services/disks/disks.go index f150d46ce26..00e092558b7 100644 --- a/azure/services/disks/disks.go +++ b/azure/services/disks/disks.go @@ -67,12 +67,16 @@ func (s *Service) Delete(ctx context.Context) error { ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout) defer cancel() - var result error + specs := s.Scope.DiskSpecs() + if len(specs) == 0 { + return nil + } // We go through the list of DiskSpecs to delete each one, independently of the result of the previous one. // If multiple errors occur, we return the most pressing one. - // Order of precedence (highest -> lowest) is: error that is not an operationNotDoneError (ie. error creating) -> operationNotDoneError (ie. creating in progress) -> no error (ie. created) - for _, diskSpec := range s.Scope.DiskSpecs() { + // Order of precedence (highest -> lowest) is: error that is not an operationNotDoneError (i.e. error creating) -> operationNotDoneError (i.e. creating in progress) -> no error (i.e. created) + var result error + for _, diskSpec := range specs { if err := s.DeleteResource(ctx, diskSpec, serviceName); err != nil { if !azure.IsOperationNotDoneError(err) || result == nil { result = err diff --git a/azure/services/disks/disks_test.go b/azure/services/disks/disks_test.go index 6c1425f0757..9622ca83d61 100644 --- a/azure/services/disks/disks_test.go +++ b/azure/services/disks/disks_test.go @@ -56,6 +56,13 @@ func TestDeleteDisk(t *testing.T) { expectedError string expect func(s *mock_disks.MockDiskScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ + { + name: "noop if no disk specs are found", + expectedError: "", + expect: func(s *mock_disks.MockDiskScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.DiskSpecs().Return([]azure.ResourceSpecGetter{}) + }, + }, { name: "delete the disk", expectedError: "", diff --git a/azure/services/groups/groups.go b/azure/services/groups/groups.go index b89ee308b9e..e301df5de5b 100644 --- a/azure/services/groups/groups.go +++ b/azure/services/groups/groups.go @@ -65,6 +65,9 @@ func (s *Service) Reconcile(ctx context.Context) error { defer cancel() groupSpec := s.Scope.GroupSpec() + if groupSpec == nil { + return nil + } _, err := s.CreateResource(ctx, groupSpec, serviceName) s.Scope.UpdatePutStatus(infrav1.ResourceGroupReadyCondition, serviceName, err) @@ -80,6 +83,9 @@ func (s *Service) Delete(ctx context.Context) error { defer cancel() groupSpec := s.Scope.GroupSpec() + if groupSpec == nil { + return nil + } // check that the resource group is not BYO. managed, err := s.IsGroupManaged(ctx) diff --git a/azure/services/groups/groups_test.go b/azure/services/groups/groups_test.go index 06ebbad11a0..c3500c3141d 100644 --- a/azure/services/groups/groups_test.go +++ b/azure/services/groups/groups_test.go @@ -62,6 +62,13 @@ func TestReconcileGroups(t *testing.T) { expectedError string expect func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ + { + name: "noop if no group spec is found", + expectedError: "", + expect: func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.GroupSpec().Return(nil) + }, + }, { name: "create group succeeds", expectedError: "", @@ -119,6 +126,13 @@ func TestDeleteGroups(t *testing.T) { expectedError string expect func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ + { + name: "noop if no group spec is found", + expectedError: "", + expect: func(s *mock_groups.MockGroupScopeMockRecorder, m *mock_groups.MockclientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.GroupSpec().Return(nil) + }, + }, { name: "delete operation is successful for managed resource group", expectedError: "", diff --git a/azure/services/inboundnatrules/inboundnatrules.go b/azure/services/inboundnatrules/inboundnatrules.go index 789d0d7dc75..c95a680f2ee 100644 --- a/azure/services/inboundnatrules/inboundnatrules.go +++ b/azure/services/inboundnatrules/inboundnatrules.go @@ -62,10 +62,6 @@ func (s *Service) Reconcile(ctx context.Context) error { // Externally managed clusters might not have an LB if s.Scope.APIServerLBName() == "" { log.V(4).Info("Skipping InboundNatRule reconciliation as the cluster has no LB configured") - // Until https://github.com/kubernetes-sigs/cluster-api-provider-azure/issues/1868 is - // resolved, this needs to be set for the machine to be able to reach the ready condition: - // https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/2066#discussion_r806150004 - s.Scope.UpdatePutStatus(infrav1.InboundNATRulesReadyCondition, serviceName, nil) return nil } @@ -84,11 +80,16 @@ func (s *Service) Reconcile(ctx context.Context) error { portsInUse[*rule.InboundNatRulePropertiesFormat.FrontendPort] = struct{}{} // Mark frontend port as in use } + specs := s.Scope.InboundNatSpecs(portsInUse) + if len(specs) == 0 { + return nil + } + // We go through the list of InboundNatSpecs to reconcile each one, independently of the result of the previous one. // If multiple errors occur, we return the most pressing one. // Order of precedence (highest -> lowest) is: error that is not an operationNotDoneError (i.e. error creating) -> operationNotDoneError (i.e. creating in progress) -> no error (i.e. created) var result error - for _, natRule := range s.Scope.InboundNatSpecs(portsInUse) { + for _, natRule := range specs { // If we are creating multiple inbound NAT rules, we could have a collision in finding an available frontend port since the newly created rule takes an available port, and we do not update portsInUse in the specs. // It doesn't matter in this case since we only create one rule per machine, but for multiple rules, we could end up restarting the Reconcile function each time to get the updated available ports. // TODO: We can update the available ports and recompute the specs each time, or alternatively, we could deterministically calculate the ports we plan on using to avoid collisions, i.e. rule #1 uses the first available port, rule #2 uses the second available port, etc. @@ -100,6 +101,7 @@ func (s *Service) Reconcile(ctx context.Context) error { } s.Scope.UpdatePutStatus(infrav1.InboundNATRulesReadyCondition, serviceName, result) + return result } @@ -111,18 +113,23 @@ func (s *Service) Delete(ctx context.Context) error { ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout) defer cancel() - var result error + specs := s.Scope.InboundNatSpecs(make(map[int32]struct{})) + if len(specs) == 0 { + return nil + } // We go through the list of InboundNatSpecs to delete each one, independently of the result of the previous one. // If multiple errors occur, we return the most pressing one. // Order of precedence (highest -> lowest) is: error that is not an operationNotDoneError (i.e. error deleting) -> operationNotDoneError (i.e. deleting in progress) -> no error (i.e. deleted) - for _, natRule := range s.Scope.InboundNatSpecs(make(map[int32]struct{})) { + var result error + for _, natRule := range specs { if err := s.DeleteResource(ctx, natRule, serviceName); err != nil { if !azure.IsOperationNotDoneError(err) || result == nil { result = err } } } + s.Scope.UpdateDeleteStatus(infrav1.InboundNATRulesReadyCondition, serviceName, result) return result } diff --git a/azure/services/inboundnatrules/inboundnatrules_test.go b/azure/services/inboundnatrules/inboundnatrules_test.go index 68c4e7bd8d0..25ad7593a40 100644 --- a/azure/services/inboundnatrules/inboundnatrules_test.go +++ b/azure/services/inboundnatrules/inboundnatrules_test.go @@ -92,6 +92,18 @@ func TestReconcileInboundNATRule(t *testing.T) { m *mock_inboundnatrules.MockclientMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ + { + name: "noop if no NAT rule specs are found", + expectedError: "", + expect: func(s *mock_inboundnatrules.MockInboundNatScopeMockRecorder, + m *mock_inboundnatrules.MockclientMockRecorder, + r *mock_async.MockReconcilerMockRecorder) { + s.ResourceGroup().AnyTimes().Return(fakeGroupName) + s.APIServerLBName().AnyTimes().Return(fakeLBName) + m.List(gomockinternal.AContext(), fakeGroupName, fakeLBName).Return(noExistingRules, nil) + s.InboundNatSpecs(noPortsInUse).Return([]azure.ResourceSpecGetter{}) + }, + }, { name: "NAT rule successfully created with with no existing rules", expectedError: "", @@ -131,7 +143,6 @@ func TestReconcileInboundNATRule(t *testing.T) { m *mock_inboundnatrules.MockclientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.APIServerLBName().AnyTimes().Return("") - s.UpdatePutStatus(infrav1.InboundNATRulesReadyCondition, serviceName, nil) }, }, { @@ -201,6 +212,14 @@ func TestDeleteNetworkInterface(t *testing.T) { expect func(s *mock_inboundnatrules.MockInboundNatScopeMockRecorder, m *mock_inboundnatrules.MockclientMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ + { + name: "noop if no NAT rules are found", + expectedError: "", + expect: func(s *mock_inboundnatrules.MockInboundNatScopeMockRecorder, + m *mock_inboundnatrules.MockclientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.InboundNatSpecs(noPortsInUse).Return([]azure.ResourceSpecGetter{}) + }, + }, { name: "successfully delete an existing NAT rule", expectedError: "", diff --git a/azure/services/loadbalancers/loadbalancers.go b/azure/services/loadbalancers/loadbalancers.go index a55ee426f54..3e5d6165fa5 100644 --- a/azure/services/loadbalancers/loadbalancers.go +++ b/azure/services/loadbalancers/loadbalancers.go @@ -63,11 +63,16 @@ func (s *Service) Reconcile(ctx context.Context) error { ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout) defer cancel() + specs := s.Scope.LBSpecs() + if len(specs) == 0 { + return nil + } + // We go through the list of LBSpecs to reconcile each one, independently of the result of the previous one. // If multiple errors occur, we return the most pressing one. // Order of precedence (highest -> lowest) is: error that is not an operationNotDoneError (i.e. error creating) -> operationNotDoneError (i.e. creating in progress) -> no error (i.e. created) var result error - for _, lbSpec := range s.Scope.LBSpecs() { + for _, lbSpec := range specs { if _, err := s.CreateResource(ctx, lbSpec, serviceName); err != nil { if !azure.IsOperationNotDoneError(err) || result == nil { result = err @@ -87,18 +92,23 @@ func (s *Service) Delete(ctx context.Context) error { ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout) defer cancel() - var result error + specs := s.Scope.LBSpecs() + if len(specs) == 0 { + return nil + } // We go through the list of LBSpecs to delete each one, independently of the result of the previous one. // If multiple errors occur, we return the most pressing one. // Order of precedence (highest -> lowest) is: error that is not an operationNotDoneError (i.e. error deleting) -> operationNotDoneError (i.e. deleting in progress) -> no error (i.e. deleted) - for _, lbSpec := range s.Scope.LBSpecs() { + var result error + for _, lbSpec := range specs { if err := s.DeleteResource(ctx, lbSpec, serviceName); err != nil { if !azure.IsOperationNotDoneError(err) || result == nil { result = err } } } + s.Scope.UpdateDeleteStatus(infrav1.LoadBalancersReadyCondition, serviceName, result) return result } diff --git a/azure/services/loadbalancers/loadbalancers_test.go b/azure/services/loadbalancers/loadbalancers_test.go index 907057081a5..586f5d75762 100644 --- a/azure/services/loadbalancers/loadbalancers_test.go +++ b/azure/services/loadbalancers/loadbalancers_test.go @@ -110,6 +110,13 @@ func TestReconcileLoadBalancer(t *testing.T) { expectedError string expect func(s *mock_loadbalancers.MockLBScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ + { + name: "noop if no LBSpecs are found", + expectedError: "", + expect: func(s *mock_loadbalancers.MockLBScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.LBSpecs().Return([]azure.ResourceSpecGetter{}) + }, + }, { name: "fail to create a public LB", expectedError: "#: Internal Server Error: StatusCode=500", @@ -194,6 +201,13 @@ func TestDeleteLoadBalancer(t *testing.T) { expectedError string expect func(s *mock_loadbalancers.MockLBScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ + { + name: "noop if no LBSpecs are found", + expectedError: "", + expect: func(s *mock_loadbalancers.MockLBScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.LBSpecs().Return([]azure.ResourceSpecGetter{}) + }, + }, { name: "delete a load balancer", expectedError: "", diff --git a/azure/services/natgateways/natgateways.go b/azure/services/natgateways/natgateways.go index 9db4332c538..1b64d94875c 100644 --- a/azure/services/natgateways/natgateways.go +++ b/azure/services/natgateways/natgateways.go @@ -70,10 +70,15 @@ func (s *Service) Reconcile(ctx context.Context) error { } // We go through the list of NatGatewaySpecs to reconcile each one, independently of the resultingErr of the previous one. + specs := s.Scope.NatGatewaySpecs() + if len(specs) == 0 { + return nil + } + // If multiple errors occur, we return the most pressing one. // Order of precedence (highest -> lowest) is: error that is not an operationNotDoneError (ie. error creating) -> operationNotDoneError (ie. creating in progress) -> no error (ie. created) var resultingErr error - for _, natGatewaySpec := range s.Scope.NatGatewaySpecs() { + for _, natGatewaySpec := range specs { result, err := s.CreateResource(ctx, natGatewaySpec, serviceName) if err != nil { if !azure.IsOperationNotDoneError(err) || resultingErr == nil { @@ -112,12 +117,16 @@ func (s *Service) Delete(ctx context.Context) error { return nil } - var resultingErr error + specs := s.Scope.NatGatewaySpecs() + if len(specs) == 0 { + return nil + } // We go through the list of NatGatewaySpecs to delete each one, independently of the resultingErr of the previous one. // If multiple errors occur, we return the most pressing one. // Order of precedence (highest -> lowest) is: error that is not an operationNotDoneError (ie. error creating) -> operationNotDoneError (ie. creating in progress) -> no error (ie. created) - for _, natGatewaySpec := range s.Scope.NatGatewaySpecs() { + var resultingErr error + for _, natGatewaySpec := range specs { if err := s.DeleteResource(ctx, natGatewaySpec, serviceName); err != nil { if !azure.IsOperationNotDoneError(err) || resultingErr == nil { resultingErr = err diff --git a/azure/services/natgateways/natgateways_test.go b/azure/services/natgateways/natgateways_test.go index 7cfc809c060..910774d63cd 100644 --- a/azure/services/natgateways/natgateways_test.go +++ b/azure/services/natgateways/natgateways_test.go @@ -77,6 +77,16 @@ func TestReconcileNatGateways(t *testing.T) { expectedError string expect func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ + { + name: "noop if no NAT gateways specs are found", + tags: customVNetTags, + expectedError: "", + expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.Vnet().Return(&ownedVNetSpec) + s.ClusterName() + s.NatGatewaySpecs().Return([]azure.ResourceSpecGetter{}) + }, + }, { name: "NAT gateways in custom vnet mode", tags: customVNetTags, @@ -161,6 +171,16 @@ func TestDeleteNatGateway(t *testing.T) { expectedError string expect func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ + { + name: "noop if no NAT gateways specs are found", + tags: ownedVNetTags, + expectedError: "", + expect: func(s *mock_natgateways.MockNatGatewayScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.Vnet().Return(&ownedVNetSpec) + s.ClusterName() + s.NatGatewaySpecs().Return([]azure.ResourceSpecGetter{}) + }, + }, { name: "NAT gateways in custom vnet mode", tags: customVNetTags, diff --git a/azure/services/networkinterfaces/networkinterfaces.go b/azure/services/networkinterfaces/networkinterfaces.go index bc956ccbf5f..bce8a6322f8 100644 --- a/azure/services/networkinterfaces/networkinterfaces.go +++ b/azure/services/networkinterfaces/networkinterfaces.go @@ -61,11 +61,16 @@ func (s *Service) Reconcile(ctx context.Context) error { ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout) defer cancel() + specs := s.Scope.NICSpecs() + if len(specs) == 0 { + return nil + } + // We go through the list of NICSpecs to reconcile each one, independently of the result of the previous one. // If multiple errors occur, we return the most pressing one. // Order of precedence (highest -> lowest) is: error that is not an operationNotDoneError (i.e. error creating) -> operationNotDoneError (i.e. creating in progress) -> no error (i.e. created) var result error - for _, nicSpec := range s.Scope.NICSpecs() { + for _, nicSpec := range specs { if _, err := s.CreateResource(ctx, nicSpec, serviceName); err != nil { if !azure.IsOperationNotDoneError(err) || result == nil { result = err @@ -85,18 +90,23 @@ func (s *Service) Delete(ctx context.Context) error { ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout) defer cancel() - var result error + specs := s.Scope.NICSpecs() + if len(specs) == 0 { + return nil + } // We go through the list of NICSpecs to delete each one, independently of the result of the previous one. // If multiple errors occur, we return the most pressing one. // Order of precedence (highest -> lowest) is: error that is not an operationNotDoneError (i.e. error deleting) -> operationNotDoneError (i.e. deleting in progress) -> no error (i.e. deleted) - for _, nicSpec := range s.Scope.NICSpecs() { + var result error + for _, nicSpec := range specs { if err := s.DeleteResource(ctx, nicSpec, serviceName); err != nil { if !azure.IsOperationNotDoneError(err) || result == nil { result = err } } } + s.Scope.UpdateDeleteStatus(infrav1.NetworkInterfaceReadyCondition, serviceName, result) return result } diff --git a/azure/services/networkinterfaces/networkinterfaces_test.go b/azure/services/networkinterfaces/networkinterfaces_test.go index c1dc657801a..4529567bf98 100644 --- a/azure/services/networkinterfaces/networkinterfaces_test.go +++ b/azure/services/networkinterfaces/networkinterfaces_test.go @@ -67,6 +67,13 @@ func TestReconcileNetworkInterface(t *testing.T) { expectedError string expect func(s *mock_networkinterfaces.MockNICScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ + { + name: "noop if no network interface specs are found", + expectedError: "", + expect: func(s *mock_networkinterfaces.MockNICScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.NICSpecs().Return([]azure.ResourceSpecGetter{}) + }, + }, { name: "successfully create a network interface", expectedError: "", @@ -134,6 +141,13 @@ func TestDeleteNetworkInterface(t *testing.T) { expectedError string expect func(s *mock_networkinterfaces.MockNICScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ + { + name: "noop if no network interface specs are found", + expectedError: "", + expect: func(s *mock_networkinterfaces.MockNICScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.NICSpecs().Return([]azure.ResourceSpecGetter{}) + }, + }, { name: "successfully delete an existing network interface", expectedError: "", diff --git a/azure/services/routetables/routetables.go b/azure/services/routetables/routetables.go index 0a769052906..456aa77c14d 100644 --- a/azure/services/routetables/routetables.go +++ b/azure/services/routetables/routetables.go @@ -63,18 +63,25 @@ func (s *Service) Reconcile(ctx context.Context) error { if !s.Scope.IsVnetManaged() { log.V(4).Info("Skipping route tables reconcile in custom vnet mode") - } else { - // We go through the list of route tables to reconcile each one, independently of the result of the previous one. - // If multiple errors occur, we return the most pressing one. - // Order of precedence (highest -> lowest) is: error that is not an operationNotDoneError (ie. error creating) -> operationNotDoneError (ie. creating in progress) -> no error (ie. created) - for _, rtSpec := range s.Scope.RouteTableSpecs() { - if _, err := s.CreateResource(ctx, rtSpec, serviceName); err != nil { - if !azure.IsOperationNotDoneError(err) || resErr == nil { - resErr = err - } + return nil + } + + specs := s.Scope.RouteTableSpecs() + if len(specs) == 0 { + return nil + } + + // We go through the list of route tables to reconcile each one, independently of the result of the previous one. + // If multiple errors occur, we return the most pressing one. + // Order of precedence (highest -> lowest) is: error that is not an operationNotDoneError (i.e. error creating) -> operationNotDoneError (i.e. creating in progress) -> no error (i.e. created) + for _, rtSpec := range specs { + if _, err := s.CreateResource(ctx, rtSpec, serviceName); err != nil { + if !azure.IsOperationNotDoneError(err) || resErr == nil { + resErr = err } } } + s.Scope.UpdatePutStatus(infrav1.RouteTablesReadyCondition, serviceName, resErr) return resErr } @@ -94,12 +101,16 @@ func (s *Service) Delete(ctx context.Context) error { return nil } - var result error + specs := s.Scope.RouteTableSpecs() + if len(specs) == 0 { + return nil + } // We go through the list of RouteTableSpecs to delete each one, independently of the result of the previous one. - // If multiple erros occur, we return the most pressing one + // If multiple errors occur, we return the most pressing one // order of precedence is: error deleting -> deleting in progress -> deleted (no error) - for _, rtSpec := range s.Scope.RouteTableSpecs() { + var result error + for _, rtSpec := range specs { if err := s.DeleteResource(ctx, rtSpec, serviceName); err != nil { if !azure.IsOperationNotDoneError(err) || result == nil { result = err diff --git a/azure/services/routetables/routetables_test.go b/azure/services/routetables/routetables_test.go index 88a37e39d98..87f9ca35e0d 100644 --- a/azure/services/routetables/routetables_test.go +++ b/azure/services/routetables/routetables_test.go @@ -52,6 +52,14 @@ func TestReconcileRouteTables(t *testing.T) { expectedError string expect func(s *mock_routetables.MockRouteTableScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ + { + name: "noop if no route table specs are found", + expectedError: "", + expect: func(s *mock_routetables.MockRouteTableScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.IsVnetManaged().Return(true) + s.RouteTableSpecs().Return([]azure.ResourceSpecGetter{}) + }, + }, { name: "create multiple route tables succeeds", expectedError: "", @@ -86,11 +94,10 @@ func TestReconcileRouteTables(t *testing.T) { }, }, { - name: "vnet is not managed", + name: "noop if vnet is not managed", expectedError: "", expect: func(s *mock_routetables.MockRouteTableScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.IsVnetManaged().Return(false) - s.UpdatePutStatus(infrav1.RouteTablesReadyCondition, serviceName, nil) }, }, } @@ -130,6 +137,14 @@ func TestDeleteRouteTable(t *testing.T) { expectedError string expect func(s *mock_routetables.MockRouteTableScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ + { + name: "noop if no route table specs are found", + expectedError: "", + expect: func(s *mock_routetables.MockRouteTableScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.IsVnetManaged().Return(true) + s.RouteTableSpecs().Return([]azure.ResourceSpecGetter{}) + }, + }, { name: "delete multiple route tables succeeds", expectedError: "", @@ -164,7 +179,7 @@ func TestDeleteRouteTable(t *testing.T) { }, }, { - name: "vnet is not managed", + name: "noop if vnet is not managed", expectedError: "", expect: func(s *mock_routetables.MockRouteTableScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.IsVnetManaged().Return(false) diff --git a/azure/services/subnets/spec_test.go b/azure/services/subnets/spec_test.go index 750a7b755bc..f18d7a9ea2d 100644 --- a/azure/services/subnets/spec_test.go +++ b/azure/services/subnets/spec_test.go @@ -76,34 +76,6 @@ var ( }, } - fakeSubnetSpecNotManaged = SubnetSpec{ - Name: "my-subnet-1", - ResourceGroup: "my-rg", - SubscriptionID: "123", - CIDRs: []string{"10.0.0.0/16"}, - IsVNetManaged: false, - VNetName: "my-vnet", - VNetResourceGroup: "my-vnet-rg", - RouteTableName: "my-subnet_route_table", - SecurityGroupName: "my-sg-1", - Role: infrav1.SubnetNode, - } - fakeSubnetNotManaged = network.Subnet{ - ID: to.StringPtr("/subscriptions/123/resourceGroups/my-rg/providers/Microsoft.Network/virtualNetworks/my-vnet/subnets/my-subnet-1"), - Name: to.StringPtr("my-subnet-1"), - SubnetPropertiesFormat: &network.SubnetPropertiesFormat{ - AddressPrefix: to.StringPtr("10.0.0.0/16"), - RouteTable: &network.RouteTable{ - ID: to.StringPtr("rt-id"), - Name: to.StringPtr("my-subnet_route_table"), - }, - NetworkSecurityGroup: &network.SecurityGroup{ - ID: to.StringPtr("sg-id-1"), - Name: to.StringPtr("my-sg-1"), - }, - }, - } - fakeIpv6SubnetSpecNotManaged = SubnetSpec{ Name: "my-ipv6-subnet", ResourceGroup: "my-rg", diff --git a/azure/services/subnets/subnets.go b/azure/services/subnets/subnets.go index d00d4511ac7..aa3a713e840 100644 --- a/azure/services/subnets/subnets.go +++ b/azure/services/subnets/subnets.go @@ -63,11 +63,16 @@ func (s *Service) Reconcile(ctx context.Context) error { ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout) defer cancel() + specs := s.Scope.SubnetSpecs() + if len(specs) == 0 { + return nil + } + // We go through the list of SubnetSpecs to reconcile each one, independently of the result of the previous one. // If multiple errors occur, we return the most pressing one. // Order of precedence (highest -> lowest) is: error that is not an operationNotDoneError (i.e. error creating) -> operationNotDoneError (i.e. creating in progress) -> no error (i.e. created) var resultErr error - for _, subnetSpec := range s.Scope.SubnetSpecs() { + for _, subnetSpec := range specs { result, err := s.CreateResource(ctx, subnetSpec, serviceName) if err != nil { if !azure.IsOperationNotDoneError(err) || resultErr == nil { @@ -90,7 +95,10 @@ func (s *Service) Reconcile(ctx context.Context) error { } } - s.Scope.UpdatePutStatus(infrav1.SubnetsReadyCondition, serviceName, resultErr) + if s.Scope.IsVnetManaged() { + s.Scope.UpdatePutStatus(infrav1.SubnetsReadyCondition, serviceName, resultErr) + } + return resultErr } @@ -104,23 +112,27 @@ func (s *Service) Delete(ctx context.Context) error { if !s.Scope.IsVnetManaged() { log.V(4).Info("Skipping subnets deletion in custom vnet mode") - - s.Scope.UpdateDeleteStatus(infrav1.SubnetsReadyCondition, serviceName, nil) return nil } - var result error + specs := s.Scope.SubnetSpecs() + if len(specs) == 0 { + return nil + } // We go through the list of SubnetSpecs to delete each one, independently of the result of the previous one. // If multiple errors occur, we return the most pressing one. // Order of precedence (highest -> lowest) is: error that is not an operationNotDoneError (i.e. error deleting) -> operationNotDoneError (i.e. deleting in progress) -> no error (i.e. deleted) - for _, subnetSpec := range s.Scope.SubnetSpecs() { + var result error + for _, subnetSpec := range specs { if err := s.DeleteResource(ctx, subnetSpec, serviceName); err != nil { if !azure.IsOperationNotDoneError(err) || result == nil { result = err } } } + s.Scope.UpdateDeleteStatus(infrav1.SubnetsReadyCondition, serviceName, result) + return result } diff --git a/azure/services/subnets/subnets_test.go b/azure/services/subnets/subnets_test.go index 32ddb495993..568cee861b8 100644 --- a/azure/services/subnets/subnets_test.go +++ b/azure/services/subnets/subnets_test.go @@ -26,6 +26,7 @@ import ( "github.com/Azure/go-autorest/autorest/to" "github.com/golang/mock/gomock" . "github.com/onsi/gomega" + "github.com/pkg/errors" infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" "sigs.k8s.io/cluster-api-provider-azure/azure" "sigs.k8s.io/cluster-api-provider-azure/azure/services/async/mock_async" @@ -92,6 +93,34 @@ var ( }, } + fakeSubnetSpecNotManaged = SubnetSpec{ + Name: "my-subnet-1", + ResourceGroup: "my-rg", + SubscriptionID: "123", + CIDRs: []string{"10.0.0.0/16"}, + IsVNetManaged: false, + VNetName: "my-vnet", + VNetResourceGroup: "my-vnet-rg", + RouteTableName: "my-subnet_route_table", + SecurityGroupName: "my-sg-1", + Role: infrav1.SubnetNode, + } + fakeSubnetNotManaged = network.Subnet{ + ID: to.StringPtr("/subscriptions/123/resourceGroups/my-rg/providers/Microsoft.Network/virtualNetworks/my-vnet/subnets/my-subnet-1"), + Name: to.StringPtr("my-subnet-1"), + SubnetPropertiesFormat: &network.SubnetPropertiesFormat{ + AddressPrefix: to.StringPtr("10.0.0.0/16"), + RouteTable: &network.RouteTable{ + ID: to.StringPtr("rt-id"), + Name: to.StringPtr("my-subnet_route_table"), + }, + NetworkSecurityGroup: &network.SecurityGroup{ + ID: to.StringPtr("sg-id-1"), + Name: to.StringPtr("my-sg-1"), + }, + }, + } + fakeCtrlPlaneSubnetSpec = SubnetSpec{ Name: "my-subnet-ctrl-plane", ResourceGroup: "my-rg", @@ -157,6 +186,8 @@ var ( }, } + notASubnet = "not a subnet" + notASubnetErr = errors.Errorf("%T is not a network.Subnet", notASubnet) internalError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error") ) @@ -166,6 +197,13 @@ func TestReconcileSubnets(t *testing.T) { expectedError string expect func(s *mock_subnets.MockSubnetScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ + { + name: "noop if no subnet specs are found", + expectedError: "", + expect: func(s *mock_subnets.MockSubnetScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.SubnetSpecs().Return([]azure.ResourceSpecGetter{}) + }, + }, { name: "create subnet", expectedError: "", @@ -176,6 +214,7 @@ func TestReconcileSubnets(t *testing.T) { s.UpdateSubnetID(fakeSubnetSpec1.Name, to.String(fakeSubnet1.ID)) s.UpdateSubnetCIDRs(fakeSubnetSpec1.Name, []string{to.String(fakeSubnet1.AddressPrefix)}) + s.IsVnetManaged().AnyTimes().Return(true) s.UpdatePutStatus(infrav1.SubnetsReadyCondition, serviceName, nil) }, }, @@ -193,9 +232,23 @@ func TestReconcileSubnets(t *testing.T) { s.UpdateSubnetID(fakeSubnetSpec2.Name, to.String(fakeSubnet2.ID)) s.UpdateSubnetCIDRs(fakeSubnetSpec2.Name, []string{to.String(fakeSubnet2.AddressPrefix)}) + s.IsVnetManaged().AnyTimes().Return(true) s.UpdatePutStatus(infrav1.SubnetsReadyCondition, serviceName, nil) }, }, + { + name: "don't update ready condition when subnet not managed", + expectedError: "", + expect: func(s *mock_subnets.MockSubnetScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.SubnetSpecs().Return([]azure.ResourceSpecGetter{&fakeSubnetSpecNotManaged}) + + r.CreateResource(gomockinternal.AContext(), &fakeSubnetSpecNotManaged, serviceName).Return(fakeSubnetNotManaged, nil) + s.UpdateSubnetID(fakeSubnetSpecNotManaged.Name, to.String(fakeSubnetNotManaged.ID)) + s.UpdateSubnetCIDRs(fakeSubnetSpecNotManaged.Name, []string{to.String(fakeSubnetNotManaged.AddressPrefix)}) + + s.IsVnetManaged().AnyTimes().Return(false) + }, + }, { name: "create ipv6 subnet", expectedError: "", @@ -206,6 +259,7 @@ func TestReconcileSubnets(t *testing.T) { s.UpdateSubnetID(fakeIpv6SubnetSpec.Name, to.String(fakeIpv6Subnet.ID)) s.UpdateSubnetCIDRs(fakeIpv6SubnetSpec.Name, to.StringSlice(fakeIpv6Subnet.AddressPrefixes)) + s.IsVnetManaged().AnyTimes().Return(true) s.UpdatePutStatus(infrav1.SubnetsReadyCondition, serviceName, nil) }, }, @@ -223,6 +277,7 @@ func TestReconcileSubnets(t *testing.T) { s.UpdateSubnetID(fakeIpv6SubnetSpecCP.Name, to.String(fakeIpv6SubnetCP.ID)) s.UpdateSubnetCIDRs(fakeIpv6SubnetSpecCP.Name, to.StringSlice(fakeIpv6SubnetCP.AddressPrefixes)) + s.IsVnetManaged().AnyTimes().Return(true) s.UpdatePutStatus(infrav1.SubnetsReadyCondition, serviceName, nil) }, }, @@ -232,9 +287,19 @@ func TestReconcileSubnets(t *testing.T) { expect: func(s *mock_subnets.MockSubnetScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.SubnetSpecs().Return([]azure.ResourceSpecGetter{&fakeSubnetSpec1}) r.CreateResource(gomockinternal.AContext(), &fakeSubnetSpec1, serviceName).Return(nil, internalError) + + s.IsVnetManaged().AnyTimes().Return(true) s.UpdatePutStatus(infrav1.SubnetsReadyCondition, serviceName, internalError) }, }, + { + name: "create returns a non subnet", + expectedError: notASubnetErr.Error(), + expect: func(s *mock_subnets.MockSubnetScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.SubnetSpecs().Return([]azure.ResourceSpecGetter{&fakeSubnetSpec1}) + r.CreateResource(gomockinternal.AContext(), &fakeSubnetSpec1, serviceName).Return(notASubnet, nil) + }, + }, { name: "fail to create subnets", expectedError: "#: Internal Server Error: StatusCode=500", @@ -246,6 +311,7 @@ func TestReconcileSubnets(t *testing.T) { s.UpdateSubnetID(fakeSubnetSpec2.Name, to.String(fakeSubnet2.ID)) s.UpdateSubnetCIDRs(fakeSubnetSpec2.Name, []string{to.String(fakeSubnet2.AddressPrefix)}) + s.IsVnetManaged().AnyTimes().Return(true) s.UpdatePutStatus(infrav1.SubnetsReadyCondition, serviceName, internalError) }, }, @@ -286,11 +352,19 @@ func TestDeleteSubnets(t *testing.T) { expectedError string expect func(s *mock_subnets.MockSubnetScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ + { + name: "noop if no subnet specs are found", + expectedError: "", + expect: func(s *mock_subnets.MockSubnetScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.IsVnetManaged().AnyTimes().Return(true) + s.SubnetSpecs().Return([]azure.ResourceSpecGetter{}) + }, + }, { name: "subnets deleted successfully", expectedError: "", expect: func(s *mock_subnets.MockSubnetScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { - s.IsVnetManaged().Return(true) + s.IsVnetManaged().AnyTimes().Return(true) s.SubnetSpecs().Return([]azure.ResourceSpecGetter{&fakeSubnetSpec1, &fakeSubnetSpec2}) r.DeleteResource(gomockinternal.AContext(), &fakeSubnetSpec1, serviceName).Return(nil) r.DeleteResource(gomockinternal.AContext(), &fakeSubnetSpec2, serviceName).Return(nil) @@ -301,7 +375,7 @@ func TestDeleteSubnets(t *testing.T) { name: "node subnet and controlplane subnet deleted successfully", expectedError: "", expect: func(s *mock_subnets.MockSubnetScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { - s.IsVnetManaged().Return(true) + s.IsVnetManaged().AnyTimes().Return(true) s.SubnetSpecs().Return([]azure.ResourceSpecGetter{&fakeSubnetSpec1, &fakeCtrlPlaneSubnetSpec}) r.DeleteResource(gomockinternal.AContext(), &fakeSubnetSpec1, serviceName).Return(nil) r.DeleteResource(gomockinternal.AContext(), &fakeCtrlPlaneSubnetSpec, serviceName).Return(nil) @@ -309,18 +383,17 @@ func TestDeleteSubnets(t *testing.T) { }, }, { - name: "skip delete if vnet is managed", + name: "skip delete if vnet is not managed", expectedError: "", expect: func(s *mock_subnets.MockSubnetScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { - s.IsVnetManaged().Return(false) - s.UpdateDeleteStatus(infrav1.SubnetsReadyCondition, serviceName, nil) + s.IsVnetManaged().AnyTimes().Return(false) }, }, { name: "fail delete subnet", expectedError: "#: Internal Server Error: StatusCode=500", expect: func(s *mock_subnets.MockSubnetScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { - s.IsVnetManaged().Return(true) + s.IsVnetManaged().AnyTimes().Return(true) s.SubnetSpecs().Return([]azure.ResourceSpecGetter{&fakeSubnetSpec1}) r.DeleteResource(gomockinternal.AContext(), &fakeSubnetSpec1, serviceName).Return(internalError) s.UpdateDeleteStatus(infrav1.SubnetsReadyCondition, serviceName, internalError) diff --git a/azure/services/virtualmachines/virtualmachines.go b/azure/services/virtualmachines/virtualmachines.go index 1f47007d755..57daeaa0678 100644 --- a/azure/services/virtualmachines/virtualmachines.go +++ b/azure/services/virtualmachines/virtualmachines.go @@ -76,6 +76,9 @@ func (s *Service) Reconcile(ctx context.Context) error { defer cancel() vmSpec := s.Scope.VMSpec() + if vmSpec == nil { + return nil + } result, err := s.CreateResource(ctx, vmSpec, serviceName) s.Scope.UpdatePutStatus(infrav1.VMRunningCondition, serviceName, err) @@ -113,6 +116,9 @@ func (s *Service) Delete(ctx context.Context) error { defer cancel() vmSpec := s.Scope.VMSpec() + if vmSpec == nil { + return nil + } err := s.DeleteResource(ctx, vmSpec, serviceName) if err != nil { diff --git a/azure/services/virtualmachines/virtualmachines_test.go b/azure/services/virtualmachines/virtualmachines_test.go index 8f07c3ed899..aeeff3e5c61 100644 --- a/azure/services/virtualmachines/virtualmachines_test.go +++ b/azure/services/virtualmachines/virtualmachines_test.go @@ -112,6 +112,13 @@ func TestReconcileVM(t *testing.T) { expectedError string expect func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_async.MockGetterMockRecorder, mpip *mock_publicips.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ + { + name: "noop if no vm spec is found", + expectedError: "", + expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_async.MockGetterMockRecorder, mpip *mock_publicips.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.VMSpec().Return(nil) + }, + }, { name: "create vm succeeds", expectedError: "", @@ -206,6 +213,13 @@ func TestDeleteVM(t *testing.T) { expectedError string expect func(s *mock_virtualmachines.MockVMScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ + { + name: "noop if no vm spec is found", + expectedError: "", + expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.VMSpec().Return(nil) + }, + }, { name: "vm doesn't exist", expectedError: "", diff --git a/azure/services/virtualnetworks/mock_virtualnetworks/virtualnetworks_mock.go b/azure/services/virtualnetworks/mock_virtualnetworks/virtualnetworks_mock.go index 982acc86d73..ce60b8e9f32 100644 --- a/azure/services/virtualnetworks/mock_virtualnetworks/virtualnetworks_mock.go +++ b/azure/services/virtualnetworks/mock_virtualnetworks/virtualnetworks_mock.go @@ -177,6 +177,20 @@ func (mr *MockVNetScopeMockRecorder) HashKey() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HashKey", reflect.TypeOf((*MockVNetScope)(nil).HashKey)) } +// IsVnetManaged mocks base method. +func (m *MockVNetScope) IsVnetManaged() bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "IsVnetManaged") + ret0, _ := ret[0].(bool) + return ret0 +} + +// IsVnetManaged indicates an expected call of IsVnetManaged. +func (mr *MockVNetScopeMockRecorder) IsVnetManaged() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsVnetManaged", reflect.TypeOf((*MockVNetScope)(nil).IsVnetManaged)) +} + // SetLongRunningOperationState mocks base method. func (m *MockVNetScope) SetLongRunningOperationState(arg0 *v1beta1.Future) { m.ctrl.T.Helper() diff --git a/azure/services/virtualnetworks/virtualnetworks.go b/azure/services/virtualnetworks/virtualnetworks.go index 4be1cd731a5..8206e52cfc0 100644 --- a/azure/services/virtualnetworks/virtualnetworks.go +++ b/azure/services/virtualnetworks/virtualnetworks.go @@ -39,6 +39,7 @@ type VNetScope interface { Vnet() *infrav1.VnetSpec VNetSpec() azure.ResourceSpecGetter ClusterName() string + IsVnetManaged() bool } // Service provides operations on Azure resources. @@ -66,9 +67,11 @@ func (s *Service) Reconcile(ctx context.Context) error { defer cancel() vnetSpec := s.Scope.VNetSpec() + if vnetSpec == nil { + return nil + } result, err := s.CreateResource(ctx, vnetSpec, serviceName) - s.Scope.UpdatePutStatus(infrav1.VNetReadyCondition, serviceName, err) if err == nil && result != nil { existingVnet, ok := result.(network.VirtualNetwork) if !ok { @@ -84,6 +87,11 @@ func (s *Service) Reconcile(ctx context.Context) error { } vnet.CIDRBlocks = prefixes } + + if s.Scope.IsVnetManaged() { + s.Scope.UpdatePutStatus(infrav1.VNetReadyCondition, serviceName, err) + } + return err } @@ -96,6 +104,9 @@ func (s *Service) Delete(ctx context.Context) error { defer cancel() vnetSpec := s.Scope.VNetSpec() + if vnetSpec == nil { + return nil + } // Check that the vnet is not BYO. managed, err := s.IsManaged(ctx, vnetSpec) diff --git a/azure/services/virtualnetworks/virtualnetworks_test.go b/azure/services/virtualnetworks/virtualnetworks_test.go index fcdb76a863f..6cbde7ba33a 100644 --- a/azure/services/virtualnetworks/virtualnetworks_test.go +++ b/azure/services/virtualnetworks/virtualnetworks_test.go @@ -67,12 +67,29 @@ func TestReconcileVnet(t *testing.T) { expectedError string expect func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ + { + name: "noop if no vnet spec is found", + expectedError: "", + expect: func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.VNetSpec().Return(nil) + }, + }, + { + name: "reconcile when vnet is not managed", + expectedError: "", + expect: func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.VNetSpec().Return(&fakeVNetSpec) + r.CreateResource(gomockinternal.AContext(), &fakeVNetSpec, serviceName).Return(nil, nil) + s.IsVnetManaged().Return(false) + }, + }, { name: "create vnet succeeds, should not return an error", expectedError: "", expect: func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.VNetSpec().Return(&fakeVNetSpec) r.CreateResource(gomockinternal.AContext(), &fakeVNetSpec, serviceName).Return(nil, nil) + s.IsVnetManaged().Return(true) s.UpdatePutStatus(infrav1.VNetReadyCondition, serviceName, nil) }, }, @@ -82,6 +99,7 @@ func TestReconcileVnet(t *testing.T) { expect: func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.VNetSpec().Return(&fakeVNetSpec) r.CreateResource(gomockinternal.AContext(), &fakeVNetSpec, serviceName).Return(nil, internalError) + s.IsVnetManaged().Return(true) s.UpdatePutStatus(infrav1.VNetReadyCondition, serviceName, internalError) }, }, @@ -124,6 +142,13 @@ func TestDeleteVnet(t *testing.T) { expectedError string expect func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ + { + name: "noop if no vnet spec is found", + expectedError: "", + expect: func(s *mock_virtualnetworks.MockVNetScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.VNetSpec().Return(nil) + }, + }, { name: "delete vnet succeeds, should not return an error", expectedError: "", diff --git a/azure/services/vnetpeerings/vnetpeerings.go b/azure/services/vnetpeerings/vnetpeerings.go index 1012097d45d..0aa995eda87 100644 --- a/azure/services/vnetpeerings/vnetpeerings.go +++ b/azure/services/vnetpeerings/vnetpeerings.go @@ -58,11 +58,16 @@ func (s *Service) Reconcile(ctx context.Context) error { ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout) defer cancel() + specs := s.Scope.VnetPeeringSpecs() + if len(specs) == 0 { + return nil + } + // We go through the list of VnetPeeringSpecs to reconcile each one, independently of the result of the previous one. // If multiple errors occur, we return the most pressing one. // Order of precedence (highest -> lowest) is: error that is not an operationNotDoneError (i.e. error creating) -> operationNotDoneError (i.e. creating in progress) -> no error (i.e. created) var result error - for _, peeringSpec := range s.Scope.VnetPeeringSpecs() { + for _, peeringSpec := range specs { if _, err := s.CreateResource(ctx, peeringSpec, serviceName); err != nil { if !azure.IsOperationNotDoneError(err) || result == nil { result = err @@ -82,12 +87,16 @@ func (s *Service) Delete(ctx context.Context) error { ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout) defer cancel() - var result error + specs := s.Scope.VnetPeeringSpecs() + if len(specs) == 0 { + return nil + } // We go through the list of VnetPeeringSpecs to delete each one, independently of the result of the previous one. // If multiple errors occur, we return the most pressing one. // Order of precedence (highest -> lowest) is: error that is not an operationNotDoneError (i.e. error deleting) -> operationNotDoneError (i.e. deleting in progress) -> no error (i.e. deleted) - for _, peeringSpec := range s.Scope.VnetPeeringSpecs() { + var result error + for _, peeringSpec := range specs { if err := s.DeleteResource(ctx, peeringSpec, serviceName); err != nil { if !azure.IsOperationNotDoneError(err) || result == nil { result = err diff --git a/azure/services/vnetpeerings/vnetpeerings_test.go b/azure/services/vnetpeerings/vnetpeerings_test.go index db70ac2933f..48442eec6d6 100644 --- a/azure/services/vnetpeerings/vnetpeerings_test.go +++ b/azure/services/vnetpeerings/vnetpeerings_test.go @@ -95,11 +95,10 @@ func TestReconcileVnetPeerings(t *testing.T) { }, }, { - name: "create no peerings", + name: "noop if no peering specs are found", expectedError: "", expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { - p.VnetPeeringSpecs().Return(fakePeeringSpecs[:0]) - p.UpdatePutStatus(infrav1.VnetPeeringReadyCondition, serviceName, nil) + p.VnetPeeringSpecs().Return([]azure.ResourceSpecGetter{}) }, }, { @@ -242,11 +241,10 @@ func TestDeleteVnetPeerings(t *testing.T) { }, }, { - name: "delete no peerings", + name: "noop if no peering specs are found", expectedError: "", expect: func(p *mock_vnetpeerings.MockVnetPeeringScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) { - p.VnetPeeringSpecs().Return(fakePeeringSpecs[:0]) - p.UpdateDeleteStatus(infrav1.VnetPeeringReadyCondition, serviceName, nil) + p.VnetPeeringSpecs().Return([]azure.ResourceSpecGetter{}) }, }, { diff --git a/controllers/azurecluster_controller.go b/controllers/azurecluster_controller.go index b0850170492..ae27afb11fb 100644 --- a/controllers/azurecluster_controller.go +++ b/controllers/azurecluster_controller.go @@ -241,6 +241,7 @@ func (acr *AzureClusterReconciler) reconcileNormal(ctx context.Context, clusterS wrappedErr := errors.Wrap(err, "failed to reconcile cluster services") acr.Recorder.Eventf(azureCluster, corev1.EventTypeWarning, "ClusterReconcilerNormalFailed", wrappedErr.Error()) + conditions.MarkFalse(azureCluster, infrav1.NetworkInfrastructureReadyCondition, infrav1.FailedReason, clusterv1.ConditionSeverityError, wrappedErr.Error()) return reconcile.Result{}, wrappedErr }