Skip to content
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

[WIP] Improve support for default ssh docker container #763

Closed
wants to merge 6 commits into from

Conversation

pjdarton
Copy link
Member

@pjdarton pjdarton commented Nov 21, 2019

The official Jenkins ssh container says it takes the SSH certificate (that it should accept) as the first (and only) argument.
The default docker-plugin behaviour is to explicitly specify the command (/usr/sbin/sshd...) to run sshd but (a) this doesn't work on Windows and (b) a bug in the container's entry-point script means that environment variables in the container don't get into the ssh daemon's environment, with the ultimate result that Java isn't on the $PATH, causing containers to fail to come online (unless you also set the "Java Path" to /usr/local/openjdk-8/bin/java).

This PR adds a new SSH connection method that provides the SSH certificate as the first argument to the container, which is exactly what the official ssh container expects.

Change summary:

  • the DockerComputerSSHConnector class has been refactored to reduce commonality between its key-strategy inner classes.
  • the ManuallyConfiguredSSHKey key-strategy inner class has been refactored but should be functionally unaltered.
  • the InjectSSHKey key-strategy inner class now shows up on the WebUI as "Inject SSH key using SSH AuthorizedKeysCommand option" instead of just "Inject SSH key".
  • the InjectSSHKey key-strategy inner class now also sets the JENKINS_SLAVE_SSH_PUBKEY environment variable (in the container) to the injected key ... although that's unlikely to be much use to anyone using the Java-8 Linux docker container because of a bug.
  • there's a new InjectSSHKeyAsContainerArgument key-strategy inner class that calls itself "Inject SSH key as 1st container argument"; this uses no Linux-specific commands and therefore may well work for Windows users.

TODO:

  • add help text that explains what each option does
  • test it.

@pjdarton pjdarton added the enhancement A PR providing an enhancement to existing functionality. label Nov 22, 2019
@pjdarton pjdarton changed the title Improve support for default ssh docker container [WIP] Improve support for default ssh docker container Nov 25, 2019
@pjdarton pjdarton added the WIP PR that is *not* ready for merging (but hopefully being worked on by the author). label Nov 25, 2019
@basil basil requested a review from a team as a code owner May 24, 2023 20:31
Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A promising start! It is never too late to finish this draft PR.

@basil
Copy link
Member

basil commented May 25, 2023

@pjdarton Is this PR still needed? I was able to successfully create a new Docker cloud template whose image was jenkins/ssh-agent:latest, whose connection method was Connect with SSH, whose SSH key was Inject SSH key, and whose user was jenkins.

@basil
Copy link
Member

basil commented May 26, 2023

@Peter-Darton-i2 Looks like at least some of this PR is still needed as explained in #966.

@pjdarton
Copy link
Member Author

It's been a while since I looked at this but IIRC the official docker image wanted to be invoked with specific arguments and then it'd "just work" whereas this plugin kinda overrode everything to force the connection to work on its terms instead of the way the official docker image intended.
... and I think this PR was to support using the official image the way that image wanted.

... but I'm no longer in a position where I can test things out "in anger" the way I used to (I used to use this plugin for work at scale; I don't use it at all anymore) so it could be abandoned without any loss to me ... unless someone else wants this enough to drive this forward.

@basil basil closed this May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A PR providing an enhancement to existing functionality. proposed-for-close WIP PR that is *not* ready for merging (but hopefully being worked on by the author).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants