change(core): keep injecting the X-Forwarded-* headers, append instead of replace

Review asked for the request-header visibility to be preserved rather than
traded away. It can be, and most of the speedup survives.

The fast path no longer returns without writing the headers. Route `vars`,
`ngx.req.get_headers()`, and the exits that copy `r->headers_in` instead of
reading `$var_x_forwarded_*` -- `@grpc_pass`, `@dubbo_pass`, the mirror
subrequest -- all observe them, and skipping the write changed what every one of
those saw. On a default install that is not an edge case: with no
`trusted_addresses` configured every peer is untrusted, so the fast path is the
common path.

What is still worth skipping is the cost of *replacing* a header. `set_header`
walks the header list to find the old entry and remove it before inserting; on
this path the entry is known absent, since that is the condition for being here,
so `add_header` is enough. The rest of the old slow path is a no-op on this
input anyway -- every `original_x_forwarded_*` value is nil, and clearing
`Forwarded` or `X-Forwarded-For` has nothing to clear -- so it stays skipped.

Eight rounds, alternating, single worker, wrk2 at saturation, worker on one
P-core and the load generator on two others (the earlier number in this PR was
taken with the worker and the load generator on hyperthread siblings and is not
comparable):

| variant                   | mean   | range         |
|---------------------------|--------|---------------|
| master                    | 79,345 | 77,772-80,806 |
| this commit               | 86,145 | 84,368-87,417 |
| previous commit (skipped) | 89,726 | 87,945-91,423 |

+8.6% instead of +13.1%, with the three ranges disjoint across all eight rounds.

The only behaviour left that differs from master is the one that was always a
fix: a plugin-set X-Forwarded-* now reaches the upstream, because
`set_upstream_x_forwarded_headers` reads `ngx.var` instead of the `ctx.var`
entry that a stale invalidation left behind. `core.request.set_header`
invalidates `ctx.var` under the hyphenated header name while `core/ctx.lua`
caches the underscored one; that cause deserves its own fix.

`t/plugin/proxy-mirror4.t` TEST 1 pinned the behaviour being given up here and
now asserts the values master produces. `t/core/trusted-addresses.t` TEST 17 and
TEST 18 pin the two visibility consumers that have no other coverage: a plugin
reading the whole header table, and a route matching on `http_x_forwarded_proto`.

Author: AlinsRan

apisix/init.lua

@@ -706,27 +706,32 @@ local function handle_x_forwarded_headers(api_ctx)
     local xf_for = ngx_var.http_x_forwarded_for
     local forwarded = ngx_var.http_forwarded
 
-    -- Nothing forgeable was sent, so there is nothing to neutralize: the request
-    -- headers are left untouched, which skips four `ngx.req.set_header()` calls.
-    -- The values handed to the upstream must not change though, and the
-    -- `set $var_x_forwarded_*` defaults in ngx_tpl.lua resolve to
-    -- $scheme/$host/$server_port, so they drop an explicit port carried by the
-    -- Host header -- which is the port the client actually used, and the only
-    -- correct X-Forwarded-Port on a port-mapped deployment. Write the variables
-    -- only in the cases where those defaults would lose information; `$scheme`
-    -- never needs one.
+    -- Nothing forgeable was sent, so there is nothing to neutralize -- but the
+    -- headers still have to be there. Route `vars`, `ngx.req.get_headers()`, and
+    -- the exits that copy `r->headers_in` rather than reading `$var_x_forwarded_*`
+    -- (`@grpc_pass`, `@dubbo_pass`, the mirror subrequest) all observe them, so
+    -- skipping the write would change what those see.
+    --
+    -- What can be skipped is the cost of *replacing* a header. `set_header` walks
+    -- the header list to find the old entry and removes it; here the entry is
+    -- known absent -- that is the condition for being on this path -- so appending
+    -- is enough. The rest of the slow path is a no-op on this input anyway: the
+    -- `original_x_forwarded_*` values are all nil, and clearing `Forwarded` or
+    -- `X-Forwarded-For` has nothing to clear.
     if not (xf_proto or xf_host or xf_port or xf_for or forwarded) then
-        local http_host = ngx_var.http_host
-        if http_host then
-            local _, port_from_host = core.utils.parse_addr(http_host)
-            if port_from_host then
-                api_ctx.var.var_x_forwarded_host = http_host
-                api_ctx.var.var_x_forwarded_port = port_from_host
-            elseif http_host ~= api_ctx.var.host then
-                -- `$host` is lower-cased, the Host header is not
-                api_ctx.var.var_x_forwarded_host = http_host
-            end
-        end
+        local proto = api_ctx.var.scheme
+        local http_host = ngx_var.http_host or api_ctx.var.host
+        -- parse_addr handles IPv6 literals and bracketed host:port correctly.
+        local _, port_from_host = core.utils.parse_addr(http_host)
+        local port = port_from_host or api_ctx.var.server_port
+
+        core.request.add_header(api_ctx, "X-Forwarded-Proto", proto)
+        core.request.add_header(api_ctx, "X-Forwarded-Host", http_host)
+        core.request.add_header(api_ctx, "X-Forwarded-Port", port)
+
+        api_ctx.var.http_x_forwarded_proto = proto
+        api_ctx.var.http_x_forwarded_host = http_host
+        api_ctx.var.http_x_forwarded_port = port
         return
     end
 

t/core/trusted-addresses.t

@@ -641,3 +641,69 @@ x-forwarded-host: LOCALHOST
 x-forwarded-port: 1984
 x-forwarded-proto: http
 x-real-ip: 127.0.0.1
+
+
+
+=== TEST 17: a request carrying no X-Forwarded-* still gets them in r->headers_in
+--- yaml_config
+apisix:
+    node_listen: 1984
+    enable_admin: false
+deployment:
+    role: data_plane
+    role_data_plane:
+        config_provider: yaml
+--- apisix_yaml
+routes:
+  -
+    id: 1
+    uri: /old_uri
+    plugins:
+        serverless-pre-function:
+            phase: rewrite
+            functions:
+              - return function(conf, ctx) local h = ngx.req.get_headers(); ngx.log(ngx.WARN, "header-table xfp=", tostring(h["x-forwarded-proto"]), " xfh=", tostring(h["x-forwarded-host"]), " xfport=", tostring(h["x-forwarded-port"])) end
+    upstream:
+        nodes:
+            "127.0.0.1:1980": 1
+        type: roundrobin
+#END
+--- request
+GET /old_uri
+--- error_log
+header-table xfp=http xfh=localhost xfport=1984
+
+
+
+=== TEST 18: a route matching on http_x_forwarded_proto still matches without one
+--- yaml_config
+apisix:
+    node_listen: 1984
+    enable_admin: false
+deployment:
+    role: data_plane
+    role_data_plane:
+        config_provider: yaml
+--- apisix_yaml
+routes:
+  -
+    id: 1
+    uri: /old_uri
+    vars:
+      - ["http_x_forwarded_proto", "==", "http"]
+    upstream:
+        nodes:
+            "127.0.0.1:1980": 1
+        type: roundrobin
+#END
+--- request
+GET /old_uri
+--- error_code: 200
+--- response_body
+uri: /old_uri
+host: localhost
+x-forwarded-for: 127.0.0.1
+x-forwarded-host: localhost
+x-forwarded-port: 1984
+x-forwarded-proto: http
+x-real-ip: 127.0.0.1

t/plugin/proxy-mirror4.t

@@ -17,9 +17,9 @@
 # The mirror subrequest, `@grpc_pass` and `@dubbo_pass` copy `r->headers_in`
 # instead of reading `$var_x_forwarded_*` the way `location /` does through
 # `proxy_set_header`. They therefore observe whether `handle_x_forwarded_headers`
-# wrote the X-Forwarded-* request headers, which it only does for a request that
-# carried a forgeable value. The mirror stands in for all three here because it
-# is the one that can be driven over plain HTTP.
+# wrote the X-Forwarded-* request headers at all, which makes them the regression
+# test for the fast path continuing to write them. The mirror stands in for all
+# three here because it is the one that can be driven over plain HTTP.
 use t::APISIX 'no_plan';
 
 repeat_each(1);
@@ -59,7 +59,7 @@ run_tests;
 
 __DATA__
 
-=== TEST 1: a request carrying no X-Forwarded-* leaves the mirror with none either
+=== TEST 1: a request carrying no X-Forwarded-* still reaches the mirror with them
 --- yaml_config
 apisix:
     node_listen: 1984
@@ -86,7 +86,7 @@ GET /hello
 --- response_body
 hello world
 --- error_log
-mirror x-forwarded-proto: nil, x-forwarded-host: nil, x-forwarded-port: nil
+mirror x-forwarded-proto: http, x-forwarded-host: localhost, x-forwarded-port: 1984