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

Don't hardcode root when running lifecycle scripts #1317

Merged
merged 3 commits into from
Oct 23, 2024

Conversation

nrontsis
Copy link
Contributor

@nrontsis nrontsis commented Oct 16, 2024

As discussed in #1313 (comment), this PR avoids a hardcoded assumption from the lifecycle scripts that the devpod is built under a root user.

Building the devpod under a non-root user can be important in restricted kubernetes corporate environments. While currently creating devpods under a non-root user in kubernetes requires some further hacks, this PR removes a blocking certain aspect of it, allowing for creating a devpod as per the following demo. Before this PR, the building of the devpod would stop because "su", user, "-c", command.Quote(c) would be called by the run lifecycle hooks logic

Minimal example (not tested end-to-end)

devpod provider add kubernetes k8s-demo
devpod provider set-options k8s-demo \
  --option STRICT_SECURITY=true \
  --option POD_MANIFEST_TEMPLATE="$(cat k8s_template.yaml)"
devpod up [email protected]:nrontsis/devpod-example-go.git --ide=none --provider=k8s-demo --open-ide=false

where k8s_template.yaml is

kind: Pod
apiVersion: v1
spec:
  initContainers:
  - name: set-volume-owner
    image: busybox
    command: [ "sh", "-c", "chown -R 1000:1000 /volume" ]
    volumeMounts:
    - mountPath: /volume
      name: devpod
      subPath: devpod/0
  containers:
    - name: devpod
      securityContext: 
        runAsGroup: 1000
        runAsUser: 1000
        runAsNonRoot: true
status: {}

Copy link
Member

@pascalbreuninger pascalbreuninger left a comment

Choose a reason for hiding this comment

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

Had a remark about error handling, other than that LGTM. Thanks for the contribution 🙌

pkg/devcontainer/setup/lifecyclehooks.go Outdated Show resolved Hide resolved
@bkneis
Copy link
Contributor

bkneis commented Oct 18, 2024

@nrontsis thanks for the contribution! The implementation is a small change with a big impact, perfect! If you have some time I would love to know an full e2e test was run, even manually, just to ensure the workspace starts with no errors and that it doesn't cause other issues. Once this is done and the 2 small remarks we can merge

@bkneis bkneis self-requested a review October 18, 2024 07:42
@nrontsis
Copy link
Contributor Author

nrontsis commented Oct 19, 2024

@bkneis I've now successfully tested the minimal example end-to-end using the following steps.

First go to my test repo, build .devcontainer/Dockerfile and push the resulting image in a registry that the k8s cluster will have access to. After that replace .devcontainer/devcontainer.json:image to point to that pushed image.

Then clone the patched devpod of this PR and do:

./hack/rebuild.sh
test/devpod-cli-linux-amd64 provider add kubernetes --name k8s-demo
test/devpod-cli-linux-amd64 provider set-options k8s-demo \
  --option STRICT_SECURITY=true \
  --option POD_MANIFEST_TEMPLATE="$(cat k8s_template.yaml)" 
test/devpod-cli-linux-amd64 up ./devpod-example-go --ide=none --provider=k8s-demo --open-ide=false --debug

where k8s_template.yaml is

kind: Pod
apiVersion: v1
spec:
  initContainers:
  - name: set-volume-owner
    image: busybox
    command: [ "sh", "-c", "chown -R 1000:1000 /volume" ]
    volumeMounts:
    - mountPath: /volume
      name: devpod
      subPath: devpod/0
  containers:
    - name: devpod
      securityContext: 
        runAsGroup: 1000
        runAsUser: 1000
        runAsNonRoot: true
status: {}

which runs without issues.

@nrontsis
Copy link
Contributor Author

I believe this is ready now.

Copy link
Contributor

@bkneis bkneis left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! LGTM

@nrontsis
Copy link
Contributor Author

Thanks for the reviews! Happy to merge this?

@pascalbreuninger pascalbreuninger merged commit de14479 into loft-sh:main Oct 23, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants