CONSOLE-5138: Enable readOnlyRootFilesystem for console and download pod#1123
CONSOLE-5138: Enable readOnlyRootFilesystem for console and download pod#1123ableischwitz wants to merge 2 commits intoopenshift:mainfrom
Conversation
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @ableischwitz. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details🧰 Additional context used🔀 Multi-repo context openshift/console[::openshift/console::] Findings
Notes / caveats observed while searching
Conclusion
WalkthroughThe changes enforce read-only root filesystems in Kubernetes deployment containers (console and download-server) by setting Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
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)
bindata/assets/deployments/downloads-deployment.yaml (1)
52-59:⚠️ Potential issue | 🟠 Major
download-serverroot filesystem is still writable, so the PR objective is only partially implemented.You added the
/tmpEmptyDir correctly, butreadOnlyRootFilesystemis stillfalse. That leaves the downloads container outside the intended hardening scope.🔧 Proposed fix
securityContext: - readOnlyRootFilesystem: false + readOnlyRootFilesystem: true allowPrivilegeEscalation: false capabilities: drop: - ALLAlso update the corresponding expected value in
pkg/console/subresource/deployment/deployment_test.go(downloadsDeploymentPodSpecSingleReplicasecurityContext) fromutilpointer.Bool(false)toutilpointer.Bool(true).Also applies to: 267-269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindata/assets/deployments/downloads-deployment.yaml` around lines 52 - 59, The download-server container's PodSecurityContext still has readOnlyRootFilesystem: false so make it true: locate the download-server container spec in the deployment YAML and change readOnlyRootFilesystem to true (ensure you keep allowPrivilegeEscalation:false and capabilities.drop: ALL and the /tmp EmptyDir volumeMount). Also update the test expectation named downloadsDeploymentPodSpecSingleReplica in deployment_test.go to use utilpointer.Bool(true) instead of utilpointer.Bool(false) so the unit test matches the hardened manifest; apply the same readOnlyRootFilesystem change where it appears again around the other occurrence (lines referenced in the review).
🧹 Nitpick comments (1)
pkg/console/subresource/deployment/deployment.go (1)
308-315: Enforce exclusive volume source selection to avoid silent overwrite.
isSecret,isConfigMap, andisEmptyDirare documented as mutually exclusive, but the current independentifchain can overwritevols[i]if more than one flag is ever set.♻️ Proposed defensive refactor
- for i, item := range volumeConfig { - if item.isSecret { - vols[i] = corev1.Volume{ - Name: item.name, - VolumeSource: corev1.VolumeSource{ - Secret: &corev1.SecretVolumeSource{ - SecretName: item.name, - }, - }, - } - } - if item.isConfigMap { - var items []corev1.KeyToPath - for key, val := range item.mappedKeys { - items = append(items, corev1.KeyToPath{ - Key: key, - Path: val, - }) - } - vols[i] = corev1.Volume{ - Name: item.name, - VolumeSource: corev1.VolumeSource{ - ConfigMap: &corev1.ConfigMapVolumeSource{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: item.name, - }, - Items: items, - }, - }, - } - } - if item.isEmptyDir { - vols[i] = corev1.Volume{ - Name: item.name, - VolumeSource: corev1.VolumeSource{ - EmptyDir: &corev1.EmptyDirVolumeSource{}, - }, - } - } - } + for i, item := range volumeConfig { + switch { + case item.isSecret && !item.isConfigMap && !item.isEmptyDir: + vols[i] = corev1.Volume{ + Name: item.name, + VolumeSource: corev1.VolumeSource{ + Secret: &corev1.SecretVolumeSource{ + SecretName: item.name, + }, + }, + } + case item.isConfigMap && !item.isSecret && !item.isEmptyDir: + var items []corev1.KeyToPath + for key, val := range item.mappedKeys { + items = append(items, corev1.KeyToPath{Key: key, Path: val}) + } + vols[i] = corev1.Volume{ + Name: item.name, + VolumeSource: corev1.VolumeSource{ + ConfigMap: &corev1.ConfigMapVolumeSource{ + LocalObjectReference: corev1.LocalObjectReference{Name: item.name}, + Items: items, + }, + }, + } + case item.isEmptyDir && !item.isSecret && !item.isConfigMap: + vols[i] = corev1.Volume{ + Name: item.name, + VolumeSource: corev1.VolumeSource{ + EmptyDir: &corev1.EmptyDirVolumeSource{}, + }, + } + default: + klog.Warningf("invalid volumeConfig for %q: exactly one source type must be set", item.name) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/console/subresource/deployment/deployment.go` around lines 308 - 315, The current independent checks for item.isSecret, item.isConfigMap, and item.isEmptyDir can silently overwrite vols[i]; change this to enforce exclusivity by replacing the independent ifs with either an if/else-if chain or a validation that detects when more than one of item.isSecret, item.isConfigMap, item.isEmptyDir is true and returns an error (or logs and skips) before assigning vols[i]; ensure the assignment to vols[i] (corev1.Volume with the appropriate VolumeSource) only happens once for the selected type to avoid silent overwrites.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@bindata/assets/deployments/downloads-deployment.yaml`:
- Around line 52-59: The download-server container's PodSecurityContext still
has readOnlyRootFilesystem: false so make it true: locate the download-server
container spec in the deployment YAML and change readOnlyRootFilesystem to true
(ensure you keep allowPrivilegeEscalation:false and capabilities.drop: ALL and
the /tmp EmptyDir volumeMount). Also update the test expectation named
downloadsDeploymentPodSpecSingleReplica in deployment_test.go to use
utilpointer.Bool(true) instead of utilpointer.Bool(false) so the unit test
matches the hardened manifest; apply the same readOnlyRootFilesystem change
where it appears again around the other occurrence (lines referenced in the
review).
---
Nitpick comments:
In `@pkg/console/subresource/deployment/deployment.go`:
- Around line 308-315: The current independent checks for item.isSecret,
item.isConfigMap, and item.isEmptyDir can silently overwrite vols[i]; change
this to enforce exclusivity by replacing the independent ifs with either an
if/else-if chain or a validation that detects when more than one of
item.isSecret, item.isConfigMap, item.isEmptyDir is true and returns an error
(or logs and skips) before assigning vols[i]; ensure the assignment to vols[i]
(corev1.Volume with the appropriate VolumeSource) only happens once for the
selected type to avoid silent overwrites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 54fa7746-24cd-4555-b144-04cd547bb25f
📒 Files selected for processing (4)
bindata/assets/deployments/console-deployment.yamlbindata/assets/deployments/downloads-deployment.yamlpkg/console/subresource/deployment/deployment.gopkg/console/subresource/deployment/deployment_test.go
jhadvig
added
ok-to-test
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ableischwitz, jhadvig The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
QE Approver: |
ableischwitz
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@ableischwitz: This pull request references CONSOLE-5138 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 story to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
|
/label px-approved |
openshift-ci
bot
added
the
px-approved
|
Checked on cluster launched against the pr, the 'readOnlyRootFilesystem' in console deployment was set true, 'readOnlyRootFilesystem' in console deployment was set false: /verified by yanpzhan |
openshift-ci-robot
added
the
verified
|
@yanpzhan: This PR has been marked as verified by DetailsIn 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-robot
removed
the
verified
|
New changes are detected. LGTM label has been removed. |
|
@ableischwitz: This pull request references CONSOLE-5138 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 story to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.coderabbit.yaml (1)
16-18: Avoid blanket exclusion of all Go tests from review scope.Line 18 excludes every
*_test.gofile, which can hide important regression/safety signals in PRs that rely on test updates. Prefer excluding only noisy test paths instead of all Go tests.Suggested adjustment
path_filters: - "!vendor/**" - - "!**/*_test.go"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.coderabbit.yaml around lines 16 - 18, The path_filters currently blanket-excludes all Go tests via the pattern "!**/*_test.go" which hides important test changes; update the configuration by removing or narrowing that pattern in the path_filters section (leave the vendor exclusion "!vendor/**" as-is) and replace the blanket test exclusion with targeted exclusions for known noisy test directories or file patterns (e.g., specific fixtures or generated-test folders) so that most *_test.go files remain in review scope; refer to the path_filters entry and the "!**/*_test.go" pattern to locate and change this.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.coderabbit.yaml:
- Around line 16-18: The path_filters currently blanket-excludes all Go tests
via the pattern "!**/*_test.go" which hides important test changes; update the
configuration by removing or narrowing that pattern in the path_filters section
(leave the vendor exclusion "!vendor/**" as-is) and replace the blanket test
exclusion with targeted exclusions for known noisy test directories or file
patterns (e.g., specific fixtures or generated-test folders) so that most
*_test.go files remain in review scope; refer to the path_filters entry and the
"!**/*_test.go" pattern to locate and change this.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 104d4cf6-ed06-4757-9817-152fe5a6341d
⛔ Files ignored due to path filters (3)
pkg/console/subresource/configmap/configmap_test.gois excluded by!**/*_test.gopkg/console/subresource/configmap/tech_preview_test.gois excluded by!**/*_test.gopkg/console/subresource/deployment/deployment_test.gois excluded by!**/*_test.go
📒 Files selected for processing (9)
.coderabbit.yamlDockerfile.ocpbindata/assets/deployments/downloads-deployment.yamlpkg/console/operator/operator.gopkg/console/operator/sync_v400.gopkg/console/starter/starter.gopkg/console/subresource/configmap/configmap.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/consoleserver/types.go
💤 Files with no reviewable changes (3)
- pkg/console/operator/sync_v400.go
- pkg/console/starter/starter.go
- pkg/console/subresource/configmap/configmap.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
pkg/**/*.go
⚙️ CodeRabbit configuration file
pkg/**/*.go: Review Go code following OpenShift operator patterns.
See CONVENTIONS.md for coding standards and patterns.
Controllers should use the library-go factory pattern.
Status conditions should use status.Handle* functions.
Files:
pkg/console/operator/operator.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/consoleserver/types.go
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/console/operator/operator.goDockerfile.ocppkg/console/subresource/consoleserver/config_builder.gobindata/assets/deployments/downloads-deployment.yamlpkg/console/subresource/consoleserver/types.go
bindata/assets/**/*.yaml
⚙️ CodeRabbit configuration file
bindata/assets/**/*.yaml: Review YAML assets for Kubernetes resource correctness.
Ensure proper annotations for cluster profiles.
Files:
bindata/assets/deployments/downloads-deployment.yaml
🔀 Multi-repo context openshift/console
[::openshift/console::]
-
downloads server writes files to a temp dir at runtime:
- cmd/downloads/main.go: creates DownloadsServerConfig and defers os.RemoveAll(downloadsServerConfig.TempDir). (cmd/downloads/main.go:31-34) — consumer behavior requires a writable filesystem for TempDir inside the container.
- cmd/downloads uses downloadsServerConfig.TempDir as the file server root (cmd/downloads/main.go:39).
- downloads config implementation manipulates/creates directories under DownloadsServerConfig.TempDir (cmd/downloads/config/downloads_config.go:275–316) — explicit directory creation and file writes occur at runtime.
-
Docker image and packaging for downloads:
- Dockerfile.downloads: installs binaries and places defaultArtifactsConfig.yaml under /opt/downloads; CMD runs the downloads binary that serves TempDir (Dockerfile.downloads:18–28). If the container root is read-only, the downloads process still needs a writable path for TempDir (not /opt/downloads).
-
Kubernetes API types confirm fields used:
- vendor/k8s.io/api/core/v1/types.go and generated proto/docs include ReadOnlyRootFilesystem and EmptyDir volume types (vendor/k8s.io/api/core/v1/types.go:8158 and generated.proto). These show the API fields the PR modifies are standard Kubernetes fields.
-
Repo contains numerous examples of emptyDir usage in front-end/integration-test fixtures (frontend/**: various files referencing emptyDir), indicating emptyDir is an established pattern in this repo for ephemeral writable storage.
Implication backed by observed files: the downloads server requires a writable TempDir at runtime (writes HTML and artifacts into TempDir). To enable readOnlyRootFilesystem for the downloads container you must mount an emptyDir (or other writable volume) at the path used for TempDir. The repository contains Dockerfile.downloads and cmd/downloads/* that show where writes occur and where a mount is needed.
🔇 Additional comments (7)
Dockerfile.ocp (1)
1-1: Version bump is consistent across build and runtime stages.No blocking concerns here; keeping both stages on the same OCP minor is a clean update.
Also applies to: 8-8
bindata/assets/deployments/downloads-deployment.yaml (2)
52-59: Read-only root filesystem is correctly paired with writable/tmp.This hardening is implemented safely: startup writes to
/tmpremain functional via the new mount.
267-269:emptyDirvolume wiring is correct.
tmpis declared at pod level and matches the containervolumeMount, which is the right pattern for ephemeral writable space..coderabbit.yaml (3)
1-4: Good config modernization and verbosity setup.The language, inheritance enablement, and review detail visibility changes look clean and consistent.
Also applies to: 15-15
48-53: Code-guideline file pattern coverage looks good.Including
AGENTS.md,CLAUDE.md,ARCHITECTURE.md,CONVENTIONS.md, andTESTING.mdshould improve review context quality.
54-62: Strong linked-repository guidance.This instruction is specific and should help catch cross-repo config-structure drift between operator and console.
pkg/console/subresource/consoleserver/types.go (1)
20-35: The concern about wire-format compatibility is incorrect. The operator'sconfig_builder.gonever populatedContentSecurityPolicyEnabledwhen constructing theConfigstruct (onlyContentSecurityPolicyis set), so console was already receiving this field as missing/false. Removing the field from the schema simply aligns it with what was actually being emitted. No code found in either repository reads or depends oncontentSecurityPolicyEnabled, so this removal does not introduce a compatibility issue with any supported console version.> Likely an incorrect or invalid review comment.
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@ableischwitz: This pull request references CONSOLE-5138 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 story to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
|
/retest |
|
@ableischwitz @jhadvig Checked against latest code, now downloads deployment also set "readOnlyRootFilesystem" as 'true': |
|
/retest-required |
|
@ableischwitz: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/verified by yanpzhan |
openshift-ci-robot
added
the
verified
|
@yanpzhan: This PR has been marked as verified by DetailsIn 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. |
5 participants
This pull request addresses the enablement of
readOnlyRootFilesystemfor theconsoleanddownloadpods managed by theopenshift-console-operator.In order to allow the
downloadpod to save it's Python-script to/tmpan emptyDir for this directory was added as a volume. Same for theconsolepod.While the console and client-downloads work as expected, there may be an issue with dynamic-plugins. They may need to get their own dedicated volumes, which location I wasn't able to identify. ---> This was affecting only plugins which do version check the console. The example deployment lists the version as "0.0.1-snapshot" and this wasn't sufficient. Therefore no issues seen with ReadOnlyRootfilesystem enabled visible any more.
The addition should be fairly easy now, as only the
defaultVolumeConfigand the resulting test will have to be adjusted.Summary by CodeRabbit