Allow overriding OSImageURL with a layered image #3272

jkyros · 2022-07-28T17:00:21Z

This allows OSImageURLin MachineConfig to be overridden with a layered image.

This isn't entirely complete, I'm still working on cleaning it up, but:

the functionality does work
I'd like to see what e2e tests this breaks 😄

In #3258 I was more worried about dealing with the image in the payload, but since that seems like it might be blocked for a little while, we'll start with this and then we can go back and connect those tubes later.

Credit where credit is due, @yuqi-zhang started doing this against the layering branch over here back in February: #2939

This is in service to: MCO-291

yuqi-zhang

Overall looking good! Some questions/nits below

Also as a general concern, none of this would affect RHEL since the gating would prevent us going into any OS update code paths, right?

yuqi-zhang · 2022-07-28T20:34:24Z

pkg/controller/common/helpers.go

+ // For layering, we want to let the user override OSImageURL again
+ for _, cfg := range configs {
+ if cfg.Spec.OSImageURL != "" {
+ osImageURL = cfg.Spec.OSImageURL


Just curious, is our current plan to override the base OSImageURL as opposed to a new field?

minor nit: there is a comment at the start of the function
// It uses only the OSImageURL provided by the CVO and ignores any MC provided OSImageURL.
we should probably remove

Maybe I let Colin twist my arm too hard, but yes, the current plan was to let the user "take the wheel" and override OSImageURL. 😄

I updated the comment to match this PR's behavior.

yuqi-zhang · 2022-07-28T20:36:08Z

pkg/daemon/rpm-ostree.go

@@ -148,6 +149,15 @@ func (r *RpmOstreeClient) GetBootedOSImageURL() (string, string, error) {
 }
 }

+ // we have container images now, make sure we can parse those too
+ if bootedDeployment.ContainerImageReference != "" {
+ // right now they start with "ostree-unverified-registry:", so scrape that off


will this always be the case?

I think at least for the time being we know there will be a prefix, but I don't know that it will always be explicitly the "ostree-unverified-registry" prefix.

I can just scrape the prefix of "ostree-unverified-registry" and move on, but then if the prefix changes, we're broken. This here, however, will break if the prefix is ever completely absent, so that's not 100% future-proof either.

We talked about this when we put this in the layering branch -- we were trying to avoid having to regex this, but I can?

So...well, yes this is a bit messy.

It actually does open up the question of how the user specifies security-related settings for this image. (Not just for the ostree-level signatures, but also e.g. TLS)

For the moment, I have to say that we hardcode injecting ostree-unverified-registry: as a prefix on the URL they provide.

We're planning to introduce a CRD, and that's probably the right place to have explicit settings for this.

That said, there is an argument that the interface to this containers-policy.json - and we shouldn't try to treat the OS image container specially at all.

But I dunno...it is different because we're going to boot it.

Anyways yeah my instinct right now is to inject ostree-unverified-registry: on the container image reference they provide.

Parsing the value by splitting on the first : is certainly something we can rely on right now.

But yes, this is all still technically experimental and change-able. But we wouldn't change it without getting the MCO to work with the new way first!

yuqi-zhang · 2022-07-28T20:37:41Z

pkg/daemon/update.go

 func (dn *CoreOSDaemon) applyOSChanges(mcDiff machineConfigDiff, oldConfig, newConfig *mcfgv1.MachineConfig) (retErr error) {
 // Extract image and add coreos-extensions repo if we have either OS update or package layering to perform

 if dn.nodeWriter != nil {
 dn.nodeWriter.Eventf(corev1.EventTypeNormal, "OSUpdateStarted", mcDiff.osChangesString())
 }

+ // TODO(jkyros): both paths (layering/non-layering) have different pre/update/post steps, but the events are the same. I hate to
+ // duplicate the eventing code, but I feel like I should split this function into like "applyLegacyOSChanges" and "applyLayeredOSChanges"


I think this makes sense

yuqi-zhang · 2022-07-28T20:38:13Z

pkg/daemon/update.go

+ // TODO(jkyros): both paths (layering/non-layering) have different pre/update/post steps, but the events are the same. I hate to
+ // duplicate the eventing code, but I feel like I should split this function into like "applyLegacyOSChanges" and "applyLayeredOSChanges"
+
+ client := NewNodeUpdaterClient()


Hmm, shouldn't there already be a client that's been initialized somewhere? Would we be able to use that?

Yes there is one in the daemon. using it now.

yuqi-zhang · 2022-07-28T20:40:33Z

pkg/daemon/update.go

- if osImageContentDir, err = ExtractOSImage(newConfig.Spec.OSImageURL); err != nil {
+
+ if isLayeredImage, err = client.IsBootableImage(newConfig.Spec.OSImageURL); err != nil {
+ // TODO(jkyros): Maybe this shouldn't be an error for now, so we don't impact any


In theory this should only return err if the image itself can't be pulled right? Which would be a failure anyways?

Probably should wrap the error with some context though

You're correct, I was just paranoid about adding additional error possibility for people who weren't using the feature. I added some context to the error.

vrutkovs · 2022-07-29T17:50:56Z

I was able to boot OKD with machine-os-content layered using this PR, excellent job!

yuqi-zhang · 2022-08-02T21:59:24Z

From https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_machine-config-operator/3272/pull-ci-openshift-machine-config-operator-master-e2e-gcp-op/1554512424382500864/artifacts/e2e-gcp-op/gather-extra/artifacts/pods/openshift-machine-config-operator_machine-config-daemon-gnpvb_machine-config-daemon.log the previous MCD appears to have not applied any kargs, which, hmm, should have been applied as far as the code is concerned? Doesn't look like the previous update failed either. Unfortunately no previous MCD logs

jkyros · 2022-08-02T22:09:50Z

I totally broke it because I nested my layering check too high in applyOSChanges() and it was skipping kargs when nothing else had changed. Sorry for the trouble, wasn't trying to waste your time 😄

yuqi-zhang · 2022-08-02T23:12:43Z

No waste 😄 just took a 5 minute look at the logs

jkyros · 2022-08-03T00:32:59Z

Looked like aws/terraform issues the first time, trying again.
/test e2e-agnostic-upgrade

jkyros · 2022-08-03T16:12:39Z

pkg/controller/common/helpers.go

+ if overriddenOSImageURL != "" && overriddenOSImageURL != osImageURL {
+
+ glog.Infof("OSImageURL has been overridden via machineconfig with %s (was: %s)", overriddenOSImageURL, osImageURL)
+ osImageURL = overriddenOSImageURL
+ }
+


This is probably where I'd safety check/feature gate it -- just prevent the merge to keep the old behavior.

If we aren't going to feature gate it, do we want a safety check in here that's:

check to make sure it's a "new format" image

only allow it to be overridden if we were able to check and it was a "new format" image?

So that no existing clusters would be impacted if they had, say, an old MachineConfig with an old image in it that previously would have been ignored?

As we discussed, this does introduce the additional behaviour of having the controller pull the image as well (or at least a remote inspect), which isn't necessarily a bad thing, since if this errors, the daemon would likely have errored anyways, so we are surfacing this earlier in the upgrade pipeline.

To summarize our options then are:

add a featuregate to enable osImageURL overrides in 4.12, and then remove this in future versions once this is more mature (does featuregates prevent upgrades at all?)

add an additional image check in the controller with additional error

no additional safety at all. the daemon will error if you have an override image that isn't in the correct format, which would only trip up users in one scenario (the user had a osImageurl override from a previous version incorrectly, but since we didn't error, didn't even notice)

I think... I would lean towards 2 personally, but don't have a strong opinion. Essentially 2 and 3 are the "same" in terms of what would fail, but 2 surfaces it earlier, and 1 would just be "make sure you absolutely know what you are doing" which I would also be ok with so long as we have a plan out of it

We talked on Monday and given:

the complexity of adding the safety

some of the container/registry library constraints (potentially having to parse/manage config.json entries ourselves in the controller, or writing secrets out to disk) and

the likelihood of someone actually having an old OSImageURL in their machineconfig

We opted not to put the safety in at this time, but we can later if we think we need to.

jkyros · 2022-08-04T15:55:07Z

This only works with public images right now because I didn't do anything with pull secrets, so registry.ci.openshift.org/rhcos-devel/rhel-coreos:latest can't be used directly.

Minimally, letting ostree have at the kubelet config.json makes it work:

mkdir /run/ostree
ln -s /var/lib/kubelet/config.json /run/ostree/auth.json

but there might be reasons we don't want to do that?

cgwalters · 2022-08-04T17:50:47Z

ln -s /var/lib/kubelet/config.json /run/ostree/auth.json

Yep, this is what we should do by default.

(But, I think it does probably long term make sense to expose a knob for this, it's one thing that would naturally be done via the "host as pod" model)

cgwalters

Thanks for doing this! Looking generally good to me

cgwalters · 2022-08-04T19:07:19Z

pkg/daemon/rpm-ostree.go

+ args := []string{"rebase", "--experimental", "ostree-unverified-registry:" + imgURL}
+
+ return runRpmOstree(args...)


Isn't this simpler as

Suggested change

args := []string{"rebase", "--experimental", "ostree-unverified-registry:" + imgURL}

return runRpmOstree(args...)

return runRpmOstree("rebase", "--experimental", "ostree-unverified-registry:" + imgURL)

yep, simplified. Thank you.

cgwalters · 2022-08-04T19:16:40Z

pkg/daemon/rpm-ostree.go

@@ -303,3 +348,23 @@ func runGetOut(command string, args ...string) ([]byte, error) {
 }
 return rawOut, nil
 }
+
+func (state *rpmOstreeState) getBootedDeployment() (*RpmOstreeDeployment, error) {


(Tangentially related to this I was kind of thinking about splitting out a github.com/coreos/rpmostree-client-go repository)

cgwalters · 2022-08-04T19:18:52Z

pkg/daemon/update.go

+ // Switch to real time kernel
+ if err := dn.switchKernel(oldConfig, newConfig); err != nil {
+ return err
+ }


This one can't work yet either...I'm thinking of creating rhel-coreos-8-rt as a separate image, but some prep work needs to happen for that.

So I think for now we should just error out if we detect kernelType != "default"

cgwalters · 2022-08-04T19:19:23Z

pkg/daemon/update.go

+ }
+
+ // TODO(jkyros): This is where we will handle Joseph's extensions container
+ glog.Infof("Can't apply extensions, not supported yet")


I'd say we should error out instead if any are set.

Previously our machine-os-content images were just "wrappers" around ostree commits. With our new "layering" work, we have a new image format that is actually a working OCI container, but this type of image needs to be applied differently by rpm-ostree. This extends the RpmOstreeClient to support new layered images by: - capturing more metadata from rpm-ostree status - adding an additional rebase function specifically for the layered case

We changed the interface for RpmOstreeClient, so we need to make sure the mocks are also up to date with the new signatures

sinnykumari · 2022-08-10T16:15:15Z

pkg/controller/common/helpers.go

+ }
+ // Make sure it's obvious in the logs that it was overridden
+ if overriddenOSImageURL != "" && overriddenOSImageURL != osImageURL {
+ glog.Infof("OSImageURL has been overridden via machineconfig with %s (was: %s)", overriddenOSImageURL, osImageURL)


maybe we should also send an event here

I moved the logging out a level so the name of the config was available to be logged, and also added an event.

pkg/daemon/update.go

sinnykumari

Few minor nits, otherwise this looks good to me. Nice work John.

Previously the MCO was arranged so that any user-supplied values for OSImageURL were ignored, and only the value from controllerconfig was used. This allows OSImageURL to be specified in a machineconfig, using the same precedence rules as everything else ( the last machineconfig with a populated OSImageURL "wins" the merge). If there is no user-specified OSImageURL (the field is blank in all machineconfigs) the MCO will fall back to using the controllerconfig OSImageURL like it did previousy. The purpose of this is for the user to be able to override the OS image using a custom/hotfix/layered image instead of being forced to use the one in the release payload.

Previously we were testing to make sure that OSImageURL overrides were prevented, but now we want to allow them again. This updates the test to make sure that OSImageURL gets merged properly as part of a machineconfig merge.

Previously there were a bunch of if conditions deciding what work needed to be done to the OS based on the mcDiff. We want to handle new format images, but adding an additional branch adds additional complexity to those ifs. This breaks the old and new paths out into separate functions for readability, ease of maintenance, and ease of retirement once we drop the old format image.

There is a place in the daemon where the image can get updated as part of the bootstrap flow. For completeness, we should make sure that it can use a "new format" image, if it were to ever get one. This makes the bootstrap flow check to see if the image it got is a new format image, so it can apply it properly.

For public images, everything works great, rpm-ostree can pull them with no problem, but when trying to use images that are in a private repository and require a pull secret, rpm-ostree needs to have that pull secret in order to use it. ostree by default looks in `/run/ostree/auth.json` for pull secrets, , and the pull secrets are already in `/var/lib/kubelet/config/json`, so this just links to them from `/run/ostree` so that ostree can access them.

jkyros · 2022-08-15T17:00:14Z

Fixed nits, added event per Sinny's request.

openshift-ci · 2022-08-15T19:44:54Z

@jkyros: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

cgwalters · 2022-08-15T19:59:58Z

/lgtm

openshift-ci · 2022-08-15T20:00:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jkyros, sinnykumari

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cgwalters,jkyros,sinnykumari]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from cgwalters and yuqi-zhang July 28, 2022 17:01

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2022

yuqi-zhang reviewed Jul 28, 2022

View reviewed changes

jkyros force-pushed the new-image-format-minimal branch from 1f88386 to c404bcd Compare August 2, 2022 16:59

jkyros force-pushed the new-image-format-minimal branch from c404bcd to 6a37ec0 Compare August 2, 2022 22:05

jkyros changed the title ~~[ WIP ] Allow overriding OSImageURL with a layered image~~ Allow overriding OSImageURL with a layered image Aug 3, 2022

jkyros commented Aug 3, 2022

View reviewed changes

cgwalters reviewed Aug 4, 2022

View reviewed changes

jkyros added 2 commits August 9, 2022 19:10

Update rpm-ostree tests with new signatures

a8d445c

We changed the interface for RpmOstreeClient, so we need to make sure the mocks are also up to date with the new signatures

jkyros force-pushed the new-image-format-minimal branch 4 times, most recently from cd1b9d3 to 3bfc9b9 Compare August 10, 2022 08:24

sinnykumari reviewed Aug 10, 2022

View reviewed changes

sinnykumari reviewed Aug 11, 2022

View reviewed changes

pkg/daemon/update.go Outdated Show resolved Hide resolved

sinnykumari reviewed Aug 11, 2022

View reviewed changes

pkg/daemon/update.go Outdated Show resolved Hide resolved

sinnykumari approved these changes Aug 11, 2022

View reviewed changes

jkyros added 4 commits August 15, 2022 11:56

Adjust unit tests to allow for OSImageURL override

55e65fa

Previously we were testing to make sure that OSImageURL overrides were prevented, but now we want to allow them again. This updates the test to make sure that OSImageURL gets merged properly as part of a machineconfig merge.

jkyros force-pushed the new-image-format-minimal branch from 3bfc9b9 to 6796633 Compare August 15, 2022 16:59

yuqi-zhang mentioned this pull request Aug 15, 2022

[WIP] Teach MCO to use the new-format image by default #3258

Closed

cgwalters requested a review from yuqi-zhang August 15, 2022 19:59

openshift-ci bot assigned cgwalters Aug 15, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2022

openshift-merge-robot merged commit 0d563c3 into openshift:master Aug 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow overriding OSImageURL with a layered image #3272

Allow overriding OSImageURL with a layered image #3272

jkyros commented Jul 28, 2022

yuqi-zhang left a comment

yuqi-zhang Jul 28, 2022

jkyros Aug 2, 2022

yuqi-zhang Jul 28, 2022

jkyros Aug 2, 2022

cgwalters Aug 2, 2022

yuqi-zhang Jul 28, 2022

yuqi-zhang Jul 28, 2022

jkyros Aug 2, 2022

yuqi-zhang Jul 28, 2022

jkyros Aug 2, 2022

vrutkovs commented Jul 29, 2022

yuqi-zhang commented Aug 2, 2022

jkyros commented Aug 2, 2022

yuqi-zhang commented Aug 2, 2022

jkyros commented Aug 3, 2022

jkyros Aug 3, 2022

yuqi-zhang Aug 3, 2022

jkyros Aug 10, 2022

jkyros commented Aug 4, 2022

cgwalters commented Aug 4, 2022

cgwalters left a comment

cgwalters Aug 4, 2022

jkyros Aug 10, 2022

cgwalters Aug 4, 2022

cgwalters Aug 4, 2022

cgwalters Aug 4, 2022

sinnykumari Aug 10, 2022 •

edited

Loading

jkyros Aug 15, 2022

sinnykumari left a comment

jkyros commented Aug 15, 2022

openshift-ci bot commented Aug 15, 2022

cgwalters commented Aug 15, 2022

openshift-ci bot commented Aug 15, 2022

		args := []string{"rebase", "--experimental", "ostree-unverified-registry:" + imgURL}

		return runRpmOstree(args...)

Allow overriding OSImageURL with a layered image #3272

Allow overriding OSImageURL with a layered image #3272

Conversation

jkyros commented Jul 28, 2022

yuqi-zhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrutkovs commented Jul 29, 2022

yuqi-zhang commented Aug 2, 2022

jkyros commented Aug 2, 2022

yuqi-zhang commented Aug 2, 2022

jkyros commented Aug 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkyros commented Aug 4, 2022

cgwalters commented Aug 4, 2022

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinnykumari Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinnykumari left a comment

Choose a reason for hiding this comment

jkyros commented Aug 15, 2022

openshift-ci bot commented Aug 15, 2022

cgwalters commented Aug 15, 2022

openshift-ci bot commented Aug 15, 2022

sinnykumari Aug 10, 2022 •

edited

Loading