Egress Network policy fixes #13071

pravisankar · 2017-02-23T04:11:33Z

We allow multiple policies to be created per namespace but we only recommend
one policy per namespace as per our design.
In case of multiple policies per namespace, we drop all corresponding ovs rules
because its hard to determine which policy to apply first but currently we keep
the latest policy firewall rules in this case. To be consistent with ovs rules
behavior, this fix will drop all firewall rules as well.
Renamed proxy.updateNetworkPolicy() to proxy.updateEgressNetworkPolicy()
to make it more clear and not to get confused with np.updateNetworkPolicy()
in networkpolicy.go

pravisankar · 2017-02-23T04:25:53Z

We also need to drop all egress firewall rules for global namespaces. This requires maintaining namespace to vnid map in the proxy. I will submit this change in a separate pr.

@openshift/networking PTAL

danwinship

Adding the rationale comments is definitely a good idea. I should have done that before.

danwinship · 2017-02-23T15:04:14Z

pkg/sdn/plugin/controller.go

@@ -384,10 +384,12 @@ func (plugin *OsdnNode) updateEgressNetworkPolicyRules(vnid uint32) {
 	} else if vnid == 0 {
 		glog.Errorf("EgressNetworkPolicy in global network namespace is not allowed (%s); ignoring", policyNames(policies))
 	} else if len(namespaces) > 1 {
+		// Rationale: If multiple namespaces have their own egress network policy, it's hard to determine the order of policies to apply. Also it may be wrong to force a network policy on a namespace which only wants to join the network.


Mention something about VNIDs or NetNamespaces there since that's the source of the trouble; EgressNetworkPolicies are defined per-Namespace, but are implemented per-NetNamespace.

danwinship · 2017-02-23T15:07:05Z

pkg/sdn/plugin/proxy.go

+			if policy.UID != ref.policyUID {
+				// We only allow one policy per namespace otherwise it's hard to determine which policy to apply first
+				glog.Errorf("found multiple egress policies, dropping all firewall rules for namespace: %s", policy.Namespace)
+				delete(proxy.firewall, policy.Namespace)


Hm... this isn't right, since it means that the next time an ENP in that namespace gets updated, proxy.firewall[policy.Namespace] will be unset, and so that ENP will get applied (even if there are also still other ENPs in the namespace too).

We need to remember all the ENPs we've seen, but only apply the policy when there's just one in the Namespace.

pravisankar · 2017-02-23T23:03:51Z

@openshift/networking @danwinship updated, ptal

pravisankar · 2017-02-28T21:05:19Z

Added one more fix (new commit): SDN egress policy should not firewall endpoints from global namespaces.

pravisankar · 2017-02-28T21:20:29Z

[test]

danwinship · 2017-03-03T13:44:41Z

pkg/sdn/plugin/proxy.go

 	}

+	// Add/Update/Delete firwall rules for the namespace


typo "firwall"

danwinship · 2017-03-03T13:48:51Z

pkg/sdn/plugin/proxy.go

 type proxyFirewallItem struct {
-	policy osapi.EgressNetworkPolicyRuleType
-	net    *net.IPNet
+	namespaceFirewalls []policyFirewallItem


it seems like the code below would be a lot simpler if this was a map[ktypes.UID]policyFirewallItem rather than a []policyFirewallItem. (You might even be able to get rid of policyFirewallItem and just do map[ktypes.UID][]firewallItem?)

Also, maybe type/field naming would help make the code clearer. It's difficult to keep track of all the different "firewalls" and "items"...

doing the map looks plausible and would make the code more readable, so +1 from me if it works

danwinship · 2017-03-03T13:53:39Z

pkg/sdn/plugin/proxy.go

@@ -98,7 +103,39 @@ func (proxy *OsdnProxy) watchEgressNetworkPolicies() {
 	})
 }

+func (proxy *OsdnProxy) watchNetNamespaces() {


blah... we need to abstract out the EgressNetworkPolicy code better so that node and proxy can share more code rather than duplicating. Can you add a TODO comment for that?

danwinship · 2017-03-03T13:55:59Z

pkg/sdn/plugin/proxy.go

+	ignorePolicy := false
+	func() {
+		proxy.idLock.Lock()
+		defer proxy.idLock.Unlock()


I would have a slight preference for splitting this out into a real helper function rather than an inline func() but if you really prefer it this way you can keep it

Could even do:

if func() bool { Lock() defer Unlock() if global vnid { log error return true } return false }() { return }

but a helper function is even clearer.

danwinship · 2017-03-03T13:56:25Z

pkg/sdn/plugin/proxy.go

@@ -111,7 +148,6 @@ func (proxy *OsdnProxy) updateEgressNetworkPolicy(policy osapi.EgressNetworkPoli
 	}

 	// Add/Update/Delete firwall rules for the namespace
-	ns := policy.Namespace


doesn't belong in this patch?

danwinship · 2017-03-13T13:54:25Z

pkg/sdn/plugin/proxy.go

+		updated := false
+		if ref, ok := proxy.firewall[ns]; ok {
+			for uid := range ref.namespaceFirewalls {
+				if uid == policy.UID {


don't need a loop now that you're using a map; just if _, ok := ref.namespaceFirewalls[policy.UID]; ok {

Although you can get rid of updated in that case too. Just check if proxy.firewall[ns] exists, and create it if not, and then set proxy.firewall[ns].namespaceFirewalls[policy.UID]

danwinship · 2017-03-13T13:59:30Z

pkg/sdn/plugin/proxy.go

+		if len(ref.namespaceFirewalls) == 1 {
+			for uid := range ref.namespaceFirewalls {
+				ref.activePolicy = &uid
+				proxy.firewall[ns] = ref


If you made proxy.firewall be map[string]*proxyFirewallItem rather than map[string]proxyFirewallItem then you wouldn't need the reassignment here (and below).

danwinship · 2017-03-13T14:02:40Z

pkg/sdn/plugin/proxy.go

+		} else {
+			// We only allow one policy per namespace otherwise it's hard to determine which policy to apply first
+			glog.Errorf("Found multiple egress policies, dropping all firewall rules for namespace: %q", ns)
+			ref.activePolicy = nil


Hm... this is the opposite of the behavior in controller.go, where having multiple EgressNetworkPolicies means all traffic gets dropped, rather than all traffic being allowed.

- We allow multiple policies to be created per namespace but we only recommend one policy per namespace as per our design. In case of multiple policies per namespace, we drop all corresponding ovs rules because its hard to determine which policy to apply first but currently we keep the latest policy firewall rules in this case. To be consistent with ovs rules behavior, this fix will drop all firewall rules as well. - Renamed proxy.updateNetworkPolicy() to proxy.updateEgressNetworkPolicy() to make it more clear and not to get confused with np.updateNetworkPolicy() in networkpolicy.go

pravisankar · 2017-03-13T23:06:20Z

@danwinship Updated, ptal

openshift-bot · 2017-03-13T23:06:45Z

Evaluated for origin test up to 1aaf633

openshift-bot · 2017-03-14T00:19:23Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/176/) (Base Commit: 209f100)

danwinship · 2017-03-22T16:45:34Z

[merge]

openshift-bot · 2017-03-22T16:53:29Z

Evaluated for origin merge up to 1aaf633

openshift-bot · 2017-03-22T18:13:35Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/178/) (Base Commit: e61510a) (Image: devenv-rhel7_6097)

danwinship suggested changes Feb 23, 2017

View reviewed changes

pravisankar force-pushed the fix-proxy-update-egress-policy branch from 277a0c1 to 9aabbd0 Compare February 23, 2017 23:01

pravisankar force-pushed the fix-proxy-update-egress-policy branch from 9aabbd0 to 7b55356 Compare February 28, 2017 21:02

danwinship reviewed Mar 3, 2017

View reviewed changes

pravisankar force-pushed the fix-proxy-update-egress-policy branch 3 times, most recently from 7d452cd to 1c76a3d Compare March 10, 2017 18:26

danwinship suggested changes Mar 13, 2017

View reviewed changes

Ravi Sankar Penta added 2 commits March 13, 2017 14:22

SDN egress policy should not firewall endpoints from global namespaces

1aaf633

pravisankar force-pushed the fix-proxy-update-egress-policy branch from 1c76a3d to 1aaf633 Compare March 13, 2017 23:05

danwinship approved these changes Mar 22, 2017

View reviewed changes

openshift-bot merged commit 04d74e0 into openshift:master Mar 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Egress Network policy fixes #13071

Egress Network policy fixes #13071

pravisankar commented Feb 23, 2017

pravisankar commented Feb 23, 2017

danwinship left a comment

danwinship Feb 23, 2017

danwinship Feb 23, 2017

pravisankar commented Feb 23, 2017

pravisankar commented Feb 28, 2017

pravisankar commented Feb 28, 2017

danwinship Mar 3, 2017

danwinship Mar 3, 2017

dcbw Mar 6, 2017

danwinship Mar 3, 2017

danwinship Mar 3, 2017

dcbw Mar 6, 2017

danwinship Mar 3, 2017

danwinship Mar 13, 2017

danwinship Mar 13, 2017

danwinship Mar 13, 2017

pravisankar commented Mar 13, 2017

openshift-bot commented Mar 13, 2017

openshift-bot commented Mar 14, 2017

danwinship commented Mar 22, 2017

openshift-bot commented Mar 22, 2017

openshift-bot commented Mar 22, 2017 •

edited

Loading

Egress Network policy fixes #13071

Egress Network policy fixes #13071

Conversation

pravisankar commented Feb 23, 2017

pravisankar commented Feb 23, 2017

danwinship left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pravisankar commented Feb 23, 2017

pravisankar commented Feb 28, 2017

pravisankar commented Feb 28, 2017

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

pravisankar commented Mar 13, 2017

openshift-bot commented Mar 13, 2017

openshift-bot commented Mar 14, 2017

danwinship commented Mar 22, 2017

openshift-bot commented Mar 22, 2017

openshift-bot commented Mar 22, 2017 • edited Loading

openshift-bot commented Mar 22, 2017 •

edited

Loading