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

refactor(aws-lambda): refactor aws-lambda plugin code with lua-resty-aws #11350

Merged
merged 14 commits into from
Aug 15, 2023

Conversation

windmgc
Copy link
Member

@windmgc windmgc commented Aug 3, 2023

Summary

This PR refactors the aws-lambda plugin by using the lua-resty-aws library.

Refactoring the aws-lambda plugin can provide Kong with multiple aspects of benefits:

  • Simplifying the code base and avoiding maintaining IAM-based authenticating code and AWS service request signing code inside the lambda plugin.
  • Doing IAM-based authenticating automatically on multiple scenarios(based on Environment variables, shared aws config/credential files, EC2 IMDS metadata service, ECS container endpoint, EKS IRSA, etc.). Before this PR, some of these were not supported by the aws-lambda.
  • Obtaining a more similar behaviour to other mainstream AWS SDKs, such as gaining support for additional AWS-related environment variable configurations.

Please note that the AWS authentication-related code and tests are removed because we no longer need them after the change, and all of the previous plugin's functional tests should pass after the change because there is no actual behavior-breaking change in this PR.

Checklist

Full changelog

  • Add lua-resty-aws as a dependency in Kong's rockspec.
  • Add libexpat as a dependency into the Bazel build process.
  • Remove Kong's implementation of fetching EC2 IMDS/ECS IAM role credential code, and remove fetching STS temporary credential code.
  • Remove Kong's implementation of the AWS v4 request signing code.
  • Re-implementing the credential fetching code in the aws-lambda plugin.
  • Re-implementing the Lambda service invoking code in the aws-lambda plugin.
  • Cleaning and re-arranging some other code blocks.

Issue reference

#7638

KAG-1386
FTI-5242
FTI-5206

@@ -355,5 +355,29 @@ function Plugins:get_handlers()
return list
end

function Plugins:execute_plugin_init()
Copy link
Member Author

Choose a reason for hiding this comment

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

@bungle I'm trying to add support for plugins executing their own code in the init phase, so that plugins can do things that may contain yield which cannot be done inside a require. I'm adding an execute_plugin_init function to achieve that. How do you feel about this? Is it a proper way or any advice?

Copy link
Member

Choose a reason for hiding this comment

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

my 2cts: in the "global patches", replace require with a pure-Lua version. Since this has been a recurring theme for a long time. That would solve a bunch of initialization problems due to "yield across C-boundary" issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

#11177 (review)
Leaving a note here: this is the discussion on the same topic. Apparently needs some works to be done to achieve that. If we can achieve that it'll be a painkiller for the restriction of not able to do any yield-able thing in module level

@windmgc
Copy link
Member Author

windmgc commented Aug 9, 2023

Tests need lua-resty-aws library version bumping(after merging Kong/lua-resty-aws#71) to make itself pass. Manually tests passed

@windmgc windmgc marked this pull request as ready for review August 9, 2023 03:30
…cies into rockspec and bazel configs

The commit introduces latest version of lua-resty-aws as Kong's
dependency. Since lua-resty-aws relies on luaexpat to do xml decoding,
Kong also needs to build libexpat during compiling/packaging.
The commits rewrite part of the aws-lambda plugin code so that the IAM
role credential fetching is replaced by using lua-resty-aws credential
provider.
This commits does refactoring on the majority of the aws-lambda plugin
code. The IAM role credential fetching and lambda function invoking has
been replaced by using the lua-resty-aws library directly.
Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@hanshuebner hanshuebner left a comment

Choose a reason for hiding this comment

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

Why is the expat dependency now needed?

The test coverage seems to only be reduced in this PR, even though we're adding features. How do we verify that the new features provided by lua-resty-aws actually work when using the aws-lambda plugin?

Please update the PR description accordingly. If no tests are needed because we have enough coverage, this should also be mentioned.

@windmgc
Copy link
Member Author

windmgc commented Aug 14, 2023

Why is the expat dependency now needed?

We now need libexpat as a runtime dependency of luaexpat, which is the lua library used in the lua-resty-aws now to do XML parsing(to replace deprecated pl.xml).

The test coverage seems only to be reduced in this PR, even though we're adding features. How do we verify that the new features provided by lua-resty-aws actually work when using the aws-lambda plugin?

Only those AWS-authenticating related codes and tests are removed because we no longer need them after the PR's change. Other remaining tests should pass without any problem, and I think we already have enough coverage on those. I'm still waiting to get a lua-resty-aws library new version bumping to make all tests green in this PR.

Speaking of the authenticating feature, it's a good point, I'd also like new tests that cover the authenticating feature included in this PR as well, but simulating the way that AWS does authenticating is quite complex in a CI environment. (I'm doing some experiments at https://github.com/Kong/kong-ee/pull/5916, and hopefully if it works well I can add similar things to OSS repo as well)

I've updated the PR description to explain the tests related issue.

@hanshuebner hanshuebner merged commit ea85db8 into master Aug 15, 2023
22 checks passed
@hanshuebner hanshuebner deleted the refactor-lambda branch August 15, 2023 08:52
windmgc added a commit that referenced this pull request Nov 2, 2023
…aws (#11350)

* refactor(aws-lambda): add lua-resty-aws library and libexpat dependencies into rockspec and bazel configs

The commit introduces latest version of lua-resty-aws as Kong's
dependency. Since lua-resty-aws relies on luaexpat to do xml decoding,
Kong also needs to build libexpat during compiling/packaging.

* refactor(aws-lambda): use lua-resty-aws and rewrite fetch credential

The commits rewrite part of the aws-lambda plugin code so that the IAM
role credential fetching is replaced by using lua-resty-aws credential
provider.

* refactor(aws-lambda): refactor aws-lambda plugin

This commits does refactoring on the majority of the aws-lambda plugin
code. The IAM role credential fetching and lambda function invoking has
been replaced by using the lua-resty-aws library directly.

* style(*): remove useless lua file

* fix(cd): fix explain manifest for libexpat

* fix(cd): fix buildifier style

* fix(*): try to fix lambda plugin init_worker

* fix(*): fix http proxy & sts regional endpoint config

* fix(*): execute plugin init code correctly

* fix(*): remove lambda returned content length

* chore(*): move libexpat from cross_deps to standalone repo

* fix(*): do not override global config credential

* chore(*): remove non-debug flag

* chore(*): bump lua-resty-aws version to 1.3.0
windmgc added a commit that referenced this pull request Nov 3, 2023
…aws (#11350)

* refactor(aws-lambda): add lua-resty-aws library and libexpat dependencies into rockspec and bazel configs

The commit introduces latest version of lua-resty-aws as Kong's
dependency. Since lua-resty-aws relies on luaexpat to do xml decoding,
Kong also needs to build libexpat during compiling/packaging.

* refactor(aws-lambda): use lua-resty-aws and rewrite fetch credential

The commits rewrite part of the aws-lambda plugin code so that the IAM
role credential fetching is replaced by using lua-resty-aws credential
provider.

* refactor(aws-lambda): refactor aws-lambda plugin

This commits does refactoring on the majority of the aws-lambda plugin
code. The IAM role credential fetching and lambda function invoking has
been replaced by using the lua-resty-aws library directly.

* style(*): remove useless lua file

* fix(cd): fix explain manifest for libexpat

* fix(cd): fix buildifier style

* fix(*): try to fix lambda plugin init_worker

* fix(*): fix http proxy & sts regional endpoint config

* fix(*): execute plugin init code correctly

* fix(*): remove lambda returned content length

* chore(*): move libexpat from cross_deps to standalone repo

* fix(*): do not override global config credential

* chore(*): remove non-debug flag

* chore(*): bump lua-resty-aws version to 1.3.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants