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

some common issues in all images (regarind using of zinit and ssh) #185

Open
muhamadazmy opened this issue Nov 30, 2023 · 1 comment
Open

Comments

@muhamadazmy
Copy link
Member

muhamadazmy commented Nov 30, 2023

I noticed that many of the zinit config files (for services) does this weird thing as follows

exec: bash -c "some command with arguments" 

I don't know why this is a pattern, why do u need bash to run a single command with arguments. This instead should become

exec: some command with arguments

For example the peertube flist contains this file ssh.yaml that looks like this

exec: bash -c "/usr/sbin/sshd -D"

which can perfectly become

exec: /usr/sbin/sshd -D

Okay, I hope you know what the different is between the two, but I will explain anyway.
When u use bash, zinit will start a bash process, that then will run this script and then run the command. Now instead of have 1 process running, we end up with 2 processes running (forever)

When you check the process tree you will see something like

zinit (pid 1)
  -> bash (pid X)
        -> sshd (pid Y)

While if u use the correct way of executing a single command you will see

zinit (pid 1)
   -> sshd (pid X)

Now, when to use bash? You can ONLY use bash if you want to run some actual BASH code. in that case it becomes acceptable. It's also HIGHELY recommended that you do exec of your last command (your long running process) at the end of the script.

For example, this is the standard ssh.yaml I use in my images

exec: |
  /bin/ash -c '
    # override path for all ssh sessions
    export PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
    # generate host key if they donot exist
    /usr/bin/ssh-keygen -A
    # exec sshd
    exec /usr/sbin/sshd -D
  '
log: stdout

I here use the bash but see what I do.

  • I make sure the PATH is exported before i start sshd, so any ssh session connected to the daemon will inherit this env var.
  • I make sure i reun ssh-keygen -A wich will generate host keys if they don't exist already
  • I also make sure SSH_KEY from the env var is written to the authorized_keys file
  • Finaly instead of running /usr/sbin/sshd -D directly I do exec /usr/sbin/sshd -D what exec does (check man exec for more) is that it transfor the current process into sshd. in other words bash process sieze to exist and instead it becomse bash. So after running this if you try to check the process tree it will look like
zinit (pid 1)
   -> sshd (pid X)
  • finally the optional log: stdout makes sure that sshd logs actually go to zinit stdout which will make it endup in zos vm logs not in the ring buffer of zinit (hence you can't see it with zinit log but can help debugging a problem if u can't ssh to the node)
    • so it's up to u if u wanna use that or not

A better way to set PATH was to use the, so feel free to update this file

env:
   PATH: <path goes here>

So what to do:

  • Make sure bash is not in every zinit yaml file unless it's needed
  • sshd config (if needed) must be as above or similar, no need for other helper unit files to support

NOTE: we will update cloud-container to also generate host keys which means that this step will not be needed and hence the entire script will not be needed but instead we can just do:

exec: /usr/sbin/sshd -D
env:
  PATH: "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
log: stdout
@scottyeager
Copy link
Contributor

Nice ideas here to improve the image definitions and provide some style guidance for new contributions.

Generally I agree, but one question: why bother with exporting a generic PATH before starting sshd?

New ssh sessions are going to source default profile provided with the shell in any container image I've seen, thus providing the same outcome of generic PATH in the environment.

@hossnys hossnys mentioned this issue May 9, 2024
@xmonader xmonader modified the milestone: 3.15.x Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants