-
Notifications
You must be signed in to change notification settings - Fork 145
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
Fix token caching for multi-cluster multi-namespace environments #261
base: master
Are you sure you want to change the base?
Conversation
@jglick this has been open for more a year, and the plugin is basically broken and unusable. I am not 100% sure but I think it will affect in-cluster JWT auth too for when jenkins is in k8s. Will this ever be merged? How is anyone using it, does everybody using static tokens and are happy? |
I am not a maintainer of this plugin (and do not even know much about it) so I am not sure why you are mentioning me. |
@jglick sorry, I looked at the last commit author and since you are everywhere I just assumed you might be a right person to tag. cc @jetersen please see above. This is a critical issue, and it renders this plugin unusable for anyone having more than one vault cluster in the environment. I have been running a custom build with my fix in production large scale environment for a year now and there are no issues. |
} | ||
} | ||
|
||
private ConcurrentMap<CacheKey, TokenHolder> cache = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of ConcurrentMap we might as well pull in caffeine.
<dependency>
<groupId>io.jenkins.plugins</groupId>
<artifactId>caffeine-api</artifactId>
</dependency>
See #260.
New implementation uses caching criteria to differentiate tokens for different environments. So far I only came up with two possible attributes - server address and a namespace, but I have created
CacheKey
to allow for easy extension later on if more colliding attributes will be discovered. I have also addedtransient
where I think it applies since I do not think we want to cache be ever written to the disk. I also added a little thread safety, although I am not sure if it is necessary (and I did not observe any issues with it so far) but it felt like it is.