From 8e8cfdf0fe3b3a9ee9736816146f2f7b556f28d6 Mon Sep 17 00:00:00 2001 From: Tudor Golubenco Date: Fri, 28 Jul 2017 17:17:49 +0300 Subject: [PATCH] Fix mixed up modules configuration (#4772) * 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 b674dc2fef4..10626a4213c 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 c4fb1032547..2f75b95c0e0 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 57dc98b1483..7a20fa158c8 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 7be4c601876..ab3e0d4b75b 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 f8c606d85d9..7aac643c1f9 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 5a3097302b5..fe506be6a79 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"}, }, }, }