Update grpc #36

nibalizer · 2020-07-07T17:53:21Z

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind documentation

/kind tests

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area api

/area client
/area grpc

/area examples

/area build

What this PR does / why we need it:

A few things in here:

Cleanup build/package so it can pip install .
Add unix socket grpc support
Update protos for existing protos
Add proto for version proto

Which issue(s) this PR fixes:

Fixes #35
Fixes #34
Fixes #25

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

The bi-directional api is different from the single direction api and uses different verbs.
client.subscribe is removed and replaced with client.sub. 
There is no need to pass keepalive=True anywhere.
client.get() will pull any events in-memory up to the point when the function is called.
client.version() will get the falco version from the server.

Also futz with build system Signed-off-by: Spencer Krum <nibz@spencerkrum.com>

Signed-off-by: Spencer Krum <nibz@spencerkrum.com>

nibalizer · 2020-07-07T17:54:09Z

I'm still working on this

/hold

falco/client.py

nibalizer · 2020-07-28T19:50:41Z

/hold cancel

nibalizer · 2020-07-28T19:51:09Z

@leodido @mmat11 I believe this is ready for review now. Thanks in advance!

nibalizer · 2020-07-28T19:53:14Z

falco/client.py

+        pass
+
+    def EmptyRequests(self):
+        while True:


I've seen examples where they put a sleep here.

After code review from @mmatt11 Also other misc fixes and an example. Signed-off-by: Spencer Krum <nibz@spencerkrum.com>

Signed-off-by: Spencer Krum <nibz@spencerkrum.com>

mmat11 · 2020-08-05T22:16:17Z

falco/client.py

+class TLSConfigError(Exception):
+    pass
+
+class RequestGenerator:


I think this should just be a function (and should belong to the outputs domain model)

falco/client.py

mmat11 · 2020-08-05T22:20:15Z

falco/client.py

+from falco.domain import Response, Request
+from falco.svc.outputs_pb2_grpc import serviceStub
+from falco.svc.version_pb2_grpc import serviceStub as versionServiceStub
+from falco.schema.version_pb2 import request as versionRequest


In my opinion the Client should only use domain models

I took a stab at the refactor you're suggesting and got pretty confused. Would you be willing to throw a patch together to move the code into the domain model?

falco/client.py

mmat11 · 2020-08-05T22:24:10Z

falco/domain/request.py

-
-    def __repr__(self):
-        return f"{self.__class__.__name__}(keepalive={self.keepalive})"
+    __slots__ = []


probably it's the same but maybe we should use a tuple here

mmat11 · 2020-08-05T22:25:08Z

tests/domain/test_request.py

@@ -1,27 +1,12 @@
 import pytest

 from falco import Request
-from output_pb2 import request
+from outputs_pb2 import request


 class TestRequest:


I think this test doesn't have a purpose anymore

Yep, this doesn't have a purpose anymore.

Anyway, I'd leave it here for now for when we'll have fields into the request messages (eg., rule/alerts tags)

mmat11 · 2020-08-05T22:27:13Z

We don't have a release process yet but probably we should still bump the version number here -> https://github.com/falcosecurity/client-py/blob/master/falco/__version__.py#L4

nibalizer · 2020-08-11T14:56:09Z

Thanks @mmat11 ! I'll get to these.

fntlnz

I just tested this and works like a charm.
I don't have merge powers on this repo so just take my approve as a sign of appreciation for this work!

poiana · 2020-08-18T09:28:12Z

LGTM label has been added.

Git tree hash: 089d3800857f2a9e8044eb1c93f6529d8727b657

Makefile

leodido

Looks great to me!

Congrats for this awesome work @nibalizer 🤗

Left a comment about max_receive_message_length grpc option.

Could you update the release-note block to match the conventions, please?

leodido · 2020-08-18T09:48:47Z

falco/client.py

+        if endpoint.startswith("unix:///"):
+            channel = grpc.insecure_channel(
+                endpoint,
+                options=[("grpc.max_receive_message_length", 1024 * 1024 * 512)],


Is this value for receiving messages exact?

This is copy and pasted from the tls connection below. I didn't find us setting a maximum size in the falco source code to match this with.

leodido · 2020-08-18T09:52:39Z

tests/domain/test_request.py

@@ -1,27 +1,12 @@
 import pytest

 from falco import Request
-from output_pb2 import request
+from outputs_pb2 import request


 class TestRequest:


Yep, this doesn't have a purpose anymore.

Anyway, I'd leave it here for now for when we'll have fields into the request messages (eg., rule/alerts tags)

BREAKING CHANGE: removes subscribe method from client and changes all examples This normalizes get/sub accross our examples Signed-off-by: Spencer Krum <nibz@spencerkrum.com>

Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com> Co-authored-by: Mattia <mattia@airmail.cc>

nibalizer · 2020-08-18T20:02:49Z

Thanks team for the reviews! I changed what I could. I don't think I can move the code into the domain model that you're asking for @mmat11, I just don't understand it well enough.

leodido

Let's merge this awesome PR by @nibalizer 🎉

Further refinements can be done in follow-up PRs, imho.

Having this merged in now helps all the people from KubeCon that want to try out the gRPC alerts.

poiana · 2020-08-19T05:06:36Z

LGTM label has been added.

Git tree hash: 66ce6ca20405f5c5e9ca38312ebdd816995ae65b

poiana · 2020-08-19T05:06:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leodido

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [leodido]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nibalizer added 4 commits July 6, 2020 21:07

chore(grpc): use latest grpc protos

c68f511

Also futz with build system Signed-off-by: Spencer Krum <nibz@spencerkrum.com>

feat(grpc): Add falco version api

ac550f3

Signed-off-by: Spencer Krum <nibz@spencerkrum.com>

chore(grpc): Updates for new outputs api

eb9790c

Signed-off-by: Spencer Krum <nibz@spencerkrum.com>

feat(grpc): add unix socket support

a451fbb

Signed-off-by: Spencer Krum <nibz@spencerkrum.com>

poiana added release-note kind/cleanup This doesn't seem clean dco-signoff: yes kind/feature New feature or request area/client labels Jul 7, 2020

poiana requested review from leodido and markjacksonfishing July 7, 2020 17:53

poiana added the size/XXL label Jul 7, 2020

poiana added the do-not-merge/hold label Jul 7, 2020

nibalizer marked this pull request as draft July 7, 2020 17:54

mmat11 reviewed Jul 8, 2020

View reviewed changes

falco/client.py Outdated Show resolved Hide resolved

mmat11 reviewed Jul 8, 2020

View reviewed changes

falco/client.py Show resolved Hide resolved

poiana added dco-signoff: no and removed dco-signoff: yes labels Jul 8, 2020

nibalizer force-pushed the update_grpc branch from 296d852 to be7a1cc Compare July 28, 2020 19:50

poiana added dco-signoff: yes and removed dco-signoff: no labels Jul 28, 2020

poiana removed the do-not-merge/hold label Jul 28, 2020

nibalizer marked this pull request as ready for review July 28, 2020 19:51

nibalizer commented Jul 28, 2020

View reviewed changes

falco/client.py

pass

def EmptyRequests(self):

while True:

Copy link

Contributor Author

nibalizer Jul 28, 2020

Choose a reason for hiding this comment

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

I've seen examples where they put a sleep here.

refactor: create channel only in if..else block

a2f9f8b

After code review from @mmatt11 Also other misc fixes and an example. Signed-off-by: Spencer Krum <nibz@spencerkrum.com>

nibalizer force-pushed the update_grpc branch from be7a1cc to a2f9f8b Compare July 28, 2020 19:57

fix: repair tests

8de3cb7

Signed-off-by: Spencer Krum <nibz@spencerkrum.com>

nibalizer force-pushed the update_grpc branch from a1a67db to 9a6cc30 Compare July 30, 2020 23:07

mmat11 reviewed Aug 5, 2020

View reviewed changes

falco/client.py Outdated Show resolved Hide resolved

mmat11 reviewed Aug 5, 2020

View reviewed changes

falco/client.py Outdated Show resolved Hide resolved

mmat11 reviewed Aug 5, 2020

View reviewed changes

falco/client.py Outdated Show resolved Hide resolved

mmat11 reviewed Aug 5, 2020

View reviewed changes

fntlnz previously approved these changes Aug 18, 2020

View reviewed changes

poiana assigned fntlnz Aug 18, 2020

poiana added the lgtm label Aug 18, 2020

leodido reviewed Aug 18, 2020

View reviewed changes

Makefile Show resolved Hide resolved

leodido dismissed fntlnz’s stale review via bd60a23 August 18, 2020 09:53

poiana removed the lgtm label Aug 18, 2020

leodido reviewed Aug 18, 2020

View reviewed changes

nibalizer and others added 2 commits August 18, 2020 20:00

feat!: add final grpc endpoint (get)

d42a411

BREAKING CHANGE: removes subscribe method from client and changes all examples This normalizes get/sub accross our examples Signed-off-by: Spencer Krum <nibz@spencerkrum.com>

update(falco): test stubs

e6876bd

Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com> Co-authored-by: Mattia <mattia@airmail.cc>

nibalizer force-pushed the update_grpc branch from bd60a23 to e6876bd Compare August 18, 2020 20:00

leodido approved these changes Aug 19, 2020

View reviewed changes

poiana assigned leodido Aug 19, 2020

poiana added the lgtm label Aug 19, 2020

poiana added the approved label Aug 19, 2020

poiana merged commit eccdecd into falcosecurity:master Aug 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update grpc #36

Update grpc #36

nibalizer commented Jul 7, 2020 •

edited

Loading

nibalizer commented Jul 7, 2020 •

edited

Loading

nibalizer commented Jul 28, 2020

nibalizer commented Jul 28, 2020

nibalizer Jul 28, 2020

mmat11 Aug 5, 2020

mmat11 Aug 5, 2020

nibalizer Aug 18, 2020

mmat11 Aug 5, 2020

mmat11 Aug 5, 2020

leodido Aug 18, 2020

mmat11 commented Aug 5, 2020

nibalizer commented Aug 11, 2020

fntlnz left a comment

poiana commented Aug 18, 2020

leodido left a comment •

edited

Loading

leodido Aug 18, 2020 •

edited

Loading

nibalizer Aug 18, 2020

leodido Aug 18, 2020

nibalizer commented Aug 18, 2020

leodido left a comment

poiana commented Aug 19, 2020

poiana commented Aug 19, 2020

Update grpc #36

Update grpc #36

Conversation

nibalizer commented Jul 7, 2020 • edited Loading

nibalizer commented Jul 7, 2020 • edited Loading

nibalizer commented Jul 28, 2020

nibalizer commented Jul 28, 2020

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

mmat11 commented Aug 5, 2020

nibalizer commented Aug 11, 2020

fntlnz left a comment

Choose a reason for hiding this comment

poiana commented Aug 18, 2020

leodido left a comment • edited Loading

Choose a reason for hiding this comment

leodido Aug 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nibalizer commented Aug 18, 2020

leodido left a comment

Choose a reason for hiding this comment

poiana commented Aug 19, 2020

poiana commented Aug 19, 2020

nibalizer commented Jul 7, 2020 •

edited

Loading

nibalizer commented Jul 7, 2020 •

edited

Loading

leodido left a comment •

edited

Loading

leodido Aug 18, 2020 •

edited

Loading