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

gid_from_name fails to create and set group on SUSE Linux. #45345

Closed
dumol opened this issue Jan 9, 2018 · 8 comments
Closed

gid_from_name fails to create and set group on SUSE Linux. #45345

dumol opened this issue Jan 9, 2018 · 8 comments
Labels
Bug broken, incorrect, or confusing behavior fixed-pls-verify fix is linked, bug author to confirm fix P4 Priority 4 severity-low 4th level, cosemtic problems, work around exists State-Module
Milestone

Comments

@dumol
Copy link
Contributor

dumol commented Jan 9, 2018

Description of Issue/Question

When trying to create a user through Salt with gid_from_name: True, our SUSE minions silently fail to create needed group and to set it afterwards. On subsequent runs, an error is returned: These values could not be changed: {'gid': ''}.

Setup

Master is Ubuntu 16.04 running 2017.7.1. The issue was observed with 2017.7.2 too.

State to reproduce the error:

test:
  user.present:
    - gid_from_name: True

Steps to Reproduce Issue

When applying above state multiple times, following two types of errors are returned on the SUSE minions after the first run (as detailed above):

bs1f-lnx-sles11-x64-51:
----------
          ID: test
    Function: user.present
      Result: False
     Comment: These values could not be changed: {'gid': ''}
     Started: 13:39:51.155928
    Duration: 18.243 ms
     Changes:

bs1g-lnx-sles12-x64-57:
----------
          ID: test
    Function: user.present
      Result: False
     Comment: These values could not be changed: {'gid': ''}
     Started: 13:39:52.314709
    Duration: 31.336 ms
     Changes:

Debug logs for the SLES 12 minion:

First run:

2018-01-09 13:39:27,792 [salt.state       ][INFO    ][1997] Running state [test] at time 13:39:27.792404
2018-01-09 13:39:27,793 [salt.state       ][INFO    ][1997] Executing state user.present for test
2018-01-09 13:39:27,802 [salt.utils.lazy  ][DEBUG   ][1997] LazyLoaded file.group_to_gid
2018-01-09 13:39:27,805 [salt.utils.lazy  ][DEBUG   ][1997] LazyLoaded shadow.info
2018-01-09 13:39:27,809 [salt.utils.lazy  ][DEBUG   ][1997] LazyLoaded user.info
2018-01-09 13:39:27,810 [salt.utils.lazy  ][DEBUG   ][1997] LazyLoaded cmd.run_all
2018-01-09 13:39:27,812 [salt.loaded.int.module.cmdmod][INFO    ][1997] Executing command ['useradd', '-m', 'test'] in directory '/root'
2018-01-09 13:39:28,125 [salt.state       ][INFO    ][1997] {'shell': '/bin/bash', 'workphone': '', 'uid': 1003, 'passwd': 'x', 'roomnumber': '', 'gr
oups': ['users'], 'home': '/home/test', 'name': 'test', 'gid': 100, 'fullname': '', 'homephone': ''}
2018-01-09 13:39:28,125 [salt.state       ][INFO    ][1997] Completed state [test] at time 13:39:28.125393 duration_in_ms=333.001

Second run:

2018-01-09 13:39:52,314 [salt.state       ][INFO    ][2022] Running state [test] at time 13:39:52.314687
2018-01-09 13:39:52,315 [salt.state       ][INFO    ][2022] Executing state user.present for test
2018-01-09 13:39:52,325 [salt.utils.lazy  ][DEBUG   ][2022] LazyLoaded file.group_to_gid
2018-01-09 13:39:52,327 [salt.utils.lazy  ][DEBUG   ][2022] LazyLoaded shadow.info
2018-01-09 13:39:52,330 [salt.utils.lazy  ][DEBUG   ][2022] LazyLoaded user.info
2018-01-09 13:39:52,334 [salt.utils.lazy  ][DEBUG   ][2022] LazyLoaded cmd.run
2018-01-09 13:39:52,335 [salt.loaded.int.module.cmdmod][INFO    ][2022] Executing command ['usermod', '-g', '', 'test'] in directory '/root'
2018-01-09 13:39:52,342 [salt.loaded.int.module.cmdmod][ERROR   ][2022] Command '['usermod', '-g', '', 'test']' failed with return code: 6
2018-01-09 13:39:52,343 [salt.loaded.int.module.cmdmod][ERROR   ][2022] output: usermod: group '' does not exist
2018-01-09 13:39:52,345 [salt.state       ][ERROR   ][2022] These values could not be changed: {'gid': ''}
2018-01-09 13:39:52,346 [salt.state       ][INFO    ][2022] Completed state [test] at time 13:39:52.346045 duration_in_ms=31.336

Versions Report

On master:

# salt --versions-report
Salt Version:
           Salt: 2017.7.1

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.4.2
      docker-py: Not Installed
          gitdb: 0.6.4
      gitpython: 1.0.1
          ioflo: Not Installed
         Jinja2: 2.8
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: 1.0.3
   msgpack-pure: Not Installed
 msgpack-python: 0.4.6
   mysql-python: 1.3.7
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.12 (default, Nov 20 2017, 18:23:56)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 15.2.0
           RAET: Not Installed
          smmap: 0.9.0
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.1.4

System Versions:
           dist: Ubuntu 16.04 xenial
         locale: UTF-8
        machine: x86_64
        release: 4.4.0-104-generic
         system: Linux
        version: Ubuntu 16.04 xenial

On slaves:

bs1f-lnx-sles11-x64-51:~ # salt-minion --versions-report
Salt Version:
           Salt: 2016.11.4

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.1
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.8
        libgit2: 0.20.0
        libnacl: Not Installed
       M2Crypto: 0.22
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.6
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: 0.20.0
         Python: 2.6.9 (unknown, Aug  5 2016, 11:15:31)
   python-gnupg: Not Installed
         PyYAML: 3.12
          PyZMQ: 15.2.0
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.3
            ZMQ: 4.1.4

System Versions:
           dist: SuSE 11 x86_64
        machine: x86_64
        release: 3.0.101-108.13-default
         system: Linux
        version: SUSE Linux Enterprise Server  11 x86_64
bs1g-lnx-sles12-x64-57:~ # salt-minion --versions-report
Salt Version:
           Salt: 2016.11.4

Dependency Versions:
           cffi: 0.9.2
       cherrypy: Not Installed
       dateutil: 2.1
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.7.3
        libgit2: 0.22.0
        libnacl: Not Installed
       M2Crypto: 0.22
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.6
   mysql-python: Not Installed
      pycparser: 2.12
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: 0.22.0
         Python: 2.7.7 (default, Jun 23 2014, 13:38:05) [GCC]
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 14.5.0
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.0.5

System Versions:
           dist: SuSE 12 x86_64
        machine: x86_64
        release: 3.12.28-4-default
         system: Linux
        version: SUSE Linux Enterprise Server  12 x86_64
@Ch3LL
Copy link
Contributor

Ch3LL commented Jan 9, 2018

ping @saltstack/team-suse any ideas here?

@Ch3LL Ch3LL added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Jan 9, 2018
@Ch3LL Ch3LL added this to the Blocked milestone Jan 9, 2018
@meaksh
Copy link
Contributor

meaksh commented Jan 10, 2018

Well, I've fixed this to prevent that error if an empty string is used as a group name, but the problem here is different.

As I understand this, a new group shouldn't be created during the user.present state execution even if gid_from_name is set. Actually, at the moment, user.present fails in cases where you set some groups and those groups are not present, so IMHO we should follow the same behavior for this situation with gid_from_name.

In your particular case, if there is no group with that name test, Salt should report an error for that state instead of creating the new user without having the right default group you set on your SLS file.

FMPOV a correct approach for the SLS would be:

test_group:
  group.present:
    - name: test

test_user:
  user.present:
    - name: test
    - gid_from_name: True
    - require:
      - group: test_group

@Ch3LL - what do you think about this?

I'm creating a PR about this in a minute.

@Ch3LL
Copy link
Contributor

Ch3LL commented Jan 10, 2018

@meaksh agreed we should use the group.present to add the group. Thanks for cleaning up the error! :)

@dumol can you give that fix a try

@Ch3LL Ch3LL added State-Module Bug broken, incorrect, or confusing behavior severity-low 4th level, cosemtic problems, work around exists P4 Priority 4 and removed Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged labels Jan 10, 2018
@Ch3LL Ch3LL modified the milestones: Blocked, Approved Jan 10, 2018
@Ch3LL Ch3LL added the fixed-pls-verify fix is linked, bug author to confirm fix label Jan 10, 2018
@dumol
Copy link
Contributor Author

dumol commented Jan 16, 2018

I appreciate your responses, but sorry, your "correct approach" seems like a workaround to me. We used to do that, but moved to the more concise form, as stated above:

test:
  user.present:
    - gid_from_name: True

Which is also "correct" according to current documentation. But it fails on SLES, therefore this bug report.

@meaksh
Copy link
Contributor

meaksh commented Jan 16, 2018

Hey @dumol , thanks for your response. I'm not sure, looking at the documentation [1] it's not clear to me whether a new group should be created or not if gid_from_name is set:

  • gid_from_name - If True, the default group id will be set to the id of the group with the same name as the user, Default is False.

More over, looking at groups parameter:

  • groups - A list of groups to assign the user to, pass a list object. If a group specified here does not exist on the minion, the state will fail...

That's the reason why I think gid_from_name should not create a new group in case it does not exist.

I would really appreciative others opinions here. Thanks!

[1] - https://docs.saltstack.com/en/latest/ref/states/all/salt.states.user.html#salt.states.user.present

@dumol
Copy link
Contributor Author

dumol commented Jan 16, 2018

Ah, I see, interesting… Perhaps the documentation should be improved to also say If a group specified here does not exist on the minion, the state will fail for gid_from_name as well?

Maybe with may instead of will? Because we use this for a number of OS'es and it only fails on SLES and MacOS (over there with a different error, see #45142). Possibly all the others (RHEL, Ubuntu, Debian, Arch, Alpine, FreeBSD and OpenBSD) automatically create the group with the user, and things just work…

Still, there's room for improvement here. The state shouldn't apply silently on first run on SLES. And an improved error should be emitted. What do you think? Thanks!

@meaksh
Copy link
Contributor

meaksh commented Jan 16, 2018

@dumol, I agree with you. I've created #45365 which addresses this issue by:

  • Making the state fails initially in case the group does not exist.
  • Updating documentation to make clear the state will fail in case the group does not exist.

I think others OS'es probably creates a group with the same name as the user when the user is being created by "useradd" or similar. Then on a second run of the state, it applies successfully as the group exist now. But for SUSE and maybe others, a group with the same name is not actually created and that's why it passes the first time and then we get the error.

With the PR I pointed here, we always check first if the group exist before creating the user so the state will fail even in other OS'es. What do you think?

@dumol
Copy link
Contributor Author

dumol commented Jan 16, 2018

Fine by me. Thanks!

meaksh added a commit to meaksh/salt that referenced this issue Jan 17, 2018
Fixes saltstack#45345

Ensure empty string gid is set to None

Make pylint happy

Fix integration tests for 'user.present' state.

Update documentation for 'gid_from_name' parameter

Refactor to prevent logical bug when gid is 0
meaksh added a commit to meaksh/salt that referenced this issue Jan 26, 2018
Fixes saltstack#45345

Ensure empty string gid is set to None

Make pylint happy

Update documentation for 'gid_from_name' parameter

Refactor to prevent logical bug when gid is 0

Fix integration tests for 'user.present' state.
thusoy added a commit to get-wrecked/salt-states that referenced this issue Apr 4, 2018
Most linux distros add this automatically, probably as a part of
adduser, but this is not done on macOS and some other systems.

Ref saltstack/salt#45345 (comment).
meaksh added a commit to meaksh/salt that referenced this issue Jun 18, 2018
Fixes saltstack#45345

Ensure empty string gid is set to None

Make pylint happy

Update documentation for 'gid_from_name' parameter

Refactor to prevent logical bug when gid is 0

Fix integration tests for 'user.present' state.
isbm pushed a commit to isbm/salt that referenced this issue Dec 4, 2018
Fixes saltstack#45345

Ensure empty string gid is set to None

Make pylint happy

Update documentation for 'gid_from_name' parameter

Refactor to prevent logical bug when gid is 0

Fix integration tests for 'user.present' state.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior fixed-pls-verify fix is linked, bug author to confirm fix P4 Priority 4 severity-low 4th level, cosemtic problems, work around exists State-Module
Projects
None yet
Development

No branches or pull requests

3 participants