Skip to content

Commit b832023

Browse files
periklisgrafana-delivery-bot[bot]
periklis
authored and
grafana-delivery-bot[bot]
committed
fix(ruler): Return StatusBadRequest on multiple org IDs (#17850)
(cherry picked from commit f7e47f0)
1 parent 7cc9e9e commit b832023

File tree

3 files changed

+101
-29
lines changed

3 files changed

+101
-29
lines changed

‎pkg/ruler/base/api.go‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,11 @@ func (a *API) PrometheusRules(w http.ResponseWriter, req *http.Request) {
151151
logger := util_log.WithContext(req.Context(), a.logger)
152152
userID, err := tenant.TenantID(req.Context())
153153
if err != nil || userID == "" {
154+
if errors.Is(err, user.ErrTooManyOrgIDs) {
155+
respondInvalidRequest(logger, w, "too many org ids found")
156+
return
157+
}
158+
154159
level.Error(logger).Log("msg", "error extracting org id from context", "err", err)
155160
respondServerError(logger, w, "no valid org id found")
156161
return
@@ -177,8 +182,12 @@ func (a *API) PrometheusRules(w http.ResponseWriter, req *http.Request) {
177182
}
178183

179184
rgs, err := a.ruler.GetRules(req.Context(), &rulesReq)
180-
181185
if err != nil {
186+
if errors.Is(err, user.ErrTooManyOrgIDs) {
187+
respondInvalidRequest(logger, w, "too many org ids found")
188+
return
189+
}
190+
182191
respondServerError(logger, w, err.Error())
183192
return
184193
}
@@ -263,6 +272,11 @@ func (a *API) PrometheusAlerts(w http.ResponseWriter, req *http.Request) {
263272
logger := util_log.WithContext(req.Context(), a.logger)
264273
userID, err := tenant.TenantID(req.Context())
265274
if err != nil || userID == "" {
275+
if errors.Is(err, user.ErrTooManyOrgIDs) {
276+
respondInvalidRequest(logger, w, "too many org ids found")
277+
return
278+
}
279+
266280
level.Error(logger).Log("msg", "error extracting org id from context", "err", err)
267281
respondServerError(logger, w, "no valid org id found")
268282
return
@@ -272,6 +286,11 @@ func (a *API) PrometheusAlerts(w http.ResponseWriter, req *http.Request) {
272286
rgs, err := a.ruler.GetRules(req.Context(), &RulesRequest{Filter: AlertingRule})
273287

274288
if err != nil {
289+
if errors.Is(err, user.ErrTooManyOrgIDs) {
290+
respondInvalidRequest(logger, w, "too many org ids found")
291+
return
292+
}
293+
275294
respondServerError(logger, w, err.Error())
276295
return
277296
}

‎pkg/ruler/base/api_test.go‎

Lines changed: 80 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ func TestRuler_PrometheusRules(t *testing.T) {
8181
}
8282

8383
testCases := map[string]struct {
84+
tenantID string
8485
configuredRules rulespb.RuleGroupList
8586
expectedConfigured int
8687
expectedStatusCode int
@@ -89,6 +90,7 @@ func TestRuler_PrometheusRules(t *testing.T) {
8990
queryParams string
9091
}{
9192
"should load and evaluate the configured rules": {
93+
tenantID: userID,
9294
configuredRules: rulespb.RuleGroupList{
9395
&rulespb.RuleGroupDesc{
9496
Name: "group1",
@@ -133,6 +135,7 @@ func TestRuler_PrometheusRules(t *testing.T) {
133135
Interval: interval,
134136
},
135137
},
138+
tenantID: userID,
136139
expectedConfigured: 1,
137140
expectedRules: []*RuleGroup{
138141
{
@@ -168,6 +171,7 @@ func TestRuler_PrometheusRules(t *testing.T) {
168171
Interval: interval,
169172
},
170173
},
174+
tenantID: userID,
171175
expectedConfigured: 1,
172176
queryParams: "?type=alert",
173177
expectedRules: []*RuleGroup{
@@ -198,6 +202,7 @@ func TestRuler_PrometheusRules(t *testing.T) {
198202
Interval: interval,
199203
},
200204
},
205+
tenantID: userID,
201206
expectedConfigured: 1,
202207
queryParams: "?type=record",
203208
expectedRules: []*RuleGroup{
@@ -217,20 +222,32 @@ func TestRuler_PrometheusRules(t *testing.T) {
217222
},
218223
},
219224
"Invalid type param": {
225+
tenantID: userID,
220226
configuredRules: rulespb.RuleGroupList{},
221227
expectedConfigured: 0,
222228
queryParams: "?type=foo",
223229
expectedStatusCode: http.StatusBadRequest,
224230
expectedErrorType: v1.ErrBadData,
225231
expectedRules: []*RuleGroup{},
226232
},
233+
"Too many org ids": {
234+
tenantID: "user1|user2|user3",
235+
configuredRules: rulespb.RuleGroupList{},
236+
expectedConfigured: 0,
237+
queryParams: "?type=record",
238+
expectedStatusCode: http.StatusBadRequest,
239+
expectedErrorType: v1.ErrBadData,
240+
expectedRules: []*RuleGroup{},
241+
},
227242
"when filtering by an unknown namespace then the API returns nothing": {
243+
tenantID: userID,
228244
configuredRules: makeFilterTestRules(),
229245
expectedConfigured: len(makeFilterTestRules()),
230246
queryParams: "?file=unknown",
231247
expectedRules: []*RuleGroup{},
232248
},
233249
"when filtering by a single known namespace then the API returns only rules from that namespace": {
250+
tenantID: userID,
234251
configuredRules: makeFilterTestRules(),
235252
expectedConfigured: len(makeFilterTestRules()),
236253
queryParams: "?" + url.Values{"file": []string{namespaceName(1)}}.Encode(),
@@ -265,6 +282,7 @@ func TestRuler_PrometheusRules(t *testing.T) {
265282
},
266283
},
267284
"when filtering by a multiple known namespaces then the API returns rules from both namespaces": {
285+
tenantID: userID,
268286
configuredRules: makeFilterTestRules(),
269287
expectedConfigured: len(makeFilterTestRules()),
270288
queryParams: "?" + url.Values{"file": []string{namespaceName(1), namespaceName(2)}}.Encode(),
@@ -326,12 +344,14 @@ func TestRuler_PrometheusRules(t *testing.T) {
326344
},
327345
},
328346
"when filtering by an unknown group then the API returns nothing": {
347+
tenantID: userID,
329348
configuredRules: makeFilterTestRules(),
330349
expectedConfigured: len(makeFilterTestRules()),
331350
queryParams: "?rule_group=unknown",
332351
expectedRules: []*RuleGroup{},
333352
},
334353
"when filtering by a known group then the API returns only rules from that group": {
354+
tenantID: userID,
335355
configuredRules: makeFilterTestRules(),
336356
expectedConfigured: len(makeFilterTestRules()),
337357
queryParams: "?" + url.Values{"rule_group": []string{groupName(2)}}.Encode(),
@@ -366,6 +386,7 @@ func TestRuler_PrometheusRules(t *testing.T) {
366386
},
367387
},
368388
"when filtering by multiple known groups then the API returns rules from both groups": {
389+
tenantID: userID,
369390
configuredRules: makeFilterTestRules(),
370391
expectedConfigured: len(makeFilterTestRules()),
371392
queryParams: "?" + url.Values{"rule_group": []string{groupName(2), groupName(3)}}.Encode(),
@@ -428,12 +449,14 @@ func TestRuler_PrometheusRules(t *testing.T) {
428449
},
429450

430451
"when filtering by an unknown rule name then the API returns all empty groups": {
452+
tenantID: userID,
431453
configuredRules: makeFilterTestRules(),
432454
expectedConfigured: len(makeFilterTestRules()),
433455
queryParams: "?rule_name=unknown",
434456
expectedRules: []*RuleGroup{},
435457
},
436458
"when filtering by a known rule name then the API returns only rules with that name": {
459+
tenantID: userID,
437460
configuredRules: makeFilterTestRules(),
438461
expectedConfigured: len(makeFilterTestRules()),
439462
queryParams: "?" + url.Values{"rule_name": []string{"UniqueNamedRuleN1G2"}}.Encode(),
@@ -449,6 +472,7 @@ func TestRuler_PrometheusRules(t *testing.T) {
449472
},
450473
},
451474
"when filtering by multiple known rule names then the API returns both rules": {
475+
tenantID: userID,
452476
configuredRules: makeFilterTestRules(),
453477
expectedConfigured: len(makeFilterTestRules()),
454478
queryParams: "?" + url.Values{"rule_name": []string{"UniqueNamedRuleN1G2", "UniqueNamedRuleN2G3"}}.Encode(),
@@ -472,6 +496,7 @@ func TestRuler_PrometheusRules(t *testing.T) {
472496
},
473497
},
474498
"when filtering by a known namespace and group then the API returns only rules from that namespace and group": {
499+
tenantID: userID,
475500
configuredRules: makeFilterTestRules(),
476501
expectedConfigured: len(makeFilterTestRules()),
477502
queryParams: "?" + url.Values{
@@ -516,7 +541,7 @@ func TestRuler_PrometheusRules(t *testing.T) {
516541

517542
a := NewAPI(r, r.store, log.NewNopLogger())
518543

519-
req := requestFor(t, "GET", "https://localhost:8080/api/prom/api/v1/rules"+tc.queryParams, nil, "user1")
544+
req := requestFor(t, "GET", "https://localhost:8080/api/prom/api/v1/rules"+tc.queryParams, nil, tc.tenantID)
520545
w := httptest.NewRecorder()
521546
a.PrometheusRules(w, req)
522547

@@ -558,35 +583,63 @@ func TestRuler_PrometheusRules(t *testing.T) {
558583
func TestRuler_PrometheusAlerts(t *testing.T) {
559584
cfg := defaultRulerConfig(t, newMockRuleStore(mockRules))
560585

561-
r := newTestRuler(t, cfg)
562-
defer services.StopAndAwaitTerminated(context.Background(), r) //nolint:errcheck
586+
tests := []struct {
587+
name string
588+
tenantID string
589+
expectedStatusCode int
590+
expectedResponse response
591+
}{
592+
{
593+
name: "single org id",
594+
tenantID: "user1",
595+
expectedStatusCode: http.StatusOK,
596+
expectedResponse: response{
597+
Status: "success",
598+
Data: &AlertDiscovery{
599+
Alerts: []*Alert{},
600+
},
601+
},
602+
},
603+
{
604+
name: "multiple org ids",
605+
tenantID: "user1|user2|user3",
606+
expectedStatusCode: http.StatusBadRequest,
607+
expectedResponse: response{
608+
Status: "error",
609+
ErrorType: v1.ErrBadData,
610+
Error: "too many org ids found",
611+
},
612+
},
613+
}
563614

564-
a := NewAPI(r, r.store, log.NewNopLogger())
615+
for _, test := range tests {
616+
t.Run(test.name, func(t *testing.T) {
617+
r := newTestRuler(t, cfg)
618+
defer services.StopAndAwaitTerminated(context.Background(), r) //nolint:errcheck
565619

566-
req := requestFor(t, http.MethodGet, "https://localhost:8080/api/prom/api/v1/alerts", nil, "user1")
567-
w := httptest.NewRecorder()
568-
a.PrometheusAlerts(w, req)
569-
570-
resp := w.Result()
571-
body, _ := io.ReadAll(resp.Body)
572-
573-
// Check status code and status response
574-
responseJSON := response{}
575-
err := json.Unmarshal(body, &responseJSON)
576-
require.NoError(t, err)
577-
require.Equal(t, http.StatusOK, resp.StatusCode)
578-
require.Equal(t, responseJSON.Status, "success")
579-
580-
// Currently there is not an easy way to mock firing alerts. The empty
581-
// response case is tested instead.
582-
expectedResponse, _ := json.Marshal(response{
583-
Status: "success",
584-
Data: &AlertDiscovery{
585-
Alerts: []*Alert{},
586-
},
587-
})
620+
a := NewAPI(r, r.store, log.NewNopLogger())
621+
622+
req := requestFor(t, http.MethodGet, "https://localhost:8080/api/prom/api/v1/alerts", nil, test.tenantID)
623+
w := httptest.NewRecorder()
624+
a.PrometheusAlerts(w, req)
588625

589-
require.Equal(t, string(expectedResponse), string(body))
626+
resp := w.Result()
627+
body, _ := io.ReadAll(resp.Body)
628+
629+
// Check status code and status response
630+
responseJSON := response{}
631+
err := json.Unmarshal(body, &responseJSON)
632+
require.NoError(t, err)
633+
require.Equal(t, test.expectedStatusCode, resp.StatusCode)
634+
require.Equal(t, test.expectedResponse.Status, responseJSON.Status)
635+
636+
// Currently there is not an easy way to mock firing alerts. The empty
637+
// response case is tested instead.
638+
expectedResponse, err := json.Marshal(test.expectedResponse)
639+
require.NoError(t, err)
640+
require.Equal(t, string(expectedResponse), string(body))
641+
})
642+
}
590643
}
591644

592645
func TestRuler_GetRulesLabelFilter(t *testing.T) {

‎pkg/ruler/base/ruler.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -802,7 +802,7 @@ func RemoveRuleTokenFromGroupName(name string) string {
802802
func (r *Ruler) GetRules(ctx context.Context, req *RulesRequest) ([]*GroupStateDesc, error) {
803803
userID, err := tenant.TenantID(ctx)
804804
if err != nil {
805-
return nil, fmt.Errorf("no user id found in context")
805+
return nil, fmt.Errorf("no user id found in context: %w", err)
806806
}
807807

808808
if r.cfg.EnableSharding {

0 commit comments

Comments
 (0)