Add Tenancy API design suggestions.

Signed-off-by: Nadia Pinaeva <[email protected]>

Author: npinaeva

npeps/npep-122.md

@@ -1,7 +1,7 @@
 # NPEP-122: Tenancy API
 
 * Issue: [#122](https://github.com/kubernetes-sigs/network-policy-api/issues/122)
-* Status: Provisional
+* Status: Implementable
 
 ## TLDR
 
@@ -158,7 +158,344 @@ Fourth, the ANP subject allows using pod selectors, while tenancy use cases only
 
 ## API
 
-TBD
+### Tenant definition
+
+For the purposes of this NPEP we define a Tenant as a set of namespaces.
+`tenancyLabels` is a set of label keys, based on which all Namespaces affected by Tenancy are be split into Tenants.
+A Tenant is identified by values of `tenancyLabels`, which are shared by all namespaces in a given Tenant.
+
+There are 2 ways to select which namespaces should be affected by Tenancy rules:
+
+1. Use a distinct new `namespaceSelector` field to define which namespaces should be affected by Tenancy, 
+while the `tenancyLabels` field can be used to define how the selected namespaces are split into Tenants.
+
+    **Cons**: selected namespace may not have some of the `tenancyLabels`, which will likely result in introducing "None" value
+    for `tenancyLabels`. It brings more complexity and is not required for any of the listed use cases.
+
+```yaml
+spec:
+  matchExpression:
+    - key: system-namespace
+      operator: DoesNotExist
+    - key: user
+      operator: Exists
+  tenancyLabels:
+    - user
+```
+
+2. Overload `tenancyLabels` and implicitly apply tenancy rules only to namespaces where `tenancyLabels` are present.
+
+    **Cons**: the simplest option that covers all given use cases.
+
+```yaml
+spec:
+  tenancyLabels:
+    - user
+```
+
+### Peers and actions
+
+Based on the existing User Stories, Tenancy only needs Allow and Deny actions (Skip is still discussed), and the only 2 types of peers are
+`SameTenant` and `NotSameTenant`.
+
+From the user stories (and common sense) looks like the only useful combinations are `SameTenant: Allow/Skip` and `NotSameTenant: Deny`.
+We can always replace `NotSameTenant: Deny` with Tenancy using `SameTenant: Allow/Skip` and lower priority
+and (B)ANP `Deny` policy for the same tenancy selector (or maybe even wider selector, e.g. all user pods).
+This helps focus on which policy is defined inside one tenant, and then "everything else"/"other tenants"/etc
+may be defined with existing CRDs.
+
+Whether `NotSameTenant` action makes API any better/easier to understand is still to be discussed.
+**PROS** of using only `SameTenant`
+- no need to specify egress and ingress
+- flexibility with Deny rules provided by ANP/BANP (e.g. unifying deny not same tenant with deny all in the same rule)
+
+Ingress and Egress don't need to be separated is we only define `SameTenant`, as subject and peer are the same set of pods (sameTenant pods).
+
+Therefore, Tenancy rules may look something like:
+
+```yaml
+spec:
+  actions: Allow
+  peer: SameTenant
+```
+
+### Priorities
+
+Based on User Story 4.4, we need to have Tenancy in the same priority range as ANP and BANP. There are multiple ways to do so:
+
+1. Reuse ANP and BANP to define priority, insert Tenancy rules to `ingress` and `egress`.
+Current (B)ANP semantics is: `subject` selects pod to which all `ingress` and `egress` rules are applied.
+Tenancy can't be used in this semantics, because it has its own "subject" defined by the tenancy labels.
+There are multiple ways to "turn on" tenancy mode in B(ANP) and allow tenancy rules.
+
+Exclusive fields `subject`/`tenancySubject`
+```yaml
+kind: AdminNetworkPolicy
+spec:
+  # this field turns on tenancy
+  tenancySubject:
+    labels: ["user"]
+  ingress:
+    - action: Allow
+      from:
+        - SameTenant
+        - pods:
+            podSelector: {}
+```
+
+A similar, but slightly different option with pushing `tenantNamespace` as an alternative subject selector to existing 
+`pods` and `namespaces`
+```yaml
+kind: AdminNetworkPolicy
+spec:
+  # this field turns on tenancy
+  subject:
+    tenantNamespaces:
+      labels: ["user"]
+  ingress:
+    - action: Allow
+      from:
+        - SameTenant
+        - pods:
+            podSelector: {}
+```
+**CONS** 
+- "switch" that enables some peer types is not the best API design
+- gives more flexibility than intended (multiple tenancy definitions)
+- conflicts with singleton BANP, meaning that if Tenancy is defined on BANP level, general-purpose BANP selecting different
+pods can't be created.
+
+2. Reuse ANP and BANP to define priority, insert Tenancy definition and Rules to `ingress` and `egress`.
+
+It splits the tenancy labels and namespace selector which defines the namespace that are affected by the tenancy.
+
+```yaml
+kind: AdminNetworkPolicy
+spec:
+  priority: 10
+  subject:
+    namespaces:
+      matchLabels:
+        kubernetes.io/metadata.name: sensitive-ns
+  ingress:
+    - action: Allow
+      from:
+      - tenant:
+          labels: ["user"]
+          tenant: Same
+```
+
+**CONS** complicates selection or affected namespaces, allows strange configuration, where namespaces selected by 
+`subject` have no tenancy labels. Has the same problem as the original design.
+
+3. Create 2 objects with ANP and BANP priorities (let's say NetworkTenancy and BaselineNetworkTenancy)
+
+```yaml
+kind: NetworkTenancy
+spec:
+  priority: 10
+  tenancyLabels: ["user"]
+  actions: Allow
+  peer: SameTenant
+---
+kind: BaselineNetworkTenancy
+spec:
+  priority: 10
+  tenancyLabels: ["user"]
+  actions: Allow
+  peer: SameTenant
+```
+
+While multiple ANPs with the same priority are allowed, we probably can allow multiple Tenancies or Tenancy and ANP
+with the same priority, but if we decide to only allow ANP per priority, Tenancy needs to be accounted for in the same range.
+
+**CONS**: BANP doesn't have a priority, to use this method we would need to define a priority for BANP.
+
+4. Create 1 new object with implicit priorities.
+
+`precedence` field + reserved highest-priority rule before (B)ANP
+Similar to the previous one, but a bit more flexible:
+```yaml
+kind: NetworkTenancy
+spec:
+  tenancyLabels: ["user"]
+  precedence: ANP/BANP
+  action: Allow/Skip
+```
+**CONS** Priorities are implicit and need to be added as extra layers between ANP/NP/BANP.
+**PROS** 
+- No changes to the existing ANP/BANP objects
+- Limited to the use cases we designed it for (smaller chance to shoot yourself in a foot)
+- Users that don't care about tenancy can just ignore this CRD
+- We can throw it away if we want to change API again
+
+
+#### Final suggestion
+
+Considering we agree with option 4 from the previous section on priorities specification, we can outline further details here.
+
+For
+```yaml
+kind: NetworkTenancy
+spec:
+  tenancyLabels: ["user"]
+  precedence: ANP/BANP
+  action: Allow/Skip
+```
+there are only 3 useful combinations of precedence+action (BANP+Skip is the same as BANP+Allow), so we can express these
+options in a different way, e.g. define 3 modes
+- always-allow (ANP+Allow)
+- delegate-to-NP (ANP+Skip)
+- baseline-allow (BANP+Allow)
+
+<details>
+<summary>Full yaml examples (with the initial fields, will be updates as we agree on the final CRD format)</summary>
+
+* 4.1 "overridable isolation"
+```yaml
+kind: NetworkTenancy
+spec:
+  tenancyLabels:
+    - "user"
+  precedence: BANP
+  action: Allow
+---
+kind: BaselineAdminNetworkPolicy
+spec:
+  subject:
+    namespaces:
+      matchExpression:
+        - key: user
+          operator: Exists
+  ingress:
+    - action: Deny
+      from:
+        - namespaces: {}
+  egress:
+    - action: Deny
+      to:
+        - namespaces: {}
+```
+BANP can also be replaced with deny-all BANP
+```yaml
+kind: BaselineAdminNetworkPolicy
+spec:
+  subject:
+    namespaces: {}
+  ingress:
+    - action: Deny
+      from:
+        - namespaces: {}
+  egress:
+    - action: Deny
+      to:
+        - namespaces: {}
+```
+
+* 4.2 strict isolation
+```yaml
+kind: NetworkTenancy
+spec:
+  tenancyLabels:
+    - "user"
+  precedence: ANP
+  action: Skip
+---
+kind: AdminNetworkPolicy
+spec:
+  priority: 1
+  subject:
+    namespaces:
+      matchExpression:
+        - key: user
+          operator: Exists
+  ingress:
+    - action: Deny
+      from:
+        - namespaces: {}
+  egress:
+    - action: Deny
+      to:
+        - namespaces: {}
+```
+
+* 4.3 Allow internal connections for tenants
+```yaml
+kind: NetworkTenancy
+spec:
+  tenancyLabels:
+    - "user"
+  precedence: ANP
+  action: Allow
+---
+kind: BaselineAdminNetworkPolicy
+spec:
+  subject:
+    namespaces:
+      matchExpression:
+        - key: user
+          operator: Exists
+  ingress:
+    - action: Deny
+      from:
+        - namespaces: {}
+  egress:
+    - action: Deny
+      to:
+        - namespaces: {}
+```
+
+* 4.4 Tenants interaction with (B)ANP
+* 4.4.1 allow from monitoring + deny from not same tenant
+```yaml
+kind: NetworkTenancy
+spec:
+  tenancyLabels:
+    - "user"
+  precedence: ANP
+  action: Skip
+---
+kind: AdminNetworkPolicy
+spec:
+  priority: 1
+  subject:
+    namespaces:
+      matchExpression:
+        - key: user
+          operator: Exists
+  ingress:
+    - action: Allow
+      from:
+        - namespaces:
+            namespaceSelector:
+              matchLabels:
+                kubernetes.io/metadata.name: monitoring-ns
+    - action: Deny
+      from:
+        - namespaces: {}
+```
+* 4.4.2 allow from same tenant + BANP deny all
+```yaml
+kind: NetworkTenancy
+spec:
+  tenancyLabels:
+    - "user"
+  precedence: ANP
+  action: Allow
+---
+kind: BaselineAdminNetworkPolicy
+spec:
+  subject:
+    namespaces:
+      matchExpression:
+        - key: user
+          operator: Exists
+  ingress:
+    - action: Deny
+      from:
+        - namespaces: {}
+```
+</details>
 
 ## Conformance Details