Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config: making protocol config explicit #14362

Merged
merged 7 commits into from
Dec 15, 2020
Merged

config: making protocol config explicit #14362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ae3d0c1
config: making protocol config explicit
alyssawilk Dec 10, 2020
fdcf274
example of a test which should fail
alyssawilk Dec 10, 2020
54047b2
actually validating
alyssawilk Dec 10, 2020
79f0808
whitespace
alyssawilk Dec 10, 2020
73c874e
comments
alyssawilk Dec 14, 2020
9e3737d
Merge branch 'master' into config_redux
alyssawilk Dec 14, 2020
8f399e2
test fix
alyssawilk Dec 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions api/envoy/extensions/upstreams/http/v3/http_protocol_options.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you are in here can you potentially make it more clear the http2 needs to be used for gRPC? I think that would help guide people in the right way even if we don't want to do something extreme like http2_and_grpc_protocol_options or something like that, which we probably don't want to do because h3 can be used with gRPC. So maybe just some comments/docs?


config.core.v3.Http1ProtocolOptions http_protocol_options = 1;

config.core.v3.Http2ProtocolOptions http2_protocol_options = 2;
Expand All @@ -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;
Expand Down
9 changes: 6 additions & 3 deletions api/envoy/extensions/upstreams/http/v4alpha/http_protocol_options.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 6 additions & 3 deletions generated_api_shadow/envoy/extensions/upstreams/http/v3/http_protocol_options.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 6 additions & 3 deletions generated_api_shadow/envoy/extensions/upstreams/http/v4alpha/http_protocol_options.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 8 additions & 5 deletions source/extensions/upstreams/http/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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"; }
Expand Down
17 changes: 17 additions & 0 deletions test/common/upstream/upstream_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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";
Expand Down
8 changes: 8 additions & 0 deletions test/integration/base_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
}
}

Expand Down
1 change: 1 addition & 0 deletions test/integration/http2_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions test/integration/tcp_proxy_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Expand Down
1 change: 1 addition & 0 deletions test/integration/tcp_tunneling_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down