-
-
Notifications
You must be signed in to change notification settings - Fork 161
Revise tests for issue JENKINS-70101 #231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…ile names [JENKINS-70101] Signed-off-by: Jim Klimov <[email protected]>
… shared property declarations and group test-case families [JENKINS-70101] Signed-off-by: Jim Klimov <[email protected]>
… refactor to use verbosePipelines test class property [JENKINS-70101] Signed-off-by: Jim Klimov <[email protected]>
… 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 <[email protected]>
…updated cpsScriptCredentialTestHttpRequest() helper, and import their siblings from credentials-plugin#391 [JENKINS-70101] Signed-off-by: Jim Klimov <[email protected]>
…helper: make use of runnerTag argument [JENKINS-70101] Signed-off-by: Jim Klimov <[email protected]>
…helper: report cred ID if drilling withLocalCertLookup [JENKINS-70101] Signed-off-by: Jim Klimov <[email protected]>
…onally) verify that credentials pass over to remote agents correctly [JENKINS-70101] Signed-off-by: Jim Klimov <[email protected]>
|
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 <[email protected]>
…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 <[email protected]>
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)