Skip to content

Commit

Permalink
Remove rule index from rule/group ID/Name and unify name for VPC and …
Browse files Browse the repository at this point in the history 
…T1 (#785)

This patch is to:
1. Remove SecurityPolicy rule index from rule ID and for VPC mode,
and keep T1 mode rule ID unchanged with rule index.
2. Remove SecurityPolicy rule index from group ID for VPC mode,
and keep T1 mode group ID unchanged with rule index.
3. Remove rule index from NSX group/rule name, and unify the NSX resource name for VPC and T1 network,
including SecurityPolicy, rule, and group.
4. Reduce length of rule hash string to 8 chars for VPC mode.
  • Loading branch information
@timdengyun
timdengyun authored Oct 9, 2024
1 parent e389624 commit 0445ed3
Show file tree
Hide file tree
Showing 7 changed files with 493 additions and 166 deletions.
197 changes: 117 additions & 80 deletions pkg/nsx/services/securitypolicy/builder.go
View file

Large diffs are not rendered by default.

401 changes: 339 additions & 62 deletions pkg/nsx/services/securitypolicy/builder_test.go
View file

Large diffs are not rendered by default.

8 changes: 4 additions & 4 deletions pkg/nsx/services/securitypolicy/expand.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func (service *SecurityPolicyService) expandRule(obj *v1alpha1.SecurityPolicy, r
var nsxGroups []*model.Group

if len(rule.Ports) == 0 {
nsxRule, err := service.buildRuleBasicInfo(obj, rule, ruleIdx, 0, 0, -1, false, createdFor)
nsxRule, err := service.buildRuleBasicInfo(obj, rule, ruleIdx, 0, 0, false, -1, createdFor)
if err != nil {
return nil, nil, err
}
Expand All @@ -39,7 +39,7 @@ func (service *SecurityPolicyService) expandRule(obj *v1alpha1.SecurityPolicy, r
// Check if there is a namedport in the rule
hasNamedPort := service.hasNamedPort(rule)
if !hasNamedPort {
nsxRule, err := service.buildRuleBasicInfo(obj, rule, ruleIdx, 0, 0, -1, false, createdFor)
nsxRule, err := service.buildRuleBasicInfo(obj, rule, ruleIdx, 0, 0, false, -1, createdFor)
if err != nil {
return nil, nil, err
}
Expand Down Expand Up @@ -88,7 +88,7 @@ func (service *SecurityPolicyService) expandRuleByPort(obj *v1alpha1.SecurityPol
if err != nil {
// In case there is no more valid ip set selected, so clear the stale ip set group in NSX if stale ips exist
if errors.As(err, &nsxutil.NoEffectiveOption{}) {
groups := service.groupStore.GetByIndex(common.TagScopeRuleID, service.buildRuleID(obj, rule, ruleIdx, createdFor))
groups := service.groupStore.GetByIndex(common.TagScopeRuleID, service.buildRuleID(obj, ruleIdx, createdFor))
var ipSetGroup *model.Group
for _, group := range groups {
ipSetGroup = group
Expand Down Expand Up @@ -121,7 +121,7 @@ func (service *SecurityPolicyService) expandRuleByService(obj *v1alpha1.Security
) ([]*model.Group, *model.Rule, error) {
var nsxGroups []*model.Group

nsxRule, err := service.buildRuleBasicInfo(obj, rule, ruleIdx, portIdx, portAddressIdx, portAddress.Port, true, createdFor)
nsxRule, err := service.buildRuleBasicInfo(obj, rule, ruleIdx, portIdx, portAddressIdx, true, portAddress.Port, createdFor)
if err != nil {
return nil, nil, err
}
Expand Down
20 changes: 17 additions & 3 deletions pkg/nsx/services/securitypolicy/expand_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,23 @@ import (
func TestSecurityPolicyService_buildRuleIPGroup(t *testing.T) {
sp := &v1alpha1.SecurityPolicy{
ObjectMeta: v1.ObjectMeta{Namespace: "ns1", Name: "spA", UID: "uidA"},
Spec: v1alpha1.SecurityPolicySpec{
Rules: []v1alpha1.SecurityPolicyRule{
{
Action: &allowAction,
Direction: &directionIn,
Sources: []v1alpha1.SecurityPolicyPeer{
{
PodSelector: &v1.LabelSelector{
MatchLabels: map[string]string{"pod_selector_1": "pod_value_1"},
},
},
},
},
},
},
}

rule := v1alpha1.SecurityPolicyRule{}
nsxRule := model.Rule{
DisplayName: &ruleNameWithPodSelector00,
Id: &ruleIDPort000,
Expand Down Expand Up @@ -66,7 +80,7 @@ func TestSecurityPolicyService_buildRuleIPGroup(t *testing.T) {
DisplayName: &policyGroupName,
Expression: []*data.StructValue{blockExpression},
// build ipset group tags from input securitypolicy and securitypolicy rule
Tags: service.buildPeerTags(sp, &rule, 0, false, false, false, common.ResourceTypeSecurityPolicy),
Tags: service.buildPeerTags(sp, &sp.Spec.Rules[0], 0, false, false, false, common.ResourceTypeSecurityPolicy),
}

type args struct {
Expand All @@ -82,7 +96,7 @@ func TestSecurityPolicyService_buildRuleIPGroup(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert.Equalf(t, tt.want, service.buildRuleIPSetGroup(sp, &rule, tt.args.obj, tt.args.ips, 0, common.ResourceTypeSecurityPolicy), "buildRuleIPSetGroup(%v, %v)",
assert.Equalf(t, tt.want, service.buildRuleIPSetGroup(sp, &sp.Spec.Rules[0], tt.args.obj, tt.args.ips, 0, common.ResourceTypeSecurityPolicy), "buildRuleIPSetGroup(%v, %v)",
tt.args.obj, tt.args.ips)
})
}
Expand Down
18 changes: 9 additions & 9 deletions pkg/nsx/services/securitypolicy/firewall_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ var (
tagScopeSecurityPolicyUID = common.TagScopeSecurityPolicyUID
tagScopeRuleID = common.TagScopeRuleID
tagScopeSelectorHash = common.TagScopeSelectorHash
spName = "sp_ns1_spA"
spGroupName = "ns1_spA_scope"
spName = "spA"
spGroupName = "spA_scope"
spID = "sp_uidA"
spID2 = "sp_uidB"
spGroupID = "sp_uidA_scope"
Expand Down Expand Up @@ -1073,11 +1073,11 @@ func TestCreateOrUpdateSecurityPolicy(t *testing.T) {
mockVPCService := common.MockVPCServiceProvider{}
fakeService.vpcService = &mockVPCService

podSelectorRule0IDPort000 := fakeService.buildExpandedRuleId(fakeService.buildRuleID(&spWithPodSelector, &spWithPodSelector.Spec.Rules[0], 0, common.ResourceTypeSecurityPolicy), 0, 0)
podSelectorRule1IDPort000 := fakeService.buildExpandedRuleId(fakeService.buildRuleID(&spWithPodSelector, &spWithPodSelector.Spec.Rules[1], 1, common.ResourceTypeSecurityPolicy), 0, 0)
podSelectorRule0IDPort000 := fakeService.buildExpandedRuleID(&spWithPodSelector, 0, 0, 0, false, -1, common.ResourceTypeSecurityPolicy)
podSelectorRule1IDPort000 := fakeService.buildExpandedRuleID(&spWithPodSelector, 1, 0, 0, false, -1, common.ResourceTypeSecurityPolicy)

podSelectorRule0Name00, _ := fakeService.buildRuleDisplayName(&spWithPodSelector.Spec.Rules[0], 0, -1, false, common.ResourceTypeSecurityPolicy)
podSelectorRule1Name00, _ := fakeService.buildRuleDisplayName(&spWithPodSelector.Spec.Rules[1], 0, -1, false, common.ResourceTypeSecurityPolicy)
podSelectorRule0Name00, _ := fakeService.buildRuleDisplayName(&spWithPodSelector.Spec.Rules[0], 0, false, -1, common.ResourceTypeSecurityPolicy)
podSelectorRule1Name00, _ := fakeService.buildRuleDisplayName(&spWithPodSelector.Spec.Rules[1], 0, false, -1, common.ResourceTypeSecurityPolicy)

type args struct {
spObj *v1alpha1.SecurityPolicy
Expand Down Expand Up @@ -1494,13 +1494,13 @@ func TestGetFinalSecurityPolicyResouceFromNetworkPolicy(t *testing.T) {
Rules: []model.Rule{
{
DisplayName: common.String("TCP.6001_ingress_allow"),
Id: common.String("np-app-access_uidNP_allow_0_6c2a026ca143812daa72699fb924ee36b33b5cdc_0_0"),
Id: common.String("np-app-access_uidNP_allow_6c2a026c_6001"),
DestinationGroups: []string{"ANY"},
Direction: &nsxRuleDirectionIn,
Scope: []string{"ANY"},
SequenceNumber: &seq0,
Services: []string{"ANY"},
SourceGroups: []string{"/orgs/default/projects/projectQuality/infra/domains/default/groups/np-app-access_uidNP_allow_0_src"},
SourceGroups: []string{"/orgs/default/projects/projectQuality/infra/domains/default/groups/np-app-access_uidNP_allow_6c2a026c_src"},
Action: &nsxRuleActionAllow,
ServiceEntries: []*data.StructValue{serviceEntry},
Tags: npAllowBasicTags,
Expand All @@ -1516,7 +1516,7 @@ func TestGetFinalSecurityPolicyResouceFromNetworkPolicy(t *testing.T) {
Rules: []model.Rule{
{
DisplayName: common.String("ingress_isolation"),
Id: common.String("np-app-access_uidNP_isolation_0_114fed106ef3b5eae2a583f312435e84c02ca97f_0_0"),
Id: common.String("np-app-access_uidNP_isolation_114fed10_all"),
DestinationGroups: []string{"ANY"},
Direction: &nsxRuleDirectionIn,
Scope: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/np-app-access_uidNP_isolation_scope"},
Expand Down
13 changes: 6 additions & 7 deletions pkg/util/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,9 @@ import (
)

const (
wcpSystemResource = "vmware-system-shared-t1"
HashLength int = 8
SubnetTypeSubnet = "subnet"
SubnetTypeSubnetSet = "subnetset"
wcpSystemResource = "vmware-system-shared-t1"
SubnetTypeSubnet = "subnet"
SubnetTypeSubnetSet = "subnetset"
)

var (
Expand Down Expand Up @@ -106,11 +105,11 @@ func NormalizeId(name string) string {
return newName
}
hashString := Sha1(name)
nameLength := common.MaxIdLength - HashLength - 1
nameLength := common.MaxIdLength - common.HashLength - 1
for strings.ContainsAny(string(newName[nameLength-1]), "-._") {
nameLength--
}
newName = fmt.Sprintf("%s-%s", newName[:nameLength], hashString[:HashLength])
newName = fmt.Sprintf("%s-%s", newName[:nameLength], hashString[:common.HashLength])
return newName
}

Expand Down Expand Up @@ -521,7 +520,7 @@ func Capitalize(s string) string {

func GetRandomIndexString() string {
uuidStr := uuid.NewString()
return Sha1(uuidStr)[:HashLength]
return Sha1(uuidStr)[:common.HashLength]
}

// IsPowerOfTwo checks if a given number is a power of 2
Expand Down
2 changes: 1 addition & 1 deletion pkg/util/utils_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func TestNormalizeName(t *testing.T) {
shortName := strings.Repeat("a", 256)
assert.Equal(t, NormalizeName(shortName), shortName)
longName := strings.Repeat("a", 257)
assert.Equal(t, NormalizeName(longName), fmt.Sprintf("%s_%s", strings.Repeat("a", 256-HashLength-1), "0c103888"))
assert.Equal(t, NormalizeName(longName), fmt.Sprintf("%s_%s", strings.Repeat("a", 256-common.HashLength-1), "0c103888"))
}

func TestNormalizeLabelKey(t *testing.T) {
Expand Down

0 comments on commit 0445ed3

Please sign in to comment.