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

Compatibility with Jenkins clustered setup #288

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

Vlatombe
Copy link
Member

@Vlatombe Vlatombe commented Apr 3, 2024

OicSession is stored in HTTP session so must be serializable. As it is, OicSecurityRealm must also be Serializable since it defines a non-static inner class extending OicSession.

AuthorizationCodeFlow is not serializable, so instead of storing it directly in OicSession, it is now provided on every call from OicSecurityRealm (commenceLogin, finishLogin onSuccess).

Removed do prefix from OicSession#doCommenceLogin, OicSession#doFinishLogin to clarify that these methods are not being called by Stapler, but through direct java calls.

These changes allow to use this plugin as security realm when running a clustered Jenkins instance (where sessions are serialized) such as when using CloudBees CI with High Availability.

Testing done

Submitter checklist

Edit tasklist title
Beta Give feedback Tasklist Submitter checklist, more options

Delete tasklist

Delete tasklist block?
Are you sure? All relationships in this tasklist will be removed.
  1. Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
    Options
  2. Ensure that the pull request title represents the desired changelog entry
    Options
  3. Please describe what you did
    Options
  4. Link to relevant issues in GitHub or Jira
    Options
  5. Link to relevant pull requests, esp. upstream and downstream changes
    Options
  6. Ensure you have provided tests - that demonstrates feature works or fixes the issue
    Options

Copy link

codecov bot commented Apr 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.54%. Comparing base (22331f0) to head (04c8bf8).
Report is 7 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #288      +/-   ##
============================================
+ Coverage     71.90%   72.54%   +0.63%     
+ Complexity      194      187       -7     
============================================
  Files             9        9              
  Lines           744      743       -1     
  Branches        124      116       -8     
============================================
+ Hits            535      539       +4     
+ Misses          150      146       -4     
+ Partials         59       58       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@michael-doubez
Copy link
Contributor

michael-doubez commented Apr 3, 2024

That's more Jenkinsfu than I can comfortably process.
I'll take your word for it.

Where can I get more information about stapler and HA requierements ?

Copy link
Contributor

@michael-doubez michael-doubez left a comment

Choose a reason for hiding this comment

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

LGTM

@michael-doubez michael-doubez merged commit 0c3e868 into jenkinsci:master Apr 3, 2024
18 checks passed
@Vlatombe Vlatombe deleted the serializable branch April 3, 2024 12:19
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Similar to jenkinsci/jenkins#8557 and (more distantly) jenkinsci/stapler#493.

@@ -119,7 +120,8 @@
* @author Steve Arch
*/
@SuppressWarnings("deprecation")
public class OicSecurityRealm extends SecurityRealm {
public class OicSecurityRealm extends SecurityRealm implements Serializable {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps cleaner to make nested classes static?

@jglick
Copy link
Member

jglick commented Apr 4, 2024

information about stapler and HA requirements

I think the simplest summary is that in general https://jakarta.ee/specifications/platform/9/apidocs/jakarta/servlet/http/httpsession#setAttribute-java.lang.String-java.lang.Object- should be called with Serializable values if possible, in case the session is clustered somehow. OSS Jenkins does not in fact do that; https://docs.cloudbees.com/docs/cloudbees-ci/latest/ha/ha-fundamentals does. Docs

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.

None yet

3 participants