Skip to content

feat(endpoint): reject alias property on unsupported record types #6188

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

Merged
k8s-ci-robot merged 7 commits into from
Feb 28, 2026
+238 −13
Merged

feat(endpoint): reject alias property on unsupported record types #6188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1d536e5
feat(endpoint): reject alias property on unsupported record types
u-kai Feb 11, 2026
520cf59
refactor(endpoint): rename supportAlias to supportsAlias and extract …
u-kai Feb 15, 2026
c6c1eae
test(endpoint): add alias warning log assertion cases
u-kai Feb 15, 2026
d596421
test(wrappers): add alias cases to dedup validation and warn tests
u-kai Feb 15, 2026
c8db779
refactor(endpoint): use providerSpecificAlias constant in tests
u-kai Feb 19, 2026
825031c
docs(annotations): note alias annotation is limited to A, AAAA, and C…
u-kai Feb 19, 2026
0c335bb
Merge branch 'master' of https://github.com/kubernetes-sigs/external-…
u-kai Feb 28, 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
3 changes: 3 additions & 0 deletions docs/annotations/annotations.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,9 @@ Additional annotations implemented by specific providers:
If the value of this annotation is `true`, specifies that CNAME records generated by the
resource should instead be alias records.

This annotation is only supported on A, AAAA, and CNAME record types. Endpoints with other
record types (e.g. MX, SRV, TXT) that have this annotation set will be rejected.

**Supported providers:**

- **AWS**: This annotation is only relevant if the `--aws-prefer-cname` flag is specified.
Expand Down
20 changes: 20 additions & 0 deletions endpoint/endpoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,9 +455,20 @@ func RemoveDuplicates(endpoints []*Endpoint) []*Endpoint {
return result
}

// TODO: review source/annotations package to consolidate alias key definitions;
// currently duplicated here to avoid circular dependency.
const providerSpecificAlias = "alias"
Copy link
Copy Markdown
Member

@ivankatliarchuk ivankatliarchuk Feb 15, 2026

Choose a reason for hiding this comment

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

most likely too long, but not an issue. You not using this constant in tests

Copy link
Copy Markdown
Member Author

@u-kai u-kai Feb 19, 2026

Choose a reason for hiding this comment

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

I've fixed it.


// TODO: rename to Validate
// CheckEndpoint Check if endpoint is properly formatted according to RFC standards
func (e *Endpoint) CheckEndpoint() bool {
if !e.supportsAlias() {
if _, ok := e.GetBoolProviderSpecificProperty(providerSpecificAlias); ok {
log.Warnf("Endpoint %s of type %s does not support alias records", e.DNSName, e.RecordType)
return false
}
}

switch recordType := e.RecordType; recordType {
case RecordTypeMX:
return e.Targets.ValidateMXRecord()
Expand All @@ -467,6 +478,15 @@ func (e *Endpoint) CheckEndpoint() bool {
return true
}

func (e *Endpoint) supportsAlias() bool {
switch e.RecordType {
case RecordTypeA, RecordTypeAAAA, RecordTypeCNAME:
return true
default:
return false
}
}

// WithMinTTL sets the endpoint's TTL to the given value if the current TTL is not configured.
func (e *Endpoint) WithMinTTL(ttl int64) {
if !e.RecordTTL.IsConfigured() && ttl > 0 {
Expand Down
155 changes: 155 additions & 0 deletions endpoint/endpoint_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,11 @@ package endpoint
import (
"fmt"
"reflect"
"strings"
"testing"

log "github.com/sirupsen/logrus"
"github.com/sirupsen/logrus/hooks/test"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"sigs.k8s.io/external-dns/pkg/events"
Expand Down Expand Up @@ -939,6 +942,95 @@ func TestCheckEndpoint(t *testing.T) {
},
expected: true,
},
{
description: "A record with alias=true is valid",
endpoint: Endpoint{
DNSName: "example.com",
RecordType: RecordTypeA,
Targets: Targets{"my-elb-123.us-east-1.elb.amazonaws.com"},
ProviderSpecific: ProviderSpecific{{Name: providerSpecificAlias, Value: "true"}},
},
expected: true,
},
{
description: "AAAA record with alias=true is valid",
endpoint: Endpoint{
DNSName: "example.com",
RecordType: RecordTypeAAAA,
Targets: Targets{"dualstack.my-elb-123.us-east-1.elb.amazonaws.com"},
ProviderSpecific: ProviderSpecific{{Name: providerSpecificAlias, Value: "true"}},
},
expected: true,
},
{
description: "CNAME record with alias=true is valid",
endpoint: Endpoint{
DNSName: "example.com",
RecordType: RecordTypeCNAME,
Targets: Targets{"d111111abcdef8.cloudfront.net"},
ProviderSpecific: ProviderSpecific{{Name: providerSpecificAlias, Value: "true"}},
},
expected: true,
},
{
description: "MX record with alias=true is invalid",
endpoint: Endpoint{
DNSName: "example.com",
RecordType: RecordTypeMX,
Targets: Targets{"10 mail.example.com"},
ProviderSpecific: ProviderSpecific{{Name: providerSpecificAlias, Value: "true"}},
},
expected: false,
},
{
description: "TXT record with alias=true is invalid",
endpoint: Endpoint{
DNSName: "example.com",
RecordType: RecordTypeTXT,
Targets: Targets{"v=spf1 ~all"},
ProviderSpecific: ProviderSpecific{{Name: providerSpecificAlias, Value: "true"}},
},
expected: false,
},
{
description: "NS record with alias=true is invalid",
endpoint: Endpoint{
DNSName: "example.com",
RecordType: RecordTypeNS,
Targets: Targets{"ns1.example.com"},
ProviderSpecific: ProviderSpecific{{Name: providerSpecificAlias, Value: "true"}},
},
expected: false,
},
{
description: "SRV record with alias=true is invalid",
endpoint: Endpoint{
DNSName: "_sip._tcp.example.com",
RecordType: RecordTypeSRV,
Targets: Targets{"10 5 5060 sip.example.com."},
ProviderSpecific: ProviderSpecific{{Name: providerSpecificAlias, Value: "true"}},
},
expected: false,
Copy link
Copy Markdown
Member

@ivankatliarchuk ivankatliarchuk Feb 12, 2026

Choose a reason for hiding this comment

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

false is default

Copy link
Copy Markdown
Member Author

@u-kai u-kai Feb 15, 2026

Choose a reason for hiding this comment

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

Could you clarify what you mean by "false is default"?

Copy link
Copy Markdown
Member

@ivankatliarchuk ivankatliarchuk Feb 15, 2026

Choose a reason for hiding this comment

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

You do not need explicit false, as its a default value

},
{
description: "MX record with alias=false is also invalid",
endpoint: Endpoint{
DNSName: "example.com",
RecordType: RecordTypeMX,
Targets: Targets{"10 mail.example.com"},
ProviderSpecific: ProviderSpecific{{Name: providerSpecificAlias, Value: "false"}},
},
expected: false,
},
{
description: "MX record without alias property is valid",
endpoint: Endpoint{
DNSName: "example.com",
RecordType: RecordTypeMX,
Targets: Targets{"10 mail.example.com"},
},
expected: true,
},
}

for _, tt := range tests {
Expand All @@ -949,6 +1041,69 @@ func TestCheckEndpoint(t *testing.T) {
}
}

func TestCheckEndpoint_AliasWarningLog(t *testing.T) {
tests := []struct {
name string
ep Endpoint
wantLog bool
}{
{
name: "unsupported type with alias logs warning",
ep: Endpoint{
DNSName: "example.com",
RecordType: RecordTypeMX,
Targets: Targets{"10 mail.example.com"},
ProviderSpecific: ProviderSpecific{{Name: providerSpecificAlias, Value: "true"}},
},
wantLog: true,
},
{
name: "supported type with alias does not log",
ep: Endpoint{
DNSName: "example.com",
RecordType: RecordTypeA,
Targets: Targets{"my-elb-123.us-east-1.elb.amazonaws.com"},
ProviderSpecific: ProviderSpecific{{Name: providerSpecificAlias, Value: "true"}},
},
wantLog: false,
},
{
name: "unsupported type without alias does not log",
ep: Endpoint{
DNSName: "example.com",
RecordType: RecordTypeMX,
Targets: Targets{"10 mail.example.com"},
},
wantLog: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
logger, hook := test.NewNullLogger()
Copy link
Copy Markdown
Member

@ivankatliarchuk ivankatliarchuk Feb 20, 2026

Choose a reason for hiding this comment

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

make sure to use shared construct. let's not create for every test a solution

Copy link
Copy Markdown
Member Author

@u-kai u-kai Feb 22, 2026

Choose a reason for hiding this comment

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

Currently, it’s difficult to use a shared construct due to circular dependencies.
Once this PR is merged, I was planning to refactor the structure and introduce a shared construct in a follow-up PR.

Copy link
Copy Markdown
Member

@ivankatliarchuk ivankatliarchuk Feb 22, 2026

Choose a reason for hiding this comment

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

Ok

log.AddHook(hook)
log.SetOutput(logger.Out)
log.SetLevel(log.WarnLevel)

tt.ep.CheckEndpoint()

warnMsg := "does not support alias records"
found := false
for _, entry := range hook.AllEntries() {
if strings.Contains(entry.Message, warnMsg) && entry.Level == log.WarnLevel {
found = true
}
}

if tt.wantLog {
assert.True(t, found, "Expected warning log message not found")
} else {
assert.False(t, found, "Unexpected warning log message found")
}
})
}
}

func TestEndpoint_WithRefObject(t *testing.T) {
ep := &Endpoint{}
ref := &events.ObjectReference{
Expand Down
73 changes: 60 additions & 13 deletions source/wrappers/dedupsource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,29 @@ func TestDedupEndpointsValidation(t *testing.T) {
{DNSName: "example.org", RecordType: endpoint.RecordTypeA, Targets: endpoint.Targets{"1.2.3.4"}},
},
},
{
name: "MX record with alias=true is filtered out",
endpoints: []*endpoint.Endpoint{
{DNSName: "example.org", RecordType: endpoint.RecordTypeMX, Targets: endpoint.Targets{"10 mail.example.org"}, ProviderSpecific: endpoint.ProviderSpecific{{Name: "alias", Value: "true"}}},
},
expected: []*endpoint.Endpoint{},
},
{
name: "A record with alias=true is kept",
endpoints: []*endpoint.Endpoint{
{DNSName: "example.org", RecordType: endpoint.RecordTypeA, Targets: endpoint.Targets{"192.168.1.1"}, ProviderSpecific: endpoint.ProviderSpecific{{Name: "alias", Value: "true"}}},
},
expected: []*endpoint.Endpoint{
{DNSName: "example.org", RecordType: endpoint.RecordTypeA, Targets: endpoint.Targets{"192.168.1.1"}, ProviderSpecific: endpoint.ProviderSpecific{{Name: "alias", Value: "true"}}},
},
},
{
name: "SRV record with alias=true is filtered out",
endpoints: []*endpoint.Endpoint{
{DNSName: "_sip._tcp.example.org", RecordType: endpoint.RecordTypeSRV, Targets: endpoint.Targets{"10 5 5060 sip.example.org."}, ProviderSpecific: endpoint.ProviderSpecific{{Name: "alias", Value: "true"}}},
},
expected: []*endpoint.Endpoint{},
},
{
name: "mixed valid and invalid TXT, A, AAAA records",
endpoints: []*endpoint.Endpoint{
Expand Down Expand Up @@ -290,21 +313,45 @@ func TestDedupEndpointsValidation(t *testing.T) {
}

func TestDedupSource_WarnsOnInvalidEndpoint(t *testing.T) {
hook := testutils.LogsUnderTestWithLogLevel(log.WarnLevel, t)

invalidEndpoint := &endpoint.Endpoint{
DNSName: "example.org",
RecordType: endpoint.RecordTypeSRV,
SetIdentifier: "default/svc/my-service",
Targets: endpoint.Targets{"10 mail.example.org"},
tests := []struct {
name string
endpoint *endpoint.Endpoint
wantLogMsg string
}{
{
name: "invalid SRV record",
endpoint: &endpoint.Endpoint{
DNSName: "example.org",
RecordType: endpoint.RecordTypeSRV,
SetIdentifier: "default/svc/my-service",
Targets: endpoint.Targets{"10 mail.example.org"},
},
wantLogMsg: "Skipping endpoint [default/svc/my-service:example.org] due to invalid configuration [SRV:10 mail.example.org]",
},
{
name: "unsupported alias on MX record",
endpoint: &endpoint.Endpoint{
DNSName: "example.org",
RecordType: endpoint.RecordTypeMX,
Targets: endpoint.Targets{"10 mail.example.org"},
ProviderSpecific: endpoint.ProviderSpecific{{Name: "alias", Value: "true"}},
},
wantLogMsg: "Endpoint example.org of type MX does not support alias records",
},
}

mockSource := new(testutils.MockSource)
mockSource.On("Endpoints").Return([]*endpoint.Endpoint{invalidEndpoint}, nil)
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
hook := testutils.LogsUnderTestWithLogLevel(log.WarnLevel, t)

src := NewDedupSource(mockSource)
_, err := src.Endpoints(context.Background())
require.NoError(t, err)
mockSource := new(testutils.MockSource)
mockSource.On("Endpoints").Return([]*endpoint.Endpoint{tt.endpoint}, nil)

testutils.TestHelperLogContains("Skipping endpoint [default/svc/my-service:example.org] due to invalid configuration [SRV:10 mail.example.org]", hook, t)
src := NewDedupSource(mockSource)
_, err := src.Endpoints(context.Background())
require.NoError(t, err)

testutils.TestHelperLogContains(tt.wantLogMsg, hook, t)
})
}
}
Loading