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

Allow sam local invoke to retrieve account id from current logged in session #7013

Merged
merged 27 commits into from
Jun 11, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
61a5c5f
Allow AWS sam local invoke to retrieve account id from current logged…
kevin-james-sp May 2, 2024
9605545
Moved account id code to separate function
kevin-james-sp May 3, 2024
c7732ac
Merge branch 'develop' into develop
mildaniel May 6, 2024
8ef323f
Update samcli/commands/local/cli_common/invoke_context.py
defenderkev May 7, 2024
6a39e1d
Update samcli/commands/local/cli_common/invoke_context.py
defenderkev May 7, 2024
fb33fbe
Requested changes
kevin-james-sp May 7, 2024
77543bf
unit tests
kevin-james-sp May 7, 2024
fe8ace6
Update tests/unit/commands/local/cli_common/test_invoke_context.py
defenderkev May 7, 2024
f2ee4a2
put docstring in
kevin-james-sp May 7, 2024
8da5919
requested assertion changes
kevin-james-sp May 7, 2024
0c6d025
Fix changes requested by @hawflau
defenderkev May 20, 2024
f412c84
Merge branch 'develop' into develop
defenderkev May 20, 2024
290c556
missed removing a debugging statement
defenderkev May 24, 2024
b3c5831
add cmd line params to client_provider init
defenderkev May 24, 2024
f046969
Merge branch 'develop' into develop
defenderkev May 24, 2024
c2543ce
import get boto client method directly
defenderkev May 28, 2024
3ec9847
Changed some existing tests, as the new code now means any profile sp…
defenderkev May 28, 2024
6190ad2
Merge branch 'develop' into develop
mildaniel May 28, 2024
40ee4e2
Added return type annotation
defenderkev May 28, 2024
29f539c
Merge branch 'develop' of github-as-defenderkev:defenderkev/aws-sam-c…
defenderkev May 28, 2024
e7061b3
catch another Exception type, as `make pr` was failing when no creden…
defenderkev May 28, 2024
5e7c72e
Merge branch 'develop' into develop
defenderkev May 30, 2024
dafa4c8
Merge branch 'develop' into develop
jysheng123 May 30, 2024
90bca1a
Add ClientError catch and update some of the tests
defenderkev Jun 3, 2024
1740627
Fixed the last tests - mocked out the new account_id code as it's not…
defenderkev Jun 5, 2024
35b0a22
Merge branch 'develop' into develop
hnnasit Jun 10, 2024
74d8852
Merge branch 'develop' into develop
lucashuy Jun 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 24 additions & 0 deletions samcli/commands/local/cli_common/invoke_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@
from pathlib import Path
from typing import Any, Dict, List, Optional, TextIO, Tuple, Type, cast

from botocore.exceptions import TokenRetrievalError

import samcli.lib.utils.boto_utils
from samcli.commands._utils.template import TemplateFailedParsingException, TemplateNotFoundException
from samcli.commands.exceptions import ContainersInitializationException
from samcli.commands.local.cli_common.user_exceptions import DebugContextException, InvokeContextException
Expand Down Expand Up @@ -173,6 +176,7 @@ def __init__(
if aws_region:
self._global_parameter_overrides = {"AWS::Region": aws_region}

self._add_account_id_to_global()
self._layer_cache_basedir = layer_cache_basedir
self._force_image_build = force_image_build
self._aws_region = aws_region
Expand Down Expand Up @@ -345,6 +349,25 @@ def _clean_running_containers_and_related_resources(self) -> None:
cast(WarmLambdaRuntime, self.lambda_runtime).clean_running_containers_and_related_resources()
cast(RefreshableSamFunctionProvider, self._function_provider).stop_observer()

def _add_account_id_to_global(self):
"""
Attempts to get the Account ID from the current session
If there is no current session, the standard parameter override for
AWS::AccountId is used
"""
client_provider = samcli.lib.utils.boto_utils.get_boto_client_provider_with_config()
defenderkev marked this conversation as resolved.
Show resolved Hide resolved

sts = client_provider("sts")

try:
account_id = sts.get_caller_identity().get("Account")
if account_id:
if self._global_parameter_overrides is None:
self._global_parameter_overrides = {}
self._global_parameter_overrides["AWS::AccountId"] = account_id
except TokenRetrievalError:
defenderkev marked this conversation as resolved.
Show resolved Hide resolved
LOG.warning("No current session found, using default AWS::AccountId")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this function is ok to error, can you also add a generic exception handling right now? If theres no exception handling here, if an error occurs outside of the 2 defined errors, there is potential for this function to panic the execution

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I caatch these two exceptions because they can be the consequences of

  • Not having a ~/.aws/credentials or ~/.aws/config file
  • Valid configuration but token is expired
    If anything else causes an error, we don't know what's caused it and in my opinion the execution should panic because we don't know how to handle it.
    Thoughts? I mean, I'm happy to put in a generic except and continue but I'm not sure if that's what we should do here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to consider catching the ClientError exception here too. Using expired credentials, botocore returns:

botocore.exceptions.ClientError: An error occurred (ExpiredToken) when calling the GetCallerIdentity operation: The security token included in the request is expired

You had mentioned that other tests start failing once the ClientError was caught, maybe we could look at fixing those?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lucashuy What I've done is add catching ClientError to the new method, and patching the new method for the existing tests so it's just a stub; they don't need a live connection so inserting a live account ID is irrelevant. All tests are passing now
I also put back in some definitions of aws_profile I had taken out of some of those tests. Now the only differences to upstream are the patching of _add_account_id_to_global and the extra tests for the new code.

@property
def function_identifier(self) -> str:
"""
Expand Down Expand Up @@ -472,6 +495,7 @@ def _is_debugging(self) -> bool:

def _get_stacks(self) -> List[Stack]:
try:
print(SamLocalStackProvider.get_stacks)
defenderkev marked this conversation as resolved.
Show resolved Hide resolved
stacks, _ = SamLocalStackProvider.get_stacks(
self._template_file,
parameter_overrides=self._parameter_overrides,
Expand Down
16 changes: 16 additions & 0 deletions tests/unit/commands/local/cli_common/test_invoke_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -1360,3 +1360,19 @@ def test_must_pass_custom_region(self, get_stacks_mock):
get_stacks_mock.assert_called_with(
"template_file", parameter_overrides=None, global_parameter_overrides={"AWS::Region": "my-custom-region"}
)


class TestInvokeContext_add_account_id_to_global(TestCase):
def test_must_work_with_no_token(self):
invoke_context = InvokeContext("template_file")
invoke_context._add_account_id_to_global()
self.assertIsNone(invoke_context._global_parameter_overrides)

@patch("samcli.lib.utils.boto_utils.get_boto_client_provider_with_config")
def test_must_work_with_token(self, get_boto_client_provider_with_config_mock):
get_boto_client_provider_with_config_mock.return_value.return_value.get_caller_identity.return_value.get.return_value = (
"210987654321"
)
invoke_context = InvokeContext("template_file")
invoke_context._add_account_id_to_global()
self.assertEqual(invoke_context._global_parameter_overrides.get("AWS::AccountId"), "210987654321")