Skip to content
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

WIP: Add /etc/rpm-ostree/roothooks.d #1882

Closed
wants to merge 1 commit into from
Closed

Conversation

cgwalters
Copy link
Member

@cgwalters cgwalters commented Aug 9, 2019

This is intended to support kernel module systems like
atomic-wireguard.
See the Fedora CoreOS tracker issue:
coreos/fedora-coreos-tracker#249

With a "roothook", one can perform arbitrary modifications to the new root
filesystem; if a hook exits with an error, that also stops the upgrade.

Specifically with this, atomic-wireguard could block an upgrade if
the new kernel isn't compatible with a module.

@lucab
Copy link
Contributor

lucab commented Aug 9, 2019

Container Linux had a similar (but less powerful) hooking point in update-engine, which was the last running logic before marking an update as locally available.
If you want to glimpse into the useful ways to bend this with very creative approaches, see https://github.com/coreos/update_engine/blob/master/coreos-postinst.

@jlebon
Copy link
Member

jlebon commented Aug 13, 2019

Hmm, if we're running the hooks directly without any containerization, then doesn't that undermine all the safeties we enforce to prevent runtime modifications?

Another approach is to require all hooks to run in containers where we pass only the target rootfs through a bind-mount?

Edit: more discussions in coreos/fedora-coreos-tracker#249 (comment).

@cgwalters
Copy link
Member Author

Hmm, if we're running the hooks directly without any containerization, then doesn't that undermine
all the safeties we enforce to prevent runtime modifications?

The hook should only operate on the target root. We still have e.g. the baseline ostree protections like the read-only /usr mount in place for the current root. But that said nothing stops the hook from killing processes or loading kernel modules etc.

Another approach is to require all hooks to run in containers where we pass only the target rootfs through a bind-mount?

I would expect most hooks to run containers, but requiring it would be quite a jump in interface complexity. Would we have something like pod spec YAML? This would also tie us quite closely to docker/OCI and make hooks unusable without that.

I guess with this the vision is more that we add an interface with ultimate flexibility - but we'd document a good best practice for hooks would be a simple shell script that did exec podman run ...?

@rh-atomic-bot
Copy link

☔ The latest upstream changes (presumably c279f92) made this pull request unmergeable. Please resolve the merge conflicts.

@cgwalters
Copy link
Member Author

Though, a downside of this is we'll be running without rofiles-fuse protection, so a hook could corrupt the ostree repo. Hmm. Yeah, we probably need that.

@cgwalters
Copy link
Member Author

Though, a downside of this is we'll be running without rofiles-fuse protection, so a hook could corrupt the ostree repo. Hmm. Yeah, we probably need that.

This is somewhat nasty to implement though, our rofiles-fuse code is tied into the bwrap stuff; and if we use that we make it way harder to have your script invoke podman or whatever.

Another option is to make the new tree read-only, and require the hook to output a manual "overlay" into a new directory.

It is actually tempting to try to match containers/storage and use the dual overlayfs (when running as root) and fuse-overlayfs path (when running unpriv)...but that's some unknown-difficulty probably-invasive work.

@cgwalters
Copy link
Member Author

One interesting thing about this hook is that in rpm-ostree there's a distinction between "check for updates" and "apply updates" - for example, if you rpm-ostree install foo, rpm-ostree upgrade will check for updates to foo and create a new commit even if the base tree didn't change. And just as important, we won't do anything if neither foo nor the base changed.

That's a hugely important property.

Another way to say this is that we go to a lot of effort to handle this instead of just having an imperative Dockerfile where we have no idea what's going on inside.

Anyways, this hook doesn't support that kind of thing - it can only mutate or block an already created tree.

I was thinking about trying to generalize this to do so...but it's not necessary for kernel module work. And the more I think about it...what we could do for that is add an updatehooks.d that is defined to generate one or more overlay layers. Oh, and we could do that in parallel too.

But we'd need a more sophisticated "protocol" to do that with an external process. Hmm. Maybe not too bad to do so.

On the other hand again, not needed for kmods, and this should straightforwardly help that.

@cgwalters cgwalters changed the title WIP: Add /etc/rpm-ostree/hooks.d WIP: Add /etc/rpm-ostree/roothooks.d Aug 23, 2019
@cgwalters
Copy link
Member Author

cgwalters commented Aug 23, 2019

And the more I think about it...what we could do for that is add an updatehooks.d that is defined to generate one or more overlay layers.

And...we'd need to track which overlay layers were created by hooks, to properly handle garbage collection. Maybe...one idea is that we write /usr/share/rpm-ostree/hooks.json into the final tree, which contains the hook refs (+commit) that were used for that final commit. Then, we can do the same thing we do for layered packages - perform GC by iterating over all deployments and pruning hook refs which aren't referenced by an active deployment.

@cgwalters
Copy link
Member Author

Container Linux had a similar (but less powerful) hooking point in update-engine, which was the last running logic before marking an update as locally available.

The "less powerful" here being e.g. that it can't mutate the OS filesystem right?

@lucab
Copy link
Contributor

lucab commented Aug 27, 2019

@cgwalters partially.
It can mutate the rootfs, with the exception of the RO /usr. But I think that part of the design is fine, as that path is for distro content.

I was referring to the facts that:

  • it's a single hooking point, so it doesn't support multiple snippets with priorities
  • it's shipped as part of the USR image, so the final user has no way to plug its customizations in there
  • it runs in the context of the old OS, so it manually tries to locate and run binaries from the new image

@cgwalters
Copy link
Member Author

It can mutate the rootfs, with the exception of the RO /usr

That's a big exception of course (and a big difference between ostree and update_engine).

it runs in the context of the old OS, so it manually tries to locate and run binaries from the new image

Ah, but this is also true of this PR; do you think that's OK? What code in that script runs new binaries? Is it everything referencing ${INSTALL_MNT}?

This is intended to support kernel module systems like
[atomic-wireguard](https://github.com/jdoss/atomic-wireguard).
See the Fedora CoreOS tracker issue:
coreos/fedora-coreos-tracker#249

With a "roothook", one can perform arbitrary modifications to the *new* root
filesystem; if a hook exits with an error, that also stops the upgrade.

Specifically with this, atomic-wireguard could *block* an upgrade if
the new kernel isn't compatible with a module.
@jlebon
Copy link
Member

jlebon commented Sep 4, 2019

OK, there's a lot of context here and I may be missing some still.

So here are my main concerns with a general hook like this:

  • There's no transparency into what it's actually doing. So ISTM to compromise the "controlled mutability" pitch.
  • It's an imperative approach, whereas the rest of rpm-ostree tries to be declarative. (E.g. a lot of work in fact went into moving rpm-ostree install from being imperative to declarative, rpm-ostree initramfs is similar, and relatedly, in the rest of Fedora there's a push for getting rid of scriptlets.)
  • It's a gateway to introducing upgrade and compatibility hacks (which I think the script @lucab pointed to illustrates well), whereas our posture with RHCOS/FCOS has been to avoid being in a position of carrying compat hacks by having more control over the upgrade path of the node through Cincinnati.

That said, I definitely agree with the need for "strong binding" mentioned in this comment. If we want to solve the kmod issue, can we instead work on defining semantics around how rpm-ostree can support that specifically, optionally even building the kmod itself? (E.g. I think it'd be pretty cool if rpm-ostree status actually printed something like KernelModules: wireguard).

@cgwalters
Copy link
Member Author

It's an imperative approach, whereas the rest of rpm-ostree tries to be declarative.

Yeah, and as noted above declarative is a necessity to integrate with upgrade in such a way that change detection, rendering status etc. works.

But, those things are not necessary for kmods.

(E.g. I think it'd be pretty cool if rpm-ostree status actually printed something like KernelModules: wireguard).

Right. But...such a thing would require driving a lot of "policy" into rpm-ostree that doesn't exist today. We'd have to make decisions around how kernel modules are built in a way that would influence how people build rpm-ostree using operating systems.

(On this topic of course, Silverblue already shipped parts of this, in a way that I'm not sure we want to do for FCOS derivatives, although it's not out of the question I guess)

@rh-atomic-bot
Copy link

☔ The latest upstream changes (presumably 553be6e) made this pull request unmergeable. Please resolve the merge conflicts.

@cgwalters
Copy link
Member Author

Ultimately I guess the question is: should rpm-ostree upgrade try to work at all in the presence of 3rd party modules, or does it need to be wrapped by some tool which is then driving the whole logic? It seems like we're leaning towards the latter which...is OK by me if there's not any consensus to ship this. This approach is certainly ugly and full of various drawbacks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants