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) {