fix(deployment): Enforce read-only permissions on logs input and stream directories (fixes #1689).#1690
Merged
Merged
Conversation
…am directories (fixes y-scope#1689).
Contributor
WalkthroughThe pull request tightens bind mount access in the Docker Compose file by appending Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related issues
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-12-04T03:31:55.239ZApplied to files:
📚 Learning: 2025-10-17T19:59:25.596ZApplied to files:
📚 Learning: 2025-07-23T09:54:45.185ZApplied to files:
📚 Learning: 2025-10-07T07:54:32.427ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (1)
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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tools/deployment/package/docker-compose-all.yaml(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1178
File: components/clp-package-utils/clp_package_utils/controller.py:315-315
Timestamp: 2025-10-17T19:59:25.596Z
Learning: In components/clp-package-utils/clp_package_utils/controller.py, worker log directories (compression_worker, query_worker, reducer) created via `mkdir()` do not need `_chown_paths_if_root()` calls because directories are created with the same owner as the script caller. This differs from infrastructure service directories (database, queue, Redis, results cache) which do require explicit ownership changes.
📚 Learning: 2025-10-17T19:59:25.596Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1178
File: components/clp-package-utils/clp_package_utils/controller.py:315-315
Timestamp: 2025-10-17T19:59:25.596Z
Learning: In components/clp-package-utils/clp_package_utils/controller.py, worker log directories (compression_worker, query_worker, reducer) created via `mkdir()` do not need `_chown_paths_if_root()` calls because directories are created with the same owner as the script caller. This differs from infrastructure service directories (database, queue, Redis, results cache) which do require explicit ownership changes.
Applied to files:
tools/deployment/package/docker-compose-all.yaml
🔇 Additional comments (1)
tools/deployment/package/docker-compose-all.yaml (1)
239-239: All four specified read-only mount changes are correctly implemented.The PR successfully adds
:roflags to:
- compression-scheduler /mnt/logs (line 239)
- compression-worker /mnt/logs (line 273)
- query-worker /var/data/archives (line 424) — addresses prior reviewer concern
- webui /var/data/streams (line 310)
The changes align with the PR objectives and validation. The prior reviewer's question about query-worker archive access has been resolved by adding
:ro.Also applies to: 273-273, 310-310, 424-424
Comment on lines
+424
to
427
| - "${CLP_ARCHIVE_OUTPUT_DIR_HOST:-empty}:/var/data/archives:ro" | ||
| - "${CLP_AWS_CONFIG_DIR_HOST:-empty}:/opt/clp/.aws:ro" | ||
| - "${CLP_STAGED_STREAM_OUTPUT_DIR_HOST:-empty}:/var/data/staged-streams" | ||
| - "${CLP_STREAM_OUTPUT_DIR_HOST:-empty}:/var/data/streams" |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider enforcing read-only for query-worker's streams directory in a follow-up.
Query-worker reads archives (line 424, now :ro) and streams (line 427, no :ro), but writes only to staged-streams (line 426). If query-worker doesn't modify /var/data/streams, the mount at line 427 should also be read-only for consistency with webui (line 310) and defensive security posture.
This is outside the current PR scope but worth validating for a future enhancement.
🤖 Prompt for AI Agents
In tools/deployment/package/docker-compose-all.yaml around lines 424 to 427, the
query-worker service mounts /var/data/streams without read-only flag while
archives and staged-streams are mounted read-only; verify whether query-worker
ever writes to /var/data/streams and if it does not, change the bind for
CLP_STREAM_OUTPUT_DIR_HOST to be read-only (add :ro) for consistency and
improved security; run a quick grep/search and a local integration run to
confirm no writes occur, then update the docker-compose file and any relevant
docs/tests in a follow-up commit.
Member
Author
There was a problem hiding this comment.
the query worker writes to /var/data/streams
Contributor
There was a problem hiding this comment.
@junhaoliao, thank you for the clarification! That makes perfect sense—if the query-worker writes to /var/data/streams, then it correctly needs read-write access, and the current configuration is appropriate. I appreciate you confirming this architectural detail.
✏️ Learnings added
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1690
File: tools/deployment/package/docker-compose-all.yaml:424-427
Timestamp: 2025-12-04T03:31:55.214Z
Learning: In tools/deployment/package/docker-compose-all.yaml, the query-worker service writes to /var/data/streams (CLP_STREAM_OUTPUT_DIR_HOST mount), so this directory must remain read-write and should not be mounted with :ro flag.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1178
File: components/clp-package-utils/clp_package_utils/controller.py:315-315
Timestamp: 2025-10-17T19:59:25.596Z
Learning: In components/clp-package-utils/clp_package_utils/controller.py, worker log directories (compression_worker, query_worker, reducer) created via `mkdir()` do not need `_chown_paths_if_root()` calls because directories are created with the same owner as the script caller. This differs from infrastructure service directories (database, queue, Redis, results cache) which do require explicit ownership changes.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
kirkrodrigues
left a comment
Member
There was a problem hiding this comment.
Changes look good. Can you resolve the conflict?
# Conflicts: # tools/deployment/package/docker-compose-all.yaml
kirkrodrigues
approved these changes
Dec 7, 2025
3 tasks
junhaoliao
added a commit
to junhaoliao/clp
that referenced
this pull request
May 17, 2026
…am directories (fixes y-scope#1689). (y-scope#1690)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Checklist
breaking change.
Validation performed
Then, in the webui, performed search with "1" and observed results returned. Clicked the "Original file" link in any results and observed the log viewer is opened with the log context shown.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.