Fix sam local support for missing headers (#38) #332

kenshih · 2021-06-19T03:12:52Z

Hello, I hope this finds you well!

I was trying to run sam local in order to make using rust more viable at work & got it working locally, so I thought I'd throw up a pull request, in case it solves an issue for other alpha runtime users.

Fix sam local support for missing headers (#38)

Issue #38
also related to:

Example of problem:

# when running "sam invoke" against an existing "sam start-lambda", something like this happened for me
...
START RequestId: 38dd307a-a56a-4def-9adf-136a5a43ac70 Version: $LATEST
thread 'main' panicked at 'no entry found for key "lambda-runtime-invoked-function-arn"', /Users/ken/wk/experiments/rust/aws-lambda-rust-runtime/lambda-runtime/src/types.rs:131:35
stack backtrace:
   0: rust_begin_unwind
             at /rustc/9bc8c42bb2f19e745a63f3445f1ac248fb015e53/library/std/src/panicking.rs:493:5
   1: std::panicking::begin_panic_fmt
             at /rustc/9bc8c42bb2f19e745a63f3445f1ac248fb015e53/library/std/src/panicking.rs:435:5
   2: <lambda_runtime::types::Context as core::convert::TryFrom<http::header::map::HeaderMap>>::try_from
   3: ken_test_rust_lambda::main::{{closure}}
   4: std::thread::local::LocalKey<T>::with
   5: tokio::runtime::enter::Enter::block_on
   6: tokio::runtime::Runtime::block_on
   7: ken_test_rust_lambda::main
...

After the following changes, the above stack trace was no more & the lambda invoke was able to run successfully to completion with expected response payload.

Description of changes:

Fix sam local support for missing headers (#38)

Added unit tests for TryFrom for Context

The issue was that when the lambda-runtime is invoked by the sam local simulated environment, the headers lambda-runtime-invoked-function-arn and lambda-runtime-trace-id are not provided (as they are in the production cloud environment).

I have confirmed this issue still exists in v0.3.0 and on master

I'm not aware if these headers are supposed to be invariants or not, but, it seems that other language runtimes are more forgiving with these missing headers with sam local invoke, so it seems appropriate for the rust runtime to be resilient to these as well?

Instead of panic-ing on a missing header key, this code now provides a default value for the two headers in question. The default string indicates that they were not found in the environment as expected.

Let me know if you the style is not to your liking, happy to change as you suggest and/or accept incoming changes. I am rather new to rust. Apologies if I have misinterpreted any conventions.

Thanks for considering this PR.

Ken

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.

* Added unit tests for TryFrom<HeaderMap> for Context

kenshih · 2021-06-19T03:16:02Z

lambda-runtime/src/types.rs

-                .to_str()
-                .expect("Missing Request ID")
+            request_id: headers
+                .get("lambda-runtime-aws-request-id")


instead of implicitly failing (by panic-ing on a missing key), i retain the original panic behavior, but now we panic at the expect as expected :) -- I think in the original code we don't ever make it to the expect because it panics on the attempt to dereference headers["lambda-runtime-aws-request-id"]. However, I kept the panic language to refer to "lambda-runtime-aws-request-id", because I found the specificity was very helpful in tracking down the bug.

I chose not to change the panic to an Error here, since sam local does fulfill this requirement, so no need to be prematurely lax.

Good catch, thanks for fixing this.

kenshih · 2021-06-19T03:19:37Z

lambda-runtime/src/types.rs

                .to_owned(),
-            deadline: headers["lambda-runtime-deadline-ms"]
+            deadline: headers
+                .get("lambda-runtime-deadline-ms")


took same approach as "lambda-runtime-aws-request-id" here.

kenshih · 2021-06-19T03:22:27Z

lambda-runtime/src/types.rs

+            invoked_function_arn: headers
+                .get("lambda-runtime-invoked-function-arn")
+                .unwrap_or(&HeaderValue::from_static(
+                    "No header lambda-runtime-invoked-function-arn found.",


Instead of honoring ARN format, I only honored the "String" type of ARN. In production, of course, it should be a real ARN. But in development (such as sam local), it should be clear that something is wrong with the environment, if the developer somehow needs to inspect this context.

Good call. One of my original worries was if someone was using the ARN in some way it would be annoying to debug if the default looked like a real value.

kenshih · 2021-06-19T03:23:10Z

lambda-runtime/src/types.rs

-                .expect("Invalid XRayTraceID sent by Lambda; this is a bug")
+            xray_trace_id: headers
+                .get("lambda-runtime-trace-id")
+                .unwrap_or(&HeaderValue::from_static("No header lambda-runtime-trace-id found."))


As reported in other issues, as well as confirming this myself, sam local invoke also leaves off this header.

kenshih · 2021-06-19T03:25:59Z

lambda-runtime/src/types.rs

                .to_owned(),
            ..Default::default()
        };
        Ok(ctx)
    }
 }

+#[cfg(test)]


I wasn't sure exactly where to put these tests, so I just placed them right after the thing they are testing.

I noticed the other test in this file does not use #[cfg(test)]... I wasn't sure if I should have left it off here or not. As I understand, without adding #[cfg(test)], it would artificially bloat the release build as well. As I said, I'm still new enough to rust though, so please let me know if I misunderstand!

kenshih · 2021-06-19T04:19:39Z

hum. so, i just noticed this:
#322
sorry i didn't see that.

I'm curious if the position has changed at all.

Given that other runtimes seem fine with these missing headers in the context of sam local, I'm wondering why the rust runtime would be different. I realize this might be something to revisit, if that context header becomes an invariant for later runtime machinery; but doesn't this bug make it even less likely that this runtime will rely an that header given the slower feedback cycle for development on it? Wondering why the strict abstraction barrier for those 2 specific headers at this low level.

For the context of the the runtime consumer, that is, the lambda application developer, the devx of lambda without sam local is a big gap. I realized that while I was writing this template for myself: https://github.com/kenshih/rust-aws-lambda-template. I was reluctant to share it with anyone on my team without a good lightweight solution for local development.

kenshih · 2021-06-19T04:38:46Z

Ok. So feel free to close this, as you like.

I'm seeing the arguments more clearly: #314

I'll leave this open until you get a chance to see it, just as another +1 to the situation this runtime is left in & the pain it causes.

Thanks for your work.

kenshih · 2021-06-19T16:48:49Z

Revisiting what i did here, I now think neither approach (this PR, as well, not doing this PR) is good.

Following the Robustness Principle...
it seems like the runtime should allow the bad header (being "liberal with what it accepts" from others) which would make it less fragile;
but, setting an arbitrary string for the values is not a good solution (not :"being conservative about what it passes along" to the consumer of the lambda environment).

I wonder if the runtime instead should:

emit a non-blocking WARN about misconfiguration
the Context data structure should have Optional values.

The cost, of course, is a more complicated interface with the consumer (Option vs. guaranteed data).

But, at the same time, good side effects are: it would encourage promulgation of the robustness principle down to the consumer & reflects the reality of the often imperfect situation inherent in layer & cross-subsystem dependencies. Contracts break. So do we live with months/if-ever delays in the meantime, or robust our way out of it, knowing we're exposed to some interface risk? I honestly don't know the right answer here. But curious to hear the balance the maintainers of this runtime hope to achieve.

Thanks for listening.

asg0451 · 2021-07-06T16:38:37Z

FWIW as a user of this project, I don't care at all about the lambda context, and I do wish we could run with sam local. Option values in Context isn't the worst idea in the world.

brainstorm · 2021-07-08T04:11:28Z

Yes please, un-draft this PR and get it shipped, it'll help a ton to have a proper sam local run & debug loop. I'd love to see this in #315.

/cc @coltonweaver, @bahildebrand

kenshih · 2021-07-15T13:48:32Z

So I'll be happy to un-Draft this, @brainstorm, thank you. But I need a Maintainer to allow me to merge, or at least comment about how I can move this into a solution that would be acceptable.

Would moving Context values to Option values and emitting WARN be an acceptable compromise, @maintainers? In the interface code, I would link a comment to the doc of the contract interface and explain the robustness approach we're taking. Meaning, we can make the framework (in this case, the lambda runtime) more resilient than the contract it accepts data from via such defensive coding. While this may seem like a technique we don't need as much of in Rust, certainly the world outside of Rust does not abide by Rust assumptions, so at the edges, we still use resilient/robust techniques instead of falling back to fragility.

I think of it this way, if I'm an engineer/architect that wants to push my team to the latest platform, but I see it has a bug in headers it passes, I'm blocked on AWS fixing it before I can take advantage of latest bug fixes/improvements, putting AWS' Rust ecosystem at a needless disadvantage vis-a-vis other runtimes.

I realize assuming Context contract as golden means the runtime itself can make assumptions and more easily maintain future instrumentation, etc... but in that too, we should have graceful failure modes. So it might be a better assumption to data clean just inside the interface anyway.

Curious to hear your thoughts, @maintainers, and if you buy what I'm saying here.

coltonweaver · 2021-07-25T20:19:53Z

Apologies for the radio silence, all. I've been on vacation a bit and also been very busy with another deliverable at work so I didn't have time to check everything out in detail. I've come back to looking at this now.

In the past I took a hard stance against setting defaults like this for the reasons you mentioned above, but at this point it appears this won't ever be resolved unless the runtime is more graceful in handling these missing headers for local environments. At this point I'm inclined to think the way you've done this PR right now is fine. I like that you provided default strings that make it abundantly clear that something funky is going on if someone were to run into this in production. I think I'm inclined to keep it this way instead of making values in the Lambda context optional, as that isn't really reflective of reality when run outside of SAM.

I'm curious to know what @bahildebrand thinks about this, but I think this PR is acceptable, personally.

coltonweaver · 2021-07-25T20:27:47Z

lambda-runtime/src/types.rs

-                .to_str()
-                .expect("Missing Request ID")
+            request_id: headers
+                .get("lambda-runtime-aws-request-id")


Good catch, thanks for fixing this.

coltonweaver · 2021-07-25T20:28:25Z

lambda-runtime/src/types.rs

+            invoked_function_arn: headers
+                .get("lambda-runtime-invoked-function-arn")
+                .unwrap_or(&HeaderValue::from_static(
+                    "No header lambda-runtime-invoked-function-arn found.",


Good call. One of my original worries was if someone was using the ARN in some way it would be annoying to debug if the default looked like a real value.

coltonweaver · 2021-07-27T16:58:26Z

I'm going to go ahead and merge this, if there are any issues with this approach we can discuss.

coltonweaver · 2021-07-27T16:58:50Z

Thanks much for the contribution! @kenshih

…rust-runtime#332, hopefully get rid of two flukes from the list

brainstorm · 2021-07-29T02:09:58Z

This works neatly! Could you produce a release? I'm pinning to a commit right now, but I reckon that a ton of users would benefit from this.

…-runtime#332

coltonweaver · 2021-07-29T16:02:45Z

@brainstorm Sure thing, I will work on getting 3.1 published today or tomorrow.

coltonweaver · 2021-08-05T17:02:34Z

@brainstorm Okay, all pushed. Apologies on the delay of a couple days!

asg0451 · 2021-08-05T17:43:08Z

It works beautifully, thanks!

Fix sam local support for missing headers (awslabs#38)

95c8e21

* Added unit tests for TryFrom<HeaderMap> for Context

kenshih commented Jun 19, 2021

View reviewed changes

kenshih marked this pull request as ready for review June 19, 2021 03:28

This was referenced Jun 19, 2021

Can't get this working with "sam local" #38

Closed

sam local start-api broken due to missing lambda-runtime-invoked-function-arn header since v1.13.0 aws/aws-lambda-runtime-interface-emulator#11

Open

kenshih marked this pull request as draft June 19, 2021 16:49

kenshih marked this pull request as ready for review July 15, 2021 13:50

brainstorm mentioned this pull request Jul 20, 2021

AWS lambda runtime integration umccr/htsget-rs#47

Closed

coltonweaver approved these changes Jul 25, 2021

View reviewed changes

coltonweaver requested a review from bahildebrand July 25, 2021 20:30

coltonweaver merged commit 7e2cd97 into awslabs:master Jul 27, 2021

brainstorm added a commit to brainstorm/s3-rust-noodles-bam that referenced this pull request Jul 29, 2021

Switch to commit that addresses sam local issues: awslabs/aws-lambda-…

d7ea103

…rust-runtime#332, hopefully get rid of two flukes from the list

brainstorm added a commit to brainstorm/s3-rust-noodles-bam that referenced this pull request Jul 29, 2021

Many of the issues got fixed thanks to the PR awslabs/aws-lambda-rust…

0fccbab

…-runtime#332

brainstorm mentioned this pull request Aug 3, 2021

aws sam local start-api regression on "provided" runtime missing header "lambda-runtime-invoked-function-arn" aws/aws-sam-cli#2715

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sam local support for missing headers (#38) #332

Fix sam local support for missing headers (#38) #332

kenshih commented Jun 19, 2021

kenshih Jun 19, 2021 •

edited

Loading

coltonweaver Jul 25, 2021

kenshih Jun 19, 2021

kenshih Jun 19, 2021

coltonweaver Jul 25, 2021

kenshih Jun 19, 2021 •

edited

Loading

kenshih Jun 19, 2021

kenshih commented Jun 19, 2021 •

edited

Loading

kenshih commented Jun 19, 2021 •

edited

Loading

kenshih commented Jun 19, 2021

asg0451 commented Jul 6, 2021

brainstorm commented Jul 8, 2021 •

edited

Loading

kenshih commented Jul 15, 2021 •

edited

Loading

coltonweaver commented Jul 25, 2021

coltonweaver Jul 25, 2021

coltonweaver Jul 25, 2021

coltonweaver commented Jul 27, 2021

coltonweaver commented Jul 27, 2021

brainstorm commented Jul 29, 2021

coltonweaver commented Jul 29, 2021

coltonweaver commented Aug 5, 2021

asg0451 commented Aug 5, 2021

Fix sam local support for missing headers (#38) #332

Fix sam local support for missing headers (#38) #332

Conversation

kenshih commented Jun 19, 2021

kenshih Jun 19, 2021 • edited Loading

Choose a reason for hiding this comment

coltonweaver Jul 25, 2021

Choose a reason for hiding this comment

kenshih Jun 19, 2021

Choose a reason for hiding this comment

kenshih Jun 19, 2021

Choose a reason for hiding this comment

coltonweaver Jul 25, 2021

Choose a reason for hiding this comment

kenshih Jun 19, 2021 • edited Loading

Choose a reason for hiding this comment

kenshih Jun 19, 2021

Choose a reason for hiding this comment

kenshih commented Jun 19, 2021 • edited Loading

kenshih commented Jun 19, 2021 • edited Loading

kenshih commented Jun 19, 2021

asg0451 commented Jul 6, 2021

brainstorm commented Jul 8, 2021 • edited Loading

kenshih commented Jul 15, 2021 • edited Loading

coltonweaver commented Jul 25, 2021

coltonweaver Jul 25, 2021

Choose a reason for hiding this comment

coltonweaver Jul 25, 2021

Choose a reason for hiding this comment

coltonweaver commented Jul 27, 2021

coltonweaver commented Jul 27, 2021

brainstorm commented Jul 29, 2021

coltonweaver commented Jul 29, 2021

coltonweaver commented Aug 5, 2021

asg0451 commented Aug 5, 2021

kenshih Jun 19, 2021 •

edited

Loading

kenshih Jun 19, 2021 •

edited

Loading

kenshih commented Jun 19, 2021 •

edited

Loading

kenshih commented Jun 19, 2021 •

edited

Loading

brainstorm commented Jul 8, 2021 •

edited

Loading

kenshih commented Jul 15, 2021 •

edited

Loading