Skip to content
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

bug: proxy-rewrite header priority wrong #11587

Open
bpasson opened this issue Sep 18, 2024 · 7 comments
Open

bug: proxy-rewrite header priority wrong #11587

bpasson opened this issue Sep 18, 2024 · 7 comments
Labels
bug Something isn't working

Comments

@bpasson
Copy link

bpasson commented Sep 18, 2024

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

  1. Run APISIX via docker image
  2. 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"
        }
      }
    }
  }
}
  1. 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
@dosubot dosubot bot added the bug Something isn't working label Sep 18, 2024
@zhoujiexiong
Copy link
Contributor

@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
Copy link
Author

bpasson commented Sep 18, 2024

@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
Copy link
Contributor

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

@bpasson Try this reference first?

@bpasson
Copy link
Author

bpasson commented Sep 18, 2024

@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
Copy link
Contributor

@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
Copy link
Author

bpasson commented Sep 19, 2024

@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
Copy link
Author

bpasson commented Sep 19, 2024

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: 📋 Backlog
Development

No branches or pull requests

2 participants