Skip to content

Commit 63e35d8

Browse files
willie-yaok8s-infra-cherrypick-robot
willie-yao
authored and
k8s-infra-cherrypick-robot
committed
Delete security rules if removed from spec
1 parent a6e57e3 commit 63e35d8

File tree

11 files changed

+447
-57
lines changed

11 files changed

+447
-57
lines changed

‎azure/const.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,12 @@ const (
3535
// for annotation formatting rules.
3636
ManagedClusterTagsLastAppliedAnnotation = "sigs.k8s.io/cluster-api-provider-azure-last-applied-tags-managedcluster"
3737

38+
// SecurityRuleLastAppliedAnnotation is the key for the Azure Cluster
39+
// object annotation which tracks the security rules for security groups.
40+
// See https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/
41+
// for annotation formatting rules.
42+
SecurityRuleLastAppliedAnnotation = "sigs.k8s.io/cluster-api-provider-azure-last-applied-security-rules"
43+
3844
// CustomDataHashAnnotation is the key for the machine object annotation
3945
// which tracks the hash of the custom data.
4046
// See https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/

‎azure/scope/cluster.go‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -351,12 +351,13 @@ func (s *ClusterScope) NSGSpecs() []azure.ResourceSpecGetter {
351351
nsgspecs := make([]azure.ResourceSpecGetter, len(s.AzureCluster.Spec.NetworkSpec.Subnets))
352352
for i, subnet := range s.AzureCluster.Spec.NetworkSpec.Subnets {
353353
nsgspecs[i] = &securitygroups.NSGSpec{
354-
Name: subnet.SecurityGroup.Name,
355-
SecurityRules: subnet.SecurityGroup.SecurityRules,
356-
ResourceGroup: s.ResourceGroup(),
357-
Location: s.Location(),
358-
ClusterName: s.ClusterName(),
359-
AdditionalTags: s.AdditionalTags(),
354+
Name: subnet.SecurityGroup.Name,
355+
SecurityRules: subnet.SecurityGroup.SecurityRules,
356+
ResourceGroup: s.ResourceGroup(),
357+
Location: s.Location(),
358+
ClusterName: s.ClusterName(),
359+
AdditionalTags: s.AdditionalTags(),
360+
LastAppliedSecurityRules: s.getLastAppliedSecurityRules(subnet.SecurityGroup.Name),
360361
}
361362
}
362363

@@ -1092,3 +1093,18 @@ func (s *ClusterScope) getPrivateEndpoints(subnet infrav1.SubnetSpec) []azure.Re
10921093

10931094
return privateEndpointSpecs
10941095
}
1096+
1097+
func (s *ClusterScope) getLastAppliedSecurityRules(nsgName string) map[string]interface{} {
1098+
// Retrieve the last applied security rules for all NSGs.
1099+
lastAppliedSecurityRulesAll, err := s.AnnotationJSON(azure.SecurityRuleLastAppliedAnnotation)
1100+
if err != nil {
1101+
return map[string]interface{}{}
1102+
}
1103+
1104+
// Retrieve the last applied security rules for this NSG.
1105+
lastAppliedSecurityRules, ok := lastAppliedSecurityRulesAll[nsgName].(map[string]interface{})
1106+
if !ok {
1107+
lastAppliedSecurityRules = map[string]interface{}{}
1108+
}
1109+
return lastAppliedSecurityRules
1110+
}

‎azure/scope/cluster_test.go‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1171,10 +1171,11 @@ func TestNSGSpecs(t *testing.T) {
11711171
Name: "fake-rule-1",
11721172
},
11731173
},
1174-
ResourceGroup: "my-rg",
1175-
Location: "centralIndia",
1176-
ClusterName: "my-cluster",
1177-
AdditionalTags: make(infrav1.Tags),
1174+
ResourceGroup: "my-rg",
1175+
Location: "centralIndia",
1176+
ClusterName: "my-cluster",
1177+
AdditionalTags: make(infrav1.Tags),
1178+
LastAppliedSecurityRules: map[string]interface{}{},
11781179
},
11791180
},
11801181
},

‎azure/services/securitygroups/mock_securitygroups/securitygroups_mock.go‎

Lines changed: 14 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎azure/services/securitygroups/securitygroups.go‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ type NSGScope interface {
3535
azure.AsyncStatusUpdater
3636
NSGSpecs() []azure.ResourceSpecGetter
3737
IsVnetManaged() bool
38+
UpdateAnnotationJSON(string, map[string]interface{}) error
3839
}
3940

4041
// Service provides operations on Azure resources.
@@ -80,15 +81,32 @@ func (s *Service) Reconcile(ctx context.Context) error {
8081

8182
var resErr error
8283

84+
newAnnotation := make(map[string]interface{})
85+
8386
// We go through the list of security groups to reconcile each one, independently of the result of the previous one.
8487
// If multiple errors occur, we return the most pressing one.
8588
// 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)
86-
for _, nsgSpec := range specs {
89+
for _, resourceSpec := range specs {
90+
nsgSpec := resourceSpec.(*NSGSpec)
91+
currentAnnotation := make(map[string]string)
92+
8793
if _, err := s.CreateOrUpdateResource(ctx, nsgSpec, serviceName); err != nil {
8894
if !azure.IsOperationNotDoneError(err) || resErr == nil {
8995
resErr = err
9096
}
9197
}
98+
99+
for _, rule := range nsgSpec.SecurityRules {
100+
currentAnnotation[rule.Name] = rule.Description
101+
}
102+
103+
if len(currentAnnotation) > 0 {
104+
newAnnotation[nsgSpec.Name] = currentAnnotation
105+
}
106+
}
107+
108+
if err := s.Scope.UpdateAnnotationJSON(azure.SecurityRuleLastAppliedAnnotation, newAnnotation); err != nil {
109+
return err
92110
}
93111

94112
s.Scope.UpdatePutStatus(infrav1.SecurityGroupsReadyCondition, serviceName, resErr)

‎azure/services/securitygroups/securitygroups_test.go‎

Lines changed: 75 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -33,43 +33,55 @@ import (
3333
)
3434

3535
var (
36-
fakeNSG = NSGSpec{
36+
annotation = azure.SecurityRuleLastAppliedAnnotation
37+
fakeNSG = NSGSpec{
3738
Name: "test-nsg",
3839
Location: "test-location",
3940
ClusterName: "my-cluster",
4041
SecurityRules: infrav1.SecurityRules{
41-
{
42-
Name: "allow_ssh",
43-
Description: "Allow SSH",
44-
Priority: 2200,
45-
Protocol: infrav1.SecurityGroupProtocolTCP,
46-
Direction: infrav1.SecurityRuleDirectionInbound,
47-
Source: pointer.String("*"),
48-
SourcePorts: pointer.String("*"),
49-
Destination: pointer.String("*"),
50-
DestinationPorts: pointer.String("22"),
51-
},
52-
{
53-
Name: "other_rule",
54-
Description: "Test Rule",
55-
Priority: 500,
56-
Protocol: infrav1.SecurityGroupProtocolTCP,
57-
Direction: infrav1.SecurityRuleDirectionInbound,
58-
Source: pointer.String("*"),
59-
SourcePorts: pointer.String("*"),
60-
Destination: pointer.String("*"),
61-
DestinationPorts: pointer.String("80"),
62-
},
42+
securityRule1,
6343
},
6444
ResourceGroup: "test-group",
6545
}
66-
fakeNSG2 = NSGSpec{
46+
noRulesNSG = NSGSpec{
6747
Name: "test-nsg-2",
6848
Location: "test-location",
6949
ClusterName: "my-cluster",
7050
SecurityRules: infrav1.SecurityRules{},
7151
ResourceGroup: "test-group",
7252
}
53+
multipleRulesNSG = NSGSpec{
54+
Name: "multiple-rules-nsg",
55+
Location: "test-location",
56+
ClusterName: "my-cluster",
57+
SecurityRules: infrav1.SecurityRules{
58+
securityRule1,
59+
securityRule2,
60+
},
61+
ResourceGroup: "test-group",
62+
}
63+
securityRule1 = infrav1.SecurityRule{
64+
Name: "allow_ssh",
65+
Description: "Allow SSH",
66+
Priority: 2200,
67+
Protocol: infrav1.SecurityGroupProtocolTCP,
68+
Direction: infrav1.SecurityRuleDirectionInbound,
69+
Source: pointer.String("*"),
70+
SourcePorts: pointer.String("*"),
71+
Destination: pointer.String("*"),
72+
DestinationPorts: pointer.String("22"),
73+
}
74+
securityRule2 = infrav1.SecurityRule{
75+
Name: "other_rule",
76+
Description: "Test Rule",
77+
Priority: 500,
78+
Protocol: infrav1.SecurityGroupProtocolTCP,
79+
Direction: infrav1.SecurityRuleDirectionInbound,
80+
Source: pointer.String("*"),
81+
SourcePorts: pointer.String("*"),
82+
Destination: pointer.String("*"),
83+
DestinationPorts: pointer.String("80"),
84+
}
7385
errFake = errors.New("this is an error")
7486
notDoneError = azure.NewOperationNotDoneError(&infrav1.Future{})
7587
)
@@ -81,13 +93,36 @@ func TestReconcileSecurityGroups(t *testing.T) {
8193
expect func(s *mock_securitygroups.MockNSGScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder)
8294
}{
8395
{
84-
name: "create multiple security groups succeeds, should return no error",
96+
name: "create single security group with single rule succeeds, should return no error",
97+
expectedError: "",
98+
expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
99+
s.IsVnetManaged().Return(true)
100+
s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG})
101+
s.UpdateAnnotationJSON(annotation, map[string]interface{}{fakeNSG.Name: map[string]string{securityRule1.Name: securityRule1.Description}}).Times(1)
102+
r.CreateOrUpdateResource(gomockinternal.AContext(), &fakeNSG, serviceName).Return(nil, nil)
103+
s.UpdatePutStatus(infrav1.SecurityGroupsReadyCondition, serviceName, nil)
104+
},
105+
},
106+
{
107+
name: "create single security group with multiple rules succeeds, should return no error",
108+
expectedError: "",
109+
expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
110+
s.IsVnetManaged().Return(true)
111+
s.NSGSpecs().Return([]azure.ResourceSpecGetter{&multipleRulesNSG})
112+
s.UpdateAnnotationJSON(annotation, map[string]interface{}{multipleRulesNSG.Name: map[string]string{securityRule1.Name: securityRule1.Description, securityRule2.Name: securityRule2.Description}}).Times(1)
113+
r.CreateOrUpdateResource(gomockinternal.AContext(), &multipleRulesNSG, serviceName).Return(nil, nil)
114+
s.UpdatePutStatus(infrav1.SecurityGroupsReadyCondition, serviceName, nil)
115+
},
116+
},
117+
{
118+
name: "create multiple security groups, should return no error",
85119
expectedError: "",
86120
expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
87121
s.IsVnetManaged().Return(true)
88-
s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &fakeNSG2})
122+
s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &noRulesNSG})
123+
s.UpdateAnnotationJSON(annotation, map[string]interface{}{fakeNSG.Name: map[string]string{securityRule1.Name: securityRule1.Description}}).Times(1)
89124
r.CreateOrUpdateResource(gomockinternal.AContext(), &fakeNSG, serviceName).Return(nil, nil)
90-
r.CreateOrUpdateResource(gomockinternal.AContext(), &fakeNSG2, serviceName).Return(nil, nil)
125+
r.CreateOrUpdateResource(gomockinternal.AContext(), &noRulesNSG, serviceName).Return(nil, nil)
91126
s.UpdatePutStatus(infrav1.SecurityGroupsReadyCondition, serviceName, nil)
92127
},
93128
},
@@ -96,9 +131,10 @@ func TestReconcileSecurityGroups(t *testing.T) {
96131
expectedError: errFake.Error(),
97132
expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
98133
s.IsVnetManaged().Return(true)
99-
s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &fakeNSG2})
134+
s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &noRulesNSG})
135+
s.UpdateAnnotationJSON(annotation, map[string]interface{}{fakeNSG.Name: map[string]string{securityRule1.Name: securityRule1.Description}}).Times(1)
100136
r.CreateOrUpdateResource(gomockinternal.AContext(), &fakeNSG, serviceName).Return(nil, errFake)
101-
r.CreateOrUpdateResource(gomockinternal.AContext(), &fakeNSG2, serviceName).Return(nil, nil)
137+
r.CreateOrUpdateResource(gomockinternal.AContext(), &noRulesNSG, serviceName).Return(nil, nil)
102138
s.UpdatePutStatus(infrav1.SecurityGroupsReadyCondition, serviceName, errFake)
103139
},
104140
},
@@ -107,9 +143,10 @@ func TestReconcileSecurityGroups(t *testing.T) {
107143
expectedError: errFake.Error(),
108144
expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
109145
s.IsVnetManaged().Return(true)
110-
s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &fakeNSG2})
146+
s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &noRulesNSG})
147+
s.UpdateAnnotationJSON(annotation, map[string]interface{}{fakeNSG.Name: map[string]string{securityRule1.Name: securityRule1.Description}}).Times(1)
111148
r.CreateOrUpdateResource(gomockinternal.AContext(), &fakeNSG, serviceName).Return(nil, errFake)
112-
r.CreateOrUpdateResource(gomockinternal.AContext(), &fakeNSG2, serviceName).Return(nil, notDoneError)
149+
r.CreateOrUpdateResource(gomockinternal.AContext(), &noRulesNSG, serviceName).Return(nil, notDoneError)
113150
s.UpdatePutStatus(infrav1.SecurityGroupsReadyCondition, serviceName, errFake)
114151
},
115152
},
@@ -119,6 +156,7 @@ func TestReconcileSecurityGroups(t *testing.T) {
119156
expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
120157
s.IsVnetManaged().Return(true)
121158
s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG})
159+
s.UpdateAnnotationJSON(annotation, map[string]interface{}{fakeNSG.Name: map[string]string{securityRule1.Name: securityRule1.Description}})
122160
r.CreateOrUpdateResource(gomockinternal.AContext(), &fakeNSG, serviceName).Return(nil, notDoneError)
123161
s.UpdatePutStatus(infrav1.SecurityGroupsReadyCondition, serviceName, notDoneError)
124162
},
@@ -171,9 +209,9 @@ func TestDeleteSecurityGroups(t *testing.T) {
171209
expectedError: "",
172210
expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
173211
s.IsVnetManaged().Return(true)
174-
s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &fakeNSG2})
212+
s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &noRulesNSG})
175213
r.DeleteResource(gomockinternal.AContext(), &fakeNSG, serviceName).Return(nil)
176-
r.DeleteResource(gomockinternal.AContext(), &fakeNSG2, serviceName).Return(nil)
214+
r.DeleteResource(gomockinternal.AContext(), &noRulesNSG, serviceName).Return(nil)
177215
s.UpdateDeleteStatus(infrav1.SecurityGroupsReadyCondition, serviceName, nil)
178216
},
179217
},
@@ -182,9 +220,9 @@ func TestDeleteSecurityGroups(t *testing.T) {
182220
expectedError: errFake.Error(),
183221
expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
184222
s.IsVnetManaged().Return(true)
185-
s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &fakeNSG2})
223+
s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &noRulesNSG})
186224
r.DeleteResource(gomockinternal.AContext(), &fakeNSG, serviceName).Return(errFake)
187-
r.DeleteResource(gomockinternal.AContext(), &fakeNSG2, serviceName).Return(nil)
225+
r.DeleteResource(gomockinternal.AContext(), &noRulesNSG, serviceName).Return(nil)
188226
s.UpdateDeleteStatus(infrav1.SecurityGroupsReadyCondition, serviceName, errFake)
189227
},
190228
},
@@ -193,9 +231,9 @@ func TestDeleteSecurityGroups(t *testing.T) {
193231
expectedError: errFake.Error(),
194232
expect: func(s *mock_securitygroups.MockNSGScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
195233
s.IsVnetManaged().Return(true)
196-
s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &fakeNSG2})
234+
s.NSGSpecs().Return([]azure.ResourceSpecGetter{&fakeNSG, &noRulesNSG})
197235
r.DeleteResource(gomockinternal.AContext(), &fakeNSG, serviceName).Return(errFake)
198-
r.DeleteResource(gomockinternal.AContext(), &fakeNSG2, serviceName).Return(notDoneError)
236+
r.DeleteResource(gomockinternal.AContext(), &noRulesNSG, serviceName).Return(notDoneError)
199237
s.UpdateDeleteStatus(infrav1.SecurityGroupsReadyCondition, serviceName, errFake)
200238
},
201239
},

‎azure/services/securitygroups/spec.go‎

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -29,12 +29,13 @@ import (
2929

3030
// NSGSpec defines the specification for a security group.
3131
type NSGSpec struct {
32-
Name string
33-
SecurityRules infrav1.SecurityRules
34-
Location string
35-
ClusterName string
36-
ResourceGroup string
37-
AdditionalTags infrav1.Tags
32+
Name string
33+
SecurityRules infrav1.SecurityRules
34+
Location string
35+
ClusterName string
36+
ResourceGroup string
37+
AdditionalTags infrav1.Tags
38+
LastAppliedSecurityRules map[string]interface{}
3839
}
3940

4041
// ResourceName returns the name of the security group.
@@ -55,6 +56,7 @@ func (s *NSGSpec) OwnerResourceName() string {
5556
// Parameters returns the parameters for the security group.
5657
func (s *NSGSpec) Parameters(ctx context.Context, existing interface{}) (interface{}, error) {
5758
securityRules := make([]network.SecurityRule, 0)
59+
newAnnotation := map[string]string{}
5860
var etag *string
5961

6062
if existing != nil {
@@ -67,14 +69,29 @@ func (s *NSGSpec) Parameters(ctx context.Context, existing interface{}) (interfa
6769
etag = existingNSG.Etag
6870
// Check if the expected rules are present
6971
update := false
70-
securityRules = *existingNSG.SecurityRules
72+
7173
for _, rule := range s.SecurityRules {
7274
sdkRule := converters.SecurityRuleToSDK(rule)
73-
if !ruleExists(securityRules, sdkRule) {
75+
if !ruleExists(*existingNSG.SecurityRules, sdkRule) {
7476
update = true
7577
securityRules = append(securityRules, sdkRule)
7678
}
79+
newAnnotation[rule.Name] = rule.Description
7780
}
81+
82+
for _, oldRule := range *existingNSG.SecurityRules {
83+
_, tracked := s.LastAppliedSecurityRules[*oldRule.Name]
84+
// If rule is owned by CAPZ and applied last, and not found in the new rules, then it has been deleted
85+
if _, ok := newAnnotation[*oldRule.Name]; !ok && tracked {
86+
// Rule has been deleted
87+
update = true
88+
continue
89+
}
90+
91+
// Add previous rules that haven't been deleted
92+
securityRules = append(securityRules, oldRule)
93+
}
94+
7895
if !update {
7996
// Skip update for NSG as the required default rules are present
8097
return nil, nil

0 commit comments

Comments
 (0)