Repository navigation
Conversation
Internal Token already tracks expires_at: Option<tokio::time::Instant>, and credentials::mds parses expires_in from the Metadata Server JSON response, but From<CacheableResource<Token>> for Result<AccessToken> previously discarded it. External token managers and watch channels (such as OpenTelemetry Collector auth extensions) need to know when a token expires or when the next refresh should be scheduled without maintaining a duplicate timer. Expose expires_at: Option<std::time::Instant> on AccessToken and populate it via tokio::time::Instant::into_std. Signed-off-by: Bartek Plotka <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds an expires_at field of type Option<std::time::Instant> to the AccessToken struct, updating its Debug implementation, conversion logic, and corresponding tests. While the implementation is solid, the documentation example for converting Instant across processes contains a potential panic risk if the expiration instant is in the past, and it introduces an external dependency on the time crate. It is recommended to use a safe, standard-library-only approach with std::time::SystemTime to prevent runtime panics.
| /// copy an expiration across processes, consider converting it to a | ||
| /// `time::OffsetDateTime` first: | ||
| /// | ||
| /// ``` | ||
| /// # let expires_at = Some(std::time::Instant::now()); | ||
| /// expires_at.map(|i| time::OffsetDateTime::now_utc() + (i - std::time::Instant::now())); | ||
| /// ``` |
There was a problem hiding this comment.
The example code has two issues:
- Panic Risk: In Rust, subtracting a larger
Instantfrom a smaller one (e.g.,i - Instant::now()wheniis in the past) will panic. Since tokens can expire,ican easily be in the past, making this example prone to runtime panics. - External Dependency: The example references
time::OffsetDateTimefrom the externaltimecrate, which may not be a dependency of the user's project. Usingstd::time::SystemTimefrom the standard library is more portable and self-contained.
Here is a safe, standard-library-only way to convert Instant to SystemTime without any panic risk:
/// copy an expiration across processes, consider converting it to a
/// std::time::SystemTime first:
///
/// ```
/// # let expires_at = Some(std::time::Instant::now());
/// let now_instant = std::time::Instant::now();
/// let now_system = std::time::SystemTime::now();
/// let expires_at_system = expires_at.map(|i| {
/// if i >= now_instant {
/// now_system + (i - now_instant)
/// } else {
/// now_system - (now_instant - i)
/// }
/// });
/// ```
| /// # let expires_at = Some(std::time::Instant::now()); | ||
| /// expires_at.map(|i| time::OffsetDateTime::now_utc() + (i - std::time::Instant::now())); | ||
| /// ``` | ||
| pub expires_at: Option<std::time::Instant>, |
There was a problem hiding this comment.
As you call out in the PR description, it is a breaking change to expose the field in this way. An accessor would be fine...
... but are you sure we need to expose it at all? That is complexity we try to hide from the consumers.
|
No longer necessary, thanks! |
Internal
Tokenalready tracksexpires_at: Option<tokio::time::Instant>, andcredentials::mdsparsesexpires_infrom the Metadata Server JSON response, butFrom<CacheableResource<Token>> for Result<AccessToken>previously discarded it.External token managers and watch channels (for example,
TokenProviderExtensioninopen-telemetry/otel-arrow) need to know when a token expires or when the next refresh should be scheduled without maintaining a separate hardcoded timer.This PR exposes
pub expires_at: Option<std::time::Instant>onAccessTokenand populates it viatokio::time::Instant::into_std.Note on semver:
AccessTokenwas introduced in #3642 without#[non_exhaustive](expires_atwas originally onAccessTokenin that PR and removed right before merge). Ifcargo semver-checksrequires avoiding a new public struct field onAccessTokenin 1.x, I am happy to expose apub fn expires_at(&self) -> Option<std::time::Instant>getter (or applyrust: no-semver-checksif preferred).Commits:
d1ed02292feat(auth): expose expires_at on AccessToken