fix: eph env + improve docker images to run in userspace #32017
Conversation
Recently realized that running docker-compose as `superset` didn't work out of the image box. docker-compose.yml sets `root` as the user, and when switching to user `superset`, there were some permission issues around excecuting certain scripts and such. Here I'm fixing up some things, paying attention to a few layers, and avoiding doing any sorts of installation of anything while starting the app as the `superset` user.
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
I've completed my review and didn't find any issues.
Files scanned
| File Path | Reviewed |
|---|---|
| docker/entrypoints/docker-ci.sh | ✅ |
| docker/docker-bootstrap.sh | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Current Korbit Configuration
General Settings
Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ❌ Issue Categories
Category Enabled Naming ✅ Database Operations ✅ Documentation ✅ Logging ✅ Error Handling ✅ Systems and Environment ✅ Objects and Data Structures ✅ Readability and Maintainability ✅ Asynchronous Processing ✅ Design Patterns ✅ Third-Party Libraries ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support
Note
Korbit Pro is free for open source projects 🎉
Looking to add Korbit to your team? Get started with a free 2 week trial here
Dockerfile
Outdated
| @@ -76,8 +76,7 @@ COPY superset-frontend /app/superset-frontend | |||
| FROM superset-node-ci AS superset-node | |||
|
|
|||
| # Build the frontend if not in dev mode | |||
| RUN --mount=type=cache,target=/app/superset-frontend/.temp_cache \ | |||
| --mount=type=cache,target=/root/.npm \ | |||
| RUN --mount=type=cache,target=$/root/.npm \ | |||
There was a problem hiding this comment.
Here caching the cache mount was preventing the docker-compose-level mount from hooking up the cache to the host. Getting a nice perf boost by letting the host mount do what it's supposed to.
| ARG DEV_MODE="false" # Skip frontend build in dev mode | ||
| ENV DEV_MODE=${DEV_MODE} | ||
|
|
||
| ENV LANG=C.UTF-8 \ |
There was a problem hiding this comment.
LANG/ LC_ALL are not needed anymore as of 3.9 I believe. Other ENVs we don't need in this layer. Turns out ENV don't carry through a layer with a different base and led to some confusion on my part. As it was ENV would not carry through to the final layers, but they do when located in python-common
| # Install Playwright and optionally setup headless browsers | ||
| ARG INCLUDE_CHROMIUM="true" | ||
| ARG INCLUDE_FIREFOX="false" | ||
| RUN --mount=type=cache,target=/root/.cache/uv\ |
There was a problem hiding this comment.
Moving this section to the python-common layer that's used for the app itself as the python/backend translation builder layer does not need this
| @@ -159,13 +138,20 @@ RUN if [ "$BUILD_TRANSLATIONS" = "true" ]; then \ | |||
| # Python APP common layer | |||
| ###################################################################### | |||
| FROM python-base AS python-common | |||
|
|
|||
| ENV SUPERSET_HOME="/app/superset_home" \ | |||
There was a problem hiding this comment.
these now carry through to the 3 final images
|
/testenv up |
|
@mistercrunch Processing your ephemeral environment request here. |
|
@mistercrunch Ephemeral environment spinning up at http://35.93.23.171:8080. Credentials are |
| uv pip install . | ||
|
|
||
| RUN uv pip install .[postgres] |
There was a problem hiding this comment.
going back to installing postgres client as part of the build process for the dev and ci layer (still not packaging the client on lean)
| # Make python interactive | ||
| if [ "$DEV_MODE" == "true" ]; then | ||
| if command -v uv > /dev/null 2>&1; then | ||
| if [ "$(whoami)" = "root" ] && command -v uv > /dev/null 2>&1; then |
There was a problem hiding this comment.
only doing this when root as superset doesn't have the rights to touch the python packaging.
| @@ -34,7 +37,8 @@ if [ "$CYPRESS_CONFIG" == "true" ]; then | |||
| export SUPERSET__SQLALCHEMY_DATABASE_URI=postgresql+psycopg2://superset:superset@db:5432/superset_cypress | |||
| PORT=8081 | |||
| fi | |||
| if [[ "$DATABASE_DIALECT" == postgres* ]] ; then | |||
| if [[ "$DATABASE_DIALECT" == postgres* ]] && [ "$(whoami)" = "root" ]; then | |||
There was a problem hiding this comment.
We need to keep this for the use case of other docker-compose files that mount the docker/ folder
|
OMG a working ephemeral env! ^^^^ |
|
|
mistercrunch
changed the title
mistercrunch
changed the title
|
@mistercrunch can you check this issue? I have problems running the cc @sfirke who helped on the investigation as well. |
|
Can you confirm you have issues when using |
While digging into fixing ephemeral envs, I realized that running docker-compose as
supersetdidn't work out of the box.docker-compose.ymlsetsrootas the user which goes around the issue while runningcompose. When switching to usersuperset, there were some permission issues around excecuting certain scripts and such.Clearly it's better/more-secure to run the app as a non-root user. So here I'm making some changes and simplifying things, paying attention to a few layers, and avoiding doing any sorts of installation of anything while starting the app as the
supersetuser in the tree of entrypoint scripts.Adding notes in PR/code to explain the changes step-by-step
Tested locally:
docker-compose upwhile using the defaultrootuserdocker-compose upwhile usingsupersetuserdocker-compose -f docker-compose-image-tag.yml updocker-compose build --build-arg BUILD_TRANSLATIONS=true