Skip to content

config: fix dfp config validation #17835

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

Merged
alyssawilk merged 4 commits into from
Aug 25, 2021
Merged

config: fix dfp config validation #17835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
57476d7
config: fix dfp config validation
alyssawilk Aug 24, 2021
1813272
comments
alyssawilk Aug 25, 2021
24485ad
test fixup
alyssawilk Aug 25, 2021
98b9c7a
Merge branch 'main' into san
alyssawilk Aug 25, 2021
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
1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ Bug Fixes
* access log: fix ``%UPSTREAM_CLUSTER%`` when used in http upstream access logs. Previously, it was always logging as an unset value.
* aws request signer: fix the AWS Request Signer extension to correctly normalize the path and query string to be signed according to AWS' guidelines, so that the hash on the server side matches. See `AWS SigV4 documentaion <https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html>`_.
* cluster: delete pools when they're idle to fix unbounded memory use when using PROXY protocol upstream with tcp_proxy. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.conn_pool_delete_when_idle`` runtime guard to false.
* dynamic forward proxy: fixing a validation bug where san and sni checks were not applied setting :ref:`http_protocol_options <envoy_v3_api_msg_extensions.upstreams.http.v3.HttpProtocolOptions>` via :ref:`typed_extension_protocol_options <envoy_v3_api_field_config.cluster.v3.Cluster.typed_extension_protocol_options>`.
Copy link
Member

@leyao-daily leyao-daily Aug 25, 2021

Choose a reason for hiding this comment

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

Looks good! And please move it to be an alphabetical order.

* ext_authz: fix the ext_authz filter to correctly merge multiple same headers using the ',' as separator in the check request to the external authorization service.
* hcm: remove deprecation for :ref:`xff_num_trusted_hops <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.xff_num_trusted_hops>` and forbid mixing ip detection extensions with old related knobs.
* http: limit use of deferred resets in the http2 codec to server-side connections. Use of deferred reset for client connections can result in incorrect behavior and performance problems.
Expand Down
26 changes: 17 additions & 9 deletions source/extensions/clusters/dynamic_forward_proxy/cluster.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include "source/common/network/transport_socket_options_impl.h"
#include "source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.h"
#include "source/extensions/filters/network/common/utility.h"

namespace Envoy {
namespace Extensions {
Expand Down Expand Up @@ -179,22 +180,29 @@ ClusterFactory::createClusterWithConfig(
context.singletonManager(), context.dispatcher(), context.tls(), context.api(),
context.runtime(), context.stats(), context.messageValidationVisitor());
envoy::config::cluster::v3::Cluster cluster_config = cluster;
if (cluster_config.has_upstream_http_protocol_options()) {
if (!proto_config.allow_insecure_cluster_options() &&
(!cluster_config.upstream_http_protocol_options().auto_sni() ||
!cluster_config.upstream_http_protocol_options().auto_san_validation())) {
throw EnvoyException(
"dynamic_forward_proxy cluster must have auto_sni and auto_san_validation true when "
"configured with upstream_http_protocol_options");
}
} else {
if (!cluster_config.has_upstream_http_protocol_options()) {
// This sets defaults which will only apply if using old style http config.
// They will be a no-op if typed_extension_protocol_options are used for
// http config.
cluster_config.mutable_upstream_http_protocol_options()->set_auto_sni(true);
cluster_config.mutable_upstream_http_protocol_options()->set_auto_san_validation(true);
}

auto new_cluster = std::make_shared<Cluster>(
cluster_config, proto_config, context.runtime(), cache_manager_factory, context.localInfo(),
socket_factory_context, std::move(stats_scope), context.addedViaApi());

auto& options = new_cluster->info()->upstreamHttpProtocolOptions();

if (!proto_config.allow_insecure_cluster_options()) {
if (!options.has_value() ||
(!options.value().auto_sni() || !options.value().auto_san_validation())) {
throw EnvoyException(
"dynamic_forward_proxy cluster must have auto_sni and auto_san_validation true unless "
"allow_insecure_cluster_options is set.");
}
Comment on lines +198 to +203
Copy link
Member

@mattklein123 mattklein123 Aug 25, 2021

Choose a reason for hiding this comment

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

I think you can remove this check from above (and just set the legacy option) since this should check both cases after the cluster is created?

}

auto lb = std::make_unique<Cluster::ThreadAwareLoadBalancer>(*new_cluster);
return std::make_pair(new_cluster, std::move(lb));
}
Expand Down
4 changes: 2 additions & 2 deletions test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,8 +249,8 @@ upstream_http_protocol_options: {}

EXPECT_THROW_WITH_MESSAGE(
createCluster(yaml_config), EnvoyException,
"dynamic_forward_proxy cluster must have auto_sni and auto_san_validation true when "
"configured with upstream_http_protocol_options");
"dynamic_forward_proxy cluster must have auto_sni and auto_san_validation true unless "
"allow_insecure_cluster_options is set.");
}

TEST_F(ClusterFactoryTest, InsecureUpstreamHttpProtocolOptions) {
Expand Down
6 changes: 6 additions & 0 deletions test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@ name: dynamic_forward_proxy
cluster_.set_name("cluster_0");
cluster_.set_lb_policy(envoy::config::cluster::v3::Cluster::CLUSTER_PROVIDED);

ConfigHelper::HttpProtocolOptions protocol_options;
protocol_options.mutable_upstream_http_protocol_options()->set_auto_sni(true);
protocol_options.mutable_upstream_http_protocol_options()->set_auto_san_validation(true);
protocol_options.mutable_explicit_http_config()->mutable_http_protocol_options();
ConfigHelper::setProtocolOptions(cluster_, protocol_options);

if (upstream_tls_) {
envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context;
auto* validation_context =
Expand Down