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.

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

resource/wafv2_web_acl: fix rule expansion at update time and refactor shared schemas #14073

Closed
wants to merge 1 commit into from

Conversation

anGie44
Copy link
Contributor

@anGie44 anGie44 commented Jul 7, 2020

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #14035
Closes #14029
Closes #14326
Affected by #14062

Notes

  • PR for brainstorming pros/cons
  • The expandWafv2Rules() originally called at Update was missing references to statement types (unlike the expandWafv2WebACLRules() used at Create) including managed_rule_group, rate_based, etc. resulting in empty Statement fields in UpdateWebACL requests (as seen in debug logs in the referenced issues above). As AWS documents the Rule object as shared by both the WebACL and Rule Group resources, expansion/flatten methods affecting rules and their statements have been refactored in an effort to conform to only 1 representation of a WAFv2 Rule across resources; majority of these methods in wafv2_web_acl migrated to wafv2_helper with naming conventions generalized to account for rule group and web acl resources.
  • Expected side-effect: the slowdown documented in Significant slowdowns running terraform for WAF resources on AWS provider v2.69.0 #14062 to affect the rule_group resource as it shares the WAFv2 rule representation with an additional statement level for and/or/not and rate_based_statements since v2.69.0 of the provider

Release note for CHANGELOG:

resource/wafv2_web_acl: fix rule expansion at update time

Output from acceptance testing:

--- PASS: TestAccAwsWafv2RuleGroup_Disappears (2519.06s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_basic (3481.61s)
--- PASS: TestAccAwsWafv2WebACLAssociation_Basic (3488.78s)
--- PASS: TestAccAwsWafv2RuleGroup_Minimal (3653.83s)
--- PASS: TestAccAwsWafv2RuleGroup_RegexPatternSetReferenceStatement (4238.47s)
--- PASS: TestAccAwsWafv2RuleGroup_IpSetReferenceStatement (4254.06s)
--- PASS: TestAccAwsWafv2WebACLAssociation_Disappears (6427.51s)
--- PASS: TestAccAwsWafv2RuleGroup_XssMatchStatement (625.52s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_webACLDisappears (2963.60s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_disappears (3633.42s)
--- PASS: TestAccAwsWafv2RuleGroup_ChangeMetricNameForceNew (7717.19s)
--- PASS: TestAccAwsWafv2RuleGroup_ChangeCapacityForceNew (7726.00s)
--- PASS: TestAccAwsWafv2RuleGroup_ChangeNameForceNew (7732.12s)
--- PASS: TestAccAwsWafv2RuleGroup_ByteMatchStatement (9307.55s)
--- PASS: TestAccAwsWafv2RuleGroup_SqliMatchStatement (9315.38s)
--- PASS: TestAccAwsWafv2RuleGroup_GeoMatchStatement (9316.32s)
--- PASS: TestAccAwsWafv2RuleGroup_SizeConstraintStatement (9319.28s)
--- PASS: TestAccAwsWafv2WebACL_Disappears (2410.79s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_changeLogDestinationConfigsForceNew (6073.56s)
--- PASS: TestAccAwsWafv2RuleGroup_Basic (9905.40s)
--- PASS: TestAccAwsWafv2WebACL_Minimal (3333.59s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_changeResourceARNForceNew (7316.90s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_update (8533.70s)
--- PASS: TestAccAwsWafv2RuleGroup_Tags (11266.88s)
--- PASS: TestAccAwsWafv2WebACL_MaxNestedRateBasedStatements (3108.54s)
--- PASS: TestAccAwsWafv2WebACL_MaxNestedOperatorStatements (3129.30s)
--- PASS: TestAccAwsWafv2WebACL_ChangeNameForceNew (6261.47s)
--- PASS: TestAccAwsWafv2WebACL_Basic (8474.62s)
--- PASS: TestAccAwsWafv2RuleGroup_RuleAction (13141.96s)
--- PASS: TestAccAwsWafv2RuleGroup_LogicalRuleStatements (13250.12s)
--- PASS: TestAccAwsWafv2WebACL_ManagedRuleGroupStatement (6395.75s)
--- PASS: TestAccAwsWafv2WebACL_RateBasedStatement (6050.93s)
--- PASS: TestAccAwsWafv2WebACL_updateDefaultAction (4706.04s)
--- PASS: TestAccAwsWafv2WebACL_updateVisibilityConfig (4707.76s)
--- PASS: TestAccAwsWafv2WebACL_Tags (6462.19s)
--- PASS: TestAccAwsWafv2WebACL_RuleGroupReferenceStatement (7834.00s)
--- PASS: TestAccAwsWafv2RuleGroup_ByteMatchStatement_FieldToMatch (16714.49s)... 

@ghost ghost added size/XXL Managed by automation to categorize the size of a PR. service/wafv2 Issues and PRs that pertain to the wafv2 service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Jul 7, 2020
@anGie44 anGie44 force-pushed the b-missing-update-fields branch 2 times, most recently from e12b07f to a366abb Compare July 9, 2020 17:42
@anGie44 anGie44 marked this pull request as ready for review July 9, 2020 17:46
@anGie44 anGie44 requested a review from a team July 9, 2020 17:46
@ghost ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/XXL Managed by automation to categorize the size of a PR. labels Jul 10, 2020
@mohsen0
Copy link

mohsen0 commented Jul 28, 2020

Can this get reviewed? the slowness is very painful especially when there are a lots of rules

@anGie44
Copy link
Contributor Author

anGie44 commented Jul 28, 2020

Hi @mohsen0 👋 , at the moment, this and related WAFv2 issues will be prioritized after the major release of v3.0.0. Note though that the work here is still affected by the slowness originally reported in #14062.

@anGie44
Copy link
Contributor Author

anGie44 commented Aug 13, 2020

Note: this potentially can be closed after merge of proposed change in #14616

@anGie44 anGie44 closed this Aug 24, 2020
@anGie44 anGie44 deleted the b-missing-update-fields branch August 24, 2020 17:27
@ghost
Copy link

ghost commented Sep 24, 2020

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 ghost locked and limited conversation to collaborators Sep 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
service/wafv2 Issues and PRs that pertain to the wafv2 service. size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
2 participants