New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs(MADR): policy on namespace #10148
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Marcin Skalski <skalskimarcin33@gmail.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Co-authored-by: Charly Molter <charly.molter@konghq.com> Signed-off-by: Ilya Lobkov <lobkovilya@yandex.ru>
Signed-off-by: Marcin Skalski <skalskimarcin33@gmail.com>
Signed-off-by: Marcin Skalski <skalskimarcin33@gmail.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
6b7bf7a
to
db02685
Compare
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This MADR is very hard to follow for the following reasons:
- It should probably start by introducing Consumer and Producer policies which I think are paramount to understand what this is about.
- It starts by explaining the implementation which is confusing as you haven't explained what you want to do.
|
||
Applying a policy on the custom namespace **affects requests to the pods in the custom namespace**. | ||
Since some policies are applied on the client-side, the scope of policy's effect is not limited to the namespace | ||
where it was applied. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 2 sentences contradict each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? The first sentence says policy in the custom namespaces affects requests to the custom namespace (not pods in the custom namespace). And the second sentence just elaborates – since some policies are applied on the client-side the policy scope is no longer limited to policy's namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I didn't see requests. I had to reread this 3 times, I think it could be made more clear
At this moment, it works only between MeshTimeout and MeshHTTPRoute, but there are plans to support it for other | ||
policies, i.e. [#6645](https://github.com/kumahq/kuma/issues/6645). | ||
Referencing a policy from another namespace or another zone should be done in a similar way MeshService is referenced. | ||
Fields `name/namespace` and `labels` are mutually exclusive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this: #10247 suggests an alternative more straightforward approach in this case that may simplify a lot of things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While thinking about: #10247 and thinking about syncing to zones with producer/consumer policies it struck me that you could have multiple to
entries with different kinds (one could be producer, the other consumer). What would be the syncing behaviour in this case?
The new validation rules are going to limit what policies you can create. If top-level targetRef doesn't specify metadata:
namespace: frontend-ns
labels:
kuma.io/origin: zone
spec:
targetRef:
kind: MeshSubset
tags:
kuma.io/zone: i-have-to-know-the-name-of-my-zone
k8s.kuma.io/namespace: frontend-ns
to:
- targetRef:
kind: Mesh
default: {} |
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
### Namespace-scoped policies and multizone | ||
|
||
Namespace-scoped policies can be applied to a custom namespace only on zones as there are simply no DPPs and custom | ||
namespaces on Global. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be enforced by validation webhook
After a user applied a policy on zone's custom namespace, the webhook automatically adds a `k8s.kuma.io/namespace` label to the policy. | ||
The label is part of the hash suffix when policy is synced to global. | ||
|
||
As mentioned previously, a policy applied in producer namespace can affect envoy configs of consumers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the first ocurence of the work "producer" so it's not "mentioned previously"
With the increased blast radius of the policies comes great responsibility, | ||
we don't want small mistake in the policy to break traffic on the other side of the world. | ||
A few things could be done to protect users from impactful mistakes: | ||
* Enhanced validation. If policy is created in `backend-ns` of the `zone-1` and the top-level targetRef is `Mesh`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm why? I'm confused I think I need an example to understand this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The next section #### Enhanced validation
provides more details and also has some examples.
A few things could be done to protect users from impactful mistakes: | ||
* Enhanced validation. If policy is created in `backend-ns` of the `zone-1` and the top-level targetRef is `Mesh`, | ||
then `to[].targetRef` should have both `k8s.kuma.io/namespace: backend-ns` and `kuma.io/zone: zone-1`. | ||
* Ability to opt-in or opt-out from syncing the policy to other zones. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's necessary you can always use a top spec.tagerRef.kind: MeshSubset
to reduce blast radius no? That's what it is for no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably wasn't clear that these points:
* Enhanced validation...
* Ability to opt-in or opt-out...
are followed by the separate sections to elaborate on each point:
#### Enhanced validation
...
#### Considered options on opting-in(out) to cross-zone policy syncing
...
and the decision for opting-in(out) is exactly what you said – we can always use a top spec.targetRef.kind: MeshSubset
to reduce blast radius.
|
||
The validation rules for namespace-scoped policy are the following: | ||
* to-policy when placed into the custom namespace must have the namespace and zone either in the top-level targetRef or in each to-item | ||
* from-policy when placed into the custom namespace must always have the namespace and zone in the top-level targetRef |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems unnecessarily annoying. Again UX is important here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't from
policy on custom namespace having an "implicit MeshSubset" a better behaviour? Especially if we go for #10247 top level targetRef slowly disappears.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also with the current zone originated policies is similar to having an implicit MeshSubset
no?
they have to put the policy into `kuma-system` namespace by checking with Mesh Operator first. | ||
|
||
The validation rules for namespace-scoped policy are the following: | ||
* to-policy when placed into the custom namespace must have the namespace and zone either in the top-level targetRef or in each to-item |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Annoying!
Zone-originated policies in `kuma-system` won't be synced to other zones. | ||
This eliminated the problem of backwards compatibility. | ||
Also, with the introduction of namespace-originated policies, applying policies to `kuma-system` becomes an edge use case. | ||
The only reason to apply a policy on zone's `kuma-system` is Zone Cluster Operator stepping in to fix the undesired behaviour. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not only undesired
? Let's say 1 zone is very far away maybe the operator will add increased default timeouts.
When policies have the same zone, go to the next step. | ||
5. **[new step]** Policy from local namespace is more specific than a policy from another namespace. | ||
When namespaces are equal, go to the next step. | ||
6. The policy is more specific if it's name is lexicographically less than other policy's name ("aaa" < "bbb" so that " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just write pseudo code? This is hard to process
`MeshGateway` at this moment is not namespace-scoped. It can change in the future with | ||
the [MeshBuiltinGateway resource](https://github.com/kumahq/kuma/issues/10014). | ||
|
||
For policies in custom namespaces, we've decided that we would allow targeting `MeshGateway` in `MeshHTTPRoute` and `MeshTCPRoute`, and policies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
targeting where? to? top level?
backendRefs: | ||
- kind: MeshService | ||
name: frontend | ||
namespace: frontend-ns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This namespace can be implicit right?
Also, if this namespace were different from frontend-ns
would this be rejected?
to: | ||
- targetRef: | ||
kind: MeshService # you need to specify MeshService in to[].targetRef.kind | ||
name: frontend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is equivalent to having the implicit namespace: frontend-ns
?
What would happen if I had namespace: backend-ns
Finally what happens with:
type: MeshCircuitBreaker
mesh: default
name: circuit-breaker
namespace: frontend-ns
spec:
targetRef:
kind: MeshGateway
name: edge-gateway
to:
- targetRef:
kind: MeshService # you need to specify MeshService in to[].targetRef.kind
name: frontend
default: {}
- targetRef:
kind: MeshService
name: backend
namespace: backend-ns
default: {}
My gut feeling is that this should rejected (entries in the to
must have the same namespace). We should likely suggest users to not set multiple to
entries
metadata: | ||
namespace: backend-ns | ||
labels: | ||
kuma.io/origin: zone | ||
kuma.io/zone: producer-zone | ||
spec: | ||
targetRef: | ||
kind: MeshSubset | ||
tags: | ||
k8s.kuma.io/namespace: backend-ns | ||
to: | ||
- targetRef: | ||
kind: MeshService | ||
labels: | ||
kuma.io/display-name: redis | ||
kuma.io/zone: producer-zone | ||
--- | ||
metadata: | ||
namespace: backend-ns | ||
labels: | ||
kuma.io/origin: zone | ||
kuma.io/zone: producer-zone | ||
spec: | ||
targetRef: | ||
kind: MeshSubset | ||
tags: | ||
k8s.kuma.io/namespace: backend-ns | ||
kuma.io/zone: producer-zone | ||
to: | ||
- targetRef: | ||
kind: MeshService | ||
name: redis | ||
namespace: redis-ns | ||
- targetRef: | ||
kind: Mesh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just get confused with the example, it looks like a cross namespace control?
So, can we define our top TargetRef MeshSubset
with another NS front-end
if our policy is backend-ns
?
Actually, I feel it's pretty complicated in the cross-namespace use cases definition :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I just get understand with this.
Maybe we should also paste the link here: https://gateway-api.sigs.k8s.io/mesh/#gateway-api-for-service-mesh
For those people who are unfamiliar with the context
Checklist prior to review
syscall.Mkfifo
have equivalent implementation on the other OS --ci/
labels to run additional/fewer testsUPGRADE.md
? --