Cache app installation token #291

jglick · 2020-03-26T17:14:44Z

Description

Amends #269 by caching the app installation token for almost the duration of its validity. Otherwise every time the token is requested, which would be for every API call to GitHub, Jenkins would by my count be making three additional HTTP requests: /app, /app/installations, and /app/installations/:id/access_tokens.

Tested directly via jshell (note that this requires jenkinsci/jenkins#4603):

var c = new org.jenkinsci.plugins.github_branch_source.GitHubAppCredentials(com.cloudbees.plugins.credentials.CredentialsScope.GLOBAL, "test", null, "…",
    hudson.util.Secret.fromString(org.apache.commons.io.FileUtils.readFileToString(new java.io.File("/…-converted.pem"))))
var tok = c.getPassword().getPlainText()
new org.kohsuke.github.GitHubBuilder().withPassword(c.getUsername(), tok).build().getRateLimit().getRemaining()

I am not sure if there is a simple way to create a logger which records every GH API access.

Have been running some simple multibranch projects with this test and have not noticed any issues.

Submitter checklist

Link to JIRA ticket in description, if appropriate.
Change is code complete and matches issue description
Automated tests have been added to exercise the changes
Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.

Reviewer checklist

Run the changes and verify that the change matches the issue description
Reviewed the code
Verified that the appropriate tests have been written or valid explanation given

Documentation changes

Link to jenkins.io PR, or an explanation for why no doc changes are needed

Users/aliases to notify

src/main/java/org/jenkinsci/plugins/github_branch_source/JwtHelper.java

timja

Looks good, I haven't tested interactively

… the safe side

timja · 2020-03-26T19:43:23Z

src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java

-        String appInstallationToken = generateAppInstallationToken(appID, privateKey.getPlainText(), apiUri);
+        long now = System.currentTimeMillis();
+        String appInstallationToken;
+        if (cachedToken != null && now - tokenCacheTime < JwtHelper.VALIDITY_MS /* extra buffer */ / 2) {


is this now valid for 4 minutes?

No, the token should be valid for 8m, but will be replaced after 4m.

oleg-nenashev · 2020-04-02T10:48:18Z

Plugin bug

22:52:17  		at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
22:52:17  java.lang.NoClassDefFoundError: Could not initialize class jenkins.model.Jenkins
22:52:17  	at hudson.ExtensionList.lookup(ExtensionList.java:433)
22:52:17  	at hudson.tasks.junit.TestNameTransformer.all(TestNameTransformer.java:40)
22:52:17  	at hudson.tasks.junit.TestNameTransformer.getTransformedName(TestNameTransformer.java:33)
22:52:17  	at hudson.tasks.junit.CaseResult.getTransformedTestName(CaseResult.java:273)
22:52:17  	at hudson.tasks.junit.SuiteResult.casesByName(SuiteResult.java:134)
22:52:17  	at hudson.tasks.junit.SuiteResult.addCase(SuiteResult.java:297)
22:52:17  	at hudson.tasks.junit.SuiteResult.<init>(SuiteResult.java:270)
22:52:17  	at hudson.tasks.junit.SuiteResult.parseSuite(SuiteResult.java:209)
22:52:17  	at hudson.tasks.junit.SuiteResult.parse(SuiteResult.java:181)
22:52:17  	at hudson.tasks.junit.TestResult.parse(TestResult.java:348)
22:52:17  	at hudson.tasks.junit.TestResult.parsePossiblyEmpty(TestResult.java:281)
22:52:17  	at hudson.tasks.junit.TestResult.parse(TestResult.java:206)
22:52:17  	at hudson.tasks.junit.TestResult.parse(TestResult.java:178)
22:52:17  	at hudson.tasks.junit.TestResult.<init>(TestResult.java:143)
22:52:17  	at hudson.tasks.junit.JUnitParser$ParseResultCallable.invoke(JUnitParser.java:146)
22:52:17  	at hudson.tasks.junit.JUnitParser$ParseResultCallable.invoke(JUnitParser.java:118)
22:52:17  	at hudson.FilePath$FileCallableWrapper.call(FilePath.java:3069)
22:52:17  	at hudson.remoting.UserRequest.perform(UserRequest.java:211)
22:52:17  	at hudson.remoting.UserRequest.perform(UserRequest.java:54)
22:52:17  	at hudson.remoting.Request$2.run(Request.java:369)
22:52:17  	at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:72)
22:52:17  Caused: java.io.IOException: Remote call on EC2 (aws) - Ubuntu 18.04 LTS (i-011fe09e35adc9532) failed
22:52:17  	at hudson.remoting.Channel.call(Channel.java:1004)
22:52:17  	at hudson.FilePath.act(FilePath.java:1069)
22:52:17  	at hudson.FilePath.act(FilePath.java:1058)

timja · 2020-04-02T10:58:44Z

Remote call on EC2 (aws) - Ubuntu 18.04 LTS (i-011fe09e35adc9532) failed

isn't it a remoting issue?

jglick · 2020-04-02T15:46:15Z

https://issues.jenkins-ci.org/browse/INFRA-2548

jglick · 2020-04-02T16:02:53Z

actually, https://issues.jenkins-ci.org/browse/JENKINS-61787

jglick · 2020-04-02T16:14:30Z

⇒ jenkinsci/junit-plugin#130

sladyn98

LGTM 👍

jglick · 2020-04-07T16:03:53Z

This seems to work for me, but has anyone else actually tried running it?

bitwiseman · 2020-04-07T21:18:11Z

@jglick Could you resolve the conflict here?

jglick · 2020-04-07T21:51:02Z

(with #290 FTR)

…ugin into GitHubAppCredentials.cachedToken

bitwiseman · 2020-04-07T22:30:33Z

@jglick

I am not sure if there is a simple way to create a logger which records every GH API access.

github-api GitHubWireMockRule makes it easy to do this. It is on my list to port that to this project, but I haven't had time yet.

EDIT: oh, you meant Jenkins Logger not rather than test logger.

bitwiseman · 2020-04-12T01:18:26Z

@jglick @timja
I wouldn't say this is a blocker for this PR, but let me get make sure I understand this.

Currently each call to getPassword gets a new key from GitHub. That key expires after 8 minutes. getPassword is called once for each a new GitHub object is being created by Connector. getPassword is not called again for that instance of GitHub object.

This change makes it so the same key is used for 4 minutes and then a new one is is created. The only key is still valid for a total of 8 minutes but that means may become invalid in as little as 4 minutes.

What happens if that GitHub object is still being used when the key expires? getPassword() is not called again, right? So, according to this that instance will start getting 401 errors:

The example above uses the maximum expiration time of 10 minutes, after which the API will start returning a 401 error

If there is bad behavior in the above situation, it may now start happening as soon as 4 minutes after a GitHub is instantiated.

I understand we create new GitHub instances at multiple points during scanning and checkout, but it is very possible for some instances to be around for as long as 8 minutes and certainly as long as 4 minutes.

Is there a point in the code that we're handling these errors and I'm not seeing it?

bitwiseman · 2020-04-12T01:24:00Z

@timja
Another question: Would it make sense to pull this functionality down into the github-api library and test it there. The test infrastructure is simpler and faster.

Also, this seems like functionality the would be useful for anyone that uses github-api with GitHub Apps.

jglick · 2020-04-12T02:40:43Z

Is there any code hanging on to an instance of GitHub (perhaps indirectly via some “wrapped” object) for more than a fraction of a second? If so, I guess that could break in App mode, with or without this PR. github-api should be asking for a potentially fresh authentication token on every HTTP request.

timja · 2020-04-12T08:10:45Z

src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java

+            appInstallationToken = cachedToken;
+        } else {
+            appInstallationToken = generateAppInstallationToken(appID, privateKey.getPlainText(), apiUri, owner);
+            cachedToken = appInstallationToken;


This token is valid for an hour not 10 minutes, and the expires_at field should be looked at (in case it changes in the future) https://github.com/jglick/github-branch-source-plugin-1/blob/GitHubAppCredentials.cachedToken/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java#L125-L129

The generated JWT that's used to get this token can only be valid for up to 10 minutes, but the retrieved token is an hour (I didn't see any documentation on this but I just tried it out locally).

Yup, this is shows the same.
https://github.com/github-api/github-api/blob/3cbddf1de9e6a0f261a55247a54f8afdfd168fef/src/test/resources/org/kohsuke/github/GHAppTest/wiremock/createToken/mappings/mapping-app-installations-3755540-access_tokens-7W6Uy.json#L22
https://github.com/github-api/github-api/blob/3cbddf1de9e6a0f261a55247a54f8afdfd168fef/src/test/resources/org/kohsuke/github/GHAppTest/wiremock/createToken/__files/body-app-installations-3755540-access_tokens-7W6Uy.json#L3

So I guess you are saying that we could potentially stretch out the interval between token refreshes to, say, 55m? Would be nice to see some docs on this though.

So I guess you are saying that we could potentially stretch out the interval between token refreshes to, say, 55m? Would be nice to see some docs on this though.

Yes, but probably better to look at expires_at in case they change it, I had a look and couldn't see any docs on it

timja · 2020-04-12T08:12:36Z

@timja
Another question: Would it make sense to pull this functionality down into the github-api library and test it there. The test infrastructure is simpler and faster.

Also, this seems like functionality the would be useful for anyone that uses github-api with GitHub Apps.

yes it would be great for caching of tokens to be handled there and possibly the authentication as a github app.

the complexities are around the key format and having to pull in JWT libraries and possibly bouncy castle if you want to handle the key format that github gives you the key in (here we work around that by asking the user to convert it to a modern format).

bitwiseman · 2020-04-13T17:05:50Z

@timja

having to pull in JWT

Can we make those optional dependencies similar to Okhttp?

timja · 2020-04-13T20:55:52Z

@timja

having to pull in JWT

Can we make those optional dependencies similar to Okhttp?

should be able to

Cache app installation token

60d07a0

jglick requested a review from timja March 26, 2020 17:14

timja reviewed Mar 26, 2020

View reviewed changes

src/main/java/org/jenkinsci/plugins/github_branch_source/JwtHelper.java Outdated Show resolved Hide resolved

Back to 8m

9da1f5a

timja approved these changes Mar 26, 2020

View reviewed changes

Refresh the token when it is halfway toward expiration (4m), to be on…

6d8b52a

… the safe side

timja reviewed Mar 26, 2020

View reviewed changes

SpotBugs

023cdca

oleg-nenashev approved these changes Apr 2, 2020

View reviewed changes

sladyn98 approved these changes Apr 7, 2020

View reviewed changes

bitwiseman closed this Apr 7, 2020

bitwiseman reopened this Apr 7, 2020

Merge branch 'master' of github.com:jenkinsci/github-branch-source-pl…

5cb7140

…ugin into GitHubAppCredentials.cachedToken

timja reviewed Apr 12, 2020

View reviewed changes

bitwiseman closed this Apr 13, 2020

bitwiseman reopened this Apr 13, 2020

bitwiseman approved these changes Apr 13, 2020

View reviewed changes

bitwiseman merged commit dbf6b03 into jenkinsci:master Apr 13, 2020

jglick deleted the GitHubAppCredentials.cachedToken branch April 13, 2020 21:04

jglick mentioned this pull request Apr 15, 2020

Skip GitHubOrgWebHook.register when using a GH app receiving all relevant events #289

Draft

jglick mentioned this pull request May 18, 2020

GitHubAppCredentials.writeReplace to avoid recalculating tokens #302

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache app installation token #291

Cache app installation token #291

jglick commented Mar 26, 2020 •

edited

Loading

timja left a comment

timja Mar 26, 2020

jglick Mar 26, 2020

oleg-nenashev commented Apr 2, 2020

timja commented Apr 2, 2020

jglick commented Apr 2, 2020

jglick commented Apr 2, 2020

jglick commented Apr 2, 2020

sladyn98 left a comment

jglick commented Apr 7, 2020

bitwiseman commented Apr 7, 2020

jglick commented Apr 7, 2020

bitwiseman commented Apr 7, 2020 •

edited

Loading

bitwiseman commented Apr 12, 2020

bitwiseman commented Apr 12, 2020

jglick commented Apr 12, 2020

timja Apr 12, 2020

bitwiseman Apr 13, 2020

jglick Apr 13, 2020

timja Apr 13, 2020

timja commented Apr 12, 2020

bitwiseman commented Apr 13, 2020 •

edited

Loading

timja commented Apr 13, 2020

Cache app installation token #291

Cache app installation token #291

Conversation

jglick commented Mar 26, 2020 • edited Loading

Description

Submitter checklist

Reviewer checklist

Documentation changes

Users/aliases to notify

timja left a comment

Choose a reason for hiding this comment

timja Mar 26, 2020

Choose a reason for hiding this comment

jglick Mar 26, 2020

Choose a reason for hiding this comment

oleg-nenashev commented Apr 2, 2020

timja commented Apr 2, 2020

jglick commented Apr 2, 2020

jglick commented Apr 2, 2020

jglick commented Apr 2, 2020

sladyn98 left a comment

Choose a reason for hiding this comment

jglick commented Apr 7, 2020

bitwiseman commented Apr 7, 2020

jglick commented Apr 7, 2020

bitwiseman commented Apr 7, 2020 • edited Loading

bitwiseman commented Apr 12, 2020

bitwiseman commented Apr 12, 2020

jglick commented Apr 12, 2020

timja Apr 12, 2020

Choose a reason for hiding this comment

bitwiseman Apr 13, 2020

Choose a reason for hiding this comment

jglick Apr 13, 2020

Choose a reason for hiding this comment

timja Apr 13, 2020

Choose a reason for hiding this comment

timja commented Apr 12, 2020

bitwiseman commented Apr 13, 2020 • edited Loading

timja commented Apr 13, 2020

jglick commented Mar 26, 2020 •

edited

Loading

bitwiseman commented Apr 7, 2020 •

edited

Loading

bitwiseman commented Apr 13, 2020 •

edited

Loading