-
Notifications
You must be signed in to change notification settings - Fork 622
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
Added bucket-lookup-type field to s3 config #3788
Open
devmentality
wants to merge
1
commit into
grafana:main
Choose a base branch
from
devmentality:bucket-lookup-type
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -31,14 +31,25 @@ const ( | |||||
// SSES3 config type constant to configure S3 server side encryption with AES-256 | ||||||
// https://docs.aws.amazon.com/AmazonS3/latest/dev/UsingServerSideEncryption.html | ||||||
SSES3 = "SSE-S3" | ||||||
|
||||||
// https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html#path-style-access | ||||||
PathStyleLookup = "path-style" | ||||||
|
||||||
// https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html#virtual-hosted-style-access | ||||||
VirtualHostedStyleLookup = "virtual-hosted-style" | ||||||
|
||||||
AutoLookup = "auto" | ||||||
) | ||||||
|
||||||
var ( | ||||||
supportedSignatureVersions = []string{SignatureVersionV4, SignatureVersionV2} | ||||||
supportedSSETypes = []string{SSEKMS, SSES3} | ||||||
supportedBucketLookupTypes = []string{PathStyleLookup, VirtualHostedStyleLookup, AutoLookup} | ||||||
errUnsupportedSignatureVersion = errors.New("unsupported signature version") | ||||||
errUnsupportedSSEType = errors.New("unsupported S3 SSE type") | ||||||
errInvalidSSEContext = errors.New("invalid S3 SSE encryption context") | ||||||
errBucketLookupConfigConflict = errors.New("cannot use s3.force-path-style = true and s3.bucket-lookup-type = virtual-hosted-style at the same time") | ||||||
errUnsupportedBucketLookupType = errors.New("invalid S3 bucket lookup type") | ||||||
) | ||||||
|
||||||
// HTTPConfig stores the http.Transport configuration for the s3 minio client. | ||||||
|
@@ -78,6 +89,7 @@ type Config struct { | |||||
Insecure bool `yaml:"insecure" category:"advanced"` | ||||||
SignatureVersion string `yaml:"signature_version" category:"advanced"` | ||||||
ForcePathStyle bool `yaml:"force_path_style" category:"advanced"` | ||||||
BucketLookupType string `yaml:"bucket_lookup_type" category:"advanced"` | ||||||
|
||||||
SSE SSEConfig `yaml:"sse"` | ||||||
HTTP HTTPConfig `yaml:"http"` | ||||||
|
@@ -96,7 +108,8 @@ func (cfg *Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) { | |||||
f.StringVar(&cfg.Region, prefix+"s3.region", "", "S3 region. If unset, the client will issue a S3 GetBucketLocation API call to autodetect it.") | ||||||
f.StringVar(&cfg.Endpoint, prefix+"s3.endpoint", "", "The S3 bucket endpoint. It could be an AWS S3 endpoint listed at https://docs.aws.amazon.com/general/latest/gr/s3.html or the address of an S3-compatible service in hostname:port format.") | ||||||
f.BoolVar(&cfg.Insecure, prefix+"s3.insecure", false, "If enabled, use http:// for the S3 endpoint instead of https://. This could be useful in local dev/test environments while using an S3-compatible backend storage, like Minio.") | ||||||
f.BoolVar(&cfg.ForcePathStyle, prefix+"s3.force-path-style", false, "Set this to `true` to force the bucket lookup to be using path-style.") | ||||||
f.BoolVar(&cfg.ForcePathStyle, prefix+"s3.force-path-style", false, "Deprecated, use s3.bucket-lookup-type instead. Set this to `true` to force the bucket lookup to be using path-style.") | ||||||
f.StringVar(&cfg.BucketLookupType, prefix+"s3.bucket-lookup-type", AutoLookup, fmt.Sprintf("S3 bucket lookup style, use one of: %v", supportedBucketLookupTypes)) | ||||||
f.StringVar(&cfg.SignatureVersion, prefix+"s3.signature-version", SignatureVersionV4, fmt.Sprintf("The signature version to use for authenticating against S3. Supported values are: %s.", strings.Join(supportedSignatureVersions, ", "))) | ||||||
cfg.SSE.RegisterFlagsWithPrefix(prefix+"s3.sse.", f) | ||||||
cfg.HTTP.RegisterFlagsWithPrefix(prefix, f) | ||||||
|
@@ -108,6 +121,14 @@ func (cfg *Config) Validate() error { | |||||
return errUnsupportedSignatureVersion | ||||||
} | ||||||
|
||||||
if cfg.ForcePathStyle && cfg.BucketLookupType == VirtualHostedStyleLookup { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Might be slightly more failsafe, once there is another lookup type added (I hope not 😆 )
Suggested change
|
||||||
return errBucketLookupConfigConflict | ||||||
} | ||||||
|
||||||
if !lo.Contains(supportedBucketLookupTypes, cfg.BucketLookupType) { | ||||||
return errUnsupportedBucketLookupType | ||||||
} | ||||||
|
||||||
if err := cfg.SSE.Validate(); err != nil { | ||||||
return err | ||||||
} | ||||||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we also should add a deprecation log warning when ForcePath argument is used, so users can notice it and migrate to
BucketLookupType
Wdyt?