Skip to content

feat: add S3-compatible storage backend - #4716

Open
Crash-- wants to merge 5 commits into
masterfrom
feat/s3-vfs-backend
Open

feat: add S3-compatible storage backend#4716
Crash-- wants to merge 5 commits into
masterfrom
feat/s3-vfs-backend

Conversation

@Crash--

@Crash-- Crash-- commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add S3-compatible object storage as a third VFS backend (alongside afero and Swift)
  • Uses minio-go/v7 — no AWS SDK dependency. Targets OVH S3, MinIO, Scaleway, and any S3-compatible provider
  • Bucket strategy: one shared bucket per organization (<prefix>-<orgId>), key prefix per instance (<DBPrefix>/)
  • Memory-efficient: single PUT when file size is known (streams like Swift ~32KB), multipart only for unknown size
  • All 17 VFS integration tests pass for the S3 backend

What's included

Area Status
Main VFS (upload, download, delete, versions, fsck) Done
Avatar storage Done
Thumbnail storage Done
App installation (Copier + FileServer) Done
Dynamic assets Done
Preview/icon cache Done
Export archiver Done
Documentation (docs/s3.md) Done
Integration tests (testcontainers + MinIO) Done
Security review fixes (MD5 verification, path traversal, error sanitization) Done

Configuration

fs:
  url: s3://s3.rbx.io.cloud.ovh.net?access_key=ACCESS&secret_key=SECRET&region=rbx&bucket_prefix=cozy&use_ssl=true

All buckets (apps, assets, previews, exports) are created automatically at startup.

Bucket layout

Bucket Content
<prefix>-<orgId> Main VFS data (files, versions, thumbs, avatars)
<prefix>-apps-web Web application assets
<prefix>-apps-konnectors Konnector assets
<prefix>-assets Dynamic assets
<prefix>-previews PDF preview and icon cache
<prefix>-exports Instance export archives

See docs/s3.md for full documentation including a local MinIO setup tutorial.

Test plan

  • Unit tests: 15/15 naming tests pass
  • Integration tests: 17/17 VFS tests pass (afero, swift, s3)
  • Manual API test: upload, download, trash, permanent delete via /files API
  • Manual browser test: Drive UI works, file upload + thumbnail generation confirmed
  • Security review: all critical/high/medium findings fixed
  • Test with OVH S3 in staging

🤖 Generated with Claude Code

@Crash--
Crash-- requested a review from a team as a code owner March 30, 2026 07:07
@Crash--

Crash-- commented Mar 30, 2026

Copy link
Copy Markdown
Contributor Author

I tried it manually with a local minio:

I've:

  • app installation working
  • files upload on Drive working
  • files download on Drive working
  • thumbnails working
  • Avatar Storage.
  • fsck command
  • instance deletion

We need to test the :

  • move
    But maybe we'll test it on public instance, I don't have the move wizard installed & else.

@Crash--

Crash-- commented Mar 30, 2026

Copy link
Copy Markdown
Contributor Author
Enregistrement.de.l.ecran.2026-03-30.a.18.51.51.mov

@Crash--
Crash-- force-pushed the feat/s3-vfs-backend branch from 81d2b27 to 1dacd7c Compare March 31, 2026 03:21
Comment thread model/vfs/vfss3/s3.go Outdated
Comment thread model/vfs/vfss3/s3.go Outdated
@@ -0,0 +1,53 @@
package vfss3

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and a little bit confusing to havein s3.go inly provate delete functions

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or, maybe I understood, there is duplication with appfs, the same functions.
Does it make sense then to move them to pkg/s3 or pkg/s3util package with

  func EnsureBucket(ctx, client, bucket, region) error
  func DeleteObjects(ctx, client, bucket, names) error
  func WrapNotFound(err) error

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — extracted the shared helpers into pkg/s3util with DeleteObjects, DeletePrefixObjects, EnsureBucket, IsNotFound, and WrapNotFound. Both vfss3 and appfs now use this shared package. Also added integration tests for the new package. See commits 643cd8c and c14f0f1.

Comment thread model/vfs/vfss3/impl.go
domain string
prefix string // DBPrefix — used as key prefix in the bucket
contextName string
ctx context.Context

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that we have context in all other vfs, but it's an execution context, not data. Let's do not spread anti-pattern and if not change interface, but at least just pass the context.Background as is

@Crash-- Crash-- Apr 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the concern — storing a context in a struct is generally considered an anti-pattern in Go. However, the VFS interface (model/vfs/vfs.go) does not pass a context.Context in its method signatures, so all backends that need one store it as a field. The Swift implementation (vfsswift/impl_v3.go) uses the exact same pattern: ctx: context.Background() set at creation time. Changing this would require updating the VFS interface, which feels out of scope for this PR - and I'm not confortable with that - . Happy to discuss if you think it's worth a broader refactor though.

Comment thread model/vfs/vfss3/impl.go Outdated
Comment thread model/vfs/vfs_test.go
@@ -17,6 +17,7 @@ import (

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, we don't test any error cases during upload. But we run all the PuObject in the background goroutine, so it would be gread to add test when miniio is down during upload to check that all errors are propagated to client

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Crash--
Crash-- force-pushed the feat/s3-vfs-backend branch 2 times, most recently from f1d60b9 to e82461e Compare April 3, 2026 05:22
@Crash--

Crash-- commented Apr 3, 2026

Copy link
Copy Markdown
Contributor Author

I need to retry every thing since the refacto / changes

@Crash--
Crash-- force-pushed the feat/s3-vfs-backend branch from dad1319 to e622bb4 Compare May 10, 2026 16:18
Crash-- added a commit that referenced this pull request May 22, 2026
Tests in this package call lifecycle.Create / instance.Get directly
without going through testutils.NewSetup → GetTestInstance →
stack.Start, so they never trigger couchdb.InitGlobalDB themselves.

They've historically relied on the side effect of earlier model/* test
packages bootstrapping the global DB and on the design doc persisting
in the shared CouchDB service across test binaries. Go's test result
cache (persisted via actions/setup-go cache) can let those packages be
skipped, breaking the implicit dependency. The CI flake on PR #4716
manifested as TestSyncCreatedOrgContact failing with
"CouchDB(not_found): missing" because instance.Service.Get queried
_design/domain-and-aliases on a global instances DB where that design
doc had never been created.

A more robust fix would be to make instance.Service.Get treat any
CouchDB "not_found" as ErrNotFound (it currently only handles
no_db_file / "Database does not exist."). That would remove the
implicit dependency for every package, not just this one. The
repercussions on other Get callers haven't been fully audited yet, so
this localized bootstrap stays in place until the broader change is
vetted.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Crash-- added a commit that referenced this pull request Jun 1, 2026
Tests in this package call lifecycle.Create / instance.Get directly
without going through testutils.NewSetup → GetTestInstance →
stack.Start, so they never trigger couchdb.InitGlobalDB themselves.

They've historically relied on the side effect of earlier model/* test
packages bootstrapping the global DB and on the design doc persisting
in the shared CouchDB service across test binaries. Go's test result
cache (persisted via actions/setup-go cache) can let those packages be
skipped, breaking the implicit dependency. The CI flake on PR #4716
manifested as TestSyncCreatedOrgContact failing with
"CouchDB(not_found): missing" because instance.Service.Get queried
_design/domain-and-aliases on a global instances DB where that design
doc had never been created.

A more robust fix would be to make instance.Service.Get treat any
CouchDB "not_found" as ErrNotFound (it currently only handles
no_db_file / "Database does not exist."). That would remove the
implicit dependency for every package, not just this one. The
repercussions on other Get callers haven't been fully audited yet, so
this localized bootstrap stays in place until the broader change is
vetted.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
shepilov pushed a commit that referenced this pull request Jun 3, 2026
Tests in this package call lifecycle.Create / instance.Get directly
without going through testutils.NewSetup → GetTestInstance →
stack.Start, so they never trigger couchdb.InitGlobalDB themselves.

They've historically relied on the side effect of earlier model/* test
packages bootstrapping the global DB and on the design doc persisting
in the shared CouchDB service across test binaries. Go's test result
cache (persisted via actions/setup-go cache) can let those packages be
skipped, breaking the implicit dependency. The CI flake on PR #4716
manifested as TestSyncCreatedOrgContact failing with
"CouchDB(not_found): missing" because instance.Service.Get queried
_design/domain-and-aliases on a global instances DB where that design
doc had never been created.

A more robust fix would be to make instance.Service.Get treat any
CouchDB "not_found" as ErrNotFound (it currently only handles
no_db_file / "Database does not exist."). That would remove the
implicit dependency for every package, not just this one. The
repercussions on other Get callers haven't been fully audited yet, so
this localized bootstrap stays in place until the broader change is
vetted.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Crash--

Crash-- commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

@shepilov For your information we've tested this version of the stack and it seems to work.

I'll redo my git history and also, maybe I'll try to be able to chose between s3 and swift not only on env level but also on instance. Like that, I'll be able to move some production instances to S3 in order to check & validate in real usage. WDYT?

@Crash--
Crash-- force-pushed the feat/s3-vfs-backend branch 2 times, most recently from f310be3 to 52655f5 Compare July 16, 2026 09:19
Crash-- added 5 commits July 30, 2026 17:13
Implement a complete vfs.VFS backend for S3-compatible object storage
(OVH, MinIO, Scaleway, ...) using minio-go/v7, mirroring the Swift V3
implementation.

- config: add SchemeS3 and an S3 connection singleton, configured via
  the fs.url query params (access_key, secret_key, region,
  bucket_prefix, use_ssl)
- model/vfs/vfss3: full VFS implementation with a bucket per orgId, a
  key prefix per instance, streaming CreateFile via io.Pipe, and
  multipart uploads with bounded memory
- wire SchemeS3 into every storage dispatch point: VFS, avatars,
  thumbnails, apps copier and file servers, archiver, cache, dynamic
  assets and capabilities
- pkg/s3util: shared S3 helpers (EnsureBucket, DeleteObjects,
  DeletePrefixObjects, IsNotFound, WrapNotFound)
- pkg/appfs and pkg/assets/dynamic: S3 app installation and dynamic
  assets storage
- security hardening: path-traversal protection, S3 error sanitization,
  MD5 integrity verification and bounded reads, plus the code-review
  fixes
- tests: add the S3 backend to the VFS integration suite (MinIO
  testcontainer)
- docs: document the S3 storage backend architecture and configuration
The model/vfs package now exercises three backends (afero, swift, s3)
plus a second MinIO container for S3UploadErrorPropagation, pushing the
package past the previous 5-minute per-package limit on GH runners.
Adds a workflow that builds the production Dockerfile and pushes the
resulting image to ghcr.io/<repo>:s3-test on every push to the
feat/s3-vfs-backend branch, plus a workflow_dispatch trigger that
accepts a custom tag.

Also publishes a ${tag}-<short-sha> tag for traceability.
scripts/build.sh runs `git describe` / `git rev-parse` to derive the
build version string. With recent git versions the COPY'd working tree
trips the "detected dubious ownership" safety check inside the
container (the host user that owned the source no longer owns the
files), causing the build step to exit 128 before producing the
binary. Whitelisting /app as safe restores the previous behaviour.
scripts/build.sh derives the version from git describe / git rev-parse
on the COPY'd working tree, which fails inside the buildx container
(exit 128 with no captured output). Inline `go build` with a build-arg
version string sidesteps the whole bash + git chain.

The workflow passes VERSION_STRING=<tag>-<sha> so the running binary
reports a recognizable version.
@shepilov
shepilov force-pushed the feat/s3-vfs-backend branch from 52655f5 to 4c7b7a9 Compare July 30, 2026 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants