extend target_ids to support AWS Organization Units

[aperov9]

Modified botocove so it can take AWS accounts and/or AWS Organization Units as target_ids.
It will fetch all child ou's from the parent ou recursively.
Exceptions will be raised if target_ids does not match the regex for account or ou.

Possible inputs now:

• target_ids = None
• target_ids = ["111111111111"]
• target_ids = ["111111111111", "222222222222]
• target_ids = ["ou-cpg3-k3ijh102"]
• target_ids = ["ou-cpg3-k3ijh102", "ou-ckf1-kp2i7tzd"]
• target_ids = ["111111111111", "ou-cpg3-k3ijh102", "222222222222" ]

[connelldave]

Hey - thanks for the excellent contribution!

I've added a few comments to the PR that it'd be good to address before merging, but I really like the change.

There are some typing and linting errors - poetry run pre-commit run --all-files will flag them up.

I'd like to have some tests for the recursive logic but I'm happy to contribute those later - I've been playing with the moto library as mocking out the org paginators manually would be very unplesant.

[iainelder]

@aperov9 , thanks for the contribution! It's been on my mental feature backlog for botocove for a while.

I agree absolutely that botocove should have a way to filter by organizational unit (OU). I don't agree with making it another responsibility of the CoveHostAccount class.

Overloading the meaning of target_account_ids makes it harder to explain what the parameter means. CoveHostAccount is already quite complicated, and I think if anything we should start trying to break it up into tightly-scoped and more testable parts.

My request is that this feature be implemented with minimal changes to the existing CoveHostAccount class. I believe it will make for more robust and reusable code.

I propose that we encapsulate the logic in a new function called something like org_account_selector that returns a list of account IDs. All the logic that your PR provides would be more cleanly encapsulated there. To use the feature, a botocove client would pass the result of the function to the target_account_ids parameter.

@cove(
  target_account_ids=org_account_selector(included_ous=["ou-cpg3-k3ijh102", "ou-ckf1-kp2i7tzd"]),
)
def wrapped_function(session):
    ...

By default the function would return the IDs of all the organization's AWS accounts whose status is active, matching botocove's current behavior.

It would take an optional parameter called something like included_ous, a list of OU IDs whose accounts are to be retrieved. The retrieval would recurse into sub-OUs. By default the function would act as if the root OU ID is passed.

It would take another optional parameter called something like excluded_ous, a list of OU IDs to skip over when retrieving the account IDs. No account IDs in these OUs or their sub-OUs would be returned. (This additional feature supports my use case of having to exclude accounts in the Suspended OU that are not yet suspended but already are subject to a service control policy (SCP) that would cause errors in botocove.)

If you need to express your account selection as mix of OU memberships and literal account IDs, you can do this by concatenating the result of the function with a list of literal IDs.

@cove(
    target_account_ids=(
        org_account_selector(included_ous=["ou-cpg3-k3ijh102", "ou-ckf1-kp2i7tzd"]) +
        ["111111111111", "222222222222"]
    ),
)
def wrapped_function(session):
    ...

I'd like to have some tests for the recursive logic but I'm happy to contribute those later - I've been playing with the moto library as mocking out the org paginators manually would be very unplesant.

The built-in botocore Stubber class allows us to mock individual API calls in a specific order. That would couple the implementation too tightly to the AWS APIs and have us testing the implementation rather than the interface.

If moto supports all the organizations APIs that we need to use, I agree that it is probably the best way to go for testing this. It should allow us to declare different organization structures using a series of create_account calls in the setup functions of the test suite.

@connelldave , @aperov9 , what do you think of my proposal? If you agree I would be able to help with the testing and implementation. I have some code in aws-org-tree that could be adapted for the purpose. Its job is to recurse over the whole AWS organization to link each AWS account to its parent OU. Since we don't even need to retain the OU membership information, a simplier verison of it would probably be good enough. aws-org-tree doesn't have any tests yet either, but I was planning on adding tests using moto before this issue appeared. :-)

[connelldave]

Overloading the meaning of target_account_ids makes it harder to explain what the parameter means. CoveHostAccount is already quite complicated, and I think if anything we should start trying to break it up into tightly-scoped and more testable parts.

I feel this concern, but given the kwarg name is target_ids and ignore_ids I do feel like this fits: my preference is overloaded behaviour and simplicity, and I'm happy to support this addition. Keen to discuss any specific examples of difficult to test logic that aren't a result of being directly coupled with the unavoidable boto3 APIs?

My primary worry with overloading is that there's an implicit coupling between passing OU id's and org_master=True. The presence of an OU id adds a surprise dependency for IAM permission and requirement for use of the Organizations client. We can check for this and fail explicitly though.

A more decomposed, decoupled approach that requires helper functions and complexity to resolve function calls in construction is a large burden on users, especially as decorators are most often constructed in global scope. The other challenge is actually making users aware of this behaviour and where to import it from: ideally the barrier for entry for Python users is low. Failing on bad input rather than requiring complexity in my head is closer to the zen of Python :)

It would take another optional parameter called something like excluded_ous, a list of OU IDs to skip over when retrieving the account IDs. No account IDs in these OUs or their sub-OUs would be returned.

Overloading ignore_ids to also take OU ids would also solve this problem, which is an extension that crossed my mind too :)

[aperov9]

@connelldave @iainelder
Thanks for your comments on this PR. Really appreciate your involvement! :)

Before writing my code I thought about the same issue if I would go with an extra parameter, an additional helper function or just overload the existing target_ids. I prefered the simplicity of the end solution that account id's and organization unit id's are both just target_ids.

However, I do see that this way is harder to test. I would be glad if someone could assist in writing the test for this new functionality. I am also not familiar enough on how the org_master parameter is impacted by this in particular.

Also I don't see an issue in overloading the ignore_ids in the same way :)

[connelldave]

No worries!

For context - the org_master argument is actually in a bit of a weird place since release 1.5.0 - it's actually not required in CoveHostAccount right now. It only gets used to disable enrichment of session information as things stand.

It's purpose is for a user to be able to disable the standard behaviour of botocove relying on running in an organisation's master account - for situations such as security account that has trust relationships with roles in many accounts. This would make calls to the organizations APIs that rely on running from an org master (list_accounts, describe_account and now list_children) unavailable.

We're actually checking only if target_ids=None today on line 110 of cove_host_account.py for "don't do org master related tasks" which is a bit smelly - maybe we should raise if org_master is False in this block or similar too.

if target_ids is None:
            # No target_ids passed 
            # new:
            if self.org_master is False:
                raise ValueError("target_ids keyword argument must be provided if org_master is set to False")
            target_accounts = self._gather_org_assume_targets()

[connelldave]

@iainelder couldn't tag you on the fork PR but aperov9#1 - fyi

This is just a first pass this morning, it can probably be used more cleanly but as a first moto attempt it was nice to work with! No obligation to merge or use it @aperov9 , just thought I'd share if useful. Feel free to alter or use it if helps dev.

[iainelder]

A more decomposed, decoupled approach that requires helper functions and complexity to resolve function calls in construction is a large burden on users, especially as decorators are most often constructed in global scope.

Before writing my code I thought about the same issue if I would go with an extra parameter, an additional helper function or just overload the existing target_ids. I prefered the simplicity of the end solution that account id's and organization unit id's are both just target_ids.

You've persuaded me on the overloading. Overloading the meaning of the existing target_ids and exclude_ids would make things easier for the user. It's obvious for a human to distinguish the OU IDs and account IDs, and programmatically it possible to partition them using string pattern matching.

I still believe that by avoiding modifications to CoveHostAccount to implement this feature we'll have an easier time maintaining and testing it. So I would propose something like the org_account_iter function as a helper function that can be tested in isolation from the rest of the parts whose job is to resolve the list of IDs in the user input to a final list of target AWS account IDs.

The presence of an OU id adds a surprise dependency for IAM permission and requirement for use of the Organizations client.

Botocove already documents the permissions that it requires. Don't we just need to add the extra permissions the feature requires to the documentation?

My primary worry with overloading is that there's an implicit coupling between passing OU id's and org_master=True. [...] For context - the org_master argument is actually in a bit of a weird place since release 1.5.0 - it's actually not required in CoveHostAccount right now.

I was thinking that we should rename the org_master parameter to something like enrich_org_account_metadata to reflect what it really does. I don't use botocove in the "security account" setup that you describe so I didn't consider that use case.

Really appreciate your involvement! :)

I also appreciate yours! It's great to see that this feature is also useful for others.

Thanks for the PR link. I'll have a look later on.

[aperov9]

With the latest commit ffc946e I addressed all your previous comments @connelldave and checked that pre-commit runs now without any errors.

Next, I am looking into extending the logic to ignore_ids and into decoupling CoveHostAccount from the logic of fetching accounts as proposed by @iainelder.

[connelldave]

With the latest commit ffc946e I addressed all your previous comments @connelldave and checked that pre-commit runs now without any errors.

Thanks - looks good!

Next, I am looking into extending the logic to ignore_ids and into decoupling CoveHostAccount from the logic of fetching accounts as proposed by @iainelder.

Suggest splitting this across two commits - I'm keen to see what problem that decoupling solves, since "getting accounts" is in the remit of the host account when running from an org master account.

[iainelder]

I didn't get chance to look at the code for this and I won't have any time until at least next week. Don't wait for me to merge it if it looks good to you @connelldave . I trust your judgement here.

[aperov9]

Extended the logic so ignore_ids can take ou's as well. I'am however not so satisfied with my _get_validated_ids() method. Do you have a suggestion on how to make the output for the user more specific without passing the type of the ids as a string parameter?

I also looked into decoupling the logic and how the current CoveHostAccount class is setup. I realised that there is no real benefit decoupling it even more. I think the existing solution is sufficient.

Lastly, while testing I encountered the following situation. Botocove will return "There are no eligible account ids to run decorated func against" in case an ou in target_ids is a subset of ignore_ids. Might be worthwhile to think of a way how to make the error more specific for the end user or just add it directly to the exception.

[connelldave]

Extended the logic so ignore_ids can take ou's as well. I'am however not so satisfied with my _get_validated_ids() method. Do you have a suggestion on how to make the output for the user more specific without passing the type of the ids as a string parameter?

I did a bit of clean-up work on my branch for multi-region support as there's a bit of duplication around how we build these two sets. We also need to guard against an OU capturing the host account itself in it's account list: I don't think botocove should impact the Host account.

With that in mind, we can just decompose the _resolve_target_accounts logic to something like

def _resolve_target_accounts(self, target_ids: Optional[List[str]]) -> Set[str]:
     accounts_to_ignore = self._gather_ignored_accounts()
     accounts_to_target = self._gather_target_accounts()
    return accounts_to_target - accounts_to_ignore

def gather_ignored():
   this_acc = ...sts.get_caller_identity
  ignored = set(this_acc)

if self.ignore_strings:
   accs, ous = self._parse_ids(self.ignore_strings
   if ous:
    # get ou accs and setify

return ignored

def gather_targeted():
...

does that make sense? I'll try and get #29 merged this evening which starts this process, you could rebase on that branch to save time if you'd like.

Lastly, while testing I encountered the following situation. Botocove will return "There are no eligible account ids to run decorated func against" in case an ou in target_ids is a subset of ignore_ids. Might be worthwhile to think of a way how to make the error more specific for the end user or just add it directly to the exception.

I agree, but for this iteration we can just ship "Did you ignore a parent OU that all target accounts belong to?" or something rather than adding complexity trying to track this edge case. My thinking so far has been only worry about checking for cases that can cause risk - that's why initially only ignore_ids got a validation pass, a malformed ignore has much higher consequence than a malformed target.

[connelldave]

let's break both paginators out into function calls so we can use a cache (functools.lru_cache would be my suggestion) to avoid re-doing work when traversing now that we're potentially going to be walking the same OU tree twice or more

(for example - in OU A, containing B and C, we want to pass A as a target but ignore C - we'd look up children of C twice, and contents of C twice without caching)

[connelldave]

we can patch this in later, let's not block merging with minor optimisation :)

[aperov9]

Great, thanks for your valuable feedback! And lets patch this optimisation later

[connelldave]

I've merged #31 and #29 which will give you a bit of a bumpy rebase from master, but hopefully gives you a much stronger place to test your change in the moto mock org. Hopefully it's relatively self explanatory to re-use that setup, I'm pleased with it as a test harness :)

[aperov9]

With the latest commits I merged the changes from master into my branch and adjusted/added some test cases with the moto mock org. The setup is indeed quite easy to use and self explanatory :)

I also refactored _resolve_target_accounts() with the _gather_ignored_accounts() and _gather_target_accounts() methods accordingly.

The only thing missing now would be the caching mechanism and some further test cases. I'll be quite busy the next weeks and I'm not too really familiar with the functools module. So feel free to look into that and create a MR in the meantime. @connelldave

[connelldave]

Thanks for making changes from feedback! Excellent addition, feels like a clean implementation and upgrade. I'd like to get it merged :)

If you want to push a commit to pick up on these final comments feel free, or I can pick them up on the release commit on master if you don't have time.

Thanks again!
Dave

[connelldave]

we can patch this in later, let's not block merging with minor optimisation :)

[connelldave]

🎉 now live on pypi under version 1.6.0! I snuck in the caching on master: it's very simple

botocove/botocove/cove_host_account.py

Line 238 in f6d52f8

@lru_cache()