fix(images): Accept http(s)+oci image references - #437
Conversation
There was a problem hiding this comment.
Pull request overview
Adds parsing and round-tripping support for http+oci and https+oci instance image references.
Changes:
- Introduces scheme-aware image parsing and wire serialization.
- Updates instance creation/editing and image listing behavior.
- Adds unit, integration, and help-output coverage.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/types/image.go |
Preserves URI-based image references. |
internal/types/image_test.go |
Tests image reference behavior. |
internal/images/images.go |
Adds scheme-aware parsing. |
internal/images/images_test.go |
Tests supported formats and errors. |
internal/cmd/instances.go |
Sends URI references to the API. |
internal/cmd/images.go |
Loads URI-backed platform images. |
cmd/unikraft/testdata/TestHelp/instances |
Updates generated help expectations. |
cmd/unikraft/integration/instance_http_oci_test.go |
Adds end-to-end coverage. |
An http(s)+oci image is fetched by the server, so the CLI only has to carry the URI through: parse it, keep it verbatim on ImageRef, and send it back as-is on create and edit. An unrecognised scheme now reports itself instead of falling through to domain splitting, which surfaced as 'invalid reference format'. GitHub-Fixes: TOOL-1093 Signed-off-by: Cezar Craciunoiu <cezar@unikraft.io>
7fa36ff to
cdca2ea
Compare
| t.Run("live", func(t *testing.T) { | ||
| image := os.Getenv("UKC_TEST_HTTP_OCI_URI") | ||
| if image == "" { | ||
| t.Skip("set UKC_TEST_HTTP_OCI_URI to an http+oci:// image reachable from the metro") | ||
| } |
There was a problem hiding this comment.
We should use the approach as in https://github.com/unikraft-cloud/integration-tests/pull/109.
Where we host the built image itself inside a VM, and then the agent pulls it from that place.
Then we don't need to be dependent on setting this here.
| if i := strings.LastIndex(s, "/@"); i >= 0 { | ||
| s = s[:i] | ||
| } else { | ||
| s, _, _ = strings.Cut(s, "@") | ||
| } |
There was a problem hiding this comment.
Hmm. Despite having written the original version here, this does not spark joy.
We shouldn't be doing string manipulation, instead, we should be actually manipulating the Reference.
| // WireURL returns the identifier to send to the platform API: the verbatim URI | ||
| // for schemes other than oci, and the canonical OCI reference otherwise. | ||
| func (ir ImageRef[T]) WireURL() string { | ||
| if ir.URI != "" { | ||
| return ir.URI | ||
| } | ||
| var zero T | ||
| if ir.Reference == zero { | ||
| return "" | ||
| } | ||
| return ir.Reference.String() | ||
| } |
| // URI is the verbatim identifier when Scheme is not oci, so that such | ||
| // identifiers round-trip back to the platform unchanged. It is empty for | ||
| // plain OCI references, which are sent as a bare reference. | ||
| URI string |
There was a problem hiding this comment.
Hm, struggling to articulate what's wrong with this. The first thing is that this type and parsing probably be in image-spec/reference. So that it's centralized, instead of needing it custom in the agent as well eventually (so we can split out https://github.com/unikraft-cloud/agent/blob/7301eb51d13162c1b05ff6a57853f4b4c1c22da2/internal/oci/uri.go#L70-L86).
We should also really be attempting to parse the magic tarball reference into constituent parts here, not just treating it as opaque.
I'm also divided on which of these approaches we might want to take:
-
Try and make an
httpRefimplementation. Then it'll work withreference.WithDigest,reference.Domainand the whole host of things. It would behave entirely as a normal reference, except thatStringwould return the scheme prefix as well.Some helpers just wouldn't really work with this, especially the way this all is manipulated.
-
(more likely) Make a single new
Referencestruct. Then it's very type-safe, very demure. Then we define a bunch of helpers on in, so that you can easily get theDigest, or remove theDigestfor the short display above. Something in the style of https://github.com/dagger/dagger/blob/8c6516e3257650b055360a7a19672a674fa34714/internal/buildkit/util/gitutil/git_url.go#L33-L56 (with similar helpers likefromURL/fromSCPStyleURLinstead beingfromReferenceandfromHTTPReference).Then across x and the CLI, we'd just use this new
Referencestruct, and avoid usingreference.Namedand similar everywhere.
I am really struggling to think of the best approach, but importantly:
- As much of the handling code should be shared, callers ideally shouldn't need to distinguish between whether it's an HTTP reference or a real OCI reference (although they should be able to somehow).
- All the parsing logic for references should live in
xso we can share it in theagentat some point.
There was a problem hiding this comment.
馃憤 on the last two bullets
馃憤 on using demure
Yup, of the two the second bullets seems more plausible
|
All comments legitimate, will address |
This PR adds better parsing for the oci references to be able to accept http+oci references in the image field.
The parsing is a bit bigger as it now parses two different formats in one (oci and http).
Also documents being able to use these types of images in the
--imageflag doc.Depends on: unikraft-cloud/x#368
GitHub-Fixes: TOOL-1093