Derive conflict resolution #561

colin-grapl · 2021-01-25T16:16:01Z

Which issue does this PR correspond to?

#147
#148

What changes does this PR make to Grapl? Why?

Removes the underlying protobuf schemas for node types such as 'Process' and 'File', and instead leverages three distinct schemas - NodeDescription, IdentifiedNode, MergedNode.
Replaces 'string', 'int', and 'uint' with conflict resolvable types. The logic to enforce that resolution does not exist and will be implemented in a separate (and much smaller!) PR.

Specifically, while this diff appears very large, there are really just three places you need to worry about:

src/rust/derive-dynamic-node/src/lib.rs
This is where the conflict resolution macro changes occur, allowing us to specify the resolution mechanism for a property.
src/proto/graplinc/grapl/api/graph/v1beta1/types.proto
This is where the new proto structures are defined. The major changes are that we now have distinct node/graph types based on the state of the processed data (described, identified, merged) and, on top of that, the protobuf types to express the conflict resolution.

I've also documented most of the protobuf.

src/rust/graph-descriptions/src/lib.rs
This is where the impl of these new protobuf types exist. The previously defined File, Process, etc are now in the endpoint-plugin.

All other changes are just to support this - there's a lot of diff noise involved, such as deleting tons of files that are no longer necessary, because the logic for nodes is now condensed.

The other churn is in the graph-merger, where I've spent some time refactoring the upsert logic, separating structures out into their own files. I've also added integration tests - 414 lines of additional integration tests are now in the graph-merger, as well as code to support those tests.

If you look at the git stats you'll find that while 104 files have been touched, the vast majority of those are just changing things like Graph::new(0) to GraphDescription;:new(), or that sort of thing. Also, apparently there have been some formatting changes, so that caused churn as well :(

How were these changes tested?

I've tested locally and I've added lots of integration tests.

…d port sysmon

docker-compose.

lgtm-com · 2021-03-02T23:31:16Z

This pull request introduces 2 alerts and fixes 2 when merging b2f85ab into a045aff - view on LGTM.com

new alerts:

1 for Unused import
1 for Unused variable, import, function or class

fixed alerts:

1 for Unused import
1 for Unused variable, import, function or class

src/rust/generators/osquery-subgraph-generator/src/tests/process.rs

d0nut-grapl

Seems mostly the same from when we went over it (loving the new tests you added in btw).

I'd love for another pair of eyes to go over this btw just because it's so huge and I'm biased (saw once before :P )

d0nut-grapl · 2021-03-03T01:29:47Z

src/rust/generators/sysmon-subgraph-generator/src/models/process/create.rs

@@ -82,7 +77,6 @@ pub fn generate_process_create_subgraph(
    );

    graph.add_edge("children", parent.clone_node_key(), child.clone_node_key());
-    graph.add_edge("parent", child.clone_node_key(), parent.clone_node_key());


@colin-grapl is there a reason this was deleted?

It's redundant - there's no need to specify an edge and a reverse edge, one implies the other, and that's handled downstream.

lgtm-com · 2021-03-03T19:49:53Z

This pull request introduces 2 alerts and fixes 2 when merging c65a449 into ce9016f - view on LGTM.com

new alerts:

1 for Unused import
1 for Unused variable, import, function or class

fixed alerts:

1 for Unused import
1 for Unused variable, import, function or class

christophermaier

Just a few minor comments... still not as familiar with this code as I feel I would need to be to be more constructive.

christophermaier · 2021-03-03T20:14:28Z

src/rust/derive-dynamic-node/src/lib.rs

+            match attr {
+                IMMUTABLE => resolution = Some(attr.to_string()),
+                INCREMENT => resolution = Some(attr.to_string()),
+                DECREMENT => resolution = Some(attr.to_string()),


You could probably just use one match arm on IMMUTABLE | INCREMENT | DECREMENT.

src/rust/derive-dynamic-node/src/lib.rs

src/proto/graplinc/grapl/api/graph/v1beta1/types.proto

inickles-grapl

I have questions on a couple items.

src/rust/Dockerfile

.gitignore

src/rust/endpoint-plugin/src/graph.rs

src/rust/endpoint-plugin/src/lib.rs

src/rust/generators/sysmon-subgraph-generator/src/models/process/create.rs

src/rust/grapl-service/src/serialization/zstd_proto_graph.rs

lgtm-com · 2021-03-03T21:17:51Z

This pull request introduces 2 alerts and fixes 2 when merging 830b556 into eb80c50 - view on LGTM.com

new alerts:

1 for Unused import
1 for Unused variable, import, function or class

fixed alerts:

1 for Unused import
1 for Unused variable, import, function or class

lgtm-com · 2021-03-03T22:53:54Z

This pull request introduces 1 alert and fixes 2 when merging 07501c4 into a47b9e9 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

fixed alerts:

1 for Unused import
1 for Unused variable, import, function or class

lgtm-com · 2021-03-04T00:43:06Z

This pull request introduces 1 alert and fixes 2 when merging 85439f0 into a47b9e9 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

fixed alerts:

1 for Unused import
1 for Unused variable, import, function or class

lgtm-com · 2021-03-04T01:31:29Z

This pull request introduces 1 alert and fixes 2 when merging 7e9891f into a47b9e9 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

fixed alerts:

1 for Unused import
1 for Unused variable, import, function or class

The `Edge#edgeName` field was renamed in #561 so we no longer need to ignore any lints. See grapl-security/issue-tracker#237 Signed-off-by: Christopher Maier <chris@graplsecurity.com>

colin-grapl and others added 30 commits December 18, 2020 17:25

Fix issues around consumer policy

2a020b3

Merge branch 'staging' of github.com:grapl-security/grapl into staging

a46d3e4

Merge branch 'staging' of github.com:grapl-security/grapl into staging

8072ae6

Improve costs by changing resource usage

1ec5bc3

Start moving rust to new executor

f652de1

Add FargateService

3d08a44

Create a FargateService construct

a1bf2c0

Port the node-identifier

0263c40

Update graph-merger to use Fargate

32c6d9f

Port analyzer-dispatcher over to Fargate

52b136c

Use the Dockerfile in /src/rust/Dockerfile to keep images smaller

b321fa8

Add sqs-service with helpers

70a82ff

Start moving to env-based construction

bb3f163

Convert more env vars

6d1a8bd

Rebuild

3de459c

Port services over to grapl-service

525110c

Add more config etc

a3313e8

Add keep_alive for sqs messages

4ac3218

Fix minio creds

fc9f646

Remove unused imports

f303427

fmt

5ebe2e8

Remove unused imports

126c801

Fix retry node-identifier retry queue. Rebuild graph-generator-lib an…

1d35be9

…d port sysmon

More monitoring, caching

c6bde9a

Cleanup

5fd0bd1

Revert docker-compose.yml

a99a390

Concurrent build with docker buildx bake. Concurrent test with

1ae5f7e

docker-compose.

Merge branch 'staging' into make-n-bake

1285906

add yarn.lock to engagement-view-deps stage.

b5e71d3

Uncomment node-identifier-retry-handler and clean up some commented code

bf85726

colin-grapl added 5 commits March 2, 2021 14:41

Run black

08428f2

Fix some typos

e1cf64b

More typos

acc2ab4

Use dgraph_query_lib from cratesio

5257041

Fix typos

b2f85ab

d0nut-grapl reviewed Mar 3, 2021

View reviewed changes

src/rust/generators/osquery-subgraph-generator/src/tests/process.rs Outdated Show resolved Hide resolved

d0nut-grapl reviewed Mar 3, 2021

View reviewed changes

colin-grapl added 2 commits March 3, 2021 11:25

Typo

cd8141c

Regen lockfile

c65a449

christophermaier reviewed Mar 3, 2021

View reviewed changes

colin-grapl added 2 commits March 3, 2021 12:44

Use version 2 of package-lock.json

0775f51

Use package-lock.json

830b556

inickles-grapl suggested changes Mar 3, 2021

View reviewed changes

colin-grapl added 2 commits March 3, 2021 14:03

Comment PRs and some cleanup

dabe844

Reformat and fix python typecheck

07501c4

inickles-grapl approved these changes Mar 3, 2021

View reviewed changes

colin-grapl added 2 commits March 3, 2021 16:07

Start handling clippy

1aa45ec

Satisfy clippy

85439f0

colin-grapl added 2 commits March 3, 2021 16:44

Remove unused variables in some tests

f6ecf3a

Reformat

7e9891f

colin-grapl merged commit 3608268 into staging Mar 4, 2021

colin-grapl deleted the DeriveConflictResolution branch March 4, 2021 02:02

christophermaier mentioned this pull request Mar 4, 2021

Remove allow_comment_ignores from buf.yaml #656

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Derive conflict resolution #561

Derive conflict resolution #561

colin-grapl commented Jan 25, 2021 •

edited

Loading

lgtm-com bot commented Mar 2, 2021

d0nut-grapl left a comment

d0nut-grapl Mar 3, 2021

colin-grapl Mar 3, 2021

lgtm-com bot commented Mar 3, 2021

christophermaier left a comment

christophermaier Mar 3, 2021

inickles-grapl left a comment

lgtm-com bot commented Mar 3, 2021

lgtm-com bot commented Mar 3, 2021

lgtm-com bot commented Mar 4, 2021

lgtm-com bot commented Mar 4, 2021

Derive conflict resolution #561

Derive conflict resolution #561

Conversation

colin-grapl commented Jan 25, 2021 • edited Loading

Which issue does this PR correspond to?

What changes does this PR make to Grapl? Why?

How were these changes tested?

lgtm-com bot commented Mar 2, 2021

d0nut-grapl left a comment

Choose a reason for hiding this comment

d0nut-grapl Mar 3, 2021

Choose a reason for hiding this comment

colin-grapl Mar 3, 2021

Choose a reason for hiding this comment

lgtm-com bot commented Mar 3, 2021

christophermaier left a comment

Choose a reason for hiding this comment

christophermaier Mar 3, 2021

Choose a reason for hiding this comment

inickles-grapl left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Mar 3, 2021

lgtm-com bot commented Mar 3, 2021

lgtm-com bot commented Mar 4, 2021

lgtm-com bot commented Mar 4, 2021

colin-grapl commented Jan 25, 2021 •

edited

Loading