-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat(proxy-rewrite): support NGINX variables in regex_uri #13800
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,9 +22,11 @@ local ipairs = ipairs | |
| local ngx = ngx | ||
| local type = type | ||
| local re_sub = ngx.re.sub | ||
| local re_gsub = ngx.re.gsub | ||
| local re_match = ngx.re.match | ||
| local req_set_uri = ngx.req.set_uri | ||
| local sub_str = string.sub | ||
| local str_gsub = string.gsub | ||
| local str_find = core.string.find | ||
|
|
||
| local switch_map = {GET = ngx.HTTP_GET, POST = ngx.HTTP_POST, PUT = ngx.HTTP_PUT, | ||
|
|
@@ -44,6 +46,26 @@ local lrucache = core.lrucache.new({ | |
| type = "plugin", | ||
| }) | ||
|
|
||
| local nginx_var_pattern = [[(?<!\$)\$(?=[a-zA-Z_]|\{\s*[a-zA-Z_])]] | ||
|
|
||
|
|
||
| local function escape_nginx_vars(replacement) | ||
| local escaped, _, err = re_gsub(replacement, nginx_var_pattern, function() | ||
| return "$$" | ||
| end, "jo") | ||
| return escaped, err | ||
| end | ||
|
|
||
|
|
||
| local function preserve_literal_dollars(replacement) | ||
| return str_gsub(replacement, "%$%$", "\\$") | ||
| end | ||
|
|
||
|
|
||
| local function restore_literal_dollars(replacement) | ||
| return str_gsub(replacement, "\\%$", "$") | ||
| end | ||
|
|
||
| core.ctx.register_var("proxy_rewrite_regex_uri_captures", function(ctx) | ||
| return ctx.proxy_rewrite_regex_uri_captures | ||
| end) | ||
|
|
@@ -225,6 +247,16 @@ function _M.check_schema(conf) | |
| if not secret.is_secret_ref(pattern) then | ||
| local test_replacement = secret.is_secret_ref(replacement) | ||
| and "" or replacement | ||
| if test_replacement ~= "" then | ||
| -- Keep validating PCRE captures without treating NGINX | ||
| -- variables in the replacement as named captures. | ||
| local err | ||
| test_replacement, err = escape_nginx_vars(test_replacement) | ||
| if err then | ||
| return false, "invalid regex_uri replacement(" .. | ||
| replacement .. "): " .. err | ||
| end | ||
| end | ||
| local _, _, err = re_sub("/fake_uri", pattern, | ||
| test_replacement, "jo") | ||
| if err then | ||
|
|
@@ -355,8 +387,14 @@ function _M.rewrite(conf, ctx) | |
| if captures then | ||
| ctx.proxy_rewrite_regex_uri_captures = captures | ||
|
|
||
| local replacement = preserve_literal_dollars(conf.regex_uri[i + 1]) | ||
| replacement = core.utils.resolve_var_with_captures(replacement, captures) | ||
| replacement = core.utils.resolve_var(replacement, ctx.var, escape_separator) | ||
| replacement = restore_literal_dollars(replacement) | ||
| local uri, _, err = re_sub(upstream_uri, | ||
| conf.regex_uri[i], conf.regex_uri[i + 1], "jo") | ||
| conf.regex_uri[i], function() | ||
| return replacement | ||
| end, "jo") | ||
|
Comment on lines
+390
to
+397
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's too complicated. Is there a simpler way?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The additional steps preserve three different replacement semantics:
Resolving NGINX variables directly would also consume I also tested resolving variables after I can move these stages into a focused helper to make the rewrite path easier to read. Would that address the concern, or would you prefer a different replacement behavior for literal |
||
| if uri then | ||
| upstream_uri = uri | ||
| else | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| # | ||
| # Licensed to the Apache Software Foundation (ASF) under one or more | ||
| # contributor license agreements. See the NOTICE file distributed with | ||
| # this work for additional information regarding copyright ownership. | ||
| # The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| # (the "License"); you may not use this file except in compliance with | ||
| # the License. You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| # | ||
| BEGIN { | ||
| if ($ENV{TEST_NGINX_CHECK_LEAK}) { | ||
| $SkipReason = "unavailable for the hup tests"; | ||
|
|
||
| } else { | ||
| $ENV{TEST_NGINX_USE_HUP} = 1; | ||
| undef $ENV{TEST_NGINX_USE_STAP}; | ||
| } | ||
| } | ||
|
|
||
| use t::APISIX 'no_plan'; | ||
|
|
||
| repeat_each(1); | ||
| no_long_string(); | ||
| no_shuffle(); | ||
| no_root_location(); | ||
| run_tests; | ||
|
|
||
| __DATA__ | ||
|
|
||
| === TEST 1: set route(regex_uri with capture and NGINX variable) | ||
| --- config | ||
| location /t { | ||
| content_by_lua_block { | ||
| local t = require("lib.test_admin").test | ||
| local code, body = t('/apisix/admin/routes/1', | ||
| ngx.HTTP_PUT, | ||
| [[{ | ||
| "plugins": { | ||
| "proxy-rewrite": { | ||
| "regex_uri": ["^/api/(.*)$", | ||
| "/plugin_proxy_rewrite_args?c=$1&n=$arg_name&l=$$x"] | ||
| } | ||
| }, | ||
| "upstream": { | ||
| "nodes": { | ||
| "127.0.0.1:1980": 1 | ||
| }, | ||
| "type": "roundrobin" | ||
| }, | ||
| "uri": "/api/*" | ||
| }]] | ||
| ) | ||
|
|
||
| if code >= 300 then | ||
| ngx.status = code | ||
| end | ||
| ngx.say(body) | ||
| } | ||
| } | ||
| --- request | ||
| GET /t | ||
| --- response_body | ||
| passed | ||
|
|
||
|
|
||
|
|
||
| === TEST 2: hit route(regex_uri resolves capture and NGINX variable) | ||
| --- request | ||
| GET /api/team?name=alice HTTP/1.1 | ||
| --- response_body | ||
| uri: /plugin_proxy_rewrite_args | ||
| c: team | ||
| l: $x | ||
| n: alice | ||
| name: alice |
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.
Why is this needed?
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.
This is needed because
check_schemacompiles the replacement withngx.re.subto validate both the regex pattern and replacement syntax.Without escaping NGINX variables,
$arg_nameis interpreted as a named PCRE capture and route creation fails with "failed to compile the replacement template", which is the original issue.Escaping only NGINX variable markers as
$$allows PCRE to keep validating regular captures such as$1and invalid replacement syntax, while treating$arg_nameas a literal during schema validation. The existing invalid replacement test also depends on this validation.I can add a comment here to explain this.
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.
Added an inline comment in
d1b362bexplaining that schema validation must keep validating PCRE captures without treating NGINX variables as named captures.