feat: allow sending headers upstream returned by OPA server by Revolyssup · Pull Request #9710 · apache/apisix

Revolyssup · 2023-06-21T09:06:01Z

Description

Fixes #9704

Documentation to be updated

Checklist

I have explained the need for this PR and the problem it solves
I have explained the changes or the new features added to this PR
I have added tests corresponding to this change
I have updated the documentation to reflect this change
I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

Signed-off-by: revolyssup <[email protected]>

juststillthinking · 2023-06-26T06:31:58Z

Please make the ci pass and no test cases?

Signed-off-by: revolyssup <[email protected]>

Revolyssup · 2023-06-26T08:22:44Z

Please make the ci pass and no test cases?

Added test case @monkeyDluffy6017

Signed-off-by: revolyssup <[email protected]>

Revolyssup · 2023-06-30T05:32:40Z

@monkeyDluffy6017 PTAL

juststillthinking · 2023-06-30T17:54:48Z

+    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


Is it necessary to proxy all of opa's request headers?

What else do you propose? How to select which ones to proxy?

what HTTP header should pass upstream? instead of forwarding all HTTP header

Sn0rt · 2023-07-07T03:40:47Z

@chusov pls help us take a look

chusov · 2023-07-10T04:54:21Z

Maybe add a parameter with a list of headers?
send_header_upstream_list

Revolyssup · 2023-07-10T05:04:03Z

Maybe add a parameter with a list of headers? send_header_upstream_list

@monkeyDluffy6017 What do you think about this?

juststillthinking · 2023-07-11T01:36:56Z

@chusov i think the headers that need to be passed to upstream from OPA server should be fixed, what headers do you exactly need?

chusov · 2023-07-11T12:10:35Z

@monkeyDluffy6017
at the moment it is enough X-Username, in the future there were plans to add more depending on the service

juststillthinking · 2023-07-14T08:12:53Z

Maybe add a parameter with a list of headers? send_header_upstream_list

@monkeyDluffy6017 What do you think about this?

LGTM

Revolyssup · 2023-07-14T08:23:06Z

Maybe add a parameter with a list of headers? send_header_upstream_list

@monkeyDluffy6017 What do you think about this?

LGTM

Okay I'm making the change. @Sn0rt This one change remaining as suggested above by user

Signed-off-by: revolyssup <[email protected]>

juststillthinking · 2023-07-18T06:34:52Z

+        send_headers_upstream = {
+            type = "array",
+            minItems = 0,
+            items = {
+                type = "string"
+            },
+            description = "list of headers to pass to upstream in request"
+        },


Suggested change

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"

},

juststillthinking · 2023-07-18T06:35:16Z

+    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


Suggested change

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

The indent is not right, only four spaces are needed

Signed-off-by: revolyssup <[email protected]>

juststillthinking · 2023-07-18T07:26:25Z

+        for key in pairs(conf.send_headers_upstream) do
+            headersToSend[key] = true
+        end
+        for key,value in pairs(result.headers) do


Suggested change

for key,value in pairs(result.headers) do

for key, value in pairs(result.headers) do

juststillthinking · 2023-07-18T07:26:44Z

+            if headersToSend[key] then
+                core.request.set_header(ctx,key,value)
+            end
+


No blank is needed

juststillthinking · 2023-07-18T07:28:50Z

+        end
+        for key,value in pairs(result.headers) do
+            if headersToSend[key] then
+                core.request.set_header(ctx,key,value)


Suggested change

core.request.set_header(ctx,key,value)

core.request.set_header(ctx, key, value)

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

Makes more sense. Done

Signed-off-by: revolyssup <[email protected]>

Revolyssup added 2 commits June 21, 2023 14:35

feat: allow sending headers upstream returned by OPA server

8f27fc5

Signed-off-by: revolyssup <[email protected]>

fix lint fail

e2e72cb

Signed-off-by: revolyssup <[email protected]>

juststillthinking added the wait for update wait for the author's response in this issue/PR label Jun 26, 2023

add test cases

3fc50ae

Signed-off-by: revolyssup <[email protected]>

fix lint

47397d9

Signed-off-by: revolyssup <[email protected]>

lingsamuel removed the wait for update wait for the author's response in this issue/PR label Jun 26, 2023

lingsamuel previously approved these changes Jun 26, 2023

View reviewed changes

juststillthinking reviewed Jun 30, 2023

View reviewed changes

juststillthinking added the discuss label Jun 30, 2023

Sn0rt approved these changes Jul 14, 2023

View reviewed changes

pass selected headers to upstream

d91053c

Signed-off-by: revolyssup <[email protected]>

Revolyssup dismissed lingsamuel’s stale review via d91053c July 17, 2023 09:17

juststillthinking reviewed Jul 18, 2023

View reviewed changes

Comment thread apisix/plugins/opa.lua Outdated

Revolyssup added 2 commits July 18, 2023 10:56

change type of items in array

ba5e71b

Signed-off-by: revolyssup <[email protected]>

remove redundant line

418a29e

Signed-off-by: revolyssup <[email protected]>

Revolyssup mentioned this pull request Jul 18, 2023

feat: opa plugins, add the ability to set headers #9704

Closed

juststillthinking reviewed Jul 18, 2023

View reviewed changes

juststillthinking added wait for update wait for the author's response in this issue/PR and removed discuss labels Jul 18, 2023

add code suggestions

8aaf9c5

Signed-off-by: revolyssup <[email protected]>

juststillthinking reviewed Jul 18, 2023

View reviewed changes

Revolyssup added 3 commits July 18, 2023 13:10

refactor with code suggestions

46aff0d

Signed-off-by: revolyssup <[email protected]>

fix lint

a784d6a

Signed-off-by: revolyssup <[email protected]>

localize ipairs

662d321

Signed-off-by: revolyssup <[email protected]>

juststillthinking approved these changes Jul 18, 2023

View reviewed changes

juststillthinking added approved and removed wait for update wait for the author's response in this issue/PR labels Jul 18, 2023

leslie-tsang approved these changes Jul 19, 2023

View reviewed changes

juststillthinking merged commit 79b24cd into apache:master Jul 19, 2023

	for key,value in pairs(result.headers) do
	for key, value in pairs(result.headers) do

	core.request.set_header(ctx,key,value)
	core.request.set_header(ctx, key, value)

Uh oh!

Conversation

Revolyssup commented Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

juststillthinking commented Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Revolyssup commented Jun 26, 2023

Uh oh!

Revolyssup commented Jun 30, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sn0rt commented Jul 7, 2023

Uh oh!

chusov commented Jul 10, 2023

Uh oh!

Revolyssup commented Jul 10, 2023

Uh oh!

juststillthinking commented Jul 11, 2023

Uh oh!

chusov commented Jul 11, 2023

Uh oh!

juststillthinking commented Jul 14, 2023

Uh oh!

Revolyssup commented Jul 14, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juststillthinking Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Revolyssup Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Revolyssup commented Jun 21, 2023 •

edited

Loading

juststillthinking commented Jun 26, 2023 •

edited

Loading

juststillthinking Jul 18, 2023 •

edited

Loading

Revolyssup Jul 18, 2023 •

edited

Loading