From ff3ef1294a73d36b16a1057b9aaf1d297b7c3090 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Tue, 15 Dec 2020 08:36:31 -0500 Subject: [PATCH] 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 --- .../http/v3/http_protocol_options.proto | 9 ++++++--- .../http/v4alpha/http_protocol_options.proto | 9 ++++++--- .../http/v3/http_protocol_options.proto | 9 ++++++--- .../http/v4alpha/http_protocol_options.proto | 9 ++++++--- source/extensions/upstreams/http/config.h | 13 ++++++++----- test/common/upstream/upstream_impl_test.cc | 17 +++++++++++++++++ test/integration/base_integration_test.cc | 8 ++++++++ test/integration/http2_integration_test.cc | 1 + test/integration/tcp_proxy_integration_test.cc | 1 + .../tcp_tunneling_integration_test.cc | 1 + 10 files changed, 60 insertions(+), 17 deletions(-) diff --git a/api/envoy/extensions/upstreams/http/v3/http_protocol_options.proto b/api/envoy/extensions/upstreams/http/v3/http_protocol_options.proto index d3cd59bbc26f..7c5dce7854ee 100644 --- a/api/envoy/extensions/upstreams/http/v3/http_protocol_options.proto +++ b/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 ` 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; diff --git a/api/envoy/extensions/upstreams/http/v4alpha/http_protocol_options.proto b/api/envoy/extensions/upstreams/http/v4alpha/http_protocol_options.proto index 3b18b128c412..4c97b8a69f8f 100644 --- a/api/envoy/extensions/upstreams/http/v4alpha/http_protocol_options.proto +++ b/api/envoy/extensions/upstreams/http/v4alpha/http_protocol_options.proto @@ -6,6 +6,7 @@ import "envoy/config/core/v4alpha/protocol.proto"; import "udpa/annotations/status.proto"; import "udpa/annotations/versioning.proto"; +import "validate/validate.proto"; option java_package = "io.envoyproxy.envoy.extensions.upstreams.http.v4alpha"; option java_outer_classname = "HttpProtocolOptionsProto"; @@ -61,13 +62,14 @@ message HttpProtocolOptions { "envoy.extensions.upstreams.http.v3.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 ` 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 { option (udpa.annotations.versioning).previous_message_type = "envoy.extensions.upstreams.http.v3.HttpProtocolOptions.ExplicitHttpConfig"; oneof protocol_config { + option (validate.required) = true; + config.core.v4alpha.Http1ProtocolOptions http_protocol_options = 1; config.core.v4alpha.Http2ProtocolOptions http2_protocol_options = 2; @@ -92,8 +94,9 @@ message HttpProtocolOptions { config.core.v4alpha.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; diff --git a/generated_api_shadow/envoy/extensions/upstreams/http/v3/http_protocol_options.proto b/generated_api_shadow/envoy/extensions/upstreams/http/v3/http_protocol_options.proto index d3cd59bbc26f..7c5dce7854ee 100644 --- a/generated_api_shadow/envoy/extensions/upstreams/http/v3/http_protocol_options.proto +++ b/generated_api_shadow/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 ` 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; diff --git a/generated_api_shadow/envoy/extensions/upstreams/http/v4alpha/http_protocol_options.proto b/generated_api_shadow/envoy/extensions/upstreams/http/v4alpha/http_protocol_options.proto index 3b18b128c412..4c97b8a69f8f 100644 --- a/generated_api_shadow/envoy/extensions/upstreams/http/v4alpha/http_protocol_options.proto +++ b/generated_api_shadow/envoy/extensions/upstreams/http/v4alpha/http_protocol_options.proto @@ -6,6 +6,7 @@ import "envoy/config/core/v4alpha/protocol.proto"; import "udpa/annotations/status.proto"; import "udpa/annotations/versioning.proto"; +import "validate/validate.proto"; option java_package = "io.envoyproxy.envoy.extensions.upstreams.http.v4alpha"; option java_outer_classname = "HttpProtocolOptionsProto"; @@ -61,13 +62,14 @@ message HttpProtocolOptions { "envoy.extensions.upstreams.http.v3.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 ` 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 { option (udpa.annotations.versioning).previous_message_type = "envoy.extensions.upstreams.http.v3.HttpProtocolOptions.ExplicitHttpConfig"; oneof protocol_config { + option (validate.required) = true; + config.core.v4alpha.Http1ProtocolOptions http_protocol_options = 1; config.core.v4alpha.Http2ProtocolOptions http2_protocol_options = 2; @@ -92,8 +94,9 @@ message HttpProtocolOptions { config.core.v4alpha.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; diff --git a/source/extensions/upstreams/http/config.h b/source/extensions/upstreams/http/config.h index f68ffe73f834..e55cb18117f0 100644 --- a/source/extensions/upstreams/http/config.h +++ b/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(&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(typed_config); } std::string category() const override { return "envoy.upstream_options"; } diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index e565609d214c..f52a5cdb5250 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/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"; diff --git a/test/integration/base_integration_test.cc b/test/integration/base_integration_test.cc index 647faae59874..45a7019ed9ac 100644 --- a/test/integration/base_integration_test.cc +++ b/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); + }); } } diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index ef311a7b6d14..a0e73bdbde68 100644 --- a/test/integration/http2_integration_test.cc +++ b/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(timeout); diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc index 8d9857e705db..9682480725ba 100644 --- a/test/integration/tcp_proxy_integration_test.cc +++ b/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")); diff --git a/test/integration/tcp_tunneling_integration_test.cc b/test/integration/tcp_tunneling_integration_test.cc index d17fc468287f..f8b5dd8273b2 100644 --- a/test/integration/tcp_tunneling_integration_test.cc +++ b/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()