Harden monit-access-helper.sh cgroupv2 mount point detection#599
Conversation
Restrict the inspection of /proc/self/mounts to cgroupv2 device (1st column) in addition to existing cgroup fstype (column 3). Also fail fast in case of multiple detected mount points. Fix cloudfoundry#585
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR renames the unified-cgroup detection helper to system_using_unified_cgroup_v2 and hardens permit_monit_access: it now extracts cgroup2 mountpoints from /proc/self/mounts only when both mount source and fs type indicate cgroup2, counts matches, requires exactly one match and a non-empty 0:: current cgroup, includes these resolved values in error output, and on success builds the monit-api-access sub-cgroup path from the validated mount and cgroup path. Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@stemcell_builder/stages/bosh_monit/assets/monit-access-helper.sh`:
- Around line 35-38: The error message in monit-access-helper.sh's
permit_monit_access check currently omits the count of matching cgroup v2
mounts; update the echo to include the nb_matching_cgroup_mounts variable so
operators can see whether zero, one, or multiple mounts were found. Locate the
conditional that checks cgroup_mount, nb_matching_cgroup_mounts, and
current_cgroup and modify the error string to append
"nb_matching_cgroup_mounts=${nb_matching_cgroup_mounts}" alongside the existing
current_cgroup and cgroup_mount values.
- Line 33: The variable nb_matching_cgroup_mounts is computed with wc -l which
returns 1 for an empty cgroup_mount (because echo "" yields a newline); replace
that wc -l approach so nb_matching_cgroup_mounts accurately reflects the number
of non-empty lines in cgroup_mount. Feed cgroup_mount to a one-pass line counter
(e.g., run awk over the input and increment for lines with content, printing 0
if none) and assign that result to nb_matching_cgroup_mounts, keeping the
surrounding logic that checks -z "${cgroup_mount}" unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cff13715-d8eb-49a1-9181-f8cc3eac0fee
📒 Files selected for processing (1)
stemcell_builder/stages/bosh_monit/assets/monit-access-helper.sh
github-project-automation
Bot
moved this from Inbox
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
beyhan
requested review from
a team,
lnguyen and
s4heid
and removed request for
a team
beyhan
moved this from Waiting for Changes | Open for Contribution
to Pending Review | Discussion
in Foundational Infrastructure Working Group
As suggested in cloudfoundry#599 (review) Verified manually and through https://stackoverflow.com/a/42399738 Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
stemcell_builder/stages/bosh_monit/assets/monit-access-helper.sh (1)
35-38: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winInclude mount count in error message for better diagnostics.
The error message does not include
nb_matching_cgroup_mounts, making it harder to distinguish between zero mounts, multiple mounts, or an empty cgroup path when troubleshooting failures.📝 Suggested enhancement
- echo "permit_monit_access: unable to resolve cgroup v2 mount or path. current_cgroup=${current_cgroup} cgroup_mount=${cgroup_mount}" >&2 + echo "permit_monit_access: unable to resolve cgroup v2 mount or path. nb_matching_cgroup_mounts=${nb_matching_cgroup_mounts} current_cgroup=${current_cgroup} cgroup_mount=${cgroup_mount}" >&2This helps operators quickly distinguish between:
- No mounts found (
nb_matching_cgroup_mounts=0)- Multiple mounts found (
nb_matching_cgroup_mounts=2+)- Empty current_cgroup path
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stemcell_builder/stages/bosh_monit/assets/monit-access-helper.sh` around lines 35 - 38, Update the error message in permit_monit_access (in monit-access-helper.sh) to include the nb_matching_cgroup_mounts variable so diagnostics show whether there were 0, multiple, or other matching cgroup mounts; change the echo line that currently prints current_cgroup and cgroup_mount to also append nb_matching_cgroup_mounts so the log reports current_cgroup=${current_cgroup} cgroup_mount=${cgroup_mount} nb_matching_cgroup_mounts=${nb_matching_cgroup_mounts} before returning 1.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@stemcell_builder/stages/bosh_monit/assets/monit-access-helper.sh`:
- Around line 35-38: Update the error message in permit_monit_access (in
monit-access-helper.sh) to include the nb_matching_cgroup_mounts variable so
diagnostics show whether there were 0, multiple, or other matching cgroup
mounts; change the echo line that currently prints current_cgroup and
cgroup_mount to also append nb_matching_cgroup_mounts so the log reports
current_cgroup=${current_cgroup} cgroup_mount=${cgroup_mount}
nb_matching_cgroup_mounts=${nb_matching_cgroup_mounts} before returning 1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fad58a8a-100a-401c-b782-2300fa5a4f71
📒 Files selected for processing (1)
stemcell_builder/stages/bosh_monit/assets/monit-access-helper.sh
Now also display nb_matching_cgroup_mounts
github-project-automation
Bot
moved this from Pending Review | Discussion
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
|
thanks @rkoster for the assisted review. Please let me know if there are other feedback that needs handling for this PR. |
|
@mkocher could you take a look at this one given you have the most context on the jammy cgroups v2 magic. |
rkoster
moved this from Pending Merge | Prioritized
to Pending Review | Discussion
in Foundational Infrastructure Working Group
Remove redundant conditional as suggested in cloudfoundry#599 (review)
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
stemcell_builder/stages/bosh_monit/assets/monit-access-helper.sh (1)
22-25:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix breaking rename:
restrict-monit-api-accessstill calls the old function name
stemcell_builder/stages/bosh_monit/assets/restrict-monit-api-accessstill referencesmonit_using_unified_cgroup_v2(line 5), butstemcell_builder/stages/bosh_monit/assets/monit-access-helper.shnow definessystem_using_unified_cgroup_v2, so this will break when the old function isn’t present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stemcell_builder/stages/bosh_monit/assets/monit-access-helper.sh` around lines 22 - 25, The helper function was renamed to system_using_unified_cgroup_v2 but the script restrict-monit-api-access still calls monit_using_unified_cgroup_v2; update restrict-monit-api-access to call system_using_unified_cgroup_v2 (or add a small shim function named monit_using_unified_cgroup_v2 that forwards to system_using_unified_cgroup_v2) so the runtime reference matches the defined symbol and avoids a missing-function error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@stemcell_builder/stages/bosh_monit/assets/monit-access-helper.sh`:
- Around line 22-25: The helper function was renamed to
system_using_unified_cgroup_v2 but the script restrict-monit-api-access still
calls monit_using_unified_cgroup_v2; update restrict-monit-api-access to call
system_using_unified_cgroup_v2 (or add a small shim function named
monit_using_unified_cgroup_v2 that forwards to system_using_unified_cgroup_v2)
so the runtime reference matches the defined symbol and avoids a
missing-function error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e16c72b5-3e5f-4780-8e8d-2001d19de0d7
📒 Files selected for processing (1)
stemcell_builder/stages/bosh_monit/assets/monit-access-helper.sh
github-project-automation
Bot
moved this from Pending Review | Discussion
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
github-project-automation
Bot
moved this from Pending Merge | Prioritized
to Done
in Foundational Infrastructure Working Group
|
Thanks! @gberche-orange |
|
Thanks for your review and merging @rkoster ! |
3 participants
Restrict the inspection of /proc/self/mounts to cgroupv2 device (1st column) in addition to existing cgroup fstype (column 3).
Also fail fast in case of multiple detected mount points.
Fix #585
NOTE: this repository uses a "Merge Forward" strategy
Changes should be made in the earliest applicable branch, and
merged forward through subsequent branches.
ubuntu-<short_name>)merge-to-<next_short_name>branchubuntu-<short_name>intomerge-to-<next_short_name>merge-to-<next_short_name>intoubuntu-<next_short_name>