CIEM-718: speed up iam_ape effective-permissions (SCP-deny caching + skip never-read denied report section) - #39
CIEM-718: speed up iam_ape effective-permissions (SCP-deny caching + skip never-read denied report section)#39yanas-orca wants to merge 20 commits into
Conversation
…on caching) Enabling the SCP-hierarchy feature flag spiked aws_effective_permissions ~8x on conditional-SCP accounts. Root cause: the account-fixed SCP deny conditions (few distinct — 249 on the btg repro) are re-negated and re-wrapped millions of times across principals, and managed-policy/boundary expansions are recomputed per role. All changes are OUTPUT-IDENTICAL: the effective-permission set is hash-identical to baseline across three account profiles (btg conditional-SCP, real954 expansion-heavy, tempus no-SCP) plus edge fixtures; 38/38 unit tests pass. Measured on the local btg corpus: deny path -63.5%, expansion-heavy account -29.3%. - helper_classes: cache the hash of HashableDict/HashableList (immutable by construction) so condition-keyed caches are O(1); short-circuit recursively() on already-converted dicts. - expand_policy: instance-scoped expansion cache for condition-free managed-policy (ARN-source) expansions, shared across principals; get_action_access_levels memo; shrink_policy copy-before-merge (no in-place condition aliasing). - evaluator: account-fixed SCP deny caches shared across entities (a should_deny RESULT cache and a merge_condition cache), plus create_json_report merge-skip. All caches are per-evaluator (per-account scan), so a refreshed actions DB starts clean.
![orca-security-eu[bot]](https://avatars.githubusercontent.com/in/276244?s=60&v=4){kind=small}
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
 Passed |
Secrets |  0  0  0  0 |
View in Orca |
| Passed |
Vulnerabilities | 0 0 0 0 |
View in Orca |
The denied_permissions report section re-serializes the account-constant SCP deny expansion per principal (~2.94M actions/role on conditional-SCP accounts), which dominates handler runtime (~80% on btg). No orca consumer reads it: the graph parser and UI/GraphQL both read only allowed_permissions + ineffective_permissions. Add include_denied_permissions (default True preserves the CLI report and the e2e golden); callers that don't need it opt out. allowed + ineffective stay byte-identical. Measured: btg full-account ~1926s -> ~449s (-77%), real954 ~4016s -> ~2409s (-40%).
- create_json_report sections built as a single conditional expression (clearer intent) - test_include_denied_permissions_preserves_read_sections: asserts allowed + ineffective are canonically identical with/without the denied section, and denied is emptied when skipped. Locks the safety property permanently.
yanas-orca
changed the title
Fixes CI black check (evaluator.py) and condenses the added comments to short single lines for a cleaner review. No behavior change.
- Revert should_deny to master's body (drop the merge-cache wrapper); move the account-fixed deny-result cache into explicitly_deny. Deny logic is byte-identical to master, so no change to computed permissions. - Extract expand_action/expand_not_action expansion bodies into helpers so the public methods are thin cache wrappers. No behavior change (e2e golden + 39 tests pass).
deniss-orca
left a comment
deniss-orca
left a comment
There was a problem hiding this comment.
Review of the caching pass. I verified each point against a local checkout by running scripts on this branch and on origin/main side by side, plus checking how clouder actually calls this library (scan_backend/services/clouder/clouder_handlers/account/aws/aws_effective_permissions.py). Inline comments below.
The short version: there is one correctness root cause with an amplifier and a mask, and the performance premise does not hold up on measurement.
Correctness. The HashableDict.recursively short-circuit removes the copy-on-convert guarantee for Action.condition, so condition dicts are now aliased across principals and shrink_policy's trailing normalize_policy writes into the evaluator's own shared SCP state. Passing merge_cache to the per-entity boundary makes one condition object reachable from every principal in the account. The memoized __hash__ on still-mutable dict/list subclasses then hides the resulting corruption as wrong statement grouping instead of raising. This matters specifically because clouder builds one evaluator per account and loops over every user, group and role, so results become dependent on scan order.
Performance. Measured, 3 interleaved runs of 12 principal-evals on tests/test_data: origin/main 1.44/1.40/1.38 s versus this branch 1.66/1.69/1.69 s, about 20% slower and reproducible. The SCP-deny cache took 23,263 probes with 0 hits on the repo's own multi-principal fixture, because the Action dataclass hash includes source (policy ARN plus Sid). The four new caches are never bounded or cleared, retaining about 19 MB per account scan that main released per principal. The one change that does pay is include_denied_permissions=False (report phase 0.53 s to 0.23 s, a 57% cut), and no caller passes it.
Not inline because the file is not in the diff: iam-ape/pyproject.toml is still at version = "1.1.8", identical to origin/main. Every prior functional change to iam-ape bumped it (git log -p --follow -- iam-ape/pyproject.toml shows 1.0.2 through 1.1.8, including the last two iam-ape commits c06f5ac and 30d8c17). The main orca repo pins iam-ape>=1.1.0,<1.2, so clouder installs the published wheel rather than this source tree. Two consequences: publishing is blocked because PyPI refuses to re-upload an existing version, and the follow-up clouder change this PR advertises would call create_json_report(..., include_denied_permissions=False) against the installed 1.1.8 and raise TypeError: unexpected keyword argument, once per principal, caught by the outer except Exception and losing the whole account's AwsEffectivePermissionsPolicy model.
Happy to walk through any of the repro scripts.
| def recursively(cls, dict_obj: Optional[Dict[Any, Any]]): | ||
| if dict_obj is None: | ||
| return None | ||
| if isinstance(dict_obj, HashableDict): |
There was a problem hiding this comment.
This is the one I would block on. The short-circuit removes the copy-on-construct isolation of Action.condition, so one principal's shrink_policy can rewrite the Condition reported for every principal evaluated after it.
Repro, run against both branches: build an EffectivePolicyEvaluator with an SCP that has not been through normalize_policy (which is exactly what this library's own CLI does at main.py:126-160 in load_scp_from_json / load_scp_from_aws, and what tests/test_e2e.py:88-99 does) carrying a scalar condition value, {"StringEquals": {"aws:RequestedRegion": "us-east-1"}}. Two roles R1 and R2, then evaluate(R1) -> shrink_policy(R1) -> evaluate(R2).
On this branch R1's condition sub-dict is the same object as self.scp_policy's condition sub-dict (my probe prints inner shared: True). shrink_policy's closing normalize_policy (helper_functions.py:112-119) then writes statement["Condition"][op][key] = [value] straight into it, and afterwards the evaluator's own scp_policy reads {'aws:RequestedRegion': ['us-east-1']}, which is also what R2 now sees. On origin/main the identical script prints inner shared: False and the SCP is untouched.
Since clouder reuses one evaluator for every principal in an account, the uploaded effective-permissions blobs become order-dependent. Returning a copy on the hot path, or making the conversion genuinely immutable, would both fix it.
|
|
||
| def __hash__(self) -> int: # type: ignore[override] | ||
| return hash(tuple(sorted(self.items()))) | ||
| if self._hash is None: |
There was a problem hiding this comment.
Memoizing the hash here is what makes the aliasing bug above fail quietly instead of loudly. HashableDict and HashableList are still mutable dict/list subclasses, so the comment above ("Immutable by construction") holds only as long as nothing writes to them, and normalize_policy does exactly that.
After such a write the object is genuinely unhashable, but the cached _hash keeps returning the pre-mutation value. shrink_policy keys on the condition hash twice, at by_resource_notresource[...][HashableDict.recursively(condition)] and at final_statements[statement_key], so a stale-hashed condition and an equal freshly built one land in different buckets and statements that should merge are emitted separately. That is the 3 to 4 statement split I saw.
Evidence that the memo is masking real corruption rather than preventing it: if I remove only the memoization from both __hash__ methods and change nothing else on this branch, the same run becomes a hard crash, TypeError: unhashable type: 'list' from evaluator.py:275 (if key in merge_cache). Worth noting where that lands in production: clouder's per-entity handler catches (KeyError, ValueError) at aws_effective_permissions.py:196, not TypeError, so it would escape to the outer handler and abort effective permissions for the entire account rather than skipping one entity.
| more_ineffective_permissions, | ||
| ) = apply_permission_boundary(final_permissions, boundary) | ||
| ) = apply_permission_boundary( | ||
| final_permissions, boundary, merge_cache, deny_result_cache |
There was a problem hiding this comment.
deny_result_cache just above is carefully gated on boundary is self.scp_policy, with the comment "Only the account-fixed SCP is cacheable; the per-entity boundary is not", but merge_cache (self._deny_merge_cache, built at line 517 and itself described as an account-fixed SCP deny cache) is handed to the per-entity permission-boundary pass unconditionally.
merge_condition is pure in its arguments so the memoized value is correct in isolation. The problem is what happens to it afterwards: permit() at line 284 does replace(at, condition=cond) with the cached object, and since Action.__post_init__ no longer copies (see helper_classes.py:47), a single condition dict becomes reachable from the allowed_permissions of every principal in the account for the whole scan. That is the amplifier that turns one in-place write during principal U0's shrink_policy into a wrong statement count for the others. With a mixed fixture (6 users, permission boundary on every other one, unnormalized SCP) I get U0 -> 3 statements, U1..U4 -> 4, U5 -> 3.
Either gate merge_cache the same way as deny_result_cache, or fix the copy at the source and update the comment, because as written it tells a reviewer that per-entity state is not shared when it is.
| return cached | ||
| res = self._expand_action(iam_action) | ||
| if cache_key is not None: | ||
| result = dict(res) |
There was a problem hiding this comment.
Two separate contract changes on the public expand_action / expand_not_action here.
First, the return type now depends on the input. The cacheable path takes result = dict(res) (here and at :309) and returns a plain dict, while an inline-policy source or a conditioned statement still returns defaultdict(set). Verified on this branch: source="MyInlinePolicy/0" returns a defaultdict and res["nope:Action"].add(1) succeeds, whereas source="arn:aws:iam::aws:policy/ReadOnlyAccess/0" returns a plain dict and the same expression raises KeyError. Both signatures still annotate Dict[str, Set[Action]], so nothing flags it. Any caller written against the old behavior now works for inline policies and raises for managed policies and permission boundaries. In clouder that KeyError is swallowed by except (KeyError, ValueError) at aws_effective_permissions.py:197, which logs model data loss and continues, so one principal's permissions are silently never uploaded while the rest of the account succeeds.
Second, return cached (lines 240 and 306) hands back the cache's own dict, and dict(res) is shallow so the Set[Action] values are the cached sets. A caller that adds to the returned mapping or to one of its sets poisons that managed policy's expansion for every principal evaluated afterwards. Nothing in this repo trips it today since expand_policies only reads, but PolicyExpander is the object clouder constructs and passes in (aws_effective_permissions.py:151 and :178), so the change is exported.
| access = self.all_iam_actions[service][action_key]["access"] | ||
| return [level.strip() for level in access.split(",")] if access else [] | ||
| result = [level.strip() for level in access.split(",")] if access else [] | ||
| self._access_levels_cache[action] = result |
There was a problem hiding this comment.
This caches and returns the mutable list itself rather than a copy. On this branch two calls for s3:GetObject return the same object (lv is lv2 is True; False on main), and lv.append('Write') makes the next get_action_access_levels('s3:GetObject') return ['Read', 'Write'].
create_json_report keys the report by access level, so one caller-side mutation permanently misfiles that action: s3:GetObject would start appearing under Write for every principal serialized afterwards, and the graph edges built from that JSON would claim write access the policy does not grant. No in-repo caller mutates it today, so this is defense in depth rather than a live bug, but return list(cached) costs almost nothing here.
| denied, new_action_values, denied_by = should_deny( | ||
| action_value, permissions.denied_permissions | ||
| ) | ||
| if result_cache is not None and action_value in result_cache: |
There was a problem hiding this comment.
I could not get this cache to hit. A hit requires two principals to hold a byte-identical Action, and the dataclass hash includes source, which is the granting policy ARN plus Sid.
Instrumented explicitly_deny on this branch and ran all six principals of tests/test_data/test_account_authorizations_details.json through one shared evaluator (the clouder pattern, aws_effective_permissions.py:184): 23,263 probes, 0 hits, 23,263 misses, and 23,263 entries retained for the whole scan. Re-keying on (action, resource, not_resource, condition) with source dropped only gets to 313 hits, or 1.3%.
So on the only multi-principal input the repo ships, the memo pays full retention cost and never once serves a cached verdict. The payoff rests entirely on an unmeasured assumption about how much real accounts reuse the exact same granting statement across principals. If you have production numbers showing that reuse, they would settle this, and they belong in the PR description. If not, dropping source from the key is the change that would make the cache do anything at all.
| ) | ||
| # Account-fixed SCP deny caches, shared across principals within one account scan. | ||
| self._deny_merge_cache: Dict[Any, Any] = {} | ||
| self._scp_deny_result_cache: Dict[Any, Any] = {} |
There was a problem hiding this comment.
Measured on the repo's own e2e fixture, the caching half of this PR is a regression, not a speedup. 3 interleaved runs of 12 principal-evals (evaluate + shrink_policy + create_json_report) on tests/test_data: origin/main 1.44/1.40/1.38 s, this branch 1.66/1.69/1.69 s. About 20% slower, reproducible.
Per phase: evaluate 1.13 to 1.17 s, shrink 0.15 to 0.19 s, report(full) 0.54 to 0.53 s. The per-action tuple-key construction and extra dict lookups, plus GC pressure from the 4x retained memory, outweigh a hit rate that is zero on this workload.
I recognize the fixture may not resemble the accounts that motivated the PR, and a workload where these caches pay off plausibly exists. But there is no benchmark or measured number in the PR, so right now the shared-mutable-state complexity is unpaid-for on the only workload either of us can measure here. A before and after on a real account would change my read of this entirely.
| self, permissions_container: PermissionsContainer | ||
| self, | ||
| permissions_container: PermissionsContainer, | ||
| include_denied_permissions: bool = True, |
There was a problem hiding this comment.
This parameter defaults to True and no caller ever passes False, so the saving in the PR title is currently dead code. The only two production callers are clouder aws_effective_permissions.py:191 (policy_calc.create_json_report(effective_policy)) and iam_ape/main.py:318 (calculator.create_json_report(res)), and grepping the orca checkout finds no include_denied_permissions anywhere.
Which is a shame, because measured, this is the one clear win in the PR: report(full) 0.53 s versus report(no-denied) 0.23 s for 12 principals, a 57% cut in the report phase, with none of the shared-state risk that the caches carry. The added branch and the new 37-line test currently guard behavior nobody reaches.
I checked the consumer side: graph_build/shared/effective_permissions_mapper.py reads only allowed_permissions and ineffective_permissions, so landing the matching clouder change looks safe. That plus the version bump would make this PR worth it on its own, even if the caches came out.
| return obj | ||
|
|
||
|
|
||
| def test_include_denied_permissions_preserves_read_sections() -> None: |
There was a problem hiding this comment.
The entire risk surface of this PR is cross-principal cache sharing, and no test exercises it: every test here, new and existing, evaluates exactly one principal per EffectivePolicyEvaluator, while clouder shares one evaluator across every principal in an account.
Concretely, the order-dependent regression I hit does not fail this suite. DevOps_admin yields 2 statements evaluated alone and 3 when evaluated after other principals through the same evaluator, and the full suite stays green either way, because nothing calls evaluate() twice on one evaluator and compares against the single-principal result.
A test that evaluates two principals through one evaluator and asserts each matches its standalone result would have caught the aliasing bug, and would keep catching it as the caches change.
| os.path.join(os.path.dirname(__file__), "test_data/test_scp_policy_1.json") | ||
| ) as f: | ||
| scp_data = json.load(f) | ||
| scp_policies = [ |
There was a problem hiding this comment.
The SCP fixture makes this new test weaker than it looks. tests/test_data/test_scp_policy_1.json decodes to {"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"*","Resource":"*"}]}, with no Deny at all.
Measured on this branch: len(evaluator.scp_policy.denied_permissions) == 0, so the SCP-stage explicitly_deny calls should_deny(action, {}) and the for denied_action in denied_actions.get(...) loop body never runs. 1,097 entries get stored for the first principal and every one of them is the trivial (False, {action}, None). So the cache this PR is named for is never exercised against a real SCP deny.
This is a distinct gap from the one-principal-per-evaluator issue: adding a second principal to this fixture still would not help, because the memoized values carry no deny information. Any regression in the partial-deny paths of should_deny, specifically the res.add(Action(..., condition=merge_condition(...))) branches that are exactly what the cache now shares between principals, would ship green.
The fixture also diverges from production a second way: clouder wraps SCP content in normalize_policy(...) (aws_effective_permissions.py, get_scp_policies) and this test does not, so it feeds a scalar "Action": "*" that only survives for action in actions because it happens to be one character long. A deny-bearing, normalize_policy'd SCP fixture would fix both.
- Condition-aliasing fix (correctness): shrink_policy now normalizes a COPY, so normalize_policy can never mutate a condition that is shared across principals via the caches (it used to rewrite a scalar SCP condition into a list, corrupting the shared scp_policy for every later principal). Root fix - keeps the recursively short-circuit and its performance. Regression tests added (proven to fail on the bug): test_shrink_does_not_mutate_shared_scp + test_shared_evaluator_matches_standalone, both with a real Deny SCP (previously an untested path). - Bound the per-account caches (BoundedDict, 100k FIFO) to remove the OOM risk. - expand_action/expand_not_action return copies (consistent dict type, no shared mutable output); get_action_access_levels returns a list copy. - Bump version 1.1.8 -> 1.1.9 so the wheel publishes and the pin-bump can pass include_denied_permissions=False. 41 tests; black/isort/flake8/mypy clean.
Only deep-copy a statement's Condition before normalize_policy when it actually holds a scalar value (the sole case normalize_policy mutates). Already-normalized conditions - the production path, since clouder normalizes SCPs - are left untouched. Heaviest btg role: 0 copies, ~8ms scan vs ~340ms unconditional copy. Correctness unchanged (regression tests still pass).
…review round 2) - Deny cache keys on (action, resource, not_resource, condition) minus source, guarded by a per-account denied-sources map so should_deny's exact-equality shortcut provably can't fire; the dominant pass-through verdict is stored as a zero-Action sentinel and every retrieved set is re-stamped with the caller's source. Collapses the diverse-source cache (40 principals x 12 actions: 480 -> 12 entries, 480 -> 4 retained Actions) and finally makes it hit. - Caches bounded by retained Action count via a clear-on-overflow circuit breaker (CircuitBreakerCache) instead of a key-count FIFO that neither bounded the Set[Action] memory nor avoided a per-insert eviction cliff. - Normalize SCPs in the CLI loaders so the "inputs are normalized" invariant the deny path relies on holds uniformly (not just on the clouder path). - Fix the misleading merge_cache/deny-cache comment in evaluate(). Tests: differential proving the source-stripped shared cache reproduces per-principal standalone output incl. source (proven to fail without re-stamp); circuit-breaker clear-on-overflow. 43 passed; mypy/isort/black/flake8 clean.
Splits the cache logic out of explicitly_deny (cognitive complexity 33->~7) into _cached_deny_verdict, and the SCP-cache selection out of evaluate (17->pre-change) into _scp_caches_for. cast(Action, replace(...)) in permit fixes the S5886 return-type nit now in scope. Behavior unchanged; 43 tests + mypy/isort/black/flake8 clean.
…source-guard (review round 3) F1 (measured on btg): the deny cache reaches the cap inside the working set (~11M Actions by p45 of 275 principals), so the overflow policy matters. Clear-on-overflow thrashed (~+18% wall rebuilding the hot shared verdicts). Switch to stop-insert at the cap (CappedMemoCache, renamed from CircuitBreakerCache): a new key past the cap is skipped and recomputed on the next lookup (pure memo -> correctness-neutral), the hot early entries stay cached, no hole-scan, monotonic counter. Cap lowered 4M -> 1M (~100-250MB/cache; each cache independent, so combined ceiling is 2M). F3: weigh every entry >=1 (sentinel=1, tuple=1+len(set)) so zero-retention entries (pass-through sentinels, full denies) are still bounded by key count. Note: the 480->4-retained-Actions collapse is a narrow-deny fixture number; on btg the conditional SCP denies broadly, so sentinels don't fire and the win is the entry collapse + the cap. F2: white-box test that the source-guard refuses to cache when the action's source is one of that action's denied sources (the exact-equality shortcut could fire) -- the whole soundness argument for the source-stripped key, previously untested (proven to fail if the guard is removed). F5: override clear() to reset the counter; document the insert-once/overwrite contract. 44 tests; mypy/isort/black/flake8 clean.
…s (review round 4) The +1.6 GB the caching added on btg is real and explained: the deny cache retains condition-bearing Actions, and a merged condition (HashableDict) makes an Action ~1.8 KB transitively vs ~300 B condition-free (measured via tracemalloc). So a 1 M Action-count cap let the deny cache reach ~1.8 GB. The weight counter is correct (it counts exactly what's retained, no hidden graph) -- the cap was just sized in the wrong unit. Split the single cap into per-cache ceilings sized from the measured footprint: EXPANSION_CACHE_MAX_WEIGHT=1M (~300 MB at ~300 B/Action) and DENY_CACHE_MAX_WEIGHT=170k (~300 MB at ~1.8 KB/Action). Combined retained caching footprint ~0.6 GB, down from ~2.1 GB. Add EffectivePolicyEvaluator.cache_stats() (per-cache entries/weight/capped) so the handler can log live cache footprint next to RSS -- prod visibility this PR otherwise lacks. 45 tests; mypy/isort/black/flake8 clean.
… net-negative) On the real btg account (273 roles, 236 SCPs — the deny cache's best case) the deny result cache buys 0 wall time and costs +104 MB peak RSS: its (action, resource, condition) keys are mostly distinct per principal, so cross-principal reuse is near zero, while it retains ~1.8 KB condition-bearing Actions. The round-2 "480->12, now it hits" was a synthetic fixture with artificial reuse; real workloads don't have it. (tempus has 0 SCPs, so it can't help there either.) Deleting it improves every axis at once: -104 MB, no runtime cost, ~180 fewer lines, and it removes the subtlest invariant in the PR — the source-strip soundness guard — along with the pass-through sentinel, the re-stamp, _scp_caches_for, DENY_CACHE_MAX_WEIGHT, the verdict tuple and the Optional-threading. explicitly_deny/apply_permission_boundary revert to master's plain form (incl. the should_deny signature); SonarCloud no longer flags either. The surviving merge cache is renamed _condition_merge_cache (it has nothing to do with denies now) and the CappedMemoCache docstring/cap comments drop the stale deny-cache prose. Kept (measured wins): the condition-merge cache (+53 s / +829 MB without it), the expansion cache (+899 MB without it), the recursively short-circuit + memoized hashes (>=2.8x without), the denied-permissions skip, and the shrink condition-copy fix. 43 tests; lint/mypy clean.
should_deny merges every allowed-action condition against the (few, fixed) SCP deny conditions, tens of millions of times with a tiny distinct-input set. Memoize the hashable path of merge_condition (pure function): on btg's full account, 184,007,341 calls collapse to 19,046 distinct computations, cutting evaluate 455.5s -> 249.4s (-45%) and total 858.7s -> 632.4s (-26%). Output-identical (pure; the shared HashableDict is read-only, the same sharing permit already relied on). - Gated to hashable=True + HashableDict|None args, so create_json_report's hashable=False / possibly-plain-dict call bypasses it (a hash TypeError there would escape to the account-level handler). Key is (allow, deny, negate). - Process-global lru_cache(100k): a deliberate exception to the per-account cache discipline, sound only because merge_condition is pure; documented in the memo docstring. - The per-account permit merge cache is now a cache in front of a cache: deleted it, so apply_permission_boundary is back to master's 2-arg signature (last threaded optional gone). 43 tests; mypy/isort/black/flake8 clean.
…nalize create_json_report ended with res = json.loads(json.dumps(res, default=serialize_set)) to turn defaultdicts->dicts and sets->lists. Replace it with an in-place walk. On btg's full account this cuts the report phase and total 632.4s -> 568.4s (-10%), and -215 MB peak. - In place, not a copy: the round-trip transiently held the original + a ~330 MB serialized string + a parallel parsed copy; the walk mutates res so the peak holds one structure. - sets -> lists is MANDATORY, not cosmetic: the S3 upload path's json handler has no set branch and falls through to str(obj), which would write every source / denied_by / NotResource as a Python-repr string instead of a JSON array and silently corrupt the blob (graph parser + gql read source as a list). Sets stay sets during the build for dedup and are converted only at the end, unchanged from before. - defaultdict factories cleared so the report reads like the plain dict the round-trip produced; HashableDict/HashableList conditions are left as-is (serialize as dict/list). Output-identical at the JSON level; golden tests (test_e2e, test_include_denied_...) pass. 43 tests; mypy/isort/black/flake8 clean.
…lize (review round 6) - F1: clear _merge_condition_memo per EffectivePolicyEvaluator and report it in cache_stats() under 'merge'; the memo is module-global but now released at each account boundary (pure fn, so correctness is unaffected either way). - F2: shrink_policy copies a scalar condition into a PLAIN dict/list (_to_plain), not copy.deepcopy - a deepcopy'd HashableDict keeps a stale memoized _hash that normalize_policy's in-place rewrite would leave inconsistent, splitting equal conditions across hash buckets. Drop the now-unused 'import copy'. - F5: guard the copy-on-write predicate with isinstance(operator_dict, dict) so a malformed non-dict operator value can't raise and lose the whole account. - F6: CappedMemoCache.update()/setdefault() route through __setitem__ so neither can bypass the weight cap. - F3: test_create_json_report_emits_no_sets asserts no set survives _finalize anywhere in the report (the S3 uploader str()s a stray set instead of raising). - F7: test_shared_evaluator_matches_standalone now compares the full create_json_report per principal, not just the shrunk policy.
info.maxsize is Optional[int] (None for lru_cache(maxsize=None)); currsize >= None would TypeError inside the diagnostics path if the memo were ever unbounded. An unbounded memo is never capped, so guard the comparison explicitly. No behaviour change today (maxsize=100_000), closes a latent wart in new code.
…account memo clear reset_merge_condition_cache() wipes a module-global memo; that is only safe because clouder dispatches account handlers sequentially (one live evaluator at a time). Note it so a future concurrent dispatch is understood as a perf cliff (cold memo), not a correctness bug (the memo is pure).
|
2 participants
Summary
Enabling the SCP-hierarchy feature flag spiked
aws_effective_permissions~8× on conditional-SCP accounts (CIEM-718). This PR recovers that entirely insideiam_ape. By measured attribution the win is not mostly the caches — it's ~15 lines of hashing:HashableDict/HashableListshort-circuit + memoized hash (~15 lines) — worth 3.7× runtime and 6.1 GB on its own. This is the highest-value and highest-risk change: it shares condition objects across principals, which is precisely the aliasing surface guarded bytest_shared_evaluator_matches_standalone,test_shrink_does_not_mutate_shared_scp, and theshrink_policycopy-on-write. Review attention belongs here.denied_permissionsreport section (opt-in) — the dominant win on report-heavy accounts.All three leave the computed permission set unchanged — with one deliberate exception that is a bug fix, not a regression (see next).
Changes
1. The
HashableDictshort-circuit + memoized hash — the primary lever (and the risk)helper_classes— memoize the hash ofHashableDict/HashableList(so condition-keyed lookups are O(1)) and short-circuitrecursively()on an already-converted dict instead of rebuilding it. ~15 lines; 3.7× runtime and 6.1 GB on their own. The short-circuit returns the same condition object rather than a fresh copy, so conditions become shared across principals — the highest-value change is therefore also the one that creates the aliasing surface. That surface is held by theshrink_policycopy-before-merge (no in-place condition mutation) and two regression tests:test_shrink_does_not_mutate_shared_scp— the tight guard. Asserts the sharedscp_policyconditions are byte-identical before and after ashrink_policycall. Verified to fail on the pre-copy-on-write code (the scalar SCP condition is rewritten'us-east-1'→['us-east-1']in place, corrupting it for every later principal on the reused evaluator).test_shared_evaluator_matches_standalone— the broad end-to-end guard. Asserts that one evaluator reused across principals (the clouder pattern) yields, per principal, exactly what a fresh per-principal evaluator yields — across bothshrink_policy(Contract B) and the fullcreate_json_report(Contract A). It passes on both the pre-fix and post-fix code (the mutation it would catch is already blocked by the copy-on-write); it exists to catch future cache-aliasing regressions on either contract, not to reproduce a currently-failing case.2. Two per-account caches — together ~10% runtime / ~1.7 GB
Released at the account boundary (a refreshed actions DB starts clean). Kept because ablation on real btg showed they dedupe small shared objects (cutting allocation churn, which sets peak RSS):
get_action_access_levelsmemo. Instance-scoped on thePolicyExpander. ACappedMemoCachebounds it by measured retained-Action bytes: past the cap it stops inserting (recompute-on-miss → correctness-neutral) rather than growing toward OOM. Itsupdate()/setdefault()route through__setitem__so the cap can't be bypassed._merge_condition_memo), the engine of this PR. It keys on(allow_cond, deny_cond, negate), and both sides come from the account's handful of distinct policy conditions, not from anything per-principal — so the key domain is tiny and the hit rate is near-total: measured 8 entries / 203 k hits (120-role fixture) and 33 entries / 960 k hits (adversarial fixture). Removing it costs +53 s / +829 MB on real btg — but that cost is allocation churn (re-creating merged conditions millions of times), not the memo's own footprint: real retention is kilobytes (tens of entries), so the earlier "~hundreds of MB" figure was wrong (extrapolated from a synthetic 20 k-distinct-condition microbenchmark that doesn't occur). Physically a module-globalfunctools.lru_cache(maxsize=100_000)(survives the hotshould_denyloop cheaply);EffectivePolicyEvaluator.__init__callsreset_merge_condition_cache(), kept because it's free, makescache_stats()meaningful, and bounds a pathological account — not as a memory mitigation.merge_conditionis pure (deep_update/negate_conditionboth copy), so the clear is a hygiene bound, never a correctness requirement.should_denyresult — the instructive contrast to the memo above. An earlier revision memoized it (source-stripped, sentinel-backed); on real btg it bought 0 wall and cost +104 MB, so it was deleted (~180 lines + the source-strip soundness invariant gone with it). Same idea as the merge memo, opposite outcome, and the reason is the key domain: its key included theAction'ssource, which is per-principal-distinct → near-zero reuse (deniss measured 0 hits) and ~1.8 KB condition-bearing values retained per miss. The merge memo wins precisely because its key is the account's small fixed condition domain (960 k hits) instead. That is the non-obvious rule this PR encodes: memoize the small fixed-domain thing, not the large per-principal-distinct thing.3.
create_json_report: in-place_finalize+ skip the never-readdenied_permissionssection (opt-in)create_json_reportgained two related changes:(a)
_finalizereplaces thejson.loads(json.dumps(...))round-trip. The old serialize/parse existed only to coerce the report'ssetsources anddefaultdictfactories into plain JSON types. It is now a single in-place walk (set→ sortedlist,defaultdict→dict), avoiding a full re-encode/re-decode of a multi-million-action structure per principal. This is a hard invariant, not an optimization nicety: the S3 upload path serializes with a json handler thatstr()s an unexpectedset(writing a Python repr, not a JSON array) instead of raising, so anysetthat_finalizemisses would corrupt the blob silently. New testtest_create_json_report_emits_no_setswalks the entire returned report and asserts nosetsurvives anywhere, and thatjson.dumpsaccepts it with nodefault=— the invariant the round-trip used to give for free.(b)
include_denied_permissions: bool = Truelets a caller skip building the never-readdenied_permissionssection.denied_permissionssection re-serializes the account-constant SCP deny expansion — ~2.94M actions per principal on conditional-SCP accounts — for every principal. On a report-heavy account it is ~80% of the entire handler, and it is near-constant per principal regardless of that principal's actual permission count.allowed_permissions+ineffective_permissions:move_to_graph_utils/orca/effective_permissions/effective_permissions_parser.py:119,125base_api/gql/gql_query.py:1249,1256Truepreserves current behavior (CLI report + golden test unchanged). The AWS handler opts out withinclude_denied_permissions=False— that opt-out lands in the follow-up pin-bump PR (this library PR only makes it available; the runtime win from the skip is realized there).allowed_permissions+ineffective_permissionsare identical (canonicalized set);denied_permissionsbecomes{}. Customer-visible UI + graph are unaffected. Locked bytest_include_denied_permissions_preserves_read_sections.Correctness
mainbug (the one intentional output change).shrink_policy's condition-merge loop no longer aliases + mutates a sharedActioncondition in place (dict(operator_conditions)copy). Onmain, a principal with ≥2 conditions on the same(source, action, resource)gets one condition nondeterministically dropped (886/4,276 conditions mutated, 3 distinct report hashes over 3 identical runs); this PR mutates 0 and is stable. Guarded bytest_shrink_does_not_mutate_shared_scp(verified to fail on the pre-fix code) andtest_shared_evaluator_matches_standalone. This is the only case where the PR's output intentionally differs frommain, and it differs by producingmain's uncorrupted answer.differ=0) across three account profiles — btg (conditional SCPs), real954 (2,694-role expansion-heavy), tempus — plus edge fixtures (permission boundary, NotResource/NotAction, group inheritance, conditional deny).Action: "*"wildcard,NotAction,NotResource, conditional permission boundaries, and an SCP with deny-by-NotResourceand deny-by-NotAction.mainvs PR: 0 allowed / 0 ineffective drift undernone/allowall/deny, denied-skip 0 drift, 0 sets left in the report. Covers the group-inheritance andNotAction/NotResourcepaths the earlier fixtures never reached.allowed_permissions+ineffective_permissionsproven identical (canonicalized set) with vs without the skip across btg (20 principals) + Tempus (2,774 principals, all entity types) = 0 mismatches. Structurally isolated too — the section is built by an independent loop overdenied_permissions;evaluate()(where permissions are computed) is untouched._to_plain), notdeepcopy. Adeepcopy'dHashableDictcarries the original's memoized_hash;normalize_policy's in-place scalar→list rewrite would then leave that hash stale, so two equal conditions could land in different hash buckets and split a statement. Copying into plaindict/listcarries no memoized hash to go stale. The copy-on-write predicate guards each operator value withisinstance(operator_dict, dict)(mirroring the merge code above) so a malformed non-dict operator can't raise mid-loop and escape to the account-level handler, losing the whole account's model.main).denied_by(not introduced here):ineffective_permissionsmatches the cache-free run on count, source, action and condition; the only divergence isdenied_by, and a main-vs-main run shows the same principals differing on the same field —should_denycredits whichever full-deny it hits first while iterating aset, so when several SCP statements fully deny an action the attribution is arbitrary and flips between scans (it surfaces assource_policiesviato_gql_effective_actions). Orthogonal to this PR; filed as a follow-up ticket.shrink_policyoutput document is order-nondeterministic on both branches (confirmed this round: 3 distinct hashes over 3 runs each; stable when no SCPs) —deep_updatededupes merged condition-value lists vialist(set(...)), whose order is not stable. Harmless to the effective set, but it meansAwsEffectivePermissionsPolicychurns between scans with no cloud change. Same follow-up ticket as thedeep_updateordering item.Measured (real btg account: 273 roles, 236 SCPs; 45 principals; full pipeline evaluate → shrink_policy → create_json_report; PYTHONHASHSEED=0)
Production sitting at 12.9 GB on 45 of 275 principals against a 16 Gi med-clouder limit is why the flag couldn't stay on. Per-piece ablation (remove one thing): short-circuit + hash memos 3.7× / +6.1 GB (the primary lever), condition-merge cache +53 s / +829 MB, expansion cache +899 MB, all-caching-off (skip only) +1,604 s / +8.7 GB. The denied-skip alone removes 133–279 MB of never-read JSON per principal (~76 GB per btg scan). Tempus (user-heavy, 2,774 principals, 0 SCPs): report compute 601 s → 45 s.
A separate real-handler A/B (the actual clouder
ClouderHandler.run()on real btg sonar, S3 upload stubbed) confirmed the end-to-end shape the library harness understates: main 1.1.8 full-report 5,200.7 s / 16,245 MB → branch + denied-skip 609.3 s / 9,436 MB = 8.5× runtime, −42% memory (both 275 principals). The extra headroom over the library-only number is the handler accumulating every principal's shrunk policy incached_result(~2 GB the library harness discards) plus the full denied report the skip removes.Honest range on the caching contribution alone (i.e. excluding denied-skip): −9 % to −49 %, workload-dependent. The spread is entirely what the caches can serve: expansion is only 13–18 % of runtime and only condition-free ARN-sourced expansions are cacheable. On a fixture of shared condition-free managed policies the cache win is −34 % to −49 % (btg-like); on a fixture dominated by inline
NotActionand conditioned policies (never cacheable) it's only −9 % to −18 %. Adding denied-skip takes those to −61 % and −14 % respectively. This is why the denied-skip — not the caches — is the dependable lever on report-heavy FF-victim accounts; the caches help most exactly where a lot of expansion is condition-free and ARN-sourced.Review feedback addressed
Round 1
shrink_policycopies a statement'sConditionbeforenormalize_policyonly when it holds a scalar (copy-on-write; the sole case normalize mutates), so a condition shared across principals via the caches is never mutated in place. Previously it rewrote a scalar SCP condition into a list, corrupting the sharedscp_policyfor every principal evaluated afterwards through the reused evaluator. Production SCPs are normalized, so this copies nothing there; keeps the cache hot-path (btg eval ~0.7 s/role vs ~7 s/role uncached). Guard:test_shrink_does_not_mutate_shared_scp(verified to fail on the pre-fix code).expand_action/expand_not_actionreturn a freshdict;get_action_access_levelsreturns a list copy.1.1.8 → 1.1.9.Rounds 2–5 (deny-result cache: added, then removed)
An SCP
should_denyresult cache was added (source-stripped key + guard + sentinel + re-stamp,CappedMemoCache-bounded) and independently verified hash-identical on btg. But per-piece ablation on real btg showed it buys 0 wall and costs +104 MB — its(action, resource, condition)keys are mostly distinct per principal (near-zero reuse) while its values are ~1.8 KB condition-bearing Actions. So it was deleted (round 5): −104 MB, ~180 fewer lines, and the source-strip soundness invariant +_scp_caches_for+DENY_CACHE_MAX_WEIGHTgo with it;explicitly_deny/apply_permission_boundaryrevert to master's plain form. TheCappedMemoCacheandcache_stats()remain for the expansion cache.Final verified review (8 findings)
lru_cache; made it per-account by clearing it inEffectivePolicyEvaluator.__init__(reset_merge_condition_cache()) and visible by reporting it incache_stats()undermerge. Corrected the earlier "per-account cache" wording — it is module-global but cleared per evaluator (pure, so correctness is unaffected either way). Also corrected the retention estimate: measured real retention is kilobytes (8–33 entries), so the clear is hygiene + observability + a pathological-account bound, not a memory mitigation — the earlier ~hundreds-of-MB figure came from a synthetic microbenchmark whose key space doesn't occur._to_plain), notcopy.deepcopy, so anormalize_policymutation can't leave aHashableDict._hashstale (which would split equal conditions across hash buckets).import copyremoved.test_create_json_report_emits_no_setslocking the_finalizeinvariant (the uploaderstr()s a stray set instead of raising).isinstance(operator_dict, dict)(mirrors the merge loop) so a non-dict operator value can't raise and lose the whole account.CappedMemoCache.update()/setdefault()now route through__setitem__so neither can bypass the weight cap.test_shared_evaluator_matches_standaloneto compare the fullcreate_json_reportper principal (not just the shrunk policy), so the aliasing guard now covers Contract A as well as Contract B.deep_updatelist ordering;negate_conditiononly negating the first operator;denied_byattribution reaching the UI assource_policies) are orthogonal and filed as a separate ticket (F8).Memory & rollout
CappedMemoCachestop-inserts past ~300 MB; merge memo cleared per account;cache_stats()logs both live next to RSS).log_model_data_lossand silently loses the account'sAwsEffectivePermissionsPolicy), the FF re-enable should be a canary on 2–3 accounts withcache_stats()+@log_memory_usagetelemetry, after confirming which tier btg lands on.main's condition-narrowing bug, a canary that diffs new blobs against current production blobs will seeallowed_permissionsdifferences on accounts with a multi-condition-same-source principal — those are the correction, not a regression. Validate against the canonicalized no-shrinkevaluate()set (which both branches agree on), not the current prod artifact. The blast radius is workload-specific: the bug only fires when a principal has ≥2 Actions sharing(source, action, resource, not_resource)with different condition operators (886 conditions affected on one fixture, 0 on another). So a clean diff on the first canary account does not mean the fix isn't working — it means that account has no colliding principal. A prod blob showing a narrower region set than the account actually grants is the live bug, not the baseline.load_scp_from_json/load_scp_from_aws) nownormalize_policyon load — the clouder path already does — so the condition-copy's "inputs are normalized" invariant holds everywhere.Deferred to a follow-up (not blockers)
(pb_arn).get_permission_boundary(evaluator.py:699-712) re-expands the boundary policy for every principal, and boundaries almost always carry aCondition, so they can never hit the condition-free expansion cache. The boundary is account-fixed per ARN → cacheable on(pb_arn)with the same safety argument the expansion cache already makes. (One of the two biggest remaining uncacheable items — see the −9 % fixture.)NotActionexpansion within a principal's inherited policies. InlineNotActionexpansion is the single largest uncacheable item; thesid.startswith("arn:")gate excludes it because inline sids are policy names, not ARNs. Keying on(entity_arn, policy_name, statement_idx)would make it cacheable across the repeats that come from group-inherited policies.HashableDict/HashableList(immutability by construction). With the deny cache gone the shared-condition invariant surface collapses to essentially the oneshrink_policycopy-on-write guard, so this is low-priority hardening now; if taken up, a raisingTypeErrormust be caught at the handler's per-entity boundary (else it escapes to the account-levelexcept Exceptionand loses the whole account's model).deep_updatelist ordering (surfaces as the shrunk-policy document churning between scans with no cloud change),negate_conditionfirst-operator-only, anddenied_byattribution (which reaches the UI assource_policies).Rollout
The iam_ape change is safe-by-default (denied section still built unless a caller opts out). The orca handler opts out (
include_denied_permissions=False) in the follow-up pin-bump PR (which also bumps theuv.lockpin to 1.1.9); a canary re-enables the FF on a few accounts to confirm the production total before fleet rollout.