Allow request parameter configuration with lambda-proxy integration

[HeroesDieYoung]

What did you implement:

Closes #2882

SDK generation is incomplete with regard to parameters (path, querystring, and headers) for lambda-proxy integration. Allowing only the request.parameters object in configuration allows for the successful generation of an SDK that supports their use.

How did you implement it:

Check for keys other than "parameters" on the request object and remove them (showing a warning to the user) if they exist. Maintained existing behavior for response configurations, they are still removed and a warning is shown.

How can we verify it:

Examples:

service: test

frameworkVersion: ">=1.13.2 <2.0.0"

provider:
  name: aws
  runtime: nodejs6.10
  region: us-east-2
  memorySize: 128

functions:
  helloWorld:
    handler: dist/hello.hello
    events:
      - http:
          path: /{name}
          method: GET
          request:
            parameters:
              querystrings:
                foo: true
              paths:
                bar: true

Todos:

• Write tests
• Write documentation
• Fix linting errors
• Make sure code coverage hasn't dropped
• Provide verification config / commands / resources
• Enable "Allow edits from maintainers" for this PR
• Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

[pmuens]

Hey @HeroesDieYoung 👋
Thank you very much for working on this PR 👍

I just tested it today using the following serverless.yml file:

service:
  name: my-service

provider:
  name: aws
  runtime: nodejs6.10

functions:
  hello:
    handler: handler.hello
    events:
      - http:
          path: /{name}
          method: GET
          request:
            parameters:
              querystrings:
                url: true
              headers:
                foo: false
              paths:
                bar: false
            template:
              application/json: '{ "foo" : "$input.params(''bar'')" }'
            passThrough: NEVER
          response:
            headers:
              Content-Type: integration.response.header.Content-Type
              Cache-Control: "'max-age=120'"
            template: $input.path('$')

One can remove the response and some of the request config to see that no warning will pop up during deployment

However I'm not sure if this feature could potentially introduce confusion.

The main concern I have is that you can configure only some of the provided request config parameter, but not all (as shown in the serverless.yml file above). There is a warning about that printed on the console, but IMHO restricting it so that request config can only be used with the LAMBDA integration type could be better here 🤔 .

My suggestion would be to not support request / response at all and advice the developer to switch to the LAMDBA integration type where the developer can use everything.

What do you think about that @HeroesDieYoung and @eahefnawy ?

I'll wrap my head around that the upcoming days. This is just my gut feeling ATM.

---

Furthermore I get the following error when deploying the service:

  Serverless Error ---------------------------------------
 
     An error occurred while provisioning your stack: ApiGatewayDeployment1498210728175
     - The REST API doesn't contain any methods.
 
  Stack Trace --------------------------------------------
 
ServerlessError: An error occurred while provisioning your stack: ApiGatewayDeployment1498210728175 - The REST API doesn't contain any methods.
    at provider.request.then (/app/lib/plugins/aws/lib/monitorStack.js:109:33)
From previous event:
    at AwsDeploy.monitorStack (/app/lib/plugins/aws/lib/monitorStack.js:26:12)
    at provider.request.then (/app/lib/plugins/aws/lib/updateStack.js:88:30)
From previous event:
    at AwsDeploy.update (/app/lib/plugins/aws/lib/updateStack.js:88:8)
From previous event:
    at AwsDeploy.BbPromise.bind.then (/app/lib/plugins/aws/lib/updateStack.js:105:12)
From previous event:
    at AwsDeploy.updateStack (/app/lib/plugins/aws/lib/updateStack.js:99:8)
From previous event:
    at Object.aws:deploy:deploy:updateStack [as hook] (/app/lib/plugins/aws/deploy/index.js:99:10)
    at BbPromise.reduce (/app/lib/classes/PluginManager.js:234:55)
From previous event:
    at PluginManager.invoke (/app/lib/classes/PluginManager.js:234:22)
    at PluginManager.spawn (/app/lib/classes/PluginManager.js:246:17)
    at AwsDeploy.BbPromise.bind.then (/app/lib/plugins/aws/deploy/index.js:85:48)
From previous event:
    at Object.deploy:deploy [as hook] (/app/lib/plugins/aws/deploy/index.js:81:10)
    at BbPromise.reduce (/app/lib/classes/PluginManager.js:234:55)
    at runCallback (timers.js:800:20)
    at tryOnImmediate (timers.js:762:5)
    at processImmediate [as _immediateCallback] (timers.js:733:5)
From previous event:
    at PluginManager.invoke (/app/lib/classes/PluginManager.js:234:22)
    at PluginManager.run (/app/lib/classes/PluginManager.js:253:17)
    at variables.populateService.then (/app/lib/Serverless.js:100:33)
From previous event:
    at Serverless.run (/app/lib/Serverless.js:87:74)
    at serverless.init.then (/app/bin/serverless:23:50)
    at <anonymous>
 
  Get Support --------------------------------------------
     Docs:          docs.serverless.com
     Bugs:          github.com/serverless/serverless/issues
     Forums:        forum.serverless.com
     Chat:          gitter.im/serverless/serverless
 
  Your Environment Information -----------------------------
     OS:                 linux
     Node Version:       8.1.0
     Serverless Version: 1.14.0

Edit: Could be smth. on AWS end since the deployment succeeds sometimes, but sometimes not. Not 100% sure though.

[HeroesDieYoung]

I get that it might be somewhat confusing to support only a limited subset of the request options. However, that's exactly how AWS is designed. Lambda-proxy integration allows easy development of lambdas as HTTP endpoints, for restful APIs and the like, and supports the request configuration options I'm trying to expose here. Moving to lambda integration is a much more configuration heavy alternative to get the same behavior that lambda-proxy provides out of the box. Lambda integration also changes the event object you receive, making it harder or impossible to get the information you'd want as an HTTP endpoint.
Since lambda-proxy is specifically for HTTP endpoints, I would go so far as to argue that not allowing configuration of request parameters makes serverless unsuitable for use with lambda-proxy.

The work-around being posted on various forums (and currently in use in my project) is to figure out the name your methods are going to get in the cloud formation stack and add the configurations yourself like this:

resources:
  Resources:
    ApiGatewayMethodConfirmGet:
      Properties:
          RequestParameters:
            method.request.querystring.email: true
            method.request.querystring.code: true

I currently have ~80 lines of extra config in my serverless.yml to accommodate, and having to figure out and write out the method name like that makes it pretty awfully un-maintainable.

I guess my argument boils down to "Amazon supports it, and for good reason. If serverless wants to claim lambda-proxy support, it too must support these configuration options."

Re-using a subset of the request configurations seems simplest, as that mirrors how AWS behaves, but can you suggest an alternative that you think might be clearer?

Edit: Sorry for the long response time, and I do very much appreciate you taking the time to consider this PR. Serverless is a great tool, hoping I can help to make it even a little better for this use-case.

[sammarks]

Just ran into this issue in one of the projects I'm currently working on. I agree with the sentiment above and think it would be better to find a solution other than just having developers use the Lambda integration instead.

It seems a little clunky, but if we wanted to avoid the confusion with the request object, why not just have the request object be reserved for the traditional Lambda integration (and keep it named to request so we don't break backwards compatibility), and then introduce a proxy object to contain all of the configuration for the Lambda-Proxy based setup.

Then, inside that object we can configure the parameters and any other Lambda-proxy-specific configuration values.

[HyperBrain]

I'm also on the same page here. The (for lambda-proxies) restricted configuration set should be fully supported. Lambda proxy does simplify and remove the complex request/response mappings, but offers you the other options as the lambda integration does.

An option would be, to allow them within an http endpoint definition, but bail out with an error, if unsupported options are specified - this might be a result of accidentally using lambda-proxy where you want to use lambda (remember, the default, if nothing is specified in Serverless is proxy).

/cc @pmuens

[pmuens]

Thanks for all the feedback @HeroesDieYoung @sammarks and @HyperBrain 👍

I just thought through this again and I agree, that we should always reflect the current status of config support AWS provides.

Therefore we should review, test and merge this PR so that users can use the request config with the LAMBDA-PROXY integration type 👍.

[pmuens]

Thanks again for taking time to work on this @HeroesDieYoung 👍

I just tested it thoroughly today and can verify that it works as expected.

After thinking through this again I agree that we should always aim to reflect the config options AWS provides in Serverless.

I merged master into this PR so that it's up to date and re-formatted the warning message a little bit. Furthermore I updated the docs so that it's visible that this feature works for both integration types.

Here's the serverless.yml I've used to test it:

service:
  name: test-service

provider:
  name: aws
  runtime: nodejs6.10

functions:
  hello:
    handler: handler.hello
    events:
      - http:
          path: /hello/{name}
          method: GET
          request:
            parameters:
              querystrings:
                url: true
              headers:
                foo: false
              paths:
                bar: false
            template:
              application/json: '{ "foo" : "$input.params(''bar'')" }'
            passThrough: NEVER
          response:
            headers:
              Content-Type: integration.response.header.Content-Type
              Cache-Control: "'max-age=120'"
            template: $input.path('$')
  goodbye:
    handler: handler.hello
    events:
      - http:
          integration: LAMBDA
          path: /goodbye/{name}
          method: GET
          request:
            parameters:
              querystrings:
                url: true
              headers:
                foo: false
              paths:
                bar: false
            template:
              application/json: '{ "foo" : "$input.params(''bar'')" }'
            passThrough: NEVER
          response:
            headers:
              Content-Type: integration.response.header.Content-Type
              Cache-Control: "'max-age=120'"
            template: $input.path('$')

Thanks for your warm words on Serverless itself! Great to have such a feedback!

@eahefnawy could you take a second look at it and merge it if it's also working in your end?

[eahefnawy]

Just took this PR for a spin for a final confirmation and didn't have any issues. This is G2M! 🎉

Tons of thanks @HeroesDieYoung for your contribution 🙌 ... hope we see you again 😊