Add Central Management blacklisting #9099

exekias · 2018-11-15T13:56:03Z

This change allows to define local blacklist for configurations coming
from Central Management. If a configuration block matches the given
regular expession, it will be ignored:

For example:

management:
  blacklist:
    output: console
    metricbeat.modules.module: k.{18}s

This change allows to define local blacklist for configurations coming from Central Management. If a configuration block matches the given regular expession, it will be ignored: For example: ``` management: blacklist: output: console metricbeat.modules.module: k.{18}s ```

ph · 2018-11-15T14:06:14Z

x-pack/libbeat/management/blacklist.go

+		return fmt.Errorf("wrong type, expect map")
+	}
+
+	var expand func(key string, value interface{})


Not really needed since you are defining it below?

It's needed because this function is recursive, so internal calls need the definition beforehand

ph · 2018-11-15T14:10:34Z

x-pack/libbeat/management/blacklist.go

+	return result
+}
+
+func (c *ConfigBlacklist) blacklisted(blockType string, block *api.ConfigBlock) bool {


maybe rename to isBlacklisted() because its a predicate?

ph · 2018-11-15T14:14:17Z

x-pack/libbeat/management/blacklist.go

+	}
+
+	return result
+}


From what I see, lets say that we receive a config block with a black listed option, we would log an error and still apply the other config blocks?

I think to have least surprise as a user it would be better to refuse to apply all the blocks, my reasoning is if we instead apply blocks in part we have an inconsistent view of the configuration and this would be unexpected?

Maybe I am wrong.

Or maybe we don't have the mechanism yet in place to communicate that problem to the user through the UI?

This is a fair question, I don't know what the user should expect in these cases. So your approach would be to "pause" the beat, and disable all modules & outputs until the configuration is not blacklisted?

We currently have a way to report an error, it refers users to the log file when that happens.

Yeah, not sure either do we want a half working beats or do we want a consistent beat?

No matter what, we also to signal the UI that configs can not be applied due to the blacklist. Just logging the issue locally, makes it somewhat 'silent'.

Also, people could have access to management but don't have physical access to the machine.

@urso I think it could be done in two PR, this one to detect errors and another one to improve the feedback loop of a beats since this would certainly require UI changes to support it.

ph · 2018-11-15T14:16:47Z

x-pack/libbeat/management/blacklist.go

+
+		for _, block := range configs.Blocks {
+			if c.blacklisted(configs.Type, block) {
+				logp.Err("Got a blacklisted configuration, ignoring it")


Log which option was blacklisted?

ph · 2018-11-15T14:19:09Z

x-pack/libbeat/management/blacklist.go

+	return false
+}
+
+func (c *ConfigBlacklist) blockMatches(pattern *regexp.Regexp, segments []string, current *common.Config) bool {


Add is prefix for predicate.

x-pack/libbeat/management/blacklist.go

urso · 2018-11-16T16:07:26Z

x-pack/libbeat/management/blacklist.go

+	}
+
+	return result
+}


No matter what, we also to signal the UI that configs can not be applied due to the blacklist. Just logging the issue locally, makes it somewhat 'silent'.

urso · 2018-11-16T16:12:06Z

x-pack/libbeat/management/blacklist.go

+}
+
+// Filter returns a copy of the given ConfigBlocks with the
+func (c *ConfigBlacklist) Filter(configBlocks api.ConfigBlocks) api.ConfigBlocks {


I'd prefer to keep the ConfigBlock api a as small as possible. Reading the code, I understand that ConfigBlock is be mostly a predicate.

e.g.

type Checker interface { Check(string, api.ConfigBlock) bool // return false if the config block is rejected by the checker } func FilterBlocks(checker Checker, blocks api.ConfigBlocks) api.ConfigBlocks { ... } func (c *ConfigBlacklist) Check(type string, blk api.ConfigBlock) bool { return !c.isBlacklisted(type, blk) }

We could also wrap the Checker to log/collect all config blocks that do not match for reporting back to kibana.

As all functionality is in the management package, do we even need to export the filter function?

Yes, it's better approach (interface)

I would say no, if we encapsulate all the checks (whitelist+blacklist) and we just check if the config is valid that removes the need do any filtering and this would probably simplify things to know what state the beats is currently in.

All config block valid => config applied
Any invalid block => do not apply the config.

I also thing validate should check all the checks and not fails on the first one so we can report all the errors at once in the UI.

+1 on collecting all errors. Use multierror.

Not sure I understand the keep ConfigBlock API small comment, I don't add anything to ConfigBlock here, as isBlacklisted is part of ConfigBlacklist.

Do we agree then on treating any a single blacklisted block as a global error that will stop applying new configs? I can do the changes and introduce multierror

ph · 2018-12-04T15:23:20Z

@exekias Just ping me if you need another look.

exekias · 2018-12-11T01:10:13Z

this should be ready for another look, I added a commit to use multierror and stop applying configs on any blocked config, added a comment here: #9099 (comment)

urso · 2018-12-11T19:15:31Z

x-pack/libbeat/management/blacklist.go

+// ConfigBlacklist takes a ConfigBlocks object and filter it based on the given
+// blacklist settings
+type ConfigBlacklist struct {
+	patterns map[string]*regexp.Regexp


better use match.Matcher here.

I will do the change in a followup PR that will include the changelog.

* Add Central Management blacklisting This change allows to define local blacklist for configurations coming from Central Management. If a configuration block matches the given regular expession, it will be ignored: For example: ``` management: blacklist: output: console metricbeat.modules.module: k.{18}s ``` (cherry picked from commit 267c76e)

exekias added enhancement review libbeat needs_backport PR is waiting to be backported to other branches. labels Nov 15, 2018

exekias requested review from ph and urso November 15, 2018 13:56

ph suggested changes Nov 15, 2018

View reviewed changes

Carlos Pérez-Aradros Herce added 2 commits November 15, 2018 22:29

Rename signatures after comments

8bc8b24

Disable blocklisting in existing tests

e91a844

exekias force-pushed the cm-blacklisting branch from fad7435 to e91a844 Compare November 16, 2018 12:13

urso reviewed Nov 16, 2018

View reviewed changes

ruflin added the Team:Integrations Label for the Integrations team label Nov 27, 2018

Carlos Pérez-Aradros Herce added 2 commits November 28, 2018 12:09

Fix config unpacking

0f802c3

Fix tests

0ac34dc

exekias force-pushed the cm-blacklisting branch from 6165c5b to 0ac34dc Compare November 28, 2018 11:51

Test blacklist ignores bad configs

9254f79

alvarolobato assigned exekias Dec 4, 2018

exekias force-pushed the cm-blacklisting branch 2 times, most recently from 1b70e4e to d92189e Compare December 10, 2018 23:16

Do not apply any settings when a block is blacklisted

7dd370e

exekias force-pushed the cm-blacklisting branch from d92189e to 7dd370e Compare December 10, 2018 23:48

fix test

b3dcca5

urso reviewed Dec 11, 2018

View reviewed changes

urso approved these changes Dec 11, 2018

View reviewed changes

ph approved these changes Dec 11, 2018

View reviewed changes

ph merged commit 267c76e into elastic:master Dec 12, 2018

ph mentioned this pull request Dec 12, 2018

Add changelog and uses match.Matcher interface #9504

Merged

ph mentioned this pull request Dec 12, 2018

Cherry-pick #9099 to 6.x: Add Central Management blacklisting #9514

Merged

ph added v6.6.0 Management and removed needs_backport PR is waiting to be backported to other branches. labels Dec 12, 2018

ph mentioned this pull request Dec 13, 2018

Document configuration blocks that are supported in Central Management #9423

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Central Management blacklisting #9099

Add Central Management blacklisting #9099

exekias commented Nov 15, 2018

ph Nov 15, 2018

exekias Nov 15, 2018

ph Nov 15, 2018

ph Nov 15, 2018 •

edited

Loading

ph Nov 15, 2018

exekias Nov 15, 2018

ph Nov 16, 2018

urso Nov 16, 2018

ph Nov 16, 2018

ph Nov 15, 2018

ph Nov 15, 2018

urso Nov 16, 2018

urso Nov 16, 2018

ph Nov 16, 2018 •

edited

Loading

ph Nov 16, 2018

urso Nov 16, 2018

exekias Nov 28, 2018

ph commented Dec 4, 2018

exekias commented Dec 11, 2018

urso Dec 11, 2018

ph Dec 11, 2018

Add Central Management blacklisting #9099

Add Central Management blacklisting #9099

Conversation

exekias commented Nov 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ph Nov 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ph Nov 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ph commented Dec 4, 2018

exekias commented Dec 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ph Nov 15, 2018 •

edited

Loading

ph Nov 16, 2018 •

edited

Loading