CloudFlare as a new provider #140

njuettner · 2017-04-10T07:58:07Z

k8s-ci-robot · 2017-04-10T07:58:09Z

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://github.com/kubernetes/kubernetes/wiki/CLA-FAQ 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.

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.

ideahitme · 2017-04-10T14:38:28Z

@njuettner thanks for the PR. I think u need to sign CLA ;)

njuettner · 2017-04-11T07:39:16Z

Thx 👍 for the comment, will have a look.

linki · 2017-04-12T10:52:17Z

@njuettner Thanks for your contribution. We scheduled to review this after the v0.3 release.

Meanwhile we'd be interested in how your experience was contributing a new provider to this project. We intend to make that as easy as possible.

…e-provider

linki · 2017-04-12T12:38:46Z

@njuettner I merged in the latest master and fixed a compile error. Let's see if that makes travis happy.

njuettner · 2017-04-12T13:15:27Z

@linki thanks for taking a look.

So far it's pretty easy to implement a new provider (I was also thinking inheriting digital ocean).

The only thing what is currently driving me crazy is getting authorized. I raised a ticket for that (CLA was signed, git email is the same which I used for signing the contract).

thelinuxfoundation · 2017-04-12T17:39:45Z

bump the CLA check

ideahitme · 2017-04-13T16:27:34Z

it would be nice if you could rebase without the vendor commit, so we can review your changes without going to commits one by one :)

njuettner · 2017-04-14T12:07:19Z

Done, but CI did break because it's not updating the packages which will be required.
So new packages which I inherited in cloudflare won't be found like "github.com/cloudflare/cloudflare-go".

Maybe the .travis.yml can be changed. Before "travis_wait 20 goveralls -service=travis-ci" runs, an glide install will be triggered and makes sure new dependencies are in place?

njuettner · 2017-04-14T12:27:58Z

Something like:

before_install:
 - go get github.com/Masterminds/glide

install:
 - cd $GOPATH/src/github.com/Masterminds/glide && git checkout v0.12.3 && go install && cd -
 - glide update

linki · 2017-04-24T15:26:36Z

@njuettner You did it perfectly fine before. We currently do vendor all our dependencies. @ideahitme was just asking to leave out the vendor folder in this PR so it's easier for us to review.

Just make sure your project builds locally, don't push your vendor folder and let travis fail. Once your code looks great you commit your vendor folder and we'll wait for travis to be green.

linki · 2017-04-24T15:29:22Z

The alternative would be to not vendor but just commit glide.(yaml|lock) and then install and use glide on travis to fetch them on each build. We're currently discussing a uniform strategy to tackle this.

njuettner · 2017-04-24T15:59:27Z

@linki Sounds good to me. Thanks for your input.

linki

~~domain-filter is ignored~~

linki · 2017-06-12T14:32:36Z

Changed it to respect the domain filter flag as well. However, this needs to be tested due to unknown behaviour regarding the trailing dot.

fixed

ideahitme

nits are not of course blockers, but it'd be great to have the rest of the changes in place before merge

ideahitme · 2017-06-13T09:49:27Z

provider/cloudflare.go

+	"github.com/kubernetes-incubator/external-dns/plan"
+)
+
+// cloudFlareDNSInterface is the subset of the CloudFlare API that we actually use.  Add methods as required. Signatures must match exactly.


nit: "Interface" suffix is redundant. type cloudFlareDNS interface{} is already explicit enough

ideahitme · 2017-06-13T09:56:35Z

provider/cloudflare.go

+// NewCloudFlareProvider initializes a new CloudFlare DNS based Provider.
+func NewCloudFlareProvider(domainFilter string, dryRun bool) (Provider, error) {
+	// initialize via API email and API key and returns new API object
+	config, err := cloudflare.New(os.Getenv("CF_API_KEY"), os.Getenv("CF_API_EMAIL"))


what are the consequences of not having this defined os.Getenv("CF_API_KEY"), os.Getenv("CF_API_EMAIL") ?
IIUC this will make external-dns consistently silently fail to perform any dns operation, can we add a check here if either is required and undefined we return an error ?

ignore, cloudflare.New already performs this check

ideahitme · 2017-06-13T11:27:25Z

provider/cloudflare.go

+	endpoints := []*endpoint.Endpoint{}
+	var record cloudflare.DNSRecord
+	for _, zone := range zones {
+		records, err := p.Client.DNSRecords(zone.ID, record)


nit: p.Client.DNSRecords(zone.ID, cloudflare.DNSRecord{})

ideahitme · 2017-06-13T11:28:07Z

provider/cloudflare.go

+		}
+
+		for _, r := range records {
+			// TODO: limit Types


is this TODO a blocker ?

ideahitme · 2017-06-13T11:32:55Z

provider/cloudflare.go

+
+func (p *cloudFlareProvider) getRecordID(zoneID string, record cloudflare.DNSRecord) (string, error) {
+	records := cloudflare.DNSRecord{}
+	zoneRecords, err := p.Client.DNSRecords(zoneID, records)


same as in DigitalOcean, let's make a single call per zone not to spam CF API

ideahitme · 2017-06-13T11:33:13Z

provider/cloudflare.go

+			return zoneRecord.ID, nil
+		}
+	}
+	return "", fmt.Errorf("No record id found")


nit: error message should be lower-cased

ideahitme · 2017-06-13T12:02:57Z

One more important thing which I forgot to include into PR review, is that ListZones call does not retrieve all zones, there is a pagination in place but it is not implemented in the sdk. It is however implemented for DNSRecords API. I am not entirely sure how easy it would be to have it fixed (it doesn't seem simple IMO).

I am personally fine with having this merged without zones pagination because pagination on Zones does not seem to be critical for External DNS implementation, though of course should be implemented when possible. We can as well block this PR until we have this feature in place. Thoughts ?

@linki mentioned that this might potentially be problematic if response on the first page is not consistent on different API calls

I created an upstream issue with more details if you are interested:

cloudflare/cloudflare-go#135

/cc @linki @mikkeloscar @njuettner

k8s-ci-robot · 2017-06-13T12:02:59Z

@ideahitme: GitHub didn't allow me to request PR reviews from the following users: mikkeloscar, njuettner.

Note that only kubernetes-incubator members can review this PR, and authors cannot review their own PRs.

In response to this:

One more important thing which I forgot to include into PR review, is that ListZones call does not retrieve all zones, there is a pagination in place but it is not implemented in the sdk. It is however implemented for DNSRecords API. I am not entirely sure how easy it would be to have it fixed (it doesn't seem simple IMO).

I am personally fine with having this merged without zones pagination because pagination on Zones does not seem to be critical for External DNS implementation, though of course should be implemented once necessary. We can as well block this PR until we have this feature in place. Thoughts ?

I created an upstream issue with more details if you are interested:

cloudflare/cloudflare-go#135

/cc @linki @mikkeloscar @njuettner

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.

* reduce the number of API calls * match by type and name for record id

ideahitme · 2017-06-14T14:49:35Z

@linki can u please take another look, we fixed some bugs and improve test coverage

ideahitme · 2017-06-15T13:51:35Z

the bug with finding suitable zone should be fixed, please give it another try
@mikkeloscar thanks for trying out and finding the bug ;)

mikkeloscar · 2017-06-15T13:54:23Z

@ideahitme works 👍

* CloudFlare Provider * updating glide * gofmt cloudflare_test.go * Unset envs to test NewCloudFlareProvider * More tests * fix(cloudflare): fix compiler errors resulting from merge * Typo * Undo vendor changes * decrease api calls, fix some nits * Cloudflare iteration (#2) * reduce the number of API calls * match by type and name for record id * improve coverage and fix the bug with suitable zone * tests failed due to wrong formatting * add cloudflare integration to the main * vendor cloudflare deps * fix cloudflare zone detection + tests * fix conflicting test function names

…#140) Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>

CloudFlare Provider

419e1e3

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 10, 2017

njuettner added 2 commits April 10, 2017 10:18

updating glide

0009297

gofmt cloudflare_test.go

9413596

njuettner added 2 commits April 11, 2017 09:54

Unset envs to test NewCloudFlareProvider

2bf7d0e

More tests

23816d7

linki added this to the v0.5 milestone Apr 11, 2017

linki added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 12, 2017

Martin Linkhorst added 2 commits April 12, 2017 14:19

Merge remote-tracking branch 'upstream/master' into feature/cloudflar…

e7ad391

…e-provider

fix(cloudflare): fix compiler errors resulting from merge

487b433

Typo

4673358

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 Apr 12, 2017

ideahitme added the awaiting review label Apr 13, 2017

Undo vendor changes

4b8ae04

linki removed the awaiting review label Apr 20, 2017

njuettner mentioned this pull request Apr 24, 2017

Travis failed due to missing package dependencies #172

Closed

linki previously requested changes Jun 12, 2017

View reviewed changes

linki added size/medium and removed size/small labels Jun 12, 2017

linki self-requested a review June 12, 2017 14:33

ideahitme suggested changes Jun 13, 2017

View reviewed changes

njuettner and others added 4 commits June 14, 2017 10:17

decrease api calls, fix some nits

e15e815

Cloudflare iteration (#2)

5085d21

* reduce the number of API calls * match by type and name for record id

Merge branch 'master' into feature/cloudflare-provider

4cf8548

improve coverage and fix the bug with suitable zone

2f6afa3

ideahitme approved these changes Jun 14, 2017

View reviewed changes

njuettner and others added 4 commits June 15, 2017 10:26

tests failed due to wrong formatting

88a5231

add cloudflare integration to the main

b00089f

vendor cloudflare deps

3b86fc1

fix cloudflare zone detection + tests

b9250ee

ideahitme added 2 commits June 16, 2017 10:50

merge with master

4114d58

fix conflicting test function names

af737de

linki added lgtm "Looks good to me", indicates that a PR is ready to be merged. provider labels Jun 16, 2017

ideahitme merged commit cb58633 into kubernetes-sigs:master Jun 16, 2017

linki added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/medium labels Jan 2, 2018

lou-lan pushed a commit to lou-lan/external-dns that referenced this pull request May 11, 2022

docs: migrate naming guide to 1.12 plugin call style (kubernetes-sigs…

e4cf7f5

…#140) Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CloudFlare as a new provider #140

CloudFlare as a new provider #140

njuettner commented Apr 10, 2017

k8s-ci-robot commented Apr 10, 2017

ideahitme commented Apr 10, 2017

njuettner commented Apr 11, 2017

linki commented Apr 12, 2017 •

edited

linki commented Apr 12, 2017

njuettner commented Apr 12, 2017 •

edited

thelinuxfoundation commented Apr 12, 2017

ideahitme commented Apr 13, 2017

njuettner commented Apr 14, 2017 •

edited

njuettner commented Apr 14, 2017 •

edited

linki commented Apr 24, 2017 •

edited

linki commented Apr 24, 2017

njuettner commented Apr 24, 2017

linki left a comment •

edited

linki commented Jun 12, 2017

ideahitme left a comment •

edited

ideahitme Jun 13, 2017

ideahitme Jun 13, 2017

ideahitme Jun 13, 2017

ideahitme Jun 13, 2017

ideahitme Jun 13, 2017

ideahitme Jun 13, 2017

ideahitme Jun 13, 2017

ideahitme commented Jun 13, 2017 •

edited

k8s-ci-robot commented Jun 13, 2017

ideahitme commented Jun 14, 2017

ideahitme commented Jun 15, 2017 •

edited

mikkeloscar commented Jun 15, 2017

CloudFlare as a new provider #140

CloudFlare as a new provider #140

Conversation

njuettner commented Apr 10, 2017

k8s-ci-robot commented Apr 10, 2017

ideahitme commented Apr 10, 2017

njuettner commented Apr 11, 2017

linki commented Apr 12, 2017 • edited

linki commented Apr 12, 2017

njuettner commented Apr 12, 2017 • edited

thelinuxfoundation commented Apr 12, 2017

ideahitme commented Apr 13, 2017

njuettner commented Apr 14, 2017 • edited

njuettner commented Apr 14, 2017 • edited

linki commented Apr 24, 2017 • edited

linki commented Apr 24, 2017

njuettner commented Apr 24, 2017

linki left a comment • edited

Choose a reason for hiding this comment

linki commented Jun 12, 2017

ideahitme left a comment • edited

Choose a reason for hiding this comment

ideahitme Jun 13, 2017

Choose a reason for hiding this comment

ideahitme Jun 13, 2017

Choose a reason for hiding this comment

ideahitme Jun 13, 2017

Choose a reason for hiding this comment

ideahitme Jun 13, 2017

Choose a reason for hiding this comment

ideahitme Jun 13, 2017

Choose a reason for hiding this comment

ideahitme Jun 13, 2017

Choose a reason for hiding this comment

ideahitme Jun 13, 2017

Choose a reason for hiding this comment

ideahitme commented Jun 13, 2017 • edited

k8s-ci-robot commented Jun 13, 2017

ideahitme commented Jun 14, 2017

ideahitme commented Jun 15, 2017 • edited

mikkeloscar commented Jun 15, 2017

linki commented Apr 12, 2017 •

edited

njuettner commented Apr 12, 2017 •

edited

njuettner commented Apr 14, 2017 •

edited

njuettner commented Apr 14, 2017 •

edited

linki commented Apr 24, 2017 •

edited

linki left a comment •

edited

ideahitme left a comment •

edited

ideahitme commented Jun 13, 2017 •

edited

ideahitme commented Jun 15, 2017 •

edited