WIP: add sysusers to compose #1376
Conversation
| @@ -250,7 +412,7 @@ rpmostree_check_passwd_groups (gboolean passwd, | |||
| return TRUE; /* Note early return */ | |||
| else if (g_str_equal (chk_type, "previous")) | |||
| ; /* Handled below */ | |||
| else if (g_str_equal (chk_type, "file")) | |||
| else if (g_str_equal (chk_type, "file") || g_str_equal (chk_type, "sysusers")) | |||
There was a problem hiding this comment.
For sysusers option, I think it might be good if we also check passwd/group as b4, which is why I added those lines. But let me know what you think about this. =)
|
I only scanned this at a very high level. Having unit tests is great. Have you played around much with whether or not the results will work? I am a bit concerned that we'll run into some blockers...I heard that an attempt to do this for all RPMs tripped up on e.g.: which actually wants to allocate a home directory and interactive shell. But eh...maybe we punt and translate those to systemd unit files which just run that on boot? I bet this would be fairly easy to play around with without changing rpm-ostree if you did a scripted/manual conversion of |
src/libpriv/rpmostree-passwd-util.c
Outdated
| @@ -213,6 +228,152 @@ compare_group_ents (gconstpointer a, gconstpointer b) | |||
| return strcmp ((*sa)->name, (*sb)->name); | |||
| } | |||
|
|
|||
| gboolean | |||
| rpmostree_passwd_ents2sysusers (gboolean is_passwd, | |||
There was a problem hiding this comment.
The wrapper function here feels unnecessary, let's just inline it into the one caller?
src/libpriv/rpmostree-passwd-util.c
Outdated
| else | ||
| sysent->id = g_strdup_printf ("%u", convent->uid); | ||
|
|
||
| sysent->type = g_strdup ("u"); |
There was a problem hiding this comment.
Minor, we can make this member a const char* and avoid the strdup/free.
src/libpriv/rpmostree-passwd-util.c
Outdated
| { | ||
| char *stored_gid = colon + 1; | ||
| g_print ("current_gid is %s\n", current_gid); | ||
| g_print ("stored_gid is %s\n", stored_gid); |
There was a problem hiding this comment.
Let's remove the debug bits if you don't need them anymore.
src/libpriv/rpmostree-passwd-util.c
Outdated
| g_print ("stored_gid is %s\n", stored_gid); | ||
| /* We skip the insertion if we found gid matches */ | ||
| if (g_str_equal (stored_gid, current_gid)) | ||
| { |
src/libpriv/rpmostree-passwd-util.c
Outdated
| else | ||
| { | ||
| *error = g_error_new (G_IO_ERROR, G_IO_ERROR_FAILED, "mismatch between /lib/passwd and /lib/group"); | ||
| g_print("Catastrophic failure\n"); |
There was a problem hiding this comment.
return glnx_throw (error, "mismatch between passwd/group: %s %s", stored_gid, current_gid); or so.
src/libpriv/rpmostree-passwd-util.c
Outdated
| sysent->name = g_strdup (convent->name); | ||
| /* Unset the gecos & dir for g entries */ | ||
| sysent->gecos = g_strdup ("-"); | ||
| sysent->dir = g_strdup ("-"); |
There was a problem hiding this comment.
Maybe use NULL to mean "default" here to avoid a strdup of a constant.
src/libpriv/rpmostree-passwd-util.c
Outdated
| sysuser_ent->name, sysuser_ent->id, | ||
| sysuser_ent->gecos, sysuser_ent->dir, | ||
| NULL); | ||
| g_string_append_printf(sysuser_content, "%s\n", line_content); |
There was a problem hiding this comment.
BTW our code style has space between identifier and paren, so g_string_append_printf (sysuser_content and elsewhere.
Yup, I wanted to have this WIP out, to have some discussions first and to make sure I am still on the right track =). I felt I did a bad job communicating/discussing ideas earlier, so I wanted to do more of it(discussion) :p. The next step I am planning to do will be adding this conversion to compose and test out if sysusers will work (and solve/test the blocker you mentioned above). |
Not yet... I will try the postgres cmd you suggested earlier and report back the status =).
Sure, lemme try those on AH, and see if things still work ok or not :). |
|
STATUS: the testing blockers might need a bit more time. I was trying to integrate the current conversion code into rpm-ostree and tested conversion of It is possible that... even though So, I will try manual conversion for now, and hopefully will have results soon. Thanks, and sorry for the long response time. |
|
STATUS UPDATE Second: I tried out the systemd-sysusers with the current implementation of conversion, there were few problems for the execution: The content for sysusers generation is like the following, it is weird that I see those trailing garbage... 2: systemd-sysusers status has condition failed label 3: With the current approach of conversion, sysusers genreation is failing. I researched a bit, and it turns out for id, it describes the following:
So, the original approach for uid:gid does not work :(. I should of read more carefully when implementing it =(. Will address/try out implementation to try to resolve above bugs shortly. Will also try out the manual translation of postgres entries into sysusers... (Sorry, I originally thought I need to convert the entire |
So, I have tested the postgres blocker, and it seems like when there are 6 fields, E.g: But it is weird tho.... on the documentation side, it said shell can be included. And for the code base, https://github.com/systemd/systemd/blob/master/src/sysusers/sysusers.c#L1390, it also seems shell get recognized.. EDIT: Was running |
|
UPDATE: Actually, I looked into systemd commit histories, and it looks like login shell is recently introduced to systemd(Feb 5 ish). I will double check on a newer systemd version and see if the result still works. =) |
|
Hi, @cgwalters, I tested the blocker on f28 with systemd.v238 and here is the output: After reboot, looks like postgres is generated correctly =). Let me know if my testing process is correct or not, thanks =). |
|
For now, I think I will look into rkt's implementation of sysusers and try redesigning the logic of current implementation. They seem to adopt systemd-sysusers 2-years ago. Will also report back which path I decided to go for converting sysusers =). |
|
Hi, I have thought about the implementation yesterday, I am thinking we can avoid detecting "collisions" for sysusers, because collision case will likely only happen if there is an exact duplicate entries in That way, we do not need to have a hashtable to track entries anymore.(which is complicated IMO :( ). Instead, we will have the following conversion logic: 1: We use a pointer array to track all of the sysuser entries (still a struct) Possible drawbacks:
Benefit:
I will be trying this method out for now, but let me know your thoughts on this, and I am willing to change accordingly :p. Thanks, @cgwalters. |
In general don't worry about performance at first unless your algorithm is worse than quadratic, and
Yeah, that shouldn't happen - I'd say we fatally error out if we find out it did. |
peterbaouoft
force-pushed
the
pr/add_sysusers_to_compose
branch
2 times, most recently
from
c6bf9c2 to
a9ef91e
Compare
|
STATUS UPDATE: 1: systemd-sysusers will error out if there already exists a group entry in 2: systemd itself ships workaround: create symlinks with exact same name, but pointing to 3: Need to remove 4: Passwd/group entries regeneration during NOTE: for item 2 and 3, it might take me some extra time to work with... As I am still quite new to file handling in GLib, but I will try the best to remove WIP label before next Monday. =) Thank you for your patience. |
|
☔ The latest upstream changes (presumably ec0af35) made this pull request unmergeable. Please resolve the merge conflicts. |
|
For the shadow files, see ostreedev/ostree#1562 Ideally it would work to delete them from the compose (ostree) and have a tmpfiles.d to create this on boot (before sysusers runs). |
|
Status Update: I think the remaining bits needing to do are likely to be the following:
|
|
Status update: 1: We can create a unified conf for systemd-sysuser placed in 4: The existing 3 way merge logic for /etc/ should be as following:
Note: Due to 3 way merge logic above, sysuser will have some difficulties as |
|
STATUS UPDATE: A way to fix the problem can be one of the following methods:
Then, theoretically, package layering and upgrade problem should be solved. Because all we need to do is now maintain the sysuser entries in Implementation:
But may be.. I am understanding the usage of 2: shadow file entries.. Hmm, this can be tricky to do so.. As systemd-sysusers itself also writes entries into them... I am still kinda unsure what will be the benefit of using |
Yeah, this one is messy, because need to handle both the base tree and layers, so I think we need two files. Let's say Though in theory we could append to an existing
In general, that's a bit dangerous to do because the uid/gid could be reused, and if there's old data lying around in e.g. And in the end what we really want is 1) services to be containerized 2) to use the new dynamic users where possible - a lot of the OS could do this. |
|
Thanks for the reply \o/!
Yup, that makes sense to me.
Right, I recall I have seen that in Dynamic user documentation. Makes sense. I think we don't need to do the user removal then. In that case, we will only need to cleanup for |
peterbaouoft
force-pushed
the
pr/add_sysusers_to_compose
branch
2 times, most recently
from
ec396b6 to
c4472fe
Compare
|
I think this one is probably now the biggest blocker for making it easier for people to do custom composes. Having uids change on you is an evil trap. I need to dive into these patches again in depth, but it looks like we could at least merge some of the preparatory patches? |
|
Hi, thanks for the update :).
This sounds exciting, good to see this patch to be needed soon =D. Out of curiosity, do you have more info related to that?
I will likely have some spare cycles after today. I can spend some time separating the preparatory commits into a PR tmr. Do you think it(separating patch) is reasonable / fitting for the timeline? |
|
☔ The latest upstream changes (presumably 9920094) made this pull request unmergeable. Please resolve the merge conflicts. |
peterbaouoft
force-pushed
the
pr/add_sysusers_to_compose
branch
from
c4472fe to
6677e6b
Compare
|
Status Update: Rebased , and pushed out what I have so far. Unfortunately, I did not get very far yesterday, so the leftover bits are still pretty similar to the comment I made here. The implementation bits are explained in the commit messages, if they are not clear, please let me know, and I can hopefully clarify it :). I will also try to keep an eye on this PR, and really looking forward about its future progress :D. Again, thanks for the help in this PR. |
The logic for writing sysusers is integrated into both compose logic and rpmostree_check_passwd_groups. The plan is that whenever "sysuser" is specified in check-passwd section, The compose will use the sysuser logic. The passwd entries instead will be converted to sysuser entries. And eventually at the end of compose a file will be written to '/usr/lib/sysusers.d'.
As tilte. This adds a 'temp-convert' command to rpm-ostree. The main goal of this command is to convert the AH's /usr/lib/passwd and /usr/lib/group for testing purposes. This can be useful to find whether your output for conversion is correct when testing on real-world entries
peterbaouoft
force-pushed
the
pr/add_sysusers_to_compose
branch
from
6677e6b to
4bb8db1
Compare
|
To make this work, we're probably going to need to re-implement sysusers for CentOS7 based systems right? |
Hi, I seem to be missing a bit of context here. Can you elaborate a bit more on that? Is it because CentOS7 is a community based version of RHEL? (and if we succeed on c7, we does for rhel also) And to clarify a bit, by "re-implementing sysusers", do you mean by modifying some existing systemd-sysuser logic or will the logic still be under |
|
The problem with supporting |
|
(Alternatively, we could ignore RHEL7) |
| if (!rpmostree_check_passwd (self->repo, self->rootfs_dfd, treefile_dirpath, self->treefile, | ||
| self->previous_checksum, | ||
| self->previous_checksum, &sysuser_entries, |
There was a problem hiding this comment.
It seems to me that if we're using sysusers - there's really no reason to go into any of the rpmostree_check_passwd() code. With sysusers, the uids are assigned on the client side, unlike every other approach here. The code here was mostly concerned with comparing from the "previous" values, but conceptually we don't have any previous values.
It seems the main thing we're lifting there is the current values of the passwd/group database.
One thing it's tempting to do here too is add a new systemd-sysusers: true boolean, and then that overrides all of the other legacy stuff. Then here in compose-builtin-tree.c we'd do:
if (self->treefile)
{
if (sysusers)
{
if (!rpmostree_passwd_sysusers_convert (self->rootfs_fd, cancellable, error))
return FALSE;
}
else
{
if (!rpmostree_check_passwd (self->repo, self->rootfs_dfd, ...)
return FALSE;
}
}
or so?
There was a problem hiding this comment.
It seems to me that if we're using sysusers - there's really no reason to go into any of the rpmostree_check_passwd() code. With sysusers, the uids are assigned on the client side, unlike every other approach here.
Ha, I was thinking to propose this approach, but then I figured some packages seem to own their specific uid/gid (Can't seem to find a source, so may be I was wrong). One example i believe is daemon sometimes own uid 2? So, I thought check_passwd and check_group was mainly used for that case (the exact mapping of package assignment).
But if not, then I agree, we don't necessarily need to check sysusers for "previous" values.
There was a problem hiding this comment.
One thing it's tempting to do here too is add a new systemd-sysusers: true boolean, and then that overrides all of the other legacy stuff. Then here in compose-builtin-tree.c we'd do:
Yea, this implementation makes sense, and if we are skipping that, may be we can also skip the sections here too?
https://github.com/projectatomic/rpm-ostree/blob/master/src/libpriv/rpmostree-postprocess.c#L971-L991.
I was thinking earlier if we could work around the sysuser logic a bit so that the migration of passwd needs not to happen (or skipped) in postprocess.
There was a problem hiding this comment.
Ha, I was thinking to propose this approach, but then I figured some packages seem to own their specific uid/gid (Can't seem to find a source, so may be I was wrong). One example i believe is daemon sometimes own uid 2?
A bunch of uid/gid values are fixed; see https://fedoraproject.org/wiki/Packaging:UsersAndGroups and note https://pagure.io/setup/blob/master/f/uidgid
Is that what you're thinking of?
But that...oh wait, argh, I think this calls into question our entire approach here 😦 - I'll comment below.
There was a problem hiding this comment.
A bunch of uid/gid values are fixed; see https://fedoraproject.org/wiki/Packaging:UsersAndGroups and note https://pagure.io/setup/blob/master/f/uidgid
Is that what you're thinking of?
Yup. :D
May be a dumb question. If we are doing that, can we reuse some of the existing logic from systemd-sysusers? :P |
The problem is that systemd has a vast array of "internal" APIs, from hash tables to filesystem utilities, etc. A lot of C projects have this to some degree, though rpm-ostree uses GLib which helps a lot. If you look at that code almost every single function call we'd need to port. I'm thinking a bit about writing a basic implementation of sysusers in Rust. But trying a line-by-line port of the systemd code could work fine too. Or, I am also OK declaring this a post-RHEL7 feature too. |
|
Following up from #1376 (comment) - The problem with our design that I just realized is that we're not correctly distinguishing between "fixed" and dynamic uid/gid values. Let's take the two cases of openssh, which has a fixed uid/gid, and chrony which has a dynamic one. Today server-side flow with this current PR is: And this is important for the The only way I can think of to fix this is to intercept calls to Sorry about not realizing this earlier...this whole problem is very complex! |
|
Hi, sorry about the late reply. Been busy with school work lately...
I agree with the server side flow, but am confused about why we can't tell between fixed or dynamic id with the flow. I thought
Yup, I believe so, if |
If we are going that route, systemd-sysusers seem to be able to handle automatic assignment of uids/gids: maybe we can use that? |
|
☔ The latest upstream changes (presumably 096f8de) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Can one of the admins verify this patch?
|
|
@peterbaouoft: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Hi, this PR includes the early version of implementation for #49. There are still many parts can be added/improved, but I would like to put this PR out to have some feedback/discussion to make sure I am on the right track :p.
Things did so far in this PR, also see discussion
rpmostree_check_passwd_groupsThings still left undone:
PS: sorry for this PR to be taking so long... I had really bad time management for past few months, and I was kinda hesitant to have a discussion back then :(. But hopefully, I will do better at those in the future =).