CSHARP-4273: Cache AWS Credentials Where Possible. #894
Conversation
DmitryLukyanov
requested review from
BorisDog,
JamesKovacs and
blink1073
and removed request for
BorisDog
| 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
| - 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.
JamesKovacs
left a comment
JamesKovacs
left a comment
There was a problem hiding this comment.
Some minor comments to address but overall looking good.
| { | ||
| 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
| return CreateAwsCreadentialsFromAwsResponse(response); | ||
| } | ||
|
|
||
| private AwsCredentials CreateAwsCreadentialsFromAwsResponse(string awsResponse) |
There was a problem hiding this comment.
Spelling:
CreateAwsCreadentialsFromAwsResponse => CreateAwsCredentialsFromAwsResponse
| public AwsCredentials(string accessKeyId, SecureString secretAccessKey, string sessionToken, string expiration) | ||
| { | ||
| _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 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.
3 participants
Some notes: