feat(Gateway API): improve annotations support #4870

Dadeos-Menlo · 2024-11-13T13:11:35Z

Description

The generation of DNS records from Gateway API resources involves combining information from a …Route resource and a Gateway resource. The current implementation, for the most part, only considers annotations from the Route resource. There are potential scenarios whereby it may be more convenient to assign annotations to the Gateway resource, thus ensuring that all DNS records associated with the given Gateway are similarly effected. An extreme example of such a scenario being when no Hostname information is specified (i.e. the given Gateway should accept any communication, irrespective of the DNS name used to establish it) but that a DNS record is desired to cover all Routes associated with the Gateway; in which case it would be convenient to apply an external-dns.alpha.kubernetes.io/hostname annotation to the Gateway itself, rather than having to apply annotations to every associated Route and having to deal with the potential confusion arising from precisely which Route owns the shared DNS record.

The proposed changes include:

Adding validation of resource endpoint label ownership to all of the Gateway related unit-tests
Refactor the process of Endpoint generation in order to preserve the relationship between Route and Gateway resources
Consider annotations from both Route and Gateway resources when generating an Endpoint
- In cases where the same annotation is present on both Route and Gateway resources, the Route derived value is used in preference
- external-dns.alpha.kubernetes.io/ttl annotations are merged to yield the lowest specified value
- In the case where external-dns.alpha.kubernetes.io/hostname is specified on a Gateway and that Gateway is the only Gateway contributing to an Endpoint the resource label is recorded as referring to the Gateway rather than the Route

No changes to the documentation are proposed because it currently implies that annotations on Gateway resources are supported, whereas in reality such annotations are only honoured when specified on Route resources.

Checklist

Unit tests updated
End user documentation updated

k8s-ci-robot · 2024-11-13T13:11:44Z

Welcome @Dadeos-Menlo!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-11-13T13:11:45Z

Hi @Dadeos-Menlo. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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-sigs/prow repository.

mloiseleur · 2024-11-13T14:33:56Z

/ok-to-test
Would you please add documentation on this feature ?

mloiseleur · 2024-11-13T14:34:30Z

@abursavich do you think you can do a first review on this PR ?

Dadeos-Menlo · 2024-11-13T14:57:55Z

Would you please add documentation on this feature ?

What additional documentation do you feel would add clarity?

In my opinion the proposed changes merely bring the implementation in line with the existing documentation.

No changes to the documentation are proposed because it currently implies that annotations on Gateway resources are supported, whereas in reality such annotations are only honoured when specified on Route resources.

mloiseleur · 2024-11-14T07:45:17Z

What additional documentation do you feel would add clarity?

In current documentation, the behavior when there are annotations on both Gateway & HTTPRoute is not detailed: no explanation and no example.

mloiseleur · 2024-11-14T07:45:46Z

/retitle feat(Gateway API): improve annotations support

ivankatliarchuk · 2025-01-12T01:20:13Z

Hi @Dadeos-Menlo . Nice work. Would it be possible to extract the refactoring into a separate pull-request to simplify a review and reduce blast radius?

Refactor the process of Endpoint generation in order to preserve the relationship between Route and Gateway resources

You could add tests as well that highlights current/incorrect behaviour to it.

Dadeos-Menlo · 2025-01-13T12:29:02Z

Hi Ivan,

In the interests of easing any review I separated the proposed changes into stand-alone commits. The sequence of which were broadly described by the bullet points provided in the initial description. My intention with these stand-alone commits was to mutate the pre-existing implementation, step-by-step, into one that exhibits what I consider to be more desirable behaviour (i.e. the consistent honouring of annotations assigned to Gateway resources).

The initial commit seeks to introduce additional unit-tests/expectations, regarding the Labels associated with every Endpoint generated from the various …Route/Gateway combinations exercised by the pre-existing tests. This commit avoids making any changes to the existing implementation, thus demonstrating that the asserted behaviour is representative of the current/existing behaviour.

The second commit seeks to refactor the existing implementation into a form conducive to tracking the relationship between …Route and Gateway resources throughout the processing required to generate the Endpoint objects. This commit avoids making any changes to the unit-tests, thus demonstrating that the asserted behaviour of the proposed, refactored, implementation is equivalent to that provided by the original implementation.

The third commit builds upon the refactored implementation in order to provide the intended additional functionality; namely the consistent honouring of annotations assigned to Gateway resources. This commit introduces new unit-test scenarios, associated with annotations being assigned to Gateway resources and in support of the proposed new behaviours, whilst avoiding modification of existing unit-test scenarios, thus demonstrating no regressions with respect to pre-existing asserted behaviours (NB: The essence of the "DualStack" tests were incorporated into the Gateway annotation orientated unit-test scenarios introduced).

The forth, and final, commit modifies the documentation as requested.

In direct response to your questions:

Would it be possible to extract the refactoring into a separate pull-request to simplify a review and reduce blast radius?

Whilst this would be possible, I consider that the proposed changes belong together in a single pull-request, and have made efforts to separate the mutations applied as outlined above.

You could add tests as well that highlights current/incorrect behaviour to it.

I consider that unit-test scenarios asserting the intended behaviour of annotations assigned to Gateway resources have been introduced, as part of the behavioural modifications proposed in commit three.

I hope that the explanation offered above aids your understanding and consideration. Thank you for your interest in these proposed changes.

Regards,

Peter

ivankatliarchuk · 2025-01-13T20:16:23Z

Thanks for the explanation. I agree with the overall direction of this change, so I won't raise any objections. However, given the complexity of the changes, (I do not possess maintainer access), I will be unable to formally approve this pull request, which is not an issue anyway.

Whilst this would be possible, I consider that the proposed changes belong together in a single pull-request, and have made efforts to separate the mutations applied as outlined above.

This make sense. To potentially accelerate the approval process, consider splitting the refactoring from the new functionality in the next pull request. This would allow community members to contribute more easily by focusing on smaller, more manageable changes.

mloiseleur · 2025-01-15T07:35:13Z

source/gateway.go

+					hosts[host] = struct{}{}
+				}
+			}
+			// TODO: The ignore-hostname-annotation flag help says "valid only when using fqdn-template"


The help is valid.
This option has been introduced with PR #745.
As you can see, it's still checked:
https://github.com/kubernetes-sigs/external-dns/blob/master/pkg/apis/externaldns/validation/validation.go#L84

This comment was merely relocated from the pre-existing sources…

I suspect that the observation being made by its original author is that the referenced help states that:

"Ignore hostname annotation when generating DNS names, valid only when --fqdn-template is set (default: false)"

and yet various implementations, including this one, do not check that the value of FQDNTemplate has been specified. However, they may not have appreciated the additional validation to which you refer that appears to preclude the IgnoreHostnameAnnotation value being true whilst the FQDNTemplate is empty.

Would you like me to eliminate the "TODO: …" comment?

Yes, please

Removed referenced comment, and associated comment, in 6ee2dc6.

k8s-ci-robot · 2025-01-23T13:49:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign mloiseleur for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mloiseleur · 2025-01-28T17:00:13Z

source/gateway.go

 		}
 		if !match {
-			log.Debugf("Gateway %s/%s section %q does not match %s %s/%s hostnames %q", namespace, ref.Name, section, c.src.rtKind, meta.Namespace, meta.Name, rtHosts)
+			log.Debugf("Gateway %s/%s section %q does not match %s %s/%s hostnames %q", namespace, ref.Name, section, c.src.rtKind, meta.Namespace, meta.Name, rt.Hostnames())


Suggested change

log.Debugf("Gateway %s/%s section %q does not match %s %s/%s hostnames %q", namespace, ref.Name, section, c.src.rtKind, meta.Namespace, meta.Name, rt.Hostnames())

log.Warnf("Gateway %s/%s section %q does not match %s %s/%s hostnames %q", namespace, ref.Name, section, c.src.rtKind, meta.Namespace, meta.Name, rt.Hostnames())

Shouldn't this be a warning ?

Quite possibly; this comment was merely relocated from the pre-existing sources…

I'm not familiar with the desired log verbosity and was attempting to minimise unnecessary changes in both behaviour and implementation.

I'll try to deduce what, if anything, that log is attempting to convey with a view to either upgrading it or eliminating it.

I see no compelling reason to change this logging…

It's arguably debug level algorithm diagnostics, similar to several other log.Debugf(…) function calls throughout the pre-existing implementation. The algorithm in question is broadly a combination of the logic outlined beneath the GatewaySpec.listeners and HTTPRouteSpec.hostnames specifications. This particular log line appears to pertain to a scenario whereby a …Route object has been accepted by a Gateway controller implementation (i.e. there is an affirmative entry in the RouteStatus.parents list) but that this implementation of the algorithm logic does not consider there to be a match between the parent Gateway's listeners and the …Route in question.

One would perhaps not expect such a scenario to occur. If such a scenario does occur then it might be a consequence of an issue with the Gateway controller or an issue with this implementation.

I'm not particularly familiar with the log level semantics employed by external-dns, or Kubernetes as a whole, but I tend to employ:

Error - something that is never expected to happen and is generally catastrophic

Warn - something that is expected to happen rarely and is potentially problematic

Debug - something that is potentially of interest to someone investigating in detail

A case could be made for upgrading most of the existing debug level logging to warn but, given the prior art, a log level of debug seems reasonable to me in this instance.

I'm not particularly familiar with the log level semantics employed by external-dns, or Kubernetes as a whole, but I tend to employ:
* Error - something that is never expected to happen and is generally catastrophic
* Warn - something that is expected to happen rarely and is potentially problematic
* Debug - something that is potentially of interest to someone investigating in detail

100% aligned with that.

A case could be made for upgrading most of the existing debug level logging to warn but, given the prior art, a log level of debug seems reasonable to me in this instance.

Works for me, even if we should not restrain ourselves from the current prior art.
@ivankatliarchuk wdyt ?

Debug is fine. I usually find myselft silencing WARN level as could not action on them

ivankatliarchuk

Could you please also provide a way to test this manually with manifests and kubectl commands for affected resources?

ivankatliarchuk · 2025-02-04T21:01:56Z

source/gateway.go

 		}
 		if !match {
-			log.Debugf("Gateway %s/%s section %q does not match %s %s/%s hostnames %q", namespace, ref.Name, section, c.src.rtKind, meta.Namespace, meta.Name, rtHosts)
+			log.Debugf("Gateway %s/%s section %q does not match %s %s/%s hostnames %q", namespace, ref.Name, section, c.src.rtKind, meta.Namespace, meta.Name, rt.Hostnames())


Debug is fine. I usually find myselft silencing WARN level as could not action on them

ivankatliarchuk · 2025-02-04T21:06:29Z

source/gateway_test.go

@@ -246,47 +245,3 @@ func TestIsDNS1123Domain(t *testing.T) {
 		})
 	}
 }
-
-func TestDualStackLabel(t *testing.T) {


Is this test moved or removed?

As noted previously…

NB: The essence of the "DualStack" tests were incorporated into the Gateway annotation orientated unit-test scenarios introduced

Dadeos-Menlo · 2025-02-10T13:06:50Z

Could you please also provide a way to test this manually with manifests and kubectl commands for affected resources?

Manual testing is generally inconvenient. This is especially the case when dealing with systems like "external-dns" that cover a significant expanse of external dependencies. That's why unit-tests are provided.

However, the following Kubernetes specification should offer a reasonable starting point for any manual testing that you may wish to perform:

apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
  name: external-dns-gateway
  annotations:
    external-dns.alpha.kubernetes.io/hostname: …
spec:
  gatewayClassName: …
  listeners:
  - protocol: HTTP
    port: 80
    name: http
---
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: external-dns-http-route
spec:
  parentRefs:
  - name: external-dns-gateway

Once customised to specify a gatewayClassName appropriate to your test environment, a DNS record, as specified via the external-dns.alpha.kubernetes.io/hostname annotation on the Gateway resource, should be created, when specifying --source gateway-httproute, where, without the proposed changes, one would not otherwise be.

ivankatliarchuk · 2025-02-11T09:22:14Z

Agree is inconvenient to test manually, but there is no other way to validate things, unless you have a proposal. I understand that this change was not tested on a cluster then, is my assumption correct?

Unit tests is just another layer, you can't test everything with them. For example, I could not validate with unit tests, that the feature is actually working. At the moment there are no automated acceptance tests or conformance tests.

Could you please as well share the arguments required, and add to an example an annotation external-dns.alpha.kubernetes.io/ttl. Am I understand correctly, the change was made, in a way, if it's set on HTTPRoute and on Gateway, it will be merge to yield the lowest specified value?

Worth to update docs/annotations/annotations.md with current behaviour

Dadeos-Menlo · 2025-02-11T10:39:35Z

I understand that this change was not tested on a cluster then, is my assumption correct?

No; your assumption is incorrect.

Unit tests is just another layer, you can't test everything with them.

You're probably not writing the correct unit tests then.

Could you please as well share the arguments required, and add to an example an annotation external-dns.alpha.kubernetes.io/ttl.

I'm sorry; I don't understand what you mean by this request.

Am I understand correctly, the change was made, in a way, if it's set on HTTPRoute and on Gateway, it will be merge to yield the lowest specified value?

You're understanding is correct; (see: description):

external-dns.alpha.kubernetes.io/ttl annotations are merged to yield the lowest specified value

and associated TTLGateway unit-test.

k8s-ci-robot · 2025-02-19T01:55:01Z

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-sigs/prow repository.

k8s-ci-robot requested review from mloiseleur and szuecs November 13, 2024 13:11

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 13, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 13, 2024

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 13, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 13, 2024

Dadeos-Menlo force-pushed the gateway_annotations branch 2 times, most recently from b632a81 to cc10e6d Compare November 13, 2024 14:50

k8s-ci-robot changed the title ~~Improve support for Gateway API annotations~~ feat(Gateway API): improve annotations support Nov 14, 2024

mloiseleur reviewed Jan 15, 2025

View reviewed changes

Dadeos-Menlo added 4 commits January 23, 2025 13:30

Add validation of Endpoint.Labels to unit-test expectations.

884f3ce

Refactor Gateway source processing.

6ee2dc6

Honour annotations specified on Gateway resources.

d9a899f

Update documentation.

b75786f

Dadeos-Menlo force-pushed the gateway_annotations branch from 304439d to b75786f Compare January 23, 2025 13:49

mloiseleur reviewed Jan 28, 2025

View reviewed changes

ivankatliarchuk reviewed Feb 4, 2025

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 19, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Gateway API): improve annotations support #4870

feat(Gateway API): improve annotations support #4870

Dadeos-Menlo commented Nov 13, 2024

k8s-ci-robot commented Nov 13, 2024

k8s-ci-robot commented Nov 13, 2024

mloiseleur commented Nov 13, 2024

mloiseleur commented Nov 13, 2024

Dadeos-Menlo commented Nov 13, 2024

mloiseleur commented Nov 14, 2024

mloiseleur commented Nov 14, 2024

ivankatliarchuk commented Jan 12, 2025

Dadeos-Menlo commented Jan 13, 2025

ivankatliarchuk commented Jan 13, 2025

mloiseleur Jan 15, 2025

Dadeos-Menlo Jan 15, 2025

mloiseleur Jan 23, 2025

Dadeos-Menlo Jan 23, 2025

k8s-ci-robot commented Jan 23, 2025

mloiseleur Jan 28, 2025

Dadeos-Menlo Jan 28, 2025

Dadeos-Menlo Jan 31, 2025

mloiseleur Jan 31, 2025

ivankatliarchuk Feb 4, 2025

ivankatliarchuk left a comment

ivankatliarchuk Feb 4, 2025

ivankatliarchuk Feb 4, 2025

Dadeos-Menlo Feb 10, 2025

Dadeos-Menlo commented Feb 10, 2025

ivankatliarchuk commented Feb 11, 2025 •

edited

Loading

Dadeos-Menlo commented Feb 11, 2025

k8s-ci-robot commented Feb 19, 2025

	log.Debugf("Gateway %s/%s section %q does not match %s %s/%s hostnames %q", namespace, ref.Name, section, c.src.rtKind, meta.Namespace, meta.Name, rt.Hostnames())
	log.Warnf("Gateway %s/%s section %q does not match %s %s/%s hostnames %q", namespace, ref.Name, section, c.src.rtKind, meta.Namespace, meta.Name, rt.Hostnames())

feat(Gateway API): improve annotations support #4870

Are you sure you want to change the base?

feat(Gateway API): improve annotations support #4870

Conversation

Dadeos-Menlo commented Nov 13, 2024

k8s-ci-robot commented Nov 13, 2024

k8s-ci-robot commented Nov 13, 2024

mloiseleur commented Nov 13, 2024

mloiseleur commented Nov 13, 2024

Dadeos-Menlo commented Nov 13, 2024

mloiseleur commented Nov 14, 2024

mloiseleur commented Nov 14, 2024

ivankatliarchuk commented Jan 12, 2025

Dadeos-Menlo commented Jan 13, 2025

ivankatliarchuk commented Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivankatliarchuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dadeos-Menlo commented Feb 10, 2025

ivankatliarchuk commented Feb 11, 2025 • edited Loading

Dadeos-Menlo commented Feb 11, 2025

k8s-ci-robot commented Feb 19, 2025

ivankatliarchuk commented Feb 11, 2025 •

edited

Loading