Workaround for #557 Defer GUID resolution

smoy commented 1 year ago

What changed?

Handle list_users and list_groups not working for identity center store due to current integration limit with AD integration
if we cannot fetch users and group in the beginning, wait til encounter of GUID, on-the-fly use describe_user or describe_group API to resolve GUID. (with simple caching decorator to minimize API hit)

Rationale

When identity center store is integrate with AD, some AD setup refuse to propagate list_users and list_groups with a filter. (this has to do with how identity center store service team has implement the integration. boto3 sdk already allow for no filter and it works with builtin identity center store directory.

How was it tested?

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

[ ] Unit Tests
[ ] Functional Tests
[x] Manually Verified

how to verify: I simulate the issue by just not calling the actual list users and list groups.

this is likely only work for import. it will take some thoughts on how to implement change things from iambic-templates without user and group knowledge

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 53.84% and project coverage change: -0.16% :warning:

Comparison is base (bec7490) 83.84% compared to head (c1032c2) 83.69%. Report is 1 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #558 +/- ## ========================================== - Coverage 83.84% 83.69% -0.16% ========================================== Files 105 105 Lines 12280 12318 +38 ========================================== + Hits 10296 10309 +13 - Misses 1984 2009 +25 ``` | Flag | Coverage Δ | | |---|---|---| | functional_tests | `65.95% <53.84%> (-0.14%)` | :arrow_down: | | functional_tests_config_discovery | `46.07% <48.71%> (+0.12%)` | :arrow_up: | | unit_tests | `73.93% <33.33%> (-0.13%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=noqdev#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files Changed](https://app.codecov.io/gh/noqdev/iambic/pull/558?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=noqdev) | Coverage Δ | | |---|---|---| | [...v0\_1\_0/aws/identity\_center/permission\_set/utils.py](https://app.codecov.io/gh/noqdev/iambic/pull/558?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=noqdev#diff-aWFtYmljL3BsdWdpbnMvdjBfMV8wL2F3cy9pZGVudGl0eV9jZW50ZXIvcGVybWlzc2lvbl9zZXQvdXRpbHMucHk=) | `89.70% <41.66%> (-4.60%)` | :arrow_down: | | [iambic/plugins/v0\_1\_0/aws/models.py](https://app.codecov.io/gh/noqdev/iambic/pull/558?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=noqdev#diff-aWFtYmljL3BsdWdpbnMvdjBfMV8wL2F3cy9tb2RlbHMucHk=) | `88.14% <63.63%> (-0.72%)` | :arrow_down: | | [...0\_1\_0/aws/identity\_center/permission\_set/models.py](https://app.codecov.io/gh/noqdev/iambic/pull/558?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=noqdev#diff-aWFtYmljL3BsdWdpbnMvdjBfMV8wL2F3cy9pZGVudGl0eV9jZW50ZXIvcGVybWlzc2lvbl9zZXQvbW9kZWxzLnB5) | `84.84% <100.00%> (+0.08%)` | :arrow_up: | | [...ntity\_center/permission\_set/template\_generation.py](https://app.codecov.io/gh/noqdev/iambic/pull/558?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=noqdev#diff-aWFtYmljL3BsdWdpbnMvdjBfMV8wL2F3cy9pZGVudGl0eV9jZW50ZXIvcGVybWlzc2lvbl9zZXQvdGVtcGxhdGVfZ2VuZXJhdGlvbi5weQ==) | `96.23% <100.00%> (+0.04%)` | :arrow_up: | ... and [3 files with indirect coverage changes](https://app.codecov.io/gh/noqdev/iambic/pull/558/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=noqdev)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

smoy commented 1 year ago

Some extra feedback as we test this on AD + identity center setup. Event describe_user and describe_group can fail during PrincipalID resolution (we are referencing principalID and GUID loosely, principalID is the term Identity center document uses).

the feedback is in those situation, maybe we want to provide extra configuration for user to simply capture the permission set configuration without the PrincipalID resolution. One further step is to configure to simply capture the permission set configuration without the assignment. so the assignment can be configure out-of-band.

smoy commented 1 year ago

well, this is actually not a bad improvement. even for existing identity center usage, there is a race condition between fetch_users_and_groups and the subsequent permission center ingestion. because there is no speak of transaction. looking users and group ahead of time does not help if the moment we ingest permission center assignment, it contains PrincipalID that are outside the known range. This PR already address the defer lookup and error handling, so we should merge this PR.

noqdev / iambic