aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.37k stars 3.78k forks source link

(aws-s3-deployment): Switch Lambda architecture to ARM_64 #29996

Open eruvanos opened 2 months ago

eruvanos commented 2 months ago

Describe the feature

To reduce costs and improve sustainability, it would be great to switch CDKBucketDeployment Lambda to ARM_64.

Use Case

Reduced costs.

Proposed Solution

The code looks compatible to ARM_64, switching architecture should be enough.

Other Information

To give more information, it would be great to be able to have little more control about created Lambdas. E.g. our environment enforces Lambdas to be deployed into a VPC. This is only possible using an Aspect.

Acknowledgements

CDK version used

2.102.0

Environment details (OS name and version, etc.)

independent

pahud commented 2 months ago

Make perfect sense to me. Welcome to submit a PR. Thank you.

tmokmss commented 2 months ago

Is Graviton Lambda available in all the regions and partitions? I could not find relevant coverage data.

eruvanos commented 2 months ago

@tmokmss That is a good question, did not think about that. I will check, otherwise we would have to introduce a property.

nmussy commented 2 months ago

The documentation states that we should be using the pricing region dropdown to get the ARM availability. It seems to be globally available as of today, comparing the dropdown options with a browser inspector, they both have the following 31 regions, including Gov:

us-east-1
us-east-2
us-west-1
us-west-2
af-south-1
ap-east-1
ap-south-2
ap-southeast-3
ap-southeast-4
ap-south-1
ap-northeast-3
ap-northeast-2
ap-southeast-1
ap-southeast-2
ap-northeast-1
ca-central-1
ca-west-1
eu-central-1
eu-west-1
eu-west-2
eu-south-1
eu-west-3
eu-south-2
eu-north-1
eu-central-2
il-central-1
me-south-1
me-central-1
sa-east-1
us-gov-east-1
us-gov-west-1

The CN regions are not included, but they also both seem to support the ARM arch, see pricing.

It should also be safe to assume newly introduced regions will get this feature, given ca-west-1 is the latest release. I don't think this is a concern, but good call @tmokmss

daschaa commented 1 month ago

The linked PR looks good imo. Does this change need an entry in the README or can the exemption request be approved? I think the pr-linter still marks the pull request as "not ready".

github-actions[bot] commented 1 month ago

This issue has received a significant amount of attention so we are automatically upgrading its priority. A member of the community will see the re-prioritization and provide an update on the issue.

eruvanos commented 1 month ago

Update: Our PR was basically rejected, because the proposed solution does not cover all requirements.

I changed the issue description accordingly.

jfuss commented 4 weeks ago

@eruvanos I think there is still value but we cannot just change it for everyone. We have to assume someone somewhere relied on this being x86-64.

The path to getting this added to CDK would be: 1) Making the AWSCLI layer Construct support both architectures. 2) Adding a property that accepts the architecture for CDKBucketDeployment Lambda. 3) Picking the correct AWS CLI Layer based on teh architecture.

This adds more work but removes risks from just changing the default to something else without worry about breaking any existing customers.

daschaa commented 3 weeks ago

@jfuss That sounds like a good plan. Is this "feature" already planned on you side and if so, can you give an ETA?