Add Azure-npm to provide native k8s network policy support #3205

saiyan86 · 2018-06-06T23:17:56Z

What this PR does / why we need it:

This PR provides native k8s policy support.
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.

saiyan86 · 2018-06-08T17:06:54Z

/assign @jackfrancis

CecileRobertMichon · 2018-06-12T18:02:48Z

pkg/acsengine/const.go

@@ -50,6 +50,8 @@ const (
 	NetworkPolicyCalico = "calico"
 	// NetworkPolicyCilium is the string expression for cilium network policy config option
 	NetworkPolicyCilium = "cilium"
+	// NetworkPolicyAzure is the string expression for cilium network policy config option


copy paste typo in the comment, should say Azure CNI, not cilium

CecileRobertMichon · 2018-06-12T18:04:03Z

pkg/acsengine/const.go

@@ -50,6 +50,8 @@ const (
 	NetworkPolicyCalico = "calico"
 	// NetworkPolicyCilium is the string expression for cilium network policy config option
 	NetworkPolicyCilium = "cilium"
+	// NetworkPolicyAzure is the string expression for cilium network policy config option
+	NetworkPolicyAzure = "azure"
 	// NetworkPluginAzure is the string expression for Azure CNI network policy


This should probably be network plugin instead of network policy since you are adding one for network policy. Or maybe we could reuse the same constant for both? Your call.

@jackfrancis separated NetworkPolicy & NetworkPlugin. So basically NetworkPlugin means CNI and NetworkPolicy means network policy plugin. These are two different things.

Yes I meant the comment should reflect that, right it's // NetworkPluginAzure is the string expression for Azure CNI network policy which is confusing

CecileRobertMichon · 2018-06-12T18:07:18Z

pkg/acsengine/defaults.go

@@ -384,7 +384,7 @@ func setOrchestratorDefaults(cs *api.ContainerService) {
 		switch o.KubernetesConfig.NetworkPolicy {
 		case NetworkPluginAzure:
 			o.KubernetesConfig.NetworkPlugin = NetworkPluginAzure
-			o.KubernetesConfig.NetworkPolicy = DefaultNetworkPolicy
+			o.KubernetesConfig.NetworkPolicy = NetworkPolicyAzure


What is the reason for changing the default here? This might break backwards compatibility so we'll have to do some extensive testing if this is something we want. cc: @jackfrancis

Previously we don't have network policy support in Azure CNI, now we just released it. Therefore we want to set default NetworkPolicy to azure.

CecileRobertMichon · 2018-06-12T18:09:47Z

pkg/api/convertertoapi.go

-			api.KubernetesConfig.NetworkPlugin = vp.OrchestratorProfile.KubernetesConfig.NetworkPolicy
-			api.KubernetesConfig.NetworkPolicy = "" // no-op but included for emphasis
-		} else if vp.OrchestratorProfile.KubernetesConfig.NetworkPolicy == NetworkPolicyNone {
+		if vp.OrchestratorProfile.KubernetesConfig.NetworkPolicy == NetworkPolicyNone {


Could you explain why this is needed?

For setting default NetworkPolicy to azure

I don't understand how this is setting the default NetworkPolicy to azure? What I'm reading is "if NetworkPolicy is none. set NetworkPolicy to "" and NetworkPlugin to kubenet". What am I missing?

Previously it overwrites NetworkPolicy to "" if NetworkPolicy == NetworkPolicyAzure. It's not needed anymore.

These changes need to be removed from this PR. The entire reason for this code (as described in the comments) is to maintain backwards-compatibility with prior usage patterns. This code is not meant to accommodate ongoing usage pattern changes/additions.

pkg/acsengine/defaults_test.go

@@ -430,10 +430,6 @@ func TestNetworkPolicyDefaults(t *testing.T) {
 		t.Fatalf("NetworkPlugin did not have the expected value, got %s, expected %s",
 			properties.OrchestratorProfile.KubernetesConfig.NetworkPlugin, "azure")
 	}
-	if properties.OrchestratorProfile.KubernetesConfig.NetworkPolicy != "" {


CecileRobertMichon · 2018-06-12T18:22:59Z

Azure-npm only supports Kubernetes v1.8 or later, as it requires NetworkPolicy API v1beta1. does this mean we should add some validation on the k8s version? Right now it looks like it is being enabled by default, even if I specify k8s 1.7.x

saiyan86 · 2018-06-12T18:34:30Z

@CecileRobertMichon You're right. We could overwrite the NetworkPolicy to DefaultNetworkPolicy for k8s < 1.8.

CecileRobertMichon · 2018-06-12T21:42:00Z

pkg/api/convertertoapi.go

 			api.KubernetesConfig.NetworkPlugin = NetworkPluginKubenet
 			api.KubernetesConfig.NetworkPolicy = "" // no-op but included for emphasis
+		} else if vp.OrchestratorProfile.KubernetesConfig.NetworkPolicy == NetworkPolicyAzure {


I think this should happen in defaults.go since it is modifying the default behavior

codecov · 2018-06-12T22:02:35Z

Codecov Report

Merging #3205 into master will increase coverage by 0.04%.
The diff coverage is 84%.

@@            Coverage Diff             @@
##           master    #3205      +/-   ##
==========================================
+ Coverage   54.55%   54.59%   +0.04%     
==========================================
  Files         104      104              
  Lines       15739    15759      +20     
==========================================
+ Hits         8586     8604      +18     
- Misses       6404     6405       +1     
- Partials      749      750       +1

CecileRobertMichon · 2018-06-12T22:12:58Z

How does this PR relate to #3198 ?

saiyan86 · 2018-06-12T22:37:32Z

@CecileRobertMichon PR #3198 is Azure Container Network Monitering Service, which is used for container networking telemetry purposes. This PR adds native k8s network policy support.

jackfrancis · 2018-06-19T17:33:04Z

pkg/acsengine/defaults.go

@@ -394,6 +394,12 @@ func setOrchestratorDefaults(cs *api.ContainerService) {
 			o.KubernetesConfig.NetworkPlugin = NetworkPolicyCilium
 		}

+		if o.KubernetesConfig.NetworkPolicy == NetworkPolicyAzure {
+			if !common.IsKubernetesVersionGe(k8sVersion, "1.8.0") {


Let's move this < 1.8.0 logic up into the above switch/case statement, as it is executing against an identical condition (NetworkPolicy == "azure")

CecileRobertMichon · 2018-06-25T20:43:46Z

What is the expected default behavior for network policy after this PR if a user does not pass in any netork plugin/policy value? ie. with the default kubernetes example apimodel

We need to update the relevant docs (especially since the default behavior isn't straightforward by looking at the code because of all the back compat cruft), specifically https://github.com/Azure/acs-engine/blob/master/examples/networkpolicy/README.md and clusterdefinition.md

jackfrancis · 2018-06-25T20:46:34Z

This change should have no impact on the current expected default configuration outcome. From a high level, this PR:

introduces a new allowd networkPlugin/networkPolicy combination of "azure"/"azure"
The above, new, configuration installs the Azure NetworkPolicy daemonset via the addons vector onto their cluster

That should be it. I believe our unit tests should ensure no regressions in our default configuration cases.

CecileRobertMichon

/lgtm

jackfrancis · 2018-06-28T21:33:42Z

/lgtm

acs-bot · 2018-06-28T21:33:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon, jackfrancis, saiyan86

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [CecileRobertMichon,jackfrancis]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

marrobi · 2018-07-04T14:04:28Z

@saiyan86 Great to see this progressing. Where do we report issues with azure-npm? Is there a repo available? Certain policies do not seem to function correctly, for example try https://github.com/ahmetb/kubernetes-network-policy-recipes/blob/master/04-deny-traffic-from-other-namespaces.md . This does not seem to work with azure-npm, but does with kube-router. Policies using pod selectors do seem to work.

@jackfrancis @CecileRobertMichon are you aware of a project that tests various network policies?

Thanks.

sharmasushant · 2018-07-05T00:10:12Z

@marrobi Thank you for trying it out. Please file the issue @ https://github.com/Azure/azure-container-networking/issues

marrobi · 2018-07-05T09:04:43Z

@sharmasushant see Azure/azure-container-networking#185

jackfrancis · 2018-07-05T16:47:49Z

@marrobi I'm not! We have a very basic NetworkPolicy validation in our E2E tests, would love to introduce something more thorough if you discover a comprehensive lib/project.

saiyan86 · 2018-07-05T23:19:25Z

@jackfrancis I will add some tests soon that should work for both Calico and us.

acs-bot added the size/L label Jun 6, 2018

acs-bot assigned jackfrancis Jun 8, 2018

CecileRobertMichon reviewed Jun 12, 2018

View reviewed changes

saiyan86 force-pushed the networkPolicyManager branch from fd63aa4 to 60ebf34 Compare June 12, 2018 21:15

CecileRobertMichon reviewed Jun 12, 2018

View reviewed changes

saiyan86 changed the title ~~Add Azure-npm to provide k8s network policy support~~ Add Azure-npm to provide native k8s network policy support Jun 12, 2018

CecileRobertMichon added the awaiting review label Jun 13, 2018

jackfrancis reviewed Jun 19, 2018

View reviewed changes

acs-bot assigned CecileRobertMichon Jun 25, 2018

CecileRobertMichon approved these changes Jun 25, 2018

View reviewed changes

acs-bot added lgtm approved labels Jun 25, 2018

Yongli Chen added 6 commits June 25, 2018 15:34

add azure-npm as addon

608f64d

use NetworkPolicyAzure insteand of NetworkPluginAzure for azure-npm

2ece359

remove taint

91f621d

Make azure-npm default network policy plugin

1ba8b41

add ensureExists labels to serviceAccount/clusterRole/clusterRoleBinding

f3c5f73

add backward compatiblity & test coverage for NetworkPolicyAzure

f9bc173

Yongli Chen and others added 10 commits June 25, 2018 15:34

enforce k8s version in defaults.go

7f10f40

enforce k8s version validation

5b3d76e

merge if statement to switch

3321976

revert changes

7995ff0

avoid overwrite when NetworkPlugin & Policy are set to "azure"

4baea91

networkPlugin + networkPolicy == azure

427cb1a

wire up addon

03ad011

cruft

85a1845

defaults change to allow "azure"/"azure"

70cf614

add doc note

959b5ba

jackfrancis force-pushed the networkPolicyManager branch from 8ed0bcc to 959b5ba Compare June 25, 2018 22:36

acs-bot removed the lgtm label Jun 25, 2018

Yongli Chen and others added 2 commits June 26, 2018 10:26

updated azure-npm image version to v0.0.2

79698ce

Run NetworkPolicy test for azure npm

965bb6e

acs-bot added the lgtm label Jun 28, 2018

jackfrancis merged commit 8f401a1 into Azure:master Jun 28, 2018

ghost removed the awaiting review label Jun 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Azure-npm to provide native k8s network policy support #3205

Add Azure-npm to provide native k8s network policy support #3205

saiyan86 commented Jun 6, 2018 •

edited

Loading

saiyan86 commented Jun 8, 2018

CecileRobertMichon Jun 12, 2018

CecileRobertMichon Jun 12, 2018

saiyan86 Jun 12, 2018

CecileRobertMichon Jun 12, 2018

CecileRobertMichon Jun 12, 2018

saiyan86 Jun 12, 2018

CecileRobertMichon Jun 12, 2018

saiyan86 Jun 12, 2018

CecileRobertMichon Jun 12, 2018

saiyan86 Jun 12, 2018

jackfrancis Jun 19, 2018

This comment was marked as resolved.

CecileRobertMichon commented Jun 12, 2018

saiyan86 commented Jun 12, 2018

CecileRobertMichon Jun 12, 2018

codecov bot commented Jun 12, 2018 •

edited

Loading

CecileRobertMichon commented Jun 12, 2018

saiyan86 commented Jun 12, 2018

jackfrancis Jun 19, 2018

CecileRobertMichon commented Jun 25, 2018 •

edited

Loading

jackfrancis commented Jun 25, 2018

CecileRobertMichon left a comment

jackfrancis commented Jun 28, 2018

acs-bot commented Jun 28, 2018

marrobi commented Jul 4, 2018

sharmasushant commented Jul 5, 2018

marrobi commented Jul 5, 2018

jackfrancis commented Jul 5, 2018

saiyan86 commented Jul 5, 2018

Add Azure-npm to provide native k8s network policy support #3205

Add Azure-npm to provide native k8s network policy support #3205

Conversation

saiyan86 commented Jun 6, 2018 • edited Loading

saiyan86 commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

CecileRobertMichon commented Jun 12, 2018

saiyan86 commented Jun 12, 2018

Choose a reason for hiding this comment

codecov bot commented Jun 12, 2018 • edited Loading

Codecov Report

CecileRobertMichon commented Jun 12, 2018

saiyan86 commented Jun 12, 2018

Choose a reason for hiding this comment

CecileRobertMichon commented Jun 25, 2018 • edited Loading

jackfrancis commented Jun 25, 2018

CecileRobertMichon left a comment

Choose a reason for hiding this comment

jackfrancis commented Jun 28, 2018

acs-bot commented Jun 28, 2018

marrobi commented Jul 4, 2018

sharmasushant commented Jul 5, 2018

marrobi commented Jul 5, 2018

jackfrancis commented Jul 5, 2018

saiyan86 commented Jul 5, 2018

saiyan86 commented Jun 6, 2018 •

edited

Loading

codecov bot commented Jun 12, 2018 •

edited

Loading

CecileRobertMichon commented Jun 25, 2018 •

edited

Loading