Fix Fleet startup crash on read-only filesystem without S3 bucket - #47099
Conversation
…7090) When no S3 software installers bucket is configured, Fleet falls back to a local filesystem org logo store. NewOrgLogoStore eagerly creates its directory under os.TempDir() (or FLEET_ORG_LOGO_STORE_DIR), and initOrgLogoStore called initFatal when that mkdir failed. On Kubernetes with readOnlyRootFilesystem: true this crashed the pod into CrashLoopBackOff. The other stores that reuse the software installers bucket (software installer, title icon) already degrade to a "failing" store and log the error instead of exiting. Make the org logo store follow that same pattern: log the error and fall back to a FailingOrgLogoStore so Fleet still boots. Custom org logos are simply unavailable for that deployment, which is the existing contract for the other filesystem-backed assets.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Pull request overview
Fixes a Fleet server startup crash when running with a read-only root filesystem and no S3 software installers bucket configured by degrading org logo storage to a “failing” store instead of exiting.
Changes:
- Update
initOrgLogoStoreto log filesystem initialization failures and fall back to a failing org logo store. - Add a dedicated
FailingOrgLogoStoreimplementation for org logos (mirrors existing failing stores for related assets). - Add a unit test covering both the filesystem-backed and fallback failing-store paths.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| server/datastore/failing/org_logo.go | Adds a failing org logo store implementation used when local filesystem storage cannot be initialized. |
| cmd/fleet/serve.go | Changes org logo store initialization to log-and-degrade (instead of fatal) on local filesystem setup failures. |
| cmd/fleet/serve_test.go | Adds TestInitOrgLogoStore to validate the new fallback behavior. |
| changes/47090-org-logo-readonly-fs | Adds release note entry describing the startup-crash fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThis PR moves org logo storage from the filesystem to the database as a fallback mechanism. When S3 is not configured, the system now stores custom org logos in a new 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #47099 +/- ##
=======================================
Coverage 67.17% 67.17%
=======================================
Files 2910 2912 +2
Lines 226140 226182 +42
Branches 11865 11865
=======================================
+ Hits 151900 151935 +35
Misses 60510 60510
- Partials 13730 13737 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
So the UX is that setup succeeds (you can "set" a logo) but then the logo doesn't work? The org logo uses the software installer bucket for storage, given Fleet Free doesn't have (doesn't need) one then we may need to make some changes to properly support Fleet Free (maybe resort to URL for logos?). |
|
Re |
Yes. I think the ideal fix would be to have a brand new bucket for logos instead of reusing the software installers bucket.
Perhaps we can share a feature flag/capability (e.g. "custom_org_logos_enabled") which is set to true/false depending on whether the bucket/filesystem was properly initialized (this would be set on init on the server and then shared in the config or similar with the FE). |
|
@coderabbitai full review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/fleet/serve.go (1)
1520-1530:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve existing filesystem-backed logos on upgrade.
Line 1520 switches the no-S3 path from the legacy filesystem store to
ds.NewOrgLogoStore()with no compatibility read or migration. Any deployment that previously relied on the local org-logo directory will stop serving its existing custom logo after upgrade until it is manually re-uploaded. Please add a migration/read-through path for the old filesystem location, or explicitly call out the manual recovery step before release.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/fleet/serve.go` around lines 1520 - 1530, initOrgLogoStore currently drops support for the legacy filesystem org-logo directory when S3 is not configured by directly returning ds.NewOrgLogoStore(), which causes existing logos to disappear after upgrade; update initOrgLogoStore to detect the legacy filesystem store (the old org-logo directory), implement a read-through/migration path that on startup reads existing filesystem logos and either (a) migrates them into the datastore returned by ds.NewOrgLogoStore() or (b) wraps ds.NewOrgLogoStore() with a fallback that serves from the filesystem when the datastore has no logo, ensuring calls to NewOrgLogoStore() continue to succeed; reference the existing symbols initOrgLogoStore, s3.NewOrgLogoStore and ds.NewOrgLogoStore when adding the migration or fallback logic and ensure failures are logged via the provided logger rather than silently dropping legacy logos.
🧹 Nitpick comments (2)
server/datastore/mysql/org_logo_store.go (1)
25-35: ⚡ Quick winReject non-storable org logo modes in
Put.
Putcan currently persistmode="all"even though only light/dark are storable. Add a mode guard before the write to keep storage consistent with the mode contract.Proposed change
import ( "bytes" "context" "database/sql" "errors" + "fmt" "io" @@ func (s *orgLogoStore) Put(ctx context.Context, mode fleet.OrgLogoMode, content io.ReadSeeker) error { + if !mode.IsStorable() { + return ctxerr.Wrap(ctx, fmt.Errorf("unsupported org logo mode: %s", mode), "storing org logo") + } + data, err := io.ReadAll(content) if err != nil { return ctxerr.Wrap(ctx, err, "reading org logo content") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/org_logo_store.go` around lines 25 - 35, The Put method on orgLogoStore accepts any fleet.OrgLogoMode and currently allows non-storable modes (e.g., "all") to be persisted; add an explicit guard at the start of orgLogoStore.Put to validate mode is one of the storable values (light or dark) and return an error for other modes before reading the content or executing the INSERT. Locate the orgLogoStore.Put function and check mode against the allowed constants/strings for fleet.OrgLogoMode (e.g., "light"/"dark") and return a clear error (wrapped with ctxerr.Wrap or similar) if invalid, then proceed with the existing ReadAll and DB Exec only when the mode is valid.server/datastore/mysql/org_logo_store_test.go (1)
12-64: ⚡ Quick winAdd a test for non-storable mode rejection.
Please add an assertion that
fleet.OrgLogoModeAllis rejected byPutto lock in the storage-mode invariant.Proposed test addition
func TestOrgLogoStore(t *testing.T) { @@ // Delete is idempotent. require.NoError(t, store.Delete(ctx, fleet.OrgLogoModeLight)) @@ require.False(t, exists) require.NoError(t, store.Delete(ctx, fleet.OrgLogoModeLight)) + + // Non-storable mode is rejected. + err = store.Put(ctx, fleet.OrgLogoModeAll, bytes.NewReader(light)) + require.Error(t, err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/org_logo_store_test.go` around lines 12 - 64, Add an assertion in TestOrgLogoStore that calling store.Put with the disallowed mode fleet.OrgLogoModeAll is rejected: call err := store.Put(ctx, fleet.OrgLogoModeAll, bytes.NewReader(light)) and require.Error(t, err); if your codebase exposes a specific sentinel helper (e.g. fleet.IsNotAllowed or similar), also assert require.True(t, fleet.IsNotAllowed(err)) to lock in the storage-mode invariant and ensure OrgLogoModeAll cannot be stored.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cmd/fleet/serve.go`:
- Around line 1520-1530: initOrgLogoStore currently drops support for the legacy
filesystem org-logo directory when S3 is not configured by directly returning
ds.NewOrgLogoStore(), which causes existing logos to disappear after upgrade;
update initOrgLogoStore to detect the legacy filesystem store (the old org-logo
directory), implement a read-through/migration path that on startup reads
existing filesystem logos and either (a) migrates them into the datastore
returned by ds.NewOrgLogoStore() or (b) wraps ds.NewOrgLogoStore() with a
fallback that serves from the filesystem when the datastore has no logo,
ensuring calls to NewOrgLogoStore() continue to succeed; reference the existing
symbols initOrgLogoStore, s3.NewOrgLogoStore and ds.NewOrgLogoStore when adding
the migration or fallback logic and ensure failures are logged via the provided
logger rather than silently dropping legacy logos.
---
Nitpick comments:
In `@server/datastore/mysql/org_logo_store_test.go`:
- Around line 12-64: Add an assertion in TestOrgLogoStore that calling store.Put
with the disallowed mode fleet.OrgLogoModeAll is rejected: call err :=
store.Put(ctx, fleet.OrgLogoModeAll, bytes.NewReader(light)) and
require.Error(t, err); if your codebase exposes a specific sentinel helper (e.g.
fleet.IsNotAllowed or similar), also assert require.True(t,
fleet.IsNotAllowed(err)) to lock in the storage-mode invariant and ensure
OrgLogoModeAll cannot be stored.
In `@server/datastore/mysql/org_logo_store.go`:
- Around line 25-35: The Put method on orgLogoStore accepts any
fleet.OrgLogoMode and currently allows non-storable modes (e.g., "all") to be
persisted; add an explicit guard at the start of orgLogoStore.Put to validate
mode is one of the storable values (light or dark) and return an error for other
modes before reading the content or executing the INSERT. Locate the
orgLogoStore.Put function and check mode against the allowed constants/strings
for fleet.OrgLogoMode (e.g., "light"/"dark") and return a clear error (wrapped
with ctxerr.Wrap or similar) if invalid, then proceed with the existing ReadAll
and DB Exec only when the mode is valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a55b0b22-f854-4471-840c-0fe0378d431c
📒 Files selected for processing (8)
changes/47090-org-logo-readonly-fscmd/fleet/serve.gocmd/fleet/serve_test.goserver/datastore/mysql/migrations/tables/20260608173427_CreateOrgLogoTable.goserver/datastore/mysql/migrations/tables/20260608173427_CreateOrgLogoTable_test.goserver/datastore/mysql/org_logo_store.goserver/datastore/mysql/org_logo_store_test.goserver/datastore/mysql/schema.sql
…AtIndex (#47168) Follow-up to #47099. Re-timestamps the `CreateOrgLogoTable` migration so it can also ship in the **4.86.2** patch. `CreateOrgLogoTable` must sort **immediately after** `20260527215817_AddHostCertificatesOriginDeletedAtIndex` (the last migration in 4.86.1). ## Testing - Started server with latest `main` migrations applied. It failed. - Ran `UPDATE migration_status_tables SET version_id = 20260527215818 WHERE version_id = 20260608173427;`. - Started server, no crashes. <img width="1458" height="628" alt="Screenshot 2026-06-09 at 10 19 40 AM" src="https://github.com/user-attachments/assets/68d24712-78ae-42ca-8989-60c39647dd33" /> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated database migration infrastructure and corresponding tests to maintain consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Cherry-pick of #47168 into the `rc-minor-fleet-v4.87.0` RC branch. Follows the cherry-pick of #47099 (#47166, already merged here). This re-timestamps the `CreateOrgLogoTable` migration `20260608173427` → `20260527215818` so it sorts immediately after `AddHostCertificatesOriginDeletedAtIndex`(required so the same migration can also ship in the 4.86.2 patch without breaking the 4.86.2 → 4.87.0 upgrade path). Note: `schema.sql` conflicted (main regenerated it against main's migration set, which differs from this RC branch) because [20260608202705_AddVulnPerfIndexes.go](https://github.com/fleetdm/fleet/blob/main/server/datastore/mysql/migrations/tables/20260608202705_AddVulnPerfIndexes.go) and [20260608210432_CleanupSoftwareLastOpenedAtSentinels.go](https://github.com/fleetdm/fleet/blob/main/server/datastore/mysql/migrations/tables/20260608210432_CleanupSoftwareLastOpenedAtSentinels.go) are not part of the RC branch yet (looks like the PRs that introduced them on main have not been cherry-picked yet).
Related issue: Resolves #47090
Fleet crashes into
CrashLoopBackOffon startup when deployed on Kubernetes withreadOnlyRootFilesystem: trueand no S3 software installers bucket configured:I realised I was calling
initFatalwhen failing to create a directory on the filesystem which doesn't match the pattern oflogging+creating a "failing" store(one that is initialized but fails all operations) as we do for e.g. software title icons (see NewFailingSoftwareTitleIconStore).Per this slack conversation: https://fleetdm.slack.com/archives/C084F4MKYSJ/p1780931127976389, we decided to fall back to a database-backed storage:
Checklist for submitter
changes/.Testing
Commented out this line to force filesystem usage:
Before
Server crashes
After
Server starts and logo upload works
Screen.Recording.2026-06-08.at.3.00.49.PM.mov
Screen.Recording.2026-06-08.at.2.53.37.PM.mov
Summary by CodeRabbit
Release Notes