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-73172] Reuse credentials object reference through scans to avoid frequent duplicated lookups #787

Merged
merged 16 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@
private static final String SALT = Long.toHexString(ENTROPY.nextLong());
private static final OkHttpClient baseClient =
JenkinsOkHttpClient.newClientBuilder(new OkHttpClient()).build();
private static final ThreadLocal<StandardCredentials> scanCredentials = new ThreadLocal<>();
Dohbedoh marked this conversation as resolved.
Show resolved Hide resolved

private Connector() {
throw new IllegalAccessError("Utility class");
Expand Down Expand Up @@ -295,23 +296,30 @@
@CheckForNull String repoOwner) {
if (Util.fixEmpty(scanCredentialsId) == null) {
return null;
}

StandardCredentials c = CredentialsCache.getOrCreate(
context,
apiUri,
scanCredentialsId,
() -> CredentialsMatchers.firstOrNull(
CredentialsProvider.lookupCredentialsInItem(
StandardUsernameCredentials.class,
context,
context instanceof Queue.Task
? ((Queue.Task) context).getDefaultAuthentication2()

Check warning on line 310 in src/main/java/org/jenkinsci/plugins/github_branch_source/Connector.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line

Line 310 is not covered by tests
: ACL.SYSTEM2,
githubDomainRequirements(apiUri)),
CredentialsMatchers.allOf(
CredentialsMatchers.withId(scanCredentialsId), githubScanCredentialsMatcher())));

if (c instanceof GitHubAppCredentials && repoOwner != null) {
c = ((GitHubAppCredentials) c).withOwner(repoOwner);
scanCredentials.set(((GitHubAppCredentials) c).withOwner(repoOwner));

Check warning on line 318 in src/main/java/org/jenkinsci/plugins/github_branch_source/Connector.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines

Lines 317-318 are not covered by tests
} 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;
}
scanCredentials.set(c);
}
return c;
}

/**
Expand All @@ -323,7 +331,7 @@
* @deprecated use {@link #listCheckoutCredentials(Item, String)}
*/
@NonNull
public static ListBoxModel listCheckoutCredentials(@CheckForNull SCMSourceOwner context, String apiUri) {

Check warning on line 334 in src/main/java/org/jenkinsci/plugins/github_branch_source/Connector.java

View check run for this annotation

ci.jenkins.io / Java Compiler

compiler:compile

NORMAL: deprecated item is not annotated with @deprecated
return listCheckoutCredentials((Item) context, apiUri);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
package org.jenkinsci.plugins.github_branch_source;

import com.cloudbees.plugins.credentials.common.StandardCredentials;
import edu.umd.cs.findbugs.annotations.CheckForNull;
import hudson.Util;
import hudson.model.Item;
import hudson.model.ItemGroup;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.TimeUnit;
import java.util.function.Supplier;
import jenkins.util.SystemProperties;

/**
* Caches the {@link com.cloudbees.plugins.credentials.common.StandardCredentials} per credentials request based on
* context, apiUri and credentialsId. The credentials are cached within a thread context using a {@link java.lang.ThreadLocal}.
* High cache hit ratio is expected in repeated usage within the same single threaded process such as Branch Indexing
* and Organization Scans.
*/
public class CredentialsCache {

Check warning on line 21 in src/main/java/org/jenkinsci/plugins/github_branch_source/CredentialsCache.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line

Line 21 is not covered by tests

/**
* The cache expiry in minutes. Default to 15 minutes.
*/
private static final long CACHE_DURATION_SECONDS = SystemProperties.getLong(
CredentialsCache.class.getName() + ".CACHE_DURATION_SECONDS", TimeUnit.MINUTES.toSeconds(1L));

/**
* Whether the cache is disabled.
*/
private static final boolean CACHE_DISABLED =
SystemProperties.getBoolean(CredentialsCache.class.getName() + ".CACHE_DISABLED", false);

private static final ThreadLocal<Map<Integer, CacheValue>> scanCredentialsRequests = new ThreadLocal<>();

/**
* Retrieve the cached credentials in the specified context for use against the specified API endpoint.
* If not found,
*
*
* @param context the context.
* @param apiUri the API endpoint.
* @param credentialsId the credentials to resolve.
* @return the {@link StandardCredentials} or {@code null}
*/
@CheckForNull
static StandardCredentials getOrCreate(
@CheckForNull Item context,
@CheckForNull String apiUri,
@CheckForNull String credentialsId,
Supplier<StandardCredentials> lookup) {
if (Util.fixEmpty(credentialsId) == null) {

Check warning on line 53 in src/main/java/org/jenkinsci/plugins/github_branch_source/CredentialsCache.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 53 is only partially covered, one branch is missing
return null;

Check warning on line 54 in src/main/java/org/jenkinsci/plugins/github_branch_source/CredentialsCache.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line

Line 54 is not covered by tests
}

if (CACHE_DISABLED) {

Check warning on line 57 in src/main/java/org/jenkinsci/plugins/github_branch_source/CredentialsCache.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 57 is only partially covered, one branch is missing
return lookup.get();

Check warning on line 58 in src/main/java/org/jenkinsci/plugins/github_branch_source/CredentialsCache.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line

Line 58 is not covered by tests
}

if (scanCredentialsRequests.get() == null) {
scanCredentialsRequests.set(new HashMap<>());
}

int cacheKey = Objects.hash(apiUri, credentialsId, context == null ? null : context.getFullName());

Check warning on line 65 in src/main/java/org/jenkinsci/plugins/github_branch_source/CredentialsCache.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 65 is only partially covered, one branch is missing
CacheValue result = scanCredentialsRequests.get().get(cacheKey);
if (result == null || result.expired()) {

Check warning on line 67 in src/main/java/org/jenkinsci/plugins/github_branch_source/CredentialsCache.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 67 is only partially covered, 3 branches are missing
if (context != null) {

Check warning on line 68 in src/main/java/org/jenkinsci/plugins/github_branch_source/CredentialsCache.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 68 is only partially covered, one branch is missing
/*
* Check if the credentials is cached for the parent context (SCMNavigator). If it is, it will be
* stored in cache for the parent context and also the requested context for better cache hit
* throughput.
* That guarantees that the credentials cache is used throughout an entire Organization Scan process
* that use the context of the OrganizationFolder and then a different child context for all repository
* MultibranchProjects that it visits.
*/
ItemGroup<?> parent = context.getParent();
int parentCacheKey = Objects.hash(apiUri, credentialsId, parent == null ? null : parent.getFullName());
result = scanCredentialsRequests.get().get(parentCacheKey);

if (result == null || result.expired()) {
result = new CacheValue(lookup.get());
scanCredentialsRequests.get().put(parentCacheKey, result);
}
} else {

Check warning on line 85 in src/main/java/org/jenkinsci/plugins/github_branch_source/CredentialsCache.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines

Lines 77-85 are not covered by tests
result = new CacheValue(lookup.get());
}
scanCredentialsRequests.get().put(cacheKey, result);
}

return result.getCredentials();
}

private static class CacheValue {

private final StandardCredentials credentials;

private final long requestTimeInMs;

public CacheValue(StandardCredentials credentials) {
this.credentials = credentials;
this.requestTimeInMs = System.currentTimeMillis();
}

public StandardCredentials getCredentials() {
return credentials;
}

public boolean expired() {
return (System.currentTimeMillis() - TimeUnit.SECONDS.toMillis(CACHE_DURATION_SECONDS)) > requestTimeInMs;

Check warning on line 110 in src/main/java/org/jenkinsci/plugins/github_branch_source/CredentialsCache.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line

Line 110 is not covered by tests
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,6 @@
if (repoUrl != null) {
withBrowser(new GithubWeb(repoUrl));
}
withCredentials(credentialsId(), null);
}

/**
Expand Down Expand Up @@ -189,7 +188,9 @@
* {@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 @@
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 @@
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
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 @@ -598,28 +599,28 @@
/** {@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
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you can be sure it is only username / password credentials?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rsandell Per my understanding, we can only have a HTTPS resolver from GitHubSCMSource. Only StandardUsernamePasswordCredentials can be selected and used to connect to the GitHub API:

I am not sure why it is not made more explicit with the typing.. But I don't see a scenario where it could be different and that makes sense since this SCM Source is using the GitHub Rest API..

The only specific scenario where we would see SSH if with the SSHCheckoutTrait and that is handled by the trait implementation.

Copy link
Member

Choose a reason for hiding this comment

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

It could be an app credential couldn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

It could be an app credential couldn't it?

Yes it is, but GitHubAppCredentials implements StandardUsernamePasswordCredentials and is used for REST calls over HTTPS as well as mentioned Alan. So "HTTPS resolver only" here looks good to me.

return HTTPS.getRepositoryUri(apiUri, repoOwner, repository);
}

/** {@inheritDoc} */
@Override
public String getPronoun() {
return Messages.GitHubSCMSource_Pronoun();
}

/**
* Returns a {@link RepositoryUriResolver} according to credentials configuration.
*
* @return a {@link RepositoryUriResolver}
* @deprecated use {@link GitHubSCMBuilder#uriResolver()} or {@link
* GitHubSCMBuilder#uriResolver(Item, String, String)}.
*/
@Deprecated
@Restricted(DoNotUse.class)
@RestrictedSince("2.2.0")
public RepositoryUriResolver getUriResolver() {
return GitHubSCMBuilder.uriResolver(getOwner(), apiUri, credentialsId);
return HTTPS;

Check warning on line 623 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 603-623 are not covered by tests
}

@Restricted(NoExternalUse.class)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@
/** {@inheritDoc} */
@Override
protected void decorateBuilder(SCMBuilder<?, ?> builder) {
((GitHubSCMBuilder) builder).withCredentials(credentialsId, GitHubSCMBuilder.SSH);
((GitHubSCMBuilder) builder).withCredentials(credentialsId).withResolver(GitHubSCMBuilder.SSH);
}

/** Our descriptor. */
Expand Down Expand Up @@ -190,24 +190,24 @@
return FormValidation.ok();
}
if (CredentialsMatchers.firstOrNull(
CredentialsProvider.lookupCredentials(
CredentialsProvider.lookupCredentialsInItem(
SSHUserPrivateKey.class,
context,
context instanceof Queue.Task
? ((Queue.Task) context).getDefaultAuthentication()
: ACL.SYSTEM,
? ((Queue.Task) context).getDefaultAuthentication2()
: ACL.SYSTEM2,
URIRequirementBuilder.fromUri(serverUrl).build()),
CredentialsMatchers.withId(value))
!= null) {
return FormValidation.ok();
}
if (CredentialsMatchers.firstOrNull(
CredentialsProvider.lookupCredentials(
CredentialsProvider.lookupCredentialsInItem(
StandardUsernameCredentials.class,
context,
context instanceof Queue.Task
? ((Queue.Task) context).getDefaultAuthentication()
: ACL.SYSTEM,
? ((Queue.Task) context).getDefaultAuthentication2()
: ACL.SYSTEM2,

Check warning on line 210 in src/main/java/org/jenkinsci/plugins/github_branch_source/SSHCheckoutTrait.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines

Lines 193-210 are not covered by tests
URIRequirementBuilder.fromUri(serverUrl).build()),
CredentialsMatchers.withId(value))
!= null) {
Expand Down
Loading
Loading