rancher / local-path-provisioner

Dynamically provisioning persistent local storage with Kubernetes
Apache License 2.0
2.07k stars 439 forks source link

Issue with the helper pod security context being overwritten #420

Closed js185692 closed 2 weeks ago

js185692 commented 3 weeks ago

A change made in v0.0.27 (https://github.com/rancher/local-path-provisioner/pull/402) has introduced an issue where the security context for the helper pod has been hard coded into the provisioner, overwriting any configuration that might be made in the configmap.

helperPod.Spec.Containers[0].SecurityContext = &v1.SecurityContext{
    SELinuxOptions: &v1.SELinuxOptions{
        Level: "s0-s0:c0.c1023",
    },
}

It's not clear to me why the above change was needed to be made in the provisioner? It seems to me that the same could have been achieved with the following config;

kind: ConfigMap
apiVersion: v1
metadata:
  name: local-path-config
  namespace: local-path-storage
data:
...
  helperPod.yaml: |-
    apiVersion: v1
    kind: Pod
    metadata:
      name: helper-pod
    spec:
      priorityClassName: system-node-critical
      tolerations:
        - key: node.kubernetes.io/disk-pressure
          operator: Exists
          effect: NoSchedule
      containers:
      - name: helper-pod
        image: busybox
        imagePullPolicy: IfNotPresent
        securityContext:
            seLinuxOptions:
                level: s0-s0:c0.c1023
js185692 commented 3 weeks ago

Hi @derekbit, I'd appreciate it if this could be addressed ASAP.

derekbit commented 3 weeks ago

@js185692 I'm thinking we can make seLinuxOptions configurable instead. WDYT? Yeah, your proposal looks a good idea. I think we can address it soon.

js185692 commented 3 weeks ago

@derekbit thanks for the quick reply. Yeah, I was thinking that you could probably just revert that PR and the person who opened the original PR can set the security context via the ConfigMap instead.

derekbit commented 3 weeks ago

@derekbit thanks for the quick reply. Yeah, I was thinking that you could probably just revert that PR and the person who opened the original PR can set the security context via the ConfigMap instead.

Agree with the opinion. I will revert it and release it soon. Thanks for raising the issue. What we can improve the e2e test is to SELinux related parts. SELinux environment is not tested before.

js185692 commented 3 weeks ago

@derekbit I saw that the commit has been reverted, could I get an ETA on the next release please? Thanks.

derekbit commented 3 weeks ago

@js185692 I think we can release it next week if lucky. The CI doesn't work for now because of the configuration. Our IT is working on it. Ref: https://github.com/rancherlabs/eio/issues/2398

derekbit commented 2 weeks ago

@js185692 Checking the manifest and helm chart. If no surprise, I will release v0.0.28 tomorrow.

derekbit commented 2 weeks ago

0.0.28 is released.