provider/aws: CloudFront post-merge review updates (#6196)

* provider/aws: Fix hashing on CloudFront certificate parameters

Adding necessary type assertion to values on the viewer_certificate hash
function to ensure that certain fields are indeed not zero string
values, versus simply zero interface{} values (aka nil, as is such for a
map[string]interface{}).

* provider/aws: CloudFront complex structure error handling

Handle errors better on calls to d.Set() in the
aws_cloudfront_distribution, namely in flattenDistributionConfig(). Also
caught a bug in the setting of the origin attribute, was incorrectly
attempting to set origins.

* provider/aws: Pass pointers to set CloudFront primitives

Change a few d.Set() for primitives in aws_cloudfront_distribution and
aws_cloudfront_origin_access_identity to use the pointer versus a
dereference.

* docs: Fix CloudFront examples formatting

Ran each example thru terraform fmt to fix indentation.

* provider/aws: Remove delete retention on CloudFront tests

To play better with Travis and not bloat the test account with disabled
distributions.

Disable-only functionality has been retained - one can enable it with
the TF_TEST_CLOUDFRONT_RETAIN environment variable.

* provider/aws: CloudFront delete waiter error handling

The call to resourceAwsCloudFrontDistributionWaitUntilDeployed() on
deletion of CloudFront distributions was not trapping error messages,
causing issues with waiter failure.

builtin/providers/aws/cloudfront_distribution_configuration_structure.go

@@ -82,42 +82,74 @@ func expandDistributionConfig(d *schema.ResourceData) *cloudfront.DistributionCo
 // aws_cloudfront_distribution resource.
 //
 // Used by the aws_cloudfront_distribution Read function.
-func flattenDistributionConfig(d *schema.ResourceData, distributionConfig *cloudfront.DistributionConfig) {
+func flattenDistributionConfig(d *schema.ResourceData, distributionConfig *cloudfront.DistributionConfig) error {
-	d.Set("origins", flattenOrigins(distributionConfig.Origins))
+	var err error
-	d.Set("enabled", *distributionConfig.Enabled)
+
-	d.Set("default_cache_behavior", flattenDefaultCacheBehavior(distributionConfig.DefaultCacheBehavior))
+	d.Set("enabled", distributionConfig.Enabled)
-	d.Set("viewer_certificate", flattenViewerCertificate(distributionConfig.ViewerCertificate))
+	d.Set("price_class", distributionConfig.PriceClass)
-	d.Set("price_class", *distributionConfig.PriceClass)
+
+	err = d.Set("default_cache_behavior", flattenDefaultCacheBehavior(distributionConfig.DefaultCacheBehavior))
+	if err != nil {
+		return err
+	}
+	err = d.Set("viewer_certificate", flattenViewerCertificate(distributionConfig.ViewerCertificate))
+	if err != nil {
+		return err
+	}
 
 	if distributionConfig.CallerReference != nil {
-		d.Set("caller_reference", *distributionConfig.CallerReference)
+		d.Set("caller_reference", distributionConfig.CallerReference)
 	}
 	if distributionConfig.Comment != nil {
 		if *distributionConfig.Comment != "" {
-			d.Set("comment", *distributionConfig.Comment)
+			d.Set("comment", distributionConfig.Comment)
 		}
 	}
 	if distributionConfig.DefaultRootObject != nil {
-		d.Set("default_root_object", *distributionConfig.DefaultRootObject)
+		d.Set("default_root_object", distributionConfig.DefaultRootObject)
-	}
-	if distributionConfig.CustomErrorResponses != nil {
-		d.Set("custom_error_response", flattenCustomErrorResponses(distributionConfig.CustomErrorResponses))
-	}
-	if distributionConfig.CacheBehaviors != nil {
-		d.Set("cache_behavior", flattenCacheBehaviors(distributionConfig.CacheBehaviors))
-	}
-	if distributionConfig.Logging != nil && *distributionConfig.Logging.Enabled {
-		d.Set("logging_config", flattenLoggingConfig(distributionConfig.Logging))
-	}
-	if distributionConfig.Aliases != nil {
-		d.Set("aliases", flattenAliases(distributionConfig.Aliases))
-	}
-	if distributionConfig.Restrictions != nil {
-		d.Set("restrictions", flattenRestrictions(distributionConfig.Restrictions))
 	}
 	if distributionConfig.WebACLId != nil {
-		d.Set("web_acl_id", *distributionConfig.WebACLId)
+		d.Set("web_acl_id", distributionConfig.WebACLId)
 	}
+
+	if distributionConfig.CustomErrorResponses != nil {
+		err = d.Set("custom_error_response", flattenCustomErrorResponses(distributionConfig.CustomErrorResponses))
+		if err != nil {
+			return err
+		}
+	}
+	if distributionConfig.CacheBehaviors != nil {
+		err = d.Set("cache_behavior", flattenCacheBehaviors(distributionConfig.CacheBehaviors))
+		if err != nil {
+			return err
+		}
+	}
+	if distributionConfig.Logging != nil && *distributionConfig.Logging.Enabled {
+		err = d.Set("logging_config", flattenLoggingConfig(distributionConfig.Logging))
+		if err != nil {
+			return err
+		}
+	}
+	if distributionConfig.Aliases != nil {
+		err = d.Set("aliases", flattenAliases(distributionConfig.Aliases))
+		if err != nil {
+			return err
+		}
+	}
+	if distributionConfig.Restrictions != nil {
+		err = d.Set("restrictions", flattenRestrictions(distributionConfig.Restrictions))
+		if err != nil {
+			return err
+		}
+	}
+	if *distributionConfig.Origins.Quantity > 0 {
+		err = d.Set("origin", flattenOrigins(distributionConfig.Origins))
+		if err != nil {
+			return err
+		}
+	}
+
+	return nil
 }
 
 func expandDefaultCacheBehavior(m map[string]interface{}) *cloudfront.DefaultCacheBehavior {
@@ -872,10 +904,12 @@ func flattenViewerCertificate(vc *cloudfront.ViewerCertificate) *schema.Set {
 	if vc.IAMCertificateId != nil {
 		m["iam_certificate_id"] = *vc.IAMCertificateId
 		m["ssl_support_method"] = *vc.SSLSupportMethod
-	} else if vc.ACMCertificateArn != nil {
+	}
+	if vc.ACMCertificateArn != nil {
 		m["acm_certificate_arn"] = *vc.ACMCertificateArn
 		m["ssl_support_method"] = *vc.SSLSupportMethod
-	} else {
+	}
+	if vc.CloudFrontDefaultCertificate != nil {
 		m["cloudfront_default_certificate"] = *vc.CloudFrontDefaultCertificate
 	}
 	if vc.MinimumProtocolVersion != nil {
@@ -889,16 +923,16 @@ func flattenViewerCertificate(vc *cloudfront.ViewerCertificate) *schema.Set {
 func viewerCertificateHash(v interface{}) int {
 	var buf bytes.Buffer
 	m := v.(map[string]interface{})
-	if v, ok := m["iam_certificate_id"]; ok {
+	if v, ok := m["iam_certificate_id"]; ok && v.(string) != "" {
 		buf.WriteString(fmt.Sprintf("%s-", v.(string)))
 		buf.WriteString(fmt.Sprintf("%s-", m["ssl_support_method"].(string)))
-	} else if v, ok := m["acm_certificate_arn"]; ok {
+	} else if v, ok := m["acm_certificate_arn"]; ok && v.(string) != "" {
 		buf.WriteString(fmt.Sprintf("%s-", v.(string)))
 		buf.WriteString(fmt.Sprintf("%s-", m["ssl_support_method"].(string)))
 	} else {
 		buf.WriteString(fmt.Sprintf("%t-", m["cloudfront_default_certificate"].(bool)))
 	}
-	if v, ok := m["minimum_protocol_version"]; ok {
+	if v, ok := m["minimum_protocol_version"]; ok && v.(string) != "" {
 		buf.WriteString(fmt.Sprintf("%s-", v.(string)))
 	}
 	return hashcode.String(buf.String())

builtin/providers/aws/cloudfront_distribution_configuration_structure_test.go

@@ -193,12 +193,18 @@ func customErrorResponsesConfFirst() map[string]interface{} {
 
 func viewerCertificateConfSetCloudFrontDefault() map[string]interface{} {
 	return map[string]interface{}{
+		"acm_certificate_arn":            "",
 		"cloudfront_default_certificate": true,
+		"iam_certificate_id":             "",
+		"minimum_protocol_version":       "",
+		"ssl_support_method":             "",
 	}
 }
 
 func viewerCertificateConfSetIAM() map[string]interface{} {
 	return map[string]interface{}{
+		"acm_certificate_arn":            "",
+		"cloudfront_default_certificate": false,
 		"iam_certificate_id":             "iamcert-01234567",
 		"ssl_support_method":             "vip",
 		"minimum_protocol_version":       "TLSv1",
@@ -208,6 +214,8 @@ func viewerCertificateConfSetIAM() map[string]interface{} {
 func viewerCertificateConfSetACM() map[string]interface{} {
 	return map[string]interface{}{
 		"acm_certificate_arn":            "arn:aws:acm:us-east-1:123456789012:certificate/12345678-1234-1234-1234-123456789012",
+		"cloudfront_default_certificate": false,
+		"iam_certificate_id":             "",
 		"ssl_support_method":             "sni-only",
 		"minimum_protocol_version":       "TLSv1",
 	}
@@ -987,3 +995,33 @@ func TestCloudFrontStructure_falttenViewerCertificate_acm_certificate_arn(t *tes
 		t.Fatalf("Expected out to be %v, got %v, diff: %v", in, out, diff)
 	}
 }
+
+func TestCloudFrontStructure_viewerCertificateHash_IAM(t *testing.T) {
+	in := viewerCertificateConfSetIAM()
+	out := viewerCertificateHash(in)
+	expected := 1157261784
+
+	if expected != out {
+		t.Fatalf("Expected %v, got %v", expected, out)
+	}
+}
+
+func TestCloudFrontStructure_viewerCertificateHash_ACM(t *testing.T) {
+	in := viewerCertificateConfSetACM()
+	out := viewerCertificateHash(in)
+	expected := 2883600425
+
+	if expected != out {
+		t.Fatalf("Expected %v, got %v", expected, out)
+	}
+}
+
+func TestCloudFrontStructure_viewerCertificateHash_default(t *testing.T) {
+	in := viewerCertificateConfSetCloudFrontDefault()
+	out := viewerCertificateHash(in)
+	expected := 69840937
+
+	if expected != out {
+		t.Fatalf("Expected %v, got %v", expected, out)
+	}
+}

builtin/providers/aws/resource_aws_cloudfront_distribution.go

@@ -492,15 +492,21 @@ func resourceAwsCloudFrontDistributionRead(d *schema.ResourceData, meta interfac
 	}
 
 	// Update attributes from DistributionConfig
-	flattenDistributionConfig(d, resp.Distribution.DistributionConfig)
+	err = flattenDistributionConfig(d, resp.Distribution.DistributionConfig)
+	if err != nil {
+		return err
+	}
 	// Update other attributes outside of DistributionConfig
 	d.SetId(*resp.Distribution.Id)
-	d.Set("active_trusted_signers", flattenActiveTrustedSigners(resp.Distribution.ActiveTrustedSigners))
+	err = d.Set("active_trusted_signers", flattenActiveTrustedSigners(resp.Distribution.ActiveTrustedSigners))
-	d.Set("status", *resp.Distribution.Status)
+	if err != nil {
-	d.Set("domain_name", *resp.Distribution.DomainName)
+		return err
+	}
+	d.Set("status", resp.Distribution.Status)
+	d.Set("domain_name", resp.Distribution.DomainName)
 	d.Set("last_modified_time", aws.String(resp.Distribution.LastModifiedTime.String()))
-	d.Set("in_progress_validation_batches", *resp.Distribution.InProgressInvalidationBatches)
+	d.Set("in_progress_validation_batches", resp.Distribution.InProgressInvalidationBatches)
-	d.Set("etag", *resp.ETag)
+	d.Set("etag", resp.ETag)
 	return nil
 }
 
@@ -537,7 +543,10 @@ func resourceAwsCloudFrontDistributionDelete(d *schema.ResourceData, meta interf
 	}
 
 	// Distribution needs to be in deployed state again before it can be deleted.
-	resourceAwsCloudFrontDistributionWaitUntilDeployed(d.Id(), meta)
+	err = resourceAwsCloudFrontDistributionWaitUntilDeployed(d.Id(), meta)
+	if err != nil {
+		return err
+	}
 
 	// now delete
 	params := &cloudfront.DeleteDistributionInput{

builtin/providers/aws/resource_aws_cloudfront_distribution_test.go

@@ -3,6 +3,7 @@ package aws
 import (
 	"fmt"
 	"math/rand"
+	"os"
 	"testing"
 	"time"
 
@@ -12,6 +13,11 @@ import (
 	"github.com/hashicorp/terraform/terraform"
 )
 
+// TestAccAWSCloudFrontDistribution_S3Origin runs an
+// aws_cloudfront_distribution acceptance test with a single S3 origin.
+//
+// If you are testing manually and can't wait for deletion, set the
+// TF_TEST_CLOUDFRONT_RETAIN environment variable.
 func TestAccAWSCloudFrontDistribution_S3Origin(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t) },
@@ -30,6 +36,11 @@ func TestAccAWSCloudFrontDistribution_S3Origin(t *testing.T) {
 	})
 }
 
+// TestAccAWSCloudFrontDistribution_customOriginruns an
+// aws_cloudfront_distribution acceptance test with a single custom origin.
+//
+// If you are testing manually and can't wait for deletion, set the
+// TF_TEST_CLOUDFRONT_RETAIN environment variable.
 func TestAccAWSCloudFrontDistribution_customOrigin(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t) },
@@ -48,6 +59,11 @@ func TestAccAWSCloudFrontDistribution_customOrigin(t *testing.T) {
 	})
 }
 
+// TestAccAWSCloudFrontDistribution_multiOrigin runs an
+// aws_cloudfront_distribution acceptance test with multiple origins.
+//
+// If you are testing manually and can't wait for deletion, set the
+// TF_TEST_CLOUDFRONT_RETAIN environment variable.
 func TestAccAWSCloudFrontDistribution_multiOrigin(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t) },
@@ -66,6 +82,11 @@ func TestAccAWSCloudFrontDistribution_multiOrigin(t *testing.T) {
 	})
 }
 
+// TestAccAWSCloudFrontDistribution_noOptionalItemsConfig runs an
+// aws_cloudfront_distribution acceptance test with no optional items set.
+//
+// If you are testing manually and can't wait for deletion, set the
+// TF_TEST_CLOUDFRONT_RETAIN environment variable.
 func TestAccAWSCloudFrontDistribution_noOptionalItemsConfig(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t) },
@@ -89,11 +110,16 @@ func testAccCheckCloudFrontDistributionDestroy(s *terraform.State) error {
 		if rs.Type != "aws_cloudfront_distribution" {
 			continue
 		}
-		dist, _ := testAccAuxCloudFrontGetDistributionConfig(s, k)
+		dist, err := testAccAuxCloudFrontGetDistributionConfig(s, k)
-
+		if err == nil {
+			if _, ok := os.LookupEnv("TF_TEST_CLOUDFRONT_RETAIN"); ok {
 				if *dist.DistributionConfig.Enabled != false {
 					return fmt.Errorf("CloudFront distribution should be disabled")
 				}
+				return nil
+			}
+			return fmt.Errorf("CloudFront distribution did not destroy")
+		}
 	}
 	return nil
 }
@@ -130,6 +156,13 @@ func testAccAuxCloudFrontGetDistributionConfig(s *terraform.State, cloudFrontRes
 	return res.Distribution, nil
 }
 
+func testAccAWSCloudFrontDistributionRetainConfig() string {
+	if _, ok := os.LookupEnv("TF_TEST_CLOUDFRONT_RETAIN"); ok {
+		return "retain_on_delete = true"
+	}
+	return ""
+}
+
 var testAccAWSCloudFrontDistributionS3Config = fmt.Sprintf(`
 variable rand_id {
 	default = %d
@@ -179,9 +212,9 @@ resource "aws_cloudfront_distribution" "s3_distribution" {
 	viewer_certificate {
 		cloudfront_default_certificate = true
 	}
-	retain_on_delete = true
+	%s
 }
-`, rand.New(rand.NewSource(time.Now().UnixNano())).Int())
+`, rand.New(rand.NewSource(time.Now().UnixNano())).Int(), testAccAWSCloudFrontDistributionRetainConfig())
 
 var testAccAWSCloudFrontDistributionCustomConfig = fmt.Sprintf(`
 variable rand_id {
@@ -234,9 +267,9 @@ resource "aws_cloudfront_distribution" "custom_distribution" {
 	viewer_certificate {
 		cloudfront_default_certificate = true
 	}
-	retain_on_delete = true
+	%s
 }
-`, rand.New(rand.NewSource(time.Now().UnixNano())).Int())
+`, rand.New(rand.NewSource(time.Now().UnixNano())).Int(), testAccAWSCloudFrontDistributionRetainConfig())
 
 var testAccAWSCloudFrontDistributionMultiOriginConfig = fmt.Sprintf(`
 variable rand_id {
@@ -336,9 +369,9 @@ resource "aws_cloudfront_distribution" "multi_origin_distribution" {
 	viewer_certificate {
 		cloudfront_default_certificate = true
 	}
-	retain_on_delete = true
+	%s
 }
-`, rand.New(rand.NewSource(time.Now().UnixNano())).Int())
+`, rand.New(rand.NewSource(time.Now().UnixNano())).Int(), testAccAWSCloudFrontDistributionRetainConfig())
 
 var testAccAWSCloudFrontDistributionNoOptionalItemsConfig = fmt.Sprintf(`
 variable rand_id {
@@ -383,6 +416,6 @@ resource "aws_cloudfront_distribution" "no_optional_items" {
 	viewer_certificate {
 		cloudfront_default_certificate = true
 	}
-	retain_on_delete = true
+	%s
 }
-`, rand.New(rand.NewSource(time.Now().UnixNano())).Int())
+`, rand.New(rand.NewSource(time.Now().UnixNano())).Int(), testAccAWSCloudFrontDistributionRetainConfig())

builtin/providers/aws/resource_aws_cloudfront_origin_access_identity.go

@@ -71,8 +71,8 @@ func resourceAwsCloudFrontOriginAccessIdentityRead(d *schema.ResourceData, meta
 	flattenOriginAccessIdentityConfig(d, resp.CloudFrontOriginAccessIdentity.CloudFrontOriginAccessIdentityConfig)
 	// Update other attributes outside of DistributionConfig
 	d.SetId(*resp.CloudFrontOriginAccessIdentity.Id)
-	d.Set("etag", *resp.ETag)
+	d.Set("etag", resp.ETag)
-	d.Set("s3_canonical_user_id", *resp.CloudFrontOriginAccessIdentity.S3CanonicalUserId)
+	d.Set("s3_canonical_user_id", resp.CloudFrontOriginAccessIdentity.S3CanonicalUserId)
 	d.Set("cloudfront_access_identity_path", fmt.Sprintf("origin-access-identity/cloudfront/%s", *resp.CloudFrontOriginAccessIdentity.Id))
 	return nil
 }
@@ -124,7 +124,7 @@ func expandOriginAccessIdentityConfig(d *schema.ResourceData) *cloudfront.Origin
 
 func flattenOriginAccessIdentityConfig(d *schema.ResourceData, originAccessIdentityConfig *cloudfront.OriginAccessIdentityConfig) {
 	if originAccessIdentityConfig.Comment != nil {
-		d.Set("comment", *originAccessIdentityConfig.Comment)
+		d.Set("comment", originAccessIdentityConfig.Comment)
 	}
-	d.Set("caller_reference", *originAccessIdentityConfig.CallerReference)
+	d.Set("caller_reference", originAccessIdentityConfig.CallerReference)
 }

website/source/docs/providers/aws/r/cloudfront_distribution.html.markdown

@@ -29,41 +29,52 @@ resource "aws_cloudfront_distribution" "s3_distribution" {
   origin {
     domain_name = "mybucket.s3.amazonaws.com"
     origin_id   = "myS3Origin"
+
     s3_origin_config {
       origin_access_identity = "origin-access-identity/cloudfront/ABCDEFG1234567"
     }
   }
+
   enabled             = true
   comment             = "Some comment"
   default_root_object = "index.html"
+
   logging_config {
     include_cookies = false
     bucket          = "mylogs.s3.amazonaws.com"
     prefix          = "myprefix"
   }
-	aliases = [ "mysite.example.com", "yoursite.example.com" ]
+
+  aliases = ["mysite.example.com", "yoursite.example.com"]
+
   default_cache_behavior {
-		allowed_methods = [ "DELETE", "GET", "HEAD", "OPTIONS", "PATCH", "POST", "PUT" ]
+    allowed_methods  = ["DELETE", "GET", "HEAD", "OPTIONS", "PATCH", "POST", "PUT"]
-		cached_methods = [ "GET", "HEAD" ]
+    cached_methods   = ["GET", "HEAD"]
     target_origin_id = "myS3Origin"
+
     forwarded_values {
       query_string = false
+
       cookies {
         forward = "none"
       }
     }
+
     viewer_protocol_policy = "allow-all"
     min_ttl                = 0
     default_ttl            = 3600
     max_ttl                = 86400
   }
+
   price_class = "PriceClass_200"
+
   restrictions {
     geo_restriction {
       restriction_type = "whitelist"
-			locations = [ "US", "CA", "GB", "DE" ]
+      locations        = ["US", "CA", "GB", "DE"]
     }
   }
+
   viewer_certificate {
     cloudfront_default_certificate = true
   }