Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Per call objstore Upload/Get options #3781

Closed
pracucci opened this issue Feb 10, 2021 · 2 comments · Fixed by #3783
Closed

Per call objstore Upload/Get options #3781

pracucci opened this issue Feb 10, 2021 · 2 comments · Fixed by #3783

Comments

@pracucci
Copy link
Contributor

Is your proposal related to a problem?

Cortex uses on Thanos objstore clients. Cortex needs to build per-tenant S3 encryption key support and would like to avoid to create a new S3 client for each tenant: having a single S3 client allows to reuse S3 connections.

Describe the solution you'd like

The Thanos S3 client allows to config the SSEConfig at client creation time. In order to reuse the same client for different tenants, we would need to be able to pass a custom SSEConfig to each single Get() and Upload() call.

A solution would be introducing the ability to pass options to Get() and Upload(). To not introduce breaking changes in the objstore.Bucket signature we could leverage on the context.Context.

An idea could be having a WithOptions(objstore.Bucket) objstore.Bucket generic function to wrap a objstore.Bucket with options which are then injected in the context by the options wrapper. The underlying client implementation (eg. s3.go) would be changed to be able to read such options and configure the Get/Upload operation accordingly.

If a bucket doesn't support options (eg. SSE encryption is only supported by S3) such options would be ignored.

Describe alternatives you've considered

  • Create an S3 client for each tenant (not an option, we have a large number of tenants managed for each process, we can't afford to re-create S3 connections for each of them)
  • Fork S3 client in Cortex (still an option, but not desired)

Additional context

  • Slack thread
  • Encryption in Thanos objstore clients is currently supported only by S3
@bwplotka
Copy link
Member

bwplotka commented Feb 11, 2021

Just to continue our discussions from Slack.

An idea could be having a WithOptions(objstore.Bucket) objstore.Bucket generic function to wrap a objstore.Bucket with options which are then injected in the context by the options wrapper. The underlying client implementation (eg. s3.go) would be changed to be able to read such options and configure the Get/Upload operation accordingly.
If a bucket doesn't support options (eg. SSE encryption is only supported by S3) such options would be ignored.

I love this direction. Ideally, it would be awesome if the implementation (fact that it is S3 or not) does not leak from objstore at all.

Wonder only what's the difference between WithOptions(objstore.Bucket) and some Wrapper struct in objstore package that can use context passed values (private to objstore)

@pracucci
Copy link
Contributor Author

We discussed this in the 2021-02-11 contributor office hour and decided to start simple, with an exposed context key defined in pkg/objstore/s3/ which allows to override the SSE config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants