New Resource: aws_wafregional_xss_match_set #1014

DennyLoko · 2017-06-29T17:33:18Z

This PR is a port of the originally opened by @yusukegoto at hashicorp/terraform#13709.

radeksimko

Thanks for the PR, I left you some comments there in a very similar fashion to #1013

I will leave your other WAF regional PRs for now until these two are fixed, but I reckon the same issues will need addressing there too, once we get there.

Let me know if you have any questions.

radeksimko · 2017-07-18T17:07:09Z

aws/resource_aws_wafregional_xss_match_set.go

+				Required: true,
+				ForceNew: true,
+			},
+			"xss_match_tuples": &schema.Schema{


As we discussed in the other resource do you mind renaming this field to singular? i.e. xss_match_tuple?

radeksimko · 2017-07-18T17:08:14Z

aws/resource_aws_wafregional_xss_match_set.go

+	resp, err := conn.GetXssMatchSet(params)
+	if err != nil {
+		if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == "WAFNonexistentItemException" {
+			log.Printf("[WARN] WAF IPSet (%s) not found, error code (404)", d.Id())


Typo here - this is XSS Match Set, not IPSet.

radeksimko · 2017-07-18T17:08:38Z

aws/resource_aws_wafregional_xss_match_set.go

+	}
+
+	d.Set("name", resp.XssMatchSet.Name)
+


I think we're missing some fields here, specifically xss_match_tuples and all nested fields under it. The expectation from the Terraform user is that for any resource Terraform will detect drifts from the configuration. In order to do that we need to set all the available data from the API via d.Set() here in Read func.

radeksimko · 2017-07-18T17:09:52Z

aws/resource_aws_wafregional_xss_match_set.go

+
+func resourceAwsWafRegionalXssMatchSetUpdate(d *schema.ResourceData, meta interface{}) error {
+	log.Printf("[INFO] Updating XssMatchSet: %s", d.Get("name").(string))
+	err := updateXssMatchSetResourceWR(d, meta, waf.ChangeActionInsert)


I think this isn't right, because during update we may be both inserting and deleting tuples, not just inserting. See 99e75ad for more context.

radeksimko · 2017-07-18T17:10:45Z

aws/resource_aws_wafregional_xss_match_set_test.go

+		},
+	})
+}
+


Because there has been a bug in the past affecting all WAF resources I'd like to see 2 more tests here, specifically with no tuples and another one changing tuples, see https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_waf_xss_match_set_test.go#L111-L197 for inspiration and 99e75ad for context (bugfix).

radeksimko · 2017-07-18T17:12:20Z

aws/resource_aws_wafregional_xss_match_set_test.go

+
+func testAccCheckAWSWafRegionalXssMatchSetDestroy(s *terraform.State) error {
+	for _, rs := range s.RootModule().Resources {
+		if rs.Type != "aws_wafregional_byte_match_set" {


Typo here 👀

radeksimko · 2017-07-18T17:12:50Z

website/docs/r/wafregional_xss_match_set.html.markdown

+The following arguments are supported:
+
+* `name` - (Required) The name or description of the SizeConstraintSet.
+* `xss_match_tuples` - The parts of web requests that you want to inspect for cross-site scripting attacks.


Do you mind documenting all the nested fields here, too?

Ninir · 2017-08-30T12:18:18Z

Hi @DennyLoko

Do you think you would have time to finish this PR?
Please let us know if we can help on something :)

Thanks!

radeksimko · 2017-10-05T14:04:54Z

FYI - As this PR has been stale for a couple of months I will take it over in coming week(s) unless you tell me not to.

Implemented feedback

radeksimko · 2018-03-08T12:52:13Z

I believe I addressed all things I saw myself as blockers, I think it's worth one more review from someone with a fresh eye though.

TF_ACC=1 go test ./aws -v -run=TestAccAWSWafRegionalXssMatchSet_ -timeout 120m
=== RUN   TestAccAWSWafRegionalXssMatchSet_basic
--- PASS: TestAccAWSWafRegionalXssMatchSet_basic (40.98s)
=== RUN   TestAccAWSWafRegionalXssMatchSet_changeNameForceNew
--- PASS: TestAccAWSWafRegionalXssMatchSet_changeNameForceNew (73.21s)
=== RUN   TestAccAWSWafRegionalXssMatchSet_disappears
--- PASS: TestAccAWSWafRegionalXssMatchSet_disappears (31.97s)
=== RUN   TestAccAWSWafRegionalXssMatchSet_changeTuples
--- PASS: TestAccAWSWafRegionalXssMatchSet_changeTuples (95.47s)
=== RUN   TestAccAWSWafRegionalXssMatchSet_noTuples
--- PASS: TestAccAWSWafRegionalXssMatchSet_noTuples (32.80s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	274.465s

bflad

A few minor nits, but it looks good!

5 tests passed (all tests)
=== RUN   TestAccAWSWafRegionalXssMatchSet_noTuples
--- PASS: TestAccAWSWafRegionalXssMatchSet_noTuples (11.10s)
=== RUN   TestAccAWSWafRegionalXssMatchSet_disappears
--- PASS: TestAccAWSWafRegionalXssMatchSet_disappears (13.78s)
=== RUN   TestAccAWSWafRegionalXssMatchSet_changeTuples
--- PASS: TestAccAWSWafRegionalXssMatchSet_changeTuples (18.37s)
=== RUN   TestAccAWSWafRegionalXssMatchSet_basic
--- PASS: TestAccAWSWafRegionalXssMatchSet_basic (25.62s)
=== RUN   TestAccAWSWafRegionalXssMatchSet_changeNameForceNew
--- PASS: TestAccAWSWafRegionalXssMatchSet_changeNameForceNew (28.88s)

bflad · 2018-03-08T20:22:25Z

aws/resource_aws_wafregional_xss_match_set_test.go

+	})
+}
+
+func TestAccAWSWafRegionalXssMatchSet_noTuples(t *testing.T) {


Minor nitpick: Given the tuples attribute is optional, seems like we should swap this test's contents for the _basic one then name that one appropriately

Yeah, I agree, but I'd prefer to do the cleanup across all WAF resources afterwards as the issue is not unique to this resource or PR 😉

bflad · 2018-03-08T20:31:04Z

website/docs/r/wafregional_xss_match_set.html.markdown

+#### `xss_match_tuple`
+
+* `field_to_match` - (Required) Specifies where in a web request to look for cross-site scripting attacks.
+* `text_transformation` - (Required) Which text transformation, if any, to perform on the web request before inspecting the request for cross-site scripting attacks.


Minor nitpick: we should probably point to the (rather complex) documentation here.

Agreed, but I'd prefer to do the cleanup across all WAF resources afterwards as the issue is not unique to this resource or PR 😉

bflad · 2018-03-08T20:31:59Z

website/docs/r/wafregional_xss_match_set.html.markdown

+
+The following arguments are supported:
+
+* `name` - (Required) The name or description of the SizeConstraintSet.


Copypasta 🍝 : SizeConstraintSet

bflad · 2018-03-23T21:03:48Z

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

ghost · 2020-04-07T17:22:55Z

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!

radeksimko added the new-resource Introduces a new resource. label Jul 3, 2017

DennyLoko force-pushed the aws_wafregional_xss_match_set branch from 5bbeb20 to f25ea30 Compare July 4, 2017 17:14

radeksimko previously requested changes Jul 18, 2017

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Jul 18, 2017

This was referenced Jul 18, 2017

New Resource: aws_wafregional_rule #1045

Closed

New Resource: aws_wafregional_web_acl #1046

Closed

New Resource: aws_wafregional_web_acl_association #1047

Closed

radeksimko mentioned this pull request Oct 5, 2017

New Resource: aws_wafregional_rule #1753

Closed

apparentlymart mentioned this pull request Oct 31, 2017

[WIP] provider/aws: AWS WAF Regional XssMatchSet support hashicorp/terraform#13709

Closed

radeksimko added the size/L Managed by automation to categorize the size of a PR. label Nov 15, 2017

radeksimko added the service/waf Issues and PRs that pertain to the waf service. label Jan 12, 2018

radeksimko changed the title ~~Add support for aws_wafregional_xss_match_set~~ New Resource: aws_wafregional_xss_match_set Jan 16, 2018

radeksimko added this to the v1.12.0 milestone Jan 16, 2018

giuliocalzolari mentioned this pull request Jan 22, 2018

AWS WAF regional support #1333

Closed

Add support for aws_wafregional_xss_match_set

4cd7cab

radeksimko force-pushed the aws_wafregional_xss_match_set branch from f25ea30 to 60f3ff6 Compare March 8, 2018 12:46

ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Mar 8, 2018

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Mar 8, 2018

radeksimko force-pushed the aws_wafregional_xss_match_set branch from 60f3ff6 to 402d66d Compare March 8, 2018 12:49

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

radeksimko force-pushed the aws_wafregional_xss_match_set branch from 402d66d to 4e0cc78 Compare March 8, 2018 12:49

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

radeksimko requested a review from bflad March 8, 2018 12:50

bflad approved these changes Mar 9, 2018

View reviewed changes

Implement feedback and clean up code

c8c384d

radeksimko force-pushed the aws_wafregional_xss_match_set branch from 4e0cc78 to c8c384d Compare March 12, 2018 13:46

ghost added the size/XL Managed by automation to categorize the size of a PR. label Mar 12, 2018

radeksimko merged commit 8f77c3c into hashicorp:master Mar 12, 2018

ghost locked and limited conversation to collaborators Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: aws_wafregional_xss_match_set #1014

New Resource: aws_wafregional_xss_match_set #1014

DennyLoko commented Jun 29, 2017

radeksimko left a comment

radeksimko Jul 18, 2017

radeksimko Jul 18, 2017

radeksimko Jul 18, 2017

radeksimko Jul 18, 2017

radeksimko Jul 18, 2017

radeksimko Jul 18, 2017

radeksimko Jul 18, 2017

Ninir commented Aug 30, 2017

radeksimko commented Oct 5, 2017

radeksimko commented Mar 8, 2018 •

edited

Loading

bflad left a comment

bflad Mar 8, 2018

radeksimko Mar 12, 2018

bflad Mar 8, 2018

radeksimko Mar 12, 2018

bflad Mar 8, 2018

bflad commented Mar 23, 2018

ghost commented Apr 7, 2020


		The following arguments are supported:

		* `name` - (Required) The name or description of the SizeConstraintSet.

New Resource: aws_wafregional_xss_match_set #1014

New Resource: aws_wafregional_xss_match_set #1014

Conversation

DennyLoko commented Jun 29, 2017

radeksimko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ninir commented Aug 30, 2017

radeksimko commented Oct 5, 2017

radeksimko commented Mar 8, 2018 • edited Loading

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Mar 23, 2018

ghost commented Apr 7, 2020

radeksimko commented Mar 8, 2018 •

edited

Loading