Skip to content

Commit

Permalink
Fix effect on target regions of assuming_session
Browse files Browse the repository at this point in the history
Change the host account init to pass these tests:

* `test_when_no_default_region_then_cove_uses_assuming_session_region`
* `test_cove_prefers_assuming_session_region`

A side effect of the change is even if the caller doesn't specify a target
region then the cove result includes a `Region` key. (PR connelldave#79 shows how to avoid
that side effect, but it's simpler to accept it.)

Set the default region for all tests to allow me to test for that side effect.
A real user will always set the region from a profile or an environment
variable if one isn't passed explicitly to botocove.

The default region forces me to update the `test_regions` module. Without
setting a default region, the old test still passed!

* Old: `test_when_region_is_unspecified_then_result_has_no_region_key`
* New: `test_when_region_is_unspecified_then_result_has_default_region_key`

The default region makes a `Region` key appear in tests that check the whole
cove output instead of a single feature. Later I'll refactor these tests
because it's not clear what feature they cover.

The default region is `eu-west-1` and all region arguments use some other value
such as `eu-central-1` or `us-east-1`. This convention makes the tests easier
to read.

Use the `_no_default_region` fixture when you really need to test how cove
behaves without the default region.

Using the `_org_with_one_member` fixture and the `raise_exception=True`
parameter together make the tests easier to write because you can assume the
cove output has a single result and no exceptions. If any exception occurs, use
`pytest.raises` to detect it. Later I'll refactor more tests to use this
calling style. We can use dedicated test modules to cover the behavior of
`raise_exception=False` and of an organization with multiple accounts.

This paves the way to querying the opt-in regions in connelldave#77.
  • Loading branch information
iainelder committed Dec 29, 2023
1 parent 10fb482 commit 45da32e
Show file tree
Hide file tree
Showing 9 changed files with 82 additions and 14 deletions.
2 changes: 1 addition & 1 deletion botocove/cove_host_account.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ def __init__(
self.host_account_partition = caller_id["Arn"].split(":")[1]

if regions is None:
self.target_regions = [None]
self.target_regions = [assuming_session.region_name]
else:
self.target_regions = regions

Expand Down
3 changes: 2 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ flake8-print = ">=5.0.0"
mypy = ">=1.5.1"
pre-commit = ">=3.4.0"
flakeheaven = ">=3.3.0"
moto = {extras = ["organizations", "sts"], version = ">=4.2.5"}
moto = {extras = ["organizations", "sts", "ec2"], version = ">=4.2.5"}
boto3-stubs = {extras = ["ec2"], version = "*"}
pytest-randomly = ">=3.15.0"

# These are the last versions compatible with flake8 4. flakeheaven 3.3.0 is
Expand Down
10 changes: 10 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,16 @@ def _clean_env(monkeypatch: MonkeyPatch) -> None:
monkeypatch.setenv(env_var, "broken_not_real_profile")


@pytest.fixture(autouse=True)
def _default_region(monkeypatch: MonkeyPatch) -> None:
monkeypatch.setenv("AWS_DEFAULT_REGION", "eu-west-1")


@pytest.fixture()
def _no_default_region(monkeypatch: MonkeyPatch) -> None:
monkeypatch.delenv("AWS_DEFAULT_REGION")


@pytest.fixture()
def mock_session() -> Iterator[Session]:
"""Returns a session with mock AWS services."""
Expand Down
53 changes: 53 additions & 0 deletions tests/test_assuming_session.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import pytest
from boto3.session import Session
from botocore.exceptions import NoRegionError
from moto import mock_ec2

from botocove import cove

# Query the region with different configurations of assuming session and
# environment variables. To make the assertions easier all `cove` calls set
# `raise_exception`. If set the assuming session region and the default region
# are always distinct to be able to assert the source of the query result.

def _query_region(session: Session) -> str:
with mock_ec2():
response = session.client("ec2").describe_availability_zones()
return response["AvailabilityZones"][0]["RegionName"]


@pytest.fixture(autouse=True)
def _org_with_one_member(mock_session: Session) -> None:
org_client = mock_session.client("organizations")
org_client.create_organization(FeatureSet="ALL")
org_client.create_account(Email="[email protected]", AccountName="Account 1")


@pytest.mark.usefixtures("_no_default_region")
def test_when_no_assuming_session_and_no_default_region_then_cove_raises_error() -> None: # noqa: 501
with pytest.raises(NoRegionError, match=r"^You must specify a region\.$"):
cove(_query_region, raise_exception=True)()


def test_when_no_assuming_session_then_cove_uses_default_region() -> None:
output = cove(_query_region, raise_exception=True)()
assert output["Results"][0]["Result"] == "eu-west-1"


@pytest.mark.usefixtures("_no_default_region")
def test_when_no_default_region_then_cove_uses_assuming_session_region() -> None:
output = cove(
_query_region,
assuming_session=Session(region_name="eu-central-1"),
raise_exception=True,
)()
assert output["Results"][0]["Result"] == "eu-central-1"


def test_cove_prefers_assuming_session_region() -> None:
output = cove(
_query_region,
assuming_session=Session(region_name="eu-central-1"),
raise_exception=True,
)()
assert output["Results"][0]["Result"] == "eu-central-1"
1 change: 1 addition & 0 deletions tests/test_decorator.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ def simple_func(session: CoveSession, arg1: int, arg2: int, arg3: int) -> int:
"Result": 6,
"RoleName": "OrganizationAccountAccessRole",
"RoleSessionName": "OrganizationAccountAccessRole",
"Region": "eu-west-1",
}
]

Expand Down
3 changes: 3 additions & 0 deletions tests/test_decorator_no_args.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ def simple_func(session: CoveSession) -> str:
"RoleName": "OrganizationAccountAccessRole",
"RoleSessionName": "OrganizationAccountAccessRole",
"Result": "hello",
"Region": "eu-west-1",
}
]
assert cove_output["Results"] == expected
Expand All @@ -58,6 +59,7 @@ def simple_func(session: CoveSession, output: str) -> str:
"RoleName": "OrganizationAccountAccessRole",
"RoleSessionName": "OrganizationAccountAccessRole",
"Result": "blue",
"Region": "eu-west-1",
}
]
assert cove_output["Results"] == expected
Expand Down Expand Up @@ -86,6 +88,7 @@ def simple_func(
"RoleName": "OrganizationAccountAccessRole",
"RoleSessionName": "OrganizationAccountAccessRole",
"Result": ("blue", "circle", "11:11"),
"Region": "eu-west-1",
}
]
assert cove_output == expected
1 change: 1 addition & 0 deletions tests/test_exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ def simple_func(session: CoveSession) -> None:
"Status": "ACTIVE",
"RoleSessionName": "OrganizationAccountAccessRole",
"ExceptionDetails": repr(Exception("oh no")),
"Region": "eu-west-1",
}

# Compare repr of exceptions
Expand Down
20 changes: 8 additions & 12 deletions tests/test_regions.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from tests.moto_mock_org.moto_models import SmallOrg


def test_when_region_is_unspecified_then_result_has_no_region_key(
def test_when_region_is_unspecified_then_result_has_default_region_key(
mock_small_org: SmallOrg,
) -> None:
@cove()
Expand All @@ -15,7 +15,7 @@ def do_nothing(session: Session) -> None:
output = do_nothing()
assert output["Results"]
for result in output["Results"]:
assert "Region" not in result
assert result["Region"] == "eu-west-1"


def test_when_region_is_unspecified_then_output_has_one_result_per_account(
Expand All @@ -26,20 +26,16 @@ def do_nothing(session: Session) -> None:
pass

output = do_nothing()
print(output["Results"])
print(len(output["Results"]))
print(_count_member_accounts(mock_session))
print(mock_small_org.all_accounts)
assert len(output["Results"]) == _count_member_accounts(mock_session)


def test_when_region_is_str_then_raises_type_error(mock_small_org: SmallOrg) -> None:
@cove(regions="eu-west-1") # type: ignore[arg-type]
@cove(regions="eu-central-1") # type: ignore[arg-type]
def do_nothing() -> None:
pass

with pytest.raises(
TypeError, match=r"regions must be a list of str\. Got str 'eu-west-1'\."
TypeError, match=r"regions must be a list of str\. Got str 'eu-central-1'\."
):
do_nothing()

Expand All @@ -58,28 +54,28 @@ def do_nothing() -> None:
def test_when_any_region_is_passed_then_result_has_region_key(
mock_small_org: SmallOrg,
) -> None:
@cove(regions=["eu-west-1"])
@cove(regions=["eu-central-1"])
def do_nothing(session: Session) -> None:
pass

output = do_nothing()
assert output["Results"]
for result in output["Results"]:
assert result["Region"] == "eu-west-1"
assert result["Region"] == "eu-central-1"


def test_when_two_regions_are_passed_then_output_has_one_result_per_account_per_region(
mock_session: Session, mock_small_org: SmallOrg
) -> None:
@cove(regions=["eu-west-1", "us-east-1"])
@cove(regions=["eu-central-1", "us-east-1"])
def do_nothing(session: Session) -> None:
pass

output = do_nothing()

number_of_member_accounts = _count_member_accounts(mock_session)

for region in ["eu-west-1", "us-east-1"]:
for region in ["eu-central-1", "us-east-1"]:
number_of_results_per_region = sum(
1 for result in output["Results"] if result["Region"] == region
)
Expand Down
3 changes: 3 additions & 0 deletions tests/test_session.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ def simple_func(session: CoveSession, a_string: str) -> str:
"Result": "test-string",
"RoleName": "OrganizationAccountAccessRole",
"RoleSessionName": "OrganizationAccountAccessRole",
"Region": "eu-west-1",
}
]
assert cove_output["Results"] == expected
Expand Down Expand Up @@ -66,6 +67,7 @@ def simple_func(session: CoveSession, a_string: str) -> str:
"RoleName": "OrganizationAccountAccessRole",
"RoleSessionName": "OrganizationAccountAccessRole",
"Policy": session_policy,
"Region": "eu-west-1",
}
]
assert cove_output["Results"] == expected
Expand Down Expand Up @@ -97,6 +99,7 @@ def simple_func(session: CoveSession, a_string: str) -> str:
"RoleName": "OrganizationAccountAccessRole",
"RoleSessionName": "OrganizationAccountAccessRole",
"PolicyArns": session_policy_arns,
"Region": "eu-west-1",
}
]
assert cove_output["Results"] == expected

0 comments on commit 45da32e

Please sign in to comment.