-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
feat: allow sending headers upstream returned by OPA server #9710
Conversation
Signed-off-by: revolyssup <[email protected]>
Signed-off-by: revolyssup <[email protected]>
Please make the ci pass and no test cases? |
Signed-off-by: revolyssup <[email protected]>
Added test case @monkeyDluffy6017 |
Signed-off-by: revolyssup <[email protected]>
@monkeyDluffy6017 PTAL |
apisix/plugins/opa.lua
Outdated
else if conf.send_header_upstream and result.headers then | ||
for key,value in pairs(result.headers) do | ||
core.request.set_header(ctx,key,value) | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to proxy all of opa's request headers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What else do you propose? How to select which ones to proxy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what HTTP header should pass upstream? instead of forwarding all HTTP header
@chusov pls help us take a look |
Maybe add a parameter with a list of headers? |
@monkeyDluffy6017 What do you think about this? |
@chusov i think the headers that need to be passed to upstream from OPA server should be fixed, what headers do you exactly need? |
@monkeyDluffy6017 |
LGTM |
Okay I'm making the change. @Sn0rt This one change remaining as suggested above by user |
Signed-off-by: revolyssup <[email protected]>
Signed-off-by: revolyssup <[email protected]>
Signed-off-by: revolyssup <[email protected]>
apisix/plugins/opa.lua
Outdated
send_headers_upstream = { | ||
type = "array", | ||
minItems = 0, | ||
items = { | ||
type = "string" | ||
}, | ||
description = "list of headers to pass to upstream in request" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
send_headers_upstream = { | |
type = "array", | |
minItems = 0, | |
items = { | |
type = "string" | |
}, | |
description = "list of headers to pass to upstream in request" | |
}, | |
send_headers_upstream = { | |
type = "array", | |
minItems = 1, | |
items = { | |
type = "string" | |
}, | |
description = "list of headers to pass to upstream in request" | |
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
apisix/plugins/opa.lua
Outdated
else if result.headers and #conf.send_headers_upstream > 0 then | ||
local headersToSend = {} | ||
for key in pairs(conf.send_headers_upstream) do | ||
headersToSend[key] = true | ||
end | ||
for key,value in pairs(result.headers) do | ||
if headersToSend[key] then | ||
core.request.set_header(ctx,key,value) | ||
end | ||
|
||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else if result.headers and #conf.send_headers_upstream > 0 then | |
local headersToSend = {} | |
for key in pairs(conf.send_headers_upstream) do | |
headersToSend[key] = true | |
end | |
for key,value in pairs(result.headers) do | |
if headersToSend[key] then | |
core.request.set_header(ctx,key,value) | |
end | |
end | |
end | |
else if result.headers and conf.send_headers_upstream then | |
local headersToSend = {} | |
for key in pairs(conf.send_headers_upstream) do | |
headersToSend[key] = true | |
end | |
for key,value in pairs(result.headers) do | |
if headersToSend[key] then | |
core.request.set_header(ctx,key,value) | |
end | |
end | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indent is not right, only four spaces are needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Signed-off-by: revolyssup <[email protected]>
apisix/plugins/opa.lua
Outdated
for key in pairs(conf.send_headers_upstream) do | ||
headersToSend[key] = true | ||
end | ||
for key,value in pairs(result.headers) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for key,value in pairs(result.headers) do | |
for key, value in pairs(result.headers) do |
apisix/plugins/opa.lua
Outdated
if headersToSend[key] then | ||
core.request.set_header(ctx,key,value) | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No blank is needed
apisix/plugins/opa.lua
Outdated
end | ||
for key,value in pairs(result.headers) do | ||
if headersToSend[key] then | ||
core.request.set_header(ctx,key,value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
core.request.set_header(ctx,key,value) | |
core.request.set_header(ctx, key, value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could refactor this block like this:
for _, name in ipairs(conf.send_headers_upstream) do
if result.headers[name] then
core.request.set_header(ctx, name, value)
end
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes more sense. Done
Signed-off-by: revolyssup <[email protected]>
Signed-off-by: revolyssup <[email protected]>
Signed-off-by: revolyssup <[email protected]>
Description
Fixes #9704
Documentation to be updated
Checklist