From 05233b0711f71dcf00171c0c343cb20253104fa3 Mon Sep 17 00:00:00 2001 From: Chlins Zhang Date: Tue, 26 Nov 2024 10:29:34 +0800 Subject: [PATCH] fix: event-based replication deletion not work when policy with label (#21215) fix: event-based replication deletion not work when policy with label filter Fix event-based replication deletion on remote registry not triggered when the replication policy configured the label filter. Signed-off-by: chlins --- src/controller/artifact/controller.go | 10 +++- src/controller/artifact/controller_test.go | 6 ++ src/controller/artifact/model.go | 10 ++++ src/controller/artifact/model_test.go | 56 +++++++++++++++++++ .../event/handler/replication/replication.go | 45 ++------------- src/controller/event/metadata/artifact.go | 4 ++ src/controller/event/metadata/tag.go | 4 ++ src/controller/event/topic.go | 3 + src/server/v2.0/handler/artifact.go | 8 ++- 9 files changed, 101 insertions(+), 45 deletions(-) diff --git a/src/controller/artifact/controller.go b/src/controller/artifact/controller.go index f7c7cfd26..943b9313f 100644 --- a/src/controller/artifact/controller.go +++ b/src/controller/artifact/controller.go @@ -326,7 +326,7 @@ func (c *controller) Delete(ctx context.Context, id int64) error { // the error handling logic for the root parent artifact and others is different // "isAccessory" is used to specify whether the artifact is an accessory. func (c *controller) deleteDeeply(ctx context.Context, id int64, isRoot, isAccessory bool) error { - art, err := c.Get(ctx, id, &Option{WithTag: true, WithAccessory: true}) + art, err := c.Get(ctx, id, &Option{WithTag: true, WithAccessory: true, WithLabel: true}) if err != nil { // return nil if the nonexistent artifact isn't the root parent if !isRoot && errors.IsErr(err, errors.NotFoundCode) { @@ -450,14 +450,20 @@ func (c *controller) deleteDeeply(ctx context.Context, id int64, isRoot, isAcces // only fire event for the root parent artifact if isRoot { - var tags []string + var tags, labels []string for _, tag := range art.Tags { tags = append(tags, tag.Name) } + + for _, label := range art.Labels { + labels = append(labels, label.Name) + } + notification.AddEvent(ctx, &metadata.DeleteArtifactEventMetadata{ Ctx: ctx, Artifact: &art.Artifact, Tags: tags, + Labels: labels, }) } diff --git a/src/controller/artifact/controller_test.go b/src/controller/artifact/controller_test.go index d4b6743db..4144e3f02 100644 --- a/src/controller/artifact/controller_test.go +++ b/src/controller/artifact/controller_test.go @@ -487,6 +487,7 @@ func (c *controllerTestSuite) TestDeleteDeeply() { // root artifact and doesn't exist c.artMgr.On("Get", mock.Anything, mock.Anything).Return(nil, errors.NotFoundError(nil)) c.accMgr.On("List", mock.Anything, mock.Anything).Return([]accessorymodel.Accessory{}, nil) + c.labelMgr.On("ListByArtifact", mock.Anything, mock.Anything).Return([]*model.Label{}, nil) err := c.ctl.deleteDeeply(orm.NewContext(nil, &ormtesting.FakeOrmer{}), 1, true, false) c.Require().NotNil(err) c.Assert().True(errors.IsErr(err, errors.NotFoundCode)) @@ -497,6 +498,7 @@ func (c *controllerTestSuite) TestDeleteDeeply() { // child artifact and doesn't exist c.artMgr.On("Get", mock.Anything, mock.Anything).Return(nil, errors.NotFoundError(nil)) c.accMgr.On("List", mock.Anything, mock.Anything).Return([]accessorymodel.Accessory{}, nil) + c.labelMgr.On("ListByArtifact", mock.Anything, mock.Anything).Return([]*model.Label{}, nil) err = c.ctl.deleteDeeply(orm.NewContext(nil, &ormtesting.FakeOrmer{}), 1, false, false) c.Require().Nil(err) @@ -516,6 +518,7 @@ func (c *controllerTestSuite) TestDeleteDeeply() { c.repoMgr.On("Get", mock.Anything, mock.Anything).Return(&repomodel.RepoRecord{}, nil) c.artrashMgr.On("Create", mock.Anything, mock.Anything).Return(int64(0), nil) c.accMgr.On("List", mock.Anything, mock.Anything).Return([]accessorymodel.Accessory{}, nil) + c.labelMgr.On("ListByArtifact", mock.Anything, mock.Anything).Return([]*model.Label{}, nil) err = c.ctl.deleteDeeply(orm.NewContext(nil, &ormtesting.FakeOrmer{}), 1, false, false) c.Require().Nil(err) @@ -532,6 +535,7 @@ func (c *controllerTestSuite) TestDeleteDeeply() { }, }, nil) c.accMgr.On("List", mock.Anything, mock.Anything).Return([]accessorymodel.Accessory{}, nil) + c.labelMgr.On("ListByArtifact", mock.Anything, mock.Anything).Return([]*model.Label{}, nil) err = c.ctl.deleteDeeply(orm.NewContext(nil, &ormtesting.FakeOrmer{}), 1, true, false) c.Require().NotNil(err) @@ -548,6 +552,7 @@ func (c *controllerTestSuite) TestDeleteDeeply() { }, }, nil) c.accMgr.On("List", mock.Anything, mock.Anything).Return([]accessorymodel.Accessory{}, nil) + c.labelMgr.On("ListByArtifact", mock.Anything, mock.Anything).Return([]*model.Label{}, nil) err = c.ctl.deleteDeeply(nil, 1, false, false) c.Require().Nil(err) @@ -573,6 +578,7 @@ func (c *controllerTestSuite) TestDeleteDeeply() { c.blobMgr.On("CleanupAssociationsForProject", mock.Anything, mock.Anything, mock.Anything).Return(nil) c.repoMgr.On("Get", mock.Anything, mock.Anything).Return(&repomodel.RepoRecord{}, nil) c.artrashMgr.On("Create", mock.Anything, mock.Anything).Return(int64(0), nil) + c.labelMgr.On("ListByArtifact", mock.Anything, mock.Anything).Return([]*model.Label{}, nil) err = c.ctl.deleteDeeply(orm.NewContext(nil, &ormtesting.FakeOrmer{}), 1, true, true) c.Require().Nil(err) diff --git a/src/controller/artifact/model.go b/src/controller/artifact/model.go index d24369c98..7305b5f3f 100644 --- a/src/controller/artifact/model.go +++ b/src/controller/artifact/model.go @@ -94,6 +94,16 @@ func (artifact *Artifact) SetSBOMAdditionLink(sbomDgst string, version string) { artifact.AdditionLinks[addition] = &AdditionLink{HREF: href, Absolute: false} } +// AbstractLabelNames abstracts the label names from the artifact. +func (artifact *Artifact) AbstractLabelNames() []string { + var names []string + for _, label := range artifact.Labels { + names = append(names, label.Name) + } + + return names +} + // AdditionLink is a link via that the addition can be fetched type AdditionLink struct { HREF string `json:"href"` diff --git a/src/controller/artifact/model_test.go b/src/controller/artifact/model_test.go index a5ef1f87b..06aade823 100644 --- a/src/controller/artifact/model_test.go +++ b/src/controller/artifact/model_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/goharbor/harbor/src/pkg/accessory/model/cosign" + "github.com/goharbor/harbor/src/pkg/label/model" ) func TestUnmarshalJSONWithACC(t *testing.T) { @@ -104,3 +105,58 @@ func TestUnmarshalJSONWithPartial(t *testing.T) { assert.Equal(t, "", artifact.Type) assert.Equal(t, "application/vnd.docker.container.image.v1+json", artifact.MediaType) } + +func TestAbstractLabelNames(t *testing.T) { + tests := []struct { + name string + artifact Artifact + want []string + }{ + { + name: "Nil labels", + artifact: Artifact{ + Labels: nil, + }, + want: []string{}, + }, + { + name: "Single label", + artifact: Artifact{ + Labels: []*model.Label{ + {Name: "label1"}, + }, + }, + want: []string{"label1"}, + }, + { + name: "Multiple labels", + artifact: Artifact{ + Labels: []*model.Label{ + {Name: "label1"}, + {Name: "label2"}, + {Name: "label3"}, + }, + }, + want: []string{"label1", "label2", "label3"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.artifact.AbstractLabelNames() + + // Check if lengths match + if len(got) != len(tt.want) { + t.Errorf("AbstractLabelNames() got length = %v, want length = %v", len(got), len(tt.want)) + return + } + + // Check if elements match + for i := range got { + if got[i] != tt.want[i] { + t.Errorf("AbstractLabelNames() got[%d] = %v, want[%d] = %v", i, got[i], i, tt.want[i]) + } + } + }) + } +} diff --git a/src/controller/event/handler/replication/replication.go b/src/controller/event/handler/replication/replication.go index 715baf316..cd09c7058 100644 --- a/src/controller/event/handler/replication/replication.go +++ b/src/controller/event/handler/replication/replication.go @@ -23,8 +23,6 @@ import ( "github.com/goharbor/harbor/src/controller/project" "github.com/goharbor/harbor/src/lib/log" "github.com/goharbor/harbor/src/lib/orm" - "github.com/goharbor/harbor/src/pkg/label" - labmodel "github.com/goharbor/harbor/src/pkg/label/model" "github.com/goharbor/harbor/src/pkg/reg/model" ) @@ -63,16 +61,6 @@ func (r *Handler) IsStateful() bool { return false } -// abstractLabelNames returns labels name. -func abstractLabelNames(labels []*labmodel.Label) []string { - res := make([]string, 0, len(labels)) - for _, lab := range labels { - res = append(res, lab.Name) - } - - return res -} - func (r *Handler) handlePushArtifact(ctx context.Context, event *event.PushArtifactEvent) error { art := event.Artifact public := false @@ -82,12 +70,6 @@ func (r *Handler) handlePushArtifact(ctx context.Context, event *event.PushArtif return err } public = prj.IsPublic() - // list attached labels - labels, err := label.Mgr.ListByArtifact(ctx, art.ID) - if err != nil { - log.Errorf("failed to list artifact %d labels, error: %v", art.ID, err) - return err - } e := &repevent.Event{ Type: repevent.EventTypeArtifactPush, @@ -105,7 +87,7 @@ func (r *Handler) handlePushArtifact(ctx context.Context, event *event.PushArtif Type: art.Type, Digest: art.Digest, Tags: event.Tags, - Labels: abstractLabelNames(labels), + Labels: event.Labels, }}, }, }, @@ -116,13 +98,6 @@ func (r *Handler) handlePushArtifact(ctx context.Context, event *event.PushArtif func (r *Handler) handleDeleteArtifact(ctx context.Context, event *event.DeleteArtifactEvent) error { art := event.Artifact - // list attached labels - labels, err := label.Mgr.ListByArtifact(ctx, art.ID) - if err != nil { - log.Errorf("failed to list artifact %d labels, error: %v", art.ID, err) - return err - } - e := &repevent.Event{ Type: repevent.EventTypeArtifactDelete, Resource: &model.Resource{ @@ -136,7 +111,7 @@ func (r *Handler) handleDeleteArtifact(ctx context.Context, event *event.DeleteA Type: art.Type, Digest: art.Digest, Tags: event.Tags, - Labels: abstractLabelNames(labels), + Labels: event.Labels, }}, }, Deleted: true, @@ -155,12 +130,6 @@ func (r *Handler) handleCreateTag(ctx context.Context, event *event.CreateTagEve return err } public = prj.IsPublic() - // list attached labels - labels, err := label.Mgr.ListByArtifact(ctx, art.ID) - if err != nil { - log.Errorf("failed to list artifact %d labels, error: %v", art.ID, err) - return err - } e := &repevent.Event{ Type: repevent.EventTypeArtifactPush, @@ -178,7 +147,7 @@ func (r *Handler) handleCreateTag(ctx context.Context, event *event.CreateTagEve Type: art.Type, Digest: art.Digest, Tags: []string{event.Tag}, - Labels: abstractLabelNames(labels), + Labels: event.Labels, }}, }, }, @@ -189,12 +158,6 @@ func (r *Handler) handleCreateTag(ctx context.Context, event *event.CreateTagEve func (r *Handler) handleDeleteTag(ctx context.Context, event *event.DeleteTagEvent) error { art := event.AttachedArtifact - // list attached labels - labels, err := label.Mgr.ListByArtifact(ctx, art.ID) - if err != nil { - log.Errorf("failed to list artifact %d labels, error: %v", art.ID, err) - return err - } e := &repevent.Event{ Type: repevent.EventTypeTagDelete, @@ -209,7 +172,7 @@ func (r *Handler) handleDeleteTag(ctx context.Context, event *event.DeleteTagEve Type: art.Type, Digest: art.Digest, Tags: []string{event.Tag}, - Labels: abstractLabelNames(labels), + Labels: event.Labels, }}, }, Deleted: true, diff --git a/src/controller/event/metadata/artifact.go b/src/controller/event/metadata/artifact.go index f543b14d5..cf515a87a 100644 --- a/src/controller/event/metadata/artifact.go +++ b/src/controller/event/metadata/artifact.go @@ -29,6 +29,7 @@ type PushArtifactEventMetadata struct { Ctx context.Context Artifact *artifact.Artifact Tag string + Labels []string } // Resolve to the event from the metadata @@ -37,6 +38,7 @@ func (p *PushArtifactEventMetadata) Resolve(event *event.Event) error { EventType: event2.TopicPushArtifact, Repository: p.Artifact.RepositoryName, Artifact: p.Artifact, + Labels: p.Labels, OccurAt: time.Now(), } if p.Tag != "" { @@ -86,6 +88,7 @@ type DeleteArtifactEventMetadata struct { Ctx context.Context Artifact *artifact.Artifact Tags []string + Labels []string } // Resolve to the event from the metadata @@ -96,6 +99,7 @@ func (d *DeleteArtifactEventMetadata) Resolve(event *event.Event) error { Repository: d.Artifact.RepositoryName, Artifact: d.Artifact, Tags: d.Tags, + Labels: d.Labels, OccurAt: time.Now(), }, } diff --git a/src/controller/event/metadata/tag.go b/src/controller/event/metadata/tag.go index 8462ffc0c..9411d7599 100644 --- a/src/controller/event/metadata/tag.go +++ b/src/controller/event/metadata/tag.go @@ -28,6 +28,7 @@ import ( type CreateTagEventMetadata struct { Ctx context.Context Tag string + Labels []string AttachedArtifact *artifact.Artifact } @@ -37,6 +38,7 @@ func (c *CreateTagEventMetadata) Resolve(event *event.Event) error { EventType: event2.TopicCreateTag, Repository: c.AttachedArtifact.RepositoryName, Tag: c.Tag, + Labels: c.Labels, AttachedArtifact: c.AttachedArtifact, OccurAt: time.Now(), } @@ -53,6 +55,7 @@ func (c *CreateTagEventMetadata) Resolve(event *event.Event) error { type DeleteTagEventMetadata struct { Ctx context.Context Tag string + Labels []string AttachedArtifact *artifact.Artifact } @@ -62,6 +65,7 @@ func (d *DeleteTagEventMetadata) Resolve(event *event.Event) error { EventType: event2.TopicDeleteTag, Repository: d.AttachedArtifact.RepositoryName, Tag: d.Tag, + Labels: d.Labels, AttachedArtifact: d.AttachedArtifact, OccurAt: time.Now(), } diff --git a/src/controller/event/topic.go b/src/controller/event/topic.go index 60dd8107a..f8f64b420 100644 --- a/src/controller/event/topic.go +++ b/src/controller/event/topic.go @@ -137,6 +137,7 @@ type ArtifactEvent struct { Repository string Artifact *artifact.Artifact Tags []string // when the artifact is pushed by digest, the tag here will be null + Labels []string Operator string OccurAt time.Time } @@ -238,6 +239,7 @@ type CreateTagEvent struct { EventType string Repository string Tag string + Labels []string AttachedArtifact *artifact.Artifact Operator string OccurAt time.Time @@ -266,6 +268,7 @@ type DeleteTagEvent struct { EventType string Repository string Tag string + Labels []string AttachedArtifact *artifact.Artifact Operator string OccurAt time.Time diff --git a/src/server/v2.0/handler/artifact.go b/src/server/v2.0/handler/artifact.go index c5ee83114..e54e8f117 100644 --- a/src/server/v2.0/handler/artifact.go +++ b/src/server/v2.0/handler/artifact.go @@ -238,7 +238,8 @@ func (a *artifactAPI) CreateTag(ctx context.Context, params operation.CreateTagP art, err := a.artCtl.GetByReference(ctx, fmt.Sprintf("%s/%s", params.ProjectName, params.RepositoryName), params.Reference, &artifact.Option{ - WithTag: true, + WithTag: true, + WithLabel: true, }) if err != nil { return a.SendError(ctx, err) @@ -256,6 +257,7 @@ func (a *artifactAPI) CreateTag(ctx context.Context, params operation.CreateTagP notification.AddEvent(ctx, &metadata.CreateTagEventMetadata{ Ctx: ctx, Tag: tag.Name, + Labels: art.AbstractLabelNames(), AttachedArtifact: &art.Artifact, }) @@ -281,7 +283,8 @@ func (a *artifactAPI) DeleteTag(ctx context.Context, params operation.DeleteTagP } artifact, err := a.artCtl.GetByReference(ctx, fmt.Sprintf("%s/%s", params.ProjectName, params.RepositoryName), params.Reference, &artifact.Option{ - WithTag: true, + WithTag: true, + WithLabel: true, }) if err != nil { return a.SendError(ctx, err) @@ -307,6 +310,7 @@ func (a *artifactAPI) DeleteTag(ctx context.Context, params operation.DeleteTagP notification.AddEvent(ctx, &metadata.DeleteTagEventMetadata{ Ctx: ctx, Tag: params.TagName, + Labels: artifact.AbstractLabelNames(), AttachedArtifact: &artifact.Artifact, })