Functional options for NewRegistryWithDriver

Clean up calling convention for NewRegistryWithDriver to use functional
arguments.

This is a first step towards the refactor described in #215. I plan to
add additional options in the process of moving configurable items from
the App structure to the registry structure.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

docs/handlers/app.go

@@ -118,13 +118,18 @@ func NewApp(ctx context.Context, configuration configuration.Configuration) *App
 	app.configureRedis(&configuration)
 	app.configureLogHook(&configuration)
 
+	options := []storage.RegistryOption{}
+
+	if app.isCache {
+		options = append(options, storage.DisableDigestResumption)
+	}
+
 	// configure deletion
-	var deleteEnabled bool
 	if d, ok := configuration.Storage["delete"]; ok {
 		e, ok := d["enabled"]
 		if ok {
-			if deleteEnabled, ok = e.(bool); !ok {
+			if deleteEnabled, ok := e.(bool); ok && deleteEnabled {
-				deleteEnabled = false
+				options = append(options, storage.EnableDelete)
 			}
 		}
 	}
@@ -139,10 +144,11 @@ func NewApp(ctx context.Context, configuration configuration.Configuration) *App
 		default:
 			panic(fmt.Sprintf("invalid type for redirect config: %#v", redirectConfig))
 		}
-
+	}
 	if redirectDisabled {
 		ctxu.GetLogger(app).Infof("backend redirection disabled")
-		}
+	} else {
+		options = append(options, storage.EnableRedirect)
 	}
 
 	// configure storage caches
@@ -158,10 +164,20 @@ func NewApp(ctx context.Context, configuration configuration.Configuration) *App
 			if app.redis == nil {
 				panic("redis configuration required to use for layerinfo cache")
 			}
-			app.registry = storage.NewRegistryWithDriver(app, app.driver, rediscache.NewRedisBlobDescriptorCacheProvider(app.redis), deleteEnabled, !redirectDisabled, app.isCache)
+			cacheProvider := rediscache.NewRedisBlobDescriptorCacheProvider(app.redis)
+			localOptions := append(options, storage.BlobDescriptorCacheProvider(cacheProvider))
+			app.registry, err = storage.NewRegistry(app, app.driver, localOptions...)
+			if err != nil {
+				panic("could not create registry: " + err.Error())
+			}
 			ctxu.GetLogger(app).Infof("using redis blob descriptor cache")
 		case "inmemory":
-			app.registry = storage.NewRegistryWithDriver(app, app.driver, memorycache.NewInMemoryBlobDescriptorCacheProvider(), deleteEnabled, !redirectDisabled, app.isCache)
+			cacheProvider := memorycache.NewInMemoryBlobDescriptorCacheProvider()
+			localOptions := append(options, storage.BlobDescriptorCacheProvider(cacheProvider))
+			app.registry, err = storage.NewRegistry(app, app.driver, localOptions...)
+			if err != nil {
+				panic("could not create registry: " + err.Error())
+			}
 			ctxu.GetLogger(app).Infof("using inmemory blob descriptor cache")
 		default:
 			if v != "" {
@@ -172,7 +188,10 @@ func NewApp(ctx context.Context, configuration configuration.Configuration) *App
 
 	if app.registry == nil {
 		// configure the registry if no cache section is available.
-		app.registry = storage.NewRegistryWithDriver(app.Context, app.driver, nil, deleteEnabled, !redirectDisabled, app.isCache)
+		app.registry, err = storage.NewRegistry(app.Context, app.driver, options...)
+		if err != nil {
+			panic("could not create registry: " + err.Error())
+		}
 	}
 
 	app.registry, err = applyRegistryMiddleware(app.Context, app.registry, configuration.Middleware["registry"])

docs/handlers/app_test.go

@@ -26,12 +26,16 @@ import (
 func TestAppDispatcher(t *testing.T) {
 	driver := inmemory.New()
 	ctx := context.Background()
+	registry, err := storage.NewRegistry(ctx, driver, storage.BlobDescriptorCacheProvider(memorycache.NewInMemoryBlobDescriptorCacheProvider()), storage.EnableDelete, storage.EnableRedirect)
+	if err != nil {
+		t.Fatalf("error creating registry: %v", err)
+	}
 	app := &App{
 		Config:   configuration.Configuration{},
 		Context:  ctx,
 		router:   v2.Router(),
 		driver:   driver,
-		registry: storage.NewRegistryWithDriver(ctx, driver, memorycache.NewInMemoryBlobDescriptorCacheProvider(), true, true, false),
+		registry: registry,
 	}
 	server := httptest.NewServer(app)
 	router := v2.Router()

docs/proxy/proxyblobstore_test.go

@@ -80,13 +80,19 @@ func (te testEnv) RemoteStats() *map[string]int {
 func makeTestEnv(t *testing.T, name string) testEnv {
 	ctx := context.Background()
 
-	localRegistry := storage.NewRegistryWithDriver(ctx, inmemory.New(), memory.NewInMemoryBlobDescriptorCacheProvider(), false, true, true)
+	localRegistry, err := storage.NewRegistry(ctx, inmemory.New(), storage.BlobDescriptorCacheProvider(memory.NewInMemoryBlobDescriptorCacheProvider()), storage.EnableRedirect, storage.DisableDigestResumption)
+	if err != nil {
+		t.Fatalf("error creating registry: %v", err)
+	}
 	localRepo, err := localRegistry.Repository(ctx, name)
 	if err != nil {
 		t.Fatalf("unexpected error getting repo: %v", err)
 	}
 
-	truthRegistry := storage.NewRegistryWithDriver(ctx, inmemory.New(), memory.NewInMemoryBlobDescriptorCacheProvider(), false, false, false)
+	truthRegistry, err := storage.NewRegistry(ctx, inmemory.New(), storage.BlobDescriptorCacheProvider(memory.NewInMemoryBlobDescriptorCacheProvider()))
+	if err != nil {
+		t.Fatalf("error creating registry: %v", err)
+	}
 	truthRepo, err := truthRegistry.Repository(ctx, name)
 	if err != nil {
 		t.Fatalf("unexpected error getting repo: %v", err)

docs/proxy/proxymanifeststore_test.go

@@ -73,7 +73,10 @@ func (sm statsManifest) Tags() ([]string, error) {
 
 func newManifestStoreTestEnv(t *testing.T, name, tag string) *manifestStoreTestEnv {
 	ctx := context.Background()
-	truthRegistry := storage.NewRegistryWithDriver(ctx, inmemory.New(), memory.NewInMemoryBlobDescriptorCacheProvider(), false, false, false)
+	truthRegistry, err := storage.NewRegistry(ctx, inmemory.New(), storage.BlobDescriptorCacheProvider(memory.NewInMemoryBlobDescriptorCacheProvider()))
+	if err != nil {
+		t.Fatalf("error creating registry: %v", err)
+	}
 	truthRepo, err := truthRegistry.Repository(ctx, name)
 	if err != nil {
 		t.Fatalf("unexpected error getting repo: %v", err)
@@ -92,7 +95,10 @@ func newManifestStoreTestEnv(t *testing.T, name, tag string) *manifestStoreTestE
 		t.Fatalf(err.Error())
 	}
 
-	localRegistry := storage.NewRegistryWithDriver(ctx, inmemory.New(), memory.NewInMemoryBlobDescriptorCacheProvider(), false, true, true)
+	localRegistry, err := storage.NewRegistry(ctx, inmemory.New(), storage.BlobDescriptorCacheProvider(memory.NewInMemoryBlobDescriptorCacheProvider()), storage.EnableRedirect, storage.DisableDigestResumption)
+	if err != nil {
+		t.Fatalf("error creating registry: %v", err)
+	}
 	localRepo, err := localRegistry.Repository(ctx, name)
 	if err != nil {
 		t.Fatalf("unexpected error getting repo: %v", err)

docs/storage/blob_test.go

@@ -33,7 +33,10 @@ func TestSimpleBlobUpload(t *testing.T) {
 	ctx := context.Background()
 	imageName := "foo/bar"
 	driver := inmemory.New()
-	registry := NewRegistryWithDriver(ctx, driver, memory.NewInMemoryBlobDescriptorCacheProvider(), true, true, false)
+	registry, err := NewRegistry(ctx, driver, BlobDescriptorCacheProvider(memory.NewInMemoryBlobDescriptorCacheProvider()), EnableDelete, EnableRedirect)
+	if err != nil {
+		t.Fatalf("error creating registry: %v", err)
+	}
 	repository, err := registry.Repository(ctx, imageName)
 	if err != nil {
 		t.Fatalf("unexpected error getting repo: %v", err)
@@ -193,7 +196,10 @@ func TestSimpleBlobUpload(t *testing.T) {
 	}
 
 	// Reuse state to test delete with a delete-disabled registry
-	registry = NewRegistryWithDriver(ctx, driver, memory.NewInMemoryBlobDescriptorCacheProvider(), false, true, false)
+	registry, err = NewRegistry(ctx, driver, BlobDescriptorCacheProvider(memory.NewInMemoryBlobDescriptorCacheProvider()), EnableRedirect)
+	if err != nil {
+		t.Fatalf("error creating registry: %v", err)
+	}
 	repository, err = registry.Repository(ctx, imageName)
 	if err != nil {
 		t.Fatalf("unexpected error getting repo: %v", err)
@@ -212,7 +218,10 @@ func TestSimpleBlobRead(t *testing.T) {
 	ctx := context.Background()
 	imageName := "foo/bar"
 	driver := inmemory.New()
-	registry := NewRegistryWithDriver(ctx, driver, memory.NewInMemoryBlobDescriptorCacheProvider(), true, true, false)
+	registry, err := NewRegistry(ctx, driver, BlobDescriptorCacheProvider(memory.NewInMemoryBlobDescriptorCacheProvider()), EnableDelete, EnableRedirect)
+	if err != nil {
+		t.Fatalf("error creating registry: %v", err)
+	}
 	repository, err := registry.Repository(ctx, imageName)
 	if err != nil {
 		t.Fatalf("unexpected error getting repo: %v", err)
@@ -316,7 +325,10 @@ func TestLayerUploadZeroLength(t *testing.T) {
 	ctx := context.Background()
 	imageName := "foo/bar"
 	driver := inmemory.New()
-	registry := NewRegistryWithDriver(ctx, driver, memory.NewInMemoryBlobDescriptorCacheProvider(), true, true, false)
+	registry, err := NewRegistry(ctx, driver, BlobDescriptorCacheProvider(memory.NewInMemoryBlobDescriptorCacheProvider()), EnableDelete, EnableRedirect)
+	if err != nil {
+		t.Fatalf("error creating registry: %v", err)
+	}
 	repository, err := registry.Repository(ctx, imageName)
 	if err != nil {
 		t.Fatalf("unexpected error getting repo: %v", err)

docs/storage/catalog_test.go

@@ -22,7 +22,10 @@ func setupFS(t *testing.T) *setupEnv {
 	d := inmemory.New()
 	c := []byte("")
 	ctx := context.Background()
-	registry := NewRegistryWithDriver(ctx, d, memory.NewInMemoryBlobDescriptorCacheProvider(), false, true, false)
+	registry, err := NewRegistry(ctx, d, BlobDescriptorCacheProvider(memory.NewInMemoryBlobDescriptorCacheProvider()), EnableRedirect)
+	if err != nil {
+		t.Fatalf("error creating registry: %v", err)
+	}
 	rootpath, _ := pathFor(repositoriesRootPathSpec{})
 
 	repos := []string{

docs/storage/manifeststore_test.go

@@ -29,7 +29,10 @@ type manifestStoreTestEnv struct {
 func newManifestStoreTestEnv(t *testing.T, name, tag string) *manifestStoreTestEnv {
 	ctx := context.Background()
 	driver := inmemory.New()
-	registry := NewRegistryWithDriver(ctx, driver, memory.NewInMemoryBlobDescriptorCacheProvider(), true, true, false)
+	registry, err := NewRegistry(ctx, driver, BlobDescriptorCacheProvider(memory.NewInMemoryBlobDescriptorCacheProvider()), EnableDelete, EnableRedirect)
+	if err != nil {
+		t.Fatalf("error creating registry: %v", err)
+	}
 
 	repo, err := registry.Repository(ctx, name)
 	if err != nil {
@@ -348,7 +351,10 @@ func TestManifestStorage(t *testing.T) {
 		t.Errorf("Deleted manifest get returned non-nil")
 	}
 
-	r := NewRegistryWithDriver(ctx, env.driver, memory.NewInMemoryBlobDescriptorCacheProvider(), false, true, false)
+	r, err := NewRegistry(ctx, env.driver, BlobDescriptorCacheProvider(memory.NewInMemoryBlobDescriptorCacheProvider()), EnableRedirect)
+	if err != nil {
+		t.Fatalf("error creating registry: %v", err)
+	}
 	repo, err := r.Repository(ctx, env.name)
 	if err != nil {
 		t.Fatalf("unexpected error getting repo: %v", err)

docs/storage/registry.go

@@ -12,28 +12,65 @@ import (
 // package. All instances should descend from this object.
 type registry struct {
 	blobStore                   *blobStore
-	blobServer                  distribution.BlobServer
+	blobServer                  *blobServer
-	statter                     distribution.BlobStatter // global statter service.
+	statter                     *blobStatter // global statter service.
 	blobDescriptorCacheProvider cache.BlobDescriptorCacheProvider
 	deleteEnabled               bool
 	resumableDigestEnabled      bool
 }
 
-// NewRegistryWithDriver creates a new registry instance from the provided
+// RegistryOption is the type used for functional options for NewRegistry.
-// driver. The resulting registry may be shared by multiple goroutines but is
+type RegistryOption func(*registry) error
-// cheap to allocate. If redirect is true, the backend blob server will
-// attempt to use (StorageDriver).URLFor to serve all blobs.
-//
-// TODO(stevvooe): This function signature is getting very out of hand. Move to
-// functional options for instance configuration.
-func NewRegistryWithDriver(ctx context.Context, driver storagedriver.StorageDriver, blobDescriptorCacheProvider cache.BlobDescriptorCacheProvider, deleteEnabled bool, redirect bool, isCache bool) distribution.Namespace {
-	// create global statter, with cache.
-	var statter distribution.BlobDescriptorService = &blobStatter{
-		driver: driver,
-	}
 
+// EnableRedirect is a functional option for NewRegistry. It causes the backend
+// blob server to attempt using (StorageDriver).URLFor to serve all blobs.
+func EnableRedirect(registry *registry) error {
+	registry.blobServer.redirect = true
+	return nil
+}
+
+// EnableDelete is a functional option for NewRegistry. It enables deletion on
+// the registry.
+func EnableDelete(registry *registry) error {
+	registry.deleteEnabled = true
+	return nil
+}
+
+// DisableDigestResumption is a functional option for NewRegistry. It should be
+// used if the registry is acting as a caching proxy.
+func DisableDigestResumption(registry *registry) error {
+	registry.resumableDigestEnabled = false
+	return nil
+}
+
+// BlobDescriptorCacheProvider returns a functional option for
+// NewRegistry. It creates a cached blob statter for use by the
+// registry.
+func BlobDescriptorCacheProvider(blobDescriptorCacheProvider cache.BlobDescriptorCacheProvider) RegistryOption {
+	// TODO(aaronl): The duplication of statter across several objects is
+	// ugly, and prevents us from using interface types in the registry
+	// struct. Ideally, blobStore and blobServer should be lazily
+	// initialized, and use the current value of
+	// blobDescriptorCacheProvider.
+	return func(registry *registry) error {
 		if blobDescriptorCacheProvider != nil {
-		statter = cache.NewCachedBlobStatter(blobDescriptorCacheProvider, statter)
+			statter := cache.NewCachedBlobStatter(blobDescriptorCacheProvider, registry.statter)
+			registry.blobStore.statter = statter
+			registry.blobServer.statter = statter
+			registry.blobDescriptorCacheProvider = blobDescriptorCacheProvider
+		}
+		return nil
+	}
+}
+
+// NewRegistry creates a new registry instance from the provided driver. The
+// resulting registry may be shared by multiple goroutines but is cheap to
+// allocate. If the Redirect option is specified, the backend blob server will
+// attempt to use (StorageDriver).URLFor to serve all blobs.
+func NewRegistry(ctx context.Context, driver storagedriver.StorageDriver, options ...RegistryOption) (distribution.Namespace, error) {
+	// create global statter
+	statter := &blobStatter{
+		driver: driver,
 	}
 
 	bs := &blobStore{
@@ -41,18 +78,24 @@ func NewRegistryWithDriver(ctx context.Context, driver storagedriver.StorageDriv
 		statter: statter,
 	}
 
-	return &registry{
+	registry := &registry{
 		blobStore: bs,
 		blobServer: &blobServer{
 			driver:  driver,
 			statter: statter,
 			pathFn:  bs.path,
-			redirect: redirect,
 		},
-		blobDescriptorCacheProvider: blobDescriptorCacheProvider,
+		statter:                statter,
-		deleteEnabled:               deleteEnabled,
+		resumableDigestEnabled: true,
-		resumableDigestEnabled:      !isCache,
 	}
+
+	for _, option := range options {
+		if err := option(registry); err != nil {
+			return nil, err
+		}
+	}
+
+	return registry, nil
 }
 
 // Scope returns the namespace scope for a registry. The registry