apisix
apisix copied to clipboard
fix: Set the session configuration
Description
Added the configuration items related to resty.session. You can set the behavior and expiration time of the session through the configuration items to avoid the problem that the access token does not expire but the session expires when the expiration time of the access token is longer than the session.
Fixes # (issue) https://github.com/apache/apisix/issues/10797
Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [x] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)
Hello, I think we may need additional testing, and documentation to ensure it is available in the plugin
Hello, I think we may need additional testing, and documentation to ensure it is available in the plugin Sure @smileby @shreemaan-abhishek I have added test code and documentation, and successfully executed it on my local machine. However, I have a question. I am using a Docker setup of Keycloak/etcd on my local machine to validate the test code, referencing test code files from other contributors that use the same realm and client. I am unsure if this approach is standard. Does the official provide standard testing support services? For example, does the official provide a designated Keycloak authentication service for testing purposes?
@illidan33 you can find the keycloak service defined here:
https://github.com/apache/apisix/blob/21d9ceeac9715dcdad37335fe402e325ee74a43d/ci/pod/docker-compose.plugin.yml#L40C1-L59C91
@illidan33 you can find the keycloak service defined here:
https://github.com/apache/apisix/blob/21d9ceeac9715dcdad37335fe402e325ee74a43d/ci/pod/docker-compose.plugin.yml#L40C1-L59C91
@shreemaan-abhishek hi, I updated the configuration to keep it the same as the official one.
@shreemaan-abhishek @monkeyDluffy6017 hi, could you help to check the ci's error. It doesn't look like it failed because of the test code I submitted
The quality of the english doc is subpar and I cannot understand what it's saying. Please hold the review to a higher standard.
My questions:
This can be configured with Nginx set $session_cookie_lifetime 3600: does this mean this new configuration option just makes it feasible to configuresession_cookie_lifetimedynamically at runtime?- The next sentence is grammatically incorrect and I cannot understand, @shreemaan-abhishek please rephrase it makes sense to you.
it is used if the cookies are configured persistent with session.cookie.persistent == true: where is this configuration? it is not in this plugin.- The Chinese doc also says
for more information see ssl_session_timeout. Where is thisssl_session_timeout
@shreemaan-abhishek could you create a pr to change what @kayx23 mentioned?
The quality of the english doc is subpar and I cannot understand what it's saying. Please hold the review to a higher standard.
My questions:
This can be configured with Nginx set $session_cookie_lifetime 3600: does this mean this new configuration option just makes it feasible to configuresession_cookie_lifetimedynamically at runtime?- The next sentence is grammatically incorrect and I cannot understand, @shreemaan-abhishek please rephrase it makes sense to you.
it is used if the cookies are configured persistent with session.cookie.persistent == true: where is this configuration? it is not in this plugin.- The Chinese doc also says
for more information see ssl_session_timeout. Where is thisssl_session_timeout
hi @kayx23 @monkeyDluffy6017 @shreemaan-abhishek , the documentation is sourced from https://github.com/bungle/lua-resty-session/tree/v3.10, where it describes the session.cookie.lifetime. This update only adds the configuration for lifetime, and the description should only focus on the role of lifetime. I have modified the documentation. Do I need to submit another PR?
@illidan33 yeah, of course, another pr is needed, and i will invite @kayx23 to review
@illidan33 yeah, of course, another pr is needed, and i will invite @kayx23 to review
hi @monkeyDluffy6017 @kayx23 , the new pr's link is 10994
