Fix handling of -cert parameter in inbound agent #925
Conversation
Fixes jenkinsci#908 Add handling for the `-cert` parameter in `jenkins-agent` and `jenkins-agent.ps1` scripts. ## jenkins-agent - Add logic to handle the `-cert` parameter by reading the certificate file content. - Update the `exec` command to include the `-cert` parameter if provided. ## jenkins-agent.ps1 - Add logic to handle the `-cert` parameter by reading the certificate file content. - Update the `Start-Process` command to include the `-cert` parameter if provided. ## debian/Dockerfile - Add instructions to copy the certificate file to the container. - Update the `ENTRYPOINT` to include the `-cert` parameter if provided. ## alpine/Dockerfile - Add instructions to copy the certificate file to the container. - Update the `ENTRYPOINT` to include the `-cert` parameter if provided.
There was a problem hiding this comment.
Thanks for trying to work on this issue @biru-codeastromer
However, this PR still need a lot of work.
First of all, it looks like you have not tested your change:
- The CI checks are all red. Building the changed Dockerfiles fails immediately due to your change. Please, do not open an untested PR because it wastes your time, maintainer time, and CI build minutes.
- I don't see anything proving you have solved the problem described in Specifying a PEM-encoded self-signed root CA certificate via -cert does not work #908. Have you setup a Jenkins controller with a self signed certificate and verified that the image built from your PR works with the self signed certificate and its CA? If you have, then please describe step by step how a maintainer could test your change (at least manually) with screenshots and/or code snippets (preferred)
Second, you've jumped in a technical solution without checking first in the issue #908 if your solution makes sense. When you start writing code, it is too late if you haven't explained at least "how" will it be used.
Third: this PR is incomplete:
- You changed the Windows entrypoint powershell script, but not the associated Dockerfile. Why?
- You changed the Linux entrypoint shell script, but only the Debian and Alpine. Why not UBI9?
As such, I'm marking this PR as "need more work", but I'll also mark it as a draft until the solution is discussed in the issue with a consensus on the "go for it", and when the PR will have been demonstrated as tested
| @@ -35,7 +35,8 @@ Param( | |||
| $JenkinsJavaBin = '', | |||
| $JavaHome = $env:JAVA_HOME, | |||
| $JenkinsJavaOpts = '', | |||
| $RemotingOpts = '' | |||
| $RemotingOpts = '', | |||
| $Cert = '' # P7458 | |||
There was a problem hiding this comment.
What is the meaning of this comment exactly?
| @@ -75,6 +76,7 @@ if(![System.String]::IsNullOrWhiteSpace($Cmd)) { | |||
| 'InstanceIdentity' = 'JENKINS_INSTANCE_IDENTITY'; | |||
| 'Protocols' = 'JENKINS_PROTOCOLS'; | |||
| 'RemotingOpts' = 'REMOTING_OPTS'; | |||
| 'Cert' = 'JENKINS_CERT' # P7458 | |||
There was a problem hiding this comment.
What is the meaning of this comment exactly?
|
Hi Sir @dduportal I acknowledge that there are several areas where the PR needs significant improvements: Testing and CI Errors: I take full responsibility for not thoroughly testing the changes before submitting the PR. I will prioritize setting up a Jenkins controller with a self-signed certificate and validating that the image works as expected. I'll also ensure that all CI checks pass before resubmitting. Documentation and Verification: I'll prepare a step-by-step guide on how to test the changes, including screenshots and relevant code snippets. This will make it easier for maintainers to verify the solution. Discussion Before Implementation: You're absolutely right that jumping to a solution without prior discussion is not ideal. I'll revisit issue #908 to ensure my approach aligns with the consensus and clarify the intended usage before proceeding further. Completeness of Changes: I'll address the inconsistencies in the entrypoint scripts and Dockerfiles, ensuring all required components (including UBI9) are covered and aligned. Comment Clarity: The comments you flagged in the jenkins-agent.ps1 file are unclear and need revision. I'll provide proper explanations and context in the updated version. |
|
I will update the PR once all these issues have been addressed, and I will also move forward with discussions in issue #908 to align on the solution before reopening. Thank you again for your patience ,constructive feedback and guidance! |
Fixes #908
Add handling for the
-certparameter injenkins-agentandjenkins-agent.ps1scripts.jenkins-agent
-certparameter by reading the certificate file content.execcommand to include the-certparameter if provided.jenkins-agent.ps1
-certparameter by reading the certificate file content.Start-Processcommand to include the-certparameter if provided.debian/Dockerfile
ENTRYPOINTto include the-certparameter if provided.alpine/Dockerfile
ENTRYPOINTto include the-certparameter if provided.Please review the changes and let me know what improvements can be made !