Skip to content

ENT-14297: Improve error message when cf-agent cannot write to an immutable file#6236

Merged
nickanderson merged 2 commits into
cfengine:masterfrom
nickanderson:ENT-14297/master
Jul 17, 2026
Merged

ENT-14297: Improve error message when cf-agent cannot write to an immutable file#6236
nickanderson merged 2 commits into
cfengine:masterfrom
nickanderson:ENT-14297/master

Conversation

@nickanderson

@nickanderson nickanderson commented Jul 14, 2026

Copy link
Copy Markdown
Member

Problem

When cf-agent cannot open a file for writing via the content attribute, the error gives no reason at all:

error: Cannot open file '/tmp/immutable.txt' for writing

The immutable bit (chattr +i) is frequently set deliberately to keep CFEngine from managing a file, and this generic message forces operators to investigate the underlying cause.

Change

The actual system error is now always reported, and when the file carries the immutable bit (and it is not being overridden) that is called out explicitly:

error: Cannot open file '/tmp/immutable.txt' for writing: Operation not permitted (the immutable bit is set)

Other failures now include the system error string, which the previous message omitted entirely:

error: Cannot open file '/etc/foo' for writing: Read-only file system

Jira: ENT-14297

@larsewi larsewi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only improves the message for when you are using the content attribute. Adding these checks everywhere would be quite tedious. Maybe it would be better to have OverrideImmutableBegin() take care of this logic?

@nickanderson

Copy link
Copy Markdown
Member Author

This only improves the message for when you are using the content attribute. Adding these checks everywhere would be quite tedious. Maybe it would be better to have OverrideImmutableBegin() take care of this logic?

WTH, not all my commits are here and I thought they were, sorry. will get them up later

@nickanderson
nickanderson marked this pull request as draft July 15, 2026 14:42
@nickanderson

Copy link
Copy Markdown
Member Author

Good call — the previous version only covered the content attribute.

I looked at generalizing via OverrideImmutableBegin(), but that helper is only used by the content path. The shared landing point for edit_line, edit_xml, copy_from and both template engines is OverrideImmutableRename() — they all write a temporary file via SaveAsFile() and rename it over the target — so I moved the detection there. That covers all of them in one place without repeating the check at each call site.

The content path writes in place rather than renaming, so it keeps its own detection at the fopen() failure. Two points total:

  • WriteContentFromString() — the in-place open (content)
  • OverrideImmutableRename() — the temp→target rename (edit_line, edit_xml, copy_from, templating)

Both confirm the bit via FSAttrsGetImmutableFlag() rather than guessing from errno, and both are skipped when overriding (that path clears the bit and renames a temp copy into place).

Added acceptance tests for edit_line and copy_from (15/16) alongside content (14). Verified end-to-end against a real immutable file (KVM VM, ext4 root):

content:   Cannot open file '/c.txt' for writing: Operation not permitted (the immutable bit is set)
edit_line: Failed to replace original file '/e.txt' with copy '...': Operation not permitted (the immutable bit is set)
copy_from: Failed to replace original file '/d.txt' with copy '...': Operation not permitted (the immutable bit is set)

…-14297)

Cover every path where cf-agent can fail to write a file because the
immutable bit is set and is not being overridden, asserting that the
error names the immutable bit:

  - content           (14, fails at the in-place open)
  - edit_line         (15)
  - copy_from         (16)
  - mustache          (17)  fail at the temporary-file rename in
  - cfengine template (18)  OverrideImmutableRename()
  - edit_xml          (19)
  - inline_mustache   (20)

Each test drives a self-contained sub policy in its own agent process:
the sub creates the target, sets the immutable bit via body fsattrs, and
then attempts the modification. The test asserts on the captured output
using native functions only (execresult_as_data + regcmp) and never fails
on the sub's non-zero exit. Teardown clears the immutable bit natively.
…4297)

When cf-agent could not write a file whose immutable bit is set, the
error gave no reason. Now the actual system error is always reported, and
the immutable bit is called out explicitly when it is set (and not being
overridden):

    error: Cannot open file '/tmp/x' for writing: Operation not permitted (the immutable bit is set)
    error: Failed to replace original file '/tmp/x' with copy '...': Operation not permitted (the immutable bit is set)

The immutable annotation is confirmed via FSAttrsGetImmutableFlag(), not
guessed from errno (an immutable file typically fails with EPERM, but
that is not exclusive to immutability), so other causes (permissions, MAC
policy, a read-only or full filesystem) are never mislabeled.

The check lives at the two points where the write actually fails: the
in-place open in WriteContentFromString() (content), and the temporary-
file rename in OverrideImmutableRename(), which is the common landing
point for edit_line, edit_xml, copy_from and templating. It is skipped
when overriding the immutable bit, since then the agent writes to a
temporary copy that is renamed into place after clearing the bit.
@nickanderson
nickanderson force-pushed the ENT-14297/master branch 2 times, most recently from b9cab70 to 4d490e5 Compare July 17, 2026 04:33
@nickanderson
nickanderson marked this pull request as ready for review July 17, 2026 06:11
@nickanderson
nickanderson requested a review from larsewi July 17, 2026 06:11
Comment on lines +224 to +225
&& (FSAttrsGetImmutableFlag(new_filename, &is_immutable) == FS_ATTRS_SUCCESS)
&& is_immutable)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if you could have used new_is_immutable from above in this condition instead?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good instinct to reuse it, but new_is_immutable isn't valid in this branch: TemporarilyClearImmutableBit() returns early on !override without writing *was_immutable (and new_is_immutable is declared uninitialized), while this check runs specifically in the !override case — so reusing it here would read an uninitialized value. Keeping the explicit FSAttrsGetImmutableFlag() also keeps the extra stat on the rare rename-failure path rather than on every temp-file write. Happy to refactor TemporarilyClearImmutableBit() to always populate the flag if you'd prefer the reuse. (Analysis via Claude.)

@nickanderson
nickanderson merged commit 18b0f91 into cfengine:master Jul 17, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants