Siyao Meng created HDDS-17197:
---------------------------------
Summary: 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
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]
