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

abstract bbolt from osquery extension code #1652

Merged
merged 8 commits into from
Mar 20, 2024
Merged

Conversation

zackattack01
Copy link
Contributor

Much credit for this PR goes to @James-Pickett - several ideas and some code have been stolen from the original here: #1421

Since that PR some of the original requirements have loosened and much code has shifted, making it easier to just start with a fresh branch here.

Follow up PRs will attempt to rip out all of the BBolt references from knapsack and beyond (wanted to keep the changeset smaller here). The concept here is to add two new interface requirements for the KVStore type to allow us to remove direct bbolt references from the osquery extension.go logic

s.mu.Lock()
defer s.mu.Unlock()

s.items = make(map[string][]byte)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure how this was ever returning deleted keys

func (s *bboltKeyValueStore) ForEach(fn func(k, v []byte) error) error {
if s == nil || s.db == nil {
return NoDbError{}
}

return s.db.Update(func(tx *bbolt.Tx) error {
return s.db.View(func(tx *bbolt.Tx) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

existing oversight, this should be read-only and in a View transaction

@zackattack01 zackattack01 marked this pull request as ready for review March 13, 2024 19:50
@James-Pickett James-Pickett mentioned this pull request Mar 13, 2024
Copy link
Contributor

@RebeccaMahany RebeccaMahany left a comment

Choose a reason for hiding this comment

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

Couple small comments/qs, lgtm overall

func (s *bboltKeyValueStore) Count() int {
if s == nil || s.db == nil {
s.slogger.Log(context.TODO(), slog.LevelError, "unable to count uninitialized bbolt storage db")
return 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Safe to return 0 instead of 0, err in the error cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was on the fence about this, it seems safe for current uses but wondering if we'll regret this for the interface long term. I will update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed!

ee/agent/storage/bbolt/keyvalue_store_bbolt.go Outdated Show resolved Hide resolved
pkg/osquery/extension.go Show resolved Hide resolved
pkg/osquery/extension.go Show resolved Hide resolved
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.

nice

@zackattack01 zackattack01 added this pull request to the merge queue Mar 20, 2024
Merged via the queue into main with commit fac5f38 Mar 20, 2024
29 checks passed
@zackattack01 zackattack01 deleted the zack/remove_bbolt branch March 20, 2024 20:35
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.

3 participants