Create NPEP-122 for Tenancy API update.

[npinaeva]

Change Tenancy implementation from SameLabels/NotSameLabels ANP peers to a separate object
that will define Tenancy considering subject and peers are related.

Tracking issue #122

[netlify]

✅ Deploy Preview for kubernetes-sigs-network-policy-api ready!

Name	Link
🔨 Latest commit	310e46a
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-network-policy-api/deploys/654959808c3764000881fb9c
😎 Deploy Preview	https://deploy-preview-123--kubernetes-sigs-network-policy-api.netlify.app/npeps/npep-122
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[k8s-ci-robot]

Hi @npinaeva. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[npinaeva]

/ok-to-test

[astoycos]

Still working through this but I've got the review mostly done.

[Dyanngg]

We can touch upon naming later but I just what to point out that, this categorization of tenants is quite different than our original sameLabels proposal, where namespaces who do not have the tenancy label at all (as opposed to have an empty label) are not affected by the policy.
When I read the tenancyLabels field, it is supposed to mean "namespaces that have the exact same values for these labels are considered a single tenant", but it does not and should not IMO imply that, namespaces who are missing this label automatically form a tenant by themselves

[npinaeva]

Current design is supposed to be more explicit on which namespaces should be affected, and also allow different configurations. So if you don't want namespace without a "tenancy label" to be affected, you just add
key: <tenancy label>, operator: Exists to the spec.namespaceSelector. But at the same time if you do want to make a separate tenant for all namespaces without that label you leave empty selector.

Also I think tenancyLabels does exactly what you said: "namespaces that have the exact same values for these labels are considered a single tenant", because all namespaces with empty label will be one tenant, and all namespaces without a label (say they all have None value for it) will be one tenant. And you can easily avoid namespaces with unset labels as I pointed above.

[danwinship]

We could alternatively say that namespaces without all of the tenancyLabels are always excluded from the definition. IOW, namespaceSelector automatically implicitly includes key: X, operator: Exists for every label in tenancyLabels

[npinaeva]

We can do that, but I see 2 possible disadvantages to this:

• namespaceSelector doesn't fully define which namespaces will be selected, so we add an implicit label selector key: X, operator: Exists and it makes the API more confusing
• there is no way to set rules for namespaces with label unset: e.g. if I want to deny-from-not-same-tenant, I may want to deny from not only namespaces with different label value, but also from namespaces without a label, otherwise forgetting to set a label on a namespace may become a security hole, because it won't be affected by tenancy.

[astoycos]

IMO I would like to merge if we agree on the "goals section" the actual API implementation should be hashed out in another PR to this NPEP.

[danwinship]

We could alternatively say that namespaces without all of the tenancyLabels are always excluded from the definition. IOW, namespaceSelector automatically implicitly includes key: X, operator: Exists for every label in tenancyLabels

[astoycos]

Looking good! I really like the way you explicitly laid out the issues with the existing implementation. I'm planning on lgtm(ing) this KEP without focusing too hard on the actual "API" section as I Think that should be completed in another PR. Let's try to build consensus around the updated user stories and various issues you pointed out in the existing implementation first.

[astoycos]

IMO I would like to merge if we agree on the "goals section" the actual API implementation should be hashed out in another PR to this NPEP.

[npinaeva]

As discussed in the upstream meeting, this version is focused on user stories now, which led me to writing down even more user stories, and also covering existing API fields that I couldn't figure out user stories for.
Since the main problem of the existing API is covering much more configs that we actually have user stories for, we probably shouldn't include fields that we don't have user stories for in the next Tenancy API.

[rahulkjoshi]

While this is true, I'm not sure this is a huge problem unique to tenancy. I could make a parallel argument for something like egress rules: There's no way to find the ANP(s) that define egress if you don't already know what they are.

I would argue that this is a problem easily solved by a good naming scheme (e.g. call my ANP "tenant-policy" or "namespace-isolation"). Easy discoverability of a particular type of rule shouldn't be a top-priority for our design, in my opinion.

[npinaeva]

Very valid point, I don't think this is the major problem, but more of an introduction to the following points, just stressing that tenants are different from the other peer types. As you said, if you want to define a tenant, you will probably create a separate ANP with a single tenancy rule and a tenancy-specific name, but that is not dictated by the API, and tenancy may easily be lost in a list of other peers.

[astoycos]

Hrm this one seems a bit duplicative to the "create and isolate" stories to me...

More specifically...

At the same time I want to setup an overridable deny-all policy to protect namespaces by default.
This policy should make sure internal connectivity for a tenant is always allowed, in case there are
lower-priority deny rules.

In this case (thinking in terms on ANP) you'd use an ANP to setup the tenants... and that ANP would implicity be a "higher-order" policy meaning even with an "overridable deny-all policy" traffic within tenants would always be allowed unless explicitly denied with another ANP or "passed" to namespace owners

[npinaeva]

It is very similar indeed, but the difference is in the action: "Isolate" user story is focused on the Deny tenancy rules, and "Allow internal connections for tenants" is more about Allow tenancy rules. Maybe they are too similar, lmk if you think I should remove this one

[astoycos]

"Isolate" user story is focused on the Deny tenancy rules, and "Allow internal connections for tenants" is more about Allow tenancy rules.

IMO this is somewhat implicit in:

As a cluster admin, I want to build tenants in my cluster that are isolated from each other where this isolation
    can't be overridden by namespace owners.

i.e we're building hard tenants where the definition is tenants can't talk to each other but members of a tenant are of one group, and can communicate.

If you want to augment 4.2 to explicitly mention something like that I think that'd be alright, but I think Dan and I agree here so I'd remove 4.3

   As a cluster admin, I want to build tenants in my cluster that are isolated from each other where this isolation
    can't be overridden by namespace owners. This policy should make every tenant completely independent and isolated 
    from another tenants, while explicitly allowing same tenant members to communicate.

[npinaeva]

let me try to explain this once again, because I don't think we are talking about the same use case

4.2 story just denies inter-tenant communication, but doesn't say anything about intra-tenant communication, e.g. you could write "deny from the other tenants" ANP, and at the same time define a NP in namespace A saying "deny from namespace B", where namespaces A and B are in the same tenant. That would limit intra-tenant communication, and ANP doesn't say anything about that, so traffic from namespace B will be dropped.

4.3 use case is aimed to prevent namespaces in the same tenant from denying each others connections (e.g. by applying any network policy and by mistake not including your tenant namespaces to the allow list). In that case even if same namespace A applies a "deny from ALL" network policy, namespace B will still be allowed by ANP.

I just want to make sure I've explained the use cases well enough, and then we can surely remove any extra parts or whole user stories.

[danwinship]

4.3 use case is aimed to prevent namespaces in the same tenant from denying each others connections

Is that a real use case? Why would an administrator want to enforce that for every tenant at the cluster level?

4.1 is "Engineering vs Marketing" and 4.2 is "Coke vs Pepsi". What is the explanation for this user story?

(Also, you should probably actually put "Engineering vs Marketing" and "Coke vs Pepsi" into the actual user stories. The user story should stand alone to explain the use case. We shouldn't have the user story and also have a separate "here's what this really means". And in this case (4.3), I don't think there is any "here's what this really means".)

[npinaeva]

I think 4.3 is the Engineering vs Engineering, meaning that after you defined Engineering vs Marketing tenants, you still may want to strictly define intra-tenant rules. e.g. you may have a strong policy where every engineering team should be always able to talk to all the other engineering teams. That prevents one engineering team from building its own room and closing the door so that other engineers can't talk to them anymore (also potentially by mistake when defining some deny-all rule).

I am not sure how to say if this is a more or less real use case compared to the previous ones.

[shaneutt]

/cc

[danwinship]

Suggested change

	As a cluster admin, I want to build tenants in my cluster that are isolated from each other and this can't
	be overridden. This policy should make every tenant completely independent and isolated from another tenants.
	As a cluster admin, I want to build tenants in my cluster that are isolated from each other where this
	isolation can't be overridden by the tenants themselves. This policy should make every tenant completely independent and isolated from another tenants.

(because the admin could still override the isolation with ANPs)

alternatively "where the isolation can't be overridden by NetworkPolicy". Actually, that might be a good way to describe both the previous case and this one; the previous case is where the tenant isolation can be overridden by NetworkPolicy and this case is where it can't be.

[npinaeva]

good point! I made it "namespace owners" just based on the first user story wording, lmk if you prefer some other name for that actor :)

[danwinship]

Agree with Andrew; I'm not exactly sure what this one is saying. It sounds like you're saying the admin wants to forbid users from using NP within a tenant

[danwinship]

Yeah, I don't think there's a good argument for using ports in tenant definitions.

[danwinship]

I think we want to make this a Non-Goal. The existing KEP allows for having lots of rules with different SameLabels/NotSameLabels doing different things, but the user stories all involve having a single cluster-wide definition of "tenancy".

[npinaeva]

Not sure if I got that right, but I added "define user stories for multiple tenancy policies in the same cluster" as a non-goal

[astoycos]

/lgtm
/hold

This is ready to go from my side (minus these small nit's and comments) I added /lgtm and I'll let either @danwinship or @Dyanngg Finish this with and approval + removal of the hold. Thanks @npinaeva

[astoycos]

"Isolate" user story is focused on the Deny tenancy rules, and "Allow internal connections for tenants" is more about Allow tenancy rules.

IMO this is somewhat implicit in:

As a cluster admin, I want to build tenants in my cluster that are isolated from each other where this isolation
    can't be overridden by namespace owners.

i.e we're building hard tenants where the definition is tenants can't talk to each other but members of a tenant are of one group, and can communicate.

If you want to augment 4.2 to explicitly mention something like that I think that'd be alright, but I think Dan and I agree here so I'd remove 4.3

   As a cluster admin, I want to build tenants in my cluster that are isolated from each other where this isolation
    can't be overridden by namespace owners. This policy should make every tenant completely independent and isolated 
    from another tenants, while explicitly allowing same tenant members to communicate.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: astoycos, npinaeva

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 [astoycos]

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

[danwinship]

4.3 use case is aimed to prevent namespaces in the same tenant from denying each others connections

Is that a real use case? Why would an administrator want to enforce that for every tenant at the cluster level?

4.1 is "Engineering vs Marketing" and 4.2 is "Coke vs Pepsi". What is the explanation for this user story?

(Also, you should probably actually put "Engineering vs Marketing" and "Coke vs Pepsi" into the actual user stories. The user story should stand alone to explain the use case. We shouldn't have the user story and also have a separate "here's what this really means". And in this case (4.3), I don't think there is any "here's what this really means".)

[danwinship]

/lgtm

[danwinship]

/hold cancel

[astoycos]

/lgtm

🍾