From dbbcf5fe420c2ebabd06eacbf8ca2cc4be1d3c2b Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Tue, 18 Aug 2015 10:56:27 -0700 Subject: [PATCH] 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 --- notifications/listener_test.go | 5 +- registry/handlers/app.go | 39 +++++++--- registry/handlers/app_test.go | 6 +- registry/proxy/proxyblobstore_test.go | 10 ++- registry/proxy/proxymanifeststore_test.go | 10 ++- registry/storage/blob_test.go | 20 ++++- registry/storage/catalog_test.go | 5 +- registry/storage/manifeststore_test.go | 10 ++- registry/storage/registry.go | 91 +++++++++++++++++------ 9 files changed, 149 insertions(+), 47 deletions(-) diff --git a/notifications/listener_test.go b/notifications/listener_test.go index ccd84593..04e4f02e 100644 --- a/notifications/listener_test.go +++ b/notifications/listener_test.go @@ -18,7 +18,10 @@ import ( func TestListener(t *testing.T) { ctx := context.Background() - registry := storage.NewRegistryWithDriver(ctx, inmemory.New(), memory.NewInMemoryBlobDescriptorCacheProvider(), true, true, false) + registry, err := storage.NewRegistry(ctx, inmemory.New(), storage.BlobDescriptorCacheProvider(memory.NewInMemoryBlobDescriptorCacheProvider()), storage.EnableDelete, storage.EnableRedirect) + if err != nil { + t.Fatalf("error creating registry: %v", err) + } tl := &testListener{ ops: make(map[string]int), } diff --git a/registry/handlers/app.go b/registry/handlers/app.go index c2b392d1..7d1f1cf5 100644 --- a/registry/handlers/app.go +++ b/registry/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 { - deleteEnabled = false + if deleteEnabled, ok := e.(bool); ok && deleteEnabled { + 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") - } + } + 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"]) diff --git a/registry/handlers/app_test.go b/registry/handlers/app_test.go index 3ef2342c..fbb0b188 100644 --- a/registry/handlers/app_test.go +++ b/registry/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() diff --git a/registry/proxy/proxyblobstore_test.go b/registry/proxy/proxyblobstore_test.go index 65d5f922..f8845ed3 100644 --- a/registry/proxy/proxyblobstore_test.go +++ b/registry/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) diff --git a/registry/proxy/proxymanifeststore_test.go b/registry/proxy/proxymanifeststore_test.go index 7b9b8091..9d5f3f66 100644 --- a/registry/proxy/proxymanifeststore_test.go +++ b/registry/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) diff --git a/registry/storage/blob_test.go b/registry/storage/blob_test.go index e5cfa83e..c84c7432 100644 --- a/registry/storage/blob_test.go +++ b/registry/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) diff --git a/registry/storage/catalog_test.go b/registry/storage/catalog_test.go index ed96f50c..eb062c5b 100644 --- a/registry/storage/catalog_test.go +++ b/registry/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{ diff --git a/registry/storage/manifeststore_test.go b/registry/storage/manifeststore_test.go index 4ad74820..7665c5c8 100644 --- a/registry/storage/manifeststore_test.go +++ b/registry/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) diff --git a/registry/storage/registry.go b/registry/storage/registry.go index da95054e..0b38ea9b 100644 --- a/registry/storage/registry.go +++ b/registry/storage/registry.go @@ -12,28 +12,65 @@ import ( // package. All instances should descend from this object. type registry struct { blobStore *blobStore - blobServer distribution.BlobServer - statter distribution.BlobStatter // global statter service. + blobServer *blobServer + statter *blobStatter // global statter service. blobDescriptorCacheProvider cache.BlobDescriptorCacheProvider deleteEnabled bool resumableDigestEnabled bool } -// NewRegistryWithDriver creates a new registry instance from the provided -// driver. The resulting registry may be shared by multiple goroutines but is -// 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, - } +// RegistryOption is the type used for functional options for NewRegistry. +type RegistryOption func(*registry) error - if blobDescriptorCacheProvider != nil { - statter = cache.NewCachedBlobStatter(blobDescriptorCacheProvider, statter) +// 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, 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 ®istry{ + registry := ®istry{ blobStore: bs, blobServer: &blobServer{ - driver: driver, - statter: statter, - pathFn: bs.path, - redirect: redirect, + driver: driver, + statter: statter, + pathFn: bs.path, }, - blobDescriptorCacheProvider: blobDescriptorCacheProvider, - deleteEnabled: deleteEnabled, - resumableDigestEnabled: !isCache, + statter: statter, + resumableDigestEnabled: true, } + + 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