Preparation for MultiAccount support #5157
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5157 +/- ##
==========================================
- Coverage 95.91% 95.88% -0.03%
==========================================
Files 686 686
Lines 67827 67483 -344
==========================================
- Hits 65055 64708 -347
- Misses 2772 2775 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
this looks great @bblommers! excited to see that multi-account is coming to moto :-) i'm sure this will simplify things also on our end for multi-account. i haven't fully understood the ramifications of the change yet, but as you said it looks like this won't have much impact since it's backwards compatible. it may affect some monkeypatches to backends but we can iron those out. |
There was a problem hiding this comment.
Wow, that is a massive set of changes, big kudos @bblommers and thanks for keeping us in the loop, 🙏 really appreciated! Great to see that you were also able to simplify/streamline some of the __init__ super calls, as well as default reset(..) implementations. 👍
My only question would be around hardcoding account ID "123456789012" in the new logic in BackendDict (see comments) - other than that, this is a huge enhancement and I think we can mostly work around the changes in here, as @thrau mentioned. 👍 Looking forward to seeing this in action! 🥳
| The latter two will be phased out in the future, and we can remove this method. | ||
| """ | ||
| if re.match(r"[0-9]+", account_id_or_region): | ||
| self._create_account_specific_backend("123456789012") |
There was a problem hiding this comment.
Wondering if we should use account_id_or_region here instead?
self._create_account_specific_backend(account_id_or_region)
There was a problem hiding this comment.
This was done on purpose - hardcoding the ID keeps everything in a single account for now, ensuring that the current behaviour doesn't change.
The follow-up PR will definitely change this to get_account_id, but that will need a few other changes - and many more tests.
Wrapping everything in one PR would make it even more unwieldy and impossible to review, that's why I chose to break it up in two steps: 1 PR to prepare, and 1 PR to actually change the behaviour.
There was a problem hiding this comment.
Understood. That makes a lot of sense, thanks for clarifying! 👍
| return True | ||
| else: | ||
| region = account_id_or_region | ||
| self._create_account_specific_backend("123456789012") |
There was a problem hiding this comment.
Similar/related to the comment above - should we replace "123456789012" with get_account_id() here? This would help us encapsulate the access to the default region returned in get_account_id() (and will also make it easier to customize it).
|
This is now part of moto >= 3.1.12.dev8 |
This PR is an first step towards support for multi-accounts, by changing the underlying data structure to enable this feature.
Current implementation:
Most services use an instance of the
BackendDict-class to persist the state.The BackendDict overrides a dictionary, and stores data in the following format:
Because global services such as S3 do not have any regions, those backends were persisted as straightforward dictionaries:
Proposed implementation:
All services now use the
BackendDict, with an option to specify whether this is a global service.The underlying data structure of the BackendDict now looks like this:
Benefits of this approach:
region_nameandaccount_id, reducing code duplicationNoticeable aspects of this PR:
There is no change in functionality.
self.account_id, but continues to importget_account_idA lot of users access the internal API using
backend_dict["us-east-1"].model.attribute = "..".The
__getitem__-method of BackendDict has been overridden to still support this, enabling backwards compatibilityThe meat of this PR lies in
moto/core/utils/BackendDict- all other changes are a logical consequence of the changes to that class.Future PR's:
Because of the massive scope of this change, the current PR only prepares multi-account support, but without any functional changes.
A future PR is necessary to then unlock this functionality. This PR would consist of the following changes:
get_account_idor IAMbackend_dict[account_id][region_name]self.account_id, instead ofget_account_id()backend_dict[region_name]directly, as we would have no context about which account we want to useBecause this PR would be a breaking change, it would have to be released as part of the next major release.
Related PR's:
#4699 introduced the BackendDict in the first place
#5098 added another way to override AccountID
TODO:
Need to verify that the scaffold-script still creates the expected templates.