Issues with network policies after cluster reboot

[zachomedia]

Is this a request for help?: Yes

---

Is this an ISSUE or FEATURE REQUEST? (choose one): ISSUE

---

Which release version?: v1.0.18

---

Which component (CNI/IPAM/CNM/CNS): NPM

---

Which Operating System (Linux/Windows): Linux

---

For Linux: Include Distro and kernel version using "uname -a"

aks-engine, Ubuntu 16.04 image

Linux k8s-master-64980839-0 4.15.0-1040-azure #44-Ubuntu SMP Thu Feb 21 14:24:01 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux

---

Which Orchestrator and version (e.g. Kubernetes, Docker)

Kubernetes v1.13.2

---

What happened:

After rebooting the cluster, I was no longer able to communicate between applications in a namespace. I have a "default-deny" policy in that namespace, which was working up until a reboot.

---

What you expected to happen:

After a cluster reboot, pods in the namespace could communicate as allowed via network policies.

---

How to reproduce it (as minimally and precisely as possible):

1. Add npm to a running Azure Kubernetes cluster (we're using aks-engine)
2. Apply network policies, starting with a deny-all (Ingress and Egress) followed by explicit allows
3. Test network traffic, and see that whitelisted traffic works as expected
4. Reboot the cluster
5. Test network traffic, observer that most/all traffic does not work as expected

---

Anything else we need to know:

In #258, a LIFO ordering was applied to rules in the cluster. This works fine when policies are applied after NPM has started.

However, after a reboot, NPM re-fills its Informer with current cluster network policies. The current policies are not sent in the same order that they were created in, but instead are sent in an undefined order. I believe this then results in the iptables entries being in the wrong order, causing the deny-all policy to apply before the allow policies.

---

[zachomedia]

I ended up testing with two different fixes:

1. Add an ability to specify the "base" rule so that it gets added to the bottom of iptables
2. Fix two cases where it had the wrong flag (dst instead of src) (Fix egress port iptables rule when no ports are specified #269 + another place)

Here's my diff that appears to work (I'm not creating a PR at this point because I don't know how you want to proceed):

diff --git a/npm/iptm/iptm.go b/npm/iptm/iptm.go
index 8344f39..5fded32 100644
--- a/npm/iptm/iptm.go
+++ b/npm/iptm/iptm.go
@@ -295,6 +295,28 @@ func (iptMgr *IptablesManager) Add(entry *IptEntry) error {
 	return nil
 }
 
+// Append adds a rule in iptables, at the end of the ruleset.
+func (iptMgr *IptablesManager) Append(entry *IptEntry) error {
+	log.Printf("Add (append) iptables entry: %+v\n", entry)
+
+	exists, err := iptMgr.Exists(entry)
+	if err != nil {
+		return err
+	}
+
+	if exists {
+		return nil
+	}
+
+	iptMgr.OperationFlag = util.IptablesAppendFlag
+	if _, err := iptMgr.Run(entry); err != nil {
+		log.Printf("Error creating iptables rules.\n")
+		return err
+	}
+
+	return nil
+}
+
 // Delete removes a rule in iptables.
 func (iptMgr *IptablesManager) Delete(entry *IptEntry) error {
 	log.Printf("Deleting iptables entry: %+v\n", entry)
diff --git a/npm/nwpolicy.go b/npm/nwpolicy.go
index 2568bb6..7af1760 100644
--- a/npm/nwpolicy.go
+++ b/npm/nwpolicy.go
@@ -3,6 +3,8 @@
 package npm
 
 import (
+	"strconv"
+
 	"github.com/Azure/azure-container-networking/log"
 	"github.com/Azure/azure-container-networking/npm/util"
 	networkingv1 "k8s.io/api/networking/v1"
@@ -62,9 +64,26 @@ func (npMgr *NetworkPolicyManager) AddNetworkPolicy(npObj *networkingv1.NetworkP
 		return err
 	}
 
+	isBaseStr, ok := npObj.ObjectMeta.Annotations["network.zacharyseguin.ca/is-base"]
+	isBase := false
+
+	if ok {
+		isBase, err = strconv.ParseBool(isBaseStr)
+		if err != nil {
+			log.Printf("Invalid value for network.zacharyseguin.ca/is-base. Expected boolean. Object: %s/%s", npObj.ObjectMeta.Namespace, npObj.ObjectMeta.Name)
+			isBase = false
+		}
+	}
+
 	iptMgr := allNs.iptMgr
 	for _, iptEntry := range iptEntries {
-		if err = iptMgr.Add(iptEntry); err != nil {
+		if isBase {
+			err = iptMgr.Append(iptEntry)
+		} else {
+			err = iptMgr.Add(iptEntry)
+		}
+
+		if err != nil {
 			log.Printf("Error applying iptables rule\n. Rule: %+v", iptEntry)
 			return err
 		}
diff --git a/npm/parse.go b/npm/parse.go
index 83ecdb7..7cce5e8 100644
--- a/npm/parse.go
+++ b/npm/parse.go
@@ -461,7 +461,7 @@ func parseEgress(ns string, targetSets []string, rules []networkingv1.NetworkPol
 				util.IptablesSetFlag,
 				util.IptablesMatchSetFlag,
 				hashedTargetSetName,
-				util.IptablesDstFlag,
+				util.IptablesSrcFlag,
 				util.IptablesJumpFlag,
 				util.IptablesDrop,
 			},
@@ -573,7 +573,7 @@ func parseEgress(ns string, targetSets []string, rules []networkingv1.NetworkPol
 						util.IptablesSetFlag,
 						util.IptablesMatchSetFlag,
 						hashedTargetSetName,
-						util.IptablesDstFlag,
+						util.IptablesSrcFlag,
 						util.IptablesJumpFlag,
 						util.IptablesAzureEgressToNsChain,
 					},

[saiyan86]

@zachomedia Thanks a lot for the contribution! Could you please explain what counts as a "base" rule?

[zachomedia]

@saiyan86 I think it probably needs a better name, but I was using it to define the "default-deny" rule so that it would always be at the bottom of the iptables rules (ie. added by append instead of insert).

I think there is still an ordering issue even with the fix since the last time I rebooted my cluster there was one rule that didn't work until I deleted it and added it back up.

[saiyan86]

@zachomedia Actually I think this is a valid issue for kubernetes. If possible, I'd like to propose an upstream fix so that all clients listening for network policies can get them in a correct order. On the other hand, I also think users are using network policies in an undefined way. Because networkpolicies doesn't support "priorities", expecting networkpolicies to behave on order of creation will result in undefined behavior.

[mgrabarz]

expecting networkpolicies to behave on order of creation will result in undefined behavior.

My assumption was this is the only ordering we can expect from policies in contrast to e.g. Calico where you have dedicated property in CRD.
What is then proper approach to ordering and how can I ensure order will be applied properly after reboot or upgrade?

[saiyan86]

expecting networkpolicies to behave on order of creation will result in undefined behavior.

My assumption was this is the only ordering we can expect from policies in contrast to e.g. Calico where you have dedicated property in CRD.
What is then proper approach to ordering and how can I ensure order will be applied properly after reboot or upgrade?

I'm thinking about a recovery mechanism for such events. Will update on this soon.

[saiyan86]

@zachomedia @mgrabarz #320 should fix this. I have pre-released the image containing the fix under label containernetworking/azure-npm:v1.0.19-rc1. Could you please give it a try and leave some feedback? Thanks! If there's anything wrong, we can reopen this.

[zachomedia]

Hello @saiyan86,

I tried the 1.0.19-rc1 version in a test cluster, and I'm still seeing some issues with the order post-reboot. I've attached the azure-npm.log file, and the iptables.conf file from before reboot (when everything worked) and then iptables.conf from post reboot (where issues were encountered). I'll see if I have some time in the next few days to try and troubleshoot it some more. Is there something specific that might be useful?

In addition, because the rules to the filesystem of the host, wouldn't this still be an issue if you were to add a new node to the cluster?

[saiyan86]

@zachomedia Thanks for trying it out. This is a first step mitigation. I will handle the horizontal scaling(new node) scenario in later releases. However, that fix will force users to apply policies without considering ordering. Current implementation supports ordering, but the future fix will consider policies as additive.