Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce Token expiry delta and add get_token retries #73

Merged
merged 4 commits into from
Jun 6, 2023

Conversation

valkum
Copy link
Contributor

@valkum valkum commented Jun 5, 2023

This tackles some smaller issues.

First, other, official, SDKs use smaller values than 30s. The Python SDK uses 20s, and the Go SDK uses 0s for credentials from the metadata server.
This PR reduces the delta to 20s, following the Python SDK.

Second, the official SDKs use a retry mechanism for fetching tokens from the various backends.
This PR adds a retry of at most 5 requests for each fetch token request.

This also bumps the version to 0.8.1. The changes above are internal only and thus are semver compatible for a patch release.

The Python SDK states that the delta should be capped at 30s and the Go
SDK uses 0s.
Copy link
Owner

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please apply suggestions across all three implementations of the retry loops. ;)

src/custom_service_account.rs Outdated Show resolved Hide resolved
src/custom_service_account.rs Outdated Show resolved Hide resolved
tracing::debug!("requesting token from service account: {:?}", request);
let res = client.request(request).await;
match res {
Ok(res) => break Ok(res),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're using res both for result and response here. Would be nice to disambiguate (I like using rsp for response, but just response might work, too).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided for response to be more explicit because it's returned by the loop and might not be immediately clear to the reader.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do the same inside the match here?

(Also -- sorry for noticing it only now -- given the break Ok() perhaps we can deindent the loop tail by one step out of the match?)

valkum added 3 commits June 6, 2023 16:33
The official SDKs implement a retry mechanism for fetching the tokens
from the metadata server in the case of I/O errors, etc.
This adds a similar mechanism to the provided ServiceAccount
implementations
This release includes a retry mechanism for fetching tokens for service
accounts, and reduces the expiry delta for tokens.
Copy link
Owner

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, thanks!

@djc djc merged commit 8ecfee5 into djc:master Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants