Allow use of resource_type and resource_type_list #17418

bill-rich · 2021-02-03T00:17:59Z

AWS::CloudFront::Distribution is only allowed in resource_type. Originally fms_policy put all resource types into resource_type_list. This PR allows the use of both.

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Output from acceptance testing:

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSFmsPolicy_ -timeout 120m
=== RUN   TestAccAWSFmsPolicy_basic
=== PAUSE TestAccAWSFmsPolicy_basic
=== RUN   TestAccAWSFmsPolicy_cloudfrontDistribution
=== PAUSE TestAccAWSFmsPolicy_cloudfrontDistribution
=== RUN   TestAccAWSFmsPolicy_includeMap
=== PAUSE TestAccAWSFmsPolicy_includeMap
=== RUN   TestAccAWSFmsPolicy_update
=== PAUSE TestAccAWSFmsPolicy_update
=== RUN   TestAccAWSFmsPolicy_tags
=== PAUSE TestAccAWSFmsPolicy_tags
=== CONT  TestAccAWSFmsPolicy_basic
=== CONT  TestAccAWSFmsPolicy_update
=== CONT  TestAccAWSFmsPolicy_tags
=== CONT  TestAccAWSFmsPolicy_includeMap
=== CONT  TestAccAWSFmsPolicy_cloudfrontDistribution
--- PASS: TestAccAWSFmsPolicy_basic (31.81s)
--- PASS: TestAccAWSFmsPolicy_includeMap (33.09s)
--- PASS: TestAccAWSFmsPolicy_tags (34.23s)
--- PASS: TestAccAWSFmsPolicy_update (51.61s)
--- PASS: TestAccAWSFmsPolicy_cloudfrontDistribution (106.17s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	106.214s

bflad · 2021-02-04T19:27:46Z

aws/resource_aws_fms_policy.go

 		return err
 	}
+	if aws.StringValue(resp.Policy.ResourceType) != "ResourceTypeList" {


To avoid logic like this, we can mark the resource_type attribute as Computed as it is generally preferable to always set the value to simplify resource logic. 👍

bflad · 2021-02-04T19:37:45Z

aws/resource_aws_fms_policy.go

-	if v, ok := d.GetOk("exclude_map"); ok {
-		fmsPolicy.ExcludeMap = expandFMSPolicyMap(v.([]interface{}))
-	}
+	fmsPolicy := resourceAwsFmsPolicyExpandPolicy(d)


I think this change is breaking creation because the resource ID and PolicyUpdateToken are not yet available:

=== CONT TestAccAWSFmsPolicy_tags resource_aws_fms_policy_test.go:129: Step 1/2 error: Error running apply: Error: Creating Policy Failed: InvalidParameter: 2 validation error(s) found. - minimum field size of 36, PutPolicyInput.Policy.PolicyId. - minimum field size of 1, PutPolicyInput.Policy.PolicyUpdateToken. === CONT TestAccAWSFmsPolicy_cloudfrontDistribution resource_aws_fms_policy_test.go:47: Step 1/2 error: Error running apply: Error: Creating Policy Failed: InvalidParameter: 2 validation error(s) found. - minimum field size of 36, PutPolicyInput.Policy.PolicyId. - minimum field size of 1, PutPolicyInput.Policy.PolicyUpdateToken. === CONT TestAccAWSFmsPolicy_basic resource_aws_fms_policy_test.go:19: Step 1/2 error: Error running apply: Error: Creating Policy Failed: InvalidParameter: 2 validation error(s) found. - minimum field size of 36, PutPolicyInput.Policy.PolicyId. - minimum field size of 1, PutPolicyInput.Policy.PolicyUpdateToken. === CONT TestAccAWSFmsPolicy_includeMap resource_aws_fms_policy_test.go:75: Step 1/2 error: Error running apply: Error: Creating Policy Failed: InvalidParameter: 2 validation error(s) found. - minimum field size of 36, PutPolicyInput.Policy.PolicyId. - minimum field size of 1, PutPolicyInput.Policy.PolicyUpdateToken. --- FAIL: TestAccAWSFmsPolicy_tags (11.14s) --- FAIL: TestAccAWSFmsPolicy_cloudfrontDistribution (13.52s) === CONT TestAccAWSFmsPolicy_update resource_aws_fms_policy_test.go:104: Step 1/2 error: Error running apply: Error: Creating Policy Failed: InvalidParameter: 2 validation error(s) found. - minimum field size of 36, PutPolicyInput.Policy.PolicyId. - minimum field size of 1, PutPolicyInput.Policy.PolicyUpdateToken. --- FAIL: TestAccAWSFmsPolicy_basic (14.61s) --- FAIL: TestAccAWSFmsPolicy_includeMap (15.72s) --- FAIL: TestAccAWSFmsPolicy_update (16.84s)

bill-rich · 2021-02-11T19:29:48Z

I got this fixed up, so it should be ready for another look.

bflad

Looking better now. 👍 Just one documentation thing and missing changelog entry

Output from acceptance testing in AWS Commercial (us-east-1):

--- PASS: TestAccAWSFmsPolicy_tags (16.55s)
--- PASS: TestAccAWSFmsPolicy_includeMap (17.11s)
--- PASS: TestAccAWSFmsPolicy_basic (20.22s)
--- PASS: TestAccAWSFmsPolicy_update (28.45s)
--- PASS: TestAccAWSFmsPolicy_cloudfrontDistribution (79.45s)

We will need to setup special handling for the new CloudFormation test as it currently fails in the default us-west-2 region:

=== CONT  TestAccAWSFmsPolicy_cloudfrontDistribution
    resource_aws_fms_policy_test.go:47: Step 1/2 error: Error running apply:
        Error: Creating Policy Failed: InvalidInputException: Resource ["AWS::CloudFront::Distribution"] can not be used in region: us-west-2.

bflad · 2021-02-11T20:21:24Z

website/docs/r/fms_policy.html.markdown

@@ -59,7 +59,8 @@ The following arguments are supported:
 * `include_map` - (Optional) A map of lists, with a single key named 'account' with a list of AWS Account IDs to include for this policy.
 * `remediation_enabled` - (Required) A boolean value, indicates if the policy should automatically applied to resources that already exist in the account.
 * `resource_tags` - (Optional) A map of resource tags, that if present will filter protections on resources based on the exclude_resource_tags.
-* `resource_type_list` - (Required, Forces new resource) A list of resource types to protect, valid values are: `AWS::ElasticLoadBalancingV2::LoadBalancer`, `AWS::ApiGateway::Stage`, `AWS::CloudFront::Distribution`.
+* `resource_type` - (Optional) A resource type to protect, valid values are: `AWS::ElasticLoadBalancingV2::LoadBalancer`, `AWS::ApiGateway::Stage`, `AWS::CloudFront::Distribution`. Conflicts with `resource_type_list`.
+* `resource_type_list` - (Optional) A list of resource types to protect, valid values are: `AWS::ElasticLoadBalancingV2::LoadBalancer`, `AWS::ApiGateway::Stage`, `AWS::CloudFront::Distribution`. Conflicts with `resource_type`.


The schema validation also includes: "AWS::EC2::NetworkInterface", "AWS::EC2::Instance", "AWS::EC2::SecurityGroup"

Suggested change

* `resource_type_list` - (Optional) A list of resource types to protect, valid values are: `AWS::ElasticLoadBalancingV2::LoadBalancer`, `AWS::ApiGateway::Stage`, `AWS::CloudFront::Distribution`. Conflicts with `resource_type`.

* `resource_type_list` - (Optional) A list of resource types to protect, valid values are: `AWS::ElasticLoadBalancingV2::LoadBalancer`, `AWS::ApiGateway::Stage`, `AWS::CloudFront::Distribution`, `AWS::EC2::Instance`, `AWS::EC2::NetworkInterface`, `AWS::EC2::SecurityGroup`. Conflicts with `resource_type`.

ghost · 2021-02-12T17:28:34Z

This has been released in version 3.28.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!

ghost · 2021-03-14T17:16:54Z

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!

bill-rich requested a review from a team as a code owner February 3, 2021 00:17

github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Feb 3, 2021

bill-rich removed the needs-triage Waiting for first response or review from a maintainer. label Feb 4, 2021

bflad suggested changes Feb 4, 2021

View reviewed changes

bflad self-assigned this Feb 4, 2021

bill-rich added 2 commits February 11, 2021 09:38

Allow use of resource_type and resource_type_list

7864e61

Fix shared expand function and use WAFv2 policy

6e9f21c

bill-rich force-pushed the b_fms_distribution branch from 50cb80a to 6e9f21c Compare February 11, 2021 18:34

bflad added the enhancement Requests to existing resources that expand the functionality or scope. label Feb 11, 2021

bflad approved these changes Feb 11, 2021

View reviewed changes

Fix docs and add changelog entry

893ba66

bill-rich merged commit 22a3875 into main Feb 12, 2021

bill-rich deleted the b_fms_distribution branch February 12, 2021 00:49

github-actions bot added this to the v3.28.0 milestone Feb 12, 2021

github-actions bot pushed a commit that referenced this pull request Feb 12, 2021

Update CHANGELOG.md for #17418

9b3b583

ghost locked as resolved and limited conversation to collaborators Mar 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow use of resource_type and resource_type_list #17418

Allow use of resource_type and resource_type_list #17418

bill-rich commented Feb 3, 2021

bflad Feb 4, 2021

bflad Feb 4, 2021

bill-rich commented Feb 11, 2021

bflad left a comment

bflad Feb 11, 2021

ghost commented Feb 12, 2021

ghost commented Mar 14, 2021

	* `resource_type_list` - (Optional) A list of resource types to protect, valid values are: `AWS::ElasticLoadBalancingV2::LoadBalancer`, `AWS::ApiGateway::Stage`, `AWS::CloudFront::Distribution`. Conflicts with `resource_type`.
	* `resource_type_list` - (Optional) A list of resource types to protect, valid values are: `AWS::ElasticLoadBalancingV2::LoadBalancer`, `AWS::ApiGateway::Stage`, `AWS::CloudFront::Distribution`, `AWS::EC2::Instance`, `AWS::EC2::NetworkInterface`, `AWS::EC2::SecurityGroup`. Conflicts with `resource_type`.

Allow use of resource_type and resource_type_list #17418

Allow use of resource_type and resource_type_list #17418

Conversation

bill-rich commented Feb 3, 2021

Community Note

bflad Feb 4, 2021

Choose a reason for hiding this comment

bflad Feb 4, 2021

Choose a reason for hiding this comment

bill-rich commented Feb 11, 2021

bflad left a comment

Choose a reason for hiding this comment

bflad Feb 11, 2021

Choose a reason for hiding this comment

ghost commented Feb 12, 2021

ghost commented Mar 14, 2021