-
Notifications
You must be signed in to change notification settings - Fork 9.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New Resource: aws_wafregional_sql_injection_match_set #1013
New Resource: aws_wafregional_sql_injection_match_set #1013
Conversation
b6260c8
to
2991e59
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, I left you some comments there. Let me know if you have any questions.
Required: true, | ||
ForceNew: true, | ||
}, | ||
"sql_injection_match_tuples": &schema.Schema{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed in the other resource do you mind renaming this field to singular? i.e. sql_injection_match_tuple
?
return err | ||
} | ||
|
||
d.Set("name", resp.SqlInjectionMatchSet.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're missing some fields here, specifically sql_injection_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.
}, | ||
}) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because there has been a bug in the past affecting all WAF resources I'd like to see 2 more tests here, specifically with no tuples and another one changing tuples, see https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_waf_sql_injection_match_set_test.go#L103-L173 for inspiration and eacece2 for context (bugfix).
|
||
func resourceAwsWafRegionalSqlInjectionMatchSetUpdate(d *schema.ResourceData, meta interface{}) error { | ||
log.Printf("[INFO] Updating SqlInjectionMatchSet: %s", d.Get("name").(string)) | ||
err := updateSqlInjectionMatchSetResourceWR(d, meta, waf.ChangeActionInsert) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this isn't right, because during update we may be both inserting and deleting tuples, not just inserting. See eacece2 for more context.
The following arguments are supported: | ||
|
||
* `name` - (Required) The name or description of the SizeConstraintSet. | ||
* `sql_injection_match_tuples` - The parts of web requests that you want AWS WAF to inspect for malicious SQL code and, if you want AWS WAF to inspect a header, the name of the header. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind documenting the nested fields here, too?
|
||
func testAccCheckAWSWafRegionalSqlInjectionMatchSetDestroy(s *terraform.State) error { | ||
for _, rs := range s.RootModule().Resources { | ||
if rs.Type != "aws_wafregional_byte_match_set" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is typo 👀
Hello, please for review my PR. I'll make the changes during this week. |
Hi @DennyLoko Eventually if you don't have time to invest into these WAF PRs do let me know too. I'm happy to finish it (while keeping your authorship in existing commits). |
FYI - As this PR has been stale for a couple of months I will take it over in coming week(s) unless you tell me not to. |
@radeksimko I am trying to finalize this PR @ #3199 |
2991e59
to
a747666
Compare
I believe I addressed all things I saw myself as blockers, I think it's worth one more review from someone with a fresh eye though.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall in pretty awesome shape as usual! Few nitpicks then its good to ship!
5 tests passed (all tests)
=== RUN TestAccAWSWafRegionalSqlInjectionMatchSet_disappears
--- PASS: TestAccAWSWafRegionalSqlInjectionMatchSet_disappears (10.55s)
=== RUN TestAccAWSWafRegionalSqlInjectionMatchSet_noTuples
--- PASS: TestAccAWSWafRegionalSqlInjectionMatchSet_noTuples (19.74s)
=== RUN TestAccAWSWafRegionalSqlInjectionMatchSet_changeTuples
--- PASS: TestAccAWSWafRegionalSqlInjectionMatchSet_changeTuples (22.75s)
=== RUN TestAccAWSWafRegionalSqlInjectionMatchSet_changeNameForceNew
--- PASS: TestAccAWSWafRegionalSqlInjectionMatchSet_changeNameForceNew (23.86s)
=== RUN TestAccAWSWafRegionalSqlInjectionMatchSet_basic
--- PASS: TestAccAWSWafRegionalSqlInjectionMatchSet_basic (25.59s)
Delete: resourceAwsWafRegionalSqlInjectionMatchSetDelete, | ||
|
||
Schema: map[string]*schema.Schema{ | ||
"name": &schema.Schema{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick: Extraneous &schema.Schema
Schema: map[string]*schema.Schema{ | ||
"name": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Required: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something up for discussion: should we automatically be adding ValidateFunc: validation.NoZeroValues,
for Required: true
and Type: schema.TypeString
where it makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I'm not sure - it sounds like something that should be solved at higher (schema) level. I mean if from provider-dev perspective I say "it's required" I'd just assume that's enough to say there are no empty strings allowed.
Maybe there are edge cases when we do not want that though? 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it likely should be fixed upstream 😄 The only weird edge cases I can think of are module usage somehow, but even that sounds pretty far fetched.
Provides a AWS WAF Regional SqlInjectionMatchSet resource for use with ALB. | ||
--- | ||
|
||
# aws\_wafregional\_sql\_injection\_match\_set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick: Extraneous backslashes
See [docs](https://docs.aws.amazon.com/waf/latest/APIReference/API_regional_FieldToMatch.html) | ||
for all supported values. | ||
|
||
## Remarks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Extraneous header
|
||
## Example Usage | ||
|
||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Should define syntax as hcl
}`, name) | ||
} | ||
|
||
func testAccAWSWafRegionalSqlInjectionMatchSetConfigChangeName(name string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: this function looks like it duplicates the above one
return conn.UpdateSqlInjectionMatchSet(req) | ||
}) | ||
if err != nil { | ||
return errwrap.Wrapf("[ERROR] Error updating SqlInjectionMatchSet: {{err}}", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick: errwrap.Wrapf()
usage instead of fmt.Errorf()
(same with below)
a747666
to
f5ee8a9
Compare
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! |
This PR implements the
aws_wafregional_sql_injection_match_set
resource.