From bf4cfe9e1e7ca4b7d1ef55455b4af7b436ac3f34 Mon Sep 17 00:00:00 2001 From: Chlins Zhang Date: Tue, 1 Nov 2022 15:04:07 +0800 Subject: [PATCH] fix: add human friendly message when export cve job failure (#17726) Add human friendly when export CVE in the condition of empty CSV file, because this file will be stored as system artifact and pushed to distribution, but it will leads to error when push empty blob to S3 storage driver. Signed-off-by: chlins --- src/controller/scandataexport/execution.go | 5 +++ .../scandataexport/execution_test.go | 2 + .../impl/scandataexport/scan_data_export.go | 24 +++++++++--- .../scandataexport/scan_data_export_test.go | 34 +++++++++++++++++ src/server/v2.0/handler/scanexport.go | 6 ++- src/server/v2.0/handler/scanexport_test.go | 38 ++++++++++++++++--- 6 files changed, 96 insertions(+), 13 deletions(-) diff --git a/src/controller/scandataexport/execution.go b/src/controller/scandataexport/execution.go index 17227c8d8..2a36f30c0 100644 --- a/src/controller/scandataexport/execution.go +++ b/src/controller/scandataexport/execution.go @@ -185,6 +185,11 @@ func (c *controller) convertToExportExecStatus(ctx context.Context, exec *task.E if userName, ok := exec.ExtraAttrs[export.UserNameAttribute]; ok { execStatus.UserName = userName.(string) } + // Do not define the 'status_message' as const because this is not the final solution, + // should refactor this part. + if statusMessage, ok := exec.ExtraAttrs["status_message"]; ok { + execStatus.StatusMessage = statusMessage.(string) + } artifactExists := c.isCsvArtifactPresent(ctx, exec.ID, execStatus.ExportDataDigest) execStatus.FilePresent = artifactExists return execStatus diff --git a/src/controller/scandataexport/execution_test.go b/src/controller/scandataexport/execution_test.go index b8b597832..6f536393f 100644 --- a/src/controller/scandataexport/execution_test.go +++ b/src/controller/scandataexport/execution_test.go @@ -97,6 +97,7 @@ func (suite *ScanDataExportExecutionTestSuite) TestGetExecution() { attrs := make(map[string]interface{}) attrs[export.JobNameAttribute] = "test-job" attrs[export.UserNameAttribute] = "test-user" + attrs["status_message"] = "test-message" { exec := task.Execution{ ID: 100, @@ -119,6 +120,7 @@ func (suite *ScanDataExportExecutionTestSuite) TestGetExecution() { suite.Equal(exec.ID, exportExec.ID) suite.Equal("test-user", exportExec.UserName) suite.Equal("test-job", exportExec.JobName) + suite.Equal("test-message", exportExec.StatusMessage) suite.Equal(true, exportExec.FilePresent) } diff --git a/src/jobservice/job/impl/scandataexport/scan_data_export.go b/src/jobservice/job/impl/scandataexport/scan_data_export.go index 3f4a23dab..92b65c127 100644 --- a/src/jobservice/job/impl/scandataexport/scan_data_export.go +++ b/src/jobservice/job/impl/scandataexport/scan_data_export.go @@ -114,12 +114,24 @@ func (sde *ScanDataExport) Run(ctx job.Context, params job.Parameters) error { if err != nil { logger.Errorf( "Export Job Id = %v. Error when persisting report file %s to persistent storage: %v", params["JobId"], fileName, err) + // NOTICE: this is a tentative solution to resolve error to push empty blob to S3 storage driver, + // should unify the behaviour for different drivers. + // Temporary set the status message to extra attributes, then the API handler will fetch it and combined to response for better experience. + if stat.Size() == 0 { + extra := map[string]interface{}{ + "status_message": "No vulnerabilities found or matched", + } + updateErr := sde.updateExecAttributes(ctx, params, extra) + if updateErr != nil { + logger.Errorf("Export Job Id = %v. Error when updating the exec extra attributes 'status_message' to 'No vulnerabilities found or matched': %v", params["JobId"], updateErr) + } + } + return err } logger.Infof("Export Job Id = %v. Created system artifact: %v for report file %s to persistent storage: %v", params["JobId"], artID, fileName, err) - err = sde.updateExecAttributes(ctx, params, err, hash) - + err = sde.updateExecAttributes(ctx, params, map[string]interface{}{export.DigestKey: hash.String()}) if err != nil { logger.Errorf("Export Job Id = %v. Error when updating execution record : %v", params["JobId"], err) return err @@ -129,7 +141,7 @@ func (sde *ScanDataExport) Run(ctx job.Context, params job.Parameters) error { return nil } -func (sde *ScanDataExport) updateExecAttributes(ctx job.Context, params job.Parameters, err error, hash digest.Digest) error { +func (sde *ScanDataExport) updateExecAttributes(ctx job.Context, params job.Parameters, attrs map[string]interface{}) error { execID := int64(params["JobId"].(float64)) exec, err := sde.execMgr.Get(ctx.SystemContext(), execID) logger := ctx.GetLogger() @@ -137,11 +149,11 @@ func (sde *ScanDataExport) updateExecAttributes(ctx job.Context, params job.Para logger.Errorf("Export Job Id = %v. Error when fetching execution record for update : %v", params["JobId"], err) return err } - attrsToUpdate := make(map[string]interface{}) - for k, v := range exec.ExtraAttrs { + // copy old extra + attrsToUpdate := exec.ExtraAttrs + for k, v := range attrs { attrsToUpdate[k] = v } - attrsToUpdate[export.DigestKey] = hash.String() return sde.execMgr.UpdateExtraAttrs(ctx.SystemContext(), execID, attrsToUpdate) } diff --git a/src/jobservice/job/impl/scandataexport/scan_data_export_test.go b/src/jobservice/job/impl/scandataexport/scan_data_export_test.go index d0d68632f..654ec97de 100644 --- a/src/jobservice/job/impl/scandataexport/scan_data_export_test.go +++ b/src/jobservice/job/impl/scandataexport/scan_data_export_test.go @@ -106,6 +106,40 @@ func (suite *ScanDataExportJobTestSuite) TestRun() { } +func (suite *ScanDataExportJobTestSuite) TestRunCreateSysArtError() { + oldSysArtMgr := suite.sysArtifactMgr + defer func() { + suite.job.sysArtifactMgr = oldSysArtMgr + }() + + sysArtMgr := &systemartifacttesting.Manager{} + suite.job.sysArtifactMgr = sysArtMgr + sysArtMgr.On("Create", mock.Anything, mock.Anything, mock.Anything).Return(int64(-1), errors.New("create sys artifact error")).Once() + // mock create system artifact error when file is empty + var data []export.Data + mock.OnAnything(suite.exportMgr, "Fetch").Return(data, nil).Once() + mock.OnAnything(suite.exportMgr, "Fetch").Return(make([]export.Data, 0), nil).Once() + mock.OnAnything(suite.digestCalculator, "Calculate").Return(digest.Digest(MockDigest), nil) + + execAttrs := make(map[string]interface{}) + execAttrs[export.JobNameAttribute] = "test-job" + execAttrs[export.UserNameAttribute] = "test-user" + mock.OnAnything(suite.execMgr, "Get").Return(&task.Execution{ID: int64(JobId), ExtraAttrs: execAttrs}, nil) + + params := job.Parameters{} + params[export.JobModeKey] = export.JobModeExport + params["JobId"] = JobId + ctx := &mockjobservice.MockJobContext{} + + err := suite.job.Run(ctx, params) + suite.Error(err) + + extraAttrsMatcher := testifymock.MatchedBy(func(attrsMap map[string]interface{}) bool { + return attrsMap["status_message"] == "No vulnerabilities found or matched" && attrsMap[export.JobNameAttribute] == "test-job" && attrsMap[export.UserNameAttribute] == "test-user" + }) + suite.execMgr.AssertCalled(suite.T(), "UpdateExtraAttrs", mock.Anything, int64(JobId), extraAttrsMatcher) +} + func (suite *ScanDataExportJobTestSuite) TestRunAttributeUpdateError() { data := suite.createDataRecords(3, 1) diff --git a/src/server/v2.0/handler/scanexport.go b/src/server/v2.0/handler/scanexport.go index e4f29192e..6b555f6fc 100644 --- a/src/server/v2.0/handler/scanexport.go +++ b/src/server/v2.0/handler/scanexport.go @@ -132,6 +132,10 @@ func (se *scanDataExportAPI) GetScanDataExportExecution(ctx context.Context, par UserName: execution.UserName, FilePresent: execution.FilePresent, } + // add human friendly message when status is error + if sdeExec.Status == job.ErrorStatus.String() && sdeExec.StatusText == "" { + sdeExec.StatusText = "Please contact the system administrator to check the logs of jobservice." + } return operation.NewGetScanDataExportExecutionOK().WithPayload(&sdeExec) } @@ -226,7 +230,7 @@ func (se *scanDataExportAPI) GetScanDataExportExecutionList(ctx context.Context, FilePresent: execution.FilePresent, } // add human friendly message when status is error - if sdeExec.Status == job.ErrorStatus.String() { + if sdeExec.Status == job.ErrorStatus.String() && sdeExec.StatusText == "" { sdeExec.StatusText = "Please contact the system administrator to check the logs of jobservice." } // store project ids diff --git a/src/server/v2.0/handler/scanexport_test.go b/src/server/v2.0/handler/scanexport_test.go index 01dfff7a7..6130813dc 100644 --- a/src/server/v2.0/handler/scanexport_test.go +++ b/src/server/v2.0/handler/scanexport_test.go @@ -350,16 +350,18 @@ func (suite *ScanExportTestSuite) TestExportScanDataNoPrivileges() { func (suite *ScanExportTestSuite) TestGetScanDataExportExecution() { suite.Security.On("GetUsername").Return("test-user") - suite.Security.On("IsAuthenticated").Return(true).Once() - suite.Security.On("Can", mock.Anything, mock.Anything, mock.Anything).Return(true).Once() + suite.Security.On("IsAuthenticated").Return(true).Twice() + suite.Security.On("Can", mock.Anything, mock.Anything, mock.Anything).Return(true).Twice() url := "/export/cve/execution/100" endTime := time.Now() startTime := endTime.Add(-10 * time.Minute) + defaultStatusMessage := "Please contact the system administrator to check the logs of jobservice." + customizeStatusMessage := "No vulnerabilities found or matched" execution := &export.Execution{ ID: 100, UserID: 3, - Status: "Success", + Status: "Error", StatusMessage: "", Trigger: "MANUAL", StartTime: startTime, @@ -377,7 +379,17 @@ func (suite *ScanExportTestSuite) TestGetScanDataExportExecution() { json.NewDecoder(res.Body).Decode(&respData) suite.Equal("test-user", respData.UserName) suite.Equal(false, respData.FilePresent) + suite.Equal(defaultStatusMessage, respData.StatusText) + // test customize status message + execution.StatusMessage = customizeStatusMessage + mock.OnAnything(suite.scanExportCtl, "GetExecution").Return(execution, nil).Once() + res, err = suite.DoReq(http.MethodGet, url, nil) + suite.Equal(200, res.StatusCode) + suite.Equal(nil, err) + respData = models.ScanDataExportExecution{} + json.NewDecoder(res.Body).Decode(&respData) + suite.Equal(customizeStatusMessage, respData.StatusText) } func (suite *ScanExportTestSuite) TestGetScanDataExportExecutionUserNotOwnerOfExport() { @@ -567,19 +579,21 @@ func (suite *ScanExportTestSuite) TestDownloadScanDataExecutionError() { func (suite *ScanExportTestSuite) TestGetScanDataExportExecutionList() { suite.Security.On("GetUsername").Return("test-user") - suite.Security.On("IsAuthenticated").Return(true).Once() - suite.Security.On("Can", mock.Anything, mock.Anything, mock.Anything).Return(true).Once() + suite.Security.On("IsAuthenticated").Return(true).Twice() + suite.Security.On("Can", mock.Anything, mock.Anything, mock.Anything).Return(true).Twice() url, err := url2.Parse("/export/cve/executions") params := url2.Values{} params.Add("user_name", "test-user") url.RawQuery = params.Encode() endTime := time.Now() startTime := endTime.Add(-10 * time.Minute) + defaultStatusMessage := "Please contact the system administrator to check the logs of jobservice." + customizeStatusMessage := "No vulnerabilities found or matched" execution := &export.Execution{ ID: 100, UserID: 3, - Status: "Success", + Status: "Error", StatusMessage: "", Trigger: "MANUAL", StartTime: startTime, @@ -597,6 +611,18 @@ func (suite *ScanExportTestSuite) TestGetScanDataExportExecutionList() { json.NewDecoder(res.Body).Decode(&respData) suite.Equal(1, len(respData.Items)) suite.Equal(int64(100), respData.Items[0].ID) + suite.Equal(defaultStatusMessage, respData.Items[0].StatusText) + // test customize status message + execution.StatusMessage = customizeStatusMessage + mock.OnAnything(suite.scanExportCtl, "ListExecutions").Return([]*export.Execution{execution}, nil).Once() + res, err = suite.DoReq(http.MethodGet, url.String(), nil) + suite.Equal(200, res.StatusCode) + suite.Equal(nil, err) + respData = models.ScanDataExportExecutionList{} + json.NewDecoder(res.Body).Decode(&respData) + suite.Equal(1, len(respData.Items)) + suite.Equal(int64(100), respData.Items[0].ID) + suite.Equal(customizeStatusMessage, respData.Items[0].StatusText) } func (suite *ScanExportTestSuite) TestGetScanDataExportExecutionListFilterNotOwned() {