[JENKINS-62249] Delegate GitHub App token generation from agents and improve caching #326
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| package org.jenkinsci.plugins.github_branch_source; | ||
|
|
||
| import org.junit.Test; | ||
|
|
||
| import java.time.Duration; | ||
| import java.time.Instant; | ||
|
|
||
| import static org.hamcrest.MatcherAssert.assertThat; | ||
| import static org.hamcrest.Matchers.*; | ||
|
|
||
| public class GithubAppCredentialsAppInstallationTokenTest { | ||
|
|
||
| @Test | ||
| public void testAppInstallationTokenStale() throws Exception { | ||
|
|
||
| GitHubAppCredentials.AppInstallationToken token; | ||
| long now; | ||
|
|
||
| now = Instant.now().getEpochSecond(); | ||
| token = new GitHubAppCredentials.AppInstallationToken("", now); | ||
| assertThat(token.isStale(), is(false)); | ||
| assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS)); | ||
|
There was a problem hiding this comment. Observed to flake in #527: I do not think you can make an assertion like this without some grace period. |
||
|
|
||
| now = Instant.now().getEpochSecond(); | ||
| token = new GitHubAppCredentials.AppInstallationToken("", | ||
| now + Duration.ofMinutes(15).getSeconds()); | ||
| assertThat(token.isStale(), is(false)); | ||
| assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS)); | ||
|
|
||
| now = Instant.now().getEpochSecond(); | ||
| token = new GitHubAppCredentials.AppInstallationToken("", | ||
| now + GitHubAppCredentials.AppInstallationToken.STALE_BEFORE_EXPIRATION_SECONDS + 2); | ||
| assertThat(token.isStale(), is(false)); | ||
| assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS)); | ||
|
|
||
| now = Instant.now().getEpochSecond(); | ||
| token = new GitHubAppCredentials.AppInstallationToken("", | ||
| now + GitHubAppCredentials.AppInstallationToken.STALE_BEFORE_EXPIRATION_SECONDS + Duration | ||
| .ofMinutes(7) | ||
| .getSeconds()); | ||
| assertThat(token.isStale(), is(false)); | ||
| assertThat(token.getTokenStaleEpochSeconds(), | ||
| equalTo(now + Duration.ofMinutes(7).getSeconds())); | ||
|
|
||
| now = Instant.now().getEpochSecond(); | ||
| token = new GitHubAppCredentials.AppInstallationToken("", | ||
| now + Duration.ofMinutes(90).getSeconds()); | ||
| assertThat(token.isStale(), is(false)); | ||
| assertThat(token.getTokenStaleEpochSeconds(), | ||
| equalTo(now + GitHubAppCredentials.AppInstallationToken.STALE_AFTER_SECONDS + 1)); | ||
|
|
||
| long notStaleSeconds = GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS; | ||
| try { | ||
| // Should revert to 1 second minimum | ||
| GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS = -10; | ||
|
|
||
| now = Instant.now().getEpochSecond(); | ||
| token = new GitHubAppCredentials.AppInstallationToken("", now); | ||
| assertThat(token.isStale(), is(false)); | ||
| assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + 1)); | ||
|
|
||
| // Verify goes stale | ||
| Thread.sleep(1000); | ||
| assertThat(token.isStale(), is(true)); | ||
| } finally { | ||
| GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS = notStaleSeconds; | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,268 @@ | ||
| package org.jenkinsci.plugins.github_branch_source; | ||
|
|
||
| import com.cloudbees.plugins.credentials.CredentialsProvider; | ||
| import com.cloudbees.plugins.credentials.CredentialsScope; | ||
| import com.cloudbees.plugins.credentials.CredentialsStore; | ||
| import com.cloudbees.plugins.credentials.domains.Domain; | ||
| import hudson.logging.LogRecorder; | ||
| import hudson.logging.LogRecorderManager; | ||
| import hudson.model.Slave; | ||
| import hudson.util.Secret; | ||
| import jenkins.plugins.git.GitSampleRepoRule; | ||
| import jenkins.plugins.git.GitStep; | ||
| import org.jenkinsci.plugins.workflow.cps.CpsScmFlowDefinition; | ||
| import org.jenkinsci.plugins.workflow.job.WorkflowJob; | ||
| import org.jenkinsci.plugins.workflow.job.WorkflowRun; | ||
| import org.junit.Before; | ||
| import org.junit.BeforeClass; | ||
| import org.junit.Rule; | ||
| import org.junit.Test; | ||
| import org.jvnet.hudson.test.JenkinsRule; | ||
|
|
||
| import java.time.Duration; | ||
| import java.time.Instant; | ||
| import java.time.format.DateTimeFormatter; | ||
| import java.time.temporal.ChronoUnit; | ||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.Comparator; | ||
| import java.util.Date; | ||
| import java.util.List; | ||
| import java.util.logging.Formatter; | ||
| import java.util.logging.Level; | ||
| import java.util.logging.LogRecord; | ||
| import java.util.logging.SimpleFormatter; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| import static com.github.tomakehurst.wiremock.client.WireMock.*; | ||
| import static org.hamcrest.Matchers.*; | ||
|
|
||
| public class GithubAppCredentialsTest extends AbstractGitHubWireMockTest { | ||
|
|
||
| private static Slave agent; | ||
| private static final String myAppCredentialsId = "myAppCredentialsId"; | ||
| private static CredentialsStore store; | ||
| private static GitHubAppCredentials appCredentials; | ||
| private static LogRecorder logRecorder; | ||
|
|
||
| @Rule | ||
| public GitSampleRepoRule sampleRepo = new GitSampleRepoRule(); | ||
|
|
||
| @BeforeClass | ||
| public static void setUpJenkins() throws Exception { | ||
| //Add credential (Must have valid private key for Jwt to work, but App doesn't have to actually exist) | ||
| store = CredentialsProvider.lookupStores(r.jenkins).iterator().next(); | ||
| appCredentials = new GitHubAppCredentials( | ||
| CredentialsScope.GLOBAL, myAppCredentialsId, "sample", "54321", Secret.fromString(JwtHelperTest.PKCS8_PRIVATE_KEY)); | ||
| appCredentials.setOwner("cloudbeers"); | ||
| store.addCredentials(Domain.global(), appCredentials); | ||
|
|
||
| LogRecorderManager mgr = r.jenkins.getLog(); | ||
| logRecorder = new LogRecorder(GitHubAppCredentials.class.getName()); | ||
| mgr.logRecorders.put(GitHubAppCredentials.class.getName(), logRecorder); | ||
| LogRecorder.Target t = new LogRecorder.Target(GitHubAppCredentials.class.getName(), Level.FINER); | ||
| logRecorder.targets.add(t); | ||
| logRecorder.save(); | ||
| t.enable(); | ||
|
There was a problem hiding this comment. Did you just mean to use There was a problem hiding this comment. Yes, I tried LoggerRule and it doesn't capture agent logs. Otherwise, yes, I'd use LoggerRule. There was a problem hiding this comment. Got it. Probably something that could be improved in |
||
|
|
||
| // Add agent | ||
| agent = r.createOnlineSlave(); | ||
| agent.setLabelString("my-agent"); | ||
| } | ||
|
|
||
| @Before | ||
| public void setUpWireMock() throws Exception { | ||
| //Add wiremock responses for App, App Installation, and App Installation Token | ||
| githubApi.stubFor( | ||
| get(urlEqualTo("/app")) | ||
| .willReturn( | ||
| aResponse() | ||
| .withHeader("Content-Type", "application/json; charset=utf-8") | ||
| .withBodyFile("../AppCredentials/files/body-mapping-githubapp-app.json"))); | ||
| githubApi.stubFor( | ||
| get(urlEqualTo("/app/installations")) | ||
| .willReturn( | ||
| aResponse() | ||
| .withHeader("Content-Type", "application/json; charset=utf-8") | ||
| .withBodyFile("../AppCredentials/files/body-mapping-githubapp-installations.json"))); | ||
|
|
||
| final String scenarioName = "credentials-accesstoken"; | ||
|
|
||
| githubApi.stubFor( | ||
| post(urlEqualTo("/app/installations/654321/access_tokens")) | ||
bitwiseman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| .inScenario(scenarioName) | ||
| .whenScenarioStateIs("Started") | ||
| .willSetStateTo("1") | ||
| .withRequestBody( | ||
| equalToJson( | ||
| "{\"permissions\":{\"pull_requests\":\"write\",\"metadata\":\"read\",\"checks\":\"write\",\"contents\":\"read\"}}", true, false)) | ||
| .willReturn( | ||
| aResponse() | ||
| .withHeader("Content-Type", "application/json; charset=utf-8") | ||
| .withBody("{\n" + | ||
| " \"token\": \"super-secret-token\",\n" + | ||
| // This token will go stale at the soonest allowed time but will no be expired for the duration of the test | ||
| " \"expires_at\": \"" + printDate(new Date(System.currentTimeMillis() + Duration.ofMinutes(10).toMillis())) + "\"" + // 2019-08-10T05:54:58Z | ||
| "}"))); | ||
|
|
||
| // Force an error to test fallback refreshing from agent | ||
| githubApi.stubFor( | ||
| post(urlEqualTo("/app/installations/654321/access_tokens")) | ||
| .inScenario(scenarioName) | ||
| .whenScenarioStateIs("1") | ||
| .willSetStateTo("2") | ||
| .withRequestBody( | ||
| equalToJson( | ||
| "{\"permissions\":{\"pull_requests\":\"write\",\"metadata\":\"read\",\"checks\":\"write\",\"contents\":\"read\"}}", true, false)) | ||
| .willReturn( | ||
| aResponse() | ||
| .withStatus(404) | ||
| .withStatusMessage("404 Not Found") | ||
| .withHeader("Content-Type", "application/json; charset=utf-8") | ||
| .withBody("{\"message\": \"File not found\"}"))); | ||
|
|
||
| // Force an error to test fallback to returning unexpired token | ||
| githubApi.stubFor( | ||
| post(urlEqualTo("/app/installations/654321/access_tokens")) | ||
| .inScenario(scenarioName) | ||
| .whenScenarioStateIs("1") | ||
| .willSetStateTo("2") | ||
| .withRequestBody( | ||
| equalToJson( | ||
| "{\"permissions\":{\"pull_requests\":\"write\",\"metadata\":\"read\",\"checks\":\"write\",\"contents\":\"read\"}}", true, false)) | ||
| .willReturn( | ||
| aResponse() | ||
| .withStatus(404) | ||
| .withStatusMessage("404 Not Found") | ||
| .withHeader("Content-Type", "application/json; charset=utf-8") | ||
| .withBody("{\"message\": \"File not found\"}"))); | ||
|
|
||
| // Force an error to test fallback refreshing from agent | ||
| githubApi.stubFor( | ||
| post(urlEqualTo("/app/installations/654321/access_tokens")) | ||
| .inScenario(scenarioName) | ||
| .whenScenarioStateIs("2") | ||
| .willSetStateTo("3") | ||
bitwiseman marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| .withRequestBody( | ||
| equalToJson( | ||
| "{\"permissions\":{\"pull_requests\":\"write\",\"metadata\":\"read\",\"checks\":\"write\",\"contents\":\"read\"}}", true, false)) | ||
| .willReturn( | ||
| aResponse() | ||
| .withStatus(404) | ||
| .withStatusMessage("404 Not Found") | ||
| .withHeader("Content-Type", "application/json; charset=utf-8") | ||
| .withBody("{\"message\": \"File not found\"}"))); | ||
|
|
||
| githubApi.stubFor( | ||
| post(urlEqualTo("/app/installations/654321/access_tokens")) | ||
| .inScenario(scenarioName) | ||
| .whenScenarioStateIs("3") | ||
| .willSetStateTo("4") | ||
| .withRequestBody( | ||
| equalToJson( | ||
| "{\"permissions\":{\"pull_requests\":\"write\",\"metadata\":\"read\",\"checks\":\"write\",\"contents\":\"read\"}}", true, false)) | ||
| .willReturn( | ||
| aResponse() | ||
| .withHeader("Content-Type", "application/json; charset=utf-8") | ||
| .withBodyFile("../AppCredentials/files/body-githubapp-create-installation-accesstokens.json"))); | ||
|
|
||
| } | ||
|
|
||
| @Test | ||
| public void testAgentRefresh() throws Exception { | ||
| long notStaleSeconds = GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS; | ||
| try { | ||
| appCredentials.setApiUri(githubApi.baseUrl()); | ||
|
|
||
| // We want to demonstrate successful caching without waiting for a the default 1 minute | ||
| // Must set this to a large enough number to avoid flaky test | ||
| GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS = 15; | ||
|
|
||
| final String jenkinsfile = String.join( | ||
| "\n", | ||
| "// run checkout several times", | ||
| "node ('my-agent') {", | ||
| // First Checkout on agent should use cached token passed via remoting | ||
bitwiseman marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| " checkout scm", | ||
| // Multiple checkouts in quick succession should cached token | ||
bitwiseman marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| " checkout scm", | ||
| " sleep " + (GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS + 2), | ||
| // Checkout after token expires refreshes via remoting - error on controller is not catastrophic | ||
| " checkout scm", | ||
| // Checkout after error will refresh again on controller | ||
| " checkout scm", | ||
| // Multiple checkouts in quick succession should use cached token | ||
| " checkout scm", | ||
| "}"); | ||
|
|
||
|
|
||
| sampleRepo.init(); | ||
| sampleRepo.write("Jenkinsfile", jenkinsfile); | ||
| sampleRepo.git("add", "Jenkinsfile"); | ||
| sampleRepo.git("commit", "--message=init"); | ||
|
|
||
| WorkflowJob job = r.createProject(WorkflowJob.class, "repo-master"); | ||
| GitStep gitStep = new GitStep(sampleRepo.toString()); | ||
| gitStep.setCredentialsId(myAppCredentialsId); | ||
| gitStep.setPoll(false); | ||
| job.setDefinition(new CpsScmFlowDefinition(gitStep.createSCM(), "Jenkinsfile")); | ||
bitwiseman marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| WorkflowRun run = job.scheduleBuild2(0).waitForStart(); | ||
| r.waitForCompletion(run); | ||
|
|
||
| System.out.println(JenkinsRule.getLog(run)); | ||
|
|
||
| List<String> credentialsLog = getOutputLines(); | ||
|
|
||
| //Verify correct messages from GitHubAppCredential logger indicating token was retrieved on agent | ||
| assertThat("Creds should cache on master, pass to agent, and refresh agent from master once", | ||
| credentialsLog, contains( | ||
| // (agent log added out of order, see below) | ||
| "Keeping cached GitHub App Installation Token for app ID 54321 on agent: token is stale but has not expired", | ||
| // node ('my-agent') { | ||
| "Generating App Installation Token for app ID 54321", | ||
| "Token will become stale after 15 seconds", | ||
| "Generated App Installation Token for app ID 54321", | ||
| "Retrieved GitHub App Installation Token for app ID 54321", | ||
| // checkout scm | ||
| // checkout scm | ||
| // sleep | ||
| // (^^^ No token generation for these three steps) | ||
| // checkout scm | ||
| "Generating App Installation Token for app ID 54321", | ||
| // (error forced by wiremock) | ||
| "Failed to retrieve GitHub App installation token for app ID 54321", | ||
| "Keeping cached GitHub App Installation Token for app ID 54321: token is stale but has not expired", | ||
| // (error forced by wiremock - failed refresh on the agent) | ||
| "Generating App Installation Token for app ID 54321 for agent", | ||
| "Failed to retrieve GitHub App installation token for app ID 54321", | ||
| // (agent log added out of order) "Keeping cached GitHub App Installation Token for app ID 54321 on agent: token is stale but has not expired", | ||
| // checkout scm - refresh on controller | ||
| "Generating App Installation Token for app ID 54321", | ||
| "Token will become stale after 15 seconds", | ||
| "Generated App Installation Token for app ID 54321", | ||
| "Retrieved GitHub App Installation Token for app ID 54321" | ||
| // checkout scm | ||
| // (No token generation) | ||
| )); | ||
| } finally { | ||
| GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS = notStaleSeconds; | ||
| logRecorder.doClear(); | ||
| } | ||
| } | ||
|
|
||
| private List<String> getOutputLines() { | ||
| final Formatter formatter = new SimpleFormatter(); | ||
| List<LogRecord> result = new ArrayList<>(logRecorder.getLogRecords()); | ||
| result.addAll(logRecorder.getSlaveLogRecords().get(agent.toComputer())); | ||
| Collections.reverse(result); | ||
| return result.stream() | ||
| .map(formatter::formatMessage) | ||
| .collect(Collectors.toList()); | ||
| } | ||
|
|
||
| static String printDate(Date dt) { | ||
| return DateTimeFormatter.ISO_INSTANT.format(Instant.ofEpochMilli(dt.getTime()).truncatedTo( | ||
| ChronoUnit.SECONDS)); | ||
| } | ||
|
|
||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
For
TimeoutI guess. Wonder if this should be moved to the likes ofplugin-util-api…