[jira] [Created] (HDDS-17216) S3 secret cache cleanup can remove the entry of an unflushed transaction, so OMs can commit different S3SecretTable contents

["Siyao Meng (Jira)" <ji...@apache.org>]

Siyao Meng created HDDS-17216:
---------------------------------

             Summary: S3 secret cache cleanup can remove the entry of an unflushed transaction, so OMs can commit different S3SecretTable contents
                 Key: HDDS-17216
                 URL: https://issues.apache.org/jira/browse/HDDS-17216
             Project: Apache Ozone
          Issue Type: Bug
            Reporter: Siyao Meng


h3. Mechanism
Each write to an S3 secret puts an entry carrying its transaction index into {{S3InMemoryCache}}. After the double buffer has committed a batch, the flush thread calls {{S3InMemoryCache.clearCache}} with the batch's indexes, which removes the entries carrying those indexes, while the apply thread keeps applying later transactions. A read that misses the cache loads the committed row and caches it with index 0. The cache is consistent as long as every applied but unflushed write keeps its entry until its own index is flushed. Three places do not ensure that:
# {{S3RevokeSecretRequest}} replaces the entry with {{S3SecretValue.deleted()}}, which keeps the index of the entry it replaces instead of the revoke's index. When the revoke is applied after the batch holding the previous write to that access ID was committed and before that batch's cleanup, the cleanup removes the deleted entry although the revoke is not flushed.
# {{OMTenantRevokeUserAccessIdRequest}} calls {{invalidateCacheEntry}}, which only replaces an entry that is already cached ({{computeIfPresent}}), and calls it without the access ID's {{S3_SECRET_LOCK}}. If the access ID has no cache entry, the tenant revoke leaves none.
# {{clearCache}} maps each cached index to its access ID in a first pass and removes each mapped access ID in a second pass, without checking the entry's index again. {{S3SecretLockedManager.clearS3Cache}} holds the {{S3_SECRET_LOCK}} stripe of the key "cache", not of the access IDs, so a write to the access ID can be applied between the two passes, and its entry is removed.

In each case a read that misses the cache before the newer transaction is flushed caches the older row with index 0, which the cleanup of the newer transaction does not match. After that flush, the OM's cache entry for the access ID no longer matches its committed row.

Which OMs are affected depends on when each OM's double buffer flush runs relative to its apply thread. That timing is local to each OM and is not part of the Ratis log.

h3. Impact
* *OMs can commit different S3SecretTable contents.* Two OMs apply the same log create@1, revoke@2, create@3 for one access ID. On an OM that flushes all three in one batch, create@3 ({{GetS3Secret}}) returns OK and commits the new secret. On an OM that applies revoke@2 between the commit and the cleanup of batch [1], create@3 reads the older row, returns {{S3_SECRET_ALREADY_EXISTS}} and writes nothing, so after both OMs have flushed index 3 one holds the new secret and the other holds no row for the access ID. Nothing compares or repairs the tables of OMs at the same applied index, so the difference survives restarts. A later {{GetS3Secret}} or {{SetS3Secret}} for that access ID can likewise get a different result on an OM whose cache entry no longer matches its committed row than on the others.
* *Cache entry and committed row disagree.* After path 3 the DB holds the newer write, but the OM's cache entry for the access ID does not match its committed row until the OM restarts or the access ID is written again.

Affected versions: the deleted entries and the index based cleanup (paths 1 and 3) exist since HDDS-9444, path 2 since the tenant revoke request. The missing cleanup registration described under Patch is on master only, since HDDS-16643.

h3. Reproduction
There is no standalone reproduction attached. The regression tests are in the patch, in the existing {{TestS3GetSecretRequest}}: real {{OmMetadataManagerImpl}} (RocksDB), {{S3SecretLockedManager}} over {{S3SecretManagerImpl}} with the in-memory cache, requests applied through {{preExecute}} and {{validateAndUpdateCache}}, and each flush done in two steps by the test (commit the responses with {{checkAndUpdateDB}}, then {{clearS3Cache}} for the responses that registered the S3 secret table), so the interleavings above become a plain sequence of calls. All four fail without the change:
* {{testRevokeKeptInCacheWhenEarlierIndexIsCleanedUp}} (path 1): commit [1], apply revoke@2, clean up [1]; expects the deleted entry with index 2 to stay until the revoke is flushed.
* {{testCreateAfterRevokeWhenEarlierIndexIsCleanedUp}} (path 1, the diverging OM): the same, then {{GetS3Secret}}@3; fails with "expected: OK but was: S3_SECRET_ALREADY_EXISTS".
* {{testTenantRevokeOfUncachedSecretIsKeptInCache}} (path 2): the secret is committed and not cached, tenant revoke@1 is applied; expects a deleted entry until the revoke is flushed.
* {{testCacheCleanupKeepsEntryReplacedDuringCleanup}} (path 3): {{SetS3Secret}}@2 is applied at the moment {{clearCache([1])}} reads the index of the create's entry (a Mockito spy on that cache value); expects the set's entry to stay.

The two OM case and paths 1 and 3 were also run through the flush method of a real {{OzoneManagerDoubleBuffer}} (the one its daemon calls) on two OM stacks in one JVM, with the results described under Impact. These runs show that the interleavings exist, not how often they occur.

h3. Patch
[^MC-2-keep-s3-secret-cache-entries-until-own-index-flushed.patch], against df04bc0e309f625a7dcdcd60790a42f34e2743d9. It also applies to master at 2cb1b6dd4e9.

The patch keeps every cache entry until its own transaction is flushed:
* {{S3SecretCache.invalidate(id, transactionLogIndex)}} (new, with a {{S3SecretManager.invalidateCacheEntry}} counterpart) always puts a deleted entry carrying the revoking transaction's index, whether or not the access ID is cached. {{S3RevokeSecretRequest}} passes {{context.getIndex()}}. {{OMTenantRevokeUserAccessIdRequest}} passes its {{transactionLogIndex}} and now calls it under the access ID's lock through {{doUnderLock}}, as {{OMTenantAssignUserAccessIdRequest}} already does. The index is the Ratis index every OM applies the entry with, so no request or proto change is needed.
* {{S3InMemoryCache.clearCache}} removes an entry only if it still carries the flushed index ({{computeIfPresent}}).
* The new {{invalidateCacheEntry}} records the S3 secret table with {{TableCacheUpdateTracker}}, as {{updateCache}} does, so the double buffer runs the cleanup for the revoke's index. Since HDDS-16643 a revoke whose secret was already cached did not register that table.

For stores with batch support (the default RocksDB store), every unflushed write is then represented by an entry that no earlier cleanup removes, and reads and writes of an access ID are serialized by its lock, so a cache miss means the committed row is current and an entry loaded with index 0 holds it. On stores without batch support the store is written at flush time; that is addressed by HDDS-17214, which also edits {{S3RevokeSecretRequest}}, {{OMTenantRevokeUserAccessIdRequest}} and {{TestS3GetSecretRequest}}, so the two patches need a rebase whichever lands second. The existing one argument {{invalidate}} is kept unchanged for its remaining caller, the flush time {{S3SecretManagerImpl.revokeSecret}} of those stores.

The lock in {{OMTenantRevokeUserAccessIdRequest}} is argued from the code (a read that missed the cache could otherwise overwrite the new deleted entry with the committed row) and is not covered by a failing test.

The patch includes the {{S3SecretManagerImpl.getSecretString}} change proposed in HDDS-17200 so that it is complete on its own; drop that hunk if HDDS-17200 is committed first.

Without the races the behaviour is unchanged, except that a deleted entry is now removed when the revoke itself is flushed instead of when the replaced entry's index is flushed.

With the patch the four tests pass, the ozone-manager unit suites for S3 requests and responses, the double buffer, the delegation token secret manager and multi tenancy pass (315 tests), {{TestMultiTenantVolume}}, {{TestSecureOzoneCluster.testGetSetRevokeS3Secret}} and {{TestSecureOzoneRpcClient.testS3Auth}} pass on a mini cluster (7 tests), and checkstyle is clean.

Found by TLA+ model checking and code review of the OM S3 secret cache and external S3 secret store paths under HDDS-15926, on commit df04bc0e309f625a7dcdcd60790a42f34e2743d9. Checked against HDDS issues and apache/ozone pull requests for duplicates before filing. Related but not duplicates: HDDS-17200 (deleted cache entries in {{getSecretString}}), HDDS-9444 (introduced the index based cleanup and the deleted entries), HDDS-16643 (removed the static cleanup table declarations) and HDDS-5358 (an earlier invalidation fix). The attached patch is a proposal for review. Generated with Specula (Claude Opus 5.5).



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org