fix(ruler): properly read OrgID from context (#3343)

* fix(ruler): properly read OrgID from context

Previously the `/prometheus/api/v1/{rules|alerts}` endpoints used
`user.ExtractOrgIDFromHTTPRequest` to read the tenant id directly from
the HTTP request headers.

This however only works in multi-tenant mode. When auth is disabled, no
such headers are needed to be set and above function returns no value.

By directly reading from the request, the HTTP auth middleware which
usually catches such cases is bypassed.

This PR changes the behavior to always read from the context instead of
the request, which always holds the correct org id as set by the middleware.

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

* fix(alertmanager): read org id from context

Ports fixes done to the ruler api also to alertmanager

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

* doc: add CHANGELOG entry

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

* Address review comments

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

Co-authored-by: gotjosh <[email protected]>

Author: tombrk

CHANGELOG.md

@@ -108,6 +108,7 @@
 * [BUGFIX] Honor configured timeout in Azure and GCS object clients. #3285
 * [BUGFIX] Shuffle sharding: fixed max global series per user/metric limit when shuffle sharding and `-distributor.shard-by-all-labels=true` are both enabled in distributor. When using these global limits you should now set `-distributor.sharding-strategy` and `-distributor.zone-awareness-enabled` to ingesters too. #3369
 * [BUGFIX] Slow query logging: when using downstream server request parameters were not logged. #3276
+* [BUGFIX] Fixed tenant detection in the ruler and alertmanager API when running without auth. #3343
 
 ## 1.4.0 / 2020-10-02
 

pkg/alertmanager/api.go

@@ -36,7 +36,7 @@ type UserConfig struct {
 func (am *MultitenantAlertmanager) GetUserConfig(w http.ResponseWriter, r *http.Request) {
 	logger := util.WithContext(r.Context(), am.logger)
 
-	userID, _, err := user.ExtractOrgIDFromHTTPRequest(r)
+	userID, err := user.ExtractOrgID(r.Context())
 	if err != nil {
 		level.Error(logger).Log("msg", errNoOrgID, "err", err.Error())
 		http.Error(w, fmt.Sprintf("%s: %s", errNoOrgID, err.Error()), http.StatusUnauthorized)
@@ -73,7 +73,7 @@ func (am *MultitenantAlertmanager) GetUserConfig(w http.ResponseWriter, r *http.
 
 func (am *MultitenantAlertmanager) SetUserConfig(w http.ResponseWriter, r *http.Request) {
 	logger := util.WithContext(r.Context(), am.logger)
-	userID, _, err := user.ExtractOrgIDFromHTTPRequest(r)
+	userID, err := user.ExtractOrgID(r.Context())
 	if err != nil {
 		level.Error(logger).Log("msg", errNoOrgID, "err", err.Error())
 		http.Error(w, fmt.Sprintf("%s: %s", errNoOrgID, err.Error()), http.StatusUnauthorized)
@@ -114,7 +114,7 @@ func (am *MultitenantAlertmanager) SetUserConfig(w http.ResponseWriter, r *http.
 
 func (am *MultitenantAlertmanager) DeleteUserConfig(w http.ResponseWriter, r *http.Request) {
 	logger := util.WithContext(r.Context(), am.logger)
-	userID, _, err := user.ExtractOrgIDFromHTTPRequest(r)
+	userID, err := user.ExtractOrgID(r.Context())
 	if err != nil {
 		level.Error(logger).Log("msg", errNoOrgID, "err", err.Error())
 		http.Error(w, fmt.Sprintf("%s: %s", errNoOrgID, err.Error()), http.StatusUnauthorized)

pkg/alertmanager/api_test.go

@@ -131,11 +131,9 @@ template_files:
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			req := httptest.NewRequest(http.MethodPost, "http://alertmanager/api/v1/alerts", bytes.NewReader([]byte(tc.cfg)))
-			ctx := context.Background()
-			ctx = user.InjectOrgID(ctx, "testing")
-			require.NoError(t, user.InjectOrgIDIntoHTTPRequest(ctx, req))
+			ctx := user.InjectOrgID(req.Context(), "testing")
 			w := httptest.NewRecorder()
-			am.SetUserConfig(w, req)
+			am.SetUserConfig(w, req.WithContext(ctx))
 			resp := w.Result()
 
 			body, err := ioutil.ReadAll(resp.Body)

pkg/alertmanager/multitenant.go

@@ -463,7 +463,7 @@ func (am *MultitenantAlertmanager) newAlertmanager(userID string, amConfig *amco
 
 // ServeHTTP serves the Alertmanager's web UI and API.
 func (am *MultitenantAlertmanager) ServeHTTP(w http.ResponseWriter, req *http.Request) {
-	userID, _, err := user.ExtractOrgIDFromHTTPRequest(req)
+	userID, err := user.ExtractOrgID(req.Context())
 	if err != nil {
 		http.Error(w, err.Error(), http.StatusUnauthorized)
 		return

pkg/alertmanager/multitenant_test.go

@@ -214,10 +214,10 @@ func TestAlertmanager_ServeHTTP(t *testing.T) {
 
 	// Request when no user configuration is present.
 	req := httptest.NewRequest("GET", externalURL.String(), nil)
-	req.Header.Add(user.OrgIDHeaderName, "user1")
+	ctx := user.InjectOrgID(req.Context(), "user1")
 	w := httptest.NewRecorder()
 
-	am.ServeHTTP(w, req)
+	am.ServeHTTP(w, req.WithContext(ctx))
 
 	resp := w.Result()
 	body, _ := ioutil.ReadAll(resp.Body)
@@ -237,7 +237,7 @@ func TestAlertmanager_ServeHTTP(t *testing.T) {
 
 	// Request when user configuration is paused.
 	w = httptest.NewRecorder()
-	am.ServeHTTP(w, req)
+	am.ServeHTTP(w, req.WithContext(ctx))
 
 	resp = w.Result()
 	body, _ = ioutil.ReadAll(resp.Body)
@@ -278,10 +278,10 @@ receivers:
 
 	// Request when no user configuration is present.
 	req := httptest.NewRequest("GET", externalURL.String()+"/api/v1/status", nil)
-	req.Header.Add(user.OrgIDHeaderName, "user1")
+	ctx := user.InjectOrgID(req.Context(), "user1")
 	w := httptest.NewRecorder()
 
-	am.ServeHTTP(w, req)
+	am.ServeHTTP(w, req.WithContext(ctx))
 
 	resp := w.Result()
 
@@ -302,7 +302,7 @@ receivers:
 
 	// Request when user configuration is paused.
 	w = httptest.NewRecorder()
-	am.ServeHTTP(w, req)
+	am.ServeHTTP(w, req.WithContext(ctx))
 
 	resp = w.Result()
 	body, _ := ioutil.ReadAll(resp.Body)

pkg/ruler/api.go

@@ -134,15 +134,15 @@ func NewAPI(r *Ruler, s rules.RuleStore) *API {
 
 func (a *API) PrometheusRules(w http.ResponseWriter, req *http.Request) {
 	logger := util.WithContext(req.Context(), util.Logger)
-	userID, ctx, err := user.ExtractOrgIDFromHTTPRequest(req)
+	userID, err := user.ExtractOrgID(req.Context())
 	if err != nil || userID == "" {
 		level.Error(logger).Log("msg", "error extracting org id from context", "err", err)
 		respondError(logger, w, "no valid org id found")
 		return
 	}
 
 	w.Header().Set("Content-Type", "application/json")
-	rgs, err := a.ruler.GetRules(ctx)
+	rgs, err := a.ruler.GetRules(req.Context())
 
 	if err != nil {
 		respondError(logger, w, err.Error())
@@ -226,15 +226,15 @@ func (a *API) PrometheusRules(w http.ResponseWriter, req *http.Request) {
 
 func (a *API) PrometheusAlerts(w http.ResponseWriter, req *http.Request) {
 	logger := util.WithContext(req.Context(), util.Logger)
-	userID, ctx, err := user.ExtractOrgIDFromHTTPRequest(req)
+	userID, err := user.ExtractOrgID(req.Context())
 	if err != nil || userID == "" {
 		level.Error(logger).Log("msg", "error extracting org id from context", "err", err)
 		respondError(logger, w, "no valid org id found")
 		return
 	}
 
 	w.Header().Set("Content-Type", "application/json")
-	rgs, err := a.ruler.GetRules(ctx)
+	rgs, err := a.ruler.GetRules(req.Context())
 
 	if err != nil {
 		respondError(logger, w, err.Error())

pkg/ruler/api_test.go

@@ -370,7 +370,6 @@ func requestFor(t *testing.T, method string, url string, body io.Reader, userID
 	t.Helper()
 
 	req := httptest.NewRequest(method, url, body)
-	req.Header.Add(user.OrgIDHeaderName, userID)
 	ctx := user.InjectOrgID(req.Context(), userID)
 
 	return req.WithContext(ctx)