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)