Add options deprecation support to dynconfig (#7162)

Enjection · web-flow · commit 74f3150f3e64 · 2024-07-30T00:09:20.000+03:00
diff --git a/ydb/core/cms/console/console__replace_yaml_config.cpp b/ydb/core/cms/console/console__replace_yaml_config.cpp
@@ -76,6 +76,11 @@ class TConfigsManager::TTxReplaceYamlConfig : public TTransactionBase<TConfigsMa
                     .Cluster = Cluster,
                 });
 
+            bool hasForbiddenUnknown = false;
+
+            TMap<TString, std::pair<TString, TString>> deprecatedFields;
+            TMap<TString, std::pair<TString, TString>> unknownFields;
+
             if (UpdatedConfig != Self->YamlConfig || Self->YamlDropped) {
                 Modify = true;
 
@@ -90,19 +95,29 @@ class TConfigsManager::TTxReplaceYamlConfig : public TTransactionBase<TConfigsMa
                     ythrow yexception() << "Version mismatch";
                 }
 
-                if (AllowUnknownFields) {
-                    UnknownFieldsCollector = new NYamlConfig::TBasicUnknownFieldsCollector;
-                }
+                UnknownFieldsCollector = new NYamlConfig::TBasicUnknownFieldsCollector;
 
                 for (auto& [_, config] : resolved.Configs) {
                     auto cfg = NYamlConfig::YamlToProto(
                         config.second,
-                        AllowUnknownFields,
+                        true,
                         true,
                         UnknownFieldsCollector);
                 }
 
-                if (!DryRun) {
+                const auto& deprecatedPaths = NKikimrConfig::TAppConfig::GetReservedChildrenPaths();
+
+                for (const auto& [path, info] : UnknownFieldsCollector->GetUnknownKeys()) {
+                    if (deprecatedPaths.contains(path)) {
+                        deprecatedFields[path] = info;
+                    } else {
+                        unknownFields[path] = info;
+                    }
+                }
+
+                hasForbiddenUnknown = !unknownFields.empty() && !AllowUnknownFields;
+
+                if (!DryRun && !hasForbiddenUnknown) {
                     DoAudit(txc, ctx);
 
                     db.Table<Schema::YamlConfig>().Key(Version + 1)
@@ -118,25 +133,34 @@ class TConfigsManager::TTxReplaceYamlConfig : public TTransactionBase<TConfigsMa
                 }
             }
 
-            auto fillResponse = [&](auto& ev){
-                if (UnknownFieldsCollector) {
-                    for (auto& [path, info] : UnknownFieldsCollector->GetUnknownKeys()) {
-                        auto *issue = ev->Record.AddIssues();
-                            issue->set_severity(NYql::TSeverityIds::S_WARNING);
-                            issue->set_message(TStringBuilder{} << "Unknown key# " << info.first << " in proto# " << info.second << " found in path# " << path);
-                    }
+            auto fillResponse = [&](auto& ev, auto errorLevel){
+                for (auto& [path, info] : unknownFields) {
+                    auto *issue = ev->Record.AddIssues();
+                        issue->set_severity(errorLevel);
+                        issue->set_message(TStringBuilder{} << "Unknown key# " << info.first << " in proto# " << info.second << " found in path# " << path);
+                }
+
+                for (auto& [path, info] : deprecatedFields) {
+                    auto *issue = ev->Record.AddIssues();
+                        issue->set_severity(NYql::TSeverityIds::S_WARNING);
+                        issue->set_message(TStringBuilder{} << "Deprecated key# " << info.first << " in proto# " << info.second << " found in path# " << path);
                 }
 
                 Response = MakeHolder<NActors::IEventHandle>(Sender, ctx.SelfID, ev.Release());
             };
 
 
-            if (!Force) {
+            if (hasForbiddenUnknown) {
+                Error = true;
+                auto ev = MakeHolder<TEvConsole::TEvGenericError>();
+                ev->Record.SetYdbStatus(Ydb::StatusIds::BAD_REQUEST);
+                fillResponse(ev, NYql::TSeverityIds::S_ERROR);
+            } else if (!Force) {
                 auto ev = MakeHolder<TEvConsole::TEvReplaceYamlConfigResponse>();
-                fillResponse(ev);
+                fillResponse(ev, NYql::TSeverityIds::S_WARNING);
             } else {
                 auto ev = MakeHolder<TEvConsole::TEvSetYamlConfigResponse>();
-                fillResponse(ev);
+                fillResponse(ev, NYql::TSeverityIds::S_WARNING);
             }
         } catch (const yexception& ex) {
             Error = true;
diff --git a/ydb/core/config/tools/protobuf_plugin/main.cpp b/ydb/core/config/tools/protobuf_plugin/main.cpp
@@ -1,21 +1,28 @@
 #include <map>
+#include <unordered_set>
 
-#include <util/system/compiler.h>
+#include <util/generic/deque.h>
 #include <util/generic/map.h>
-#include <util/generic/set.h>
 #include <util/generic/maybe.h>
 #include <util/generic/ptr.h>
+#include <util/generic/set.h>
 #include <util/generic/vector.h>
 #include <util/string/builder.h>
 #include <util/string/cast.h>
 #include <util/string/printf.h>
 #include <util/string/subst.h>
+#include <util/string/join.h>
+#include <util/system/compiler.h>
+
+#include <library/cpp/protobuf/json/util.h>
 
 #include <google/protobuf/compiler/code_generator.h>
 #include <google/protobuf/compiler/plugin.h>
 #include <google/protobuf/io/printer.h>
 #include <google/protobuf/io/zero_copy_stream.h>
+
 #include <ydb/core/config/protos/marker.pb.h>
+#include <ydb/core/config/utils/config_traverse.h>
 
 #include <ydb/public/lib/protobuf/base_message_generator.h>
 #include <ydb/public/lib/protobuf/helpers.h>
@@ -31,8 +38,63 @@ class TMessageGenerator
     : public TBaseMessageGenerator
 {
 private:
+    TString FieldName(const FieldDescriptor* field) const {
+        TString name = field->name();
+        NProtobufJson::ToSnakeCaseDense(&name);
+        return name;
+    }
+
+    TString ConstructFullFieldPath(const TDeque<const FieldDescriptor*>& fieldPath, const FieldDescriptor* field) const {
+
+        TVector<TString> path;
+        path.push_back("");
+        for (size_t i = 1; i < fieldPath.size(); ++i) {
+            TString fieldName = FieldName(fieldPath[i]);
+            path.push_back(fieldName);
+        }
+
+        return JoinSeq("/", path);
+    }
 
     void GenerateConfigRoot(TVars vars) {
+        std::unordered_set<TString> reservedPaths;
+        NKikimr::NConfig::Traverse([&](const Descriptor* d, const TDeque<const Descriptor*>& typePath, const TDeque<const FieldDescriptor*>& fieldPath, const FieldDescriptor* field, ssize_t loop) {
+            Y_UNUSED(fieldPath, typePath, fieldPath, field, loop);
+            if (field && d) {
+                for (int i = 0; i < d->reserved_name_count(); ++i) {
+                    TString name = d->reserved_name(i);
+                    NProtobufJson::ToSnakeCaseDense(&name);
+                    reservedPaths.insert(ConstructFullFieldPath(fieldPath, field) + "/" + name);
+                }
+            }
+        }, Message);
+
+        for (int i = 0; i < Message->reserved_name_count(); ++i) {
+            TString name = Message->reserved_name(i);
+            NProtobufJson::ToSnakeCaseDense(&name);
+            reservedPaths.insert(TString("/") + name);
+        }
+
+        WITH_PLUGIN_MARKUP(HeaderIncludes, PLUGIN_NAME) {
+            HeaderIncludes->Print(Vars, "#include <unordered_set>\n");
+        }
+
+        WITH_PLUGIN_MARKUP(Header, PLUGIN_NAME) {
+            Header->Print(vars, "inline static const std::unordered_set<TString>& GetReservedChildrenPaths() {\n");
+            WITH_INDENT(Header) {
+                Header->Print(vars, "static const std::unordered_set<TString> reserved = {\n");
+                WITH_INDENT(Header) {
+                    for (const auto& path : reservedPaths) {
+                        vars["reservedPath"] = path;
+                        Header->Print(vars, "\"$reservedPath$\",\n");
+                    }
+                }
+                Header->Print(vars, "};\n");
+                Header->Print(vars, "return reserved;\n");
+            }
+            Header->Print(vars, "}\n");
+        }
+
         for (auto i = 0; i < Message->field_count(); ++i) {
             const FieldDescriptor* field = Message->field(i);
             if (field->is_repeated()) {
diff --git a/ydb/core/config/tools/protobuf_plugin/ut.cpp b/ydb/core/config/tools/protobuf_plugin/ut.cpp
@@ -322,6 +322,16 @@ Y_UNIT_TEST_SUITE(ValidationTests) {
         UNIT_ASSERT_VALUES_EQUAL(thirdSink.GetRenamedComplexField1().GetStringField(), source.GetComplexMessage2().GetStringField());
         UNIT_ASSERT(!thirdSink.GetRenamedComplexField1().HasIntField());
     }
+
+    Y_UNIT_TEST(HasReservedPaths) {
+        NKikimrConfig::ActualConfigMessage msg;
+
+        const auto& reserved = msg.GetReservedChildrenPaths();
+
+        UNIT_ASSERT(reserved.contains("/field1/another_reserved_field"));
+        UNIT_ASSERT(reserved.contains("/field21/another_reserved_field"));
+        UNIT_ASSERT(reserved.contains("/root_reserved_field"));
+    }
 }
 
 template <>
diff --git a/ydb/core/config/tools/protobuf_plugin/ut/protos/config_root_test.proto b/ydb/core/config/tools/protobuf_plugin/ut/protos/config_root_test.proto
@@ -4,16 +4,19 @@ package NKikimrConfig;
 option java_package = "ru.yandex.kikimr.proto";
 
 message AdditionalMessage {
+    reserved "AnotherReservedField";
 }
 
 message AnotherMessage {
     optional string Content = 1;
     optional AdditionalMessage Field1 = 2;
+    reserved "IncludedReservedField";
 }
 
 message ActualConfigMessage {
     option (NMarkers.Root) = true;
     optional AdditionalMessage Field1 = 1;
     optional AdditionalMessage Field21 = 21;
     optional string Field201 = 201;
+    reserved "RootReservedField";
 }
diff --git a/ydb/core/config/tools/protobuf_plugin/ya.make b/ydb/core/config/tools/protobuf_plugin/ya.make
@@ -2,8 +2,10 @@ PROGRAM(config_proto_plugin)
 
 PEERDIR(
     contrib/libs/protoc
+    library/cpp/protobuf/json
     ydb/public/lib/protobuf
     ydb/core/config/protos
+    ydb/core/config/utils
 )
 
 SRCS(
diff --git a/ydb/core/config/ut/main.cpp b/ydb/core/config/ut/main.cpp
@@ -1,4 +1,5 @@
 #include <ydb/core/config/utils/config_traverse.h>
+#include <ydb/core/protos/config.pb.h>
 
 #include <library/cpp/colorizer/colors.h>
 #include <library/cpp/protobuf/json/util.h>
@@ -115,7 +116,8 @@ class TConfigDumper {
                     << " " << (field ? FieldName(field) : RootName())
                     << " = " << (field ? field->number() : 0) << ";"
                     << (loop != -1 ? Loop() : "")  << std::endl;
-        });
+        },
+        NKikimrConfig::TAppConfig::default_instance().GetDescriptor());
     }
 
     void PrintLoops() const {
@@ -130,7 +132,8 @@ class TConfigDumper {
                 path.push_back(DescriptorName(d));
                 std::cout << JoinSeq(" -> ", path) << std::endl;
             }
-        });
+        },
+        NKikimrConfig::TAppConfig::default_instance().GetDescriptor());
 
     }
 
@@ -141,7 +144,8 @@ class TConfigDumper {
             if (field && field->is_required()) {
                 fields[typePath.back()->file()][typePath.back()][field].insert(ConstructFullFieldPath(fieldPath, field));
             }
-        });
+        },
+        NKikimrConfig::TAppConfig::default_instance().GetDescriptor());
         return fields;
     }
 
diff --git a/ydb/core/config/ut/ya.make b/ydb/core/config/ut/ya.make
@@ -6,6 +6,7 @@ SRCS(
 
 PEERDIR(
     ydb/core/config/utils
+    ydb/core/protos
     library/cpp/colorizer
     library/cpp/testing/unittest
 )
diff --git a/ydb/core/config/utils/config_traverse.cpp b/ydb/core/config/utils/config_traverse.cpp
@@ -1,7 +1,5 @@
 #include "config_traverse.h"
 
-#include <ydb/core/protos/config.pb.h>
-
 #include <contrib/libs/protobuf/src/google/protobuf/descriptor.h>
 
 #include <util/generic/deque.h>
@@ -45,10 +43,7 @@ void Traverse(const Descriptor* d, TDeque<const Descriptor*>& typePath, TDeque<c
     typePath.pop_back();
 }
 
-void Traverse(TOnEntryFn onEntry) {
-    auto& inst = NKikimrConfig::TAppConfig::default_instance();
-    const Descriptor* descriptor = inst.GetDescriptor();
-
+void Traverse(TOnEntryFn onEntry, const Descriptor* descriptor) {
     TDeque<const Descriptor*> typePath;
     TDeque<const FieldDescriptor*> fieldPath;
     fieldPath.push_back(nullptr);
diff --git a/ydb/core/config/utils/config_traverse.h b/ydb/core/config/utils/config_traverse.h
@@ -15,6 +15,6 @@ using namespace google::protobuf;
 
 using TOnEntryFn = std::function<void(const Descriptor*, const TDeque<const Descriptor*>&, const TDeque<const FieldDescriptor*>&, const FieldDescriptor*, ssize_t)>;
 
-void Traverse(TOnEntryFn onEntry);
+void Traverse(TOnEntryFn onEntry, const Descriptor* descriptor);
 
 } // namespace NKikimr::NConfig
diff --git a/ydb/core/config/utils/ya.make b/ydb/core/config/utils/ya.make
@@ -5,7 +5,6 @@ SRCS(
 )
 
 PEERDIR(
-    ydb/core/protos
     library/cpp/protobuf/json
 )
 
diff --git a/ydb/core/protos/feature_flags.proto b/ydb/core/protos/feature_flags.proto
@@ -9,6 +9,10 @@ message TFeatureFlags {
         VALUE_FALSE = 2;
     }
 
+    // leave removed fields below
+    reserved "SomeRemovedFeatureFlagExample";
+    reserved "AnotherRemovedFeatureFlagExample";
+
     optional bool EnableSeparateSolomonShardForPDisk = 1 [default = true];
     optional bool UseForsetiSchedulerByDefaultForPDisk = 2 [default = true];
     optional bool EnableSeparateTrimThreadForPDisk = 3 [default = true];

Original file line number	Diff line number	Diff line change
`@@ -4,16 +4,19 @@ package NKikimrConfig;`
`4`	`4`	`option java_package = "ru.yandex.kikimr.proto";`
`5`	`5`
`6`	`6`	`message AdditionalMessage {`
	`7`	`+ reserved "AnotherReservedField";`
`7`	`8`	`}`
`8`	`9`
`9`	`10`	`message AnotherMessage {`
`10`	`11`	`optional string Content = 1;`
`11`	`12`	`optional AdditionalMessage Field1 = 2;`
	`13`	`+ reserved "IncludedReservedField";`
`12`	`14`	`}`
`13`	`15`
`14`	`16`	`message ActualConfigMessage {`
`15`	`17`	`option (NMarkers.Root) = true;`
`16`	`18`	`optional AdditionalMessage Field1 = 1;`
`17`	`19`	`optional AdditionalMessage Field21 = 21;`
`18`	`20`	`optional string Field201 = 201;`
	`21`	`+ reserved "RootReservedField";`
`19`	`22`	`}`
Original file line number	Diff line number	Diff line change
`@@ -2,8 +2,10 @@ PROGRAM(config_proto_plugin)`
`2`	`2`
`3`	`3`	`PEERDIR(`
`4`	`4`	`contrib/libs/protoc`
	`5`	`+ library/cpp/protobuf/json`
`5`	`6`	`ydb/public/lib/protobuf`
`6`	`7`	`ydb/core/config/protos`
	`8`	`+ ydb/core/config/utils`
`7`	`9`	`)`
`8`	`10`
`9`	`11`	`SRCS(`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,5 @@`
`1`	`1`	`#include <ydb/core/config/utils/config_traverse.h>`
	`2`	`+#include <ydb/core/protos/config.pb.h>`
`2`	`3`
`3`	`4`	`#include <library/cpp/colorizer/colors.h>`
`4`	`5`	`#include <library/cpp/protobuf/json/util.h>`
`@@ -115,7 +116,8 @@ class TConfigDumper {`
`115`	`116`	`<< " " << (field ? FieldName(field) : RootName())`
`116`	`117`	`<< " = " << (field ? field->number() : 0) << ";"`
`117`	`118`	`<< (loop != -1 ? Loop() : "") << std::endl;`
`118`		`- });`
	`119`	`+ },`
	`120`	`+ NKikimrConfig::TAppConfig::default_instance().GetDescriptor());`
`119`	`121`	`}`
`120`	`122`
`121`	`123`	`void PrintLoops() const {`
`@@ -130,7 +132,8 @@ class TConfigDumper {`
`130`	`132`	`path.push_back(DescriptorName(d));`
`131`	`133`	`std::cout << JoinSeq(" -> ", path) << std::endl;`
`132`	`134`	`}`
`133`		`- });`
	`135`	`+ },`
	`136`	`+ NKikimrConfig::TAppConfig::default_instance().GetDescriptor());`
`134`	`137`
`135`	`138`	`}`
`136`	`139`
`@@ -141,7 +144,8 @@ class TConfigDumper {`
`141`	`144`	`if (field && field->is_required()) {`
`142`	`145`	`fields[typePath.back()->file()][typePath.back()][field].insert(ConstructFullFieldPath(fieldPath, field));`
`143`	`146`	`}`
`144`		`- });`
	`147`	`+ },`
	`148`	`+ NKikimrConfig::TAppConfig::default_instance().GetDescriptor());`
`145`	`149`	`return fields;`
`146`	`150`	`}`
`147`	`151`
Original file line number	Diff line number	Diff line change
`@@ -6,6 +6,7 @@ SRCS(`
`6`	`6`
`7`	`7`	`PEERDIR(`
`8`	`8`	`ydb/core/config/utils`
	`9`	`+ ydb/core/protos`
`9`	`10`	`library/cpp/colorizer`
`10`	`11`	`library/cpp/testing/unittest`
`11`	`12`	`)`
Original file line number	Diff line number	Diff line change
`@@ -5,7 +5,6 @@ SRCS(`
`5`	`5`	`)`
`6`	`6`
`7`	`7`	`PEERDIR(`
`8`		`- ydb/core/protos`
`9`	`8`	`library/cpp/protobuf/json`
`10`	`9`	`)`
`11`	`10`