Openshift Multi-tenant Sandbox Hooks #600

shawn-hurley · 2017-12-19T19:30:56Z

Describe what this PR does and why we need it:
Implementation of #572
Changes proposed in this pull request

Adding post sandbox create/destroy hooks
Adding actual join/isolate network hook implementation
Adding cluster specific shouldJoinNetworks method to retrieve the hooks to be used.
Using openshift api to get structures for openshift calls partial fix for We should move to using the openshift client-go #590

shawn-hurley · 2017-12-19T19:53:57Z

(5) Implement network isolation proposal for multi-tenet environment.

coveralls · 2017-12-19T20:10:23Z

Changes Unknown when pulling 30dae9d on shawn-hurley:network-iso-impl into ** on openshift:master**.

coveralls · 2017-12-19T21:38:37Z

Changes Unknown when pulling 8e2b60a on shawn-hurley:network-iso-impl into ** on openshift:master**.

coveralls · 2017-12-19T22:12:48Z

Changes Unknown when pulling d8a6f0b on shawn-hurley:network-iso-impl into ** on openshift:master**.

coveralls · 2017-12-20T15:44:40Z

Changes Unknown when pulling b3df86b on shawn-hurley:network-iso-impl into ** on openshift:master**.

shawn-hurley · 2017-12-20T16:45:35Z

This PR is related to openshift/openshift-ansible#6536.

They do not block each other, but they are needed to work together.

coveralls · 2017-12-20T17:39:24Z

Changes Unknown when pulling 63b6aac on shawn-hurley:network-iso-impl into ** on openshift:master**.

shawn-hurley · 2018-01-03T20:51:39Z

pkg/clients/openshift.go

+}
+
+// JoinNamespacesNetworks - Will take the net namespace to be added to a network,
+// and the namespace ID of that netwok.


make this more clear that the namespace id is the network that we are joining to.

shawn-hurley · 2018-01-03T20:56:20Z

pkg/runtime/openshift.go

+	pluginName, err := ocli.GetClusterNetworkPlugin()
+	log.Debugf("plugin for the network - %v", pluginName)
+	if err != nil {
+		// Plugins could not be defined oc cluster up or other use cases.


make this statement more clear

shawn-hurley · 2018-01-03T21:00:24Z

pkg/runtime/runtime.go

@@ -172,6 +186,16 @@ func (p provider) CreateSandbox(podName string,
 	}

 	log.Info("Successfully created apb sandbox: [ %s ], with %s permissions in namespace %s", podName, apbRole, namespace)
+	log.Info("Running post create sandbox fuctions if defined.")
+	for i, f := range p.postSandboxCreate {
+		log.Debug("Running function: %v", i)


Debug -> Debugf

Also, make this more useful.

shawn-hurley · 2018-01-03T21:02:20Z

pkg/runtime/sandbox_hooks.go

+// return error.
+type PostSandboxCreate func(string, string, []string, string) error
+
+// AddPostCreateSandbox - Adds a post create sandbox to the runtime.


Adds a post create sandbox to the runtime -> Adds a post create sandbox function to the runtime.

shawn-hurley · 2018-01-03T21:03:19Z

pkg/runtime/sandbox_hooks.go

+}
+
+// PostSandboxDestroy - The post sand box destroy function will be called
+// after the sandbox is destory. This could mean the namespace is kept around


destroy -> destroyed

shawn-hurley · 2018-01-03T21:04:05Z

pkg/runtime/sandbox_hooks.go

+
+// PostSandboxDestroy - The post sand box destroy function will be called
+// after the sandbox is destory. This could mean the namespace is kept around
+// if the error and configuration conditions are met. This function should not


error/APB errors ....

coveralls · 2018-01-03T22:00:39Z

Changes Unknown when pulling ca3ff56 on shawn-hurley:network-iso-impl into ** on openshift:master**.

jwmatthews · 2018-01-04T20:48:18Z

@smarterclayton had some concerns with this PR, let's hold off on merging until we learn more about the concerns and can address.

jwmatthews · 2018-01-05T14:47:17Z

Adding some additional background to help others understand the context of this PR.

Background of the issue:
With APBs there will be 2 namespaces at play.
"target" namespace, which is the namespace the deployed services will run in
"transient" namespace, the temporary namespace the broker creates for the APB to run
only the APB code runs in here, then the namespace is destroyed.

A typical workflow is:

'target' namespace exists
User runs an action that triggers an APB action
Broker creates a new namespace, the 'transient' namespace
Broker runs the APB in the 'transient' namespace
a.. The actual ansible logic for the APB runs in the 'transient' namespace
OCP Resources are manipulated in the 'target' namespace
Broker destroys the 'transient' namespace

In a multi-tenant setup, the namespace networks are isolated. This means the APB logic is unable to talk to the deployed services.

The goal of the PR referenced is to allow the transient namespace to communicate to the target namespace.

knobunc

Nits

knobunc · 2018-01-05T15:05:27Z

pkg/clients/openshift.go

+}
+
+// JoinNamespacesNetworks - Will take the net namespace to be added to a network,
+// and the namespace ID of the netwok that is being added to.


netwok -> network

knobunc · 2018-01-05T15:05:34Z

pkg/clients/openshift.go

+}
+
+// IsolateNamespacesNetworks - Will take the net namespace to be added to a network,
+// and the namespace ID of the netwok that is being added to..


netwok -> network

danwinship · 2018-01-05T15:37:30Z

Summing up the bluejeans conversation:

The only obvious alternative approach would be to have the APB deploy a helper pod in the target namespace and then use "oc exec" to run commands through that. But that would probably require lots of additional complexity in every APB that needed these features, whereas the proposal here puts all of the extra work in the infrastructure and makes things easier for the individual APBs.
If the cluster is using the multitenant plugin, then joining the networks together is the right answer. I would have suggested just using oc adm pod-network ... but using the API directly works as well. Also, there's no need to re-isolate them afterward since "isolating" just means giving the transient NetNamespace a unique NetID again, but there's no point in doing that since you're about to delete it anyway.
If the cluster is using the redhat/openshift-ovs-networkpolicy plugin, or basically any third-party plugin, then you can't "join networks", and you'll instead have to create a NetworkPolicy in the target namespace, allowing access from the transient namespace. (Which will require putting some sort of label on the transient Namespace object first so that you can match it with a LabelSelector from the NetworkPolicy.) (If the cluster is using the multitenant plugin, you can still do this anyway; it will just be a no-op.)
Looking at the "default" ClusterNetwork record is the best currently-available way to figure out what network plugin is in use, but it's only guaranteed to be right if one of the openshift plugins is being used; otherwise, it's possible that it will contain stale data. (Our "how to change network plugins" documentation tells people to delete the ClusterNetwork object when switching to a third-party plugin, but they might not.). So the safe/right way to set up cross-namespace network access is:
- always create a NetworkPolicy
- if the default ClusterNetwork claims that multitenant is in use, then also do the join-projects hack, but it's possible that the ClusterNetwork record is wrong, so don't actually error out if the master ignores the request to join the networks. (But maybe warn about it?)

shawn-hurley · 2018-01-05T15:41:54Z

@danwinship 1 clarification

Also, there's no need to re-isolate them afterward since "isolating" just means giving the transient NetNamespace a unique NetID again, but there's no point in doing that since you're about to delete it anyway.

we could potentially keep the transient namespace alive so that a cluster admin could diagnose issues if an APB fails. In that case, we should re-isolate the network correct?

danwinship · 2018-01-05T15:41:06Z

pkg/clients/openshift.go

+const (
+	// ChangePodNetworkAnnotation - Annotation used for changing a pod
+	// network, used to join networks together.
+	ChangePodNetworkAnnotation = "pod.network.openshift.io/multitenant.change-network"


ah, right... I was going to say "that constant should be available in networkoapi", but it's not, because we had decided that this is "internal API", not public API. So maybe that's an argument that you should use oc adm pod-network instead? (But the API is still guaranteed to keep existing, since we have to preserve compatibility between old/new clients/servers, so it's safe for you to depend on it if you'd rather keep the code this way.)

I think that we are attempting to remove shelling out in our code, therefore I think on a whole we would prefer to keep using the API endpoints.

@jmrodri @eriknelson can you guys weigh in as far as broker code base is concerned? It sounds like the question is if we have to duplicate some constants and some logic around checking if an annotation has been updated vs shelling out to the oc command.

@danwinship Is that a fair characterization of the concern here? Do you see more code duplication then the above mentioned?

danwinship · 2018-01-05T15:42:13Z

pkg/clients/openshift.go

+// GetClusterNetworkPlugin - Get cluster network
+func (o OpenshiftClient) GetClusterNetworkPlugin() (string, error) {
+	net := &networkoapi.ClusterNetwork{}
+	err := o.networkClient.Get().Resource("clusternetworks").Name("default").Do().Into(net)


Name(networkoapi.ClusterNetworkDefault)

danwinship · 2018-01-05T15:43:58Z

pkg/runtime/openshift.go

+	}
+	return wait.ExponentialBackoff(backoff, func() (bool, error) {
+		return didAnnotationUpdate("join", netns.NetName)
+	})


as mentioned above, this could fail if the ClusterNetwork object is stale and mistakenly claims that they're using multitenant when really they're using a third-party plugin now

danwinship · 2018-01-05T15:45:06Z

pkg/runtime/openshift.go

+	// Case insensitive check here because want to prepare if things change.
+	if strings.ToLower(pluginName) == "redhat/openshift-ovs-multitenant" {
+		log.Debugf("stating that the pluginname is multitenant - %v", pluginName)
+		return true, addPodNetworks, isolatePodNetworks


as mentioned above, there's really no need to re-isolate the transient network on cleanup

danwinship · 2018-01-05T16:00:54Z

we could potentially keep the transient namespace alive so that a cluster admin could diagnose issues if an APB fails. In that case, we should re-isolate the network correct?

You wouldn't want to keep things in the same state as when the APB ran? I guess there are arguments both ways.

Hm... I was about to say "leaving the networks joined won't hurt anything", but I just realized that's not actually the case: joining networks breaks multicast and EgressNetworkPolicy.

For multicast you can make things work by having the setup process set the "multicast-enabled" plugin on the transient NetNamespace if it's set on the target NetNamespace (https://docs.openshift.org/latest/admin_guide/managing_networking.html#admin-guide-networking-multicast). (Ideally you should not just set it unconditionally; the nodes will log warnings if you join two namespaces and one of them has the multicast annotation but the other doesn't.)

But for EgressNetworkPolicy, there's no way to make things work; if the target namespace has any EgressNetworkPolicies defined, then joining another namespace to it will break all cluster-external traffic from that namespace.

EgressNetworkPolicy isn't that widely used though, and we're thinking about deprecating it, so maybe you can just punt on that and say that APBs can't be deployed into a namespace that has any EgressNetworkPolicies defined in it, if the cluster is using the multitenant plugin.

shawn-hurley · 2018-01-05T16:07:03Z

You wouldn't want to keep things in the same state as when the APB ran? I guess there are arguments both ways.

We are currently removing the role bindings and such even if we are keeping the namespace. I figured that we would want to do the same and if the cluster admin would want to re-add the networks together then they could. @jwmatthews Do you have any preference here. I don't think it hurts to leave them attached, just thought the right thing was to remove our failed run from the cluster as much as possible?

EgressNetworkPolicy isn't that widely used though, and we're thinking about deprecating it, so maybe you can just punt on that and say that APBs can't be deployed into a namespace that has any EgressNetworkPolicies defined in it, if the cluster is using the multitenant plugin.

Sorry, I have not read about this policy yet, do you think that a large documentation warning on this would be sufficient? Should the broker be able to look up some value and error if a setting/condition is met?

coveralls · 2018-01-05T22:08:32Z

Changes Unknown when pulling 91cd0ef on shawn-hurley:network-iso-impl into ** on openshift:master**.

* Post destroy sandbox hook to isolate the networks * Post create sandbox hook to join the networks. * Adding implementations for these hooks. * Adding the new permissions to the "production" deployment temp. * Addressing wording and spelling issues. * Adding network policy object creation and adressing comments. * Adding ability to delete network policy on sandbox deletion.

shawn-hurley · 2018-01-08T15:11:28Z

@enj @danwinship @knobunc Can you guys double check this PR. I believe that I have updated it to match the comments and process that was agreed upon. Thanks!

coveralls · 2018-01-08T15:51:37Z

Changes Unknown when pulling 746ff2f on shawn-hurley:network-iso-impl into ** on openshift:master**.

danwinship · 2018-01-08T17:43:34Z

You removed the "re-isolate" part at the end, but I think you should put it back. In particular, joining the temp namespace to the target namespace could cause disruption of multicast and EgressNetworkPolicy, so you want that to be undisrupted when you finish installing.
addPodNetworks() still always returns an error if the join fails, but there's the problem that the user might be using a third-party plugin and ClusterNetwork has stale info, in which case joining would time out (because the controller that watches for the "join" annotation wouldn't be running). Maybe the right fix here is just to document it though: "if you're using a third-party network plugin, and get such-and-such error message, then the fix is to run oc delete clusternetwork default to delete the stale config object that claims you're running the multitenant plugin".
The new NetworkPolicy-related code looks correct. (I assume that that value you're sticking in apb-pod-name is unique-ish?)

shawn-hurley · 2018-01-08T17:49:10Z

addPodNetworks() still always returns an error if the join fails, but there's the problem that the user might be using a third-party plugin and ClusterNetwork has stale info, in which case joining would time out (because the controller that watches for the "join" annotation wouldn't be running). Maybe the right fix here is just to document it though: "if you're using a third-party network plugin, and get such-and-such error message, then the fix is to run oc delete clusternetwork default to delete the stale config object that claims you're running the multitenant plugin".

We are logging the error but the way in which the post hook works is it will not cause the rest of the APB execution to quit. I think that this one we do not need to change and can add that documentation regarding the message.

The new NetworkPolicy-related code looks correct. (I assume that that value you're sticking in apb-pod-name is unique-ish?)

It is a UUID so it does have some uniqueness guarantee, as far as I understand?

coveralls · 2018-01-08T18:43:23Z

Changes Unknown when pulling ec2971c on shawn-hurley:network-iso-impl into ** on openshift:master**.

liggitt · 2018-01-09T18:27:52Z

pkg/runtime/runtime.go

@@ -171,7 +186,44 @@ func (p provider) CreateSandbox(podName string,
 		}
 	}

+	// Must create a Network policy to allow for comunication from the APB pod to the target namespace.
+	networkPolicy := &networkingv1.NetworkPolicy{


is there a "deny all ingress" network policy created in the apb pod namespace that will prevent the user from initiating communication in the other direction?

Correct me if I am wrong, but if the network policy does not exist to allow the traffic the base state of the plugin ovs-networkpolicy is to disallow traffic.

If you are using ovs-multitenant plugin I don't think that you can.

@danwinship are these statements correct?

The base state of the networkpolicy plugin is to allow all traffic. But you said before there's nothing listening to any ports in the temp namespace, so this shouldn't matter?

But if you want to be paranoid, you could add a policy:

kind: NetworkPolicy apiVersion: extensions/v1beta1 metadata: name: deny-all spec: podSelector: ingress: []

I think that if we ever open a port for the APB to listen on then we would want to control the network with NetworkPolicy as @danwinship said. I don't think it is a concern as there is nothing exposed to the network in the apb pod namespace.

@liggitt and I think @enj you had the same concern, does the above sound acceptable to you?

danwinship · 2018-01-09T20:01:19Z

We are logging the error but the way in which the post hook works is it will not cause the rest of the APB execution to quit.

OK

It is a UUID so it does have some uniqueness guarantee, as far as I understand?

Yeah, that's good. Not predictable, and an attacker can't trick you into using a value of their choice.

rthallisey

Just some additional nits

rthallisey · 2018-01-10T19:39:55Z

pkg/runtime/openshift.go

+	log.Debugf("adding pod networks together namespace: %v, target namespaces: %v", ns, targetNS)
+	// Check to make sure that we have a target namespace.
+	if len(targetNS) < 1 {
+		return fmt.Errorf("Can not find target namespace to add to its networ")


its networ -> its network

rthallisey · 2018-01-10T19:44:53Z

pkg/runtime/openshift.go

+	// Check to make sure that we have a target namespace.
+	if len(targetNS) < 1 {
+		return fmt.Errorf("Can not find target namespace to add to its networ")
+	}


its networ->its network

coveralls · 2018-01-10T22:40:43Z

Changes Unknown when pulling 2e4121e on shawn-hurley:network-iso-impl into ** on openshift:master**.

jmontleon · 2018-01-11T15:04:10Z

I tested this (albeit before changes to support the tech preview network-policy plugin) and I was able to update an APB in a multi-tenant environment.

jmontleon

ACK. When I tested I was able to update an APB that required talking to the deployed pod in a multi-tenant environment.

rthallisey · 2018-01-11T15:10:37Z

pkg/clients/openshift.go

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


Why return result if it's not used by the caller?

no good reason. eventually, you may need it and you can ignore it when you call the method.

do you have strong feelings on this one?

It doesn't matter to me if you think it will be used eventually I'm fine with it.

I would double check though that result isn't going to be the same as thenetns param. I would have to do some testing to verify, but I don't think NetName and NetId will change.

rthallisey · 2018-01-11T15:11:18Z

pkg/clients/openshift.go

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


same as above.

eriknelson · 2018-01-11T18:51:16Z

pkg/runtime/kubernetes.go

@@ -3,3 +3,7 @@ package runtime
 func (k kubernetes) getRuntime() string {
 	return "kubernetes"
 }
+
+func (k kubernetes) shouldJoinNetworks() (bool, PostSandboxCreate, PostSandboxDestroy) {
+	return false, nil, nil


Confirming for my own edification -- this is strictly an Openshift feature?

The openshift plugin for the multitenant environment is only for openshift.

The network policy stuff is for both k8s and openshift so that is apart of the actual create sandbox and destroy sandbox methods.

eriknelson

VISIACK, don't see anything that sticks out to me, just a minor spelling comment.

eriknelson · 2018-01-11T18:58:27Z

pkg/runtime/sandbox_hooks.go

+}
+
+// PostSandboxDestroy - The post sand box destroy function will be called
+// after the sandbox is destoryed. This could mean the namespace is kept around


destoryed -> destroyed

jmrodri

ACK, I like the PostHooks

* adding openshift-api to vendor * Adding ability to add sandbox hooks to join pod networks. * Post destroy sandbox hook to isolate the networks * Post create sandbox hook to join the networks. * Adding implementations for these hooks. * Adding the new permissions to the "production" deployment temp. * Addressing wording and spelling issues. * Adding network policy object creation and addressing comments. * Adding ability to delete network policy on sandbox deletion. * re-adding isolation of net namespaces.

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 19, 2017

shawn-hurley requested review from jmrodri, jmontleon, rthallisey and eriknelson December 19, 2017 19:49

shawn-hurley added enhancement needs-review labels Dec 19, 2017

shawn-hurley force-pushed the network-iso-impl branch 2 times, most recently from 8e2b60a to d8a6f0b Compare December 19, 2017 21:28

shawn-hurley force-pushed the network-iso-impl branch from d8a6f0b to b3df86b Compare December 20, 2017 14:58

shawn-hurley force-pushed the network-iso-impl branch from b3df86b to 63b6aac Compare December 20, 2017 17:01

shawn-hurley commented Jan 3, 2018

View reviewed changes

shawn-hurley force-pushed the network-iso-impl branch from 63b6aac to ca3ff56 Compare January 3, 2018 21:18

jwmatthews added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 4, 2018

knobunc requested changes Jan 5, 2018

View reviewed changes

danwinship reviewed Jan 5, 2018

View reviewed changes

shawn-hurley force-pushed the network-iso-impl branch from 852f565 to 91cd0ef Compare January 5, 2018 21:27

shawn-hurley removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 5, 2018

shawn-hurley force-pushed the network-iso-impl branch from 91cd0ef to 746ff2f Compare January 8, 2018 15:08

shawn-hurley removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 8, 2018

re-adding isolation of net namespaces.

ec2971c

rthallisey added the 3.10 | release-1.2 Kubernetes 1.10 | Openshift 3.10 | Broker release-1.2 label Jan 9, 2018

liggitt reviewed Jan 9, 2018

View reviewed changes

rthallisey reviewed Jan 10, 2018

View reviewed changes

addressing pr comments.

2e4121e

jmontleon approved these changes Jan 11, 2018

View reviewed changes

rthallisey suggested changes Jan 11, 2018

View reviewed changes

rthallisey approved these changes Jan 11, 2018

View reviewed changes

eriknelson reviewed Jan 11, 2018

View reviewed changes

eriknelson approved these changes Jan 11, 2018

View reviewed changes

jmrodri approved these changes Jan 11, 2018

View reviewed changes

jmrodri merged commit fba586e into openshift:master Jan 11, 2018

shawn-hurley mentioned this pull request Jan 11, 2018

Adding ability for ASB to interact with network resources. openshift/openshift-ansible#6536

Merged

Openshift Multi-tenant Sandbox Hooks #600

Openshift Multi-tenant Sandbox Hooks #600

Conversation

shawn-hurley commented Dec 19, 2017

shawn-hurley commented Dec 19, 2017

coveralls commented Dec 19, 2017

coveralls commented Dec 19, 2017

coveralls commented Dec 19, 2017 • edited

coveralls commented Dec 20, 2017

shawn-hurley commented Dec 20, 2017

coveralls commented Dec 20, 2017 • edited

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

coveralls commented Jan 3, 2018

jwmatthews commented Jan 4, 2018

jwmatthews commented Jan 5, 2018

knobunc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwinship commented Jan 5, 2018

shawn-hurley commented Jan 5, 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

danwinship commented Jan 5, 2018

shawn-hurley commented Jan 5, 2018

coveralls commented Jan 5, 2018 • edited

shawn-hurley commented Jan 8, 2018

coveralls commented Jan 8, 2018

danwinship commented Jan 8, 2018

shawn-hurley commented Jan 8, 2018

coveralls commented Jan 8, 2018 • edited

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

danwinship commented Jan 9, 2018

rthallisey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 10, 2018

jmontleon commented Jan 11, 2018

jmontleon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rthallisey Jan 11, 2018 • edited

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

eriknelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmrodri left a comment

Choose a reason for hiding this comment

coveralls commented Dec 19, 2017 •

edited

coveralls commented Dec 20, 2017 •

edited

coveralls commented Jan 5, 2018 •

edited

coveralls commented Jan 8, 2018 •

edited

rthallisey Jan 11, 2018 •

edited