Allow pod/container overrides for volumes/volumemounts

[dkwon17]

What does this PR do?

Allows pod overrides for spec.volumes and container overrides for spec.containers[*].volumeMounts.

What issues does this PR fix or reference?

eclipse-che/che#22751

Is it tested? How?

Checkout this PR and install DWO on a fresh cluster with Run controller locally. Switch to the devworkspace-controller namespace by running oc project devworkspace-controller

Mounting secrets to workspace container using overrides

1. Create secret to mount:

kubectl apply -f - <<EOF
apiVersion: v1
kind: Secret
metadata:
  name: dotfile-secret
data:
  .secret-file: dmFsdWUtMg0KDQo=
EOF

2. Create a devworkspace that uses pod-overrides and container-overrides to mount the secret:

curl https://gist.githubusercontent.com/dkwon17/4e698c3dcf6e36fc474eb73f5c20ac7c/raw/e7e4cf11d2081ced4b69e9cf9a230a2c8c30cf0c/che-code-secret-override.yaml | kubectl apply -f -

3. Wait until the DevWorkspace's status becomes 'Running'.
4. Check that the volume has been added in the workspace pod. In this case, we should see secret-volume in the list of volumes:

$ oc get pod <podname> -o=jsonpath='{.spec.volumes[*].name}'
secret-volume checode projects workspace-metadata kube-api-access-hcdjv

5. Check that the volumeMount has been added in the workspace pod's tools container:

$oc get pod <podname> -o=jsonpath='{.spec.containers[1].volumeMounts[*].name}'
secret-volume projects workspace-metadata kube-api-access-hcdjv

6. Run the following in the workspace tools container to check that the secret has been mounted:

$ cat /etc/secret-volume/.secret-file 
value-2

7. Stop the workspace and update the storage-type to per-user:

oc patch dw secret-volume-override --type merge -p '{"spec":{"template":{"att
ributes":{"controller.devfile.io/storage-type":"per-user"}}}}'

8. Start the workspace, and recheck steps 3-6.
9. Stop the workspace and update the storage-type to per-workspace, start the workspace and recheck steps 3-6 again.

The feature also works for adding PVC volumes:

      pod-overrides:
        spec:
          volumes:
            - name: my-pv
              persistentVolumeClaim:
                claimName: my-pvc

Caveat of this feature

When introducing a new volumes using pod-overrides, the new volume must contain all fields that Kubernetes may add. For example, pods that specify configmap and secret volumes have a defaultMode: 420 added to the yaml even if it's not originally provided in the pod's yaml.

Therefore when introducing a new configmap/secret volumes using pod-overrides, the defaultMode field must be provided along with the configmap/secret details.

For example:

      pod-overrides:
        spec:
          volumes:
            - name: secret-volume
              secret:
                secretName: dotfile-secret
                defaultMode: 420

If the defaultMode is missing in the pod overrides, the DevWorkspace will never reach the Running state because the new volume in the pod will have the defaultMode: 420 added by default. At the same time, the DWO will try to remove defaultMode if it doesn't exist in pod overrides. To clarify, from my testing this only happens when introducing new volumes through pod-overrides. This logic causes reconciliation until workspace start fails with this message:

The workspace status remains "Starting" in the last 300 seconds.

PR Checklist

• E2E tests pass (when PR is ready, comment /test v8-devworkspace-operator-e2e, v8-che-happy-path to trigger)
  • v8-devworkspace-operator-e2e: DevWorkspace e2e test
  • v8-che-happy-path: Happy path for verification integration with Che

[amisevsk]

Generally looks good, I have a few comments

Nice work 👍

[dkwon17]

Thank you for the review @amisevsk , I've updated the PR

[codecov]

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (8125d08) 52.74% compared to head (7f1f26f) 52.52%.
Report is 4 commits behind head on main.

Files	Patch %	Lines
pkg/library/overrides/pods.go	26.08%	17 Missing ⚠️
pkg/provision/storage/commonStorage.go	14.28%	4 Missing and 2 partials ⚠️
pkg/provision/storage/perWorkspaceStorage.go	14.28%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1223      +/-   ##
==========================================
- Coverage   52.74%   52.52%   -0.22%     
==========================================
  Files          84       84              
  Lines        7616     7636      +20     
==========================================
- Hits         4017     4011       -6     
- Misses       3310     3333      +23     
- Partials      289      292       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[amisevsk]

LGTM 👍

Thanks @dkwon17!

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, dkwon17

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [amisevsk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[amisevsk]

Regarding the merge key for volumeMounts being mountPath -- we may in the future want to add some guardrails for which volumes/mounts can be overridden (e.g. you can't override default ones? I'm not sure here).

[openshift-ci]

New changes are detected. LGTM label has been removed.

[amisevsk]

@dkwon17 One somewhat annoying thing in this repo: all commits need a signoff before merge (including fixup commits that would be squashed when merging). You'll need to squash fixups manually (or sign off on them) before we can merge this PR.

[dkwon17]

@amisevsk I've squashed the GitHub fixup commits and signed it off.

we may in the future want to add some guardrails for which volumes/mounts can be overridden (e.g. you can't override default ones? I'm not sure here)

I will make a PR to update the pod/container overrides documentation removing the mention about volume/mounts being non-overridable. Maybe I can add a sentence about not overriding default volumes/mounts? Nothing at the moment prevents a user from doing this however.

[amisevsk]

Maybe I can add a sentence about not overriding default volumes/mounts? Nothing at the moment prevents a user from doing this however.

That would be great -- even just a warning that you can override built-in volume mounts and should avoid doing it would be useful.