Support env var credential expiry#1187
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1187 +/- ##
===========================================
+ Coverage 98% 98.05% +0.04%
===========================================
Files 45 45
Lines 7277 7306 +29
===========================================
+ Hits 7132 7164 +32
+ Misses 145 142 -3
Continue to review full report at Codecov.
|
kyleknap
left a comment
There was a problem hiding this comment.
Looks good. Just had a question and a comment.
tests/unit/test_credentials.py
Outdated
There was a problem hiding this comment.
One more test that might be good to add is one that checks that it does not refresh the credentials when it is not going to expire any time soon.
botocore/credentials.py
Outdated
There was a problem hiding this comment.
So for the very first time you load credentials and you do not have an expiry time set, it will not ever try to refresh if it you add the expiry time variable later on? I'm just curious about the behavior. Have not really thought out what should be desired.
There was a problem hiding this comment.
Correct. I'm not sure how else we would do it without hacks
There was a problem hiding this comment.
Yeah I cannot think of other ways to do this. I guess another question I have is it possible to transition from refershable environment credentials to non-refreshable? So say I had an expiry time set and then unset the expiry time variable before the next refresh what would happen?
There was a problem hiding this comment.
It would fail as it expects it to be there upon refresh
There was a problem hiding this comment.
Ok cool. That at least ensures the behavior is consistent for both directions (i.e. you can't do it).
There was a problem hiding this comment.
I think it would good to have tests for these cases (fixed->refreshable, refreshable->fixed are not allowed). Unlike other refreshable providers were it was unlikely to have missing keys across subsequent responses, this is the first refreshable provider that seems more prone to refresh errors.
|
Updated |
3b8d18b to
1a12887
Compare
botocore/credentials.py
Outdated
There was a problem hiding this comment.
I don't think we should rely on no expiry time meaning non-refreshable credentials. I would prefer we're explicit. If we get expiry time use RefreshableCredentials, otherwise use Credentials.
I'm guessing that code was put in place to protect against malformed responses and not necessarily as part of its contract. It's always confusing to have RefreshableCredentials not actually refresh if you don't provide expiry.
8992935 to
725ebd8
Compare
|
How should this feature be used? how would boto refresh the credentials once it finds they've expired? |
This adds support for environment credential expiration via the
AWS_CREDENTIAL_EXPIRATIONenvironment variable.