MGMT-22078: Add readOnlyRootFilesystem to assisted-service template #8229
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rivkyrizel: This pull request references MGMT-22079 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
size/XS
WalkthroughAdds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rivkyrizel The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
rivkyrizel
changed the title
|
@rivkyrizel: This pull request references MGMT-22078 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
rivkyrizel
force-pushed
the
MGMT-22079
branch
from
c45a047 to
6590750
Compare
rivkyrizel
force-pushed
the
MGMT-22079
branch
from
6590750 to
b56a036
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8229 +/- ##
=======================================
Coverage 43.30% 43.30%
=======================================
Files 405 405
Lines 70678 70678
=======================================
Hits 30604 30604
Misses 37333 37333
Partials 2741 2741 🚀 New features to boost your workflow:
|
rivkyrizel
force-pushed
the
MGMT-22079
branch
from
b56a036 to
2919f61
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
config/manager/manager.yaml(1 hunks)deploy/assisted-image-service.yaml(1 hunks)deploy/assisted-service.yaml(1 hunks)openshift/template.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- openshift/template.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/manager/manager.yamldeploy/assisted-image-service.yamldeploy/assisted-service.yaml
🔇 Additional comments (2)
deploy/assisted-service.yaml (1)
27-28: Verify assisted-service functions without filesystem write access.With
readOnlyRootFilesystem: trueand all volumeMounts markedreadOnly: true(lines 137–147), the container has no writable filesystem access. This includes the root filesystem,/tmp, and all mounted paths. If the application attempts to write logs, create runtime files, or cache data locally, the container will fail.Confirm that the assisted-service application:
- Writes logs and metrics only to stdout/stderr or external systems
- Does not create or modify files on the local filesystem
- Does not require temporary storage or working directories within the container
- Has been tested in production or CI with this security constraint
If the application requires writable storage, consider adding an
emptyDirmount for temporary data or logs.config/manager/manager.yaml (1)
59-61: Security context hardening looks good for the operator.Adding
readOnlyRootFilesystem: truealigns with cloud-native operator patterns where state is managed externally via Kubernetes objects (CRDs, ConfigMaps, Secrets) rather than local filesystem.
| securityContext: | ||
| readOnlyRootFilesystem: true |
There was a problem hiding this comment.
🧩 Analysis chain
Verify assisted-image-service compatibility with read-only root filesystem.
With readOnlyRootFilesystem: true, the container can only write to the explicitly mounted /data volume (lines 61–63). If the application attempts to write temporary files or logs to root filesystem paths (e.g., /tmp, /var/tmp, or other standard directories), the container will fail at runtime.
Confirm that the assisted-image-service application:
- Does not attempt to write outside the
/datamount point - Does not require a writable
/tmpor other standard temp directories - Has been tested or validated to function with this security constraint
🏁 Script executed:
#!/bin/bash
# Description: Search for write operations and temp directory usage in the assisted-image-service source code.
# Look for common temp directory references and write operations
rg -i "(tmp|tempdir|temp_|stdout|stderr|log)" --type=go -C2 | head -100Length of output: 6771
🏁 Script executed:
#!/bin/bash
# Find the assisted-image-service main package and entry point
# Look for main.go or main package in the codebase
fd -e go -type f | xargs rg -l "func main\(\)" | grep -E "(cmd|main)" | head -20Length of output: 297
🏁 Script executed:
#!/bin/bash
# Search for file operations in non-test code that might write to filesystem
# Look for os.Create, os.WriteFile, ioutil.WriteFile in non-test code
rg "(os\.Create|os\.WriteFile|ioutil\.WriteFile|os\.Mkdir|ioutil\.TempFile)" \
--type=go -g '!**/test/**' -g '!**/*_test.go' -C2 | head -150Length of output: 9733
🏁 Script executed:
#!/bin/bash
# Look for logging setup that might require writable filesystem
rg -i "(log\.setup|logger|logrus|zap|slog)" --type=go -g '!**/test/**' -g '!**/*_test.go' -C1 | head -100Length of output: 8705
🏁 Script executed:
#!/bin/bash
# Check the deployment YAML to understand what volumes are mounted and what app expects
cat deploy/assisted-image-service.yaml | head -100Length of output: 1893
🏁 Script executed:
#!/bin/bash
# Search for any code that references /tmp, /var, or other standard paths
rg '"/tmp|/var|/run|/dev' --type=go -g '!**/test/**' -g '!**/*_test.go' -C2Length of output: 211625
Verify and fix assisted-image-service compatibility with read-only root filesystem.
The code reveals a critical configuration issue: internal/bminventory/inventory.go defines ISOCacheDir with a default value of "/tmp/isocache". With readOnlyRootFilesystem: true and only /data writable, any attempt to write temporary files to /tmp will fail at runtime. Additionally, code in internal/oc/release.go uses os.CreateTemp("", "") which defaults to system temp directories.
Required actions:
- Configure
ISO_CACHE_DIRenvironment variable to point to/dataor a subdirectory - Add explicit writable volume mounts (tmpfs for
/tmp,/var/tmp) if necessary for application functionality - Test the container startup and health checks to confirm it functions with the security constraint applied
🤖 Prompt for AI Agents
In deploy/assisted-image-service.yaml around lines 31-32, the manifest enables
readOnlyRootFilesystem but the app defaults to writing to /tmp and
/tmp/isocache; update the Deployment to set an environment variable
ISO_CACHE_DIR=/data/isocache (or similar under /data) and ensure the container
has a writable volume mounted at /data (create the volume and corresponding
volumeMount), and either mount writable tmpfs volumes for /tmp and /var/tmp or
set TMPDIR=/data/tmp and mount /data/tmp writable; after making these changes,
create the target directories in the container init or via an emptyDir volume
with appropriate medium if needed, then test container startup and health checks
to confirm operation with readOnlyRootFilesystem enabled.
There was a problem hiding this comment.
I don't think ISOCacheDir is actually used anywhere.
That said, this is failing because we need an additional volume for the image service
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify ISOCacheDir is not used by assisted-image-service
# Find image service directory/package
fd -t d -i image.service | head -5
# Search for ISOCacheDir references
echo "=== Searching for ISOCacheDir usage ==="
rg "ISOCacheDir" --type=go
# Find the image service main package
echo -e "\n=== Finding image service entry point ==="
fd main.go | xargs rg -l "image.service|image-service" | head -10
Length of output: 382
🏁 Script executed:
#!/bin/bash
# Search for file operations in image service code
# Find image service code
echo "=== Finding image service source ==="
fd -t d -g "*image*service*" -g "*imageservice*" | grep -v vendor | grep -v test
# Look for os package imports and temp file operations
echo -e "\n=== Searching for file writes in image service ==="
rg "(os\.Create|os\.WriteFile|ioutil\.WriteFile|os\.Mkdir|ioutil\.TempFile|ioutil\.TempDir|os\.MkdirTemp|os\.CreateTemp)" \
--type=go -g "*image*service*" -g "*imageservice*" -C3
Length of output: 190
🏁 Script executed:
#!/bin/bash
# Check for /tmp references in image service code
echo "=== Searching for /tmp usage in image service ==="
rg '"/tmp|filepath\.Join.*tmp' --type=go -g "*image*service*" -g "*imageservice*" -C2
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Look at other similar deployments to see what volumes they have
echo "=== Checking assisted-service.yaml for volume configuration ==="
cat deploy/assisted-service.yaml | grep -A30 "volumes:" | head -40
echo -e "\n=== Checking template.yaml for volume configuration ==="
cat openshift/template.yaml | grep -A30 "volumes:" | head -50
Length of output: 1447
🏁 Script executed:
#!/bin/bash
# Find the image service source code location more carefully
echo "=== Finding image service package ==="
fd -e go -t f | xargs rg -l "package.*image.*service" | head -10
echo -e "\n=== Looking for imageservice imports ==="
rg "import.*imageservice|\".*imageservice\"" --type=go -l | head -10
echo -e "\n=== Searching for cmd/main.go references to image service ==="
rg -A5 -B5 "assisted-image-service|IMAGE.SERVICE|ImageService" cmd/main.go
Length of output: 4845
🏁 Script executed:
#!/bin/bash
# Check what main.go actually runs for image service vs regular service
echo "=== Checking main.go for service type determination ==="
cat cmd/main.go | grep -A10 -B10 "image.*service\|SERVICE_TYPE"
Length of output: 1020
🏁 Script executed:
#!/bin/bash
# Look for the workdir volume mount in template.yaml to understand usage
echo "=== Checking workdir-volume usage in template.yaml ==="
rg -A10 -B5 "workdir-volume|workdir" openshift/template.yaml
Length of output: 636
🏁 Script executed:
#!/bin/bash
# Check if there are any CI test logs or documentation about volumes needed
echo "=== Searching for documentation or comments about volumes ==="
rg -i "volume|mount|tmpfs|emptydir" deploy/assisted-image-service.yaml -C5
echo -e "\n=== Checking for any README or docs about image service ==="
fd -e md -t f | xargs rg -l "image.service" | head -5
Length of output: 743
🏁 Script executed:
#!/bin/bash
# Check the assisted-image-service external package for what it needs
echo "=== Looking for assisted-image-service usage patterns ==="
rg "github.com/openshift/assisted-image-service" cmd/main.go -A20 -B5
echo -e "\n=== Check what WORK_DIR is set to in template.yaml ==="
rg "WORK_DIR" openshift/template.yaml -C3
Length of output: 2374
🏁 Script executed:
#!/bin/bash
# Look for tmp or temp directory usage patterns that would require writable filesystem
echo "=== Checking if image service needs /tmp ==="
rg "os\.TempDir|ioutil\.TempDir|os\.MkdirTemp|/tmp" --type=go -g "!*_test.go" | grep -i image | head -30
echo -e "\n=== Check for any environment variables related to temp dirs ==="
rg "TMPDIR|TEMP_DIR|TMP_DIR|WORK_DIR" deploy/assisted-image-service.yaml -C2
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Compare with other deployments that have readOnlyRootFilesystem
echo "=== Check if template.yaml has tmpfs or emptyDir for /tmp ==="
cat openshift/template.yaml | grep -A50 "readOnlyRootFilesystem" | head -60
echo -e "\n=== Check assisted-service.yaml for comparison ==="
cat deploy/assisted-service.yaml | grep -A50 "readOnlyRootFilesystem" | head -60
Length of output: 3869
🏁 Script executed:
#!/bin/bash
# Look for any existing issues or PRs that might mention this
echo "=== Check git log for related changes ==="
git log --oneline --all -20 | grep -i "readonly\|volume\|image.service"
echo -e "\n=== Check recent commits to these deployment files ==="
git log --oneline -10 -- deploy/assisted-image-service.yaml
Length of output: 695
🏁 Script executed:
#!/bin/bash
# Check what the assisted-image-service external package might need
echo "=== Check if there's a go.mod reference to assisted-image-service version ==="
rg "assisted-image-service" go.mod
echo -e "\n=== Look for any documentation about image service requirements ==="
rg -i "image.service" docs/ -C3 | grep -i "volume\|mount\|write\|tmp\|data" | head -20
Length of output: 2475
🏁 Script executed:
#!/bin/bash
# Look at the PR that added the /data volume to understand what it was for
echo "=== Check the commit that added emptyDir for data in image service ==="
git show dbe1decf --stat
echo -e "\n=== Show the actual changes in that commit ==="
git show dbe1decf -- deploy/assisted-image-service.yaml
Length of output: 1162
🏁 Script executed:
#!/bin/bash
# Check if CI is failing and what the error is
echo "=== Look for test configurations or CI scripts that might show failures ==="
fd -e yaml -e yml ci test | head -10
echo -e "\n=== Check if there are any deployment test scripts ==="
fd deploy -e sh | head -10
Length of output: 409
🏁 Script executed:
#!/bin/bash
# Look for patterns in how other services handle readOnlyRootFilesystem with volumes
echo "=== Check all volumeMounts patterns in deployment files ==="
rg "volumeMounts:" -A10 deploy/*.yaml | head -80
echo -e "\n=== Check if there's a /tmp mount pattern anywhere ==="
rg "mountPath.*tmp|path.*tmp" deploy/*.yaml openshift/*.yaml config/**/*.yaml
Length of output: 1420
🏁 Script executed:
#!/bin/bash
# Check the assisted-image-service external package to understand what directories it might write to
echo "=== Check go.sum for the exact assisted-image-service version details ==="
rg "assisted-image-service.*c402402f52bf" go.sum
echo -e "\n=== Search for any server initialization or startup code that might use tmp ==="
rg "func.*[Ss]erver|func.*[Ss]tart|func main" cmd/main.go -A30 | grep -i "tmp\|temp\|work\|data" | head -20
Length of output: 514
🏁 Script executed:
#!/bin/bash
# Look for what the external assisted-image-service package might need in terms of writable paths
# Since it's an external package, look for any integration points or documentation
echo "=== Check if there's any configuration for image service paths ==="
rg "IMAGE_SERVICE|image.service" deploy/assisted-image-service.yaml -C5
echo -e "\n=== Check what environment variables are passed to image service ==="
rg "env:" -A30 deploy/assisted-image-service.yaml
Length of output: 2271
🏁 Script executed:
#!/bin/bash
# Check if there are any recent CI failures or test logs mentioning this
echo "=== Look for CI configuration that might show test failures ==="
rg "assisted-image-service" .github/ -C3 | head -40
echo -e "\n=== Check for any Makefile targets related to image service deployment ==="
rg "image.service" Makefile | head -20
Length of output: 595
@carbonin Thank you for the correction! You're right that ISOCacheDir is only used by the main assisted-service, not the assisted-image-service. I apologize for the confusion in my analysis.
Since the deployment is failing with readOnlyRootFilesystem: true, could you clarify what specific volume or mount path the image service needs? Based on common patterns for containers with read-only root filesystems, I suspect it might need:
A writable /tmp directory - which would typically be mounted as a tmpfs emptyDir:
volumeMounts:
- mountPath: /data
name: image-service-data
- mountPath: /tmp
name: tmp-volume
volumes:
- emptyDir: {}
name: image-service-data
- emptyDir:
medium: Memory
name: tmp-volumeIs /tmp the missing volume, or is it a different path that the assisted-image-service needs writable access to?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look up PR #8361 to understand what it does
gh pr view 8361 --json title,body,state,url,number
Length of output: 1892
@carbonin Thank you for opening #8361 to clean up the unused ISO_CACHE_DIR variable! That makes sense given it hasn't been used since 2021.
I apologize for the confusion in my initial analysis about ISOCacheDir affecting the assisted-image-service. Looking at the current state of this PR, I can see the actual fix is already in place with:
DATA_TEMP_DIRenvironment variable pointing to/data_temp- A dedicated
data-temp-volumeemptyDir mount at/data_temp
This should provide the writable directory needed for the image service to function properly with readOnlyRootFilesystem: true. Is there anything else needed for this PR, or does the current configuration address the volume requirements you mentioned?
|
@rivkyrizel: This pull request references MGMT-22078 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
rivkyrizel
force-pushed
the
MGMT-22079
branch
from
2919f61 to
3cb0e0f
Compare
openshift-ci
bot
added
size/S
rivkyrizel
force-pushed
the
MGMT-22079
branch
from
3cb0e0f to
9d7f299
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deploy/assisted-image-service.yaml (1)
31-32: Harden the security context with runAsNonRoot and allowPrivilegeEscalation.The
securityContextis incomplete. In addition toreadOnlyRootFilesystem: true, addallowPrivilegeEscalation: falseand considerrunAsNonRoot: truewith a specificrunAsUserto further reduce attack surface and align with OpenShift hardening best practices.Apply this diff to harden the security context:
securityContext: readOnlyRootFilesystem: true + allowPrivilegeEscalation: false + runAsNonRoot: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
config/manager/manager.yaml(1 hunks)deploy/assisted-image-service.yaml(2 hunks)deploy/assisted-service.yaml(1 hunks)deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml(1 hunks)openshift/template.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- config/manager/manager.yaml
- deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml
- openshift/template.yaml
- deploy/assisted-service.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
deploy/assisted-image-service.yaml
🪛 Checkov (3.2.334)
deploy/assisted-image-service.yaml
[medium] 9-73: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 9-73: Minimize the admission of root containers
(CKV_K8S_23)
🔇 Additional comments (1)
deploy/assisted-image-service.yaml (1)
31-32: Verify that assisted-image-service uses the DATA_TEMP_DIR environment variable.The changes add
readOnlyRootFilesystem: truewith a corresponding/data_tempvolume, but this only works if the assisted-image-service application actually reads and respects theDATA_TEMP_DIRenvironment variable (lines 61–62). Previous reviews flagged that the deployment was failing withreadOnlyRootFilesystemenabled, suggesting the application needed configuration changes to write to a writable volume.Confirm that:
- The assisted-image-service application code (or the external package it depends on) reads the
DATA_TEMP_DIRenvironment variable and uses it for temporary files- The deployment has been tested and successfully starts/runs with these changes
- If the external package doesn't yet support this env var, verify that package changes have been made and released
You can verify this by checking the assisted-image-service source code or package documentation for
DATA_TEMP_DIRsupport, or by running the deployment in a test environment.
|
@rivkyrizel: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Add
readOnlyRootFilesystem: trueto container specs in the assisted-service to comply with OpenShift security and hardening guidelines.Applied across all relevant configurations (CI, SaaS, and ACM) to ensure consistent security behavior in every deployment environment.
Containers that cannot support a read-only root filesystem include a clear technical justification.
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist