feat(aws): support alias=A and alias=AAAA to create single record type#5997
Conversation
…r-specific options) Signed-off-by: u-kai <76635578+u-kai@users.noreply.github.com>
|
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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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 thanks for this interesting PR. Wdyt of re-use existing annotation, It could be configurable like this :
|
|
@mloiseleur |
|
/ok-to-test |
Coverage Report for CI Build 25616170037Coverage increased (+0.01%) to 80.596%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions20 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
| return endpoints, nil | ||
| } | ||
|
|
||
| func aliasDisableARecord(ep *endpoint.Endpoint) bool { |
There was a problem hiding this comment.
My opinion, we will benefit to have a generic,centralised method on Endpoint object for all annotations that contains booleans like true/false
There was a problem hiding this comment.
something like
func (ep *endpoint.Endpoint) GetProviderSpecificBool(key string bool) bool {
val, ok := ep.GetProviderSpecificProperty(key)
if !ok {
return false
}
// Normalize whitespace and case; accept common truthy values
v := strings.TrimSpace(strings.ToLower(val))
switch v {
case "1", "t", "true", "yes", "y":
return true
case "0", "f", "false", "no", "n":
return false
default:
return false
}
}
There was a problem hiding this comment.
That does sound like a good idea.
However, in this change we’re going to use values other than just true/false, as @mloiseleur mentioned, and I’m a bit concerned that introducing a generic, centralized method here would make the scope of this PR too large.
I’d prefer to handle that refactor in a separate PR that can apply the change across the whole codebase. What do you think?
…ewAaaaIfNeeded Signed-off-by: u-kai <76635578+u-kai@users.noreply.github.com>
|
While creating comprehensive tests for supporting annotations like Should I include this fix in this PR, or would it be better to submit it as a separate PR? Discovered IssueWhen a ProviderSpecific property
Reproductionep := &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 be consistent. |
It's better as a separate PR. It's easier to review & test this way. |
|
I’ve submitted the fix in a separate PR here: #6017. |
|
Refactoring, bug fixes, new features - all should be in they own PRs. We are trying to de-resk releases. Way too many issues opened ;-) |
There was a problem hiding this comment.
No update to:
- docs/tutorials/aws.md
- docs/examples/aws.md
This should include:
New provider-specific annotations:
external-dns.alpha.kubernetes.io/aws-alias-disable-a: "true"
external-dns.alpha.kubernetes.io/aws-alias-disable-aaaa: "true"
or
external-dns.alpha.kubernetes.io/alias-disable-a: "true"
external-dns.alpha.kubernetes.io/alias-disable-aaaa: "true"And more important, what are the use cases, as at the moment this is just added annotation for no reasons
| } | ||
| if enableAandAAAA { | ||
| // Add a new endpoint for the AAAA record | ||
| aliasCnameAaaaEndpoints = append(aliasCnameAaaaEndpoints, &endpoint.Endpoint{ |
There was a problem hiding this comment.
Make sure not to use &endpoint.Endpoint{}, but methods available in the package. This will create tech debt for us.
There was a problem hiding this comment.
I'd say it should be clone or similar function in Endpoint package, to make sure we do not mutate original
|
|
||
| func aliasDisableARecord(ep *endpoint.Endpoint) bool { | ||
| disable, ok := ep.GetProviderSpecificProperty(providerSpecificAliasDisableA) | ||
| return ok && disable == "true" |
There was a problem hiding this comment.
Use constants instead of strings for "true".
| disableAlias := disableA && disableAaaa | ||
| enableAandAAAA := !disableA && !disableAaaa | ||
|
|
||
| if ep.RecordType == endpoint.RecordTypeCNAME && !disableAlias { |
There was a problem hiding this comment.
Something does not sound here. This most likely could and should be a method on it's own, so we could add tests just for this method
something like
log.Debugf("Modifying endpoint: %v, changing record type from %s to %s", ep, ....)
if ep.RecordType == endpoint.RecordTypeCNAME && strings.CutPrefix(k, disableAliasPrefix) {
epModified = .....modify me...
or
epC = ep.DeepCopy()
epC.RecordType = modifyMe(......)
}
|
I'm unsure how big is the problem. At the moment this change affects only AWS. It could be that same behaviour is relevant for other providers as well. In such case we could have annotation without a provider prefix, and modify type in post processor wrapper https://github.com/kubernetes-sigs/external-dns/blob/master/source/wrappers/post_processor.go |
I'm not sure about 4 state solution, need to double check. Same time this feets wrapper case, so it should not be in |
|
Would be nice to get some sort of mermaid diagram https://mermaid.live/edit or similar before and after change behavoir hightlight. |
|
My understanding is that alias itself is an AWS-specific feature. Given that, I think it’s worth taking a step back and clarifying |
|
I spent some time reading through the codebase, and my current understanding is:
Given these trade-offs, I think it’s not obvious what the right approach is here yet. To make the trade-offs more explicit, here is how I see the current vs desired dependency graph: Current flowchart LR
EndpointPkg["endpoint package
(provider-agnostic)"]
TxtRegistry["registry/txt
(TXT Registry)"]
AwsProvider["provider/aws
(AWS provider)"]
ProviderIface["provider interface"]
%% code dependencies (imports)
TxtRegistry -->|depends on| EndpointPkg
TxtRegistry -->|depends on| ProviderIface
AwsProvider -->|depends on| ProviderIface
AwsProvider -->|depends on| EndpointPkg
%% problematic knowledge leak
TxtRegistry -->|knows AWS alias semantics| AwsProvider
%% emphasis
TxtRegistry:::problem
My desired flowchart LR
EndpointPkg["endpoint package
(provider-agnostic)"]
TxtRegistry["registry/txt
(TXT Registry)"]
AwsProvider["provider/aws
(AWS provider)"]
ProviderIface["provider interface"]
%% code dependencies (imports)
TxtRegistry -->|depends on| EndpointPkg
TxtRegistry -->|depends on| ProviderIface
AwsProvider -->|depends on| ProviderIface
AwsProvider -->|depends on| EndpointPkg
|
|
Personally, even if it takes time, I’d like to focus on eventually removing |
Signed-off-by: u-kai <76635578+u-kai@users.noreply.github.com>
Signed-off-by: u-kai <76635578+u-kai@users.noreply.github.com>
|
I introduced Today, alias semantics are effectively only implemented by the AWS provider, and other providers have very different behaviors and constraints. Based on the provider docs, trying to centralize too much alias logic in So the Endpoint only carries the minimal, provider-agnostic intent, and each provider is responsible for mapping that intent to its own record model. |
|
I have similar concerns here as in the previous PR #6017 While the implementation itself looks reasonable, it feels like we’re again compensating at the provider layer for situations that should ideally be handled earlier. Carrying this logic into the provider risks increasing hidden behavior and makes it harder to reason about correctness across providers. As a suggestion, it may be worth revisiting whether these cases could be normalized before provider-specific processing, rather than teaching each provider how to interpret this annotations. Basically ExternalDNS should enforce provider-agnostic semantics before provider code runs. The proposed responsibility is wrong from my view and does not scale. |
|
We don't have specific |
|
If this contributing docs missing designs https://github.com/kubernetes-sigs/external-dns/blob/master/docs/contributing/source-wrappers.md worth to update them as well. |
| If the value of this annotation is `true`, specifies that CNAME records generated by the | ||
| resource should instead be alias records. | ||
|
|
||
| Additionally, you can set the value to `A` or `AAAA` to create only one type of alias record: |
There was a problem hiding this comment.
Missing full example, hard to understand how to configure it.
example
apiVersion: v1
kind: RESOURCE
metadata:
name: app-assets-dns
namespace: default
annotations:
xternal-dns.alpha.kubernetes.io/ABRAKADABRA: ABRAKADABRA
# Result:
# DNS -> TARGET (CNAME)
Signed-off-by: u-kai <76635578+u-kai@users.noreply.github.com>
304115a to
c65b5f5
Compare
|
Validation of invalid combinations is being handled separately in the CheckEndpoint PR. Given that, I’m not entirely sure what additional responsibility we should move into the common layer in this PR. From my perspective, this change is intentionally kept minimal and only exposes the intent, while leaving provider-specific interpretation to each provider. If there are specific cases you think should still be normalized earlier, I’d be happy to discuss them. |
|
Is this supported/not supported? |
d7de208 to
33fea72
Compare
|
Rebased onto the latest master. The implementation is done — the issue will be resolved once this PR is merged. |
| func shouldUseCNAMEForTxtRecord(ep *endpoint.Endpoint) bool { | ||
| aliasType := ep.GetAliasProperty() | ||
| return (aliasType == endpoint.AliasTrue || aliasType == endpoint.AliasA) && ep.RecordType == endpoint.RecordTypeA | ||
| } |
There was a problem hiding this comment.
@u-kai Thanks for the rebase. You have addressed many comments 👍 .
I have still one, though: What should be the behavior of this func when aliasType is endpoint.AliasAAAA ?
There was a problem hiding this comment.
@mloiseleur
Thanks for the comments!
AliasAAAA doesn't need any special handling in shouldUseCNAMEForTxtRecord.
The CNAME encoding for alias A records may exist for backward compatibility: before #3910, alias records were represented as RecordTypeCNAME in external-dns.
When that PR changed them to RecordTypeA, the TXT ownership records had to keep using "cname" as the encoded type.
The alias AAAA record was never represented as CNAME, so its TXT record should use "AAAA" as the record type.
|
/lgtm |
|
I'm on the fence. This functionality is not AWS specific. Should not be in aws provider |
|
The alias intent (annotation parsing, preferAlias, AliasType definition) is already centralized in the common layer. But each provider handles that intent differently — for example, Is there a specific part you think should be moved earlier that we're missing? |
|
ok. let's have a look /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ivankatliarchuk The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What does it do ?
Fixes: #5815
This PR extends the existing
aliasannotation to support selective control over A and AAAA alias record creation:alias: "A"- creates only an A ALIAS record (IPv4 only)alias: "AAAA"- creates only an AAAA ALIAS record (IPv6 only)When a CNAME endpoint creates alias records by aws provider, external-dns automatically creates both A and AAAA alias records by default. These new values allow users to create only a specific record type on a per-endpoint basis.
Motivation
Issue #5815 requested the ability to selectively disable AAAA alias records for specific use cases. Currently, the only option is the global
--exclude-record-types=AAAAflag, which affects all records across all providers.This feature is particularly important for same-zone alias records where CNAME endpoints point to other records within the same hosted zone. While AWS managed services (like ELBs, CloudFront) typically support both IPv4 and IPv6, custom applications and services within the same zone often have different connectivity requirements.
Unlike AWS managed resources which are generally dual-stack capable, user-managed records in the same zone frequently have single-stack limitations, making this granular control essential for proper DNS configuration.
This enhancement provides the targeted control needed for these scenarios while maintaining full backward compatibility.
More