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

replace hardcoded aws partition in ARNs with ${AWS::Partition} #606

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

@andreaswittig
Copy link
Contributor

andreaswittig commented Dec 20, 2021

@PatMyron Thanks a lot for your contribution. I do have two questions.

  1. What's the motivation for this change? I understand, that cfn-lint advises to use the pseudo-parameter for the AWS partition. Is that the only reason for this change? Or do you plan to deploy the templates in China or GovCloud?
  2. Is there a reason to keep the hardcoded partitions for s3.yaml and cloudtrail.yaml?

My only concern is, that we don't have the capability to run our test suite in China/GovCloud. I expect, that some templates break in these environments. For example, due to missing or limited features/services. What do you think about that? @PatMyron @michaelwittig?

@PatMyron
Copy link
Author

PatMyron commented Dec 20, 2021

do you plan to deploy the templates in China or GovCloud?

Not personally.. for others to run into fewer issues deploying into other partitions

we don't have the capability to run our test suite in China/GovCloud. I expect, that some templates break in these environments

I also no longer have access to those partitions, so I can't test there either. Agree there are likely other issues not yet addressed, but this should be a decent first step

Is there a reason to keep the hardcoded partitions for s3.yaml and cloudtrail.yaml?

state/s3.yaml seems okay anyways since it looks region-specific already:

- !If [HasPartitionPublic, 'arn:aws:iam::507241528517:root', !Ref 'AWS::NoValue'] # sa-east-1
- !If [HasPartitionUsGov, 'arn:aws-us-gov:iam::048591011584:root', !Ref 'AWS::NoValue'] # us-gov-west-1*
- !If [HasPartitionUsGov, 'arn:aws-us-gov:iam::190560391635:root', !Ref 'AWS::NoValue'] # us-gov-east-1*
- !If [HasPartitionChina, 'arn:aws-cn:iam::638102146993:root', !Ref 'AWS::NoValue'] # cn-north-1*
- !If [HasPartitionChina, 'arn:aws-cn:iam::037604701340:root', !Ref 'AWS::NoValue'] # cn-northwest-1*

security/cloudtrail.yaml just also needed Sub so I just pushed that too:

EventSelectors: !If [IsS3DataEvents, [{DataResources: [{Type: 'AWS::S3::Object', Values: [!Sub 'arn:${AWS::Partition}:s3:::']}], IncludeManagementEvents: true, ReadWriteType: All}], !Ref 'AWS::NoValue']

EventSelectors: !If [IsS3DataEvents, [{DataResources: [{Type: 'AWS::S3::Object', Values: [!Sub 'arn:${AWS::Partition}:s3:::']}], IncludeManagementEvents: true, ReadWriteType: All}], !Ref 'AWS::NoValue']

@michaelwittig
Copy link
Contributor

michaelwittig commented Dec 21, 2021

The reason why I never worked on #191 is that CloudFormation (sometimes/always?) believes that an attribute changes even if the resulting !Sub has the same value as the String before. So we might replace resources because of that. What we would need to do is this:

  1. Launch stack using old template.
  2. Update using new template and see what happens.

I also agree that we have no way to test the templates in other partitions which is a problem.

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.

None yet

3 participants