Configurable s3 storage class

[inbarpatashnik]

What this PR does

Added support in configurable s3 storage class.

Which issue(s) this PR fixes or relates to

Fixes #3438

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[CLAassistant]

All committers have signed the CLA.

[inbarpatashnik]

I haven't tried to run it against a real s3.
So I would really appreciate if you could also check it after reviewing :)

[charleskorn]

Thanks for the PR @inbarpatashnik!

[charleskorn]

Might be being overly cautious here: it's probably safest for the default value of cfg.StorageClass to be an empty string, and to not set PutUserMetadata at all if StorageClass is not set - some S3-compatible storage services may not support STANDARD.

[inbarpatashnik]

Hi! thank you so much for the review!
It sounds like a very good point - Ive tried to edit so the default value of the storage class in the config.go will be an empty string, but it seemed to fail some tests because the object storage class "" doesn't exist.
Maybe I should edit it in this file to be an empty string?
I fixed the pr according to all the comments, but I still think how to fix so solve this one.

[inbarpatashnik]

And also I'm thinking - maybe its better if it will have an actual default value that will be supported by default (If it exists?)

[charleskorn]

Rather than setting it to an empty string, I would simply not set it at all (which is what we were doing before).

[pstibrany]

The CHANGELOG has just been cut to prepare for the next Mimir release. Please rebase main and eventually move the CHANGELOG entry added / updated in this PR to the top of the CHANGELOG document. Thanks!

[charleskorn]

34a746f went too far 😄

This comment was a suggestion to only set the X-Amz-Storage-Class value if cfg.StorageClass was not an empty string.

So, I'm imagining something like this:

func newS3Config(cfg Config) (s3.Config, error) {
	sseCfg, err := cfg.SSE.BuildThanosConfig()
	if err != nil {
		return s3.Config{}, err
	}

	putUserMetadata := map[string]string{}

	if cfg.StorageClass != "" {
		putUserMetadata[awsStorageClassHeader] = cfg.StorageClass
	}

	return s3.Config{
		Bucket:          cfg.BucketName,
		Endpoint:        cfg.Endpoint,
		Region:          cfg.Region,
		AccessKey:       cfg.AccessKeyID,
		SecretKey:       cfg.SecretAccessKey.String(),
		Insecure:        cfg.Insecure,
		PutUserMetadata: putUserMetadata,
		SSEConfig:       sseCfg,
		HTTPConfig: s3.HTTPConfig{
			IdleConnTimeout:       model.Duration(cfg.HTTP.IdleConnTimeout),
			ResponseHeaderTimeout: model.Duration(cfg.HTTP.ResponseHeaderTimeout),
			InsecureSkipVerify:    cfg.HTTP.InsecureSkipVerify,
			TLSHandshakeTimeout:   model.Duration(cfg.HTTP.TLSHandshakeTimeout),
			ExpectContinueTimeout: model.Duration(cfg.HTTP.ExpectContinueTimeout),
			MaxIdleConns:          cfg.HTTP.MaxIdleConns,
			MaxIdleConnsPerHost:   cfg.HTTP.MaxIdleConnsPerHost,
			MaxConnsPerHost:       cfg.HTTP.MaxConnsPerHost,
			Transport:             cfg.HTTP.Transport,
		},
		// Enforce signature version 2 if CLI flag is set
		SignatureV2: cfg.SignatureVersion == SignatureVersionV2,
	}, nil
}

[inbarpatashnik]

34a746f went too far 😄

This comment was a suggestion to only set the X-Amz-Storage-Class value if cfg.StorageClass was not an empty string.

So, I'm imagining something like this:

func newS3Config(cfg Config) (s3.Config, error) {
	sseCfg, err := cfg.SSE.BuildThanosConfig()
	if err != nil {
		return s3.Config{}, err
	}

	putUserMetadata := map[string]string{}

	if cfg.StorageClass != "" {
		putUserMetadata[awsStorageClassHeader] = cfg.StorageClass
	}

	return s3.Config{
		Bucket:          cfg.BucketName,
		Endpoint:        cfg.Endpoint,
		Region:          cfg.Region,
		AccessKey:       cfg.AccessKeyID,
		SecretKey:       cfg.SecretAccessKey.String(),
		Insecure:        cfg.Insecure,
		PutUserMetadata: putUserMetadata,
		SSEConfig:       sseCfg,
		HTTPConfig: s3.HTTPConfig{
			IdleConnTimeout:       model.Duration(cfg.HTTP.IdleConnTimeout),
			ResponseHeaderTimeout: model.Duration(cfg.HTTP.ResponseHeaderTimeout),
			InsecureSkipVerify:    cfg.HTTP.InsecureSkipVerify,
			TLSHandshakeTimeout:   model.Duration(cfg.HTTP.TLSHandshakeTimeout),
			ExpectContinueTimeout: model.Duration(cfg.HTTP.ExpectContinueTimeout),
			MaxIdleConns:          cfg.HTTP.MaxIdleConns,
			MaxIdleConnsPerHost:   cfg.HTTP.MaxIdleConnsPerHost,
			MaxConnsPerHost:       cfg.HTTP.MaxConnsPerHost,
			Transport:             cfg.HTTP.Transport,
		},
		// Enforce signature version 2 if CLI flag is set
		SignatureV2: cfg.SignatureVersion == SignatureVersionV2,
	}, nil
}

ohh, yes. Makes more sense🤭 @charleskorn
When changing I didn't understand how It would be saved in the config when removing it.
thanks :)

[charleskorn]

Thanks again @inbarpatashnik!

[inbarpatashnik]

Thanks again @inbarpatashnik!

Thank you!!🤗

[56quarters]

LGTM with some minor tweaks. Thanks!

[56quarters]

Can you group this with the other stdlib imports?

[inbarpatashnik]

@56quarters i tried to reorganize them, but it seems to fail in the lint :(

[56quarters]

Can you move these with the other stdlib imports?