user state with groups always fails on initial run #4336

Closed
maspwr opened this Issue Mar 31, 2013 · 5 comments

Comments

Projects
None yet
3 participants
Contributor

maspwr commented Mar 31, 2013

If you had the following state:

maspwr:
  user.present:
    - groups:
      - sudo

and maspwr already existed with the following groups: maspwr, foobar. This state will fail on its first run.

The reason is that user.getent (which is called from the user state, https://github.com/saltstack/salt/blob/develop/salt/states/user.py#L54) seems to return a cached version, https://github.com/saltstack/salt/blob/develop/salt/modules/useradd.py#L174. If you rerun the state the changes to the groups (which were applied successfully) will be picked up and everything passes.

I believe the fix will be to remove the cached user.getent either conditionally or outright. Let me know if you need more info.

Member

terminalmage commented Mar 31, 2013

I'm pretty sure this was fixed in git a while back. Checking the source now.

Member

terminalmage commented Mar 31, 2013

Ahh, I fixed this in user.getent, not group.info (which is still returning data from __context__.

@thatch45 The issue here is that the data from the dunder context dict is being returned. This is a bad idea for states since we want to detect changes.

I removed the setting of the context variable for useradd_getent in a different issue a couple weeks ago. However, I do think that there is a good case to be made for putting user/group data in the dunder context dict. So, I was thinking of doing the following:

  1. Adding back the line that I removed in a558d84 which updates the __context__ dict.
  2. Add either a cache or nocache kwarg which can be used to tell user.getent and group.list_groups not to return the data from __context__. The default would be to use the value from __context__, if present.
  3. Modify the user and group states to pass this newly-added kwarg to calls to user.getent and group.list_groups, so that changes can be properly detected in states.

What do you think? If you like it, what would you like the kwarg to be?

Member

terminalmage commented Apr 1, 2013

Chatted with @thatch45 just a few mins ago, and there's a much simpler way. The data in __context__ for user.getent and group.list_groups can simply be cleared before the user or group state does that lookup, and this will make those functions retrieve the information again rather than returning the cached data.

terminalmage was assigned Apr 1, 2013

Contributor

maspwr commented Apr 1, 2013

Seems like that style could be prone to error, but y'all know the code base better than I. :)

Member

terminalmage commented Apr 1, 2013

Well, so would doing it the other way, since you'd have to override the default behavior of using the data from __context__. Either way, you're doing something to invalidate the cached data. Simply clearing that data is the path of least resistance, since it doesn't involve extra code being added to the useradd and groupadd modules.

thatch45 closed this Apr 4, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment