sous-chefs / users

Development repository for the users cookbook
https://supermarket.chef.io/cookbooks/users
Apache License 2.0
138 stars 217 forks source link

user resource invocation doesn't use manage_home correctly #349

Closed ghost closed 8 years ago

ghost commented 8 years ago

Cookbook version

3.0.0

Chef-client version

12.14.40

Platform Details

Amazon Linux AMI release 2016.03

Scenario:

Create a non-system user with a home directory.

Steps to Reproduce:

In a recipe:

users_manage 'admin' do
  action [:create, :remove]
end

admin group has a few users with ssh public keys. Run chef-client.

Expected Result:

Create /home/$user.

Actual Result:

Dies on trying to create $user/.ssh.

Suggested fix:

--- /var/chef/cache/cookbooks/users/providers/manage.rb.orig    2016-08-29 21:25:50.055575742 +0000
+++ /var/chef/cache/cookbooks/users/providers/manage.rb 2016-08-29 21:26:36.947092911 +0000
@@ -90,7 +90,7 @@
       password u['password'] if u['password']
       salt u['salt'] if u['salt']
       iterations u['iterations'] if u['iterations']
-      supports manage_home: manage_home
+      manage_home manage_home
       home home_dir
       action u['action'] if u['action']
     end
koertkuipers commented 8 years ago

i believe the issue is the chef-client version. i get the same issue with chef 12.14 but not with chef 12.12 so is this a bug in chef 12.14?

ghost commented 8 years ago

@koertkuipers the part that has me confused is that supports is used instead of manage_home. Does supports actually ensure that the home dir is managed/created if absent? It looks like it doesn't, in recent chef-client versions. My testing suggests that manage_home itself does. I thought supports is a provider attribute which signals that a given provider supports a given attribute/operation.

lamont-granquist commented 8 years ago

"supports" never made any sense and in 12.14 its a no-op and throws a deprecation warning that you should be seeing.

the fix appears to be correct.

ghost commented 8 years ago

Thanks for confirming, @lamont-granquist !

lamont-granquist commented 8 years ago

and "supports" should be saying "the linux useradd utility supports manage_home". the answer to that need to be 'true' because the linux useradd provider really does support manage_home. by setting this flag on the resource what users were saying is 'yeah, i know the linux useradd provider supports manage home, but i want to lie to you and say it doesn't' -- which is not quite the same as saying 'dont manage the homedir on this resource'. and its both important to think about those semantic differences, and at the same time when you really do it quickly gets a bit insane. the whole 'supports' thing makes a lot more sense on the service resource -- where a random given init script may 'support' the restart action or not and the chef user really knows this and needs to inform the service resource about the capabilities of the underlying service resource.

ghost commented 8 years ago

@lamont-granquist yeah, that's what I thought, too. It's kind of a weird override on that particular resource here, and doesn't do what we want.

lamont-granquist commented 8 years ago

it literally took me 2 weeks to think it through and track down where it got introduced (way back in chef 0.6.x times) and what the original intent looked like it was (to have one monolithic useradd provider with different supports flags that would be enabled and disabled based on the O/S) and how we had gone a completely different architectural direction over the years (the useradd provider became linux-specific and we wrote aix and solaris and other user providers instead of leveraging the 'supports' feature at all).

ghost commented 8 years ago

Gotcha.

So can my fix be applied, please?

tas50 commented 8 years ago

Related but unrelated statement: We should get a 12.1 test added to the suite so we make sure we're testing at least 1 OS on 12.1 and the others the latest.

unixorn commented 8 years ago

FWIW, I just ran into this today on Ubuntu 16.04.1 too.

tas50 commented 8 years ago

This has been fixed in the Users 4.0 cookbook. Give that one a try and you should be golden

ghost commented 8 years ago

@tas50 thanks!