From 04c059313a6862caa31cf0701b5d1dcde3396885 Mon Sep 17 00:00:00 2001 From: Wenying Dong Date: Thu, 10 Oct 2024 15:52:38 +0800 Subject: [PATCH] Refine the security policy logic related with named port --- pkg/nsx/services/common/services_mock.go | 6 +- pkg/nsx/services/securitypolicy/builder.go | 155 ++++---- .../services/securitypolicy/builder_test.go | 86 ++-- pkg/nsx/services/securitypolicy/expand.go | 65 +-- .../services/securitypolicy/expand_test.go | 376 ++++++++++++++++++ .../services/securitypolicy/firewall_test.go | 8 +- 6 files changed, 527 insertions(+), 169 deletions(-) diff --git a/pkg/nsx/services/common/services_mock.go b/pkg/nsx/services/common/services_mock.go index abe8bd57a..53e37b9d8 100644 --- a/pkg/nsx/services/common/services_mock.go +++ b/pkg/nsx/services/common/services_mock.go @@ -6,6 +6,7 @@ import ( type MockVPCServiceProvider struct { mock.Mock + VPCResource *VPCResourceInfo } func (m *MockVPCServiceProvider) GetNamespacesByNetworkconfigName(nc string) []string { @@ -44,6 +45,9 @@ func (m *MockVPCServiceProvider) GetDefaultNetworkConfig() (bool, *VPCNetworkCon } func (m *MockVPCServiceProvider) ListVPCInfo(ns string) []VPCResourceInfo { - m.Called() + m.Called(ns) + if m.VPCResource != nil { + return []VPCResourceInfo{*m.VPCResource} + } return []VPCResourceInfo{} } diff --git a/pkg/nsx/services/securitypolicy/builder.go b/pkg/nsx/services/securitypolicy/builder.go index 6a7bfb56a..293c9d21b 100644 --- a/pkg/nsx/services/securitypolicy/builder.go +++ b/pkg/nsx/services/securitypolicy/builder.go @@ -480,17 +480,18 @@ func (service *SecurityPolicyService) buildRuleAndGroups(obj *v1alpha1.SecurityP return nsxRules, ruleGroups, nsxGroupShares, nil } -func (service *SecurityPolicyService) buildRuleServiceEntries(port v1alpha1.SecurityPolicyPort, portAddress nsxutil.PortAddress) *data.StructValue { +func buildRuleServiceEntries(port v1alpha1.SecurityPolicyPort) *data.StructValue { var portRange string sourcePorts := data.NewListValue() destinationPorts := data.NewListValue() + // Note: the caller ensures the given port.Port type is Int. For named port case, the caller should + // convert to a new SecurityPolicyPort using the correct port number. // In case that the destination_port in NSX-T is 0. - endPort := port.EndPort - if endPort == 0 { - portRange = fmt.Sprint(portAddress.Port) + if port.EndPort == 0 { + portRange = port.Port.String() } else { - portRange = fmt.Sprintf("%d-%d", portAddress.Port, endPort) + portRange = fmt.Sprintf("%s-%d", port.Port.String(), port.EndPort) } destinationPorts.Add(data.NewStringValue(portRange)) @@ -600,25 +601,32 @@ func (service *SecurityPolicyService) buildRuleID(obj *v1alpha1.SecurityPolicy, // A rule containing named port may be expanded to multiple NSX rules if the name ports map to multiple port numbers. // So, in VPC network, the rule port numbers, which either are defined in rule Port or resolved from named port, will be appended as CR rule baseID to distinguish them. // For T1, the portIdx and portAddressIdx are appended as suffix. -func (service *SecurityPolicyService) buildExpandedRuleID(obj *v1alpha1.SecurityPolicy, ruleIdx int, portIdx, portAddressIdx int, - hasNamedport bool, portNumber int, createdFor string, +func (service *SecurityPolicyService) buildExpandedRuleID(obj *v1alpha1.SecurityPolicy, ruleIdx int, + createdFor string, namedPort *namedPortInfo, ) string { ruleBaseID := service.buildRuleID(obj, ruleIdx, createdFor) if IsVPCEnabled(service) { portNumberSuffix := "" - if !hasNamedport { - portNumberSuffix = service.buildRulePortsNumberString(obj.Spec.Rules[ruleIdx].Ports) + if namedPort != nil { + portNumberSuffix = service.buildRulePortNumberString(namedPort.port) } else { - portNumberSuffix = service.buildRulePortNumberString(obj.Spec.Rules[ruleIdx].Ports[portIdx], portNumber) + portNumberSuffix = service.buildRulePortsNumberString(obj.Spec.Rules[ruleIdx].Ports) } return strings.Join([]string{ruleBaseID, portNumberSuffix}, common.ConnectorUnderline) } - return strings.Join([]string{ruleBaseID, strconv.Itoa(portIdx), strconv.Itoa(portAddressIdx)}, common.ConnectorUnderline) + // With T1 topology, the NSX Rule ID includes the index of the rule's SecurityPolicyPort and the + // index of the PortAddress, this is to make ID format consistent with named port case. For a Rule + // without any named ports, 0 is used for both SecurityPolicyPort and PortAddress indexes. + idSuffix := "0_0" + if namedPort != nil { + idSuffix = namedPort.idSuffix + } + return strings.Join([]string{ruleBaseID, idSuffix}, common.ConnectorUnderline) } -func (service *SecurityPolicyService) buildRuleDisplayName(rule *v1alpha1.SecurityPolicyRule, portIdx int, hasNamedport bool, portNumber int, createdFor string) (string, error) { +func (service *SecurityPolicyService) buildRuleDisplayName(rule *v1alpha1.SecurityPolicyRule, createdFor string, namedPortInfo *namedPortInfo) (string, error) { var ruleName string var ruleAct string @@ -649,27 +657,27 @@ func (service *SecurityPolicyService) buildRuleDisplayName(rule *v1alpha1.Securi // 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 = strings.Join([]string{ruleName, suffix}, common.ConnectorUnderline) + // We don't append the suffix to display name if the NSX security rule is created for a NetworkPolicy and + // its rule.Name is set. This is applicable for the internally generated "isolation" SecurityPolicy from a + // user created NetworkPolicy. + if createdFor == common.ResourceTypeNetworkPolicy { + suffix = "" } } else { - ruleName = service.buildRulePortsString(rule.Ports, suffix) + ruleName = service.buildRulePortsString(rule.Ports) } - if !hasNamedport { - 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, common.ConnectorUnderline+suffix) - return util.GenerateTruncName(common.MaxNameLength, ruleName[:index]+"."+service.buildRulePortString(rule.Ports[portIdx], portNumber), "", suffix, "", ""), nil + if namedPortInfo == nil { + return util.GenerateTruncName(common.MaxNameLength, ruleName, "", suffix, "", ""), nil } + + // 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. + + return util.GenerateTruncName(common.MaxNameLength, ruleName+"."+service.buildRulePortString(namedPortInfo.port), "", suffix, "", ""), nil } func (service *SecurityPolicyService) buildRuleAppliedGroupByPolicy(obj *v1alpha1.SecurityPolicy, nsxRuleSrcGroupPath string, nsxRuleDstGroupPath string, createdFor string) (string, error) { @@ -922,8 +930,8 @@ func (service *SecurityPolicyService) buildRulePeerGroup(obj *v1alpha1.SecurityP // Build rule basic info, ruleIdx is the index of the rules of security policy, // portIdx is the index of rule's ports, portAddressIdx is the index // of multiple port number if one named port maps to multiple port numbers. -func (service *SecurityPolicyService) buildRuleBasicInfo(obj *v1alpha1.SecurityPolicy, rule *v1alpha1.SecurityPolicyRule, ruleIdx int, portIdx int, portAddressIdx int, - hasNamedport bool, portNumber int, createdFor string, +func (service *SecurityPolicyService) buildRuleBasicInfo(obj *v1alpha1.SecurityPolicy, rule *v1alpha1.SecurityPolicyRule, ruleIdx int, + createdFor string, namedPortInfo *namedPortInfo, ) (*model.Rule, error) { ruleAction, err := getRuleAction(rule) if err != nil { @@ -933,13 +941,13 @@ func (service *SecurityPolicyService) buildRuleBasicInfo(obj *v1alpha1.SecurityP if err != nil { return nil, err } - displayName, err := service.buildRuleDisplayName(rule, portIdx, hasNamedport, portNumber, createdFor) + displayName, err := service.buildRuleDisplayName(rule, createdFor, namedPortInfo) if err != nil { log.Error(err, "failed to build rule's display name", "securityPolicyUID", obj.UID, "rule", rule, "createdFor", createdFor) } nsxRule := model.Rule{ - Id: String(service.buildExpandedRuleID(obj, ruleIdx, portIdx, portAddressIdx, hasNamedport, portNumber, createdFor)), + Id: String(service.buildExpandedRuleID(obj, ruleIdx, createdFor, namedPortInfo)), DisplayName: &displayName, Direction: &ruleDirection, SequenceNumber: Int64(int64(ruleIdx)), @@ -1843,45 +1851,22 @@ func (service *SecurityPolicyService) getNamespaceUID(ns string) (nsUid types.UI return namespace_uid } -func (service *SecurityPolicyService) buildRulePortString(port v1alpha1.SecurityPolicyPort, portNumber int) string { - protocol := string(port.Protocol) - // Build the rule port string name for non named port. - // This is a common case where the string is built from port definition. For instance, - // - protocol: TCP - // port: 8282 - // endPort: 8286 - // The built port string is: TCP.8282.8286 - // - protocol: UDP - // port: 3308 - // The built port string is: UDP.3308 - if port.Port.Type == intstr.String && portNumber > 0 { - // Build the rule port string name for named port. - // The port string is built from specific port number resolved from named port. - return fmt.Sprintf("%s.%d", protocol, portNumber) - } - if port.EndPort != 0 { - return fmt.Sprintf("%s.%s.%d", protocol, (port.Port).String(), port.EndPort) - } - return fmt.Sprintf("%s.%s", protocol, (port.Port).String()) +func (service *SecurityPolicyService) buildRulePortString(port v1alpha1.SecurityPolicyPort) string { + return fmt.Sprintf("%s.%s", port.Protocol, service.buildRulePortNumberString(port)) } -func (service *SecurityPolicyService) buildRulePortsString(ports []v1alpha1.SecurityPolicyPort, suffix string) string { - portsString := "" +func (service *SecurityPolicyService) buildRulePortsString(ports []v1alpha1.SecurityPolicyPort) string { if ports == nil || len(ports) == 0 { - portsString = common.RuleAnyPorts - } else { - portStrings := make([]string, len(ports)) - for idx, p := range ports { - port := p - portStrings[idx] = service.buildRulePortString(port, -1) - } - portsString = strings.Join(portStrings, common.ConnectorUnderline) + return common.RuleAnyPorts } - - return util.GenerateTruncName(common.MaxNameLength, portsString, "", suffix, "", "") + portStrings := make([]string, len(ports)) + for idx := range ports { + portStrings[idx] = service.buildRulePortString(ports[idx]) + } + return strings.Join(portStrings, common.ConnectorUnderline) } -func (service *SecurityPolicyService) buildRulePortNumberString(port v1alpha1.SecurityPolicyPort, portNumber int) string { +func (service *SecurityPolicyService) buildRulePortNumberString(port v1alpha1.SecurityPolicyPort) string { // Build the rule port number string name for non named port. // This is a common case where the string is built from port definition. For instance, // - protocol: TCP @@ -1891,11 +1876,6 @@ func (service *SecurityPolicyService) buildRulePortNumberString(port v1alpha1.Se // - protocol: UDP // port: 3308 // The built port number string is: 3308 - if port.Port.Type == intstr.String && portNumber > 0 { - // Build the rule port number string name for named port. - // The port number string is built from specific port number resolved from named port. - return fmt.Sprintf("%d", portNumber) - } if port.EndPort != 0 { return fmt.Sprintf("%s.%d", (port.Port).String(), port.EndPort) } @@ -1910,7 +1890,7 @@ func (service *SecurityPolicyService) buildRulePortsNumberString(ports []v1alpha portNumStrings := make([]string, len(ports)) for idx, p := range ports { port := p - portNumStrings[idx] = service.buildRulePortNumberString(port, -1) + portNumStrings[idx] = service.buildRulePortNumberString(port) } return strings.Join(portNumStrings, common.ConnectorUnderline) } @@ -1932,3 +1912,36 @@ func (service *SecurityPolicyService) BuildNetworkPolicyAllowPolicyID(uid string func (service *SecurityPolicyService) BuildNetworkPolicyIsolationPolicyID(uid string) string { return strings.Join([]string{uid, common.RuleActionDrop}, common.ConnectorUnderline) } + +type namedPortInfo struct { + port v1alpha1.SecurityPolicyPort + ips []string + + // idSuffix is used in T1 environment to generate the NSX rule ID. It is constructed by + // SecurityPolicyPortIdx_PortAddressIdx. + // TODO: Remove this field after we don't use the SecurityPolicyPort index in NSX rule ID with T1 topology. + idSuffix string +} + +func newNamedPortInfo(port v1alpha1.SecurityPolicyPort) *namedPortInfo { + return &namedPortInfo{ + port: port, + idSuffix: fmt.Sprintf("0%s0", common.ConnectorUnderline), + } +} + +// newNamedPortInfoByAddressPort constructs a new SecurityPolicyPort object using the provided protocol and +// portAddr, and the generated SecurityPolicyPort.Port type is Int. +// Note, we don't support the case a SecurityPolicyPort is configured with a named port and end port at the +// same time, so we don't set the generated SecurityPolicyPort.EndPort in this function. The caller has a +// pre-check on that case. +func newNamedPortInfoByAddressPort(portAddr nsxutil.PortAddress, protocol corev1.Protocol) *namedPortInfo { + return &namedPortInfo{ + port: v1alpha1.SecurityPolicyPort{ + Protocol: protocol, + Port: intstr.FromInt32(int32(portAddr.Port)), + }, + ips: portAddr.IPs, + idSuffix: fmt.Sprintf("0%s0", common.ConnectorUnderline), + } +} diff --git a/pkg/nsx/services/securitypolicy/builder_test.go b/pkg/nsx/services/securitypolicy/builder_test.go index aef63e991..52e3997de 100644 --- a/pkg/nsx/services/securitypolicy/builder_test.go +++ b/pkg/nsx/services/securitypolicy/builder_test.go @@ -17,6 +17,7 @@ import ( "github.com/vmware-tanzu/nsx-operator/pkg/apis/legacy/v1alpha1" "github.com/vmware-tanzu/nsx-operator/pkg/config" "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common" + nsxutil "github.com/vmware-tanzu/nsx-operator/pkg/nsx/util" ) func TestBuildSecurityPolicy(t *testing.T) { @@ -945,61 +946,52 @@ func TestBuildRulePortsString(t *testing.T) { tests := []struct { name string inputPorts []v1alpha1.SecurityPolicyPort - suffix string expectedRulePortsString string }{ { 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", + expectedRulePortsString: "TCP.80_UDP.1234.1235", }, { name: "build-string-for-multiple-ports-userdefinedrule-without-named-port", inputPorts: securityPolicyWithMultipleNormalPorts.Spec.Rules[1].Ports, - suffix: "egress_drop", - expectedRulePortsString: "TCP.88_UDP.1236.1237_egress_drop", + expectedRulePortsString: "TCP.88_UDP.1236.1237", }, { name: "build-string-for-multiple-ports-start-end-port-same-without-named-port", inputPorts: securityPolicyWithMultipleNormalPorts.Spec.Rules[2].Ports, - suffix: "egress_allow", - expectedRulePortsString: "TCP.80_UDP.1234.1234_egress_allow", + expectedRulePortsString: "TCP.80_UDP.1234.1234", }, { name: "build-string-for-multiple-ports-with-http-named-port", inputPorts: securityPolicyWithOneNamedPort.Spec.Rules[0].Ports, - suffix: "ingress_allow", - expectedRulePortsString: "TCP.http_UDP.1234.1235_ingress_allow", + expectedRulePortsString: "TCP.http_UDP.1234.1235", }, { name: "build-string-for-multiple-ports-with-https-named-port", inputPorts: securityPolicyWithOneNamedPort.Spec.Rules[1].Ports, - suffix: "ingress_allow", - expectedRulePortsString: "TCP.https_UDP.1236.1237_ingress_allow", + expectedRulePortsString: "TCP.https_UDP.1236.1237", }, { name: "build-string-for-multiple-ports-with-web-named-port", inputPorts: securityPolicyWithOneNamedPort.Spec.Rules[2].Ports, - suffix: "ingress_allow", - expectedRulePortsString: "TCP.web_UDP.533_ingress_allow", + expectedRulePortsString: "TCP.web_UDP.533", }, { name: "build-string-for-multiple-ports-with-db-named-port", inputPorts: securityPolicyWithOneNamedPort.Spec.Rules[3].Ports, - suffix: "ingress_allow", - expectedRulePortsString: "TCP.db_ingress_allow", + expectedRulePortsString: "TCP.db", }, { name: "build-string-for-nil-ports", inputPorts: nil, - suffix: "ingress_allow", - expectedRulePortsString: "all_ingress_allow", + expectedRulePortsString: "all", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - observedString := service.buildRulePortsString(tt.inputPorts, tt.suffix) + observedString := service.buildRulePortsString(tt.inputPorts) assert.Equal(t, tt.expectedRulePortsString, observedString) }) } @@ -1066,10 +1058,8 @@ func TestBuildRuleDisplayName(t *testing.T) { inputSecurityPolicy *v1alpha1.SecurityPolicy inputRule *v1alpha1.SecurityPolicyRule ruleIdx int - portIdx int - hasNamedPort bool - portNumber int createdFor string + namedPort *namedPortInfo expectedRuleDisplayName string }{ { @@ -1077,10 +1067,8 @@ func TestBuildRuleDisplayName(t *testing.T) { inputSecurityPolicy: &securityPolicyWithMultipleNormalPorts, inputRule: &securityPolicyWithMultipleNormalPorts.Spec.Rules[0], ruleIdx: 0, - portIdx: 0, - hasNamedPort: false, - portNumber: -1, createdFor: common.ResourceTypeNetworkPolicy, + namedPort: nil, expectedRuleDisplayName: "TCP.80_UDP.1234.1235_ingress_allow", }, { @@ -1088,10 +1076,8 @@ func TestBuildRuleDisplayName(t *testing.T) { inputSecurityPolicy: &securityPolicyWithMultipleNormalPorts, inputRule: &securityPolicyWithMultipleNormalPorts.Spec.Rules[1], ruleIdx: 1, - portIdx: 0, - hasNamedPort: false, - portNumber: -1, createdFor: common.ResourceTypeNetworkPolicy, + namedPort: nil, expectedRuleDisplayName: "MultipleNormalPorts-rule1", }, { @@ -1099,10 +1085,8 @@ func TestBuildRuleDisplayName(t *testing.T) { inputSecurityPolicy: &securityPolicyWithMultipleNormalPorts, inputRule: &securityPolicyWithMultipleNormalPorts.Spec.Rules[1], ruleIdx: 1, - portIdx: 0, - hasNamedPort: false, - portNumber: -1, createdFor: common.ResourceTypeSecurityPolicy, + namedPort: nil, expectedRuleDisplayName: "MultipleNormalPorts-rule1_egress_isolation", }, { @@ -1110,10 +1094,8 @@ func TestBuildRuleDisplayName(t *testing.T) { inputSecurityPolicy: &securityPolicyWithOneNamedPort, inputRule: &securityPolicyWithOneNamedPort.Spec.Rules[0], ruleIdx: 0, - portIdx: 0, - hasNamedPort: true, - portNumber: 80, createdFor: common.ResourceTypeSecurityPolicy, + namedPort: newNamedPortInfoByAddressPort(nsxutil.PortAddress{Port: 80}, "TCP"), expectedRuleDisplayName: "user-defined-rule-namedport.TCP.80_ingress_allow", }, { @@ -1121,16 +1103,14 @@ func TestBuildRuleDisplayName(t *testing.T) { inputSecurityPolicy: &securityPolicyWithOneNamedPort, inputRule: &securityPolicyWithOneNamedPort.Spec.Rules[1], ruleIdx: 1, - portIdx: 0, - hasNamedPort: true, - portNumber: 443, createdFor: common.ResourceTypeSecurityPolicy, + namedPort: newNamedPortInfoByAddressPort(nsxutil.PortAddress{Port: 443}, "TCP"), expectedRuleDisplayName: "TCP.https_UDP.1236.1237.TCP.443_ingress_allow", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - observedDisplayName, observedError := service.buildRuleDisplayName(tt.inputRule, tt.portIdx, tt.hasNamedPort, tt.portNumber, tt.createdFor) + observedDisplayName, observedError := service.buildRuleDisplayName(tt.inputRule, tt.createdFor, tt.namedPort) assert.Equal(t, tt.expectedRuleDisplayName, observedDisplayName) assert.Equal(t, nil, observedError) }) @@ -1154,11 +1134,8 @@ func TestBuildExpandedRuleID(t *testing.T) { inputSecurityPolicy *v1alpha1.SecurityPolicy inputRule *v1alpha1.SecurityPolicyRule ruleIdx int - portIdx int - portAddressIdx int - hasNamedPort bool - portNumber int createdFor string + namedPort *namedPortInfo expectedRuleID string }{ { @@ -1167,11 +1144,8 @@ func TestBuildExpandedRuleID(t *testing.T) { inputSecurityPolicy: &securityPolicyWithMultipleNormalPorts, inputRule: &securityPolicyWithMultipleNormalPorts.Spec.Rules[0], ruleIdx: 0, - portIdx: 0, - portAddressIdx: 0, - hasNamedPort: false, - portNumber: -1, createdFor: common.ResourceTypeSecurityPolicy, + namedPort: nil, expectedRuleID: "spMulPorts_spMulPortsuidA_d0b8e36c_80_1234.1235", }, { @@ -1180,11 +1154,8 @@ func TestBuildExpandedRuleID(t *testing.T) { inputSecurityPolicy: &securityPolicyWithMultipleNormalPorts, inputRule: &securityPolicyWithMultipleNormalPorts.Spec.Rules[0], ruleIdx: 0, - portIdx: 0, - portAddressIdx: 0, - hasNamedPort: false, - portNumber: -1, createdFor: common.ResourceTypeSecurityPolicy, + namedPort: nil, expectedRuleID: "sp_spMulPortsuidA_d0b8e36cf858e76624b9706c3c8e77b6006c0e10_0_0_0", }, { @@ -1193,11 +1164,8 @@ func TestBuildExpandedRuleID(t *testing.T) { inputSecurityPolicy: &securityPolicyWithMultipleNormalPorts, inputRule: &securityPolicyWithMultipleNormalPorts.Spec.Rules[1], ruleIdx: 1, - portIdx: 0, - portAddressIdx: 0, - hasNamedPort: false, - portNumber: -1, createdFor: common.ResourceTypeNetworkPolicy, + namedPort: nil, expectedRuleID: "spMulPorts_spMulPortsuidA_555356be_88_1236.1237", }, { @@ -1206,11 +1174,8 @@ func TestBuildExpandedRuleID(t *testing.T) { inputSecurityPolicy: &securityPolicyWithOneNamedPort, inputRule: &securityPolicyWithOneNamedPort.Spec.Rules[0], ruleIdx: 0, - portIdx: 0, - portAddressIdx: 0, - hasNamedPort: true, - portNumber: 80, createdFor: common.ResourceTypeSecurityPolicy, + namedPort: newNamedPortInfoByAddressPort(nsxutil.PortAddress{Port: 80}, "TCP"), expectedRuleID: "spNamedPorts_spNamedPortsuidA_3f7c7d8c_80", }, { @@ -1219,11 +1184,8 @@ func TestBuildExpandedRuleID(t *testing.T) { inputSecurityPolicy: &securityPolicyWithOneNamedPort, inputRule: &securityPolicyWithOneNamedPort.Spec.Rules[0], ruleIdx: 0, - portIdx: 0, - portAddressIdx: 0, - hasNamedPort: true, - portNumber: 80, createdFor: common.ResourceTypeSecurityPolicy, + namedPort: newNamedPortInfoByAddressPort(nsxutil.PortAddress{Port: 80}, "TCP"), expectedRuleID: "sp_spNamedPortsuidA_3f7c7d8c8449687178002f23599add04bf0c3250_0_0_0", }, } @@ -1231,7 +1193,7 @@ func TestBuildExpandedRuleID(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { svc.NSXConfig.EnableVPCNetwork = tt.vpcEnabled - observedRuleID := svc.buildExpandedRuleID(tt.inputSecurityPolicy, tt.ruleIdx, tt.portIdx, tt.portAddressIdx, tt.hasNamedPort, tt.portNumber, tt.createdFor) + observedRuleID := svc.buildExpandedRuleID(tt.inputSecurityPolicy, tt.ruleIdx, tt.createdFor, tt.namedPort) assert.Equal(t, tt.expectedRuleID, observedRuleID) }) } diff --git a/pkg/nsx/services/securitypolicy/expand.go b/pkg/nsx/services/securitypolicy/expand.go index 5a78496ef..29197d5a2 100644 --- a/pkg/nsx/services/securitypolicy/expand.go +++ b/pkg/nsx/services/securitypolicy/expand.go @@ -24,44 +24,40 @@ import ( func (service *SecurityPolicyService) expandRule(obj *v1alpha1.SecurityPolicy, rule *v1alpha1.SecurityPolicyRule, ruleIdx int, createdFor string, ) ([]*model.Group, []*model.Rule, error) { - var nsxRules []*model.Rule - var nsxGroups []*model.Group - if len(rule.Ports) == 0 { - nsxRule, err := service.buildRuleBasicInfo(obj, rule, ruleIdx, 0, 0, false, -1, createdFor) + nsxRule, err := service.buildRuleBasicInfo(obj, rule, ruleIdx, createdFor, nil) if err != nil { return nil, nil, err } - nsxRules = append(nsxRules, nsxRule) - return nsxGroups, nsxRules, nil + return nil, []*model.Rule{nsxRule}, nil } // Check if there is a namedport in the rule hasNamedPort := service.hasNamedPort(rule) if !hasNamedPort { - nsxRule, err := service.buildRuleBasicInfo(obj, rule, ruleIdx, 0, 0, false, -1, createdFor) + nsxRule, err := service.buildRuleBasicInfo(obj, rule, ruleIdx, createdFor, nil) if err != nil { return nil, nil, err } var ruleServiceEntries []*data.StructValue for _, port := range rule.Ports { - portAddress := nsxutil.PortAddress{Port: port.Port.IntValue()} - serviceEntry := service.buildRuleServiceEntries(port, portAddress) + serviceEntry := buildRuleServiceEntries(port) ruleServiceEntries = append(ruleServiceEntries, serviceEntry) } nsxRule.ServiceEntries = ruleServiceEntries - nsxRules = append(nsxRules, nsxRule) + return nil, []*model.Rule{nsxRule}, nil } - if hasNamedPort { - for portIdx, port := range rule.Ports { - nsxGroups2, nsxRules2, err := service.expandRuleByPort(obj, rule, ruleIdx, port, portIdx, createdFor) - if err != nil { - return nil, nil, err - } - nsxGroups = append(nsxGroups, nsxGroups2...) - nsxRules = append(nsxRules, nsxRules2...) + var nsxRules []*model.Rule + // nsxGroups is a slice for the IPSet groups referred by a security Rule if named port is configured. + var nsxGroups []*model.Group + for portIdx, port := range rule.Ports { + nsxGroups2, nsxRules2, err := service.expandRuleByPort(obj, rule, ruleIdx, port, portIdx, createdFor) + if err != nil { + return nil, nil, err } + nsxGroups = append(nsxGroups, nsxGroups2...) + nsxRules = append(nsxRules, nsxRules2...) } return nsxGroups, nsxRules, nil @@ -70,21 +66,20 @@ func (service *SecurityPolicyService) expandRule(obj *v1alpha1.SecurityPolicy, r func (service *SecurityPolicyService) expandRuleByPort(obj *v1alpha1.SecurityPolicy, rule *v1alpha1.SecurityPolicyRule, ruleIdx int, port v1alpha1.SecurityPolicyPort, portIdx int, createdFor string, ) ([]*model.Group, []*model.Rule, error) { - var err error - var startPort []nsxutil.PortAddress - var nsxGroups []*model.Group - var nsxRules []*model.Rule + var portInfos []*namedPortInfo // Use PortAddress to handle normal port and named port, if it only contains int value Port, // then it is a normal port. If it contains a list of IPs, it is a named port. if port.Port.Type == intstr.Int { - startPort = append(startPort, nsxutil.PortAddress{Port: port.Port.IntValue()}) + portInfo := newNamedPortInfo(port) + portInfo.idSuffix = fmt.Sprintf("%d%s0", portIdx, common.ConnectorUnderline) + portInfos = append(portInfos, portInfo) } else { // endPort can only be defined if port is also defined. Both ports must be numeric. if port.EndPort != 0 { return nil, nil, nsxutil.RestrictionError{Desc: "endPort can only be defined if port is also numeric."} } - startPort, err = service.resolveNamedPort(obj, rule, port) + startPort, err := service.resolveNamedPort(obj, rule, port) if err != nil { // In case there is no more valid ip set selected, so clear the stale ip set group in NSX if stale ips exist if errors.As(err, &nsxutil.NoEffectiveOption{}) { @@ -103,10 +98,18 @@ func (service *SecurityPolicyService) expandRuleByPort(obj *v1alpha1.SecurityPol } return nil, nil, err } + + for addrIdx, portAddr := range startPort { + portInfo := newNamedPortInfoByAddressPort(portAddr, port.Protocol) + portInfo.idSuffix = fmt.Sprintf("%d%s%d", portIdx, common.ConnectorUnderline, addrIdx) + portInfos = append(portInfos, portInfo) + } } - for portAddressIdx, portAddress := range startPort { - gs, r, err := service.expandRuleByService(obj, rule, ruleIdx, port, portIdx, portAddress, portAddressIdx, createdFor) + var nsxGroups []*model.Group + var nsxRules []*model.Rule + for _, portInfo := range portInfos { + gs, r, err := service.expandRuleByService(obj, rule, ruleIdx, createdFor, portInfo) if err != nil { return nil, nil, err } @@ -117,23 +120,23 @@ func (service *SecurityPolicyService) expandRuleByPort(obj *v1alpha1.SecurityPol } func (service *SecurityPolicyService) expandRuleByService(obj *v1alpha1.SecurityPolicy, rule *v1alpha1.SecurityPolicyRule, ruleIdx int, - port v1alpha1.SecurityPolicyPort, portIdx int, portAddress nsxutil.PortAddress, portAddressIdx int, createdFor string, + createdFor string, namedPort *namedPortInfo, ) ([]*model.Group, *model.Rule, error) { var nsxGroups []*model.Group - nsxRule, err := service.buildRuleBasicInfo(obj, rule, ruleIdx, portIdx, portAddressIdx, true, portAddress.Port, createdFor) + nsxRule, err := service.buildRuleBasicInfo(obj, rule, ruleIdx, createdFor, namedPort) if err != nil { return nil, nil, err } var ruleServiceEntries []*data.StructValue - serviceEntry := service.buildRuleServiceEntries(port, portAddress) + serviceEntry := buildRuleServiceEntries(namedPort.port) ruleServiceEntries = append(ruleServiceEntries, serviceEntry) nsxRule.ServiceEntries = ruleServiceEntries // If portAddress contains a list of IPs, we should build an ip set group for the rule. - if len(portAddress.IPs) > 0 { - ruleIPSetGroup := service.buildRuleIPSetGroup(obj, rule, nsxRule, portAddress.IPs, ruleIdx, createdFor) + if len(namedPort.ips) > 0 { + ruleIPSetGroup := service.buildRuleIPSetGroup(obj, rule, nsxRule, namedPort.ips, ruleIdx, createdFor) // In VPC network, NSGroup with IPAddressExpression type can be supported in VPC level as well. IPSetGroupPath, err := service.buildRuleIPSetGroupPath(obj, nsxRule) diff --git a/pkg/nsx/services/securitypolicy/expand_test.go b/pkg/nsx/services/securitypolicy/expand_test.go index 56c30485e..e61a4f7ed 100644 --- a/pkg/nsx/services/securitypolicy/expand_test.go +++ b/pkg/nsx/services/securitypolicy/expand_test.go @@ -1,22 +1,28 @@ package securitypolicy import ( + "context" "fmt" "reflect" "testing" gomonkey "github.com/agiledragon/gomonkey/v2" + "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" "github.com/vmware/vsphere-automation-sdk-go/runtime/data" "github.com/vmware/vsphere-automation-sdk-go/services/nsxt/model" core_v1 "k8s.io/api/core/v1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/tools/cache" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/vmware-tanzu/nsx-operator/pkg/apis/legacy/v1alpha1" "github.com/vmware-tanzu/nsx-operator/pkg/config" + mock_client "github.com/vmware-tanzu/nsx-operator/pkg/mock/controller-runtime/client" "github.com/vmware-tanzu/nsx-operator/pkg/nsx" "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common" ) @@ -236,3 +242,373 @@ func TestSecurityPolicyService_getPodSelectors(t *testing.T) { }) } } + +var secPolicy = &v1alpha1.SecurityPolicy{ + ObjectMeta: v1.ObjectMeta{Namespace: "ns1", Name: "p1", UID: "uid1"}, + Spec: v1alpha1.SecurityPolicySpec{ + Rules: []v1alpha1.SecurityPolicyRule{ + { + Name: "rule1", + Action: &allowAction, + Direction: &directionIn, + Ports: []v1alpha1.SecurityPolicyPort{}, + }, + { + Name: "rule2", + Action: &allowAction, + Direction: &directionIn, + Ports: []v1alpha1.SecurityPolicyPort{ + { + Protocol: "TCP", + Port: intstr.IntOrString{Type: intstr.Int, IntVal: 1000}, + }, + { + Protocol: "UDP", + Port: intstr.IntOrString{Type: intstr.Int, IntVal: 1234}, + EndPort: 1235, + }, + }, + }, + { + Action: &allowAction, + Direction: &directionIn, + Ports: []v1alpha1.SecurityPolicyPort{ + { + Protocol: "TCP", + Port: intstr.IntOrString{Type: intstr.String, StrVal: "http"}, // http port is 443 + }, + { + Protocol: "UDP", + Port: intstr.IntOrString{Type: intstr.Int, IntVal: 1236}, + EndPort: 1237, + }, + }, + }, + }, + AppliedTo: []v1alpha1.SecurityPolicyTarget{ + { + PodSelector: &v1.LabelSelector{ + MatchLabels: map[string]string{"pod_selector_1": "pod_value_1"}, + }, + }, + }, + }, +} + +func TestExpandRule(t *testing.T) { + ruleTagsFn := func(policyType string) []model.Tag { + return []model.Tag{ + {Scope: common.String("nsx-op/cluster"), Tag: common.String("")}, + {Scope: common.String("nsx-op/version"), Tag: common.String("1.0.0")}, + {Scope: common.String("nsx-op/namespace"), Tag: common.String("ns1")}, + {Scope: common.String("nsx-op/namespace_uid"), Tag: common.String("ns1-uid")}, + {Scope: common.String(fmt.Sprintf("nsx-op/%s_name", policyType)), Tag: common.String("p1")}, + {Scope: common.String(fmt.Sprintf("nsx-op/%s_uid", policyType)), Tag: common.String("uid1")}, + } + } + npRuleTags := ruleTagsFn("network_policy") + spRuleTags := ruleTagsFn("security_policy_cr") + + mockCtl := gomock.NewController(t) + k8sClient := mock_client.NewMockClient(mockCtl) + k8sClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Do( + func(_ context.Context, _ client.ObjectKey, obj client.Object, option ...client.GetOption) error { + namespace, _ := obj.(*core_v1.Namespace) + namespace.UID = types.UID("ns1-uid") + return nil + }).AnyTimes() + k8sClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Do( + func(_ context.Context, objList client.ObjectList, opts ...*client.ListOptions) error { + podsList, _ := objList.(*core_v1.PodList) + podsList.Items = []core_v1.Pod{ + { + Spec: core_v1.PodSpec{ + Containers: []core_v1.Container{ + { + Ports: []core_v1.ContainerPort{ + { + Name: "http", + Protocol: "TCP", + ContainerPort: 8080, + }, + }, + }, + }, + }, + Status: core_v1.PodStatus{ + Phase: core_v1.PodRunning, + PodIP: "1.1.1.1", + }, + }, + } + return nil + }, + ).AnyTimes() + + mockVPCService := common.MockVPCServiceProvider{VPCResource: &common.VPCResourceInfo{ + OrgID: "default", + ProjectID: "pro1", + VPCID: "vpc1", + }} + mockVPCService.On("ListVPCInfo", mock.Anything).Return(nil).Maybe() + + getTestIPsetGroup := func(id string, displayName string, ruleId string, isVPC bool, policyType string) *model.Group { + groupTags := []model.Tag{ + {Scope: common.String("nsx-op/group_type"), Tag: common.String("destination")}, + {Scope: common.String("nsx-op/rule_id"), Tag: common.String(ruleId)}, + {Scope: common.String("nsx-op/selector_hash"), Tag: common.String("2be88ca4242c76e8253ac62474851065032d6833")}, + } + groupTags = append(groupTags, ruleTagsFn(policyType)...) + if isVPC { + groupTags = append(groupTags, + model.Tag{Scope: common.String("nsx-op/nsx_share_created_for"), Tag: common.String("notShared")}) + } + + addresses := data.NewListValue() + addresses.Add(data.NewStringValue("1.1.1.1")) + + return &model.Group{ + Id: common.String(id), + DisplayName: common.String(displayName), + Tags: groupTags, + Expression: []*data.StructValue{ + data.NewStructValue( + "", + map[string]data.DataValue{ + "resource_type": data.NewStringValue("IPAddressExpression"), + "ip_addresses": addresses, + }, + ), + }, + } + } + + for _, tc := range []struct { + name string + vpcEnabled bool + ruleIdx int + createdFor string + expGroups []*model.Group + expRules []*model.Rule + expErr string + }{ + { + name: "VPC: rule without and ports for NetworkPolicy", + vpcEnabled: true, + ruleIdx: 0, + createdFor: common.ResourceTypeNetworkPolicy, + expRules: []*model.Rule{ + { + Id: common.String("p1_uid1_26e848bd_all"), + DisplayName: common.String("rule1"), + Direction: common.String(string("IN")), + SequenceNumber: Int64(int64(0)), + Action: common.String(string("ALLOW")), + Services: []string{"ANY"}, + Tags: npRuleTags, + }, + }, + }, { + name: "T1: rule without and ports for NetworkPolicy", + vpcEnabled: false, + ruleIdx: 0, + createdFor: common.ResourceTypeNetworkPolicy, + expRules: []*model.Rule{ + { + Id: common.String("np_uid1_26e848bd5724c09acc141b8c30b05d59533e694b_0_0_0"), + DisplayName: common.String("rule1"), + Direction: common.String(string("IN")), + SequenceNumber: Int64(int64(0)), + Action: common.String(string("ALLOW")), + Services: []string{"ANY"}, + Tags: npRuleTags, + }, + }, + }, { + name: "VPC: named rule without named ports for NetworkPolicy", + vpcEnabled: true, + ruleIdx: 1, + createdFor: common.ResourceTypeNetworkPolicy, + expRules: []*model.Rule{ + { + Id: common.String("p1_uid1_2a54787a_1000_1234.1235"), + DisplayName: common.String("rule2"), + Direction: common.String(string("IN")), + SequenceNumber: Int64(int64(1)), + Action: common.String(string("ALLOW")), + Services: []string{"ANY"}, + ServiceEntries: []*data.StructValue{getRuleServiceEntries(1000, 0, "TCP"), + getRuleServiceEntries(1234, 1235, "UDP")}, + Tags: npRuleTags, + }, + }, + }, { + name: "T1: named rule without named ports for NetworkPolicy", + vpcEnabled: false, + ruleIdx: 1, + createdFor: common.ResourceTypeNetworkPolicy, + expRules: []*model.Rule{ + { + Id: common.String("np_uid1_2a54787ab7a813c379d67051283b4de550d38360_1_0_0"), + DisplayName: common.String("rule2"), + Direction: common.String(string("IN")), + SequenceNumber: Int64(int64(1)), + Action: common.String(string("ALLOW")), + Services: []string{"ANY"}, + ServiceEntries: []*data.StructValue{getRuleServiceEntries(1000, 0, "TCP"), + getRuleServiceEntries(1234, 1235, "UDP")}, + Tags: npRuleTags, + }, + }, + }, { + name: "VPC: rule with named ports for NetworkPolicy", + vpcEnabled: true, + ruleIdx: 2, + createdFor: common.ResourceTypeNetworkPolicy, + expGroups: []*model.Group{ + getTestIPsetGroup("p1_uid1_94b44028_8080_ipset", "TCP.http_UDP.1236.1237.TCP.8080_ingress_allow_ipset", "p1_uid1_94b44028", true, "network_policy"), + }, + expRules: []*model.Rule{ + { + Id: common.String("p1_uid1_94b44028_8080"), + DisplayName: common.String("TCP.http_UDP.1236.1237.TCP.8080_ingress_allow"), + Direction: common.String("IN"), + SequenceNumber: Int64(int64(2)), + Action: common.String("ALLOW"), + Services: []string{"ANY"}, + ServiceEntries: []*data.StructValue{getRuleServiceEntries(8080, 0, "TCP")}, + Tags: npRuleTags, + DestinationGroups: []string{"/orgs/default/projects/pro1/vpcs/vpc1/groups/p1_uid1_94b44028_8080_ipset"}, + }, { + Id: common.String("p1_uid1_94b44028_1236.1237"), + DisplayName: common.String("TCP.http_UDP.1236.1237.UDP.1236.1237_ingress_allow"), + Direction: common.String("IN"), + SequenceNumber: Int64(int64(2)), + Action: common.String("ALLOW"), + Services: []string{"ANY"}, + ServiceEntries: []*data.StructValue{getRuleServiceEntries(1236, 1237, "UDP")}, + Tags: npRuleTags, + }, + }, + }, { + name: "T1: rule with named ports for NetworkPolicy", + vpcEnabled: false, + ruleIdx: 2, + createdFor: common.ResourceTypeNetworkPolicy, + expGroups: []*model.Group{ + getTestIPsetGroup("np_uid1_94b44028488f3e719879abbc27c75e5cb44872b7_2_0_0_ipset", "TCP.http_UDP.1236.1237.TCP.8080_ingress_allow_ipset", "np_uid1_94b44028488f3e719879abbc27c75e5cb44872b7_2", false, "network_policy"), + }, + expRules: []*model.Rule{ + { + Id: common.String("np_uid1_94b44028488f3e719879abbc27c75e5cb44872b7_2_0_0"), + DisplayName: common.String("TCP.http_UDP.1236.1237.TCP.8080_ingress_allow"), + Direction: common.String("IN"), + SequenceNumber: Int64(int64(2)), + Action: common.String("ALLOW"), + Services: []string{"ANY"}, + ServiceEntries: []*data.StructValue{getRuleServiceEntries(8080, 0, "TCP")}, + Tags: npRuleTags, + DestinationGroups: []string{"/infra/domains//groups/np_uid1_94b44028488f3e719879abbc27c75e5cb44872b7_2_0_0_ipset"}, + }, { + Id: common.String("np_uid1_94b44028488f3e719879abbc27c75e5cb44872b7_2_1_0"), + DisplayName: common.String("TCP.http_UDP.1236.1237.UDP.1236.1237_ingress_allow"), + Direction: common.String("IN"), + SequenceNumber: Int64(int64(2)), + Action: common.String("ALLOW"), + Services: []string{"ANY"}, + ServiceEntries: []*data.StructValue{getRuleServiceEntries(1236, 1237, "UDP")}, + Tags: npRuleTags, + }, + }, + }, { + name: "VPC: rule with named ports for SecurityPolicy", + vpcEnabled: true, + ruleIdx: 2, + createdFor: common.ResourceTypeSecurityPolicy, + expGroups: []*model.Group{ + getTestIPsetGroup("p1_uid1_94b44028_8080_ipset", "TCP.http_UDP.1236.1237.TCP.8080_ingress_allow_ipset", "p1_uid1_94b44028", true, "security_policy_cr"), + }, + expRules: []*model.Rule{ + { + Id: common.String("p1_uid1_94b44028_8080"), + DisplayName: common.String("TCP.http_UDP.1236.1237.TCP.8080_ingress_allow"), + Direction: common.String("IN"), + SequenceNumber: Int64(int64(2)), + Action: common.String("ALLOW"), + Services: []string{"ANY"}, + ServiceEntries: []*data.StructValue{getRuleServiceEntries(8080, 0, "TCP")}, + Tags: spRuleTags, + DestinationGroups: []string{"/orgs/default/projects/pro1/vpcs/vpc1/groups/p1_uid1_94b44028_8080_ipset"}, + }, { + Id: common.String("p1_uid1_94b44028_1236.1237"), + DisplayName: common.String("TCP.http_UDP.1236.1237.UDP.1236.1237_ingress_allow"), + Direction: common.String("IN"), + SequenceNumber: Int64(int64(2)), + Action: common.String("ALLOW"), + Services: []string{"ANY"}, + ServiceEntries: []*data.StructValue{getRuleServiceEntries(1236, 1237, "UDP")}, + Tags: spRuleTags, + }, + }, + }, { + name: "T1: rule with named ports for SecurityPolicy", + vpcEnabled: false, + ruleIdx: 2, + createdFor: common.ResourceTypeSecurityPolicy, + expGroups: []*model.Group{ + getTestIPsetGroup("sp_uid1_94b44028488f3e719879abbc27c75e5cb44872b7_2_0_0_ipset", "TCP.http_UDP.1236.1237.TCP.8080_ingress_allow_ipset", "sp_uid1_94b44028488f3e719879abbc27c75e5cb44872b7_2", false, "security_policy_cr"), + }, + expRules: []*model.Rule{ + { + Id: common.String("sp_uid1_94b44028488f3e719879abbc27c75e5cb44872b7_2_0_0"), + DisplayName: common.String("TCP.http_UDP.1236.1237.TCP.8080_ingress_allow"), + Direction: common.String("IN"), + SequenceNumber: Int64(int64(2)), + Action: common.String("ALLOW"), + Services: []string{"ANY"}, + ServiceEntries: []*data.StructValue{getRuleServiceEntries(8080, 0, "TCP")}, + Tags: spRuleTags, + DestinationGroups: []string{"/infra/domains//groups/sp_uid1_94b44028488f3e719879abbc27c75e5cb44872b7_2_0_0_ipset"}, + }, { + Id: common.String("sp_uid1_94b44028488f3e719879abbc27c75e5cb44872b7_2_1_0"), + DisplayName: common.String("TCP.http_UDP.1236.1237.UDP.1236.1237_ingress_allow"), + Direction: common.String("IN"), + SequenceNumber: Int64(int64(2)), + Action: common.String("ALLOW"), + Services: []string{"ANY"}, + ServiceEntries: []*data.StructValue{getRuleServiceEntries(1236, 1237, "UDP")}, + Tags: spRuleTags, + }, + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + svc := &SecurityPolicyService{ + Service: common.Service{ + Client: k8sClient, + NSXConfig: &config.NSXOperatorConfig{ + CoeConfig: &config.CoeConfig{EnableVPCNetwork: tc.vpcEnabled}, + }, + }, + vpcService: &mockVPCService, + } + rule := secPolicy.Spec.Rules[tc.ruleIdx] + nsxGroups, nsxRules, err := svc.expandRule(secPolicy, &rule, tc.ruleIdx, tc.createdFor) + if tc.expErr != "" { + require.EqualError(t, err, tc.expErr) + } else { + require.NoError(t, err) + } + assert.ElementsMatch(t, tc.expGroups, nsxGroups) + assert.ElementsMatch(t, tc.expRules, nsxRules) + }) + } +} + +func getRuleServiceEntries(portStart, portEnd int, protocol string) *data.StructValue { + return buildRuleServiceEntries(v1alpha1.SecurityPolicyPort{ + Protocol: core_v1.Protocol(protocol), + Port: intstr.FromInt32(int32(portStart)), + EndPort: portEnd, + }) +} diff --git a/pkg/nsx/services/securitypolicy/firewall_test.go b/pkg/nsx/services/securitypolicy/firewall_test.go index 38d268ee1..7ef8532ec 100644 --- a/pkg/nsx/services/securitypolicy/firewall_test.go +++ b/pkg/nsx/services/securitypolicy/firewall_test.go @@ -1073,11 +1073,11 @@ func TestCreateOrUpdateSecurityPolicy(t *testing.T) { mockVPCService := common.MockVPCServiceProvider{} fakeService.vpcService = &mockVPCService - podSelectorRule0IDPort000 := fakeService.buildExpandedRuleID(&spWithPodSelector, 0, 0, 0, false, -1, common.ResourceTypeSecurityPolicy) - podSelectorRule1IDPort000 := fakeService.buildExpandedRuleID(&spWithPodSelector, 1, 0, 0, false, -1, common.ResourceTypeSecurityPolicy) + podSelectorRule0IDPort000 := fakeService.buildExpandedRuleID(&spWithPodSelector, 0, common.ResourceTypeSecurityPolicy, nil) + podSelectorRule1IDPort000 := fakeService.buildExpandedRuleID(&spWithPodSelector, 1, common.ResourceTypeSecurityPolicy, nil) - podSelectorRule0Name00, _ := fakeService.buildRuleDisplayName(&spWithPodSelector.Spec.Rules[0], 0, false, -1, common.ResourceTypeSecurityPolicy) - podSelectorRule1Name00, _ := fakeService.buildRuleDisplayName(&spWithPodSelector.Spec.Rules[1], 0, false, -1, common.ResourceTypeSecurityPolicy) + podSelectorRule0Name00, _ := fakeService.buildRuleDisplayName(&spWithPodSelector.Spec.Rules[0], common.ResourceTypeSecurityPolicy, nil) + podSelectorRule1Name00, _ := fakeService.buildRuleDisplayName(&spWithPodSelector.Spec.Rules[1], common.ResourceTypeSecurityPolicy, nil) type args struct { spObj *v1alpha1.SecurityPolicy