refactor(GuLogShippingPolicy)!: make GuLogShippingPolicy a singleton #337
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This parameter is written as a singleton as we only ever want one per stack - if there are multiple apps defined in the stack, they'll all share the same parameter (as opposed to the AMI parameter which is app specific).
…ter` parameter Also extend `GuAllowPolicy` to stay DRY.
This policy is now a singleton as we only ever want one per stack. If there are multiple roles defined in this stack that require log shipping permissions, a single policy will be defined and attached to the roles. This DRYness helps keep within the file size limits of cloudformation.
akash1810
commented
Mar 19, 2021
| @@ -4,10 +4,10 @@ import type { Stack } from "@aws-cdk/core"; | |||
| import { Tags } from "@aws-cdk/core"; | |||
|
|
|||
| // IAM Policies need to be attached to a role, group or user to be created in a stack | |||
| export const attachPolicyToTestRole = (stack: Stack, policy: Policy, app: string = "testing"): void => { | |||
| const role = new Role(stack, "TestRole", { | |||
| export const attachPolicyToTestRole = (stack: Stack, policy: Policy, id: string = "TestRole"): void => { | |||
There was a problem hiding this comment.
The default value of the app param was never overridden, so (safely) removed it.
Comment on lines
+16
to
+18
| Type: "AWS::SSM::Parameter::Value<String>", | ||
| Default: "/account/services/logging.stream.name", | ||
| Description: "SSM parameter containing the Name (not ARN) on the kinesis stream", |
stephengeller
approved these changes
Mar 19, 2021
|
🎉 This PR is included in version 4.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This was referenced Mar 19, 2021
akash1810
added a commit
that referenced
this pull request
Mar 24, 2021
Following #337 and part of a bigger bit of work in #332, this makes `GuDistributionBucketParameter` a singleton as we only ever want one of them in a stack. BREAKING CHANGE: * The removal of the static `parameterName` field means `GuDistributionBucketParameter.parameterName` becomes `GuDistributionBucketParameter.getInstance(stack).id`
akash1810
added a commit
that referenced
this pull request
Mar 24, 2021
A `GuInstanceRole` provides the basic permissions needed by EC2, it grants access to: * SSM * Describing tags * Writing to kinesis for log shipping #326 started the work to support the declaration of multiple apps in a stack, where an app is a collection of ASG, LB etc. However, when actually attempting to use the updated constructs in reality, the synth step failed due to multiple constructs using the same ID. This change follows #341 and #337 and converts `GuSSMRunCommandPolicy` and `GuDescribeEC2Policy` to singletons. This means these policies will only get created once regardless of how many roles use them - there will be one policy with multiple roles attached to it. This helps keep the stack DRY and also reduces the chance of reaching the max file size limits of cloudformation. The commits have been crafted to be standalone, reviewing them one by one might make the overall review easier. Requires #341. BREAKING CHANGE: * `GuSSMRunCommandPolicy` and `GuDescribeEC2Policy` can no longer be directly instantiated
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this change?
This is ultimately part of #332.
This policy is now a singleton as we only ever want one per stack. If there are multiple roles defined in this stack that require log shipping permissions, a single policy will be defined and attached to the roles.
This DRYness helps keep within the file size limits of cloudformation too.
Also included in this change is a new
GuLoggingStreamNameParameter, also written as a singleton for the same reasons.To make the review easier, each commit can be read in sequence as they have been crafted to tell the story.
Usage hasn't really changed as this policy will never really be used by consumer stacks as they'l be using
GuInstanceRoleinstead, which adds this policy by default. However, usage looks roughly like this:It's worth noting that #92 was a naive implementation of singleton parameters and this new style removes the need to have the
getParamhelper function on theGuStack.BREAKING CHANGE:
GuLogShippingPolicyhas changed. It is now a singleton and direct instantiation is not possible.Does this change require changes to existing projects or CDK CLI?
Yes. They'll need a version bump by as
GuLogShippingPolicyis provided through theGuInstanceRole, this should be a no-op.How to test
See updated and new tests. Either by running them or observing CI.
How can we measure success?
A much simpler API and ground work for supporting multiple apps (for example a backend api and frontend web app microservice) within a single stack.
Have we considered potential risks?
n/a