Google Cloud DNS dnsprovider #25389

ghost · 2016-05-10T01:25:53Z

Add Google Cloud DNS dnsprovider.

ghost · 2016-05-10T01:27:13Z

Successfully tested against both real and stubbed cloud backends.
Separate PR for Amazon Route53 coming shortly.

@mfanjie @nikhiljindal FYI

ghost · 2016-05-10T15:59:25Z

cc: @justinsb

ghost · 2016-05-10T17:06:31Z

federation/pkg/dnsprovider/plugins.go

+	"github.com/golang/glog"
+)
+
+// Factory is a function that returns a cloudprovider.Interface.


My apologies - this file needs updating, and has not been tested yet. I'll fix it now. But you can review the rest of the PR in the mean time,

ghost · 2016-05-11T22:20:53Z

Looks like a vendor-related problem. I'm looking...

pkg/cloudprovider/providers/gce/gce.go:31:2: cannot find package "code.google.com/p/gcfg" in any of:
    /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/vendor/code.google.com/p/gcfg (vendor tree)

ghost · 2016-05-12T17:20:57Z

@k8s-bot test this issue #25516

ghost · 2016-05-12T17:45:30Z

Apparently #25516 did not fix this:

pkg/cloudprovider/providers/gce/gce.go:31:2: cannot find package "code.google.com/p/gcfg" in any of:
    /go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/code.google.com/p/gcfg (vendor tree)
    /usr/local/go/src/code.google.com/p/gcfg (from $GOROOT)
    /go/src/k8s.io/kubernetes/_output/local/go/src/code.google.com/p/gcfg (from $GOPATH)

…fully Tested against both real backend and stubbed backend.

ghost · 2016-05-12T19:16:29Z

@k8s-bot Test this please

ghost · 2016-05-12T22:29:52Z

Looks like I need a better header comment. Fixing...

Verifying ./hack/../hack/verify-boilerplate.sh
Boilerplate header is wrong for: /go/src/k8s.io/kubernetes/federation/pkg/dnsprovider/dns.go
...
FAILED   ./hack/../hack/verify-boilerplate.sh   0s

madhusudancs · 2016-05-13T03:44:37Z

federation/pkg/dnsprovider/plugins.go

+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/


Same comment as previous files.

Agreed, done.

ghost · 2016-05-13T05:19:50Z

@k8s-bot Test this please.

madhusudancs · 2016-05-13T05:32:11Z

federation/pkg/dnsprovider/plugins.go

+
+// All registered dns providers.
+var providersMutex sync.Mutex
+var providers = make(map[string]Factory)


These two are always used together and should be structurally reflected that way. Something like:

type providersMap struct { providers map[string]Factory mutex sync.Mutex }

And initialize the struct in InitDnsProviders() below.

Yeah, that makes sense.

InitDnsProvider() seems to be not the right place. It seems to initialize a single provider. providersMap can probably be globally initialized. Or could be done in a package level init() function.

This code is actually a direct mirror of what's in the cloudprovider package. I'd suggest that we fix both of them, as you suggest, but in a separate cleanup PR. At least the two (dnsprovider and cloudprovider) are consistent now :-)

madhusudancs · 2016-05-13T07:13:02Z

federation/pkg/dnsprovider/providers/google/clouddns/internal/changes_create_call.go

+package internal
+
+import (
+	"google.golang.org/api/dns/v1"


Same comment as before.

madhusudancs · 2016-05-13T07:54:59Z

@quinton-hoole I did one full pass. Have left some comments.

In terms of what it accomplishes, it looks good. But on the other hand, there are too many levels in the hierarchy. I guess you are doing all that to provide fakes for testing. But that makes it hard to form a very good mental model in the first pass.

madhusudancs · 2016-05-13T07:57:35Z

Also, why is all this in federation/pkg/dnsprovider instead of /pkg/dnsprovider? It seems useful for non-federation stuff too. People have asked for the ability to program external DNS in the context of PetSets and other places. And this seems to be generic enough to be useful elsewhere.

ghost · 2016-05-13T20:56:58Z

@madhusudancs Agreed it's a lot of code, but there is no other/better way to do unit testing. At least not that I could think of. I don't like it either, but I couldn't think of a better/more lucid way.

Also, agreed, this could/should be moved to pkg/dnsprovider. Will do.

ghost · 2016-05-13T21:00:00Z

@madhusudancs One more thing about the unit testing. The root of the complication is that Google Cloud DNS golang library does not include any golang interfaces. It implements structs directly, with read/write fields. This is what makes it fundamentally unstubbable without a separate interface level, and another level of implementations of that interface (as well as the stub implementation) - hence three levels.

AWS Route 53 on the other hand, provides the interfaces and implementations thereof, hence requiring only a single layer of stub implementations. Make sense?

k8s-bot · 2016-05-16T18:11:46Z

GCE e2e build/test passed for commit 4988c43.

ghost · 2016-05-16T18:16:13Z

Should compile correctly after #25679 merges. Needs updated googleapi package.

madhusudancs · 2016-05-19T04:11:47Z

@quinton-hoole Is this ready for next review pass?

ghost · 2016-05-19T14:04:15Z

All that remains is to move it out of the federation package tree, I think.
On May 18, 2016 23:12, "Madhusudan.C.S" notifications@github.com wrote:

@quinton-hoole https://github.com/quinton-hoole Is this ready for next
review pass?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#25389 (comment)

madhusudancs · 2016-05-19T21:08:22Z

@quinton-hoole Except for a few very minor nits, this code looks good to me for the first cut. I definitely don't want to hold this PR for the dnsprovider package to be moved to pkg/. Please do consider doing that in a follow up PR and also address the tiny nits.

I am applying the LGTM label. If you need to rebase or something and the merge bot removes the label, please feel free to reapply.

madhusudancs · 2016-05-20T07:13:11Z

@madhusudancs One more thing about the unit testing. The root of the complication is that Google Cloud DNS golang library does not include any golang interfaces. It implements structs directly, with read/write fields. This is what makes it fundamentally unstubbable without a separate interface level, and another level of implementations of that interface (as well as the stub implementation) - hence three levels.

AWS Route 53 on the other hand, provides the interfaces and implementations thereof, hence requiring only a single layer of stub implementations. Make sense?

Yeah, makes sense.

ghost · 2016-05-21T18:35:17Z

Replaced by identical #26020

ghost assigned madhusudancs May 10, 2016

ghost added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. team/control-plane labels May 10, 2016

ghost added this to the v1.3 milestone May 10, 2016

googlebot added the cla: yes label May 10, 2016

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels May 10, 2016

ghost reviewed May 10, 2016
View reviewed changes

Quinton Hoole added 4 commits May 12, 2016 12:06

Google Cloud DNS dnsprovider rewrite/refactor for unit tests. Success…

3093d37

…fully Tested against both real backend and stubbed backend.

WIP - rebase

83ab076

Add config for Google Cloud DNS dnsprovider

0d6b7c0

Fix import paths screwed up by emaacs auto-import :-(

bd4cfb0

ghost mentioned this pull request May 12, 2016

[WIP]Fed service controller #25517

Closed

madhusudancs reviewed May 13, 2016
View reviewed changes

Fixed license text in dnsprovider package.

cc571c6

ghost closed this May 13, 2016

ghost reopened this May 13, 2016

madhusudancs reviewed May 13, 2016
View reviewed changes

Quinton Hoole added 2 commits May 13, 2016 15:07

Address review comments - still need to move to pkg/dnsprovider.

67b3c5b

Fixed govet issues.

4988c43

madhusudancs added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2016

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2016

ghost added release-note Denotes a PR that will be considered when it comes time to generate release notes. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 21, 2016

k8s-github-robot removed the release-note-label-needed label May 21, 2016

ghost mentioned this pull request May 21, 2016

Google Cloud DNS dnsprovider - replacement for #25389 #26020

Merged

ghost closed this May 21, 2016

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Google Cloud DNS dnsprovider #25389

Google Cloud DNS dnsprovider #25389

ghost commented May 10, 2016

ghost commented May 10, 2016

ghost commented May 10, 2016

ghost May 10, 2016

ghost commented May 11, 2016

ghost commented May 12, 2016

ghost commented May 12, 2016

ghost commented May 12, 2016

ghost commented May 12, 2016

madhusudancs May 13, 2016

ghost May 13, 2016

ghost commented May 13, 2016

madhusudancs May 13, 2016 •

edited

Loading

ghost May 13, 2016

madhusudancs May 13, 2016

ghost May 13, 2016

madhusudancs May 13, 2016

madhusudancs May 13, 2016

ghost May 13, 2016

ghost May 13, 2016

madhusudancs commented May 13, 2016

madhusudancs commented May 13, 2016 •

edited

Loading

ghost commented May 13, 2016

ghost commented May 13, 2016

k8s-bot commented May 16, 2016

ghost commented May 16, 2016

madhusudancs commented May 19, 2016

ghost commented May 19, 2016

madhusudancs commented May 19, 2016

madhusudancs commented May 20, 2016

ghost commented May 21, 2016

Google Cloud DNS dnsprovider #25389

Google Cloud DNS dnsprovider #25389

Conversation

ghost commented May 10, 2016

ghost commented May 10, 2016

ghost commented May 10, 2016

Choose a reason for hiding this comment

ghost commented May 11, 2016

ghost commented May 12, 2016

ghost commented May 12, 2016

ghost commented May 12, 2016

ghost commented May 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented May 13, 2016

madhusudancs May 13, 2016 • edited Loading

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

madhusudancs commented May 13, 2016

madhusudancs commented May 13, 2016 • edited Loading

ghost commented May 13, 2016

ghost commented May 13, 2016

k8s-bot commented May 16, 2016

ghost commented May 16, 2016

madhusudancs commented May 19, 2016

ghost commented May 19, 2016

madhusudancs commented May 19, 2016

madhusudancs commented May 20, 2016

ghost commented May 21, 2016

madhusudancs May 13, 2016 •

edited

Loading

madhusudancs commented May 13, 2016 •

edited

Loading