handle apex domain through gandi ALIAS #3855

tommy31 · 2023-08-10T14:51:48Z

Description

This PR allow gandi provider to redirect apex domain to an FQDN.

From my understanding, CNAME record only can be used for subdomains and external-dns force CNAME if record value is an FQDN. To fill this gap some DNS provider introduce a new record type ALIAS. ALIAS record is a type of DNS record that points your domain name to a hostname instead of an IP address. The ALIAS record is similar to a CNAME record, which is used to point subdomains to a hostname. This allow us to register a record an apex domain @ with a FQDN.

So back on this PR, the fix convert an CNAME to an ALIAS, if the record.RrsetName == "@" and record.RrsetType == "CNAME". And env var (GANDI_PREFER_ALIAS) has been added to keep old things working as expected.

What Are ALIAS Records?

This fix has been tested in our preproduction environment and work as expected.

Checklist

Unit tests updated
End user documentation updated

k8s-ci-robot · 2023-08-10T14:51:57Z

Hi @tommy31. 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/test-infra repository.

mloiseleur · 2023-08-11T08:14:08Z

Hello @tommy31,

Thanks for this PR. Alias seems tested.
This PR also introduces TXTPrefix & TXTSuffix support, shouldn't this be tested also ?

tommy31 · 2023-08-11T12:10:05Z

Hey @mloiseleur, thank for reaching me.

TXTPrefix and TXTSuffix are only used in inferZone function, to infer requested zone by removing prefix and suffix from TXT record name. Usage should be trivial.

// inferZone determines the zone based on the RrsetName
func inferZone(RrsetName string, TXTPrefix string, TXTSuffix string) string {
	cleanRrsetName := strings.Replace(RrsetName, TXTPrefix, "", 1)
	cleanRrsetName = strings.Replace(cleanRrsetName, TXTSuffix, "", 1)
	cleanRrsetName = strings.Replace(cleanRrsetName, "cname-", "", 1)

	return cleanRrsetName
}

To better answer your question, of course we should add more tests, but my understanding of gandi_test.go... is near 0.

Maybe you could help as gandi provider miss mainteners.

johngmyers

You might want to take a look at how the AWS provider handles aliases, with providerSpecificAlias and the external-dns.alpha.kubernetes.io/alias annotation.

johngmyers · 2023-08-12T00:08:01Z

provider/gandi/gandi.go

 	sharingID, _ := os.LookupEnv("GANDI_SHARING_ID")
+	_, PreferALIAS := os.LookupEnv("GANDI_PREFER_ALIAS")


Why are we taking configuration from environment variables? Shouldn't we be using the flags mechanism that everything else uses?

The "PREFER_ALIAS" config was added to be "retro-compatible" with previous behaviour - which is to fail and raise an error. If it's alright we'd like to get rid of this config altogether and use the "CNAME is an ALIAS in gandi" feature by default
(I'll be working on this PR with @tommy31 - thanks for your review!)

Updated in 2892142 - the fixed behaviour is now the default, and we stopped using an extra environment variable for this

johngmyers · 2023-08-12T00:09:32Z

provider/gandi/gandi.go

+
+// isGandiAlias determines if a given endpoint is supposed to create an Gandi Alias record
+func isGandiAlias(r livedns.DomainRecord, preferALIAS bool) bool {
+	return preferALIAS && r.RrsetType == RecordTypeALIAS


Why does the setting of preferAlias affect whether an existing DomainRecord is an alias?

Since we removed the "prefer alias" mechanism, I think this is no longer an issue?

johngmyers · 2023-08-12T00:10:53Z

provider/gandi/gandi.go

@@ -113,16 +151,24 @@ func (p *GandiProvider) Records(ctx context.Context) ([]*endpoint.Endpoint, erro
 		}

 		for _, r := range records {
+			if isGandiAlias(r, p.PreferALIAS) {
+				// Convert back ALIAS to CNAME
+				r.RrsetType = endpoint.RecordTypeCNAME


Shouldn't this set a provider-specific property to indicate the underlying record is an alias?

This is already manipulating a provider-specific object - Gandi's livedns.DomainRecord

Do you think this should be set before in the endpoint.ProviderSpecificProperty prop and reused later?

johngmyers · 2023-08-12T00:17:32Z

provider/gandi/gandi.go

+func inferZone(RrsetName string, TXTPrefix string, TXTSuffix string) string {
+	cleanRrsetName := strings.Replace(RrsetName, TXTPrefix, "", 1)
+	cleanRrsetName = strings.Replace(cleanRrsetName, TXTSuffix, "", 1)
+	cleanRrsetName = strings.Replace(cleanRrsetName, "cname-", "", 1)


This just screams layering violation to me. Why is the provider mucking about with things that are in the purview of the TXT registry? Why is this only removing a prefix of cname-?

There probably is a better way to do it but not sure how - the issue is that gandi's API requires us explicitly declare which "zone" the records we are manipulating are in.
When working on a CNAME on an apex domain, TXTRegistry creates the old and new txt records, with and without the type prefix. When we receive the endpoint with value cname-example.com 0 IN TXT \"heritage=external-dns,external-dns/owner=default\" [], we have no way to reliability find the zone without this kind of tomfoolery, do we?

I have tried to improve this with 83a1fcd - I still feel like we have no better way to compute a "zone" from an Endpoint

We can spin this change in a new PR if you think it is necessary?

johngmyers · 2023-08-12T00:20:55Z

provider/gandi/gandi.go

+			if change.Record.RrsetType == endpoint.RecordTypeCNAME && !strings.HasSuffix(change.Record.RrsetValues[0], ".") {
+				change.Record.RrsetValues[0] += "."
+			}
+
 			// Prepare record name
 			recordName := strings.TrimSuffix(change.Record.RrsetName, "."+change.ZoneName)
 			if recordName == change.ZoneName {


If the zone is example.com and the change.Record.RrsetName is example.com.example.com, then this code will incorrectly replace recordName with @.

To reduce noise on this PR, I have fixed this in #3893

linux-foundation-easycla · 2023-09-18T10:25:34Z

The committers listed above are authorized under a signed CLA.

✅ login: gfaugere / name: Gaëtan Faugère (ff45612)

k8s-ci-robot · 2023-09-18T10:25:36Z

[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 ask for approval from johngmyers. For more information see the Kubernetes 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

gfaugere · 2023-09-18T10:42:19Z

This has been updated with new tests following #3893 !

mloiseleur · 2023-09-18T11:26:44Z

/ok-to-test

mloiseleur

I understand from this PR that MX record type is not supported.
=> If that's the case, I guess you need to improve documentation about this.

For TXT registry, we are trying to fix it. See #3757 and #3774.
Wdyt about adding the zone at the end of the record when it's not here ?

gfaugere · 2023-09-18T13:14:37Z

Regarding MX records, I think the current doc (https://github.com/kubernetes-sigs/external-dns/blob/master/docs/tutorials/mx-record.md) is clear enough about the providers that support this record type?

gfaugere · 2023-09-18T14:28:33Z

Thanks for your work on the TXT registry side! It is the only confusing thing about external-dns for me

I have added test cases (in 48e9d6d), I think we are handling them correctly?

gfaugere · 2023-09-18T14:28:52Z

Wdyt about adding the zone at the end of the record when it's not here ?

During which actions? Not sure what you mean here

mloiseleur · 2023-09-18T15:55:06Z

Wdyt about adding the zone at the end of the record when it's not here ?

During which actions? Not sure what you mean here

I agree with @johngmyers that we should try our best to avoid dependency between Txt Registry logic and provider logic.

Following this, adding Gandi constraints you described:

gandi's API requires us explicitly declare which "zone" the records we are manipulating are in.

The idea would be instead of adding the zone (L174)

				name := r.RrsetName + "." + zone

and removing it a few lines after if/when it comes from TXT registry.

Something like

  if ! strings.HasSuffix(r.RrsetName, zone) {
     r.RrsetName := r.RrsetName + "." + zone
  }

Without any need to check wheither it's coming from TXT Registry or not.

It seems quite a naive idea, so I'm unsure and I feel I missed something.

gfaugere · 2023-09-21T15:25:25Z

(sorry for the delay, and thanks so much for your feedback!)

This works for most cases and allows us to drop our weird dropAffix function, but it fails for one use case: using TXT suffix with "apex" domains

To me the root of the issue is this: let's say the my provider receives this Endpoint in its ApplyChanges method:

Endpoint{
	DNSName:    "a-example__suffix.com",
	Targets:    endpoint.Targets{"\"heritage=external-dns,external-dns/owner=default\""},
	RecordType: "TXT",
	RecordTTL:  600,
}

This is the TXT record flagging that there is an A record on example.com that was created by external-dns, with --txt-suffix=__suffix used

I really don't think there is a way for me to work out this belongs to the example.com zone. I tried adding test cases to the zonefinder, and as soon as I add a prefix or suffix as the TXT registry does, it falls apart. I do feel like I am missing something though, am I?

mloiseleur · 2023-09-22T08:32:47Z

Considering #3757, txt prefix & suffix will go away. The PR implementing it, #3774 has made good progress.

So, 🤔 maybe dropping support of txt prefix & suffix a bit earlier for new versions of external-dns with this provider is acceptable.

gfaugere · 2023-09-22T10:21:44Z

You're right, this is hair-pulling for no benefits - I'll update this today to drop the prefix/suffix part

Should we mark this as holding for #3774? I feel like it is cleaner to wait for it to drop

mloiseleur · 2023-09-22T12:36:06Z

/hold for #3774

Co-Authored-By: vinhas tommy <tommy.vinhas@gmail.com>

mloiseleur · 2023-09-26T12:27:08Z

/lgtm

k8s-triage-robot · 2024-01-21T22:20:50Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

mloiseleur · 2024-01-22T10:51:29Z

/remove-lifecycle stale

k8s-ci-robot · 2024-02-28T08:48:13Z

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/test-infra repository.

k8s-triage-robot · 2024-05-28T09:06:47Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2024-06-27T09:27:35Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2024-07-27T09:43:35Z

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:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2024-07-27T09:43:40Z

@k8s-triage-robot: Closed this PR.

In response to this:

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:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 10, 2023

k8s-ci-robot requested review from johngmyers and seanmalloy August 10, 2023 14:51

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 Aug 10, 2023

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 10, 2023

johngmyers suggested changes Aug 12, 2023

View reviewed changes

k8s-ci-robot assigned johngmyers Aug 12, 2023

gfaugere mentioned this pull request Aug 24, 2023

Gandi provider: refactor tests to be more readable, robusts, and extensible; and minor fixes #3893

Merged

2 tasks

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

gfaugere force-pushed the feat/gandi_apex_domain branch from 83a1fcd to 8704636 Compare September 18, 2023 10:25

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 18, 2023

gfaugere force-pushed the feat/gandi_apex_domain branch from 8704636 to 84990a9 Compare September 18, 2023 10:35

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 18, 2023

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 Sep 18, 2023

k8s-ci-robot assigned mloiseleur Sep 18, 2023

mloiseleur reviewed Sep 18, 2023

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2023

gfaugere force-pushed the feat/gandi_apex_domain branch 2 times, most recently from 95003c4 to 53dbf49 Compare September 26, 2023 09:10

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 26, 2023

feat(gandi): handle CNAME on apex domains through ALIAS records

ff45612

Co-Authored-By: vinhas tommy <tommy.vinhas@gmail.com>

gfaugere force-pushed the feat/gandi_apex_domain branch from 53dbf49 to ff45612 Compare September 26, 2023 09:11

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 26, 2023

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 21, 2024

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 22, 2024

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

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 28, 2024

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 27, 2024

k8s-ci-robot closed this Jul 27, 2024

		sharingID, _ := os.LookupEnv("GANDI_SHARING_ID")
		_, PreferALIAS := os.LookupEnv("GANDI_PREFER_ALIAS")

handle apex domain through gandi ALIAS #3855

handle apex domain through gandi ALIAS #3855

Conversation

tommy31 commented Aug 10, 2023

k8s-ci-robot commented Aug 10, 2023

mloiseleur commented Aug 11, 2023

tommy31 commented Aug 11, 2023 • edited Loading

johngmyers 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

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

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

linux-foundation-easycla bot commented Sep 18, 2023 • edited Loading

k8s-ci-robot commented Sep 18, 2023

gfaugere commented Sep 18, 2023

mloiseleur commented Sep 18, 2023

mloiseleur left a comment • edited Loading

Choose a reason for hiding this comment

gfaugere commented Sep 18, 2023

gfaugere commented Sep 18, 2023 • edited Loading

gfaugere commented Sep 18, 2023

mloiseleur commented Sep 18, 2023 • edited Loading

gfaugere commented Sep 21, 2023 • edited Loading

mloiseleur commented Sep 22, 2023

gfaugere commented Sep 22, 2023

mloiseleur commented Sep 22, 2023

mloiseleur commented Sep 26, 2023

k8s-triage-robot commented Jan 21, 2024

mloiseleur commented Jan 22, 2024

k8s-ci-robot commented Feb 28, 2024

k8s-triage-robot commented May 28, 2024

k8s-triage-robot commented Jun 27, 2024

k8s-triage-robot commented Jul 27, 2024

k8s-ci-robot commented Jul 27, 2024

tommy31 commented Aug 11, 2023 •

edited

Loading

linux-foundation-easycla bot commented Sep 18, 2023 •

edited

Loading

mloiseleur left a comment •

edited

Loading

gfaugere commented Sep 18, 2023 •

edited

Loading

mloiseleur commented Sep 18, 2023 •

edited

Loading

gfaugere commented Sep 21, 2023 •

edited

Loading