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

feat: adding new token cache #169

Merged
merged 10 commits into from
Aug 15, 2023
Merged

Conversation

akouznetchik
Copy link
Contributor

@akouznetchik akouznetchik commented Apr 19, 2023

  • Added SSO token auth caching, can be enabled via web UI. This idea is originally from Adding caching for crowd tokens. #34 and Fix cache, add token auth caching #162 but modified to work. By default, the authentication-filter is mapped to /* therefore all requests including for /static where the header contains an SSO token are validated. This can significantly degrade page loading times depending on latency and Crowd server load. In our testing, after clearing the content cache, enabling this feature improved performance by a factor of 10-20x. The token validation cache will remain valid for the duration of the specified cache TTL, as is the case with other caches.

@DuMaM DuMaM added documentation Documentation updates feature New features and improvements and removed documentation Documentation updates labels Apr 19, 2023
@DuMaM DuMaM changed the title Add token cache feat: adding new token cache Apr 19, 2023
@DuMaM DuMaM added the documentation Documentation updates label Apr 19, 2023
@DuMaM
Copy link
Contributor

DuMaM commented Apr 19, 2023

I will release new version with fix only.
Then after 2 weeks I will do the same with this PR

@DuMaM DuMaM force-pushed the add-token-cache branch 2 times, most recently from 7e31abe to 4cb7a17 Compare April 22, 2023 10:50
@DuMaM DuMaM self-assigned this Apr 25, 2023
@DuMaM
Copy link
Contributor

DuMaM commented May 5, 2023

In this weekend i will merge this if everything works as expected.

@DuMaM
Copy link
Contributor

DuMaM commented May 13, 2023

@akouznetchik Could you make it work? This checkbox is not rendering.

@DuMaM DuMaM closed this May 13, 2023
@DuMaM DuMaM reopened this May 13, 2023
@akouznetchik
Copy link
Contributor Author

@akouznetchik Could you make it work? This checkbox is not rendering.

Seems to work for me. Try an mvn clean first. Also the cache TTL is in seconds. Do you still wanna do checkbox per cache type?

@DuMaM
Copy link
Contributor

DuMaM commented May 14, 2023

Seems to work for me. Try an mvn clean first.

This is a bit confusing to me. I'm doing a clean build every time with docker image.
Could you try to start this with ./_start.sh? Could you please tell me what's the results?

Also the cache TTL is in seconds.

This should be updated in separated PR.

Do you still wanna do checkbox per cache type?

Yes, but maybe in feature it's not mandatory now. I misunderstand your change because it was not rendering for me.

@DuMaM
Copy link
Contributor

DuMaM commented May 19, 2023

@akouznetchik ? :)

@akouznetchik
Copy link
Contributor Author

akouznetchik commented May 19, 2023

@akouznetchik ? :)

Hey man, I’m just on vacation and didn’t grab my laptop, so I won’t be able to give docker a go. I was testing using the Jenkins maven hpi plugin, hpi run, because I have my own crowd server to test against and didn’t need the docker image. I will give this a try next weekend.

@akouznetchik
Copy link
Contributor Author

@DuMaM ok so this pull request is fine. The reason you do not see the changes is because our current build script does not override anything that was already deployed via /usr/share/jenkins/ref/plugins/

See: https://github.com/jenkinsci/docker/blob/master/README.md#installing-custom-plugins

We can fix this by copying the plugin HPI file with .override extension.

+++ w/casc/Dockerfile
@@ -22,4 +22,4 @@ RUN     jenkins-plugin-cli --plugins configuration-as-code \
                                      monitoring \
                                      jaxb
 
-COPY    --chown=jenkins:jenkins target/crowd2.hpi /usr/share/jenkins/ref/plugins/
+COPY    --chown=jenkins:jenkins target/crowd2.hpi /usr/share/jenkins/ref/plugins/crowd2.hpi.override

I will add a separate PR for this. Essentially our latest plugin was being copied from the target folder to the staging area, but never re-installed as a previous version was already deployed.

You can verify this if you connect to the docker container and do

ls -lah /var/jenkins_home/plugins/crowd2.hpi
ls -lah /usr/share/jenkins/ref/plugins/crowd2.hpi

They will differ.

@DuMaM
Copy link
Contributor

DuMaM commented Jul 4, 2023

@akouznetchik #169 (comment)
Can you prepare this pr? 😉

To be fair im waiting for it to start test it

@DuMaM
Copy link
Contributor

DuMaM commented Jul 4, 2023

i would be grateful if you can hop in and help me maintain this plugin

@akouznetchik
Copy link
Contributor Author

i would be grateful if you can hop in and help me maintain this plugin

Would be my pleasure.

@akouznetchik
Copy link
Contributor Author

@DuMaM okay so I opened two PRs, test them out, then let's merge them, and rebase this. Then we need to remove the additional null check that I added, as you see you already re-added it to master. Anything else?

@DuMaM
Copy link
Contributor

DuMaM commented Jul 20, 2023

i would be grateful if you can hop in and help me maintain this plugin

Would be my pleasure.

Please post request on https://github.com/jenkins-infra/helpdesk
I will approve it :)

@DuMaM
Copy link
Contributor

DuMaM commented Jul 20, 2023

@DuMaM okay so I opened two PRs, test them out, then let's merge them, and rebase this. Then we need to remove the additional null check that I added, as you see you already re-added it to master. Anything else?

Please rebase this now ;)

@akouznetchik akouznetchik force-pushed the add-token-cache branch 2 times, most recently from 47688e2 to 0ff9fbd Compare July 21, 2023 04:19
@DuMaM
Copy link
Contributor

DuMaM commented Jul 21, 2023

Why you closed this PR?

@akouznetchik
Copy link
Contributor Author

akouznetchik commented Jul 21, 2023

Why you closed this PR?

Mistake, re-opened

@akouznetchik akouznetchik reopened this Jul 21, 2023
@akouznetchik
Copy link
Contributor Author

akouznetchik commented Jul 24, 2023

@DuMaM Let me know when you have done your testing. Also there are 'Changes requested' pending, but all seem resolved, please double check.

@DuMaM
Copy link
Contributor

DuMaM commented Aug 13, 2023

lgtm 😉
did you tested the plugin update scenario?

@DuMaM
Copy link
Contributor

DuMaM commented Aug 13, 2023

@akouznetchik We need to check if this is a potential breaking change.
This will introduce a new serialization structure which may break configs stored on the file system.

@akouznetchik
Copy link
Contributor Author

We need to check if this is a potential breaking change. This will introduce a new serialization structure which may break configs stored on the file system.

Just tested this by upgrading via UI on a Jenkins master that had production plugin v4.0.0 without <useTokenCache>true</useTokenCache> node in config.xml. After upgrade it was added with false value.

I also downgraded from my 4.0.1-SNAPSHOT down to 4.0.0 and the extra node was removed without issue.

Looks OK to me.

@DuMaM DuMaM merged commit 0a2947d into jenkinsci:master Aug 15, 2023
19 checks passed
@DuMaM
Copy link
Contributor

DuMaM commented Aug 15, 2023

Tomorrow I will test this out on my prod env and if everything works, then I will perform release

@DuMaM
Copy link
Contributor

DuMaM commented Aug 15, 2023

@akouznetchik could you please take a look on this PR
#25
?

@akouznetchik
Copy link
Contributor Author

@DuMaM can we release this?

@DuMaM
Copy link
Contributor

DuMaM commented Sep 27, 2023

Yes we can.

@DuMaM
Copy link
Contributor

DuMaM commented Sep 27, 2023

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Documentation updates feature New features and improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants