GCLOUD: Support "private domains" plus many bugfixes#2482
GCLOUD: Support "private domains" plus many bugfixes#2482tlimoncelli merged 6 commits intoStackExchange:masterfrom asn-iac:gcloud-changes
Conversation
|
Wow! What a generous contribution! I'm on vacation now but will get to this when I return. |
|
@riyadhalnur could you please review? |
riyadhalnur
left a comment
riyadhalnur
left a comment
There was a problem hiding this comment.
Thanks for these changes 😃. I'll test this branch against real data and get back soon
providers/gcloud/gcloudProvider.go
Outdated
| } | ||
|
|
||
| func (g *gcloudProvider) loadZoneInfo() error { | ||
| // TODO: In order to fully support split horizon domains within the same GCP project, |
There was a problem hiding this comment.
Probably a good idea to create an issue for this for future action
providers/gcloud/gcloudProvider.go
Outdated
| var opt option.ClientOption | ||
| if key, ok := cfg["private_key"]; ok { | ||
| cfg["private_key"] = strings.Replace(key, "\\n", "\n", -1) | ||
| printer.Debugf("GCLOUD :private_key configured, attempting key-based authentication\n") |
providers/gcloud/gcloudProvider.go
Outdated
| if err != nil { | ||
| return nil, fmt.Errorf("no creds.json private_key found and ADC failed with:\n%s", err) | ||
| } | ||
| printer.Debugf("GCLOUD :using application default credentials\n") |
providers/gcloud/gcloudProvider.go
Outdated
| printer.Printf("GCLOUD :visibility %s configured\n", g.Visibility) | ||
| } | ||
| for i, v := range g.Networks { | ||
| networkURLCheck := regexp.MustCompile("^" + selfLinkBasePath + "[a-z][-a-z0-9]{4,28}[a-z0-9]/global/networks/[a-z]([-a-z0-9]{0,61}[a-z0-9])?$") |
There was a problem hiding this comment.
Would prefer to compile this once at the top rather than compiling it on every run of the loop
There was a problem hiding this comment.
moved all of the regexp.MustComplie() calls to var block at the top - hope that was what you were looking for. let me know if you had something else in mind.
providers/gcloud/gcloudProvider.go
Outdated
| if networkURLCheck.MatchString(v) { | ||
| // the user specified a fully qualified network url | ||
| } else { | ||
| networkNameCheck := regexp.MustCompile("^[a-z]([-a-z0-9]{0,61}[a-z0-9])?$") |
There was a problem hiding this comment.
Would prefer to compile this once at the top rather than compiling it on every run of the loop
There was a problem hiding this comment.
moved all of the regexp.MustComplie() calls to var block at the top - hope that was what you were looking for. let me know if you had something else in mind.
providers/gcloud/gcloudProvider.go
Outdated
| } | ||
| for i, v := range g.Networks { | ||
| networkURLCheck := regexp.MustCompile("^" + selfLinkBasePath + "[a-z][-a-z0-9]{4,28}[a-z0-9]/global/networks/[a-z]([-a-z0-9]{0,61}[a-z0-9])?$") | ||
| if networkURLCheck.MatchString(v) { |
There was a problem hiding this comment.
Would this not be simpler to just continue if value is a fully qualified URL rather than an if-else statement
if ok := networkURLCheck.MatchString(v); ok {
continue
}
if ok := networkNameCheck.MatchString(v); !ok {
return nil, fmt.Errorf("GCLOUD :networks set but %s does not appear to be a valid network name or url", v)
}
...There was a problem hiding this comment.
updated logic to remove if-else
providers/gcloud/gcloudProvider.go
Outdated
| return nil, err | ||
| } | ||
| if len(g.Visibility) != 0 { | ||
| visibilityCheck := regexp.MustCompile("^(public|private)$") |
There was a problem hiding this comment.
Would prefer to compile this once at the top
There was a problem hiding this comment.
moved all of the regexp.MustComplie() calls to var block at the top - hope that was what you were looking for. let me know if you had something else in mind.
providers/gcloud/gcloudProvider.go
Outdated
| if len(g.Visibility) != 0 { | ||
| visibilityCheck := regexp.MustCompile("^(public|private)$") | ||
| if ok := visibilityCheck.MatchString(g.Visibility); !ok { | ||
| return nil, fmt.Errorf("GCLOUD :visibility set but does not match ^(public|private)$") |
There was a problem hiding this comment.
Maybe better to say GCLOUD :visibility set but not one of public or private"
| Additions, Deletions []string | ||
| } | ||
|
|
||
| type orderedChanges struct { |
There was a problem hiding this comment.
The struct below duplicates this. We can just have one of them
|
Thanks team! I'm a bit tied up today but will review comments early next week |
|
Hi! I'm back from sabbatical/vacation! Code looks great! Thank you so much for this contribution! Merging soon. |
tlimoncelli
changed the title
tlimoncelli
changed the title
5 participants
Hi all,
First I'd like to acknowledge and thank the dnscontrol community for contributing to such a useful project.
We needed to implement some changes with the GCLOUD provider in a private fork of dnscontrol a few years back and finally have had time to submit some of these back to the public project.
This PR addresses a few existing bugs and adds some new features:
dnscontrol/providers/gcloud/gcloudProvider.go
Lines 95 to 96 in 31bf652
Happy to break these up into smaller PRs if needed.
TIA