[CXP-913] fix: retry Okta rate limits instead of failing provisioning tasks - #197
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Connector PR Review: [CXP-913] fix: retry Okta rate limits instead of failing provisioning tasksBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe new commit addresses the prior finding directly: all four Open suggestion threads from earlier rounds are unchanged and not re-reported here ( Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
|
|
||
| // isRateLimitError reports a 429 from the response status, or from the "too many | ||
| // requests" sentinel once the v2 SDK exhausts its own retries and drops the response. | ||
| func isRateLimitError(resp *okta.Response, err error) bool { |
There was a problem hiding this comment.
This 429 → Unavailable classification is only routed through group.go and app.go. The role Grant/Revoke paths (role.go) and resource_sets_bindings.go still return the raw error with %s/%v, so a rate limit on those paths stays codes.Unknown and the SDK retryer skips it — the exact failure this PR fixes, just on the provisioning paths it doesn't touch. Since CXP-913 is about retrying rate limits across provisioning, can we route those sites through handleOktaResponseError too? As-is, role and resource-set-binding grants keep hard-failing on 429.
| if resp != nil && resp.StatusCode == http.StatusTooManyRequests { | ||
| return true | ||
| } | ||
| return err != nil && strings.Contains(err.Error(), "too many requests") |
There was a problem hiding this comment.
nit: the v2 SDK emits this sentinel as errors.New("too many requests") with a nil response, so an exact match is safer than strings.Contains here — otherwise any unrelated error whose text happens to include the phrase (e.g. a validation error echoing user input) gets classified as a rate limit and burns retries on a permanent failure. Something like resp == nil && err != nil && err.Error() == "too many requests" pins it to the real case (the 429-with-response path is already handled above).
| okta.WithCache(cc.Cache), | ||
| okta.WithCacheTti(cacheTTI), | ||
| okta.WithCacheTtl(cacheTTL), | ||
| okta.WithRateLimitMaxBackOff(oktaRateLimitMaxBackoffSeconds), |
There was a problem hiding this comment.
question: this doubles the SDK default (30s → 60s) for every request, sync included, not just provisioning. With MaxRetries(2) and no RequestTimeout, a rate-limited call can block ~120s in-process before the connector sees the error and can checkpoint. Is the longer ceiling intended for the sync path too, or would it be worth pairing it with an explicit okta.WithRequestTimeout(...)?
There was a problem hiding this comment.
heads up — this line isn't in the diff anymore, git diff origin/main...HEAD -- pkg/connector/connector.go comes back completely empty on the current tip. looks like the backoff bump got pulled out in a later push (maybe in response to this exact thread?), but the PR description still says "Backoff ceiling 30s → 60s" which is now stale/inaccurate. worth updating the description so it matches what's actually shipping.
5ac3c2a to
4d9db1b
Compare
There was a problem hiding this comment.
Blocking issues found — see review comments. No new blocking issues in this pass, but the five correctness findings from the previous review are still unaddressed at 4d9db1b (restated in the summary comment).
| } | ||
|
|
||
| l.Warn("App Membership has been created.", | ||
| l.Debug("App Membership has been created.", |
There was a problem hiding this comment.
*assignedUser.LastUpdated is deref'd with no nil check (same at :509).
https://github.com/okta/okta-sdk-golang/blob/v2.20.0/okta/appUser.go#L39
4d9db1b to
6726b21
Compare
FeliLucero1
left a comment
FeliLucero1
left a comment
There was a problem hiding this comment.
One more small thing, not inline-commentable since it's outside the diff hunk: group.go:511 (groupResourceType.Grant, the AddUserToGroup error path) returns nil, handleOktaResponseError(response, err) with no fmt.Errorf wrap, so it's missing the okta-connector: prefix everything else uses — unlike the Revoke path right below it in the same file, which now gets the prefix via revokeNotFoundOrError. Low priority, there are ~136 other spots in pkg/connector with the same gap, but flagging since you're already touching this exact function for the rate-limit annotations.
|
Quick note — I replied to 3 existing review threads earlier today, but 2 of them were already resolved and all 3 are marked outdated by GitHub, so they're collapsed by default and easy to miss. Linking them here so they don't get lost:
Worth expanding "Show resolved"/"Show outdated" on the Files changed tab to see them inline. |
Okta 429s reached baton-sdk as codes.Unknown, so the retryer rejected them and provisioning tasks hard-failed instead of waiting. Classify them as codes.Unavailable and make Revoke idempotent on 404.
…ith a synthetic rate-limit description, so the SDK retryer waits out Okta's reset window instead of hard-failing the task. - Route every Grant/Revoke error path through handleOktaResponseError; only group and app were classified before, so a 429 on role, resource-set or resource-set-binding provisioning still surfaced as codes.Unknown. - Return GrantAlreadyRevoked/GrantAlreadyExists when the target is already in the desired state instead of failing the operation. - Report rate-limit annotations from successful provisioning responses, matching what the read paths already emit. - Drop the unguarded LastUpdated/CreatedAt derefs from success logs and lower those logs to Debug. - Add error-classification tests that drive the real vendored Okta SDK.
- Classify the two 429s that leave the SDK's retry loop through Get429BackoffTime, not just the exhausted-retries sentinel. Okta's concurrency limit omits X-Rate-Limit-Reset and hit that path, so those 429s still reached baton-sdk as codes.Unknown. - Stop shadowing the SDK's error when re-parsing the response body, so 5xx failures keep the x-okta-request-id the SDK appends for support triage. - Return GrantAlreadyRevoked when the principal is not a member of a resource-set binding, matching what the role paths already do. - Add tests for both classification paths and for the rate-limit annotations on Grant/Revoke success, which nothing asserted before. - Inline the revoke-success log again; a helper wrapping one debug call was not worth the indirection.
afb7ac7 to
5d12421
Compare
| } | ||
|
|
||
| func newTestAppBuilder(client *okta.Client) *appResourceType { | ||
| return appBuilder("", "", false, nil, client) |
There was a problem hiding this comment.
🟠 Bug: appBuilder takes six parameters at this SHA — appBuilder(domain, apiToken string, syncInactiveApps, skipAppGroups bool, filterEmailDomains []string, client *okta.Client) (pkg/connector/app.go:73). This call passes five, with nil landing on the skipAppGroups bool parameter, so the whole connector test package fails to compile and none of the new tests (or the pre-existing ones) run.
| return appBuilder("", "", false, nil, client) | |
| return appBuilder("", "", false, false, nil, client) |
| return rateLimitAnnotations(response), nil | ||
| } | ||
|
|
||
| return nil, nil |
There was a problem hiding this comment.
🟡 Suggestion: when the principal isn't among the binding's members, this returns bare nil, nil. Every other revoke path in this PR now signals idempotency, and this is the most common "already revoked" shape for bindings (the list succeeds, the member just isn't there). Returning annotations.New(&v2.GrantAlreadyRevoked{}), nil here would make it consistent with revokeNotFoundOrError.
| email := unknownProfileValue | ||
| if user.Profile != nil { | ||
| if v, ok := (*user.Profile)[profileFieldEmail].(string); ok { | ||
| email = v | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the new user.Profile != nil guard is an improvement, but user itself can still be nil here. UserResource.GetUser returns (nil, resp, nil) when a 2xx carries an empty body — the SDK only unmarshals when len(bodyBytes) > 0 — so user.Profile would panic. Same shape applies to assignedUser.Id (line 619), assignedGroup.Id (line 671), and createdRole.Id in role.go. Folding it into the existing check (if user != nil && user.Profile != nil) covers it.
| @@ -507,22 +509,21 @@ func (g *roleResourceType) Grant(ctx context.Context, principal *v2.Resource, en | |||
| ) | |||
| } | |||
|
|
|||
| return nil, fmt.Errorf("okta-connector: %v", errOkta) | |||
| return nil, fmt.Errorf("okta-connector: failed to assign role to group: %w", handleOktaResponseError(response, errors.Join(&errOkta, err))) | |||
There was a problem hiding this comment.
🟡 Suggestion: the user branch returns annotations.New(&v2.GrantAlreadyExists{}), nil on alreadyAssignedRole (line 469), but this group branch only logs and then falls through to the error return on line 512 — so an already-assigned group role reports failure. It's currently unreachable dead code (the guard at line 431 rejects every non-user principal before the switch), which is worth resolving one way or the other: either add the matching GrantAlreadyExists return, or drop the resourceTypeGroup.Id cases from Grant/Revoke since they can't be entered.
| if status.Code(classified) == codes.NotFound { | ||
| l.Debug(notFoundMsg, | ||
| zap.String("principal_id", principal.Id.String()), | ||
| zap.String("principal_type", principal.Id.ResourceType), | ||
| ) | ||
| return annotations.New(&v2.GrantAlreadyRevoked{}), nil | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the already-revoked path discards the rate-limit state even though resp is in hand and carries the X-Rate-Limit-* headers. Every other provisioning path in this PR now reports remaining quota via rateLimitAnnotations, so merging it here would keep reporting consistent: build from rateLimitAnnotations(resp) and append &v2.GrantAlreadyRevoked{}. (confidence: medium)
| return uhttp.WrapErrorsWithRateLimitInfo(codes.Unavailable, &http.Response{ | ||
| Status: "429 Too Many Requests", | ||
| StatusCode: http.StatusTooManyRequests, | ||
| Header: http.Header{}, | ||
| }, err) |
There was a problem hiding this comment.
🟡 Suggestion: with an empty Header, ratelimit.ExtractRateLimitData falls into its no-headers-on-429 default and sets ResetAt = now + 60s (ratelimit/http.go:134), so the retryer always waits a fixed minute here — it isn't recovering Okta's actual reset window, which the SDK already discarded. That's a defensible choice, but the comment on TestRateLimitExhaustedCarriesRetryDetail ("waits out Okta's reset window instead of its short fixed backoff") describes the opposite; worth correcting so the 60s default is an explicit, documented decision. (confidence: high)
- Fix the retry-detail test comment: the 60s reset is our default, not Okta's"
| @@ -590,14 +586,16 @@ func (g *appResourceType) Grant(ctx context.Context, principal *v2.Resource, ent | |||
| return annotations.New(&v2.GrantAlreadyExists{}), nil | |||
There was a problem hiding this comment.
🟡 Suggestion (high confidence): this already-exists path returns bare annotations even though response (line 553) is in hand and carries the X-Rate-Limit-* headers. This is the same gap that was just closed in revokeNotFoundOrError (helpers.go:242), and "rate-limit annotations on Grant/Revoke success paths" is one of the four changes this PR lists — an already-assigned grant is a success path too. Same at app.go:660, role.go:469, and resource_sets_bindings.go:327. Consider annos := rateLimitAnnotations(response); annos.Append(&v2.GrantAlreadyExists{}); return annos, nil.
I already fixed what this bot asked me to, but it's glitchy
6 participants
Okta 429s on provisioning endpoints reached baton-sdk as codes.Unknown. The
retryer only retries Unavailable / DeadlineExceeded, so tasks hard-failed
instead of waiting for the rate limit to reset — thousands got stuck needing
manual retry.
The v2 SDK exhausts its internal retries and returns a bare error, dropping the
response and its rate-limit headers. Our classifier had no 429 case.
Changes