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

[2018.3] Strip trailing commas on Linux user's GECOS fields #47149

Merged
merged 9 commits into from May 22, 2018

Conversation

Projects
None yet
5 participants
@meaksh
Member

meaksh commented Apr 18, 2018

What does this PR do?

This PR strips the trailing commas (if any) created when the user GECOS fields (comma-delimited list) are generated by the useradd module on Linux systems.

Given this example state:

create_my_test_user:
  user.present:
    - name: testuser
    - fullname: Testing User

After applying the state, the generated entry on /etc/passwd looks like this:

testuser:x:1002:100:Testing User,,,:/home/testuser:/bin/bash

Most of the times, the users are defined without roomnumber, homephone and workphone attributes so entries created on /etc/passwd are always containing those trailing commas.

On this cases the usual representation should be like:

mysql:x:60:466:MySQL database admin:/var/lib/mysql:/bin/false

So, after applying this PR, the entry generated on /etc/passwd would look like:

testuser:x:1002:100:Testing User:/home/testuser:/bin/bash

Tests written?

Yes

Commits signed with GPG?

Yes

@meaksh meaksh force-pushed the meaksh:2018.3-remove-trailing-commas-on-linux-user-gecos-fields branch from 6614ef8 to 16de6e0 Apr 18, 2018

@terminalmage

Unless this is fixing a known bug, this change should not be made in the middle of a release cycle.

@meaksh

This comment has been minimized.

Member

meaksh commented Apr 19, 2018

@terminalmage - This is actually fixing an issue reported by one of our customers. We already have this changes included in our Salt 2018.3.0 and 2016.11.4 packages.

Please let me know how to proceed here. Thanks! 👍

@terminalmage

This comment has been minimized.

Member

terminalmage commented Apr 23, 2018

@cachedout should this be treated as a bug and included in 2018.3.1? My concern is that people who have user.present states which are currently returning True without making changes, will change the /etc/passwd once upgraded to 2018.3.1. We usually reserve these sort of changes for feature releases, when the behavior change isn't being made to resolve a bug affecting functionality. This seems more like a cosmetic change to me, though.

@meaksh

This comment has been minimized.

Member

meaksh commented Apr 23, 2018

@terminalmage - AFAIK this change wouldn't produce any change on a user.present state that is currently returning True. No changes would be made on /etc/passwd after upgrading and applying user.present states because this module read first the GECOS fields from /etc/passwd and stores the values on a dict {"workphone":.. "homephone":...} which is the one used for making the comparison with the fields provided on the SLS file. Therefore, since there are no changes on "workphone", "homephone" etc, no changes will be detected and the state will keep returning True.

I agree with you that this is more like a cosmetic change than a bug itself.

@cachedout

This comment has been minimized.

Contributor

cachedout commented Apr 23, 2018

I'm just not sure I can tell what the bug is here though. If this is cosmetic, as it seems to be, then I agree that this should not go into a point release. If it's a functional problem then we can continue this discussion?

@isbm

This comment has been minimized.

Contributor

isbm commented Apr 24, 2018

Seems like Salt is treating GECOS with commas only. @meaksh isn't that chfn can make a difference?

@meaksh

This comment has been minimized.

Member

meaksh commented Apr 24, 2018

Oh, wait! I just noticed that user.info is not actually handling properly the GECOS fields on Linux (even without this PR):

  1. salt-call user.add testuser
  2. salt-call user.info testuser

Now we call chfn to add "other" attribute to GECOS fields for user testuser:

  1. chfn -o foobar testuser

Then the user.info output is broken (notice the "homephone" attribute):

  1. salt-call user.info testuser
local:
    ----------
    fullname:
    gid:
        100
    groups:
        - users
    home:
        /home/testuser
    homephone:
        ,foobar
    name:
        testuser
    passwd:
        x
    roomnumber:
    shell:
        /bin/bash
    uid:
        1001
    workphone:

I'll do the necessary changes here and will ping you back for a review. 👍

@isbm

@meaksh then I would rather fix all that. GECOS aren't exactly defined, and seems like trailing commas is OK to strip. However, this should be consistent with the external tools...

@meaksh meaksh force-pushed the meaksh:2018.3-remove-trailing-commas-on-linux-user-gecos-fields branch from 16de6e0 to fef8ce2 Apr 25, 2018

@meaksh

This comment has been minimized.

Member

meaksh commented Apr 26, 2018

I pushed new commits to this PR to enable handling the other attribute on the GECOS fields. The previous behavior of Salt was adding all extra GECOS fields as part of the homephone field, but this is confusing and also inconsistent with chfn as pointed on #47149 (comment)

Now, all extra GECOS fields are handled as an independent other attribute and user.chother function is now added to handle it. This other attribute will include all extra GECOS fields added after the fixed fullname, roomnumber, workphone and homephone fields:

min-sles12sp3:~ # cat /etc/passwd | grep pepito
pepito:x:1000:100:Pepito Grillo,45,1234,1112233,my other stuff, accepting commas:/home/pepito:/bin/bash

min-sles12sp3:~ # salt-call --local user.info pepito
local:
    ----------
    fullname:
        Pepito Grillo
    gid:
        100
    groups:
        - users
    home:
        /home/pepito
    homephone:
        1112233
    name:
        pepito
    other:
        my other stuff, accepting commas
    passwd:
        x
    roomnumber:
        45
    shell:
        /bin/bash
    uid:
        1000
    workphone:
        1234

Btw, I added 795b380 which actually fixes a bug which prevents checking if the GECOS fields contains unsupported characters.

@isbm @terminalmage @cachedout @rallytime - I think this is now ready for another review. Thanks!

@rallytime rallytime requested review from terminalmage and isbm Apr 30, 2018

@@ -448,6 +459,8 @@ def present(name,
workphone = sdecode(workphone)
if homephone is not None:
homephone = sdecode(homephone)
if other is not None:

This comment has been minimized.

@isbm

isbm May 14, 2018

Contributor

So if it is an empty string, you get other.

This comment has been minimized.

@meaksh

meaksh May 14, 2018

Member

Yes, this is behaving like the other GECOS attributes, in case of an empty string, it would be then stripped while rendering the GECOS fields.

@@ -188,6 +189,12 @@ def _changes(name,
lusr['homephone'] = sdecode_if_string(lusr['homephone'])
if lusr['homephone'] != homephone:
change['homephone'] = homephone
if 'user.chother' in __salt__ \
and other is not None:

This comment has been minimized.

@isbm

isbm May 14, 2018

Contributor

Can it be one line, pls? 😉

This comment has been minimized.

@meaksh

meaksh May 14, 2018

Member

Ok, I just did mimic the surrounding but you're right. All these should be one line comparison. I'm fixing it.

gecos_dict.get('roomnumber', ''),
gecos_dict.get('workphone', ''),
gecos_dict.get('homephone', ''),
gecos_dict.get('other', ''),).rstrip(',')

This comment has been minimized.

@isbm

isbm May 14, 2018

Contributor

I'd go here more pythonic way, actually:

out = []
for field in ['fullname', 'roomnumber', 'workphone', 'homephone', 'other']:
    field = gecos_dict.get(field)
    if field:
        out.append(field)
return ','.join(out)

This comment has been minimized.

@meaksh

meaksh May 14, 2018

Member

I would prefer to keep the current logic which is simpler and explicit rather than replacing it with a for loop + more variables and some extra calls, which unnecessarily makes the logic more complex.

@isbm

This comment has been minimized.

Contributor

isbm commented May 14, 2018

Arrgghh, I forgot to press "Finish review". 😭

meaksh added some commits Apr 18, 2018

@meaksh meaksh force-pushed the meaksh:2018.3-remove-trailing-commas-on-linux-user-gecos-fields branch from fef8ce2 to f1680f1 May 14, 2018

@isbm

isbm approved these changes May 14, 2018

@rallytime

This comment has been minimized.

Contributor

rallytime commented May 15, 2018

@meaksh These test failures are related to this change: https://jenkins.saltstack.com/job/PR/job/salt-pr-linode-ubuntu14-n/22849/#showFailuresLink

Can you take a look?

@meaksh

This comment has been minimized.

Member

meaksh commented May 16, 2018

@rallytime - Oops! I missed those failures. They should be fixed now 👍

@rallytime

This comment has been minimized.

Contributor

rallytime commented May 22, 2018

re-run py

@rallytime rallytime merged commit f8a6a85 into saltstack:2018.3 May 22, 2018

4 of 9 checks passed

jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #5144 — ABORTED
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #23077 — ABORTED
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #19200 — ABORTED
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #10114 — RUNNING
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #25334 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17422 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #22052 — SUCCESS
Details

@meaksh meaksh deleted the meaksh:2018.3-remove-trailing-commas-on-linux-user-gecos-fields branch May 23, 2018

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