avoid appending directory as file path in s3 driver Walk #4485

flavianmissi · 2024-10-14T12:46:52Z

when a directory is empty, the s3 api lists it with a trailing slash.
this causes the path to be appended twice to the walkInfo slice, causing
purge uploads path transformations to panic when the _uploads is
emtpy.

this adds a check for file paths ending on slash, and do not append
those as regular files to the walkInfo slice.

fixes #4358

Signed-off-by: Flavian Missi <fmissi@redhat.com>

when a directory is empty, the s3 api lists it with a trailing slash. this causes the path to be appended twice to the walkInfo slice, causing purge uploads path transformations to panic when the `_uploads` is emtpy. this adds a check for file paths ending on slash, and do not append those as regular files to the walkInfo slice. fixes distribution#4358 Signed-off-by: Flavian Missi <fmissi@redhat.com>

milosgajdos

I need to have a proper look at this, but this seems like a bit of a "hack" 🤔

milosgajdos · 2024-10-14T16:32:13Z

registry/storage/driver/s3-aws/s3.go

+			// to skip appending filePath to walkInfos if it ends in "/". the loop through
+			// dirs will already have handled it in that case, so it's safe to continue this
+			// loop.
+			if strings.HasSuffix(filePath, "/") {


Hmm, curious. The fact that a "directory" is "leaking" into "files" makes me wonder about directoryDiff func 🤔

Ignore this wondering, see: #4485 (comment)

flavianmissi · 2024-10-15T08:41:51Z

I need to have a proper look at this, but this seems like a bit of a "hack" 🤔

Yeah that's fair. I'll dig some more and see if there's a better way to solve this.
This comment gives interesting insight into the current behavior:

// When the "delimiter" argument is omitted, the S3 list API will list all objects in the bucket
// recursively, omitting directory paths. Objects are listed in sorted, depth-first order so we
// can infer all the directories by comparing each object path to the last one we saw.
// See: https://docs.aws.amazon.com/AmazonS3/latest/userguide/ListingKeysUsingAPIs.html

The claim that directory paths are omitted is only true when the directory in question is not empty. directoryDiff seems to only work on non-empty directories.

Here's a sample output of list objects v2:

$ aws s3api list-objects-v2 --bucket fmissi-images --prefix=s3walktest
CONTENTS        "f339720b172fa37468491c1d298a2f70"      s3walktest/docker/registry/v2/blobs/sha256/04/046909    2024-10-15T08:38:29+00:00       50      STANDARD
CONTENTS        "47f2f2a00ed0450266d0385769ef0347"      s3walktest/docker/registry/v2/blobs/sha256/07/071a45    2024-10-15T08:38:29+00:00       50      STANDARD
CONTENTS        "44de784b8d8e7a1b9af85dffb5f072f2"      s3walktest/docker/registry/v2/repositories/testns/testimg/_layers/sha256/2a43dc 2024-10-15T08:38:30+00:00       77      STANDARD
CONTENTS        "27516816763353e1abfd9501a319a4f2"      s3walktest/docker/registry/v2/repositories/testns/testimg/_layers/sha256/3ae7e8 2024-10-15T08:38:30+00:00       77      STANDARD
CONTENTS        "eed715501bf8c634e10462dff0375db4"      s3walktest/docker/registry/v2/repositories/testns/testimg/_manifests/revisions/sha256/3ae7e8    2024-10-15T08:38:30+00:00       90      STANDARD
CONTENTS        "1fc2b1056c14903607fc17c961f90ee3"      s3walktest/docker/registry/v2/repositories/testns/testimg/_uploads/     2024-10-15T08:38:30+00:00       65      STANDARD

milosgajdos · 2024-10-15T09:20:36Z

The claim that directory paths are omitted is only true when the directory in question is not empty. directoryDiff seems to only work on non-empty directories.

I have often wondered about that comment, though I can't remember if I ever looked into whether those statements hold 🥲

Ah them pesky edge conditions.........I'd still prefer modifying directoryDiff 🤔

flavianmissi · 2024-10-15T12:34:48Z

Ah them pesky edge conditions.........I'd still prefer modifying directoryDiff 🤔

After looking into it I think directoryDiff is innocent. It correctly returns the _uploads entry as a dir.
I wrote the following test for it:

func TestDirectoryDiff(t *testing.T) {
	prefix := "docker/registry/v2"
	filePath := "docker/registry/v2/repositories/testns/testimg/_uploads/"
	dirs := directoryDiff(prefix, filePath)
	expectedDirs := []string{
		"docker/registry/v2/repositories",
		"docker/registry/v2/repositories/testns",
		"docker/registry/v2/repositories/testns/testimg",
		"docker/registry/v2/repositories/testns/testimg/_uploads",
	}
	if !slices.Equal(dirs, expectedDirs) {
		t.Fatalf("directoryDiff returned incorrect list of dirs.\nwanted: %v\ngot:    %v", expectedDirs, dirs)
	}
}

It works as expected:

go test -v -run TestDirectoryDiff  ./registry/storage/driver/s3-aws
=== RUN   TestDirectoryDiff
--- PASS: TestDirectoryDiff (0.00s)
PASS
ok  	github.com/distribution/distribution/v3/registry/storage/driver/s3-aws	1.033s

Could you expand on how you would expect directoryDiff to behave instead?

The entire loop through objects.Contents in doWalk was written with the premise that every file.Key entry listed is an actual file path, and we just found out that this isn't the case. I'm honestly not sure how to fix this without treating it like an exception (and without rewriting the entire loop, which I can do but didn't think necessary).

Anyways, I'll pause this while I wait for further feedback.
Thanks for reviewing o/

milosgajdos · 2024-10-15T12:44:20Z

I was speculating. I didn't make any recommendations or say anything other than "it makes me wonder" which is does/did. I'd have to zoom in when I find some free time.

flavianmissi · 2024-10-15T14:23:43Z

Ohh, thanks for clarifying.
Either way I'll sleep on it, maybe I'll wake up with an idea of a more elegant way to approach this.

flavianmissi · 2024-10-16T13:49:57Z

oh my look at what I found: 87cbd09#diff-69ca1a0abf51616b5031c87b33ce5a35fb6439b5bcd744aef65ddbc2131ec431R170
this explains why older versions do not panic 😞

EDIT: this is the commit that removed that test: f78d81e

milosgajdos · 2024-10-16T14:06:27Z

this explains why older versions do not panic 😞

Removing a test explains why the older version doesn't panic? Very strange and kinda.... worrying

milosgajdos · 2024-10-16T14:11:47Z

oh you mean this #3302

flavianmissi · 2024-10-16T14:57:39Z

Yes! PR 3302 fixes the same error reported on #4358, and the regression is caused by f78d81e which not only removes the test but also the code treating the /.

milosgajdos · 2024-10-16T15:01:44Z

Ok, makes sense. Thanks for digging in. Let's roll with this then!

milosgajdos

PTAL @Jamstah

milosgajdos · 2024-10-16T15:02:19Z

registry/storage/driver/s3-aws/s3.go

+			// to skip appending filePath to walkInfos if it ends in "/". the loop through
+			// dirs will already have handled it in that case, so it's safe to continue this
+			// loop.
+			if strings.HasSuffix(filePath, "/") {


Ignore this wondering, see: #4485 (comment)

squizzi

lgtm, @Jamstah should probably take a look too though

squizzi · 2024-10-16T20:10:58Z

registry/storage/driver/s3-aws/s3_test.go

+			},
+		})
+		if err != nil {
+			t.Logf("DeleteObjectsWithContext resp: %+v", resp)


thanks for these extras, they'll be useful for test debugging to someone one day

github-actions bot added area/storage area/storage/s3 labels Oct 14, 2024

flavianmissi force-pushed the purge-uploads-panic branch from 471166f to 68e80d2 Compare October 14, 2024 12:47

flavianmissi mentioned this pull request Oct 14, 2024

Registry keeps crashing after starting "purgeuploads.go" #4358

Closed

flavianmissi added 2 commits October 14, 2024 14:53

test s3 driver walk of empty dir

e44d931

Signed-off-by: Flavian Missi <fmissi@redhat.com>

flavianmissi force-pushed the purge-uploads-panic branch from 68e80d2 to 2e7482c Compare October 14, 2024 12:53

milosgajdos requested a review from Jamstah October 14, 2024 14:58

milosgajdos reviewed Oct 14, 2024

View reviewed changes

milosgajdos approved these changes Oct 16, 2024

View reviewed changes

milosgajdos requested review from thaJeztah, davidspek, wy65701436 and squizzi October 16, 2024 15:02

squizzi approved these changes Oct 16, 2024

View reviewed changes

milosgajdos merged commit bce9fcd into distribution:main Oct 16, 2024
17 checks passed

flavianmissi deleted the purge-uploads-panic branch October 17, 2024 08:04

milosgajdos mentioned this pull request Mar 6, 2025

S3 PurgeUploads panic if a root directory in the registry repositories folder is empty #4590

Open

avoid appending directory as file path in s3 driver Walk #4485

avoid appending directory as file path in s3 driver Walk #4485

Uh oh!

Conversation

flavianmissi commented Oct 14, 2024

Uh oh!

milosgajdos left a comment

Choose a reason for hiding this comment

Uh oh!

milosgajdos Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

milosgajdos Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

flavianmissi commented Oct 15, 2024

Uh oh!

milosgajdos commented Oct 15, 2024

Uh oh!

flavianmissi commented Oct 15, 2024

Uh oh!

milosgajdos commented Oct 15, 2024

Uh oh!

flavianmissi commented Oct 15, 2024

Uh oh!

flavianmissi commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

milosgajdos commented Oct 16, 2024

Uh oh!

milosgajdos commented Oct 16, 2024

Uh oh!

flavianmissi commented Oct 16, 2024

Uh oh!

milosgajdos commented Oct 16, 2024

Uh oh!

milosgajdos left a comment

Choose a reason for hiding this comment

Uh oh!

milosgajdos Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

squizzi left a comment

Choose a reason for hiding this comment

Uh oh!

squizzi Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

flavianmissi commented Oct 16, 2024 •

edited

Loading