Skip to content

test: add tests for cloudflare provider #5248

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
merged 4 commits into from
May 4, 2025
+120 −49
Merged

test: add tests for cloudflare provider #5248

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
863dda0
Improve cloudflare code coverage
natitomattis Apr 2, 2025
8942385
refactor TestCloudflareProvider
natitomattis Apr 8, 2025
18c867f
go lint
natitomattis Apr 8, 2025
e8d9f30
fix data race
natitomattis Apr 8, 2025
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
169 changes: 120 additions & 49 deletions provider/cloudflare/cloudflare_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ type mockCloudFlareClient struct {
Records map[string]map[string]cloudflare.DNSRecord
Actions []MockAction
listZonesError error
zoneDetailsError error
listZonesContextError error
dnsRecordsError error
customHostnames map[string][]cloudflare.CustomHostname
Expand Down Expand Up @@ -384,6 +385,10 @@ func (m *mockCloudFlareClient) ListZonesContext(ctx context.Context, opts ...clo
}

func (m *mockCloudFlareClient) ZoneDetails(ctx context.Context, zoneID string) (cloudflare.Zone, error) {
if m.zoneDetailsError != nil {
return cloudflare.Zone{}, m.zoneDetailsError
}

for id, zoneName := range m.Zones {
if zoneID == id {
return cloudflare.Zone{
Expand Down Expand Up @@ -771,6 +776,24 @@ func TestCloudflareZones(t *testing.T) {
assert.Equal(t, "bar.com", zones[0].Name)
}

// test failures on zone lookup
func TestCloudflareZonesFailed(t *testing.T) {

client := NewMockCloudFlareClient()
client.zoneDetailsError = errors.New("zone lookup failed")

provider := &CloudFlareProvider{
Client: client,
domainFilter: endpoint.NewDomainFilter([]string{"bar.com"}),
zoneIDFilter: provider.NewZoneIDFilter([]string{"001"}),
}

_, err := provider.Zones(context.Background())
if err == nil {
t.Errorf("should fail, %s", err)
}
}

func TestCloudFlareZonesWithIDFilter(t *testing.T) {
client := NewMockCloudFlareClient()
client.listZonesError = errors.New("shouldn't need to list zones when ZoneIDFilter in use")
Expand Down Expand Up @@ -882,64 +905,88 @@ func TestCloudflareRecords(t *testing.T) {
}

func TestCloudflareProvider(t *testing.T) {
_ = os.Setenv("CF_API_TOKEN", "abc123def")
_, err := NewCloudFlareProvider(
endpoint.NewDomainFilter([]string{"bar.com"}),
provider.NewZoneIDFilter([]string{""}),
false,
true,
5000,
"",
CustomHostnamesConfig{Enabled: false})
if err != nil {
t.Errorf("should not fail, %s", err)
var err error

type EnvVar struct {
Key string
Value string
}

_ = os.Unsetenv("CF_API_TOKEN")
tokenFile := "/tmp/cf_api_token"
if err := os.WriteFile(tokenFile, []byte("abc123def"), 0o644); err != nil {
t.Errorf("failed to write token file, %s", err)
}
_ = os.Setenv("CF_API_TOKEN", tokenFile)
_, err = NewCloudFlareProvider(
endpoint.NewDomainFilter([]string{"bar.com"}),
provider.NewZoneIDFilter([]string{""}),
false,
true,
5000,
"",
CustomHostnamesConfig{Enabled: false})
if err != nil {
t.Errorf("should not fail, %s", err)
}

_ = os.Unsetenv("CF_API_TOKEN")
_ = os.Setenv("CF_API_KEY", "xxxxxxxxxxxxxxxxx")
_ = os.Setenv("CF_API_EMAIL", "[email protected]")
_, err = NewCloudFlareProvider(
endpoint.NewDomainFilter([]string{"bar.com"}),
provider.NewZoneIDFilter([]string{""}),
false,
true,
5000,
"",
CustomHostnamesConfig{Enabled: false})
if err != nil {
t.Errorf("should not fail, %s", err)
testCases := []struct {
Name string
Environment []EnvVar
ShouldFail bool
}{
{
Name: "use_api_token",
Environment: []EnvVar{
{Key: "CF_API_TOKEN", Value: "abc123def"},
},
ShouldFail: false,
},
{
Name: "use_api_token_file_contents",
Environment: []EnvVar{
{Key: "CF_API_TOKEN", Value: tokenFile},
},
ShouldFail: false,
},
{
Name: "use_email_and_key",
Environment: []EnvVar{
{Key: "CF_API_KEY", Value: "xxxxxxxxxxxxxxxxx"},
{Key: "CF_API_EMAIL", Value: "[email protected]"},
},
ShouldFail: false,
},
{
Name: "no_use_email_and_key",
Environment: []EnvVar{},
ShouldFail: true,
},
{
Name: "use_credentials_in_missing_file",
Environment: []EnvVar{
{Key: "CF_API_TOKEN", Value: "file://abc"},
},
ShouldFail: true,
},
{
Name: "use_credentials_in_missing_file",
Environment: []EnvVar{
{Key: "CF_API_TOKEN", Value: "file:/tmp/cf_api_token"},
},
ShouldFail: false,
},
}

_ = os.Unsetenv("CF_API_KEY")
_ = os.Unsetenv("CF_API_EMAIL")
_, err = NewCloudFlareProvider(
endpoint.NewDomainFilter([]string{"bar.com"}),
provider.NewZoneIDFilter([]string{""}),
false,
true,
5000,
"",
CustomHostnamesConfig{Enabled: false})
if err == nil {
t.Errorf("expected to fail")
for _, tc := range testCases {
t.Run(tc.Name, func(t *testing.T) {
for _, env := range tc.Environment {
t.Setenv(env.Key, env.Value)
}

_, err = NewCloudFlareProvider(
endpoint.NewDomainFilter([]string{"bar.com"}),
provider.NewZoneIDFilter([]string{""}),
false,
true,
5000,
"",
CustomHostnamesConfig{Enabled: false})
if err != nil && !tc.ShouldFail {
t.Errorf("should not fail, %s", err)
}
if err == nil && tc.ShouldFail {
t.Errorf("should fail, %s", err)
}
})

}
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that this pattern existed before this PR, but it's really taking some time to realize that the difference is only in the envs, while the NewCloudFlareProvider() arguments are identical.
To improve the readability could we possibly combine all those tests into a loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a small refactor in this test, so we can define a map with all the env changes and the expected test result.

}

Expand Down Expand Up @@ -1012,6 +1059,30 @@ func TestCloudflareApplyChanges(t *testing.T) {
}
}

func TestCloudflareDryRunApplyChanges(t *testing.T) {
changes := &plan.Changes{}
client := NewMockCloudFlareClient()

provider := &CloudFlareProvider{
Client: client,
DryRun: true,
}
changes.Create = []*endpoint.Endpoint{{
DNSName: "new.bar.com",
Targets: endpoint.Targets{"target"},
}}
err := provider.ApplyChanges(context.Background(), changes)
if err != nil {
t.Errorf("should not fail, %s", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we could use this test to make sure it really doesn't make changes in dry run, not only hitting the continue line to increase the coverage score.
I would suggest adding:

	ctx := context.Background()
	err := provider.ApplyChanges(ctx, changes)
	if err != nil {
		t.Errorf("should not fail, %s", err)
	}
	records, err := provider.Records(ctx)
	if err != nil {
		t.Errorf("should not fail, %s", err)
	}
	assert.Equal(t, 0, len(records), "should not have any records")

does it make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, added

ctx := context.Background()
records, err := provider.Records(ctx)
if err != nil {
t.Errorf("should not fail, %s", err)
}
assert.Equal(t, 0, len(records), "should not have any records")
}

func TestCloudflareApplyChangesError(t *testing.T) {
changes := &plan.Changes{}
client := NewMockCloudFlareClient()
Expand Down
Loading