Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: event-based replication deletion not work when policy with label #21215

Merged
merged 1 commit into from
Nov 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/controller/artifact/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
})
}

Expand Down
6 changes: 6 additions & 0 deletions src/controller/artifact/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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)

Expand All @@ -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)

Expand All @@ -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)

Expand All @@ -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)

Expand All @@ -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)

Expand Down
10 changes: 10 additions & 0 deletions src/controller/artifact/model.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down
56 changes: 56 additions & 0 deletions src/controller/artifact/model_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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])
}
}
})
}
}
45 changes: 4 additions & 41 deletions src/controller/event/handler/replication/replication.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand All @@ -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,
}},
},
},
Expand All @@ -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{
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
}},
},
},
Expand All @@ -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,
Expand All @@ -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,
Expand Down
4 changes: 4 additions & 0 deletions src/controller/event/metadata/artifact.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ type PushArtifactEventMetadata struct {
Ctx context.Context
Artifact *artifact.Artifact
Tag string
Labels []string
}

// Resolve to the event from the metadata
Expand All @@ -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 != "" {
Expand Down Expand Up @@ -86,6 +88,7 @@ type DeleteArtifactEventMetadata struct {
Ctx context.Context
Artifact *artifact.Artifact
Tags []string
Labels []string
}

// Resolve to the event from the metadata
Expand All @@ -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(),
},
}
Expand Down
4 changes: 4 additions & 0 deletions src/controller/event/metadata/tag.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
type CreateTagEventMetadata struct {
Ctx context.Context
Tag string
Labels []string
AttachedArtifact *artifact.Artifact
}

Expand All @@ -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(),
}
Expand All @@ -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
}

Expand All @@ -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(),
}
Expand Down
3 changes: 3 additions & 0 deletions src/controller/event/topic.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -238,6 +239,7 @@ type CreateTagEvent struct {
EventType string
Repository string
Tag string
Labels []string
AttachedArtifact *artifact.Artifact
Operator string
OccurAt time.Time
Expand Down Expand Up @@ -266,6 +268,7 @@ type DeleteTagEvent struct {
EventType string
Repository string
Tag string
Labels []string
AttachedArtifact *artifact.Artifact
Operator string
OccurAt time.Time
Expand Down
Loading
Loading