From 922c9f5808f87a73ee0325b0e96b5eb8c3f232dc Mon Sep 17 00:00:00 2001
From: Murali Reddy <murali.reddy@cloudnativelabs.net>
Date: Sat, 29 Jul 2017 16:53:52 +0530
Subject: [PATCH] GA network policy does not reject if there is not a single
 source pod matching a policy

Fix ensures below two cases are explicitly handled

 - in the network policy spec for the ingress rule, its optionsl to give 'ports' and 'from' details
   when not specified it translates to match all ports, match all sources respectivley

 - user may explicitly give the 'ports' and 'from' details in the ingress rule. But at any given point
   its possible there is no matching pods (with labels defined in 'from') in the namespace.

Before the fix both the cases were handled similarly resulting in unexpected behaviour

Fixes #85
---
 app/controllers/network_policy_controller.go | 56 ++++++++++++++------
 1 file changed, 40 insertions(+), 16 deletions(-)

diff --git a/app/controllers/network_policy_controller.go b/app/controllers/network_policy_controller.go
index 44f5ee11..f6dd8654 100644
--- a/app/controllers/network_policy_controller.go
+++ b/app/controllers/network_policy_controller.go
@@ -68,8 +68,10 @@ type podInfo struct {
 }
 
 type ingressRule struct {
-	ports   []protocolAndPort
-	srcPods []podInfo
+	matchAllPorts  bool
+	ports          []protocolAndPort
+	matchAllSource bool
+	srcPods        []podInfo
 }
 
 type protocolAndPort struct {
@@ -304,7 +306,7 @@ func (npc *NetworkPolicyController) syncNetworkPolicyChains() (map[string]bool,
 
 			// case where only 'ports' details specified but no 'from' details in the ingress rule
 			// so match on all sources, with specified port and protocol
-			if len(ingressRule.srcPods) == 0 && len(ingressRule.ports) != 0 {
+			if ingressRule.matchAllSource && !ingressRule.matchAllPorts {
 				for _, portProtocol := range ingressRule.ports {
 					comment := "rule to ACCEPT traffic from source pods to dest pods selected by policy name: " +
 						policy.name + " namespace " + policy.namespace
@@ -322,7 +324,14 @@ func (npc *NetworkPolicyController) syncNetworkPolicyChains() (map[string]bool,
 
 			// case where nether ports nor from details are speified in the ingress rule
 			// so match on all ports, protocol, source IP's
-			if len(ingressRule.srcPods) == 0 && len(ingressRule.ports) == 0 {
+			if ingressRule.matchAllSource && ingressRule.matchAllPorts {
+
+				// if no ports or source information is present in spec this is specical case
+				// where network policy does not allow any traffic
+				if npc.v1NetworkPolicy {
+					continue
+				}
+
 				comment := "rule to ACCEPT traffic from source pods to dest pods selected by policy name: " +
 					policy.name + " namespace " + policy.namespace
 				args := []string{"-m", "comment", "--comment", comment,
@@ -655,24 +664,39 @@ func buildNetworkPoliciesInfo() (*[]networkPolicyInfo, error) {
 			ingressRule := ingressRule{}
 
 			ingressRule.ports = make([]protocolAndPort, 0)
-			for _, port := range specIngressRule.Ports {
-				protocolAndPort := protocolAndPort{protocol: string(*port.Protocol), port: port.Port.String()}
-				ingressRule.ports = append(ingressRule.ports, protocolAndPort)
+
+			// If this field is empty or missing in the spec, this rule matches all ports
+			if len(specIngressRule.Ports) == 0 {
+				ingressRule.matchAllPorts = true
+			} else {
+				ingressRule.matchAllPorts = false
+				for _, port := range specIngressRule.Ports {
+					protocolAndPort := protocolAndPort{protocol: string(*port.Protocol), port: port.Port.String()}
+					ingressRule.ports = append(ingressRule.ports, protocolAndPort)
+				}
 			}
 
 			ingressRule.srcPods = make([]podInfo, 0)
-			for _, peer := range specIngressRule.From {
-				matchingPods, err := watchers.PodWatcher.ListByNamespaceAndLabels(policy.Namespace, peer.PodSelector.MatchLabels)
-				if err == nil {
-					for _, matchingPod := range matchingPods {
-						ingressRule.srcPods = append(ingressRule.srcPods,
-							podInfo{ip: matchingPod.Status.PodIP,
-								name:      matchingPod.ObjectMeta.Name,
-								namespace: matchingPod.ObjectMeta.Namespace,
-								labels:    matchingPod.ObjectMeta.Labels})
+
+			// If this field is empty or missing in the spec, this rule matches all sources
+			if len(specIngressRule.From) == 0 {
+				ingressRule.matchAllSource = true
+			} else {
+				ingressRule.matchAllSource = false
+				for _, peer := range specIngressRule.From {
+					matchingPods, err := watchers.PodWatcher.ListByNamespaceAndLabels(policy.Namespace, peer.PodSelector.MatchLabels)
+					if err == nil {
+						for _, matchingPod := range matchingPods {
+							ingressRule.srcPods = append(ingressRule.srcPods,
+								podInfo{ip: matchingPod.Status.PodIP,
+									name:      matchingPod.ObjectMeta.Name,
+									namespace: matchingPod.ObjectMeta.Namespace,
+									labels:    matchingPod.ObjectMeta.Labels})
+						}
 					}
 				}
 			}
+
 			newPolicy.ingressRules = append(newPolicy.ingressRules, ingressRule)
 		}
 		NetworkPolicies = append(NetworkPolicies, newPolicy)