From 4900e7d90855cd45cf594d6a220ebda6727a444f Mon Sep 17 00:00:00 2001 From: Deng Yun Date: Mon, 23 Sep 2024 11:44:10 +0800 Subject: [PATCH] Unify NSX resoruce ID and name connector as underline (#763) Previsouly, we use hyphen "-" to connect strings when building NSX resource name, and use underline "_" to connect strings when builindg NSX resource ID. This patch is to unify NSX resource ID and name connecotr as underline when building ID and name from K8s CR. For the NSX resoruce ID and name convention, this patch is follow the standard in PR:https://github.com/vmware-tanzu/nsx-operator/pull/643 These NSX resources name are impacted in this change: VPC Subnet SubnetPort SecurityPolicy and NetworkPolicy NSGroup and IPSetGroup NSRule Share StaticRoute IPAllocation This patch is aslo to add two flags for GC and vpcCleanup in order to distinguish these two cases. --- .../networkpolicy/networkpolicy_controller.go | 4 +- .../securitypolicy_controller.go | 4 +- .../securitypolicy_controller_test.go | 8 +- pkg/nsx/services/common/types.go | 29 +- .../services/ipaddressallocation/builder.go | 2 +- .../ipaddressallocation/builder_test.go | 7 +- pkg/nsx/services/securitypolicy/builder.go | 108 ++-- .../services/securitypolicy/builder_test.go | 296 ++++++++--- pkg/nsx/services/securitypolicy/expand.go | 16 +- .../services/securitypolicy/expand_test.go | 6 +- pkg/nsx/services/securitypolicy/firewall.go | 43 +- .../services/securitypolicy/firewall_test.go | 462 ++++++++++++++++-- pkg/nsx/services/securitypolicy/store_test.go | 8 +- pkg/nsx/services/securitypolicy/wrap_test.go | 18 +- pkg/nsx/services/staticroute/builder_test.go | 2 +- .../services/staticroute/staticroute_test.go | 10 +- pkg/nsx/services/subnet/builder.go | 4 +- pkg/nsx/services/subnet/builder_test.go | 8 +- pkg/nsx/services/subnetport/builder_test.go | 8 +- pkg/nsx/services/vpc/builder_test.go | 26 +- pkg/util/utils.go | 33 +- pkg/util/utils_test.go | 55 ++- test/e2e/nsx_security_policy_test.go | 12 +- 23 files changed, 869 insertions(+), 300 deletions(-) diff --git a/pkg/controllers/networkpolicy/networkpolicy_controller.go b/pkg/controllers/networkpolicy/networkpolicy_controller.go index 293fb4e3b..66097ee98 100644 --- a/pkg/controllers/networkpolicy/networkpolicy_controller.go +++ b/pkg/controllers/networkpolicy/networkpolicy_controller.go @@ -137,7 +137,7 @@ func (r *NetworkPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reques } else { if controllerutil.ContainsFinalizer(networkPolicy, servicecommon.NetworkPolicyFinalizerName) { metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteTotal, MetricResType) - if err := r.Service.DeleteSecurityPolicy(networkPolicy, false, servicecommon.ResourceTypeNetworkPolicy); err != nil { + if err := r.Service.DeleteSecurityPolicy(networkPolicy, false, false, servicecommon.ResourceTypeNetworkPolicy); err != nil { log.Error(err, "deletion failed, would retry exponentially", "networkpolicy", req.NamespacedName) deleteFail(r, ctx, networkPolicy, &err) return ResultRequeue, err @@ -203,7 +203,7 @@ func (r *NetworkPolicyReconciler) CollectGarbage(ctx context.Context) { for elem := range diffSet { log.V(1).Info("GC collected NetworkPolicy", "ID", elem) metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteTotal, MetricResType) - err = r.Service.DeleteSecurityPolicy(types.UID(elem), true, servicecommon.ResourceTypeNetworkPolicy) + err = r.Service.DeleteSecurityPolicy(types.UID(elem), true, false, servicecommon.ResourceTypeNetworkPolicy) if err != nil { metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteFailTotal, MetricResType) } else { diff --git a/pkg/controllers/securitypolicy/securitypolicy_controller.go b/pkg/controllers/securitypolicy/securitypolicy_controller.go index 3bcddf95d..f63a41bef 100644 --- a/pkg/controllers/securitypolicy/securitypolicy_controller.go +++ b/pkg/controllers/securitypolicy/securitypolicy_controller.go @@ -201,7 +201,7 @@ func (r *SecurityPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reque log.Info("reconciling CR to delete securitypolicy", "securitypolicy", req.NamespacedName) if controllerutil.ContainsFinalizer(obj, finalizerName) { metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteTotal, MetricResType) - if err := r.Service.DeleteSecurityPolicy(realObj.UID, false, servicecommon.ResourceTypeSecurityPolicy); err != nil { + if err := r.Service.DeleteSecurityPolicy(realObj.UID, false, false, servicecommon.ResourceTypeSecurityPolicy); err != nil { log.Error(err, "deletion failed, would retry exponentially", "securitypolicy", req.NamespacedName) deleteFail(r, ctx, realObj, &err) return ResultRequeue, err @@ -377,7 +377,7 @@ func (r *SecurityPolicyReconciler) CollectGarbage(ctx context.Context) { for elem := range diffSet { log.V(1).Info("GC collected SecurityPolicy CR", "securityPolicyUID", elem) metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteTotal, MetricResType) - err = r.Service.DeleteSecurityPolicy(types.UID(elem), true, servicecommon.ResourceTypeSecurityPolicy) + err = r.Service.DeleteSecurityPolicy(types.UID(elem), true, false, servicecommon.ResourceTypeSecurityPolicy) if err != nil { metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteFailTotal, MetricResType) } else { diff --git a/pkg/controllers/securitypolicy/securitypolicy_controller_test.go b/pkg/controllers/securitypolicy/securitypolicy_controller_test.go index 2da389dbb..49f70a42c 100644 --- a/pkg/controllers/securitypolicy/securitypolicy_controller_test.go +++ b/pkg/controllers/securitypolicy/securitypolicy_controller_test.go @@ -231,7 +231,7 @@ func TestSecurityPolicyReconciler_Reconcile(t *testing.T) { v1sp.ObjectMeta.DeletionTimestamp = &time return nil }) - patch := gomonkey.ApplyMethod(reflect.TypeOf(service), "DeleteSecurityPolicy", func(_ *securitypolicy.SecurityPolicyService, UID interface{}, isVPCCleanupOrGC bool) error { + patch := gomonkey.ApplyMethod(reflect.TypeOf(service), "DeleteSecurityPolicy", func(_ *securitypolicy.SecurityPolicyService, UID interface{}, isGc bool, isVPCCleanup bool) error { assert.FailNow(t, "should not be called") return nil }) @@ -247,7 +247,7 @@ func TestSecurityPolicyReconciler_Reconcile(t *testing.T) { v1sp.Finalizers = []string{common.T1SecurityPolicyFinalizerName} return nil }) - patch = gomonkey.ApplyMethod(reflect.TypeOf(service), "DeleteSecurityPolicy", func(_ *securitypolicy.SecurityPolicyService, UID interface{}, isVPCCleanupOrGC bool) error { + patch = gomonkey.ApplyMethod(reflect.TypeOf(service), "DeleteSecurityPolicy", func(_ *securitypolicy.SecurityPolicyService, UID interface{}, isGc bool, isVPCCleanup bool) error { return nil }) k8sClient.EXPECT().Update(ctx, gomock.Any(), gomock.Any()).Return(nil) @@ -276,7 +276,7 @@ func TestSecurityPolicyReconciler_GarbageCollector(t *testing.T) { a.Insert("2345") return a }) - patch.ApplyMethod(reflect.TypeOf(service), "DeleteSecurityPolicy", func(_ *securitypolicy.SecurityPolicyService, UID interface{}, isVPCCleanupOrGC bool) error { + patch.ApplyMethod(reflect.TypeOf(service), "DeleteSecurityPolicy", func(_ *securitypolicy.SecurityPolicyService, UID interface{}, isGc bool, isVPCCleanup bool) error { return nil }) defer patch.Reset() @@ -325,7 +325,7 @@ func TestSecurityPolicyReconciler_GarbageCollector(t *testing.T) { a := sets.New[string]() return a }) - patch.ApplyMethod(reflect.TypeOf(service), "DeleteSecurityPolicy", func(_ *securitypolicy.SecurityPolicyService, UID interface{}, isVPCCleanupOrGC bool) error { + patch.ApplyMethod(reflect.TypeOf(service), "DeleteSecurityPolicy", func(_ *securitypolicy.SecurityPolicyService, UID interface{}, isGc bool, isVPCCleanup bool) error { assert.FailNow(t, "should not be called") return nil }) diff --git a/pkg/nsx/services/common/types.go b/pkg/nsx/services/common/types.go index f4b77e2a3..31ec87ff3 100644 --- a/pkg/nsx/services/common/types.go +++ b/pkg/nsx/services/common/types.go @@ -91,6 +91,7 @@ const ( ValueMajorVersion string = "1" ValueMinorVersion string = "0" ValuePatchVersion string = "0" + ConnectorUnderline string = "_" GCInterval = 60 * time.Second RealizeTimeout = 2 * time.Minute @@ -119,20 +120,20 @@ const ( IndexKeyNodeName = "IndexKeyNodeName" GCValidationInterval uint16 = 720 - RuleSuffixIngressAllow = "ingress-allow" - RuleSuffixEgressAllow = "egress-allow" - RuleSuffixIngressDrop = "ingress-isolation" - RuleSuffixEgressDrop = "egress-isolation" - RuleSuffixIngressReject = "ingress-reject" - RuleSuffixEgressReject = "egress-reject" - DefaultProject = "default" - SecurityPolicyPrefix = "sp" - NetworkPolicyPrefix = "np" - TargetGroupSuffix = "scope" - SrcGroupSuffix = "src" - DstGroupSuffix = "dst" - IpSetGroupSuffix = "ipset" - ShareSuffix = "share" + RuleIngress = "ingress" + RuleEgress = "egress" + RuleActionAllow = "allow" + RuleActionDrop = "isolation" + RuleActionReject = "reject" + RuleAnyPorts = "all" + DefaultProject = "default" + SecurityPolicyPrefix = "sp" + NetworkPolicyPrefix = "np" + TargetGroupSuffix = "scope" + SrcGroupSuffix = "src" + DstGroupSuffix = "dst" + IpSetGroupSuffix = "ipset" + ShareSuffix = "share" ) var ( diff --git a/pkg/nsx/services/ipaddressallocation/builder.go b/pkg/nsx/services/ipaddressallocation/builder.go index d1acc2e9b..9f7ac65d8 100644 --- a/pkg/nsx/services/ipaddressallocation/builder.go +++ b/pkg/nsx/services/ipaddressallocation/builder.go @@ -49,7 +49,7 @@ func (service *IPAddressAllocationService) buildIPAddressAllocationID(IPAddressA } func (service *IPAddressAllocationService) buildIPAddressAllocationName(IPAddressAllocation *v1alpha1.IPAddressAllocation) string { - return util.GenerateTruncName(common.MaxNameLength, IPAddressAllocation.ObjectMeta.Name, "", "", "", service.NSXConfig.Cluster) + return util.GenerateTruncName(common.MaxNameLength, IPAddressAllocation.ObjectMeta.Name, "", "", "", "") } func (service *IPAddressAllocationService) buildIPAddressAllocationTags(IPAddressAllocation *v1alpha1.IPAddressAllocation) []model.Tag { diff --git a/pkg/nsx/services/ipaddressallocation/builder_test.go b/pkg/nsx/services/ipaddressallocation/builder_test.go index 2c8cd8884..a117e1cae 100644 --- a/pkg/nsx/services/ipaddressallocation/builder_test.go +++ b/pkg/nsx/services/ipaddressallocation/builder_test.go @@ -23,8 +23,7 @@ import ( "github.com/stretchr/testify/assert" ) -type fakeQueryClient struct { -} +type fakeQueryClient struct{} func (qIface *fakeQueryClient) List(_ string, _ *string, _ *string, _ *int64, _ *bool, _ *string) (model.SearchResponse, error) { cursor := "2" @@ -140,8 +139,8 @@ func TestBuildIPAddressAllocation(t *testing.T) { result, err := ipAllocService.BuildIPAddressAllocation(ipAlloc) assert.Nil(t, err) - assert.Equal(t, "test-ip-alloc-uid1", *result.Id) - assert.Equal(t, "default-test-ip-alloc", *result.DisplayName) + assert.Equal(t, "test-ip-alloc_uid1", *result.Id) + assert.Equal(t, "test-ip-alloc", *result.DisplayName) assert.Equal(t, int64(10), *result.AllocationSize) assert.Equal(t, "EXTERNAL", *result.IpAddressBlockVisibility) assert.Equal(t, 5, len(result.Tags)) diff --git a/pkg/nsx/services/securitypolicy/builder.go b/pkg/nsx/services/securitypolicy/builder.go index c641ed3f7..b1a31e45e 100644 --- a/pkg/nsx/services/securitypolicy/builder.go +++ b/pkg/nsx/services/securitypolicy/builder.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "sort" + "strconv" "strings" "github.com/vmware/vsphere-automation-sdk-go/runtime/data" @@ -49,7 +50,7 @@ func (service *SecurityPolicyService) buildSecurityPolicyName(obj *v1alpha1.Secu } // 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, fmt.Sprintf("%s-%s", obj.Namespace, obj.Name), prefix, "", "", "") + return util.GenerateTruncName(common.MaxNameLength, strings.Join([]string{obj.Namespace, obj.Name}, common.ConnectorUnderline), prefix, "", "", "") } func (service *SecurityPolicyService) buildSecurityPolicyID(obj *v1alpha1.SecurityPolicy, createdFor string) string { @@ -378,7 +379,7 @@ func (service *SecurityPolicyService) buildAppliedGroupID(obj *v1alpha1.Security if IsVPCEnabled(service) { suffix := common.TargetGroupSuffix if ruleIdx != -1 { - suffix = fmt.Sprintf("%d_%s", ruleIdx, suffix) + suffix = strings.Join([]string{strconv.Itoa(ruleIdx), suffix}, common.ConnectorUnderline) } return util.GenerateIDByObjectWithSuffix(obj, suffix) } @@ -403,10 +404,10 @@ func (service *SecurityPolicyService) buildAppliedGroupPath(obj *v1alpha1.Securi if err != nil { return "", err } - orgId := (*vpcInfo).OrgID - projectId := (*vpcInfo).ProjectID - vpcId := (*vpcInfo).VPCID - return fmt.Sprintf("/orgs/%s/projects/%s/vpcs/%s/groups/%s", orgId, projectId, vpcId, groupID), nil + orgID := (*vpcInfo).OrgID + projectID := (*vpcInfo).ProjectID + vpcID := (*vpcInfo).VPCID + return fmt.Sprintf("/orgs/%s/projects/%s/vpcs/%s/groups/%s", orgID, projectID, vpcID, groupID), nil } return fmt.Sprintf("/infra/domains/%s/groups/%s", getDomain(service), groupID), nil @@ -417,13 +418,13 @@ func (service *SecurityPolicyService) buildAppliedGroupName(obj *v1alpha1.Securi var rule *v1alpha1.SecurityPolicyRule if ruleIdx != -1 { rule = &(obj.Spec.Rules[ruleIdx]) - ruleName := fmt.Sprintf("%s-%d", obj.Name, 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, "", "") } - ruleName := fmt.Sprintf("%s-%s", obj.Namespace, obj.Name) + ruleName := strings.Join([]string{obj.Namespace, obj.Name}, common.ConnectorUnderline) return util.GenerateTruncName(common.MaxNameLength, ruleName, "", common.TargetGroupSuffix, "", "") } @@ -602,7 +603,7 @@ func (service *SecurityPolicyService) buildRuleID(obj *v1alpha1.SecurityPolicy, ruleHash := fmt.Sprintf("%s", util.Sha1(string(serializedBytes))) ruleIdxStr := fmt.Sprintf("%d", ruleIdx) if IsVPCEnabled(service) { - suffix := fmt.Sprintf("%s_%s", ruleIdxStr, ruleHash) + suffix := strings.Join([]string{ruleIdxStr, ruleHash}, common.ConnectorUnderline) return util.GenerateIDByObjectWithSuffix(obj, suffix) } prefix := common.SecurityPolicyPrefix @@ -614,7 +615,7 @@ func (service *SecurityPolicyService) buildRuleID(obj *v1alpha1.SecurityPolicy, func (service *SecurityPolicyService) buildRuleDisplayName(rule *v1alpha1.SecurityPolicyRule, portIdx, portNumber int, hasNamedport bool, createdFor string) (string, error) { var ruleName string - var suffix string + var ruleAct string ruleDirection, err := getRuleDirection(rule) if err != nil { @@ -625,35 +626,29 @@ func (service *SecurityPolicyService) buildRuleDisplayName(rule *v1alpha1.Securi return "", err } + switch ruleAction { + case util.ToUpper(v1alpha1.RuleActionAllow): + ruleAct = common.RuleActionAllow + case util.ToUpper(v1alpha1.RuleActionDrop): + ruleAct = common.RuleActionDrop + case util.ToUpper(v1alpha1.RuleActionReject): + ruleAct = common.RuleActionReject + } + ruleDir := common.RuleEgress if ruleDirection == "IN" { - switch ruleAction { - case util.ToUpper(v1alpha1.RuleActionAllow): - suffix = common.RuleSuffixIngressAllow - case util.ToUpper(v1alpha1.RuleActionDrop): - suffix = common.RuleSuffixIngressDrop - case util.ToUpper(v1alpha1.RuleActionReject): - suffix = common.RuleSuffixIngressReject - } - } else { - switch ruleAction { - case util.ToUpper(v1alpha1.RuleActionAllow): - suffix = common.RuleSuffixEgressAllow - case util.ToUpper(v1alpha1.RuleActionDrop): - suffix = common.RuleSuffixEgressDrop - case util.ToUpper(v1alpha1.RuleActionReject): - suffix = common.RuleSuffixEgressReject - } + ruleDir = common.RuleIngress } + suffix := strings.Join([]string{ruleDir, ruleAct}, common.ConnectorUnderline) if len(rule.Name) > 0 { // For the internal security policy rule converted from network policy, skipping to add suffix for the rule name // if it has its own name generated, usually, it's for the internal isolation security policy rule created for network policy. ruleName = rule.Name if createdFor != common.ResourceTypeNetworkPolicy { - // If user defines the rule name, the generated NSX security policy rule will also be added with the same suffix: "-direction-action" as building rulePortsString - // e.g. input security policy's rule name: sp-rule, - // the generated NSX security policy rule name: sp-rule-ingress-allow - ruleName = ruleName + "-" + suffix + // If user defines the rule name, the generated NSX security policy rule will also be added with the same suffix: "_direction_action" as building rulePortsString + // e.g. input security policy's rule name: sp_rule, + // the generated NSX security policy rule name: sp_rule_ingress_allow + ruleName = strings.Join([]string{ruleName, suffix}, common.ConnectorUnderline) } } else { ruleName = service.buildRulePortsString(&rule.Ports, suffix) @@ -663,11 +658,11 @@ func (service *SecurityPolicyService) buildRuleDisplayName(rule *v1alpha1.Securi return util.GenerateTruncName(common.MaxNameLength, ruleName, "", "", "", ""), nil } 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. - // 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, "-"+suffix) + // 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. + // 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 } } @@ -753,9 +748,10 @@ func (service *SecurityPolicyService) buildRulePeerGroupID(obj *v1alpha1.Securit suffix = common.SrcGroupSuffix } if IsVPCEnabled(service) { - return util.GenerateIDByObjectWithSuffix(obj, fmt.Sprintf("%d_%s", ruleIdx, suffix)) + suffix = strings.Join([]string{strconv.Itoa(ruleIdx), suffix}, common.ConnectorUnderline) + return util.GenerateIDByObjectWithSuffix(obj, suffix) } - return util.GenerateID(string(obj.UID), common.SecurityPolicyPrefix, suffix, fmt.Sprintf("%d", ruleIdx)) + return util.GenerateID(string(obj.UID), common.SecurityPolicyPrefix, suffix, strconv.Itoa(ruleIdx)) } func (service *SecurityPolicyService) buildRulePeerGroupName(obj *v1alpha1.SecurityPolicy, ruleIdx int, isSource bool) string { @@ -764,7 +760,7 @@ func (service *SecurityPolicyService) buildRulePeerGroupName(obj *v1alpha1.Secur if isSource == true { suffix = common.SrcGroupSuffix } - ruleName := fmt.Sprintf("%s-%d", obj.Name, ruleIdx) + ruleName := strings.Join([]string{obj.Name, strconv.Itoa(ruleIdx)}, common.ConnectorUnderline) if len(rule.Name) > 0 { ruleName = rule.Name } @@ -918,7 +914,7 @@ func (service *SecurityPolicyService) buildRulePeerGroup(obj *v1alpha1.SecurityP } func (service *SecurityPolicyService) buildExpandedRuleId(ruleBaseId string, portIdx int, portAddressIdx int) string { - return fmt.Sprintf("%s_%d_%d", ruleBaseId, portIdx, portAddressIdx) + 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, @@ -1709,13 +1705,15 @@ func (service *SecurityPolicyService) updatePeerExpressions(obj *v1alpha1.Securi return totalCriteriaCount, totalExprCount, nil } -func (service *SecurityPolicyService) buildShareName(nsxResourceID, groupName string) string { - nsxShareName := util.GenerateTruncName(common.MaxNameLength, fmt.Sprintf("%s-group-%s", nsxResourceID, groupName), "", common.ShareSuffix, "", "") +func (service *SecurityPolicyService) buildShareName(nsxProjectID, groupName string) string { + resName := strings.Join([]string{nsxProjectID, "group", groupName}, common.ConnectorUnderline) + nsxShareName := util.GenerateTruncName(common.MaxNameLength, resName, "", common.ShareSuffix, "", "") return nsxShareName } -func (service *SecurityPolicyService) buildShareID(nsxResourceID, groupID string) string { - nsxShareId := util.GenerateID(fmt.Sprintf("%s_group_%s", nsxResourceID, groupID), "", common.ShareSuffix, "") +func (service *SecurityPolicyService) buildShareID(nsxProjectID, groupID string) string { + resID := strings.Join([]string{nsxProjectID, "group", groupID}, common.ConnectorUnderline) + nsxShareId := util.GenerateID(resID, "", common.ShareSuffix, "") return nsxShareId } @@ -1795,9 +1793,9 @@ func (service *SecurityPolicyService) buildGroupShare(obj *v1alpha1.SecurityPoli var shareName string resourceType := common.ResourceTypeShare - projectId := vpcInfo.ProjectID - shareId = service.buildShareID(projectId, *group.Id) - shareName = service.buildShareName(projectId, *group.DisplayName) + projectID := vpcInfo.ProjectID + shareId = service.buildShareID(projectID, *group.Id) + shareName = service.buildShareName(projectID, *group.DisplayName) shareTags = service.buildShareTags(obj, infraGroupShared, projectGroupShared, createdFor) childSharedResource, err := service.buildChildSharedResource(shareId, sharedGroupPath) if err != nil { @@ -1875,7 +1873,7 @@ func (service *SecurityPolicyService) buildRulePortString(port *v1alpha1.Securit func (service *SecurityPolicyService) buildRulePortsString(ports *[]v1alpha1.SecurityPolicyPort, suffix string) string { portsString := "" if ports == nil || len(*ports) == 0 { - portsString = "all" + portsString = common.RuleAnyPorts } else { for idx, p := range *ports { port := p @@ -1883,25 +1881,17 @@ func (service *SecurityPolicyService) buildRulePortsString(ports *[]v1alpha1.Sec if idx == 0 { portsString = portString } else { - portsString = portsString + "-" + portString + portsString = strings.Join([]string{portsString, portString}, common.ConnectorUnderline) } } } return util.GenerateTruncName(common.MaxNameLength, portsString, "", suffix, "", "") } -func (service *SecurityPolicyService) BuildNetworkPolicyAllowPolicyName(uid string) string { - return fmt.Sprintf("%s-allow", uid) -} - func (service *SecurityPolicyService) BuildNetworkPolicyAllowPolicyID(uid string) string { - return fmt.Sprintf("%s_allow", uid) -} - -func (service *SecurityPolicyService) BuildNetworkPolicyIsolationPolicyName(uid string) string { - return fmt.Sprintf("%s-isolation", uid) + return strings.Join([]string{uid, common.RuleActionAllow}, common.ConnectorUnderline) } func (service *SecurityPolicyService) BuildNetworkPolicyIsolationPolicyID(uid string) string { - return fmt.Sprintf("%s_isolation", uid) + return strings.Join([]string{uid, common.RuleActionDrop}, common.ConnectorUnderline) } diff --git a/pkg/nsx/services/securitypolicy/builder_test.go b/pkg/nsx/services/securitypolicy/builder_test.go index 7c9ef1b5f..5fbca2def 100644 --- a/pkg/nsx/services/securitypolicy/builder_test.go +++ b/pkg/nsx/services/securitypolicy/builder_test.go @@ -34,18 +34,20 @@ func TestBuildSecurityPolicy(t *testing.T) { }, ) - podSelectorRule0IDPort000 := service.buildExpandedRuleId(service.buildRuleID(&spWithPodSelector, &spWithPodSelector.Spec.Rules[0], 0, common.ResourceTypeSecurityPolicy), 0, 0) - podSelectorRule1IDPort000 := service.buildExpandedRuleId(service.buildRuleID(&spWithPodSelector, &spWithPodSelector.Spec.Rules[1], 1, common.ResourceTypeSecurityPolicy), 0, 0) - vmSelectorRule0IDPort000 := service.buildExpandedRuleId(service.buildRuleID(&spWithVMSelector, &spWithVMSelector.Spec.Rules[0], 0, common.ResourceTypeSecurityPolicy), 0, 0) - vmSelectorRule1IDPort000 := service.buildExpandedRuleId(service.buildRuleID(&spWithVMSelector, &spWithVMSelector.Spec.Rules[1], 1, common.ResourceTypeSecurityPolicy), 0, 0) - vmSelectorRule2IDPort000 := service.buildExpandedRuleId(service.buildRuleID(&spWithVMSelector, &spWithVMSelector.Spec.Rules[2], 2, common.ResourceTypeSecurityPolicy), 0, 0) + podSelectorRule0Name00 := "rule-with-pod-ns-selector_ingress_allow" + podSelectorRule0IDPort000 := "sp_uidA_0_2c822e90b1377b346014adfa583f08a99dee52a8_0_0" - podSelectorRule0Name00, _ := service.buildRuleDisplayName(&spWithPodSelector.Spec.Rules[0], 0, -1, false, common.ResourceTypeSecurityPolicy) - podSelectorRule1Name00, _ := service.buildRuleDisplayName(&spWithPodSelector.Spec.Rules[1], 0, -1, false, common.ResourceTypeSecurityPolicy) + podSelectorRule1Name00 := "rule-with-ns-selector_ingress_allow" + podSelectorRule1IDPort000 := "sp_uidA_1_2a4595d0dd582c2ae5613245ad7b39de5ade2e20_0_0" - vmSelectorRule0Name00, _ := service.buildRuleDisplayName(&spWithVMSelector.Spec.Rules[0], 0, -1, false, common.ResourceTypeSecurityPolicy) - vmSelectorRule1Name00, _ := service.buildRuleDisplayName(&spWithVMSelector.Spec.Rules[1], 0, -1, false, common.ResourceTypeSecurityPolicy) - vmSelectorRule2Name00, _ := service.buildRuleDisplayName(&spWithVMSelector.Spec.Rules[2], 0, -1, false, common.ResourceTypeSecurityPolicy) + vmSelectorRule0Name00 := "rule-with-VM-selector_egress_isolation" + vmSelectorRule0IDPort000 := "sp_uidB_0_67410606c486d2ba38002ed076a2a4211c9d49b5_0_0" + + vmSelectorRule1Name00 := "rule-with-ns-selector_egress_isolation" + vmSelectorRule1IDPort000 := "sp_uidB_1_7d721f087be35f0bf318f4847b5acdc3d2b91446_0_0" + + vmSelectorRule2Name00 := "all_egress_isolation" + vmSelectorRule2IDPort000 := "sp_uidB_2_a40c813916cc397fcd2260e48cc773d4c9b08565_0_0" tests := []struct { name string @@ -53,7 +55,7 @@ func TestBuildSecurityPolicy(t *testing.T) { expectedPolicy *model.SecurityPolicy }{ { - name: "security-policy-with-pod-selector", + name: "security-policy-with-pod-selector For T1", inputPolicy: &spWithPodSelector, expectedPolicy: &model.SecurityPolicy{ DisplayName: &spName, @@ -65,24 +67,24 @@ func TestBuildSecurityPolicy(t *testing.T) { DisplayName: &podSelectorRule0Name00, Id: &podSelectorRule0IDPort000, DestinationGroups: []string{"ANY"}, - Direction: &nsxDirectionIn, + Direction: &nsxRuleDirectionIn, Scope: []string{"/infra/domains/k8scl-one/groups/sp_uidA_0_scope"}, SequenceNumber: &seq0, Services: []string{"ANY"}, SourceGroups: []string{"/infra/domains/k8scl-one/groups/sp_uidA_0_src"}, - Action: &nsxActionAllow, + Action: &nsxRuleActionAllow, Tags: basicTags, }, { DisplayName: &podSelectorRule1Name00, Id: &podSelectorRule1IDPort000, DestinationGroups: []string{"ANY"}, - Direction: &nsxDirectionIn, + Direction: &nsxRuleDirectionIn, Scope: []string{"ANY"}, SequenceNumber: &seq1, Services: []string{"ANY"}, SourceGroups: []string{"/infra/domains/k8scl-one/groups/sp_uidA_1_src"}, - Action: &nsxActionAllow, + Action: &nsxRuleActionAllow, ServiceEntries: []*data.StructValue{serviceEntry}, Tags: basicTags, }, @@ -91,53 +93,53 @@ func TestBuildSecurityPolicy(t *testing.T) { }, }, { - name: "security-policy-with-VM-selector", + name: "security-policy-with-VM-selector For T1", inputPolicy: &spWithVMSelector, expectedPolicy: &model.SecurityPolicy{ - DisplayName: &spName, - Id: &spID, - Scope: []string{"/infra/domains/k8scl-one/groups/sp_uidA_scope"}, + DisplayName: common.String("sp_ns1_spB"), + Id: common.String("sp_uidB"), + Scope: []string{"/infra/domains/k8scl-one/groups/sp_uidB_scope"}, SequenceNumber: &seq0, Rules: []model.Rule{ { DisplayName: &vmSelectorRule0Name00, Id: &vmSelectorRule0IDPort000, - DestinationGroups: []string{"/infra/domains/k8scl-one/groups/sp_uidA_0_dst"}, - Direction: &nsxDirectionOut, - Scope: []string{"/infra/domains/k8scl-one/groups/sp_uidA_0_scope"}, + DestinationGroups: []string{"/infra/domains/k8scl-one/groups/sp_uidB_0_dst"}, + Direction: &nsxRuleDirectionOut, + Scope: []string{"/infra/domains/k8scl-one/groups/sp_uidB_0_scope"}, SequenceNumber: &seq0, Services: []string{"ANY"}, SourceGroups: []string{"ANY"}, - Action: &nsxActionDrop, - Tags: basicTags, + Action: &nsxRuleActionDrop, + Tags: basicTagsForSpWithVMSelector, }, { DisplayName: &vmSelectorRule1Name00, Id: &vmSelectorRule1IDPort000, - DestinationGroups: []string{"/infra/domains/k8scl-one/groups/sp_uidA_1_dst"}, - Direction: &nsxDirectionOut, + DestinationGroups: []string{"/infra/domains/k8scl-one/groups/sp_uidB_1_dst"}, + Direction: &nsxRuleDirectionOut, Scope: []string{"ANY"}, SequenceNumber: &seq1, Services: []string{"ANY"}, SourceGroups: []string{"ANY"}, - Action: &nsxActionDrop, - Tags: basicTags, + Action: &nsxRuleActionDrop, + Tags: basicTagsForSpWithVMSelector, }, { DisplayName: &vmSelectorRule2Name00, Id: &vmSelectorRule2IDPort000, - DestinationGroups: []string{"/infra/domains/k8scl-one/groups/sp_uidA_2_dst"}, - Direction: &nsxDirectionOut, + DestinationGroups: []string{"/infra/domains/k8scl-one/groups/sp_uidB_2_dst"}, + Direction: &nsxRuleDirectionOut, Scope: []string{"ANY"}, SequenceNumber: &seq2, Services: []string{"ANY"}, SourceGroups: []string{"ANY"}, - Action: &nsxActionDrop, - Tags: basicTags, + Action: &nsxRuleActionDrop, + Tags: basicTagsForSpWithVMSelector, }, }, - Tags: basicTags, + Tags: basicTagsForSpWithVMSelector, }, }, } @@ -157,6 +159,165 @@ func TestBuildSecurityPolicy(t *testing.T) { } } +func TestBuildSecurityPolicyForVPC(t *testing.T) { + VPCInfo := make([]common.VPCResourceInfo, 1) + VPCInfo[0].OrgID = "default" + VPCInfo[0].ProjectID = "projectQuality" + VPCInfo[0].VPCID = "vpc1" + + fakeService := fakeSecurityPolicyService() + fakeService.NSXConfig.EnableVPCNetwork = true + mockVPCService := common.MockVPCServiceProvider{} + fakeService.vpcService = &mockVPCService + + // For VPC mode + common.TagValueScopeSecurityPolicyName = common.TagScopeSecurityPolicyName + common.TagValueScopeSecurityPolicyUID = common.TagScopeSecurityPolicyUID + + destinationPorts := data.NewListValue() + destinationPorts.Add(data.NewStringValue("53")) + serviceEntry := data.NewStructValue( + "", + map[string]data.DataValue{ + "source_ports": data.NewListValue(), + "destination_ports": destinationPorts, + "l4_protocol": data.NewStringValue("UDP"), + "resource_type": data.NewStringValue("L4PortSetServiceEntry"), + "marked_for_delete": data.NewBooleanValue(false), + "overridden": data.NewBooleanValue(false), + }, + ) + + patches := gomonkey.ApplyPrivateMethod(reflect.TypeOf(fakeService), "getVPCInfo", + func(s *SecurityPolicyService, spNameSpace string) (*common.VPCResourceInfo, error) { + return &VPCInfo[0], nil + }) + + patches.ApplyPrivateMethod(reflect.TypeOf(fakeService), "getNamespaceUID", + func(s *SecurityPolicyService, ns string) types.UID { + return types.UID(tagValueNSUID) + }) + + defer patches.Reset() + + podSelectorRule0Name00 := "rule-with-pod-ns-selector_ingress_allow" + podSelectorRule0IDPort000 := "spA_uidA_0_2c822e90b1377b346014adfa583f08a99dee52a8_0_0" + + podSelectorRule1Name00 := "rule-with-ns-selector_ingress_allow" + podSelectorRule1IDPort000 := "spA_uidA_1_2a4595d0dd582c2ae5613245ad7b39de5ade2e20_0_0" + + vmSelectorRule0Name00 := "rule-with-VM-selector_egress_isolation" + vmSelectorRule0IDPort000 := "spB_uidB_0_67410606c486d2ba38002ed076a2a4211c9d49b5_0_0" + + vmSelectorRule1Name00 := "rule-with-ns-selector_egress_isolation" + vmSelectorRule1IDPort000 := "spB_uidB_1_7d721f087be35f0bf318f4847b5acdc3d2b91446_0_0" + + vmSelectorRule2Name00 := "all_egress_isolation" + vmSelectorRule2IDPort000 := "spB_uidB_2_a40c813916cc397fcd2260e48cc773d4c9b08565_0_0" + + tests := []struct { + name string + inputPolicy *v1alpha1.SecurityPolicy + expectedPolicy *model.SecurityPolicy + }{ + { + name: "security-policy-with-pod-selector For VPC", + inputPolicy: &spWithPodSelector, + expectedPolicy: &model.SecurityPolicy{ + DisplayName: common.String("spA"), + Id: common.String("spA_uidA"), + Scope: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/spA_uidA_scope"}, + SequenceNumber: &seq0, + Rules: []model.Rule{ + { + DisplayName: &podSelectorRule0Name00, + Id: &podSelectorRule0IDPort000, + DestinationGroups: []string{"ANY"}, + Direction: &nsxRuleDirectionIn, + Scope: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/spA_uidA_0_scope"}, + SequenceNumber: &seq0, + Services: []string{"ANY"}, + SourceGroups: []string{"/orgs/default/projects/projectQuality/infra/domains/default/groups/spA_uidA_0_src"}, + Action: &nsxRuleActionAllow, + Tags: vpcBasicTags, + }, + { + DisplayName: &podSelectorRule1Name00, + Id: &podSelectorRule1IDPort000, + DestinationGroups: []string{"ANY"}, + Direction: &nsxRuleDirectionIn, + Scope: []string{"ANY"}, + SequenceNumber: &seq1, + Services: []string{"ANY"}, + SourceGroups: []string{"/orgs/default/projects/projectQuality/infra/domains/default/groups/spA_uidA_1_src"}, + Action: &nsxRuleActionAllow, + ServiceEntries: []*data.StructValue{serviceEntry}, + Tags: vpcBasicTags, + }, + }, + Tags: vpcBasicTags, + }, + }, + { + name: "security-policy-with-VM-selector For VPC", + inputPolicy: &spWithVMSelector, + expectedPolicy: &model.SecurityPolicy{ + DisplayName: common.String("spB"), + Id: common.String("spB_uidB"), + Scope: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/spB_uidB_scope"}, + SequenceNumber: &seq0, + Rules: []model.Rule{ + { + DisplayName: &vmSelectorRule0Name00, + Id: &vmSelectorRule0IDPort000, + DestinationGroups: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/spB_uidB_0_dst"}, + Direction: &nsxRuleDirectionOut, + Scope: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/spB_uidB_0_scope"}, + SequenceNumber: &seq0, + Services: []string{"ANY"}, + SourceGroups: []string{"ANY"}, + Action: &nsxRuleActionDrop, + Tags: vpcBasicTagsForSpWithVMSelector, + }, + { + DisplayName: &vmSelectorRule1Name00, + Id: &vmSelectorRule1IDPort000, + DestinationGroups: []string{"/orgs/default/projects/projectQuality/infra/domains/default/groups/spB_uidB_1_dst"}, + Direction: &nsxRuleDirectionOut, + Scope: []string{"ANY"}, + SequenceNumber: &seq1, + Services: []string{"ANY"}, + SourceGroups: []string{"ANY"}, + Action: &nsxRuleActionDrop, + Tags: vpcBasicTagsForSpWithVMSelector, + }, + + { + DisplayName: &vmSelectorRule2Name00, + Id: &vmSelectorRule2IDPort000, + DestinationGroups: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/spB_uidB_2_dst"}, + Direction: &nsxRuleDirectionOut, + Scope: []string{"ANY"}, + SequenceNumber: &seq2, + Services: []string{"ANY"}, + SourceGroups: []string{"ANY"}, + Action: &nsxRuleActionDrop, + Tags: vpcBasicTagsForSpWithVMSelector, + }, + }, + Tags: vpcBasicTagsForSpWithVMSelector, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + observedPolicy, _, _, _ := fakeService.buildSecurityPolicy(tt.inputPolicy, common.ResourceTypeSecurityPolicy) + assert.Equal(t, tt.expectedPolicy, observedPolicy) + }) + } +} + func TestBuildPolicyGroup(t *testing.T) { tests := []struct { name string @@ -190,6 +351,9 @@ func TestBuildPolicyGroup(t *testing.T) { } func TestBuildTargetTags(t *testing.T) { + common.TagValueScopeSecurityPolicyName = common.TagScopeSecurityPolicyCRName + common.TagValueScopeSecurityPolicyUID = common.TagScopeSecurityPolicyCRUID + ruleTagID0 := service.buildRuleID(&spWithPodSelector, &spWithPodSelector.Spec.Rules[0], 0, common.ResourceTypeSecurityPolicy) tests := []struct { name string @@ -234,7 +398,7 @@ func TestBuildTargetTags(t *testing.T) { }, { Scope: &tagScopeCluster, - Tag: &cluster, + Tag: &clusterName, }, { Scope: &tagScopeNamespace, @@ -303,7 +467,7 @@ func TestBuildPeerTags(t *testing.T) { }, { Scope: &tagScopeCluster, - Tag: &cluster, + Tag: &clusterName, }, { Scope: &tagScopeNamespace, @@ -710,7 +874,7 @@ var securityPolicyWithOneNamedPort = v1alpha1.SecurityPolicy{ Spec: v1alpha1.SecurityPolicySpec{ Rules: []v1alpha1.SecurityPolicyRule{ { - Name: "TCP.http-UDP.1234.1235-ingress-allow", + Name: "TCP.http_UDP.1234.1235_ingress_allow", Action: &allowAction, Direction: &directionIn, Ports: []v1alpha1.SecurityPolicyPort{ @@ -739,20 +903,20 @@ func TestBuildRulePortsString(t *testing.T) { { name: "build-string-for-multiple-ports-without-named-port", inputPorts: &securityPolicyWithMultipleNormalPorts.Spec.Rules[0].Ports, - suffix: "ingress-allow", - expectedRulePortsString: "TCP.80-UDP.1234.1235-ingress-allow", + 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, - suffix: "ingress-allow", - expectedRulePortsString: "TCP.http-UDP.1234.1235-ingress-allow", + suffix: "ingress_allow", + expectedRulePortsString: "TCP.http_UDP.1234.1235_ingress_allow", }, { name: "build-string-for-nil-ports", inputPorts: nil, - suffix: "ingress-allow", - expectedRulePortsString: "all-ingress-allow", + suffix: "ingress_allow", + expectedRulePortsString: "all_ingress_allow", }, } for _, tt := range tests { @@ -780,7 +944,7 @@ func TestBuildRuleDisplayName(t *testing.T) { ruleIdx: 0, portIdx: 0, createdFor: common.ResourceTypeNetworkPolicy, - expectedRuleDisplayName: "TCP.80-UDP.1234.1235-ingress-allow", + expectedRuleDisplayName: "TCP.80_UDP.1234.1235_ingress_allow", }, { name: "build-display-name-for-multiple-ports-1", @@ -798,7 +962,7 @@ func TestBuildRuleDisplayName(t *testing.T) { ruleIdx: 1, portIdx: 0, createdFor: common.ResourceTypeSecurityPolicy, - expectedRuleDisplayName: "MultipleNormalPorts-rule1-egress-isolation", + expectedRuleDisplayName: "MultipleNormalPorts-rule1_egress_isolation", }, } for _, tt := range tests { @@ -840,7 +1004,7 @@ func TestBuildSecurityPolicyName(t *testing.T) { }, }, createdFor: common.ResourceTypeSecurityPolicy, - expName: "sp-ns1-securitypolicy1", + expName: "sp_ns1_securitypolicy1", expId: "sp_uid1", }, { @@ -855,7 +1019,7 @@ func TestBuildSecurityPolicyName(t *testing.T) { }, createdFor: common.ResourceTypeSecurityPolicy, expName: "securitypolicy2", - expId: "securitypolicy2-uid2", + expId: "securitypolicy2_uid2", }, { name: "NetworkPolicy with VPC enabled", @@ -869,7 +1033,7 @@ func TestBuildSecurityPolicyName(t *testing.T) { }, createdFor: common.ResourceTypeNetworkPolicy, expName: "networkpolicy1", - expId: "networkpolicy1-uid3", + expId: "networkpolicy1_uid3", }, { name: "NetworkPolicy with VPC enabled with name truncated", @@ -882,8 +1046,8 @@ func TestBuildSecurityPolicyName(t *testing.T) { }, }, createdFor: common.ResourceTypeNetworkPolicy, - expName: fmt.Sprintf("%s-c64163f0", strings.Repeat("a", 246)), - expId: fmt.Sprintf("%s-fb85d834", strings.Repeat("a", 246)), + expName: fmt.Sprintf("%s_c64163f0", strings.Repeat("a", 246)), + expId: fmt.Sprintf("%s_fb85d834", strings.Repeat("a", 246)), }, } { t.Run(tc.name, func(t *testing.T) { @@ -933,23 +1097,23 @@ func TestBuildGroupName(t *testing.T) { ruleIdx: 0, isSource: true, enableVPC: true, - expName: "sp1-0-src", - expId: "sp1-c5db1800-ce4c-11de-bedc-84a0de00c35b_0_src", + expName: "sp1_0_src", + expId: "sp1_c5db1800-ce4c-11de-bedc-84a0de00c35b_0_src", }, { name: "dst rule without name", ruleIdx: 0, isSource: false, enableVPC: true, - expName: "sp1-0-dst", - expId: "sp1-c5db1800-ce4c-11de-bedc-84a0de00c35b_0_dst", + expName: "sp1_0_dst", + expId: "sp1_c5db1800-ce4c-11de-bedc-84a0de00c35b_0_dst", }, { name: "dst rule without name with T1", ruleIdx: 0, isSource: false, enableVPC: false, - expName: "sp1-0-dst", + expName: "sp1_0_dst", expId: "sp_c5db1800-ce4c-11de-bedc-84a0de00c35b_0_dst", }, { @@ -957,23 +1121,23 @@ func TestBuildGroupName(t *testing.T) { ruleIdx: 1, isSource: true, enableVPC: true, - expName: "MultipleNormalPorts-rule1-src", - expId: "sp1-c5db1800-ce4c-11de-bedc-84a0de00c35b_1_src", + expName: "MultipleNormalPorts-rule1_src", + expId: "sp1_c5db1800-ce4c-11de-bedc-84a0de00c35b_1_src", }, { name: "dst rule with name", ruleIdx: 1, isSource: false, enableVPC: true, - expName: "MultipleNormalPorts-rule1-dst", - expId: "sp1-c5db1800-ce4c-11de-bedc-84a0de00c35b_1_dst", + expName: "MultipleNormalPorts-rule1_dst", + expId: "sp1_c5db1800-ce4c-11de-bedc-84a0de00c35b_1_dst", }, { name: "dst rule with name with T1", ruleIdx: 1, isSource: false, enableVPC: false, - expName: "MultipleNormalPorts-rule1-dst", + expName: "MultipleNormalPorts-rule1_dst", expId: "sp_c5db1800-ce4c-11de-bedc-84a0de00c35b_1_dst", }, } { @@ -1000,28 +1164,28 @@ func TestBuildGroupName(t *testing.T) { name: "rule without name", ruleIdx: 0, enableVPC: true, - expName: "sp1-0-scope", - expId: "sp1-c5db1800-ce4c-11de-bedc-84a0de00c35b_0_scope", + expName: "sp1_0_scope", + expId: "sp1_c5db1800-ce4c-11de-bedc-84a0de00c35b_0_scope", }, { name: "rule with name", ruleIdx: 1, enableVPC: true, - expName: "MultipleNormalPorts-rule1-scope", - expId: "sp1-c5db1800-ce4c-11de-bedc-84a0de00c35b_1_scope", + expName: "MultipleNormalPorts-rule1_scope", + expId: "sp1_c5db1800-ce4c-11de-bedc-84a0de00c35b_1_scope", }, { name: "policy applied group", ruleIdx: -1, enableVPC: true, - expName: "ns1-sp1-scope", - expId: "sp1-c5db1800-ce4c-11de-bedc-84a0de00c35b_scope", + expName: "ns1_sp1_scope", + expId: "sp1_c5db1800-ce4c-11de-bedc-84a0de00c35b_scope", }, { name: "policy applied group with T1", ruleIdx: -1, enableVPC: false, - expName: "ns1-sp1-scope", + expName: "ns1_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 b2ae52685..91ee24139 100644 --- a/pkg/nsx/services/securitypolicy/expand.go +++ b/pkg/nsx/services/securitypolicy/expand.go @@ -136,7 +136,7 @@ func (service *SecurityPolicyService) expandRuleByService(obj *v1alpha1.Security ruleIPSetGroup := service.buildRuleIPSetGroup(obj, rule, nsxRule, portAddress.IPs, ruleIdx, createdFor) // In VPC network, NSGroup with IPAddressExpression type can be supported in VPC level as well. - IPSetGroupPath, err := service.buildRuleIPSetGroupPath(obj, nsxRule, false) + IPSetGroupPath, err := service.buildRuleIPSetGroupPath(obj, nsxRule) if err != nil { return nil, nil, err } @@ -216,7 +216,7 @@ func (service *SecurityPolicyService) buildRuleIPSetGroupName(ruleModel *model.R return util.GenerateTruncName(common.MaxNameLength, *ruleModel.DisplayName, "", common.IpSetGroupSuffix, "", "") } -func (service *SecurityPolicyService) buildRuleIPSetGroupPath(obj *v1alpha1.SecurityPolicy, ruleModel *model.Rule, groupShared bool) (string, error) { +func (service *SecurityPolicyService) buildRuleIPSetGroupPath(obj *v1alpha1.SecurityPolicy, ruleModel *model.Rule) (string, error) { ipSetGroupID := service.buildRuleIPSetGroupID(ruleModel) if IsVPCEnabled(service) { @@ -224,14 +224,10 @@ func (service *SecurityPolicyService) buildRuleIPSetGroupPath(obj *v1alpha1.Secu if err != nil { return "", err } - orgId := (*vpcInfo).OrgID - projectId := (*vpcInfo).ProjectID - vpcId := (*vpcInfo).VPCID - - if groupShared { - return fmt.Sprintf("/orgs/%s/projects/%s/infra/domains/%s/groups/%s", orgId, projectId, getVPCProjectDomain(), ipSetGroupID), nil - } - return fmt.Sprintf("/orgs/%s/projects/%s/vpcs/%s/groups/%s", orgId, projectId, vpcId, ipSetGroupID), nil + orgID := (*vpcInfo).OrgID + projectID := (*vpcInfo).ProjectID + vpcID := (*vpcInfo).VPCID + return fmt.Sprintf("/orgs/%s/projects/%s/vpcs/%s/groups/%s", orgID, projectID, vpcID, ipSetGroupID), nil } return fmt.Sprintf("/infra/domains/%s/groups/%s", getDomain(service), ipSetGroupID), nil diff --git a/pkg/nsx/services/securitypolicy/expand_test.go b/pkg/nsx/services/securitypolicy/expand_test.go index d225ac42b..7f30e60ac 100644 --- a/pkg/nsx/services/securitypolicy/expand_test.go +++ b/pkg/nsx/services/securitypolicy/expand_test.go @@ -31,19 +31,19 @@ func TestSecurityPolicyService_buildRuleIPGroup(t *testing.T) { DisplayName: &ruleNameWithPodSelector00, Id: &ruleIDPort000, DestinationGroups: []string{"ANY"}, - Direction: &nsxDirectionIn, + Direction: &nsxRuleDirectionIn, Scope: []string{"/infra/domains/k8scl-one/groups/sp_uidA_0_scope"}, SequenceNumber: &seq0, Services: []string{"ANY"}, SourceGroups: []string{"/infra/domains/k8scl-one/groups/sp_uidA_0_src"}, - Action: &nsxActionAllow, + Action: &nsxRuleActionAllow, ServiceEntries: []*data.StructValue{}, Tags: basicTags, } ips := []string{"1.1.1.1", "2.2.2.2"} policyGroupID := fmt.Sprintf("%s_ipset", ruleIDPort000) - policyGroupName := fmt.Sprintf("%s-ipset", ruleNameWithPodSelector00) + policyGroupName := fmt.Sprintf("%s_ipset", ruleNameWithPodSelector00) addresses := data.NewListValue() for _, ip := range ips { addresses.Add(data.NewStringValue(ip)) diff --git a/pkg/nsx/services/securitypolicy/firewall.go b/pkg/nsx/services/securitypolicy/firewall.go index 10672f84f..3d06ad578 100644 --- a/pkg/nsx/services/securitypolicy/firewall.go +++ b/pkg/nsx/services/securitypolicy/firewall.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "os" + "strings" "sync" "github.com/vmware/vsphere-automation-sdk-go/runtime/data" @@ -286,14 +287,14 @@ func (service *SecurityPolicyService) populateRulesForIsolationSection(spIsolati spIsolation.Spec.Rules = append(spIsolation.Spec.Rules, v1alpha1.SecurityPolicyRule{ Action: &actionDrop, Direction: &directionIn, - Name: "ingress-isolation", + Name: strings.Join([]string{common.RuleIngress, common.RuleActionDrop}, common.ConnectorUnderline), }) } else if policyType == networkingv1.PolicyTypeEgress { // Generating egress deny rule in isolation section. spIsolation.Spec.Rules = append(spIsolation.Spec.Rules, v1alpha1.SecurityPolicyRule{ Action: &actionDrop, Direction: &directionOut, - Name: "egress-isolation", + Name: strings.Join([]string{common.RuleEgress, common.RuleActionDrop}, common.ConnectorUnderline), }) } else { // This logic branch is impossible, leave it just for following the coding rules. @@ -304,11 +305,10 @@ func (service *SecurityPolicyService) populateRulesForIsolationSection(spIsolati } func (service *SecurityPolicyService) generateSectionForNetworkPolicy(networkPolicy *networkingv1.NetworkPolicy, sectionType string) (*v1alpha1.SecurityPolicy, error) { - name := service.BuildNetworkPolicyAllowPolicyName(networkPolicy.Name) + name := networkPolicy.Name uid := types.UID(service.BuildNetworkPolicyAllowPolicyID(string(networkPolicy.UID))) priority := common.PriorityNetworkPolicyAllowRule - if sectionType == "isolation" { - name = service.BuildNetworkPolicyIsolationPolicyName(networkPolicy.Name) + if sectionType == common.RuleActionDrop { uid = types.UID(service.BuildNetworkPolicyIsolationPolicyID(string(networkPolicy.UID))) priority = common.PriorityNetworkPolicyIsolationRule } @@ -334,7 +334,7 @@ func (service *SecurityPolicyService) convertNetworkPolicyToInternalSecurityPoli securityPolicies := []*v1alpha1.SecurityPolicy{} // Generating allow section. - spAllow, err := service.generateSectionForNetworkPolicy(networkPolicy, "allow") + spAllow, err := service.generateSectionForNetworkPolicy(networkPolicy, common.RuleActionAllow) if err != nil { return nil, err } @@ -344,7 +344,7 @@ func (service *SecurityPolicyService) convertNetworkPolicyToInternalSecurityPoli } // Generating isolation section. - spIsolation, err := service.generateSectionForNetworkPolicy(networkPolicy, "isolation") + spIsolation, err := service.generateSectionForNetworkPolicy(networkPolicy, common.RuleActionDrop) if err != nil { return nil, err } @@ -597,7 +597,7 @@ func (service *SecurityPolicyService) createOrUpdateVPCSecurityPolicy(obj *v1alp return nil } -func (service *SecurityPolicyService) DeleteSecurityPolicy(obj interface{}, isVPCCleanupOrGC bool, createdFor string) error { +func (service *SecurityPolicyService) DeleteSecurityPolicy(obj interface{}, isGC, isVPCCleanup bool, createdFor string) error { var err error switch sp := obj.(type) { case *networkingv1.NetworkPolicy: @@ -605,13 +605,14 @@ func (service *SecurityPolicyService) DeleteSecurityPolicy(obj interface{}, isVP CRPolicySet.Insert(service.BuildNetworkPolicyAllowPolicyID(string(sp.UID))) CRPolicySet.Insert(service.BuildNetworkPolicyIsolationPolicyID(string(sp.UID))) for elem := range CRPolicySet { - err = service.deleteVPCSecurityPolicy(types.UID(elem), isVPCCleanupOrGC, createdFor) + err = service.deleteVPCSecurityPolicy(types.UID(elem), isGC, createdFor) } case types.UID: - if IsVPCEnabled(service) || isVPCCleanupOrGC { - err = service.deleteVPCSecurityPolicy(sp, isVPCCleanupOrGC, createdFor) + // For VPC network, SecurityPolicy normal deletion, GC deletion and cleanup + if IsVPCEnabled(service) || isVPCCleanup { + err = service.deleteVPCSecurityPolicy(sp, isGC, createdFor) } else { - // For T1 network SecurityPolicy deletion + // For T1 network, SecurityPolicy normal deletion and GC deletion err = service.deleteSecurityPolicy(sp) } } @@ -691,7 +692,7 @@ func (service *SecurityPolicyService) deleteSecurityPolicy(sp types.UID) error { return nil } -func (service *SecurityPolicyService) deleteVPCSecurityPolicy(sp types.UID, isVPCCleanupOrGC bool, createdFor string) error { +func (service *SecurityPolicyService) deleteVPCSecurityPolicy(sp types.UID, isGC bool, createdFor string) error { var nsxSecurityPolicy *model.SecurityPolicy var err error g := make([]model.Group, 0) @@ -717,7 +718,7 @@ func (service *SecurityPolicyService) deleteVPCSecurityPolicy(sp types.UID, isVP existingSecurityPolices := securityPolicyStore.GetByIndex(indexScope, string(sp)) existingNsxInfraShares := infraShareStore.GetByIndex(indexScope, string(sp)) existingNsxInfraShareGroups := infraGroupStore.GetByIndex(indexScope, string(sp)) - if isVPCCleanupOrGC && len(existingSecurityPolices) == 0 && (len(existingNsxInfraShares) != 0 || len(existingNsxInfraShareGroups) != 0) { + if isGC && len(existingSecurityPolices) == 0 && (len(existingNsxInfraShares) != 0 || len(existingNsxInfraShareGroups) != 0) { // There is a specific case that needs to be handle in GC process, that is, // When the NSX security policy, rules, and groups at the VPC level are deleted, // The following infra API call to delete infra share resources fail or NSX Operator restarts suddenly. @@ -732,7 +733,7 @@ func (service *SecurityPolicyService) deleteVPCSecurityPolicy(sp types.UID, isVP nsxSecurityPolicy = existingSecurityPolices[0] // vpcInfo should be listed directly from security policy store to avoid calling VPC service. - // Get orgId, projectId, vpcId from security policy path "/orgs//projects//vpcs//security-policies/" + // Get orgID, projectID, vpcID from security policy path "/orgs//projects//vpcs//security-policies/" if nsxSecurityPolicy.Path == nil { err = errors.New("nsxSecurityPolicy path is empty") log.Error(err, "failed to delete SecurityPolicy in VPC", "nsxSecurityPolicyUID", sp) @@ -804,11 +805,11 @@ func (service *SecurityPolicyService) createOrUpdateGroups(obj *v1alpha1.Securit if err != nil { return err } - orgId := (*vpcInfo).OrgID - projectId := (*vpcInfo).ProjectID - vpcId := (*vpcInfo).VPCID + orgID := (*vpcInfo).OrgID + projectID := (*vpcInfo).ProjectID + vpcID := (*vpcInfo).VPCID - err = service.NSXClient.VpcGroupClient.Patch(orgId, projectId, vpcId, *group.Id, *group) + err = service.NSXClient.VpcGroupClient.Patch(orgID, projectID, vpcID, *group.Id, *group) err = nsxutil.TransNSXApiError(err) } else { err = service.NSXClient.GroupClient.Patch(getDomain(service), *group.Id, *group) @@ -1112,7 +1113,7 @@ func (service *SecurityPolicyService) Cleanup(ctx context.Context) error { case <-ctx.Done(): return errors.Join(nsxutil.TimeoutFailed, ctx.Err()) default: - err := service.DeleteSecurityPolicy(types.UID(uid), true, common.ResourceTypeSecurityPolicy) + err := service.DeleteSecurityPolicy(types.UID(uid), false, true, common.ResourceTypeSecurityPolicy) if err != nil { return err } @@ -1127,7 +1128,7 @@ func (service *SecurityPolicyService) Cleanup(ctx context.Context) error { case <-ctx.Done(): return errors.Join(nsxutil.TimeoutFailed, ctx.Err()) default: - err := service.DeleteSecurityPolicy(types.UID(uid), true, common.ResourceTypeNetworkPolicy) + err := service.DeleteSecurityPolicy(types.UID(uid), false, true, common.ResourceTypeNetworkPolicy) if err != nil { return err } diff --git a/pkg/nsx/services/securitypolicy/firewall_test.go b/pkg/nsx/services/securitypolicy/firewall_test.go index 64ca7f129..85969bc3f 100644 --- a/pkg/nsx/services/securitypolicy/firewall_test.go +++ b/pkg/nsx/services/securitypolicy/firewall_test.go @@ -11,8 +11,10 @@ import ( "github.com/agiledragon/gomonkey/v2" "github.com/stretchr/testify/assert" + "github.com/vmware/vsphere-automation-sdk-go/runtime/data" "github.com/vmware/vsphere-automation-sdk-go/services/nsxt/model" corev1 "k8s.io/api/core/v1" + networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" @@ -36,10 +38,12 @@ var ( tagScopeNamespaceUID = common.TagScopeNamespaceUID tagScopeSecurityPolicyCRName = common.TagValueScopeSecurityPolicyName tagScopeSecurityPolicyCRUID = common.TagValueScopeSecurityPolicyUID + tagScopeSecurityPolicyName = common.TagScopeSecurityPolicyName + tagScopeSecurityPolicyUID = common.TagScopeSecurityPolicyUID tagScopeRuleID = common.TagScopeRuleID tagScopeSelectorHash = common.TagScopeSelectorHash - spName = "sp-ns1-spA" - spGroupName = "ns1-spA-scope" + spName = "sp_ns1_spA" + spGroupName = "ns1_spA_scope" spID = "sp_uidA" spID2 = "sp_uidB" spGroupID = "sp_uidA_scope" @@ -53,11 +57,11 @@ var ( ruleID1 = "sp_uidA_1" ruleIDPort000 = "sp_uidA_0_0_0" ruleIDPort100 = "sp_uidA_1_0_0" - nsxDirectionIn = "IN" - nsxActionAllow = "ALLOW" - nsxDirectionOut = "OUT" - nsxActionDrop = "DROP" - cluster = "k8scl-one" + nsxRuleDirectionIn = "IN" + nsxRuleActionAllow = "ALLOW" + nsxRuleDirectionOut = "OUT" + nsxRuleActionDrop = "DROP" + clusterName = "k8scl-one" tagValueVersion = strings.Join(common.TagValueVersion, ".") tagValueGroupScope = common.TagValueGroupScope tagValueGroupSource = common.TagValueGroupSource @@ -143,7 +147,7 @@ var ( { Action: &allowAction, Direction: &directionIn, - Name: "rule-with-pod-selector", + Name: "rule-with-pod-ns-selector", AppliedTo: []v1alpha1.SecurityPolicyTarget{ { PodSelector: &metav1.LabelSelector{ @@ -190,7 +194,7 @@ var ( } spWithVMSelector = v1alpha1.SecurityPolicy{ - ObjectMeta: metav1.ObjectMeta{Namespace: "ns1", Name: "spA", UID: "uidA"}, + ObjectMeta: metav1.ObjectMeta{Namespace: "ns1", Name: "spB", UID: "uidB"}, Spec: v1alpha1.SecurityPolicySpec{ AppliedTo: []v1alpha1.SecurityPolicyTarget{ { @@ -237,7 +241,6 @@ var ( { Action: &allowDrop, Direction: &directionOut, - Name: "rule-with-ip-block", Destinations: []v1alpha1.SecurityPolicyPeer{ { IPBlocks: []v1alpha1.IPBlock{ @@ -251,10 +254,37 @@ var ( }, } + basicTagsForSpWithVMSelector = []model.Tag{ + { + Scope: &tagScopeCluster, + Tag: &clusterName, + }, + { + Scope: &tagScopeVersion, + Tag: &tagValueVersion, + }, + { + Scope: &tagScopeNamespace, + Tag: &tagValueNS, + }, + { + Scope: &tagScopeNamespaceUID, + Tag: &tagValueNSUID, + }, + { + Scope: &tagScopeSecurityPolicyCRName, + Tag: String(spWithVMSelector.Name), + }, + { + Scope: &tagScopeSecurityPolicyCRUID, + Tag: String(string(spWithVMSelector.UID)), + }, + } + basicTags = []model.Tag{ { Scope: &tagScopeCluster, - Tag: &cluster, + Tag: &clusterName, }, { Scope: &tagScopeVersion, @@ -277,6 +307,165 @@ var ( Tag: &tagValuePolicyCRUID, }, } + + vpcBasicTags = []model.Tag{ + { + Scope: &tagScopeCluster, + Tag: &clusterName, + }, + { + Scope: &tagScopeVersion, + Tag: &tagValueVersion, + }, + { + Scope: &tagScopeNamespace, + Tag: &tagValueNS, + }, + { + Scope: &tagScopeNamespaceUID, + Tag: &tagValueNSUID, + }, + { + Scope: &tagScopeSecurityPolicyName, + Tag: &tagValuePolicyCRName, + }, + { + Scope: &tagScopeSecurityPolicyUID, + Tag: &tagValuePolicyCRUID, + }, + } + + vpcBasicTagsForSpWithVMSelector = []model.Tag{ + { + Scope: &tagScopeCluster, + Tag: &clusterName, + }, + { + Scope: &tagScopeVersion, + Tag: &tagValueVersion, + }, + { + Scope: &tagScopeNamespace, + Tag: &tagValueNS, + }, + { + Scope: &tagScopeNamespaceUID, + Tag: &tagValueNSUID, + }, + { + Scope: &tagScopeSecurityPolicyName, + Tag: String(spWithVMSelector.Name), + }, + { + Scope: &tagScopeSecurityPolicyUID, + Tag: String(string(spWithVMSelector.UID)), + }, + } + + // Create the NetworkPolicy object + npWithNsSelecotr = networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "np-app-access", + Namespace: "ns1", + UID: "uidNP", + }, + + Spec: networkingv1.NetworkPolicySpec{ + PolicyTypes: []networkingv1.PolicyType{ + networkingv1.PolicyTypeIngress, + }, + PodSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "role": "db", + }, + }, + Ingress: []networkingv1.NetworkPolicyIngressRule{ + { + From: []networkingv1.NetworkPolicyPeer{ + { + PodSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "coffee", + }, + }, + }, + { + NamespaceSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "ns-name": "ns-3", + }, + }, + }, + }, + Ports: []networkingv1.NetworkPolicyPort{ + { + Protocol: func() *corev1.Protocol { + proto := corev1.ProtocolTCP + return &proto + }(), + Port: &intstr.IntOrString{ + IntVal: 6001, + }, + }, + }, + }, + }, + }, + } + + npAllowBasicTags = []model.Tag{ + { + Scope: &tagScopeCluster, + Tag: &clusterName, + }, + { + Scope: &tagScopeVersion, + Tag: &tagValueVersion, + }, + { + Scope: &tagScopeNamespace, + Tag: &tagValueNS, + }, + { + Scope: &tagScopeNamespaceUID, + Tag: &tagValueNSUID, + }, + { + Scope: common.String(common.TagScopeNetworkPolicyName), + Tag: String(npWithNsSelecotr.Name), + }, + { + Scope: common.String(common.TagScopeNetworkPolicyUID), + Tag: String(string(npWithNsSelecotr.UID + "_allow")), + }, + } + + npIsolationBasicTags = []model.Tag{ + { + Scope: &tagScopeCluster, + Tag: &clusterName, + }, + { + Scope: &tagScopeVersion, + Tag: &tagValueVersion, + }, + { + Scope: &tagScopeNamespace, + Tag: &tagValueNS, + }, + { + Scope: &tagScopeNamespaceUID, + Tag: &tagValueNSUID, + }, + { + Scope: common.String(common.TagScopeNetworkPolicyName), + Tag: String(npWithNsSelecotr.Name), + }, + { + Scope: common.String(common.TagScopeNetworkPolicyUID), + Tag: String(string(npWithNsSelecotr.UID + "_isolation")), + }, + } ) func TestListSecurityPolicyID(t *testing.T) { @@ -373,12 +562,12 @@ func TestGetUpdateRules(t *testing.T) { DisplayName: String("nsxrule1"), Id: String("nsxrule_1"), DestinationGroups: []string{"ANY"}, - Direction: &nsxDirectionIn, + Direction: &nsxRuleDirectionIn, Scope: []string{"/infra/domains/k8scl-one/groups/sp_uidA_0_scope"}, SequenceNumber: &seq0, Services: []string{"ANY"}, SourceGroups: []string{"/infra/domains/k8scl-one/groups/sp_uidA_0_src"}, - Action: &nsxActionAllow, + Action: &nsxRuleActionAllow, } tests := []struct { @@ -397,12 +586,12 @@ func TestGetUpdateRules(t *testing.T) { DisplayName: String("nsxrule1"), Id: String("nsxrule_1"), DestinationGroups: []string{"ANY"}, - Direction: &nsxDirectionIn, + Direction: &nsxRuleDirectionIn, Scope: []string{"/infra/domains/k8scl-one/groups/sp_uidA_0_scope"}, SequenceNumber: &seq0, Services: []string{"ANY"}, SourceGroups: []string{"/infra/domains/k8scl-one/groups/sp_uidA_0_src"}, - Action: &nsxActionAllow, + Action: &nsxRuleActionAllow, }, }, finalRulesLen: 0, @@ -417,12 +606,12 @@ func TestGetUpdateRules(t *testing.T) { DisplayName: String("nsxrule1"), Id: String("nsxrule_1"), DestinationGroups: []string{"ANY"}, - Direction: &nsxDirectionIn, + Direction: &nsxRuleDirectionIn, Scope: []string{"/infra/domains/k8scl-one/groups/sp_uidA_1_scope"}, SequenceNumber: &seq0, Services: []string{"ANY"}, SourceGroups: []string{"/infra/domains/k8scl-one/groups/sp_uidA_0_src"}, - Action: &nsxActionAllow, + Action: &nsxRuleActionAllow, }, }, finalRulesLen: 1, @@ -582,12 +771,12 @@ func TestListMarkDeleteRules(t *testing.T) { DisplayName: String("nsxrule1"), Id: String("nsxrule_1"), DestinationGroups: []string{"ANY"}, - Direction: &nsxDirectionIn, + Direction: &nsxRuleDirectionIn, Scope: []string{"/infra/domains/k8scl-one/groups/sp_uidA_0_scope"}, SequenceNumber: &seq0, Services: []string{"ANY"}, SourceGroups: []string{"/infra/domains/k8scl-one/groups/sp_uidA_0_src"}, - Action: &nsxActionAllow, + Action: &nsxRuleActionAllow, MarkedForDelete: &markNoDelete, } @@ -764,24 +953,24 @@ func TestDleleteVPCSecurityPolicy(t *testing.T) { DisplayName: &ruleNameWithPodSelector00, Id: &ruleID0, DestinationGroups: []string{"ANY"}, - Direction: &nsxDirectionIn, + Direction: &nsxRuleDirectionIn, Scope: []string{"ANY"}, SequenceNumber: &seq0, Services: []string{"ANY"}, SourceGroups: []string{"ANY"}, - Action: &nsxActionAllow, + Action: &nsxRuleActionAllow, Tags: basicTags, }, { DisplayName: &ruleNameWithNsSelector00, Id: &ruleID1, DestinationGroups: []string{"ANY"}, - Direction: &nsxDirectionIn, + Direction: &nsxRuleDirectionIn, Scope: []string{"ANY"}, SequenceNumber: &seq1, Services: []string{"ANY"}, SourceGroups: []string{"/orgs/default/projects/projectQuality/infra/domains/default/groups/sp_uidA_1_src"}, - Action: &nsxActionAllow, + Action: &nsxRuleActionAllow, Tags: basicTags, }, }, @@ -829,12 +1018,12 @@ func TestDleleteVPCSecurityPolicy(t *testing.T) { DisplayName: &ruleNameWithPodSelector00, Id: &ruleID0, DestinationGroups: []string{"ANY"}, - Direction: &nsxDirectionIn, + Direction: &nsxRuleDirectionIn, Scope: []string{"ANY"}, SequenceNumber: &seq0, Services: []string{"ANY"}, SourceGroups: []string{"ANY"}, - Action: &nsxActionAllow, + Action: &nsxRuleActionAllow, Tags: basicTags, }, }, @@ -933,31 +1122,27 @@ func TestCreateOrUpdateSecurityPolicy(t *testing.T) { expectedPolicy: &model.SecurityPolicy{ DisplayName: &spName, Id: &spID, - Scope: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/sp_uidA_scope"}, SequenceNumber: &seq0, Rules: []model.Rule{ { DisplayName: &podSelectorRule0Name00, Id: &podSelectorRule0IDPort000, DestinationGroups: []string{"ANY"}, - Direction: &nsxDirectionIn, - Scope: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/sp_uidA_0_scope"}, + Direction: &nsxRuleDirectionIn, SequenceNumber: &seq0, Services: []string{"ANY"}, - SourceGroups: []string{"/orgs/default/projects/projectQuality/infra/domains/default/groups/sp_uidA_0_src"}, - Action: &nsxActionAllow, + Action: &nsxRuleActionAllow, Tags: basicTags, }, { DisplayName: &podSelectorRule1Name00, Id: &podSelectorRule1IDPort000, DestinationGroups: []string{"ANY"}, - Direction: &nsxDirectionIn, + Direction: &nsxRuleDirectionIn, Scope: []string{"ANY"}, SequenceNumber: &seq1, Services: []string{"ANY"}, - SourceGroups: []string{"/orgs/default/projects/projectQuality/infra/domains/default/groups/sp_uidA_1_src"}, - Action: &nsxActionAllow, + Action: &nsxRuleActionAllow, Tags: basicTags, }, }, @@ -1077,8 +1262,8 @@ func TestGetFinalSecurityPolicyResouce(t *testing.T) { }, expectedPolicy: &model.SecurityPolicy{ DisplayName: common.String("spA"), - Id: common.String("spA-uidA"), - Scope: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/spA-uidA_scope"}, + Id: common.String("spA_uidA"), + Scope: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/spA_uidA_scope"}, SequenceNumber: &seq0, Rules: []model.Rule{}, Tags: basicTags, @@ -1114,7 +1299,7 @@ func TestGetFinalSecurityPolicyResouce(t *testing.T) { expectedPolicy: &model.SecurityPolicy{ DisplayName: &spName, Id: &spID, - Scope: []string{"/infra/domains/k8scl-one:test/groups/sp_uidA_scope"}, + Scope: []string{"/infra/domains/k8scl-one/groups/sp_uidA_scope"}, SequenceNumber: &seq0, Rules: []model.Rule{}, Tags: basicTags, @@ -1161,3 +1346,208 @@ func TestGetFinalSecurityPolicyResouce(t *testing.T) { }) } } + +func TestConvertNetworkPolicyToInternalSecurityPolicies(t *testing.T) { + VPCInfo := make([]common.VPCResourceInfo, 1) + VPCInfo[0].OrgID = "default" + VPCInfo[0].ProjectID = "projectQuality" + VPCInfo[0].VPCID = "vpc1" + + fakeService := fakeSecurityPolicyService() + fakeService.NSXConfig.EnableVPCNetwork = true + mockVPCService := common.MockVPCServiceProvider{} + fakeService.vpcService = &mockVPCService + + tests := []struct { + name string + inputPolicy *networkingv1.NetworkPolicy + expPolicyAllowSection *v1alpha1.SecurityPolicy + expPolicyIsolationSection *v1alpha1.SecurityPolicy + }{ + { + name: "Convert NetworkPolicy", + inputPolicy: &npWithNsSelecotr, + expPolicyAllowSection: &v1alpha1.SecurityPolicy{ + ObjectMeta: metav1.ObjectMeta{Namespace: "ns1", Name: "np-app-access", UID: "uidNP_allow"}, + Spec: v1alpha1.SecurityPolicySpec{ + AppliedTo: []v1alpha1.SecurityPolicyTarget{ + { + PodSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"role": "db"}, + }, + }, + }, + Rules: []v1alpha1.SecurityPolicyRule{ + { + Action: &allowAction, + Direction: &directionIn, + Sources: []v1alpha1.SecurityPolicyPeer{ + { + PodSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "coffee"}, + }, + }, + { + PodSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{}, + }, + NamespaceSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"ns-name": "ns-3"}, + }, + }, + }, + Ports: []v1alpha1.SecurityPolicyPort{ + { + Protocol: corev1.ProtocolTCP, + Port: intstr.IntOrString{Type: intstr.Int, IntVal: 6001}, + }, + }, + }, + }, + Priority: common.PriorityNetworkPolicyAllowRule, + }, + }, + expPolicyIsolationSection: &v1alpha1.SecurityPolicy{ + ObjectMeta: metav1.ObjectMeta{Namespace: "ns1", Name: "np-app-access", UID: "uidNP_isolation"}, + Spec: v1alpha1.SecurityPolicySpec{ + AppliedTo: []v1alpha1.SecurityPolicyTarget{ + { + PodSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"role": "db"}, + }, + }, + }, + Rules: []v1alpha1.SecurityPolicyRule{ + { + Action: &allowDrop, + Direction: &directionIn, + Name: "ingress_isolation", + }, + }, + Priority: common.PriorityNetworkPolicyIsolationRule, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + observedPolicy, err := fakeService.convertNetworkPolicyToInternalSecurityPolicies(tt.inputPolicy) + assert.Equal(t, nil, err) + assert.Equal(t, tt.expPolicyAllowSection, observedPolicy[0]) + assert.Equal(t, tt.expPolicyIsolationSection, observedPolicy[1]) + }) + } +} + +func TestGetFinalSecurityPolicyResouceFromNetworkPolicy(t *testing.T) { + VPCInfo := make([]common.VPCResourceInfo, 1) + VPCInfo[0].OrgID = "default" + VPCInfo[0].ProjectID = "projectQuality" + VPCInfo[0].VPCID = "vpc1" + + fakeService := fakeSecurityPolicyService() + fakeService.NSXConfig.EnableVPCNetwork = true + mockVPCService := common.MockVPCServiceProvider{} + fakeService.vpcService = &mockVPCService + + destinationPorts := data.NewListValue() + destinationPorts.Add(data.NewStringValue("6001")) + serviceEntry := data.NewStructValue( + "", + map[string]data.DataValue{ + "source_ports": data.NewListValue(), + "destination_ports": destinationPorts, + "l4_protocol": data.NewStringValue("TCP"), + "resource_type": data.NewStringValue("L4PortSetServiceEntry"), + "marked_for_delete": data.NewBooleanValue(false), + "overridden": data.NewBooleanValue(false), + }, + ) + + patches := gomonkey.ApplyPrivateMethod(reflect.TypeOf(fakeService), "getVPCInfo", + func(s *SecurityPolicyService, spNameSpace string) (*common.VPCResourceInfo, error) { + return &VPCInfo[0], nil + }) + + patches.ApplyPrivateMethod(reflect.TypeOf(fakeService), "getNamespaceUID", + func(s *SecurityPolicyService, ns string) types.UID { + return types.UID(tagValueNSUID) + }) + + defer patches.Reset() + + tests := []struct { + name string + inputPolicy *networkingv1.NetworkPolicy + expAllowPolicy *model.SecurityPolicy + expIsolationPolicy *model.SecurityPolicy + }{ + { + name: "Get SecurityPolicy from NetworkPolicy", + inputPolicy: &npWithNsSelecotr, + expAllowPolicy: &model.SecurityPolicy{ + DisplayName: common.String("np-app-access"), + Id: common.String("np-app-access_uidNP_allow"), + Scope: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/np-app-access_uidNP_allow_scope"}, + SequenceNumber: Int64(int64(common.PriorityNetworkPolicyAllowRule)), + Rules: []model.Rule{ + { + DisplayName: common.String("TCP.6001_ingress_allow"), + Id: common.String("np-app-access_uidNP_allow_0_6c2a026ca143812daa72699fb924ee36b33b5cdc_0_0"), + 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"}, + Action: &nsxRuleActionAllow, + ServiceEntries: []*data.StructValue{serviceEntry}, + Tags: npAllowBasicTags, + }, + }, + Tags: npAllowBasicTags, + }, + expIsolationPolicy: &model.SecurityPolicy{ + DisplayName: common.String("np-app-access"), + Id: common.String("np-app-access_uidNP_isolation"), + Scope: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/np-app-access_uidNP_isolation_scope"}, + SequenceNumber: Int64(int64(common.PriorityNetworkPolicyIsolationRule)), + Rules: []model.Rule{ + { + DisplayName: common.String("ingress_isolation"), + Id: common.String("np-app-access_uidNP_isolation_0_114fed106ef3b5eae2a583f312435e84c02ca97f_0_0"), + DestinationGroups: []string{"ANY"}, + Direction: &nsxRuleDirectionIn, + Scope: []string{"/orgs/default/projects/projectQuality/vpcs/vpc1/groups/np-app-access_uidNP_isolation_scope"}, + SequenceNumber: &seq0, + Services: []string{"ANY"}, + SourceGroups: []string{"ANY"}, + Action: &nsxRuleActionDrop, + Tags: npIsolationBasicTags, + }, + }, + Tags: npIsolationBasicTags, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fakeService.setUpStore(common.TagScopeSecurityPolicyUID) + + convertSecurityPolicy, err := fakeService.convertNetworkPolicyToInternalSecurityPolicies(tt.inputPolicy) + assert.Equal(t, nil, err) + + finalAllowSecurityPolicy, _, _, _, _, err := fakeService.getFinalSecurityPolicyResource(convertSecurityPolicy[0], common.ResourceTypeNetworkPolicy, false) + assert.Equal(t, nil, err) + + assert.Equal(t, tt.expAllowPolicy, finalAllowSecurityPolicy) + + finalIsolationSecurityPolicy, _, _, _, _, err := fakeService.getFinalSecurityPolicyResource(convertSecurityPolicy[1], common.ResourceTypeNetworkPolicy, false) + assert.Equal(t, nil, err) + + assert.Equal(t, tt.expIsolationPolicy, finalIsolationSecurityPolicy) + }) + } +} diff --git a/pkg/nsx/services/securitypolicy/store_test.go b/pkg/nsx/services/securitypolicy/store_test.go index c3298d369..fdbbb5088 100644 --- a/pkg/nsx/services/securitypolicy/store_test.go +++ b/pkg/nsx/services/securitypolicy/store_test.go @@ -301,24 +301,24 @@ func TestRuleStore_Apply(t *testing.T) { DisplayName: &ruleNameWithPodSelector00, Id: &ruleIDPort000, DestinationGroups: []string{"ANY"}, - Direction: &nsxDirectionIn, + Direction: &nsxRuleDirectionIn, Scope: []string{"/infra/domains/k8scl-one/groups/sp_uidA_0_scope"}, SequenceNumber: &seq0, Services: []string{"ANY"}, SourceGroups: []string{"/infra/domains/k8scl-one/groups/sp_uidA_0_src"}, - Action: &nsxActionAllow, + Action: &nsxRuleActionAllow, Tags: basicTags, }, { DisplayName: &ruleNameWithNsSelector00, Id: &ruleIDPort100, DestinationGroups: []string{"ANY"}, - Direction: &nsxDirectionIn, + Direction: &nsxRuleDirectionIn, Scope: []string{"ANY"}, SequenceNumber: &seq1, Services: []string{"ANY"}, SourceGroups: []string{"/infra/domains/k8scl-one/groups/sp_uidA_1_src"}, - Action: &nsxActionAllow, + Action: &nsxRuleActionAllow, Tags: basicTags, }, }, diff --git a/pkg/nsx/services/securitypolicy/wrap_test.go b/pkg/nsx/services/securitypolicy/wrap_test.go index 86f4b54d8..d899119f2 100644 --- a/pkg/nsx/services/securitypolicy/wrap_test.go +++ b/pkg/nsx/services/securitypolicy/wrap_test.go @@ -79,31 +79,31 @@ func (f fakeOrgClient) Patch(orgRootParam model.OrgRoot, enforceRevisionCheckPar return nil } -func (f fakeVPCSecurityClient) Delete(orgIdParam string, projectIdParam string, vpcIdParam string, securityPolicyIdParam string) error { +func (f fakeVPCSecurityClient) Delete(orgIDParam string, projectIDParam string, vpcIDParam string, securityPolicyIdParam string) error { return nil } -func (f fakeVPCSecurityClient) Get(orgIdParam string, projectIdParam string, vpcIdParam string, securityPolicyIdParam string) (model.SecurityPolicy, error) { +func (f fakeVPCSecurityClient) Get(orgIDParam string, projectIDParam string, vpcIDParam string, securityPolicyIdParam string) (model.SecurityPolicy, error) { return model.SecurityPolicy{}, nil } -func (f fakeVPCSecurityClient) List(orgIdParam string, projectIdParam string, vpcIdParam string, cursorParam *string, includeMarkForDeleteObjectsParam *bool, +func (f fakeVPCSecurityClient) List(orgIDParam string, projectIDParam string, vpcIDParam string, cursorParam *string, includeMarkForDeleteObjectsParam *bool, includeRuleCountParam *bool, includedFieldsParam *string, pageSizeParam *int64, sortAscendingParam *bool, sortByParam *string, ) (model.SecurityPolicyListResult, error) { return model.SecurityPolicyListResult{}, nil } -func (f fakeVPCSecurityClient) Patch(orgIdParam string, projectIdParam string, vpcIdParam string, securityPolicyIdParam string, securityPolicyParam model.SecurityPolicy) error { +func (f fakeVPCSecurityClient) Patch(orgIDParam string, projectIDParam string, vpcIDParam string, securityPolicyIdParam string, securityPolicyParam model.SecurityPolicy) error { return nil } -func (f fakeVPCSecurityClient) Revise(orgIdParam string, projectIdParam string, vpcIdParam string, securityPolicyIdParam string, securityPolicyParam model.SecurityPolicy, +func (f fakeVPCSecurityClient) Revise(orgIDParam string, projectIDParam string, vpcIDParam string, securityPolicyIdParam string, securityPolicyParam model.SecurityPolicy, anchorPathParam *string, operationParam *string, ) (model.SecurityPolicy, error) { return model.SecurityPolicy{}, nil } -func (f fakeVPCSecurityClient) Update(orgIdParam string, projectIdParam string, vpcIdParam string, securityPolicyIdParam string, securityPolicyParam model.SecurityPolicy) (model.SecurityPolicy, error) { +func (f fakeVPCSecurityClient) Update(orgIDParam string, projectIDParam string, vpcIDParam string, securityPolicyIdParam string, securityPolicyParam model.SecurityPolicy) (model.SecurityPolicy, error) { return model.SecurityPolicy{}, nil } @@ -122,13 +122,13 @@ func fakeSecurityPolicyService() *SecurityPolicyService { RestConnector: rc, NsxConfig: &config.NSXOperatorConfig{ CoeConfig: &config.CoeConfig{ - Cluster: "k8scl-one:test", + Cluster: clusterName, }, }, }, NSXConfig: &config.NSXOperatorConfig{ CoeConfig: &config.CoeConfig{ - Cluster: "k8scl-one:test", + Cluster: clusterName, }, }, }, @@ -265,7 +265,7 @@ func TestSecurityPolicyService_wrapResourceReference(t *testing.T) { for _, v := range got { r, _ := Converter.ConvertToGolang(v, model.ChildResourceReferenceBindingType()) rc := r.(model.ChildResourceReference) - assert.Equal(t, "k8scl-one:test", *rc.Id) + assert.Equal(t, "k8scl-one", *rc.Id) assert.Equal(t, "ChildResourceReference", rc.ResourceType) assert.NotNil(t, "Domain", rc.TargetType) } diff --git a/pkg/nsx/services/staticroute/builder_test.go b/pkg/nsx/services/staticroute/builder_test.go index d0e7967d1..c7c36e65a 100644 --- a/pkg/nsx/services/staticroute/builder_test.go +++ b/pkg/nsx/services/staticroute/builder_test.go @@ -43,6 +43,6 @@ func TestBuildStaticRoute(t *testing.T) { assert.Equal(t, len(staticroutes.NextHops), 2) expName := "teststaticroute" assert.Equal(t, expName, *staticroutes.DisplayName) - expId := "teststaticroute-uuid1" + expId := "teststaticroute_uuid1" assert.Equal(t, expId, *staticroutes.Id) } diff --git a/pkg/nsx/services/staticroute/staticroute_test.go b/pkg/nsx/services/staticroute/staticroute_test.go index 5cbc4ab5c..25298df01 100644 --- a/pkg/nsx/services/staticroute/staticroute_test.go +++ b/pkg/nsx/services/staticroute/staticroute_test.go @@ -23,6 +23,7 @@ import ( "github.com/vmware-tanzu/nsx-operator/pkg/nsx/ratelimiter" "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common" "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/vpc" + "github.com/vmware-tanzu/nsx-operator/pkg/util" ) var ( @@ -40,8 +41,7 @@ var ( tagScopeNamespace = common.TagScopeNamespace ) -type fakeQueryClient struct { -} +type fakeQueryClient struct{} func (qIface *fakeQueryClient) List(queryParam string, cursorParam *string, includedFieldsParam *string, pageSizeParam *int64, sortAscendingParam *bool, sortByParam *string) (model.SearchResponse, error) { resultCount := int64(1) @@ -93,7 +93,8 @@ func Test_InitializeStaticRouteStore(t *testing.T) { defer mockController.Finish() commonService := service.Service patch := gomonkey.ApplyMethod(reflect.TypeOf(&commonService), "InitializeResourceStore", func(_ *common.Service, wg *sync.WaitGroup, - fatalErrors chan error, resourceTypeValue string, tags []model.Tag, store common.Store) { + fatalErrors chan error, resourceTypeValue string, tags []model.Tag, store common.Store, + ) { wg.Done() return }) @@ -135,7 +136,7 @@ func TestStaticRouteService_DeleteStaticRoute(t *testing.T) { Name: "sr", }, } - id := "sr-uid-123" + id := util.GenerateIDByObject(srObj) path := "/orgs/default/projects/project-1/vpcs/vpc-1" sr1 := &model.StaticRoutes{Id: &id, Path: &path} @@ -173,7 +174,6 @@ func TestStaticRouteService_CreateorUpdateStaticRoute(t *testing.T) { vpcService := &vpc.VPCService{} returnservice, err := InitializeStaticRoute(service.Service, vpcService) - if err != nil { t.Error(err) } diff --git a/pkg/nsx/services/subnet/builder.go b/pkg/nsx/services/subnet/builder.go index 7333d59fc..d4da6caff 100644 --- a/pkg/nsx/services/subnet/builder.go +++ b/pkg/nsx/services/subnet/builder.go @@ -32,14 +32,14 @@ func (service *SubnetService) buildSubnetSetID(subnetset *v1alpha1.SubnetSet, in return util.GenerateIDByObjectWithSuffix(subnetset, index) } -// buildSubnetName uses format "subnet.Name-subnet.UUID" to ensure the Subnet's display_name is not +// buildSubnetName uses format "subnet.Name_subnet.UUID" to ensure the Subnet's display_name is not // conflict with others. This is because VC will use the Subnet's display_name to created folder, so // the name string must be unique. func (service *SubnetService) buildSubnetName(subnet *v1alpha1.Subnet) string { return util.GenerateIDByObjectByLimit(subnet, common.MaxSubnetNameLength) } -// buildSubnetSetName uses format "subnetset.Name-subnetset.UUID-index" to ensure the generated Subnet's +// buildSubnetSetName uses format "subnetset.Name_subnetset.UUID_index" to ensure the generated Subnet's // display_name is not conflict with others. func (service *SubnetService) buildSubnetSetName(subnetset *v1alpha1.SubnetSet, index string) string { resName := util.GenerateIDByObjectByLimit(subnetset, common.MaxSubnetNameLength-(len(index)+1)) diff --git a/pkg/nsx/services/subnet/builder_test.go b/pkg/nsx/services/subnet/builder_test.go index f269aad28..225b277cd 100644 --- a/pkg/nsx/services/subnet/builder_test.go +++ b/pkg/nsx/services/subnet/builder_test.go @@ -28,10 +28,10 @@ func TestBuildSubnetName(t *testing.T) { }, } name := svc.buildSubnetName(subnet) - expName := "subnet1-uuid1" + expName := "subnet1_uuid1" assert.Equal(t, expName, name) id := svc.BuildSubnetID(subnet) - expId := "subnet1-uuid1" + expId := "subnet1_uuid1" assert.Equal(t, expId, id) } @@ -53,10 +53,10 @@ func TestBuildSubnetSetName(t *testing.T) { } index := "0c5d588b" name := svc.buildSubnetSetName(subnetset, index) - expName := "pod-default-28e85c0b-21e4-4cab-b1c3-597639dfe752-0c5d588b" + expName := "pod-default_28e85c0b-21e4-4cab-b1c3-597639dfe752_0c5d588b" assert.Equal(t, expName, name) assert.True(t, len(name) <= 80) id := svc.buildSubnetSetID(subnetset, index) - expId := "pod-default-28e85c0b-21e4-4cab-b1c3-597639dfe752_0c5d588b" + expId := "pod-default_28e85c0b-21e4-4cab-b1c3-597639dfe752_0c5d588b" assert.Equal(t, expId, id) } diff --git a/pkg/nsx/services/subnetport/builder_test.go b/pkg/nsx/services/subnetport/builder_test.go index b7be31503..f9406c998 100644 --- a/pkg/nsx/services/subnetport/builder_test.go +++ b/pkg/nsx/services/subnetport/builder_test.go @@ -74,7 +74,7 @@ func TestBuildSubnetPort(t *testing.T) { labelTags: nil, expectedPort: &model.VpcSubnetPort{ DisplayName: common.String("fake_subnetport"), - Id: common.String("fake_subnetport-2ccec3b9-7546-4fd2-812a-1e3a4afd7acc"), + Id: common.String("fake_subnetport_2ccec3b9-7546-4fd2-812a-1e3a4afd7acc"), Tags: []model.Tag{ { Scope: common.String("nsx-op/cluster"), @@ -97,7 +97,7 @@ func TestBuildSubnetPort(t *testing.T) { Tag: common.String("2ccec3b9-7546-4fd2-812a-1e3a4afd7acc"), }, }, - Path: common.String("fake_path/ports/fake_subnetport-2ccec3b9-7546-4fd2-812a-1e3a4afd7acc"), + Path: common.String("fake_path/ports/fake_subnetport_2ccec3b9-7546-4fd2-812a-1e3a4afd7acc"), ParentPath: common.String("fake_path"), Attachment: &model.PortAttachment{ AllocateAddresses: common.String("DHCP"), @@ -131,7 +131,7 @@ func TestBuildSubnetPort(t *testing.T) { labelTags: nil, expectedPort: &model.VpcSubnetPort{ DisplayName: common.String("fake_pod"), - Id: common.String("fake_pod-c5db1800-ce4c-11de-a935-8105ba7ace78"), + Id: common.String("fake_pod_c5db1800-ce4c-11de-a935-8105ba7ace78"), Tags: []model.Tag{ { Scope: common.String("nsx-op/cluster"), @@ -154,7 +154,7 @@ func TestBuildSubnetPort(t *testing.T) { Tag: common.String("c5db1800-ce4c-11de-a935-8105ba7ace78"), }, }, - Path: common.String("fake_path/ports/fake_pod-c5db1800-ce4c-11de-a935-8105ba7ace78"), + Path: common.String("fake_path/ports/fake_pod_c5db1800-ce4c-11de-a935-8105ba7ace78"), ParentPath: common.String("fake_path"), Attachment: &model.PortAttachment{ AllocateAddresses: common.String("DHCP"), diff --git a/pkg/nsx/services/vpc/builder_test.go b/pkg/nsx/services/vpc/builder_test.go index 7eadd8582..571f8797b 100644 --- a/pkg/nsx/services/vpc/builder_test.go +++ b/pkg/nsx/services/vpc/builder_test.go @@ -83,7 +83,7 @@ func TestBuildNSXVPC(t *testing.T) { PrivateIPs: []string{"192.168.1.0/24"}, } netInfoObj := &v1alpha1.NetworkInfo{ - ObjectMeta: metav1.ObjectMeta{Namespace: "ns1", Name: "ns1", UID: "netinfouid1"}, + ObjectMeta: metav1.ObjectMeta{Namespace: "ns1", Name: "netinfo1", UID: "netinfouid1"}, VPCs: nil, } nsObj := &v1.Namespace{ @@ -126,8 +126,8 @@ func TestBuildNSXVPC(t *testing.T) { useAVILB: true, lbProviderChanged: false, expVPC: &model.Vpc{ - Id: common.String("ns1-netinfouid1"), - DisplayName: common.String("ns1-netinfouid1"), + Id: common.String("netinfo1_netinfouid1"), + DisplayName: common.String("netinfo1_netinfouid1"), LoadBalancerVpcEndpoint: &model.LoadBalancerVPCEndpoint{Enabled: common.Bool(true)}, PrivateIps: []string{"192.168.3.0/24"}, IpAddressType: common.String("IPV4"), @@ -146,8 +146,8 @@ func TestBuildNSXVPC(t *testing.T) { useAVILB: false, lbProviderChanged: false, expVPC: &model.Vpc{ - Id: common.String("ns1-netinfouid1"), - DisplayName: common.String("ns1-netinfouid1"), + Id: common.String("netinfo1_netinfouid1"), + DisplayName: common.String("netinfo1_netinfouid1"), PrivateIps: []string{"192.168.3.0/24"}, IpAddressType: common.String("IPV4"), Tags: []model.Tag{ @@ -163,16 +163,16 @@ func TestBuildNSXVPC(t *testing.T) { name: "update VPC with AVI load balancer disabled -> enabled", ncPrivateIps: []string{"192.168.3.0/24"}, existingVPC: &model.Vpc{ - Id: common.String("ns1-netinfouid1"), - DisplayName: common.String("ns1-netinfouid1"), + Id: common.String("netinfo1_netinfouid1"), + DisplayName: common.String("netinfo1_netinfouid1"), PrivateIps: []string{"192.168.3.0/24"}, IpAddressType: common.String("IPV4"), }, useAVILB: true, lbProviderChanged: true, expVPC: &model.Vpc{ - Id: common.String("ns1-netinfouid1"), - DisplayName: common.String("ns1-netinfouid1"), + Id: common.String("netinfo1_netinfouid1"), + DisplayName: common.String("netinfo1_netinfouid1"), LoadBalancerVpcEndpoint: &model.LoadBalancerVPCEndpoint{Enabled: common.Bool(true)}, PrivateIps: []string{"192.168.3.0/24"}, IpAddressType: common.String("IPV4"), @@ -182,16 +182,16 @@ func TestBuildNSXVPC(t *testing.T) { name: "update VPC with NSX load balancer disabled -> enabled", ncPrivateIps: []string{"192.168.3.0/24"}, existingVPC: &model.Vpc{ - Id: common.String("ns1-netinfouid1"), - DisplayName: common.String("ns1-netinfouid1"), + Id: common.String("netinfo1_netinfouid1"), + DisplayName: common.String("netinfo1_netinfouid1"), PrivateIps: []string{"192.168.3.0/24"}, IpAddressType: common.String("IPV4"), }, useAVILB: false, lbProviderChanged: true, expVPC: &model.Vpc{ - Id: common.String("ns1-netinfouid1"), - DisplayName: common.String("ns1-netinfouid1"), + Id: common.String("netinfo1_netinfouid1"), + DisplayName: common.String("netinfo1_netinfouid1"), PrivateIps: []string{"192.168.3.0/24"}, IpAddressType: common.String("IPV4"), }, diff --git a/pkg/util/utils.go b/pkg/util/utils.go index 4dbea56b8..e3e663c72 100644 --- a/pkg/util/utils.go +++ b/pkg/util/utils.go @@ -82,7 +82,7 @@ func NormalizeName(name string) string { } func normalizeNameByLimit(name string, suffix string, limit int) string { - newName := connectStrings("-", name, suffix) + newName := connectStrings(common.ConnectorUnderline, name, suffix) if len(newName) <= limit { return newName } @@ -97,7 +97,7 @@ func normalizeNameByLimit(name string, suffix string, limit int) string { if len(name) < nameLength { nameLength = len(name) } - return strings.Join([]string{name[:nameLength], hashString[:common.HashLength]}, "-") + return strings.Join([]string{name[:nameLength], hashString[:common.HashLength]}, common.ConnectorUnderline) } func NormalizeId(name string) string { @@ -403,12 +403,12 @@ func GenerateIDByObjectByLimit(obj metav1.Object, limit int) string { func GenerateIDByObjectWithSuffix(obj metav1.Object, suffix string) string { limit := common.MaxIdLength limit -= len(suffix) + 1 - return connectStrings("_", normalizeNameByLimit(obj.GetName(), string(obj.GetUID()), limit), suffix) + return connectStrings(common.ConnectorUnderline, normalizeNameByLimit(obj.GetName(), string(obj.GetUID()), limit), suffix) } // GenerateID generate id for NSX resource, some resources has complex index, so set it type to string -func GenerateID(res_id, prefix, suffix string, index string) string { - return connectStrings("_", prefix, res_id, index, suffix) +func GenerateID(resID, prefix, suffix string, index string) string { + return connectStrings(common.ConnectorUnderline, prefix, resID, index, suffix) } func connectStrings(sep string, parts ...string) string { @@ -421,24 +421,25 @@ func connectStrings(sep string, parts ...string) string { return strings.Join(strParts, sep) } -func GenerateDisplayName(res_name, prefix, suffix, project, cluster string) string { - // Return a string in this format: prefix-cluster-res_name-project-suffix. - return connectStrings("-", prefix, cluster, res_name, project, suffix) +func generateDisplayName(connector, resName, prefix, suffix, project, cluster string) string { + // Return a string in this format: + // prefixclusterresNameprojectsuffix. + return connectStrings(connector, prefix, cluster, resName, project, suffix) } -func GenerateTruncName(limit int, res_name string, prefix, suffix, project, cluster string) string { - adjusted_limit := limit - len(prefix) - len(suffix) +func GenerateTruncName(limit int, resName string, prefix, suffix, project, cluster string) string { + adjustedLimit := limit - len(prefix) - len(suffix) for _, i := range []string{prefix, suffix} { if len(i) > 0 { - adjusted_limit -= 1 + adjustedLimit -= 1 } } - old_name := GenerateDisplayName(res_name, "", "", project, cluster) - if len(old_name) > adjusted_limit { - new_name := normalizeNameByLimit(old_name, "", adjusted_limit) - return GenerateDisplayName(new_name, prefix, suffix, "", "") + oldName := generateDisplayName(common.ConnectorUnderline, resName, "", "", project, cluster) + if len(oldName) > adjustedLimit { + newName := normalizeNameByLimit(oldName, "", adjustedLimit) + return generateDisplayName(common.ConnectorUnderline, newName, prefix, suffix, "", "") } - return GenerateDisplayName(res_name, prefix, suffix, project, cluster) + return generateDisplayName(common.ConnectorUnderline, resName, prefix, suffix, project, cluster) } func BuildBasicTags(cluster string, obj interface{}, namespaceID types.UID) []model.Tag { diff --git a/pkg/util/utils_test.go b/pkg/util/utils_test.go index 638460222..87b121e5d 100644 --- a/pkg/util/utils_test.go +++ b/pkg/util/utils_test.go @@ -8,10 +8,14 @@ import ( "fmt" "net" "reflect" + "strconv" "strings" "testing" "github.com/stretchr/testify/assert" + + "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -26,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-HashLength-1), "0c103888")) } func TestNormalizeLabelKey(t *testing.T) { @@ -98,7 +102,6 @@ func TestUtil_IsNsInSystemNamespace(t *testing.T) { ns = types.NamespacedName{Namespace: "sys-ns", Name: "dummy"} isCRInSysNs, err = IsSystemNamespace(client, ns.Namespace, nil) - if err != nil { t.Fatalf(err.Error()) } @@ -410,8 +413,8 @@ func TestGenerateDisplayName(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := GenerateDisplayName(tt.args.res_name, tt.args.prefix, tt.args.suffix, tt.args.project, tt.args.cluster); got != tt.want { - t.Errorf("GenerateDisplayName() = %v, want %v", got, tt.want) + if got := generateDisplayName("-", tt.args.res_name, tt.args.prefix, tt.args.suffix, tt.args.project, tt.args.cluster); got != tt.want { + t.Errorf("generateDisplayName() = %v, want %v", got, tt.want) } }) } @@ -440,7 +443,7 @@ func TestGenerateTruncName(t *testing.T) { suffix: "", project: "", }, - want: "sp-1234-456", + want: "sp_1234-456", }, { name: "test-only-name", @@ -462,7 +465,7 @@ func TestGenerateTruncName(t *testing.T) { suffix: "scope", project: "", }, - want: "sp-1234-456-scope", + want: "sp_1234-456_scope", }, { name: "test-index", @@ -473,7 +476,7 @@ func TestGenerateTruncName(t *testing.T) { suffix: "scope", project: "test", }, - want: "sp-1234-456-test-scope", + want: "sp_1234-456_test_scope", }, { name: "test-cluster", @@ -485,7 +488,7 @@ func TestGenerateTruncName(t *testing.T) { project: "", cluster: "k8scl-one", }, - want: "k8scl-one-1234-456-scope", + want: "k8scl-one_1234-456_scope", }, { name: "test-project-cluster", @@ -497,7 +500,7 @@ func TestGenerateTruncName(t *testing.T) { project: strings.Repeat("s", 300), cluster: "k8scl-one", }, - want: "sr-k8scl-one-1234-456-ssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssss-813dffe8-scope", + want: "sr_k8scl-one_1234-456_ssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssss_e89b45cc_scope", }, } for _, tt := range tests { @@ -651,19 +654,19 @@ func TestGenerateIDByObject(t *testing.T) { name: "no limit set", obj: &metav1.ObjectMeta{Name: "abcdefg", UID: "b720ee2c-5788-4680-9796-0f93db33d8a9"}, limit: 0, - expID: "abcdefg-b720ee2c-5788-4680-9796-0f93db33d8a9", + expID: "abcdefg_b720ee2c-5788-4680-9796-0f93db33d8a9", }, { name: "truncate with hash on uid", obj: &metav1.ObjectMeta{Name: "abcdefg", UID: "b720ee2c-5788-4680-9796-0f93db33d8a9"}, limit: 20, - expID: "abcdefg-df78acb2", + expID: "abcdefg_df78acb2", }, { name: "longer name with truncate", obj: &metav1.ObjectMeta{Name: strings.Repeat("a", 256), UID: "b720ee2c-5788-4680-9796-0f93db33d8a9"}, limit: 0, - expID: fmt.Sprintf("%s-df78acb2", strings.Repeat("a", 246)), + expID: fmt.Sprintf("%s_df78acb2", strings.Repeat("a", 246)), }, } { t.Run(tc.name, func(t *testing.T) { @@ -691,14 +694,14 @@ func TestGenerateIDByObjectWithSuffix(t *testing.T) { obj: &metav1.ObjectMeta{Name: "abcdefg", UID: "b720ee2c-5788-4680-9796-0f93db33d8a9"}, limit: 0, suffix: "2", - expID: "abcdefg-b720ee2c-5788-4680-9796-0f93db33d8a9_2", + expID: "abcdefg_b720ee2c-5788-4680-9796-0f93db33d8a9_2", }, { name: "longer name with truncate", obj: &metav1.ObjectMeta{Name: strings.Repeat("a", 256), UID: "b720ee2c-5788-4680-9796-0f93db33d8a9"}, limit: 0, suffix: "28e85c0b-21e4-4cab-b1c3-597639dfe752", - expID: fmt.Sprintf("%s-df78acb2_28e85c0b-21e4-4cab-b1c3-597639dfe752", strings.Repeat("a", 209)), + expID: fmt.Sprintf("%s_df78acb2_28e85c0b-21e4-4cab-b1c3-597639dfe752", strings.Repeat("a", 209)), }, } { t.Run(tc.name, func(t *testing.T) { @@ -707,3 +710,27 @@ func TestGenerateIDByObjectWithSuffix(t *testing.T) { }) } } + +func TestConnectStrings(t *testing.T) { + string1 := "aa" + string2 := "bb" + connectString := connectStrings(common.ConnectorUnderline, string1, string2) + expString := "aa" + common.ConnectorUnderline + "bb" + assert.Equal(t, connectString, expString) + + connectString = connectStrings("-", string1, string2) + expString = "aa" + "-" + "bb" + assert.Equal(t, connectString, expString) + + int1 := 11 + int2 := 22 + connectString = connectStrings(common.ConnectorUnderline, strconv.Itoa(int1), strconv.Itoa(int2)) + expString = "11" + common.ConnectorUnderline + "22" + expString = fmt.Sprintf("%d%s%d", int1, common.ConnectorUnderline, int2) + assert.Equal(t, connectString, expString) + + connectString = connectStrings(common.ConnectorUnderline, string1, strconv.Itoa(int2)) + expString = "aa" + common.ConnectorUnderline + "22" + expString = fmt.Sprintf("%s%s%d", string1, common.ConnectorUnderline, int2) + assert.Equal(t, connectString, expString) +} diff --git a/test/e2e/nsx_security_policy_test.go b/test/e2e/nsx_security_policy_test.go index 8ef8bcd7c..0d204ec36 100644 --- a/test/e2e/nsx_security_policy_test.go +++ b/test/e2e/nsx_security_policy_test.go @@ -30,8 +30,8 @@ const ( func TestSecurityPolicyBasicTraffic(t *testing.T) { ns := "test-security-policy-1" securityPolicyName := "isolate-policy-1" - ruleName0 := "all-ingress-isolation" - ruleName1 := "all-egress-isolation" + ruleName0 := "all_ingress_isolation" + ruleName1 := "all_egress_isolation" var err error setupTest(t, ns) defer teardownTest(t, ns, defaultTimeout) @@ -107,8 +107,8 @@ func TestSecurityPolicyBasicTraffic(t *testing.T) { func TestSecurityPolicyAddDeleteRule(t *testing.T) { ns := "test-security-policy-2" securityPolicyName := "isolate-policy-1" - ruleName0 := "all-ingress-isolation" - ruleName1 := "all-egress-isolation" + ruleName0 := "all_ingress_isolation" + ruleName1 := "all_egress_isolation" setupTest(t, ns) defer teardownTest(t, ns, defaultTimeout) @@ -242,8 +242,8 @@ func TestSecurityPolicyNamedPortWithoutPod(t *testing.T) { securityPolicyCRName := "named-port-policy-without-pod" webA := "web" labelWeb := "tcp-deployment" - ruleName0 := "all-ingress-isolation" - ruleName1 := "all-egress-isolation" + ruleName0 := "all_ingress_isolation" + ruleName1 := "all_egress_isolation" testData.deleteNamespace(nsClient, defaultTimeout) testData.deleteNamespace(nsWeb, defaultTimeout)