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

Add per-tenant S3 KMS SSE encryption key support to blocks storage #3811

Conversation

pracucci
Copy link
Contributor

@pracucci pracucci commented Feb 10, 2021

What this PR does:
In the PR #3810 I'm proposing to add S3 KMS SSE encryption key support with a global config. On top of that, we'll need to also add support for a per-tenant encryption key override.

In this PR I'm proposing a refactoring to add the support in the blocks storage. The idea is to use UserBucketClient everywhere and take in input overrides. The UserBucketClient.Upload() will read the per-tenant encryption key from overrides and then inject it when calling the parent Upload() (idea is to inject via context).

I've manually tested it and looks working.

Which issue(s) this PR fixes:
N/A

Checklist

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

Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM so far, except I wouldn't rename existing interfaces to include "Overrides", and instead of adding the S3SSEKMSKeyID method everywhere, we can extend existing interfaces with newly introduced one.

pkg/storage/bucket/user_bucket_client.go Outdated Show resolved Hide resolved
pkg/chunk/purger/blocks_purger_api.go Outdated Show resolved Hide resolved
pkg/querier/blocks_store_queryable.go Outdated Show resolved Hide resolved
pkg/storage/bucket/user_bucket_client.go Show resolved Hide resolved
@pracucci pracucci force-pushed the add-per-tenant-s3-kms-sse-encryption-key branch 2 times, most recently from 4418cfe to 3aeec40 Compare February 11, 2021 11:18
Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I would still prefer if we didn't rename existing BlocksStoreLimits type to BlocksStoreConfigOverrides.

pkg/querier/blocks_store_queryable.go Outdated Show resolved Hide resolved
@pracucci pracucci force-pushed the add-per-tenant-s3-kms-sse-encryption-key branch 2 times, most recently from 14702f4 to 9261743 Compare February 16, 2021 14:09
@pracucci pracucci marked this pull request as ready for review February 16, 2021 14:12
Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice work!

…nt basis.

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci force-pushed the add-per-tenant-s3-kms-sse-encryption-key branch from 0f038da to e31f09b Compare February 22, 2021 08:43
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed latest changes – great job! I think CHANGELOG.md should mention the fact that SSE config can be changed for each tenant individually, using overrides mechanism in the runtime config.

Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci merged commit 3018a54 into cortexproject:master Feb 22, 2021
@pracucci pracucci deleted the add-per-tenant-s3-kms-sse-encryption-key branch February 22, 2021 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants