feat: limit access to S3 by VPC ID #1829
Conversation
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/187636617 The labels on this github issue will be updated when the story is started. |
zucchinidev
force-pushed
the
feat_limit_access_to_s3_by_vpc_id_187430076
branch
2 times, most recently
from
d595ff2 to
4616c3f
Compare
There was a problem hiding this comment.
The implementation of the feature is great. My main comment is that it would be really nice if the test could create the VPC endpoint that it needs. If a user needs to
- create a VPC endpoint
- set and environment variable
- run the test
- delete the VPC endpoint
Then this is just a barrier to running the tests. An automated test is by definition automated. Perhaps I'm missing something that makes this impossible.
| @@ -74,6 +74,22 @@ data "aws_iam_policy_document" "user_policy" { | |||
| format("%s/*", var.arn) | |||
| ] | |||
| } | |||
|
|
|||
| dynamic "statement" { | |||
| for_each = var.allowed_aws_vpc_id != "" ? [1] : [] | |||
There was a problem hiding this comment.
Is this better than using count which is commonly used in other *.tf files? I'm not asking for it to be changed, just curious.
There was a problem hiding this comment.
A dynamic block acts much like a for expression and the for_each argument provides the complex value to iterate over. This is a build-in expression that does not admit the count.
There was a problem hiding this comment.
Thanks. I missed that it was a dynamic.
* create a VPC endpoint * run the test * delete the VPC endpoint [#187430076](https://www.pivotaltracker.com/story/show/187430076)
zucchinidev
force-pushed
the
feat_limit_access_to_s3_by_vpc_id_187430076
branch
from
2464bf0 to
f7951c2
Compare
| @@ -2,7 +2,7 @@ package acceptance_tests_test | |||
|
|
|||
| import ( | |||
| "csbbrokerpakaws/acceptance-tests/helpers/apps" | |||
| "csbbrokerpakaws/acceptance-tests/helpers/dms" | |||
| "csbbrokerpakaws/acceptance-tests/helpers/awscli/dms" | |||
There was a problem hiding this comment.
I would have been inclined to leave the dms package as it was and just factor out one function into awscli, but I don't see any harm in moving it.
| func CreateEndpoint(allowedVPCID, defaultRegion string) string { | ||
|
|
||
| // Get the ARN of the current user | ||
| getCallerIdentityCommand := []string{ |
There was a problem hiding this comment.
Personally I prefer to inline these rather than have another variable (that you have to think of a name for), but this approach works too.
| By("deleting the file from bucket using the app") | ||
| app.DELETE(filename) | ||
|
|
||
| By("verifying that the file no longer exists") |
There was a problem hiding this comment.
It feels to be that this is testing something else that's beyond the VPC endpoint access restriction, but I don't think it necessary to remove it.
| @@ -74,6 +74,22 @@ data "aws_iam_policy_document" "user_policy" { | |||
| format("%s/*", var.arn) | |||
| ] | |||
| } | |||
|
|
|||
| dynamic "statement" { | |||
| for_each = var.allowed_aws_vpc_id != "" ? [1] : [] | |||
There was a problem hiding this comment.
Thanks. I missed that it was a dynamic.
#187430076
Checklist:
make run-integration-testsandmake run-terraform-tests?