Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(lambda): introduce a new kind of Code, CfnParametersCode. #2027

Merged
merged 1 commit into from
Apr 8, 2019

Conversation

skinny85
Copy link
Contributor

@skinny85 skinny85 commented Mar 15, 2019

This Code type is helpful when there is no local file/directory to use Assets with,
and the Lambda is supposed to be deployed in a CodePipeline.


Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
  • Title and Description
    • Change type: title prefixed with fix, feat will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

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

@skinny85 skinny85 requested a review from a team as a code owner March 15, 2019 21:28
@skinny85 skinny85 force-pushed the feature/lambda-pipeline-code branch from 6ab070f to f825d39 Compare March 15, 2019 21:28
@skinny85 skinny85 requested a review from rix0rrr March 15, 2019 22:06
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand the use case here. Where is the Lambda code actually coming from? The example seems to imply that there's some lambdaBuildAction that has the artifacts of your lambda function, but I am not sure how this interplays with your infrastructure code...

What happens if this stack is deployed through cdk deploy? What does it mean for the Lambda to use a CodePipelineCode in that case?

packages/@aws-cdk/aws-lambda/README.md Outdated Show resolved Hide resolved
@skinny85
Copy link
Contributor Author

Not sure I understand the use case here. Where is the Lambda code actually coming from? The example seems to imply that there's some lambdaBuildAction that has the artifacts of your lambda function, but I am not sure how this interplays with your infrastructure code...

The use case is deploying your Lambda through a CodePipeline. The lambdaBuildAction packages your Lambda code (compiles it if needed, does npm install if you need it, etc.), and this is where your Lambda code is coming from.

This was the experience we discussed in the doc review for Lambda in CodePipeline.

What happens if this stack is deployed through cdk deploy? What does it mean for the Lambda to use a CodePipelineCode in that case?

That wouldn't work really - you would need to provide the values for the 2 CFN Parameters, and we don't support that from the toolkit I believe. This Stack is meant to be deployed only through a CloudFormation Action.

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 20, 2019

Is this PR superseded by your other one about exposing the parameter names?

@skinny85
Copy link
Contributor Author

Is this PR superseded by your other one about exposing the parameter names?

In my opinion, no.

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 21, 2019

In my opinion, no.

But isn't the other PR also about overriding an asset's source when deployed through a pipeline?

@skinny85
Copy link
Contributor Author

There is still a usecase that is not handled well, even if that PR is merged, and that is a situation where you don't have any Assets to point to when creating your Lambda construct (example when this comes up: when the CDK code and the Lambda code are mastered in separate codebases).

In that situation, you currently have to do something like this:

        const bucketNameParam = new cdk.CfnParameter(lambdaStack, 'BucketNameLambdaSourceParam', {
            type: 'String',
        });
        const objectKeyParam = new cdk.CfnParameter(lambdaStack, 'ObjectKeyLambdaSourceParam', {
            type: 'String',
        });
        const bucket = s3.Bucket.import(lambdaStack, 'LambdaBucket', {
            bucketName: bucketNameParam.stringValue,
        });
        const lambdaCode = lambda.Code.bucket(bucket, objectKeyParam.stringValue);

// later, in the Pipeline Stack:

        const parameterOverrides: { [name: string]: any } = {};
        parameterOverrides[bucketNameParam.logicalId] = lambdaBuildAction.outputArtifact.bucketName;
        parameterOverrides[objectKeyParam.logicalId] = lambdaBuildAction.outputArtifact.objectKey;
        const cfnDeployAction = new cloudformation.PipelineCreateUpdateStackAction({
            // bunch of stuff here...
            parameterOverrides,
            additionalInputArtifacts: [lambdaBuildAction.outputArtifact],
        });

This is far from ideal. What I want to have is:

        const lambdaCode = lambda.Code.codePipeline(); // name TBD

// later, in the Pipeline Stack:

        const cfnDeployAction = new cloudformation.PipelineCreateUpdateStackAction({
            // bunch of stuff here...
            parameterOverrides: lambdaCode.overrideWith(lambdaBuildAction.outputArtifact),
            additionalInputArtifacts: [lambdaBuildAction.outputArtifact],
        });

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 22, 2019

I see, thanks for explaining. I would like to see the use case for this type of asset documented very clearly on the class then. Think of our poor users who won't understand! (i.e., me in 3 months ;))

@@ -120,6 +120,68 @@ lambdaAction.outputArtifact('Out2'); // returns the named output Artifact, or th
See [the AWS documentation](https://docs.aws.amazon.com/codepipeline/latest/userguide/actions-invoke-lambda-function.html)
on how to write a Lambda function invoked from CodePipeline.

### Lambda deployed through CodePipeline

If you want to deploy your Lambda through CodePipeline,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lambda deployed exclusively through CodePipeline, right? Might be worth a note that if you're using assets, you can override them anyway so no need to change your code then. I'm saying this because I want this to be the 99% case:

  • Use an asset, use "cdk deploy" for "personal dev stack"
  • That same asset is reused for CI/CD deployments.

I don't want the docs to send people down paths where they're going to be writing complicated conditionals because they think this is what they have to do to deploy via a Pipeline. Or worse, get confused and come ask us.

Maybe it's worth combining those 2 PRs so we can get the messaging and the examples right in one place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the other PR with overriding Assets has been closed without merging, I kept this title as-is. Let me know if that's fine, or if you still want to change it.

packages/@aws-cdk/aws-lambda/lib/code.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/README.md Outdated Show resolved Hide resolved
@skinny85 skinny85 self-assigned this Mar 28, 2019
@skinny85 skinny85 force-pushed the feature/lambda-pipeline-code branch from f825d39 to 5ab21c3 Compare April 2, 2019 21:31
@skinny85 skinny85 changed the title feat(lambda): introduce a new kind of Code, CodePipelineCode feat(lambda): introduce a new kind of Code, CfnParametersCode. Apr 2, 2019
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. Artifact.fillParameters is a bit off. It’s very specific to lambda code which doesn’t feel right. I am wondering if there’s room for a more abstract concept for an “s3 path” (basically the tuple bucket+key[+version]) and then lambda.CfnParametersCode can implement this interface or have a property that returns this tuple.

packages/@aws-cdk/aws-codepipeline-actions/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/code.ts Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/code.ts Show resolved Hide resolved
@skinny85
Copy link
Contributor Author

skinny85 commented Apr 3, 2019

looks good. Artifact.fillParameters is a bit off. It’s very specific to lambda code which doesn’t feel right. I am wondering if there’s room for a more abstract concept for an “s3 path” (basically the tuple bucket+key[+version]) and then lambda.CfnParametersCode can implement this interface or have a property that returns this tuple.

I thought about this for some time, and I didn't really get anywhere. The only thing that came to me was something like this:

// in the S3 module
export interface S3Coordinates {
  readonly bucketName: string;
  readonly objectKey: string;
  readonly version?: string; // not really needed at this point, but why not
}

, then have lambda.CfnParametersCode implement S3Coordinates, and change Artifact#fillParameters to accept an S3Coordinates instead of a lambda.CfnParametersCode.

I'm not in love with this solution.

How about we invert the control a little bit?

// in the S3 module
export interface S3CoordinatesProvider {
  readonly bucketName: string;
  readonly objectKey: string;
}

// in the CodePipeline module
export class Artifact implements s3.S3CoordinatesProvider {
  // ...
}

// in the Lambda module
export class CfnParametersCode extends /* ... */ {
  // ...

  public fillParameters(provider: s3.S3CoordinatesProvider): { [name: string]: any } {
    const ret: { [name: string]: any } = {};
    ret[this.bucketNameParam.logicalId] = provider.bucketName;
    ret[this.objectKeyParam.logicalId] = provider.objectKey;
    return ret;
  }
}

And its usage looks like this in the CloudFormation Action:

new codepipeline_actions.CloudFormationCreateUpdateStackAction({
  parameterOverrides: {
    ...lambdaCode.fillParameters(lambdaBuildAction.outputArtifact),
  },
  additionalInputArtifacts: [
    lambdaBuildAction.outputArtifact,
  ],
  // ...
});

Thoughts on this?

@eladb
Copy link
Contributor

eladb commented Apr 4, 2019

You are right that it doesn't make sense for CfnParametersCode to expose the coordinates, because those are not the coordinates but rather the parameters... But CoordinateProvider feels like over design. The coordinates come from the artifacts, not from CfnParametersCode.

How about:

parameterOverrides: cfnParametersCode.assign({ 
    bucketName: outputArtifact.bucketName,
    objectKey: outputArtifact.objectKey
}),

Or if we really want to model coordinates, then:

parameterOverrides: cfnParametersCode.assign(outputArtifact.s3Coordinates)

@skinny85 skinny85 force-pushed the feature/lambda-pipeline-code branch from 5ab21c3 to fd3b26c Compare April 4, 2019 19:03
@skinny85
Copy link
Contributor Author

skinny85 commented Apr 4, 2019

Or if we really want to model coordinates, then:

parameterOverrides: cfnParametersCode.assign(outputArtifact.s3Coordinates)

I went with this option, let me know if that works.

This Code type is helpful when there is no local file/directory to use Assets with,
and the Lambda is supposed to be deployed in a CodePipeline.
@skinny85 skinny85 force-pushed the feature/lambda-pipeline-code branch from fd3b26c to 4c5d89e Compare April 8, 2019 19:08
@skinny85
Copy link
Contributor Author

skinny85 commented Apr 8, 2019

Renamed ObjectBucketCoordinates to Coordinates after talking with Elad offline.

@skinny85 skinny85 merged commit 4247966 into aws:master Apr 8, 2019
@skinny85 skinny85 deleted the feature/lambda-pipeline-code branch April 8, 2019 21:16
eladb pushed a commit that referenced this pull request Apr 8, 2019
…2027)

This Code type is helpful when there is no local file/directory to use Assets with,
and the Lambda is supposed to be deployed in a CodePipeline.
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants