Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add chmod to mkdir to ignore umask settings #405

Closed

Conversation

IronCore864
Copy link
Contributor

@IronCore864 IronCore864 commented Apr 2, 2024

Add a flag so that pebble mkdir won't be affected by umask settings.

Closes #372.

Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Left some comments. Happy to discuss further too.

I'm not 100% sure we should add the flag. Maybe we should just always chmod it? But there is a precedent for the flag in the AtomicWrite functions, so that's what made me think we should add it.

internals/osutil/mkdirallchown.go Outdated Show resolved Hide resolved
internals/osutil/mkdirallchown.go Outdated Show resolved Hide resolved
internals/osutil/mkdirallchown.go Outdated Show resolved Hide resolved
internals/osutil/mkdirallchown_test.go Outdated Show resolved Hide resolved
@IronCore864 IronCore864 marked this pull request as ready for review April 7, 2024 01:09
@IronCore864 IronCore864 requested review from benhoyt and tonyandrewmeyer and removed request for tonyandrewmeyer April 7, 2024 01:09
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Haven't reviewed in detail yet, but just a couple of comments that might unblock you.

internals/osutil/mkdirallchown.go Outdated Show resolved Hide resolved
internals/osutil/mkdirallchown_test.go Outdated Show resolved Hide resolved
internals/osutil/mkdirallchown.go Outdated Show resolved Hide resolved
@IronCore864 IronCore864 requested a review from benhoyt April 8, 2024 09:29
@benhoyt
Copy link
Contributor

benhoyt commented Apr 18, 2024

I've looked at this more closely -- sorry for the delay. The function signatures, as well as the number of functions in the implementation, is getting unwieldy. I think we should go back to the drawing board and design a better function. How about this: make the two required arguments positional (path and perm) and put everything else in an options struct (which can be nil or the zero value for defaults):

package osutil

func Mkdir(path string, perm os.FileMode, options *MkdirOptions) error { ... }

type MkdirOptions struct {
    // True to create any parent directories if they don't already exist.
    //
    // If false and path already exists, Mkdir returns an error that wraps os.ErrExist (like os.Mkdir).
    // If true and path already exists, Mkdir does nothing and returns nil (like os.MkdirAll).
    MakeParents bool

    // True to perform an explicit chmod on any directories created.
    Chmod bool

    // True to call chown on any directories created, using the user ID and group ID provided.
    Chown bool // NOTE: additional bool is needed because 0 is a valid value for UserID and GroupID (root)
    UserID sys.UserID
    GroupID sys.GroupID
}

I'm not sure about the "clever" MakeParents behaviour in the case where the path already exists. It imitates Go's Mkdir / MkdirAll quirks, but IMO that's a bit of a design flaw. Might be better to have an explicit ExistOK bool flag.

The above is somewhat modelled after https://docs.python.org/3/library/pathlib.html#pathlib.Path.mkdir

@IronCore864
Copy link
Contributor Author

Closing this PR for now, see the new PR which does the refactor and implements the feature here.

@IronCore864 IronCore864 deleted the make-dirs-permission-umask branch June 5, 2024 04:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pebble make-dirs permission is masked by pebble daemon's umask
2 participants