Adds custom registry User-Agent header to s3 HTTP requests

[BrianBland]

Uses docker/goamz instead of AdRoll/goamz

Adds a registry UA string param to the storage parameters when
constructing the storage driver for the registry App.
This could be used by other storage drivers as well

Addresses #1353

[BrianBland]

Also enables s3 connection pooling as a side-effect

[codecov-io]

Current coverage is 53.82%

Merging #1381 into master will decrease coverage by -3.43% as of 2dc1af1

@@            master   #1381   diff @@
======================================
  Files          123     123       
  Stmts        11338   11127   -211
  Branches       798     705    -93
  Methods          0       0       
======================================
- Hit           6492    5989   -503
+ Partial        798     705    -93
- Missed        4048    4433   +385

Review entire Coverage Diff as of 2dc1af1

---

Uncovered Suggestions

1. +0.29% via ...ge/driver/gcs/gcs.go#182...213
2. +0.24% via ...ge/driver/gcs/gcs.go#85...111
3. +0.21% via ...ge/driver/gcs/gcs.go#471...494
4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

[aaronlehmann]

Connection pooling should be HUGE. It would be great to get some informal benchmarks to see how much this helps.

I know I was cheerleading for a fork, but it looks like AdRoll/goamz is still active. I think we should contribute these changes upstream. If they don't want them, fine. And we still may need to maintain a fork for other reasons. But contributing the improvements is a nice thing to do.

[BrianBland]

I was also looking at the AWS-owned client (https://github.com/aws/aws-sdk-go) and it looks like there's tons more active development there than goamz. I could try a rewrite of the s3 driver using aws-sdk-go, which uses a persistent (and pluggable) *http.Client and has much more readable v4 signing code.

[BrianBland]

Updated to include docker/goamz#10 for fixed connection pooling

[aaronlehmann]

LGTM

@dmp42 @RichardScothern Should we consider this for Registry 2.3 because of the big performance improvement?

[RichardScothern]

I'm reluctant to. Although the unit tests show a big performance
improvement, I'm not convinced we have tuned MaxIdleConnsPerHost correctly.

Do we know what this is set to in the official go client?

On Fri, Jan 22, 2016 at 2:55 PM, Aaron Lehmann notifications@github.com
wrote:

LGTM

@dmp42 https://github.com/dmp42 @RichardScothern
https://github.com/RichardScothern Should we consider this for Registry
2.3 because of the big performance improvement?

—
Reply to this email directly or view it on GitHub
#1381 (comment).

[BrianBland]

@RichardScothern MaxIdleConnsPerHost does not exist anywhere in aws/aws-sdk-go, and it seems to just be using http.DefaultClient unless you pass your own in.

[stevvooe]

@RichardScothern @BrianBland We can do another release after 2.3 with the new S3 drivers, once it is warranted (2.4, perhaps). I believe its nice to release with each docker release, but we can always release more often. There are a few other big changes, such as the path fixup that will save on bandwidth (something like 10% of url keys, although not much in reality 😸 ).

[BrianBland]

@docker/distribution-maintainers can we merge this?

[dmcgowan]

LGTM

[dmp42]

@BrianBland at this point it's a matter of branch and release management.
@RichardScothern what would be the path forward here? Shall we merge this into a feature branch and converge to master after the release?

[RichardScothern]

We can merge to master and I will create a branch for the release

As a matter of interest, this is being tested in a container on prod with interesting results (see Noah for details)

[RichardScothern]

Before we do, are we ok with the user agent string @BrianBland ?

[BrianBland]

Good point, I'm going to update it to fmt.Sprintf("docker-distribution/%s %s", version.Version, runtime.Version()) unless someone has a better suggestion

[stevvooe]

LGTM

[aaronlehmann]

Is the plan to merge this first and then have a seprate PR that introduces an alternative s3 driver using the official SDK (which will probably become the one named "s3")?

[BrianBland]

Exactly, #1385 adds the alternative s3 driver and changes the name of this one to s3goamz

[aaronlehmann]

Minor cosmetic point, but I think this would be easier to read if broken over multiple lines, like this:

s3obj.Client = &http.Client{
    Transport: transport.NewTransport(http.DefaultTransport,
        transport.NewHeaderRequestModifier(http.Header{
            http.CanonicalHeaderKey("User-Agent"): []string{params.UserAgent},
        }),
    ),
}

[aaronlehmann]

LGTM

[BrianBland]

Updated for @aaronlehmann's style nit, merging on green CI