From ded5d22cf4e3e85a0e981695f8e42f1e7b66a49b Mon Sep 17 00:00:00 2001 From: Jaime Soriano Pastor Date: Tue, 21 Jan 2020 15:46:53 +0100 Subject: [PATCH] Remove datasource option from SQL module and add tests (#15686) Remove datasource option from SQL module. This option was intended to set the DSN of a database connection, and we were ignoring the hosts setting. In other SQL modules we are using the values in hosts as DSNs, do here the same for consistency. Host is redacted when we cannot parse it as it can contain passwords. StandardizeEvent is exposed in mbtest.Fetcher interface so we can more easily check contents of events in tests. Add integration tests of the module with MySQL and PostgreSQL. Add real data.json with data from MySQL and PostgreSQL. (cherry picked from commit 8c71abc3dd960e510ae34d91e464fcd913676455) --- metricbeat/docs/modules/sql.asciidoc | 5 +- metricbeat/mb/testing/fetcher.go | 14 ++ x-pack/metricbeat/metricbeat.reference.yml | 3 +- x-pack/metricbeat/module/sql/_meta/config.yml | 3 +- .../metricbeat/module/sql/_meta/docs.asciidoc | 2 +- .../metricbeat/module/sql/docker-compose.yml | 12 ++ .../module/sql/query/_meta/data.json | 46 ++++--- .../module/sql/query/_meta/data_postgres.json | 45 +++++++ x-pack/metricbeat/module/sql/query/dsn.go | 42 ++++++ x-pack/metricbeat/module/sql/query/query.go | 20 ++- .../sql/query/query_integration_test.go | 126 ++++++++++++++++++ x-pack/metricbeat/modules.d/sql.yml.disabled | 3 +- 12 files changed, 279 insertions(+), 42 deletions(-) create mode 100644 x-pack/metricbeat/module/sql/docker-compose.yml create mode 100644 x-pack/metricbeat/module/sql/query/_meta/data_postgres.json create mode 100644 x-pack/metricbeat/module/sql/query/dsn.go create mode 100644 x-pack/metricbeat/module/sql/query/query_integration_test.go diff --git a/metricbeat/docs/modules/sql.asciidoc b/metricbeat/docs/modules/sql.asciidoc index 30e5baf89da6..c3f3d412ea77 100644 --- a/metricbeat/docs/modules/sql.asciidoc +++ b/metricbeat/docs/modules/sql.asciidoc @@ -8,7 +8,7 @@ This file is generated! See scripts/mage/docs_collector.go beta[] -This is the sql module that fetches metrics from a SQL database. You can define driver, datasource and SQL query. +This is the sql module that fetches metrics from a SQL database. You can define driver and SQL query. @@ -26,10 +26,9 @@ metricbeat.modules: metricsets: - query period: 10s - hosts: ["localhost"] + hosts: ["user=myuser password=mypassword dbname=mydb sslmode=disable"] driver: "postgres" - datasource: "user=myuser password=mypassword dbname=mydb sslmode=disable" sql_query: "select now()" ---- diff --git a/metricbeat/mb/testing/fetcher.go b/metricbeat/mb/testing/fetcher.go index be8264b5cbb5..5b01e1b81382 100644 --- a/metricbeat/mb/testing/fetcher.go +++ b/metricbeat/mb/testing/fetcher.go @@ -20,6 +20,7 @@ package testing import ( "testing" + "github.com/elastic/beats/libbeat/beat" "github.com/elastic/beats/libbeat/common" "github.com/elastic/beats/metricbeat/mb" ) @@ -32,6 +33,7 @@ type Fetcher interface { FetchEvents() ([]mb.Event, []error) WriteEvents(testing.TB, string) WriteEventsCond(testing.TB, string, func(common.MapStr) bool) + StandardizeEvent(mb.Event, ...mb.EventModifier) beat.Event } // NewFetcher returns a test fetcher from a Metricset configuration @@ -73,6 +75,10 @@ func (f *reportingMetricSetV2Fetcher) WriteEventsCond(t testing.TB, path string, } } +func (f *reportingMetricSetV2Fetcher) StandardizeEvent(event mb.Event, modifiers ...mb.EventModifier) beat.Event { + return StandardizeEvent(f, event, modifiers...) +} + type reportingMetricSetV2FetcherError struct { mb.ReportingMetricSetV2Error } @@ -96,6 +102,10 @@ func (f *reportingMetricSetV2FetcherError) WriteEventsCond(t testing.TB, path st } } +func (f *reportingMetricSetV2FetcherError) StandardizeEvent(event mb.Event, modifiers ...mb.EventModifier) beat.Event { + return StandardizeEvent(f, event, modifiers...) +} + type reportingMetricSetV2FetcherWithContext struct { mb.ReportingMetricSetV2WithContext } @@ -118,3 +128,7 @@ func (f *reportingMetricSetV2FetcherWithContext) WriteEventsCond(t testing.TB, p t.Fatal("writing events", err) } } + +func (f *reportingMetricSetV2FetcherWithContext) StandardizeEvent(event mb.Event, modifiers ...mb.EventModifier) beat.Event { + return StandardizeEvent(f, event, modifiers...) +} diff --git a/x-pack/metricbeat/metricbeat.reference.yml b/x-pack/metricbeat/metricbeat.reference.yml index 7eef53265a2b..60e79272f778 100644 --- a/x-pack/metricbeat/metricbeat.reference.yml +++ b/x-pack/metricbeat/metricbeat.reference.yml @@ -971,10 +971,9 @@ metricbeat.modules: metricsets: - query period: 10s - hosts: ["localhost"] + hosts: ["user=myuser password=mypassword dbname=mydb sslmode=disable"] driver: "postgres" - datasource: "user=myuser password=mypassword dbname=mydb sslmode=disable" sql_query: "select now()" diff --git a/x-pack/metricbeat/module/sql/_meta/config.yml b/x-pack/metricbeat/module/sql/_meta/config.yml index 92b5f3882210..5c6a419e77ba 100644 --- a/x-pack/metricbeat/module/sql/_meta/config.yml +++ b/x-pack/metricbeat/module/sql/_meta/config.yml @@ -2,9 +2,8 @@ metricsets: - query period: 10s - hosts: ["localhost"] + hosts: ["user=myuser password=mypassword dbname=mydb sslmode=disable"] driver: "postgres" - datasource: "user=myuser password=mypassword dbname=mydb sslmode=disable" sql_query: "select now()" diff --git a/x-pack/metricbeat/module/sql/_meta/docs.asciidoc b/x-pack/metricbeat/module/sql/_meta/docs.asciidoc index d7bb818a58b8..f22edf1fa200 100644 --- a/x-pack/metricbeat/module/sql/_meta/docs.asciidoc +++ b/x-pack/metricbeat/module/sql/_meta/docs.asciidoc @@ -1,3 +1,3 @@ -This is the sql module that fetches metrics from a SQL database. You can define driver, datasource and SQL query. +This is the sql module that fetches metrics from a SQL database. You can define driver and SQL query. diff --git a/x-pack/metricbeat/module/sql/docker-compose.yml b/x-pack/metricbeat/module/sql/docker-compose.yml new file mode 100644 index 000000000000..a053c322d304 --- /dev/null +++ b/x-pack/metricbeat/module/sql/docker-compose.yml @@ -0,0 +1,12 @@ +version: '2.3' + +services: + mysql: + extends: + file: ../../../../metricbeat/module/mysql/docker-compose.yml + service: mysql + + postgresql: + extends: + file: ../../../../metricbeat/module/postgresql/docker-compose.yml + service: postgresql diff --git a/x-pack/metricbeat/module/sql/query/_meta/data.json b/x-pack/metricbeat/module/sql/query/_meta/data.json index 1a92415be34c..799c66fe7bc9 100644 --- a/x-pack/metricbeat/module/sql/query/_meta/data.json +++ b/x-pack/metricbeat/module/sql/query/_meta/data.json @@ -1,26 +1,30 @@ { - "@timestamp":"2016-05-23T08:05:34.853Z", - "beat":{ - "hostname":"beathost", - "name":"beathost" + "@timestamp": "2017-10-12T08:05:34.853Z", + "event": { + "dataset": "sql.query", + "duration": 115000, + "module": "sql" }, - "metricset":{ - "host":"localhost", - "module":"sql", - "name":"query", - "rtt":44269 + "metricset": { + "name": "query", + "period": 10000 }, - "sql":{ - "metrics":{ - "numeric":{ - "mynumericfield":1 - }, - "string":{ - "mystringfield":"abc" - } + "service": { + "address": "172.22.0.3:3306", + "type": "sql" + }, + "sql": { + "driver": "mysql", + "metrics": { + "numeric": { + "table_rows": 6 + }, + "string": { + "engine": "InnoDB", + "table_name": "sys_config", + "table_schema": "sys" + } }, - "driver":"postgres", - "query":"select * from mytable" - }, - "type":"metricsets" + "query": "select table_schema, table_name, engine, table_rows from information_schema.tables where table_rows \u003e 0;" + } } \ No newline at end of file diff --git a/x-pack/metricbeat/module/sql/query/_meta/data_postgres.json b/x-pack/metricbeat/module/sql/query/_meta/data_postgres.json new file mode 100644 index 000000000000..0f2db40c5b12 --- /dev/null +++ b/x-pack/metricbeat/module/sql/query/_meta/data_postgres.json @@ -0,0 +1,45 @@ +{ + "@timestamp": "2017-10-12T08:05:34.853Z", + "event": { + "dataset": "sql.query", + "duration": 115000, + "module": "sql" + }, + "metricset": { + "name": "query", + "period": 10000 + }, + "service": { + "address": "172.22.0.2:5432", + "type": "sql" + }, + "sql": { + "driver": "postgres", + "metrics": { + "numeric": { + "blk_read_time": 0, + "blk_write_time": 0, + "blks_hit": 1923, + "blks_read": 111, + "conflicts": 0, + "datid": 12379, + "deadlocks": 0, + "numbackends": 1, + "temp_bytes": 0, + "temp_files": 0, + "tup_deleted": 0, + "tup_fetched": 1249, + "tup_inserted": 0, + "tup_returned": 1356, + "tup_updated": 0, + "xact_commit": 18, + "xact_rollback": 0 + }, + "string": { + "datname": "postgres", + "stats_reset": "2020-01-21 11:23:56.53" + } + }, + "query": "select * from pg_stat_database" + } +} \ No newline at end of file diff --git a/x-pack/metricbeat/module/sql/query/dsn.go b/x-pack/metricbeat/module/sql/query/dsn.go new file mode 100644 index 000000000000..2a472c3fbe5b --- /dev/null +++ b/x-pack/metricbeat/module/sql/query/dsn.go @@ -0,0 +1,42 @@ +// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one +// or more contributor license agreements. Licensed under the Elastic License; +// you may not use this file except in compliance with the Elastic License. + +package query + +import ( + "net/url" + + "github.com/go-sql-driver/mysql" + + "github.com/elastic/beats/metricbeat/mb" +) + +// ParseDSN tries to parse the host +func ParseDSN(mod mb.Module, host string) (mb.HostData, error) { + // TODO: Add support for `username` and `password` as module options + + sanitized := sanitize(host) + + return mb.HostData{ + URI: host, + SanitizedURI: sanitized, + Host: sanitized, + }, nil +} + +func sanitize(host string) string { + // Host is a standard URL + if url, err := url.Parse(host); err == nil && len(url.Host) > 0 { + return url.Host + } + + // Host is a MySQL DSN + if config, err := mysql.ParseDSN(host); err == nil { + return config.Addr + } + + // TODO: Add support for PostgreSQL connection strings and other formats + + return "(redacted)" +} diff --git a/x-pack/metricbeat/module/sql/query/query.go b/x-pack/metricbeat/module/sql/query/query.go index 884df1b9df91..3322144d993d 100644 --- a/x-pack/metricbeat/module/sql/query/query.go +++ b/x-pack/metricbeat/module/sql/query/query.go @@ -10,13 +10,12 @@ import ( "strings" "time" + "github.com/jmoiron/sqlx" "github.com/pkg/errors" "github.com/elastic/beats/libbeat/common" "github.com/elastic/beats/libbeat/common/cfgwarn" "github.com/elastic/beats/metricbeat/mb" - - "github.com/jmoiron/sqlx" ) // init registers the MetricSet with the central registry as soon as the program @@ -24,7 +23,9 @@ import ( // the MetricSet for each host defined in the module's configuration. After the // MetricSet has been created then Fetch will begin to be called periodically. func init() { - mb.Registry.MustAddMetricSet("sql", "query", New) + mb.Registry.MustAddMetricSet("sql", "query", New, + mb.WithHostParser(ParseDSN), + ) } // MetricSet holds any configuration or state information. It must implement @@ -33,9 +34,8 @@ func init() { // interface methods except for Fetch. type MetricSet struct { mb.BaseMetricSet - Driver string - Datasource string - Query string + Driver string + Query string } // New creates a new instance of the MetricSet. New is responsible for unpacking @@ -44,9 +44,8 @@ func New(base mb.BaseMetricSet) (mb.MetricSet, error) { cfgwarn.Beta("The sql query metricset is beta.") config := struct { - Driver string `config:"driver"` - Datasource string `config:"datasource"` - Query string `config:"sql_query"` + Driver string `config:"driver"` + Query string `config:"sql_query"` }{} if err := base.Module().UnpackConfig(&config); err != nil { @@ -56,7 +55,6 @@ func New(base mb.BaseMetricSet) (mb.MetricSet, error) { return &MetricSet{ BaseMetricSet: base, Driver: config.Driver, - Datasource: config.Datasource, Query: config.Query, }, nil } @@ -65,7 +63,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(report mb.ReporterV2) error { - db, err := sqlx.Open(m.Driver, m.Datasource) + db, err := sqlx.Open(m.Driver, m.HostData().URI) if err != nil { return errors.Wrap(err, "error opening connection") } diff --git a/x-pack/metricbeat/module/sql/query/query_integration_test.go b/x-pack/metricbeat/module/sql/query/query_integration_test.go new file mode 100644 index 000000000000..d7e04f23c406 --- /dev/null +++ b/x-pack/metricbeat/module/sql/query/query_integration_test.go @@ -0,0 +1,126 @@ +// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one +// or more contributor license agreements. Licensed under the Elastic License; +// you may not use this file except in compliance with the Elastic License. + +// +build integration + +package query + +import ( + "fmt" + "net" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + // Drivers + _ "github.com/go-sql-driver/mysql" + _ "github.com/lib/pq" + + "github.com/elastic/beats/libbeat/beat" + "github.com/elastic/beats/libbeat/tests/compose" + "github.com/elastic/beats/metricbeat/mb" + mbtest "github.com/elastic/beats/metricbeat/mb/testing" + "github.com/elastic/beats/metricbeat/module/mysql" + "github.com/elastic/beats/metricbeat/module/postgresql" +) + +type testFetchConfig struct { + Driver string + Query string + Host string + + Assertion func(t *testing.T, event beat.Event) +} + +func TestMySQL(t *testing.T) { + service := compose.EnsureUp(t, "mysql") + config := testFetchConfig{ + Driver: "mysql", + Query: "select table_schema, table_name, engine, table_rows from information_schema.tables where table_rows > 0;", + Host: mysql.GetMySQLEnvDSN(service.Host()), + Assertion: assertFieldNotContains("service.address", ":test@"), + } + + t.Run("fetch", func(t *testing.T) { + testFetch(t, config) + }) + + t.Run("data", func(t *testing.T) { + testData(t, config, "") + }) +} + +func TestPostgreSQL(t *testing.T) { + service := compose.EnsureUp(t, "postgresql") + host, port, err := net.SplitHostPort(service.Host()) + require.NoError(t, err) + + user := postgresql.GetEnvUsername() + password := postgresql.GetEnvPassword() + + config := testFetchConfig{ + Driver: "postgres", + Query: "select * from pg_stat_database", + Host: fmt.Sprintf("user=%s password=%s sslmode=disable host=%s port=%s", user, password, host, port), + Assertion: assertFieldNotContains("service.address", "password="+password), + } + + t.Run("fetch", func(t *testing.T) { + testFetch(t, config) + }) + + config = testFetchConfig{ + Driver: "postgres", + Query: "select * from pg_stat_database", + Host: fmt.Sprintf("postgres://%s:%s@%s:%s/?sslmode=disable", user, password, host, port), + Assertion: assertFieldNotContains("service.address", ":"+password+"@"), + } + + t.Run("fetch with URL", func(t *testing.T) { + testFetch(t, config) + }) + + t.Run("data", func(t *testing.T) { + testData(t, config, "./_meta/data_postgres.json") + }) +} + +func testFetch(t *testing.T, cfg testFetchConfig) { + m := mbtest.NewFetcher(t, getConfig(cfg)) + events, errs := m.FetchEvents() + require.Empty(t, errs) + require.NotEmpty(t, events) + t.Logf("%s/%s event: %+v", m.Module().Name(), m.Name(), events[0]) + + if cfg.Assertion != nil { + for _, event := range events { + cfg.Assertion(t, m.StandardizeEvent(event, mb.AddMetricSetInfo)) + } + } +} + +func testData(t *testing.T, cfg testFetchConfig, postfix string) { + m := mbtest.NewFetcher(t, getConfig(cfg)) + m.WriteEvents(t, postfix) +} + +func getConfig(cfg testFetchConfig) map[string]interface{} { + return map[string]interface{}{ + "module": "sql", + "metricsets": []string{"query"}, + "hosts": []string{cfg.Host}, + "driver": cfg.Driver, + "sql_query": cfg.Query, + } +} + +func assertFieldNotContains(field, s string) func(t *testing.T, event beat.Event) { + return func(t *testing.T, event beat.Event) { + value, err := event.GetValue(field) + assert.NoError(t, err) + require.NotEmpty(t, value.(string)) + require.NotContains(t, value.(string), s) + } +} diff --git a/x-pack/metricbeat/modules.d/sql.yml.disabled b/x-pack/metricbeat/modules.d/sql.yml.disabled index 6dd178b4501a..4fd521c69eef 100644 --- a/x-pack/metricbeat/modules.d/sql.yml.disabled +++ b/x-pack/metricbeat/modules.d/sql.yml.disabled @@ -5,9 +5,8 @@ metricsets: - query period: 10s - hosts: ["localhost"] + hosts: ["user=myuser password=mypassword dbname=mydb sslmode=disable"] driver: "postgres" - datasource: "user=myuser password=mypassword dbname=mydb sslmode=disable" sql_query: "select now()"