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.

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

Jump to bottom

chore(unittests): if not defined set a default region in boto3_proxy.py #232

Closed
wants to merge 2 commits into from
Closed

chore(unittests): if not defined set a default region in boto3_proxy.py #232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d5aca5a
if not defined set a default region in boto3_proxy.py
Dec 6, 2022
de8ef18
check for region being explicitly set to None and tests
Dec 6, 2022
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
4 changes: 2 additions & 2 deletions src/cloudformation_cli_python_lib/boto3_proxy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ def __init__(self, session: Session):


def _get_boto_session(
credentials: Optional[Credentials], region: Optional[str] = None
credentials: Optional[Credentials], region: Optional[str] = "us-east-1"
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the default being something other than None.

Wondering though.... why not

if not credentials or not region:
  return None

Seems weird to get credentials not be None but then have no region?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the check for region being explicitly set to none and tests around it

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we want to have this be the default but I'm still trying to think through it all. While I get we are fixing for this for testing purposes I'm afraid of what will happen under normal operations:

So I'll try my best to explain. For a resource we are going to call _get_boto_session with the credentials and provide no region. If this resource lives in us-west-2 are we resetting the session to be in us-east-1? Is the region picked up from the environment variables provided in the Lambda runtime?

cloudformation-cli-python-plugin/src/cloudformation_cli_python_lib/resource.py

Lines 151 to 152 in eb8d6ca

caller_sess = _get_boto_session(event.requestData.callerCredentials)
provider_sess = _get_boto_session(event.requestData.providerCredentials)

Those values are a class defined by either None or:

cloudformation-cli-python-plugin/src/cloudformation_cli_python_lib/utils.py

Lines 48 to 52 in eb8d6ca

@dataclass
class Credentials:
accessKeyId: str
secretAccessKey: str
sessionToken: str

That being said from what I can tell we should not be testing for region being none.

) -> Optional[SessionProxy]:
if not credentials:
if not credentials or not region:
return None
session = Session(
aws_access_key_id=credentials.accessKeyId,
Expand Down
12 changes: 11 additions & 1 deletion tests/lib/boto3_proxy_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@
from cloudformation_cli_python_lib.utils import Credentials


def test_get_boto_session_returns_proxy():
def test_get_boto_session_region_returns_proxy():
proxy = _get_boto_session(Credentials("", "", ""), region="test-region")
assert isinstance(proxy, SessionProxy)


def test_get_boto_session_no_region_returns_proxy():
proxy = _get_boto_session(Credentials("", "", ""))
assert isinstance(proxy, SessionProxy)

Expand All @@ -13,6 +18,11 @@ def test_get_boto_session_returns_none():
assert proxy is None


def test_get_boto_no_session_no_region_returns_none():
proxy = _get_boto_session(None, None)
assert proxy is None


def test_can_retrieve_boto_session():
proxy = _get_boto_session(Credentials("", "", ""))
session = proxy.session
Expand Down