Skip to content

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

Merged
spacewander merged 8 commits into
apache:masterfrom
liangliang4ward:fix/proxy-rewrite-method-withoutheaders
Feb 15, 2022
Merged

fix(proxy-rewrite): when conf.headers are missing,conf.method can make effect#6300
spacewander merged 8 commits into
apache:masterfrom
liangliang4ward:fix/proxy-rewrite-method-withoutheaders

Conversation

@liangliang4ward

@liangliang4ward liangliang4ward commented Feb 11, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

tokers commented Feb 11, 2022

Copy link
Copy Markdown
Contributor

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

spacewander
spacewander reviewed Feb 13, 2022
View reviewed changes

@spacewander spacewander left a comment

Copy link
Copy Markdown
Member

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

apisix/t/plugin/proxy-rewrite3.t

Lines 87 to 129 in ebe91aa

=== 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

liangliang4ward commented Feb 14, 2022 •
edited
Loading

Copy link
Copy Markdown
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

apisix/t/plugin/proxy-rewrite3.t

Lines 87 to 129 in ebe91aa

=== 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

spacewander commented Feb 14, 2022

Copy link
Copy Markdown
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

apisix/t/plugin/proxy-rewrite3.t

Lines 87 to 129 in ebe91aa

=== 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

liangliang4ward commented Feb 14, 2022

Copy link
Copy Markdown
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

apisix/t/plugin/proxy-rewrite3.t

Lines 87 to 129 in ebe91aa

=== 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

liangliang4ward commented Feb 15, 2022

Copy link
Copy Markdown
Contributor Author

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

add a test case

spacewander
spacewander reviewed Feb 15, 2022
View reviewed changes
Comment thread t/plugin/proxy-rewrite3.t Outdated
Comment thread t/plugin/proxy-rewrite3.t Outdated
tokers
tokers reviewed Feb 15, 2022
View reviewed changes
Comment thread t/plugin/proxy-rewrite3.t
@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

spacewander commented Feb 15, 2022

Copy link
Copy Markdown
Member

Merged. Thanks!

@liangliang4ward
liangliang4ward deleted the fix/proxy-rewrite-method-withoutheaders branch February 16, 2022 01:06
@soulbird soulbird mentioned this pull request Jun 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@spacewander spacewander spacewander approved these changes
@tokers tokers tokers left review comments
@tzssangglass tzssangglass tzssangglass approved these changes

Assignees

No one assigned

Labels

need backport

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@liangliang4ward @tokers @spacewander @tzssangglass