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: [email protected]
For additional commands, e-mail: [email protected]
