From 06cfe9175f6125450a352f58b9becae8178fba55 Mon Sep 17 00:00:00 2001 From: Deng Yun Date: Sun, 29 Sep 2024 03:29:48 +0000 Subject: [PATCH] Remove rule index from rule/group ID/Name and unify name for VPC and T1 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. --- pkg/nsx/services/securitypolicy/builder.go | 197 +++++---- .../services/securitypolicy/builder_test.go | 401 +++++++++++++++--- pkg/nsx/services/securitypolicy/expand.go | 8 +- .../services/securitypolicy/expand_test.go | 20 +- .../services/securitypolicy/firewall_test.go | 18 +- pkg/util/utils.go | 13 +- pkg/util/utils_test.go | 2 +- 7 files changed, 493 insertions(+), 166 deletions(-) diff --git a/pkg/nsx/services/securitypolicy/builder.go b/pkg/nsx/services/securitypolicy/builder.go index b1a31e45e..6a7bfb56a 100644 --- a/pkg/nsx/services/securitypolicy/builder.go +++ b/pkg/nsx/services/securitypolicy/builder.go @@ -5,7 +5,6 @@ import ( "encoding/json" "errors" "fmt" - "sort" "strconv" "strings" @@ -14,6 +13,7 @@ import ( corev1 "k8s.io/api/core/v1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/sets" "github.com/vmware-tanzu/nsx-operator/pkg/apis/legacy/v1alpha1" @@ -39,24 +39,15 @@ var ( Int64 = common.Int64 ) -func (service *SecurityPolicyService) buildSecurityPolicyName(obj *v1alpha1.SecurityPolicy, createdFor string) string { - if IsVPCEnabled(service) { - // For VPC scenario, we use obj.Name as the NSX resource display name for both SecurityPolicy and NetworkPolicy. - return util.GenerateTruncName(common.MaxNameLength, obj.Name, "", "", "", "") - } - prefix := common.SecurityPolicyPrefix - if createdFor != common.ResourceTypeSecurityPolicy { - prefix = common.NetworkPolicyPrefix - } - // For T1 scenario, we use ns-name as the key resource name for SecurityPolicy, it is to be consistent with the - // previous solutions. - return util.GenerateTruncName(common.MaxNameLength, strings.Join([]string{obj.Namespace, obj.Name}, common.ConnectorUnderline), prefix, "", "", "") +func (service *SecurityPolicyService) buildSecurityPolicyName(obj *v1alpha1.SecurityPolicy) string { + return util.GenerateTruncName(common.MaxNameLength, obj.Name, "", "", "", "") } func (service *SecurityPolicyService) buildSecurityPolicyID(obj *v1alpha1.SecurityPolicy, createdFor string) string { if IsVPCEnabled(service) { return util.GenerateIDByObject(obj) } + prefix := common.SecurityPolicyPrefix if createdFor != common.ResourceTypeSecurityPolicy { prefix = common.NetworkPolicyPrefix @@ -76,7 +67,7 @@ func (service *SecurityPolicyService) buildSecurityPolicy(obj *v1alpha1.Security nsxSecurityPolicy := &model.SecurityPolicy{} nsxSecurityPolicy.Id = String(service.buildSecurityPolicyID(obj, createdFor)) - nsxSecurityPolicy.DisplayName = String(service.buildSecurityPolicyName(obj, createdFor)) + nsxSecurityPolicy.DisplayName = String(service.buildSecurityPolicyName(obj)) // TODO: confirm the sequence number: offset nsxSecurityPolicy.SequenceNumber = Int64(int64(obj.Spec.Priority)) @@ -93,7 +84,7 @@ func (service *SecurityPolicyService) buildSecurityPolicy(obj *v1alpha1.Security currentSet := sets.Set[string]{} for ruleIdx, r := range obj.Spec.Rules { rule := r - // A rule containing named port may expand to multiple rules if the name maps to multiple port numbers. + // A rule containing named port may be expanded to multiple rules if the named ports map to multiple port numbers. expandRules, buildGroups, buildGroupShares, err := service.buildRuleAndGroups(obj, &rule, ruleIdx, createdFor) if err != nil { log.Error(err, "failed to build rule and groups", "rule", rule, "ruleIndex", ruleIdx) @@ -204,11 +195,6 @@ func (service *SecurityPolicyService) buildTargetTags(obj *v1alpha1.SecurityPoli rule *v1alpha1.SecurityPolicyRule, ruleIdx int, createdFor string, ) []model.Tag { basicTags := service.buildBasicTags(obj, createdFor) - sort.Slice(*targets, func(i, j int) bool { - k1, _ := json.Marshal((*targets)[i]) - k2, _ := json.Marshal((*targets)[j]) - return string(k1) < string(k2) - }) serializedBytes, _ := json.Marshal(*targets) targetTags := []model.Tag{ { @@ -242,7 +228,7 @@ func (service *SecurityPolicyService) buildTargetTags(obj *v1alpha1.SecurityPoli targetTags = append(targetTags, model.Tag{ Scope: String(common.TagScopeRuleID), - Tag: String(service.buildRuleID(obj, rule, ruleIdx, createdFor)), + Tag: String(service.buildRuleID(obj, ruleIdx, createdFor)), }, ) } @@ -379,7 +365,8 @@ func (service *SecurityPolicyService) buildAppliedGroupID(obj *v1alpha1.Security if IsVPCEnabled(service) { suffix := common.TargetGroupSuffix if ruleIdx != -1 { - suffix = strings.Join([]string{strconv.Itoa(ruleIdx), suffix}, common.ConnectorUnderline) + ruleHash := service.buildLimitedRuleHashString(&(obj.Spec.Rules[ruleIdx])) + suffix = strings.Join([]string{ruleHash, suffix}, common.ConnectorUnderline) } return util.GenerateIDByObjectWithSuffix(obj, suffix) } @@ -415,17 +402,13 @@ func (service *SecurityPolicyService) buildAppliedGroupPath(obj *v1alpha1.Securi // build appliedTo group display name for both policy and rule levels. func (service *SecurityPolicyService) buildAppliedGroupName(obj *v1alpha1.SecurityPolicy, ruleIdx int) string { - var rule *v1alpha1.SecurityPolicyRule if ruleIdx != -1 { - rule = &(obj.Spec.Rules[ruleIdx]) - ruleName := strings.Join([]string{obj.Name, strconv.Itoa(ruleIdx)}, common.ConnectorUnderline) - if len(rule.Name) > 0 { - ruleName = rule.Name - } - return util.GenerateTruncName(common.MaxNameLength, ruleName, "", common.TargetGroupSuffix, "", "") + ruleHash := service.buildLimitedRuleHashString(&(obj.Spec.Rules[ruleIdx])) + suffix := strings.Join([]string{ruleHash, common.TargetGroupSuffix}, common.ConnectorUnderline) + return util.GenerateTruncName(common.MaxNameLength, obj.Name, "", suffix, "", "") } - ruleName := strings.Join([]string{obj.Namespace, obj.Name}, common.ConnectorUnderline) - return util.GenerateTruncName(common.MaxNameLength, ruleName, "", common.TargetGroupSuffix, "", "") + + return util.GenerateTruncName(common.MaxNameLength, obj.Name, "", common.TargetGroupSuffix, "", "") } func (service *SecurityPolicyService) buildRuleAndGroups(obj *v1alpha1.SecurityPolicy, rule *v1alpha1.SecurityPolicyRule, @@ -598,22 +581,44 @@ func (service *SecurityPolicyService) buildRuleOutGroup(obj *v1alpha1.SecurityPo return nsxRuleDstGroup, nsxRuleSrcGroupPath, nsxRuleDstGroupPath, nsxGroupShare, nil } -func (service *SecurityPolicyService) buildRuleID(obj *v1alpha1.SecurityPolicy, rule *v1alpha1.SecurityPolicyRule, ruleIdx int, createdFor string) string { - serializedBytes, _ := json.Marshal(rule) - ruleHash := fmt.Sprintf("%s", util.Sha1(string(serializedBytes))) - ruleIdxStr := fmt.Sprintf("%d", ruleIdx) +func (service *SecurityPolicyService) buildRuleID(obj *v1alpha1.SecurityPolicy, ruleIdx int, createdFor string) string { + ruleIndexHash := service.buildRuleHashString(&(obj.Spec.Rules[ruleIdx])) + if IsVPCEnabled(service) { - suffix := strings.Join([]string{ruleIdxStr, ruleHash}, common.ConnectorUnderline) - return util.GenerateIDByObjectWithSuffix(obj, suffix) + ruleIndexHash = service.buildLimitedRuleHashString(&(obj.Spec.Rules[ruleIdx])) + return util.GenerateIDByObjectWithSuffix(obj, ruleIndexHash) } + prefix := common.SecurityPolicyPrefix if createdFor == common.ResourceTypeNetworkPolicy { prefix = common.NetworkPolicyPrefix } - return util.GenerateID(fmt.Sprintf("%s", obj.UID), prefix, ruleHash, ruleIdxStr) + ruleIdxStr := fmt.Sprintf("%d", ruleIdx) + return strings.Join([]string{prefix, string(obj.UID), ruleIndexHash, ruleIdxStr}, common.ConnectorUnderline) } -func (service *SecurityPolicyService) buildRuleDisplayName(rule *v1alpha1.SecurityPolicyRule, portIdx, portNumber int, hasNamedport bool, createdFor string) (string, error) { +// A rule containing named port may be expanded to multiple NSX rules if the name ports map to multiple port numbers. +// So, in VPC network, the rule port numbers, which either are defined in rule Port or resolved from named port, will be appended as CR rule baseID to distinguish them. +// For T1, the portIdx and portAddressIdx are appended as suffix. +func (service *SecurityPolicyService) buildExpandedRuleID(obj *v1alpha1.SecurityPolicy, ruleIdx int, portIdx, portAddressIdx int, + hasNamedport bool, portNumber int, createdFor string, +) string { + ruleBaseID := service.buildRuleID(obj, ruleIdx, createdFor) + + if IsVPCEnabled(service) { + portNumberSuffix := "" + if !hasNamedport { + portNumberSuffix = service.buildRulePortsNumberString(obj.Spec.Rules[ruleIdx].Ports) + } else { + portNumberSuffix = service.buildRulePortNumberString(obj.Spec.Rules[ruleIdx].Ports[portIdx], portNumber) + } + return strings.Join([]string{ruleBaseID, portNumberSuffix}, common.ConnectorUnderline) + } + + return strings.Join([]string{ruleBaseID, strconv.Itoa(portIdx), strconv.Itoa(portAddressIdx)}, common.ConnectorUnderline) +} + +func (service *SecurityPolicyService) buildRuleDisplayName(rule *v1alpha1.SecurityPolicyRule, portIdx int, hasNamedport bool, portNumber int, createdFor string) (string, error) { var ruleName string var ruleAct string @@ -651,7 +656,7 @@ func (service *SecurityPolicyService) buildRuleDisplayName(rule *v1alpha1.Securi ruleName = strings.Join([]string{ruleName, suffix}, common.ConnectorUnderline) } } else { - ruleName = service.buildRulePortsString(&rule.Ports, suffix) + ruleName = service.buildRulePortsString(rule.Ports, suffix) } if !hasNamedport { @@ -659,11 +664,11 @@ func (service *SecurityPolicyService) buildRuleDisplayName(rule *v1alpha1.Securi } else { // For the security policy rule with namedPort, it will be expanded to the multiple security policy rules based on resolution of named port. // e.g. input: security policy's rule name: TCP.http_UDP.1234_ingress_allow, - // expand to NSX security policy rules with name TCP.http_UDP.1234_TCP.80_ingress_allow and TCP.http_UDP.1234_UDP.1234_ingress_allow. + // expand to NSX security policy rules with name TCP.http_UDP.1234.TCP.80_ingress_allow and TCP.http_UDP.1234.UDP.1234_ingress_allow. // in case that user defined input security policy's rule name: sp_namedport_rule, // expand to NSX security policy rules with name sp_namedport_rule.TCP.80_ingress_allow and sp_namedport_rule.UDP.1234_ingress_allow. index := strings.Index(ruleName, common.ConnectorUnderline+suffix) - return util.GenerateTruncName(common.MaxNameLength, ruleName[:index]+"."+service.buildRulePortString(&rule.Ports[portIdx], true, portNumber), "", suffix, "", ""), nil + return util.GenerateTruncName(common.MaxNameLength, ruleName[:index]+"."+service.buildRulePortString(rule.Ports[portIdx], portNumber), "", suffix, "", ""), nil } } @@ -747,24 +752,25 @@ func (service *SecurityPolicyService) buildRulePeerGroupID(obj *v1alpha1.Securit if isSource == true { suffix = common.SrcGroupSuffix } + if IsVPCEnabled(service) { - suffix = strings.Join([]string{strconv.Itoa(ruleIdx), suffix}, common.ConnectorUnderline) + ruleHash := service.buildLimitedRuleHashString(&(obj.Spec.Rules[ruleIdx])) + suffix = strings.Join([]string{ruleHash, suffix}, common.ConnectorUnderline) return util.GenerateIDByObjectWithSuffix(obj, suffix) } + return util.GenerateID(string(obj.UID), common.SecurityPolicyPrefix, suffix, strconv.Itoa(ruleIdx)) } func (service *SecurityPolicyService) buildRulePeerGroupName(obj *v1alpha1.SecurityPolicy, ruleIdx int, isSource bool) string { - rule := &(obj.Spec.Rules[ruleIdx]) suffix := common.DstGroupSuffix if isSource == true { suffix = common.SrcGroupSuffix } - ruleName := strings.Join([]string{obj.Name, strconv.Itoa(ruleIdx)}, common.ConnectorUnderline) - if len(rule.Name) > 0 { - ruleName = rule.Name - } - return util.GenerateTruncName(common.MaxNameLength, ruleName, "", suffix, "", "") + ruleHash := service.buildLimitedRuleHashString(&(obj.Spec.Rules[ruleIdx])) + suffix = strings.Join([]string{ruleHash, suffix}, common.ConnectorUnderline) + + return util.GenerateTruncName(common.MaxNameLength, obj.Name, "", suffix, "", "") } func (service *SecurityPolicyService) buildRulePeerGroupPath(obj *v1alpha1.SecurityPolicy, ruleIdx int, isSource, infraGroupShared, projectGroupShared bool, vpcInfo *common.VPCResourceInfo) (string, error) { @@ -913,15 +919,11 @@ func (service *SecurityPolicyService) buildRulePeerGroup(obj *v1alpha1.SecurityP return &rulePeerGroup, rulePeerGroupPath, nil, err } -func (service *SecurityPolicyService) buildExpandedRuleId(ruleBaseId string, portIdx int, portAddressIdx int) string { - return strings.Join([]string{ruleBaseId, strconv.Itoa(portIdx), strconv.Itoa(portAddressIdx)}, common.ConnectorUnderline) -} - // Build rule basic info, ruleIdx is the index of the rules of security policy, // portIdx is the index of rule's ports, portAddressIdx is the index // of multiple port number if one named port maps to multiple port numbers. func (service *SecurityPolicyService) buildRuleBasicInfo(obj *v1alpha1.SecurityPolicy, rule *v1alpha1.SecurityPolicyRule, ruleIdx int, portIdx int, portAddressIdx int, - portNumber int, hasNamedport bool, createdFor string, + hasNamedport bool, portNumber int, createdFor string, ) (*model.Rule, error) { ruleAction, err := getRuleAction(rule) if err != nil { @@ -931,13 +933,13 @@ func (service *SecurityPolicyService) buildRuleBasicInfo(obj *v1alpha1.SecurityP if err != nil { return nil, err } - displayName, err := service.buildRuleDisplayName(rule, portIdx, portNumber, hasNamedport, createdFor) + displayName, err := service.buildRuleDisplayName(rule, portIdx, hasNamedport, portNumber, createdFor) if err != nil { log.Error(err, "failed to build rule's display name", "securityPolicyUID", obj.UID, "rule", rule, "createdFor", createdFor) } nsxRule := model.Rule{ - Id: String(service.buildExpandedRuleId(service.buildRuleID(obj, rule, ruleIdx, createdFor), portIdx, portAddressIdx)), + Id: String(service.buildExpandedRuleID(obj, ruleIdx, portIdx, portAddressIdx, hasNamedport, portNumber, createdFor)), DisplayName: &displayName, Direction: &ruleDirection, SequenceNumber: Int64(int64(ruleIdx)), @@ -957,13 +959,6 @@ func (service *SecurityPolicyService) buildPeerTags(obj *v1alpha1.SecurityPolicy groupTypeTag = String(common.TagValueGroupSource) peers = &rule.Sources } - - // TODO: abstract sort func for both peers and targets - sort.Slice(*peers, func(i, j int) bool { - k1, _ := json.Marshal((*peers)[i]) - k2, _ := json.Marshal((*peers)[j]) - return string(k1) < string(k2) - }) serializedBytes, _ := json.Marshal(*peers) peerTags := []model.Tag{ @@ -973,7 +968,7 @@ func (service *SecurityPolicyService) buildPeerTags(obj *v1alpha1.SecurityPolicy }, { Scope: String(common.TagScopeRuleID), - Tag: String(service.buildRuleID(obj, rule, ruleIdx, createdFor)), + Tag: String(service.buildRuleID(obj, ruleIdx, createdFor)), }, { Scope: String(common.TagScopeSelectorHash), @@ -1009,6 +1004,7 @@ func (service *SecurityPolicyService) buildPeerTags(obj *v1alpha1.SecurityPolicy ) } } + return peerTags } @@ -1847,7 +1843,7 @@ func (service *SecurityPolicyService) getNamespaceUID(ns string) (nsUid types.UI return namespace_uid } -func (service *SecurityPolicyService) buildRulePortString(port *v1alpha1.SecurityPolicyPort, hasNamedport bool, portNumber int) string { +func (service *SecurityPolicyService) buildRulePortString(port v1alpha1.SecurityPolicyPort, portNumber int) string { protocol := string(port.Protocol) // Build the rule port string name for non named port. // This is a common case where the string is built from port definition. For instance, @@ -1858,36 +1854,77 @@ func (service *SecurityPolicyService) buildRulePortString(port *v1alpha1.Securit // - protocol: UDP // port: 3308 // The built port string is: UDP.3308 - if !hasNamedport { - if port.EndPort != 0 { - return fmt.Sprintf("%s.%s.%d", protocol, (port.Port).String(), port.EndPort) - } - return fmt.Sprintf("%s.%s", protocol, (port.Port).String()) - } else { + if port.Port.Type == intstr.String && portNumber > 0 { // Build the rule port string name for named port. // The port string is built from specific port number resolved from named port. return fmt.Sprintf("%s.%d", protocol, portNumber) } + if port.EndPort != 0 { + return fmt.Sprintf("%s.%s.%d", protocol, (port.Port).String(), port.EndPort) + } + return fmt.Sprintf("%s.%s", protocol, (port.Port).String()) } -func (service *SecurityPolicyService) buildRulePortsString(ports *[]v1alpha1.SecurityPolicyPort, suffix string) string { +func (service *SecurityPolicyService) buildRulePortsString(ports []v1alpha1.SecurityPolicyPort, suffix string) string { portsString := "" - if ports == nil || len(*ports) == 0 { + if ports == nil || len(ports) == 0 { portsString = common.RuleAnyPorts } else { - for idx, p := range *ports { + portStrings := make([]string, len(ports)) + for idx, p := range ports { port := p - portString := service.buildRulePortString(&port, false, -1) - if idx == 0 { - portsString = portString - } else { - portsString = strings.Join([]string{portsString, portString}, common.ConnectorUnderline) - } + portStrings[idx] = service.buildRulePortString(port, -1) } + portsString = strings.Join(portStrings, common.ConnectorUnderline) } + return util.GenerateTruncName(common.MaxNameLength, portsString, "", suffix, "", "") } +func (service *SecurityPolicyService) buildRulePortNumberString(port v1alpha1.SecurityPolicyPort, portNumber int) string { + // Build the rule port number string name for non named port. + // This is a common case where the string is built from port definition. For instance, + // - protocol: TCP + // port: 8282 + // endPort: 8286 + // The built port number string is: 8282.8286 + // - protocol: UDP + // port: 3308 + // The built port number string is: 3308 + if port.Port.Type == intstr.String && portNumber > 0 { + // Build the rule port number string name for named port. + // The port number string is built from specific port number resolved from named port. + return fmt.Sprintf("%d", portNumber) + } + if port.EndPort != 0 { + return fmt.Sprintf("%s.%d", (port.Port).String(), port.EndPort) + } + return (port.Port).String() +} + +func (service *SecurityPolicyService) buildRulePortsNumberString(ports []v1alpha1.SecurityPolicyPort) string { + if ports == nil || len(ports) == 0 { + return common.RuleAnyPorts + } + + portNumStrings := make([]string, len(ports)) + for idx, p := range ports { + port := p + portNumStrings[idx] = service.buildRulePortNumberString(port, -1) + } + return strings.Join(portNumStrings, common.ConnectorUnderline) +} + +func (service *SecurityPolicyService) buildLimitedRuleHashString(rule *v1alpha1.SecurityPolicyRule) string { + serializedBytes, _ := json.Marshal(rule) + return util.Sha1(string(serializedBytes))[:common.HashLength] +} + +func (service *SecurityPolicyService) buildRuleHashString(rule *v1alpha1.SecurityPolicyRule) string { + serializedBytes, _ := json.Marshal(rule) + return util.Sha1(string(serializedBytes)) +} + func (service *SecurityPolicyService) BuildNetworkPolicyAllowPolicyID(uid string) string { return strings.Join([]string{uid, common.RuleActionAllow}, common.ConnectorUnderline) } diff --git a/pkg/nsx/services/securitypolicy/builder_test.go b/pkg/nsx/services/securitypolicy/builder_test.go index 5fbca2def..aef63e991 100644 --- a/pkg/nsx/services/securitypolicy/builder_test.go +++ b/pkg/nsx/services/securitypolicy/builder_test.go @@ -35,19 +35,19 @@ func TestBuildSecurityPolicy(t *testing.T) { ) podSelectorRule0Name00 := "rule-with-pod-ns-selector_ingress_allow" - podSelectorRule0IDPort000 := "sp_uidA_0_2c822e90b1377b346014adfa583f08a99dee52a8_0_0" + podSelectorRule0IDPort000 := "sp_uidA_2c822e90b1377b346014adfa583f08a99dee52a8_0_0_0" podSelectorRule1Name00 := "rule-with-ns-selector_ingress_allow" - podSelectorRule1IDPort000 := "sp_uidA_1_2a4595d0dd582c2ae5613245ad7b39de5ade2e20_0_0" + podSelectorRule1IDPort000 := "sp_uidA_2a4595d0dd582c2ae5613245ad7b39de5ade2e20_1_0_0" vmSelectorRule0Name00 := "rule-with-VM-selector_egress_isolation" - vmSelectorRule0IDPort000 := "sp_uidB_0_67410606c486d2ba38002ed076a2a4211c9d49b5_0_0" + vmSelectorRule0IDPort000 := "sp_uidB_67410606c486d2ba38002ed076a2a4211c9d49b5_0_0_0" vmSelectorRule1Name00 := "rule-with-ns-selector_egress_isolation" - vmSelectorRule1IDPort000 := "sp_uidB_1_7d721f087be35f0bf318f4847b5acdc3d2b91446_0_0" + vmSelectorRule1IDPort000 := "sp_uidB_7d721f087be35f0bf318f4847b5acdc3d2b91446_1_0_0" vmSelectorRule2Name00 := "all_egress_isolation" - vmSelectorRule2IDPort000 := "sp_uidB_2_a40c813916cc397fcd2260e48cc773d4c9b08565_0_0" + vmSelectorRule2IDPort000 := "sp_uidB_a40c813916cc397fcd2260e48cc773d4c9b08565_2_0_0" tests := []struct { name string @@ -96,7 +96,7 @@ func TestBuildSecurityPolicy(t *testing.T) { name: "security-policy-with-VM-selector For T1", inputPolicy: &spWithVMSelector, expectedPolicy: &model.SecurityPolicy{ - DisplayName: common.String("sp_ns1_spB"), + DisplayName: common.String("spB"), Id: common.String("sp_uidB"), Scope: []string{"/infra/domains/k8scl-one/groups/sp_uidB_scope"}, SequenceNumber: &seq0, @@ -201,19 +201,19 @@ func TestBuildSecurityPolicyForVPC(t *testing.T) { defer patches.Reset() podSelectorRule0Name00 := "rule-with-pod-ns-selector_ingress_allow" - podSelectorRule0IDPort000 := "spA_uidA_0_2c822e90b1377b346014adfa583f08a99dee52a8_0_0" + podSelectorRule0IDPort000 := "spA_uidA_2c822e90_all" podSelectorRule1Name00 := "rule-with-ns-selector_ingress_allow" - podSelectorRule1IDPort000 := "spA_uidA_1_2a4595d0dd582c2ae5613245ad7b39de5ade2e20_0_0" + podSelectorRule1IDPort000 := "spA_uidA_2a4595d0_53" vmSelectorRule0Name00 := "rule-with-VM-selector_egress_isolation" - vmSelectorRule0IDPort000 := "spB_uidB_0_67410606c486d2ba38002ed076a2a4211c9d49b5_0_0" + vmSelectorRule0IDPort000 := "spB_uidB_67410606_all" vmSelectorRule1Name00 := "rule-with-ns-selector_egress_isolation" - vmSelectorRule1IDPort000 := "spB_uidB_1_7d721f087be35f0bf318f4847b5acdc3d2b91446_0_0" + vmSelectorRule1IDPort000 := "spB_uidB_7d721f08_all" vmSelectorRule2Name00 := "all_egress_isolation" - vmSelectorRule2IDPort000 := "spB_uidB_2_a40c813916cc397fcd2260e48cc773d4c9b08565_0_0" + vmSelectorRule2IDPort000 := "spB_uidB_a40c8139_all" tests := []struct { name string @@ -234,10 +234,10 @@ func TestBuildSecurityPolicyForVPC(t *testing.T) { Id: &podSelectorRule0IDPort000, DestinationGroups: []string{"ANY"}, Direction: &nsxRuleDirectionIn, - Scope: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/spA_uidA_0_scope"}, + Scope: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/spA_uidA_2c822e90_scope"}, SequenceNumber: &seq0, Services: []string{"ANY"}, - SourceGroups: []string{"/orgs/default/projects/projectQuality/infra/domains/default/groups/spA_uidA_0_src"}, + SourceGroups: []string{"/orgs/default/projects/projectQuality/infra/domains/default/groups/spA_uidA_2c822e90_src"}, Action: &nsxRuleActionAllow, Tags: vpcBasicTags, }, @@ -249,7 +249,7 @@ func TestBuildSecurityPolicyForVPC(t *testing.T) { Scope: []string{"ANY"}, SequenceNumber: &seq1, Services: []string{"ANY"}, - SourceGroups: []string{"/orgs/default/projects/projectQuality/infra/domains/default/groups/spA_uidA_1_src"}, + SourceGroups: []string{"/orgs/default/projects/projectQuality/infra/domains/default/groups/spA_uidA_2a4595d0_src"}, Action: &nsxRuleActionAllow, ServiceEntries: []*data.StructValue{serviceEntry}, Tags: vpcBasicTags, @@ -270,9 +270,9 @@ func TestBuildSecurityPolicyForVPC(t *testing.T) { { DisplayName: &vmSelectorRule0Name00, Id: &vmSelectorRule0IDPort000, - DestinationGroups: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/spB_uidB_0_dst"}, + DestinationGroups: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/spB_uidB_67410606_dst"}, Direction: &nsxRuleDirectionOut, - Scope: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/spB_uidB_0_scope"}, + Scope: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/spB_uidB_67410606_scope"}, SequenceNumber: &seq0, Services: []string{"ANY"}, SourceGroups: []string{"ANY"}, @@ -282,7 +282,7 @@ func TestBuildSecurityPolicyForVPC(t *testing.T) { { DisplayName: &vmSelectorRule1Name00, Id: &vmSelectorRule1IDPort000, - DestinationGroups: []string{"/orgs/default/projects/projectQuality/infra/domains/default/groups/spB_uidB_1_dst"}, + DestinationGroups: []string{"/orgs/default/projects/projectQuality/infra/domains/default/groups/spB_uidB_7d721f08_dst"}, Direction: &nsxRuleDirectionOut, Scope: []string{"ANY"}, SequenceNumber: &seq1, @@ -295,7 +295,7 @@ func TestBuildSecurityPolicyForVPC(t *testing.T) { { DisplayName: &vmSelectorRule2Name00, Id: &vmSelectorRule2IDPort000, - DestinationGroups: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/spB_uidB_2_dst"}, + DestinationGroups: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/spB_uidB_a40c8139_dst"}, Direction: &nsxRuleDirectionOut, Scope: []string{"ANY"}, SequenceNumber: &seq2, @@ -354,7 +354,7 @@ func TestBuildTargetTags(t *testing.T) { common.TagValueScopeSecurityPolicyName = common.TagScopeSecurityPolicyCRName common.TagValueScopeSecurityPolicyUID = common.TagScopeSecurityPolicyCRUID - ruleTagID0 := service.buildRuleID(&spWithPodSelector, &spWithPodSelector.Spec.Rules[0], 0, common.ResourceTypeSecurityPolicy) + ruleTagID0 := service.buildRuleID(&spWithPodSelector, 0, common.ResourceTypeSecurityPolicy) tests := []struct { name string inputPolicy *v1alpha1.SecurityPolicy @@ -437,7 +437,7 @@ func TestBuildTargetTags(t *testing.T) { } func TestBuildPeerTags(t *testing.T) { - ruleTagID0 := service.buildRuleID(&spWithPodSelector, &spWithPodSelector.Spec.Rules[0], 0, common.ResourceTypeSecurityPolicy) + ruleTagID0 := service.buildRuleID(&spWithPodSelector, 0, common.ResourceTypeSecurityPolicy) tests := []struct { name string inputPolicy *v1alpha1.SecurityPolicy @@ -825,10 +825,7 @@ func TestUpdateMixedExpressionsMatchExpression(t *testing.T) { } var securityPolicyWithMultipleNormalPorts = v1alpha1.SecurityPolicy{ - ObjectMeta: v1.ObjectMeta{ - Namespace: "null", - Name: "null", - }, + ObjectMeta: v1.ObjectMeta{Namespace: "ns1", Name: "spMulPorts", UID: "spMulPortsuidA"}, Spec: v1alpha1.SecurityPolicySpec{ Rules: []v1alpha1.SecurityPolicyRule{ { @@ -862,25 +859,37 @@ var securityPolicyWithMultipleNormalPorts = v1alpha1.SecurityPolicy{ }, }, }, + { + Action: &allowAction, + Direction: &directionOut, + Ports: []v1alpha1.SecurityPolicyPort{ + { + Protocol: "TCP", + Port: intstr.IntOrString{Type: intstr.Int, IntVal: 80}, + }, + { + Protocol: "UDP", + Port: intstr.IntOrString{Type: intstr.Int, IntVal: 1234}, + EndPort: 1234, + }, + }, + }, }, }, } var securityPolicyWithOneNamedPort = v1alpha1.SecurityPolicy{ - ObjectMeta: v1.ObjectMeta{ - Namespace: "null", - Name: "null", - }, + ObjectMeta: v1.ObjectMeta{Namespace: "ns1", Name: "spNamedPorts", UID: "spNamedPortsuidA"}, Spec: v1alpha1.SecurityPolicySpec{ Rules: []v1alpha1.SecurityPolicyRule{ { - Name: "TCP.http_UDP.1234.1235_ingress_allow", + Name: "user-defined-rule-namedport", Action: &allowAction, Direction: &directionIn, Ports: []v1alpha1.SecurityPolicyPort{ { Protocol: "TCP", - Port: intstr.IntOrString{Type: intstr.String, StrVal: "http"}, + Port: intstr.IntOrString{Type: intstr.String, StrVal: "http"}, // http port is 80 }, { Protocol: "UDP", @@ -889,6 +898,45 @@ var securityPolicyWithOneNamedPort = v1alpha1.SecurityPolicy{ }, }, }, + { + Action: &allowAction, + Direction: &directionIn, + Ports: []v1alpha1.SecurityPolicyPort{ + { + Protocol: "TCP", + Port: intstr.IntOrString{Type: intstr.String, StrVal: "https"}, // http port is 443 + }, + { + Protocol: "UDP", + Port: intstr.IntOrString{Type: intstr.Int, IntVal: 1236}, + EndPort: 1237, + }, + }, + }, + { + Action: &allowAction, + Direction: &directionIn, + Ports: []v1alpha1.SecurityPolicyPort{ + { + Protocol: "TCP", + Port: intstr.IntOrString{Type: intstr.String, StrVal: "web"}, + }, + { + Protocol: "UDP", + Port: intstr.IntOrString{Type: intstr.Int, IntVal: 533}, + }, + }, + }, + { + Action: &allowAction, + Direction: &directionIn, + Ports: []v1alpha1.SecurityPolicyPort{ + { + Protocol: "TCP", + Port: intstr.IntOrString{Type: intstr.String, StrVal: "db"}, + }, + }, + }, }, }, } @@ -896,22 +944,52 @@ var securityPolicyWithOneNamedPort = v1alpha1.SecurityPolicy{ func TestBuildRulePortsString(t *testing.T) { tests := []struct { name string - inputPorts *[]v1alpha1.SecurityPolicyPort + inputPorts []v1alpha1.SecurityPolicyPort suffix string expectedRulePortsString string }{ { name: "build-string-for-multiple-ports-without-named-port", - inputPorts: &securityPolicyWithMultipleNormalPorts.Spec.Rules[0].Ports, + inputPorts: securityPolicyWithMultipleNormalPorts.Spec.Rules[0].Ports, suffix: "ingress_allow", expectedRulePortsString: "TCP.80_UDP.1234.1235_ingress_allow", }, { - name: "build-string-for-multiple-ports-without-one-named-port", - inputPorts: &securityPolicyWithOneNamedPort.Spec.Rules[0].Ports, + name: "build-string-for-multiple-ports-userdefinedrule-without-named-port", + inputPorts: securityPolicyWithMultipleNormalPorts.Spec.Rules[1].Ports, + suffix: "egress_drop", + expectedRulePortsString: "TCP.88_UDP.1236.1237_egress_drop", + }, + { + name: "build-string-for-multiple-ports-start-end-port-same-without-named-port", + inputPorts: securityPolicyWithMultipleNormalPorts.Spec.Rules[2].Ports, + suffix: "egress_allow", + expectedRulePortsString: "TCP.80_UDP.1234.1234_egress_allow", + }, + { + name: "build-string-for-multiple-ports-with-http-named-port", + inputPorts: securityPolicyWithOneNamedPort.Spec.Rules[0].Ports, suffix: "ingress_allow", expectedRulePortsString: "TCP.http_UDP.1234.1235_ingress_allow", }, + { + name: "build-string-for-multiple-ports-with-https-named-port", + inputPorts: securityPolicyWithOneNamedPort.Spec.Rules[1].Ports, + suffix: "ingress_allow", + expectedRulePortsString: "TCP.https_UDP.1236.1237_ingress_allow", + }, + { + name: "build-string-for-multiple-ports-with-web-named-port", + inputPorts: securityPolicyWithOneNamedPort.Spec.Rules[2].Ports, + suffix: "ingress_allow", + expectedRulePortsString: "TCP.web_UDP.533_ingress_allow", + }, + { + name: "build-string-for-multiple-ports-with-db-named-port", + inputPorts: securityPolicyWithOneNamedPort.Spec.Rules[3].Ports, + suffix: "ingress_allow", + expectedRulePortsString: "TCP.db_ingress_allow", + }, { name: "build-string-for-nil-ports", inputPorts: nil, @@ -927,6 +1005,61 @@ func TestBuildRulePortsString(t *testing.T) { } } +func TestBuildRulePortsNumberString(t *testing.T) { + tests := []struct { + name string + inputPorts []v1alpha1.SecurityPolicyPort + expectedRulePortsString string + }{ + { + name: "build-string-for-multiple-ports-without-named-port", + inputPorts: securityPolicyWithMultipleNormalPorts.Spec.Rules[0].Ports, + expectedRulePortsString: "80_1234.1235", + }, + { + name: "build-string-for-multiple-ports-userdefinedrule-without-named-port", + inputPorts: securityPolicyWithMultipleNormalPorts.Spec.Rules[1].Ports, + expectedRulePortsString: "88_1236.1237", + }, + { + name: "build-string-for-multiple-ports-start-end-port-same-without-named-port", + inputPorts: securityPolicyWithMultipleNormalPorts.Spec.Rules[2].Ports, + expectedRulePortsString: "80_1234.1234", + }, + { + name: "build-string-for-multiple-ports-with-http-named-port", + inputPorts: securityPolicyWithOneNamedPort.Spec.Rules[0].Ports, + expectedRulePortsString: "http_1234.1235", + }, + { + name: "build-string-for-multiple-ports-with-https-named-port", + inputPorts: securityPolicyWithOneNamedPort.Spec.Rules[1].Ports, + expectedRulePortsString: "https_1236.1237", + }, + { + name: "build-string-for-multiple-ports-with-web-named-port", + inputPorts: securityPolicyWithOneNamedPort.Spec.Rules[2].Ports, + expectedRulePortsString: "web_533", + }, + { + name: "build-string-for-multiple-ports-with-db-named-port", + inputPorts: securityPolicyWithOneNamedPort.Spec.Rules[3].Ports, + expectedRulePortsString: "db", + }, + { + name: "build-string-for-nil-ports", + inputPorts: nil, + expectedRulePortsString: "all", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + observedString := service.buildRulePortsNumberString(tt.inputPorts) + assert.Equal(t, tt.expectedRulePortsString, observedString) + }) + } +} + func TestBuildRuleDisplayName(t *testing.T) { tests := []struct { name string @@ -934,6 +1067,8 @@ func TestBuildRuleDisplayName(t *testing.T) { inputRule *v1alpha1.SecurityPolicyRule ruleIdx int portIdx int + hasNamedPort bool + portNumber int createdFor string expectedRuleDisplayName string }{ @@ -943,6 +1078,8 @@ func TestBuildRuleDisplayName(t *testing.T) { inputRule: &securityPolicyWithMultipleNormalPorts.Spec.Rules[0], ruleIdx: 0, portIdx: 0, + hasNamedPort: false, + portNumber: -1, createdFor: common.ResourceTypeNetworkPolicy, expectedRuleDisplayName: "TCP.80_UDP.1234.1235_ingress_allow", }, @@ -952,6 +1089,8 @@ func TestBuildRuleDisplayName(t *testing.T) { inputRule: &securityPolicyWithMultipleNormalPorts.Spec.Rules[1], ruleIdx: 1, portIdx: 0, + hasNamedPort: false, + portNumber: -1, createdFor: common.ResourceTypeNetworkPolicy, expectedRuleDisplayName: "MultipleNormalPorts-rule1", }, @@ -961,19 +1100,143 @@ func TestBuildRuleDisplayName(t *testing.T) { inputRule: &securityPolicyWithMultipleNormalPorts.Spec.Rules[1], ruleIdx: 1, portIdx: 0, + hasNamedPort: false, + portNumber: -1, createdFor: common.ResourceTypeSecurityPolicy, expectedRuleDisplayName: "MultipleNormalPorts-rule1_egress_isolation", }, + { + name: "build-display-name-for-user-defined-rulename-with-one-named-http-port", + inputSecurityPolicy: &securityPolicyWithOneNamedPort, + inputRule: &securityPolicyWithOneNamedPort.Spec.Rules[0], + ruleIdx: 0, + portIdx: 0, + hasNamedPort: true, + portNumber: 80, + createdFor: common.ResourceTypeSecurityPolicy, + expectedRuleDisplayName: "user-defined-rule-namedport.TCP.80_ingress_allow", + }, + { + name: "build-display-name-for-multiple-ports-with-one-named-https-port", + inputSecurityPolicy: &securityPolicyWithOneNamedPort, + inputRule: &securityPolicyWithOneNamedPort.Spec.Rules[1], + ruleIdx: 1, + portIdx: 0, + hasNamedPort: true, + portNumber: 443, + createdFor: common.ResourceTypeSecurityPolicy, + expectedRuleDisplayName: "TCP.https_UDP.1236.1237.TCP.443_ingress_allow", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - observedDisplayName, observedError := service.buildRuleDisplayName(tt.inputRule, tt.portIdx, -1, false, tt.createdFor) + observedDisplayName, observedError := service.buildRuleDisplayName(tt.inputRule, tt.portIdx, tt.hasNamedPort, tt.portNumber, tt.createdFor) assert.Equal(t, tt.expectedRuleDisplayName, observedDisplayName) assert.Equal(t, nil, observedError) }) } } +func TestBuildExpandedRuleID(t *testing.T) { + svc := &SecurityPolicyService{ + Service: common.Service{ + NSXConfig: &config.NSXOperatorConfig{ + CoeConfig: &config.CoeConfig{ + Cluster: "cluster1", + }, + }, + }, + } + + tests := []struct { + name string + vpcEnabled bool + inputSecurityPolicy *v1alpha1.SecurityPolicy + inputRule *v1alpha1.SecurityPolicyRule + ruleIdx int + portIdx int + portAddressIdx int + hasNamedPort bool + portNumber int + createdFor string + expectedRuleID string + }{ + { + name: "build-ruleID-for-multiple-ports-0-for-vpc", + vpcEnabled: true, + inputSecurityPolicy: &securityPolicyWithMultipleNormalPorts, + inputRule: &securityPolicyWithMultipleNormalPorts.Spec.Rules[0], + ruleIdx: 0, + portIdx: 0, + portAddressIdx: 0, + hasNamedPort: false, + portNumber: -1, + createdFor: common.ResourceTypeSecurityPolicy, + expectedRuleID: "spMulPorts_spMulPortsuidA_d0b8e36c_80_1234.1235", + }, + { + name: "build-ruleID-for-multiple-ports-0-for-T1", + vpcEnabled: false, + inputSecurityPolicy: &securityPolicyWithMultipleNormalPorts, + inputRule: &securityPolicyWithMultipleNormalPorts.Spec.Rules[0], + ruleIdx: 0, + portIdx: 0, + portAddressIdx: 0, + hasNamedPort: false, + portNumber: -1, + createdFor: common.ResourceTypeSecurityPolicy, + expectedRuleID: "sp_spMulPortsuidA_d0b8e36cf858e76624b9706c3c8e77b6006c0e10_0_0_0", + }, + { + name: "build-ruleID-for-multiple-ports-1-for-vpc-NP", + vpcEnabled: true, + inputSecurityPolicy: &securityPolicyWithMultipleNormalPorts, + inputRule: &securityPolicyWithMultipleNormalPorts.Spec.Rules[1], + ruleIdx: 1, + portIdx: 0, + portAddressIdx: 0, + hasNamedPort: false, + portNumber: -1, + createdFor: common.ResourceTypeNetworkPolicy, + expectedRuleID: "spMulPorts_spMulPortsuidA_555356be_88_1236.1237", + }, + { + name: "build-ruleID-for-multiple-ports-with-one-named-port-for-VPC", + vpcEnabled: true, + inputSecurityPolicy: &securityPolicyWithOneNamedPort, + inputRule: &securityPolicyWithOneNamedPort.Spec.Rules[0], + ruleIdx: 0, + portIdx: 0, + portAddressIdx: 0, + hasNamedPort: true, + portNumber: 80, + createdFor: common.ResourceTypeSecurityPolicy, + expectedRuleID: "spNamedPorts_spNamedPortsuidA_3f7c7d8c_80", + }, + { + name: "build-ruleID-for-multiple-ports-with-one-named-port-for-T1", + vpcEnabled: false, + inputSecurityPolicy: &securityPolicyWithOneNamedPort, + inputRule: &securityPolicyWithOneNamedPort.Spec.Rules[0], + ruleIdx: 0, + portIdx: 0, + portAddressIdx: 0, + hasNamedPort: true, + portNumber: 80, + createdFor: common.ResourceTypeSecurityPolicy, + expectedRuleID: "sp_spNamedPortsuidA_3f7c7d8c8449687178002f23599add04bf0c3250_0_0_0", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + svc.NSXConfig.EnableVPCNetwork = tt.vpcEnabled + observedRuleID := svc.buildExpandedRuleID(tt.inputSecurityPolicy, tt.ruleIdx, tt.portIdx, tt.portAddressIdx, tt.hasNamedPort, tt.portNumber, tt.createdFor) + assert.Equal(t, tt.expectedRuleID, observedRuleID) + }) + } +} + func TestBuildSecurityPolicyName(t *testing.T) { svc := &SecurityPolicyService{ Service: common.Service{ @@ -1004,7 +1267,7 @@ func TestBuildSecurityPolicyName(t *testing.T) { }, }, createdFor: common.ResourceTypeSecurityPolicy, - expName: "sp_ns1_securitypolicy1", + expName: "securitypolicy1", expId: "sp_uid1", }, { @@ -1052,7 +1315,7 @@ func TestBuildSecurityPolicyName(t *testing.T) { } { t.Run(tc.name, func(t *testing.T) { svc.NSXConfig.EnableVPCNetwork = tc.vpcEnabled - name := svc.buildSecurityPolicyName(tc.obj, tc.createdFor) + name := svc.buildSecurityPolicyName(tc.obj) assert.Equal(t, tc.expName, name) assert.True(t, len(name) <= common.MaxNameLength) id := svc.buildSecurityPolicyID(tc.obj, tc.createdFor) @@ -1093,51 +1356,51 @@ func TestBuildGroupName(t *testing.T) { expId string }{ { - name: "src rule without name", + name: "src peer group for rule without user-defined name", ruleIdx: 0, isSource: true, enableVPC: true, - expName: "sp1_0_src", - expId: "sp1_c5db1800-ce4c-11de-bedc-84a0de00c35b_0_src", + expName: "sp1_d0b8e36c_src", + expId: "sp1_c5db1800-ce4c-11de-bedc-84a0de00c35b_d0b8e36c_src", }, { - name: "dst rule without name", + name: "dst peer group for rule without user-defined name", ruleIdx: 0, isSource: false, enableVPC: true, - expName: "sp1_0_dst", - expId: "sp1_c5db1800-ce4c-11de-bedc-84a0de00c35b_0_dst", + expName: "sp1_d0b8e36c_dst", + expId: "sp1_c5db1800-ce4c-11de-bedc-84a0de00c35b_d0b8e36c_dst", }, { - name: "dst rule without name with T1", + name: "dst peer group for rule without user-defined name for T1", ruleIdx: 0, isSource: false, enableVPC: false, - expName: "sp1_0_dst", + expName: "sp1_d0b8e36c_dst", expId: "sp_c5db1800-ce4c-11de-bedc-84a0de00c35b_0_dst", }, { - name: "src rule with name", + name: "src peer group for rule with user-defined name", ruleIdx: 1, isSource: true, enableVPC: true, - expName: "MultipleNormalPorts-rule1_src", - expId: "sp1_c5db1800-ce4c-11de-bedc-84a0de00c35b_1_src", + expName: "sp1_555356be_src", + expId: "sp1_c5db1800-ce4c-11de-bedc-84a0de00c35b_555356be_src", }, { - name: "dst rule with name", + name: "dst peer group for rule with user-defined name", ruleIdx: 1, isSource: false, enableVPC: true, - expName: "MultipleNormalPorts-rule1_dst", - expId: "sp1_c5db1800-ce4c-11de-bedc-84a0de00c35b_1_dst", + expName: "sp1_555356be_dst", + expId: "sp1_c5db1800-ce4c-11de-bedc-84a0de00c35b_555356be_dst", }, { - name: "dst rule with name with T1", + name: "dst peer group for rule with user-defined name for T1", ruleIdx: 1, isSource: false, enableVPC: false, - expName: "MultipleNormalPorts-rule1_dst", + expName: "sp1_555356be_dst", expId: "sp_c5db1800-ce4c-11de-bedc-84a0de00c35b_1_dst", }, } { @@ -1161,31 +1424,45 @@ func TestBuildGroupName(t *testing.T) { expId string }{ { - name: "rule without name", + name: "applied group for rule without user-defined name", ruleIdx: 0, enableVPC: true, - expName: "sp1_0_scope", - expId: "sp1_c5db1800-ce4c-11de-bedc-84a0de00c35b_0_scope", + expName: "sp1_d0b8e36c_scope", + expId: "sp1_c5db1800-ce4c-11de-bedc-84a0de00c35b_d0b8e36c_scope", }, { - name: "rule with name", + name: "applied group for rule with user-defined name", ruleIdx: 1, enableVPC: true, - expName: "MultipleNormalPorts-rule1_scope", - expId: "sp1_c5db1800-ce4c-11de-bedc-84a0de00c35b_1_scope", + expName: "sp1_555356be_scope", + expId: "sp1_c5db1800-ce4c-11de-bedc-84a0de00c35b_555356be_scope", + }, + { + name: "applied group for rule without user-defined name", + ruleIdx: 0, + enableVPC: false, + expName: "sp1_d0b8e36c_scope", + expId: "sp_c5db1800-ce4c-11de-bedc-84a0de00c35b_0_scope", + }, + { + name: "applied group fpr rule with user-defined name for T1", + ruleIdx: 1, + enableVPC: false, + expName: "sp1_555356be_scope", + expId: "sp_c5db1800-ce4c-11de-bedc-84a0de00c35b_1_scope", }, { name: "policy applied group", ruleIdx: -1, enableVPC: true, - expName: "ns1_sp1_scope", + expName: "sp1_scope", expId: "sp1_c5db1800-ce4c-11de-bedc-84a0de00c35b_scope", }, { - name: "policy applied group with T1", + name: "policy applied group for T1", ruleIdx: -1, enableVPC: false, - expName: "ns1_sp1_scope", + expName: "sp1_scope", expId: "sp_c5db1800-ce4c-11de-bedc-84a0de00c35b_scope", }, } { diff --git a/pkg/nsx/services/securitypolicy/expand.go b/pkg/nsx/services/securitypolicy/expand.go index 91ee24139..5a78496ef 100644 --- a/pkg/nsx/services/securitypolicy/expand.go +++ b/pkg/nsx/services/securitypolicy/expand.go @@ -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 } @@ -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 } @@ -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 @@ -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 } diff --git a/pkg/nsx/services/securitypolicy/expand_test.go b/pkg/nsx/services/securitypolicy/expand_test.go index 7f30e60ac..56c30485e 100644 --- a/pkg/nsx/services/securitypolicy/expand_test.go +++ b/pkg/nsx/services/securitypolicy/expand_test.go @@ -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, @@ -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 { @@ -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) }) } diff --git a/pkg/nsx/services/securitypolicy/firewall_test.go b/pkg/nsx/services/securitypolicy/firewall_test.go index 85969bc3f..38d268ee1 100644 --- a/pkg/nsx/services/securitypolicy/firewall_test.go +++ b/pkg/nsx/services/securitypolicy/firewall_test.go @@ -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" @@ -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 @@ -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, @@ -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"}, diff --git a/pkg/util/utils.go b/pkg/util/utils.go index e3e663c72..efe098e55 100644 --- a/pkg/util/utils.go +++ b/pkg/util/utils.go @@ -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 ( @@ -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 } @@ -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 diff --git a/pkg/util/utils_test.go b/pkg/util/utils_test.go index 87b121e5d..a46ca0a9f 100644 --- a/pkg/util/utils_test.go +++ b/pkg/util/utils_test.go @@ -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) {