From 3d63a7b1065da2e88202717bba0e48c7d3900f18 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Mon, 14 Oct 2024 15:59:25 -0700 Subject: [PATCH] [CSM O11y] Change how mesh_id is populated (#37801) Instead of getting value of `csm_mesh_id` from the bootstrap file, get it from the env var `CSM_MESH_ID` Closes #37801 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/37801 from yashykt:CsmMeshIdChange d0f149e02363350a3c0a43ddaf5cc872638b9067 PiperOrigin-RevId: 685864223 --- src/cpp/ext/csm/BUILD | 4 - src/cpp/ext/csm/metadata_exchange.cc | 84 +------------ src/cpp/ext/csm/metadata_exchange.h | 5 - test/cpp/ext/csm/BUILD | 16 --- test/cpp/ext/csm/csm_observability_test.cc | 2 +- test/cpp/ext/csm/mesh_id_test.cc | 136 --------------------- test/cpp/ext/csm/metadata_exchange_test.cc | 62 +--------- 7 files changed, 8 insertions(+), 301 deletions(-) delete mode 100644 test/cpp/ext/csm/mesh_id_test.cc diff --git a/src/cpp/ext/csm/BUILD b/src/cpp/ext/csm/BUILD index cfa4dd822cb..765d09a191d 100644 --- a/src/cpp/ext/csm/BUILD +++ b/src/cpp/ext/csm/BUILD @@ -66,10 +66,6 @@ grpc_cc_library( "//src/core:channel_args", "//src/core:env", "//src/core:error", - "//src/core:json_args", - "//src/core:json_object_loader", - "//src/core:json_reader", - "//src/core:load_file", "//src/core:metadata_batch", "//src/core:slice", "//src/core:xds_enabled_server", diff --git a/src/cpp/ext/csm/metadata_exchange.cc b/src/cpp/ext/csm/metadata_exchange.cc index 5934554d10d..d9c64e793f1 100644 --- a/src/cpp/ext/csm/metadata_exchange.cc +++ b/src/cpp/ext/csm/metadata_exchange.cc @@ -40,10 +40,6 @@ #include "src/core/lib/slice/slice_internal.h" #include "src/core/telemetry/call_tracer.h" #include "src/core/util/env.h" -#include "src/core/util/json/json_args.h" -#include "src/core/util/json/json_object_loader.h" -#include "src/core/util/json/json_reader.h" -#include "src/core/util/load_file.h" #include "src/cpp/ext/otel/key_value_iterable.h" #include "upb/base/string_view.h" @@ -106,58 +102,6 @@ google_protobuf_Struct* DecodeMetadata(grpc_core::Slice slice, return nullptr; } -// A minimal class for helping with the information we need from the xDS -// bootstrap file for GSM Observability reasons. -class XdsBootstrapForGSM { - public: - class Node { - public: - const std::string& id() const { return id_; } - - static const grpc_core::JsonLoaderInterface* JsonLoader( - const grpc_core::JsonArgs&) { - static const auto* loader = - grpc_core::JsonObjectLoader().Field("id", &Node::id_).Finish(); - return loader; - } - - private: - std::string id_; - }; - - const Node& node() const { return node_; } - - static const grpc_core::JsonLoaderInterface* JsonLoader( - const grpc_core::JsonArgs&) { - static const auto* loader = - grpc_core::JsonObjectLoader() - .Field("node", &XdsBootstrapForGSM::node_) - .Finish(); - return loader; - } - - private: - Node node_; -}; - -// Returns an empty string if no bootstrap config is found. -std::string GetXdsBootstrapContents() { - // First, try GRPC_XDS_BOOTSTRAP env var. - auto path = grpc_core::GetEnv("GRPC_XDS_BOOTSTRAP"); - if (path.has_value()) { - auto contents = grpc_core::LoadFile(*path, /*add_null_terminator=*/true); - if (!contents.ok()) return ""; - return std::string(contents->as_string_view()); - } - // Next, try GRPC_XDS_BOOTSTRAP_CONFIG env var. - auto env_config = grpc_core::GetEnv("GRPC_XDS_BOOTSTRAP_CONFIG"); - if (env_config.has_value()) { - return std::move(*env_config); - } - // No bootstrap config found. - return ""; -} - MeshLabelsIterable::GcpResourceType StringToGcpResourceType( absl::string_view type) { if (type == kGkeType) { @@ -308,31 +252,6 @@ size_t MeshLabelsIterable::Size() const { GetAttributesForType(remote_type_).size(); } -// Returns the mesh ID by reading and parsing the bootstrap file. Returns -// "unknown" if for some reason, mesh ID could not be figured out. -std::string GetMeshId() { - auto json = grpc_core::JsonParse(GetXdsBootstrapContents()); - if (!json.ok()) { - return "unknown"; - } - auto bootstrap = grpc_core::LoadFromJson(*json); - if (!bootstrap.ok()) { - return "unknown"; - } - // The format of the Node ID is - - // projects/[GCP Project number]/networks/mesh:[Mesh ID]/nodes/[UUID] - std::vector parts = - absl::StrSplit(bootstrap->node().id(), '/'); - if (parts.size() != 6) { - return "unknown"; - } - absl::string_view mesh_id = parts[3]; - if (!absl::ConsumePrefix(&mesh_id, "mesh:")) { - return "unknown"; - } - return std::string(mesh_id); -} - // // ServiceMeshLabelsInjector // @@ -397,7 +316,8 @@ ServiceMeshLabelsInjector::ServiceMeshLabelsInjector( // from the peer. local_labels_.emplace_back(kCanonicalServiceAttribute, canonical_service_value); - local_labels_.emplace_back(kMeshIdAttribute, GetMeshId()); + local_labels_.emplace_back( + kMeshIdAttribute, grpc_core::GetEnv("CSM_MESH_ID").value_or("unknown")); } std::unique_ptr ServiceMeshLabelsInjector::GetLabels( diff --git a/src/cpp/ext/csm/metadata_exchange.h b/src/cpp/ext/csm/metadata_exchange.h index 0830acdd7ef..1442a738392 100644 --- a/src/cpp/ext/csm/metadata_exchange.h +++ b/src/cpp/ext/csm/metadata_exchange.h @@ -112,11 +112,6 @@ class MeshLabelsIterable : public LabelsIterable { uint32_t pos_ = 0; }; -// Returns the mesh ID by reading and parsing the bootstrap file. Returns -// "unknown" if for some reason, mesh ID could not be figured out. -// EXPOSED FOR TESTING PURPOSES ONLY. -std::string GetMeshId(); - } // namespace internal } // namespace grpc diff --git a/test/cpp/ext/csm/BUILD b/test/cpp/ext/csm/BUILD index d8272519b0f..427a254a085 100644 --- a/test/cpp/ext/csm/BUILD +++ b/test/cpp/ext/csm/BUILD @@ -64,19 +64,3 @@ grpc_cc_test( "//test/cpp/ext/otel:otel_test_library", ], ) - -grpc_cc_test( - name = "mesh_id_test", - srcs = [ - "mesh_id_test.cc", - ], - external_deps = [ - "gtest", - ], - language = "C++", - deps = [ - "//:grpc++", - "//src/cpp/ext/csm:csm_observability", - "//test/core/test_util:grpc_test_util", - ], -) diff --git a/test/cpp/ext/csm/csm_observability_test.cc b/test/cpp/ext/csm/csm_observability_test.cc index c36b37efee1..ea113f5ca3e 100644 --- a/test/cpp/ext/csm/csm_observability_test.cc +++ b/test/cpp/ext/csm/csm_observability_test.cc @@ -41,7 +41,7 @@ TEST(CsmObservabilityBuilderTest, Basic) { absl::OkStatus()); } -TEST(GsmDependencyTest, GoogleCloudOpenTelemetryDependency) { +TEST(CsmDependencyTest, GoogleCloudOpenTelemetryDependency) { EXPECT_NE(google::cloud::otel::MakeResourceDetector(), nullptr); } diff --git a/test/cpp/ext/csm/mesh_id_test.cc b/test/cpp/ext/csm/mesh_id_test.cc deleted file mode 100644 index c18526dbf84..00000000000 --- a/test/cpp/ext/csm/mesh_id_test.cc +++ /dev/null @@ -1,136 +0,0 @@ -// -// -// Copyright 2023 gRPC authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// -// - -#include - -#include "gtest/gtest.h" -#include "src/core/util/env.h" -#include "src/core/util/tmpfile.h" -#include "src/cpp/ext/csm/metadata_exchange.h" -#include "test/core/test_util/test_config.h" - -namespace grpc { -namespace testing { -namespace { - -class TestScenario { - public: - enum class XdsBootstrapSource : std::uint8_t { kFromFile, kFromConfig }; - - explicit TestScenario(XdsBootstrapSource bootstrap_source) - : bootstrap_source_(bootstrap_source) {} - - static std::string Name(const ::testing::TestParamInfo& info) { - switch (info.param.bootstrap_source_) { - case TestScenario::XdsBootstrapSource::kFromFile: - return "BootstrapFromFile"; - case TestScenario::XdsBootstrapSource::kFromConfig: - return "BootstrapFromConfig"; - } - } - - XdsBootstrapSource bootstrap_source() const { return bootstrap_source_; } - - private: - XdsBootstrapSource bootstrap_source_; -}; - -class MeshIdTest : public ::testing::TestWithParam { - protected: - void SetBootstrap(const char* bootstrap) { - switch (GetParam().bootstrap_source()) { - case TestScenario::XdsBootstrapSource::kFromFile: { - ASSERT_EQ(bootstrap_file_name_, nullptr); - FILE* bootstrap_file = - gpr_tmpfile("gcp_observability_config", &bootstrap_file_name_); - fputs(bootstrap, bootstrap_file); - fclose(bootstrap_file); - grpc_core::SetEnv("GRPC_XDS_BOOTSTRAP", bootstrap_file_name_); - break; - } - case TestScenario::XdsBootstrapSource::kFromConfig: - grpc_core::SetEnv("GRPC_XDS_BOOTSTRAP_CONFIG", bootstrap); - break; - } - } - - ~MeshIdTest() override { - grpc_core::UnsetEnv("GRPC_GCP_OBSERVABILITY_CONFIG"); - grpc_core::UnsetEnv("GRPC_XDS_BOOTSTRAP"); - if (bootstrap_file_name_ != nullptr) { - remove(bootstrap_file_name_); - gpr_free(bootstrap_file_name_); - } - } - - char* bootstrap_file_name_ = nullptr; -}; - -TEST_P(MeshIdTest, Empty) { - SetBootstrap(""); - EXPECT_EQ(grpc::internal::GetMeshId(), "unknown"); -} - -TEST_P(MeshIdTest, NothingSet) { - EXPECT_EQ(grpc::internal::GetMeshId(), "unknown"); -} - -TEST_P(MeshIdTest, BadJson) { - SetBootstrap("{"); - EXPECT_EQ(grpc::internal::GetMeshId(), "unknown"); -} - -TEST_P(MeshIdTest, UnexpectedMeshIdFormatType1) { - SetBootstrap( - "{\"node\": {\"id\": " - "\"abcdef\"}}"); - EXPECT_EQ(grpc::internal::GetMeshId(), "unknown"); -} - -TEST_P(MeshIdTest, UnexpectedMeshIdFormatType2) { - SetBootstrap( - "{\"node\": {\"id\": " - "\"projects/1234567890/networks/mesh-id/nodes/" - "01234567-89ab-4def-8123-456789abcdef\"}}"); - EXPECT_EQ(grpc::internal::GetMeshId(), "unknown"); -} - -TEST_P(MeshIdTest, Basic) { - SetBootstrap( - "{\"node\": {\"id\": " - "\"projects/1234567890/networks/mesh:mesh-id/nodes/" - "01234567-89ab-4def-8123-456789abcdef\"}}"); - EXPECT_EQ(grpc::internal::GetMeshId(), "mesh-id"); -} - -INSTANTIATE_TEST_SUITE_P( - MeshId, MeshIdTest, - ::testing::Values( - TestScenario(TestScenario::XdsBootstrapSource::kFromFile), - TestScenario(TestScenario::XdsBootstrapSource::kFromConfig)), - &TestScenario::Name); - -} // namespace -} // namespace testing -} // namespace grpc - -int main(int argc, char** argv) { - grpc::testing::TestEnvironment env(&argc, argv); - ::testing::InitGoogleTest(&argc, argv); - return RUN_ALL_TESTS(); -} diff --git a/test/cpp/ext/csm/metadata_exchange_test.cc b/test/cpp/ext/csm/metadata_exchange_test.cc index 08fb8bd9823..969376f96fb 100644 --- a/test/cpp/ext/csm/metadata_exchange_test.cc +++ b/test/cpp/ext/csm/metadata_exchange_test.cc @@ -76,10 +76,8 @@ opentelemetry::sdk::resource::Resource TestUnknownResource() { class TestScenario { public: enum class ResourceType : std::uint8_t { kGke, kGce, kUnknown }; - enum class XdsBootstrapSource : std::uint8_t { kFromFile, kFromConfig }; - explicit TestScenario(ResourceType type, XdsBootstrapSource bootstrap_source) - : type_(type), bootstrap_source_(bootstrap_source) {} + explicit TestScenario(ResourceType type) : type_(type) {} opentelemetry::sdk::resource::Resource GetTestResource() const { switch (type_) { @@ -105,24 +103,13 @@ class TestScenario { ret_val += "Unknown"; break; } - switch (info.param.bootstrap_source_) { - case TestScenario::XdsBootstrapSource::kFromFile: - ret_val += "BootstrapFromFile"; - break; - case TestScenario::XdsBootstrapSource::kFromConfig: - ret_val += "BootstrapFromConfig"; - break; - } return ret_val; } ResourceType type() const { return type_; } - XdsBootstrapSource bootstrap_source() const { return bootstrap_source_; } - private: ResourceType type_; - XdsBootstrapSource bootstrap_source_; }; // A PluginOption that injects `ServiceMeshLabelsInjector`. (This is different @@ -157,24 +144,6 @@ class MetadataExchangeTest public ::testing::WithParamInterface { protected: void Init(Options options, bool enable_client_side_injector = true) { - const char* kBootstrap = - "{\"node\": {\"id\": " - "\"projects/1234567890/networks/mesh:mesh-id/nodes/" - "01234567-89ab-4def-8123-456789abcdef\"}}"; - switch (GetParam().bootstrap_source()) { - case TestScenario::XdsBootstrapSource::kFromFile: { - ASSERT_EQ(bootstrap_file_name_, nullptr); - FILE* bootstrap_file = - gpr_tmpfile("xds_bootstrap", &bootstrap_file_name_); - fputs(kBootstrap, bootstrap_file); - fclose(bootstrap_file); - grpc_core::SetEnv("GRPC_XDS_BOOTSTRAP", bootstrap_file_name_); - break; - } - case TestScenario::XdsBootstrapSource::kFromConfig: - grpc_core::SetEnv("GRPC_XDS_BOOTSTRAP_CONFIG", kBootstrap); - break; - } OpenTelemetryPluginEnd2EndTest::Init(std::move( options .add_plugin_option(std::make_unique( @@ -186,15 +155,6 @@ class MetadataExchangeTest }))); } - ~MetadataExchangeTest() override { - grpc_core::UnsetEnv("GRPC_XDS_BOOTSTRAP_CONFIG"); - grpc_core::UnsetEnv("GRPC_XDS_BOOTSTRAP"); - if (bootstrap_file_name_ != nullptr) { - remove(bootstrap_file_name_); - gpr_free(bootstrap_file_name_); - } - } - void VerifyServiceMeshAttributes( const std::map& @@ -269,9 +229,6 @@ class MetadataExchangeTest attributes) { EXPECT_EQ(attributes.find("csm.remote_workload_type"), attributes.end()); } - - private: - char* bootstrap_file_name_ = nullptr; }; // Verify that grpc.client.attempt.started does not get service mesh attributes @@ -603,19 +560,9 @@ TEST(MeshLabelsIterableTest, TestResetIteratorPosition) { INSTANTIATE_TEST_SUITE_P( MetadataExchange, MetadataExchangeTest, - ::testing::Values( - TestScenario(TestScenario::ResourceType::kGke, - TestScenario::XdsBootstrapSource::kFromConfig), - TestScenario(TestScenario::ResourceType::kGke, - TestScenario::XdsBootstrapSource::kFromFile), - TestScenario(TestScenario::ResourceType::kGce, - TestScenario::XdsBootstrapSource::kFromConfig), - TestScenario(TestScenario::ResourceType::kGce, - TestScenario::XdsBootstrapSource::kFromFile), - TestScenario(TestScenario::ResourceType::kUnknown, - TestScenario::XdsBootstrapSource::kFromConfig), - TestScenario(TestScenario::ResourceType::kUnknown, - TestScenario::XdsBootstrapSource::kFromFile)), + ::testing::Values(TestScenario(TestScenario::ResourceType::kGke), + TestScenario(TestScenario::ResourceType::kGce), + TestScenario(TestScenario::ResourceType::kUnknown)), &TestScenario::Name); } // namespace @@ -627,5 +574,6 @@ int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); grpc_core::SetEnv("CSM_WORKLOAD_NAME", "workload"); grpc_core::SetEnv("CSM_CANONICAL_SERVICE_NAME", "canonical_service"); + grpc_core::SetEnv("CSM_MESH_ID", "mesh-id"); return RUN_ALL_TESTS(); }