Preserve attributes in syncfiles without disturbing parent directories - #264
Merged
Merged
Conversation
The rsync push carried no preservation flags, so files arrived with their special permission bits explicitly disabled and a setuid/setgid entry could only be honored by the permissions= chmod on the client side. Preservation was turned on once before in e52a9ff ("Have syncfiles attempt to preserve more") and rolled back the same day in c0287e9 ("Roll back rsync ownership"), because rsync also applied the staging copy's attributes to the parent directories it merely traversed on the way to the synced files, clobbering the permissions of system directories such as /etc. Naming every staged file explicitly through --files-from and adding --no-implied-dirs confines preservation to the content actually being synchronized, leaving traversed directories alone and creating missing ones with default attributes. Two details follow from the way the staging tree is built. Files are staged as symlinks, so rsync reads their attributes through to the real file, but directories are staged as directories and need the source attributes copied onto them for the otherwise empty ones that have to be named explicitly. Ownership is mapped from the account the daemon runs as to root, since that account generally does not exist on the node and would otherwise arrive as a meaningless numeric id. --xattrs from that earlier attempt is deliberately left out: with --copy-links rsync reads xattrs off the symlink rather than its referent, so it transfers nothing here while adding a failure mode on hosts without xattr support.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The syncfiles rsync push carried no preservation flags:
Without
-p, rsync explicitly disables setuid/setgid/sticky on new files, so a syncfile entry could never delivera setuid binary, and existing files on the node kept whatever mode they already had.
Why the flags could not just be added back
It was tried once in
e52a9ff7("Have syncfiles attempt to preserve more") and rolled back in
c0287e93:"Unfortunately, it will try to change parent directories on the way to the files actually being written."
The staging tree mirrors the full destination path, so
-rwalks it as real directory entries and rsync appliedthe staging directory's attributes to the real
/etcon the node. Naming every staged file through--files-fromplus
--no-implied-dirsconfines preservation to the content actually being synchronized:What changed
--files-from=-on stdin. Otherwise empty directories are namedexplicitly, since nothing else would pull them across.
util.check_outputgained a keyword onlyinput=, mirroringsubprocess.run.stdinis only piped wheninputis given, so existing callers are unaffected.mkpathorlinkcopies the source directory's attributes onto the staged copy. Files are staged as symlinks andrsync reads through to the real file, but directories are staged as directories.
--usermap/--groupmap, since it normally does notexist on the node. Every other owner still matches by name, so this is not a blanket
--chown.--xattrsfrom the earlier attempt is left out: with--copy-linksrsync reads xattrs off the symlink, not itsreferent, so it transfers nothing here while adding a failure mode on hosts without xattr support.
Warning
This changes existing syncfiles behaviour. Attributes that used to be left alone are now taken from the
source on every sync:
chmodon a synced file survived. It is now reverted on the next sync.root:rootregardless of the source. A fileowned by a named account now arrives owned by that account, matched by name, falling back to the server's
numeric id when that name does not exist on the node. Only the confluent account is mapped to root.
-t,-A), where previously they were not.Directories that are only traversed on the way to a synced file remain untouched, which is what the second test
table below covers.
Testing
End to end against a deployed AlmaLinux 10 node, same fixture before and after.
Files and directories delivered by the sync:
/opt/sync/sub/setuidapp4755 root:root7554755 root:root/opt/sync/sub/setgidapp2755 root:root7552755 root:root/opt/sync/sub/tightmode0640 root:confluent666, stale mode kept640 root:root/opt/sync/sub/cflowned0640 confluent:confluent640 root:root640 root:root/opt/sync/sub/daemonowned0644 daemon:daemon644 root:root644 daemon:daemon/opt/sync/emptydir2750 confluent:confluent700 root:root2750 root:rootDirectories the sync only has to traverse to reach those files. Nothing in the syncfile list refers to them, and
both before and after they must come out exactly as the node had them:
/opt/sync)700 root:root700 root:root700 root:root/opt/sync/sub)750 daemon:daemon750 daemon:daemon750 daemon:daemonThe second table is the point: files were written into the subdirectory, yet neither its mode nor its ownership
moved. In the first table, the system user file keeping
daemon:daemonwhile the confluent owned file becameroot:rootconfirms the ownership mapping is selective.