-
Notifications
You must be signed in to change notification settings - Fork 192
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
core: wrap and intercept groupadd
calls in scriptlets
#3778
Conversation
Result from current F36 FCOS built with this PR:
|
53600a3
to
0e7b906
Compare
5af74ef
to
fa2cdc6
Compare
groupadd
calls in scriptletsgroupadd
calls in scriptlets
This should be ready to go, the CI is currently failing on coreos/fedora-coreos-tracker#1258. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to handle the case where a package already contains a sysusers fragment and is running groupadd? Does that case exist? Seems like e.g. we should avoid writing duplicate sysusers fragments.
.long("password") | ||
.takes_value(true), | ||
) | ||
.arg(Arg::new("system").short('r').long("system")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should either:
- Error if this flag isn't passed
- No-op if this flag isn't passed
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I think longer term we'd want to error out.
I'm not sure if we currently have scriptlets not using -r
, so in the short term I'd just log a warning and proceed with the logic, so that we can adjust affected packages (if any).
static SYSUSERS_DIR: &str = "usr/lib/sysusers.d"; | ||
|
||
// The filename of the configuration fragment is in fact a public | ||
// API, because users may have masked it in /etc. Do not change this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Why would someone do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be one approach for handling users/groups mismatches like coreos/fedora-coreos-tracker#1201 (either as a transitory measure or a permanent override).
|
||
// The filename of the configuration fragment is in fact a public | ||
// API, because users may have masked it in /etc. Do not change this. | ||
let filename = format!("30-pkg-group-{groupname}.conf"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For tmpfiles snippets we derive the filename from the package name - I think we could do that here by passing it through the environment.
I don't have a strong opinion on that though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered doing that, I explored the pkgname-through-env approach and it's indeed feasible.
I'm still oscillating on the file-naming decision though.
My current stance is that having {groupname}
in the file helps with idempotence, as a single package may be doing multiple groupadd
. Current naming convention avoid interferences and allows for atomic-replaces without parsing/merging.
One third option I considered is the {pkgname}-{groupname}
format, but I didn't see many additional benefits in doing that.
I'm open to more brainstorming on this topic. /cc @jlebon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My current stance is that having {groupname} in the file helps with idempotence, as a single package may be doing multiple groupadd.
Makes sense to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, looking at /usr/lib/sysusers.d
in my local FSB and FCOS, I don't see any config using priority prefixes in their names (except Zincati). So even with 30-
, we'd be at the top of the list.
One third option I considered is the
{pkgname}-{groupname}
format, but I didn't see many additional benefits in doing that.
That'd be my vote. I think the additional benefit of being able to tell which package we derived from is nice (given that we don't currently include the package name in the generated fragment either). It would also better conform to the guideline established in sysusers.d(5)
:
Each configuration file shall be named in the style of
package.conf
orpackage-part.conf
. The second variant should be used when it is desirable to make it easy to override just this part of configuration.
(Though it's a bit fuzzy here because we're doing it for them, hehe, but it's in the same spirit I think).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it looks like there is consensus about adding the package name in the filename. Sorry I didn't see the last comment here before rebasing, I'll wire the pkgname in a followup.
Regarding the numerical prefix and suggested naming pattern, there has been some friction reported upstream already: systemd/systemd#21155
In our case, I already know we'd need to properly priority-sort several sources with (possibly conflicting) definitions:
- packaged fragment (e.g. https://pagure.io/setup/pull-request/39)
- file overlays (e.g. overlay/sysusers: add fragments for all FCOS system groups fedora-coreos-config#1836)
groupadd
translation (this PR)
While we aren't strictly following the suggested pattern from the docs, the numerical prefix at least allows predictable sorting/overriding which is helpful in the above case of multiple/conflicting sources of truth.
That case exists. But in general it's ok to have multiple group re-definitions as long as they are consistent (e.g. even We can add some heuristic for this, but I'm unsure how much deep you want to go. The general case would involve parsing all sysusers.d entries in /usr and /etc, then doing equivalence check for Also keep in mind that we found several cases where the groupadd/useradd wouldn't match the sysusers.d content, and we are still actively working around those in FCOS until they are fixed in Fedora. My hope is that eventually scriptlets would stop directly calling groupadd/useradd, and we'd just use the packaged sysusers.d fragment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One final thought; should this be configurable? I hesitate to add a lot of options, but OTOH, in theory this could break someone, and it'd be handy to have an option to turn it off without downgrading.
Alternatively, it could be opt-in.
At some point we will definitely have a configuration knob to switch from nss-altfiles to plain systemd-sysusers. The logic in this PR though is not interesting to configure in itself. It shouldn't cause regressions (as the fragment are not really used right now). But I do understand your concerns. |
I'm fine with that. Could also be a hidden option to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
Some comments. I see this was approved already, so I'm fine deferring them to a followup.
@@ -84,6 +84,8 @@ static RpmOstreeCommand commands[] = { | |||
/* Rust-implemented commands; they're here so that they show up in `rpm-ostree | |||
* --help` alongside the other commands, but the command itself is fully | |||
* handled Rust side. */ | |||
{ "scriptlet-intercept", static_cast<RpmOstreeBuiltinFlags> (RPM_OSTREE_BUILTIN_FLAG_HIDDEN), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note if the command is hidden, there's no reason to have it here since it won't show up anyway in the --help
output.
|
||
// The filename of the configuration fragment is in fact a public | ||
// API, because users may have masked it in /etc. Do not change this. | ||
let filename = format!("30-pkg-group-{groupname}.conf"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, looking at /usr/lib/sysusers.d
in my local FSB and FCOS, I don't see any config using priority prefixes in their names (except Zincati). So even with 30-
, we'd be at the top of the list.
One third option I considered is the
{pkgname}-{groupname}
format, but I didn't see many additional benefits in doing that.
That'd be my vote. I think the additional benefit of being able to tell which package we derived from is nice (given that we don't currently include the package name in the generated fragment either). It would also better conform to the guideline established in sysusers.d(5)
:
Each configuration file shall be named in the style of
package.conf
orpackage-part.conf
. The second variant should be used when it is desirable to make it easy to override just this part of configuration.
(Though it's a bit fuzzy here because we're doing it for them, hehe, but it's in the same spirit I think).
} | ||
|
||
/// CLI parser, matches <https://linux.die.net/man/8/groupadd>. | ||
fn cli_cmd() -> Command<'static> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, shouldn't we error out if any of the switches we don't actually support are used? And I guess one way to do that is to just drop those switches from the clap definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely yes. I'll do that in a followup PR together with enforcing the --system
flag, as soon as I've verified that all packages in the FCOS base image are fine.
This re-routes `groupadd` calls in scriptlets, in order to inspect group creation by RPMs and automatically generate corresponding sysusers.d fragments. It uses a new hidden `scriptlet-intercept` subcommand, which can be re-used in the future to intercept additional commands (e.g. `useradd`). For the moment, this logic is experimental and gated behind an environment feature flag.
fa2cdc6
to
770b7e4
Compare
This re-routes
groupadd
calls in scriptlets, in order to inspectgroup creation by RPMs and automatically generate corresponding
sysusers.d fragments.
It uses a new hidden
scriptlet-intercept
subcommand, which canbe re-used in future to intercept additional commands (e.g.
useradd
).