Bifrostbase/create session - #50157
Conversation
* generate base files based on swagger docs * create live tests for createSession - downgrade blobserviceversion to 2026_04_06 - change AZURE_LIVE_TEST_SERVICE_VERSION to V2026_04_06 in ci.system.properties in azure-storage-common - create both sync and async * add recordings * create new files based on swagger update * add two params to BlobContainerClient#createSessionWithResponse * add sanitizers for SessionToken and SessionKey to BlobTestBase * add recording for createSessionReturnsTokenAndKey * create StorageSessionCredential with isExpired * create BlobSessionClient so that BlobSessionProvider takes it as a dep instead of ContainersImpl * create BlobSEssionClient with tests * add recorings for BlobSessionClient * fix BlobContainerAsyncClient to match new swagger, add new recording * add SessionProvider and SessionProviderTest * add accountName to BlobSessionClient * add accountName to StorageSessionCredential and SesionTestHelper * wip * change sessionprovider to SEssionTokenCredentialPolicy * wip * move session tests from containerapi to blobsessionclienttests * fix blobsessiontests and add place holder for end-to-end tests in containerapitests * add recordings for blobsessionclient * linting * refactor cache into separate class so it follows BearerTokenAuthenticationPolicy + AccessTokenCache pattern * add 503 fallback * add tests for udsas, but disabled for now * refactor createContext to use hardcoded endpoint * add SessionMode and tests for SessionMode * add sessionOptions to buildPipeline, add null to builders not using sessions * move SessionTokenCredentialPolicy ahead of StorageBearerTokenChallengeAuthorizationPolicy * fix linting issues * add session to BlobServiceClients and BlobServiceClientBuildeer * change expiration so that it defaults to 5 minutes, if there is no expiration * move SessionOptions so that it is public * remove old SessionOptions * remove unnecessary references to containerName and serviceVersion * add BlobContainerSessionInfo, add other Copilot recommendations * delete BlobContainerSessionInfo, restore return CreateSessionResponse * create createSession end-to-end test with recordings * only allow get requests for getblob * wrap tests in try-with-resources * make createSession package private * fixes based on copilot suggestions * Update sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/implementation/util/BlobSessionClient.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * add containerName to SessionOptions * move accountName to SessionOptions * refactor: SessionTokenCredentialPolicy accepts bearer policy as constructor dependency SessionTokenCredentialPolicy now takes StorageBearerTokenChallengeAuthorizationPolicy as a constructor dependency instead of relying on pipeline ordering to coordinate with the bearer policy. Key changes: - SessionTokenCredentialPolicy delegates to bearerPolicy.process() for non-session requests (non-GetBlob, NONE mode, AUTO first request) and fallback (503). - Added getBearerPolicy() accessor for use in per-container pipeline construction. - BuilderHelper updated to pass bearer policy through to SessionTokenCredentialPolicy constructor in both addSessionPolicyIfEnabled and wrapWithSessionPolicy. - Tests updated to mock bearerPolicy and verify delegation behavior. This is step 1 of the session auth refactor to align with the .NET SDK's SessionAuthenticationPolicy pattern. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor: introduce AuthStrategy enum and consolidate analyzeRequest Consolidate isGetBlobRequest() and shouldUseSession() into a single analyzeRequest() method that returns an AuthStrategy enum (USE_BEARER_TOKEN or USE_SESSION_TOKEN), following the .NET SessionAuthenticationPolicy pattern. Also extract response handling into handleSessionResponse() and handleSessionResponseSync() methods for cleaner process()/processSync() flow. The process methods now have a clear structure: 1. analyzeRequest() -> decide strategy 2. USE_BEARER_TOKEN -> delegate to bearer policy 3. USE_SESSION_TOKEN -> acquire session, sign, send, handle response No behavioral changes - all existing tests pass unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor: remove redundant restype check from analyzeRequest The restype query parameter check was redundant because requests with restype=container are container-level operations that have no blob name. They are already filtered by the blob name presence check. This aligns more closely with the .NET SessionAuthenticationPolicy which relies on URL structure rather than query parameter checks. The comp check is retained as a safety measure to exclude sub-operations like GetBlobMetadata (comp=metadata) that have a blob name but are not download operations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * change sessionmode from always to singlespecifciedcontainer, add resolve method * wrap bearer token in sessioncredentialpolicy * fix NPE for SessionOptions, sessionoptions always non null * add tests for sessiontokencredpolicy and storagesessioncred * add logic to avoid wrapping Bearertoken, if session is not needed * add overloaded oauth in blobtestbase to be able to add sessionoptions * add overloaded getOAuthServiceAsyncClient to be able to pass session options * add custom buildStringToSign to remove `0` from get requests * readd versions * readd ci.system.properties * change session options check for null in BuilderHelper which affected other tests * add recordings for create sessions, change time to testResource time * add requestInspectionPolicy and overloaded getoauth client in base test * fix null sessionsoptions issue * add fix in storagesessioncredntial for query params * add SessionTokenCredPolicy to checks for anonymousAccess * remove constructor for BlobSessionClients that uses parse url * linting issues, remove SessionOptions from service methods * add comments to policyrefreshNearExpiry test * fix linting issues * add check for container name * fix return javadoc for SessionMode * add LOGGER and appropriate error throwing in BlobSessionClient * changes based on feedback from isabelle * add single retry for all 401 errors * remove unused imports * add suppression for SessionTokenPolicy for linting * fix ubuntu tests hanging by removing local dns bypass * add session for blob client with tests * create unified http transport between data requests and session request, add ResourceLock for live tests * test multiple concurrency * add branching if for bearer policy * refactor validateSessionMode to builderhelper * add revert to bearertoken for 400 errors * remove unused imports * readd comment to bearertokechallenge --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…hing into BlobSessionClient - Expand SessionProvider with default invalidateSession/refreshSession lifecycle methods - Move AutoRefreshingCache per-container caching from deleted SessionCredentialManager into BlobSessionClient - Extract account cooldown into narrow SessionAcquisitionCooldown helper - SessionTokenCredentialPolicy depends directly on SessionProvider; no manager wrapper - BYO custom providers are used directly without SDK-owned cache wrapping - Remove SessionOptions.orDefault (internal helper made public accidentally) - Update CHANGELOG and SessionOptions JavaDoc to reflect new BYO provider ownership model - Add BlobSessionClientCacheTest, SessionAcquisitionCooldownTest, update SessionTokenCredentialPolicyTest Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 1 pipeline(s). 34 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
… for Autorefcache
Isabelle (ibrandes)
left a comment
Isabelle (ibrandes)
left a comment
There was a problem hiding this comment.
sorry this took so long! this was a dense feature x)
| * {@link Function} that extracts the expiration instant from a value, decoupling this cache from any | ||
| * specific credential shape. | ||
| */ | ||
| public final class AutoRefreshingCache<T> { |
There was a problem hiding this comment.
tbh, i'm reluctant to reimplement this from scratch. core already has refresh/in flight dedup machinery, but since those are AccessToken specific and can't carry the session HMAC key, we can't reuse it.... maybe we should ask someone on core if they would be able to provide a generic auto refresh cache (generalizing simpletokencache over a value + expiry extractor)? idk. alternatively, if we keep our own cache, we should mirror core's concurrency model (with a single AtomicReference<Sinks.One<T>> + Mono.defer for in-flight dedup, instead of the current hand-rolled synchronized + inflightCreation + block() mix in getValidValueSync, which is the riskiest part of this class.
There was a problem hiding this comment.
Okay. I think I'm going to leave the implementation like this for now for the private drop (and look into changing the sync implementation before the release on the private drop. Since both datalocality and create session rely on it, I would have to change it in two places, so I'll merge them into the base branch, then change it there.
But I've created a task for this on the board to check to see whether core will agree to creating a generic for us from the SimpleTokenCache.
| * <p>The client supports a global FIFO queue, optional request-keyed FIFO queues, custom response factories, | ||
| * explicit failures, and pending responses that can be completed later by the test.</p> | ||
| */ | ||
| public final class ScriptedHttpClient implements HttpClient { |
There was a problem hiding this comment.
this is a lottt of new test infrastructure that kind of overlaps what we already have - WireTapHttpClient,a nd the stuff provided by core-test -- MockHttpClient, NoOpHttpClient, and MockHttpResponse`.
where used, the vast majority of call sites just do new ScriptedHttpClient().enqueueResponse(200), which a one-line inline HttpClient returning a MockHttpResponse (or a subclass of NoOpHttpClient) already covers... only TokenCredentialSessionProviderCacheTest uses the richer features (keyed queues, enqueuePendingResponse, enqueueFailure). could we trim this to just the features actually exercised, and lean on WireTapHttpClient where we only need to capture the last request?
There was a problem hiding this comment.
I had to read this a few times.
- Keep it for TokenCredentialSessionProviderCacheTest? Perhaps contained within the class might be better.
- Yeah I'll change the others to WireTapHttpClient.
| * <p>Used by the session-auth live tests as a wire-level sanity check (e.g. to assert | ||
| * which authentication scheme was applied to a given request).</p> | ||
| */ | ||
| public final class RequestInspectionPolicy implements HttpPipelinePolicy { |
There was a problem hiding this comment.
this is a behavioral dupe of WireTapHttpClient - which also sees the final Authorization header these assertions check. if a policy is genuinely preferred here, we normally just use an inline HttpPipelinePolicy lambda for this. regardless, if we keep this policy, we should move it to test-shared so the datalake tests can reuse it rather than adding a second copy.
| private DataLakeServiceVersion version; | ||
| private FileSystemEncryptionScopeOptions fileSystemEncryptionScopeOptions; | ||
| private DataLakeAudience audience; | ||
| private SessionOptions sessionOptions = new SessionOptions(); |
There was a problem hiding this comment.
this field is left in a dead state - the default value is never forwarded to blobServiceClientBuilder (only the setter forwards), and the underlying blob builder already defaults to ENABLED on its own. we should consider removing this field and just forwarding in the setter to avoid implying the datalake builder holds independent session state.
| * </ol> | ||
| * <p> | ||
| * Regardless of the provider used, the SDK always retains ownership of HMAC request signing, bearer-token | ||
| * fallback for ineligible requests, and account-level acquisition cooldown (suppressing further session |
There was a problem hiding this comment.
"account-level acquisition cooldown) is inaccurate here, since accountCooldowns lives on SessionTokenCredentialPolicy, which is instantiated per pipeline / per service client. when a custom SessionProvider is shared across multiple service clients (as this same doc encourages, to share its cache), each client keeps an independent cooldown map, so a cooldown tripped on one client won't suppress acquisition on another, even for the same account.
| * Handles the response after a session-authenticated async request. Inspects for | ||
| * session-expiring hints, retryable failures, and fallback conditions. | ||
| */ | ||
| private Mono<HttpResponse> handleSessionResponse(HttpPipelineCallContext context, HttpResponse response, |
There was a problem hiding this comment.
doesnt this need to engage the account cooldown after a 401? after evicting the cached credential and falling back to to bearer, i don't see a way to note the cooldown. i only see it in handleSessionAcquisitionFailure, when acquisition fails.
when we're handling the response here, if the service accepted a create session request but rejected the session token on the data path, (e.g an account container where session auth isn't honored), every request after will re-mint a session, get with the session, recieve a 401, invalidate it, and initiate a bearer retry (indefinitely with no backoff).
please let me know if i'm missing something though!
| nextRefreshTime = null; | ||
| refreshing = false; | ||
| } | ||
| inflightCreation = null; |
There was a problem hiding this comment.
should this be inside the value == target guard?
| synchronized (creationLock) { | ||
| current = value; | ||
| now = OffsetDateTime.now(clock); | ||
| if (isUsable(current, now)) { | ||
| if (isRefreshDue(now)) { | ||
| refreshValueInBackground(); | ||
| } | ||
| return current; | ||
| } | ||
|
|
||
| T created = valueProvider.createSync(); | ||
| setActiveValue(created); | ||
| return created; | ||
| } |
There was a problem hiding this comment.
i didn't dive too deep into the locking logic here as its not something i'm comfortable with (and why i suggested to use core's logic) but copilot generated this -
"This calls valueProvider.createSync() while holding creationLock, but startSessionCreationAsync also synchronizes on creationLock to register the in-flight async creation. So while one thread is blocked in a synchronous CreateSession HTTP call under the lock, any async caller entering startSessionCreationAsync blocks on creationLock until that network call returns — coupling async latency to unrelated sync calls, in a class that advertises a non-blocking async path. (Not a deadlock, since the async block() join is correctly done outside the lock.)"
…peline Session credentials are bound to the network context of the call that created them, so CreateSession must take the same network path as the data requests its credential signs. Hoist the per-retry, after-retry, response validation, logging and ScrubEtag policies into a postAuthenticationPolicies list shared by the data pipeline and the bearer-only CreateSession pipeline. Main pipeline ordering is unchanged.
Invalidating a rejected session makes the next request create a brand new one, so an environment where sessions can never be used would mint and lose a session on every request indefinitely. Count consecutive 401s per account and, after three, place the account into the existing five minute cooldown so requests fall straight through to bearer. Any non-401 response clears the count.
Add SessionToken and SessionKey body sanitizers so session secrets never reach recordings, matching the pair already present in BlobTestBase. readFileOverSessionAuth cannot be recorded at all: session credentials are bound to the network context of the CreateSession call and the TLS-terminating test proxy invalidates them, so mark it @liveonly instead of @disabled.
The HttpPipelineBuilder configuration was written out twice, and the bearer-only CreateSession pipeline was threaded through an extra single-caller method that re-resolved a service version its caller had already resolved. Extract createPipeline for the shared transport, client options and tracer setup, and fold buildBearerPipeline into createDefaultSessionProvider. No behavior change.
2 participants
No description provided.