build(sentry): Prepare for a non-root sentry image - #4535
Conversation
The upstream sentry image is moving to run as the non-root sentry user and will drop gosu (getsentry/sentry#125848). Make self-hosted work with both the current root image and the upcoming non-root one: - Run build customizations (nodestore-s3, enhance-image.sh, requirements.txt) as root, then switch to the sentry user. - Trust custom CAs without update-ca-certificates, which needs root: build a combined bundle in /tmp and point the TLS env vars at it. - Fix sentry-data ownership once during install instead of at every container start. - Install gosu in the cleanup image instead of relying on the sentry image.
Coverage Results 📊✅ 22 passed | Total: 22 | Pass Rate: 100% | Execution Time: 11m 18s 📊 Comparison with Base Branch
✨ Test counts unchanged from base. All tests are passing successfully. ✅ Patch coverage is 100.00% (no changed executable lines found; target 50%). Coverage diff@@ Coverage Diff @@
## master #4535 +/-##
==========================================
Coverage 95.54% 95.54% —%
==========================================
Files 5 5 —
Tracked lines 336 336 —
Branches 0 0 —
==========================================
Hits 321 321 —
Misses 15 15 —
Partials 0 0 —Generated by Coverage Action |
Replace the /tmp CA bundle with making /etc/ssl/certs writable by the sentry user, so the existing entrypoint can run update-ca-certificates unchanged. The bundle approach left the standard bundle path and the hashed certs directory without the custom CAs, and did not apply to docker compose exec, so clients that don't read the TLS env vars (e.g. librdkafka, capath lookups, paths set in sentry.conf.py) would stop trusting them.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 582885b. Configure here.
| for root, dirs, files in os.walk("/data"): | ||
| for path in (root, *(os.path.join(root, name) for name in dirs + files)): | ||
| if os.lstat(path).st_uid != SENTRY_UID: | ||
| os.lchown(path, SENTRY_UID, -1) |
There was a problem hiding this comment.
Ownership check skips nested files
Medium Severity
The install-time chown only walks /data when /data or /data/files themselves are not uid 999. Those two directories are the first paths updated, so an interrupted run, a volume whose root is already 999, or leftover root-owned trees such as custom-packages make the next ./install.sh skip the walk. Nested files then stay root-owned, and the non-root sentry process cannot write them. The old entrypoint repaired ! -user sentry files on every start; this path cannot recover.
Reviewed by Cursor Bugbot for commit 582885b. Configure here.
There was a problem hiding this comment.
Good bot. Now walks bottom-up so interrupted runs resume. Nested-files case matches the old entrypoint's guard, so leaving that.
Chown /data last, so if the walk is interrupted the ownership guard still triggers a full walk on the next install.
|
|
||
| os.makedirs("/data/files", exist_ok=True) | ||
| if any(os.stat(p).st_uid != SENTRY_UID for p in ("/data", "/data/files")): | ||
| for root, dirs, files in os.walk("/data", topdown=False): |
There was a problem hiding this comment.
Current os.walk does not fail if something cannot be read for whatever reason (I/O errors mainly is what crosses mind).
Previous find command would have fail.
There was a problem hiding this comment.
Added onerror=fail option to be on par with find /data behavior (we are going to delete it in getsentry/sentry#125848)
os.walk skips directories it cannot list by default, which would leave them root-owned without any error. Raise instead so install.sh stops, like the old entrypoint's find under set -e.
Run the self-hosted `sentry` image as the `sentry` user (uid 999) instead of starting as root and stepping down with `gosu`, like getsentry/snuba#2777. `/data` is created with the right owner at build time, so the entrypoint no longer needs to `chown` it. Production already runs getsentry as 999 via `runAsUser`. Prep for moving the image to a distroless base. **Depends on getsentry/self-hosted#4535 being on self-hosted `master`.** Self-hosted master and relocation validation use `:nightly`, so they'd pick this up within a day of merging.
4 participants
Make self-hosted work with both the current root
sentryimage and the upcoming non-root one (getsentry/sentry#125848, which has to wait for this PR).sentry/Dockerfile: run customizations as root, then switch toUSER sentry. Make/etc/ssl/certswritable bysentryso the entrypoint'supdate-ca-certificateskeeps working for custom CAs.install/ensure-sentry-data-ownership.sh: chownsentry-datato 999 once during install instead of on every container start (the old entrypoint did this as root).cron/Dockerfile: installgosuitself, since the upstream image will drop it.Follow-up for distroless: build the custom CA trust store at install time instead of in the entrypoint.