Open AWS storage to being used with non-AWS MySQL and S3 services #428
Conversation
| var awsConfig *aaws.Config | ||
| var s3Opts func(o *s3.Options) | ||
| if *s3Endpoint != "" { | ||
| const defaultRegion = "us-east-1" |
There was a problem hiding this comment.
It looks to me that this will always set the region to us-east-1 as soon as someone sets s3Endpoint. That will then set SDKConfig which means that LoadDefaultConfig will be skipped, thus skipping other values that would be in the default config. At the moment, we only use LoadDefaultConfig for S3 but that might not always be the case, so let's disambiguate this to avoid leaving something to trip on later? Note, that I don't know how LoadDefaultConfig works with MinIO, but it looks like this might be tricky if someone uses both AWS and the new flags. That should not be done in the first place, but if the code doesn't prevent it (should it?!), then let's make sure there's no abiguity?
There was a problem hiding this comment.
That's intentional - the idea here is that you only ever set --s3_endpoint if you're going "off-piste" and running outside of AWS, in which case the aws default config no longer applies.
There was a problem hiding this comment.
Things I'm thinking of:
- If someone uses a local MySQL instance, they're "off piste", but might not experience the same behavior depending on whether they specify the S3 endpoint using the flags, or
LoadDefaultConfig, which is..... fine because they're off piste? - Right now
LoadDefaultConfigis only called for S3, but my plan was to change this such that it can use IAM authentication for MySQL instead of user and password. We might have to reorganise things then, but seems fine to me with the current code.
There was a problem hiding this comment.
Cool, yeah - I think this is only a problem if they want to use "half AWS, half off"(?) which is definitely a non-supported config, so, as you say, probably ok that it doesn't work.
I'm trying to avoid having the amazon SDK "inadvertently" populate the client with AWS credentials, endpoint URLs, or anything else it does "by default" if you're in off-piste mode.
| // supported configuration. | ||
| SDKConfig *aws.Config | ||
| // S3Options is an optional function which can be used to configure the S3 library. | ||
| // This is primarily useful when configuring the use of non-AWS S3 or MySQL services. |
There was a problem hiding this comment.
I think that S3Options has no impact on MySQL, right? The comment says otherwise.
There was a problem hiding this comment.
Strictly speaking you're right, but intention of the comment is to make it clear that if you're setting these options you're outside of AWS and the supported config; i.e. it's only really useful to use S3Options if you're configuring the binary to use non-AWS services.
| @@ -873,3 +891,13 @@ func (s *s3Storage) lastModified(ctx context.Context, obj string) (time.Time, er | |||
|
|
|||
| return *r.LastModified, r.Body.Close() | |||
| } | |||
|
|
|||
| func printDragonsWarning() { | |||
| d := `H4sIAFZYZGcAA01QMQ7EIAzbeYXV5UCqkq1bf2IFtpNuPalj334hFQdkwLGNAwBzyXnKitOiqTYj | |||
There was a problem hiding this comment.
Couldn't pass up the option :)
storage/aws/aws.go
Outdated
| g, _ := base64.StdEncoding.DecodeString(d) | ||
| r, _ := gzip.NewReader(bytes.NewReader(g)) | ||
| t, _ := io.ReadAll(r) | ||
| klog.Infof("Running in non-AWS mode: here be dragons!\n%s", t) |
| var awsConfig *aaws.Config | ||
| var s3Opts func(o *s3.Options) | ||
| if *s3Endpoint != "" { | ||
| const defaultRegion = "us-east-1" |
There was a problem hiding this comment.
Things I'm thinking of:
- If someone uses a local MySQL instance, they're "off piste", but might not experience the same behavior depending on whether they specify the S3 endpoint using the flags, or
LoadDefaultConfig, which is..... fine because they're off piste? - Right now
LoadDefaultConfigis only called for S3, but my plan was to change this such that it can use IAM authentication for MySQL instead of user and password. We might have to reorganise things then, but seems fine to me with the current code.
This PR enables the
awsstorage implementation to be used withMySQLandS3compatible storage services outside of theAWSenvironment.Such usage is unsupported; folks doing this must verify that things are working correctly themselves. That said, this change has been (very minimally and quickly) tested locally with MariaDB and a local MinIO server running in docker.