feat: upgrade to Okta SDK v3 and pool the client on the server loop#509
Draft
somethingnew2-0 wants to merge 2 commits into
Draft
feat: upgrade to Okta SDK v3 and pool the client on the server loop#509somethingnew2-0 wants to merge 2 commits into
somethingnew2-0 wants to merge 2 commits into
Conversation
acad342 to
78505f4
Compare
29db6f8 to
2668815
Compare
b1c3d04 to
89cfcee
Compare
Implements POST_MIGRATION_TODO items 11a (Okta SDK v3 upgrade) and 11b (shared pooled Okta client), both contained behind the OktaService facade. 11a bumps okta 2.9.8 -> 3.4.4 (the openapi-generator rewrite). The generated methods keep the (data, resp, error) tuple contract and return errors rather than raising, so the tuple-based retry loop survives (just response.get_status() -> response.status_code). Group ownership is now a native SDK feature, replacing the hand-rolled request-executor plumbing. Also handles method renames, cursor-based pagination, the GroupProfile anyOf union, and already-parsed datetime fields. The SDK's built-in rate-limit retry is disabled so _retry stays the single backoff layer. 11b holds one client (and its pooled aiohttp connector) for the server loop via a lifespan hook, reused when the running loop matches; CLI and per-test loops keep a fresh client per call. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2668815 to
caa4f6f
Compare
…ustom _retry The v3 SDK retries rate-limited (429) requests internally, honoring the X-Rate-Limit-Reset header, and its requestTimeout config bounds each request (an aiohttp per-request socket timeout plus a cumulative deadline across those retries). So the facade's hand-rolled _retry loop and asyncio.wait_for ceiling are both redundant. Remove them and rely on the SDK: its built-in 429 retries (default maxRetries=2) and requestTimeout. A thin _call wrapper inspects the returned error and raises OktaTimeout when the SDK gave up — a request timeout or a 429 that outlived its retries (OktaAPIError status 429) — so the membership/ownership fan-out can still catch and swallow it and let the syncer reconcile. 5xx errors are intentionally no longer retried; other errors pass through unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
e9cb385 to
7e001a0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements POST_MIGRATION_TODO 11a (Okta SDK v3 upgrade) and 11b (shared pooled Okta client). Both are contained behind the
OktaServicefacade, so no call site outside the service changed — the syncer, operations, integrity checks, and CLI all keep going through the same facade methods.11a —
okta2.9.8 → 3.4.4v3 is the openapi-generator rewrite the TODO anticipated (
okta.api.*, Pydantic models). Two realities cut the blast radius down from what that note assumed, and are the key things to know when reviewing:(data, resp, error)/(resp, error)and return API errors rather than raising them. So the facade's tuple-based retry loop stayed intact — the main change isresponse.get_status()→response.status_codeand reading the rate-limit reset header case-insensitively.assign_group_owner/delete_group_owner/list_group_owners, so the hand-rolled request-executor plumbing the TODO called out is gone.Other v3 deltas absorbed inside the facade: method renames (
add_group/replace_group/assign_user_to_group/unassign_user_from_group); cursor-based pagination viaPaginationHelper(the oldresp.next()walker is gone, and the SDK's bundledpaginate_allhelper is unusable in this build — it calls*_with_http_infovariants that don't exist — so the facade drives theaftercursor itself);Group.profileas an anyOf union reached throughactual_instance; and already-parseddatetimefields (no moredateutilparsing).get_user_schemarecovers the schema_linkshref from the raw response body because the v3UserTypemodel drops_links.Retry posture: rate-limiting and timeouts are both the SDK's job. It retries 429s (honoring
X-Rate-Limit-Reset) up torateLimit.maxRetries(default 2), andrequestTimeoutbounds each request — an aiohttp per-request socket timeout plus a cumulative deadline across those retries. The facade keeps no retry loop orwait_forof its own; a thin_callwrapper just maps the SDK's "gave up" outcomes — a request timeout, or a 429 that outlived its retries — toOktaTimeout, which the membership/ownership fan-out catches and swallows (the syncer reconciles later). 5xx errors are intentionally not retried — left to Okta.11b — pooled client
OktaServicenow holds one client (and its pooled aiohttp connector) for the server loop, created in the FastAPI lifespan and reused by_okta_client()whenasyncio.get_running_loop()matches the loop it was built on. CLI invocations and per-test loops still build a fresh client per call — no pooled client is started when Okta is unconfigured, and the running-loop check prevents cross-loop session reuse. The TODO's "measure before bothering" caveat still stands; this just puts the pooling in place to measure.Verification note
The test suite mocks Okta at the facade boundary, so the v3 wire behavior it can't exercise — 429/rate-limit handling, multi-page pagination,
get_user_schema's_linksrecovery, and the group-owner endpoints (off by default behindOKTA_USE_GROUP_OWNERS_API) — should be validated against a live/staging org before this rolls out, as the TODO flagged.