Closed andrey-zelenkov closed 4 months ago
Hi @andrey-zelenkov,
This PR looks good, but could you separate it into two patches?
The first one is to test "rewrite" together with "proxy" without changing any rules, I mean $requst_uri
is still changeable, it doesn't depend on https://github.com/nginx/unit/pull/1162.
Then I'll commit a patch separated from #1162, it will rework the "proxy" module.
Let me know if you are confused with the idea.
Dropped the first patch to submit it as a separate pull request.
% git range-diff 0852b615...51cdaa9c
1: 5030a900 < -: -------- Tests: added $rewrite_uri test with proxy
2: 0852b615 = 1: 51cdaa9c Tests: added REQUEST_URI test with rewrite rule
Is this not an issue?
=================================== FAILURES ===================================
__________________ test_python_application_variables[3.10.12] __________________
date_to_sec_epoch = <function date_to_sec_epoch.<locals>._date_to_sec_epoch at 0x7fe46df140d0>
sec_epoch = 1712839114.0
def test_python_application_variables(date_to_sec_epoch, sec_epoch):
client.load('variables')
body = 'Test body string.'
resp = client.http(
f"""POST / HTTP/1.1
Host: localhost
Content-Length: {len(body)}
Custom-Header: blah
Custom-hEader: Blah
Content-Type: text/html
Connection: close
custom-header: BLAH
{body}""".encode(),
raw=True,
)
assert resp['status'] == 200, 'status'
headers = resp['headers']
header_server = headers.pop('Server')
assert re.search(r'Unit/[\d\.]+', header_server), 'server header'
assert (
headers.pop('Server-Software') == header_server
), 'server software header'
date = headers.pop('Date')
assert date[-4:] == ' GMT', 'date header timezone'
assert abs(date_to_sec_epoch(date) - sec_epoch) < 5, 'date header'
assert headers == {
'Connection': 'close',
'Content-Length': str(len(body)),
'Content-Type': 'text/html',
'Request-Method': 'POST',
'Request-Uri': '/',
'Http-Host': 'localhost',
'Server-Protocol': 'HTTP/1.1',
'Custom-Header': 'blah, Blah, BLAH',
'Wsgi-Version': '(1, 0)',
'Wsgi-Url-Scheme': 'http',
'Wsgi-Multithread': 'False',
'Wsgi-Multiprocess': 'True',
'Wsgi-Run-Once': 'False',
}, 'headers'
assert resp['body'] == body, 'body'
# REQUEST_URI unchanged
path = f'{option.test_dir}/python/variables'
assert 'success' in client.conf(
{
"listeners": {"*:8080": {"pass": "routes"}},
"routes": [
{
"action": {
"rewrite": "/foo",
"pass": "applications/variables",
}
}
],
"applications": {
"variables": {
"type": client.get_application_type(),
"processes": {'spare': 0},
"path": path,
"working_directory": path,
"module": "wsgi",
}
},
}
)
resp = client.http(
f"""POST /bar HTTP/1.1
Host: localhost
Content-Length: 1
Custom-Header: blah
Content-Type: text/html
Connection: close
a""".encode(),
raw=True,
)
> assert resp['headers']['Request-Uri'] == '/bar', 'REQUEST_URI unchanged'
E AssertionError: REQUEST_URI unchanged
E assert '/foo' == '/bar'
E
E - /bar
E + /foo
test/test_python_application.py:105: AssertionError
=========================== short test summary info ============================
FAILED test/test_python_application.py::test_python_application_variables[3.10.12] - AssertionError: REQUEST_URI unchanged
assert '/foo' == '/bar'
- /bar
+ /foo
Is this not an issue?
This should be fixed once #1162 is merged.
Rebased:
% git range-diff 51cdaa9c...e04be85a
-: -------- > 1: d494d2eb Wasm-wc: Bump the h2 crate from 0.4.2 to 0.4.4
-: -------- > 2: e6d8fc66 njs (lowercase) is more preferred way to mention
-: -------- > 3: 6e79da47 Docs: njs (lowercase) is more preferred way to mention
-: -------- > 4: 5f606742 Tests: added $request_uri tests with proxy
-: -------- > 5: a625a0b1 Tests: compatibility with OpenSSL 3.2.0
-: -------- > 6: 2d7a8468 HTTP: Added variable validation to the response_headers option
-: -------- > 7: 62697773 Tests: error report corrected for unknown variables in "response_headers"
1: 51cdaa9c = 8: e04be85a Tests: added REQUEST_URI test with rewrite rule
Bit of a chicken and egg problem...
I think I'd wait for the code change to be merged first, then we won't get failing tests all the time...
The other option would be to have Zhidao merge it via his prll-request, which would be a completely reasonable thing to do...
The two latest commits, 2d7a84684312fc1c46043fa10af0ea336e73f11d and 626977730f0e9029ee15b6321d35cb5aa311379d, had the exact same problem. So I was planning to wait for https://github.com/nginx/unit/pull/1162 to be merged before pushing this one (as we did yesterday).
Another option is to commit this test under pytest.skip()
call and add skip removal in #1162, but I am personally would prefer just to wait.
Yes, waiting's good...
Hi @andrey-zelenkov @ac000, Does it make sense to release #1162 and #1197 in a single commit? If released separately, the tests won't pass.
The other option would be to have Zhidao merge it via his prll-request, which would be a completely reasonable thing to do...
LGTM.
Hi @andrey-zelenkov,
It seems you missed reworking test/test_rewrite.py
.
Does it make sense to release #1162 and #1197 in a single commit? If released separately, the tests won't pass.
Not the same commit. But they could go via the same pull-request...
Once Andrei and you are happy with the test. You could pull the patch, e.g
$ wget https://github.com/nginx/unit/commit/e04be85ab5af4ef4ff5da89750ef1b487bc32a91.patch
(Don't just use that URL as the commit id will change if the commit is modified)
You can then apply it to your tree with
$ git am e04be85ab5af4ef4ff5da89750ef1b487bc32a91.patch
Hi @andrey-zelenkov, It seems you missed reworking test/test_rewrite.py.
Last time when we discussed that it was necessary to fix relevant log message https://github.com/nginx/unit/blob/master/src/nxt_http_rewrite.c#L110.
Was noted here: https://github.com/nginx/unit/pull/1162#discussion_r1522782113 No plans to fix this anymore?
Hi @andrey-zelenkov, It seems you missed reworking test/test_rewrite.py.
Last time when we discussed that it was necessary to fix relevant log message https://github.com/nginx/unit/blob/master/src/nxt_http_rewrite.c#L110.
Was noted here: #1162 (comment) No plans to fix this anymore?
Fixed, thanks.
Hi Andrei, Take a look at the test, it's broken with https://github.com/nginx/unit/pull/1162.
assert (
wait_for_record(fr'^::1 /bar /bar\?a=b$', 'access.log') is not None
), 'req 8081 (proxy) rewrite'
E AssertionError: req 8081 (proxy) rewrite
E assert None is not None
E + where None = <function wait_for_record.<locals>._wait_for_record at 0x7f62e92daaf0>('^::1 /bar /bar\\?a=b$', 'access.log')
FAILED test/test_variables.py::test_variables_request_uri - AssertionError: re...
I think it's because of the rule change of request uri which is switched from changeable to constant.
Hi @andrey-zelenkov, If you don't mind, I'll close this PR :)
Sure!
For history: this PR was merged as part of #1162
Tests for the https://github.com/nginx/unit/pull/1162