Skip to content

YQ-3653 added resource pools classifiers validations #9037

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.

Sign up for GitHub

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 2 commits into from
Sep 11, 2024
Merged

YQ-3653 added resource pools classifiers validations #9037

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
93a30e6
Added classifiers validations
GrigoriyPA Sep 10, 2024
49151bc
Fixed typo
GrigoriyPA Sep 10, 2024
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
19 changes: 18 additions & 1 deletion ydb/core/kqp/gateway/behaviour/resource_pool_classifier/manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,11 @@ NMetadata::NModifications::TOperationParsingResult TResourcePoolClassifierManage
} catch (...) {
throw yexception() << "Failed to parse property " << property << ": " << CurrentExceptionMessage();
}
} else if (!featuresExtractor.ExtractResetFeature(property)) {
} else if (featuresExtractor.ExtractResetFeature(property)) {
if (property == "resource_pool") {
ythrow yexception() << "Cannot reset required property resource_pool";
}
} else {
continue;
}

Expand All @@ -65,6 +69,19 @@ NMetadata::NModifications::TOperationParsingResult TResourcePoolClassifierManage
}
}

if (context.GetActivityType() == EActivityType::Create) {
if (!configJson.GetMap().contains("resource_pool")) {
ythrow yexception() << "Missing required property resource_pool";
}

static const TString extraPathSymbolsAllowed = "!\"#$%&'()*+,-.:;<=>?@[\\]^_`{|}~";
const auto& name = settings.GetObjectId();
if (const auto brokenAt = PathPartBrokenAt(name, extraPathSymbolsAllowed); brokenAt != name.end()) {
ythrow yexception() << "Symbol '" << *brokenAt << "'" << " is not allowed in the resource pool classifier name '" << name << "'";
}
}
resourcePoolClassifierSettings.Validate();

NJsonWriter::TBuf writer;
writer.WriteJsonValue(&configJson);
result.SetColumn(TResourcePoolClassifierConfig::TDecoder::ConfigJson, NMetadata::NInternal::TYDBValue::Utf8(writer.Str()));
Expand Down
24 changes: 14 additions & 10 deletions ydb/core/kqp/proxy_service/kqp_proxy_service_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,17 +422,19 @@ class TResourcePoolsCache {
struct TClassifierInfo {
const TString Membername;
const TString PoolId;
const i64 Rank;

TClassifierInfo(const NResourcePool::TClassifierSettings& classifierSettings)
: Membername(classifierSettings.Membername)
, PoolId(classifierSettings.ResourcePool)
, Rank(classifierSettings.Rank)
{}
};

struct TDatabaseInfo {
std::unordered_map<TString, TResourcePoolClassifierConfig> ResourcePoolsClassifiers = {};
std::map<i64, TClassifierInfo> RankToClassifierInfo = {};
std::unordered_map<TString, TString> UserToResourcePool = {};
std::unordered_map<TString, std::pair<TString, i64>> UserToResourcePool = {};
bool Serverless = false;
};

Expand Down Expand Up @@ -462,16 +464,16 @@ class TResourcePoolsCache {
}

TDatabaseInfo& databaseInfo = *GetOrCreateDatabaseInfo(database);
if (const auto& poolId = GetPoolIdFromClassifiers(database, userToken->GetUserSID(), databaseInfo, userToken, actorContext)) {
return poolId;
}
auto [resultPoolId, resultRank] = GetPoolIdFromClassifiers(database, userToken->GetUserSID(), databaseInfo, userToken, actorContext);
for (const auto& userSID : userToken->GetGroupSIDs()) {
if (const auto& poolId = GetPoolIdFromClassifiers(database, userSID, databaseInfo, userToken, actorContext)) {
return poolId;
const auto& [poolId, rank] = GetPoolIdFromClassifiers(database, userSID, databaseInfo, userToken, actorContext);
if (poolId && (!resultPoolId || resultRank > rank)) {
resultPoolId = poolId;
resultRank = rank;
}
}

return NResourcePool::DEFAULT_POOL_ID;
return resultPoolId ? resultPoolId : NResourcePool::DEFAULT_POOL_ID;
}

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

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

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

poolId = classifier.PoolId;
rank = classifier.Rank;
break;
}

usersMap[userSID] = poolId;
return poolId;
usersMap[userSID] = {poolId, rank};
return {poolId, rank};
}

TDatabaseInfo* GetOrCreateDatabaseInfo(const TString& database) {
Expand Down
62 changes: 51 additions & 11 deletions ydb/core/kqp/ut/scheme/kqp_scheme_ut.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6877,8 +6877,8 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
// ALTER RESOURCE POOL CLASSIFIER
checkDisabled(R"(
ALTER RESOURCE POOL CLASSIFIER MyResourcePoolClassifier
SET (RANK = 1, MEMBERNAME = "test@user"),
RESET (RESOURCE_POOL);
SET (RANK = 1, RESOURCE_POOL = "test"),
RESET (MEMBERNAME);
)");

// DROP RESOURCE POOL CLASSIFIER
Expand Down Expand Up @@ -6911,8 +6911,8 @@ Y_UNIT_TEST_SUITE(KqpScheme) {

const auto& alterSql = R"(
ALTER RESOURCE POOL CLASSIFIER MyResourcePoolClassifier
SET (RANK = 1, MEMBERNAME = "test@user"),
RESET (RESOURCE_POOL);
SET (RANK = 1, RESOURCE_POOL = "test"),
RESET (MEMBERNAME);
)";

const auto& dropSql = "DROP RESOURCE POOL CLASSIFIER MyResourcePoolClassifier;";
Expand Down Expand Up @@ -6951,6 +6951,7 @@ Y_UNIT_TEST_SUITE(KqpScheme) {

auto result = session.ExecuteSchemeQuery(R"(
CREATE RESOURCE POOL CLASSIFIER MyResourcePoolClassifier WITH (
RESOURCE_POOL="test",
ANOTHER_PROPERTY=20
);)").GetValueSync();
UNIT_ASSERT_VALUES_EQUAL(result.GetStatus(), EStatus::GENERIC_ERROR);
Expand All @@ -6966,10 +6967,40 @@ Y_UNIT_TEST_SUITE(KqpScheme) {

result = session.ExecuteSchemeQuery(R"(
CREATE RESOURCE POOL CLASSIFIER MyResourcePoolClassifier WITH (
RESOURCE_POOL="test",
RANK="StringValue"
);)").GetValueSync();
UNIT_ASSERT_VALUES_EQUAL(result.GetStatus(), EStatus::GENERIC_ERROR);
UNIT_ASSERT_STRING_CONTAINS(result.GetIssues().ToString(), "Failed to parse property rank:");

result = session.ExecuteSchemeQuery(R"(
CREATE RESOURCE POOL CLASSIFIER MyResourcePoolClassifier WITH (
RANK="0"
);)").GetValueSync();
UNIT_ASSERT_VALUES_EQUAL(result.GetStatus(), EStatus::GENERIC_ERROR);
UNIT_ASSERT_STRING_CONTAINS(result.GetIssues().ToString(), "Missing required property resource_pool");

result = session.ExecuteSchemeQuery(R"(
ALTER RESOURCE POOL CLASSIFIER MyResourcePoolClassifier
RESET (RESOURCE_POOL);
)").GetValueSync();
UNIT_ASSERT_VALUES_EQUAL(result.GetStatus(), EStatus::GENERIC_ERROR);
UNIT_ASSERT_STRING_CONTAINS(result.GetIssues().ToString(), "Cannot reset required property resource_pool");

result = session.ExecuteSchemeQuery(R"(
CREATE RESOURCE POOL CLASSIFIER `MyResource/PoolClassifier` WITH (
RESOURCE_POOL="test"
);)").GetValueSync();
UNIT_ASSERT_VALUES_EQUAL(result.GetStatus(), EStatus::GENERIC_ERROR);
UNIT_ASSERT_STRING_CONTAINS(result.GetIssues().ToString(), "Symbol '/' is not allowed in the resource pool classifier name 'MyResource/PoolClassifier'");

result = session.ExecuteSchemeQuery(TStringBuilder() << R"(
CREATE RESOURCE POOL CLASSIFIER MyResourcePoolClassifier WITH (
RESOURCE_POOL="test",
MEMBERNAME=")" << BUILTIN_ACL_METADATA << R"("
);)").GetValueSync();
UNIT_ASSERT_VALUES_EQUAL(result.GetStatus(), EStatus::GENERIC_ERROR);
UNIT_ASSERT_STRING_CONTAINS(result.GetIssues().ToString(), TStringBuilder() << "Invalid resource pool classifier configuration, cannot create classifier for system user " << BUILTIN_ACL_METADATA);
}

Y_UNIT_TEST(ResourcePoolClassifiersRankValidation) {
Expand All @@ -6986,13 +7017,15 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
// Create with sample rank
auto result = session.ExecuteSchemeQuery(R"(
CREATE RESOURCE POOL CLASSIFIER ClassifierRank42 WITH (
RESOURCE_POOL="test_pool",
RANK=42
);)").GetValueSync();
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToOneLineString());

// Try to create with same rank
result = session.ExecuteSchemeQuery(R"(
CREATE RESOURCE POOL CLASSIFIER AnotherClassifierRank42 WITH (
RESOURCE_POOL="test_pool",
RANK=42
);)").GetValueSync();
UNIT_ASSERT_VALUES_EQUAL(result.GetStatus(), EStatus::GENERIC_ERROR);
Expand All @@ -7001,13 +7034,15 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
// Create with high rank
result = session.ExecuteSchemeQuery(R"(
CREATE RESOURCE POOL CLASSIFIER `ClassifierRank2^63` WITH (
RESOURCE_POOL="test_pool",
RANK=9223372036854775807
);)").GetValueSync();
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToOneLineString());

// Try to create with auto rank
result = session.ExecuteSchemeQuery(R"(
CREATE RESOURCE POOL CLASSIFIER ClassifierRankAuto WITH (
RESOURCE_POOL="test_pool",
MEMBERNAME="test@user"
);)").GetValueSync();
UNIT_ASSERT_VALUES_EQUAL(result.GetStatus(), EStatus::GENERIC_ERROR);
Expand Down Expand Up @@ -7065,11 +7100,12 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
// Auto rank
query = R"(
CREATE RESOURCE POOL CLASSIFIER AnotherResourcePoolClassifier WITH (
RESOURCE_POOL="test_pool",
MEMBERNAME="another@user"
);)";
result = session.ExecuteSchemeQuery(query).GetValueSync();
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
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\"}]}");
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\"}]}");
}

Y_UNIT_TEST(DoubleCreateResourcePoolClassifier) {
Expand All @@ -7086,6 +7122,7 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
{
auto query = R"(
CREATE RESOURCE POOL CLASSIFIER MyResourcePoolClassifier WITH (
RESOURCE_POOL="test_pool",
RANK=20
);)";
auto result = session.ExecuteSchemeQuery(query).GetValueSync();
Expand All @@ -7095,6 +7132,7 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
{
auto query = R"(
CREATE RESOURCE POOL CLASSIFIER MyResourcePoolClassifier WITH (
RESOURCE_POOL="test_pool",
RANK=1
);)";
auto result = session.ExecuteSchemeQuery(query).GetValueSync();
Expand Down Expand Up @@ -7141,22 +7179,23 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
{
auto query = R"(
CREATE RESOURCE POOL CLASSIFIER AnotherResourcePoolClassifier WITH (
RESOURCE_POOL="test_pool",
RANK=42
);)";
auto result = session.ExecuteSchemeQuery(query).GetValueSync();
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
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\"}]}");
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\"}]}");
}

// Test reset
{
auto query = R"(
ALTER RESOURCE POOL CLASSIFIER MyResourcePoolClassifier
RESET (RANK, RESOURCE_POOL);
RESET (RANK, MEMBERNAME);
)";
auto result = session.ExecuteSchemeQuery(query).GetValueSync();
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
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\"}]}");
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\"}]}");
}
}

Expand All @@ -7173,8 +7212,8 @@ Y_UNIT_TEST_SUITE(KqpScheme) {

auto query = R"(
ALTER RESOURCE POOL CLASSIFIER MyResourcePoolClassifier
SET (MEMBERNAME = "test@user", RANK = 100),
RESET (RESOURCE_POOL);
SET (RESOURCE_POOL = "test", RANK = 100),
RESET (MEMBERNAME);
)";
auto result = session.ExecuteSchemeQuery(query).GetValueSync();
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::GENERIC_ERROR, result.GetIssues().ToString());
Expand All @@ -7195,11 +7234,12 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
{
auto query = R"(
CREATE RESOURCE POOL CLASSIFIER MyResourcePoolClassifier WITH (
RESOURCE_POOL="test_pool",
RANK=20
);)";
auto result = session.ExecuteSchemeQuery(query).GetValueSync();
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
UNIT_ASSERT_VALUES_EQUAL(FetchResourcePoolClassifiers(kikimr), "{\"resource_pool_classifiers\":[{\"rank\":20,\"name\":\"MyResourcePoolClassifier\",\"config\":{},\"database\":\"\\/Root\"}]}");
UNIT_ASSERT_VALUES_EQUAL(FetchResourcePoolClassifiers(kikimr), "{\"resource_pool_classifiers\":[{\"rank\":20,\"name\":\"MyResourcePoolClassifier\",\"config\":{\"resource_pool\":\"test_pool\"},\"database\":\"\\/Root\"}]}");
}

{
Expand Down
2 changes: 1 addition & 1 deletion ydb/core/kqp/workload_service/ut/common/kqp_workload_service_ut_common.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,7 +518,7 @@ class TWorkloadServiceYdbSetup : public IYdbSetup {
UNIT_ASSERT_C(settings.PoolId_, "Query pool id is not specified");

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

auto request = event->Record.MutableRequest();
request->SetQuery(query);
Expand Down
1 change: 1 addition & 0 deletions ydb/core/kqp/workload_service/ut/common/kqp_workload_service_ut_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ struct TQueryRunnerSettings {
FLUENT_SETTING_DEFAULT(ui32, NodeIndex, 0);
FLUENT_SETTING_DEFAULT(std::optional<TString>, PoolId, std::nullopt);
FLUENT_SETTING_DEFAULT(TString, UserSID, "user@" BUILTIN_SYSTEM_DOMAIN);
FLUENT_SETTING_DEFAULT(TVector<TString>, GroupSIDs, {});
FLUENT_SETTING_DEFAULT(TString, Database, "");

// Runner settings
Expand Down
Loading
Loading