feat: Become IDNA aware in Plan and DomainFilter

[kimsondrup]

Description

Ensure support for Internationalized Domain Names for Applications (aka. domains using Unicode) using golang.org/x/net/idna

Disclaimer, this is my first Go code so please look at it with extra skepticism

Checklist

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

[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 assign raffo for approval. 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

[k8s-ci-robot]

Hi @kimsondrup. 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]

Thanks for this PR @kimsondrup ! Reviewing the tests with all those emojis in FQDN are really fun :) /ok-to-test

[kimsondrup]

After looking at the code a bit more, I'm a little confused. I am trying to figure out the reasoning behind some of the places that External DNS normalize values. For example, the function RemoveDuplicates() in endpoint/endpoint.go or the places that use strings.ToLower() in registry/txt.go could look like some places where IDNA awareness might be wanted. I am tempted to think that all the places should be normalized, but I am unsure of the impact. If there were more well-defined tests for normalization in these places, like the ones seen in Plan and DomainFilter, I would be less hesitant to touch things.

~~But I could give a shoot at implementing it if you think it make sense to do so?~~

[kimsondrup]

After looking at bit more at the code I think I will just summit the changes for registry/txt.go IDNA awareness in another pull request. It is less cut and dry since txtPrefix and txtSuffix might conflict with how Ponycode prefix each level of the domain with xn-- and again suffix each level with the encoded characters. But that is a problem for future me.

@mloiseleur if this pull request looks good to you then I have nothing else to add.

[mloiseleur]

It is less cut and dry since txtPrefix and txtSuffix might conflict with how Ponycode prefix each level of the domain with xn-- and again suffix each level with the encoded characters.

:thinking: If the prefix and suffix are applied before the idna toAscii is called, then it should work, isn't it ?

[kimsondrup]

It is less cut and dry since txtPrefix and txtSuffix might conflict with how Ponycode prefix each level of the domain with xn-- and again suffix each level with the encoded characters.

🤔 If the prefix and suffix are applied before the idna toAscii is called, then it should work, isn't it ?

Yes, I would also prefer a solution where prefix and suffix are applied before the IDNA toAscii is called, but given I don't know the codebase that well, I would like to either unquestioningly trust someone to tell me how it should be done or get some more time to read the code myself 😅

Some providers might be IDNA aware themself so that I can imagine an External DNS playing around with prefix and suffix introducing some problems.

Just an example of some results

prefix	Unicode concat	Unicode concat	ASCII concat
extd.	ørsted.dk	extd.xn--rsted-uua.dk	extd.xn--rsted-uua.dk
extd.	orsted.dk	extd.orsted.dk	extd.orsted.dk
extd-	ørsted.dk	xn--extd-rsted-4cb.dk	extd-xn--rsted-uua.dk
extd-	orsted.dk	extd-orsted.dk	extd-orsted.dk
xn-	ørsted.dk	xn--xn-rsted-74a.dk	xn-xn--rsted-uua.dk
xn-	orsted.dk	xn--xn-rsted-74a.dk	xn-xn--rsted-uua.dk
xn--1	ørsted.dk	xn--xn--rsted-o8a.dk	xn--xn--rsted-o8a.dk
xn--1	orsted.dk	xn--orsted.dk	xn--orsted.dk

I think that when adding the logic to registry/txt.go, it should also include some sane constraints on --txt-prefix and --txt-suffix with the most simple being something like no unicode and no double dash in the params.

So, a "finished" implementation™ for IDNA awareness in External DNS should, IMHO, also handle whether a provider might normalize the domains themselves and then return either the Punycode, Unicode or the raw string used to create it, which could be a mix of the two² at their discretion. To help with testing this, I would like the InMemory provider to support these three different behaviours so that they can be used in tests.

I don't mind making all of this now that I am already invested in the task, but I would like that when the changelog says, "IDNA awareness now brings Unicode support", it is a thoroughly tested implementation.

¹ I can't see a reason why anyone would prefix with xn--, but the potential to shoot oneself in the foot this bad feels like something that should be blocked so that we don't end up with something like this idna.ToUnicode("xn--orsted.dk") -> 夹夺夃.dk (I hope this random string doesn't mean anything rude)

² Example morsø.xn--rsted-uua.dk

[mloiseleur]

I think that when adding the logic to registry/txt.go, it should also include some sane constraints on --txt-prefix and --txt-suffix with the most simple being something like no unicode and no double dash in the params.

That sounds reasonable, yes :+1: .

So, a "finished" implementation™ for IDNA awareness in External DNS should, IMHO, also handle whether a provider might normalize the domains themselves and then return either the Punycode, Unicode or the raw string used to create it, which could be a mix of the two at their discretion. To help with testing this, I would like the InMemory provider to support these three different behaviours so that they can be used in tests.

:thinking: Why would we want to enter in such a messy and complex world ? External Dns is maintained by very few people on their spare time, so we clearly need something as simple as possible. What about a switch like --enable-idna, which would enable or disable this feature ? Disabled by default to start, and enabled by default when most providers support it ?

I don't mind making all of this now that I am already invested in the task, but I would like that when the changelog says, "IDNA awareness now brings Unicode support", it is a thoroughly tested implementation.

Yes, sure :100: !

[kimsondrup]

🤔 Why would we want to enter in such a messy and complex world ? External Dns is maintained by very few people on their spare time, so we clearly need something as simple as possible. What about a switch like --enable-idna, which would enable or disable this feature ? Disabled by default to start, and enabled by default when most providers support it ?

Great point! Perhaps a --enable-idna flag could be combined with something like how SupportedRecordType() bool is implemented so that providers could implement SupportIDNA() bool to tell External DNS if IDNA is supported without the users of External DNS having to think about it?

[mloiseleur]

Perhaps a --enable-idna flag could be combined with something like how SupportedRecordType() bool is implemented so that providers could implement SupportIDNA() bool to tell External DNS if IDNA is supported without the users of External DNS having to think about it?

Following KISS principle, I suggest to just add the feature in this PR and provide this additional feature to switch default in an other PR. You'll need to design it also for webhook providers.

[kimsondrup]

@mloiseleur I would like to remove the changes made to the Plan logic from this PR and only leave in the DomainFilter changes.

The reason is that I am already prototyping the larger change with the switch and, in the process, realized that DomainFilter is used all over the place in both External DNS and for external providers. So, I can't see a good way to make a switch global without using an environment variable, which I'm not much for.

So my suggestion would be to let DomainFilter be IDNA aware by default with this PR, focusing on the best effort where encoding problems are logged but the domain is still processed. Then, in a separate PR, the logic for an --idna-enabled will be added, and one of the changes will enforce that the domain must be valid for IDNA encoded/decoded.

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

/lgtm /assign @Raffo

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

[ivankatliarchuk]

Hi @kimsondrup are you ok to rebase?

[mloiseleur]

/close Replaced by #5049

[k8s-ci-robot]

@mloiseleur: Closed this PR.

In response to this:

/close Replaced by #5049

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.