feat(cloudflare): custom hostname and fix apex

[mrozentsvayg]

Description

Associates and manages Cloudflare custom hostnames with external-dns.alpha.kubernetes.io/cloudflare-custom-hostname annotations.

Checklist

• Unit tests updated
• End user documentation updated

[k8s-ci-robot]

Hi @mrozentsvayg. 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.

[mrozentsvayg]

There are a couple of changes not directly related to the cloudflare custom hostnames support:

• the regional hostnames cloudflare API currently requires superadmin or admin role; submitChanges() always errors out on any change.Action == cloudFlareUpdate if the api key is more restrictive, even if the regional hostnames are not being used; To address it, proposing to only call updateDataLocalizationRegionalHostnameParams() if regionalHostnameParam.RegionKey is set;
• the commit e8bda89 is reverted, because UpdateDataLocalizationRegionalHostname doesn't have to be a part of more common TestCloudflareComplexUpdate test;
  the test case is not using RegionalKey for UpdateDataLocalizationRegionalHostname and should not expect a change.

[mloiseleur]

@AndrewCharlesHay Would you please review this PR ? Wdyt of this approach ?

[mloiseleur]

/ok-to-test

[AndrewCharlesHay]

LGTM!

[AndrewCharlesHay]

Can you rename this to listCustomHostnamesWithAutoPagination to match the naming of listDNSRecordsWithAutoPagination

[mrozentsvayg]

@AndrewCharlesHay done!

it stemmed from the API / cloudflare-go library method name CustomHostnames (https://github.com/cloudflare/cloudflare-go/blob/v0/custom_hostname.go#L222) and I agree that the internal helper function doesn't have to follow it.
The library naming inconsistency is only v0 specific, newer cloudflare SDK versions drastically addressed it.

[AndrewCharlesHay]

Ohh that makes sense. Thanks for updating it!

[AndrewCharlesHay]

Do you want to make the error statement something like Failed to add replacement custom hostname so that it is different and more specific to the error message on line 405

[mrozentsvayg]

All possible scenarios:

• adding record (DNS along with the custom hostname) - "failed to create"
• deleting record - "failed to delete"
• updating record:
  • custom hostname didn't not exist before - "failed to add"
  • custom hostname exists and needs to be removed - "failed to remove"
  • custom hostname needs to be changed (cloudflare doesn't allow to rename the existing hostname, recreate only):
    • "failed to remove"
    • "failed to add"

I couldn't come up with a clear message for the last 2, besides "failed to add/remove custom hostname on replace" or "failed to add custom hostname on update".
Do you think it would it be less confusing? I would be happy to rephrase it.

[AndrewCharlesHay]

Yeah, I think just adding the word replacement would differentiate it from line 405 and let them know that it was in the process of replacing the record and is probably in a bad state now since it deleted the record and couldn't recreate the record.

[mrozentsvayg]

updated, hope that helps, thanks!

[AndrewCharlesHay]

Looks good thanks!

[ivankatliarchuk]

Most likely same requirement for Setting cloudflare-custom-hostname, as I was not able to use non Admin role.

[mrozentsvayg]

It's documented in the new Setting cloudflare-custom-hostname section:

Requires Cloudflare for SaaS product and "SSL and Certificates" API permission.

[ivankatliarchuk]

Suggested change

	v, exists = annotations[CloudflareCustomHostnameKey]
	if exists {
	providerSpecificAnnotations = append(providerSpecificAnnotations, endpoint.ProviderSpecificProperty{
	Name: CloudflareCustomHostnameKey,
	Value: v,
	})
	}
	if v, exists := annotations[CloudflareCustomHostnameKey]; exists {
	providerSpecificAnnotations = append(providerSpecificAnnotations, endpoint.ProviderSpecificProperty{
	Name: CloudflareCustomHostnameKey,
	Value: v,
	})
	}

[ivankatliarchuk]

Looks like unit test is missing as well for this code

[mrozentsvayg]

Would you please elaborate on that?
All added functionality is based on the single external-dns.alpha.kubernetes.io/cloudflare-custom-hostname annotation, the new test function contains all possible test cases (add, replace, remove) with it:
https://github.com/kubernetes-sigs/external-dns/pull/5087/files#diff-537daf60acab77454c1d11e74a7076d095600e9b67cbf3541782c569aa162dbfR1501

Please let me know if i'm missing something, thanks!

[ivankatliarchuk]

Need to ensure that records[0].Proxied is not nil before dereferencing it

[ivankatliarchuk]

Could be something like that

proxied := false
if records[0].Proxied != nil {
	proxied = *records[0].Proxied
}
ep.WithProviderSpecific(source.CloudflareProxiedKey, strconv.FormatBool(proxied))

[mrozentsvayg]

added
(applicable to the existing code as well)

[ivankatliarchuk]

Suggested change

	if _, ok := customOriginServers[records[0].Name]; ok {
	ep.WithProviderSpecific(source.CloudflareCustomHostnameKey, customOriginServers[records[0].Name])
	}
	if customHostname, ok := customOriginServers[records[0].Name]; ok {
	ep.WithProviderSpecific(source.CloudflareCustomHostnameKey, customHostname)
	}

[mrozentsvayg]

my bad, thanks
not even sure why i committed it like that

[ivankatliarchuk]

it is possible for records[0] to cause an out-of-index exception if the records slice is empty.

we most likely do need

if len(records) == 0 {
    return endpoints
}

[mrozentsvayg]

added
(applicable to the existing code as well)

[ivankatliarchuk]

ep could be null, we should have something like

if ep == nil {
  continue
}

somewhere

[mrozentsvayg]

added
(applicable to the existing code as well)

[ivankatliarchuk]

Is this function only used in tests? If YES, could we move it to cloudflare_test.go?

[mrozentsvayg]

in this case we just do not need the original version.
renaming groupByNameAndTypeWithCh to groupByNameAndTypeWithCustomHostnames and using it directly from the single cloudflare_test.go ref

[ivankatliarchuk]

#5087 (comment)
This code is not coverade with testing. How I knew it? The source_test.go no changes

And I just did a double check with code coverage. I thinks this is the last thing that needs resolving.

[mrozentsvayg]

Right.
Currently source_test.go is only covering A, AAAA, CNAME records validator function suitableType() (with the logic if not ipv4 address and not ipv6 address, then it's CNAME) and TTL validator getTTLFromAnnotations() (which makes sense, because the code implements ttl value parser).
Neither existing provider specific annotations (getProviderSpecificAnnotations()), nor anything else implemented in source.go, besides the TTL annotations (including the very hostnameAnnotationKey = "external-dns.alpha.kubernetes.io/hostname" in getHostnamesFromAnnotations()) are having unit tests. I believe with the reasoning that the string values are just being used as it is and there's basically no code to cover with tests.

However I used the occasion to cover with unit tests all the logic implemented in getProviderSpecificAnnotations().
If we need to add more unit tests for more source.go functions, I believe it should not be in this PR scope.

[ivankatliarchuk]

/lgtm

[ivankatliarchuk]

cc @mloiseleur for review

[mloiseleur]

@AndrewCharlesHay Do you want to do a final review before merge ?

[AndrewCharlesHay]

/lgtm

[k8s-ci-robot]

@AndrewCharlesHay: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

[mloiseleur]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ivankatliarchuk, 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

[mloiseleur]

Many thanks for this great PR 👍.
@AndrewCharlesHay @mrozentsvayg if you have time to help us on other PRs or issues, you are more than welcome !