From 7fad103e4616aaedd9f0dacf706304eae58395fd Mon Sep 17 00:00:00 2001 From: Steven Zou Date: Wed, 23 Oct 2019 16:02:18 +0800 Subject: [PATCH] - fix API test cases failures Signed-off-by: Steven Zou - fix scan report dao bug --- src/core/api/scan_all.go | 40 ++++++++++++--- src/core/api/scan_all_test.go | 64 +++++++++++++++++------- src/pkg/scan/api/scan/base_controller.go | 10 ++-- src/pkg/scan/dao/scan/report.go | 1 + src/pkg/scan/report/base_manager.go | 18 +++++-- src/pkg/scan/report/summary.go | 3 ++ 6 files changed, 105 insertions(+), 31 deletions(-) diff --git a/src/core/api/scan_all.go b/src/core/api/scan_all.go index 11ca8ab44..ef95463c9 100644 --- a/src/core/api/scan_all.go +++ b/src/core/api/scan_all.go @@ -1,14 +1,15 @@ package api import ( - "errors" "net/http" "strconv" + "github.com/goharbor/harbor/src/pkg/q" + common_job "github.com/goharbor/harbor/src/common/job" - "github.com/goharbor/harbor/src/common/utils/log" "github.com/goharbor/harbor/src/core/api/models" - "github.com/goharbor/harbor/src/core/config" + "github.com/goharbor/harbor/src/pkg/scan/api/scanner" + "github.com/pkg/errors" ) // ScanAllAPI handles request of scan all images... @@ -19,11 +20,7 @@ type ScanAllAPI struct { // Prepare validates the URL and parms, it needs the system admin permission. func (sc *ScanAllAPI) Prepare() { sc.BaseController.Prepare() - if !config.WithClair() { - log.Warningf("Harbor is not deployed with Clair, it's not possible to scan images.") - sc.SendStatusServiceUnavailableError(errors.New("")) - return - } + if !sc.SecurityCtx.IsAuthenticated() { sc.SendUnAuthorizedError(errors.New("UnAuthorized")) return @@ -32,6 +29,17 @@ func (sc *ScanAllAPI) Prepare() { sc.SendForbiddenError(errors.New(sc.SecurityCtx.GetUsername())) return } + + enabled, err := isScanEnabled() + if err != nil { + sc.SendInternalServerError(err) + return + } + + if !enabled { + sc.SendStatusServiceUnavailableError(errors.New("no scanner is configured, it's not possible to scan")) + return + } } // Post according to the request, it creates a cron schedule or a manual trigger for scan all. @@ -88,3 +96,19 @@ func (sc *ScanAllAPI) Get() { func (sc *ScanAllAPI) List() { sc.list(common_job.ImageScanAllJob) } + +func isScanEnabled() (bool, error) { + kws := make(map[string]interface{}) + kws["is_default"] = true + + query := &q.Query{ + Keywords: kws, + } + + l, err := scanner.DefaultController.ListRegistrations(query) + if err != nil { + return false, errors.Wrap(err, "scan all API: check if scan is enabled") + } + + return len(l) > 0, nil +} diff --git a/src/core/api/scan_all_test.go b/src/core/api/scan_all_test.go index 347b39182..9f20e2ad4 100644 --- a/src/core/api/scan_all_test.go +++ b/src/core/api/scan_all_test.go @@ -3,36 +3,66 @@ package api import ( "testing" + "github.com/goharbor/harbor/src/pkg/scan/dao/scanner" + sc "github.com/goharbor/harbor/src/pkg/scan/scanner" "github.com/goharbor/harbor/src/testing/apitests/apilib" - "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/stretchr/testify/suite" ) var adminJob002 apilib.AdminJobReq -func TestScanAllPost(t *testing.T) { +// ScanAllAPITestSuite is a test suite to test scan all API. +type ScanAllAPITestSuite struct { + suite.Suite - assert := assert.New(t) + m sc.Manager + uuid string +} + +// TestScanAllAPI is an entry point for ScanAllAPITestSuite. +func TestScanAllAPI(t *testing.T) { + suite.Run(t, &ScanAllAPITestSuite{}) +} + +// SetupSuite prepares env for test suite. +func (suite *ScanAllAPITestSuite) SetupSuite() { + // Ensure scanner is there + reg := &scanner.Registration{ + Name: "Clair", + Description: "The clair scanner adapter", + URL: "https://clair.com:8080", + Disabled: false, + IsDefault: true, + } + + scMgr := sc.New() + uuid, err := scMgr.Create(reg) + require.NoError(suite.T(), err, "failed to initialize clair scanner") + + suite.uuid = uuid + suite.m = scMgr +} + +// TearDownSuite clears env for the test suite. +func (suite *ScanAllAPITestSuite) TearDownSuite() { + err := suite.m.Delete(suite.uuid) + suite.NoError(err, "clear scanner") +} + +func (suite *ScanAllAPITestSuite) TestScanAllPost() { apiTest := newHarborAPI() // case 1: add a new scan all job code, err := apiTest.AddScanAll(*admin, adminJob002) - if err != nil { - t.Error("Error occurred while add a scan all job", err.Error()) - t.Log(err) - } else { - assert.Equal(200, code, "Add scan all status should be 200") - } + require.NoError(suite.T(), err, "Error occurred while add a scan all job") + suite.Equal(200, code, "Add scan all status should be 200") } -func TestScanAllGet(t *testing.T) { - assert := assert.New(t) +func (suite *ScanAllAPITestSuite) TestScanAllGet() { apiTest := newHarborAPI() code, _, err := apiTest.ScanAllScheduleGet(*admin) - if err != nil { - t.Error("Error occurred while get a scan all job", err.Error()) - t.Log(err) - } else { - assert.Equal(200, code, "Get scan all status should be 200") - } + require.NoError(suite.T(), err, "Error occurred while get a scan all job") + suite.Equal(200, code, "Get scan all status should be 200") } diff --git a/src/pkg/scan/api/scan/base_controller.go b/src/pkg/scan/api/scan/base_controller.go index c810fc3f8..ecc906d94 100644 --- a/src/pkg/scan/api/scan/base_controller.go +++ b/src/pkg/scan/api/scan/base_controller.go @@ -119,6 +119,11 @@ func (bc *basicController) Scan(artifact *v1.Artifact) error { return errors.Wrap(err, "scan controller: scan") } + // Check if it is disabled + if r.Disabled { + return errors.Errorf("scanner %s is disabled", r.Name) + } + // Check the health of the registration by ping. // The metadata of the scanner adapter is also returned. meta, err := bc.sc.Ping(r) @@ -296,9 +301,8 @@ func (bc *basicController) HandleJobHooks(trackID string, change *job.StatusChan } // Clear robot account - // All final statuses (success, error and stopped) share the same code. - // Only need to check one of them. - if job.Status(change.Status).Compare(job.SuccessStatus) == 0 { + // Only when the job is successfully done! + if change.Status == job.SuccessStatus.String() { if v, ok := change.Metadata.Parameters[sca.JobParameterRobotID]; ok { if rid, y := v.(float64); y { if err := robot.RobotCtr.DeleteRobotAccount(int64(rid)); err != nil { diff --git a/src/pkg/scan/dao/scan/report.go b/src/pkg/scan/dao/scan/report.go index 6b428f8a2..29b9cba21 100644 --- a/src/pkg/scan/dao/scan/report.go +++ b/src/pkg/scan/dao/scan/report.go @@ -63,6 +63,7 @@ func ListReports(query *q.Query) ([]*Report, error) { for k, v := range query.Keywords { if vv, ok := v.([]interface{}); ok { qt = qt.Filter(fmt.Sprintf("%s__in", k), vv...) + continue } qt = qt.Filter(k, v) diff --git a/src/pkg/scan/report/base_manager.go b/src/pkg/scan/report/base_manager.go index 9dce344d0..d0f5319fb 100644 --- a/src/pkg/scan/report/base_manager.go +++ b/src/pkg/scan/report/base_manager.go @@ -24,6 +24,10 @@ import ( "github.com/pkg/errors" ) +const ( + reportTimeout = 1 * time.Hour +) + // basicManager is a default implementation of report manager. type basicManager struct{} @@ -72,8 +76,10 @@ func (bm *basicManager) Create(r *scan.Report) (string, error) { } // Status conflict - if theStatus.Compare(job.RunningStatus) <= 0 { - return "", errors.Errorf("conflict: a previous scanning is %s", theCopy.Status) + if theCopy.StartTime.Add(reportTimeout).After(time.Now()) { + if theStatus.Compare(job.RunningStatus) <= 0 { + return "", errors.Errorf("conflict: a previous scanning is %s", theCopy.Status) + } } // Otherwise it will be a completed report @@ -201,7 +207,13 @@ func (bm *basicManager) DeleteByDigests(digests ...string) error { } kws := make(map[string]interface{}) - kws["digest"] = digests + ds := make([]interface{}, 0) + + for _, dig := range digests { + ds = append(ds, dig) + } + + kws["digest"] = ds query := &q.Query{ Keywords: kws, } diff --git a/src/pkg/scan/report/summary.go b/src/pkg/scan/report/summary.go index ea13cabd5..cbd611b17 100644 --- a/src/pkg/scan/report/summary.go +++ b/src/pkg/scan/report/summary.go @@ -82,6 +82,9 @@ func GenerateNativeSummary(r *scan.Report, options ...Option) (interface{}, erro sum.StartTime = r.StartTime sum.EndTime = r.EndTime sum.Duration = r.EndTime.Unix() - r.StartTime.Unix() + if sum.Duration < 0 { + sum.Duration = 0 + } if len(ops.CVEWhitelist) > 0 { sum.CVEBypassed = make([]string, 0) }