[BUG] user.present does not leave existing groups alone when groups is unset
#64211
Comments
|
You are using You can't mix the methods of managing group membership. |
|
Though I see what you mean. Having |
|
Oh I think I see what's happening now... What's the right way to do this then? Seems like a common pattern. In my mind, a user's main gid is different than auxiliary groups (which is what I'm really trying to manage). There's no way to create a user account without a primary group. If I want a specific GID for a user's group, wouldn't it be needed during user creation? Or should I just manage the group and have that as a required item when managing the user account and let user.present implicitly figure out the gid from user I feel like no matter what I do, I'm getting into tricky implicit behavior. Either that, or I have to change the approach in which I manage groups and enumerate out all of the groups that any given user is a member of on any given machine (which may not be the same across machines) |
|
I tried managing the user's group prior to creating the user while not specifying a GID, and I couldn't get it to work. Example state fileManage group - test-user:
group.present:
- name: test-user
- gid: 61001
Manage user - test-user:
user.present:
- name: test-user
- uid: 61001
- fullname: Test User
- shell: /bin/bash
- home: /home/test-user
- password: $6$REDACTED
- require:
- Manage group - test-user
Manage group - test-group1:
group.present:
- name: test-group1
- members:
- test-user
- require:
- Manage user - test-user
Manage group - test-group2:
group.present:
- name: test-group2
- members:
- test-user
- require:
- Manage user - test-userstate run outputsalt-minion logIs there a better way to approach this? Going back to what I described in my post above and hoping to clarify -- the data-model (pillar) we use for managing groups is very much oriented towards "on this minion, this group exists, and has exactly these members"; not every group exists on every machine. We usually don't care if any given user is in additional groups (ie: a locally created group for a shared folder), but we very much care if users there are extra users in a group that shouldn't be. For example, I really don't want a locally created user in the I can write some code to flip that data-model around at state rendering time to figure out which groups a given user belongs in, but I still need to purge out users that shouldn't be in a managed group and this starts to feel really hacky. |
|
... or do I just need to set If that's the case, then I think the docs just need to be updated to say that |
|
same issue here and saltstack already made a breaking change, if it is a bug, it should be fixed. |
|
Thanks for opening this. I agree this is a breaking change and needs to be fixed, this has been set to be fixed in 3006.2 |
|
If it's a breaking change, surely it has to wait until 3008, with a deprecation warning in 3007? |
I think what @Ch3LL may have meant is that this behavior accidentally changed after v3005.1-2 (as @FlowBreeze noted) and THAT was an accidental breaking change. Fixing this bug would fix that breaking change. Please correct me if I interpreted that wrong. |
|
Yes thats what I meant, we should not introduce a breaking change without warning. I have not dove into this issue though so my assessment could change, once I get time to look into this to see if there is a reason behind this change. |
|
Maybe I can provide a more atomic example: Sample SLS file: Assume the user exists already. If the state is run on 3005.1: If the same state is run on 3006.1: The behaviour in 3006.1 contradicts the documentation as @FlowBreeze quoted, and the old behaviour was definitely preferable for me. Adding |
OrangeDog
added
Regression
|
Well put @stooj I have another question for thought:
Breaking change policy aside, is there any semantic behavior difference between Honestly, for me, the workaround is actually working perfectly fine, I'm comfortable with it as a long term solution, and it makes semantic sense to me too (presuming the docs get updated). A product owner would have to balance the effort of bringing back the old behavior with the impact of keeping a breaking change (which I've lived with for a while, if I'm being honest). It's just food for thought. |
|
@nicholasmhughes any ideas here? It looks like git blame is showing commit 81ff432 (from PR #62503) as the cause to this change. |
|
Edit: I confirm that setting I have something like: and elsewhere: When running state.apply, every time, I see both: and Whereas I would only expect to see these changes once, not every time. I'm not entirely sure if this has the same cause as the present issue. |
|
Anyone willing to try this fix out? #64530 |
|
@Ch3LL I just tested it and it works for me. I'm not getting any errors when |
|
Thanks for confirming. I'll go ahead and close for now, but please feel free to open a new issue if there is an issue with those other arguments. |
Description
salt.states.user.present documentation states that when the
groupsparameter is left unset, the groups for a given user will not be changed. The current behavior in saltstack seems to diverge from this behavior and treatsgroupsas an empty list, clearing out all groups except for the user's primary group.The documented behavior is actually very useful depending on how a saltstack administrator wants to manage group memberships. An administrator can choose to list of each group a user is a member of, or list each user that is in a given group. In our case, we chose the latter as it seems to be easier to manage across multiple systems.
Setup
Salt-Stack 3006.0 installed on a VM (KVM) running on-prem. Salt is installed via onedir packaging and is up-to-date at the creation of this issue. Based on behavior, I don't believe that installation method will affect the apparent bug.
I tested this against the following operating systems and they all have similar behavior:
SLS File:
Steps to Reproduce the behavior
test=trueExpected behavior
When the
groupsparameter ofuser.presentis left unset, it should leave the groups as-is. This is what the documentation states for salt.states.user.presentScreenshots
State run (2nd and 3rd run)
Versions Report
salt --versions-report (master)
salt --versions-report (minion)
```yaml Salt Version: Salt: 3006.0Python Version:
Python: 3.10.11 (main, Apr 14 2023, 05:57:16) [GCC 11.2.0]
Dependency Versions:
cffi: 1.14.6
cherrypy: unknown
dateutil: 2.8.1
docker-py: Not Installed
gitdb: Not Installed
gitpython: Not Installed
Jinja2: 3.1.2
libgit2: Not Installed
looseversion: 1.0.2
M2Crypto: Not Installed
Mako: Not Installed
msgpack: 1.0.2
msgpack-pure: Not Installed
mysql-python: Not Installed
packaging: 22.0
pycparser: 2.21
pycrypto: Not Installed
pycryptodome: 3.9.8
pygit2: Not Installed
python-gnupg: 0.4.8
PyYAML: 5.4.1
PyZMQ: 23.2.0
relenv: 0.11.2
smmap: Not Installed
timelib: 0.2.4
Tornado: 4.5.3
ZMQ: 4.3.4
System Versions:
dist: almalinux 9.1 Lime Lynx
locale: utf-8
machine: x86_64
release: 5.14.0-162.18.1.el9_1.x86_64
system: Linux
version: AlmaLinux 9.1 Lime Lynx
The text was updated successfully, but these errors were encountered: