resource/aws_wafregional_size_constraint_set #3796

appilon · 2018-03-15T15:44:16Z

This implements size constraints for WAF regional. Full disclosure, I tried my hardest to understand the API/how things worked, but this is mostly copy paste of size constraint set from global WAF. As a result this PR doesn't try and change anything really between the resources (regional vs global).

I did notice there are some validations missing from the schema (ranges, max sizes, enum's of allowable strings), that could be present. I've refactored the schema to a common helper and could add the validations but does that require more tests/docs that I need to replicate to waf and wafregional? Let me know and I am happy to implement them but it does lend to a larger discussion below.

The following is a discussion/commentary on the overall waf/wafregional codebase:

IMO the best course of action is to merge the less "robust" PR like so (implementing any feedback of course), but make note that WAF and WAF regional are very much 1 API with different client conns and could use some harmonizing. There is code that is shareable, for instance the docs is complete copypaste since the internal types/rules are the same between waf and wafregional. I suspect we could almost half the LOCs and account for the region/vs global differences with a special flag/conditional code, but this would make the 2 sets of resources completely coupled.

On the other hand if Amazon changes/evolves the service we would have to rip everything apart again. Something tells me that when it comes to provider code it's better to have copypaste/duplication for the sake of not modelling something that we don't control (and could break later). Feedback from more experienced developers on this would be greatly appreciated.... sorry for long write up

Tests:

TF_ACC=1 go test ./aws -v -run=TestAccAWSWafRegionalSizeConstraintSet -timeout 120m
=== RUN   TestAccAWSWafRegionalSizeConstraintSet_basic
--- PASS: TestAccAWSWafRegionalSizeConstraintSet_basic (26.65s)
=== RUN   TestAccAWSWafRegionalSizeConstraintSet_changeNameForceNew
--- PASS: TestAccAWSWafRegionalSizeConstraintSet_changeNameForceNew (52.43s)
=== RUN   TestAccAWSWafRegionalSizeConstraintSet_disappears
--- PASS: TestAccAWSWafRegionalSizeConstraintSet_disappears (21.33s)
=== RUN   TestAccAWSWafRegionalSizeConstraintSet_changeConstraints
--- PASS: TestAccAWSWafRegionalSizeConstraintSet_changeConstraints (39.25s)
=== RUN   TestAccAWSWafRegionalSizeConstraintSet_noConstraints
--- PASS: TestAccAWSWafRegionalSizeConstraintSet_noConstraints (18.64s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	158.363s

AWS_PROFILE=personal make testacc TEST=./aws TESTARGS='-run=TestAccAWSWafSizeConstraintSet'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSWafSizeConstraintSet -timeout 120m
=== RUN   TestAccAWSWafSizeConstraintSet_basic
--- PASS: TestAccAWSWafSizeConstraintSet_basic (21.21s)
=== RUN   TestAccAWSWafSizeConstraintSet_changeNameForceNew
--- PASS: TestAccAWSWafSizeConstraintSet_changeNameForceNew (32.32s)
=== RUN   TestAccAWSWafSizeConstraintSet_disappears
--- PASS: TestAccAWSWafSizeConstraintSet_disappears (18.54s)
=== RUN   TestAccAWSWafSizeConstraintSet_changeConstraints
--- PASS: TestAccAWSWafSizeConstraintSet_changeConstraints (29.28s)
=== RUN   TestAccAWSWafSizeConstraintSet_noConstraints
--- PASS: TestAccAWSWafSizeConstraintSet_noConstraints (15.51s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	116.898s

radeksimko

Thanks for the PR, this overall looks good, just a few comments.
👍 for decoupling all the helper functions for diffing etc.

I have mixed feelings about decoupling the schema though. It's not that many LOC and the downside of moving it away from the CRUD is that it becomes much more difficult to build the mind-map of relationships/context between schema and CRUD.

Sometimes duplication is more than abstraction 😉
It's not a big deal though - certainly not a blocker.

radeksimko · 2018-03-20T08:16:41Z

aws/resource_aws_wafregional_size_constraint_set.go

+
+	name := d.Get("name").(string)
+
+	log.Printf("[INFO] Creating SizeConstraintSet: %s", name)


Nitpick: All of the log messages should probably say "WAF Regional Size Constraint Set" to prevent mismatching those with logs from the global resource.

radeksimko · 2018-03-20T08:17:30Z

aws/resource_aws_wafregional_size_constraint_set.go

+
+	resp, err := conn.GetSizeConstraintSet(params)
+	if err != nil {
+		if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == "WAFNonexistentItemException" {


Nitpick: Do you mind replacing this logic with isAWSErr(err, wafregional.ErrCodeWAFNonexistentItemException, "")?

radeksimko · 2018-03-20T08:21:16Z

aws/resource_aws_wafregional_size_constraint_set.go

+		oldS, newS := o.(*schema.Set).List(), n.(*schema.Set).List()
+
+		if err := updateRegionalSizeConstraintSetResource(d.Id(), oldS, newS, client.wafregionalconn, client.region); err != nil {
+			return errwrap.Wrapf("[ERROR] Error updating SizeConstraintSet: {{err}}", err)


Nitpick: there's no value in using errwrap. I know it's sprinkled all around the codebase atm, but in most cases for no particular reason.
The goal of the library is to allow you to wrap errors so you can later check the original error type. In most cases in AWS provider though - and it includes this snippet of code - there is no "later" when we'd ever care about the original error type, because the error is just returned from CRUD and bubbles up to the UI/CLI. In the UI user only ever cares about strings, not error types.

TL;DR fmt.Errorf would do the job equally well here.

radeksimko · 2018-03-20T10:40:56Z

aws/resource_aws_wafregional_size_constraint_set.go

+
+	if d.HasChange("size_constraints") {
+		o, n := d.GetChange("size_constraints")
+		oldS, newS := o.(*schema.Set).List(), n.(*schema.Set).List()


Nitpick: What's the idea behind oldS - what does "S" mean in this context? Can't we call it e.g. oldConstraints, newConstraints?

this was copypasta I will change it in waf global as well

radeksimko · 2018-03-20T10:42:41Z

aws/resource_aws_wafregional_size_constraint_set_test.go

+}
+
+func TestAccAWSWafRegionalSizeConstraintSet_noConstraints(t *testing.T) {
+	var ipset waf.SizeConstraintSet


Copy-pasta error? 👀 🙂

😬 yes... again it is present in waf global also, will fix both?

radeksimko · 2018-03-20T10:44:02Z

aws/resource_aws_wafregional_size_constraint_set_test.go

+}
+
+func TestAccAWSWafRegionalSizeConstraintSet_disappears(t *testing.T) {
+	var v waf.SizeConstraintSet


Nitpick: What did you mean by v as the name of the argument here? Can't we give it more descriptive name?

radeksimko · 2018-03-20T10:44:45Z

aws/resource_aws_wafregional_size_constraint_set_test.go

+	}
+}
+
+func testAccCheckAWSWafRegionalSizeConstraintSetExists(n string, v *waf.SizeConstraintSet) resource.TestCheckFunc {


Nitpick: What did you mean by v as the name of the argument here? Can't we give it more descriptive name?

radeksimko · 2018-03-20T10:45:10Z

aws/resource_aws_wafregional_size_constraint_set_test.go

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


Copy-pasta error? 👀

I'm not sure.... again was present in waf global

radeksimko · 2018-03-20T10:45:49Z

aws/resource_aws_wafregional_size_constraint_set_test.go

+
+		// Return nil if the SizeConstraintSet is already destroyed
+		if awsErr, ok := err.(awserr.Error); ok {
+			if awsErr.Code() == "WAFNonexistentItemException" {


Do you mind replacing this with the helper function? i.e.

isAWSErr(err, wafregional.ErrCodeWAFNonexistentItemException, "")

radeksimko · 2018-03-20T10:46:44Z

website/docs/r/wafregional_size_constraint_set.html.markdown

+  See [docs](http://docs.aws.amazon.com/waf/latest/APIReference/API_FieldToMatch.html)
+  for all supported values.
+
+## Remarks


Extra header here ^ 👓

begin moving helpers/common code to helper file

appilon · 2018-03-21T20:14:56Z

@radeksimko I addressed all the comments. If it's fine with you I would leave the schema in the helper, if a need arises to separate them again I think it should be trivial to do so, unless you feel strongly otherwise (and I do see your point!)

radeksimko

LGTM. Feel free to merge once the two comments in tests are addressed (and acceptance tests are still passing). 😉

radeksimko · 2018-03-22T07:49:16Z

aws/resource_aws_waf_size_constraint_set_test.go

@@ -258,9 +258,6 @@ func testAccCheckAWSWafSizeConstraintSetExists(n string, v *waf.SizeConstraintSe

 func testAccCheckAWSWafSizeConstraintSetDestroy(s *terraform.State) error {
 	for _, rs := range s.RootModule().Resources {
-		if rs.Type != "aws_waf_byte_match_set" {


So the conditional should IMO stay here, it should be just changed to if rs.Type != "aws_waf_size_constraint_set" {. Otherwise this would cause confusing errors if there was more than 1 resource in the state after deletion and we'd use the ID of that resource for looking up Size Constraint Set (even though it may be a completely different resource).

radeksimko · 2018-03-22T08:39:26Z

aws/resource_aws_wafregional_size_constraint_set_test.go

+
+func testAccCheckAWSWafRegionalSizeConstraintSetDestroy(s *terraform.State) error {
+	for _, rs := range s.RootModule().Resources {
+


As mentioned above we should keep the conditional in place, my comment was more about the resource name we were checking here. Sorry for not making it clear.

radeksimko · 2018-03-22T08:43:53Z

aws/waf_helpers.go

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


Somehow I didn't notice this in my previous code review, but we tend to use singular names for TypeList/TypeSet fields with non-primitive nested types. This is because such fields are commonly represented as

size_constraint { ... } size_constraint { ... } size_constraint { ... }

unlike TypeList/TypeSet with primitive (e.g. TypeString) field:

availability_zones = ["us-east-1a", "us-east-1b"]

However I'm 👌 with keeping it as is for this initial implementation. We can deprecate later - which we'll need to do anyway for some other WAF resources.

bflad · 2018-03-23T21:02:21Z

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:23:13Z

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 added the size/L Managed by automation to categorize the size of a PR. label Mar 15, 2018

appilon added the service/waf Issues and PRs that pertain to the waf service. label Mar 15, 2018

ghost added size/L Managed by automation to categorize the size of a PR. 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 15, 2018

appilon changed the title ~~[WIP] resource/aws_wafregional_size_constraint_set~~ resource/aws_wafregional_size_constraint_set Mar 15, 2018

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

radeksimko added the new-resource Introduces a new resource. label Mar 18, 2018

radeksimko reviewed Mar 20, 2018

View reviewed changes

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

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

appilon added 3 commits March 21, 2018 16:13

implement wafregional size constraint set

c25cfeb

begin moving helpers/common code to helper file

implement acceptance tests

b66fcae

wafregional size constraint set docs

4d45b3f

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

radeksimko approved these changes Mar 22, 2018

View reviewed changes

add type check in acceptance test teardown

c215252

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

appilon merged commit 618b1b1 into hashicorp:master Mar 22, 2018

bflad added this to the v1.12.0 milestone Mar 23, 2018

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

ghost locked and limited conversation to collaborators Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/aws_wafregional_size_constraint_set #3796

resource/aws_wafregional_size_constraint_set #3796

appilon commented Mar 15, 2018 •

edited

Loading

radeksimko left a comment

radeksimko Mar 20, 2018

appilon Mar 21, 2018

radeksimko Mar 20, 2018

appilon Mar 21, 2018

radeksimko Mar 20, 2018

appilon Mar 21, 2018

radeksimko Mar 20, 2018

appilon Mar 21, 2018

radeksimko Mar 20, 2018

appilon Mar 21, 2018

radeksimko Mar 20, 2018

appilon Mar 21, 2018

radeksimko Mar 20, 2018

appilon Mar 21, 2018

radeksimko Mar 20, 2018

appilon Mar 21, 2018

radeksimko Mar 20, 2018

appilon Mar 21, 2018

radeksimko Mar 20, 2018

appilon Mar 21, 2018

appilon commented Mar 21, 2018

radeksimko left a comment

radeksimko Mar 22, 2018

radeksimko Mar 22, 2018

radeksimko Mar 22, 2018

bflad commented Mar 23, 2018

ghost commented Apr 7, 2020


		name := d.Get("name").(string)

		log.Printf("[INFO] Creating SizeConstraintSet: %s", name)


		func testAccCheckAWSWafRegionalSizeConstraintSetDestroy(s *terraform.State) error {
		for _, rs := range s.RootModule().Resources {

resource/aws_wafregional_size_constraint_set #3796

resource/aws_wafregional_size_constraint_set #3796

Conversation

appilon commented Mar 15, 2018 • edited Loading

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

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

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

appilon commented Mar 21, 2018

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

bflad commented Mar 23, 2018

ghost commented Apr 7, 2020

appilon commented Mar 15, 2018 •

edited

Loading