fix(image): fold permissions into the OS build id so they OTA - #208
fix(image): fold permissions into the OS build id so they OTA#208nicksinas wants to merge 2 commits into
Conversation
A rootfs change that only edits `permissions:` (users/groups/passwords)
builds fine but never lands over `avocado deploy` — the device logs
`OS already at target version, skipping OS bundle download` and no-ops.
AVOCADO_OS_BUILD_ID is a pure function of the rootfs rpmdb (uuid5 over
the sorted NEVRA set), and `permissions:` rewrites /etc/{passwd,shadow,
group} without touching the package DB. So an identical package set
produced an identical build id regardless of the auth-file bytes, and
the deploy-side gate short-circuited the update.
Fold a hash of the assembled auth files into the build id alongside the
NEVRA hash, via a shared `render_auth_files_hash` helper used by both the
rootfs and initramfs build scripts. The hash is `LC_ALL=C`-sorted so it
is independent of the order the permissions section appends entries in,
and tolerates a missing gshadow. Any permissions change now moves the id
and therefore OTAs; an unchanged block hashes identically, so there is no
spurious update churn.
Note: because the build-id formula changes, every existing image re-ids
once on the first rebuild after this lands, triggering a single OS OTA
even when nothing else changed. Steady state is churn-free thereafter.
mobileoverlord
left a comment
There was a problem hiding this comment.
Approach looks right and appropriately scoped — content hash folded in beside the NEVRA hash, one shared helper for both images, computed after the permissions section. Ordering checks out in both generated scripts, and the sysroot os-release gets the same id the manifest patcher reads back (runtime/build.rs:2455), so no split-brain. Migration note is accurate.
One blocking issue inline: the missing-file tolerance doesn't survive pipefail.
Two follow-ups filed rather than piled onto this PR:
- ENG-2440 —
overlay:contents aren't content-hashed by the install stamps, so an overlay edit leaves the sysroot stale and never reaches the image at all. Same blind spot, one layer earlier. - ENG-2441 — derive the id from the assembled work tree instead of an enumerated component list, on one shared rootfs/initramfs code path, so this doesn't need a third patch. Notes the
HashMap→BTreeMapdeterminism prerequisite (which is also what theLC_ALL=C sorthere is working around).
The `2>/dev/null` suppressed the message but not cat's exit status. Under
the `set -euo pipefail` used by the standalone `avocado rootfs image` and
`avocado initramfs image` scripts, a missing file (e.g. a minimal
initramfs with no /etc/shadow) failed the pipeline, the $(…) assignment
took that status, and set -e killed the build with no output at all.
Wrap the cat in a `{ …; } || true` brace group so a missing file no
longer fails the pipeline. Update the exact-output test and add a
pipefail-safety assertion.
There was a problem hiding this comment.
Pull request overview
This PR fixes ENG-2437 by ensuring permissions:-only changes (which rewrite /etc/{passwd,shadow,group,gshadow} without changing the rpmdb) still change the computed build IDs, so avocado deploy will no longer skip the OS/initramfs bundle download/apply when only auth files changed.
Changes:
- Added a shared helper
render_auth_files_hash(work_var, out_var)that emits a deterministic shell snippet to hash assembled auth files (tolerating missing files and pinning collation). - Updated both rootfs and initramfs build-id derivations to compute
uuid5(namespace, "$PKG_HASH:$AUTH_HASH")(with initramfs using its corresponding var names), keeping rootfs/initramfs logic aligned via the shared helper. - Added/updated unit tests to assert the auth hash snippet shape and verify the build-id input folds in the auth hash and is computed after the permissions section.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/commands/rootfs/image.rs |
Adds shared auth-file hash snippet helper, folds it into rootfs build-id derivation, and adds regression/unit tests for ordering and uuid5 input. |
src/commands/initramfs/image.rs |
Reuses the shared helper from rootfs to fold auth-file hashing into initramfs build-id derivation and adds a matching regression test. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jetm
left a comment
There was a problem hiding this comment.
Two blocking findings inline, both reproduced. The core intent (fold permissions into the build id so a permissions:-only change OTAs, per ENG-2437) is right and needed; the hash input isn't order-stable yet, and the id rotates unconditionally for every project regardless of whether permissions: is even set. Everything else - the four-file staging guard, the pipefail-safe { …; } || true wrapping, the passwd/shadow sed application - checked out clean (1446 tests pass, clippy/fmt clean, ran the rendered script directly under set -euo pipefail with files missing).
| /// the same way. | ||
| /// | ||
| /// `work_var` names the image work-dir shell variable (e.g. `ROOTFS_WORK`, | ||
| /// `INITRAMFS_WORK`). `LC_ALL=C sort` makes the hash independent of the |
There was a problem hiding this comment.
LC_ALL=C sort can't make this deterministic - the inputs it's sorting are already order-dependent. sort normalizes line order only. It can't normalize the uid/gid values baked into each line, or the member order within one /etc/group line. PermissionsConfig.users/groups are HashMap<String, serde_yaml::Value> (config.rs:840-841), and mapping_from_hashmap (permissions.rs:380) copies them via for (k, v) in map with no sort - Rust's HashMap iteration order is randomized per process. stamps.rs:1283-1292 documents exactly this failure mode for the same reason ("HashMap iteration order varies per process... so the keys have to be sorted here or the hash is unstable between runs") and sorts before hashing; mapping_from_hashmap doesn't.
Ran the real mapping_from_hashmap -> render_users_groups_script path in 4 separate processes with identical input: 3 distinct user-emission orders. For a user declared without an explicit uid: (optional per configs/default.yaml:82-86), that changes the assigned uid/gid, and for two users sharing a supplementary group, it changes the member order within one /etc/group line - both survive the line-level sort and change AUTH_HASH.
Before this PR the build id was PKG_HASH-only and stable across rebuilds. After, a rebuild that changed nothing in the config can OTA the whole fleet, contradicting "deterministic build ID" at line 138. Fix upstream of the hash - sort the keys in mapping_from_hashmap, the same way packages_for_hash already does.
| # OTAs (ENG-2437). See render_auth_files_hash. | ||
| {auth_hash_block} | ||
|
|
||
| OS_BUILD_ID=$(python3 -c "import uuid; print(uuid.uuid5(uuid.UUID('{namespace_uuid}'), '$PKG_HASH:$AUTH_HASH'))") |
There was a problem hiding this comment.
This rotates the build id for every project, even one with no permissions: block at all. The uuid5 input changes unconditionally from '$PKG_HASH' to '$PKG_HASH:$AUTH_HASH', and AUTH_HASH is the digest of the base packages' own passwd/shadow/group - never empty, even with zero permissions: config. The id is appended to os-release before mkfs.erofs runs (line 206 vs 220), so this changes the image bytes, sha256, image_id, spot hashes, and the re-signed AMF for every build across the merge boundary.
First build after this merges, zero config change, zero package change: rootfs + initramfs + os-bundle all re-upload and every device takes a full OS OTA for nothing. Worth a CHANGELOG rollout note - this repo already carries one for smaller changes (the avocado --version entry under [Unreleased]) - so downstream fleets know to expect one unscheduled full OTA rather than reading it as a regression.
ENG-2437
Problem
A rootfs change that only edits
permissions:(users/groups/passwords) builds fine locally but never lands on a device overavocado deploy. The device logs:…and no-ops.
Root cause
AVOCADO_OS_BUILD_IDis a pure function of the rootfs rpmdb —uuid5(namespace, sha256(sorted NEVRA of every installed RPM)). Thepermissions:section rewrites/etc/{passwd,shadow,group}in the image work dir but never touches the package DB, so an identical package set yields an identical build id regardless of the auth-file bytes. The deploy-side gate compares build ids and short-circuits both the download and the apply of the OS bundle, so the change silently never ships.(Same class of blind spot existed for the initramfs build id.)
Fix
Fold a hash of the assembled auth files into the build id alongside the NEVRA hash:
uuid5(ns, "$PKG_HASH:$AUTH_HASH")uuid5(ns, "$INITRAMFS_PKG_HASH:$INITRAMFS_AUTH_HASH")AUTH_HASHis produced by a new shared helper,render_auth_files_hash, that lives beside the existingrender_hook_block/resolve_install_hooks/render_users_groups_scripthelpers and is imported byinitramfs::image— so rootfs and initramfs derive their build id the exact same way, from one implementation.LC_ALL=C sortmakes the digest independent of the order the permissions section appends entries in (theusers:/groups:maps have no guaranteed iteration order), so it's fully deterministic.2>/dev/nulltolerates a missinggshadow(minimal images) — absent files contribute nothing rather than aborting.Why this doesn't cause spurious updates
The id is a content hash: an unchanged
permissions:block hashes identically → same id → no OTA. It moves if and only if the auth-file content actually changes. This is the minimal correct mechanism — the deploy gate reads nothing butAVOCADO_OS_BUILD_ID, so a permissions-only change has to move the id to land.One-time migration note
Because the build-id formula changes (
$PKG_HASH→$PKG_HASH:$AUTH_HASH), every existing image re-ids once on the first rebuild after this lands, triggering a single OS OTA even when nothing else changed. Steady state is churn-free thereafter.Tests
test_render_auth_files_hash_snippet— exact-output unit test on the pure helper + reuse under the initramfs var names.test_build_id_folds_auth_files(rootfs and initramfs) — structural + ordering assertions (auth hash computed after the permissions section, folded into the uuid5 input).cargo fmt/cargo clippyclean on the changed files.Scope / follow-up
This is the correctness fix only. A separate
--forceoverride foravocado deploy(an escape hatch for any future rootfs change that doesn't move the id) was intentionally split out into its own PR — this fix largely subsumes thepermissions:case, so--forceis a nice-to-have rather than a dependency.Fixes ENG-2437.