feat: Add Fastly Logs IAM and related constructs #761

frankie297 · 2021-08-26T08:53:38Z

What does this change?

Adds an s3 put object construct, and creates a Fastly Logs IAM pattern. This is specifically to allow Fastly to log directly to s3 rather than using keys and having to keep them rotated. See documentation: https://docs.fastly.com/en/guides/creating-an-aws-iam-role-for-fastly-logging

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

No

Does this change require changes to the library documentation?

Yes

How to test

Snapshot test added

How can we measure success?

Manual key rotation is no longer needed, so less security issues.

Paired with @jfsoul

akash1810

This looks great! Thanks for the contribution!

I'm not sure how semantic-release will parse these commits as they're not following the angular commit message conventions, I suspect you'll end up needing to squash and merge. There is a beta release channel available if needed.

I'd recommend configuring your IDE to format on save to avoid CI failing on linting errors; you can run npm run lint --fix to fix them.

src/patterns/fastly-logs-iam.ts

src/constructs/core/parameters/fastly.ts

src/patterns/fastly-logs-iam.ts

jfsoul · 2021-09-03T13:22:04Z

@akash1810 we missed the requirement for angular commit messages - have we considered adding a commit hook if that's a pretty strict expectation for contributors?

akash1810 · 2021-09-06T08:14:23Z

@akash1810 we missed the requirement for angular commit messages - have we considered adding a commit hook if that's a pretty strict expectation for contributors?

IIRC we discussed commit hooks in the early days and opted against after experience on other repositories (they're more restrictive than helpful); keen to hear from @jacobwinch and @sihil to see if this still holds.

I'm not too sure how semantic-release will parse this PR, either:

It will parse the feat part of the merge commit and release to npm
Or it will parse each commit and not deem the change as not needing a release

I'm really interested to find out! Happy to merge as is and we can update any docs to reflect our new learning.

I think a sure fire way to make semantic-release happy is to squash and merge.

akash1810

🎉

jfsoul · 2021-09-06T08:27:38Z

I'm really interested to find out! Happy to merge as is and we can update any docs to reflect our new learning.

It's a tempting option to experiment, but I think our commits are not ideal for the history in this case so I was always assuming it'd be a squash and merge for this PR!

This branch needs rebasing now.

akash1810 · 2021-12-23T11:29:24Z

A new required status check has been added to this repository, this branch now needs rebasing against main as a result. See #998 (comment).

jfsoul · 2022-01-07T13:22:27Z

src/constants/ssm-parameter-paths.ts

@@ -1,6 +1,7 @@
 export interface SsmParameterPath {
  path: string;
  description: string;
+  optional?: boolean;


The account-readiness CLI command currently expects all these parameters to be required, but actually we're introducing an optional, account-wide parameter here (account-wide because you might have multiple instances of the fastly IAM logging construct in a single account)

akash1810 · 2022-01-07T14:32:10Z

docs/005-default-parameter-store-locations.md

+| /account/services/artifact.bucket       | DistributionBucketName       | String                       | GuGetDistributablePolicy   |
+| /account/services/logging.stream.name   | LoggingStreamName            | String                       | GuLogShippingPolicy        |
+| /account/services/anghammarad.topic.arn | AnghammaradSnsArn            | String                       | AnghammaradSenderPolicy    |
+| /account/external/fastly/customer.id    | GuFastlyCustomerIdParameter  | String                       | GuFastlyLogsIam            |


We should really auto-generate this list!

src/constants/ssm-parameter-paths.ts

akash1810 · 2022-01-07T14:36:14Z

src/constructs/iam/fastly-logs-iam.test.ts

+    const stack = simpleGuStackForTesting();
+    new GuFastlyLogsIam(stack, "FastlyS3LoggingIam", {
+      bucketName: "test",
+      path: "",


Can we avoid the empty string at all?

akash1810 · 2022-01-07T14:39:28Z

src/constructs/iam/fastly-logs-iam.ts

+ *
+ * See https://docs.fastly.com/en/guides/creating-an-aws-iam-role-for-fastly-logging
+ */
+export class GuFastlyLogsIam {


I think this should be extend GuRole rather than being a plain class.

I've made that change, although it doesn't make a whole lot of sense now because I have a policy being created inside a role constructor. I think semantically it was correct before - a construct formed of 2 resources (policy + role) which both used Gu constructs themselves (but the containing construct was its own entity).

Tempted to revert to previous implementation now!

Extending GuRole follows the current convention of the repo, for example the GuInstanceRole. This also makes me realise that this should probably be called GuFastlyLogRole too!

To nitpick a little:

I think semantically it was correct before - a construct formed of 2 resources

This isn't correct as before we had a vanilla class formed of two resources. In CDK there is a Construct class which almost everything extends from 🤯 .

I think generally we'd only create vanilla classes for patterns, rather than constructs - but even then, I think patterns could extend Construct (see this example of a pattern from AWS).

One of the benefits from using the Construct type is you can hook into validate more easily. Additionally any exceptions that you might throw in the constructor of a Construct get pretty printed with a breadcrumb so it's a little easier to debug.

akash1810

🎉

akash1810 · 2022-01-07T17:02:23Z

src/constructs/iam/fastly-logs-iam.ts

+    });
+    const policy = new GuPutS3ObjectsPolicy(scope, "GuFastlyLogsIamRolePolicy", {
+      bucketName: props.bucketName,
+      paths: props.path ? [props.path] : undefined,


minor: I've recently started to set defaults via destructuring and find it more explicit and easier to follow, e.g.

const { path = "*" } = props;

Not massive, but this method would make the description of path above more true, specifically @default - '*' as at the moment it defaults to undefined, and its GuPutS3ObjectsPolicy that is setting it to * (obviously you can use * in the ternary too).

github-actions · 2022-01-10T10:28:23Z

🎉 This PR is included in version 33.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

frankie297 requested a review from a team August 26, 2021 08:53

akash1810 reviewed Aug 26, 2021

View reviewed changes

src/patterns/fastly-logs-iam.ts Outdated Show resolved Hide resolved

src/constructs/core/parameters/fastly.ts Outdated Show resolved Hide resolved

src/patterns/fastly-logs-iam.ts Outdated Show resolved Hide resolved

akash1810 previously approved these changes Sep 6, 2021

View reviewed changes

frankie297 and others added 16 commits January 7, 2022 09:59

Add cdk pattern for fastly logs s3

55b97e9

Make sure code is there

7b0a0e0

Adding the fastly log permission pattern

434c48e

Create s3-put-object test

01c3e03

Add test for fastly-logs-iam

dec13a9

Add a snapshot for the fastly logs iam test

2df2fcd

Add snapshot

e66872a

Change id name

a3b691d

Update comment on parameter construct

831f413

Formatting

0ead559

Update snapshot

9af7c3e

Account ID as const

5ab3def

Move fastly-logs-iam to constructs

3a6e293

Docs

81ecd0c

Update snapshot

4ed02a0

Update reference after moved util

c493d61

jfsoul force-pushed the fh-js-cdk-fastly-logs-s3 branch from efb82ed to c493d61 Compare January 7, 2022 10:12

jfsoul added 4 commits January 7, 2022 11:18

Add fastly parameter to docs

97bb3c4

Improved docs

9fd2537

Export props interface for docs

a579b3b

Optional ssm parameters

e364d53

jfsoul reviewed Jan 7, 2022

View reviewed changes

akash1810 reviewed Jan 7, 2022

View reviewed changes

jfsoul added 4 commits January 7, 2022 15:12

How to obtain Fastly customer ID

2ba3943

Inherit from GuRole

22c8846

Non-empty path for test

d8b874c

Renaming

0e3b9e9

akash1810 approved these changes Jan 7, 2022

View reviewed changes

jfsoul added 2 commits January 10, 2022 09:32

Set defaults via destructuring

386966f

Set prop defaults via destructuring

bb34230

jfsoul changed the title ~~feat: Add Fastly Logs IAM pattern, and s3 put object construct~~ feat: Add Fastly Logs IAM and related constructs Jan 10, 2022

jfsoul merged commit 5ff8648 into main Jan 10, 2022

jfsoul deleted the fh-js-cdk-fastly-logs-s3 branch January 10, 2022 10:23

github-actions bot added the released label Jan 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Fastly Logs IAM and related constructs #761

feat: Add Fastly Logs IAM and related constructs #761

frankie297 commented Aug 26, 2021

akash1810 left a comment

jfsoul commented Sep 3, 2021

akash1810 commented Sep 6, 2021 •

edited

Loading

akash1810 left a comment

jfsoul commented Sep 6, 2021

akash1810 commented Dec 23, 2021

jfsoul Jan 7, 2022

akash1810 Jan 7, 2022

akash1810 Jan 7, 2022

akash1810 Jan 7, 2022

jfsoul Jan 7, 2022

akash1810 Jan 7, 2022

akash1810 left a comment

akash1810 Jan 7, 2022 •

edited

Loading

github-actions bot commented Jan 10, 2022

feat: Add Fastly Logs IAM and related constructs #761

feat: Add Fastly Logs IAM and related constructs #761

Conversation

frankie297 commented Aug 26, 2021

What does this change?

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

Does this change require changes to the library documentation?

How to test

How can we measure success?

akash1810 left a comment

Choose a reason for hiding this comment

jfsoul commented Sep 3, 2021

akash1810 commented Sep 6, 2021 • edited Loading

akash1810 left a comment

Choose a reason for hiding this comment

jfsoul commented Sep 6, 2021

akash1810 commented Dec 23, 2021

jfsoul Jan 7, 2022

Choose a reason for hiding this comment

akash1810 Jan 7, 2022

Choose a reason for hiding this comment

akash1810 Jan 7, 2022

Choose a reason for hiding this comment

akash1810 Jan 7, 2022

Choose a reason for hiding this comment

jfsoul Jan 7, 2022

Choose a reason for hiding this comment

akash1810 Jan 7, 2022

Choose a reason for hiding this comment

akash1810 left a comment

Choose a reason for hiding this comment

akash1810 Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jan 10, 2022

akash1810 commented Sep 6, 2021 •

edited

Loading

akash1810 Jan 7, 2022 •

edited

Loading