feat: adapted the [GitHubChecksPublisher] to be compatible with [Stan… #341

pcanilho · 2023-05-25T10:27:33Z

Added support for `StandardUsernameCredentials` for `github-branch-source-plugin` SCM credentials compatibility

Adapted the GitHubChecksPublisher publish function to support StandardUsernameCredentials instead of always enforcing credentials of the GitHubAppCredentials type.

Whenever the selected credentials type is of the GitHubAppCredentials type, the ApiUri is still fetched to guarantee backwards compatibility:

StandardUsernameCredentials credentials = context.getCredentials();
String apiUri = null;
if (credentials instanceof GitHubAppCredentials) {
    apiUri = ((GitHubAppCredentials) credentials).getApiUri();
}

Note:
It is worth raising attention that all StandardUsernameCredentials used with this plugin should be GHA-derived tokens and should also be compatible with github-branch-source-plugin.
In my use-case, the vault-plugin-secrets-github is used to generate such tokens which are then consumed by the Jenkins pipelines.

Testing done

Procedure

As extracted from here, I have ran the following command to both build & run the test suite:

$ mvn -Penable-jacoco clean verify -B -V -ntp

I have then installed the plugin (generated .hpi) via the /manage/pluginManager/advanced URL and restarted the Jenkins service to assert that the plugin was successfully installed.

Tests

SCM used:

Before change using VaultUsernamePasswordCredentialImpl:

After change using VaultUsernamePasswordCredentialImpl:

GitHub:

Submitter checklist

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

…dardUsernameCredentials] instead of enforcing [GitHubAppCredentials] allowing full compatibility with the [github-branch-source-plugin] credentials format

codecov · 2023-05-25T10:31:58Z

Codecov Report

Merging #341 (bd133c5) into master (26ee75c) will decrease coverage by 0.04%.
The diff coverage is 88.88%.

@@             Coverage Diff              @@
##             master     #341      +/-   ##
============================================
- Coverage     72.05%   72.02%   -0.04%     
  Complexity      162      162              
============================================
  Files            16       16              
  Lines           526      529       +3     
  Branches         49       50       +1     
============================================
+ Hits            379      381       +2     
  Misses          123      123              
- Partials         24       25       +1

Impacted Files	Coverage Δ
...ins/plugins/checks/github/GitHubChecksContext.java	`100.00% <ø> (ø)`
...s/plugins/checks/github/GitHubChecksPublisher.java	`98.07% <80.00%> (-1.93%)`	⬇️
...va/io/jenkins/plugins/checks/github/SCMFacade.java	`53.70% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

timja · 2023-05-25T11:56:05Z

I don't think a StandardUsernameCredentials is safe for this.
The tokens are only valid for 1 hour if I remember correctly.

Correct me if I'm wrong but I don't think a StandardUsernameCredentials will be automatically refreshed.
The GitHub app credentials has special handling for every time getPassword is called where it checks if it needs to refresh the credential.

Can you confirm if jobs that take over 1 hour work with this functionality?

Also can you add a test for this please?

isometry · 2023-05-25T13:09:45Z

Can you confirm if jobs that take over 1 hour work with this functionality?

For an actual StandardUsernameCredentials credential, almost certainly not, no, but those jobs are in the minority. The patch simply opens the door to using more capable subclasses, such as VaultUsernamePasswordCredentialImpl in combination with https://github.com/martinbaillie/vault-plugin-secrets-github, which well derive a new token on every credential reference. Without these fixes we simply can't use the plugin with our externalised token generator.

timja · 2023-05-25T13:33:15Z

which well derive a new token on every credential referenc

You may not want to get a new token on every reference but if it works might be fine.

Is there a test you can add to have some coverage of this?

anyone else got any thoughts? cc @XiongKezhi / @uhafner

pcanilho · 2023-05-25T13:38:14Z

Can you confirm if jobs that take over 1 hour work with this functionality?

For an actual StandardUsernameCredentials credential, almost certainly not, no, but those jobs are in the minority. The patch simply opens the door to using more capable subclasses, such as VaultUsernamePasswordCredentialImpl in combination with https://github.com/martinbaillie/vault-plugin-secrets-github, which well derive a new token on every credential reference. Without these fixes we simply can't use the plugin with our externalised token generator.

Thank you for jumping in @isometry and thank you @timja for raising this to my attention.

As @isometry mentioned, this change aims to allow the same SCM credentials that github-branch-source-plugin uses here. For example: click me!
We are otherwise blocked from using this plugin.

Since vault-plugin-secrets-github is the underlying credentials source engine, I don't expect collisions between the GHA-derived token and long-running pipelines.

I am taking steps to confirm that the long running jobs are not impacted with this change and will update this thread with my findings.

Also can you add a test for this please?

My intention was to keep the original functionality untouched by guaranteeing that credentials instances of the type GitHubAppCredentials still had getApiUri executed. Which kind of tests did you have in mind @timja ?

timja · 2023-05-25T14:05:27Z

possibly that a StandardUsernameCredentials is accepted and a different type isn't?

timja · 2023-05-25T14:06:05Z

We are otherwise blocked from using this plugin.

why can't you just let Jenkins issue the tokens like everyone else? 😄

pcanilho · 2023-05-25T14:25:57Z

Can you confirm if jobs that take over 1 hour work with this functionality?

For an actual StandardUsernameCredentials credential, almost certainly not, no, but those jobs are in the minority. The patch simply opens the door to using more capable subclasses, such as VaultUsernamePasswordCredentialImpl in combination with https://github.com/martinbaillie/vault-plugin-secrets-github, which well derive a new token on every credential reference. Without these fixes we simply can't use the plugin with our externalised token generator.

Thank you for jumping in @isometry and thank you @timja for raising this to my attention.

As @isometry mentioned, this change aims to allow the same SCM credentials that github-branch-source-plugin uses here. For example: click me! We are otherwise blocked from using this plugin.

Since vault-plugin-secrets-github is the underlying credentials source engine, I don't expect collisions between the GHA-derived token and long-running pipelines.

I am taking steps to confirm that the long running jobs are not impacted with this change and will update this thread with my findings.

Also can you add a test for this please?

My intention was to keep the original functionality untouched by guaranteeing that credentials instances of the type GitHubAppCredentials still had getApiUri executed. Which kind of tests did you have in mind @timja ?

I am happy to confirm that vault-plugin-secrets-github correctly handles token rotation which resolves the inherent issue of having long-running pipelines with GHA-derived tokens.

isometry · 2023-05-25T14:29:38Z

why can't you just let Jenkins issue the tokens like everyone else?

We have too many Jenkins instances to make this viable (don't ask), and it's rather too easy to extract the contents of internal Jenkins credentials (i.e. the indefinite validity GHA private key). With the Vault plugin we've wholly externalised per-instance credential handling to a trusted system, only expose secrets with maximum 1hr validity, and achieved fully granular per-instance authorization :-)

isometry · 2023-05-25T17:08:44Z

@timja : can you suggest an appropriate test to be implemented to unblock merge & release? The method signatures already ensure that only credentials inheriting from StandardUsernameCredentials will be returned.

timja · 2023-05-25T19:16:08Z

Let's go with this

aguthrie · 2023-06-21T17:25:27Z

This change appears to have broken using SSH credentials with this plugin: https://issues.jenkins.io/browse/JENKINS-71510

kutzi · 2023-07-05T12:33:29Z

I think I have an issue with this change, as when I updated to the latest GitHub Checks plugin, all jobs started trying to update the status in the public Github instead of our on-prem Enterprise GH - which caused all jobs to hang or fail!
I've downgraded now to 536.v2e4ea_8fa_e423 and the jobs are working again

kutzi · 2023-07-05T12:43:16Z

Found errors like this in the log:

java.io.FileNotFoundException: https://api.github.com/repos/our_orga/our_repo
	at org.kohsuke.github.GitHubConnectorResponseErrorHandler$1.onError(GitHubConnectorResponseErrorHandler.java:60)
	at org.kohsuke.github.GitHubClient.detectKnownErrors(GitHubClient.java:473)
	at org.kohsuke.github.GitHubClient.sendRequest(GitHubClient.java:435)
Caused: org.kohsuke.github.GHFileNotFoundException: https://api.github.com/repos/our_orga/our_repo {"message":"Not Found","documentation_url":"https://docs.github.com/rest/reference/repos#get-a-repository"}
	at org.kohsuke.github.GitHubClient.interpretApiError(GitHubClient.java:604)
	at org.kohsuke.github.GitHubClient.sendRequest(GitHubClient.java:450)
	at org.kohsuke.github.GitHubClient.sendRequest(GitHubClient.java:403)
	at org.kohsuke.github.Requester.fetch(Requester.java:85)
	at org.kohsuke.github.GHRepository.read(GHRepository.java:145)
	at org.kohsuke.github.GitHub.getRepository(GitHub.java:684)
	at io.jenkins.plugins.checks.github.GitHubChecksPublisher.getCreator(GitHubChecksPublisher.java:117)
	at io.jenkins.plugins.checks.github.GitHubChecksPublisher.publish(GitHubChecksPublisher.java:85)
	at io.jenkins.plugins.checks.status.BuildStatusChecksPublisher.publish(BuildStatusChecksPublisher.java:64)
	at io.jenkins.plugins.checks.status.BuildStatusChecksPublisher$ChecksGraphListener.lambda$onNewHead$0(BuildStatusChecksPublisher.java:244)
	at java.base/java.util.Optional.ifPresent(Optional.java:183)
	at io.jenkins.plugins.checks.status.BuildStatusChecksPublisher$ChecksGraphListener.onNewHead(BuildStatusChecksPublisher.java:244)
	at org.jenkinsci.plugins.workflow.cps.CpsFlowExecution.notifyListeners(CpsFlowExecution.java:1587)
	at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup$3.run(CpsThreadGroup.java:509)
	at org.jenkinsci.plugins.workflow.cps.CpsVmExecutorService$1.run(CpsVmExecutorService.java:38)
	at hudson.remoting.SingleLaneExecutorService$1.run(SingleLaneExecutorService.java:139)
	at jenkins.util.ContextResettingExecutorService$1.run(ContextResettingExecutorService.java:28)
	at jenkins.security.ImpersonatingExecutorService$1.run(ImpersonatingExecutorService.java:68)
	at jenkins.util.ErrorLoggingExecutorService.lambda$wrap$0(ErrorLoggingExecutorService.java:51)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:829)

our_orga/our_repo resides on out Github enterprise

BTW: I'm not aware that we have configured credentials of a specific GitHubAppCredentials type
Resp.: I cannot find any field in the credentials where I could configure an API URL

pcanilho · 2023-07-05T14:37:48Z

@kutzi , the current implementation relies on having the user provide a Jenkins credentials object of the GitHubAppCredentials type to be able to extract the GitHub Enterprise custom API URI:

String apiUri = null;
if (credentials instanceof GitHubAppCredentials) {
    apiUri = ((GitHubAppCredentials) credentials).getApiUri();
}

GitHub gitHub = Connector.connect(StringUtils.defaultIfBlank(apiUri, gitHubUrl),
        credentials);

AFAIK, it is not possible to attach this URI metadata elsewhere.

The change that this PR introduces is that this plugin now allows for any credentials of the type StandardUsernameCredentials to be provided as long as the GitHub API that the user is targeting is the default one:

private static final String GITHUB_URL = "https://api.github.com";

Having said this, this change shouldn't break any previously working configuration. However, currently it also only implicitly allows the usage of credentials of the GitHubAppCredentials type for GitHub Enterprise.

Q: Can you confirm that reverting the plugin version whilst maintaining the exact same Jenkins credential objects resolved your issue? i.e. is it that perhaps the GitHubAppCredentials object is not being correctly identified with the instanceof operator and therefore the *.getApiUri() method never gets called?

kutzi · 2023-07-05T14:52:47Z

I didn't change anything. I just reverted the GitHub Checks plugin and restarted.

Looking at the code changes, I cannot really see where the error lies, either...

kutzi · 2023-07-05T14:56:20Z

Checked the credentials again: we're not using any GitHubAppCredentials
I think the credential used for Github should be a simple Username/Password one

kutzi · 2023-07-05T15:01:44Z

I think that's the issue:
https://github.com/pcanilho/github-checks-plugin/blob/bd133c5bb2704d35ceefd7e4f1eb231945156be5/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksPublisher.java#L68
If not anything else is configured, apiUri is null and it will so fall back eventually to the public Github API

pcanilho · 2023-07-05T16:03:22Z

FYI: @kutzi I have opened a PR addressing this issue here: #351

meiswjn · 2023-07-07T07:09:56Z

This change essentially enabled the plugin to try to publish checks for all jobs and not just those that use an GitHub App credentials. In our case, we use service users with Personal Access Tokens (see #352) which throws plenty of exceptions.

feat: adapted the [GitHubChecksPublisher] to be compatible with [Stan…

bd133c5

…dardUsernameCredentials] instead of enforcing [GitHubAppCredentials] allowing full compatibility with the [github-branch-source-plugin] credentials format

pcanilho requested a review from a team as a code owner May 25, 2023 10:27

timja added the enhancement New feature or request label May 25, 2023

timja merged commit 79aa68b into jenkinsci:master May 25, 2023

timja mentioned this pull request Jun 21, 2023

Broken using SSH credentials with this plugin #347

Closed

pcanilho mentioned this pull request Jul 5, 2023

fix: prevent publication with unsupported credential types #351

Merged

meiswjn mentioned this pull request Jul 7, 2023

GitHub Checks is spamming the logs #352

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adapted the [GitHubChecksPublisher] to be compatible with [Stan… #341

feat: adapted the [GitHubChecksPublisher] to be compatible with [Stan… #341

pcanilho commented May 25, 2023 •

edited

Loading

codecov bot commented May 25, 2023

timja commented May 25, 2023 •

edited

Loading

isometry commented May 25, 2023

timja commented May 25, 2023

pcanilho commented May 25, 2023 •

edited

Loading

timja commented May 25, 2023

timja commented May 25, 2023

pcanilho commented May 25, 2023

isometry commented May 25, 2023

isometry commented May 25, 2023 •

edited

Loading

timja commented May 25, 2023

aguthrie commented Jun 21, 2023

kutzi commented Jul 5, 2023

kutzi commented Jul 5, 2023 •

edited

Loading

pcanilho commented Jul 5, 2023

kutzi commented Jul 5, 2023

kutzi commented Jul 5, 2023

kutzi commented Jul 5, 2023 •

edited

Loading

pcanilho commented Jul 5, 2023

meiswjn commented Jul 7, 2023

feat: adapted the [GitHubChecksPublisher] to be compatible with [Stan… #341

feat: adapted the [GitHubChecksPublisher] to be compatible with [Stan… #341

Conversation

pcanilho commented May 25, 2023 • edited Loading

Added support for StandardUsernameCredentials for github-branch-source-plugin SCM credentials compatibility

Testing done

Procedure

Tests

Submitter checklist

codecov bot commented May 25, 2023

Codecov Report

timja commented May 25, 2023 • edited Loading

isometry commented May 25, 2023

timja commented May 25, 2023

pcanilho commented May 25, 2023 • edited Loading

timja commented May 25, 2023

timja commented May 25, 2023

pcanilho commented May 25, 2023

isometry commented May 25, 2023

isometry commented May 25, 2023 • edited Loading

timja commented May 25, 2023

aguthrie commented Jun 21, 2023

kutzi commented Jul 5, 2023

kutzi commented Jul 5, 2023 • edited Loading

pcanilho commented Jul 5, 2023

kutzi commented Jul 5, 2023

kutzi commented Jul 5, 2023

kutzi commented Jul 5, 2023 • edited Loading

pcanilho commented Jul 5, 2023

meiswjn commented Jul 7, 2023

pcanilho commented May 25, 2023 •

edited

Loading

Added support for `StandardUsernameCredentials` for `github-branch-source-plugin` SCM credentials compatibility

timja commented May 25, 2023 •

edited

Loading

pcanilho commented May 25, 2023 •

edited

Loading

isometry commented May 25, 2023 •

edited

Loading

kutzi commented Jul 5, 2023 •

edited

Loading

kutzi commented Jul 5, 2023 •

edited

Loading