From 3a93fecf367aceca7cd4cf9d6d91be793f2b4210 Mon Sep 17 00:00:00 2001 From: Flavian Missi Date: Wed, 7 Feb 2024 16:58:00 +0100 Subject: [PATCH] wip: add back leading slash on azure driver also support no leading slash, since some customers are running the registry with the previous code that removed it. --- registry/storage/driver/azure/azure.go | 46 ++++--- registry/storage/driver/azure/azure_test.go | 115 +++++++++++++++--- .../sdk/storage/azblob/container/client.go | 5 +- 3 files changed, 133 insertions(+), 33 deletions(-) diff --git a/registry/storage/driver/azure/azure.go b/registry/storage/driver/azure/azure.go index 0fb28bcdda1..9612958d81d 100644 --- a/registry/storage/driver/azure/azure.go +++ b/registry/storage/driver/azure/azure.go @@ -66,6 +66,7 @@ func New(params *Parameters) (*Driver, error) { client: client, rootDirectory: params.RootDirectory, } + driver := &Driver{baseEmbed: baseEmbed{Base: base.Base{StorageDriver: d}}} // the previous code removed the leading "/" on paths. // this caused backwards incompatible upgrades between v2 and v3 @@ -77,34 +78,47 @@ func New(params *Parameters) (*Driver, error) { // paths start with a leading slach by default in v2 - to be backwards // compatible we default to that and set it to false if we detect the // existing storage does not use it. - d.useLeadingSlash = true + + // installations that upgraded from v2 to v3 (previous to this patch) + // might have data in / in their storage account, but will actually be + // using data from a path without the leading /. + // to make sure there is no data loss, we first check for data in the + // path with no leading slash, and only when we do not find data there, + // we default to the leading slash + d.useLeadingSlash = false // we use listBlobs instead of List because List is recursive, // and we only need to look at the root. - blobs, err := d.listBlobs(context.TODO(), "/") - // // TODO: revisit error handling? - if err != nil { + blobRef := d.client.NewBlobClient("/") + _, err = blobRef.GetProperties(context.TODO(), nil) + // TODO: revisit error handling? + if err != nil && !is404(err) { return nil, err } // a fresh registry install will contain no blobs in its root dir. // a registry that used the previous v3 path handling however might // also return 0 blobs under "/", but actually contain blobs under "". - if len(blobs) == 0 { - d.useLeadingSlash = false - blobs, err := d.listBlobs(context.TODO(), "/") + if is404(err) { + fmt.Println("GetProperties of empty blob returned 404") + d.useLeadingSlash = true + blobRef := d.client.NewBlobClient("//") + _, err := blobRef.GetProperties(context.TODO(), nil) // TODO: revisit error handling? - if err != nil { + if err != nil && !is404(err) { return nil, err } - if len(blobs) == 0 { - // no blobs found with and without a leading slash, this - // means this is a new registry. - d.useLeadingSlash = true - } + // blobs, err := d.listBlobs(context.TODO(), "/") + // // TODO: revisit error handling? + // if err != nil { + // return nil, err + // } + // if len(blobs) == 0 { + // d.useLeadingSlash = false + // } } - return &Driver{baseEmbed: baseEmbed{Base: base.Base{StorageDriver: d}}}, nil + return driver, nil } // Implement the storagedriver.StorageDriver interface. @@ -200,6 +214,7 @@ func (d *driver) Reader(ctx context.Context, path string, offset int64) (io.Read // at the location designated by "path" after the call to Commit. func (d *driver) Writer(ctx context.Context, path string, append bool) (storagedriver.FileWriter, error) { blobName := d.blobName(path) + fmt.Println("Writer blobName:", blobName) blobRef := d.client.NewBlobClient(blobName) props, err := blobRef.GetProperties(ctx, nil) @@ -502,6 +517,9 @@ func (d *driver) blobName(path string) string { trimmedRoot := strings.TrimRight(d.rootDirectory, "/") trimmedPath := strings.TrimLeft(path, "/") if d.useLeadingSlash { + if !strings.HasPrefix(trimmedRoot, "/") { + trimmedRoot = "/" + trimmedRoot + } return trimmedRoot + "/" + trimmedPath } diff --git a/registry/storage/driver/azure/azure_test.go b/registry/storage/driver/azure/azure_test.go index 6a3070bd979..5d6ab772112 100644 --- a/registry/storage/driver/azure/azure_test.go +++ b/registry/storage/driver/azure/azure_test.go @@ -91,44 +91,123 @@ func randStringRunes(n int) string { return string(b) } +func writeBlob(ctx context.Context, driver storagedriver.StorageDriver, contents, path string, append_ bool) error { + writer, err := driver.Writer(ctx, path, append_) + if err != nil { + return fmt.Errorf("unexpected error from driver.Writer: %v", err) + } + _, err = writer.Write([]byte(contents)) + if err != nil { + return fmt.Errorf("writer.Write: unexpected error: %v", err) + } + + err = writer.Commit() + if err != nil { + return fmt.Errorf("writer.Commit: unexpected error: %v", err) + } + err = writer.Close() + if err != nil { + return fmt.Errorf("writer.Close: unexpected error: %v", err) + } + return nil +} + func TestUseTrailingSlash(t *testing.T) { + ctx := context.Background() + fmt.Println("================================== empty storage ======================================") d, err := azureDriverConstructor() if err != nil { t.Fatalf("unexpected error creating azure driver: %v", err) } storageDriver := d.(*Driver) drvr := storageDriver.Base.StorageDriver.(*driver) - fmt.Println(drvr.useLeadingSlash) -} -func TestListBlobs(t *testing.T) { - driver, err := azureDriverConstructor() - if err != nil { - t.Fatalf("unexpected error creating azure driver: %v", err) + // at this point there should be no data in the storage with or without + // a leading slash. this simulates a fresh install of the registry, so we + // expect the default useLeadingSlash to be used (true). + if !drvr.useLeadingSlash { + t.Fatal("expected a fresh install of the registry to use a leading slash on storage paths by default") } + fmt.Println("========================================================================") + fmt.Println("================================== /docker/ ======================================") + // create files in storage with leading slash + // this should detect that files starting with a / + // exist, which will set useLeadingSlash to true contents := randStringRunes(4 * 1024 * 1024) - path := "/file" - ctx := context.Background() + path1 := "/docker/registry/v2/repositories/test/hello/_layers/sha256/abc123/link" - defer driver.Delete(ctx, path) + defer d.Delete(ctx, path1) + if err := writeBlob(ctx, d, contents, path1, false); err != nil { + t.Fatal(err) + } - writer, err := driver.Writer(ctx, path, false) + // create another instance of the storage driver. this is the only way + // to reset the value of useLeadingSlash + d, err = azureDriverConstructor() if err != nil { - t.Fatalf("unexpected error from driver.Writer: %v", err) + t.Fatalf("unexpected error creating azure driver: %v", err) } - _, err = writer.Write([]byte(contents)) - if err != nil { - t.Fatalf("writer.Write: unexpected error: %v", err) + storageDriver = d.(*Driver) + drvr = storageDriver.Base.StorageDriver.(*driver) + + // when files exist in a path with a leading / the registry is using + // existing storage and it should follow the /, useLeadingSlash should + // be true. + if !drvr.useLeadingSlash { + t.Error("expected an existing install with data under `/docker` to use a leading slash on storage paths by default") + t.Logf("sample blob name: %s", drvr.blobName("/docker/registry/v2/foo")) + } + fmt.Println("========================================================================") + + fmt.Println("========================================================================") + fmt.Println("================================== docker/ ======================================") + // now we'll simulate a case where a user updated from a v2 to a v3 registry, + // then pushed data into the registry again. in this situation, users will have + // data both, under /docker and under docker (without a leading /). + // in this case we assume the user has recovered from the apparent data loss, + // and wants to keep using the docker path (without a leading /). + // this means useLeadingSlash should be false. + contents = randStringRunes(4 * 1024 * 1024) + path2 := "/docker/registry/v2/repositories/test/hello/_layers/sha256/abc123/link" + + defer d.Delete(ctx, path2) + // we have to manually set useLeadingSlash to false so that the path ends up + // not containing the leading slash - the driver framework doesn't allow us to + // create blobs that do not start with / + drvr.useLeadingSlash = false + if err := writeBlob(ctx, d, contents, path2, false); err != nil { + t.Fatal(err) } - err = writer.Commit() + // create another instance of the storage driver. this is the only way + // to reset the value of useLeadingSlash + d, err = azureDriverConstructor() if err != nil { - t.Fatalf("writer.Commit: unexpected error: %v", err) + t.Fatalf("unexpected error creating azure driver: %v", err) } - err = writer.Close() + storageDriver = d.(*Driver) + drvr = storageDriver.Base.StorageDriver.(*driver) + + if drvr.useLeadingSlash { + t.Error("expected a existing install of the registry with data under `docker` (without a leading slash) and `/docker` (with a leading slash) to default to use `docker` (without leading slash) but it default to `/docker`") + } + fmt.Println("========================================================================") +} + +func TestListBlobs(t *testing.T) { + driver, err := azureDriverConstructor() if err != nil { - t.Fatalf("writer.Close: unexpected error: %v", err) + t.Fatalf("unexpected error creating azure driver: %v", err) + } + + contents := randStringRunes(4 * 1024 * 1024) + path := "/file" + ctx := context.Background() + + defer driver.Delete(ctx, path) + if err := writeBlob(ctx, driver, contents, path, false); err != nil { + t.Fatal(err) } blobNames, err := driver.List(ctx, "/") diff --git a/vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/container/client.go b/vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/container/client.go index 5de51e0d753..9ea0eb70781 100644 --- a/vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/container/client.go +++ b/vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/container/client.go @@ -8,11 +8,13 @@ package container import ( "context" - "github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/bloberror" + "fmt" "net/http" "net/url" "time" + "github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/bloberror" + "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime" @@ -113,6 +115,7 @@ func (c *Client) URL() string { func (c *Client) NewBlobClient(blobName string) *blob.Client { blobName = url.PathEscape(blobName) blobURL := runtime.JoinPaths(c.URL(), blobName) + fmt.Println("NewBlobClient blobURL:", blobURL) return (*blob.Client)(base.NewBlobClient(blobURL, c.generated().Pipeline(), c.sharedKey())) }