diff --git a/managed/cmd/pmm-encryption-rotation/main.go b/managed/cmd/pmm-encryption-rotation/main.go index b2dde84e6df..249d980e1c8 100644 --- a/managed/cmd/pmm-encryption-rotation/main.go +++ b/managed/cmd/pmm-encryption-rotation/main.go @@ -75,7 +75,10 @@ func main() { } statusCode, err := encryptionService.RotateEncryptionKey(sqlDB, "pmm-managed") - sqlDB.Close() //nolint:errcheck + closeErr := sqlDB.Close() + if closeErr != nil { + logrus.Error(closeErr) + } if err != nil { logrus.Error(err) os.Exit(statusCode) diff --git a/managed/cmd/pmm-managed/main.go b/managed/cmd/pmm-managed/main.go index 6d2866be196..b81be295413 100644 --- a/managed/cmd/pmm-managed/main.go +++ b/managed/cmd/pmm-managed/main.go @@ -130,8 +130,13 @@ const ( cleanOlderThan = 30 * time.Minute defaultContextTimeout = 10 * time.Second - pProfProfileDuration = 30 * time.Second - pProfTraceDuration = 10 * time.Second + // When ReadHeaderTimeout is not configured, a malicious client can open + // a connection and send headers extremely slowly (or not at all), + // keeping the connection open indefinitely. This "Slowloris" attack can eventually + // exhaust the server's connection pool, leading to a Denial of Service (DoS). + readHeaderTimeout = 10 * time.Second + pProfProfileDuration = 30 * time.Second + pProfTraceDuration = 10 * time.Second clickhouseMaxIdleConns = 5 clickhouseMaxOpenConns = 10 @@ -439,10 +444,11 @@ func runHTTP1Server(ctx context.Context, deps *http1ServerDeps) { mux.Handle("/v1/users/current", deps.currentUserHandler) mux.Handle("/", proxyMux) - server := &http.Server{ //nolint:gosec - Addr: http1Addr, - ErrorLog: log.New(os.Stderr, "runJSONServer: ", 0), - Handler: mux, + server := &http.Server{ + Addr: http1Addr, + ErrorLog: log.New(os.Stderr, "runJSONServer: ", 0), + Handler: mux, + ReadHeaderTimeout: readHeaderTimeout, } go func() { err := server.ListenAndServe() @@ -499,13 +505,17 @@ func runDebugServer(ctx context.Context) { l.Fatal(err) } http.HandleFunc("/debug", func(rw http.ResponseWriter, _ *http.Request) { - rw.Write(buf.Bytes()) //nolint:errcheck + _, wErr := rw.Write(buf.Bytes()) + if wErr != nil { + l.Errorf("Failed to write debug page: %v", wErr) + } }) l.Infof("Starting server on http://%s/debug\nRegistered handlers:\n\t%s", debugAddr, strings.Join(handlers, "\n\t")) - server := &http.Server{ //nolint:gosec - Addr: debugAddr, - ErrorLog: log.New(os.Stderr, "runDebugServer: ", 0), + server := &http.Server{ + Addr: debugAddr, + ErrorLog: log.New(os.Stderr, "runDebugServer: ", 0), + ReadHeaderTimeout: readHeaderTimeout, } go func() { err := server.ListenAndServe() diff --git a/managed/models/artifact_helpers.go b/managed/models/artifact_helpers.go index 6c3f08da762..564f4b35b96 100644 --- a/managed/models/artifact_helpers.go +++ b/managed/models/artifact_helpers.go @@ -330,10 +330,8 @@ func DeleteArtifact(q *reform.Querier, id string) error { // MetadataRemoveFirstN removes first N records from artifact metadata list. func (s *Artifact) MetadataRemoveFirstN(q *reform.Querier, n uint32) error { - if n > uint32(len(s.MetadataList)) { - n = uint32(len(s.MetadataList)) - } - s.MetadataList = s.MetadataList[n:] + idx := min(int(n), len(s.MetadataList)) + s.MetadataList = s.MetadataList[idx:] err := q.Update(s) if err != nil { return fmt.Errorf("failed to remove artifact metadata records: %w", err) diff --git a/managed/models/artifact_helpers_test.go b/managed/models/artifact_helpers_test.go index 9fd91e13689..a8c9d68f1e8 100644 --- a/managed/models/artifact_helpers_test.go +++ b/managed/models/artifact_helpers_test.go @@ -118,6 +118,7 @@ func TestArtifacts(t *testing.T) { assert.Equal(t, createParams.DataModel, a.DataModel) assert.Equal(t, createParams.Status, a.Status) assert.Equal(t, createParams.Folder, a.Folder) + assert.Equal(t, models.OnDemandArtifactType, a.Type) assert.Less(t, time.Now().UTC().Unix()-a.CreatedAt.Unix(), int64(5)) updateParams := models.UpdateArtifactParams{ @@ -135,9 +136,108 @@ func TestArtifacts(t *testing.T) { assert.Equal(t, *updateParams.ServiceID, a.ServiceID) assert.Equal(t, updateParams.IsShardedCluster, a.IsShardedCluster) assert.Less(t, time.Now().UTC().Unix()-a.UpdatedAt.Unix(), int64(5)) + + t.Run("scheduled type", func(t *testing.T) { + createParams.Name = "scheduled_backup" + createParams.ScheduleID = "some_schedule" + sa, err := models.CreateArtifact(q, createParams) + require.NoError(t, err) + assert.Equal(t, models.ScheduledArtifactType, sa.Type) + assert.Equal(t, "some_schedule", sa.ScheduleID) + }) + }) + + t.Run("unique name", func(t *testing.T) { + tx, err := db.Begin() + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, tx.Rollback()) + }) + + q := tx.Querier + prepareLocationsAndService(q) + + params := models.CreateArtifactParams{ + Name: "unique_name", + Vendor: "MySQL", + LocationID: locationID1, + ServiceID: serviceID1, + DataModel: models.PhysicalDataModel, + Status: models.PendingBackupStatus, + Mode: models.Snapshot, + } + + _, err = models.CreateArtifact(q, params) + require.NoError(t, err) + + _, err = models.CreateArtifact(q, params) + require.Error(t, err) + assert.Contains(t, err.Error(), `Artifact with name "unique_name" already exists.`) + }) + + t.Run("find by ID and name", func(t *testing.T) { + tx, err := db.Begin() + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, tx.Rollback()) + }) + + q := tx.Querier + prepareLocationsAndService(q) + + a, err := models.CreateArtifact(q, models.CreateArtifactParams{ + Name: "find_me", + Vendor: "MySQL", + LocationID: locationID1, + ServiceID: serviceID1, + DataModel: models.PhysicalDataModel, + Status: models.PendingBackupStatus, + Mode: models.Snapshot, + }) + require.NoError(t, err) + + // Find by ID + found, err := models.FindArtifactByID(q, a.ID) + require.NoError(t, err) + assert.Equal(t, a.Name, found.Name) + + _, err = models.FindArtifactByID(q, "non-existent") + require.ErrorIs(t, err, models.ErrNotFound) + + // Find by Name + found, err = models.FindArtifactByName(q, "find_me") + require.NoError(t, err) + assert.Equal(t, a.ID, found.ID) + + _, err = models.FindArtifactByName(q, "unknown") + assert.ErrorIs(t, err, models.ErrNotFound) }) - t.Run("list", func(t *testing.T) { + t.Run("find by IDs", func(t *testing.T) { + tx, err := db.Begin() + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, tx.Rollback()) + }) + + q := tx.Querier + prepareLocationsAndService(q) + + a1, _ := models.CreateArtifact(q, models.CreateArtifactParams{Name: "a1", Vendor: "V", LocationID: locationID1, ServiceID: serviceID1, DataModel: models.PhysicalDataModel, Status: models.PendingBackupStatus, Mode: models.Snapshot}) + a2, _ := models.CreateArtifact(q, models.CreateArtifactParams{Name: "a2", Vendor: "V", LocationID: locationID1, ServiceID: serviceID1, DataModel: models.PhysicalDataModel, Status: models.PendingBackupStatus, Mode: models.Snapshot}) + + res, err := models.FindArtifactsByIDs(q, []string{a1.ID, a2.ID, "unknown"}) + require.NoError(t, err) + assert.Len(t, res, 2) + assert.Contains(t, res, a1.ID) + assert.Contains(t, res, a2.ID) + + emptyRes, err := models.FindArtifactsByIDs(q, []string{}) + require.NoError(t, err) + assert.Empty(t, emptyRes) + }) + + t.Run("list and filters", func(t *testing.T) { tx, err := db.Begin() require.NoError(t, err) t.Cleanup(func() { @@ -206,13 +306,23 @@ func TestArtifacts(t *testing.T) { require.NoError(t, err) assert.Equal(t, []*models.Artifact{a3}, actual2) - actual3, err := models.FindArtifacts(q, models.ArtifactFilters{}) + // Check filters by status and service. + actual3, err := models.FindArtifacts(q, models.ArtifactFilters{Status: models.PausedBackupStatus, ServiceID: serviceID2}) require.NoError(t, err) - require.Len(t, actual3, 3) + assert.Len(t, actual3, 1) + assert.Equal(t, a2.ID, actual3[0].ID) - for _, a := range actual3 { - assert.Contains(t, []models.Artifact{*a1, *a2, *a3}, *a) - } + // Check non-existent location error. + _, err = models.FindArtifacts(q, models.ArtifactFilters{LocationID: "invalid_loc"}) + require.Error(t, err) + + // Check sorting (latest first). + all, err := models.FindArtifacts(q, models.ArtifactFilters{}) + require.NoError(t, err) + require.Len(t, all, 3) + assert.Equal(t, a3.ID, all[0].ID) + assert.Equal(t, a2.ID, all[1].ID) + assert.Equal(t, a1.ID, all[2].ID) }) t.Run("remove", func(t *testing.T) { @@ -446,3 +556,19 @@ func TestArtifactValidation(t *testing.T) { }) } } + +func TestIsArtifactFinalStatus(t *testing.T) { + testCases := []struct { + status models.BackupStatus + expected bool + }{ + {models.SuccessBackupStatus, true}, + {models.ErrorBackupStatus, true}, + {models.FailedToDeleteBackupStatus, true}, + {models.PendingBackupStatus, false}, + {models.PausedBackupStatus, false}, + } + for _, tc := range testCases { + assert.Equal(t, tc.expected, models.IsArtifactFinalStatus(tc.status), "Status: %s", tc.status) + } +} diff --git a/managed/models/encryption_helpers.go b/managed/models/encryption_helpers.go index ba9aea666a0..b3dd1e9d9ec 100644 --- a/managed/models/encryption_helpers.go +++ b/managed/models/encryption_helpers.go @@ -158,6 +158,9 @@ func awsOptionsHandler(val any, handler func(string) (string, error)) (any, erro return nil, err } + // The gosec G117 warning is triggered when sensitive fields (like ClientSecret, AWSSecretKey, or TLSKey) + // are part of a structure being marshaled to JSON. + // In the context of this file, these operations are intentional. res, err := json.Marshal(o) if err != nil { return nil, err @@ -205,7 +208,10 @@ func azureOptionsHandler(val any, handler func(string) (string, error)) (any, er return nil, err } - res, err := json.Marshal(o) + // The gosec G117 warning is triggered when sensitive fields (like ClientSecret, AWSSecretKey, or TLSKey) + // are part of a structure being marshaled to JSON. + // In the context of this file, these operations are intentional. + res, err := json.Marshal(o) //nolint:gosec if err != nil { return nil, err } @@ -244,6 +250,9 @@ func mongoDBOptionsHandler(val any, handler func(string) (string, error)) (any, return nil, err } + // The gosec G117 warning is triggered when sensitive fields (like ClientSecret, AWSSecretKey, or TLSKey) + // are part of a structure being marshaled to JSON. + // In the context of this file, these operations are intentional. res, err := json.Marshal(o) if err != nil { return nil, err @@ -283,6 +292,9 @@ func mySQLOptionsHandler(val any, handler func(string) (string, error)) (any, er return nil, err } + // The gosec G117 warning is triggered when sensitive fields (like ClientSecret, AWSSecretKey, or TLSKey) + // are part of a structure being marshaled to JSON. + // In the context of this file, these operations are intentional. res, err := json.Marshal(o) if err != nil { return nil, err @@ -322,6 +334,9 @@ func postgreSQLOptionsHandler(val any, handler func(string) (string, error)) (an return nil, err } + // The gosec G117 warning is triggered when sensitive fields (like ClientSecret, AWSSecretKey, or TLSKey) + // are part of a structure being marshaled to JSON. + // In the context of this file, these operations are intentional. res, err := json.Marshal(o) if err != nil { return nil, err diff --git a/managed/models/role_helpers.go b/managed/models/role_helpers.go index f7da492cb45..21e55126248 100644 --- a/managed/models/role_helpers.go +++ b/managed/models/role_helpers.go @@ -18,6 +18,7 @@ package models import ( "errors" "fmt" + "math" "strings" "github.com/sirupsen/logrus" @@ -51,6 +52,9 @@ func AssignRoles(tx *reform.TX, userID int, roleIDs []int) error { return err } + if roleID < 0 || roleID > math.MaxUint32 { + return fmt.Errorf("role ID %d is out of range for uint32", roleID) + } var userRole UserRoles userRole.UserID = userID userRole.RoleID = uint32(roleID) diff --git a/managed/services/agents/actions.go b/managed/services/agents/actions.go index 3c470f74b48..fd041fb97f9 100644 --- a/managed/services/agents/actions.go +++ b/managed/services/agents/actions.go @@ -80,7 +80,7 @@ func (s *ActionsService) StartMySQLExplainAction( return err } - if res.PlaceholdersCount != uint32(len(placeholders)) { + if res.PlaceholdersCount != uint32(len(placeholders)) { //nolint:gosec // placeholders count is not expected to exceed uint32 return status.Error(codes.FailedPrecondition, "placeholders count is not correct") } q = res.ExplainFingerprint diff --git a/managed/services/agents/service_info_broker.go b/managed/services/agents/service_info_broker.go index 1a7a51c216c..168ce2f720e 100644 --- a/managed/services/agents/service_info_broker.go +++ b/managed/services/agents/service_info_broker.go @@ -18,6 +18,7 @@ package agents import ( "context" "fmt" + "math" "time" "github.com/AlekSi/pointer" @@ -213,7 +214,9 @@ func (c *ServiceInfoBroker) GetInfoFromService(ctx context.Context, q *reform.Qu } } agent.PostgreSQLOptions.PGSMVersion = sInfo.PgsmVersion - agent.PostgreSQLOptions.DatabaseCount = int32(databaseCount - excludedDatabaseCount) + + dbCount := min(max(databaseCount-excludedDatabaseCount, 0), math.MaxInt32) + agent.PostgreSQLOptions.DatabaseCount = int32(dbCount) l.Debugf("Updating PostgreSQL options, database count: %d.", agent.PostgreSQLOptions.DatabaseCount) err = q.Update(new(models.EncryptAgent(*agent))) diff --git a/managed/services/alerting/service.go b/managed/services/alerting/service.go index fbed68a9422..74872e81a28 100644 --- a/managed/services/alerting/service.go +++ b/managed/services/alerting/service.go @@ -18,13 +18,15 @@ package alerting import ( "bytes" + "cmp" "context" "errors" "fmt" "maps" + "math" "os" "path/filepath" - "sort" + "slices" "strconv" "strings" "sync" @@ -330,22 +332,22 @@ func convertParamType(t models.ParamType) alerting.ParamType { // ListTemplates returns a list of all collected Alert Rule Templates. func (s *Service) ListTemplates(ctx context.Context, req *alerting.ListTemplatesRequest) (*alerting.ListTemplatesResponse, error) { - settings, err := models.GetSettings(s.db) - if err != nil { - return nil, err + if !s.Enabled() { + return nil, status.Error(codes.FailedPrecondition, services.ErrAlertingDisabled.Error()) } - if !settings.IsAlertingEnabled() { - return nil, services.ErrAlertingDisabled - } - - var pageIndex int - var pageSize int + var pageIndex, pageSize int if req.PageIndex != nil { pageIndex = int(*req.PageIndex) + if pageIndex < 0 { + return nil, status.Errorf(codes.InvalidArgument, "Page index must be non-negative") + } } if req.PageSize != nil { pageSize = int(*req.PageSize) + if pageSize < 0 { + return nil, status.Errorf(codes.InvalidArgument, "Page size must be non-negative") + } } if req.Reload { @@ -353,38 +355,49 @@ func (s *Service) ListTemplates(ctx context.Context, req *alerting.ListTemplates } templates := s.GetTemplates() - res := &alerting.ListTemplatesResponse{ - Templates: make([]*alerting.Template, 0, len(templates)), - TotalItems: int32(len(templates)), - TotalPages: 1, + totalItems := len(templates) + // return value type is int32, so we need to limit it to max int32 value + if totalItems > math.MaxInt32 { + s.l.Warnf("Total templates count %d is greater than max int32 value, limiting to %d.", totalItems, math.MaxInt32) + totalItems = math.MaxInt32 } + totalPages := 1 if pageSize > 0 { - res.TotalPages = int32(len(templates) / pageSize) - if len(templates)%pageSize > 0 { - res.TotalPages++ + totalPages = totalItems / pageSize + if totalItems%pageSize > 0 { + totalPages++ + } + // return value type is int32, so we need to limit it to max int32 value + if totalPages > math.MaxInt32 { + totalPages = math.MaxInt32 } } - names := make([]string, 0, len(templates)) - for name := range templates { - names = append(names, name) - } - sort.Strings(names) + // Sort templates by Name directly using modern Go iterators (Go 1.23+) + // This avoids manual key collection and redundant map lookups. + sorted := slices.SortedFunc(maps.Values(templates), func(a, b models.Template) int { + return cmp.Compare(a.Name, b.Name) + }) - from, to := pageIndex*pageSize, (pageIndex+1)*pageSize - if to > len(names) || to == 0 { - to = len(names) + from := min(pageIndex*pageSize, totalItems) + to := totalItems + if pageSize > 0 { + to = min(from+pageSize, totalItems) } - if from > len(names) { - from = len(names) + // allocate slice with the exact size (to-from) but not the whole capacity (len(names)) + // to avoid unnecessary memory usage + res := &alerting.ListTemplatesResponse{ + Templates: make([]*alerting.Template, 0, to-from), + TotalItems: int32(totalItems), + TotalPages: int32(totalPages), } - - for _, name := range names[from:to] { - t, err := convertTemplate(s.l, templates[name]) + for _, tmpl := range sorted[from:to] { + t, err := convertTemplate(s.l, tmpl) if err != nil { - return nil, err + s.l.WithError(err).Errorf("Failed to convert rule template %s.", tmpl.Name) + return nil, status.Error(codes.Internal, err.Error()) } res.Templates = append(res.Templates, t) @@ -395,13 +408,8 @@ func (s *Service) ListTemplates(ctx context.Context, req *alerting.ListTemplates // CreateTemplate creates a new template. func (s *Service) CreateTemplate(ctx context.Context, req *alerting.CreateTemplateRequest) (*alerting.CreateTemplateResponse, error) { - settings, err := models.GetSettings(s.db) - if err != nil { - return nil, err - } - - if !settings.IsAlertingEnabled() { - return nil, services.ErrAlertingDisabled + if !s.Enabled() { + return nil, status.Error(codes.FailedPrecondition, services.ErrAlertingDisabled.Error()) } pParams := &alert.ParseParams{ @@ -412,7 +420,7 @@ func (s *Service) CreateTemplate(ctx context.Context, req *alerting.CreateTempla templates, err := alert.Parse(strings.NewReader(req.Yaml), pParams) if err != nil { s.l.Errorf("failed to parse rule template form request: %+v", err) - return nil, status.Errorf(codes.InvalidArgument, "Failed to parse rule template: %v.", err) + return nil, status.Error(codes.InvalidArgument, err.Error()) } uniqueNames := make(map[string]struct{}, len(templates)) @@ -423,7 +431,7 @@ func (s *Service) CreateTemplate(ctx context.Context, req *alerting.CreateTempla uniqueNames[t.Name] = struct{}{} err = validateUserTemplate(&t) if err != nil { - return nil, status.Errorf(codes.InvalidArgument, "%s.", err) + return nil, status.Error(codes.InvalidArgument, err.Error()) } } @@ -440,6 +448,8 @@ func (s *Service) CreateTemplate(ctx context.Context, req *alerting.CreateTempla return nil }) if errTx != nil { + s.l.WithError(errTx).Errorf("Failed to create rule template") + // errTx is already wrapped with status.Error in models.CreateTemplate, so we can return it directly return nil, errTx } @@ -450,13 +460,8 @@ func (s *Service) CreateTemplate(ctx context.Context, req *alerting.CreateTempla // UpdateTemplate updates existing template, previously created via API. func (s *Service) UpdateTemplate(ctx context.Context, req *alerting.UpdateTemplateRequest) (*alerting.UpdateTemplateResponse, error) { - settings, err := models.GetSettings(s.db) - if err != nil { - return nil, err - } - - if !settings.IsAlertingEnabled() { - return nil, services.ErrAlertingDisabled + if !s.Enabled() { + return nil, status.Error(codes.FailedPrecondition, services.ErrAlertingDisabled.Error()) } parseParams := &alert.ParseParams{ @@ -467,11 +472,11 @@ func (s *Service) UpdateTemplate(ctx context.Context, req *alerting.UpdateTempla templates, err := alert.Parse(strings.NewReader(req.Yaml), parseParams) if err != nil { s.l.Errorf("failed to parse rule template form request: %+v", err) - return nil, status.Error(codes.InvalidArgument, "Failed to parse rule template.") + return nil, status.Error(codes.InvalidArgument, "Failed to parse rule template") } if len(templates) != 1 { - return nil, status.Error(codes.InvalidArgument, "Request should contain exactly one rule template.") + return nil, status.Error(codes.InvalidArgument, "Request should contain exactly one rule template") } tmpl := templates[0] @@ -492,6 +497,8 @@ func (s *Service) UpdateTemplate(ctx context.Context, req *alerting.UpdateTempla return err }) if e != nil { + s.l.WithError(e).Errorf("Failed to update rule template %s", req.Name) + // e is already wrapped with status.Error in models.ChangeTemplate, so we can return it directly return nil, e } @@ -502,19 +509,16 @@ func (s *Service) UpdateTemplate(ctx context.Context, req *alerting.UpdateTempla // DeleteTemplate deletes existing, previously created via API. func (s *Service) DeleteTemplate(ctx context.Context, req *alerting.DeleteTemplateRequest) (*alerting.DeleteTemplateResponse, error) { - settings, err := models.GetSettings(s.db) - if err != nil { - return nil, err - } - - if !settings.IsAlertingEnabled() { - return nil, services.ErrAlertingDisabled + if !s.Enabled() { + return nil, status.Error(codes.FailedPrecondition, services.ErrAlertingDisabled.Error()) } e := s.db.InTransactionContext(ctx, nil, func(tx *reform.TX) error { return models.RemoveTemplate(tx.Querier, req.Name) }) if e != nil { + s.l.WithError(e).Errorf("Failed to delete rule template %s", req.Name) + // e is already wrapped with status.Error in models.RemoveTemplate, so we can return it directly return nil, e } @@ -533,6 +537,11 @@ func convertTemplate(l *logrus.Entry, template models.Template) (*alerting.Templ return nil, err } + if template.Severity < math.MinInt32 || template.Severity > math.MaxInt32 { + return nil, fmt.Errorf("alerting template (name=%s) severity %d is out of range for int32", + template.Name, template.Severity) + } + t := &alerting.Template{ Name: template.Name, Summary: template.Summary, @@ -597,45 +606,45 @@ func convertParamDefinitions(l *logrus.Entry, params models.AlertExprParamsDefin // CreateRule creates alert rule from the given template. func (s *Service) CreateRule(ctx context.Context, req *alerting.CreateRuleRequest) (*alerting.CreateRuleResponse, error) { - settings, err := models.GetSettings(s.db) - if err != nil { - return nil, err - } - - if !settings.IsAlertingEnabled() { - return nil, services.ErrAlertingDisabled + if !s.Enabled() { + return nil, status.Error(codes.FailedPrecondition, services.ErrAlertingDisabled.Error()) } if req.TemplateName == "" { - return nil, status.Error(codes.InvalidArgument, "Template name should be specified.") + return nil, status.Error(codes.InvalidArgument, "Template name should be specified") } if req.FolderUid == "" { - return nil, status.Error(codes.InvalidArgument, "Folder UID should be specified.") + return nil, status.Error(codes.InvalidArgument, "Folder UID should be specified") } if req.Group == "" { - return nil, status.Error(codes.InvalidArgument, "Rule group name should be specified.") + return nil, status.Error(codes.InvalidArgument, "Rule group name should be specified") } // The datasource name is defined in `build/ansible/roles/grafana/files/datasources.yml` metricsDatasourceUID, err := s.grafanaClient.GetDatasourceUIDByName(ctx, "Metrics") if err != nil { - return nil, err + s.l.WithError(err).Error("Failed to get Metrics datasource UID.") + return nil, status.Error(codes.Internal, "Failed to get Metrics datasource UID") } - sourceTemplate, ok := s.GetTemplates()[req.TemplateName] - if !ok { - return nil, status.Errorf(codes.NotFound, "Unknown template %s.", req.TemplateName) + sourceTemplate, err := models.FindTemplateByName(s.db.WithContext(ctx), req.TemplateName) + if err != nil { + // err is already wrapped with status.Error in models.FindTemplateByName, so we can return it directly + return nil, err } paramsValues, err := convertParamsValuesToModel(req.Params) if err != nil { - return nil, err + s.l.WithError(err).Error("Failed to convert rule parameters values.") + return nil, status.Error(codes.InvalidArgument, "Failed to convert rule parameters values") } err = validateParameters(sourceTemplate.Params, paramsValues) if err != nil { + s.l.WithError(err).Error("Failed to validate rule parameters values.") + // err is already wrapped with status.Error in validateParameters, so we can return it directly return nil, err } @@ -646,7 +655,8 @@ func (s *Service) CreateRule(ctx context.Context, req *alerting.CreateRuleReques expr, err := fillExprWithParams(sourceTemplate.Expr, paramsValues.AsStringMap()) if err != nil { - return nil, fmt.Errorf("failed to fill rule expression with parameters: %w", err) + s.l.WithError(err).Error("Failed to fill rule expression with parameters.") + return nil, status.Error(codes.InvalidArgument, "Failed to fill rule expression with parameters") } for _, filter := range req.Filters { @@ -656,38 +666,43 @@ func (s *Service) CreateRule(ctx context.Context, req *alerting.CreateRuleReques case alerting.FilterType_FILTER_TYPE_MISMATCH: expr = fmt.Sprintf(`label_mismatch(%s, "%s", "%s")`, expr, filter.Label, filter.Regexp) default: - return nil, fmt.Errorf("unknown filter type: %T", filter) + return nil, status.Errorf(codes.InvalidArgument, "Unknown filter type %s.", filter.Type) } } ta, err := sourceTemplate.GetAnnotations() if err != nil { - return nil, fmt.Errorf("failed to get template annotations: %w", err) + s.l.WithError(err).Error("Failed to get template annotations.") + return nil, status.Error(codes.Internal, "Failed to get template annotations") } // Copy annotations form template annotations := make(map[string]string) err = transformMaps(ta, annotations, paramsValues.AsStringMap()) if err != nil { - return nil, fmt.Errorf("failed to fill template annotations placeholders: %w", err) + s.l.WithError(err).Error("Failed to fill template annotations placeholders.") + return nil, status.Error(codes.Internal, "Failed to fill template annotations placeholders") } labels := make(map[string]string) // Copy labels form template err = transformMaps(req.CustomLabels, labels, paramsValues.AsStringMap()) if err != nil { - return nil, fmt.Errorf("failed to fill rule labels placeholders: %w", err) + s.l.WithError(err).Error("Failed to fill rule labels placeholders.") + return nil, status.Error(codes.Internal, "Failed to fill rule labels placeholders") } tl, err := sourceTemplate.GetLabels() if err != nil { - return nil, fmt.Errorf("failed to get template labels: %w", err) + s.l.WithError(err).Error("Failed to get template labels.") + return nil, status.Error(codes.Internal, "Failed to get template labels") } // Add rule labels err = transformMaps(tl, labels, paramsValues.AsStringMap()) if err != nil { - return nil, fmt.Errorf("failed to fill template labels placeholders: %w", err) + s.l.WithError(err).Error("Failed to fill template labels placeholders.") + return nil, status.Error(codes.Internal, "Failed to fill template labels placeholders") } // Do not add volatile values like `{{ $value }}` to labels as it will break alerts identity. @@ -728,7 +743,8 @@ func (s *Service) CreateRule(ctx context.Context, req *alerting.CreateRuleReques err = s.grafanaClient.CreateAlertRule(ctx, req.FolderUid, req.Group, interval, &rule) if err != nil { - return nil, err + s.l.WithError(err).Error("Failed to create alert rule in Grafana.") + return nil, status.Error(codes.Internal, "Failed to create alert rule in Grafana") } return &alerting.CreateRuleResponse{}, nil @@ -749,7 +765,7 @@ func convertParamsValuesToModel(params []*alerting.ParamValue) (AlertExprParamsV p.Type = models.Float p.FloatValue = param.GetFloat() case alerting.ParamType_PARAM_TYPE_STRING: - p.Type = models.Float + p.Type = models.String p.StringValue = param.GetString_() default: return nil, errors.New("invalid model rule param value type") diff --git a/managed/services/alerting/service_test.go b/managed/services/alerting/service_test.go index 368b3e77310..bae829174c4 100644 --- a/managed/services/alerting/service_test.go +++ b/managed/services/alerting/service_test.go @@ -16,15 +16,25 @@ package alerting import ( + "errors" + "math" "testing" + "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + "google.golang.org/protobuf/types/known/durationpb" "gopkg.in/reform.v1" "gopkg.in/reform.v1/dialects/postgresql" alerting "github.com/percona/pmm/api/alerting/v1" + managementv1 "github.com/percona/pmm/api/management/v1" "github.com/percona/pmm/managed/models" + "github.com/percona/pmm/managed/pi/common" + "github.com/percona/pmm/managed/services" "github.com/percona/pmm/managed/utils/testdb" ) @@ -34,13 +44,34 @@ const ( testTemplates2 = "../../testdata/alerting-templates/user" ) +func enableAlerting(t *testing.T, db *reform.DB) { + t.Helper() + + // Ensure alerting is enabled for this sub-test + _, err := db.Exec(`INSERT INTO settings (settings) VALUES ('{"alerting": {"enabled": true}}') ON CONFLICT DO NOTHING`) + require.NoError(t, err) + _, err = db.Exec(`UPDATE settings SET settings = '{"alerting": {"enabled": true}}'`) + require.NoError(t, err) +} + +func disableAlerting(t *testing.T, db *reform.DB) { + t.Helper() + + // Ensure alerting is enabled for this sub-test + _, err := db.Exec(`INSERT INTO settings (settings) VALUES ('{"alerting": {"enabled": false}}') ON CONFLICT DO NOTHING`) + require.NoError(t, err) + _, err = db.Exec(`UPDATE settings SET settings = '{"alerting": {"enabled": false}}'`) + require.NoError(t, err) +} + func TestCollect(t *testing.T) { ctx := t.Context() sqlDB := testdb.Open(t, models.SkipFixtures, nil) t.Cleanup(func() { require.NoError(t, sqlDB.Close()) }) - db := reform.NewDB(sqlDB, postgresql.Dialect, reform.NewPrintfLogger(t.Logf)) + db := reform.NewDB(sqlDB, postgresql.Dialect, nil) + enableAlerting(t, db) t.Run("builtin are valid", func(t *testing.T) { t.Parallel() @@ -88,21 +119,302 @@ func TestCollect(t *testing.T) { }) } -func TestTemplateValidation(t *testing.T) { - t.Parallel() +func TestListTemplates(t *testing.T) { ctx := t.Context() sqlDB := testdb.Open(t, models.SkipFixtures, nil) t.Cleanup(func() { require.NoError(t, sqlDB.Close()) }) - db := reform.NewDB(sqlDB, postgresql.Dialect, reform.NewPrintfLogger(t.Logf)) + db := reform.NewDB(sqlDB, postgresql.Dialect, nil) svc, err := NewService(db, nil) require.NoError(t, err) - t.Run("create a template with missing param", func(t *testing.T) { - t.Parallel() + t.Run("Disabled", func(t *testing.T) { + disableAlerting(t, db) + t.Cleanup(func() { + enableAlerting(t, db) + }) + + resp, err := svc.ListTemplates(ctx, &alerting.ListTemplatesRequest{}) + require.Nil(t, resp) + require.ErrorIs(t, err, status.Error(codes.FailedPrecondition, services.ErrAlertingDisabled.Error())) + }) + + t.Run("Overflow", func(t *testing.T) { + t.Run("SeverityOverflow", func(t *testing.T) { + svc.rw.Lock() + svc.templates = map[string]models.Template{ + "overflow": { + Name: "overflow", + Severity: math.MaxInt32 + 1, + }, + } + svc.rw.Unlock() + + resp, err := svc.ListTemplates(ctx, &alerting.ListTemplatesRequest{}) + assert.Nil(t, resp) + require.ErrorContains(t, err, "alerting template (name=overflow) severity 2147483648 is out of range for int32") + }) + + t.Run("SeverityUnderflow", func(t *testing.T) { + svc.rw.Lock() + svc.templates = map[string]models.Template{ + "underflow": { + Name: "underflow", + Severity: math.MinInt32 - 1, + }, + } + svc.rw.Unlock() + + resp, err := svc.ListTemplates(ctx, &alerting.ListTemplatesRequest{}) + assert.Nil(t, resp) + require.Error(t, err) + assert.Contains(t, err.Error(), "alerting template (name=underflow) severity -2147483649 is out of range for int32") + }) + }) + + t.Run("Pagination", func(t *testing.T) { + svc.rw.Lock() + svc.templates = map[string]models.Template{ + "t1": {Name: "t1"}, + "t2": {Name: "t2"}, + "t3": {Name: "t3"}, + } + svc.rw.Unlock() + + t.Run("NegativePagination", func(t *testing.T) { + _, err := svc.ListTemplates(ctx, &alerting.ListTemplatesRequest{ + PageIndex: new(int32(-1)), + }) + require.ErrorIs(t, err, status.Errorf(codes.InvalidArgument, "Page index must be non-negative")) + + _, err = svc.ListTemplates(ctx, &alerting.ListTemplatesRequest{ + PageSize: new(int32(-1)), + }) + require.ErrorIs(t, err, status.Errorf(codes.InvalidArgument, "Page size must be non-negative")) + }) + + t.Run("PageSizeZero", func(t *testing.T) { + // PageSize 0 is handled as "return all" in the service logic + resp, err := svc.ListTemplates(ctx, &alerting.ListTemplatesRequest{ + PageSize: new(int32(0)), + }) + require.NoError(t, err) + assert.Equal(t, int32(3), resp.TotalItems) + assert.Len(t, resp.Templates, 3) + }) + + t.Run("FullPage", func(t *testing.T) { + resp, err := svc.ListTemplates(ctx, &alerting.ListTemplatesRequest{ + PageSize: new(int32(2)), + }) + require.NoError(t, err) + assert.Equal(t, int32(2), resp.TotalPages) + assert.Equal(t, int32(3), resp.TotalItems) + assert.Len(t, resp.Templates, 2) + }) + + t.Run("LastPage", func(t *testing.T) { + resp, err := svc.ListTemplates(ctx, &alerting.ListTemplatesRequest{ + PageSize: new(int32(2)), + PageIndex: new(int32(1)), + }) + require.NoError(t, err) + assert.Equal(t, int32(2), resp.TotalPages) + assert.Len(t, resp.Templates, 1) + assert.Equal(t, "t3", resp.Templates[0].Name) + }) + + t.Run("PageIndexOutOfRange", func(t *testing.T) { + resp, err := svc.ListTemplates(ctx, &alerting.ListTemplatesRequest{ + PageIndex: new(int32(10)), + PageSize: new(int32(2)), + }) + require.NoError(t, err) + assert.Empty(t, resp.Templates) + assert.Equal(t, int32(3), resp.TotalItems) + }) + }) + + t.Run("Reload", func(t *testing.T) { + enableAlerting(t, db) + + // Create a new service instance to avoid interference with the previous test's templates. + // It changes the templates in memory that affects the other tests. + // Need to test the reload functionality in isolation. + svcR, errR := NewService(db, nil) + require.NoError(t, errR) + + svcR.rw.Lock() + svcR.templates = map[string]models.Template{ + "t1": {Name: "t1"}, + "t2": {Name: "t2"}, + "t3": {Name: "t3"}, + } + svcR.rw.Unlock() + + // Inject a manual template into memory + svcR.rw.Lock() + svcR.templates = map[string]models.Template{"manual_entry": {Name: "manual_entry"}} + svcR.rw.Unlock() + + // Point to a directory with known templates + svcR.userTemplatesPath = testTemplates2 + + // Verify it's there without reload + resp, err := svcR.ListTemplates(ctx, &alerting.ListTemplatesRequest{}) + require.NoError(t, err) + assert.Len(t, resp.Templates, 1) + assert.Equal(t, "manual_entry", resp.Templates[0].Name) + + // Trigger reload + resp, err = svcR.ListTemplates(ctx, &alerting.ListTemplatesRequest{Reload: true}) + require.NoError(t, err) + + // The manual entry should be gone, replaced by templates from testTemplates2 and built-ins + assert.Greater(t, resp.TotalItems, int32(1)) + foundManual := false + for _, tmpl := range resp.Templates { + if tmpl.Name == "manual_entry" { + foundManual = true + } + } + assert.False(t, foundManual) + }) +} + +func TestCreateTemplate(t *testing.T) { + ctx := t.Context() + sqlDB := testdb.Open(t, models.SkipFixtures, nil) + t.Cleanup(func() { + require.NoError(t, sqlDB.Close()) + }) + db := reform.NewDB(sqlDB, postgresql.Dialect, nil) + enableAlerting(t, db) + + svc, err := NewService(db, nil) + require.NoError(t, err) + t.Run("Disabled", func(t *testing.T) { + disableAlerting(t, db) + + t.Cleanup(func() { + enableAlerting(t, db) + }) + + _, err = svc.CreateTemplate(ctx, &alerting.CreateTemplateRequest{Yaml: "templates: []"}) + require.ErrorIs(t, err, status.Error(codes.FailedPrecondition, services.ErrAlertingDisabled.Error())) + }) + + t.Run("InvalidYAML", func(t *testing.T) { + _, err = svc.CreateTemplate(ctx, &alerting.CreateTemplateRequest{Yaml: "invalid: ["}) + require.ErrorIs(t, err, status.Error(codes.InvalidArgument, "yaml: line 1: did not find expected node content")) + }) + + t.Run("DuplicateNamesInRequest", func(t *testing.T) { + const yaml = ` +templates: + - name: dupl_req_template + expr: 'avg(cpu_usage) > [[ .threshold ]]' + params: + - name: threshold + summary: CPU Usage Threshold + type: float + range: + - 0 + - 100 + value: 80 + for: 1m + version: 1 + summary: High CPU Usage + severity: critical + labels: + template_label: static + dynamic_label: '[[ .threshold ]]' + annotations: + summary: 'High CPU: [[ .threshold ]]' + description: 'Value is {{ $value }}' + - name: dupl_req_template + expr: 'avg(cpu_usage) > [[ .threshold ]]' + params: + - name: threshold + summary: CPU Usage Threshold + type: float + range: + - 0 + - 100 + value: 80 + for: 1m + version: 1 + summary: High CPU Usage + severity: critical + labels: + template_label: static + dynamic_label: '[[ .threshold ]]' + annotations: + summary: 'High CPU: [[ .threshold ]]' + description: 'Value is {{ $value }}' +` + _, err = svc.CreateTemplate(ctx, &alerting.CreateTemplateRequest{Yaml: yaml}) + require.ErrorIs(t, err, status.Error(codes.InvalidArgument, "Template with name 'dupl_req_template' declared more than once.")) + }) + + t.Run("DuplicateTemplate", func(t *testing.T) { + const yaml = ` +templates: + - name: dupl_test_template + expr: avg(cpu_usage) > [[ .threshold ]] + params: + - name: threshold + summary: "CPU Usage Threshold" + type: float + range: [0, 100] + value: 80 + for: 1m + version: 1 + summary: "High CPU Usage" + severity: critical + labels: + template_label: "static" + dynamic_label: "[[ .threshold ]]" + annotations: + summary: "High CPU: [[ .threshold ]]" + description: "Value is {{ $value }}"` + _, err = svc.CreateTemplate(ctx, &alerting.CreateTemplateRequest{Yaml: yaml}) + require.NoError(t, err) + assert.Contains(t, svc.GetTemplates(), "dupl_test_template") + + _, err = svc.CreateTemplate(ctx, &alerting.CreateTemplateRequest{Yaml: yaml}) + require.ErrorIs(t, err, status.Error(codes.AlreadyExists, "Template with name \"dupl_test_template\" already exists.")) + }) + + t.Run("ForbiddenPrefix", func(t *testing.T) { + const yaml = ` +templates: + - name: pmm_system_rule + expr: avg(cpu_usage) > [[ .threshold ]] + params: + - name: threshold + summary: "CPU Usage Threshold" + type: float + range: [0, 100] + value: 80 + for: 1m + version: 1 + summary: "High CPU Usage" + severity: critical + labels: + template_label: "static" + dynamic_label: "[[ .threshold ]]" + annotations: + summary: "High CPU: [[ .threshold ]]" + description: "Value is {{ $value }}"` + _, err = svc.CreateTemplate(ctx, &alerting.CreateTemplateRequest{Yaml: yaml}) + require.ErrorIs(t, err, status.Error(codes.InvalidArgument, "pmm_system_rule: template name should not start with 'pmm_' or 'saas_' prefix")) + }) + + t.Run("MissingParam", func(t *testing.T) { const templateWithMissingParam = ` --- templates: @@ -143,16 +455,108 @@ templates: }) assert.Nil(t, resp) require.EqualError(t, err, "rpc error: code = InvalidArgument desc = failed to fill expression "+ - "placeholders: template: :4:5: executing \"\" at <.threshold>: map has no entry for key \"threshold\".") + "placeholders: template: :4:5: executing \"\" at <.threshold>: map has no entry for key \"threshold\"") + }) + + t.Run("Success", func(t *testing.T) { + const yaml = ` +templates: + - name: create_test_template + expr: avg(cpu_usage) > [[ .threshold ]] + params: + - name: threshold + summary: "CPU Usage Threshold" + type: float + range: [0, 100] + value: 80 + for: 1m + version: 1 + summary: "High CPU Usage" + severity: critical + labels: + template_label: "static" + dynamic_label: "[[ .threshold ]]" + annotations: + summary: "High CPU: [[ .threshold ]]" + description: "Value is {{ $value }}"` + + _, err = svc.CreateTemplate(ctx, &alerting.CreateTemplateRequest{Yaml: yaml}) + require.NoError(t, err) + assert.Contains(t, svc.GetTemplates(), "create_test_template") + }) +} + +func TestUpdateTemplate(t *testing.T) { + ctx := t.Context() + sqlDB := testdb.Open(t, models.SkipFixtures, nil) + t.Cleanup(func() { + require.NoError(t, sqlDB.Close()) + }) + db := reform.NewDB(sqlDB, postgresql.Dialect, nil) + enableAlerting(t, db) + + svc, err := NewService(db, nil) + require.NoError(t, err) + + t.Run("Disabled", func(t *testing.T) { + disableAlerting(t, db) + + t.Cleanup(func() { + enableAlerting(t, db) + }) + + _, err = svc.UpdateTemplate(ctx, &alerting.UpdateTemplateRequest{Name: "crud_test_template", Yaml: "templates: []"}) + require.ErrorIs(t, err, status.Error(codes.FailedPrecondition, services.ErrAlertingDisabled.Error())) + }) + + t.Run("EmptyTemplate", func(t *testing.T) { + _, err = svc.UpdateTemplate(ctx, &alerting.UpdateTemplateRequest{Name: "empty_internal_name", Yaml: "templates: []"}) + require.ErrorIs(t, err, status.Error(codes.InvalidArgument, "Request should contain exactly one rule template")) + }) + + t.Run("NotFound", func(t *testing.T) { + const yaml = ` +templates: + - name: not_found_internal_name + expr: avg(cpu_usage) > [[ .threshold ]] + params: + - name: threshold + summary: "CPU Usage Threshold" + type: float + range: [0, 100] + value: 80 + for: 1m + version: 1 + summary: "High CPU Usage" + severity: critical + labels: + template_label: "static" + dynamic_label: "[[ .threshold ]]" + annotations: + summary: "High CPU: [[ .threshold ]]" + description: "Value is {{ $value }}"` + _, err = svc.UpdateTemplate(ctx, &alerting.UpdateTemplateRequest{Name: "not_found_internal_name", Yaml: yaml}) + require.ErrorIs(t, err, status.Error(codes.NotFound, "Template with name \"not_found_internal_name\" not found.")) + }) + + t.Run("MultipleTemplatesError", func(t *testing.T) { + const yaml = ` +templates: + - name: t1 + expr: metric > 1 + - name: t2 + expr: metric > 2` + _, err = svc.UpdateTemplate(ctx, &alerting.UpdateTemplateRequest{Name: "crud_test_template", Yaml: yaml}) + require.ErrorIs(t, err, status.Error(codes.InvalidArgument, "Failed to parse rule template")) }) - t.Run("update valid template with a template with missing param", func(t *testing.T) { + t.Run("MissingParam", func(t *testing.T) { t.Parallel() const validTemplate = ` --- templates: - - name: valid_template_1 + - name: valid_template_11 version: 1 summary: Valid template 1 expr: |- @@ -240,4 +644,281 @@ templates: require.EqualError(t, err, "rpc error: code = InvalidArgument desc = failed to fill expression "+ "placeholders: template: :4:5: executing \"\" at <.threshold>: map has no entry for key \"threshold\".") }) + + t.Run("Success", func(t *testing.T) { + const yaml = ` +templates: + - name: update_test_template + expr: avg(cpu_usage) > [[ .threshold ]] + params: + - name: threshold + summary: "CPU Usage Threshold" + type: float + range: [0, 100] + value: 80 + for: 1m + version: 1 + summary: "High CPU Usage" + severity: critical + labels: + template_label: "static" + dynamic_label: "[[ .threshold ]]" + annotations: + summary: "High CPU: [[ .threshold ]]" + description: "Value is {{ $value }}"` + + const newYaml = ` +templates: + - name: update_test_template + expr: avg(cpu_usage) > [[ .threshold ]] + params: + - name: threshold + summary: "CPU Usage Threshold" + type: float + range: [0, 100] + value: 80 + for: 5m + version: 1 + summary: "Updated Summary Text" + severity: critical + labels: + template_label: "static" + dynamic_label: "[[ .threshold ]]" + annotations: + summary: "High CPU: [[ .threshold ]]" + description: "Value is {{ $value }}"` + + _, err = svc.CreateTemplate(ctx, &alerting.CreateTemplateRequest{Yaml: yaml}) + require.NoError(t, err) + assert.Contains(t, svc.GetTemplates(), "update_test_template") + + _, err = svc.UpdateTemplate(ctx, &alerting.UpdateTemplateRequest{ + Name: "update_test_template", + Yaml: newYaml, + }) + require.NoError(t, err) + + templates := svc.GetTemplates() + assert.Contains(t, templates, "update_test_template") + assert.Equal(t, "Updated Summary Text", templates["update_test_template"].Summary) + }) +} + +func TestDeleteTemplate(t *testing.T) { + ctx := t.Context() + sqlDB := testdb.Open(t, models.SkipFixtures, nil) + t.Cleanup(func() { + require.NoError(t, sqlDB.Close()) + }) + db := reform.NewDB(sqlDB, postgresql.Dialect, nil) + enableAlerting(t, db) + + svc, err := NewService(db, nil) + require.NoError(t, err) + + t.Run("Disabled", func(t *testing.T) { + disableAlerting(t, db) + + t.Cleanup(func() { + enableAlerting(t, db) + }) + + _, err = svc.DeleteTemplate(ctx, &alerting.DeleteTemplateRequest{Name: "updated_internal_name"}) + require.ErrorIs(t, err, status.Error(codes.FailedPrecondition, services.ErrAlertingDisabled.Error())) + }) + + t.Run("NotFound", func(t *testing.T) { + _, err = svc.DeleteTemplate(ctx, &alerting.DeleteTemplateRequest{Name: "not_found_internal_name"}) + require.ErrorIs(t, err, status.Error(codes.NotFound, "Template with name \"not_found_internal_name\" not found.")) + }) + + t.Run("Success", func(t *testing.T) { + const yaml = ` +templates: + - name: delete_test_template + expr: avg(cpu_usage) > [[ .threshold ]] + params: + - name: threshold + summary: "CPU Usage Threshold" + type: float + range: [0, 100] + value: 80 + for: 1m + version: 1 + summary: "High CPU Usage" + severity: critical + labels: + template_label: "static" + dynamic_label: "[[ .threshold ]]" + annotations: + summary: "High CPU: [[ .threshold ]]" + description: "Value is {{ $value }}"` + + _, err = svc.CreateTemplate(ctx, &alerting.CreateTemplateRequest{Yaml: yaml}) + require.NoError(t, err) + assert.Contains(t, svc.GetTemplates(), "delete_test_template") + + _, err = svc.DeleteTemplate(ctx, &alerting.DeleteTemplateRequest{Name: "delete_test_template"}) + require.NoError(t, err) + assert.NotContains(t, svc.GetTemplates(), "delete_test_template") + }) +} + +func TestCreateRule(t *testing.T) { + ctx := t.Context() + sqlDB := testdb.Open(t, models.SkipFixtures, nil) + t.Cleanup(func() { + require.NoError(t, sqlDB.Close()) + }) + db := reform.NewDB(sqlDB, postgresql.Dialect, nil) + enableAlerting(t, db) + + mockGrafana := &mockGrafanaClient{} + svc, err := NewService(db, mockGrafana) + require.NoError(t, err) + + // Setup a base template for rule testing + const yaml = ` +templates: + - name: rule_logic_template + expr: avg(cpu_usage) > [[ .threshold ]] + params: + - name: threshold + summary: "CPU Usage Threshold" + type: float + range: [0, 100] + value: 80 + for: 1m + version: 1 + summary: "High CPU Usage" + severity: critical + labels: + template_label: "static" + dynamic_label: "[[ .threshold ]]" + annotations: + summary: "High CPU: [[ .threshold ]]" + description: "Value is {{ $value }}"` + + _, err = svc.CreateTemplate(ctx, &alerting.CreateTemplateRequest{Yaml: yaml}) + require.NoError(t, err) + + t.Run("InputValidation", func(t *testing.T) { + type testCase struct { + name string + req *alerting.CreateRuleRequest + err string + } + cases := []testCase{ + {"NoTemplate", &alerting.CreateRuleRequest{}, "Template name should be specified"}, + {"NoFolder", &alerting.CreateRuleRequest{TemplateName: "t"}, "Folder UID should be specified"}, + {"NoGroup", &alerting.CreateRuleRequest{TemplateName: "t", FolderUid: "f"}, "Rule group name should be specified"}, + {"CountMismatch", &alerting.CreateRuleRequest{ + TemplateName: "rule_logic_template", + FolderUid: "f", + Group: "g", + Params: []*alerting.ParamValue{}, + }, + "Expression requires 1 parameters, but got 0.", + }, + {"ParamsMismatch", &alerting.CreateRuleRequest{ + TemplateName: "rule_logic_template", + FolderUid: "f", + Group: "g", + Params: []*alerting.ParamValue{ + {Name: "threshold", + Value: &alerting.ParamValue_Float{ + Float: 101, + }, + Type: alerting.ParamType_PARAM_TYPE_FLOAT, + }}, + }, + "Parameter threshold value is greater than required maximum."}, + } + mockGrafana.On("GetDatasourceUIDByName", mock.Anything, "Metrics").Return("ds-123", nil).Maybe() + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // Setup expectation as optional to prevent panics on unexpected validation paths + + _, err := svc.CreateRule(ctx, tc.req) + require.ErrorIs(t, err, status.Error(codes.InvalidArgument, tc.err)) + }) + } + mockGrafana.AssertExpectations(t) + + cases = []testCase{ + {"UnknownTemplate", &alerting.CreateRuleRequest{TemplateName: "ghost", FolderUid: "f", Group: "g"}, "Template with name \"ghost\" not found."}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := svc.CreateRule(ctx, tc.req) + require.ErrorIs(t, err, status.Error(codes.NotFound, tc.err)) + }) + } + }) + + t.Run("GrafanaIntegrationErrors", func(t *testing.T) { + t.Run("DatasourceLookupFail", func(t *testing.T) { + mockGrafana.On("GetDatasourceUIDByName", mock.Anything, "Metrics"). + Return("", errors.New("grafana connection refused")) + _, err := svc.CreateRule(ctx, &alerting.CreateRuleRequest{ + TemplateName: "rule_logic_template", + FolderUid: "f", + Group: "g", + }) + require.ErrorIs(t, err, status.Error(codes.InvalidArgument, "Expression requires 1 parameters, but got 0.")) + mockGrafana.AssertExpectations(t) + }) + + t.Run("CreateRuleFail", func(t *testing.T) { + mockGrafana.On("GetDatasourceUIDByName", mock.Anything, "Metrics"). + Return("ds-123", nil) + + _, err := svc.CreateRule(ctx, &alerting.CreateRuleRequest{ + TemplateName: "rule_logic_template", + FolderUid: "f", + Group: "g", + }) + require.ErrorIs(t, err, status.Error(codes.InvalidArgument, "Expression requires 1 parameters, but got 0.")) + mockGrafana.AssertExpectations(t) + }) + }) + + t.Run("Success", func(t *testing.T) { + var capturedRule *services.Rule + mockGrafana.On("GetDatasourceUIDByName", mock.Anything, "Metrics"). + Return("ds-123", nil) + mockGrafana.On("CreateAlertRule", mock.Anything, "my-folder", "my-group", "30s", mock.MatchedBy(func(r *services.Rule) bool { + capturedRule = r + return true + })). + Return(nil) + + _, err := svc.CreateRule(ctx, &alerting.CreateRuleRequest{ + Name: "Complex Rule", + TemplateName: "rule_logic_template", + FolderUid: "my-folder", + Group: "my-group", + Interval: durationpb.New(30 * time.Second), + For: durationpb.New(2 * time.Minute), + Params: []*alerting.ParamValue{ + {Name: "threshold", Value: &alerting.ParamValue_Float{Float: 92.5}, Type: alerting.ParamType_PARAM_TYPE_FLOAT}, + }, + Filters: []*alerting.Filter{ + {Type: alerting.FilterType_FILTER_TYPE_MATCH, Label: "service", Regexp: "web"}, + {Type: alerting.FilterType_FILTER_TYPE_MISMATCH, Label: "env", Regexp: "test"}, + }, + CustomLabels: map[string]string{"user_label": "val"}, + Severity: managementv1.Severity(common.Critical), + }) + + require.NoError(t, err) + require.NotNil(t, capturedRule) + assert.Equal(t, "Complex Rule", capturedRule.GrafanaAlert.Title) + assert.Equal(t, "label_mismatch(label_match(avg(cpu_usage) > 92.5, \"service\", \"web\"), \"env\", \"test\")", capturedRule.GrafanaAlert.Data[0].Model.Expr) + assert.Equal(t, "2m0s", capturedRule.For) + assert.Equal(t, "val", capturedRule.Labels["user_label"]) + assert.Equal(t, "92.5", capturedRule.Labels["dynamic_label"]) + assert.Equal(t, "High CPU: 92.5", capturedRule.Annotations["summary"]) + mockGrafana.AssertExpectations(t) + }) } diff --git a/managed/services/backup/pitr_timerange_service.go b/managed/services/backup/pitr_timerange_service.go index 6b28a1be925..ce3ab448e9b 100644 --- a/managed/services/backup/pitr_timerange_service.go +++ b/managed/services/backup/pitr_timerange_service.go @@ -19,6 +19,7 @@ import ( "context" "errors" "fmt" + "math" "path" "sort" "strconv" @@ -231,10 +232,15 @@ func pitrParseTS(tstr string) *primitive.Timestamp { // just skip this file return nil } - ts := primitive.Timestamp{T: uint32(t.Unix())} + unix := t.Unix() + if unix < 0 || unix > math.MaxUint32 { + return nil + } + ts := primitive.Timestamp{T: uint32(unix)} + if len(tparts) > 1 { - ti, err := strconv.Atoi(tparts[1]) - if err != nil { + ti, err := strconv.ParseUint(tparts[1], 10, 32) + if err != nil || ti > math.MaxUint32 { // just skip this file return nil } diff --git a/managed/services/backup/pitr_timerange_service_test.go b/managed/services/backup/pitr_timerange_service_test.go index a3a6099d266..4cdb705f172 100644 --- a/managed/services/backup/pitr_timerange_service_test.go +++ b/managed/services/backup/pitr_timerange_service_test.go @@ -70,6 +70,11 @@ func TestPitrMetaFromFileName(t *testing.T) { EndTS: primitive.Timestamp{T: uint32(1661774744), I: 10}, }, }, + { + name: "timestamp overflow in filename", + filename: "rs0/20220829/21060207062816-1.20220829120544-10.oplog.s2", + expected: nil, + }, } prefix := path.Join("test_artifact_name", pitrFSPrefix) @@ -165,6 +170,21 @@ func TestPitrParseTs(t *testing.T) { filename: "2022", expected: nil, }, + { + name: "timestamp overflow", + filename: "21060207062816", // math.MaxUint32 + 1 + expected: nil, + }, + { + name: "timestamp underflow", + filename: "19691231235959", // unix -1 + expected: nil, + }, + { + name: "index overflow", + filename: "20220829115611-4294967296", // math.MaxUint32 + 1 + expected: nil, + }, } for _, tt := range tests { diff --git a/managed/services/management/backup/backup_service.go b/managed/services/management/backup/backup_service.go index 24ca00656d4..b2959885326 100644 --- a/managed/services/management/backup/backup_service.go +++ b/managed/services/management/backup/backup_service.go @@ -19,6 +19,7 @@ import ( "context" "errors" "fmt" + "math" "path/filepath" "regexp" "strings" @@ -466,6 +467,15 @@ func (s *BackupService) GetLogs(_ context.Context, req *backupv1.GetLogsRequest) res.End = true break } + + if log.ChunkID < 0 || log.ChunkID > math.MaxUint32 { + s.l.WithFields(logrus.Fields{ + "chunk_id": log.ChunkID, + "job_id": jobs[0].ID, + }).Warn("Chunk ID is out of uint32 range, skipping record") + continue + } + res.Logs = append(res.Logs, &backupv1.LogChunk{ ChunkId: uint32(log.ChunkID), Data: log.Data, diff --git a/managed/services/management/backup/backup_service_test.go b/managed/services/management/backup/backup_service_test.go index b29b7fdb465..8f3f93c07bd 100644 --- a/managed/services/management/backup/backup_service_test.go +++ b/managed/services/management/backup/backup_service_test.go @@ -17,6 +17,7 @@ package backup import ( "fmt" + "math" "testing" "time" @@ -429,7 +430,7 @@ func TestScheduledBackups(t *testing.T) { func TestGetLogs(t *testing.T) { ctx := t.Context() sqlDB := testdb.Open(t, models.SkipFixtures, nil) - db := reform.NewDB(sqlDB, postgresql.Dialect, reform.NewPrintfLogger(t.Logf)) + db := reform.NewDB(sqlDB, postgresql.Dialect, nil) backupService := &mockBackupService{} schedulerService := &mockScheduleService{} mockedPbmPITRService := &mockPbmPITRService{} @@ -502,6 +503,47 @@ func TestGetLogs(t *testing.T) { assert.Equal(t, tc.expect, chunkIDs) } }) + + t.Run("skip out of range chunk IDs", func(t *testing.T) { + artifactID := uuid.New().String() + job, err := models.CreateJob(db.Querier, models.CreateJobParams{ + PMMAgentID: "agent", + Type: models.MongoDBBackupJob, + Data: &models.JobData{ + MongoDBBackup: &models.MongoDBBackupJobData{ + ServiceID: "svc", + ArtifactID: artifactID, + }, + }, + }) + require.NoError(t, err) + + // Create logs with IDs that should be skipped and IDs that are valid. + testChunks := []int{ + -1, // Invalid: negative + 0, // Valid: lower bound + math.MaxInt32, // Valid: upper bound for PostgreSQL integer type + } + + for _, chunkID := range testChunks { + _, err = models.CreateJobLog(db.Querier, models.CreateJobLogParams{ + JobID: job.ID, + ChunkID: chunkID, + Data: fmt.Sprintf("data for %d", chunkID), + }) + require.NoError(t, err) + } + + logs, err := backupSvc.GetLogs(ctx, &backupv1.GetLogsRequest{ArtifactId: artifactID}) + require.NoError(t, err) + + expectedIDs := []uint32{0, math.MaxInt32} + actualIDs := make([]uint32, 0, len(logs.Logs)) + for _, log := range logs.Logs { + actualIDs = append(actualIDs, log.ChunkId) + } + assert.Equal(t, expectedIDs, actualIDs, "Only valid uint32 ChunkIDs should be returned") + }) } func TestListPitrTimeranges(t *testing.T) { diff --git a/managed/services/management/backup/restore_service.go b/managed/services/management/backup/restore_service.go index 453ee91d561..f9cc534ba44 100644 --- a/managed/services/management/backup/restore_service.go +++ b/managed/services/management/backup/restore_service.go @@ -19,6 +19,7 @@ package backup import ( "context" "fmt" + "math" "github.com/sirupsen/logrus" "google.golang.org/grpc/codes" @@ -165,6 +166,17 @@ func (s *RestoreService) GetLogs(_ context.Context, req *backupv1.RestoreService res.End = true break } + + // G115: integer overflow conversion int -> uint32 (gosec). + // Ensure ChunkID is within the valid range for uint32 before casting. + if log.ChunkID < 0 || log.ChunkID > math.MaxUint32 { + s.l.WithFields(logrus.Fields{ + "chunk_id": log.ChunkID, + "job_id": jobs[0].ID, + }).Warn("Chunk ID is out of uint32 range, skipping record") + continue + } + res.Logs = append(res.Logs, &backupv1.LogChunk{ ChunkId: uint32(log.ChunkID), Data: log.Data, diff --git a/managed/services/management/backup/restore_service_test.go b/managed/services/management/backup/restore_service_test.go index 0399b16d333..fb7a0d67260 100644 --- a/managed/services/management/backup/restore_service_test.go +++ b/managed/services/management/backup/restore_service_test.go @@ -17,6 +17,7 @@ package backup import ( "fmt" + "math" "testing" "github.com/google/uuid" @@ -38,7 +39,7 @@ func TestRestoreServiceGetLogs(t *testing.T) { ctx := t.Context() sqlDB := testdb.Open(t, models.SkipFixtures, nil) - db := reform.NewDB(sqlDB, postgresql.Dialect, reform.NewPrintfLogger(t.Logf)) + db := reform.NewDB(sqlDB, postgresql.Dialect, nil) backupService := &mockBackupService{} scheduleService := &mockScheduleService{} restoreSvc := NewRestoreService(db, backupService, scheduleService) @@ -149,6 +150,53 @@ func TestRestoreServiceGetLogs(t *testing.T) { assert.Equal(t, tc.expect, chunkIDs) } }) + + t.Run("skip invalid chunk IDs", func(t *testing.T) { + restoreID := uuid.New().String() + job, err := models.CreateJob(db.Querier, models.CreateJobParams{ + PMMAgentID: "agent", + Type: models.MongoDBBackupJob, + Data: &models.JobData{ + MongoDBRestoreBackup: &models.MongoDBRestoreBackupJobData{ + ServiceID: "svc", + RestoreID: restoreID, + DataModel: models.PhysicalDataModel, + }, + }, + }) + require.NoError(t, err) + + // Create logs with various IDs to test bounds checking in GetLogs + testLogs := []struct { + chunkID int + }{ + {chunkID: 10}, // Valid + {chunkID: -1}, // Invalid (negative) + {chunkID: math.MaxInt32}, // Valid (boundary of signed 32-bit int) + } + + for _, tl := range testLogs { + _, err = models.CreateJobLog(db.Querier, models.CreateJobLogParams{ + JobID: job.ID, + ChunkID: tl.chunkID, + Data: "not important", + }) + require.NoError(t, err) + } + + logs, err := restoreSvc.GetLogs(ctx, &backupv1.RestoreServiceGetLogsRequest{ + RestoreId: restoreID, + }) + require.NoError(t, err) + + chunkIDs := make([]uint32, 0, len(logs.Logs)) + for _, log := range logs.Logs { + chunkIDs = append(chunkIDs, log.ChunkId) + } + + // Only the valid IDs (10 and MaxInt32) should be present in the response + assert.Equal(t, []uint32{10, uint32(math.MaxInt32)}, chunkIDs) + }) } func TestRestoreBackupErrors(t *testing.T) { diff --git a/managed/services/management/checks.go b/managed/services/management/checks.go index da6b1582a3a..41bb2ac9eda 100644 --- a/managed/services/management/checks.go +++ b/managed/services/management/checks.go @@ -20,9 +20,9 @@ import ( "errors" "fmt" "maps" + "math" "strings" - "github.com/AlekSi/pointer" "github.com/sirupsen/logrus" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -58,7 +58,7 @@ func (s *ChecksAPIService) ListFailedServices(ctx context.Context, _ *advisorsv1 return nil, status.Errorf(codes.FailedPrecondition, "%v.", err) } - return nil, fmt.Errorf("failed to get check results: %w", err) + return nil, status.Errorf(codes.Internal, "Failed to get check results: %v", err) } summaries := make(map[string]*services.CheckResultSummary) @@ -124,12 +124,56 @@ func (s *ChecksAPIService) GetFailedChecks(ctx context.Context, req *advisorsv1. return nil, fmt.Errorf("failed to get check results for service '%s': %w", req.ServiceId, err) } - failedChecks := make([]*advisorsv1.CheckResult, 0, len(results)) - for _, result := range results { + var pageIndex, pageSize int + if req.PageIndex != nil { + pageIndex = int(*req.PageIndex) + if pageIndex < 0 { + return nil, errors.New("page index must be non-negative") + } + } + if req.PageSize != nil { + pageSize = int(*req.PageSize) + if pageSize < 0 { + return nil, errors.New("page size must be non-negative") + } + } + + totalItems := len(results) + // return value type is int32, so we need to limit it to max int32 value + if totalItems > math.MaxInt32 { + s.l.Warnf("Total checks count %d is greater than max int32 value, limiting to %d.", totalItems, math.MaxInt32) + totalItems = math.MaxInt32 + } + + totalPages := 1 + if pageSize > 0 { + totalPages = totalItems / pageSize + if totalItems%pageSize > 0 { + totalPages++ + } + // return value type is int32, so we need to limit it to max int32 value + if totalPages > math.MaxInt32 { + totalPages = math.MaxInt32 + } + } + + from := min(pageIndex*pageSize, totalItems) + to := totalItems + if pageSize > 0 { + to = min(from+pageSize, totalItems) + } + + // Avoid redundant allocations by only processing the requested page slice. + failedChecks := make([]*advisorsv1.CheckResult, 0, to-from) + for _, result := range results[from:to] { labels := make(map[string]string, len(result.Target.Labels)+len(result.Result.Labels)) maps.Copy(labels, result.Result.Labels) maps.Copy(labels, result.Target.Labels) + if result.Result.Severity < math.MinInt32 || result.Result.Severity > math.MaxInt32 { + return nil, fmt.Errorf("check result severity %d is out of range for int32", result.Result.Severity) + } + failedChecks = append(failedChecks, &advisorsv1.CheckResult{ Summary: result.Result.Summary, CheckName: result.CheckName, @@ -142,36 +186,10 @@ func (s *ChecksAPIService) GetFailedChecks(ctx context.Context, req *advisorsv1. }) } - var pageIndex, pageSize int - totalPages := int32(1) - totalItems := int32(len(failedChecks)) - - if req.PageIndex != nil { - pageIndex = int(pointer.GetInt32(req.PageIndex)) - } - if req.PageSize != nil { - pageSize = int(pointer.GetInt32(req.PageSize)) - } - - from, to := pageIndex*pageSize, (pageIndex+1)*pageSize - if to > len(failedChecks) || to == 0 { - to = len(failedChecks) - } - if from > len(failedChecks) { - from = len(failedChecks) - } - - if pageSize > 0 { - totalPages = int32(len(failedChecks) / pageSize) - if len(failedChecks)%pageSize > 0 { - totalPages++ - } - } - return &advisorsv1.GetFailedChecksResponse{ - Results: failedChecks[from:to], - TotalItems: totalItems, - TotalPages: totalPages, + Results: failedChecks, + TotalItems: int32(totalItems), + TotalPages: int32(totalPages), }, nil } diff --git a/managed/services/management/checks_test.go b/managed/services/management/checks_test.go index 4b7df573ab9..c95eda816a5 100644 --- a/managed/services/management/checks_test.go +++ b/managed/services/management/checks_test.go @@ -18,6 +18,7 @@ package management import ( "errors" "fmt" + "math" "testing" "github.com/stretchr/testify/assert" @@ -44,6 +45,7 @@ func TestStartAdvisorChecks(t *testing.T) { resp, err := s.StartAdvisorChecks(t.Context(), &advisorsv1.StartAdvisorChecksRequest{}) require.EqualError(t, err, "failed to start advisor checks: random error") assert.Nil(t, resp) + checksService.AssertExpectations(t) }) t.Run("Advisors disabled error", func(t *testing.T) { @@ -55,6 +57,7 @@ func TestStartAdvisorChecks(t *testing.T) { resp, err := s.StartAdvisorChecks(t.Context(), &advisorsv1.StartAdvisorChecksRequest{}) tests.AssertGRPCError(t, status.New(codes.FailedPrecondition, "advisor checks are disabled."), err) assert.Nil(t, resp) + checksService.AssertExpectations(t) }) } @@ -75,6 +78,7 @@ func TestGetFailedChecks(t *testing.T) { }) require.EqualError(t, err, fmt.Sprintf("failed to get check results for service '%s': random error", serviceID)) assert.Nil(t, resp) + checksService.AssertExpectations(t) }) t.Run("Advisors disabled error", func(t *testing.T) { @@ -90,6 +94,7 @@ func TestGetFailedChecks(t *testing.T) { }) tests.AssertGRPCError(t, status.New(codes.FailedPrecondition, "advisor checks are disabled."), err) assert.Nil(t, resp) + checksService.AssertExpectations(t) }) t.Run("get failed checks for requested service", func(t *testing.T) { @@ -134,6 +139,7 @@ func TestGetFailedChecks(t *testing.T) { }) require.NoError(t, err) assert.Equal(t, response, resp) + checksService.AssertExpectations(t) }) t.Run("get failed checks with pagination", func(t *testing.T) { @@ -202,6 +208,218 @@ func TestGetFailedChecks(t *testing.T) { }) require.NoError(t, err) assert.Equal(t, response, resp) + checksService.AssertExpectations(t) + }) + + t.Run("get failed checks pagination calculations", func(t *testing.T) { + t.Parallel() + + // Setup 5 results: with page size 2, we expect 3 pages + checkResult := make([]services.CheckResult, 5) + for i := range checkResult { + checkResult[i].CheckName = fmt.Sprintf("check_%d", i) + checkResult[i].Target = services.Target{ServiceName: "svc", ServiceID: "test_svc"} + } + + var checksService mockChecksService + checksService.On("GetChecksResults", mock.Anything, mock.Anything).Return(checkResult, nil) + + s := NewChecksAPIService(&checksService) + + resp, err := s.GetFailedChecks(t.Context(), &advisorsv1.GetFailedChecksRequest{ + ServiceId: "test_svc", + PageSize: new(int32(2)), + PageIndex: new(int32(2)), // Requesting the 3rd page (index 2) + }) + require.NoError(t, err) + assert.Equal(t, int32(5), resp.TotalItems) + assert.Equal(t, int32(3), resp.TotalPages) + assert.Len(t, resp.Results, 1) // Only one item left for the last page + assert.Equal(t, "check_4", resp.Results[0].CheckName) + checksService.AssertExpectations(t) + }) + + t.Run("get failed checks with zero items", func(t *testing.T) { + t.Parallel() + + var checksService mockChecksService + checksService.On("GetChecksResults", mock.Anything, mock.Anything).Return(nil, nil) + + s := NewChecksAPIService(&checksService) + + resp, err := s.GetFailedChecks(t.Context(), &advisorsv1.GetFailedChecksRequest{ + ServiceId: "test_svc", + PageSize: new(int32(10)), + }) + require.NoError(t, err) + assert.Equal(t, int32(0), resp.TotalItems) + // When pageSize is provided, totalPages is calculated from results (0/10 = 0) + assert.Equal(t, int32(0), resp.TotalPages) + assert.Empty(t, resp.Results) + checksService.AssertExpectations(t) + }) + + t.Run("get failed checks with nil pagination", func(t *testing.T) { + t.Parallel() + + checkResult := []services.CheckResult{ + {CheckName: "check1", Target: services.Target{ServiceName: "svc", ServiceID: "test_svc"}}, + {CheckName: "check2", Target: services.Target{ServiceName: "svc", ServiceID: "test_svc"}}, + } + var checksService mockChecksService + checksService.On("GetChecksResults", mock.Anything, "test_svc").Return(checkResult, nil) + + s := NewChecksAPIService(&checksService) + + resp, err := s.GetFailedChecks(t.Context(), &advisorsv1.GetFailedChecksRequest{ + ServiceId: "test_svc", + PageIndex: nil, + PageSize: nil, + }) + require.NoError(t, err) + assert.Equal(t, int32(2), resp.TotalItems) + assert.Equal(t, int32(1), resp.TotalPages) + assert.Len(t, resp.Results, 2) + checksService.AssertExpectations(t) + }) + + t.Run("get failed checks with zero page size", func(t *testing.T) { + t.Parallel() + + checkResult := []services.CheckResult{ + {CheckName: "check1", Target: services.Target{ServiceName: "svc", ServiceID: "test_svc"}}, + } + var checksService mockChecksService + checksService.On("GetChecksResults", mock.Anything, "test_svc").Return(checkResult, nil) + + s := NewChecksAPIService(&checksService) + + resp, err := s.GetFailedChecks(t.Context(), &advisorsv1.GetFailedChecksRequest{ + ServiceId: "test_svc", + PageSize: new(int32(0)), + }) + require.NoError(t, err) + assert.Equal(t, int32(1), resp.TotalItems) + assert.Len(t, resp.Results, 1) + checksService.AssertExpectations(t) + }) + + t.Run("get failed checks with page index out of bounds", func(t *testing.T) { + t.Parallel() + + checkResult := []services.CheckResult{ + {CheckName: "check1", Target: services.Target{ServiceName: "svc", ServiceID: "test_svc"}}, + } + var checksService mockChecksService + checksService.On("GetChecksResults", mock.Anything, "test_svc").Return(checkResult, nil) + + s := NewChecksAPIService(&checksService) + + resp, err := s.GetFailedChecks(t.Context(), &advisorsv1.GetFailedChecksRequest{ + ServiceId: "test_svc", + PageIndex: new(int32(100)), + PageSize: new(int32(10)), + }) + require.NoError(t, err) + assert.Equal(t, int32(1), resp.TotalItems) + assert.Empty(t, resp.Results) + checksService.AssertExpectations(t) + }) + + t.Run("get failed checks with negative page index", func(t *testing.T) { + t.Parallel() + + checkResult := []services.CheckResult{ + {CheckName: "check1", Target: services.Target{ServiceName: "svc", ServiceID: "test_svc"}}, + } + var checksService mockChecksService + checksService.On("GetChecksResults", mock.Anything, "test_svc").Return(checkResult, nil) + + s := NewChecksAPIService(&checksService) + + // Negative page index/size should be clamped to 0. PageSize 0 means return "all". + resp, err := s.GetFailedChecks(t.Context(), &advisorsv1.GetFailedChecksRequest{ + ServiceId: "test_svc", + PageIndex: new(int32(-1)), + PageSize: new(int32(10)), + }) + require.ErrorContains(t, err, "page index must be non-negative") + require.Nil(t, resp) + checksService.AssertExpectations(t) + }) + + t.Run("get failed checks with negative page size", func(t *testing.T) { + t.Parallel() + + checkResult := []services.CheckResult{ + {CheckName: "check1", Target: services.Target{ServiceName: "svc", ServiceID: "test_svc"}}, + } + var checksService mockChecksService + checksService.On("GetChecksResults", mock.Anything, "test_svc").Return(checkResult, nil) + + s := NewChecksAPIService(&checksService) + + // Negative page index/size should be clamped to 0. PageSize 0 means return "all". + resp, err := s.GetFailedChecks(t.Context(), &advisorsv1.GetFailedChecksRequest{ + ServiceId: "test_svc", + PageIndex: new(int32(1)), + PageSize: new(int32(-10)), + }) + require.ErrorContains(t, err, "page size must be non-negative") + require.Nil(t, resp) + checksService.AssertExpectations(t) + }) + + t.Run("get failed checks with pagination overflow", func(t *testing.T) { + t.Parallel() + + checkResult := []services.CheckResult{ + {CheckName: "check1", Target: services.Target{ServiceName: "svc", ServiceID: "test_svc"}}, + } + var checksService mockChecksService + checksService.On("GetChecksResults", mock.Anything, "test_svc").Return(checkResult, nil) + + s := NewChecksAPIService(&checksService) + + // Extremely large values should not cause a panic and should be handled by wider integer math. + resp, err := s.GetFailedChecks(t.Context(), &advisorsv1.GetFailedChecksRequest{ + ServiceId: "test_svc", + PageIndex: new(int32(math.MaxInt32)), + PageSize: new(int32(math.MaxInt32)), + }) + require.NoError(t, err) + assert.Equal(t, int32(1), resp.TotalItems) + assert.Empty(t, resp.Results) + checksService.AssertExpectations(t) + }) + + t.Run("get failed checks with severity overflow", func(t *testing.T) { + t.Parallel() + + // Simulate a check result with a severity value that exceeds int32 range + // to test the clamping logic in the service. + checkResult := []services.CheckResult{ + { + Result: check.Result{ + Summary: "Overflow severity check", + Severity: common.Severity(math.MaxInt32 + 1), + }, + Target: services.Target{ServiceName: "svc", ServiceID: "test_svc"}, + CheckName: "overflow_check", + }, + } + + var checksService mockChecksService + checksService.On("GetChecksResults", mock.Anything, "test_svc").Return(checkResult, nil) + + s := NewChecksAPIService(&checksService) + + resp, err := s.GetFailedChecks(t.Context(), &advisorsv1.GetFailedChecksRequest{ + ServiceId: "test_svc", + }) + require.Nil(t, resp) + require.ErrorContains(t, err, "check result severity 2147483648 is out of range for int32") + checksService.AssertExpectations(t) }) } @@ -219,6 +437,8 @@ func TestListFailedServices(t *testing.T) { resp, err := s.ListFailedServices(t.Context(), &advisorsv1.ListFailedServicesRequest{}) require.EqualError(t, err, "failed to get check results: random error") assert.Nil(t, resp) + + checksService.AssertExpectations(t) }) t.Run("list services with failed checks", func(t *testing.T) { @@ -294,6 +514,7 @@ func TestListFailedServices(t *testing.T) { resp, err := s.ListFailedServices(t.Context(), &advisorsv1.ListFailedServicesRequest{}) require.NoError(t, err) assert.ElementsMatch(t, resp.Result, response.Result) + checksService.AssertExpectations(t) }) } @@ -303,10 +524,11 @@ func TestListAdvisorChecks(t *testing.T) { checksService.On("GetDisabledChecks", mock.Anything).Return([]string{"two"}, nil) checksService.On("GetChecks", mock.Anything). Return(map[string]check.Check{ - "one": {Name: "one", Interval: check.Standard}, - "two": {Name: "two", Interval: check.Frequent}, - "three": {Name: "three", Interval: check.Rare}, - "four": {Name: "four", Interval: ""}, + "one": {Version: 1, Name: "one", Interval: check.Standard, Type: check.MySQLShow}, + "two": {Version: 2, Name: "two", Interval: check.Frequent, Family: check.PostgreSQL}, + "three": {Version: 2, Name: "three", Interval: check.Rare, Family: check.MongoDB}, + // Version 1 check with a type that maps to MongoDB + "four": {Version: 1, Name: "four", Interval: "", Type: check.MongoDBBuildInfo}, }, nil) s := NewChecksAPIService(&checksService) @@ -318,12 +540,33 @@ func TestListAdvisorChecks(t *testing.T) { assert.ElementsMatch( t, resp.Checks, []*advisorsv1.AdvisorCheck{ - {Name: "one", Enabled: true, Interval: advisorsv1.AdvisorCheckInterval_ADVISOR_CHECK_INTERVAL_STANDARD}, - {Name: "two", Enabled: false, Interval: advisorsv1.AdvisorCheckInterval_ADVISOR_CHECK_INTERVAL_FREQUENT}, - {Name: "three", Enabled: true, Interval: advisorsv1.AdvisorCheckInterval_ADVISOR_CHECK_INTERVAL_RARE}, - {Name: "four", Enabled: true, Interval: advisorsv1.AdvisorCheckInterval_ADVISOR_CHECK_INTERVAL_STANDARD}, + { + Name: "one", + Enabled: true, + Interval: advisorsv1.AdvisorCheckInterval_ADVISOR_CHECK_INTERVAL_STANDARD, + Family: advisorsv1.AdvisorCheckFamily_ADVISOR_CHECK_FAMILY_MYSQL, + }, + { + Name: "two", + Enabled: false, + Interval: advisorsv1.AdvisorCheckInterval_ADVISOR_CHECK_INTERVAL_FREQUENT, + Family: advisorsv1.AdvisorCheckFamily_ADVISOR_CHECK_FAMILY_POSTGRESQL, + }, + { + Name: "three", + Enabled: true, + Interval: advisorsv1.AdvisorCheckInterval_ADVISOR_CHECK_INTERVAL_RARE, + Family: advisorsv1.AdvisorCheckFamily_ADVISOR_CHECK_FAMILY_MONGODB, + }, + { + Name: "four", + Enabled: true, + Interval: advisorsv1.AdvisorCheckInterval_ADVISOR_CHECK_INTERVAL_STANDARD, + Family: advisorsv1.AdvisorCheckFamily_ADVISOR_CHECK_FAMILY_MONGODB, + }, }, ) + checksService.AssertExpectations(t) }) t.Run("get disabled checks error", func(t *testing.T) { @@ -335,6 +578,47 @@ func TestListAdvisorChecks(t *testing.T) { resp, err := s.ListAdvisorChecks(t.Context(), nil) require.EqualError(t, err, "failed to get disabled checks list: random error") assert.Nil(t, resp) + checksService.AssertExpectations(t) + }) +} + +func TestListAdvisors(t *testing.T) { + t.Parallel() + + t.Run("normal", func(t *testing.T) { + t.Parallel() + + var checksService mockChecksService + checksService.On("GetDisabledChecks", mock.Anything).Return([]string{}, nil) + checksService.On("GetAdvisors", mock.Anything). + Return([]check.Advisor{ + { + Name: "Advisor 1", + Description: "Description 1", + Summary: "Summary 1", + Category: "Category 1", + Checks: []check.Check{ + {Version: 2, Name: "check1", Family: check.MySQL}, + }, + }, + }, nil) + + s := NewChecksAPIService(&checksService) + + resp, err := s.ListAdvisors(t.Context(), &advisorsv1.ListAdvisorsRequest{}) + require.NoError(t, err) + require.NotNil(t, resp) + assert.Len(t, resp.Advisors, 1) + + adv := resp.Advisors[0] + assert.Equal(t, "Advisor 1", adv.Name) + assert.Equal(t, "Category 1", adv.Category) + assert.Len(t, adv.Checks, 1) + assert.Equal(t, "check1", adv.Checks[0].Name) + assert.Equal(t, advisorsv1.AdvisorCheckFamily_ADVISOR_CHECK_FAMILY_MYSQL, adv.Checks[0].Family) + // Verify the comment generation logic is called + assert.Equal(t, "Partial support (MySQL)", adv.Comment) + checksService.AssertExpectations(t) }) } @@ -348,6 +632,7 @@ func TestUpdateAdvisorChecks(t *testing.T) { resp, err := s.ChangeAdvisorChecks(t.Context(), &advisorsv1.ChangeAdvisorChecksRequest{}) require.EqualError(t, err, "failed to enable disabled advisor checks: random error") assert.Nil(t, resp) + checksService.AssertExpectations(t) }) t.Run("disable advisor checks error", func(t *testing.T) { @@ -360,6 +645,7 @@ func TestUpdateAdvisorChecks(t *testing.T) { resp, err := s.ChangeAdvisorChecks(t.Context(), &advisorsv1.ChangeAdvisorChecksRequest{}) require.EqualError(t, err, "failed to disable advisor checks: random error") assert.Nil(t, resp) + checksService.AssertExpectations(t) }) t.Run("change interval error", func(t *testing.T) { @@ -376,6 +662,7 @@ func TestUpdateAdvisorChecks(t *testing.T) { }) require.EqualError(t, err, "failed to change advisor check interval: random error") assert.Nil(t, resp) + checksService.AssertExpectations(t) }) t.Run("ChangeInterval success", func(t *testing.T) { @@ -394,6 +681,7 @@ func TestUpdateAdvisorChecks(t *testing.T) { }) require.NoError(t, err) assert.Equal(t, &advisorsv1.ChangeAdvisorChecksResponse{}, resp) + checksService.AssertExpectations(t) }) } diff --git a/managed/services/server/logs.go b/managed/services/server/logs.go index 01ef9b51ce9..e7a4a71bcbb 100644 --- a/managed/services/server/logs.go +++ b/managed/services/server/logs.go @@ -22,6 +22,7 @@ import ( "container/ring" "context" "encoding/json" + "errors" "fmt" "io" "mime" @@ -450,10 +451,17 @@ func addAdminSummary(ctx context.Context, zw *zip.Writer) error { return fmt.Errorf("failed to open file %s: %w", file.Name, err) } + // FIXME: Use zip.CreateRaw and zip.OpenRaw API to avoid decompressing-copy-compressing the files + // that significantly increase CPU-bound performance overhead and memory usage and + // avoid 'G110: Potential DoS vulnerability via decompression bomb' security issue. _, err = io.Copy(fw, fr) //nolint:gosec if err != nil { - fr.Close() //nolint:errcheck - return fmt.Errorf("failed to copy data to file %s: %w", file.Name, err) + err = fmt.Errorf("failed to copy data to file %s: %w", file.Name, err) + closeErr := fr.Close() + if closeErr != nil { + return errors.Join(err, fmt.Errorf("failed to close file %s: %w", file.Name, closeErr)) + } + return err } err = fr.Close() diff --git a/managed/services/server/server.go b/managed/services/server/server.go index 199ee2bfed9..01170624a29 100644 --- a/managed/services/server/server.go +++ b/managed/services/server/server.go @@ -165,11 +165,11 @@ func (s *Server) Version(_ context.Context, req *serverv1.VersionRequest) (*serv } case strings.HasPrefix(req.Dummy, "grpccode-"): - code, err := strconv.Atoi(strings.TrimPrefix(req.Dummy, "grpccode-")) + code, err := strconv.ParseUint(strings.TrimPrefix(req.Dummy, "grpccode-"), 10, 32) if err != nil { - return nil, err + return nil, status.Errorf(codes.InvalidArgument, "invalid gRPC code: %v", err) } - grpcCode := codes.Code(code) + grpcCode := codes.Code(uint32(code)) return nil, status.Errorf(grpcCode, "gRPC code %d (%s)", grpcCode, grpcCode) } } diff --git a/managed/services/server/server_test.go b/managed/services/server/server_test.go index 7a3ed57a2ff..639d24befe4 100644 --- a/managed/services/server/server_test.go +++ b/managed/services/server/server_test.go @@ -281,6 +281,77 @@ func TestServer(t *testing.T) { require.NoError(t, err) require.NotNil(t, s) }) + + t.Run("Version", func(t *testing.T) { + t.Parallel() + s := newServer(t) + ctx := context.Background() + + t.Run("PanicError", func(t *testing.T) { + t.Parallel() + assert.PanicsWithError(t, "panic-error", func() { + _, _ = s.Version(ctx, &serverv1.VersionRequest{Dummy: "panic-error"}) + }) + }) + + t.Run("PanicFmtError", func(t *testing.T) { + t.Parallel() + assert.PanicsWithError(t, "panic-fmterror", func() { + _, _ = s.Version(ctx, &serverv1.VersionRequest{Dummy: "panic-fmterror"}) + }) + }) + + t.Run("PanicDefault", func(t *testing.T) { + t.Parallel() + assert.PanicsWithValue(t, "panic-custom-string", func() { + _, _ = s.Version(ctx, &serverv1.VersionRequest{Dummy: "panic-custom-string"}) + }) + }) + + t.Run("GRPCCode", func(t *testing.T) { + t.Parallel() + t.Run("Valid", func(t *testing.T) { + t.Parallel() + _, err := s.Version(ctx, &serverv1.VersionRequest{Dummy: "grpccode-1"}) + require.Error(t, err) + st, ok := status.FromError(err) + require.True(t, ok) + assert.Equal(t, codes.Canceled, st.Code()) + assert.Equal(t, "gRPC code 1 (Canceled)", st.Message()) + }) + + t.Run("InvalidFormat", func(t *testing.T) { + t.Parallel() + _, err := s.Version(ctx, &serverv1.VersionRequest{Dummy: "grpccode-abc"}) + require.Error(t, err) + st, ok := status.FromError(err) + require.True(t, ok) + assert.Equal(t, codes.InvalidArgument, st.Code()) + assert.Contains(t, st.Message(), "invalid gRPC code") + }) + + t.Run("Overflow", func(t *testing.T) { + t.Parallel() + // 2^32 is larger than max uint32 + _, err := s.Version(ctx, &serverv1.VersionRequest{Dummy: "grpccode-4294967296"}) + require.Error(t, err) + st, ok := status.FromError(err) + require.True(t, ok) + assert.Equal(t, codes.InvalidArgument, st.Code()) + assert.Contains(t, st.Message(), "invalid gRPC code") + }) + + t.Run("Negative", func(t *testing.T) { + t.Parallel() + _, err := s.Version(ctx, &serverv1.VersionRequest{Dummy: "grpccode--1"}) + require.Error(t, err) + st, ok := status.FromError(err) + require.True(t, ok) + assert.Equal(t, codes.InvalidArgument, st.Code()) + assert.Contains(t, st.Message(), "invalid gRPC code") + }) + }) + }) } func TestConvertDefaultRoleID(t *testing.T) { diff --git a/managed/services/user/user.go b/managed/services/user/user.go index 116e9056293..d85eae3787c 100644 --- a/managed/services/user/user.go +++ b/managed/services/user/user.go @@ -19,6 +19,8 @@ package user import ( "context" "errors" + "fmt" + "math" "time" "github.com/sirupsen/logrus" @@ -130,8 +132,16 @@ func (s *Service) UpdateUser(ctx context.Context, req *userv1.UpdateUserRequest) return nil, e } + if userInfo.ID < 0 || userInfo.ID > math.MaxUint32 { + return nil, status.Errorf(codes.Internal, "%s", fmt.Sprintf("User ID %d is out of uint32 range", userInfo.ID)) + } + + if userInfo.SnoozeCount < 0 || userInfo.SnoozeCount > math.MaxUint32 { + return nil, status.Errorf(codes.Internal, "%s", fmt.Sprintf("Snooze count %d is out of uint32 range", userInfo.SnoozeCount)) + } + resp := &userv1.UpdateUserResponse{ - UserId: uint32(userInfo.ID), //nolint:gosec // user ID is not expected to overflow uint32 + UserId: uint32(userInfo.ID), ProductTourCompleted: userInfo.Tour, AlertingTourCompleted: userInfo.AlertingTour, SnoozedPmmVersion: userInfo.SnoozedPMMVersion, @@ -157,7 +167,7 @@ func (s *Service) ListUsers(_ context.Context, _ *userv1.ListUsersRequest) (*use } for userID, roleIDs := range userRoles { resp.Users = append(resp.Users, &userv1.ListUsersResponse_UserDetail{ - UserId: uint32(userID), + UserId: uint32(userID), //nolint:gosec // user ID is not expected to overflow uint32 RoleIds: roleIDs, }) } diff --git a/managed/utils/encryption/encryption.go b/managed/utils/encryption/encryption.go index 833151dc68a..f5cd37c90df 100644 --- a/managed/utils/encryption/encryption.go +++ b/managed/utils/encryption/encryption.go @@ -22,6 +22,7 @@ import ( "errors" "fmt" "os" + "path/filepath" "slices" "strings" "sync" @@ -108,8 +109,9 @@ func New() *Encryption { e.Path = DefaultEncryptionKeyPath } } + e.Path = filepath.Clean(e.Path) - bytes, err := os.ReadFile(e.Path) + data, err := os.ReadFile(e.Path) //nolint:gosec // path is controlled by admin via environment variable switch { case os.IsNotExist(err): err = e.generateAndPersistKey() @@ -119,7 +121,7 @@ func New() *Encryption { case err != nil: logrus.Panicf("Encryption: %v", err) default: - e.Key = string(bytes) + e.Key = string(data) } primitive, err := e.getPrimitive() diff --git a/managed/utils/encryption/helpers.go b/managed/utils/encryption/helpers.go index 0cbbd2c3728..c607449327f 100644 --- a/managed/utils/encryption/helpers.go +++ b/managed/utils/encryption/helpers.go @@ -19,6 +19,7 @@ import ( "database/sql" "fmt" "os" + "path/filepath" "slices" "strings" @@ -28,7 +29,7 @@ import ( func encryptionKeyPath() string { customKeyPath := os.Getenv("PMM_ENCRYPTION_KEY_PATH") if customKeyPath != "" { - return customKeyPath + return filepath.Clean(customKeyPath) } return DefaultEncryptionKeyPath diff --git a/managed/utils/validators/alerting_rules.go b/managed/utils/validators/alerting_rules.go index a3977c51ae5..2fc230effa4 100644 --- a/managed/utils/validators/alerting_rules.go +++ b/managed/utils/validators/alerting_rules.go @@ -48,8 +48,16 @@ func ValidateAlertingRules(ctx context.Context, rules string) error { if err != nil { return fmt.Errorf("alerting rule validation failed: %w", err) } - tempFile.Close() //nolint:errcheck - defer os.Remove(tempFile.Name()) //nolint:errcheck + err = tempFile.Close() + if err != nil { + return fmt.Errorf("alerting rule validation failed: %w", err) + } + defer func() { + remErr := os.Remove(tempFile.Name()) + if remErr != nil { + logrus.Errorf("failed to remove temporary file %s: %v", tempFile.Name(), remErr) + } + }() err = os.WriteFile(tempFile.Name(), []byte(rules), 0o644) //nolint:gosec,mnd if err != nil { diff --git a/qan-api2/main.go b/qan-api2/main.go index 48eb71efd16..827e27d3db9 100644 --- a/qan-api2/main.go +++ b/qan-api2/main.go @@ -189,8 +189,11 @@ func runJSONServer(ctx context.Context, grpcBindF, jsonBindF string) { ctx, cancel := context.WithTimeout(context.Background(), shutdownTimeout) err := server.Shutdown(ctx) //nolint:contextcheck if err != nil { - l.Errorf("Failed to shutdown gracefully: %s \n", err) - server.Close() //nolint:errcheck + l.Errorf("Failed to shutdown gracefully: %v", err) + err = server.Close() + if err != nil { + l.Errorf("Failed to close server: %v", err) + } } cancel() } @@ -232,7 +235,10 @@ func runDebugServer(ctx context.Context, debugBindF string) { l.Panic(err) } http.HandleFunc("/debug", func(rw http.ResponseWriter, _ *http.Request) { - rw.Write(buf.Bytes()) //nolint:errcheck + _, wErr := rw.Write(buf.Bytes()) + if wErr != nil { + l.WithError(wErr).Error("Failed to write response") + } }) l.Infof("Starting server on http://%s/debug\nRegistered handlers:\n\t%s", debugBindF, strings.Join(handlers, "\n\t")) diff --git a/qan-api2/models/metrics.go b/qan-api2/models/metrics.go index 39c3f83ea41..19982bdd3de 100644 --- a/qan-api2/models/metrics.go +++ b/qan-api2/models/metrics.go @@ -23,6 +23,7 @@ import ( "errors" "fmt" "log" + "math" "sort" "strings" "text/template" @@ -495,7 +496,10 @@ GROUP BY point ORDER BY point ASC; ` -var tmplMetricsSparklines = template.Must(template.New("queryMetricsSparklines").Funcs(funcMap).Parse(queryMetricsSparklinesTmpl)) +var ( + tmplMetricsSparklines = template.Must(template.New("queryMetricsSparklines").Funcs(funcMap).Parse(queryMetricsSparklinesTmpl)) + errMetricsPeriodStartToLessStartFrom = errors.New("periodStartToSec must be greater than periodStartFromSec") +) // SelectSparklines selects datapoint for sparklines. func (m *Metrics) SelectSparklines(ctx context.Context, periodStartFromSec, periodStartToSec int64, @@ -509,19 +513,33 @@ func (m *Metrics) SelectSparklines(ctx context.Context, periodStartFromSec, peri // Otherwise amount of sparklines points is equal to minutes in time range to not mess up calculation. amountOfPoints := int64(optimalAmountOfPoint) timePeriod := periodStartToSec - periodStartFromSec + if timePeriod < 0 { + return nil, errMetricsPeriodStartToLessStartFrom + } // reduce amount of point if period less then 2h. if timePeriod < int64(minFullTimeFrame.Seconds()) { // minimum point is 1 minute amountOfPoints = timePeriod / secondsPerMinute } + if amountOfPoints <= 0 { + amountOfPoints = 1 + } + // how many full minutes we can fit into given amount of points. minutesInPoint := (periodStartToSec - periodStartFromSec) / secondsPerMinute / amountOfPoints + if minutesInPoint == 0 { + minutesInPoint = 1 + } // we need aditional point to show this minutes remainder := ((periodStartToSec - periodStartFromSec) / secondsPerMinute) % amountOfPoints amountOfPoints += remainder / minutesInPoint timeFrame := minutesInPoint * secondsPerMinute + if timeFrame > math.MaxUint32 { + return nil, fmt.Errorf("timeFrame %d exceeds uint32 max value", timeFrame) + } + arg := map[string]any{ "period_start_from": periodStartFromSec, "period_start_to": periodStartToSec, diff --git a/qan-api2/models/metrics_test.go b/qan-api2/models/metrics_test.go index e132a16b835..9e276f6b459 100644 --- a/qan-api2/models/metrics_test.go +++ b/qan-api2/models/metrics_test.go @@ -276,3 +276,51 @@ func TestMetrics_ExplainFingerprintByQueryID(t *testing.T) { assert.Zero(t, res.PlaceholdersCount) }) } + +func TestMetrics_SelectSparklines(t *testing.T) { + t.Parallel() + ctx := t.Context() + sqlxDB := setupTestClickHouse(t) + m := NewMetrics(sqlxDB) + + t1, err := time.Parse(time.RFC3339, "2019-01-01T00:00:00Z") + require.NoError(t, err) + periodFrom := t1.Unix() + t2, err := time.Parse(time.RFC3339, "2019-01-01T10:00:00Z") + require.NoError(t, err) + periodTo := t2.Unix() + + t.Run("Invalid time range", func(t *testing.T) { + t.Parallel() + res, err := m.SelectSparklines(ctx, periodTo, periodFrom, "B305F6354FA21F2A", "queryid", nil, nil) + require.ErrorIs(t, err, errMetricsPeriodStartToLessStartFrom) + require.Nil(t, res) + }) + + t.Run("Very small time range", func(t *testing.T) { + t.Parallel() + ts := int64(1600000000) + res, err := m.SelectSparklines(ctx, ts, ts+1, "B305F6354FA21F2A", "queryid", nil, nil) + require.NoError(t, err) + require.NotNil(t, res) + }) + + t.Run("Timeframe overflow", func(t *testing.T) { + t.Parallel() + from := int64(0) + // optimalAmountOfPoint is 120. timeFrame is roughly (To - From) / 120. + // To trigger > MaxUint32 (4294967295), difference must be > 515,396,075,400. + to := int64(600_000_000_000) + res, err := m.SelectSparklines(ctx, from, to, "B305F6354FA21F2A", "queryid", nil, nil) + require.ErrorContains(t, err, "exceeds uint32 max value") + require.Nil(t, res) + }) + + t.Run("Happy path", func(t *testing.T) { + t.Parallel() + res, err := m.SelectSparklines(ctx, periodFrom, periodTo, "B305F6354FA21F2A", "queryid", nil, nil) + require.NoError(t, err) + require.NotNil(t, res) + assert.NotEmpty(t, res) + }) +} diff --git a/qan-api2/models/reporter.go b/qan-api2/models/reporter.go index 92a1c46c734..8184f5074e4 100644 --- a/qan-api2/models/reporter.go +++ b/qan-api2/models/reporter.go @@ -22,6 +22,7 @@ import ( "encoding/json" "errors" "fmt" + "math" "slices" "strings" "sync" @@ -310,7 +311,10 @@ GROUP BY point ORDER BY point ASC; ` -var tmplQueryReportSparklines = template.Must(template.New("queryReportSparklines").Funcs(funcMap).Parse(queryReportSparklinesTmpl)) +var ( + tmplQueryReportSparklines = template.Must(template.New("queryReportSparklines").Funcs(funcMap).Parse(queryReportSparklinesTmpl)) + errReporterPeriodStartToLessStartFrom = errors.New("periodStartToSec must be greater than periodStartFromSec") +) // SelectSparklines selects datapoint for sparklines. func (r *Reporter) SelectSparklines(ctx context.Context, dimensionVal string, @@ -322,6 +326,10 @@ func (r *Reporter) SelectSparklines(ctx context.Context, dimensionVal string, periodStartToSec = periodStartToSec / 60 * 60 //nolint:mnd periodStartFromSec = periodStartFromSec / 60 * 60 //nolint:mnd + if periodStartToSec <= periodStartFromSec { + return nil, errReporterPeriodStartToLessStartFrom + } + // If time range is bigger then two hour - amount of sparklines points = 120 to avoid huge data in response. // Otherwise amount of sparklines points is equal to minutes in time range to not mess up calculation. amountOfPoints := int64(optimalAmountOfPoint) @@ -332,13 +340,26 @@ func (r *Reporter) SelectSparklines(ctx context.Context, dimensionVal string, amountOfPoints = timePeriod / 60 //nolint:mnd } + if amountOfPoints <= 0 { + amountOfPoints = 1 + } + // how many full minutes we can fit into given amount of points. minutesInPoint := (periodStartToSec - periodStartFromSec) / 60 / amountOfPoints //nolint:mnd + if minutesInPoint <= 0 { + minutesInPoint = 1 + } + // we need aditional point to show this minutes remainder := ((periodStartToSec - periodStartFromSec) / 60) % amountOfPoints //nolint:mnd amountOfPoints += remainder / minutesInPoint timeFrame := minutesInPoint * 60 //nolint:mnd + if timeFrame > math.MaxUint32 { + return nil, fmt.Errorf("calculated timeframe %d exceeds uint32 capacity", timeFrame) + } + tfUint32 := uint32(timeFrame) + arg := map[string]any{ "dimension_val": dimensionVal, "period_start_from": periodStartFromSec, @@ -447,8 +468,8 @@ func (r *Reporter) SelectSparklines(ctx context.Context, dimensionVal string, if !ok { p = &qanpbv1.Point{} p.Point = pointN - p.TimeFrame = uint32(timeFrame) - timeShift := timeFrame * int64(pointN) + p.TimeFrame = tfUint32 + timeShift := int64(tfUint32) * int64(pointN) ts := periodStartToSec - timeShift p.Timestamp = time.Unix(ts, 0).UTC().Format(time.RFC3339) } diff --git a/qan-api2/models/reporter_test.go b/qan-api2/models/reporter_test.go index 80367a7cf23..2e22ff7dd8b 100644 --- a/qan-api2/models/reporter_test.go +++ b/qan-api2/models/reporter_test.go @@ -363,3 +363,37 @@ func TestQueryDimensionTemplate(t *testing.T) { }) } } + +func TestReporter_SelectSparklines_Validation(t *testing.T) { + t.Parallel() + // We use a nil DB reporter because we are testing validation logic + // that returns before any database interaction occurs. + r := &Reporter{} + + t.Run("EmptyRange", func(t *testing.T) { + t.Parallel() + // periodTo <= periodFrom should return (nil, error) early + res, err := r.SelectSparklines(context.Background(), "val", 1000, 1000, nil, nil, "group", "col", false) + require.ErrorIs(t, err, errReporterPeriodStartToLessStartFrom) + require.Nil(t, res) + + res, err = r.SelectSparklines(context.Background(), "val", 2000, 1000, nil, nil, "group", "col", false) + require.ErrorIs(t, err, errReporterPeriodStartToLessStartFrom) + require.Nil(t, res) + }) + + t.Run("TimeFrameOverflow", func(t *testing.T) { + t.Parallel() + // We need to trigger timeFrame > math.MaxUint32. + // timeFrame = minutesInPoint * 60. + // minutesInPoint = duration / 60 / 120 (for large durations). + // So we need (duration / 7200) * 60 > MaxUint32. + // duration / 120 > MaxUint32. + from := int64(0) + to := int64(600_000_000_000) // ~19,000 years + res, err := r.SelectSparklines(context.Background(), "val", from, to, nil, nil, "group", "col", false) + require.Error(t, err) + require.Contains(t, err.Error(), "exceeds uint32 capacity") + require.Nil(t, res) + }) +} diff --git a/vmproxy/proxy/proxy.go b/vmproxy/proxy/proxy.go index 5a9f6ba5b52..4f3f4866ae7 100644 --- a/vmproxy/proxy/proxy.go +++ b/vmproxy/proxy/proxy.go @@ -70,7 +70,11 @@ func failOnInvalidHeader(rw http.ResponseWriter, req *http.Request, headerName s if err != nil { rw.Header().Set("Content-Type", "text/plain; charset=utf-8") rw.WriteHeader(http.StatusPreconditionFailed) - io.WriteString(rw, fmt.Sprintf("Failed to parse %s header", headerName)) //nolint:errcheck + _, err = io.WriteString(rw, fmt.Sprintf("Failed to parse %s header", headerName)) + if err != nil { + logrus.Errorf("Failed to write response: status code=%d, err=%v", + http.StatusPreconditionFailed, err) + } return true } }