Use version 2 STS tokens to query opt-in regions #79
Conversation
| from boto3.session import Session | ||
|
|
||
| from botocove import cove | ||
| from tests.moto_mock_org.moto_models import SmallOrg | ||
|
|
||
| cove = functools.partial(cove, thread_workers=1, raise_exception=True) |
There was a problem hiding this comment.
In real use cases I almost always use the default settings for thread_workers and raise_exception.
But setting them like this makes the tests easier to read, write and debug.
- We can assume that
ExceptionDetailsandFailedAssumeRoleare empty and just checkResults. - We can use
pytest.raisesto catch any exception. - We can set a breakpoint in threaded code and get just one debugger instance.
I'd like to refactor all the tests to use this setup.
I'd rewrite test_exceptions to check that ExceptionDetails and FailedAssumeRole are set correctly for a few examples when raise_exception is False.
I would also add a test for the threading.
| if f["AssumeRoleSuccess"] is False | ||
| ], | ||
| ) | ||
|
|
||
| if regions is None: |
There was a problem hiding this comment.
I'm not following why this is required - I'm sure I'm missing something obvious but could you explain?
There was a problem hiding this comment.
- Test: When the region is unspecified, the result has no region key
cove_output:{k: v for k, v in r.items() if v is not None}, so region key is excluded when it'sNoneCoveHostAccount.__init__:self.target_regions = [self._session.region_name], so the region is neverNone
I think self.target_regions = [self._session.region_name] was the easiest way to pass one of the assuming_session tests, but it's unclear now from this big PR.
And it always made me feel uneasy to see that [None] for the regions list. It was a nice hack to add multi-region support in a backwards-compatible way, but it is confusing.
There was a problem hiding this comment.
Gotcha - makes sense. I don't think we need to maintain this behaviour in a new release: we can just always emit a Region key in the output and bump the sem ver appropriately. I'm not overly worried about adding to output, it's removing things that's more of a place to be wary of previous API contracts.
|
"TL;DR" is good feedback for this. I was thinking the same. I'll break it into the three parts you identified over the next few days so it's easier to reason about. First the refactoring, then the |
|
I wouldn't go as far as to be ungrateful and say TLDR :) It's just that when all the tests change, and then there's new logic that doesn't appear to impact something in the diff (the del regions loop), plus positive refactors of old behaviour, it's hard to reason about without relearning the whole code base. |
|
I know, I didn't take it badly :-) I've already confused myself about why exactly some of the test changes were necessary, so I don't want to make you spend time trying to figure it out. I'll break it up into smaller PRs that are easier to digest. Probably won't get time this week, but I hope to submit the first one next week. |
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.
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.
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 #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 #77.
The new tests use the
cove()form because the tests are easier to read and write. I trust that Python's syntactic sugar makes it work the same way for the@coveform.The only thing that makes me uneasy about this PR is not having a clear answer for why I have to set the default region.
If you want I can split this into two PRs; one to fix the bug in
assuming_sessionand another to add support to query opt-in regions.There are no unit tests for opt-in region support because Moto doesn't implement the concept. Use the test in #77 to check it.