diff --git a/azure/const.go b/azure/const.go index 33a5236c07a..cae610d71ec 100644 --- a/azure/const.go +++ b/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/ diff --git a/azure/scope/cluster.go b/azure/scope/cluster.go index 0de2fdf214a..6f36144f6b1 100644 --- a/azure/scope/cluster.go +++ b/azure/scope/cluster.go @@ -358,12 +358,13 @@ func (s *ClusterScope) NSGSpecs() []azure.ResourceSpecGetter { 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), } } @@ -1119,3 +1120,18 @@ func (s *ClusterScope) getPrivateEndpoints(subnet infrav1.SubnetSpec) []azure.Re 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 +} diff --git a/azure/scope/cluster_test.go b/azure/scope/cluster_test.go index f3d0ae94954..744d84fa6b5 100644 --- a/azure/scope/cluster_test.go +++ b/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{}{}, }, }, }, diff --git a/azure/services/securitygroups/mock_securitygroups/securitygroups_mock.go b/azure/services/securitygroups/mock_securitygroups/securitygroups_mock.go index d27d7aef24d..3dee07671c3 100644 --- a/azure/services/securitygroups/mock_securitygroups/securitygroups_mock.go +++ b/azure/services/securitygroups/mock_securitygroups/securitygroups_mock.go @@ -231,6 +231,20 @@ func (mr *MockNSGScopeMockRecorder) TenantID() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "TenantID", reflect.TypeOf((*MockNSGScope)(nil).TenantID)) } +// UpdateAnnotationJSON mocks base method. +func (m *MockNSGScope) UpdateAnnotationJSON(arg0 string, arg1 map[string]interface{}) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "UpdateAnnotationJSON", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// UpdateAnnotationJSON indicates an expected call of UpdateAnnotationJSON. +func (mr *MockNSGScopeMockRecorder) UpdateAnnotationJSON(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateAnnotationJSON", reflect.TypeOf((*MockNSGScope)(nil).UpdateAnnotationJSON), arg0, arg1) +} + // UpdateDeleteStatus mocks base method. func (m *MockNSGScope) UpdateDeleteStatus(arg0 v1beta10.ConditionType, arg1 string, arg2 error) { m.ctrl.T.Helper() diff --git a/azure/services/securitygroups/securitygroups.go b/azure/services/securitygroups/securitygroups.go index e49f46c1b73..430709e3182 100644 --- a/azure/services/securitygroups/securitygroups.go +++ b/azure/services/securitygroups/securitygroups.go @@ -35,6 +35,7 @@ type NSGScope interface { azure.AsyncStatusUpdater NSGSpecs() []azure.ResourceSpecGetter IsVnetManaged() bool + UpdateAnnotationJSON(string, map[string]interface{}) error } // Service provides operations on Azure resources. @@ -80,15 +81,32 @@ func (s *Service) Reconcile(ctx context.Context) error { 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) diff --git a/azure/services/securitygroups/securitygroups_test.go b/azure/services/securitygroups/securitygroups_test.go index e4bcf9149ca..3c3088d62cc 100644 --- a/azure/services/securitygroups/securitygroups_test.go +++ b/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) }, }, diff --git a/azure/services/securitygroups/spec.go b/azure/services/securitygroups/spec.go index 8d503add478..119e2c33197 100644 --- a/azure/services/securitygroups/spec.go +++ b/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 diff --git a/azure/services/securitygroups/spec_test.go b/azure/services/securitygroups/spec_test.go index 4cdd067536b..ff08fc0ed74 100644 --- a/azure/services/securitygroups/spec_test.go +++ b/azure/services/securitygroups/spec_test.go @@ -119,6 +119,54 @@ func TestParameters(t *testing.T) { }, }, }, + expect: func(g *WithT, result interface{}) { + g.Expect(result).To(BeAssignableToTypeOf(network.SecurityGroup{})) + g.Expect(result).To(Equal(network.SecurityGroup{ + Location: pointer.String("test-location"), + Etag: pointer.String("fake-etag"), + SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{ + SecurityRules: &[]network.SecurityRule{ + converters.SecurityRuleToSDK(otherRule), + converters.SecurityRuleToSDK(sshRule), + converters.SecurityRuleToSDK(customRule), + }, + }, + Tags: map[string]*string{ + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": pointer.String("owned"), + "Name": pointer.String("test-nsg"), + }, + })) + }, + }, + { + name: "NSG already exists and a rule is deleted", + spec: &NSGSpec{ + Name: "test-nsg", + Location: "test-location", + SecurityRules: infrav1.SecurityRules{ + sshRule, + customRule, + }, + ResourceGroup: "test-group", + ClusterName: "my-cluster", + LastAppliedSecurityRules: map[string]interface{}{ + "allow_ssh": sshRule, + "custom_rule": customRule, + "other_rule": otherRule, + }, + }, + existing: network.SecurityGroup{ + Name: pointer.String("test-nsg"), + Location: pointer.String("test-location"), + Etag: pointer.String("fake-etag"), + SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{ + SecurityRules: &[]network.SecurityRule{ + converters.SecurityRuleToSDK(sshRule), + converters.SecurityRuleToSDK(customRule), + converters.SecurityRuleToSDK(otherRule), + }, + }, + }, expect: func(g *WithT, result interface{}) { g.Expect(result).To(BeAssignableToTypeOf(network.SecurityGroup{})) g.Expect(result).To(Equal(network.SecurityGroup{ @@ -128,7 +176,6 @@ func TestParameters(t *testing.T) { SecurityRules: &[]network.SecurityRule{ converters.SecurityRuleToSDK(sshRule), converters.SecurityRuleToSDK(customRule), - converters.SecurityRuleToSDK(otherRule), }, }, Tags: map[string]*string{ @@ -138,6 +185,38 @@ func TestParameters(t *testing.T) { })) }, }, + { + name: "NSG already exists and a rule not owned by CAPZ is present", + spec: &NSGSpec{ + Name: "test-nsg", + Location: "test-location", + SecurityRules: infrav1.SecurityRules{ + sshRule, + customRule, + }, + ResourceGroup: "test-group", + ClusterName: "my-cluster", + LastAppliedSecurityRules: map[string]interface{}{ + "allow_ssh": sshRule, + "custom_rule": customRule, + }, + }, + existing: network.SecurityGroup{ + Name: pointer.String("test-nsg"), + Location: pointer.String("test-location"), + Etag: pointer.String("fake-etag"), + SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{ + SecurityRules: &[]network.SecurityRule{ + converters.SecurityRuleToSDK(sshRule), + converters.SecurityRuleToSDK(customRule), + converters.SecurityRuleToSDK(otherRule), + }, + }, + }, + expect: func(g *WithT, result interface{}) { + g.Expect(result).To(BeNil()) + }, + }, { name: "NSG does not exist", spec: &NSGSpec{