Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: nil pointer on notifier #504

Merged
merged 1 commit into from
Mar 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions charts/flagger/templates/deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Expand Down
18 changes: 8 additions & 10 deletions cmd/flagger/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Collaborator Author

@mathetake mathetake Mar 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stefanprodan i am not sure what 30 means. could you tell me what is the purpose for slicing in the logging🤔 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to avoid logging secrets as the Slack/Teams URL will contain the API Key. But this is poorly done since the URL can be shorter for other providers (for example a self-hosted Rocket) and the info will never get logged.

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
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/canary/daemonset_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/canary/deployment_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
}

Expand Down
4 changes: 0 additions & 4 deletions pkg/controller/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions pkg/controller/scheduler_daemonset_fixture_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -102,6 +103,7 @@ func newDaemonSetFixture(c *flaggerv1.Canary) daemonSetFixture {
observerFactory: observerFactory,
recorder: metrics.NewRecorder(controllerAgentName, false),
routerFactory: rf,
notifier: &notifier.NopNotifier{},
}
ctrl.flaggerSynced = alwaysReady
ctrl.flaggerInformers.CanaryInformer.Informer().GetIndexer().Add(c)
Expand Down
2 changes: 2 additions & 0 deletions pkg/controller/scheduler_deployment_fixture_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -104,6 +105,7 @@ func newDeploymentFixture(c *flaggerv1.Canary) fixture {
observerFactory: observerFactory,
recorder: metrics.NewRecorder(controllerAgentName, false),
routerFactory: rf,
notifier: &notifier.NopNotifier{},
}
ctrl.flaggerSynced = alwaysReady
ctrl.flaggerInformers.CanaryInformer.Informer().GetIndexer().Add(c)
Expand Down
29 changes: 21 additions & 8 deletions pkg/notifier/factory.go
Original file line number Diff line number Diff line change
@@ -1,32 +1,45 @@
package notifier

import "fmt"
import (
"fmt"
)

type Factory struct {
URL string
Username string
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) {
if f.URL == "" {
return &NopNotifier{}, nil
}

var n Interface
var err error
switch provider {
case "slack":
return NewSlack(f.URL, f.Username, f.Channel)
n, err = NewSlack(f.URL, f.Username, f.Channel)
case "discord":
return NewDiscord(f.URL, f.Username, f.Channel)
n, err = NewDiscord(f.URL, f.Username, f.Channel)
case "rocket":
return NewRocket(f.URL, f.Username, f.Channel)
n, err = NewRocket(f.URL, f.Username, f.Channel)
case "msteams":
return NewMSTeams(f.URL)
n, err = NewMSTeams(f.URL)
default:
err = fmt.Errorf("provider %s not supported", provider)
}

return nil, fmt.Errorf("provider %s not supported", provider)
if err != nil {
n = &NopNotifier{}
}
return n, err
}
7 changes: 7 additions & 0 deletions pkg/notifier/nop.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package notifier

type NopNotifier struct{}

func (n *NopNotifier) Post(string, string, string, []Field, string) error {
return nil
}