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

Allow customizing the base path and don't mount the BPF fs if it is already mounted #741

Merged
merged 6 commits into from
Apr 16, 2024

Conversation

dashpole
Copy link
Contributor

Fixes #732

This enables beyla to be run without the SYS_ADMIN capability. See dashpole#1 for more details.

The code to determine if a mount is a bpf mount is based on https://github.com/cilium/cilium/blob/5130d33a835638c78dda2572d7dc92091ffb3dc1/pkg/mountinfo/mountinfo_linux.go#L27. Importing that would add a dependency on github.com/cilium/cilium, which the project doesn't currently have.

I could:

  • Add a dependency on cilium, or
  • Put the code in its own file with the cilium copyright header, and also copy the unit tests.

Let me know which you would prefer, and if there is anything else needed (docs updates, etc) for the change.

@mariomac
Copy link
Contributor

Nice! It looks good to me. What do you think would be the correct approach to provide unit/integration tests for this feature?

@mariomac
Copy link
Contributor

With respect to what to do with the Cilium imported code, I think that copying it and give proper attribution in the file headers is OK. Maybe also update this sentence: https://github.com/grafana/beyla/blob/main/NOTICE#L14-L16

@dashpole
Copy link
Contributor Author

I've updated the copyright headers and the NOTICE file.

For unit/integration testing, i've followed the model used by cilium, which is to have some unit tests that are run as privileged. I have unit tests for IsMountFS, as well as mountBpfPinPath and unmountBpfPinPath that are only run when the PRIVILEGED_TESTS env var is set. I've added a test-privileged case to the Makefile, which can be run with sudo make test-privileged.

But it isn't run as a presubmit right now, as that setup looks complex: https://github.com/cilium/cilium/blob/e3ea2ed7c1250f6f3410c3fe4d6b1f51ea759d65/.github/workflows/conformance-runtime.yaml#L374

@dashpole dashpole marked this pull request as ready for review April 16, 2024 18:15
@grcevski
Copy link
Contributor

We have an intermittent test issue, I've kicked off a re-run on the integration tests. I think it should pass now...

Copy link
Contributor

@grcevski grcevski left a comment

Choose a reason for hiding this comment

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

LGTM! Looks great, thanks for this contribution @dashpole !!!

@grcevski grcevski merged commit 0dda593 into grafana:main Apr 16, 2024
4 checks passed
@dashpole dashpole deleted the customize_base_path branch April 17, 2024 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support running beyla without SYS_ADMIN
3 participants