-
Notifications
You must be signed in to change notification settings - Fork 353
docs(MADR): mesh-scoped zone proxies representation and address syncing #15566
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Reviewer Checklist🔍 Each of these sections need to be checked by the reviewer of the PR 🔍:
|
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
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.
Pull request overview
Adds a new MADR documenting the proposed representation of Zone Ingress/Zone Egress as mesh-scoped Dataplane resources and how their advertised addresses should be shared across zones.
Changes:
- Introduces MADR 095 describing a mesh-scoped schema for zone proxies using
Dataplaneextensions. - Documents policy targeting considerations for zone proxy dataplanes (including
targetRef.sectionNameusage). - Proposes address/port syncing via
MeshService.spec(zone ingress) andMeshExternalService.status(zone egress).
Co-authored-by: Copilot <[email protected]> Signed-off-by: Ilya Lobkov <[email protected]>
lahabana
left a comment
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.
There are really at least 3 MADRs here. We should consider splitting things.
I'm fine with using DP but we need to think a bit more about what it means to have multiple Ingress or Egress deployments.
I'm still not convinced at all with having egress and ingress addresses in MES/MS it just feels like "we didn't know what to do with this info so we put it there..." I'm sure we can do better!
|
|
||
| ##### Open Questions | ||
|
|
||
| 1. Should `networking.zoneIngress` and `networking.zoneEgress` be an array? |
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?
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.
We discussed this offline yesterday. It should never be an array. I missed that this question was still in the doc.
| 1. Should `networking.zoneIngress` and `networking.zoneEgress` be an array? | ||
| 2. There is also `networking.advertisedAddress` field (but no `networking.advertisedPort`). | ||
| It was a very niche Universal-only field [contributed by community](https://github.com/kumahq/kuma/pull/2116) that I think we should remove in v3. | ||
| I think `networking.zoneIngress.advertisedAddress` should be `required` so it never falls back to `networking.advertisedAddress`. |
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.
Sounds good. So in a more concrete manner we disallow using networking.advertisedAddress always in 2.14? Or do we warn in 2.14 and remove later?
| 2. There is also `networking.advertisedAddress` field (but no `networking.advertisedPort`). | ||
| It was a very niche Universal-only field [contributed by community](https://github.com/kumahq/kuma/pull/2116) that I think we should remove in v3. | ||
| I think `networking.zoneIngress.advertisedAddress` should be `required` so it never falls back to `networking.advertisedAddress`. | ||
|
|
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 be at the same time:
ZoneEgress, ZoneIngress, Regular DP?
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.
How do we filter in the API to get just the Ingress? Just the Egress?
Does that mean you can have multiple Egress/Ingress?
| However, on Universal we can have both sidecar and zone proxy listeners on the same DPP since transparent proxying is not required. | ||
| But the question is do we want to allow that? It might be confusing for users and doesn't allow us having schema level validation. | ||
|
|
||
| ### Policy Targeting for Zone Ingress and Zone Egress |
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 should be its own MADR IMO
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.
There will be a separate MADR for policies. Here, I only want to cover how the top-level targetRef will work with the extended Dataplane resource. Since this MADR focuses on the new Dataplane API, I think this part belongs here.
| Dataplanes that represent zone proxies don't need `transparentProxying` even on Kubernetes. | ||
| This means on Kubernetes we can't mix `inbounds` and zone proxy listeners on the same DPP. | ||
| However, on Universal we can have both sidecar and zone proxy listeners on the same DPP since transparent proxying is not required. | ||
| But the question is do we want to allow that? It might be confusing for users and doesn't allow us having schema level validation. |
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 let's not allow it but we can at least be both Ingress and Egress.
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.
TODO: think if we can make less exceptions if nothing wrong with inbound and zoneIngress/zoneEgress listeners working together then we probably shouldn't limit it
| value: "spiffe://default/" | ||
| ``` | ||
|
|
||
| Field `targetRef.sectionName` can be used to select only zone egress or zone ingress listener. |
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 you show an actual example here?
| New `zoneEgress` and `zoneIngress` sections have `name` field and can be selected with `spec.targetRef.sectionName`. | ||
| There are no reasons to keep `kuma.io/proxy-type` label and it should be removed in v3, see [#15567](https://github.com/kumahq/kuma/issues/15567). | ||
|
|
||
| ### Syncing Zone Ingress Addresses via MeshService |
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.
Same other MADR
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 sure. This MADR explains how we’re going to convert the globally scoped ZoneIngress resource, which is synced everywhere, into a mesh-scoped Dataplane resource that is synced only to Global. IMO answering the syncing question here makes the design more complete. The entire doc is still only about 200 lines.
| port: 16739 | ||
| targetPort: target-port-from-container | ||
| appProtocol: tcp | ||
| zoneIngress: # different name? maybe 'multizone'? |
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.
We need a better name.
Also I feel like we should keep the same field names and on Ingress (advertised).
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.
todo: single MeshZoneAddress resource per zone?
spec:
address: 192.168.0.1
port: 10001
status:
origin: # k8s service ???todo: write why can't we use zone object?
| zoneIngress: | ||
| address: 10.0.0.1 # required, address listener binds to | ||
| port: 10001 # required, port listener binds to | ||
| advertisedAddress: 192.168.1.100 # required, address visible 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.
There's something wrong about this because advertisedAddress is common to all ZoneIngress...
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’s the same today, right? Multiple ZoneIngress resources in the same zone can have the same advertisedAddress when they're behind the LB
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 fact that it is like this today doesn't mean it's right :)
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.
TODO:
There are 2 flows possible:
- meshservice_controller.go watches
Servicewhich is labels as "ingress" and reconciles MS (or . In that case we don't need theadvertisedAddressin DPP - pod_controller.go reconciles
We'd like to get rid of the advertisedAddress in the DPP.
Motivation
We want to replace global-scoped ZoneIngress/ZoneEgress with mesh-scoped Dataplane (with new fields for zone proxy specific info).
Supporting documentation
Fix https://github.com/Kong/kong-mesh/issues/9028