feat(gateway-api): add ability to override target on *Route resources#5319
feat(gateway-api): add ability to override target on *Route resources#5319davidspek wants to merge 6 commits intokubernetes-sigs:masterfrom
Conversation
|
Hi @davidspek. Thanks for your PR. I'm waiting for a kubernetes-sigs 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. DetailsInstructions 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-sigs/prow repository. |
|
/ok-to-test |
|
/retest |
docs/sources/gateway.md
Outdated
There was a problem hiding this comment.
I took some time to think about this. I understand the use case, it can give more flexibility. The use case is ok for me.
The current approach looks dangerous and quite an anti-pattern to me: as a user, when I set an empty value, I won't expect it to enable/disable something else in a parent. This annotation is already an override over the parent Gateway target.
IMHO, to override this override, you need to introduce an other annotation and/or a CLI flag. Something like external-dns.alpha.kubernetes.io/skip-gateway-target: true or external-dns.alpha.kubernetes.io/force-gateway-address: true.
I'll let others reviewers share their thoughts, they may see it differently.
There was a problem hiding this comment.
My thinking was that the annotation on the routes would override the annotation on the gateway. As such, setting an empty string would be as if you set an empty string on the Gateway. However, looking at it from another perspective a user might not see the route annotation as an override of the annotation on the gateway and expect an empty string to be the same as if there was no annotation at all. I guess it depends on how you look at it.
Admittedly I would find it a bit strange to explicitly be setting an empty string value on a route if you don't intend it to be an override for the annotation on the gateway.
Personally I'm always in favor of the simplest solution which I think the current approach is, but I'd also be fine in adding a second annotation. I think using a CLI flag would reduce the flexibility. The question then become, what should this annotation be?
There was a problem hiding this comment.
@mloiseleur Is there possibly somebody else that could also provide a review?
|
/label tide/merge-method-squash |
81aaf28 to
aacc8df
Compare
|
@ivankatliarchuk Would you be able to add a review here? |
|
Hi. Gateway CRDs is not something I'm familiar with. Too many legacy projects, so need some time to understand in full Gateway API. Before to review code, would you be able to share manifests files to reproduce the setup, so I could apply in my local cluster and results of your smoke tests as well? |
|
@ivankatliarchuk Sorry for the slow response, I've been juggling a lot of projects at work. You most likely would need to deploy some kind of controller that implements Gateway API, such as Istio. They have a good guide for getting started here. Just copy/pasting their examples here to make the workflow clear: A apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
name: gateway
namespace: istio-ingress
annotations:
external-dns.alpha.kubernetes.io/target: 4.3.2.1
spec:
gatewayClassName: istio
listeners:
- name: default
hostname: "*.example.com"
port: 80
protocol: HTTP
allowedRoutes:
namespaces:
from: All
status:
addresses:
- type: IPAddress
value: 1.2.3.4A apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
name: http
namespace: default
spec:
parentRefs:
- name: gateway
namespace: istio-ingress
hostnames: ["httpbin.example.com"]
rules:
- matches:
- path:
type: PathPrefix
value: /get
backendRefs:
- name: httpbin
port: 8000If a So the following apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
name: http-route-target
namespace: default
annotations:
external-dns.alpha.kubernetes.io/target: 2.3.4.5
spec:
parentRefs:
- name: gateway
namespace: istio-ingress
hostnames: ["httpbin-route-target.example.com"]
rules:
- matches:
- path:
type: PathPrefix
value: /get
backendRefs:
- name: httpbin
port: 8000And the following apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
name: http-override
namespace: default
annotations:
external-dns.alpha.kubernetes.io/target: ""
spec:
parentRefs:
- name: gateway
namespace: istio-ingress
hostnames: ["httpbin-override.example.com"]
rules:
- matches:
- path:
type: PathPrefix
value: /get
backendRefs:
- name: httpbin
port: 8000 |
|
My recent testing/review supports the discussion found at with #5319 (comment). With slightly a difference, not to add new annotations, but to support something very similar to My reasoning is that:
Therefore, I advocate for implementing both a "soft" override (activated by presence of specific annotation) and a "force" override (beneficial for critical SRE, platform, or infrastructure migrations). |
|
@ivankatliarchuk Could you maybe explain in a little more detail what you have in mind exactly? Then I can get that implemented so this PR hopefully doesn't stay open for too long. |
|
It's tough for me to suggest a solution without potentially disrupting someone's workflow, as I haven't personally encountered a use case that requires this yet. I'm currently working on a proposal for annotation improvements (see #5080), and I don't believe an empty target annotation is a desirable feature. The binary approach is easier to understand and support: the target annotation is either set with a value or it's not set at all. I'd suggest sharing the pros and cons of each approach you're considering. Alternatively, you might already have a preferred idea based on our discussion. |
|
Any plans to include this change in the upcoming 0.18 release? |
|
The usecase is if you have multiple HTTPRoutes on a single gateway for use with subdomains. For example, the Gateway handles all For some services I run need to be behind a CDN, they need a different target. Instead of having to maintain multiple gateways (which is cumbersome, and not easily automateable), I set the annotation on the HTTPRoute. This way those specific services get set to a CDN target that they points to the appropriate cname that hits the server. Assuming 1 gateway per domain is a bad assumption. Becuase you can have 64 domains technically per gateway, including wildcards, and an unlimited number of HTTPRoutes with Hostnames that bind to that gateway. You should be able to control all annotations on a per HTTPRoute/GRPCRoute, then looking up to the Gateway for gateway wide defaults (like TTLs or anything else). Right now this doesn't work and is a serious blocker for using external DNS with gateways. Without the annotation, it just sets A/AAAA records directly to the servers IP, removing them from the CDN/Load Balancer So this PR is really important for me to hopefully be merged soon. |
|
@daegalus @nichcuta @davidspek Following @ivankatliarchuk and my review, this PR won't be merged "as is". The use case is ok, the current approach / implementation is not. Feel free to:
|
|
@mloiseleur @ivankatliarchuk Sorry for the long time it's taken for me to get back to this. I've revised the implementation and documentation and updated the PR description. Please let me know what your thoughts are regarding this approach. |
a1561c4 to
94c7f37
Compare
3e48d4d to
0052dc5
Compare
|
/retest |
1 similar comment
|
/retest |
|
@mloiseleur @ivankatliarchuk It seems like the unit tests are now failing because of a race condition in the tests that should be resolved by #5841. Other than that, how does the new implementation look? |
|
@davidspek: The PR to fix the race condition has been merged.
It looks way better to me. @davidspek: Has it been tested on a real cluster ? @abursavich: Do you think you can take a look and review this PR ? This approach is more flexible and may solve some configuration challenges users can have with Gateway API on dns. But maybe I missed something. |
Signed-off-by: David van der Spek <david.vanderspek@flyrlabs.com>
Signed-off-by: David van der Spek <david.vanderspek@flyrlabs.com>
Co-authored-by: Michel Loiseleur <97035654+mloiseleur@users.noreply.github.com>
Co-authored-by: Michel Loiseleur <97035654+mloiseleur@users.noreply.github.com>
Signed-off-by: David van der Spek <david.vanderspek@flyrlabs.com>
Signed-off-by: David van der Spek <david.vanderspek@flyrlabs.com>
0052dc5 to
3f0449f
Compare
|
My general position is that perceived deficiencies in the expressiveness of Gateway API should be addressed in the Gateway API spec, wherever possible, instead of hacking them into ExternalDNS. They're very welcoming, have regular meetings, and a well-defined enhancements proposal process. Adding features to override Gateway API specifications in ExternalDNS makes the implementation more complicated, unpredictable, and brittle to future upstream changes. If target overrides on Routes are necessary, then the preference of Consider the explosion of strategies that might be added to support the currently experimental GEP-1713 ListenerSets, which adds ListenerSets as another link in the chain and possible place to add an override annotation between the Route and Gateway. If ListenerSets were standard at this point, I would suggest that the preference of From the perspective of Gateway API's design for roles and personas, I think IPs should be controlled by Infrastructure Providers or Cluster Operators that own Gateways and Listeners not Application Developers that own Routes. I don't understand the use case for a Route needing a different IP address. That sounds like a different Gateway (or Listener) to me. All of that said, I'm okay with supporting the implicit route-preferred strategy if it's really needed, but I would need convincing for the other strategies. |
|
I just saw @daegalus's comment:
My preference to support this would be to add an annotation that causes ExternalDNS to ignore the HTTPRoute (or any Source for that matter) so the user can create a DNSEndpoint for the CDN that won't fight with the HTTPRoute's endpoints. That way there's an escape hatch for anyone that wants to do something different, without needing to bake every possible special case into every Source. |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
|
PR needs rebase. DetailsInstructions 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-sigs/prow repository. |
FWIW I was able to achieve ignoring a Gateway resource via the existing I did find it confusing that Thank you @ivankatliarchuk @mloiseleur @lexfrei for all of your work on the Gateway API annotation/label placement clarity and @davidspek for this target PR! |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. DetailsIn 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-sigs/prow repository. |
Description
This PR enhances Gateway API *Route (HTTPRoute, GRPCRoute, TLSRoute, TCPRoute, UDPRoute) target resolution by:
Supported strategies (and behavior):
Fallback to Gateway status.addresses always ensures a valid target set when annotations contribute none.
Backward Compatibility:
Default behavior for Routes without a strategy now aligns with typical operator expectations: a Route’s own target takes precedence if specified; otherwise existing Gateway behavior is preserved.
Fixes #4779
Checklist