feat(plugins) AWS Lambda plugin #1777

subnetmarco · 2016-10-27T23:17:03Z

AWS Lambda support.

Closes #1005, and #1190 from which it is heavily inspired (by @timerickson).

sonicaghi · 2016-10-28T09:24:34Z

On the plugin gallery we now have to create a new category called: Serverless

Tieske · 2016-11-14T13:41:47Z

kong/plugins/aws-lambda/access.lua

+      ["X-Amz-Invocation-Type"] = conf.invocation_type,
+      ["X-Amx-Log-Type"] = conf.log_type,
+      ["Content-Type"] = "application/x-amz-json-1.1",
+      ["Content-Length"] = tostring(string.len(bodyJson))


don't use the len method, just use tostring(#bodyJson)

Tieske · 2016-11-14T13:42:23Z

kong/plugins/aws-lambda/access.lua

+  local bodyJson = cjson.encode(retrieve_parameters())
+
+  local host = string.format("lambda.%s.amazonaws.com", conf.aws_region)
+  local path = string.format("/2015-03-31/functions/%s/invocations", 


I don't know the aws lambda stuff, but are these literals here intentional?

Tieske · 2016-11-14T13:42:43Z

kong-0.9.5-0.rockspec

+    ["kong.plugins.aws-lambda.handler"] = "kong/plugins/aws-lambda/handler.lua",
+    ["kong.plugins.aws-lambda.schema"] = "kong/plugins/aws-lambda/schema.lua",
+    ["kong.plugins.aws-lambda.access"] = "kong/plugins/aws-lambda/access.lua",
+    ["kong.plugins.aws-lambda.v4"] = "kong/plugins/aws-lambda/v4.lua"


please use trailing comma for table costructors

Tieske · 2016-11-14T13:46:57Z

kong/plugins/aws-lambda/handler.lua

+
+AWSLambdaHandler.PRIORITY = 750
+
+return AWSLambdaHandler


Merge the access.lua file with this one, no use in multiple files here

Tieske · 2016-11-14T13:48:03Z

kong/plugins/aws-lambda/schema.lua

+    aws_secret = {type = "string", required = true},
+    aws_region = {type = "string", required = true, enum = {
+                  "us-east-1", "us-east-2", "ap-northeast-1", "ap-northeast-2", 
+                  "ap-southeast-1", "ap-southeast-2", "eu-central-1", "eu-west-1"}},


do we want those to be literals? requires maintenance when amazon decides to change things

for now I would keep it like this - we can always change it in the future

Tieske · 2016-11-14T14:12:25Z

kong/plugins/aws-lambda/v4.lua

+    for name, value in pairs(headers) do
+      if value then -- ignore headers with 'false', they are used to override defaults
+        i = i + 1
+        local name_lower = name:lower()


using lower here, while just above we're using upper on a first glance not efficient

I don't see the upper.

It's line 140 now, where it is commented as -- convert to standard header title case

Tieske · 2016-11-14T14:15:01Z

kong/plugins/aws-lambda/v4.lua

+  }
+end
+
+return prepare_awsv4_request


general; lots of locals to cache; from string and table, and then some

Tieske · 2016-11-14T14:15:19Z

kong/plugins/aws-lambda/v4.lua

+    method = request_method;
+    target = target;
+    headers = headers;
+    body = req_payload;


don't use ;, use ,

Tieske · 2016-11-14T14:17:25Z

kong/tools/responses.lua

@@ -95,7 +95,7 @@ local function send_response(status_code)
  -- @see https://github.com/openresty/lua-nginx-module
  -- @param content (Optional) The content to send as a response.
  -- @return ngx.exit (Exit current context)
-  return function(content, headers)
+  return function(content, headers, raw_body)


update comments for the extra parameter

What does this have to do with this the aws-lambda plugin?

Tieske · 2016-11-14T14:17:38Z

kong/tools/responses.lua

@@ -144,14 +146,14 @@ local closure_cache = {}
 -- @param body A string or table which will be the body of the sent response. If table, the response will be encoded as a JSON object. If string, the response will be a JSON object and the string will be contained in the `message` property.
 -- @param[type=table] headers Response headers to send.
 -- @return ngx.exit (Exit current context)
-function _M.send(status_code, body, headers)
+function _M.send(status_code, body, headers, raw_body)


update comments for extra parameter

Tieske · 2016-11-16T09:21:34Z

kong/plugins/aws-lambda/v4.lua

+end
+
+local function hex_encode(str) -- From prosody's util.hex
+  local char_to_hex = {};


This char_to_hex table is a constant, filled right below. This one must move outside the scope (top of module?) or it will regenerate the table on every invocation

Tieske · 2016-11-16T21:33:59Z

also; missing integration tests.

subnetmarco · 2016-11-16T23:20:12Z

@Tieske tests in the 01-access_spec.lua file.

timerickson · 2016-11-20T04:38:32Z

kong/plugins/ldap-auth/schema.lua

@@ -1,14 +1,15 @@
 return {
-fields = {


What does this have to do with the aws-lambda plugin?

timerickson · 2016-11-20T04:50:44Z

Why does this drop the support of IAM Instance Role authentication that was in #1190 ?

timerickson · 2016-11-20T04:51:35Z

Why does this drop the the support for implicit configuration (via parsed aws-lambda:// url definition) that was in #1190 ?

timerickson · 2016-11-20T04:52:58Z

What is the explanation/expectation of behavior when specifying anything other than RequestResponse invocation_type? It is undefined, and that is why it was left off #1190 .

timerickson · 2016-11-20T04:53:50Z

Why does this drop support for supplying IAM credentials in the Authorization header as had been in #1190 ?

timerickson · 2016-11-20T04:56:37Z

This does not appear to have proper error handling as was in #1190 . On error, AWS Lambda returns a 200 with a response header indicating the presence of the error. Lambda execution is essentially RPC not restful.

subnetmarco · 2016-11-21T06:50:38Z

Why does this drop the support of IAM Instance Role authentication that was in #1190 ?

The older implementation is blocking and needs to be rewritten with a non-blocking cosocket API. It is missing in this PR and it just has to be done.

Why does this drop the the support for implicit configuration (via parsed aws-lambda:// url definition) that was in #1190 ?

Looking at the big picture it seems like Kong will have two kind of routes: proxy routes, and functional routes that don't proxy to anything but expect a plugin to take over and return a response (like this plugin). We can start discussing about this so we can properly support all those future plugins that return a response without proxying it to an upstream. Also, the previous implementation was changing the core, breaking the core/plugin separation. A better internal API is needed instead.

What is the explanation/expectation of behavior when specifying anything other than RequestResponse invocation_type? It is undefined, and that is why it was left off #1190 .

I have added some more tests that show the usage of the other invocation types. Basically the behavior of the plugin is that it invokes the Lambda function with the specified type and returns whatever response Lambda returns.

Why does this drop support for supplying IAM credentials in the Authorization header as had been in #1190 ?

Because the user might want to add other plugins on top of the Lambda plugin that might conflict with the Authorization header: ie, if I want to protect the Lambda function with Basic Authentication. Another header could be used, but what's the use-case of being able to dynamically set the AWS credentials? Can't that be done by applying the plugin multiple times per different user, each one of them with different credentials?

This does not appear to have proper error handling as was in #1190 . On error, AWS Lambda returns a 200 with a response header indicating the presence of the error. Lambda execution is essentially RPC not restful.

The plugin returns the response the same way Lambda returns it. I didn't change the response being returned by Lambda, because I don't think the plugin should do it. The user can refer to the Lambda documentation to understand how to handle the response. Thoughts?

subnetmarco · 2016-12-07T22:40:14Z

On a second though this plugin should also be using content_by_lua so we can properly handle response and Log plugins on top of it.

thibaultcha · 2016-12-07T23:01:17Z

On a second though this plugin should also be using content_by_lua so we can properly handle response and Log plugins on top of it.

As we know though, only one content handler must be provided per location in Nginx, and thus proxy_pass and content_by_lua conflicts if used together.

subnetmarco · 2016-12-10T08:37:13Z

proxy_pass and content_by_lua conflicts if used together

I am just brainstorming, but maybe we could leverage ngx.exec to accomplish our task with an internal redirect, ie (pseudo-code):

worker_processes auto;
error_log logs/error.log info;
daemon off;

events {
  worker_connections 1024;
}

http {
  server {
    listen 9000;

    location @proxy {
      internal;
      proxy_pass http://httpbin.org;
    }

    location / {
      content_by_lua '
        local content_handler = plugin.handler.content
        if content_handler then
          content_handler()
        else
          return ngx.exec("@proxy")
        end
      ';
    }
  }

}

Although it seems like ngx.exec doesn't not allow to share nginx variables or ctx variables, and this may be a problem.

shavo007 · 2017-03-10T01:56:53Z

@sonicaghi "On the plugin gallery we now have to create a new category called: Serverless"

Where is this defined in the doc?

I do not see it here:
https://getkong.org/plugins/

subnetmarco · 2017-03-10T18:21:38Z

@shavo007 coming soon.

subnetmarco added the pr/status/needs review label Oct 27, 2016

subnetmarco added this to the 0.10 RC milestone Oct 27, 2016

subnetmarco mentioned this pull request Nov 3, 2016

feat(plugin) AWS Lambda Integration #1190

Closed

Tieske reviewed Nov 14, 2016

View reviewed changes

Tieske reviewed Nov 16, 2016

View reviewed changes

subnetmarco force-pushed the feat/plugin-lambda branch 2 times, most recently from b352494 to e32d94d Compare November 19, 2016 01:08

feat(plugins) AWS Lambda plugin

200c54c

subnetmarco force-pushed the feat/plugin-lambda branch from e32d94d to 200c54c Compare November 19, 2016 01:09

timerickson reviewed Nov 20, 2016

View reviewed changes

kong/plugins/ldap-auth/schema.lua

@@ -1,14 +1,15 @@

return {

fields = {

Copy link

timerickson Nov 20, 2016

Choose a reason for hiding this comment

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

What does this have to do with the aws-lambda plugin?

subnetmarco added 3 commits November 20, 2016 17:31

merge

9351760

newline

0c7c193

tests other invocation types

ce647bb

subnetmarco added 5 commits December 19, 2016 17:57

fix

2a52469

Merge branch 'next' into feat/plugin-lambda

5cec020

wip

86fffbe

revert

44b3eb0

wip

c49519c

Merge branch 'next' into feat/plugin-lambda

b1bf789

subnetmarco merged commit ef52403 into next Dec 28, 2016

subnetmarco deleted the feat/plugin-lambda branch December 28, 2016 23:55

brycehemme mentioned this pull request Dec 5, 2019

Lambda plugin support IAM instance roles #5297

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(plugins) AWS Lambda plugin #1777

feat(plugins) AWS Lambda plugin #1777

subnetmarco commented Oct 27, 2016 •

edited

Loading

sonicaghi commented Oct 28, 2016

Tieske Nov 14, 2016

Tieske Nov 14, 2016

subnetmarco Nov 15, 2016

Tieske Nov 14, 2016

Tieske Nov 14, 2016

Tieske Nov 14, 2016

subnetmarco Nov 16, 2016

Tieske Nov 14, 2016

subnetmarco Nov 16, 2016

Tieske Nov 16, 2016

C45tr0 Dec 16, 2016

Tieske Nov 14, 2016

Tieske Nov 14, 2016

Tieske Nov 14, 2016

subnetmarco Nov 19, 2016

timerickson Nov 20, 2016

Tieske Nov 14, 2016

Tieske Nov 16, 2016

subnetmarco Nov 16, 2016

Tieske commented Nov 16, 2016

subnetmarco commented Nov 16, 2016

timerickson Nov 20, 2016

timerickson commented Nov 20, 2016 •

edited

Loading

timerickson commented Nov 20, 2016

timerickson commented Nov 20, 2016

timerickson commented Nov 20, 2016

timerickson commented Nov 20, 2016

subnetmarco commented Nov 21, 2016 •

edited

Loading

subnetmarco commented Dec 7, 2016

thibaultcha commented Dec 7, 2016

subnetmarco commented Dec 10, 2016 •

edited

Loading

shavo007 commented Mar 10, 2017

subnetmarco commented Mar 10, 2017

feat(plugins) AWS Lambda plugin #1777

feat(plugins) AWS Lambda plugin #1777

Conversation

subnetmarco commented Oct 27, 2016 • edited Loading

sonicaghi commented Oct 28, 2016

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

Tieske commented Nov 16, 2016

subnetmarco commented Nov 16, 2016

Choose a reason for hiding this comment

timerickson commented Nov 20, 2016 • edited Loading

timerickson commented Nov 20, 2016

timerickson commented Nov 20, 2016

timerickson commented Nov 20, 2016

timerickson commented Nov 20, 2016

subnetmarco commented Nov 21, 2016 • edited Loading

subnetmarco commented Dec 7, 2016

thibaultcha commented Dec 7, 2016

subnetmarco commented Dec 10, 2016 • edited Loading

shavo007 commented Mar 10, 2017

subnetmarco commented Mar 10, 2017

subnetmarco commented Oct 27, 2016 •

edited

Loading

timerickson commented Nov 20, 2016 •

edited

Loading

subnetmarco commented Nov 21, 2016 •

edited

Loading

subnetmarco commented Dec 10, 2016 •

edited

Loading