-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat(LocalLambdaService): Invoke Local Lambdas through a Local Lambda Service #508
Conversation
|
||
Parameters | ||
---------- | ||
function_name_list list of str |
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.
I had two options here:
- Use a list of Function Logical Ids
- A list of Function NamedTuple
I went with the first to not couple them together but I think it might be ok. Wanted to get your thoughts on this @sanathkr
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 looks good. The NamedTuple has too much information which is not a concern of this class
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.
Ya that is why I originally leaned this way but couldn't completely convince myself it was right. :)
self._app = Flask(__name__) | ||
|
||
for function_name in self.function_name_list: | ||
path = '/2015-03-31/functions/{}/invocations'.format(function_name) |
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.
At the moment, I create a path for each function but not sure this is the right approach at the moment. There is a date (api version) in the path of the API and am not sure if we should be hardcoding that into the path for Flask. Instead, we can catch all paths and do some path matching (we have to do some of this already to get the Function Logical Id out of the path). If we do the path matching, we can ignore the first part (2015-03-31). This would make our code a little more future proof but we have do handle most of the error cases and route handling ourselves (and not get some of this from Flask).
I am leaning towards accepting all paths and doing the path matching/inspection but haven't fully convinced myself yet. Thoughts?
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.
Invoke might be version locked today, but could change in the future. Since we only support one Invoke API, this is good for now. It doesn't lead us thru one way doors and can be changed easily in the future.
@sanathkr I made two comments and things I would like your thoughts on :) |
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.
Great start! I am excited about this 🎉
LOG = logging.getLogger(__name__) | ||
|
||
|
||
class LocalLambdaService(BaseService): |
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.
worth calling this something other than LocalLambdaService
. "Service" isn't too clear on what it means
|
||
class LocalLambdaService(BaseService): | ||
|
||
_DEFAULT_PORT = 3001 |
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.
I don't like hard-coding defaults like port & host deep within this class. Let's make the port mandatory & have it dependency injected by the caller. The caller could read from a config file or from CLI args.
We did this for the APIGW service but I don't think it is a good idea there either
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.
Make sense. I can't recall why I did this initially for APIGW originally. Will work on cleaning this up (and for APIGW as well).
|
||
Parameters | ||
---------- | ||
function_name_list list of str |
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 looks good. The NamedTuple has too much information which is not a concern of this class
self._app = Flask(__name__) | ||
|
||
for function_name in self.function_name_list: | ||
path = '/2015-03-31/functions/{}/invocations'.format(function_name) |
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.
Invoke might be version locked today, but could change in the future. Since we only support one Invoke API, this is good for now. It doesn't lead us thru one way doors and can be changed easily in the future.
if not self._app: | ||
raise RuntimeError("The application must be created before running") | ||
|
||
# Flask can operate as a single threaded server (which is default) and a multi-threaded server which is |
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.
so this logic is spread in two classes now? This one and the APIGW Service?
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.
Ya. I didn't get far enough to get this abstracted yet but it totally should be. Will work on getting that and cleaning up the current 'Base' class.
""" | ||
flask_request = request | ||
|
||
function_name_regex = re.compile(r'/2015-03-31/functions/(.*)/invocations') |
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.
you don't need a regex here. Flask should have already parsed the {}
part out from the path.
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.
I constructed every endpoint but instead, I can move to letting flask do this. It will clean up a lot of this code, which is great. Will update.
stdout_stream = io.BytesIO() | ||
|
||
try: | ||
self.lambda_runner.invoke(function_name, {}, stdout=stdout_stream, stderr=self.stderr) |
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 event
parameter empty dict? We should be passing the request's body to 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.
Because I didn't get that far just yet :). I focused on just getting the main details working. It will be apart of the next commit.
self.assertEquals(response.status_code, 200) | ||
|
||
|
||
def make_service(list_of_function_names, function_provider, cwd): |
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.
Move all the make_service
and random_port
to a util class. Too many places where this is repated
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.
I choose to ignore this but realize we need to deal with this sooner rather than later. I will try to setup a 'framework' functional tests can follow like the one we have for integ. Updating the existing functional tests will come later though. I want to keep this PR scoped (I already feel it is getting a little large).
@@ -0,0 +1,128 @@ | |||
"""Local Lambda Service""" | |||
|
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.
Can we rename the file to something else? The word "service" is very generic, even though I know the file is creating a service
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.
We can. :)
self._app = Flask(__name__) | ||
|
||
for function_name in self.function_name_list: | ||
path = '/2015-03-31/functions/{}/invocations'.format(function_name) |
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, I think the path must respond to /lambda/2015....
because, then users can set the general AWS endpoint to be localhost:3001
instead of overriding just the Lambda one. What do you think?
Documenting here for reference but not part of this PR: This command should automatically create and use a default Docker network called var lambda = new AWS.Lambda()
if(process.env.AWS_SAM_LOCAL) {
// only run inside local lambda runner
// Note the endpoint name
var ep = new AWS.Endpoint('samcliproxy.example.com:3001');
var lambda = new AWS.Lambda({endpoint: ep})
} This will allow one local Lambda to call another locally. |
@sanathkr Updated base on your comments. I still think this is WIP. I still need up to update some naming (more on files/paths) and I combines to classes into one for because it was easy but they do not belong together (in the localhost runner file). It felt wrong to create yet another file for some 'helper related methods'. But would still like your thoughts on what is here. |
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 looks much better! I have a few minor suggestions to cleanup the base class, nothing too critical.
|
||
def __init__(self, lambda_runner, port, host, stderr=None): | ||
""" | ||
Creates a BaseService class |
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.
update class name
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.
Missed that one.
return response | ||
|
||
|
||
class LambdaOutputParser(object): |
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.
optional: doesn't need a class. It could be just its own top-level method
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.
I am going to leave this as a class for now. We need to read the output for the Lambda to see if the return value from the container is a user exception/error. This is so we can properly handle some error cases and setting headers correctly. I was planning to use this class to contain that as well. I may just move it then but want to see how the structure/code looks after I handle that.
Optional. port for the service to start listening on Defaults to 3000 | ||
host str | ||
Optional. host to start the service on Defaults to '127.0.0.1 | ||
stderr io.BaseIO |
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.
stderr
is not used in this class
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.
I originally placed this here because it was used in both Local APIGW and Local Lambda. Since it is unused here, I pushed this up the hierarchy.
|
||
Parameters | ||
---------- | ||
lambda_runner samcli.commands.local.lib.local_lambda.LocalLambdaRunner |
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 used only to set is_debugging
flag. Instead, can't you just take a is_debugging
flag in the constructor and remove lambda_runner from it? That way this class is only about encapsulating Flask. So any regular class can inherit from this one to immediately become a "service".
In this light, BaseService
makes sense for class name, or BaseLocalService
. Subclasses could be LocalApigwService
and LocalLambdaService
. I won't nit pick on the names, but this is my two cents to keep them consistent.
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.
Good call out. Will update the lambda_runner
to is_debugging
.
NAMING IS HARD!!! :) I like your suggestion more. Will update.
# TODO Change this | ||
raise Exception('Change this later') | ||
|
||
lambda_response, lambda_logs = LambdaOutputParser.get_lambda_output(stdout_stream) |
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.
Thanks for the refactor, this method reads fluently :)
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.
Looks great! I like the new class name. Suits well :) Do you want me to approve & merge this PR while you build on top of this?
Let's get this merged if you feel it is ready. I have spun off a branch locally that has the error handling cases (based on this already). I prefer incremental and scoped changes :) I forgot to update the title and issue above. Let me do that now. |
…quest Details: * Moved shared code between APIGW and Lambda service to a Base Class * Updated the Invoke function on the LocalLambdaService to invoke the Local Lambda based on the request * Updated Functional tests for invoking the Local Lambda
Details: * Updated some names * Forwarded request body to the event of the invoke * Added functional tests for echoing the event sent to lambda * More refactoring around shared code
87cd117
to
628a506
Compare
@sanathkr I force updated with the changes for |
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.
LGTM
@sanathkr Is this supported in 0.4.0? I am trying to trigger a lambda locally using another lambda which is triggered by API GW. Do I need to add an Edit1: I upgraded to 0.6.0 and I can start the lambda service locally. However, I still can't trigger the lamdba locally. Do I need to specify anything for events in the CloudFormation template? Edit2: I got this working. I was running into a ECONNREFUSED error for 127.0.0.1. I have worked around it by aliasing the loopback address. Please let me know if there is a better approach to this. |
@rohandesai, can you confirm how you got this working? I'm trying to do: API Gateway ➡️
package main
import (
"github.com/aws/aws-lambda-go/lambda"
"github.com/aws/aws-lambda-go/events"
"github.com/aws/aws-sdk-go/aws/session"
"github.com/aws/aws-sdk-go/aws"
lambdasvc "github.com/aws/aws-sdk-go/service/lambda"
"os"
)
var svc *lambdasvc.Lambda
func init() {
if os.Getenv("AWS_SAM_LOCAL") == "true" {
const e = "127.0.0.1:3000"
svc = lambdasvc.New(session.Must(session.NewSession()), aws.NewConfig().WithEndpoint(e))
} else {
svc = lambdasvc.New(session.Must(session.NewSession()))
}
}
func main() {
lambda.Start(func (_ events.APIGatewayProxyRequest) (events.APIGatewayProxyResponse, error) {
input := &lambdasvc.InvokeInput{
ClientContext: aws.String("Test"),
FunctionName: aws.String("Function2"),
InvocationType: aws.String("Event"),
}
r, err := svc.Invoke(input)
if err != nil {
return events.APIGatewayProxyResponse{}, err
}
return events.APIGatewayProxyResponse{
Body: r.GoString(),
}, nil
})
}
AWSTemplateFormatVersion : '2010-09-09'
Transform: AWS::Serverless-2016-10-31
Resources:
Function1:
Type: AWS::Serverless::Function
Properties:
Runtime: go1.x
Handler: function1/function1
Events:
HttpHandler:
Type: Api
Properties:
Path: '/'
Method: get
Function2:
Type: AWS::Serverless::Function
Properties:
Runtime: go1.x
Handler: function2/function2 |
@markwilson Try aliasing the loopback address by using the following command - |
I tried running this:-
... and updated to use It didn't work though. I'll keep trying... It would be good to get some official docs on how this is supposed to work 😅 |
Is there any official documentation by now? |
@markwilson I needed to use host.docker.internal:3000 to connect to the endpoint on the host machine since the function code runs inside a container. |
I have created two lambdas -validateuserpermission and updateuserpermission. Here is the situation updateuserpermission calls validateuserpermission internally. "Could not connect to the endpoint URL: "http://127.0.0.1:3001/2015-03-31/functions/validateuserpermission/invocations\"" ON AWS console this is working but not able to work it from SAM local. However, if validateuserpermission invocation is removed from updateuserpermission , both lambda works fine in SAM LOCAL. Could you please help me in resolving this issue so that i would be able to test this situation. |
+1 Any update on this? We need an officially supported way to locally invoke Lambda A from API Gateway, which in turn invokes Lambda B, built by the SAM CLI |
For anyone wondering how to use it, here is an example: sam local start-lambda --port 3000 --region local You can then invoke your lambda through the aws-sdk (Example in JavaScript): const { Lambda } = require('aws-sdk');
const lambda = new Lambda({
endpoint: 'http://localhost:3000',
region: 'local',
});
lambda
.invoke({
FunctionName: 'myFunction',
InvocationType: 'RequestResponse',
Payload: JSON.stringify({ hello: 'world' }),
})
.promise()
.then((result) => {
console.log('Received Response:', result.Payload);
})
.catch((error) => {
console.error('Error:', error);
}); You could also run this code from inside another local Lambda. |
Is there a python example? I can't seem to find a way to do this from one python function to another locally, and I'm not sure boto is respecting any options I pass it when I get the lambda client, i.e. lambda_client = boto3.client('lambda',
endpoint_url="http://127.0.0.1:3001",
use_ssl=False,
verify=False) |
* Add nodejs20.x support for samcli * Add nodejs20.x as preview runtime * change github action to install nodejs 20.x --------- Co-authored-by: Mohamed Elasmar <[email protected]>
* Add nodejs20.x support for samcli (#508) * Add nodejs20.x support for samcli * Add nodejs20.x as preview runtime * change github action to install nodejs 20.x --------- Co-authored-by: Mohamed Elasmar <[email protected]> * move test to arm64 --------- Co-authored-by: Andrea Culot <[email protected]> Co-authored-by: Mohamed Elasmar <[email protected]> Co-authored-by: Haresh Nasit <[email protected]>
Details:
/2015-03-31/functions/<function_name>/invocations
for a POSTDescription of changes:
This was modeled off of the API GW local service.
Adds a class that is responsible for standing up a Local Lambda Service. Functional and unit test pass but the code is not used in any context outside of this. Currently, only the happy path is implemented.
I used the AWS CLI and pointed it to this localhost service to inspect the path and request. I have not done a deeper dive into the Lambda Service, so things may be different than the cloud service at the moment.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.