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

feat: add streaming support #119

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

tysoekong
Copy link
Collaborator

@tysoekong tysoekong commented Jun 6, 2024

Adds:

  • Re-usable parser for Amazon VND stream events binary format ("chunk streaming")
  • Execution branch in execute.lua to return the body stream reader to the caller, for use in the AWS Lambda plugin (streaming mode)
  • Update SDK to v2.1353.0 to add missing Invoke Lambda with Streaming Response shapes

Copy link

github-actions bot commented Jun 6, 2024

Luacheck Report

27 tests    2 ✅  0s ⏱️
 1 suites   0 💤
 1 files    25 ❌

For more details on these failures, see this check.

Results for commit bec7666.

♻️ This comment has been updated with latest results.

@tysoekong tysoekong force-pushed the feat/add_streaming_support branch from cbd40a4 to a735ed1 Compare June 6, 2024 09:32
@tysoekong tysoekong requested review from windmgc and Tieske June 6, 2024 09:33
@tysoekong tysoekong force-pushed the feat/add_streaming_support branch from 40ec8b2 to 4c5300d Compare June 6, 2024 09:45
@windmgc
Copy link
Member

windmgc commented Jun 6, 2024

Just FYI, we might need to adjust the validation logic after upgrading SDK version.(I've tried before in #72, but didn't get back to this since then)

And a related ticket: https://konghq.atlassian.net/browse/KAG-3834

@tysoekong
Copy link
Collaborator Author

tysoekong commented Jun 6, 2024

@windmgc Yeah I wondered why the tests randomly failing, I can look at that shortly if you want.

Do you know why string.buffer package is missing? I can't find its dependency in Luarocks, but I think it's something to do with ffi dependency?

I am totally prepared to re-test by hand the AWS vault, auth, and other stuff too btw in Kong(/EE) if you want me to...

@tysoekong
Copy link
Collaborator Author

@Tieske Yeah I missed up the release process completely, now the branch is stuck.

Ignore it, check this one instead.

@windmgc
Copy link
Member

windmgc commented Jun 6, 2024

Do you know why string.buffer package is missing? I can't find its dependency in Luarocks, but I think it's something to do with ffi dependency?

String buffer does not exist in older LuaJIT versions, in the lua-resty-aws library's CI we test against OpenResty 1.17.8.2 which was released 4 years ago and may not have built-in string buffer in LuaJIT

@@ -6,7 +6,7 @@
# It will convert the service descriptions of the specified SDK version
# (see SDK_VERSION_TAG) into Lua modules and generate a rockspec.

SDK_VERSION_TAG=v2.751.0
SDK_VERSION_TAG=v2.1353.0
Copy link
Member

Choose a reason for hiding this comment

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

This is a tricky change. We have way too little test coverage over different AWS services to find potential issues.

wdyt @windmgc ?

Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid so... I do not have much confidence in upgrading it directly

tostring(signed_request.host),
tostring(signed_request.port),
tostring(err))
if response.headers["application/vnd.amazon.eventstream"] then

Choose a reason for hiding this comment

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

Should this be if response.headers["Content-Type"] == "application/vnd.amazon.eventstream" then?

Choose a reason for hiding this comment

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

I fixed this locally in my test environment and applied other changes from f0ac80e , works like a charm in my lambda streaming case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@DiscreteTom I have made changes to the header format in the stream handler, otherwise we are wasting CPU by scanning for "header_key == x"

Can you check your plugin code again?

Minor change, now headers are:

{
  body = "...",
  headers = {
    key = "value",
    key2 = "value2",
  },
}

Choose a reason for hiding this comment

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

No problem. I updated my code locally against this change and it is ok. Gonna push to my PR when this lib is released.

@DiscreteTom
Copy link

Any update on this?

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