AWS aws_cloudfront_distribution custom_error_response error

[kristjanelias]

Hi,

Started using the Cloudfront distribution (AWS) resource and hit a problem.
When defining custom_error_response block and leaving response_code and response_page_path (both optional attributes) undefined i get an error like:

...
aws_cloudfront_distribution.cf: Modifying...
  custom_error_response.#:                               "0" => "1"
  custom_error_response.860345937.error_caching_min_ttl: "" => "30"
  custom_error_response.860345937.error_code:            "" => "404"
  custom_error_response.860345937.response_code:         "" => ""
  custom_error_response.860345937.response_page_path:    "" => ""
Error applying plan:

1 error(s) occurred:

* aws_cloudfront_distribution.cf: IllegalUpdate: Your request must specify both ResponsePagePath and ResponseCode together or both should be empty.
    status code: 400, request id: c08f70b1-0ad0-11e6-8cec-59ec2c8f45a1

Terraform does not automatically rollback in the face of errors.
Instead, your Terraform state file has been partially updated with
any resources that successfully completed. Please address the error
above and apply again to incrementally change your infrastructure.

The code block in question inside the resource looks like:

custom_error_response {
    error_caching_min_ttl = 30
    error_code            = 404
    # response_code
    # response_page_path
  }

Haven't found any way of bypassing this yet except providing these 2 values...
But as it is not mandatory i would rather not. Hope there is a solution.

My example is when i am trying to add the custom_error_response to an existing CloudFront resource.
Same error appears on a resource that is being created when the two values are omitted.

Kristjan

[jen20]

Hi @kristjanelias! Thanks for opening an issue. This definitely looks like a bug given the combination of plan, configuration and error message. We'll get this taken care of!

[vancluever]

@jen20 I know what's up here - there was a similar issue that came up pre-merge due to the fact that primitives in some of the sub-structures were not being checked for their underlying zero values. I can fix this if you want - it should not take long. I'm taking a stab at it this morning and if I finish quickly here I'll put in the PR at least.

If you guys have started on it already let me know!

[jrnt30]

@vancluever I started an attempt myself @ https://github.com/jrnt30/terraform/tree/GH-6342-cloudfront-custom-error but would be very interested in seeing if there is a better more idiomatic way of approaching this. I was going to start some testing with the plan -> apply -> plan -> update, etc. to ensure the state file remains consistent.

There is a bit of a strangeness with the diff output but had to start work :)

[vancluever]

@jrnt30 I think that's exactly what is needed here, unless the SDK docs are lying, then those fields should not be required and their respective pointer values should be nil. I'm taking a slightly different approach to testing initially first though and writing unit tests for the expansion functions, so that it's certain that the helper functions are behaving as intended first. Then once that's taken care of then integration tests can be tried, and if the API is behaving different than the SDK mentions, we can adjust accordingly!

[jrnt30]

@vancluever Leaving the fields as nil was my first approach, however when sending the request with those fields present at all, AWS will send a different error response about the custom error not being present so I went that around, setting them all to empty.

That resulted in the underlying XML request having the fields present but unset and the distribution being created properly.

I also looked around to see if there was a better way of finding if the value was set properly or not with something like the "GetOk" method on the ResourceData but didn't see anything for the schema.Set. Is there a better way of checking for these fields if the value is the "Zero" value of that type.

[vancluever]

@jrnt30 cool - I'll check it out. Pulling in your test right now and about to give it a go!

[vancluever]

@jrnt30 Confirmed your issue. I'm wondering if all of these fields should just be moved to being required, seeing as I can't seem to find an answer in the SDK docs on what they should be if they are not specified, and PUT DistributionConfig doesn't seem to really say either. Just doing a quick check of the CloudFormation docs before I make the decision.

[jrnt30]

If you keep them as empty strings it will work properly. So in my branch it checks for the default int value of 0 and if present, changes it to "", which results in the XML config rendering the attribute but no value actually being associated with it. This does allow for the same behavior you can get via the UI which is nice, just seems a bit janky.

I wanted to look into the state rehydration stuff with this approach, but in general that seemed to work.

[jrnt30]

Oh, I just noticed one difference between our branches, jrnt30@60bef88#diff-754b6e16b29ee15b1d7463edb2e1f4f7R724

Instead of leaving nil, I explicitly set that response_code to ""

[vancluever]

Ahh, I betcha it's because strconv.Itoa(0) is "0", versus "".

And response_page_path might have been OK the way it was, without checking for any empty values. I'm on the fence here on to be explicit or not on this - I've noticed one other thing where if that test on origin_path is not defined either (another "optional" field), it fails as well:

IllegalUpdate: The 'originPath' field is missing.

Trying to decide if redundant else in these cases better than just leaving it as is....

[jrnt30]

The redundant "else" was only required for the response_error field in my trails. paybyphone@f4f09be#diff-754b6e16b29ee15b1d7463edb2e1f4f7R722 , will leave the response_error nil which results in it not being sent via the SDK call and failing.

I just removed the check for response_page_path since that itself results in my desired outcome (an empty string)

[vancluever]

@jrnt30 yeah, you know what, I think your branch is okay. No need introducing anything unintended into this if it's not needed, and it looks like the CloudFront API is happy enough with empty values in a lot of places.

Slowly backing away 😉

[vancluever]

PS: The only thing I would recommend adding is a unit test for this in cloudfront_distribution_configuration_structure_test.go for the case, to test that a value of 0 for ResponseCode translates into "".

[jrnt30]

Oh no, you get back here, you touched it ;) I'll take a look at the test. If you have it setup already we can just merge it in and issue the PR.

Thanks for the feedback and discussion as well, trying to get oriented to the whole stack is a bit overwhelming

[vancluever]

Haha ok, I dropped my branch, I'll pull in yours, add the unit test and put in the PR.

Oh, also to answer your question on zero values on map[string]interface{} - I've looked into that as well and haven't seen much. Luckily it hasn't come up as an issue too much for me where it's burned me, except in these few minor instances. What I might look into when writing future code is how to treat sets that are actually complex structures as *schema.ResourceData values instead - I'm not too sure if that's possible, but it could be worth a shot. If that can happen, you'd have access to GetOk. However, it seems that every example I've looked at so far seems to show treating this kind of pattern as map[string]interface{}, however, and with that you are kind of on your own to check the underlying zero value properly.

[vancluever]

@jrnt30, I've added the test, see paybyphone@c5b0bdc.

Not going to put in the PR yet because I think the first two commits should be squashed, seeing as the second is a sort of a revert of the first one. But otherwise I think it's good to go 👍

[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 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.