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

System Ready Minimal Images fixes #137

Merged
merged 11 commits into from
Sep 13, 2023
Merged

System Ready Minimal Images fixes #137

merged 11 commits into from
Sep 13, 2023

Conversation

nullr0ute
Copy link
Contributor

These are some fixes for the minimal image that are needed to fix up a few minor issues.

Signed-off-by: Peter Robinson [email protected]

@nullr0ute
Copy link
Contributor Author

@sarmahaj FYI

@runcom
Copy link
Member

runcom commented Aug 25, 2023

Lgtm

ondrejbudai
ondrejbudai previously approved these changes Aug 28, 2023
@ondrejbudai ondrejbudai added this pull request to the merge queue Aug 28, 2023
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

I think that the PR is missing changes to

func minimalrpmPackageSet(t *imageType) rpmmd.PackageSet {
return rpmmd.PackageSet{
Include: []string{
"@core",
},
}
}
and nothing in the commits suggests otherwise

Also the Fedora minimal-raw does not reflect the changes to kernel command line arguments done in RHEL ->

minimalrawImgType = imageType{
name: "minimal-raw",
filename: "raw.img.xz",
compression: "xz",
mimeType: "application/xz",
packageSets: map[string]packageSetFunc{
osPkgsKey: minimalrpmPackageSet,
},
rpmOstree: false,
kernelOptions: defaultKernelOptions,
bootable: true,
defaultSize: 2 * common.GibiByte,
image: diskImage,
buildPipelines: []string{"build"},
payloadPipelines: []string{"os", "image", "xz"},
exports: []string{"xz"},
basePartitionTables: defaultBasePartitionTables,
}
)

defaultKernelOptions = "ro no_timer_check console=ttyS0,115200n8 biosdevname=0 net.ifnames=0"

@thozza thozza removed this pull request from the merge queue due to a manual request Aug 28, 2023
@miabbott
Copy link

Do these changes prevent the minimal-raw artifacts from being used on devices or are they nice to have?

Given that we are past dev-freeze for RHEL 9.3, I don't think we should be making changes to the minimal-raw type unless they are absolutely necessary.

@blomquisg
Copy link

I've asked around to figure out what is controversial about this change, and the main thing I've heard is the inclusion of initial-setup.

The workflow as I understand it is to use Image Builder to produce a bootable image from these minimal-raw images.

It sounds like the compromise here is to remove initial-setup from this PR, then document two available paths for users:

  1. Use the minimal-raw image and a blueprint with Image Builder to create an initial user account, or
  2. Add initial-setup back using Image Builder.

If users are looking for an experience that's similar to building other images with Image Builder, they would choose option 1. If users are looking to test out a simple bootable image and don't mind manually configuring the installation, they could choose option 2.

Thoughts?

/cc @nullr0ute @thozza @runcom @miabbott @mrguitar

@nullr0ute
Copy link
Contributor Author

nullr0ute commented Aug 31, 2023

I've asked around to figure out what is controversial about this change, and the main thing I've heard is the inclusion of initial-setup.

The workflow as I understand it is to use Image Builder to produce a bootable image from these minimal-raw images.

That's the first step of the whole process, the next step is to provision it onto the device.

It sounds like the compromise here is to remove initial-setup from this PR, then document two available paths for users:

I did not take that conclusion away from yesterday's meeting. The decision from what I understand it was "what ever we do we can't change it once it's shipped". Also they're not mutually exclusive.

1. Use the minimal-raw image and a blueprint with Image Builder to create an initial user account, or

This will cause potential security implications. We don't ship default credentials because of that.

2. Add `initial-setup` back using Image Builder.

If users are looking for an experience that's similar to building other images with Image Builder, they would choose option 1. If users are looking to test out a simple bootable image and don't mind manually configuring the installation, they could choose option 2.

That depends on the usecase. In the case of 1) that may be useful for a single user, but it falls over if it's being provided to end users, partners or other cases.

@nullr0ute
Copy link
Contributor Author

I have found more fixes needed so I'm going to update this PR with more commits soon once I've worked it all out. New update soon.

@nullr0ute
Copy link
Contributor Author

I think that the PR is missing changes to
and nothing in the commits suggests otherwise

You are correct, but TBH this was never meant to land for el8 as these devices will only ever be supported on el9, but I will fix now it's there, I thought the package set was the same across both, I missed the 9 in the name.

Also the Fedora minimal-raw does not reflect the changes to kernel command line arguments done in RHEL ->

And I missed that variable, I have other fixes coming in too, will update.

@miabbott
Copy link

I did not take that conclusion away from yesterday's meeting. The decision from what I understand it was "what ever we do we can't change it once it's shipped". Also they're not mutually exclusive.

Correct; we want to understand what (if any) changes are required in order for users/customers to be able to boot devices using this artifact type.

That depends on the usecase. In the case of 1) that may be useful for a single user, but it falls over if it's being provided to end users, partners or other cases.

I'd like to understand what would be the more common use case for this artifact.

Even if it is the latter use case, including initial-setup by default would cause this artifact to be the unicorn among all the other artifacts produced by osbuild. Maybe that is the decision we come to, but it does feel anti-pattern compared to how most users will handle user provisioning/configuration when using osbuild.

The other consideration when adding packages to the default set is that end-users have no means of removing those default packages as part of the osbuild compose process, so we need to be very certain that any defaults are going to be applicable to the majority of users.

Regardless of the decisions made about this PR, we will need strong documentation explaining the outcome and how user provisioning/configuration will be done with this artifact.

@nullr0ute
Copy link
Contributor Author

Even if it is the latter use case, including initial-setup by default would cause this artifact to be the unicorn among all the other artifacts produced by osbuild. Maybe that is the decision we come to, but it does feel anti-pattern compared to how most users will handle user provisioning/configuration when using osbuild.

For osbuild maybe, but from an install experience on devices (AKA phase two in this process) the user generally does have to deal with users and root passwords in the traditional installer (anaconda) use case, whether that be specifying it in the UX doing a "CD style installer (via traditional media or USB stick)" or as an option in the kickstart so for the "install on device" part it is not differing from the standard user experience. If say Partner Management provided a image to a partner to put on a device the partner would never deal with osbuild so from their perspective it would not be different because with anaconda you have to specify those pieces. In fact if you do that install over a serial or text console it's that exact same UX that is shown to the user.

@nullr0ute
Copy link
Contributor Author

Documentation for RHEL-8 here: https://access.redhat.com/articles/4323781

@miabbott
Copy link

For osbuild maybe, but from an install experience on devices (AKA phase two in this process) the user generally does have to deal with users and root passwords in the traditional installer (anaconda) use case, whether that be specifying it in the UX doing a "CD style installer (via traditional media or USB stick)" or as an option in the kickstart so for the "install on device" part it is not differing from the standard user experience.

If the user composed an image using osbuild and the blueprint was missing any user customizations, then yes they would have to handle it during the install phase. But that can be avoided if the blueprint customization is used.

If say Partner Management provided a image to a partner to put on a device the partner would never deal with osbuild so from their perspective it would not be different because with anaconda you have to specify those pieces. In fact if you do that install over a serial or text console it's that exact same UX that is shown to the user.

I think this use case is where I would dig into...is the most common use case for minimal-raw going to be something like this? What happens if someone provides user customization in a blueprint for a minimal-raw image? Will initial-setup still run and have to be dealt with?

@mrguitar
Copy link

Documentation for RHEL-8 here: https://access.redhat.com/articles/4323781

That kbase describes how to answer the questions in the TUI. That part is fine and self explanatory. It's more about making the call to force this on users by default, when blueprints make it crazy trivial to add it when you need it. We don't have any product plans to distribute these images so I don't know why we care if customers embed users/credentials. I don't particularly like it, but it's super common (like a million other bad security practices that user do and we don't prohibit). Any images we make for hardware partners can easily include this in a blueprint (which we have to make anyway to include the HTS packages).

I suspect there's a unit file we can disable/mask to stop this from running but I haven't cracked open the rpm to look at it yet. .....I still think it's a bad experience to surprise users with a bespoke experience here and expect them to figure out how to skip initial-setup when they want something more streamlined. I think we'd all agree that a manual process like this doesn't scale for any real deployments. I can see this being a useful image type for embedded style systems ....but not if they need hands-on-keys to complete a first boot. Why push for that? That feels useless and very limiting to me.

@nullr0ute
Copy link
Contributor Author

For osbuild maybe, but from an install experience on devices (AKA phase two in this process) the user generally does have to deal with users and root passwords in the traditional installer (anaconda) use case, whether that be specifying it in the UX doing a "CD style installer (via traditional media or USB stick)" or as an option in the kickstart so for the "install on device" part it is not differing from the standard user experience.

If the user composed an image using osbuild and the blueprint was missing any user customizations, then yes they would have to handle it during the install phase. But that can be avoided if the blueprint customization is used.

The question then becomes how many people will take the defaults vs customising it? I suspect the answer to that is impossible to predict BUT we do need to provide SANE defaults, if a user does nothing in the blueprint as it stands now they will have an image that they have no way to login to, that is confusing and I am absolutely against providing a default username and password (as I'm sure product security will be as well), on the other side of that in Fedora we have had no issues with users understanding how to use initial-setup.

If say Partner Management provided a image to a partner to put on a device the partner would never deal with osbuild so from their perspective it would not be different because with anaconda you have to specify those pieces. In fact if you do that install over a serial or text console it's that exact same UX that is shown to the user.

I think this use case is where I would dig into...is the most common use case for minimal-raw going to be something like this? What happens if someone provides user customization in a blueprint for a minimal-raw image? Will initial-setup still run and have to be dealt with?

Well the intention of the minimal-raw was intended for basic development, board bring up etc, which would generally be a single user..... BUT like all these things predicting what is intended vs how customers end up using things is impossible to predict.

@nullr0ute
Copy link
Contributor Author

Documentation for RHEL-8 here: https://access.redhat.com/articles/4323781

That kbase describes how to answer the questions in the TUI. That part is fine and self explanatory. It's more about making the call to force this on users by default, when blueprints make it crazy trivial to add it when you need it. We don't have any product plans to distribute these images so I don't know why we care if customers embed users/credentials. I

What happens if they don't specify a username/password in the blueprint? They can't login. In the case with initial-setup they should be able to just exit and login, or just ssh in straight up if they're doing it remotely. Also initial-setup may just exit if a user or root password is set (I need to verify this, because it's nuanced and I don't remember exact details, it may just not make them create it and they can just continue).

@mrguitar
Copy link

What happens if they don't specify a username/password in the blueprint? They can't login. In the case with initial-setup they should be able to just exit and login, or just ssh in straight up if they're doing it remotely. Also initial-setup may just exit if a user or root password is set (I need to verify this, because it's nuanced and I don't remember exact details, it may just not make them create it and they can just continue).

Honestly, neither option sounds great. :(

I cracked the package open and I believe this will skip the "wizard" in a blueprint (or something similar).

[customizations]

[[customizations.user]]
name = "core"
description = "core"
password = "hash"
key = "ssh-rsa AAAAB3NzaC"
groups = ["wheel"]

[customizations.services]
disabled = ["initial-setup", "initial-setup-reconfigure"]

That's probably overkill because there's a ConditionPathExists=/.unconfigured in the initial-setup-reconfigure unit file. I don't see that file as part of the rpm, so I'm not sure if a post script creates it. My guess is the firstboot --reconfigure (from memory so could be wrong) kickstart options tickles this. I know we can create files in the blueprint, but I don't know if we can remove or "untouch" a file like this. A drop-in feels more complicated and a worse customer experience to override. I think knowing the easiest/best way to bypass initial-setup would really help us weight the pros & cons here.

If I can dream a bit, an even better experience would be keeping the defaults @nullr0ute provided and empowering users to override/remove the base packages via a blueprint (or other method). That way we get away from hard coding things and backing ourselves (and customers) into corners. This causes so much pain right now particularly with packages like podman, when users don't need/want containers. We can still provide support because we can point to an image override if something doesn't work as expected.

...Just thinking out loud here, and unfortunately we don't live in this world w/ 9.3 around the corner.

@blomquisg
Copy link

I'm mostly stuck on various types of users for the minimal-raw image. Looking back over the comments, it seems to be a mostly open question, honestly. We have some ideas of some users, but I don't see anything concrete.

That said, if we have a definite way to bypass the initial-setup tui, and we can document that somewhere, then I guess it's less of a big deal. But, that needs to be included in testing this before we release it. If including initial-setup necessarily drops the user into an interactive tui, that feels like a miss.

Honestly, since we'll have to document the user interaction either way (whether we have to show them how to skip initial-setup with a blueprint, or how the user would add initial-setup back in via Image Builder), I would lean towards including fewer packages, and document how to build an image that includes what they need. But, this goes back to the understanding who we think will use the minimal-raw, and how.

@nullr0ute nullr0ute force-pushed the srir-fixes branch 3 times, most recently from 9f0cb71 to 7201a05 Compare August 31, 2023 21:42
@nullr0ute
Copy link
Contributor Author

I think that the PR is missing changes to

I think these are now all fixed, along with a bunch of other changes :)

@thozza
Copy link
Member

thozza commented Sep 1, 2023

That's probably overkill because there's a ConditionPathExists=/.unconfigured in the initial-setup-reconfigure unit file. I don't see that file as part of the rpm, so I'm not sure if a post script creates it. My guess is the firstboot --reconfigure (from memory so could be wrong) kickstart options tickles this. I know we can create files in the blueprint, but I don't know if we can remove or "untouch" a file like this. A drop-in feels more complicated and a worse customer experience to override. I think knowing the easiest/best way to bypass initial-setup would really help us weight the pros & cons here.

No, there is no option to delete files from the image.

From what I can tell, disabling the initial-setup.service unit looks like the most reasonable way to disable initial-setup from running in the image on the first boot.

If I can dream a bit, an even better experience would be keeping the defaults @nullr0ute provided and empowering users to override/remove the base packages via a blueprint (or other method). That way we get away from hard coding things and backing ourselves (and customers) into corners. This causes so much pain right now particularly with packages like podman, when users don't need/want containers. We can still provide support because we can point to an image override if something doesn't work as expected.

I don't see this happening any time soon (or at all). We intentionally don't want users to be able to exclude packages from the image base package set. @supakeen is experimenting with an "empty" image in Fedora and an option to exclude packages in BP, but that would still be possible only with user-specified packages, not the base package set.

On a general note, all image types produced by image builder come without any user by default. Things are different for various image types, but in general there is always some form of mechanism for creating users on the first boot (cloud-init, ignition, cloud-specific guest tools, etc.).

So having an image type which does not have any default user (which is desired), does not have any way to create it on the first-boot and needs a user in order to use the OS (e.g. container images don't need one) is an anti-pattern from my PoV and something that would not make sense...

@supakeen
Copy link
Member

supakeen commented Sep 1, 2023

Looks OK in my book; I agree with @thozza on the users/initial-setup.

@nullr0ute Are these images to be used on Raspberry Pi's as well (at least for Fedora)? If so you might want to do something similar as done for Aarch64_IoT in distro/fedora/distro.go.

Add WiFi support and initial-setup to create users. The image
should have basic WiFi support as a lot of the devices have
WiFi so being able to use it out of the box is important. The
majority of the WiFi firmware is currently in linux-firmware but
also add the most common Intel cards too.

Signed-off-by: Peter Robinson <[email protected]>
It's useful for a number of usecases to be able to
specify a starting offset for the first partition on
disk. This is needed for some arm images to allow
space for firmware, but it's also often needed for
virt images so VMs are optimally places on disk.

Fix an issue where the starting offset isn't set for
partition offsets.

Signed-off-by: Peter Robinson <[email protected]>
@nullr0ute nullr0ute force-pushed the srir-fixes branch 2 times, most recently from 6474f88 to 9e57df8 Compare September 8, 2023 16:22
A number of edge devices, in most cases pre-prod that may be on
the edge of proper certification, need to write the firmware to
the beginning of the storage, while this is far from ideal the
default 2048 blocks (1Mb) is generally not enough and it means
when the firmware is written out we will zap the EFI ESP
partition so let's start this with an offset of 8Mb for
these images to ensure we have enough space for those (hopefully
rare) use cases.

Signed-off-by: Peter Robinson <[email protected]>
By default there's no user and the root account should be
locked so we need to provide a means for images that maybe
distributed to enable a user to create accounts to login
as we don't want to, by default, unlock the root account
and provide default credentials as that's a major security
issue. If a user doesn't add an account in a blueprint
they end up with an unusable image.

We add libxkbcommon as it's required by i-s but isn't
pulled in.

Signed-off-by: Peter Robinson <[email protected]>
Fix the default Fedora kernel command line. It was advertised as the
default but the reality is the options were for cloud, so set a real
default as the option and update the cloud options to something
special for them. It's not truly correct as at least one of the
options is x86 specific but when booted on other architectures those
kernels will ignore it (and I honestly think it's ancient cargo
culted kernel command line options).

Signed-off-by: Peter Robinson <[email protected]>
A number of bits of WiFi firmware have changed in Fedora, with
some moved out of linux-firmware, and some Intel ones being
renamed so update to the current naming which is valid for all
currently supported releases.

Signed-off-by: Peter Robinson <[email protected]>
A number of edge devices, in most cases pre-prod that may be on
the edge of proper certification, need to write the firmware to
the beginning of the storage, while this is far from ideal the
default 2048 blocks (1Mb) is generally not enough and it means
when the firmware is written out we will zap the EFI ESP
partition so let's start this with an offset of 8Mb for
these images to ensure we have enough space for those (hopefully
rare) use cases.

Also the Fedora image, due to support requirements around devices
like the Raspberry Pi 3 require the default partition tables
format to be msdos because the initial program loader in silicon
doesn't support loading the firmware off a GPT partition table.

Signed-off-by: Peter Robinson <[email protected]>
Add WiFi support and initial-setup to create users. The image
should have basic WiFi support as a lot of the devices have
WiFi so being able to use it out of the box is important. The
majority of the WiFi firmware is currently in linux-firmware but
also add the most common Intel cards too.

We need inital-setup to create users, set root password etc as
we don't want to be using default user/password as it's a major
security risk. We add libxkbcommon as i-s needs it.

We need to start default services to ensure the right things run
and we end up with things like firewalls and networks.

Finally we need to setup the firmware for the RPi.

Signed-off-by: Peter Robinson <[email protected]>
@nullr0ute
Copy link
Contributor Author

OK I think this is ready for review and merge, just generating test images.

Support adding fsnodes to the ImageConfig so we can define arbitrary
files as part of the base config.

This should be avoided and only used as a last resort or short term
workaround.
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

LGTM but I wont approve since I wrote some of the changes myself. I'd like to get an extra pair of eyes on this before merging.

@nullr0ute
Copy link
Contributor Author

OK I think this is ready for review and merge, just generating test images.

I've tested both RHEL and Fedora images on an imx8m device and works as expected 🥳

Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

I added a few minor comments, but in general this looks OK.

@nullr0ute I don't see any partition table start offset being set for RHEL-8 image. Is this intentional?

pkg/platform/aarch64.go Show resolved Hide resolved
pkg/distro/fedora/images.go Outdated Show resolved Hide resolved
@thozza thozza dismissed achilleas-k’s stale review September 11, 2023 10:56

Commented "LGTM" later in the PR...

Currently initial-setup requires a kickstart file in the /root directory
to work.  Adding a temporary file node to the image config for
minimal-raw images so that initial-setup runs and a user can be created
if necessary.  The minimal-raw image has no other mechanism for creating
a user if one wasn't specified at build time, so we need to be sure that
initial-setup runs.
@mrguitar
Copy link

@thozza RHEL 8 doesn't have the necessary aarch64 kernel enablement for minimal-raw to be viable. I don't believe that's a target. I suspect that's why it's missing here.

@nullr0ute
Copy link
Contributor Author

@nullr0ute I don't see any partition table start offset being set for RHEL-8 image. Is this intentional?

Yes, RHEL-8 only supports ServerReady and that doesn't need the offsets if people wish to use it on that release.

@nullr0ute
Copy link
Contributor Author

I had planned, had a brief discussion with @achilleas-k, on doing some more cleanups around the Fedora pieces on a some follow up clean ups (I have some other ideas for cleanups too), the el8 offset one has now been answered, what's left?

@thozza
Copy link
Member

thozza commented Sep 13, 2023

I had planned, had a brief discussion with @achilleas-k, on doing some more cleanups around the Fedora pieces on a some follow up clean ups (I have some other ideas for cleanups too), the el8 offset one has now been answered, what's left?

Nothing, I got all the answers I needed... Approved...

@thozza thozza added this pull request to the merge queue Sep 13, 2023
Merged via the queue into osbuild:main with commit 9f7c75e Sep 13, 2023
9 checks passed
@miabbott
Copy link

on doing some more cleanups around the Fedora pieces on a some follow up clean ups (I have some other ideas for cleanups too),

@nullr0ute could you file issues for these changes? It would enable us to get others working on the changes while you are on PTO (or even when you are not on PTO).

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.