rules: fix silly mistakes in the rules API (#2957)

* rules: fix silly mistakes in the rules API Fixes #2938 and is a follow up to #2925. Changes: * Copying all fields properly to the group-level as well * Set TZ to UTC explicitly in other `time.Time` fields to avoid a panic * Make sure that `RuleGroup` always has an empty array in the `rules` field because that's customary and that is what the new React UI expects. Manual testing for now. Signed-off-by: Giedrius Statkevičius <[email protected]> * rules: remove DeprecatedPartialResponseStrategy It has been marked deprecated for more than 2 months. Remove it finally. Signed-off-by: Giedrius Statkevičius <[email protected]> * e2e: rules_api: add checks for non-zero values Signed-off-by: Giedrius Statkevičius <[email protected]> * test: e2e: fix rule test Signed-off-by: Giedrius Statkevičius <[email protected]> * test: rule: fix govet error Signed-off-by: Giedrius Statkevičius <[email protected]> * Update CHANGELOG.md Signed-off-by: Giedrius Statkevičius <[email protected]> Co-authored-by: Lucas Servén Marín <[email protected]> * Update CHANGELOG.md Signed-off-by: Giedrius Statkevičius <[email protected]> Co-authored-by: Lucas Servén Marín <[email protected]> Co-authored-by: Lucas Servén Marín <[email protected]>
thanos-io · Aug 11, 2020 · 600318d · 600318d
1 parent 626f248
commit 600318d
Show file tree

Hide file tree

Showing 10 changed files with 277 additions and 266 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,8 @@ We use *breaking* word for marking changes that are not backward compatible (rel
 
 ## Unreleased
 
+:warning: **WARNING** :warning: Thanos Rule's `/api/v1/rules` endpoint no longer returns the old, deprecated `partial_response_strategy`. The old, deprecated value has been fixed to `WARN` for quite some time. _Please_ use `partialResponseStrategy`.
+
 ### Fixed
 
 - [#2937](https://github.com/thanos-io/thanos/pull/2937) Receive: Fixing auto-configuration of --receive.local-endpoint
@@ -23,6 +25,7 @@ We use *breaking* word for marking changes that are not backward compatible (rel
 - [#2936](https://github.com/thanos-io/thanos/pull/2936) Compact: Fix ReplicaLabelRemover panic when replicaLabels are not specified.
 - [#2956](https://github.com/thanos-io/thanos/pull/2956) Store: Fix fetching of chunks bigger than 16000 bytes.
 - [#2970](https://github.com/thanos-io/thanos/pull/2970) Store: Upgrade minio-go/v7 to fix slowness when running on EKS.
+- [#2957](https://github.com/thanos-io/thanos/pull/2957) Rule: now sets all of the relevant fields properly; avoids a panic when `/api/v1/rules` is called and the time zone is _not_ UTC; `rules` field is an empty array now if no rules have been defined in a rule group.
 - [#2976](https://github.com/thanos-io/thanos/pull/2976) Query: Better rounding for incoming query timestamps.
 
 ### Added

diff --git a/pkg/api/query/v1_test.go b/pkg/api/query/v1_test.go
@@ -1138,48 +1138,44 @@ func TestRulesHandler(t *testing.T) {
 		g: map[rulespb.RulesRequest_Type][]*rulespb.RuleGroup{
 			rulespb.RulesRequest_ALL: {
 				{
-					Name:                              "grp",
-					File:                              "/path/to/groupfile1",
-					Rules:                             all,
-					Interval:                          1,
-					EvaluationDurationSeconds:         214,
-					LastEvaluation:                    time.Time{}.Add(10 * time.Minute),
-					DeprecatedPartialResponseStrategy: 0,
-					PartialResponseStrategy:           storepb.PartialResponseStrategy_WARN,
+					Name:                      "grp",
+					File:                      "/path/to/groupfile1",
+					Rules:                     all,
+					Interval:                  1,
+					EvaluationDurationSeconds: 214,
+					LastEvaluation:            time.Time{}.Add(10 * time.Minute),
+					PartialResponseStrategy:   storepb.PartialResponseStrategy_WARN,
 				},
 				{
-					Name:                              "grp2",
-					File:                              "/path/to/groupfile2",
-					Rules:                             all[3:],
-					Interval:                          10,
-					EvaluationDurationSeconds:         2142,
-					LastEvaluation:                    time.Time{}.Add(100 * time.Minute),
-					DeprecatedPartialResponseStrategy: 0,
-					PartialResponseStrategy:           storepb.PartialResponseStrategy_ABORT,
+					Name:                      "grp2",
+					File:                      "/path/to/groupfile2",
+					Rules:                     all[3:],
+					Interval:                  10,
+					EvaluationDurationSeconds: 2142,
+					LastEvaluation:            time.Time{}.Add(100 * time.Minute),
+					PartialResponseStrategy:   storepb.PartialResponseStrategy_ABORT,
 				},
 			},
 			rulespb.RulesRequest_RECORD: {
 				{
-					Name:                              "grp",
-					File:                              "/path/to/groupfile1",
-					Rules:                             all[:2],
-					Interval:                          1,
-					EvaluationDurationSeconds:         214,
-					LastEvaluation:                    time.Time{}.Add(20 * time.Minute),
-					DeprecatedPartialResponseStrategy: 0,
-					PartialResponseStrategy:           storepb.PartialResponseStrategy_WARN,
+					Name:                      "grp",
+					File:                      "/path/to/groupfile1",
+					Rules:                     all[:2],
+					Interval:                  1,
+					EvaluationDurationSeconds: 214,
+					LastEvaluation:            time.Time{}.Add(20 * time.Minute),
+					PartialResponseStrategy:   storepb.PartialResponseStrategy_WARN,
 				},
 			},
 			rulespb.RulesRequest_ALERT: {
 				{
-					Name:                              "grp",
-					File:                              "/path/to/groupfile1",
-					Rules:                             all[2:],
-					Interval:                          1,
-					EvaluationDurationSeconds:         214,
-					LastEvaluation:                    time.Time{}.Add(30 * time.Minute),
-					DeprecatedPartialResponseStrategy: 0,
-					PartialResponseStrategy:           storepb.PartialResponseStrategy_WARN,
+					Name:                      "grp",
+					File:                      "/path/to/groupfile1",
+					Rules:                     all[2:],
+					Interval:                  1,
+					EvaluationDurationSeconds: 214,
+					LastEvaluation:            time.Time{}.Add(30 * time.Minute),
+					PartialResponseStrategy:   storepb.PartialResponseStrategy_WARN,
 				},
 			},
 		},
@@ -1262,24 +1258,22 @@ func TestRulesHandler(t *testing.T) {
 			response: &testpromcompatibility.RuleDiscovery{
 				RuleGroups: []*testpromcompatibility.RuleGroup{
 					{
-						Name:                              "grp",
-						File:                              "/path/to/groupfile1",
-						Rules:                             expectedAll,
-						Interval:                          1,
-						EvaluationTime:                    214,
-						LastEvaluation:                    time.Time{}.Add(10 * time.Minute),
-						PartialResponseStrategy:           "WARN",
-						DeprecatedPartialResponseStrategy: "WARN",
+						Name:                    "grp",
+						File:                    "/path/to/groupfile1",
+						Rules:                   expectedAll,
+						Interval:                1,
+						EvaluationTime:          214,
+						LastEvaluation:          time.Time{}.Add(10 * time.Minute),
+						PartialResponseStrategy: "WARN",
 					},
 					{
-						Name:                              "grp2",
-						File:                              "/path/to/groupfile2",
-						Rules:                             expectedAll[3:],
-						Interval:                          10,
-						EvaluationTime:                    2142,
-						LastEvaluation:                    time.Time{}.Add(100 * time.Minute),
-						PartialResponseStrategy:           "ABORT",
-						DeprecatedPartialResponseStrategy: "WARN",
+						Name:                    "grp2",
+						File:                    "/path/to/groupfile2",
+						Rules:                   expectedAll[3:],
+						Interval:                10,
+						EvaluationTime:          2142,
+						LastEvaluation:          time.Time{}.Add(100 * time.Minute),
+						PartialResponseStrategy: "ABORT",
 					},
 				},
 			},
@@ -1289,14 +1283,13 @@ func TestRulesHandler(t *testing.T) {
 			response: &testpromcompatibility.RuleDiscovery{
 				RuleGroups: []*testpromcompatibility.RuleGroup{
 					{
-						Name:                              "grp",
-						File:                              "/path/to/groupfile1",
-						Rules:                             expectedAll[:2],
-						Interval:                          1,
-						EvaluationTime:                    214,
-						LastEvaluation:                    time.Time{}.Add(20 * time.Minute),
-						PartialResponseStrategy:           "WARN",
-						DeprecatedPartialResponseStrategy: "WARN",
+						Name:                    "grp",
+						File:                    "/path/to/groupfile1",
+						Rules:                   expectedAll[:2],
+						Interval:                1,
+						EvaluationTime:          214,
+						LastEvaluation:          time.Time{}.Add(20 * time.Minute),
+						PartialResponseStrategy: "WARN",
 					},
 				},
 			},
@@ -1306,14 +1299,13 @@ func TestRulesHandler(t *testing.T) {
 			response: &testpromcompatibility.RuleDiscovery{
 				RuleGroups: []*testpromcompatibility.RuleGroup{
 					{
-						Name:                              "grp",
-						File:                              "/path/to/groupfile1",
-						Rules:                             expectedAll[2:],
-						Interval:                          1,
-						EvaluationTime:                    214,
-						LastEvaluation:                    time.Time{}.Add(30 * time.Minute),
-						PartialResponseStrategy:           "WARN",
-						DeprecatedPartialResponseStrategy: "WARN",
+						Name:                    "grp",
+						File:                    "/path/to/groupfile1",
+						Rules:                   expectedAll[2:],
+						Interval:                1,
+						EvaluationTime:          214,
+						LastEvaluation:          time.Time{}.Add(30 * time.Minute),
+						PartialResponseStrategy: "WARN",
 					},
 				},
 			},

diff --git a/pkg/rules/manager.go b/pkg/rules/manager.go
@@ -44,6 +44,9 @@ func (g Group) toProto() *rulespb.RuleGroup {
 		File:                    g.OriginalFile,
 		Interval:                g.Interval().Seconds(),
 		PartialResponseStrategy: g.PartialResponseStrategy,
+		// https://github.com/gogo/protobuf/issues/519
+		LastEvaluation:            g.GetEvaluationTimestamp().UTC(),
+		EvaluationDurationSeconds: g.GetEvaluationDuration().Seconds(),
 	}
 
 	for _, r := range g.Rules() {
@@ -66,7 +69,8 @@ func (g Group) toProto() *rulespb.RuleGroup {
 					Health:                    string(rule.Health()),
 					LastError:                 lastError,
 					EvaluationDurationSeconds: rule.GetEvaluationDuration().Seconds(),
-					LastEvaluation:            rule.GetEvaluationTimestamp(),
+					// https://github.com/gogo/protobuf/issues/519
+					LastEvaluation: rule.GetEvaluationTimestamp().UTC(),
 				}}})
 		case *rules.RecordingRule:
 			ret.Rules = append(ret.Rules, &rulespb.Rule{
@@ -77,7 +81,8 @@ func (g Group) toProto() *rulespb.RuleGroup {
 					Health:                    string(rule.Health()),
 					LastError:                 lastError,
 					EvaluationDurationSeconds: rule.GetEvaluationDuration().Seconds(),
-					LastEvaluation:            rule.GetEvaluationTimestamp(),
+					// https://github.com/gogo/protobuf/issues/519
+					LastEvaluation: rule.GetEvaluationTimestamp().UTC(),
 				}}})
 		default:
 			// We cannot do much, let's panic, API will recover.

diff --git a/pkg/rules/rules_test.go b/pkg/rules/rules_test.go
@@ -28,25 +28,25 @@ func testRulesAgainstExamples(t *testing.T, dir string, server rulespb.RulesServ
 
 	expected := []*rulespb.RuleGroup{
 		{
-			Name:                              "thanos-bucket-replicate.rules",
-			File:                              filepath.Join(dir, "alerts.yaml"),
-			Rules:                             []*rulespb.Rule{someAlert, someAlert, someAlert},
-			Interval:                          60,
-			DeprecatedPartialResponseStrategy: storepb.PartialResponseStrategy_WARN, PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
+			Name:                    "thanos-bucket-replicate.rules",
+			File:                    filepath.Join(dir, "alerts.yaml"),
+			Rules:                   []*rulespb.Rule{someAlert, someAlert, someAlert},
+			Interval:                60,
+			PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
 		},
 		{
-			Name:                              "thanos-compact.rules",
-			File:                              filepath.Join(dir, "alerts.yaml"),
-			Rules:                             []*rulespb.Rule{someAlert, someAlert, someAlert, someAlert, someAlert},
-			Interval:                          60,
-			DeprecatedPartialResponseStrategy: storepb.PartialResponseStrategy_WARN, PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
+			Name:                    "thanos-compact.rules",
+			File:                    filepath.Join(dir, "alerts.yaml"),
+			Rules:                   []*rulespb.Rule{someAlert, someAlert, someAlert, someAlert, someAlert},
+			Interval:                60,
+			PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
 		},
 		{
-			Name:                              "thanos-component-absent.rules",
-			File:                              filepath.Join(dir, "alerts.yaml"),
-			Rules:                             []*rulespb.Rule{someAlert, someAlert, someAlert, someAlert, someAlert, someAlert},
-			Interval:                          60,
-			DeprecatedPartialResponseStrategy: storepb.PartialResponseStrategy_WARN, PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
+			Name:                    "thanos-component-absent.rules",
+			File:                    filepath.Join(dir, "alerts.yaml"),
+			Rules:                   []*rulespb.Rule{someAlert, someAlert, someAlert, someAlert, someAlert, someAlert},
+			Interval:                60,
+			PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
 		},
 		{
 			Name: "thanos-query.rules",
@@ -55,8 +55,8 @@ func testRulesAgainstExamples(t *testing.T, dir string, server rulespb.RulesServ
 				someAlert, someAlert, someAlert, someAlert, someAlert, someAlert, someAlert,
 				someRecording, someRecording, someRecording, someRecording, someRecording,
 			},
-			Interval:                          60,
-			DeprecatedPartialResponseStrategy: storepb.PartialResponseStrategy_WARN, PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
+			Interval:                60,
+			PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
 		},
 		{
 			Name: "thanos-receive.rules",
@@ -65,22 +65,22 @@ func testRulesAgainstExamples(t *testing.T, dir string, server rulespb.RulesServ
 				someAlert, someAlert, someAlert, someAlert, someAlert, someAlert, someAlert,
 				someRecording, someRecording, someRecording, someRecording, someRecording, someRecording, someRecording,
 			},
-			Interval:                          60,
-			DeprecatedPartialResponseStrategy: storepb.PartialResponseStrategy_WARN, PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
+			Interval:                60,
+			PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
 		},
 		{
-			Name:                              "thanos-rule.rules",
-			File:                              filepath.Join(dir, "alerts.yaml"),
-			Rules:                             []*rulespb.Rule{someAlert, someAlert, someAlert, someAlert, someAlert, someAlert, someAlert, someAlert, someAlert, someAlert, someAlert},
-			Interval:                          60,
-			DeprecatedPartialResponseStrategy: storepb.PartialResponseStrategy_WARN, PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
+			Name:                    "thanos-rule.rules",
+			File:                    filepath.Join(dir, "alerts.yaml"),
+			Rules:                   []*rulespb.Rule{someAlert, someAlert, someAlert, someAlert, someAlert, someAlert, someAlert, someAlert, someAlert, someAlert, someAlert},
+			Interval:                60,
+			PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
 		},
 		{
-			Name:                              "thanos-sidecar.rules",
-			File:                              filepath.Join(dir, "alerts.yaml"),
-			Rules:                             []*rulespb.Rule{someAlert, someAlert},
-			Interval:                          60,
-			DeprecatedPartialResponseStrategy: storepb.PartialResponseStrategy_WARN, PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
+			Name:                    "thanos-sidecar.rules",
+			File:                    filepath.Join(dir, "alerts.yaml"),
+			Rules:                   []*rulespb.Rule{someAlert, someAlert},
+			Interval:                60,
+			PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
 		},
 		{
 			Name: "thanos-store.rules",
@@ -89,8 +89,8 @@ func testRulesAgainstExamples(t *testing.T, dir string, server rulespb.RulesServ
 				someAlert, someAlert, someAlert, someAlert,
 				someRecording, someRecording, someRecording, someRecording,
 			},
-			Interval:                          60,
-			DeprecatedPartialResponseStrategy: storepb.PartialResponseStrategy_WARN, PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
+			Interval:                60,
+			PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
 		},
 	}
 

diff --git a/pkg/rules/rulespb/custom.go b/pkg/rules/rulespb/custom.go
@@ -241,6 +241,15 @@ func (m *Rule) MarshalJSON() ([]byte, error) {
 	})
 }
 
+func (r *RuleGroup) MarshalJSON() ([]byte, error) {
+	if r.Rules == nil {
+		// Ensure that empty slices are marshaled as '[]' and not 'null'.
+		r.Rules = make([]*Rule, 0)
+	}
+	type plain RuleGroup
+	return json.Marshal((*plain)(r))
+}
+
 func (x *AlertState) UnmarshalJSON(entry []byte) error {
 	fieldStr, err := strconv.Unquote(string(entry))
 	if err != nil {