From 06b32ee76021ecdcd11172de310b8a1441e71ba4 Mon Sep 17 00:00:00 2001 From: Itai Gilo Date: Sun, 26 Jan 2025 16:08:35 +0200 Subject: [PATCH 01/14] Add StorageID to adapter metadata functions --- cmd/lakefs/cmd/run.go | 5 +- pkg/api/controller.go | 83 ++++++++++--------- pkg/block/adapter.go | 8 +- pkg/block/azure/adapter.go | 8 +- pkg/block/gs/adapter.go | 10 +-- pkg/block/local/adapter.go | 8 +- pkg/block/mem/adapter.go | 8 +- pkg/block/metrics.go | 52 ++++++------ pkg/block/s3/adapter.go | 10 +-- pkg/block/transient/adapter.go | 8 +- pkg/block/validations.go | 2 +- .../retention/garbage_collection_manager.go | 9 +- pkg/testutil/adapter.go | 8 +- 13 files changed, 113 insertions(+), 106 deletions(-) diff --git a/cmd/lakefs/cmd/run.go b/cmd/lakefs/cmd/run.go index be99b374581..bdb85666843 100644 --- a/cmd/lakefs/cmd/run.go +++ b/cmd/lakefs/cmd/run.go @@ -430,7 +430,8 @@ func checkRepos(ctx context.Context, logger logging.Logger, authMetadataManager logger.Debug("lakeFS isn't initialized, skipping mismatched adapter checks") } else { logger. - WithField("adapter_type", blockStore.BlockstoreType()). + // TODO (gilo): uncomment this? + //WithField("adapter_type", blockStore.BlockstoreType()). Debug("lakeFS is initialized, checking repositories for mismatched adapter") hasMore := true next := "" @@ -443,8 +444,8 @@ func checkRepos(ctx context.Context, logger logging.Logger, authMetadataManager logger.WithError(err).Fatal("Checking existing repositories failed") } - adapterStorageType := blockStore.BlockstoreType() for _, repo := range repos { + adapterStorageType := blockStore.BlockstoreType(repo.StorageID) nsURL, err := url.Parse(repo.StorageNamespace) if err != nil { logger.WithError(err).Fatalf("Failed to parse repository %s namespace '%s'", repo.Name, repo.StorageNamespace) diff --git a/pkg/api/controller.go b/pkg/api/controller.go index e42e41fe3a5..b40292e5bbc 100644 --- a/pkg/api/controller.go +++ b/pkg/api/controller.go @@ -167,15 +167,15 @@ func (c *Controller) CreatePresignMultipartUpload(w http.ResponseWriter, r *http ctx := r.Context() c.LogAction(ctx, "create_presign_multipart_upload", r, repository, branch, "") - // check if api is supported - storageConfig := c.getStorageConfig() - if !swag.BoolValue(storageConfig.PreSignMultipartUpload) { - writeError(w, r, http.StatusNotImplemented, "presign multipart upload API is not supported") + repo, err := c.Catalog.GetRepository(ctx, repository) + if c.handleAPIError(ctx, w, r, err) { return } - repo, err := c.Catalog.GetRepository(ctx, repository) - if c.handleAPIError(ctx, w, r, err) { + // check if api is supported + storageConfig := c.getStorageConfig(repo.StorageID) + if !swag.BoolValue(storageConfig.PreSignMultipartUpload) { + writeError(w, r, http.StatusNotImplemented, "presign multipart upload API is not supported") return } @@ -211,7 +211,7 @@ func (c *Controller) CreatePresignMultipartUpload(w http.ResponseWriter, r *http return } - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, address, block.IdentifierTypeRelative) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, address, block.IdentifierTypeRelative) if err != nil { writeError(w, r, http.StatusInternalServerError, err) return @@ -267,8 +267,13 @@ func (c *Controller) AbortPresignMultipartUpload(w http.ResponseWriter, r *http. ctx := r.Context() c.LogAction(ctx, "abort_presign_multipart_upload", r, repository, branch, "") + repo, err := c.Catalog.GetRepository(ctx, repository) + if c.handleAPIError(ctx, w, r, err) { + return + } + // check if api is supported - storageConfig := c.getStorageConfig() + storageConfig := c.getStorageConfig(repo.StorageID) if !swag.BoolValue(storageConfig.PreSignMultipartUpload) { writeError(w, r, http.StatusNotImplemented, "presign multipart upload API is not supported") return @@ -288,11 +293,6 @@ func (c *Controller) AbortPresignMultipartUpload(w http.ResponseWriter, r *http. return } - repo, err := c.Catalog.GetRepository(ctx, repository) - if c.handleAPIError(ctx, w, r, err) { - return - } - // verify physical address physicalAddress, addressType := normalizePhysicalAddress(repo.StorageNamespace, body.PhysicalAddress) if addressType != catalog.AddressTypeRelative { @@ -327,8 +327,14 @@ func (c *Controller) CompletePresignMultipartUpload(w http.ResponseWriter, r *ht ctx := r.Context() c.LogAction(ctx, "complete_presign_multipart_upload", r, repository, branch, "") + // verify physical address + repo, err := c.Catalog.GetRepository(ctx, repository) + if c.handleAPIError(ctx, w, r, err) { + return + } + // check if api is supported - storageConfig := c.getStorageConfig() + storageConfig := c.getStorageConfig(repo.StorageID) if !swag.BoolValue(storageConfig.PreSignMultipartUpload) { writeError(w, r, http.StatusNotImplemented, "presign multipart upload API is not supported") return @@ -352,12 +358,6 @@ func (c *Controller) CompletePresignMultipartUpload(w http.ResponseWriter, r *ht return } - // verify physical address - repo, err := c.Catalog.GetRepository(ctx, repository) - if c.handleAPIError(ctx, w, r, err) { - return - } - physicalAddress, addressType := normalizePhysicalAddress(repo.StorageNamespace, body.PhysicalAddress) if addressType != catalog.AddressTypeRelative { writeError(w, r, http.StatusBadRequest, "physical address must be relative to the storage namespace") @@ -696,7 +696,7 @@ func (c *Controller) GetPhysicalAddress(w http.ResponseWriter, r *http.Request, return } - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, address, block.IdentifierTypeRelative) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, address, block.IdentifierTypeRelative) if err != nil { writeError(w, r, http.StatusInternalServerError, err) return @@ -750,7 +750,7 @@ func (c *Controller) LinkPhysicalAddress(w http.ResponseWriter, r *http.Request, return } // write metadata - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, params.Path, block.IdentifierTypeRelative) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, params.Path, block.IdentifierTypeRelative) if err != nil { writeError(w, r, http.StatusInternalServerError, err) return @@ -765,7 +765,7 @@ func (c *Controller) LinkPhysicalAddress(w http.ResponseWriter, r *http.Request, ifAbsent = true } - blockStoreType := c.BlockAdapter.BlockstoreType() + blockStoreType := c.BlockAdapter.BlockstoreType(repo.StorageID) expectedType := qk.GetStorageType().BlockstoreType() if expectedType != blockStoreType { c.Logger.WithContext(ctx).WithFields(logging.Fields{ @@ -1851,7 +1851,8 @@ func (c *Controller) GetConfig(w http.ResponseWriter, r *http.Request) { } } - storageCfg := c.getStorageConfig() + // TODO (gilo): is StorageID relevant here? + storageCfg := c.getStorageConfig("") // TODO (niro): Needs to be populated storageListCfg := apigen.StorageConfigList{} versionConfig := c.getVersionConfig() @@ -1868,11 +1869,12 @@ func (c *Controller) GetStorageConfig(w http.ResponseWriter, r *http.Request) { return } - writeResponse(w, r, http.StatusOK, c.getStorageConfig()) + // TODO (gilo): is StorageID relevant here? + writeResponse(w, r, http.StatusOK, c.getStorageConfig("")) } -func (c *Controller) getStorageConfig() apigen.StorageConfig { - info := c.BlockAdapter.GetStorageNamespaceInfo() +func (c *Controller) getStorageConfig(storageID string) apigen.StorageConfig { + info := c.BlockAdapter.GetStorageNamespaceInfo(storageID) defaultNamespacePrefix := swag.String(info.DefaultNamespacePrefix) if c.Config.GetBaseConfig().Blockstore.DefaultNamespacePrefix != nil { defaultNamespacePrefix = c.Config.GetBaseConfig().Blockstore.DefaultNamespacePrefix @@ -1971,7 +1973,7 @@ func (c *Controller) CreateRepository(w http.ResponseWriter, r *http.Request, bo c.LogAction(ctx, "repo_sample_data", r, body.Name, "", "") } - if err := c.validateStorageNamespace(storageNamespace); err != nil { + if err := c.validateStorageNamespace(storageID, storageNamespace); err != nil { writeError(w, r, http.StatusBadRequest, err) return } @@ -2000,7 +2002,7 @@ func (c *Controller) CreateRepository(w http.ResponseWriter, r *http.Request, bo retErr = err reason = "bad_url" case errors.Is(err, block.ErrInvalidAddress): - retErr = fmt.Errorf("%w, must match: %s", err, c.BlockAdapter.BlockstoreType()) + retErr = fmt.Errorf("%w, must match: %s", err, c.BlockAdapter.BlockstoreType(storageID)) reason = "invalid_namespace" case errors.Is(err, ErrStorageNamespaceInUse): retErr = err @@ -2075,8 +2077,8 @@ func (c *Controller) CreateRepository(w http.ResponseWriter, r *http.Request, bo writeResponse(w, r, http.StatusCreated, response) } -func (c *Controller) validateStorageNamespace(storageNamespace string) error { - validRegex := c.BlockAdapter.GetStorageNamespaceInfo().ValidityRegex +func (c *Controller) validateStorageNamespace(storageID, storageNamespace string) error { + validRegex := c.BlockAdapter.GetStorageNamespaceInfo(storageID).ValidityRegex storagePrefixRegex, err := regexp.Compile(validRegex) if err != nil { return fmt.Errorf("failed to compile validity regex %s: %w", validRegex, block.ErrInvalidNamespace) @@ -3312,7 +3314,7 @@ func (c *Controller) UploadObject(w http.ResponseWriter, r *http.Request, reposi identifierType = block.IdentifierTypeRelative } - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, blob.PhysicalAddress, identifierType) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, blob.PhysicalAddress, identifierType) if err != nil { writeError(w, r, http.StatusInternalServerError, err) return @@ -3348,16 +3350,16 @@ func (c *Controller) StageObject(w http.ResponseWriter, r *http.Request, body ap return } // write metadata - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, body.PhysicalAddress, block.IdentifierTypeFull) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, body.PhysicalAddress, block.IdentifierTypeFull) if c.handleAPIError(ctx, w, r, err) { return } // see what storage type this is and whether it fits our configuration - uriRegex := c.BlockAdapter.GetStorageNamespaceInfo().ValidityRegex + uriRegex := c.BlockAdapter.GetStorageNamespaceInfo(repo.StorageID).ValidityRegex if match, err := regexp.MatchString(uriRegex, body.PhysicalAddress); err != nil || !match { writeError(w, r, http.StatusBadRequest, fmt.Sprintf("physical address is not valid for block adapter: %s", - c.BlockAdapter.BlockstoreType(), + c.BlockAdapter.BlockstoreType(repo.StorageID), )) return } @@ -3453,7 +3455,7 @@ func (c *Controller) CopyObject(w http.ResponseWriter, r *http.Request, body api return } - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, entry.PhysicalAddress, block.IdentifierTypeRelative) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, entry.PhysicalAddress, block.IdentifierTypeRelative) if err != nil { writeError(w, r, http.StatusInternalServerError, err) return @@ -4177,7 +4179,7 @@ func (c *Controller) CreateSymlinkFile(w http.ResponseWriter, r *http.Request, r } // loop all entries enter to map[path] physicalAddress for _, entry := range entries { - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, entry.PhysicalAddress, entry.AddressType.ToIdentifierType()) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, entry.PhysicalAddress, entry.AddressType.ToIdentifierType()) if err != nil { writeError(w, r, http.StatusInternalServerError, fmt.Sprintf("error while resolving address: %s", err)) return @@ -4610,7 +4612,7 @@ func (c *Controller) ListObjects(w http.ResponseWriter, r *http.Request, reposit objList := make([]apigen.ObjectStats, 0, len(res)) for _, entry := range res { - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, entry.PhysicalAddress, entry.AddressType.ToIdentifierType()) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, entry.PhysicalAddress, entry.AddressType.ToIdentifierType()) if err != nil { writeError(w, r, http.StatusInternalServerError, err) return @@ -4708,7 +4710,7 @@ func (c *Controller) StatObject(w http.ResponseWriter, r *http.Request, reposito return } - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, entry.PhysicalAddress, entry.AddressType.ToIdentifierType()) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, entry.PhysicalAddress, entry.AddressType.ToIdentifierType()) if c.handleAPIError(ctx, w, r, err) { return } @@ -5091,7 +5093,8 @@ func (c *Controller) Setup(w http.ResponseWriter, r *http.Request, body apigen.S return } - meta := stats.NewMetadata(ctx, c.Logger, c.BlockAdapter.BlockstoreType(), c.MetadataManager, c.CloudMetadataProvider) + // TODO (gilo): which storageID should we use here? + meta := stats.NewMetadata(ctx, c.Logger, c.BlockAdapter.BlockstoreType(""), c.MetadataManager, c.CloudMetadataProvider) c.Collector.SetInstallationID(meta.InstallationID) c.Collector.CollectMetadata(meta) c.Collector.CollectEvent(stats.Event{Class: "global", Name: "init", UserID: body.Username, Client: httputil.GetRequestLakeFSClient(r)}) diff --git a/pkg/block/adapter.go b/pkg/block/adapter.go index 7f6db15b180..ba7a5ac26f2 100644 --- a/pkg/block/adapter.go +++ b/pkg/block/adapter.go @@ -194,10 +194,10 @@ type Adapter interface { AbortMultiPartUpload(ctx context.Context, obj ObjectPointer, uploadID string) error CompleteMultiPartUpload(ctx context.Context, obj ObjectPointer, uploadID string, multipartList *MultipartUploadCompletion) (*CompleteMultiPartUploadResponse, error) - BlockstoreType() string - BlockstoreMetadata(ctx context.Context) (*BlockstoreMetadata, error) - GetStorageNamespaceInfo() StorageNamespaceInfo - ResolveNamespace(storageNamespace, key string, identifierType IdentifierType) (QualifiedKey, error) + BlockstoreType(storageID string) string + BlockstoreMetadata(ctx context.Context, storageID string) (*BlockstoreMetadata, error) + GetStorageNamespaceInfo(storageID string) StorageNamespaceInfo + ResolveNamespace(storageID, storageNamespace, key string, identifierType IdentifierType) (QualifiedKey, error) // GetRegion storageID is not actively used, and it's here mainly for completeness GetRegion(ctx context.Context, storageID, storageNamespace string) (string, error) diff --git a/pkg/block/azure/adapter.go b/pkg/block/azure/adapter.go index 898b07c2bb6..123e98625a9 100644 --- a/pkg/block/azure/adapter.go +++ b/pkg/block/azure/adapter.go @@ -583,11 +583,11 @@ func (a *Adapter) AbortMultiPartUpload(_ context.Context, _ block.ObjectPointer, return nil } -func (a *Adapter) BlockstoreType() string { +func (a *Adapter) BlockstoreType(_ string) string { return block.BlockstoreTypeAzure } -func (a *Adapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { +func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } @@ -606,7 +606,7 @@ func (a *Adapter) CompleteMultiPartUpload(ctx context.Context, obj block.ObjectP return completeMultipart(ctx, multipartList.Part, *containerURL, qualifiedKey.BlobURL) } -func (a *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { +func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeAzure) info.ImportValidityRegex = fmt.Sprintf(`^https?://[a-z0-9_-]+\.%s`, a.clientCache.params.Domain) @@ -622,7 +622,7 @@ func (a *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { return info } -func (a *Adapter) ResolveNamespace(storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { +func (a *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { return block.DefaultResolveNamespace(storageNamespace, key, identifierType) } diff --git a/pkg/block/gs/adapter.go b/pkg/block/gs/adapter.go index ebfe32ad476..71d48d4da9b 100644 --- a/pkg/block/gs/adapter.go +++ b/pkg/block/gs/adapter.go @@ -650,15 +650,15 @@ func (a *Adapter) Close() error { return a.client.Close() } -func (a *Adapter) BlockstoreType() string { +func (a *Adapter) BlockstoreType(_ string) string { return block.BlockstoreTypeGS } -func (a *Adapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { +func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } -func (a *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { +func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeGS) if a.disablePreSigned { info.PreSignSupport = false @@ -670,7 +670,7 @@ func (a *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { } func (a *Adapter) extractParamsFromObj(obj block.ObjectPointer) (string, string, error) { - qk, err := a.ResolveNamespace(obj.StorageNamespace, obj.Identifier, obj.IdentifierType) + qk, err := a.ResolveNamespace(obj.StorageID, obj.StorageNamespace, obj.Identifier, obj.IdentifierType) if err != nil { return "", "", err } @@ -682,7 +682,7 @@ func (a *Adapter) extractParamsFromObj(obj block.ObjectPointer) (string, string, return bucket, key, nil } -func (a *Adapter) ResolveNamespace(storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { +func (a *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { qualifiedKey, err := block.DefaultResolveNamespace(storageNamespace, key, identifierType) if err != nil { return qualifiedKey, err diff --git a/pkg/block/local/adapter.go b/pkg/block/local/adapter.go index f89291da51b..d9c42470b9c 100644 --- a/pkg/block/local/adapter.go +++ b/pkg/block/local/adapter.go @@ -537,15 +537,15 @@ func (l *Adapter) getPartFiles(uploadID string, obj block.ObjectPointer) ([]stri return names, nil } -func (l *Adapter) BlockstoreType() string { +func (l *Adapter) BlockstoreType(_ string) string { return block.BlockstoreTypeLocal } -func (l *Adapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { +func (l *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } -func (l *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { +func (l *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeLocal) info.PreSignSupport = false info.DefaultNamespacePrefix = DefaultNamespacePrefix @@ -553,7 +553,7 @@ func (l *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { return info } -func (l *Adapter) ResolveNamespace(storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { +func (l *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { qk, err := block.DefaultResolveNamespace(storageNamespace, key, identifierType) if err != nil { return nil, err diff --git a/pkg/block/mem/adapter.go b/pkg/block/mem/adapter.go index 0299856b89e..feb76412fb4 100644 --- a/pkg/block/mem/adapter.go +++ b/pkg/block/mem/adapter.go @@ -335,22 +335,22 @@ func (a *Adapter) CompleteMultiPartUpload(_ context.Context, obj block.ObjectPoi }, nil } -func (a *Adapter) BlockstoreType() string { +func (a *Adapter) BlockstoreType(_ string) string { return block.BlockstoreTypeMem } -func (a *Adapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { +func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } -func (a *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { +func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeMem) info.PreSignSupport = false info.ImportSupport = false return info } -func (a *Adapter) ResolveNamespace(storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { +func (a *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { return block.DefaultResolveNamespace(storageNamespace, key, identifierType) } diff --git a/pkg/block/metrics.go b/pkg/block/metrics.go index 0c892ec2d8e..c7d2474a914 100644 --- a/pkg/block/metrics.go +++ b/pkg/block/metrics.go @@ -23,12 +23,12 @@ func (m *MetricsAdapter) InnerAdapter() Adapter { } func (m *MetricsAdapter) Put(ctx context.Context, obj ObjectPointer, sizeBytes int64, reader io.Reader, opts PutOpts) (*PutResponse, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.Put(ctx, obj, sizeBytes, reader, opts) } func (m *MetricsAdapter) Get(ctx context.Context, obj ObjectPointer) (io.ReadCloser, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.Get(ctx, obj) } @@ -37,94 +37,94 @@ func (m *MetricsAdapter) GetWalker(uri *url.URL) (Walker, error) { } func (m *MetricsAdapter) GetPreSignedURL(ctx context.Context, obj ObjectPointer, mode PreSignMode) (string, time.Time, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.GetPreSignedURL(ctx, obj, mode) } func (m *MetricsAdapter) GetPresignUploadPartURL(ctx context.Context, obj ObjectPointer, uploadID string, partNumber int) (string, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.GetPresignUploadPartURL(ctx, obj, uploadID, partNumber) } func (m *MetricsAdapter) Exists(ctx context.Context, obj ObjectPointer) (bool, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.Exists(ctx, obj) } func (m *MetricsAdapter) GetRange(ctx context.Context, obj ObjectPointer, startPosition int64, endPosition int64) (io.ReadCloser, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.GetRange(ctx, obj, startPosition, endPosition) } func (m *MetricsAdapter) GetProperties(ctx context.Context, obj ObjectPointer) (Properties, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.GetProperties(ctx, obj) } func (m *MetricsAdapter) Remove(ctx context.Context, obj ObjectPointer) error { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.Remove(ctx, obj) } func (m *MetricsAdapter) Copy(ctx context.Context, sourceObj, destinationObj ObjectPointer) error { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(sourceObj.StorageID)) return m.adapter.Copy(ctx, sourceObj, destinationObj) } func (m *MetricsAdapter) CreateMultiPartUpload(ctx context.Context, obj ObjectPointer, r *http.Request, opts CreateMultiPartUploadOpts) (*CreateMultiPartUploadResponse, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.CreateMultiPartUpload(ctx, obj, r, opts) } func (m *MetricsAdapter) UploadPart(ctx context.Context, obj ObjectPointer, sizeBytes int64, reader io.Reader, uploadID string, partNumber int) (*UploadPartResponse, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.UploadPart(ctx, obj, sizeBytes, reader, uploadID, partNumber) } func (m *MetricsAdapter) ListParts(ctx context.Context, obj ObjectPointer, uploadID string, opts ListPartsOpts) (*ListPartsResponse, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.ListParts(ctx, obj, uploadID, opts) } func (m *MetricsAdapter) UploadCopyPart(ctx context.Context, sourceObj, destinationObj ObjectPointer, uploadID string, partNumber int) (*UploadPartResponse, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(sourceObj.StorageID)) return m.adapter.UploadCopyPart(ctx, sourceObj, destinationObj, uploadID, partNumber) } func (m *MetricsAdapter) UploadCopyPartRange(ctx context.Context, sourceObj, destinationObj ObjectPointer, uploadID string, partNumber int, startPosition, endPosition int64) (*UploadPartResponse, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(sourceObj.StorageID)) return m.adapter.UploadCopyPartRange(ctx, sourceObj, destinationObj, uploadID, partNumber, startPosition, endPosition) } func (m *MetricsAdapter) AbortMultiPartUpload(ctx context.Context, obj ObjectPointer, uploadID string) error { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.AbortMultiPartUpload(ctx, obj, uploadID) } func (m *MetricsAdapter) CompleteMultiPartUpload(ctx context.Context, obj ObjectPointer, uploadID string, multipartList *MultipartUploadCompletion) (*CompleteMultiPartUploadResponse, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.CompleteMultiPartUpload(ctx, obj, uploadID, multipartList) } -func (m *MetricsAdapter) BlockstoreType() string { - return m.adapter.BlockstoreType() +func (m *MetricsAdapter) BlockstoreType(storageID string) string { + return m.adapter.BlockstoreType(storageID) } -func (m *MetricsAdapter) BlockstoreMetadata(ctx context.Context) (*BlockstoreMetadata, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) - return m.adapter.BlockstoreMetadata(ctx) +func (m *MetricsAdapter) BlockstoreMetadata(ctx context.Context, storageID string) (*BlockstoreMetadata, error) { + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(storageID)) + return m.adapter.BlockstoreMetadata(ctx, storageID) } -func (m *MetricsAdapter) GetStorageNamespaceInfo() StorageNamespaceInfo { - return m.adapter.GetStorageNamespaceInfo() +func (m *MetricsAdapter) GetStorageNamespaceInfo(storageID string) StorageNamespaceInfo { + return m.adapter.GetStorageNamespaceInfo(storageID) } -func (m *MetricsAdapter) ResolveNamespace(storageNamespace, key string, identifierType IdentifierType) (QualifiedKey, error) { - return m.adapter.ResolveNamespace(storageNamespace, key, identifierType) +func (m *MetricsAdapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType IdentifierType) (QualifiedKey, error) { + return m.adapter.ResolveNamespace(storageID, storageNamespace, key, identifierType) } func (m *MetricsAdapter) GetRegion(ctx context.Context, storageID, storageNamespace string) (string, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(storageID)) return m.adapter.GetRegion(ctx, storageID, storageNamespace) } diff --git a/pkg/block/s3/adapter.go b/pkg/block/s3/adapter.go index d0e96e79cb2..428b794eab8 100644 --- a/pkg/block/s3/adapter.go +++ b/pkg/block/s3/adapter.go @@ -851,11 +851,11 @@ func (a *Adapter) ListParts(ctx context.Context, obj block.ObjectPointer, upload return &partsResp, nil } -func (a *Adapter) BlockstoreType() string { +func (a *Adapter) BlockstoreType(_ string) string { return block.BlockstoreTypeS3 } -func (a *Adapter) BlockstoreMetadata(ctx context.Context) (*block.BlockstoreMetadata, error) { +func (a *Adapter) BlockstoreMetadata(ctx context.Context, _ string) (*block.BlockstoreMetadata, error) { region, err := a.clients.GetBucketRegionDefault(ctx, "") if err != nil { return nil, err @@ -863,7 +863,7 @@ func (a *Adapter) BlockstoreMetadata(ctx context.Context) (*block.BlockstoreMeta return &block.BlockstoreMetadata{Region: ®ion}, nil } -func (a *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { +func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeS3) if a.disablePreSigned { info.PreSignSupport = false @@ -888,7 +888,7 @@ func resolveNamespace(obj block.ObjectPointer) (block.CommonQualifiedKey, error) return qualifiedKey, nil } -func (a *Adapter) ResolveNamespace(storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { +func (a *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { return block.DefaultResolveNamespace(storageNamespace, key, identifierType) } @@ -945,7 +945,7 @@ func (a *Adapter) managerUpload(ctx context.Context, obj block.ObjectPointer, re } func (a *Adapter) extractParamsFromObj(obj block.ObjectPointer) (string, string, block.QualifiedKey, error) { - qk, err := a.ResolveNamespace(obj.StorageNamespace, obj.Identifier, obj.IdentifierType) + qk, err := a.ResolveNamespace(obj.StorageID, obj.StorageNamespace, obj.Identifier, obj.IdentifierType) if err != nil { return "", "", nil, err } diff --git a/pkg/block/transient/adapter.go b/pkg/block/transient/adapter.go index b5d70e1bca9..d3cf49864ea 100644 --- a/pkg/block/transient/adapter.go +++ b/pkg/block/transient/adapter.go @@ -143,15 +143,15 @@ func (a *Adapter) CompleteMultiPartUpload(context.Context, block.ObjectPointer, }, nil } -func (a *Adapter) BlockstoreType() string { +func (a *Adapter) BlockstoreType(_ string) string { return block.BlockstoreTypeTransient } -func (a *Adapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { +func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } -func (a *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { +func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeTransient) info.PreSignSupport = false info.PreSignSupportUI = false @@ -159,7 +159,7 @@ func (a *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { return info } -func (a *Adapter) ResolveNamespace(storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { +func (a *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { return block.DefaultResolveNamespace(storageNamespace, key, identifierType) } diff --git a/pkg/block/validations.go b/pkg/block/validations.go index 84f26c2d6a3..a961447cc91 100644 --- a/pkg/block/validations.go +++ b/pkg/block/validations.go @@ -7,7 +7,7 @@ import ( ) func ValidateInterRegionStorage(ctx context.Context, adapter Adapter, storageID, storageNamespace string) error { - blockstoreMetadata, err := adapter.BlockstoreMetadata(ctx) + blockstoreMetadata, err := adapter.BlockstoreMetadata(ctx, storageID) if errors.Is(err, ErrOperationNotSupported) { // region detection not supported for the server's blockstore, skip validation return nil diff --git a/pkg/graveler/retention/garbage_collection_manager.go b/pkg/graveler/retention/garbage_collection_manager.go index de7580f54d1..841b7a337d6 100644 --- a/pkg/graveler/retention/garbage_collection_manager.go +++ b/pkg/graveler/retention/garbage_collection_manager.go @@ -36,7 +36,8 @@ type GarbageCollectionManager struct { func (m *GarbageCollectionManager) GetCommitsCSVLocation(runID string, sn graveler.StorageNamespace) (string, error) { key := fmt.Sprintf(commitsFileSuffixTemplate, m.committedBlockStoragePrefix, runID) - qk, err := m.blockAdapter.ResolveNamespace(sn.String(), key, block.IdentifierTypeRelative) + // TODO (gilo): replace StorageID with a real value + qk, err := m.blockAdapter.ResolveNamespace("", sn.String(), key, block.IdentifierTypeRelative) if err != nil { return "", err } @@ -45,7 +46,8 @@ func (m *GarbageCollectionManager) GetCommitsCSVLocation(runID string, sn gravel func (m *GarbageCollectionManager) GetAddressesLocation(sn graveler.StorageNamespace) (string, error) { key := fmt.Sprintf(addressesFilePrefixTemplate, m.committedBlockStoragePrefix) - qk, err := m.blockAdapter.ResolveNamespace(sn.String(), key, block.IdentifierTypeRelative) + // TODO (gilo): replace StorageID with a real value + qk, err := m.blockAdapter.ResolveNamespace("", sn.String(), key, block.IdentifierTypeRelative) if err != nil { return "", err } @@ -55,7 +57,8 @@ func (m *GarbageCollectionManager) GetAddressesLocation(sn graveler.StorageNames // GetUncommittedLocation return full path to underlying storage path to store uncommitted information func (m *GarbageCollectionManager) GetUncommittedLocation(runID string, sn graveler.StorageNamespace) (string, error) { key := fmt.Sprintf(uncommittedFilePrefixTemplate, m.committedBlockStoragePrefix, runID) - qk, err := m.blockAdapter.ResolveNamespace(sn.String(), key, block.IdentifierTypeRelative) + // TODO (gilo): replace StorageID with a real value + qk, err := m.blockAdapter.ResolveNamespace("", sn.String(), key, block.IdentifierTypeRelative) if err != nil { return "", err } diff --git a/pkg/testutil/adapter.go b/pkg/testutil/adapter.go index 442ab22193b..08d87c70dd0 100644 --- a/pkg/testutil/adapter.go +++ b/pkg/testutil/adapter.go @@ -129,11 +129,11 @@ func (a *MockAdapter) ListParts(_ context.Context, _ block.ObjectPointer, _ stri panic("try to list parts in mock adapter") } -func (a *MockAdapter) BlockstoreType() string { +func (a *MockAdapter) BlockstoreType(_ string) string { return "s3" } -func (a *MockAdapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { +func (a *MockAdapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { if a.blockstoreMetadata != nil { return a.blockstoreMetadata, nil } else { @@ -141,14 +141,14 @@ func (a *MockAdapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMe } } -func (a *MockAdapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { +func (a *MockAdapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { info := block.DefaultStorageNamespaceInfo("s3") info.PreSignSupport = false info.ImportSupport = false return info } -func (a *MockAdapter) ResolveNamespace(storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { +func (a *MockAdapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { return block.DefaultResolveNamespace(storageNamespace, key, identifierType) } From a2a4ab5eac9edf585aba9b03f1ff06362f2da660 Mon Sep 17 00:00:00 2001 From: Itai Gilo Date: Sun, 26 Jan 2025 16:45:53 +0200 Subject: [PATCH 02/14] Fix tests --- cmd/lakefs/cmd/run.go | 2 +- pkg/api/controller_test.go | 2 +- pkg/block/azure/adapter_test.go | 2 +- pkg/block/blocktest/adapter.go | 12 ++++++------ pkg/block/blocktest/basic_suite.go | 2 +- pkg/block/blocktest/multipart_suite.go | 10 +++++----- pkg/block/gs/adapter_test.go | 2 +- pkg/block/local/adapter_test.go | 2 +- pkg/block/s3/adapter_test.go | 2 +- 9 files changed, 18 insertions(+), 18 deletions(-) diff --git a/cmd/lakefs/cmd/run.go b/cmd/lakefs/cmd/run.go index bdb85666843..12bc4e2cf44 100644 --- a/cmd/lakefs/cmd/run.go +++ b/cmd/lakefs/cmd/run.go @@ -431,7 +431,7 @@ func checkRepos(ctx context.Context, logger logging.Logger, authMetadataManager } else { logger. // TODO (gilo): uncomment this? - //WithField("adapter_type", blockStore.BlockstoreType()). + // WithField("adapter_type", blockStore.BlockstoreType()). Debug("lakeFS is initialized, checking repositories for mismatched adapter") hasMore := true next := "" diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index 9e319989c40..7b4b24afe58 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -77,7 +77,7 @@ func verifyResponseOK(t testing.TB, resp Statuser, err error) { } func onBlock(deps *dependencies, path string) string { - return fmt.Sprintf("%s://%s", deps.blocks.BlockstoreType(), path) + return fmt.Sprintf("%s://%s", deps.blocks.BlockstoreType(""), path) } func TestController_ListRepositoriesHandler(t *testing.T) { diff --git a/pkg/block/azure/adapter_test.go b/pkg/block/azure/adapter_test.go index 5b13deb0357..32a0efa4801 100644 --- a/pkg/block/azure/adapter_test.go +++ b/pkg/block/azure/adapter_test.go @@ -135,7 +135,7 @@ func TestAdapterNamespace(t *testing.T) { } require.NoError(t, err, "create new adapter") - namespaceInfo := adapter.GetStorageNamespaceInfo() + namespaceInfo := adapter.GetStorageNamespaceInfo("") expr, err := regexp.Compile(namespaceInfo.ValidityRegex) require.NoError(t, err) diff --git a/pkg/block/blocktest/adapter.go b/pkg/block/blocktest/adapter.go index 5fa7f8010de..3cc39df5f15 100644 --- a/pkg/block/blocktest/adapter.go +++ b/pkg/block/blocktest/adapter.go @@ -126,7 +126,7 @@ func testAdapterWalker(t *testing.T, adapter block.Adapter, storageNamespace str }, } for _, tt := range cases { - qk, err := adapter.ResolveNamespace(storageNamespace, filepath.Join(testPrefix, tt.prefix), block.IdentifierTypeRelative) + qk, err := adapter.ResolveNamespace("", storageNamespace, filepath.Join(testPrefix, tt.prefix), block.IdentifierTypeRelative) require.NoError(t, err) uri, err := url.Parse(qk.Format()) require.NoError(t, err) @@ -142,7 +142,7 @@ func testAdapterWalker(t *testing.T, adapter block.Adapter, storageNamespace str require.NoError(t, err) var prefix string if tt.prefix == "" { - if adapter.BlockstoreType() != block.BlockstoreTypeLocal { + if adapter.BlockstoreType("") != block.BlockstoreTypeLocal { prefix = testPrefix } @@ -154,7 +154,7 @@ func testAdapterWalker(t *testing.T, adapter block.Adapter, storageNamespace str } } } else { - if adapter.BlockstoreType() != block.BlockstoreTypeLocal { + if adapter.BlockstoreType("") != block.BlockstoreTypeLocal { prefix = tt.prefix } for j := 0; j < filesAndFolders; j++ { @@ -193,10 +193,10 @@ func getPresignedURLBasicTest(t *testing.T, adapter block.Adapter, storageNamesp preSignedURL, exp, err := adapter.GetPreSignedURL(ctx, obj, block.PreSignModeRead) - if adapter.BlockstoreType() == block.BlockstoreTypeGS { + if adapter.BlockstoreType("") == block.BlockstoreTypeGS { require.ErrorContains(t, err, "no credentials found") return "", nil - } else if adapter.BlockstoreType() == block.BlockstoreTypeLocal { + } else if adapter.BlockstoreType("") == block.BlockstoreTypeLocal { require.ErrorIs(t, err, block.ErrOperationNotSupported) return "", nil } @@ -205,7 +205,7 @@ func getPresignedURLBasicTest(t *testing.T, adapter block.Adapter, storageNamesp } func expectedURLExp(adapter block.Adapter) time.Time { - if adapter.BlockstoreType() == block.BlockstoreTypeAzure { + if adapter.BlockstoreType("") == block.BlockstoreTypeAzure { // we didn't implement expiry for Azure yet return time.Time{} } else { diff --git a/pkg/block/blocktest/basic_suite.go b/pkg/block/blocktest/basic_suite.go index f96d5efb98a..9e8e85c9dbc 100644 --- a/pkg/block/blocktest/basic_suite.go +++ b/pkg/block/blocktest/basic_suite.go @@ -153,7 +153,7 @@ func testAdapterRemove(t *testing.T, adapter block.Adapter, storageNamespace str t.Errorf("Remove() error = %v, wantErr %v", err, tt.wantErr) } - qk, err := adapter.ResolveNamespace(storageNamespace, tt.name, block.IdentifierTypeRelative) + qk, err := adapter.ResolveNamespace("", storageNamespace, tt.name, block.IdentifierTypeRelative) require.NoError(t, err) tree := dumpPathTree(t, ctx, adapter, qk) diff --git a/pkg/block/blocktest/multipart_suite.go b/pkg/block/blocktest/multipart_suite.go index a8961bd95fb..79b16cd0482 100644 --- a/pkg/block/blocktest/multipart_suite.go +++ b/pkg/block/blocktest/multipart_suite.go @@ -39,7 +39,7 @@ func testAdapterMultipartUpload(t *testing.T, adapter block.Adapter, storageName } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - blockstoreType := adapter.BlockstoreType() + blockstoreType := adapter.BlockstoreType("") obj := block.ObjectPointer{ StorageID: "", StorageNamespace: storageNamespace, @@ -132,7 +132,7 @@ func testAdapterCopyPart(t *testing.T, adapter block.Adapter, storageNamespace s resp, err := adapter.CreateMultiPartUpload(ctx, objCopy, nil, block.CreateMultiPartUploadOpts{}) require.NoError(t, err) multiParts, err := copyPart(t, ctx, adapter, obj, objCopy, resp.UploadID) - if adapter.BlockstoreType() == block.BlockstoreTypeAzure { + if adapter.BlockstoreType("") == block.BlockstoreTypeAzure { require.ErrorContains(t, err, "not implemented") // azurite block store emulator did not yet implement this return } @@ -155,7 +155,7 @@ func testAdapterCopyPartRange(t *testing.T, adapter block.Adapter, storageNamesp resp, err := adapter.CreateMultiPartUpload(ctx, objCopy, nil, block.CreateMultiPartUploadOpts{}) require.NoError(t, err) multiParts := copyPartRange(t, ctx, adapter, obj, objCopy, resp.UploadID) - if adapter.BlockstoreType() == block.BlockstoreTypeAzure { + if adapter.BlockstoreType("") == block.BlockstoreTypeAzure { // not implemented in Azure require.Nil(t, multiParts) // azurite block store emulator did not yet implement this return @@ -200,7 +200,7 @@ func uploadMultiPart(t *testing.T, ctx context.Context, adapter block.Adapter, o func verifyListInvalid(t *testing.T, ctx context.Context, adapter block.Adapter, obj block.ObjectPointer, uploadID string) { t.Helper() _, err := adapter.ListParts(ctx, obj, uploadID, block.ListPartsOpts{}) - if adapter.BlockstoreType() != block.BlockstoreTypeS3 { + if adapter.BlockstoreType("") != block.BlockstoreTypeS3 { require.ErrorIs(t, err, block.ErrOperationNotSupported) } else { require.NotNil(t, err) @@ -231,7 +231,7 @@ func copyPartRange(t *testing.T, ctx context.Context, adapter block.Adapter, obj partNumber := i + 1 var endPosition = startPosition + multipartPartSize partResp, err := adapter.UploadCopyPartRange(ctx, obj, objCopy, uploadID, partNumber, startPosition, endPosition) - if adapter.BlockstoreType() == block.BlockstoreTypeAzure { + if adapter.BlockstoreType("") == block.BlockstoreTypeAzure { require.ErrorContains(t, err, "not implemented") // azurite block store emulator did not yet implement this return nil } diff --git a/pkg/block/gs/adapter_test.go b/pkg/block/gs/adapter_test.go index bea1bd39146..252cef86613 100644 --- a/pkg/block/gs/adapter_test.go +++ b/pkg/block/gs/adapter_test.go @@ -36,7 +36,7 @@ func TestAdapterNamespace(t *testing.T) { require.NoError(t, adapter.Close()) }() - expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo().ValidityRegex) + expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo("").ValidityRegex) require.NoError(t, err) tests := []struct { diff --git a/pkg/block/local/adapter_test.go b/pkg/block/local/adapter_test.go index 054b13e589c..a45ee299d9e 100644 --- a/pkg/block/local/adapter_test.go +++ b/pkg/block/local/adapter_test.go @@ -31,7 +31,7 @@ func TestAdapterNamespace(t *testing.T) { localPath := path.Join(tmpDir, "lakefs") adapter, err := local.NewAdapter(localPath, local.WithRemoveEmptyDir(false)) require.NoError(t, err, "create new adapter") - expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo().ValidityRegex) + expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo("").ValidityRegex) require.NoError(t, err) tests := []struct { diff --git a/pkg/block/s3/adapter_test.go b/pkg/block/s3/adapter_test.go index 90dde42c06d..f0e4aa406d7 100644 --- a/pkg/block/s3/adapter_test.go +++ b/pkg/block/s3/adapter_test.go @@ -66,7 +66,7 @@ func TestS3AdapterPresignedOverride(t *testing.T) { // TestAdapterNamespace tests the namespace validity regex with various paths func TestAdapterNamespace(t *testing.T) { adapter := getS3BlockAdapter(t, nil) - expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo().ValidityRegex) + expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo("").ValidityRegex) require.NoError(t, err) tests := []struct { From 9d3f0a2edb9889f628ee4c36af6fe856cc8f3e74 Mon Sep 17 00:00:00 2001 From: Itai Gilo Date: Mon, 27 Jan 2025 17:36:07 +0200 Subject: [PATCH 03/14] Add error handling --- cmd/lakefs/cmd/run.go | 5 +- pkg/api/controller.go | 66 +++++++++++---- pkg/api/controller_test.go | 3 +- pkg/block/adapter.go | 4 +- pkg/block/azure/adapter.go | 8 +- pkg/block/azure/adapter_test.go | 3 +- pkg/block/blocktest/multipart_suite.go | 18 ++-- pkg/block/gs/adapter.go | 8 +- pkg/block/gs/adapter_test.go | 4 +- pkg/block/local/adapter.go | 8 +- pkg/block/local/adapter_test.go | 4 +- pkg/block/mem/adapter.go | 8 +- pkg/block/metrics.go | 112 ++++++++++++++++++++----- pkg/block/s3/adapter.go | 8 +- pkg/block/s3/adapter_test.go | 4 +- pkg/block/transient/adapter.go | 8 +- pkg/testutil/adapter.go | 8 +- 17 files changed, 204 insertions(+), 75 deletions(-) diff --git a/cmd/lakefs/cmd/run.go b/cmd/lakefs/cmd/run.go index 12bc4e2cf44..f0f730fdc91 100644 --- a/cmd/lakefs/cmd/run.go +++ b/cmd/lakefs/cmd/run.go @@ -445,7 +445,10 @@ func checkRepos(ctx context.Context, logger logging.Logger, authMetadataManager } for _, repo := range repos { - adapterStorageType := blockStore.BlockstoreType(repo.StorageID) + adapterStorageType, err := blockStore.BlockstoreType(repo.StorageID) + if err != nil { + logger.WithError(err).Fatalf("Failed to get storage type for repository %s", repo.Name) + } nsURL, err := url.Parse(repo.StorageNamespace) if err != nil { logger.WithError(err).Fatalf("Failed to parse repository %s namespace '%s'", repo.Name, repo.StorageNamespace) diff --git a/pkg/api/controller.go b/pkg/api/controller.go index b40292e5bbc..18324c8f3a0 100644 --- a/pkg/api/controller.go +++ b/pkg/api/controller.go @@ -172,8 +172,12 @@ func (c *Controller) CreatePresignMultipartUpload(w http.ResponseWriter, r *http return } + storageConfig, err := c.getStorageConfig(repo.StorageID) + if c.handleAPIError(ctx, w, r, err) { + return + } + // check if api is supported - storageConfig := c.getStorageConfig(repo.StorageID) if !swag.BoolValue(storageConfig.PreSignMultipartUpload) { writeError(w, r, http.StatusNotImplemented, "presign multipart upload API is not supported") return @@ -272,8 +276,12 @@ func (c *Controller) AbortPresignMultipartUpload(w http.ResponseWriter, r *http. return } + storageConfig, err := c.getStorageConfig(repo.StorageID) + if c.handleAPIError(ctx, w, r, err) { + return + } + // check if api is supported - storageConfig := c.getStorageConfig(repo.StorageID) if !swag.BoolValue(storageConfig.PreSignMultipartUpload) { writeError(w, r, http.StatusNotImplemented, "presign multipart upload API is not supported") return @@ -333,8 +341,12 @@ func (c *Controller) CompletePresignMultipartUpload(w http.ResponseWriter, r *ht return } + storageConfig, err := c.getStorageConfig(repo.StorageID) + if c.handleAPIError(ctx, w, r, err) { + return + } + // check if api is supported - storageConfig := c.getStorageConfig(repo.StorageID) if !swag.BoolValue(storageConfig.PreSignMultipartUpload) { writeError(w, r, http.StatusNotImplemented, "presign multipart upload API is not supported") return @@ -765,7 +777,10 @@ func (c *Controller) LinkPhysicalAddress(w http.ResponseWriter, r *http.Request, ifAbsent = true } - blockStoreType := c.BlockAdapter.BlockstoreType(repo.StorageID) + blockStoreType, err := c.BlockAdapter.BlockstoreType(repo.StorageID) + if err != nil { + c.handleAPIError(ctx, w, r, err) + } expectedType := qk.GetStorageType().BlockstoreType() if expectedType != blockStoreType { c.Logger.WithContext(ctx).WithFields(logging.Fields{ @@ -1852,7 +1867,10 @@ func (c *Controller) GetConfig(w http.ResponseWriter, r *http.Request) { } // TODO (gilo): is StorageID relevant here? - storageCfg := c.getStorageConfig("") + storageCfg, err := c.getStorageConfig("") + if c.handleAPIError(r.Context(), w, r, err) { + return + } // TODO (niro): Needs to be populated storageListCfg := apigen.StorageConfigList{} versionConfig := c.getVersionConfig() @@ -1870,16 +1888,24 @@ func (c *Controller) GetStorageConfig(w http.ResponseWriter, r *http.Request) { } // TODO (gilo): is StorageID relevant here? - writeResponse(w, r, http.StatusOK, c.getStorageConfig("")) + storageConfig, err := c.getStorageConfig("") + if c.handleAPIError(r.Context(), w, r, err) { + return + } + + writeResponse(w, r, http.StatusOK, storageConfig) } -func (c *Controller) getStorageConfig(storageID string) apigen.StorageConfig { - info := c.BlockAdapter.GetStorageNamespaceInfo(storageID) +func (c *Controller) getStorageConfig(storageID string) (apigen.StorageConfig, error) { + info, err := c.BlockAdapter.GetStorageNamespaceInfo(storageID) + if err != nil { + return apigen.StorageConfig{}, err + } defaultNamespacePrefix := swag.String(info.DefaultNamespacePrefix) if c.Config.GetBaseConfig().Blockstore.DefaultNamespacePrefix != nil { defaultNamespacePrefix = c.Config.GetBaseConfig().Blockstore.DefaultNamespacePrefix } - return apigen.StorageConfig{ + storageConfig := apigen.StorageConfig{ BlockstoreType: c.Config.GetBaseConfig().Blockstore.Type, BlockstoreNamespaceValidityRegex: info.ValidityRegex, BlockstoreNamespaceExample: info.Example, @@ -1890,6 +1916,7 @@ func (c *Controller) getStorageConfig(storageID string) apigen.StorageConfig { ImportValidityRegex: info.ImportValidityRegex, PreSignMultipartUpload: swag.Bool(info.PreSignSupportMultipart), } + return storageConfig, nil } func (c *Controller) HealthCheck(w http.ResponseWriter, r *http.Request) { @@ -2002,7 +2029,8 @@ func (c *Controller) CreateRepository(w http.ResponseWriter, r *http.Request, bo retErr = err reason = "bad_url" case errors.Is(err, block.ErrInvalidAddress): - retErr = fmt.Errorf("%w, must match: %s", err, c.BlockAdapter.BlockstoreType(storageID)) + blockstoreType, _ := c.BlockAdapter.BlockstoreType(storageID) + retErr = fmt.Errorf("%w, must match: %s", err, blockstoreType) reason = "invalid_namespace" case errors.Is(err, ErrStorageNamespaceInUse): retErr = err @@ -2078,7 +2106,11 @@ func (c *Controller) CreateRepository(w http.ResponseWriter, r *http.Request, bo } func (c *Controller) validateStorageNamespace(storageID, storageNamespace string) error { - validRegex := c.BlockAdapter.GetStorageNamespaceInfo(storageID).ValidityRegex + storageNamespaceInfo, err := c.BlockAdapter.GetStorageNamespaceInfo(storageID) + if err != nil { + return fmt.Errorf("failed to get validity regex for storage ID %s: %w", storageID, block.ErrInvalidNamespace) + } + validRegex := storageNamespaceInfo.ValidityRegex storagePrefixRegex, err := regexp.Compile(validRegex) if err != nil { return fmt.Errorf("failed to compile validity regex %s: %w", validRegex, block.ErrInvalidNamespace) @@ -3356,10 +3388,15 @@ func (c *Controller) StageObject(w http.ResponseWriter, r *http.Request, body ap } // see what storage type this is and whether it fits our configuration - uriRegex := c.BlockAdapter.GetStorageNamespaceInfo(repo.StorageID).ValidityRegex + namespaceInfo, err := c.BlockAdapter.GetStorageNamespaceInfo(repo.StorageID) + if c.handleAPIError(ctx, w, r, err) { + return + } + uriRegex := namespaceInfo.ValidityRegex if match, err := regexp.MatchString(uriRegex, body.PhysicalAddress); err != nil || !match { + blockstoreType, _ := c.BlockAdapter.BlockstoreType(repo.StorageID) writeError(w, r, http.StatusBadRequest, fmt.Sprintf("physical address is not valid for block adapter: %s", - c.BlockAdapter.BlockstoreType(repo.StorageID), + blockstoreType, )) return } @@ -5094,7 +5131,8 @@ func (c *Controller) Setup(w http.ResponseWriter, r *http.Request, body apigen.S } // TODO (gilo): which storageID should we use here? - meta := stats.NewMetadata(ctx, c.Logger, c.BlockAdapter.BlockstoreType(""), c.MetadataManager, c.CloudMetadataProvider) + blockstoreType, _ := c.BlockAdapter.BlockstoreType("") + meta := stats.NewMetadata(ctx, c.Logger, blockstoreType, c.MetadataManager, c.CloudMetadataProvider) c.Collector.SetInstallationID(meta.InstallationID) c.Collector.CollectMetadata(meta) c.Collector.CollectEvent(stats.Event{Class: "global", Name: "init", UserID: body.Username, Client: httputil.GetRequestLakeFSClient(r)}) diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index 7b4b24afe58..274d9c419b6 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -77,7 +77,8 @@ func verifyResponseOK(t testing.TB, resp Statuser, err error) { } func onBlock(deps *dependencies, path string) string { - return fmt.Sprintf("%s://%s", deps.blocks.BlockstoreType(""), path) + blockstoreType, _ := deps.blocks.BlockstoreType("") + return fmt.Sprintf("%s://%s", blockstoreType, path) } func TestController_ListRepositoriesHandler(t *testing.T) { diff --git a/pkg/block/adapter.go b/pkg/block/adapter.go index ba7a5ac26f2..a6a89a9f498 100644 --- a/pkg/block/adapter.go +++ b/pkg/block/adapter.go @@ -194,9 +194,9 @@ type Adapter interface { AbortMultiPartUpload(ctx context.Context, obj ObjectPointer, uploadID string) error CompleteMultiPartUpload(ctx context.Context, obj ObjectPointer, uploadID string, multipartList *MultipartUploadCompletion) (*CompleteMultiPartUploadResponse, error) - BlockstoreType(storageID string) string + BlockstoreType(storageID string) (string, error) BlockstoreMetadata(ctx context.Context, storageID string) (*BlockstoreMetadata, error) - GetStorageNamespaceInfo(storageID string) StorageNamespaceInfo + GetStorageNamespaceInfo(storageID string) (StorageNamespaceInfo, error) ResolveNamespace(storageID, storageNamespace, key string, identifierType IdentifierType) (QualifiedKey, error) // GetRegion storageID is not actively used, and it's here mainly for completeness diff --git a/pkg/block/azure/adapter.go b/pkg/block/azure/adapter.go index 123e98625a9..0809cdcc382 100644 --- a/pkg/block/azure/adapter.go +++ b/pkg/block/azure/adapter.go @@ -583,8 +583,8 @@ func (a *Adapter) AbortMultiPartUpload(_ context.Context, _ block.ObjectPointer, return nil } -func (a *Adapter) BlockstoreType(_ string) string { - return block.BlockstoreTypeAzure +func (a *Adapter) BlockstoreType(_ string) (string, error) { + return block.BlockstoreTypeAzure, nil } func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { @@ -606,7 +606,7 @@ func (a *Adapter) CompleteMultiPartUpload(ctx context.Context, obj block.ObjectP return completeMultipart(ctx, multipartList.Part, *containerURL, qualifiedKey.BlobURL) } -func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { +func (a *Adapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, error) { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeAzure) info.ImportValidityRegex = fmt.Sprintf(`^https?://[a-z0-9_-]+\.%s`, a.clientCache.params.Domain) @@ -619,7 +619,7 @@ func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { if !(a.disablePreSignedUI || a.disablePreSigned) { info.PreSignSupportUI = true } - return info + return info, nil } func (a *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { diff --git a/pkg/block/azure/adapter_test.go b/pkg/block/azure/adapter_test.go index 32a0efa4801..7dbcf25d2eb 100644 --- a/pkg/block/azure/adapter_test.go +++ b/pkg/block/azure/adapter_test.go @@ -135,7 +135,8 @@ func TestAdapterNamespace(t *testing.T) { } require.NoError(t, err, "create new adapter") - namespaceInfo := adapter.GetStorageNamespaceInfo("") + namespaceInfo, err := adapter.GetStorageNamespaceInfo("") + require.NoError(t, err) expr, err := regexp.Compile(namespaceInfo.ValidityRegex) require.NoError(t, err) diff --git a/pkg/block/blocktest/multipart_suite.go b/pkg/block/blocktest/multipart_suite.go index 79b16cd0482..3cb006d316a 100644 --- a/pkg/block/blocktest/multipart_suite.go +++ b/pkg/block/blocktest/multipart_suite.go @@ -39,7 +39,8 @@ func testAdapterMultipartUpload(t *testing.T, adapter block.Adapter, storageName } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - blockstoreType := adapter.BlockstoreType("") + blockstoreType, err := adapter.BlockstoreType("") + require.NoError(t, err) obj := block.ObjectPointer{ StorageID: "", StorageNamespace: storageNamespace, @@ -132,7 +133,9 @@ func testAdapterCopyPart(t *testing.T, adapter block.Adapter, storageNamespace s resp, err := adapter.CreateMultiPartUpload(ctx, objCopy, nil, block.CreateMultiPartUploadOpts{}) require.NoError(t, err) multiParts, err := copyPart(t, ctx, adapter, obj, objCopy, resp.UploadID) - if adapter.BlockstoreType("") == block.BlockstoreTypeAzure { + blockstoreType, err := adapter.BlockstoreType("") + require.NoError(t, err) + if blockstoreType == block.BlockstoreTypeAzure { require.ErrorContains(t, err, "not implemented") // azurite block store emulator did not yet implement this return } @@ -155,7 +158,9 @@ func testAdapterCopyPartRange(t *testing.T, adapter block.Adapter, storageNamesp resp, err := adapter.CreateMultiPartUpload(ctx, objCopy, nil, block.CreateMultiPartUploadOpts{}) require.NoError(t, err) multiParts := copyPartRange(t, ctx, adapter, obj, objCopy, resp.UploadID) - if adapter.BlockstoreType("") == block.BlockstoreTypeAzure { + blockstoreType, err := adapter.BlockstoreType("") + require.NoError(t, err) + if blockstoreType == block.BlockstoreTypeAzure { // not implemented in Azure require.Nil(t, multiParts) // azurite block store emulator did not yet implement this return @@ -200,7 +205,9 @@ func uploadMultiPart(t *testing.T, ctx context.Context, adapter block.Adapter, o func verifyListInvalid(t *testing.T, ctx context.Context, adapter block.Adapter, obj block.ObjectPointer, uploadID string) { t.Helper() _, err := adapter.ListParts(ctx, obj, uploadID, block.ListPartsOpts{}) - if adapter.BlockstoreType("") != block.BlockstoreTypeS3 { + blockstoreType, err := adapter.BlockstoreType("") + require.NoError(t, err) + if blockstoreType != block.BlockstoreTypeS3 { require.ErrorIs(t, err, block.ErrOperationNotSupported) } else { require.NotNil(t, err) @@ -231,7 +238,8 @@ func copyPartRange(t *testing.T, ctx context.Context, adapter block.Adapter, obj partNumber := i + 1 var endPosition = startPosition + multipartPartSize partResp, err := adapter.UploadCopyPartRange(ctx, obj, objCopy, uploadID, partNumber, startPosition, endPosition) - if adapter.BlockstoreType("") == block.BlockstoreTypeAzure { + blockstoreType, err := adapter.BlockstoreType("") + if blockstoreType == block.BlockstoreTypeAzure { require.ErrorContains(t, err, "not implemented") // azurite block store emulator did not yet implement this return nil } diff --git a/pkg/block/gs/adapter.go b/pkg/block/gs/adapter.go index 71d48d4da9b..04b943e54c3 100644 --- a/pkg/block/gs/adapter.go +++ b/pkg/block/gs/adapter.go @@ -650,15 +650,15 @@ func (a *Adapter) Close() error { return a.client.Close() } -func (a *Adapter) BlockstoreType(_ string) string { - return block.BlockstoreTypeGS +func (a *Adapter) BlockstoreType(_ string) (string, error) { + return block.BlockstoreTypeGS, nil } func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } -func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { +func (a *Adapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, error) { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeGS) if a.disablePreSigned { info.PreSignSupport = false @@ -666,7 +666,7 @@ func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { if !(a.disablePreSignedUI || a.disablePreSigned) { info.PreSignSupportUI = true } - return info + return info, nil } func (a *Adapter) extractParamsFromObj(obj block.ObjectPointer) (string, string, error) { diff --git a/pkg/block/gs/adapter_test.go b/pkg/block/gs/adapter_test.go index 252cef86613..70594cfff68 100644 --- a/pkg/block/gs/adapter_test.go +++ b/pkg/block/gs/adapter_test.go @@ -36,7 +36,9 @@ func TestAdapterNamespace(t *testing.T) { require.NoError(t, adapter.Close()) }() - expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo("").ValidityRegex) + namespaceInfo, err := adapter.GetStorageNamespaceInfo("") + require.NoError(t, err) + expr, err := regexp.Compile(namespaceInfo.ValidityRegex) require.NoError(t, err) tests := []struct { diff --git a/pkg/block/local/adapter.go b/pkg/block/local/adapter.go index d9c42470b9c..002e6e0c254 100644 --- a/pkg/block/local/adapter.go +++ b/pkg/block/local/adapter.go @@ -537,20 +537,20 @@ func (l *Adapter) getPartFiles(uploadID string, obj block.ObjectPointer) ([]stri return names, nil } -func (l *Adapter) BlockstoreType(_ string) string { - return block.BlockstoreTypeLocal +func (l *Adapter) BlockstoreType(_ string) (string, error) { + return block.BlockstoreTypeLocal, nil } func (l *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } -func (l *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { +func (l *Adapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, error) { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeLocal) info.PreSignSupport = false info.DefaultNamespacePrefix = DefaultNamespacePrefix info.ImportSupport = l.importEnabled - return info + return info, nil } func (l *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { diff --git a/pkg/block/local/adapter_test.go b/pkg/block/local/adapter_test.go index a45ee299d9e..ac78c91f2c7 100644 --- a/pkg/block/local/adapter_test.go +++ b/pkg/block/local/adapter_test.go @@ -31,7 +31,9 @@ func TestAdapterNamespace(t *testing.T) { localPath := path.Join(tmpDir, "lakefs") adapter, err := local.NewAdapter(localPath, local.WithRemoveEmptyDir(false)) require.NoError(t, err, "create new adapter") - expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo("").ValidityRegex) + namespaceInfo, err := adapter.GetStorageNamespaceInfo("") + require.NoError(t, err) + expr, err := regexp.Compile(namespaceInfo.ValidityRegex) require.NoError(t, err) tests := []struct { diff --git a/pkg/block/mem/adapter.go b/pkg/block/mem/adapter.go index feb76412fb4..d09c66dffc7 100644 --- a/pkg/block/mem/adapter.go +++ b/pkg/block/mem/adapter.go @@ -335,19 +335,19 @@ func (a *Adapter) CompleteMultiPartUpload(_ context.Context, obj block.ObjectPoi }, nil } -func (a *Adapter) BlockstoreType(_ string) string { - return block.BlockstoreTypeMem +func (a *Adapter) BlockstoreType(_ string) (string, error) { + return block.BlockstoreTypeMem, nil } func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } -func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { +func (a *Adapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, error) { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeMem) info.PreSignSupport = false info.ImportSupport = false - return info + return info, nil } func (a *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { diff --git a/pkg/block/metrics.go b/pkg/block/metrics.go index c7d2474a914..df8166f13d3 100644 --- a/pkg/block/metrics.go +++ b/pkg/block/metrics.go @@ -23,12 +23,20 @@ func (m *MetricsAdapter) InnerAdapter() Adapter { } func (m *MetricsAdapter) Put(ctx context.Context, obj ObjectPointer, sizeBytes int64, reader io.Reader, opts PutOpts) (*PutResponse, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) + if err != nil { + return nil, err + } + ctx = httputil.SetClientTrace(ctx, blockstoreType) return m.adapter.Put(ctx, obj, sizeBytes, reader, opts) } func (m *MetricsAdapter) Get(ctx context.Context, obj ObjectPointer) (io.ReadCloser, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) + if err != nil { + return nil, err + } + ctx = httputil.SetClientTrace(ctx, blockstoreType) return m.adapter.Get(ctx, obj) } @@ -37,85 +45,145 @@ func (m *MetricsAdapter) GetWalker(uri *url.URL) (Walker, error) { } func (m *MetricsAdapter) GetPreSignedURL(ctx context.Context, obj ObjectPointer, mode PreSignMode) (string, time.Time, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) + if err != nil { + return "", time.Time{}, err + } + ctx = httputil.SetClientTrace(ctx, blockstoreType) return m.adapter.GetPreSignedURL(ctx, obj, mode) } func (m *MetricsAdapter) GetPresignUploadPartURL(ctx context.Context, obj ObjectPointer, uploadID string, partNumber int) (string, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) + if err != nil { + return "", err + } + ctx = httputil.SetClientTrace(ctx, blockstoreType) return m.adapter.GetPresignUploadPartURL(ctx, obj, uploadID, partNumber) } func (m *MetricsAdapter) Exists(ctx context.Context, obj ObjectPointer) (bool, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) + if err != nil { + return false, err + } + ctx = httputil.SetClientTrace(ctx, blockstoreType) return m.adapter.Exists(ctx, obj) } func (m *MetricsAdapter) GetRange(ctx context.Context, obj ObjectPointer, startPosition int64, endPosition int64) (io.ReadCloser, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) + if err != nil { + return nil, err + } + ctx = httputil.SetClientTrace(ctx, blockstoreType) return m.adapter.GetRange(ctx, obj, startPosition, endPosition) } func (m *MetricsAdapter) GetProperties(ctx context.Context, obj ObjectPointer) (Properties, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) + if err != nil { + return Properties{}, err + } + ctx = httputil.SetClientTrace(ctx, blockstoreType) return m.adapter.GetProperties(ctx, obj) } func (m *MetricsAdapter) Remove(ctx context.Context, obj ObjectPointer) error { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) + if err != nil { + return err + } + ctx = httputil.SetClientTrace(ctx, blockstoreType) return m.adapter.Remove(ctx, obj) } func (m *MetricsAdapter) Copy(ctx context.Context, sourceObj, destinationObj ObjectPointer) error { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(sourceObj.StorageID)) + blockstoreType, err := m.adapter.BlockstoreType(sourceObj.StorageID) + if err != nil { + return err + } + ctx = httputil.SetClientTrace(ctx, blockstoreType) return m.adapter.Copy(ctx, sourceObj, destinationObj) } func (m *MetricsAdapter) CreateMultiPartUpload(ctx context.Context, obj ObjectPointer, r *http.Request, opts CreateMultiPartUploadOpts) (*CreateMultiPartUploadResponse, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) + if err != nil { + return nil, err + } + ctx = httputil.SetClientTrace(ctx, blockstoreType) return m.adapter.CreateMultiPartUpload(ctx, obj, r, opts) } func (m *MetricsAdapter) UploadPart(ctx context.Context, obj ObjectPointer, sizeBytes int64, reader io.Reader, uploadID string, partNumber int) (*UploadPartResponse, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) + if err != nil { + return nil, err + } + ctx = httputil.SetClientTrace(ctx, blockstoreType) return m.adapter.UploadPart(ctx, obj, sizeBytes, reader, uploadID, partNumber) } func (m *MetricsAdapter) ListParts(ctx context.Context, obj ObjectPointer, uploadID string, opts ListPartsOpts) (*ListPartsResponse, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) + if err != nil { + return nil, err + } + ctx = httputil.SetClientTrace(ctx, blockstoreType) return m.adapter.ListParts(ctx, obj, uploadID, opts) } func (m *MetricsAdapter) UploadCopyPart(ctx context.Context, sourceObj, destinationObj ObjectPointer, uploadID string, partNumber int) (*UploadPartResponse, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(sourceObj.StorageID)) + blockstoreType, err := m.adapter.BlockstoreType(sourceObj.StorageID) + if err != nil { + return nil, err + } + ctx = httputil.SetClientTrace(ctx, blockstoreType) return m.adapter.UploadCopyPart(ctx, sourceObj, destinationObj, uploadID, partNumber) } func (m *MetricsAdapter) UploadCopyPartRange(ctx context.Context, sourceObj, destinationObj ObjectPointer, uploadID string, partNumber int, startPosition, endPosition int64) (*UploadPartResponse, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(sourceObj.StorageID)) + blockstoreType, err := m.adapter.BlockstoreType(sourceObj.StorageID) + if err != nil { + return nil, err + } + ctx = httputil.SetClientTrace(ctx, blockstoreType) return m.adapter.UploadCopyPartRange(ctx, sourceObj, destinationObj, uploadID, partNumber, startPosition, endPosition) } func (m *MetricsAdapter) AbortMultiPartUpload(ctx context.Context, obj ObjectPointer, uploadID string) error { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) + if err != nil { + return err + } + ctx = httputil.SetClientTrace(ctx, blockstoreType) return m.adapter.AbortMultiPartUpload(ctx, obj, uploadID) } func (m *MetricsAdapter) CompleteMultiPartUpload(ctx context.Context, obj ObjectPointer, uploadID string, multipartList *MultipartUploadCompletion) (*CompleteMultiPartUploadResponse, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) + if err != nil { + return nil, err + } + ctx = httputil.SetClientTrace(ctx, blockstoreType) return m.adapter.CompleteMultiPartUpload(ctx, obj, uploadID, multipartList) } -func (m *MetricsAdapter) BlockstoreType(storageID string) string { +func (m *MetricsAdapter) BlockstoreType(storageID string) (string, error) { return m.adapter.BlockstoreType(storageID) } func (m *MetricsAdapter) BlockstoreMetadata(ctx context.Context, storageID string) (*BlockstoreMetadata, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(storageID)) + blockstoreType, err := m.adapter.BlockstoreType(storageID) + if err != nil { + return nil, err + } + ctx = httputil.SetClientTrace(ctx, blockstoreType) return m.adapter.BlockstoreMetadata(ctx, storageID) } -func (m *MetricsAdapter) GetStorageNamespaceInfo(storageID string) StorageNamespaceInfo { +func (m *MetricsAdapter) GetStorageNamespaceInfo(storageID string) (StorageNamespaceInfo, error) { return m.adapter.GetStorageNamespaceInfo(storageID) } @@ -124,7 +192,11 @@ func (m *MetricsAdapter) ResolveNamespace(storageID, storageNamespace, key strin } func (m *MetricsAdapter) GetRegion(ctx context.Context, storageID, storageNamespace string) (string, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(storageID)) + blockstoreType, err := m.adapter.BlockstoreType(storageID) + if err != nil { + return "", err + } + ctx = httputil.SetClientTrace(ctx, blockstoreType) return m.adapter.GetRegion(ctx, storageID, storageNamespace) } diff --git a/pkg/block/s3/adapter.go b/pkg/block/s3/adapter.go index 428b794eab8..021066c670e 100644 --- a/pkg/block/s3/adapter.go +++ b/pkg/block/s3/adapter.go @@ -851,8 +851,8 @@ func (a *Adapter) ListParts(ctx context.Context, obj block.ObjectPointer, upload return &partsResp, nil } -func (a *Adapter) BlockstoreType(_ string) string { - return block.BlockstoreTypeS3 +func (a *Adapter) BlockstoreType(_ string) (string, error) { + return block.BlockstoreTypeS3, nil } func (a *Adapter) BlockstoreMetadata(ctx context.Context, _ string) (*block.BlockstoreMetadata, error) { @@ -863,7 +863,7 @@ func (a *Adapter) BlockstoreMetadata(ctx context.Context, _ string) (*block.Bloc return &block.BlockstoreMetadata{Region: ®ion}, nil } -func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { +func (a *Adapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, error) { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeS3) if a.disablePreSigned { info.PreSignSupport = false @@ -874,7 +874,7 @@ func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { if !a.disablePreSignedMultipart && info.PreSignSupport { info.PreSignSupportMultipart = true } - return info + return info, nil } func resolveNamespace(obj block.ObjectPointer) (block.CommonQualifiedKey, error) { diff --git a/pkg/block/s3/adapter_test.go b/pkg/block/s3/adapter_test.go index f0e4aa406d7..ad83aa4f4b3 100644 --- a/pkg/block/s3/adapter_test.go +++ b/pkg/block/s3/adapter_test.go @@ -66,7 +66,9 @@ func TestS3AdapterPresignedOverride(t *testing.T) { // TestAdapterNamespace tests the namespace validity regex with various paths func TestAdapterNamespace(t *testing.T) { adapter := getS3BlockAdapter(t, nil) - expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo("").ValidityRegex) + namespaceInfo, err := adapter.GetStorageNamespaceInfo("") + require.NoError(t, err) + expr, err := regexp.Compile(namespaceInfo.ValidityRegex) require.NoError(t, err) tests := []struct { diff --git a/pkg/block/transient/adapter.go b/pkg/block/transient/adapter.go index d3cf49864ea..e2bef6a5cce 100644 --- a/pkg/block/transient/adapter.go +++ b/pkg/block/transient/adapter.go @@ -143,20 +143,20 @@ func (a *Adapter) CompleteMultiPartUpload(context.Context, block.ObjectPointer, }, nil } -func (a *Adapter) BlockstoreType(_ string) string { - return block.BlockstoreTypeTransient +func (a *Adapter) BlockstoreType(_ string) (string, error) { + return block.BlockstoreTypeTransient, nil } func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } -func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { +func (a *Adapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, error) { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeTransient) info.PreSignSupport = false info.PreSignSupportUI = false info.ImportSupport = false - return info + return info, nil } func (a *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { diff --git a/pkg/testutil/adapter.go b/pkg/testutil/adapter.go index 08d87c70dd0..1820d40594f 100644 --- a/pkg/testutil/adapter.go +++ b/pkg/testutil/adapter.go @@ -129,8 +129,8 @@ func (a *MockAdapter) ListParts(_ context.Context, _ block.ObjectPointer, _ stri panic("try to list parts in mock adapter") } -func (a *MockAdapter) BlockstoreType(_ string) string { - return "s3" +func (a *MockAdapter) BlockstoreType(_ string) (string, error) { + return "s3", nil } func (a *MockAdapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { @@ -141,11 +141,11 @@ func (a *MockAdapter) BlockstoreMetadata(_ context.Context, _ string) (*block.Bl } } -func (a *MockAdapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { +func (a *MockAdapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, error) { info := block.DefaultStorageNamespaceInfo("s3") info.PreSignSupport = false info.ImportSupport = false - return info + return info, nil } func (a *MockAdapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { From 7bf9accbd57049636f52b028ed83ca0dd661255c Mon Sep 17 00:00:00 2001 From: Itai Gilo Date: Mon, 27 Jan 2025 18:01:37 +0200 Subject: [PATCH 04/14] Fix tests --- pkg/block/blocktest/adapter.go | 24 ++++++++++++++++-------- pkg/block/blocktest/multipart_suite.go | 14 +++++--------- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/pkg/block/blocktest/adapter.go b/pkg/block/blocktest/adapter.go index 3cc39df5f15..277ddd9631c 100644 --- a/pkg/block/blocktest/adapter.go +++ b/pkg/block/blocktest/adapter.go @@ -142,7 +142,9 @@ func testAdapterWalker(t *testing.T, adapter block.Adapter, storageNamespace str require.NoError(t, err) var prefix string if tt.prefix == "" { - if adapter.BlockstoreType("") != block.BlockstoreTypeLocal { + blockstoreType, err := adapter.BlockstoreType("") + require.NoError(t, err) + if blockstoreType != block.BlockstoreTypeLocal { prefix = testPrefix } @@ -154,7 +156,9 @@ func testAdapterWalker(t *testing.T, adapter block.Adapter, storageNamespace str } } } else { - if adapter.BlockstoreType("") != block.BlockstoreTypeLocal { + blockstoreType, err := adapter.BlockstoreType("") + require.NoError(t, err) + if blockstoreType != block.BlockstoreTypeLocal { prefix = tt.prefix } for j := 0; j < filesAndFolders; j++ { @@ -169,7 +173,7 @@ func testAdapterWalker(t *testing.T, adapter block.Adapter, storageNamespace str func testGetPreSignedURL(t *testing.T, adapter block.Adapter, storageNamespace string) { preSignedURL, exp := getPresignedURLBasicTest(t, adapter, storageNamespace) require.NotNil(t, exp) - expectedExpiry := expectedURLExp(adapter) + expectedExpiry := expectedURLExp(t, adapter) require.Equal(t, expectedExpiry, *exp) _, err := url.Parse(preSignedURL) require.NoError(t, err) @@ -179,7 +183,7 @@ func testGetPreSignedURL(t *testing.T, adapter block.Adapter, storageNamespace s func testGetPreSignedURLEndpointOverride(t *testing.T, adapter block.Adapter, storageNamespace string, oe *url.URL) { preSignedURL, exp := getPresignedURLBasicTest(t, adapter, storageNamespace) require.NotNil(t, exp) - expectedExpiry := expectedURLExp(adapter) + expectedExpiry := expectedURLExp(t, adapter) require.Equal(t, expectedExpiry, *exp) u, err := url.Parse(preSignedURL) require.NoError(t, err) @@ -193,10 +197,12 @@ func getPresignedURLBasicTest(t *testing.T, adapter block.Adapter, storageNamesp preSignedURL, exp, err := adapter.GetPreSignedURL(ctx, obj, block.PreSignModeRead) - if adapter.BlockstoreType("") == block.BlockstoreTypeGS { + blockstoreType, err := adapter.BlockstoreType("") + require.NoError(t, err) + if blockstoreType == block.BlockstoreTypeGS { require.ErrorContains(t, err, "no credentials found") return "", nil - } else if adapter.BlockstoreType("") == block.BlockstoreTypeLocal { + } else if blockstoreType == block.BlockstoreTypeLocal { require.ErrorIs(t, err, block.ErrOperationNotSupported) return "", nil } @@ -204,8 +210,10 @@ func getPresignedURLBasicTest(t *testing.T, adapter block.Adapter, storageNamesp return preSignedURL, &exp } -func expectedURLExp(adapter block.Adapter) time.Time { - if adapter.BlockstoreType("") == block.BlockstoreTypeAzure { +func expectedURLExp(t *testing.T, adapter block.Adapter) time.Time { + blockstoreType, err := adapter.BlockstoreType("") + require.NoError(t, err) + if blockstoreType == block.BlockstoreTypeAzure { // we didn't implement expiry for Azure yet return time.Time{} } else { diff --git a/pkg/block/blocktest/multipart_suite.go b/pkg/block/blocktest/multipart_suite.go index 3cb006d316a..1fdea8d5392 100644 --- a/pkg/block/blocktest/multipart_suite.go +++ b/pkg/block/blocktest/multipart_suite.go @@ -39,8 +39,7 @@ func testAdapterMultipartUpload(t *testing.T, adapter block.Adapter, storageName } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - blockstoreType, err := adapter.BlockstoreType("") - require.NoError(t, err) + blockstoreType, _ := adapter.BlockstoreType("") obj := block.ObjectPointer{ StorageID: "", StorageNamespace: storageNamespace, @@ -133,8 +132,7 @@ func testAdapterCopyPart(t *testing.T, adapter block.Adapter, storageNamespace s resp, err := adapter.CreateMultiPartUpload(ctx, objCopy, nil, block.CreateMultiPartUploadOpts{}) require.NoError(t, err) multiParts, err := copyPart(t, ctx, adapter, obj, objCopy, resp.UploadID) - blockstoreType, err := adapter.BlockstoreType("") - require.NoError(t, err) + blockstoreType, _ := adapter.BlockstoreType("") if blockstoreType == block.BlockstoreTypeAzure { require.ErrorContains(t, err, "not implemented") // azurite block store emulator did not yet implement this return @@ -158,8 +156,7 @@ func testAdapterCopyPartRange(t *testing.T, adapter block.Adapter, storageNamesp resp, err := adapter.CreateMultiPartUpload(ctx, objCopy, nil, block.CreateMultiPartUploadOpts{}) require.NoError(t, err) multiParts := copyPartRange(t, ctx, adapter, obj, objCopy, resp.UploadID) - blockstoreType, err := adapter.BlockstoreType("") - require.NoError(t, err) + blockstoreType, _ := adapter.BlockstoreType("") if blockstoreType == block.BlockstoreTypeAzure { // not implemented in Azure require.Nil(t, multiParts) // azurite block store emulator did not yet implement this @@ -205,8 +202,7 @@ func uploadMultiPart(t *testing.T, ctx context.Context, adapter block.Adapter, o func verifyListInvalid(t *testing.T, ctx context.Context, adapter block.Adapter, obj block.ObjectPointer, uploadID string) { t.Helper() _, err := adapter.ListParts(ctx, obj, uploadID, block.ListPartsOpts{}) - blockstoreType, err := adapter.BlockstoreType("") - require.NoError(t, err) + blockstoreType, _ := adapter.BlockstoreType("") if blockstoreType != block.BlockstoreTypeS3 { require.ErrorIs(t, err, block.ErrOperationNotSupported) } else { @@ -238,7 +234,7 @@ func copyPartRange(t *testing.T, ctx context.Context, adapter block.Adapter, obj partNumber := i + 1 var endPosition = startPosition + multipartPartSize partResp, err := adapter.UploadCopyPartRange(ctx, obj, objCopy, uploadID, partNumber, startPosition, endPosition) - blockstoreType, err := adapter.BlockstoreType("") + blockstoreType, _ := adapter.BlockstoreType("") if blockstoreType == block.BlockstoreTypeAzure { require.ErrorContains(t, err, "not implemented") // azurite block store emulator did not yet implement this return nil From 850f17bbcb9e9a1bce36fbc1b9c7ecb8000a8c7e Mon Sep 17 00:00:00 2001 From: Itai Gilo Date: Mon, 27 Jan 2025 21:07:54 +0200 Subject: [PATCH 05/14] Fix lint --- pkg/block/blocktest/adapter.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/block/blocktest/adapter.go b/pkg/block/blocktest/adapter.go index 277ddd9631c..4d52c5183c0 100644 --- a/pkg/block/blocktest/adapter.go +++ b/pkg/block/blocktest/adapter.go @@ -196,6 +196,7 @@ func getPresignedURLBasicTest(t *testing.T, adapter block.Adapter, storageNamesp obj, _ := objPointers(storageNamespace) preSignedURL, exp, err := adapter.GetPreSignedURL(ctx, obj, block.PreSignModeRead) + require.NoError(t, err) blockstoreType, err := adapter.BlockstoreType("") require.NoError(t, err) From de5213ed949c799b5a286d0a8156de715575e080 Mon Sep 17 00:00:00 2001 From: Itai Gilo Date: Tue, 28 Jan 2025 11:17:43 +0200 Subject: [PATCH 06/14] Revert "Fix lint" This reverts commit 850f17bbcb9e9a1bce36fbc1b9c7ecb8000a8c7e. --- pkg/block/blocktest/adapter.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/block/blocktest/adapter.go b/pkg/block/blocktest/adapter.go index 4d52c5183c0..277ddd9631c 100644 --- a/pkg/block/blocktest/adapter.go +++ b/pkg/block/blocktest/adapter.go @@ -196,7 +196,6 @@ func getPresignedURLBasicTest(t *testing.T, adapter block.Adapter, storageNamesp obj, _ := objPointers(storageNamespace) preSignedURL, exp, err := adapter.GetPreSignedURL(ctx, obj, block.PreSignModeRead) - require.NoError(t, err) blockstoreType, err := adapter.BlockstoreType("") require.NoError(t, err) From 00fe06e5f1803f4c9693befa48a58bf42b51dc29 Mon Sep 17 00:00:00 2001 From: Itai Gilo Date: Tue, 28 Jan 2025 11:17:51 +0200 Subject: [PATCH 07/14] Revert "Fix tests" This reverts commit 7bf9accbd57049636f52b028ed83ca0dd661255c. --- pkg/block/blocktest/adapter.go | 24 ++++++++---------------- pkg/block/blocktest/multipart_suite.go | 14 +++++++++----- 2 files changed, 17 insertions(+), 21 deletions(-) diff --git a/pkg/block/blocktest/adapter.go b/pkg/block/blocktest/adapter.go index 277ddd9631c..3cc39df5f15 100644 --- a/pkg/block/blocktest/adapter.go +++ b/pkg/block/blocktest/adapter.go @@ -142,9 +142,7 @@ func testAdapterWalker(t *testing.T, adapter block.Adapter, storageNamespace str require.NoError(t, err) var prefix string if tt.prefix == "" { - blockstoreType, err := adapter.BlockstoreType("") - require.NoError(t, err) - if blockstoreType != block.BlockstoreTypeLocal { + if adapter.BlockstoreType("") != block.BlockstoreTypeLocal { prefix = testPrefix } @@ -156,9 +154,7 @@ func testAdapterWalker(t *testing.T, adapter block.Adapter, storageNamespace str } } } else { - blockstoreType, err := adapter.BlockstoreType("") - require.NoError(t, err) - if blockstoreType != block.BlockstoreTypeLocal { + if adapter.BlockstoreType("") != block.BlockstoreTypeLocal { prefix = tt.prefix } for j := 0; j < filesAndFolders; j++ { @@ -173,7 +169,7 @@ func testAdapterWalker(t *testing.T, adapter block.Adapter, storageNamespace str func testGetPreSignedURL(t *testing.T, adapter block.Adapter, storageNamespace string) { preSignedURL, exp := getPresignedURLBasicTest(t, adapter, storageNamespace) require.NotNil(t, exp) - expectedExpiry := expectedURLExp(t, adapter) + expectedExpiry := expectedURLExp(adapter) require.Equal(t, expectedExpiry, *exp) _, err := url.Parse(preSignedURL) require.NoError(t, err) @@ -183,7 +179,7 @@ func testGetPreSignedURL(t *testing.T, adapter block.Adapter, storageNamespace s func testGetPreSignedURLEndpointOverride(t *testing.T, adapter block.Adapter, storageNamespace string, oe *url.URL) { preSignedURL, exp := getPresignedURLBasicTest(t, adapter, storageNamespace) require.NotNil(t, exp) - expectedExpiry := expectedURLExp(t, adapter) + expectedExpiry := expectedURLExp(adapter) require.Equal(t, expectedExpiry, *exp) u, err := url.Parse(preSignedURL) require.NoError(t, err) @@ -197,12 +193,10 @@ func getPresignedURLBasicTest(t *testing.T, adapter block.Adapter, storageNamesp preSignedURL, exp, err := adapter.GetPreSignedURL(ctx, obj, block.PreSignModeRead) - blockstoreType, err := adapter.BlockstoreType("") - require.NoError(t, err) - if blockstoreType == block.BlockstoreTypeGS { + if adapter.BlockstoreType("") == block.BlockstoreTypeGS { require.ErrorContains(t, err, "no credentials found") return "", nil - } else if blockstoreType == block.BlockstoreTypeLocal { + } else if adapter.BlockstoreType("") == block.BlockstoreTypeLocal { require.ErrorIs(t, err, block.ErrOperationNotSupported) return "", nil } @@ -210,10 +204,8 @@ func getPresignedURLBasicTest(t *testing.T, adapter block.Adapter, storageNamesp return preSignedURL, &exp } -func expectedURLExp(t *testing.T, adapter block.Adapter) time.Time { - blockstoreType, err := adapter.BlockstoreType("") - require.NoError(t, err) - if blockstoreType == block.BlockstoreTypeAzure { +func expectedURLExp(adapter block.Adapter) time.Time { + if adapter.BlockstoreType("") == block.BlockstoreTypeAzure { // we didn't implement expiry for Azure yet return time.Time{} } else { diff --git a/pkg/block/blocktest/multipart_suite.go b/pkg/block/blocktest/multipart_suite.go index 1fdea8d5392..3cb006d316a 100644 --- a/pkg/block/blocktest/multipart_suite.go +++ b/pkg/block/blocktest/multipart_suite.go @@ -39,7 +39,8 @@ func testAdapterMultipartUpload(t *testing.T, adapter block.Adapter, storageName } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - blockstoreType, _ := adapter.BlockstoreType("") + blockstoreType, err := adapter.BlockstoreType("") + require.NoError(t, err) obj := block.ObjectPointer{ StorageID: "", StorageNamespace: storageNamespace, @@ -132,7 +133,8 @@ func testAdapterCopyPart(t *testing.T, adapter block.Adapter, storageNamespace s resp, err := adapter.CreateMultiPartUpload(ctx, objCopy, nil, block.CreateMultiPartUploadOpts{}) require.NoError(t, err) multiParts, err := copyPart(t, ctx, adapter, obj, objCopy, resp.UploadID) - blockstoreType, _ := adapter.BlockstoreType("") + blockstoreType, err := adapter.BlockstoreType("") + require.NoError(t, err) if blockstoreType == block.BlockstoreTypeAzure { require.ErrorContains(t, err, "not implemented") // azurite block store emulator did not yet implement this return @@ -156,7 +158,8 @@ func testAdapterCopyPartRange(t *testing.T, adapter block.Adapter, storageNamesp resp, err := adapter.CreateMultiPartUpload(ctx, objCopy, nil, block.CreateMultiPartUploadOpts{}) require.NoError(t, err) multiParts := copyPartRange(t, ctx, adapter, obj, objCopy, resp.UploadID) - blockstoreType, _ := adapter.BlockstoreType("") + blockstoreType, err := adapter.BlockstoreType("") + require.NoError(t, err) if blockstoreType == block.BlockstoreTypeAzure { // not implemented in Azure require.Nil(t, multiParts) // azurite block store emulator did not yet implement this @@ -202,7 +205,8 @@ func uploadMultiPart(t *testing.T, ctx context.Context, adapter block.Adapter, o func verifyListInvalid(t *testing.T, ctx context.Context, adapter block.Adapter, obj block.ObjectPointer, uploadID string) { t.Helper() _, err := adapter.ListParts(ctx, obj, uploadID, block.ListPartsOpts{}) - blockstoreType, _ := adapter.BlockstoreType("") + blockstoreType, err := adapter.BlockstoreType("") + require.NoError(t, err) if blockstoreType != block.BlockstoreTypeS3 { require.ErrorIs(t, err, block.ErrOperationNotSupported) } else { @@ -234,7 +238,7 @@ func copyPartRange(t *testing.T, ctx context.Context, adapter block.Adapter, obj partNumber := i + 1 var endPosition = startPosition + multipartPartSize partResp, err := adapter.UploadCopyPartRange(ctx, obj, objCopy, uploadID, partNumber, startPosition, endPosition) - blockstoreType, _ := adapter.BlockstoreType("") + blockstoreType, err := adapter.BlockstoreType("") if blockstoreType == block.BlockstoreTypeAzure { require.ErrorContains(t, err, "not implemented") // azurite block store emulator did not yet implement this return nil From 9d4f4bc787aa7af6511b59fd29c6cdddc2092de1 Mon Sep 17 00:00:00 2001 From: Itai Gilo Date: Tue, 28 Jan 2025 11:17:59 +0200 Subject: [PATCH 08/14] Revert "Add error handling" This reverts commit 9d3f0a2edb9889f628ee4c36af6fe856cc8f3e74. --- cmd/lakefs/cmd/run.go | 5 +- pkg/api/controller.go | 66 ++++----------- pkg/api/controller_test.go | 3 +- pkg/block/adapter.go | 4 +- pkg/block/azure/adapter.go | 8 +- pkg/block/azure/adapter_test.go | 3 +- pkg/block/blocktest/multipart_suite.go | 18 ++-- pkg/block/gs/adapter.go | 8 +- pkg/block/gs/adapter_test.go | 4 +- pkg/block/local/adapter.go | 8 +- pkg/block/local/adapter_test.go | 4 +- pkg/block/mem/adapter.go | 8 +- pkg/block/metrics.go | 112 +++++-------------------- pkg/block/s3/adapter.go | 8 +- pkg/block/s3/adapter_test.go | 4 +- pkg/block/transient/adapter.go | 8 +- pkg/testutil/adapter.go | 8 +- 17 files changed, 75 insertions(+), 204 deletions(-) diff --git a/cmd/lakefs/cmd/run.go b/cmd/lakefs/cmd/run.go index f0f730fdc91..12bc4e2cf44 100644 --- a/cmd/lakefs/cmd/run.go +++ b/cmd/lakefs/cmd/run.go @@ -445,10 +445,7 @@ func checkRepos(ctx context.Context, logger logging.Logger, authMetadataManager } for _, repo := range repos { - adapterStorageType, err := blockStore.BlockstoreType(repo.StorageID) - if err != nil { - logger.WithError(err).Fatalf("Failed to get storage type for repository %s", repo.Name) - } + adapterStorageType := blockStore.BlockstoreType(repo.StorageID) nsURL, err := url.Parse(repo.StorageNamespace) if err != nil { logger.WithError(err).Fatalf("Failed to parse repository %s namespace '%s'", repo.Name, repo.StorageNamespace) diff --git a/pkg/api/controller.go b/pkg/api/controller.go index 18324c8f3a0..b40292e5bbc 100644 --- a/pkg/api/controller.go +++ b/pkg/api/controller.go @@ -172,12 +172,8 @@ func (c *Controller) CreatePresignMultipartUpload(w http.ResponseWriter, r *http return } - storageConfig, err := c.getStorageConfig(repo.StorageID) - if c.handleAPIError(ctx, w, r, err) { - return - } - // check if api is supported + storageConfig := c.getStorageConfig(repo.StorageID) if !swag.BoolValue(storageConfig.PreSignMultipartUpload) { writeError(w, r, http.StatusNotImplemented, "presign multipart upload API is not supported") return @@ -276,12 +272,8 @@ func (c *Controller) AbortPresignMultipartUpload(w http.ResponseWriter, r *http. return } - storageConfig, err := c.getStorageConfig(repo.StorageID) - if c.handleAPIError(ctx, w, r, err) { - return - } - // check if api is supported + storageConfig := c.getStorageConfig(repo.StorageID) if !swag.BoolValue(storageConfig.PreSignMultipartUpload) { writeError(w, r, http.StatusNotImplemented, "presign multipart upload API is not supported") return @@ -341,12 +333,8 @@ func (c *Controller) CompletePresignMultipartUpload(w http.ResponseWriter, r *ht return } - storageConfig, err := c.getStorageConfig(repo.StorageID) - if c.handleAPIError(ctx, w, r, err) { - return - } - // check if api is supported + storageConfig := c.getStorageConfig(repo.StorageID) if !swag.BoolValue(storageConfig.PreSignMultipartUpload) { writeError(w, r, http.StatusNotImplemented, "presign multipart upload API is not supported") return @@ -777,10 +765,7 @@ func (c *Controller) LinkPhysicalAddress(w http.ResponseWriter, r *http.Request, ifAbsent = true } - blockStoreType, err := c.BlockAdapter.BlockstoreType(repo.StorageID) - if err != nil { - c.handleAPIError(ctx, w, r, err) - } + blockStoreType := c.BlockAdapter.BlockstoreType(repo.StorageID) expectedType := qk.GetStorageType().BlockstoreType() if expectedType != blockStoreType { c.Logger.WithContext(ctx).WithFields(logging.Fields{ @@ -1867,10 +1852,7 @@ func (c *Controller) GetConfig(w http.ResponseWriter, r *http.Request) { } // TODO (gilo): is StorageID relevant here? - storageCfg, err := c.getStorageConfig("") - if c.handleAPIError(r.Context(), w, r, err) { - return - } + storageCfg := c.getStorageConfig("") // TODO (niro): Needs to be populated storageListCfg := apigen.StorageConfigList{} versionConfig := c.getVersionConfig() @@ -1888,24 +1870,16 @@ func (c *Controller) GetStorageConfig(w http.ResponseWriter, r *http.Request) { } // TODO (gilo): is StorageID relevant here? - storageConfig, err := c.getStorageConfig("") - if c.handleAPIError(r.Context(), w, r, err) { - return - } - - writeResponse(w, r, http.StatusOK, storageConfig) + writeResponse(w, r, http.StatusOK, c.getStorageConfig("")) } -func (c *Controller) getStorageConfig(storageID string) (apigen.StorageConfig, error) { - info, err := c.BlockAdapter.GetStorageNamespaceInfo(storageID) - if err != nil { - return apigen.StorageConfig{}, err - } +func (c *Controller) getStorageConfig(storageID string) apigen.StorageConfig { + info := c.BlockAdapter.GetStorageNamespaceInfo(storageID) defaultNamespacePrefix := swag.String(info.DefaultNamespacePrefix) if c.Config.GetBaseConfig().Blockstore.DefaultNamespacePrefix != nil { defaultNamespacePrefix = c.Config.GetBaseConfig().Blockstore.DefaultNamespacePrefix } - storageConfig := apigen.StorageConfig{ + return apigen.StorageConfig{ BlockstoreType: c.Config.GetBaseConfig().Blockstore.Type, BlockstoreNamespaceValidityRegex: info.ValidityRegex, BlockstoreNamespaceExample: info.Example, @@ -1916,7 +1890,6 @@ func (c *Controller) getStorageConfig(storageID string) (apigen.StorageConfig, e ImportValidityRegex: info.ImportValidityRegex, PreSignMultipartUpload: swag.Bool(info.PreSignSupportMultipart), } - return storageConfig, nil } func (c *Controller) HealthCheck(w http.ResponseWriter, r *http.Request) { @@ -2029,8 +2002,7 @@ func (c *Controller) CreateRepository(w http.ResponseWriter, r *http.Request, bo retErr = err reason = "bad_url" case errors.Is(err, block.ErrInvalidAddress): - blockstoreType, _ := c.BlockAdapter.BlockstoreType(storageID) - retErr = fmt.Errorf("%w, must match: %s", err, blockstoreType) + retErr = fmt.Errorf("%w, must match: %s", err, c.BlockAdapter.BlockstoreType(storageID)) reason = "invalid_namespace" case errors.Is(err, ErrStorageNamespaceInUse): retErr = err @@ -2106,11 +2078,7 @@ func (c *Controller) CreateRepository(w http.ResponseWriter, r *http.Request, bo } func (c *Controller) validateStorageNamespace(storageID, storageNamespace string) error { - storageNamespaceInfo, err := c.BlockAdapter.GetStorageNamespaceInfo(storageID) - if err != nil { - return fmt.Errorf("failed to get validity regex for storage ID %s: %w", storageID, block.ErrInvalidNamespace) - } - validRegex := storageNamespaceInfo.ValidityRegex + validRegex := c.BlockAdapter.GetStorageNamespaceInfo(storageID).ValidityRegex storagePrefixRegex, err := regexp.Compile(validRegex) if err != nil { return fmt.Errorf("failed to compile validity regex %s: %w", validRegex, block.ErrInvalidNamespace) @@ -3388,15 +3356,10 @@ func (c *Controller) StageObject(w http.ResponseWriter, r *http.Request, body ap } // see what storage type this is and whether it fits our configuration - namespaceInfo, err := c.BlockAdapter.GetStorageNamespaceInfo(repo.StorageID) - if c.handleAPIError(ctx, w, r, err) { - return - } - uriRegex := namespaceInfo.ValidityRegex + uriRegex := c.BlockAdapter.GetStorageNamespaceInfo(repo.StorageID).ValidityRegex if match, err := regexp.MatchString(uriRegex, body.PhysicalAddress); err != nil || !match { - blockstoreType, _ := c.BlockAdapter.BlockstoreType(repo.StorageID) writeError(w, r, http.StatusBadRequest, fmt.Sprintf("physical address is not valid for block adapter: %s", - blockstoreType, + c.BlockAdapter.BlockstoreType(repo.StorageID), )) return } @@ -5131,8 +5094,7 @@ func (c *Controller) Setup(w http.ResponseWriter, r *http.Request, body apigen.S } // TODO (gilo): which storageID should we use here? - blockstoreType, _ := c.BlockAdapter.BlockstoreType("") - meta := stats.NewMetadata(ctx, c.Logger, blockstoreType, c.MetadataManager, c.CloudMetadataProvider) + meta := stats.NewMetadata(ctx, c.Logger, c.BlockAdapter.BlockstoreType(""), c.MetadataManager, c.CloudMetadataProvider) c.Collector.SetInstallationID(meta.InstallationID) c.Collector.CollectMetadata(meta) c.Collector.CollectEvent(stats.Event{Class: "global", Name: "init", UserID: body.Username, Client: httputil.GetRequestLakeFSClient(r)}) diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index 274d9c419b6..7b4b24afe58 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -77,8 +77,7 @@ func verifyResponseOK(t testing.TB, resp Statuser, err error) { } func onBlock(deps *dependencies, path string) string { - blockstoreType, _ := deps.blocks.BlockstoreType("") - return fmt.Sprintf("%s://%s", blockstoreType, path) + return fmt.Sprintf("%s://%s", deps.blocks.BlockstoreType(""), path) } func TestController_ListRepositoriesHandler(t *testing.T) { diff --git a/pkg/block/adapter.go b/pkg/block/adapter.go index a6a89a9f498..ba7a5ac26f2 100644 --- a/pkg/block/adapter.go +++ b/pkg/block/adapter.go @@ -194,9 +194,9 @@ type Adapter interface { AbortMultiPartUpload(ctx context.Context, obj ObjectPointer, uploadID string) error CompleteMultiPartUpload(ctx context.Context, obj ObjectPointer, uploadID string, multipartList *MultipartUploadCompletion) (*CompleteMultiPartUploadResponse, error) - BlockstoreType(storageID string) (string, error) + BlockstoreType(storageID string) string BlockstoreMetadata(ctx context.Context, storageID string) (*BlockstoreMetadata, error) - GetStorageNamespaceInfo(storageID string) (StorageNamespaceInfo, error) + GetStorageNamespaceInfo(storageID string) StorageNamespaceInfo ResolveNamespace(storageID, storageNamespace, key string, identifierType IdentifierType) (QualifiedKey, error) // GetRegion storageID is not actively used, and it's here mainly for completeness diff --git a/pkg/block/azure/adapter.go b/pkg/block/azure/adapter.go index 0809cdcc382..123e98625a9 100644 --- a/pkg/block/azure/adapter.go +++ b/pkg/block/azure/adapter.go @@ -583,8 +583,8 @@ func (a *Adapter) AbortMultiPartUpload(_ context.Context, _ block.ObjectPointer, return nil } -func (a *Adapter) BlockstoreType(_ string) (string, error) { - return block.BlockstoreTypeAzure, nil +func (a *Adapter) BlockstoreType(_ string) string { + return block.BlockstoreTypeAzure } func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { @@ -606,7 +606,7 @@ func (a *Adapter) CompleteMultiPartUpload(ctx context.Context, obj block.ObjectP return completeMultipart(ctx, multipartList.Part, *containerURL, qualifiedKey.BlobURL) } -func (a *Adapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, error) { +func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeAzure) info.ImportValidityRegex = fmt.Sprintf(`^https?://[a-z0-9_-]+\.%s`, a.clientCache.params.Domain) @@ -619,7 +619,7 @@ func (a *Adapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, if !(a.disablePreSignedUI || a.disablePreSigned) { info.PreSignSupportUI = true } - return info, nil + return info } func (a *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { diff --git a/pkg/block/azure/adapter_test.go b/pkg/block/azure/adapter_test.go index 7dbcf25d2eb..32a0efa4801 100644 --- a/pkg/block/azure/adapter_test.go +++ b/pkg/block/azure/adapter_test.go @@ -135,8 +135,7 @@ func TestAdapterNamespace(t *testing.T) { } require.NoError(t, err, "create new adapter") - namespaceInfo, err := adapter.GetStorageNamespaceInfo("") - require.NoError(t, err) + namespaceInfo := adapter.GetStorageNamespaceInfo("") expr, err := regexp.Compile(namespaceInfo.ValidityRegex) require.NoError(t, err) diff --git a/pkg/block/blocktest/multipart_suite.go b/pkg/block/blocktest/multipart_suite.go index 3cb006d316a..79b16cd0482 100644 --- a/pkg/block/blocktest/multipart_suite.go +++ b/pkg/block/blocktest/multipart_suite.go @@ -39,8 +39,7 @@ func testAdapterMultipartUpload(t *testing.T, adapter block.Adapter, storageName } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - blockstoreType, err := adapter.BlockstoreType("") - require.NoError(t, err) + blockstoreType := adapter.BlockstoreType("") obj := block.ObjectPointer{ StorageID: "", StorageNamespace: storageNamespace, @@ -133,9 +132,7 @@ func testAdapterCopyPart(t *testing.T, adapter block.Adapter, storageNamespace s resp, err := adapter.CreateMultiPartUpload(ctx, objCopy, nil, block.CreateMultiPartUploadOpts{}) require.NoError(t, err) multiParts, err := copyPart(t, ctx, adapter, obj, objCopy, resp.UploadID) - blockstoreType, err := adapter.BlockstoreType("") - require.NoError(t, err) - if blockstoreType == block.BlockstoreTypeAzure { + if adapter.BlockstoreType("") == block.BlockstoreTypeAzure { require.ErrorContains(t, err, "not implemented") // azurite block store emulator did not yet implement this return } @@ -158,9 +155,7 @@ func testAdapterCopyPartRange(t *testing.T, adapter block.Adapter, storageNamesp resp, err := adapter.CreateMultiPartUpload(ctx, objCopy, nil, block.CreateMultiPartUploadOpts{}) require.NoError(t, err) multiParts := copyPartRange(t, ctx, adapter, obj, objCopy, resp.UploadID) - blockstoreType, err := adapter.BlockstoreType("") - require.NoError(t, err) - if blockstoreType == block.BlockstoreTypeAzure { + if adapter.BlockstoreType("") == block.BlockstoreTypeAzure { // not implemented in Azure require.Nil(t, multiParts) // azurite block store emulator did not yet implement this return @@ -205,9 +200,7 @@ func uploadMultiPart(t *testing.T, ctx context.Context, adapter block.Adapter, o func verifyListInvalid(t *testing.T, ctx context.Context, adapter block.Adapter, obj block.ObjectPointer, uploadID string) { t.Helper() _, err := adapter.ListParts(ctx, obj, uploadID, block.ListPartsOpts{}) - blockstoreType, err := adapter.BlockstoreType("") - require.NoError(t, err) - if blockstoreType != block.BlockstoreTypeS3 { + if adapter.BlockstoreType("") != block.BlockstoreTypeS3 { require.ErrorIs(t, err, block.ErrOperationNotSupported) } else { require.NotNil(t, err) @@ -238,8 +231,7 @@ func copyPartRange(t *testing.T, ctx context.Context, adapter block.Adapter, obj partNumber := i + 1 var endPosition = startPosition + multipartPartSize partResp, err := adapter.UploadCopyPartRange(ctx, obj, objCopy, uploadID, partNumber, startPosition, endPosition) - blockstoreType, err := adapter.BlockstoreType("") - if blockstoreType == block.BlockstoreTypeAzure { + if adapter.BlockstoreType("") == block.BlockstoreTypeAzure { require.ErrorContains(t, err, "not implemented") // azurite block store emulator did not yet implement this return nil } diff --git a/pkg/block/gs/adapter.go b/pkg/block/gs/adapter.go index 04b943e54c3..71d48d4da9b 100644 --- a/pkg/block/gs/adapter.go +++ b/pkg/block/gs/adapter.go @@ -650,15 +650,15 @@ func (a *Adapter) Close() error { return a.client.Close() } -func (a *Adapter) BlockstoreType(_ string) (string, error) { - return block.BlockstoreTypeGS, nil +func (a *Adapter) BlockstoreType(_ string) string { + return block.BlockstoreTypeGS } func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } -func (a *Adapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, error) { +func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeGS) if a.disablePreSigned { info.PreSignSupport = false @@ -666,7 +666,7 @@ func (a *Adapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, if !(a.disablePreSignedUI || a.disablePreSigned) { info.PreSignSupportUI = true } - return info, nil + return info } func (a *Adapter) extractParamsFromObj(obj block.ObjectPointer) (string, string, error) { diff --git a/pkg/block/gs/adapter_test.go b/pkg/block/gs/adapter_test.go index 70594cfff68..252cef86613 100644 --- a/pkg/block/gs/adapter_test.go +++ b/pkg/block/gs/adapter_test.go @@ -36,9 +36,7 @@ func TestAdapterNamespace(t *testing.T) { require.NoError(t, adapter.Close()) }() - namespaceInfo, err := adapter.GetStorageNamespaceInfo("") - require.NoError(t, err) - expr, err := regexp.Compile(namespaceInfo.ValidityRegex) + expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo("").ValidityRegex) require.NoError(t, err) tests := []struct { diff --git a/pkg/block/local/adapter.go b/pkg/block/local/adapter.go index 002e6e0c254..d9c42470b9c 100644 --- a/pkg/block/local/adapter.go +++ b/pkg/block/local/adapter.go @@ -537,20 +537,20 @@ func (l *Adapter) getPartFiles(uploadID string, obj block.ObjectPointer) ([]stri return names, nil } -func (l *Adapter) BlockstoreType(_ string) (string, error) { - return block.BlockstoreTypeLocal, nil +func (l *Adapter) BlockstoreType(_ string) string { + return block.BlockstoreTypeLocal } func (l *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } -func (l *Adapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, error) { +func (l *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeLocal) info.PreSignSupport = false info.DefaultNamespacePrefix = DefaultNamespacePrefix info.ImportSupport = l.importEnabled - return info, nil + return info } func (l *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { diff --git a/pkg/block/local/adapter_test.go b/pkg/block/local/adapter_test.go index ac78c91f2c7..a45ee299d9e 100644 --- a/pkg/block/local/adapter_test.go +++ b/pkg/block/local/adapter_test.go @@ -31,9 +31,7 @@ func TestAdapterNamespace(t *testing.T) { localPath := path.Join(tmpDir, "lakefs") adapter, err := local.NewAdapter(localPath, local.WithRemoveEmptyDir(false)) require.NoError(t, err, "create new adapter") - namespaceInfo, err := adapter.GetStorageNamespaceInfo("") - require.NoError(t, err) - expr, err := regexp.Compile(namespaceInfo.ValidityRegex) + expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo("").ValidityRegex) require.NoError(t, err) tests := []struct { diff --git a/pkg/block/mem/adapter.go b/pkg/block/mem/adapter.go index d09c66dffc7..feb76412fb4 100644 --- a/pkg/block/mem/adapter.go +++ b/pkg/block/mem/adapter.go @@ -335,19 +335,19 @@ func (a *Adapter) CompleteMultiPartUpload(_ context.Context, obj block.ObjectPoi }, nil } -func (a *Adapter) BlockstoreType(_ string) (string, error) { - return block.BlockstoreTypeMem, nil +func (a *Adapter) BlockstoreType(_ string) string { + return block.BlockstoreTypeMem } func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } -func (a *Adapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, error) { +func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeMem) info.PreSignSupport = false info.ImportSupport = false - return info, nil + return info } func (a *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { diff --git a/pkg/block/metrics.go b/pkg/block/metrics.go index df8166f13d3..c7d2474a914 100644 --- a/pkg/block/metrics.go +++ b/pkg/block/metrics.go @@ -23,20 +23,12 @@ func (m *MetricsAdapter) InnerAdapter() Adapter { } func (m *MetricsAdapter) Put(ctx context.Context, obj ObjectPointer, sizeBytes int64, reader io.Reader, opts PutOpts) (*PutResponse, error) { - blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) - if err != nil { - return nil, err - } - ctx = httputil.SetClientTrace(ctx, blockstoreType) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.Put(ctx, obj, sizeBytes, reader, opts) } func (m *MetricsAdapter) Get(ctx context.Context, obj ObjectPointer) (io.ReadCloser, error) { - blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) - if err != nil { - return nil, err - } - ctx = httputil.SetClientTrace(ctx, blockstoreType) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.Get(ctx, obj) } @@ -45,145 +37,85 @@ func (m *MetricsAdapter) GetWalker(uri *url.URL) (Walker, error) { } func (m *MetricsAdapter) GetPreSignedURL(ctx context.Context, obj ObjectPointer, mode PreSignMode) (string, time.Time, error) { - blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) - if err != nil { - return "", time.Time{}, err - } - ctx = httputil.SetClientTrace(ctx, blockstoreType) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.GetPreSignedURL(ctx, obj, mode) } func (m *MetricsAdapter) GetPresignUploadPartURL(ctx context.Context, obj ObjectPointer, uploadID string, partNumber int) (string, error) { - blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) - if err != nil { - return "", err - } - ctx = httputil.SetClientTrace(ctx, blockstoreType) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.GetPresignUploadPartURL(ctx, obj, uploadID, partNumber) } func (m *MetricsAdapter) Exists(ctx context.Context, obj ObjectPointer) (bool, error) { - blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) - if err != nil { - return false, err - } - ctx = httputil.SetClientTrace(ctx, blockstoreType) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.Exists(ctx, obj) } func (m *MetricsAdapter) GetRange(ctx context.Context, obj ObjectPointer, startPosition int64, endPosition int64) (io.ReadCloser, error) { - blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) - if err != nil { - return nil, err - } - ctx = httputil.SetClientTrace(ctx, blockstoreType) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.GetRange(ctx, obj, startPosition, endPosition) } func (m *MetricsAdapter) GetProperties(ctx context.Context, obj ObjectPointer) (Properties, error) { - blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) - if err != nil { - return Properties{}, err - } - ctx = httputil.SetClientTrace(ctx, blockstoreType) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.GetProperties(ctx, obj) } func (m *MetricsAdapter) Remove(ctx context.Context, obj ObjectPointer) error { - blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) - if err != nil { - return err - } - ctx = httputil.SetClientTrace(ctx, blockstoreType) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.Remove(ctx, obj) } func (m *MetricsAdapter) Copy(ctx context.Context, sourceObj, destinationObj ObjectPointer) error { - blockstoreType, err := m.adapter.BlockstoreType(sourceObj.StorageID) - if err != nil { - return err - } - ctx = httputil.SetClientTrace(ctx, blockstoreType) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(sourceObj.StorageID)) return m.adapter.Copy(ctx, sourceObj, destinationObj) } func (m *MetricsAdapter) CreateMultiPartUpload(ctx context.Context, obj ObjectPointer, r *http.Request, opts CreateMultiPartUploadOpts) (*CreateMultiPartUploadResponse, error) { - blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) - if err != nil { - return nil, err - } - ctx = httputil.SetClientTrace(ctx, blockstoreType) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.CreateMultiPartUpload(ctx, obj, r, opts) } func (m *MetricsAdapter) UploadPart(ctx context.Context, obj ObjectPointer, sizeBytes int64, reader io.Reader, uploadID string, partNumber int) (*UploadPartResponse, error) { - blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) - if err != nil { - return nil, err - } - ctx = httputil.SetClientTrace(ctx, blockstoreType) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.UploadPart(ctx, obj, sizeBytes, reader, uploadID, partNumber) } func (m *MetricsAdapter) ListParts(ctx context.Context, obj ObjectPointer, uploadID string, opts ListPartsOpts) (*ListPartsResponse, error) { - blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) - if err != nil { - return nil, err - } - ctx = httputil.SetClientTrace(ctx, blockstoreType) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.ListParts(ctx, obj, uploadID, opts) } func (m *MetricsAdapter) UploadCopyPart(ctx context.Context, sourceObj, destinationObj ObjectPointer, uploadID string, partNumber int) (*UploadPartResponse, error) { - blockstoreType, err := m.adapter.BlockstoreType(sourceObj.StorageID) - if err != nil { - return nil, err - } - ctx = httputil.SetClientTrace(ctx, blockstoreType) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(sourceObj.StorageID)) return m.adapter.UploadCopyPart(ctx, sourceObj, destinationObj, uploadID, partNumber) } func (m *MetricsAdapter) UploadCopyPartRange(ctx context.Context, sourceObj, destinationObj ObjectPointer, uploadID string, partNumber int, startPosition, endPosition int64) (*UploadPartResponse, error) { - blockstoreType, err := m.adapter.BlockstoreType(sourceObj.StorageID) - if err != nil { - return nil, err - } - ctx = httputil.SetClientTrace(ctx, blockstoreType) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(sourceObj.StorageID)) return m.adapter.UploadCopyPartRange(ctx, sourceObj, destinationObj, uploadID, partNumber, startPosition, endPosition) } func (m *MetricsAdapter) AbortMultiPartUpload(ctx context.Context, obj ObjectPointer, uploadID string) error { - blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) - if err != nil { - return err - } - ctx = httputil.SetClientTrace(ctx, blockstoreType) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.AbortMultiPartUpload(ctx, obj, uploadID) } func (m *MetricsAdapter) CompleteMultiPartUpload(ctx context.Context, obj ObjectPointer, uploadID string, multipartList *MultipartUploadCompletion) (*CompleteMultiPartUploadResponse, error) { - blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID) - if err != nil { - return nil, err - } - ctx = httputil.SetClientTrace(ctx, blockstoreType) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) return m.adapter.CompleteMultiPartUpload(ctx, obj, uploadID, multipartList) } -func (m *MetricsAdapter) BlockstoreType(storageID string) (string, error) { +func (m *MetricsAdapter) BlockstoreType(storageID string) string { return m.adapter.BlockstoreType(storageID) } func (m *MetricsAdapter) BlockstoreMetadata(ctx context.Context, storageID string) (*BlockstoreMetadata, error) { - blockstoreType, err := m.adapter.BlockstoreType(storageID) - if err != nil { - return nil, err - } - ctx = httputil.SetClientTrace(ctx, blockstoreType) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(storageID)) return m.adapter.BlockstoreMetadata(ctx, storageID) } -func (m *MetricsAdapter) GetStorageNamespaceInfo(storageID string) (StorageNamespaceInfo, error) { +func (m *MetricsAdapter) GetStorageNamespaceInfo(storageID string) StorageNamespaceInfo { return m.adapter.GetStorageNamespaceInfo(storageID) } @@ -192,11 +124,7 @@ func (m *MetricsAdapter) ResolveNamespace(storageID, storageNamespace, key strin } func (m *MetricsAdapter) GetRegion(ctx context.Context, storageID, storageNamespace string) (string, error) { - blockstoreType, err := m.adapter.BlockstoreType(storageID) - if err != nil { - return "", err - } - ctx = httputil.SetClientTrace(ctx, blockstoreType) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(storageID)) return m.adapter.GetRegion(ctx, storageID, storageNamespace) } diff --git a/pkg/block/s3/adapter.go b/pkg/block/s3/adapter.go index 021066c670e..428b794eab8 100644 --- a/pkg/block/s3/adapter.go +++ b/pkg/block/s3/adapter.go @@ -851,8 +851,8 @@ func (a *Adapter) ListParts(ctx context.Context, obj block.ObjectPointer, upload return &partsResp, nil } -func (a *Adapter) BlockstoreType(_ string) (string, error) { - return block.BlockstoreTypeS3, nil +func (a *Adapter) BlockstoreType(_ string) string { + return block.BlockstoreTypeS3 } func (a *Adapter) BlockstoreMetadata(ctx context.Context, _ string) (*block.BlockstoreMetadata, error) { @@ -863,7 +863,7 @@ func (a *Adapter) BlockstoreMetadata(ctx context.Context, _ string) (*block.Bloc return &block.BlockstoreMetadata{Region: ®ion}, nil } -func (a *Adapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, error) { +func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeS3) if a.disablePreSigned { info.PreSignSupport = false @@ -874,7 +874,7 @@ func (a *Adapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, if !a.disablePreSignedMultipart && info.PreSignSupport { info.PreSignSupportMultipart = true } - return info, nil + return info } func resolveNamespace(obj block.ObjectPointer) (block.CommonQualifiedKey, error) { diff --git a/pkg/block/s3/adapter_test.go b/pkg/block/s3/adapter_test.go index ad83aa4f4b3..f0e4aa406d7 100644 --- a/pkg/block/s3/adapter_test.go +++ b/pkg/block/s3/adapter_test.go @@ -66,9 +66,7 @@ func TestS3AdapterPresignedOverride(t *testing.T) { // TestAdapterNamespace tests the namespace validity regex with various paths func TestAdapterNamespace(t *testing.T) { adapter := getS3BlockAdapter(t, nil) - namespaceInfo, err := adapter.GetStorageNamespaceInfo("") - require.NoError(t, err) - expr, err := regexp.Compile(namespaceInfo.ValidityRegex) + expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo("").ValidityRegex) require.NoError(t, err) tests := []struct { diff --git a/pkg/block/transient/adapter.go b/pkg/block/transient/adapter.go index e2bef6a5cce..d3cf49864ea 100644 --- a/pkg/block/transient/adapter.go +++ b/pkg/block/transient/adapter.go @@ -143,20 +143,20 @@ func (a *Adapter) CompleteMultiPartUpload(context.Context, block.ObjectPointer, }, nil } -func (a *Adapter) BlockstoreType(_ string) (string, error) { - return block.BlockstoreTypeTransient, nil +func (a *Adapter) BlockstoreType(_ string) string { + return block.BlockstoreTypeTransient } func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } -func (a *Adapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, error) { +func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeTransient) info.PreSignSupport = false info.PreSignSupportUI = false info.ImportSupport = false - return info, nil + return info } func (a *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { diff --git a/pkg/testutil/adapter.go b/pkg/testutil/adapter.go index 1820d40594f..08d87c70dd0 100644 --- a/pkg/testutil/adapter.go +++ b/pkg/testutil/adapter.go @@ -129,8 +129,8 @@ func (a *MockAdapter) ListParts(_ context.Context, _ block.ObjectPointer, _ stri panic("try to list parts in mock adapter") } -func (a *MockAdapter) BlockstoreType(_ string) (string, error) { - return "s3", nil +func (a *MockAdapter) BlockstoreType(_ string) string { + return "s3" } func (a *MockAdapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { @@ -141,11 +141,11 @@ func (a *MockAdapter) BlockstoreMetadata(_ context.Context, _ string) (*block.Bl } } -func (a *MockAdapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, error) { +func (a *MockAdapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { info := block.DefaultStorageNamespaceInfo("s3") info.PreSignSupport = false info.ImportSupport = false - return info, nil + return info } func (a *MockAdapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { From 24f5136bcda82a8ce8f4a7e78d6cab7c2cd38f0e Mon Sep 17 00:00:00 2001 From: Itai Gilo Date: Tue, 28 Jan 2025 11:18:06 +0200 Subject: [PATCH 09/14] Revert "Fix tests" This reverts commit a2a4ab5eac9edf585aba9b03f1ff06362f2da660. --- cmd/lakefs/cmd/run.go | 2 +- pkg/api/controller_test.go | 2 +- pkg/block/azure/adapter_test.go | 2 +- pkg/block/blocktest/adapter.go | 12 ++++++------ pkg/block/blocktest/basic_suite.go | 2 +- pkg/block/blocktest/multipart_suite.go | 10 +++++----- pkg/block/gs/adapter_test.go | 2 +- pkg/block/local/adapter_test.go | 2 +- pkg/block/s3/adapter_test.go | 2 +- 9 files changed, 18 insertions(+), 18 deletions(-) diff --git a/cmd/lakefs/cmd/run.go b/cmd/lakefs/cmd/run.go index 12bc4e2cf44..bdb85666843 100644 --- a/cmd/lakefs/cmd/run.go +++ b/cmd/lakefs/cmd/run.go @@ -431,7 +431,7 @@ func checkRepos(ctx context.Context, logger logging.Logger, authMetadataManager } else { logger. // TODO (gilo): uncomment this? - // WithField("adapter_type", blockStore.BlockstoreType()). + //WithField("adapter_type", blockStore.BlockstoreType()). Debug("lakeFS is initialized, checking repositories for mismatched adapter") hasMore := true next := "" diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index 7b4b24afe58..9e319989c40 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -77,7 +77,7 @@ func verifyResponseOK(t testing.TB, resp Statuser, err error) { } func onBlock(deps *dependencies, path string) string { - return fmt.Sprintf("%s://%s", deps.blocks.BlockstoreType(""), path) + return fmt.Sprintf("%s://%s", deps.blocks.BlockstoreType(), path) } func TestController_ListRepositoriesHandler(t *testing.T) { diff --git a/pkg/block/azure/adapter_test.go b/pkg/block/azure/adapter_test.go index 32a0efa4801..5b13deb0357 100644 --- a/pkg/block/azure/adapter_test.go +++ b/pkg/block/azure/adapter_test.go @@ -135,7 +135,7 @@ func TestAdapterNamespace(t *testing.T) { } require.NoError(t, err, "create new adapter") - namespaceInfo := adapter.GetStorageNamespaceInfo("") + namespaceInfo := adapter.GetStorageNamespaceInfo() expr, err := regexp.Compile(namespaceInfo.ValidityRegex) require.NoError(t, err) diff --git a/pkg/block/blocktest/adapter.go b/pkg/block/blocktest/adapter.go index 3cc39df5f15..5fa7f8010de 100644 --- a/pkg/block/blocktest/adapter.go +++ b/pkg/block/blocktest/adapter.go @@ -126,7 +126,7 @@ func testAdapterWalker(t *testing.T, adapter block.Adapter, storageNamespace str }, } for _, tt := range cases { - qk, err := adapter.ResolveNamespace("", storageNamespace, filepath.Join(testPrefix, tt.prefix), block.IdentifierTypeRelative) + qk, err := adapter.ResolveNamespace(storageNamespace, filepath.Join(testPrefix, tt.prefix), block.IdentifierTypeRelative) require.NoError(t, err) uri, err := url.Parse(qk.Format()) require.NoError(t, err) @@ -142,7 +142,7 @@ func testAdapterWalker(t *testing.T, adapter block.Adapter, storageNamespace str require.NoError(t, err) var prefix string if tt.prefix == "" { - if adapter.BlockstoreType("") != block.BlockstoreTypeLocal { + if adapter.BlockstoreType() != block.BlockstoreTypeLocal { prefix = testPrefix } @@ -154,7 +154,7 @@ func testAdapterWalker(t *testing.T, adapter block.Adapter, storageNamespace str } } } else { - if adapter.BlockstoreType("") != block.BlockstoreTypeLocal { + if adapter.BlockstoreType() != block.BlockstoreTypeLocal { prefix = tt.prefix } for j := 0; j < filesAndFolders; j++ { @@ -193,10 +193,10 @@ func getPresignedURLBasicTest(t *testing.T, adapter block.Adapter, storageNamesp preSignedURL, exp, err := adapter.GetPreSignedURL(ctx, obj, block.PreSignModeRead) - if adapter.BlockstoreType("") == block.BlockstoreTypeGS { + if adapter.BlockstoreType() == block.BlockstoreTypeGS { require.ErrorContains(t, err, "no credentials found") return "", nil - } else if adapter.BlockstoreType("") == block.BlockstoreTypeLocal { + } else if adapter.BlockstoreType() == block.BlockstoreTypeLocal { require.ErrorIs(t, err, block.ErrOperationNotSupported) return "", nil } @@ -205,7 +205,7 @@ func getPresignedURLBasicTest(t *testing.T, adapter block.Adapter, storageNamesp } func expectedURLExp(adapter block.Adapter) time.Time { - if adapter.BlockstoreType("") == block.BlockstoreTypeAzure { + if adapter.BlockstoreType() == block.BlockstoreTypeAzure { // we didn't implement expiry for Azure yet return time.Time{} } else { diff --git a/pkg/block/blocktest/basic_suite.go b/pkg/block/blocktest/basic_suite.go index 9e8e85c9dbc..f96d5efb98a 100644 --- a/pkg/block/blocktest/basic_suite.go +++ b/pkg/block/blocktest/basic_suite.go @@ -153,7 +153,7 @@ func testAdapterRemove(t *testing.T, adapter block.Adapter, storageNamespace str t.Errorf("Remove() error = %v, wantErr %v", err, tt.wantErr) } - qk, err := adapter.ResolveNamespace("", storageNamespace, tt.name, block.IdentifierTypeRelative) + qk, err := adapter.ResolveNamespace(storageNamespace, tt.name, block.IdentifierTypeRelative) require.NoError(t, err) tree := dumpPathTree(t, ctx, adapter, qk) diff --git a/pkg/block/blocktest/multipart_suite.go b/pkg/block/blocktest/multipart_suite.go index 79b16cd0482..a8961bd95fb 100644 --- a/pkg/block/blocktest/multipart_suite.go +++ b/pkg/block/blocktest/multipart_suite.go @@ -39,7 +39,7 @@ func testAdapterMultipartUpload(t *testing.T, adapter block.Adapter, storageName } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - blockstoreType := adapter.BlockstoreType("") + blockstoreType := adapter.BlockstoreType() obj := block.ObjectPointer{ StorageID: "", StorageNamespace: storageNamespace, @@ -132,7 +132,7 @@ func testAdapterCopyPart(t *testing.T, adapter block.Adapter, storageNamespace s resp, err := adapter.CreateMultiPartUpload(ctx, objCopy, nil, block.CreateMultiPartUploadOpts{}) require.NoError(t, err) multiParts, err := copyPart(t, ctx, adapter, obj, objCopy, resp.UploadID) - if adapter.BlockstoreType("") == block.BlockstoreTypeAzure { + if adapter.BlockstoreType() == block.BlockstoreTypeAzure { require.ErrorContains(t, err, "not implemented") // azurite block store emulator did not yet implement this return } @@ -155,7 +155,7 @@ func testAdapterCopyPartRange(t *testing.T, adapter block.Adapter, storageNamesp resp, err := adapter.CreateMultiPartUpload(ctx, objCopy, nil, block.CreateMultiPartUploadOpts{}) require.NoError(t, err) multiParts := copyPartRange(t, ctx, adapter, obj, objCopy, resp.UploadID) - if adapter.BlockstoreType("") == block.BlockstoreTypeAzure { + if adapter.BlockstoreType() == block.BlockstoreTypeAzure { // not implemented in Azure require.Nil(t, multiParts) // azurite block store emulator did not yet implement this return @@ -200,7 +200,7 @@ func uploadMultiPart(t *testing.T, ctx context.Context, adapter block.Adapter, o func verifyListInvalid(t *testing.T, ctx context.Context, adapter block.Adapter, obj block.ObjectPointer, uploadID string) { t.Helper() _, err := adapter.ListParts(ctx, obj, uploadID, block.ListPartsOpts{}) - if adapter.BlockstoreType("") != block.BlockstoreTypeS3 { + if adapter.BlockstoreType() != block.BlockstoreTypeS3 { require.ErrorIs(t, err, block.ErrOperationNotSupported) } else { require.NotNil(t, err) @@ -231,7 +231,7 @@ func copyPartRange(t *testing.T, ctx context.Context, adapter block.Adapter, obj partNumber := i + 1 var endPosition = startPosition + multipartPartSize partResp, err := adapter.UploadCopyPartRange(ctx, obj, objCopy, uploadID, partNumber, startPosition, endPosition) - if adapter.BlockstoreType("") == block.BlockstoreTypeAzure { + if adapter.BlockstoreType() == block.BlockstoreTypeAzure { require.ErrorContains(t, err, "not implemented") // azurite block store emulator did not yet implement this return nil } diff --git a/pkg/block/gs/adapter_test.go b/pkg/block/gs/adapter_test.go index 252cef86613..bea1bd39146 100644 --- a/pkg/block/gs/adapter_test.go +++ b/pkg/block/gs/adapter_test.go @@ -36,7 +36,7 @@ func TestAdapterNamespace(t *testing.T) { require.NoError(t, adapter.Close()) }() - expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo("").ValidityRegex) + expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo().ValidityRegex) require.NoError(t, err) tests := []struct { diff --git a/pkg/block/local/adapter_test.go b/pkg/block/local/adapter_test.go index a45ee299d9e..054b13e589c 100644 --- a/pkg/block/local/adapter_test.go +++ b/pkg/block/local/adapter_test.go @@ -31,7 +31,7 @@ func TestAdapterNamespace(t *testing.T) { localPath := path.Join(tmpDir, "lakefs") adapter, err := local.NewAdapter(localPath, local.WithRemoveEmptyDir(false)) require.NoError(t, err, "create new adapter") - expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo("").ValidityRegex) + expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo().ValidityRegex) require.NoError(t, err) tests := []struct { diff --git a/pkg/block/s3/adapter_test.go b/pkg/block/s3/adapter_test.go index f0e4aa406d7..90dde42c06d 100644 --- a/pkg/block/s3/adapter_test.go +++ b/pkg/block/s3/adapter_test.go @@ -66,7 +66,7 @@ func TestS3AdapterPresignedOverride(t *testing.T) { // TestAdapterNamespace tests the namespace validity regex with various paths func TestAdapterNamespace(t *testing.T) { adapter := getS3BlockAdapter(t, nil) - expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo("").ValidityRegex) + expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo().ValidityRegex) require.NoError(t, err) tests := []struct { From 17f2e8e1bad10907a09d0a01ae64db5c272a10a8 Mon Sep 17 00:00:00 2001 From: Itai Gilo Date: Tue, 28 Jan 2025 11:18:12 +0200 Subject: [PATCH 10/14] Revert "Add StorageID to adapter metadata functions" This reverts commit 06b32ee76021ecdcd11172de310b8a1441e71ba4. --- cmd/lakefs/cmd/run.go | 5 +- pkg/api/controller.go | 83 +++++++++---------- pkg/block/adapter.go | 8 +- pkg/block/azure/adapter.go | 8 +- pkg/block/gs/adapter.go | 10 +-- pkg/block/local/adapter.go | 8 +- pkg/block/mem/adapter.go | 8 +- pkg/block/metrics.go | 52 ++++++------ pkg/block/s3/adapter.go | 10 +-- pkg/block/transient/adapter.go | 8 +- pkg/block/validations.go | 2 +- .../retention/garbage_collection_manager.go | 9 +- pkg/testutil/adapter.go | 8 +- 13 files changed, 106 insertions(+), 113 deletions(-) diff --git a/cmd/lakefs/cmd/run.go b/cmd/lakefs/cmd/run.go index bdb85666843..be99b374581 100644 --- a/cmd/lakefs/cmd/run.go +++ b/cmd/lakefs/cmd/run.go @@ -430,8 +430,7 @@ func checkRepos(ctx context.Context, logger logging.Logger, authMetadataManager logger.Debug("lakeFS isn't initialized, skipping mismatched adapter checks") } else { logger. - // TODO (gilo): uncomment this? - //WithField("adapter_type", blockStore.BlockstoreType()). + WithField("adapter_type", blockStore.BlockstoreType()). Debug("lakeFS is initialized, checking repositories for mismatched adapter") hasMore := true next := "" @@ -444,8 +443,8 @@ func checkRepos(ctx context.Context, logger logging.Logger, authMetadataManager logger.WithError(err).Fatal("Checking existing repositories failed") } + adapterStorageType := blockStore.BlockstoreType() for _, repo := range repos { - adapterStorageType := blockStore.BlockstoreType(repo.StorageID) nsURL, err := url.Parse(repo.StorageNamespace) if err != nil { logger.WithError(err).Fatalf("Failed to parse repository %s namespace '%s'", repo.Name, repo.StorageNamespace) diff --git a/pkg/api/controller.go b/pkg/api/controller.go index b40292e5bbc..e42e41fe3a5 100644 --- a/pkg/api/controller.go +++ b/pkg/api/controller.go @@ -167,18 +167,18 @@ func (c *Controller) CreatePresignMultipartUpload(w http.ResponseWriter, r *http ctx := r.Context() c.LogAction(ctx, "create_presign_multipart_upload", r, repository, branch, "") - repo, err := c.Catalog.GetRepository(ctx, repository) - if c.handleAPIError(ctx, w, r, err) { - return - } - // check if api is supported - storageConfig := c.getStorageConfig(repo.StorageID) + storageConfig := c.getStorageConfig() if !swag.BoolValue(storageConfig.PreSignMultipartUpload) { writeError(w, r, http.StatusNotImplemented, "presign multipart upload API is not supported") return } + repo, err := c.Catalog.GetRepository(ctx, repository) + if c.handleAPIError(ctx, w, r, err) { + return + } + // check if the branch exists - it is still possible for a branch to be deleted later, but we don't want to // upload to start and fail at the end when the branch was not there in the first place branchExists, err := c.Catalog.BranchExists(ctx, repository, branch) @@ -211,7 +211,7 @@ func (c *Controller) CreatePresignMultipartUpload(w http.ResponseWriter, r *http return } - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, address, block.IdentifierTypeRelative) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, address, block.IdentifierTypeRelative) if err != nil { writeError(w, r, http.StatusInternalServerError, err) return @@ -267,13 +267,8 @@ func (c *Controller) AbortPresignMultipartUpload(w http.ResponseWriter, r *http. ctx := r.Context() c.LogAction(ctx, "abort_presign_multipart_upload", r, repository, branch, "") - repo, err := c.Catalog.GetRepository(ctx, repository) - if c.handleAPIError(ctx, w, r, err) { - return - } - // check if api is supported - storageConfig := c.getStorageConfig(repo.StorageID) + storageConfig := c.getStorageConfig() if !swag.BoolValue(storageConfig.PreSignMultipartUpload) { writeError(w, r, http.StatusNotImplemented, "presign multipart upload API is not supported") return @@ -293,6 +288,11 @@ func (c *Controller) AbortPresignMultipartUpload(w http.ResponseWriter, r *http. return } + repo, err := c.Catalog.GetRepository(ctx, repository) + if c.handleAPIError(ctx, w, r, err) { + return + } + // verify physical address physicalAddress, addressType := normalizePhysicalAddress(repo.StorageNamespace, body.PhysicalAddress) if addressType != catalog.AddressTypeRelative { @@ -327,14 +327,8 @@ func (c *Controller) CompletePresignMultipartUpload(w http.ResponseWriter, r *ht ctx := r.Context() c.LogAction(ctx, "complete_presign_multipart_upload", r, repository, branch, "") - // verify physical address - repo, err := c.Catalog.GetRepository(ctx, repository) - if c.handleAPIError(ctx, w, r, err) { - return - } - // check if api is supported - storageConfig := c.getStorageConfig(repo.StorageID) + storageConfig := c.getStorageConfig() if !swag.BoolValue(storageConfig.PreSignMultipartUpload) { writeError(w, r, http.StatusNotImplemented, "presign multipart upload API is not supported") return @@ -358,6 +352,12 @@ func (c *Controller) CompletePresignMultipartUpload(w http.ResponseWriter, r *ht return } + // verify physical address + repo, err := c.Catalog.GetRepository(ctx, repository) + if c.handleAPIError(ctx, w, r, err) { + return + } + physicalAddress, addressType := normalizePhysicalAddress(repo.StorageNamespace, body.PhysicalAddress) if addressType != catalog.AddressTypeRelative { writeError(w, r, http.StatusBadRequest, "physical address must be relative to the storage namespace") @@ -696,7 +696,7 @@ func (c *Controller) GetPhysicalAddress(w http.ResponseWriter, r *http.Request, return } - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, address, block.IdentifierTypeRelative) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, address, block.IdentifierTypeRelative) if err != nil { writeError(w, r, http.StatusInternalServerError, err) return @@ -750,7 +750,7 @@ func (c *Controller) LinkPhysicalAddress(w http.ResponseWriter, r *http.Request, return } // write metadata - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, params.Path, block.IdentifierTypeRelative) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, params.Path, block.IdentifierTypeRelative) if err != nil { writeError(w, r, http.StatusInternalServerError, err) return @@ -765,7 +765,7 @@ func (c *Controller) LinkPhysicalAddress(w http.ResponseWriter, r *http.Request, ifAbsent = true } - blockStoreType := c.BlockAdapter.BlockstoreType(repo.StorageID) + blockStoreType := c.BlockAdapter.BlockstoreType() expectedType := qk.GetStorageType().BlockstoreType() if expectedType != blockStoreType { c.Logger.WithContext(ctx).WithFields(logging.Fields{ @@ -1851,8 +1851,7 @@ func (c *Controller) GetConfig(w http.ResponseWriter, r *http.Request) { } } - // TODO (gilo): is StorageID relevant here? - storageCfg := c.getStorageConfig("") + storageCfg := c.getStorageConfig() // TODO (niro): Needs to be populated storageListCfg := apigen.StorageConfigList{} versionConfig := c.getVersionConfig() @@ -1869,12 +1868,11 @@ func (c *Controller) GetStorageConfig(w http.ResponseWriter, r *http.Request) { return } - // TODO (gilo): is StorageID relevant here? - writeResponse(w, r, http.StatusOK, c.getStorageConfig("")) + writeResponse(w, r, http.StatusOK, c.getStorageConfig()) } -func (c *Controller) getStorageConfig(storageID string) apigen.StorageConfig { - info := c.BlockAdapter.GetStorageNamespaceInfo(storageID) +func (c *Controller) getStorageConfig() apigen.StorageConfig { + info := c.BlockAdapter.GetStorageNamespaceInfo() defaultNamespacePrefix := swag.String(info.DefaultNamespacePrefix) if c.Config.GetBaseConfig().Blockstore.DefaultNamespacePrefix != nil { defaultNamespacePrefix = c.Config.GetBaseConfig().Blockstore.DefaultNamespacePrefix @@ -1973,7 +1971,7 @@ func (c *Controller) CreateRepository(w http.ResponseWriter, r *http.Request, bo c.LogAction(ctx, "repo_sample_data", r, body.Name, "", "") } - if err := c.validateStorageNamespace(storageID, storageNamespace); err != nil { + if err := c.validateStorageNamespace(storageNamespace); err != nil { writeError(w, r, http.StatusBadRequest, err) return } @@ -2002,7 +2000,7 @@ func (c *Controller) CreateRepository(w http.ResponseWriter, r *http.Request, bo retErr = err reason = "bad_url" case errors.Is(err, block.ErrInvalidAddress): - retErr = fmt.Errorf("%w, must match: %s", err, c.BlockAdapter.BlockstoreType(storageID)) + retErr = fmt.Errorf("%w, must match: %s", err, c.BlockAdapter.BlockstoreType()) reason = "invalid_namespace" case errors.Is(err, ErrStorageNamespaceInUse): retErr = err @@ -2077,8 +2075,8 @@ func (c *Controller) CreateRepository(w http.ResponseWriter, r *http.Request, bo writeResponse(w, r, http.StatusCreated, response) } -func (c *Controller) validateStorageNamespace(storageID, storageNamespace string) error { - validRegex := c.BlockAdapter.GetStorageNamespaceInfo(storageID).ValidityRegex +func (c *Controller) validateStorageNamespace(storageNamespace string) error { + validRegex := c.BlockAdapter.GetStorageNamespaceInfo().ValidityRegex storagePrefixRegex, err := regexp.Compile(validRegex) if err != nil { return fmt.Errorf("failed to compile validity regex %s: %w", validRegex, block.ErrInvalidNamespace) @@ -3314,7 +3312,7 @@ func (c *Controller) UploadObject(w http.ResponseWriter, r *http.Request, reposi identifierType = block.IdentifierTypeRelative } - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, blob.PhysicalAddress, identifierType) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, blob.PhysicalAddress, identifierType) if err != nil { writeError(w, r, http.StatusInternalServerError, err) return @@ -3350,16 +3348,16 @@ func (c *Controller) StageObject(w http.ResponseWriter, r *http.Request, body ap return } // write metadata - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, body.PhysicalAddress, block.IdentifierTypeFull) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, body.PhysicalAddress, block.IdentifierTypeFull) if c.handleAPIError(ctx, w, r, err) { return } // see what storage type this is and whether it fits our configuration - uriRegex := c.BlockAdapter.GetStorageNamespaceInfo(repo.StorageID).ValidityRegex + uriRegex := c.BlockAdapter.GetStorageNamespaceInfo().ValidityRegex if match, err := regexp.MatchString(uriRegex, body.PhysicalAddress); err != nil || !match { writeError(w, r, http.StatusBadRequest, fmt.Sprintf("physical address is not valid for block adapter: %s", - c.BlockAdapter.BlockstoreType(repo.StorageID), + c.BlockAdapter.BlockstoreType(), )) return } @@ -3455,7 +3453,7 @@ func (c *Controller) CopyObject(w http.ResponseWriter, r *http.Request, body api return } - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, entry.PhysicalAddress, block.IdentifierTypeRelative) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, entry.PhysicalAddress, block.IdentifierTypeRelative) if err != nil { writeError(w, r, http.StatusInternalServerError, err) return @@ -4179,7 +4177,7 @@ func (c *Controller) CreateSymlinkFile(w http.ResponseWriter, r *http.Request, r } // loop all entries enter to map[path] physicalAddress for _, entry := range entries { - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, entry.PhysicalAddress, entry.AddressType.ToIdentifierType()) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, entry.PhysicalAddress, entry.AddressType.ToIdentifierType()) if err != nil { writeError(w, r, http.StatusInternalServerError, fmt.Sprintf("error while resolving address: %s", err)) return @@ -4612,7 +4610,7 @@ func (c *Controller) ListObjects(w http.ResponseWriter, r *http.Request, reposit objList := make([]apigen.ObjectStats, 0, len(res)) for _, entry := range res { - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, entry.PhysicalAddress, entry.AddressType.ToIdentifierType()) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, entry.PhysicalAddress, entry.AddressType.ToIdentifierType()) if err != nil { writeError(w, r, http.StatusInternalServerError, err) return @@ -4710,7 +4708,7 @@ func (c *Controller) StatObject(w http.ResponseWriter, r *http.Request, reposito return } - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, entry.PhysicalAddress, entry.AddressType.ToIdentifierType()) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, entry.PhysicalAddress, entry.AddressType.ToIdentifierType()) if c.handleAPIError(ctx, w, r, err) { return } @@ -5093,8 +5091,7 @@ func (c *Controller) Setup(w http.ResponseWriter, r *http.Request, body apigen.S return } - // TODO (gilo): which storageID should we use here? - meta := stats.NewMetadata(ctx, c.Logger, c.BlockAdapter.BlockstoreType(""), c.MetadataManager, c.CloudMetadataProvider) + meta := stats.NewMetadata(ctx, c.Logger, c.BlockAdapter.BlockstoreType(), c.MetadataManager, c.CloudMetadataProvider) c.Collector.SetInstallationID(meta.InstallationID) c.Collector.CollectMetadata(meta) c.Collector.CollectEvent(stats.Event{Class: "global", Name: "init", UserID: body.Username, Client: httputil.GetRequestLakeFSClient(r)}) diff --git a/pkg/block/adapter.go b/pkg/block/adapter.go index ba7a5ac26f2..7f6db15b180 100644 --- a/pkg/block/adapter.go +++ b/pkg/block/adapter.go @@ -194,10 +194,10 @@ type Adapter interface { AbortMultiPartUpload(ctx context.Context, obj ObjectPointer, uploadID string) error CompleteMultiPartUpload(ctx context.Context, obj ObjectPointer, uploadID string, multipartList *MultipartUploadCompletion) (*CompleteMultiPartUploadResponse, error) - BlockstoreType(storageID string) string - BlockstoreMetadata(ctx context.Context, storageID string) (*BlockstoreMetadata, error) - GetStorageNamespaceInfo(storageID string) StorageNamespaceInfo - ResolveNamespace(storageID, storageNamespace, key string, identifierType IdentifierType) (QualifiedKey, error) + BlockstoreType() string + BlockstoreMetadata(ctx context.Context) (*BlockstoreMetadata, error) + GetStorageNamespaceInfo() StorageNamespaceInfo + ResolveNamespace(storageNamespace, key string, identifierType IdentifierType) (QualifiedKey, error) // GetRegion storageID is not actively used, and it's here mainly for completeness GetRegion(ctx context.Context, storageID, storageNamespace string) (string, error) diff --git a/pkg/block/azure/adapter.go b/pkg/block/azure/adapter.go index 123e98625a9..898b07c2bb6 100644 --- a/pkg/block/azure/adapter.go +++ b/pkg/block/azure/adapter.go @@ -583,11 +583,11 @@ func (a *Adapter) AbortMultiPartUpload(_ context.Context, _ block.ObjectPointer, return nil } -func (a *Adapter) BlockstoreType(_ string) string { +func (a *Adapter) BlockstoreType() string { return block.BlockstoreTypeAzure } -func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { +func (a *Adapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } @@ -606,7 +606,7 @@ func (a *Adapter) CompleteMultiPartUpload(ctx context.Context, obj block.ObjectP return completeMultipart(ctx, multipartList.Part, *containerURL, qualifiedKey.BlobURL) } -func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { +func (a *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeAzure) info.ImportValidityRegex = fmt.Sprintf(`^https?://[a-z0-9_-]+\.%s`, a.clientCache.params.Domain) @@ -622,7 +622,7 @@ func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { return info } -func (a *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { +func (a *Adapter) ResolveNamespace(storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { return block.DefaultResolveNamespace(storageNamespace, key, identifierType) } diff --git a/pkg/block/gs/adapter.go b/pkg/block/gs/adapter.go index 71d48d4da9b..ebfe32ad476 100644 --- a/pkg/block/gs/adapter.go +++ b/pkg/block/gs/adapter.go @@ -650,15 +650,15 @@ func (a *Adapter) Close() error { return a.client.Close() } -func (a *Adapter) BlockstoreType(_ string) string { +func (a *Adapter) BlockstoreType() string { return block.BlockstoreTypeGS } -func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { +func (a *Adapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } -func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { +func (a *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeGS) if a.disablePreSigned { info.PreSignSupport = false @@ -670,7 +670,7 @@ func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { } func (a *Adapter) extractParamsFromObj(obj block.ObjectPointer) (string, string, error) { - qk, err := a.ResolveNamespace(obj.StorageID, obj.StorageNamespace, obj.Identifier, obj.IdentifierType) + qk, err := a.ResolveNamespace(obj.StorageNamespace, obj.Identifier, obj.IdentifierType) if err != nil { return "", "", err } @@ -682,7 +682,7 @@ func (a *Adapter) extractParamsFromObj(obj block.ObjectPointer) (string, string, return bucket, key, nil } -func (a *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { +func (a *Adapter) ResolveNamespace(storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { qualifiedKey, err := block.DefaultResolveNamespace(storageNamespace, key, identifierType) if err != nil { return qualifiedKey, err diff --git a/pkg/block/local/adapter.go b/pkg/block/local/adapter.go index d9c42470b9c..f89291da51b 100644 --- a/pkg/block/local/adapter.go +++ b/pkg/block/local/adapter.go @@ -537,15 +537,15 @@ func (l *Adapter) getPartFiles(uploadID string, obj block.ObjectPointer) ([]stri return names, nil } -func (l *Adapter) BlockstoreType(_ string) string { +func (l *Adapter) BlockstoreType() string { return block.BlockstoreTypeLocal } -func (l *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { +func (l *Adapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } -func (l *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { +func (l *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeLocal) info.PreSignSupport = false info.DefaultNamespacePrefix = DefaultNamespacePrefix @@ -553,7 +553,7 @@ func (l *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { return info } -func (l *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { +func (l *Adapter) ResolveNamespace(storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { qk, err := block.DefaultResolveNamespace(storageNamespace, key, identifierType) if err != nil { return nil, err diff --git a/pkg/block/mem/adapter.go b/pkg/block/mem/adapter.go index feb76412fb4..0299856b89e 100644 --- a/pkg/block/mem/adapter.go +++ b/pkg/block/mem/adapter.go @@ -335,22 +335,22 @@ func (a *Adapter) CompleteMultiPartUpload(_ context.Context, obj block.ObjectPoi }, nil } -func (a *Adapter) BlockstoreType(_ string) string { +func (a *Adapter) BlockstoreType() string { return block.BlockstoreTypeMem } -func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { +func (a *Adapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } -func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { +func (a *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeMem) info.PreSignSupport = false info.ImportSupport = false return info } -func (a *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { +func (a *Adapter) ResolveNamespace(storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { return block.DefaultResolveNamespace(storageNamespace, key, identifierType) } diff --git a/pkg/block/metrics.go b/pkg/block/metrics.go index c7d2474a914..0c892ec2d8e 100644 --- a/pkg/block/metrics.go +++ b/pkg/block/metrics.go @@ -23,12 +23,12 @@ func (m *MetricsAdapter) InnerAdapter() Adapter { } func (m *MetricsAdapter) Put(ctx context.Context, obj ObjectPointer, sizeBytes int64, reader io.Reader, opts PutOpts) (*PutResponse, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) return m.adapter.Put(ctx, obj, sizeBytes, reader, opts) } func (m *MetricsAdapter) Get(ctx context.Context, obj ObjectPointer) (io.ReadCloser, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) return m.adapter.Get(ctx, obj) } @@ -37,94 +37,94 @@ func (m *MetricsAdapter) GetWalker(uri *url.URL) (Walker, error) { } func (m *MetricsAdapter) GetPreSignedURL(ctx context.Context, obj ObjectPointer, mode PreSignMode) (string, time.Time, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) return m.adapter.GetPreSignedURL(ctx, obj, mode) } func (m *MetricsAdapter) GetPresignUploadPartURL(ctx context.Context, obj ObjectPointer, uploadID string, partNumber int) (string, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) return m.adapter.GetPresignUploadPartURL(ctx, obj, uploadID, partNumber) } func (m *MetricsAdapter) Exists(ctx context.Context, obj ObjectPointer) (bool, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) return m.adapter.Exists(ctx, obj) } func (m *MetricsAdapter) GetRange(ctx context.Context, obj ObjectPointer, startPosition int64, endPosition int64) (io.ReadCloser, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) return m.adapter.GetRange(ctx, obj, startPosition, endPosition) } func (m *MetricsAdapter) GetProperties(ctx context.Context, obj ObjectPointer) (Properties, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) return m.adapter.GetProperties(ctx, obj) } func (m *MetricsAdapter) Remove(ctx context.Context, obj ObjectPointer) error { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) return m.adapter.Remove(ctx, obj) } func (m *MetricsAdapter) Copy(ctx context.Context, sourceObj, destinationObj ObjectPointer) error { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(sourceObj.StorageID)) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) return m.adapter.Copy(ctx, sourceObj, destinationObj) } func (m *MetricsAdapter) CreateMultiPartUpload(ctx context.Context, obj ObjectPointer, r *http.Request, opts CreateMultiPartUploadOpts) (*CreateMultiPartUploadResponse, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) return m.adapter.CreateMultiPartUpload(ctx, obj, r, opts) } func (m *MetricsAdapter) UploadPart(ctx context.Context, obj ObjectPointer, sizeBytes int64, reader io.Reader, uploadID string, partNumber int) (*UploadPartResponse, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) return m.adapter.UploadPart(ctx, obj, sizeBytes, reader, uploadID, partNumber) } func (m *MetricsAdapter) ListParts(ctx context.Context, obj ObjectPointer, uploadID string, opts ListPartsOpts) (*ListPartsResponse, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) return m.adapter.ListParts(ctx, obj, uploadID, opts) } func (m *MetricsAdapter) UploadCopyPart(ctx context.Context, sourceObj, destinationObj ObjectPointer, uploadID string, partNumber int) (*UploadPartResponse, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(sourceObj.StorageID)) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) return m.adapter.UploadCopyPart(ctx, sourceObj, destinationObj, uploadID, partNumber) } func (m *MetricsAdapter) UploadCopyPartRange(ctx context.Context, sourceObj, destinationObj ObjectPointer, uploadID string, partNumber int, startPosition, endPosition int64) (*UploadPartResponse, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(sourceObj.StorageID)) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) return m.adapter.UploadCopyPartRange(ctx, sourceObj, destinationObj, uploadID, partNumber, startPosition, endPosition) } func (m *MetricsAdapter) AbortMultiPartUpload(ctx context.Context, obj ObjectPointer, uploadID string) error { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) return m.adapter.AbortMultiPartUpload(ctx, obj, uploadID) } func (m *MetricsAdapter) CompleteMultiPartUpload(ctx context.Context, obj ObjectPointer, uploadID string, multipartList *MultipartUploadCompletion) (*CompleteMultiPartUploadResponse, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(obj.StorageID)) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) return m.adapter.CompleteMultiPartUpload(ctx, obj, uploadID, multipartList) } -func (m *MetricsAdapter) BlockstoreType(storageID string) string { - return m.adapter.BlockstoreType(storageID) +func (m *MetricsAdapter) BlockstoreType() string { + return m.adapter.BlockstoreType() } -func (m *MetricsAdapter) BlockstoreMetadata(ctx context.Context, storageID string) (*BlockstoreMetadata, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(storageID)) - return m.adapter.BlockstoreMetadata(ctx, storageID) +func (m *MetricsAdapter) BlockstoreMetadata(ctx context.Context) (*BlockstoreMetadata, error) { + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) + return m.adapter.BlockstoreMetadata(ctx) } -func (m *MetricsAdapter) GetStorageNamespaceInfo(storageID string) StorageNamespaceInfo { - return m.adapter.GetStorageNamespaceInfo(storageID) +func (m *MetricsAdapter) GetStorageNamespaceInfo() StorageNamespaceInfo { + return m.adapter.GetStorageNamespaceInfo() } -func (m *MetricsAdapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType IdentifierType) (QualifiedKey, error) { - return m.adapter.ResolveNamespace(storageID, storageNamespace, key, identifierType) +func (m *MetricsAdapter) ResolveNamespace(storageNamespace, key string, identifierType IdentifierType) (QualifiedKey, error) { + return m.adapter.ResolveNamespace(storageNamespace, key, identifierType) } func (m *MetricsAdapter) GetRegion(ctx context.Context, storageID, storageNamespace string) (string, error) { - ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType(storageID)) + ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) return m.adapter.GetRegion(ctx, storageID, storageNamespace) } diff --git a/pkg/block/s3/adapter.go b/pkg/block/s3/adapter.go index 428b794eab8..d0e96e79cb2 100644 --- a/pkg/block/s3/adapter.go +++ b/pkg/block/s3/adapter.go @@ -851,11 +851,11 @@ func (a *Adapter) ListParts(ctx context.Context, obj block.ObjectPointer, upload return &partsResp, nil } -func (a *Adapter) BlockstoreType(_ string) string { +func (a *Adapter) BlockstoreType() string { return block.BlockstoreTypeS3 } -func (a *Adapter) BlockstoreMetadata(ctx context.Context, _ string) (*block.BlockstoreMetadata, error) { +func (a *Adapter) BlockstoreMetadata(ctx context.Context) (*block.BlockstoreMetadata, error) { region, err := a.clients.GetBucketRegionDefault(ctx, "") if err != nil { return nil, err @@ -863,7 +863,7 @@ func (a *Adapter) BlockstoreMetadata(ctx context.Context, _ string) (*block.Bloc return &block.BlockstoreMetadata{Region: ®ion}, nil } -func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { +func (a *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeS3) if a.disablePreSigned { info.PreSignSupport = false @@ -888,7 +888,7 @@ func resolveNamespace(obj block.ObjectPointer) (block.CommonQualifiedKey, error) return qualifiedKey, nil } -func (a *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { +func (a *Adapter) ResolveNamespace(storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { return block.DefaultResolveNamespace(storageNamespace, key, identifierType) } @@ -945,7 +945,7 @@ func (a *Adapter) managerUpload(ctx context.Context, obj block.ObjectPointer, re } func (a *Adapter) extractParamsFromObj(obj block.ObjectPointer) (string, string, block.QualifiedKey, error) { - qk, err := a.ResolveNamespace(obj.StorageID, obj.StorageNamespace, obj.Identifier, obj.IdentifierType) + qk, err := a.ResolveNamespace(obj.StorageNamespace, obj.Identifier, obj.IdentifierType) if err != nil { return "", "", nil, err } diff --git a/pkg/block/transient/adapter.go b/pkg/block/transient/adapter.go index d3cf49864ea..b5d70e1bca9 100644 --- a/pkg/block/transient/adapter.go +++ b/pkg/block/transient/adapter.go @@ -143,15 +143,15 @@ func (a *Adapter) CompleteMultiPartUpload(context.Context, block.ObjectPointer, }, nil } -func (a *Adapter) BlockstoreType(_ string) string { +func (a *Adapter) BlockstoreType() string { return block.BlockstoreTypeTransient } -func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { +func (a *Adapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } -func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { +func (a *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeTransient) info.PreSignSupport = false info.PreSignSupportUI = false @@ -159,7 +159,7 @@ func (a *Adapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { return info } -func (a *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { +func (a *Adapter) ResolveNamespace(storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { return block.DefaultResolveNamespace(storageNamespace, key, identifierType) } diff --git a/pkg/block/validations.go b/pkg/block/validations.go index a961447cc91..84f26c2d6a3 100644 --- a/pkg/block/validations.go +++ b/pkg/block/validations.go @@ -7,7 +7,7 @@ import ( ) func ValidateInterRegionStorage(ctx context.Context, adapter Adapter, storageID, storageNamespace string) error { - blockstoreMetadata, err := adapter.BlockstoreMetadata(ctx, storageID) + blockstoreMetadata, err := adapter.BlockstoreMetadata(ctx) if errors.Is(err, ErrOperationNotSupported) { // region detection not supported for the server's blockstore, skip validation return nil diff --git a/pkg/graveler/retention/garbage_collection_manager.go b/pkg/graveler/retention/garbage_collection_manager.go index 841b7a337d6..de7580f54d1 100644 --- a/pkg/graveler/retention/garbage_collection_manager.go +++ b/pkg/graveler/retention/garbage_collection_manager.go @@ -36,8 +36,7 @@ type GarbageCollectionManager struct { func (m *GarbageCollectionManager) GetCommitsCSVLocation(runID string, sn graveler.StorageNamespace) (string, error) { key := fmt.Sprintf(commitsFileSuffixTemplate, m.committedBlockStoragePrefix, runID) - // TODO (gilo): replace StorageID with a real value - qk, err := m.blockAdapter.ResolveNamespace("", sn.String(), key, block.IdentifierTypeRelative) + qk, err := m.blockAdapter.ResolveNamespace(sn.String(), key, block.IdentifierTypeRelative) if err != nil { return "", err } @@ -46,8 +45,7 @@ func (m *GarbageCollectionManager) GetCommitsCSVLocation(runID string, sn gravel func (m *GarbageCollectionManager) GetAddressesLocation(sn graveler.StorageNamespace) (string, error) { key := fmt.Sprintf(addressesFilePrefixTemplate, m.committedBlockStoragePrefix) - // TODO (gilo): replace StorageID with a real value - qk, err := m.blockAdapter.ResolveNamespace("", sn.String(), key, block.IdentifierTypeRelative) + qk, err := m.blockAdapter.ResolveNamespace(sn.String(), key, block.IdentifierTypeRelative) if err != nil { return "", err } @@ -57,8 +55,7 @@ func (m *GarbageCollectionManager) GetAddressesLocation(sn graveler.StorageNames // GetUncommittedLocation return full path to underlying storage path to store uncommitted information func (m *GarbageCollectionManager) GetUncommittedLocation(runID string, sn graveler.StorageNamespace) (string, error) { key := fmt.Sprintf(uncommittedFilePrefixTemplate, m.committedBlockStoragePrefix, runID) - // TODO (gilo): replace StorageID with a real value - qk, err := m.blockAdapter.ResolveNamespace("", sn.String(), key, block.IdentifierTypeRelative) + qk, err := m.blockAdapter.ResolveNamespace(sn.String(), key, block.IdentifierTypeRelative) if err != nil { return "", err } diff --git a/pkg/testutil/adapter.go b/pkg/testutil/adapter.go index 08d87c70dd0..442ab22193b 100644 --- a/pkg/testutil/adapter.go +++ b/pkg/testutil/adapter.go @@ -129,11 +129,11 @@ func (a *MockAdapter) ListParts(_ context.Context, _ block.ObjectPointer, _ stri panic("try to list parts in mock adapter") } -func (a *MockAdapter) BlockstoreType(_ string) string { +func (a *MockAdapter) BlockstoreType() string { return "s3" } -func (a *MockAdapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { +func (a *MockAdapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { if a.blockstoreMetadata != nil { return a.blockstoreMetadata, nil } else { @@ -141,14 +141,14 @@ func (a *MockAdapter) BlockstoreMetadata(_ context.Context, _ string) (*block.Bl } } -func (a *MockAdapter) GetStorageNamespaceInfo(_ string) block.StorageNamespaceInfo { +func (a *MockAdapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { info := block.DefaultStorageNamespaceInfo("s3") info.PreSignSupport = false info.ImportSupport = false return info } -func (a *MockAdapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { +func (a *MockAdapter) ResolveNamespace(storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { return block.DefaultResolveNamespace(storageNamespace, key, identifierType) } From 1d6d56f366ed9a54c90f0961569546d2720761b4 Mon Sep 17 00:00:00 2001 From: Itai Gilo Date: Tue, 28 Jan 2025 11:40:02 +0200 Subject: [PATCH 11/14] Add storageID to metadata functions --- pkg/api/controller.go | 104 ++++++++++++------ pkg/block/adapter.go | 6 +- pkg/block/azure/adapter.go | 8 +- pkg/block/blocktest/adapter.go | 2 +- pkg/block/blocktest/basic_suite.go | 2 +- pkg/block/gs/adapter.go | 10 +- pkg/block/local/adapter.go | 8 +- pkg/block/local/adapter_test.go | 4 +- pkg/block/mem/adapter.go | 8 +- pkg/block/metrics.go | 12 +- pkg/block/s3/adapter.go | 10 +- pkg/block/transient/adapter.go | 8 +- pkg/block/validations.go | 2 +- .../retention/garbage_collection_manager.go | 9 +- pkg/testutil/adapter.go | 6 +- 15 files changed, 119 insertions(+), 80 deletions(-) diff --git a/pkg/api/controller.go b/pkg/api/controller.go index e42e41fe3a5..099bf177431 100644 --- a/pkg/api/controller.go +++ b/pkg/api/controller.go @@ -167,18 +167,22 @@ func (c *Controller) CreatePresignMultipartUpload(w http.ResponseWriter, r *http ctx := r.Context() c.LogAction(ctx, "create_presign_multipart_upload", r, repository, branch, "") - // check if api is supported - storageConfig := c.getStorageConfig() - if !swag.BoolValue(storageConfig.PreSignMultipartUpload) { - writeError(w, r, http.StatusNotImplemented, "presign multipart upload API is not supported") + repo, err := c.Catalog.GetRepository(ctx, repository) + if c.handleAPIError(ctx, w, r, err) { return } - repo, err := c.Catalog.GetRepository(ctx, repository) + storageConfig, err := c.getStorageConfig(repo.StorageID) if c.handleAPIError(ctx, w, r, err) { return } + // check if api is supported + if !swag.BoolValue(storageConfig.PreSignMultipartUpload) { + writeError(w, r, http.StatusNotImplemented, "presign multipart upload API is not supported") + return + } + // check if the branch exists - it is still possible for a branch to be deleted later, but we don't want to // upload to start and fail at the end when the branch was not there in the first place branchExists, err := c.Catalog.BranchExists(ctx, repository, branch) @@ -211,7 +215,7 @@ func (c *Controller) CreatePresignMultipartUpload(w http.ResponseWriter, r *http return } - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, address, block.IdentifierTypeRelative) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, address, block.IdentifierTypeRelative) if err != nil { writeError(w, r, http.StatusInternalServerError, err) return @@ -267,8 +271,17 @@ func (c *Controller) AbortPresignMultipartUpload(w http.ResponseWriter, r *http. ctx := r.Context() c.LogAction(ctx, "abort_presign_multipart_upload", r, repository, branch, "") + repo, err := c.Catalog.GetRepository(ctx, repository) + if c.handleAPIError(ctx, w, r, err) { + return + } + + storageConfig, err := c.getStorageConfig(repo.StorageID) + if c.handleAPIError(ctx, w, r, err) { + return + } + // check if api is supported - storageConfig := c.getStorageConfig() if !swag.BoolValue(storageConfig.PreSignMultipartUpload) { writeError(w, r, http.StatusNotImplemented, "presign multipart upload API is not supported") return @@ -288,11 +301,6 @@ func (c *Controller) AbortPresignMultipartUpload(w http.ResponseWriter, r *http. return } - repo, err := c.Catalog.GetRepository(ctx, repository) - if c.handleAPIError(ctx, w, r, err) { - return - } - // verify physical address physicalAddress, addressType := normalizePhysicalAddress(repo.StorageNamespace, body.PhysicalAddress) if addressType != catalog.AddressTypeRelative { @@ -327,8 +335,17 @@ func (c *Controller) CompletePresignMultipartUpload(w http.ResponseWriter, r *ht ctx := r.Context() c.LogAction(ctx, "complete_presign_multipart_upload", r, repository, branch, "") + repo, err := c.Catalog.GetRepository(ctx, repository) + if c.handleAPIError(ctx, w, r, err) { + return + } + + storageConfig, err := c.getStorageConfig(repo.StorageID) + if c.handleAPIError(ctx, w, r, err) { + return + } + // check if api is supported - storageConfig := c.getStorageConfig() if !swag.BoolValue(storageConfig.PreSignMultipartUpload) { writeError(w, r, http.StatusNotImplemented, "presign multipart upload API is not supported") return @@ -353,11 +370,6 @@ func (c *Controller) CompletePresignMultipartUpload(w http.ResponseWriter, r *ht } // verify physical address - repo, err := c.Catalog.GetRepository(ctx, repository) - if c.handleAPIError(ctx, w, r, err) { - return - } - physicalAddress, addressType := normalizePhysicalAddress(repo.StorageNamespace, body.PhysicalAddress) if addressType != catalog.AddressTypeRelative { writeError(w, r, http.StatusBadRequest, "physical address must be relative to the storage namespace") @@ -696,7 +708,7 @@ func (c *Controller) GetPhysicalAddress(w http.ResponseWriter, r *http.Request, return } - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, address, block.IdentifierTypeRelative) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, address, block.IdentifierTypeRelative) if err != nil { writeError(w, r, http.StatusInternalServerError, err) return @@ -750,7 +762,7 @@ func (c *Controller) LinkPhysicalAddress(w http.ResponseWriter, r *http.Request, return } // write metadata - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, params.Path, block.IdentifierTypeRelative) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, params.Path, block.IdentifierTypeRelative) if err != nil { writeError(w, r, http.StatusInternalServerError, err) return @@ -1851,7 +1863,11 @@ func (c *Controller) GetConfig(w http.ResponseWriter, r *http.Request) { } } - storageCfg := c.getStorageConfig() + // TODO (gilo): is StorageID relevant here? + storageCfg, err := c.getStorageConfig("") + if c.handleAPIError(r.Context(), w, r, err) { + return + } // TODO (niro): Needs to be populated storageListCfg := apigen.StorageConfigList{} versionConfig := c.getVersionConfig() @@ -1868,16 +1884,25 @@ func (c *Controller) GetStorageConfig(w http.ResponseWriter, r *http.Request) { return } - writeResponse(w, r, http.StatusOK, c.getStorageConfig()) + // TODO (gilo): is StorageID relevant here? + storageConfig, err := c.getStorageConfig("") + if c.handleAPIError(r.Context(), w, r, err) { + return + } + + writeResponse(w, r, http.StatusOK, storageConfig) } -func (c *Controller) getStorageConfig() apigen.StorageConfig { - info := c.BlockAdapter.GetStorageNamespaceInfo() +func (c *Controller) getStorageConfig(storageID string) (apigen.StorageConfig, error) { + info, err := c.BlockAdapter.GetStorageNamespaceInfo(storageID) + if err != nil { + return apigen.StorageConfig{}, err + } defaultNamespacePrefix := swag.String(info.DefaultNamespacePrefix) if c.Config.GetBaseConfig().Blockstore.DefaultNamespacePrefix != nil { defaultNamespacePrefix = c.Config.GetBaseConfig().Blockstore.DefaultNamespacePrefix } - return apigen.StorageConfig{ + storageConfig := apigen.StorageConfig{ BlockstoreType: c.Config.GetBaseConfig().Blockstore.Type, BlockstoreNamespaceValidityRegex: info.ValidityRegex, BlockstoreNamespaceExample: info.Example, @@ -1888,6 +1913,7 @@ func (c *Controller) getStorageConfig() apigen.StorageConfig { ImportValidityRegex: info.ImportValidityRegex, PreSignMultipartUpload: swag.Bool(info.PreSignSupportMultipart), } + return storageConfig, nil } func (c *Controller) HealthCheck(w http.ResponseWriter, r *http.Request) { @@ -1971,7 +1997,7 @@ func (c *Controller) CreateRepository(w http.ResponseWriter, r *http.Request, bo c.LogAction(ctx, "repo_sample_data", r, body.Name, "", "") } - if err := c.validateStorageNamespace(storageNamespace); err != nil { + if err := c.validateStorageNamespace(storageID, storageNamespace); err != nil { writeError(w, r, http.StatusBadRequest, err) return } @@ -2075,8 +2101,12 @@ func (c *Controller) CreateRepository(w http.ResponseWriter, r *http.Request, bo writeResponse(w, r, http.StatusCreated, response) } -func (c *Controller) validateStorageNamespace(storageNamespace string) error { - validRegex := c.BlockAdapter.GetStorageNamespaceInfo().ValidityRegex +func (c *Controller) validateStorageNamespace(storageID, storageNamespace string) error { + namespaceInfo, err := c.BlockAdapter.GetStorageNamespaceInfo(storageID) + if err != nil { + return fmt.Errorf("failed to get storage namespace info: %w", err) + } + validRegex := namespaceInfo.ValidityRegex storagePrefixRegex, err := regexp.Compile(validRegex) if err != nil { return fmt.Errorf("failed to compile validity regex %s: %w", validRegex, block.ErrInvalidNamespace) @@ -3312,7 +3342,7 @@ func (c *Controller) UploadObject(w http.ResponseWriter, r *http.Request, reposi identifierType = block.IdentifierTypeRelative } - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, blob.PhysicalAddress, identifierType) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, blob.PhysicalAddress, identifierType) if err != nil { writeError(w, r, http.StatusInternalServerError, err) return @@ -3348,13 +3378,17 @@ func (c *Controller) StageObject(w http.ResponseWriter, r *http.Request, body ap return } // write metadata - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, body.PhysicalAddress, block.IdentifierTypeFull) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, body.PhysicalAddress, block.IdentifierTypeFull) if c.handleAPIError(ctx, w, r, err) { return } // see what storage type this is and whether it fits our configuration - uriRegex := c.BlockAdapter.GetStorageNamespaceInfo().ValidityRegex + namespaceInfo, err := c.BlockAdapter.GetStorageNamespaceInfo(repo.StorageID) + if c.handleAPIError(ctx, w, r, err) { + return + } + uriRegex := namespaceInfo.ValidityRegex if match, err := regexp.MatchString(uriRegex, body.PhysicalAddress); err != nil || !match { writeError(w, r, http.StatusBadRequest, fmt.Sprintf("physical address is not valid for block adapter: %s", c.BlockAdapter.BlockstoreType(), @@ -3453,7 +3487,7 @@ func (c *Controller) CopyObject(w http.ResponseWriter, r *http.Request, body api return } - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, entry.PhysicalAddress, block.IdentifierTypeRelative) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, entry.PhysicalAddress, block.IdentifierTypeRelative) if err != nil { writeError(w, r, http.StatusInternalServerError, err) return @@ -4177,7 +4211,7 @@ func (c *Controller) CreateSymlinkFile(w http.ResponseWriter, r *http.Request, r } // loop all entries enter to map[path] physicalAddress for _, entry := range entries { - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, entry.PhysicalAddress, entry.AddressType.ToIdentifierType()) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, entry.PhysicalAddress, entry.AddressType.ToIdentifierType()) if err != nil { writeError(w, r, http.StatusInternalServerError, fmt.Sprintf("error while resolving address: %s", err)) return @@ -4610,7 +4644,7 @@ func (c *Controller) ListObjects(w http.ResponseWriter, r *http.Request, reposit objList := make([]apigen.ObjectStats, 0, len(res)) for _, entry := range res { - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, entry.PhysicalAddress, entry.AddressType.ToIdentifierType()) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, entry.PhysicalAddress, entry.AddressType.ToIdentifierType()) if err != nil { writeError(w, r, http.StatusInternalServerError, err) return @@ -4708,7 +4742,7 @@ func (c *Controller) StatObject(w http.ResponseWriter, r *http.Request, reposito return } - qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageNamespace, entry.PhysicalAddress, entry.AddressType.ToIdentifierType()) + qk, err := c.BlockAdapter.ResolveNamespace(repo.StorageID, repo.StorageNamespace, entry.PhysicalAddress, entry.AddressType.ToIdentifierType()) if c.handleAPIError(ctx, w, r, err) { return } diff --git a/pkg/block/adapter.go b/pkg/block/adapter.go index 7f6db15b180..03e1cd2c5b5 100644 --- a/pkg/block/adapter.go +++ b/pkg/block/adapter.go @@ -195,9 +195,9 @@ type Adapter interface { CompleteMultiPartUpload(ctx context.Context, obj ObjectPointer, uploadID string, multipartList *MultipartUploadCompletion) (*CompleteMultiPartUploadResponse, error) BlockstoreType() string - BlockstoreMetadata(ctx context.Context) (*BlockstoreMetadata, error) - GetStorageNamespaceInfo() StorageNamespaceInfo - ResolveNamespace(storageNamespace, key string, identifierType IdentifierType) (QualifiedKey, error) + BlockstoreMetadata(ctx context.Context, storageID string) (*BlockstoreMetadata, error) + GetStorageNamespaceInfo(storageID string) (StorageNamespaceInfo, error) + ResolveNamespace(storageID, storageNamespace, key string, identifierType IdentifierType) (QualifiedKey, error) // GetRegion storageID is not actively used, and it's here mainly for completeness GetRegion(ctx context.Context, storageID, storageNamespace string) (string, error) diff --git a/pkg/block/azure/adapter.go b/pkg/block/azure/adapter.go index 898b07c2bb6..06b2209f248 100644 --- a/pkg/block/azure/adapter.go +++ b/pkg/block/azure/adapter.go @@ -587,7 +587,7 @@ func (a *Adapter) BlockstoreType() string { return block.BlockstoreTypeAzure } -func (a *Adapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { +func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } @@ -606,7 +606,7 @@ func (a *Adapter) CompleteMultiPartUpload(ctx context.Context, obj block.ObjectP return completeMultipart(ctx, multipartList.Part, *containerURL, qualifiedKey.BlobURL) } -func (a *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { +func (a *Adapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, error) { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeAzure) info.ImportValidityRegex = fmt.Sprintf(`^https?://[a-z0-9_-]+\.%s`, a.clientCache.params.Domain) @@ -619,10 +619,10 @@ func (a *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { if !(a.disablePreSignedUI || a.disablePreSigned) { info.PreSignSupportUI = true } - return info + return info, nil } -func (a *Adapter) ResolveNamespace(storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { +func (a *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { return block.DefaultResolveNamespace(storageNamespace, key, identifierType) } diff --git a/pkg/block/blocktest/adapter.go b/pkg/block/blocktest/adapter.go index 5fa7f8010de..30bb74f9d98 100644 --- a/pkg/block/blocktest/adapter.go +++ b/pkg/block/blocktest/adapter.go @@ -126,7 +126,7 @@ func testAdapterWalker(t *testing.T, adapter block.Adapter, storageNamespace str }, } for _, tt := range cases { - qk, err := adapter.ResolveNamespace(storageNamespace, filepath.Join(testPrefix, tt.prefix), block.IdentifierTypeRelative) + qk, err := adapter.ResolveNamespace("", storageNamespace, filepath.Join(testPrefix, tt.prefix), block.IdentifierTypeRelative) require.NoError(t, err) uri, err := url.Parse(qk.Format()) require.NoError(t, err) diff --git a/pkg/block/blocktest/basic_suite.go b/pkg/block/blocktest/basic_suite.go index f96d5efb98a..9e8e85c9dbc 100644 --- a/pkg/block/blocktest/basic_suite.go +++ b/pkg/block/blocktest/basic_suite.go @@ -153,7 +153,7 @@ func testAdapterRemove(t *testing.T, adapter block.Adapter, storageNamespace str t.Errorf("Remove() error = %v, wantErr %v", err, tt.wantErr) } - qk, err := adapter.ResolveNamespace(storageNamespace, tt.name, block.IdentifierTypeRelative) + qk, err := adapter.ResolveNamespace("", storageNamespace, tt.name, block.IdentifierTypeRelative) require.NoError(t, err) tree := dumpPathTree(t, ctx, adapter, qk) diff --git a/pkg/block/gs/adapter.go b/pkg/block/gs/adapter.go index ebfe32ad476..4b876047923 100644 --- a/pkg/block/gs/adapter.go +++ b/pkg/block/gs/adapter.go @@ -654,11 +654,11 @@ func (a *Adapter) BlockstoreType() string { return block.BlockstoreTypeGS } -func (a *Adapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { +func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } -func (a *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { +func (a *Adapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, error) { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeGS) if a.disablePreSigned { info.PreSignSupport = false @@ -666,11 +666,11 @@ func (a *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { if !(a.disablePreSignedUI || a.disablePreSigned) { info.PreSignSupportUI = true } - return info + return info, nil } func (a *Adapter) extractParamsFromObj(obj block.ObjectPointer) (string, string, error) { - qk, err := a.ResolveNamespace(obj.StorageNamespace, obj.Identifier, obj.IdentifierType) + qk, err := a.ResolveNamespace(obj.StorageID, obj.StorageNamespace, obj.Identifier, obj.IdentifierType) if err != nil { return "", "", err } @@ -682,7 +682,7 @@ func (a *Adapter) extractParamsFromObj(obj block.ObjectPointer) (string, string, return bucket, key, nil } -func (a *Adapter) ResolveNamespace(storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { +func (a *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { qualifiedKey, err := block.DefaultResolveNamespace(storageNamespace, key, identifierType) if err != nil { return qualifiedKey, err diff --git a/pkg/block/local/adapter.go b/pkg/block/local/adapter.go index f89291da51b..fa0f111e27c 100644 --- a/pkg/block/local/adapter.go +++ b/pkg/block/local/adapter.go @@ -541,19 +541,19 @@ func (l *Adapter) BlockstoreType() string { return block.BlockstoreTypeLocal } -func (l *Adapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { +func (l *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } -func (l *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { +func (l *Adapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, error) { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeLocal) info.PreSignSupport = false info.DefaultNamespacePrefix = DefaultNamespacePrefix info.ImportSupport = l.importEnabled - return info + return info, nil } -func (l *Adapter) ResolveNamespace(storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { +func (l *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { qk, err := block.DefaultResolveNamespace(storageNamespace, key, identifierType) if err != nil { return nil, err diff --git a/pkg/block/local/adapter_test.go b/pkg/block/local/adapter_test.go index 054b13e589c..ac78c91f2c7 100644 --- a/pkg/block/local/adapter_test.go +++ b/pkg/block/local/adapter_test.go @@ -31,7 +31,9 @@ func TestAdapterNamespace(t *testing.T) { localPath := path.Join(tmpDir, "lakefs") adapter, err := local.NewAdapter(localPath, local.WithRemoveEmptyDir(false)) require.NoError(t, err, "create new adapter") - expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo().ValidityRegex) + namespaceInfo, err := adapter.GetStorageNamespaceInfo("") + require.NoError(t, err) + expr, err := regexp.Compile(namespaceInfo.ValidityRegex) require.NoError(t, err) tests := []struct { diff --git a/pkg/block/mem/adapter.go b/pkg/block/mem/adapter.go index 0299856b89e..419d3302702 100644 --- a/pkg/block/mem/adapter.go +++ b/pkg/block/mem/adapter.go @@ -339,18 +339,18 @@ func (a *Adapter) BlockstoreType() string { return block.BlockstoreTypeMem } -func (a *Adapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { +func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } -func (a *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { +func (a *Adapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, error) { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeMem) info.PreSignSupport = false info.ImportSupport = false - return info + return info, nil } -func (a *Adapter) ResolveNamespace(storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { +func (a *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { return block.DefaultResolveNamespace(storageNamespace, key, identifierType) } diff --git a/pkg/block/metrics.go b/pkg/block/metrics.go index 0c892ec2d8e..b3e8d085be9 100644 --- a/pkg/block/metrics.go +++ b/pkg/block/metrics.go @@ -110,17 +110,17 @@ func (m *MetricsAdapter) BlockstoreType() string { return m.adapter.BlockstoreType() } -func (m *MetricsAdapter) BlockstoreMetadata(ctx context.Context) (*BlockstoreMetadata, error) { +func (m *MetricsAdapter) BlockstoreMetadata(ctx context.Context, storageID string) (*BlockstoreMetadata, error) { ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) - return m.adapter.BlockstoreMetadata(ctx) + return m.adapter.BlockstoreMetadata(ctx, storageID) } -func (m *MetricsAdapter) GetStorageNamespaceInfo() StorageNamespaceInfo { - return m.adapter.GetStorageNamespaceInfo() +func (m *MetricsAdapter) GetStorageNamespaceInfo(storageID string) (StorageNamespaceInfo, error) { + return m.adapter.GetStorageNamespaceInfo(storageID) } -func (m *MetricsAdapter) ResolveNamespace(storageNamespace, key string, identifierType IdentifierType) (QualifiedKey, error) { - return m.adapter.ResolveNamespace(storageNamespace, key, identifierType) +func (m *MetricsAdapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType IdentifierType) (QualifiedKey, error) { + return m.adapter.ResolveNamespace(storageID, storageNamespace, key, identifierType) } func (m *MetricsAdapter) GetRegion(ctx context.Context, storageID, storageNamespace string) (string, error) { diff --git a/pkg/block/s3/adapter.go b/pkg/block/s3/adapter.go index d0e96e79cb2..3cb1b70d291 100644 --- a/pkg/block/s3/adapter.go +++ b/pkg/block/s3/adapter.go @@ -855,7 +855,7 @@ func (a *Adapter) BlockstoreType() string { return block.BlockstoreTypeS3 } -func (a *Adapter) BlockstoreMetadata(ctx context.Context) (*block.BlockstoreMetadata, error) { +func (a *Adapter) BlockstoreMetadata(ctx context.Context, _ string) (*block.BlockstoreMetadata, error) { region, err := a.clients.GetBucketRegionDefault(ctx, "") if err != nil { return nil, err @@ -863,7 +863,7 @@ func (a *Adapter) BlockstoreMetadata(ctx context.Context) (*block.BlockstoreMeta return &block.BlockstoreMetadata{Region: ®ion}, nil } -func (a *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { +func (a *Adapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, error) { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeS3) if a.disablePreSigned { info.PreSignSupport = false @@ -874,7 +874,7 @@ func (a *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { if !a.disablePreSignedMultipart && info.PreSignSupport { info.PreSignSupportMultipart = true } - return info + return info, nil } func resolveNamespace(obj block.ObjectPointer) (block.CommonQualifiedKey, error) { @@ -888,7 +888,7 @@ func resolveNamespace(obj block.ObjectPointer) (block.CommonQualifiedKey, error) return qualifiedKey, nil } -func (a *Adapter) ResolveNamespace(storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { +func (a *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { return block.DefaultResolveNamespace(storageNamespace, key, identifierType) } @@ -945,7 +945,7 @@ func (a *Adapter) managerUpload(ctx context.Context, obj block.ObjectPointer, re } func (a *Adapter) extractParamsFromObj(obj block.ObjectPointer) (string, string, block.QualifiedKey, error) { - qk, err := a.ResolveNamespace(obj.StorageNamespace, obj.Identifier, obj.IdentifierType) + qk, err := a.ResolveNamespace(obj.StorageID, obj.StorageNamespace, obj.Identifier, obj.IdentifierType) if err != nil { return "", "", nil, err } diff --git a/pkg/block/transient/adapter.go b/pkg/block/transient/adapter.go index b5d70e1bca9..99d870741c3 100644 --- a/pkg/block/transient/adapter.go +++ b/pkg/block/transient/adapter.go @@ -147,19 +147,19 @@ func (a *Adapter) BlockstoreType() string { return block.BlockstoreTypeTransient } -func (a *Adapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { +func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } -func (a *Adapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { +func (a *Adapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, error) { info := block.DefaultStorageNamespaceInfo(block.BlockstoreTypeTransient) info.PreSignSupport = false info.PreSignSupportUI = false info.ImportSupport = false - return info + return info, nil } -func (a *Adapter) ResolveNamespace(storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { +func (a *Adapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { return block.DefaultResolveNamespace(storageNamespace, key, identifierType) } diff --git a/pkg/block/validations.go b/pkg/block/validations.go index 84f26c2d6a3..a961447cc91 100644 --- a/pkg/block/validations.go +++ b/pkg/block/validations.go @@ -7,7 +7,7 @@ import ( ) func ValidateInterRegionStorage(ctx context.Context, adapter Adapter, storageID, storageNamespace string) error { - blockstoreMetadata, err := adapter.BlockstoreMetadata(ctx) + blockstoreMetadata, err := adapter.BlockstoreMetadata(ctx, storageID) if errors.Is(err, ErrOperationNotSupported) { // region detection not supported for the server's blockstore, skip validation return nil diff --git a/pkg/graveler/retention/garbage_collection_manager.go b/pkg/graveler/retention/garbage_collection_manager.go index de7580f54d1..841b7a337d6 100644 --- a/pkg/graveler/retention/garbage_collection_manager.go +++ b/pkg/graveler/retention/garbage_collection_manager.go @@ -36,7 +36,8 @@ type GarbageCollectionManager struct { func (m *GarbageCollectionManager) GetCommitsCSVLocation(runID string, sn graveler.StorageNamespace) (string, error) { key := fmt.Sprintf(commitsFileSuffixTemplate, m.committedBlockStoragePrefix, runID) - qk, err := m.blockAdapter.ResolveNamespace(sn.String(), key, block.IdentifierTypeRelative) + // TODO (gilo): replace StorageID with a real value + qk, err := m.blockAdapter.ResolveNamespace("", sn.String(), key, block.IdentifierTypeRelative) if err != nil { return "", err } @@ -45,7 +46,8 @@ func (m *GarbageCollectionManager) GetCommitsCSVLocation(runID string, sn gravel func (m *GarbageCollectionManager) GetAddressesLocation(sn graveler.StorageNamespace) (string, error) { key := fmt.Sprintf(addressesFilePrefixTemplate, m.committedBlockStoragePrefix) - qk, err := m.blockAdapter.ResolveNamespace(sn.String(), key, block.IdentifierTypeRelative) + // TODO (gilo): replace StorageID with a real value + qk, err := m.blockAdapter.ResolveNamespace("", sn.String(), key, block.IdentifierTypeRelative) if err != nil { return "", err } @@ -55,7 +57,8 @@ func (m *GarbageCollectionManager) GetAddressesLocation(sn graveler.StorageNames // GetUncommittedLocation return full path to underlying storage path to store uncommitted information func (m *GarbageCollectionManager) GetUncommittedLocation(runID string, sn graveler.StorageNamespace) (string, error) { key := fmt.Sprintf(uncommittedFilePrefixTemplate, m.committedBlockStoragePrefix, runID) - qk, err := m.blockAdapter.ResolveNamespace(sn.String(), key, block.IdentifierTypeRelative) + // TODO (gilo): replace StorageID with a real value + qk, err := m.blockAdapter.ResolveNamespace("", sn.String(), key, block.IdentifierTypeRelative) if err != nil { return "", err } diff --git a/pkg/testutil/adapter.go b/pkg/testutil/adapter.go index 442ab22193b..8e130e75707 100644 --- a/pkg/testutil/adapter.go +++ b/pkg/testutil/adapter.go @@ -133,7 +133,7 @@ func (a *MockAdapter) BlockstoreType() string { return "s3" } -func (a *MockAdapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { +func (a *MockAdapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { if a.blockstoreMetadata != nil { return a.blockstoreMetadata, nil } else { @@ -141,14 +141,14 @@ func (a *MockAdapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMe } } -func (a *MockAdapter) GetStorageNamespaceInfo() block.StorageNamespaceInfo { +func (a *MockAdapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceInfo, error) { info := block.DefaultStorageNamespaceInfo("s3") info.PreSignSupport = false info.ImportSupport = false return info } -func (a *MockAdapter) ResolveNamespace(storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { +func (a *MockAdapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { return block.DefaultResolveNamespace(storageNamespace, key, identifierType) } From eee05b3f585273e1b9de261a507e7384c071a385 Mon Sep 17 00:00:00 2001 From: Itai Gilo Date: Tue, 28 Jan 2025 11:53:36 +0200 Subject: [PATCH 12/14] Fix tests --- pkg/testutil/adapter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/testutil/adapter.go b/pkg/testutil/adapter.go index 8e130e75707..223df385578 100644 --- a/pkg/testutil/adapter.go +++ b/pkg/testutil/adapter.go @@ -145,7 +145,7 @@ func (a *MockAdapter) GetStorageNamespaceInfo(_ string) (block.StorageNamespaceI info := block.DefaultStorageNamespaceInfo("s3") info.PreSignSupport = false info.ImportSupport = false - return info + return info, nil } func (a *MockAdapter) ResolveNamespace(storageID, storageNamespace, key string, identifierType block.IdentifierType) (block.QualifiedKey, error) { From 9be47652e938393a6180c9eb369f35d1c2ad9aa1 Mon Sep 17 00:00:00 2001 From: Itai Gilo Date: Tue, 28 Jan 2025 13:25:23 +0200 Subject: [PATCH 13/14] Fix tests --- pkg/block/azure/adapter_test.go | 2 +- pkg/block/gs/adapter_test.go | 3 ++- pkg/block/s3/adapter_test.go | 3 ++- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/pkg/block/azure/adapter_test.go b/pkg/block/azure/adapter_test.go index 5b13deb0357..a4d6a3babd0 100644 --- a/pkg/block/azure/adapter_test.go +++ b/pkg/block/azure/adapter_test.go @@ -135,7 +135,7 @@ func TestAdapterNamespace(t *testing.T) { } require.NoError(t, err, "create new adapter") - namespaceInfo := adapter.GetStorageNamespaceInfo() + namespaceInfo, _ := adapter.GetStorageNamespaceInfo("") expr, err := regexp.Compile(namespaceInfo.ValidityRegex) require.NoError(t, err) diff --git a/pkg/block/gs/adapter_test.go b/pkg/block/gs/adapter_test.go index bea1bd39146..c6bed26be20 100644 --- a/pkg/block/gs/adapter_test.go +++ b/pkg/block/gs/adapter_test.go @@ -36,7 +36,8 @@ func TestAdapterNamespace(t *testing.T) { require.NoError(t, adapter.Close()) }() - expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo().ValidityRegex) + namespaceInfo, _ := adapter.GetStorageNamespaceInfo("") + expr, err := regexp.Compile(namespaceInfo.ValidityRegex) require.NoError(t, err) tests := []struct { diff --git a/pkg/block/s3/adapter_test.go b/pkg/block/s3/adapter_test.go index 90dde42c06d..905f6e6b038 100644 --- a/pkg/block/s3/adapter_test.go +++ b/pkg/block/s3/adapter_test.go @@ -66,7 +66,8 @@ func TestS3AdapterPresignedOverride(t *testing.T) { // TestAdapterNamespace tests the namespace validity regex with various paths func TestAdapterNamespace(t *testing.T) { adapter := getS3BlockAdapter(t, nil) - expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo().ValidityRegex) + namespaceInfo, _ := adapter.GetStorageNamespaceInfo("") + expr, err := regexp.Compile(namespaceInfo.ValidityRegex) require.NoError(t, err) tests := []struct { From 82e261d1b2e6a9ef4919507ce4097be61cfe7bce Mon Sep 17 00:00:00 2001 From: Itai Gilo Date: Wed, 29 Jan 2025 19:38:57 +0200 Subject: [PATCH 14/14] Remove storageID from BlockstoreMetadata --- pkg/block/adapter.go | 2 +- pkg/block/azure/adapter.go | 2 +- pkg/block/gs/adapter.go | 2 +- pkg/block/local/adapter.go | 2 +- pkg/block/mem/adapter.go | 2 +- pkg/block/metrics.go | 4 ++-- pkg/block/s3/adapter.go | 2 +- pkg/block/transient/adapter.go | 2 +- pkg/block/validations.go | 2 +- pkg/testutil/adapter.go | 2 +- 10 files changed, 11 insertions(+), 11 deletions(-) diff --git a/pkg/block/adapter.go b/pkg/block/adapter.go index 03e1cd2c5b5..fa70226f8a0 100644 --- a/pkg/block/adapter.go +++ b/pkg/block/adapter.go @@ -195,7 +195,7 @@ type Adapter interface { CompleteMultiPartUpload(ctx context.Context, obj ObjectPointer, uploadID string, multipartList *MultipartUploadCompletion) (*CompleteMultiPartUploadResponse, error) BlockstoreType() string - BlockstoreMetadata(ctx context.Context, storageID string) (*BlockstoreMetadata, error) + BlockstoreMetadata(ctx context.Context) (*BlockstoreMetadata, error) GetStorageNamespaceInfo(storageID string) (StorageNamespaceInfo, error) ResolveNamespace(storageID, storageNamespace, key string, identifierType IdentifierType) (QualifiedKey, error) diff --git a/pkg/block/azure/adapter.go b/pkg/block/azure/adapter.go index 06b2209f248..7e43989d5f6 100644 --- a/pkg/block/azure/adapter.go +++ b/pkg/block/azure/adapter.go @@ -587,7 +587,7 @@ func (a *Adapter) BlockstoreType() string { return block.BlockstoreTypeAzure } -func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { +func (a *Adapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } diff --git a/pkg/block/gs/adapter.go b/pkg/block/gs/adapter.go index 4b876047923..cfc59f9ea61 100644 --- a/pkg/block/gs/adapter.go +++ b/pkg/block/gs/adapter.go @@ -654,7 +654,7 @@ func (a *Adapter) BlockstoreType() string { return block.BlockstoreTypeGS } -func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { +func (a *Adapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } diff --git a/pkg/block/local/adapter.go b/pkg/block/local/adapter.go index fa0f111e27c..d4aaeeda80a 100644 --- a/pkg/block/local/adapter.go +++ b/pkg/block/local/adapter.go @@ -541,7 +541,7 @@ func (l *Adapter) BlockstoreType() string { return block.BlockstoreTypeLocal } -func (l *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { +func (l *Adapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } diff --git a/pkg/block/mem/adapter.go b/pkg/block/mem/adapter.go index 419d3302702..022627ca425 100644 --- a/pkg/block/mem/adapter.go +++ b/pkg/block/mem/adapter.go @@ -339,7 +339,7 @@ func (a *Adapter) BlockstoreType() string { return block.BlockstoreTypeMem } -func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { +func (a *Adapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } diff --git a/pkg/block/metrics.go b/pkg/block/metrics.go index b3e8d085be9..75eae719f3b 100644 --- a/pkg/block/metrics.go +++ b/pkg/block/metrics.go @@ -110,9 +110,9 @@ func (m *MetricsAdapter) BlockstoreType() string { return m.adapter.BlockstoreType() } -func (m *MetricsAdapter) BlockstoreMetadata(ctx context.Context, storageID string) (*BlockstoreMetadata, error) { +func (m *MetricsAdapter) BlockstoreMetadata(ctx context.Context) (*BlockstoreMetadata, error) { ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType()) - return m.adapter.BlockstoreMetadata(ctx, storageID) + return m.adapter.BlockstoreMetadata(ctx) } func (m *MetricsAdapter) GetStorageNamespaceInfo(storageID string) (StorageNamespaceInfo, error) { diff --git a/pkg/block/s3/adapter.go b/pkg/block/s3/adapter.go index 3cb1b70d291..c28c1322e46 100644 --- a/pkg/block/s3/adapter.go +++ b/pkg/block/s3/adapter.go @@ -855,7 +855,7 @@ func (a *Adapter) BlockstoreType() string { return block.BlockstoreTypeS3 } -func (a *Adapter) BlockstoreMetadata(ctx context.Context, _ string) (*block.BlockstoreMetadata, error) { +func (a *Adapter) BlockstoreMetadata(ctx context.Context) (*block.BlockstoreMetadata, error) { region, err := a.clients.GetBucketRegionDefault(ctx, "") if err != nil { return nil, err diff --git a/pkg/block/transient/adapter.go b/pkg/block/transient/adapter.go index 99d870741c3..1b349dcf2d6 100644 --- a/pkg/block/transient/adapter.go +++ b/pkg/block/transient/adapter.go @@ -147,7 +147,7 @@ func (a *Adapter) BlockstoreType() string { return block.BlockstoreTypeTransient } -func (a *Adapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { +func (a *Adapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { return nil, block.ErrOperationNotSupported } diff --git a/pkg/block/validations.go b/pkg/block/validations.go index a961447cc91..84f26c2d6a3 100644 --- a/pkg/block/validations.go +++ b/pkg/block/validations.go @@ -7,7 +7,7 @@ import ( ) func ValidateInterRegionStorage(ctx context.Context, adapter Adapter, storageID, storageNamespace string) error { - blockstoreMetadata, err := adapter.BlockstoreMetadata(ctx, storageID) + blockstoreMetadata, err := adapter.BlockstoreMetadata(ctx) if errors.Is(err, ErrOperationNotSupported) { // region detection not supported for the server's blockstore, skip validation return nil diff --git a/pkg/testutil/adapter.go b/pkg/testutil/adapter.go index 223df385578..ebd7b19eb69 100644 --- a/pkg/testutil/adapter.go +++ b/pkg/testutil/adapter.go @@ -133,7 +133,7 @@ func (a *MockAdapter) BlockstoreType() string { return "s3" } -func (a *MockAdapter) BlockstoreMetadata(_ context.Context, _ string) (*block.BlockstoreMetadata, error) { +func (a *MockAdapter) BlockstoreMetadata(_ context.Context) (*block.BlockstoreMetadata, error) { if a.blockstoreMetadata != nil { return a.blockstoreMetadata, nil } else {