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.

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

http: allow ignoring unknown upgrade requests #37150

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
// HTTP connection manager :ref:`configuration overview <config_http_conn_man>`.
// [#extension: envoy.filters.network.http_connection_manager]

// [#next-free-field: 59]
// [#next-free-field: 60]
message HttpConnectionManager {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager";
Expand Down Expand Up @@ -289,11 +289,18 @@ message HttpConnectionManager {
// HTTP connections will be used for this upgrade type.
repeated HttpFilter filters = 2;

// Determines if upgrades are enabled or disabled by default. Defaults to true.
// Determines if upgrades are enabled or disabled by default. Defaults to true, unless
// :ref:`ignore <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.UpgradeConfig.ignore>`
// is enabled. It is invalid to set both this option and ``disabled`` to true.
// This can be overridden on a per-route basis with :ref:`cluster
// <envoy_v3_api_field_config.route.v3.RouteAction.upgrade_configs>` as documented in the
// :ref:`upgrade documentation <arch_overview_upgrades>`.
google.protobuf.BoolValue enabled = 3;

// If enabled, Envoy will ignore upgrade requests of this type. It is invalid to set both this option and
// :ref:`enabled <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.UpgradeConfig.enabled>`
// to true.
google.protobuf.BoolValue ignore = 4;
ggreenway marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

actually follow-up on that, do we have or need a way to ignore for all types?

Copy link
Contributor Author

@ggreenway ggreenway Nov 14, 2024

Choose a reason for hiding this comment

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

What do you mean by "ignore for all types"? If you mean "ignore all upgrade requests", that's https://github.com/envoyproxy/envoy/pull/37150/files#diff-4b0f0f4bf391293548732ba0dac7ea417d196e9e5bb87ad4041850f453a982f5R802. If that's not what you meant, please clarify the question.

}

// [#not-implemented-hide:] Transformations that apply to path headers. Transformations are applied
Expand Down Expand Up @@ -787,6 +794,13 @@ message HttpConnectionManager {

repeated UpgradeConfig upgrade_configs = 23;

// If enabled, if Envoy receives an Upgrade request from a client and no configuration for
// the upgrade type is found in
// :ref:`upgrade_configs <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.upgrade_configs>`,
// Envoy will ignore the upgrade request. If disabled, Envoy will respond with an error and close the connection.
// The default is true.
google.protobuf.BoolValue ignore_unconfigured_upgrades = 59;

// Should paths be normalized according to RFC 3986 before any processing of
// requests by HTTP filters or routing? This affects the upstream ``:path`` header
// as well. For paths that fail this check, Envoy will respond with 400 to
Expand Down
13 changes: 10 additions & 3 deletions envoy/http/filter_factory.h
Original file line number Diff line number Diff line change
Expand Up @@ -100,17 +100,24 @@ class FilterChainFactory {
createFilterChain(FilterChainManager& manager, bool only_create_if_configured = false,
const FilterChainOptions& options = EmptyFilterChainOptions{}) const PURE;

enum class UpgradeAction {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think move the comments below up here in case the enum gets used multiple places

Rejected,
Accepted,
Ignored,
};

/**
* Called when a new upgrade stream is created on the connection.
* @param upgrade supplies the upgrade header from downstream
* @param per_route_upgrade_map supplies the upgrade map, if any, for this route.
* @param manager supplies the "sink" that is used for actually creating the filter chain. @see
* FilterChainManager.
* @return true if upgrades of this type are allowed and the filter chain has been created.
* returns false if this upgrade type is not configured, and no filter chain is created.
* @return ACCEPTED if upgrades of this type are allowed and the filter chain has been created.
* returns REJECTED if this upgrade type is not allowed, and no filter chain is created.
* returns IGNORED if the upgrade type should be ignored and no upgrade will take place.
*/
using UpgradeMap = std::map<std::string, bool>;
virtual bool createUpgradeFilterChain(
virtual UpgradeAction createUpgradeFilterChain(
absl::string_view upgrade, const UpgradeMap* per_route_upgrade_map,
FilterChainManager& manager,
const FilterChainOptions& options = EmptyFilterChainOptions{}) const PURE;
Expand Down
22 changes: 14 additions & 8 deletions source/common/http/filter_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1647,14 +1647,20 @@ bool FilterManager::createFilterChain() {
if (upgrade != nullptr) {
const Router::RouteEntry::UpgradeMap* upgrade_map = filter_manager_callbacks_.upgradeMap();

if (filter_chain_factory_.createUpgradeFilterChain(upgrade->value().getStringView(),
upgrade_map, *this, options)) {
auto result = filter_chain_factory_.createUpgradeFilterChain(upgrade->value().getStringView(),
upgrade_map, *this, options);
switch (result) {
case FilterChainFactory::UpgradeAction::Accepted:
filter_manager_callbacks_.upgradeFilterChainCreated();
return true;
} else {
case FilterChainFactory::UpgradeAction::Rejected:
upgrade_rejected = true;
// Fall through to the default filter chain. The function calling this
// will send a local reply indicating that the upgrade failed.
break;
case FilterChainFactory::UpgradeAction::Ignored:
filter_manager_callbacks_.requestHeaders()->removeUpgrade();
break;
}
}

Expand Down Expand Up @@ -1714,11 +1720,11 @@ bool ActiveStreamDecoderFilter::recreateStream(const ResponseHeaderMap* headers)

if (headers != nullptr) {
// The call to setResponseHeaders is needed to ensure that the headers are properly logged in
// access logs before the stream is destroyed. Since the function expects a ResponseHeaderPtr&&,
// ownership of the headers must be passed. This cannot happen earlier in the flow (such as in
// the call to setupRedirect) because at that point it is still possible for the headers to be
// used in a different logical branch. We work around this by creating a copy and passing
// ownership of the copy instead.
// access logs before the stream is destroyed. Since the function expects a
// ResponseHeaderPtr&&, ownership of the headers must be passed. This cannot happen earlier in
// the flow (such as in the call to setupRedirect) because at that point it is still possible
// for the headers to be used in a different logical branch. We work around this by creating a
// copy and passing ownership of the copy instead.
ResponseHeaderMapPtr headers_copy = createHeaderMap<ResponseHeaderMapImpl>(*headers);
parent_.filter_manager_callbacks_.setResponseHeaders(std::move(headers_copy));
parent_.filter_manager_callbacks_.chargeStats(*headers);
Expand Down
7 changes: 4 additions & 3 deletions source/common/router/router.h
Original file line number Diff line number Diff line change
Expand Up @@ -248,10 +248,11 @@ class FilterConfig : Http::FilterChainFactory {
return true;
}

bool createUpgradeFilterChain(absl::string_view, const UpgradeMap*, Http::FilterChainManager&,
const Http::FilterChainOptions&) const override {
Http::FilterChainFactory::UpgradeAction
createUpgradeFilterChain(absl::string_view, const UpgradeMap*, Http::FilterChainManager&,
const Http::FilterChainOptions&) const override {
// Upgrade filter chains not yet supported for upstream HTTP filters.
return false;
return FilterChainFactory::UpgradeAction::Rejected;
}

using HeaderVector = std::vector<Http::LowerCaseString>;
Expand Down
7 changes: 4 additions & 3 deletions source/common/upstream/upstream_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -1029,10 +1029,11 @@ class ClusterInfoImpl : public ClusterInfo,
manager, Http::EmptyFilterChainOptions{}, http_filter_factories_);
return true;
}
bool createUpgradeFilterChain(absl::string_view, const UpgradeMap*, Http::FilterChainManager&,
const Http::FilterChainOptions&) const override {
Http::FilterChainFactory::UpgradeAction
createUpgradeFilterChain(absl::string_view, const UpgradeMap*, Http::FilterChainManager&,
const Http::FilterChainOptions&) const override {
// Upgrade filter chains not yet supported for upstream HTTP filters.
return false;
return Http::FilterChainFactory::UpgradeAction::Rejected;
}

Http::Http1::CodecStats& http1CodecStats() const override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,8 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
internal_address_config_(createInternalAddressConfig(config, creation_status)),
xff_num_trusted_hops_(config.xff_num_trusted_hops()),
skip_xff_append_(config.skip_xff_append()), via_(config.via()),
ignore_unconfigured_upgrades_(
PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, ignore_unconfigured_upgrades, true)),
scoped_routes_config_provider_manager_(scoped_routes_config_provider_manager),
filter_config_provider_manager_(filter_config_provider_manager),
http3_options_(Http3::Utility::initializeAndValidateOptions(
Expand Down Expand Up @@ -729,13 +731,19 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(

for (const auto& upgrade_config : config.upgrade_configs()) {
const std::string& name = upgrade_config.upgrade_type();
const bool enabled = upgrade_config.has_enabled() ? upgrade_config.enabled().value() : true;
const bool ignored = PROTOBUF_GET_WRAPPED_OR_DEFAULT(upgrade_config, ignore, false);
const bool enabled = PROTOBUF_GET_WRAPPED_OR_DEFAULT(upgrade_config, enabled, !ignored);
if (findUpgradeCaseInsensitive(upgrade_filter_factories_, name) !=
upgrade_filter_factories_.end()) {
creation_status = absl::InvalidArgumentError(
fmt::format("Error: multiple upgrade configs with the same name: '{}'", name));
return;
}
if (enabled && ignored) {
creation_status = absl::InvalidArgumentError(fmt::format(
"Error: upgrade config set both `ignored` and `enabled` for upgrade type: '{}'", name));
return;
}
if (!upgrade_config.filters().empty()) {
std::unique_ptr<FilterFactoriesList> factories = std::make_unique<FilterFactoriesList>();
Http::DependencyManager upgrade_dependency_manager;
Expand All @@ -747,11 +755,11 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
SET_AND_RETURN_IF_NOT_OK(status, creation_status);

upgrade_filter_factories_.emplace(
std::make_pair(name, FilterConfig{std::move(factories), enabled}));
std::make_pair(name, FilterConfig{std::move(factories), enabled, ignored}));
} else {
std::unique_ptr<FilterFactoriesList> factories(nullptr);
upgrade_filter_factories_.emplace(
std::make_pair(name, FilterConfig{std::move(factories), enabled}));
std::make_pair(name, FilterConfig{std::move(factories), enabled, ignored}));
}
}
}
Expand Down Expand Up @@ -796,7 +804,7 @@ bool HttpConnectionManagerConfig::createFilterChain(Http::FilterChainManager& ma
return true;
}

bool HttpConnectionManagerConfig::createUpgradeFilterChain(
Http::FilterChainFactory::UpgradeAction HttpConnectionManagerConfig::createUpgradeFilterChain(
absl::string_view upgrade_type,
const Http::FilterChainFactory::UpgradeMap* per_route_upgrade_map,
Http::FilterChainManager& callbacks, const Http::FilterChainOptions& options) const {
Expand All @@ -806,26 +814,37 @@ bool HttpConnectionManagerConfig::createUpgradeFilterChain(
if (route_it != per_route_upgrade_map->end()) {
// Upgrades explicitly not allowed on this route.
if (route_it->second == false) {
return false;
return Http::FilterChainFactory::UpgradeAction::Rejected;
}
// Upgrades explicitly enabled on this route.
route_enabled = true;
}
}

auto it = findUpgradeCaseInsensitive(upgrade_filter_factories_, upgrade_type);
if ((it == upgrade_filter_factories_.end() || !it->second.allow_upgrade) && !route_enabled) {
// Either the HCM disables upgrades and the route-config does not override,
// or neither is configured for this upgrade.
return false;
if (route_enabled || (it != upgrade_filter_factories_.end() && it->second.allow_upgrade)) {
// Either the per-route config enables this upgrade, or the HCM config allows the upgrade.
const FilterFactoriesList* filters_to_use = &filter_factories_;
if (it != upgrade_filter_factories_.end() && it->second.filter_factories != nullptr) {
filters_to_use = it->second.filter_factories.get();
}

Http::FilterChainUtility::createFilterChainForFactories(callbacks, options, *filters_to_use);
return Http::FilterChainFactory::UpgradeAction::Accepted;
}
const FilterFactoriesList* filters_to_use = &filter_factories_;
if (it != upgrade_filter_factories_.end() && it->second.filter_factories != nullptr) {
filters_to_use = it->second.filter_factories.get();

if ((ignore_unconfigured_upgrades_ && it == upgrade_filter_factories_.end()) ||
(it != upgrade_filter_factories_.end() && it->second.ignore)) {
// Either the HCM ignores all unconfigured upgrades, or this upgrade type is configured to be
// ignored.
return Http::FilterChainFactory::UpgradeAction::Ignored;
}

Http::FilterChainUtility::createFilterChainForFactories(callbacks, options, *filters_to_use);
return true;
// Fall-through cases: the HCM does not ignore unconfigured upgrades, or this upgrade type is
// configured to be disallowed.
ASSERT(!ignore_unconfigured_upgrades_ || (it != upgrade_filter_factories_.end() &&
!it->second.ignore && !it->second.allow_upgrade));
return Http::FilterChainFactory::UpgradeAction::Rejected;
}

const Network::Address::Instance& HttpConnectionManagerConfig::localAddress() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,12 +144,14 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
using FilterFactoriesList = Envoy::Http::FilterChainUtility::FilterFactoriesList;
struct FilterConfig {
std::unique_ptr<FilterFactoriesList> filter_factories;
bool allow_upgrade;
const bool allow_upgrade;
const bool ignore;
};
bool createUpgradeFilterChain(absl::string_view upgrade_type,
const Http::FilterChainFactory::UpgradeMap* per_route_upgrade_map,
Http::FilterChainManager& manager,
const Http::FilterChainOptions& options) const override;
Http::FilterChainFactory::UpgradeAction
createUpgradeFilterChain(absl::string_view upgrade_type,
const Http::FilterChainFactory::UpgradeMap* per_route_upgrade_map,
Http::FilterChainManager& manager,
const Http::FilterChainOptions& options) const override;

// Http::ConnectionManagerConfig
const Http::RequestIDExtensionSharedPtr& requestIDExtension() override {
Expand Down Expand Up @@ -307,6 +309,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
const uint32_t xff_num_trusted_hops_;
const bool skip_xff_append_;
const std::string via_;
const bool ignore_unconfigured_upgrades_;
Http::ForwardClientCertType forward_client_cert_;
std::vector<Http::ClientCertDetailsType> set_current_client_cert_details_;
Config::ConfigProviderManager* scoped_routes_config_provider_manager_;
Expand Down
9 changes: 5 additions & 4 deletions source/server/admin/admin.h
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,11 @@ class AdminImpl : public Admin,
// Http::FilterChainFactory
bool createFilterChain(Http::FilterChainManager& manager, bool,
const Http::FilterChainOptions&) const override;
bool createUpgradeFilterChain(absl::string_view, const Http::FilterChainFactory::UpgradeMap*,
Http::FilterChainManager&,
const Http::FilterChainOptions&) const override {
return false;
Http::FilterChainFactory::UpgradeAction
createUpgradeFilterChain(absl::string_view, const Http::FilterChainFactory::UpgradeMap*,
Http::FilterChainManager&,
const Http::FilterChainOptions&) const override {
return Http::FilterChainFactory::UpgradeAction::Ignored;
}

// Http::ConnectionManagerConfig
Expand Down
Loading