feat(cloudflare): improve cloudflare regional hostnames implementation

[vflaux]

Description
Improves my previous implementation of Cloudflare Regional Services (aka Regional Hostnames) by:

• adding a flag to enable Regional Services feature (disabled by default, enabled if region key is set)
• supporting deletion of regional hostnames on annotation edit: removing the annotation if the default key is empty or setting it to empty string will remove the regional hostname config
• correctly supporting differences detection with cloudflare state: periodic reconcile / restart will detect external modifications on regional hostname config
• increasing tests coverage

This implementation use a call to list regional hostnames on the zone and use them to plan changes on cloudflare.

Checklist

• Unit tests updated
• End user documentation updated

[k8s-ci-robot]

Hi @vflaux. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[ivankatliarchuk]

probably worth to create cloudflare_regional file and move logic there.

Would you be able to share manifests files and all arguments you are using and how you do integration testing?

[vflaux]

@ivankatliarchuk I moved the logic & tests to their dedicated file as you suggested.

I use the following flags to test :

--events
--source=service
--source=ingress
--policy=sync
--registry=txt
--txt-owner-id=foobar
--txt-prefix=%{record_type}-
--provider=cloudflare
--cloudflare-proxied
--cloudflare-regional-hostnames
--cloudflare-region-key=xx
--ingress-class=cloudflare

I run external-dns with --cloudflare-region-key flag defined and change an ingress external-dns.alpha.kubernetes.io/cloudflare-region-key annotation.
I check the result with the /zones/:zone_id/addressing/regional_hostnames endpoint on clouflare api.

Tests :

Default regional key	Ingress regional key	Expected regional hostname key	Result
undefined	undefined	none	ok
undefined	""	none	ok
undefined	"eu"	"eu"	ok
"us"	undefined	"us"	ok
"us"	""	none	ok
"us"	"eu"	"eu"	ok

[ivankatliarchuk]

A lot of stuff, but not sure if there is a way to split pull request.

Do you think you could move to separate PRs

• correctly supporting differences detection with cloudflare state: periodic reconcile / restart will detect external modifications on regional hostname config
• increasing tests coverage

This may take time to get an approval as is

[ivankatliarchuk]

/ok-to-test

[vflaux]

I opened a new PR with some changes from this one: #5329
I'm not sure if I can split this any further. The improvements in test coverage are related to the new code in this PR.

[ivankatliarchuk]

let's fix linter so then we could review the rest

Could you also share manifest and end-2-end smoke tests results

[ivankatliarchuk]

Would you be able to share the Kubernetes manifests and the results from your smoke tests?

[mloiseleur]

@vflaux Have you tested this PR on a real environment ?

We will make a new release next week. If you can fix the ci issue on linter, maybe it can be a part of this release.

@ivankatliarchuk Anything left on your side ?

[vflaux]

@ivankatliarchuk i've added new tests to cover failures cases.

@mloiseleur I've run a previous version of this PR on testing & production environments but I now use the official build again.
This was to cleanup all the regional hostnames created by external-dns as the current implementation can't do that. That's the main reason for this PR actually.
I still need to do tests on a real environment with my latest changes. I'll try to do this this weekend.

[ivankatliarchuk]

/lgtm

[ivankatliarchuk]

Just a quick note for the future — I know we created a slice for this PR already, but next time this approach could be hardly acceptable. There are too many changes packed into one: slight refactoring, new features, and behavior improvements all in a single PR. This increases the risk significantly. A lot can go wrong in a scenario like this. From PC looks manageable, from phone - not really 😳

The bright side is the test coverage bump. Will see where or not we could trust or unit test framework ;-)

Worth to update the PR description if it's not up to date.

[mloiseleur]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mloiseleur

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [mloiseleur]

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