-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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(plugin) AWS Lambda Integration #1190
feat(plugin) AWS Lambda Integration #1190
Conversation
d6f3641
to
c0ee568
Compare
Please consider this for merging. |
@timerickson I will create a sample Lambda function in our Mashape staging account that will be always up and running and that we can use for integration tests. Can you send me some lambda code that I will upload on our staging environment? This lambda function will run forever, and can be updated by contacting me. |
@thefosk I have just been using hello-world-python:
|
This is the ARN |
@thefosk I will need an IAM credential to do this. There is no credential-less invocation of AWS lambda. Ideally we would do this via IAM Roles for EC2. Depending on your CI infrastructure, that might be possible here, but not for individual developers. For individual devs testing outside your AWS infrastructure, we would be left with a regular IAM credential, thus storing a credential probably in the repository. This is part of why I hesitated with the integration test for this in favor of unit tests. |
@timerickson I have created a user and a IAM policy specifically for this use-case and locked to this one function, here are the credentials that you (or anybody else) can use to test the Lambda function: ARN: Let me know if it works. |
@thefosk It works. Adjusting the tests now. |
I would appreciate some more guidance here... It's working. What do you see remaining necessary before this gets merged? And should I keep working on the list in the README.md for now? Certainly one of the highest priorities I have for now is being able to pass some sort of auth credential through the api instead of requiring auth to be configured on the plugin. But I don't know that this is required for MVP, or should be done on this same PR. |
@@ -35,7 +35,8 @@ dependencies = { | |||
"lua-llthreads2 ~> 0.1.3-1", | |||
"luacrypto >= 0.3.2-1", | |||
"luasyslog >= 1.0.0-2", | |||
"lua_pack ~> 1.0.4-0" | |||
"lua_pack ~> 1.0.4-0", | |||
"luaossl ~> 20151221-0" |
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.
@timerickson Can you use the functions available at https://github.com/openresty/lua-resty-string instead of introducing a new dependency?
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 will look into it. I agree it would be better/easier not to introduce any more dependencies.
I wrote some comments about the implementation, but I think I will checkout your branch and submit a contribution for some of those points. |
b8a76d4
to
10a5c42
Compare
Per comments on openresty/lua-resty-string#18, it doesn't appear that lua-resty-string contains what is needed. I will try adding in separate plugin modules like I did with the kong.plugins.aws-lambda.aws.v4 stuff just what is missing from lua-resty-string, if you still think it is worthwhile to avoid one extra dependency. |
@timerickson can you pls PR on getkong.org repo with docs, logo, and description of the plugin. thanks. |
In process. |
Kong/docs.konghq.com#235 submitted, @sinzone |
@thefosk The luaossl dependency is removed, but I had to add a (modified version of a) file from openresty/lua-resty-string#18 in order to do it. It's not the prettiest, but maybe it's not so ugly? ;-) |
@thefosk Could you create two new versions of kongLambdaTest as follows? from __future__ import print_function
import json
print('Loading function')
def lambda_handler(event, context):
#print("Received event: " + json.dumps(event, indent=2))
print("value1 = " + event['key1'])
print("value2 = " + event['key2'])
print("value3 = " + event['key3'])
return event['key2'] # Echo back the second key value
#raise Exception('Something went wrong') and v3 from __future__ import print_function
import json
print('Loading function')
def lambda_handler(event, context):
#print("Received event: " + json.dumps(event, indent=2))
print("value1 = " + event['key1'])
print("value2 = " + event['key2'])
print("value3 = " + event['key3'])
return event['key1'] # Echo back the first key value
#raise Exception('Something went wrong') Yes, v3 is identical to v1, and the only difference with v2 is returning key2 instead of v1. This is to allow writing tests for calling specific versions of the lambda function. |
@timerickson just an update on my side. I am currently busy in the |
@thefosk That's great. Will do. FYI, my next trick will be IAM Role auth support, but I don't know how long that will take. But I think this is more than MVP as is already anyway. |
Implement Kong#1005 - AWS Lamba Integration. For design consideration/review only - not for merge consideration as yet. Test cases to follow. Create MVP Lambda plugin. See README.md for details. Add luaossl to rockspec (now with less entanglement)
Create handler spec Clean up handler code
Add upstream_url scheme validation
Refactor url/config code
…d function_name optional in config
… with aws-lambda://region/func upstream_url) Rearrange remaining schema and comments reflective of priority Update tests and remove unnecessary ones Lint
f7a49ce
to
fba7ace
Compare
@thefosk IAM Instance Role auth support added but that would only be testable from an EC2 in that role or I will have to research how otherwise (I tested it in a personal instance). The code is there in the plugin, and the test is there but commented out. |
Refactor spec to remove unused init Remove dead schema Update README.md
@timerickson @thefosk What is the status on this? |
@stramel The status of the code is that it was completely functional as documented. The status of the PR with the project is somewhat unknown. You can see it is marked with priority/HUGE (!) and tagged for Milestone 0.9, but I have not been given a date as yet or asked to update/rebase the PR with new changes. I certainly will be glad to once I am asked, but have been letting it progress at its own pace. |
@timerickson sorry we weren't able to add this plugin in 0.9 - but I can take a look at it now that 0.9 has been released. |
Be my guest - I have left this for whenever it may reawaken. If/when you want me to revisit it at all it will probably take me a bit to refresh my context, but I'll get to it eventually. |
@@ -0,0 +1,176 @@ | |||
local cjson = require 'cjson' | |||
local http_client = require 'kong.tools.http_client' |
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.
Ran into this while quickly glancing over this code. This http_client should definitely NOT be used, it uses LuaSocket under the hood, and hence uses blocking socket connections.
That client is to be used for testing only (and was actually already removed in 0.9)
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.
Okay - I will swap it out when I get a chance to look at this again - which I don't know when that will be...
Will this new feature be available for 0.10? |
I took inspiration from this PR and submitted #1777. Can you try to use that branch instead? |
Implement #1005 - AWS Lamba Integration.
See README.md for details.
NOTE: I included the aws.v4 module in the plugin code instead of adding to the rockspec as it is only on the dev side of luarocks and didn't know how best to handle that (I am new to lua and luarocks).