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

fix dir mounting collision when both gRPC and nats are enabled #55

Closed
wants to merge 2 commits into from

Conversation

leogr
Copy link
Member

@leogr leogr commented Jul 22, 2020

What type of PR is this?

/kind bug

/kind chart-release

Any specific area of the project related to this PR?

/area falco-chart

What this PR does / why we need it:

This PR fixes the duplicate /var/run/falco mounting problem (when both gRPC and nats are enabled) by prefixing mounting point inside the container with /host for the gRPC Unix socket path.
Note that at the host level the mounting point is still equal to one specified by the config (ie. without the /host prefix)

Which issue(s) this PR fixes:

Fixes #54

Special notes for your reviewer:

Checklist

  • Chart Version bumped
  • CHANGELOG.md updated

Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
@poiana poiana added do-not-merge/work-in-progress kind/bug Something isn't working dco-signoff: yes kind/chart-release Add this label when the chart version has been bumped area/falco-chart labels Jul 22, 2020
@poiana
Copy link
Contributor

poiana commented Jul 22, 2020

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign leogr
You can assign the PR to them by writing /assign @leogr in a comment when ready.

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

Needs approval from an approver in each of these files:

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

@poiana poiana added the size/M label Jul 22, 2020
@leogr leogr mentioned this pull request Jul 22, 2020
@leogr
Copy link
Member Author

leogr commented Jul 23, 2020

/cc @nibalizer
PTAL

Could someone here help me in testing this PR with NATS enabled?

@poiana poiana requested a review from nibalizer July 23, 2020 12:32
@leogr leogr changed the title wip: fix dir mounting collision when both gRPC and nats are enabled fix dir mounting collision when both gRPC and nats are enabled Jul 24, 2020
@nibalizer
Copy link
Contributor

I can try to test this @leogr :)

@nibalizer
Copy link
Contributor

nibalizer commented Jul 24, 2020

This worked for me when I tested it. It brings up a couple questions:

  1. Why is NATS support a special case now? The sidekick supports it (and a million other things) without a need for a emptydir/fifo or custom daemon or a docker image still in the sysding namespace. I think we could clean up the helm chart a bit by saying if you want nats use the sidekick.

  2. The use of /host is I think maybe not the right choice. If we know the exporter or another grpc-unixsocket consumer will be deployed in the same pod as falco I think we could use emptydir. I don't think using the host's filesystem is a bad idea but using the specific path of (the host's) /var/run/falco doesn't seem right to me and assumes any consumer pod would mount in that directory. As a user I'd rather not mount in /var/run from my host if I didn't have to.

My request here is to move the path to something unique and unimportant on the base host or to use an emptyDir.

@leogr
Copy link
Member Author

leogr commented Jul 27, 2020

  1. Why is NATS support a special case now? The sidekick supports it (and a million other things) without a need for a emptydir/fifo or custom daemon or a docker image still in the sysding namespace. I think we could clean up the helm chart a bit by saying if you want nats use the sidekick.

I totally agree, but unfortunately, I don't have any experience with NATS so I can't make a decision.
cc @fntlnz @leodido

  1. The use of /host is I think maybe not the right choice. If we know the exporter or another grpc-unixsocket consumer will be deployed in the same pod as falco I think we could use emptydir. I don't think using the host's filesystem is a bad idea but using the specific path of (the host's) /var/run/falco doesn't seem right to me and assumes any consumer pod would mount in that directory. As a user I'd rather not mount in /var/run from my host if I didn't have to.

There're different issues here.

Regarding /host I can agree. I have chosen that path just because Falco is already mounting other host paths there. I know there's no formal convention about that, but it seems to be a de facto standard for Falco.

Secondly, since Falco can be installed either with a DaemonSet or in the host directly, /var/run/falco is the natural place a consumer should look for the Falco UnixSocket socket. Indeed, falco-exporter - that does NOT run the in the same pod since it's a different deployment - mounts /var/run/falco from the host for this reason.
That being said, I'm not sure this is the best approach, but it was natural to me because of the host installation use case.

My request here is to move the path to something unique and unimportant on the base host or to use an emptyDir.

I'd take a different strategy: if we could modify the NATS configuration to do not use /var/run/falco, then I will just leave the gRPC configuration as-is, and I will not merge this PR (mainly because of the host level use case).

But, can we modify the NATS?

@nibalizer
Copy link
Contributor

I believe the NATS code in question is actually sysdig/falco code here. So we could modify that, yes.

@leogr
Copy link
Member Author

leogr commented Jul 27, 2020

I believe the NATS code in question is actually sysdig/falco code here. So we could modify that, yes.

I have to look into that.
cc @nestorsalceda

@leogr
Copy link
Member Author

leogr commented Aug 18, 2020

I have looked again into this and @nibalizer reasons are enough to close this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/falco-chart dco-signoff: yes kind/bug Something isn't working kind/chart-release Add this label when the chart version has been bumped size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate mount point
3 participants