-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[content-service] Add support to use existing S3 bucket #10073
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
8519e01
to
5401386
Compare
/werft run 👍 started the job as gitpod-build-aledbf-bucket.7 |
Configuration example:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome to see this change! 🚀
Added a few comments regarding backward compatibility. 🤔
Follow-up issue: Add config for S3 bucket to KOTS (Replicated) installer. |
Follow-up issue: Document S3 bucket config in self-hosted docs |
} | ||
|
||
func (rs *DirectMinIOStorage) objectName(name string) string { | ||
return minioWorkspaceBackupObjectName(rs.WorkspaceName, name) | ||
return minioWorkspaceBackupObjectName(rs.Username, rs.WorkspaceName, name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return minioWorkspaceBackupObjectName(rs.Username, rs.WorkspaceName, name) | |
var username string | |
if rs.MinIOConfig.BucketName != "" { | |
username = rs.Username | |
} | |
return minioWorkspaceBackupObjectName(username, rs.WorkspaceName, name) | |
that way if bucket name is set, then we default to new way of doing things, otherwise we maintain backwards compatibility to old way of accessing buckets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please add unit tests that would ensure old code is working correctly, and new one also generates correct bucket and path.
@corneliusludmann @aledbf I added tests and fixed directMinio and presignedMinio access. Please double check tests that I added to ensure they are correct. This also ensures backward compatibility (rolling out this update should not break any existing self hosted installations). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with tests it looks good now (feels weird approving my own changes to PR 😃 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -105,6 +105,8 @@ type MinIOConfig struct { | |||
|
|||
Region string `json:"region"` | |||
ParallelUpload uint `json:"parallelUpload,omitempty"` | |||
|
|||
BucketName string `json:"bucket"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BucketName string `json:"bucket"` | |
BucketName string `json:"bucket,omitempty"` |
Do we need omitempty
here in case the value is not given or for backward compatibility reasons?
I don't think so but wanted to raise this question here just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@corneliusludmann good catch! Since that param is optional, I added omitempty to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in supervisor mod 🙈
Description
This PR introduces a new "single bucket mode" to the S3 storage implementation.
In some scenarios, we could not have enough permissions to create buckets. In those cases the "one-bucket per user" implementation fails.
If there's a bucket name configured (see installer/content-service config change), that bucket name is used and the user ID is prefixed to the object name. If no such bucket name is configured, the old "one bucket per user" behaviour kicks in. This way, users can maintain backwards compatibility for now. There is no automatic migration or "hybrid mode" where we fall back to the old behaviour. We don't have enough installations in the wild which were able to actually use the old behaviour to justify introducing such a mode.
We maintain the old mode in GCP (and maybe even S3) for now because
How to test
Precursor: ensure the installation uses minio/S3, and that the
bucket
is configuredRelease Notes
Fixes #10070