Repository: distribution/distribution
Pull request #3950 (2023-06-28 10:45:25)

Enable Go build tags
It would appear we are missing the Go build tags so the binary packaged in the image built from the source in this repo  would miss support for some storage drivers causing breakages to end users trying to use them.

This PR enables Go build tags so the GCS and OSS driver support is available in the binary distributed via the image build by Dockerfile.

This led to quite a few fixes in the GCS and OSS packages raised as warnings by `golang-ci` linter.


Patch:
diff --git a/BUILDING.md b/BUILDING.md
index 1fe2269b..4b405309 100644
--- a/BUILDING.md
+++ b/BUILDING.md
@@ -97,7 +97,7 @@ Run `make validate` to run the validators, including the linter and vendor valid
 ### Optional build tags
 
 Optional [build tags](http://golang.org/pkg/go/build/) can be provided using
-the environment variable `DOCKER_BUILDTAGS`.
+the environment variable `BUILDTAGS`.
 
 <dl>
 <dt>noresumabledigest</dt>
diff --git a/Dockerfile b/Dockerfile
index c6ebb789..85fa1335 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -22,12 +22,12 @@ RUN --mount=target=. \
 FROM base AS build
 ARG TARGETPLATFORM
 ARG LDFLAGS="-s -w"
-ARG BUILDTAGS="include_oss include_gcs"
+ARG BUILDTAGS="include_oss,include_gcs"
 RUN --mount=type=bind,target=/src,rw \
     --mount=type=cache,target=/root/.cache/go-build \
     --mount=target=/go/pkg/mod,type=cache \
     --mount=type=bind,source=/tmp/.ldflags,target=/tmp/.ldflags,from=version \
-      set -x ; xx-go build -trimpath -ldflags "$(cat /tmp/.ldflags) ${LDFLAGS}" -o /usr/bin/registry ./cmd/registry \
+      set -x ; xx-go build -tags "${BUILDTAGS}" -trimpath -ldflags "$(cat /tmp/.ldflags) ${LDFLAGS}" -o /usr/bin/registry ./cmd/registry \
       && xx-verify --static /usr/bin/registry
 
 FROM scratch AS binary
diff --git a/dockerfiles/lint.Dockerfile b/dockerfiles/lint.Dockerfile
index a9743c16..69453ba3 100644
--- a/dockerfiles/lint.Dockerfile
+++ b/dockerfiles/lint.Dockerfile
@@ -3,6 +3,7 @@
 ARG GO_VERSION=1.19.9
 ARG ALPINE_VERSION=3.16
 ARG GOLANGCI_LINT_VERSION=v1.52
+ARG BUILDTAGS="include_oss,include_gcs"
 
 FROM golangci/golangci-lint:${GOLANGCI_LINT_VERSION}-alpine AS golangci-lint
 
@@ -15,4 +16,4 @@ ENV GOFLAGS="-buildvcs=false"
 RUN --mount=type=bind,target=. \
     --mount=type=cache,target=/root/.cache \
     --mount=from=golangci-lint,source=/usr/bin/golangci-lint,target=/usr/bin/golangci-lint \
-      golangci-lint run
+      golangci-lint --build-tags "${BUILDTAGS}" run
diff --git a/registry/storage/driver/gcs/gcs.go b/registry/storage/driver/gcs/gcs.go
index d47db2e1..eb1d636b 100644
--- a/registry/storage/driver/gcs/gcs.go
+++ b/registry/storage/driver/gcs/gcs.go
@@ -1,3 +1,6 @@
+//go:build include_gcs
+// +build include_gcs
+
 // Package gcs provides a storagedriver.StorageDriver implementation to
 // store blobs in Google cloud storage.
 //
@@ -9,10 +12,6 @@
 //
 // Note that the contents of incomplete uploads are not accessible even though
 // Stat returns their length
-//
-//go:build include_gcs
-// +build include_gcs
-
 package gcs
 
 import (
@@ -66,7 +65,6 @@ var _ storagedriver.FileWriter = &writer{}
 // driverParameters is a struct that encapsulates all of the driver parameters after all values have been set
 type driverParameters struct {
 	bucket        string
-	config        *jwt.Config
 	email         string
 	privateKey    []byte
 	client        *http.Client
@@ -93,6 +91,8 @@ func (factory *gcsDriverFactory) Create(parameters map[string]interface{}) (stor
 	return FromParameters(parameters)
 }
 
+var _ storagedriver.StorageDriver = &driver{}
+
 // driver is a storagedriver.StorageDriver implementation backed by GCS
 // Objects are stored at absolute keys in the provided bucket.
 type driver struct {
@@ -289,6 +289,8 @@ func (d *driver) GetContent(ctx context.Context, path string) ([]byte, error) {
 // PutContent stores the []byte content at a location designated by "path".
 // This should primarily be used for small objects.
 func (d *driver) PutContent(ctx context.Context, path string, contents []byte) error {
+	ctx, cancel := context.WithCancel(ctx)
+	defer cancel()
 	wc := d.gcs.Bucket(d.bucket).Object(d.pathToKey(path)).NewWriter(ctx)
 	wc.ContentType = "application/octet-stream"
 	return putContentsClose(wc, contents)
@@ -312,7 +314,7 @@ func (d *driver) Reader(ctx context.Context, path string, offset int64) (io.Read
 				if err != nil {
 					return nil, err
 				}
-				if offset == int64(obj.Size) {
+				if offset == obj.Size {
 					return io.NopCloser(bytes.NewReader([]byte{})), nil
 				}
 				return nil, storagedriver.InvalidOffsetError{Path: path, Offset: offset}
@@ -450,7 +452,6 @@ func putContentsClose(wc *storage.Writer, contents []byte) error {
 		}
 	}
 	if err != nil {
-		wc.CloseWithError(err)
 		return err
 	}
 	return wc.Close()
@@ -630,9 +631,9 @@ func (d *driver) Stat(ctx context.Context, path string) (storagedriver.FileInfo,
 	// try to get as folder
 	dirpath := d.pathToDirKey(path)
 
-	var query *storage.Query
-	query = &storage.Query{}
-	query.Prefix = dirpath
+	query := &storage.Query{
+		Prefix: dirpath,
+	}
 
 	objects, err := storageListObjects(ctx, d.bucket, query, d.gcs)
 	if err != nil {
@@ -656,10 +657,10 @@ func (d *driver) Stat(ctx context.Context, path string) (storagedriver.FileInfo,
 // List returns a list of the objects that are direct descendants of the
 // given path.
 func (d *driver) List(ctx context.Context, path string) ([]string, error) {
-	var query *storage.Query
-	query = &storage.Query{}
-	query.Delimiter = "/"
-	query.Prefix = d.pathToDirKey(path)
+	query := &storage.Query{
+		Delimiter: "/",
+		Prefix:    d.pathToDirKey(path),
+	}
 	list := make([]string, 0, 64)
 	objects, err := storageListObjects(ctx, d.bucket, query, d.gcs)
 	if err != nil {
@@ -690,7 +691,7 @@ func (d *driver) List(ctx context.Context, path string) ([]string, error) {
 // Move moves an object stored at sourcePath to destPath, removing the
 // original object.
 func (d *driver) Move(ctx context.Context, sourcePath string, destPath string) error {
-	_, err := storageCopyObject(ctx, d.bucket, d.pathToKey(sourcePath), d.bucket, d.pathToKey(destPath), nil, d.gcs)
+	_, err := storageCopyObject(ctx, d.bucket, d.pathToKey(sourcePath), d.bucket, d.pathToKey(destPath), d.gcs)
 	if err != nil {
 		if status, ok := err.(*googleapi.Error); ok {
 			if status.Code == http.StatusNotFound {
@@ -794,7 +795,7 @@ func storageListObjects(ctx context.Context, bucket string, q *storage.Query, gc
 	return objs, nil
 }
 
-func storageCopyObject(ctx context.Context, srcBucket, srcName string, destBucket, destName string, attrs *storage.ObjectAttrs, gcs *storage.Client) (*storage.ObjectAttrs, error) {
+func storageCopyObject(ctx context.Context, srcBucket, srcName string, destBucket, destName string, gcs *storage.Client) (*storage.ObjectAttrs, error) {
 	src := gcs.Bucket(srcBucket).Object(srcName)
 	dst := gcs.Bucket(destBucket).Object(destName)
 	attrs, err := dst.CopierFrom(src).Run(ctx)
diff --git a/registry/storage/driver/oss/oss.go b/registry/storage/driver/oss/oss.go
index 6d42af95..aa3f0893 100644
--- a/registry/storage/driver/oss/oss.go
+++ b/registry/storage/driver/oss/oss.go
@@ -1,3 +1,6 @@
+//go:build include_oss
+// +build include_oss
+
 // Package oss provides a storagedriver.StorageDriver implementation to
 // store blobs in Aliyun OSS cloud storage.
 //
@@ -6,10 +9,6 @@
 //
 // Because OSS is a key, value store the Stat call does not support last modification
 // time for directories (directories are an abstraction for key, value stores)
-//
-//go:build include_oss
-// +build include_oss
-
 package oss
 
 import (
@@ -70,6 +69,8 @@ func (factory *ossDriverFactory) Create(parameters map[string]interface{}) (stor
 	return FromParameters(parameters)
 }
 
+var _ storagedriver.StorageDriver = &driver{}
+
 type driver struct {
 	Client          *oss.Client
 	Bucket          *oss.Bucket
@@ -507,11 +508,6 @@ func parseError(path string, err error) error {
 	return err
 }
 
-func hasCode(err error, code string) bool {
-	ossErr, ok := err.(*oss.Error)
-	return ok && ossErr.Code == code
-}
-
 func (d *driver) getOptions() oss.Options {
 	return oss.Options{
 		ServerSideEncryption:      d.Encrypt,


Test patch:
diff --git a/registry/storage/driver/gcs/gcs_test.go b/registry/storage/driver/gcs/gcs_test.go
index 74c590f3..d9f0da3e 100644
--- a/registry/storage/driver/gcs/gcs_test.go
+++ b/registry/storage/driver/gcs/gcs_test.go
@@ -66,7 +66,7 @@ func init() {
 			panic(fmt.Sprintf("Error reading JWT config : %s", err))
 		}
 		email = jwtConfig.Email
-		privateKey = []byte(jwtConfig.PrivateKey)
+		privateKey = jwtConfig.PrivateKey
 		if len(privateKey) == 0 {
 			panic("Error reading JWT config : missing private_key property")
 		}
@@ -262,6 +262,9 @@ func TestEmptyRootList(t *testing.T) {
 		}
 	}()
 	keys, err := emptyRootDriver.List(ctx, "/")
+	if err != nil {
+		t.Fatalf("unexpected error listing empty root content: %v", err)
+	}
 	for _, path := range keys {
 		if !storagedriver.PathRegexp.MatchString(path) {
 			t.Fatalf("unexpected string in path: %q != %q", path, storagedriver.PathRegexp)
@@ -269,6 +272,9 @@ func TestEmptyRootList(t *testing.T) {
 	}
 
 	keys, err = slashRootDriver.List(ctx, "/")
+	if err != nil {
+		t.Fatalf("unexpected error listing slash root content: %v", err)
+	}
 	for _, path := range keys {
 		if !storagedriver.PathRegexp.MatchString(path) {
 			t.Fatalf("unexpected string in path: %q != %q", path, storagedriver.PathRegexp)
diff --git a/registry/storage/driver/oss/oss_test.go b/registry/storage/driver/oss/oss_test.go
index a23e166a..f1518424 100644
--- a/registry/storage/driver/oss/oss_test.go
+++ b/registry/storage/driver/oss/oss_test.go
@@ -128,6 +128,9 @@ func TestEmptyRootList(t *testing.T) {
 	defer rootedDriver.Delete(ctx, filename)
 
 	keys, err := emptyRootDriver.List(ctx, "/")
+	if err != nil {
+		t.Fatalf("unexpected error listing empty root content: %v", err)
+	}
 	for _, path := range keys {
 		if !storagedriver.PathRegexp.MatchString(path) {
 			t.Fatalf("unexpected string in path: %q != %q", path, storagedriver.PathRegexp)
@@ -135,6 +138,9 @@ func TestEmptyRootList(t *testing.T) {
 	}
 
 	keys, err = slashRootDriver.List(ctx, "/")
+	if err != nil {
+		t.Fatalf("unexpected error listing slash root content: %v", err)
+	}
 	for _, path := range keys {
 		if !storagedriver.PathRegexp.MatchString(path) {
 			t.Fatalf("unexpected string in path: %q != %q", path, storagedriver.PathRegexp)