Skip to content
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

Specify botocore config for all boto clients #1339

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

achantavy
Copy link
Contributor

Summary

Describe your changes.

  • Updates all boto3 client instantiations to use the same config object so that they all have the same retry and timeout behavior. This will improve reliability of AWS syncs across the board.
  • Moves get_botocore_config() from cartography.intel.aws.ec2.util to cartography.intel.aws.util.common.
  • Updates imports of get_botocore_config() to be explicit.

@chandanchowdhury chandanchowdhury added maintenance Care and feeding of this software project AWS Related to cartography's AWS module labels Jul 25, 2024
@chandanchowdhury
Copy link
Collaborator

From the documentation, it seems users should be able to use environment variables or ~/.aws/config
https://boto3.amazonaws.com/v1/documentation/api/latest/guide/configuration.html#guide-configuration

Broader question:

  1. Do we really need to override those configs instead of suggesting those values in doc and let the user set those as they prefer?
  2. Should we use a common function (say get_botocore_client() in cartography/intel/aws/util/boto3.py) to get the client so we can centralize the client creation process in case we need similar client-wide change in future?

@achantavy
Copy link
Contributor Author

Good points. I'll reply in reverse:

Should we use a common function (say get_botocore_client() in cartography/intel/aws/util/boto3.py) to get the client so we can centralize the client creation process in case we need similar client-wide change in future?

If we choose to override botocore, then yes we should centralize it in that one place.

Do we really need to override those configs instead of suggesting those values in doc and let the user set those as they prefer?

This is a hard one. I can see it either way. On the one hand, we know that AWS APIs can be flaky in a way that overriding the boto3 defaults can be helpful with cartography, so for that reason cartography users will benefit from us picking those defaults. But on the other hand, doing this goes against how cartography acts as sort of a 'pass through' for boto3 configuration like how we rely on boto3's search order for finding credentials.

I'm not sure what way to go.

@chandanchowdhury
Copy link
Collaborator

As we are currently overriding some botocore configs, lets continue doing so to avoid any sudden disruption.
Lets centralize it so that it is easy to document to tell users how to change the overrides and also in case we decide not to override in distant future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AWS Related to cartography's AWS module maintenance Care and feeding of this software project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants