K8s Network policy Support by g1rana · Pull Request #1090 · contiv/netplugin

g1rana · 2017-11-30T01:07:54Z

Description of the changes

This is feature commit to support K8s Network Policy at contiv. Using feature, Contiv will support K8s Ingress Network Policy however egress policy support comes in future code commit

Type of fix: New feature

Fixes #1089

Please describe:

changes made in the Pull request
type of testing done (both manual and automated)
Manual Testing :
= Create k8s Network Policy without configuring Pods
cat network-policy.yaml
kind: NetworkPolicy
apiVersion: networking.k8s.io/v1
metadata:
name: access-nginx
spec:
podSelector:
matchLabels:
app: nginx
ingress:
- from:
  - podSelector:
    matchLabels:
    app: myapp

kubectl get netpol

NAME POD-SELECTOR AGE
access-nginx app=nginx 6m

Contiv system after k8s policy :
netctl group ls
Tenant Group Network IP Pool CfgdTag Policies Network profile

default default default-net
default default-group default-net ingress-policy,access-nginx
3. Bringup Ingress policy Pod and Src Pods
: kubectl create -f nginx-deployment.yaml
: kubectl create -f apod.yaml

kubectl get pods -o wide
NAME READY STATUS RESTARTS AGE IP NODE
apod 1/1 Running 0 2m 10.233.64.8 k8s1
nginx-deployment-431080787-6b6zh 1/1 Running 0 2m 10.233.64.7 k8s1
nginx-deployment-431080787-9d949 1/1 Running 0 2m 10.233.64.6 k8s1

netctl policy rule-ls access-nginx
Incoming Rules:
Rule Priority From EndpointGroup From Network From IpAddress TO IpAddress Protocol Port Action

access-nginx-10.233.64.6-10.233.64.8 2 10.233.64.8 10.233.64.6 0 allow
access-nginx-10.233.64.7-10.233.64.8 2 10.233.64.8 10.233.64.7 0 allow
Outgoing Rules:
Rule Priority To EndpointGroup To Network To IpAddress Protocol Port Action

====

Tests = Only for Ingress K8s Network policy

Create Network Policy resource before configuring any policy pods configuration ;

Result : make sure K8s policy object created in K8s and contiv system

Create Network policy after configuring Policy Pods without policy Label .

Result : make sure only K8s policy object create but no policy configured in contiv system

Create Network Policy after configuring Policy Pods with policy Label .

Result : Make sure K8s Policy object and its policy get programmed in Contiv system verify it by "nlicyetctl policy rule-ls

Add more Pods In existing policy on Ingress policy Src side .

Result : More Policy rules should be added in contiv system

Add more Pods in existing policy on Policy . destination side.

Result : more policy rules should show up in contiv system

Repeat test-4 and 5 by with Pod deletio operation
delete network-policy
- Result: Policy object should be deleted from both K8s and contiv system
Update Policy Pod information in existing Policy .
-Result : Make sure update policy rules programmed in contiv system

Documentation

g1rana · 2017-11-30T01:24:07Z

@eng-contiv

tiewei

reviewed half of the changes. some suggestions

logging on success request as well
logging and avoid infinity loop
transactional operation should be handled on server side, this way will avoid wired race condition and other concurrency issue

tiewei · 2017-12-01T02:35:53Z

netmaster/k8snetwork/networkpolicy.go


 const defaultTenantName = "default"
-const defaultNetworkName = "net"
+const defaultNetworkName = "default-net"


do we need to provide upgrading guide for this change ?

Seems from earlier conversations, admin would need to create a "default-net" but would have to be a different subnet? Could that break some deployments? Requires network changes to pods and redeploy?

@tiewei: No , don't need to provide upgrade . infect, we can revert this change
@chrisplo: based one tiewei query, user won't be have option to configure "net" network
"

tiewei · 2017-12-01T23:45:33Z

netmaster/k8snetwork/networkpolicy.go

+	return defaultEpgName
+}
+func getTenantInfo() string {
+	return defaultTenantName


why these 4 functions are required ?

Initially thinking to use wrapper function to access default tenant and EPG name . look like currently not using it. will remove it

tiewei · 2017-12-01T23:48:22Z

netmaster/k8snetwork/networkpolicy.go

+			nwName)
 		return err
 	}() != nil {
+		//XXX:Should we really poll here ;


if an infinity loop is here ( without enough logs and hints), no one would able to know how to find out.
please put logs and a maxretry here.

idea: log first loop iteration
optionally, additionally we log every minute so we know there is an issue

agree, will put logs with timeout

tiewei · 2017-12-01T23:48:51Z

netmaster/k8snetwork/networkpolicy.go

+			nwName)
 		return err
-	}() == nil {
+	}() == nil { //XXX: Same here as above


tiewei · 2017-12-01T23:49:07Z

netmaster/k8snetwork/networkpolicy.go

-	if _, err := k8sNet.contivClient.NetworkGet(defaultTenantName, nwName); err != nil {
+	if _, err := k8sNet.contivClient.NetworkGet(
+		defaultTenantName,
+		nwName); err != nil {


log the network deleted correctly

ok. will put log.

tiewei · 2017-12-01T23:49:31Z

netmaster/k8snetwork/networkpolicy.go

-	if _, err := k8sNet.contivClient.NetworkGet(defaultTenantName, nwName); err == nil {
+	if _, err := k8sNet.contivClient.NetworkGet(
+		defaultTenantName,
+		nwName); err == nil {


get network info and log it

to follow up, creating a network that is already there could be construed as a problem, would be good to log so that we can track it down when we have time, I know this was just a reformat

ok , Will put the meaningful logs

tiewei · 2017-12-01T23:49:46Z

netmaster/k8snetwork/networkpolicy.go

+			epgName)
 		return err
-	}() != nil {
+	}() != nil { //XXX: Same as above


tiewei · 2017-12-01T23:50:50Z

netmaster/k8snetwork/networkpolicy.go

+		npLog.Errorf("failed to update EPG %v: err:%v ", epgName, err)
+		return err
+	}
+	k8sNet.epgName[epgName] = epgName


log default policy and default epg content

tiewei · 2017-12-01T23:52:37Z

netmaster/k8snetwork/networkpolicy.go

+	//policy := k8sutils.EpgNameToPolicy(epgName, policyName)
+	if _, err := k8sNet.contivClient.
+		PolicyGet(tenantName, policyName); err == nil {
+		npLog.Infof("Policy:%v found contiv", policyName)


i think instead get-check-create, the creating checking operations should be handled on server side, this way will avoid concurrency issue

chrisplo

haven't finished reviewing, however, the pod selector logic for pod selectors matchlabels should be AND and not OR:

matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels map is equivalent to an element of matchExpressions, whose key field is “key”, the operator is “In”, and the values array contains only “value”. The requirements are ANDed.

https://kubernetes.io/docs/api-reference/v1.8/#labelselector-v1-meta

chrisplo · 2017-12-07T17:46:50Z

netmaster/k8snetwork/networkpolicy.go

+}
+type labelPolicySpec struct {
+	policy []*k8sNetworkPolicy
+}


this struct is never referenced

chrisplo · 2017-12-07T18:44:52Z

netmaster/k8snetwork/networkpolicy.go

+	Port     int
+	Protocol string
+}
+type k8sNameSelector struct {


if this is namespace selector, can we name the struct k8sNamespaceSelector?

chrisplo · 2017-12-07T18:46:41Z

netmaster/k8snetwork/networkpolicy.go

+type k8sIngress struct {
+	IngressRules           []k8sPolicyPorts
+	IngressPodSelector     []*k8sPodSelector
+	IngressNameSelector    *k8sNameSelector


IngressNamespaceSelector?

chrisplo · 2017-12-07T18:52:42Z

netmaster/k8snetwork/networkpolicy.go

+type npPodInfo struct {
+	nameSpace      string
+	labelSelectors []string
+	IP             string //??? should care for ipv6 address


use net.IP ?

chrisplo · 2017-12-07T18:52:59Z

netmaster/k8snetwork/networkpolicy.go

+	podEpg       string
+}
+type k8sIPBlockSelector struct {
+	subNetIps []string


use net.IPNet?

how is the "except" clause going to be handled? see in the docs an example:

- ipBlock: cidr: 172.17.0.0/16 except: - 172.17.1.0/24

chrisplo · 2017-12-07T19:51:39Z

netmaster/k8snetwork/networkpolicy.go

+	//List of Rules Per Policy
+	//	policyRules map[string][]string
+	//List of Network configured
+	network map[string]bool


chrisplo · 2017-12-07T19:51:57Z

netmaster/k8snetwork/networkpolicy.go

+	//List of Network configured
+	network map[string]bool
+	//List of EPG configured as set
+	epgName map[string]bool


chrisplo · 2017-12-07T19:53:21Z

netmaster/k8snetwork/networkpolicy.go

+	policyPerEpg map[string]map[string][]string
+	//Cache table for given Pods
+	//Policy Obj per  Policy Name
+	nwPolicyPerNameSpace map[string][]*k8sNetworkPolicy


nwPoliciesPerNamespace (plural)

chrisplo · 2017-12-07T21:00:56Z

netmaster/k8snetwork/networkpolicy.go

-		return
+//Get  Network Policy object sets which ToSpec labelMap information match
+//with given pods labelMap
+func (k8sNet *k8sContext) getMatchToSpecPartNetPolicy(


This might be easier to unit test if you passed in the podInfo's namespace and labelSelectors values instead of the podInfo itself.

I had a hard time with this function name, could we rename with some hints for input, output, and more specific behavior?
e.g.: getPoliciesForPodByLabels

chrisplo · 2017-12-07T21:02:02Z

netmaster/k8snetwork/networkpolicy.go

+
+	if len(toPartPolicy) > 0 {
+		npLog.Infof("Recv Pod belongs to ToSpec part of Policy")
+		for _, nw := range toPartPolicy {


nw looks like a network, but this is a policy, please update the name?

chrisplo

getting closer to first pass complete! but having to rethink how I approach the review since it's so big, working through a top down code execution path review now so I can get some clarity on some implementation choices that have been made.

most of the feedback is minor naming / logging type stuff to help fresh eyes figure out what's going on, but there are a few real concerns sprinkled in

chrisplo · 2017-12-08T21:13:12Z

netmaster/k8snetwork/networkpolicy.go

+	return defaultTenantName
+}
+
+//Start Network Policy feature enabler


I know you didn't touch, but could you please log infon on first pass through he infinite sleep below?

chrisplo · 2017-12-08T21:22:34Z

netmaster/k8snetwork/networkpolicy.go

-	if _, err := k8sNet.contivClient.NetworkGet(defaultTenantName, nwName); err == nil {
+	if _, err := k8sNet.contivClient.NetworkGet(
+		defaultTenantName,
+		nwName); err == nil {


to follow up, creating a network that is already there could be construed as a problem, would be good to log so that we can track it down when we have time, I know this was just a reformat

chrisplo · 2017-12-08T21:52:17Z

netmaster/k8snetwork/networkpolicy.go

+			nwName)
 		return err
 	}() != nil {
+		//XXX:Should we really poll here ;


idea: log first loop iteration
optionally, additionally we log every minute so we know there is an issue

chrisplo · 2017-12-08T22:01:06Z

netmaster/k8snetwork/networkpolicy.go

+	if err = k8sNet.createDefaultPolicy(
+		defaultTenantName,
+		epgName); err != nil {
+		npLog.Errorf("failed  Default ingress policy EPG %v: err:%v ",


failed creating? maybe could be more clear, default ingress policy for which network/tenant?

chrisplo · 2017-12-08T22:02:10Z

netmaster/k8snetwork/networkpolicy.go

+		return err
+	}
+	if err = k8sNet.createEpg(nwName, epgName, policy); err != nil {
+		npLog.Errorf("failed to update EPG %v: err:%v ", epgName, err)


failed to create?

chrisplo · 2017-12-10T23:08:02Z

netmaster/k8snetwork/networkpolicy.go

+		k8sNet.parseIngressPolicy(np.Spec.Ingress,
+			np.Namespace)
+	if err != nil {
+		npLog.Warnf("ignore network policy: %s, %v", np.Name, err)


like above, maybe not "ignore"

chrisplo · 2017-12-11T00:57:18Z

netmaster/k8snetwork/networkpolicy.go

@@ -343,7 +750,7 @@ func (k8sNet *k8sContext) watchK8sEvents(errChan chan error) {
 	for k8sNet.isLeader() != true {
 		time.Sleep(time.Millisecond * 100)


marking a spot where we can use a logging retry function

chrisplo · 2017-12-11T01:14:45Z

netmaster/k8snetwork/networkpolicy.go

+	opCode watch.EventType, np *network_v1.NetworkPolicy) {
+	if np.Namespace == "kube-system" { //not applicable for system namespace
+		return
 	}


minor, but since both this and processK8sPod both do this check, you could do it in processK8sEvent instead?

chrisplo · 2017-12-11T01:30:30Z

netmaster/k8snetwork/networkpolicy.go

+
+func (k8sNet *k8sContext) addNetworkPolicy(np *network_v1.NetworkPolicy) {
+	//check if given Policy already exist
+	if _, ok := k8sNet.networkPolicy[np.Name]; ok {


I'm not sure about the top level network policy map as I thought network policy names can be reused across namespaces

chrisplo · 2017-12-11T01:33:30Z

netmaster/k8snetwork/networkpolicy.go

+}
+type k8sNetworkPolicy struct {
+	PodSelector *k8sPodSelector
+	Ingress     []k8sIngress


I think the policy name should be here instead of in the pod selector for instance

chrisplo · 2017-12-11T03:59:29Z

netmaster/k8snetwork/networkpolicy.go

+	PodSelector := k8sPodSelector{
+		TenantName:  getTenantInfo(),
+		NetworkName: getNetworkInfo(),
+		GroupName:   getEpgInfo()}


these three look like placeholders for needed functionality, I think Ranjith's other PR I'm looking at (well I made my own PR based on his) might fit in here for the group name.

Let's discuss/see if we can get to wider consensus that mapping tenant to namespace make sense (thought I saw some conversations that this could be a really good mapping)

chrisplo · 2017-12-11T06:53:39Z

netmaster/k8snetwork/networkpolicy.go

+			for _, l := range label {
+				if ipMap, ok :=
+					podSelector.labelPodMap[l]; ok {
+					ipMap[ip] = true


I think this might be a spot to adjust for the AND logic of the matchLabels

chrisplo · 2017-12-11T06:53:55Z

netmaster/k8snetwork/networkpolicy.go

+	nw *k8sNetworkPolicy, label []string, ip string) {
+	for _, ingress := range nw.Ingress {
+		for _, podSelector := range ingress.IngressPodSelector {
+			npLog.Infof("podSelector:%+v", podSelector)


leftover debug statement?

chrisplo · 2017-12-11T06:54:27Z

netmaster/k8snetwork/networkpolicy.go

+		//At each Label Walk all its Ips
+		for ip := range lMap {
+			nw.PodSelector.podIps[ip] = ip
+		}


another spot that might be implementing OR logic instead of AND logic for matchLabels

chrisplo · 2017-12-11T06:55:18Z

netmaster/k8snetwork/networkpolicy.go

-	if err = k8sNet.deleteRule(policyName, defaultRuleID); err != nil {
-		npLog.Errorf("failed to delete default rule, %s", err)
+//Consolidate all Ips belongs to Label for Pod Selector object
+func (k8sNet *k8sContext) updatePodSelectorPodIps(


this function doesn't appear to use k8sContext, be more appropriate to be part of the k8sPodSelector struct?

chrisplo · 2017-12-11T23:34:45Z

netmaster/k8snetwork/networkpolicy.go

+func (k8sNet *k8sContext) initPodSelectorCacheTbl(m map[string]string,
+	podSelector *k8sPodSelector) error {
+	if podSelector == nil {
+		return fmt.Errorf("Passe Nil Pod Selector reference")


"Passed", though this is an internal method and we create the podselector in the caller, why would we expect this to ever be nil?

chrisplo · 2017-12-12T18:24:44Z

netmaster/k8snetwork/networkpolicy.go

+		for key, val := range pod.ObjectMeta.Labels {
+			lkey := getLabelSelector(key, val)
+			npLog.Infof("Update label Selector key:%v", lkey)
+			if ipMap, ok := PodSelector.labelPodMap[lkey]; ok {


marking a spot that I think is aligning with OR based logic for label matching instead of AND

chrisplo · 2017-12-12T18:30:02Z

netmaster/k8snetwork/networkpolicy.go

+	// reset policy to deny on any error
+	policyResetOnErr := func(tenantName, groupName string) {
+		if err != nil {
+			//k8sNet.resetPolicy(tenantName, groupName)


missing implementation, though this could be touchy, not sure we should back out of the existing working policy . . . we might want to focus more on reconciliation of policy against contiv resources for the next attempt

chrisplo · 2017-12-12T18:33:28Z

netmaster/k8snetwork/networkpolicy.go

+	//Get PolicyMap using EPG
+	policyMap := k8sNet.policyPerEpg[np.PodSelector.GroupName]
+	//Check if Policy is already programmed in EPG or not
+	if _, ok := k8sNet.networkPolicy[np.PodSelector.PolicyName]; !ok {


policy names I think can be repeated across namespaces, this datastructure would have false positives for existence

chrisplo · 2017-12-12T18:34:59Z

netmaster/k8snetwork/networkpolicy.go

+	if _, ok := k8sNet.networkPolicy[np.PodSelector.PolicyName]; !ok {
+		//Create Policy and published to ofnet controller
+		if err = k8sNet.createPolicy(
+			defaultTenantName,


shouldn't this be the podselector's tenant?

chrisplo · 2017-12-12T18:35:12Z

netmaster/k8snetwork/networkpolicy.go

+		}
+		//k8sNet.policyPerEpg[np.PodSelector.GroupName] = attachPolicy
+	} else {
+		//XXX: Need check if policy rules are still same or not


noting missing implementation

chrisplo · 2017-12-12T18:35:28Z

netmaster/k8snetwork/networkpolicy.go

+	policyName string) (lRules *[]client.Rule) {
+	var listRules []client.Rule
+	for _, ingress := range np.Ingress {
+		isPortsCfg := false


just initiaze the variable to the expression eval instead of separate if block?

chrisplo · 2017-12-12T18:36:44Z

netmaster/k8snetwork/networkpolicy.go

+		attachPolicy := []string{}
+		for policyN := range policyMap {
+			attachPolicy = append(attachPolicy, policyN)
+		}


append(attachPolicy, policyMap...) ?

chrisplo · 2017-12-12T18:37:06Z

netmaster/k8snetwork/networkpolicy.go

+	attachPolicy := []string{}
+	for policyN := range policyMap {
+		attachPolicy = append(attachPolicy, policyN)
+	}


append(attachPolicy, policyMap...) ? or even

attachPolicy := make([]string, len(policyMap)) copy(attachPolicy, policyMap)

Include default deny policy Policy cleanup code Pod Add/Delete event processing unit test cases fix fix core issue in pod delete fix core issue in pod delete some cleanup in existing code K8s Network Policy support code changes

lihezhong93 · 2018-07-30T06:29:35Z

After creating EPG, are the spec pods updated to belong to this EPG? If not,will the flow table of ovs install this policy?

g1rana changed the title ~~Network policy~~ K8s Network policy Support Nov 30, 2017

g1rana force-pushed the network-policy branch from 78db510 to afad54f Compare November 30, 2017 01:18

network policy chages

f1c9df0

g1rana force-pushed the network-policy branch from afad54f to d94ce81 Compare November 30, 2017 05:50

g1rana requested review from chrisplo, dseevr, kahou82, rchirakk, srampal and tiewei November 30, 2017 05:55

g1rana force-pushed the network-policy branch 2 times, most recently from 6dc0038 to b518659 Compare December 1, 2017 02:19

tiewei reviewed Dec 1, 2017

View reviewed changes

g1rana force-pushed the network-policy branch from b518659 to 54c9c88 Compare December 3, 2017 01:01

chrisplo requested changes Dec 7, 2017

View reviewed changes

dseevr removed the request for review from rchirakk December 8, 2017 19:39

chrisplo reviewed Dec 11, 2017

View reviewed changes

chrisplo reviewed Dec 12, 2017

View reviewed changes

g1rana force-pushed the network-policy branch from 54c9c88 to 531e319 Compare December 15, 2017 02:58

Implement K8s NetworkPolicy Support

8896dbe

Include default deny policy Policy cleanup code Pod Add/Delete event processing unit test cases fix fix core issue in pod delete fix core issue in pod delete some cleanup in existing code K8s Network Policy support code changes

g1rana force-pushed the network-policy branch from 531e319 to 8896dbe Compare January 8, 2018 21:46

		@@ -343,7 +750,7 @@ func (k8sNet *k8sContext) watchK8sEvents(errChan chan error) {
		for k8sNet.isLeader() != true {
		time.Sleep(time.Millisecond * 100)

Conversation

g1rana commented Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of the changes

Type of fix: New feature

Fixes #1089

Uh oh!

g1rana commented Nov 30, 2017

Uh oh!

tiewei left a comment

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!

jerana Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

chrisplo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

g1rana commented Nov 30, 2017 •

edited

Loading

jerana Jan 4, 2018 •

edited

Loading

chrisplo left a comment •

edited

Loading