test: fixing grpc access_logger test visibility (envoyproxy#19267)

part of envoyproxy#9953 Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Josh Perry <josh.perry@mx.com>
joshperry · Feb 13, 2022 · 05c3ddc · 05c3ddc
1 parent 0ac553a
commit 05c3ddc
Show file tree

Hide file tree

Showing 6 changed files with 124 additions and 135 deletions.
diff --git a/source/extensions/access_loggers/grpc/BUILD b/source/extensions/access_loggers/grpc/BUILD
@@ -97,11 +97,6 @@ envoy_cc_extension(
     name = "http_config",
     srcs = ["http_config.cc"],
     hdrs = ["http_config.h"],
-    # TODO(#9953) clean up.
-    extra_visibility = [
-        "//test/common/access_log:__subpackages__",
-        "//test/integration:__subpackages__",
-    ],
     deps = [
         ":config_utils",
         "//envoy/server:access_log_config_interface",
@@ -118,11 +113,6 @@ envoy_cc_extension(
     name = "tcp_config",
     srcs = ["tcp_config.cc"],
     hdrs = ["tcp_config.h"],
-    # TODO(#9953) clean up.
-    extra_visibility = [
-        "//test/common/access_log:__subpackages__",
-        "//test/integration:__subpackages__",
-    ],
     deps = [
         ":config_utils",
         "//envoy/server:access_log_config_interface",

diff --git a/test/common/access_log/BUILD b/test/common/access_log/BUILD
@@ -22,8 +22,6 @@ envoy_cc_test(
         "//source/common/stream_info:utility_lib",
         "//source/extensions/access_loggers/file:config",
         "//source/extensions/access_loggers/filters/cel:config",
-        "//source/extensions/access_loggers/grpc:http_config",
-        "//source/extensions/access_loggers/grpc:tcp_config",
         "//source/extensions/access_loggers/stream:config",
         "//test/common/stream_info:test_util",
         "//test/common/upstream:utility_lib",

diff --git a/test/extensions/access_loggers/grpc/http_grpc_access_log_integration_test.cc b/test/extensions/access_loggers/grpc/http_grpc_access_log_integration_test.cc
@@ -15,6 +15,7 @@
 #include "gtest/gtest.h"
 
 using testing::AssertionResult;
+using testing::HasSubstr;
 
 namespace Envoy {
 namespace {
@@ -222,5 +223,128 @@ TEST_P(AccessLogIntegrationTest, BasicAccessLogFlow) {
   cleanup();
 }
 
+// Regression test to make sure that configuring upstream logs over gRPC will not crash Envoy.
+// TODO(asraa): Test output of the upstream logs.
+// See https://github.com/envoyproxy/envoy/issues/8828.
+TEST_P(AccessLogIntegrationTest, ConfigureHttpOverGrpcLogs) {
+  setUpstreamProtocol(Http::CodecType::HTTP2);
+  setDownstreamProtocol(Http::CodecType::HTTP2);
+  config_helper_.addConfigModifier(
+      [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
+              hcm) -> void {
+        // Configure just enough of an upstream access log to reference the upstream headers.
+        const std::string yaml_string = R"EOF(
+name: router
+typed_config:
+  "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router
+  upstream_log:
+    name: grpc_accesslog
+    filter:
+      not_health_check_filter: {}
+    typed_config:
+      "@type": type.googleapis.com/envoy.extensions.access_loggers.grpc.v3.HttpGrpcAccessLogConfig
+      common_config:
+        log_name: foo
+        transport_api_version: V3
+        grpc_service:
+          envoy_grpc:
+            cluster_name: cluster_0
+  )EOF";
+        // Replace the terminal envoy.router.
+        hcm.clear_http_filters();
+        TestUtility::loadFromYaml(yaml_string, *hcm.add_http_filters());
+      });
+
+  initialize();
+
+  // Send the request.
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+  auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
+  waitForNextUpstreamRequest();
+
+  // Send the response headers.
+  upstream_request_->encodeHeaders(default_response_headers_, true);
+  ASSERT_TRUE(response->waitForEndStream());
+  EXPECT_EQ("200", response->headers().getStatusValue());
+}
+
+// Verify the grpc cached logger is available after the initial logger filter is destroyed.
+// Regression test for https://github.com/envoyproxy/envoy/issues/18066
+TEST_P(AccessLogIntegrationTest, GrpcLoggerSurvivesAfterReloadConfig) {
+  autonomous_upstream_ = true;
+  // The grpc access logger connection never closes. It's ok to see an incomplete logging stream.
+  autonomous_allow_incomplete_streams_ = true;
+
+  const std::string grpc_logger_string = R"EOF(
+    name: grpc_accesslog
+    typed_config:
+      "@type": type.googleapis.com/envoy.extensions.access_loggers.grpc.v3.HttpGrpcAccessLogConfig
+      common_config:
+        log_name: bar
+        transport_api_version: V3
+        grpc_service:
+          envoy_grpc:
+            cluster_name: cluster_0
+  )EOF";
+
+  config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
+    auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0);
+    listener->set_stat_prefix("listener_0");
+  });
+  config_helper_.addConfigModifier(
+      [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
+              hcm) { TestUtility::loadFromYaml(grpc_logger_string, *hcm.add_access_log()); });
+  initialize();
+  // Given we're using LDS in this test, initialize() will not complete until
+  // the initial LDS file has loaded.
+  EXPECT_EQ(1, test_server_->counter("listener_manager.lds.update_success")->value());
+
+  // HTTP 1.1 is allowed and the connection is kept open until the listener update.
+  std::string response;
+  auto connection =
+      createConnectionDriver(lookupPort("http"), "GET / HTTP/1.1\r\nHost: host\r\n\r\n",
+                             [&response, &dispatcher = *dispatcher_](
+                                 Network::ClientConnection&, const Buffer::Instance& data) -> void {
+                               response.append(data.toString());
+                               if (response.find("\r\n\r\n") != std::string::npos) {
+                                 dispatcher.exit();
+                               }
+                             });
+  connection->run();
+  EXPECT_TRUE(response.find("HTTP/1.1 200") == 0);
+
+  test_server_->waitForCounterEq("access_logs.grpc_access_log.logs_written", 2);
+
+  // Create a new config with HTTP/1.0 proxying. The goal is to trigger a listener update.
+  ConfigHelper new_config_helper(
+      version_, *api_, MessageUtil::getJsonStringFromMessageOrDie(config_helper_.bootstrap()));
+  new_config_helper.addConfigModifier(
+      [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
+              hcm) {
+        hcm.mutable_http_protocol_options()->set_accept_http_10(true);
+        hcm.mutable_http_protocol_options()->set_default_host_for_http_10("default.com");
+      });
+
+  // Create an LDS response with the new config, and reload config.
+  new_config_helper.setLds("1");
+  test_server_->waitForCounterGe("listener_manager.listener_in_place_updated", 1);
+  test_server_->waitForCounterEq("listener_manager.lds.update_success", 2);
+
+  // Wait until the http 1.1 connection is destroyed due to the listener update. It indicates the
+  // listener starts draining.
+  test_server_->waitForGaugeEq("listener.listener_0.downstream_cx_active", 0);
+  // Wait until all the draining filter chain is gone. It indicates the old listener and filter
+  // chains are destroyed.
+  test_server_->waitForGaugeEq("listener_manager.total_filter_chains_draining", 0);
+
+  // Verify that the new listener config is applied.
+  std::string response2;
+  sendRawHttpAndWaitForResponse(lookupPort("http"), "GET / HTTP/1.0\r\n\r\n", &response2, true);
+  EXPECT_THAT(response2, HasSubstr("HTTP/1.0 200 OK\r\n"));
+
+  // Verify that the grpc access logger is available after the listener update.
+  test_server_->waitForCounterEq("access_logs.grpc_access_log.logs_written", 4);
+}
+
 } // namespace
 } // namespace Envoy
diff --git a/test/integration/BUILD b/test/integration/BUILD
@@ -544,7 +544,6 @@ envoy_cc_test(
     deps = [
         ":http_protocol_integration_lib",
         "//source/common/http:header_map_lib",
-        "//source/extensions/access_loggers/grpc:http_config",
         "//source/extensions/filters/http/buffer:config",
         "//test/integration/filters:encoder_decoder_buffer_filter_lib",
         "//test/integration/filters:random_pause_filter_lib",
@@ -1413,7 +1412,6 @@ envoy_cc_test(
     deps = [
         ":http_integration_lib",
         ":http_protocol_integration_lib",
-        "//source/extensions/access_loggers/grpc:http_config",
         "//source/extensions/filters/listener/tls_inspector:config",
         "//source/extensions/filters/listener/tls_inspector:tls_inspector_lib",
         "//source/extensions/filters/network/tcp_proxy:config",

diff --git a/test/integration/multiplexed_upstream_integration_test.cc b/test/integration/multiplexed_upstream_integration_test.cc
@@ -548,49 +548,6 @@ TEST_P(MultiplexedUpstreamIntegrationTest, LargeResponseHeadersRejected) {
   EXPECT_EQ("503", response->headers().getStatusValue());
 }
 
-// Regression test to make sure that configuring upstream logs over gRPC will not crash Envoy.
-// TODO(asraa): Test output of the upstream logs.
-// See https://github.com/envoyproxy/envoy/issues/8828.
-TEST_P(MultiplexedUpstreamIntegrationTest, ConfigureHttpOverGrpcLogs) {
-  config_helper_.addConfigModifier(
-      [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
-              hcm) -> void {
-        // Configure just enough of an upstream access log to reference the upstream headers.
-        const std::string yaml_string = R"EOF(
-name: router
-typed_config:
-  "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router
-  upstream_log:
-    name: grpc_accesslog
-    filter:
-      not_health_check_filter: {}
-    typed_config:
-      "@type": type.googleapis.com/envoy.extensions.access_loggers.grpc.v3.HttpGrpcAccessLogConfig
-      common_config:
-        log_name: foo
-        transport_api_version: V3
-        grpc_service:
-          envoy_grpc:
-            cluster_name: cluster_0
-  )EOF";
-        // Replace the terminal envoy.router.
-        hcm.clear_http_filters();
-        TestUtility::loadFromYaml(yaml_string, *hcm.add_http_filters());
-      });
-
-  initialize();
-
-  // Send the request.
-  codec_client_ = makeHttpConnection(lookupPort("http"));
-  auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
-  waitForNextUpstreamRequest();
-
-  // Send the response headers.
-  upstream_request_->encodeHeaders(default_response_headers_, true);
-  ASSERT_TRUE(response->waitForEndStream());
-  EXPECT_EQ("200", response->headers().getStatusValue());
-}
-
 // Regression test for https://github.com/envoyproxy/envoy/issues/13933
 TEST_P(MultiplexedUpstreamIntegrationTest, MultipleRequestsLowStreamLimit) {
   autonomous_upstream_ = true;

diff --git a/test/integration/xds_integration_test.cc b/test/integration/xds_integration_test.cc
@@ -610,84 +610,6 @@ TEST_P(LdsIntegrationTest, NewListenerWithBadPostListenSocketOption) {
   test_server_->waitForCounterGe("listener_manager.listener_create_failure", 1);
 }
 
-// Verify the grpc cached logger is available after the initial logger filter is destroyed.
-// Regression test for https://github.com/envoyproxy/envoy/issues/18066
-TEST_P(LdsIntegrationTest, GrpcLoggerSurvivesAfterReloadConfig) {
-  autonomous_upstream_ = true;
-  // The grpc access logger connection never closes. It's ok to see an incomplete logging stream.
-  autonomous_allow_incomplete_streams_ = true;
-
-  const std::string grpc_logger_string = R"EOF(
-    name: grpc_accesslog
-    typed_config:
-      "@type": type.googleapis.com/envoy.extensions.access_loggers.grpc.v3.HttpGrpcAccessLogConfig
-      common_config:
-        log_name: bar
-        transport_api_version: V3
-        grpc_service:
-          envoy_grpc:
-            cluster_name: cluster_0
-  )EOF";
-
-  config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
-    auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0);
-    listener->set_stat_prefix("listener_0");
-  });
-  config_helper_.addConfigModifier(
-      [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
-              hcm) { TestUtility::loadFromYaml(grpc_logger_string, *hcm.add_access_log()); });
-  initialize();
-  // Given we're using LDS in this test, initialize() will not complete until
-  // the initial LDS file has loaded.
-  EXPECT_EQ(1, test_server_->counter("listener_manager.lds.update_success")->value());
-
-  // HTTP 1.1 is allowed and the connection is kept open until the listener update.
-  std::string response;
-  auto connection =
-      createConnectionDriver(lookupPort("http"), "GET / HTTP/1.1\r\nHost: host\r\n\r\n",
-                             [&response, &dispatcher = *dispatcher_](
-                                 Network::ClientConnection&, const Buffer::Instance& data) -> void {
-                               response.append(data.toString());
-                               if (response.find("\r\n\r\n") != std::string::npos) {
-                                 dispatcher.exit();
-                               }
-                             });
-  connection->run();
-  EXPECT_TRUE(response.find("HTTP/1.1 200") == 0);
-
-  test_server_->waitForCounterEq("access_logs.grpc_access_log.logs_written", 1);
-
-  // Create a new config with HTTP/1.0 proxying. The goal is to trigger a listener update.
-  ConfigHelper new_config_helper(
-      version_, *api_, MessageUtil::getJsonStringFromMessageOrDie(config_helper_.bootstrap()));
-  new_config_helper.addConfigModifier(
-      [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
-              hcm) {
-        hcm.mutable_http_protocol_options()->set_accept_http_10(true);
-        hcm.mutable_http_protocol_options()->set_default_host_for_http_10("default.com");
-      });
-
-  // Create an LDS response with the new config, and reload config.
-  new_config_helper.setLds("1");
-  test_server_->waitForCounterGe("listener_manager.listener_in_place_updated", 1);
-  test_server_->waitForCounterEq("listener_manager.lds.update_success", 2);
-
-  // Wait until the http 1.1 connection is destroyed due to the listener update. It indicates the
-  // listener starts draining.
-  test_server_->waitForGaugeEq("listener.listener_0.downstream_cx_active", 0);
-  // Wait until all the draining filter chain is gone. It indicates the old listener and filter
-  // chains are destroyed.
-  test_server_->waitForGaugeEq("listener_manager.total_filter_chains_draining", 0);
-
-  // Verify that the new listener config is applied.
-  std::string response2;
-  sendRawHttpAndWaitForResponse(lookupPort("http"), "GET / HTTP/1.0\r\n\r\n", &response2, true);
-  EXPECT_THAT(response2, HasSubstr("HTTP/1.0 200 OK\r\n"));
-
-  // Verify that the grpc access logger is available after the listener update.
-  test_server_->waitForCounterEq("access_logs.grpc_access_log.logs_written", 2);
-}
-
 // Sample test making sure our config framework informs on listener failure.
 TEST_P(LdsIntegrationTest, FailConfigLoad) {
   config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void {