New Resource: aws_waf_geo_match_set #3275
Conversation
mikesouza
changed the title
bflad
added
new-resource
There was a problem hiding this comment.
Hi @mikesouza
thanks for the PR.
This is looking pretty good, especially as it's your first contribution 😃
I left you some comments to address + I think the branch will need rebasing & resolving conflicts. Once that is done I'm happy to merge this.
|
|
||
| for k, v := range rs.Primary.Attributes { | ||
| fmt.Println(k) | ||
| fmt.Println(v) |
There was a problem hiding this comment.
👆 Do you plan to finish this? I'm not sure what was supposed to be the purpose of this function - maybe it just needs removing?
|
|
||
| func testAccCheckAWSWafGeoMatchSetDestroy(s *terraform.State) error { | ||
| for _, rs := range s.RootModule().Resources { | ||
| if rs.Type != "aws_waf_byte_match_set" { |
There was a problem hiding this comment.
Typo - I assume this should be aws_waf_geo_match_set
| @@ -54,7 +54,7 @@ The following arguments are supported: | |||
| For example, if an IPSet includes the IP address `192.0.2.44`, AWS WAF will allow or block requests based on that IP address. | |||
| If set to `true`, AWS WAF will allow, block, or count requests based on all IP addresses _except_ `192.0.2.44`. | |||
| * `data_id` - (Required) A unique identifier for a predicate in the rule, such as Byte Match Set ID or IPSet ID. | |||
| * `type` - (Required) The type of predicate in a rule, such as `ByteMatchSet` or `IPSet` | |||
| * `type` - (Required) The type of predicate in a rule. Must be one of `IPMatch`, `ByteMatch`, `SqlInjectionMatch`, `SizeConstraint`, `XssMatch`, or `GeoMatch`. | |||
There was a problem hiding this comment.
Nitpick: We don't usually try to put extensive list of all valid values in our documentation, but instead link to official docs. This is to avoid a constant uphill battle of being out of date as Amazon releases new stuff and changes these things.
|
|
||
| // Return nil if the GeoMatchSet is already destroyed | ||
| if awsErr, ok := err.(awserr.Error); ok { | ||
| if awsErr.Code() == "WAFNonexistentItemException" { |
There was a problem hiding this comment.
Do you mind using a helper function here and simplify the code that way a bit? 😉
if isAWSErr(err, "WAFNonexistentItemException", "") {|
|
||
| resp, err := conn.GetGeoMatchSet(params) | ||
| if err != nil { | ||
| if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == "WAFNonexistentItemException" { |
There was a problem hiding this comment.
Do you mind using a helper function here and simplify the code that way a bit? 😉
if isAWSErr(err, "WAFNonexistentItemException", "") {| Schema: map[string]*schema.Schema{ | ||
| "type": { | ||
| Type: schema.TypeString, | ||
| Optional: true, |
There was a problem hiding this comment.
This field seems to be required per docs, do you mind reflecting it here?
| Required: true, | ||
| ForceNew: true, | ||
| }, | ||
| "geo_match_constraints": &schema.Schema{ |
There was a problem hiding this comment.
As discussed in all recent WAF PRs we tend to use singular names for Sets/Lists which have nested fields of non-primitive data types, like here. This is because we expect them to be used in HCL like this:
geo_match_constraint {
type = "Country"
value = "US"
}
geo_match_constraint {
type = "Country"
value = "CA"
}as opposed to lists/sets with primitive data types, like
countries = ["CA", "US", "CN"]Do you mind changing it to geo_match_constraint and reflecting it in CRUD & docs too?
radeksimko
added
the
waiting-response
|
@radeksimko Thanks for reviewing! I believe I've addressed all of your comments, and rebased and merged. Unfortunately, I force-pushed the changes and so I apologize if this had caused any inconvenience or issues with reviewing/ merging. I've learned my lesson. |
radeksimko
removed
the
waiting-response
|
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. |
|
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
locked and limited conversation to collaborators
I added a
aws_waf_geo_match_setresource which supports the AWS WAF GeoMatchSet feature added back in October 2017.Per the contributing guidelines, I followed the conventions used by the existing WAF resources/ match sets, included and passed acceptance tests, updated the documentation, and kept the LOC to a minimum.
I also added an acceptance test to the existing
aws_waf_ruleresource to verify that it works with the newaws_waf_geo_match_setresource.This was relatively straightforward to implement, as the behavior of the new resource is identical to the existing WAF match set resources, with its main difference being a simpler schema (i.e. compared to
aws_waf_xss_match_set).Please let me know if you have any questions, concerns, or would like me to make any changes, thanks!
Related