bug: proxy-rewrite header priority wrong

bpasson commented 1 week ago

Current Behavior

The current priority for header operations is: add > remove > set. This makes it impossible to handle the use-case where you want to add multiple headers with the same name but first remove all the headers with that name. The following configuration would make sense in my opinion:

"proxy-rewrite": {
   "headers": {
       "remove: ["foo"]
       "add": {
              "foo": "bar1",
              "foo": "bar2",
        }
    }
}

But due to the header priority add > remove > set this leads to no foo headers, as first they are added and then all removed again.

Expected Behavior

The expected behaviour would be to have both foo headers added and all earlier foo headers removed. For this to work the priority order should be remove > set > add. The set operation is basically a shortcut for remove, add but is confusing when you need a header multiple times, which set does not allow.

Error Logs

No response

Steps to Reproduce

Run APISIX via docker image

Create a route:

{
"uri": "/*",
"name": "httpbin-demo",
"host": "httpbin.org",
"upstream": {
"scheme": "https",
"type": "roundrobin",
"nodes": {
  "httpbin.org:443": 1
}
},
"plugins": {
"proxy-rewrite": {
  "headers": {
    "remove": ["foo"],
    "add": {
      "foo": "bar2",
      "foo": "bar3"
    }
  }
}
}
}

Test using `curl -H"foo: bar1" -H"Host: httpbin.org" http://localhost:9080/anything

The resulting json is missing the expected foo: bar1 and foo: bar2 headers.

Environment

APISIX version (run apisix version): Stock docker container with version 3.10.0
Operating system (run uname -a): Not relevant, but Ubuntu 24.04 LTS
OpenResty / Nginx version (run openresty -V or nginx -V):
etcd version, if relevant (run curl http://127.0.0.1:9090/v1/server_info): Not relevant
APISIX Dashboard version, if relevant: Not relevant
Plugin runner version, for issues related to plugin runners: Not relevant
LuaRocks version, for installation issues (run luarocks --version): Not relevant

zhoujiexiong commented 1 week ago

@bpasson

The Header Priority mentioned in the document should refer to the priority of the header configuration to take effect, so the order of configuration application should be remove | set, then add to ensure the Header Priority. That would be more reasonable and also meet your current scenario.

So will you raise a PR to fix this problem? :D

bpasson commented 1 week ago

@zhoujiexiong I have very little knowledge about Lua language, but I will try I suppose this is not much more than swapping some lines. Is there a dev-container I can use for a local build?

zhoujiexiong commented 1 week ago

Is there a dev-container I can use for a local build?

@bpasson Try this reference first?

bpasson commented 1 week ago

@zhoujiexiong I followed there reference and got stuk at docker exec -it apisix-dev-env make run which gave the error:

root@lima-rancher-desktop:/apisix# make run
/bin/bash: -V: invalid option
Usage:  /bin/bash [GNU long option] [option] ...
    /bin/bash [GNU long option] [option] script-file ...
GNU long options:
    --debug
    --debugger
    --dump-po-strings
    --dump-strings
    --help
    --init-file
    --login
    --noediting
    --noprofile
    --norc
    --posix
    --pretty-print
    --rcfile
    --restricted
    --verbose
    --version
Shell options:
    -ilrsD or -c command or -O shopt_option     (invocation only)
    -abefhkmnptuvxBCHP or -o option
[ info ] Use openresty as default runtime
[ info ] run -> [ Start ]
/apisix/bin/apisix start
ERROR: Please check the version of OpenResty and Lua, OpenResty 1.19+ + LuaJIT is required for Apache APISIX.
[ info ] run -> [ Done ]

Any suggestions?

zhoujiexiong commented 1 week ago

@bpasson

docker exec -it apisix-dev-env make deps

Did this step run successfully?

And what is the output of docker exec -it apisix-dev-env which openresty?

bpasson commented 1 week ago

@zhoujiexiong I tried rebuilding and now it won't build any more. I had to change the tar line from linux-install-luarocks.sh from tar -xf v"$LUAROCKS_VER".tar.gz to tar -xf v"$LUAROCKS_VER".tar.gz --no-same-owner to prevent tar giving errors like: Cannot change ownership to uid 0, gid 0: Permission denied. That made build continue, but in the end I ran into:

Error: Failed installing dependency: https://luarocks.org/api7-lua-resty-dns-client-7.0.1-0.src.rock - Failed installing dependency: https://luarocks.org/penlight-1.14.0-2.src.rock - Build error: Failed copying contents of 'lua' directory: Failed copying lua to /apisix/deps/lib/luarocks/rocks-5.1/penlight/1.14.0-2/lua

No idea why, but I assume it has something to do with the --no-same-owner I had to include to even make tar work.

Further I'm guessing the /bin/bash: -V: invalid option has something to do with the makefile line:

ENV_RUNTIME_VER      ?= $(shell $(ENV_NGINX_EXEC) -V 2>&1 | tr ' ' '\n'  | grep 'APISIX_RUNTIME_VER' | cut -d '=' -f2)

My guess is $(ENV_NGINX_EXEC) resolved to an empty value, which gives shell ( bash ) the -V option and that does not exit.

I tried doing all from scratch, from a clean checkout with a clean new docker image build. That did not help. I keep getting stuck at this point now, or earlier if I remove the --no-same-owner.

I use Rancher Desktop btw with moby as container engine all running on OSX aarch64.

Any suggestions on how to get around this?

bpasson commented 1 week ago

@zhoujiexiong I managed to get it to compile and not give errors. I skip the docker mount and just do all editing in the container. Though running the testcase from the reference now gives me:

root@lima-rancher-desktop:/apisix# prove t/admin/routes.t
t/admin/routes.t .. Use of uninitialized value $version in pattern match (m//) at t/APISIX.pm line 144.
Use of uninitialized value $version in pattern match (m//) at t/APISIX.pm line 193.
Use of uninitialized value $version in pattern match (m//) at t/APISIX.pm line 225.
Use of uninitialized value $version in pattern match (m//) at t/APISIX.pm line 234.
Bailout called.  Further testing stopped:  Failed to get the version of the Nginx in PATH:
FAILED--Further testing stopped: Failed to get the version of the Nginx in PATH:

Any suggestions?

apache / apisix