Skip to content
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

Set appropriate AWS ARN for S3 bucket policies in GovCloud #39

Merged
merged 1 commit into from
Mar 26, 2020
Merged

Set appropriate AWS ARN for S3 bucket policies in GovCloud #39

merged 1 commit into from
Mar 26, 2020

Conversation

ericamador
Copy link
Contributor

What

  • Parameterize the ARN prefix used for S3 bucket policies such that if we are deploying in an AWS GovCloud region, the ARN prefix is arn:aws-us-gov:s3. Otherwise, the ARN prefix is arn:aws:s3.

Why

  • These changes allow successful deployment in GovCloud (tested in us-gov-west-1). Prior to this change, attempting to deploy in GovCloud would fail with:
Error: Error putting S3 policy: MalformedPolicy: Policy has invalid resource status code: 400

References

@gloterman
Copy link

Hi @ericamador, I've encountered the same issue with the ARN prefix.
I made also a PR #41 .
Maybe we can discuss with the maintainers for a global solution (manage the GOV and China ARN). ?

Regards,

@ericamador
Copy link
Contributor Author

@gloterman Thanks for reaching out! If it sounds good to the maintainers, I'll update my PR to move s3_arn_prefix out of locals and make it default to arn:aws:s3, with documentation that states if you're deploying in GovCloud/China, you need to update the value of the variable accordingly.

@osterman
Copy link
Member

osterman commented Mar 9, 2020

Another idea might be to define an arn_format variable?

e.g.

variable "arn_format" {
  default = "arn:aws:s3:::%s/*"
}

Then define the bucket_arn like this:

local bucket_arn = format(arn_format, local.bucket_name)

@osterman
Copy link
Member

osterman commented Mar 9, 2020

This will allow callers to define any format for the arn required by their region (E.g. china or govcloud)

@ericamador
Copy link
Contributor Author

Sounds good. I'll work on an update to this PR this evening. Thanks for the feedback.

@ericamador
Copy link
Contributor Author

@osterman Sorry for the delay. I've made the requested changes. Let me know what you think.

@RothAndrew
Copy link
Sponsor Contributor

+1. This will be the perfect solution for me as well.

@osterman osterman requested a review from maximmi March 26, 2020 17:31
@maximmi
Copy link
Contributor

maximmi commented Mar 26, 2020

/codefresh run test

@maximmi
Copy link
Contributor

maximmi commented Mar 26, 2020

/rebuild-readme

@maximmi
Copy link
Contributor

maximmi commented Mar 26, 2020

@ericamador
Looks good, but please rebuild README by executing these commands:

make init
make readme/deps
make readme

It will add the new variables and outputs to README.md automatically.
In general, any changes to README should be made in README.yaml (not in this case), and after that executing the commands above will rebuild README.yaml into README.md and add all new variables and outputs to README.md

or you can open repository, so bot can do it for you, right now there is no permissions to update your fork

thanks

This change allows a user to override the AWS ARN (which to defaults to
the most common use case of "arn:aws") in order to allow one to use this
module in the AWS GovCloud/China regions. This can be done by setting
the value of the arn_format variable to "arn:aws-us-gov"/"arn:aws-cn"
respectively.
@ericamador
Copy link
Contributor Author

@maximmi Thanks, I've made those requested changed to build the docs and have force pushed over my branch.

@maximmi
Copy link
Contributor

maximmi commented Mar 26, 2020

/codefresh run test

Copy link
Contributor

@maximmi maximmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @ericamador

@maximmi maximmi merged commit 7a0f82a into cloudposse:master Mar 26, 2020
@maximmi
Copy link
Contributor

maximmi commented Mar 26, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants