OTA-1480: Add readonlyRootFilesystem #1208
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@dusk125: This pull request references OTA-1480 which is a valid jira issue. 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-required |
|
/lgtm |
|
/assign @DavidHurta |
pkg/cvo/updatepayload.go
Outdated
| SecurityContext: &corev1.SecurityContext{ | ||
| ReadOnlyRootFilesystem: ptr.To(true), | ||
| }, |
There was a problem hiding this comment.
Should the init containers (L236-L262) have this too? If not, can the commit + pr description contain reasoning for that?.
Or even better, change the setContainerDefaults to default to true so that the right thing is default and whatever needs a false would need to override?
There was a problem hiding this comment.
Yeah it wouldn't hurt to have everything as read only and only turn off what explicitly needs it.
There was a problem hiding this comment.
So looks like the init containers (specifically move-operator-manifests-to-temporary-directory, move-release-manifests-to-temporary-directory) do need to write to the filesystem as they do a file move on the /manifests and /release-manifests directories.
The destination is a volume mount so there's no issue there, but since they're moving the filesystem, they can't delete the files in the source directories as those are built into the container image.
There was a problem hiding this comment.
So I'm thinking I either remove the read only filesystem from those init containers or I change them to copy instead of move and leave read only enabled on all. Do you have a preference @petr-muller ?
openshift-ci
bot
added
lgtm
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
|
/qe-approved |
|
/label qe-approved |
/label qe-approved |
openshift-ci
bot
added
the
qe-approved
|
@dusk125: This pull request references OTA-1480 which is a valid jira issue. 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. |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
@petr-muller if you have a chance to review again, please see my comments about the "move" containers. |
|
/retest |
1 similar comment
|
/retest |
|
/retest |
|
/test e2e-agnostic-operator-devpreview |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dusk125, jacobsee, petr-muller, wking 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 |
|
@dusk125: The following test failed, say
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. |
openshift-merge-bot
bot
merged commit a7d6e43
into
openshift:main
|
[ART PR BUILD NOTIFIER] Distgit: cluster-version-operator |
11 participants
No description provided.