Adding credential caching for ECR #523

sentientmonkey · 2016-09-12T17:06:44Z

Summary

Adding ECR credential caching. This avoids having to get fetch new authorization tokens on each ECR image pull.

Implementation details

This adds an LRU expiring cache for each ECR Client. This also adds a new client wrapper for ECR client vs. using the SDK directly. This does not yet address early expiration for shorter use tokens (still TODO).

Testing

Builds (make release)
Unit tests (make short-test) pass
Integration tests (make test) pass
Functional tests (make run-functional-tests) pass

New tests cover the changes: unit & integration, but no functional tests yet. Need to add setup for ECR testing.

Description for the changelog

Enhancement - Add credential caching for ECR

Licensing

This contribution is under the terms of the Apache 2.0 License: yes

* Also adding testify to LICENSE * Also adding mocks for async.Cache

* Adding benchmark target * Updating factory * Updating GoDeps

aaithal · 2016-09-12T23:15:15Z

agent/ecr/client.go

+
+func (client *ecrClient) GetAuthorizationToken(registryId string) (*ecrapi.AuthorizationData, error) {
+	cachedAuthData, ok := client.tokenCache.Get(registryId)
+	if ok {


we still need to verify if the token hasn't expired here, yes?

Yes, if the current token has a shorter expiration than the TTL, we'll have to check to see if it's still valid here. Will add the check.

seems ExpiresAt in response is 12 hours.

There is delay to post the Token to ECR.
It needs to check if the token will not expire in few seconds.

Before using a cached token, we validate that it's not yet expired. This also early expires to handle request time and clock skew, as well adds some jitter to ensure that they don't request new tokens all at once.

aaithal

Looks good. Some minor comments.

aaithal · 2016-09-15T21:31:13Z

agent/ecr/client.go

+		aws.TimeValue(authData.ExpiresAt).Add(-1*client.expirationWindowWithJitter()).After(time.Now())
+}
+
+const (


nit: can you move this to the top of the file?

Sounds good - wasn't sure what we prefer for style on agent (const close to use or const at top of file)

To the top!

aaithal · 2016-09-15T21:31:52Z

agent/ecr/client.go

+
+// Ensure token is still within it's expiration window. We early expire to allow for timing in calls and add jitter to avoid
+// refreshing all of the tokens at once.
+func (client *ecrClient) isTokenValid(authData *ecrapi.AuthorizationData) bool {


can you also add unit tests just around this method?

samuelkarp · 2016-09-16T17:25:17Z

agent/ecr/client.go

+)
+
+// returns a random expiration duration between 1 and 5 minutes
+func (client *ecrClient) expirationWindowWithJitter() time.Duration {


You can get rid of this and just use utils.AddJitter.

Well that's much more clever. Will use that 😄

samuelkarp · 2016-09-16T17:25:54Z

agent/ecr/client.go

+			log.Debugf("Token found, but expires at %s", aws.TimeValue(cachedAuthData.ExpiresAt))
+		}
+	}
+


Might be good to add a log.Debug here showing that it's actually making a call to GetAuthorizationToken.

Makes sense - will add

samuelkarp · 2016-09-16T17:29:07Z

agent/ecr/client.go

+}
+
+const (
+	maxTokenExpirationWindowInSeconds = 300 // 5 minutes


Do you only want to cut 5 minutes off this? Might be worthwhile to set this closer to 15 or 20 minutes?

I was worried a bit about shorter lived tokens. I think the minimum for an MFA or STS token is 15 minutes. How about 10 minutes?

The ECS agent only runs on EC2 and typically uses the EC2 instance profile for credentials. While credentials can be specified through environment variables as well, those credentials cannot change for the lifetime of the agent. If you really want to be clever, you can detect whether the agent is using the EC2 instance profile and examine when those credentials expire by looking at the instance metadata service and checking the expiration time for those credentials (curl 169.254.169.254/latest/meta-data-/iam/security-credentials/$PROFILE_NAME).

Make sense - I think I'm okay giving a much larger window in that case, and we can change if we end up with use cases that are not running using the EC2 instance profile. I've moved to renewing within 30-60 minutes of credentials expiring (seems pretty reasonable).

* Using AddJitter * Adding test cases for IsTokenValid (also moved to public for testing) * Updated window to be from 30-60 minutes

sentientmonkey · 2016-09-20T00:27:11Z

I think I've addressed the outstanding feedback. Will likely create new PR with ECR integ tests - let me know if this looks good! 😄

samuelkarp · 2016-09-20T15:33:50Z

agent/ecr/client.go

+		}
+	}
+
+	log.Debugf("Calling GetAuthorizationToken for %q", registryId)


Any particular reason you picked %q instead of %s?

Just a minor preference - sometimes I find it more clear with quoted strings if they happen to be empty. Unlikely to happy, but I don't mind the extra quotes when there are there

Okay, no need to change it (unless someone else objects).

samuelkarp · 2016-09-20T15:35:13Z

agent/ecr/client.go

+		aws.TimeValue(authData.ExpiresAt).Add(-1*client.expirationWindowWithJitter()).After(time.Now())
+}
+
+func (client *ecrClient) expirationWindowWithJitter() time.Duration {


This name feels inaccurate to me since it doesn't include the existing expiration window. Can you change it to expirationWindowJitter?

Sure, will do

samuelkarp · 2016-09-20T15:38:00Z

agent/ecr/client.go

+func (client *ecrClient) IsTokenValid(authData *ecrapi.AuthorizationData) bool {
+	return authData != nil &&
+		authData.ExpiresAt != nil &&
+		aws.TimeValue(authData.ExpiresAt).Add(-1*client.expirationWindowWithJitter()).After(time.Now())


How about rewriting this so it's a bit more verbose (and maybe more readable)?

refreshTime := aws.TimeValue(authData.ExpiresAt).Add(-1*client.expirationWindowJitter()) return time.Now().Before(refreshTime)

(I also like using Before instead of After since that feels like a more natural question; you're not asking whether the expiration time is after now but whether now is before the expiration time.)

I can go either way on before/after. I really also wish time.Sub() took a time.Duration and not a time.Time 😞 . I agree on breaking it out to a multi-line for readability.

samuelkarp · 2016-09-20T15:44:27Z

agent/ecr/client.go

+)
+
+const (
+	minimumExpirationDuration = 30 * time.Minute


I think these are minimum/maximum jitter, not expiration duration.

I was thinking "minimum" and "maximum" expiration window (as in, the earliest and latest credentials could expire. I can change to "minimum/maximumJitterDuration" if that's clearer.

I'd prefer either minimumJitterDuration or minimumExpirationJitter over minimumExpirationDuration.

samuelkarp · 2016-09-20T15:45:04Z

agent/ecr/client_test.go

+		{-1 * time.Minute, false},
+		{time.Duration(0), false},
+		{1 * time.Minute, false},
+		{30 * time.Minute, false},


You can use the minimumExpirationJitter and maximumExpirationJitter constants here.

Those aren't exported (and I have to use ecr_test because of the mock circular dependency). Will export & use.

aaithal

Code looks fine. Just noticed some minor copyright header changes that i'd like to see.

aaithal · 2016-09-20T17:55:45Z

agent/async/generate_mocks.go

@@ -0,0 +1,16 @@
+// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved.


Should be 2014-2016

aaithal · 2016-09-20T17:56:37Z

agent/async/mocks/async_mocks.go

@@ -0,0 +1,62 @@
+// Copyright 2015-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved.


Might be an issue with the mock gen script. This should be 2014-2016.

Looks like this is set in mockgen.sh

amazon-ecs-agent/scripts/generate/mockgen.sh

Line 29 in a902b57

// Copyright 2015-${year} Amazon.com, Inc. or its affiliates. All Rights Reserved.

I can update mockgen.sh if you want - will mean I'm updating the older mocks as well to back date them.

I think 2015-${year} is right, since that's when we started doing mockgen.

yeah, 2015 seems right for this.

aaithal

thanks a lot for doing this!

aaithal · 2016-09-20T21:50:06Z

agent/async/mocks/async_mocks.go

@@ -0,0 +1,62 @@
+// Copyright 2015-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved.


yeah, 2015 seems right for this.

sentientmonkey · 2016-09-20T21:54:13Z

🎉 Should I merge or squash/merge?

aaithal · 2016-09-20T21:55:00Z

I usually 'squash and merge'.

sentientmonkey added 3 commits September 12, 2016 09:53

Adding LRU cache

be3f614

* Also adding testify to LICENSE * Also adding mocks for async.Cache

Adding ECRClient wrapper with token caching

ae6c6e1

* Adding benchmark target * Updating factory * Updating GoDeps

Vending testify

206f847

aaithal reviewed Sep 12, 2016
View reviewed changes

samuelkarp mentioned this pull request Sep 12, 2016

Cache the ECR authorizationToken #522

Closed

4 tasks

Adding cached token validation

659e430

Before using a cached token, we validate that it's not yet expired. This also early expires to handle request time and clock skew, as well adds some jitter to ensure that they don't request new tokens all at once.

bobzoller mentioned this pull request Sep 14, 2016

ThrottlingException #498

Closed

aaithal reviewed Sep 15, 2016

View reviewed changes

samuelkarp reviewed Sep 16, 2016

View reviewed changes

Fixing PR comments

498d16f

* Using AddJitter * Adding test cases for IsTokenValid (also moved to public for testing) * Updated window to be from 30-60 minutes

samuelkarp reviewed Sep 20, 2016

View reviewed changes

PR feedback round 2

6c459bc

aaithal requested changes Sep 20, 2016

View reviewed changes

samuelkarp approved these changes Sep 20, 2016

View reviewed changes

aaithal approved these changes Sep 20, 2016

View reviewed changes

sentientmonkey merged commit dba442c into aws:dev Sep 21, 2016

samuelkarp added this to the 1.13.0 milestone Sep 27, 2016

		@@ -0,0 +1,16 @@
		// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved.

		@@ -0,0 +1,62 @@
		// Copyright 2015-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved.

Adding credential caching for ECR #523

Adding credential caching for ECR #523

Conversation

sentientmonkey commented Sep 12, 2016

Summary

Implementation details

Testing

Description for the changelog

Licensing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaithal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaithal Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sentientmonkey commented Sep 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaithal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaithal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sentientmonkey commented Sep 20, 2016

aaithal commented Sep 20, 2016

aaithal Sep 15, 2016 •

edited

Loading