[release-1.9] Delete security rules if removed from spec

azure/const.go

@@ -35,6 +35,12 @@ const (
 	// for annotation formatting rules.
 	ManagedClusterTagsLastAppliedAnnotation = "sigs.k8s.io/cluster-api-provider-azure-last-applied-tags-managedcluster"
 
+	// SecurityRuleLastAppliedAnnotation is the key for the Azure Cluster
+	// object annotation which tracks the security rules for security groups.
+	// See https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/
+	// for annotation formatting rules.
+	SecurityRuleLastAppliedAnnotation = "sigs.k8s.io/cluster-api-provider-azure-last-applied-security-rules"
+
 	// CustomDataHashAnnotation is the key for the machine object annotation
 	// which tracks the hash of the custom data.
 	// See https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/

azure/scope/cluster.go

@@ -351,12 +351,13 @@
 	nsgspecs := make([]azure.ResourceSpecGetter, len(s.AzureCluster.Spec.NetworkSpec.Subnets))
 	for i, subnet := range s.AzureCluster.Spec.NetworkSpec.Subnets {
 		nsgspecs[i] = &securitygroups.NSGSpec{
-			Name:           subnet.SecurityGroup.Name,
-			SecurityRules:  subnet.SecurityGroup.SecurityRules,
-			ResourceGroup:  s.ResourceGroup(),
-			Location:       s.Location(),
-			ClusterName:    s.ClusterName(),
-			AdditionalTags: s.AdditionalTags(),
+			Name:                     subnet.SecurityGroup.Name,
+			SecurityRules:            subnet.SecurityGroup.SecurityRules,
+			ResourceGroup:            s.ResourceGroup(),
+			Location:                 s.Location(),
+			ClusterName:              s.ClusterName(),
+			AdditionalTags:           s.AdditionalTags(),
+			LastAppliedSecurityRules: s.getLastAppliedSecurityRules(subnet.SecurityGroup.Name),
 		}
 	}
 
@@ -1100,3 +1101,18 @@
 
 	return privateEndpointSpecs
 }
+
+func (s *ClusterScope) getLastAppliedSecurityRules(nsgName string) map[string]interface{} {
+	// Retrieve the last applied security rules for all NSGs.
+	lastAppliedSecurityRulesAll, err := s.AnnotationJSON(azure.SecurityRuleLastAppliedAnnotation)
+	if err != nil {
+		return map[string]interface{}{}
+	}
+
+	// Retrieve the last applied security rules for this NSG.
+	lastAppliedSecurityRules, ok := lastAppliedSecurityRulesAll[nsgName].(map[string]interface{})
+	if !ok {
+		lastAppliedSecurityRules = map[string]interface{}{}
+	}
+	return lastAppliedSecurityRules
+}

azure/scope/cluster_test.go

@@ -1172,10 +1172,11 @@ func TestNSGSpecs(t *testing.T) {
 							Name: "fake-rule-1",
 						},
 					},
-					ResourceGroup:  "my-rg",
-					Location:       "centralIndia",
-					ClusterName:    "my-cluster",
-					AdditionalTags: make(infrav1.Tags),
+					ResourceGroup:            "my-rg",
+					Location:                 "centralIndia",
+					ClusterName:              "my-cluster",
+					AdditionalTags:           make(infrav1.Tags),
+					LastAppliedSecurityRules: map[string]interface{}{},
 				},
 			},
 		},

azure/services/securitygroups/securitygroups.go

@@ -35,6 +35,7 @@
 	azure.AsyncStatusUpdater
 	NSGSpecs() []azure.ResourceSpecGetter
 	IsVnetManaged() bool
+	UpdateAnnotationJSON(string, map[string]interface{}) error
 }
 
 // Service provides operations on Azure resources.
@@ -80,15 +81,32 @@
 
 	var resErr error
 
+	newAnnotation := make(map[string]interface{})
+
 	// We go through the list of security groups 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 _, nsgSpec := range specs {
+	for _, resourceSpec := range specs {
+		nsgSpec := resourceSpec.(*NSGSpec)
+		currentAnnotation := make(map[string]string)
+
 		if _, err := s.CreateOrUpdateResource(ctx, nsgSpec, serviceName); err != nil {
 			if !azure.IsOperationNotDoneError(err) || resErr == nil {
 				resErr = err
 			}
 		}
+
+		for _, rule := range nsgSpec.SecurityRules {
+			currentAnnotation[rule.Name] = rule.Description
+		}
+
+		if len(currentAnnotation) > 0 {
+			newAnnotation[nsgSpec.Name] = currentAnnotation
+		}
+	}
+
+	if err := s.Scope.UpdateAnnotationJSON(azure.SecurityRuleLastAppliedAnnotation, newAnnotation); err != nil {
+		return err
 	}
 
 	s.Scope.UpdatePutStatus(infrav1.SecurityGroupsReadyCondition, serviceName, resErr)

azure/services/securitygroups/securitygroups_test.go

@@ -33,43 +33,55 @@ import (
 )
 
 var (
-	fakeNSG = NSGSpec{
+	annotation = azure.SecurityRuleLastAppliedAnnotation
+	fakeNSG    = NSGSpec{
 		Name:        "test-nsg",
 		Location:    "test-location",
 		ClusterName: "my-cluster",
 		SecurityRules: infrav1.SecurityRules{
-			{
-				Name:             "allow_ssh",
-				Description:      "Allow SSH",
-				Priority:         2200,
-				Protocol:         infrav1.SecurityGroupProtocolTCP,
-				Direction:        infrav1.SecurityRuleDirectionInbound,
-				Source:           pointer.String("*"),
-				SourcePorts:      pointer.String("*"),
-				Destination:      pointer.String("*"),
-				DestinationPorts: pointer.String("22"),
-			},
-			{
-				Name:             "other_rule",
-				Description:      "Test Rule",
-				Priority:         500,
-				Protocol:         infrav1.SecurityGroupProtocolTCP,
-				Direction:        infrav1.SecurityRuleDirectionInbound,
-				Source:           pointer.String("*"),
-				SourcePorts:      pointer.String("*"),
-				Destination:      pointer.String("*"),
-				DestinationPorts: pointer.String("80"),
-			},
+			securityRule1,
 		},
 		ResourceGroup: "test-group",
 	}
-	fakeNSG2 = NSGSpec{
+	noRulesNSG = NSGSpec{
 		Name:          "test-nsg-2",
 		Location:      "test-location",
 		ClusterName:   "my-cluster",
 		SecurityRules: infrav1.SecurityRules{},
 		ResourceGroup: "test-group",
 	}
+	multipleRulesNSG = NSGSpec{
+		Name:        "multiple-rules-nsg",
+		Location:    "test-location",
+		ClusterName: "my-cluster",
+		SecurityRules: infrav1.SecurityRules{
+			securityRule1,
+			securityRule2,
+		},
+		ResourceGroup: "test-group",
+	}
+	securityRule1 = infrav1.SecurityRule{
+		Name:             "allow_ssh",
+		Description:      "Allow SSH",
+		Priority:         2200,
+		Protocol:         infrav1.SecurityGroupProtocolTCP,
+		Direction:        infrav1.SecurityRuleDirectionInbound,
+		Source:           pointer.String("*"),
+		SourcePorts:      pointer.String("*"),
+		Destination:      pointer.String("*"),
+		DestinationPorts: pointer.String("22"),
+	}
+	securityRule2 = infrav1.SecurityRule{
+		Name:             "other_rule",
+		Description:      "Test Rule",
+		Priority:         500,
+		Protocol:         infrav1.SecurityGroupProtocolTCP,
+		Direction:        infrav1.SecurityRuleDirectionInbound,
+		Source:           pointer.String("*"),
+		SourcePorts:      pointer.String("*"),
+		Destination:      pointer.String("*"),
+		DestinationPorts: pointer.String("80"),
+	}
 	errFake      = errors.New("this is an error")
 	notDoneError = azure.NewOperationNotDoneError(&infrav1.Future{})
 )
@@ -81,13 +93,36 @@ func TestReconcileSecurityGroups(t *testing.T) {
 		expect        func(s *mock_securitygroups.MockNSGScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder)
 	}{
 		{
-			name:          "create multiple security groups succeeds, should return no error",
+			name:          "create single security group with single rule succeeds, should return no error",
+			expectedError: "",
+			expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
+				s.IsVnetManaged().Return(true)
+				s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG})
+				s.UpdateAnnotationJSON(annotation, map[string]interface{}{fakeNSG.Name: map[string]string{securityRule1.Name: securityRule1.Description}}).Times(1)
+				r.CreateOrUpdateResource(gomockinternal.AContext(), &fakeNSG, serviceName).Return(nil, nil)
+				s.UpdatePutStatus(infrav1.SecurityGroupsReadyCondition, serviceName, nil)
+			},
+		},
+		{
+			name:          "create single security group with multiple rules succeeds, should return no error",
+			expectedError: "",
+			expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
+				s.IsVnetManaged().Return(true)
+				s.NSGSpecs().Return([]azure.ResourceSpecGetter{&multipleRulesNSG})
+				s.UpdateAnnotationJSON(annotation, map[string]interface{}{multipleRulesNSG.Name: map[string]string{securityRule1.Name: securityRule1.Description, securityRule2.Name: securityRule2.Description}}).Times(1)
+				r.CreateOrUpdateResource(gomockinternal.AContext(), &multipleRulesNSG, serviceName).Return(nil, nil)
+				s.UpdatePutStatus(infrav1.SecurityGroupsReadyCondition, serviceName, nil)
+			},
+		},
+		{
+			name:          "create multiple security groups, should return no error",
 			expectedError: "",
 			expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
 				s.IsVnetManaged().Return(true)
-				s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &fakeNSG2})
+				s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &noRulesNSG})
+				s.UpdateAnnotationJSON(annotation, map[string]interface{}{fakeNSG.Name: map[string]string{securityRule1.Name: securityRule1.Description}}).Times(1)
 				r.CreateOrUpdateResource(gomockinternal.AContext(), &fakeNSG, serviceName).Return(nil, nil)
-				r.CreateOrUpdateResource(gomockinternal.AContext(), &fakeNSG2, serviceName).Return(nil, nil)
+				r.CreateOrUpdateResource(gomockinternal.AContext(), &noRulesNSG, serviceName).Return(nil, nil)
 				s.UpdatePutStatus(infrav1.SecurityGroupsReadyCondition, serviceName, nil)
 			},
 		},
@@ -96,9 +131,10 @@ func TestReconcileSecurityGroups(t *testing.T) {
 			expectedError: errFake.Error(),
 			expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
 				s.IsVnetManaged().Return(true)
-				s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &fakeNSG2})
+				s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &noRulesNSG})
+				s.UpdateAnnotationJSON(annotation, map[string]interface{}{fakeNSG.Name: map[string]string{securityRule1.Name: securityRule1.Description}}).Times(1)
 				r.CreateOrUpdateResource(gomockinternal.AContext(), &fakeNSG, serviceName).Return(nil, errFake)
-				r.CreateOrUpdateResource(gomockinternal.AContext(), &fakeNSG2, serviceName).Return(nil, nil)
+				r.CreateOrUpdateResource(gomockinternal.AContext(), &noRulesNSG, serviceName).Return(nil, nil)
 				s.UpdatePutStatus(infrav1.SecurityGroupsReadyCondition, serviceName, errFake)
 			},
 		},
@@ -107,9 +143,10 @@ func TestReconcileSecurityGroups(t *testing.T) {
 			expectedError: errFake.Error(),
 			expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
 				s.IsVnetManaged().Return(true)
-				s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &fakeNSG2})
+				s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &noRulesNSG})
+				s.UpdateAnnotationJSON(annotation, map[string]interface{}{fakeNSG.Name: map[string]string{securityRule1.Name: securityRule1.Description}}).Times(1)
 				r.CreateOrUpdateResource(gomockinternal.AContext(), &fakeNSG, serviceName).Return(nil, errFake)
-				r.CreateOrUpdateResource(gomockinternal.AContext(), &fakeNSG2, serviceName).Return(nil, notDoneError)
+				r.CreateOrUpdateResource(gomockinternal.AContext(), &noRulesNSG, serviceName).Return(nil, notDoneError)
 				s.UpdatePutStatus(infrav1.SecurityGroupsReadyCondition, serviceName, errFake)
 			},
 		},
@@ -119,6 +156,7 @@ func TestReconcileSecurityGroups(t *testing.T) {
 			expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
 				s.IsVnetManaged().Return(true)
 				s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG})
+				s.UpdateAnnotationJSON(annotation, map[string]interface{}{fakeNSG.Name: map[string]string{securityRule1.Name: securityRule1.Description}})
 				r.CreateOrUpdateResource(gomockinternal.AContext(), &fakeNSG, serviceName).Return(nil, notDoneError)
 				s.UpdatePutStatus(infrav1.SecurityGroupsReadyCondition, serviceName, notDoneError)
 			},
@@ -171,9 +209,9 @@ func TestDeleteSecurityGroups(t *testing.T) {
 			expectedError: "",
 			expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
 				s.IsVnetManaged().Return(true)
-				s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &fakeNSG2})
+				s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &noRulesNSG})
 				r.DeleteResource(gomockinternal.AContext(), &fakeNSG, serviceName).Return(nil)
-				r.DeleteResource(gomockinternal.AContext(), &fakeNSG2, serviceName).Return(nil)
+				r.DeleteResource(gomockinternal.AContext(), &noRulesNSG, serviceName).Return(nil)
 				s.UpdateDeleteStatus(infrav1.SecurityGroupsReadyCondition, serviceName, nil)
 			},
 		},
@@ -182,9 +220,9 @@ func TestDeleteSecurityGroups(t *testing.T) {
 			expectedError: errFake.Error(),
 			expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
 				s.IsVnetManaged().Return(true)
-				s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &fakeNSG2})
+				s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &noRulesNSG})
 				r.DeleteResource(gomockinternal.AContext(), &fakeNSG, serviceName).Return(errFake)
-				r.DeleteResource(gomockinternal.AContext(), &fakeNSG2, serviceName).Return(nil)
+				r.DeleteResource(gomockinternal.AContext(), &noRulesNSG, serviceName).Return(nil)
 				s.UpdateDeleteStatus(infrav1.SecurityGroupsReadyCondition, serviceName, errFake)
 			},
 		},
@@ -193,9 +231,9 @@ func TestDeleteSecurityGroups(t *testing.T) {
 			expectedError: errFake.Error(),
 			expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
 				s.IsVnetManaged().Return(true)
-				s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &fakeNSG2})
+				s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &noRulesNSG})
 				r.DeleteResource(gomockinternal.AContext(), &fakeNSG, serviceName).Return(errFake)
-				r.DeleteResource(gomockinternal.AContext(), &fakeNSG2, serviceName).Return(notDoneError)
+				r.DeleteResource(gomockinternal.AContext(), &noRulesNSG, serviceName).Return(notDoneError)
 				s.UpdateDeleteStatus(infrav1.SecurityGroupsReadyCondition, serviceName, errFake)
 			},
 		},

azure/services/securitygroups/spec.go

@@ -29,12 +29,13 @@ import (
 
 // NSGSpec defines the specification for a security group.
 type NSGSpec struct {
-	Name           string
-	SecurityRules  infrav1.SecurityRules
-	Location       string
-	ClusterName    string
-	ResourceGroup  string
-	AdditionalTags infrav1.Tags
+	Name                     string
+	SecurityRules            infrav1.SecurityRules
+	Location                 string
+	ClusterName              string
+	ResourceGroup            string
+	AdditionalTags           infrav1.Tags
+	LastAppliedSecurityRules map[string]interface{}
 }
 
 // ResourceName returns the name of the security group.
@@ -55,6 +56,7 @@ func (s *NSGSpec) OwnerResourceName() string {
 // Parameters returns the parameters for the security group.
 func (s *NSGSpec) Parameters(ctx context.Context, existing interface{}) (interface{}, error) {
 	securityRules := make([]network.SecurityRule, 0)
+	newAnnotation := map[string]string{}
 	var etag *string
 
 	if existing != nil {
@@ -67,14 +69,29 @@ func (s *NSGSpec) Parameters(ctx context.Context, existing interface{}) (interfa
 		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) {
+			if !ruleExists(*existingNSG.SecurityRules, sdkRule) {
 				update = true
 				securityRules = append(securityRules, sdkRule)
 			}
+			newAnnotation[rule.Name] = rule.Description
 		}
+
+		for _, oldRule := range *existingNSG.SecurityRules {
+			_, tracked := s.LastAppliedSecurityRules[*oldRule.Name]
+			// If rule is owned by CAPZ and applied last, and not found in the new rules, then it has been deleted
+			if _, ok := newAnnotation[*oldRule.Name]; !ok && tracked {
+				// Rule has been deleted
+				update = true
+				continue
+			}
+
+			// Add previous rules that haven't been deleted
+			securityRules = append(securityRules, oldRule)
+		}
+
 		if !update {
 			// Skip update for NSG as the required default rules are present
 			return nil, nil