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

Default Context Values for Local Containerized Testing #314

Closed
wants to merge 15 commits into from

Conversation

twhit0611
Copy link

*Issue #, if available: *
#313

Description of changes:
In order to make the runtime easier to consume, we would like to introduce a reasonable set of default values for the AWS Context to be used in combination with the official AWS Lambda container image. The values we chose came directly from the official AWS documentation for Lambda context.

By submitting this pull request

  • I confirm that my contribution is made under the terms of the Apache 2.0 license.
  • I confirm that I've made a best effort attempt to update all relevant documentation.

@twhit0611 twhit0611 changed the title Test lambda Default Context Values for Local Containerized Testing Mar 29, 2021
@rimutaka
Copy link
Contributor

I'm not sure this is the right way of solving the problem.

Correct me if I'm wrong.

  1. The code works fine on AWS AL2
  2. The code fails while running a Docker image of AWS AL2

In that case the Docker image should be modified to supply all the required headers.
With the changes you proposed my Lambda will get some arbitrary values if any of the headers are missing in production. I'd prefer it to fail as it does now.

@coltonweaver
Copy link
Contributor

coltonweaver commented Mar 30, 2021

I agree with @rimutaka on this. Adding in arbitrary default values seems to be the opposite of desired and could lead to potential cases where consumers assume things are working until they realize they aren't, too late. In that case, I too would rather have failures indicating what is wrong.

My suggestion would be that if we want to address this issue, we should add a guide or example on how to actually handle placing this runtime into the container and ensure that these fields are handled properly.

@dspatoulas
Copy link

dspatoulas commented Apr 1, 2021

@twhit0611 it seems like the suggestions from the contributors indicate that a better location for setting reasonable defaults would be somewhere in the aws-runtime-emulator, which is used in the lambda container images during invocation. At an initial glance, I believe the changes could go somewhere around https://github.com/aws/aws-lambda-runtime-interface-emulator/blob/main/cmd/aws-lambda-rie/handlers.go#L173. @coltonweaver please let us know if that is not the correct location for the proposed changes.

As for updating the documentation, I would be happy to provide a guide of how to setup a working local environment for developing lambda rust functions once the headers have been properly injected by the RIE. The ability to quickly create and invoke lambda functions in a local environment provides a much faster feedback cycle than constantly uploading, deploying and invoking a lambda in a native AWS environment.

@coltonweaver
Copy link
Contributor

@dspatoulas Yep, I agree with you on this. Having a reasonable default set within the emulator for local testing, and then having a guide in the documentation on how to make this ready for deployment sounds like the best approach to this situation in my opinion.

@davidbarsky
Copy link
Contributor

davidbarsky commented Apr 8, 2021

I was told that this issue is due to a regression in aws/aws-sam-cli#2715. My personal (non-binding, maintainer emeritus) vote to have this bug be fixed upstream rather than trying to work around the issue in the runtime.

In the meantime, can y'all use an earlier version of the sam cli to work around this regression until the bug is fixed upstream?

@bahildebrand
Copy link
Contributor

I'm gonna throw my vote in with @davidbarsky suggestion, and close this PR. If you need any further assistance please feel free to start a discussion thread, and we can help you look into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants