From 8f5e7e4a792dd9969adbc7abd52811e096318f1a Mon Sep 17 00:00:00 2001 From: Jaime Soriano Pastor Date: Tue, 1 Sep 2020 14:07:47 +0200 Subject: [PATCH 1/5] Avoid generating incomplete configurations in autodiscover Handle errors when configuration unpacking fails. In principle this can only happen when some variable is missing, because configuration has been previously parsed as YAML. Errors on unpacking were previously ignored. When a variable is missing, this is clearly logged at the debug level. This changes the behaviour, previously an incomplete configuration was generated on this case. --- CHANGELOG.next.asciidoc | 2 ++ libbeat/autodiscover/template/config.go | 16 ++++++++++++++-- libbeat/autodiscover/template/config_test.go | 14 ++++++++++++++ 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc index afc0b40ac05..0e65687b229 100644 --- a/CHANGELOG.next.asciidoc +++ b/CHANGELOG.next.asciidoc @@ -20,6 +20,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d - Ensure dynamic template names are unique for the same field. {pull}18849[18849] - Remove the deprecated `xpack.monitoring.*` settings. Going forward only `monitoring.*` settings may be used. {issue}9424[9424] {pull}18608[18608] - Added `certificate` TLS verification mode to ignore server name mismatch. {issue}12283[12283] {pull}20293[20293] +- Autodiscover doesn't generate any configuration when a variable is missing. Previously it generated an incomplete configuration. {pull}[] *Auditbeat* @@ -167,6 +168,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d - [Metricbeat][Kubernetes] Change cluster_ip field from ip to keyword. {pull}20571[20571] - Rename cloud.provider `az` value to `azure` inside the add_cloud_metadata processor. {pull}20689[20689] - Add missing country_name geo field in `add_host_metadata` and `add_observer_metadata` processors. {issue}20796[20796] {pull}20811[20811] +- Explicitly detect missing variables in autodiscover configuration, log them at the debug level. {issue}20568[20568] {pull}[] *Auditbeat* diff --git a/libbeat/autodiscover/template/config.go b/libbeat/autodiscover/template/config.go index a34cec10444..dc799afb0c3 100644 --- a/libbeat/autodiscover/template/config.go +++ b/libbeat/autodiscover/template/config.go @@ -18,7 +18,10 @@ package template import ( + "fmt" + "github.com/elastic/go-ucfg" + "github.com/elastic/go-ucfg/parse" "github.com/elastic/beats/v7/libbeat/common" "github.com/elastic/beats/v7/libbeat/common/bus" @@ -123,11 +126,20 @@ func ApplyConfigTemplate(event bus.Event, configs []*common.Config, options ...u if err != nil { logp.Err("Error building config: %v", err) } + opts := []ucfg.Option{ ucfg.PathSep("."), ucfg.Env(vars), ucfg.ResolveEnv, ucfg.VarExp, + + // Catch-all resolve function to log fields not resolved in any other way. + ucfg.Resolve(func(name string) (string, parse.Config, error) { + // Logging here the whole name of the field because it is not included in + // the error reported by Unpack. + logp.Debug("autodiscover", "Configuration includes a field not available in event: %s", name) + return "", parse.Config{}, fmt.Errorf("unavailable field %s", name) + }), } opts = append(opts, options...) @@ -139,9 +151,9 @@ func ApplyConfigTemplate(event bus.Event, configs []*common.Config, options ...u } // Unpack config to process any vars in the template: var unpacked map[string]interface{} - c.Unpack(&unpacked, opts...) + err = c.Unpack(&unpacked, opts...) if err != nil { - logp.Err("Error unpacking config: %v", err) + logp.Debug("autodiscover", "Configuration template cannot be resolved: %v", err) continue } // Repack again: diff --git a/libbeat/autodiscover/template/config_test.go b/libbeat/autodiscover/template/config_test.go index 87e9ef5592a..d1de3370731 100644 --- a/libbeat/autodiscover/template/config_test.go +++ b/libbeat/autodiscover/template/config_test.go @@ -28,9 +28,12 @@ import ( "github.com/elastic/beats/v7/libbeat/common" "github.com/elastic/beats/v7/libbeat/common/bus" "github.com/elastic/beats/v7/libbeat/keystore" + "github.com/elastic/beats/v7/libbeat/logp" ) func TestConfigsMapping(t *testing.T) { + logp.TestingSetup() + config, _ := common.NewConfigFrom(map[string]interface{}{ "correct": "config", }) @@ -111,6 +114,17 @@ func TestConfigsMapping(t *testing.T) { }, expected: []*common.Config{configPorts}, }, + // Missing variable, config is not generated + { + mapping: ` +- config: + - module: something + hosts: ["${not.exists.host}"]`, + event: bus.Event{ + "host": "1.2.3.4", + }, + expected: nil, + }, } for _, test := range tests { From 98fdab673cf06aab932bd2347a46a33a1e48da5a Mon Sep 17 00:00:00 2001 From: Jaime Soriano Pastor Date: Tue, 1 Sep 2020 17:07:37 +0200 Subject: [PATCH 2/5] Fix changelog --- CHANGELOG.next.asciidoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc index 0e65687b229..93c34e9f6a6 100644 --- a/CHANGELOG.next.asciidoc +++ b/CHANGELOG.next.asciidoc @@ -20,7 +20,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d - Ensure dynamic template names are unique for the same field. {pull}18849[18849] - Remove the deprecated `xpack.monitoring.*` settings. Going forward only `monitoring.*` settings may be used. {issue}9424[9424] {pull}18608[18608] - Added `certificate` TLS verification mode to ignore server name mismatch. {issue}12283[12283] {pull}20293[20293] -- Autodiscover doesn't generate any configuration when a variable is missing. Previously it generated an incomplete configuration. {pull}[] +- Autodiscover doesn't generate any configuration when a variable is missing. Previously it generated an incomplete configuration. {pull}20898[20898] *Auditbeat* @@ -168,7 +168,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d - [Metricbeat][Kubernetes] Change cluster_ip field from ip to keyword. {pull}20571[20571] - Rename cloud.provider `az` value to `azure` inside the add_cloud_metadata processor. {pull}20689[20689] - Add missing country_name geo field in `add_host_metadata` and `add_observer_metadata` processors. {issue}20796[20796] {pull}20811[20811] -- Explicitly detect missing variables in autodiscover configuration, log them at the debug level. {issue}20568[20568] {pull}[] +- Explicitly detect missing variables in autodiscover configuration, log them at the debug level. {issue}20568[20568] {pull}20898[20898] *Auditbeat* From 76b79bb853c7efe0ba5b113225cee4dc6d5b5c3f Mon Sep 17 00:00:00 2001 From: Jaime Soriano Pastor Date: Wed, 2 Sep 2020 11:34:41 +0200 Subject: [PATCH 3/5] Take into account resolution of environment variables --- libbeat/autodiscover/template/config.go | 15 ++++++++------- libbeat/autodiscover/template/config_test.go | 17 +++++++++++++++++ 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/libbeat/autodiscover/template/config.go b/libbeat/autodiscover/template/config.go index dc799afb0c3..7beaf1417ac 100644 --- a/libbeat/autodiscover/template/config.go +++ b/libbeat/autodiscover/template/config.go @@ -128,18 +128,19 @@ func ApplyConfigTemplate(event bus.Event, configs []*common.Config, options ...u } opts := []ucfg.Option{ - ucfg.PathSep("."), - ucfg.Env(vars), - ucfg.ResolveEnv, - ucfg.VarExp, - - // Catch-all resolve function to log fields not resolved in any other way. + // Catch-all resolve function to log fields not resolved in any other way, + // it needs to be the first resolver added, so it is executed the last one. ucfg.Resolve(func(name string) (string, parse.Config, error) { // Logging here the whole name of the field because it is not included in // the error reported by Unpack. - logp.Debug("autodiscover", "Configuration includes a field not available in event: %s", name) + logp.Debug("autodiscover", "Configuration includes a field not available in event or environment: %s", name) return "", parse.Config{}, fmt.Errorf("unavailable field %s", name) }), + + ucfg.PathSep("."), + ucfg.Env(vars), + ucfg.VarExp, + ucfg.ResolveEnv, } opts = append(opts, options...) diff --git a/libbeat/autodiscover/template/config_test.go b/libbeat/autodiscover/template/config_test.go index d1de3370731..7964ba24126 100644 --- a/libbeat/autodiscover/template/config_test.go +++ b/libbeat/autodiscover/template/config_test.go @@ -43,6 +43,13 @@ func TestConfigsMapping(t *testing.T) { "hosts": [1]string{"1.2.3.4:8080"}, }) + const envValue = "valuefromenv" + configFromEnv, _ := common.NewConfigFrom(map[string]interface{}{ + "correct": envValue, + }) + + os.Setenv("CONFIGS_MAPPING_TESTENV", envValue) + tests := []struct { mapping string event bus.Event @@ -82,6 +89,16 @@ func TestConfigsMapping(t *testing.T) { }, expected: []*common.Config{config}, }, + // No condition, value from environment + { + mapping: ` +- config: + - correct: ${CONFIGS_MAPPING_TESTENV}`, + event: bus.Event{ + "foo": 3, + }, + expected: []*common.Config{configFromEnv}, + }, // Match config and replace data.host and data.ports. properly { mapping: ` From a83dca6bef7227877c41273f6b0e7b0f67660cc7 Mon Sep 17 00:00:00 2001 From: Jaime Soriano Pastor Date: Wed, 2 Sep 2020 11:45:47 +0200 Subject: [PATCH 4/5] Revert unnecesary change --- libbeat/autodiscover/template/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libbeat/autodiscover/template/config.go b/libbeat/autodiscover/template/config.go index 7beaf1417ac..5781af47ada 100644 --- a/libbeat/autodiscover/template/config.go +++ b/libbeat/autodiscover/template/config.go @@ -139,8 +139,8 @@ func ApplyConfigTemplate(event bus.Event, configs []*common.Config, options ...u ucfg.PathSep("."), ucfg.Env(vars), - ucfg.VarExp, ucfg.ResolveEnv, + ucfg.VarExp, } opts = append(opts, options...) From 9412722cdadbdbea5891a9d00ceff1f3cd697bfa Mon Sep 17 00:00:00 2001 From: Jaime Soriano Pastor Date: Wed, 2 Sep 2020 11:52:53 +0200 Subject: [PATCH 5/5] After returning our own error at the end of resolvers, we don't need to log it additionally --- libbeat/autodiscover/template/config.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/libbeat/autodiscover/template/config.go b/libbeat/autodiscover/template/config.go index 5781af47ada..a1f87d2bcfc 100644 --- a/libbeat/autodiscover/template/config.go +++ b/libbeat/autodiscover/template/config.go @@ -130,11 +130,10 @@ func ApplyConfigTemplate(event bus.Event, configs []*common.Config, options ...u opts := []ucfg.Option{ // Catch-all resolve function to log fields not resolved in any other way, // it needs to be the first resolver added, so it is executed the last one. + // Being the last one, its returned error will be the one returned by `Unpack`, + // this is important to give better feedback in case of failure. ucfg.Resolve(func(name string) (string, parse.Config, error) { - // Logging here the whole name of the field because it is not included in - // the error reported by Unpack. - logp.Debug("autodiscover", "Configuration includes a field not available in event or environment: %s", name) - return "", parse.Config{}, fmt.Errorf("unavailable field %s", name) + return "", parse.Config{}, fmt.Errorf("field '%s' not available in event or environment", name) }), ucfg.PathSep("."),