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

fix(proxy-rewrite): when conf.headers are missing,conf.method can make effect #6300

Conversation

liangliang4ward
Copy link
Contributor

@liangliang4ward liangliang4ward commented Feb 11, 2022

What this PR does / why we need it:

when conf.headers are missing,conf.method can make effect

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

@liangliang4ward liangliang4ward changed the title fix:when conf.headers are missing,conf.method can make effect fix: when conf.headers are missing,conf.method can make effect Feb 11, 2022
@liangliang4ward liangliang4ward changed the title fix: when conf.headers are missing,conf.method can make effect fix: when proxy-rewrite plugin's conf.headers are missing,conf.method can make effect Feb 11, 2022
@tokers
Copy link
Contributor

tokers commented Feb 11, 2022

@liangliang4ward Better to add some test cases to cover it.

Copy link
Member

@spacewander spacewander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix LGTM.

You can learn how to write tests via https://github.com/apache/apisix/blob/master/docs/en/latest/internal/testing-framework.md and the current test

=== TEST 3: set route(update rewrite method)
--- config
location /t {
content_by_lua_block {
local t = require("lib.test_admin").test
local code, body = t('/apisix/admin/routes/1',
ngx.HTTP_PUT,
[[{
"methods": ["GET"],
"plugins": {
"proxy-rewrite": {
"uri": "/plugin_proxy_rewrite",
"method": "GET",
"scheme": "http",
"host": "apisix.iresty.com"
}
},
"upstream": {
"nodes": {
"127.0.0.1:1980": 1
},
"type": "roundrobin"
},
"uri": "/hello"
}]]
)
if code >= 300 then
ngx.status = code
end
ngx.say(body)
}
}
--- response_body
passed
=== TEST 4: hit route(upstream uri: should be /hello)
--- request
GET /hello
--- grep_error_log_out
plugin_proxy_rewrite get method: GET

@liangliang4ward
Copy link
Contributor Author

liangliang4ward commented Feb 14, 2022

The fix LGTM.

You can learn how to write tests via https://github.com/apache/apisix/blob/master/docs/en/latest/internal/testing-framework.md and the current test

=== TEST 3: set route(update rewrite method)
--- config
location /t {
content_by_lua_block {
local t = require("lib.test_admin").test
local code, body = t('/apisix/admin/routes/1',
ngx.HTTP_PUT,
[[{
"methods": ["GET"],
"plugins": {
"proxy-rewrite": {
"uri": "/plugin_proxy_rewrite",
"method": "GET",
"scheme": "http",
"host": "apisix.iresty.com"
}
},
"upstream": {
"nodes": {
"127.0.0.1:1980": 1
},
"type": "roundrobin"
},
"uri": "/hello"
}]]
)
if code >= 300 then
ngx.status = code
end
ngx.say(body)
}
}
--- response_body
passed
=== TEST 4: hit route(upstream uri: should be /hello)
--- request
GET /hello
--- grep_error_log_out
plugin_proxy_rewrite get method: GET

in proxy-rewrite3.t ,I thought the TEST 2 case according to the original code, it should not succeed。

@spacewander
Copy link
Member

The fix LGTM.
You can learn how to write tests via https://github.com/apache/apisix/blob/master/docs/en/latest/internal/testing-framework.md and the current test

=== TEST 3: set route(update rewrite method)
--- config
location /t {
content_by_lua_block {
local t = require("lib.test_admin").test
local code, body = t('/apisix/admin/routes/1',
ngx.HTTP_PUT,
[[{
"methods": ["GET"],
"plugins": {
"proxy-rewrite": {
"uri": "/plugin_proxy_rewrite",
"method": "GET",
"scheme": "http",
"host": "apisix.iresty.com"
}
},
"upstream": {
"nodes": {
"127.0.0.1:1980": 1
},
"type": "roundrobin"
},
"uri": "/hello"
}]]
)
if code >= 300 then
ngx.status = code
end
ngx.say(body)
}
}
--- response_body
passed
=== TEST 4: hit route(upstream uri: should be /hello)
--- request
GET /hello
--- grep_error_log_out
plugin_proxy_rewrite get method: GET

in proxy-rewrite3.t ,I thought the TEST 2 case according to the original code, it should not succeed。

Let's add a test to prove your suspect. The test is passed in the CI.

@liangliang4ward
Copy link
Contributor Author

The fix LGTM.
You can learn how to write tests via https://github.com/apache/apisix/blob/master/docs/en/latest/internal/testing-framework.md and the current test

=== TEST 3: set route(update rewrite method)
--- config
location /t {
content_by_lua_block {
local t = require("lib.test_admin").test
local code, body = t('/apisix/admin/routes/1',
ngx.HTTP_PUT,
[[{
"methods": ["GET"],
"plugins": {
"proxy-rewrite": {
"uri": "/plugin_proxy_rewrite",
"method": "GET",
"scheme": "http",
"host": "apisix.iresty.com"
}
},
"upstream": {
"nodes": {
"127.0.0.1:1980": 1
},
"type": "roundrobin"
},
"uri": "/hello"
}]]
)
if code >= 300 then
ngx.status = code
end
ngx.say(body)
}
}
--- response_body
passed
=== TEST 4: hit route(upstream uri: should be /hello)
--- request
GET /hello
--- grep_error_log_out
plugin_proxy_rewrite get method: GET

in proxy-rewrite3.t ,I thought the TEST 2 case according to the original code, it should not succeed。

Let's add a test to prove your suspect. The test is passed in the CI.

this PR will to solve this issue; TEST 2 case I don't why CI will success . this is my confusion.

@liangliang4ward
Copy link
Contributor Author

@liangliang4ward Better to add some test cases to cover it.

add a test case

t/plugin/proxy-rewrite3.t Outdated Show resolved Hide resolved
t/plugin/proxy-rewrite3.t Outdated Show resolved Hide resolved
@spacewander spacewander changed the title fix: when proxy-rewrite plugin's conf.headers are missing,conf.method can make effect fix(proxy-rewrite): when conf.headers are missing,conf.method can make effect Feb 15, 2022
@spacewander spacewander merged commit bcf13cd into apache:master Feb 15, 2022
@spacewander
Copy link
Member

Merged. Thanks!

@liangliang4ward liangliang4ward deleted the fix/proxy-rewrite-method-withoutheaders branch February 16, 2022 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants