Filebeat: Have filesets disabled unless explicitly configured (elasti…

…c#27526) This changes fileset loading so that only filesets that are explicitly defined in the configuration are enabled. Until now, an enabled module will have all its filesets enabled unless explicitly disabled, which makes for a bad user experience with modules that contain a lot of filesets. Closes elastic#17256
Icedroid · Nov 1, 2021 · 46ab1c1 · 46ab1c1
1 parent 791ba68
commit 46ab1c1
Show file tree

Hide file tree

Showing 7 changed files with 252 additions and 31 deletions.
diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc
@@ -85,6 +85,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
 - Add option for S3 input to work without SQS notification {issue}18205[18205] {pull}27332[27332]
 - Fix Crowdstrike ingest pipeline that was creating flattened `process` fields. {issue}27622[27622] {pull}27623[27623]
 - Rename `log.path` to `log.file.path` in filestream to be consistent with `log` input and ECS. {pull}27761[27761]
+- Only filesets that are explicitly configured will be enabled. {issue}17256[17256] {pull}27526[27526]
 
 *Heartbeat*
 - Remove long deprecated `watch_poll` functionality. {pull}27166[27166]

diff --git a/filebeat/autodiscover/builder/hints/logs.go b/filebeat/autodiscover/builder/hints/logs.go
@@ -201,7 +201,7 @@ func (l *logHints) getFilesets(hints common.MapStr, module string) map[string]*f
 	var configured bool
 	filesets := make(map[string]*filesetConfig)
 
-	moduleFilesets, err := l.registry.ModuleFilesets(module)
+	moduleFilesets, err := l.registry.ModuleAvailableFilesets(module)
 	if err != nil {
 		logp.Err("Error retrieving module filesets: %+v", err)
 		return nil

diff --git a/filebeat/beater/filebeat.go b/filebeat/beater/filebeat.go
@@ -113,6 +113,19 @@ func newBeater(b *beat.Beat, plugins PluginFactory, rawConfig *common.Config) (b
 	}
 	if !moduleRegistry.Empty() {
 		logp.Info("Enabled modules/filesets: %s", moduleRegistry.InfoString())
+		for _, mod := range moduleRegistry.ModuleNames() {
+			if mod == "" {
+				continue
+			}
+			filesets, err := moduleRegistry.ModuleConfiguredFilesets(mod)
+			if err != nil {
+				logp.Err("Failed listing filesets for module %s", mod)
+				continue
+			}
+			if len(filesets) == 0 {
+				logp.Warn("Module %s is enabled but has no enabled filesets", mod)
+			}
+		}
 	}
 
 	moduleInputs, err := moduleRegistry.GetInputConfigs()

diff --git a/filebeat/docs/filebeat-modules-options.asciidoc b/filebeat/docs/filebeat-modules-options.asciidoc
@@ -75,8 +75,12 @@ The following example shows a configuration that runs the `nginx`,`mysql`, and
 ----
 {beatname_lc}.modules:
 - module: nginx
+  access:
+  error:
 - module: mysql
+  slowlog:
 - module: system
+  auth:
 ----
 
 [[advanced-settings]]

diff --git a/filebeat/fileset/modules.go b/filebeat/fileset/modules.go
@@ -69,33 +69,13 @@ func newModuleRegistry(modulesPath string,
 			return nil, fmt.Errorf("error getting filesets for module %s: %v", mcfg.Module, err)
 		}
 
-		for _, filesetName := range moduleFilesets {
-			fcfg, exists := mcfg.Filesets[filesetName]
-			if !exists {
-				fcfg = &FilesetConfig{}
-			}
+		for filesetName, fcfg := range mcfg.Filesets {
 
 			fcfg, err = applyOverrides(fcfg, mcfg.Module, filesetName, overrides)
 			if err != nil {
 				return nil, fmt.Errorf("error applying overrides on fileset %s/%s: %v", mcfg.Module, filesetName, err)
 			}
 
-			if fcfg.Enabled != nil && !(*fcfg.Enabled) {
-				continue
-			}
-
-			fileset, err := New(modulesPath, filesetName, mcfg, fcfg)
-			if err != nil {
-				return nil, err
-			}
-			if err = fileset.Read(beatInfo); err != nil {
-				return nil, fmt.Errorf("error reading fileset %s/%s: %v", mcfg.Module, filesetName, err)
-			}
-			reg.registry[mcfg.Module][filesetName] = fileset
-		}
-
-		// check that no extra filesets are configured
-		for filesetName, fcfg := range mcfg.Filesets {
 			if fcfg.Enabled != nil && !(*fcfg.Enabled) {
 				continue
 			}
@@ -108,6 +88,15 @@ func newModuleRegistry(modulesPath string,
 			if !found {
 				return nil, fmt.Errorf("fileset %s/%s is configured but doesn't exist", mcfg.Module, filesetName)
 			}
+
+			fileset, err := New(modulesPath, filesetName, mcfg, fcfg)
+			if err != nil {
+				return nil, err
+			}
+			if err = fileset.Read(beatInfo); err != nil {
+				return nil, fmt.Errorf("error reading fileset %s/%s: %v", mcfg.Module, filesetName, err)
+			}
+			reg.registry[mcfg.Module][filesetName] = fileset
 		}
 	}
 
@@ -152,9 +141,30 @@ func NewModuleRegistry(moduleConfigs []*common.Config, beatInfo beat.Info, init
 		return nil, err
 	}
 
+	enableFilesetsFromOverrides(mcfgs, modulesOverrides)
 	return newModuleRegistry(modulesPath, mcfgs, modulesOverrides, beatInfo)
 }
 
+// enableFilesetsFromOverrides enables in mcfgs the filesets mentioned in overrides,
+// so that the overridden configuration can be applied.
+func enableFilesetsFromOverrides(mcfgs []*ModuleConfig, overrides *ModuleOverrides) {
+	if overrides == nil {
+		return
+	}
+	for _, mcfg := range mcfgs {
+		if modOvr, ok := (*overrides)[mcfg.Module]; ok {
+			for fset := range modOvr {
+				if _, ok = mcfg.Filesets[fset]; !ok {
+					if mcfg.Filesets == nil {
+						mcfg.Filesets = make(map[string]*FilesetConfig)
+					}
+					mcfg.Filesets[fset] = &FilesetConfig{}
+				}
+			}
+		}
+	}
+}
+
 func mcfgFromConfig(cfg *common.Config) (*ModuleConfig, error) {
 	var mcfg ModuleConfig
 
@@ -171,11 +181,18 @@ func mcfgFromConfig(cfg *common.Config) (*ModuleConfig, error) {
 	}
 
 	mcfg.Filesets = map[string]*FilesetConfig{}
-	for name, filesetConfig := range dict {
+
+	// This calls cfg.GetFields() instead of iterating over `dict` keys
+	// because cfg.Unpack above doesn't return keys that map to a nil value,
+	// but GetFields() returns all keys. We need to observe filesets that
+	// don't contain any configuration (all default values).
+	for _, name := range cfg.GetFields() {
 		if name == "module" || name == "enabled" || name == "path" {
 			continue
 		}
 
+		filesetConfig, _ := dict[name] // Nil config if name is not present.
+
 		tmpCfg, err := common.NewConfigFrom(filesetConfig)
 		if err != nil {
 			return nil, fmt.Errorf("error creating config from fileset %s/%s: %v", mcfg.Module, name, err)
@@ -400,9 +417,19 @@ func (reg *ModuleRegistry) ModuleNames() []string {
 	return modules
 }
 
-// ModuleFilesets return the list of available filesets for the given module
+// ModuleAvailableFilesets return the list of available filesets for the given module
 // it returns an empty list if the module doesn't exist
-func (reg *ModuleRegistry) ModuleFilesets(module string) ([]string, error) {
+func (reg *ModuleRegistry) ModuleAvailableFilesets(module string) ([]string, error) {
 	modulesPath := paths.Resolve(paths.Home, "module")
 	return getModuleFilesets(modulesPath, module)
 }
+
+// ModuleConfiguredFilesets return the list of configured filesets for the given module
+// it returns an empty list if the module doesn't exist
+func (reg *ModuleRegistry) ModuleConfiguredFilesets(module string) (list []string, err error) {
+	filesets, _ := reg.registry[module]
+	for name := range filesets {
+		list = append(list, name)
+	}
+	return
+}
diff --git a/filebeat/fileset/modules_integration_test.go b/filebeat/fileset/modules_integration_test.go
@@ -105,7 +105,13 @@ func TestSetupNginx(t *testing.T) {
 	require.NoError(t, err)
 
 	configs := []*ModuleConfig{
-		{Module: "nginx"},
+		{
+			Module: "nginx",
+			Filesets: map[string]*FilesetConfig{
+				"error":  {},
+				"access": {},
+			},
+		},
 	}
 
 	reg, err := newModuleRegistry(modulesPath, configs, nil, makeTestInfo("5.2.0"))

diff --git a/filebeat/fileset/modules_test.go b/filebeat/fileset/modules_test.go
@@ -45,11 +45,39 @@ func TestNewModuleRegistry(t *testing.T) {
 	modulesPath, err := filepath.Abs("../module")
 	require.NoError(t, err)
 
+	falseVar := false
+
 	configs := []*ModuleConfig{
-		{Module: "nginx"},
-		{Module: "mysql"},
-		{Module: "system"},
-		{Module: "auditd"},
+		{
+			Module: "nginx",
+			Filesets: map[string]*FilesetConfig{
+				"access": {},
+				"error":  {},
+				"ingress_controller": {
+					Enabled: &falseVar,
+				},
+			},
+		},
+		{
+			Module: "mysql",
+			Filesets: map[string]*FilesetConfig{
+				"slowlog": {},
+				"error":   {},
+			},
+		},
+		{
+			Module: "system",
+			Filesets: map[string]*FilesetConfig{
+				"syslog": {},
+				"auth":   {},
+			},
+		},
+		{
+			Module: "auditd",
+			Filesets: map[string]*FilesetConfig{
+				"log": {},
+			},
+		},
 	}
 
 	reg, err := newModuleRegistry(modulesPath, configs, nil, beat.Info{Version: "5.2.0"})
@@ -58,7 +86,7 @@ func TestNewModuleRegistry(t *testing.T) {
 
 	expectedModules := map[string][]string{
 		"auditd": {"log"},
-		"nginx":  {"access", "error", "ingress_controller"},
+		"nginx":  {"access", "error"},
 		"mysql":  {"slowlog", "error"},
 		"system": {"syslog", "auth"},
 	}
@@ -374,6 +402,19 @@ func TestMcfgFromConfig(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "empty fileset (nil)",
+			config: load(t, map[string]interface{}{
+				"module": "nginx",
+				"error":  nil,
+			}),
+			expected: ModuleConfig{
+				Module: "nginx",
+				Filesets: map[string]*FilesetConfig{
+					"error": {},
+				},
+			},
+		},
 	}
 
 	for _, test := range tests {
@@ -451,3 +492,132 @@ func TestInterpretError(t *testing.T) {
 		})
 	}
 }
+
+func TestEnableFilesetsFromOverrides(t *testing.T) {
+	tests := []struct {
+		Name      string
+		Cfg       []*ModuleConfig
+		Overrides *ModuleOverrides
+		Expected  []*ModuleConfig
+	}{
+		{
+			Name: "add fileset",
+			Cfg: []*ModuleConfig{
+				{
+					Module: "foo",
+					Filesets: map[string]*FilesetConfig{
+						"bar": {},
+					},
+				},
+			},
+			Overrides: &ModuleOverrides{
+				"foo": {
+					"baz": nil,
+				},
+			},
+			Expected: []*ModuleConfig{
+				{
+					Module: "foo",
+					Filesets: map[string]*FilesetConfig{
+						"bar": {},
+						"baz": {},
+					},
+				},
+			},
+		},
+		{
+			Name: "defined fileset",
+			Cfg: []*ModuleConfig{
+				{
+					Module: "foo",
+					Filesets: map[string]*FilesetConfig{
+						"bar": {
+							Var: map[string]interface{}{
+								"a": "b",
+							},
+						},
+					},
+				},
+			},
+			Overrides: &ModuleOverrides{
+				"foo": {
+					"bar": nil,
+				},
+			},
+			Expected: []*ModuleConfig{
+				{
+					Module: "foo",
+					Filesets: map[string]*FilesetConfig{
+						"bar": {
+							Var: map[string]interface{}{
+								"a": "b",
+							},
+						},
+					},
+				},
+			},
+		},
+		{
+			Name: "disabled module",
+			Cfg: []*ModuleConfig{
+				{
+					Module: "foo",
+					Filesets: map[string]*FilesetConfig{
+						"bar": {},
+					},
+				},
+			},
+			Overrides: &ModuleOverrides{
+				"other": {
+					"bar": nil,
+				},
+			},
+			Expected: []*ModuleConfig{
+				{
+					Module: "foo",
+					Filesets: map[string]*FilesetConfig{
+						"bar": {},
+					},
+				},
+			},
+		},
+		{
+			Name: "nil overrides",
+			Cfg: []*ModuleConfig{
+				{
+					Module: "foo",
+					Filesets: map[string]*FilesetConfig{
+						"bar": {},
+					},
+				},
+			},
+			Overrides: nil,
+			Expected: []*ModuleConfig{
+				{
+					Module: "foo",
+					Filesets: map[string]*FilesetConfig{
+						"bar": {},
+					},
+				},
+			},
+		},
+		{
+			Name: "no modules",
+			Cfg:  nil,
+			Overrides: &ModuleOverrides{
+				"other": {
+					"bar": nil,
+				},
+			},
+			Expected: nil,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.Name, func(t *testing.T) {
+			enableFilesetsFromOverrides(test.Cfg, test.Overrides)
+			assert.Equal(t, test.Expected, test.Cfg)
+		})
+	}
+
+}