feat(cloudflare): change defaultCA from google to empty string

[henryjarend]

Change the default Certificate Authority in custom hostnames config to be an empty string.

What does it do ?

Changing the default for CloudflareCustomHostnamesCertificateAuthority and adding an allowable empty string removes the configuration in the customHostnamesConfig which allows for custom hostnames to be used on more plan types than just enterprise.

Motivation

We don't have a cloudflare enterprise plan however we do have custom hostnames enabled and we'd like to be able to use the feature in external-dns without requiring a more expensive plan. This should close this issue

More

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

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: henryjarend / name: Henry Arend (517fc3b, 12e82b4, 3807e39, 426ea7e, 7d307ad, 58f7601, 29a6345, a9d9079, 21590e2, 9e0f44f, f78ede1, b55a04c, 285d976, a1944d1, 34e9aea, 7931d8d)

[k8s-ci-robot]

Welcome @henryjarend!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

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

[mloiseleur]

/ok-to-test
@ThePooN Can you confirm it fixes your issue ?
@mrozentsvayg @vflaux Any comments on this PR ?

[vflaux]

Changing the default might be a breaking change for some users, don't you think?
I don't use this feature so I don't really know how this works.

[henryjarend]

@vflaux so right now if the flag is omitted it'll default to google, however in the API itself if you omit that option it will let Cloudflare decide. I'll have to test if you add the flag but don't supply an option what that will result in. I wasn't sure of a better way to accomplish it but if you have any suggestions I'm all ears!

[henryjarend]

@vflaux so right now if the flag is omitted it'll default to google, however in the API itself if you omit that option it will let Cloudflare decide. I'll have to test if you add the flag but don't supply an option what that will result in. I wasn't sure of a better way to accomplish it but if you have any suggestions I'm all ears!

quickly testing this locally, if you pass in just the flag it will error saying FATA[0000] flag parsing error: expected argument for flag '--cloudflare-custom-hostnames-certificate-authority'
This would mean that it would have some potential to break environments if people were passing in that argument but not selecting a CA. Previously that would have defaulted to google but let me see if I can figure out how to get that to still keep that default if the flag is given but no CA is selected

[mrozentsvayg]

@mrozentsvayg @vflaux Any comments on this PR ?

This is a good one and we need it - the non-enterprise accounts got affected after adding --cloudflare-custom-hostnames-certificate-authority configuration (cloudflare api change is also possible though).
I checked it locally with a free cloudflare account and the implemented logic works as before, allowing for the custom hostnames to be created.

I expect the PR to be safe, as the configuration parameter default/value change (from "google" to anything) should not update any existing custom hostnames and only get used when creating new custom hostnames.

@henryjarend
I would suggest to use "none" instead of "" everywhere in the PR for the consistency.
Additionally, please note this line:
https://github.com/kubernetes-sigs/external-dns/blob/master/pkg/apis/externaldns/types_test.go#L437
"EXTERNAL_DNS_CLOUDFLARE_CUSTOM_HOSTNAMES_CERTIFICATE_AUTHORITY": "google",
updating it should help to pass the tests.

[henryjarend]

@mrozentsvayg thanks for the feedback. I talked with a colleague about using "none" vs "" and ended up landing on "" because it seemed strange to say that you weren't using a CA (ie none) vs not specifying a CA (ie ""). Happy to change this if you feel it would match more with the rest of the k8s ecosystem.

I'm not sure I quite follow for the environment variable change. I did update the tests so that it expects no CA to be selected when that flag is omitted but it is possible to override it by passing in google or another specified CA

Thanks again for your time.

[mrozentsvayg]

@mrozentsvayg thanks for the feedback. I talked with a colleague about using "none" vs "" and ended up landing on "" because it seemed strange to say that you weren't using a CA (ie none) vs not specifying a CA (ie ""). Happy to change this if you feel it would match more with the rest of the k8s ecosystem.

Could be done either way, but consistently everywhere. As currently implemented - the none is mentioned in all the text descriptions, whereas the code expects to have "" instead.
Style wise, Default("") is currently used in the external-dns flags for the user provided values, in contrast to the predefined options.
There's also well known convention of using None to indicate the absence of the value, while "" means an empty string value. It's rather coming from python though, there's nothing like that in golang.
I personally would prefer a distinct option (like none), also documenting that it would skip specifying the CA in the custom hostnames Cloudflare API and the only option to be used with the free tier.

I'm not sure I quite follow for the environment variable change. I did update the tests so that it expects no CA to be selected when that flag is omitted but it is possible to override it by passing in google or another specified CA

there are 3 related tests in types_test.go - first is that the default options match to those listed in minimalConfig var;
2nd and 3rd that overriddenConfig.CloudflareCustomHostnamesCertificateAuthority matches to the value set by --cloudflare-custom-hostnames-certificate-authority= command line flag and EXTERNAL_DNS_CLOUDFLARE_CUSTOM_HOSTNAMES_CERTIFICATE_AUTHORITY env var respectively.

[ThePooN]

@ThePooN Can you confirm it fixes your issue ?

works perfectly 👍

@henryjarend thanks a lot!

[mrozentsvayg]

/lgtm

[k8s-ci-robot]

@mrozentsvayg: 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]

/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