-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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.
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
feat: added support per dns cf proxy #4325
base: master
Are you sure you want to change the base?
feat: added support per dns cf proxy #4325
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @Tazer! |
Hi @Tazer. 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 Once the patch is verified, the new status will be reflected by the 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/test-infra repository. |
…dpoints using the same ProviderSpecific
Thanks for this PR. The test looks good.
I'm unsure to understand why you needed to make a deepcopy in Would you please detail what is the behavior you observed without the deepcopy ? |
/ok-to-test |
First of all thanks for checking it out. @mloiseleur will look at documentation and additional test tomorrow. Let me explain the deepcopy need.As we are using this functionality:
on a kubernetes service. The following code will run:
so the tricky part here is that source: https://github.com/kubernetes-sigs/external-dns/blob/master/source/service.go#L395 And in the cloudflare provider we have this code:
And it will loop both of the endpoints. but on the first itirate it will call
Without source: https://github.com/kubernetes-sigs/external-dns/blob/master/provider/cloudflare/cloudflare.go#L386 So the |
@mloiseleur added docs & additional tests also explained the reasoning on |
Thanks @Tazer, it's clear. You made quite a good job in order to avoid a breaking change 👍 . I mean, sometimes, avoiding a breaking change may cause more harm than good. In a fictional world where In the real world where Wdyt about:
cc @cxuu @aoz-turk @johngmyers @danie1sullivan as last PR authors on cloudflare |
@mloiseleur I think we can change it from bool to string if this helps. For backwards compatibility we could try to cast as bool and if it's not ok we assume a string. @Tazer wdyt? |
Description
We have a use case where we want to have 1 domain proxied through cloudflare and another one not proxied via cloudflare but using the same
service
So in this PR we are adding the possibility to pass a domain list in the
external-dns.alpha.kubernetes.io/cloudflare-proxied
So it's still possible to pass
external-dns.alpha.kubernetes.io/cloudflare-proxied: true/false
but also
external-dns.alpha.kubernetes.io/cloudflare-proxied: example.com,bar.com
The setup we are going todo is like this:
Checklist