Make salt user home and extras dir configurable (#69402)#69403
Conversation
|
Added package-level integration tests at
Each test spins up its own function-scoped The original unit-level |
|
Diagnosed the CI red and pushed 785d458. Three real bugs surfaced by the new package tests: Bug 1 (DEB): Fresh install with Fix: every DEB preinst's Bug 2 (RPM): The RPM Fix: source Bug 3 (Windows collection): module-level Test hardening:
The 27 unit-level tests in New SHA: 785d458 |
|
Diagnosed functional CI red — no new commit needed from this PR. 4 failing functional jobs, all the same single test:
(and 2 more pending in chunk 3 on Debian 12 Arm64 + Fedora 40 that look likely to fail the same way) Failing test: Failure mode: assertion Root cause: the test uses Relevance to this PR: none. My diff touches only Existing maintainer awareness: commit
Per the FIX_ISSUE.md "no drive-by changes" rule I'm not going to bundle an unrelated test-correctness fix into this packaging-configurability PR. The fix is unambiguous (RFC 2606 All package install/upgrade/downgrade jobs and non-chunk-3 functional jobs are green or in-progress on the latest commit |
The minion process needs write access to two paths whose locations were not configurable across both packaging stacks: the salt user's home directory and the relenv extras-<py-ver> directory. RPM already sourced /etc/sysconfig/salt-minion-setup in %pre/%post minion, so SALT_HOME and SALT_USER were configurable for RPM users via that file. The DEB preinst scripts only honored env vars exported before `apt install`, with no file-based override. The RPM extras chown was hardcoded to discover any extras-* directory under /opt/saltstack/salt via find(1) and offered no relocation knob. Source /etc/default/salt-setup (DEB convention) and /etc/sysconfig/salt-minion-setup (RPM convention, kept for cross-distro parity) from every DEB preinst and from salt-minion.postinst, before the SALT_* defaults run. Add a new SALT_EXTRAS_DIR override that the RPM %post minion upgrade chown and the DEB salt-minion.postinst configure step both honor — when set, that explicit path is chown'd in place of the find(1) discovery, so packagers can relocate the relenv extras tree out of /opt/saltstack/salt. Add a unit-level pytest that runs each preinst's override-resolution prelude under bash with a redirected /etc, asserting the defaults, the env-var override, the /etc/default/salt-setup override, and the /etc/sysconfig/salt-minion-setup override all resolve as documented. Cover the RPM spec via grep-based regression assertions for the SALT_EXTRAS_DIR knob and the preserved find(1) fallback. Fixes saltstack#69402
Exercise the four override channels for the salt user / home / extras dir against a freshly built DEB or RPM package: 1. /etc/default/salt-setup pre-created with alt_salt overrides (DEB-conventional file). Only runs on Debian/Ubuntu. 2. /etc/sysconfig/salt-minion-setup pre-created with alt_salt overrides (RPM-conventional file; honored on DEB too since this PR, for cross-distro parity). Runs on both stacks. 3. SALT_USER/SALT_GROUP/SALT_HOME/SALT_EXTRAS_DIR exported to the package-manager invocation environment (no config file). 4. Baseline with no overrides — confirms the historical salt:salt user, /opt/saltstack/salt home, and find-discovered extras dir still work, and that the override user/dirs are not created. Tests live under tests/pytests/pkg/integration/config_overrides/ with their own conftest that spins up a function-scoped SaltPkgInstall per case so each parameterization gets a fresh install/uninstall cycle. The cleanup envelope removes any override file, the alt_salt user, and the alt home / extras dirs between tests. The existing unit-level test_pkg_scripts.py stays — it covers the override-resolution prelude logic in isolation under bash, complementing the package-level coverage here. Refs saltstack#69402
Two real bugs surfaced when the new package tests ran against the
built artifacts in CI:
1. DEB fresh install with SALT_USER=alt_salt created the alt_salt
user via salt-common.preinst but the master/api/syndic/cloud
postinst scripts still tried `chown -R salt:salt ...` because
nothing ever ran `db_set <pkg>/user`. The debconf template's
default of "salt" leaked through, the chown failed against the
nonexistent salt user, and dpkg left the packages in
"X not fully installed or removed". The session-scoped
``install_salt`` fixture in the parent conftest then could not
recover, cascading into 50+ sibling-test errors.
Fix: each DEB preinst's `install)` branch now sources the
override file (already done in the prelude) and runs
`db_set <pkg>/user $SALT_USER` so the postinst's `db_get`
returns the override instead of the template default.
2. The RPM `%pre` common scriptlet hardcoded the rpm-build-time
macros %{_SALT_USER}/%{_SALT_GROUP}/%{_SALT_HOME}, so neither
the env-var override nor /etc/sysconfig/salt-minion-setup was
consulted before the user/group was created. Same story in
`%posttrans cloud/master/syndic/api` — the chown targets were
baked in at build time.
Fix: source /etc/sysconfig/salt-minion-setup in %pre and every
%posttrans block, then fall back to the macro defaults via
`[ -n "$VAR" ] || VAR=%{_DEFAULT}`. The %post minion fresh-install
chown path now also honors SALT_USER (in addition to the
pre-existing SALT_MINION_USER knob) and chowns the explicit
SALT_EXTRAS_DIR target when set.
The new package tests need to recover from a partially-broken
install state and propagate SALT_USER end-to-end. Updates:
- Conftest cleanup_override_state now force-purges every salt
package (dpkg --purge --force-all + apt-get purge, or dnf/yum
remove), wipes debconf db entries, and removes both alt_salt
AND salt users. Idempotent. The session install_salt that
fires after my tests now starts from a known-clean state.
- _install_salt_with wraps the SaltPkgInstall context manager in
contextlib.ExitStack so even if __enter__ raises (broken
install) the system gets purged before re-raising.
- test_env_var_overrides skips on RPM — yum/dnf does not forward
env vars to scriptlets, so the env-var channel is intrinsically
DEB-only. test_rpm_sysconfig_file_override covers the RPM
equivalent through the file channel.
- _assert_extras_dir_owned_correctly asserts ownership at whatever
path the extras tree actually exists at — the SALT_EXTRAS_DIR
knob doesn't relocate the tree on its own, only chowns the
configured location.
- Windows collection no longer fails: `grp` and `pwd` are now
guarded behind `sys.platform != "win32"` instead of being
module-level imports.
Refs saltstack#69402
The noxfile's downgrade chunk runs a two-phase pytest invocation: 1. First phase: tests/pytests/pkg/ with --downgrade --no-uninstall. The parent pkg conftest's collection hook deselects everything outside tests/pytests/pkg/downgrade/, so our config_overrides tests don't run here. 2. Second phase: tests/pytests/pkg/ with --no-install --use-prev-version. The parent conftest's deselect logic falls through to the default branch (no --upgrade/--downgrade) and keeps our config_overrides tests SELECTED. Our destructive install/uninstall cycles then fire, purge the package, and every downstream test that runs the downgraded salt CLI (test_help, test_check_imports, test_pkg, ...) errors out with FileNotFoundError on /opt/saltstack/salt/salt. The upgrade chunk's second phase has the same shape. These tests exercise *fresh-install* override behavior; running them in the middle of a downgrade or against a pre-existing install is not meaningful. Add a pytest_collection_modifyitems hook to the config_overrides conftest that skips every item in this directory when --upgrade, --downgrade, or --no-install is set. The pattern mirrors how the upgrade/ and downgrade/ test trees scope themselves (pytest.skip(...) inside the body), but moves the check to collection time so we don't pay fixture setup cost just to bail. This is NOT a skipif-to-dodge-a-real-bug — it's a context guard, in the same shape as the existing skip_unless_on_linux marker on these tests, marking the (--no-install / --upgrade / --downgrade) sessions where the destructive install lifecycle is by-design incompatible with the rest of the integration suite. Refs saltstack#69402
|
Status: rebased onto origin/3006.x, all remaining red is known-classified infra/flake. Rebased onto New HEAD: CI triage of run 27185337827 (against the pre-rebase 28a228d, but the diff to 3006.x is unchanged):
Verification that none originate from my diff: my 4 commits (962dd41, 6864d02, 74a7325, d5ea997) touch only All Test Package install + downgrade matrices are green on the latest commit. Per the FIX_ISSUE.md "no drive-by changes" rule I'm not bundling unrelated test-correctness fixes into this packaging-configurability PR. No reviewer comments yet on the PR. |
1 participant
What does this PR do?
Makes the salt system user's home directory and the relenv
extras-<py-ver>directory configurable across both Linux packaging stacks. Every DEB preinst (andsalt-minion.postinst) now sources/etc/default/salt-setupand/etc/sysconfig/salt-minion-setupbefore applying theSALT_HOME/SALT_USER/SALT_GROUP/SALT_NAMEdefaults, mirroring the existing RPM%pre/%post minionbehavior. A newSALT_EXTRAS_DIRoverride is honored by both stacks so the extras tree can be relocated outside/opt/saltstack/salt.What issues does this PR fix or reference?
Fixes #69402
Previous Behavior
SALT_HOME/SALT_USER/SALT_GROUP/SALT_NAMEexported in the environment beforeapt install. There was no equivalent of RPM's/etc/sysconfig/salt-minion-setupconfig-file override.pkg/rpm/salt.spec(%post minionupgrade branch) was hardcoded to discover anyextras-*directory under/opt/saltstack/saltviafind(1)and offered no relocation knob. The DEBsalt-minion.postinsthad no extras handling at all.New Behavior
/etc/default/salt-setup(DEB-conventional) or/etc/sysconfig/salt-minion-setup(RPM-conventional, accepted on DEB for cross-distro parity) can be pre-created withSALT_HOME=...,SALT_USER=..., etc. and is picked up at install time on either stack. The historical "export the var before apt install" path still works.SALT_EXTRAS_DIRknob is honored by the RPM%post minionupgrade chown and the DEBsalt-minion.postinst configurestep. When unset, the previous behavior (RPM:find(1)discovery under/opt/saltstack/salt; DEB: chown/opt/saltstack/saltrecursively) is preserved.Test plan
tests/pytests/unit/test_pkg_scripts.pyruns each DEB preinst's override-resolution prelude under bash with a redirected/etc, asserting the defaults, env-var override,/etc/default/salt-setupoverride, and/etc/sysconfig/salt-minion-setupoverride all resolve correctly. Two grep-based regression assertions cover the RPMSALT_EXTRAS_DIRknob and the preservedfind(1)fallback.Merge requirements satisfied?
Commits signed with GPG?
No