Add Azure-npm to provide k8s network policy support #173

saiyan86 · 2018-06-09T22:12:39Z

What this PR does / why we need it:

This PR adds Azure Network Policy Manager, a.k.a, azure-npm, to Azure CNI.
Azure-npm is a Microsoft Azure's fault-tolerant and scalable implementation of Kubernetes Network Policy plugin.
Azure-npm supports all network policies specified by current version of Kubernetes, and will continue to support future versions.

Azure-npm only supports Kubernetes v1.8 or later, as it requires NetworkPolicy API v1beta1.

jaer-tsun · 2018-06-29T17:50:31Z

npm/ipsm/ipsm.go

+		set:           util.GetHashedName(listName),
+	}
+
+	errCode, err := ipsMgr.Run(entry)


if err != nil {
if errCode == 1 {
log.Printf("Cannot delete list %s as it's being referred or doesn't exist.\n", listName)
return nil
}

log.Printf("Error deleting ipset %s\n %+v\n", listName, entry) return err

}

jaer-tsun · 2018-06-29T17:52:17Z

npm/ipsm/ipsm.go

+	}
+
+	if _, err := ipsMgr.Run(entry); err != nil {
+		log.Printf("Error creating ipset rules.\n")


combine logs

jaer-tsun · 2018-06-29T18:47:32Z

npm/ipsm/ipsm.go

+		return err
+	}
+
+	ipsMgr.setMap[setName] = NewIpset(setName)


space to stick with styling

jaer-tsun · 2018-06-29T18:49:27Z

npm/ipsm/ipsm.go

+	}
+
+	if err != nil {
+		log.Printf("Error deleting ipset %s", setName)


combine log

similar comment as above -

if err != nil {
if errCode == 1 {

jaer-tsun · 2018-06-29T18:50:28Z

npm/ipsm/ipsm.go

+		log.Printf("rule: %+v\n", entry)
+		return err
+	}
+	ipsMgr.setMap[setName].elements = append(ipsMgr.setMap[setName].elements, ip)


space after if brace

jaer-tsun · 2018-06-29T20:50:16Z

npm/namespace.go

+
+	allNs := npMgr.nsMap[util.KubeAllNamespacesFlag]
+	// Create ipset for the namespace.
+	ipsMgr := allNs.ipsMgr


ipsMgr := npMgr.nsMap[util.KubeAllNamespacesFlag].ipsMgr?

jaer-tsun · 2018-06-29T21:00:54Z

npm/nwpolicy.go

+	iptMgr := allNs.iptMgr
+	for _, iptEntry := range iptEntries {
+		if err := iptMgr.Delete(iptEntry); err != nil {
+			log.Printf("Error applying iptables rule\n")


combine logs

jaer-tsun · 2018-06-29T21:48:29Z

npm/parse.go

+		entries = append(entries, egressEntries...)
+
+		entries = append(entries, getDefaultDropEntries(affectedSets)...)
+


remove space

This is a different block. Need space here.

jaer-tsun · 2018-06-29T22:45:35Z

npm/pod.go

+		return nil
+	}
+
+	oldPodObjNs, oldPodObjName, oldPodObjPhase, oldPodObjIP, newPodObjNs, newPodObjName, newPodObjPhase, newPodObjIP := oldPodObj.ObjectMeta.Namespace, oldPodObj.ObjectMeta.Name, oldPodObj.Status.Phase, oldPodObj.Status.PodIP, newPodObj.ObjectMeta.Namespace, newPodObj.ObjectMeta.Name, newPodObj.Status.Phase, newPodObj.Status.PodIP


var (
oldPodObjNs = oldPodObj.ObjectMeta.Namespace
...
...
)

jaer-tsun · 2018-06-29T22:52:24Z

npm/pod.go

+		return nil
+	}
+
+	podNs, podName, podNodeName, podLabels := podObj.ObjectMeta.Namespace, podObj.ObjectMeta.Name, podObj.Spec.NodeName, podObj.ObjectMeta.Labels


var initialize all of these guys

ashvindeodhar · 2018-07-10T23:58:44Z

npm/ipsm/ipsm.go

+
+// NotReferredByNwPolicy checks if a specific ipset is referred by any network policy.
+func (ipsMgr *IpsetManager) NotReferredByNwPolicy(setName string) bool {
+	return ipsMgr.setMap[setName].referCount == 0


what if setName is not present in the setMap?

unused func. It is removed.

ashvindeodhar · 2018-07-11T00:22:23Z

npm/ipsm/ipsm.go

+	errCode, err := ipsMgr.Run(entry)
+	if errCode > 1 && err != nil {
+		log.Printf("Error deleting ipset entry.\n")
+		log.Printf("%+v\n", entry)


combine the logs

ashvindeodhar · 2018-07-11T00:26:40Z

npm/ipsm/ipsm.go

+	}
+
+	if err != nil {
+		log.Printf("Error deleting ipset %s", setName)


similar comment as above -

if err != nil {
if errCode == 1 {

ashvindeodhar · 2018-07-11T00:36:48Z

npm/ipsm/ipsm.go

+
+	if msg, failed := err.(*exec.ExitError); failed {
+		errCode = msg.Sys().(syscall.WaitStatus).ExitStatus()
+		if errCode > 1 {


combine these as
if errCode := msg.Sys().(syscall.WaitStatus).ExitStatus(); errCode > 1 {
?

ashvindeodhar · 2018-07-11T00:42:04Z

npm/iptm/iptm.go

+
+// InitNpmChains initializes Azure NPM chains in iptables.
+func (iptMgr *IptablesManager) InitNpmChains() error {
+	log.Printf("Initializing AZURE-NPM")


Is initializing azure-npm same as azure-npm-chain?

Changed it to "Initializing AZURE-NPM chains"

ashvindeodhar · 2018-07-11T00:47:39Z

npm/iptm/iptm.go

+			util.IptablesJumpFlag,
+			util.IptablesAzureChain,
+		},
+	}


space here?

This is one code block. entry is used right after declaration

ashvindeodhar · 2018-07-11T00:48:23Z

npm/iptm/iptm.go

+		return nil
+	}
+
+	if err != nil {


Similar comment as above about restructure

ashvindeodhar · 2018-07-11T00:49:16Z

npm/iptm/iptm.go

+
+// Add adds a rule in iptables.
+func (iptMgr *IptablesManager) Add(entry *IptEntry) error {
+	log.Printf("%+v\n", entry)


Is this log helpful?

added description.

ashvindeodhar · 2018-07-11T00:49:25Z

npm/iptm/iptm.go

+
+// Delete removes a rule in iptables.
+func (iptMgr *IptablesManager) Delete(entry *IptEntry) error {
+	log.Printf("%+v\n", entry)


Is this log helpful?

added description.

ashvindeodhar · 2018-07-11T00:50:58Z

npm/iptm/iptm.go

+
+	if msg, failed := err.(*exec.ExitError); failed {
+		errCode = msg.Sys().(syscall.WaitStatus).ExitStatus()
+		if errCode > 1 {


combine these as
if errCode := msg.Sys().(syscall.WaitStatus).ExitStatus(); errCode > 1 {
?

ashvindeodhar · 2018-07-11T01:06:15Z

npm/namespace.go

+	name   string
+	setMap map[string]string
+	podMap map[types.UID]*corev1.Pod
+	npMap  map[string]*networkingv1.NetworkPolicy // TODO: Optimize to ordered map.


saiyan86

address comments

ashvindeodhar · 2018-07-18T23:31:59Z

npm/nwpolicy.go

+
+	allNs := npMgr.nsMap[util.KubeAllNamespacesFlag]
+
+	_, _, iptEntries := parsePolicy(npObj)


Can parsePolicy return an error? What happens in case of error?

This is basically pure string manipulation. This function has no effect by itself. Its return value is used in other functions, and I catch errors there.

ashvindeodhar · 2018-07-18T23:37:22Z

npm/nwpolicy_test.go

+
+	allNs, err := newNs(util.KubeAllNamespacesFlag)
+	if err != nil {
+		panic(err.Error)


Where are you handling these panics? I don't see any recover.

This error is fatal. It shouldn't be recovered.

ashvindeodhar · 2018-07-18T23:48:28Z

npm/parse.go

+		isAppliedToNs = true
+	}
+
+	//TODO: handle IPBlock


Is this still a TODO?

Nope. Removed. Thanks!

ashvindeodhar · 2018-07-18T23:52:12Z

npm/plugin/main.go

+	log.SetName("azure-npm")
+	log.SetLevel(log.LevelInfo)
+	err := log.SetTarget(log.TargetLogfile)
+	if err != nil {


if err := log.SetTarget(log.TargetLogfile); err != nil {

ashvindeodhar · 2018-07-18T23:53:16Z

npm/plugin/main.go

+	// Creates the in-cluster config
+	config, err := rest.InClusterConfig()
+	if err != nil {
+		panic(err.Error())


Similar comment to above - where are you handing these panics?

Addressed. Thanks!

ashvindeodhar · 2018-07-19T00:13:02Z

npm/pod.go

+	podNodeName := podObj.Spec.NodeName
+	podLabels := podObj.ObjectMeta.Labels
+	podIP := podObj.Status.PodIP
+	log.Printf("POD DELETED: %s/%s/%s\n", podNs, podName, podNodeName)


Move this to line 138 - which makes sure we log 'deleted' only when the operation is successful

changed from deleted to deleting

ashvindeodhar · 2018-07-19T00:14:56Z

npm/pod_test.go

+	"testing"
+
+	"github.com/Azure/azure-container-networking/npm/ipsm"
+


any reason for space here?

ashvindeodhar · 2018-07-19T00:15:37Z

npm/pod_test.go

@@ -0,0 +1,173 @@
+package npm


You added a bunch of new files - I think we need to add correct copyright info in those.

Where should we add to copyright info?

check the top of https://github.com/Azure/azure-container-networking/blob/master/cni/network/network.go .
Please check with Sushant if these are needed. I think we need to have Microsoft under copyright statement for new files.

Thanks. I added Microsoft copyright info.

ashvindeodhar · 2018-07-19T00:20:46Z

npm/pod_test.go

+	}
+}
+
+func TestUpdatePod(t *testing.T) {


You can combine TestUpdatePod and TestAddPod (like TestAddUpdatePod) because 95% of the code is same. Is there any particular reason to have these separate? It's possible to combine delete in this as well. But I don't have any strong push for this

This is incremental test. This will give a bit more coverage.

ashvindeodhar · 2018-07-19T00:36:56Z

npm/util/util.go

@@ -0,0 +1,42 @@
+package util


Can this util package be brought out of npm and at azure-container-networking scope? That way we can build this util and can use it everywhere within azure-container-networking. If easily possible, otherwise no worries.

Nice suggestion. I'll do it later. But can we check this in first? This is pretty important :D

* azure-npm * set logging file * parameterize telemetry API * avoid null ptr derefence * add telemetry to npm

saiyan86 · 2018-07-19T21:23:26Z

@ashvindeodhar @jaer-tsun Thanks!

saiyan86 requested review from sharmasushant and tamilmani1989 June 9, 2018 22:12

saiyan86 force-pushed the master branch 3 times, most recently from 59bf95f to c612106 Compare June 26, 2018 23:17

jaer-tsun reviewed Jun 29, 2018

View reviewed changes

saiyan86 force-pushed the master branch from a816d37 to 7288b4e Compare July 3, 2018 22:07

saiyan86 requested a review from ashvindeodhar July 10, 2018 22:30

ashvindeodhar reviewed Jul 10, 2018

View reviewed changes

ashvindeodhar reviewed Jul 17, 2018

View reviewed changes

saiyan86 commented Jul 17, 2018

View reviewed changes

saiyan86 force-pushed the master branch from 3dce3a2 to 100747c Compare July 17, 2018 18:43

address comments

44e5398

saiyan86 force-pushed the master branch from 100747c to 44e5398 Compare July 17, 2018 18:48

make azure-npm versiona consistent with main version

75f54df

ashvindeodhar requested changes Jul 19, 2018

View reviewed changes

saiyan86 force-pushed the master branch from 9517650 to 75f54df Compare July 19, 2018 03:14

Yongli Chen added 5 commits July 18, 2018 20:15

Npm telemetry (#12)

db3af4f

* azure-npm * set logging file * parameterize telemetry API * avoid null ptr derefence * add telemetry to npm

address comments

a543b3d

add descriptive comments

3427459

add copyright info

32a627e

returns on err

4b60b7e

ashvindeodhar approved these changes Jul 19, 2018

View reviewed changes

saiyan86 merged commit 6a0f9ff into Azure:master Jul 19, 2018

		entries = append(entries, egressEntries...)

		entries = append(entries, getDefaultDropEntries(affectedSets)...)


		allNs := npMgr.nsMap[util.KubeAllNamespacesFlag]

		_, _, iptEntries := parsePolicy(npObj)

		"testing"

		"github.com/Azure/azure-container-networking/npm/ipsm"

Add Azure-npm to provide k8s network policy support #173

Add Azure-npm to provide k8s network policy support #173

Uh oh!

Conversation

saiyan86 commented Jun 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saiyan86 left a comment

Choose a reason for hiding this comment