aws / aws-extensions-for-dotnet-cli

Extensions to the dotnet CLI to simplify the process of building and publishing .NET Core applications to AWS services
Apache License 2.0
374 stars 87 forks source link

donet8 AOT: cannot dyamically set lambda architecture from cloud formation parameter #302

Open cdaley0 opened 9 months ago

cdaley0 commented 9 months ago

Describe the bug

In my cloudformation json template I have the following parameter in my parameter list:

"LambdaArchitecture": {
  "Type": "String",
  "Default": "arm64",
  "AllowedValues": ["x86_64", "arm64"],
  "Description": "Architecture for lambda functions."
}

And in my AWS::Serverless::Function definitions I set the architecture by referencing this parameter:

"Architectures": [
  {
    "Ref": "LambdaArchitecture"
  }
]

While trying to deploy on my Macbook using dotnet lambda deploy-serverless I get the following error:

"Host machine architecture (Arm64) differs from Lambda architecture (X64). Building Native AOT Lambda functions require the host and lambda architectures to match."

If I hard code the architecture to arm64 I don't get this error.

"Architectures": ["arm64"]

Expected Behavior

The architecture should be set based on the LambdaArchitecture parameter.

Current Behavior

I get an error telling me that the target architecture is set to x86_64 even though I am setting it to arm64 via a cloud formation parameter.

Reproduction Steps

See description

Possible Solution

No response

Additional Information/Context

No response

AWS .NET SDK and/or Package version used

Amazon Lambda Tools for .NET Core applications (5.10.1)

Targeted .NET Platform

.NET 8

Operating System and version

MacOS Monterey

ashovlin commented 9 months ago

Was this working for you on 5.10.0 or earlier versions?

We have a test that started failing with a similar template, that passes on 5.10.0. https://github.com/aws/aws-lambda-dotnet/blob/bd4db5a9abe4a0bfda0b651f87dd0fe048636630/Libraries/test/TestServerlessApp/serverless.template#L5-L23

We're still investigating, and recent commit https://github.com/aws/aws-extensions-for-dotnet-cli/commit/01a0dfd4613a5a7229113ed002b7fc991d08147d did change some Architecture handling.

normj commented 9 months ago

The recent commit was done to fix a bug where if the CloudFormation template said arm64 the function would be created with that architecture but docker image would be created in whatever is the native platform docker is using. Admittedly when we did this is was specifically fixing the issue of I'm on X64 and I said ARM but the image was being created a X64. I over indexed thinking users were on X64 and didn't test the use case of running on an arm Mac.

Let me see how we can rework this change that broke you to solve both scenarios.

normj commented 9 months ago

@cdaley0 Out of curiosity did your workflow used to work but started failing with version 5.10.1? I'm curious if you were doing a multi stage build in your Dockerfile and if so are you specifying the --runtime when doing dotnet publish. If so how were you passing down the architecture into the Dockerfile or were you not setting the --runtime flag.

I have the code changes to essentially revert the behavior added in 5.10.1 if the architecture is set to a Ref but there are more gotchas after that and before I push those changes out I want to make sure you aren't just hitting the next road block or have you already worked around those.

cdaley0 commented 9 months ago

@normj This is the first time I'm trying to set the Architectures property of my lambdas, so I can't comment as to whether it works on previous versions of the sdk.

normj commented 9 months ago

I pushed out version 5.10.2 that I would say is a partial fix in that it reverts back to tools original behavior not specifying the architecture to Docker when it creates the build when the CloudFormation template is using a REF instead of a hardcoded issue. That means the image will be created in whatever is the native platforms architecture.

The full fix should be to resolve the REF but that is a bigger refactor and we had some blocking behavior due to this issue that needed to get fixed quickly.

ashishdhingra commented 1 week ago

Since we have a workaround, downgrading it to p2.