Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ADR for osquery extension, runner, and instance refactor #1905

Merged
merged 8 commits into from
Oct 25, 2024

Conversation

RebeccaMahany
Copy link
Contributor

@RebeccaMahany RebeccaMahany commented Oct 23, 2024

Relates to #1827.

Relates to #1936.

(ER diagram docs are here.)

zackattack01
zackattack01 previously approved these changes Oct 23, 2024
Copy link
Contributor

@zackattack01 zackattack01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is really well written and the approach makes sense to me! being somewhat unfamiliar with these pieces I am curious if there were any alternative approaches you had considered? mostly just for my own learning but maybe worth adding if so


The osquery instance will now be responsible for managing extension health -- if the extension exits, the instance will need to take corrective action. (This is potentially a better outcome than before, since now extension shutdown can be remediated without a full launcher restart.)

This decision does preserve a shared JSONRPC client to talk to Kolide SaaS. If we determine this is not optimal, it would be fairly easy to update to a model where we have one JSONRPC client per extension.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any known reasons to share vs break out per extension? or we're just not aware of a reason to duplicate the client

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or we're just not aware of a reason to duplicate the client

Basically this! I am imagining it's the sort of thing that I'll have a stronger opinion on once we get further through implementation. But either way, not hard to change after the fact.

James-Pickett
James-Pickett previously approved these changes Oct 23, 2024
Copy link
Contributor

@James-Pickett James-Pickett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would definitely be an improvement. More logical separation of concerns. Much better to let the osqueryrunner -> instance handle the extension so we don't have restart launcher if it dies.

@RebeccaMahany
Copy link
Contributor Author

@zackattack01 -- the main alternative was to leave the extension where it is, and then either 1) share the extension among all the instances, or 2) create multiple extensions. I added more details in this commit: 915b78b

James-Pickett
James-Pickett previously approved these changes Oct 23, 2024
zackattack01
zackattack01 previously approved these changes Oct 23, 2024
Copy link
Contributor

@zackattack01 zackattack01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great, thank you!

Copy link
Contributor

@directionless directionless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love the diagrams

@RebeccaMahany RebeccaMahany added this pull request to the merge queue Oct 25, 2024
Merged via the queue into kolide:main with commit b2d9b6c Oct 25, 2024
20 of 21 checks passed
@RebeccaMahany RebeccaMahany deleted the becca/osq-refactor-adr branch October 25, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants