-
Notifications
You must be signed in to change notification settings - Fork 42
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
add support for TLSConfig in webhook receiver #247
base: main
Are you sure you want to change the base?
Conversation
@@ -111,13 +112,21 @@ func (wn *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error | |||
return false, tmplErr | |||
} | |||
|
|||
var tlsConfig *tls.Config | |||
if wn.settings.TLSConfig != nil { | |||
if tlsConfig, err = wn.settings.TLSConfig.ToCryptoTLSConfig(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly unrelated to this PR, but we are pretty much redefining the library here: https://github.com/grafana/dskit/blob/main/crypto/tls/tls.go#L87
It would need to be updated to take the certs/keys as a string though (currently only takes filepaths)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, that's a good point. You'd mention that library but I just went with the same setup that we had for mqtt and forgot about it.
The dskit is more complete also. I could adapt both to use that instead or create another issue to standardize it after this is merged - wdyt?
}, | ||
}, | ||
expMsgError: fmt.Errorf("Unable to use the provided CA certificate"), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add another test case for InsecureSkipVerify: true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, done now 👍
caa9015
to
e29973e
Compare
Add TLS configuration support for webhook receivers
Related PR's