feat(aws-s3objectlambda): add L2 construct for S3 Object Lambda #15833

duarten · 2021-07-30T04:12:25Z

This PR adds an L2 construct for the S3 Object Lambda.

To avoid a circular dependency, the construct lives outside of the aws-s3 package.

Fixes #13675

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2021-07-30T04:12:27Z

duarten · 2021-07-30T17:29:50Z

I wonder if the construct should implement IBucket?

nija-at · 2021-08-12T12:08:41Z

Thanks so much for submitting this pull request. I am marking this pull request as p2, which means that we are unable to work on it immediately, but it's definitely still on our radar.

We use +1s to help prioritize our work, and are happy to revaluate this pull request based on community feedback. You can reach out to the cdk.dev community on Slack to solicit support for reprioritization.

corymhall · 2022-02-16T15:20:10Z

packages/@aws-cdk/aws-s3objectlambda/lib/object-lambda.ts

+/**
+ * An S3 Object Lambda for intercepting and transforming `GetObject` requests.
+ */
+export class ObjectLambda extends CoreConstruct {


@duarten thanks for the PR and sorry it took so long to review! The main comment I have is that I think this PR should introduce an AccessPoint construct instead of ObjectLambda since that is the name of the corresponding L1. Also instead of extending CoreConstruct this should extend Resource. I would recommend taking a look at the example here which has the base setup we would be looking for.

Thanks for the feedback, @corymhall! Isn't an AccessPoint different than an ObjectLambda?

Yeah and they are in different libraries, so you would use them like this.

import * as objectlambda from '@aws-cdk/aws-s3objectlambda'; import * as s3 from '@aws-cdk/aws-s3'; new objectlambda.AccessPoint(); new s3.AccessPoint();

In this case S3ObjectLambda is the name of the library and AccessPoint is the name of the class.

Ah, makes sense! I'll update the PR asap.

Pull request has been modified.

corymhall

@duarten updates look good! Just some minor comments, and we need to add

README documentation
unit tests

packages/@aws-cdk/aws-s3objectlambda/README.md

corymhall · 2022-02-17T15:23:09Z

packages/@aws-cdk/aws-s3objectlambda/lib/access-point.ts

+ /**
+ * The name of the access point access point.
+ */
+ readonly accessPointName: string


This should be optional and I think the doc should say The name of the s3objectlambda access point.

corymhall · 2022-02-17T15:24:25Z

packages/@aws-cdk/aws-s3objectlambda/lib/access-point.ts

+ *
+ * @default - No data.
+ */
+ readonly payload?: string


If this is json data can we allow object input that we convert to string?

corymhall · 2022-02-17T15:27:35Z

packages/@aws-cdk/aws-s3objectlambda/lib/access-point.ts

+ public virtualHostedUrlForObject(key?: string, options?: s3.VirtualHostedStyleUrlOptions): string {
+ const domainName = options?.regional ?? true ? this.regionalDomainName : this.domainName;
+ const prefix = `https://${domainName}`;
+ if (typeof key !== 'string') {


If you are just checking to see if key is provided you can do

if (!key) {}

corymhall · 2022-02-17T15:34:03Z

packages/@aws-cdk/aws-s3objectlambda/lib/access-point.ts

+ public abstract readonly accessPointArn: string
+ public abstract readonly accessPointCreationDate: string
+
+ protected abstract readonly name: string;


Suggested change

protected abstract readonly name: string;

public abstract readonly accessPointName: string;

Let's just go ahead and make this public.

corymhall · 2022-02-17T15:52:53Z

packages/@aws-cdk/aws-s3objectlambda/lib/access-point.ts

+ objectLambdaConfiguration: {
+ allowedFeatures,
+ cloudWatchMetricsEnabled: props.cloudWatchMetricsEnabled,
+ supportingAccessPoint: supporting.getAtt('Arn').toString(),


Suggested change

supportingAccessPoint: supporting.getAtt('Arn').toString(),

supportingAccessPoint: supporting.attrArn,

corymhall · 2022-02-17T15:55:22Z

packages/@aws-cdk/aws-s3objectlambda/lib/access-point.ts

+ /**
+ * The Lambda function used to transform objects.
+ */
+ readonly fn: lambda.IFunction


Suggested change

readonly fn: lambda.IFunction

readonly handler: lambda.IFunction

lets call this handler instead.

corymhall · 2022-02-17T15:58:40Z

packages/@aws-cdk/aws-s3objectlambda/lib/access-point.ts

+ ],
+ },
+ });
+ this.accessPoint.addDependsOn(supporting);


I don't think that this is required since an implicit dependency should be added by the GetAttr.

packages/@aws-cdk/aws-s3objectlambda/lib/access-point.ts

corymhall · 2022-02-17T16:41:49Z

packages/@aws-cdk/aws-s3objectlambda/lib/access-point.ts

+ }
+
+ /** Implement the {@link IAccessPoint.domainName} field. */
+ get domainName(): string {


These should either be here or on the abstract class, you don't need them both places.

Oops, forgot to remove them.

Pull request has been modified.

duarten · 2022-02-18T04:45:11Z

@duarten updates look good! Just some minor comments, and we need to add

README documentation

unit tests

Thanks for all the feedback! :)

I've added the documentation, but I'm struggling a bit with the unit tests. Because now we're doing this.accessPointName = this.accessPoint.ref, accessPointName is an unresolved token (because .ref will get us the logicalId), which is awkward to handle in unit tests. Do you have some suggestion about this?

corymhall · 2022-02-18T12:40:21Z

packages/@aws-cdk/aws-s3objectlambda/lib/access-point.ts

+
+ constructor(scope: Construct, id: string, props: AccessPointProps) {
+ super(scope, id, {
+ physicalName: props.accessPointName ?? core.Lazy.string({


Actually I wasn't thinking on this one. We don't need to have CDK generate a name so we can just do

physicalName: props.accessPointName,

corymhall · 2022-02-18T12:47:52Z

packages/@aws-cdk/aws-s3objectlambda/lib/access-point.ts

+ *
+ * @default - No data.
+ */
+ readonly payload?: Record<string, unknown>;


Let's do { [key: string]: any } instead. I think that is the convention we use elsewhere.

corymhall · 2022-02-18T12:54:15Z

@duarten it's looking really good! Just two minor comments.

I've added the documentation, but I'm struggling a bit with the unit tests. Because now we're doing this.accessPointName = this.accessPoint.ref, accessPointName is an unresolved token (because .ref will get us the logicalId), which is awkward to handle in unit tests. Do you have some suggestion about this?

Unit tests with ids can be tricky. If you need to validate a value that is only available after synth (you don't know what it will be when writing the test), I usually do something like this which will give you a failing test with the content of the template.

expect(Template.from_stack(stack).toJSON()).toEqual({});

Pull request has been modified.

duarten · 2022-02-23T13:49:29Z

I added the unit test.

corymhall

Almost there! Just a couple last comments.

corymhall · 2022-02-28T14:54:00Z

packages/@aws-cdk/aws-s3objectlambda/lib/access-point.ts

+ allowedFeatures.push('GetObject-Range');
+ }
+
+ this.accessPoint = new CfnAccessPoint(this, 'LambdaAccessPoint', {


const accessPoint = new CfnAccessPoint(this, 'Resource', {

The default resource should have the id of Resource. Also can you just assign accessPoint to a const.

corymhall · 2022-02-28T14:57:07Z

packages/@aws-cdk/aws-s3objectlambda/package.json

+ },
+ "awslint": {
+ "exclude": [
+ "attribute-tag:@aws-cdk/aws-s3objectlambda.AccessPoint.accessPointName"


I don't think this is needed is it?

Without this I get a lint error.

corymhall · 2022-02-28T15:11:48Z

packages/@aws-cdk/aws-s3objectlambda/test/s3objectlambda.test.ts

+ regional: false,
+ }),
+ });
+ expect(Template.fromStack(stack).toJSON()).toEqual({


Can you update these tests to just use the assertions methods like

Template.fromStack(stack).hasResourceProperties('AWS::S3ObjectLambda::AccessPoint', { ObjectLambdaConfiguration: {}, });

Sorry if my comment about using expect(Template.fromStack(stack).toJSON()) was misleading. I was meaning that that method can be useful to find out logical ids of resources if the normal test output isn't giving you what you need.

Also to make things easier you might want to just create the resources in a beforeEach and then have a separate test to assert individual things. Combined with using the assertions methods each test can be smaller and validate a smaller piece of the template, which makes it easier to review.

let stack: cdk.Stack; let bucket: s3.Bucket; let handler: lambda.Function; beforeEach(() => { stack = new cdk.Stack(); bucket = new s3.Bucket(stack, 'MyBucket'); handler = new lambda.Function(stack, 'MyFunction', { runtime: lambda.Runtime.NODEJS_14_X, handler: 'index.hello', code: new lambda.InlineCode('def hello(): pass'), }); }); test('can create a valid access point'); test('regional virtual hosted URL'); test('non-regional virtual hosted URL'); ...etc.

Pull request has been modified.

corymhall

@duarten looks great!

mergify · 2022-03-04T13:17:21Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-03-04T13:59:33Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 79e2e63
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-03-04T13:59:36Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…15833) This PR adds an L2 construct for the S3 Object Lambda. To avoid a circular dependency, the construct lives outside of the aws-s3 package. Fixes aws#13675 *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

duarten force-pushed the feature/object-lambda-l2 branch 3 times, most recently from cf82e8e to af83e6c Compare July 30, 2021 06:20

duarten force-pushed the feature/object-lambda-l2 branch 2 times, most recently from b8b70b1 to 1cce38a Compare July 30, 2021 23:53

peterwoodworth added the @aws-cdk/aws-s3objectlambda label Jul 31, 2021

peterwoodworth assigned nija-at Jul 31, 2021

duarten force-pushed the feature/object-lambda-l2 branch from 1cce38a to 5504bc1 Compare August 1, 2021 22:29

nija-at added effort/medium Medium work item – several days of effort p2 labels Aug 12, 2021

nija-at removed their assignment Aug 12, 2021

RomainMuller force-pushed the master branch from 924c117 to ebfd5f2 Compare September 29, 2021 13:07

corymhall previously requested changes Feb 16, 2022

View reviewed changes

duarten force-pushed the feature/object-lambda-l2 branch from 5504bc1 to f9175ac Compare February 17, 2022 02:21

duarten force-pushed the feature/object-lambda-l2 branch from f9175ac to 37fafd3 Compare February 17, 2022 03:21

duarten closed this Feb 17, 2022

duarten deleted the feature/object-lambda-l2 branch February 17, 2022 03:22

duarten reopened this Feb 17, 2022

duarten restored the feature/object-lambda-l2 branch February 17, 2022 03:27

duarten force-pushed the feature/object-lambda-l2 branch from 37fafd3 to 6823642 Compare February 17, 2022 04:31

corymhall previously requested changes Feb 17, 2022

View reviewed changes

corymhall added the pr/needs-tests This PR is missing tests. label Feb 17, 2022

duarten force-pushed the feature/object-lambda-l2 branch 2 times, most recently from 015d52a to 10b3fc7 Compare February 18, 2022 05:29

duarten force-pushed the feature/object-lambda-l2 branch from 10b3fc7 to 2a517ce Compare February 18, 2022 06:14

corymhall previously requested changes Feb 18, 2022

View reviewed changes

duarten force-pushed the feature/object-lambda-l2 branch from 2a517ce to 60ba8ae Compare February 19, 2022 01:45

duarten force-pushed the feature/object-lambda-l2 branch from 08b63e8 to 8d57302 Compare February 19, 2022 04:18

corymhall previously requested changes Feb 28, 2022

View reviewed changes

duarten added 4 commits February 28, 2022 12:54

feat(aws-s3objectlambda): add L2 construct for S3 Object Lambda

b302760

address comments

223b179

comments and unit test

0fed862

comments

006b146

duarten force-pushed the feature/object-lambda-l2 branch from 8d57302 to 006b146 Compare February 28, 2022 18:51

corymhall approved these changes Mar 4, 2022

View reviewed changes

Merge branch 'master' into feature/object-lambda-l2

79e2e63

mergify bot merged commit fe9f750 into aws:master Mar 4, 2022

sakurai-ryo mentioned this pull request Nov 19, 2023

(aws-s3objectlambda): support multiple transformationConfigurations #28064

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aws-s3objectlambda): add L2 construct for S3 Object Lambda #15833

feat(aws-s3objectlambda): add L2 construct for S3 Object Lambda #15833

duarten commented Jul 30, 2021 •

edited

Loading

gitpod-io bot commented Jul 30, 2021

duarten commented Jul 30, 2021 •

edited

Loading

nija-at commented Aug 12, 2021

corymhall Feb 16, 2022

duarten Feb 16, 2022

corymhall Feb 16, 2022

duarten Feb 16, 2022

duarten Feb 17, 2022

corymhall left a comment

corymhall Feb 17, 2022

corymhall Feb 17, 2022

corymhall Feb 17, 2022

corymhall Feb 17, 2022

corymhall Feb 17, 2022

corymhall Feb 17, 2022

corymhall Feb 17, 2022

corymhall Feb 17, 2022

duarten Feb 18, 2022

duarten commented Feb 18, 2022 •

edited

Loading

corymhall Feb 18, 2022

corymhall Feb 18, 2022

corymhall commented Feb 18, 2022

duarten commented Feb 23, 2022

corymhall left a comment

corymhall Feb 28, 2022

corymhall Feb 28, 2022

duarten Feb 28, 2022

corymhall Feb 28, 2022

duarten Feb 28, 2022

corymhall left a comment

mergify bot commented Mar 4, 2022

aws-cdk-automation commented Mar 4, 2022

mergify bot commented Mar 4, 2022

	protected abstract readonly name: string;
	public abstract readonly accessPointName: string;

	supportingAccessPoint: supporting.getAtt('Arn').toString(),
	supportingAccessPoint: supporting.attrArn,

	readonly fn: lambda.IFunction
	readonly handler: lambda.IFunction

feat(aws-s3objectlambda): add L2 construct for S3 Object Lambda #15833

feat(aws-s3objectlambda): add L2 construct for S3 Object Lambda #15833

Conversation

duarten commented Jul 30, 2021 • edited Loading

gitpod-io bot commented Jul 30, 2021

duarten commented Jul 30, 2021 • edited Loading

nija-at commented Aug 12, 2021

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

corymhall left a 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

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

duarten commented Feb 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corymhall commented Feb 18, 2022

duarten commented Feb 23, 2022

corymhall left a 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

Choose a reason for hiding this comment

corymhall left a comment

Choose a reason for hiding this comment

mergify bot commented Mar 4, 2022

aws-cdk-automation commented Mar 4, 2022

AWS CodeBuild CI Report

mergify bot commented Mar 4, 2022

duarten commented Jul 30, 2021 •

edited

Loading

duarten commented Jul 30, 2021 •

edited

Loading

duarten commented Feb 18, 2022 •

edited

Loading