AWS Route53 dnsprovider #26049

ghost · 2016-05-23T04:51:19Z

Still needs unit tests, and some other cleanup. Review not urgent, but feel free to make a first pass.
Only need to look at the last two commits. The prior commits will go in as #26020. This will need to be rebased against #26020 once that merges.

It's a bare minimum implementation, only what's required for Ubernetes Federated Services (managing basic A and CNAME records). More functionality (health checks, geolocation etc) can be fairly easily added as required.

It also requires github.com/aws/aws-sdk-go/service/route53 to be vendored into godeps, which I haven't managed to do successfully yet (Oh Godep!)

cc: @justinsb FYI

ghost · 2016-05-23T05:10:18Z

@nikhiljindal FYI, as you're doing such a marvellous job of shepherding the cluster federation PR's through the pipeline.

ghost · 2016-05-23T05:15:09Z

@mfanjie FYI

ghost · 2016-05-24T16:24:55Z

!!! 'gofmt -s' needs to be run on the following files: 
./federation/pkg/dnsprovider/providers/aws/route53/route53_test.go
./federation/pkg/dnsprovider/providers/aws/route53/zone.go
./federation/pkg/dnsprovider/providers/aws/route53/interface.go
./federation/pkg/dnsprovider/providers/aws/route53/rrset.go
./federation/pkg/dnsprovider/providers/aws/route53/internal/stubs/doc.go
./federation/pkg/dnsprovider/providers/aws/route53/rrsets.go
./federation/pkg/dnsprovider/providers/aws/route53/route53.go
FAILED   ./hack/../hack/verify-gofmt.sh 8s

ghost · 2016-06-01T05:46:43Z

@madhusudancs I added unit tests, which all pass. Ready for review. I'll clean up the last few todo's tomorrow, and check hack/verify-all.sh, but please go ahead and do a first pass before that if you can.

ghost · 2016-06-01T05:47:13Z

cc: @mfanjie @nikhiljindal FYI.

ghost · 2016-06-01T17:44:11Z

Similar gofmt error to one I saw in one of my other PR's #25991. I must have an incompatible version of gfmt on my local machine or something. Pity that gofmt refuses to tell us what version it is :-)

./federation/pkg/dnsprovider/providers/aws/route53/rrsets.go
FAILED   ./hack/../hack/verify-gofmt.sh 17s

 which gofmt
/usr/local/go/bin/gofmt
$ which go
/usr/local/go/bin/go
$ go version
go version go1.6 darwin/amd64
$ gofmt --version
flag provided but not defined: -version
usage: gofmt [flags] [path ...]
  -cpuprofile string
        write cpu profile to this file
  -d    display diffs instead of rewriting files
  -e    report all errors (not just the first 10 on different lines)
  -l    list files whose formatting differs from gofmt's
  -r string
        rewrite rule (e.g., 'a[b:len(a)] -> a[b:]')
  -s    simplify code
  -w    write result to (source) file instead of stdout

ghost · 2016-06-01T18:04:56Z

Reassigning to @nikhiljindal for review, as discussed with @madhusudancs

nikhiljindal · 2016-06-01T20:42:07Z

federation/pkg/dnsprovider/providers/aws/route53/interface.go

+)
+
+// Compile time check for interface adeherence
+var _ Interface = Interface{}


Sorry I seem to be missing something here. We are checking that it implements itself?

Good catch - the left hand one is supposed to be dnsprovider.Interface. Will fix.

nikhiljindal · 2016-06-01T20:43:46Z

federation/pkg/dnsprovider/providers/aws/route53/rrsets.go

+		},
+		HostedZoneId: aws.String(*rrset.(ResourceRecordSet).rrsets.zone.impl.Id), // Required
+	}
+	_, err := rrsets.zone.zones.interface_.service.ChangeResourceRecordSets(input)


Its surprising that we have to construct the whole set to remove as well.
Suprised that there is no Add and Remove methods, only Change.

Yeah, that's how most of the DNS interfaces seem to work. To delete, you have to pass a fully matching record set, not just an ID. Otherwise it fails, by design. The method is called Change...(), but Action:"DELETE" makes it equivalent to a Delete() method.

ghost · 2016-06-01T23:07:40Z

@nikhiljindal All review comments addressed. PTAL.

ghost · 2016-06-01T23:57:33Z

Removed unused stubs package, which should resolve the gofmt verification error.

nikhiljindal · 2016-06-02T00:11:08Z

federation/pkg/dnsprovider/providers/aws/route53/interface.go

+// Compile time check for interface adeherence
+var _ dnsprovider.Interface = Interface{}
+
+type Interface struct {


Is this a test only interface? Move it to testing package?

No, it's the interface to Route53. I think we should leave it where it is.

nikhiljindal · 2016-06-02T00:12:38Z

Thanks for the fixes @quinton-hoole
Mostly looks good, added a few comments.

Feel free to self apply the label after fixing.

ghost · 2016-06-02T04:55:46Z

Oh dear. Lingering godep issues. I'll sort this out in the morning.

federation/pkg/dnsprovider/providers/aws/route53/route53.go:26:2: cannot find package "github.com/aws/aws-sdk-go/service/route53" in any of:
    /go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/github.com/aws/aws-sdk-go/service/route53 (vendor tree)
    /usr/local/go/src/github.com/aws/aws-sdk-go/service/route53 (from $GOROOT)
    /go/src/k8s.io/kubernetes/_output/local/go/src/github.com/aws/aws-sdk-go/service/route53 (from $GOPATH)

ghost · 2016-06-02T18:10:55Z

OK, addressed final review comments, and added missing godeps. Applying LGTM.

ghost · 2016-06-03T23:07:18Z

Fixed minor typo that snuck in somehow and broke compilation.

k8s-bot · 2016-06-03T23:49:46Z

GCE e2e build/test passed for commit 5b42184.

k8s-github-robot · 2016-06-04T00:05:25Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-06-04T00:58:50Z

GCE e2e build/test passed for commit 5b42184.

k8s-github-robot · 2016-06-04T01:00:09Z

Automatic merge from submit-queue

ghost added team/control-plane labels May 23, 2016

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

ghost assigned madhusudancs May 23, 2016

googlebot added the cla: yes label May 23, 2016

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

justinsb added the area/platform/aws label May 27, 2016

ghost changed the title ~~WIP: AWS Route53 dnsprovider~~ AWS Route53 dnsprovider May 29, 2016

ghost added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 29, 2016

ghost added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jun 1, 2016

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 1, 2016

ghost assigned nikhiljindal and unassigned madhusudancs Jun 1, 2016

nikhiljindal reviewed Jun 1, 2016
View reviewed changes

ghost mentioned this pull request Jun 1, 2016

AWS Route53 dnsprovider: address outstanding review comments #26668

Closed

nikhiljindal reviewed Jun 2, 2016
View reviewed changes

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 2, 2016

ghost added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2016

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 2, 2016

ghost added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 2, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 3, 2016

Quinton Hoole added 2 commits June 3, 2016 15:51

AWS Route53 dnsprovider

7c14d76

Added AWS Route53 libraries to godeps.

5b42184

ghost added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 3, 2016

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 3, 2016

k8s-github-robot merged commit 6420496 into kubernetes:master Jun 4, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AWS Route53 dnsprovider #26049

AWS Route53 dnsprovider #26049

ghost commented May 23, 2016 •

edited by ghost

Loading

ghost commented May 23, 2016

ghost commented May 23, 2016

ghost commented May 24, 2016

ghost commented Jun 1, 2016

ghost commented Jun 1, 2016

ghost commented Jun 1, 2016

ghost commented Jun 1, 2016

nikhiljindal Jun 1, 2016

ghost Jun 1, 2016

ghost Jun 1, 2016

nikhiljindal Jun 1, 2016

ghost Jun 1, 2016

ghost commented Jun 1, 2016

ghost commented Jun 1, 2016

nikhiljindal Jun 2, 2016

ghost Jun 2, 2016

nikhiljindal commented Jun 2, 2016

ghost commented Jun 2, 2016

ghost commented Jun 2, 2016

ghost commented Jun 3, 2016 •

edited by ghost

Loading

k8s-bot commented Jun 3, 2016

k8s-github-robot commented Jun 4, 2016

k8s-bot commented Jun 4, 2016

k8s-github-robot commented Jun 4, 2016

AWS Route53 dnsprovider #26049

AWS Route53 dnsprovider #26049

Conversation

ghost commented May 23, 2016 • edited by ghost Loading

ghost commented May 23, 2016

ghost commented May 23, 2016

ghost commented May 24, 2016

ghost commented Jun 1, 2016

ghost commented Jun 1, 2016

ghost commented Jun 1, 2016

ghost commented Jun 1, 2016

nikhiljindal Jun 1, 2016

Choose a reason for hiding this comment

ghost Jun 1, 2016

Choose a reason for hiding this comment

ghost Jun 1, 2016

Choose a reason for hiding this comment

nikhiljindal Jun 1, 2016

Choose a reason for hiding this comment

ghost Jun 1, 2016

Choose a reason for hiding this comment

ghost commented Jun 1, 2016

ghost commented Jun 1, 2016

nikhiljindal Jun 2, 2016

Choose a reason for hiding this comment

ghost Jun 2, 2016

Choose a reason for hiding this comment

nikhiljindal commented Jun 2, 2016

ghost commented Jun 2, 2016

ghost commented Jun 2, 2016

ghost commented Jun 3, 2016 • edited by ghost Loading

k8s-bot commented Jun 3, 2016

k8s-github-robot commented Jun 4, 2016

k8s-bot commented Jun 4, 2016

k8s-github-robot commented Jun 4, 2016

ghost commented May 23, 2016 •

edited by ghost

Loading

ghost commented Jun 3, 2016 •

edited by ghost

Loading