feat: add hostname field in gRPC output #927

aarroyoc · 2019-11-18T12:30:03Z

Signed-off-by: Adrián Arroyo Calle adrian.arroyocalle@gmail.com

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area integrations

What this PR does / why we need it:

This PR adds a new field (hostname) in gRPC event messages.

Which issue(s) this PR fixes:

Fixes #923

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

In theory, it adds a new field without causing trouble to old clients. It also passes hostname to other outputs but it does nothing. This was done to keep consistency between methods signatures. Maybe we want to move the field to outputFields

update: adds a hostname field to gRPC output

Signed-off-by: Adrián Arroyo Calle <adrian.arroyocalle@gmail.com>

poiana · 2019-11-18T12:30:05Z

Welcome @aarroyoc! It looks like this is your first PR to falcosecurity/falco 🎉

fntlnz

@aarroyoc - this PR looks very good overall and this had also been a very needed feature in the community so thanks a lot for this!

I find this PR complete overall, I only want to see if we can do differently for the hostname, see my comment below.

fntlnz · 2019-11-18T20:04:28Z

userspace/falco/falco_outputs.cpp

@@ -146,6 +146,12 @@ void falco_outputs::handle_event(gen_event *ev, string &rule, string &source,

 	std::lock_guard<std::mutex> guard(m_ls_semaphore);
 	lua_getglobal(m_ls, m_lua_output_event.c_str());
+	char hostname[1024];
+	int err = gethostname(hostname, sizeof(hostname));


While this might be good for most use cases, imagine this scenario.

You run this as a Daemonset on Kubernetes.

Then you consume the outputs somewhere else and try to get the hostname.

You notice that you are getting the hostname of the container and not the one of the node.

My proposed solution, is to create a new environment variable for the hostname. Then we read that environment variable and if it's not present we get the system hostname.

In that way, when deploying in kubernetes we can just go and do something like this in the Pod spec:

... containers: - name: falco image: falcosecurity/falco:latest env: - name: FALCO_GRPC_HOSTNAME valueFrom: fieldRef: fieldPath: spec.nodeName ...

Please note that FALCO_GRPC_HOSTNAME is just a filler idea I had for the variable name, I'm totally unsure what name to pick !

I've updated the PR to reflect that change

Let's merge the functionality then we can pick battle on names (I really like such battles Lorenzo knows ahaha).

Signed-off-by: Adrián Arroyo Calle <adrian.arroyocalle@gmail.com>

leodido

Ok changes go in the correct direction. Nevertheless the env variable should be setup in another place. Also I would be prefer to use safer version of strcpy.

Finally there is a typo in your last commit message (253 rather than 256?) probably.

Signed-off-by: Adrián Arroyo Calle <adrian.arroyocalle@gmail.com>

palmerabollo · 2019-11-20T14:44:34Z

Hi @leodido, thanks for your feedback. Could you please point us to a better place to setup the env var? Thanks!

fntlnz · 2019-11-21T11:19:39Z

userspace/falco/falco_outputs.cpp

+		int err = gethostname(c_hostname, 256);
+		if(err != 0){
+			string err = "Failed to get hostname";
+			throw falco_exception(err);


You can pass the error string directly here

fntlnz · 2019-11-21T11:33:34Z

@palmerabollo @aarroyoc - the environment variable setup is all done in falco.cpp - see what we do with the k8s api env variable for example here: https://github.com/falcosecurity/falco/blob/dev/userspace/falco/falco.cpp#L1112-L1116

Once you have that, we have two options:

pass it as a member when creating falco_outputs
pass it to do_inspect so that when it comes to call handle_event we can give the information to the outputs.

Wdyt?

Signed-off-by: Adrián Arroyo Calle <adrian.arroyocalle@gmail.com>

aarroyoc · 2019-11-21T16:22:38Z

I've added it in falco_outputs because I think it's more of an initialization value

leodido

Thanks Adrian for applying the changes requested!

This seems pretty good to me! 🎉

There is only one nitpick: not sure we should throw falco_exception there. But to be fair I think that all the exceptions/errors should be reviewed and that's not the scope of this PR. So let's merge this! :)

poiana · 2019-11-27T18:33:04Z

LGTM label has been added.

Git tree hash: 6289fbf7c0d289f6d42887c2638d88a32b13e12d

palmerabollo · 2019-11-27T20:47:44Z

@fntlnz does it look good to you?

poiana · 2019-11-27T21:22:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fntlnz, 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~~ [fntlnz,leodido]

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

fntlnz · 2019-11-27T21:23:24Z

Looks good! Thanks @aarroyoc for the pr and for makin the requested changes ! This is a very needed and cool improvement ! Merging !

fntlnz · 2019-11-27T21:24:11Z

/milestone 0.19.0

feat: add hostname field in gRPC output

e73a70f

Signed-off-by: Adrián Arroyo Calle <adrian.arroyocalle@gmail.com>

poiana added release-note kind/feature labels Nov 18, 2019

poiana added dco-signoff: yes area/integrations labels Nov 18, 2019

poiana requested review from Kaizhe and mfdii November 18, 2019 12:30

poiana added the size/M label Nov 18, 2019

leodido requested review from fntlnz and leodido November 18, 2019 19:56

fntlnz suggested changes Nov 18, 2019

View reviewed changes

poiana assigned fntlnz Nov 18, 2019

aarroyoc added 2 commits November 19, 2019 16:57

chore: add environment variable FALCO_GRPC_HOSTNAME

fdcd89a

Signed-off-by: Adrián Arroyo Calle <adrian.arroyocalle@gmail.com>

chore: hostname can be 253 characters maximum

e5f5c5b

Signed-off-by: Adrián Arroyo Calle <adrian.arroyocalle@gmail.com>

leodido reviewed Nov 19, 2019

View reviewed changes

chore: use std::string to have safer copies

3dd2edc

Signed-off-by: Adrián Arroyo Calle <adrian.arroyocalle@gmail.com>

fntlnz reviewed Nov 21, 2019

View reviewed changes

poiana added dco-signoff: no and removed dco-signoff: yes labels Nov 21, 2019

chore: read hostname in initialization

2b463fe

Signed-off-by: Adrián Arroyo Calle <adrian.arroyocalle@gmail.com>

poiana added dco-signoff: yes and removed dco-signoff: no labels Nov 21, 2019

leodido approved these changes Nov 27, 2019

View reviewed changes

poiana assigned leodido Nov 27, 2019

poiana added the lgtm label Nov 27, 2019

poiana added the approved label Nov 27, 2019

fntlnz approved these changes Nov 27, 2019

View reviewed changes

fntlnz merged commit 1b05f0e into falcosecurity:dev Nov 27, 2019

poiana added this to the 0.19.0 milestone Nov 27, 2019

aarroyoc mentioned this pull request Nov 28, 2019

feat: add hostname in schema falcosecurity/client-go#27

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add hostname field in gRPC output #927

feat: add hostname field in gRPC output #927

aarroyoc commented Nov 18, 2019 •

edited by leodido

poiana commented Nov 18, 2019

fntlnz left a comment

fntlnz Nov 18, 2019

aarroyoc Nov 19, 2019

leodido Nov 27, 2019

leodido left a comment

palmerabollo commented Nov 20, 2019

fntlnz Nov 21, 2019

fntlnz commented Nov 21, 2019

aarroyoc commented Nov 21, 2019

leodido left a comment

poiana commented Nov 27, 2019

palmerabollo commented Nov 27, 2019 •

edited

poiana commented Nov 27, 2019

fntlnz commented Nov 27, 2019

fntlnz commented Nov 27, 2019

feat: add hostname field in gRPC output #927

feat: add hostname field in gRPC output #927

Conversation

aarroyoc commented Nov 18, 2019 • edited by leodido

poiana commented Nov 18, 2019

fntlnz left a comment

Choose a reason for hiding this comment

fntlnz Nov 18, 2019

Choose a reason for hiding this comment

aarroyoc Nov 19, 2019

Choose a reason for hiding this comment

leodido Nov 27, 2019

Choose a reason for hiding this comment

leodido left a comment

Choose a reason for hiding this comment

palmerabollo commented Nov 20, 2019

fntlnz Nov 21, 2019

Choose a reason for hiding this comment

fntlnz commented Nov 21, 2019

aarroyoc commented Nov 21, 2019

leodido left a comment

Choose a reason for hiding this comment

poiana commented Nov 27, 2019

palmerabollo commented Nov 27, 2019 • edited

poiana commented Nov 27, 2019

fntlnz commented Nov 27, 2019

fntlnz commented Nov 27, 2019

aarroyoc commented Nov 18, 2019 •

edited by leodido

palmerabollo commented Nov 27, 2019 •

edited