Skip to content

feat: update tenant management RFC to include multipart uploads - #17

Open
alanshaw wants to merge 3 commits into
mainfrom
ash/feat/multipart-uploads
Open

feat: update tenant management RFC to include multipart uploads#17
alanshaw wants to merge 3 commits into
mainfrom
ash/feat/multipart-uploads

Conversation

@alanshaw

@alanshaw alanshaw commented Jul 29, 2026

Copy link
Copy Markdown
Member

馃摉 Preview

Summary

This PR just updates the existing RFC for tenant management to document the S3 permissions and Forge commands required to support multipart uploads.

@alanshaw
alanshaw requested a review from frrist July 29, 2026 08:11
| `s3:GetObjectVersion` | `/content/retrieve` |
| `s3:GetObjectRetention` | `/content/retrieve` |
| `s3:GetObjectLegalHold` | `/content/retrieve` |
| `s3:PutObject` | `/content/retrieve`, `/blob/add`, `/index/add`, `/upload/add`, `/blob/abort` |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

as written this needs /blob/remove, or blobs need to be parked until Complete (conflicts with https://github.com/fil-one/RFC/pull/17/changes#diff-18ca746d83487824ea53f04b055a5ba3b0002c06c981c108aab5ac81b70efb92R232) in case of e.g. re-uploading, otherwise we need specific GC machinery with its own state

@parkan parkan Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

also for overwrites (S3 semantics require only s3:PutObject for that unless locked/versioned/etc) but we'd need to remove first

we could say we default to If-None-Match and 412/409 but this needs to be explicit

generally speaking not requiring /blob/remove as baseline for uploads seems good but as currently specified I see at least two cases where this isn't conformant

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

鉂わ笍 thank you - we also realised this and addressed in fil-forge/hilt#37

I'll update here also.

Comment on lines +126 to +127
| `s3:PutObjectRetention` | `/content/retrieve`, `/blob/add`, `/index/add`, `/upload/add`, `/blob/abort` |
| `s3:PutObjectLegalHold` | `/content/retrieve`, `/blob/add`, `/index/add`, `/upload/add`, `/blob/abort` |

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.

Why do these two S3 permissions need /blob/abort? Any why they don't need /blob/remove similarly to s3:PutObject?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point. I don't think they need /blob/abort...they don't need /blob/remove because IIRC it is writing local state, not actually writing to the Forge network (which is always a remove then add operation when updating due to immutable data).

Comment thread rfcs/2026-06-forge-s3-tenant-management.md Outdated
Co-authored-by: Miroslav Bajto拧 <oss@bajtos.net>
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.

3 participants