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

#814 Add Transport Layer: AWS Lambda #815

Merged
merged 31 commits into from
Feb 22, 2019

Conversation

suekto-andreas
Copy link
Contributor

@suekto-andreas suekto-andreas commented Dec 23, 2018

Follows up on issue #814, this is a proposal code on transport layer for building API service with AWS Serverless stack.

Kindly help to peruse, as for me personally this is very useful addition for developing in serverless stack.


Based on discussion with @dimiro1 we expand the scope of this PR to provide a more generic transport layer for awslambda by relying on lambda.Handler interface. Therefore the title is updated into #814 Add Transport Layer: AWS Lambda

@dimiro1
Copy link

dimiro1 commented Dec 23, 2018

@suekto-andreas Why not accept a generic event and then let the user decided how to decode and encode the request and response?

@suekto-andreas
Copy link
Contributor Author

@dimiro1 by generic event, would it means change the request & response type into interface{} ? (Please correct me if my assumption is wrong.)

The essential issue is due to AWS SDK behavior that if we provide with interface{} then any event coming in will become map[string]interface[], hence it become non-trivial for user to cast it into the proper event.

This root cause brings up the following impediments

  1. Non-trivial casting for user
    has to convert into JSON first then marshal it back to the proper event.
  2. We lost type safe / The interface of other facilities (ServerBefore, ServerAfter etc.) become not straightforward
    e.g. the EncodeResponseFunc become like this
    type EncodeResponseFunc func(
    ctx context.Context, response interface{}, resp interface{},
     ) (interface{}, error)
    
    its not straight forward to inform the user via the function definition itself, that this function actually demands a specific event type response. This makes we lost compile time error check, and would give impediment to user to use the full facilities of the transport layer, as there are more thing that user has to be careful about.
  3. When we specified the APIGatewayEvent directly the Encode / Decode is meant mainly like how http server is, such as to encode the proper string format in resp.Body (JSON/XML/YAML)

I think mainly because using interface{} is not equivalent as is the convenient of Java generic.

@dimiro1
Copy link

dimiro1 commented Dec 24, 2018

Hey @suekto-andreas, I was thinking about relying on the lambda.Handler as the entry point.

A few days ago I was playing with something very similar, this is what I am using in a Pet project. In the end it is not very different of the go-kit http package.

package lambda

import (
	"context"

	"github.com/go-kit/kit/endpoint"
	"github.com/go-kit/kit/log"
)

type DecodeRequestFunc func(context.Context, []byte) (interface{}, error)
type EncodeResponseFunc func(context.Context, interface{}) ([]byte, error)

type ServerRequestFunc func(ctx context.Context, payload []byte) context.Context
type ServerResponseFunc func(ctx context.Context, response interface{}) context.Context

type Server struct {
	e            endpoint.Endpoint
	dec          DecodeRequestFunc
	enc          EncodeResponseFunc
	before       []ServerRequestFunc
	after        []ServerResponseFunc
	errorEncoder ErrorEncoder
	logger       log.Logger
}

// ServerOption sets an optional parameter for servers.
type ServerOption func(*Server)

// ErrorEncoder is responsible for handling custom errors for AWS Lambda.
type ErrorEncoder func(error) error

// DefaultErrorEncoder the default behaviour is just return the same error.
func DefaultErrorEncoder(e error) error { return e }

// ServerErrorEncoder is used to return custom errors
// it can be used to control the state of the state machine in AWS Step Functions
//
// See: https://docs.aws.amazon.com/lambda/latest/dg/go-programming-model-errors.html
// See: https://docs.aws.amazon.com/step-functions/latest/dg/tutorial-handling-error-conditions.html
func ServerErrorEncoder(e ErrorEncoder) ServerOption {
	return func(s *Server) { s.errorEncoder = e }
}

// ServerBefore functions are executed on the AWS Lambda event before the event is decoded.
func ServerBefore(before ...ServerRequestFunc) ServerOption {
	return func(s *Server) { s.before = append(s.before, before...) }
}

// ServerAfter functions are executed on the response after the
// endpoint is invoked, but before anything is sent to the client.
func ServerAfter(after ...ServerResponseFunc) ServerOption {
	return func(s *Server) { s.after = append(s.after, after...) }
}

func NewServer(
	e endpoint.Endpoint,
	dec DecodeRequestFunc,
	enc EncodeResponseFunc,
	options ...ServerOption) *Server {
	s := &Server{
		e:            e,
		dec:          dec,
		enc:          enc,
		errorEncoder: DefaultErrorEncoder,
		logger:       log.NewNopLogger(),
	}
	for _, option := range options {
		option(s)
	}
	return s
}

// Handler it is the AWS Lambda entry point.
// This function is the one you have to pass to lambda.Start function.
func (s *Server) Invoke(ctx context.Context, payload []byte) ([]byte, error) {
	var (
		request  interface{}
		response interface{}
	)

	for _, f := range s.before {
		ctx = f(ctx, payload)
	}

	request, err := s.dec(ctx, payload)
	if err != nil {
		_ = s.logger.Log("err", err)
		return []byte{}, s.errorEncoder(err)
	}

	response, err = s.e(ctx, request)
	if err != nil {
		_ = s.logger.Log("err", err)
		return []byte{}, s.errorEncoder(err)
	}

	bytes, err := s.enc(ctx, response)
	if err != nil {
		_ = s.logger.Log("err", err)
		return []byte{}, s.errorEncoder(err)
	}

	for _, f := range s.after {
		ctx = f(ctx, response)
	}

	return bytes, nil
}
package myendpoint

import (
    awslambda "github.com/aws/aws-lambda-go/lambda"
    "my/endpoints"
)

func NewAWSLambdaHandler(endpoints Endpoints) awslambda.Handler {
	return lambda.NewServer(endpoints.MyServiceEndpoint, decodeAWSLambdaRequest, encodeAWSLambdaResponse)
}

func decodeAWSLambdaRequest(_ context.Context, payload []byte) (interface{}, error) {
	var req myRequestType

	err := json.Unmarshal(payload, &req)
	if err != nil {
		return req, err
	}

	return req, nil
}

func encodeAWSLambdaResponse(_ context.Context, response interface{}) ([]byte, error) {
	bytes, err := json.Marshal(response)
	if err != nil {
		return []byte{}, err
	}

	return bytes, nil
}
package main

func main() {
    handler := NewAWSLambdaHandler(myendpoints)
    awslambda.StartHandler(handler)
}

@suekto-andreas
Copy link
Contributor Author

suekto-andreas commented Dec 24, 2018

Many Thanks @dimiro1 ! Yes this is great, this makes the component much more generic.

I am refactoring the code as per your advice above.

Additionally want to add something like wrapper for the encoder/decoder so that it can be an equivalent of the AWS Lambda SDK's newHandler() wrapper.

btw Merry Chrismast !


Still tidying a bit and designing some wrapper.
Some little update - a bit of designs diff with yours suggestion above

  1. Add ServerFinalizer
    Sample use-case is for logging / metric recording purposes.
  2. ErrorEncoder interface
    type ErrorEncoder func(ctx context.Context, err error) ([]byte, error)
    
    By doing this we give power to ErrorEncoder to instead have smooth Error response, sample use-case is if we create API, and we want to customise error JSON response, we would want to encode a proper JSON response, and the error return by Invoke() to be nil; so that we can control the error messages received by our API Users - i.e. we can design developer experience.

…d for AWS APIGateway event handling, as if we return error not nil to lambda it shall treated as error, and the API response would not be pretty
@suekto-andreas suekto-andreas changed the title #814 Add Transport Layer: AWS Lambda APIGateway Events #814 Add Transport Layer: AWS Lambda Dec 26, 2018
@suekto-andreas
Copy link
Contributor Author

Hi @dimiro1 - have updated the PR, could you help to share your input/feedback ?

@dimiro1
Copy link

dimiro1 commented Dec 27, 2018

From my POV these wrapper functions are nice to have and interesting but not strictly required. At some point, the user still has to do some decoding/encoding, e.g: Decode the Body from APIGatewayProxyRequest.

What do you think @peterbourgon and @ChrisHines?

@suekto-andreas
Copy link
Contributor Author

From my POV these wrapper functions are nice to have and interesting but not strictly required. At some point, the user still has to do some decoding/encoding, e.g: Decode the Body from APIGatewayProxyRequest.

What do you think @peterbourgon and @ChrisHines?

😄 Hi @dimiro1, @peterbourgon, @ChrisHines - let me know if there is anything I can assist with, would love to refactor my stuff if this is ok

@peterbourgon
Copy link
Member

I'm still away for the holidays, I'll take a look in a week or so.

@suekto-andreas
Copy link
Contributor Author

Thanks @peterbourgon have a good holiday !

transport/awslambda/doc.go Outdated Show resolved Hide resolved
transport/awslambda/doc.go Outdated Show resolved Hide resolved
transport/awslambda/encode_decode.go Outdated Show resolved Hide resolved
transport/awslambda/encode_decode.go Outdated Show resolved Hide resolved
// ready to be sent as AWS lambda response.
type EncodeResponseFunc func(context.Context, interface{}) ([]byte, error)

// ErrorEncoder is responsible for encoding an error.
Copy link
Member

Choose a reason for hiding this comment

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

To what? Is the []byte returned just like a response?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are u referring to ErrorEncoder ? if it is - then yes, the []byte is for controlling the response, for example in the context of building an API, we might want to response gracefully with a body of JSON describing detail of the error to Consumer.

transport/awslambda/request_response_funcs.go Outdated Show resolved Hide resolved
transport/awslambda/server.go Outdated Show resolved Hide resolved
transport/awslambda/server.go Outdated Show resolved Hide resolved
// The decoderSymbol function signature has to receive 2 args, which is the
// context.Context and event request to decode.
// It has also to return 2 values, the user-domain object and error.
func DecodeRequestWrapper(decoderSymbol interface{}) DecodeRequestFunc {
Copy link
Member

Choose a reason for hiding this comment

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

I understand what the functions in wrapper.go do, but I don't understand why they're useful. Can you provide some more context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The motivation is to bring the Decode function purpose closer as decoder to user-domain object.

Without the wrapper function, in some use cases, the user actually needs to decode for 2 purposes, first into the AWS corresponding events, then next they decode it further to their business domain.

The idea actually coming from the lambda.Start() in aws-lambda-go. With this convenient function it become so simple to implement a lambda handler.

func show(req events.APIGatewayProxyRequest) (events.APIGatewayProxyResponse, error) {
...
}

func main() {
    lambda.Start(show)
}

With the wrapper it could help the consumer to focus on converting their user-domain from the familiar AWS events package.

// DecodeUppercaseRequest is a decoder of the JSON body into upperRequest.
func DecodeUppercaseRequest(
    _ context.Context,
    apigwReq events.APIGatewayProxyRequest,
) (
    UppercaseRequest,
    error,
) {
    request := UppercaseRequest{}
    err := json.Unmarshal([]byte(apigwReq.Body), &request)
    return request, err
}

and start the server like this

uppercaseHandler := awslambdatransport.NewServer(
    stringsvc.MakeUppercaseEndpoint(svc),
    awslambdatransport.DecodeRequestWrapper(stringsvc.DecodeUppercaseRequest),
    awslambdatransport.EncodeResponseWrapper(stringsvc.EncodeResponse),
    awslambdatransport.ServerErrorEncoder(
        awslambdatransport.ErrorEncoderWrapper(stringsvc.EncodeError),
    ),
)

Copy link
Member

Choose a reason for hiding this comment

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

What is the use case for not using this wrapper?

Copy link
Contributor Author

@suekto-andreas suekto-andreas Jan 25, 2019

Choose a reason for hiding this comment

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

Use case for not using wrapper:

  1. If performance matters more than convenient. As the wrapper is using reflections.
  2. Provide the most possible open for extension. For example if we want to provide custom message format into Lambda.

Copy link
Member

Choose a reason for hiding this comment

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

OK, thanks for the clarification. I'm not OK with a function in the public API that takes an empty interface, which must be one of a pre-ordained number of function types. I understand this is how the AWS lambda.Start function works, but that doesn't excuse the bad practice.

I'd like to see this whole layer removed. The most obvious way I can think of is to provide separate, explicit wrapper functions for each of the supported function signatures. If there were a way to remove the need for this altogether, I'd prefer that, but I can't really judge if there's an acceptable way to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point, yes you are right with API that takes empty interface{} its actually not trivial to understand its usage just from looking at the package itself, only if one see a whole lot of example of usages which means the package cant stand by its own.

OK - removing the wrapper. Thanks @peterbourgon !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated 5a7cbf4

transport/awslambda/wrapper_test.go Outdated Show resolved Hide resolved
suekto-andreas and others added 6 commits January 20, 2019 21:57
Co-Authored-By: suekto-andreas <suekto.andreas@gmail.com>
Co-Authored-By: suekto-andreas <suekto.andreas@gmail.com>
Co-Authored-By: suekto-andreas <suekto.andreas@gmail.com>
)

// Server wraps an endpoint.
type Server struct {

Choose a reason for hiding this comment

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

I think it would be better to call this struct Handler since this is the interface it implements. The rpc server is started by the lambda sdk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are totally right ! Thanks, refactoring it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated here: 6a69716


if resp, err = s.enc(ctx, response); err != nil {
s.logger.Log("err", err)
if s.errorEncoder != nil {

Choose a reason for hiding this comment

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

Why not initialize errorEncoder to a noop and eliminate the need for the nil checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep that makes it prettier, and will name it DefaultErrorEncoder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated 19d5afe

enc EncodeResponseFunc,
options ...HandlerOption,
) *Handler {
s := &Handler{

Choose a reason for hiding this comment

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

Probably now h instead of s. Same goes for the methods below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated e831b16

if resp, err = s.enc(ctx, response); err != nil {
s.logger.Log("err", err)
resp, err = s.errorEncoder(ctx, err)
return

Choose a reason for hiding this comment

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

Taste: Maybe now just return s.errorEncoder(ctx, err)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep that looks nicer - updated 1be9848

transport/awslambda/handler.go Outdated Show resolved Hide resolved
transport/awslambda/handler.go Outdated Show resolved Hide resolved
transport/awslambda/handler.go Outdated Show resolved Hide resolved

// DefaultErrorEncoder defines the default behavior of encoding an error response,
// where it returns nil, and the error itself.
func DefaultErrorEncoder(ctx context.Context, err error) ([]byte, error) {

Choose a reason for hiding this comment

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

I think this code is now uncovered by the tests? Though I obviously don't expect it to have bugs 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add it in 1a31338 ☺️pretty important for future safe-guard from accidental breaking changes.

fahrradflucht and others added 4 commits January 29, 2019 22:55
Co-Authored-By: suekto-andreas <suekto.andreas@gmail.com>
Co-Authored-By: suekto-andreas <suekto.andreas@gmail.com>
Co-Authored-By: suekto-andreas <suekto.andreas@gmail.com>
@peterbourgon peterbourgon merged commit 048aff0 into go-kit:master Feb 22, 2019
@peterbourgon
Copy link
Member

Thanks for the contribution and sorry for the delay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants