From a8e96f35729d4a6641e353f8f0a9dde5fb562c63 Mon Sep 17 00:00:00 2001 From: Dyanngg Date: Thu, 11 May 2023 16:06:05 -0700 Subject: [PATCH] Address comments Signed-off-by: Dyanngg --- .../antrea-multicluster-leader-global.yml | 12 ++-- ...cluster.crd.antrea.io_resourceexports.yaml | 6 +- ...cluster.crd.antrea.io_resourceimports.yaml | 6 +- pkg/apis/crd/v1alpha1/types.go | 1 + .../networkpolicy_controller_test.go | 4 +- pkg/controller/networkpolicy/validate.go | 26 ++++++--- pkg/controller/networkpolicy/validate_test.go | 57 +++++++++++++++++++ test/e2e/antreapolicy_test.go | 27 +++++++-- test/e2e/k8s_util.go | 2 +- test/e2e/utils/cnp_spec_builder.go | 4 +- 10 files changed, 121 insertions(+), 24 deletions(-) diff --git a/multicluster/build/yamls/antrea-multicluster-leader-global.yml b/multicluster/build/yamls/antrea-multicluster-leader-global.yml index 88b8ce89905..5710616f021 100644 --- 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 index 18ca05f3122..81426d079cd 100644 --- 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 index ae5b242172f..f33a63e00d2 100644 --- 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 index a6bf0d61a5c..ba8eaddcec0 100644 --- 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 index a5ac1b77c4c..67ae6b4f46a 100644 --- 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 index a76a4188de7..db2a09c2582 100644 --- 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 index dcddd52f23c..a12b76e2727 100644 --- 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 index f9bb3239810..b720e8e69d7 100644 --- 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 index ce3c1cd8a69..7d0d8bee891 100644 --- 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 index 9f1922f68ed..f4d2e5aef5e 100644 --- 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,