Introducing Flags interface, with implementation for control server related flags #1114

seejdev · 2023-03-28T15:58:59Z

Related to #1086, and #1083

Motivation for these changes:

We have a number of launcher flags controlling various behaviors, specified through different means, and generally accessed and modified in non-standard and inconsistent ways. This PR lays the foundation for streamlining:

Hooking into our pervasive Knapsack to enable disparate areas of the code base to utilize common flag functionality easily and with low developer overhead.
How flags are defined, their defaults, their available command line options, and how they are stored in key-value stores.
What flag values take precedence over others.
Sanitizing flag values to prevent unreasonable values being used.
Enabling temporary overrides for flags in a generic fashion, avoiding custom one-off code written to accomplish short-lived modifications to flags.

What is in this PR:

The Flags interface: provides an easy-to-use and readable API for setting/getting launcher flags. It's been added to the Knapsack struct, so anywhere Knapsack is passed to, you can access Flags just as easily.
Implementations of default flag values, and command line options which can take precedence over defaults.
Implementation of stored flag values, backed by a key-value store. This is principally designed for control server to push updates of launcher flags, and these can take precedence over defaults, and command line options.
Implementation of flag value overrides, where a flag can be temporarily overridden for a period of time with a special-case value.
Sanitization of integer flag values to constrain values to sensible limits.
Re-working of the types.Updater interface to better separate JSON parsing from the logic of bulk-replacing the contents of the agent_flags key-value store. This was necessary to enable clients of the interface to have better control and knowledge of what keys are being changed.
Implementation of a flag change notification framework, where clients can register as observers of various FlagKey and receive notifications when they change.
Re-working of the control server request interval acceleration feature to tie into the Flags implementation, notification framework, and common temporary override functionality.
Using all of the above to re-implement control server flags: DesktopEnabled, DebugServerData, ForceControlSubsystems, ControlServerURL, ControlRequestInterval, DisableControlTLS, InsecureControlTLS

James-Pickett

I like the general shape of this. Cool to see generics in action. Look forward to more tests.

directionless

There's a lot of meat here. But it feels very hard to understand.

pkg/agent/flags/flag_controller.go

ee/desktop/runner/runner.go

pkg/agent/flags/flag_values_constraints.go

pkg/agent/flags/flag_controller.go

docs/architecture/knapsack.md

pkg/agent/flags/flag_controller.go

pkg/agent/flags/flag_values_defaults.go

cmd/launcher/launcher.go

pkg/agent/flags/flag_value_sanitizer.go

pkg/agent/flags/flag_values_stored.go

Co-authored-by: seph <seph@kolide.co>

James-Pickett

... still digesting

docs/architecture/flags.md

pkg/agent/flags/flag_controller.go

James-Pickett · 2023-04-10T20:02:54Z

pkg/agent/knapsack/knapsack.go

+	return k.flags.DebugServerData()
+}
+
+func (k *knapsack) SetForceControlSubsystems(force bool) error {


The above funcs to retrieve KVStores are suffixed with Store should we consider suffixing the flags with Flag?

I could do that if folks think it's more readable (or, conversely remove the Store suffix) from the ones above. Always a balance between readability and conciseness.

pkg/agent/types/keyvalue_store.go

ee/desktop/runner/runner.go

pkg/agent/flags/flag_controller.go

pkg/agent/flags/flag_value_duration.go

directionless

There's a lot here, so I'm not confident I absorbed it all. But this looks like it's a huge improvement. I say we merge, and iterate based on what we learn.

Caveat -- don't merge until we cut 1.0.7, since I'm hoping we can come up with that tomorrow

directionless · 2023-04-12T19:43:00Z

pkg/agent/knapsack/knapsack.go

 	return k.getKVStore(storage.ServerProvidedDataStore)
 }
+
+func (k *knapsack) RegisterChangeObserver(observer types.FlagsChangeObserver, flagKeys ...keys.FlagKey) {


I'm not totally sold on this -- I think exposing keys.FlagKey feels wrong, it's abstraction breaking in a way. But I think it's fine to keep it in this PR

directionless · 2023-04-12T22:44:18Z

pkg/agent/flags/flag_controller.go

@@ -0,0 +1,189 @@
+package flags


I wonder if we'll want to keep this, vs absorbing it into knapsack. I guess we'll see!

directionless · 2023-04-12T22:45:47Z

pkg/agent/flags/flag_controller.go

+	agentFlagsStore        types.KVStore
+	overrideMutex          sync.RWMutex
+	controlRequestOverride FlagValueOverride
+	observers              map[types.FlagsChangeObserver][]keys.FlagKey


Does observers need a mutex?

Doesn't hurt to add one - in practice I think most observers will register early and wait for notifications, so modifications to the map would be rare later on. But better to be safe.

directionless · 2023-04-12T22:48:02Z

pkg/agent/flags/flag_controller.go

+// getControlServerValue looks for a control-server-provided value for the key and returns it.
+// If a control server value is not found, nil is returned.
+func (fc *FlagController) getControlServerValue(key keys.FlagKey) []byte {
+	value, err := fc.agentFlagsStore.Get([]byte(key))


The comment and name is control server, but the underlying store is simply agentFlagsStore. This is a bit confusing, given the next set function, which uses the same store, but does not use the control server name

Yeah I can clean up the language inconsistency here.

directionless · 2023-04-12T22:51:05Z

pkg/agent/flags/flag_controller.go

+	// Extract just the keys from the key-value pairs
+	updatedKeys := make([]string, len(kvPairs))


This is fine, but FYI there's now an x/exp/maps. (no idea how the generics work there, or if it would need casting, as said, this is fine)

directionless · 2023-04-13T01:21:05Z

pkg/agent/flags/flag_value_string.go

+	}
+}
+
+func WithSanitizer(sanitizer func(value string) string) stringFlagValueOption {


Makes sense. Though I wonder if we should write a WithEnum as a precanned sanitizer. Eh, wait till we need it...

directionless · 2023-04-13T01:22:12Z

pkg/agent/flags/keys/keys.go

+
+// When adding a new FlagKey:
+// 1. Define the FlagKey identifier, and the string key value it corresponds to, in the block below
+// 2. Add a getter and setter to the Flags interface (flags.go)


is this still correct?

directionless · 2023-04-13T01:27:32Z

cmd/launcher/launcher.go

 			desktopRunner.WithLogger(logger),
 			desktopRunner.WithUpdateInterval(time.Second*5),
 			desktopRunner.WithMenuRefreshInterval(time.Minute*15),
 			desktopRunner.WithHostname(opts.KolideServerURL),
 			desktopRunner.WithAuthToken(ulid.New()),
 			desktopRunner.WithUsersFilesRoot(rootDirectory),
-			desktopRunner.WithProcessSpawningEnabled(desktopProcessSpawningEnabled),
-			desktopRunner.WithGetter(k.AgentFlagsStore()),
+			desktopRunner.WithProcessSpawningEnabled(k.DesktopEnabled()),


I think, long term, we should replace things like desktopRunner.WithProcessSpawningEnabled(k.DesktopEnabled()) with having the underlying thing just call k.DesktopEnabled() directly.

We don't expect there to be other consumers of desktopRunner and the functional arguments exist mostly as a style to manage the developer experience around them. I think knapsack replaces them.

Yeah that's a good point, I'll add this to a list of cleanup items. I'll check and see if there are other option style functions that can be eliminated as well.

Yeah, it's all the long arc of using knapsack.

directionless · 2023-04-13T01:29:20Z

docs/architecture/flags.md

+
+## Retrieving Flags
+
+```mermaid


This diagram is different than the implementation. It's semantically the same, but the implementation calculates each value, and then adds the next overlay. This diagram implies early returns.

You're right, I'll make this more accurate.

directionless · 2023-04-13T01:34:00Z

pkg/agent/flags/flag_controller_option.go

+type Option func(*FlagController)
+
+// WithStore sets the key/value store for control data
+func WithCmdLineOpts(cmdLineOpts *launcher.Options) Option {


Is this basically required? Do you think we'll have more options? (eg: should we just add it to the method signature)

CJ Barrett added 20 commits March 24, 2023 17:55

Introducing Flags, and implementation for control service stuff

9349480

Tests actually work

6a04980

Working with updates

a523e51

Swapped flags and observers map

727a3ed

Sanitizers and constraints done

00c3730

Doc start

aea138c

Test fixes

f27803b

Consistent naming

eae0de5

Reorg

85535de

Cleanup

5d23d0e

Removed key map

953667c

more docs

f04193b

Begininning test

360e342

Override WIP

7dffd7d

Override working

15dc646

Got rid of generics in flagValues

8d34074

Test fixes

fca3f72

Cleanup

15f7377

Linter fixes

ea08a6b

Fixed request id test

ecb2ff1

James-Pickett reviewed Mar 28, 2023

View reviewed changes

directionless reviewed Mar 28, 2023

View reviewed changes

CJ Barrett added 3 commits March 28, 2023 17:24

Mocks

0ada08f

Fixed mutex locking

380a433

Lots of tiny fixes, as tests reveal issues.

cea2f3e

RebeccaMahany reviewed Mar 29, 2023

View reviewed changes

CJ Barrett and others added 4 commits March 29, 2023 11:43

More test fixes

a314586

Update pkg/agent/flags/flag_values_constraints.go

01c4438

Co-authored-by: seph <seph@kolide.co>

Code review feedback

b21266e

Merge branch 'main' into flags

3bb303b

CJ Barrett added 15 commits April 4, 2023 10:25

Just a small change where I MADE EVERYTHING DIFFERENT :)

d8444e8

Test fixes

aabbd76

Override fixes

b38dc21

Duration test

a197ac4

Tests

649f1ac

Fixed some things, and tests

f7d5c39

Moar fixes

91bf2c9

Merge branch 'main' into flags

d82a1f8

More test fixes

2369150

Mutex locking is important

44c80ab

Removed unneeded file

5e746db

Docs update

b0c80d6

Fixing data race

cd550a3

Merge branch 'main' into flags

7bfe617

Some comments cleanup

baa7529

James-Pickett reviewed Apr 10, 2023

View reviewed changes

RebeccaMahany previously approved these changes Apr 10, 2023

View reviewed changes

ee/desktop/runner/runner.go Show resolved Hide resolved

pkg/agent/flags/flag_controller.go Outdated Show resolved Hide resolved

pkg/agent/flags/flag_value_duration.go Show resolved Hide resolved

CJ Barrett added 2 commits April 10, 2023 22:35

More comments cleaning up

0ae389b

Converted variadic pairs to map

29bdb10

seejdev dismissed RebeccaMahany’s stale review via 29bdb10 April 11, 2023 03:16

CJ Barrett added 2 commits April 10, 2023 23:27

Dead code

ea93a9f

Merge branch 'main' into flags

b0c6724

RebeccaMahany approved these changes Apr 12, 2023

View reviewed changes

James-Pickett approved these changes Apr 12, 2023

View reviewed changes

directionless approved these changes Apr 13, 2023

View reviewed changes

Merge branch 'main' into flags

aca3ab1

seejdev merged commit 2d3108a into kolide:main Apr 17, 2023

seejdev mentioned this pull request Apr 18, 2023

Moving remaining launcher flags to Knapsack #1138

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing Flags interface, with implementation for control server related flags #1114

Introducing Flags interface, with implementation for control server related flags #1114

seejdev commented Mar 28, 2023

James-Pickett left a comment

directionless left a comment

James-Pickett left a comment

James-Pickett Apr 10, 2023

seejdev Apr 12, 2023

directionless left a comment

directionless Apr 12, 2023

directionless Apr 12, 2023

directionless Apr 12, 2023

seejdev Apr 13, 2023

directionless Apr 12, 2023

seejdev Apr 13, 2023

directionless Apr 12, 2023

directionless Apr 13, 2023

directionless Apr 13, 2023

directionless Apr 13, 2023

seejdev Apr 13, 2023

directionless Apr 17, 2023

directionless Apr 13, 2023

seejdev Apr 13, 2023

directionless Apr 13, 2023

		// Extract just the keys from the key-value pairs
		updatedKeys := make([]string, len(kvPairs))

Introducing Flags interface, with implementation for control server related flags #1114

Introducing Flags interface, with implementation for control server related flags #1114

Conversation

seejdev commented Mar 28, 2023

Motivation for these changes:

What is in this PR:

James-Pickett left a comment

Choose a reason for hiding this comment

directionless left a comment

Choose a reason for hiding this comment

James-Pickett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

directionless left a 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

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