Image inspect rework #3017

apostasie · 2024-05-17T00:16:30Z

Apologies for the big commit here, but I do not believe we can fix image inspect incrementally, as some of the core assumptions are wrong (specifically in the way we walk images, in the returned data types, etc).

Furthermore, there is more work needed here.
Other command which are doing images lookup (rmi) are also faulty.
Fixing these as well would turn this PR into a large monster, and I would rather do that separately as a follow-up once this one is reviewed and accepted.

So, this PR fixes the following:

image inspect (Current implementation of images inspect is faulty (eg: RepoTags) #3004 among other things):
- allow querying all sort of references:
  - repo short name
  - repo full name
  - repo with tag and/or digest
  - digest, with or without sha prefix
  - short ids
- properly populate RepoTags and RepoDigests
- always return a JSON array, instead of a stream, for example when multiple references are provided (fixes docker / nerdctl inconsistent return value from image inspect --format json #2988)
dockercompat:
- implement Parent (if it has been set on the image - note: I am not sure nerdctl does add the label when committing, but this is a different problem)
- implement Variant
- add GraphDriverData struct for future implementation
- updated Image struct to the latest version at https://github.com/moby/moby/blob/v26.1.2/api/types/types.go#L34-L140
- implement (deprecated) VirtualSize for compatibility

Obviously, the reworked image inspect should be wired into inspect at a later point, and the overall revised lookup mechanics should be generalized and used by other methods (like rmi) - again, as a follow-up.
Overall the current ImageWalker should either be removed or replaced with the updated workflow here.

Please note that our implementation also diverges from Docker, for good reasons:
For example, when retrieving an image using a digest and a tagged name, we do verify the tag name against the result, unlike Moby which only verifies the image name (because of their use of ParseDockerRef, which should be banned from existence IMHO…).
Because of that, Moby will allow inspecting images using random bogus tag names as long as the digest matches and return a different image than the requested one altogether.
This has had far reaching consequences: specifically, mind-bending bugs for build caching invalidation.
I do believe Moby must fix it, but it’s not my problem.

What is left overall:

nerdctl images displays the wrong image id #3011 is a problem - we need to look into it and this might have consequences for the lookup implementation here - specifically wrt ContentStore
generalizing the lookup method so that other commands can leverage it (will fix nerdctl rmi can be broken by using tag names with repo short sha #3016 for example)
rewrite overall inspect to fix nerdctl inspect does not allow inspecting networks, volumes #3005, nerdctl inspect should not return a stream of json array, but a single array #3006

Also, this PR adds a bunch of integration tests on the above.

PTAL

cc @AkihiroSuda

pkg/referenceutil/referenceutil.go

apostasie · 2024-05-17T00:26:49Z

pkg/inspecttypes/dockercompat/dockercompat.go

-	i := &Image{}
-
-	imgoci := n.ImageConfig
+func ImageFromNative(nativeImage *native.Image) (*Image, error) {


Light cleanup:

improve variable name readability

introduce Variant, Parent and VirtualSize

populate RepoDigest

apostasie · 2024-05-17T00:27:06Z

pkg/inspecttypes/dockercompat/dockercompat.go

+	// Deprecated: TODO: ContainerConfig *container.Config
+}
+
+// From: https://github.com/moby/moby/blob/v26.1.2/api/types/graph_driver_data.go


Provisional for future implementation

apostasie · 2024-05-17T00:29:02Z

pkg/inspecttypes/dockercompat/dockercompat.go

@@ -47,28 +47,39 @@ import (
 	"github.com/tidwall/gjson"
 )

-// Image mimics a `docker image inspect` object.
-// From https://github.com/moby/moby/blob/v20.10.1/api/types/types.go#L340-L374
+// From https://github.com/moby/moby/blob/v26.1.2/api/types/types.go#L34-L140
 type Image struct {


Updated with latest definition and reordered a bit for readability.

moved down deprecated fields

added VirtualSize, Parent, Variant

apostasie · 2024-05-17T00:29:18Z

pkg/inspecttypes/dockercompat/dockercompat.go

@@ -37,7 +37,7 @@ import (

 	"github.com/containerd/containerd"
 	"github.com/containerd/containerd/runtime/restart"
-	gocni "github.com/containerd/go-cni"


Unnecessary.

apostasie · 2024-05-17T00:29:54Z

README.md

@@ -268,6 +268,10 @@ docker build -t test-integration --target test-integration .
 docker run -t --rm --privileged test-integration
 ```

+To run a single integration test (in this case, `image_inspect_test`):


Just a note for newcomers.

apostasie · 2024-05-17T00:30:21Z

cmd/nerdctl/image_inspect_test.go

 	// test RawFormat support
 	base.Cmd("image", "inspect", testutil.CommonImage, "--format", "{{.Id}}").AssertOK()

 	// test typedFormat support
 	base.Cmd("image", "inspect", testutil.CommonImage, "--format", "{{.ID}}").AssertOK()
 }
+
+func inspectImageHelper(base *testutil.Base, identifier ...string) []dockercompat.Image {


Cannot use the one in testutil, as it assumes single entry array.

apostasie · 2024-05-17T00:37:15Z

cmd/nerdctl/image_inspect_test.go

+}
+
+func TestImageInspectDifferentValidReferencesForTheSameImage(t *testing.T) {
+	testutil.DockerIncompatible(t)


If really necessary, I could split this in two parts - the part that is docker compatible and the part that is not.

Either way we need first to resolve #3011

apostasie · 2024-05-17T01:24:33Z

Reviewer:

I don't want to introduce churn here by regularly adding more fixes.
The PR in its current state may have some minor issues (specifically there is one: Docker Hub names are not shortened properly before being added to RepoTags).
In the name of keeping our sanity, and if these minor issues are not a big deal for you, I will fix these in a follow-up once this is merged.

apostasie · 2024-05-17T19:21:57Z

Windows CI failure is on me.
~~Will try to fix it.~~
I need help with that Windows thing.
I do not have access to a Windows system, and not quite sure how to use it properly so we can test with linux images.

apostasie · 2024-05-18T05:27:44Z

Note this is likely fixing issues like #2061

Zheaoli · 2024-05-18T18:10:59Z

I will convert this PR to a draft before we solve the Windows issue together. I will help to figure out what happened.

Zheaoli · 2024-05-18T18:13:32Z

https://github.com/containerd/nerdctl/blob/main/cmd/nerdctl/image_save_test.go#L35C22-L35C44

You can output the inspect.RepoDigests[0] in test during the debug, I think there one more : char in the string

apostasie · 2024-05-18T21:40:26Z

https://github.com/containerd/nerdctl/blob/main/cmd/nerdctl/image_save_test.go#L35C22-L35C44

You can output the inspect.RepoDigests[0] in test during the debug, I think there one more : char in the string

Thanks @Zheaoli

I believe the failure on save_test (TestSaveById) are unrelated, and are just flakyness.

The actual test failing is the one I introduced, and the issue here is that I am using test images that do not have a Windows version, so, pull fails on windows: https://github.com/containerd/nerdctl/actions/runs/9133230483/job/25136088395?pr=3017#step:8:569

As I said, unfortunately, I know very little about Win...

apostasie · 2024-05-19T06:05:06Z

More accurately, the issue is:
time="2024-05-18T17:41:26Z" level=fatal msg="failed to extract layer sha256:d4fc045c9e3a848011de66f34b81f052d4f2c15a17bb196d637e526349601820: hcsshim::ProcessBaseLayer \\\\?\\C:\\ProgramData\\containerd\\root\\io.containerd.snapshotter.v1.windows\\snapshots\\106: The system cannot find the path specified.: unknown"

And I know nothing about hcsshim, or why it would fail extracting an image.

apostasie · 2024-05-30T02:32:12Z

@Zheaoli @AkihiroSuda
CI is green.
I disabled the test on window for now.
Will revisit soon when I get a Windows environment to test on.

README.md

cmd/nerdctl/image_inspect_test.go

ktock · 2024-05-30T02:56:03Z

cmd/nerdctl/image_inspect_test.go

+	// Prove that invalid reference return no result without crashing
+	for _, id := range []string{"∞∞∞∞∞∞∞∞∞∞", "busybox:∞∞∞∞∞∞∞∞∞∞"} {


Can we use more readable string like ! or a very long tag?

FYI: https://docs.docker.com/reference/cli/docker/image/tag/#description

The tag must be valid ASCII and can contain lowercase and uppercase letters, digits, underscores, periods, and hyphens. It can't start with a period or hyphen and must be no longer than 128 characters.

I prefer something outside of the ASCII range if you don't mind, as this tends to uncover issues not necessarily seen without.

pkg/cmd/image/inspect.go

ktock

LGTM on green

Signed-off-by: apostasie <[email protected]>

Zheaoli

LGTM

AkihiroSuda · 2024-06-01T15:05:49Z

@ktock @Zheaoli You can merge PRs

apostasie commented May 17, 2024

View reviewed changes

pkg/referenceutil/referenceutil.go Outdated Show resolved Hide resolved

apostasie commented May 17, 2024

View reviewed changes

apostasie force-pushed the dev-image-inspect branch 2 times, most recently from 3dbeff3 to 27becf0 Compare May 17, 2024 00:35

apostasie commented May 17, 2024

View reviewed changes

AkihiroSuda requested a review from Zheaoli May 17, 2024 00:40

apostasie force-pushed the dev-image-inspect branch from 27becf0 to 909d4af Compare May 17, 2024 19:21

apostasie mentioned this pull request May 18, 2024

image history issues #3019

Open

Zheaoli marked this pull request as draft May 18, 2024 18:11

apostasie force-pushed the dev-image-inspect branch from 909d4af to 12a3485 Compare May 19, 2024 17:44

apostasie force-pushed the dev-image-inspect branch from 12a3485 to ca4e4c9 Compare May 30, 2024 02:08

apostasie marked this pull request as ready for review May 30, 2024 02:32

AkihiroSuda added this to the v2.0.0 milestone May 30, 2024

AkihiroSuda requested a review from ktock May 30, 2024 02:34

ktock reviewed May 30, 2024

View reviewed changes

pkg/cmd/image/inspect.go Show resolved Hide resolved

apostasie force-pushed the dev-image-inspect branch from ca4e4c9 to ada252c Compare May 30, 2024 03:15

apostasie mentioned this pull request May 30, 2024

State how to run a single integration test #3040

Merged

ktock approved these changes May 30, 2024

View reviewed changes

Image inspect rework

eb001fa

Signed-off-by: apostasie <[email protected]>

apostasie force-pushed the dev-image-inspect branch from ada252c to eb001fa Compare May 30, 2024 03:29

Zheaoli approved these changes May 30, 2024

View reviewed changes

ktock merged commit 27396f5 into containerd:main Jun 2, 2024
22 checks passed

This was referenced Jun 2, 2024

nerdctl inspect does not allow inspecting networks, volumes #3005

Open

Current implementation of images inspect is faulty (eg: RepoTags) #3004

Closed

nerdctl rmi can be broken by using tag names with repo short sha #3016

Closed

austinvazquez mentioned this pull request Dec 27, 2024

Fix image inspect exit code on image not found error #3792

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Image inspect rework #3017

Image inspect rework #3017

apostasie commented May 17, 2024 •

edited

Loading

apostasie May 17, 2024 •

edited

Loading

apostasie May 17, 2024

apostasie May 17, 2024 •

edited

Loading

apostasie May 17, 2024

apostasie May 17, 2024

apostasie May 17, 2024

apostasie May 17, 2024

apostasie commented May 17, 2024

apostasie commented May 17, 2024 •

edited

Loading

apostasie commented May 18, 2024

Zheaoli commented May 18, 2024

Zheaoli commented May 18, 2024

apostasie commented May 18, 2024 •

edited

Loading

apostasie commented May 19, 2024

apostasie commented May 30, 2024

ktock May 30, 2024

apostasie May 30, 2024

ktock left a comment

Zheaoli left a comment

AkihiroSuda commented Jun 1, 2024

		// Prove that invalid reference return no result without crashing
		for _, id := range []string{"∞∞∞∞∞∞∞∞∞∞", "busybox:∞∞∞∞∞∞∞∞∞∞"} {

Image inspect rework #3017

Image inspect rework #3017

Conversation

apostasie commented May 17, 2024 • edited Loading

apostasie May 17, 2024 • edited Loading

Choose a reason for hiding this comment

apostasie May 17, 2024

Choose a reason for hiding this comment

apostasie May 17, 2024 • edited Loading

Choose a reason for hiding this comment

apostasie May 17, 2024

Choose a reason for hiding this comment

apostasie May 17, 2024

Choose a reason for hiding this comment

apostasie May 17, 2024

Choose a reason for hiding this comment

apostasie May 17, 2024

Choose a reason for hiding this comment

apostasie commented May 17, 2024

apostasie commented May 17, 2024 • edited Loading

apostasie commented May 18, 2024

Zheaoli commented May 18, 2024

Zheaoli commented May 18, 2024

apostasie commented May 18, 2024 • edited Loading

apostasie commented May 19, 2024

apostasie commented May 30, 2024

ktock May 30, 2024

Choose a reason for hiding this comment

apostasie May 30, 2024

Choose a reason for hiding this comment

ktock left a comment

Choose a reason for hiding this comment

Zheaoli left a comment

Choose a reason for hiding this comment

AkihiroSuda commented Jun 1, 2024

apostasie commented May 17, 2024 •

edited

Loading

apostasie May 17, 2024 •

edited

Loading

apostasie May 17, 2024 •

edited

Loading

apostasie commented May 17, 2024 •

edited

Loading

apostasie commented May 18, 2024 •

edited

Loading