feat!: Tag support for a stack with multiple apps #326

akash1810 · 2021-03-16T22:43:58Z

What does this change?

Currently, as noted by this comment, our constructs are tailored for single app stacks. We don't support a stack that has, for example, a frontend web app and an API app. If we were to create two ASGs in the same stack, they'd both get incorrectly tagged as the same "app". That is, we don't support micro-services very well.

This change moves the app string into an interface, this interface is then extended by any construct's props if they need to be app-aware.

As "app" is not in GuStack and we want to continue adding the "app" tag to resources, we're having to call Tags.of(this).add("App", props.app); a few more times, maybe there's a better solution here that keeps things DRY?

Lastly, this is quite a big diff - a lot of the changes are within test files. I've tried to make each commit standalone, so it might be easier to review commit by commit?

BREAKING CHANGE:

GuStack no longer takes knows about an "app"
Constructors for GuAutoScalingGroup, GuUserData, GuParameterStoreReadPolicy, GuGetDistributablePolicy and GuInstanceRole now require props
Props for GuLambdaFunction, GuDatabaseInstance, GuInstanceTypeParameter and GuAmiParameter now take an "app" prop

Does this change require changes to existing projects or CDK CLI?

Yes! There are numerous breaking changes in this PR!

How to test

See updated tests, aka observe CI.

How can we measure success?

It is possible to have a stack with multiple apps that are tagged correctly.

Have we considered potential risks?

akash1810 · 2021-03-16T22:54:40Z

PR to fix the build - #327.

jacobwinch · 2021-03-17T08:19:50Z

src/constructs/iam/policies/s3-get-object.ts

-  constructor(scope: GuStack, id: string = "GetDistributablePolicy", props?: GuNoStatementsPolicyProps) {
-    const path = [scope.stack, scope.stage, scope.app, "*"].join("/");
-    super(scope, id, { ...props, bucketName: new GuDistributionBucketParameter(scope).valueAsString, paths: [path] });
+  // eslint-disable-next-line custom-rules/valid-constructors -- TODO be better


Yeah, I'm not sure if this is the best change as it means the constructor for the constructs take different forms and goes against this decision slightly.

jacobwinch

Great work 💯

👍 once you've rebased / merged in #327!

jacobwinch · 2021-03-17T08:29:31Z

I've tried to make each commit standalone, so it might be easier to review commit by commit?

Thanks for this suggestion, it was helpful!

jacobwinch · 2021-03-17T14:14:59Z

src/constructs/iam/roles/instance-role.ts

  withoutLogShipping?: boolean; // optional to have log shipping added by default, you have to opt out
  additionalPolicies?: GuPolicy[];
 }

 export class GuInstanceRole extends GuRole {
  private policies: GuPolicy[];

-  constructor(scope: GuStack, id: string = "InstanceRole", props?: GuInstanceRoleProps) {
-    super(scope, id, {
+  // eslint-disable-next-line custom-rules/valid-constructors -- TODO be better


As we are starting to regularly break this convention perhaps we should consider relaxing the linting rule?

Definitely! I started on this a couple of weeks ago, but looks like I deleted my branch! 😱

I think the rules should be:

1, 2 or 3 arguments

1st argument is always GuStack

If only 2 arguments, 2nd argument is required and are props w/no default

If 3 arguments, 2nd is ID w/no default, 3rd are props w/no default and optional

Will raise this in a separate PR to see how it pans out.

Agree with the rules stated above!

#352 revises the linting rules.

akash1810 · 2021-03-17T14:15:29Z

src/constructs/core/parameters/ec2.ts

+    super(scope, AppIdentity.suffixText(props, "InstanceType"), {
      type: "String",
-      description: "EC2 Instance Type",
+      description: `EC2 Instance Type for the app ${props.app}`,


For the app "janus", the ID/name of this parameter becomes "InstanceTypeJanus". Hopefully the description is helpful?

jacobwinch · 2021-03-17T14:16:43Z

src/constructs/core/identity.ts

+  },
+  addTag<T extends IConstruct>(appIdentity: AppIdentity, construct: T): T {
+    Tags.of(construct).add("App", appIdentity.app);
+    return construct;


I find this return type a little surprising for a function called addTag

Agreed. It's mainly out of laziness as we can do this:

const snsTopic = props.existingSnsTopic?.externalTopicName ? Topic.fromTopicArn( scope, topicId, `arn:aws:sns:${scope.region}:${scope.account}:${props.existingSnsTopic.externalTopicName}` ) : AppIdentity.addTag( props, new GuSnsTopic(scope, topicId, { overrideId: !!props.existingSnsTopic?.logicalIdFromCloudFormation, }) );

The alternative is:

const newTopic = () => { const topic = new GuSnsTopic(scope, topicId, { overrideId: !!props.existingSnsTopic?.logicalIdFromCloudFormation, }); AppIdentity.addTag(props, topic); return topic; }; const snsTopic = props.existingSnsTopic?.externalTopicName ? Topic.fromTopicArn( scope, topicId, `arn:aws:sns:${scope.region}:${scope.account}:${props.existingSnsTopic.externalTopicName}` ) : newTopic();

Maybe withTag is a better name?

Ah yes, I see the benefit now. We could go for withTag or taggedResource, but it's not a big deal so feel free to ignore this!

Renamed to taggedConstruct to match the name of the parameter.

…` for clarity

github-actions · 2021-03-17T15:02:31Z

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Looks like some @guardian/cdk constructs are not applying the App tag. I suspect since guardian/cdk#326. Until that is fixed, we can safely, manually apply it to all constructs in tree from `this` as it's a single app stack.

akash1810 requested a review from a team March 16, 2021 22:43

jacobwinch reviewed Mar 17, 2021

View reviewed changes

jacobwinch approved these changes Mar 17, 2021

View reviewed changes

refactor: split identity into App and Stack+Stage identities

61ab536

akash1810 force-pushed the aa-app-tag branch from e3a59d9 to 63e4322 Compare March 17, 2021 08:58

akash1810 added 2 commits March 17, 2021 10:12

feat: add helper function to suffix a string with an AppIdentity

b23ef7a

feat: add helper function to tag an IConstruct with an AppIdentity

d44c5e4

akash1810 force-pushed the aa-app-tag branch from 63e4322 to c9bb938 Compare March 17, 2021 12:36

akash1810 marked this pull request as draft March 17, 2021 13:30

akash1810 added 2 commits March 17, 2021 14:02

refactor: remove App from GuStack to support a multi-app stack

3cc534f

fix(tests): update tests in reaction to GuStack not knowing about "app"

e0ce458

akash1810 force-pushed the aa-app-tag branch from c9bb938 to e0ce458 Compare March 17, 2021 14:02

akash1810 marked this pull request as ready for review March 17, 2021 14:04

jacobwinch reviewed Mar 17, 2021

View reviewed changes

akash1810 commented Mar 17, 2021

View reviewed changes

jacobwinch reviewed Mar 17, 2021

View reviewed changes

jacobwinch approved these changes Mar 17, 2021

View reviewed changes

refactor: rename AppIdentity.addTag to `AppIdentity.taggedConstruct…

8cc1a95

…` for clarity

akash1810 merged commit 1bda3c1 into main Mar 17, 2021

akash1810 deleted the aa-app-tag branch March 17, 2021 15:00

github-actions bot added the released label Mar 17, 2021

jacobwinch mentioned this pull request Mar 18, 2021

feat: Create GuSSMParameter using AWS Custom Resources #336

Merged

This was referenced Mar 19, 2021

feat: support multiple GuInstanceRoles in a stack #332

Merged

chore(deps): upgrade @guardian/cdk to latest (8.2.0) guardian/prism#155

Merged

akash1810 mentioned this pull request Apr 1, 2021

feat: allow GuSecurityGroup to be specified at the app level #378

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Tag support for a stack with multiple apps #326

feat!: Tag support for a stack with multiple apps #326

akash1810 commented Mar 16, 2021 •

edited

Loading

akash1810 commented Mar 16, 2021

jacobwinch Mar 17, 2021

akash1810 Mar 17, 2021 •

edited

Loading

jacobwinch left a comment

jacobwinch commented Mar 17, 2021

jacobwinch Mar 17, 2021

akash1810 Mar 17, 2021

stephengeller Mar 19, 2021

akash1810 Mar 25, 2021 •

edited

Loading

akash1810 Mar 17, 2021

jacobwinch Mar 17, 2021

akash1810 Mar 17, 2021

jacobwinch Mar 17, 2021

akash1810 Mar 17, 2021

github-actions bot commented Mar 17, 2021

feat!: Tag support for a stack with multiple apps #326

feat!: Tag support for a stack with multiple apps #326

Conversation

akash1810 commented Mar 16, 2021 • edited Loading

What does this change?

Does this change require changes to existing projects or CDK CLI?

How to test

How can we measure success?

Have we considered potential risks?

akash1810 commented Mar 16, 2021

Choose a reason for hiding this comment

akash1810 Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

jacobwinch left a comment

Choose a reason for hiding this comment

jacobwinch commented Mar 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akash1810 Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 17, 2021

akash1810 commented Mar 16, 2021 •

edited

Loading

akash1810 Mar 17, 2021 •

edited

Loading

akash1810 Mar 25, 2021 •

edited

Loading