Added public_dns and private_dns to aws_eip

[hamstah]

Fixes #1149

Changes proposed in this pull request:

• Added public_dns and private_dns for aws_eip data and resource

Looking at the discussion in #3731 this seems like a more simple implementation than using the reverse lookup.

I've created/looked up IPs in different regions (us-east-1, us-west-1, us-east-2, eu-west-1) to double check they follow the documentation.

Could you please advise on the best way to add unit tests for the new attributes?

Thanks

Output from acceptance testing:

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSAvailabilityZones -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSAvailabilityZones_basic
=== PAUSE TestAccAWSAvailabilityZones_basic
=== RUN   TestAccAWSAvailabilityZones_stateFilter
=== PAUSE TestAccAWSAvailabilityZones_stateFilter
=== CONT  TestAccAWSAvailabilityZones_basic
=== CONT  TestAccAWSAvailabilityZones_stateFilter
--- PASS: TestAccAWSAvailabilityZones_stateFilter (25.13s)
--- PASS: TestAccAWSAvailabilityZones_basic (25.13s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	25.153s
...

[bflad]

Hey @hamstah 👋 Thanks for this contribution! Left some minor items below. Please reach out with any questions.

Could you please advise on the best way to add unit tests for the new attributes?

As written, these attributes would require integration/acceptance testing as there is not a simple function boundary to unit test. For acceptance testing, I would recommend adding something like the following into the Check portion of TestAccAWSEIP_basic:

// inside the TestStep

testAccCheckAWSEIPPrivateDNS("aws_eip.bar"),
testAccCheckAWSEIPPublicDNS("aws_eip.bar"),

// outside the acceptance test function

func testAccCheckAWSEIPPrivateDNS(resourceName string) resource.TestCheckFunc {
	return func(s *terraform.State) error {
		rs, ok := s.RootModule().Resources[resourceName]
		if !ok {
			return fmt.Errorf("Not found: %s", resourceName)
		}

		privateIPDashed := strings.Replace(rs.Primary.Attributes["private_ip"], ".", "-", -1)
		privateDNS := rs.Primary.Attributes["private_dns"]
		expectedPrivateDNS := fmt.Sprintf("ip-%s.%s.compute.internal", privateIPDashed, testAccGetRegion())

		if testAccGetRegion() == "us-east-1" {
			expectedPrivateDNS = fmt.Sprintf("ip-%s.ec2.internal", privateIPDashed)
		}

		if privateDNS != expectedPrivateDNS {
			return fmt.Errorf("expected private_dns value (%s), received: %s", expectedPrivateDNS, privateDNS)
		}

		return nil
	}
}

func testAccCheckAWSEIPPublicDNS(resourceName string) resource.TestCheckFunc {
	return func(s *terraform.State) error {
		rs, ok := s.RootModule().Resources[resourceName]
		if !ok {
			return fmt.Errorf("Not found: %s", resourceName)
		}

		publicIPDashed := strings.Replace(rs.Primary.Attributes["public_ip"], ".", "-", -1)
		publicDNS := rs.Primary.Attributes["public_dns"]
		expectedPublicDNS := fmt.Sprintf("ec2-%s.%s.compute.amazonaws.com", publicIPDashed, testAccGetRegion())

		if testAccGetRegion() == "us-east-1" {
			expectedPublicDNS = fmt.Sprintf("ec2-%s.compute.amazonaws.com", publicIPDashed)
		}

		if publicDNS != expectedPublicDNS {
			return fmt.Errorf("expected public_dns value (%s), received: %s", expectedPublicDNS, publicDNS)
		}

		return nil
	}
}

Since the attribute value is dependent on region-specific logic, unfortunately we cannot use the simple resource.TestCheckResourceAttr("aws_eip.bar", "private_dns", ...) in this case, hence the more complicated implementation above.

If you are interested in unit testing instead, I would recommend creating two functions to output the private/public DNS from the IP:

func ec2PrivateIPDNS(privateIP, region string) string {
	privateIPDashed := strings.Replace(privateIP, ".", "-", -1)
	privateDNS := fmt.Sprintf("ip-%s.%s.compute.internal", privateIPDashed, region)

	if region == "us-east-1" {
		privateDNS = fmt.Sprintf("ip-%s.ec2.internal", privateIPDashed)
	}

	return privateDNS
}

func ec2PublicIPDNS(publicIP, region string) string {
	publicIPDashed := strings.Replace(publicIP, ".", "-", -1)
	publicDNS := fmt.Sprintf("ec2-%s.%s.compute.amazonaws.com", publicIPDashed, region)

	if region == "us-east-1" {
		publicDNS = fmt.Sprintf("ec2-%s.compute-1.amazonaws.com", publicIPDashed)
	}

	return publicDNS
}

Then unit test via:

func TestEc2PrivateIPDNS(t *testing.T) {
	testCases := []struct {
		PrivateIP  string
		Region string
		ExpectedPrivateDNS string
	}{
		{
			PrivateIP:  "1.2.3.4",
			Region: "us-east-1",
			ExpectedPrivateDNS: "ip-1-2-3-4.ec2.internal",
		},
		{
			PrivateIP:  "1.2.3.4",
			Region: "us-west-2",
			ExpectedPrivateDNS: "ip-1-2-3-4.us-west-2.compute.internal",
		},
	}

	for i, tc := range testCases {
		privateDNS := ec2PrivateIPDNS(tc.PrivateIP, tc.Region)
		if privateDNS != tc.ExpectedPrivateDNS {
			t.Fatalf("%d: expected private DNS (%s), received: %s", i, tc.ExpectedPrivateDNS, privateDNS)
		}
	}
}

func TestEc2PublicIPDNS(t *testing.T) {
	testCases := []struct {
		PublicIP  string
		Region string
		ExpectedPublicDNS string
	}{
		{
			PublicIP:  "1.2.3.4",
			Region: "us-east-1",
			ExpectedPublicDNS: "ec2-1-2-3-4.compute-1.amazonaws.com",
		},
		{
			PrivateIP:  "1.2.3.4",
			Region: "us-west-2",
			ExpectedPublicDNS: "ec2-1-2-3-4.us-west-2.compute.amazonaws.com",
		},
	}

	for i, tc := range testCases {
		publicDNS := ec2PublicIPDNS(tc.PublicIP, tc.Region)
		if publicDNS != tc.ExpectedPublicDNS {
			t.Fatalf("%d: expected public DNS (%s), received: %s", i, tc.ExpectedPublicDNS, publicDNS)
		}
	}
}

Converting the logic of both of these to functions would be nice. 👍 Even with unit testing these, I think we would still prefer to include the acceptance testing as well.

[bflad]

Optional: true should be removed here as operators cannot specify this argument.

[bflad]

Optional: true should be removed here as operators cannot specify this argument.

[bflad]

Minor nit: We tend to access the provider region across the codebase via region := meta.(*AWSClient).region instead of via the client.

[bflad]

Also FYI from your acceptance testing output above, it looks like you ran the aws_availability_zones data source acceptance tests. If you filter on TestAccAWSEIP_ and TestAccDataSourceAwsEip_ it will run the aws_eip resource and data source acceptance tests.

[bflad]

Hi again @hamstah 👋 Since we didn't hear back from you regarding this, we went ahead and implemented the feedback in a followup commit (ac09b67). Thanks for your contribution. 🚀

Output from acceptance testing:

--- PASS: TestAccAWSEIP_disappears (6.81s)
--- PASS: TestAccAWSEIP_PublicIpv4Pool_default (8.29s)
--- PASS: TestAccAWSEIP_basic (8.58s)
--- PASS: TestAccDataSourceAwsEip_Id (9.73s)
--- PASS: TestAccDataSourceAwsEip_PublicIP_VPC (10.02s)
--- PASS: TestAccDataSourceAwsEip_Filter (10.05s)
--- PASS: TestAccDataSourceAwsEip_Tags (11.68s)
--- PASS: TestAccDataSourceAwsEip_PublicIP_EC2Classic (15.80s)
--- PASS: TestAccAWSEIP_tags (16.97s)
--- PASS: TestAccAWSEIP_twoEIPsOneNetworkInterface (24.87s)
--- PASS: TestAccAWSEIP_network_interface (25.61s)
--- PASS: TestAccDataSourceAwsEip_NetworkInterface (27.86s)
--- PASS: TestAccAWSEIP_importVpc (28.10s)
--- PASS: TestAccAWSEIP_importEc2Classic (121.35s)
--- PASS: TestAccAWSEIP_associated_user_private_ip (138.09s)
--- PASS: TestAccDataSourceAwsEip_Instance (192.65s)
--- PASS: TestAccAWSEIP_instance (281.95s)

[bflad]

This has been released in version 2.2.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

[ghost]

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!