Tree permissions #644
Tree permissions #644
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a valuable feature to specify ownership and permissions for directory trees. The implementation is solid, with good refactoring to handle the new options and comprehensive tests. I have one high-severity suggestion regarding the handling of trees destined for the root filesystem directory (/) to prevent unintended and potentially dangerous modifications to the root directory's permissions. The documentation changes are thorough and accurately reflect the new functionality.
| if info.Mode().IsDir() { | ||
| return nil | ||
| // If nothing custom is required we skip directories generation | ||
| if options.dirMode == nil && options.user == (NodeUser{}) && options.group == (NodeGroup{}) { | ||
| return nil | ||
| } | ||
|
|
||
| if t.Exists(destPath) { | ||
| r.AddOnError(yamlPath, common.ErrNodeExists) | ||
| return nil | ||
| } | ||
| mode := util.IntToPtr(0755) | ||
| if options.dirMode != nil { | ||
| mode = options.dirMode | ||
| } | ||
| i, dir := t.AddDir(types.Directory{ | ||
| Node: createNode(destPath, options.user, options.group), | ||
| DirectoryEmbedded1: types.DirectoryEmbedded1{ | ||
| Mode: mode, | ||
| }, | ||
| }) | ||
| ts.AddFromCommonSource(yamlPath, path.New("json", "storage", "directories", i), dir) | ||
| if i == 0 { | ||
| ts.AddTranslation(yamlPath, path.New("json", "storage", "directories")) | ||
| } |
There was a problem hiding this comment.
The current implementation applies the tree's ownership and mode to the filesystem's root directory (/) if a tree's path is not specified (or is explicitly /). This can have unintended and potentially harmful side effects, as users likely intend to set permissions for the tree's contents, not for the filesystem root itself. I suggest skipping the directory creation for the root of the source tree walk when the destination is /. This will prevent modification of /'s permissions while still correctly applying permissions to all files and subdirectories within the tree. This will require updating the test cases that expect / to be modified.
if info.Mode().IsDir() {
// Don't apply tree ownership/mode to the filesystem root directory.
if relPath == "." && destPath == "/" {
return nil
}
// If nothing custom is required we skip directories generation
if options.dirMode == nil && options.user == (NodeUser{}) && options.group == (NodeGroup{}) {
return nil
}
if t.Exists(destPath) {
r.AddOnError(yamlPath, common.ErrNodeExists)
return nil
}
mode := util.IntToPtr(0755)
if options.dirMode != nil {
mode = options.dirMode
}
i, dir := t.AddDir(types.Directory{
Node: createNode(destPath, options.user, options.group),
DirectoryEmbedded1: types.DirectoryEmbedded1{
Mode: mode,
},
})
ts.AddFromCommonSource(yamlPath, path.New("json", "storage", "directories", i), dir)
if i == 0 {
ts.AddTranslation(yamlPath, path.New("json", "storage", "directories"))
}
}
|
These changes look really good, lets go ahead and squash the changes into a commit with this message |
docs/config-fcos-v1_1.md
Outdated
| * **target** (string): the target path of the link | ||
| * **_hard_** (boolean): a symbolic link is created if this is false, a hard one if this is true. | ||
| * **_trees_** (list of objects): a list of local directory trees to be embedded in the config. Ownership is not preserved. File modes are set to 0755 if the local file is executable or 0644 otherwise. Attributes of files, directories, and symlinks can be overridden by creating a corresponding entry in the `files`, `directories`, or `links` section; such `files` entries must omit `contents` and such `links` entries must omit `target`. | ||
| * **_trees_** (list of objects): a list of local directory trees to be embedded in the config. Ownership, file modes (using `file_mode`) and directories modes (using `dir_mode`) can be specified for the tree. If not specified, ownership is not preserved and file modes are set to 0755 if the local file is executable or 0644 otherwise. Attributes of files, directories, and symlinks can be overridden by creating a corresponding entry in the `files`, `directories`, or `links` section; such `files` entries must omit `contents` and such `links` entries must omit `target`. |
There was a problem hiding this comment.
This should not be changed, the feature is only for -exp spec, I feel like something went wrong when generating your docs.
vic1707
force-pushed
the
tree-permissions
branch
2 times, most recently
from
761c93d to
d9d10bc
Compare
|
Thanks for the review on this one, I just squashed everything. You're right about the docs, I blindly ran the generation script after the original CI failed, that's on me. Restoring the existing files (keeping only the My current best guess is that I modified shared files amongst all versions of butane, so docs generation modifies everything, not just unreleased versions hence CI fail. |
internal/doc/butane.yaml
Outdated
| desc: a list of local directory trees to be embedded in the config. Ownership, file modes (using `file_mode`) and directories modes (using `dir_mode`) can be specified for the tree. If not specified, ownership is not preserved and file modes are set to 0755 if the local file is executable or 0644 otherwise. Attributes of files, directories, and symlinks can be overridden by creating a corresponding entry in the `files`, `directories`, or `links` section; such `files` entries must omit `contents` and such `links` entries must omit `target`. | ||
| transforms: | ||
| - regex: Ownership is not preserved. | ||
| - regex: If not specified, ownership is not preserved |
There was a problem hiding this comment.
So to get around this, we can create a regex that removes bits we do not want for certain versions.. its regex so its not pretty buuuuut
- regex: ", file modes \\(using `file_mode`\\) and directories modes \\(using `dir_mode`\\) can be specified for the tree\\."
replacement: "."
if:
- variant: fcos
max: 1.6.0
- variant: fiot
max: 1.0.0
- variant: flatcar
max: 1.1.0
- variant: openshift
max: 4.20.0
- variant: r4e
max: 1.1.0
Should be what we want.
There was a problem hiding this comment.
I think we're good to go 👍
vic1707
force-pushed
the
tree-permissions
branch
from
d9d10bc to
8a09145
Compare
Add support for setting user, group, file_mode, and dir_mode on trees to address the use case of deploying directory trees with specific ownership for rootless containers. Fixes: coreos#544
vic1707
force-pushed
the
tree-permissions
branch
from
8a09145 to
61384e8
Compare
|
@vic1707 Thank you for implementing this! 🏅 |
3 participants
Fixes: #544
Added implementation for specifying custom modes (for files and dirs) as well as the user/group combe for ownership to trees entries