-
Notifications
You must be signed in to change notification settings - Fork 369
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
GCLOUD: Support "private domains" plus many bugfixes #2482
Conversation
Wow! What a generous contribution! I'm on vacation now but will get to this when I return. |
@riyadhalnur could you please review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these changes 😃. I'll test this branch against real data and get back soon
providers/gcloud/gcloudProvider.go
Outdated
return g, g.loadZoneInfo() | ||
} | ||
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a good idea to create an issue for this for future action
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created #2483
providers/gcloud/gcloudProvider.go
Outdated
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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer to compile this once at the top
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better to say GCLOUD :visibility set but not one of public or private"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated message
Additions, Deletions []string | ||
} | ||
|
||
type orderedChanges struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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