ECR auth: multi-region support

[Flygsand]

Since ECR has launched in additional regions, I've added support by passing a factory function to NewECRAuthProvider.

This also addresses a stupid oversight with my original implementation, when running Empire in a region different from the ECR image it is trying to pull.

[Flygsand]

Alright, hold this for a bit. I'm seeing authorization errors after deploying this to our staging environment.

EDIT: latest changes weren't pulled. Works fine now.

[Flygsand]

Perhaps it's smarter to close over an instance of ConfigProvider, rather than construct it every time the factory is called. Looks like the AWS SDK will create a copy of the session, which should make it safe to do so.

[ejholmes]

You could also just close over a map so that the ecr clients are memoized. Something like:

var clients map[region]*ecr.ECR
provider.AddProvider(dockerauth.NewECRAuthProvider(func(region string) dockerauth.ECR {
        var client *ecr.ECR
        client, ok := clients[region]
        if !ok {
                client = ecr.New(newConfigProvider(c), &aws.Config{Region: aws.String(region)})
                clients[region] = client
        }
        return client
}

Although, it's probably fine to instantiate a new client every time.

[Flygsand]

If wonder if by doing memoization here you introduce thread-safety concerns. According to the docs, http.Serve runs each request in its own goroutine, and I don't think Go makes any atomicity guarantees in most cases.

I did a bit of benchmarking on session creation vs copying:

func BenchmarkAwsSessionCreate(b *testing.B) {
  var svc *ecr.ECR

  for i := 0; i < b.N; i++ {
    svc = ecr.New(session.New(), &aws.Config{Region: aws.String("us-east-1")})
  }

  _ = svc
}

func BenchmarkAwsSessionCopy(b *testing.B) {
  sess := session.New()
  var svc *ecr.ECR

  for i := 0; i < b.N; i++ {
    svc = ecr.New(sess, &aws.Config{Region: aws.String("us-east-1")})
  }

  _ = svc
}

BenchmarkAwsSessionCreate-8   200000          9235 ns/op
BenchmarkAwsSessionCopy-8     300000          5401 ns/op

So I think I'll move it out for now.

[ejholmes]

Nice catch! This looks great. I had one comment, if you want to make that change. Otherwise, this looks good. Do you mind updating the CHANGELOG as well?

[Flygsand]

Ok, moved session creation out of the factory and added a row to "Bugs" in the CHANGELOG.

[ejholmes]

Awesome. Thanks!

[mcansky]

is there a specific documentation section on the use of these changes ?

[phobologic]

@protomouse any chance you could provide some documentation for using ECR? It's catching a few people up, and quite honestly I haven't used it before - digging into the code, but it's not quite clear what changes need to be made in config to use ECR so far.

[Flygsand]

@phobologic Sure. See #1034!