chore(cloudflare): migrate customhostname to v5 #5891

AndrewCharlesHay · 2025-10-06T21:49:49Z

What does it do ?

Migrate customhostname to v5

Motivation

Closes #5540

More

Yes, this PR title follows Conventional Commits
Yes, I added unit tests
Yes, I updated end user documentation accordingly

k8s-ci-robot · 2025-10-06T21:49:57Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign szuecs for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

coveralls · 2025-10-06T21:57:47Z

Pull Request Test Coverage Report for Build 18786857940

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 0 changed or added relevant lines in 0 files are covered.
111 unchanged lines in 7 files lost coverage.
Overall coverage increased (+0.1%) to 78.695%

Files with Coverage Reduction	New Missed Lines	%
apis/externaldns/types.go	1	99.71%
execute.go	3	64.89%
txt.go	10	95.28%
zz_generated.deepcopy.go	11	0.0%
endpoint.go	12	90.42%
coredns/coredns.go	13	92.25%
cloudflare/cloudflare.go	61	91.17%

Totals
Change from base Build 18242454154:	0.1%
Covered Lines:	15883
Relevant Lines:	20183

💛 - Coveralls

docs/tutorials/cloudflare.md

Moved all tests from cloudflare_customhostname_test.go into cloudflare_test.go to ensure proper coverage tracking by coveralls. Tests maintain 92.4% coverage.

Co-authored-by: Michel Loiseleur <[email protected]>

mloiseleur · 2025-10-09T06:01:56Z

@vflaux @mrozentsvayg Any comment on this PR ? Do you think you can review it ?

go.mod

provider/cloudflare/cloudflare.go

vflaux · 2025-10-09T15:39:35Z

provider/cloudflare/cloudflare.go

+	if strings.Contains(errStr, "exceeded available rate limit retries") ||
+		strings.Contains(errStr, "rate limit") ||
+		strings.Contains(errStr, "429") {


I think we should handle this according to the doc here: https://github.com/cloudflare/cloudflare-go?tab=readme-ov-file#errors

var apierr *cloudflare.Error if errors.As(err, &apierr) { if apierr.StatusCode == http.StatusTooManyRequests { } }

This was implemented

so why do we have this code here?

Rate limit errors are not returned as ErrorTypeRateLimit cloudflare/cloudflare-go#4155 talks about the v0 version of the library which we don't use after merging your PR.

the first string.Contains is not needed if you have the second one.

Sorry forgot to commit it. It's gone now

provider/cloudflare/cloudflare.go

k8s-ci-robot · 2025-10-09T15:47:49Z

@vflaux: changing LGTM is restricted to collaborators

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Co-authored-by: vflaux <[email protected]>

…Flare API - Change CustomHostnames method signature to return autoPager[CustomHostnameListResponse] instead of processed []CustomHostname for better API matching and testability - Keep listAllCustomHostnames helper function for callers that need processed results - Update mock implementation to return autoPager with realistic CloudFlare API behavior - All tests continue to pass with improved interface design Addresses PR feedback about method signatures being closer to CloudFlare API for better mocking.

- Handle CloudFlare v5 SDK errors according to official documentation - Use errors.As() to check for cloudflare.Error type - Convert 429 (rate limit) status codes to SoftError for retry logic - Add comprehensive test coverage for v5 error handling - Use wrapper types in tests to avoid CloudFlare SDK internal nil pointer issues Addresses PR comment on line 376 requesting proper v5 error handling.

AndrewCharlesHay · 2025-10-10T17:11:58Z

How did broken tests get merged in?

vflaux · 2025-10-13T11:06:27Z

How did broken tests get merged in?

This should be fixed, see #5896.

AndrewCharlesHay · 2025-10-13T15:01:36Z

/retest

AndrewCharlesHay · 2025-10-13T19:55:19Z

@vflaux everything look good now?

vflaux

I've not review all the new tests yet.
Looks like IA generated, some clearly improve test coverage, others seems less relevant. 😄

vflaux · 2025-10-17T21:55:11Z

provider/cloudflare/cloudflare.go

+	if client == nil {
+		return nil, fmt.Errorf("failed to initialize cloudflare provider")
 	}


I's unreachable, right?

Suggested change

if client == nil {

return nil, fmt.Errorf("failed to initialize cloudflare provider")

}

vflaux · 2025-10-17T22:11:29Z

provider/cloudflare/cloudflare.go

+func (z zoneService) CreateCustomHostname(ctx context.Context, zoneID string, ch CustomHostname) error {
+	params := custom_hostnames.CustomHostnameNewParams{
+		ZoneID:   cloudflare.F(zoneID),
+		Hostname: cloudflare.F(ch.Hostname),
+	}
+
+	if ch.SSL != nil {
+		sslParams := custom_hostnames.CustomHostnameNewParamsSSL{}
+		if ch.SSL.Method != "" {
+			sslParams.Method = cloudflare.F(custom_hostnames.DCVMethod(ch.SSL.Method))
+		}
+		if ch.SSL.Type != "" {
+			sslParams.Type = cloudflare.F(custom_hostnames.DomainValidationType(ch.SSL.Type))
+		}
+		if ch.SSL.BundleMethod != "" {
+			sslParams.BundleMethod = cloudflare.F(custom_hostnames.BundleMethod(ch.SSL.BundleMethod))
+		}
+		if ch.SSL.CertificateAuthority != "" && ch.SSL.CertificateAuthority != "none" {
+			sslParams.CertificateAuthority = cloudflare.F(cloudflare.CertificateCA(ch.SSL.CertificateAuthority))
+		}
+		if ch.SSL.Settings.MinTLSVersion != "" {
+			sslParams.Settings = cloudflare.F(custom_hostnames.CustomHostnameNewParamsSSLSettings{
+				MinTLSVersion: cloudflare.F(custom_hostnames.CustomHostnameNewParamsSSLSettingsMinTLSVersion(ch.SSL.Settings.MinTLSVersion)),
+			})
+		}
+		params.SSL = cloudflare.F(sslParams)
+	}
+
+	_, err := z.service.CustomHostnames.New(ctx, params)
+	return err


Is it possible to do the same you've done with CustomHostnames() ?
Signature close to the cloudflare api & extract the build of CustomHostnameNewParams{} to another func + tests ?

vflaux · 2025-10-17T22:18:22Z

provider/cloudflare/cloudflare_test.go

+	})
+}
+
+func TestBuildCustomHostnameSSLParams(t *testing.T) {


Not sure what this is supposed to test 😄.
This doesn't call any function from this package.

vflaux · 2025-10-17T22:21:03Z

provider/cloudflare/cloudflare_test.go

+		var foundID string
+		for _, h := range hostnames {
+			if h.Hostname == "test.example.com" {
+				foundID = h.ID
+				break
+			}
+		}
+		assert.Equal(t, "ch1", foundID)


No need to loop over the list as there is only one element.

Suggested change

var foundID string

for _, h := range hostnames {

if h.Hostname == "test.example.com" {

foundID = h.ID

break

}

}

assert.Equal(t, "ch1", foundID)

assert.Equal(t, "ch1", hostnames[0].ID)

vflaux · 2025-10-17T22:26:41Z

provider/cloudflare/cloudflare_test.go

+		}
+	})
+
+	t.Run("IteratorError", func(t *testing.T) {


This is case is very similar to the next one (PartialIteratorError). I'm not sure if this one is needed?

provider/cloudflare/cloudflare_test.go

provider/cloudflare/cloudflare.go

Co-authored-by: vflaux <[email protected]>

go.mod

…rkaround no longer needed, per reviewer feedback)

AndrewCharlesHay · 2025-10-24T18:44:37Z

The tests just fail because of rate limits to coveralls

szuecs · 2025-10-31T08:58:56Z

provider/cloudflare/cloudflare.go

+		Hostname: cloudflare.F(ch.Hostname),
+	}
+
+	if ch.SSL != nil {


This smells like extract function refactoring. Put the whole branch in a function please

szuecs · 2025-10-31T09:01:58Z

Did you @AndrewCharlesHay address comments by @vflaux ?

Cleaning up things that are not useful is always great for maintaining the software.

AndrewCharlesHay added 2 commits October 6, 2025 21:28

chore(cloudflare): migrate remaining custom hostnames to v5

2d877aa

Apply linter fixes and formatting

ee6b5c4

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 6, 2025

k8s-ci-robot requested a review from ivankatliarchuk October 6, 2025 21:49

k8s-ci-robot added the provider Issues or PRs related to a provider label Oct 6, 2025

k8s-ci-robot requested a review from szuecs October 6, 2025 21:49

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 6, 2025

AndrewCharlesHay added 2 commits October 7, 2025 00:24

docs: remove pr description

2eabb3b

docs: update Cloudflare tutorial to reflect v5 SDK migration completion

e7fdab6

k8s-ci-robot added the docs label Oct 7, 2025

mloiseleur reviewed Oct 7, 2025

View reviewed changes

docs/tutorials/cloudflare.md Outdated Show resolved Hide resolved

test: consolidate custom hostname tests into main test file

1031c1b

Moved all tests from cloudflare_customhostname_test.go into cloudflare_test.go to ensure proper coverage tracking by coveralls. Tests maintain 92.4% coverage.

AndrewCharlesHay changed the title ~~Andrewcharleshay/cloudflare migration v5~~ chore(cloudflare): migrate customhostname to v5 Oct 7, 2025

docs: v0.20 update

4031774

Co-authored-by: Michel Loiseleur <[email protected]>

vflaux suggested changes Oct 9, 2025

View reviewed changes

AndrewCharlesHay and others added 6 commits October 9, 2025 20:58

fix: address comments

ab60f44

Update go.mod

de0063b

Co-authored-by: vflaux <[email protected]>

ci: remove from go mod

90235b4

Co-authored-by: vflaux <[email protected]>

style(cloudflare): make go-lint

4b955ba

AndrewCharlesHay added 2 commits October 13, 2025 17:20

test(cloudflare): add test for ssl params

d166272

chore: remove extra files

243968b

style: make lint

20b5413

vflaux mentioned this pull request Oct 14, 2025

REQUEST: New membership for @vflaux kubernetes/org#5925

Closed

11 tasks

vflaux suggested changes Oct 17, 2025

View reviewed changes

k8s-ci-robot assigned vflaux Oct 17, 2025

AndrewCharlesHay and others added 7 commits October 18, 2025 10:05

docs: update comment

4ac4988

Co-authored-by: vflaux <[email protected]>

fix: address comments

440f208

fix: remove dead code

2b88a62

fix: update test

4214531

fix: update tests

f214657

fix: update tests

70eff0f

fix: update test

f507fe5

szuecs reviewed Oct 24, 2025

View reviewed changes

go.mod Outdated Show resolved Hide resolved

AndrewCharlesHay added 2 commits October 24, 2025 17:09

ci: update utils package

0d97486

refactor(cloudflare): remove redundant rate limit error checks (v0 wo…

e91d496

…rkaround no longer needed, per reviewer feedback)

szuecs reviewed Oct 31, 2025

View reviewed changes

	if client == nil {
	return nil, fmt.Errorf("failed to initialize cloudflare provider")
	}

chore(cloudflare): migrate customhostname to v5 #5891

Are you sure you want to change the base?

chore(cloudflare): migrate customhostname to v5 #5891

Conversation

AndrewCharlesHay commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does it do ?

Motivation

More

Uh oh!

k8s-ci-robot commented Oct 6, 2025

Uh oh!

coveralls commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 18786857940

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

Uh oh!

mloiseleur commented Oct 9, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szuecs Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

k8s-ci-robot commented Oct 9, 2025

Uh oh!

AndrewCharlesHay commented Oct 10, 2025

Uh oh!

vflaux commented Oct 13, 2025

Uh oh!

AndrewCharlesHay commented Oct 13, 2025

Uh oh!

AndrewCharlesHay commented Oct 13, 2025

Uh oh!

vflaux left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrewCharlesHay commented Oct 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szuecs commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

AndrewCharlesHay commented Oct 6, 2025 •

edited

Loading

coveralls commented Oct 6, 2025 •

edited

Loading

szuecs Oct 24, 2025 •

edited

Loading

szuecs commented Oct 31, 2025 •

edited

Loading