handle apex domain through gandi ALIAS

[tommy31]

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

• [x] Unit tests updated
• [x] End user documentation updated

[k8s-ci-robot]

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]

Hello @tommy31,

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

[tommy31]

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.

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: gfaugere / name: Gaëtan Faugère (ff45612f5cc9b388baf18a1d526e8d3459656042)

[k8s-ci-robot]

[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]

This has been updated with new tests following #3893 !

[mloiseleur]

/ok-to-test

[gfaugere]

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]

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 48e9d6d9557bd8c14a10c0e18af70c8f007c2060), I think we are handling them correctly?

[gfaugere]

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]

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]

(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]

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

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

[gfaugere]

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]

/hold for #3774

[mloiseleur]

/lgtm

[k8s-triage-robot]

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]

/remove-lifecycle stale

[k8s-ci-robot]

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]

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]

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]

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]

@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.