JingsongLi commented on PR #10178: URL: https://github.com/apache/paimon/pull/10178#issuecomment-5951841200
Reviewed current head `5d6796f9f847533a4e3022517d3272b602dddfdb`. The open-stream token-refresh use case has clear end-to-end value. The earlier cache-eviction/supplier-lifetime finding is addressed: open streams refresh independently of the cache, and table/user isolation is now preserved. One new credential-replacement issue remains. **[P2] Replace the previous vended credential bundle instead of merging refreshes over it** (`RESTTokenRefresher.java:169`). `fromOptions` receives options that already contain the initial server-issued OSS keys and security token. `load()` then overlays each new response on those initial merged options. If a valid replacement response provides a new access-key pair and omits `fs.oss.securityToken` (ordinary AK-only credentials are supported), the omitted field retains the old STS token. The SDK consequently signs with the new key pair while sending the previous, unrelated/expired security token. I reproduced this through real `OSSFileIO` and the OSS SDK against local REST/HTTP fixtures: after refresh, both UploadPart and CompleteMultipartUpload used the new access key together with `x-oss-security-token: sts-1`. A fresh delegate built from the original catalog-only options and the same replacement response has no security token. This can make an otherwise valid rotation fail with OSS authentication errors. Please keep original catalog configuration separate from the initial vended credentials, or replace the storage credential fields atomically on refresh. Add an omission/removal test as well as the existing full-token rotation tests. Validation on JDK 8 with normal Maven checks: RESTTokenRefresherTest (6), RESTTokenFileIOTest (12), RESTTokenCredentialsProviderTest (3), RESTTokenFileIOOnOSSTest (2), OSSLoaderTest (1), all passed. An additional actual SDK multipart test uploaded 20 MiB across refresh and completed using the new token. Independent real HTTP tests verified two catalog users with identical initial OSS credentials, branch identity, serialization, cache eviction and eight signed range reads; each stream refreshed as its own user. Current-head CI is green. These use deterministic local REST/OSS protocol fixtures; no live OSS account was exercised. The pre-existing plugin-close/resource leak is outside this finding. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
