[JENKINS-69488] do not SnapShot GitHubAppCredentials #616

jtnord · 2022-09-01T12:55:30Z

The GitHubApp Credentials converts itself when serialized over remoting
to a DelegatingGitHubAppCredentials to support token refresh.

Whilst it would naively be trivial to replace this with an
implenentation in the GitHubAppCredentialsSnapshotTaker that did the
conversion, this causes issues in controller to controller propagation.

When a Snapshot is taken there may or may not be a Channel in place,
however this does not detract from the fact that it is done so
credentials can be held complete
The code could be obtaining the credentials to send to some code but may
be doing it in outside of a specific task (th a remoting channel (e.g. snapshot a credential, then
obtain a channel from a computer to execute some code). Whilst unlikely
for this specific credential type - it is also a very generic type and
nothing would prevent code from doing this - nor would that code be
incorrect.

This change purely goes back to the status quo before jenkinsci/credentials-plugin#327 was introduced.

Description

A brief summary describing the changes in this pull request. See
JENKINS-69488 for further information.

required due to jenkinsci/credentials-plugin#327 see also #336 #334

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

The GitHubApp Credentials converts itself when serialized over remoting to a DelegatingGitHubAppCredentials to support token refresh. Whilst it would naively be trivial to replace this with an implenentation in the GitHubAppCredentialsSnapshotTaker that did the conversion, this causes issues in controller to controller propagation. When a Snapshot is taken there may or may not be a Channel in place, however this does not detract from the fact that it is done so credentials can be held complete The code could be obtaining the credentials to send to some code but may be doing it in outside of a specific task (th a remoting channel (e.g. snapshot a credential, then obtain a channel from a computer to execute some code). Whilst unlikely for this specific credential type - it is also a very generic type and nothing would prevent code from doing this - nor would that code be incorrect.

jglick · 2022-09-01T14:10:21Z

testAgentRefresh failing.

jglick

Seems safe enough for a hotfix, though I recommend removing the writeReplace so that only CredentialsProvider.snapshot (typically called from git-client IIRC) will activate the special behavior.

jtnord · 2022-09-01T14:15:33Z

testAgentRefresh

I think this is the same flaky test @olamy was trying to track down in https://github.com/jenkinsci/github-branch-source-plugin/pull/609/checks?check_run_id=7918023888 ?

jtnord · 2022-09-01T15:01:26Z

possibly not flaky - failed twice locally, reset to master and it passed twice in a row...

Selectivly reverts part of 609

jglick · 2022-09-02T16:58:50Z

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

-              "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


Why is this happening? The tokens are supposed to be generated on the controller, not the agent.

#609 (comment)?

the logs are logs of both the agent and the controller - so the token should be "generated" on the agent (but it is not generated there - it is just asking the controller).

github-branch-source-plugin/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java

Line 583 in 7618247

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.

should be a bit more cleaner now in 751a870

jtnord · 2022-09-02T18:56:22Z

I have run the test locally with both java8 and java11 (windows) where it had appeared to be flaky previously. it did not fail locally for me. There is now more logging should it become flaky or the CI fails.

jtnord · 2022-09-02T18:57:46Z

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

      WorkflowRun run = job.scheduleBuild2(0).waitForStart();
      r.waitUntilNoActivity();

-      System.out.println(JenkinsRule.getLog(run));


replaced by the BuildWatcher.

jglick · 2022-09-02T19:16:32Z

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

+    // but even though we can not capture the logs we want to echo them
+    r.showAgentLogs(agent, loggerRule);


Could probably rework showAgentLogs to mirror messages to the controller with a prefix or whatever.

Not sure I understand.
Do you mean set a formatter here that prepends "$agentName"?

In which case something for JTH outside of the scope here?

something for JTH

Right.

outside of the scope here

Well, except insofar as it would be handy to pick up here.

the logs already contain the agent name., slave0 in the below example

61.145 [test-creds #1] using credential myAppCredentialsId 61.145 [test-creds #1] Fetching changes from the remote Git repository 61.206 [slave0] [id=211] FINE o.j.p.g.GitHubAppCredentials$DelegatingGitHubAppCredentials#getPassword: Generating App Installation Token for app ID 54321 on agent 61.207 [id=102] FINE o.j.p.g.GitHubAppCredentials$DelegatingGitHubAppCredentials$GetToken#call: Generating App Installation Token for app ID 54321 for agent 61.403 [test-creds #1] Checking out Revision a9cd113a37fe363bc18dc40ac3bb1024bbc62842 (refs/remotes/origin/master) 61.558 [test-creds #1] Commit message: "init" 61.578 [id=181] FINE o.j.p.g.GitHubAppCredentials#getToken: Generating App Installation Token for app ID 54321

So I'm no longer clear what is being asked here @jglick

showAgentLogs -> publish

perhaps it is the comment that is missleading. it is in reference to the comment on the LoggerRule

github-branch-source-plugin/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsTest.java

Lines 98 to 102 in a45d053

// here to aid debugging - we can not use LoggerRule for the test assertion as it only captures

// logs from the controller

@ClassRule

public static LoggerRule loggerRule =

new LoggerRule().record(GitHubAppCredentials.class, Level.FINE);

jglick · 2022-09-02T19:17:02Z

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

-    long notStaleSeconds = GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS;
+    final long notStaleSeconds =
+        GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS;


Used in the finally block which is untouched so not in the diff.

github-branch-source-plugin/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsTest.java

Lines 397 to 400 in f961f1d

} finally {

GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS = notStaleSeconds;

logRecorder.doClear();

}

Made it final to avoid it being changed accidentally..

Probably should use a FlagRule but probably best left for a larger cleanup

jglick · 2022-09-02T19:17:12Z

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

-    long notStaleSeconds = GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS;
+    final long notStaleSeconds =
+        GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS;


jglick · 2022-09-02T19:18:07Z

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

-              // (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
              // node ('my-agent') {
-              // checkout scm
-              // checkout scm
+              //   echo 'First Checkout on agent should use cached token passed via remoting'
+              //   git url: REPO, credentialsId: 'myAppCredentialsId'
+              "Generating App Installation Token for app ID 54321",
+              //   echo 'Multiple checkouts in quick succession should use cached token'
+              //   git ....
+              // (No token generation)
+              //   sleep
+              //   echo 'Checkout after token is stale refreshes via remoting - fallback due to
+              // unexpired token'
+              //   git ....


Is there a diff link to the pre-#609 state for ease of review?

jtnord/github-branch-source-plugin@a14edd2...JENKINS-69488#diff-03b8ddb1abb4051c64c1e2898e14bf79ff304a1fce3442fa3e789f0af96cdeab

Thanks. I guess the non-obvious parts of the test diff are due to the addition of chronological sorting.

yup. those where already commented inline (but the message may have changed slightly

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

…bAppCredentialsTest.java Co-authored-by: Jesse Glick <jglick@cloudbees.com>

jglick · 2022-09-06T16:19:12Z

Fine for me; let me know if and when you want this merged & released.

jtnord requested review from jglick and a team September 1, 2022 12:55

jglick added the bug label Sep 1, 2022

jglick approved these changes Sep 1, 2022

View reviewed changes

jtnord enabled auto-merge September 1, 2022 14:32

jtnord disabled auto-merge September 1, 2022 15:01

olamy approved these changes Sep 2, 2022

View reviewed changes

jtnord added 2 commits September 2, 2022 13:29

do not extend assert

7ffae8c

Restore working unit test

e655ec4

Selectivly reverts part of 609

jglick reviewed Sep 2, 2022

View reviewed changes

sort all logs by chronological order

751a870

jtnord commented Sep 2, 2022

View reviewed changes

remove stray logging left over from debug

f961f1d

jglick approved these changes Sep 2, 2022

View reviewed changes

Update src/test/java/org/jenkinsci/plugins/github_branch_source/Githu…

a45d053

…bAppCredentialsTest.java Co-authored-by: Jesse Glick <jglick@cloudbees.com>

jtnord merged commit d46793a into jenkinsci:master Sep 6, 2022

jtnord deleted the JENKINS-69488 branch September 6, 2022 16:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-69488] do not SnapShot GitHubAppCredentials #616

[JENKINS-69488] do not SnapShot GitHubAppCredentials #616

jtnord commented Sep 1, 2022 •

edited

jglick commented Sep 1, 2022

jglick left a comment

jtnord commented Sep 1, 2022

jtnord commented Sep 1, 2022

jglick Sep 2, 2022

jglick Sep 2, 2022

jtnord Sep 2, 2022 •

edited

jtnord Sep 2, 2022

jtnord commented Sep 2, 2022

jtnord Sep 2, 2022

jglick Sep 2, 2022

jtnord Sep 2, 2022 •

edited

jglick Sep 2, 2022

jtnord Sep 5, 2022 •

edited

jtnord Sep 5, 2022

jglick Sep 2, 2022

jtnord Sep 2, 2022 •

edited

jglick Sep 2, 2022

jglick Sep 2, 2022

jtnord Sep 5, 2022

jglick Sep 6, 2022

jtnord Sep 6, 2022

jglick commented Sep 6, 2022

		// but even though we can not capture the logs we want to echo them
		r.showAgentLogs(agent, loggerRule);

	// here to aid debugging - we can not use LoggerRule for the test assertion as it only captures
	// logs from the controller
	@ClassRule
	public static LoggerRule loggerRule =
	new LoggerRule().record(GitHubAppCredentials.class, Level.FINE);

	} finally {
	GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS = notStaleSeconds;
	logRecorder.doClear();
	}

[JENKINS-69488] do not SnapShot GitHubAppCredentials #616

[JENKINS-69488] do not SnapShot GitHubAppCredentials #616

Conversation

jtnord commented Sep 1, 2022 • edited

Description

Submitter checklist

Reviewer checklist

Documentation changes

Users/aliases to notify

jglick commented Sep 1, 2022

jglick left a comment

Choose a reason for hiding this comment

jtnord commented Sep 1, 2022

jtnord commented Sep 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord Sep 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord commented Sep 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord Sep 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord Sep 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord Sep 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Sep 6, 2022

jtnord commented Sep 1, 2022 •

edited

jtnord Sep 2, 2022 •

edited

jtnord Sep 2, 2022 •

edited

jtnord Sep 5, 2022 •

edited

jtnord Sep 2, 2022 •

edited