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

fix: add client context and cognito identity to runtime context #382

Merged
merged 1 commit into from
Dec 28, 2021
Merged

fix: add client context and cognito identity to runtime context #382

merged 1 commit into from
Dec 28, 2021

Conversation

realtimetodie
Copy link
Contributor

Issue #, if available:

Description of changes:

Fixes an issue where the client context lambda-runtime-client-context and the cognito identity lambda-runtime-cognito-identity were not deserialized from the incoming request headers.

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.

@realtimetodie realtimetodie changed the title fix: add client context and icognito identity to runtime context fix: add client context and cognito identity to runtime context Dec 16, 2021
@nmoutschen nmoutschen mentioned this pull request Dec 17, 2021
7 tasks
Copy link
Contributor

@bahildebrand bahildebrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure I understand the issue. This was previously panicking if the headers were not present correct? So the addition of these Options allows the deserialization to succeed if they are not present?

Also can you fix any clippy issues?

lambda-runtime/src/types.rs Show resolved Hide resolved
@davidbarsky
Copy link
Contributor

This isn't really my project to maintain anymore, but as the person who made this decision a few years ago, I think it'd be helpful if I provided a bit of context. At the time—and I believe this is still the case—the client context and Cognito Identity were only filled by the AWS Mobile SDKs that directly invoked Lambda functions. This pattern was discouraged in favor of delegating authentication/authorization to API Gateway, and the thinking was that anyone who'd be using this new runtime will probably not be using those older SDKs.

However, I don't know if this guidance has changed and someone a bit closer to those decisions should weigh in.

@realtimetodie
Copy link
Contributor Author

@davidbarsky We use the custom property of the client context to transfer metadata to the client. We don't use the Mobile SDKs. The client context seemed to be universal in nature for us, so we were like .. "Hey, there is something that we can use to solve our problem".

@bahildebrand

This was previously panicking if the headers were not present correct? So the addition of these Options allows the deserialization to succeed if they are not present?

Yes, I noticed that it works in the Node.js client and the .NET client, but not in the Rust client.

https://github.com/aws/aws-lambda-nodejs-runtime-interface-client/blob/e5d62346eb08f1723348197df70d4f55f9c2ebcf/src/Runtime/InvokeContext.ts#L114-L121

https://github.com/aws/aws-lambda-dotnet/blob/master/Libraries/src/Amazon.Lambda.RuntimeSupport/Client/RuntimeApiHeaders.cs#L33-L34

Copy link
Contributor

@calavera calavera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for indulging me with my very own personal opinions @diceride. I really like how clean this looks like now 🙌

Copy link
Contributor

@bahildebrand bahildebrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Waiting on all checks to pass and then I'll merge.

@bahildebrand bahildebrand merged commit 5c15750 into awslabs:master Dec 28, 2021
@nmoutschen nmoutschen added the 0.5 label Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants