Support per-task configuration for async host processing configuration by mna · Pull Request #5700 · fleetdm/fleet

mna · 2022-05-11T17:45:17Z

#5536 , specifically this comment on the original PR: #5640 (comment) and the related discussion on slack.

Checklist for submitter

If some of the following don't apply, delete the relevant line.

Changes file added for user-visible changes (in changes/ and/or orbit/changes/).
Ensured that input data is properly validated, SQL injection is prevented (using placeholders for values in statements)
Added/updated tests

mna

Note that I haven't forgotten about the renaming of the async package, it will be done in a separate PR, this already turned out to be more work than I hoped for.

mna · 2022-05-11T17:48:15Z

+
+			// TODO: tried to test command-line flags too by using cmd.SetArgs to
+			// test-case values, but that didn't seem to work, not sure how it can
+			// be done in our particlar setup.


Haven't found a way to do this but I've tested the flags manually. Would be a nice-to-have if we find a way to support this eventually.

chiiph · 2022-05-11T19:27:24Z

 		"(DEPRECATED: Use filesystem.enable_log_rotation) Enable automatic rotation for osquery log files")
 	man.addConfigInt("osquery.max_jitter_percent", 10,
 		"Maximum percentage of the interval to add as jitter")
-	man.addConfigBool("osquery.enable_async_host_processing", false,


So this kind of change is backwards compatible because it only changes how the value is parsed, but false/0/no would parse the same, right?

Yeah, I tested with various yaml bool encodings (env vars and flags are strings to begin with anyway, so that's less of a concern although tested too) to make sure that they were all still parsed properly.

roperzh

LGTM. I really like when a factory function makes the code way more readable

roperzh · 2022-05-13T13:56:54Z

-	man.addConfigDuration("osquery.async_host_collect_lock_timeout", 1*time.Minute,
-		"Timeout of the exclusive lock held during async host collection")
+	man.addConfigString("osquery.async_host_collect_lock_timeout", (1 * time.Minute).String(),
+		"Timeout of the exclusive lock held during async host collection (i.e. 30s or set per task, e.g. 'label_membership=10s&policy_membership=1m'")


nit: should this be 1min to be consistent with the default?

Suggested change

"Timeout of the exclusive lock held during async host collection (i.e. 30s or set per task, e.g. 'label_membership=10s&policy_membership=1m'")

"Timeout of the exclusive lock held during async host collection (i.e. 1 minute or set per task, e.g. 'label_membership=10s&policy_membership=1m'")

That was meant to be an example of a literal value that can be used for this option, 30s or label_membership=10s&policy_membership=1m. The default value is printed out separately when running fleet --help. I'll add the missing quotes around 30s, that may help indicate that this is the actual value to set.

roperzh · 2022-05-13T14:23:21Z

+	datastore   fleet.Datastore
+	pool        fleet.RedisPool
+	clock       clock.Clock
+	taskConfigs map[string]config.AsyncProcessingConfig


thinking out loud, not even sure if it's a good idea: I wonder if instead of a raw string we should declare our own custom type (which just alias to string) to better communicate that there's a well defined and constant set of tasks which can be configured.

kind of the classic:

type TaskName string const ( AsyncTaskLabelMembership TaskName = "label_membership" AsyncTaskPolicyMembership TaskName = "policy_membership" AsyncTaskHostLastSeen TaskName = "host_last_seen" )

Yeah there's not too much value type-safety-wise because you can still pass a string constant and it will accept it fine, but I guess it's true that it communicates that there's some set defined somewhere and new ones should be added there.

indeed, it might be a good idea just to communicate intent, but I don't think it's super important!

offtopic but tangentially related I was reading this blog post the other day that talks about a way to enforce type safety in a simple(ish) way:

type FlagID struct { name string } func (f FlagID) String() { return f.name } var ( FooBar = FlagID{ “FooBar” } FizzBuzz = FlagID{ “FizzBuzz” } )

Yeah I saw that recently on HN I think? That's a clever approach.

roperzh · 2022-05-13T14:26:33Z

@@ -0,0 +1 @@
+* Support an extended configuration syntax to configure asynchronous host processing task on a per-task basis.


niiiiiiit:

Suggested change

* Support an extended configuration syntax to configure asynchronous host processing task on a per-task basis.

* Support an extended syntax to configure asynchronous host processing task on a per-task basis.

lucasmrod · 2022-05-13T20:03:45Z


 Note that currently, if both the failing policies webhook *and* this `osquery.enable_async_host_processing` option are set, some failing policies webhooks could be missing (some transitions from succeeding to failing or vice-versa could happen without triggering a webhook request).

+It can be set to a single boolean value ("true" or "false"), which controls all async host processing tasks, or it can be set for specific async tasks using a syntax similar to an URL query string or parameters in a Data Source Name (DSN) string, e.g. "label_membership=true&policy_membership=true". The supported async task names are:


Let's clarify that when using the "URL query", if the field is not set, e.g. host_last_seen in the case of label_membership=true&policy_membership=true, then it will be set the default value.

…g configuration # Editor pass for: - #5700

Desmi-Dizney · 2022-05-19T16:11:57Z

Editor pass completed on:

Editor pass - Support per-task configuration for async host processing configuration #5810

…g configuration (#5810) * Editor pass - Support per-task configuration for async host processing configuration # Editor pass for: - #5700 * Update Configuration.md * Update config.go

mna added 7 commits May 10, 2022 16:59

Start refactoring config to support mini-language

b1ffb51

Change creation of task, configuration per task, adjust tests

dbfac28

Add config testing with per-task options and yaml/env var configs

da06a3e

Document the new supported format for async host processing config

169cf27

Add changes file

7ecd11c

Fix bad async task name used

51fc0aa

Formatting

b4ab5b5

mna commented May 11, 2022

View reviewed changes

mna marked this pull request as ready for review May 11, 2022 18:18

mna requested a review from a team as a code owner May 11, 2022 18:18

chiiph reviewed May 11, 2022

View reviewed changes

roperzh previously approved these changes May 13, 2022

View reviewed changes

lucasmrod reviewed May 13, 2022

View reviewed changes

Comment thread server/config/config.go

lucasmrod reviewed May 13, 2022

View reviewed changes

lucasmrod previously approved these changes May 13, 2022

View reviewed changes

mna added 2 commits May 16, 2022 09:08

Update docs and comments based on pr review

7257c03

Use specific type for async task names

d059e43

mna dismissed stale reviews from lucasmrod and roperzh via d059e43 May 16, 2022 13:20

roperzh approved these changes May 16, 2022

View reviewed changes

mna merged commit e6b90ca into fleetdm:main May 16, 2022

Desmi-Dizney added a commit that referenced this pull request May 19, 2022

Editor pass - Support per-task configuration for async host processin…

da3cdbb

…g configuration # Editor pass for: - #5700

Desmi-Dizney mentioned this pull request May 19, 2022

Editor pass - Support per-task configuration for async host processing configuration #5810

Merged

	"Timeout of the exclusive lock held during async host collection (i.e. 30s or set per task, e.g. 'label_membership=10s&policy_membership=1m'")
	"Timeout of the exclusive lock held during async host collection (i.e. 1 minute or set per task, e.g. 'label_membership=10s&policy_membership=1m'")

		@@ -0,0 +1 @@
		* Support an extended configuration syntax to configure asynchronous host processing task on a per-task basis.

	* Support an extended configuration syntax to configure asynchronous host processing task on a per-task basis.
	* Support an extended syntax to configure asynchronous host processing task on a per-task basis.


		Note that currently, if both the failing policies webhook and this `osquery.enable_async_host_processing` option are set, some failing policies webhooks could be missing (some transitions from succeeding to failing or vice-versa could happen without triggering a webhook request).

		It can be set to a single boolean value ("true" or "false"), which controls all async host processing tasks, or it can be set for specific async tasks using a syntax similar to an URL query string or parameters in a Data Source Name (DSN) string, e.g. "label_membership=true&policy_membership=true". The supported async task names are:

Conversation

mna commented May 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist for submitter

Uh oh!

mna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roperzh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Desmi-Dizney commented May 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mna commented May 11, 2022 •

edited

Loading