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.

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

add origin_request_policy_id & cache_policy_id to caching #17

Merged
merged 3 commits into from
Feb 22, 2021

Conversation

alexremn
Copy link
Contributor

Description

Recently, AWS started to offer to assigned managed policies to caching:

  • Managed-CachingOptimized
  • Managed-CachingDisabled
  • Managed-CachingOptimizedForUncompressedObjects
  • Managed-Elemental-MediaPackage

Motivation and Context

This change is considered to be like a step to easier setup of CF.

Breaking Changes

No breaking changes.

How Has This Been Tested?

Tested on creation of the new resource / update of resource created w/o managed policy.
If CDN was created w/o managed policy, it possibly would throw an error in time of apply:
The parameter ForwardedValues cannot be used when a cache policy is associated to the cache behavior.

@alexremn
Copy link
Contributor Author

Good day, @antonbabenko!
Didn't know that you have semantic check, would you like me to cancel this PR and create another with correct naming?

@antonbabenko
Copy link
Member

This semantic check is not really what we look after when we merge.

To make it pass, you can do another commit with the message like: "feat: add origin_request_policy_id & cache_policy_id to caching".

PS: Please update the minimum version of AWS provider everywhere to 3.28.0.

@alexremn
Copy link
Contributor Author

@antonbabenko sure, done.

@antonbabenko antonbabenko merged commit 2dca8c7 into terraform-aws-modules:master Feb 22, 2021
@antonbabenko
Copy link
Member

Thank you for the PR!

v1.6.0 has been just released.

@christiansaiki
Copy link
Contributor

@alexremn @antonbabenko first of all thanks for implementing the cache_policy_id and the origin_request_policy_id it really helped me to import my CloudFront distribution resources.

But I was wondering that if I have cache_policy_id shouldn't the forwarded_values be omitted?
Because after I imported the resource and ran terraform plan I keep getting the default values of query_string and cookies which I don't want.

default cache behavior config

  default_cache_behavior = {
    cache_policy_id = "4135ea2d-6df8-44a3-9df3-4b5a84be39ad"
    origin_request_policy_id = "216adef6-5c7f-47e4-b989-5492eafa07d3"
    target_origin_id = "ELB-p-elb"
    viewer_protocol_policy = "redirect-to-https"

    allowed_methods = [
      "GET",
      "HEAD",
      "DELETE",
      "OPTIONS",
      "PATCH",
      "POST",
      "PUT"
    ]
    cached_methods = [
      "GET",
      "HEAD"
    ]
    compress = true
  }

Terraform stdout

  # aws_cloudfront_distribution.this[0] will be updated in-place
  ~ resource "aws_cloudfront_distribution" "this" {
        id                             = "ETXNOC3G8ODTM"
        tags                           = {}
        # (16 unchanged attributes hidden)

      ~ default_cache_behavior {
            # (12 unchanged attributes hidden)

          + forwarded_values {
              + headers                 = (known after apply)
              + query_string            = false
              + query_string_cache_keys = []

              + cookies {
                  + forward           = "none"
                  + whitelisted_names = (known after apply)
                }
            }
        }

@antonbabenko
Copy link
Member

This seems like a known issue in Terraform AWS provider - hashicorp/terraform-provider-aws#17626

@alexremn
Copy link
Contributor Author

@christiansaiki thank you for this comment. When you would create the CloudFront with cached policy id specified, it would have more weight, then other cache configs and would be prioritized. So, anyway, there possibly would be default values for query_string and cookies .
@antonbabenko thank you! Just searched for this bug. ;)

@christiansaiki
Copy link
Contributor

Ah alright! Thanks, @antonbabenko @alexremn

@github-actions
Copy link

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants