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

give option to run container as non-root windmill user #4959

Merged
merged 17 commits into from
Jan 3, 2025

Conversation

alpetric
Copy link
Contributor

@alpetric alpetric commented Dec 20, 2024

uid/gid 1000:1000


Important

Add option to run container as non-root 'windmill' user and update volume paths in Docker and Terraform configurations.

  • Dockerfile:
    • Adds non-root user 'windmill' with UID/GID 1000.
    • Copies cache from /root/.cache to /home/windmill/.cache.
    • Adjusts ownership of ${APP}, /tmp/windmill, and /home/windmill/.cache to 'windmill'.
  • docker-compose.yml:
    • Changes volume path from /root/.cache to /pyls/.cache for lsp service.
  • Terraform (windmill_lsp.tf):
    • Updates host_path for lsp_cache volume to /pyls/.cache.
  • lsp/Dockerfile:
    • Sets WORKDIR to /pyls and ensures permissions for all users.
    • Installs necessary packages and sets up environment for pyls service.

This description was created by Ellipsis for af1efd6. It will automatically update as commits are pushed.

Copy link

cloudflare-workers-and-pages bot commented Dec 20, 2024

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3d89c0c
Status: ✅  Deploy successful!
Preview URL: https://272b48ec.windmill.pages.dev
Branch Preview URL: https://alp-docker-non-root.windmill.pages.dev

View logs

@alpetric alpetric marked this pull request as ready for review December 24, 2024 14:51
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 5d71040 in 52 seconds

More details
  • Looked at 86 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. lsp/Dockerfile:54
  • Draft comment:
    Setting the user back to root after creating a non-root user defeats the purpose of running the container as a non-root user. Consider removing this line to enhance security.
  • Reason this comment was not posted:
    Marked as duplicate.
2. Dockerfile:203
  • Draft comment:
    Copying files from /root/.cache to /home/windmill/.cache assumes that the cache directory exists and is accessible. Ensure that /root/.cache exists before copying to avoid potential errors.
  • Reason this comment was not posted:
    Comment did not seem useful.
3. docker-compose.yml:146
  • Draft comment:
    The key pull_policy should be pull_policy. This typo appears in multiple places (lines 28, 46, 73, 96, 121).
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.

Workflow ID: wflow_79ToDRyLSx5QiGfq


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Dockerfile Show resolved Hide resolved
@alpetric alpetric changed the title [WIP] give option to run container as non-root windmill user give option to run container as non-root windmill user Dec 27, 2024
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on af1efd6 in 8 seconds

More details
  • Looked at 24 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. lsp/Dockerfile:62
  • Draft comment:
    The CMD line is unnecessarily complex. Consider simplifying it for better readability and maintainability. For example, you can separate the logic into a shell script and call the script from CMD.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The CMD line is unnecessarily complex and could be simplified for better readability and maintainability.

Workflow ID: wflow_JkoVDoyVKuo36fFj


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@alpetric
Copy link
Contributor Author

@rubenfiszel tested last commit both with root / non_root and it works both with the old /root/.cache mount and the /pyls/.cache mount

@rubenfiszel rubenfiszel merged commit 0a72687 into main Jan 3, 2025
22 checks passed
@rubenfiszel rubenfiszel deleted the alp/docker_non_root branch January 3, 2025 15:51
@github-actions github-actions bot locked and limited conversation to collaborators Jan 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants