apache / iceberg-rust

Apache Iceberg
https://rust.iceberg.apache.org/
Apache License 2.0
619 stars 139 forks source link

discussion: token refresh mechanism for rest client #437

Open TennyZhuang opened 3 months ago

TennyZhuang commented 3 months ago

Background: #301

The token fetched from the token server may have a TTL, see TokenResponse::expires_in. In most implementations, the value is about several hours. Our catalog client is a long-lived object, which means that we should handle the token expiration event.

There are two ways:

  1. Spawn a background task, and setup a ticker with a specified time interval, e.g. expires_in * 0.9 seconds, and refetch the token when triggered.
    • Pros: Easy to implement
    • Cons: Must trust the local timer skew
    • Cons: Must introduce a timer, which means depending on a specified async runtime
  2. Call every methods with a retry wrapper. When meeting an unauthorized error, refetch the token and retry the method.
    • Pros: Consistent with iceberg-python
    • Pros: Does not rely on local clock and specific runtime
    • Cons: When expired, thousands of concurrent requests may fail, and then all of them will trigger a token refetch, which is not ideal.
    • This can be workaround by some concurrency control, to force only one request will refetch the token and others must wait for the result, but it introduced complexity.
Xuanwo commented 3 months ago

2. Call every methods with a retry wrapper. When meeting an unauthorized error, refetch the token and retry the method.

Our rest catalog client now has a authenticate function.

https://github.com/apache/iceberg-rust/blob/0937f19d4c4b4ea14d423e6c78731904b4c9012e/crates/catalog/rest/src/client.rs#L88

We can perform the expiration logic here.

liurenjie1024 commented 3 months ago

I also prefer 2. The iceberg community may deprecate current auth approach in future, and introduce true oauth client in future. I prefer to have a simpler solution without introducing too much extra dependency.

TennyZhuang commented 3 months ago
  1. Store (expired_at, token) pair in an async Mutex, and check whether it's close to expiration when needed. If it's close to expiration, lock the mutex, update the pair, and unlock. Then only one request will update the token.
Xuanwo commented 3 months ago

3. Store (expired_at, token) pair in an async Mutex, and check whether it's close to expiration when needed. If it's close to expiration, lock the mutex, update the pair, and unlock. Then only one request will update the token.

Great!

liurenjie1024 commented 3 months ago
  1. Store (expired_at, token) pair in an async Mutex, and check whether it's close to expiration when needed. If it's close to expiration, lock the mutex, update the pair, and unlock. Then only one request will update the token.

Sounds reasonable to me.

TennyZhuang commented 3 months ago

Then we need https://docs.rs/tokio/latest/tokio/sync/struct.RwLock.html, still depends on tokio :(

This is rust

liurenjie1024 commented 3 months ago

Then we need https://docs.rs/tokio/latest/tokio/sync/struct.RwLock.html, still depends on tokio :(

This is rust

How about futures Mutex?

Xuanwo commented 3 months ago

The current implementation utilizes std::sync::Mutex with a precisely controlled locking scope.

https://github.com/apache/iceberg-rust/blob/85627083f0791629be3d73413af936165ebd38eb/crates/catalog/rest/src/client.rs#L88-L179

The worst thing that could happen here is that we might send multiple token refresh requests, which I think is fine for a catalog.

TennyZhuang commented 3 months ago

It's acceptable, but not as expected. When a client request a token, the server may expect it will be used for several hours.