pkg/rpm: look up current owner in %posttrans shell, not via a broken %global#68965
pkg/rpm: look up current owner in %posttrans shell, not via a broken %global#68965SAY-5 wants to merge 2 commits intosaltstack:3007.xfrom
Conversation
…a a broken %global
The %pre scriptlets for master, minion, syndic, cloud and api all do:
_MS_LCUR_USER=$(ls -dl /run/salt/master | cut -d ' ' -f 3)
_MS_LCUR_GROUP=$(ls -dl /run/salt/master | cut -d ' ' -f 4)
%global _MS_CUR_USER %{_MS_LCUR_USER}
%global _MS_CUR_GROUP %{_MS_LCUR_GROUP}
with the intent that the matching %posttrans scriptlet picks up those
values via %{_MS_CUR_USER}. RPM macro expansion runs at scriptlet
parse time, not at shell execution time, and the %global body is not a
shell context: %{_MS_LCUR_USER} is not a defined macro, so the
redefinition yields the literal string %{_MS_LCUR_USER}. In
%posttrans, chown is invoked with that literal and exits with:
chown: invalid user: '%{_MN_LCUR_USER}:%{_MN_LCUR_GROUP}'
which is exactly the failure reported on AlmaLinux 10.1 against
salt-minion-3007.11 when running `dnf -y reinstall salt-minion`
(salt#68646). On a reinstall or upgrade every chown -R in
%posttrans master, minion, syndic, cloud and api fails, which leaves
the resulting install with whatever ownership rpm picked during file
extraction (typically root) instead of the salt service user.
Drop the broken %global trick and compute the current owner inside
each %posttrans scriptlet, falling back to %{_SALT_USER} /
%{_SALT_GROUP} when the /run directory is absent. This runs in the
correct shell context on the target system, so the value survives to
chown and reinstall/upgrade flows restore the previous ownership as
intended. Fresh installs still go through the else branch with
%{_SALT_USER} and are unchanged.
Fixes saltstack#68646
|
Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here's some information that may help as you continue your Salt journey. There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar. |
| _MS_CUR_USER=$(ls -dl /etc/salt/cloud.deploy.d 2>/dev/null | cut -d ' ' -f 3) | ||
| _MS_CUR_GROUP=$(ls -dl /etc/salt/cloud.deploy.d 2>/dev/null | cut -d ' ' -f 4) |
There was a problem hiding this comment.
Using stat is preferable to trying to manually parse ls output with cut
_MS_CUR_USER=$(stat --format '%U' /etc/salt/cloud.deploy.d)
_MS_CUR_GROUP=$(stat --format '%G' /etc/salt/cloud.deploy.d)(use lowercase %u and %g for uid and gid instead of names)
There was a problem hiding this comment.
If we are only trying to use the user and group to set the permissions of other files with chown then the --reference argument to chown can just be used instead of doing extra manual lookups.
chown --reference /etc/salt/cloud.deploy.d -R /var/log/salt/cloud /opt/saltstack/salt/lib/python${PY_VER}/site-packages/salt/cloud/deploy…lookup bdrx312 on saltstack#68965 pointed out that shell-parsing ls is fragile (locale-dependent padding, extra-info rows) and that chown --reference reads the reference path's owner directly from stat(2). Replace the ls|cut lookups in the four %posttrans scriptlets (cloud, master, syndic/api, minion) with chown --reference against the same reference path each block already used for the ls lookup. The fresh-install branch is unchanged - it still uses the %{_SALT_USER}:%{_SALT_GROUP} macros, which are fine in that path. Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
|
@bdrx312 good point, thanks - |
2 participants
Fixes #68646.
Problem
The
%prescriptlets formaster,minion,syndic,cloudandapiall do:with the intent that the matching
%posttransscriptlet picks up those values via%{_MS_CUR_USER}. RPM macro expansion runs at scriptlet parse time, not at shell execution time, and the%globalbody is not a shell context:%{_MS_LCUR_USER}is not a defined macro, so the redefinition yields the literal string%{_MS_LCUR_USER}. In%posttrans,chownis invoked with that literal and exits with:which is exactly the failure reported on AlmaLinux 10.1 against
salt-minion-3007.11when runningdnf -y reinstall salt-minion. On a reinstall or upgrade, everychown -Rin%posttrans master,minion,syndic,cloudandapifails, which leaves the resulting install with whatever ownership rpm picked during file extraction (typicallyroot) instead of the salt service user.Fix
Drop the broken
%globaltrick and compute the current owner inside each%posttransscriptlet, falling back to%{_SALT_USER}/%{_SALT_GROUP}when the/rundirectory is absent. This runs in the correct shell context on the target system, so the value survives tochownand reinstall/upgrade flows restore the previous ownership as intended. Fresh installs still go through theelsebranch with%{_SALT_USER}and are unchanged.