Repository navigation
Welcome the operator once, as Keel - #9
marcos-mendez wants to merge 6 commits into
Conversation
Measured on the wordpress-demo container, 2026-09-28, an appliance login
printed the Keel mark, the appliance name and the addresses, and then:
Welcome to Wordpress-demo, TurnKey GNU/Linux 19.0 (Debian 13/Trixie)
...
TKLBAM (Backup and Migration): NOT INITIALIZED
To initialize TKLBAM, run the "tklbam-init" command to link this
system to your TurnKey Hub account.
Three faults in those lines: the operator is welcomed twice, by two
products; the second welcome names a distribution this image is not; and
it advertises a service decision 0002 deferred and this project is
decoupling from, so an operator who follows the instruction is sent
somewhere we do not go.
Where the fix had to live, read from the makefiles rather than assumed.
root.patched applies the common overlays, runs the common conf scripts
(conf/turnkey.d/motd, which writes those drop-ins, is one of them),
applies the common patches and removelists, then the product units, then
the product overlay, and only then the product conf scripts. So this
repository's overlay lands after the files it has to replace, and
conf.d/main runs after the overlay. An overlay entry can therefore add a
drop-in but cannot remove one, and removal is the whole point:
00-turnkey-sysinfo and 08-turnkey-confconsole go in conf.d/main, which
until now did nothing.
What the login prints instead:
- 01-keel-sysinfo keeps every fact the old block gave, from the same
command as before, and drops the backup client's block by the shape
of the output and not by the words that block uses today. The command
prints a header, a blank line, the table, and any tail after a second
blank line; the rule is that second blank line, so the next tail goes
too. A filter written against the word TKLBAM would have passed its
replacement through in silence, which is this issue all over again.
- In its place, one true line about backup: not configured, no backup
service yet, and where it will be configured when there is one.
Nothing that does not exist is advertised, and no URL that answers
404 is printed.
- 08-keel-confconsole says the same two things upstream's did, pointing
at this project's documentation of its own console. The command keeps
the name it has today. The tput emphasis is dropped: the console
surface of this appliance is plain ASCII with no escape.
- 07-check-inithooks and 10-nonpersistent-mode are kept untouched. Both
were read: one warns that the first boot hooks will run again, the
other that the system is running non-persistent, and neither names a
product.
- 00-keel-banner reads /etc/keel_version for the appliance name and the
version, falling back to /etc/turnkey_version. Its comment
anticipated that file; common now writes it.
The assertion that closes this is behavioural. Step 8 of the boot test
renders /etc/update-motd.d in the running container with run-parts, which
is what pam_motd does at an interactive login, and requires exactly one
welcome, that it names Keel, that the system information block still
carries the load, the memory, the processes, the swap, the usage of / and
an address, and that neither "turnkey" nor "tklbam" appears anywhere in
it. It reads the login, never a file (docs/traps.md, "Asserting the
configuration is not asserting the behaviour"). Run against the login of
a live appliance today it fails on both counts and says which; run
against the chain this commit produces, replayed in fab's order on a
scratch directory, it passes.
The boot test boots the layer the mirror serves, and that layer is older
than this commit, so the appliance job stays red until the core layer is
rebuilt and published. The verdict says exactly that when it fails.
Weakening it to make the gate green would produce a green gate over the
wrong login, which is the fault being fixed.
Everything is measured, 100 percent under kcov 43, threshold unchanged at
100: overlay/usr/lib/keel/motd.sh 58 of 58 lines and 39 bats tests,
overlay/usr/lib/keel/banner.sh 124 of 124 and 71, tests/lib/boot-test-lib.sh
167 of 167 and 48, conf.d/main 8 of 8, run by the same bats file against a
scratch drop-in directory. 158 tests in all. Every exit path is covered,
including the two ways conf.d/main fails the build: the library missing
from the chroot, and a Keel drop-in absent because the overlay did not
land.
Closes #7. Context and the two decisions it forced: Keel-Linux/tracker#6,
handbook decision 0014.
|
Checks as expected.
That is the assertion working on a real appliance. It goes green when the core layer is rebuilt from this branch and published; I have not built or published anything. |
|
Review of the change set as one piece (this, Keel-Linux/common#5, handbook #6 and #4). They are not separable in the direction that matters: keel-core#9 reads What I verified by running rather than reading, so the findings below can be weighed against it:
HIGH1. The gate now requires an IPv4 address, on a distribution that is IPv6 first. 2. The new verdict runs before MEDIUM3. The removal lives in one recipe, so a product built without a parent gets the drop-ins back and nothing says so. 4. The two login drop-ins take their library path from the environment, and the drop-ins run with the PAM stack's privileges. 5. The documented build order is wrong in one position, in three places that will be read as authoritative. LOW6. 7. 8. 9. Note. The rendered login has two consecutive blank lines between the backup block and the confconsole block, because WarningNothing here is a correctness defect in what the operator sees: the login this branch produces is right, and I confirmed it by replaying the chain rather than by reading the fixture. HIGH 1 and HIGH 2 are both about the gate rather than the image, and both are the kind of thing that gets a verdict weakened six weeks from now by somebody who was not in this conversation. Worth fixing before merge; neither blocks. |
…verdicts Review of #9 found two ways the new gate could be wrong about a machine that is right, and one place where the change documented fab's order incorrectly. None of them changed what an operator sees. **The gate required an IPv4 address on a distribution that is IPv6 first.** BT_MOTD_FIELDS listed "IP address" among the facts the login must carry. The system information command reports an address per interface from netinfo.InterfaceInfo.address, which is SIOCGIFADDR on an AF_INET socket, so on a machine with no IPv4 it prints the single row "Networking not configured" and those words never appear. tests/instance.yaml declares an IPv6-only appliance; the gate passed only because lxcbr0 also hands out IPv4. An IPv6-only bridge, or a real IPv6-only appliance, would have failed step 6 naming a field the operator never lost, and the pressure then is to weaken the verdict. Split in two. The five labels the command always prints stay required. The address row is checked by shape, either form of it, so the block is still required to report on the network without requiring a family. And what the issue actually asks for, that the operator is told how to reach the machine, is now asserted against the machine: bt_motd_verdict takes the address the container answered on and requires the login to carry it. The banner prints IPv6 first, so an IPv6-only appliance passes. That is a stronger assertion than the one it replaces, not a weaker one, and a fixture of an IPv6-only login holds it open. **The login verdict ran before keel diff, so a deliberately red assertion hid a green one.** Under set -euo pipefail the failing verdict exited the script and the drift check never ran, for the whole interval the login check is waiting on a layer rebuild. The drift check was this job's only behavioural assertion before the login one existed. Both are now collected with "|| rc=$?" and reported together by bt_checks_verdict, which names every check that failed rather than stopping at the first, and calls no check at all a failure rather than a pass. **The build order was documented wrong in one position.** It was read from a stock tkldev's /usr/share/fab/product.mk rather than from the build host's, and this project's fab (1.1.1+keel1) orders the unit phases differently. Read from root.patched/body on the build host, and replayed with make -n against that file with a unit present, the order is: common overlays, common conf scripts, common patches, unit overlays, unit conf scripts, unit removelists, common removelists, the product overlay, the product conf scripts, the product patches, the product removelist, the initramfs, the common removelists-final, then root.patched/post. Nothing in the change depended on the wrong position, and the two facts it rests on are unchanged, but a recipe with units would have been read against it: unit conf scripts run before the product overlay, not after. Corrected in conf.d/main, COVERAGE.md and the changelog, each of which now says which fab it was read from. Also from the review: - The three login drop-ins write out the path of the library they source instead of taking it from the environment. pam_motd runs them with the privileges of the PAM stack and a sourced path is executed, not read; one AcceptEnv or pam_env line away, a login would source a file the caller named. No test needs the override there, and conf.d/main keeps it, because that runs in a chroot at build time and is where it is used. This closes the same shape in 00-keel-banner, which had it before this branch. - keel_motd_system_block counts blank lines from the first line that has something on it. Counting from the first line read made a single blank in front of the header the first blank and dropped the whole table; the drop-in discards stderr so nothing can inject one today, but now "the blank line that ends the table" is true rather than true by luck. - A machine with no system information command says so instead of printing nothing. The command is not named in that line: its name is a separate decision and printing today's would put another product's name back in the login. - conf.d/main and COVERAGE.md record the limit of pruning in one recipe. bt-layer subtracts the parent's common_conf from the child's, so no appliance built on core re-runs conf/turnkey.d/motd and core's removals are inherited by every layer above it; a plain make of a non-core recipe writes the drop-ins again and needs the same two calls in its own conf.d. - tests/coverage.sh runs each bats file once, however many files that file measures, and reports a verdict per file out of kcov's own per-file entries. motd.bats was running twice for no extra coverage. Still 100 percent, threshold unchanged at 100, 174 bats tests: boot-test-lib.sh 197 of 197, banner.sh 124 of 124, motd.sh 62 of 62, conf.d/main 8 of 8. The chain was replayed again on a scratch directory with the library paths written out, once with an IPv4 address and once with "Networking not configured", and the verdict passes on both.
The address assertion added for the IPv4 finding took the first global IPv6 lxc-info reported and required the login to carry that one. A machine can hold several, a SLAAC address and a privacy one or a static and a dynamic, and which of them the banner shows is the banner's own rule: keel_banner_pick_ipv6 takes static before dynamic and privacy last, while lxc-info lists them in whatever order it has. On a bridge that hands out more than one the two would disagree and the gate would fail a machine that is right, which is the fault the review found in the first place. bt_global_ipv6_all and bt_container_ipv6_all print every global address, and bt_motd_verdict takes them all and is satisfied by any one, naming all of them when none appears. It passed on the CI bridge with the single address it hands out; this is for the bridge that hands out two. 214 of 214 lines, 65 bats tests in that file and 179 in all, everything still at 100 percent.
|
Both HIGH findings and all three MEDIUM are addressed, plus LOW 7, LOW 6 and LOW 9. Three commits on top of the reviewed one; nothing the operator sees changed. HIGH 1, the IPv4 assumption. Correct, and it would have bitten the moment the bridge lost IPv4. Split in two: the five labels the command always prints stay required; the address row is checked by shape and accepts either HIGH 2, the short circuit. Both verdicts are collected with The drift check now reports while the login check waits for the layer. MEDIUM 5, the build order. You are right and I had read the wrong file: a stock tkldev's MEDIUM 4, the sourced library path. Written out in all three login drop-ins, including MEDIUM 3, one recipe prunes. Not solvable inside this repository without either touching every recipe or editing common's conf script, which would cost the upstream-offerable posture you noted. Written down instead, in LOW 7. The count starts at the first line with something on it, so a leading blank costs nothing. LOW 6. A machine with no system information command now prints LOW 8 is left:
|
|
Re-review of the fixes at HIGH 1, the IPv4 assumption: fixed, and the second-degree finding is rightThe split is the correct one. The five labels in Moving "the operator is told how to reach the machine" onto the address the container actually answers on is a better assertion than the one I asked for, because it is about the machine and not about a word. The reasoning behind collecting every address holds, and it is latent rather than live. HIGH 2, the short circuit: fixed, confirmed in CIRun 36381004119 shows both verdicts reported. The login check fails, and the job continues to Also visible in that log, and worth recording: the two new assertions both pass on the published layer ( MEDIUM 5, the build order: root cause verified, and it is a trapThe explanation is exactly right and I confirmed both halves. The local The corrected order in MEDIUM (new): the correction names the wrong fab version, and it is the one without the documented order
MEDIUM 4: fixed, and the distinction is the right lineAll three library paths are hardcoded now, The data-versus-code distinction is sound and I would draw it the same way. Sourcing a path executes whatever it names with the privileges the PAM stack has; reading MEDIUM 3: recording it is enoughThe note in LOWs6 is fixed well: 8 can wait. Tests179 tests pass. ApproveNo CRITICAL and no HIGH. Both HIGH findings are fixed and I confirmed each against something that runs rather than against the diff; the one new MEDIUM is a version string, and the two deferrals are recorded with their reasons in the places that will be read. |
The correction to the build order named fab 1.1.1+keel1 in conf.d/main, COVERAGE.md and the changelog. The build host runs 1.1.1+keel2, and the unit phases are keel2's: 1.1.1+keel1 is b07a733, which touches only debian/changelog and the build dependencies, and the reordering arrives afterwards through be8ef5b, "Apply the units before the common removelists", and 29a9462, "Let a unit carry a removelist". So the documented order is right and the version beside it named the one release that does not have it, which is the same defect the correction exists to prevent: a reader who checks against 1.1.1+keel1 finds the note wrong exactly as the first version was. Verified on the build host rather than inferred: dpkg reports fab 1.1.1+keel2, /usr/share/fab/product.mk is md5 a06bfe03 and is identical to share/product.mk at the fab checkout's HEAD, b07a733's copy of that file is md5 c04cb601 and contains no unit phase at all, and the local tkldev container that produced the original wrong reading has stock 1.1.1, md5 0657df1a. All three places now name 1.1.1+keel2 and give the md5, so the next reader can check the file in front of them rather than trust a version string, and each says the reading has to come from the machine that builds. Coverage unchanged: 214/214, 124/124, 62/62, 8/8, all 100 percent, 179 bats tests.
|
The new MEDIUM is fixed in Verified on the build host rather than taken from the review. All three places now say The trap is written up as its own pull request in the handbook, Keel-Linux/handbook#13, closing Keel-Linux/handbook#12, with the three files and their md5 sums in a table so the check is one line rather than an instruction to be careful. MEDIUM 3 now has its follow-up: Keel-Linux/tracker#16, naming the ten recipes and the two calls each needs, and recording why it should not block this.
|
The appliance check boots the layer the mirror publishes, never this branch (Keel-Linux/.github#12 renamed it boot-published-layer for that reason). The published core is product_commit 53d9fb5, which predates the login change, so the login verdict failed on every run, by design. Why that cannot work: the check is required and strict on master, the layer can only be rebuilt and published from master after a merge, and so no merge could ever turn it green. A required check that nothing can satisfy is a lock, not a gate. Whether it can be made to work as it was: only by publishing a layer built from an unmerged branch, which is the thing publishing from master exists to prevent. Why this is better: the login is checked on every layer that can answer the question and on no other. A layer built with the change is recognised by /usr/lib/keel/motd.sh, which conf.d/main refuses to build without and which the login drop-ins source, so its absence means an older layer and never a regressed one. On such a layer the check says it did not apply and names the remedy; it is not counted as a pass, and a run in which no check applied fails. On a layer that has the file, a wrong login fails exactly as before, and its message now says the change is not working rather than blaming the layer's age.
|
It was red because the appliance check boots the published core ( |
1 participant
Closes #7. Org-wide context and the two decisions it forced:
Keel-Linux/tracker#6 and handbook decision 0014
(Keel-Linux/handbook#6).
Measured on the
wordpress-democontainer, 2026-09-28, a login printedthe Keel mark, the appliance name and the addresses, and then:
Two welcomes, the second naming a distribution this image is not, and an
instruction to register the machine with a service decision 0002
deferred and this project is decoupling from.
Where the fix had to live
Read out of the makefiles with
make -n, not assumed.root.patchedapplies the common overlays, runs the common conf scripts
(
conf/turnkey.d/motd, which writes those drop-ins, is one of them),applies the common patches and removelists, then the product units, then
the product overlay, and only then the product conf scripts.
So this repository's overlay lands after the files it has to replace,
and
conf.d/mainruns after the overlay. An overlay entry can add adrop-in but cannot remove one, and removal is the point:
00-turnkey-sysinfoand08-turnkey-confconsolego inconf.d/main,which until now did nothing.
What the login prints instead
01-keel-sysinfokeeps every fact the old block gave (load, memory,processes, swap, usage of
/, an address per interface) from the samecommand as before, and drops the backup client's block by the shape
of the output, not by the words that block uses today. The command
prints a header, a blank line, the table, and any tail after a second
blank line; the rule is that second blank line, so whatever replaces
TKLBAM upstream is cut too. A filter written against the word TKLBAM
would have let the next version of this same mistake through in
silence.
service yet, and where it will be configured when there is one.
Nothing that does not exist is advertised and no URL that answers 404
is printed.
08-keel-confconsolesays what upstream's said, pointing at thisproject's documentation of its own console. The command keeps the name
it has today. The
tputemphasis is dropped: this appliance's consolesurface is plain ASCII with no escape.
07-check-inithooksand10-nonpersistent-modeare kept untouched.Both were read first: one warns that the first boot hooks will run
again, the other that the system is non-persistent. Neither names a
product.
00-keel-bannerreads/etc/keel_versionand falls back to/etc/turnkey_version. Its comment anticipated that file;Write /etc/keel_version beside the compatibility file common#5 writes it.
Rendered from the real chain, replayed in fab's order on a scratch
directory:
The assertion that closes this is behavioural
Step 8 of the boot test renders
/etc/update-motd.din the runningcontainer with
run-parts, which is what pam_motd does at aninteractive login, and requires exactly one welcome, that it names Keel,
that the system information block still carries every field, and that
neither
turnkeynortklbamappears anywhere in it. It reads thelogin, never a file (
docs/traps.md, "Asserting the configuration isnot asserting the behaviour"). Run against the login of a live appliance
today it fails and names both faults; run against the chain this branch
produces it passes.
The login check applies only to a layer built with this change. The
appliance check (
appliance / boot-published-layersinceKeel-Linux/.github#12) boots the layer the mirror serves, whose
product_commitis53d9fb5, older than this branch. As first written,the login verdict failed on it every time; since the check is required
and strict on master and the layer can only be rebuilt from master after
a merge, nothing could ever have turned it green. The boot test now asks
the login check only of a layer carrying
/usr/lib/keel/motd.sh, whichconf.d/mainrefuses to build without: on an older layer it reports thatit did not apply and names the remedy (rebuild and publish), without
counting as a pass; on a layer that carries the change a wrong login
still fails. The first core layer published after the merge is where the
login is checked for real.
Tests
100 percent under kcov 43, threshold unchanged at 100:
overlay/usr/lib/keel/motd.shoverlay/usr/lib/keel/banner.shtests/lib/boot-test-lib.shconf.d/main158 tests in all.
conf.d/mainis measured by running the conf scriptitself against a scratch drop-in directory, so both ways it fails the
build are covered: the library missing from the chroot, and a Keel
drop-in absent because the overlay did not land.
Test plan
bats tests/boot-test.bats tests/banner.bats tests/motd.batsCOVERAGE_THRESHOLD=100 tests/coverage.shshellcheck -S warningclean on every changed shell fileproduct.mkandmk/turnkey.mkwithmake -n, and the whole chain replayed on a scratch directory:the common conf script's own text, then this overlay, then
conf.d/main(fails, names both faults) and against the chain this branch
produces (passes)
appliance / boot-published-layergreen on the published layer,with the login check reported as not applicable to it
published after the merge