fix(aws): incorrect behavior for non-aliasable record types

[u-kai]

What does it do?

This PR fixes incorrect behavior for record types that do not support AWS Alias records (e.g., MX, TXT, SRV, etc.).

Motivation

This work originated from the discussion here:
#5997 (comment)

When a ProviderSpecific property alias=true exists on record types that don't support alias records
(like MX records):

• Alias processing continues even after alias property deletion
  • The alias property is deleted at aws.go, but the alias variable remains true
  • This results in unnecessary evaluateTargetHealth=false being added
• TTL value gets unintentionally modified
  • When RecordTTL is configured, it gets fixed to 300

Reproduction

ep := &endpoint.Endpoint{
    RecordType: endpoint.RecordTypeMX,
    RecordTTL:  600,
    ProviderSpecific: endpoint.ProviderSpecific{
        {Name: "alias", Value: "true"},
    },
}
// Expected: TTL=600, ProviderSpecific=empty
// But result is: TTL=300, evaluateTargetHealth=false gets added

While this affects cases with incorrectly configured ProviderSpecific properties, I believe the behavior should

More

• Yes, this PR title follows Conventional Commits
• Yes, I added unit tests
• Yes, I updated end user documentation accordingly

[k8s-ci-robot]

Hi @u-kai. Thanks for your PR.

I'm waiting for a github.com 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.

Details

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]

Refactoring and bug fix in single PR. Too risky. As well as for bug, it should be manifests provided to reproduce

[u-kai]

@ivankatliarchuk
Sorry about that — I’ll be more careful next time.

To make the existing issue easier to fix, I’ve opened a separate PR with the refactoring work.
Once that PR is merged, I will rebase this PR and continue the bug fix on top of it.

It would be great if you could review the refactoring PR first. Thanks!

[u-kai]

@ivankatliarchuk
Now that the refactoring PR has been merged, I’ve rebased and updated this PR so it’s ready for review again.

[ivankatliarchuk]

Missing links to docs in description with facts about this behaviour. Missing end to end tests with the proof.

I'll review only after other reviewers share their review. I could not keep approving on my own.

[ivankatliarchuk]

There’s no example showing the current behaviour, so it’s unclear whether this is actually a problem or not.

[u-kai]

I’ve updated the PR description.

Previously, even for record types that do not support alias (such as MX records), ExternalDNS was applying alias-specific behavior — updating TTL and adding the evaluateTargetHealth property — whenever alias was set to true.

This was incorrect and also hurt code readability.
With this change, MX records no longer have their TTL modified even if alias is true, and alias-related properties are removed for record types that do not support alias.

[ivankatliarchuk]

Missing before and after real example. And missing links to official documentation describing AWS alias record behavior

[ivankatliarchuk]

not sure about ep.RecordType == "SOA"

[coveralls]

Pull Request Test Coverage Report for Build 23925061512

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 198 unchanged lines in 4 files lost coverage.
• Overall coverage increased (+0.04%) to 80.517%

---

Files with Coverage Reduction	New Missed Lines	%
aws/aws.go	23	89.07%
store.go	37	65.08%
azure/azure.go	58	76.0%
gateway.go	80	86.53%

Totals
Change from base Build 23734434195:	0.04%
Covered Lines:	17147
Relevant Lines:	21296

---

💛 - Coveralls

[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 ivankatliarchuk. For more information see the Code Review Process.

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

Details

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

[u-kai]

As noted in the code comments, AWS Route53 allows Alias records for record types other than NS and SOA.
However, the ExternalDNS documentation mainly describes replacing CNAME records with Alias records.

From a practical perspective, there is little real-world value in using Alias records for types other than A and AAAA. In fact, the current code already does not accept Alias records for MX during validation. While the concept of “Alias” exists across multiple providers, its semantics differ significantly.

For example, GCP Cloud DNS ALIAS records are specifically designed to synthesize A/AAAA records at the zone apex and are not intended for use with MX or other non-address record types.

To keep the Alias behavior simple and consistent across providers, this change removes the Alias property
when it is set on record types other than A and AAAA.

Previously, when non-CNAME records had the Alias property set, ExternalDNS applied Alias-specific settings
(such as default TTL and evaluateTargetHealth) and then removed the Alias property.
With this change, we instead remove the Alias property and related fields (including evaluateTargetHealth)
entirely.

This helps avoid ambiguous behavior and keeps the implementation aligned with practical use cases.

[u-kai]

Here is a concrete example to illustrate the behavior.

Given the following DNSEndpoint:

apiVersion: externaldns.k8s.io/v1alpha1
kind: DNSEndpoint
metadata:
  name: alias
  namespace: default
spec:
  endpoints:
    - dnsName: mail.example.com
      recordType: MX
      recordTTL: 600
      targets:
        - "10 mail1.example.com."
      providerSpecific:
        - name: "alias"
          value: "true"

With the previous behavior, when this was applied:

• The TTL was automatically rewritten to 300 (the default for Alias records).
• The evaluateTargetHealth property was added to the endpoint.

As a result, ExternalDNS detected a diff on every sync and continuously issued UPDATE requests, as shown in the logs, because alias-related properties were dropped during request generation for MX records.

INFO[0075] Desired change: UPSERT mx-mail.example.com TXT  profile=default zoneID=/hostedzone/ZXXXXXXXXXXXXX zoneName=example.com.
INFO[0075] Desired change: UPSERT test.mail.example.com MX  profile=default zoneID=/hostedzone/ZXXXXXXXXXXXXX zoneName=example.com.
INFO[0076] 2 record(s) were successfully updated         profile=default zoneID=/hostedzone/ZXXXXXXXXXXXXX zoneName=example.com.
...
INFO[0086] Desired change: UPSERT mx-mail.example.com TXT  profile=default zoneID=/hostedzone/ZXXXXXXXXXXXXX zoneName=example.com.
INFO[0086] Desired change: UPSERT mail.example.com MX  profile=default zoneID=/hostedzone/ZXXXXXXXXXXXXX zoneName=example.com.
INFO[0087] 2 record(s) were successfully updated         profile=default zoneID=/hostedzone/ZXXXXXXXXXXXXX zoneName=example.com.

With the current change:

• The TTL remains as the user-specified value (600 in this example).
• No unnecessary alias-related properties are injected.

As a result, after the initial sync, no further UPDATE operations are triggered, and the record remains stable.

This avoids unnecessary reconciliation loops and keeps the behavior consistent with what the provider actually supports.

INFO[0001] Desired change: UPSERT mx-mail.example.com TXT  profile=default zoneID=/hostedzone/ZXXXXXXXXXXXXXX zoneName=example.com.
INFO[0001] Desired change: UPSERT mail.example.com MX  profile=default zoneID=/hostedzone/ZXXXXXXXXXXXXXX zoneName=example.com.
INFO[0002] 2 record(s) were successfully updated         profile=default zoneID=/hostedzone/ZXXXXXXXXXXXXXX zoneName=example.com.
...
INFO[0011] All records are already up to date

[ivankatliarchuk]

I gotcha now.

What’s bothering me isn’t the mechanics of the fix, it’s where the responsibility boundary is being moved. Right now this PR quietly shifts user configuraiont error handling into the AWS provider, and that’s a design smell.

An MX record with alias=true is invalid in ExternalDNS or DNS terms, regardless of what Route53 might technically support.

If the input is invalid:

• it should be rejected earlier
• it should fail validation

  external-dns/source/wrappers/dedupsource.go

  Line 56 in d38daef

  if ok := ep.CheckEndpoint(); !ok {

Silently “fixing” it in the provider:

• hides user mistakes
• creates provider-specific behavior
• makes debugging harder
• encourages cargo-cult annotations

ExternalDNS already has clear layering:

Sources → Endpoints → Wrappers Validation/Normalization → Providers

The provider’s job is:

• translate valid, normalized endpoints into provider API calls

Not:

• reinterpret semantics
• drop fields conditionally
• "guess" user intent

This PR does exactly that:

• it accepts invalid input
• then mutates it deep in the AWS provider
• instead of rejecting it earlier

That’s backwards.

[ivankatliarchuk]

Now:

• AWS provider silently “fixes” invalid alias usage
• Other providers may not
• Behavior diverges across providers

ExternalDNS should enforce provider-agnostic semantics before provider code runs.

[ivankatliarchuk]

Ideally, what we should have

Reject invalid combinations at middleware layer:

• alias=true only allowed for A / AAAA
• fail endpoint validation
• surface a clear debug msg

This is the cleanest contract.

[u-kai]

Totally agree — the issue here is the responsibility boundary.

To move in that direction, I opened a follow-up PR that makes endpoint validation fail when alias=true is used for non A/AAAA,CNAME record types. Once that PR is merged, this PR can be simplified further.

I’ll update this PR accordingly once the validation change lands.

[ivankatliarchuk]

Is this resolved or still a problem?

[u-kai]

Sorry, it has already been resolved.

[ivankatliarchuk]

Sorry, it has already been resolved.

/close

[k8s-ci-robot]

@ivankatliarchuk: Closed this PR.

Details

In response to this:

Sorry, it has already been resolved.

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

[u-kai]

Sorry for the confusion. "Resolved" was referring to the code review comment being addressed, not the PR itself being resolved. I'll reopen this.

[u-kai]

/reopen

[k8s-ci-robot]

@u-kai: Reopened this PR.

Details

In response to this:

/reopen

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]

Ok

[ivankatliarchuk]

I think the change missing some links in PR title + debug logs or something.

High level. We have a gap - no end 2 end test that stitches together CheckEndpoint → dedupSource → AdjustEndpoints to show the full rejection path. The two unit tests exist, but their composition is not visible in unit tests.

[ivankatliarchuk]

Are we cleaning up the providerSpecificAlias property in wrapper layer? This was just added in recent PR https://github.com/kubernetes-sigs/external-dns/pull/6021/changes

[u-kai]

I've separated this into the PR below:
#6342

[ivankatliarchuk]

I'm not sure. Maybe instead of removing test, actually provide a current code behaviour with changed name?

[u-kai]

You're right that this case can still exist from a unit test perspective.
However, in the current design, it's difficult for an alias record to be used with MX, since it's prevented at another layer, so I removed the test.

What kind of test name would you suggest if we keep it?