Take expiry timestamp into account in stream connections #10299
Conversation
acogoluegnes
force-pushed
the
token-expiration-in-stream-connections
branch
5 times, most recently
from
ea2c944 to
301aeed
Compare
acogoluegnes
force-pushed
the
token-expiration-in-stream-connections
branch
3 times, most recently
from
32f9db8 to
add0eaa
Compare
acogoluegnes
changed the title
acogoluegnes
force-pushed
the
token-expiration-in-stream-connections
branch
from
add0eaa to
c69dbd8
Compare
| TimerRef = | ||
| maybe | ||
| rabbit_log:debug("Checking token expiry"), | ||
| true ?= rabbit_access_control:permission_cache_can_expire(User), |
There was a problem hiding this comment.
I wonder whether we can remove function rabbit_access_control:permission_cache_can_expire/1 in future? This function seems to be unnecessary given that we now have rabbit_access_control:expiry_timestamp/1 which will return never if the permission cache cannot expire. Ideally rabbit_channel will use the same new approach as done by Stream connections in this PR. This change doesn't have to be part of this PR though. However, ideally we don't add new calls to rabbit_access_control:permission_cache_can_expire/1 such that we can remove that funtion more easily in the future. WDYT?
There was a problem hiding this comment.
Sounds reasonable. Do we try to remove rabbit_access_control:permission_cache_can_expire/1 in this PR? The sooner, the better.
There was a problem hiding this comment.
It's up to you :) I also think, the sooner the better, yes.
| undefined -> | ||
| ok; | ||
| _ -> | ||
| timer:cancel(Timer) |
There was a problem hiding this comment.
Maybe we can use the newer erlang:cancel_timer/2 API instead.
There was a problem hiding this comment.
I ended up using the timer-related functions in erlang instead of timer. I used {async, false} in erlang:cancel_timer/2 to be able to test the cancellation. Asynchronous would be better, but I don't think it makes much of a difference. WDYT?
There was a problem hiding this comment.
Thanks, that's perfect, synchronous cancellation (ie. {async, false}) is good enough in this rarely executed code path.
acogoluegnes
force-pushed
the
token-expiration-in-stream-connections
branch
2 times, most recently
from
efe3d83 to
f3c029a
Compare
Backends return 'never' or the timestamp of the expiry time of the credentials. Only the OAuth2 backend returns a timestamp, other RabbitMQ authz backends return 'never'. Client code uses rabbit_access_control, so it contains now a new expiry_timestamp/1 function that returns the earliest expiry time of the underlying backends. Fixes #10298
Re-evaluate permissions, cancel publishers and subscriptions, send metadata update accordingly. Move record definitions from the stream reader to a dedicated header file to be able to write unit tests. Fixes #10292
Stream write access is already checked when creating the publisher. Fixes #10371
acogoluegnes
force-pushed
the
token-expiration-in-stream-connections
branch
from
f3c029a to
6fc3187
Compare
PR for: