iam: add JMH benchmark suite - #589
Conversation
Adds an abstract AbstractIamBenchmarkTest in iam-client plus thin AWS/GCP concretes, gated by @EnabledIfSystemProperty(runBenchmarks=true). Swept: getIdentity, getAttachedPolicies, getInlinePolicyDetails, createAttachDeleteIdentity. The lifecycle benchmark uses a bounded retry to absorb create->attach eventual-consistency lag on providers that create identities asynchronously.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #589 +/- ##
============================================
- Coverage 83.61% 83.60% -0.01%
Complexity 674 674
============================================
Files 215 215
Lines 15010 15010
Branches 2076 2076
============================================
- Hits 12550 12549 -1
- Misses 1636 1637 +1
Partials 824 824
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| .policyDocument(buildPolicyDocument(harness.getPolicyName())) | ||
| .tenantId(harness.getTenantId()) | ||
| .region(harness.getRegion()) | ||
| .identityName(harness.getIdentityName()) |
There was a problem hiding this comment.
🚨 On GCP a single getIdentityName() feeds two incompatible string forms, so two of the four benchmarks measure a not-found lookup
The harness exposes one getIdentityName() (:62) that gets consumed two different ways.
benchmarkGetIdentity (:189) needs a bare service-account id or email: GcpIam.getServiceAccountResourceName (iam-gcp/src/main/java/com/salesforce/multicloudj/iam/gcp/GcpIam.java:560-572) branches only on contains("@"), so a serviceAccount:-prefixed value is spliced through verbatim into projects/{p}/serviceAccounts/serviceAccount:sa@….
The seed here (:134), teardown (:150), benchmarkGetAttachedPolicies (:204) and benchmarkGetInlinePolicyDetails (:221) need a GCP member string — which this PR's own comment states at GcpIamBenchmarkTest.java:68-70, and which GcpIamIT encodes by keeping two accessors with different values: getTestIdentityName() → "testSa" (:90) versus getIdentityName() → "serviceAccount:chameleon@…" (:194-196).
The run contract in the PR body documents IAM_BENCHMARK_GCP_IDENTITY_NAME=<sa-email>, so doAttachInlinePolicy (GcpIam.java:183) writes an untyped member, SetIamPolicy rejects it, and the comment-only catch at :136-138 swallows it. From then on doGetAttachedPolicies (GcpIam.java:395) returns an empty list and doGetInlinePolicyDetails (GcpIam.java:322-325) returns null — both without raising. That reconciles the PR's "8 entries each, zero errors" with two of the four GCP benchmarks never finding a policy at all. AWS is unaffected: getRoleName() and getIdentityName() are the same value there and both role-scoped calls resolve against the role the seed attached to.
toPolicyMember (:91) is the right hook — it's just called from exactly one place, :255 in the lifecycle benchmark. Routing the seed, teardown and both policy reads through it, or splitting the accessor the way GcpIamIT does, closes this. Worth logging the seed failure rather than swallowing it too: on AWS a missing seed throws NoSuchEntity and fails loudly, on GCP it's silent, so the asymmetry hides it on exactly the provider where it bites.
Anticipating the fair objection — both reads do issue their GetIamPolicy RPC either way, so the latency isn't meaningless. But getInlinePolicyDetails returns at GcpIam.java:324 before the PolicyDocument build and serialise at :346 that this benchmark's javadoc (:214) counts as "fetch + parse", and more to the point the run can no longer distinguish "worked" from "found nothing" — which is what the AWS-vs-GCP comparison in the PR body rests on.
One related coupling worth checking while you're here: the seeded binding's role comes from the action (storage:GetObject → roles/storage.objectViewer, GcpIamPolicyTranslator.java:40) while the read filters on IAM_BENCHMARK_GCP_ROLE_NAME (GcpIam.java:318), so those two also have to be set consistently for the reads to resolve.
There was a problem hiding this comment.
Confirmed and fixed — you're exactly right, and it's worse than a latency-only issue: on GCP the seed itself was rejected. I reproduced it live: seeding/reading with the bare email gives getAttachedPolicies size 0 and getInlinePolicyDetails null, while routing through the serviceAccount: member gives size 32 and a populated document. So the PR's "8 entries, zero errors" was the swallowed catch hiding a rejected SetIamPolicy, and two of the four GCP benchmarks were measuring a not-found lookup — precisely as you described.
Fix: added a getPolicyMemberName() helper on the harness that routes the read identity through the existing toPolicyMember hook, and switched the seed, teardown, benchmarkGetAttachedPolicies and benchmarkGetInlinePolicyDetails over to it. benchmarkGetIdentity stays on the bare name since that path needs the un-prefixed value. Also replaced the silent seed catch with a logged warning, for exactly the AWS-loud / GCP-silent asymmetry you called out. Verified on both clouds on JDK 25.
Good catch on the role/actions coupling too — the seeded role comes from the translated action and the reads filter on IAM_BENCHMARK_GCP_ROLE_NAME, so those have to line up. I've kept them consistent in the run contract.
| throw e; | ||
| } | ||
| try { | ||
| Thread.sleep(ATTACH_RETRY_BASE_MILLIS * attempt); |
There was a problem hiding this comment.
Two things about benchmarkCreateAttachDeleteIdentity, both inside the measured window.
attachWithPropagationRetry is invoked from the benchmark body at :259, and its backoff sleeps here — ATTACH_RETRY_BASE_MILLIS 500 ms × attempt, up to 5 attempts, so one invocation can absorb 5 s of pure Thread.sleep. On GCP the retry is the expected path rather than a rare one, since service-account creation is eventually consistent, so the SampleTime distribution that the class javadoc calls the signal becomes bimodal with its slow mode set by the hardcoded schedule rather than by anything the SDK does. Moving the wait out of the timed region — or doing the create in @Setup and benchmarking attach/delete against an already-propagated identity — would make the number mean something. No other benchmark in the repo sleeps inside a timed method.
Separately, each invocation mints a real cloud identity (:236) and the cleanup at :266-269 discards any failure. With @Warmup(3, 2s) + @Measurement(5, 3s) across two @BenchmarkModes that's on the order of tens of service accounts per run; nextLifecycleId restarts at 0 in every fork so consecutive runs collide on names; and GCP soft-deleted accounts keep counting against the 100-per-project quota for 30 days. There's also a silent coupling on the GCP side — removePolicy matches on binding.getRole().equals(request.getPolicyName()) (GcpIam.java:419-428) while attach derives the role from the actions (storage:GetObject → roles/storage.objectViewer, GcpIamPolicyTranslator.java:40), so unless IAM_BENCHMARK_GCP_ROLE_NAME happens to be exactly that translated value the remove is a no-op and every invocation permanently adds a member to the project allow policy while deleting the account behind it.
The docstore suite's pattern would at least make the leak recoverable: track created identities in a ConcurrentHashMap.newKeySet() and drain it in @TearDown, the way AbstractDocstoreBenchmarkTest does at :69 and :158-182.
There was a problem hiding this comment.
Both fixed. I took the second option you suggested — create the identity in @Setup and benchmark against an already-propagated one — so the create RPC and the propagation wait (and therefore the Thread.sleep backoff) are entirely out of the timed region now. Renamed the method to benchmarkAttachRemovePolicy since that's what it actually measures.
One honest note from validating this live: in my 3 lifecycle runs the attach succeeded first try every time, so the Thread.sleep never actually fired — the timed window was dominated by the create + delete RPCs (5–10s), not the backoff. So the sleep was a latent hazard rather than the thing distorting my particular numbers, but moving it out is still correct, and pulling create/delete out of the measurement is the bigger accuracy win. I'd rather the number mean "attach + remove" cleanly than bundle four ops.
On the leak: adopted the docstore pattern — created identities go into a ConcurrentHashMap.newKeySet() and get drained in @TearDown. Also switched the name suffix from the fork-local counter (which restarted at 0 and collided across runs) to nanoTime, and shortened the prefix to iam-bench-lc- so GCP's 30-char account-id cap isn't blown. Verified live: 0 leaked service accounts after the run on GCP, 0 on AWS.
…tach/remove Two fixes to the IAM JMH suite from review: - On GCP a single getIdentityName() was consumed as both a bare service-account email (getIdentity) and a policy member string (seed/attach/reads). The bare form is rejected by SetIamPolicy, the failure was swallowed, and getAttachedPolicies/getInlinePolicyDetails then silently returned empty. Route the seed, teardown and both policy reads through toPolicyMember via a new getPolicyMemberName() helper; keep getIdentity on the bare name; log the seed failure instead of swallowing it. - The lifecycle benchmark created the identity and ran its eventual-consistency backoff sleep inside the timed region, and could leak real cloud identities. Create the identity once in @setup (with the propagation wait), time only attach/remove, delete in @teardown, and track created identities in a drained key set. Rename to benchmarkAttachRemovePolicy; suffix names with nanoTime to avoid cross-fork collisions and shorten the prefix under GCP's 30-char cap.
| harness.getRegion(), | ||
| Optional.empty(), | ||
| Optional.empty()); | ||
| createdIdentities.add(identityName); |
There was a problem hiding this comment.
🚨 The new @TearDown drain is unreachable on every error path, so a failed trial still leaks the service account it created
The tracking set and the drain are right, but JMH emits @TearDown(Level.Trial) after the measurement loop, guarded by if (control.isLastIteration()) and if (control.isFailing) throw new FailureAssistException() — it is not in a finally. I ran a class carrying this file's exact annotations under JMH 1.37: when the @Benchmark threw mid-iteration, @Setup ran twice (once per @BenchmarkMode) and @TearDown ran zero times, exit code 0. Same result when @Setup itself threw after registering the identity.
Both paths are live here. setupBenchmark creates a real principal (:173) and registers it (:180), then attachWithPropagationRetry (:186) or removePolicy (:187) can throw; :193 rethrows and the state object is discarded before it is ever published, so nothing can reach createdIdentities afterwards. And benchmarkAttachRemovePolicy (:310) has no try/catch, so a single throttled SetIamPolicy during a 21 s × 2-mode sweep aborts the trial — and :44 says that backend "throttles hard". With 8 trials per run each minting their own identity, a systemic failure leaks 8. The previous version deleted in a finally inside the benchmark, which survived failures; the drain is now the only cleanup path and it is the one JMH skips.
Suggested fix: arm the drain from a JVM shutdown hook — I confirmed it does run in the aborted-benchmark case — and wrap everything after createIdentity in @Setup in a try/catch that deletes before rethrowing.
There was a problem hiding this comment.
Confirmed, and you're right my refactor is what opened this — the old version deleted in a finally inside the benchmark, which survived aborts; moving cleanup to @TearDown traded that for the one path JMH skips. Read the generated ..._benchmarkAttachRemovePolicy_jmhTest.java to confirm the mechanism: teardownBenchmark() is only called inside if (control.isLastIteration()), behind the isFailing/FailureAssistException guard, never in a finally; and the state object / readyTrial publishes only after setupBenchmark() returns, so a throw in @Setup after the identity is registered strands it too. Both paths exactly as you laid out.
Fix (pushed): one idempotent cleanup() (guarded by an AtomicBoolean) that removes the seed policy, drains createdIdentities, and closes client/harness, swallowing per-step failures so one bad delete can't strand the rest. Called from both @TearDown (happy path) and a JVM shutdown hook armed at the top of @Setup, before any RPC can throw; whichever wins the race does the work, the other no-ops. @TearDown de-registers the hook on the clean path.
I skipped the separate @Setup try/catch — the identity goes into createdIdentities before anything that can throw, so the shutdown hook already covers the failed-setup case (one cleanup path instead of two), and the shared method also removes the seed policy on the abort path, which leaked the same way.
Validated live, JDK 21 / JMH 1.37, both clouds:
- GCP happy path — BUILD SUCCESS, 8/8 populated (getIdentity 2.87, getAttachedPolicies 1.57, getInlinePolicyDetails 1.71, attachRemovePolicy 0.24 ops/s), 0 leaked service accounts.
- AWS happy path — BUILD SUCCESS, 8/8 populated (getIdentity 3.13, getAttachedPolicies 3.27, getInlinePolicyDetails 3.15, attachRemovePolicy 1.43 ops/s), 0 leaked roles.
- GCP forced-abort (the actual regression) — injected a fault so
benchmarkAttachRemovePolicythrows; JMH marked the trial a<failure>and skipped@TearDown, so the shutdown hook was the only cleanup path: 0 leaked service accounts (gcloud iam service-accounts list). Fault was-D-gated and reverted before commit.
Thanks for running it under real JMH rather than eyeballing it — that's what made the diagnosis unambiguous.
| /** | ||
| * The member string for the pre-seeded read identity ({@link #getIdentityName()}) as | ||
| * attach/remove/get-policy expect it. GCP needs "serviceAccount:email" here, not the bare | ||
| * email that {@code getIdentity} consumes; AWS is role-scoped and unaffected. |
There was a problem hiding this comment.
iam-client module, which CLAUDE.md forbids
CLAUDE.md → Dependency management: "Never put any provider specific code, documentation, keywords in client, driver packages." And Provider Isolation (CRITICAL RULE) lists among forbidden patterns "comparisons or references to other providers in code OR comments" and "Comparing implementation approaches in javadoc or inline comments." This file is iam/iam-client, the provider-neutral module.
Two comparisons were added in this commit: here at :112, "GCP needs serviceAccount:email here, not the bare email that getIdentity consumes; AWS is role-scoped and unaffected", and :151-152, "on AWS a missing seed throws loudly, on GCP it is silent, so an unlogged swallow hides the failure on the provider where it bites." :305-307 similarly explains the shared policy name in terms of GCP's removePolicy.
The mechanism is already correct — toPolicyMember/getPolicyMemberName is the harness hook, and GcpIamBenchmarkTest.toPolicyMember (:67-71) already carries the same explanation in the provider module where it belongs. Only the client-module prose needs to change.
Suggested fix: state the contract neutrally here ("the member form the provider's attach/remove calls expect; providers needing a typed member override toPolicyMember") and keep the GCP/AWS specifics in each provider's harness.
There was a problem hiding this comment.
You're right, clean violation — the neutral module shouldn't carry provider prose. Reworded the three you flagged to state the contract neutrally: toPolicyMember/getPolicyMemberName is the hook, providers needing a typed member override it, and attach/remove must name the same role because some providers match remove on the translated role rather than the document name. The specifics stay in GcpIamBenchmarkTest.toPolicyMember where they belong.
While in there I also scrubbed the remaining single-provider mentions in the file (the quota/char-cap notes and a couple of provider API method names in javadoc) for the same reason — kept the why, stated as provider-neutral SDK behavior. Pushed.
…rose Two fixes to the IAM JMH suite from review: - @teardown runs only on the clean path — JMH skips it when a trial aborts (RPC failure) or @setup throws after the identity is minted, leaking real cloud principals. Extract an idempotent cleanup() (guarded by an AtomicBoolean) that removes the seed policy, drains createdIdentities and closes client/harness, swallowing per-step failures. Call it from both @teardown and a JVM shutdown hook armed at the top of @setup before any RPC can throw; whichever wins the race does the work, the other no-ops. Validated live on AWS + GCP: happy-path and forced-abort both leak zero identities. - The neutral iam-client harness carried provider-specific prose (service-account/quota/char-cap notes, provider API names, cross- provider comparisons), violating provider isolation. Reword to state the contract in provider-neutral terms; the specifics stay in the provider concretes. Trim multi-line comments to one line where the why still reads.
16d4e31 to
61090f3
Compare
Summary
Adds a JMH benchmark suite for IAM:
AbstractIamBenchmarkTest(iam-client) plus thin AWS/GCP concretes. Gated by@EnabledIfSystemProperty(named="runBenchmarks", matches="true")— inert in CI.Swept (4):
getIdentity,getAttachedPolicies,getInlinePolicyDetails,createAttachDeleteIdentity.The lifecycle benchmark uses a bounded retry (
attachWithPropagationRetry) to absorb create->attach eventual-consistency lag on providers that create identities asynchronously; read-after-write providers pass on the first attempt.Testing proof
Run locally against live AWS + GCP on 2026-08-11, both JMH modes. All 4 methods produced populated results on both clouds (8 entries each, zero errors). AWS IAM is global — the run must sign against
us-east-1(documented in the run contract below).Invocation (creds via OS env only):
Run contract
IAM_BENCHMARK_AWS_{REGION,IDENTITY_NAME,TENANT_ID,POLICY_NAME,ENDPOINT,POLICY_RESOURCE}IAM_BENCHMARK_GCP_{TENANT_ID,REGION,IDENTITY_NAME,ROLE_NAME}JMH config note
Class-level
@Warmup/@Measurement/@Forkare the local-run baseline; the downstream benchmark runner overrides these at execution time.Downstream
Running this in the downstream benchmark pipeline needs a separate vendor-sync of these files (new build target + wiring); it does not propagate automatically.
Merge order
Independent. Recommend the root-pom JMH fix merges first.