-
Notifications
You must be signed in to change notification settings - Fork 34
Update autocalib image to run as non root user #492
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the autocalibration Docker image to run as a non-root user for improved security. The changes eliminate the need for privileged containers and SYS_ADMIN capabilities by replacing the complex initialization script with a simpler entrypoint and using init containers for volume permission fixes.
- Removes the
camcalibration-init
script and replaces it with a lightweight entrypoint - Configures the container to run as user ID 1000 instead of root
- Adds init containers in deployment configurations to fix volume permissions
- Removes privileged container requirements and security capabilities
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
sample_data/docker-compose-dls-perf.yml | Adds init container for volume permissions, configures camcalibration to run as user 1000, removes privileged settings |
sample_data/docker-compose-dl-streamer-example.yml | Same changes as dls-perf compose file for consistency |
kubernetes/scenescape-chart/templates/camcalibration-dep.yaml | Updates Kubernetes deployment to run as non-root with init container for permissions |
autocalibration/src/camcalibration-init | Removes the entire 179-line initialization script |
autocalibration/Dockerfile | Updates image to create user with UID 1000, removes root dependencies, adds new entrypoint script |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In review
autocalibration/Dockerfile
Outdated
# Create model directory and tmp directory for healthcheck | ||
RUN mkdir -p $NETVLAD_MODEL_DIR /tmp && \ | ||
chown -R $WSUSER:$WSUSER $NETVLAD_MODEL_DIR && \ | ||
chmod 1777 /tmp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the tmp folder require +x permissions? 666 should suffice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This permission is very different on directories compared to files. Essentially, you can think of it as providing access to the directory. Having execute permission on a directory authorizes you to look at extended information on files in the directory (using ls -l
, for instance) but also allows you to change your working directory (using cd) or pass through this directory on your way to a subdirectory underneath.
Lacking execute permission on a directory can limit the other permissions in interesting ways. For example, how can you add a new file to a directory (by leveraging the write permission) if you can't access the directory's metadata to store the information for a new, additional file? You cannot.
autocalibration/Dockerfile
Outdated
RUN chmod +x /usr/local/bin/camcalibration-init | ||
COPY --chown=scenescape:scenescape autocalibration/src/*.py $SCENESCAPE_HOME/ | ||
COPY --chown=scenescape:scenescape autocalibration/tools/ondemand_model_loader.py /usr/local/bin/download_models.py | ||
RUN chmod +x /usr/local/bin/download_models.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we always call it with python3. So setting the execution permission is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the chmod +x
networks: | ||
scenescape: | ||
command: camcalibration --broker broker.scenescape.intel.com --resturl https://web.scenescape.intel.com:443/api/v1 | ||
user: "1000:1000" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"${UID:-1000}:${GID:-1000}"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
|
||
# Init container to fix volume permissions for camcalibration | ||
camcalibration-init: | ||
image: alpine:3.19 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use alpine:latest in other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using latest
now
📝 Description
Updates the autocalibration Docker image to run as non root user.
Adds init container to fix volume permissions.
Removes entire camcalib-init script, thanks to this change the container:
sys_admin
capabilityBoth Kubernetes deployment and Docker compose files are updated.
✨ Type of Change
Select the type of change your PR introduces:
🧪 Testing Scenarios
Describe how the changes were tested and how reviewers can test them too:
✅ Checklist
Before submitting the PR, ensure the following: