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

Add ignition-apply entrypoint #1285

Merged
merged 2 commits into from
Feb 25, 2022
Merged

Add ignition-apply entrypoint #1285

merged 2 commits into from
Feb 25, 2022

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Nov 22, 2021

This new entrypoint takes an Ignition config and applies it to the
current rootfs (or one provided by -root). This tool could then be
used as part of build processes where we want to bake an Ignition
config, such as in CoreOS layering:

https://github.com/coreos/enhancements/blob/main/os/coreos-layering.md

This in turn should also help drain from the MCO the re-implementation
of large parts of the Ignition spec.

Another approach would be to build on top of ignition -stage=files,
but in the end I think we want a cleaner dedicated UX for this workflow
long-term (and in fact we should clean up the hacky places where we call
Ignition like this).

It shouldn't be used by end-users to live apply changes to their
Ignition config. A check that we are in a container is added as a
safeguard against this.

By default, unsupported modifications (like partitioning) trigger
errors, though one may pass -ignore-unsupported to override this. Also
by default, remote resources are automatically fetched, though one may
pass -offline to override this.

@jlebon
Copy link
Member Author

jlebon commented Nov 22, 2021

Example:

$ jq . config.ign
{
  "ignition": {
    "version": "3.2.0"
  },
  "storage": {
    "files": [
      {
        "path": "/etc/fedora-coreos/iptables-legacy.stamp",
        "mode": 420
      }
    ]
  }
}

$ mkdir rootfs

$ ./ignition-liveapply -root rootfs config.ign
INFO     : Ignition v2.12.0-46-gfdafbe53-dirty
INFO     : createFilesystemsFiles: createFiles: op(1): [started]  writing file "rootfs/etc/fedora-coreos/iptables-legacy.stamp"
INFO     : createFilesystemsFiles: createFiles: op(1): [finished] writing file "rootfs/etc/fedora-coreos/iptables-legacy.stamp"

$ find rootfs
rootfs
rootfs/etc
rootfs/etc/fedora-coreos
rootfs/etc/fedora-coreos/iptables-legacy.stamp

@jlebon
Copy link
Member Author

jlebon commented Nov 22, 2021

Lots of prep patches to split out still, but putting this up for any early high-level feedback.

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks so much for hacking on this!

build Outdated

echo "Building ${NAME}..."
go build -ldflags "${GLDFLAGS}" -o ${BIN_PATH}/${NAME} ${REPO_PATH}/validate
for bin in validate liveapply; do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One downside of a new binary is it's going to basically be another one of these right?
-rwxr-xr-x. 2 root root 22M Jan 1 1970 /usr/lib/dracut/modules.d/30ignition/ignition

I think perhaps we should have a tiny shell script like:

#!/usr/bin/bash
exec /usr/lib/dracut/modules.d/30ignition/ignition live-apply

as /usr/sbin/ignition-live-apply or so?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, do we actually want this in the OS though? I wasn't planning to push for that. For CoreOS layering we could use it as part of a multi-stage build. In which case having it be independent of the main Ignition binary would be nicer.

build Outdated Show resolved Hide resolved
internal/exec/stages/files/files.go Outdated Show resolved Hide resolved
internal/exec/engine.go Outdated Show resolved Hide resolved
@bgilbert
Copy link
Contributor

This is intended for a build process that pre-applies an Ignition config to a directory tree, right? If so, I worry that the name liveapply suggests that it's intended for updating existing systems, which I don't think we actually want. As such, I'd suggest renaming it (ignition-apply-to-directory, or preferably something better) and erroring out if -root is set to /.

If we really want the ability to affect the running system, I'd suggest requiring a scary option like -ignition-is-not-designed-for-this alongside -root /. We should probably think carefully before doing so, though, since it opens the door to Ignition having to handle runtime races that haven't been a factor so far.

@jlebon
Copy link
Member Author

jlebon commented Nov 23, 2021

This is intended for a build process that pre-applies an Ignition config to a directory tree, right? If so, I worry that the name liveapply suggests that it's intended for updating existing systems, which I don't think we actually want. As such, I'd suggest renaming it (ignition-apply-to-directory, or preferably something better) and erroring out if -root is set to /.

If we really want the ability to affect the running system, I'd suggest requiring a scary option like -ignition-is-not-designed-for-this alongside -root /.

The primary use case for this is CoreOS layering where we do want to affect / (the container rootfs), so it'd be unfortunate to require a switch for it. That said, we can probably have it detect the container case and not require it if so? Seems like a un-Ignition thing to do though.

We should probably think carefully before doing so, though, since it opens the door to Ignition having to handle runtime races that haven't been a factor so far.

I think we can declare that not supported (and as mentioned higher up, not even ship it in the base at all).

@bgilbert
Copy link
Contributor

The primary use case for this is CoreOS layering where we do want to affect / (the container rootfs), so it'd be unfortunate to require a switch for it. That said, we can probably have it detect the container case and not require it if so? Seems like a un-Ignition thing to do though.

Ah, okay. Not shipping in the base is a good signal for FCOS/RHCOS users, but doesn't really help for other distros or for people building Ignition themselves. Since this is a such a large footgun in a potentially user-facing tool (i.e., a binary that isn't embedded in the initramfs), I'd be inclined to detect the container case if that's not too difficult.

liveapply/main.go Outdated Show resolved Hide resolved
@miabbott
Copy link
Member

This functionality was discussed as part of our internal demo meeting today and there was concern that users/customers would attempt to abuse the functionality on a running system and get themselves into trouble. Implementing some safeguards to discourage this from happening would be useful, i.e. checking for /run/ostree-booted or checking if the rootfs target was /.

@runcom
Copy link
Member

runcom commented Dec 1, 2021

This could be helpful even in the OS (for safe things like files and such) - anybody can use ignition as a backend on a live system at configuration time (I’ve read the concerns ofc, but pretty much like the MCO did, people reimplements basic things that ignition already does)

@LorbusChris
Copy link
Contributor

+1 @runcom. Not sure if Fedora IoT still runs Ignition on the real root, but this could be viable alternative for them.

@jlebon
Copy link
Member Author

jlebon commented Dec 17, 2021

So, was playing a bit with how this would work UX wise as part of CoreOS layering, and the multi-stage build idea I had in mind is more awkward than I expected. So now I'm thinking we do bake this into the host.

I think perhaps we should have a tiny shell script like:

How about instead we turn the primary Ignition binary into a multicall app and do something similar to the approach we're discussing in coreos/fedora-coreos-tracker#1050: shove the hardlinked binary somewhere outside of PATH and wrap it via some rpm-ostree command. (I guess we could even skip the hardlinked binary to make it even harder to use and have rpm-ostree set arg0? Makes it more annoying to test though.)

@jlebon
Copy link
Member Author

jlebon commented Jan 7, 2022

How about instead we turn the primary Ignition binary into a multicall app and do something similar to the approach we're discussing in coreos/fedora-coreos-tracker#1050: shove the hardlinked binary somewhere outside of PATH and wrap it via some rpm-ostree command. (I guess we could even skip the hardlinked binary to make it even harder to use and have rpm-ostree set arg0? Makes it more annoying to test though.)

Updated this PR to do this now, but still need to test the full flow!

@jlebon jlebon changed the title WIP: Add ignition-liveapply binary Add ignition-liveapply entrypoint Feb 2, 2022
@jlebon jlebon marked this pull request as ready for review February 2, 2022 19:59
@jlebon
Copy link
Member Author

jlebon commented Feb 2, 2022

Rebased, fixed error handling in a few places, enhanced container environment checking, appeased golangci-lint, and dropped WIP status!

Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit about the framing of the commit message: I don't think we have plans to migrate toward greater use of multicall. So I'd think of this as "add a multicall entrypoint for this use case" and not "switch to multicall generally".

It sounds like we don't plan to ship even a symlink to the liveapply name upstream, but rather it'll be the package's job to expose that if desired?

internal/main.go Outdated Show resolved Hide resolved
internal/main.go Outdated Show resolved Hide resolved
internal/main.go Outdated Show resolved Hide resolved
internal/main.go Outdated Show resolved Hide resolved
internal/liveapply/liveapply.go Outdated Show resolved Hide resolved
internal/exec/stages/files/files.go Outdated Show resolved Hide resolved
internal/exec/stages/files/files.go Show resolved Hide resolved
internal/exec/stages/files/files.go Outdated Show resolved Hide resolved
internal/exec/stages/files/files.go Outdated Show resolved Hide resolved
internal/liveapply/liveapply.go Outdated Show resolved Hide resolved
@jlebon jlebon force-pushed the pr/apply branch 2 times, most recently from 0e8706c to ddf1dc9 Compare February 24, 2022 21:40
This new entrypoint takes an Ignition config and applies it to the
current rootfs (or one provided by `-root`). This tool could then be
used as part of build processes where we want to bake an Ignition
config, such as in CoreOS layering:

https://github.com/coreos/enhancements/blob/main/os/coreos-layering.md

This in turn should also help drain from the MCO the re-implementation
of large parts of the Ignition spec.

Another approach would be to build on top of `ignition -stage=files`,
but in the end I think we want a cleaner dedicated UX for this workflow
long-term (and in fact we should clean up the hacky places where we call
Ignition like this).

It shouldn't be used by end-users to live apply changes to their
Ignition config. A check that we are in a container is added as a
safeguard against this.

By default, unsupported modifications (like partitioning) trigger
errors, though one may pass `-ignore-unsupported` to override this. Also
by default, remote resources are automatically fetched, though one may
pass `-offline` to override this.
@jlebon jlebon changed the title Add ignition-liveapply entrypoint Add ignition-apply entrypoint Feb 25, 2022
@jlebon jlebon merged commit d554570 into coreos:main Feb 25, 2022
@jlebon jlebon deleted the pr/apply branch February 25, 2022 19:23
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.

None yet

6 participants