issue #4772 add arn to aws_waf_ipset #4784
Conversation
|
$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSWafIPSet_basic' |
saravanan30erd
force-pushed
the
issue-4772
branch
from
bac121a
to
d8fc2b6
Compare
bflad
added
enhancement
There was a problem hiding this comment.
Aside from the ARN regex being slightly off in the testing, this LGTM 🚀
5 tests passed (all tests)
=== RUN TestAccAWSWafIPSet_changeDescriptors
--- PASS: TestAccAWSWafIPSet_changeDescriptors (26.30s)
=== RUN TestAccAWSWafIPSet_noDescriptors
--- PASS: TestAccAWSWafIPSet_noDescriptors (30.09s)
=== RUN TestAccAWSWafIPSet_disappears
--- PASS: TestAccAWSWafIPSet_disappears (40.15s)
=== RUN TestAccAWSWafIPSet_changeNameForceNew
--- PASS: TestAccAWSWafIPSet_changeNameForceNew (41.18s)
=== RUN TestAccAWSWafIPSet_basic
--- PASS: TestAccAWSWafIPSet_basic (48.34s)
| @@ -33,6 +34,8 @@ func TestAccAWSWafIPSet_basic(t *testing.T) { | |||
| "aws_waf_ipset.ipset", "ip_set_descriptors.4037960608.type", "IPV4"), | |||
| resource.TestCheckResourceAttr( | |||
| "aws_waf_ipset.ipset", "ip_set_descriptors.4037960608.value", "192.0.7.0/24"), | |||
| resource.TestMatchResourceAttr("aws_waf_ipset.ipset", "arn", | |||
| regexp.MustCompile(`^arn:[\w-]+:([a-zA-Z0-9\-])+:([a-z]{2}-(gov-)?[a-z]+-\d{1})?:(\d{12})?:(.*)$`)), | |||
There was a problem hiding this comment.
This regex is pretty hard to read and has some issues:
- The service name should always be
waf - The region name should always exist
- The account ID should always exist
- The resource should check for
ipset/.+
At some point we'll probably create a helper function for ARN testing like this, but as for this case, I will fix these items post-merge with the simpler: ^arn:[\w-]+:waf:[^:]+:\d{12}:ipset/.+$.
There was a problem hiding this comment.
Actually double checking the AWS documentation here: https://docs.aws.amazon.com/general/latest/gr/aws-arns-and-namespaces.html#arn-syntax-waf
arn:aws:waf::123456789012:ipset/3f74bd8c-f046-4970-a1a7-41aa52e05480
Looks like region name should never be set. I'll adjust the ARN logic and regex here accordingly.
In the regional case, it will include the region, but that is a separate resource and test 👍
arn:aws:waf-regional:us-east-1:123456789012:ipset/3f74bd8c-f046-4970-a1a7-41aa52e05480
There was a problem hiding this comment.
Yeah, correct. Will I add arn attribute to aws_wafregional_ipset, is it useful?
There was a problem hiding this comment.
@saravanan30erd I'm sure someone will eventually be looking for it! 👍
|
This has been released in version 1.23.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
Fixes #4772
Changes proposed in this pull request:
added attribute arn to aws_waf_ipset