passwd: Add a way to delete users/groups #1014

sohankunkerkar · 2020-07-06T23:13:18Z

Fixes #738
We need to wait until coreos/ignition-dracut#197 gets merged. Not ready for review yet.

config/v3_2_experimental/schema/ignition.json

internal/exec/stages/files/passwd.go

config/v3_2_experimental/schema/ignition.json

internal/exec/stages/files/passwd.go

internal/exec/util/passwd.go

arithx · 2020-07-08T04:35:54Z

From the CI output it looks like you need to update the translate function (here's an example of what I did in the LUKS stuff: 5e91412#diff-9f93889254bb669415809d4953fd3fbe)

Both `getpwnam_r(3)` and `getgrnam_r(3)` can return `ENOENT` or `ESRCH` if the record is not found. Handle these. The manpages also list `EBADF` and `EPERM` (and implies others via `...`) as other possible codes, which is not great. Digging into this a bit, I think it may be due to different conventions in different NSS providers? Let's just handle `ENOENT` and `ESRCH` for now and leave the less obvious ones until we actually hit something that returns it. This fixes the error CI is hitting in coreos#1014 where it gets `ESRCH` when looking up a non-existent group. Tested-by: Sohan Kunkerkar <[email protected]>

sohankunkerkar · 2020-07-15T13:00:13Z

I would like to add a test to verify this change, however, I'm a little skeptical about the testing part because there's no way I can delete the existing users/groups, at least for an FCOS base image (all users listed below are created via Ignition during first boot and not built into the image)

[core@localhost ~]$ cat /etc/passwd
root:x:0:0:root:/root:/bin/bash
core:x:1000:1000:CoreOS Admin:/var/home/core:/bin/bash
fedora-coreos-pinger:x:981:981:Fedora CoreOS telemetry service user:/:/usr/sbin/nologin
zincati:x:980:980:Zincati user for auto-updates:/:/usr/sbin/nologin

internal/exec/stages/files/passwd.go

jlebon

I would like to add a test to verify this change, however, I'm a little skeptical about the testing part.

Hmm, I think we can expand the blackbox tests we have for this? See e.g. tests/positive/passwd/users.go.

internal/exec/util/passwd.go

tests/stubs/userdel-stub/main.go

cgwalters · 2020-07-20T17:47:16Z

I want to more strongly emphasize my feelings on this: #738 (comment)

I'm not opposed to this code as is, but it seems like it'd be a lot simpler for us to instead skip adding the core user in the first place than to have our default config add the user, then we immediately go and delete it when processing later Ignition.

Wouldn't it work to skip the useradd if shouldExist: false or so?

internal/exec/stages/files/passwd.go

internal/exec/util/passwd.go

tests/positive/passwd/users.go

arithx

Is the current code working in FCOS to be able to not create the core user?

internal/exec/stages/files/passwd.go

internal/exec/util/passwd.go

tests/positive/passwd/users.go

sohankunkerkar · 2020-07-24T15:28:34Z

Is the current code working in FCOS to be able to not create the core user?

Yeah, it is.
I think #1014 (comment) suggestion helped to resolve the above issue.

arithx

Changes LGTM; CI looks like it needs a retrigger and will let someone else take another glance at it.

jlebon

Just some nits, LGTM as is!

internal/exec/stages/files/passwd.go

internal/exec/util/passwd.go

Fixes coreos#738 Some users may want to add a custom user, and remove the `core` user injected via the default configuration. This PR will enable users to delete the existing users/groups in a given distro.

This variable is added to indicate if we want to delete the existing users/groups

miabbott · 2020-08-06T19:08:22Z

We've got 3 approvals and green CI...can we merge this?

sohankunkerkar · 2020-08-26T17:11:38Z

For tracking purpose: coreos/fedora-coreos-tracker#155

This addition to the docs was missed in the original PR coreos#1014

darkmuggle reviewed Jul 7, 2020

View reviewed changes

config/v3_2_experimental/schema/ignition.json Show resolved Hide resolved

internal/exec/stages/files/passwd.go Outdated Show resolved Hide resolved

sohankunkerkar force-pushed the delete-user-groups branch from 85fe04e to 07ddcd4 Compare July 7, 2020 01:06

arithx reviewed Jul 7, 2020

View reviewed changes

config/v3_2_experimental/schema/ignition.json Show resolved Hide resolved

internal/exec/stages/files/passwd.go Outdated Show resolved Hide resolved

internal/exec/util/passwd.go Show resolved Hide resolved

internal/exec/util/passwd.go Outdated Show resolved Hide resolved

sohankunkerkar force-pushed the delete-user-groups branch from 07ddcd4 to c89095f Compare July 7, 2020 15:39

sohankunkerkar force-pushed the delete-user-groups branch 5 times, most recently from 733df71 to c7e716d Compare July 13, 2020 17:21

jlebon mentioned this pull request Jul 14, 2020

util/user_group_lookup: handle more error codes #1028

Merged

sohankunkerkar force-pushed the delete-user-groups branch 2 times, most recently from 7b96d10 to 0178899 Compare July 14, 2020 20:55

sohankunkerkar marked this pull request as ready for review July 15, 2020 02:59

sohankunkerkar changed the title ~~WIP passwd: Add a way to delete users/groups~~ passwd: Add a way to delete users/groups Jul 15, 2020

sohankunkerkar requested review from arithx and jlebon July 15, 2020 14:49

arithx approved these changes Jul 16, 2020

View reviewed changes

internal/exec/stages/files/passwd.go Outdated Show resolved Hide resolved

sohankunkerkar force-pushed the delete-user-groups branch from 0178899 to 4c57257 Compare July 16, 2020 12:24

arithx previously approved these changes Jul 16, 2020

View reviewed changes

jlebon reviewed Jul 16, 2020

View reviewed changes

internal/exec/util/passwd.go Show resolved Hide resolved

sohankunkerkar force-pushed the delete-user-groups branch 2 times, most recently from bc0e4bc to a5735ab Compare July 20, 2020 14:13

jlebon reviewed Jul 20, 2020

View reviewed changes

tests/stubs/userdel-stub/main.go Outdated Show resolved Hide resolved

sohankunkerkar force-pushed the delete-user-groups branch 2 times, most recently from ca0cf88 to 873d405 Compare July 20, 2020 16:54

bgilbert reviewed Jul 22, 2020

View reviewed changes

sohankunkerkar force-pushed the delete-user-groups branch from 2e99883 to bdda6b6 Compare July 22, 2020 16:42

sohankunkerkar changed the title ~~passwd: Add a way to delete users/groups~~ WIP passwd: Add a way to delete users/groups Jul 22, 2020

sohankunkerkar force-pushed the delete-user-groups branch 2 times, most recently from 67bb289 to a2f9661 Compare July 22, 2020 19:30

sohankunkerkar changed the title ~~WIP passwd: Add a way to delete users/groups~~ passwd: Add a way to delete users/groups Jul 22, 2020

sohankunkerkar requested a review from bgilbert July 23, 2020 12:41

darkmuggle mentioned this pull request Jul 23, 2020

dracut/30ignition: Add packages to delete users/groups coreos/ignition-dracut#197

Closed

arithx reviewed Jul 24, 2020

View reviewed changes

internal/exec/stages/files/passwd.go Outdated Show resolved Hide resolved

internal/exec/stages/files/passwd.go Outdated Show resolved Hide resolved

internal/exec/util/passwd.go Show resolved Hide resolved

tests/positive/passwd/users.go Show resolved Hide resolved

sohankunkerkar force-pushed the delete-user-groups branch from a2f9661 to aa76153 Compare July 24, 2020 15:24

sohankunkerkar requested a review from arithx July 24, 2020 16:10

sohankunkerkar force-pushed the delete-user-groups branch 2 times, most recently from 8568a0e to 74cd722 Compare July 24, 2020 22:27

arithx approved these changes Jul 28, 2020

View reviewed changes

jlebon approved these changes Jul 29, 2020

View reviewed changes

internal/exec/stages/files/passwd.go Outdated Show resolved Hide resolved

internal/exec/util/passwd.go Outdated Show resolved Hide resolved

sohankunkerkar added 5 commits July 29, 2020 10:17

passwd: allow removing the existing users/groups

6ad5c06

Fixes coreos#738 Some users may want to add a custom user, and remove the `core` user injected via the default configuration. This PR will enable users to delete the existing users/groups in a given distro.

config: add ShouldExist to schema

1e0f293

This variable is added to indicate if we want to delete the existing users/groups

tests/stubs: Add blackbox testing stub for userdel and groupdel

f5a48c8

tests/positive: Add test for deleting users

c64b565

dracut/30ignition: Add packages to delete users/groups

786c866

sohankunkerkar force-pushed the delete-user-groups branch from 74cd722 to 786c866 Compare July 29, 2020 14:18

bgilbert approved these changes Aug 6, 2020

View reviewed changes

sohankunkerkar merged commit 2433c55 into coreos:master Aug 6, 2020

cgwalters added a commit to cgwalters/ignition that referenced this pull request Aug 29, 2020

doc: Add shouldExist for users

70786e6

This addition to the docs was missed in the original PR coreos#1014

cgwalters mentioned this pull request Aug 29, 2020

doc: Add shouldExist for users #1080

Merged

cgwalters added a commit to cgwalters/ignition that referenced this pull request Aug 30, 2020

doc: Add shouldExist for users

a9823d1

This addition to the docs was missed in the original PR coreos#1014

cgwalters added a commit to cgwalters/ignition that referenced this pull request Aug 31, 2020

doc: Add shouldExist for users and groups

db3299a

This addition to the docs was missed in the original PR coreos#1014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

passwd: Add a way to delete users/groups #1014

passwd: Add a way to delete users/groups #1014

sohankunkerkar commented Jul 6, 2020

arithx commented Jul 8, 2020

sohankunkerkar commented Jul 15, 2020

jlebon left a comment

cgwalters commented Jul 20, 2020

arithx left a comment

sohankunkerkar commented Jul 24, 2020

arithx left a comment

jlebon left a comment

miabbott commented Aug 6, 2020

sohankunkerkar commented Aug 26, 2020

passwd: Add a way to delete users/groups #1014

passwd: Add a way to delete users/groups #1014

Conversation

sohankunkerkar commented Jul 6, 2020

arithx commented Jul 8, 2020

sohankunkerkar commented Jul 15, 2020

jlebon left a comment

Choose a reason for hiding this comment

cgwalters commented Jul 20, 2020

arithx left a comment

Choose a reason for hiding this comment

sohankunkerkar commented Jul 24, 2020

arithx left a comment

Choose a reason for hiding this comment

jlebon left a comment

Choose a reason for hiding this comment

miabbott commented Aug 6, 2020

sohankunkerkar commented Aug 26, 2020