Skip to content

fix: Move boto3 client creation to a computed property #1059

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.

Sign up for GitHub

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

Merged
jfuss merged 5 commits into from
May 23, 2019
Merged

fix: Move boto3 client creation to a computed property #1059

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dff9fe4
fix: Move boto3 client creation to a computed property
jfuss Mar 13, 2019
73c20a6
Only create on boto3 client
jfuss Mar 19, 2019
c55ffd6
Explictly set region in boto3 client for start-lambda integ tests
jfuss May 22, 2019
472067a
Force sessions to be created per thread
jfuss May 23, 2019
993ce4f
Add comment about session thread safeness or lack there of
jfuss May 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion samcli/commands/local/cli_common/invoke_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,9 @@ def __init__(self, # pylint: disable=R0914
parameter_overrides=None,
layer_cache_basedir=None,
force_image_build=None,
aws_region=None):
aws_region=None,
aws_profile=None,
):
"""
Initialize the context

Expand Down Expand Up @@ -109,6 +111,7 @@ def __init__(self, # pylint: disable=R0914
self._layer_cache_basedir = layer_cache_basedir
self._force_image_build = force_image_build
self._aws_region = aws_region
self._aws_profile = aws_profile

self._template_dict = None
self._function_provider = None
Expand Down Expand Up @@ -197,6 +200,8 @@ def local_lambda_runner(self):
return LocalLambdaRunner(local_runtime=lambda_runtime,
function_provider=self._function_provider,
cwd=self.get_cwd(),
aws_profile=self._aws_profile,
aws_region=self._aws_region,
env_vars_values=self._env_vars_value,
debug_context=self._debug_context)

Expand Down
3 changes: 2 additions & 1 deletion samcli/commands/local/invoke/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@ def do_cli(ctx, function_identifier, template, event, no_event, env_vars, debug_
parameter_overrides=parameter_overrides,
layer_cache_basedir=layer_cache_basedir,
force_image_build=force_image_build,
aws_region=ctx.region) as context:
aws_region=ctx.region,
aws_profile=ctx.profile) as context:

# Invoke the function
context.local_lambda_runner.invoke(context.function_name,
Expand Down
15 changes: 8 additions & 7 deletions samcli/commands/local/lib/local_lambda.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ def __init__(self,
local_runtime,
function_provider,
cwd,
aws_profile=None,
aws_region=None,
env_vars_values=None,
debug_context=None):
"""
Expand All @@ -43,6 +45,8 @@ def __init__(self,
self.local_runtime = local_runtime
self.provider = function_provider
self.cwd = cwd
self.aws_profile = aws_profile
self.aws_region = aws_region
self.env_vars_values = env_vars_values or {}
self.debug_context = debug_context

Expand Down Expand Up @@ -203,13 +207,10 @@ def get_aws_creds(self):
result = {}

# to pass command line arguments for region & profile to setup boto3 default session
if boto3.DEFAULT_SESSION:
session = boto3.DEFAULT_SESSION
else:
session = boto3.session.Session()

profile_name = session.profile_name if session else None
LOG.debug("Loading AWS credentials from session with profile '%s'", profile_name)
LOG.debug("Loading AWS credentials from session with profile '%s'", self.aws_profile)
# boto3.session.Session is not thread safe. To ensure we do not run into a race condition with start-lambda
# or start-api, we create the session object here on every invoke.
session = boto3.session.Session(profile_name=self.aws_profile, region_name=self.aws_region)
Copy link
Contributor

@sriram-mv sriram-mv May 23, 2019

Choose a reason for hiding this comment

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

Should we explicitly call out new session in debug statement as well?

Copy link
Contributor Author

@jfuss jfuss May 23, 2019

Choose a reason for hiding this comment

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

For the customer, it really doesn't matter. I don't think a debug statement for this makes sense.

For further context, this is how we did it prior to the work we did on Layers. We got fancy in using the default session. Turns out that only really worked because we were calling boto3.client. Once we moved this call to be computed, I was seeing sporadic failures on a KeyError within botocore. So I reverted us back to our previous state, which ensures Sessions get created per invoke. Which means for start-lambda and start-api that could be per request.


if not session:
return result
Expand Down
3 changes: 2 additions & 1 deletion samcli/commands/local/start_api/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ def do_cli(ctx, host, port, static_dir, template, env_vars, debug_port, debug_ar
parameter_overrides=parameter_overrides,
layer_cache_basedir=layer_cache_basedir,
force_image_build=force_image_build,
aws_region=ctx.region) as invoke_context:
aws_region=ctx.region,
aws_profile=ctx.profile) as invoke_context:

service = LocalApiService(lambda_invoke_context=invoke_context,
port=port,
Expand Down
3 changes: 2 additions & 1 deletion samcli/commands/local/start_lambda/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,8 @@ def do_cli(ctx, host, port, template, env_vars, debug_port, debug_args, # pylin
parameter_overrides=parameter_overrides,
layer_cache_basedir=layer_cache_basedir,
force_image_build=force_image_build,
aws_region=ctx.region) as invoke_context:
aws_region=ctx.region,
aws_profile=ctx.profile) as invoke_context:

service = LocalLambdaService(lambda_invoke_context=invoke_context,
port=port,
Expand Down
7 changes: 6 additions & 1 deletion samcli/local/layers/layer_downloader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,12 @@ def __init__(self, layer_cache, cwd, lambda_client=None):
"""
self._layer_cache = layer_cache
self.cwd = cwd
self.lambda_client = lambda_client or boto3.client('lambda')
self._lambda_client = lambda_client

@property
def lambda_client(self):
self._lambda_client = self._lambda_client or boto3.client('lambda')
return self._lambda_client

@property
def layer_cache(self):
Expand Down
4 changes: 4 additions & 0 deletions tests/integration/local/start_lambda/test_start_lambda.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ def setUp(self):
self.url = "http://127.0.0.1:{}".format(self.port)
self.lambda_client = boto3.client('lambda',
endpoint_url=self.url,
region_name='us-east-1',
use_ssl=False,
verify=False,
config=Config(signature_version=UNSIGNED,
Expand Down Expand Up @@ -52,6 +53,7 @@ def setUp(self):
self.url = "http://127.0.0.1:{}".format(self.port)
self.lambda_client = boto3.client('lambda',
endpoint_url=self.url,
region_name='us-east-1',
use_ssl=False,
verify=False,
config=Config(signature_version=UNSIGNED,
Expand All @@ -69,6 +71,7 @@ def setUp(self):
self.url = "http://127.0.0.1:{}".format(self.port)
self.lambda_client = boto3.client('lambda',
endpoint_url=self.url,
region_name='us-east-1',
use_ssl=False,
verify=False,
config=Config(signature_version=UNSIGNED,
Expand Down Expand Up @@ -110,6 +113,7 @@ def setUp(self):
self.url = "http://127.0.0.1:{}".format(self.port)
self.lambda_client = boto3.client('lambda',
endpoint_url=self.url,
region_name='us-east-1',
use_ssl=False,
verify=False,
config=Config(signature_version=UNSIGNED,
Expand Down
14 changes: 10 additions & 4 deletions tests/unit/commands/local/cli_common/test_invoke_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ def test_must_read_from_necessary_files(self, SamFunctionProviderMock):
debugger_path="path-to-debugger",
debug_args='args',
parameter_overrides={},
aws_region="region")
aws_region="region",
aws_profile="profile")

template_dict = "template_dict"
invoke_context._get_template_data = Mock()
Expand Down Expand Up @@ -159,7 +160,8 @@ def test_must_work_in_with_statement(self, ExitMock, EnterMock):
skip_pull_image=True,
debug_port=1111,
debugger_path="path-to-debugger",
debug_args='args') as context:
debug_args='args',
aws_profile="profile") as context:
self.assertEquals(context_obj, context)

EnterMock.assert_called_with()
Expand Down Expand Up @@ -207,7 +209,9 @@ def setUp(self):
force_image_build=True,
debug_port=1111,
debugger_path="path-to-debugger",
debug_args='args')
debug_args='args',
aws_profile="profile",
aws_region="region")

@patch("samcli.commands.local.cli_common.invoke_context.LambdaImage")
@patch("samcli.commands.local.cli_common.invoke_context.LayerDownloader")
Expand Down Expand Up @@ -256,7 +260,9 @@ def test_must_create_runner(self,
function_provider=ANY,
cwd=cwd,
debug_context=None,
env_vars_values=ANY)
env_vars_values=ANY,
aws_profile="profile",
aws_region="region")


class TestInvokeContext_stdout_property(TestCase):
Expand Down
13 changes: 11 additions & 2 deletions tests/unit/commands/local/invoke/test_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ def setUp(self):
self.layer_cache_basedir = "/some/layers/path"
self.force_image_build = True
self.region_name = "region"
self.profile = "profile"

@patch("samcli.commands.local.invoke.cli.InvokeContext")
@patch("samcli.commands.local.invoke.cli._get_event")
Expand All @@ -47,6 +48,7 @@ def test_cli_must_setup_context_and_invoke(self, get_event_mock, InvokeContextMo

ctx_mock = Mock()
ctx_mock.region = self.region_name
ctx_mock.profile = self.profile

# Mock the __enter__ method to return a object inside a context manager
context_mock = Mock()
Expand Down Expand Up @@ -82,7 +84,8 @@ def test_cli_must_setup_context_and_invoke(self, get_event_mock, InvokeContextMo
parameter_overrides=self.parameter_overrides,
layer_cache_basedir=self.layer_cache_basedir,
force_image_build=self.force_image_build,
aws_region=self.region_name)
aws_region=self.region_name,
aws_profile=self.profile)

context_mock.local_lambda_runner.invoke.assert_called_with(context_mock.function_name,
event=event_data,
Expand All @@ -97,6 +100,7 @@ def test_cli_must_invoke_with_no_event(self, get_event_mock, InvokeContextMock):

ctx_mock = Mock()
ctx_mock.region = self.region_name
ctx_mock.profile = self.profile

# Mock the __enter__ method to return a object inside a context manager
context_mock = Mock()
Expand Down Expand Up @@ -131,7 +135,8 @@ def test_cli_must_invoke_with_no_event(self, get_event_mock, InvokeContextMock):
parameter_overrides=self.parameter_overrides,
layer_cache_basedir=self.layer_cache_basedir,
force_image_build=self.force_image_build,
aws_region=self.region_name)
aws_region=self.region_name,
aws_profile=self.profile)

context_mock.local_lambda_runner.invoke.assert_called_with(context_mock.function_name,
event="{}",
Expand All @@ -146,6 +151,7 @@ def test_must_raise_user_exception_on_no_event_and_event(self, get_event_mock, I

ctx_mock = Mock()
ctx_mock.region = self.region_name
ctx_mock.profile = self.profile

with self.assertRaises(UserException) as ex_ctx:

Expand Down Expand Up @@ -185,6 +191,7 @@ def test_must_raise_user_exception_on_function_not_found(self,

ctx_mock = Mock()
ctx_mock.region = self.region_name
ctx_mock.profile = self.profile

# Mock the __enter__ method to return a object inside a context manager
context_mock = Mock()
Expand Down Expand Up @@ -231,6 +238,7 @@ def test_must_raise_user_exception_on_invalid_sam_template(self,

ctx_mock = Mock()
ctx_mock.region = self.region_name
ctx_mock.profile = self.profile

InvokeContextMock.side_effect = exeception_to_raise

Expand Down Expand Up @@ -264,6 +272,7 @@ def test_must_raise_user_exception_on_invalid_env_vars(self, get_event_mock, Inv

ctx_mock = Mock()
ctx_mock.region = self.region_name
ctx_mock.profile = self.profile

InvokeContextMock.side_effect = OverridesNotWellDefinedError("bad env vars")

Expand Down
17 changes: 9 additions & 8 deletions tests/unit/commands/local/lib/test_local_lambda.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ def setUp(self):
self.function_provider_mock,
self.cwd,
env_vars_values=self.env_vars_values,
debug_context=self.debug_context)
debug_context=self.debug_context,
aws_profile=self.aws_profile,
aws_region=self.aws_region)

@patch("samcli.commands.local.lib.local_lambda.boto3")
def test_must_get_from_boto_session(self, boto3_mock):
Expand All @@ -43,7 +45,7 @@ def test_must_get_from_boto_session(self, boto3_mock):
mock_session = Mock()
mock_session.region_name = self.region

boto3_mock.DEFAULT_SESSION = mock_session
boto3_mock.session.Session.return_value = mock_session
mock_session.get_credentials.return_value = creds

expected = {
Expand All @@ -56,6 +58,8 @@ def test_must_get_from_boto_session(self, boto3_mock):
actual = self.local_lambda.get_aws_creds()
self.assertEquals(expected, actual)

boto3_mock.session.Session.assert_called_with(profile_name=self.aws_profile, region_name=self.aws_region)

@patch("samcli.commands.local.lib.local_lambda.boto3")
def test_must_work_with_no_region_name(self, boto3_mock):
creds = Mock()
Expand All @@ -66,7 +70,6 @@ def test_must_work_with_no_region_name(self, boto3_mock):
mock_session = Mock()
del mock_session.region_name # Ask mock to return AttributeError when 'region_name' is accessed

boto3_mock.DEFAULT_SESSION = None
boto3_mock.session.Session.return_value = mock_session
mock_session.get_credentials.return_value = creds

Expand All @@ -79,7 +82,7 @@ def test_must_work_with_no_region_name(self, boto3_mock):
actual = self.local_lambda.get_aws_creds()
self.assertEquals(expected, actual)

boto3_mock.session.Session.assert_called_with()
boto3_mock.session.Session.assert_called_with(profile_name=self.aws_profile, region_name=self.aws_region)

@patch("samcli.commands.local.lib.local_lambda.boto3")
def test_must_work_with_no_access_key(self, boto3_mock):
Expand All @@ -91,7 +94,6 @@ def test_must_work_with_no_access_key(self, boto3_mock):
mock_session = Mock()
mock_session.region_name = self.region

boto3_mock.DEFAULT_SESSION = None
boto3_mock.session.Session.return_value = mock_session
mock_session.get_credentials.return_value = creds

Expand All @@ -104,7 +106,7 @@ def test_must_work_with_no_access_key(self, boto3_mock):
actual = self.local_lambda.get_aws_creds()
self.assertEquals(expected, actual)

boto3_mock.session.Session.assert_called_with()
boto3_mock.session.Session.assert_called_with(profile_name=self.aws_profile, region_name=self.aws_region)

@patch("samcli.commands.local.lib.local_lambda.boto3")
def test_must_work_with_no_secret_key(self, boto3_mock):
Expand All @@ -116,7 +118,6 @@ def test_must_work_with_no_secret_key(self, boto3_mock):
mock_session = Mock()
mock_session.region_name = self.region

boto3_mock.DEFAULT_SESSION = None
boto3_mock.session.Session.return_value = mock_session
mock_session.get_credentials.return_value = creds

Expand All @@ -129,7 +130,7 @@ def test_must_work_with_no_secret_key(self, boto3_mock):
actual = self.local_lambda.get_aws_creds()
self.assertEquals(expected, actual)

boto3_mock.session.Session.assert_called_with()
boto3_mock.session.Session.assert_called_with(profile_name=self.aws_profile, region_name=self.aws_region)

@patch("samcli.commands.local.lib.local_lambda.boto3")
def test_must_work_with_no_session_token(self, boto3_mock):
Expand Down
5 changes: 4 additions & 1 deletion tests/unit/commands/local/start_api/test_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,11 @@ def setUp(self):
self.layer_cache_basedir = "/some/layers/path"
self.force_image_build = True
self.region_name = "region"
self.profile = "profile"

self.ctx_mock = Mock()
self.ctx_mock.region = self.region_name
self.ctx_mock.profile = self.profile

self.host = "host"
self.port = 123
Expand Down Expand Up @@ -65,7 +67,8 @@ def test_cli_must_setup_context_and_start_service(self, local_api_service_mock,
parameter_overrides=self.parameter_overrides,
layer_cache_basedir=self.layer_cache_basedir,
force_image_build=self.force_image_build,
aws_region=self.region_name)
aws_region=self.region_name,
aws_profile=self.profile)

local_api_service_mock.assert_called_with(lambda_invoke_context=context_mock,
port=self.port,
Expand Down
5 changes: 4 additions & 1 deletion tests/unit/commands/local/start_lambda/test_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,11 @@ def setUp(self):
self.layer_cache_basedir = "/some/layers/path"
self.force_image_build = True
self.region_name = "region"
self.profile = "profile"

self.ctx_mock = Mock()
self.ctx_mock.region = self.region_name
self.ctx_mock.profile = self.profile

self.host = "host"
self.port = 123
Expand Down Expand Up @@ -60,7 +62,8 @@ def test_cli_must_setup_context_and_start_service(self, local_lambda_service_moc
parameter_overrides=self.parameter_overrides,
layer_cache_basedir=self.layer_cache_basedir,
force_image_build=self.force_image_build,
aws_region=self.region_name)
aws_region=self.region_name,
aws_profile=self.profile)

local_lambda_service_mock.assert_called_with(lambda_invoke_context=context_mock,
port=self.port,
Expand Down