Skip to content

Commit

Permalink
[JENKINS-73172] Reuse credentials object reference through scans to a…
Browse files Browse the repository at this point in the history 
…void frequent duplicated lookups (#787)

* [JENKINS-73172] Cache scan credentials in ThreadLocal to avoid frequent duplicated lookups

* [JENKINS-73172] Adapt GitHubSCMBuilder and fix tests

* [JENKINS-73172] Use CredentialsProvider.lookupCredentialsInItem

* [JENKINS-73172] Extract in a CredentialsCache

* [JENKINS-73172] Handle expiration

* [JENKINS-73172] Improve thoughput using parent context + add disabled via system property

* [JENKINS-73172] Keep referenc of the credentials through scans

* [JENKINS-73172] Revert ThreadLocal from Connector

* [JENKINS-73172] Add missing volatile

Co-authored-by: Robert Sandell <rsandell@cloudbees.com>

* Restrict

* [JENKINS-73172] Apply spotless

* [JENKINS-73172] Spotbugs fix / Prevent potential NPE

* [JENKINS-73172] Apply non deprecated API suggestions

Co-authored-by: Jérôme Pochat <jpochat@cloudbees.com>

* [JENKINS-73172] Make method private

Co-authored-by: Jérôme Pochat <jpochat@cloudbees.com>

* [JENKINS-73172] Infer ID from credentials object if not null for consistency

---------

Co-authored-by: Robert Sandell <rsandell@cloudbees.com>
Co-authored-by: Jérôme Pochat <jpochat@cloudbees.com>
  • Loading branch information
@Dohbedoh @rsandell @jeromepochat
3 people authored Oct 16, 2024
1 parent 11ce4fe commit 98e3d8a
Show file tree
Hide file tree
Showing 7 changed files with 150 additions and 97 deletions.
31 changes: 15 additions & 16 deletions src/main/java/org/jenkinsci/plugins/github_branch_source/Connector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -295,23 +295,22 @@ public static StandardCredentials lookupScanCredentials(
@CheckForNull String repoOwner) {
if (Util.fixEmpty(scanCredentialsId) == null) {
return null;
} else {
StandardCredentials c = CredentialsMatchers.firstOrNull(
CredentialsProvider.lookupCredentials(
StandardUsernameCredentials.class,
context,
context instanceof Queue.Task
? ((Queue.Task) context).getDefaultAuthentication()
: ACL.SYSTEM,
githubDomainRequirements(apiUri)),
CredentialsMatchers.allOf(
CredentialsMatchers.withId(scanCredentialsId), githubScanCredentialsMatcher()));
if (c instanceof GitHubAppCredentials && repoOwner != null) {
return ((GitHubAppCredentials) c).withOwner(repoOwner);
} else {
return c;
}
}
StandardCredentials c = CredentialsMatchers.firstOrNull(
CredentialsProvider.lookupCredentialsInItem(
StandardUsernameCredentials.class,
context,
context instanceof Queue.Task
? ((Queue.Task) context).getDefaultAuthentication2()
: ACL.SYSTEM2,
githubDomainRequirements(apiUri)),
CredentialsMatchers.allOf(
CredentialsMatchers.withId(scanCredentialsId), githubScanCredentialsMatcher()));

if (c instanceof GitHubAppCredentials && repoOwner != null) {
c = ((GitHubAppCredentials) c).withOwner(repoOwner);
}
return c;
}

/**
Expand Down
15 changes: 11 additions & 4 deletions src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,6 @@ public GitHubSCMBuilder(
if (repoUrl != null) {
withBrowser(new GithubWeb(repoUrl));
}
withCredentials(credentialsId(), null);
}

/**
Expand Down Expand Up @@ -189,7 +188,9 @@ public final RepositoryUriResolver uriResolver() {
* {@code null} to detect the the protocol based on the credentialsId. Defaults to HTTP if
* credentials are {@code null}. Enables support for blank SSH credentials.
* @return {@code this} for method chaining.
* @deprecated Use {@link #withCredentials(String)} and {@link #withResolver(RepositoryUriResolver)}
*/
@Deprecated
@NonNull
public GitHubSCMBuilder withCredentials(String credentialsId, RepositoryUriResolver uriResolver) {
if (uriResolver == null) {
Expand All @@ -200,6 +201,12 @@ public GitHubSCMBuilder withCredentials(String credentialsId, RepositoryUriResol
return withCredentials(credentialsId);
}

@NonNull
public GitHubSCMBuilder withResolver(RepositoryUriResolver uriResolver) {
this.uriResolver = uriResolver;
return this;
}

/**
* Returns a {@link RepositoryUriResolver} according to credentials configuration.
*
Expand All @@ -215,12 +222,12 @@ public static RepositoryUriResolver uriResolver(
return HTTPS;
} else {
StandardCredentials credentials = CredentialsMatchers.firstOrNull(
CredentialsProvider.lookupCredentials(
CredentialsProvider.lookupCredentialsInItem(
StandardCredentials.class,
context,
context instanceof Queue.Task
? ((Queue.Task) context).getDefaultAuthentication()
: ACL.SYSTEM,
? ((Queue.Task) context).getDefaultAuthentication2()
: ACL.SYSTEM2,

Check warning on line 230 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMBuilder.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 225-230 are not covered by tests
URIRequirementBuilder.create()
.withHostname(RepositoryUriResolver.hostnameFromApiUri(apiUri))
.build()),
Expand Down
29 changes: 17 additions & 12 deletions src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,9 @@ public class GitHubSCMNavigator extends SCMNavigator {
private transient Boolean buildForkPRHead;

private static final LoadingCache<String, Boolean> privateModeCache = createPrivateModeCache();
/** The cache of the credentials object */
@CheckForNull
private transient volatile StandardCredentials credentials;

/**
* Constructor.
Expand Down Expand Up @@ -252,6 +255,15 @@ public GitHubSCMNavigator(String apiUri, String repoOwner, String scanCredential
}
}

@CheckForNull
@Restricted(NoExternalUse.class)
private StandardCredentials getCredentials(@CheckForNull Item context, boolean forceRefresh) {
if (credentials == null || forceRefresh) {

Check warning on line 261 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigator.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 261 is only partially covered, 3 branches are missing
credentials = Connector.lookupScanCredentials(context, getApiUri(), getCredentialsId(), getRepoOwner());
}
return credentials;
}

/**
* Gets the API endpoint for the GitHub server.
*
Expand Down Expand Up @@ -962,9 +974,7 @@ public void visitSources(SCMSourceObserver observer) throws IOException, Interru
throw new AbortException("Must specify user or organization");
}

StandardCredentials credentials =
Connector.lookupScanCredentials((Item) observer.getContext(), apiUri, credentialsId, repoOwner);

StandardCredentials credentials = getCredentials(observer.getContext(), true);
// Github client and validation
GitHub github = Connector.connect(apiUri, credentials);
try {
Expand Down Expand Up @@ -1278,9 +1288,7 @@ public void visitSource(String sourceName, SCMSourceObserver observer) throws IO
throw new AbortException("Must specify user or organization");
}

StandardCredentials credentials =
Connector.lookupScanCredentials((Item) observer.getContext(), apiUri, credentialsId, repoOwner);

StandardCredentials credentials = getCredentials(observer.getContext(), false);
// Github client and validation
GitHub github;
try {
Expand Down Expand Up @@ -1544,9 +1552,7 @@ public List<Action> retrieveActions(
listener.getLogger().printf("Looking up details of %s...%n", getRepoOwner());
List<Action> result = new ArrayList<>();
String apiUri = Util.fixEmptyAndTrim(getApiUri());
StandardCredentials credentials =
Connector.lookupScanCredentials((Item) owner, getApiUri(), credentialsId, repoOwner);
GitHub hub = Connector.connect(getApiUri(), credentials);
GitHub hub = Connector.connect(getApiUri(), getCredentials(owner, true));
Connector.configureLocalRateLimitChecker(listener, hub);
boolean privateMode = !isEnableAvatar() || determinePrivateMode(apiUri);
try {
Expand Down Expand Up @@ -1642,9 +1648,7 @@ public void afterSave(@NonNull SCMNavigatorOwner owner) {
GitHubWebHook.get().registerHookFor(owner);
try {
// FIXME MINOR HACK ALERT

Check warning on line 1650 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigator.java

View check run for this annotation

ci.jenkins.io / Open Tasks Scanner

FIXME 

HIGH:
MINOR HACK ALERT
StandardCredentials credentials =
Connector.lookupScanCredentials((Item) owner, getApiUri(), credentialsId, repoOwner);
GitHub hub = Connector.connect(getApiUri(), credentials);
GitHub hub = Connector.connect(getApiUri(), getCredentials(owner, true));
try {
GitHubOrgWebHook.register(hub, repoOwner);
} finally {
Expand Down Expand Up @@ -1984,6 +1988,7 @@ public SourceFactory(GitHubSCMNavigatorRequest request) {
public SCMSource create(@NonNull String name) {
return new GitHubSCMSourceBuilder(getId() + "::" + name, apiUri, credentialsId, repoOwner, name)
.withRequest(request)
.withCredentials(credentials)

Check warning on line 1991 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigator.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 1651-1991 are not covered by tests
.build();
}
}
Expand Down
69 changes: 41 additions & 28 deletions src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import static org.apache.commons.lang.StringUtils.removeEnd;
import static org.jenkinsci.plugins.github_branch_source.Connector.isCredentialValid;
import static org.jenkinsci.plugins.github_branch_source.GitHubSCMBuilder.API_V3;
import static org.jenkinsci.plugins.github_branch_source.GitHubSCMBuilder.HTTPS;

import com.cloudbees.jenkins.GitHubWebHook;
import com.cloudbees.plugins.credentials.CredentialsNameProvider;
Expand Down Expand Up @@ -272,6 +273,9 @@ public class GitHubSCMSource extends AbstractGitSCMSource {
/** The cache of {@link ObjectMetadataAction} instances for each open PR. */
@NonNull
private transient /*effectively final*/ Map<Integer, ContributorMetadataAction> pullRequestContributorCache;
/** The cache of the credentials object */
@CheckForNull
private transient volatile StandardCredentials credentials;

/**
* Used during upgrade from 1.x to 2.2.0+ only.
Expand Down Expand Up @@ -317,6 +321,19 @@ public GitHubSCMSource(String repoOwner, String repository, String repositoryUrl
this.traits = new ArrayList<>();
}

/**
* Constructor that passes a looked up credentials object.
*
* @param repoOwner the repository owner.
* @param repository the repository name.
* @param credentials a {@link com.cloudbees.plugins.credentials.common.StandardCredentials}
*/
@Restricted(NoExternalUse.class)
GitHubSCMSource(String repoOwner, String repository, StandardCredentials credentials) {
this(repoOwner, repository, null, false);
this.credentials = credentials;
}

Check warning on line 335 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 333-335 are not covered by tests

/**
* Legacy constructor.
*
Expand Down Expand Up @@ -362,6 +379,15 @@ public GitHubSCMSource(
}
}

@CheckForNull
@Restricted(NoExternalUse.class)
private StandardCredentials getCredentials(@CheckForNull Item context, boolean forceRefresh) {
if (credentials == null || forceRefresh) {

Check warning on line 385 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 385 is only partially covered, one branch is missing
credentials = Connector.lookupScanCredentials(context, getApiUri(), getCredentialsId(), getRepoOwner());
}
return credentials;
}

@Restricted(NoExternalUse.class)
public boolean isConfiguredByUrl() {
return repositoryUrl != null;
Expand Down Expand Up @@ -598,8 +624,8 @@ public static void setCacheSize(int cacheSize) {
/** {@inheritDoc} */
@Override
public String getRemote() {
return GitHubSCMBuilder.uriResolver(getOwner(), apiUri, credentialsId)
.getRepositoryUri(apiUri, repoOwner, repository);
// Only HTTPS is applicable to the source remote with Username / Password credentials
return HTTPS.getRepositoryUri(apiUri, repoOwner, repository);
}

/** {@inheritDoc} */
Expand All @@ -619,7 +645,7 @@ public String getPronoun() {
@Restricted(DoNotUse.class)
@RestrictedSince("2.2.0")
public RepositoryUriResolver getUriResolver() {
return GitHubSCMBuilder.uriResolver(getOwner(), apiUri, credentialsId);
return HTTPS;

Check warning on line 648 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 628-648 are not covered by tests
}

@Restricted(NoExternalUse.class)
Expand Down Expand Up @@ -977,8 +1003,10 @@ protected final void retrieve(
@CheckForNull SCMHeadEvent<?> event,
@NonNull final TaskListener listener)
throws IOException, InterruptedException {
StandardCredentials credentials =
Connector.lookupScanCredentials((Item) getOwner(), apiUri, credentialsId, repoOwner);
// In case we are in an Organization Scan - i.e. (observer instanceof SCMHeadObserver.Any) - use the cached
// credentials
// https://github.com/jenkinsci/branch-api-plugin/blob/2.1169.va_f810c56e895/src/main/java/jenkins/branch/MultiBranchProjectFactory.java#L262
StandardCredentials credentials = getCredentials(getOwner(), !(observer instanceof SCMHeadObserver.Any));

Check warning on line 1009 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 1009 is only partially covered, one branch is missing
// Github client and validation
final GitHub github = Connector.connect(apiUri, credentials);
try {
Expand Down Expand Up @@ -1279,10 +1307,8 @@ public SCMRevision create(@NonNull PullRequestSCMHead head, @Nullable Void ignor
@Override
protected Set<String> retrieveRevisions(@NonNull TaskListener listener, Item retrieveContext)
throws IOException, InterruptedException {
StandardCredentials credentials =
Connector.lookupScanCredentials(retrieveContext, apiUri, credentialsId, repoOwner);
// Github client and validation
final GitHub github = Connector.connect(apiUri, credentials);
final GitHub github = Connector.connect(apiUri, getCredentials(retrieveContext, false));
try {
Connector.configureLocalRateLimitChecker(listener, github);
Set<String> result = new TreeSet<>();
Expand Down Expand Up @@ -1385,10 +1411,8 @@ protected Set<String> retrieveRevisions(@NonNull TaskListener listener, Item ret
@Override
protected SCMRevision retrieve(@NonNull String headName, @NonNull TaskListener listener, Item retrieveContext)
throws IOException, InterruptedException {
StandardCredentials credentials =
Connector.lookupScanCredentials(retrieveContext, apiUri, credentialsId, repoOwner);
// Github client and validation
final GitHub github = Connector.connect(apiUri, credentials);
final GitHub github = Connector.connect(apiUri, getCredentials(retrieveContext, false));
try {
Connector.configureLocalRateLimitChecker(listener, github);
// Input data validation
Expand Down Expand Up @@ -1614,10 +1638,8 @@ public void unwrap() throws IOException, InterruptedException {
@NonNull
@Override
protected SCMProbe createProbe(@NonNull SCMHead head, @CheckForNull final SCMRevision revision) throws IOException {
StandardCredentials credentials =
Connector.lookupScanCredentials((Item) getOwner(), apiUri, credentialsId, repoOwner);
// Github client and validation
GitHub github = Connector.connect(apiUri, credentials);
GitHub github = Connector.connect(apiUri, getCredentials(getOwner(), false));
try {
String fullName = repoOwner + "/" + repository;
final GHRepository repo = github.getRepository(fullName);
Expand All @@ -1632,11 +1654,8 @@ protected SCMProbe createProbe(@NonNull SCMHead head, @CheckForNull final SCMRev
@Override
@CheckForNull
protected SCMRevision retrieve(SCMHead head, TaskListener listener) throws IOException, InterruptedException {
StandardCredentials credentials =
Connector.lookupScanCredentials((Item) getOwner(), apiUri, credentialsId, repoOwner);

// Github client and validation
GitHub github = Connector.connect(apiUri, credentials);
GitHub github = Connector.connect(apiUri, getCredentials(getOwner(), false));
try {
try {
Connector.checkConnectionValidity(apiUri, listener, credentials, github);
Expand Down Expand Up @@ -1783,11 +1802,9 @@ PullRequestSource retrievePullRequestSource(int number) {
if (StringUtils.isNotBlank(repository)) {
String fullName = repoOwner + "/" + repository;
LOGGER.log(Level.INFO, "Getting remote pull requests from {0}", fullName);
StandardCredentials credentials =
Connector.lookupScanCredentials((Item) getOwner(), apiUri, credentialsId, repoOwner);
LogTaskListener listener = new LogTaskListener(LOGGER, Level.INFO);
try {
GitHub github = Connector.connect(apiUri, credentials);
GitHub github = Connector.connect(apiUri, getCredentials(getOwner(), false));

Check warning on line 1807 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 1311-1807 are not covered by tests
try {
Connector.configureLocalRateLimitChecker(listener, github);
ghRepository = github.getRepository(fullName);
Expand Down Expand Up @@ -1952,8 +1969,7 @@ protected List<Action> retrieveActions(@CheckForNull SCMSourceEvent event, @NonN
result.add(new GitHubRepoMetadataAction());
String repository = this.repository;

StandardCredentials credentials =
Connector.lookupScanCredentials((Item) getOwner(), apiUri, credentialsId, repoOwner);
StandardCredentials credentials = getCredentials(getOwner(), true);
GitHub hub = Connector.connect(apiUri, credentials);
try {
Connector.checkConnectionValidity(apiUri, listener, credentials, hub);
Expand Down Expand Up @@ -2857,8 +2873,7 @@ protected Set<String> create() {
.format(
"Connecting to %s to obtain list of collaborators for %s/%s%n",
apiUri, repoOwner, repository);
StandardCredentials credentials =
Connector.lookupScanCredentials((Item) getOwner(), apiUri, credentialsId, repoOwner);
StandardCredentials credentials = getCredentials(getOwner(), false);
// Github client and validation
try {
GitHub github = Connector.connect(apiUri, credentials);
Expand Down Expand Up @@ -2922,9 +2937,7 @@ public GHPermissionType fetch(String username) throws IOException, InterruptedEx
.format(
"Connecting to %s to check permissions of obtain list of %s for %s/%s%n",
apiUri, username, repoOwner, repository);
StandardCredentials credentials =
Connector.lookupScanCredentials((Item) getOwner(), apiUri, credentialsId, repoOwner);
github = Connector.connect(apiUri, credentials);
github = Connector.connect(apiUri, getCredentials(getOwner(), false));

Check warning on line 2940 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 2876-2940 are not covered by tests
String fullName = repoOwner + "/" + repository;
repo = github.getRepository(fullName);
}
Expand Down
19 changes: 17 additions & 2 deletions src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSourceBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
*/
package org.jenkinsci.plugins.github_branch_source;

import com.cloudbees.plugins.credentials.common.StandardCredentials;
import edu.umd.cs.findbugs.annotations.CheckForNull;
import edu.umd.cs.findbugs.annotations.NonNull;
import jenkins.scm.api.trait.SCMSourceBuilder;
Expand All @@ -45,6 +46,9 @@ public class GitHubSCMSourceBuilder extends SCMSourceBuilder<GitHubSCMSourceBuil
/** The repository owner. */
@NonNull
private final String repoOwner;
/** The credentials. */
@CheckForNull
private StandardCredentials credentials;

/**
* Constructor.
Expand Down Expand Up @@ -94,7 +98,7 @@ public final String apiUri() {
*/
@CheckForNull
public final String credentialsId() {
return credentialsId;
return credentials == null ? credentialsId : credentials.getId();
}

/**
Expand All @@ -107,11 +111,22 @@ public final String repoOwner() {
return repoOwner;
}

/**
* Pass the credentials object to the {@link GitHubSCMSource}.
* @param credentials the {@link com.cloudbees.plugins.credentials.common.StandardCredentials}
* @return the current builder
*/
@NonNull
SCMSourceBuilder<GitHubSCMSourceBuilder, GitHubSCMSource> withCredentials(StandardCredentials credentials) {
this.credentials = credentials;
return this;
}

/** {@inheritDoc} */
@NonNull
@Override
public GitHubSCMSource build() {
GitHubSCMSource result = new GitHubSCMSource(repoOwner, projectName());
GitHubSCMSource result = new GitHubSCMSource(repoOwner, projectName(), credentials);

Check warning on line 129 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSourceBuilder.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 101-129 are not covered by tests
result.setId(id());
result.setApiUri(apiUri());
result.setCredentialsId(credentialsId());
Expand Down
Loading

0 comments on commit 98e3d8a

Please sign in to comment.