Skip to content

[CXP-913] fix: retry Okta rate limits instead of failing provisioning tasks #197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mateovespConductor wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[CXP-913] fix: retry Okta rate limits instead of failing provisioning tasks #197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 54 additions & 54 deletions pkg/connector/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,10 +376,6 @@ func appResource(app *okta.Application) (*v2.Resource, error) {
}

func (g *appResourceType) Grant(ctx context.Context, principal *v2.Resource, entitlement *v2.Entitlement) (annotations.Annotations, error) {
var (
ok bool
email string
)
l := ctxzap.Extract(ctx)
if principal.Id.ResourceType != resourceTypeUser.Id && principal.Id.ResourceType != resourceTypeGroup.Id {
l.Warn(
Expand All @@ -399,7 +395,7 @@ func (g *appResourceType) Grant(ctx context.Context, principal *v2.Resource, ent
if response == nil {
l.Warn("okta-connector: failed to fetch application user, nil response",
zap.String("app_id", appID), zap.String("user_id", userID), zap.Error(err))
return nil, fmt.Errorf("okta-connector: failed to fetch application user: %s", err.Error())
return nil, fmt.Errorf("okta-connector: failed to fetch application user: %w", handleOktaResponseError(response, err))
}
defer response.Body.Close()
errOkta, err := getError(response)
Expand All @@ -421,7 +417,7 @@ func (g *appResourceType) Grant(ctx context.Context, principal *v2.Resource, ent
}

if appUser != nil && userID == appUser.Id {
l.Warn(
l.Debug(
"okta-connector: The app specified is already assigned to the user",
zap.String("principal_id", principal.Id.String()),
zap.String("principal_type", principal.Id.ResourceType),
Expand All @@ -430,13 +426,14 @@ func (g *appResourceType) Grant(ctx context.Context, principal *v2.Resource, ent
return annotations.New(&v2.GrantAlreadyExists{}), nil
}

user, _, err := g.client.User.GetUser(ctx, userID)
user, userResp, err := g.client.User.GetUser(ctx, userID)
if err != nil {
return nil, err
return nil, handleOktaResponseError(userResp, err)
}

profile := *user.Profile
if email, ok = profile[profileFieldEmail].(string); !ok {
email, ok := profile[profileFieldEmail].(string)
if !ok {
email = unknownProfileValue
}

Expand All @@ -447,31 +444,32 @@ func (g *appResourceType) Grant(ctx context.Context, principal *v2.Resource, ent
Id: userID,
Scope: strings.ToUpper(principal.Id.ResourceType),
}
assignedUser, _, err := g.client.Application.AssignUserToApplication(ctx, appID, payload)
assignedUser, assignResp, err := g.client.Application.AssignUserToApplication(ctx, appID, payload)
if err != nil {
l.Warn(
"okta-connector: The app specified cannot be assigned to the user",
zap.String("principal_id", principal.Id.String()),
zap.String("principal_type", principal.Id.ResourceType),
)
return nil, fmt.Errorf("okta-connector: The app specified cannot be assigned to the user %s",
err.Error())
return nil, fmt.Errorf("okta-connector: the app specified cannot be assigned to the user: %w", handleOktaResponseError(assignResp, err))
}

l.Warn("App Membership has been created.",
l.Debug("App Membership has been created.",
zap.String("userID", assignedUser.Id),
zap.String("Status", assignedUser.Status),
zap.Time("LastUpdated", *assignedUser.LastUpdated),

@github-actions github-actions Bot Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: *assignedUser.LastUpdated is dereferenced without a nil check, and zap evaluates field arguments eagerly regardless of level — so dropping this to Debug does not avoid the deref. If Okta ever omits lastUpdated from the assignment response, the grant panics after the assignment already succeeded. Same pattern at line 511 for assignedGroup.LastUpdated. Guard with if assignedUser.LastUpdated != nil or use zap.Any.

zap.String("Scope", assignedUser.Scope),
)

return rateLimitAnnotations(assignResp), nil
case resourceTypeGroup.Id:
groupID := principal.Id.Resource
appGroup, response, err := g.client.Application.GetApplicationGroupAssignment(ctx, appID, groupID, nil)
if err != nil {
if response == nil {
l.Warn("okta-connector: failed to fetch application group assignment, nil response",
zap.String("app_id", appID), zap.String("group_id", groupID), zap.Error(err))
return nil, fmt.Errorf("okta-connector: failed to fetch application group assignment: %s", err.Error())
return nil, fmt.Errorf("okta-connector: failed to fetch application group assignment: %w", handleOktaResponseError(response, err))
}
defer response.Body.Close()
errOkta, err := getError(response)
Expand All @@ -493,7 +491,7 @@ func (g *appResourceType) Grant(ctx context.Context, principal *v2.Resource, ent
}

if appGroup != nil && groupID == appGroup.Id {
l.Warn(
l.Debug(
"okta-connector: The app specified is already assigned to the group",
zap.String("principal_id", principal.Id.String()),
zap.String("principal_type", principal.Id.ResourceType),
Expand All @@ -503,20 +501,20 @@ func (g *appResourceType) Grant(ctx context.Context, principal *v2.Resource, ent
}

payload := okta.ApplicationGroupAssignment{}
assignedGroup, _, err := g.client.Application.CreateApplicationGroupAssignment(ctx, appID, groupID, payload)
assignedGroup, createResp, err := g.client.Application.CreateApplicationGroupAssignment(ctx, appID, groupID, payload)
if err != nil {
return nil, err
return nil, handleOktaResponseError(createResp, err)
}

l.Warn("App Membership has been created.",
l.Debug("App Membership has been created.",
zap.String("userID", assignedGroup.Id),
zap.Time("LastUpdated", *assignedGroup.LastUpdated),
)

return rateLimitAnnotations(createResp), nil
default:
return nil, fmt.Errorf("okta-connector: invalid grant resource type: %s", principal.Id.ResourceType)
}

return nil, nil
}

func (g *appResourceType) Revoke(ctx context.Context, grant *v2.Grant) (annotations.Annotations, error) {
Expand All @@ -538,59 +536,61 @@ func (g *appResourceType) Revoke(ctx context.Context, grant *v2.Grant) (annotati
userID := principal.Id.Resource
_, resp, err := g.client.Application.GetApplicationUser(ctx, appID, userID, nil)
if err != nil {
if resp != nil && resp.StatusCode == http.StatusNotFound {
l.Debug(
"okta-connector: revoke: user does not have app membership",
zap.String("principal_id", principal.Id.String()),
zap.String("principal_type", principal.Id.ResourceType),
)
return annotations.New(&v2.GrantAlreadyRevoked{}), nil
}
l.Warn(
"okta-connector: user does not have app membership",
zap.String("principal_id", principal.Id.String()),
zap.String("principal_type", principal.Id.ResourceType),
)
return nil, fmt.Errorf("okta-connector: user does not have app membership: %s", err.Error())
return appRevokeNotFoundOrError(l, principal, resp, err, "user")
}

response, err := g.client.Application.DeleteApplicationUser(ctx, appID, userID, nil)
if err != nil {
return nil, fmt.Errorf("okta-connector: failed to remove user from application: %s", err.Error())
return nil, fmt.Errorf("okta-connector: failed to remove user from application: %w", handleOktaResponseError(response, err))
}
logAppMembershipRevoked(l, response)

if response != nil && response.StatusCode == http.StatusNoContent {
l.Warn("Membership has been revoked",
zap.String("Status", response.Status),
)
}
return rateLimitAnnotations(response), nil
case resourceTypeGroup.Id:
groupID := principal.Id.Resource
_, _, err := g.client.Application.GetApplicationGroupAssignment(ctx, appID, groupID, nil)
_, groupResp, err := g.client.Application.GetApplicationGroupAssignment(ctx, appID, groupID, nil)
if err != nil {
l.Warn(
"okta-connector: group does not have app membership",
zap.String("principal_id", principal.Id.String()),
zap.String("principal_type", principal.Id.ResourceType),
)
return nil, fmt.Errorf("okta-connector: group does not have app membership: %s", err.Error())
return appRevokeNotFoundOrError(l, principal, groupResp, err, "group")
}

response, err := g.client.Application.DeleteApplicationGroupAssignment(ctx, appID, groupID)
if err != nil {
return nil, fmt.Errorf("okta-connector: failed to remove group from application: %s", err.Error())
return nil, fmt.Errorf("okta-connector: failed to remove group from application: %w", handleOktaResponseError(response, err))
}
logAppMembershipRevoked(l, response)

if response != nil && response.StatusCode == http.StatusNoContent {
l.Warn("Membership has been revoked",
zap.String("Status", response.Status),
)
}
return rateLimitAnnotations(response), nil
default:
return nil, fmt.Errorf("okta-connector: invalid grant resource type: %s", principal.Id.ResourceType)
}
}

// appRevokeNotFoundOrError classifies a failed pre-delete existence check ("user" or
// "group" kind): already-gone yields GrantAlreadyRevoked, else the wrapped lookup error.
func appRevokeNotFoundOrError(l *zap.Logger, principal *v2.Resource, resp *okta.Response, err error, kind string) (annotations.Annotations, error) {
if isRevokeNotFoundError(resp, err) {
l.Debug(
fmt.Sprintf("okta-connector: revoke: %s does not have app membership", kind),
zap.String("principal_id", principal.Id.String()),
zap.String("principal_type", principal.Id.ResourceType),
)
return annotations.New(&v2.GrantAlreadyRevoked{}), nil
}

return nil, nil
l.Warn(
fmt.Sprintf("okta-connector: %s does not have app membership", kind),
zap.String("principal_id", principal.Id.String()),
zap.String("principal_type", principal.Id.ResourceType),
)
return nil, fmt.Errorf("okta-connector: %s does not have app membership: %w", kind, handleOktaResponseError(resp, err))
Comment on lines +580 to +585

@github-actions github-actions Bot Aug 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: this fallback branch fires for every non-404 pre-check failure, including the 429 and 5xx cases this PR makes retryable — so a rate-limited app revoke now logs okta-connector: user does not have app membership at Warn and wraps the returned error with the same wrong text, hiding the real cause. Consider logging/wrapping generically (e.g. "failed to look up app membership") and letting handleOktaResponseError supply the classification.

}

// logAppMembershipRevoked logs the NoContent delete confirmation shared by both
// app revoke branches.
func logAppMembershipRevoked(l *zap.Logger, resp *okta.Response) {
if resp != nil && resp.StatusCode == http.StatusNoContent {
l.Debug("Membership has been revoked", zap.String("Status", resp.Status))
}
}

func (o *appResourceType) Get(ctx context.Context, resourceId *v2.ResourceId, parentResourceId *v2.ResourceId) (*v2.Resource, annotations.Annotations, error) {
Expand Down
6 changes: 6 additions & 0 deletions pkg/connector/connector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ const oktaURLScheme = "https"
// oktaSDKAuthSentinel activates the SDK's Bearer auth path; the oktaauth RoundTripper substitutes the real DPoP/Bearer token per request.
const oktaSDKAuthSentinel = "dpop-managed"

// oktaRateLimitMaxBackoffSeconds covers Okta's ~60s rate-limit reset window; the bigger
// fix is classifying 429 as Unavailable (rateLimitError) so provisioning retries at all.
const oktaRateLimitMaxBackoffSeconds = 60

type Okta struct {
client *okta.Client
clientV5 *oktav5.APIClient
Expand Down Expand Up @@ -429,6 +433,7 @@ func New(ctx context.Context, cc *cfg.Okta, opts *cli.ConnectorOpts) (connectorb
okta.WithCache(cc.Cache),
okta.WithCacheTti(cacheTTI),
okta.WithCacheTtl(cacheTTL),
okta.WithRateLimitMaxBackOff(oktaRateLimitMaxBackoffSeconds),

@github-actions github-actions Bot Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: This doubles the SDK default (WithRateLimitMaxBackOff(30), okta.go:187) for every request, sync included, not just provisioning. With the default MaxRetries(2) and RequestTimeout(0) (no per-request deadline), a rate-limited call can now block in-process for ~120s before the connector ever sees the error and can annotate/checkpoint. Worth confirming that is intended for the sync path, or pairing with an explicit okta.WithRequestTimeout(...).

@mateoHernandez123 mateoHernandez123 Aug 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(...)?

)
if err != nil {
return nil, nil, err
Expand Down Expand Up @@ -479,6 +484,7 @@ func New(ctx context.Context, cc *cfg.Okta, opts *cli.ConnectorOpts) (connectorb
okta.WithCache(cc.Cache),
okta.WithCacheTti(cacheTTI),
okta.WithCacheTtl(cacheTTL),
okta.WithRateLimitMaxBackOff(oktaRateLimitMaxBackoffSeconds),
)
if err != nil {
return nil, nil, err
Expand Down
Loading
Loading