Add validation for newS3Config (#3410) #3414
Conversation
|
|
| if strings.HasPrefix(cfg.Endpoint, cfg.BucketName) { | ||
| return s3.Config{}, fmt.Errorf("the endpoint must not prefixed with the bucket name: %s", cfg.BucketName) | ||
| } |
There was a problem hiding this comment.
We should do this in the Config.Validate() method:
mimir/pkg/storage/bucket/s3/config.go
Lines 104 to 108 in 931429b
There was a problem hiding this comment.
@colega Thanks, reviewing. I fixed it and added change log and unit test.
|
Can you please move the validation to the |
CHANGELOG.md
Outdated
| @@ -4,6 +4,7 @@ | |||
|
|
|||
| ### Grafana Mimir | |||
|
|
|||
| * [CHANGE] Added Validation when S3 endpoint had a prefix of bucket name. #3414 | |||
There was a problem hiding this comment.
I would say this is an ENHANCEMENT, since we're not adding a new restriction, we're just validating it earlier, WDYT?
Also, please move the changelog entry at the end of the category.
CHANGELOG.md
Outdated
| @@ -30,6 +30,7 @@ | |||
| * [BUGFIX] Updated `golang.org/x/text` dependency to fix CVE-2022-32149. #3285 | |||
| * [BUGFIX] Query-frontend: properly close gRPC streams to the query-scheduler to stop memory and goroutines leak. #3302 | |||
| * [BUGFIX] Ruler: persist evaluation delay configured in the rulegroup. #3392 | |||
| * [ENHANCEMENT] Added Validation when S3 endpoint had a prefix of bucket name. #3414 | |||
There was a problem hiding this comment.
Sorry again, I mean that this should go after all the "[ENHANCEMENT]" elements.
Also could you please rehprase this as something like:
S3 bucket configuration now validates that the endpoint does not have the bucket name prefix.
(Why am I suggestion this change? This way the first words say what we're changing: S3 bucket configuration, so it's easier to visually scan for changes when something went wrong. Also lowercased the "validation word.)
| @@ -37,7 +37,6 @@ func newS3Config(cfg Config) (s3.Config, error) { | |||
| if err != nil { | |||
| return s3.Config{}, err | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Maybe we can revert this file to main's version to avoid adding the unneeded change here?
pkg/storage/bucket/s3/config_test.go
Outdated
| Endpoint: "mimir-blocks.s3.eu-central-1.amazonaws.com", | ||
| BucketName: "mimir-block", |
There was a problem hiding this comment.
This is interesting because shows a sort of flaw in the check. The bucket name is NOT the exact endpoint prefix. I think you should improve the check (strings.HasPrefix()) to also look for the dot (.) at the end of the bucket name when checking it.
* endpoint has prefixed with bucket name Signed-off-by: u5surf <u5.horie@gmail.com>
* Add validation for newS3Config (grafana#3410) * endpoint has prefixed with bucket name Signed-off-by: u5surf <u5.horie@gmail.com> * Update pkg/storage/bucket/s3/config.go Signed-off-by: u5surf <u5.horie@gmail.com> Co-authored-by: Marco Pracucci <marco@pracucci.com>
* Add validation for newS3Config (grafana#3410) * endpoint has prefixed with bucket name Signed-off-by: u5surf <u5.horie@gmail.com> * Update pkg/storage/bucket/s3/config.go Signed-off-by: u5surf <u5.horie@gmail.com> Co-authored-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: u5surf u5.horie@gmail.com
What this PR does
To fix the issue of 3410, we should be added the validation of endpoint which has not prefixed with bucket name.
Which issue(s) this PR fixes or relates to
Fixes #3410
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]