New Resource: aws_wafregional_web_acl_association #3755
Conversation
radeksimko
added
service/waf
There was a problem hiding this comment.
Thanks for this PR too, I left you some initial comments and I assume the branch will need rebasing.
If I understand correctly, the relationship between Web ACL & ALB is 1-to-N (1 Web ACL can be assigned to N ALBs). Do you mind exercising this in an acceptance test too? i.e. add a test which assigns a single ACL to two different ALBs?
| _, err = conn.AssociateWebACL(params) | ||
| if err != nil { | ||
| if awsErr, ok := err.(awserr.Error); ok { | ||
| if awsErr.Code() == "WAFUnavailableEntityException" { |
There was a problem hiding this comment.
Do you mind replacing this with a helper?
if isAWSErr(err, wafregional.ErrCodeWAFUnavailableEntityException, "") {| } | ||
| if !found { | ||
| // It seems it doesn't exist anymore, so clear the ID | ||
| d.SetId("") |
There was a problem hiding this comment.
It's a convention to log a WARN message in these cases, before we wipe the resource from state. Do you mind adding it?
|
|
||
| func resourceAwsWafRegionalWebAclAssociationUpdate(d *schema.ResourceData, meta interface{}) error { | ||
| return resourceAwsWafRegionalWebAclAssociationRead(d, meta) | ||
| } |
There was a problem hiding this comment.
If update isn't actually implemented I think we should just mark web_acl_id as ForceNew too and remove the Update altogether, otherwise we'd pretend that it's being updated when it's not.
| continue | ||
| } | ||
|
|
||
| web_acl_id, resource_arn := resourceAwsWafRegionalWebAclAssociationParseId(rs.Primary.ID) |
There was a problem hiding this comment.
Nitpick: it is a convention in Go generally to follow camelCase instead of under_score for variables.
| type = "COUNT" | ||
| } | ||
| priority = 100 | ||
| rule_id = "${aws_wafregional_rule.foo.id}" |
There was a problem hiding this comment.
Nitpick: Do you mind fixing the indentation here?
| } | ||
|
|
||
| resource "aws_wafregional_web_acl_association" "foo" { | ||
| depends_on = ["aws_alb.foo", "aws_wafregional_web_acl.foo"] |
There was a problem hiding this comment.
The depends_on is redundant here as the relationship is already defined by the references below. Do you mind removing it?
| resource "aws_wafregional_ipset" "ipset" { | ||
| name = "tfIPSet" | ||
|
|
||
| ip_set_descriptors { |
There was a problem hiding this comment.
I think this should be ip_set_descriptor (singular)
| name = "tfWAFRule" | ||
| metric_name = "tfWAFRule" | ||
|
|
||
| predicates { |
There was a problem hiding this comment.
I think this should be predicate (singular)
| type = "ALLOW" | ||
| } | ||
|
|
||
| rules { |
There was a problem hiding this comment.
I think this should be rule (singular)
| } | ||
|
|
||
| resource "aws_alb" "alb" { | ||
| subnets = ["${aws_subnet.foo.id}", "${aws_subnet.bar.id}"] |
There was a problem hiding this comment.
We'll either need to make this ALB internal, so it doesn't need IGW, or add an IGW to the VPC.
Also we'll need to use aws_availability_zones data source to ensure that each subnet is created in a different AZ, otherwise we might end up with both subnets in the same AZ, which will effectively cause the ALB creation to fail (it requires subnets in two different AZs).
radeksimko
added
the
waiting-response
pvanbuijtene
force-pushed
the
wafregional-web-acl-association
branch
from
d51f274 to
b74a955
Compare
|
@radeksimko the finish is near ;) ... ready for review |
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
Trying to help a bit to finish the WAF Regional PRs, this is the result of the split up of #3199
Related PRs: #1047 hashicorp/terraform#13712
Thanks to the contributors: @yusukegoto @DennyLoko @BSick7