From 827d6bcb18cb46fa3124a7b04d9f26757b53e035 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 | 17 ++++ 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, 181 insertions(+), 16 deletions(-) diff --git a/src/pkg/accessory/dao/dao.go b/src/pkg/accessory/dao/dao.go index 651871f8a55..96e63e05ba8 100644 --- a/src/pkg/accessory/dao/dao.go +++ b/src/pkg/accessory/dao/dao.go @@ -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 4a23a7b3046..06e9ae2cccf 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 6c1d68d276d..0c02908d571 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 7e436ce46f4..a06bb5ccd01 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 b285891e70a..1eac45b6ed1 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 cd8a585ac63..4ba5bbad4c5 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 a59eb8a011e..ad5bc393724 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 fd9ebb99f9e..8d16162f78a 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())