Skip to content
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.

Sign up for GitHub

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

r/waf_rule: catch referenced item exception when removing WAF rule; additional linting #17876

Merged
merged 2 commits into from
Mar 16, 2021
Merged

r/waf_rule: catch referenced item exception when removing WAF rule; additional linting #17876

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1291515
catch in-use exception when removing WAF rule; linting
anGie44 Mar 2, 2021
62adf9a
add AWS SDK enumerations and simplify validations; test linting
anGie44 Mar 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 39 additions & 16 deletions aws/resource_aws_waf_rule.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ import (

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/arn"
"github.com/aws/aws-sdk-go/aws/awserr"
"github.com/aws/aws-sdk-go/service/waf"
"github.com/hashicorp/aws-sdk-go-base/tfawserr"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
Expand Down Expand Up @@ -95,7 +95,7 @@ func resourceAwsWafRuleCreate(d *schema.ResourceData, meta interface{}) error {
noPredicates := []interface{}{}
err := updateWafRuleResource(d.Id(), noPredicates, newPredicates, conn)
if err != nil {
return fmt.Errorf("Error Updating WAF Rule: %s", err)
return fmt.Errorf("error updating WAF Rule (%s): %w", d.Id(), err)
}
}

Expand All @@ -111,14 +111,24 @@ func resourceAwsWafRuleRead(d *schema.ResourceData, meta interface{}) error {
}

resp, err := conn.GetRule(params)
if !d.IsNewResource() && tfawserr.ErrCodeEquals(err, waf.ErrCodeNonexistentItemException) {
log.Printf("[WARN] WAF Rule (%s) not found, removing from state", d.Id())
d.SetId("")
return nil
}

if err != nil {
if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == waf.ErrCodeNonexistentItemException {
log.Printf("[WARN] WAF Rule (%s) not found, removing from state", d.Id())
d.SetId("")
return nil
return fmt.Errorf("error getting WAF Rule (%s): %w", d.Id(), err)
}

if resp == nil || resp.Rule == nil {
if d.IsNewResource() {
return fmt.Errorf("error getting WAF Rule (%s): not found", d.Id())
}

return err
log.Printf("[WARN] WAF Rule (%s) not found, removing from state", d.Id())
d.SetId("")
return nil
}

var predicates []map[string]interface{}
Expand All @@ -143,11 +153,11 @@ func resourceAwsWafRuleRead(d *schema.ResourceData, meta interface{}) error {
tags, err := keyvaluetags.WafListTags(conn, arn)

if err != nil {
return fmt.Errorf("error listing tags for WAF Rule (%s): %s", arn, err)
return fmt.Errorf("error listing tags for WAF Rule (%s): %w", d.Id(), err)
}

if err := d.Set("tags", tags.IgnoreAws().IgnoreConfig(ignoreTagsConfig).Map()); err != nil {
return fmt.Errorf("error setting tags: %s", err)
return fmt.Errorf("error setting tags: %w", err)
}

d.Set("predicates", predicates)
Expand All @@ -166,15 +176,15 @@ func resourceAwsWafRuleUpdate(d *schema.ResourceData, meta interface{}) error {

err := updateWafRuleResource(d.Id(), oldP, newP, conn)
if err != nil {
return fmt.Errorf("Error Updating WAF Rule: %s", err)
return fmt.Errorf("error updating WAF Rule (%s): %w", d.Id(), err)
}
}

if d.HasChange("tags") {
o, n := d.GetChange("tags")

if err := keyvaluetags.WafUpdateTags(conn, d.Get("arn").(string), o, n); err != nil {
return fmt.Errorf("error updating tags: %s", err)
return fmt.Errorf("error updating WAF Rule (%s) tags: %w", d.Id(), err)
}
}

Expand All @@ -189,7 +199,7 @@ func resourceAwsWafRuleDelete(d *schema.ResourceData, meta interface{}) error {
noPredicates := []interface{}{}
err := updateWafRuleResource(d.Id(), oldPredicates, noPredicates, conn)
if err != nil {
return fmt.Errorf("Error updating WAF Rule Predicates: %s", err)
return fmt.Errorf("error updating WAF Rule (%s) predicates: %w", d.Id(), err)
}
}

Expand All @@ -199,11 +209,24 @@ func resourceAwsWafRuleDelete(d *schema.ResourceData, meta interface{}) error {
ChangeToken: token,
RuleId: aws.String(d.Id()),
}
log.Printf("[INFO] Deleting WAF Rule")
return conn.DeleteRule(req)

output, err := conn.DeleteRule(req)

// Deleting a WAF Rule after being removed from a WAF WebACL
// can return a WAFReferencedItemException when attempted in quick succession;
// thus, we catch the error here and re-attempt
if tfawserr.ErrCodeEquals(err, waf.ErrCodeReferencedItemException) {
return output, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain how this error is being retried? The RetryWithToken helper currently only handles the waf.ErrCodeStaleDataException error and this logic is swallowing waf.ErrCodeReferencedItemException. I'm guessing this might be passing TestAccAWSWafWebAcl_Rules because that test uses CheckDestroy: testAccCheckAWSWafWebAclDestroy and does not verify rule deletion.

This logic would need another layer of resource.Retry() to retry successfully on waf.ErrCodeReferencedItemException and it might be worth creating a covering TestAccAWSWafRule_ test for this behavior so it can use CheckDestroy: testAccCheckAWSWafRuleDestroy,. 👍

Copy link
Contributor Author

@anGie44 anGie44 Mar 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh cannot be explained ..totally in the wrong place there! but I forgot i've wrestled with the TestAccAWSWafWebACL_Rules test quite some time ago and from what i've tried it's a case where if the test config steps were applied outside of the test env (i've most recently used the latest version of the provider w/terraform 0.14), the resource modifications occur without any error. And with the suggestion you've provided, I'm still seeing the same error :/ should i close this PR and revisit?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I'd love to get the rest of these PR changes in just so those refactorings are out of the way for the future. Up to you if you'd like to revert this small little bit so we can get those in or if you would prefer working on this section some more here. Feel free to grab me directly to chat more!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep let me keep the refactorings in and remove the attempted test fix 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...or may be not 😅 ..i think 62adf9a should correctly use the retry and the TestAccWafRule_webACL added for additional checking


return output, err
})

if err != nil {
return fmt.Errorf("Error deleting WAF Rule: %s", err)
if tfawserr.ErrCodeEquals(err, waf.ErrCodeNonexistentItemException) {
return nil
}
return fmt.Errorf("error deleting WAF Rule (%s): %w", d.Id(), err)
}

return nil
Expand All @@ -221,7 +244,7 @@ func updateWafRuleResource(id string, oldP, newP []interface{}, conn *waf.WAF) e
return conn.UpdateRule(req)
})
if err != nil {
return fmt.Errorf("Error Updating WAF Rule: %s", err)
return fmt.Errorf("error updating WAF Rule (%s): %w", id, err)
}

return nil
Expand Down
56 changes: 28 additions & 28 deletions aws/resource_aws_waf_rule_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,7 @@ func testAccPreCheckAWSWaf(t *testing.T) {
func testAccAWSWafRuleConfig(name string) string {
return fmt.Sprintf(`
resource "aws_waf_ipset" "ipset" {
name = "%s"
name = %[1]q

ip_set_descriptors {
type = "IPV4"
Expand All @@ -515,22 +515,22 @@ resource "aws_waf_ipset" "ipset" {

resource "aws_waf_rule" "wafrule" {
depends_on = [aws_waf_ipset.ipset]
name = "%s"
metric_name = "%s"
name = %[1]q
metric_name = %[1]q

predicates {
data_id = aws_waf_ipset.ipset.id
negated = false
type = "IPMatch"
}
}
`, name, name, name)
`, name)
}

func testAccAWSWafRuleConfigChangeName(name string) string {
return fmt.Sprintf(`
resource "aws_waf_ipset" "ipset" {
name = "%s"
name = %[1]q

ip_set_descriptors {
type = "IPV4"
Expand All @@ -540,22 +540,22 @@ resource "aws_waf_ipset" "ipset" {

resource "aws_waf_rule" "wafrule" {
depends_on = [aws_waf_ipset.ipset]
name = "%s"
metric_name = "%s"
name = %[1]q
metric_name = %[1]q

predicates {
data_id = aws_waf_ipset.ipset.id
negated = false
type = "IPMatch"
}
}
`, name, name, name)
`, name)
}

func testAccAWSWafRuleConfig_changePredicates(name string) string {
return fmt.Sprintf(`
resource "aws_waf_ipset" "ipset" {
name = "%s"
name = %[1]q

ip_set_descriptors {
type = "IPV4"
Expand All @@ -564,7 +564,7 @@ resource "aws_waf_ipset" "ipset" {
}

resource "aws_waf_byte_match_set" "set" {
name = "%s"
name = %[1]q

byte_match_tuples {
text_transformation = "NONE"
Expand All @@ -579,31 +579,31 @@ resource "aws_waf_byte_match_set" "set" {
}

resource "aws_waf_rule" "wafrule" {
name = "%s"
metric_name = "%s"
name = %[1]q
metric_name = %[1]q

predicates {
data_id = aws_waf_byte_match_set.set.id
negated = true
type = "ByteMatch"
}
}
`, name, name, name, name)
`, name)
}

func testAccAWSWafRuleConfig_noPredicates(name string) string {
return fmt.Sprintf(`
resource "aws_waf_rule" "wafrule" {
name = "%s"
metric_name = "%s"
name = %[1]q
metric_name = %[1]q
}
`, name, name)
`, name)
}

func testAccAWSWafRuleConfig_geoMatchSetPredicate(name string) string {
return fmt.Sprintf(`
resource "aws_waf_geo_match_set" "geo_match_set" {
name = "%s"
name = %[1]q

geo_match_constraint {
type = "Country"
Expand All @@ -612,22 +612,22 @@ resource "aws_waf_geo_match_set" "geo_match_set" {
}

resource "aws_waf_rule" "wafrule" {
name = "%s"
metric_name = "%s"
name = %[1]q
metric_name = %[1]q

predicates {
data_id = aws_waf_geo_match_set.geo_match_set.id
negated = true
type = "GeoMatch"
}
}
`, name, name, name)
`, name)
}

func testAccAWSWafRuleConfigTags1(rName, tag1Key, tag1Value string) string {
return fmt.Sprintf(`
resource "aws_waf_ipset" "ipset" {
name = "%s"
name = %[1]q

ip_set_descriptors {
type = "IPV4"
Expand All @@ -637,8 +637,8 @@ resource "aws_waf_ipset" "ipset" {

resource "aws_waf_rule" "wafrule" {
depends_on = [aws_waf_ipset.ipset]
name = "%s"
metric_name = "%s"
name = %[1]q
metric_name = %[1]q

predicates {
data_id = aws_waf_ipset.ipset.id
Expand All @@ -650,13 +650,13 @@ resource "aws_waf_rule" "wafrule" {
%q = %q
}
}
`, rName, rName, rName, tag1Key, tag1Value)
`, rName, tag1Key, tag1Value)
}

func testAccAWSWafRuleConfigTags2(rName, tag1Key, tag1Value, tag2Key, tag2Value string) string {
return fmt.Sprintf(`
resource "aws_waf_ipset" "ipset" {
name = "%s"
name = %[1]q

ip_set_descriptors {
type = "IPV4"
Expand All @@ -666,8 +666,8 @@ resource "aws_waf_ipset" "ipset" {

resource "aws_waf_rule" "wafrule" {
depends_on = [aws_waf_ipset.ipset]
name = "%s"
metric_name = "%s"
name = %[1]q
metric_name = %[1]q

predicates {
data_id = aws_waf_ipset.ipset.id
Expand All @@ -680,5 +680,5 @@ resource "aws_waf_rule" "wafrule" {
%q = %q
}
}
`, rName, rName, rName, tag1Key, tag1Value, tag2Key, tag2Value)
`, rName, tag1Key, tag1Value, tag2Key, tag2Value)
}
Loading