-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add Support for Gateway API Route Sources #2292
Conversation
2245893
to
4b6a8ee
Compare
Pushed changes to fix some doc formatting and make some names more consistent. |
4b6a8ee
to
7ebfe73
Compare
7ebfe73
to
c30a3f6
Compare
Rebased after dependabot merges. |
04d78ec
to
603baca
Compare
603baca
to
0f08392
Compare
0f08392
to
6e26f94
Compare
Updated to sigs.k8s.io/gateway-api@v0.4.0. |
6e26f94
to
6e905b9
Compare
Rebased changes from #2371. |
6e905b9
to
0d77082
Compare
0d77082
to
32d2e20
Compare
11e9028
to
db8435d
Compare
From a Gateway API point of view, this LGTM. I'd love to see it in, it will really help Gateway API users. (Gateway API maintainer here :) ) The tricky part is in the Gateway and Route checking logic, and that's all correct. @Raffo, over to you. |
/unassign @Raffo |
I noticed that @Raffo's status is "💤 Taking a break from open source." So... reassigned. |
/kind feature |
/lgtm |
@njuettner would you have time to take a look at this one? |
/assign @Raffo |
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.
@abursavich I started to take a look at this PR, added two comments and I appreciate the abundant use of TODOs to signal points in which we might need to offer guidance. My problem reviewing this is that it's really huge and it's hard for me to make a sense of it. I will do the following:
- ask in Slack is someone is willing to help with the review of this.
- if we get no answer, give it another pass and eventually merge as it's an optional source anyway and the common parts modified seem fine.
} | ||
|
||
func newGatewayRouteSource(clients ClientGenerator, config *Config, kind string, newInformerFn newGatewayRouteInformerFunc) (Source, error) { | ||
ctx := context.TODO() |
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.
Is context.TODO
the right context to use here?
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.
I think so.
https://pkg.go.dev/context#TODO
TODO returns a non-nil, empty Context. Code should use context.TODO when it's unclear which Context to use or it is not yet available (because the surrounding function has not yet been extended to accept a Context parameter).
} | ||
|
||
informerFactory := newGatewayInformerFactory(client, config.GatewayNamespace, gwLabels) | ||
gwInformer := informerFactory.Gateway().V1alpha2().Gateways() // TODO: Gateway informer should be shared across gateway sources. |
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.
Are we fine with keeping the non shared across resources informers? Could it cause problems?
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 shouldn't cause problems. The issue is that each informer instance creates another watch on the apiserver. Sharing wherever possible will reduce network traffic and load on the apiserver.
Informers are not shared across existing sources. The node, service, and pod sources are good examples of this duplication.
resourceKey := fmt.Sprintf("%s/%s/%s", kind, meta.Namespace, meta.Name) | ||
providerSpecific, setIdentifier := getProviderSpecificAnnotations(annots) | ||
ttl, err := getTTLFromAnnotations(annots) | ||
if err != nil { |
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.
If we ignore the error here, we'll be passing nil
to endpointsForHostname
. Would this even be correct? Isn't it better to return the error?
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.
A few sources return the error, but the most common behavior seems to be log a warning and continue. When there's an error, the ttl value is 0 which ends up being replaced with whatever the default ttl is configured as.
e.g.: https://github.com/kubernetes-sigs/external-dns/blob/v0.11.1/source/ingress.go#L229-L234
Btw, it's already gotten LGTMs from @youngnick and @seanmalloy. |
@Raffo any thoughts on merging this? |
We can merge this 👍 |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abursavich, Raffo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description
This adds support for Gateway API (v1alpha2 at v0.4.0-rc1) HTTPRoute, TLSRoute, TCPRoute, and UDPRoute kinds as sources.
Since the upstream API is still in alpha, no backwards compatibility guarantees are made for its support in ExternalDNS.
Fixes #2045
Checklist