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

daemon/transaction-types: Support custom origins for digested pullspecs #5120

Merged
merged 6 commits into from
Oct 16, 2024

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Oct 4, 2024

Having a digested pullspec is the container flow equivalent of an
ostree checksum origin. So having a custom origin in that case is useful
there for the same reasons (i.e. it means that something else is likely
managing the host and we want to reflect that info in the origin).

We're thinking of using this in FCOS when we move from OSTree to OCI
for updates.

@jlebon
Copy link
Member Author

jlebon commented Oct 4, 2024

This is what it looks like:

root@cosa-devsh:~# rpm-ostree rebase --custom-origin-description "This is my custom description" --custom-origin-url "quay.io/fedora/fedora-coreos:stable" ostree-unverified-registry:quay.io/fedora/fedora-coreos@sha256:2a3de749f850460372072da32be9c7238f4fb955c574d90f8eb9c79fa1a601e5
...
root@cosa-devsh:~# rpm-ostree status
State: idle
Deployments:
  quay.io/fedora/fedora-coreos:stable
             CustomOrigin: This is my custom description
                  Version: 40.20240906.3.0 (2024-09-23T17:17:26Z)
                     Diff: 71 downgraded

● 47a7471ffcbdccb7ab0fcd743ea79833b6f1ff8b3ace90ac2eb079b0571bae03
                  Version: v2024.8-77-gf4fa489112 (2024-10-04T21:18:37Z)

@jlebon
Copy link
Member Author

jlebon commented Oct 4, 2024

Ahhh, I wasn't finding a good place to add an integration test for this, but I see container-rebase-upgrade now which looks like a good spot.

We're thinking of using this in FCOS when we move from OSTree to OCI for updates.

The alternative of course is to support something like rpm-ostree rebase ostree-unverified-registry:quay.io/fedora/fedora-coreos:stable sha256:..., which is analogous to the OSTree version: rpm-ostree rebase $branch $revision, but doesn't map perfectly. E.g. in the OSTree case we actually check that the revision is on that branch, whereas that's meaningless in the container case so it's a bit awkward. But anyway... all that will go away eventually once we move to bootc.

Copy link
Member

@travier travier left a comment

Choose a reason for hiding this comment

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

Weak LGTM as I've not tested this and I'm not familiar with the code. One suggestion.

rust/src/core.rs Outdated
Comment on lines 123 to 125
&& refspec.len() > (PREFIX_LEN + DIGEST_LEN)
&& refspec[(refspec.len() - DIGEST_LEN - PREFIX_LEN)..].starts_with(PREFIX)
&& ostree::validate_checksum_string(&refspec[(refspec.len() - DIGEST_LEN)..]).is_ok()
Copy link
Member

Choose a reason for hiding this comment

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

That feels like a bit manual string manipulations.

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.

Not opposed, but

(i.e. it means that something else is likely
managing the host and we want to reflect that info in the origin).

That goal seems to heavily overlap with the "driver" API to me...it's not so much that the origin is custom as it is that something else is managing.

A big difference versus the original thing we did here for the MCO/OCP case is that the admin couldn't upgrade via rpm-ostree "natively".

I guess another way to say this is: Is this purely cosmetic, and if so could we improve those cosmetics in another way?

rust/src/core.rs Outdated Show resolved Hide resolved
src/daemon/rpmostreed-transaction-types.cxx Show resolved Hide resolved
Add a new `is_container_image_digest_reference()` function which checks
if the container image pullspec is using a digest.

Prep for next patch.
Almost no functional change (we do now error out if no description is
given even if we're not going to use those fields, but that seems fine).

Prep for next patch.
Having a digested pullspec is the container flow equivalent of an
ostree checksum origin. So having a custom origin in that case is useful
there for the same reasons (i.e. it means that something else is likely
managing the host and we want to reflect that info in the origin).

We're thinking of using this in FCOS when we move from OSTree to OCI
for updates.
The rebase code already filters out the custom origin bits in cases
where it doesn't make sense, so there's no need to additionally filter
down here.

This makes it work now in both the OSTree checksum case and the
container digested pullspec case.
Now that we support this, let's make sure we render it as well.
Verify that one can use a custom origin with a digest pullspec. While
we're here, use testing-devel instead of stable since it's going to be
much closer to what we're running on.
@jlebon jlebon force-pushed the pr/custom-origin-containers branch from d03ce28 to a65b3af Compare October 8, 2024 12:10
@jlebon
Copy link
Member Author

jlebon commented Oct 8, 2024

(i.e. it means that something else is likely
managing the host and we want to reflect that info in the origin).

That goal seems to heavily overlap with the "driver" API to me...it's not so much that the origin is custom as it is that something else is managing.

Right yeah, this is a good distinction. I think there's a strong argument to be made that the "origin" in the FCOS case is indeed custom; the source of truth for what to update to is not an OSTree branch or container tag, but a graph. I used quay.io/fedora/fedora-coreos:stable as the custom origin URL in the example above but I think it'd be more appropriate to have e.g. the image ref for the OCI artifact containing the graph instead.

But yeah, this is definitely a big change in how things are wired today. Users currently can do rpm-ostree upgrade --bypass-driver and we'd consciously be breaking that. It's not clear yet if that's the way we want to go. The other alternative is #5120 (comment) which is closer to what we currently do in the OSTree model. But again, maybe this is a good opportunity to refine our intent better.

Anyway, since this is already written up and seems generally useful, I tweaked it to address the comment! (Relatedly, I feel like we could open up custom origins to all methods? When using an agent, how rpm-ostree fetches the content seems separate from whether we want rpm-ostree to directly update from that same source.)

@jlebon jlebon merged commit 974d2af into coreos:main Oct 16, 2024
17 checks passed
@jlebon jlebon deleted the pr/custom-origin-containers branch October 16, 2024 17:40
@jmarrero jmarrero mentioned this pull request Nov 15, 2024
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.

3 participants