[
https://issues.apache.org/jira/browse/HDDS-17197?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17197:
------------------------------
Attachment: CR-7-check-before-tenant-cache-entries.patch
> Tenant assign and create add table cache entries before a check that can
> fail, so a rejected assign can leave a dangling accessId
> ---------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17197
> URL: https://issues.apache.org/jira/browse/HDDS-17197
> Project: Apache Ozone
> Issue Type: Bug
> Reporter: Siyao Meng
> Priority: Major
> Attachments: CR-7-check-before-tenant-cache-entries.patch
>
>
> h3. Mechanism
> {{OMTenantAssignUserAccessIdRequest.validateAndUpdateCache}} adds the new
> accessId to the {{tenantAccessIdTable}} cache and the updated user row to the
> {{principalToAccessIdsTable}} cache, and only then checks that the accessId
> is absent from S3SecretTable. When that check fails with
> {{TENANT_USER_ACCESS_ID_ALREADY_EXISTS}}, the response writes nothing to the
> DB, but the two cache entries stay. They are visible to every later request
> on that OM until the double buffer flushes the failed transaction and the
> cache drops entries of that epoch.
> {{OMTenantCreateRequest.validateAndUpdateCache}} has the same order: it adds
> the volume and the owner's volume list to the volume and user table caches,
> and only then checks {{tenantStateTable}} for the tenant. A
> {{TENANT_ALREADY_EXISTS}} at that point leaves both cache entries until the
> flush.
> h3. Impact
> * While the failed assign's entries are visible, the next write of the same
> {{principalToAccessIdsTable}} row starts from the cached row, which already
> lists the failed accessId, and persists it. The DB then has a principal row
> that lists an accessId with no {{tenantAccessIdTable}} row.
> * No request removes that dangling reference: no tenant request drops an
> accessId without a {{tenantAccessIdTable}} row from a principal row. It stays
> across restart, and there is no repair command for the tenant tables.
> * Every later assign of the same user to any tenant reads that row and fails
> with {{INVALID_ACCESS_ID}} ("accessIdInfo is null").
> * Whether the failed entries are still visible depends on when each OM
> flushes, which is local to each OM. OMs that apply the same log can therefore
> persist different {{principalToAccessIdsTable}} entries for that user. Shown
> with two metadata managers applying the same transactions in one JVM, not on
> a Ratis HA cluster.
> * {{OMMultiTenantManagerImpl.isTenantAdmin}} returns {{false}} at the
> dangling accessId, so a tenant admin check of that user can fail permanently.
> That is HDDS-17196; with its patch this effect goes away.
> * For create, a volume and an owner volume list entry that the DB never gets
> are visible until the flush, so a request that reads them in between can
> persist state against a volume that does not exist. Argued from the code, not
> run.
> h3. Patch
> [^CR-7-check-before-tenant-cache-entries.patch], against
> a6b7bdb937109ba2893688b41a89470bc95c88cf. It also applies to master at
> 1753288f333.
> The patch only moves existing checks. In
> {{OMTenantAssignUserAccessIdRequest}} the S3SecretTable check, together with
> its {{updateCache}} of the new secret, moves ahead of the two tenant table
> cache adds, which is the order the class Javadoc already describes. In
> {{OMTenantCreateRequest}} the {{tenantStateTable}} check moves to right after
> the volume existence check, before the user lock and the volume creation, so
> {{VOLUME_ALREADY_EXISTS}} still takes precedence. A failed assign or create
> therefore adds no table cache entries, and no later request can read and
> persist state that the failed one never wrote. Successful requests write the
> same cache entries and DB batch as before, and failed ones return the same
> result codes; the audit entry of a {{TENANT_ALREADY_EXISTS}} from apply no
> longer carries the volume fields. Dangling references that are already in the
> DB are not cleaned up.
> The issue has no standalone reproduction. The two new tests fail without the
> change and pass with it:
> *
> {{TestOMTenantAssignUserAccessIdRequest.testFailedAssignLeavesNoTableCacheEntries}}
> (new class): an assign that fails the S3SecretTable check must leave no
> {{tenantAccessIdTable}} or {{principalToAccessIdsTable}} entry, and the next
> assign of the same user must store only its own accessId. Without the change
> it fails with {{expected: null but was: OmDBAccessIdInfo}}.
> * {{TestOMTenantCreateRequest.testValidateAndUpdateCacheWhenTenantExists}}: a
> create that fails the tenant check in apply must leave no volume or user
> table entry. Without the change it fails with {{expected: null but was:
> OmVolumeArgs}}.
> With the patch the tenant request and tenant manager unit suites pass,
> {{TestMultiTenantVolume}} and {{TestOzoneTenantShell}} pass, and checkstyle
> on {{hadoop-ozone/ozone-manager}} is clean.
> Found by code review of the OM multi-tenancy and Ranger sync paths, as part
> of the TLA+ verification effort under HDDS-15926, on commit
> a6b7bdb937109ba2893688b41a89470bc95c88cf. Checked against HDDS issues and
> apache/ozone pull requests for duplicates before filing. Related but not
> duplicates: HDDS-7001 (lock release in the same create method), HDDS-7368
> (added the volume existence check to create preExecute). 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]
