fix exclude_unset issues

repokid/commands/repo.py

@@ -73,8 +73,10 @@ def _repo_role(
 
     continuing = True
 
-    if not role.is_eligible_for_repo():
-        continuing = False
+    eligible, reason = role.is_eligible_for_repo()
+    if not eligible:
+        errors.append(f"Role {role_name} not eligible for repo: {reason}")
+        return errors
 
     role.calculate_repo_scores(
         config["filter_config"]["AgeFilter"]["minimum_age"], hooks
@@ -120,7 +122,7 @@ def _repo_role(
             LOGGER.error(e)
             errors.append(str(e))
 
-    current_policies = get_role_inline_policies(role.dict(), **conn) or {}
+    current_policies = get_role_inline_policies(role.dict(by_alias=True), **conn) or {}
     role.add_policy_version(current_policies, source="Repo")
 
     # regardless of whether we're successful we want to unschedule the repo

repokid/commands/role.py

@@ -23,6 +23,7 @@
 from tqdm import tqdm
 
 import repokid.hooks
+from repokid.exceptions import MissingRepoableServices
 from repokid.role import Role
 from repokid.role import RoleList
 from repokid.types import RepokidConfig
@@ -241,16 +242,16 @@ def _display_role(
     rows = sorted(rows, key=lambda x: (x[2], x[0], x[1]))
     print(tabulate(rows, headers=headers) + "\n\n")
 
-    repoed_policies, _ = role.get_repoed_policy()
-
-    if repoed_policies:
+    try:
+        repoed_policies, _ = role.get_repoed_policy()
         print(
             "Repo'd Policies: \n{}".format(
                 json.dumps(repoed_policies, indent=2, sort_keys=True)
             )
         )
-    else:
+    except MissingRepoableServices:
         print("All Policies Removed")
+        repoed_policies = {}
 
     # need to check if all policies would be too large
     if inline_policies_size_exceeds_maximum(repoed_policies):

repokid/commands/role_cache.py

@@ -65,7 +65,7 @@ def _update_role_cache(
     LOGGER.info("Updating role data for account {}".format(account_number))
     for role in tqdm(roles):
         role.account = account_number
-        current_policies = iam_datasource[role.role_id]["RolePolicyList"]
+        current_policies = iam_datasource[role.role_id].get("RolePolicyList", {})
         role.gather_role_data(
             current_policies, hooks, config, source="Scan", store=False
         )

repokid/role.py

@@ -77,7 +77,7 @@ class Role(BaseModel):
     repoable_permissions: int = Field(default=0)
     repoable_services: List[str] = Field(default=[])
     repoed: Optional[str] = Field()
-    repo_scheduled: float = Field(default=0.0)
+    repo_scheduled: int = Field(default=0)
     role_id: str = Field(default="")
     role_name: str = Field(default="")
     scheduled_perms: List[str] = Field(default=[])
@@ -88,7 +88,6 @@ class Role(BaseModel):
     _dirty: bool = PrivateAttr(default=False)
     _default_exclude: Set[str] = {
         "role_id",
-        "role_name",
         "account",
         "config",
         "_dirty",
@@ -119,12 +118,16 @@ def datetime_normalize(cls, v: datetime.datetime) -> datetime.datetime:
 
     def add_policy_version(
         self, policy: Dict[str, Any], source: str = "Scan", store: bool = True
-    ) -> None:
+    ) -> bool:
+        if not policy:
+            logger.debug("no policy provided, not adding")
+            return False
         if self.policies:
             last_policy = self.policies[-1]["Policy"]
-            if policy == last_policy:
+            last_source = self.policies[-1]["Source"]
+            if policy == last_policy and source == last_source:
                 # we're already up to date, so this is a noop
-                return
+                return False
         policy_entry = {
             "Source": source,
             "Discovered": datetime.datetime.now().isoformat(),
@@ -134,6 +137,7 @@ def add_policy_version(
         self._calculate_no_repo_permissions()
         if store:
             self.store(fields=["Policies", "NoRepoPermissions"])
+        return True
 
     def calculate_repo_scores(self, minimum_age: int, hooks: RepokidHooks) -> None:
         (
@@ -179,6 +183,8 @@ def get_permissions_for_policy_version(
         )
 
     def _calculate_no_repo_permissions(self) -> None:
+        if not self.policies:
+            return
         try:
             previous_policy = self.policies[-2]
         except IndexError:
@@ -243,7 +249,7 @@ def _stale_aa_services(self) -> List[str]:
         stale_services = []
         if self.aa_data:
             for service in self.aa_data:
-                if ts_parse(service["lastUpdated"]) < thresh:
+                if ts_parse(service["lastUpdated"], ignoretz=True) < thresh:
                     stale_services.append(service["serviceName"])
         return stale_services
 
@@ -258,7 +264,7 @@ def update(self, values: Dict[str, Any], store: bool = True) -> None:
         self._dirty = True
         self._updated_fields.update(values.keys())
         temp_role = Role(**values)
-        role_data = temp_role.dict(exclude_unset=True, exclude=self._default_exclude)
+        role_data = temp_role.dict(exclude=self._default_exclude)
         for k, v in role_data.items():
             setattr(self, k, v)
         if store:
@@ -296,12 +302,8 @@ def fetch(self, fields: Optional[List[str]] = None, update: bool = True) -> None
             )
 
         if update:
-            temp_role = Role(**stored_role_data)
-            role_data = temp_role.dict(
-                exclude_unset=True, exclude=self._default_exclude
-            )
-            self.update(role_data, store=False)
-            self._updated_fields - set(role_data.keys())
+            self.update(stored_role_data, store=False)
+            self._updated_fields - set(stored_role_data.keys())
 
     def mark_inactive(self, store: bool = True) -> None:
         self.active = False
@@ -326,7 +328,11 @@ def store(self, fields: Optional[List[str]] = None) -> None:
 
         if create:
             # TODO: handle this case in set_role_data() to simplify logic here
-            create_dynamodb_entry(self.dict(exclude_unset=True, by_alias=True))
+            create_dynamodb_entry(
+                self.dict(
+                    by_alias=True, exclude={"config", "_dirty", "_updated_fields"}
+                )
+            )
             self._updated_fields = set()
         else:
             if fields:
@@ -344,9 +350,7 @@ def store(self, fields: Optional[List[str]] = None) -> None:
             else:
                 set_role_data(
                     self.role_id,
-                    self.dict(
-                        exclude_unset=True, by_alias=True, exclude=self._default_exclude
-                    ),
+                    self.dict(by_alias=True, exclude=self._default_exclude),
                 )
                 self._updated_fields = set()
         self._dirty = False
@@ -361,9 +365,12 @@ def gather_role_data(
         store: bool = True,
     ) -> None:
         config = config or CONFIG
+        self.fetch()
         self.fetch_aa_data()
-        self.add_policy_version(current_policy, source=source, store=False)
-        if add_no_repo:
+        policy_added = self.add_policy_version(
+            current_policy, source=source, store=False
+        )
+        if policy_added and add_no_repo:
             self._calculate_no_repo_permissions()
         self._update_opt_out()
         self._update_refreshed()

tests/test_commands.py

@@ -435,14 +435,16 @@ def test_schedule_repo(
         # first role is not repoable, second role is repoable
         test_roles = [
             ROLES_FOR_DISPLAY[0].copy(
-                update=Role(**{"RoleId": "AROAABCDEFGHIJKLMNOPA"}).dict(
-                    exclude_unset=True
-                )
+                update=Role(**{"RoleId": "AROAABCDEFGHIJKLMNOPA"}).dict()
             ),
             ROLES_FOR_DISPLAY[1].copy(
                 update=Role(
-                    **{"RoleId": "AROAABCDEFGHIJKLMNOPB", "AAData": [{"foo": "bar"}]}
-                ).dict(exclude_unset=True)
+                    **{
+                        "RoleId": "AROAABCDEFGHIJKLMNOPB",
+                        "AAData": [{"foo": "bar"}],
+                        "RepoablePermissions": 10,
+                    }
+                ).dict()
             ),
         ]
         mock_role_list_from_ids.return_value = RoleList([test_roles[0], test_roles[1]])

tests/test_role.py

@@ -103,12 +103,11 @@ def test_role_add_policy_version_duplicate(
     mock_store, mock_calculate_no_repo_permissions, role_dict
 ):
     r = Role(**role_dict)
-    source = "Test"
+    source = "Fixture"
     fake_policy = vars.policies[0]["Policy"]
     assert len(r.policies) == 1
     r.add_policy_version(fake_policy, source=source, store=True)
     assert len(r.policies) == 1
-    assert r.policies[0]["Source"] != source
     assert r.policies[0]["Policy"] == fake_policy
     mock_calculate_no_repo_permissions.assert_not_called()
     mock_store.assert_not_called()
@@ -512,7 +511,6 @@ def test_role_store(
 ):
     expected = copy.deepcopy(role_dict_with_aliases)
     expected.pop("RoleId")
-    expected.pop("RoleName")
     expected.pop("Account")
     mock_get_role_by_id.return_value = {
         "LastUpdated": vars.last_updated.strftime("%Y-%m-%d %H:%M")