From b95280ef3843e066f67bc808c7c1e68b1ee4a6f6 Mon Sep 17 00:00:00 2001 From: Tudor Golubenco Date: Mon, 31 Jul 2017 21:26:51 +0300 Subject: [PATCH] Fix mixed up modules configuration (#4772) (#4785) * Fix mixed up modules configuration Fixes #4761. Due to combining pointer and reference passing we ended up with passing the same module pointer to multiple filesets. The pointer was correct during initialization, but wrong during run time. Also added an Info with the enabled modules / filesets. (cherry picked from commit 4ce5360d08cd94d9d982a6cce57a8da1b59cc185) --- CHANGELOG.asciidoc | 1 + filebeat/beater/filebeat.go | 3 + filebeat/fileset/config.go | 2 - filebeat/fileset/modules.go | 34 +++++++--- filebeat/fileset/modules_integration_test.go | 4 +- filebeat/fileset/modules_test.go | 66 +++++++++++--------- 6 files changed, 70 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index b674dc2fef46..10626a4213cb 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -37,6 +37,7 @@ https://github.com/elastic/beats/compare/v5.4.1...master[Check the HEAD diff] - Fix modules default file permissions. {pull}3879[3879] - Properly shut down crawler in case one prospector is misconfigured. {pull}4037[4037] - Fix importing the dashboards when the limit for max open files is too low. {issue}4244[4244] +- Fix issue where the `fileset.module` could have the wrong value. {issue}4761[4761] *Filebeat* - Fix issue that new prospector was not reloaded on conflict {pull}4128[4128] diff --git a/filebeat/beater/filebeat.go b/filebeat/beater/filebeat.go index c4fb10325474..2f75b95c0e0c 100644 --- a/filebeat/beater/filebeat.go +++ b/filebeat/beater/filebeat.go @@ -42,6 +42,9 @@ func New(b *beat.Beat, rawConfig *common.Config) (beat.Beater, error) { if err != nil { return nil, err } + if !moduleRegistry.Empty() { + logp.Info("Enabled modules/filesets: %s", moduleRegistry.InfoString()) + } moduleProspectors, err := moduleRegistry.GetProspectorConfigs() if err != nil { diff --git a/filebeat/fileset/config.go b/filebeat/fileset/config.go index 57dc98b1483d..7a20fa158c81 100644 --- a/filebeat/fileset/config.go +++ b/filebeat/fileset/config.go @@ -15,5 +15,3 @@ type FilesetConfig struct { Var map[string]interface{} `config:"var"` Prospector map[string]interface{} `config:"prospector"` } - -var defaultFilesetConfig = FilesetConfig{} diff --git a/filebeat/fileset/modules.go b/filebeat/fileset/modules.go index 7be4c6018762..ab3e0d4b75b2 100644 --- a/filebeat/fileset/modules.go +++ b/filebeat/fileset/modules.go @@ -22,7 +22,7 @@ type ModuleRegistry struct { // newModuleRegistry reads and loads the configured module into the registry. func newModuleRegistry(modulesPath string, - moduleConfigs []ModuleConfig, + moduleConfigs []*ModuleConfig, overrides *ModuleOverrides, beatVersion string) (*ModuleRegistry, error) { @@ -43,7 +43,7 @@ func newModuleRegistry(modulesPath string, for _, filesetName := range moduleFilesets { fcfg, exists := mcfg.Filesets[filesetName] if !exists { - fcfg = &defaultFilesetConfig + fcfg = &FilesetConfig{} } fcfg, err = applyOverrides(fcfg, mcfg.Module, filesetName, overrides) @@ -55,7 +55,7 @@ func newModuleRegistry(modulesPath string, continue } - fileset, err := New(modulesPath, filesetName, &mcfg, fcfg) + fileset, err := New(modulesPath, filesetName, mcfg, fcfg) if err != nil { return nil, err } @@ -100,13 +100,13 @@ func NewModuleRegistry(moduleConfigs []*common.Config, beatVersion string) (*Mod if err != nil { return nil, err } - mcfgs := []ModuleConfig{} + mcfgs := []*ModuleConfig{} for _, moduleConfig := range moduleConfigs { mcfg, err := mcfgFromConfig(moduleConfig) if err != nil { return nil, fmt.Errorf("Error unpacking module config: %v", err) } - mcfgs = append(mcfgs, *mcfg) + mcfgs = append(mcfgs, mcfg) } mcfgs, err = appendWithoutDuplicates(mcfgs, modulesCLIList) if err != nil { @@ -209,7 +209,7 @@ func applyOverrides(fcfg *FilesetConfig, // appendWithoutDuplicates appends basic module configuration for each module in the // modules list, unless the same module is not already loaded. -func appendWithoutDuplicates(moduleConfigs []ModuleConfig, modules []string) ([]ModuleConfig, error) { +func appendWithoutDuplicates(moduleConfigs []*ModuleConfig, modules []string) ([]*ModuleConfig, error) { if len(modules) == 0 { return moduleConfigs, nil } @@ -226,7 +226,7 @@ func appendWithoutDuplicates(moduleConfigs []ModuleConfig, modules []string) ([] // add the non duplicates to the list for _, module := range modules { if _, exists := modulesMap[module]; !exists { - moduleConfigs = append(moduleConfigs, ModuleConfig{Module: module}) + moduleConfigs = append(moduleConfigs, &ModuleConfig{Module: module}) } } return moduleConfigs, nil @@ -282,6 +282,26 @@ func (reg *ModuleRegistry) LoadPipelines(esClient PipelineLoader) error { return nil } +// InfoString returns the enabled modules and filesets in a single string, ready to +// be shown to the user +func (reg *ModuleRegistry) InfoString() string { + var result string + for module, filesets := range reg.registry { + var filesetNames string + for name, _ := range filesets { + if filesetNames != "" { + filesetNames += ", " + } + filesetNames += name + } + if result != "" { + result += ", " + } + result += fmt.Sprintf("%s (%s)", module, filesetNames) + } + return result +} + // checkAvailableProcessors calls the /_nodes/ingest API and verifies that all processors listed // in the requiredProcessors list are available in Elasticsearch. Returns nil if all required // processors are available. diff --git a/filebeat/fileset/modules_integration_test.go b/filebeat/fileset/modules_integration_test.go index f8c606d85d90..7aac643c1f95 100644 --- a/filebeat/fileset/modules_integration_test.go +++ b/filebeat/fileset/modules_integration_test.go @@ -58,8 +58,8 @@ func TestSetupNginx(t *testing.T) { modulesPath, err := filepath.Abs("../module") assert.NoError(t, err) - configs := []ModuleConfig{ - {Module: "nginx"}, + configs := []*ModuleConfig{ + &ModuleConfig{Module: "nginx"}, } reg, err := newModuleRegistry(modulesPath, configs, nil, "5.2.0") diff --git a/filebeat/fileset/modules_test.go b/filebeat/fileset/modules_test.go index 5a3097302b5e..fe506be6a79a 100644 --- a/filebeat/fileset/modules_test.go +++ b/filebeat/fileset/modules_test.go @@ -25,11 +25,11 @@ func TestNewModuleRegistry(t *testing.T) { modulesPath, err := filepath.Abs("../module") assert.NoError(t, err) - configs := []ModuleConfig{ - {Module: "nginx"}, - {Module: "mysql"}, - {Module: "system"}, - {Module: "auditd"}, + configs := []*ModuleConfig{ + &ModuleConfig{Module: "nginx"}, + &ModuleConfig{Module: "mysql"}, + &ModuleConfig{Module: "system"}, + &ModuleConfig{Module: "auditd"}, } reg, err := newModuleRegistry(modulesPath, configs, nil, "5.2.0") @@ -57,8 +57,16 @@ func TestNewModuleRegistry(t *testing.T) { for module, filesets := range reg.registry { for name, fileset := range filesets { - _, err = fileset.getProspectorConfig() + cfg, err := fileset.getProspectorConfig() assert.NoError(t, err, fmt.Sprintf("module: %s, fileset: %s", module, name)) + + moduleName, err := cfg.String("_module_name", -1) + assert.NoError(t, err) + assert.Equal(t, module, moduleName) + + filesetName, err := cfg.String("_fileset_name", -1) + assert.NoError(t, err) + assert.Equal(t, name, filesetName) } } } @@ -69,8 +77,8 @@ func TestNewModuleRegistryConfig(t *testing.T) { falseVar := false - configs := []ModuleConfig{ - { + configs := []*ModuleConfig{ + &ModuleConfig{ Module: "nginx", Filesets: map[string]*FilesetConfig{ "access": { @@ -83,7 +91,7 @@ func TestNewModuleRegistryConfig(t *testing.T) { }, }, }, - { + &ModuleConfig{ Module: "mysql", Enabled: &falseVar, }, @@ -191,24 +199,24 @@ func TestAppendWithoutDuplicates(t *testing.T) { falseVar := false tests := []struct { name string - configs []ModuleConfig + configs []*ModuleConfig modules []string - expected []ModuleConfig + expected []*ModuleConfig }{ { name: "just modules", - configs: []ModuleConfig{}, + configs: []*ModuleConfig{}, modules: []string{"moduleA", "moduleB", "moduleC"}, - expected: []ModuleConfig{ - {Module: "moduleA"}, - {Module: "moduleB"}, - {Module: "moduleC"}, + expected: []*ModuleConfig{ + &ModuleConfig{Module: "moduleA"}, + &ModuleConfig{Module: "moduleB"}, + &ModuleConfig{Module: "moduleC"}, }, }, { name: "eliminate a duplicate, no override", - configs: []ModuleConfig{ - { + configs: []*ModuleConfig{ + &ModuleConfig{ Module: "moduleB", Filesets: map[string]*FilesetConfig{ "fileset": { @@ -220,8 +228,8 @@ func TestAppendWithoutDuplicates(t *testing.T) { }, }, modules: []string{"moduleA", "moduleB", "moduleC"}, - expected: []ModuleConfig{ - { + expected: []*ModuleConfig{ + &ModuleConfig{ Module: "moduleB", Filesets: map[string]*FilesetConfig{ "fileset": { @@ -231,14 +239,14 @@ func TestAppendWithoutDuplicates(t *testing.T) { }, }, }, - {Module: "moduleA"}, - {Module: "moduleC"}, + &ModuleConfig{Module: "moduleA"}, + &ModuleConfig{Module: "moduleC"}, }, }, { name: "disabled config", - configs: []ModuleConfig{ - { + configs: []*ModuleConfig{ + &ModuleConfig{ Module: "moduleB", Enabled: &falseVar, Filesets: map[string]*FilesetConfig{ @@ -251,8 +259,8 @@ func TestAppendWithoutDuplicates(t *testing.T) { }, }, modules: []string{"moduleA", "moduleB", "moduleC"}, - expected: []ModuleConfig{ - { + expected: []*ModuleConfig{ + &ModuleConfig{ Module: "moduleB", Enabled: &falseVar, Filesets: map[string]*FilesetConfig{ @@ -263,9 +271,9 @@ func TestAppendWithoutDuplicates(t *testing.T) { }, }, }, - {Module: "moduleA"}, - {Module: "moduleB"}, - {Module: "moduleC"}, + &ModuleConfig{Module: "moduleA"}, + &ModuleConfig{Module: "moduleB"}, + &ModuleConfig{Module: "moduleC"}, }, }, }