panxiaoquan commented on code in PR #13268:
URL: https://github.com/apache/apisix/pull/13268#discussion_r3146375243
##########
apisix/plugins/forward-auth.lua:
##########
@@ -184,14 +218,31 @@ function _M.access(conf, ctx)
return res.status, res.body
end
- -- set headers from the auth response, clearing any client-supplied values
- -- for configured headers not present in the auth response
+ local code, body = attach_consumer_by_header(conf, ctx, res)
+ if code or body then
+ return code, body
+ end
+
+ -- append headers that need to be get from the auth response header
for _, header in ipairs(conf.upstream_headers) do
local header_value = res.headers[header]
- -- if header_value is nil, the client header's value will be removed
if it exists
- core.request.set_header(ctx, header, header_value)
+ if header_value then
+ core.request.set_header(ctx, header, header_value)
+ end
end
end
+function _M.rewrite(conf, ctx)
+ return do_auth(conf, ctx)
Review Comment:
(a) The reason for moving `forward-auth` into `rewrite` is specifically
the `consumer_header` path.
For this feature, we do not just need to authenticate the request. We
need the auth result early enough to attach an APISIX Consumer and allow
Consumer / Consumer Group plugins to participate in the same request.
In APISIX, route-level `rewrite` runs first, then APISIX merges Consumer
plugins, and only after that does it enter `access` (`apisix/init.lua`).
If `forward-auth` stayed in `access`, it could still attach
`ctx.consumer`, but that would be too late for Consumer-scoped plugins to
be merged for the current request. Since the Consumer identity comes from
the auth response header, the auth HTTP call itself has to complete
before that merge point.
(b) With the current priorities, `forward-auth` runs before
`proxy-rewrite` in the `rewrite` phase (`forward-auth`: 2002,
`proxy-rewrite`: 1008). That means the auth service sees the original
client-facing request state, and `proxy-rewrite` only mutates the
request that goes to the upstream afterwards.
This does change the interaction with rewrite-phase mutations compared
with the old access-phase behavior.
(c) If another auth plugin on the same route also attaches a Consumer,
the later plugin wins. `consumer.attach_consumer()` overwrites
`ctx.consumer` rather than merging. With `key-auth` specifically,
`key-auth` runs first (priority 2500) and `forward-auth` runs later
(2002), so `forward-auth` would overwrite the Consumer set by `key-auth`
if `consumer_header` is configured.
So the current behavior is last-writer-wins when multiple auth plugins
attach a Consumer on the same route. If we keep this feature, we should
either document that limitation clearly or add an explicit guard for
conflicting Consumer attachment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
