-
Notifications
You must be signed in to change notification settings - Fork 34
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: remove the lambda layers used for custom resources underneath lambda functions #1731
Conversation
lib/__tests__/test-stack.ts
Outdated
@@ -142,7 +142,8 @@ export class TestStack extends Stack { | |||
identity: 'aws-cdk-dev', | |||
keySizeBits: 4_096, | |||
pubKeyParameterName: `/${this.node.path}/CodeSign.pub`, | |||
secretName: this.node.path + '/CodeSign', | |||
// I am not able to immediately delete the existing secret, it needs to wait at least 7 days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens to the old secret here? Are we not deleting it by changing the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole chain of resource replacements is:
- We need to change the custom resource handler's packaging type from zip to Docker image, but Lambda doesn't allow that; we need to make a new Function, which we do by changing the logical ID.
- Changing the Custom Resource Service Token is similarly forbidden, so we must also create a new one; this is another logical ID change.
- Recreating the custom resource will cause it to create a new key, which will fail because the key with that name (
this.node.path + '/CodeSign'
) already exists; so, we must change the key name.
@moelasmar I would think that step 3 would cause the secret to be deleted as well, because the custom resource should delete it on the DELETE
event. Can you explain why it doesn't?
lib/custom-resource-handlers/certificateSigningRequestDockerfile
Outdated
Show resolved
Hide resolved
# Use a NodeJS 20.x runtime | ||
FROM public.ecr.aws/lambda/nodejs:20-x86_64 | ||
|
||
# install openssel and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# install openssel and | |
# install openssel |
@@ -0,0 +1,15 @@ | |||
FROM public.ecr.aws/lambda/nodejs:20-x86_64 | |||
|
|||
# install openssel and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need all three of these Dockerfiles? They look like the same one each time, is there any harm in using one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline; can use an env var to change the one difference between them
Broader context: We used to store GPG and Ideally, we'd rebuild this dockerfile every time we build, so that dependency upgrade PRs happen to fetch the latest versions of these for us. |
Signed-off-by: github-actions <github-actions@github.com>
pipeline.autoBuild({ | ||
publicLogs: true, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the deletion here? We want this to be tested, no?
This change removed the Lambda layers we used to add the pgp and openssl binaries to the lambda functions that are used to implement the OpenPGPKeyPair, RsaPrivateKeySecret, CodeSigningCertificate, and CertificateSigningRequest constructs.
The change is to make these lambda functions to be of Image package type, so we can install the required binaries in the docker image, instead of maintaining the binaries in the github repo.
I followed this section https://github.com/cdklabs/aws-delivlib/blob/main/CONTRIBUTING.md#testing for testing these changes, and mainly the part of running
yarn integ:update
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.