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

Check edge cases for automounted configmaps/secrets #495

Merged
merged 6 commits into from
Jul 27, 2021

Conversation

amisevsk
Copy link
Collaborator

What does this PR do?

Checks some edge cases in automounted secrets and volumes that can cause workspace startup to hang

  1. Fail startup if the deployment we're creating is invalid
  2. Check for collision in automounted volumes:
    • automounted volume collides with DevWorkspace volume on name
    • automounted volume collides with DevWorkspace volume on mountPath
    • automounted volumes collide on mountPath

Also prefixes automounted volumes with automount-[secret|configmap] to avoid collisions with each other and with DevWorkspace volumes (e.g. you can't automount a configmap named "project" currently).

Note: prefixing volume names as above can lead to issues, so it may be worth it to drop the last commit in this PR. If a secret/configmap's name is very long and the prefix pushes it over 63 chars, workspace start will fail.

What issues does this PR fix or reference?

Minor issue I happened to run into while checking #455 where my secrets shared a mountPath and no error was shown (except in logs)

Is it tested? How?

Here comes a long testing doc:

  1. Store the DevWorkspace we'll use for testing in /tmp/
    cat > /tmp/test-dw.yaml <<EOF
    kind: DevWorkspace
    apiVersion: workspace.devfile.io/v1alpha2
    metadata:
      name: theia-next
    spec:
      started: true
      template:
        components:
          - name: test-container
            container:
              image: quay.io/libpod/busybox:latest
              volumeMounts:
                - name: testing-volume
                  path: /test-path
          - name: theia
            plugin:
              uri: https://che-plugin-registry-main.surge.sh/v3/plugins/eclipse/che-theia/next/devfile.yaml
          - name: testing-volume
            volume: {}
    EOF
  2. Create secrets and configmaps for testing
    kubectl create secret generic test
    kubectl create configmap test
    kubectl patch secret test --type merge -p \
      '{
        "metadata": {
          "labels": {
            "controller.devfile.io/mount-to-devworkspace": "true"
          }
        }
      }'
    kubectl patch cm test --type merge -p \
      '{
        "metadata": {
          "labels": {
            "controller.devfile.io/mount-to-devworkspace": "true"
          }
        }
      }'
  3. Case 1: automounted volumes collide on mountPath
    kubectl patch secret test --type merge -p \
      '{
        "metadata": {
          "annotations": {
            "controller.devfile.io/mount-path": "/collision"
          }
        }
      }'
    kubectl patch cm test --type merge -p \
      '{
        "metadata": {
          "annotations": {
            "controller.devfile.io/mount-path": "/collision"
          }
        }
      }'
    kubectl apply -f /tmp/test-dw.yaml
    kubectl get dw -w
    (should fail with meaningful error message)
  4. Case 2: automounted volume and DevWorkspace volume collide on mountPath
    kubectl delete dw theia-next
    kubectl patch cm test --type merge -p \
      '{
        "metadata": {
          "annotations": {
            "controller.devfile.io/mount-path": "/test-path"
          }
        }
      }'
    kubectl apply -f /tmp/test-dw.yaml
    kubectl get dw -w
    (should fail with meaningful error message)

PR Checklist

  • E2E tests pass (when PR is ready, comment /test v7-devworkspaces-operator-e2e, v7-devworkspace-happy-path to trigger)
    • v7-devworkspaces-operator-e2e: DevWorkspace e2e test
    • v7-devworkspace-happy-path: DevWorkspace e2e test

Move functions related to automounting configmaps and secrets to a
separate file to make reading deployment.go easier.

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Check edge cases in automount that can result in collisions:
1. automounted volumes collide on name
2. automounted volumes collide on mountPath
3. devworkspace volumes collide with automounted volume on name
4. devworkspace volume mountPath in a container collides with
   automounted volume's mountPath

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Prefix automounted configmaps and secrets with
"automount-[secret|configmap]" to avoid collisions with volumes defined
on DevWorkspaces.

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested but changes look meaningful.

Ideally, we should have unit tests for covered use-cases, but it's not mandatory.

@amisevsk
Copy link
Collaborator Author

Ideally, we should have unit tests for covered use-cases, but it's not mandatory.

I'll look into this -- I've generally been avoidant of adding tests for things that require the k8s API, so maybe it's time to dive into fakeClient :)

Copy link
Contributor

@JPinkney JPinkney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes make sense to me 👍

@sleshchenko
Copy link
Member

/test v7-devworkspaces-operator-e2e, v7-devworkspace-happy-path

@amisevsk
Copy link
Collaborator Author

/test v7-devworkspace-happy-path

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm label Jul 23, 2021
controllers/workspace/provision/automount_test.go Outdated Show resolved Hide resolved
controllers/workspace/provision/automount_test.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added the lgtm label Jul 26, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jul 26, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, JPinkney, sleshchenko

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 [JPinkney,amisevsk,sleshchenko]

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

@JPinkney JPinkney mentioned this pull request Jul 26, 2021
8 tasks
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@openshift-ci
Copy link

openshift-ci bot commented Jul 26, 2021

New changes are detected. LGTM label has been removed.

@openshift-ci openshift-ci bot removed the lgtm label Jul 26, 2021
@amisevsk
Copy link
Collaborator Author

/test v7-devworkspaces-operator-e2e, v7-devworkspace-happy-path

@sleshchenko sleshchenko merged commit 46618e2 into devfile:main Jul 27, 2021
@amisevsk amisevsk deleted the automount-checks branch July 28, 2021 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants