From f88c8071f8c8422590c752aeae297b5baf344c76 Mon Sep 17 00:00:00 2001 From: Valentin Flaux <38909103+vflaux@users.noreply.github.com> Date: Wed, 27 Aug 2025 15:44:43 +0200 Subject: [PATCH] fix(cloudflare): unneeded records updates --- provider/cloudflare/cloudflare.go | 7 +- provider/cloudflare/cloudflare_regional.go | 23 +++- .../cloudflare/cloudflare_regional_test.go | 107 ++++++++++++++++++ 3 files changed, 130 insertions(+), 7 deletions(-) diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go index 144947a9ca..c7ff356585 100644 --- a/provider/cloudflare/cloudflare.go +++ b/provider/cloudflare/cloudflare.go @@ -744,12 +744,7 @@ func (p *CloudFlareProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) ([] e.DeleteProviderSpecificProperty(annotations.CloudflareCustomHostnameKey) } - if p.RegionalServicesConfig.Enabled { - // Add default region key if not set - if _, ok := e.GetProviderSpecificProperty(annotations.CloudflareRegionKey); !ok { - e.SetProviderSpecificProperty(annotations.CloudflareRegionKey, p.RegionalServicesConfig.RegionKey) - } - } + p.adjustEndpointProviderSpecificRegionKeyProperty(e) adjustedEndpoints = append(adjustedEndpoints, e) } diff --git a/provider/cloudflare/cloudflare_regional.go b/provider/cloudflare/cloudflare_regional.go index 9a78851d6c..08372993ea 100644 --- a/provider/cloudflare/cloudflare_regional.go +++ b/provider/cloudflare/cloudflare_regional.go @@ -222,13 +222,34 @@ func (p *CloudFlareProvider) addEnpointsProviderSpecificRegionKeyProperty(ctx co } for _, ep := range supportedEndpoints { + var regionKey string if rh, found := regionalHostnames[ep.DNSName]; found { - ep.SetProviderSpecificProperty(annotations.CloudflareRegionKey, rh.regionKey) + regionKey = rh.regionKey } + ep.SetProviderSpecificProperty(annotations.CloudflareRegionKey, regionKey) } return nil } +// adjustEnpointProviderSpecificRegionKeyProperty updates the given endpoint's provider-specific +// Cloudflare region key based on the provider's RegionalServicesConfig. +// - If regional services are disabled or the endpoint's record type does not +// support regional hostnames, the Cloudflare region key is removed. +// - If enabled and supported, and the key is not already set, it is initialized +// to the provider's default RegionKey. +// +// The endpoint is modified in place and any explicitly set region key is left unchanged. +func (p *CloudFlareProvider) adjustEndpointProviderSpecificRegionKeyProperty(ep *endpoint.Endpoint) { + if !p.RegionalServicesConfig.Enabled || !recordTypeRegionalHostnameSupported[ep.RecordType] { + ep.DeleteProviderSpecificProperty(annotations.CloudflareRegionKey) + return + } + // Add default region key if not set + if _, ok := ep.GetProviderSpecificProperty(annotations.CloudflareRegionKey); !ok { + ep.SetProviderSpecificProperty(annotations.CloudflareRegionKey, p.RegionalServicesConfig.RegionKey) + } +} + // desiredRegionalHostnames builds a list of desired regional hostnames from changes. // // If there is a delete and a create or update action for the same hostname, diff --git a/provider/cloudflare/cloudflare_regional_test.go b/provider/cloudflare/cloudflare_regional_test.go index 28ec48f318..b114adafcc 100644 --- a/provider/cloudflare/cloudflare_regional_test.go +++ b/provider/cloudflare/cloudflare_regional_test.go @@ -32,6 +32,7 @@ import ( "sigs.k8s.io/external-dns/endpoint" "sigs.k8s.io/external-dns/internal/testutils" "sigs.k8s.io/external-dns/plan" + "sigs.k8s.io/external-dns/source/annotations" ) func (m *mockCloudFlareClient) ListDataLocalizationRegionalHostnames(ctx context.Context, params addressing.RegionalHostnameListParams) autoPager[addressing.RegionalHostnameListResponse] { @@ -1186,3 +1187,109 @@ func TestApplyChangesWithRegionalHostnamesDryRun(t *testing.T) { }) } } + +func TestCloudflareAdjustEndpointsRegionalServices(t *testing.T) { + testCases := []struct { + name string + recordType string + regionalServicesConfig RegionalServicesConfig + initialRegionKey string // existing region key on endpoint + expectedRegionKey *string // expected region key after AdjustEndpoints (nil = should not be present) + }{ + // Supported types should get region key when enabled + { + name: "A record with regional services enabled", + recordType: "A", + regionalServicesConfig: RegionalServicesConfig{Enabled: true, RegionKey: "us"}, + initialRegionKey: "", + expectedRegionKey: testutils.ToPtr("us"), + }, + { + name: "AAAA record with regional services enabled", + recordType: "AAAA", + regionalServicesConfig: RegionalServicesConfig{Enabled: true, RegionKey: "us"}, + initialRegionKey: "", + expectedRegionKey: testutils.ToPtr("us"), + }, + { + name: "CNAME record with regional services enabled", + recordType: "CNAME", + regionalServicesConfig: RegionalServicesConfig{Enabled: true, RegionKey: "us"}, + initialRegionKey: "", + expectedRegionKey: testutils.ToPtr("us"), + }, + + // Unsupported types should NOT get region key even when enabled + { + name: "TXT record with regional services enabled", + recordType: "TXT", + regionalServicesConfig: RegionalServicesConfig{Enabled: true, RegionKey: "us"}, + initialRegionKey: "", + expectedRegionKey: nil, + }, + + // Disabled regional services should remove region key for all types + { + name: "A record with regional services disabled", + recordType: "A", + regionalServicesConfig: RegionalServicesConfig{Enabled: false}, + initialRegionKey: "existing-region", + expectedRegionKey: nil, + }, + { + name: "TXT record with regional services disabled", + recordType: "TXT", + regionalServicesConfig: RegionalServicesConfig{Enabled: false}, + initialRegionKey: "existing-region", + expectedRegionKey: nil, + }, + + // Existing region key should be preserved when already set + { + name: "A record with existing custom region key", + recordType: "A", + regionalServicesConfig: RegionalServicesConfig{Enabled: true, RegionKey: "us"}, + initialRegionKey: "eu", + expectedRegionKey: testutils.ToPtr("eu"), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Create endpoint with initial region key if specified + testEndpoint := &endpoint.Endpoint{ + RecordType: tc.recordType, + DNSName: "test.bar.com", + Targets: endpoint.Targets{"127.0.0.1"}, + } + + if tc.initialRegionKey != "" { + testEndpoint.ProviderSpecific = endpoint.ProviderSpecific{ + endpoint.ProviderSpecificProperty{ + Name: annotations.CloudflareRegionKey, + Value: tc.initialRegionKey, + }, + } + } + + provider := &CloudFlareProvider{ + RegionalServicesConfig: tc.regionalServicesConfig, + } + + adjustedEndpoints, err := provider.AdjustEndpoints([]*endpoint.Endpoint{testEndpoint}) + assert.NoError(t, err) + assert.Len(t, adjustedEndpoints, 1) + + regionKey, exists := adjustedEndpoints[0].GetProviderSpecificProperty(annotations.CloudflareRegionKey) + + if tc.expectedRegionKey != nil { + // Region key should be present with expected value + assert.True(t, exists, "Region key should be present") + assert.Equal(t, *tc.expectedRegionKey, regionKey, "Region key value should match expected") + } else { + // Region key should not be present + assert.False(t, exists, "Region key should not be present") + } + }) + } +}