Skip to content

[JENKINS-73474] Fix GitHubAppCredentials owner inference #803

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.

Sign up for GitHub

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

Jump to bottom
Merged
dwnusbaum merged 1 commit into from
Aug 20, 2024
Merged

[JENKINS-73474] Fix GitHubAppCredentials owner inference #803

dwnusbaum merged 1 commit into from
Aug 20, 2024

Conversation

@jeromepochat
Copy link
Contributor

@jeromepochat jeromepochat commented Aug 20, 2024 •
edited
Loading

GitHub App installed in multiple organizations get:

java.lang.IllegalArgumentException: Found multiple installations for GitHub app ID ... but none match credential owner "". Set the right owner in the credential advanced options to one of: ..., ...
	at PluginClassLoader for github-branch-source//org.jenkinsci.plugins.github_branch_source.GitHubAppCredentials.generateAppInstallationToken(GitHubAppCredentials.java:244)
	at PluginClassLoader for github-branch-source//org.jenkinsci.plugins.github_branch_source.GitHubAppCredentials.getToken(GitHubAppCredentials.java:295)
	at PluginClassLoader for github-branch-source//org.jenkinsci.plugins.github_branch_source.GitHubAppCredentials.getPassword(GitHubAppCredentials.java:324)
	at PluginClassLoader for git-client//org.jenkinsci.plugins.gitclient.CliGitAPIImpl.createPasswordFile(CliGitAPIImpl.java:2547)
	at PluginClassLoader for git-client//org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandWithCredentials(CliGitAPIImpl.java:2143)
	at PluginClassLoader for git-client//org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandWithCredentials(CliGitAPIImpl.java:2079)
	at PluginClassLoader for git-client//org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandWithCredentials(CliGitAPIImpl.java:2070)
	at PluginClassLoader for git-client//org.jenkinsci.plugins.gitclient.CliGitAPIImpl.getRemoteSymbolicReferences(CliGitAPIImpl.java:3842)
	at PluginClassLoader for git//jenkins.plugins.git.AbstractGitSCMSource.retrieveActions(AbstractGitSCMSource.java:1176)
	at PluginClassLoader for scm-api//jenkins.scm.api.SCMSource.fetchActions(SCMSource.java:847)
	at PluginClassLoader for branch-api//jenkins.branch.MultiBranchProject.computeChildren(MultiBranchProject.java:611)
	at PluginClassLoader for cloudbees-folder//com.cloudbees.hudson.plugins.folder.computed.ComputedFolder.updateChildren(ComputedFolder.java:269)
	at PluginClassLoader for cloudbees-folder//com.cloudbees.hudson.plugins.folder.computed.FolderComputation.run(FolderComputation.java:167)
	at PluginClassLoader for branch-api//jenkins.branch.MultiBranchProject$BranchIndexing.run(MultiBranchProject.java:1057)
	at hudson.model.ResourceController.execute(ResourceController.java:101)
	at hudson.model.Executor.run(Executor.java:447)

Steps to reproduce:

  • install GitHub App in more than one organization
  • configure GitHub App Credentials with owner left blank to infer value
  • configure multi branch pipeline using GitHub branch source
  • trigger "scan multibranch pipeline"

The regression was introduced since #796. The owner parameter is not used anymore and the credentials clone is initialized using the owner field read method instead of owner parameter value, resulting with null owner in the cloned credentials. While generating the token, there is no installation matching with empty owner, resulting in exception.

This fix the credentials clone by setting the owner from the parameter instead of clone field null value.

See JENKINS-73474 for more information.

@jeromepochat jeromepochat requested a review from a team as a code owner August 20, 2024 07:45
@jeromepochat jeromepochat marked this pull request as draft August 20, 2024 08:03
@jeromepochat jeromepochat marked this pull request as ready for review August 20, 2024 08:52
@jeromepochat jeromepochat mentioned this pull request Aug 20, 2024
Merged
@dwnusbaum
Copy link
Member

dwnusbaum commented Aug 20, 2024 •
edited
Loading

@jeromepochat What do you think about trying to create a WireMock-based regression test for this logic? It seems undesirable that something like this was not caught by tests.

@thiencisco
Copy link

thiencisco commented Aug 20, 2024

I agreed with @dwnusbaum. Please expedite this process because the latest version of the plugin is tied to the latest version of Jenkins. Many of our pipelines are currently blocked right now.

@jglick jglick mentioned this pull request Aug 20, 2024
Merged
jglick
jglick approved these changes Aug 20, 2024
View reviewed changes
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.

Looks right. As a regression, I would recommend releasing immediately and spend time on a proper integration test for the whole system as a follow-up.

@jglick jglick added the bug label Aug 20, 2024
@jglick jglick requested review from car-roll and dwnusbaum August 20, 2024 17:11
@jeromepochat
Copy link
Contributor Author

jeromepochat commented Aug 20, 2024 •
edited
Loading

Looks right. As a regression, I would recommend releasing immediately and spend time on a proper integration test for the whole system as a follow-up.

Agree. I'll create another PR for the integration test ASAP.

BTW, can anyone merge this one please as I don't have the permission? Thanks!

@dwnusbaum dwnusbaum merged commit 86fdb4d into jenkinsci:master Aug 20, 2024
@jeromepochat jeromepochat mentioned this pull request Aug 21, 2024
Merged
@jeromepochat
Copy link
Contributor Author

jeromepochat commented Aug 21, 2024

@jeromepochat What do you think about trying to create a WireMock-based regression test for this logic? It seems undesirable that something like this was not caught by tests.

#804

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jglick jglick jglick approved these changes

@dwnusbaum dwnusbaum dwnusbaum approved these changes

@car-roll car-roll Awaiting requested review from car-roll

Assignees

No one assigned

Labels

bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jeromepochat @dwnusbaum @thiencisco @jglick