Conversation
prymitive
force-pushed
the
tls-auth
branch
2 times, most recently
from
489d902 to
c4c28e2
Compare
docs/CONFIGURATION.md
Outdated
| Note that this option requires `tls:key` to be also set. | ||
| * `tls:key` - path to a TLS client key file to use when establishing | ||
| TLS connections to this Alertmanager instance. | ||
| Note that this option requires `tls:key` to be also set. |
There was a problem hiding this comment.
tls:cert. also might be worth being clear these are optional, even if tls:ca is set.
internal/alertmanager/models.go
Outdated
|
|
||
| start := time.Now() | ||
| silences, err := mapper.GetSilences(am.URI, am.RequestTimeout) | ||
| silences, err := mapper.GetSilences(am.URI, am.RequestTimeout, am.TLSConfig) |
There was a problem hiding this comment.
Why are you having to thread TLSConfig? I'd expect the transport to be allocated (with the TLS config) when the AM was added.
There was a problem hiding this comment.
yeah, that's not great, refactored to pass around tls.Config instead
anything else stands out here?
terinjokes
left a comment
terinjokes
left a comment
There was a problem hiding this comment.
In addition to what we talked about in our in-person code review.
internal/alertmanager/upstream.go
Outdated
| // WithTLSConfig option can be passed to NewAlertmanager in order to set | ||
| // custom options for the TLS handling when sending requests to Alertmanager | ||
| // upstream | ||
| func WithTLSConfig(tlsOptions transport.TLSConnectionOptions) Option { |
There was a problem hiding this comment.
Either rename this function to WithTLSOptions or similar or do this work to turn the YAML options into a standard tls.Config and pass that here instead.
There was a problem hiding this comment.
I think I'm gonna go with dedicated option for setting CA certs and client cert/key
…pair This adds support for passing a custom http.RoundTripper so in effect we can customize TLS client config for HTTPS requests. Fixes #184
There's identical method on the Alertmanager instance that's being used
This package adds handlers for different URI schemes, name clashes with http.Transport which is now passed around. Rename it to make it more obvious what it does
|
@terinjokes please review latest version, with previous refactoring this is now a smaller |
When we proxy requests we should use same transport as we use when collecting alerts & silences
2 participants
…pair
Fixes #184