Skip to content
This repository has been archived by the owner on Dec 5, 2022. It is now read-only.

Add note about async initialisation of tokens in K8S #203

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DestyNova
Copy link

When used in Kubernetes, I observed a persistent failure with an error message like this:

org.zalando.stups.tokens.AccessTokenUnavailableException: No token available for
tokenId 'someToken'. Tokens are available for the following tokenIds [someToken]

This has the look of a race condition about it. After I raised the issue on an internal Zalando chat channel, Alexey Ermakov provided a helpful explanation:

unfortunately the tokens library initialises asynchronously by default on Kubernetes (but synchronously on STUPS, even though it's much more expensive), so the first couple of calls will fail.

The PR adds an example to the README with a single line of code shared by Alexey which forces the filesystem refresher to initialise and validate the tokens immediately on startup, which avoids this issue.

With thanks to Alexey Ermakov. Full details will be provided in the PR description because I'm not sure Markdown works in the commit description.
@codecov-commenter
Copy link

Codecov Report

Merging #203 into master will increase coverage by 0.23%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #203      +/-   ##
============================================
+ Coverage     72.55%   72.79%   +0.23%     
  Complexity      245      245              
============================================
  Files            48       48              
  Lines           849      849              
  Branches         40       40              
============================================
+ Hits            616      618       +2     
+ Misses          209      207       -2     
  Partials         24       24              
Impacted Files Coverage Δ Complexity Δ
.../org/zalando/stups/tokens/fs/FilesystemReader.java 100.00% <0.00%> (+10.52%) 4.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f362498...0233d64. Read the comment docs.

@DestyNova
Copy link
Author

Not sure how adding a little to the README could increase code coverage, but I'm not gonna argue 😆

@fatroom
Copy link
Member

fatroom commented Jun 5, 2020

👍

1 similar comment
@vadeg
Copy link
Contributor

vadeg commented Feb 19, 2021

👍

@DestyNova
Copy link
Author

@szuecs can you merge this, or is it no longer needed?

@szuecs
Copy link
Member

szuecs commented Feb 27, 2021

👍

@codecov-io
Copy link

codecov-io commented Feb 27, 2021

Codecov Report

Merging #203 (0233d64) into master (f362498) will increase coverage by 0.23%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #203      +/-   ##
============================================
+ Coverage     72.55%   72.79%   +0.23%     
  Complexity      245      245              
============================================
  Files            48       48              
  Lines           849      849              
  Branches         40       40              
============================================
+ Hits            616      618       +2     
+ Misses          209      207       -2     
  Partials         24       24              
Impacted Files Coverage Δ Complexity Δ
.../org/zalando/stups/tokens/fs/FilesystemReader.java 100.00% <0.00%> (+10.52%) 4.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f362498...0233d64. Read the comment docs.

@tkrop
Copy link
Member

tkrop commented Dec 13, 2021

👍

@szuecs
Copy link
Member

szuecs commented Dec 13, 2021

@vadeg should be able to merge

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

Successfully merging this pull request may close these issues.

7 participants