provider/aws: AWS WAF Regional IPSet + ByteMatchSet support #13705

yusukegoto · 2017-04-17T13:18:39Z

This PR is from #13676.

test

resource "aws_wafregional_ipset" "ipset" {
  name = "tfIPSet"
  ip_set_descriptors {
    type = "IPV4"
    value = "192.0.7.0/24"
  }
}

exec

aws_wafregional_ipset.ipset: Creating...
  ip_set_descriptors.#:                "" => "1"
  ip_set_descriptors.4037960608.type:  "" => "IPV4"
  ip_set_descriptors.4037960608.value: "" => "192.0.7.0/24"
  name:                                "" => "tfIPSet"
aws_wafregional_ipset.ipset: Creation complete (ID: 38bf9b97-72bb-412c-bd6f-f8c5b911127b)

radeksimko

Thanks again for splitting this out. It's so much easier to review! 👍 💯

I left you some comments there - some are rather nitpicks, some are more important.
Read() funcs not setting all fields and region-based locking are the main ones.

I will ping you once I have a PR for WAF fixes ready - just for the reference. 😃

radeksimko · 2017-04-18T07:06:41Z

builtin/providers/aws/resource_aws_wafregional_byte_match_set.go

+			ByteMatchSetId: aws.String(d.Id()),
+		}
+
+		ByteMatchTuples := d.Get("byte_match_tuples").(*schema.Set)


Perhaps a nitpick, but It's a convention to keep all internal variables (valid only within the context of a function) with lowercased letter at the beginning.

radeksimko · 2017-04-18T07:25:21Z

builtin/providers/aws/resource_aws_wafregional_byte_match_set.go

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


I think we're missing some fields here, specifically byte_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.

Sets can be slightly trickier to work with, I will try and fix the WAF resources first and point you there for reference instead of trying to explain how this can be done. 😅

It's probably the cause of some of the reported WAF bugs I saw earlier, so it's worth fixing there either way.

btw. this would also break import functionality when it's actually implemented as it leverages Read() (at least the default implementation)

radeksimko · 2017-04-18T07:28:26Z

builtin/providers/aws/resource_aws_wafregional_byte_match_set_test.go

+			resource.TestStep{
+				Config: testAccAWSWafRegionalByteMatchSetConfig(byteMatchSet),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSWafRegionalByteMatchSetExists("aws_wafregional_byte_match_set.byte_set", &v),


It would be nice to have some checks for the real object, i.e. v - like we have in many other resources, but it's not a blocker for the PR. 👌

radeksimko · 2017-04-18T07:33:13Z

builtin/providers/aws/resource_aws_wafregional_byte_match_set.go

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


Given how the sets and lists are represented in HCL, i.e.

byte_match_tuples { ... } byte_match_tuples { ... }

it would make more sense if this field name was singular - i.e. byte_match_tuple. I will have a look into deprecating byte_match_tuples in favour of singular name in WAF resources too as we seemed to overlook this in the review of that resource code there.

@radeksimko I agree with this. AWS CLI's api ByteMatchTuples requires list but in this case we can write each tuple. Go SDK uses tuple.
Should I change into byte_match_tuple only in WAF Regional? Or leave them in this time and change them later at the same time with WAF?

uhm... I got the idea resource name should be singular during writing document.

Should I change into byte_match_tuple only in WAF Regional?

Yes, please.

Don't worry about other (WAF) resources - we can rename/deprecate those later - that's a job for a separate PR.

radeksimko · 2017-04-18T07:34:02Z

builtin/providers/aws/resource_aws_wafregional_ipset.go

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


Similar to the other schema - it would make more sense to call this ip_set_descriptor IMO.

radeksimko · 2017-04-18T07:37:40Z

builtin/providers/aws/resource_aws_wafregional_ipset.go

+		IPSetDescriptors = append(IPSetDescriptors, IPSet)
+	}
+
+	d.Set("ip_set_descriptors", IPSetDescriptors)


Would you mind adding error check here? We tend to avoid error checking for d.Set() of all primitive fields merely to avoid cluttering the code, but we tend to do it for lists and sets as we were bitten by not doing it many times in the past.

radeksimko · 2017-04-18T07:41:12Z

builtin/providers/aws/wafregionl_token_handlers.go

+)
+
+type WafRegionalRetryer struct {
+	Connection *wafregional.WAFRegional


I thought my original WafRetryer would be useful for you here too, but I never realized it's a different service with a different name 🤦‍♂️

Oh well, good decision to create a copy of it. 👍

radeksimko · 2017-04-18T07:44:01Z

builtin/providers/aws/wafregionl_token_handlers.go

+	return out, err
+}
+
+func newWafRegionalRetryer(conn *wafregional.WAFRegional) *WafRegionalRetryer {


I'm aware this will require some refactoring throughout all the resources, but I think it's important to only lock for a given region, so that if folks deploy regional WAF in say 2 regions, one region doesn't wait for lock of the other region as it doesn't have to.

radeksimko · 2017-04-18T07:44:37Z

website/source/docs/providers/aws/r/wafregional_byte_match_set.html.markdown

+The following arguments are supported:
+
+* `name` - (Required) The name or description of the ByteMatchSet.
+* `byte_match_tuples` - Settings for the ByteMatchSet, such as the bytes (typically a string that corresponds with ASCII characters) that you want AWS WAF to search for in web requests. 


Would you mind documenting the nested fields of byte_match_tuples here too? 🙂

radeksimko · 2017-04-18T07:44:55Z

website/source/docs/providers/aws/r/wafregional_ipset.html.markdown

+The following arguments are supported:
+
+* `name` - (Required) The name or description of the IPSet.
+* `ip_set_descriptors` - (Required) The IP address type and IP address range (in CIDR notation) from which web requests originate. 


Would you mind documenting the nested fields of ip_set_descriptors here too? 🙂

radeksimko · 2017-04-18T07:51:35Z

FYI I will hold on reviewing the other WAF Regional PRs until we get this in as those will most likely require rebase and/or some cherry-picking of changes from this PR.

radeksimko · 2017-04-27T18:46:46Z

Hi @yusukegoto
thanks for the changes you've made so far in ByteMatchSet (singular, docs updates, diffing of tuples). Those are looking ok 👌

#13766 was merged - would you mind looking over it 👀 and adapting the rest of the changes/fixes there to this PR too?

Namely:

Two tests for ByteMatchSet (set with no tuples + changing existing tuples within a set)
WafRegionalRetryer now has Region field, but it needs to be actually in use, ideally as argument to newWafRegionalRetryer
Two tests for IPSet (set with no descriptors + changing existing descriptors within a set)
Turn ip_set_descriptors into a singleton - i.e. ip_set_descriptor
Allow changing existing descriptors within an existing IPSet + allow creating IPSet with no descriptor
Document descriptor fields for IPSet

I hope I didn't miss anything 😃
Let me know if anything's unclear and/or if you need any help in making these changes.

yusukegoto · 2017-04-28T00:34:53Z

@radeksimko Thanks for clearing remaining tasks! And sorry for slow response.
I will be able to have much time to fix these on weekend!

yusukegoto · 2017-05-04T02:15:57Z

@radeksimko
hi, I think I could, plz re-review them!! Thanks in advance.

reedloden · 2017-05-04T02:22:14Z

@yusukegoto, I think you mean @radeksimko, not me. :)

yusukegoto · 2017-05-04T02:31:15Z

@reedloden sorry for disturbing 🙇

radeksimko

There was a few small last things which I decided to fix and push into your branch to speed things up, I hope you don't mind. 😅

Thank you for all the changes and patience. 👍

radeksimko · 2017-05-13T17:45:37Z

builtin/providers/aws/resource_aws_wafregional_ipset_test.go

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


Nice, good to have this test 👍

yusukegoto · 2017-05-15T23:22:14Z

@radeksimko Thank you very much for additional commits and care!! I'm trying to clean other WIP PRs:smile:

ghost · 2020-04-12T02:19:53Z

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

radeksimko requested changes Apr 18, 2017

View reviewed changes

radeksimko mentioned this pull request Apr 18, 2017

provider/aws: AWS WAF Regional ByteMatchSet support #13703

Closed

radeksimko added provider/aws new-resource labels Apr 18, 2017

radeksimko changed the title ~~providor/aws: AWS WAF Regional IPSet support~~ providor/aws: AWS WAF Regional IPSet + ByteMatchSet support Apr 18, 2017

This was referenced Apr 18, 2017

provider/aws Add support for WAF Regional #11263

Closed

provider/aws: Fix WAF IPSet descriptors removal on update #13766

Merged

yusukegoto force-pushed the wafregional-ipset branch 2 times, most recently from d4a118e to 38a0c16 Compare April 23, 2017 00:23

yusukegoto changed the title ~~providor/aws: AWS WAF Regional IPSet + ByteMatchSet support~~ provider/aws: AWS WAF Regional IPSet + ByteMatchSet support Apr 23, 2017

yusukegoto force-pushed the wafregional-ipset branch from 38a0c16 to 3bd1683 Compare April 23, 2017 01:03

radeksimko self-assigned this Apr 27, 2017

radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Apr 27, 2017

yusukegoto force-pushed the wafregional-ipset branch from 3bd1683 to 2ba6e2b Compare May 3, 2017 15:33

yusukegoto added 10 commits May 4, 2017 10:03

provider/aws: AWS WAF Regional ByteMatchSet support

2b80e31

providor/aws: AWS WAF Regional IPSet support

3917cca

fix typo

71907d9

Nested ByteMatchTuples of WAF Regional support

87899f4

fix name byte_match_tuple singular WAF Regional

6f44bd2

update wafregional_byte_match_set doc

8be3e3c

Add wafregional_byte_match_set test

cbf5afb

fix wafregional ipset to look descriptor

46339cf

fix wafregional_ipset_descriptor to be singular

cb32fc9

fix newWafRegionalRetryer to receive region

465cf4a

yusukegoto force-pushed the wafregional-ipset branch from f058f6b to 465cf4a Compare May 4, 2017 01:04

fix updateSetWR argument

69326b6

radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label May 4, 2017

fix wafregional_ipset_descriptor doc

5a73a6f

yusukegoto force-pushed the wafregional-ipset branch from 3d2c512 to 5a73a6f Compare May 4, 2017 08:36

yusukegoto mentioned this pull request May 4, 2017

[WIP] provider/aws: AWS WAF Regional SizeConstraintSet support #13707

Closed

radeksimko added 2 commits May 13, 2017 20:52

Separate out logic into flatteners

f1313c3

Fix failing test

d30580e

radeksimko approved these changes May 13, 2017

View reviewed changes

radeksimko merged commit 3b14fc9 into hashicorp:master May 13, 2017

This was referenced Jul 18, 2017

New Resource: aws_wafregional_sql_injection_match_set hashicorp/terraform-provider-aws#1013

Merged

New Resource: aws_wafregional_xss_match_set hashicorp/terraform-provider-aws#1014

Merged

ghost locked and limited conversation to collaborators Apr 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/aws: AWS WAF Regional IPSet + ByteMatchSet support #13705

provider/aws: AWS WAF Regional IPSet + ByteMatchSet support #13705

yusukegoto commented Apr 17, 2017 •

edited

Loading

radeksimko left a comment

radeksimko Apr 18, 2017

radeksimko Apr 18, 2017

radeksimko Apr 18, 2017

radeksimko Apr 18, 2017

radeksimko Apr 18, 2017

yusukegoto Apr 22, 2017 •

edited

Loading

yusukegoto Apr 22, 2017

radeksimko Apr 22, 2017

radeksimko Apr 18, 2017

radeksimko Apr 18, 2017

radeksimko Apr 18, 2017

radeksimko Apr 18, 2017

radeksimko Apr 18, 2017

radeksimko Apr 18, 2017

radeksimko commented Apr 18, 2017

radeksimko commented Apr 27, 2017

yusukegoto commented Apr 28, 2017 •

edited

Loading

yusukegoto commented May 4, 2017 •

edited

Loading

reedloden commented May 4, 2017

yusukegoto commented May 4, 2017

radeksimko left a comment

radeksimko May 13, 2017

yusukegoto commented May 15, 2017

ghost commented Apr 12, 2020

provider/aws: AWS WAF Regional IPSet + ByteMatchSet support #13705

provider/aws: AWS WAF Regional IPSet + ByteMatchSet support #13705

Conversation

yusukegoto commented Apr 17, 2017 • 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

yusukegoto Apr 22, 2017 • 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

radeksimko commented Apr 18, 2017

radeksimko commented Apr 27, 2017

yusukegoto commented Apr 28, 2017 • edited Loading

yusukegoto commented May 4, 2017 • edited Loading

reedloden commented May 4, 2017

yusukegoto commented May 4, 2017

radeksimko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yusukegoto commented May 15, 2017

ghost commented Apr 12, 2020

yusukegoto commented Apr 17, 2017 •

edited

Loading

yusukegoto Apr 22, 2017 •

edited

Loading

yusukegoto commented Apr 28, 2017 •

edited

Loading

yusukegoto commented May 4, 2017 •

edited

Loading