reviewer comments

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

docs/root/intro/version_history.rst

@@ -25,7 +25,7 @@ Version history
 * grpc-json: added support for :ref:`ignoring unknown query parameters<envoy_api_field_config.filter.http.transcoder.v2.GrpcJsonTranscoder.ignore_unknown_query_parameters>`.
 * header to metadata: added :ref:`PROTOBUF_VALUE <envoy_api_enum_value_config.filter.http.header_to_metadata.v2.Config.ValueType.PROTOBUF_VALUE>` and :ref:`ValueEncode <envoy_api_enum_config.filter.http.header_to_metadata.v2.Config.ValueEncode>` to support protobuf Value and Base64 encoding.
 * http: added the ability to reject HTTP/1.1 requests with invalid HTTP header values, using the runtime feature `envoy.reloadable_features.strict_header_validation`.
-* http: changed Envoy to forward existing x-forwarded-proto from upstream trusted proxies. Guarded by `envoy.reloadable_features.trusted_forwarded_proto`.
+* http: changed Envoy to forward existing x-forwarded-proto from upstream trusted proxies. Guarded by `envoy.reloadable_features.trusted_forwarded_proto` which defaults true.
 * http: added the ability to :ref:`merge adjacent slashes<envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.merge_slashes>` in the path.
 * listeners: added :ref:`continue_on_listener_filters_timeout <envoy_api_field_Listener.continue_on_listener_filters_timeout>` to configure whether a listener will still create a connection when listener filters time out.
 * listeners: added :ref:`HTTP inspector listener filter <config_listener_filters_http_inspector>`.

source/common/http/conn_manager_utility.cc

@@ -85,7 +85,7 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest
   Network::Address::InstanceConstSharedPtr final_remote_address;
   bool single_xff_address;
   const uint32_t xff_num_trusted_hops = config.xffNumTrustedHops();
-  bool trusted_forwarded_proto =
+  const bool trusted_forwarded_proto =
       Runtime::runtimeFeatureEnabled("envoy.reloadable_features.trusted_forwarded_proto");
 
   if (config.useRemoteAddress()) {
@@ -111,13 +111,17 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest
       }
     }
     if (trusted_forwarded_proto) {
-      // Set x-forwarded-proto unless it already exists and was forwarded on by a trusted proxy.
+      // If the prior hop is not a trusted proxy, overwrite any x-forwarded-proto value it set as
+      // untrusted. Alternately if no x-forwarded-proto header exists, add one.
       if (xff_num_trusted_hops == 0 || request_headers.ForwardedProto() == nullptr) {
         request_headers.insertForwardedProto().value().setReference(
             connection.ssl() ? Headers::get().SchemeValues.Https
                              : Headers::get().SchemeValues.Http);
       }
     } else {
+      // Previously, before the trusted_forwarded_proto logic, Envoy would always overwrite the
+      // x-forwarded-proto header even if it was set by a trusted proxy. This code path is
+      // deprecated and will be removed.
       request_headers.insertForwardedProto().value().setReference(
           connection.ssl() ? Headers::get().SchemeValues.Https : Headers::get().SchemeValues.Http);
     }
@@ -130,8 +134,8 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest
     single_xff_address = ret.single_address_;
   }
 
-  // If we didn't already replace x-forwarded-proto because we are using the remote address, and
-  // remote hasn't set it (trusted proxy), we set it, since we then use this for setting scheme.
+  // If the x-forwarded-proto header is not set, set it here, since Envoy uses it for determining
+  // scheme and communicating it upstream.
   if (!request_headers.ForwardedProto()) {
     request_headers.insertForwardedProto().value().setReference(
         connection.ssl() ? Headers::get().SchemeValues.Https : Headers::get().SchemeValues.Http);

source/common/runtime/runtime_features.cc

@@ -26,6 +26,7 @@ constexpr const char* runtime_features[] = {
     // Enabled
     "envoy.reloadable_features.test_feature_true",
     "envoy.reloadable_features.buffer_filter_populate_content_length",
+    "envoy.reloadable_features.trusted_forwarded_proto",
 };
 
 // This is a list of configuration fields which are disallowed by default in Envoy

test/test_common/test_runtime.h

@@ -5,6 +5,9 @@
 //  TestScopedRuntime scoped_runtime;
 //  Runtime::LoaderSingleton::getExisting()->mergeValues(
 //      {{"envoy.reloadable_features.test_feature_true", "false"}});
+//
+//  As long as a TestScopedRuntime exists, Runtime::LoaderSingleton::getExisting()->mergeValues()
+//  can safely be called to override runtime values.
 
 #pragma once
 
@@ -29,9 +32,9 @@ class TestScopedRuntime {
     envoy::config::bootstrap::v2::LayeredRuntime config;
     config.add_layers()->mutable_admin_layer();
 
-    loader_ = std::make_unique<Runtime::ScopedLoaderSingleton>(Runtime::LoaderPtr{
-        new Runtime::LoaderImpl(dispatcher_, tls_, config, local_info_, init_manager_, store_,
-                                generator_, validation_visitor_, *api_)});
+    loader_ = std::make_unique<Runtime::ScopedLoaderSingleton>(
+        std::make_unique<Runtime::LoaderImpl>(dispatcher_, tls_, config, local_info_, init_manager_,
+                                              store_, generator_, validation_visitor_, *api_));
   }
 
 private: