fix(notifier): Handle nil pointer dereference

At notifier/plotting.go inside fetchAvailableSeries in case of error returned from `metricsSource.Fetch` only `local.ErrEvaluateTargetFailedWithPanic` was checked. Another errors did not handled properly and function was trying to call mehods of fetch result which became nil. That caused Nil Pointer Dereference panic. Add proper error handling to prevent nil pointer dereference. Closes #437
moira-alert · Nov 21, 2019 · 04054d1 · 04054d1
1 parent 3fa28a3
commit 04054d1
Show file tree

Hide file tree

Showing 2 changed files with 106 additions and 8 deletions.
diff --git a/notifier/plotting.go b/notifier/plotting.go
@@ -7,7 +7,7 @@ import (
 
 	"github.com/beevee/go-chart"
 	"github.com/moira-alert/moira"
-	"github.com/moira-alert/moira/metric_source"
+	metricSource "github.com/moira-alert/moira/metric_source"
 	"github.com/moira-alert/moira/metric_source/local"
 	"github.com/moira-alert/moira/plotting"
 )
@@ -128,15 +128,17 @@ func (notifier *StandardNotifier) evaluateTriggerMetrics(from, to int64, trigger
 // fetchAvailableSeries calls fetch function with realtime alerting and retries on fail without
 func fetchAvailableSeries(metricsSource metricSource.MetricSource, target string, from, to int64) ([]*metricSource.MetricData, error) {
 	realtimeFetchResult, realtimeErr := metricsSource.Fetch(target, from, to, true)
-	switch realtimeErr.(type) {
-	case local.ErrEvaluateTargetFailedWithPanic:
+	if realtimeErr == nil {
+		return realtimeFetchResult.GetMetricsData(), realtimeErr
+	}
+	if errFailedWithPanic, ok := realtimeErr.(local.ErrEvaluateTargetFailedWithPanic); ok {
 		fetchResult, err := metricsSource.Fetch(target, from, to, false)
 		if err != nil {
-			return nil, errFetchAvailableSeriesFailed{realtimeErr: realtimeErr.Error(), storedErr: err.Error()}
+			return nil, errFetchAvailableSeriesFailed{realtimeErr: errFailedWithPanic.Error(), storedErr: err.Error()}
 		}
 		return fetchResult.GetMetricsData(), nil
 	}
-	return realtimeFetchResult.GetMetricsData(), realtimeErr
+	return nil, realtimeErr
 }
 
 // getMetricDataToShow returns MetricData limited by whitelist

diff --git a/notifier/plotting_test.go b/notifier/plotting_test.go
@@ -1,15 +1,18 @@
 package notifier
 
 import (
+	"errors"
 	"fmt"
 	"testing"
 	"time"
 
-	"github.com/moira-alert/moira/metric_source"
+	"github.com/golang/mock/gomock"
+	"github.com/moira-alert/moira"
+	metricSource "github.com/moira-alert/moira/metric_source"
+	"github.com/moira-alert/moira/metric_source/local"
+	mockMetricSource "github.com/moira-alert/moira/mock/metric_source"
 	"github.com/op/go-logging"
 	. "github.com/smartystreets/goconvey/convey"
-
-	"github.com/moira-alert/moira"
 )
 
 func TestResolveMetricsWindow(t *testing.T) {
@@ -127,3 +130,96 @@ func TestGetMetricDataToShow(t *testing.T) {
 		So(len(metricsData), ShouldEqual, len(givenSeries))
 	})
 }
+
+type MockCfg func(*mockMetricSource.MockMetricSource, *mockMetricSource.MockFetchResult) (*mockMetricSource.MockMetricSource, *mockMetricSource.MockFetchResult)
+
+func Test_fetchAvailableSeries(t *testing.T) {
+	type args struct {
+		target string
+		from   int64
+		to     int64
+	}
+	tests := []struct {
+		name    string
+		args    args
+		mockCfg MockCfg
+		wantErr bool
+	}{
+		{
+			name: "Run without error",
+			args: args{
+				target: "testTarget",
+				from:   17,
+				to:     67,
+			},
+			mockCfg: func(source *mockMetricSource.MockMetricSource, result *mockMetricSource.MockFetchResult) (*mockMetricSource.MockMetricSource, *mockMetricSource.MockFetchResult) {
+				result.EXPECT().GetMetricsData().Return(nil).Times(1)
+				source.EXPECT().Fetch("testTarget", int64(17), int64(67), true).Return(result, nil).Times(1)
+				return source, result
+			},
+			wantErr: false,
+		},
+		{
+			name: "Run with error ErrEvaluateTargetFailedWithPanic",
+			args: args{
+				target: "testTarget",
+				from:   17,
+				to:     67,
+			},
+			mockCfg: func(source *mockMetricSource.MockMetricSource, result *mockMetricSource.MockFetchResult) (*mockMetricSource.MockMetricSource, *mockMetricSource.MockFetchResult) {
+				result.EXPECT().GetMetricsData().Return(nil).Times(1)
+				err := local.ErrEvaluateTargetFailedWithPanic{}
+				source.EXPECT().Fetch("testTarget", int64(17), int64(67), true).Return(nil, err).Times(1)
+				source.EXPECT().Fetch("testTarget", int64(17), int64(67), false).Return(result, nil).Times(1)
+				return source, result
+			},
+			wantErr: false,
+		},
+		{
+			name: "Run with error ErrEvaluateTargetFailedWithPanic and error again",
+			args: args{
+				target: "testTarget",
+				from:   17,
+				to:     67,
+			},
+			mockCfg: func(source *mockMetricSource.MockMetricSource, result *mockMetricSource.MockFetchResult) (*mockMetricSource.MockMetricSource, *mockMetricSource.MockFetchResult) {
+				var err error = local.ErrEvaluateTargetFailedWithPanic{}
+				source.EXPECT().Fetch("testTarget", int64(17), int64(67), true).Return(nil, err).Times(1)
+				err = errors.New("Test error")
+				source.EXPECT().Fetch("testTarget", int64(17), int64(67), false).Return(nil, err).Times(1)
+				return source, result
+			},
+			wantErr: true,
+		},
+		{
+			name: "Run with unknown error",
+			args: args{
+				target: "testTarget",
+				from:   17,
+				to:     67,
+			},
+			mockCfg: func(source *mockMetricSource.MockMetricSource, result *mockMetricSource.MockFetchResult) (*mockMetricSource.MockMetricSource, *mockMetricSource.MockFetchResult) {
+				err := errors.New("Test error")
+				source.EXPECT().Fetch("testTarget", int64(17), int64(67), true).Return(nil, err).Times(1)
+				return source, result
+			},
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mockCtrl := gomock.NewController(t)
+			defer mockCtrl.Finish()
+
+			source := mockMetricSource.NewMockMetricSource(mockCtrl)
+			result := mockMetricSource.NewMockFetchResult(mockCtrl)
+			source, _ = tt.mockCfg(source, result)
+
+			_, err := fetchAvailableSeries(source, tt.args.target, tt.args.from, tt.args.to)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("fetchAvailableSeries() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+		})
+	}
+}