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-73474] Fix GitHubAppCredentials owner inference #803

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

jeromepochat
Copy link
Contributor

@jeromepochat jeromepochat commented Aug 20, 2024

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.

@dwnusbaum
Copy link
Member

dwnusbaum commented Aug 20, 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.

@thiencisco
Copy link

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.

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
@jeromepochat
Copy link
Contributor Author

jeromepochat commented Aug 20, 2024

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
16 checks passed
@jeromepochat
Copy link
Contributor Author

@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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants