Enable readOnlyRootFilesystem for console and download pod#1123
Enable readOnlyRootFilesystem for console and download pod#1123ableischwitz wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ableischwitz The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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. |
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Kubernetes Deployment Manifests bindata/assets/deployments/console-deployment.yaml, bindata/assets/deployments/downloads-deployment.yaml |
Console deployment sets readOnlyRootFilesystem to true; downloads deployment adds emptyDir volume named "tmp" with mount at /tmp. |
Deployment Code Generation pkg/console/subresource/deployment/deployment.go |
Introduces isEmptyDir flag to volumeConfig; adds "tmp" volume entry at /tmp with isEmptyDir flag; updates volume assembly logic to handle EmptyDir volumes. |
Deployment Tests pkg/console/subresource/deployment/deployment_test.go |
Updates test fixtures across TestWithConsoleVolumes and TestDefaultDownloadsDeployment to include tmpVolume and tmpVolumeMount in default and derived volume sets. |
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title directly reflects the main changes: enabling readOnlyRootFilesystem for console and download pods, which is the primary security enhancement across the changeset. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
Tip
Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.
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 help to get the list of available commands and usage tips.
![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
1 participant
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.
The addition should be fairly easy now, as only the
defaultVolumeConfigand the resulting test will have to be adjusted.Summary by CodeRabbit