Allow ZoneIDFilter for Cloudflare #1494

james-callahan · 2020-04-08T04:47:13Z

This PR is a rebased and cleaned up #1307

This commit should help #1127. While users in the past were able to
define ZoneIDFilter for this provider, it did not actually do anything
under the hood.

In this case, we're changing Zones() to iterate over the provided
zoneIDs and return only those zones.

I would have also done this for domainFilter, but unfortunately the
CloudFlare API requires that in order to list zones (and find them by
name) that you have "all" permissions, which seems silly. After talking
to their support, this is probably the best way to do this.

k8s-ci-robot · 2020-04-08T04:47:21Z

Welcome @james-callahan!

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. 😃

james-callahan · 2020-04-08T04:48:59Z

/assign @hjacobs

drGrove · 2020-04-08T05:05:27Z

Here's an image with the changes. i built it as I needed it for a project and it works as intended: https://hub.docker.com/layers/drgrove/external-dns/0.7.1_dev1/images/sha256-27b1618cb574ec6a3a79cec4ba4e909c2b1d0c75f399851f1092bdaa54abc334?context=repo

james-callahan · 2020-04-08T05:09:55Z

CI failure seems unrelated:

$ make lint
golangci-lint run --timeout=5m ./...
provider/coredns.go:33: File is not `goimports`-ed (goimports)
	etcdcv3 "go.etcd.io/etcd/clientv3"
Makefile:34: recipe for target 'lint' failed
make: *** [lint] Error 1
The command "make lint" exited with 2.

njuettner · 2020-04-08T10:56:33Z

Thanks, can you please rebase from master again, should be fixed now 👍.

njuettner

Added some comments, PTAL.

njuettner · 2020-04-08T13:18:09Z

provider/cloudflare.go

@@ -150,6 +155,26 @@ func (p *CloudFlareProvider) Zones(ctx context.Context) ([]cloudflare.Zone, erro
 	result := []cloudflare.Zone{}
 	p.PaginationOptions.Page = 1

+	// if there is a zoneIDfilter configured


I don't think this is needed, the code already explains it so I would drop the comments

(note: I didn't write this: the author of #1307 did):

The following line that notes that empty strings are used by tests is informative.

Ah I see my bad, I would still like to drop those lines. Maybe only keep the second ... blank string for unit tests ...

njuettner · 2020-04-08T13:37:58Z

provider/cloudflare.go

@@ -150,6 +155,26 @@ func (p *CloudFlareProvider) Zones(ctx context.Context) ([]cloudflare.Zone, erro
 	result := []cloudflare.Zone{}
 	p.PaginationOptions.Page = 1

+	// if there is a zoneIDfilter configured
+	// && if the filter isnt just a blank string (used in tests)
+	if len(p.zoneIDFilter.zoneIDs) > 0 && p.zoneIDFilter.zoneIDs[0] != "" {


Maybe you could add a new method ZoneDetailsByDomainFilter which includes your code.

or ListZonesByIDFilter

I tried to split this off (and create functions that each returned iterators) but my minimal knowledge of Go wasn't enough to get things working.

Splitting off just the contents of this branch didn't seem to improve readability.

njuettner · 2020-04-08T13:47:44Z

provider/cloudflare.go

-				continue
-			}
-
-			if !p.zoneIDFilter.Match(zone.ID) {


Just to understand it a bit better, this is dropped because it's not working?

Dropped because if there was any zoneIDFilter then the above branch would be taken and this code is unreachable.

sheerun · 2020-04-16T09:50:23Z

Cheering for this to be merged. Right now I need to issue access token that has access to read all zones

…lter Allow ZoneIDFilter for Cloudflare

k8s-ci-robot · 2020-05-21T04:57:32Z

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.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

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.

james-callahan · 2020-05-21T05:40:19Z

/assign @sheerun

sheerun · 2020-05-21T10:08:00Z

@james-callahan Please sign CLA

sheerun · 2020-05-21T10:44:45Z

Or is it @dmizelle that haven't signed?

james-callahan · 2020-05-21T10:55:12Z

cla check passed: it is dmizelle's code.

sheerun · 2020-05-21T11:07:32Z

I need to consult kubernetes team what to do in cases like this...

sheerun · 2020-05-21T12:06:34Z

Two things smell bad here:

There's no need to define ZoneDetails on interface
If some zoneId doesn't exist, none are processed, instead it should be skipped with error message

This commit _should_ help kubernetes-sigs#1127. While users in the past were able to define ZoneIDFilter for this provider, it did not actually do anything under the hood. In this case, we're changing Zones() to iterate over the provided zoneIDs and return only those zones. I would have also done this for domainFilter, but unfortunately the CloudFlare API requires that in order to list zones (and find them by name) that you have "all" permissions, which seems silly. After talking to their support, this is probably the best way to do this. Signed-off-by: James Callahan <jamescallahan@bitgo.com>

james-callahan · 2020-05-21T12:29:19Z

There's no need to define ZoneDetails on interface

Why not?

If some zoneId doesn't exist, none are processed, instead it should be skipped with error message

Fixed.

sheerun · 2020-05-22T12:04:11Z

I thought that ZoneList supports multiple domains, but it's probably not the case: https://community.cloudflare.com/t/bug-zone-detail-by-name-requires-zone-list-permission/128042/15

sheerun · 2020-05-22T12:07:25Z

/approve

k8s-ci-robot · 2020-05-22T12:07:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: james-callahan, sheerun

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:

~~provider/cloudflare/OWNERS~~ [sheerun]

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

sheerun · 2020-05-22T12:08:25Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 8, 2020

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 8, 2020

k8s-ci-robot requested review from linki and Raffo April 8, 2020 04:47

k8s-ci-robot assigned hjacobs Apr 8, 2020

james-callahan force-pushed the zoneid-filter branch from be5cd89 to adb8143 Compare April 8, 2020 11:52

njuettner requested changes Apr 8, 2020

View reviewed changes

bit-project-de approved these changes Apr 16, 2020

View reviewed changes

sheerun added a commit to sheerun/external-dns that referenced this pull request Apr 16, 2020

Merge pull request kubernetes-sigs#1494 from james-callahan/zoneid-fi…

4059225

…lter Allow ZoneIDFilter for Cloudflare

sheerun added a commit to sheerun/external-dns that referenced this pull request Apr 16, 2020

Merge pull request kubernetes-sigs#1494 from james-callahan/zoneid-fi…

d51a878

…lter Allow ZoneIDFilter for Cloudflare

sheerun mentioned this pull request Apr 22, 2020

Cloudflare provider causes repeated UPDATE #1463

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 5, 2020

james-callahan force-pushed the zoneid-filter branch from adb8143 to d0a05c6 Compare May 21, 2020 04:57

k8s-ci-robot removed the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 21, 2020

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 21, 2020

james-callahan force-pushed the zoneid-filter branch from d0a05c6 to 87dce22 Compare May 21, 2020 04:58

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 21, 2020

james-callahan force-pushed the zoneid-filter branch from 87dce22 to ae5e894 Compare May 21, 2020 05:29

k8s-ci-robot assigned sheerun May 21, 2020

james-callahan force-pushed the zoneid-filter branch from ae5e894 to 60d4ef2 Compare May 21, 2020 12:29

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 22, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2020

k8s-ci-robot merged commit 4f6ba41 into kubernetes-sigs:master May 22, 2020

james-callahan deleted the zoneid-filter branch May 25, 2020 01:32

loozhengyuan mentioned this pull request Aug 13, 2020

Cloudflare requires API token granted to all zones even with --domain-filter #1719

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow ZoneIDFilter for Cloudflare #1494

Allow ZoneIDFilter for Cloudflare #1494

james-callahan commented Apr 8, 2020

k8s-ci-robot commented Apr 8, 2020

james-callahan commented Apr 8, 2020

drGrove commented Apr 8, 2020 •

edited

Loading

james-callahan commented Apr 8, 2020

njuettner commented Apr 8, 2020

njuettner left a comment

njuettner Apr 8, 2020

james-callahan Apr 9, 2020

njuettner Apr 9, 2020

njuettner Apr 8, 2020

njuettner Apr 8, 2020

james-callahan Apr 10, 2020

njuettner Apr 8, 2020

james-callahan Apr 10, 2020

sheerun commented Apr 16, 2020

k8s-ci-robot commented May 21, 2020

james-callahan commented May 21, 2020

sheerun commented May 21, 2020

sheerun commented May 21, 2020

james-callahan commented May 21, 2020

sheerun commented May 21, 2020

sheerun commented May 21, 2020

james-callahan commented May 21, 2020 •

edited

Loading

sheerun commented May 22, 2020

sheerun commented May 22, 2020

k8s-ci-robot commented May 22, 2020

sheerun commented May 22, 2020

Allow ZoneIDFilter for Cloudflare #1494

Allow ZoneIDFilter for Cloudflare #1494

Conversation

james-callahan commented Apr 8, 2020

k8s-ci-robot commented Apr 8, 2020

james-callahan commented Apr 8, 2020

drGrove commented Apr 8, 2020 • edited Loading

james-callahan commented Apr 8, 2020

njuettner commented Apr 8, 2020

njuettner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sheerun commented Apr 16, 2020

k8s-ci-robot commented May 21, 2020

james-callahan commented May 21, 2020

sheerun commented May 21, 2020

sheerun commented May 21, 2020

james-callahan commented May 21, 2020

sheerun commented May 21, 2020

sheerun commented May 21, 2020

james-callahan commented May 21, 2020 • edited Loading

sheerun commented May 22, 2020

sheerun commented May 22, 2020

k8s-ci-robot commented May 22, 2020

sheerun commented May 22, 2020

drGrove commented Apr 8, 2020 •

edited

Loading

james-callahan commented May 21, 2020 •

edited

Loading