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.

Sign up for GitHub

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

Jump to bottom

feature: writeable data source #3725

Merged
merged 5 commits into from
Dec 19, 2023
Merged

feature: writeable data source #3725

merged 5 commits into from
Dec 19, 2023

Conversation

NDStrahilevitz
Copy link
Collaborator

@NDStrahilevitz NDStrahilevitz commented Dec 3, 2023 •
edited
Loading

1. Explain what the PR does

dec01f2 tests(inst): add writable data source test
c4a7eed feature(grpc): support data source writing
90c10dc chore(go.mod): bump types and api

2. Explain how to test it

Added e2e instrumentation test.

3. Other comments

Resolve #3704

@NDStrahilevitz NDStrahilevitz self-assigned this Dec 3, 2023
josedonizetti
josedonizetti reviewed Dec 5, 2023
View reviewed changes
Copy link
Contributor

@josedonizetti josedonizetti left a comment

Choose a reason for hiding this comment

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

@NDStrahilevitz found one typo, and suggested name suggestions. Is write the only operation required? If I added a DNS that no longer makes sense, what should be the process to clear it from the datasource?

api/v1beta1/datasource.proto Outdated Show resolved Hide resolved
api/v1beta1/datasource.proto Outdated Show resolved Hide resolved
api/v1beta1/datasource.proto Outdated Show resolved Hide resolved
types/detect/detect.go Outdated
@@ -94,3 +94,18 @@ type DataSource interface {

var ErrDataNotFound = errors.New("requested data was not found")
var ErrKeyNotSupported = errors.New("queried key is not supported")
var ErrFailedToUnmarshal = errors.New("given value could not be unmarshaled")

type WriteableDataSource interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps rename to DataSourceWriter? I think Writer instead of Writeable matches golang naming better.

Copy link
Collaborator Author

@NDStrahilevitz NDStrahilevitz Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think if we do that then DataSource should be changed to DataSourceReader too no?
But I don't think so anyway, because a WritableDataSource necessarily has to implement the full DataSource interface, and not just be able to write into one. That's why the name conveys additional behavior and not a functionality (this data source is also writable as opposed to this is the method to write to a particular data source).

@NDStrahilevitz NDStrahilevitz mentioned this pull request Dec 13, 2023
Merged
@NDStrahilevitz
Copy link
Collaborator Author

Types PR in #3759.

@NDStrahilevitz NDStrahilevitz marked this pull request as ready for review December 13, 2023 15:15
@rafaeldtinoco
Copy link
Contributor

Please rebase, merged the types PR. Will review this one asap.

@NDStrahilevitz NDStrahilevitz mentioned this pull request Dec 13, 2023
Merged
@NDStrahilevitz
Copy link
Collaborator Author

NDStrahilevitz commented Dec 13, 2023 •
edited
Loading

@denisgersh FYI if you want to review.
In particular take note of @josedonizetti comment on adding a Delete operation, do you think we need to add this in the current PR, later, or not to bother with it?

@NDStrahilevitz
Copy link
Collaborator Author

TODO: add the instrumentation test in the pr.yaml file.

rafaeldtinoco
rafaeldtinoco reviewed Dec 14, 2023
View reviewed changes
Makefile Outdated Show resolved Hide resolved
rafaeldtinoco
rafaeldtinoco reviewed Dec 14, 2023
View reviewed changes
Copy link
Contributor

@rafaeldtinoco rafaeldtinoco left a comment

Choose a reason for hiding this comment

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

You should add more comments to functions and specific variables in this code.

IMO you should also put the external data source example in the documentation as an example how one could implement it (allowing open source users to benefit from the feature and flagging this in the release).

Left a comment about expanding the grpc client + datasource feed test.

@NDStrahilevitz NDStrahilevitz mentioned this pull request Dec 14, 2023
Merged
@NDStrahilevitz NDStrahilevitz force-pushed the writeable_data_source branch 2 times, most recently from df55f38 to 17ffd77 Compare December 14, 2023 17:27
@rafaeldtinoco rafaeldtinoco self-requested a review December 14, 2023 17:33
rafaeldtinoco
rafaeldtinoco approved these changes Dec 14, 2023
View reviewed changes
Copy link
Contributor

@rafaeldtinoco rafaeldtinoco left a comment

Choose a reason for hiding this comment

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

LGTM. Feel free to merge whenever wanted (after including the docs you mentioned probably).

@rafaeldtinoco
Copy link
Contributor

#3763 has been merged. I believe you need to update types before merging this one now. All good for me (on latest changes).

@NDStrahilevitz NDStrahilevitz merged commit c11ad26 into aquasecurity:main Dec 19, 2023
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josedonizetti josedonizetti josedonizetti left review comments

@rafaeldtinoco rafaeldtinoco rafaeldtinoco approved these changes

Assignees

@NDStrahilevitz NDStrahilevitz

Labels
area/build area/datasources area/ebpf area/flags area/testing area/UX kind/documentation kind/feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add WriteableDataSource interface
3 participants
@NDStrahilevitz @rafaeldtinoco @josedonizetti