From afe2bdd1c5194ac85a59adf1fdd14c9da054356e Mon Sep 17 00:00:00 2001 From: Richard Scothern Date: Fri, 18 Mar 2016 15:30:47 -0700 Subject: [PATCH] Propogate tag as a functional argument into the notification system to attach tags to manifest push and pull event notifications. Signed-off-by: Richard Scothern --- notifications/bridge.go | 30 +++++++++++++++++++++++---- notifications/bridge_test.go | 33 ++++++++++++++++++++++++++++++ notifications/event.go | 3 +++ notifications/listener.go | 8 ++++---- notifications/listener_test.go | 6 +++--- registry.go | 12 +++++++++++ registry/client/repository.go | 25 +++++----------------- registry/client/repository_test.go | 8 ++++---- registry/handlers/images.go | 12 +++++++++-- 9 files changed, 100 insertions(+), 37 deletions(-) diff --git a/notifications/bridge.go b/notifications/bridge.go index 584ba976..502288a4 100644 --- a/notifications/bridge.go +++ b/notifications/bridge.go @@ -53,12 +53,34 @@ func NewRequestRecord(id string, r *http.Request) RequestRecord { } } -func (b *bridge) ManifestPushed(repo reference.Named, sm distribution.Manifest) error { - return b.createManifestEventAndWrite(EventActionPush, repo, sm) +func (b *bridge) ManifestPushed(repo reference.Named, sm distribution.Manifest, options ...distribution.ManifestServiceOption) error { + manifestEvent, err := b.createManifestEvent(EventActionPush, repo, sm) + if err != nil { + return err + } + + for _, option := range options { + if opt, ok := option.(distribution.WithTagOption); ok { + manifestEvent.Target.Tag = opt.Tag + break + } + } + return b.sink.Write(*manifestEvent) } -func (b *bridge) ManifestPulled(repo reference.Named, sm distribution.Manifest) error { - return b.createManifestEventAndWrite(EventActionPull, repo, sm) +func (b *bridge) ManifestPulled(repo reference.Named, sm distribution.Manifest, options ...distribution.ManifestServiceOption) error { + manifestEvent, err := b.createManifestEvent(EventActionPull, repo, sm) + if err != nil { + return err + } + + for _, option := range options { + if opt, ok := option.(distribution.WithTagOption); ok { + manifestEvent.Target.Tag = opt.Tag + break + } + } + return b.sink.Write(*manifestEvent) } func (b *bridge) ManifestDeleted(repo reference.Named, dgst digest.Digest) error { diff --git a/notifications/bridge_test.go b/notifications/bridge_test.go index 6f361449..dc42621a 100644 --- a/notifications/bridge_test.go +++ b/notifications/bridge_test.go @@ -3,6 +3,7 @@ package notifications import ( "testing" + "github.com/docker/distribution" "github.com/docker/distribution/digest" "github.com/docker/distribution/manifest/schema1" "github.com/docker/distribution/reference" @@ -61,6 +62,38 @@ func TestEventBridgeManifestPushed(t *testing.T) { } } +func TestEventBridgeManifestPushedWithTag(t *testing.T) { + l := createTestEnv(t, testSinkFn(func(events ...Event) error { + checkCommonManifest(t, EventActionPush, events...) + if events[0].Target.Tag != "latest" { + t.Fatalf("missing or unexpected tag: %#v", events[0].Target) + } + + return nil + })) + + repoRef, _ := reference.ParseNamed(repo) + if err := l.ManifestPushed(repoRef, sm, distribution.WithTag(m.Tag)); err != nil { + t.Fatalf("unexpected error notifying manifest pull: %v", err) + } +} + +func TestEventBridgeManifestPulledWithTag(t *testing.T) { + l := createTestEnv(t, testSinkFn(func(events ...Event) error { + checkCommonManifest(t, EventActionPull, events...) + if events[0].Target.Tag != "latest" { + t.Fatalf("missing or unexpected tag: %#v", events[0].Target) + } + + return nil + })) + + repoRef, _ := reference.ParseNamed(repo) + if err := l.ManifestPulled(repoRef, sm, distribution.WithTag(m.Tag)); err != nil { + t.Fatalf("unexpected error notifying manifest pull: %v", err) + } +} + func TestEventBridgeManifestDeleted(t *testing.T) { l := createTestEnv(t, testSinkFn(func(events ...Event) error { checkDeleted(t, EventActionDelete, events...) diff --git a/notifications/event.go b/notifications/event.go index 19d6a776..b59a72be 100644 --- a/notifications/event.go +++ b/notifications/event.go @@ -68,6 +68,9 @@ type Event struct { // URL provides a direct link to the content. URL string `json:"url,omitempty"` + + // Tag provides the tag + Tag string `json:"tag,omitempty"` } `json:"target,omitempty"` // Request covers the request that generated the event. diff --git a/notifications/listener.go b/notifications/listener.go index 0817d463..fa4b722d 100644 --- a/notifications/listener.go +++ b/notifications/listener.go @@ -12,8 +12,8 @@ import ( // ManifestListener describes a set of methods for listening to events related to manifests. type ManifestListener interface { - ManifestPushed(repo reference.Named, sm distribution.Manifest) error - ManifestPulled(repo reference.Named, sm distribution.Manifest) error + ManifestPushed(repo reference.Named, sm distribution.Manifest, options ...distribution.ManifestServiceOption) error + ManifestPulled(repo reference.Named, sm distribution.Manifest, options ...distribution.ManifestServiceOption) error ManifestDeleted(repo reference.Named, dgst digest.Digest) error } @@ -81,7 +81,7 @@ func (msl *manifestServiceListener) Delete(ctx context.Context, dgst digest.Dige func (msl *manifestServiceListener) Get(ctx context.Context, dgst digest.Digest, options ...distribution.ManifestServiceOption) (distribution.Manifest, error) { sm, err := msl.ManifestService.Get(ctx, dgst) if err == nil { - if err := msl.parent.listener.ManifestPulled(msl.parent.Repository.Named(), sm); err != nil { + if err := msl.parent.listener.ManifestPulled(msl.parent.Repository.Named(), sm, options...); err != nil { logrus.Errorf("error dispatching manifest pull to listener: %v", err) } } @@ -93,7 +93,7 @@ func (msl *manifestServiceListener) Put(ctx context.Context, sm distribution.Man dgst, err := msl.ManifestService.Put(ctx, sm, options...) if err == nil { - if err := msl.parent.listener.ManifestPushed(msl.parent.Repository.Named(), sm); err != nil { + if err := msl.parent.listener.ManifestPushed(msl.parent.Repository.Named(), sm, options...); err != nil { logrus.Errorf("error dispatching manifest push to listener: %v", err) } } diff --git a/notifications/listener_test.go b/notifications/listener_test.go index b979626b..d16a4560 100644 --- a/notifications/listener_test.go +++ b/notifications/listener_test.go @@ -44,7 +44,7 @@ func TestListener(t *testing.T) { "manifest:delete": 1, "layer:push": 2, "layer:pull": 2, - "layer:delete": 2, // deletes not supported for now + "layer:delete": 2, } if !reflect.DeepEqual(tl.ops, expectedOps) { @@ -57,13 +57,13 @@ type testListener struct { ops map[string]int } -func (tl *testListener) ManifestPushed(repo reference.Named, m distribution.Manifest) error { +func (tl *testListener) ManifestPushed(repo reference.Named, m distribution.Manifest, options ...distribution.ManifestServiceOption) error { tl.ops["manifest:push"]++ return nil } -func (tl *testListener) ManifestPulled(repo reference.Named, m distribution.Manifest) error { +func (tl *testListener) ManifestPulled(repo reference.Named, m distribution.Manifest, options ...distribution.ManifestServiceOption) error { tl.ops["manifest:pull"]++ return nil } diff --git a/registry.go b/registry.go index fbe605e0..463eeb89 100644 --- a/registry.go +++ b/registry.go @@ -58,6 +58,18 @@ type ManifestServiceOption interface { Apply(ManifestService) error } +// WithTag allows a tag to be passed into Put +func WithTag(tag string) ManifestServiceOption { + return WithTagOption{tag} +} + +type WithTagOption struct{ Tag string } + +func (o WithTagOption) Apply(m ManifestService) error { + // no implementation + return nil +} + // Repository is a named collection of manifests and layers. type Repository interface { // Named returns the name of the repository. diff --git a/registry/client/repository.go b/registry/client/repository.go index 936a3f1b..643e23a0 100644 --- a/registry/client/repository.go +++ b/registry/client/repository.go @@ -402,9 +402,9 @@ func (ms *manifests) Get(ctx context.Context, dgst digest.Digest, options ...dis ) for _, option := range options { - if opt, ok := option.(withTagOption); ok { - digestOrTag = opt.tag - ref, err = reference.WithTag(ms.name, opt.tag) + if opt, ok := option.(distribution.WithTagOption); ok { + digestOrTag = opt.Tag + ref, err = reference.WithTag(ms.name, opt.Tag) if err != nil { return nil, err } @@ -465,21 +465,6 @@ func (ms *manifests) Get(ctx context.Context, dgst digest.Digest, options ...dis return nil, HandleErrorResponse(resp) } -// WithTag allows a tag to be passed into Put which enables the client -// to build a correct URL. -func WithTag(tag string) distribution.ManifestServiceOption { - return withTagOption{tag} -} - -type withTagOption struct{ tag string } - -func (o withTagOption) Apply(m distribution.ManifestService) error { - if _, ok := m.(*manifests); ok { - return nil - } - return fmt.Errorf("withTagOption is a client-only option") -} - // Put puts a manifest. A tag can be specified using an options parameter which uses some shared state to hold the // tag name in order to build the correct upload URL. func (ms *manifests) Put(ctx context.Context, m distribution.Manifest, options ...distribution.ManifestServiceOption) (digest.Digest, error) { @@ -487,9 +472,9 @@ func (ms *manifests) Put(ctx context.Context, m distribution.Manifest, options . var tagged bool for _, option := range options { - if opt, ok := option.(withTagOption); ok { + if opt, ok := option.(distribution.WithTagOption); ok { var err error - ref, err = reference.WithTag(ref, opt.tag) + ref, err = reference.WithTag(ref, opt.Tag) if err != nil { return "", err } diff --git a/registry/client/repository_test.go b/registry/client/repository_test.go index df26b631..2faeb276 100644 --- a/registry/client/repository_test.go +++ b/registry/client/repository_test.go @@ -710,7 +710,7 @@ func TestV1ManifestFetch(t *testing.T) { t.Fatal(err) } - manifest, err = ms.Get(ctx, dgst, WithTag("latest")) + manifest, err = ms.Get(ctx, dgst, distribution.WithTag("latest")) if err != nil { t.Fatal(err) } @@ -723,7 +723,7 @@ func TestV1ManifestFetch(t *testing.T) { t.Fatal(err) } - manifest, err = ms.Get(ctx, dgst, WithTag("badcontenttype")) + manifest, err = ms.Get(ctx, dgst, distribution.WithTag("badcontenttype")) if err != nil { t.Fatal(err) } @@ -761,7 +761,7 @@ func TestManifestFetchWithEtag(t *testing.T) { if !ok { panic("wrong type for client manifest service") } - _, err = clientManifestService.Get(ctx, d1, WithTag("latest"), AddEtagToTag("latest", d1.String())) + _, err = clientManifestService.Get(ctx, d1, distribution.WithTag("latest"), AddEtagToTag("latest", d1.String())) if err != distribution.ErrManifestNotModified { t.Fatal(err) } @@ -861,7 +861,7 @@ func TestManifestPut(t *testing.T) { t.Fatal(err) } - if _, err := ms.Put(ctx, m1, WithTag(m1.Tag)); err != nil { + if _, err := ms.Put(ctx, m1, distribution.WithTag(m1.Tag)); err != nil { t.Fatal(err) } diff --git a/registry/handlers/images.go b/registry/handlers/images.go index 8ef7197a..5f2d8855 100644 --- a/registry/handlers/images.go +++ b/registry/handlers/images.go @@ -86,7 +86,11 @@ func (imh *imageManifestHandler) GetImageManifest(w http.ResponseWriter, r *http return } - manifest, err = manifests.Get(imh, imh.Digest) + var options []distribution.ManifestServiceOption + if imh.Tag != "" { + options = append(options, distribution.WithTag(imh.Tag)) + } + manifest, err = manifests.Get(imh, imh.Digest, options...) if err != nil { imh.Errors = append(imh.Errors, v2.ErrorCodeManifestUnknown.WithDetail(err)) return @@ -245,7 +249,11 @@ func (imh *imageManifestHandler) PutImageManifest(w http.ResponseWriter, r *http return } - _, err = manifests.Put(imh, manifest) + var options []distribution.ManifestServiceOption + if imh.Tag != "" { + options = append(options, distribution.WithTag(imh.Tag)) + } + _, err = manifests.Put(imh, manifest, options...) if err != nil { // TODO(stevvooe): These error handling switches really need to be // handled by an app global mapper.