Skip to content

Commit c8be0ed

Browse files
GrigoriyPAGrigoriyPA
committed
YQ-3653 added resource pools classifiers validations (ydb-platform#9037)
1 parent 0858a72 commit c8be0ed

10 files changed

+160
-35
lines changed

‎ydb/core/kqp/gateway/behaviour/resource_pool_classifier/manager.cpp

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,11 @@ NMetadata::NModifications::TOperationParsingResult TResourcePoolClassifierManage
5353
} catch (...) {
5454
throw yexception() << "Failed to parse property " << property << ": " << CurrentExceptionMessage();
5555
}
56-
} else if (!featuresExtractor.ExtractResetFeature(property)) {
56+
} else if (featuresExtractor.ExtractResetFeature(property)) {
57+
if (property == "resource_pool") {
58+
ythrow yexception() << "Cannot reset required property resource_pool";
59+
}
60+
} else {
5761
continue;
5862
}
5963

@@ -65,6 +69,19 @@ NMetadata::NModifications::TOperationParsingResult TResourcePoolClassifierManage
6569
}
6670
}
6771

72+
if (context.GetActivityType() == EActivityType::Create) {
73+
if (!configJson.GetMap().contains("resource_pool")) {
74+
ythrow yexception() << "Missing required property resource_pool";
75+
}
76+
77+
static const TString extraPathSymbolsAllowed = "!\"#$%&'()*+,-.:;<=>?@[\\]^_`{|}~";
78+
const auto& name = settings.GetObjectId();
79+
if (const auto brokenAt = PathPartBrokenAt(name, extraPathSymbolsAllowed); brokenAt != name.end()) {
80+
ythrow yexception() << "Symbol '" << *brokenAt << "'" << " is not allowed in the resource pool classifier name '" << name << "'";
81+
}
82+
}
83+
resourcePoolClassifierSettings.Validate();
84+
6885
NJsonWriter::TBuf writer;
6986
writer.WriteJsonValue(&configJson);
7087
result.SetColumn(TResourcePoolClassifierConfig::TDecoder::ConfigJson, NMetadata::NInternal::TYDBValue::Utf8(writer.Str()));

‎ydb/core/kqp/proxy_service/kqp_proxy_service_impl.h

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -422,17 +422,19 @@ class TResourcePoolsCache {
422422
struct TClassifierInfo {
423423
const TString Membername;
424424
const TString PoolId;
425+
const i64 Rank;
425426

426427
TClassifierInfo(const NResourcePool::TClassifierSettings& classifierSettings)
427428
: Membername(classifierSettings.Membername)
428429
, PoolId(classifierSettings.ResourcePool)
430+
, Rank(classifierSettings.Rank)
429431
{}
430432
};
431433

432434
struct TDatabaseInfo {
433435
std::unordered_map<TString, TResourcePoolClassifierConfig> ResourcePoolsClassifiers = {};
434436
std::map<i64, TClassifierInfo> RankToClassifierInfo = {};
435-
std::unordered_map<TString, TString> UserToResourcePool = {};
437+
std::unordered_map<TString, std::pair<TString, i64>> UserToResourcePool = {};
436438
bool Serverless = false;
437439
};
438440

@@ -462,16 +464,16 @@ class TResourcePoolsCache {
462464
}
463465

464466
TDatabaseInfo& databaseInfo = *GetOrCreateDatabaseInfo(database);
465-
if (const auto& poolId = GetPoolIdFromClassifiers(database, userToken->GetUserSID(), databaseInfo, userToken, actorContext)) {
466-
return poolId;
467-
}
467+
auto [resultPoolId, resultRank] = GetPoolIdFromClassifiers(database, userToken->GetUserSID(), databaseInfo, userToken, actorContext);
468468
for (const auto& userSID : userToken->GetGroupSIDs()) {
469-
if (const auto& poolId = GetPoolIdFromClassifiers(database, userSID, databaseInfo, userToken, actorContext)) {
470-
return poolId;
469+
const auto& [poolId, rank] = GetPoolIdFromClassifiers(database, userSID, databaseInfo, userToken, actorContext);
470+
if (poolId && (!resultPoolId || resultRank > rank)) {
471+
resultPoolId = poolId;
472+
resultRank = rank;
471473
}
472474
}
473475

474-
return NResourcePool::DEFAULT_POOL_ID;
476+
return resultPoolId ? resultPoolId : NResourcePool::DEFAULT_POOL_ID;
475477
}
476478

477479
std::optional<TPoolInfo> GetPoolInfo(const TString& database, const TString& poolId, TActorContext actorContext) const {
@@ -582,13 +584,14 @@ class TResourcePoolsCache {
582584
}
583585
}
584586

585-
TString GetPoolIdFromClassifiers(const TString& database, const TString& userSID, TDatabaseInfo& databaseInfo, const TIntrusiveConstPtr<NACLib::TUserToken>& userToken, TActorContext actorContext) const {
587+
std::pair<TString, i64> GetPoolIdFromClassifiers(const TString& database, const TString& userSID, TDatabaseInfo& databaseInfo, const TIntrusiveConstPtr<NACLib::TUserToken>& userToken, TActorContext actorContext) const {
586588
auto& usersMap = databaseInfo.UserToResourcePool;
587589
if (const auto it = usersMap.find(userSID); it != usersMap.end()) {
588590
return it->second;
589591
}
590592

591593
TString poolId = "";
594+
i64 rank = -1;
592595
for (const auto& [_, classifier] : databaseInfo.RankToClassifierInfo) {
593596
if (classifier.Membername != userSID) {
594597
continue;
@@ -605,11 +608,12 @@ class TResourcePoolsCache {
605608
}
606609

607610
poolId = classifier.PoolId;
611+
rank = classifier.Rank;
608612
break;
609613
}
610614

611-
usersMap[userSID] = poolId;
612-
return poolId;
615+
usersMap[userSID] = {poolId, rank};
616+
return {poolId, rank};
613617
}
614618

615619
TDatabaseInfo* GetOrCreateDatabaseInfo(const TString& database) {

‎ydb/core/kqp/ut/scheme/kqp_scheme_ut.cpp

Lines changed: 51 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6550,8 +6550,8 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
65506550
// ALTER RESOURCE POOL CLASSIFIER
65516551
checkDisabled(R"(
65526552
ALTER RESOURCE POOL CLASSIFIER MyResourcePoolClassifier
6553-
SET (RANK = 1, MEMBERNAME = "test@user"),
6554-
RESET (RESOURCE_POOL);
6553+
SET (RANK = 1, RESOURCE_POOL = "test"),
6554+
RESET (MEMBERNAME);
65556555
)");
65566556

65576557
// DROP RESOURCE POOL CLASSIFIER
@@ -6584,8 +6584,8 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
65846584

65856585
const auto& alterSql = R"(
65866586
ALTER RESOURCE POOL CLASSIFIER MyResourcePoolClassifier
6587-
SET (RANK = 1, MEMBERNAME = "test@user"),
6588-
RESET (RESOURCE_POOL);
6587+
SET (RANK = 1, RESOURCE_POOL = "test"),
6588+
RESET (MEMBERNAME);
65896589
)";
65906590

65916591
const auto& dropSql = "DROP RESOURCE POOL CLASSIFIER MyResourcePoolClassifier;";
@@ -6624,6 +6624,7 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
66246624

66256625
auto result = session.ExecuteSchemeQuery(R"(
66266626
CREATE RESOURCE POOL CLASSIFIER MyResourcePoolClassifier WITH (
6627+
RESOURCE_POOL="test",
66276628
ANOTHER_PROPERTY=20
66286629
);)").GetValueSync();
66296630
UNIT_ASSERT_VALUES_EQUAL(result.GetStatus(), EStatus::GENERIC_ERROR);
@@ -6639,10 +6640,40 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
66396640

66406641
result = session.ExecuteSchemeQuery(R"(
66416642
CREATE RESOURCE POOL CLASSIFIER MyResourcePoolClassifier WITH (
6643+
RESOURCE_POOL="test",
66426644
RANK="StringValue"
66436645
);)").GetValueSync();
66446646
UNIT_ASSERT_VALUES_EQUAL(result.GetStatus(), EStatus::GENERIC_ERROR);
66456647
UNIT_ASSERT_STRING_CONTAINS(result.GetIssues().ToString(), "Failed to parse property rank:");
6648+
6649+
result = session.ExecuteSchemeQuery(R"(
6650+
CREATE RESOURCE POOL CLASSIFIER MyResourcePoolClassifier WITH (
6651+
RANK="0"
6652+
);)").GetValueSync();
6653+
UNIT_ASSERT_VALUES_EQUAL(result.GetStatus(), EStatus::GENERIC_ERROR);
6654+
UNIT_ASSERT_STRING_CONTAINS(result.GetIssues().ToString(), "Missing required property resource_pool");
6655+
6656+
result = session.ExecuteSchemeQuery(R"(
6657+
ALTER RESOURCE POOL CLASSIFIER MyResourcePoolClassifier
6658+
RESET (RESOURCE_POOL);
6659+
)").GetValueSync();
6660+
UNIT_ASSERT_VALUES_EQUAL(result.GetStatus(), EStatus::GENERIC_ERROR);
6661+
UNIT_ASSERT_STRING_CONTAINS(result.GetIssues().ToString(), "Cannot reset required property resource_pool");
6662+
6663+
result = session.ExecuteSchemeQuery(R"(
6664+
CREATE RESOURCE POOL CLASSIFIER `MyResource/PoolClassifier` WITH (
6665+
RESOURCE_POOL="test"
6666+
);)").GetValueSync();
6667+
UNIT_ASSERT_VALUES_EQUAL(result.GetStatus(), EStatus::GENERIC_ERROR);
6668+
UNIT_ASSERT_STRING_CONTAINS(result.GetIssues().ToString(), "Symbol '/' is not allowed in the resource pool classifier name 'MyResource/PoolClassifier'");
6669+
6670+
result = session.ExecuteSchemeQuery(TStringBuilder() << R"(
6671+
CREATE RESOURCE POOL CLASSIFIER MyResourcePoolClassifier WITH (
6672+
RESOURCE_POOL="test",
6673+
MEMBERNAME=")" << BUILTIN_ACL_METADATA << R"("
6674+
);)").GetValueSync();
6675+
UNIT_ASSERT_VALUES_EQUAL(result.GetStatus(), EStatus::GENERIC_ERROR);
6676+
UNIT_ASSERT_STRING_CONTAINS(result.GetIssues().ToString(), TStringBuilder() << "Invalid resource pool classifier configuration, cannot create classifier for system user " << BUILTIN_ACL_METADATA);
66466677
}
66476678

66486679
Y_UNIT_TEST(ResourcePoolClassifiersRankValidation) {
@@ -6659,13 +6690,15 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
66596690
// Create with sample rank
66606691
auto result = session.ExecuteSchemeQuery(R"(
66616692
CREATE RESOURCE POOL CLASSIFIER ClassifierRank42 WITH (
6693+
RESOURCE_POOL="test_pool",
66626694
RANK=42
66636695
);)").GetValueSync();
66646696
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToOneLineString());
66656697

66666698
// Try to create with same rank
66676699
result = session.ExecuteSchemeQuery(R"(
66686700
CREATE RESOURCE POOL CLASSIFIER AnotherClassifierRank42 WITH (
6701+
RESOURCE_POOL="test_pool",
66696702
RANK=42
66706703
);)").GetValueSync();
66716704
UNIT_ASSERT_VALUES_EQUAL(result.GetStatus(), EStatus::GENERIC_ERROR);
@@ -6674,13 +6707,15 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
66746707
// Create with high rank
66756708
result = session.ExecuteSchemeQuery(R"(
66766709
CREATE RESOURCE POOL CLASSIFIER `ClassifierRank2^63` WITH (
6710+
RESOURCE_POOL="test_pool",
66776711
RANK=9223372036854775807
66786712
);)").GetValueSync();
66796713
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToOneLineString());
66806714

66816715
// Try to create with auto rank
66826716
result = session.ExecuteSchemeQuery(R"(
66836717
CREATE RESOURCE POOL CLASSIFIER ClassifierRankAuto WITH (
6718+
RESOURCE_POOL="test_pool",
66846719
MEMBERNAME="test@user"
66856720
);)").GetValueSync();
66866721
UNIT_ASSERT_VALUES_EQUAL(result.GetStatus(), EStatus::GENERIC_ERROR);
@@ -6738,11 +6773,12 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
67386773
// Auto rank
67396774
query = R"(
67406775
CREATE RESOURCE POOL CLASSIFIER AnotherResourcePoolClassifier WITH (
6776+
RESOURCE_POOL="test_pool",
67416777
MEMBERNAME="another@user"
67426778
);)";
67436779
result = session.ExecuteSchemeQuery(query).GetValueSync();
67446780
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
6745-
UNIT_ASSERT_VALUES_EQUAL(FetchResourcePoolClassifiers(kikimr), "{\"resource_pool_classifiers\":[{\"rank\":20,\"name\":\"MyResourcePoolClassifier\",\"config\":{\"membername\":\"test@user\",\"resource_pool\":\"test_pool\"},\"database\":\"\\/Root\"},{\"rank\":1020,\"name\":\"AnotherResourcePoolClassifier\",\"config\":{\"membername\":\"another@user\"},\"database\":\"\\/Root\"}]}");
6781+
UNIT_ASSERT_VALUES_EQUAL(FetchResourcePoolClassifiers(kikimr), "{\"resource_pool_classifiers\":[{\"rank\":20,\"name\":\"MyResourcePoolClassifier\",\"config\":{\"membername\":\"test@user\",\"resource_pool\":\"test_pool\"},\"database\":\"\\/Root\"},{\"rank\":1020,\"name\":\"AnotherResourcePoolClassifier\",\"config\":{\"membername\":\"another@user\",\"resource_pool\":\"test_pool\"},\"database\":\"\\/Root\"}]}");
67466782
}
67476783

67486784
Y_UNIT_TEST(DoubleCreateResourcePoolClassifier) {
@@ -6759,6 +6795,7 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
67596795
{
67606796
auto query = R"(
67616797
CREATE RESOURCE POOL CLASSIFIER MyResourcePoolClassifier WITH (
6798+
RESOURCE_POOL="test_pool",
67626799
RANK=20
67636800
);)";
67646801
auto result = session.ExecuteSchemeQuery(query).GetValueSync();
@@ -6768,6 +6805,7 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
67686805
{
67696806
auto query = R"(
67706807
CREATE RESOURCE POOL CLASSIFIER MyResourcePoolClassifier WITH (
6808+
RESOURCE_POOL="test_pool",
67716809
RANK=1
67726810
);)";
67736811
auto result = session.ExecuteSchemeQuery(query).GetValueSync();
@@ -6814,22 +6852,23 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
68146852
{
68156853
auto query = R"(
68166854
CREATE RESOURCE POOL CLASSIFIER AnotherResourcePoolClassifier WITH (
6855+
RESOURCE_POOL="test_pool",
68176856
RANK=42
68186857
);)";
68196858
auto result = session.ExecuteSchemeQuery(query).GetValueSync();
68206859
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
6821-
UNIT_ASSERT_VALUES_EQUAL(FetchResourcePoolClassifiers(kikimr), "{\"resource_pool_classifiers\":[{\"rank\":20,\"name\":\"MyResourcePoolClassifier\",\"config\":{\"membername\":\"test@user\",\"resource_pool\":\"test_pool\"},\"database\":\"\\/Root\"},{\"rank\":42,\"name\":\"AnotherResourcePoolClassifier\",\"config\":{},\"database\":\"\\/Root\"}]}");
6860+
UNIT_ASSERT_VALUES_EQUAL(FetchResourcePoolClassifiers(kikimr), "{\"resource_pool_classifiers\":[{\"rank\":20,\"name\":\"MyResourcePoolClassifier\",\"config\":{\"membername\":\"test@user\",\"resource_pool\":\"test_pool\"},\"database\":\"\\/Root\"},{\"rank\":42,\"name\":\"AnotherResourcePoolClassifier\",\"config\":{\"resource_pool\":\"test_pool\"},\"database\":\"\\/Root\"}]}");
68226861
}
68236862

68246863
// Test reset
68256864
{
68266865
auto query = R"(
68276866
ALTER RESOURCE POOL CLASSIFIER MyResourcePoolClassifier
6828-
RESET (RANK, RESOURCE_POOL);
6867+
RESET (RANK, MEMBERNAME);
68296868
)";
68306869
auto result = session.ExecuteSchemeQuery(query).GetValueSync();
68316870
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
6832-
UNIT_ASSERT_VALUES_EQUAL(FetchResourcePoolClassifiers(kikimr), "{\"resource_pool_classifiers\":[{\"rank\":1042,\"name\":\"MyResourcePoolClassifier\",\"config\":{\"membername\":\"test@user\",\"resource_pool\":\"default\"},\"database\":\"\\/Root\"},{\"rank\":42,\"name\":\"AnotherResourcePoolClassifier\",\"config\":{},\"database\":\"\\/Root\"}]}");
6871+
UNIT_ASSERT_VALUES_EQUAL(FetchResourcePoolClassifiers(kikimr), "{\"resource_pool_classifiers\":[{\"rank\":1042,\"name\":\"MyResourcePoolClassifier\",\"config\":{\"membername\":\"\",\"resource_pool\":\"test_pool\"},\"database\":\"\\/Root\"},{\"rank\":42,\"name\":\"AnotherResourcePoolClassifier\",\"config\":{\"resource_pool\":\"test_pool\"},\"database\":\"\\/Root\"}]}");
68336872
}
68346873
}
68356874

@@ -6846,8 +6885,8 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
68466885

68476886
auto query = R"(
68486887
ALTER RESOURCE POOL CLASSIFIER MyResourcePoolClassifier
6849-
SET (MEMBERNAME = "test@user", RANK = 100),
6850-
RESET (RESOURCE_POOL);
6888+
SET (RESOURCE_POOL = "test", RANK = 100),
6889+
RESET (MEMBERNAME);
68516890
)";
68526891
auto result = session.ExecuteSchemeQuery(query).GetValueSync();
68536892
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::GENERIC_ERROR, result.GetIssues().ToString());
@@ -6868,11 +6907,12 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
68686907
{
68696908
auto query = R"(
68706909
CREATE RESOURCE POOL CLASSIFIER MyResourcePoolClassifier WITH (
6910+
RESOURCE_POOL="test_pool",
68716911
RANK=20
68726912
);)";
68736913
auto result = session.ExecuteSchemeQuery(query).GetValueSync();
68746914
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
6875-
UNIT_ASSERT_VALUES_EQUAL(FetchResourcePoolClassifiers(kikimr), "{\"resource_pool_classifiers\":[{\"rank\":20,\"name\":\"MyResourcePoolClassifier\",\"config\":{},\"database\":\"\\/Root\"}]}");
6915+
UNIT_ASSERT_VALUES_EQUAL(FetchResourcePoolClassifiers(kikimr), "{\"resource_pool_classifiers\":[{\"rank\":20,\"name\":\"MyResourcePoolClassifier\",\"config\":{\"resource_pool\":\"test_pool\"},\"database\":\"\\/Root\"}]}");
68766916
}
68776917

68786918
{

‎ydb/core/kqp/workload_service/ut/common/kqp_workload_service_ut_common.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -518,7 +518,7 @@ class TWorkloadServiceYdbSetup : public IYdbSetup {
518518
UNIT_ASSERT_C(settings.PoolId_, "Query pool id is not specified");
519519

520520
auto event = std::make_unique<TEvKqp::TEvQueryRequest>();
521-
event->Record.SetUserToken(NACLib::TUserToken("", settings.UserSID_, {}).SerializeAsString());
521+
event->Record.SetUserToken(NACLib::TUserToken("", settings.UserSID_, settings.GroupSIDs_).SerializeAsString());
522522

523523
auto request = event->Record.MutableRequest();
524524
request->SetQuery(query);

‎ydb/core/kqp/workload_service/ut/common/kqp_workload_service_ut_common.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ struct TQueryRunnerSettings {
2626
FLUENT_SETTING_DEFAULT(ui32, NodeIndex, 0);
2727
FLUENT_SETTING_DEFAULT(std::optional<TString>, PoolId, std::nullopt);
2828
FLUENT_SETTING_DEFAULT(TString, UserSID, "user@" BUILTIN_SYSTEM_DOMAIN);
29+
FLUENT_SETTING_DEFAULT(TVector<TString>, GroupSIDs, {});
2930
FLUENT_SETTING_DEFAULT(TString, Database, "");
3031

3132
// Runner settings

0 commit comments

Comments
 (0)