Revise tests for certificate authentication on ssh agents#231
Revise tests for certificate authentication on ssh agents#231MarkEWaite merged 15 commits intojenkinsci:masterfrom
Conversation
…ile names [JENKINS-70101] Signed-off-by: Jim Klimov <jimklimov+jenkinsci@gmail.com>
… shared property declarations and group test-case families [JENKINS-70101] Signed-off-by: Jim Klimov <jimklimov+jenkinsci@gmail.com>
… refactor to use verbosePipelines test class property [JENKINS-70101] Signed-off-by: Jim Klimov <jimklimov+jenkinsci@gmail.com>
… refactor to optionally juggle withReentrability and to debug-trace withLocalCertLookup [JENKINS-70101] Import and adapt code evicted from jenkinsci/credentials-plugin#391 Signed-off-by: Jim Klimov <jimklimov+jenkinsci@gmail.com>
…updated cpsScriptCredentialTestHttpRequest() helper, and import their siblings from credentials-plugin#391 [JENKINS-70101] Signed-off-by: Jim Klimov <jimklimov+jenkinsci@gmail.com>
…helper: make use of runnerTag argument [JENKINS-70101] Signed-off-by: Jim Klimov <jimklimov+jenkinsci@gmail.com>
…helper: report cred ID if drilling withLocalCertLookup [JENKINS-70101] Signed-off-by: Jim Klimov <jimklimov+jenkinsci@gmail.com>
…onally) verify that credentials pass over to remote agents correctly [JENKINS-70101] Signed-off-by: Jim Klimov <jimklimov+jenkinsci@gmail.com>
|
UPDATE: Got it to run and pass all defined test cases with custom-built Credentials plugin (which requires a recent ecosystem of Jenkins core and dependency plugins), with auto-detection of the build with fixed JENKINS-70101 working as expected:
What failed was a too-stone-age approach:
...with this, the run-time unpacked test area seems to be the same as when the |
…(), it seems to be more reliable [JENKINS-70101] Signed-off-by: Jim Klimov <jimklimov+jenkinsci@gmail.com>
…t code for use of HTTP Request plugin with Credentials [JENKINS-70101] Tests to make sure the complex call stack works properly are offloaded into that plugin, see jenkinsci/http-request-plugin#231 Signed-off-by: Jim Klimov <jimklimov+jenkinsci@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR updates HttpRequestStepCredentialsTest to exercise credentials usage in http-request Pipeline steps across controller, local node {} execution, and (optionally) a remoting agent JVM to validate behavior related to JENKINS-70101.
Changes:
- Expanded and refactored the test class with more helper methods/Javadoc and additional pipeline-script generation.
- Added conditional remote-agent tests (gated by detecting a credentials-plugin fix) for certificate credentials and username/password credentials.
- Introduced agent setup logic and additional credential preparation helpers.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/test/java/jenkins/plugins/http_request/HttpRequestStepCredentialsTest.java
Show resolved
Hide resolved
src/test/java/jenkins/plugins/http_request/HttpRequestStepCredentialsTest.java
Show resolved
Hide resolved
src/test/java/jenkins/plugins/http_request/HttpRequestStepCredentialsTest.java
Outdated
Show resolved
Hide resolved
| // Can be used to skip optional tests if we know we could not set up an agent | ||
| if (agent == null) | ||
| return false; | ||
| return agentUsable; | ||
| } | ||
|
|
||
| private Boolean setupAgent() throws OutOfMemoryError, Exception { | ||
| if (isAvailableAgent()) | ||
| return true; | ||
|
|
There was a problem hiding this comment.
agentUsable is initialized as null and isAvailableAgent() returns it directly when agent != null. Since setupAgent() uses if (isAvailableAgent()), a null value would trigger auto-unboxing and a NullPointerException. Consider making agentUsable a primitive boolean (default false) or have isAvailableAgent() return Boolean.TRUE.equals(agentUsable).
| if (isAvailableAgent()) | ||
| return true; | ||
|
|
||
| // See how credentialsPluginTestRemoteAlways is determined above | ||
| // and revise if the ultimately merged fix that started as | ||
| // https://github.com/jenkinsci/credentials-plugin/pull/391 | ||
| // gets changed before the merge or later on... | ||
| String msg_70101 = "This test needs a version of credentials-plugin with a fix for JENKINS-70101, and that does not seem to be deployed here"; | ||
| if (!credentialsPluginTestRemoteAlways) | ||
| assumeTrue(credentialsPluginDoesSnapshotsRight, msg_70101); | ||
|
|
||
| // else: credentialsPluginTestRemoteAlways, even if we fail | ||
| if (!credentialsPluginDoesSnapshotsRight) { | ||
| System.err.println("WARNING: " + msg_70101 + "; this test run was configured to try remote agents anyway"); | ||
| // return false; | ||
| } | ||
|
|
||
| // Note we anticipate this might fail e.g. due to system resources; | ||
| // it should not block the whole test suite from running | ||
| // (we would just dynamically skip certain test cases) | ||
| try { | ||
| // Define a "Permanent Agent" | ||
| Label agentLabel = Label.get(agentLabelString); |
There was a problem hiding this comment.
setupAgent() is intended to allow remote-agent tests to be skipped when resources are unavailable, but it still declares throws Exception and only catches Descriptor.FormException | NullPointerException. Failures like IOException, InterruptedException, or other runtime exceptions will fail the test rather than returning false for the surrounding assumeTrue(...). Consider catching broader exceptions around agent creation/connection, setting agentUsable=false, and returning false so the tests are reliably skipped instead of failing the suite.
| agentUsable = false; | ||
| for (long i = 0; i < 5; i++) { | ||
| Thread.sleep(1000); | ||
| agentLog = agent.getComputer().getLog(); | ||
| if (i == 2 && (agentLog == null || agentLog.isEmpty())) { | ||
| // Give it a little time to autostart, then kick it up if needed: | ||
| agent.getComputer().connect(true); // "always" should have started it; avoid duplicate runs | ||
| } | ||
| if (agentLog != null && agentLog.contains("Agent successfully connected and online")) { | ||
| agentUsable = true; | ||
| break; | ||
| } | ||
| } | ||
| System.out.println("Spawned build agent " + |
There was a problem hiding this comment.
Agent readiness is currently detected by polling agent.getComputer().getLog() and matching the hard-coded string "Agent successfully connected and online" with Thread.sleep retries. This is brittle (log messages can change) and slows the test suite. Prefer checking agent.getComputer().isOnline() / waitUntilOnline() (or JenkinsRule helpers) and waiting on a condition rather than parsing logs.
| System.err.println("WARNING: " + msg_70101 + "; this test run was configured to try remote agents anyway"); | ||
| // return false; | ||
| } | ||
|
|
||
| // Note we anticipate this might fail e.g. due to system resources; | ||
| // it should not block the whole test suite from running | ||
| // (we would just dynamically skip certain test cases) | ||
| try { | ||
| // Define a "Permanent Agent" | ||
| Label agentLabel = Label.get(agentLabelString); | ||
| agent = j.createOnlineSlave(agentLabel); | ||
| agent.setNodeDescription("Worker in another JVM, remoting used"); | ||
| agent.setNumExecutors(1); | ||
| agent.setMode(Node.Mode.EXCLUSIVE); | ||
| ///agent.setRetentionStrategy(new RetentionStrategy.Always()); | ||
|
|
||
| /* | ||
| // Add node envvars | ||
| List<Entry> env = new ArrayList<Entry>(); | ||
| env.add(new Entry("key1","value1")); | ||
| env.add(new Entry("key2","value2")); | ||
| EnvironmentVariablesNodeProperty envPro = new EnvironmentVariablesNodeProperty(env); | ||
| agent.getNodeProperties().add(envPro); | ||
| */ | ||
|
|
||
| String agentLog = null; | ||
| agentUsable = false; | ||
| for (long i = 0; i < 5; i++) { | ||
| Thread.sleep(1000); | ||
| agentLog = agent.getComputer().getLog(); | ||
| if (i == 2 && (agentLog == null || agentLog.isEmpty())) { | ||
| // Give it a little time to autostart, then kick it up if needed: | ||
| agent.getComputer().connect(true); // "always" should have started it; avoid duplicate runs | ||
| } | ||
| if (agentLog != null && agentLog.contains("Agent successfully connected and online")) { | ||
| agentUsable = true; | ||
| break; | ||
| } | ||
| } | ||
| System.out.println("Spawned build agent " + | ||
| "usability: " + agentUsable.toString() + | ||
| "; connection log:" + (agentLog == null ? " <null>" : "\n" + agentLog)); | ||
| } catch (Descriptor.FormException | NullPointerException e) { | ||
| agentUsable = false; |
There was a problem hiding this comment.
setupAgent() prints warnings and connection logs directly to System.err / System.out unconditionally. This can add a lot of noise to CI logs even when verbosePipelines is false. Consider guarding these prints behind verbosePipelines (or using a Logger with an appropriate level) so normal runs stay quiet.
src/test/java/jenkins/plugins/http_request/HttpRequestStepCredentialsTest.java
Outdated
Show resolved
Hide resolved
First First HTTP Request -> First HTTP Request Fixes jenkinsci#231 (comment)
MarkEWaite
changed the title
Context:
A long-standing PR jenkinsci/credentials-plugin#391 aims to fix (Certificate) Credential
snapshot()implementation to allow remote build agents to correctly use a Credential (there was originally a mismatch of encrypted bytes and a randomKEYspecific to each JVM run-time instance to manipulate them).The original problem was noticed with use of this HTTP Request plugin, so some of the tests in that PR revolved around making sure the multi-node pipelines using this plugin would work correctly.
Reviewers of that PR insisted on not adding dependencies (even test ones) unless strictly required, so this PR is posted to shift the weight of those tests into HTTP Request plugin: it already has dependencies on Credentials Plugin anyway, it does one way or another suffer practical usability issues (even through no fault of its own), and an earlier version of those tests (running only in Jenkins controller's local JVM) was imported and merged years ago as part of PR #120 for a loosely related issue.
The chain of commits in this PR modifies just the
HttpRequestStepCredentialsTestfile of all the Java code, first to better javadoc its existing data and methods, and later to import code from jenkinsci/credentials-plugin#391 (most of which would be dropped there) as needed to set up the remote build agent and run test cases with pipelines using it. Beside certificate credentials, this also adds testing of username credentials for good measure.Tests with a remote agent are currently optionally blocked away by
static private Boolean credentialsPluginDoesSnapshotsRightwhich istrueif class loader knows aboutcom.cloudbees.plugins.credentials.impl.CertificateCredentialsSnapshotTaker(it is added in jenkinsci/credentials-plugin#391 and so means using a build of Credentials Plugin with the fix for JENKINS-70101 issue) -- if the ultimate fix in that other PR ends up differently identifiable, or later code evolution changes it, this part may need revision. Actually when some fix gets merged over there, a small commit to HTTP Request over here to require a certain minimum version of Credentials plugin and to drop the complexity of this check in the test would also be a decent solution.credentialsPluginTestRemoteAlways = trueto do run remote tests and maybe fail trying, to confirm whether the original issue was resolved or not. I thought about configuring it via a JVM property or environment variable, but chose against adding a complex feature into this hopefully temporary fix.I have verified locally that:
credentialsPluginDoesSnapshotsRightis detected asfalseusing the default Credentials Plugin version, developer togglecredentialsPluginTestRemoteAlwaysis hard-coded asfalse)credentialsPluginTestRemoteAlwaystotrue, thetestCertHttpRequestOnNodeRemote()fails (as expected so far) with... but
testUsernamePasswordHttpRequestOnNodeRemote()actually passes (is not impacted by botched transfer ofSecretBytes)