From 02083a6bfc40e693a9371f96b9ac37241ec889fa Mon Sep 17 00:00:00 2001 From: Robert van Gent Date: Fri, 15 Feb 2019 14:30:37 -0800 Subject: [PATCH 1/9] ckpt --- blob/s3blob/example_test.go | 4 ++- blob/s3blob/s3blob.go | 58 +++++++++++++++++++++++++++++-------- 2 files changed, 49 insertions(+), 13 deletions(-) diff --git a/blob/s3blob/example_test.go b/blob/s3blob/example_test.go index e9dfeea8b8..d0f755309d 100644 --- a/blob/s3blob/example_test.go +++ b/blob/s3blob/example_test.go @@ -18,6 +18,7 @@ import ( "context" "log" + "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/session" "gocloud.dev/blob" "gocloud.dev/blob/s3blob" @@ -26,7 +27,8 @@ import ( func Example() { // Establish an AWS session. // See https://docs.aws.amazon.com/sdk-for-go/api/aws/session/ for more info. - session, err := session.NewSession(nil) + // The region must match the region for "my_bucket". + session, err := session.NewSession(&aws.Config{Region: aws.String("us-west-1")}) if err != nil { log.Fatal(err) } diff --git a/blob/s3blob/s3blob.go b/blob/s3blob/s3blob.go index dc85061315..7f30805c51 100644 --- a/blob/s3blob/s3blob.go +++ b/blob/s3blob/s3blob.go @@ -81,6 +81,11 @@ func init() { } // URLOpener opens S3 URLs like "s3://mybucket". +// The following query options are supported: +// - region: The AWS region for requests; sets aws.Config.Region. +// - endpoint: The endpoint URL (hostname only or fully qualified URI); sets aws.Config.Endpoint. +// - disableSSL: A value of "true" disables SSL when sending requests; sets aws.Config.DisableSSL. +// - s3ForcePathStyle: A value of "true" forces the request to use path-style addressing; sets aws.Config.S3ForcePathStyle. type URLOpener struct { // ConfigProvider must be set to a non-nil value. ConfigProvider client.ConfigProvider @@ -120,32 +125,62 @@ const Scheme = "s3" // OpenBucketURL opens the S3 bucket with the same name as the host in the URL. func (o *URLOpener) OpenBucketURL(ctx context.Context, u *url.URL) (*blob.Bucket, error) { - for k := range u.Query() { - return nil, fmt.Errorf("open S3 bucket %q: unknown query parameter %s", u, k) - } - return OpenBucket(ctx, o.ConfigProvider, u.Host, &o.Options) + // See if there are any aws.Config overrides in the query parameters. + var cfg aws.Config + q := u.Query() + override := false + if region := q["region"]; len(region) > 0 { + cfg.Region = aws.String(region[0]) + override = true + } + if endpoint := q["endpoint"]; len(endpoint) > 0 { + cfg.Endpoint = aws.String(endpoint[0]) + override = true + } + if disableSSL := q["disableSSL"]; len(disableSSL) > 0 { + cfg.DisableSSL = aws.Bool(disableSSL[0] == "true") + override = true + } + if s3ForcePathStyle := q["s3ForcePathStyle"]; len(s3ForcePathStyle) > 0 { + cfg.S3ForcePathStyle = aws.Bool(s3ForcePathStyle[0] == "true") + override = true + } + opt := o.Options + if override { + opt.Cfgs = append(opt.Cfgs, &cfg) + } + return OpenBucket(ctx, o.ConfigProvider, u.Host, &opt) } // Options sets options for constructing a *blob.Bucket backed by fileblob. -type Options struct{} +type Options struct { + // Cfgs holds configuration overrides for the S3 client. For example, you + // may want to use the same *session.Session for buckets across multiple + // regions; you can override the aws.Config.Region here. + Cfgs []*aws.Config +} // openBucket returns an S3 Bucket. -func openBucket(ctx context.Context, sess client.ConfigProvider, bucketName string, _ *Options) (*bucket, error) { +func openBucket(ctx context.Context, sess client.ConfigProvider, bucketName string, opts *Options) (*bucket, error) { if sess == nil { return nil, errors.New("s3blob.OpenBucket: sess is required") } if bucketName == "" { return nil, errors.New("s3blob.OpenBucket: bucketName is required") } + if opts == nil { + opts = &Options{} + } return &bucket{ name: bucketName, - sess: sess, - client: s3.New(sess), + client: s3.New(sess, opts.Cfgs...), }, nil } -// OpenBucket returns a *blob.Bucket backed by S3. See the package documentation -// for an example. +// OpenBucket returns a *blob.Bucket backed by S3. +// AWS buckets are bound to a region; sess must have been created using an +// aws.Config with Region set to the right region for bucketName. +// See the package documentation for an example. func OpenBucket(ctx context.Context, sess client.ConfigProvider, bucketName string, opts *Options) (*blob.Bucket, error) { drv, err := openBucket(ctx, sess, bucketName, opts) if err != nil { @@ -260,7 +295,6 @@ func (w *writer) Close() error { // bucket represents an S3 bucket and handles read, write and delete operations. type bucket struct { name string - sess client.ConfigProvider client *s3.S3 } @@ -525,7 +559,7 @@ func unescapeKey(key string) string { // NewTypedWriter implements driver.NewTypedWriter. func (b *bucket) NewTypedWriter(ctx context.Context, key string, contentType string, opts *driver.WriterOptions) (driver.Writer, error) { key = escapeKey(key) - uploader := s3manager.NewUploader(b.sess, func(u *s3manager.Uploader) { + uploader := s3manager.NewUploaderWithClient(b.client, func(u *s3manager.Uploader) { if opts.BufferSize != 0 { u.PartSize = int64(opts.BufferSize) } From 31d8e91ae273042c2c9cf4a4392de65cdfe86f6a Mon Sep 17 00:00:00 2001 From: Robert van Gent Date: Fri, 15 Feb 2019 16:07:35 -0800 Subject: [PATCH 2/9] add tests --- blob/gcsblob/gcsblob_test.go | 37 ++++++++++++----- blob/s3blob/s3blob.go | 32 ++++++++++++--- blob/s3blob/s3blob_test.go | 78 ++++++++++++++++++++++++++++++++++++ 3 files changed, 130 insertions(+), 17 deletions(-) diff --git a/blob/gcsblob/gcsblob_test.go b/blob/gcsblob/gcsblob_test.go index 160de10c33..8ab1d06a16 100644 --- a/blob/gcsblob/gcsblob_test.go +++ b/blob/gcsblob/gcsblob_test.go @@ -294,35 +294,50 @@ func TestURLOpenerForParams(t *testing.T) { tests := []struct { name string query url.Values + prev Options wantOpts Options wantErr bool }{ { - name: "AccessID", + name: "Invalid query parameter", query: url.Values{ - "access_id": {"bar"}, + "foo": {"bar"}, }, + wantErr: true, + }, + { + name: "Previous options are carried over", + query: url.Values{}, + prev: Options{GoogleAccessID: "bar"}, wantOpts: Options{GoogleAccessID: "bar"}, }, { - name: "BadPrivateKeyPath", - query: url.Values{ - "private_key_path": {"/path/does/not/exist"}, - }, + name: "AccessID", + query: url.Values{"access_id": {"bar"}}, + wantOpts: Options{GoogleAccessID: "bar"}, + }, + { + name: "AccessID override", + query: url.Values{"access_id": {"baz"}}, + prev: Options{GoogleAccessID: "bar"}, + wantOpts: Options{GoogleAccessID: "baz"}, + }, + { + name: "BadPrivateKeyPath", + query: url.Values{"private_key_path": {"/path/does/not/exist"}}, wantErr: true, }, { - name: "PrivateKeyPath", - query: url.Values{ - "private_key_path": {pkFile.Name()}, - }, + name: "PrivateKeyPath", + query: url.Values{"private_key_path": {pkFile.Name()}}, wantOpts: Options{PrivateKey: privateKey}, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got, err := new(URLOpener).forParams(ctx, test.query) + u := &URLOpener{Options: test.prev} + got, err := u.forParams(ctx, test.query) if (err != nil) != test.wantErr { t.Errorf("got err %v want error %v", err, test.wantErr) } diff --git a/blob/s3blob/s3blob.go b/blob/s3blob/s3blob.go index 7f30805c51..3b6e8fe629 100644 --- a/blob/s3blob/s3blob.go +++ b/blob/s3blob/s3blob.go @@ -125,9 +125,27 @@ const Scheme = "s3" // OpenBucketURL opens the S3 bucket with the same name as the host in the URL. func (o *URLOpener) OpenBucketURL(ctx context.Context, u *url.URL) (*blob.Bucket, error) { - // See if there are any aws.Config overrides in the query parameters. + opts, err := o.forParams(ctx, u.Query()) + if err != nil { + return nil, fmt.Errorf("open bucket %v: %v", u, err) + } + return OpenBucket(ctx, o.ConfigProvider, u.Host, opts) +} + +var legalQueryParam = map[string]bool{ + "region": true, + "endpoint": true, + "disableSSL": true, + "s3ForcePathStyle": true, +} + +func (o *URLOpener) forParams(ctx context.Context, q url.Values) (*Options, error) { + for k := range q { + if !legalQueryParam[k] { + return nil, fmt.Errorf("unknown S3 query parameter %s", k) + } + } var cfg aws.Config - q := u.Query() override := false if region := q["region"]; len(region) > 0 { cfg.Region = aws.String(region[0]) @@ -145,11 +163,13 @@ func (o *URLOpener) OpenBucketURL(ctx context.Context, u *url.URL) (*blob.Bucket cfg.S3ForcePathStyle = aws.Bool(s3ForcePathStyle[0] == "true") override = true } - opt := o.Options - if override { - opt.Cfgs = append(opt.Cfgs, &cfg) + if !override { + return &o.Options, nil } - return OpenBucket(ctx, o.ConfigProvider, u.Host, &opt) + opts := new(Options) + *opts = o.Options + opts.Cfgs = append(opts.Cfgs, &cfg) + return opts, nil } // Options sets options for constructing a *blob.Bucket backed by fileblob. diff --git a/blob/s3blob/s3blob_test.go b/blob/s3blob/s3blob_test.go index 26e5ed8190..443ee495cb 100644 --- a/blob/s3blob/s3blob_test.go +++ b/blob/s3blob/s3blob_test.go @@ -18,7 +18,9 @@ import ( "context" "errors" "fmt" + "github.com/google/go-cmp/cmp" "net/http" + "net/url" "testing" "github.com/aws/aws-sdk-go/aws" @@ -220,3 +222,79 @@ func TestOpenBucket(t *testing.T) { }) } } + +func TestURLOpenerForParams(t *testing.T) { + ctx := context.Background() + + tests := []struct { + name string + query url.Values + prev Options + wantOpts Options + wantErr bool + }{ + { + name: "Invalid query parameter", + query: url.Values{"foo": {"bar"}}, + wantErr: true, + }, + { + name: "Previous options are carried over", + query: url.Values{}, + prev: Options{Cfgs: []*aws.Config{{Region: aws.String("foo")}}}, + wantOpts: Options{Cfgs: []*aws.Config{{Region: aws.String("foo")}}}, + }, + { + name: "Region", + query: url.Values{"region": {"my_region"}}, + wantOpts: Options{Cfgs: []*aws.Config{{Region: aws.String("my_region")}}}, + }, + { + name: "Region override", + query: url.Values{"region": {"foo"}}, + prev: Options{Cfgs: []*aws.Config{{Region: aws.String("my_region")}}}, + wantOpts: Options{Cfgs: []*aws.Config{{Region: aws.String("my_region")}, {Region: aws.String("foo")}}}, + }, + { + name: "Endpoint", + query: url.Values{"endpoint": {"foo"}}, + wantOpts: Options{Cfgs: []*aws.Config{{Endpoint: aws.String("foo")}}}, + }, + { + name: "DisableSSL true", + query: url.Values{"disableSSL": {"true"}}, + wantOpts: Options{Cfgs: []*aws.Config{{DisableSSL: aws.Bool(true)}}}, + }, + { + name: "DisableSSL false", + query: url.Values{"disableSSL": {"not-true"}}, + wantOpts: Options{Cfgs: []*aws.Config{{DisableSSL: aws.Bool(false)}}}, + }, + { + name: "S3ForcePathStyle true", + query: url.Values{"s3ForcePathStyle": {"true"}}, + wantOpts: Options{Cfgs: []*aws.Config{{S3ForcePathStyle: aws.Bool(true)}}}, + }, + { + name: "S3ForcePathStyle false", + query: url.Values{"s3ForcePathStyle": {"not-true"}}, + wantOpts: Options{Cfgs: []*aws.Config{{S3ForcePathStyle: aws.Bool(false)}}}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + u := &URLOpener{Options: test.prev} + got, err := u.forParams(ctx, test.query) + if (err != nil) != test.wantErr { + t.Errorf("got err %v want error %v", err, test.wantErr) + } + if err != nil { + return + } + if diff := cmp.Diff(got, &test.wantOpts); diff != "" { + t.Errorf("opener.forParams(...) diff (-want +got):\n%s", diff) + } + }) + } +} From bb4ff5a33a1b43d25d64634a200ee90de39d886c Mon Sep 17 00:00:00 2001 From: Robert van Gent Date: Mon, 25 Feb 2019 10:12:55 -0800 Subject: [PATCH 3/9] ckpt --- blob/gcsblob/gcsblob_test.go | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/blob/gcsblob/gcsblob_test.go b/blob/gcsblob/gcsblob_test.go index 131ac38686..7f3f13ddfc 100644 --- a/blob/gcsblob/gcsblob_test.go +++ b/blob/gcsblob/gcsblob_test.go @@ -295,7 +295,6 @@ func TestURLOpenerForParams(t *testing.T) { name string currOpts Options query url.Values - prev Options wantOpts Options wantErr bool }{ @@ -306,19 +305,19 @@ func TestURLOpenerForParams(t *testing.T) { }, wantErr: true, }, - { - name: "AccessID", - query: url.Values{ - "foo": {"bar"}, - }, - wantErr: true, - }, { name: "Previous options are carried over", query: url.Values{}, prev: Options{GoogleAccessID: "bar"}, wantOpts: Options{GoogleAccessID: "bar"}, }, + { + name: "AccessID", + query: url.Values{ + "access_id": {"bar"}, + }, + wantErr: true, + }, { name: "AccessID override", currOpts: Options{GoogleAccessID: "foo"}, From 6868fd014461d98a226de33131bb264ddfeb2c26 Mon Sep 17 00:00:00 2001 From: Robert van Gent Date: Mon, 25 Feb 2019 10:13:15 -0800 Subject: [PATCH 4/9] ckpt --- blob/gcsblob/gcsblob_test.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/blob/gcsblob/gcsblob_test.go b/blob/gcsblob/gcsblob_test.go index 7f3f13ddfc..0eb6fae477 100644 --- a/blob/gcsblob/gcsblob_test.go +++ b/blob/gcsblob/gcsblob_test.go @@ -305,18 +305,12 @@ func TestURLOpenerForParams(t *testing.T) { }, wantErr: true, }, - { - name: "Previous options are carried over", - query: url.Values{}, - prev: Options{GoogleAccessID: "bar"}, - wantOpts: Options{GoogleAccessID: "bar"}, - }, { name: "AccessID", query: url.Values{ "access_id": {"bar"}, }, - wantErr: true, + wantOpts: Options{GoogleAccessID: "bar"}, }, { name: "AccessID override", @@ -339,8 +333,10 @@ func TestURLOpenerForParams(t *testing.T) { wantErr: true, }, { - name: "PrivateKeyPath", - query: url.Values{"private_key_path": {pkFile.Name()}}, + name: "PrivateKeyPath", + query: url.Values{ + "private_key_path": {pkFile.Name()}, + }, wantOpts: Options{PrivateKey: privateKey}, }, } From 84febc671f3808727b112936597b45b666dbd538 Mon Sep 17 00:00:00 2001 From: Robert van Gent Date: Mon, 25 Feb 2019 10:29:27 -0800 Subject: [PATCH 5/9] use aws.ConfigProvider instead of slice of Configs --- aws/aws.go | 14 +++++++++ blob/s3blob/s3blob.go | 18 ++++++----- blob/s3blob/s3blob_test.go | 61 +++++++++++++++----------------------- 3 files changed, 48 insertions(+), 45 deletions(-) diff --git a/aws/aws.go b/aws/aws.go index b7b6ee3be5..e80db0688f 100644 --- a/aws/aws.go +++ b/aws/aws.go @@ -42,3 +42,17 @@ func SessionConfig(sess *session.Session) *aws.Config { func ConfigCredentials(cfg *aws.Config) *credentials.Credentials { return cfg.Credentials } + +// ConfigOverrider implements client.ConfigProvider by overlaying a list of +// configurations over a base configuration provider. +type ConfigOverrider struct { + Base client.ConfigProvider + Configs []*aws.Config +} + + // ClientConfig calls the base provider's ClientConfig method with co.Configs +// followed by the arguments given to ClientConfig. +func (co ConfigOverrider) ClientConfig(serviceName string, cfgs ...*aws.Config) client.Config { + cfgs = append(co.Configs[:len(co.Configs):len(co.Configs)], cfgs...) + return co.Base.ClientConfig(serviceName, cfgs...) +} \ No newline at end of file diff --git a/blob/s3blob/s3blob.go b/blob/s3blob/s3blob.go index 3b6e8fe629..8c530eb6b1 100644 --- a/blob/s3blob/s3blob.go +++ b/blob/s3blob/s3blob.go @@ -61,6 +61,7 @@ import ( "strings" "sync" + gcaws "gocloud.dev/aws" "gocloud.dev/blob" "gocloud.dev/blob/driver" "gocloud.dev/gcerrors" @@ -125,11 +126,15 @@ const Scheme = "s3" // OpenBucketURL opens the S3 bucket with the same name as the host in the URL. func (o *URLOpener) OpenBucketURL(ctx context.Context, u *url.URL) (*blob.Bucket, error) { - opts, err := o.forParams(ctx, u.Query()) + configProvider := &gcaws.ConfigOverrider{ + Base: o.ConfigProvider, + } + overrideCfg, err := o.forParams(ctx, u.Query()) if err != nil { return nil, fmt.Errorf("open bucket %v: %v", u, err) } - return OpenBucket(ctx, o.ConfigProvider, u.Host, opts) + configProvider.Configs = append(configProvider.Configs, overrideCfg) + return OpenBucket(ctx, configProvider, u.Host, &o.Options) } var legalQueryParam = map[string]bool{ @@ -139,7 +144,7 @@ var legalQueryParam = map[string]bool{ "s3ForcePathStyle": true, } -func (o *URLOpener) forParams(ctx context.Context, q url.Values) (*Options, error) { +func (o *URLOpener) forParams(ctx context.Context, q url.Values) (*aws.Config, error) { for k := range q { if !legalQueryParam[k] { return nil, fmt.Errorf("unknown S3 query parameter %s", k) @@ -164,12 +169,9 @@ func (o *URLOpener) forParams(ctx context.Context, q url.Values) (*Options, erro override = true } if !override { - return &o.Options, nil + return nil, nil } - opts := new(Options) - *opts = o.Options - opts.Cfgs = append(opts.Cfgs, &cfg) - return opts, nil + return &cfg, nil } // Options sets options for constructing a *blob.Bucket backed by fileblob. diff --git a/blob/s3blob/s3blob_test.go b/blob/s3blob/s3blob_test.go index 443ee495cb..02a86c6f23 100644 --- a/blob/s3blob/s3blob_test.go +++ b/blob/s3blob/s3blob_test.go @@ -227,11 +227,10 @@ func TestURLOpenerForParams(t *testing.T) { ctx := context.Background() tests := []struct { - name string - query url.Values - prev Options - wantOpts Options - wantErr bool + name string + query url.Values + wantCfg *aws.Config + wantErr bool }{ { name: "Invalid query parameter", @@ -239,52 +238,40 @@ func TestURLOpenerForParams(t *testing.T) { wantErr: true, }, { - name: "Previous options are carried over", - query: url.Values{}, - prev: Options{Cfgs: []*aws.Config{{Region: aws.String("foo")}}}, - wantOpts: Options{Cfgs: []*aws.Config{{Region: aws.String("foo")}}}, + name: "Region", + query: url.Values{"region": {"my_region"}}, + wantCfg: &aws.Config{Region: aws.String("my_region")}, }, { - name: "Region", - query: url.Values{"region": {"my_region"}}, - wantOpts: Options{Cfgs: []*aws.Config{{Region: aws.String("my_region")}}}, + name: "Endpoint", + query: url.Values{"endpoint": {"foo"}}, + wantCfg: &aws.Config{Endpoint: aws.String("foo")}, }, { - name: "Region override", - query: url.Values{"region": {"foo"}}, - prev: Options{Cfgs: []*aws.Config{{Region: aws.String("my_region")}}}, - wantOpts: Options{Cfgs: []*aws.Config{{Region: aws.String("my_region")}, {Region: aws.String("foo")}}}, + name: "DisableSSL true", + query: url.Values{"disableSSL": {"true"}}, + wantCfg: &aws.Config{DisableSSL: aws.Bool(true)}, }, { - name: "Endpoint", - query: url.Values{"endpoint": {"foo"}}, - wantOpts: Options{Cfgs: []*aws.Config{{Endpoint: aws.String("foo")}}}, + name: "DisableSSL false", + query: url.Values{"disableSSL": {"not-true"}}, + wantCfg: &aws.Config{DisableSSL: aws.Bool(false)}, }, { - name: "DisableSSL true", - query: url.Values{"disableSSL": {"true"}}, - wantOpts: Options{Cfgs: []*aws.Config{{DisableSSL: aws.Bool(true)}}}, + name: "S3ForcePathStyle true", + query: url.Values{"s3ForcePathStyle": {"true"}}, + wantCfg: &aws.Config{S3ForcePathStyle: aws.Bool(true)}, }, { - name: "DisableSSL false", - query: url.Values{"disableSSL": {"not-true"}}, - wantOpts: Options{Cfgs: []*aws.Config{{DisableSSL: aws.Bool(false)}}}, - }, - { - name: "S3ForcePathStyle true", - query: url.Values{"s3ForcePathStyle": {"true"}}, - wantOpts: Options{Cfgs: []*aws.Config{{S3ForcePathStyle: aws.Bool(true)}}}, - }, - { - name: "S3ForcePathStyle false", - query: url.Values{"s3ForcePathStyle": {"not-true"}}, - wantOpts: Options{Cfgs: []*aws.Config{{S3ForcePathStyle: aws.Bool(false)}}}, + name: "S3ForcePathStyle false", + query: url.Values{"s3ForcePathStyle": {"not-true"}}, + wantCfg: &aws.Config{S3ForcePathStyle: aws.Bool(false)}, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - u := &URLOpener{Options: test.prev} + u := &URLOpener{} got, err := u.forParams(ctx, test.query) if (err != nil) != test.wantErr { t.Errorf("got err %v want error %v", err, test.wantErr) @@ -292,7 +279,7 @@ func TestURLOpenerForParams(t *testing.T) { if err != nil { return } - if diff := cmp.Diff(got, &test.wantOpts); diff != "" { + if diff := cmp.Diff(got, test.wantCfg); diff != "" { t.Errorf("opener.forParams(...) diff (-want +got):\n%s", diff) } }) From 3e98172c9f29af7b7e3dd6d323f03451891986d5 Mon Sep 17 00:00:00 2001 From: Robert van Gent Date: Mon, 25 Feb 2019 10:30:13 -0800 Subject: [PATCH 6/9] gofmt --- aws/aws.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/aws/aws.go b/aws/aws.go index e80db0688f..27eb72200b 100644 --- a/aws/aws.go +++ b/aws/aws.go @@ -50,9 +50,9 @@ type ConfigOverrider struct { Configs []*aws.Config } - // ClientConfig calls the base provider's ClientConfig method with co.Configs +// ClientConfig calls the base provider's ClientConfig method with co.Configs // followed by the arguments given to ClientConfig. func (co ConfigOverrider) ClientConfig(serviceName string, cfgs ...*aws.Config) client.Config { cfgs = append(co.Configs[:len(co.Configs):len(co.Configs)], cfgs...) return co.Base.ClientConfig(serviceName, cfgs...) -} \ No newline at end of file +} From 83be65bc89a04ea6ded9f846ccd0e8471ec2c17a Mon Sep 17 00:00:00 2001 From: Robert van Gent Date: Mon, 25 Feb 2019 10:31:35 -0800 Subject: [PATCH 7/9] don't append nil cfg --- blob/s3blob/s3blob.go | 4 +++- blob/s3blob/s3blob_test.go | 5 +++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/blob/s3blob/s3blob.go b/blob/s3blob/s3blob.go index 8c530eb6b1..9e8b58e87e 100644 --- a/blob/s3blob/s3blob.go +++ b/blob/s3blob/s3blob.go @@ -133,7 +133,9 @@ func (o *URLOpener) OpenBucketURL(ctx context.Context, u *url.URL) (*blob.Bucket if err != nil { return nil, fmt.Errorf("open bucket %v: %v", u, err) } - configProvider.Configs = append(configProvider.Configs, overrideCfg) + if overrideCfg != nil { + configProvider.Configs = append(configProvider.Configs, overrideCfg) + } return OpenBucket(ctx, configProvider, u.Host, &o.Options) } diff --git a/blob/s3blob/s3blob_test.go b/blob/s3blob/s3blob_test.go index 02a86c6f23..c3429ebf88 100644 --- a/blob/s3blob/s3blob_test.go +++ b/blob/s3blob/s3blob_test.go @@ -232,6 +232,11 @@ func TestURLOpenerForParams(t *testing.T) { wantCfg *aws.Config wantErr bool }{ + { + name: "No overrides", + query: url.Values{}, + wantCfg: nil, + }, { name: "Invalid query parameter", query: url.Values{"foo": {"bar"}}, From 953c89387904f4dce0cf42dd5bd280dd4fd24a8e Mon Sep 17 00:00:00 2001 From: Robert van Gent Date: Mon, 25 Feb 2019 10:33:00 -0800 Subject: [PATCH 8/9] remove unused Option.Cfgs --- blob/s3blob/s3blob.go | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/blob/s3blob/s3blob.go b/blob/s3blob/s3blob.go index 9e8b58e87e..dac62dfd69 100644 --- a/blob/s3blob/s3blob.go +++ b/blob/s3blob/s3blob.go @@ -177,27 +177,19 @@ func (o *URLOpener) forParams(ctx context.Context, q url.Values) (*aws.Config, e } // Options sets options for constructing a *blob.Bucket backed by fileblob. -type Options struct { - // Cfgs holds configuration overrides for the S3 client. For example, you - // may want to use the same *session.Session for buckets across multiple - // regions; you can override the aws.Config.Region here. - Cfgs []*aws.Config -} +type Options struct {} // openBucket returns an S3 Bucket. -func openBucket(ctx context.Context, sess client.ConfigProvider, bucketName string, opts *Options) (*bucket, error) { +func openBucket(ctx context.Context, sess client.ConfigProvider, bucketName string, _ *Options) (*bucket, error) { if sess == nil { return nil, errors.New("s3blob.OpenBucket: sess is required") } if bucketName == "" { return nil, errors.New("s3blob.OpenBucket: bucketName is required") } - if opts == nil { - opts = &Options{} - } return &bucket{ name: bucketName, - client: s3.New(sess, opts.Cfgs...), + client: s3.New(sess), }, nil } From 6e35d5308cd4124ef899406bfa05d8d5d9245a39 Mon Sep 17 00:00:00 2001 From: Robert van Gent Date: Mon, 25 Feb 2019 18:12:39 -0800 Subject: [PATCH 9/9] remove extra space --- blob/s3blob/s3blob.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/blob/s3blob/s3blob.go b/blob/s3blob/s3blob.go index dac62dfd69..85fd5ed48c 100644 --- a/blob/s3blob/s3blob.go +++ b/blob/s3blob/s3blob.go @@ -177,7 +177,7 @@ func (o *URLOpener) forParams(ctx context.Context, q url.Values) (*aws.Config, e } // Options sets options for constructing a *blob.Bucket backed by fileblob. -type Options struct {} +type Options struct{} // openBucket returns an S3 Bucket. func openBucket(ctx context.Context, sess client.ConfigProvider, bucketName string, _ *Options) (*bucket, error) {