[SERVER-84275] Change tenant ID feature flag to use isEnabled Created: 18/Dec/23  Updated: 19/Jan/24

Status: Open
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Task Priority: Major - P3
Reporter: Huayu Ouyang Assignee: Backlog - Service Architecture
Resolution: Unresolved Votes: 0
Labels: ntdi_releasability
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
depends on SERVER-82246 Change isEnabled to invariant when FC... Closed
Related
Assigned Teams:
Service Arch
Participants:
Story Points: 3

 Description   

Follow up to SERVER-82246. We changed some tenant ID feature flags to use isEnabledUseLastLTSFCVWhenUninitialized since they could be run during startup/when FCV is uninitialized. However, ideally we should figure out a way to just use isEnabled because ideally we don't want to turn off the tenant ID feature when it actually should be turned on.

Specifically, in database_name_util.cpp and namespace_string_util.cpp's serialize/deserializeForStorage functions, they might invariant if called during startup. We should avoid checking the feature flag if this is not a tenant specific collection/if there is no tenant id by doing something like this:

std::string DatabaseNameUtil::serializeForStorage(const DatabaseName& dbName,
                                                  const SerializationContext& context) {
    auto fcv = serverGlobalParams.featureCompatibility.acquireFCVSnapshot();
    if (!fcv.isVersionInitialized()) {
        invariant(!dbName.tenantId());
        return dbName.toString();
    }
    if (gFeatureFlagRequireTenantID.isEnabled(fcv)) {
        return dbName.toString();
    }
    return dbName.toStringWithTenantId();
}

@@ -195,10 +199,18 @@ DatabaseName DatabaseNameUtil::deserializeForAuthPrevalidated(boost::optional<Te
 DatabaseName DatabaseNameUtil::deserializeForStorage(boost::optional<TenantId> tenantId,
                                                      StringData db,
                                                      const SerializationContext& context) {
-    if (gFeatureFlagRequireTenantID.isEnabled(
-            serverGlobalParams.featureCompatibility.acquireFCVSnapshot())) {
-        if (!DatabaseName::kAdmin.dbNameEqual(db) && !DatabaseName::kLocal.dbNameEqual(db) &&
-            !DatabaseName::kConfig.dbNameEqual(db) && !DatabaseName::kExternal.dbNameEqual(db))
+    auto isTenantAgnosticDb = [](StringData db) {
+        return DatabaseName::kAdmin.dbNameEqual(db) || DatabaseName::kLocal.dbNameEqual(db) ||
+            DatabaseName::kConfig.dbNameEqual(db) || DatabaseName::kExternal.dbNameEqual(db);
+    };
+
+    auto fcv = serverGlobalParams.featureCompatibility.acquireFCVSnapshot();
+    if (!fcv.isVersionInitialized()) {
+        invariant(isTenantAgnosticDb(db) && !tenantId);
+        return DatabaseName(boost::none, db);
+    }
+    if (gFeatureFlagRequireTenantID.isEnabled(fcv)) {
+        if (!isTenantAgnosticDb(db))
             uassert(7005300, "TenantId must be set", tenantId != boost::none);

However, it also seems like there are some tenant specific namespaces that are being run through these functions during startup. In this patch with the above changes, the native_tenant_data_isolation_stop_restart.js test failed with an invariant, as it was a tenant specific collection, with a tenant ID. I believe this happened when we stopped and restarted the replica set, and load the catalog again with already existing views


Generated at Thu Feb 08 06:54:32 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.