Skip to content
Closed
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
35 changes: 34 additions & 1 deletion api/envoy/config/bootstrap/v3/bootstrap.proto
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,37 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
// of the Envoy v3 configuration. See the :ref:`v3 configuration overview
// <config_overview_bootstrap>` for more detail.

// Global configuration for on-demand discovery behavior.
message OnDemandConfig {
// Controls behavior for requests with body data during VH discovery.
//
// **IMPORTANT LIMITATION**: When VH discovery brings in per-route filter
// configuration overrides, requests with body data cannot use stream recreation
// because it would lose the buffered request body. This creates inconsistent
// behavior where:
//
// - Bodyless requests (``GET``, ``HEAD``, etc.) receive per-route config overrides (YES)
// - Requests with body (``POST``, ``PUT``, etc.) do NOT receive per-route config overrides (NO)
//
// This setting allows you to choose the behavior for requests with body data:
//
// - If ``false`` (default): Requests with body continue with original filter chain
// configuration to preserve body data. Per-route overrides are NOT applied.
// This is the safest option but creates inconsistent behavior.
//
// - If ``true``: Requests with body will attempt stream recreation to apply
// per-route overrides, but this will LOSE the buffered request body data.
// Only enable this if you understand the data loss implications.
//
// **Recommendation**: Keep this ``false`` unless you have a specific need for
// consistent per-route configuration behavior and can tolerate request body loss.
// The ideal solution is to make stream recreation work with buffered bodies,
// but that requires significant architectural changes.
bool allow_body_data_loss_for_per_route_config = 1;
}

// Bootstrap :ref:`configuration overview <config_overview_bootstrap>`.
// [#next-free-field: 43]
// [#next-free-field: 44]
message Bootstrap {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.bootstrap.v2.Bootstrap";
Expand Down Expand Up @@ -421,6 +450,10 @@ message Bootstrap {
// Optional configuration for memory allocation manager.
// Memory releasing is only supported for `tcmalloc allocator <https://github.com/google/tcmalloc>`_.
MemoryAllocatorManager memory_allocator_manager = 41;

// Global configuration for on-demand discovery behavior that applies to all
// on-demand filters throughout the Envoy instance.
OnDemandConfig on_demand_config = 43;
}

// Administration interface :ref:`operations documentation
Expand Down
8 changes: 8 additions & 0 deletions changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,14 @@ minor_behavior_changes:

bug_fixes:
# *Changes expected to improve the state of the world and are unlikely to have negative effects*
- area: on_demand
change: |
Fixed on-demand VHDS (Virtual Host Discovery Service) to properly handle requests with request bodies.
Previously, requests with body data (``POST``, ``PUT``, etc.) would fail with 404 NR responses when triggering
on-demand virtual host discovery because the buffered request body was lost during stream processing.
This fix ensures request body data is properly preserved during VHDS discovery by avoiding stream recreation
when body data is present. Added new bootstrap configuration option ``allow_body_data_loss_for_per_route_config``
to control whether requests with bodies should prioritize body preservation or per-route config overrides. Fixes GitHub issue #18741.
- area: tcp_proxy
change: |
Fixed a bug where when a downstream TCP connection is created and the upstream connection is not fully established, no idle timeout
Expand Down
11 changes: 11 additions & 0 deletions docs/root/_configs/on_demand/allow-body-loss-config.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
name: envoy.filters.http.on_demand
typed_config:
"@type": type.googleapis.com/envoy.extensions.filters.http.on_demand.v3.OnDemand
odcds:
source:
ads: {}
timeout: 5s
# Bootstrap configuration required for this option
bootstrap:
on_demand_config:
allow_body_data_loss_for_per_route_config: true # WARNING: May lose request body data!
8 changes: 8 additions & 0 deletions docs/root/_configs/on_demand/basic-config.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
name: envoy.filters.http.on_demand
typed_config:
"@type": type.googleapis.com/envoy.extensions.filters.http.on_demand.v3.OnDemand
odcds:
source:
ads: {}
timeout: 5s
# allow_body_data_loss_for_per_route_config: false # Default - preserves request body data
49 changes: 49 additions & 0 deletions docs/root/_configs/on_demand/complete-config.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
static_resources:
listeners:
- name: listener_0
address:
socket_address:
address: 0.0.0.0
port_value: 10000
filter_chains:
- filters:
- name: envoy.filters.network.http_connection_manager
typed_config:
"@type": type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
stat_prefix: ingress_http
http_filters:
# On-demand filter must be placed before the router filter
- name: envoy.filters.http.on_demand
typed_config:
"@type": type.googleapis.com/envoy.extensions.filters.http.on_demand.v3.OnDemand
odcds:
source:
ads: {}
timeout: 5s
- name: envoy.filters.http.router
typed_config:
"@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router
route_config:
name: local_route
virtual_hosts:
- name: local_service
domains: ["*"]
routes:
- match:
prefix: "/"
route:
cluster: service_cluster
clusters:
- name: service_cluster
connect_timeout: 30s
type: LOGICAL_DNS
dns_lookup_family: V4_ONLY
load_assignment:
cluster_name: service_cluster
endpoints:
- lb_endpoints:
- endpoint:
address:
socket_address:
address: httpbin.org
port_value: 80
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,53 @@ the on demand CDS for requests using this virtual host or route. Conversely,
if :ref:`odcds <envoy_v3_api_field_extensions.filters.http.on_demand.v3.OnDemand.odcds>` is specified,
on demand CDS is enabled for requests using this virtual host or route.

Per-route configuration limitations
------------------------------------

.. warning::

**IMPORTANT LIMITATION**: When VH discovery brings in per-route filter configuration overrides,
requests with body data cannot use stream recreation because it would lose the buffered request body.
This creates inconsistent behavior where:

- Bodyless requests (``GET``, ``HEAD``, etc.) receive per-route config overrides (YES)
- Requests with body (``POST``, ``PUT``, etc.) do NOT receive per-route config overrides (NO)

The filter provides a configuration option ``allow_body_data_loss_for_per_route_config`` to control this behavior:

- If ``false`` (default): Requests with body continue with original filter chain configuration to preserve body data. Per-route overrides are NOT applied. This is the safest option but creates inconsistent behavior.

- If ``true``: Requests with body will attempt stream recreation to apply per-route overrides, but this will LOSE the buffered request body data. Only enable this if you understand the data loss implications.

**Recommendation**: Keep this ``false`` unless you have a specific need for consistent per-route configuration behavior and can tolerate request body loss.

Configuration
-------------
* This filter should be configured with the type URL ``type.googleapis.com/envoy.extensions.filters.http.on_demand.v3.OnDemand``.
* :ref:`v3 API reference <envoy_v3_api_msg_extensions.filters.http.on_demand.v3.OnDemand>`
* :ref:`v3 API reference for per route/virtual host config <envoy_v3_api_msg_extensions.filters.http.on_demand.v3.PerRouteConfig>`
* The filter should be placed before *envoy.filters.http.router* filter in the HttpConnectionManager's filter chain.

Example Configuration
~~~~~~~~~~~~~~~~~~~~~

Basic configuration with default behavior (preserves request body, may not apply per-route overrides):

.. literalinclude:: /_configs/on_demand/basic-config.yaml
:language: yaml
:linenos:
:caption: Basic on-demand filter configuration

Configuration allowing body data loss for consistent per-route behavior:

.. literalinclude:: /_configs/on_demand/allow-body-loss-config.yaml
:language: yaml
:linenos:
:caption: Configuration with body data loss allowed

Complete example showing the filter in an HTTP connection manager context:

.. literalinclude:: /_configs/on_demand/complete-config.yaml
:language: yaml
:linenos:
:caption: Complete Envoy configuration with on-demand filter
2 changes: 1 addition & 1 deletion source/extensions/filters/http/on_demand/config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ Http::FilterFactoryCb OnDemandFilterFactory::createFilterFactoryFromProtoTyped(
const std::string&, Server::Configuration::FactoryContext& context) {
OnDemandFilterConfigSharedPtr config = std::make_shared<OnDemandFilterConfig>(
proto_config, context.serverFactoryContext().clusterManager(),
context.messageValidationVisitor());
context.messageValidationVisitor(), context.serverFactoryContext());
return [config](Http::FilterChainFactoryCallbacks& callbacks) -> void {
callbacks.addStreamDecoderFilter(std::make_shared<OnDemandRouteUpdate>(config));
};
Expand Down
83 changes: 69 additions & 14 deletions source/extensions/filters/http/on_demand/on_demand_update.cc
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,15 @@ OnDemandFilterConfig::OnDemandFilterConfig(DecodeHeadersBehaviorPtr behavior)

OnDemandFilterConfig::OnDemandFilterConfig(
const envoy::extensions::filters::http::on_demand::v3::OnDemand& proto_config,
Upstream::ClusterManager& cm, ProtobufMessage::ValidationVisitor& validation_visitor)
: OnDemandFilterConfig(
createDecodeHeadersBehavior(getOdCdsConfig(proto_config), cm, validation_visitor)) {}
Upstream::ClusterManager& cm, ProtobufMessage::ValidationVisitor& validation_visitor,
Server::Configuration::ServerFactoryContext& server_context)
: behavior_(createDecodeHeadersBehavior(getOdCdsConfig(proto_config), cm, validation_visitor)),
allow_body_data_loss_for_per_route_config_(
server_context.bootstrap().has_on_demand_config()
? server_context.bootstrap()
.on_demand_config()
.allow_body_data_loss_for_per_route_config()
: false) {}

OnDemandFilterConfig::OnDemandFilterConfig(
const envoy::extensions::filters::http::on_demand::v3::PerRouteConfig& proto_config,
Expand Down Expand Up @@ -208,9 +214,11 @@ const OnDemandFilterConfig* OnDemandRouteUpdate::getConfig() {
}

Http::FilterDataStatus OnDemandRouteUpdate::decodeData(Buffer::Instance&, bool) {
return filter_iteration_state_ == Http::FilterHeadersStatus::StopIteration
? Http::FilterDataStatus::StopIterationAndWatermark
: Http::FilterDataStatus::Continue;
if (filter_iteration_state_ == Http::FilterHeadersStatus::StopIteration) {
has_body_data_ = true;
return Http::FilterDataStatus::StopIterationAndWatermark;
}
return Http::FilterDataStatus::Continue;
}

Http::FilterTrailersStatus OnDemandRouteUpdate::decodeTrailers(Http::RequestTrailerMap&) {
Expand Down Expand Up @@ -241,11 +249,42 @@ void OnDemandRouteUpdate::onRouteConfigUpdateCompletion(bool route_exists) {
return;
}

if (route_exists && // route can be resolved after an on-demand
// VHDS update
!callbacks_->decodingBuffer() && // Redirects with body not yet supported.
callbacks_->recreateStream(/*headers=*/nullptr)) {
return;
if (route_exists) {
// IMPORTANT: Two different processing paths based on request body presence
//
// The choice between continueDecoding() and recreateStream() is critical for correctness:
//
// Path 1: continueDecoding() - Used when request has body data
// - Preserves already-buffered request body during VHDS discovery
// - Continues processing with current stream to avoid losing buffered content
// - Essential for POST/PUT requests with payloads (fixes GitHub issue #17891)
// - LIMITATION: Per-route configuration overrides discovered during VH discovery
// are NOT applied because we cannot recreate the stream without losing body data
//
// Path 2: recreateStream() - Used when request has no body data
// - Recreates the entire request processing pipeline with updated per-route config
// - Ensures per-route configuration overrides are properly applied
// - Safe for GET requests and other body-less requests
//
// IDEAL SOLUTION: Make recreateStream() work with buffered body data to eliminate
// this trade-off. This would require significant architectural changes to preserve
// buffered body state during stream recreation, but would provide consistent
// per-route configuration behavior for all request types.

if (has_body_data_ && !getConfig()->allowBodyDataLossForPerRouteConfig()) {
// If we have body data and the configuration doesn't allow data loss,
// we cannot recreate the stream as it would lose the buffered body.
// Instead, we continue processing with the current stream which has the body already
// buffered. This means per-route configuration overrides will NOT be applied.
callbacks_->continueDecoding();
return;
}

// For requests without body data, or when explicitly allowing body data loss,
// we can use stream recreation to apply per-route config overrides
if (callbacks_->recreateStream(/*headers=*/nullptr)) {
return;
}
}

// route cannot be resolved after an on-demand VHDS update or
Expand All @@ -257,11 +296,27 @@ void OnDemandRouteUpdate::onClusterDiscoveryCompletion(
Upstream::ClusterDiscoveryStatus cluster_status) {
filter_iteration_state_ = Http::FilterHeadersStatus::Continue;
cluster_discovery_handle_.reset();
if (cluster_status == Upstream::ClusterDiscoveryStatus::Available &&
!callbacks_->decodingBuffer()) { // Redirects with body not yet supported.
if (cluster_status == Upstream::ClusterDiscoveryStatus::Available) {
callbacks_->downstreamCallbacks()->clearRouteCache();

// IMPORTANT: Same dual-path logic as in onRouteConfigUpdateCompletion()
// See detailed explanation above - this preserves body data but creates
// inconsistent per-route configuration behavior. The ideal solution would
// be to make recreateStream() work with buffered body data.

if (has_body_data_ && !getConfig()->allowBodyDataLossForPerRouteConfig()) {
// If we have body data and the configuration doesn't allow data loss,
// we cannot recreate the stream as it would lose the buffered body.
// Instead, we continue processing with the current stream which has the body already
// buffered. This means per-route configuration overrides will NOT be applied.
callbacks_->continueDecoding();
return;
}

// For requests without body data, or when explicitly allowing body data loss,
// we can use stream recreation to apply per-route config overrides
const Http::ResponseHeaderMap* headers = nullptr;
if (callbacks_->recreateStream(headers)) {
callbacks_->downstreamCallbacks()->clearRouteCache();
return;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,16 +42,21 @@ class OnDemandFilterConfig : public Router::RouteSpecificFilterConfig {
// Constructs config from extension's proto config.
OnDemandFilterConfig(
const envoy::extensions::filters::http::on_demand::v3::OnDemand& proto_config,
Upstream::ClusterManager& cm, ProtobufMessage::ValidationVisitor& validation_visitor);
Upstream::ClusterManager& cm, ProtobufMessage::ValidationVisitor& validation_visitor,
Server::Configuration::ServerFactoryContext& server_context);
// Constructs config from extension's per-route proto config.
OnDemandFilterConfig(
const envoy::extensions::filters::http::on_demand::v3::PerRouteConfig& proto_config,
Upstream::ClusterManager& cm, ProtobufMessage::ValidationVisitor& validation_visitor);

DecodeHeadersBehavior& decodeHeadersBehavior() const { return *behavior_; }
bool allowBodyDataLossForPerRouteConfig() const {
return allow_body_data_loss_for_per_route_config_;
}

private:
DecodeHeadersBehaviorPtr behavior_;
bool allow_body_data_loss_for_per_route_config_{false};
};

using OnDemandFilterConfigSharedPtr = std::shared_ptr<OnDemandFilterConfig>;
Expand Down Expand Up @@ -97,6 +102,7 @@ class OnDemandRouteUpdate : public Http::StreamDecoderFilter {
Upstream::ClusterDiscoveryCallbackHandlePtr cluster_discovery_handle_;
Envoy::Http::FilterHeadersStatus filter_iteration_state_{Http::FilterHeadersStatus::Continue};
bool decode_headers_active_{false};
bool has_body_data_{false};
};

} // namespace OnDemand
Expand Down
1 change: 1 addition & 0 deletions test/extensions/filters/http/on_demand/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ envoy_extension_cc_test(
"//test/mocks/http:http_mocks",
"//test/mocks/router:router_mocks",
"//test/mocks/runtime:runtime_mocks",
"//test/mocks/server:server_mocks",
"//test/mocks/upstream:upstream_mocks",
"//test/test_common:utility_lib",
],
Expand Down
Loading