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

support for using unix socket for gRPC #552

Merged
merged 3 commits into from
Dec 1, 2022
Merged

support for using unix socket for gRPC #552

merged 3 commits into from
Dec 1, 2022

Conversation

kkourt
Copy link
Contributor

@kkourt kkourt commented Nov 22, 2022

This PR adds support for using a unix socket for the gRPC agent interface.

@kkourt kkourt requested a review from a team as a code owner November 22, 2022 08:00
@kkourt kkourt requested a review from tixxdz November 22, 2022 08:00
@kkourt kkourt force-pushed the pr/kkourt/unix-socket branch from b56d0ae to 5aaa41a Compare November 22, 2022 10:52
@tixxdz
Copy link
Member

tixxdz commented Nov 23, 2022

Thank you @kkourt !

Could you please add an explicit chmod fix to the socket 0700 after it is created, so we don't rely on any lib...

@kkourt
Copy link
Contributor Author

kkourt commented Nov 23, 2022 •
edited
Loading

Thank you @kkourt !

Could you please add an explicit chmod fix to the socket 0700 after it is created, so we don't rely on any lib...

I don't think it's a good idea to assume what permissions users want. I was thinking users can just define a proper umask for what they want.

@tixxdz
Copy link
Member

tixxdz commented Nov 23, 2022 •
edited
Loading

Thank you @kkourt !
Could you please add an explicit chmod fix to the socket 0700 after it is created, so we don't rely on any lib...

I don't think it's a good idea to assume what permissions users want. I was thinking users can just define a proper umask for what they want.

We should be safe by default, so doing it the other way is better: have proper permission fix after it is created, then document users if they want to expose it : chmod go+rwx something like that

@kkourt kkourt marked this pull request as draft November 24, 2022 10:39
@kkourt kkourt marked this pull request as ready for review November 25, 2022 10:27
@kkourt
Copy link
Contributor Author

kkourt commented Nov 25, 2022

Thank you @kkourt !
Could you please add an explicit chmod fix to the socket 0700 after it is created, so we don't rely on any lib...

I don't think it's a good idea to assume what permissions users want. I was thinking users can just define a proper umask for what they want.

We should be safe by default, so doing it the other way is better: have proper permission fix after it is created, then document users if they want to expose it : chmod go+rwx something like that

I don't think we should expect users to chmod things manually. I've added a patch that makes it so that the socket file is created with 0660. Long term, I think we need something similar to what the Cilium agent does: https://github.com/cilium/cilium/blob/54e70e2933102bdadfadb27dd352f35d0b35622f/api/v1/server/server.go#L189.

PTAL.

@kkourt kkourt force-pushed the pr/kkourt/unix-socket branch from 02c69a2 to 6833fd2 Compare November 25, 2022 10:33
@kkourt
tetragon: add support for gRPC over unix socket
4b25128 
Now we can provide unix://abosolute_path arguments to both the client
(tetra) and the agent (tetragon).

Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
@kkourt
helm: remove grpc.port and fix livenessProbe
9bec5e9 
Up until now, configuring gRPC listen address for the agent via the helm
chart used two variables grpc.port and grpc.address.

The previous patch added support for passing a unix socket address to
the agent, which does not match well the grpc.address:grpc.port
configuration.

This patch removes the grpc.port helm variable, and, instead, relies
only on grpc.address for configuration.

Users can use "localhost:54321", ":54321", or
"unix:///var/run/tetragon/tetragon.sock" to configure the gRPC address
that the agent listens to.

Furthermore, the livenessProbe of the agent relies on checking health
status via the gRPC interface via the CLI (tetra status). This patch
also fixes the livenessProbe so that:
 - it is only defined, if grpc is enabled
 - the proper gRPC address is used to contact the agent

Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
@kkourt
tetragon: chmod unix socket to 0660
0d7f811 
This patch ensures that the control unix socket for the agent has 0660
file permissions.

Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
@kkourt kkourt force-pushed the pr/kkourt/unix-socket branch from 6833fd2 to 0d7f811 Compare November 25, 2022 15:12
tixxdz
tixxdz approved these changes Nov 28, 2022
View reviewed changes
Copy link
Member

@tixxdz tixxdz left a comment

Choose a reason for hiding this comment

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

Thank you! unix socket files should behave like normal files.. maybe a test for the ListenWithRename to stat ensure that socket is created with the right mode... up to you!

@kkourt kkourt merged commit 9aae81d into main Dec 1, 2022
@kkourt kkourt deleted the pr/kkourt/unix-socket branch December 1, 2022 10:40
@kkourt kkourt mentioned this pull request Dec 9, 2022
Merged
@lambdanis lambdanis mentioned this pull request May 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tixxdz tixxdz tixxdz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kkourt @tixxdz