digitalocean support -- dns provider #2864
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
/retest |
andrewsykim
force-pushed
the
digitalocean
branch
from
8df48fa to
3b0bc00
Compare
andrewsykim
force-pushed
the
digitalocean
branch
4 times, most recently
from
df40822 to
e760014
Compare
andrewsykim
changed the title
|
/assign @chrislovecnm @justinsb |
|
@joonas any idea who can do a PR review of the new code that we are bringing in for DO support with kops? |
There was a problem hiding this comment.
Exciting stuff btw. Lots of questions - mostly around how you are handling the HTTP API calls.
General questions. Should we put this behind a feature flag? I know this is just the start of support, but should you just include a small document of what has been done, and update that doc with each PR?
pkg/resources/digitalocean/cloud.go
Outdated
| tags map[string]string | ||
| } | ||
|
|
||
| func NewCloud() (*Cloud, error) { |
There was a problem hiding this comment.
Code level comments, please.
|
|
||
| // List returns a list of all dns zones | ||
| func (z *zones) List() ([]dnsprovider.Zone, error) { | ||
| domains, err := listDomains(z.client) |
There was a problem hiding this comment.
We filter calls like these with AWS. Are we able to filter with DO?
There was a problem hiding this comment.
For certain resources you can, for example you can get droplets by tag, but for domains I don't think so.
There was a problem hiding this comment.
In this case, you don't want to filter List() anyways since zones.List() is actually supposed to return all the zones registered under the cloud provider.
There was a problem hiding this comment.
zone represents just 1 of the zones (aka domains, this interface is really confusing IMO)
| } | ||
|
|
||
| oauthClient := oauth2.NewClient(oauth2.NoContext, tokenSource) | ||
| client := godo.NewClient(oauthClient) |
There was a problem hiding this comment.
Can we check that the client is valid? I am surprised that this does not return error handling.
There was a problem hiding this comment.
The godo client is pretty straight forward since it only needs to initialize a http client so there's really no errors to return. See implementation here: https://github.com/digitalocean/godo/blob/master/godo.go#L150-L179
|
|
||
| // List returns a list of dnsprovider.ResourceRecordSet | ||
| func (r *resourceRecordSets) List() ([]dnsprovider.ResourceRecordSet, error) { | ||
| records, err := getRecords(r.client, r.zone.Name()) |
There was a problem hiding this comment.
Same question about filtering. This may return ALOT of records on big accounts. Can we paginate or filter?
There was a problem hiding this comment.
There's no way to filter records iirc, but for the case where there's a lot of records, you can paginate. This is definitely something I'd want to address before DO support goes beta, for now I think this will be okay. (I'll leave a note though)
| } | ||
|
|
||
| // New returns an implementation of dnsprovider.ResourceRecordSet | ||
| func (r *resourceRecordSets) New(name string, rrdatas []string, ttl int64, rrstype rrstype.RrsType) dnsprovider.ResourceRecordSet { |
There was a problem hiding this comment.
You are not doing much error handling with values that are not defined. Which values are mandatory?
There was a problem hiding this comment.
I believe all the values are required, I would return an err in this function, but I'm using the dnsprovider interface https://github.com/kubernetes/kubernetes/blob/master/federation/pkg/dnsprovider/dns.go#L63
| // TODO (andrewsykim): pagination in ListOptions | ||
| domains, resp, err := c.Domains.List(context.TODO(), &godo.ListOptions{}) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
fmt.Errorf on this please, with resp code and error.
| return nil, err | ||
| } | ||
|
|
||
| if resp.StatusCode != 200 { |
There was a problem hiding this comment.
I am wondering if this is unreachable. Will you get an err from the API if the HTTP status is not 200?
There was a problem hiding this comment.
If I'm not mistaken, the err returned from godo will still be nil if the API returns a 4XX or 5XX. The above err may not be nil if there's a connection timeout or some client side validation failed.
| } | ||
|
|
||
| // deleteDomain deletes a domain given its name | ||
| func deleteDomain(c *godo.Client, name string) error { |
There was a problem hiding this comment.
Again same pattern for HTTP / API handling. Looks like some sort of interface would be appropriate.
| return fmt.Errorf("error applying changeset: %v", err) | ||
| } | ||
|
|
||
| if resp.StatusCode != http.StatusOK { |
There was a problem hiding this comment.
need to use the status code everywhere, mostly you have 200.
| @@ -0,0 +1,489 @@ | |||
| /* | |||
|
@chrislovecnm added the feature gate here #2929 re: documentation, I was hoping to use this issue for task tracking/updates but I can open up a doc in another PR if you think that's best :) |
andrewsykim
force-pushed
the
digitalocean
branch
from
a4e5046 to
d94357b
Compare
| for _, record := range r.additions { | ||
| recordCreateRequest := &godo.DomainRecordEditRequest{ | ||
| Name: record.Name(), | ||
| Data: record.Rrdatas()[0], |
There was a problem hiding this comment.
FYI I do think we use multiple RRDatas (e.g. HA) but I guess we'll see :-)
|
LGTM, and all feature-flagged anyway. At some stage we probably want to adopt external-dns, but I really like the idea of putting the dns provider code into our codebase rather than putting it into core. Makes it easier both to develop the dnsprovider, fix any bugs we find, and maybe move to external-dns at some stage in the future. 👍 |
Initial support for DO on kops, this includes init code for DO Cloud and DNS provider.
Related:
#2150
TODO: