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

fix(lambda): function version ignores layer version changes #20150

Merged
merged 29 commits into from
May 31, 2022

Conversation

kaizencc
Copy link
Contributor

@kaizencc kaizencc commented Apr 29, 2022

Fixes #19098.

This introduces two bug fixes that are hidden behind a feature flag to preserve the current hash:

  • lambda layer order is ignored by the hash now
  • lambda layer version is included in the hash (along with other lambda layer attributes)

I also added a few more tests around this area to confirm the current behavior which should help demonstrate what the feature flag will change.


All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

@gitpod-io
Copy link

gitpod-io bot commented Apr 29, 2022

@github-actions github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Apr 29, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team April 29, 2022 22:03
@kaizencc kaizencc requested review from rix0rrr and removed request for a team April 29, 2022 22:03
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Apr 29, 2022
@kaizencc kaizencc marked this pull request as draft April 29, 2022 22:03
Copy link
Contributor Author

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

Reviving this PR that has been sitting in my local copy of cdk for a while. It's still a bit rough around the edges but I think it's worth getting out there.

packages/@aws-cdk/cx-api/lib/features.ts Show resolved Hide resolved
@@ -640,7 +641,7 @@ export class Function extends FunctionBase {

protected readonly canCreatePermissions = true;

private readonly layers: ILayerVersion[] = [];
public readonly layers: ILayerVersion[] = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The alternative to making this public is to send the layers directly into the calculateFunctionHash() like: calculateFunctionHash(fn, fn.layers). That seems awkward to me so I chose to make this public.

Copy link
Contributor

Choose a reason for hiding this comment

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

calculateFunctionHash is a private function, so I don't really care about its signature. I care more about making this public, which this change does... so I'd rather you didn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Marked @internal instead. I need this to be publicly accessible for the aspect as well, and if my understanding of @internal is correct, this makes the most sense.

@kaizencc kaizencc requested a review from a team May 4, 2022 15:35
@kaizencc kaizencc marked this pull request as ready for review May 11, 2022 16:03
@rix0rrr rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label May 12, 2022
@github-actions github-actions bot added effort/medium Medium work item – several days of effort and removed effort/small Small work item – less than a day of effort labels May 26, 2022
@@ -291,8 +291,15 @@ by default.
Existing users will need to enable the [feature flag]
`@aws-cdk/aws-lambda:recognizeVersionProps`. Since CloudFormation does not
allow duplicate versions, they will also need to make some modification to
their function so that a new version can be created. Any trivial change such as
Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated this readme

* version. This aspect will change the function description in these cases,
* which "validates" the new function hash.
*/
export class FunctionVersionUpgrade implements IAspect {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added this Aspect that should help users update their existing lambdas

@@ -31,4 +32,8 @@ alias.addFunctionUrl({
authType: lambda.FunctionUrlAuthType.NONE,
});

// Changes the function description when the feature flag is present
// to validate the changed function hash.
cdk.Aspects.of(stack).add(new lambda.FunctionVersionUpgrade(LAMBDA_RECOGNIZE_LAYER_VERSION));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this aspect here, so that yarn integ --update-on-failed succeeds. For the integ test updates in other modules, I just ran yarn integ --update-on-failed --disable-update-workflow --dry-run.

@kaizencc kaizencc requested a review from rix0rrr May 27, 2022 15:22
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

Conditionally approved

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 Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/function-hash.ts Outdated Show resolved Hide resolved
@@ -640,7 +641,7 @@ export class Function extends FunctionBase {

protected readonly canCreatePermissions = true;

private readonly layers: ILayerVersion[] = [];
public readonly layers: ILayerVersion[] = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

calculateFunctionHash is a private function, so I don't really care about its signature. I care more about making this public, which this change does... so I'd rather you didn't.

@kaizencc kaizencc removed the pr/do-not-merge This PR should not be merged at this time. label May 31, 2022
@mergify
Copy link
Contributor

mergify bot commented May 31, 2022

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
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit f19ecef into master May 31, 2022
@mergify mergify bot deleted the conroy/lambdalayer branch May 31, 2022 18:26
@mergify
Copy link
Contributor

mergify bot commented May 31, 2022

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).

@izsherpa
Copy link

izsherpa commented Oct 4, 2022

modify cdk.json to enable this feature, pasting this inside the context:
{...
"context": {
"@aws-cdk/aws-lambda:recognizeLayerVersion": true,
....
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. contribution/core This is a PR that came from AWS. effort/medium Medium work item – several days of effort p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-lambda): Layer updates not creating new function version
5 participants