schema: added non-unique parameter to PasswdUser #985
Conversation
stages: added function that checks if the user's uid is unique util: added paramter for non-unique to useradd. tests: added postive tests for non-unique parameters for users
|
Can one of the admins verify this patch? |
|
Not entirely sure how the translators work :( |
|
@sohankunkerkar Spec changes should never be made to released spec versions, only to experimental ones. #977 refers to Ignition 2.3.0, which supports spec 3.x only, and in general we don't backport changes to spec2x unless there's a need. @Quantum-Sicarius I think we shouldn't add the new field to the spec, and should just unconditionally allow non-unique UIDs and GIDs. Ignition generally doesn't prevent users from doing legal but questionable things, and non-unique UIDs have historically been (semi-)functional in Unix. In addition, if the user is specifying a UID, they presumably already know what they're doing. |
|
@bgilbert Is it maybe not worth considering adding this to the future release/spec? I do see your point with just allowing non-unique UIDs however I was thinking the idea was to model the spec after the command (https://linux.die.net/man/8/useradd). Also if this is not a spec change but a functionality change, will it not mean that if this parameter were to be added in the future the logic will need to be changed to match the command? (default of not allowing non-unique) My use case for this was that I am changing a bunch of VMs to be seeded via ignition however we run chef-infra and I wanted the UIDs to matchup. |
There was a problem hiding this comment.
Several of us discussed this offline, and there's a rough consensus that you're right: we should leave the defaults as they are, and add a field allowing the user to enable duplicate user IDs.
For the translator, you just need to add and register a custom function which does an identity translation of all the existing fields in the PasswdUser struct. The new field should default to false, so you don't need to do anything there. See the v3_1 translation code for examples.
It would be good to add a config validation check for duplicate UIDs/GIDs specified within a config when nonUnique is not specified.
| @@ -514,6 +514,9 @@ | |||
| }, | |||
| "shell": { | |||
| "type": ["string", "null"] | |||
| }, | |||
| "nonUnique": { | |||
There was a problem hiding this comment.
The name nonUnique follows the existing pattern of naming fields after useradd options. But I'm wondering if the name is too unclear in this case, since it doesn't mention UIDs.
| @@ -99,6 +99,8 @@ func (u Util) EnsureUser(c types.PasswdUser) error { | |||
| strconv.FormatUint(uint64(*c.UID), 10)) | |||
| } | |||
|
|
|||
| args = appendIfTrue(args, c.NonUnique, "--non-unique") | |||
There was a problem hiding this comment.
If a config specifies two users with the same UID, and one of them specifies nonUnique: true and one of them doesn't, what should happen? The answer can't depend on the order of the entries, since Ignition configs are declarative.
| @@ -514,6 +514,9 @@ | |||
| }, | |||
| "shell": { | |||
| "type": ["string", "null"] | |||
| }, | |||
| "nonUnique": { | |||
| "type": ["boolean", "null"] | |||
There was a problem hiding this comment.
Please add a similar field to PasswdGroup for the groupadd call.
| @@ -77,6 +78,23 @@ func (s *stage) createPasswd(config types.Config) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func userUIDConflict(a types.PasswdUser, list []types.PasswdUser) error { | |||
There was a problem hiding this comment.
I might be missing something, but I don't think this function is necessary, since useradd will catch duplicates for us. And userUIDConflict won't catch conflicts with users that already exist on the system.
|
I will take a look at your review this weekend. Thanks for the feedback! |
|
@Quantum-Sicarius I was wondering if you got a chance to address the issues above? or If not do you mind if I go-ahead and make the required changes and get it pushed in? |
I think I will still add emulation of the non-unique parameter to the useradd-stub
This fixes #977 however this does add a new schema. I am not sure how the config migration etc works yet.