CloudFront Origin Request Policy #17342
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
size/XXL
github-actions
bot
added
the
needs-triage
bill-rich
removed
the
needs-triage
YakDriver
requested changes
Jan 29, 2021
Comment on lines
14
to
104
| "name": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| }, | ||
| "id": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| }, | ||
| "comment": { | ||
| Type: schema.TypeString, | ||
| Computed: true, | ||
| }, | ||
| "etag": { | ||
| Type: schema.TypeString, | ||
| Computed: true, | ||
| }, | ||
| "cookies_config": { | ||
| Type: schema.TypeList, | ||
| Computed: true, | ||
| Elem: &schema.Resource{ | ||
| Schema: map[string]*schema.Schema{ | ||
| "cookie_behavior": { | ||
| Computed: true, | ||
| Type: schema.TypeString, | ||
| }, | ||
| "cookies": { | ||
| Type: schema.TypeList, | ||
| Computed: true, | ||
| Elem: &schema.Resource{ | ||
| Schema: map[string]*schema.Schema{ | ||
| "items": { | ||
| Type: schema.TypeSet, | ||
| Computed: true, | ||
| Elem: &schema.Schema{Type: schema.TypeString}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| "headers_config": { | ||
| Type: schema.TypeList, | ||
| Computed: true, | ||
| Elem: &schema.Resource{ | ||
| Schema: map[string]*schema.Schema{ | ||
| "header_behavior": { | ||
| Computed: true, | ||
| Type: schema.TypeString, | ||
| }, | ||
| "headers": { | ||
| Type: schema.TypeList, | ||
| Computed: true, | ||
| Elem: &schema.Resource{ | ||
| Schema: map[string]*schema.Schema{ | ||
| "items": { | ||
| Type: schema.TypeSet, | ||
| Computed: true, | ||
| Elem: &schema.Schema{Type: schema.TypeString}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| "query_strings_config": { | ||
| Type: schema.TypeList, | ||
| Computed: true, | ||
| Elem: &schema.Resource{ | ||
| Schema: map[string]*schema.Schema{ | ||
| "query_string_behavior": { | ||
| Type: schema.TypeString, | ||
| Computed: true, | ||
| }, | ||
| "query_strings": { | ||
| Type: schema.TypeList, | ||
| Computed: true, | ||
| Elem: &schema.Resource{ | ||
| Schema: map[string]*schema.Schema{ | ||
| "items": { | ||
| Type: schema.TypeSet, | ||
| Computed: true, | ||
| Elem: &schema.Schema{Type: schema.TypeString}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Suggested change
| "name": { | |
| Type: schema.TypeString, | |
| Optional: true, | |
| }, | |
| "id": { | |
| Type: schema.TypeString, | |
| Optional: true, | |
| }, | |
| "comment": { | |
| Type: schema.TypeString, | |
| Computed: true, | |
| }, | |
| "etag": { | |
| Type: schema.TypeString, | |
| Computed: true, | |
| }, | |
| "cookies_config": { | |
| Type: schema.TypeList, | |
| Computed: true, | |
| Elem: &schema.Resource{ | |
| Schema: map[string]*schema.Schema{ | |
| "cookie_behavior": { | |
| Computed: true, | |
| Type: schema.TypeString, | |
| }, | |
| "cookies": { | |
| Type: schema.TypeList, | |
| Computed: true, | |
| Elem: &schema.Resource{ | |
| Schema: map[string]*schema.Schema{ | |
| "items": { | |
| Type: schema.TypeSet, | |
| Computed: true, | |
| Elem: &schema.Schema{Type: schema.TypeString}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| "headers_config": { | |
| Type: schema.TypeList, | |
| Computed: true, | |
| Elem: &schema.Resource{ | |
| Schema: map[string]*schema.Schema{ | |
| "header_behavior": { | |
| Computed: true, | |
| Type: schema.TypeString, | |
| }, | |
| "headers": { | |
| Type: schema.TypeList, | |
| Computed: true, | |
| Elem: &schema.Resource{ | |
| Schema: map[string]*schema.Schema{ | |
| "items": { | |
| Type: schema.TypeSet, | |
| Computed: true, | |
| Elem: &schema.Schema{Type: schema.TypeString}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| "query_strings_config": { | |
| Type: schema.TypeList, | |
| Computed: true, | |
| Elem: &schema.Resource{ | |
| Schema: map[string]*schema.Schema{ | |
| "query_string_behavior": { | |
| Type: schema.TypeString, | |
| Computed: true, | |
| }, | |
| "query_strings": { | |
| Type: schema.TypeList, | |
| Computed: true, | |
| Elem: &schema.Resource{ | |
| Schema: map[string]*schema.Schema{ | |
| "items": { | |
| Type: schema.TypeSet, | |
| Computed: true, | |
| Elem: &schema.Schema{Type: schema.TypeString}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| "comment": { | |
| Type: schema.TypeString, | |
| Computed: true, | |
| }, | |
| "cookies_config": { | |
| Type: schema.TypeList, | |
| Computed: true, | |
| Elem: &schema.Resource{ | |
| Schema: map[string]*schema.Schema{ | |
| "cookie_behavior": { | |
| Computed: true, | |
| Type: schema.TypeString, | |
| }, | |
| "cookies": { | |
| Type: schema.TypeList, | |
| Computed: true, | |
| Elem: &schema.Resource{ | |
| Schema: map[string]*schema.Schema{ | |
| "items": { | |
| Type: schema.TypeSet, | |
| Computed: true, | |
| Elem: &schema.Schema{Type: schema.TypeString}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| "etag": { | |
| Type: schema.TypeString, | |
| Computed: true, | |
| }, | |
| "headers_config": { | |
| Type: schema.TypeList, | |
| Computed: true, | |
| Elem: &schema.Resource{ | |
| Schema: map[string]*schema.Schema{ | |
| "header_behavior": { | |
| Computed: true, | |
| Type: schema.TypeString, | |
| }, | |
| "headers": { | |
| Type: schema.TypeList, | |
| Computed: true, | |
| Elem: &schema.Resource{ | |
| Schema: map[string]*schema.Schema{ | |
| "items": { | |
| Type: schema.TypeSet, | |
| Computed: true, | |
| Elem: &schema.Schema{Type: schema.TypeString}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| "id": { | |
| Type: schema.TypeString, | |
| Optional: true, | |
| }, | |
| "name": { | |
| Type: schema.TypeString, | |
| Optional: true, | |
| }, | |
| "query_strings_config": { | |
| Type: schema.TypeList, | |
| Computed: true, | |
| Elem: &schema.Resource{ | |
| Schema: map[string]*schema.Schema{ | |
| "query_string_behavior": { | |
| Type: schema.TypeString, | |
| Computed: true, | |
| }, | |
| "query_strings": { | |
| Type: schema.TypeList, | |
| Computed: true, | |
| Elem: &schema.Resource{ | |
| Schema: map[string]*schema.Schema{ | |
| "items": { | |
| Type: schema.TypeSet, | |
| Computed: true, | |
| Elem: &schema.Schema{Type: schema.TypeString}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, |
Nit: Alphabetizing the arguments helps with quickly finding them in the future.
| } | ||
| } | ||
|
|
||
| func flattenCloudFrontOriginRequestPolicy(d *schema.ResourceData, originRequestPolicy *cloudfront.OriginRequestPolicyConfig) { |
There was a problem hiding this comment.
As this is not flattening, I would not preface this function name with flatten.
However, more importantly, it's a bad idea to set these values in a separate function. It creates an extra burden later trying to figure out what the read() is doing. Even though it's redundant, these 5 d.Set() calls should be moved to the data source and resource read functions.
Comment on lines
9
to
20
| func expandCloudFrontOriginRequestPolicyCookieNames(cookieNamesFlat map[string]interface{}) *cloudfront.CookieNames { | ||
| cookieNames := &cloudfront.CookieNames{} | ||
|
|
||
| var newCookieItems []*string | ||
| for _, cookie := range cookieNamesFlat["items"].(*schema.Set).List() { | ||
| newCookieItems = append(newCookieItems, aws.String(cookie.(string))) | ||
| } | ||
| cookieNames.Items = newCookieItems | ||
| cookieNames.Quantity = aws.Int64(int64(len(newCookieItems))) | ||
|
|
||
| return cookieNames | ||
| } |
There was a problem hiding this comment.
Suggested change
| func expandCloudFrontOriginRequestPolicyCookieNames(cookieNamesFlat map[string]interface{}) *cloudfront.CookieNames { | |
| cookieNames := &cloudfront.CookieNames{} | |
| var newCookieItems []*string | |
| for _, cookie := range cookieNamesFlat["items"].(*schema.Set).List() { | |
| newCookieItems = append(newCookieItems, aws.String(cookie.(string))) | |
| } | |
| cookieNames.Items = newCookieItems | |
| cookieNames.Quantity = aws.Int64(int64(len(newCookieItems))) | |
| return cookieNames | |
| } | |
| func expandCloudFrontOriginRequestPolicyCookieNames(tfMap map[string]interface{}) *cloudfront.CookieNames { | |
| if tfMap == nil { | |
| return ni | |
| } | |
| apiObject := &cloudfront.CookieNames{} | |
| var items []*string | |
| for _, item := range tfMap["items"].(*schema.Set).List() { | |
| items = append(items, aws.String(item.(string))) | |
| } | |
| apiObject.Items = items | |
| apiObject.Quantity = aws.Int64(int64(len(items))) | |
| return apiObject | |
| } |
- Include a safety
nilcheck. If Go hits the type cast on line 13 old/17 new andcookieNamesFlatisnil, it will panic. Even though with your current setup,nilwould not be possible, in the future if things change, it protects us. Sinceexpandfuncs should usually have the check, it's more likely someone would assume it's there. - Expand and flatten funcs are exceptions to the descriptive variables names rule. Keeping the names generic helps future contributors to quickly mentally process flatteners/expanders.
These changes should be made to all 10 of the expanders/flatteners. See https://github.com/hashicorp/terraform-provider-aws/blob/master/docs/contributing/data-handling-and-conversion.md#expand-functions-for-blocks for more.
| } | ||
| } | ||
|
|
||
| func dataSourceAwsCloudFrontOriginRequestPolicyFindByName(d *schema.ResourceData, conn *cloudfront.CloudFront) error { |
There was a problem hiding this comment.
Any non-CRUD func should come after the CRUD funcs.
| }, | ||
|
|
||
| Schema: map[string]*schema.Schema{ | ||
| "comment": { |
There was a problem hiding this comment.
As with the data source, these args should be alphebetized.
| ### Cookies Config | ||
|
|
||
| `cookie_behavior` - Determines whether any cookies in viewer requests are included in the origin request key and automatically included in requests that CloudFront sends to the origin. Valid values are `none`, `whitelist` `all`. | ||
| `cookies` - An object that contains a list of cookie names. See [Items](#items) for more information. |
There was a problem hiding this comment.
Suggested change
| `cookies` - An object that contains a list of cookie names. See [Items](#items) for more information. | |
| `cookies` - Object that contains a list of cookie names. See [Items](#items) for more information. |
| * `cookies_config` - An object that determines whether any cookies in viewer requests (and if so, which cookies) are included in the origin request key and automatically included in requests that CloudFront sends to the origin. See [Cookies Config](#cookies-config) for more information. | ||
| * `etag` - The current version of the origin request policy. | ||
| * `headers_config` - An object that determines whether any HTTP headers (and if so, which headers) are included in the origin request key and automatically included in requests that CloudFront sends to the origin. See [Headers Config](#headers-config) for more information. | ||
| * `query_strings_config` - An object that determines whether any URL query strings in viewer requests (and if so, which query strings) are included in the origin request key and automatically included in requests that CloudFront sends to the origin. See [Query Strings Config](#query-strings-config) for more information. |
There was a problem hiding this comment.
Suggested change
| * `query_strings_config` - An object that determines whether any URL query strings in viewer requests (and if so, which query strings) are included in the origin request key and automatically included in requests that CloudFront sends to the origin. See [Query Strings Config](#query-strings-config) for more information. | |
| * `query_strings_config` - Object that determines whether any URL query strings in viewer requests (and if so, which query strings) are included in the origin request key and automatically included in requests that CloudFront sends to the origin. See [Query Strings Config](#query-strings-config) for more information. |
| * `comment` - A comment to describe the origin request policy. | ||
| * `cookies_config` - An object that determines whether any cookies in viewer requests (and if so, which cookies) are included in the origin request key and automatically included in requests that CloudFront sends to the origin. See [Cookies Config](#cookies-config) for more information. | ||
| * `etag` - The current version of the origin request policy. | ||
| * `headers_config` - An object that determines whether any HTTP headers (and if so, which headers) are included in the origin request key and automatically included in requests that CloudFront sends to the origin. See [Headers Config](#headers-config) for more information. |
There was a problem hiding this comment.
Suggested change
| * `headers_config` - An object that determines whether any HTTP headers (and if so, which headers) are included in the origin request key and automatically included in requests that CloudFront sends to the origin. See [Headers Config](#headers-config) for more information. | |
| * `headers_config` - Object that determines whether any HTTP headers (and if so, which headers) are included in the origin request key and automatically included in requests that CloudFront sends to the origin. See [Headers Config](#headers-config) for more information. |
| ## Attributes Reference | ||
|
|
||
| * `comment` - A comment to describe the origin request policy. | ||
| * `cookies_config` - An object that determines whether any cookies in viewer requests (and if so, which cookies) are included in the origin request key and automatically included in requests that CloudFront sends to the origin. See [Cookies Config](#cookies-config) for more information. |
There was a problem hiding this comment.
Suggested change
| * `cookies_config` - An object that determines whether any cookies in viewer requests (and if so, which cookies) are included in the origin request key and automatically included in requests that CloudFront sends to the origin. See [Cookies Config](#cookies-config) for more information. | |
| * `cookies_config` - Object that determines whether any cookies in viewer requests (and if so, which cookies) are included in the origin request key and automatically included in requests that CloudFront sends to the origin. See [Cookies Config](#cookies-config) for more information. |
|
|
||
| ## Attributes Reference | ||
|
|
||
| * `comment` - A comment to describe the origin request policy. |
There was a problem hiding this comment.
Suggested change
| * `comment` - A comment to describe the origin request policy. | |
| * `comment` - Comment to describe the origin request policy. |
YakDriver
approved these changes
Feb 4, 2021
There was a problem hiding this comment.
This is looking good. Few minors things left (especially d.IsNewResource()) and it's good to go!
GovCloud:
--- SKIP: TestAccAWSCloudFrontOriginRequestPolicy_basic (1.16s)
--- SKIP: TestAccAWSCloudFrontOriginRequestPolicy_noneBehavior (1.16s)
--- SKIP: TestAccAWSCloudFrontOriginRequestPolicy_update (1.16s)
us-west-2:
--- PASS: TestAccAWSCloudFrontOriginRequestPolicy_basic (10.76s)
--- PASS: TestAccAWSCloudFrontOriginRequestPolicy_noneBehavior (10.77s)
--- PASS: TestAccAWSCloudFrontOriginRequestPolicy_update (17.53s)
|
|
||
| if d.Get("id").(string) == "" { | ||
| if err := dataSourceAwsCloudFrontOriginRequestPolicyFindByName(d, conn); err != nil { | ||
| return fmt.Errorf("Unable to find origin request policy by name: %s", err.Error()) |
There was a problem hiding this comment.
Suggested change
| return fmt.Errorf("Unable to find origin request policy by name: %s", err.Error()) | |
| return fmt.Errorf("unable to find origin request policy by name: %s", err.Error()) |
|
|
||
| resp, err := conn.GetOriginRequestPolicy(request) | ||
| if err != nil { | ||
| return fmt.Errorf("Unable to retrieve origin request policy with ID %s: %s", d.Id(), err.Error()) |
There was a problem hiding this comment.
Suggested change
| return fmt.Errorf("Unable to retrieve origin request policy with ID %s: %s", d.Id(), err.Error()) | |
| return fmt.Errorf("unable to retrieve origin request policy with ID %s: %s", d.Id(), err.Error()) |
|
This has been released in version 3.27.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks! |
oleg-codaio
reviewed
Feb 14, 2021
| @@ -151,6 +151,10 @@ func resourceAwsCloudFrontDistribution() *schema.Resource { | |||
| Optional: true, | |||
| Default: 0, | |||
| }, | |||
| "origin_request_policy_id": { | |||
There was a problem hiding this comment.
It seems that this new parameter was never added to the documentation.
|
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! |
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Community Note
Relates #14373
Output from acceptance testing: