-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix IAM token expiration #234
Conversation
It's really annoying that my solution has to access internal members of botocore classes (hence the |
Hi! Looks good I think. What do you think about splitting the IAM and "regular" logic? Right now it's a bit tangled. I'm thinking of something like: def init_snippet():
# first part of the __init__ method
if iam_auth:
self.s3 = iam_auth()
return # or `if self.s3 is not None: return` to try fallback, like the current code does
if access_key is None or secret_key is None:
raise RuntimeError(
"Attempting to get S3 client without an access_key/secret_key set"
)
self.s3 = boto3.Session().client(
"s3",
endpoint_url=endpoint,
aws_access_key_id=access_key,
aws_secret_access_key=secret_key,
)
def iam_auth():
boto_session = get_session()
iam_providers = [
ContainerProvider(),
InstanceMetadataProvider(
iam_role_fetcher=InstanceMetadataFetcher(
timeout=1000, num_attempts=2
)
),
]
for provider in iam_providers:
creds = provider.load()
if creds:
boto_session._credentials = creds # type: ignore
return boto3.Session(botocore_session=boto_session).client(
"s3",
endpoint_url=endpoint,
) (not tested ofc, but I think it's significantly easier to understand) |
Btw, similar code is used in MWDB, so when we agree on final solution I'll be happy to apply the change to mwdb-core as well: https://github.com/CERT-Polska/mwdb-core/blob/master/mwdb/core/util.py#L164 |
Sure, if you guys think this is cleaner I have no real objection :) I do want to note however, that during some reading in the botocore repo I noticed they have some function called So instead of selecting a few providers (in our case 2), we can call Edit: this^ will require some more thinking and work however. The implementation proposed in the PR tested to be working as is. |
Co-authored-by: Michał Praszmo <[email protected]>
Since botocore doesn't have typings by default, this will not work unless we settle on something less specific than S3 client or add typing stubs to dependencies. In my opinion it is not worth it for this small method
This PR solves the issue described in #233. Botocore's
*Provider
classes such asContainerProvider
return aRefreshableCredentials
object when calling theirload
method. This object knows when and how to refresh the session token, so there's no need to implement custom code to do that. In current code, this object is thrown away so it gets no chance to auto-refresh the token.Instead, use a lower level session object and if possible, attach this
RefreshableCredentials
object to it, and create the s3 client from it.Tested on MWDB v2.10.2 with a karton that uses v5.3.0 of karton-core with the diff in this PR patched into it and seems to still work even a couple of hours after the karton is up (whereas the current code fails). Kept as draft until I'm sure it solves the issue
Fixes #233