feat: add support for k8s networking API gateway backed istio gateways#6438
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @mwhittington21! |
|
Hi @mwhittington21. 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 Regular contributors should join the org to skip this step. 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. |
|
From first look - no go. This PR is proposing coupling istio with Gateway. This will be too expensive to maintain. |
|
You most likely need to talk to istio and gateway to describe the challange the team is facing and try to understand what the challanges are. |
|
/hold |
Fix 1: fix(istio): resolve annotation prefix not applied to gateway/ingress annotation keys IstioGatewayIngressSource and K8sGatewaySource were package-level vars evaluated at import time, before --annotation-prefix is applied. This caused Istio gateways using the /ingress or /gateway annotations to silently produce no endpoints, resulting in record deletion under sync policy. Fix 2: fix(istio): skip gateways with bad annotations instead of crashing the controller A nonexistent gateway or ingress reference in an annotation caused a fatal error that killed the pod. Now logs a warning and continues processing remaining resources.
96b81d1 to
bab1bf9
Compare
|
@ivankatliarchuk can you help me to understand why this is harder to maintain than the existing functionality pairing to the Ingress object? Is this purely because the Gateway API is planned to evolve, while the Ingress API was frozen? This PR is just seeking to get parity with how Ingress objects are already allowed to be wired using external-dns annotations. EDIT: to summarise, the core challenge of knowing the hostname of the Gateway's DNS name is too difficult in cluster setups where the ingress layer is not owned by the teams running services in disparate namespaces. Allowing external-dns to discover this solves it. |
|
Consider other option. We are not going to merge multiple products in single source. Tomorrow someone will decide to suppport gloo + Traefik, or istio with with something else like ALB contoller CRDs. The intention is to keep sources simple. If there are missing capablitiles on istio or gateway - propose solutions there. |
|
Ingress is a core Kubernetes API (not a separate product), while the Gateway API is an evolving SIG project with its own release cadence and CRD lifecycle. Coupling Istio sources to Gateway API introduces a second optional CRD dependency into those sources, which is a qualitatively different kind of coupling. On top of that, Gateway have multiple resources. For this specific use case - the best option is to create inhouse solution, it could couple different products - generate annotations for external-dns or create DNSEdnpoints. Plust open an issue, if there is enough attraction - we could consider options. |
|
Somewhere similar problem (reference muitliple products) #6190 |
|
Thanks for the clear explanation, this makes sense on the direction that external-dns wants to head now.
I suspect this is the direction we'll have to head in. I'm thinking the best approximation to what we have today is a well-known DNS name per cluster which can be calculated, is placed onto the Gateway via other means (possibly even external-dns annotations), and is then referenced by all the On another note, I found two bugs affecting current |
|
Let's keep it for now, maybe other maintaners think differently and we should start making sources heavy and richer in terms of cross referencing them. It is clear, that we have gaps in functionalities. Supporting only gateway - the use case is quite specific. Have a look at #6190 (comment). i'm not sure if it's even possible to implement what was mentioned there. We do need something more dynamic to mary/cross reference different sources as coupling them is gong to be a challanging for us. |
What does it do ?
Feature
This adds support for a new annotation on Istio Gateway objects which acts similarly to the existing ingress annotation, but can point at a
gateway.networking.k8s.ioGateway object instead. It exists for similar reasons to the original PR adding ingress annotations #3842.If the Gateway API is installed in a cluster, the Istio Gateway and VirtualService sources will use it to enable this feature. If not, the sources will act as before and the annotation will not have an effect. This implements similar test coverage to the original Ingress annotation, ensuring the annotation should work in an identical way but for a different backing resource.
The PR size is large primarily due to test coverage, and the documentation inclusion.
Bugfixes
During testing of the functionality in a lab, I came across and fixed two other bugs. The first was related to using
--annotation-prefixto move back to the*.alpha.*external-dns annotations, and how this actually wasn't propagated into the Istio source controllers. This led to all/ingressannotations having their DNS entries deleted on my test cluster.The second was that a
/ingressannotation pointing to a nonexistent object would cause the controller to crashloop. This could cause denial of service in a shared tenanted cluster. This bug was extended to gateway, and the code path was fixed for both.Tests were added to cover the bugs.
Motivation
My team has been using the ingress annotation functionality in order to run a single Ingress in the cluster and allow Service owners to hook into a centrally managed Ingress object and create CNAMEs pointing to it. This allows ingress through a shared set of Istio gateways, fronted by a shared cloud provider LB. One big advantage to this process is that we can change anything about that central Ingress (e.g. it's IP address, associated DNS names, etc) and service owners do not need to be aware of these changes. They only need point at
<namespace>/<ingress-name>via the annotation and external-dns handles updates to their own DNS names automatically.In GKE clusters we are looking at moving away from provisioning the LB using the Ingress object and instead using the
gateway.networking.k8s.ioGateway object in order to get access to more modern GCP load balancer implementations. As part of this migration, we could not find a way to use the existing provided annotations to keep the same pattern where services do not need to know implementation details of the centrally provisioned LB. The closest suitable pattern we found was using theexternal-dns.alpha.kubernetes.io/targetannotation along with the hostname of the LB, but this leaks more implementation details than we are comfortable with since it requires a hostname to be specified.This led to the creation of this PR, looking to duplicate the same functionality of looking at an externally provisioned LB and creating CNAMEs to it (or A records if it exposes an IP). If the same pattern was suitable for Ingress objects, it should also be suitable for Gateway objects.
More