New Resource: aws_wafregional_rule #3756

pvanbuijtene · 2018-03-12T22:49:46Z

Trying to help a bit to finish the WAF Regional PRs, this is the result of the split up of #3199

Related PRs: #1045 hashicorp/terraform#13710

Thanks to the contributors: @yusukegoto @DennyLoko @BSick7

radeksimko · 2018-03-13T08:45:13Z

aws/resource_aws_wafregional_rule.go

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


Due to the way this may (and likely is going to be) expressed in HCL:

predicates { negated = true data_id = "..." } predicates { data_id = "..." }

we tend to prefer singular names for TypeSet/TypeList fields with non-primitive nested types. i.e. predicate in this case. Do you mind changing it here in the schema and in the CRUD & docs?

radeksimko · 2018-03-13T08:49:42Z

aws/resource_aws_wafregional_rule.go

+										"%q must be one of IPMatch | ByteMatch | SqlInjectionMatch | SizeConstraint | XssMatch", k))
+								}
+								return
+							},


Nitpick: We have a handy validation helper for this:

ValidateFunc: validation.StringInSlice([]string{ "IPMatch", "ByteMatch", "SqlInjectionMatch", "SizeConstraint", "XssMatch", }, false),

radeksimko · 2018-03-13T08:51:14Z

aws/resource_aws_wafregional_rule.go

+								}
+								return
+							},
+						},


Nipick: We have a validation helper for this:

ValidateFunc: validation.StringLenBetween(1, 128),

radeksimko · 2018-03-13T08:52:42Z

aws/resource_aws_wafregional_rule.go

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


Do you mind using a helper to simplify the code here?

if isAWSErr(err, wafregional.ErrCodeWAFNonexistentItemException, "") {

radeksimko · 2018-03-13T08:55:28Z

aws/resource_aws_wafregional_rule.go

+		predicates = append(predicates, predicate)
+	}
+
+	d.Set("predicates", predicates)


Nitpick: We tend to decouple logic like this (anything converting SDK structures to schema-friendly structures and vice-versa) to functions which are usually called expanders (schema -> SDK) or flatteners (SDK -> schema).
This may then simplify the code above to

d.Set("predicates", flattenWafPredicates(resp.Rule.Predicates))

aws/resource_aws_wafregional_rule.go

+			},
+			"metric_name": &schema.Schema{
+				Type:     schema.TypeString,
+				Required: true,


aws/resource_aws_wafregional_rule.go

+		Schema: map[string]*schema.Schema{
+			"name": &schema.Schema{
+				Type:     schema.TypeString,
+				Required: true,


aws/resource_aws_wafregional_rule.go

+			},
+			"predicates": &schema.Schema{
+				Type:     schema.TypeSet,
+				Optional: true,


radeksimko · 2018-03-13T09:10:17Z

aws/resource_aws_wafregional_rule.go

+						},
+						"data_id": &schema.Schema{
+							Type:     schema.TypeString,
+							Optional: true,


It seems this field is actually required per docs:
https://docs.aws.amazon.com/waf/latest/APIReference/API_regional_Rule.html 🤔
Do you mind double checking it and eventually reflecting it here and in the CRUD?

radeksimko · 2018-03-13T09:14:00Z

aws/resource_aws_wafregional_rule_test.go

+}
+
+resource "aws_wafregional_rule" "wafrule" {
+  depends_on = ["aws_wafregional_ipset.ipset"]


This line is redundant as the relationship is already defined by the reference below (${aws_wafregional_ipset.ipset.id}). Do you mind removing it?

radeksimko · 2018-03-15T08:49:06Z

@pvanbuijtene let me know if you need any help finishing this. I'd like to get it into 1.12.0 (tentatively scheduled for the end of next week) and this particular resource has been PR'd a couple of times, so I'm willing to take it to the finish line myself, if necessary 😉

pvanbuijtene · 2018-03-15T22:41:22Z

@radeksimko I will finish this one first since the tests of #3754 depend on it.

So most work for this PR, #3754 and #3755 I can probably do in the weekend, but finishing it depends on when this PR can be merged which will be Monday or later guess?

radeksimko · 2018-03-16T06:37:29Z

@pvanbuijtene The plan makes sense to me. I'll do my best to review any WAF PR that you tell me is ready for review. 😉 Just feel free to ping me the same way you just did.

pvanbuijtene · 2018-03-17T20:46:18Z

@radeksimko this one is ready :) ... hope I didn't forget anything.

Some questions arose while being busy with this:
I guess there is an internal build which runs the acceptance tests?
What are the rules/guidelines regarding the usage of funcs from different resources?

radeksimko · 2018-03-18T12:24:50Z

Thanks @pvanbuijtene
I just reformatted imports in one file and removed a few lines of redundant code related to error checks. I hope these changes make sense. Let me know if you need any further explanation.

I guess there is an internal build which runs the acceptance tests?

We run acceptance tests nightly on our side, but anyone can run them locally e.g.

AWS_PROFILE=... make testacc TEST=./aws TESTARGS='-run=TestAccAWSWafRegionalRule_'

What are the rules/guidelines regarding the usage of funcs from different resources?

We don't have any particular rules, but WAF resources in particular could use some refactoring in that sense. We can address that in a separate PR, eventually.

bflad · 2018-03-23T21:01:39Z

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:19Z

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!

DennyLoko and others added 2 commits March 12, 2018 22:22

Add support for aws_wafregional_rule

f95d112

Updating docs

5a42045

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

pvanbuijtene mentioned this pull request Mar 12, 2018

Adding missing wafregional resources #3199

Closed

radeksimko added new-resource Introduces a new resource. service/waf Issues and PRs that pertain to the waf service. labels Mar 13, 2018

radeksimko suggested changes Mar 13, 2018

View reviewed changes

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

radeksimko mentioned this pull request Mar 13, 2018

New Resource: aws_wafregional_web_acl_association #3755

Merged

radeksimko added this to the v1.12.0 milestone Mar 15, 2018

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

pvanbuijtene force-pushed the wafregional-rule branch from 899f7b3 to 61478f2 Compare March 15, 2018 20:55

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

pvanbuijtene added 4 commits March 15, 2018 21:56

Use singular name for predicate field

cc2d84c

Use helpers

61478f2

Use flatten func

40b0a08

Add test for rule without predicates

210fef9

pvanbuijtene mentioned this pull request Mar 15, 2018

New Resource: aws_wafregional_web_acl #3754

Merged

pvanbuijtene added 2 commits March 17, 2018 21:28

Add test for predicate changes

1f23227

Require data_id for predicate

61cf64a

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

Reformat imports + remove redundant code

81e084a

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

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

radeksimko approved these changes Mar 18, 2018

View reviewed changes

radeksimko merged commit cecf1d8 into hashicorp:master Mar 18, 2018

This was referenced Mar 18, 2018

New Resource: aws_wafregional_rule #1045

Closed

New Resource: aws_wafregional_rule #1753

Closed

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_rule #3756

New Resource: aws_wafregional_rule #3756

pvanbuijtene commented Mar 12, 2018

radeksimko Mar 13, 2018

radeksimko Mar 13, 2018

radeksimko Mar 13, 2018

radeksimko Mar 13, 2018

radeksimko Mar 13, 2018

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

radeksimko Mar 13, 2018

radeksimko Mar 13, 2018

radeksimko commented Mar 15, 2018

pvanbuijtene commented Mar 15, 2018

radeksimko commented Mar 16, 2018

pvanbuijtene commented Mar 17, 2018

radeksimko commented Mar 18, 2018

bflad commented Mar 23, 2018

ghost commented Apr 7, 2020

New Resource: aws_wafregional_rule #3756

New Resource: aws_wafregional_rule #3756

Conversation

pvanbuijtene commented Mar 12, 2018

radeksimko Mar 13, 2018

Choose a reason for hiding this comment

radeksimko Mar 13, 2018

Choose a reason for hiding this comment

radeksimko Mar 13, 2018

Choose a reason for hiding this comment

radeksimko Mar 13, 2018

Choose a reason for hiding this comment

radeksimko Mar 13, 2018

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

radeksimko Mar 13, 2018

Choose a reason for hiding this comment

radeksimko Mar 13, 2018

Choose a reason for hiding this comment

radeksimko commented Mar 15, 2018

pvanbuijtene commented Mar 15, 2018

radeksimko commented Mar 16, 2018

pvanbuijtene commented Mar 17, 2018

radeksimko commented Mar 18, 2018

bflad commented Mar 23, 2018

ghost commented Apr 7, 2020