Skip to content

Commit

Permalink
Fix NSX rule deleted not saved in store (#722)
Browse files Browse the repository at this point in the history 
This patch is to the fix the issue that NSX rules are not deleted from
the store even the rules have been deleted from NSX.
  • Loading branch information
@timdengyun
timdengyun authored Sep 4, 2024
1 parent e3898b6 commit 72baaea
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 26 deletions.
1 change: 1 addition & 0 deletions pkg/nsx/services/securitypolicy/expand.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ func (service *SecurityPolicyService) expandRule(obj *v1alpha1.SecurityPolicy, r
return nil, nil, err
}
nsxRules = append(nsxRules, nsxRule)
return nsxGroups, nsxRules, nil
}

// Check if there is a namedport in the rule
Expand Down
44 changes: 23 additions & 21 deletions pkg/nsx/services/securitypolicy/firewall.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -439,8 +439,9 @@ func (service *SecurityPolicyService) createOrUpdateSecurityPolicy(obj *v1alpha1
return err
}

// WrapHighLevelSecurityPolicy will modify the input security policy, so we need to make a copy for the following store update.
finalSecurityPolicyCopy := *finalSecurityPolicy
// WrapHierarchyVpcSecurityPolicy will modify the input security policy rules and move the rules to Children fields for HAPI wrap,
// so we need to make a copy for the rules store update.
finalRules := finalSecurityPolicy.Rules

if !isChanged && len(finalSecurityPolicy.Rules) == 0 && len(finalGroups) == 0 {
log.Info("securityPolicy, rules, groups are not changed, skip updating them", "nsxSecurityPolicyId", finalSecurityPolicy.Id)
Expand All @@ -459,7 +460,7 @@ func (service *SecurityPolicyService) createOrUpdateSecurityPolicy(obj *v1alpha1
return err
}
// Get SecurityPolicy from NSX after HAPI call as NSX renders several fields like `path`/`parent_path`.
finalSecurityPolicyCopy, err = service.NSXClient.SecurityClient.Get(getDomain(service), *finalSecurityPolicy.Id)
finalGetNSXSecurityPolicy, err := service.NSXClient.SecurityClient.Get(getDomain(service), *finalSecurityPolicy.Id)
err = nsxutil.NSXApiError(err)
if err != nil {
log.Error(err, "failed to get SecurityPolicy", "nsxSecurityPolicyId", finalSecurityPolicy.Id)
Expand All @@ -470,23 +471,23 @@ func (service *SecurityPolicyService) createOrUpdateSecurityPolicy(obj *v1alpha1
// The steps below know how to deal with NSX resources, if there is MarkedForDelete, then delete it from store,
// otherwise add or update it to store.
if isChanged {
err = securityPolicyStore.Apply(&finalSecurityPolicyCopy)
err = securityPolicyStore.Apply(&finalGetNSXSecurityPolicy)
if err != nil {
log.Error(err, "failed to apply store", "securityPolicy", finalSecurityPolicyCopy)
log.Error(err, "failed to apply store", "securityPolicy", finalGetNSXSecurityPolicy)
return err
}
}
err = ruleStore.Apply(&finalSecurityPolicyCopy)
err = ruleStore.Apply(&finalRules)
if err != nil {
log.Error(err, "failed to apply store", "nsxRules", finalSecurityPolicyCopy.Rules)
log.Error(err, "failed to apply store", "nsxRules", finalRules)
return err
}
err = groupStore.Apply(&finalGroups)
if err != nil {
log.Error(err, "failed to apply store", "nsxGroups", finalGroups)
return err
}
log.Info("successfully created or updated NSX SecurityPolicy", "nsxSecurityPolicy", finalSecurityPolicyCopy)
log.Info("successfully created or updated NSX SecurityPolicy", "nsxSecurityPolicy", finalGetNSXSecurityPolicy)
return nil
}

Expand All @@ -508,8 +509,9 @@ func (service *SecurityPolicyService) createOrUpdateVPCSecurityPolicy(obj *v1alp
indexScope = common.TagScopeNetworkPolicyUID
}

// WrapHierarchyVpcSecurityPolicy will modify the input security policy, so we need to make a copy for the following store update.
finalSecurityPolicyCopy := *finalSecurityPolicy
// WrapHierarchyVpcSecurityPolicy will modify the input security policy rules and move the rules to Children fields for HAPI wrap,
// so we need to make a copy for the rules store update.
finalRules := finalSecurityPolicy.Rules

nsxProjectGroups := make([]model.Group, 0)
nsxProjectShares := make([]model.Share, 0)
Expand Down Expand Up @@ -542,7 +544,7 @@ func (service *SecurityPolicyService) createOrUpdateVPCSecurityPolicy(obj *v1alp
}

// 2.Wrap SecurityPolicy, groups, rules under VPC level together with project groups and shares into one hierarchy resource tree.
orgRoot, err := service.WrapHierarchyVpcSecurityPolicy(&finalSecurityPolicyCopy, finalGroups, projectInfra, vpcInfo)
orgRoot, err := service.WrapHierarchyVpcSecurityPolicy(finalSecurityPolicy, finalGroups, projectInfra, vpcInfo)
if err != nil {
log.Error(err, "failed to wrap SecurityPolicy in VPC", "nsxSecurityPolicyId", finalSecurityPolicy.Id)
return err
Expand All @@ -557,7 +559,7 @@ func (service *SecurityPolicyService) createOrUpdateVPCSecurityPolicy(obj *v1alp
}

// Get SecurityPolicy from NSX after HAPI call as NSX renders several fields like `path`/`parent_path`.
finalSecurityPolicyCopy, err = service.NSXClient.VPCSecurityClient.Get(vpcInfo.OrgID, vpcInfo.ProjectID, vpcInfo.VPCID, *finalSecurityPolicyCopy.Id)
finalGetNSXSecurityPolicy, err := service.NSXClient.VPCSecurityClient.Get(vpcInfo.OrgID, vpcInfo.ProjectID, vpcInfo.VPCID, *finalSecurityPolicy.Id)
err = nsxutil.NSXApiError(err)
if err != nil {
log.Error(err, "failed to get SecurityPolicy in VPC", "nsxSecurityPolicyId", finalSecurityPolicy.Id)
Expand All @@ -568,16 +570,16 @@ func (service *SecurityPolicyService) createOrUpdateVPCSecurityPolicy(obj *v1alp
// The steps below know how to deal with NSX resources, if there is MarkedForDelete, then delete it from store,
// otherwise add or update it to store.
if isChanged {
err = securityPolicyStore.Apply(&finalSecurityPolicyCopy)
err = securityPolicyStore.Apply(&finalGetNSXSecurityPolicy)
if err != nil {
log.Error(err, "failed to apply store", "securityPolicy", finalSecurityPolicyCopy)
log.Error(err, "failed to apply store", "securityPolicy", finalGetNSXSecurityPolicy)
return err
}
}

err = ruleStore.Apply(&finalSecurityPolicyCopy)
err = ruleStore.Apply(&finalRules)
if err != nil {
log.Error(err, "failed to apply store", "nsxRules", finalSecurityPolicyCopy.Rules)
log.Error(err, "failed to apply store", "nsxRules", finalRules)
return err
}
err = groupStore.Apply(&finalGroups)
Expand All @@ -596,7 +598,7 @@ func (service *SecurityPolicyService) createOrUpdateVPCSecurityPolicy(obj *v1alp
return err
}

log.Info("successfully created or updated NSX SecurityPolicy in VPC", "nsxSecurityPolicy", finalSecurityPolicyCopy)
log.Info("successfully created or updated NSX SecurityPolicy in VPC", "nsxSecurityPolicy", finalGetNSXSecurityPolicy)
return nil
}

Expand Down Expand Up @@ -679,7 +681,7 @@ func (service *SecurityPolicyService) deleteSecurityPolicy(sp types.UID) error {
log.Error(err, "failed to apply store", "securityPolicy", finalSecurityPolicyCopy)
return err
}
err = ruleStore.Apply(&finalSecurityPolicyCopy)
err = ruleStore.Apply(&finalSecurityPolicyCopy.Rules)
if err != nil {
log.Error(err, "failed to apply store", "nsxRules", finalSecurityPolicyCopy.Rules)
return err
Expand Down Expand Up @@ -782,7 +784,7 @@ func (service *SecurityPolicyService) deleteVPCSecurityPolicy(sp types.UID, crea
log.Error(err, "failed to apply store", "securityPolicy", finalSecurityPolicyCopy)
return err
}
err = ruleStore.Apply(&finalSecurityPolicyCopy)
err = ruleStore.Apply(&finalSecurityPolicyCopy.Rules)
if err != nil {
log.Error(err, "failed to apply store", "nsxRules", finalSecurityPolicyCopy.Rules)
return err
Expand Down Expand Up @@ -972,8 +974,8 @@ func (service *SecurityPolicyService) markDeleteShares(existingShares []*model.S
}
}

func (s *SecurityPolicyService) getVpcInfo(spNameSpace string) (*common.VPCResourceInfo, error) {
VPCInfo := s.vpcService.ListVPCInfo(spNameSpace)
func (service *SecurityPolicyService) getVpcInfo(spNameSpace string) (*common.VPCResourceInfo, error) {
VPCInfo := service.vpcService.ListVPCInfo(spNameSpace)
if len(VPCInfo) == 0 {
errorMsg := fmt.Sprintf("there is no VPC info found for namespace %s", spNameSpace)
err := errors.New(errorMsg)
Expand Down
2 changes: 1 addition & 1 deletion pkg/nsx/services/securitypolicy/firewall_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -860,7 +860,7 @@ func TestDleleteVPCSecurityPolicy(t *testing.T) {
fakeService.setUpStore(common.TagValueScopeSecurityPolicyUID)

assert.NoError(t, fakeService.securityPolicyStore.Apply(tt.inputPolicy))
assert.NoError(t, fakeService.ruleStore.Apply(tt.inputPolicy))
assert.NoError(t, fakeService.ruleStore.Apply(&tt.inputPolicy.Rules))

patches := tt.prepareFunc(t, fakeService)
defer patches.Reset()
Expand Down
6 changes: 3 additions & 3 deletions pkg/nsx/services/securitypolicy/store.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,8 @@ func (securityPolicyStore *SecurityPolicyStore) GetByIndex(key string, value str
}

func (ruleStore *RuleStore) Apply(i interface{}) error {
sp := i.(*model.SecurityPolicy)
for _, rule := range sp.Rules {
rules := i.(*[]model.Rule)
for _, rule := range *rules {
tempRule := rule
if rule.MarkedForDelete != nil && *rule.MarkedForDelete {
err := ruleStore.Delete(&tempRule)
Expand Down Expand Up @@ -217,7 +217,7 @@ func (shareStore *ShareStore) Apply(i interface{}) error {
}
} else {
err := shareStore.Add(&tempShare)
log.V(1).Info("add share to store", "group", tempShare)
log.V(1).Info("add share to store", "share", tempShare)
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/nsx/services/securitypolicy/store_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ func TestRuleStore_Apply(t *testing.T) {
args args
wantErr assert.ErrorAssertionFunc
}{
{"1", args{i: &sp}, assert.NoError},
{"1", args{i: &sp.Rules}, assert.NoError},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down

0 comments on commit 72baaea

Please sign in to comment.