Review feedback:

- Simplify AM starting logic
- Don't upload any config to object store

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

Author: gotjosh

CHANGELOG.md

@@ -28,11 +28,8 @@
 * [ENHANCEMENT] Add "integration" as a label for `cortex_alertmanager_notifications_total` and `cortex_alertmanager_notifications_failed_total` metrics. #3056
 * [ENHANCEMENT] Add `cortex_ruler_config_last_reload_successful` and `cortex_ruler_config_last_reload_successful_seconds` to check status of users rule manager. #3056
 * [ENHANCEMENT] Memcached dial() calls now have an optional circuit-breaker to avoid hammering a broken cache #3051
-<<<<<<< HEAD
 * [ENHANCEMENT] Add TLS support to etcd client. #3102
-=======
-* [ENHANCEMENT] When a tenant accesses the Alertmanager UI or its API, if we have valid `-alertmanager.configs.fallback` we'll use that to start the manager and avoid failing the request. #XXXX
->>>>>>> baf7a0e40... Start an Alertmanager when the first request is received
+* [ENHANCEMENT] When a tenant accesses the Alertmanager UI or its API, if we have valid `-alertmanager.configs.fallback` we'll use that to start the manager and avoid failing the request. #3073
 * [BUGFIX] Query-frontend: Fixed rounding for incoming query timestamps, to be 100% Prometheus compatible. #2990
 * [BUGFIX] Querier: Merge results from chunks and blocks ingesters when using streaming of results. #3013
 * [BUGFIX] Querier: query /series from ingesters regardless the `-querier.query-ingesters-within` setting. #3035

pkg/alertmanager/multitenant.go

@@ -463,31 +463,36 @@ func (am *MultitenantAlertmanager) ServeHTTP(w http.ResponseWriter, req *http.Re
 	userAM, ok := am.alertmanagers[userID]
 	am.alertmanagersMtx.Unlock()
 
-	if !ok && am.fallbackConfig != "" {
+	if ok {
+		if !userAM.IsActive() {
+			http.Error(w, "the Alertmanager is not configured", http.StatusNotFound)
+			return
+		}
+
+		userAM.mux.ServeHTTP(w, req)
+		return
+	}
+
+	if am.fallbackConfig != "" {
 		userAM, err = am.alertmanagerFromFallbackConfig(userID)
 		if err != nil {
 			http.Error(w, "Failed to initialize the Alertmanager", http.StatusInternalServerError)
+			return
 		}
-	}
 
-	if !userAM.IsActive() {
-		http.Error(w, "the Alertmanager is not configured", http.StatusNotFound)
+		userAM.mux.ServeHTTP(w, req)
 		return
 	}
 
-	userAM.mux.ServeHTTP(w, req)
+	http.Error(w, "the Alertmanager is not configured", http.StatusNotFound)
 }
 
 func (am *MultitenantAlertmanager) alertmanagerFromFallbackConfig(userID string) (*Alertmanager, error) {
-	// There's a potential race condition here. If a tenant:
-	// a) uploads a config and b) receives an alert or access the UI
-	// before we're able the start the new manager we'll replace that config with an empty
-	// config.
+	// Upload an empty config so that the Alertmanager is no de-activated in the next poll
 	cfg := &UserConfig{AlertmanagerConfig: ""}
 	cfgDesc := alerts.ToProto(cfg.AlertmanagerConfig, cfg.TemplateFiles, userID)
 	err := am.store.SetAlertConfig(context.Background(), cfgDesc)
 	if err != nil {
-		level.Error(am.logger).Log("msg", "unable to store fallback configuration", "user", userID, "err", err.Error())
 		return nil, err
 	}
 
@@ -497,7 +502,6 @@ func (am *MultitenantAlertmanager) alertmanagerFromFallbackConfig(userID string)
 		return nil, err
 	}
 
-	// We need to check again in case they've had a new alertmanager started by the time we're ready
 	am.alertmanagersMtx.Lock()
 	defer am.alertmanagersMtx.Unlock()
 	return am.alertmanagers[userID], nil

pkg/alertmanager/multitenant_test.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"fmt"
 	"io/ioutil"
+	"net/http"
 	"net/http/httptest"
 	"os"
 	"testing"
@@ -48,7 +49,8 @@ func (m *mockAlertStore) GetAlertConfig(ctx context.Context, user string) (alert
 }
 
 func (m *mockAlertStore) SetAlertConfig(ctx context.Context, cfg alerts.AlertConfigDesc) error {
-	return fmt.Errorf("not implemented")
+	m.configs[cfg.User] = cfg
+	return nil
 }
 
 func (m *mockAlertStore) DeleteAlertConfig(ctx context.Context, user string) error {
@@ -241,3 +243,69 @@ func TestAlertmanager_ServeHTTP(t *testing.T) {
 	body, _ = ioutil.ReadAll(resp.Body)
 	require.Equal(t, "the Alertmanager is not configured\n", string(body))
 }
+
+func TestAlertmanager_ServeHTTPWithFallbackConfig(t *testing.T) {
+	mockStore := &mockAlertStore{
+		configs: map[string]alerts.AlertConfigDesc{},
+	}
+
+	externalURL := flagext.URLValue{}
+	err := externalURL.Set("http://localhost:8080/alertmanager")
+	require.NoError(t, err)
+
+	tempDir, err := ioutil.TempDir(os.TempDir(), "alertmanager")
+	require.NoError(t, err)
+	defer os.RemoveAll(tempDir)
+
+	fallbackCfg := `
+global:
+  smtp_smarthost: 'localhost:25'
+  smtp_from: '[email protected]'
+route:
+  receiver: example-email
+receivers:
+  - name: example-email
+    email_configs:
+    - to: '[email protected]'
+`
+
+	// Create the Multitenant Alertmanager.
+	reg := prometheus.NewPedanticRegistry()
+	am := createMultitenantAlertmanager(&MultitenantAlertmanagerConfig{
+		ExternalURL: externalURL,
+		DataDir:     tempDir,
+	}, nil, nil, mockStore, log.NewNopLogger(), reg)
+	am.fallbackConfig = fallbackCfg
+
+	// Request when no user configuration is present.
+	req := httptest.NewRequest("GET", externalURL.String()+"/api/v1/status", nil)
+	req.Header.Add(user.OrgIDHeaderName, "user1")
+	w := httptest.NewRecorder()
+
+	am.ServeHTTP(w, req)
+
+	resp := w.Result()
+
+	// It succeeds and the Alertmanager is started
+	require.Equal(t, http.StatusOK, resp.StatusCode)
+	require.Len(t, am.alertmanagers, 1)
+	require.True(t, am.alertmanagers["user1"].IsActive())
+
+	// Even after a poll it does not pause your Alertmanager
+	err = am.updateConfigs()
+	require.NoError(t, err)
+
+	require.True(t, am.alertmanagers["user1"].IsActive())
+	require.Len(t, am.alertmanagers, 1)
+
+	// Pause the alertmanager
+	am.alertmanagers["user1"].Pause()
+
+	// Request when user configuration is paused.
+	w = httptest.NewRecorder()
+	am.ServeHTTP(w, req)
+
+	resp = w.Result()
+	body, _ := ioutil.ReadAll(resp.Body)
+	require.Equal(t, "the Alertmanager is not configured\n", string(body))
+}