-
Notifications
You must be signed in to change notification settings - Fork 386
[JENKINS-69488] do not SnapShot GitHubAppCredentials #616
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
Changes from 3 commits
6b4afa5
7ffae8c
e655ec4
751a870
f961f1d
a45d053
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| package org.jenkinsci.plugins.github_branch_source; | ||
|
|
||
| import com.cloudbees.plugins.credentials.CredentialsSnapshotTaker; | ||
| import com.cloudbees.plugins.credentials.common.UsernamePasswordCredentials; | ||
| import com.cloudbees.plugins.credentials.impl.UsernamePasswordCredentialsSnapshotTaker; | ||
| import hudson.Extension; | ||
|
|
||
| /** | ||
| * A {@link CredentialsSnapshotTaker} for {@link GitHubAppCredentials} that is a no-op. | ||
| * | ||
| * <p>As {@code GitHubAppCredentials} tokens are time limited they need to be refreshed | ||
| * periodically. This is currently addressed by its use of the {@code writeReplace()} and {@code | ||
| * readResolve}, but as these credentials are {@link UsernamePasswordCredentials} this behaviour | ||
| * conflicts with the {@link UsernamePasswordCredentialsSnapshotTaker}. This SnapshotTaker restores | ||
| * the status quo allowing the Credentials to be replaced using the existing mechanism. | ||
| */ | ||
| @Extension | ||
| public class GitHubAppCredentialsSnapshotTaker | ||
| extends CredentialsSnapshotTaker<GitHubAppCredentials> { | ||
|
|
||
| @Override | ||
| public GitHubAppCredentials snapshot(GitHubAppCredentials credentials) { | ||
| return credentials; | ||
| } | ||
|
|
||
| @Override | ||
| public Class<GitHubAppCredentials> type() { | ||
| return GitHubAppCredentials.class; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -1,8 +1,10 @@ | ||||
| package org.jenkinsci.plugins.github_branch_source; | ||||
|
|
||||
| import static com.github.tomakehurst.wiremock.client.WireMock.*; | ||||
| import static org.hamcrest.MatcherAssert.assertThat; | ||||
| import static org.hamcrest.Matchers.*; | ||||
| import static org.jenkinsci.plugins.github_branch_source.Connector.createGitHubBuilder; | ||||
| import static org.junit.Assert.assertThrows; | ||||
|
|
||||
| import com.cloudbees.plugins.credentials.CredentialsProvider; | ||||
| import com.cloudbees.plugins.credentials.CredentialsScope; | ||||
|
|
@@ -31,12 +33,9 @@ | |||
| import java.util.logging.SimpleFormatter; | ||||
| import java.util.stream.Collectors; | ||||
| import jenkins.plugins.git.GitSampleRepoRule; | ||||
| import org.apache.commons.lang3.JavaVersion; | ||||
| import org.apache.commons.lang3.SystemUtils; | ||||
| import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; | ||||
| import org.jenkinsci.plugins.workflow.job.WorkflowJob; | ||||
| import org.jenkinsci.plugins.workflow.job.WorkflowRun; | ||||
| import org.junit.Assume; | ||||
| import org.junit.Before; | ||||
| import org.junit.BeforeClass; | ||||
| import org.junit.Rule; | ||||
|
|
@@ -390,12 +389,8 @@ public void testProviderRefresh() throws Exception { | |||
|
|
||||
| @Test | ||||
| public void testAgentRefresh() throws Exception { | ||||
| // test is really flaky with Java 8 so let's make this Java 11 minimum as we will be Java 11 | ||||
| // soon | ||||
| Assume.assumeTrue( | ||||
| "Test will run only on Java11 minimum", | ||||
| SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_11)); | ||||
| long notStaleSeconds = GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS; | ||||
| final long notStaleSeconds = | ||||
| GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS; | ||||
| try { | ||||
| appCredentials.setApiUri(githubApi.baseUrl()); | ||||
|
|
||||
|
|
@@ -445,10 +440,11 @@ public void testAgentRefresh() throws Exception { | |||
|
|
||||
| // Create a pipeline job that points the above repo | ||||
| WorkflowJob job = r.createProject(WorkflowJob.class, "test-creds"); | ||||
| job.setDefinition(new CpsFlowDefinition(jenkinsfile, false)); | ||||
| job.setDefinition(new CpsFlowDefinition(jenkinsfile, true)); | ||||
| job.addProperty( | ||||
| new ParametersDefinitionProperty( | ||||
| new StringParameterDefinition("REPO", sampleRepo.toString()))); | ||||
|
|
||||
| WorkflowRun run = job.scheduleBuild2(0).waitForStart(); | ||||
| r.waitUntilNoActivity(); | ||||
|
|
||||
|
|
@@ -463,38 +459,37 @@ public void testAgentRefresh() throws Exception { | |||
| credentialsLog, | ||||
| contains( | ||||
| // (agent log added out of order, see below) | ||||
| "Generating App Installation Token for app ID 54321", // 1 | ||||
| "Generating App Installation Token for app ID 54321", // 2 | ||||
| "Failed to generate new GitHub App Installation Token for app ID 54321: cached token is stale but has not expired", // 3 | ||||
| "Generating App Installation Token for app ID 54321 on agent", // 1 | ||||
| "Failed to generate new GitHub App Installation Token for app ID 54321 on agent: cached token is stale but has not expired", // 2 | ||||
| "Generating App Installation Token for app ID 54321 on agent", // 3 | ||||
|
||||
| Level.FINE, "Generating App Installation Token for app ID {0} on agent", appID); |
The order here is messed up - the logs are "all the agent logs" then "all the controller logs". that really confused me at first and I am fixing that now so the logs will always be in the correct order. (which is why I have not yet enabled auto merge)
#609 fixed the test (but left the bug) - because the Delegated credential was no longer sent to the agent (as it was Snapshotted and converted to a basic username password) and so it never refreshed on the agent as it never existed there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be a bit more cleaner now in 751a870
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto