From 71324721a6889642733b420c1e25ffdff64b5196 Mon Sep 17 00:00:00 2001 From: subham sarkar Date: Mon, 24 Apr 2023 20:26:58 +0530 Subject: [PATCH] Refactor mongodb module to align with other modules --- .../module/mongodb/collstats/collstats.go | 28 ++-- metricbeat/module/mongodb/dbstats/dbstats.go | 11 +- metricbeat/module/mongodb/metrics/metrics.go | 11 +- metricbeat/module/mongodb/mongodb.go | 111 +++++----------- metricbeat/module/mongodb/mongodb_test.go | 125 ------------------ .../module/mongodb/replstatus/replstatus.go | 11 +- metricbeat/module/mongodb/status/status.go | 16 +-- 7 files changed, 65 insertions(+), 248 deletions(-) delete mode 100644 metricbeat/module/mongodb/mongodb_test.go diff --git a/metricbeat/module/mongodb/collstats/collstats.go b/metricbeat/module/mongodb/collstats/collstats.go index 5df4a52c595c..7cc6074af6cb 100644 --- a/metricbeat/module/mongodb/collstats/collstats.go +++ b/metricbeat/module/mongodb/collstats/collstats.go @@ -29,37 +29,33 @@ import ( ) func init() { - mb.Registry.MustAddMetricSet("mongodb", "collstats", New, - mb.WithHostParser(mongodb.ParseURL), - mb.DefaultMetricSet(), - ) + mb.Registry.MustAddMetricSet("mongodb", "collstats", New, mb.DefaultMetricSet()) } -// Metricset type defines all fields of the Metricset +// MetricSet type defines all fields of the MetricSet // As a minimum it must inherit the mb.BaseMetricSet fields, but can be extended with // additional entries. These variables can be used to persist data or configuration between // multiple fetch calls. -type Metricset struct { - *mongodb.Metricset +type MetricSet struct { + *mongodb.MetricSet } -// New creates a new instance of the Metricset +// New creates a new instance of the MetricSet // Part of new is also setting up the configuration by processing additional // configuration entries if needed. func New(base mb.BaseMetricSet) (mb.MetricSet, error) { - ms, err := mongodb.NewMetricset(base) + ms, err := mongodb.NewMetricSet(base) if err != nil { - return nil, fmt.Errorf("could not create mongodb metricset: %w", err) + return nil, err } - - return &Metricset{ms}, nil + return &MetricSet{ms}, nil } // Fetch methods implements the data gathering and data conversion to the right // format. It publishes the event which is then forwarded to the output. In case // of an error set the Error field of mb.Event or simply call report.Error(). -func (m *Metricset) Fetch(reporter mb.ReporterV2) error { - client, err := mongodb.NewClient(m.Metricset.Config, m.Module().Config().Timeout, 0) +func (m *MetricSet) Fetch(reporter mb.ReporterV2) error { + client, err := mongodb.NewClient(m.ClientOptions) if err != nil { return fmt.Errorf("could not create mongodb client: %w", err) } @@ -112,9 +108,7 @@ func (m *Metricset) Fetch(reporter mb.ReporterV2) error { continue } - reporter.Event(mb.Event{ - MetricSetFields: event, - }) + reporter.Event(mb.Event{MetricSetFields: event}) } return nil diff --git a/metricbeat/module/mongodb/dbstats/dbstats.go b/metricbeat/module/mongodb/dbstats/dbstats.go index 3fb6c8db3ea7..c029c7ba6be3 100644 --- a/metricbeat/module/mongodb/dbstats/dbstats.go +++ b/metricbeat/module/mongodb/dbstats/dbstats.go @@ -33,10 +33,7 @@ import ( // init registers the MetricSet with the central registry. // The New method will be called after the setup of the module and before starting to fetch data func init() { - mb.Registry.MustAddMetricSet("mongodb", "dbstats", New, - mb.WithHostParser(mongodb.ParseURL), - mb.DefaultMetricSet(), - ) + mb.Registry.MustAddMetricSet("mongodb", "dbstats", New, mb.DefaultMetricSet()) } // MetricSet type defines all fields of the MetricSet @@ -44,14 +41,14 @@ func init() { // additional entries. These variables can be used to persist data or configuration between // multiple fetch calls. type MetricSet struct { - *mongodb.Metricset + *mongodb.MetricSet } // New creates a new instance of the MetricSet // Part of new is also setting up the configuration by processing additional // configuration entries if needed. func New(base mb.BaseMetricSet) (mb.MetricSet, error) { - ms, err := mongodb.NewMetricset(base) + ms, err := mongodb.NewMetricSet(base) if err != nil { return nil, err } @@ -62,7 +59,7 @@ func New(base mb.BaseMetricSet) (mb.MetricSet, error) { // format. It publishes the event which is then forwarded to the output. In case // of an error set the Error field of mb.Event or simply call report.Error(). func (m *MetricSet) Fetch(reporter mb.ReporterV2) error { - client, err := mongodb.NewClient(m.Metricset.Config, m.Module().Config().Timeout, 0) + client, err := mongodb.NewClient(m.ClientOptions) if err != nil { return fmt.Errorf("could not create mongodb client: %w", err) } diff --git a/metricbeat/module/mongodb/metrics/metrics.go b/metricbeat/module/mongodb/metrics/metrics.go index 79a8cd436fb1..fc5a1c8b4cba 100644 --- a/metricbeat/module/mongodb/metrics/metrics.go +++ b/metricbeat/module/mongodb/metrics/metrics.go @@ -29,10 +29,7 @@ import ( ) func init() { - mb.Registry.MustAddMetricSet("mongodb", "metrics", New, - mb.WithHostParser(mongodb.ParseURL), - mb.DefaultMetricSet(), - ) + mb.Registry.MustAddMetricSet("mongodb", "metrics", New, mb.DefaultMetricSet()) } // MetricSet type defines all fields of the MetricSet @@ -40,14 +37,14 @@ func init() { // additional entries. These variables can be used to persist data or configuration between // multiple fetch calls. type MetricSet struct { - *mongodb.Metricset + *mongodb.MetricSet } // New creates a new instance of the MetricSet // Part of new is also setting up the configuration by processing additional // configuration entries if needed. func New(base mb.BaseMetricSet) (mb.MetricSet, error) { - ms, err := mongodb.NewMetricset(base) + ms, err := mongodb.NewMetricSet(base) if err != nil { return nil, err } @@ -58,7 +55,7 @@ func New(base mb.BaseMetricSet) (mb.MetricSet, error) { // format. It publishes the event which is then forwarded to the output. In case // of an error set the Error field of mb.Event or simply call report.Error(). func (m *MetricSet) Fetch(reporter mb.ReporterV2) error { - client, err := mongodb.NewClient(m.Metricset.Config, m.Module().Config().Timeout, 0) + client, err := mongodb.NewClient(m.ClientOptions) if err != nil { return fmt.Errorf("could not create mongodb client: %w", err) } diff --git a/metricbeat/module/mongodb/mongodb.go b/metricbeat/module/mongodb/mongodb.go index 5539d84131f6..24dbf177c7d9 100644 --- a/metricbeat/module/mongodb/mongodb.go +++ b/metricbeat/module/mongodb/mongodb.go @@ -20,16 +20,13 @@ package mongodb import ( "context" "fmt" - "net/url" "strings" - "time" "go.mongodb.org/mongo-driver/mongo" "go.mongodb.org/mongo-driver/mongo/options" "go.mongodb.org/mongo-driver/mongo/readpref" "github.com/elastic/beats/v7/metricbeat/mb" - "github.com/elastic/beats/v7/metricbeat/mb/parse" "github.com/elastic/elastic-agent-libs/transport/tlscommon" ) @@ -59,9 +56,10 @@ type ModuleConfig struct { } `config:"credentials"` } -type Metricset struct { +type MetricSet struct { mb.BaseMetricSet - Config ModuleConfig + Config ModuleConfig + ClientOptions *options.ClientOptions } type module struct { @@ -74,72 +72,17 @@ func NewModule(base mb.BaseModule) (mb.Module, error) { return &module{base}, nil } -func NewMetricset(base mb.BaseMetricSet) (*Metricset, error) { +func NewMetricSet(base mb.BaseMetricSet) (*MetricSet, error) { // Validate that at least one host has been specified. config := ModuleConfig{} if err := base.Module().UnpackConfig(&config); err != nil { return nil, fmt.Errorf("could not read config: %w", err) } - return &Metricset{Config: config, BaseMetricSet: base}, nil -} - -// ParseURL parses valid MongoDB URL strings into an mb.HostData instance -func ParseURL(module mb.Module, host string) (mb.HostData, error) { - c := struct { - Username string `config:"username"` - Password string `config:"password"` - Credentials struct { - AuthMechanism string `config:"auth_mechanism"` - AuthMechanismProperties map[string]string `config:"auth_mechanism_properties"` - AuthSource string `config:"auth_source"` - PasswordSet bool `config:"password_set"` - } `config:"credentials"` - }{} - if err := module.UnpackConfig(&c); err != nil { - return mb.HostData{}, err - } - - if parts := strings.SplitN(host, "://", 2); len(parts) != 2 { - // Add scheme. - host = fmt.Sprintf("mongodb://%s", host) - } - - // This doesn't use URLHostParserBuilder because MongoDB URLs can contain - // multiple hosts separated by commas (mongodb://host1,host2,host3?options). - u, err := url.Parse(host) - if err != nil { - return mb.HostData{}, fmt.Errorf("error parsing URL: %w", err) - } - - parse.SetURLUser(u, c.Username, c.Password) - clientOptions := options.Client() - clientOptions.Auth = &options.Credential{ - AuthMechanism: c.Credentials.AuthMechanism, - AuthMechanismProperties: c.Credentials.AuthMechanismProperties, - AuthSource: c.Credentials.AuthSource, - PasswordSet: c.Credentials.PasswordSet, - Username: c.Username, - Password: c.Password, - } - - clientOptions.ApplyURI(host) - - // https://docs.mongodb.com/manual/reference/connection-string/ - _, err = url.Parse(clientOptions.GetURI()) - if err != nil { - return mb.HostData{}, fmt.Errorf("error parsing URL: %w", err) - } - return parse.NewHostDataFromURL(u), nil -} - -func NewClient(config ModuleConfig, timeout time.Duration, mode readpref.Mode) (*mongo.Client, error) { - clientOptions := options.Client() - - // options.Credentials must be nil for the driver to work properly if no auth is provided. Zero values breaks - // the connnection + // options.Credentials must be nil for the driver to work properly if no auth is provided. + // Zero values breaks the connnection. if config.Username != "" && config.Password != "" { clientOptions.Auth = &options.Credential{ AuthMechanism: config.Credentials.AuthMechanism, @@ -155,33 +98,51 @@ func NewClient(config ModuleConfig, timeout time.Duration, mode readpref.Mode) ( } } - if mode == 0 { - mode = readpref.NearestMode + if config.TLS.IsEnabled() { + tlsConfig, err := tlscommon.LoadTLSConfig(config.TLS) + if err != nil { + return nil, fmt.Errorf("could not load provided TLS configuration: %w", err) + } + + clientOptions.SetTLSConfig(tlsConfig.ToConfig()) + } + + host := base.Host() + if parts := strings.SplitN(host, "://", 2); len(parts) != 2 { + host = "mongodb://" + host // add scheme + } + if err := clientOptions.ApplyURI(host).Validate(); err != nil { + return nil, fmt.Errorf("URL validation error: %w", err) } - readPreference, err := readpref.New(mode) + readPreference, err := readpref.New(readpref.PrimaryMode) if err != nil { return nil, err } + clientOptions.SetReadPreference(readPreference) - clientOptions.SetConnectTimeout(timeout) - if config.TLS.IsEnabled() { - tlsConfig, err := tlscommon.LoadTLSConfig(config.TLS) - if err != nil { - return nil, fmt.Errorf("could not load provided TLS configuration: %w", err) - } + // TODO(SS): Decide a better timeout? Currently, mb.DefaultModuleConfig().Timeout + // will make the client use no timeout. If we do not call this method, then by default, + // it is set to 30s. + clientOptions.SetConnectTimeout(mb.DefaultModuleConfig().Timeout) - clientOptions.SetTLSConfig(tlsConfig.ToConfig()) + if err := clientOptions.Validate(); err != nil { + return nil, fmt.Errorf("client options validation failed: %w", err) } - client, err := mongo.NewClient(clientOptions) + return &MetricSet{Config: config, ClientOptions: clientOptions, BaseMetricSet: base}, nil +} + +func NewClient(opts *options.ClientOptions) (*mongo.Client, error) { + client, err := mongo.NewClient(opts) if err != nil { return nil, fmt.Errorf("could not create mongodb client: %w", err) } if err = client.Connect(context.Background()); err != nil { - return client, fmt.Errorf("could not connect to mongodb: %w", err) + return nil, fmt.Errorf("could not connect to mongodb: %w", err) } + return client, nil } diff --git a/metricbeat/module/mongodb/mongodb_test.go b/metricbeat/module/mongodb/mongodb_test.go deleted file mode 100644 index 865e31642848..000000000000 --- a/metricbeat/module/mongodb/mongodb_test.go +++ /dev/null @@ -1,125 +0,0 @@ -// Licensed to Elasticsearch B.V. under one or more contributor -// license agreements. See the NOTICE file distributed with -// this work for additional information regarding copyright -// ownership. Elasticsearch B.V. licenses this file to you under -// the Apache License, Version 2.0 (the "License"); you may -// not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -package mongodb - -import ( - "testing" - - mbtest "github.com/elastic/beats/v7/metricbeat/mb/testing" - - "github.com/stretchr/testify/assert" -) - -func TestParseMongoURL(t *testing.T) { - tests := []struct { - Name string - URL string - Username string - Password string - ExpectedAddr string - ExpectedUsername string - ExpectedPassword string - }{ - { - Name: "basic test", - URL: "localhost:40001", - Username: "user", - Password: "secret", - - ExpectedAddr: "localhost:40001", - ExpectedUsername: "user", - ExpectedPassword: "secret", - }, - { - Name: "with schema", - URL: "mongodb://localhost:40001", - Username: "user", - Password: "secret", - - ExpectedAddr: "localhost:40001", - ExpectedUsername: "user", - ExpectedPassword: "secret", - }, - { - Name: "user password in url", - URL: "mongodb://user:secret@localhost:40001", - Username: "", - Password: "", - - ExpectedAddr: "localhost:40001", - ExpectedUsername: "user", - ExpectedPassword: "secret", - }, - { - Name: "username and password do not override", - URL: "mongodb://user:secret@localhost:40001", - Username: "anotheruser", - Password: "anotherpass", - - ExpectedAddr: "localhost:40001", - ExpectedUsername: "user", - ExpectedPassword: "secret", - }, - { - Name: "with options", - URL: "mongodb://localhost:40001/directConnection=true&authSource=me", - Username: "anotheruser", - Password: "anotherpass", - - ExpectedAddr: "localhost:40001", - ExpectedUsername: "anotheruser", - ExpectedPassword: "anotherpass", - }, - { - Name: "multiple hosts", - URL: "mongodb://localhost:40001,localhost:40002", - Username: "", - Password: "", - - ExpectedAddr: "localhost:40001,localhost:40002", - ExpectedUsername: "", - ExpectedPassword: "", - }, - { - Name: "with options replicaset", - URL: "mongodb://localhost:40001,localhost:40002/?replicaSet=dbrs", - Username: "anotheruser", - Password: "anotherpass", - - ExpectedAddr: "localhost:40001,localhost:40002", - ExpectedUsername: "anotheruser", - ExpectedPassword: "anotherpass", - }, - } - - for _, test := range tests { - mod := mbtest.NewTestModule(t, map[string]interface{}{ - "username": test.Username, - "password": test.Password, - }) - hostData, err := ParseURL(mod, test.URL) - if err != nil { - t.Error(err) - continue - } - - assert.Equal(t, test.ExpectedAddr, hostData.Host, test.Name) - assert.Equal(t, test.ExpectedUsername, hostData.User, test.Name) - assert.Equal(t, test.ExpectedPassword, hostData.Password, test.Name) - } -} diff --git a/metricbeat/module/mongodb/replstatus/replstatus.go b/metricbeat/module/mongodb/replstatus/replstatus.go index 05c66d80dbc5..b0d1e0bc032d 100644 --- a/metricbeat/module/mongodb/replstatus/replstatus.go +++ b/metricbeat/module/mongodb/replstatus/replstatus.go @@ -21,15 +21,12 @@ import ( "context" "fmt" - "go.mongodb.org/mongo-driver/mongo/readpref" - "github.com/elastic/beats/v7/metricbeat/mb" "github.com/elastic/beats/v7/metricbeat/module/mongodb" ) func init() { - mb.Registry.MustAddMetricSet("mongodb", "replstatus", New, - mb.WithHostParser(mongodb.ParseURL)) + mb.Registry.MustAddMetricSet("mongodb", "replstatus", New) } // MetricSet type defines all fields of the MetricSet @@ -37,14 +34,14 @@ func init() { // additional entries. These variables can be used to persist data or configuration between // multiple fetch calls. type MetricSet struct { - *mongodb.Metricset + *mongodb.MetricSet } // New creates a new instance of the MetricSet // Part of new is also setting up the configuration by processing additional // configuration entries if needed. func New(base mb.BaseMetricSet) (mb.MetricSet, error) { - ms, err := mongodb.NewMetricset(base) + ms, err := mongodb.NewMetricSet(base) if err != nil { return nil, err } @@ -55,7 +52,7 @@ func New(base mb.BaseMetricSet) (mb.MetricSet, error) { // format. It publishes the event which is then forwarded to the output. In case // of an error set the Error field of mb.Event or simply call report.Error(). func (m *MetricSet) Fetch(reporter mb.ReporterV2) error { - client, err := mongodb.NewClient(m.Metricset.Config, m.Module().Config().Timeout, readpref.PrimaryMode) + client, err := mongodb.NewClient(m.ClientOptions) if err != nil { return fmt.Errorf("could not create mongodb client: %w", err) } diff --git a/metricbeat/module/mongodb/status/status.go b/metricbeat/module/mongodb/status/status.go index 1c7f8c6a863a..8399b716ee80 100644 --- a/metricbeat/module/mongodb/status/status.go +++ b/metricbeat/module/mongodb/status/status.go @@ -23,7 +23,6 @@ import ( "go.mongodb.org/mongo-driver/bson" "go.mongodb.org/mongo-driver/bson/primitive" - "go.mongodb.org/mongo-driver/mongo/readpref" "github.com/elastic/beats/v7/metricbeat/mb" "github.com/elastic/beats/v7/metricbeat/module/mongodb" @@ -32,10 +31,7 @@ import ( ) func init() { - mb.Registry.MustAddMetricSet("mongodb", "status", New, - mb.WithHostParser(mongodb.ParseURL), - mb.DefaultMetricSet(), - ) + mb.Registry.MustAddMetricSet("mongodb", "status", New, mb.DefaultMetricSet()) } // MetricSet type defines all fields of the MetricSet @@ -43,14 +39,14 @@ func init() { // additional entries. These variables can be used to persist data or configuration between // multiple fetch calls. type MetricSet struct { - *mongodb.Metricset + *mongodb.MetricSet } // New creates a new instance of the MetricSet // Part of new is also setting up the configuration by processing additional // configuration entries if needed. func New(base mb.BaseMetricSet) (mb.MetricSet, error) { - ms, err := mongodb.NewMetricset(base) + ms, err := mongodb.NewMetricSet(base) if err != nil { return nil, err } @@ -60,8 +56,8 @@ func New(base mb.BaseMetricSet) (mb.MetricSet, error) { // Fetch methods implements the data gathering and data conversion to the right format // It returns the event which is then forward to the output. In case of an error, a // descriptive error must be returned. -func (m *MetricSet) Fetch(r mb.ReporterV2) error { - client, err := mongodb.NewClient(m.Metricset.Config, m.Module().Config().Timeout, readpref.PrimaryMode) +func (m *MetricSet) Fetch(reporter mb.ReporterV2) error { + client, err := mongodb.NewClient(m.ClientOptions) if err != nil { return fmt.Errorf("could not create mongodb client: %w", err) } @@ -105,7 +101,7 @@ func (m *MetricSet) Fetch(r mb.ReporterV2) error { _, _ = event.RootFields.Put("process.name", v) _ = event.MetricSetFields.Delete("process") } - r.Event(event) + reporter.Event(event) return nil }