[JENKINS-62249] Delegate GitHub App token generation from agents and improve caching #326

bitwiseman · 2020-08-14T20:36:20Z

Description

Makes GitHub App credentials more stable by requesting them less often and only requesting them via master.

The code was treating tokens as expired after 4 minutes on the assumption they were only valid for 8 minutes. They are actually valid for 60 minutes. It is safe to continue to return them for at least 15 minutes, leaving 45 minutes of valid use.

(NOTE: this PR does not address cases where individual operations take more than 45 minutes.)

It appears that a number of users have experienced network issues around agents requesting tokens directly from GitHub.
This change makes agents ask the controller to for updated tokens, rather than trying to get them directly.

Requesting new tokens less often and routing the requests through the controller mitigates most cases of checkout failing due to "socket timeout".

See
JENKINS-62249 for further information.

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/GitHubAppCredentials.java

bitwiseman · 2020-08-17T15:15:37Z

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

-            appInstallationToken = generateAppInstallationToken(appID, privateKey.getPlainText(), apiUri, owner);
-            cachedToken = appInstallationToken;
-            tokenCacheTime = now;
+        synchronized (this) {


If multiple requests for a password are made concurrently, we want to just get one refreshed token.

jglick · 2020-08-20T16:00:15Z

Amends #302.

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

…rial form is plaintext)

bitwiseman · 2020-08-24T17:31:58Z

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

@@ -104,7 +111,7 @@ public void setOwner(String owner) {
    }

    @SuppressWarnings("deprecation") // preview features are required for GitHub app integration, GitHub api adds deprecated to all preview methods
-    static String generateAppInstallationToken(String appId, String appPrivateKey, String apiUrl, String owner) {
+    static AppInstallationToken generateAppInstallationToken(String appId, String appPrivateKey, String apiUrl, String owner) {


Instead of returning the bare token, return a serializable instance of the token and when it should be refreshed.

jglick · 2020-08-24T17:41:35Z

Subsumes #330.

dwnusbaum

Looks ok to me. Would it be relatively easy to add WireMock tests for this behavior, especially with a token being used on an agent, or not really?

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

…bAppCredentials.java Co-authored-by: Devin Nusbaum <[email protected]>

bitwiseman · 2020-08-25T19:38:38Z

@dwnusbaum

JenkinsRule
Add agent
Add credential (Must have valid private key for Jwt to work, but App doesn't have to actually exist)
Add wiremock responses for App, App Installation, and App Installation Token
Add log reader
Run pipeline that includes a withEnv that includes the credential
Verify correct messages from GitHubAppCredential logger indicating token was retrieved on agent

Not breaking this is pretty important. I'll add a test.

pom.xml

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

…ation

Removed sorting that was likely to result in flaky results. There is only one way the agent log makes sense and the output is still checked.

jglick

Certainly do not follow all the twists and turns of the token expiration logic, but overall it looks good, assuming sanity testing passes.

jglick · 2020-09-01T13:57:52Z

pom.xml

@@ -52,6 +52,10 @@
            <artifactId>github</artifactId>
            <version>1.31.0</version>
        </dependency>
+        <dependency>
+            <groupId>org.jenkins-ci.plugins.workflow</groupId>
+            <artifactId>workflow-support</artifactId>


For Timeout I guess. Wonder if this should be moved to the likes of plugin-util-api…

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

jglick · 2020-09-01T14:01:13Z

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

+            return appInstallationToken.getExpiresAt()
+                .toInstant()
+                .getEpochSecond();
+        } catch (Exception e) {


What exception is expected here?

I was confused about this earlier too. IOException is declared to be thrown by GHAppInstallationToken.getExpiresAt, but looking at GitHubClient.parseDate I think the throws clause on GHAppInstallationToken.getExpiresAt could just be dropped. Date parsing could throw runtime exceptions like DateTimeParseException (not sure what would cause it in practice, perhaps changes on GitHub's side), so maybe that is the kind of thing this is trying to catch.

Implementation has changed over time, but changing the throws is an API change that will cause compilation to fail, right?

It is also possible for GitHub to hand us values that don't parse. Regardless of why it fails, we want to recover and continue.

jglick · 2020-09-01T14:05:08Z

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

+
+    private static final class DelegatingGitHubAppCredentials extends BaseStandardCredentials implements StandardUsernamePasswordCredentials {
+
+        private static final String SEP = "%%%";


Tried to pick something unlikely to occur in any of the field values. I guess you could use JSONObject to be a little safer.

src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsTest.java

dwnusbaum · 2020-09-01T14:44:04Z

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

+            return appInstallationToken.getExpiresAt()
+                .toInstant()
+                .getEpochSecond();
+        } catch (Exception e) {


I was confused about this earlier too. IOException is declared to be thrown by GHAppInstallationToken.getExpiresAt, but looking at GitHubClient.parseDate I think the throws clause on GHAppInstallationToken.getExpiresAt could just be dropped. Date parsing could throw runtime exceptions like DateTimeParseException (not sure what would cause it in practice, perhaps changes on GitHub's side), so maybe that is the kind of thing this is trying to catch.

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

jglick · 2022-03-23T22:31:42Z

...org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsAppInstallationTokenTest.java

+        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));


Observed to flake in #527:

java.lang.AssertionError: Expected: <1648072806L> but: was <1648072807L> at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20) at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6) at org.jenkinsci.plugins.github_branch_source.GithubAppCredentialsAppInstallationTokenTest.testAppInstallationTokenStale(GithubAppCredentialsAppInstallationTokenTest.java:23)

I do not think you can make an assertion like this without some grace period.

… flaking; use `closeTo` for actual times

bitwiseman requested a review from timja August 14, 2020 20:36

bitwiseman added the work-in-progress label Aug 14, 2020

timja reviewed Aug 14, 2020

View reviewed changes

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

bitwiseman marked this pull request as draft August 17, 2020 15:11

bitwiseman removed the work-in-progress label Aug 17, 2020

bitwiseman commented Aug 17, 2020

View reviewed changes

timja reviewed Aug 20, 2020

View reviewed changes

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

jglick mentioned this pull request Aug 20, 2020

Let GitHubAppCredentials on an agent just delegate to the master #329

Closed

bitwiseman force-pushed the issue/appid-expiration branch from f9e088d to f3a2aa1 Compare August 20, 2020 18:08

jglick added 7 commits August 20, 2020 14:56

GitHubAppCredentials on remote side to use SlaveToMasterCallable

fd58e5e

SpotBugs

22ef38d

No need to contact controller just to get appID

74902c3

Secret.fromString will not work on the agent side prior to 2.235.x

7f23a79

Maybe it works to call the Secret ctor on the controller side (the se…

de2860b

…rial form is plaintext)

Adding some JenkinsJVM calls to try to clarify the control flow

790ec23

Possibly more descriptive name

e48eb83

bitwiseman force-pushed the issue/appid-expiration branch 2 times, most recently from 8244fde to 1136fca Compare August 24, 2020 17:21

Add accurate token expiration and agent-side caching

8367aeb

bitwiseman force-pushed the issue/appid-expiration branch from 1136fca to 8367aeb Compare August 24, 2020 17:23

bitwiseman commented Aug 24, 2020

View reviewed changes

timja approved these changes Aug 24, 2020

View reviewed changes

bitwiseman added 2 commits August 24, 2020 16:43

Add more logging

1443730

Add test for AppInstallationToken

b76dbdb

bitwiseman force-pushed the issue/appid-expiration branch from 5aea4eb to b76dbdb Compare August 25, 2020 01:06

bitwiseman marked this pull request as ready for review August 25, 2020 01:21

bitwiseman requested review from dwnusbaum and jtnord August 25, 2020 01:21

bitwiseman changed the title ~~Delegate GitHub App token generation from agents and improve caching~~ [JENKINS-62249] Delegate GitHub App token generation from agents and improve caching Aug 25, 2020

dwnusbaum approved these changes Aug 25, 2020

View reviewed changes

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

Update src/main/java/org/jenkinsci/plugins/github_branch_source/GitHu…

3ab69eb

…bAppCredentials.java Co-authored-by: Devin Nusbaum <[email protected]>

bitwiseman added 3 commits August 28, 2020 15:32

Renamed token test

23773cd

Add test for GitHubAppCredentials

81c9163

Addtional testing revealed poor behavior

dc3b565

bitwiseman commented Sep 1, 2020

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

bitwiseman requested review from jglick and dwnusbaum and removed request for jglick September 1, 2020 01:37

bitwiseman commented Sep 1, 2020

View reviewed changes

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

bitwiseman force-pushed the issue/appid-expiration branch 2 times, most recently from 34e5409 to 399ee15 Compare September 1, 2020 08:15

bitwiseman added 2 commits September 1, 2020 01:19

Check validity of app token before sending to agents

52526b4

Merge remote-tracking branch 'upstream/master' into issue/appid-expir…

54e01d5

…ation

bitwiseman force-pushed the issue/appid-expiration branch from 399ee15 to 54e01d5 Compare September 1, 2020 08:21

Simplified log checking

3ac6082

Removed sorting that was likely to result in flaky results. There is only one way the agent log makes sense and the output is still checked.

bitwiseman force-pushed the issue/appid-expiration branch from 746d626 to 3ac6082 Compare September 1, 2020 08:42

jglick approved these changes Sep 1, 2020

View reviewed changes

dwnusbaum approved these changes Sep 1, 2020

View reviewed changes

Update based on review feedback

d80588e

bitwiseman force-pushed the issue/appid-expiration branch from cefdf52 to d80588e Compare September 1, 2020 19:32

Switch to JSONObject for data blob

b605c64

bitwiseman merged commit 27830d9 into jenkinsci:master Sep 1, 2020

This was referenced Sep 3, 2020

Use CredentialsSnapshotTaker #334

Closed

Store token as Secret instead of String #335

Merged

bitwiseman mentioned this pull request Sep 21, 2020

Github apps token should be automatically refreshed hub4j/github-api#941

Closed

jglick reviewed Mar 23, 2022

View reviewed changes

jglick added a commit to jglick/github-branch-source-plugin-1 that referenced this pull request Mar 24, 2022

GithubAppCredentialsAppInstallationTokenTest from jenkinsci#326 was…

84d12a2

… flaking; use `closeTo` for actual times


		private static final class DelegatingGitHubAppCredentials extends BaseStandardCredentials implements StandardUsernamePasswordCredentials {

		private static final String SEP = "%%%";

[JENKINS-62249] Delegate GitHub App token generation from agents and improve caching #326

[JENKINS-62249] Delegate GitHub App token generation from agents and improve caching #326

Uh oh!

Conversation

bitwiseman commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Submitter checklist

Reviewer checklist

Documentation changes

Users/aliases to notify

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick commented Aug 20, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick commented Aug 24, 2020

Uh oh!

dwnusbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bitwiseman commented Aug 25, 2020

Uh oh!

Uh oh!

Uh oh!

jglick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bitwiseman commented Aug 14, 2020 •

edited

Loading