[
https://issues.apache.org/jira/browse/HDDS-17214?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17214:
------------------------------
Attachment: MC-5-update-non-batch-s3-secret-store-at-apply-time.patch
> With an S3 secret store without batch support, a revoke can delete a newer
> secret and a store failure at flush time terminates the OM
> -------------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17214
> URL: https://issues.apache.org/jira/browse/HDDS-17214
> Project: Apache Ozone
> Issue Type: Bug
> Reporter: Siyao Meng
> Priority: Major
> Attachments:
> MC-5-update-non-batch-s3-secret-store-at-apply-time.patch,
> TestBugMC5NonBatchRevokeThenGetSecret.java
>
>
> h3. Mechanism
> For an S3 secret store whose {{S3SecretStore.batcher}} returns null, such as
> {{VaultS3SecretStore}}, the OM updates the store at two different points.
> Since HDDS-10469, {{S3GetSecretRequest.validateAndUpdateCache}} writes a new
> secret to the store when the entry is applied. Revoke, set and the tenant
> access ID requests still update the store when the double buffer flushes the
> entry: {{S3RevokeSecretResponse.addToDBBatch}} calls
> {{S3SecretManagerImpl.revokeSecret}}, and {{OMSetSecretResponse}},
> {{OMTenantAssignUserAccessIdResponse}} and
> {{OMTenantRevokeUserAccessIdResponse}} do the same for their updates.
> {{OzoneManagerStateMachine.applyTransaction}} answers the client after apply
> and does not wait for the flush, and the store delete is unconditional.
> This has two consequences.
> * *A revoke deletes a newer secret.* A revoke at index n only marks the cache
> entry as deleted at apply time. A getsecret for the same access ID at index
> n+1, applied before n is flushed, sees the deleted marker, writes a new
> secret to the store and returns it to the client. The flush of n then deletes
> that secret from the store and marks the new cache entry as deleted. After
> the cache entry is cleaned up, {{S3SecretManagerImpl.getSecretString}} throws
> S3_SECRET_NOT_FOUND for the secret the client holds.
> * *A store failure at flush time terminates the OM.* If the store call in
> {{addToDBBatch}} fails, the request has already been answered as successful,
> and {{OzoneManagerDoubleBuffer}} terminates the OM before the batch and the
> transaction info are committed. The transaction info does not advance, so
> each restart replays the entry, calls the store again and terminates again
> until the call succeeds.
> Affected versions: the lost secret since 2.0.0 (HDDS-10469 moved only the
> getsecret write to apply time); the flush time termination since the external
> store was added in 1.4.0 (HDDS-7814).
> h3. Trigger
> An external store without batch support (Vault, configured through
> {{ozone.secret.s3.store.provider}}).
> * Lost secret: a revoke and a getsecret for the same access ID applied by one
> OM in the same flush window. On the leader that needs the two requests to
> follow each other closely. In OM HA every OM applies the log and updates the
> shared store itself, so a follower that applies both entries before flushing
> the revoke (for example while catching up) deletes the secret the leader
> issued, however far apart the client sent them. The follower case is argued
> from the code; the attached reproduction runs one OM.
> * OM termination: the store call fails (store unreachable, or rejecting
> writes) while a revoke, set, tenant access ID assign or tenant access ID
> revoke is flushed.
> h3. Impact
> * The secret returned by getsecret is not in the store, so S3 requests signed
> with it fail with S3_SECRET_NOT_FOUND, also after an OM restart. Nothing
> reports the loss; the user has to call getsecret again and reconfigure with
> the new secret. Fail closed, no access is gained.
> * A store failure at flush time stops the OM for all clients, not only S3,
> after the request was answered as successful, and restarts fail until the
> store call succeeds.
> h3. Reproduction
> [^TestBugMC5NonBatchRevokeThenGetSecret.java]
> ({{hadoop-ozone/ozone-manager}}): real {{preExecute}},
> {{validateAndUpdateCache}} and a running double buffer on one OM, with an in
> memory store without batch support, not Vault. The only timing aid is the
> existing {{OzoneManagerDoubleBuffer.pause}} / {{unpause}} hook, so the run
> shows that the window exists, not how often real clients hit it. After a
> revoke and a getsecret in one flush the store is empty: the assertion on the
> store fails with {{Expecting actual not to be null}}. It passes with the
> patch.
> The flush time termination is argued from the code above and was not
> reproduced in an attachment.
> h3. Patch
> [^MC-5-update-non-batch-s3-secret-store-at-apply-time.patch], against
> df04bc0e309f625a7dcdcd60790a42f34e2743d9. It also applies to master at
> 2cb1b6dd4e9.
> For a store without batch support, {{S3RevokeSecretRequest}},
> {{OMSetSecretRequest}}, {{OMTenantAssignUserAccessIdRequest}} and
> {{OMTenantRevokeUserAccessIdRequest}} now update the store in
> {{validateAndUpdateCache}}, as {{S3GetSecretRequest}} already does, and their
> responses no longer touch such a store. Revoke and set do this under the
> existing per access ID lock, tenant revoke through
> {{S3SecretLockedManager.revokeSecret}}. Apply is single threaded and in log
> order, so on one OM every store update follows the log order and a revoke can
> no longer delete a secret written by a later entry. With a shared store, a
> lagging OM still removes such a secret until it applies the later entry; that
> belongs to the design issue filed alongside. All four requests move together;
> moving only the revoke would let a flush time set write a secret after a
> later revoke deleted it. The default RocksDB store supports batches and is
> unchanged.
> Because the store now holds the secret as soon as a tenant assign is applied,
> the assign treats a store entry holding its own secret as already applied.
> The secret is generated in {{preExecute}} and carried in the log entry, so it
> identifies the entry. Without this, a replay after a restart, or a follower
> applying the entry after the leader, would reject the assign with
> TENANT_USER_ACCESS_ID_ALREADY_EXISTS and leave the access ID out of that OM's
> tenant tables. This also fixes a case that exists today: a follower that
> applies a tenant assign after the leader has flushed it finds the secret in
> the shared store and rejects the assign, and a later tenant revoke of that
> access ID fails on that follower with a NullPointerException
> ({{omDBAccessIdInfo == null}}), which terminates it. The assign still rejects
> an entry with a different secret, so the case where the access ID was revoked
> and assigned again before a lagging OM applies the first assign remains; it
> is part of the design issue. For a single OM replaying after a restart that
> case was limited to a crash within one flush before this patch, and with it
> any restart between the apply and the flush of the first assign is enough.
> With the patch, a store failure in these requests happens at apply time, as
> for getsecret: the request fails instead of being answered as successful, and
> nothing calls the store from the flush thread. The OM still terminates on
> that apply time failure (INTERNAL_ERROR); that is part of the design issue
> filed alongside.
> This is a contained fix in line with HDDS-10469, and it adds apply time store
> calls for these four requests. A leader side design, as discussed in the
> design issue filed alongside, would replace them.
> Tests added to the existing {{TestS3GetSecretRequest}}, all with a store
> without batch support:
> * {{testRevokeThenGetSecretBeforeFlushWithNonBatchStore}}: S3 revoke then
> getsecret in one flush; the store keeps the issued secret.
> * {{testRevokeFailsWhenNonBatchStoreFails}}: the store rejects the revoke;
> the request fails with INTERNAL_ERROR and the secret is kept.
> * {{testSetThenRevokeBeforeFlushWithNonBatchStore}}: set then revoke in one
> flush; the store ends empty. It passes without the patch too, and fails if
> only the revoke is moved.
> * {{testTenantAccessIdReplayAndRevokeWithNonBatchStore}}: a tenant assign
> writes the store when applied, applying it again after a restart without a
> flush succeeds, and a tenant revoke clears the store.
> Without the patch the first, second and fourth fail. Store failures of set
> and of the tenant requests and the follower cases are not covered by unit
> tests. The existing S3 secret, tenant, double buffer and state machine unit
> tests (193), {{TestMultiTenantVolume}} and {{TestOzoneTenantShell}} pass, but
> they use the default RocksDB store and do not run the changed branches.
> Checkstyle is clean.
> The S3 secret cache fix filed alongside also edits {{S3RevokeSecretRequest}},
> {{OMTenantRevokeUserAccessIdRequest}} and {{TestS3GetSecretRequest}}, so
> whichever lands second needs a rebase.
> 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-10469 (moved only the getsecret store write to apply time),
> HDDS-17200 ({{getSecretString}} ignores the deleted marker) and HDDS-9444
> (cache and table consistency for the default store). 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]
