Add public_dns to Elastic Ips #3731

appilon · 2018-03-09T22:39:40Z

Fix #1149
Fix #3474 (modify config to reference the eip)

Uses net.LookupAddr to find public dns name based on ip address. In scenarios where DNS is not working correctly simply set to empty string (soft failure).

Also included minor refactor and cleanup of the data source acceptance tests.

- uses net.LookupAddr - set to empty string on error/no results - update docs - cleanup/refactor tests

apparentlymart

LGTM!

Some style-related feedback is inline. Since style is always subjective, feel free to disregard that feedback after considering it. 😀

apparentlymart · 2018-03-13T22:57:19Z

website/docs/d/eip.html.markdown

@@ -48,3 +48,6 @@ All of the argument attributes are also exported as result attributes. This
 data source will complete the data by populating any fields that are not
 included in the configuration with the data for the selected Elastic IP.

+* `public_dns` - DNS name of the allocated EIP
+
+__Note__: The `public_dns` attribute is provided as a convenience, it is computed via Go's built-in reverse lookup and we select only the first result returned. Depending on how and where Terraform is run this lookup may fail, in which case we simply leave it as an empty string `""`.


Thanks for including this note. I wonder if this is important enough to justify use of the ~> extension to create a warning box:

~> **Note:** The `public_dns` attribute ....

Also, we usually try to write our documentation as if it's in the voice of a "narrator" that is monitoring Terraform's progress and describing what it is doing, which tends to lead to sentences using the active voice rather than the passive voice. I think this would suggest wording like the following:

This resource exports the public_dns attribute as a convenience by performing a reverse-DNS lookup and taking the first record returned. In some environments this DNS request may fail, in which case the resource exports the empty string "".

(Note also that we don't generally talk about Go features because that's an implementation detail rather than something the user needs to worry about. Users shouldn't feel like they need to be familiar with Go in order to use Terraform.)

Noted, I will try and remember that and stay consistent going forward.

apparentlymart · 2018-03-13T22:58:22Z

aws/dns.go

+
+import "net"
+
+func reverseLookup(ip string) (name string) {


Since the context of this function is the entire aws package, I'd suggest the name reverseDNSLookup, just to make it clearer at the call site what sort of lookup this function does.

apparentlymart · 2018-03-13T23:10:08Z

aws/dns.go

+
+func reverseLookup(ip string) (name string) {
+	if names, err := net.LookupAddr(ip); err == nil && len(names) > 0 {
+		name = names[0]


Although this is valid Go, I think normal idiom in this case would be to use an unnamed return value and have multiple return statements:

if ... { return names[0] } return ""

This makes the result in the unsuccessful case more obvious. Named return values are usually used only where the name adds useful context in package documentation for functions with multiple return values of the same type.

This is discussed in the common Go code review comments. This is of course a subjective thing, so feel free to ignore this if you'd made a conscious decision here.

appilon · 2018-03-14T00:46:10Z

@apparentlymart I think @radeksimko had some questions/comments (he was sick I believe when we originally discussed this in the slack channel).

radeksimko

My concern with this enhancement is that it's introducing quite a lot of (hidden) complexity into otherwise simple resource.

The resource is suddenly responsible for a direct reverse DNS resolution which then brings in the burden of related DNS issues I'd expect to be already handled in the DNS provider.

e.g. what happens if there's firewall in the way blocking the resolution - then we're going to silently fail? or what if the user wants to set custom DNS servers for the resolution?

TL;DR Why can't this reside in the DNS provider instead?

bflad · 2018-03-20T13:27:13Z

Another option is that we could just synthesize these according to the VPC DNS documentation?

A public (external) DNS hostname takes the form ec2-public-ipv4-address.compute-1.amazonaws.com for the us-east-1 region, and ec2-public-ipv4-address.region.amazonaws.com for other regions.

I'm pretty sure that applies to EIPs as well, but probably worth verifying (potentially with AWS support). We can also submit an AWS feature support request to provide this value in the API. Worst case, I also lean towards recommending the DNS provider here as well to keep the complexity down. 👍

That said, DNS resolution could potentially be an issue if the resolver is also in EC2 within the same region/network (again, I think this also applies to EIPs):

We resolve a public DNS hostname to the public IPv4 address of the instance outside the network of the instance, and to the private IPv4 address of the instance from within the network of the instance.

appilon · 2018-03-22T16:13:28Z

@bflad

We resolve a public DNS hostname to the public IPv4 address of the instance outside the network of the instance, and to the private IPv4 address of the instance from within the network of the instance.

This was the goal of this PR, to address this issue: #1149

In some AWS forums the "recommended" way of referencing an EIP is the public DNS so that internal and external networks will resolve to the same machine.

I actually added the reverse lookup to the dns provider, is there some way I could recommend leveraging that? Another issue: #3474 wants to be able to pass the DNS name to route53 in one pass, would that be possible with the DNS provider in conjunction with the AWS provider.

catsby · 2018-04-25T17:08:38Z

Hello – this is a month old, if possible let's please resolve what's left and merge, or close the PR. Thanks 😉

appilon · 2018-04-25T18:45:39Z

I stopped working on this PR hoping for a consensus, from the comments it still appears to be debatable where or not we want this. I actually implemented the reverse lookup in the dns provider a while back, maybe the suggestion is to pass the EIP address to the dns provider for a reverse lookup and thus the user will have reference the the allocated DNS name of the elastic IP? If that is the accepted official workaround I can close this PR and comment on the associated issues?

Please react with 👍 if we want to merge (after code changes) this code, or 👎 if we would prefer suggest a workaround?

catsby · 2018-07-26T16:13:45Z

@appilon based on Radek's objection and this doc note:

Depending on how and where Terraform is run this lookup may fail, in which case we simply leave it as an empty string

I suggest we close this PR. If AWS does not provide a way to do this via API, then our attempts to do so in code can be subject to painful cat-and-mouse updates to support varied scenarios. I like Brian's suggestion of putting in a feature request, so we're not just throwing our hands up completely ¯_(ツ)_/¯

appilon · 2018-07-26T16:36:31Z

Yup I agree having code do things beyond what the provider API providers is probably a bad idea. I added functionality to the DNS provider a while ago that they could use in conjunction with their EIPs to get the DNS name.

ghost · 2020-04-04T17:30:15Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

ghost added size/M Managed by automation to categorize the size of a PR. labels Mar 9, 2018

appilon requested review from radeksimko and apparentlymart March 12, 2018 21:24

ghost added the size/M Managed by automation to categorize the size of a PR. label Mar 12, 2018

implement public_dns attr for eip

021bf77

- uses net.LookupAddr - set to empty string on error/no results - update docs - cleanup/refactor tests

ghost added the size/M Managed by automation to categorize the size of a PR. label Mar 12, 2018

bflad added enhancement Requests to existing resources that expand the functionality or scope. service/ec2 Issues and PRs that pertain to the ec2 service. labels Mar 13, 2018

apparentlymart approved these changes Mar 13, 2018

View reviewed changes

radeksimko reviewed Mar 20, 2018

View reviewed changes

catsby closed this Jul 26, 2018

hamstah mentioned this pull request Jan 27, 2019

Added public_dns and private_dns to aws_eip #7349

Merged

ghost locked and limited conversation to collaborators Apr 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add public_dns to Elastic Ips #3731

Add public_dns to Elastic Ips #3731

appilon commented Mar 9, 2018 •

edited

Loading

apparentlymart left a comment

apparentlymart Mar 13, 2018

appilon Mar 14, 2018

apparentlymart Mar 13, 2018

appilon Mar 14, 2018

apparentlymart Mar 13, 2018

appilon Mar 14, 2018

appilon commented Mar 14, 2018

radeksimko left a comment •

edited

Loading

bflad commented Mar 20, 2018

appilon commented Mar 22, 2018 •

edited

Loading

catsby commented Apr 25, 2018

appilon commented Apr 25, 2018

catsby commented Jul 26, 2018

appilon commented Jul 26, 2018

ghost commented Apr 4, 2020

Add public_dns to Elastic Ips #3731

Add public_dns to Elastic Ips #3731

Conversation

appilon commented Mar 9, 2018 • edited Loading

apparentlymart left a comment

Choose a reason for hiding this comment

apparentlymart Mar 13, 2018

Choose a reason for hiding this comment

appilon Mar 14, 2018

Choose a reason for hiding this comment

apparentlymart Mar 13, 2018

Choose a reason for hiding this comment

appilon Mar 14, 2018

Choose a reason for hiding this comment

apparentlymart Mar 13, 2018

Choose a reason for hiding this comment

appilon Mar 14, 2018

Choose a reason for hiding this comment

appilon commented Mar 14, 2018

radeksimko left a comment • edited Loading

Choose a reason for hiding this comment

bflad commented Mar 20, 2018

appilon commented Mar 22, 2018 • edited Loading

catsby commented Apr 25, 2018

appilon commented Apr 25, 2018

catsby commented Jul 26, 2018

appilon commented Jul 26, 2018

ghost commented Apr 4, 2020

appilon commented Mar 9, 2018 •

edited

Loading

radeksimko left a comment •

edited

Loading

appilon commented Mar 22, 2018 •

edited

Loading