-
Notifications
You must be signed in to change notification settings - Fork 485
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
Add 6MB request and response size check #1990
Conversation
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Services/LambdaClient.cs
Show resolved
Hide resolved
{ | ||
testOutputHelper.WriteLine($"Testing endpoint: http://localhost:{apiGatewayPort}/{routeName}"); | ||
using (var client = new HttpClient()) | ||
{ | ||
client.Timeout = TimeSpan.FromSeconds(2); | ||
client.Timeout = TimeSpan.FromSeconds(60); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the part that is having some issues on the pipeline that i am investigating still
{ | ||
// lambda client automatically adds some extra verbiage: "The service returned an error with Error Code xxxx and HTTP Body: <bodyhere>". | ||
// removing the extra verbiage to make the error message smaller and look better on the ui. | ||
_errorMessage = e.Message.Split("HTTP Body: ")[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case the error message ever changes can you make the code defensive? If the split returns back a single token then just use that token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in latest version
{ | ||
if (settings.ApiGatewayEmulatorMode.Equals(ApiGatewayEmulatorMode.HttpV2)) | ||
var endpoint = routeConfig.Endpoint ?? $"http://{settings.LambdaEmulatorHost}:{settings.LambdaEmulatorPort}"; | ||
lambdaClient.SetEndpoint(endpoint); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There could be multiple threads using this method and each changing the endpoint of a singleton could cause problems. We might someday support having multiple Lambda emulators running inside Aspire and there could be different endpoints. Maybe get rid of the SetEndpoint
method and overload the InvokeAsync
to take in an endpoint since there is no rule it has to match the IAmazonLambda
interface. Then in LambdaClient
you can get rid of the _currentEndpoint
global state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated in latest version
/// </summary> | ||
public class LambdaClient : ILambdaClient, IDisposable | ||
{ | ||
internal Dictionary<string, IAmazonLambda> Clients => _clients; // used for unit tests only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there can be multiple threads using this you need to make access to this dictionary thread safe. Maybe switch to ConcurrentDictionary.
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
namespace Amazon.Lambda.TestTool.Models; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wasnt sure where to put this in models folder or in LambdaClient file/folder
|
||
builder.Services.Configure<LambdaOptions>(options => | ||
{ | ||
options.Endpoint = $"http://{settings.LambdaEmulatorHost}:{settings.LambdaEmulatorPort}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seemed like the simplest way of passing this value in
// removing the extra verbiage to make the error message smaller and look better on the ui. | ||
_errorMessage = e.Message.Contains("HTTP Body: ") | ||
? e.Message.Split("HTTP Body: ")[1] | ||
: e.Message; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Fix the formatting of the }
.
}; | ||
}; | ||
|
||
Environment.SetEnvironmentVariable("AWS_LAMBDA_RUNTIME_API", $"localhost:{lambdaPort}/largerequestfunction"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coordinate with @philasmar PR so that you can set the API via a command line instead of environment variable. #1992
|
||
var handler = (APIGatewayHttpApiV2ProxyRequest request, ILambdaContext context) => | ||
{ | ||
var env = Environment.GetEnvironmentVariable("AWS_LAMBDA_RUNTIME_API"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coordinate with @philasmar PR so that you can set the API via a command line instead of environment variable. #1992
}; | ||
}; | ||
|
||
Environment.SetEnvironmentVariable("AWS_LAMBDA_RUNTIME_API", $"localhost:{lambdaPort}/largerequestfunction"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coordinate with @philasmar PR so that you can set the API via a command line instead of environment variable. #1992
{ | ||
<div class="text-danger ms-2"> | ||
<i class="bi bi-exclamation-triangle-fill"></i> | ||
@_errorMessage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message you are adding will wrap if the screen is smaller than yours. This is what it would look like:
Im thinking instead of displaying a message directly, use that danger icon and align it to the end of that div and when the user hovers over it they see the message as a tooltip?
Or put the error message on a line below "Function Input" if it doesn't fit next to it.
{ | ||
FunctionName = SelectedFunctionName, | ||
Payload = payload, | ||
InvocationType = InvocationType.Event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the invocation type "Event" and not "RequestResponse"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the original implementation was also set to Event type (When invocated as DataStore.QueueEvent(editorValue, false);
, the false
was for the isRequestResponse
boolean, so kept it the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Event is what we want here because we just want to queue the event. We don't want to block the UI waiting for the response.
} | ||
catch (AmazonLambdaException e) | ||
{ | ||
// lambda client automatically adds some extra verbiage: "The service returned an error with Error Code xxxx and HTTP Body: <bodyhere>". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: log the entire error using the logger
/// - JSON error message | ||
/// - Content-Type header set to application/json | ||
/// </returns> | ||
public static APIGatewayProxyResponse ToHttpApiRequestTooLargeResponse(ApiGatewayEmulatorMode emulatorMode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, does this not apply to HttpV2? If not, maybe add a note to the method docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
theres a separate function for httpv2, ill update the docs
IsBase64Encoded = false | ||
}; | ||
} | ||
throw new InvalidOperationException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add a message for why things went wrong
/// - JSON error message | ||
/// - Content-Type header set to application/json | ||
/// </returns> | ||
public static APIGatewayHttpApiV2ProxyResponse ToHttpApiV2RequestTooLargeResponse() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: same comment about naming
|
||
namespace Amazon.Lambda.TestTool.Models; | ||
|
||
public class LambdaOptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add docs for class and members
d62319a
to
cf39cc7
Compare
Issue #, if available: DOTNET-7881
Description of changes:
Screenshots
Lambda (i hardcoded the max size to 1 byte for testing which is why the error message says 1 byte)


Api gateway


Why
This is to mimic lambdas 6MB limit defined here https://docs.aws.amazon.com/lambda/latest/dg/gettingstarted-limits.html.
How
PostEvent
is the main entry point for where we can check the request size. This is invoked when the lambda client does invokeRequest. One thing to note is that I had to update the UI code to use a lambda client, rather than adding to the data store directly in order to reachPostEvent
.PostInvocationResponse
is the main entry point for where we can check the response size. This is invoked when the lambda returns its response.LambdaClient
injectable and have it reused across requests (if the endpoint is the same it will reuse it)Testing real world behavior
First I validated what the real world behavior of lambda/api gateway was for request/responses exceeding the limit.
Lambda Testing for request size
413
response saying that the payload size exceeded the limit. (I tried both regular request/response mode and also event type invocation and they both immediately give back 413)Lambda Testing for response size
200
success code with error details populated saying the response size exceeded the limit. Note that this error handling behavior is identical to how other errors in lambda are handled, where a successful status code is returned, but the errors are populated in the body.Api gateway Testing for request size
413
response code (with some slight differences in the error message depending on the type (HTTPV1, V2, REST)Api gateway Testing for response size
Testing test tool
1, I ran the test tool locally and manually updated the limit to 1 byte and invoked it with request/responses exceeding the limit (see screenshots above)
2. I created unit test and an integration test to validate the behavior
3. All existing tests pass
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.