code review for wechat

config/notifiers.go

@@ -101,14 +101,8 @@ var (
 		NotifierConfig: NotifierConfig{
 			VSendResolved: true,
 		},
-		Message:   `{{ template "wechat.default.message" . }}`,
-		APIURL:    `{{ template "wechat.default.api_url" . }}`,
-		APISecret: `{{ template "wechat.default.api_secret" . }}`,
-		ToUser:    `{{ template "wechat.default.to_user" . }}`,
-		ToParty:   `{{ template "wechat.default.to_party" . }}`,
-		ToTag:     `{{ template "wechat.default.to_tag" . }}`,
-		AgentID:   `{{ template "wechat.default.agent_id" . }}`,
-		// TODO: Add a details field with all the alerts.
+		Message: `{{ template "wechat.default.message" . }}`,
+		APIURL:  DefaultGlobalConfig.WeChatAPIURL,
 	}
 
 	// DefaultVictorOpsConfig defines default values for VictorOps configurations.

config/notifiers_test.go

@@ -129,6 +129,7 @@ api_secret: ''
 }
 func TestWechatCorpIDIsPresent(t *testing.T) {
 	in := `
+api_secret: 'api_secret'
 corp_id: ''
 `
 	var cfg WechatConfig

notify/impl.go

@@ -823,7 +823,7 @@ type weChatMessageContent struct {
 	Content string `json:"content"`
 }
 
-type weChatErrorResponse struct {
+type weChatResponse struct {
 	Code  int    `json:"code"`
 	Error string `json:"error"`
 }
@@ -849,11 +849,8 @@ func (n *Wechat) Notify(ctx context.Context, as ...*types.Alert) (bool, error) {
 		return false, err
 	}
 
-	var accessToken string
 	// Refresh AccessToken over 2 hours
-	if n.accessToken != "" && time.Now().Sub(n.accessTokenAt) < 110*time.Minute {
-		accessToken = n.accessToken
-	} else {
+	if n.accessToken == "" || time.Now().Sub(n.accessTokenAt) > 2*time.Hour {
 		parameters := url.Values{}
 		parameters.Add("corpsecret", tmpl(string(n.conf.APISecret)))
 		parameters.Add("corpid", tmpl(string(n.conf.CorpID)))
@@ -867,9 +864,16 @@ func (n *Wechat) Notify(ctx context.Context, as ...*types.Alert) (bool, error) {
 
 		u.RawQuery = parameters.Encode()
 
-		level.Debug(n.logger).Log("msg", "Sending Wechat  message", "incident", key, "url", u.String())
+		level.Debug(n.logger).Log("msg", "Sending Wechat message", "incident", key, "url", u.String())
 
-		resp, err := ctxhttp.Get(ctx, http.DefaultClient, u.String())
+		req, err := http.NewRequest(http.MethodGet, u.String(), nil)
+		if err != nil {
+			return true, err
+		}
+
+		req.Header.Set("Content-Type", contentTypeJSON)
+
+		resp, err := http.DefaultClient.Do(req.WithContext(ctx))
 		if err != nil {
 			return true, err
 		}
@@ -880,10 +884,12 @@ func (n *Wechat) Notify(ctx context.Context, as ...*types.Alert) (bool, error) {
 			return false, err
 		}
 
-		accessToken = wechatToken.AccessToken
+		if wechatToken.AccessToken == "" {
+			return false, fmt.Errorf("invalid APISecret for CorpID: %s", n.conf.CorpID)
+		}
 
 		// Cache accessToken
-		n.accessToken = accessToken
+		n.accessToken = wechatToken.AccessToken
 		n.accessTokenAt = time.Now()
 	}
 
@@ -904,30 +910,42 @@ func (n *Wechat) Notify(ctx context.Context, as ...*types.Alert) (bool, error) {
 		return false, err
 	}
 
-	postMessageURL := n.conf.APIURL + "message/send?access_token=" + accessToken
+	postMessageURL := n.conf.APIURL + "message/send?access_token=" + n.accessToken
 
-	resp, err := ctxhttp.Post(ctx, http.DefaultClient, postMessageURL, contentTypeJSON, &buf)
+	req, err := http.NewRequest(http.MethodPost, postMessageURL, &buf)
 	if err != nil {
 		return true, err
 	}
 
+	resp, err := http.DefaultClient.Do(req.WithContext(ctx))
+	if err != nil {
+		return true, err
+	}
 	defer resp.Body.Close()
 
 	body, _ := ioutil.ReadAll(resp.Body)
 	level.Debug(n.logger).Log("msg", "response: "+string(body), "incident", key)
 
-	return n.retry(resp.StatusCode)
-}
+	if resp.StatusCode != 200 {
+		return true, fmt.Errorf("unexpected status code %v", resp.StatusCode)
+	} else {
+		var weResp weChatResponse
+		if err := json.Unmarshal(body, &weResp); err != nil {
+			return true, err
+		}
 
-func (n *Wechat) retry(statusCode int) (bool, error) {
-	// https://work.weixin.qq.com/api/doc#10649
-	if statusCode/100 == 5 || statusCode == 429 {
-		return true, fmt.Errorf("unexpected status code %v", statusCode)
-	} else if statusCode/100 != 2 {
-		return false, fmt.Errorf("unexpected status code %v", statusCode)
-	}
+		// https://work.weixin.qq.com/api/doc#10649
+		if weResp.Code == 0 {
+			return false, nil
+		}
 
-	return false, nil
+		// AccessToken is expired
+		if weResp.Code == 42001 {
+			return true, errors.New(weResp.Error)
+		}
+
+		return false, errors.New(weResp.Error)
+	}
 }
 
 // OpsGenie implements a Notifier for OpsGenie notifications.

notify/impl_test.go

@@ -10,12 +10,13 @@ import (
 	"github.com/stretchr/testify/require"
 	"golang.org/x/net/context"
 
+	"io/ioutil"
+	"net/url"
+
 	"github.com/prometheus/alertmanager/config"
 	"github.com/prometheus/alertmanager/template"
 	"github.com/prometheus/alertmanager/types"
 	"github.com/prometheus/common/model"
-	"io/ioutil"
-	"net/url"
 )
 
 func TestWebhookRetry(t *testing.T) {
@@ -63,15 +64,6 @@ func TestHipchatRetry(t *testing.T) {
 	}
 }
 
-func TestWechatRetry(t *testing.T) {
-	notifier := new(Wechat)
-	retryCodes := append(defaultRetryCodes(), http.StatusTooManyRequests)
-	for statusCode, expected := range retryTests(retryCodes) {
-		actual, _ := notifier.retry(statusCode)
-		require.Equal(t, expected, actual, fmt.Sprintf("error on status %d", statusCode))
-	}
-}
-
 func TestOpsGenieRetry(t *testing.T) {
 	notifier := new(OpsGenie)
 
@@ -267,3 +259,52 @@ func TestOpsGenie(t *testing.T) {
 	req, retry, err = notifier.createRequest(ctx, alert2)
 	require.Equal(t, expectedBody, readBody(t, req))
 }
+
+func TestWechat(t *testing.T) {
+	logger := log.NewNopLogger()
+	tmpl := createTmpl(t)
+
+	conf := &config.WechatConfig{
+		NotifierConfig: config.NotifierConfig{
+			VSendResolved: true,
+		},
+		Message: `{{ template "wechat.default.message" . }}`,
+		APIURL:  config.DefaultGlobalConfig.WeChatAPIURL,
+
+		APISecret: "invalidSecret",
+		CorpID:    "invalidCorpID",
+		AgentID:   "1",
+		ToUser:    "admin",
+	}
+	notifier := NewWechat(conf, tmpl, logger)
+
+	ctx := context.Background()
+
+	alert := &types.Alert{
+		Alert: model.Alert{
+			Labels: model.LabelSet{
+				"Message":     "message",
+				"Description": "description",
+				"Source":      "http://prometheus",
+				"Teams":       "TeamA,TeamB,",
+				"Tags":        "tag1,tag2",
+				"Note":        "this is a note",
+				"Priotity":    "P1",
+			},
+			StartsAt: time.Now(),
+			EndsAt:   time.Now().Add(time.Hour),
+		},
+	}
+
+	// miss group key
+	retry, err := notifier.Notify(ctx, alert)
+	require.False(t, retry)
+	require.Error(t, err)
+
+	ctx = WithGroupKey(ctx, "2")
+
+	// invalid secret
+	retry, err = notifier.Notify(ctx, alert)
+	require.False(t, retry)
+	require.Error(t, err)
+}