Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set readOnlyRootFilesystem to true for all containers #637

Closed
timp87 opened this issue May 1, 2023 · 3 comments
Closed

Set readOnlyRootFilesystem to true for all containers #637

timp87 opened this issue May 1, 2023 · 3 comments
Assignees
Labels
enhancement New feature or request TBD To Be Done

Comments

@timp87
Copy link

timp87 commented May 1, 2023

My proposal is to set (hardcode) readOnlyRootFilesystem to true by operator for all containers securityContext. For example prometheus-operator currently deploys the stack with hardcoded readOnlyRootFilesystem in container's securityContext. I. e. it hardcodes this option for all resources deployed withing CRs.

I find this option as a nice security measure and in my experience many companies enforce this setting to be on for all containers.

@f41gh7 f41gh7 added the enhancement New feature or request label May 3, 2023
@f41gh7 f41gh7 self-assigned this May 18, 2023
lvyanru8200 added a commit to lvyanru8200/operator-1 that referenced this issue Jun 27, 2023
set vmalert container SecurityContext the value of ReadOnlyRootFilesystem is set to true

fix: VictoriaMetrics#637

Signed-off-by: fedstate <[email protected]>
@hagen1778 hagen1778 added the TBD To Be Done label Jul 14, 2023
@hagen1778 hagen1778 assigned Haleygo and unassigned f41gh7 Jul 14, 2023
@f41gh7
Copy link
Collaborator

f41gh7 commented Jul 24, 2023

Also it'd be great to add PodSecurityContext https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#set-the-security-context-for-a-pod.

It has really nice feature fsGroupChangePolicy. https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#configure-volume-permission-and-ownership-change-policy-for-pods

By default, Kubernetes recursively changes ownership and permissions for the contents of each 
volume to match the fsGroup specified in a Pod's securityContext when that volume is mounted.
 For large volumes, checking and changing ownership and 
permissions can take a lot of time, slowing Pod startup.

It should simplify managing volume permissions.

cc @Amper @hagen1778

@Haleygo
Copy link
Contributor

Haleygo commented Jul 26, 2023

PodSecurityContext already added in #692.
As for fsGroupChangePolicy: OnRootMismatch, need to do some research on default fs group for pod volume, see if it's necessary here.

Update: FSGroup: 65534 was added as default PodSecurityContext, and kubelet will recursively chown() and chmod() all the files and directories inside the volume when "mount" move is needed[when pod create or restart after kubelet restart(no volumeName/podName combo exists in cache)].

Jul 30 17:41:27 bms-master1 kubelet[1690786]: I0730 17:41:27.759356 1690786 volume_linux.go:111] "Perform recursive ownership change for directory" path="/var/lib/kubelet/pods/06a75c98-9468-4a7c-bd2c-73c1c3458683/volumes/kubernetes.io~csi/pvc-41a4bc23-8cb5-4966-bfcb-8d0f446f4d53/mount"

This will slow down pod startup if volume has many files, like vmstorage. And since pod's fsgroup won't be changed every time, we should set fsGroupChangePolicy: "OnRootMismatch" to skip this like @f41gh7 said and as recommended.

Jul 30 17:50:56 bms-master1 kubelet[1710796]: I0730 17:50:56.926934 1710796 volume_linux.go:54] "Skipping permission and ownership change for volume" path="/var/lib/kubelet/pods/96e7db3f-6900-411b-ad91-46b4c29560ac/volumes/kubernetes.io~csi/pvc-41a4bc23-8cb5-4966-bfcb-8d0f446f4d53/mount"

@Haleygo
Copy link
Contributor

Haleygo commented Aug 25, 2023

Strict SecurityContext will be added by default since v0.36.0 , users can disable it by setting VM_ENABLESTRICTSECURITY=false.
If SecurityContext already defined before for pod or containers, the default Strict SecurityContext won't be applied too.

Close this as completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request TBD To Be Done
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants