config: making protocol config explicit (#14362)

Commit Message: Making the recently added ProtocolOptionsConfig require explicit configuration
Risk Level: Medium (config breaking, for config which is 7 days old)
Testing: n/a
Docs Changes: inline
Release Notes: n/a

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

api/envoy/extensions/upstreams/http/v3/http_protocol_options.proto

@@ -5,6 +5,7 @@ package envoy.extensions.upstreams.http.v3;
 import "envoy/config/core/v3/protocol.proto";
 
 import "udpa/annotations/status.proto";
+import "validate/validate.proto";
 
 option java_package = "io.envoyproxy.envoy.extensions.upstreams.http.v3";
 option java_outer_classname = "HttpProtocolOptionsProto";
@@ -57,10 +58,11 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
 //        .... [further cluster config]
 message HttpProtocolOptions {
   // If this is used, the cluster will only operate on one of the possible upstream protocols (HTTP/1.1, HTTP/2).
-  // If :ref:`http2_protocol_options <envoy_api_field_config.cluster.v3.Cluster.http2_protocol_options>` are
-  // present, HTTP2 will be used, otherwise HTTP1.1 will be used.
+  // Note that HTTP/2 should generally be used for upstream clusters doing gRPC.
   message ExplicitHttpConfig {
     oneof protocol_config {
+      option (validate.required) = true;
+
       config.core.v3.Http1ProtocolOptions http_protocol_options = 1;
 
       config.core.v3.Http2ProtocolOptions http2_protocol_options = 2;
@@ -82,8 +84,9 @@ message HttpProtocolOptions {
   config.core.v3.UpstreamHttpProtocolOptions upstream_http_protocol_options = 2;
 
   // This controls the actual protocol to be used upstream.
-  // If none of the *upstream_protocol_options* are chosen, the default is *explicit_http_config*.
   oneof upstream_protocol_options {
+    option (validate.required) = true;
+
     // To explicitly configure either HTTP/1 or HTTP/2 (but not both!) use *explicit_http_config*.
     // If the *explicit_http_config* is empty, HTTP/1.1 is used.
     ExplicitHttpConfig explicit_http_config = 3;

source/extensions/upstreams/http/config.h

@@ -12,8 +12,10 @@
 #include "envoy/extensions/upstreams/http/v3/http_protocol_options.pb.validate.h"
 #include "envoy/http/filter.h"
 #include "envoy/server/filter_config.h"
+#include "envoy/server/transport_socket_config.h"
 
 #include "common/common/logger.h"
+#include "common/protobuf/message_validator_impl.h"
 
 namespace Envoy {
 namespace Extensions {
@@ -44,11 +46,12 @@ class ProtocolOptionsConfigImpl : public Upstream::ProtocolOptionsConfig {
 
 class ProtocolOptionsConfigFactory : public Server::Configuration::ProtocolOptionsFactory {
 public:
-  Upstream::ProtocolOptionsConfigConstSharedPtr
-  createProtocolOptionsConfig(const Protobuf::Message& config,
-                              Server::Configuration::ProtocolOptionsFactoryContext&) override {
-    const envoy::extensions::upstreams::http::v3::HttpProtocolOptions& typed_config =
-        *dynamic_cast<const envoy::extensions::upstreams::http::v3::HttpProtocolOptions*>(&config);
+  Upstream::ProtocolOptionsConfigConstSharedPtr createProtocolOptionsConfig(
+      const Protobuf::Message& config,
+      Server::Configuration::ProtocolOptionsFactoryContext& context) override {
+    const auto& typed_config = MessageUtil::downcastAndValidate<
+        const envoy::extensions::upstreams::http::v3::HttpProtocolOptions&>(
+        config, context.messageValidationVisitor());
     return std::make_shared<ProtocolOptionsConfigImpl>(typed_config);
   }
   std::string category() const override { return "envoy.upstream_options"; }

test/common/upstream/upstream_impl_test.cc

@@ -2561,6 +2561,16 @@ TEST_F(ClusterInfoImplTest, Timeouts) {
   )EOF";
 
   const std::string explicit_timeout_new = R"EOF(
+    typed_extension_protocol_options:
+      envoy.extensions.upstreams.http.v3.HttpProtocolOptions:
+        "@type": type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions
+        explicit_http_config:
+          http_protocol_options: {}
+        common_http_protocol_options:
+          idle_timeout: 1s
+  )EOF";
+
+  const std::string explicit_timeout_bad = R"EOF(
     typed_extension_protocol_options:
       envoy.extensions.upstreams.http.v3.HttpProtocolOptions:
         "@type": type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions
@@ -2578,6 +2588,11 @@ TEST_F(ClusterInfoImplTest, Timeouts) {
     ASSERT_TRUE(cluster2->info()->idleTimeout().has_value());
     EXPECT_EQ(std::chrono::seconds(1), cluster2->info()->idleTimeout().value());
   }
+  {
+    auto cluster2 = makeCluster(yaml + explicit_timeout_new);
+    EXPECT_THROW_WITH_REGEX(makeCluster(yaml + explicit_timeout_bad, false), EnvoyException,
+                            ".*Proto constraint validation failed.*");
+  }
   const std::string no_timeout = R"EOF(
     common_http_protocol_options:
       idle_timeout: 0s
@@ -2587,6 +2602,8 @@ TEST_F(ClusterInfoImplTest, Timeouts) {
     typed_extension_protocol_options:
       envoy.extensions.upstreams.http.v3.HttpProtocolOptions:
         "@type": type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions
+        explicit_http_config:
+          http_protocol_options: {}
         common_http_protocol_options:
           idle_timeout: 0s
   )EOF";

test/integration/base_integration_test.cc

@@ -218,6 +218,14 @@ void BaseIntegrationTest::setUpstreamProtocol(FakeHttpConnection::Type protocol)
         });
   } else {
     RELEASE_ASSERT(protocol == FakeHttpConnection::Type::HTTP1, "");
+    config_helper_.addConfigModifier(
+        [&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void {
+          RELEASE_ASSERT(bootstrap.mutable_static_resources()->clusters_size() >= 1, "");
+          ConfigHelper::HttpProtocolOptions protocol_options;
+          protocol_options.mutable_explicit_http_config()->mutable_http_protocol_options();
+          ConfigHelper::setProtocolOptions(
+              *bootstrap.mutable_static_resources()->mutable_clusters(0), protocol_options);
+        });
   }
 }
 

test/integration/http2_integration_test.cc

@@ -957,6 +957,7 @@ TEST_P(Http2IntegrationTest, IdleTimeoutWithSimultaneousRequests) {
   config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
     ConfigHelper::HttpProtocolOptions protocol_options;
     auto* http_protocol_options = protocol_options.mutable_common_http_protocol_options();
+    protocol_options.mutable_explicit_http_config()->mutable_http_protocol_options();
     auto* idle_time_out = http_protocol_options->mutable_idle_timeout();
     std::chrono::milliseconds timeout(1000);
     auto seconds = std::chrono::duration_cast<std::chrono::seconds>(timeout);

test/integration/tcp_proxy_integration_test.cc

@@ -96,6 +96,7 @@ TEST_P(TcpProxyIntegrationTest, TcpProxyUpstreamWritesFirst) {
 // Test TLS upstream.
 TEST_P(TcpProxyIntegrationTest, TcpProxyUpstreamTls) {
   upstream_tls_ = true;
+  setUpstreamProtocol(FakeHttpConnection::Type::HTTP1);
   config_helper_.configureUpstreamTls();
   initialize();
   IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("tcp_proxy"));

test/integration/tcp_tunneling_integration_test.cc

@@ -186,6 +186,7 @@ TEST_P(ConnectTerminationIntegrationTest, BuggyHeaders) {
 }
 
 TEST_P(ConnectTerminationIntegrationTest, BasicMaxStreamDuration) {
+  setUpstreamProtocol(upstreamProtocol());
   config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
     ConfigHelper::HttpProtocolOptions protocol_options;
     protocol_options.mutable_common_http_protocol_options()