Skip to content
This repository was archived by the owner on May 12, 2026. It is now read-only.

Add caching for JWT tokens - #151

Merged
lesv merged 4 commits into
googleapis:masterfrom
igorbernstein2:jwt-caching
Apr 2, 2018
Merged

Add caching for JWT tokens#151
lesv merged 4 commits into
googleapis:masterfrom
igorbernstein2:jwt-caching

Conversation

@igorbernstein2

@igorbernstein2 igorbernstein2 commented Mar 20, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Use guava's LoadingCache to cache JWT tokens. This should address performance for both clientside & serverside:

  • Avoid the need for the client to recompute the signature
  • Allows the serverside to cache tokens

Benchmarks (n1-standard-4):

  • Generating a JWT token takes about 7~8ms (p50) on an idle machine
  • Additionally, the newly generated tokens will add 9ms to p99 processing on serverside (for Cloud Bigtable)
  • For reference, Cloud Bigtable has a 5ms response @ p99 using cached JWTs or OAuth tokens.
  • Combined, the clientside & serverside overhead for using uncached JWT tokens triples Cloud Bigtable's p99 latency.

Merging this PR will have a significant impact on the consistency of Cloud Bigtable's performance (and other low latency products as well).

@igorbernstein2

igorbernstein2 commented Mar 20, 2018

Copy link
Copy Markdown
Contributor Author

@garrettjonesgoogle PTAL when you have moment

garrettjonesgoogle
garrettjonesgoogle reviewed Mar 20, 2018
View reviewed changes
Comment thread oauth2_http/java/com/google/auth/oauth2/ServiceAccountJwtAccessCredentials.java
throw new IllegalStateException("generateJwtAccess threw an unchecked exception that couldn't be rethrown", e);
}
}
private String generateJwtAccess(URI uri) throws IOException {

This comment was marked as spam.

Sign in to view

@garrettjonesgoogle
garrettjonesgoogle requested a review from lesv March 20, 2018 17:40
@garrettjonesgoogle

garrettjonesgoogle commented Mar 20, 2018

Copy link
Copy Markdown
Contributor

@lesv could you take a look too?

@igorbernstein2

igorbernstein2 commented Mar 21, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Updated:

  • add tests
  • fixed whitespace

PTAL

@igorbernstein2 igorbernstein2 mentioned this pull request Mar 21, 2018
Merged
@garrettjonesgoogle

garrettjonesgoogle commented Mar 21, 2018

Copy link
Copy Markdown
Contributor

Oh I was wanting to do the snapshot bump in a separate PR that gets committed before this one...

@igorbernstein2

igorbernstein2 commented Mar 21, 2018

Copy link
Copy Markdown
Contributor Author

Oops, sorry I misunderstood. I cherrypicked the version bump to a separate PR:
#152

lesv
lesv reviewed Mar 21, 2018
View reviewed changes

@lesv lesv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick look, I think you might wish to refresh at 30-60 seconds 'til instead of 10. I don't see much else thats obvious. I'm going to look again before I give an approval.

Comment thread oauth2_http/java/com/google/auth/oauth2/ServiceAccountJwtAccessCredentials.java Outdated
private LoadingCache<URI, String> createCache() {
return CacheBuilder.newBuilder()
.maximumSize(100)
.expireAfterWrite(LIFE_SPAN_SECS - 10, TimeUnit.SECONDS)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

lesv
lesv suggested changes Mar 21, 2018
View reviewed changes

@lesv lesv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL - I'm just not finding how tokenCache is being set by fromStream(credentialsStream) flow.

Comment thread oauth2_http/java/com/google/auth/oauth2/ServiceAccountJwtAccessCredentials.java
this.privateKey = Preconditions.checkNotNull(privateKey);
this.privateKeyId = privateKeyId;
this.defaultAudience = defaultAudience;
this.tokenCache = createCache();

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread oauth2_http/java/com/google/auth/oauth2/ServiceAccountJwtAccessCredentials.java
@@ -228,6 +241,28 @@ public static ServiceAccountJwtAccessCredentials fromStream(InputStream credenti
+ " Expecting '%s'.", fileType, SERVICE_ACCOUNT_FILE_TYPE));
}

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@igorbernstein2

igorbernstein2 commented Mar 23, 2018

Copy link
Copy Markdown
Contributor Author

@lesv PTAL

garrettjonesgoogle
garrettjonesgoogle approved these changes Apr 2, 2018
View reviewed changes

@garrettjonesgoogle garrettjonesgoogle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lesv
lesv merged commit 664754e into googleapis:master Apr 2, 2018
@j256 j256 mentioned this pull request Apr 26, 2018
Closed
@chingor13 chingor13 mentioned this pull request Jul 12, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@lesv lesv lesv approved these changes
@garrettjonesgoogle garrettjonesgoogle garrettjonesgoogle approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@igorbernstein2 @garrettjonesgoogle @lesv