From a99a65b6e2b72f9f30b5d7f57a5220763ea9cd24 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Mon, 31 Oct 2022 17:02:20 -0700 Subject: [PATCH] GcpObservability: Add parsing for logging config (#31502) * GcpObservability: Add parsing for logging config * Unused parameters * Reviewer comments --- src/cpp/ext/gcp/BUILD | 1 + src/cpp/ext/gcp/observability_config.cc | 69 +++++++++ src/cpp/ext/gcp/observability_config.h | 35 ++++- test/cpp/ext/gcp/observability_config_test.cc | 145 +++++++++++++++++- 4 files changed, 247 insertions(+), 3 deletions(-) diff --git a/src/cpp/ext/gcp/BUILD b/src/cpp/ext/gcp/BUILD index b2e9eb602e4..c9796083696 100644 --- a/src/cpp/ext/gcp/BUILD +++ b/src/cpp/ext/gcp/BUILD @@ -85,5 +85,6 @@ grpc_cc_library( "//src/core:json_object_loader", "//src/core:slice_refcount", "//src/core:status_helper", + "//src/core:validation_errors", ], ) diff --git a/src/cpp/ext/gcp/observability_config.cc b/src/cpp/ext/gcp/observability_config.cc index b7157025902..97444e001a2 100644 --- a/src/cpp/ext/gcp/observability_config.cc +++ b/src/cpp/ext/gcp/observability_config.cc @@ -18,9 +18,16 @@ #include "src/cpp/ext/gcp/observability_config.h" +#include + +#include +#include #include #include "absl/status/status.h" +#include "absl/strings/match.h" +#include "absl/strings/str_cat.h" +#include "absl/strings/str_split.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" @@ -29,6 +36,7 @@ #include "src/core/lib/gprpp/env.h" #include "src/core/lib/gprpp/status_helper.h" +#include "src/core/lib/gprpp/validation_errors.h" #include "src/core/lib/iomgr/error.h" #include "src/core/lib/iomgr/load_file.h" #include "src/core/lib/json/json.h" @@ -95,6 +103,67 @@ std::string GetProjectIdFromGcpEnvVar() { } // namespace +// +// GcpObservabilityConfig::CloudLogging::RpcEventConfiguration +// + +const grpc_core::JsonLoaderInterface* +GcpObservabilityConfig::CloudLogging::RpcEventConfiguration::JsonLoader( + const grpc_core::JsonArgs&) { + static const auto* loader = + grpc_core::JsonObjectLoader() + .OptionalField("methods", &RpcEventConfiguration::qualified_methods) + .OptionalField("exclude", &RpcEventConfiguration::exclude) + .OptionalField("max_metadata_bytes", + &RpcEventConfiguration::max_metadata_bytes) + .OptionalField("max_message_bytes", + &RpcEventConfiguration::max_message_bytes) + .Finish(); + return loader; +} + +void GcpObservabilityConfig::CloudLogging::RpcEventConfiguration::JsonPostLoad( + const grpc_core::Json& /* json */, const grpc_core::JsonArgs& /* args */, + grpc_core::ValidationErrors* errors) { + grpc_core::ValidationErrors::ScopedField methods_field(errors, ".methods"); + parsed_methods.reserve(qualified_methods.size()); + for (size_t i = 0; i < qualified_methods.size(); ++i) { + grpc_core::ValidationErrors::ScopedField methods_index( + errors, absl::StrCat("[", i, "]")); + std::vector parts = + absl::StrSplit(qualified_methods[i], '/', absl::SkipEmpty()); + if (parts.size() > 2) { + errors->AddError("methods[] can have at most a single '/'"); + continue; + } else if (parts.empty()) { + errors->AddError("Empty configuration"); + continue; + } else if (parts.size() == 1) { + if (parts[0] != "*") { + errors->AddError("Illegal methods[] configuration"); + continue; + } + if (exclude) { + errors->AddError( + "Wildcard match '*' not allowed when 'exclude' is set"); + continue; + } + parsed_methods.push_back(ParsedMethod{parts[0], ""}); + } else { + // parts.size() == 2 + if (absl::StrContains(parts[0], '*')) { + errors->AddError("Configuration of type '*/method' not allowed"); + continue; + } + if (absl::StrContains(parts[1], '*') && parts[1].size() != 1) { + errors->AddError("Wildcard specified for method in incorrect manner"); + continue; + } + parsed_methods.push_back(ParsedMethod{parts[0], parts[1]}); + } + } +} + absl::StatusOr GcpObservabilityConfig::ReadFromEnv() { auto config_contents = GetGcpObservabilityConfigContents(); if (!config_contents.ok()) { diff --git a/src/cpp/ext/gcp/observability_config.h b/src/cpp/ext/gcp/observability_config.h index 56efa1a16f9..44272660e15 100644 --- a/src/cpp/ext/gcp/observability_config.h +++ b/src/cpp/ext/gcp/observability_config.h @@ -17,11 +17,17 @@ #ifndef GRPC_INTERNAL_CPP_EXT_GCP_OBSERVABILITY_GCP_OBSERVABILITY_CONFIG_H #define GRPC_INTERNAL_CPP_EXT_GCP_OBSERVABILITY_GCP_OBSERVABILITY_CONFIG_H +#include + #include +#include #include "absl/status/statusor.h" +#include "absl/strings/string_view.h" #include "absl/types/optional.h" +#include "src/core/lib/gprpp/validation_errors.h" +#include "src/core/lib/json/json.h" #include "src/core/lib/json/json_args.h" #include "src/core/lib/json/json_object_loader.h" @@ -30,10 +36,37 @@ namespace internal { struct GcpObservabilityConfig { struct CloudLogging { + struct RpcEventConfiguration { + struct ParsedMethod { + absl::string_view service; // backed by methods + absl::string_view method; // backed by methods + }; + std::vector qualified_methods; + std::vector parsed_methods; + bool exclude = false; + uint32_t max_metadata_bytes = 0; + uint32_t max_message_bytes = 0; + + static const grpc_core::JsonLoaderInterface* JsonLoader( + const grpc_core::JsonArgs&); + + void JsonPostLoad(const grpc_core::Json& json, + const grpc_core::JsonArgs& args, + grpc_core::ValidationErrors* errors); + }; + + std::vector client_rpc_events; + std::vector server_rpc_events; + static const grpc_core::JsonLoaderInterface* JsonLoader( const grpc_core::JsonArgs&) { static const auto* loader = - grpc_core::JsonObjectLoader().Finish(); + grpc_core::JsonObjectLoader() + .OptionalField("client_rpc_events", + &CloudLogging::client_rpc_events) + .OptionalField("server_rpc_events", + &CloudLogging::server_rpc_events) + .Finish(); return loader; } }; diff --git a/test/cpp/ext/gcp/observability_config_test.cc b/test/cpp/ext/gcp/observability_config_test.cc index bdfbd80638a..bbf68951829 100644 --- a/test/cpp/ext/gcp/observability_config_test.cc +++ b/test/cpp/ext/gcp/observability_config_test.cc @@ -32,7 +32,25 @@ namespace { TEST(GcpObservabilityConfigJsonParsingTest, Basic) { const char* json_str = R"json({ - "cloud_logging": {}, + "cloud_logging": { + "client_rpc_events": [ + { + "methods": ["google.pubsub.v1.Subscriber/Acknowledge", "google.pubsub.v1.Publisher/CreateTopic"], + "exclude": true + }, + { + "methods": ["google.pubsub.v1.Subscriber/*", "google.pubsub.v1.Publisher/*"], + "max_metadata_bytes": 4096, + "max_message_bytes": 4096 + }], + "server_rpc_events": [ + { + "methods": ["*"], + "max_metadata_bytes": 4096, + "max_message_bytes": 4096 + } + ] + }, "cloud_monitoring": {}, "cloud_trace": { "sampling_rate": 0.05 @@ -45,7 +63,27 @@ TEST(GcpObservabilityConfigJsonParsingTest, Basic) { auto config = grpc_core::LoadFromJson( *json, grpc_core::JsonArgs(), &errors); ASSERT_TRUE(errors.ok()) << errors.status("unexpected errors"); - EXPECT_TRUE(config.cloud_logging.has_value()); + ASSERT_TRUE(config.cloud_logging.has_value()); + ASSERT_EQ(config.cloud_logging->client_rpc_events.size(), 2); + EXPECT_THAT(config.cloud_logging->client_rpc_events[0].qualified_methods, + ::testing::ElementsAre("google.pubsub.v1.Subscriber/Acknowledge", + "google.pubsub.v1.Publisher/CreateTopic")); + EXPECT_TRUE(config.cloud_logging->client_rpc_events[0].exclude); + EXPECT_EQ(config.cloud_logging->client_rpc_events[0].max_metadata_bytes, 0); + EXPECT_EQ(config.cloud_logging->client_rpc_events[0].max_message_bytes, 0); + EXPECT_THAT(config.cloud_logging->client_rpc_events[1].qualified_methods, + ::testing::ElementsAre("google.pubsub.v1.Subscriber/*", + "google.pubsub.v1.Publisher/*")); + EXPECT_FALSE(config.cloud_logging->client_rpc_events[1].exclude); + EXPECT_EQ(config.cloud_logging->client_rpc_events[1].max_metadata_bytes, + 4096); + EXPECT_EQ(config.cloud_logging->client_rpc_events[1].max_message_bytes, 4096); + ASSERT_EQ(config.cloud_logging->server_rpc_events.size(), 1); + EXPECT_THAT(config.cloud_logging->server_rpc_events[0].qualified_methods, + ::testing::ElementsAre("*")); + EXPECT_EQ(config.cloud_logging->server_rpc_events[0].max_metadata_bytes, + 4096); + EXPECT_EQ(config.cloud_logging->server_rpc_events[0].max_message_bytes, 4096); EXPECT_TRUE(config.cloud_monitoring.has_value()); EXPECT_TRUE(config.cloud_trace.has_value()); EXPECT_FLOAT_EQ(config.cloud_trace->sampling_rate, 0.05); @@ -67,6 +105,109 @@ TEST(GcpObservabilityConfigJsonParsingTest, Defaults) { EXPECT_TRUE(config.project_id.empty()); } +TEST(GcpObservabilityConfigJsonParsingTest, LoggingConfigMethodIllegalSlashes) { + const char* json_str = R"json({ + "cloud_logging": { + "client_rpc_events": [ + { + "methods": ["servicemethod", "service/method/foo"] + } + ] + } + })json"; + auto json = grpc_core::Json::Parse(json_str); + ASSERT_TRUE(json.ok()) << json.status(); + grpc_core::ValidationErrors errors; + auto config = grpc_core::LoadFromJson( + *json, grpc_core::JsonArgs(), &errors); + EXPECT_THAT(errors.status("Parsing error").ToString(), + ::testing::AllOf( + ::testing::HasSubstr( + "field:cloud_logging.client_rpc_events[0].methods[0]" + " error:Illegal methods[] configuration"), + ::testing::HasSubstr( + "field:cloud_logging.client_rpc_events[0].methods[1] " + "error:methods[] can have at most a single '/'"))); +} + +TEST(GcpObservabilityConfigJsonParsingTest, LoggingConfigEmptyMethod) { + const char* json_str = R"json({ + "cloud_logging": { + "client_rpc_events": [ + { + "methods": [""] + } + ] + } + })json"; + auto json = grpc_core::Json::Parse(json_str); + ASSERT_TRUE(json.ok()) << json.status(); + grpc_core::ValidationErrors errors; + auto config = grpc_core::LoadFromJson( + *json, grpc_core::JsonArgs(), &errors); + EXPECT_THAT( + errors.status("Parsing error").ToString(), + ::testing::HasSubstr("field:cloud_logging.client_rpc_events[0].methods[0]" + " error:Empty configuration")); +} + +TEST(GcpObservabilityConfigJsonParsingTest, LoggingConfigWildcardEntries) { + const char* json_str = R"json({ + "cloud_logging": { + "client_rpc_events": [ + { + "methods": ["*", "service/*"] + } + ] + } + })json"; + auto json = grpc_core::Json::Parse(json_str); + ASSERT_TRUE(json.ok()) << json.status(); + grpc_core::ValidationErrors errors; + auto config = grpc_core::LoadFromJson( + *json, grpc_core::JsonArgs(), &errors); + ASSERT_TRUE(errors.ok()) << errors.status("unexpected errors"); + ASSERT_TRUE(config.cloud_logging.has_value()); + ASSERT_EQ(config.cloud_logging->client_rpc_events.size(), 1); + EXPECT_THAT(config.cloud_logging->client_rpc_events[0].qualified_methods, + ::testing::ElementsAre("*", "service/*")); +} + +TEST(GcpObservabilityConfigJsonParsingTest, + LoggingConfigIncorrectWildcardSpecs) { + const char* json_str = R"json({ + "cloud_logging": { + "client_rpc_events": [ + { + "methods": ["*"], + "exclude": true + }, + { + "methods": ["*/method", "service/*blah"], + "exclude": true + } + ] + } + })json"; + auto json = grpc_core::Json::Parse(json_str); + ASSERT_TRUE(json.ok()) << json.status(); + grpc_core::ValidationErrors errors; + auto config = grpc_core::LoadFromJson( + *json, grpc_core::JsonArgs(), &errors); + EXPECT_THAT( + errors.status("Parsing error").ToString(), + ::testing::AllOf( + ::testing::HasSubstr( + "field:cloud_logging.client_rpc_events[0].methods[0]" + " error:Wildcard match '*' not allowed when 'exclude' is set"), + ::testing::HasSubstr( + "field:cloud_logging.client_rpc_events[1].methods[0] " + "error:Configuration of type '*/method' not allowed"), + ::testing::HasSubstr( + "field:cloud_logging.client_rpc_events[1].methods[1] " + "error:Wildcard specified for method in incorrect manner"))); +} + TEST(GcpObservabilityConfigJsonParsingTest, SamplingRateDefaults) { const char* json_str = R"json({ "cloud_trace": {