Address comments

Signed-off-by: Dyanngg <dingyang@vmware.com>
antrea-io · May 11, 2023 · a8e96f3 · a8e96f3
1 parent 072a506
commit a8e96f3
Show file tree

Hide file tree

Showing 10 changed files with 121 additions and 24 deletions.
diff --git a/multicluster/build/yamls/antrea-multicluster-leader-global.yml b/multicluster/build/yamls/antrea-multicluster-leader-global.yml
@@ -1127,7 +1127,8 @@ spec:
                                 type: string
                               sourceEndPort:
                                 description: SourceEndPort defines the end of the
-                                  source port range, inclusive.
+                                  source port range, inclusive. It can only be specified
+                                  when `sourcePort` is specified.
                                 format: int32
                                 type: integer
                               sourcePort:
@@ -2026,7 +2027,8 @@ spec:
                                 type: string
                               sourceEndPort:
                                 description: SourceEndPort defines the end of the
-                                  source port range, inclusive.
+                                  source port range, inclusive. It can only be specified
+                                  when `sourcePort` is specified.
                                 format: int32
                                 type: integer
                               sourcePort:
@@ -3841,7 +3843,8 @@ spec:
                                 type: string
                               sourceEndPort:
                                 description: SourceEndPort defines the end of the
-                                  source port range, inclusive.
+                                  source port range, inclusive. It can only be specified
+                                  when `sourcePort` is specified.
                                 format: int32
                                 type: integer
                               sourcePort:
@@ -4740,7 +4743,8 @@ spec:
                                 type: string
                               sourceEndPort:
                                 description: SourceEndPort defines the end of the
-                                  source port range, inclusive.
+                                  source port range, inclusive. It can only be specified
+                                  when `sourcePort` is specified.
                                 format: int32
                                 type: integer
                               sourcePort:

diff --git a/multicluster/config/crd/bases/multicluster.crd.antrea.io_resourceexports.yaml b/multicluster/config/crd/bases/multicluster.crd.antrea.io_resourceexports.yaml
@@ -822,7 +822,8 @@ spec:
                                 type: string
                               sourceEndPort:
                                 description: SourceEndPort defines the end of the
-                                  source port range, inclusive.
+                                  source port range, inclusive. It can only be specified
+                                  when `sourcePort` is specified.
                                 format: int32
                                 type: integer
                               sourcePort:
@@ -1721,7 +1722,8 @@ spec:
                                 type: string
                               sourceEndPort:
                                 description: SourceEndPort defines the end of the
-                                  source port range, inclusive.
+                                  source port range, inclusive. It can only be specified
+                                  when `sourcePort` is specified.
                                 format: int32
                                 type: integer
                               sourcePort:

diff --git a/multicluster/config/crd/bases/multicluster.crd.antrea.io_resourceimports.yaml b/multicluster/config/crd/bases/multicluster.crd.antrea.io_resourceimports.yaml
@@ -820,7 +820,8 @@ spec:
                                 type: string
                               sourceEndPort:
                                 description: SourceEndPort defines the end of the
-                                  source port range, inclusive.
+                                  source port range, inclusive. It can only be specified
+                                  when `sourcePort` is specified.
                                 format: int32
                                 type: integer
                               sourcePort:
@@ -1719,7 +1720,8 @@ spec:
                                 type: string
                               sourceEndPort:
                                 description: SourceEndPort defines the end of the
-                                  source port range, inclusive.
+                                  source port range, inclusive. It can only be specified
+                                  when `sourcePort` is specified.
                                 format: int32
                                 type: integer
                               sourcePort:

diff --git a/pkg/apis/crd/v1alpha1/types.go b/pkg/apis/crd/v1alpha1/types.go
@@ -588,6 +588,7 @@ type NetworkPolicyPort struct {
 	// +optional
 	SourcePort *int32 `json:"sourcePort,omitempty"`
 	// SourceEndPort defines the end of the source port range, inclusive.
+	// It can only be specified when `sourcePort` is specified.
 	// +optional
 	SourceEndPort *int32 `json:"sourceEndPort,omitempty"`
 }

diff --git a/pkg/controller/networkpolicy/networkpolicy_controller_test.go b/pkg/controller/networkpolicy/networkpolicy_controller_test.go
@@ -75,7 +75,9 @@ var (
 	int81   = intstr.FromInt(81)
 	int1000 = intstr.FromInt(1000)
 
-	int32For1999 = int32(1999)
+	int32For1999  = int32(1999)
+	int32For32220 = int32(32220)
+	int32For32230 = int32(32230)
 
 	strHTTP = intstr.FromString("http")
 )

diff --git a/pkg/controller/networkpolicy/validate.go b/pkg/controller/networkpolicy/validate.go
@@ -285,17 +285,27 @@ func (v *antreaPolicyValidator) validatePort(ingress, egress []crdv1alpha1.Rule)
 	isValid := func(rules []crdv1alpha1.Rule) error {
 		for _, rule := range rules {
 			for _, port := range rule.Ports {
-				if port.EndPort == nil {
+				if port.EndPort == nil && port.SourceEndPort == nil {
 					continue
 				}
-				if port.Port == nil {
-					return fmt.Errorf("if `endPort` is specified `port` must be specified")
+				if port.EndPort != nil {
+					if port.Port == nil {
+						return fmt.Errorf("if `endPort` is specified `port` must be specified")
+					}
+					if port.Port.Type == intstr.String {
+						return fmt.Errorf("if `port` is a string `endPort` cannot be specified")
+					}
+					if *port.EndPort < port.Port.IntVal {
+						return fmt.Errorf("`endPort` should be greater than or equal to `port`")
+					}
 				}
-				if port.Port.Type == intstr.String {
-					return fmt.Errorf("if `port` is a string `endPort` cannot be specified")
-				}
-				if *port.EndPort < port.Port.IntVal {
-					return fmt.Errorf("`endPort` should be greater than or equal to `port`")
+				if port.SourceEndPort != nil {
+					if port.SourcePort == nil {
+						return fmt.Errorf("if `sourceEndPort` is specified `sourcePort` must be specified")
+					}
+					if *port.SourceEndPort < *port.SourcePort {
+						return fmt.Errorf("`sourceEndPort` should be greater than or equal to `sourcePort`")
+					}
 				}
 			}
 		}

diff --git a/pkg/controller/networkpolicy/validate_test.go b/pkg/controller/networkpolicy/validate_test.go
@@ -893,6 +893,34 @@ func TestValidateAntreaPolicy(t *testing.T) {
 			},
 			expectedReason: "if `endPort` is specified `port` must be specified",
 		},
+		{
+			name: "acnp-sourceendport-without-sourceport-in-ports",
+			policy: &crdv1alpha1.ClusterNetworkPolicy{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "acnp-sourceendport-without-port-in-ports",
+				},
+				Spec: crdv1alpha1.ClusterNetworkPolicySpec{
+					AppliedTo: []crdv1alpha1.AppliedTo{
+						{
+							NamespaceSelector: &metav1.LabelSelector{
+								MatchLabels: map[string]string{"foo1": "bar1"},
+							},
+						},
+					},
+					Ingress: []crdv1alpha1.Rule{
+						{
+							Action: &allowAction,
+							Ports: []crdv1alpha1.NetworkPolicyPort{
+								{
+									SourceEndPort: &int32For32230,
+								},
+							},
+						},
+					},
+				},
+			},
+			expectedReason: "if `sourceEndPort` is specified `sourcePort` must be specified",
+		},
 		{
 			name: "acnp-endport-smaller-port-in-ports",
 			policy: &crdv1alpha1.ClusterNetworkPolicy{
@@ -922,6 +950,35 @@ func TestValidateAntreaPolicy(t *testing.T) {
 			},
 			expectedReason: "`endPort` should be greater than or equal to `port`",
 		},
+		{
+			name: "acnp-sourceendport-smaller-sourceport-in-ports",
+			policy: &crdv1alpha1.ClusterNetworkPolicy{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "acnp-sourceendport-smaller-port-in-ports",
+				},
+				Spec: crdv1alpha1.ClusterNetworkPolicySpec{
+					AppliedTo: []crdv1alpha1.AppliedTo{
+						{
+							NamespaceSelector: &metav1.LabelSelector{
+								MatchLabels: map[string]string{"foo1": "bar1"},
+							},
+						},
+					},
+					Ingress: []crdv1alpha1.Rule{
+						{
+							Action: &allowAction,
+							Ports: []crdv1alpha1.NetworkPolicyPort{
+								{
+									SourcePort:    &int32For32230,
+									SourceEndPort: &int32For32220,
+								},
+							},
+						},
+					},
+				},
+			},
+			expectedReason: "`sourceEndPort` should be greater than or equal to `sourcePort`",
+		},
 		{
 			name: "acnp-named-port-with-endport-in-ports",
 			policy: &crdv1alpha1.ClusterNetworkPolicy{

diff --git a/test/e2e/antreapolicy_test.go b/test/e2e/antreapolicy_test.go
@@ -580,14 +580,24 @@ func testACNPSourcePort(t *testing.T) {
 	builder = builder.SetName("acnp-source-port").
 		SetPriority(1.0).
 		SetAppliedToGroup([]ACNPAppliedToSpec{{PodSelector: map[string]string{"pod": "a"}}})
-	builder.AddIngressForSrcPort(ProtocolTCP, &portStart, &portEnd, nil, nil, nil, nil, nil, map[string]string{"pod": "b"}, map[string]string{"ns": namespaces["x"]},
+	builder.AddIngressForSrcPort(ProtocolTCP, nil, nil, &portStart, &portEnd, nil, nil, nil, nil, nil, map[string]string{"pod": "b"}, map[string]string{"ns": namespaces["x"]},
+		nil, nil, false, nil, crdv1alpha1.RuleActionDrop, "", "", nil)
+
+	updatedACNPBuilder := &ClusterNetworkPolicySpecBuilder{}
+	updatedACNPBuilder = updatedACNPBuilder.SetName("acnp-source-port").
+		SetPriority(1.0).
+		SetAppliedToGroup([]ACNPAppliedToSpec{{PodSelector: map[string]string{"pod": "a"}}})
+	updatedACNPBuilder.AddIngressForSrcPort(ProtocolTCP, &p80, nil, &portStart, &portEnd, nil, nil, nil, nil, nil, map[string]string{"pod": "b"}, map[string]string{"ns": namespaces["x"]},
 		nil, nil, false, nil, crdv1alpha1.RuleActionDrop, "", "", nil)
 
 	reachability := NewReachability(allPods, Connected)
 	reachability.Expect(Pod(namespaces["x"]+"/b"), Pod(namespaces["x"]+"/a"), Dropped)
 	reachability.Expect(Pod(namespaces["x"]+"/b"), Pod(namespaces["y"]+"/a"), Dropped)
 	reachability.Expect(Pod(namespaces["x"]+"/b"), Pod(namespaces["z"]+"/a"), Dropped)
-	testStep := []*TestStep{
+	// After adding the dst port constraint of port 80, traffic on port 81 should not be affected.
+	updatedReachability := NewReachability(allPods, Connected)
+
+	testSteps := []*TestStep{
 		{
 			"Port 80",
 			reachability,
@@ -597,9 +607,18 @@ func testACNPSourcePort(t *testing.T) {
 			0,
 			nil,
 		},
+		{
+			"Port 81",
+			updatedReachability,
+			[]metav1.Object{updatedACNPBuilder.Get()},
+			[]int32{81},
+			ProtocolTCP,
+			0,
+			nil,
+		},
 	}
 	testCase := []*TestCase{
-		{"ACNP Drop X/B to A based on source port", testStep},
+		{"ACNP Drop X/B to A based on source port", testSteps},
 	}
 	executeTests(t, testCase)
 }
@@ -4323,6 +4342,7 @@ func TestAntreaPolicy(t *testing.T) {
 		t.Run("Case=ACNPDropEgressSCTP", func(t *testing.T) { testACNPDropEgress(t, ProtocolSCTP) })
 		t.Run("Case=ACNPDropIngressInNamespace", func(t *testing.T) { testACNPDropIngressInSelectedNamespace(t) })
 		t.Run("Case=ACNPPortRange", func(t *testing.T) { testACNPPortRange(t) })
+		t.Run("Case=ACNPSourcePort", func(t *testing.T) { testACNPSourcePort(t) })
 		t.Run("Case=ACNPRejectEgress", func(t *testing.T) { testACNPRejectEgress(t) })
 		t.Run("Case=ACNPRejectIngress", func(t *testing.T) { testACNPRejectIngress(t, ProtocolTCP) })
 		t.Run("Case=ACNPRejectIngressUDP", func(t *testing.T) { testACNPRejectIngress(t, ProtocolUDP) })
@@ -4336,7 +4356,6 @@ func TestAntreaPolicy(t *testing.T) {
 		t.Run("Case=ACNPPriorityConflictingRule", func(t *testing.T) { testACNPPriorityConflictingRule(t) })
 		t.Run("Case=ACNPRulePriority", func(t *testing.T) { testACNPRulePriority(t) })
 		t.Run("Case=ANPPortRange", func(t *testing.T) { testANPPortRange(t) })
-		t.Run("Case=ACNPSourcePort", func(t *testing.T) { testACNPSourcePort(t) })
 		t.Run("Case=ANPBasic", func(t *testing.T) { testANPBasic(t) })
 		t.Run("Case=testANPMultipleAppliedToSingleRule", func(t *testing.T) { testANPMultipleAppliedTo(t, data, true) })
 		t.Run("Case=testANPMultipleAppliedToMultipleRules", func(t *testing.T) { testANPMultipleAppliedTo(t, data, false) })

diff --git a/test/e2e/k8s_util.go b/test/e2e/k8s_util.go
@@ -148,7 +148,7 @@ func (k *KubernetesUtils) getTCPv4SourcePortRangeFromPod(podNamespace, podNameLa
 		log.Errorf("Failed to retrieve TCP source port range for Pod %s/%s", podNamespace, podNameLabel)
 		return 0, 0, err
 	}
-	ports := strings.Split(stdout, "	")
+	ports := strings.Fields(stdout)
 	if len(ports) < 2 {
 		log.Errorf("Failed to retrieve TCP source port range for Pod %s/%s", podNamespace, podNameLabel)
 		return 0, 0, err

diff --git a/test/e2e/utils/cnp_spec_builder.go b/test/e2e/utils/cnp_spec_builder.go
@@ -179,7 +179,7 @@ func (b *ClusterNetworkPolicySpecBuilder) AddIngress(protoc AntreaPolicyProtocol
 //
 //	all conflicting PRs are merged.
 func (b *ClusterNetworkPolicySpecBuilder) AddIngressForSrcPort(protoc AntreaPolicyProtocol,
-	srcPort *int32, endPort, icmpType, icmpCode, igmpType *int32,
+	port, endPort, srcPort, endSrcPort, icmpType, icmpCode, igmpType *int32,
 	groupAddress, cidr *string, podSelector map[string]string, nsSelector map[string]string,
 	podSelectorMatchExp []metav1.LabelSelectorRequirement, nsSelectorMatchExp []metav1.LabelSelectorRequirement, selfNS bool,
 	ruleAppliedToSpecs []ACNPAppliedToSpec, action crdv1alpha1.RuleAction, ruleClusterGroup, name string, serviceAccount *crdv1alpha1.NamespacedName) *ClusterNetworkPolicySpecBuilder {
@@ -232,7 +232,7 @@ func (b *ClusterNetworkPolicySpecBuilder) AddIngressForSrcPort(protoc AntreaPoli
 			ServiceAccount:    serviceAccount,
 		}}
 	}
-	ports, protocols := GenPortsOrProtocols(protoc, nil, nil, nil, srcPort, endPort, icmpType, icmpCode, igmpType, groupAddress)
+	ports, protocols := GenPortsOrProtocols(protoc, port, nil, endPort, srcPort, endSrcPort, icmpType, icmpCode, igmpType, groupAddress)
 	newRule := crdv1alpha1.Rule{
 		From:      policyPeer,
 		Ports:     ports,