Skip to content
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.

Sign up for GitHub

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 - CloudFront custom_error_response fixes for missing #6382

Merged
merged 2 commits into from
Apr 27, 2016
Merged

provider/aws - CloudFront custom_error_response fixes for missing #6382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a319b27
provider/aws - CloudFront custom_error_response fixes for missing
jrnt30 Apr 27, 2016
b2e15b1
- Fixing ACC test case resource names
jrnt30 Apr 27, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion builtin/providers/aws/cloudfront_distribution_configuration_structure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -719,12 +719,15 @@ func expandCustomErrorResponse(m map[string]interface{}) *cloudfront.CustomError
if v, ok := m["error_caching_min_ttl"]; ok {
er.ErrorCachingMinTTL = aws.Int64(int64(v.(int)))
}
if v, ok := m["response_code"]; ok {
if v, ok := m["response_code"]; ok && v.(int) != 0 {
er.ResponseCode = aws.String(strconv.Itoa(v.(int)))
} else {
er.ResponseCode = aws.String("")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually want to send an empty string here or just omit it from the request completely?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do. If the element is missing AWS will error out unless they are present (and in our case empty).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stack72, as @jrnt30 mentioned, this is actually necessary, because the attribute is TypeInt, but the API and SDK expect string. There's a breakdown in the conversion between @jrnt30 in the referenced issue.

The reasoning for keeping response_code as a TypeInt is three-fold:

  • It's the HTTP response code that needs to be sent back (and hence can only be int)
  • It's that way in CloudFormation (which is what I was using as my reference implementation as much as possible)
  • cloudfront.CustomErrorResponse.ErrorCode (the incoming error code) is *int64, so it's kind of odd that this would be a *string.

As such, we have to set this to "", as the zero value of int, 0, converts to "0" with strconv.Itoa(), versus an empty string.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it! Just had to check :)

}
if v, ok := m["response_page_path"]; ok {
er.ResponsePagePath = aws.String(v.(string))
}

return &er
}

Expand Down
18 changes: 18 additions & 0 deletions builtin/providers/aws/cloudfront_distribution_configuration_structure_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,13 @@ func customErrorResponsesConfFirst() map[string]interface{} {
return customErrorResponsesConf()[0].(map[string]interface{})
}

func customErrorResponseConfNoResponseCode() map[string]interface{} {
er := customErrorResponsesConf()[0].(map[string]interface{})
er["response_code"] = 0
er["response_page_path"] = ""
return er
}

func viewerCertificateConfSetCloudFrontDefault() map[string]interface{} {
return map[string]interface{}{
"acm_certificate_arn": "",
Expand Down Expand Up @@ -759,6 +766,17 @@ func TestCloudFrontStructure_expandCustomErrorResponse(t *testing.T) {
}
}

func TestCloudFrontStructure_expandCustomErrorResponse_emptyResponseCode(t *testing.T) {
data := customErrorResponseConfNoResponseCode()
er := expandCustomErrorResponse(data)
if *er.ResponseCode != "" {
t.Fatalf("Expected ResponseCode to be empty string, got %v", *er.ResponseCode)
}
if *er.ResponsePagePath != "" {
t.Fatalf("Expected ResponsePagePath to be empty string, got %v", *er.ResponsePagePath)
}
}

func TestCloudFrontStructure_flattenCustomErrorResponse(t *testing.T) {
in := customErrorResponsesConfFirst()
er := expandCustomErrorResponse(in)
Expand Down
69 changes: 69 additions & 0 deletions builtin/providers/aws/resource_aws_cloudfront_distribution_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,24 @@ func TestAccAWSCloudFrontDistribution_noOptionalItemsConfig(t *testing.T) {
})
}

func TestAccAWSCloudFrontDistribution_noCustomErrorResponseConfig(t *testing.T) {
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckCloudFrontDistributionDestroy,
Steps: []resource.TestStep{
resource.TestStep{
Config: testAccAWSCloudFrontDistributionNoCustomErroResponseInfo,
Check: resource.ComposeTestCheckFunc(
testAccCheckCloudFrontDistributionExistence(
"aws_cloudfront_distribution.no_custom_error_responses",
),
),
},
},
})
}

func testAccCheckCloudFrontDistributionDestroy(s *terraform.State) error {
for k, rs := range s.RootModule().Resources {
if rs.Type != "aws_cloudfront_distribution" {
Expand Down Expand Up @@ -373,6 +391,57 @@ resource "aws_cloudfront_distribution" "multi_origin_distribution" {
}
`, rand.New(rand.NewSource(time.Now().UnixNano())).Int(), testAccAWSCloudFrontDistributionRetainConfig())

var testAccAWSCloudFrontDistributionNoCustomErroResponseInfo = fmt.Sprintf(`
variable rand_id {
default = %d
}

resource "aws_cloudfront_distribution" "no_custom_error_responses" {
origin {
domain_name = "www.example.com"
origin_id = "myCustomOrigin"
custom_origin_config {
http_port = 80
https_port = 443
origin_protocol_policy = "http-only"
origin_ssl_protocols = [ "SSLv3", "TLSv1" ]
}
}
enabled = true
comment = "Some comment"
default_cache_behavior {
allowed_methods = [ "DELETE", "GET", "HEAD", "OPTIONS", "PATCH", "POST", "PUT" ]
cached_methods = [ "GET", "HEAD" ]
target_origin_id = "myCustomOrigin"
smooth_streaming = false
forwarded_values {
query_string = false
cookies {
forward = "all"
}
}
viewer_protocol_policy = "allow-all"
min_ttl = 0
default_ttl = 3600
max_ttl = 86400
}
custom_error_response {
error_code = 404
error_caching_min_ttl = 30
}
restrictions {
geo_restriction {
restriction_type = "whitelist"
locations = [ "US", "CA", "GB", "DE" ]
}
}
viewer_certificate {
cloudfront_default_certificate = true
}
%s
}
`, rand.New(rand.NewSource(time.Now().UnixNano())).Int(), testAccAWSCloudFrontDistributionRetainConfig())

var testAccAWSCloudFrontDistributionNoOptionalItemsConfig = fmt.Sprintf(`
variable rand_id {
default = %d
Expand Down