envoyproxy · mattklein123 · Nov 30, 2021 · Nov 9, 2021 · Nov 9, 2021 · Nov 10, 2021
diff --git a/docs/root/configuration/observability/access_log/usage.rst b/docs/root/configuration/observability/access_log/usage.rst
@@ -384,6 +384,13 @@ The following command operators are supported:
 %ROUTE_NAME%
   Name of the route.
 
+%VIRTUAL_CLUSTER_NAME%
+  HTTP*/gRPC
+    Name of the matched Virtual Cluster (if any).
+
+  TCP/UDP
+    Not implemented ("-")
+
 %UPSTREAM_HOST%
   Upstream host URL (e.g., tcp://ip:port for TCP connections).
 

diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -54,6 +54,7 @@ Removed Config or Runtime
 New Features
 ------------
 * access log: added :ref:`grpc_stream_retry_policy <envoy_v3_api_field_extensions.access_loggers.grpc.v3.CommonGrpcAccessLogConfig.grpc_stream_retry_policy>` to the gRPC logger to reconnect when a connection fails to be established.
+* access_log: added new access_log command operator ``%VIRTUAL_CLUSTER_NAME%`` to retrieve the matched Virtual Cluster name.
 * api: added support for *xds.type.v3.TypedStruct* in addition to the now-deprecated *udpa.type.v1.TypedStruct* proto message, which is a wrapper proto used to encode typed JSON data in a *google.protobuf.Any* field.
 * aws_request_signing_filter: added :ref:`match_excluded_headers <envoy_v3_api_field_extensions.filters.http.aws_request_signing.v3.AwsRequestSigning.match_excluded_headers>` to the signing filter to optionally exclude request headers from signing.
 * bootstrap: added :ref:`typed_dns_resolver_config <envoy_v3_api_field_config.bootstrap.v3.Bootstrap.typed_dns_resolver_config>` in the bootstrap to support DNS resolver as an extension.

diff --git a/envoy/router/router.h b/envoy/router/router.h
@@ -501,6 +501,11 @@ class VirtualCluster {
 public:
   virtual ~VirtualCluster() = default;
 
+  /**
+   * @return the string name of the virtual cluster.
+   */
+  virtual const absl::optional<std::string>& name() const PURE;
+
   /**
    * @return the stat-name of the virtual cluster.
    */

diff --git a/envoy/stream_info/stream_info.h b/envoy/stream_info/stream_info.h
@@ -466,6 +466,16 @@ class StreamInfo {
    */
   virtual const std::string& getRouteName() const PURE;
 
+  /**
+   * @param std::string name denotes the name of the virtual cluster.
+   */
+  virtual void setVirtualClusterName(const absl::optional<std::string>& name) PURE;
+
+  /**
+   * @return std::string& the name of the virtual cluster which got matched.
+   */
+  virtual const absl::optional<std::string>& virtualClusterName() const PURE;
+
   /**
    * @param bytes_received denotes number of bytes to add to total received bytes.
    */

diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc
@@ -686,6 +686,7 @@ class StreamInfoSslConnectionInfoFieldExtractor : public StreamInfoFormatter::Fi
 };
 
 StreamInfoFormatter::StreamInfoFormatter(const std::string& field_name) {
+  // TODO: Change this huge if-else ladder to use a switch case instead.
   if (field_name == "REQUEST_DURATION") {
     field_extractor_ = std::make_unique<StreamInfoDurationFieldExtractor>(
         [](const StreamInfo::StreamInfo& stream_info) {
@@ -953,6 +954,11 @@ StreamInfoFormatter::StreamInfoFormatter(const std::string& field_name) {
           }
           return absl::nullopt;
         });
+  } else if (field_name == "VIRTUAL_CLUSTER_NAME") {
+    field_extractor_ = std::make_unique<StreamInfoStringFieldExtractor>(
+        [](const StreamInfo::StreamInfo& stream_info) -> absl::optional<std::string> {
+          return stream_info.virtualClusterName();
+        });
   } else if (field_name == "TLS_JA3_FINGERPRINT") {
     field_extractor_ = std::make_unique<StreamInfoStringFieldExtractor>(
         [](const StreamInfo::StreamInfo& stream_info) {

diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc
@@ -1430,7 +1430,7 @@ VirtualHostImpl::VirtualClusterEntry::VirtualClusterEntry(
     const envoy::config::route::v3::VirtualCluster& virtual_cluster, Stats::Scope& scope,
     const VirtualClusterStatNames& stat_names)
     : StatNameProvider(virtual_cluster.name(), scope.symbolTable()),
-      VirtualClusterBase(stat_name_storage_.statName(),
+      VirtualClusterBase(virtual_cluster.name(), stat_name_storage_.statName(),
                          scope.scopeFromStatName(stat_name_storage_.statName()), stat_names) {
   if (virtual_cluster.headers().empty()) {
     throw EnvoyException("virtual clusters must define 'headers'");

diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h
@@ -236,16 +236,20 @@ class VirtualHostImpl : public VirtualHost, Logger::Loggable<Logger::Id::router>
 
   struct VirtualClusterBase : public VirtualCluster {
   public:
-    VirtualClusterBase(Stats::StatName stat_name, Stats::ScopePtr&& scope,
-                       const VirtualClusterStatNames& stat_names)
-        : stat_name_(stat_name), scope_(std::move(scope)),
+    VirtualClusterBase(const absl::optional<std::string>& name, Stats::StatName stat_name,
+                       Stats::ScopePtr&& scope, const VirtualClusterStatNames& stat_names)
+        : name_(name), stat_name_(stat_name), scope_(std::move(scope)),
           stats_(generateStats(*scope_, stat_names)) {}
 
     // Router::VirtualCluster
+    // name_ and stat_name_ are two different representations for the same string, retained in
+    // memory to avoid symbol-table locks that would be needed when converting on-the-fly.
+    const absl::optional<std::string>& name() const override { return name_; }
     Stats::StatName statName() const override { return stat_name_; }
     VirtualClusterStats& stats() const override { return stats_; }
 
   private:
+    const absl::optional<std::string> name_;
     const Stats::StatName stat_name_;
     Stats::ScopePtr scope_;
     mutable VirtualClusterStats stats_;
@@ -259,8 +263,8 @@ class VirtualHostImpl : public VirtualHost, Logger::Loggable<Logger::Id::router>
 
   struct CatchAllVirtualCluster : public VirtualClusterBase {
     CatchAllVirtualCluster(Stats::Scope& scope, const VirtualClusterStatNames& stat_names)
-        : VirtualClusterBase(stat_names.other_, scope.scopeFromStatName(stat_names.other_),
-                             stat_names) {}
+        : VirtualClusterBase(absl::nullopt, stat_names.other_,
+                             scope.scopeFromStatName(stat_names.other_), stat_names) {}
   };
 
   static const std::shared_ptr<const SslRedirectRoute> SSL_REDIRECT_ROUTE;

diff --git a/source/common/router/router.cc b/source/common/router/router.cc
@@ -453,6 +453,9 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers,
 
   // Set up stat prefixes, etc.
   request_vcluster_ = route_entry_->virtualCluster(headers);
+  if (request_vcluster_ != nullptr) {
+    callbacks_->streamInfo().setVirtualClusterName(request_vcluster_->name());
+  }
   ENVOY_STREAM_LOG(debug, "cluster '{}' match for URL '{}'", *callbacks_,
                    route_entry_->clusterName(), headers.getPathValue());
 

diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h
@@ -292,6 +292,14 @@ struct StreamInfoImpl : public StreamInfo {
 
   const std::string& getRouteName() const override { return route_name_; }
 
+  void setVirtualClusterName(const absl::optional<std::string>& virtual_cluster_name) override {
+    virtual_cluster_name_ = virtual_cluster_name;
+  }
+
+  const absl::optional<std::string>& virtualClusterName() const override {
+    return virtual_cluster_name_;
+  }
+
   void setUpstreamLocalAddress(
       const Network::Address::InstanceConstSharedPtr& upstream_local_address) override {
     maybeCreateUpstreamInfo();
@@ -443,6 +451,9 @@ struct StreamInfoImpl : public StreamInfo {
   FilterStateSharedPtr legacy_upstream_filter_state_;
   std::string route_name_;
   absl::optional<uint32_t> attempt_count_;
+  // TODO(agrawroh): Check if the owner of this storage outlives the StreamInfo. We should only copy
+  // the string if it could outlive the StreamInfo.
+  absl::optional<std::string> virtual_cluster_name_;
 
 private:
   static Network::ConnectionInfoProviderSharedPtr emptyDownstreamAddressProvider() {

diff --git a/test/common/formatter/substitution_formatter_test.cc b/test/common/formatter/substitution_formatter_test.cc
@@ -752,6 +752,22 @@ TEST(SubstitutionFormatterTest, streamInfoFormatterWithSsl) {
   Http::TestResponseTrailerMapImpl response_trailers;
   std::string body;
 
+  {
+    NiceMock<StreamInfo::MockStreamInfo> stream_info;
+    StreamInfoFormatter upstream_format("VIRTUAL_CLUSTER_NAME");
+    std::string virtual_cluster_name = "authN";
+    stream_info.setVirtualClusterName(virtual_cluster_name);
+    EXPECT_EQ("authN", upstream_format.format(request_headers, response_headers, response_trailers,
+                                              stream_info, body));
+  }
+
+  {
+    NiceMock<StreamInfo::MockStreamInfo> stream_info;
+    StreamInfoFormatter upstream_format("VIRTUAL_CLUSTER_NAME");
+    EXPECT_EQ(absl::nullopt, upstream_format.format(request_headers, response_headers,
+                                                    response_trailers, stream_info, body));
+  }
+
   {
     // Use a local stream info for these tests as as setSslConnection can only be called once.
     NiceMock<StreamInfo::MockStreamInfo> stream_info;

diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc
@@ -5912,6 +5912,9 @@ TEST_F(RouterTest, CanaryStatusTrue) {
   Http::TestRequestHeaderMapImpl headers{{"x-envoy-upstream-alt-stat-name", "alt_stat"},
                                          {"x-envoy-internal", "true"}};
   HttpTestUtility::addDefaultHeaders(headers);
+  const absl::optional<std::string> virtual_cluster_name =
+      absl::optional<std::string>("fake_virtual_cluster");
+  EXPECT_CALL(callbacks_.stream_info_, setVirtualClusterName(virtual_cluster_name));
   router_.decodeHeaders(headers, true);
   EXPECT_EQ(1U,
             callbacks_.route_->route_entry_.virtual_cluster_.stats().upstream_rq_total_.value());
@@ -5949,6 +5952,9 @@ TEST_F(RouterTest, CanaryStatusFalse) {
   Http::TestRequestHeaderMapImpl headers{{"x-envoy-upstream-alt-stat-name", "alt_stat"},
                                          {"x-envoy-internal", "true"}};
   HttpTestUtility::addDefaultHeaders(headers);
+  const absl::optional<std::string> virtual_cluster_name =
+      absl::optional<std::string>("fake_virtual_cluster");
+  EXPECT_CALL(callbacks_.stream_info_, setVirtualClusterName(virtual_cluster_name));
   router_.decodeHeaders(headers, true);
   EXPECT_EQ(1U,
             callbacks_.route_->route_entry_.virtual_cluster_.stats().upstream_rq_total_.value());

diff --git a/test/common/stream_info/test_util.h b/test/common/stream_info/test_util.h
@@ -34,6 +34,10 @@ class TestStreamInfo : public StreamInfo::StreamInfoImpl {
     return *downstream_connection_info_provider_;
   }
 
+  const absl::optional<std::string>& virtualClusterName() const override {
+    return virtual_cluster_name_;
+  }
+
   void onRequestComplete() override { end_time_ = timeSystem().monotonicTime(); }
 
   absl::optional<std::chrono::nanoseconds> requestComplete() const override {
@@ -54,6 +58,7 @@ class TestStreamInfo : public StreamInfo::StreamInfoImpl {
   SystemTime start_time_;
   MonotonicTime start_time_monotonic_;
   absl::optional<MonotonicTime> end_time_;
+  absl::optional<std::string> virtual_cluster_name_;
   Network::ConnectionInfoSetterSharedPtr downstream_connection_info_provider_{
       std::make_shared<Network::ConnectionInfoSetterImpl>(nullptr, nullptr)};
   Envoy::Event::SimulatedTimeSystem test_time_;

diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h
@@ -263,9 +263,11 @@ class MockShadowWriter : public ShadowWriter {
 class TestVirtualCluster : public VirtualCluster {
 public:
   // Router::VirtualCluster
+  const absl::optional<std::string>& name() const override { return name_; }
   Stats::StatName statName() const override { return stat_name_.statName(); }
   VirtualClusterStats& stats() const override { return stats_; }
 
+  const absl::optional<std::string> name_ = "fake_virtual_cluster";
   Stats::TestUtil::TestSymbolTable symbol_table_;
   Stats::StatNameManagedStorage stat_name_{"fake_virtual_cluster", *symbol_table_};
   Stats::IsolatedStoreImpl stats_store_;

diff --git a/test/mocks/stream_info/mocks.cc b/test/mocks/stream_info/mocks.cc
@@ -110,7 +110,12 @@ MockStreamInfo::MockStreamInfo()
   ON_CALL(*this, setRouteName(_)).WillByDefault(Invoke([this](const absl::string_view route_name) {
     route_name_ = std::string(route_name);
   }));
+  ON_CALL(*this, setVirtualClusterName(_))
+      .WillByDefault(Invoke([this](const absl::optional<std::string>& virtual_cluster_name) {
+        virtual_cluster_name_ = virtual_cluster_name;
+      }));
   ON_CALL(*this, getRouteName()).WillByDefault(ReturnRef(route_name_));
+  ON_CALL(*this, virtualClusterName()).WillByDefault(ReturnRef(virtual_cluster_name_));
   ON_CALL(*this, upstreamTransportFailureReason())
       .WillByDefault(ReturnRef(upstream_transport_failure_reason_));
   ON_CALL(*this, setConnectionID(_)).WillByDefault(Invoke([this](uint64_t id) {

diff --git a/test/mocks/stream_info/mocks.h b/test/mocks/stream_info/mocks.h
@@ -54,7 +54,10 @@ class MockStreamInfo : public StreamInfo {
   MOCK_METHOD(void, addWireBytesReceived, (uint64_t));
   MOCK_METHOD(uint64_t, wireBytesReceived, (), (const));
   MOCK_METHOD(void, setRouteName, (absl::string_view route_name));
+  MOCK_METHOD(void, setVirtualClusterName,
+              (const absl::optional<std::string>& virtual_cluster_name));
   MOCK_METHOD(const std::string&, getRouteName, (), (const));
+  MOCK_METHOD(const absl::optional<std::string>&, virtualClusterName, (), (const));
   MOCK_METHOD(absl::optional<Http::Protocol>, protocol, (), (const));
   MOCK_METHOD(void, protocol, (Http::Protocol protocol));
   MOCK_METHOD(absl::optional<uint32_t>, responseCode, (), (const));
@@ -145,6 +148,7 @@ class MockStreamInfo : public StreamInfo {
   std::string filter_chain_name_;
   absl::optional<uint64_t> upstream_connection_id_;
   absl::optional<uint32_t> attempt_count_;
+  absl::optional<std::string> virtual_cluster_name_;
   DownstreamTiming downstream_timing_;
 };