nic-6443 commented on PR #13178:
URL: https://github.com/apache/apisix/pull/13178#issuecomment-4659033649
Thanks for picking this up — fixing `cookie.lifetime` is overdue. One
suggestion on the schema direction before it lands.
Instead of keeping the nested `session.cookie` object with
`name`/`path`/`lifetime` aliases mapped onto lua-resty-session keys, I'd lean
toward exposing the 4.x options **flat, under their real names** —
`cookie_name`, `cookie_path`, `cookie_domain`, `cookie_secure`,
`cookie_http_only`, `cookie_same_site`, `idling_timeout`, `rolling_timeout`,
`absolute_timeout`, etc. That drops the alias layer and `build_session_opts()`
entirely: the keys are identical to what `resty.session.start()` already
consumes, so they pass straight through with no mapping and no alias-precedence
rules to reason about. The only thing left to translate is the legacy
`session.cookie.lifetime` — we keep accepting it (deprecated) and map it to
`absolute_timeout` so existing configs keep working, even though, as you noted,
it's been silently ignored since the 4.x bump in #12862.
I'd also drop the `additionalProperties` pass-through and enumerate the
supported options explicitly, the way Kong's session and OIDC plugins do. A few
reasons:
- Silent pass-through is exactly how `cookie.lifetime` broke in the first
place: a key the bundled lua-resty-session version doesn't recognize is happily
accepted by the schema and then quietly dropped at runtime. An explicit
allowlist turns that into a validation error instead of a silent no-op.
- lua-resty-session is a pinned dependency, not a fast-moving external API.
APISIX already enumerates stable-library config explicitly — see the `redis`
block in this very plugin, or `limit-count`'s redis schema.
`additionalProperties = true` is reserved for volatile surfaces (AI provider
options, protobuf decoder flags, OTel attributes), which this isn't.
- Explicit fields are documentable and discoverable; with pass-through,
users have to read the lua-resty-session source for the exact pinned version to
know what's actually valid.
The tradeoff is that adding a new option later is a one-line schema addition
— but that's cheap, and it keeps the surface honest about what the bundled
library actually supports.
One more data point that reinforces this: the same #12862 upgrade left
`authz-casdoor` broken in the same way. It still builds its session with
`session.new{ cookie = {lifetime = ...} }` in
`apisix/plugins/authz-casdoor.lua`, so the post-login session ignores the
token's `expires_in` and falls back to the 4.x default `absolute_timeout`.
Probably worth fixing in the same sweep, or at least tracking as a separate
issue.
Happy to send a PR with the flat-schema version if this direction sounds
good. What do you think?
--
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]
