ObjectClient List refactoring (#3238)

* Make delimeter parameter to List method.

Signed-off-by: Peter Štibraný <[email protected]>

* Make FSObjectClient support recursive List and delimiter parameter.

Signed-off-by: Peter Štibraný <[email protected]>

* Fix errors and lint issues.

Signed-off-by: Peter Štibraný <[email protected]>

* Allow listing non-existant prefix, or single file.

Signed-off-by: Peter Štibraný <[email protected]>

* Review feedback.

Signed-off-by: Peter Štibraný <[email protected]>

* Fixed formatting error. 🤦

Signed-off-by: Peter Štibraný <[email protected]>

* Added comment to List method.

Signed-off-by: Peter Štibraný <[email protected]>

Author: pstibrany

integration/s3_storage_client_test.go

@@ -67,7 +67,7 @@ func TestS3Client(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			client, err := s3.NewS3ObjectClient(tt.cfg, "/")
+			client, err := s3.NewS3ObjectClient(tt.cfg)
 
 			require.NoError(t, err)
 

pkg/alertmanager/alerts/objectclient/store.go

@@ -33,7 +33,7 @@ func NewAlertStore(client chunk.ObjectClient) *AlertStore {
 
 // ListAlertConfigs returns all of the active alert configs in this store
 func (a *AlertStore) ListAlertConfigs(ctx context.Context) (map[string]alerts.AlertConfigDesc, error) {
-	objs, _, err := a.client.List(ctx, alertPrefix)
+	objs, _, err := a.client.List(ctx, alertPrefix, "")
 	if err != nil {
 		return nil, err
 	}

pkg/alertmanager/storage.go

@@ -55,9 +55,9 @@ func NewAlertStore(cfg AlertStoreConfig) (AlertStore, error) {
 	case "local":
 		return local.NewStore(cfg.Local)
 	case "gcs":
-		return newObjAlertStore(gcp.NewGCSObjectClient(context.Background(), cfg.GCS, ""))
+		return newObjAlertStore(gcp.NewGCSObjectClient(context.Background(), cfg.GCS))
 	case "s3":
-		return newObjAlertStore(aws.NewS3ObjectClient(cfg.S3, ""))
+		return newObjAlertStore(aws.NewS3ObjectClient(cfg.S3))
 	default:
 		return nil, fmt.Errorf("unrecognized alertmanager storage backend %v, choose one of: azure, configdb, gcs, local, s3", cfg.Type)
 	}

pkg/chunk/aws/fixtures.go

@@ -44,10 +44,7 @@ var Fixtures = []testutils.Fixture{
 				schemaCfg:               schemaConfig,
 				metrics:                 newMetrics(nil),
 			}
-			object := objectclient.NewClient(&S3ObjectClient{
-				S3:        newMockS3(),
-				delimiter: chunk.DirDelim,
-			}, nil)
+			object := objectclient.NewClient(&S3ObjectClient{S3: newMockS3()}, nil)
 			return index, object, table, schemaConfig, testutils.CloserFunc(func() error {
 				table.Stop()
 				index.Stop()

pkg/chunk/aws/s3_storage_client.go

@@ -94,12 +94,11 @@ func (cfg *S3Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
 type S3ObjectClient struct {
 	bucketNames   []string
 	S3            s3iface.S3API
-	delimiter     string
 	sseEncryption *string
 }
 
 // NewS3ObjectClient makes a new S3-backed ObjectClient.
-func NewS3ObjectClient(cfg S3Config, delimiter string) (*S3ObjectClient, error) {
+func NewS3ObjectClient(cfg S3Config) (*S3ObjectClient, error) {
 	s3Config, bucketNames, err := buildS3Config(cfg)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to build s3 config")
@@ -120,7 +119,6 @@ func NewS3ObjectClient(cfg S3Config, delimiter string) (*S3ObjectClient, error)
 	client := S3ObjectClient{
 		S3:            s3Client,
 		bucketNames:   bucketNames,
-		delimiter:     delimiter,
 		sseEncryption: sseEncryption,
 	}
 	return &client, nil
@@ -289,8 +287,8 @@ func (a *S3ObjectClient) PutObject(ctx context.Context, objectKey string, object
 	})
 }
 
-// List objects and common-prefixes i.e synthetic directories from the store non-recursively
-func (a *S3ObjectClient) List(ctx context.Context, prefix string) ([]chunk.StorageObject, []chunk.StorageCommonPrefix, error) {
+// List implements chunk.ObjectClient.
+func (a *S3ObjectClient) List(ctx context.Context, prefix, delimiter string) ([]chunk.StorageObject, []chunk.StorageCommonPrefix, error) {
 	var storageObjects []chunk.StorageObject
 	var commonPrefixes []chunk.StorageCommonPrefix
 
@@ -299,7 +297,7 @@ func (a *S3ObjectClient) List(ctx context.Context, prefix string) ([]chunk.Stora
 			input := s3.ListObjectsV2Input{
 				Bucket:    aws.String(a.bucketNames[i]),
 				Prefix:    aws.String(prefix),
-				Delimiter: aws.String(a.delimiter),
+				Delimiter: aws.String(delimiter),
 			}
 
 			for {
@@ -337,7 +335,3 @@ func (a *S3ObjectClient) List(ctx context.Context, prefix string) ([]chunk.Stora
 
 	return storageObjects, commonPrefixes, nil
 }
-
-func (a *S3ObjectClient) PathSeparator() string {
-	return a.delimiter
-}

pkg/chunk/aws/s3_storage_client_test.go

@@ -59,7 +59,7 @@ func TestRequestMiddleware(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			cfg.Inject = tt.fn
-			client, err := NewS3ObjectClient(cfg, "/")
+			client, err := NewS3ObjectClient(cfg)
 			require.NoError(t, err)
 
 			readCloser, err := client.GetObject(context.Background(), "key")

pkg/chunk/azure/blob_storage_client.go

@@ -88,15 +88,13 @@ type BlobStorage struct {
 	//blobService storage.Serv
 	cfg          *BlobStorageConfig
 	containerURL azblob.ContainerURL
-	delimiter    string
 }
 
 // NewBlobStorage creates a new instance of the BlobStorage struct.
-func NewBlobStorage(cfg *BlobStorageConfig, delimiter string) (*BlobStorage, error) {
+func NewBlobStorage(cfg *BlobStorageConfig) (*BlobStorage, error) {
 	util.WarnExperimentalUse("Azure Blob Storage")
 	blobStorage := &BlobStorage{
-		cfg:       cfg,
-		delimiter: delimiter,
+		cfg: cfg,
 	}
 
 	var err error
@@ -196,8 +194,8 @@ func (b *BlobStorage) newPipeline() (pipeline.Pipeline, error) {
 	}), nil
 }
 
-// List objects and common-prefixes i.e synthetic directories from the store non-recursively
-func (b *BlobStorage) List(ctx context.Context, prefix string) ([]chunk.StorageObject, []chunk.StorageCommonPrefix, error) {
+// List implements chunk.ObjectClient.
+func (b *BlobStorage) List(ctx context.Context, prefix, delimiter string) ([]chunk.StorageObject, []chunk.StorageCommonPrefix, error) {
 	var storageObjects []chunk.StorageObject
 	var commonPrefixes []chunk.StorageCommonPrefix
 
@@ -206,7 +204,7 @@ func (b *BlobStorage) List(ctx context.Context, prefix string) ([]chunk.StorageO
 			return nil, nil, ctx.Err()
 		}
 
-		listBlob, err := b.containerURL.ListBlobsHierarchySegment(ctx, marker, b.delimiter, azblob.ListBlobsSegmentOptions{Prefix: prefix})
+		listBlob, err := b.containerURL.ListBlobsHierarchySegment(ctx, marker, delimiter, azblob.ListBlobsSegmentOptions{Prefix: prefix})
 		if err != nil {
 			return nil, nil, err
 		}
@@ -240,10 +238,6 @@ func (b *BlobStorage) DeleteObject(ctx context.Context, blobID string) error {
 	return err
 }
 
-func (b *BlobStorage) PathSeparator() string {
-	return b.delimiter
-}
-
 // Validate the config.
 func (c *BlobStorageConfig) Validate() error {
 	if !util.StringsContain(supportedEnvironments, c.Environment) {

pkg/chunk/gcp/fixtures.go

@@ -78,9 +78,7 @@ func (f *fixture) Clients() (
 	}
 
 	if f.gcsObjectClient {
-		cClient = objectclient.NewClient(newGCSObjectClient(GCSConfig{
-			BucketName: "chunks",
-		}, f.gcssrv.Client(), chunk.DirDelim), nil)
+		cClient = objectclient.NewClient(newGCSObjectClient(GCSConfig{BucketName: "chunks"}, f.gcssrv.Client()), nil)
 	} else {
 		cClient = newBigtableObjectClient(Config{}, schemaConfig, client)
 	}

pkg/chunk/gcp/gcs_object_client.go

@@ -13,10 +13,9 @@ import (
 )
 
 type GCSObjectClient struct {
-	cfg       GCSConfig
-	client    *storage.Client
-	bucket    *storage.BucketHandle
-	delimiter string
+	cfg    GCSConfig
+	client *storage.Client
+	bucket *storage.BucketHandle
 }
 
 // GCSConfig is config for the GCS Chunk Client.
@@ -39,7 +38,7 @@ func (cfg *GCSConfig) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
 }
 
 // NewGCSObjectClient makes a new chunk.Client that writes chunks to GCS.
-func NewGCSObjectClient(ctx context.Context, cfg GCSConfig, delimiter string) (*GCSObjectClient, error) {
+func NewGCSObjectClient(ctx context.Context, cfg GCSConfig) (*GCSObjectClient, error) {
 	option, err := gcsInstrumentation(ctx, storage.ScopeReadWrite)
 	if err != nil {
 		return nil, err
@@ -49,16 +48,15 @@ func NewGCSObjectClient(ctx context.Context, cfg GCSConfig, delimiter string) (*
 	if err != nil {
 		return nil, err
 	}
-	return newGCSObjectClient(cfg, client, delimiter), nil
+	return newGCSObjectClient(cfg, client), nil
 }
 
-func newGCSObjectClient(cfg GCSConfig, client *storage.Client, delimiter string) *GCSObjectClient {
+func newGCSObjectClient(cfg GCSConfig, client *storage.Client) *GCSObjectClient {
 	bucket := client.Bucket(cfg.BucketName)
 	return &GCSObjectClient{
-		cfg:       cfg,
-		client:    client,
-		bucket:    bucket,
-		delimiter: delimiter,
+		cfg:    cfg,
+		client: client,
+		bucket: bucket,
 	}
 }
 
@@ -107,12 +105,12 @@ func (s *GCSObjectClient) PutObject(ctx context.Context, objectKey string, objec
 	return nil
 }
 
-// List objects and common-prefixes i.e synthetic directories from the store non-recursively
-func (s *GCSObjectClient) List(ctx context.Context, prefix string) ([]chunk.StorageObject, []chunk.StorageCommonPrefix, error) {
+// List implements chunk.ObjectClient.
+func (s *GCSObjectClient) List(ctx context.Context, prefix, delimiter string) ([]chunk.StorageObject, []chunk.StorageCommonPrefix, error) {
 	var storageObjects []chunk.StorageObject
 	var commonPrefixes []chunk.StorageCommonPrefix
 
-	iter := s.bucket.Objects(ctx, &storage.Query{Prefix: prefix, Delimiter: s.delimiter})
+	iter := s.bucket.Objects(ctx, &storage.Query{Prefix: prefix, Delimiter: delimiter})
 	for {
 		if ctx.Err() != nil {
 			return nil, nil, ctx.Err()
@@ -155,7 +153,3 @@ func (s *GCSObjectClient) DeleteObject(ctx context.Context, objectKey string) er
 
 	return nil
 }
-
-func (s *GCSObjectClient) PathSeparator() string {
-	return s.delimiter
-}

pkg/chunk/inmemory_storage_client.go

@@ -425,7 +425,8 @@ func (m *MockStorage) DeleteObject(ctx context.Context, objectKey string) error
 	return nil
 }
 
-func (m *MockStorage) List(ctx context.Context, prefix string) ([]StorageObject, []StorageCommonPrefix, error) {
+// List implements chunk.ObjectClient.
+func (m *MockStorage) List(ctx context.Context, prefix, delimiter string) ([]StorageObject, []StorageCommonPrefix, error) {
 	m.mtx.RLock()
 	defer m.mtx.RUnlock()
 
@@ -442,10 +443,6 @@ func (m *MockStorage) List(ctx context.Context, prefix string) ([]StorageObject,
 	return storageObjects, []StorageCommonPrefix{}, nil
 }
 
-func (m *MockStorage) PathSeparator() string {
-	return DirDelim
-}
-
 type mockWriteBatch struct {
 	inserts []struct {
 		tableName, hashValue string