From f0bd307d3ccda8e3ff9d8d62427e04e24d6113b5 Mon Sep 17 00:00:00 2001 From: mathetake Date: Sun, 15 Mar 2020 18:44:05 +0900 Subject: [PATCH] fix: nil pointer on notifier --- charts/flagger/templates/deployment.yaml | 4 ++ cmd/flagger/main.go | 18 ++++----- pkg/canary/daemonset_controller.go | 2 +- pkg/canary/deployment_controller.go | 2 +- pkg/controller/events.go | 4 -- .../scheduler_daemonset_fixture_test.go | 2 + .../scheduler_deployment_fixture_test.go | 2 + pkg/notifier/factory.go | 39 ++++++++++++------- pkg/notifier/nop.go | 7 ++++ 9 files changed, 51 insertions(+), 29 deletions(-) create mode 100644 pkg/notifier/nop.go diff --git a/charts/flagger/templates/deployment.yaml b/charts/flagger/templates/deployment.yaml index f5c264a4..5d6c5278 100644 --- a/charts/flagger/templates/deployment.yaml +++ b/charts/flagger/templates/deployment.yaml @@ -85,7 +85,11 @@ spec: {{- end }} {{- if .Values.slack.url }} - -slack-url={{ .Values.slack.url }} + {{- end }} + {{- if .Values.slack.user }} - -slack-user={{ .Values.slack.user }} + {{- end }} + {{- if .Values.slack.channel }} - -slack-channel={{ .Values.slack.channel }} {{- end }} {{- if .Values.msteams.url }} diff --git a/cmd/flagger/main.go b/cmd/flagger/main.go index ec9ef41b..1c31f677 100644 --- a/cmd/flagger/main.go +++ b/cmd/flagger/main.go @@ -318,21 +318,19 @@ func initNotifier(logger *zap.SugaredLogger) (client notifier.Interface) { } notifierFactory := notifier.NewFactory(notifierURL, slackUser, slackChannel) - if notifierURL != "" { - var err error - client, err = notifierFactory.Notifier(provider) - if err != nil { - logger.Errorf("Notifier %v", err) - } else { - logger.Infof("Notifications enabled for %s", notifierURL[0:30]) - } + var err error + client, err = notifierFactory.Notifier(provider) + if err != nil { + logger.Errorf("Notifier %v", err) + } else if len(notifierURL) > 30 { + logger.Infof("Notifications enabled for %s", notifierURL[0:30]) } return } func fromEnv(envVar string, defaultVal string) string { - if os.Getenv(envVar) != "" { - return os.Getenv(envVar) + if v := os.Getenv(envVar); v != "" { + return v } return defaultVal } diff --git a/pkg/canary/daemonset_controller.go b/pkg/canary/daemonset_controller.go index 4beb1b1d..0642f6b8 100644 --- a/pkg/canary/daemonset_controller.go +++ b/pkg/canary/daemonset_controller.go @@ -289,7 +289,7 @@ func (c *DaemonSetController) getSelectorLabel(daemonSet *appsv1.DaemonSet) (str return "", fmt.Errorf( "daemonset %s.%s spec.selector.matchLabels must contain one of %v'", - c.labels, daemonSet.Name, daemonSet.Namespace, + daemonSet.Name, daemonSet.Namespace, c.labels, ) } diff --git a/pkg/canary/deployment_controller.go b/pkg/canary/deployment_controller.go index 41ff1b57..3a89526c 100644 --- a/pkg/canary/deployment_controller.go +++ b/pkg/canary/deployment_controller.go @@ -369,7 +369,7 @@ func (c *DeploymentController) getSelectorLabel(deployment *appsv1.Deployment) ( return "", fmt.Errorf( "deployment %s.%s spec.selector.matchLabels must contain one of %v", - c.labels, deployment.Name, deployment.Namespace, + deployment.Name, deployment.Namespace, c.labels, ) } diff --git a/pkg/controller/events.go b/pkg/controller/events.go index 6f9740c2..cc7bd22b 100644 --- a/pkg/controller/events.go +++ b/pkg/controller/events.go @@ -50,10 +50,6 @@ func (c *Controller) sendEventToWebhook(r *flaggerv1.Canary, eventType, template } func (c *Controller) alert(canary *flaggerv1.Canary, message string, metadata bool, severity flaggerv1.AlertSeverity) { - if c.notifier == nil && len(canary.GetAnalysis().Alerts) == 0 { - return - } - var fields []notifier.Field if metadata { fields = alertMetadata(canary) diff --git a/pkg/controller/scheduler_daemonset_fixture_test.go b/pkg/controller/scheduler_daemonset_fixture_test.go index 62b61667..5ce0cacb 100644 --- a/pkg/controller/scheduler_daemonset_fixture_test.go +++ b/pkg/controller/scheduler_daemonset_fixture_test.go @@ -24,6 +24,7 @@ import ( "github.com/weaveworks/flagger/pkg/logger" "github.com/weaveworks/flagger/pkg/metrics" "github.com/weaveworks/flagger/pkg/metrics/observers" + "github.com/weaveworks/flagger/pkg/notifier" "github.com/weaveworks/flagger/pkg/router" ) @@ -102,6 +103,7 @@ func newDaemonSetFixture(c *flaggerv1.Canary) daemonSetFixture { observerFactory: observerFactory, recorder: metrics.NewRecorder(controllerAgentName, false), routerFactory: rf, + notifier: ¬ifier.NopNotifier{}, } ctrl.flaggerSynced = alwaysReady ctrl.flaggerInformers.CanaryInformer.Informer().GetIndexer().Add(c) diff --git a/pkg/controller/scheduler_deployment_fixture_test.go b/pkg/controller/scheduler_deployment_fixture_test.go index 77f7f3a5..3006d3eb 100644 --- a/pkg/controller/scheduler_deployment_fixture_test.go +++ b/pkg/controller/scheduler_deployment_fixture_test.go @@ -25,6 +25,7 @@ import ( "github.com/weaveworks/flagger/pkg/logger" "github.com/weaveworks/flagger/pkg/metrics" "github.com/weaveworks/flagger/pkg/metrics/observers" + "github.com/weaveworks/flagger/pkg/notifier" "github.com/weaveworks/flagger/pkg/router" ) @@ -104,6 +105,7 @@ func newDeploymentFixture(c *flaggerv1.Canary) fixture { observerFactory: observerFactory, recorder: metrics.NewRecorder(controllerAgentName, false), routerFactory: rf, + notifier: ¬ifier.NopNotifier{}, } ctrl.flaggerSynced = alwaysReady ctrl.flaggerInformers.CanaryInformer.Informer().GetIndexer().Add(c) diff --git a/pkg/notifier/factory.go b/pkg/notifier/factory.go index 8bcab4df..3aad5806 100644 --- a/pkg/notifier/factory.go +++ b/pkg/notifier/factory.go @@ -1,6 +1,8 @@ package notifier -import "fmt" +import ( + "fmt" +) type Factory struct { URL string @@ -8,25 +10,36 @@ type Factory struct { Channel string } -func NewFactory(URL string, username string, channel string) *Factory { +func NewFactory(url string, username string, channel string) *Factory { return &Factory{ - URL: URL, + URL: url, Channel: channel, Username: username, } } func (f Factory) Notifier(provider string) (Interface, error) { - switch provider { - case "slack": - return NewSlack(f.URL, f.Username, f.Channel) - case "discord": - return NewDiscord(f.URL, f.Username, f.Channel) - case "rocket": - return NewRocket(f.URL, f.Username, f.Channel) - case "msteams": - return NewMSTeams(f.URL) + if f.URL == "" { + return &NopNotifier{}, nil } - return nil, fmt.Errorf("provider %s not supported", provider) + var n Interface + var err error + switch provider { + case "slack": + n, err = NewSlack(f.URL, f.Username, f.Channel) + case "discord": + n, err = NewDiscord(f.URL, f.Username, f.Channel) + case "rocket": + n, err = NewRocket(f.URL, f.Username, f.Channel) + case "msteams": + n, err = NewMSTeams(f.URL) + default: + err = fmt.Errorf("provider %s not supported", provider) + } + + if err != nil { + n = &NopNotifier{} + } + return n, err } diff --git a/pkg/notifier/nop.go b/pkg/notifier/nop.go new file mode 100644 index 00000000..b8eced3b --- /dev/null +++ b/pkg/notifier/nop.go @@ -0,0 +1,7 @@ +package notifier + +type NopNotifier struct{} + +func (n *NopNotifier) Post(string, string, string, []Field, string) error { + return nil +}