add sse config to tempo #3914
add sse config to tempo #3914
Conversation
KyriosGN0
requested review from
joe-elliott,
annanay25,
mdisibio,
mapno,
yvrhdn,
zalegrala,
electron0zero,
ie-pham and
stoewer
as code owners
|
@javiermolinar according to this, it is needed |
tempodb/backend/s3/s3.go
Outdated
| @@ -128,11 +130,31 @@ func internalNew(cfg *Config, confirm bool) (*readerWriter, error) { | |||
| } | |||
|
|
|||
| func getPutObjectOptions(rw *readerWriter) minio.PutObjectOptions { | |||
| sseConfig, err := buildSSEConfig(rw.cfg) | |||
| if sseConfig == nil && err == nil { | |||
There was a problem hiding this comment.
This condition is wrong, for instance, the following configuration:
sse:
type: blabla
Will create a PutObjectOption with SSE
There was a problem hiding this comment.
it should not, since buildSSEConfig checks if the config is incorrect, did you happen to test it and see it working (i didn't have the change to test it myself yet nor write test)
There was a problem hiding this comment.
In that scenario, sseConfig = nil and err != nil, therefore it will go for the else branch. It doesn't matter since the sseConfig is nil so it won't affect. Maybe we could log a warning indicating that the SSE type is not valid
| StorageClass: rw.cfg.StorageClass, | ||
| UserMetadata: rw.cfg.Metadata, | ||
| ServerSideEncryption: sseConfig, | ||
| } |
There was a problem hiding this comment.
I would expect an early fatal error if the SSE is set but wrongly configured. Nevertheless, it will fail trying to write or read the blocks
There was a problem hiding this comment.
what do you mean erroring fataly?, at the moment the buildSSEConfig will return errUnsupportedSSEType if it is configured with unsupported SSE type, and i have added an error for missing KMSKeyID, what else am i missing
There was a problem hiding this comment.
As a user configuring SSE I would like to know if my configuration is not correctly set. Maybe a warning log?
There was a problem hiding this comment.
if we've set a the config incorrectly we should error, i'll try to set up a minio locally with SSE and spin up tempo and test various configs
|
Will this PR impact how a user configures Tempo? Should we update the config doc in this PR or open a doc issue specifically for those changes? |
It will add a way to configure server-side encryption for S3. It should be documented. |
KyriosGN0
force-pushed
the
feat/sse-tempo
branch
from
1ebae80
to
f8a3edf
Compare
|
Thank you for updating the changelog. Should we add doc for this config option? Is it a user-accessible configuration option? |
KyriosGN0
force-pushed
the
feat/sse-tempo
branch
from
441751c
to
528d892
Compare
|
@knylander-grafana i will update the docs and ping you once i've verified it works, Thanks! |
|
hey @javiermolinar, im still trying to test with minio, could you (plase) try to test with S3 bucket in AWS ? |
|
hey @javiermolinar did you have a chance to test this ? if not i'll try to spin this myself and hopefully incur too much cost |
Hey, I'll take a look at this on Monday |
|
Thank you for your patience @KyriosGN0, this looks much better now, we need to fix the conflicts and add the new configuration to the docs and we are ready to go: https://github.com/grafana/tempo/blob/main/docs/sources/tempo/configuration/_index.md#storage |
Signed-off-by: AvivGuiser <[email protected]>
Signed-off-by: AvivGuiser <[email protected]>
Signed-off-by: AvivGuiser <[email protected]>
Signed-off-by: AvivGuiser <[email protected]>
Signed-off-by: AvivGuiser <[email protected]>
Signed-off-by: AvivGuiser <[email protected]>
Signed-off-by: AvivGuiser <[email protected]>
Signed-off-by: AvivGuiser <[email protected]>
KyriosGN0
force-pushed
the
feat/sse-tempo
branch
from
528d892
to
bd0e2c1
Compare
Signed-off-by: AvivGuiser <[email protected]>
KyriosGN0
force-pushed
the
feat/sse-tempo
branch
from
ba62d06
to
e77b37b
Compare
|
@javiermolinar fixed the conflicts and added dock @knylander-grafana, can you check please? |
One pending comment and we are ready |
What this PR does:
Which issue(s) this PR fixes:
Fixes #1486
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]