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

Change the workflow on how we set the right permissions for perf-plugin #16558

Merged
merged 8 commits into from Dec 11, 2023

Conversation

tkatsoulas
Copy link
Contributor

@tkatsoulas tkatsoulas commented Dec 7, 2023

Summary

The current workflow has a flaw, the post installation script tries to set capabilities without making sure that the system supports them, so the script fails.

The new workflow follows the least privileged approach. Uses the CAP_PERFMON in systems that support it and if it fails to set CAP_SYS_ADMIN then sets the setuid bit.

Test Plan

On a deb based environment without support for CAPS

  1. Build native packages locally for your arch https://github.com/netdata/netdata/blob/master/packaging/building-native-packages-locally.md
  2. Install them.
    installation must finish without errors
  3. dpkg --configure -a, must not return any errors.

Closes #16556

Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>
@github-actions github-actions bot added the area/packaging Packaging and operating systems support label Dec 7, 2023
@tkatsoulas tkatsoulas added the patch-release-candidate Mark a PR as being a candidate for inclusion in a patch release. label Dec 7, 2023
Ferroin
Ferroin previously approved these changes Dec 7, 2023
Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>
Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>
Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>
Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>
Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>
Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>
@tkatsoulas
Copy link
Contributor Author

I will spin up test envs for the 3 cases

  • with cap_perfmon
  • without cap_perfmon but supporting capabilities
  • without capabilities support

@ilyam8
Copy link
Member

ilyam8 commented Dec 7, 2023

@tkatsoulas I think it looks good now and will work. A minor note: we don't echo "Set..." in other postint files, i don't know if that is good practice or not to echo something in those files cc @Ferroin If not and you add echo to not have first branch empty, consider the following:

if ! first && ! second; then
  action
fi

if that is not a big deal nvm, lgtm.

@Ferroin
Copy link
Member

Ferroin commented Dec 7, 2023

@tkatsoulas I think it looks good now and will work. A minor note: we don't echo "Set..." in other postint files, i don't know if that is good practice or not to echo something in those files cc @Ferroin If not and you add echo to not have first branch empty, consider the following:

if ! first && ! second; then
  action
fi

if that is not a big deal nvm, lgtm.

It’s generally not recommended to echo stuff there, but it’s not a major issue in most cases either.

@Ferroin Ferroin merged commit e8a12b3 into master Dec 11, 2023
87 checks passed
@Ferroin Ferroin deleted the fix-caps-perf branch December 11, 2023 14:03
stelfrag pushed a commit to stelfrag/netdata that referenced this pull request Dec 11, 2023
…in (netdata#16558)

* Change the workflow on how we set the right permissions

Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>

* Add a failsafe in case fail to set the cap_perfmon

Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>

* add EOF new line

Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>

* Fix workflow

Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>

* minor spelling

Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>

* we made it odyssey

Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>

* Apply suggestion from code review

Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>

* Tidy-up postinst script.

---------

Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>
Co-authored-by: Austin S. Hemmelgarn <austin@netdata.cloud>
(cherry picked from commit e8a12b3)
@stelfrag stelfrag mentioned this pull request Dec 11, 2023
tkatsoulas added a commit that referenced this pull request Dec 12, 2023
…in (#16558)

* Change the workflow on how we set the right permissions

Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>

* Add a failsafe in case fail to set the cap_perfmon

Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>

* add EOF new line

Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>

* Fix workflow

Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>

* minor spelling

Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>

* we made it odyssey

Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>

* Apply suggestion from code review

Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>

* Tidy-up postinst script.

---------

Signed-off-by: Tasos Katsoulas <tasos@netdata.cloud>
Co-authored-by: Austin S. Hemmelgarn <austin@netdata.cloud>
(cherry picked from commit e8a12b3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/packaging Packaging and operating systems support patch-release-candidate Mark a PR as being a candidate for inclusion in a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Perf-plugin error on systems with no setcap
3 participants