feat(s3-deployment): switch CDKBucketDeployment Lambda architecture to ARM_64

[eruvanos]

Issue # (if applicable)

Closes #29996

Reason for this change

Reduce costs for s3 bucket deployments.

Description of changes

Change LambdaFunction architecture

Description of how you validated changes

Run tests

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

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

[aws-cdk-automation]

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

[eruvanos]

Exemption Request: This change does not need an entry in the README

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 6867779
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[jfuss]

How did we validate this works correctly? I have a couple concerns with this:

1. The Lambda Function that is getting updated from x86 to arm is using a Lambda Layer that is not built for arm. Since AwsCliLayer is using https://github.com/cdklabs/awscdk-asset-awscli which looks to only be built on an non arm version: https://github.com/cdklabs/awscdk-asset-awscli/blob/awscli-v1/main/layer/Dockerfile#L1.
2. I also worry when ARM is added to Lambda for new regions and if they are in all the existing regions. I need to follow up on this internally but usually blanket things like this don't always work on. Which leads me to wanting to expose architecture as a property on the Construct and allowing customers to change this instead of us flipping under the hood.
3. It's possible someone is building on top of this construct and switching the arch without a way for these customers to keep the current arch is a breaking changing.

I think 1 is really important to get right. Which means we need to stage multiple changes here. We will need AwsCliLayer to be able to support both archs of Lambda before we can even look at 2/3. I will follow up internally but I think we need to be exposing this as a property on the Construct not just updating the value.

There also looks to be other changes in this PR that are outside of the Lambda Arch update. Please remove them as it makes it harder as a reviewer to understand the side effects of changes you are making. I can't tell if the appconfig tests are changing because of the intent of the PR or because you made other changes.

[aws-cdk-automation]

The pull request linter fails with the following errors:

❌ Features must contain a change to a README file.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

✅ A exemption request has been requested. Please wait for a maintainer's review.

[daschaa]

@jfuss Thank you for your review/reply! Actually I'm a bit confused with 1) because I deployed all the integration tests here so if the Layer does not support ARM, how is it then possible that the integration tests succeeded?

And additonally I don't understand why the Dockerfile that you linked indicates that it is only built on an non arm version. I guess I lack some knowledge on containers here, but when I google that image I see that it is also available for ARM. Can you explain how I can see that this is only built for ARM?

Regarding 2): I think opening up the lambdas and make the architecture configurable is a really good idea here.

Regarding the additonal changes: No, I didn't made any additional changes. I adjusted the appconfig test because the integration test was not working like it was implemented there (to be specific: the used bucket could not be deleted because it contained files).

[aws-cdk-automation]

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

[aws-cdk-automation]

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

[jfuss]

@daschaa Sorry for the delayed response, I had to take a couple unexpected days off last week.

1. because I deployed all the integration tests here so if the Layer does not support ARM, how is it then possible that the integration tests succeeded?

It's possible, so it's not surprising but my worry is at some point a change breaks things. Let me try to articulate things better here.

So the AwsCliLayer is a python package (AWS CLI v1). Lambda Layers are meant to target a Runtime[s] and Architecture[s]. So the part I am worried about is the AwsCliLayer is building on an x86_64 image (public.ecr.aws/sam/build-python3.8). Docker will auto pull the latest tag, which is why I know it is an x86_64 image (I was also a maintainer of SAM CLI which owns all the build images). It's very possible/likely that the the dependencies of the CLI don't have any architecture requirements today but that doesn't mean that can't change or you just didn't hit a case in the CLI that requires a different dependency for arm. I have been bitten by Python to many times to trust things will just always work in different architectures.

Can you explain how I can see that this is only built for ARM?

What I would expect is, a change to AwsCliLayer repo that would add an AwsCliLayerArm (not locked on name). We can follow the same process as the current one but instead of building from public.ecr.aws/sam/build-python3.8, we would need to build from public.ecr.aws/sam/build-python3.8@latest-arm64. We can keep the AwsCliLayer the same for now, we might want to add CompatibleArchitectures but if it isn't on AwsCliLayer already, let's only add it to the AwsCliLayerArm to keep from unexpected breaks for existing customers. Once we have that, we can then do a if arch == arm: use AwsCliLayerArm, else AwsCliLayer. This just gets us into a better spot for maintaining things longer term.

Regarding the additonal changes: No, I didn't made any additional changes. I adjusted the appconfig test because the integration test was not working like it was implemented there (to be specific: the used bucket could not be deleted because it contained files).

Can you submit them in a different diff. I usually don't care that much but it would really help me as I don't have deep familiarity with the codebase. So I heavily rely on the diff to help me understand possible impact.

[aws-cdk-automation]

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

[aws-cdk-automation]

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

[daschaa]

@jfuss Thanks for the clarification and the detailed writeup, now I understand the issue!
What are now the next steps to have a solution to the "problem"?

Opening up the underlying lambda functions would be a really good idea, because for our enterprise context we also have to ensure that all lambdas are running inside a VPC (compliance reasons). Right now we solve this by using an aspect, but it would be a lot easier if the CDK just allows to put additional properties to the (internal) lambda functions.

[aws-cdk-automation]

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

[aws-cdk-automation]

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

[aws-cdk-automation]

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

[aws-cdk-automation]

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

[aws-cdk-automation]

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

[aws-cdk-automation]

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

[eruvanos]

@jfuss @daschaa

Looks like the proposed solution does not cover all aspects. I think it makes sense to close this PR and have the discussions on the issue.