Skip to content

Commit ad3d958

Browse files
vflauxvflaux
authored
fix(cloudflare): reconciliation loop with default comment (#5828)
1 parent 55b24ad commit ad3d958

File tree

2 files changed

+137
-40
lines changed

2 files changed

+137
-40
lines changed

‎provider/cloudflare/cloudflare.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -738,6 +738,12 @@ func (p *CloudFlareProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) ([]
738738

739739
p.adjustEndpointProviderSpecificRegionKeyProperty(e)
740740

741+
if p.DNSRecordsConfig.Comment != "" {
742+
if _, found := e.GetProviderSpecificProperty(annotations.CloudflareRecordCommentKey); !found {
743+
e.SetProviderSpecificProperty(annotations.CloudflareRecordCommentKey, p.DNSRecordsConfig.Comment)
744+
}
745+
}
746+
741747
adjustedEndpoints = append(adjustedEndpoints, e)
742748
}
743749
return adjustedEndpoints, nil

‎provider/cloudflare/cloudflare_test.go‎

Lines changed: 131 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1479,60 +1479,139 @@ func TestGroupByNameAndTypeWithCustomHostnames_MX(t *testing.T) {
14791479
}
14801480

14811481
func TestProviderPropertiesIdempotency(t *testing.T) {
1482+
t.Parallel()
1483+
14821484
testCases := []struct {
1483-
Name string
1484-
ProviderProxiedByDefault bool
1485-
RecordsAreProxied bool
1486-
ShouldBeUpdated bool
1485+
Name string
1486+
SetupProvider func(*CloudFlareProvider)
1487+
SetupRecord func(*dns.RecordResponse)
1488+
CustomHostnames []cloudflarev0.CustomHostname
1489+
RegionKey string
1490+
ShouldBeUpdated bool
1491+
PropertyKey string
1492+
ExpectPropertyPresent bool
1493+
ExpectPropertyValue string
14871494
}{
14881495
{
1489-
Name: "ProxyDefault: false, ShouldBeProxied: false, ExpectUpdates: false",
1490-
ProviderProxiedByDefault: false,
1491-
RecordsAreProxied: false,
1492-
ShouldBeUpdated: false,
1496+
Name: "No custom properties, ExpectUpdates: false",
1497+
SetupProvider: func(p *CloudFlareProvider) {},
1498+
SetupRecord: func(r *dns.RecordResponse) {},
1499+
ShouldBeUpdated: false,
1500+
},
1501+
// Proxied tests
1502+
{
1503+
Name: "ProxiedByDefault: true, ProxiedRecord: true, ExpectUpdates: false",
1504+
SetupProvider: func(p *CloudFlareProvider) { p.proxiedByDefault = true },
1505+
SetupRecord: func(r *dns.RecordResponse) { r.Proxied = true },
1506+
ShouldBeUpdated: false,
1507+
},
1508+
{
1509+
Name: "ProxiedByDefault: true, ProxiedRecord: false, ExpectUpdates: true",
1510+
SetupProvider: func(p *CloudFlareProvider) { p.proxiedByDefault = true },
1511+
SetupRecord: func(r *dns.RecordResponse) { r.Proxied = false },
1512+
ShouldBeUpdated: true,
1513+
PropertyKey: annotations.CloudflareProxiedKey,
1514+
ExpectPropertyValue: "true",
1515+
},
1516+
{
1517+
Name: "ProxiedByDefault: false, ProxiedRecord: true, ExpectUpdates: true",
1518+
SetupProvider: func(p *CloudFlareProvider) { p.proxiedByDefault = false },
1519+
SetupRecord: func(r *dns.RecordResponse) { r.Proxied = true },
1520+
ShouldBeUpdated: true,
1521+
PropertyKey: annotations.CloudflareProxiedKey,
1522+
ExpectPropertyValue: "false",
1523+
},
1524+
// Comment tests
1525+
{
1526+
Name: "DefaultComment: 'foo', RecordComment: 'foo', ExpectUpdates: false",
1527+
SetupProvider: func(p *CloudFlareProvider) { p.DNSRecordsConfig.Comment = "foo" },
1528+
SetupRecord: func(r *dns.RecordResponse) { r.Comment = "foo" },
1529+
ShouldBeUpdated: false,
14931530
},
14941531
{
1495-
Name: "ProxyDefault: true, ShouldBeProxied: true, ExpectUpdates: false",
1496-
ProviderProxiedByDefault: true,
1497-
RecordsAreProxied: true,
1498-
ShouldBeUpdated: false,
1532+
Name: "DefaultComment: '', RecordComment: none, ExpectUpdates: true",
1533+
SetupProvider: func(p *CloudFlareProvider) { p.DNSRecordsConfig.Comment = "" },
1534+
SetupRecord: func(r *dns.RecordResponse) { r.Comment = "foo" },
1535+
ShouldBeUpdated: true,
1536+
PropertyKey: annotations.CloudflareRecordCommentKey,
1537+
ExpectPropertyPresent: false,
14991538
},
15001539
{
1501-
Name: "ProxyDefault: true, ShouldBeProxied: false, ExpectUpdates: true",
1502-
ProviderProxiedByDefault: true,
1503-
RecordsAreProxied: false,
1504-
ShouldBeUpdated: true,
1540+
Name: "DefaultComment: 'foo', RecordComment: 'foo', ExpectUpdates: true",
1541+
SetupProvider: func(p *CloudFlareProvider) { p.DNSRecordsConfig.Comment = "foo" },
1542+
SetupRecord: func(r *dns.RecordResponse) { r.Comment = "" },
1543+
ShouldBeUpdated: true,
1544+
PropertyKey: annotations.CloudflareRecordCommentKey,
1545+
ExpectPropertyValue: "foo",
15051546
},
1547+
// Regional Hostname tests
15061548
{
1507-
Name: "ProxyDefault: false, ShouldBeProxied: true, ExpectUpdates: true",
1508-
ProviderProxiedByDefault: false,
1509-
RecordsAreProxied: true,
1510-
ShouldBeUpdated: true,
1549+
Name: "DefaultRegionKey: 'us', RecordRegionKey: 'us', ExpectUpdates: false",
1550+
SetupProvider: func(p *CloudFlareProvider) {
1551+
p.RegionalServicesConfig.Enabled = true
1552+
p.RegionalServicesConfig.RegionKey = "us"
1553+
},
1554+
RegionKey: "us",
1555+
ShouldBeUpdated: false,
1556+
},
1557+
{
1558+
Name: "DefaultRegionKey: 'us', RecordRegionKey: 'us', ExpectUpdates: false",
1559+
SetupProvider: func(p *CloudFlareProvider) {
1560+
p.RegionalServicesConfig.Enabled = true
1561+
p.RegionalServicesConfig.RegionKey = "us"
1562+
},
1563+
RegionKey: "eu",
1564+
ShouldBeUpdated: true,
1565+
PropertyKey: annotations.CloudflareRegionKey,
1566+
ExpectPropertyValue: "us",
15111567
},
1568+
// Custom Hostname tests
1569+
// TODO: add tests for custom hostnames when properly supported
15121570
}
15131571

15141572
for _, test := range testCases {
15151573
t.Run(test.Name, func(t *testing.T) {
1574+
t.Parallel()
1575+
1576+
record := dns.RecordResponse{
1577+
ID: "1234567890",
1578+
Name: "foobar.bar.com",
1579+
Type: endpoint.RecordTypeA,
1580+
TTL: 120,
1581+
Content: "1.2.3.4",
1582+
}
1583+
if test.SetupRecord != nil {
1584+
test.SetupRecord(&record)
1585+
}
15161586
client := NewMockCloudFlareClientWithRecords(map[string][]dns.RecordResponse{
1517-
"001": {
1518-
{
1519-
ID: "1234567890",
1520-
Name: "foobar.bar.com",
1521-
Type: endpoint.RecordTypeA,
1522-
TTL: 120,
1523-
Content: "1.2.3.4",
1524-
Proxied: test.RecordsAreProxied,
1525-
},
1526-
},
1587+
"001": {record},
15271588
})
15281589

1590+
if len(test.CustomHostnames) > 0 {
1591+
customHostnames := make([]cloudflarev0.CustomHostname, 0, len(test.CustomHostnames))
1592+
for _, ch := range test.CustomHostnames {
1593+
ch.CustomOriginServer = record.Name
1594+
customHostnames = append(customHostnames, ch)
1595+
}
1596+
client.customHostnames = map[string][]cloudflarev0.CustomHostname{
1597+
"001": customHostnames,
1598+
}
1599+
}
1600+
1601+
if test.RegionKey != "" {
1602+
client.regionalHostnames = map[string][]regionalHostname{
1603+
"001": {{hostname: record.Name, regionKey: test.RegionKey}},
1604+
}
1605+
}
1606+
15291607
provider := &CloudFlareProvider{
1530-
Client: client,
1531-
proxiedByDefault: test.ProviderProxiedByDefault,
1608+
Client: client,
1609+
}
1610+
if test.SetupProvider != nil {
1611+
test.SetupProvider(provider)
15321612
}
1533-
ctx := context.Background()
15341613

1535-
current, err := provider.Records(ctx)
1614+
current, err := provider.Records(t.Context())
15361615
if err != nil {
15371616
t.Errorf("should not fail, %s", err)
15381617
}
@@ -1561,19 +1640,31 @@ func TestProviderPropertiesIdempotency(t *testing.T) {
15611640
}
15621641

15631642
plan = *plan.Calculate()
1564-
assert.NotNil(t, plan.Changes, "should have plan")
1565-
if plan.Changes == nil {
1566-
return
1567-
}
1643+
require.NotNil(t, plan.Changes, "should have plan")
15681644
assert.Empty(t, plan.Changes.Create, "should not have creates")
15691645
assert.Empty(t, plan.Changes.Delete, "should not have deletes")
15701646

15711647
if test.ShouldBeUpdated {
1572-
assert.Len(t, plan.Changes.UpdateNew, 1, "should not have new updates")
1573-
assert.Len(t, plan.Changes.UpdateOld, 1, "should not have old updates")
1648+
assert.Len(t, plan.Changes.UpdateOld, 1, "should have old updates")
1649+
require.Len(t, plan.Changes.UpdateNew, 1, "should have new updates")
1650+
if test.PropertyKey != "" {
1651+
value, ok := plan.Changes.UpdateNew[0].GetProviderSpecificProperty(test.PropertyKey)
1652+
if test.ExpectPropertyPresent || test.ExpectPropertyValue != "" {
1653+
assert.Truef(t, ok, "should have property %s", test.PropertyKey)
1654+
assert.Equal(t, test.ExpectPropertyValue, value)
1655+
} else {
1656+
assert.Falsef(t, ok, "should not have property %s", test.PropertyKey)
1657+
}
1658+
} else {
1659+
assert.Empty(t, test.ExpectPropertyValue, "test misconfigured, should not expect property value if no property key set")
1660+
assert.False(t, test.ExpectPropertyPresent, "test misconfigured, should not expect property presence if no property key set")
1661+
}
15741662
} else {
15751663
assert.Empty(t, plan.Changes.UpdateNew, "should not have new updates")
15761664
assert.Empty(t, plan.Changes.UpdateOld, "should not have old updates")
1665+
assert.Empty(t, test.PropertyKey, "test misconfigured, should not expect property if no update expected")
1666+
assert.Empty(t, test.ExpectPropertyValue, "test misconfigured, should not expect property value if no update expected")
1667+
assert.False(t, test.ExpectPropertyPresent, "test misconfigured, should not expect property presence if no update expected")
15771668
}
15781669
})
15791670
}

0 commit comments

Comments
 (0)