-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
provider/aws: data source for AWS Hosted Zone #9766
provider/aws: data source for AWS Hosted Zone #9766
Conversation
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.
Hi @mathieuherbert! This looks like a great addition! I've left a few comments on the code inline, but there is one missing feature I think we need to add - zone name is not sufficient to narrow down the search to one zone, since you may have the same name in a private and public zone. I think we need to add a private_zone
boolean field, defaulting to false, to control which of these we find in this case. Thanks for the PR! If you are able to make the changes noted we'll get this merged soon, if not one of us can pick it up and take it through to completion!
package aws | ||
|
||
import ( | ||
"fmt" |
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.
This should have goimports
run on it for correct formatting.
package aws | ||
|
||
import ( | ||
"fmt" |
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.
This should have goimports
run on it for correct formatting.
d.Set("name", hostedZone.Name) | ||
d.Set("comment", hostedZone.Config.Comment) | ||
} else { | ||
return fmt.Errorf("name or zone_id have to be setted") |
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.
This should read either name or zone_id must be set
, I think.
resp, err := conn.GetHostedZone(req) | ||
|
||
if err != nil { | ||
return err |
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 should wrap this in errwrap.Wrapf("Error finding Route 53 Hosted Zone: {{err}}", err)
.
d.SetId(id) | ||
d.Set("zone_id", id) | ||
d.Set("name", hostedZone.Name) | ||
d.Set("comment", hostedZone.Config.Comment) |
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.
This will fail - since there is no comment
field defined in the schema. This should be declared as Computed since it is set here.
@mathieuherbert hi there! Thank you for this! This is a highly desirable new data source. I mean to send a Pull Request which adds such data source, but since you did it already, I will focus on helping you make yours amazing! The output we would be aiming at is something along the lines of the following:
Above shows a Hosted Zone which was found based on zone name and VPC ID, as per:
In other words, for this resource to be very useful, it should support:
I will be more than happy to help you to get there. |
Related to #9590. |
…e zone and trailing dot
} | ||
|
||
// used to manage trailing . | ||
func HostedZoneName(name string) string { |
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.
This is a good idea to have a helper, but few things to consider:
Have a look at the https://golang.org/pkg/strings/ package which is part of the Go's standard library, it should have the tools you need to add and/or remove a trailing dot.
Also, if you are not planning on making this method accessible outside of this package, it should by a convention (and also language mechanics) start with a lower-case letter, so that the symbol is not going to be exported. Albeit, the aws
package (namespace) is large at the moment, nevertheless we should follow some common idioms.
|
||
`aws_hosted_zone` provides details about a specific Hosted Zone. | ||
|
||
This resource can be useful when a module accepts a Hosted Zone name as |
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.
Should probably say "data source".
`aws_hosted_zone` provides details about a specific Hosted Zone. | ||
|
||
This resource can be useful when a module accepts a Hosted Zone name as | ||
an input variable and needs to, for example, add a Record Set in Route 53 |
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.
I would mention that it allows to find Hosted Zone ID, which is often needed, given a Hosted Zone name and certain search criteria.
} | ||
// We test that the first HZ is private or not, if it's not match the field private_zone, we test the second one | ||
index := -1 | ||
if *resp.HostedZones[0].Config.PrivateZone == d.Get("private_zone") { |
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.
You might need to do d.Get("private_zone").(bool)
in this case, as Get()
and GetOk()
both return interface{}
when they return the value.
Hi, |
@mathieuherbert hi there! You are welcome! I had a look at the changes and it looks good. I would have few suggestions to consider which might help with the VPC ID logic implementation - especially, since the The search criteria are either:
Also, only a Private Zone can have an association with a VPC, and it can only be associated once with a Hosted Zone that has unique name (domain name, if you wish). It can be associated with multiple Hosted Zones, but they need to have different name (which implies different Hosted Zone altogether; different ID). Given the above, you could search for:
Where the (2) and (3) probably makes little sense (especially (2) would yield empty results), rest of them are useful, especially concerning a scenario when people have multiple Private Hosted Zones that have the same name. One could also add region there as the filter to be able to only consider VPCs from current or given regions when looking them up. This allows for removal of duplication around Note, that you need only to fetch details about each and every zone only when match against a given VPC has to be performed. I hope this helps! Let me know if you anything else - I am here to help. |
Adding an example of what happens when you try to associate VPC with a different private hosted zone that has the same name as the one that already has VPC associated with it (since you can have multiple zones having the same name): Thus, for a private zone, its name together with a VPC ID would be enough to identify it. |
Good work so far @mathieuherbert I agree with @kwilczynski about the ambiguity of the zone - it may look like a 🐰 hole by now, but we do need to support VPC ID as a field. Also I think we should not just simply take the 1st matching zone - as it may not be what the user wants. We should probably error out if we get
As @kwilczynski mentioned above you can have multiple private zones w/ the same name, but each zone would have 1-1 mapping between |
@mathieuherbert @radeksimko just to add, support for searching tags would also be nice (and might help resolve some of the issues around multiple zone with the same name). |
Thank you for the help.
Thank you! |
In my last commit I added the support of tags |
Hi, |
Would be nice to output the NS records for the zone. We would use this for delegating zones from other zones and dns providers |
Thanks for all the work here - this LGTM! now :)
Paul |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
To add Hosted Zone data source for AWS provider.
For example we can do that with this data source
The acceptance test