EN-1912: add service control policy support #384

JonathanLoscalzo · 2023-05-04T14:24:30Z

What changed?

apply: support service control policies files, tags and targets
import: import service control policies with tags and targets
detect: cloudformation files changes:
- force event-bus, rules and sqs to be at us-east-1 region (cloudtrail is not crossregional)

How was it tested?

If it was manually verified, list the instructions for your reviewers to follow.

codecov · 2023-05-04T14:29:46Z

Codecov Report

Patch coverage: 76.89% and project coverage change: -9.66 ⚠️

Comparison is base (b9d1aa9) 85.20% compared to head (5432eaf) 75.54%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #384      +/-   ##
==========================================
- Coverage   85.20%   75.54%   -9.66%     
==========================================
  Files          98      102       +4     
  Lines       10731    11372     +641     
==========================================
- Hits         9143     8591     -552     
- Misses       1588     2781    +1193

Flag	Coverage Δ
functional_tests	`?`
functional_tests_config_discovery	`?`
unit_tests	`75.54% <76.89%> (+0.25%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
iambic/core/template_generation.py	`84.67% <ø> (-3.35%)`	⬇️
iambic/main.py	`50.49% <0.00%> (ø)`
iambic/plugins/v0_1_0/aws/handlers.py	`30.05% <42.55%> (-25.81%)`	⬇️
iambic/plugins/v0_1_0/aws/models.py	`59.79% <43.47%> (-26.65%)`	⬇️
...bic/plugins/v0_1_0/aws/organizations/scp/models.py	`62.30% <62.30%> (ø)`
iambic/plugins/v0_1_0/aws/iambic_plugin.py	`75.71% <75.00%> (-5.77%)`	⬇️
...0_1_0/aws/organizations/scp/template_generation.py	`95.23% <95.23%> (ø)`
...mbic/plugins/v0_1_0/aws/organizations/scp/utils.py	`96.39% <96.39%> (ø)`
iambic/core/models.py	`82.32% <100.00%> (-7.89%)`	⬇️
iambic/core/utils.py	`82.51% <100.00%> (-4.82%)`	⬇️
... and 3 more

... and 39 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

iambic/plugins/v0_1_0/aws/organizations/scp/models.py

iambic/config/wizard.py

iambic/core/models.py

iambic/plugins/v0_1_0/aws/handlers.py

iambic/plugins/v0_1_0/aws/models.py

iambic/plugins/v0_1_0/aws/organizations/scp/models.py

iambic/plugins/v0_1_0/aws/models.py

iambic/plugins/v0_1_0/aws/handlers.py

Will-NOQ · 2023-05-22T15:04:22Z

iambic/plugins/v0_1_0/aws/models.py

+    organization_account: bool = Field(
+        False, description="if this is an organization account"
+    )
+    organization: Optional[AWSOrganization] = Field(


It's not a blocker but at some point it may be worth circling back and removing organization and aws_config to prevent accidental circular refs.

We could do something like adding the additional required org attributes to the AWSAccount object and have some type of org directory on the org AWSAccount. We're currently doing something similar with CloudUMI.

Indeed the problem is you are not able to know if your account should be used to execute any operation over aws-organizations. You need the config to parse the account names to id's.

iambic/plugins/v0_1_0/aws/organizations/scp/template_generation.py

Will-NOQ · 2023-05-22T15:11:14Z

iambic/config/wizard.py

@@ -1875,7 +1876,9 @@ def configuration_wizard_change_detection_setup(self, aws_org: AWSOrganization):

        role_name = IAMBIC_SPOKE_ROLE_NAME
        hub_account_id = self.hub_account_id
-        sqs_arn = f"arn:aws:sqs:{self.aws_default_region}:{hub_account_id}:IAMbicChangeDetectionQueue{IAMBIC_CHANGE_DETECTION_SUFFIX}"
+        # cloudtrail is not cross-region, so we need to use us-east-1


We'll need to check if SSO messages are duplicated or if we're going to need a dedicated queue for SSO.

It may just require a ticket and not need to worry about fixing in this PR.

Will-NOQ · 2023-05-22T15:14:11Z

It'd also be worth adding some functional tests to this PR to check the typical CRUD scenarios with multiple SCPs on the org.

Will-NOQ

Left some notes but nothing huge. Nice stuff!

Will-NOQ · 2023-05-26T16:30:51Z

iambic/plugins/v0_1_0/aws/organizations/scp/models.py

+        return self.statement.sid
+
+    @staticmethod
+    def parse_raw_policy(resource_raw) -> "PolicyDocument":


Not a big deal but classmethod is a better fit here

Will-NOQ · 2023-05-26T16:37:57Z

iambic/plugins/v0_1_0/aws/organizations/scp/utils.py

+    ]
+
+
+@retry(


Not a big deal but legacy_paginated_search, paginated_search, and boto_crud_call already have a retry with backoff on recoverable errors. It's really just throttling limit right now but if you're hitting an exception in these functions that aren't SCP specific the handling should really go there.

Will-NOQ · 2023-05-26T16:40:46Z

iambic/plugins/v0_1_0/aws/organizations/scp/utils.py

+
+    await asyncio.gather(*targets_tasks)
+
+    log.info(f"Deleting policy {policyId}")


It may be worth changing a lot of these logging messages to debug to reduce the noise a user will see when they're running it locally. I've def be guilty of using info instead of debug in here but I've been trying to go back and adjust.

Will-NOQ · 2023-05-26T16:45:28Z

iambic/plugins/v0_1_0/aws/organizations/scp/utils.py

+    return tasks, response
+
+
+@retry(


I don't think this function will encounter a recoverable exception

Will-NOQ · 2023-05-26T16:46:15Z

iambic/plugins/v0_1_0/aws/organizations/scp/utils.py

+        return response
+
+
+@retry(


I don't think this function will encounter a recoverable exception

Will-NOQ · 2023-05-26T16:46:51Z

iambic/plugins/v0_1_0/aws/organizations/scp/utils.py

+    return tasks, response
+
+
+@retry(


I don't think this function will encounter a recoverable exception

Will-NOQ · 2023-05-26T16:47:27Z

iambic/plugins/v0_1_0/aws/organizations/scp/utils.py

+    return tasks, response
+
+
+@retry(


I don't think this function will encounter a recoverable exception

castrapel · 2023-05-26T20:02:31Z

@Will-NOQ - Unfortunately all of these need retry operators because AWS Orgs has ridiculously low rate limits (I saw TooManyRequestsException too easily, it was super brittle), A lot of ConcurrentModificationException errors (even though our modifications often are not being performed in parallel), and some other issues. Some discussions around this in Slack:

https://noqglobal.slack.com/archives/C02HF22G4MU/p1685030515330609?thread_ts=1685029944.899119&cid=C02HF22G4MU

https://noqglobal.slack.com/archives/C02HF22G4MU/p1685029944899119

castrapel

LGTM!

JonathanLoscalzo force-pushed the task/ner-1912 branch 9 times, most recently from b01ba37 to 093b13e Compare May 17, 2023 13:32

JonathanLoscalzo force-pushed the task/ner-1912 branch 6 times, most recently from 81917c6 to c29e9c4 Compare May 19, 2023 21:25

JonathanLoscalzo changed the title ~~EN-1912: adding SCP WIP~~ EN-1912: add service control policy support May 19, 2023

JonathanLoscalzo force-pushed the task/ner-1912 branch from c29e9c4 to 938cb18 Compare May 19, 2023 21:31

JonathanLoscalzo requested review from castrapel and Will-NOQ May 19, 2023 21:34

JonathanLoscalzo self-assigned this May 19, 2023

JonathanLoscalzo marked this pull request as ready for review May 19, 2023 21:34

JonathanLoscalzo force-pushed the task/ner-1912 branch from 938cb18 to fc14167 Compare May 19, 2023 21:43