Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Shared proxy-state mutation, nested-context inheritance, and missing public documentation must be addressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds kong_pass to dynamically mediate HTTP, HTTP/2, and gRPC upstream dispatch.
Changes:
- Implements runtime protocol and version selection.
- Registers the directive and location state.
- Adds protocol tests and disables connection reuse where needed.
File summaries
| File | Description |
|---|---|
src/ngx_http_lua_kong_pass.c |
Implements directive parsing and dispatch. |
src/ngx_http_lua_kong_module.c |
Registers kong_pass. |
src/ngx_http_lua_kong_directive.h |
Declares the handler. |
src/ngx_http_lua_kong_common.h |
Adds mediator configuration fields. |
config |
Includes the new source file. |
t/015-kong-pass.t |
Tests dispatch and validation. |
t/002-upstream-tls.t |
Prevents TLS connection reuse during testing. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+306
to
+307
| if (ver.len == 1 && ver.data[0] == (u_char) '2') { | ||
| return ngx_http_proxy_v2_handler(r); |
| NULL }, | ||
|
|
||
| { ngx_string("kong_pass"), | ||
| NGX_HTTP_LOC_CONF|NGX_HTTP_LIF_CONF|NGX_HTTP_LMT_CONF|NGX_CONF_2MORE, |
| offsetof(ngx_http_lua_kong_loc_conf_t, request_id_var_index), | ||
| NULL }, | ||
|
|
||
| { ngx_string("kong_pass"), |
bungle
force-pushed
the
feat/kong-pass
branch
2 times, most recently
from
September 14, 2026 12:53
017efa7 to
2ef4b38
Compare
bungle
force-pushed
the
feat/kong-pass
branch
from
September 23, 2026 14:37
2ef4b38 to
2a95cbe
Compare
Kong needs to proxy to an upstream over HTTP/2 per request. The scheme cannot carry the version, because `ngx_http_proxy_eval` accepts only `http://` and `https://`, and `$upstream_scheme` already means the transport scheme everywhere else. Add a `version=$variable` parameter. A value of `2` calls `ngx_http_proxy_v2_handler` directly. An empty value and `1.1` use the `proxy_http_version` directive. Any other value logs a warning and uses the directive, because kong_pass cannot change the HTTP/1.x minor version. Make the path argument required, so that every later argument is a `name=value` parameter. Add tests, which kong_pass did not have.
nginx pools upstream connections by default since 1.31. TEST 3 reads the verification line that the upstream logs during the TLS handshake. The second run of the test reuses the pooled connection, performs no handshake, and finds no line. Turn the pool off for that upstream. The other tests in this file either proxy to a literal URL, which nginx does not pool, or expect the handshake to fail, so no connection is cached.
nginx added ngx_http_proxy_module.h together with the HTTP/2 upstream
handler in version 1.29.4. The kong_pass source included that header
without a guard, so the build failed on nginx 1.27.1, which the CI uses:
ngx_http_lua_kong_pass.c:19:10: fatal error:
ngx_http_proxy_module.h: No such file or directory
Include the header only when nginx supplies it. The directive keeps its
version= parameter on an older nginx, and warns that the parameter has no
effect. Skip the two tests that need an HTTP/2 upstream.
nginx 1.29.7 turned on the upstream keepalive cache by default, and made
"keepalive 0" the way to switch it off. Earlier versions reject the value:
nginx: [emerg] invalid value "0" in "keepalive" directive
Use "keepalive_requests 1" instead. It gives every request its own
connection on 1.29.7 and later, and earlier versions cache no upstream
connection at all, so the value has no effect there.
ngx_http_proxy_v2_handler writes preserve_output into the shared proxy location config, not the request. Calling it dynamically for only some requests would leak that flag onto every other request on the location for the life of the worker. Only enable the dynamic call when NGX_HTTP_UPSTREAM_PRESERVE_OUTPUT_PATCH shows the fix (request-local preserve_output) is present; otherwise fall back to proxy_http_version, same as on nginx older than 1.29.4. Add mixed version/body coverage.
if and limit_except each get their own, freshly created location config, merged from the parent like any nested location. This module's merge never touched pass_selector, pass_version, proxy_handler or grpc_handler, so a kong_pass set only on the parent left those NULL in the child: - if blocks: nginx already latches the content handler from the outer location, so the mediator still runs, but reads NULL state through the swapped location config and returns 500. - limit_except blocks: proxy_pass/grpc_pass already inherit their own state into these children and reinstall their native handler there; since this module's merge runs after theirs, that raw handler would win and silently skip per-request version dispatch. Merge the mediator's state into "noname" children that don't set their own, and reinstall the mediator as the limit_except child's handler, mirroring ngx_http_proxy_merge_loc_conf/ngx_http_grpc_merge_loc_conf. Add nested-context regression coverage for both.
kong_pass had no entry in the README's Directives reference, unlike every other directive this module ships. Document its syntax, contexts, selector dispatch (including the grpc/grpcs prefix match), host/path templating and resolver implications, version= semantics and fallback, and the nginx version and patch requirements for version=2.
"limit_except" runs the request against a separate location config, and ngx_http_rewrite_module does not inherit the enclosing location's codes into it, nor does "set" accept NGX_HTTP_LMT_CONF, so no "set" can reach those requests. The test drove kong_pass's selector and version from "set" in the enclosing location, so both evaluated empty and proxy_pass rejected "://test_upstream" instead of dispatching. Take them from $arg_ variables, which any location config can read. The test never ran in CI, where nginx is older than the 1.29.4 its skip_nginx asks for, so this went unnoticed.
ngx_http_lua_kong_pass.c invokes proxy_pass and grpc_pass through their
own set handlers, so it references ngx_http_proxy_module and
ngx_http_grpc_module. auto/modules builds the first only for
HTTP_PROXY=YES and the second only for HTTP_GRPC=YES together with
HTTP_V2=YES, and configure leaves HTTP_V2 off unless
--with-http_v2_module is given, in nginx and in OpenResty alike. A
default build therefore failed to link the whole module:
ngx_http_lua_kong_pass.c: undefined reference to `ngx_http_grpc_module'
even for a configuration that never mentions kong_pass.
Compile that source only when all three modules are built, mark such a
build with NGX_HTTP_LUA_KONG_PASS, and hide the directive and the merge
call behind it. A build without them keeps every other directive this
module offers and rejects a kong_pass line as an unknown directive, the
same way nginx handles any directive whose module was left out.
The tests that assert HTTP/2.0 through version=2 gated on nginx 1.29.4, which is only one of the three things that have to hold: the build also needs ngx_http_v2_module and the request-local preserve_output patch. On a 1.29.4 or later build without the patch they failed, six blocks at a time, for a configuration this module supports and documents as falling back to proxy_http_version. Since kong_pass is now compiled only where proxy_pass and grpc_pass are, a build without those failed every block in the file instead. Ask the module which of the two it is, once, by running "nginx -t" over a config that uses kong_pass with version=: it already reports an unavailable directive and an ineffective version= at configuration time. Skip the version=2 blocks for the second answer and the whole file for the first, and treat anything else, including a probe that could not run, as a fully capable build, so a broken probe fails loudly instead of skipping in silence.
host and path are concatenated into one URL with no separator, so a literal path that does not start with "/" becomes part of the host. The request then fails per request on a host nobody configured, and the error misdirects: "kong_pass $selector upstream :8080" logs "no resolver defined to resolve upstream", naming the right host while saying nothing about the appended ":8080" that stopped it matching the upstream block. Require a literal path to start with "/", leaving empty, which hands the client's URI to the upstream, and a $variable, which only the request can resolve, as the other two forms. Cover all three. Document two behaviours that go with it. Dropping path for grpc_pass means path decides the upstream URI only for the requests the selector sends to proxy_pass, since nginx's gRPC module takes :path from the client's URI, so a rewritten path does not reach a gRPC request. And a kong_pass in a location covers that location's if and limit_except blocks: for limit_except it also becomes the block's content handler, so the selector, not a proxy_pass written in the block, decides which of the two handlers runs there, while the block's own directive still configures the one that does. Note as well that no "set" reaches the requests a limit_except restricts, so the selector has to come from a variable any location config can read.
The two per-request evaluations went through complex values, which run a
script and allocate from r->pool for what is always a single variable.
Index the variables at configuration time and read them with
ngx_http_get_indexed_variable() instead, sharing the cache entry that
proxy_pass's own script already fills for the same variable, and
following the same shape as request_id_var_index in this module.
This also makes "must be a $variable" mean it. A complex value accepted
anything starting with "$", so version=$a$b compiled as a concatenation;
an index is taken by name, so nginx now rejects that at startup with
unknown "a$b" variable. Note that ${name} is not accepted either, as in
this module's other $variable directives.
Match grpc and grpcs exactly while here, rather than any scheme starting
with "grpc", so that a value this module does not know reaches
proxy_pass and is reported as the invalid URL prefix it is. Add tests for
both, and record why the stack array that
ngx_http_lua_kong_pass_invoke() hands to a set handler is safe for
proxy_pass and grpc_pass.
… use A build without the request-local preserve_output patch cannot proxy to an upstream over HTTP/2, and says so once at configuration time. The request path then treated version=2 as an unsupported value and warned about it for every request, which is a synchronous write per request for a configuration that is correct: 2 is what a caller sets for a service whose upstream speaks HTTP/2. Recognise 2 on that build as a value kong_pass knows and this nginx cannot honour, and fall back to proxy_http_version without a word. A value kong_pass does not know keeps warning per request, as nginx itself reports an invalid proxy_pass URL or a missing resolver.
A location that already has a proxy_pass or a grpc_pass made kong_pass
report "\"kong_pass\" directive is duplicate", because the set handler's
message was returned as kong_pass's own. Nothing about kong_pass is
duplicated there, and the message sends the reader looking for a second
one. Log such a message against the directive that produced it instead:
"\"kong_pass\" cannot use \"proxy_pass\": is duplicate".
The selector and version= also took only the "$name" spelling, so
"${name}", which nginx accepts wherever a variable appears, reached
ngx_http_get_variable_index() with the braces still on and failed at
startup with "unknown \"{name}\" variable". Parse both spellings in one
place, which the two arguments now share.
kong_pass learned both spellings nginx accepts for a variable, but
lua_kong_load_var_index and lua_kong_error_log_request_id still took only
"$name", and passed "${name}" through with the braces attached, so nginx
failed at startup with unknown "{name}" variable.
Move the parser kong_pass grew out of its own file, which is compiled
only where proxy_pass and grpc_pass are, into ngx_http_lua_kong_var_index.c,
which is always compiled and already indexes variables, and use it from
all three directives. Each keeps reporting a non-variable argument in its
own words, through the NGX_DECLINED return.
Both callers now include ngx_http_lua_kong_directive.h rather than
ngx_http_lua_kong_common.h, so that they see the prototypes of the
directive handlers they define.
ngx_http_get_indexed_variable() returns whatever r->variables holds, a cached value and a cached not_found alike, even where nginx marked the variable NGX_HTTP_VAR_NOCACHEABLE. Eleven of the variables this module indexes by default are marked that way, $args, $is_args, $request_length, $request_method, $upstream_status and the upstream timestamps among them, so reading one of those twice in a request answered with the first read: $upstream_status read before the upstream had answered kept answering nil for the rest of the request, blanking it in the access log. ngx.var does not behave that way, because ngx_http_get_variable() sends an indexed variable through ngx_http_get_flushed_variable(), which drops a non-cacheable entry before reading it. Read by index through the same accessor, so that reaching a variable by index is the only difference.
nginx caches a variable it did not mark non-cacheable, on the grounds
that what it reads does not change during a request. A request header
does change, when Lua rewrites it. ngx.var stays right about that only by
accident: a variable nginx never put in variables_hash is read through a
prefix lookup that caches nothing. Reading by index does cache, so the
indexed read answered a rewritten header with what it held beforehand:
- clear_header left $http_<name> holding the cleared value
- set_header("Cookie") left every $cookie_ variable on the old cookies
- set_header("Content-Type") left $content_type on the old type
set_header alone escaped this, because the hook dropped the variable's
index and so pushed later reads onto the uncached prefix lookup. That
also dropped it for every later request the worker served, and clearing a
header had no hook at all.
Add an FFI call that drops what a variable cached, for the current
request, and invalidate through it instead: the $http_ name for any
header, $content_type or $content_length where the header is one of
those, and every $cookie_ variable when the Cookie header is rewritten.
The variable stays indexed, so later requests keep the faster read.
set_uri_args needs no hook at all: $args, $is_args, $query_string and
$arg_ are all non-cacheable, so reading one by index re-reads it since
the getter started flushing them. Drop that wrapper.
$uri is read on the proxy path, by the balancer hashing on it, and no Kong template names it, so it took the slower unindexed read. Index it: that is only safe now that an indexed read flushes a non-cacheable variable, since ngx.req.set_uri would otherwise leave it stale. The two upstream TLS variables answer not_found until there is an upstream connection to ask, and nginx caches a not_found as readily as a value, so asking during access left header_filter and the log phase being told there is nothing. Mark just that answer non-cacheable, rather than the variables: the raw certificate one encodes the peer certificate into the request pool on every call, so re-reading it per read would trade a stale value for unbounded per-request allocation. A value, once there is one, still caches. t/006's TEST 15 covers the same sequence but with no lua_kong_load_var_index, so the variable is not indexed there and nginx caches nothing; the new test indexes it, as Kong does.
ngx_http_lua_kong_ffi_var_get_by_index() called ngx_http_get_flushed_variable() directly, whose fast path indexes r->variables[index] with no range check, unlike ngx_http_get_indexed_variable(). Its set/invalidate siblings already guard against an out-of-range index; add the same check here.
kong_pass_handler read both through ngx_http_get_indexed_variable(), which hands back whatever r->variables already holds, cached not_found and all. $selector and version= are commonly bound to a $arg_ variable or similar, which nginx marks NGX_HTTP_VAR_NOCACHEABLE, so an earlier read elsewhere in the request (a plugin, a rewrite) could leave kong_pass dispatching on a stale value instead of the fresh, per-request one the README documents.
The description said a value "starting with grpc" dispatches to grpc_pass, but ngx_http_lua_kong_pass_handler() matches only the exact strings "grpc" or "grpcs", never a prefix. t/015-kong-pass.t TEST 22 already exercises this: a selector of "grpcfoo" does not route to grpc_pass and instead fails through proxy_pass. Correct the wording so it matches the implementation.
ngx_http_lua_kong_pass_invoke() hands proxy_pass's/grpc_pass's set handler a hand-built, 2-element cf->args on the stack, relying on both directives taking exactly one argument today. Nothing enforced that here, unlike a directive read from the configuration file, whose argument count the core parser checks against cmd->type before ever calling set(). If a future nginx changed either directive's arity, its set handler could read or write past the stack array with no compiler warning and no config-time error. Check cmd->type against NGX_CONF_TAKE1 before building items, so a mismatch fails configuration loudly instead of corrupting memory.
ngx_http_lua_kong_get_upstream_raw_certificate() and ngx_http_lua_kong_get_upstream_tls_protocol() each inlined the same not_found/no_cacheable block, with the same explanatory comment, under a local "not_found" label. A future fix to that fallback (or a third such getter) could easily land on one copy and miss the other, silently reintroducing the staleness bug this behavior exists to avoid. Factor it into ngx_http_lua_kong_upstream_tls_not_found() and have both getters return through it.
The `ver->len == 1 && ver->data[0] == '2'` check was duplicated verbatim across the two branches of the NGX_HTTP_LUA_KONG_HAVE_PROXY_V2 #if/#else — one calling ngx_http_proxy_v2_handler, the other falling back to proxy_handler. Since the two branches never compile together, a future change to what counts as the HTTP/2 version value could easily be applied to only one of them without anyone noticing. Evaluate the condition once and gate only the two handler bodies on the build config.
if [ $HTTP_PROXY = YES -a $HTTP_GRPC = YES -a $HTTP_V2 = YES ] expands each variable unquoted into the test expression. A stock nginx configure always assigns these YES/NO, so this doesn't misfire today, but an unusual configure wrapper leaving one unset would abort with a "test: argument expected" shell error instead of just skipping the kong_pass build. Quote them, consistent with the rest of this file.
This file contains hidden or 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
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.
No description provided.