Allow project admins to create/edit/delete NetworkPolicies

[danwinship]

This is how it's supposed to work.

Some customers have argued that they don't actually want this behavior, but as far as we've been able to tell, that has always been based on the mistaken belief that this would allow project admins to give themselves access to other projects. But anyway, we can adjust the permissions for 3.7 if really necessary. This is definitely the right default behavior for 3.6.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1461208
https://trello.com/c/CBQ3Rc00

@knobunc

[knobunc]

LGTM

[knobunc]

@openshift/networking PTAL

@liggitt I assume this is ok by you?

[liggitt]

What facility allows cluster admins to lock down network access to/from a namespace?

[danwinship]

You mean "as a cluster admin, I want to be able to block two projects from communicating, even when both of those projects want to communicate with each other"? This PR does not allow for that. Do we need to?

(If you just meant "can cluster admins prevent project admins from creating policies allowing themselves to access particular other namespaces?" then that's the misunderstanding about NetworkPolicy I mentioned above. NetworkPolicy doesn't work that way. You can only allow other people to connect to your namespace, not allow yourself to connect to other people's namespaces.)

[eparis]

[test]

[danwinship]

transient networking problem ("Docker registry lookup failed"). [test]

[danwinship]

flake #14043 [test]

[knobunc]

[test] Last was flake #14897 logs

[deads2k]

But anyway, we can adjust the permissions for 3.7 if really necessary. This is definitely the right default behavior for 3.6.

You can't actually. We never auto-tighten and tightening by hand is generally scary for people.

[deads2k]

So a few things about this need be very clear in our docs:

1. this API could be removed as early as 3.8 since networkpolicies is moving
2. you cannot label your namespaces, so you can't use this to select particular namespaces
3. If we do allow users to label their namespaces, then it becomes easy to search for likely matches when attacking another namespace
4. When choosing a namespace label selector, you probably need it to be a UID or something

@smarterclayton This will significantly complicate any, "let users label their namespaces" feature in the future. Are we going to give up on the self-labeling feature (which makes this resource far less useful), lock down the allowed label selectors for networkpolicies (which has compatibility problems), or something else?

[danwinship]

Hrmph. Where were you when I was trying to convince everyone of all that upstream a few months ago.

I'm assuming that eventually upstream will add a new selector to NetworkPolicy to allow you to select namespaces by name (or else require that all namespaces have certain well-known labels with values that can't be modified by users). Until then, yes, NetworkPolicy NamespaceSelectors are either useless or insecure, depending on how other things are configured.

But anyway, that's not really relevant to this PR: we're already shipping NetworkPolicy (as Tech Preview), people are already using it, and there are use cases for it even without using NamespaceSelectors. (And there are even scenarios where project admins can write NetworkPolicies using NamespaceSelectors, if the cluster admins modify the default project template to automatically add a "name" label to every namespace).

So, anyway, this PR makes it so that project admins can create/edit their own NetworkPolicies. Is anyone objecting to merging this?

[deads2k]

Hrmph. Where were you when I was trying to convince everyone of all that upstream a few months ago.

I remember bringing it up in a sig-auth call quite some time ago and suggesting a specific whitelist of namespace names to avoid accidents and increase clarity for the simplest cases. It would also have eliminated the labelling need problem. Doing that would have effectively discouraged label selection except by cluster-admins which I think would work well.

So, anyway, this PR makes it so that project admins can create/edit their own NetworkPolicies. Is anyone objecting to merging this?

I'm currently against because I'm concerned about unexpected interactions with work on a feature to allow for namespace labelling by users. If we flesh one or other other out more completely, perhaps we can come up with a set of rules that will work properly. Throwing out the first idea that pops into my head, admission plugin (so that we can enforce on any cluster with webhook admission) that only allows namespace labels under a particular namespace. You get most of the power you want today and we don't get excessively boxed in for namespace labels later.

[smarterclayton]

Enabling this doesn't block us in for namespace labels. End users are able to allow labels if they want, we just need to communicate in the future that its cooperative, not adversarial. Adversarial label selection might come from autogenerated labels users can't edit. Either way, this should go into 3.6, and protections on label selection when we enable them will have to be taken into account on those systems at that time. If we allow users to set labels on projects, they're going to have to be namespaced to prevent collisions with crq or other existing use cases anyway.

[smarterclayton]

Restated another way - we're already constrained in how we allow users to set labels, and this only makes it more obvious.

[deads2k]

Alright, ret[test] (others touched this file) and [merge].

[deads2k]

So a few things about this need be very clear in our docs:

1. this API could be removed as early as 3.8 since networkpolicies is moving
2. you cannot label your namespaces, so you can't use this to select particular namespaces
3. If we do allow users to label their namespaces, then it becomes easy to search for likely matches when attacking another namespace

These should still be documented. This could easily become pretend protection in an update that allows labeling and online dedicated admins in particular may care.

[openshift-bot]

Evaluated for origin test up to 292931f

[openshift-bot]

Evaluated for origin merge up to 292931f

[adelton]

@danwinship, should this PR get merged? The title (still) contains [DO NOT MERGE] so it's confusing ...

[deads2k]

@danwinship, should this PR get merged? The title (still) contains [DO NOT MERGE] so it's confusing ...

He added it when I wanted the hold (very polite). I've updated it now.

[openshift-bot]

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2808/) (Base Commit: 91c5480) (PR Branch Commit: 292931f)

[openshift-bot]

continuous-integration/openshift-jenkins/merge FAILURE (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1173/) (Base Commit: 91c5480) (PR Branch Commit: 292931f)

[deads2k]

green test on recent head with no other permission changes at the top of the queue or in master after base.