feat: refactor AWS and identity pool credentials to use suppliers #1484
Conversation
|
Please fix lint |
| credentials.get("AccessKeyId"), | ||
| credentials.get("SecretAccessKey"), | ||
| credentials.get("Token"), |
There was a problem hiding this comment.
Should there be any type of validation here? Is it guaranteed that the credential variable has these keys?
There was a problem hiding this comment.
yes, the response we get back will have at least accessKeyId and SecretAccesskey, and token is optional. This code and the rest of the comments on the AWS credential aren't actually changed at all btw, just refactored and moved around a bit to support the supplier pattern we are using for programmatic. Since this isn't actually getting changed and won't cause a regression, are you OK if I pull all your suggestions into a to-do bug to go and do some of these fixes in a different PR targeted at that?
There was a problem hiding this comment.
I am okay with you tracking non-regressions in a separate bug and following up with small PRs if you prefer.
| } | ||
| def _get_imdsv2_session_token(self, request): | ||
| if request is not None and self._imdsv2_session_token_url is not None: | ||
| headers = {"X-aws-ec2-metadata-token-ttl-seconds": "300"} |
There was a problem hiding this comment.
Why 300? Can you make it a self-documenting variable.
| credentials = self._get_metadata_security_credentials( | ||
| request, role_name, imdsv2_session_token | ||
| ) | ||
| if imdsv2_session_token_response.status != 200: |
There was a problem hiding this comment.
Use a http status object instead of a magic number. See transport/__init__.py for examples.
| "Unable to retrieve AWS Session Token", | ||
| imdsv2_session_token_response.data, |
There was a problem hiding this comment.
Should this string be formatted?
| audience (str): The STS audience field. | ||
| subject_token_type (str): The subject token type. | ||
| token_url (str): The STS endpoint URL. | ||
| credential_source (Mapping): The credential source dictionary used |
There was a problem hiding this comment.
This documentation is missing how to build a credential source dict
| ) | ||
|
|
||
| # Get the environment ID. Currently, only one version supported (v1). | ||
| matches = re.match(r"^(aws)([\d]+)$", environment_id) |
There was a problem hiding this comment.
Add a comment with an example string that this regex is matching.
| raise exceptions.InvalidResource( | ||
| "No valid AWS 'credential_source' provided" | ||
| ) | ||
| elif int(env_version or "") != 1: |
There was a problem hiding this comment.
int("") will result in a ValueError being raised.
| # Serialize AWS signed request. | ||
| # Keeping inner keys in sorted order makes testing easier for Python | ||
| # versions <=3.5 as the stringified JSON string would have a predictable | ||
| # key order. |
There was a problem hiding this comment.
Can the tests be rewritten to not rely on order? This does not seem like a good reason to sort the data.
There was a problem hiding this comment.
I would like to follow up on this. It would be best to avoid sorting this data twice.
| if not os.path.exists(self._path): | ||
| raise exceptions.RefreshError("File '{}' was not found.".format(self._path)) | ||
|
|
||
| with io.open(self._path, "r", encoding="utf-8") as file_obj: |
There was a problem hiding this comment.
nit: open is a builtin, you don't need to use the io module.
clundin25
left a comment
clundin25
left a comment
There was a problem hiding this comment.
Additional follow up tracked in b/328671918
…entials (#1496) * feat: refactor AWS and identity pool credentials to use suppliers (#1484) * feat: refactor aws and identity pool credentials to use supplier framework * Linting * changing class types * linting * remove unused import * Fix typing * add docstring and fix casing * feat: Adds support for custom suppliers in AWS and Identity Pool credential instantiation (#1494) * feat: refactor aws and identity pool credentials to use supplier framework * Linting * changing class types * linting * remove unused import * Fix typing * add docstring and fix casing * feat: adds support for passing suppliers to credentials. * fixes merge issues and adds _has_custom_supplier method * adds _has_custom_supplier function to identity_pool * Update google/auth/external_account.py Co-authored-by: Carl Lundin <[email protected]> * Apply suggestions from code review Co-authored-by: Carl Lundin <[email protected]> * Respond to comments and fix docs --------- Co-authored-by: Carl Lundin <[email protected]> * docs: add documentation for suppliers (#1495) * docs: update docs for programmatic * add space * update user guide * update docs * Apply suggestions from code review Co-authored-by: Leo <[email protected]> * Update docs * Add docs about context and request --------- Co-authored-by: Carl Lundin <[email protected]> Co-authored-by: Leo <[email protected]>
5 participants
No description provided.