Skip to content

fix(executor): a config yip cannot read or cannot parse must not cost the directory the rest - #345

Open
ci-robbot wants to merge 2 commits into
mudler:masterfrom
ci-robbot:bot/4865-non-regular-config
Open

ci-robbot wants to merge 2 commits into
mudler:masterfrom
ci-robbot:bot/4865-non-regular-config

Conversation

@ci-robbot

@ci-robbot ci-robbot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Fixes kairos-io/kairos#4865
Fixes kairos-io/kairos#5382

One unreadable or unparseable file in a scanned directory must not cost that directory every other config. Two causes, same invariant, so they ship together.


2. A config that will not parse ends the walk (kairos-io/kairos#5382)

dirOps returned the schema.Load error straight out of the vfs.Walk callback. A non-nil return ends the walk, so no file after the broken one is read, and prepareDAG then threw away the ops already collected from the files that did parse:

ops, err = e.dirOps(stage, uri, fs, console)
if err != nil {
	return nil, err
}

So the directory was all-or-nothing. On Kairos that directory is /oem, and a single typo in one file left the node with no users, no passwords and no k3s config, while still booting. kairos-agent logged an error without naming the file; immucore discarded it entirely.

dirOps now names the file, logs it at error level and keeps walking, and carries the failure back next to the ops that did load. prepareDAG returns the graph and that error together, runStage runs the graph and appends the error, and Graph / Analyze keep showing the DAG instead of returning nil. A strict caller still fails on it.

How this half was tested

CI on 590d8a8 is green on all four legs, and the Executor Suite went from 19 to 21/21 specs, which is the two new ones. This box can no longer build yip offline (several modules renovate has since bumped are not in the local module cache), so what I could verify locally I verified by building the modified executor through kairos-io/kairos (whose module graph is complete) with a replace onto this worktree, and exercising the real code path:

  • a directory with 00-good.yaml + 99-bad.yaml: before, 00-good.yaml did not run at all; after, it runs and the error names 99-bad.yaml.
  • Graph over 01_first / 02_broken / 03_last: returns a non-nil DAG with 01_first before 03_last, plus the error.
  • unchanged paths: a missing path still errors, an all-broken directory still errors and names the file, an empty directory is still clean, a healthy directory still runs.
  • go test ./immucore/... green, agent/pkg/cloudinit and agent/pkg/config green. agent/pkg/utils (5 SyncData specs, no rsync on the box) and sdk/sysext fail identically with upstream yip v1.26.4.

1. A config that is not a regular file blocks the run (kairos-io/kairos#4865)

Summary

A FIFO named like a config in a scanned directory blocked the whole stage run. ReadFile opens without O_NONBLOCK, and opening a FIFO for reading blocks in open(2) until a writer arrives, which on a boot is never. On Kairos the stage runner parked in openat with wchan=wait_for_partner, kept its shutdown inhibitor, and the node could be neither reached nor rebooted until someone deleted the file from recovery media.

FromFile now opens with O_NONBLOCK and rejects anything the fstat says is not a regular file. That also covers a character device such as /dev/zero named like a config, where the read would never end either. O_NOFOLLOW is deliberately not set: a symlink pointing at a real config is a layout yip supports, and one pointing at a FIFO is caught by the same fstat.

dirOps resolves each entry (the walk hands it the Lstat) and skips a non-regular file with a warning rather than returning an error, so a single planted path cannot cost the directory every config next to it.

The same guard landed on the Kairos side in kairos-io/kairos#4869, which fixed kairos-agent config show and notify. QA on that PR found the boot still hung, because immucore's initramfs stages and the cos-setup-* units read the same directories through yip rather than through the collector. This is that second half.

Planting the file needs root-equivalent write access to a scanned path, so this is a robustness guard rather than a privilege boundary.

How it was tested

New specs in both packages, each running the call on its own goroutine with a 10s bound, because a regression here blocks forever and a plain call would hang the suite instead of failing it.

pkg/schema: a named pipe, a symlink to a named pipe, and a directory are all refused by name, a regular file and a symlink to a regular file still load.
pkg/executor: a stage directory holding a FIFO (and one holding a symlink to a FIFO) still runs the config next to it, and Run returns nil.

Counts, go test -count=1 on this branch against master at 3c7db49 in a throwaway worktree:

package master this branch
pkg/schema 11 of 11 15 of 15
pkg/executor 15 Passed, 2 Failed 17 Passed, 2 Failed

The 2 executor failures are Get Users and Deletes Users, which need root and fail identically on master.

Also run with -race, as CI does: no data races, same counts.

Regression proof: disabling only the dirOps skip and keeping the FromFile guard makes both new executor specs fail, which is what makes the second hunk load-bearing rather than belt-and-braces. Before the fix the schema path did not fail, it blocked: a throwaway probe showed Load still parked after 5s on a FIFO.

pkg/plugins is 84 Passed / 31 Failed here and identically on master: those specs need root.

A FIFO named like a config in a scanned directory blocked the whole
stage run. ReadFile opens without O_NONBLOCK, and opening a FIFO for
reading blocks in open(2) until a writer arrives, which on a boot is
never: the stage runner parked in openat with wchan=wait_for_partner,
kept its shutdown inhibitor, and the node could be neither reached nor
rebooted until the file was deleted from recovery media.

FromFile now opens with O_NONBLOCK and rejects anything the fstat says
is not a regular file, which also covers a character device such as
/dev/zero, where the read would never end either. O_NOFOLLOW is not
set, so a symlink to a real config still works and one pointing at a
FIFO is caught by the same fstat.

The directory walk resolves the entry and skips a non-regular file with
a warning rather than returning an error, so one planted path cannot
cost the directory every config next to it.

Fixes kairos-io/kairos#4865

Signed-off-by: Ettore Di Giacinto <mudler@kairos.io>
…parse

The same invariant as the previous commit, for a file the walk can read
but schema.Load cannot parse. Returning that error from the Walk
callback ends the walk, so no file after the broken one is read, and
prepareDAG then dropped the ops already collected from the files that
did parse. One typo in one config therefore cancelled every other
config in the directory, in lexicographic order or not.

On Kairos that directory is /oem, so a single bad file silently left
the node with no users, no passwords and no k3s config, and the node
still booted.

dirOps now names the file, logs it and keeps walking, and carries the
error back alongside the ops that did load. prepareDAG returns the
graph and that error together, runStage runs the graph and appends it,
and Graph and Analyze keep showing the DAG instead of discarding it.
A strict caller still sees the failure.

Fixes kairos-io/kairos#5382

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Ettore Di Giacinto <mudler@kairos.io>
@ci-robbot ci-robbot changed the title fix(schema): skip a config path that is not a regular file fix(executor): a config yip cannot read or cannot parse must not cost the directory the rest Oct 10, 2026
@ci-robbot

Copy link
Copy Markdown
Contributor Author

Pushed 590d8a8, which grows this PR by one commit: the same invariant for a file the walk can read but schema.Load cannot parse. Returning that error from the Walk callback ended the walk and prepareDAG then dropped the ops already collected, so one typo in one /oem file cancelled every other cloud config in that directory while the node still booted (kairos-io/kairos#5382).

Kept it here rather than in a second PR because it edits the same dirOps hunk this one already owns, and the two would conflict.

@mudler the design call worth a second opinion: the error is now reported and the configs that do parse still run, instead of failing the whole directory. That matches what kairos-agent already documents ("Cloud configs are being loaded and executed on a best-effort") and --strict still fails, but it is a behaviour change for anyone relying on all-or-nothing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants