Instance list cache #33
Conversation
|
Hi, thanks for the PR. I don't want this to be the default behaviour, as it could be very confusing for users. If anything the cache should be an explicit argument (e.g. Also please check the latest release from the master branch to see if the speed has improved for you. We now no longer fetch all the records but instead only the active ones, it may help. |
|
Thank you for your feedback. The reason I didn't use the I'll merge your changes to my branch and update the cache behaviour. |
|
Hi again, one more thing - the cache should probably be aware of the account id and region. It makes sense to cache the records for each acct-id+region pair. |
|
I made all the recommended changes, seems to work as requested now. |
There was a problem hiding this comment.
Thanks for the patch. I have added some comments, nothing major. The support should also be added to ec2-ssh and to ssm-tunnel - it's using the same reolver, just the argument parsing and the logic around allowed argument combination should be added. Perhaps externalise the arguments to ssm_tools/common.py and create add_cache_parameters() function?
Also we now have type hints and black formatting, you can use ./check.sh to make sure that all the linting checks pass.
| @@ -18,21 +21,84 @@ def __init__(self, args): | |||
| # Construct boto3 session with MFA cache | |||
| self.session = boto3.session.Session(profile_name=args.profile, region_name=args.region) | |||
| self.session._session.get_component('credential_provider').get_provider('assume-role').cache = botocore.credentials.JSONFileCache(cli_cache) | |||
|
|
|||
| self.account_region = self.session.region_name | |||
| self.account_id = self.session.resource('iam').CurrentUser().user_id | |||
There was a problem hiding this comment.
This should be session.client('sts').get_caller_identity()['Account'] to make it work with Users as well as EC2 IAM Roles.
| @@ -18,21 +21,84 @@ def __init__(self, args): | |||
| # Construct boto3 session with MFA cache | |||
| self.session = boto3.session.Session(profile_name=args.profile, region_name=args.region) | |||
| self.session._session.get_component('credential_provider').get_provider('assume-role').cache = botocore.credentials.JSONFileCache(cli_cache) | |||
|
|
|||
| self.account_region = self.session.region_name | |||
There was a problem hiding this comment.
What if region_name is not set, e.g. when using EC2 IAM Role?
|
|
||
| if self.use_cache: | ||
| if self.cache_file is None: | ||
| self.instance_cache = os.path.join(self.cache_dir, f'ssm_instances_{self.account_id}_{self.account_region}') |
| self.instance_cache = self.cache_file | ||
|
|
||
| def get_list_cache(self): | ||
| if not os.path.exists(self.instance_cache): |
|
|
||
| if time.time() - os.path.getmtime(self.instance_cache) > self.RESOLVER_CACHE_DURATION: | ||
| logger.debug('Did not load cache because file is older that one day') | ||
| return None |
There was a problem hiding this comment.
Again, use pathlib. Also should the cache file be deleted in this case?
| if bool(args.INSTANCE) + bool(args.list) != 1: | ||
| # Require exactly one of INSTANCE or --list and allow to do only cache update | ||
| if bool(args.INSTANCE) and bool(args.list) or \ | ||
| not args.update_cache and not bool(args.INSTANCE) and not bool(args.list): |
There was a problem hiding this comment.
How about:
if bool(args.INSTANCE) + bool(args.list) + bool(args.update_cache) != 1:
parser.error("Specify either INSTANCE or --list or --update-cache")
The ssm resolver as it is implemented now needs to fetch all the instances from AWS in every connection attempt. I thought it would faster to store the response in a temporary location for caching to speed up consecutive requests.
The cache lasts for one day (
RESOLVER_CACHE_DURATION) and there is a flag--update-cachethat can be used to trigger a cache update.