WIP: Added refresh to the metadata credentials. #251
Conversation
This also heavily restructures the module itself, which enables adding/removing credentials without altering module-level scope.
|
|
||
| """ |
There was a problem hiding this comment.
This commented block will get removed, just was keeping it around for reference that we were processing things in the correct order.
| # to calculate this ourselves. This is straight from the | ||
| # datetime docs. | ||
| day_in_seconds = delta.days * 24 * 3600 | ||
| micro_in_seconds = delta.microseconds * 10**6 |
There was a problem hiding this comment.
Shouldn't this be division?
There was a problem hiding this comment.
Yep. Poorly ported from Boto. Will fix & add a test.
|
|
||
|
|
||
| class UnknownCredentialError(BotoCoreError): | ||
| """Tried to insert before/after an unregister credential type.""" |
There was a problem hiding this comment.
Grammar. Maybe unregistered?
|
Post-talking with James, I'm going to further restructure this a bit, making the |
|
Is this intended to accommodate other temporary credentials besides IAM Roles deployed to an EC2 instance? What about the ability to use credentials from an I am finding the ability to do cross-account roles extremely important for teams with multiple AWS environments (e.g. dev, prod). Being able to create a single account in dev and then assume roles for specific activities in prod is a killer feature. Should I try to rewrite #226 to use this? Or will this do what #226 was trying to do? |
|
@garnaat Yes, the intent is to be able to support basically any type of credentials, even ones we haven't thought of yet. Those examples ( As for #226, I'm guilty of not having reviewed it before doing this work. The primary reason was just to get IAM refresh in-place, but it grew beyond those bounds. 😬 There's a lot of overlap between them. The upshot of the approach here is that we can make the credential changes specific to each IMO, I think the best would be updating #226 to build off of this (should actually just be deleting some of the code & shuffling other parts), because I think the temporary credentials would be useful to have. I would hold off on that until this is finalized though, as I'm rearchitecting bits (which should make the rewrite easier). Sorry for the pain & more than willing to change this further so we're all happy with it. |
|
I'm not particularly concerned with #226. It's kind of kludgy and I wasn't really that happy with it. I am, however, really determined to get the AssumeRole functionality (at least) enabled because I think it is a really important feature. Probably more so for the CLI but we need the the core functionality here. I will try to rethink #226 in the context of this. Thanks. |
|
Alright, I pushed an updated version of this. Changes:
I think this is done (relatively happy with it), but would be happy to incorporate feedback/changes & write docs for it. Review again please? @jamesls @danielgtaylor @garnaat |
|
LGTM, but I'd wait and see if others have any feedback. |
|
|
||
| def __init__(self, access_key=None, secret_key=None, token=None): | ||
| def __init__(self, access_key=None, secret_key=None, token=None, |
There was a problem hiding this comment.
Is there ever a case where we should allow access_key/secret_key to be None? Seems like these should be required params.
There was a problem hiding this comment.
It was done for back-compat - https://github.com/boto/botocore/blob/develop/botocore/credentials.py#L36-L54.
There was a problem hiding this comment.
Sorry, hit "Comment" too soon. I'm open to changing it, I just did that to be consistent.
It's possible you might instantiate temporary credentials on their own, then wait to populate them until they're needed (to lazy-load). That's about the only valid use I can see.
There was a problem hiding this comment.
Do we cover the "anonymous S3 connection" use case from boto in this?
|
I think this looks good, I've made just a few small suggestions. |
This also heavily restructures the module itself, which enables adding/removing credentials without altering module-level scope. The behavior should be backward-compatible, though if anyone was importing the functions (or altering the global list of credentials), that will break.
At this time, there's one failing test & I'm planning on adding tests for the
CredentialResolver, but I'd like to get some early-ish feedback on the approach. Feedback is most welcome. I'd like to think this is a good change, but would be happy to fix/change any part of it.Review please? @jamesls @danielgtaylor @garnaat