From 9cb3e52f082e87e8db66b47b2a5cb20ebb53afde Mon Sep 17 00:00:00 2001 From: wang yan Date: Mon, 18 Sep 2023 14:14:51 +0800 Subject: [PATCH] support accessor in either order In certain cases, the OCI client may push the subject artifact and accessory in either order. Therefore, it is necessary to handle situations where the client pushes the accessory ahead of the subject artifact. Signed-off-by: wang yan --- src/pkg/accessory/dao/dao.go | 19 +++- src/pkg/accessory/dao/dao_test.go | 13 +++ src/pkg/accessory/manager.go | 10 +++ src/pkg/accessory/manager_test.go | 10 +++ src/server/middleware/subject/subject.go | 30 +++++++ src/server/middleware/subject/subject_test.go | 89 +++++++++++++++---- src/testing/pkg/accessory/dao/dao.go | 14 +++ src/testing/pkg/accessory/manager.go | 14 +++ 8 files changed, 182 insertions(+), 17 deletions(-) diff --git a/src/pkg/accessory/dao/dao.go b/src/pkg/accessory/dao/dao.go index 651871f8a551..3203025d7b3b 100644 --- a/src/pkg/accessory/dao/dao.go +++ b/src/pkg/accessory/dao/dao.go @@ -16,7 +16,7 @@ package dao import ( "context" - + "github.com/goharbor/harbor/src/lib/errors" "github.com/goharbor/harbor/src/lib/orm" "github.com/goharbor/harbor/src/lib/q" @@ -32,6 +32,8 @@ type DAO interface { Get(ctx context.Context, id int64) (accessory *Accessory, err error) // Create the accessory Create(ctx context.Context, accessory *Accessory) (id int64, err error) + // Update the accessory + Update(ctx context.Context, accessory *Accessory) error // Delete the accessory specified by ID Delete(ctx context.Context, id int64) (err error) // DeleteAccessories deletes accessories by query @@ -103,6 +105,21 @@ func (d *dao) Create(ctx context.Context, acc *Accessory) (int64, error) { return id, err } +func (d *dao) Update(ctx context.Context, acc *Accessory) error { + ormer, err := orm.FromContext(ctx) + if err != nil { + return err + } + n, err := ormer.Update(acc, "SubjectArtifactID") + if n == 0 { + if e := orm.AsConflictError(err, "accessory %s already exists", acc.Digest); e != nil { + err = e + } + return err + } + return err +} + func (d *dao) Delete(ctx context.Context, id int64) error { ormer, err := orm.FromContext(ctx) if err != nil { diff --git a/src/pkg/accessory/dao/dao_test.go b/src/pkg/accessory/dao/dao_test.go index 4a23a7b30460..06e9ae2cccf9 100644 --- a/src/pkg/accessory/dao/dao_test.go +++ b/src/pkg/accessory/dao/dao_test.go @@ -169,6 +169,19 @@ func (d *daoTestSuite) TestCreate() { d.True(errors.IsErr(err, errors.ConflictCode)) } +func (d *daoTestSuite) TestUpdate() { + acc := &Accessory{ + ID: d.accID, + SubjectArtifactID: 333, + } + err := d.dao.Update(d.ctx, acc) + d.Require().Nil(err) + + accAfter, err := d.dao.Get(d.ctx, d.accID) + d.Require().Nil(err) + d.Require().Equal(int64(333), accAfter.SubjectArtifactID) +} + func (d *daoTestSuite) TestDelete() { // happy pass is covered in TearDown diff --git a/src/pkg/accessory/manager.go b/src/pkg/accessory/manager.go index 6c1d68d276da..0c02908d5719 100644 --- a/src/pkg/accessory/manager.go +++ b/src/pkg/accessory/manager.go @@ -48,6 +48,8 @@ type Manager interface { List(ctx context.Context, query *q.Query) (accs []model.Accessory, err error) // Create the accessory and returns the ID Create(ctx context.Context, accessory model.AccessoryData) (id int64, err error) + // Update the accessory + Update(ctx context.Context, accessory model.AccessoryData) error // Delete the accessory specified by ID Delete(ctx context.Context, id int64) (err error) // DeleteAccessories deletes accessories according to the query @@ -150,6 +152,14 @@ func (m *manager) Create(ctx context.Context, accessory model.AccessoryData) (in return m.dao.Create(ctx, acc) } +func (m *manager) Update(ctx context.Context, accessory model.AccessoryData) error { + acc := &dao.Accessory{ + ID: accessory.ID, + SubjectArtifactID: accessory.SubArtifactID, + } + return m.dao.Update(ctx, acc) +} + func (m *manager) Delete(ctx context.Context, id int64) error { return m.dao.Delete(ctx, id) } diff --git a/src/pkg/accessory/manager_test.go b/src/pkg/accessory/manager_test.go index 7e436ce46f46..a06bb5ccd012 100644 --- a/src/pkg/accessory/manager_test.go +++ b/src/pkg/accessory/manager_test.go @@ -87,6 +87,16 @@ func (m *managerTestSuite) TestCreate() { m.dao.AssertExpectations(m.T()) } +func (m *managerTestSuite) TestUpdate() { + mock.OnAnything(m.dao, "Update").Return(nil) + err := m.mgr.Update(nil, model.AccessoryData{ + ID: 1, + SubArtifactID: 2, + }) + m.Require().Nil(err) + m.dao.AssertExpectations(m.T()) +} + func (m *managerTestSuite) TestDelete() { mock.OnAnything(m.dao, "Delete").Return(nil) err := m.mgr.Delete(nil, 1) diff --git a/src/server/middleware/subject/subject.go b/src/server/middleware/subject/subject.go index b285891e70a1..1eac45b6ed14 100644 --- a/src/server/middleware/subject/subject.go +++ b/src/server/middleware/subject/subject.go @@ -21,6 +21,7 @@ import ( "net/http" "github.com/docker/distribution/manifest/schema2" + "github.com/opencontainers/go-digest" ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/goharbor/harbor/src/controller/artifact" @@ -28,6 +29,7 @@ import ( "github.com/goharbor/harbor/src/lib/errors" "github.com/goharbor/harbor/src/lib/log" "github.com/goharbor/harbor/src/lib/orm" + "github.com/goharbor/harbor/src/lib/q" "github.com/goharbor/harbor/src/pkg/accessory" "github.com/goharbor/harbor/src/pkg/accessory/model" "github.com/goharbor/harbor/src/server/middleware" @@ -147,6 +149,34 @@ func Middleware() func(http.Handler) http.Handler { } } w.Header().Set("OCI-Subject", subjectArt.Digest) + } else { + // In certain cases, the OCI client may push the subject artifact and accessory in either order. + // Therefore, it is necessary to handle situations where the client pushes the accessory ahead of the subject artifact. + digest := digest.FromBytes(body) + accs, err := accessory.Mgr.List(ctx, q.New(q.KeyWords{"SubjectArtifactDigest": digest})) + if err != nil { + logger.Errorf("failed to list accessory artifact: %s, error: %v", digest, err) + return err + } + if len(accs) <= 0 { + return nil + } + art, err := artifact.Ctl.GetByReference(ctx, info.Repository, digest.String(), nil) + if err != nil { + logger.Errorf("failed to list artifact: %s, error: %v", digest, err) + return err + } + if art != nil { + for _, acc := range accs { + accData := model.AccessoryData{ + ID: acc.GetData().ID, + SubArtifactID: art.ID, + } + if err := accessory.Mgr.Update(ctx, accData); err != nil { + return err + } + } + } } return nil diff --git a/src/server/middleware/subject/subject_test.go b/src/server/middleware/subject/subject_test.go index cd8a585ac63a..4ba5bbad4c56 100644 --- a/src/server/middleware/subject/subject_test.go +++ b/src/server/middleware/subject/subject_test.go @@ -37,7 +37,7 @@ func (suite *MiddlewareTestSuite) SetupTest() { func (suite *MiddlewareTestSuite) TearDownTest() { } -func (suite *MiddlewareTestSuite) prepare(name, subject string) (distribution.Manifest, distribution.Descriptor, *http.Request) { +func (suite *MiddlewareTestSuite) prepare(name, digset string, withoutSub ...bool) (distribution.Manifest, distribution.Descriptor, *http.Request) { body := fmt.Sprintf(` { "schemaVersion":2, @@ -61,7 +61,29 @@ func (suite *MiddlewareTestSuite) prepare(name, subject string) (distribution.Ma "mediaType":"application/vnd.oci.image.manifest.v1+json", "size":419, "digest":"%s" - }}`, subject) + }}`, digset) + + if len(withoutSub) > 0 && withoutSub[0] { + body = fmt.Sprintf(` + { + "schemaVersion":2, + "mediaType":"application/vnd.oci.image.manifest.v1+json", + "config":{ + "mediaType":"application/vnd.example.sbom", + "size":2, + "digest":"%s" + }, + "layers":[ + { + "mediaType":"application/vnd.example.sbom.text", + "size":37, + "digest":"sha256:45592a729ef6884ea3297e9510d79104f27aeef5f4919b3a921e3abb7f469709" + } + ], + "annotations":{ + "org.example.sbom.format":"text" + }}`, digset) + } manifest, descriptor, err := distribution.UnmarshalManifest("application/vnd.oci.image.manifest.v1+json", []byte(body)) suite.Nil(err) @@ -94,19 +116,21 @@ func (suite *MiddlewareTestSuite) addArt(pid, repositoryID int64, repositoryName return afid } -func (suite *MiddlewareTestSuite) addArtAcc(pid, repositoryID int64, repositoryName, dgt, accdgt string) int64 { - subaf := &artifact.Artifact{ - Type: "Docker-Image", - ProjectID: pid, - RepositoryID: repositoryID, - RepositoryName: repositoryName, - Digest: dgt, - Size: 1024, - PushTime: time.Now(), - PullTime: time.Now(), +func (suite *MiddlewareTestSuite) addArtAcc(pid, repositoryID int64, repositoryName, dgt, accdgt string, createSub ...bool) int64 { + if len(createSub) > 0 && createSub[0] { + subaf := &artifact.Artifact{ + Type: "Docker-Image", + ProjectID: pid, + RepositoryID: repositoryID, + RepositoryName: repositoryName, + Digest: dgt, + Size: 1024, + PushTime: time.Now(), + PullTime: time.Now(), + } + _, err := pkg.ArtifactMgr.Create(suite.Context(), subaf) + suite.Nil(err, fmt.Sprintf("Add artifact failed for %d", repositoryID)) } - _, err := pkg.ArtifactMgr.Create(suite.Context(), subaf) - suite.Nil(err, fmt.Sprintf("Add artifact failed for %d", repositoryID)) af := &artifact.Artifact{ Type: "Subject", @@ -124,7 +148,7 @@ func (suite *MiddlewareTestSuite) addArtAcc(pid, repositoryID int64, repositoryN accid, err := accessory.Mgr.Create(suite.Context(), accessorymodel.AccessoryData{ ID: 1, ArtifactID: afid, - SubArtifactDigest: subaf.Digest, + SubArtifactDigest: dgt, Digest: accdgt, Type: accessorymodel.TypeSubject, }) @@ -165,6 +189,39 @@ func (suite *MiddlewareTestSuite) TestSubject() { }) } +func (suite *MiddlewareTestSuite) TestSubjectAfterAcc() { + // add acc, with subject digest + // add subject + suite.WithProject(func(projectID int64, projectName string) { + name := fmt.Sprintf("%s/hello-world", projectName) + _, repoId, err := repository.Ctl.Ensure(suite.Context(), name) + + _, descriptor, req := suite.prepare(name, suite.DigestString(), true) + suite.Nil(err) + + subArtDigest := descriptor.Digest.String() + accArtDigest := suite.DigestString() + + accID := suite.addArtAcc(projectID, repoId, name, subArtDigest, accArtDigest) + subArtID := suite.addArt(projectID, repoId, name, subArtDigest) + + res := httptest.NewRecorder() + next := suite.NextHandler(http.StatusCreated, map[string]string{"Docker-Content-Digest": subArtDigest}) + Middleware()(next).ServeHTTP(res, req) + suite.Equal(http.StatusCreated, res.Code) + + accs, err := accessory.Mgr.List(suite.Context(), &q.Query{ + Keywords: map[string]interface{}{ + "SubjectArtifactDigest": subArtDigest, + }, + }) + suite.Equal(1, len(accs)) + suite.Equal(subArtDigest, accs[0].GetData().SubArtifactDigest) + suite.Equal(subArtID, accs[0].GetData().SubArtifactID) + suite.Equal(accID, accs[0].GetData().ID) + }) +} + func (suite *MiddlewareTestSuite) TestSubjectDup() { suite.WithProject(func(projectID int64, projectName string) { name := fmt.Sprintf("%s/hello-world", projectName) @@ -174,7 +231,7 @@ func (suite *MiddlewareTestSuite) TestSubjectDup() { _, descriptor, req := suite.prepare(name, subArtDigest) suite.Nil(err) - accID := suite.addArtAcc(projectID, repoId, name, subArtDigest, descriptor.Digest.String()) + accID := suite.addArtAcc(projectID, repoId, name, subArtDigest, descriptor.Digest.String(), true) res := httptest.NewRecorder() next := suite.NextHandler(http.StatusCreated, map[string]string{"Docker-Content-Digest": descriptor.Digest.String()}) diff --git a/src/testing/pkg/accessory/dao/dao.go b/src/testing/pkg/accessory/dao/dao.go index a59eb8a011ed..ad5bc393724a 100644 --- a/src/testing/pkg/accessory/dao/dao.go +++ b/src/testing/pkg/accessory/dao/dao.go @@ -154,6 +154,20 @@ func (_m *DAO) List(ctx context.Context, query *q.Query) ([]*dao.Accessory, erro return r0, r1 } +// Update provides a mock function with given fields: ctx, accessory +func (_m *DAO) Update(ctx context.Context, accessory *dao.Accessory) error { + ret := _m.Called(ctx, accessory) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, *dao.Accessory) error); ok { + r0 = rf(ctx, accessory) + } else { + r0 = ret.Error(0) + } + + return r0 +} + type mockConstructorTestingTNewDAO interface { mock.TestingT Cleanup(func()) diff --git a/src/testing/pkg/accessory/manager.go b/src/testing/pkg/accessory/manager.go index fd9ebb99f9ea..8d16162f78af 100644 --- a/src/testing/pkg/accessory/manager.go +++ b/src/testing/pkg/accessory/manager.go @@ -158,6 +158,20 @@ func (_m *Manager) List(ctx context.Context, query *q.Query) ([]model.Accessory, return r0, r1 } +// Update provides a mock function with given fields: ctx, _a1 +func (_m *Manager) Update(ctx context.Context, _a1 model.AccessoryData) error { + ret := _m.Called(ctx, _a1) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, model.AccessoryData) error); ok { + r0 = rf(ctx, _a1) + } else { + r0 = ret.Error(0) + } + + return r0 +} + type mockConstructorTestingTNewManager interface { mock.TestingT Cleanup(func())