Change tenant ID feature flag to use isEnabled

XMLWordPrintableJSON

    • Type: Task
    • Resolution: Won't Do
    • Priority: Major - P3
    • None
    • Affects Version/s: None
    • Component/s: None
    • Service Arch
    • 3
    • None
    • None
    • None
    • None
    • None
    • None
    • None

      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

            Assignee:
            [DO NOT USE] Backlog - Service Architecture
            Reporter:
            Huayu Ouyang
            Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

              Created:
              Updated:
              Resolved: