From d08124476f3ec0716dadafe6bc8005767439be51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20Serv=C3=A9n=20Mar=C3=ADn?= Date: Sun, 14 Nov 2021 22:06:13 +0100 Subject: [PATCH 1/4] rules/spec.yaml: fix enum MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rules are either Recording Rules or Alerting Rules, but not both. This commit fixes this fact in the OpenAPI specification. Signed-off-by: Lucas Servén Marín --- rules/spec.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rules/spec.yaml b/rules/spec.yaml index d8864b017..6c2ff8176 100644 --- a/rules/spec.yaml +++ b/rules/spec.yaml @@ -78,7 +78,7 @@ components: rules: type: array items: - anyOf: + oneOf: - $ref: '#/components/schemas/RecordingRule' - $ref: '#/components/schemas/AlertingRule' RecordingRule: From e3cbf2d4d374f1e896b9de7bb65897872615a839 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20Serv=C3=A9n=20Mar=C3=ADn?= Date: Sun, 14 Nov 2021 22:07:12 +0100 Subject: [PATCH 2/4] rules: add custom UnmarshalJSON function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, when a group of rules is unmarshaled, the concrete type in Go will never be a rules.RecordingRule or a rules.AlertingRule, although those types are defined in the rules package. Instead those rules will always be some map[string]interface{} type. This means that it's not possible for users of the package to perform type assertions on the rules that are fetched via the client, which is a serious annoyance for anyone using the package. This commit fixes this by providing a custom unmarshaling function. Furthermore, in order to fix unmarshaling via both JSON and YAML, the commit switches the YAML package to ghodss/yaml, which is a superset of the current YAML package, go-yaml/yaml. Signed-off-by: Lucas Servén Marín --- Makefile | 2 +- go.mod | 1 - rules/custom_types.go | 50 +++++++++ rules/custom_types_test.go | 209 +++++++++++++++++++++++++++++++++++++ rules/rules.go | 1 + 5 files changed, 261 insertions(+), 2 deletions(-) create mode 100644 rules/custom_types.go create mode 100644 rules/custom_types_test.go diff --git a/Makefile b/Makefile index 070752cc5..caa5890c6 100644 --- a/Makefile +++ b/Makefile @@ -204,4 +204,4 @@ jsonnet-fmt: | $(JSONNETFMT) PATH=$$PATH:$(BIN_DIR):$(FIRST_GOPATH)/bin echo ${JSONNET_SRC} | xargs -n 1 -- $(JSONNETFMT_CMD) -i rules/rules.go: $(OAPI_CODEGEN) rules/spec.yaml - $(OAPI_CODEGEN) -generate types,client,chi-server -package rules -o $@ rules/spec.yaml + $(OAPI_CODEGEN) -generate types,client,chi-server -package rules rules/spec.yaml | sed 's|gopkg.in/yaml.v2|github.com/ghodss/yaml|g' | gofmt -s > $@ diff --git a/go.mod b/go.mod index a71420ce0..8c1ab5423 100644 --- a/go.mod +++ b/go.mod @@ -41,7 +41,6 @@ require ( google.golang.org/grpc v1.38.0 google.golang.org/protobuf v1.27.1 gopkg.in/square/go-jose.v2 v2.4.1 // indirect - gopkg.in/yaml.v2 v2.4.0 k8s.io/apimachinery v0.21.1 k8s.io/apiserver v0.21.1 k8s.io/client-go v0.21.1 diff --git a/rules/custom_types.go b/rules/custom_types.go new file mode 100644 index 000000000..54035c6c2 --- /dev/null +++ b/rules/custom_types.go @@ -0,0 +1,50 @@ +package rules + +import ( + "encoding/json" +) + +func (rg *RuleGroup) UnmarshalJSON(data []byte) error { + raw := struct { + Interval string `json:"interval"` + Name string `json:"name"` + Rules []json.RawMessage `json:"rules"` + }{} + if err := json.Unmarshal(data, &raw); err != nil { + return err + } + + rg.Interval = raw.Interval + rg.Name = raw.Name + rules := make([]interface{}, 0, len(raw.Rules)) + + for i := range raw.Rules { + rawRule := make(map[string]json.RawMessage) + if err := json.Unmarshal(raw.Rules[i], &rawRule); err != nil { + return err + } + + switch _, ok := rawRule["alert"]; ok { + case true: + var ar AlertingRule + if err := json.Unmarshal(raw.Rules[i], &ar); err != nil { + return err + } + + rules = append(rules, ar) + case false: + var rr RecordingRule + if err := json.Unmarshal(raw.Rules[i], &rr); err != nil { + return err + } + + rules = append(rules, rr) + } + } + + if len(rules) != 0 { + rg.Rules = rules + } + + return nil +} diff --git a/rules/custom_types_test.go b/rules/custom_types_test.go new file mode 100644 index 000000000..43b814b9e --- /dev/null +++ b/rules/custom_types_test.go @@ -0,0 +1,209 @@ +package rules + +import ( + "testing" + + "github.com/ghodss/yaml" +) + +func TestRuleGroupUnmarshalJSON(t *testing.T) { + for _, testCase := range []struct { + name string + raw []byte + out RuleGroup + err bool + }{ + { + name: "almost empty", + raw: []byte("{}"), + }, + { + name: "one recording rule", + raw: []byte(` +name: foo +interval: 5s +rules: +- record: bar + expr: vector(1)`), + out: RuleGroup{ + Name: "foo", + Interval: "5s", + Rules: []interface{}{ + RecordingRule{ + Record: "bar", + Expr: "vector(1)", + Labels: RecordingRule_Labels{AdditionalProperties: make(map[string]string)}, + }, + }, + }, + }, + { + name: "one alerting rule", + raw: []byte(` +name: foo +interval: 5s +rules: +- alert: HighRequestLatency + expr: job:request_latency_seconds:mean5m{job="myjob"} > 0.5 + for: 10m`), + out: RuleGroup{ + Name: "foo", + Interval: "5s", + Rules: []interface{}{ + AlertingRule{ + Alert: "HighRequestLatency", + Expr: `job:request_latency_seconds:mean5m{job="myjob"} > 0.5`, + For: "10m", + Annotations: AlertingRule_Annotations{AdditionalProperties: make(map[string]string)}, + Labels: AlertingRule_Labels{AdditionalProperties: make(map[string]string)}, + }, + }, + }, + }, + { + name: "one of each", + raw: []byte(` +name: foo +interval: 5s +rules: +- record: bar + expr: vector(1) +- alert: HighRequestLatency + expr: job:request_latency_seconds:mean5m{job="myjob"} > 0.5 + for: 10m`), + out: RuleGroup{ + Name: "foo", + Interval: "5s", + Rules: []interface{}{ + RecordingRule{ + Record: "bar", + Expr: "vector(1)", + Labels: RecordingRule_Labels{AdditionalProperties: make(map[string]string)}, + }, + AlertingRule{ + Alert: "HighRequestLatency", + Expr: `job:request_latency_seconds:mean5m{job="myjob"} > 0.5`, + For: "10m", + Annotations: AlertingRule_Annotations{AdditionalProperties: make(map[string]string)}, + Labels: AlertingRule_Labels{AdditionalProperties: make(map[string]string)}, + }, + }, + }, + }, + } { + tc := testCase + t.Run(tc.name, func(t *testing.T) { + var out RuleGroup + if err := yaml.Unmarshal(tc.raw, &out); err != nil { + if !tc.err { + t.Fatalf("got unexpected error %v", err) + } + } else { + if tc.err { + t.Fatal("expected error") + } + if !ruleGroupsEqual(out, tc.out) { + t.Errorf("expected %v; got %v", tc.out, out) + } + } + }) + } +} + +func ruleGroupsEqual(a, b RuleGroup) bool { + if a.Interval != b.Interval { + return false + } + + if a.Name != b.Name { + return false + } + + if (a.Rules != nil) != (b.Rules != nil) { + return false + } + + if len(a.Rules) != len(b.Rules) { + return false + } + + for i := range a.Rules { + ara, aok := a.Rules[i].(AlertingRule) + bra, bok := b.Rules[i].(AlertingRule) + + if aok != bok { + return false + } + + if aok { + if ara.Alert != bra.Alert { + return false + } + + if ara.Expr != bra.Expr { + return false + } + + if ara.For != bra.For { + return false + } + + if mapsEqual(ara.Annotations.AdditionalProperties, bra.Annotations.AdditionalProperties) { + return false + } + + if mapsEqual(ara.Labels.AdditionalProperties, bra.Labels.AdditionalProperties) { + return false + } + + continue + } + + arr, aok := a.Rules[i].(RecordingRule) + brr, bok := b.Rules[i].(RecordingRule) + + if aok != bok { + return false + } + + if aok { + if arr.Expr != brr.Expr { + return false + } + + if arr.Record != brr.Record { + return false + } + + if mapsEqual(arr.Labels.AdditionalProperties, brr.Labels.AdditionalProperties) { + return false + } + + continue + } + } + + return true +} + +func mapsEqual(a, b map[string]string) bool { + if a == nil && b == nil { + return true + } + + if (a != nil) != (b != nil) { + return false + } + + if len(a) != len(b) { + return false + } + + for k := range a { + if a[k] != b[k] { + return false + } + } + + return true +} diff --git a/rules/rules.go b/rules/rules.go index 50ca26f91..418815eb6 100644 --- a/rules/rules.go +++ b/rules/rules.go @@ -741,3 +741,4 @@ func HandlerWithOptions(si ServerInterface, options ChiServerOptions) http.Handl return r } + From 5d6a45213091444e92c3cb449edd914160b97e3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20Serv=C3=A9n=20Mar=C3=ADn?= Date: Sun, 14 Nov 2021 22:07:00 +0100 Subject: [PATCH 3/4] rules/rules.go: regenerate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Lucas Servén Marín --- rules/rules.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/rules/rules.go b/rules/rules.go index 418815eb6..aeee8667f 100644 --- a/rules/rules.go +++ b/rules/rules.go @@ -13,7 +13,7 @@ import ( "net/url" "strings" - "gopkg.in/yaml.v2" + "github.com/ghodss/yaml" "github.com/deepmap/oapi-codegen/pkg/runtime" "github.com/go-chi/chi/v5" @@ -741,4 +741,3 @@ func HandlerWithOptions(si ServerInterface, options ChiServerOptions) http.Handl return r } - From d6fd21b023cf0c1ffaf1209213c7bfed50caf2a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20Serv=C3=A9n=20Mar=C3=ADn?= Date: Sun, 14 Nov 2021 22:12:20 +0100 Subject: [PATCH 4/4] api: enforce tenant on generated time series MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, recording and alerting rules that are fetched from the new rules/raw API will not have any tenant label attached to them. This means that when the rules are configured on a Thanos Ruler, the time series that are generated will not have a tenant label on them. This is a big problem, because without this tenant label, the very tenant to whom the rules belong cannot query for the time series, making them in effect useless. This commit enforces that all rules returned by the rules/raw endpoint have a tenant_id label attached to them. Note: this is a distinct problem from ensuring that the rules written by a tenant are guaranteed to only include data from that tenant. Currently, the rules from a tenant can select data from any tenant. This is a distinct problem and should absolutely be tackled in a follow up. Signed-off-by: Lucas Servén Marín --- api/metrics/v1/http.go | 10 +++++- api/metrics/v1/rules.go | 69 +++++++++++++++++++++++++++++++++++++++-- main.go | 1 + 3 files changed, 76 insertions(+), 4 deletions(-) diff --git a/api/metrics/v1/http.go b/api/metrics/v1/http.go index b8e1cd9ee..070565181 100644 --- a/api/metrics/v1/http.go +++ b/api/metrics/v1/http.go @@ -40,6 +40,7 @@ type handlerConfiguration struct { registry *prometheus.Registry instrument handlerInstrumenter spanRoutePrefix string + tenantLabel string queryMiddlewares []func(http.Handler) http.Handler readMiddlewares []func(http.Handler) http.Handler uiMiddlewares []func(http.Handler) http.Handler @@ -77,6 +78,13 @@ func WithSpanRoutePrefix(spanRoutePrefix string) HandlerOption { } } +// WithTenantLabel adds tenant label for the handler to use. +func WithTenantLabel(tenantLabel string) HandlerOption { + return func(h *handlerConfiguration) { + h.tenantLabel = tenantLabel + } +} + // WithReadMiddleware adds a middleware for all "matcher based" read operations (series, label names and values). func WithReadMiddleware(m func(http.Handler) http.Handler) HandlerOption { return func(h *handlerConfiguration) { @@ -284,7 +292,7 @@ func NewHandler(read, write, rulesEndpoint *url.URL, upstreamCA []byte, opts ... return r } - rh := rulesHandler{client: client} + rh := rulesHandler{client: client, logger: c.logger, tenantLabel: c.tenantLabel} r.Group(func(r chi.Router) { r.Use(c.uiMiddlewares...) diff --git a/api/metrics/v1/rules.go b/api/metrics/v1/rules.go index 210265897..385101cee 100644 --- a/api/metrics/v1/rules.go +++ b/api/metrics/v1/rules.go @@ -2,14 +2,20 @@ package v1 import ( "io" + "io/ioutil" "net/http" + "github.com/ghodss/yaml" + "github.com/go-kit/kit/log" + "github.com/go-kit/kit/log/level" "github.com/observatorium/api/authentication" "github.com/observatorium/api/rules" ) type rulesHandler struct { - client rules.ClientInterface + client rules.ClientInterface + logger log.Logger + tenantLabel string } func (rh *rulesHandler) get(w http.ResponseWriter, r *http.Request) { @@ -19,8 +25,16 @@ func (rh *rulesHandler) get(w http.ResponseWriter, r *http.Request) { return } + id, ok := authentication.GetTenantID(r.Context()) + if !ok { + http.Error(w, "error finding tenant ID", http.StatusUnauthorized) + return + } + resp, err := rh.client.ListRules(r.Context(), tenant) if err != nil { + level.Error(rh.logger).Log("msg", "could not list rules", "err", err.Error()) + sc := http.StatusInternalServerError if resp != nil { sc = resp.StatusCode @@ -33,8 +47,56 @@ func (rh *rulesHandler) get(w http.ResponseWriter, r *http.Request) { defer resp.Body.Close() - if _, err := io.Copy(w, resp.Body); err != nil { - http.Error(w, "error writing rules response", http.StatusInternalServerError) + if resp.StatusCode/100 != 2 { + http.Error(w, "error listing rules", resp.StatusCode) + return + } + + body, err := ioutil.ReadAll(resp.Body) + if err != nil { + http.Error(w, "error listing rules", http.StatusInternalServerError) + return + } + + var rawRules rules.Rules + if err := yaml.Unmarshal(body, &rawRules); err != nil { + level.Error(rh.logger).Log("msg", "could not unmarshal rules", "err", err.Error()) + http.Error(w, "error unmarshaling rules", http.StatusInternalServerError) + + return + } + + for i := range rawRules.Groups { + for j := range rawRules.Groups[i].Rules { + switch r := rawRules.Groups[i].Rules[j].(type) { + case rules.RecordingRule: + if r.Labels.AdditionalProperties == nil { + r.Labels.AdditionalProperties = make(map[string]string) + } + + r.Labels.AdditionalProperties[rh.tenantLabel] = id + rawRules.Groups[i].Rules[j] = r + case rules.AlertingRule: + if r.Labels.AdditionalProperties == nil { + r.Labels.AdditionalProperties = make(map[string]string) + } + + r.Labels.AdditionalProperties[rh.tenantLabel] = id + rawRules.Groups[i].Rules[j] = r + } + } + } + + body, err = yaml.Marshal(rawRules) + if err != nil { + level.Error(rh.logger).Log("msg", "could not marshal YAML", "err", err.Error()) + http.Error(w, "error marshaling YAML", http.StatusInternalServerError) + + return + } + + if _, err := w.Write(body); err != nil { + level.Error(rh.logger).Log("msg", "could not write body", "err", err.Error()) return } } @@ -52,6 +114,7 @@ func (rh *rulesHandler) put(w http.ResponseWriter, r *http.Request) { sc = resp.StatusCode } + level.Error(rh.logger).Log("msg", "could not set rules", "err", err.Error()) http.Error(w, "error creating rules", sc) return diff --git a/main.go b/main.go index 443a984f1..de9d16d50 100644 --- a/main.go +++ b/main.go @@ -548,6 +548,7 @@ func main() { metricsv1.WithRegistry(reg), metricsv1.WithHandlerInstrumenter(ins), metricsv1.WithSpanRoutePrefix("/api/metrics/v1/{tenant}"), + metricsv1.WithTenantLabel(cfg.metrics.tenantLabel), metricsv1.WithQueryMiddleware(authorization.WithAuthorizers(authorizers, rbac.Read, "metrics")), metricsv1.WithQueryMiddleware(metricsv1.WithEnforceTenancyOnQuery(cfg.metrics.tenantLabel)), metricsv1.WithReadMiddleware(authorization.WithAuthorizers(authorizers, rbac.Read, "metrics")),