From feaf7af4af07d1c8221197247526636790e53ccd Mon Sep 17 00:00:00 2001 From: Artem Gavrilov Date: Fri, 28 Jul 2023 18:32:07 +0200 Subject: [PATCH] PMM-11206 Remove limit for custom templates upload (#2166) * PMM-11206 Refactoring * PMM-11206 Regenerate mocks * PMM-11206 Refactoring * PMM-11206 Update comment * PMM-11206 Allow to upload multiple templates at once, improve error messages * PMM-11206 Fix * PMM-11206 Add API test * PMM-11206 Fix linter warning * PMM-11206 Cleanup * PMM-11206 Fix API tests * PMM-11206 Update saas dependency * PMM-11206 Fix * PMM-11206 Fix API test * PMM-11206 Update saas dependency * PMM-11206 Fix --- .../management/alerting/alerting_test.go | 40 ++- api-tests/pmm-api-tests-output.txt | 0 api-tests/testdata/ia/multiple-templates.yaml | 49 +++ api/managementpb/alerting/alerting.pb.go | 6 +- api/managementpb/alerting/alerting.proto | 6 +- api/managementpb/alerting/json/alerting.json | 6 +- .../alerting/create_template_responses.go | 2 +- .../alerting/list_templates_responses.go | 2 +- .../alerting/update_template_responses.go | 2 +- api/swagger/swagger-dev.json | 6 +- api/swagger/swagger.json | 6 +- go.mod | 6 +- go.sum | 12 +- managed/models/models.go | 7 + managed/models/template_helpers.go | 79 +++-- managed/models/template_helpers_test.go | 4 +- managed/models/template_model.go | 2 +- .../services/management/alerting/service.go | 317 +++++++----------- managed/services/management/ia/deps.go | 4 +- .../ia/mock_templates_service_test.go | 10 +- .../services/management/ia/rules_service.go | 17 +- 21 files changed, 316 insertions(+), 267 deletions(-) create mode 100644 api-tests/pmm-api-tests-output.txt create mode 100644 api-tests/testdata/ia/multiple-templates.yaml diff --git a/api-tests/management/alerting/alerting_test.go b/api-tests/management/alerting/alerting_test.go index 960500e36e..f68107683f 100644 --- a/api-tests/management/alerting/alerting_test.go +++ b/api-tests/management/alerting/alerting_test.go @@ -114,6 +114,10 @@ func assertTemplate(t *testing.T, expectedTemplate alert.Template, listTemplates assert.Equal(t, expectedTemplate.Labels, tmpl.Labels) assert.Equal(t, expectedTemplate.Annotations, tmpl.Annotations) + expectedYAML, err := alert.ToYAML([]alert.Template{expectedTemplate}) + require.NoError(t, err) + assert.Equal(t, expectedYAML, tmpl.Yaml) + assert.NotEmpty(t, tmpl.CreatedAt) } @@ -124,6 +128,9 @@ func TestTemplatesAPI(t *testing.T) { templateData, err := os.ReadFile("../../testdata/ia/template.yaml") require.NoError(t, err) + multipleTemplatesData, err := os.ReadFile("../../testdata/ia/multiple-templates.yaml") + require.NoError(t, err) + invalidTemplateData, err := os.ReadFile("../../testdata/ia/invalid-template.yaml") require.NoError(t, err) @@ -156,6 +163,35 @@ func TestTemplatesAPI(t *testing.T) { assertTemplate(t, alertTemplates[0], resp.Payload.Templates) }) + t.Run("multiple templates at once", func(t *testing.T) { + t.Parallel() + + alertTemplates, yml := formatTemplateYaml(t, string(multipleTemplatesData)) + _, err := client.CreateTemplate(&alerting.CreateTemplateParams{ + Body: alerting.CreateTemplateBody{ + Yaml: yml, + }, + Context: pmmapitests.Context, + }) + require.NoError(t, err) + require.Len(t, alertTemplates, 2) + t.Cleanup(func() { + deleteTemplate(t, client, alertTemplates[0].Name) + deleteTemplate(t, client, alertTemplates[1].Name) + }) + + resp, err := client.ListTemplates(&alerting.ListTemplatesParams{ + Body: alerting.ListTemplatesBody{ + Reload: true, + }, + Context: pmmapitests.Context, + }) + require.NoError(t, err) + + assertTemplate(t, alertTemplates[0], resp.Payload.Templates) + assertTemplate(t, alertTemplates[1], resp.Payload.Templates) + }) + t.Run("duplicate", func(t *testing.T) { t.Parallel() @@ -188,7 +224,7 @@ func TestTemplatesAPI(t *testing.T) { }, Context: pmmapitests.Context, }) - pmmapitests.AssertAPIErrorf(t, err, 400, codes.InvalidArgument, "Failed to parse rule template.") + pmmapitests.AssertAPIErrorf(t, err, 400, codes.InvalidArgument, "Failed to parse rule template") }) t.Run("invalid template", func(t *testing.T) { @@ -201,7 +237,7 @@ func TestTemplatesAPI(t *testing.T) { Context: pmmapitests.Context, }) - pmmapitests.AssertAPIErrorf(t, err, 400, codes.InvalidArgument, "Failed to parse rule template.") + pmmapitests.AssertAPIErrorf(t, err, 400, codes.InvalidArgument, "Failed to parse rule template") }) }) diff --git a/api-tests/pmm-api-tests-output.txt b/api-tests/pmm-api-tests-output.txt new file mode 100644 index 0000000000..e69de29bb2 diff --git a/api-tests/testdata/ia/multiple-templates.yaml b/api-tests/testdata/ia/multiple-templates.yaml new file mode 100644 index 0000000000..c2dcec7554 --- /dev/null +++ b/api-tests/testdata/ia/multiple-templates.yaml @@ -0,0 +1,49 @@ +--- +templates: + - name: test_template_1 + version: 1 + summary: Test summary 1 + tiers: [ anonymous, registered ] + expr: "test expression" + params: + - name: param1 + summary: first parameter with default value and defined range + unit: s + type: float + range: [ 0, 100 ] + value: 80 + - name: param2 + summary: second parameter without default value and defined range + unit: s + type: float + for: 300s + severity: warning + labels: + foo: bar + annotations: + description: test description + summary: test summary + + - name: test_template_2 + version: 1 + summary: Test summary 2 + tiers: [ anonymous, registered ] + expr: "test expression" + params: + - name: param1 + summary: first parameter with default value and defined range + unit: s + type: float + range: [ 0, 100 ] + value: 80 + - name: param2 + summary: second parameter without default value and defined range + unit: s + type: float + for: 300s + severity: warning + labels: + foo: bar + annotations: + description: test description + summary: test summary diff --git a/api/managementpb/alerting/alerting.pb.go b/api/managementpb/alerting/alerting.pb.go index 7d6f223563..13ec12832d 100644 --- a/api/managementpb/alerting/alerting.pb.go +++ b/api/managementpb/alerting/alerting.pb.go @@ -500,7 +500,7 @@ type Template struct { Source TemplateSource `protobuf:"varint,9,opt,name=source,proto3,enum=alerting.v1.TemplateSource" json:"source,omitempty"` // Template creation time. Empty for built-in and SaaS templates. CreatedAt *timestamppb.Timestamp `protobuf:"bytes,10,opt,name=created_at,json=createdAt,proto3" json:"created_at,omitempty"` - // YAML (or JSON) template file content. Empty for built-in and SaaS templates. + // YAML template file content. Empty for built-in and SaaS templates. Yaml string `protobuf:"bytes,11,opt,name=yaml,proto3" json:"yaml,omitempty"` } @@ -731,7 +731,7 @@ type CreateTemplateRequest struct { sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields - // YAML (or JSON) template file content. + // YAML template file content. Yaml string `protobuf:"bytes,1,opt,name=yaml,proto3" json:"yaml,omitempty"` } @@ -819,7 +819,7 @@ type UpdateTemplateRequest struct { // Machine-readable name (ID). Name string `protobuf:"bytes,1,opt,name=name,proto3" json:"name,omitempty"` - // YAML (or JSON) template file content. + // YAML template file content. Yaml string `protobuf:"bytes,2,opt,name=yaml,proto3" json:"yaml,omitempty"` } diff --git a/api/managementpb/alerting/alerting.proto b/api/managementpb/alerting/alerting.proto index 82d8df2f01..2356bb6b34 100644 --- a/api/managementpb/alerting/alerting.proto +++ b/api/managementpb/alerting/alerting.proto @@ -98,7 +98,7 @@ message Template { TemplateSource source = 9; // Template creation time. Empty for built-in and SaaS templates. google.protobuf.Timestamp created_at = 10; - // YAML (or JSON) template file content. Empty for built-in and SaaS templates. + // YAML template file content. Empty for built-in and SaaS templates. string yaml = 11; } @@ -116,7 +116,7 @@ message ListTemplatesResponse { } message CreateTemplateRequest { - // YAML (or JSON) template file content. + // YAML template file content. string yaml = 1 [(validate.rules).string.min_len = 1]; } @@ -125,7 +125,7 @@ message CreateTemplateResponse {} message UpdateTemplateRequest { // Machine-readable name (ID). string name = 1 [(validate.rules).string.min_len = 1]; - // YAML (or JSON) template file content. + // YAML template file content. string yaml = 2 [(validate.rules).string.min_len = 1]; } diff --git a/api/managementpb/alerting/json/alerting.json b/api/managementpb/alerting/json/alerting.json index a9c0e57d1f..31a3e0fe4f 100644 --- a/api/managementpb/alerting/json/alerting.json +++ b/api/managementpb/alerting/json/alerting.json @@ -215,7 +215,7 @@ "type": "object", "properties": { "yaml": { - "description": "YAML (or JSON) template file content.", + "description": "YAML template file content.", "type": "string", "x-order": 0 } @@ -575,7 +575,7 @@ "x-order": 1 }, "yaml": { - "description": "YAML (or JSON) template file content. Empty for built-in and SaaS templates.", + "description": "YAML template file content. Empty for built-in and SaaS templates.", "type": "string", "x-order": 10 } @@ -660,7 +660,7 @@ "x-order": 0 }, "yaml": { - "description": "YAML (or JSON) template file content.", + "description": "YAML template file content.", "type": "string", "x-order": 1 } diff --git a/api/managementpb/alerting/json/client/alerting/create_template_responses.go b/api/managementpb/alerting/json/client/alerting/create_template_responses.go index b7f4dd8f7d..ab5c000932 100644 --- a/api/managementpb/alerting/json/client/alerting/create_template_responses.go +++ b/api/managementpb/alerting/json/client/alerting/create_template_responses.go @@ -121,7 +121,7 @@ CreateTemplateBody create template body swagger:model CreateTemplateBody */ type CreateTemplateBody struct { - // YAML (or JSON) template file content. + // YAML template file content. Yaml string `json:"yaml,omitempty"` } diff --git a/api/managementpb/alerting/json/client/alerting/list_templates_responses.go b/api/managementpb/alerting/json/client/alerting/list_templates_responses.go index cee8c814a3..a4f455229a 100644 --- a/api/managementpb/alerting/json/client/alerting/list_templates_responses.go +++ b/api/managementpb/alerting/json/client/alerting/list_templates_responses.go @@ -539,7 +539,7 @@ type ListTemplatesOKBodyTemplatesItems0 struct { // Format: date-time CreatedAt strfmt.DateTime `json:"created_at,omitempty"` - // YAML (or JSON) template file content. Empty for built-in and SaaS templates. + // YAML template file content. Empty for built-in and SaaS templates. Yaml string `json:"yaml,omitempty"` } diff --git a/api/managementpb/alerting/json/client/alerting/update_template_responses.go b/api/managementpb/alerting/json/client/alerting/update_template_responses.go index 3d051b737b..242e579151 100644 --- a/api/managementpb/alerting/json/client/alerting/update_template_responses.go +++ b/api/managementpb/alerting/json/client/alerting/update_template_responses.go @@ -124,7 +124,7 @@ type UpdateTemplateBody struct { // Machine-readable name (ID). Name string `json:"name,omitempty"` - // YAML (or JSON) template file content. + // YAML template file content. Yaml string `json:"yaml,omitempty"` } diff --git a/api/swagger/swagger-dev.json b/api/swagger/swagger-dev.json index 63af6135a9..4e04d3fe73 100644 --- a/api/swagger/swagger-dev.json +++ b/api/swagger/swagger-dev.json @@ -31110,7 +31110,7 @@ "type": "object", "properties": { "yaml": { - "description": "YAML (or JSON) template file content.", + "description": "YAML template file content.", "type": "string", "x-order": 0 } @@ -31470,7 +31470,7 @@ "x-order": 9 }, "yaml": { - "description": "YAML (or JSON) template file content. Empty for built-in and SaaS templates.", + "description": "YAML template file content. Empty for built-in and SaaS templates.", "type": "string", "x-order": 10 } @@ -31555,7 +31555,7 @@ "x-order": 0 }, "yaml": { - "description": "YAML (or JSON) template file content.", + "description": "YAML template file content.", "type": "string", "x-order": 1 } diff --git a/api/swagger/swagger.json b/api/swagger/swagger.json index f91076ead0..908e2702de 100644 --- a/api/swagger/swagger.json +++ b/api/swagger/swagger.json @@ -21220,7 +21220,7 @@ "type": "object", "properties": { "yaml": { - "description": "YAML (or JSON) template file content.", + "description": "YAML template file content.", "type": "string", "x-order": 0 } @@ -21580,7 +21580,7 @@ "x-order": 9 }, "yaml": { - "description": "YAML (or JSON) template file content. Empty for built-in and SaaS templates.", + "description": "YAML template file content. Empty for built-in and SaaS templates.", "type": "string", "x-order": 10 } @@ -21665,7 +21665,7 @@ "x-order": 0 }, "yaml": { - "description": "YAML (or JSON) template file content.", + "description": "YAML template file content.", "type": "string", "x-order": 1 } diff --git a/go.mod b/go.mod index 0f74f774f7..5d3d35016a 100644 --- a/go.mod +++ b/go.mod @@ -55,7 +55,7 @@ require ( github.com/operator-framework/api v0.17.6 github.com/operator-framework/operator-lifecycle-manager v0.24.0 github.com/percona-platform/dbaas-api v0.0.0-20230103182808-d79c449a9f4c - github.com/percona-platform/saas v0.0.0-20230713134421-bb403194c5f7 + github.com/percona-platform/saas v0.0.0-20230728161159-ad6bdeb8a3d9 github.com/percona/dbaas-operator v0.1.6 github.com/percona/exporter_shared v0.7.4 github.com/percona/go-mysql v0.0.0-20210427141028-73d29c6da78c @@ -133,7 +133,7 @@ require ( github.com/spf13/pflag v1.0.5 // indirect github.com/xlab/treeprint v1.1.0 // indirect go.opentelemetry.io/otel/metric v1.16.0 // indirect - go.uber.org/atomic v1.10.0 // indirect + go.uber.org/atomic v1.11.0 // indirect golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1 // indirect golang.org/x/time v0.3.0 // indirect google.golang.org/genproto v0.0.0-20230526203410-71b5a4ffd15e // indirect @@ -172,7 +172,7 @@ require ( github.com/andybalholm/brotli v1.0.5 // indirect github.com/armon/go-metrics v0.4.0 // indirect github.com/beorn7/perks v1.0.1 // indirect - github.com/cenkalti/backoff/v4 v4.2.0 // indirect + github.com/cenkalti/backoff/v4 v4.2.1 // indirect github.com/cespare/xxhash/v2 v2.2.0 // indirect github.com/charmbracelet/harmonica v0.2.0 // indirect github.com/cloudflare/golz4 v0.0.0-20150217214814-ef862a3cdc58 // indirect diff --git a/go.sum b/go.sum index ee78e3c92f..c420e69174 100644 --- a/go.sum +++ b/go.sum @@ -144,8 +144,8 @@ github.com/brianvoe/gofakeit v3.18.0+incompatible/go.mod h1:kfwdRA90vvNhPutZWfH7 github.com/brianvoe/gofakeit/v6 v6.23.0 h1:pgVhyWpYq4e0GEVCh2gdZnS/nBX+8SnyTBliHg5xjks= github.com/brianvoe/gofakeit/v6 v6.23.0/go.mod h1:Ow6qC71xtwm79anlwKRlWZW6zVq9D2XHE4QSSMP/rU8= github.com/buger/jsonparser v1.1.1/go.mod h1:6RYKKt7H4d4+iWqouImQ9R2FZql3VbhNgx27UK13J/0= -github.com/cenkalti/backoff/v4 v4.2.0 h1:HN5dHm3WBOgndBH6E8V0q2jIYIR3s9yglV8k/+MN3u4= -github.com/cenkalti/backoff/v4 v4.2.0/go.mod h1:Y3VNntkOUPxTVeUxJ/G5vcM//AlwfmyYozVcomhLiZE= +github.com/cenkalti/backoff/v4 v4.2.1 h1:y4OZtCnogmCPw98Zjyt5a6+QwPLGkiQsYW5oUqylYbM= +github.com/cenkalti/backoff/v4 v4.2.1/go.mod h1:Y3VNntkOUPxTVeUxJ/G5vcM//AlwfmyYozVcomhLiZE= github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU= github.com/cespare/xxhash v1.1.0/go.mod h1:XrSqR1VqqWfGrhpAt58auRo0WTKS1nRRg3ghfAqPWnc= github.com/cespare/xxhash/v2 v2.1.1/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= @@ -645,8 +645,8 @@ github.com/percona-lab/crypto v0.0.0-20220811043533-d164de3c7f08 h1:NprWeXddFZJS github.com/percona-lab/crypto v0.0.0-20220811043533-d164de3c7f08/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4= github.com/percona-platform/dbaas-api v0.0.0-20230103182808-d79c449a9f4c h1:1JySfwdjVfc9ahl0466OX7nSQ7Z4SjQkLe3ZdLkMOJI= github.com/percona-platform/dbaas-api v0.0.0-20230103182808-d79c449a9f4c/go.mod h1:/jgle33awfHq1va/T6NnNS5wWAETSnl6wUZ1bew+CJ0= -github.com/percona-platform/saas v0.0.0-20230713134421-bb403194c5f7 h1:9XwfsWsQjWLWZpm9ouuAMZGZ3g4bT4pt0E/fr0Tc/Vo= -github.com/percona-platform/saas v0.0.0-20230713134421-bb403194c5f7/go.mod h1:lZuFcqj0EoQWx28SYkTcdhJOCQEbRcAyahYPfRMY7tc= +github.com/percona-platform/saas v0.0.0-20230728161159-ad6bdeb8a3d9 h1:KkOH+Y4sVRP7qvRtTDmfPFNjjQcwU2054/jNl9DZhEo= +github.com/percona-platform/saas v0.0.0-20230728161159-ad6bdeb8a3d9/go.mod h1:lZuFcqj0EoQWx28SYkTcdhJOCQEbRcAyahYPfRMY7tc= github.com/percona/dbaas-operator v0.1.6 h1:NsZXDKcPXk38kET+X6r8Es+3Supyu5XJZMS0gqPejKs= github.com/percona/dbaas-operator v0.1.6/go.mod h1:52B/kh+Jmtfv0JiZgDcc34qgbwwEi9U4A3311JBxIZg= github.com/percona/exporter_shared v0.7.4 h1:S+xnfK/CySiYqr4XqLiLAfO3rxgEOUFK+m6lCBi3mgc= @@ -832,8 +832,8 @@ go.starlark.net v0.0.0-20230717150657-8a3343210976 h1:7ljYNcZU84T2N0tZdDgvL7U3M4 go.starlark.net v0.0.0-20230717150657-8a3343210976/go.mod h1:jxU+3+j+71eXOW14274+SmmuW82qJzl6iZSeqEtTGds= go.uber.org/atomic v1.7.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc= go.uber.org/atomic v1.9.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc= -go.uber.org/atomic v1.10.0 h1:9qC72Qh0+3MqyJbAn8YU5xVq1frD8bn3JtD2oXtafVQ= -go.uber.org/atomic v1.10.0/go.mod h1:LUxbIzbOniOlMKjJjyPfpl4v+PKK2cNJn91OQbhoJI0= +go.uber.org/atomic v1.11.0 h1:ZvwS0R+56ePWxUNi+Atn9dWONBPp/AUETXlHW0DxSjE= +go.uber.org/atomic v1.11.0/go.mod h1:LUxbIzbOniOlMKjJjyPfpl4v+PKK2cNJn91OQbhoJI0= go.uber.org/goleak v1.1.10/go.mod h1:8a7PlsEVH3e/a/GLqe5IIrQx6GzcnRmZEufDUTk4A7A= go.uber.org/multierr v1.6.0/go.mod h1:cdWPpRnG4AhwMwsgIHip0KRBQjJy5kYEpYjJxpXp9iU= go.uber.org/multierr v1.9.0 h1:7fIwc/ZtS0q++VgcfqFDxSBZVv/Xo49/SYnDFupUwlI= diff --git a/managed/models/models.go b/managed/models/models.go index 871e3766b3..b65d461cd4 100644 --- a/managed/models/models.go +++ b/managed/models/models.go @@ -230,3 +230,10 @@ const ( Bool = ParamType("bool") String = ParamType("string") ) + +type ParamUnit string + +const ( + Percent = ParamUnit("%") + Seconds = ParamUnit("s") +) diff --git a/managed/models/template_helpers.go b/managed/models/template_helpers.go index e89f749f2d..2aae1437b6 100644 --- a/managed/models/template_helpers.go +++ b/managed/models/template_helpers.go @@ -44,17 +44,15 @@ func checkUniqueTemplateName(q *reform.Querier, name string) error { } // FindTemplates returns saved notification rule templates. -func FindTemplates(q *reform.Querier) ([]Template, error) { +func FindTemplates(q *reform.Querier) ([]*Template, error) { structs, err := q.SelectAllFrom(TemplateTable, "") if err != nil { return nil, errors.Wrap(err, "failed to select notification rule templates") } - templates := make([]Template, len(structs)) + templates := make([]*Template, len(structs)) for i, s := range structs { - c := s.(*Template) //nolint:forcetypeassert - - templates[i] = *c + templates[i] = s.(*Template) //nolint:forcetypeassert } return templates, nil @@ -81,7 +79,6 @@ func FindTemplateByName(q *reform.Querier, name string) (*Template, error) { // CreateTemplateParams are params for creating new rule template. type CreateTemplateParams struct { Template *alert.Template - Yaml string Source Source } @@ -92,33 +89,13 @@ func CreateTemplate(q *reform.Querier, params *CreateTemplateParams) (*Template, return nil, status.Errorf(codes.InvalidArgument, "Invalid rule template: %v.", err) } - if err := checkUniqueTemplateName(q, params.Template.Name); err != nil { + if err := checkUniqueTemplateName(q, template.Name); err != nil { return nil, err } - p, err := ConvertParamsDefinitions(params.Template.Params) + row, err := ConvertTemplate(template, params.Source) if err != nil { - return nil, status.Errorf(codes.InvalidArgument, "Invalid rule template parameters: %v.", err) - } - - row := &Template{ - Name: template.Name, - Version: template.Version, - Summary: template.Summary, - Expr: template.Expr, - Params: p, - For: time.Duration(template.For), - Severity: Severity(template.Severity), - Source: params.Source, - Yaml: params.Yaml, - } - - if err := row.SetLabels(template.Labels); err != nil { - return nil, err - } - - if err := row.SetAnnotations(template.Annotations); err != nil { - return nil, err + return nil, status.Errorf(codes.InvalidArgument, "Failed to convert template: %v.", err) } if err = q.Insert(row); err != nil { @@ -132,7 +109,6 @@ func CreateTemplate(q *reform.Querier, params *CreateTemplateParams) (*Template, type ChangeTemplateParams struct { Template *alert.Template Name string - Yaml string } // ChangeTemplate updates existing rule template. @@ -151,6 +127,11 @@ func ChangeTemplate(q *reform.Querier, params *ChangeTemplateParams) (*Template, return nil, status.Errorf(codes.InvalidArgument, "Invalid rule template: %v.", err) } + yaml, err := alert.ToYAML([]alert.Template{*template}) + if err != nil { + return nil, errors.WithStack(err) + } + p, err := ConvertParamsDefinitions(params.Template.Params) if err != nil { return nil, status.Errorf(codes.InvalidArgument, "Invalid rule template parameters: %v.", err) @@ -163,7 +144,7 @@ func ChangeTemplate(q *reform.Querier, params *ChangeTemplateParams) (*Template, row.Params = p row.For = time.Duration(template.For) row.Severity = Severity(template.Severity) - row.Yaml = params.Yaml + row.Yaml = yaml if err = row.SetLabels(template.Labels); err != nil { return nil, err @@ -193,6 +174,40 @@ func RemoveTemplate(q *reform.Querier, name string) error { return nil } +func ConvertTemplate(template *alert.Template, source Source) (*Template, error) { + p, err := ConvertParamsDefinitions(template.Params) + if err != nil { + return nil, errors.Errorf("invalid rule template parameters: %v.", err) + } + + yaml, err := alert.ToYAML([]alert.Template{*template}) + if err != nil { + return nil, errors.WithStack(err) + } + + res := &Template{ + Name: template.Name, + Version: template.Version, + Summary: template.Summary, + Expr: template.Expr, + Params: p, + For: time.Duration(template.For), + Severity: Severity(template.Severity), + Source: source, + Yaml: yaml, + } + + if err := res.SetLabels(template.Labels); err != nil { + return nil, err + } + + if err := res.SetAnnotations(template.Annotations); err != nil { + return nil, err + } + + return res, nil +} + // ConvertParamsDefinitions converts parameters definitions to the model. func ConvertParamsDefinitions(params []alert.Parameter) (AlertExprParamsDefinitions, error) { res := make(AlertExprParamsDefinitions, 0, len(params)) @@ -200,7 +215,7 @@ func ConvertParamsDefinitions(params []alert.Parameter) (AlertExprParamsDefiniti p := AlertExprParamDefinition{ Name: param.Name, Summary: param.Summary, - Unit: string(param.Unit), + Unit: ParamUnit(param.Unit), Type: ParamType(param.Type), } diff --git a/managed/models/template_helpers_test.go b/managed/models/template_helpers_test.go index 9e7bc11465..b364ae06ba 100644 --- a/managed/models/template_helpers_test.go +++ b/managed/models/template_helpers_test.go @@ -60,7 +60,7 @@ func TestRuleTemplates(t *testing.T) { models.AlertExprParamsDefinitions{{ Name: params.Template.Params[0].Name, Summary: params.Template.Params[0].Summary, - Unit: string(params.Template.Params[0].Unit), + Unit: models.ParamUnit(params.Template.Params[0].Unit), Type: models.Float, FloatParam: &models.FloatParam{ Default: pointer.ToFloat64(params.Template.Params[0].Value.(float64)), @@ -110,7 +110,7 @@ func TestRuleTemplates(t *testing.T) { models.AlertExprParamsDefinitions{{ Name: updateParams.Template.Params[0].Name, Summary: updateParams.Template.Params[0].Summary, - Unit: string(updateParams.Template.Params[0].Unit), + Unit: models.ParamUnit(updateParams.Template.Params[0].Unit), Type: models.Float, FloatParam: &models.FloatParam{ Default: pointer.ToFloat64(updateParams.Template.Params[0].Value.(float64)), diff --git a/managed/models/template_model.go b/managed/models/template_model.go index 71dc652228..622028827a 100644 --- a/managed/models/template_model.go +++ b/managed/models/template_model.go @@ -111,7 +111,7 @@ func (p *AlertExprParamsDefinitions) Scan(src interface{}) error { return jsonSc type AlertExprParamDefinition struct { Name string `json:"name"` Summary string `json:"summary"` - Unit string `json:"unit"` + Unit ParamUnit `json:"unit"` Type ParamType `json:"type"` FloatParam *FloatParam `json:"float_param"` diff --git a/managed/services/management/alerting/service.go b/managed/services/management/alerting/service.go index 040e5188bf..47e2f23fe3 100644 --- a/managed/services/management/alerting/service.go +++ b/managed/services/management/alerting/service.go @@ -32,7 +32,6 @@ import ( "github.com/AlekSi/pointer" "github.com/percona-platform/saas/pkg/alert" "github.com/percona-platform/saas/pkg/common" - "github.com/percona/promconfig" "github.com/pkg/errors" "github.com/sirupsen/logrus" "google.golang.org/grpc/codes" @@ -59,18 +58,6 @@ const ( dirPerm = os.FileMode(0o775) ) -// TemplateInfo represents alerting rule template information from various sources. -// -// TODO We already have models.Template, iav1beta1.Template, and alert.Template. -// -// We probably can remove that type. -type TemplateInfo struct { - alert.Template - Yaml string - Source alerting.TemplateSource - CreatedAt *time.Time -} - // Service is responsible alerting templates and rules creation from them. type Service struct { db *reform.DB @@ -81,7 +68,7 @@ type Service struct { platformPublicKeys []string rw sync.RWMutex - templates map[string]TemplateInfo + templates map[string]models.Template alerting.UnimplementedAlertingServer } @@ -108,7 +95,7 @@ func NewService(db *reform.DB, platformClient *platform.Client, grafanaClient gr grafanaClient: grafanaClient, userTemplatesPath: templatesDir, platformPublicKeys: platformPublicKeys, - templates: make(map[string]TemplateInfo), + templates: make(map[string]models.Template), } return s, nil @@ -125,11 +112,11 @@ func (s *Service) Enabled() bool { } // GetTemplates return collected templates. -func (s *Service) GetTemplates() map[string]TemplateInfo { +func (s *Service) GetTemplates() map[string]models.Template { s.rw.RLock() defer s.rw.RUnlock() - res := make(map[string]TemplateInfo, len(s.templates)) + res := make(map[string]models.Template, len(s.templates)) for n, r := range s.templates { res[n] = r } @@ -142,23 +129,27 @@ func (s *Service) GetTemplates() map[string]TemplateInfo { // User file templates: read from yaml files created by the user in `/srv/alerting/templates`. // User API templates: in the DB created using the API. func (s *Service) CollectTemplates(ctx context.Context) { + var templates []*models.Template builtInTemplates, err := s.loadTemplatesFromAssets(ctx) if err != nil { s.l.Errorf("Failed to load built-in rule templates: %s.", err) return } + templates = append(templates, builtInTemplates...) userDefinedTemplates, err := s.loadTemplatesFromUserFiles(ctx) if err != nil { s.l.Errorf("Failed to load user-defined rule templates: %s.", err) return } + templates = append(templates, userDefinedTemplates...) dbTemplates, err := s.loadTemplatesFromDB() if err != nil { s.l.Errorf("Failed to load rule templates from DB: %s.", err) return } + templates = append(templates, dbTemplates...) saasTemplates, err := s.downloadTemplates(ctx) if err != nil { @@ -166,48 +157,20 @@ func (s *Service) CollectTemplates(ctx context.Context) { // we should still collect and show the Built-In templates. s.l.Errorf("Failed to download rule templates from SaaS: %s.", err) } - - templates := make([]TemplateInfo, 0, len(builtInTemplates)+len(userDefinedTemplates)+len(dbTemplates)+len(saasTemplates)) - - for _, t := range builtInTemplates { - templates = append(templates, TemplateInfo{ - Template: t, - Source: alerting.TemplateSource_BUILT_IN, - }) - } - - for _, t := range userDefinedTemplates { - templates = append(templates, TemplateInfo{ - Template: t, - Source: alerting.TemplateSource_USER_FILE, - }) - } - - for _, t := range saasTemplates { - templates = append(templates, TemplateInfo{ - Template: t, - Source: alerting.TemplateSource_SAAS, - }) - } - - templates = append(templates, dbTemplates...) + templates = append(templates, saasTemplates...) // replace previously stored templates with newly collected ones. s.rw.Lock() defer s.rw.Unlock() - s.templates = make(map[string]TemplateInfo, len(templates)) + s.templates = make(map[string]models.Template, len(templates)) for _, t := range templates { - // TODO Check for name clashes? Allow users to re-define built-in templates? - // Reserve prefix for built-in or user-defined templates? - // https://jira.percona.com/browse/PMM-7023 - - s.templates[t.Name] = t + s.templates[t.Name] = *t } } // loadTemplatesFromAssets loads built-in alerting rule templates from pmm-managed binary's assets. -func (s *Service) loadTemplatesFromAssets(ctx context.Context) ([]alert.Template, error) { - var res []alert.Template +func (s *Service) loadTemplatesFromAssets(ctx context.Context) ([]*models.Template, error) { + var res []*models.Template walkDirFunc := func(path string, d fs.DirEntry, err error) error { if err != nil { return errors.Wrapf(err, "error occurred while traversing templates folder: %s", path) @@ -259,24 +222,28 @@ func (s *Service) loadTemplatesFromAssets(ctx context.Context) ([]alert.Template return errors.Errorf("%s %q: template should contain exactly two annotations: summary and description", path, t.Name) } - res = append(res, t) + tm, err := models.ConvertTemplate(&t, models.BuiltInSource) + if err != nil { + return errors.Wrap(err, "failed to convert alert rule template") + } + + res = append(res, tm) return nil } - err := fs.WalkDir(data.IATemplates, ".", walkDirFunc) - if err != nil { + if err := fs.WalkDir(data.IATemplates, ".", walkDirFunc); err != nil { return nil, err } return res, nil } // loadTemplatesFromUserFiles loads user's alerting rule templates from /srv/alerting/templates. -func (s *Service) loadTemplatesFromUserFiles(ctx context.Context) ([]alert.Template, error) { +func (s *Service) loadTemplatesFromUserFiles(ctx context.Context) ([]*models.Template, error) { paths, err := dir.FindFilesWithExtensions(s.userTemplatesPath, "yml", "yaml") if err != nil { return nil, errors.Wrap(err, "failed to get paths") } - res := make([]alert.Template, 0, len(paths)) + res := make([]*models.Template, 0, len(paths)) for _, path := range paths { if ctx.Err() != nil { return nil, ctx.Err() @@ -300,89 +267,39 @@ func (s *Service) loadTemplatesFromUserFiles(ctx context.Context) ([]alert.Templ } for _, t := range templates { - if err = validateUserTemplate(&t); err != nil { //nolint:gosec + t := t + if err = validateUserTemplate(&t); err != nil { s.l.Warnf("%s %s", path, err) continue } - res = append(res, t) + tm, err := models.ConvertTemplate(&t, models.UserFileSource) + if err != nil { + return nil, errors.Wrap(err, "failed to convert alert rule template") + } + + res = append(res, tm) } } return res, nil } -func (s *Service) loadTemplatesFromDB() ([]TemplateInfo, error) { - var templates []models.Template - e := s.db.InTransaction(func(tx *reform.TX) error { +func (s *Service) loadTemplatesFromDB() ([]*models.Template, error) { + var templates []*models.Template + errTx := s.db.InTransaction(func(tx *reform.TX) error { var err error templates, err = models.FindTemplates(tx.Querier) return err }) - if e != nil { - return nil, errors.Wrap(e, "failed to load rule templates from DB") + if errTx != nil { + return nil, errors.Wrap(errTx, "failed to load rule templates from DB") } - res := make([]TemplateInfo, 0, len(templates)) - for _, t := range templates { - t := t - params := make([]alert.Parameter, 0, len(t.Params)) - for _, param := range t.Params { - p := alert.Parameter{ - Name: param.Name, - Summary: param.Summary, - Unit: alert.Unit(param.Unit), - Type: alert.Type(param.Type), - } - - if alert.Type(param.Type) == alert.Float { - f := param.FloatParam - - if f.Default != nil { - p.Value = *f.Default - } - - if f.Min != nil && f.Max != nil { - p.Range = []interface{}{*f.Min, *f.Max} - } - } - - params = append(params, p) - } - - labels, err := t.GetLabels() - if err != nil { - return nil, errors.Wrap(err, "failed to load template labels") - } - - annotations, err := t.GetAnnotations() - if err != nil { - return nil, errors.Wrap(err, "failed to load template annotations") - } - - res = append(res, - TemplateInfo{ - Template: alert.Template{ - Name: t.Name, - Version: t.Version, - Summary: t.Summary, - Expr: t.Expr, - Params: params, - For: promconfig.Duration(t.For), - Severity: common.Severity(t.Severity), - Labels: labels, - Annotations: annotations, - }, - Yaml: t.Yaml, - Source: convertSource(t.Source), - CreatedAt: &t.CreatedAt, - }, - ) - } - return res, nil + return templates, nil } -// downloadTemplates downloads IA templates from SaaS. -func (s *Service) downloadTemplates(ctx context.Context) ([]alert.Template, error) { +// downloadTemplates downloads alerting templates from SaaS. +func (s *Service) downloadTemplates(ctx context.Context) ([]*models.Template, error) { settings, err := models.GetSettings(s.db) if err != nil { return nil, err @@ -415,7 +332,17 @@ func (s *Service) downloadTemplates(ctx context.Context) ([]alert.Template, erro return nil, err } - return templates, nil + res := make([]*models.Template, 0, len(templates)) + for _, t := range templates { + t := t + tm, err := models.ConvertTemplate(&t, models.SAASSource) + if err != nil { + return nil, errors.Wrap(err, "failed to convert alert rule template") + } + res = append(res, tm) + } + + return res, nil } // validateUserTemplate validates user-provided template (API or file). @@ -468,14 +395,19 @@ func convertSource(source models.Source) alerting.TemplateSource { } } -func convertParamType(t alert.Type) alerting.ParamType { - // TODO: add another types. +func convertParamType(t models.ParamType) alerting.ParamType { switch t { - case alert.Float: + case models.Float: return alerting.ParamType_FLOAT - default: - return alerting.ParamType_PARAM_TYPE_INVALID + case models.Bool: + return alerting.ParamType_BOOL + case models.String: + return alerting.ParamType_STRING } + + // do not add `default:` to make exhaustive linter do its job + + return alerting.ParamType_PARAM_TYPE_INVALID } // ListTemplates returns a list of all collected Alert Rule Templates. @@ -544,32 +476,34 @@ 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.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.Errorf(codes.InvalidArgument, "Failed to parse rule template: %v.", err) } + uniqueNames := make(map[string]struct{}, len(templates)) for _, t := range templates { + if _, ok := uniqueNames[t.Name]; ok { + return nil, status.Errorf(codes.InvalidArgument, "Template with name '%s' declared more that once.", t.Name) + } + uniqueNames[t.Name] = struct{}{} if err = validateUserTemplate(&t); err != nil { //nolint:gosec return nil, status.Errorf(codes.InvalidArgument, "%s.", err) } } - params := &models.CreateTemplateParams{ - Template: &templates[0], - Yaml: req.Yaml, - Source: models.UserAPISource, - } - - e := s.db.InTransaction(func(tx *reform.TX) error { - var err error - _, err = models.CreateTemplate(tx.Querier, params) - return err + errTx := s.db.InTransaction(func(tx *reform.TX) error { + for _, t := range templates { + t := t + if _, err = models.CreateTemplate(tx.Querier, &models.CreateTemplateParams{ + Template: &t, + Source: models.UserAPISource, + }); err != nil { + return err + } + } + return nil }) - if e != nil { - return nil, e + if errTx != nil { + return nil, errTx } s.CollectTemplates(ctx) @@ -603,7 +537,6 @@ func (s *Service) UpdateTemplate(ctx context.Context, req *alerting.UpdateTempla changeParams := &models.ChangeTemplateParams{ Template: &tmpl, Name: req.Name, - Yaml: req.Yaml, } e := s.db.InTransaction(func(tx *reform.TX) error { @@ -634,37 +567,38 @@ func (s *Service) DeleteTemplate(ctx context.Context, req *alerting.DeleteTempla return &alerting.DeleteTemplateResponse{}, nil } -func convertTemplate(l *logrus.Entry, template TemplateInfo) (*alerting.Template, error) { - var err error +func convertTemplate(l *logrus.Entry, template models.Template) (*alerting.Template, error) { + labels, err := template.GetLabels() + if err != nil { + return nil, errors.WithStack(err) + } + annotations, err := template.GetAnnotations() + if err != nil { + return nil, errors.WithStack(err) + } + t := &alerting.Template{ Name: template.Name, Summary: template.Summary, Expr: template.Expr, - Params: make([]*alerting.ParamDefinition, 0, len(template.Params)), - For: durationpb.New(time.Duration(template.For)), + Params: convertParamDefinitions(l, template.Params), + For: durationpb.New(template.For), Severity: managementpb.Severity(template.Severity), - Labels: template.Labels, - Annotations: template.Annotations, - Source: template.Source, + Labels: labels, + Annotations: annotations, + Source: convertSource(template.Source), Yaml: template.Yaml, } - if template.CreatedAt != nil { - t.CreatedAt = timestamppb.New(*template.CreatedAt) - if err = t.CreatedAt.CheckValid(); err != nil { - return nil, err - } - } - - t.Params, err = convertParamDefinitions(l, template.Params) - if err != nil { + t.CreatedAt = timestamppb.New(template.CreatedAt) + if err = t.CreatedAt.CheckValid(); err != nil { return nil, err } return t, nil } -func convertParamDefinitions(l *logrus.Entry, params []alert.Parameter) ([]*alerting.ParamDefinition, error) { +func convertParamDefinitions(l *logrus.Entry, params models.AlertExprParamsDefinitions) []*alerting.ParamDefinition { res := make([]*alerting.ParamDefinition, 0, len(params)) for _, p := range params { pd := &alerting.ParamDefinition{ @@ -674,37 +608,37 @@ func convertParamDefinitions(l *logrus.Entry, params []alert.Parameter) ([]*aler Type: convertParamType(p.Type), } - var err error switch p.Type { - case alert.Float: + case models.Float: var fp alerting.FloatParamDefinition - if p.Value != nil { - fp.Default, err = p.GetValueForFloat() - if err != nil { - return nil, errors.Wrap(err, "failed to get value for float parameter") + if p.FloatParam != nil { + if p.FloatParam.Default != nil { + fp.Default = *p.FloatParam.Default + fp.HasDefault = true + } + + if p.FloatParam.Min != nil { + fp.Min = *p.FloatParam.Min + fp.HasMin = true } - fp.HasDefault = true - } - if len(p.Range) != 0 { - fp.Min, fp.Max, err = p.GetRangeForFloat() - if err != nil { - return nil, errors.Wrap(err, "failed to get range for float parameter") + if p.FloatParam.Max != nil { + fp.Max = *p.FloatParam.Max + fp.HasMax = true } - fp.HasMin, fp.HasMax = true, true } pd.Value = &alerting.ParamDefinition_Float{Float: &fp} res = append(res, pd) - case alert.Bool, alert.String: + case models.Bool, models.String: l.Warnf("Skipping unsupported parameter type %q.", p.Type) } // do not add `default:` to make exhaustive linter do its job } - return res, nil + return res } // CreateRule creates alert rule from the given template. @@ -736,21 +670,16 @@ func (s *Service) CreateRule(ctx context.Context, req *alerting.CreateRuleReques return nil, status.Errorf(codes.NotFound, "Unknown template %s.", req.TemplateName) } - paramsDefinitions, err := models.ConvertParamsDefinitions(template.Params) - if err != nil { - return nil, err - } - paramsValues, err := convertParamsValuesToModel(req.Params) if err != nil { return nil, err } - if err := validateParameters(paramsDefinitions, paramsValues); err != nil { + if err := validateParameters(template.Params, paramsValues); err != nil { return nil, err } - forDuration := time.Duration(template.For) + forDuration := template.For if req.For != nil { forDuration = req.For.AsDuration() } @@ -771,9 +700,14 @@ func (s *Service) CreateRule(ctx context.Context, req *alerting.CreateRuleReques } } + ta, err := template.GetAnnotations() + if err != nil { + return nil, errors.Wrap(err, "failed to get template annotations") + } + // Copy annotations form template annotations := make(map[string]string) - if err = transformMaps(template.Annotations, annotations, paramsValues.AsStringMap()); err != nil { + if err = transformMaps(ta, annotations, paramsValues.AsStringMap()); err != nil { return nil, errors.Wrap(err, "failed to fill template annotations placeholders") } @@ -783,8 +717,13 @@ func (s *Service) CreateRule(ctx context.Context, req *alerting.CreateRuleReques return nil, errors.Wrap(err, "failed to fill rule labels placeholders") } + tl, err := template.GetLabels() + if err != nil { + return nil, errors.Wrap(err, "failed to get template labels") + } + // Add rule labels - if err = transformMaps(template.Labels, labels, paramsValues.AsStringMap()); err != nil { + if err = transformMaps(tl, labels, paramsValues.AsStringMap()); err != nil { return nil, errors.Wrap(err, "failed to fill template labels placeholders") } @@ -869,11 +808,11 @@ func transformMaps(src map[string]string, dest map[string]string, data map[strin return nil } -func convertParamUnit(u alert.Unit) alerting.ParamUnit { +func convertParamUnit(u models.ParamUnit) alerting.ParamUnit { switch u { - case alert.Percentage: + case models.Percent: return alerting.ParamUnit_PERCENTAGE - case alert.Seconds: + case models.Seconds: return alerting.ParamUnit_SECONDS } diff --git a/managed/services/management/ia/deps.go b/managed/services/management/ia/deps.go index 12f19bc0a4..1a9d39b33d 100644 --- a/managed/services/management/ia/deps.go +++ b/managed/services/management/ia/deps.go @@ -19,8 +19,8 @@ import ( "context" "github.com/percona/pmm/api/alertmanager/ammodels" + "github.com/percona/pmm/managed/models" "github.com/percona/pmm/managed/services" - "github.com/percona/pmm/managed/services/management/alerting" ) //go:generate ../../../../bin/mockery --name=alertManager --case=snake --inpackage --testonly @@ -44,5 +44,5 @@ type vmAlert interface { } type templatesService interface { - GetTemplates() map[string]alerting.TemplateInfo + GetTemplates() map[string]models.Template } diff --git a/managed/services/management/ia/mock_templates_service_test.go b/managed/services/management/ia/mock_templates_service_test.go index 11c5f28e0e..478779eda2 100644 --- a/managed/services/management/ia/mock_templates_service_test.go +++ b/managed/services/management/ia/mock_templates_service_test.go @@ -5,7 +5,7 @@ package ia import ( mock "github.com/stretchr/testify/mock" - alerting "github.com/percona/pmm/managed/services/management/alerting" + models "github.com/percona/pmm/managed/models" ) // mockTemplatesService is an autogenerated mock type for the templatesService type @@ -14,15 +14,15 @@ type mockTemplatesService struct { } // GetTemplates provides a mock function with given fields: -func (_m *mockTemplatesService) GetTemplates() map[string]alerting.TemplateInfo { +func (_m *mockTemplatesService) GetTemplates() map[string]models.Template { ret := _m.Called() - var r0 map[string]alerting.TemplateInfo - if rf, ok := ret.Get(0).(func() map[string]alerting.TemplateInfo); ok { + var r0 map[string]models.Template + if rf, ok := ret.Get(0).(func() map[string]models.Template); ok { r0 = rf() } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(map[string]alerting.TemplateInfo) + r0 = ret.Get(0).(map[string]models.Template) } } diff --git a/managed/services/management/ia/rules_service.go b/managed/services/management/ia/rules_service.go index 134b2c60a5..7ea8f672a1 100644 --- a/managed/services/management/ia/rules_service.go +++ b/managed/services/management/ia/rules_service.go @@ -22,7 +22,6 @@ import ( "os" "path/filepath" "strings" - "time" "github.com/AlekSi/pointer" "github.com/percona-platform/saas/pkg/alert" @@ -384,14 +383,18 @@ func (s *RulesService) CreateAlertRule(ctx context.Context, req *iav1beta1.Creat params.TemplateName = template.Name params.Summary = template.Summary params.ExprTemplate = template.Expr - params.DefaultFor = time.Duration(template.For) - params.DefaultSeverity = models.Severity(template.Severity) - params.Labels = template.Labels - params.Annotations = template.Annotations + params.DefaultFor = template.For + params.DefaultSeverity = template.Severity + params.ParamsDefinitions = template.Params - params.ParamsDefinitions, err = models.ConvertParamsDefinitions(template.Params) + params.Labels, err = template.GetLabels() if err != nil { - return nil, err + return nil, errors.WithStack(err) + } + + params.Annotations, err = template.GetAnnotations() + if err != nil { + return nil, errors.WithStack(err) } } else { sourceRule, err := models.FindRuleByID(s.db.Querier, req.SourceRuleId)