-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Allow setting Cloudflare proxying on a per-ingress basis #650
Allow setting Cloudflare proxying on a per-ingress basis #650
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
CLA should now be signed by both of us. |
/LGTM |
endpoint/endpoint.go
Outdated
@@ -107,6 +107,9 @@ func (t Targets) IsLess(o Targets) bool { | |||
return false | |||
} | |||
|
|||
// ProviderSpecific is a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment is incomplete. Can you please fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
endpoint/endpoint.go
Outdated
@@ -107,6 +107,9 @@ func (t Targets) IsLess(o Targets) bool { | |||
return false | |||
} | |||
|
|||
// ProviderSpecific holds configuration which is specific to individual DNS providers | |||
type ProviderSpecific map[string]string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please reconsider using list of k-v values as per kubernetes api conventions. please refer to this comment for more information #657 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a look when I'm back from holidays (end of September)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the type and updated the CRD docs. Could you take a look specifically if the changes to these docs are correct?
@shashidharatd Do you mind having a look again? 🙂 |
@shashidharatd Any chance you could take a look? 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eswets, apologies, somehow i missed earlier notifications.
except the following comment, this lgtm. Also please do re-organize the commits to exclude commits like fixed comments
etc.. Thanks!
endpoint/endpoint.go
Outdated
// ProviderSpecificProperty holds the name and value of a configuration which is specific to individual DNS providers | ||
type ProviderSpecificProperty struct { | ||
Name string | ||
Value string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing json tags
@eswets sorry again for taking so long to merge this PR, do you mind rebase from the master again to solve the conflicting files? |
That should be all! |
A change to an existing |
Is there anything left to be done with this, or only a lgtm is missing? |
I'll take a look at the feedback from @unix-way tonight, as this behaviour is not intended. Having said that, I would like to have this merged to prevent having to fix all merge conflicts again. |
@eswets we are looking forward to this being merged as we recently started migrating some stuff to cloudflare. Did you find any blockers or is it ready to be merged? We really appreciate you taking the time to create the PR 👍 |
Hi, We are also looking at using Cloudflare in the future for this so it would be great if this could be pulled in to give us the option to specify the proxy is on where required. Cheers |
I've pushed a fix for supporting updates to the |
@njuettner can this be merged? |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: njuettner 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:
Approvers can indicate their approval by writing |
* Add favicon to docs * Use svg * Add relURL
The problem
Cloudflare offers the functionality to proxy all traffic through their servers. Currently, this feature can only be enabled globally, causing it to apply for all created records.
The solution
Using annotations, we can override the globally specified proxying option:
external-dns.alpha.kubernetes.io/cloudflare-proxied: "true"
This closes #370
Attribution
This PR was co-created with @amolenaar