diff --git a/provider/aws/aws.go b/provider/aws/aws.go index 873d860d8b..83ad875019 100644 --- a/provider/aws/aws.go +++ b/provider/aws/aws.go @@ -904,6 +904,21 @@ func adjustGeoProximityLocationEndpoint(ep *endpoint.Endpoint) { } } +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. // action=ChangeActionCreate returns a change for creation of the record and @@ -954,6 +969,24 @@ 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 == "" { + ignoredProperties := make([]string, 0, len(ep.ProviderSpecific)) + for _, prop := range ep.ProviderSpecific { + if _, ok := providerSpecificRequiringSetIdentifier[prop.Name]; ok { + ignoredProperties = append(ignoredProperties, prop.Name) + } + } + if len(ignoredProperties) > 0 { + 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, 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..51803a1334 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/failover aws/region aws/weight] 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) + } + }) + } +}