threefoldtech / tf-images

Apache License 2.0
1 stars 3 forks source link

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

Open muhamadazmy opened 1 year ago

muhamadazmy commented 1 year ago

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.

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:

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 commented 12 months ago

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.