From 6e920e30deaa835bcc44a475595c459f6ad27f5c Mon Sep 17 00:00:00 2001 From: u-kai <76635578+u-kai@users.noreply.github.com> Date: Wed, 3 Sep 2025 21:30:41 +0900 Subject: [PATCH 1/3] feat(aws): add warning for provider-specific properties without setIdentifier --- provider/aws/aws.go | 32 +++++++++++ provider/aws/aws_test.go | 118 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 150 insertions(+) diff --git a/provider/aws/aws.go b/provider/aws/aws.go index 873d860d8b..c14d5a4c44 100644 --- a/provider/aws/aws.go +++ b/provider/aws/aws.go @@ -954,6 +954,38 @@ func (p *AWSProvider) newChange(action route53types.ChangeAction, ep *endpoint.E } setIdentifier := ep.SetIdentifier + + // Check if provider-specific values requiring setIdentifier are present but setIdentifier is empty + // AWS Route53 requires setIdentifier for routing policies: + // https://docs.aws.amazon.com/Route53/latest/APIReference/API_ResourceRecordSet.html + if setIdentifier == "" { + providerSpecificRequiringSetIdentifier := []string{ + providerSpecificWeight, + providerSpecificRegion, + providerSpecificFailover, + providerSpecificGeolocationContinentCode, + providerSpecificGeolocationCountryCode, + providerSpecificGeolocationSubdivisionCode, + providerSpecificGeoProximityLocationAWSRegion, + providerSpecificGeoProximityLocationBias, + providerSpecificGeoProximityLocationCoordinates, + providerSpecificGeoProximityLocationLocalZoneGroup, + providerSpecificMultiValueAnswer, + } + + ignoredProperties := make([]string, 0, len(providerSpecificRequiringSetIdentifier)) + + for _, prop := range providerSpecificRequiringSetIdentifier { + if _, ok := ep.GetProviderSpecificProperty(prop); ok { + ignoredProperties = append(ignoredProperties, prop) + } + } + if len(ignoredProperties) > 0 { + log.Warnf("Endpoint %s has provider-specific properties %v that require a setIdentifier, but none was set; ignoring these properties", + ep.DNSName, ignoredProperties) + } + } + if setIdentifier != "" { change.ResourceRecordSet.SetIdentifier = aws.String(setIdentifier) if prop, ok := ep.GetProviderSpecificProperty(providerSpecificWeight); ok { diff --git a/provider/aws/aws_test.go b/provider/aws/aws_test.go index a269c3ede8..dba3a2626f 100644 --- a/provider/aws/aws_test.go +++ b/provider/aws/aws_test.go @@ -2850,3 +2850,121 @@ func TestAWSProvider_createUpdateChanges_NewMoreThanOld(t *testing.T) { require.Equal(t, 1, upserts, "should upsert the matching endpoint") require.Equal(t, 0, deletes, "should not delete anything") } + +func TestAWSProvider_newChange_WarnForProviderSpecificWithoutSetIdentifier(t *testing.T) { + provider, _ := newAWSProvider(t, endpoint.NewDomainFilter([]string{"foo.bar."}), provider.NewZoneIDFilter([]string{}), provider.NewZoneTypeFilter(""), true, false, nil) + + tests := []struct { + name string + endpoint *endpoint.Endpoint + expectedWarnLog string + unexpectedWarnLogs []string + }{ + { + name: "endpoint with weight but no setIdentifier should warn", + endpoint: &endpoint.Endpoint{ + DNSName: "test.foo.bar.", + RecordType: endpoint.RecordTypeA, + Targets: []string{"1.2.3.4"}, + ProviderSpecific: []endpoint.ProviderSpecificProperty{ + {Name: providerSpecificWeight, Value: "100"}, + }, + }, + expectedWarnLog: "Endpoint test.foo.bar. has provider-specific properties [aws/weight] that require a setIdentifier, but none was set; ignoring these properties", + unexpectedWarnLogs: nil, + }, + { + name: "endpoint with region but no setIdentifier should warn", + endpoint: &endpoint.Endpoint{ + DNSName: "test.foo.bar.", + RecordType: endpoint.RecordTypeA, + Targets: []string{"1.2.3.4"}, + ProviderSpecific: []endpoint.ProviderSpecificProperty{ + {Name: providerSpecificRegion, Value: "us-east-1"}, + }, + }, + expectedWarnLog: "Endpoint test.foo.bar. has provider-specific properties [aws/region] that require a setIdentifier, but none was set; ignoring these properties", + unexpectedWarnLogs: nil, + }, + { + name: "endpoint with multiple properties but no setIdentifier should warn", + endpoint: &endpoint.Endpoint{ + DNSName: "test.foo.bar.", + RecordType: endpoint.RecordTypeA, + Targets: []string{"1.2.3.4"}, + ProviderSpecific: []endpoint.ProviderSpecificProperty{ + {Name: providerSpecificWeight, Value: "100"}, + {Name: providerSpecificRegion, Value: "us-east-1"}, + {Name: providerSpecificFailover, Value: "PRIMARY"}, + }, + }, + expectedWarnLog: "Endpoint test.foo.bar. has provider-specific properties [aws/weight aws/region aws/failover] that require a setIdentifier, but none was set; ignoring these properties", + unexpectedWarnLogs: nil, + }, + { + name: "endpoint with setIdentifier should not warn", + endpoint: &endpoint.Endpoint{ + DNSName: "test.foo.bar.", + RecordType: endpoint.RecordTypeA, + Targets: []string{"1.2.3.4"}, + SetIdentifier: "primary", + ProviderSpecific: []endpoint.ProviderSpecificProperty{ + {Name: providerSpecificWeight, Value: "100"}, + }, + }, + expectedWarnLog: "", + unexpectedWarnLogs: []string{ + "test.foo.bar. has provider-specific properties", + "ignoring these properties", + }, + }, + { + name: "endpoint without provider specific properties should not warn", + endpoint: &endpoint.Endpoint{ + DNSName: "test.foo.bar.", + RecordType: endpoint.RecordTypeA, + Targets: []string{"1.2.3.4"}, + }, + expectedWarnLog: "", + unexpectedWarnLogs: []string{ + "test.foo.bar. has provider-specific properties", + "ignoring these properties", + }, + }, + { + name: "endpoint with alias property (not requiring setIdentifier) should not warn", + endpoint: &endpoint.Endpoint{ + DNSName: "test.foo.bar.", + RecordType: endpoint.RecordTypeA, + Targets: []string{"test-elb-123456789.us-east-1.elb.amazonaws.com"}, + ProviderSpecific: []endpoint.ProviderSpecificProperty{ + {Name: providerSpecificAlias, Value: "true"}, + }, + }, + expectedWarnLog: "", + unexpectedWarnLogs: []string{ + "test.foo.bar. has provider-specific properties", + "ignoring these properties", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + hook := testutils.LogsUnderTestWithLogLevel(log.WarnLevel, t) + + change := provider.newChange(route53types.ChangeActionCreate, tt.endpoint) + + assert.NotNil(t, change) + assert.Equal(t, route53types.ChangeActionCreate, change.Action) + + if tt.expectedWarnLog != "" { + testutils.TestHelperLogContains(tt.expectedWarnLog, hook, t) + } + + for _, unexpectedLog := range tt.unexpectedWarnLogs { + testutils.TestHelperLogNotContains(unexpectedLog, hook, t) + } + }) + } +} From 394755372f060ed4142ed686b886d6a704e0bc13 Mon Sep 17 00:00:00 2001 From: u-kai <76635578+u-kai@users.noreply.github.com> Date: Sat, 6 Sep 2025 13:59:55 +0900 Subject: [PATCH 2/3] refactor(aws): optimize provider-specific property validation using sets --- provider/aws/aws.go | 44 +++++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/provider/aws/aws.go b/provider/aws/aws.go index c14d5a4c44..211eaf0218 100644 --- a/provider/aws/aws.go +++ b/provider/aws/aws.go @@ -32,6 +32,8 @@ import ( route53types "github.com/aws/aws-sdk-go-v2/service/route53/types" log "github.com/sirupsen/logrus" + "k8s.io/utils/set" + "sigs.k8s.io/external-dns/endpoint" "sigs.k8s.io/external-dns/plan" "sigs.k8s.io/external-dns/provider" @@ -904,6 +906,21 @@ func adjustGeoProximityLocationEndpoint(ep *endpoint.Endpoint) { } } +var providerSpecificRequiringSetIdentifier = set.New( + providerSpecificEvaluateTargetHealth, + providerSpecificWeight, + providerSpecificRegion, + providerSpecificFailover, + providerSpecificGeolocationContinentCode, + providerSpecificGeolocationCountryCode, + providerSpecificGeolocationSubdivisionCode, + providerSpecificGeoProximityLocationAWSRegion, + providerSpecificGeoProximityLocationBias, + providerSpecificGeoProximityLocationCoordinates, + providerSpecificGeoProximityLocationLocalZoneGroup, + providerSpecificMultiValueAnswer, +) + // newChange returns a route53 Change // returned Change is based on the given record by the given action, e.g. // action=ChangeActionCreate returns a change for creation of the record and @@ -959,30 +976,15 @@ func (p *AWSProvider) newChange(action route53types.ChangeAction, ep *endpoint.E // AWS Route53 requires setIdentifier for routing policies: // https://docs.aws.amazon.com/Route53/latest/APIReference/API_ResourceRecordSet.html if setIdentifier == "" { - providerSpecificRequiringSetIdentifier := []string{ - providerSpecificWeight, - providerSpecificRegion, - providerSpecificFailover, - providerSpecificGeolocationContinentCode, - providerSpecificGeolocationCountryCode, - providerSpecificGeolocationSubdivisionCode, - providerSpecificGeoProximityLocationAWSRegion, - providerSpecificGeoProximityLocationBias, - providerSpecificGeoProximityLocationCoordinates, - providerSpecificGeoProximityLocationLocalZoneGroup, - providerSpecificMultiValueAnswer, - } - - ignoredProperties := make([]string, 0, len(providerSpecificRequiringSetIdentifier)) - - for _, prop := range providerSpecificRequiringSetIdentifier { - if _, ok := ep.GetProviderSpecificProperty(prop); ok { - ignoredProperties = append(ignoredProperties, prop) - } + providerSpecificSet := make(set.Set[string], len(ep.ProviderSpecific)) + for _, p := range ep.ProviderSpecific { + providerSpecificSet.Insert(p.Name) } + ignoredProperties := providerSpecificRequiringSetIdentifier.Intersection(providerSpecificSet) if len(ignoredProperties) > 0 { + pMsg := ignoredProperties.SortedList() log.Warnf("Endpoint %s has provider-specific properties %v that require a setIdentifier, but none was set; ignoring these properties", - ep.DNSName, ignoredProperties) + ep.DNSName, pMsg) } } From 7106c67059bb2054de62eadcb89bad048dfd7458 Mon Sep 17 00:00:00 2001 From: u-kai <76635578+u-kai@users.noreply.github.com> Date: Mon, 6 Oct 2025 08:14:12 +0900 Subject: [PATCH 3/3] fix set to map and slices --- provider/aws/aws.go | 43 ++++++++++++++++++++-------------------- provider/aws/aws_test.go | 6 +++--- 2 files changed, 24 insertions(+), 25 deletions(-) diff --git a/provider/aws/aws.go b/provider/aws/aws.go index 211eaf0218..83ad875019 100644 --- a/provider/aws/aws.go +++ b/provider/aws/aws.go @@ -32,8 +32,6 @@ import ( route53types "github.com/aws/aws-sdk-go-v2/service/route53/types" log "github.com/sirupsen/logrus" - "k8s.io/utils/set" - "sigs.k8s.io/external-dns/endpoint" "sigs.k8s.io/external-dns/plan" "sigs.k8s.io/external-dns/provider" @@ -906,20 +904,20 @@ func adjustGeoProximityLocationEndpoint(ep *endpoint.Endpoint) { } } -var providerSpecificRequiringSetIdentifier = set.New( - providerSpecificEvaluateTargetHealth, - providerSpecificWeight, - providerSpecificRegion, - providerSpecificFailover, - providerSpecificGeolocationContinentCode, - providerSpecificGeolocationCountryCode, - providerSpecificGeolocationSubdivisionCode, - providerSpecificGeoProximityLocationAWSRegion, - providerSpecificGeoProximityLocationBias, - providerSpecificGeoProximityLocationCoordinates, - providerSpecificGeoProximityLocationLocalZoneGroup, - providerSpecificMultiValueAnswer, -) +var providerSpecificRequiringSetIdentifier = map[string]struct{}{ + providerSpecificEvaluateTargetHealth: {}, + providerSpecificWeight: {}, + providerSpecificRegion: {}, + providerSpecificFailover: {}, + providerSpecificGeolocationContinentCode: {}, + providerSpecificGeolocationCountryCode: {}, + providerSpecificGeolocationSubdivisionCode: {}, + providerSpecificGeoProximityLocationAWSRegion: {}, + providerSpecificGeoProximityLocationBias: {}, + providerSpecificGeoProximityLocationCoordinates: {}, + providerSpecificGeoProximityLocationLocalZoneGroup: {}, + providerSpecificMultiValueAnswer: {}, +} // newChange returns a route53 Change // returned Change is based on the given record by the given action, e.g. @@ -976,15 +974,16 @@ func (p *AWSProvider) newChange(action route53types.ChangeAction, ep *endpoint.E // AWS Route53 requires setIdentifier for routing policies: // https://docs.aws.amazon.com/Route53/latest/APIReference/API_ResourceRecordSet.html if setIdentifier == "" { - providerSpecificSet := make(set.Set[string], len(ep.ProviderSpecific)) - for _, p := range ep.ProviderSpecific { - providerSpecificSet.Insert(p.Name) + ignoredProperties := make([]string, 0, len(ep.ProviderSpecific)) + for _, prop := range ep.ProviderSpecific { + if _, ok := providerSpecificRequiringSetIdentifier[prop.Name]; ok { + ignoredProperties = append(ignoredProperties, prop.Name) + } } - ignoredProperties := providerSpecificRequiringSetIdentifier.Intersection(providerSpecificSet) if len(ignoredProperties) > 0 { - pMsg := ignoredProperties.SortedList() + slices.Sort(ignoredProperties) log.Warnf("Endpoint %s has provider-specific properties %v that require a setIdentifier, but none was set; ignoring these properties", - ep.DNSName, pMsg) + ep.DNSName, ignoredProperties) } } diff --git a/provider/aws/aws_test.go b/provider/aws/aws_test.go index dba3a2626f..51803a1334 100644 --- a/provider/aws/aws_test.go +++ b/provider/aws/aws_test.go @@ -2870,7 +2870,7 @@ func TestAWSProvider_newChange_WarnForProviderSpecificWithoutSetIdentifier(t *te {Name: providerSpecificWeight, Value: "100"}, }, }, - expectedWarnLog: "Endpoint test.foo.bar. has provider-specific properties [aws/weight] that require a setIdentifier, but none was set; ignoring these properties", + expectedWarnLog: "Endpoint test.foo.bar. has provider-specific properties [aws/weight] that require a setIdentifier, but none was set; ignoring these properties", unexpectedWarnLogs: nil, }, { @@ -2883,7 +2883,7 @@ func TestAWSProvider_newChange_WarnForProviderSpecificWithoutSetIdentifier(t *te {Name: providerSpecificRegion, Value: "us-east-1"}, }, }, - expectedWarnLog: "Endpoint test.foo.bar. has provider-specific properties [aws/region] that require a setIdentifier, but none was set; ignoring these properties", + expectedWarnLog: "Endpoint test.foo.bar. has provider-specific properties [aws/region] that require a setIdentifier, but none was set; ignoring these properties", unexpectedWarnLogs: nil, }, { @@ -2898,7 +2898,7 @@ func TestAWSProvider_newChange_WarnForProviderSpecificWithoutSetIdentifier(t *te {Name: providerSpecificFailover, Value: "PRIMARY"}, }, }, - expectedWarnLog: "Endpoint test.foo.bar. has provider-specific properties [aws/weight aws/region aws/failover] that require a setIdentifier, but none was set; ignoring these properties", + expectedWarnLog: "Endpoint test.foo.bar. has provider-specific properties [aws/failover aws/region aws/weight] that require a setIdentifier, but none was set; ignoring these properties", unexpectedWarnLogs: nil, }, {