fix(aws-lambda) proxy correctly with lua-resty-http #8406
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lua-resty-http only supports http connections to the proxy, regardless of request scheme
This config parameter is not used anywhere.
github-actions
bot
added
chore
hbagdi
reviewed
Feb 14, 2022
| @@ -75,6 +75,7 @@ return { | |||
| type = "boolean", | |||
| default = false, | |||
| } }, | |||
| -- TODO: remove proxy_scheme in Kong 3.0 | |||
There was a problem hiding this comment.
@bungle We should explore a "deprecated = true" property on the schema to manage these programmatically.
aboudreault
reviewed
Feb 14, 2022
aboudreault
reviewed
Feb 15, 2022
| listen 13128; | ||
|
|
||
| content_by_lua_block { | ||
| require("spec.fixtures.forward-proxy-server").connect() |
flrgh
added a commit
that referenced
this pull request
Feb 22, 2022
Fix some bugs and warts in the aws-lambda plugin: * Fix broken proxying by always using `https_proxy` with resty.http * Deprecate `proxy_scheme` config param Some minimal test coverage for proxying was added, and some defunct test cases were removed.
This was referenced Feb 22, 2022
kikito
pushed a commit
that referenced
this pull request
Feb 23, 2022
Fix some bugs and warts in the aws-lambda plugin: * Fix broken proxying by always using `https_proxy` with resty.http * Deprecate `proxy_scheme` config param Some minimal test coverage for proxying was added, and some defunct test cases were removed.
kikito
pushed a commit
that referenced
this pull request
Feb 23, 2022
Fix some bugs and warts in the aws-lambda plugin: * Fix broken proxying by always using `https_proxy` with resty.http * Deprecate `proxy_scheme` config param Some minimal test coverage for proxying was added, and some defunct test cases were removed.
flrgh
added a commit
that referenced
this pull request
Mar 2, 2022
Fix some bugs and warts in the aws-lambda plugin: * Fix broken proxying by always using `https_proxy` with resty.http * Deprecate `proxy_scheme` config param Some minimal test coverage for proxying was added, and some defunct test cases were removed.
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
The aws-lambda plugin currently mishandles user proxy configuration, passing
proxy_opts.http_proxytoresty.httpwhen it ought to be passingproxy_opts.https_proxy(since the request to AWS lambda is always https).This PR remedies that problem along with a couple other shortcomings surrounding the config schema:
proxy_schemeparam is unused and is not needed with the current implementation (proxy_urlexpects a full url with scheme). This PR deprecates it, logging a warning that it will be removed in Kong 3.0.proxy_urlto ensure only http is used.Testing
This plugin used to ship with it's own specialized http connect code (and tests). This is not used anymore, so the associated tests (
spec/03-plugins/27-aws-lambda/50-http-proxy_spec.lua) have been removed, as they do not test anything specific to this plugin or its code.The access handler tests didn't have any coverage for instances using an http proxy, so I sought to add one, which was a little bit more involved of a chore than I expected. The repo's CI pipeline stuff includes a squid container (previously used for the aforementioned http connect tests), but using this in tests is a royal pain:
127.0.0.1for all targets without requiring the container to run in host networking mode.So I ditched squid and wrote a minimal TCP server that implements CONNECT (
spec/fixtures/forward-proxy-server.lua). It's not awesome, but it gets the job done.Result: the test coverage for aws-lambda+http proxy still leaves much to be desired, but this is a step in the right direction.
Full changelog
Issue reference
Fix #8317