resource/cloudfront_distribution: Added ordered cache behaviors #4117

Ninir · 2018-04-08T22:13:27Z

Description

This allows configuring CloudFront cache behavior precedence.
This is an alternative approach to #1269 and heavily inspired by it.

While the other PR is a breaking change and thus does not allow a quick merge, this approach introduces a new attribute called ordered_cache_behavior, deprecating cache_behavior.
This work is backward-compatible as it keeps the old behaviour and adds a new one, mainly benefiting from new methods. The previous methods are marked and named as XXXDeprecated.

It also fixes the sub-options allowed_methods and cached_methods which were lists whereas it should have been set.

Alternative & better names for the new attribute are welcomed :)

Sidenote: panic when running tests after 2hours, main tests + ordered cache behaviors are passing but might be a good idea to run a TC test.

Related issues

#188
#1269

Test

$ TF_LOG=DEBUG TF_LOG_PATH=terraform.log make testacc TEST=./aws TESTARGS='-run=TestAccAWSCloudFrontDistribution_'                    
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSCloudFrontDistribution_ -timeout 120m
=== RUN   TestAccAWSCloudFrontDistribution_importBasic
--- PASS: TestAccAWSCloudFrontDistribution_importBasic (1247.34s)
=== RUN   TestAccAWSCloudFrontDistribution_S3Origin
--- PASS: TestAccAWSCloudFrontDistribution_S3Origin (1323.08s)
=== RUN   TestAccAWSCloudFrontDistribution_S3OriginWithTags
--- PASS: TestAccAWSCloudFrontDistribution_S3OriginWithTags (1382.72s)
=== RUN   TestAccAWSCloudFrontDistribution_customOrigin
--- PASS: TestAccAWSCloudFrontDistribution_customOrigin (1373.03s)
=== RUN   TestAccAWSCloudFrontDistribution_multiOrigin
--- PASS: TestAccAWSCloudFrontDistribution_multiOrigin (1042.85s)
=== RUN   TestAccAWSCloudFrontDistribution_orderedCacheBehavior
--- PASS: TestAccAWSCloudFrontDistribution_orderedCacheBehavior (798.87s)
=== RUN   TestAccAWSCloudFrontDistribution_Origin_EmptyDomainName
--- PASS: TestAccAWSCloudFrontDistribution_Origin_EmptyDomainName (2.29s)
=== RUN   TestAccAWSCloudFrontDistribution_Origin_EmptyOriginID
--- PASS: TestAccAWSCloudFrontDistribution_Origin_EmptyOriginID (2.01s)
=== RUN   TestAccAWSCloudFrontDistribution_noOptionalItemsConfig
panic: test timed out after 2h0m0s

Ninir · 2018-04-08T22:39:13Z

aws/cloudfront_distribution_configuration_structure.go

+	if v, ok := d.GetOk("ordered_cache_behavior"); ok {
+		distributionConfig.CacheBehaviors = expandCacheBehaviors(v.([]interface{}))
+	} else {
+		distributionConfig.CacheBehaviors = expandCacheBehaviorsDeprecated(d.Get("cache_behavior").(*schema.Set))


Old behavior is kept under a expandCacheBehaviorsDeprecated function, which contains the exact same previous code.

Ninir · 2018-04-08T22:42:00Z

aws/cloudfront_distribution_configuration_structure.go

+		cb.SmoothStreaming = aws.Bool(v.(bool))
+	}
+	if v, ok := m["allowed_methods"]; ok {
+		cb.AllowedMethods = expandAllowedMethodsDeprecated(v.([]interface{}))


This also has the old behavior. The new behavior is a Set rather than lists for allowed_methods & cached_method

bflad · 2018-04-17T16:12:31Z

@Ninir hmm I wonder if you caught this right before the CloudFront SDK changed, but can you ensure this is rebased to handle field_level_encryption_id? #4102

I'm currently getting test failures for everything in TC using refs/pull/4117/merge (so merged with the SDK update in master):

--- FAIL: TestAccAWSCloudFrontDistribution_HTTP11Config (14.03s)
    testing.go:579: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.
        
        Error: Error applying: 1 error(s) occurred:
        
        * aws_cloudfront_distribution.http_1_1 (destroy): 1 error(s) occurred:
        
        * aws_cloudfront_distribution.http_1_1: InvalidArgument: The parameter Field level encryption id is required.
            status code: 400, request id: f688ddfe-4258-11e8-9d0c-4baa8bbf9e0d

Ninir · 2018-04-17T20:48:46Z

@bflad just rebased and currently running tests (which are passing right now)

Ninir · 2018-04-17T21:50:59Z

@bflad Would it be possible to trigger tests in TC?

still has the panic: test timed out after 2h0m0s after:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSCloudFrontDistribution_'           
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSCloudFrontDistribution_ -timeout 120m
=== RUN   TestAccAWSCloudFrontDistribution_importBasic
--- PASS: TestAccAWSCloudFrontDistribution_importBasic (1472.03s)
=== RUN   TestAccAWSCloudFrontDistribution_S3Origin
--- PASS: TestAccAWSCloudFrontDistribution_S3Origin (1433.31s)
=== RUN   TestAccAWSCloudFrontDistribution_S3OriginWithTags
--- PASS: TestAccAWSCloudFrontDistribution_S3OriginWithTags (1444.00s)
=== RUN   TestAccAWSCloudFrontDistribution_customOrigin
--- PASS: TestAccAWSCloudFrontDistribution_customOrigin (1313.38s)
=== RUN   TestAccAWSCloudFrontDistribution_multiOrigin
--- PASS: TestAccAWSCloudFrontDistribution_multiOrigin (1501.19s)
=== RUN   TestAccAWSCloudFrontDistribution_orderedCacheBehavior
panic: test timed out after 2h0m0s

Thanks!

bflad · 2018-04-17T23:53:14Z

@Ninir can you push up the rebase commit? 😄 Its not showing up in Github and still throwing the same error in TC.

Ninir · 2018-04-18T08:09:34Z

@bflad Yep done. Tried a few things to tweak that setting without success. Is there anything you do on TC to fix this panic?

bflad

This is awesome! 🚀 LGTM

12 tests passed (all tests)
=== RUN   TestAccAWSCloudFrontDistribution_Origin_EmptyOriginID
--- PASS: TestAccAWSCloudFrontDistribution_Origin_EmptyOriginID (1.55s)
=== RUN   TestAccAWSCloudFrontDistribution_Origin_EmptyDomainName
--- PASS: TestAccAWSCloudFrontDistribution_Origin_EmptyDomainName (1.73s)
=== RUN   TestAccAWSCloudFrontDistribution_HTTP11Config
--- PASS: TestAccAWSCloudFrontDistribution_HTTP11Config (1535.58s)
=== RUN   TestAccAWSCloudFrontDistribution_S3OriginWithTags
--- PASS: TestAccAWSCloudFrontDistribution_S3OriginWithTags (1536.80s)
=== RUN   TestAccAWSCloudFrontDistribution_orderedCacheBehavior
--- PASS: TestAccAWSCloudFrontDistribution_orderedCacheBehavior (1538.30s)
=== RUN   TestAccAWSCloudFrontDistribution_S3Origin
--- PASS: TestAccAWSCloudFrontDistribution_S3Origin (1539.00s)
=== RUN   TestAccAWSCloudFrontDistribution_IsIPV6EnabledConfig
--- PASS: TestAccAWSCloudFrontDistribution_IsIPV6EnabledConfig (1539.26s)
=== RUN   TestAccAWSCloudFrontDistribution_multiOrigin
--- PASS: TestAccAWSCloudFrontDistribution_multiOrigin (1540.74s)
=== RUN   TestAccAWSCloudFrontDistribution_customOrigin
--- PASS: TestAccAWSCloudFrontDistribution_customOrigin (1541.16s)
=== RUN   TestAccAWSCloudFrontDistribution_noOptionalItemsConfig
--- PASS: TestAccAWSCloudFrontDistribution_noOptionalItemsConfig (1542.22s)
=== RUN   TestAccAWSCloudFrontDistribution_noCustomErrorResponseConfig
--- PASS: TestAccAWSCloudFrontDistribution_noCustomErrorResponseConfig (1543.61s)
=== RUN   TestAccAWSCloudFrontDistribution_importBasic
--- PASS: TestAccAWSCloudFrontDistribution_importBasic (1543.98s)

bflad · 2018-04-18T18:21:05Z

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

danielfm · 2018-04-18T18:50:30Z

@Ninir First of all, thanks for this fix! 🎉

I did a test by importing an existing CloudFront distribution with v1.15.0, but apparently it still uses the now deprecated cache_behavior, instead of the new ordered_cache_behavior. Are there any plans of changing the import code as well to leverage the new ordered_cache_behavior directive?

Also, is it safe to update a Terraform-managed distribution by replacing all cache_behavior by ordered_cache_behavior? Will Terraform delete and then re-create all cache behaviors (which might cause a brief unavailability), or just reorder the existing cache behaviors (much less disruptive)?

Ninir · 2018-04-18T20:57:56Z

Hey @danielfm

It indeed still uses the old behaviour. I tried to come up with solutions about migrating the old behaviour to the new one without success: migrating the schema is a tough task that was nearly impossible to handle in some cases, hence the "opt-in" proposal.

Since both of these 2 attributes are conflicting with the other one, you can only set one or the other, meaning if you try to set ordered ones in place of obsolete ones, there will just be a replacement & Distribution update.

Hope it's clear enough! :)

ghost · 2020-04-06T17:40:01Z

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!

Ninir added enhancement Requests to existing resources that expand the functionality or scope. service/cloudfront Issues and PRs that pertain to the cloudfront service. labels Apr 8, 2018

ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 8, 2018

Ninir commented Apr 8, 2018

View reviewed changes

Ninir requested a review from bflad April 12, 2018 12:38

resource/cloudfront_distribution: Added ordered cache behaviors

1791517

Ninir force-pushed the f-cloudfront-ordered-cache-behavior branch from 1fc3865 to 1791517 Compare April 18, 2018 07:48

ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 18, 2018

bflad approved these changes Apr 18, 2018

View reviewed changes

bflad added this to the v1.15.0 milestone Apr 18, 2018

bflad merged commit da4a177 into hashicorp:master Apr 18, 2018

bflad added a commit that referenced this pull request Apr 18, 2018

Update CHANGELOG for #4117

0b66dbc

Ninir mentioned this pull request Apr 18, 2018

resource/aws_cloudfront_distribution: Add precendence for cache behaviors #1269

Closed

Ninir deleted the f-cloudfront-ordered-cache-behavior branch April 19, 2018 07:44

ghost locked and limited conversation to collaborators Apr 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/cloudfront_distribution: Added ordered cache behaviors #4117

resource/cloudfront_distribution: Added ordered cache behaviors #4117

Ninir commented Apr 8, 2018 •

edited

Loading

Ninir Apr 8, 2018

Ninir Apr 8, 2018

bflad commented Apr 17, 2018 •

edited

Loading

Ninir commented Apr 17, 2018

Ninir commented Apr 17, 2018

bflad commented Apr 17, 2018

Ninir commented Apr 18, 2018

bflad left a comment

bflad commented Apr 18, 2018

danielfm commented Apr 18, 2018 •

edited

Loading

Ninir commented Apr 18, 2018

ghost commented Apr 6, 2020

resource/cloudfront_distribution: Added ordered cache behaviors #4117

resource/cloudfront_distribution: Added ordered cache behaviors #4117

Conversation

Ninir commented Apr 8, 2018 • edited Loading

Description

Related issues

Test

Ninir Apr 8, 2018

Choose a reason for hiding this comment

Ninir Apr 8, 2018

Choose a reason for hiding this comment

bflad commented Apr 17, 2018 • edited Loading

Ninir commented Apr 17, 2018

Ninir commented Apr 17, 2018

bflad commented Apr 17, 2018

Ninir commented Apr 18, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad commented Apr 18, 2018

danielfm commented Apr 18, 2018 • edited Loading

Ninir commented Apr 18, 2018

ghost commented Apr 6, 2020

Ninir commented Apr 8, 2018 •

edited

Loading

bflad commented Apr 17, 2018 •

edited

Loading

danielfm commented Apr 18, 2018 •

edited

Loading