Skip to content
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

Support of target annotations in gateway source #3749

Closed
solmonk opened this issue Jun 29, 2023 · 9 comments
Closed

Support of target annotations in gateway source #3749

solmonk opened this issue Jun 29, 2023 · 9 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@solmonk
Copy link

solmonk commented Jun 29, 2023

What would you like to be added:

LIke other sources, it would be great if Gateway source also has external-dns.alpha.kubernetes.io/target annotation support.

Why is this needed:

We are working on AWS Gateway API Controller which is a Gateway API implementation using AWS VPC Lattice. We would like to have ExternalDNS as a default way of user to configure customized domain names.

Currently Gateway source uses gateway.Status.Addresses as DNS record target for all hostnames. This is fine in most cases, but AWS Gateway API Controller does not have a centralized entrypoint for the Gateway - because it varies based on VPC, AZs, etc. To support those kind of different address behaviors, it would be great if DNS record targets are configurable at Route level.

external-dns.alpha.kubernetes.io/target annotation seems like the right way to support this use case, but this is not supported in Gateway source yet. Support of target annotation will give better flexibility to lots of Gateway API implementations.

@solmonk solmonk added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 29, 2023
@johngmyers
Copy link
Contributor

My thoughts are that we should deprecate the external-dns.alpha.kubernetes.io/target annotation, as redundant with the crd source.

If the workload deployer needs to specify the target, what's the advantage of having it be specified on the Gateway instead of straightforwardly in a DNSEndpoint?

@solmonk
Copy link
Author

solmonk commented Jun 30, 2023

For clarification: It needs to be specified on HTTPRoute, not Gateway - for overriding purpose. Just like most other annotations that gateway source supports.

If the workload deployer needs to specify the target, what's the advantage of having it be specified on the Gateway instead of straightforwardly in a DNSEndpoint?

When setting up any Gateway API implementation, I think gateway source will be what users have a look first. Otherwise we would need to ask users to use CRD source and explicitly avoid the use of gateway source. This does not look straightforward personally.

Also I would like to have controller take care of target override dynamically, instead of letting users specify a fixed endpoint spec. In this case adding annotation seems more straightforward than managing a new resource.

@johngmyers
Copy link
Contributor

Thank you for the additional information. For background, I am working on a proposal to promote some of the external-dns annotations to stable. The target annotation is one that doesn't make a lot of sense to me, so information about its use cases is helpful.

It would be helpful to me to have more details as to why gateway.Status.Addresses is not useful for AWS VPC Lattice. What is a situation where the same Gateway would have two HTTPRoutes with different DNS targets?

If it is the workload deployer who is creating the target annotation, that does not seem to be a terribly usable workflow. They would have to create the HTTPRoute, find out through some external process what the target is, then modify the HTTPRoute to add the annotation.

If the Gateway controller is what is supplying the target, that is something that should be going into a Status field, not an annotation. Have you considered getting the Gateway API to add an Status.Addresses field to HTTPRoute? I do see that having the Gateway controller create DNSEndpoint resources would require duplicating external-dns logic around annotations, but need to balance that against carrying something for the use case of source resources that don't have adequate Status fields.

@BadLiveware
Copy link
Contributor

While I didnt have the exact use-case as @solmonk in this issue. We did need to override the .status.addresses of gateways, thus my PR #3452 to support it.

We ultimately did not settle on gateway but the general use-case for us was to use google external https load balancers as a TLS-terminating edge proxy and CDN, provisioned via terraform and point that to one or more internal clusters where we handle the actual routing with other ingress controllers(or in our case at the time, istio).

We are still running a similar solution but not via gateway, but ingress(GCE<pre-provisioned static external ip(s)>) -> ingress-nginx(--publish-status-address=<pre-provisioned static external ip(s)>) -> ingress(nginx) -> actual service. Running this we can use external-dns using it's implicit configurtion(.status.addresses, domain name from Host: etc. without overrides everywhere, or statically configuring DNSEndpoints for every leaf ingress(nginx))

The ultimate reason is that googles ingress support essentially no features we needed/wanted, and very few ingress controllers or gateway api implementations support overriding the status address of ingress/gateways. ingress-nginx is the only one I could find that actually allows for it.

@solmonk
Copy link
Author

solmonk commented Jul 4, 2023

In VPC Lattice each ingress endpoint of Gateway can only handle traffic to certain group of services - depending on AWS account, etc. So yes, rather than through gateway status, users find out DNS targets provided by VPC Lattice API. On using custom domain names, users need to put DNS record manually because ExternalDNS does not work in our use case. We are looking for improving this process.

I would say Gateway API has enough Status fields, it is just that the API leaves a lot of room for interpretation. I think addresses can have a few restrictions in depending on implementations. Maybe we could find some way in Gateway API itself, but I think annotations can provide a solution that works regardless of direction, even before the Gateway API spec is finalized.

@johngmyers
Copy link
Contributor

Annotations, especially those that are expected to be written as controllers, are a code smell indicating that either the API is missing fields or that the controller is misapplying the API. Controllers should be writing to Status.

In this case, I suspect that VPC Lattice is misapplying the Gateway API. If there are two different ingress points, there should be two different Gateway resources.

If we were to support external-dns.alpha.kubernetes.io/target for the gateway source, for the "extra proxy in front" use case, we would likely do so on the Gateway, not the various Route resources.

@johngmyers
Copy link
Contributor

@BadLiveware thank you for describing the "extra proxy in front" use case; that was quite helpful. We will probably want to update the documentation of the annotation to describe that use case.

@johngmyers
Copy link
Contributor

Implemented in #3452
/close

@k8s-ci-robot
Copy link
Contributor

@johngmyers: Closing this issue.

In response to this:

Implemented in #3452
/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants