feat(ws): containerize frontend component #394
Conversation
There was a problem hiding this comment.
In general - I think we want to approach the Dockerfile more geared to a production build of the frontend.
⚠️ We should verify this with the community - so please only take this as a discussion point for now!
By "production build" - I mean making sure the image is built deterministically and focused on being as small as possible.
npm ci- only install required dependencies
NODE_ENV=production- using multi-stage built to not leak dev/optional dependencies into image
Here is a rough outline of how I could see this Dockerfile structured given the above (but just use this is a discussion point for time being)
# Build stage
FROM node:20-slim AS builder
# Set working directory
WORKDIR /usr/src/app
# Copy package files
COPY package*.json ./
# Install ALL dependencies (including devDependencies)
RUN npm ci
# Copy source code
COPY . .
# Build the application
RUN npm run build
# Production stage
FROM node:20-slim
# Set working directory
WORKDIR /usr/src/app
# Copy package files from builder stage
COPY --from=builder /usr/src/app/package*.json ./
# Install only production dependencies
RUN npm ci --only=production
# Copy built assets from builder stage
COPY --from=builder /usr/src/app/dist ./dist
COPY --from=builder /usr/src/app/public ./public
# Create non-root user
RUN addgroup --system appgroup && \
adduser --system appuser --ingroup appgroup && \
chown -R appuser:appgroup /usr/src/app
# Switch to non-root user
USER appuser
# Expose the development port (matching webpack dev server)
EXPOSE 8080
# Set environment variables
ENV NODE_ENV=production
ENV PORT=8080
# Start the production server
CMD ["npm", "run", "start:prod"]
|
@andyatmiami: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
workspaces/frontend/Dockerfile
Outdated
| # Use a non-root user for security | ||
| RUN addgroup --system appgroup && adduser --system appuser --ingroup appgroup | ||
| USER appuser |
There was a problem hiding this comment.
Not implying anything wrong with this RUN statement to define a non-root user - but we should definitely align how we do this across controller + backend + frontend for sake of consistency.
Right now, backend + controller simply use hard-coded value of USER 65532:65532
(fwiw - I like this solution better - but def want consensus from the community at large!)
There was a problem hiding this comment.
Yeah, I used the following method beacuse this approach is a bit more explicit and readable.
but I totally agree with you that consistency across the project definitely matters.
I’ll change it..
Noa-limoy
force-pushed
the
feat/containerize_fronted_component/392
branch
2 times, most recently
from
89a691b to
f77bc77
Compare
Noa-limoy
changed the title
Noa-limoy
changed the title
Noa-limoy
force-pushed
the
feat/containerize_fronted_component/392
branch
from
f77bc77 to
a4f2b22
Compare
There was a problem hiding this comment.
Really nice work getting this all fleshed out...
I have some initial suggestions for the nginx.conf file - not sure this will be enough to make it production-ready - but I feel its a step in the right direction....
worker_processes auto;
error_log /dev/stderr warn;
pid /tmp/nginx.pid;
events {
worker_connections 1024;
}
http {
log_format main '$remote_addr - $remote_user [$time_local] - $http_x_api_version - "$request" '
'$status $body_bytes_sent "$http_referer" '
'"$http_user_agent" "$http_x_forwarded_for"';
access_log /dev/stdout main;
include /etc/nginx/mime.types;
default_type application/octet-stream;
# Security headers
add_header X-Frame-Options "SAMEORIGIN" always;
add_header X-XSS-Protection "1; mode=block" always;
add_header X-Content-Type-Options "nosniff" always;
add_header Referrer-Policy "no-referrer-when-downgrade" always;
add_header Content-Security-Policy "default-src 'self' http: https: data: blob: 'unsafe-inline'" always;
# --- Gzip Compression ---
gzip on;
gzip_types text/plain text/html text/css application/json application/javascript text/xml application/xml application/xml+rss text/javascript image/svg+xml;
gzip_comp_level 5;
gzip_min_length 1000;
gzip_proxied any;
gzip_vary on;
gzip_disable "msie6";
# Health check endpoint
server {
listen 8080;
# Health check endpoint
location /health {
access_log off;
return 200 'healthy\n';
}
location / {
root /usr/share/nginx/html;
index index.html;
try_files $uri $uri/ /index.html;
}
# Static assets (cache enabled)
location ~* \.(css|js|gif|jpeg|jpg|png|ico|woff|woff2|ttf|otf|svg|eot)$ {
root /usr/share/nginx/html;
expires 30d;
add_header Cache-Control "public, no-transform";
try_files $uri =404;
}
# Backend API - Using Kubernetes service discovery
location /api/ {
# Use environment variable for backend service
set $backend_service "${BACKEND_SERVICE:-backend-service}:4000";
proxy_pass http://$backend_service;
proxy_http_version 1.1;
proxy_set_header Host $host;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header X-Forwarded-Proto $scheme;
# Timeouts
proxy_connect_timeout 60s;
proxy_send_timeout 60s;
proxy_read_timeout 60s;
}
}
}
Key changes included above:
- logs write to stdout / stderr (so they can be captured by k8s)
- worker_processes: auto to avoid hard-coded 1
- added various security headers (although feel free to suggest better configurations of said headers)
- added simple /health endpoint for k8s liveness/readiness
- added proxy timeout settings (again, suggest better values vs. the 60s there now)
- most importantly, though, added env var support for the proxy_pass value... as the hardcoded docker hostname isn't going to work for us.. although unclear if this "env var" solution is good enough
|
/ok-to-test |
Noa-limoy
force-pushed
the
feat/containerize_fronted_component/392
branch
2 times, most recently
from
5828fcf to
b146c0e
Compare
workspaces/frontend/Dockerfile
Outdated
| ENV PORT=8080 | ||
|
|
||
| # Set default backend service | ||
| ENV BACKEND_SERVICE=localhost:4000 |
There was a problem hiding this comment.
I think we should change this default value to backend-service:4000 (albeit there isn't a clear cut "best default" imho)
Problem with localhost:4000
- Naively,
localhostwill resolve within the running container and not be able to communicate with thebackendwithout additional flags/configs specified - This also doesn't really make sense in a k8s environment
My Suggestion
ENV BACKEND_SERVICE=backend-service:4000
Rationale
- this still won't "just work" if trying to run locally, but it at least alludes to user that this "external dependency" exists (i.e. its a more descriptive error vs. just
localhostresolution errors) - Both Docker and Kubernetes could (if configured correctly) support the
backend-service:4000- so under the right conditions - the-eflag need not be provided to Docker and/or specified inenvconfig in Kubernetes
There was a problem hiding this comment.
I completely agree with your suggestion...
the name backend-service indeed implies it's an external service – unlike localhost – and it's more appropriate for Kubernetes envs, where there's internal DNS that can resolve backend-service.
| @@ -0,0 +1,70 @@ | |||
| # ---------- Builder stage ---------- | |||
There was a problem hiding this comment.
in an effort to promote more deterministic builds - while offering a little more flexibility - I would if we should introduce some global build args here:
ARG NODE_VERSION=20.11.0
ARG NGINX_VERSION=1.25.3
So then our various "stage declarations" would look like:
FROM node:${NODE_VERSION}-slim AS builderFROM nginx:${NGINX_VERSION}-alpine
Of course this would also mean net-new commits would be required to pick up even incremental (non-breaking) version updates... while the "transparency" is nice - the "housekeeping" aspect is less nice 🤔
There was a problem hiding this comment.
For now - we will err on the side of consistency and not act on this change - as the controller and backend modules do not leverage these types of ARG inputs for granular version control.
workspaces/frontend/Dockerfile
Outdated
| USER root | ||
|
|
||
| # Install envsubst (gettext package) | ||
| RUN apk add --no-cache gettext | ||
|
|
||
| # Copy built assets from builder stage | ||
| COPY --from=builder /usr/src/app/dist /usr/share/nginx/html | ||
|
|
||
| # Copy nginx template | ||
| COPY nginx.conf.template /etc/nginx/nginx.conf.template | ||
|
|
||
| # Create directories and set permissions for non-root user | ||
| RUN mkdir -p /var/cache/nginx/client_temp \ | ||
| /var/cache/nginx/proxy_temp \ | ||
| /var/cache/nginx/fastcgi_temp \ | ||
| /var/cache/nginx/uwsgi_temp \ | ||
| /var/cache/nginx/scgi_temp \ | ||
| /var/run/nginx \ | ||
| /tmp/nginx && \ | ||
| # Change ownership of nginx directories to nginx user (UID 101) | ||
| chown -R 101:101 /var/cache/nginx \ | ||
| /var/run/nginx \ | ||
| /usr/share/nginx/html \ | ||
| /tmp/nginx \ | ||
| /etc/nginx | ||
|
|
||
| # Create startup script that works with non-root user | ||
| RUN echo '#!/bin/sh' > /docker-entrypoint.sh && \ | ||
| echo 'envsubst "\${BACKEND_SERVICE}" < /etc/nginx/nginx.conf.template > /tmp/nginx/nginx.conf' >> /docker-entrypoint.sh && \ | ||
| echo 'exec nginx -c /tmp/nginx/nginx.conf -g "daemon off;"' >> /docker-entrypoint.sh && \ | ||
| chmod +x /docker-entrypoint.sh && \ | ||
| chown 101:101 /docker-entrypoint.sh |
There was a problem hiding this comment.
I feel like we can be more precise on the commands that actually need to run as root - and then execute other commands after the USER 101:101 declaration... to avoid the various chown commands
I think the following encapsulates what is actually required to run as root
# Install envsubst (gettext package)
RUN apk add --no-cache gettext
# Copy built assets from builder stage
COPY --from=builder /usr/src/app/dist /usr/share/nginx/html
# Copy nginx template
COPY nginx.conf.template /etc/nginx/nginx.conf.template
# Create base directories that need root
RUN mkdir -p /var/cache/nginx \
/var/run/nginx \
/tmp/nginx
All other commands can be moved down into the "user space" so the permissions are just set appropriately from the outset...
There was a problem hiding this comment.
Once we drop root privileges — as per your suggestion — and switch to USER 101:101 we can no longer create or modify directories under those system paths, as those are owned by root and not writable by the nginx user
/var/cache/nginx/client_temp \ /var/cache/nginx/proxy_temp \ /var/cache/nginx/fastcgi_temp \ /var/cache/nginx/uwsgi_temp \ /var/cache/nginx/scgi_temp
So even though we want to avoid chown, in this case it’s necessary otherwise, we will run into permission issues when creating these directories.
|
|
||
| # Gzip Compression | ||
| gzip on; | ||
| gzip_types text/plain text/css application/json application/javascript text/xml application/xml application/xml+rss text/javascript image/svg+xml; |
There was a problem hiding this comment.
I hesitate to offer this suggestion now as #434 is not yet merged - but we should probably include gzip support for the custom content-type header we are leveraging:
application/vnd.kubeflow-notebooks.manifest+yaml
Granted if that value were to change - it should be reflected here.
There was a problem hiding this comment.
Just wondering — is the payload for this custom content-type typically large enough to justify enabling gzip and the extra maintenance if this value ever changes?
There was a problem hiding this comment.
we ended up changing the content-type to application/yaml - and at scale it could conceivably be large enough to warrant compression.. so i think we should add it.
|
As my outstanding PR review comments don't fundamentally change the integrity of the code provided on the PR (are moreso just "nice to haves") - I went ahead and tested the changes in a variety of scenarios and happy to confirm everything works as intended 💯 Running
|
Noa-limoy
force-pushed
the
feat/containerize_fronted_component/392
branch
from
b146c0e to
2a961da
Compare
google-oss-prow
bot
added
area/frontend
Noa-limoy
force-pushed
the
feat/containerize_fronted_component/392
branch
from
2a961da to
ecb7535
Compare
| # Upstream backend configuration | ||
| upstream backend { | ||
| server ${BACKEND_SERVICE}; | ||
| } |
There was a problem hiding this comment.
In discussions with Mathew - it was mentioned that we do NOT want to expose the backend via NGINX
we will only expose the frontend - and then configure the frontend container such that it will communicate directly with the backend
| # Backend API | ||
| location /api/ { | ||
| proxy_pass http://backend/api/; | ||
| proxy_http_version 1.1; | ||
| proxy_set_header Host $host; | ||
| proxy_set_header X-Real-IP $remote_addr; | ||
| proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; | ||
| proxy_set_header X-Forwarded-Proto $scheme; | ||
|
|
||
| # Timeouts | ||
| proxy_connect_timeout 60s; | ||
| proxy_send_timeout 60s; | ||
| proxy_read_timeout 60s; | ||
|
|
||
| } |
There was a problem hiding this comment.
workspaces/frontend/Dockerfile
Outdated
| ENV PORT=8080 | ||
|
|
||
| # Set default backend service | ||
| ENV BACKEND_SERVICE=backend-service:4000 |
There was a problem hiding this comment.
We want to set whatever environment variables are supported by frontend here that allows configuring the network information to connect to backend...
workspaces/frontend/Dockerfile
Outdated
| # Create startup script that works with non-root user | ||
| RUN echo '#!/bin/sh' > /docker-entrypoint.sh && \ | ||
| echo 'envsubst "\${BACKEND_SERVICE}" < /etc/nginx/nginx.conf.template > /tmp/nginx/nginx.conf' >> /docker-entrypoint.sh && \ | ||
| echo 'exec nginx -c /tmp/nginx/nginx.conf -g "daemon off;"' >> /docker-entrypoint.sh && \ | ||
| chmod +x /docker-entrypoint.sh && \ | ||
| chown 101:101 /docker-entrypoint.sh |
There was a problem hiding this comment.
This can probably be simplified in light of:
Signed-off-by: Noa <[email protected]>
Noa-limoy
force-pushed
the
feat/containerize_fronted_component/392
branch
from
ecb7535 to
01dc84a
Compare
|
/lgtm as discussed with community - the ability to have the frontend container interact with the backend will be enabled via work on the manifests... so this PR it itself laser-focused on simply containerizing the frontend code - which I have verified to be doing just that 
|
thesuperzapper
changed the title
|
@Noa-limoy thanks for your work on this, great to have another contributor. Since we aren't pushing the image yet, I am happy to merge without as much testing. However, I expect we might need to add configs for HTTP_PATH_PREFIX (or similar), or just be very careful to ensure always using relative paths in the frontend itself, and the IFRAME stuff might not work properly once we wrap the frontend in the central dashboard. /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: thesuperzapper 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:
Approvers can indicate their approval by writing |
github-project-automation
bot
moved this from Needs Triage
to Done
in Kubeflow Notebooks
closes #392
I've created a Dockerfile for the frontend component, which allows for containerization of the frontend application. Currently, the frontend can only be run locally via the command line.
now we can run the frontend as a k8s workload resource, and its separate from the backend container.
Build the frontend image:
docker build -f Dockerfile -t frontend-img .Run the container and expose it on port 8080:
docker run -it --rm --name workspace-frontend -p 8080:8080 frontend-img