Retry stale service-credential 403s on the internal backbeat routes - #2793
Draft
delthas wants to merge 2 commits into
Draft
Retry stale service-credential 403s on the internal backbeat routes#2793delthas wants to merge 2 commits into
delthas wants to merge 2 commits into
Conversation
A credential rotation leaves backbeat and cloudserver disagreeing until both have restarted, so cloudserver rejects the key it does not know with a 403. Smithy marks a 403 non-retryable, which is right for a customer endpoint but drops the replication-status entry on the internal routes: the object keeps a PENDING status, is never marked FAILED, and is invisible to crrRetry. Retry InvalidAccessKeyId and SignatureDoesNotMatch a bounded number of times on the three BackbeatMetadataProxy source-metadata calls. internal-cloudserver runs several replicas and rolls one pod at a time, so a later attempt can be served by an already-updated pod. AccessDenied stays non-retryable: on an internal route it is the signal for a misconfigured source role. The budget is scoped to the stale-credential class by a stateful predicate, so the existing err.retryable behaviour is unchanged, and it spans ~13s on the proxy's default backoff, far below max.poll.interval.ms. Issue: BB-832
MultipleBackendTask._handleReplicationOutcome listed InvalidAccessKeyId as a permanent failure and returned a bare done(): the offset was committed with no status published, so the object stayed PENDING and never reached the failed-CRR listing. ReplicateObject already omits it and falls through to publishing FAILED with committable: false. Align the subclass with its parent so an entry whose retries were exhausted stays visible to crrRetry instead of being lost silently. Issue: BB-832
Contributor
Hello delthas,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 4 files with indirect coverage changes
@@ Coverage Diff @@
## development/9.5 #2793 +/- ##
===================================================
+ Coverage 75.50% 75.64% +0.14%
===================================================
Files 201 202 +1
Lines 13928 13932 +4
===================================================
+ Hits 10516 10539 +23
+ Misses 3402 3383 -19
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
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
2 participants
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.
Problem
Artesca regenerates builtin service-account keys on every overlay write (PSVAPI-134). Backbeat and cloudserver each snapshot authdata at process start and neither reloads it, so during the rolling restarts that follow a rotation cloudserver returns
403 InvalidAccessKeyIdon internal calls.Smithy classifies a 403 as non-retryable (
lib/clients/utils.js), every retry wrapper gates onerr.retryable, andBackbeatConsumercommits the offset regardless — so a replication-status entry is destroyed outright. The failure lands in_refreshSourceEntry, upstream of every FAILED bookkeeping step, so the object keeps itsPENDINGstatus, never enters the failed-CRR sorted set, and is invisible to crrRetry.A census on the ticket (10 CTST runs, 100 rejected requests, 8 skew episodes) found 95 % of these are cloudserver-stale — so a retry in backbeat can succeed — and that staleness is a per-request property: internal-cloudserver runs 4 replicas to backbeat's 1 and rolls a pod at a time, so a later attempt can be served by an already-updated pod. One backbeat pod was observed both failing and succeeding within the same second.
Changes
lib/util/staleCredentialError.js(new) —isStaleCredentialError()matchingInvalidAccessKeyIdandSignatureDoesNotMatch, andretryOnStaleCredentials(), a stateful predicate that preserves the existingerr.retryablebehaviour and adds a bounded number of stale-credential retries.AccessDeniedis deliberately excluded: on an internal route it is the existing signal for a misconfigured source role (lib/util/replicationPermissionError.js) and must keep failing fast.lib/BackbeatMetadataProxy.js— the three source-metadata calls (getMetadata,putMetadata,headLocation) use that predicate. A fresh one perretry()call, so the budget applies to a single logical operation.extensions/replication/tasks/MultipleBackendTask.js—_handleReplicationOutcomelistedInvalidAccessKeyIdas a permanent failure and returned a baredone(), committing the offset with no status published.ReplicateObjectalready omits it and publishes FAILED withcommittable: false; the subclass now matches, so an entry whose retries are exhausted stays visible to crrRetry.Notes for review
super()with no retryParams, so it uses the defaults (min: 1000, factor: 1.5): attempts at 1 s, 2.5 s, 4.75 s, 8.1 s, 13.2 s — ~13 s total, ~20x undermax.poll.interval.ms(300 s). This matters because the failure is correlated across allconcurrency: 10slots during a skew window. A unit test guards the property.60000ms backoff /900 stimeout picked inReplicateObject.js:67-71belongs to the task instances and only governs retries those tasks start themselves.MultipleBackendTask._refreshSourceEntrycalls the proxy directly, so a stale-credential retry runs on the proxy's own params regardless of destination type.committable: falsedefers the commit, it does not withhold it —_publishReplicationStatuscallsonEntryCommittablein the producer callback whether the produce succeeded or not, and this is already the path taken by every COMPLETED and FAILED outcome. No new stall mode; the change removes a special case that bypassed the normal path.isRetryableMiddleware, so a customer-destination 403 stays permanent by construction. The four call sites that opt in only ever talk to the internal endpoint.Deliberately out of scope
GetBucketReplicationin_setupRolesOnce) are not retried. With the second commit they become visible FAILED entries rather than silent losses.operatorBackendcredential reload would close it.Issue: BB-832