Skip to content

Unsupported database type (DataStreams) by 403 http error / Merge to stable #944

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 42 additions & 19 deletions ydb/core/fq/libs/actors/database_resolver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,31 @@ class TResponseProcessor : public TActorBootstrapped<TResponseProcessor>

LOG_T("ResponseProcessor::Handle(HttpIncomingResponse): got MDB API response: code=" << ev->Get()->Response->Status);

try {
HandleResponse(ev, requestIter, errorMessage, result);
} catch (...) {
const TString msg = TStringBuilder() << "error while response processing, params "
<< ((requestIter != Requests.end()) ? requestIter->second.ToDebugString() : TString{"unknown"})
<< ", details: " << CurrentExceptionMessage();
LOG_E("ResponseProccessor::Handle(TEvHttpIncomingResponse): " << msg);
}

LOG_T("ResponseProcessor::Handle(HttpIncomingResponse): progress: "
<< DatabaseId2Description.size() << " of " << Requests.size() << " requests are done");

if (HandledIds == Requests.size()) {
SendResolvedEndpointsAndDie(errorMessage);
}
}

private:

void HandleResponse(
NHttp::TEvHttpProxy::TEvHttpIncomingResponse::TPtr& ev,
const TRequestMap::const_iterator& requestIter,
TString& errorMessage,
TMaybe<TDatabaseDescription>& result)
{
if (ev->Get()->Error.empty() && (ev->Get()->Response && ev->Get()->Response->Status == "200")) {
errorMessage = HandleSuccessfulResponse(ev, requestIter, result);
} else {
Expand All @@ -152,34 +177,26 @@ class TResponseProcessor : public TActorBootstrapped<TResponseProcessor>
auto key = std::make_tuple(params.Id, params.DatabaseType, params.DatabaseAuth);
if (errorMessage) {
LOG_T("ResponseProcessor::Handle(HttpIncomingResponse): put value in cache"
<< "; params: " << params.ToDebugString()
<< ", error: " << errorMessage);
<< "; params: " << params.ToDebugString()
<< ", error: " << errorMessage);
Cache.Put(key, errorMessage);
} else {
LOG_T("ResponseProcessor::Handle(HttpIncomingResponse): put value in cache"
<< "; params: " << params.ToDebugString()
<< ", result: " << result->ToDebugString());
<< "; params: " << params.ToDebugString()
<< ", result: " << result->ToDebugString());
Cache.Put(key, result);
}
}

LOG_D("ResponseProcessor::Handle(HttpIncomingResponse): progress: "
<< DatabaseId2Description.size() << " of " << Requests.size() << " requests are done");

if (HandledIds == Requests.size()) {
SendResolvedEndpointsAndDie(errorMessage);
}
}

private:
TString HandleSuccessfulResponse(
NHttp::TEvHttpProxy::TEvHttpIncomingResponse::TPtr& ev,
const TRequestMap::const_iterator& requestIter,
TMaybe<TDatabaseDescription>& result
) {
if (requestIter == Requests.end()) {
return "unknown request";
}
}

NJson::TJsonReaderConfig jsonConfig;
NJson::TJsonValue databaseInfo;
Expand Down Expand Up @@ -224,12 +241,7 @@ class TResponseProcessor : public TActorBootstrapped<TResponseProcessor>
const auto& status = ev->Get()->Response->Status;

if (status == "403") {
const auto second = requestIter->second;
auto mdbTypeStr = NYql::DatabaseTypeLowercase(second.DatabaseType);

return TStringBuilder() << "You have no permission to resolve database id into database endpoint. " <<
"Please check that your service account has role " <<
"`managed-" << mdbTypeStr << ".viewer`.";
return TStringBuilder() << "You have no permission to resolve database id into database endpoint. " << DetailedPermissionsError(requestIter->second);
}

auto errorMessage = ev->Get()->Error;
Expand All @@ -245,6 +257,17 @@ class TResponseProcessor : public TActorBootstrapped<TResponseProcessor>
return errorMessage;
}


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 " <<
"`managed-" << mdbTypeStr << ".viewer`.";
}
return {};
}

const TActorId Sender;
TCache& Cache;
const TRequestMap Requests;
Expand Down
27 changes: 25 additions & 2 deletions ydb/core/fq/libs/actors/ut/database_resolver_ut.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ namespace {
using namespace NKikimr;
using namespace NFq;

TString NoPermissionStr = "You have no permission to resolve database id into database endpoint. ";

struct TTestBootstrap : public TTestActorRuntime {
NConfig::TCheckpointCoordinatorConfig Settings;
NActors::TActorId DatabaseResolver;
Expand Down Expand Up @@ -295,7 +297,7 @@ Y_UNIT_TEST_SUITE(TDatabaseResolverTests) {
Y_UNIT_TEST(ClickHouse_PermissionDenied) {
NYql::TIssues issues{
NYql::TIssue(
"You have no permission to resolve database id into database endpoint. Please check that your service account has role `managed-clickhouse.viewer`."
TStringBuilder{} << NoPermissionStr << "Please check that your service account has role `managed-clickhouse.viewer`."
)
};

Expand Down Expand Up @@ -363,7 +365,7 @@ Y_UNIT_TEST_SUITE(TDatabaseResolverTests) {
Y_UNIT_TEST(PostgreSQL_PermissionDenied) {
NYql::TIssues issues{
NYql::TIssue(
"You have no permission to resolve database id into database endpoint. Please check that your service account has role `managed-postgresql.viewer`."
TStringBuilder{} << NoPermissionStr << "Please check that your service account has role `managed-postgresql.viewer`."
)
};

Expand All @@ -389,6 +391,27 @@ Y_UNIT_TEST_SUITE(TDatabaseResolverTests) {
issues
);
}

Y_UNIT_TEST(DataStreams_PermissionDenied) {
NYql::TIssues issues{
NYql::TIssue(
NoPermissionStr
)
};
Test(
NYql::EDatabaseType::DataStreams,
NYql::NConnector::NApi::EProtocol::PROTOCOL_UNSPECIFIED,
"https://ydbc.ydb.cloud.yandex.net:8789/ydbc/cloud-prod/database?databaseId=etn021us5r9rhld1vgbh",
"403",
R"(
{
"message": "Permission denied"
})",
NYql::TDatabaseResolverResponse::TDatabaseDescription{
},
issues
);
}
}

} // namespace NFq
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,7 @@ inline NConnector::NApi::EDataSourceKind DatabaseTypeToDataSourceKind(EDatabaseT
inline TString DatabaseTypeLowercase(EDatabaseType databaseType) {
auto dump = ToString(databaseType);
dump.to_lower();

switch (databaseType) {
case EDatabaseType::ClickHouse:
case EDatabaseType::PostgreSQL:
return dump;
default:
ythrow yexception() << "Unsupported database type: " << ToString(databaseType);
}
return dump;
}

// TODO: remove this function after /kikimr/yq/tests/control_plane_storage is moved to /ydb.
Expand All @@ -73,7 +66,7 @@ struct TDatabaseAuth {
NConnector::NApi::EProtocol Protocol = NConnector::NApi::EProtocol::PROTOCOL_UNSPECIFIED;

bool operator==(const TDatabaseAuth& other) const {
return std::tie(StructuredToken, AddBearerToToken, UseTls) == std::tie(other.StructuredToken, other.AddBearerToToken, other.UseTls);
return std::tie(StructuredToken, AddBearerToToken, UseTls, Protocol) == std::tie(other.StructuredToken, other.AddBearerToToken, other.UseTls, other.Protocol);
}

bool operator!=(const TDatabaseAuth& other) const {
Expand Down