New Resource: aws_wafregional_web_acl #3754

pvanbuijtene · 2018-03-12T22:42:52Z

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

Related PRs: #1046 hashicorp/terraform#13711

Thanks to the contributors: @yusukegoto @DennyLoko @BSick7

radeksimko · 2018-03-13T11:44:09Z

aws/resource_aws_wafregional_web_acl.go

+				ForceNew: true,
+			},
+			"default_action": &schema.Schema{
+				Type:     schema.TypeSet,


As this is practically a "singleton" there is no value in using TypeSet (which would deal with ordering of items in the set). Do you mind changing it to TypeList?
This should reduce complexity in CRUD and make the resource easier to use as users will be able to reference nested type field as default_action.0.type.

radeksimko · 2018-03-13T11:44:39Z

aws/resource_aws_wafregional_web_acl.go

+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"action": &schema.Schema{
+							Type:     schema.TypeSet,


Do you mind changing this to TypeList for the same reason as mentioned above in default_action?

radeksimko · 2018-03-13T11:53:39Z

aws/resource_aws_wafregional_web_acl.go

+	out, err := wr.RetryWithToken(func(token *string) (interface{}, error) {
+		params := &waf.CreateWebACLInput{
+			ChangeToken:   token,
+			DefaultAction: expandDefaultAction(d),


Couple of questions here:

Is there any reason we can't use expandDefaultActionWR here?

Is there a reason expandDefaultActionWR can't just accept the bare minimum - which is []interface{} instead of the whole schema.ResourceData?

radeksimko · 2018-03-13T11:54:41Z

aws/resource_aws_wafregional_web_acl.go

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


Do you mind replacing this check with a helper and simplify the code that way?

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

radeksimko · 2018-03-13T11:56:21Z

aws/resource_aws_wafregional_web_acl.go

+	}
+
+	defaultAction := flattenDefaultActionWR(resp.WebACL.DefaultAction)
+	if defaultAction != nil {


It is totally fine to pass nil to d.Set() it can handle (ignore) it safely, so this nil check here is redundant and we tend to avoid such nil checks to prevent cluttering the CRUD.

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

aws/resource_aws_wafregional_web_acl_test.go

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


This is redundant because the relationship is already built by the reference ${aws_wafregional_ipset.ipset.id} below. Do you mind removing it?

radeksimko · 2018-03-13T12:23:18Z

aws/resource_aws_wafregional_web_acl_test.go

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


This is redundant because the relationship is already built by the reference ${aws_wafregional_rule.wafrule.id} below and further reference inside the rule. Do you mind removing it?

radeksimko · 2018-03-13T12:23:53Z

website/docs/r/wafregional_web_acl.html.markdown

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


This is redundant because the relationship is already built by the reference ${aws_wafregional_rule.wafrule.id} below and further reference inside the rule. Do you mind removing it?

radeksimko · 2018-03-13T12:23:58Z

website/docs/r/wafregional_web_acl.html.markdown

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


This is redundant because the relationship is already built by the reference ${aws_wafregional_ipset.ipset.id} below. Do you mind removing it?

radeksimko · 2018-03-13T12:25:38Z

website/docs/r/wafregional_web_acl.html.markdown

+* `priority` - (Required) Specifies the order in which the rules in a WebACL are evaluated.
+  Rules with a lower value are evaluated before rules with a higher value.
+* `rule_id` - (Required) ID of the associated [rule](/docs/providers/aws/r/wafregional_rule.html)
+* `type` - (Optional) The rule type, either `REGULAR`, as defined by [Rule](https://docs.aws.amazon.com/waf/latest/APIReference/API_regional_Rule.html), or `RATE_BASED`, as defined by [RateBasedRule](http://docs.aws.amazon.com/waf/latest/APIReference/API_RateBasedRule.html). The default is REGULAR. If you add a RATE_BASED rule, you need to set `type` as `RATE_BASED`.


This field was not actually implemented yet - do you plan on finishing it?

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

@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-15T11:44:37Z

@radeksimko I understand, these are my first baby steps in golang.
Will check tonight if I can finish everything before end of this weekend, will let you know the outcome.

pvanbuijtene · 2018-03-15T22:42:57Z

Will finish other PR first because of dependency, see: #3756 (comment)

radeksimko · 2018-03-18T12:40:57Z

@pvanbuijtene #3756 was just merged. Do you mind rebasing this PR and looking at the feedback?

Thanks.

pvanbuijtene · 2018-03-18T20:28:04Z

@radeksimko thanks for the quick review and merge.

didn't have as much time as planned, will finish this one tomorrow night.

pvanbuijtene · 2018-03-19T22:52:31Z

@radeksimko it requires a bit more time and code, continuing tomorrow.

pvanbuijtene · 2018-03-20T21:43:27Z

aws/resource_aws_wafregional_web_acl_test.go

+		m := map[string]interface{}{
+			"rule_id":  **ruleId,
+			"priority": priority,
+			"action":   actionType,


@radeksimko I'm afraid I'm stuck here, can't figure out how to solve the puzzle how to pass the action in order to get the index.

Wanted to add some more tests steps for the rule fields, but this is blocking me.

You can check by running the test TestAccAWSWafRegionalWebAcl_changeRules

pvanbuijtene · 2018-03-20T21:45:16Z

@radeksimko this one was is a bit more challenging :)

Most of it is done so can be reviewed, needs some more tests but I'm stuck as explained above.

radeksimko

Hey @pvanbuijtene
as the PR is otherwise in a pretty good shape, I took the liberty and pushed some tiny fixes to get it across the finish line. I hope you don't mind.

As you can see from the diff the root cause of the crash, as demonstrated by your test, was that you were setting action as map, when it's TypeList in the schema, hence it needs to be wrapped as []interface{} (despite the fact that there'll always be only 1 item in the list).
It may be a little bit confusing, but I hope it makes sense.

I also fixed the flattener which wasn't producing correct data as uncovered by running tests with a special ENV variable:

TF_SCHEMA_PANIC_ON_ERROR=1 make testacc TEST=./aws TESTARGS='-run=TestAccAWSWafRegionalWebAcl_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSWafRegionalWebAcl_ -timeout 120m
=== RUN   TestAccAWSWafRegionalWebAcl_basic
panic: rule.0.action: '': source data must be an array or slice, got struct

Lastly I just cleaned up the docs a little bit.

I think this is now in mergeable state, so I will go ahead and merge it. In regards to missing type I understand that we'll first need to implement aws_wafregional_rule_group and aws_wafregional_rate_based_rule to take advantage of that field. i.e. it's ok to leave it out in this initial implementation.

Thank you for all your work. Let me know if you have any further questions.

pvanbuijtene · 2018-03-21T19:43:58Z

@radeksimko thanks for the finishing touch and clarification.
The changes are quite clear, but still early in my golang experience.

Tomorrow I'll have a look at the other PR.

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

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

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/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 service/waf Issues and PRs that pertain to the waf service. new-resource Introduces a new resource. 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

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

pvanbuijtene force-pushed the wafregional-web-acl branch from 134a56e to dac457a Compare March 13, 2018 20:53

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

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-web-acl branch from 1ce4708 to e558c83 Compare March 15, 2018 18:47

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

pvanbuijtene mentioned this pull request Mar 15, 2018

New Resource: aws_wafregional_rule #3756

Merged

DennyLoko and others added 9 commits March 19, 2018 21:15

Add support for aws_wafregional_web_acl

c368261

Updating wafregional web acl to match waf web acl

539cbd4

Remove redundant relationships

b66cf68

Use TypeList for singleton items

12116b4

Use regional expand func

fa9f756

Use aws error helper

b34550e

Remove redundant nil check

201b6e9

Only pass minimum required

ee1b809

Use convension

d0a8a40

pvanbuijtene force-pushed the wafregional-web-acl branch from e558c83 to ea6666f Compare March 19, 2018 21:10

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

Use singular name for rules field

ea6666f

pvanbuijtene added 2 commits March 20, 2018 21:59

Add tests without and changing rules

9c85098

Test extra fields

70cd91a

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

Remove unsupported rule type

9a592d8

pvanbuijtene commented Mar 20, 2018

View reviewed changes

radeksimko added 2 commits March 21, 2018 10:58

Fix flattener + hash func + clean up

ace9534

Clean up documentation

35450ff

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

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

radeksimko approved these changes Mar 21, 2018

View reviewed changes

radeksimko merged commit 90db988 into hashicorp:master Mar 21, 2018

radeksimko mentioned this pull request Mar 21, 2018

New Resource: aws_wafregional_web_acl #1046

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_web_acl #3754

New Resource: aws_wafregional_web_acl #3754

pvanbuijtene commented Mar 12, 2018 •

edited

Loading

radeksimko Mar 13, 2018

radeksimko Mar 13, 2018

radeksimko Mar 13, 2018

radeksimko Mar 13, 2018

radeksimko Mar 13, 2018

radeksimko Mar 13, 2018

radeksimko Mar 13, 2018

radeksimko Mar 13, 2018

radeksimko Mar 13, 2018

radeksimko Mar 13, 2018

radeksimko commented Mar 15, 2018

pvanbuijtene commented Mar 15, 2018

pvanbuijtene commented Mar 15, 2018

radeksimko commented Mar 18, 2018

pvanbuijtene commented Mar 18, 2018

pvanbuijtene commented Mar 19, 2018

pvanbuijtene Mar 20, 2018

pvanbuijtene Mar 20, 2018

pvanbuijtene commented Mar 20, 2018

radeksimko left a comment

pvanbuijtene commented Mar 21, 2018

bflad commented Mar 23, 2018

ghost commented Apr 7, 2020

New Resource: aws_wafregional_web_acl #3754

New Resource: aws_wafregional_web_acl #3754

Conversation

pvanbuijtene commented Mar 12, 2018 • edited Loading

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

radeksimko commented Mar 15, 2018

pvanbuijtene commented Mar 15, 2018

pvanbuijtene commented Mar 15, 2018

radeksimko commented Mar 18, 2018

pvanbuijtene commented Mar 18, 2018

pvanbuijtene commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvanbuijtene commented Mar 20, 2018

radeksimko left a comment

Choose a reason for hiding this comment

pvanbuijtene commented Mar 21, 2018

bflad commented Mar 23, 2018

ghost commented Apr 7, 2020

pvanbuijtene commented Mar 12, 2018 •

edited

Loading