Metricbeat: MongoDB TLS connection support #7401
Conversation
jsoriano
requested review from
ruflin and
acchen97
and removed request for
acchen97
| # Username to use when connecting to MongoDB. Empty by default. | ||
| #username: user | ||
|
|
||
| # Password to use when connecting to MongoDB. Empty by default. | ||
| #password: pass | ||
| ---- | ||
|
|
||
| This module supports TLS connection when using `ssl` config field, as described in <<configuration-ssl>>. It also supports the options described in <<module-http-config-options>>. |
There was a problem hiding this comment.
This message is autogenerated after adding the ssl settings to fields.yml. I think it shouldn't mention by default the http options, as for example this module uses TLS but doesn't use HTTP. To be fixed in a future PR.
| } | ||
|
|
||
| dialInfo.DialServer = func(addr *mgo.ServerAddr) (net.Conn, error) { | ||
| return tls.Dial("tcp", addr.String(), tlsConfig.BuildModuleConfig("")) |
There was a problem hiding this comment.
Should we pass addr.String() to tlsConfig.BuildModuleConfig instead of ""? Maybe it doesn't matter but reading through https://golang.org/pkg/crypto/tls/#Config it appears this value is used for hostname verification.
There was a problem hiding this comment.
In other uses of this function we are passing the empty string but yes, it'd make sense to pass the hostname in the address, I'll try this.
| @@ -47,24 +46,18 @@ func init() { | |||
| // additional entries. These variables can be used to persist data or configuration between | |||
| // multiple fetch calls. | |||
| type MetricSet struct { | |||
| mb.BaseMetricSet | |||
| dialInfo *mgo.DialInfo | |||
| *mongodb.MetricSet | |||
There was a problem hiding this comment.
This may be an invalid/stupid question because I'm still new to beats + golang but why do we need to embed the *mongodb.MetricSet type inside this MetricSet struct? Couldn't we get rid of the outer/wrapper struct now and in the New function below do this instead:
func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
return mongodb.NewMetricSet(base)
}There was a problem hiding this comment.
There are no stupid questions, this is a good one :)
Here all the metricsets share the same fields, so I use the same builder, but they need different implementations for the Fetch() method, so I embed the common data in structs implementing these specializations.
|
@ycombinator thanks for your review! I have added the server name to the TLS config, but after some tests I couldn't see any change in how it behaves. It seems that the dialer method already tries to infer the server name from the address if it is not set. In any case I'm fine with leaving it explicit. |
| # Mode of verification of server certificate ('none' or 'full') | ||
| #ssl.verification_mode: 'full' | ||
|
|
||
| # List of root certificates for HTTPS server verifications |
There was a problem hiding this comment.
Probably should say TLS or SSL instead of HTTPS (since there's no HTTP connection here)?
Add support for TLS in MongoDB metricbeat module. A common builder is also added for MongoDB metricsets.
Fixes #6619