(PUP-2867) Support ~ in file paths, allows to deploy to ~<user> directories

[raphink]

Note: validation will let ~ pass, but munging will fail if the user
does not exist since it calls File#expand_path

[puppetcla]

CLA signed by all contributors.

[adrienthebo]

@raphink thanks for this contribution! Is there an existing issue for this change?

[raphink]

Not that I know of. Shall I open one?

[adrienthebo]

That would be really helpful; our changelog is generated based on JIRA queries (I think) and it's nice to have all issues in a release associated with a ticket that describes the problem being fixed or feature being added.

[adrienthebo]

If ENV['HOME'] is not set and File.expand_path is given a path with a tilde it will fail hard (as is described in a similar issue, https://tickets.puppetlabs.com/browse/FACT-190), is this safe to do?

[raphink]

Yes, and while it's a problem in facter because it will raise a full Ruby stack and interrupt Puppet, I think it's OK in Puppet because it returns the error without a stack and makes the resource fail:

$ puppet apply user_home.pp 
Notice: Compiled catalog for obadiah.bouyguesbox.fr in environment production in 0.12 seconds
Error: Parameter path failed on File[~foo/bas/test.txt]: Munging failed for value "~foo/bas/test.txt" in class path: user foo doesn't exist at /home/raphink/bas/puppet/user_home.pp:3
Wrapped exception:
Munging failed for value "~foo/bas/test.txt" in class path: user foo doesn't exist

or would you rather still catch this?

[raphink]

Come to think of it, it actually prevents other resources from being applied, so it should probably be caught and send a clean Puppet::Error instead.

[raphink]

That said, it seems exec has the same kind of behavior when munging timeout: https://github.com/puppetlabs/puppet/blob/master/lib/puppet/type/exec.rb#L268

[adrienthebo]

So this raises a number of cases we have to worry about:

• Path is ~/path and ENV['HOME'] is unset, which will cause Puppet to crash when attempting to expand the path.
• Path is ~user/path but user does not exist at the time of the File resource application

If one of these conditions is satisfied, how should we fail?

In addition, this change isn't entirely truthful in how it modifies absolute_path?. ~user/path can be expanded to an absolute path, but it is not in and of itself an absolute path. If this is specifically for the file resource then I think that the changes need to be localized to the file resource, instead of affecting anything that could invoke absolute_path?.

[raphink]

~/path and ENV['HOME'] unset

When ENV['HOME'] is unset in Puppet 3.6.2, Puppet refuses to start:

$ HOME= puppet apply user_file.pp 
Error: Could not initialize global default settings: non-absolute home

I don't know if that's a known bug, but it certainly won't get to the absolute path check in this case.

~user/path and user does not exist on client

The current code will have munge fail with a clear error message:

$ puppet apply user_home.pp 
Notice: Compiled catalog for obadiah.bouyguesbox.fr in environment production in 0.12 seconds
Error: Parameter path failed on File[~foo/bas/test.txt]: Munging failed for value "~foo/bas/test.txt" in class path: user foo doesn't exist at /home/raphink/bas/puppet/user_home.pp:3
Wrapped exception:
Munging failed for value "~foo/bas/test.txt" in class path: user foo doesn't exist

Modifying absolute_path?

I've thought a bit about that too and wondered what an absolute path actually is. If an absolute path is a path that refers to an absolute location, i.e. a unique file, regardless of the $CWD, then ~<user>/path qualifies for this, and even ~/path, as it only depends on $HOME. These two paths refer to a unique file in the file system and do not depend on $CWD. The file might not exist, but then absolute_path? is not a garantee that the file exists, only that the syntax refers to an absolute location, which it does in both cases here.

[adrienthebo]

I disagree that ~user is an absolute path. If you change the passwd field /etc/nsswitch.conf then you can can a different expansion for ~user, so while you can do the expansion you are not guaranteed the same path every time.

[ferventcoder]

I agree with @adrienthebo - while this would be really awesome to get, with it being non-deterministic it could cause issues.

[raphink]

Ok. It would be too bad to have specific code for that in the file type
though. How about a wrapping method that would check for either
absolute_path? or beginning with ~?

[raphink]

Maybe something like

def expanded_absolute_path?(path)
  begin
    absolute_path?(File.expand_path(path))
  rescue
    false # or maybe raise the error so the type can catch it and fail cleanly?
  end
end

[raphink]

There: https://tickets.puppetlabs.com/browse/PUP-2867

[zaphod42]

I think that the general idea for the feature is sound, however this has some ramifications beyond the desired feature that need to be dealt with. Before this using File.open on a path that reported absolute_path?()==true was safe. After this, that is no longer a safe operation and instead even an "absolute" path needs to be expanded before it can be used. This possibly breaks a lot of assumptions in the code.

@raphink I think your proposal for the expanded_absolute_path? might be a good direction to head since that allows us to keep truly absolute paths separate from these environmentally determined paths (~ is only absolute given a particular set of environment variables). That then allows us to open up use of ~ only the in the situations where it is really wanted.

[adrienthebo]

@raphink does @zaphod42's comment give you direction to continue work on this?

[raphink]

Yes. I just need some time to refactor.

[adrienthebo]

@raphink any progress on this?

[raphink]

No, sorry. I haven't had time to spend on this recently.

[raphink]

I just added a Puppet::Util#absolute_expanded_path? method and associated unit tests, and used it in the file type.

[adrienthebo]

Couple of points - first off, could you squash the commits in this pull request into a single commit and amend the message to match our contributing guide (https://github.com/puppetlabs/puppet/blob/master/CONTRIBUTING.md#making-changes)? In addition it looks like the spec tests failed on Windows file paths: (https://travis-ci.org/puppetlabs/puppet/jobs/31379092#L243-L280), could you check those out?

[ferventcoder]

Should this be using the Puppet File System Abstraction?

[ferventcoder]

Concerned with how this would work with Windows...

[kylog]

@raphink arriving at this PR late but a couple questions/comments:

1. I share @ffrank's concern about the user experience for ~/. I.e., while I can see where this would be handy for the ~${owner} case, it may be opening the door for surprising behavior for ~/ users. Thoughts on how to remedy that?

2. Did we address @ferventcoder's question about how this would affect Windows platforms?

[kylog]

@raphink closing this PR for now. That doesn't mean we don't value the contribution but we weren't able to reach consensus on the best approach here and didn't have any updates for a few weeks.

However, please reopen this PR (or create a new one) if you have more comments or ideas on how to proceed. Thanks!