[PROF-14068] Remove privileges for host-profiler#2586
Conversation
theomagellan
added
the
datadog/minor-version
fdaa036 to
00fe6fb
Compare
github-actions
Bot
added
the
chart/datadog
1356c97 to
6c21719
Compare
theomagellan
changed the title
44b7570 to
84eafd6
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84eafd6197
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
b2934f6 to
210515e
Compare
There was a problem hiding this comment.
Hm, I didn't really challenge the existing deployment configuration.
cilium/ebpf seems to rely on writing to tracefs as fallback to old kernel limitations (cf. here).
The 5.10 kernel's alloc_trace_kprobe is not able to return -EINVAL based on symbol name, so we should be safe even if the fallback still exists in current cilium/ebpf versions.
Overall I would lean towards keeping it writeable in case something changes or odd behaviors occur somehow, but no strong opinions. I can make it read only if you feel strongly about this
There was a problem hiding this comment.
Actually, deployments in datadog-operator seem to be mounting it in read only so let's align to that as well. I'll comment previous findings in the yaml so we can keep an eye on it just in case.
|
We discussed a couple of points here. |
|
The PR needs to be aligned |
r1viollet
left a comment
r1viollet
left a comment
There was a problem hiding this comment.
LGTM, some minor improvement proposals
This is a step in the correct direction (even if we will no longer be able to do things like gdb / ptrace etc 😄 )
ee93e41 to
78cd53a
Compare
Yes since upstream now has kernel 5.10 as minimum. While testing though I found that we also needed
I looked into this and it's still pretty unclear to me if they are needed at all. Earlier today the container would fail without these but now it seems to be running fine in my testing environment. The only changes made were the capabilities, but I don't think this should change much 🤔 |
| readOnlyRootFilesystem: true | ||
| privileged: true | ||
| privileged: false | ||
| capabilities: |
3deacc1 to
67cc2de
Compare
|
hey @gpalmz 👋 this is ready for a review from your team! Thank you 🙇 |
ca1e730 to
04360ce
Compare
- provide necessary capabilities - restrict available syscalls - support apparmor profiles
- sourcemap-intake.%site for symbols - otlp.%site for metrics
the resourcedetection processor uses syscalls like `sysinfo` that were not present in the original allow-list that was bundled-centric.
e8c64cd to
bc70da0
Compare
- bump version for datadog to 3.209.0 (minor-version) - update changelog for datadog with version 3.209.0 - update readme for datadog
- bump version for datadog to 3.210.0 (minor-version) - update readme for datadog
4 participants
What this PR does / why we need it
This PR removes the Host Profiler's needs for
privileges: trueby only allowing strictly minimal capabilities and restricting available syscalls.Special notes for your reviewer
SYS_ADMIN, SYS_PTRACE, SYS_RESOURCE, DAC_READ_SEARCH, SYSLOGSCMP_ACT_ERRNOdefault.killrestricted to signal 0 via arg filter.setnsexcluded.generate-security-context(defaults tounconfined, matchingsystem-probe)sourcemap-intake.%s: symbol intakeotlp.%s: OTLP metrics intakeTest plan
--go-runtime-metricsOTLP self-loop worksProfiles using the chart can be found here for both supported architectures.
Note: self profiles (the profiler profiling its own process) are currently broken since non-privileged containers with hostPID get their own cgroup namespace and
container.idextraction fails. The cluster used for testing is using a custom image with a hotfix for this.Checklist
[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]
<chartName>/minor-version,<chartName>/patch-version, or<chartName>/no-version-bump)datadogordatadog-operatorchart or value changes, update the test baselines (run:make update-test-baselines)datadogchart changes, received ✅ from a member of your teamGitHub CI takes care of the below, but are still required:
.github/helm-docs.sh)CHANGELOG.mdhas been updatedREADME.md