-
Notifications
You must be signed in to change notification settings - Fork 9
configurable external access to buckets for cross-account #142
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
Changes from all commits
a4b12df
726b398
bdc39d8
2f303f8
1cc7f9f
9ee223f
1d8ce04
e5a2960
70d9777
ab784e0
760e0eb
b02c3f5
612b986
c38f5e7
7f2ebf0
c186cb5
ca597c9
b52f3a3
2298e19
45f5458
533a0dc
9b20575
fda10bd
aa2cf5f
921fe75
dc035b6
65ee588
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| resource "aws_s3_bucket_policy" "allow_crud_from_consolidation" { | ||
| for_each = var.consolidation_acct_id != null ? merge( | ||
| aws_s3_bucket.public-bucket, | ||
| aws_s3_bucket.standard-bucket, | ||
| aws_s3_bucket.protected-bucket, | ||
| aws_s3_bucket.workflow-bucket | ||
| ) : {} | ||
| bucket = each.key | ||
| policy = jsonencode({ | ||
| Version = "2012-10-17", | ||
| Statement = [ | ||
| { | ||
| Sid = "${each.key}-CrossAccountReadAccess", | ||
| Effect = "Allow" | ||
| Principal = { | ||
| AWS = local.consolidation_crud_roles | ||
| }, | ||
|
|
||
| Action = [ | ||
| "s3:GetObject", | ||
| "s3:GetObjectVersion", | ||
| "s3:ListBucket" | ||
| ], | ||
|
|
||
| Resource = [ | ||
| each.value.arn, | ||
| "${each.value.arn}/*" | ||
| ] | ||
| }, | ||
| { | ||
| Sid = "${each.key}-CrossAccountWriteAccess", | ||
| Effect = "Allow" | ||
| Principal = { | ||
| AWS = local.consolidation_crud_roles | ||
| }, | ||
|
|
||
| Action = [ | ||
| "s3:PutObject", | ||
| "s3:PutObjectAcl", | ||
| "s3:DeleteObject" | ||
| ], | ||
|
|
||
| Resource = [ | ||
| "${each.value.arn}/*" | ||
| ] | ||
| }, | ||
| ] | ||
| }) | ||
| } |
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing trailing new line on file
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing newline at end of file |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,3 +67,21 @@ variable "s3_replicator_target_prefix" { | |
| default = null | ||
| description = "Prefix that the S3 replicator will write logs to in the target bucket." | ||
| } | ||
|
|
||
| variable "consolidation_acct_id" { | ||
| type = string | ||
| description = "account id of relevant cumulus consolidation stack" | ||
| default = null | ||
| } | ||
|
|
||
| variable "consolidation_deploy_name" { | ||
| type = string | ||
| description = "deploy_name of relevant consolidation stack" | ||
| default = "willow" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is willow?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think id be more in favor of not having a default here. Forgetting to set this resulting in stuff prefixed
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, willow is currently the prefix of the consolidation stack(s)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well if thats the prefix and its sticking around :D |
||
| } | ||
|
|
||
| variable "consolidation_maturity" { | ||
| type = string | ||
| description = "maturity of relevant consolidation stack" | ||
| default = null | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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 think it would structurally be better to instead of manually merging the old distribution policy into your new one is to keep the iam_policy_document approach and rely on the policy document merging you can use https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document#example-of-merging-source-documents
This makes it a bit more composable for the users in my opinion and a bit cleaner as far as extending policies in the future.
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.
good idea, that looks much cleaner