feat(LocalLambdaService): Error handling for local lambda #532

jfuss · 2018-07-05T14:49:23Z

Details:

Handling for when lambda returns an error
Handled payload could not be parsed as JSON
Abstracted if the lambda output is an error from the container into LambdaOutputParser
Added handling for X-Amz-Log-Type
Added handling for X-Amz-Invocation-Type
Moved all Request validate to flask.before_request call
Added handling for Query parameters (not supported)
Added handling for unsupported_media_type
Added handling for ServiceError
Added handling for generic 404 Error handling
Fix up tests to make them runable

Description of changes:
This is to handle the Error cases that can occur in the Local Lambda Service. This is meant to mimic the actually service with some caveats that are specific to local:

The generic 404 has the same response if the Lambda Function does not exits but with an empty string for the function. This is due to the generic 404 being for any endpoint that does not exist. We handle the functions not found if it follows the endpoint schema we setup. I am considering moving this to, be something along the lines of 'this endpoint does not exist' but haven't landed or changed this yet.
Any Service error will be a 'generic' 500 error.
Currently, we only supported X-Amz-Log-Type = 'None' and X-Amz-Invocation-Type = 'RequestResponse'
The arn of the function given in the ResourceNotFound 404 is hardcoded with an account id and region: us-west-2. The region is passed to the service through a 'context' in the request: context': {'auth_type': None, 'client_region': u'us-west-2', but Flask seems to swallow this part of the request and have not found a way to get at this information.

Work In Progress
Following items are remaining:

Add Functional Tests for each Error case handled.
Add Unit tests
validate error cases match the Lambda Service (note: some will be different and specific to local only)
Test the LocalLambdaInvoke by using the AWS CLI to call
Some general code cleanup

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Details: * Handling for when lambda returns an error * Handled payload could not be parsed as JSON * Abstracted if the lambda output is an error from the container into LambdaOutputParser * Added handling for X-Amz-Log-Type * Added handling for X-Amz-Invocation-Type * Moved all Request validate to flask.before_request call * Added handling for Query parameters (not supported) * Added handling for unsupported_media_type * Added handling for ServiceError * Added handling for generic 404 Error handling * Fix up tests to make them runable

sanathkr · 2018-07-05T19:57:26Z

Thanks for the detailed information in this PR. Really helps to set context. I will take a peek at the code.

sanathkr

General idea looks good. I have some comments to improve readability and highlighted one bug.

sanathkr · 2018-07-05T19:59:24Z

samcli/local/services/base_local_service.py

+            lambda_response_dict = json.loads(lambda_response)
+
+            if isinstance(lambda_response_dict, dict) and \
+                    len(lambda_response_dict.keys()) == 3 and \


just the len(lambda_respons_dict) will give you 3. Dont need keys()

duh... :) will update

sanathkr · 2018-07-05T20:00:47Z

samcli/local/services/base_local_service.py

-        return lambda_response, lambda_logs
+        is_lambda_user_error_response = False
+
+        try:


Can you move this to a separate method for readability & testability? This whole blob is only to check if this is an error response.

sanathkr · 2018-07-05T20:07:20Z

samcli/local/services/base_local_service.py

+            if isinstance(lambda_response_dict, dict) and \
+                    len(lambda_response_dict.keys()) == 3 and \
+                    'errorMessage' in lambda_response_dict and \
+                    'errorType' in lambda_response_dict and \


errorType and stackTrace are required. For example, this just returns {"errorMessage": "error" } (I verified by calling Lambda). But if you returned a regular exception like TypeError, then the other two fields are returned.

exports.handler = (event, context, callback) => { // TODO implement callback('error'); };

sanathkr · 2018-07-05T20:08:51Z

samcli/local/lambda_service/lambda_error_responses.py

+
+    @staticmethod
+    def resource_not_found(function_name):
+        """


I assume you will fill in the doc strings later

Yup. Didn't get that far in life yet :)

sanathkr · 2018-07-05T20:09:22Z

samcli/local/lambda_service/lambda_error_responses.py

+        )
+
+    @staticmethod
+    def _service_response(body, headers, status_code):


Isn't this method already in the BaseService class? Can we use from there?

It is and will combine this for the lambda_error_response and the api gateway one as well.

sanathkr · 2018-07-05T20:16:07Z

samcli/local/lambda_service/local_lambda_invoke_service.py

+            return LambdaErrorResponses.invalid_request_content(
+                "Could not parse request body into json: {}".format(str(json_error)))
+
+        if flask_request.args:


This is a POST request. so query params are less of a problem. But anyway, good to handle.

sanathkr · 2018-07-05T20:17:09Z

samcli/local/lambda_service/local_lambda_invoke_service.py

+
+        log_type = flask_request.headers.get('X-Amz-Log-Type', 'None')
+        if log_type != 'None':
+            LOG.info("log-type: %s is not supported. Ignoring X-Amz-Log-Type header", log_type)


debug instead?

I did this as info to display we are ignoring the X-Amz-Log-Type header, it feels wrong to only display that to users when the --debug is set. May be better to just exit there instead of still handling the request, which then it would make more sense to move this to debug.

I think exiting and returning error would be better. It is explicit and less surprises.

sanathkr · 2018-07-05T20:21:32Z

samcli/local/lambda_service/local_lambda_invoke_service.py

+        if log_type != 'None':
+            LOG.info("log-type: %s is not supported. Ignoring X-Amz-Log-Type header", log_type)
+
+        invocation_type = flask_request.headers.get('X-Amz-Invocation-Type', 'RequestResponse')


Headers names are case sensitive?

They are not (just started with whatever AWS CLI sends). We have a CaseInsensitiveDictionary in the API GW class. Should be able to move that and use it here for all the header look ups.

That would be helpful. Can you include that in your change?

Yup. the move was easy :)

sanathkr · 2018-07-05T20:23:45Z

samcli/local/lambda_service/local_lambda_invoke_service.py

    def _construct_error_handling(self):
        """
        Updates the Flask app with Error Handlers for different Error Codes

        """
-        pass
+        self._app.register_error_handler(500, LambdaErrorResponses.generic_service_exception())
+        self._app.register_error_handler(404, LambdaErrorResponses.resource_not_found(""))


What is the downside of letting the FunctionNotFound handle this? Having the function name there in response is useful.

This is for the generic 404, aka when a request is sent to an endpoint that does not exist (I called this out in the description above). I think it makes more sense to move this to being a path not found instead of resource not found.

sanathkr · 2018-07-05T20:28:33Z

samcli/local/lambda_service/local_lambda_invoke_service.py


-        lambda_response, lambda_logs = LambdaOutputParser.get_lambda_output(stdout_stream)
+        if is_lambda_user_error_response:
+            return self._service_response(lambda_response,


This is the response I got from Lambda Service on a sample invoke that failed. We are missing x-amzn-requestid, date, x-amz-executed-version from these. Can we add them?

In fact, can we add a requestId header to every invocation?

{ "date": "Thu, 05 Jul 2018 20:24:47 GMT", "content-type": "application/json", "content-length": "114", "connection": "keep-alive", "x-amzn-requestid": "754e50c3-8091-11e8-afcc-a3b411a0cfac", "x-amz-function-error": "Unhandled", "x-amzn-remapped-content-length": "0", "x-amz-executed-version": "$LATEST", "x-amzn-trace-id": "root=1-5b3e7e8e-81e41598a657a40c4ad823a6;sampled=0" }

The lambda docker does not allow any requestId setting (it generates the uuid on the fly). So in order to make this experience clean, we would have to parse the output of the logs for the requestId and then set the header. That seems overkill. As of know, I am going to omit the requestId header.

Adding x-amz-executed-version makes sense to me and will default to $LATEST (since we do not support qualifiers).

Not sure what x-amzn-remapped-content-length means or how it is commuted, so was going to omit it.

x-amzn-trace-id only makes sense if we support XRay right?

Okay, let's omit requestId. How about setting x-amz-executed-version to $LATEST because there is no version locally?

Following up, what if they use AutoPublishAlias and try to invoke the alias? I am okay if we don't support aliases now. Doesn't hurt.

From my response :):

Adding x-amz-executed-version makes sense to me and will default to $LATEST (since we do not support qualifiers).

Qualifiers might be easy to just add but we don't have a concept of a qualifier locally. I think I will hold off on adding the qualifier for now.

sanathkr

Can you answer the decode comment?

sanathkr · 2018-07-06T19:04:49Z

samcli/local/services/base_local_service.py

+        is_lambda_user_error_response = False
+
+        try:
+            lambda_response_dict = json.loads(lambda_response)


Also, you need to decode the lambda_response before json.loads it. Might be worth moving the decode to a upstream call somewhere. See this comment: #531 (comment)

…aInvokeService

…for is_lambda_error_response

… paths

sanathkr

LGTM 🎉

* Add support for custom Log group in samcli * Added more unit test * Updated pr to use f string

* Add support for Custom Log Group (#532) * Add support for custom Log group in samcli * Added more unit test * Updated pr to use f string * SAM INIT support for Advanced Logging (#556) * SAM Init support for Advanced Logging * feature links added * SAM Init support for Advanced Logging * change class named from LoggingConfigTemplateModifier to LoggingConfigTemplateModifier * update file name to match structured_logging_template_modifier * Sam local Invoke support for Advanced logging (#552) * Sam local support for Advanced logging * Sam local support for Advanced logging * pass AWS_LAMBDA_LOG_FORMAT to runtime * remove unnecessary if statement * Update test_init_command.py --------- Co-authored-by: jonife <[email protected]>

jfuss requested a review from sanathkr July 5, 2018 14:49

sanathkr suggested changes Jul 5, 2018

View reviewed changes

jfuss mentioned this pull request Jul 6, 2018

[Feature request] offline mode / local cache #529

Closed

sanathkr suggested changes Jul 6, 2018

View reviewed changes

sanathkr mentioned this pull request Jul 6, 2018

Getting an invalid response with new line symbol at the response on SAM CLI 0.4.0 #531

Closed

jfuss added 7 commits July 6, 2018 14:49

Handled some PR feedback and finished functional tests for LocalLambd…

72232aa

…aInvokeService

Move CaseInsensiveDict test to test_base_local_service and add tests …

6f01f5b

…for is_lambda_error_response

Updated and add tests for LocalLambdaInvokeService for error handling…

f8524b7

… paths

Added unit tests for LambdaErrorResponses class

7d4dc19

Fix linting

0755bc0

Update tests to support Py2 and Py3 differences

47cbb47

Added docs to BaseLocalService.is_lambda_user_error_response

8760cde

sanathkr approved these changes Jul 9, 2018

View reviewed changes

sanathkr changed the title ~~[WIP] feat(LocalLambdaService): Error handling for local lambda~~ feat(LocalLambdaService): Error handling for local lambda Jul 9, 2018

Merge branch 'develop' into lls-error-handling

ee9f3d5

sanathkr merged commit 23ffd03 into aws:develop Jul 9, 2018

hawflau pushed a commit to hawflau/aws-sam-cli that referenced this pull request Nov 15, 2023

Add support for Custom Log Group (aws#532)

e32d800

* Add support for custom Log group in samcli * Added more unit test * Updated pr to use f string

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(LocalLambdaService): Error handling for local lambda #532

feat(LocalLambdaService): Error handling for local lambda #532

jfuss commented Jul 5, 2018 •

edited

Loading

sanathkr commented Jul 5, 2018

sanathkr left a comment

sanathkr Jul 5, 2018

jfuss Jul 5, 2018

sanathkr Jul 5, 2018

jfuss Jul 5, 2018

sanathkr Jul 5, 2018

sanathkr Jul 5, 2018

jfuss Jul 5, 2018

sanathkr Jul 5, 2018

jfuss Jul 5, 2018

sanathkr Jul 5, 2018

sanathkr Jul 5, 2018

jfuss Jul 5, 2018

sanathkr Jul 6, 2018

sanathkr Jul 5, 2018

jfuss Jul 5, 2018

sanathkr Jul 6, 2018

jfuss Jul 6, 2018

sanathkr Jul 5, 2018

jfuss Jul 5, 2018

sanathkr Jul 5, 2018

jfuss Jul 5, 2018

sanathkr Jul 6, 2018

jfuss Jul 6, 2018

sanathkr left a comment

sanathkr Jul 6, 2018

sanathkr left a comment

feat(LocalLambdaService): Error handling for local lambda #532

feat(LocalLambdaService): Error handling for local lambda #532

Conversation

jfuss commented Jul 5, 2018 • edited Loading

sanathkr commented Jul 5, 2018

sanathkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanathkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanathkr left a comment

Choose a reason for hiding this comment

jfuss commented Jul 5, 2018 •

edited

Loading