From 450178e4bd1a44cd9eb82765cb27bb1ab88a8842 Mon Sep 17 00:00:00 2001 From: Pavel Tumik <18602811+sagor999@users.noreply.github.com> Date: Mon, 23 May 2022 20:24:54 +0000 Subject: [PATCH] [content-service] fix s3 access when using dedicated bucket --- .../content-service/pkg/layer/provider_test.go | 4 ++-- .../pkg/service/headless-log-service.go | 4 ++-- .../pkg/service/headless-log-service_test.go | 2 +- .../pkg/service/workspace-service.go | 10 +++++----- .../content-service/pkg/storage/gcloud.go | 6 +++--- .../content-service/pkg/storage/minio.go | 18 +++++++++++++----- .../content-service/pkg/storage/mock/mock.go | 16 ++++++++-------- components/content-service/pkg/storage/noop.go | 4 ++-- .../content-service/pkg/storage/storage.go | 4 ++-- 9 files changed, 38 insertions(+), 30 deletions(-) diff --git a/components/content-service/pkg/layer/provider_test.go b/components/content-service/pkg/layer/provider_test.go index c886b925d84578..b499cbd591d973 100644 --- a/components/content-service/pkg/layer/provider_test.go +++ b/components/content-service/pkg/layer/provider_test.go @@ -244,11 +244,11 @@ func (s *testStorage) DeleteBucket(ctx context.Context, bucket string) error { return nil } -func (*testStorage) BackupObject(workspaceID string, name string) string { +func (*testStorage) BackupObject(ownerID string, workspaceID string, name string) string { return "" } -func (*testStorage) InstanceObject(workspaceID string, instanceID string, name string) string { +func (*testStorage) InstanceObject(ownerID string, workspaceID string, instanceID string, name string) string { return "" } diff --git a/components/content-service/pkg/service/headless-log-service.go b/components/content-service/pkg/service/headless-log-service.go index 35044bd3bbdb39..974a54e28a8400 100644 --- a/components/content-service/pkg/service/headless-log-service.go +++ b/components/content-service/pkg/service/headless-log-service.go @@ -54,7 +54,7 @@ func (ls *HeadlessLogService) LogDownloadURL(ctx context.Context, req *api.LogDo span.SetTag("instanceId", req.InstanceId) defer tracing.FinishSpan(span, &err) - blobName := ls.s.InstanceObject(req.WorkspaceId, req.InstanceId, logs.UploadedHeadlessLogPath(req.TaskId)) + blobName := ls.s.InstanceObject(req.OwnerId, req.WorkspaceId, req.InstanceId, logs.UploadedHeadlessLogPath(req.TaskId)) info, err := ls.s.SignDownload(ctx, ls.s.Bucket(req.OwnerId), blobName, &storage.SignedURLOptions{}) if err != nil { log.WithFields(log.OWI(req.OwnerId, req.WorkspaceId, "")). @@ -87,7 +87,7 @@ func (ls *HeadlessLogService) ListLogs(ctx context.Context, req *api.ListLogsReq // we do not need to check whether the bucket exists because ListObjects() does that for us // all files under this prefix are headless log files, named after their respective taskId - prefix := ls.s.InstanceObject(req.WorkspaceId, req.InstanceId, logs.UploadedHeadlessLogPathPrefix) + prefix := ls.s.InstanceObject(req.OwnerId, req.WorkspaceId, req.InstanceId, logs.UploadedHeadlessLogPathPrefix) objects, err := da.ListObjects(ctx, prefix) if err != nil { return nil, err diff --git a/components/content-service/pkg/service/headless-log-service_test.go b/components/content-service/pkg/service/headless-log-service_test.go index e4771d0db4fe19..e4a4b2a365f424 100644 --- a/components/content-service/pkg/service/headless-log-service_test.go +++ b/components/content-service/pkg/service/headless-log-service_test.go @@ -70,7 +70,7 @@ func TestListLogs(t *testing.T) { daFactory: daFactory, } - s.EXPECT().InstanceObject(gomock.Any(), gomock.Any(), gomock.Any()). + s.EXPECT().InstanceObject(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). Return("") da.EXPECT().Init(gomock.Any(), gomock.Eq(OwnerId), gomock.Eq(WorkspaceId), gomock.Not(gomock.Eq(""))). Times(1) diff --git a/components/content-service/pkg/service/workspace-service.go b/components/content-service/pkg/service/workspace-service.go index 0c17ba613eea08..44c4d33667ae6e 100644 --- a/components/content-service/pkg/service/workspace-service.go +++ b/components/content-service/pkg/service/workspace-service.go @@ -44,7 +44,7 @@ func (cs *WorkspaceService) WorkspaceDownloadURL(ctx context.Context, req *api.W span.SetTag("workspaceId", req.WorkspaceId) defer tracing.FinishSpan(span, &err) - blobName := cs.s.BackupObject(req.WorkspaceId, storage.DefaultBackup) + blobName := cs.s.BackupObject(req.OwnerId, req.WorkspaceId, storage.DefaultBackup) info, err := cs.s.SignDownload(ctx, cs.s.Bucket(req.OwnerId), blobName, &storage.SignedURLOptions{}) if err != nil { @@ -73,7 +73,7 @@ func (cs *WorkspaceService) DeleteWorkspace(ctx context.Context, req *api.Delete defer tracing.FinishSpan(span, &err) if req.IncludeSnapshots { - prefix := cs.s.BackupObject(req.WorkspaceId, "") + prefix := cs.s.BackupObject(req.OwnerId, req.WorkspaceId, "") if !strings.HasSuffix(prefix, "/") { prefix = prefix + "/" } @@ -89,7 +89,7 @@ func (cs *WorkspaceService) DeleteWorkspace(ctx context.Context, req *api.Delete return &api.DeleteWorkspaceResponse{}, nil } - blobName := cs.s.BackupObject(req.WorkspaceId, storage.DefaultBackup) + blobName := cs.s.BackupObject(req.OwnerId, req.WorkspaceId, storage.DefaultBackup) err = cs.s.DeleteObject(ctx, cs.s.Bucket(req.OwnerId), &storage.DeleteObjectQuery{Name: blobName}) if err != nil { if errors.Is(err, storage.ErrNotFound) { @@ -100,7 +100,7 @@ func (cs *WorkspaceService) DeleteWorkspace(ctx context.Context, req *api.Delete return nil, status.Error(codes.Unknown, err.Error()) } - trailPrefix := cs.s.BackupObject(req.WorkspaceId, "trail-") + trailPrefix := cs.s.BackupObject(req.OwnerId, req.WorkspaceId, "trail-") err = cs.s.DeleteObject(ctx, cs.s.Bucket(req.OwnerId), &storage.DeleteObjectQuery{Prefix: trailPrefix}) if err != nil { if errors.Is(err, storage.ErrNotFound) { @@ -121,7 +121,7 @@ func (cs *WorkspaceService) WorkspaceSnapshotExists(ctx context.Context, req *ap span.SetTag("filename", req.Filename) defer tracing.FinishSpan(span, &err) - exists, err := cs.s.ObjectExists(ctx, cs.s.Bucket(req.OwnerId), cs.s.BackupObject(req.WorkspaceId, req.Filename)) + exists, err := cs.s.ObjectExists(ctx, cs.s.Bucket(req.OwnerId), cs.s.BackupObject(req.OwnerId, req.WorkspaceId, req.Filename)) if err != nil { return nil, status.Error(codes.Unknown, err.Error()) } diff --git a/components/content-service/pkg/storage/gcloud.go b/components/content-service/pkg/storage/gcloud.go index 1b2fb289038d39..9d5e1958b6dc9b 100644 --- a/components/content-service/pkg/storage/gcloud.go +++ b/components/content-service/pkg/storage/gcloud.go @@ -870,11 +870,11 @@ func (p *PresignedGCPStorage) ObjectExists(ctx context.Context, bucket, obj stri } // BackupObject returns a backup's object name that a direct downloader would download -func (p *PresignedGCPStorage) BackupObject(workspaceID string, name string) string { +func (p *PresignedGCPStorage) BackupObject(ownerID string, workspaceID string, name string) string { return fmt.Sprintf("workspaces/%s", gcpWorkspaceBackupObjectName(workspaceID, name)) } // InstanceObject returns a instance's object name that a direct downloader would download -func (p *PresignedGCPStorage) InstanceObject(workspaceID string, instanceID string, name string) string { - return p.BackupObject(workspaceID, InstanceObjectName(instanceID, name)) +func (p *PresignedGCPStorage) InstanceObject(ownerID string, workspaceID string, instanceID string, name string) string { + return p.BackupObject(ownerID, workspaceID, InstanceObjectName(instanceID, name)) } diff --git a/components/content-service/pkg/storage/minio.go b/components/content-service/pkg/storage/minio.go index 2c9bd175aac64a..6149855810ef91 100644 --- a/components/content-service/pkg/storage/minio.go +++ b/components/content-service/pkg/storage/minio.go @@ -339,7 +339,11 @@ func (rs *DirectMinIOStorage) bucketName() string { } func (rs *DirectMinIOStorage) objectName(name string) string { - return minioWorkspaceBackupObjectName(rs.Username, rs.WorkspaceName, name) + var username string + if rs.MinIOConfig.BucketName != "" { + username = rs.Username + } + return minioWorkspaceBackupObjectName(username, rs.WorkspaceName, name) } func newPresignedMinIOAccess(cfg config.MinIOConfig) (*presignedMinIOStorage, error) { @@ -532,13 +536,17 @@ func (s *presignedMinIOStorage) BlobObject(name string) (string, error) { } // BackupObject returns a backup's object name that a direct downloader would download -func (s *presignedMinIOStorage) BackupObject(workspaceID, name string) string { - return minioWorkspaceBackupObjectName("", workspaceID, name) +func (s *presignedMinIOStorage) BackupObject(ownerID string, workspaceID, name string) string { + var username string + if s.MinIOConfig.BucketName != "" { + username = ownerID + } + return minioWorkspaceBackupObjectName(username, workspaceID, name) } // InstanceObject returns a instance's object name that a direct downloader would download -func (s *presignedMinIOStorage) InstanceObject(workspaceID string, instanceID string, name string) string { - return s.BackupObject(workspaceID, InstanceObjectName(instanceID, name)) +func (s *presignedMinIOStorage) InstanceObject(ownerID string, workspaceID string, instanceID string, name string) string { + return s.BackupObject(ownerID, workspaceID, InstanceObjectName(instanceID, name)) } func translateMinioError(err error) error { diff --git a/components/content-service/pkg/storage/mock/mock.go b/components/content-service/pkg/storage/mock/mock.go index d826ff5d3572a8..62c8b4bc141e24 100644 --- a/components/content-service/pkg/storage/mock/mock.go +++ b/components/content-service/pkg/storage/mock/mock.go @@ -41,17 +41,17 @@ func (m *MockPresignedAccess) EXPECT() *MockPresignedAccessMockRecorder { } // BackupObject mocks base method. -func (m *MockPresignedAccess) BackupObject(arg0, arg1 string) string { +func (m *MockPresignedAccess) BackupObject(arg0, arg1, arg2 string) string { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "BackupObject", arg0, arg1) + ret := m.ctrl.Call(m, "BackupObject", arg0, arg1, arg2) ret0, _ := ret[0].(string) return ret0 } // BackupObject indicates an expected call of BackupObject. -func (mr *MockPresignedAccessMockRecorder) BackupObject(arg0, arg1 interface{}) *gomock.Call { +func (mr *MockPresignedAccessMockRecorder) BackupObject(arg0, arg1, arg2 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "BackupObject", reflect.TypeOf((*MockPresignedAccess)(nil).BackupObject), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "BackupObject", reflect.TypeOf((*MockPresignedAccess)(nil).BackupObject), arg0, arg1, arg2) } // BlobObject mocks base method. @@ -141,17 +141,17 @@ func (mr *MockPresignedAccessMockRecorder) EnsureExists(arg0, arg1 interface{}) } // InstanceObject mocks base method. -func (m *MockPresignedAccess) InstanceObject(arg0, arg1, arg2 string) string { +func (m *MockPresignedAccess) InstanceObject(arg0, arg1, arg2, arg3 string) string { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "InstanceObject", arg0, arg1, arg2) + ret := m.ctrl.Call(m, "InstanceObject", arg0, arg1, arg2, arg3) ret0, _ := ret[0].(string) return ret0 } // InstanceObject indicates an expected call of InstanceObject. -func (mr *MockPresignedAccessMockRecorder) InstanceObject(arg0, arg1, arg2 interface{}) *gomock.Call { +func (mr *MockPresignedAccessMockRecorder) InstanceObject(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "InstanceObject", reflect.TypeOf((*MockPresignedAccess)(nil).InstanceObject), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "InstanceObject", reflect.TypeOf((*MockPresignedAccess)(nil).InstanceObject), arg0, arg1, arg2, arg3) } // ObjectExists mocks base method. diff --git a/components/content-service/pkg/storage/noop.go b/components/content-service/pkg/storage/noop.go index 6dd4581ba94191..b62957a1963e3d 100644 --- a/components/content-service/pkg/storage/noop.go +++ b/components/content-service/pkg/storage/noop.go @@ -121,11 +121,11 @@ func (p *PresignedNoopStorage) ObjectExists(ctx context.Context, bucket, obj str } // BackupObject returns a backup's object name that a direct downloader would download -func (*PresignedNoopStorage) BackupObject(workspaceID string, name string) string { +func (*PresignedNoopStorage) BackupObject(ownerID string, workspaceID string, name string) string { return "" } // InstanceObject returns a instance's object name that a direct downloader would download -func (*PresignedNoopStorage) InstanceObject(workspaceID string, instanceID string, name string) string { +func (*PresignedNoopStorage) InstanceObject(ownerID string, workspaceID string, instanceID string, name string) string { return "" } diff --git a/components/content-service/pkg/storage/storage.go b/components/content-service/pkg/storage/storage.go index f38dac80faff2c..88ffbceabeda94 100644 --- a/components/content-service/pkg/storage/storage.go +++ b/components/content-service/pkg/storage/storage.go @@ -89,10 +89,10 @@ type PresignedAccess interface { ObjectExists(ctx context.Context, bucket string, path string) (bool, error) // BackupObject returns a backup's object name that a direct downloader would download - BackupObject(workspaceID string, name string) string + BackupObject(ownerID string, workspaceID string, name string) string // InstanceObject returns a instance's object name that a direct downloader would download - InstanceObject(workspaceID string, instanceID string, name string) string + InstanceObject(ownerID string, workspaceID string, instanceID string, name string) string } // ObjectMeta describtes the metadata of a remote object