From b788e1a92347d6f050042cbf2356119cc75105de Mon Sep 17 00:00:00 2001 From: Joao Grassi Date: Thu, 8 Feb 2024 06:51:17 +0100 Subject: [PATCH] opentelemetry tracer: Not crash envoy when environment resource detector can't detect attributes (#32229) * Not crash envoy when env resource detector can't detect attributes Signed-off-by: Joao Grassi * Add log warning for invalid env variable contents Signed-off-by: Joao Grassi --------- Signed-off-by: Joao Grassi --- changelogs/current.yaml | 3 + .../environment_resource_detector.cc | 18 ++--- .../resource_detectors/environment/BUILD | 16 ++++ ...ment_resource_detector_integration_test.cc | 76 +++++++++++++++++++ .../environment_resource_detector_test.cc | 27 +++---- 5 files changed, 116 insertions(+), 24 deletions(-) create mode 100644 test/extensions/tracers/opentelemetry/resource_detectors/environment/environment_resource_detector_integration_test.cc diff --git a/changelogs/current.yaml b/changelogs/current.yaml index ef621bf7ea88..e08127197d53 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -40,6 +40,9 @@ bug_fixes: fixed a bug where second HTTP response headers received would cause Envoy to crash in cases where ``propagate_response_headers`` and retry configurations are enabled at the same time, and an upstream request is retried multiple times. +- area: tracing + change: | + Prevent Envoy from crashing at start up when the OpenTelemetry environment resource detector cannot detect any attributes. removed_config_or_runtime: # *Normally occurs at the end of the* :ref:`deprecation period ` diff --git a/source/extensions/tracers/opentelemetry/resource_detectors/environment/environment_resource_detector.cc b/source/extensions/tracers/opentelemetry/resource_detectors/environment/environment_resource_detector.cc index 7cf34bb08cde..0a5592df94f6 100644 --- a/source/extensions/tracers/opentelemetry/resource_detectors/environment/environment_resource_detector.cc +++ b/source/extensions/tracers/opentelemetry/resource_detectors/environment/environment_resource_detector.cc @@ -29,22 +29,22 @@ Resource EnvironmentResourceDetector::detect() { resource.schema_url_ = ""; std::string attributes_str = ""; - attributes_str = Config::DataSource::read(ds, true, context_.serverFactoryContext().api()); + TRY_NEEDS_AUDIT { + attributes_str = Config::DataSource::read(ds, true, context_.serverFactoryContext().api()); + } + END_TRY catch (const EnvoyException& e) { + ENVOY_LOG(warn, "Failed to detect resource attributes from the environment: {}.", e.what()); + } if (attributes_str.empty()) { - throw EnvoyException( - fmt::format("The OpenTelemetry environment resource detector is configured but the '{}'" - " environment variable is empty.", - kOtelResourceAttributesEnv)); + return resource; } for (const auto& pair : StringUtil::splitToken(attributes_str, ",")) { const auto keyValue = StringUtil::splitToken(pair, "="); if (keyValue.size() != 2) { - throw EnvoyException( - fmt::format("The OpenTelemetry environment resource detector is configured but the '{}'" - " environment variable has an invalid format.", - kOtelResourceAttributesEnv)); + ENVOY_LOG(warn, "Invalid resource format from the environment, invalid text: {}.", pair); + continue; } const std::string key = std::string(keyValue[0]); diff --git a/test/extensions/tracers/opentelemetry/resource_detectors/environment/BUILD b/test/extensions/tracers/opentelemetry/resource_detectors/environment/BUILD index 2e6598200c38..3bfb2f65f15a 100644 --- a/test/extensions/tracers/opentelemetry/resource_detectors/environment/BUILD +++ b/test/extensions/tracers/opentelemetry/resource_detectors/environment/BUILD @@ -35,3 +35,19 @@ envoy_extension_cc_test( "@envoy_api//envoy/extensions/tracers/opentelemetry/resource_detectors/v3:pkg_cc_proto", ], ) + +envoy_extension_cc_test( + name = "environment_resource_detector_integration_test", + srcs = [ + "environment_resource_detector_integration_test.cc", + ], + extension_names = ["envoy.tracers.opentelemetry.resource_detectors.environment"], + deps = [ + "//source/exe:main_common_lib", + "//source/extensions/tracers/opentelemetry:config", + "//source/extensions/tracers/opentelemetry/resource_detectors/environment:config", + "//test/integration:http_integration_lib", + "//test/test_common:utility_lib", + "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto", + ], +) diff --git a/test/extensions/tracers/opentelemetry/resource_detectors/environment/environment_resource_detector_integration_test.cc b/test/extensions/tracers/opentelemetry/resource_detectors/environment/environment_resource_detector_integration_test.cc new file mode 100644 index 000000000000..607687257925 --- /dev/null +++ b/test/extensions/tracers/opentelemetry/resource_detectors/environment/environment_resource_detector_integration_test.cc @@ -0,0 +1,76 @@ +#include +#include + +#include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h" + +#include "test/integration/http_integration.h" +#include "test/test_common/utility.h" + +#include "absl/strings/match.h" +#include "absl/strings/string_view.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { +namespace { + +class EnvironmentResourceDetectorIntegrationTest + : public Envoy::HttpIntegrationTest, + public testing::TestWithParam { +public: + EnvironmentResourceDetectorIntegrationTest() + : HttpIntegrationTest(Http::CodecType::HTTP1, GetParam()) { + + const std::string yaml_string = R"EOF( + provider: + name: envoy.tracers.opentelemetry + typed_config: + "@type": type.googleapis.com/envoy.config.trace.v3.OpenTelemetryConfig + grpc_service: + envoy_grpc: + cluster_name: opentelemetry_collector + timeout: 0.250s + service_name: "a_service_name" + resource_detectors: + - name: envoy.tracers.opentelemetry.resource_detectors.environment + typed_config: + "@type": type.googleapis.com/envoy.extensions.tracers.opentelemetry.resource_detectors.v3.EnvironmentResourceDetectorConfig + )EOF"; + + auto tracing_config = + std::make_unique<::envoy::extensions::filters::network::http_connection_manager::v3:: + HttpConnectionManager_Tracing>(); + TestUtility::loadFromYaml(yaml_string, *tracing_config.get()); + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { hcm.set_allocated_tracing(tracing_config.release()); }); + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + } +}; + +INSTANTIATE_TEST_SUITE_P(IpVersions, EnvironmentResourceDetectorIntegrationTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + TestUtility::ipTestParamsToString); + +// Verify Envoy starts even when the Environment resource detector wasn't able to detect any +// attributes (env variable OTEL_RESOURCE_ATTRIBUTES not set) +TEST_P(EnvironmentResourceDetectorIntegrationTest, TestWithNoEnvVariableSet) { + Http::TestRequestHeaderMapImpl request_headers{ + {":method", "GET"}, {":path", "/test/long/url"}, {":scheme", "http"}, {":authority", "host"}}; + + auto response = sendRequestAndWaitForResponse(request_headers, 0, default_response_headers_, 0); + + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_TRUE(response->complete()); + EXPECT_EQ(response->headers().getStatusValue(), "200"); +} + +} // namespace +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/tracers/opentelemetry/resource_detectors/environment/environment_resource_detector_test.cc b/test/extensions/tracers/opentelemetry/resource_detectors/environment/environment_resource_detector_test.cc index 3a596f3d0e22..cc62387f4a98 100644 --- a/test/extensions/tracers/opentelemetry/resource_detectors/environment/environment_resource_detector_test.cc +++ b/test/extensions/tracers/opentelemetry/resource_detectors/environment/environment_resource_detector_test.cc @@ -29,8 +29,10 @@ TEST(EnvironmentResourceDetectorTest, EnvVariableNotPresent) { EnvironmentResourceDetectorConfig config; auto detector = std::make_unique(config, context); - EXPECT_THROW_WITH_MESSAGE(detector->detect(), EnvoyException, - "Environment variable doesn't exist: OTEL_RESOURCE_ATTRIBUTES"); + Resource resource = detector->detect(); + + EXPECT_EQ(resource.schema_url_, ""); + EXPECT_TRUE(resource.attributes_.empty()); } // Test detector when env variable is present but contains an empty value @@ -43,16 +45,10 @@ TEST(EnvironmentResourceDetectorTest, EnvVariablePresentButEmpty) { EnvironmentResourceDetectorConfig config; auto detector = std::make_unique(config, context); + Resource resource = detector->detect(); -#ifdef WIN32 - EXPECT_THROW_WITH_MESSAGE(detector->detect(), EnvoyException, - "Environment variable doesn't exist: OTEL_RESOURCE_ATTRIBUTES"); -#else - EXPECT_THROW_WITH_MESSAGE(detector->detect(), EnvoyException, - "The OpenTelemetry environment resource detector is configured but the " - "'OTEL_RESOURCE_ATTRIBUTES'" - " environment variable is empty."); -#endif + EXPECT_EQ(resource.schema_url_, ""); + EXPECT_TRUE(resource.attributes_.empty()); } // Test detector with valid values in the env variable @@ -96,11 +92,12 @@ TEST(EnvironmentResourceDetectorTest, EnvVariablePresentAndWithAttributesWrongFo EnvironmentResourceDetectorConfig config; auto detector = std::make_unique(config, context); + Resource resource = detector->detect(); - EXPECT_THROW_WITH_MESSAGE(detector->detect(), EnvoyException, - "The OpenTelemetry environment resource detector is configured but the " - "'OTEL_RESOURCE_ATTRIBUTES'" - " environment variable has an invalid format."); + EXPECT_EQ(resource.schema_url_, ""); + EXPECT_EQ(1, resource.attributes_.size()); + EXPECT_EQ(resource.attributes_.begin()->first, resource.attributes_.begin()->first); + EXPECT_EQ(resource.attributes_.begin()->second, resource.attributes_.begin()->second); } } // namespace OpenTelemetry