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,336 @@ 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`. To only select namespaces with existing labels extra `key: X, operator: Exists` condition should
+    be added to the selector.
+
+```yaml
+kind: NetworkTenancy
+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**: applying all labels from `tenancyLabels` to a namespaces automatically makes it affected by tenancy.
+    No way to make sure ALL namespaces in a cluster are a part of some tenant, e.g. newly created namespace will not
+    be limited/protected by Tenancy rules until it gets `tenancyLabels` assigned, may potentially be a security problem.
+    
+    Example: by setting deny-from-other-tenants I want to ensure only namespaces with the same label `security-zone` value
+    can send incoming connections. That policy will be violated if a new tenant is created without `security-zone` label,
+    meaning it is not a part of any Tenant. This may potentially be solved by using allow-from-same-tenant + deny ALL
+    ANP, but it is stricter, since it affects all incoming connections, not just from other pods.
+
+```yaml
+kind: NetworkTenancy
+spec:
+  tenancyLabels:
+    - user
+```
+
+### Peers and actions
+
+Based on the existing User Stories, Tenancy only needs Allow and Deny actions, and the only 2 types of peers are
+`SameTenant` and `NotSameTenant`.
+
+Therefore, Tenancy rules will look something like:
+
+```yaml
+kind: NetworkTenancy
+spec:
+  tenancyLabels: ['label1']
+  ingress:
+    - actions: Deny
+      from: NotSameTenant
+    - actions: Allow
+      from: SameTenant
+  egress:
+    - actions: Deny
+      to: NotSameTenant
+```
+
+### 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, link Tenancy API
+
+```yaml
+kind: AdminNetworkPolicy
+spec:
+  priority: 10
+  # EITHER
+  subject: <...>
+  ingress: <...>
+  egress: <...>
+  # OR
+  tenant: <tenant reference>
+```
+
+```yaml
+kind: BaselineAdminNetworkPolicy
+spec:
+  # EITHER
+  subject: <...>
+  ingress: <...>
+  egress: <...>
+  # OR
+  tenant: <tenant reference>
+```
+
+We may want to allow either `subject`+`ingress`/`egress` or `tenant`, since having both at the same priority
+is confusing (`tenancyLabels` and `subject` may select unrelated sets of pods) and may result in conflicting rules.
+
+2. Create 2 objects with ANP and BANP priorities (let's say Tenancy and BaselineTenancy)
+
+```yaml
+kind: NetworkTenancy
+spec:
+  priority: 10
+  ingress:
+    - actions: Deny
+      from: NotSameTenant
+    - actions: Allow
+      from: SameTenant
+  egress:
+    - actions: Deny
+      to: NotSameTenant
+```
+
+```yaml
+kind: BaselineNetworkTenancy
+spec:
+  ingress:
+    - actions: Deny
+      from: NotSameTenant
+    - actions: Allow
+      from: SameTenant
+  egress:
+    - actions: Deny
+      to: NotSameTenant
+```
+
+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.
+
+3. Create 1 object with extra `baselinePolicy` field or IntOrString value for priority.
+
+```yaml
+kind: NetworkTenancy
+spec:
+  baselinePolicy: false
+  priority: 10
+  ingress:
+    - actions: Deny
+      from: NotSameTenant
+    - actions: Allow
+      from: SameTenant
+  egress:
+    - actions: Deny
+      to: NotSameTenant
+```
+
+```yaml
+kind: NetworkTenancy
+spec:
+  priority: baseline
+  ingress:
+    - actions: Deny
+      from: NotSameTenant
+    - actions: Allow
+      from: SameTenant
+  egress:
+    - actions: Deny
+      to: NotSameTenant
+```
+
+### Another approach
+
+There are 4 (6 if we consider Skip as an action, which I will do for now, but we can remove it later if we consider it
+unnecessary) option for tenancy rules: `SameTenant: Allow/Skip/Deny`, `NotSameTenant: Allow/Skip/Deny`. 
+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` with 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.
+
+In this case, we only need to define tenancy for the same tenant (which seems to be easier to understand).
+Tenancy itself only needs a set of `tenancyLabels` and one of the `Allow` or `Skip` actions in every direction.
+
+If we consider tenancy to be always the highest-priority rule on ANP or BANP level, we could add one more field
+`precedence: ANP/BANP` to define at which precedence it should be evaluated (for BANP `Skip` and `Allow` actions
+actually mean the same, we can only allow value for it)
+
+**Note** we could also have `Allow/Skip/DefaultAllow` actions without `precedence` filed, but I am still not sure if
+I like implying rule priority from the action.
+
+we can express existing user stories as follows:
+
+* 4.1 "overridable isolation"
+```yaml
+kind: NetworkTenancy
+spec:
+  tenancyLabels:
+    - "user"
+  precedence: BANP
+  ingress: Allow
+  egress: Allow
+---
+kind: BaselineAdminNetworkPolicy
+spec:
+  subject:
+    namespaces:
+      matchExpression:
+        - key: user
+          operator: Exists
+  ingress:
+    - action: Deny
+      from:
+        - namespaces: {}
+  egress:
+    - action: Deny
+      to:
+        - namespaces: {}
+```
+
+* 4.2 strict isolation
+```yaml
+kind: NetworkTenancy
+spec:
+  tenancyLabels:
+    - "user"
+  precedence: ANP
+  ingress: Skip
+  egress: 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
+  ingress: Allow
+  egress: 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
+  ingress: Skip
+  egress: 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
+  ingress: Allow
+---
+kind: BaselineAdminNetworkPolicy
+spec:
+  subject:
+    namespaces:
+      matchExpression:
+        - key: user
+          operator: Exists
+  ingress:
+    - action: Deny
+      from:
+        - namespaces: {}
+```
+
+This simplifies the tenancy to only define sameTenant rules, and you basically only have 3 options for every direction:
+strictly allow connections inside one tenant; delegate network policy inside one tenant to namespaces; .
+And the new object will be 
+
+```yaml
+kind: NetworkTenancy
+spec:
+  tenancyLabels:
+    - "user"
+  precedence: ANP/BANP
+  ingress: Allow/Skip
+  egress: Allow/Skip
+```
 
 ## Conformance Details