api: use traffic_direction over operation_name if specified (#7999)

Use the listener-level field for the tracing direction over the per-filter field. Unfortunately, the per filter did not provide an "unspecified" default, so this appears to be the right approach to deprecate the per-filter field with minimal impact.

Risk Level: low (uses a newly introduce field traffic_direction)
Testing: unit test
Docs Changes: proto docs

Signed-off-by: Kuat Yessenov <kuat@google.com>

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

@@ -79,7 +79,6 @@ message HttpConnectionManager {
   google.protobuf.BoolValue add_user_agent = 6;
 
   message Tracing {
-    // [#comment:TODO(kyessenov): Align this field with listener traffic direction field.]
     enum OperationName {
       option (gogoproto.goproto_enum_prefix) = false;
 
@@ -90,8 +89,13 @@ message HttpConnectionManager {
       EGRESS = 1;
     }
 
-    // The span name will be derived from this field.
-    OperationName operation_name = 1 [(validate.rules).enum.defined_only = true];
+    // The span name will be derived from this field. If
+    // :ref:`traffic_direction <envoy_api_field_Listener.traffic_direction>` is
+    // specified on the parent listener, then it is used instead of this field.
+    //
+    // .. attention::
+    //  This field has been deprecated in favor of `traffic_direction`.
+    OperationName operation_name = 1 [(validate.rules).enum.defined_only = true, deprecated = true];
 
     // A list of header names used to create tags for the active span. The header name is used to
     // populate the tag name, and the header value is used to populate the tag value. The tag is

docs/root/intro/deprecated.rst

@@ -34,6 +34,11 @@ Version 1.12.0 (pending)
 * The use of HTTP_JSON_V1 :ref:`Zipkin collector endpoint version
   <envoy_api_field_config.trace.v2.ZipkinConfig.collector_endpoint_version>` or not explicitly
   specifying it is deprecated, use HTTP_JSON or HTTP_PROTO instead.
+* The `operation_name` field in :ref:`HTTP connection manager
+  <envoy_api_msg_config.filter.network.http_connection_manager.v2.HttpConnectionManager>`
+  has been deprecated in favor of the `traffic_direction` field in
+  :ref:`Listener <envoy_api_msg_Listener>`. The latter takes priority if
+  specified.
 
 Version 1.11.0 (July 11, 2019)
 ==============================

source/extensions/filters/network/http_connection_manager/config.cc

@@ -244,13 +244,27 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
     Tracing::OperationName tracing_operation_name;
     std::vector<Http::LowerCaseString> request_headers_for_tags;
 
-    switch (tracing_config.operation_name()) {
-    case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager::
-        Tracing::INGRESS:
+    // Listener level traffic direction overrides the operation name
+    switch (context.direction()) {
+    case envoy::api::v2::core::TrafficDirection::UNSPECIFIED: {
+      switch (tracing_config.operation_name()) {
+      case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager::
+          Tracing::INGRESS:
+        tracing_operation_name = Tracing::OperationName::Ingress;
+        break;
+      case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager::
+          Tracing::EGRESS:
+        tracing_operation_name = Tracing::OperationName::Egress;
+        break;
+      default:
+        NOT_REACHED_GCOVR_EXCL_LINE;
+      }
+      break;
+    }
+    case envoy::api::v2::core::TrafficDirection::INBOUND:
       tracing_operation_name = Tracing::OperationName::Ingress;
       break;
-    case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager::
-        Tracing::EGRESS:
+    case envoy::api::v2::core::TrafficDirection::OUTBOUND:
       tracing_operation_name = Tracing::OperationName::Egress;
       break;
     default:

test/extensions/filters/network/http_connection_manager/config_test.cc

@@ -242,6 +242,62 @@ stat_prefix: router
   EXPECT_EQ(5 * 60 * 1000, config.streamIdleTimeout().count());
 }
 
+TEST_F(HttpConnectionManagerConfigTest, ListenerDirectionOutboundOverride) {
+  const std::string yaml_string = R"EOF(
+stat_prefix: router
+route_config:
+  virtual_hosts:
+  - name: service
+    domains:
+    - "*"
+    routes:
+    - match:
+        prefix: "/"
+      route:
+        cluster: cluster
+tracing:
+  operation_name: ingress
+http_filters:
+- name: envoy.router
+  config: {}
+  )EOF";
+
+  ON_CALL(context_, direction())
+      .WillByDefault(Return(envoy::api::v2::core::TrafficDirection::OUTBOUND));
+  HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_,
+                                     date_provider_, route_config_provider_manager_,
+                                     scoped_routes_config_provider_manager_);
+  EXPECT_EQ(Tracing::OperationName::Egress, config.tracingConfig()->operation_name_);
+}
+
+TEST_F(HttpConnectionManagerConfigTest, ListenerDirectionInboundOverride) {
+  const std::string yaml_string = R"EOF(
+stat_prefix: router
+route_config:
+  virtual_hosts:
+  - name: service
+    domains:
+    - "*"
+    routes:
+    - match:
+        prefix: "/"
+      route:
+        cluster: cluster
+tracing:
+  operation_name: egress
+http_filters:
+- name: envoy.router
+  config: {}
+  )EOF";
+
+  ON_CALL(context_, direction())
+      .WillByDefault(Return(envoy::api::v2::core::TrafficDirection::INBOUND));
+  HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_,
+                                     date_provider_, route_config_provider_manager_,
+                                     scoped_routes_config_provider_manager_);
+  EXPECT_EQ(Tracing::OperationName::Ingress, config.tracingConfig()->operation_name_);
+}
+
 TEST_F(HttpConnectionManagerConfigTest, SamplingDefault) {
   const std::string yaml_string = R"EOF(
   stat_prefix: ingress_http