named-port bug fix and tests (#412)

* bug fix and tests

* Update pkg/netpol/connlist/internal/ingressanalyzer/ingress_analyzer.go

Co-authored-by: Adi Sosnovich <[email protected]>

* typo fix

* reverting some changes (1 &3 in PR desc.)

* deleting un-necessary tests (results same as main)

* comments

---------

Co-authored-by: Adi Sosnovich <[email protected]>

pkg/netpol/connlist/connlist.go

@@ -347,9 +347,9 @@ func (c *connection) ProtocolsAndPorts() map[v1.Protocol][]common.PortRange {
 // returns a *common.ConnectionSet from Peer2PeerConnection data
 func GetConnectionSetFromP2PConnection(c Peer2PeerConnection) *common.ConnectionSet {
 	protocolsToPortSetMap := make(map[v1.Protocol]*common.PortSet, len(c.ProtocolsAndPorts()))
-	for protocol, portRageArr := range c.ProtocolsAndPorts() {
+	for protocol, portRangeArr := range c.ProtocolsAndPorts() {
 		protocolsToPortSetMap[protocol] = common.MakePortSet(false)
-		for _, p := range portRageArr {
+		for _, p := range portRangeArr {
 			protocolsToPortSetMap[protocol].AddPortRange(p.Start(), p.End())
 		}
 	}

pkg/netpol/connlist/connlist_test.go

@@ -1124,4 +1124,19 @@ var goodPathTests = []struct {
 		exposureAnalysis: true,
 		outputFormats:    []string{output.DefaultFormat},
 	},
+	{
+		// the netpol allows connection to pod-a on a named port "newport" with "protocol UDP",
+		// but since the configuration of "pod-a" contains a port with same name but a different protocol,
+		// i.e. there is no matching named port in the pod's configuration; then the output does not contain
+		// a connection from new-pod to pod-a
+		testDirName:   "netpol_named_port_test",
+		outputFormats: []string{output.DefaultFormat},
+	},
+	{
+		// the netpol allows connection to "pod-b" on multiple named-ports;
+		// only some of the ports have a matching named-port + protocol in the pod's configuration
+		// so we see only the successfully converted ports in the connlist output
+		testDirName:   "netpol_named_port_test_2",
+		outputFormats: []string{output.DefaultFormat},
+	},
 }

pkg/netpol/connlist/internal/ingressanalyzer/ingress_analyzer.go

@@ -360,11 +360,12 @@ func (ia *IngressAnalyzer) getIngressPeerConnection(peer eval.Peer, actualServic
 	for _, peerPortToFind := range peerPortsToFind {
 		portNum := peerPortToFind.IntValue()
 		if peerPortToFind.StrVal != "" { // if the port we are searching for is namedPort
-			portInt, err := ia.pe.ConvertPeerNamedPort(peerPortToFind.StrVal, peer)
+			protocol, portInt, err := ia.pe.ConvertPeerNamedPort(peerPortToFind.StrVal, peer)
 			if err != nil {
 				return nil, err
 			}
-			if portInt < 0 { // no matching port for the given named port
+			// only TCP ports are acceptable for Ingress resource
+			if protocol != string(corev1.ProtocolTCP) || portInt < 0 { // no matching port for the given named port
 				continue
 			}
 			portNum = int(portInt)

pkg/netpol/eval/internal/k8s/netpol.go

@@ -72,34 +72,41 @@ func getProtocolStr(p *v1.Protocol) string {
 	return string(*p)
 }
 
-func (np *NetworkPolicy) convertNamedPort(namedPort string, pod *Pod) int32 {
-	return pod.ConvertPodNamedPort(namedPort)
-}
-
-// getPortsRange returns the start and end port numbers given input port, endPort and dest peer
-// and the portName if it is a named port
-// if input port is a named port, and the dst peer is nil or  does not have a matching named port defined, return
-// an empty range represented by (-1,-1)
-func (np *NetworkPolicy) getPortsRange(port *intstr.IntOrString, endPort *int32, dst Peer) (startNum, endNum int32,
-	namedPort string, err error) {
-	var start, end int32
-	portName := ""
-	if port.Type == intstr.String {
+// getPortsRange given a rule port and dest peer, returns: the start and end port numbers,
+// or the port name if it is a named port
+// if input port is a named port, and the dst peer is nil or does not have a matching named port defined, returns
+// an empty range represented by (-1,-1) with the named port string
+func (np *NetworkPolicy) getPortsRange(rulePort netv1.NetworkPolicyPort, dst Peer) (start, end int32,
+	portName string, err error) {
+	if rulePort.Port.Type == intstr.String { // rule.Port is namedPort
+		ruleProtocol := getProtocolStr(rulePort.Protocol)
+		portName = rulePort.Port.StrVal
 		if dst == nil {
-			return common.NoPort, common.NoPort, port.StrVal, nil
+			// dst is nil, so the namedPort can not be converted, return string port name
+			return common.NoPort, common.NoPort, portName, nil
 		}
 		if dst.PeerType() != PodType {
+			// namedPort is not defined for IP-blocks
 			return start, end, "", np.netpolErr(netpolerrors.NamedPortErrTitle, netpolerrors.ConvertNamedPortErrStr)
 		}
-		portName = port.StrVal
-		portNum := np.convertNamedPort(portName, dst.GetPeerPod())
-		start = portNum
-		end = portNum
-	} else {
-		start = port.IntVal
+		podProtocol, podPortNum := dst.GetPeerPod().ConvertPodNamedPort(portName)
+		if podProtocol == "" && podPortNum == common.NoPort {
+			// there is no match for the namedPort in the configuration of the pod, return the portName string as "ports range"
+			return common.NoPort, common.NoPort, portName, nil
+		}
+		if podProtocol != ruleProtocol {
+			// the pod has a matching namedPort, but not on same protocol as the rule's protocol; so it can not be converted,
+			// return the string named-port as the "ports range"
+			return common.NoPort, common.NoPort, portName, nil
+		}
+		// else, found match for the rule's named-port in the pod's ports, so it may be converted to port number
+		start = podPortNum
+		end = podPortNum
+	} else { // rule.Port is number
+		start = rulePort.Port.IntVal
 		end = start
-		if endPort != nil {
-			end = *endPort
+		if rulePort.EndPort != nil {
+			end = *rulePort.EndPort
 		}
 	}
 	return start, end, portName, nil
@@ -124,17 +131,25 @@ func (np *NetworkPolicy) ruleConnections(rulePorts []netv1.NetworkPolicyPort, ds
 		if rulePorts[i].Port == nil {
 			ports = common.MakePortSet(true)
 		} else {
-			startPort, endPort, portName, err := np.getPortsRange(rulePorts[i].Port, rulePorts[i].EndPort, dst)
+			startPort, endPort, portName, err := np.getPortsRange(rulePorts[i], dst)
 			if err != nil {
 				return res, err
 			}
-			if (dst == nil || isPeerRepresentative(dst)) && portName != "" {
+			// valid returned values from `getsPortsRange` :
+			// 1. empty-numbered-range with the string namedPort, if the rule has a named-port which is not (or cannot be) converted to a
+			// numbered-range by the dst's ports.
+			//  2. non empty-range when the rule ports are numbered or the named-port was converted
+			if (dst == nil || isPeerRepresentative(dst)) && isEmptyPortRange(startPort, endPort) && portName != "" {
 				// this func may be called:
-				// - for computing cluster-wide exposure of the policy (dst is nil);
-				// - in-order to get a connection from a real workload to a representative dst.
-				// - in order to get a connection from any pod to a real dst or an ip dst (will not get to this if)
+				// 1- for computing cluster-wide exposure of the policy (dst is nil);
+				// 2- in-order to get a connection from a real workload to a representative dst.
 				// in both first cases, we can't convert the named port to its number, like when dst peer is a real
 				// pod from manifests, so we use the named-port as is.
+				// 3- in order to get a connection from any pod to a real dst.
+				// in the third case the namedPort of the policy rule may not have a match in the Pod's configuration,
+				// so it will be ignored (the pod has no matching named-port in its configuration - unknown connection is not allowed)
+				// 4- in order to get a connection from any pod to an ip dst (will not get here, as named ports are not defined for ip-blocks)
+
 				// adding portName string to the portSet
 				ports.AddPort(intstr.FromString(portName))
 			}
@@ -167,7 +182,7 @@ func (np *NetworkPolicy) ruleConnsContain(rulePorts []netv1.NetworkPolicyPort, p
 		if rulePorts[i].Port == nil { // If this field is not provided, this matches all port names and numbers.
 			return true, nil
 		}
-		startPort, endPort, _, err := np.getPortsRange(rulePorts[i].Port, rulePorts[i].EndPort, dst)
+		startPort, endPort, _, err := np.getPortsRange(rulePorts[i], dst)
 		if err != nil {
 			return false, err
 		}

pkg/netpol/eval/internal/k8s/netpol_test.go

@@ -87,7 +87,8 @@ var (
 func TestNetworkPolicyPortAnalysis(t *testing.T) {
 	// tested function: func ruleConnections(rulePorts []netv1.NetworkPolicyPort, dst Peer) ConnectionSet
 	dst := PodPeer{Pod: &Pod{Name: "A", Namespace: "default"}}
-	dst.Pod.Ports = []v1.ContainerPort{{Name: PortHello.StrVal, ContainerPort: 22}}
+	dst.Pod.Ports = []v1.ContainerPort{{Name: PortHello.StrVal, ContainerPort: 22, Protocol: "UDP"}}
+	// default protocol for containerPort is TCP, if the Protocol is not defined will get a mismatch
 	var AllowNamedPortOnProtocol = netv1.NetworkPolicyPort{
 		Protocol: &UDP,
 		Port:     &PortHello,

pkg/netpol/eval/internal/k8s/pod.go

@@ -277,16 +277,20 @@ func (pod *Pod) PodExposedTCPConnections() *common.ConnectionSet {
 	return res
 }
 
-// ConvertPodNamedPort returns the ContainerPort number that matches the named port
-// if there is no match, returns -1
+// ConvertPodNamedPort returns the ContainerPort's protocol and number that matches the named port
+// if there is no match, returns empty string for protocol and -1 for number
 // namedPort is unique within the pod
-func (pod *Pod) ConvertPodNamedPort(namedPort string) int32 {
+func (pod *Pod) ConvertPodNamedPort(namedPort string) (protocol string, portNum int32) {
 	for _, containerPort := range pod.Ports {
-		if namedPort == containerPort.Name {
-			return containerPort.ContainerPort
+		if namedPort == containerPort.Name { // found
+			if containerPort.Protocol == "" {
+				// found the named port with unspecified protocol this means "TCP" protocol (default)
+				return string(corev1.ProtocolTCP), containerPort.ContainerPort
+			}
+			return string(containerPort.Protocol), containerPort.ContainerPort
 		}
 	}
-	return common.NoPort
+	return "", common.NoPort
 }
 
 // updatePodXgressExposureToEntireClusterData updates the pods' fields which are related to entire class exposure on ingress/egress
@@ -313,11 +317,13 @@ func (pod *Pod) checkAndConvertNamedPortsInConnection(conns *common.ConnectionSe
 		return nil
 	} // else - found named ports
 	connsCopy := conns.Copy() // copying the connectionSet; in order to replace
-	// the named ports with pod's port numbers
+	// the named ports with pod's port numbers if possible
 	for protocol, namedPorts := range connNamedPorts {
 		for _, namedPort := range namedPorts {
-			portNum := pod.ConvertPodNamedPort(namedPort)
-			connsCopy.ReplaceNamedPortWithMatchingPortNum(protocol, namedPort, portNum)
+			podProtocol, portNum := pod.ConvertPodNamedPort(namedPort)
+			if podProtocol == string(protocol) && portNum != common.NoPort { // matching port and protocol
+				connsCopy.ReplaceNamedPortWithMatchingPortNum(protocol, namedPort, portNum)
+			}
 		}
 	}
 	return connsCopy

pkg/netpol/eval/resources.go

@@ -579,14 +579,16 @@ func (pe *PolicyEngine) GetSelectedPeers(selectors labels.Selector, namespace st
 
 // ConvertPeerNamedPort returns the peer.pod.containerPort matching the named port of the peer
 // if there is no match for the input named port, return -1
-func (pe *PolicyEngine) ConvertPeerNamedPort(namedPort string, peer Peer) (int32, error) {
+func (pe *PolicyEngine) ConvertPeerNamedPort(namedPort string, peer Peer) (protocol string, portNum int32, err error) {
 	switch currentPeer := peer.(type) {
 	case *k8s.WorkloadPeer:
-		return currentPeer.Pod.ConvertPodNamedPort(namedPort), nil
+		protocol, portNum := currentPeer.Pod.ConvertPodNamedPort(namedPort)
+		return protocol, portNum, nil
 	case *k8s.PodPeer:
-		return currentPeer.Pod.ConvertPodNamedPort(namedPort), nil
+		protocol, portNum := currentPeer.Pod.ConvertPodNamedPort(namedPort)
+		return protocol, portNum, nil
 	default:
-		return 0, errors.New("peer type does not have ports") // should not get here
+		return "", 0, errors.New("peer type does not have ports") // should not get here
 	}
 }
 

test_outputs/connlist/netpol_named_port_test_2_connlist_output.txt

@@ -0,0 +1,3 @@
+0.0.0.0-255.255.255.255 => helloworld/pod-a[Deployment] : All Connections
+helloworld/pod-a[Deployment] => 0.0.0.0-255.255.255.255 : All Connections
+helloworld/pod-a[Deployment] => helloworld/new-pod[Deployment] : SCTP 8956,9090

test_outputs/connlist/netpol_named_port_test_connlist_output.txt

@@ -0,0 +1,2 @@
+0.0.0.0-255.255.255.255 => helloworld/new-pod[Deployment] : All Connections
+helloworld/new-pod[Deployment] => 0.0.0.0-255.255.255.255 : All Connections

tests/netpol_named_port_test/netpol.yaml

@@ -0,0 +1,18 @@
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: enable-ingress-from-named-port
+  namespace: helloworld
+spec:
+  podSelector:
+    matchLabels:
+      app: app-a
+  policyTypes:
+    - Ingress
+    - Egress
+  ingress:
+  - from:
+    - namespaceSelector: {}
+    ports:
+    - port: newport # this port with its protocol has no match in the pod configuration
+      protocol: UDP

tests/netpol_named_port_test/pods.yaml

@@ -0,0 +1,59 @@
+---
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: helloworld
+spec: {}
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: pod-a
+  namespace: helloworld
+  labels:
+    app: app-a
+spec:
+  selector:
+    matchLabels:
+      app: app-a
+  template:
+    metadata:
+      labels:
+        app: app-a
+    spec:
+      containers:
+      - name: helloworld
+        image: quay.io/shfa/app-a:latest
+        ports:
+        - name: newport
+          containerPort: 8956
+          protocol: SCTP
+        - name: udps
+          containerport: 3535
+          protocol: UDP
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: new-pod
+  namespace: helloworld
+  labels:
+    app: app-b
+spec:
+  selector:
+    matchLabels:
+      app: app-b
+  template:
+    metadata:
+      labels:
+        app: app-b
+    spec:
+      containers:
+      - name: helloworld
+        image: quay.io/shfa/ingress-world:latest
+        ports:
+        - containerPort: 8956  # containerport1
+          protocol: SCTP
+        - containerPort: 8050  # containerport2
+        - containerPort: 8090  # containerport3
+---

tests/netpol_named_port_test_2/netpol.yaml

@@ -0,0 +1,24 @@
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: enable-ingress-from-named-port
+  namespace: helloworld
+spec:
+  podSelector:
+    matchLabels:
+      app: app-b
+  policyTypes:
+    - Ingress
+    - Egress
+  ingress:
+  - from:
+    - namespaceSelector: {}
+    ports:
+    - port: newport # this port with its protocol has no match in the pod configuration
+      protocol: UDP
+    - port: sctp-port # its matching port number in the pod configuration is 8956
+      protocol: SCTP
+    - port: newport # its matching port number in the pod configuration is 9090
+      protocol: SCTP
+    - port: not-found # this port with its protocol has no match in the pod configuration
+      protocol: SCTP