unit icon indicating copy to clipboard operation
unit copied to clipboard

HTTP: improved request uri.

Open hongzhidao opened this issue 11 months ago • 3 comments

  1. Another implementation of proxy request creation.
  2. Made the $request_uri constant.
  3. Passed constant REQUEST_URI to applications.

Need to separate the PR after tests pass.

Here's a manual test.

# conf.json

{
    "listeners": {
        "*:8080": {
            "pass": "routes/a"
        },
        "*:8081": {
            "pass": "routes/b"
        }
    },
    "routes": {
        "a": [
            {
                "action": {
                    "rewrite": "/foo.php",
                    "proxy": "http://127.0.0.1:8081"
                }
            }
        ],
        "b": [
            {
                "action": {
                    "rewrite": "/index.php",
                    "pass": "applications/app"
                }
            }
        ]
    },
    "applications": {
        "app": {
            "type": "php",
            "root": "/www",
            "index": "index.php"
        }
    }
}
# /www/index.php

<?php

phpinfo();
> curl http://127.1:8080
$_SERVER['REQUEST_URI'] => /foo.php

hongzhidao avatar Feb 28 '24 08:02 hongzhidao

Hi @andrey-zelenkov, Since this is a change, we need to add more tests to it.

  1. add rewrite on "proxy" which is similar to "php application rewrite". It doesn't depend on the PR.
  2. change the tests for $request_uri if possible.
  3. change the tests for "applications" if possible. Thanks.

hongzhidao avatar Feb 28 '24 08:02 hongzhidao

Hmm, I thought the idea was that REQUEST_URI doesn't get munged?

> curl http://127.1:8080
$_SERVER['REQUEST_URI'] => /foo.php

In which case I would have expected REQUEST_URI would be set to / in the above instance.

ac000 avatar Mar 05 '24 06:03 ac000

Hmm, I thought the idea was that REQUEST_URI doesn't get munged?

> curl http://127.1:8080
$_SERVER['REQUEST_URI'] => /foo.php

In which case I would have expected REQUEST_URI would be set to / in the above instance.

Here are a few namings:

  1. REQEUST_URI is for applications, it must be unchanged.
  2. $request_uri corresponding internal r->target, both of them are unchanged.

So your understanding is correct. For the above configuration, there are two requests. The first one is the client request and is accepted by 8080.

"action": {
                    "rewrite": "/foo.php",
                    "proxy": "http://127.0.0.1:8081"
                }

The request_uri is unchanged. Then Units generates the second request with a rewritten uri to pass to 8081. Now the second request is /foo.php, and it will be accepted by 8081.

{
                "action": {
                    "rewrite": "/index.php",
                    "pass": "applications/app"
                }
            }

hongzhidao avatar Mar 05 '24 07:03 hongzhidao

Hi, I created a new PR https://github.com/nginx/unit/pull/1214 prepared for this patch, and it will simplify this PR a lot. I'll rework this PR after https://github.com/nginx/unit/pull/1214 and https://github.com/nginx/unit/pull/1211 are shipped.

hongzhidao avatar Apr 09 '24 11:04 hongzhidao

@andrey-zelenkov @hongzhidao: I want to double check the failing tests: https://github.com/nginx/unit/actions/runs/8718395388/job/23915553886?pr=1162#step:41:1060

I tested manually, and given the configuration in the test and the request, the log line ends up being

::1 /bar /blah%2fblah?a=b
192.168.65.1 /foo /blah%2fblah?a=b

This is correct given the request URI. I think those tests should be changed given the outcome of the code they're testing has been changed in this PR.

I'm working on a PR to this branch at https://github.com/hongzhidao/unit/pull/1, but wanted to run this by you as you're most familiar with the tests 🙂

I'll keep working on that PR as I fix the tests.

javorszky avatar Apr 26 '24 13:04 javorszky