From 28927615e9208c3e642171ec49b570ec24faae7c Mon Sep 17 00:00:00 2001 From: Tudor Golubenco Date: Wed, 2 Aug 2017 15:32:32 +0300 Subject: [PATCH] Fix mixed up modules configuration (#4772) (#4784) * 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 | 2 + 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, 71 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index d70c36b5c096..73254bcf7827 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -43,6 +43,8 @@ https://github.com/elastic/beats/compare/v6.0.0-beta1...master[Check the HEAD di *Filebeat* +- Fix issue where the `fileset.module` could have the wrong value. {issue}4761[4761] + *Heartbeat* *Metricbeat* diff --git a/filebeat/beater/filebeat.go b/filebeat/beater/filebeat.go index 2b47adcc1aca..b0619722e4d2 100644 --- a/filebeat/beater/filebeat.go +++ b/filebeat/beater/filebeat.go @@ -56,6 +56,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 d9c364efb8de..398997df139e 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 @@ -285,6 +285,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 02c7d12f14ec..a0ca7d006881 100644 --- a/filebeat/fileset/modules_integration_test.go +++ b/filebeat/fileset/modules_integration_test.go @@ -68,8 +68,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 f5a6e83b68fb..da4e34ec3c03 100644 --- a/filebeat/fileset/modules_test.go +++ b/filebeat/fileset/modules_test.go @@ -26,11 +26,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") @@ -58,8 +58,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) } } } @@ -70,8 +78,8 @@ func TestNewModuleRegistryConfig(t *testing.T) { falseVar := false - configs := []ModuleConfig{ - { + configs := []*ModuleConfig{ + &ModuleConfig{ Module: "nginx", Filesets: map[string]*FilesetConfig{ "access": { @@ -84,7 +92,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"}, }, }, }