-
Notifications
You must be signed in to change notification settings - Fork 3.7k
fix(packaging): init.sh should run buildtsi as influxdb user #26699
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
Conversation
The init.sh script assumes that it being run as root so thus we can use sudo to switch user to the influxdb user to run buildtsi; otherwise the files are owned by root and influxdb can't start. * fixes #26698
su - influxdb -c "/usr/bin/influx_inspect buildtsi -compact-series-file \ | ||
-datadir "${DATA_DIR}" \ | ||
-waldir "${WAL_DIR}" | ||
-waldir "${WAL_DIR}"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The quoting seems off here. I think you want:
su - influxdb -c "/usr/bin/influx_inspect buildtsi -compact-series-file \
-datadir '${DATA_DIR}' \
-waldir '${WAL_DIR}'"
(this puts ${DATA_DIR}
and ${WAL_DIR}
in single quotes, which should be fine since you are double quoting the entire call to /usr/bin/influx_inspect buildtsi
under su
. I did not test this, but it is clear that you have nested double quotes (you could backslash the inner double quotes instead of using single quotes). Untested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the single quote around the vars will prevent their substitution so that's probably not it.
Lemme try it with runuser
instead. That seems to be available by default in my ubuntu container, but sudo isn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the single quote around the vars will prevent their substitution so that's probably not it
I was thinking it wouldn't cause this problem because at the time of the evaluation by the shell, ${DATA_DIR}
and ${WAL_DIR}
should be filled in for the su
invocation. Eg:
$ cat ./foo.sh
#!/bin/sh
set -x
SOMEVAR="here-i-am"
su - jamie -c "echo SOMEVAR='$SOMEVAR'"
$ sudo ./foo.sh
+ SOMEVAR=here-i-am
+ su - jamie -c echo SOMEVAR='here-i-am' # at invocation, $SOMEVAR is already evaluated and quoted
SOMEVAR=here-i-am
You could also backslash the inner double quotes to achieve a similar result:
su - influxdb -c "/usr/bin/influx_inspect buildtsi -compact-series-file \
-datadir \"${DATA_DIR}\" \
-waldir \"${WAL_DIR}\""
Lemme try it with
runuser
instead
su
is annoying and there are better tools, yes. I suggest setpriv
which is also from util-linux
if you don't want to use su
. runuser
uses the PAM stack where setpriv
does not.
-datadir "${DATA_DIR}" \ | ||
-waldir "${WAL_DIR}"" | ||
echo "Building tsi with influxd_inspect buildtsi." | ||
runuser -u influxdb -- /usr/bin/influx_inspect buildtsi -compact-series-file \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest setpriv
instead of runuser
as it doesn't go through the PAM stack (which you don't need). See man runuser
. Ie:
setpriv --reuid influxdb --regid influxdb --clear-groups /usr/bin/influx_inspect buildtsi -compact-series-file ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Jamie - please take another look.
see also: https://man7.org/linux/man-pages/man1/setpriv.1.html
We want to run a command as another user but su has a frustrating syntax for calling a command and escaping, runuser is simpiler but delegates to su so both use PAM which is not needed in this case. It was recommended to use setpriv which a full toolkit for setting privilege bits and can mimic su/runuser by setting privilege to a specific user for running a command. It seems to work and be easy to use in a script.
-datadir "${DATA_DIR}" \ | ||
# buildtsi prompts with a warning it is is run as root as the files it makes will be owned by root. | ||
# In that case, it awaits an interactive Yes but that can't be supplied. All around, best to run it | ||
# as the influxdb user. sudo is also an option but not as available as su |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: s/as su/as setpriv/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch - this comment had several issues; i improved it.
* fix(packaging): init.sh should run buildtsi as influxdb user The init.sh script assumes that it being run as root so thus we can use sudo to switch user to the influxdb user to run buildtsi; otherwise the files are owned by root and influxdb can't start. * fixes #26698 * chore: switch from sudo to su; update comment * chore: switch to runuser as the su syntax and debugging is difficult * chore: switch to setpriv to avoid PAM We want to run a command as another user but su has a frustrating syntax for calling a command and escaping, runuser is simpiler but delegates to su so both use PAM which is not needed in this case. It was recommended to use setpriv which a full toolkit for setting privilege bits and can mimic su/runuser by setting privilege to a specific user for running a command. It seems to work and be easy to use in a script. * chore: update comment to reflect setpriv usage (cherry picked from commit 7ca78d1)
…#26862) Co-authored-by: Phil Bracikowski <[email protected]> fixes #26698
The init.sh script assumes that it being run as root so thus we can use
sudo to switch user to the influxdb user to run buildtsi; otherwise the
files are owned by root and influxdb can't start.
buildtsi
which shouldn't be run with root #26698