Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
u-kai wants to merge 14 commits into
base: master
Choose a base branch
from
+2 −43
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
7fda240
test: add regression test to ensure behavior before refactor
u-kai Dec 10, 2025
b50b0a5
refactor aws adjustEndpointAndAaaaIfNeeded
u-kai Dec 10, 2025
94dd0d3
fix clean up alias properties for unsupported record types
u-kai Dec 11, 2025
881e5c4
test add comprehensive tests and remove old logic
u-kai Dec 11, 2025
7be5ded
Merge branch 'master' of https://github.com/kubernetes-sigs/external-…
u-kai Jan 28, 2026
1ff3f5d
fix: adjustOtherRecord
u-kai Jan 28, 2026
9bb249b
fix(aws): support alias records for non-A/AAAA/CNAME types except NS …
u-kai Feb 1, 2026
ef7b98a
refactor(aws): simplify adjustOtherRecord with clarifying comment
u-kai Feb 7, 2026
a01ce04
Merge branch 'master' of https://github.com/kubernetes-sigs/external-…
u-kai Mar 1, 2026
b4b27fd
chore(aws): remove unnecessary adjustRecord method and its tests
u-kai Mar 1, 2026
5d588c1
Merge branch 'master' of https://github.com/kubernetes-sigs/external-…
u-kai Apr 2, 2026
ba3279d
fix typo provider/aws/aws_test.go
u-kai Apr 2, 2026
67f0f5a
Merge branch 'master' of https://github.com/kubernetes-sigs/external-…
u-kai Apr 22, 2026
d67f8fe
Merge branch 'fix/aws-alias-logic' of https://github.com/u-kai/extern…
u-kai Apr 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 0 additions & 14 deletions provider/aws/aws.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -865,8 +865,6 @@ func (p *AWSProvider) adjustEndpointAndNewAaaaIfNeeded(ep *endpoint.Endpoint) *e
aaaa.RecordType = endpoint.RecordTypeAAAA
}
return aaaa
default:
p.adjustOtherRecord(ep)
}
adjustGeoProximityLocationEndpoint(ep)
return aaaa
Expand Down Expand Up @@ -923,18 +921,6 @@ func (p *AWSProvider) adjustCNAMERecord(ep *endpoint.Endpoint) {
}
}

func (p *AWSProvider) adjustOtherRecord(ep *endpoint.Endpoint) {
// TODO: fix For records other than A, AAAA, and CNAME, if an alias record is set, the alias record processing is not performed.
// This will be fixed in another PR.
if isAlias, _ := ep.GetBoolProviderSpecificProperty(endpoint.ProviderSpecificAlias); isAlias {
p.adjustAliasRecord(ep)
ep.DeleteProviderSpecificProperty(endpoint.ProviderSpecificAlias)
} else {
ep.DeleteProviderSpecificProperty(endpoint.ProviderSpecificAlias)
ep.DeleteProviderSpecificProperty(providerSpecificEvaluateTargetHealth)
}
}

// if the endpoint is using geoproximity, set the bias to 0 if not set
// this is needed to avoid unnecessary Upserts if the desired endpoint doesn't specify a bias
func adjustGeoProximityLocationEndpoint(ep *endpoint.Endpoint) {
Expand Down
31 changes: 2 additions & 29 deletions provider/aws/aws_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3324,35 +3324,8 @@ func TestAWSProvider_adjustEndpointAndNewAaaaIfNeeded(t *testing.T) {
},
expectedAaaa: nil,
},
// TODO: fix For records other than A, AAAA, and CNAME, if an alias record is set, the alias record processing is not performed. This will be fixed in another PR.
{
Copy link
Copy Markdown
Member

@ivankatliarchuk ivankatliarchuk Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@u-kai u-kai Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

name: "MX record with alias=true should remove alias and set default ttl, add evaluateTargetHealth and not create AAAA",
ep: &endpoint.Endpoint{
DNSName: "test.foo.bar.",
RecordType: endpoint.RecordTypeMX,
Targets: endpoint.Targets{"10 mail.example.com."},
RecordTTL: 600,
ProviderSpecific: endpoint.ProviderSpecific{
{
Name: endpoint.ProviderSpecificAlias,
Value: "true",
},
},
},
expected: &endpoint.Endpoint{
DNSName: "test.foo.bar.",
RecordType: endpoint.RecordTypeMX,
Targets: endpoint.Targets{"10 mail.example.com."},
RecordTTL: defaultTTL,
ProviderSpecific: endpoint.ProviderSpecific{
{
Name: providerSpecificEvaluateTargetHealth,
Value: "false",
},
},
},
expectedAaaa: nil,
},
// Other record types that has alias properties should be rejected by endpoint validation,
// so we don't need to test them here as adjustEndpointAndNewAaaaIfNeeded should not be called for them.
}

for _, tt := range tests {
Expand Down
Loading