c8d/image: Allow truncated id to have sha256: prefix by vvoland · Pull Request #46435 · moby/moby

vvoland · 2023-09-08T12:12:08Z

Fixes TestInspectByPrefix when running with c8d snapshotters enabled.

- What I did

- How I did it

- How to verify it

$ make TEST_IGNORE_CGROUP_CHECK=1  \
             DOCKER_GRAPHDRIVER=overlayfs TEST_INTEGRATION_USE_SNAPSHOTTER=1  \
             TEST_FILTER='TestInspectByPrefix'  \
             test-integration

=== RUN   TestDockerCLIInspectSuite
=== RUN   TestDockerCLIInspectSuite/TestInspectByPrefix
-    docker_cli_inspect_test.go:312: assertion failed: error is not nil: failed to inspect sha256:40350:
-        Error: No such object: sha256:40350
-
---- FAIL: TestDockerCLIInspectSuite (0.13s)
-    --- FAIL: TestDockerCLIInspectSuite/TestInspectByPrefix (0.13s)
-
+--- PASS: TestDockerCLIInspectSuite (0.11s)
+    --- PASS: TestDockerCLIInspectSuite/TestInspectByPrefix (0.11s)

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

Fixes TestInspectByPrefix when running with c8d snapshotters enabled. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland · 2023-09-08T12:39:44Z

DockerCLICommitSuite|DockerCLICpSuite|DockerCLICreateSuite|DockerCLIEventSuite|D... is now 🟢 !

https://github.com/moby/moby/actions/runs/6121551710/job/16615613531?pr=45232

thaJeztah · 2023-09-08T13:27:03Z

daemon/containerd/image.go


 	// If the identifier could be a short ID, attempt to match
 	if truncatedID.MatchString(refOrID) {
+		idWithoutAlgo := strings.TrimPrefix(refOrID, "sha256:")


Do you know if the non-containerd variant actually allows ID prefixes here? The stringed. IsShortID function only accepts short (fixed length, 12-characters) ID;

moby/pkg/stringid/stringid.go

Lines 18 to 31 in 152036f

var (

validShortID = regexp.MustCompile("^[a-f0-9]{12}$")

validHex = regexp.MustCompile(`^[a-f0-9]{64}$`)

)

// IsShortID determines if id has the correct format and length for a short ID.

// It checks the IDs length and if it consists of valid characters for IDs (a-f0-9).

func IsShortID(id string) bool {

if len(id) != shortLen {

return false

}

return validShortID.MatchString(id)

}

(Also slightly wondering if trimming the algorithm before matching it would be an alternative (I haven't benchmarked the regex though))

It does, there is a test for it

Thanks! LOL, so much for consistency 😂

wouldn't surprise me if the test was written based on "it works" and not "what was meant to work"

Did a quick check; this was the PR adding that test;

Vendor distribution and fix inspect by sha256 prefix #18409

Fix image deletion conflicts with search #18443

related to Validate digest length on parsing distribution/distribution#1231

Funny thing is that that last one seems to be changing things to use longer digests in tests (and removing the short ones).

Oh, wait, but that test does use 12 characters (originally it took 10, but updated in the second PR above)? Did it overlook the sha256: prefix in that length 🤔 ?

moby/integration-cli/docker_cli_inspect_test.go

Lines 308 to 317 in 6dcefa3

func (s *DockerCLIInspectSuite) TestInspectByPrefix(c *testing.T) {

id := inspectField(c, "busybox", "Id")

assert.Assert(c, strings.HasPrefix(id, "sha256:"))

id2 := inspectField(c, id[:12], "Id")

assert.Equal(c, id, id2)

id3 := inspectField(c, strings.TrimPrefix(id, "sha256:")[:12], "Id")

assert.Equal(c, id, id3)

}

thaJeztah

LGTM

we should probably double-check if the test was meant to be supporting this (maybe, maybe not) 😂

c8d/image: Allow truncated id to have sha256: prefix

5728516

Fixes TestInspectByPrefix when running with c8d snapshotters enabled. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland added status/2-code-review area/images Image Distribution kind/bugfix PR's that fix bugs containerd-integration Issues and PRs related to containerd integration labels Sep 8, 2023

vvoland added this to the 25.0.0 milestone Sep 8, 2023

rumpl approved these changes Sep 8, 2023

View reviewed changes

thaJeztah reviewed Sep 8, 2023

View reviewed changes

thaJeztah approved these changes Sep 8, 2023

View reviewed changes

thaJeztah merged commit 0434b65 into moby:master Sep 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

c8d/image: Allow truncated id to have sha256: prefix#46435

c8d/image: Allow truncated id to have sha256: prefix#46435
thaJeztah merged 1 commit intomoby:masterfrom
vvoland:c8d-inspect-prefix

vvoland commented Sep 8, 2023

Uh oh!

vvoland commented Sep 8, 2023

Uh oh!

thaJeztah Sep 8, 2023

Uh oh!

rumpl Sep 8, 2023

Uh oh!

thaJeztah Sep 8, 2023

Uh oh!

thaJeztah Sep 8, 2023

Uh oh!

thaJeztah left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	var (
	validShortID = regexp.MustCompile("^[a-f0-9]{12}$")
	validHex = regexp.MustCompile(`^[a-f0-9]{64}$`)
	)

	// IsShortID determines if id has the correct format and length for a short ID.
	// It checks the IDs length and if it consists of valid characters for IDs (a-f0-9).
	func IsShortID(id string) bool {
	if len(id) != shortLen {
	return false
	}
	return validShortID.MatchString(id)
	}

	func (s DockerCLIInspectSuite) TestInspectByPrefix(c testing.T) {
	id := inspectField(c, "busybox", "Id")
	assert.Assert(c, strings.HasPrefix(id, "sha256:"))

	id2 := inspectField(c, id[:12], "Id")
	assert.Equal(c, id, id2)

	id3 := inspectField(c, strings.TrimPrefix(id, "sha256:")[:12], "Id")
	assert.Equal(c, id, id3)
	}

Conversation

vvoland commented Sep 8, 2023

Uh oh!

vvoland commented Sep 8, 2023

Uh oh!

thaJeztah Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

rumpl Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants