configurable external access to buckets for cross-account #142
Conversation
|
This may be a good use case for S3 Access Points. We could create Access points under Consolidated control and then the DAAC side update would simply be to delegate the necessary control to the access point. See https://docs.aws.amazon.com/AmazonS3/latest/userguide/access-points-policies.html#access-points-delegating-control |
|
ok, so the same basic shape of: add to each relevant bucket a bucket policy, but using this data access point conditional instead of specifying a principal |
There was a problem hiding this comment.
missing trailing new line on file
| variable "consolidation_deploy_name" { | ||
| type = string | ||
| description = "deploy_name of relevant consolidation stack" | ||
| default = "willow" |
There was a problem hiding this comment.
I think id be more in favor of not having a default here. Forgetting to set this resulting in stuff prefixed willow is unintuitive for users of CIRRUS. Probably better to let the TF fail if nothing is provided?
There was a problem hiding this comment.
sure, willow is currently the prefix of the consolidation stack(s)
which got named because I had to stand one up before people had made any decisions
but I can make it non-defaulted
There was a problem hiding this comment.
Well if thats the prefix and its sticking around :D
There was a problem hiding this comment.
missing newline at end of file
daac/policy.tf
Outdated
There was a problem hiding this comment.
Sorry to be nitpicky here. There is a big mix of allow and Allow I think they should all be Allow in the Effect of the policy.
mattp0
left a comment
mattp0
left a comment
There was a problem hiding this comment.
This looks good, I approved the tf lint to run. I had some what I would say is nitpicks but otherwise thanks for contributing!
Co-authored-by: Matt Perry <mperry37@alaska.edu>
Co-authored-by: Matt Perry <mperry37@alaska.edu>
Co-authored-by: Matt Perry <mperry37@alaska.edu>
Co-authored-by: Matt Perry <mperry37@alaska.edu>
|
ok, we went down a bit of a rabbit hole on access points and also getting some "plays well with others" parsing right, but it's ready for re-review now. I believe everything previously present has been fixed and there's been a bit of cleanup generally |
There was a problem hiding this comment.
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.
good idea, that looks much cleaner
|
hey Matt, I found an issue with how I was handling the oai's case, and fixed that, I thought I'd tested for it, but hadn't effectively |
3 participants
configuration for cross-account permission to s3 buckets.
configured so that, if nothing is changed, nothing will change.
but a deployment can declare a consolidation_account_id (to not null) and it will add to
standard-buckets
protected-buckets
public-buckets
and workflow-buckets
a permission policy that allows read and write access to the declared account('s stack)
additionally deploy_name and maturity can be declared or left default, our current best guess of what those values will be