fix searching on non-searchable image registries #1821

tomasmatus · 2024-08-14T12:28:12Z

Add support to pull images from registries which do not support search API.

src/ImageRunModal.jsx

garrett · 2024-08-14T13:33:04Z

I already had this container on my system:

I tried a few example ones, but perhaps they're not actually there...

Then I tried a container that GitHub talked about called "Super Linter", which has this as a container URI:
ghcr.io/github/super-linter@sha256:b723c817982b85f6f98c9c24a653f9f760bf5941577f5ea9532d31bd89aa8823
...as seen @ https://github.com/orgs/github/packages/container/super-linter/207523715

If you add the URI without the @..., it seems like it'd work:

But you have to specifically click on the item in the dropdown. (Is that intended?) If you change the focus, the entry goes blank. Also, if you try to edit the text (like if you have a typo, like copying the URI incorrectly), the entry also goes blank.

However, it does seem to work if you click on the entry, so that's great!

I see a related discussion at https://github.com/orgs/community/discussions/26279 which uses tags/list but it seems things need a token, so I guess that won't work for us.

martinpitt

Cheers! Please update the commit message to explain the change more verbosely (use case, reference to https://issues.redhat.com/browse/COCKPIT-1148 and the correspondig upstream and RHEL issues).

src/ImageRunModal.jsx

test/check-application

tomasmatus · 2024-08-20T12:55:49Z

fedora 41 fails because of this:


[root@fedora-41-127-0-0-2-2201 ~]# podman manifest inspect localhost:80/my-busybox
Error: parsing manifest blob "{\"schemaVersion\":2,\"mediaType\":\"application/vnd.oci.image.manifest.v1+json\",\"config\":{\"mediaType\":\"application/vnd.oci.image.config.v1+json\",\"digest\":\"sha256:02154912ea3e0a26dd3d9878cc35592995a559c1894ed9f1f3ad2e67c601e8a3\",\"size\":1051},\"layers\":[{\"mediaType\":\"application/vnd.oci.image.layer.v1.tar+zstd\",\"digest\":\"sha256:979ef0eafe970832b8df3c3233c50a1ceb863501afd0fdd0819f4e74459a269b\",\"size\":783654,\"annotations\":{\"io.github.containers.zstd-chunked.manifest-checksum\":\"sha256:1818e53836a49be9f33ab634b8cecc242402366faff167cc29c346acba6c786c\",\"io.github.containers.zstd-chunked.manifest-position\":\"769759:3878:48899:1\",\"io.github.containers.zstd-chunked.tarsplit-position\":\"773645:9937:339414\"}},{\"mediaType\":\"application/vnd.oci.image.layer.v1.tar+zstd\",\"digest\":\"sha256:5126ce266a4b0023a57d535a42dfdedee252be565ab10967140a8cf5c2babfc6\",\"size\":973884,\"annotations\":{\"io.github.containers.zstd-chunked.manifest-checksum\":\"sha256:9a91d82526737b46f2f6e5d9478f597fe989943445cda912aa8af5e28b7351c9\",\"io.github.containers.zstd-chunked.manifest-position\":\"859954:57329:277072:1\",\"io.github.containers.zstd-chunked.tarsplit-position\":\"917291:56521:1414602\"}}]}" as a "application/vnd.oci.image.manifest.v1+json": Treating single images as manifest lists is not implemented

I checked f40 and it gives a warning rather than "not implemented" error. Both are on version 5.2.0 idk what this means/how to fix for now, will look into it

martinpitt

Thanks for the fixes! Next round.

src/ImageRunModal.jsx

test/check-application

garrett · 2024-09-03T08:17:57Z

It's getting there!

I found two interaction problems so far:

It doesn't strip spaces at the end (or start), so if I copy a URI from somewhere and it has a space around it, it fails.
Can't use the keyboard: After pasting the URI, hitting enter fails; it empties the line. I would expect it to accept the URL, so I could continue configuring it.

I was testing with ghcr.io/cockpit-project/tasks-tmp:10449236342.1-arm64 (which had a space when I copied it), which I found at https://github.com/cockpit-project/cockpituous/pkgs/container/tasks-tmp (as it's on ghcr and isn't our standard tasks container, which I'm using as my dev environment in toolbox already).

garrett

(fix the issues with the spaces and hitting enter, as pointed out above; thanks!)

src/ImageRunModal.jsx

martinpitt · 2024-09-03T08:53:21Z

src/ImageRunModal.jsx

+                searches.push(...utils.fallbackRegistries.map(registry =>
+                    this.activeConnection.call({
+                        method: "GET",
+                        path: client.VERSION + "libpod/images/search",
+                        body: "",
+                        params: {
+                            term: registry + "/" + value


This is a significant piece of code, please add a test for fallback registries.

martinpitt · 2024-09-03T08:54:52Z

test/check-application

+        # Searching for container by prefix fails
+        b.set_input_text("#create-image-image-select-typeahead", "localhost:80/my-busy")
+        b.wait_text("button.pf-v5-c-select__menu-item.pf-m-disabled", "No images found")
+        b.wait_in_text(".pf-v5-c-alert.pf-m-danger", "couldn't search registry")


This shouldn't be "danger", but "info" or "default". (It needs to be changed in the code, but it's visible here)

src/ImageRunModal.jsx

martinpitt · 2024-09-03T08:57:28Z

test/check-application

+        b.wait_text("button.pf-v5-c-select__menu-item.pf-m-disabled", "No images found")
+        b.wait_in_text(".pf-v5-c-alert.pf-m-danger", "couldn't search registry")
+
+        # Searching for full url with tag finds the image


s/url with //

src/ImageRunModal.jsx

martinpitt · 2024-09-03T09:03:10Z

@tomasmatus thanks! I cleaned up outdated review threads and comments to make the remaining stuff easier to see. There's a few small things to fix; if you really don't want to rewrite/simplify the search result loop (which is so hard to read and so easy to get wrong!), perhaps @jelly can help with that, or also me when I'm back from PTO. I also wouldn't veto landing it as-is and cleaning up later, if @jelly is ok with it. But in three weeks nobody of us will be able to follow that logic..

tomasmatus · 2024-09-03T09:42:23Z

I also wouldn't veto landing it as-is and cleaning up later.

yes, I agree with this, let's not

fixes: cockpit-project#1220 Add support to pull images from registries which do not support search API.

Utilizes `podman manifest inspect` in the background and when manifest is found for the user input image url it is possible to select this image from the search dropdown. Normal `podman search` queries are disabled when image tag is specified as the search API doesn't seem to support searching for specific tags.

tomasmatus · 2024-09-25T14:43:43Z

@martinpitt if you'd like to have a look it is this Select that is misbehaving.

What happens is when you type something in and press enter the input bar is cleared rather than preserve the value. It is also cleared once the input loses focus. This shouldn't be happening as the isInputValuePersisted prop is used. I haven't figured out if this is a bug in the now deprecated PF4 Select or if it is somehow cleared in our code.

martinpitt · 2024-09-26T05:32:25Z

I can reproduce the issue on main. AFAICS the only sensible thing is to ignore the Enter key completely then -- if you type a random string into the image bar, it cannot be "accepted" as a valid value (which is usually the meaning of "Enter"). You must actually select a valid image for the dialog to make sense. Clearing an invalid value on focus change also makes sense IMHO.

However, I do see how this would collide with the workflow of this PR -- it'd still force you to actually click on the search result instead of just copy&pasting an image name and doing nothing else. Did I get this right?

The demo on the PF docs (v4, as that's the deprecated one) behaves in the above way: "Enter" is a no-op, and changing focus clears the input value. This doesn't do any special keyboard input tricks.

My hunch is that it's the <FormGroup> or its <Popover> which react to Enter, as it focuses the (?) and opens the popup help. The same happens on e.g. the "Memory limit" checkbox -- pressing Enter there also opens the "Owner" popup help.

Pressing "Enter" in the dialog on any active form element (such as the search input or the checkboxes) previously bubbled up to the Form, which then activated the first popover help. This is unexpected and very irritating especially when using the search input. Just ignore the Enter key to fix that, similar to what the image search modal already does. See cockpit-project#1821 (comment)

martinpitt · 2024-09-26T08:19:21Z

@tomasmatus Fixed in #1861 . This is independent from this PR.

cockpituous · 2024-09-26T08:48:51Z

src/ImageRunModal.jsx

+        const changeDialogError = (reason) => {
+            dialogError = _("Failed to search for new images");
+            // TODO: add registry context, podman does not include it in the reply.
+            dialogErrorDetail = reason ? cockpit.format(_("Failed to search for images: $0"), reason.message) : _("Failed to search for images.");


This added line is not executed by any test.

cockpituous · 2024-09-26T08:48:51Z

src/ImageRunModal.jsx

+        if (!RE_CONTAINER_TAG.test(value)) {
+            // If there are registries configured search in them, or if a user searches for `docker.io/cockpit` let
+            // podman search in the user specified registry.
+            if (Object.keys(this.props.podmanInfo.registries).length !== 0 || value.includes('/')) {


This added line is not executed by any test.

cockpituous · 2024-09-26T08:48:51Z

src/ImageRunModal.jsx

+                searches.push(...utils.fallbackRegistries.map(registry =>
+                    this.activeConnection.call({
+                        method: "GET",
+                        path: client.VERSION + "libpod/images/search",
+                        body: "",
+                        params: {
+                            term: registry + "/" + value


These 7 added lines are not executed by any test.

cockpituous · 2024-09-26T08:48:51Z

src/ImageRunModal.jsx

-                                dialogError = _("Failed to search for new images");
-                                // TODO: add registry context, podman does not include it in the reply.
-                                dialogErrorDetail = result.reason ? cockpit.format(_("Failed to search for images: $0"), result.reason.message) : _("Failed to search for images.");
+                            } else if (!manifestResult && !imageExistsLocally(value, this.props.localImages)) {


This added line is not executed by any test.

Pressing "Enter" in the dialog on any active form element (such as the search input or the checkboxes) previously bubbled up to the Form, which then activated the first popover help. This is unexpected and very irritating especially when using the search input. Just ignore the Enter key to fix that, similar to what the image search modal already does. See cockpit-project#1821 (comment)

martinpitt

Cheers @tomasmatus ! The previous review still has three current threads, and I have some more questions. This is coming together!

martinpitt · 2024-09-26T11:30:52Z

src/ImageRunModal.jsx

+
+        const changeDialogError = (reason) => {
+            dialogError = _("Failed to search for new images");
+            // TODO: add registry context, podman does not include it in the reply.


Follow-up, or is that easy to do and you just forgot about it?

martinpitt · 2024-09-26T11:34:59Z

src/ImageRunModal.jsx

        }

        Promise.allSettled(searches)
                .then(reply => {
                    if (reply && this._isMounted) {
                        let imageResults = [];
-                        let dialogError = "";
-                        let dialogErrorDetail = "";
+                        const manifestResult = handleManifestsQuery(reply[0]);


So if this fails, but the other search replies succeed, this quickly sets an error in the dialog, but the code below will then reset/update the error, right? I.e. that may be intended (even though it's a bit ugly). The error handling strategy should at least be commented here, or better made explicit.

martinpitt · 2024-09-26T11:36:55Z

src/ImageRunModal.jsx

-                                // TODO: add registry context, podman does not include it in the reply.
-                                dialogErrorDetail = result.reason ? cockpit.format(_("Failed to search for images: $0"), result.reason.message) : _("Failed to search for images.");
+                            } else if (!manifestResult && !imageExistsLocally(value, this.props.localImages)) {
+                                changeDialogError(result.reason);


So this overwrites the error set by handleManifestQuery(), which seems fine -- but it also overwrites earlier error from reply[i-1]. It probably doesn't matter that much if we show the error from the first or last registry, it just feels a bit random.

hit the wrong button, sorry

Pressing "Enter" in the dialog on any active form element (such as the search input or the checkboxes) previously bubbled up to the Form, which then activated the first popover help. This is unexpected and very irritating especially when using the search input. Just ignore the Enter key to fix that, similar to what the image search modal already does. See cockpit-project#1821 (comment)

Pressing "Enter" in the dialog on any active form element (such as the search input or the checkboxes) previously bubbled up to the Form, which then activated the first popover help. This is unexpected and very irritating especially when using the search input. Just ignore the Enter key to fix that, similar to what the image search modal already does. See #1821 (comment)

tomasmatus commented Aug 14, 2024

View reviewed changes

src/ImageRunModal.jsx Outdated Show resolved Hide resolved

tomasmatus force-pushed the non-searchable-repos branch from 9934989 to c0b19df Compare August 14, 2024 14:14

tomasmatus requested a review from martinpitt August 19, 2024 08:16

tomasmatus force-pushed the non-searchable-repos branch from c0b19df to bfa093e Compare August 19, 2024 08:29

martinpitt requested changes Aug 19, 2024

View reviewed changes

tomasmatus force-pushed the non-searchable-repos branch 2 times, most recently from f916975 to 5a5fe4a Compare August 20, 2024 11:37

tomasmatus force-pushed the non-searchable-repos branch from 5a5fe4a to 24d666e Compare August 21, 2024 08:01

martinpitt reviewed Aug 21, 2024

View reviewed changes

src/ImageRunModal.jsx Outdated Show resolved Hide resolved

src/ImageRunModal.jsx Outdated Show resolved Hide resolved

src/ImageRunModal.jsx Outdated Show resolved Hide resolved

martinpitt reviewed Aug 29, 2024

View reviewed changes

src/ImageRunModal.jsx Outdated Show resolved Hide resolved

src/ImageRunModal.jsx Outdated Show resolved Hide resolved

martinpitt reviewed Aug 29, 2024

View reviewed changes

src/ImageRunModal.jsx Show resolved Hide resolved

src/ImageRunModal.jsx Outdated Show resolved Hide resolved

src/ImageRunModal.jsx Outdated Show resolved Hide resolved

test/check-application Outdated Show resolved Hide resolved

test/check-application Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

tomasmatus requested a review from martinpitt September 2, 2024 11:28

garrett requested changes Sep 3, 2024

View reviewed changes

martinpitt reviewed Sep 3, 2024

View reviewed changes

tomasmatus added 9 commits September 23, 2024 09:41

fix searching on non-searchable image registries

e28aad8

fixes: cockpit-project#1220 Add support to pull images from registries which do not support search API.

ghcr: drop unnecessary cleanup

82dbe86

nosearch: fixes after review

b27689d

nosearch: use manifest instead of tagging

f704d54

nosearch: always query manifest

bbf9e9b

nosearch: handle manifest result separately, search tag test

4d9263e

fmf: add nginx dependency

29329bc

nosearch: trim whitespaces around image name

766ef1b

handle manifest query separately

839df1d

tomasmatus force-pushed the non-searchable-repos branch from a1ceaef to 839df1d Compare September 24, 2024 11:05

martinpitt mentioned this pull request Sep 26, 2024

Fix irritating Enter key behaviour in container creation dialog #1861

Merged

cockpituous reviewed Sep 26, 2024

View reviewed changes

martinpitt previously approved these changes Sep 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix searching on non-searchable image registries #1821

fix searching on non-searchable image registries #1821

tomasmatus commented Aug 14, 2024 •

edited

Loading

garrett commented Aug 14, 2024

martinpitt left a comment

tomasmatus commented Aug 20, 2024

martinpitt left a comment

This comment was marked as resolved.

garrett commented Sep 3, 2024

garrett left a comment

martinpitt Sep 3, 2024

martinpitt Sep 3, 2024

martinpitt Sep 3, 2024

martinpitt commented Sep 3, 2024

tomasmatus commented Sep 3, 2024

tomasmatus commented Sep 25, 2024

martinpitt commented Sep 26, 2024 •

edited

Loading

martinpitt commented Sep 26, 2024

cockpituous Sep 26, 2024

cockpituous Sep 26, 2024

cockpituous Sep 26, 2024

cockpituous Sep 26, 2024

martinpitt left a comment

martinpitt Sep 26, 2024

martinpitt Sep 26, 2024

martinpitt Sep 26, 2024

fix searching on non-searchable image registries #1821

Are you sure you want to change the base?

fix searching on non-searchable image registries #1821

Conversation

tomasmatus commented Aug 14, 2024 • edited Loading

garrett commented Aug 14, 2024

martinpitt left a comment

Choose a reason for hiding this comment

tomasmatus commented Aug 20, 2024

martinpitt left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

garrett commented Sep 3, 2024

garrett 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

martinpitt commented Sep 3, 2024

tomasmatus commented Sep 3, 2024

tomasmatus commented Sep 25, 2024

martinpitt commented Sep 26, 2024 • edited Loading

martinpitt commented Sep 26, 2024

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

martinpitt 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

tomasmatus commented Aug 14, 2024 •

edited

Loading

martinpitt commented Sep 26, 2024 •

edited

Loading