AWS R53 "A" alias record incorrectly uses TXT record with "cname-" prefix

[artem-nefedov]

What happened:

When A alias record is created by external-dns, it also creates two TXT records, one of which contains "cname-" prefix, despite documentation on new registry format stating that it should be based on record type, and TXT records related to A records should have "a-" prefix.

What you expected to happen:

TXT record having "a-" prefix.

How to reproduce it (as minimally and precisely as possible):

1. Run external-dns in the cluster with options:

- --source=service
- --registry=txt
- --txt-owner-id=some-owner-id
- --txt-prefix=myprefix-
- --provider=aws
- --aws-zone-type=public

2. Deploy ExternalName Service pointing to the hostname of an existing AWS Load Balancer (required to create A alias, otherwise CNAME will be created):

apiVersion: v1
kind: Service
metadata:
  name: my-svc
  annotations:
    external-dns.alpha.kubernetes.io/hostname: foo.bar.com
spec:
  type: ExternalName
  externalName: foo-bar-00000000.us-west-2.elb.amazonaws.com # Hostname of ALB

Anything else we need to know?:

Logs from the creation process look like this:

level=info msg="Desired change: CREATE foo.bar.com A [Id: /hostedzone/<redacted>]"
level=info msg="Desired change: CREATE myprefix-foo.bar.com TXT [Id: /hostedzone/<redacted>]"
level=info msg="Desired change: CREATE myprefix-cname-foo.bar.com TXT [Id: /hostedzone/<redacted>]"

Environment:

• External-DNS version: v0.12.2
• DNS provider: AWS Route53

[artem-nefedov]

Also, one thing that's not clear to me is why it even adds this record type prefix when I already specified my own prefix without %{record_type}. I think that's also a bug?

[rivetmichael]

I noticed the same bug here with version 0.12.2 and AWS Route53 as well.

[benjimin]

So, if you want a DNS record to point at a load balancer, you can put it in an A(-ddress) record rather than a CNAME record, and route53 will automatically resolve (alias) the IP for clients. And it is also cheaper.

https://docs.aws.amazon.com/Route53/latest/DeveloperGuide/resource-record-sets-choosing-alias-non-alias.html

Still seems like a bug, how the new-style TXT records are currently treating it.

It looks like much of this aliasing logic is in the provider rather than the common parts of external-dns (despite that the feature is also supported by non-AWS providers such as Azure)?

[artem-nefedov]

@benjimin Yes, it creates A "alias" record. That part is normal and working as expected. The problem is that it appends "cname" to TXT record, despite alias actually being A record type.

[benjimin]

Unfortunately, I think this problem is caused by poor modularisation of the external-dns internals.

The program main loop consists of repetitions of controller.Controller.RunOnce. This uses the source subpackage to gather what rules are desired, and uses the TXT subclass of the registry subpackage to ascertain what rules already exist on the DNS server and which ones external-dns has responsibility for managing. (To accomplish this, the registry wraps the provider interface.) The controller then uses the plan subpackage to compare these two lists to decide what updates to perform, and has the registry action the result.

The problem is that the source object is (inappropriately) opinionated about what types of DNS records should be used to represent each desired rule. So type CNAME is already specified (solely based on the target not resembling an IP address) before the rule even gets suggested to the controller or registry. This is why, when TXTRegistry injects its TXT records, it labels them "CNAME".

Alias records are supported by multiple cloud providers (as they make the DNS system more efficient), but unfortunately the aliasing logic in external-dns is applied transparently at the last moment by the AWS provider. (The conversion is applied in AWSProvider.ApplyChanges and removed again in AWSProvider.Records.) Not only does it swap CNAME records for A records (depending on whether the target is something AWS supports DNS aliasing for) but it may additionally create AAAA records (for IPv6 support). The registry has no knowledge that this transformation has occurred, and the AWSProvider has no knowledge that the registry subclass may have intended the TXT records to have a relationship to those modified records.

To properly solve this problem, I think a major refactor of external-dns is appropriate (bringing aliasing logic outside of the provider subpackage, possibly leading to changes in how the controller interacts with registries), which is difficult because there are so many different providers and components at risk of being impacted. You could just do a bandaid fix (like duplicating some of the TXTRegistry logic inside of the AWSProvider), but I think this is a bad idea (making the entire codebase more spaghetti complicated and harder to refactor in the future), although there already appears to be a history of such bandaids (for example some of the TXTRegistry logic has leaked into the main controller loop and hence also into the interface for other kinds of registry, namely the missing records interface which is solely for upgrades/downgrades of the format used by TXTRegistry and is handled separately from all other kinds of missing DNS records, aside from that it misleadingly pretends to be an independent method call rather than an extra return value...).

Incidentally, when (A and/or AAAA) aliasing is in use, it might also be possible for MX and SRV records to exist for the same hostname (unlike if a CNAME record were used), which could further interfere with the ability of the TXTRegistry to judge which records external-dns can safely manage at present.

[artem-nefedov]

@benjimin Thanks for the in-depth analysis. I guess we'll just have to accept that it works like this for now.

Can you also clarify if the fact that it still adds "cname-" prefix when I specify my own txt prefix without %{record_type} in it is a bug, or am I missing something here?
(e.g. --txt-prefix=myprefix- leads to TXT record "myprefix-cname-foo.bar.com")

[benjimin]

Pretty sure that last part is the intended behaviour (so that different DNS record types for the same hostname could in-principle be managed independently without conflict).

[artem-nefedov]

@benjimin However, this document says:

Prefix and suffix are extended with %{record_type} template where the user can control how prefixed/suffixed records should look like.

I would assume that would mean that I decide whether to include record type in prefix or not when specifying "--txt-prefix", and if %{record_type} isn't there it shouldn't include record type. At least that's what I gathered from this sentence.

[benjimin]

@artem-nefedov maybe raise that as a separate issue?

[jcogilvie]

I think this is a dupe of #2903.

[k8s-triage-robot]

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

This bot triages un-triaged issues 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 issue is closed

You can:

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

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

/lifecycle stale

[jcogilvie]

/remove-lifecycle stale

…

On Tue, May 9, 2023, 3:12 PM Kubernetes Triage Robot < ***@***.***> wrote: The Kubernetes project currently lacks enough contributors to adequately respond to all issues. This bot triages un-triaged issues 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 issue is closed You can: - Mark this issue as fresh with /remove-lifecycle stale - Close this issue with /close - Offer to help out with Issue Triage <https://www.kubernetes.dev/docs/guide/issue-triage/> Please send feedback to sig-contributor-experience at kubernetes/community <https://github.com/kubernetes/community>. /lifecycle stale — Reply to this email directly, view it on GitHub <#3164 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFF3ALRLL254HFSIPW2O73XFKJLNANCNFSM6AAAAAASCOLZ5I> . You are receiving this because you commented.Message ID: ***@***.***>

[k8s-triage-robot]

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

This bot triages un-triaged issues 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 issue is closed

You can:

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

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

/lifecycle rotten

[jcogilvie]

/remove-lifecycle rotten

[johngmyers]

Duplicate of #2903
/close

[k8s-ci-robot]

@johngmyers: Closing this issue.

In response to this:

Duplicate of #2903
/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/test-infra repository.