From 63d492c63926db328fd101b7328ed78e5a95ff03 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Thu, 22 Jul 2021 16:50:01 +0000 Subject: [PATCH 1/2] config: allow .yml files to load as YAML This is a common source of user confusion. Signed-off-by: Matt Klein --- docs/root/version_history/current.rst | 1 + source/common/protobuf/utility.cc | 3 ++- source/common/protobuf/utility.h | 1 + test/config/integration/BUILD | 2 +- ...ver_xds.bootstrap.yaml => server_xds.bootstrap.yml} | 0 .../integration/dynamic_validation_integration_test.cc | 10 +++++----- test/integration/xds_integration_test.cc | 4 ++-- test/test_common/environment.cc | 4 +--- 8 files changed, 13 insertions(+), 12 deletions(-) rename test/config/integration/{server_xds.bootstrap.yaml => server_xds.bootstrap.yml} (100%) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 5261f906b4ec3..e1b9730fd04f2 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -9,6 +9,7 @@ Minor Behavior Changes ---------------------- *Changes that may cause incompatibilities for some users, but should not for most* +* config: configuration files ending in .yml now load as YAML. * grpc: gRPC async client can be cached and shared accross filter instances in the same thread, this feature is turned off by default, can be turned on by setting runtime guard ``envoy.reloadable_features.enable_grpc_async_client_cache`` to true. * http: correct the use of the ``x-forwarded-proto`` header and the ``:scheme`` header. Where they differ (which is rare) ``:scheme`` will now be used for serving redirect URIs and cached content. This behavior diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index d611a3a69318b..8992988888f70 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -482,7 +482,8 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa } return; } - if (absl::EndsWith(path, FileExtensions::get().Yaml)) { + if (absl::EndsWith(path, FileExtensions::get().Yaml) || + absl::EndsWith(path, FileExtensions::get().Yml)) { loadFromYaml(contents, message, validation_visitor, do_boosting); } else { loadFromJson(contents, message, validation_visitor, do_boosting); diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index 0ba43337a8344..7ccc41394eafd 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -237,6 +237,7 @@ class MessageUtil { const std::string ProtoText = ".pb_text"; const std::string Json = ".json"; const std::string Yaml = ".yaml"; + const std::string Yml = ".yml"; }; using FileExtensions = ConstSingleton; diff --git a/test/config/integration/BUILD b/test/config/integration/BUILD index 0c401ad01bb0a..94c9f35bd99d5 100644 --- a/test/config/integration/BUILD +++ b/test/config/integration/BUILD @@ -16,7 +16,7 @@ filegroup( name = "server_xds_files", srcs = [ "server_xds.bootstrap.udpa.yaml", - "server_xds.bootstrap.yaml", + "server_xds.bootstrap.yml", "server_xds.cds.with_unknown_field.yaml", "server_xds.cds.yaml", "server_xds.eds.ads_cluster.yaml", diff --git a/test/config/integration/server_xds.bootstrap.yaml b/test/config/integration/server_xds.bootstrap.yml similarity index 100% rename from test/config/integration/server_xds.bootstrap.yaml rename to test/config/integration/server_xds.bootstrap.yml diff --git a/test/integration/dynamic_validation_integration_test.cc b/test/integration/dynamic_validation_integration_test.cc index e6f7c5dddc53d..171b7bc22cf07 100644 --- a/test/integration/dynamic_validation_integration_test.cc +++ b/test/integration/dynamic_validation_integration_test.cc @@ -96,7 +96,7 @@ INSTANTIATE_TEST_SUITE_P( // Protocol options in CDS with unknown fields are rejected if and only if strict. TEST_P(DynamicValidationIntegrationTest, CdsProtocolOptionsRejected) { api_filesystem_config_ = { - "test/config/integration/server_xds.bootstrap.yaml", + "test/config/integration/server_xds.bootstrap.yml", "test/config/integration/server_xds.cds.with_unknown_field.yaml", "test/config/integration/server_xds.eds.yaml", "test/config/integration/server_xds.lds.yaml", @@ -122,7 +122,7 @@ TEST_P(DynamicValidationIntegrationTest, CdsProtocolOptionsRejected) { TEST_P(DynamicValidationIntegrationTest, LdsFilterRejected) { allow_lds_rejection_ = true; api_filesystem_config_ = { - "test/config/integration/server_xds.bootstrap.yaml", + "test/config/integration/server_xds.bootstrap.yml", "test/config/integration/server_xds.cds.yaml", "test/config/integration/server_xds.eds.yaml", "test/config/integration/server_xds.lds.with_unknown_field.yaml", @@ -153,7 +153,7 @@ TEST_P(DynamicValidationIntegrationTest, LdsFilterRejected) { TEST_P(DynamicValidationIntegrationTest, LdsFilterRejectedTypedStruct) { allow_lds_rejection_ = true; api_filesystem_config_ = { - "test/config/integration/server_xds.bootstrap.yaml", + "test/config/integration/server_xds.bootstrap.yml", "test/config/integration/server_xds.cds.yaml", "test/config/integration/server_xds.eds.yaml", "test/config/integration/server_xds.lds.with_unknown_field.typed_struct.yaml", @@ -182,7 +182,7 @@ TEST_P(DynamicValidationIntegrationTest, LdsFilterRejectedTypedStruct) { // Unknown fields in RDS cause config load failure if and only if strict. TEST_P(DynamicValidationIntegrationTest, RdsFailedBySubscription) { api_filesystem_config_ = { - "test/config/integration/server_xds.bootstrap.yaml", + "test/config/integration/server_xds.bootstrap.yml", "test/config/integration/server_xds.cds.yaml", "test/config/integration/server_xds.eds.yaml", "test/config/integration/server_xds.lds.yaml", @@ -210,7 +210,7 @@ TEST_P(DynamicValidationIntegrationTest, RdsFailedBySubscription) { // Unknown fields in EDS cause config load failure if and only if strict. TEST_P(DynamicValidationIntegrationTest, EdsFailedBySubscription) { api_filesystem_config_ = { - "test/config/integration/server_xds.bootstrap.yaml", + "test/config/integration/server_xds.bootstrap.yml", "test/config/integration/server_xds.cds.yaml", "test/config/integration/server_xds.eds.with_unknown_field.yaml", "test/config/integration/server_xds.lds.yaml", diff --git a/test/integration/xds_integration_test.cc b/test/integration/xds_integration_test.cc index 6a0cadb551b49..fe6a3cca164f4 100644 --- a/test/integration/xds_integration_test.cc +++ b/test/integration/xds_integration_test.cc @@ -27,7 +27,7 @@ class XdsIntegrationTest : public testing::TestWithParam Date: Fri, 23 Jul 2021 17:46:33 +0000 Subject: [PATCH 2/2] comments Signed-off-by: Matt Klein --- docs/root/version_history/current.rst | 1 + source/common/protobuf/utility.cc | 8 ++-- test/common/protobuf/utility_test.cc | 59 ++++++++++++++++++++++++++- 3 files changed, 62 insertions(+), 6 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index c284acb3797c3..9877d650b1233 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -10,6 +10,7 @@ Minor Behavior Changes *Changes that may cause incompatibilities for some users, but should not for most* * config: configuration files ending in .yml now load as YAML. +* config: configuration file extensions now ignore case when deciding the file type. E.g., .JSON file load as JSON. * config: reduced log level for "Unable to establish new stream" xDS logs to debug. The log level for "gRPC config stream closed" is now reduced to debug when the status is ``Ok`` or has been retriable (``DeadlineExceeded``, ``ResourceExhausted``, or ``Unavailable``) for less than 30 diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 8992988888f70..04f5907db2cb1 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -423,7 +423,7 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa Api::Api& api, bool do_boosting) { const std::string contents = api.fileSystem().fileReadToEnd(path); // If the filename ends with .pb, attempt to parse it as a binary proto. - if (absl::EndsWith(path, FileExtensions::get().ProtoBinary)) { + if (absl::EndsWithIgnoreCase(path, FileExtensions::get().ProtoBinary)) { // Attempt to parse the binary format. auto read_proto_binary = [&contents, &validation_visitor](Protobuf::Message& message, MessageVersion message_version) { @@ -459,7 +459,7 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa } // If the filename ends with .pb_text, attempt to parse it as a text proto. - if (absl::EndsWith(path, FileExtensions::get().ProtoText)) { + if (absl::EndsWithIgnoreCase(path, FileExtensions::get().ProtoText)) { auto read_proto_text = [&contents, &path](Protobuf::Message& message, MessageVersion message_version) { if (Protobuf::TextFormat::ParseFromString(contents, &message)) { @@ -482,8 +482,8 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa } return; } - if (absl::EndsWith(path, FileExtensions::get().Yaml) || - absl::EndsWith(path, FileExtensions::get().Yml)) { + if (absl::EndsWithIgnoreCase(path, FileExtensions::get().Yaml) || + absl::EndsWithIgnoreCase(path, FileExtensions::get().Yml)) { loadFromYaml(contents, message, validation_visitor, do_boosting); } else { loadFromJson(contents, message, validation_visitor, do_boosting); diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 070b6cecb46b0..bbb9c9c8a9130 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -293,8 +293,9 @@ TEST_F(ProtobufUtilityTest, LoadBinaryProtoFromFile) { ->mutable_source_address() ->set_address("1.1.1.1"); + // Test mixed case extension. const std::string filename = - TestEnvironment::writeStringToFileForTest("proto.pb", bootstrap.SerializeAsString()); + TestEnvironment::writeStringToFileForTest("proto.pB", bootstrap.SerializeAsString()); envoy::config::bootstrap::v3::Bootstrap proto_from_file; TestUtility::loadFromFile(filename, proto_from_file, *api_); @@ -302,6 +303,59 @@ TEST_F(ProtobufUtilityTest, LoadBinaryProtoFromFile) { EXPECT_TRUE(TestUtility::protoEqual(bootstrap, proto_from_file)); } +// Verify different YAML extensions using different cases. +TEST_F(ProtobufUtilityTest, YamlExtensions) { + const std::string bootstrap_yaml = R"EOF( +layered_runtime: + layers: + - name: static_layer + static_layer: + foo: true)EOF"; + + { + const std::string filename = + TestEnvironment::writeStringToFileForTest("proto.yAml", bootstrap_yaml); + + envoy::config::bootstrap::v3::Bootstrap proto_from_file; + TestUtility::loadFromFile(filename, proto_from_file, *api_); + TestUtility::validate(proto_from_file); + } + { + const std::string filename = + TestEnvironment::writeStringToFileForTest("proto.yMl", bootstrap_yaml); + + envoy::config::bootstrap::v3::Bootstrap proto_from_file; + TestUtility::loadFromFile(filename, proto_from_file, *api_); + TestUtility::validate(proto_from_file); + } +} + +// Verify different JSON extensions using different cases. +TEST_F(ProtobufUtilityTest, JsonExtensions) { + const std::string bootstrap_json = R"EOF( +{ + "layered_runtime": { + "layers": [ + { + "name": "static_layer", + "static_layer": { + "foo": true + } + } + ] + } +})EOF"; + + { + const std::string filename = + TestEnvironment::writeStringToFileForTest("proto.JSoN", bootstrap_json); + + envoy::config::bootstrap::v3::Bootstrap proto_from_file; + TestUtility::loadFromFile(filename, proto_from_file, *api_); + TestUtility::validate(proto_from_file); + } +} + // Verify that a config with a deprecated field can be loaded with runtime global override. TEST_F(ProtobufUtilityTest, DEPRECATED_FEATURE_TEST(LoadBinaryGlobalOverrideProtoFromFile)) { const std::string bootstrap_yaml = R"EOF( @@ -364,8 +418,9 @@ TEST_F(ProtobufUtilityTest, LoadTextProtoFromFile) { std::string bootstrap_text; ASSERT_TRUE(Protobuf::TextFormat::PrintToString(bootstrap, &bootstrap_text)); + // Test mixed case extension. const std::string filename = - TestEnvironment::writeStringToFileForTest("proto.pb_text", bootstrap_text); + TestEnvironment::writeStringToFileForTest("proto.pB_Text", bootstrap_text); envoy::config::bootstrap::v3::Bootstrap proto_from_file; TestUtility::loadFromFile(filename, proto_from_file, *api_);