Service Internal Traffic Policy: use more generic API #2166
Conversation
The current choice of `Cluster` will likely cause some confusion for implementations providing multicluster Service routing. For example, Cilium and Istio both transparently route to all endpoints across all clusters (see https://cilium.io/blog/2019/03/12/clustermesh#service-discovery). This makes the statement that `Route to all cluster-wide endpoints (or use topology aware subsetting if enabled)` no longer correct for these implementations. As a counter example, I believe Linkerd's model technically is compatible, as it is actually copying Endpoints over into the cluster. However, it may still confuse users as despite the Endpoint being in the cluster, the destination of the Endpoint is not, and the fact the Endpoint is in the cluster is an implementation detail. Another counter example is MCSD, where the Service is still "cluster", and a separate ServiceExport is used to define the supercluster. It is also possible this could instead be extended to have 3 options: Supercluster, Cluster, and Local, effectively replacing ServiceExport. So this PR changes the naming a bit to allow for both models to exist gracefully
k8s-ci-robot
added
cncf-cla: yes
|
Welcome @howardjohn! |
|
Hi @howardjohn. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: howardjohn The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
kind/kep
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
Also for multicluster perspective: |
|
@andrewsykim for it. Seems reasonable |
There was a problem hiding this comment.
I'm fine with All but I don't love how it's inconsistent with externalTrafficPolicy that uses Cluster. Should we support "All" for both or is it not worth it cause we can't change the default for externalTrafficPolicy anyways? cc @maplain who has started on the implementation.
|
We could support "All" as an alias for "Cluster" in external, I suppose.
We'd have to alpha it like any other field change.
…On Mon, Nov 30, 2020 at 11:12 AM Andrew Sy Kim ***@***.***> wrote:
***@***.**** commented on this pull request.
I'm fine with All but I don't love how it's inconsistent with
externalTrafficPolicy that uses Cluster. Should we support "All" for both
or is it not worth it cause we can't change the default for
externalTrafficPolicy anyways? cc @maplain <https://github.com/maplain>
who has started on the implementation.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#2166 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKWAVCYQSCQSLEO5PEKV6DSSPVC3ANCNFSM4UC74EAA>
.
|
Sounds good to me, we can discuss this further as a follow-up or during the PR review. I would also like to consider if it makes sense to support /lgtm |
k8s-ci-robot
added
the
lgtm
|
Yeah, the SNAT is why "prefer local" was never a thing - the driving reason
for `eTP` was "preserve source IP". That said, I am not against it *if* we
can show its value.
…On Sun, Dec 6, 2020 at 6:47 AM Andrew Sy Kim ***@***.***> wrote:
We could support "All" as an alias for "Cluster" in external, I suppose.
We'd have to alpha it like any other field change.
Sounds good to me, we can discuss this further as a follow-up or during
the PR review. I would also like to consider if it makes sense to support
PreferLocal for externalTrafficPolicy as well. Likely more difficult than
internal because of SNAT semantics.
/lgtm
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#2166 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKWAVBBIWJWPHW7VRDKUPLSTOKPDANCNFSM4UC74EAA>
.
|
|
Hi there, 1.21 Enhancements Lead here. Since this PR is open, I wanted to mention it in case you wanted to include it here. Please make sure that all requirements are met before the upcoming enhancements freeze Feb 9th. For PRR review, please reach out to #prod-readiness slack channel to get it reviewed asap. thanks! |
|
What is needed for this to be merged? |
|
/cc @maplain |
|
@robscott: GitHub didn't allow me to request PR reviews from the following users: maplain. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
|
Thinking about this today. The problem with "All" as an alias is that most people who want "All" are not going to specify anything, and we can't really change the default. So let's not do that. We can either merge this (and change/followup #96600) to "All" which is still not exactly correct once topology lands or leave it at "Cluster" and document that some implementations span clusters. |
|
@howardjohn: PR needs rebase. 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. |
k8s-ci-robot
added
the
needs-rebase
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-contributor-experience at kubernetes/community. |
k8s-ci-robot
added
the
lifecycle/stale
7 participants
The current choice of
Clusterwill likely cause some confusion for implementations providing multicluster Service routing.For example, Cilium and Istio both transparently route to all endpoints across all clusters (see https://cilium.io/blog/2019/03/12/clustermesh#service-discovery). This makes the statement that
Route to all cluster-wide endpoints (or use topology aware subsetting if enabled)no longer correct for these implementations.As a counter example, I believe Linkerd's model technically is compatible, as it is actually copying Endpoints over into the cluster. However, it may still confuse users as despite the Endpoint being in the cluster, the destination of the Endpoint is not, and the fact the Endpoint is in the cluster is an implementation detail.
Another counter example is MCSD, where the Service is still "cluster", and a separate ServiceExport is used to define the supercluster.
It is also possible this could instead be extended to have 3 options: Supercluster, Cluster, and Local, effectively replacing ServiceExport.
So this PR changes the naming a bit to allow for both models to exist gracefully
cc @robscott @JeremyOT @nmittler