feat: non-root self hosted images for standard deployment #5701
Conversation
Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com>
Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com>
|
New Issues (1)Checkmarx found the following issues in this Pull Request
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5701 +/- ##
=======================================
Coverage 47.51% 47.51%
=======================================
Files 1662 1662
Lines 75215 75216 +1
Branches 6761 6761
=======================================
+ Hits 35737 35738 +1
Misses 38015 38015
Partials 1463 1463 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Checkmarx One found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
justindbaur
force-pushed
the
feat/pm-14496-non-root-self-hosted-images
branch
from
e47a1bb to
2e156c2
Compare
justindbaur
force-pushed
the
feat/pm-14496-non-root-self-hosted-images
branch
from
2e156c2 to
284501a
Compare
There was a problem hiding this comment.
Overall looks pretty good to me, though we'll need to make sure this is well tested by QA to ensure everything is working.
I've left some suggestions simplify the Dockerfiles a bit more. We have a lot of them so any extra code that we require gets multiplied by 10.
|
There's a merge conflict in the build workflow. I'm reworking things to reduce the changes between the workflow on this branch and |
|
Apologies for the re-review requests. I think I've reduced the build workflow changes to bring it more inline with |
| dotnet: true | ||
| node: true |
There was a problem hiding this comment.
This project is built for node in the artifact job and also build for dotnet in the docker image job, removing the artifact job will build this image for both node and dotnet, is that the intended behaviour?
There was a problem hiding this comment.
Unless I'm misunderstanding your concern, Admin needs node to build the static files and dotnet to build the server.
There was a problem hiding this comment.
Just ensuring it's not a mistake
| - project_name: Server | ||
| base_path: ./util |
There was a problem hiding this comment.
Any reason why we are removing this image name in the build?
There was a problem hiding this comment.
We previously only needed a dedicated bitwarden/server image for attachments. Since we're using multi-stage builds, we no longer require it.
| - name: Check branch to publish | ||
| env: | ||
| PUBLISH_BRANCHES: "main,rc,hotfix-rc" | ||
| id: publish-branch-check | ||
| run: | | ||
| IFS="," read -a publish_branches <<< $PUBLISH_BRANCHES |
There was a problem hiding this comment.
Why are we not checking for branch to publish anymore?
There was a problem hiding this comment.
This was removed by mistake. Thanks for catching it. Added it back in 88848e6.
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-14496
Update our self-hosted standard deployment images (this work excludes Unified) to allow running in non-root contexts, such as kubernetes deployments using
runAsNonRootor specifyinguser: ${PUID}:${PGID}in a container compose override file.Note that this work excludes the MSSQL and Nginx images, so a fully non-rootful deployment will require hosting those services separately. Also note that the images are intended to run as non-root users on an opt-in basis. They are not rootless by default, as we want to avoid making breaking changes to existing rootful deployments for the time being.
📔 Objective
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes