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

[JENKINS-72268] Missing permission due to desync with cache #256

Merged

Conversation

Wadeck
Copy link
Contributor

@Wadeck Wadeck commented Nov 1, 2023

Should resolves JENKINS-72268 and potentially JENKINS-72209.

The gh variable is set for a token only when the cache does not contain the username (code). But, if you are using impersonation, like in situations explained in the ticket, the cache will contain the token and thus, me != null, leading to a token with gh=null.
Due to this, the authorities are not loaded because the gh is null.

Testing done

I tested with and without the user in my local storage (same behavior). The impersonation did not work before and now is working as expected.

The second commit is about a cast exception being thrown when trying to request the group information without directly using the expected token. Better to return the expected exception instead. Tested before and after, the behavior is "cleaner".

Due to the number of manual tests I did using breakpoints here and there to reproduce the problem and understand the source, I am not sure I can write a unit test without spending 4-5x the time.

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. Ensure you have provided tests - that demonstrates feature works or fixes the issue
    Options

In case of impersonation the gh variable and the usersByTokenCache could be de-sync, leading to token not able to connect.

Could be related to JENKINS-72209 as well.
@Wadeck Wadeck requested a review from a team as a code owner November 1, 2023 15:13
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Thanks very much for the commit and for the detailed description of the testing you performed!

@MarkEWaite MarkEWaite merged commit 0646c4a into jenkinsci:master Nov 1, 2023
16 checks passed
@MarkEWaite MarkEWaite added the bug label Nov 1, 2023
@lprimak
Copy link

lprimak commented Nov 3, 2023

Hi, @Wadeck and @MarkEWaite

I believe this change introduced a huge performance regression. When installing 596.v0646c4a_0a_962 I have seen 20x performance degradation for every GH-based build pipeline.

Can you please take a look and fix / revert this please? Thank you.

@@ -504,6 +504,9 @@ private GHMyself loadMyself(@NonNull String token) throws IOException {
// Also stick into usersByIdCache (to have latest copy)
String username = ghMyself.getLogin();
usersByIdCache.put(username, new GithubUser(ghMyself));
} else {
// force creation of the gh variable, esp. in case of impersonation
getGitHub();
Copy link

Choose a reason for hiding this comment

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

I think this is the cause of the performance degradation

Copy link
Member

Choose a reason for hiding this comment

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

@lprimak can you describe more about your environment and usage?

  • In what ways to you use auth? Just oauth web or GitHub personal access tokens, too?
  • How are your jobs organized? Are they in folders or grouped into views? What columns do you have enabled on the landing page if Jenkins for the All view?
  • Where were performance issues most noticeable?
  • How many users?
  • How many jobs and can you break down numbers by job type?

Hopefully, with enough info, it can be reproduced and fixed upon reproduction. Any answers you provide can help us figure out a performance profile and attempt to emulate your environment.

Copy link
Member

Choose a reason for hiding this comment

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

Also if you could describe how you evaluated the 20x performance reduction it will give us hints of where to look as a root cause. It's not always just the contributed area that could be the root cause, it's possible an unrelated area of code could trigger the conditions for this performance issue. So it would give us a hint for evaluating other areas if we understand exactly how you experienced the degradation.

Copy link

@lprimak lprimak Nov 4, 2023

Choose a reason for hiding this comment

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

In what ways to you use auth? Just oauth web or GitHub personal access tokens, too?

I use auth to sign in to jenkins with GH credentials, I believe that's all:
Here is my oath configuration (config-as-code)

jenkins:
  securityRealm:
    github:
      clientID: "XXX"
      clientSecret: "XXX"
      githubApiUri: "https://api.github.com"
      githubWebUri: "https://github.com"
      oauthScopes: "read:org,user:email,repo"

How are your jobs organized? Are they in folders or grouped into views?

There are less than 10 jobs. No folders

Where were performance issues most noticeable?

Running any declarative pipeline. It's slow as molasses

How many users?

Just me

Also if you could describe how you evaluated the 20x

Pipelines that used to take 1 minute now take 15 minutes :)

Thanks for your help

@lprimak
Copy link

lprimak commented Nov 3, 2023

I created a JIRA issue: https://issues.jenkins.io/browse/JENKINS-72276

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