CSHARP-4273: Cache AWS Credentials Where Possible. #894
Conversation
DmitryLukyanov
requested review from
JamesKovacs,
BorisDog and
blink1073
and removed request for
BorisDog
| @@ -25,20 +25,27 @@ namespace MongoDB.Driver.Core.Authentication.External | |||
| { | |||
| internal class AwsCredentials : IExternalCredentials | |||
| { | |||
| // credentials are considered expired when: Expiration - now > 5 mins | |||
| private static readonly TimeSpan __overlapWhenExpired = TimeSpan.FromMinutes(5); | |||
There was a problem hiding this comment.
This value was suggested to change up to 5 mins. cc: @blink1073
DmitryLukyanov
force-pushed
the
csharp4273_2
branch
from
c17dfc5 to
0bb467a
Compare
| @@ -987,7 +988,7 @@ tasks: | |||
| - func: run-aws-auth-test-with-aws-credentials-as-environment-variables | |||
| - func: run-aws-auth-test-with-aws-credentials-and-session-token-as-environment-variables | |||
| - func: run-aws-auth-test-with-aws-EC2-credentials | |||
| # ECS test is skipped untill testing on Linux becomes possible | |||
| # ECS test is skipped until testing on Linux becomes possible | |||
There was a problem hiding this comment.
| @@ -25,20 +25,27 @@ namespace MongoDB.Driver.Core.Authentication.External | |||
| { | |||
| internal class AwsCredentials : IExternalCredentials | |||
| { | |||
| // credentials are considered expired when: Expiration - now > 5 mins | |||
There was a problem hiding this comment.
Shouldn't this be Expiration - now < 5 mins? For example if credentials expire at noon and it's currently 11:50am, noon - 11:50am = 10 minutes and credentials don't need to be refreshed yet. But at 11:55am, noon - 11:55am = 5 minutes and credentials need refreshing.
Note that this only affects the comment and not the implemented logic, which uses Expiration - now < 5 mins.
There was a problem hiding this comment.
yeah, it's typo, fixed
| @@ -116,12 +123,23 @@ private async Task<AwsCredentials> CreateAwsCredentialsFromEc2ResponseAsync(Canc | |||
|
|
|||
| private AwsCredentials CreateAwsCreadentialsFromAwsResponse(string awsResponse) | |||
There was a problem hiding this comment.
Spelling:
CreateAwsCreadentialsFromAwsResponse => CreateAwsCredentialsFromAwsResponse
| { | ||
| _accessKeyId = Ensure.IsNotNull(accessKeyId, nameof(accessKeyId)); | ||
| _expiration = expiration != null ? DateTime.Parse(expiration) : null; |
There was a problem hiding this comment.
The constructor should take the expiration as a DateTime?. See CreateAwsCredentialsFromAwsResponse below.
| var parsedResponse = BsonDocument.Parse(awsResponse); | ||
| var accessKeyId = parsedResponse.GetValue("AccessKeyId", null)?.AsString; | ||
| var secretAccessKey = parsedResponse.GetValue("SecretAccessKey", null)?.AsString; | ||
| var sessionToken = parsedResponse.GetValue("Token", null)?.AsString; | ||
| var expiration = parsedResponse.GetValue("Expiration", null)?.AsString; |
There was a problem hiding this comment.
We should do any input validation here in the factory method rather than delegating to the constructor. For example if the Expiration isn't a valid DateTime and fails parsing for any reason, it is better to throw in this factory method rather than in the constructor. We should also consider using DateTime.TryParse and how to best handle an error where the KMS sends back an invalid DateTime.
| { | ||
| if (expiration != null) | ||
| { | ||
| throw new InvalidOperationException($"Expiration in AWS response is in invalid datetime format: {expiration}."); |
There was a problem hiding this comment.
open to suggestions about error message
| // 3. Ensure that a find operation adds credentials to the cache.. | ||
| GetCache().Should().NotBeNull(); | ||
|
|
||
| // 4. Override the cached credentials with an "Expiration" that is within thirty seconds of the current UTC time. |
There was a problem hiding this comment.
I updated mongodb/specifications#1281, with one tweak: this should be one minute.
CSHARP-4273: Cache AWS Credentials Where Possible.
Some notes: