Ruler Experimental API: Fix rule group validation (#3210)

* Ruler Experimental API: Fix rule group validation

Fixed a few cases where the API would allow us to submit a bad rule
group payload which would end up overwriting existing ones.

Signed-off-by: gotjosh <[email protected]>

* Add changelog

Signed-off-by: gotjosh <[email protected]>

* Appease the linter

Signed-off-by: gotjosh <[email protected]>

Co-authored-by: Marco Pracucci <[email protected]>

Author: gotjosh

CHANGELOG.md

@@ -14,6 +14,7 @@
 * [BUGFIX] Ruler: directories in the configured `rules-path` will be removed on startup and shutdown in order to ensure they don't persist between runs. #3195
 * [BUGFIX] Handle hash-collisions in the query path. #3192
 * [BUGFIX] Check for postgres rows errors. #3197
+* [BUGFIX] Ruler Experimental API: Don't allow rule groups without names or empty rule groups. #3210
 * [BUGFIX] Experimental Alertmanager API: Do not allow empty Alertmanager configurations or bad template filenames to be submitted through the configuration API. #3185
 
 ## 1.4.0-rc.0 in progress

pkg/ruler/api.go

@@ -7,6 +7,7 @@ import (
 	"net/url"
 	"sort"
 	"strconv"
+	"strings"
 	"time"
 
 	"github.com/go-kit/kit/log"
@@ -443,10 +444,13 @@ func (r *Ruler) CreateRuleGroup(w http.ResponseWriter, req *http.Request) {
 
 	errs := r.manager.ValidateRuleGroup(rg)
 	if len(errs) > 0 {
+		e := []string{}
 		for _, err := range errs {
 			level.Error(logger).Log("msg", "unable to validate rule group payload", "err", err.Error())
+			e = append(e, err.Error())
 		}
-		http.Error(w, errs[0].Error(), http.StatusBadRequest)
+
+		http.Error(w, strings.Join(e, ", "), http.StatusBadRequest)
 		return
 	}
 

pkg/ruler/api_test.go

@@ -3,6 +3,7 @@ package ruler
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	io "io"
 	"io/ioutil"
 	"net/http"
@@ -170,7 +171,44 @@ func TestRuler_Create(t *testing.T) {
 	defer rcleanup()
 	defer services.StopAndAwaitTerminated(context.Background(), r) //nolint:errcheck
 
-	rules := `name: test
+	tc := []struct {
+		name   string
+		input  string
+		output string
+		err    error
+		status int
+	}{
+		{
+			name:   "with an empty payload",
+			input:  "",
+			status: 400,
+			err:    errors.New("invalid rules config: rule group name must not be empty"),
+		},
+		{
+			name: "with no rule group name",
+			input: `
+interval: 15s
+rules:
+- record: up_rule
+  expr: up
+`,
+			status: 400,
+			err:    errors.New("invalid rules config: rule group name must not be empty"),
+		},
+		{
+			name: "with no rules",
+			input: `
+name: rg_name
+interval: 15s
+`,
+			status: 400,
+			err:    errors.New("invalid rules config: rule group 'rg_name' has no rules"),
+		},
+		{
+			name:   "with a a valid rules file",
+			status: 202,
+			input: `
+name: test
 interval: 15s
 rules:
 - record: up_rule
@@ -182,26 +220,36 @@ rules:
     test: test
   labels:
     test: test
-`
-
-	router := mux.NewRouter()
-	router.Path("/api/v1/rules/{namespace}").Methods("POST").HandlerFunc(r.CreateRuleGroup)
-	router.Path("/api/v1/rules/{namespace}/{groupName}").Methods("GET").HandlerFunc(r.GetRuleGroup)
-
-	// POST
-	req := requestFor(t, http.MethodPost, "https://localhost:8080/api/v1/rules/namespace", strings.NewReader(rules), "user1")
-	w := httptest.NewRecorder()
-
-	router.ServeHTTP(w, req)
-	require.Equal(t, 202, w.Code)
-
-	// GET
-	req = requestFor(t, http.MethodGet, "https://localhost:8080/api/v1/rules/namespace/test", nil, "user1")
-	w = httptest.NewRecorder()
-
-	router.ServeHTTP(w, req)
-	require.Equal(t, 200, w.Code)
-	require.Equal(t, "name: test\ninterval: 15s\nrules:\n    - record: up_rule\n      expr: up{}\n    - alert: up_alert\n      expr: sum(up{}) > 1\n      for: 30s\n      labels:\n        test: test\n      annotations:\n        test: test\n", w.Body.String())
+`,
+			output: "name: test\ninterval: 15s\nrules:\n    - record: up_rule\n      expr: up{}\n    - alert: up_alert\n      expr: sum(up{}) > 1\n      for: 30s\n      labels:\n        test: test\n      annotations:\n        test: test\n",
+		},
+	}
+
+	for _, tt := range tc {
+		t.Run(tt.name, func(t *testing.T) {
+			router := mux.NewRouter()
+			router.Path("/api/v1/rules/{namespace}").Methods("POST").HandlerFunc(r.CreateRuleGroup)
+			router.Path("/api/v1/rules/{namespace}/{groupName}").Methods("GET").HandlerFunc(r.GetRuleGroup)
+			// POST
+			req := requestFor(t, http.MethodPost, "https://localhost:8080/api/v1/rules/namespace", strings.NewReader(tt.input), "user1")
+			w := httptest.NewRecorder()
+
+			router.ServeHTTP(w, req)
+			require.Equal(t, tt.status, w.Code)
+
+			if tt.err == nil {
+				// GET
+				req = requestFor(t, http.MethodGet, "https://localhost:8080/api/v1/rules/namespace/test", nil, "user1")
+				w = httptest.NewRecorder()
+
+				router.ServeHTTP(w, req)
+				require.Equal(t, 200, w.Code)
+				require.Equal(t, tt.output, w.Body.String())
+			} else {
+				require.Equal(t, tt.err.Error()+"\n", w.Body.String())
+			}
+		})
+	}
 }
 
 func TestRuler_DeleteNamespace(t *testing.T) {

pkg/ruler/manager.go

@@ -2,12 +2,14 @@ package ruler
 
 import (
 	"context"
+	"fmt"
 	"net/http"
 	"sync"
 
 	"github.com/go-kit/kit/log"
 	"github.com/go-kit/kit/log/level"
 	ot "github.com/opentracing/opentracing-go"
+	"github.com/pkg/errors"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promauto"
 	"github.com/prometheus/prometheus/config"
@@ -253,6 +255,17 @@ func (r *DefaultMultiTenantManager) Stop() {
 
 func (*DefaultMultiTenantManager) ValidateRuleGroup(g rulefmt.RuleGroup) []error {
 	var errs []error
+
+	if g.Name == "" {
+		errs = append(errs, errors.New("invalid rules config: rule group name must not be empty"))
+		return errs
+	}
+
+	if len(g.Rules) == 0 {
+		errs = append(errs, fmt.Errorf("invalid rules config: rule group '%s' has no rules", g.Name))
+		return errs
+	}
+
 	for i, r := range g.Rules {
 		for _, err := range r.Validate() {
 			var ruleName string