Fix connection error handling in database resolver (#2990)

vitalyisaev2 · web-flow · commit acfab9bea8ee · 2024-03-20T17:50:21.000+03:00
diff --git a/ydb/core/fq/libs/actors/database_resolver.cpp b/ydb/core/fq/libs/actors/database_resolver.cpp
@@ -155,12 +155,19 @@ class TResponseProcessor : public TActorBootstrapped<TResponseProcessor>
         NHttp::TEvHttpProxy::TEvHttpIncomingResponse::TPtr& ev,
         const TRequestMap::const_iterator& requestIter,
         TMaybe<TDatabaseDescription>& result)
-    {
+    { 
         TString errorMessage;
-        if (ev->Get()->Error.empty() && (ev->Get()->Response && ev->Get()->Response->Status == "200")) {
-            errorMessage = HandleSuccessfulResponse(ev, requestIter, result);
+
+        if (requestIter == Requests.end()) {
+            // Requests are guaranteed to be kept in within TResponseProcessor until the response arrives.
+            // If there is no appropriate request, it's a fatal error.
+            errorMessage = "Invariant violation: unknown request";
         } else {
-            errorMessage = HandleFailedResponse(ev, requestIter);
+            if (ev->Get()->Error.empty() && (ev->Get()->Response && ev->Get()->Response->Status == "200")) {
+                errorMessage = HandleSuccessfulResponse(ev, *requestIter, result);
+            } else {
+                errorMessage = HandleFailedResponse(ev, *requestIter);
+            }
         }
 
         if (errorMessage) {
@@ -185,17 +192,13 @@ class TResponseProcessor : public TActorBootstrapped<TResponseProcessor>
 
     TString HandleSuccessfulResponse(
         NHttp::TEvHttpProxy::TEvHttpIncomingResponse::TPtr& ev,
-        const TRequestMap::const_iterator& requestIter,
+        const TRequestMap::value_type& requestWithParams,
         TMaybe<TDatabaseDescription>& result
     ) {
-        if (requestIter == Requests.end()) {
-            return "unknown request";
-        }
-
         NJson::TJsonReaderConfig jsonConfig;
         NJson::TJsonValue databaseInfo;
 
-        const auto& params = requestIter->second;
+        const auto& params = requestWithParams.second;
         const bool parseJsonOk = NJson::ReadJsonTree(ev->Get()->Response->Body, &jsonConfig, &databaseInfo);
         TParsers::const_iterator parserIt;
         if (parseJsonOk && (parserIt = Parsers.find(params.DatabaseType)) != Parsers.end()) {
@@ -226,37 +229,37 @@ class TResponseProcessor : public TActorBootstrapped<TResponseProcessor>
 
     TString HandleFailedResponse(
         NHttp::TEvHttpProxy::TEvHttpIncomingResponse::TPtr& ev,
-        const TRequestMap::const_iterator& requestIter
+        const TRequestMap::value_type& requestWithParams
     ) const {
-        if (requestIter == Requests.end()) {
-            return "unknown request";
-        } 
+        auto sb = TStringBuilder() 
+            << "Error while trying to resolve managed " << ToString(requestWithParams.second.DatabaseType)
+            << " database with id " << requestWithParams.second.Id << " via HTTP request to"
+            << ": endpoint '" << requestWithParams.first->Host << "'"
+            << ", url '" << requestWithParams.first->URL << "'"
+            << ": ";
+
+        // Handle network error (when the response is empty)
+        if (!ev->Get()->Response) {
+            return sb << ev->Get()->Error;
+        }
 
+        // Handle unauthenticated error
         const auto& status = ev->Get()->Response->Status;
-
         if (status == "403") {
-            return TStringBuilder() << "You have no permission to resolve database id into database endpoint. " + DetailedPermissionsError(requestIter->second);
-        }
-
-        auto errorMessage = ev->Get()->Error;
-
-        const TString error = TStringBuilder()
-            << "Cannot resolve database id (status = " << status << "). "
-            << "Response body from " << ev->Get()->Request->URL << ": " << (ev->Get()->Response ? ev->Get()->Response->Body : "empty");
-        if (!errorMessage.empty()) {
-            errorMessage += '\n';
+            return sb << "you have no permission to resolve database id into database endpoint." + DetailedPermissionsError(requestWithParams.second);
         }
-        errorMessage += error;
 
-        return errorMessage;
+        // Unexpected error. Add response body for debug
+        return sb << Endl
+                  << "Status: " << status << Endl
+                  << "Response body: " << ev->Get()->Response->Body;
     }
 
 
     TString DetailedPermissionsError(const TResolveParams& params) const {
- 
         if (params.DatabaseType == EDatabaseType::ClickHouse || params.DatabaseType == EDatabaseType::PostgreSQL) {
                 auto mdbTypeStr = NYql::DatabaseTypeLowercase(params.DatabaseType);
-                return TStringBuilder() << "Please check that your service account has role "  << 
+                return TStringBuilder() << " Please check that your service account has role "  << 
                                        "`managed-" << mdbTypeStr << ".viewer`.";
         }
         return {};
diff --git a/ydb/core/fq/libs/actors/ut/database_resolver_ut.cpp b/ydb/core/fq/libs/actors/ut/database_resolver_ut.cpp
@@ -15,7 +15,22 @@ namespace {
 using namespace NKikimr;
 using namespace NFq;
 
-TString NoPermissionStr = "You have no permission to resolve database id into database endpoint. ";
+TString MakeErrorPrefix(
+    const TString& host, 
+    const TString& url,
+    const TString& databaseId,
+    const NYql::EDatabaseType& databaseType) {
+    TStringBuilder ss;
+    
+    return TStringBuilder() 
+        << "Error while trying to resolve managed " << ToString(databaseType)
+        << " database with id " << databaseId << " via HTTP request to"
+        << ": endpoint '" << host << "'"
+        << ", url '" << url << "'"
+        << ": ";
+}
+
+TString NoPermissionStr = "you have no permission to resolve database id into database endpoint.";
 
 struct TTestBootstrap : public TTestActorRuntime {
     NConfig::TCheckpointCoordinatorConfig Settings;
@@ -114,7 +129,9 @@ Y_UNIT_TEST_SUITE(TDatabaseResolverTests) {
         const TString& status,
         const TString& responseBody,
         const NYql::TDatabaseResolverResponse::TDatabaseDescription& description,
-        const NYql::TIssues& issues)
+        const NYql::TIssues& issues,
+        const TString& error = ""
+        )
     {
         TTestBootstrap bootstrap;
 
@@ -132,7 +149,7 @@ Y_UNIT_TEST_SUITE(TDatabaseResolverTests) {
                 NYql::IDatabaseAsyncResolver::TDatabaseAuthMap(
                     {std::make_pair(requestIdAndDatabaseType, databaseAuth)}),
                 TString("https://ydbc.ydb.cloud.yandex.net:8789/ydbc/cloud-prod"),
-                TString("mdbGateway"),
+                TString("https://mdb.api.cloud.yandex.net:443"),
                 TString("traceId"),
                 NFq::MakeMdbEndpointGeneratorGeneric(true))));
 
@@ -145,14 +162,17 @@ Y_UNIT_TEST_SUITE(TDatabaseResolverTests) {
 
         bootstrap.WaitForBootstrap();
 
-        auto response = std::make_unique<NHttp::THttpIncomingResponse>(nullptr);
-        response->Status = status;
-        response->Body = responseBody;
+        std::unique_ptr<NHttp::THttpIncomingResponse> httpIncomingResponse;
+        if (!error) {
+            httpIncomingResponse = std::make_unique<NHttp::THttpIncomingResponse>(nullptr);
+            httpIncomingResponse->Status = status;
+            httpIncomingResponse->Body = responseBody;
+        }
 
         bootstrap.Send(new IEventHandle(
             processorActorId,
             bootstrap.HttpProxy,
-            new NHttp::TEvHttpProxy::TEvHttpIncomingResponse(httpOutgoingRequest->Request, response.release(), "")));
+            new NHttp::TEvHttpProxy::TEvHttpIncomingResponse(httpOutgoingRequest->Request, httpIncomingResponse.release(), error)));
 
         NYql::TDatabaseResolverResponse::TDatabaseDescriptionMap result;
         if (status == "200") {
@@ -184,6 +204,36 @@ Y_UNIT_TEST_SUITE(TDatabaseResolverTests) {
             );
     }
 
+    Y_UNIT_TEST(Ydb_Serverless_Timeout) {
+        NYql::TIssues issues{
+            NYql::TIssue(
+                TStringBuilder{} << MakeErrorPrefix(
+                    "ydbc.ydb.cloud.yandex.net:8789",
+                    "/ydbc/cloud-prod/database?databaseId=etn021us5r9rhld1vgbh",
+                    "etn021us5r9rhld1vgbh",
+                    NYql::EDatabaseType::Ydb
+                ) << "Connection timeout"
+            )
+        };
+
+        Test(
+            NYql::EDatabaseType::Ydb,
+            NYql::NConnector::NApi::EProtocol::PROTOCOL_UNSPECIFIED,
+            "https://ydbc.ydb.cloud.yandex.net:8789/ydbc/cloud-prod/database?databaseId=etn021us5r9rhld1vgbh",
+            "",
+            "",           
+            NYql::TDatabaseResolverResponse::TDatabaseDescription{
+                TString{"ydb.serverless.yandexcloud.net:2135"},
+                TString{"ydb.serverless.yandexcloud.net"},
+                2135,
+                TString("/ru-central1/b1g7jdjqd07qg43c4fmp/etn021us5r9rhld1vgbh"),
+                true
+                },
+                issues,
+                "Connection timeout"
+            );
+    }
+
     Y_UNIT_TEST(DataStreams_Serverless) {
         Test(
             NYql::EDatabaseType::DataStreams,
@@ -298,7 +348,12 @@ Y_UNIT_TEST_SUITE(TDatabaseResolverTests) {
     Y_UNIT_TEST(ClickHouse_PermissionDenied) {
         NYql::TIssues issues{
             NYql::TIssue(
-                TStringBuilder{} << NoPermissionStr << "Please check that your service account has role `managed-clickhouse.viewer`."
+                TStringBuilder{} << MakeErrorPrefix(
+                    "mdb.api.cloud.yandex.net:443",
+                    "/managed-clickhouse/v1/clusters/etn021us5r9rhld1vgbh/hosts",
+                    "etn021us5r9rhld1vgbh",
+                    NYql::EDatabaseType::ClickHouse
+                ) << NoPermissionStr << " Please check that your service account has role `managed-clickhouse.viewer`."
             )
         };
 
@@ -366,7 +421,12 @@ Y_UNIT_TEST_SUITE(TDatabaseResolverTests) {
     Y_UNIT_TEST(PostgreSQL_PermissionDenied) {
         NYql::TIssues issues{
             NYql::TIssue(
-                TStringBuilder{} << NoPermissionStr << "Please check that your service account has role `managed-postgresql.viewer`."
+                TStringBuilder{} << MakeErrorPrefix(
+                    "mdb.api.cloud.yandex.net:443",
+                    "/managed-postgresql/v1/clusters/etn021us5r9rhld1vgbh/hosts",
+                    "etn021us5r9rhld1vgbh",
+                    NYql::EDatabaseType::PostgreSQL
+                ) << NoPermissionStr << " Please check that your service account has role `managed-postgresql.viewer`."
             )
         };
 
@@ -396,7 +456,12 @@ Y_UNIT_TEST_SUITE(TDatabaseResolverTests) {
     Y_UNIT_TEST(DataStreams_PermissionDenied) {
         NYql::TIssues issues{
             NYql::TIssue(
-                NoPermissionStr
+                TStringBuilder{} << MakeErrorPrefix(
+                    "ydbc.ydb.cloud.yandex.net:8789",
+                    "/ydbc/cloud-prod/database?databaseId=etn021us5r9rhld1vgbh",
+                    "etn021us5r9rhld1vgbh",
+                    NYql::EDatabaseType::DataStreams
+                ) << NoPermissionStr 
             )
         };
         Test(
@@ -434,7 +499,7 @@ Y_UNIT_TEST_SUITE(TDatabaseResolverTests) {
                     std::make_pair(requestIdAndDatabaseType1, databaseAuth),
                     std::make_pair(requestIdAndDatabaseType2, databaseAuth)}),
                 TString("https://ydbc.ydb.cloud.yandex.net:8789/ydbc/cloud-prod"),
-                TString("mdbGateway"),
+                TString("https://mdb.api.cloud.yandex.net:443"),
                 TString("traceId"),
                 NFq::MakeMdbEndpointGeneratorGeneric(true))));
 
@@ -481,7 +546,11 @@ Y_UNIT_TEST_SUITE(TDatabaseResolverTests) {
 
         NYql::TIssues issues{
             NYql::TIssue(
-                TStringBuilder{} << "Cannot resolve database id (status = 404). Response body from /ydbc/cloud-prod/database?databaseId=etn021us5r9rhld1vgb1: {\"message\":\"Database not found\"}"
+                TStringBuilder() << MakeErrorPrefix(
+                    "ydbc.ydb.cloud.yandex.net:8789", 
+                    "/ydbc/cloud-prod/database?databaseId=etn021us5r9rhld1vgb1", 
+                    "etn021us5r9rhld1vgb1", 
+                    NYql::EDatabaseType::DataStreams)<< "\nStatus: 404\nResponse body: {\"message\":\"Database not found\"}"
             )
         };
 
diff --git a/ydb/library/yql/providers/generic/provider/yql_generic_provider.cpp b/ydb/library/yql/providers/generic/provider/yql_generic_provider.cpp
@@ -21,6 +21,7 @@ namespace NYql {
             Y_UNUSED(randomProvider);
             Y_UNUSED(progressWriter);
             Y_UNUSED(operationOptions);
+            Y_ENSURE(credentialsFactory);
 
             auto state = MakeIntrusive<TGenericState>(
                 typeCtx.Get(),
diff --git a/ydb/library/yql/tools/dqrun/dqrun.cpp b/ydb/library/yql/tools/dqrun/dqrun.cpp
@@ -276,7 +276,8 @@ struct TActorIds {
 std::tuple<std::unique_ptr<TActorSystemManager>, TActorIds> RunActorSystem(
     const TGatewaysConfig& gatewaysConfig,
     IMetricsRegistryPtr& metricsRegistry,
-    NYql::NLog::ELevel loggingLevel
+    NYql::NLog::ELevel loggingLevel,
+    ISecuredServiceAccountCredentialsFactory::TPtr& credentialsFactory
 ) {
     auto actorSystemManager = std::make_unique<TActorSystemManager>(metricsRegistry, YqlToActorsLogLevel(loggingLevel));
     TActorIds actorIds;
@@ -297,7 +298,7 @@ std::tuple<std::unique_ptr<TActorSystemManager>, TActorIds> RunActorSystem(
         auto httpProxy = NHttp::CreateHttpProxy();
         actorIds.HttpProxy = actorSystemManager->GetActorSystem()->Register(httpProxy);
 
-        auto databaseResolver = NFq::CreateDatabaseResolver(actorIds.HttpProxy, nullptr);
+        auto databaseResolver = NFq::CreateDatabaseResolver(actorIds.HttpProxy, credentialsFactory);
         actorIds.DatabaseResolver = actorSystemManager->GetActorSystem()->Register(databaseResolver);
     }
 
@@ -760,12 +761,21 @@ int RunMain(int argc, const char* argv[])
         dataProvidersInit.push_back(GetYtNativeDataProviderInitializer(ytNativeGateway));
     }
 
+    ISecuredServiceAccountCredentialsFactory::TPtr credentialsFactory;
+
+    if (tokenAccessorEndpoint) {
+        TVector<TString> ss = StringSplitter(tokenAccessorEndpoint).SplitByString("://");
+        YQL_ENSURE(ss.size() == 2, "Invalid tokenAccessorEndpoint: " << tokenAccessorEndpoint); 
+
+        credentialsFactory = NYql::CreateSecuredServiceAccountCredentialsOverTokenAccessorFactory(ss[1], ss[0] == "grpcs", "");
+    }
+
     auto dqCompFactory = NMiniKQL::GetCompositeWithBuiltinFactory(factories);
 
     // Actor system starts here and will be automatically destroyed when goes out of the scope.
     std::unique_ptr<TActorSystemManager> actorSystemManager;
     TActorIds actorIds;
-    std::tie(actorSystemManager, actorIds) = RunActorSystem(gatewaysConfig, metricsRegistry, loggingLevel);
+    std::tie(actorSystemManager, actorIds) = RunActorSystem(gatewaysConfig, metricsRegistry, loggingLevel, credentialsFactory);
 
     IHTTPGateway::TPtr httpGateway;
     if (gatewaysConfig.HasClickHouse()) {
@@ -789,16 +799,6 @@ int RunMain(int argc, const char* argv[])
         );
     }
 
-    ISecuredServiceAccountCredentialsFactory::TPtr credentialsFactory;
-
-    if (tokenAccessorEndpoint) {
-        TVector<TString> ss = StringSplitter(tokenAccessorEndpoint).SplitByString("://");
-        YQL_ENSURE(ss.size() == 2, "Invalid tokenAccessorEndpoint: " << tokenAccessorEndpoint); 
-
-        credentialsFactory = NYql::CreateSecuredServiceAccountCredentialsOverTokenAccessorFactory(ss[1], ss[0] == "grpcs", "");
-    }
-
-
     NConnector::IClient::TPtr genericClient;
     if (gatewaysConfig.HasGeneric()) {
         for (auto& cluster : *gatewaysConfig.MutableGeneric()->MutableClusterMapping()) {
