Improvements to the SAR app template

[benbridts]

This improves the SAR Application template, I used three commits to make review easier and allow cherry-picking if that would be preferred:

Improve SAR Application template (These are things I discovered by running cfn-lint -e -cI):

• replaces every arn:aws: with arn:${AWS::Partition} (should help with Basic GovCloud Support #332 )
• replaces !Join ['', ...] with !Sub
• added UpdateReplacePolicy if there was a DeletionPolicy
• changed the type of properties where it didn't match the specification
• removed an unused !Sub

Remove invalid permissions: s3:Get (without a wildcard) doesn't give any permissions

Use references where possible:
There were still some hardcoded values in the template. I removed those where it was a relatively small change. This should make #342 possible, and make it safer to change names of resources the future.
There are some hardcoded values I didn't change:

• If the resources are created outside of this template
• project/aws-deployment-framework-base-templates, because removing that circle would require converting a managed policy to an inline policy, which is a bigger change

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[sbkok]

Thanks for cleaning up the template!
I've added a few comments that I think should be improved before we can merge this one.

[sbkok]

Personally I don't like relying on a to-be-generated transform resource.
This makes reviewing the code very hard to follow.
If this is the only way to break-out of the circular reference, I would rather move the other policies away from the lambda function into a separate role definition as well.

Plus I think this would need to be a !Ref instead of a !GetAtt.

[benbridts]

I'll keep the hardcoded name for now, making it a dependency can be done in a different PR

[benbridts]

Thanks for the review!

I'm not sure how I made those mistakes with !Ref and !GetAtt, I had the docs open to check the return values, but apparently still mistyped some things.

I'll clean these up, and will have another think about the transformed resource / move that one to a different PR where we can look at defining the Lambda Role in the template instead of relying on the transform.

[benbridts]

@sbkok I think I fixed everything, I also made a quick overview of the changes that actually reduce permissions:

• L439-440: arn:aws:s3:::serverlessrepo-aws-deplo-bootstraptemplatesbucket-* replaced with BootstrapTemplatesBucket.Arn
• L443-444: arn:aws:s3:::adf-shared-modules-*-* replace with arn:${AWS::Partition}:s3:::${SharedModulesBucket.BucketName}
• L833: arn:aws:s3:::adf-shared-modules-* replaced with arn:${AWS::Partition}:s3:::adf-shared-modules-${DeploymentAccountMainRegion}-*

Edit: I have to double-check why I did something different for L443 and L833
Edit2: Those differences are correct. L833 is the Lambda Function that creates the bucket, so the name isn't known yet (but determined by BucketNamePrefix of SharedModulesBucket). L443 uses that created bucket, so it can be more specific

[sbkok]

I have updated this PR to reflect the latest changes from our main branch + updated some of the issues that were flagged before but not resolved yet.

It looks good to me to merge. Thanks for contributing.

[StewartW]

Looks great. Really tidies up the template 👍