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

ACM-15764: fix access token renewal from the metrics collector #1796

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

thibaultmg
Copy link
Contributor

@thibaultmg thibaultmg commented Jan 22, 2025

Introduces a new struct named TokenFile to automatically renew the token by re-reading the token file when it approaches the expiration date. The token is accessed by the RoundTripper injecting the bearer token calling the GetToken() method.

Copy link

openshift-ci bot commented Jan 22, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thibaultmg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Thibault Mange <[email protected]>
Signed-off-by: Thibault Mange <[email protected]>
@thibaultmg thibaultmg force-pushed the ACM-15764_renew_access_token branch from 8d5be3b to 48ba97a Compare January 22, 2025 17:07
Signed-off-by: Thibault Mange <[email protected]>
@thibaultmg
Copy link
Contributor Author

/cherry-pick release-2.12

@openshift-cherrypick-robot
Copy link
Collaborator

@thibaultmg: once the present PR merges, I will cherry-pick it on top of release-2.12 in a new PR and assign it to you.

In response to this:

/cherry-pick release-2.12

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Comment on lines +97 to +99
// It assumes that kubernetes renews the token when it reaches 80% of its lifetime. Most lifetimes are 1y or 1h.
// The strategy is to read the token file when we reach 85% of the remaining lifetime, and then every backoff interval
// when the remaining time is below 4 times the read backoff duration.
Copy link
Member

Choose a reason for hiding this comment

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

This feels a bit too complicated. How about renewing when you hit some constant margin like 5m instead?
I believe this is what we do for token-refresher https://github.com/observatorium/token-refresher/blob/master/main.go#L113

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By updating the token early, I assumed we are more robust to clock skews or potential zone aware time problems (even this this last one should not happen). Even if a bit more complicated, the described strategy is validated through unit tests.
But if you prefer the referenced strategy, it is fine for me. Let's wait and see if we have other opinions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants