Add option to use v2 signatures for S3 blocks store #3540
Conversation
simonswine
force-pushed
the
expose-option-to-configure-v2-auth
branch
from
aedd81a
to
7d99f48
Compare
pkg/storage/backend/s3/config.go
Outdated
| @@ -32,6 +32,7 @@ type Config struct { | |||
| SecretAccessKey flagext.Secret `yaml:"secret_access_key"` | |||
| AccessKeyID string `yaml:"access_key_id"` | |||
| Insecure bool `yaml:"insecure"` | |||
| SignatureV2 bool `yaml:"signature_v2"` | |||
There was a problem hiding this comment.
I think we’re missing setting of the corresponding field here:
There was a problem hiding this comment.
Yes, this field is currently unused. Needs to be passed to the S3 client.
Another comment I have. Such booleans are bad for extensibility in the future, in case a new version scheme will be introduced. You may achieve the same result allowing to configure the version number signature_version: 2|4, introducing a check in Config.Validate() (function to be added and called from parents) to make sure it's either 2 or 4. If in the future v6 will be introduced, this flag will support it too.
There was a problem hiding this comment.
Thanks for spotting the missing passing-through 😬
On using an enum parameter: I have done that as part of the last commit. I think it might be misrepresenting what is actually happening, I am not too sure as in the background the minio client seems to use https://github.com/minio/minio-go/blob/26addf203d4a7d73e33f0731b67a7c02a910632a/pkg/credentials/signature-type.go#L28 SignatureDefault, which is equal to v4. It might be more accurate to rename the parameters from v4 to default, but I felt that is more confusing.
Btw: I have purposely used strings with v prefix to avoid YAML parsing issues around integers vs. strings.
simonswine
force-pushed
the
expose-option-to-configure-v2-auth
branch
from
7d99f48
to
ca38b49
Compare
This allow to configure the block store client to use V2 signatures for S3 authentication. Signed-off-by: Christian Simon <simon@swine.de>
Signed-off-by: Christian Simon <simon@swine.de>
simonswine
force-pushed
the
expose-option-to-configure-v2-auth
branch
from
ca38b49
to
4533886
Compare
What this PR does:
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]