-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
provider/aws: Support multiple subnets in Network ACL #1931
Conversation
just tested this out and it works well 👍 |
}, | ||
}) | ||
|
||
} |
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.
Can you add another step to this test to exercise the update functionality?
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 can, but the test doesn't pass ATM b/c plan
isn't empty (sorting issue mentioned above)
Two nits - LGTM generally! |
@phinze applied feedback, thanks! |
Ah yes - this is a great candidate for "subnet_ids": schema.Schema{
Type: TypeSet,
Elem: TypeString,
Set: func(v interface{}) int {
return hashcode.String(v.(string))
}
} |
@phinze updated tests, take a look when you can |
Solid solid - LGTM! |
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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Ref #1717
The AWS API allows Network ACLs to be applied to multiple subnets, but Terraform is currently limited to 1-to-1. With this PR we introduce
subnet_ids
and deprecatesubnet_id
, so you can a single ACL to multiple subnets.Subnets in a VPC must be assigned an ACL, so, if we remove a subnet from this ACL, that subnet gets associated to the Default ACL. This needs to be documented because it could have weird behavior.
Usage:
State generated: (things omitted)
Needs:
sort.Strings
here, but the sorting is still not correct. This may be because I didn't sort them in the config(?). Not sure what to do about that.. ~~ fixed by making it aTypeSet
fromTypeList