Skip to content
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

Add Google Cloud DNS plugin #3011

Merged
merged 1 commit into from Aug 17, 2019

Conversation

@palash25
Copy link
Contributor

commented Jul 16, 2019

Closes: #2822

1. Why is this pull request needed and what does it do?

2. Which issues (if any) are related?

3. Which documentation changes (if any) need to be made?

4. Does this introduce a backward incompatible change or deprecation?

@palash25 palash25 force-pushed the palash25:gcp-dns branch from c74616b to a39597b Jul 22, 2019

plugin/clouddns/clouddns.go Show resolved Hide resolved
plugin/clouddns/clouddns.go Outdated Show resolved Hide resolved
plugin/clouddns/clouddns.go Outdated Show resolved Hide resolved
@miekg miekg referenced this pull request Jul 23, 2019

@palash25 palash25 force-pushed the palash25:gcp-dns branch 3 times, most recently from f8b1c99 to e85e15e Jul 25, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Jul 25, 2019

Codecov Report

Merging #3011 into master will increase coverage by 0.27%.
The diff coverage is 71.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3011      +/-   ##
==========================================
+ Coverage    55.5%   55.78%   +0.27%     
==========================================
  Files         207      210       +3     
  Lines       10337    10517     +180     
==========================================
+ Hits         5738     5867     +129     
- Misses       4184     4225      +41     
- Partials      415      425      +10
Impacted Files Coverage Δ
plugin/clouddns/gcp.go 0% <0%> (ø)
plugin/clouddns/setup.go 58.33% <58.33%> (ø)
plugin/clouddns/clouddns.go 85.45% <85.45%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a6e9a9...0826d86. Read the comment docs.

@palash25 palash25 force-pushed the palash25:gcp-dns branch 10 times, most recently from 25d3a50 to f68e4e3 Jul 27, 2019

@palash25 palash25 changed the title [WIP] Add Google Cloud DNS plugin Add Google Cloud DNS plugin Aug 1, 2019

@corbot corbot bot requested a review from miekg Aug 1, 2019

@corbot

This comment has been minimized.

Copy link

commented Aug 1, 2019

Thank you for your contribution. I've just checked the OWNERS files to find a suitable reviewer. This search was successful and I've asked miekg (via /OWNERS) for a review.

If you have questions or suggestions for this bot, please file an issue against the miekg/dreck repository.

The bot understands the commands that are listed here.

@palash25 palash25 force-pushed the palash25:gcp-dns branch from f68e4e3 to dde5a3f Aug 1, 2019

plugin/clouddns/README.md Outdated Show resolved Hide resolved

@palash25 palash25 force-pushed the palash25:gcp-dns branch from dde5a3f to 2a34edf Aug 3, 2019

@palash25

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

@yongtang can you please review?

@dilyevsky
Copy link
Collaborator

left a comment

Took initial pass.

plugin/clouddns/clouddns.go Show resolved Hide resolved
"sync"
"time"

"github.com/coredns/coredns/plugin"

This comment has been minimized.

Copy link
@dilyevsky

dilyevsky Aug 4, 2019

Collaborator

This block and block below should be reversed - from less specific to more.

This comment has been minimized.

Copy link
@palash25

palash25 Aug 4, 2019

Author Contributor

I am sorry but I am not really sure what you mean. The imports are already in the order of less specific to more (plugin -> plugin/file -> plugin/file/pkg/fall and so on) so why should they be reversed?

plugin/clouddns/clouddns.go Outdated Show resolved Hide resolved
}
}

ctx := context.Background()

This comment has been minimized.

Copy link
@dilyevsky

dilyevsky Aug 4, 2019

Collaborator

Probably better to obtain context from *caddy.Controller.

This comment has been minimized.

Copy link
@palash25

palash25 Aug 5, 2019

Author Contributor

I was following the same pattern as the route53 plugin as it also uses context.Background(), even the current Azure plugin PR also uses the same, so I used context.Background() for uniformity across the plugins.

plugin/clouddns/clouddns_test.go Outdated Show resolved Hide resolved
plugin/clouddns/clouddns.go Outdated Show resolved Hide resolved
plugin/clouddns/clouddns.go Outdated Show resolved Hide resolved
plugin/clouddns/clouddns.go Outdated Show resolved Hide resolved
plugin/clouddns/clouddns.go Outdated Show resolved Hide resolved
plugin/clouddns/clouddns.go Outdated Show resolved Hide resolved

@palash25 palash25 force-pushed the palash25:gcp-dns branch 2 times, most recently from 861a81a to c3cdfc2 Aug 4, 2019

plugin/clouddns/README.md Outdated Show resolved Hide resolved
plugin/clouddns/README.md Outdated Show resolved Hide resolved
plugin/clouddns/README.md Outdated Show resolved Hide resolved
plugin/clouddns/README.md Outdated Show resolved Hide resolved

@palash25 palash25 force-pushed the palash25:gcp-dns branch 3 times, most recently from 9d9c334 to dd03777 Aug 6, 2019

@palash25

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

@yongtang I have updated the PR, please take another look 😄

@yongtang

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

@palash25 First pass looks ok. I will take another look during the weekend. Also, can you rebase the PR?

@palash25 palash25 force-pushed the palash25:gcp-dns branch from dd03777 to 3f849f4 Aug 9, 2019

Add Google Cloud DNS plugin
Signed-off-by: Palash Nigam <npalash25@gmail.com>

Closes: #2822

@palash25 palash25 force-pushed the palash25:gcp-dns branch from 3f849f4 to 0826d86 Aug 9, 2019

@palash25

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

done 👍

@yongtang

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

I played with the plugin over the weekend, and look good to me.

One interesting find while I am testing on Google Cloud Platform, is that GCP actually allows you to create a zone without attaching to a VPC. Able to dig the DNS records while seeing This zone isn't visible to any networks. Add networks to make this zone active. on the GCP console.

This could be very useful as essentially you could use Google Cloud DNS as a database or datastore for your onprem usage. Essentially you could create a zone on Google Cloud DNS without attaching to a VPC, then you can fully access the record on your local cluster.

It does comes with a cost, though you could combine with cache and forward plugin to cache the result. The advantage is that there is no need to managed a local database or file.

Note on AWS, Route53 does require you to specify the VPC ID when create a Hosted Zone. So it is not applicable on AWS.

Overall the plugin is a useful addition.

@yongtang yongtang merged commit 194b0f9 into coredns:master Aug 17, 2019

4 checks passed

ci/circleci: kubernetes-tests Your tests passed on CircleCI!
Details
codecov/project 55.78% (target 50%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
stickler-ci No lint errors found.

@palash25 palash25 deleted the palash25:gcp-dns branch Aug 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.