diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 1e3841585cb6..5107a534cc1c 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -63,6 +63,15 @@ minor_behavior_changes: change: | Check that the response header count and size is less than the configured limits after applying mutations and send a local reply if not. +- area: ext_authz + change: | + Fixed HTTP ext_authz client to correctly respect user-configured ``retry_on`` configuration in the + :ref:`retry_policy `. Previously, the + configured ``retry_on`` value was being overridden with hardcoded defaults + ``5xx,gateway-error,connect-failure,reset``, causing user-specified retry conditions to be ignored. + This behavior is controlled by the runtime flag + ``envoy.reloadable_features.ext_authz_http_client_retries_respect_user_retry_on`` which defaults to + ``true``. To preserve the old behavior, set this flag to ``false``. - area: ext_authz change: | Fixed HTTP ext_authz service to properly propagate headers (such as ``set-cookie``) back to clients. The diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 388178bb10f5..771890b86ce7 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -41,6 +41,7 @@ RUNTIME_GUARD(envoy_reloadable_features_disallow_quic_client_udp_mmsg); RUNTIME_GUARD(envoy_reloadable_features_enable_cel_regex_precompilation); RUNTIME_GUARD(envoy_reloadable_features_enable_compression_bomb_protection); RUNTIME_GUARD(envoy_reloadable_features_enable_new_query_param_present_match_behavior); +RUNTIME_GUARD(envoy_reloadable_features_ext_authz_http_client_retries_respect_user_retry_on); // Ignore the automated "remove this flag" issue: we should keep this for 1 year. Confirm with // @yanjunxiang-google before removing. RUNTIME_GUARD(envoy_reloadable_features_ext_proc_fail_close_spurious_resp); diff --git a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc index 9f492a92bc13..7400a7c14fbc 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc @@ -112,9 +112,16 @@ absl::StatusOr createRetryPolicy(const envoy::config::core::v3::RetryPolicy& core_retry_policy, Server::Configuration::CommonFactoryContext& context) { // Convert core retry policy to route retry policy and create the implementation. + // By default when runtime flag is true, pass empty string to respect user's configured + // retry_on, not override it. When flag is false, use hardcoded defaults for backwards + // compatibility. + const std::string default_retry_on = + Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.ext_authz_http_client_retries_respect_user_retry_on") + ? "" + : "5xx,gateway-error,connect-failure,reset"; envoy::config::route::v3::RetryPolicy route_retry_policy = - Http::Utility::convertCoreToRouteRetryPolicy(core_retry_policy, - "5xx,gateway-error,connect-failure,reset"); + Http::Utility::convertCoreToRouteRetryPolicy(core_retry_policy, default_retry_on); return Router::RetryPolicyImpl::create(route_retry_policy, context.messageValidationVisitor(), context); diff --git a/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc b/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc index 9f413dbf12ec..e27f46351226 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc @@ -1777,6 +1777,177 @@ TEST_P(ExtAuthzHttpIntegrationTest, HttpRetryPolicy) { cleanup(); } +// Test that user-configured retry_on conditions are respected in HTTP ext_authz. +TEST_P(ExtAuthzHttpIntegrationTest, HttpRetryPolicyRespectedNotOverridden) { + config_helper_.addConfigModifier([this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + auto* ext_authz_cluster = bootstrap.mutable_static_resources()->add_clusters(); + ext_authz_cluster->MergeFrom(bootstrap.static_resources().clusters()[0]); + ext_authz_cluster->set_name("ext_authz"); + + // Configure retry_on with "retriable-4xx" which is not one of the hardcoded values we were + // setting previously, "5xx,gateway-error,connect-failure,reset". + // Before the fix, this would be ignored and only 5xx/gateway-error/etc would trigger retries. + // After the fix, 4XX should trigger a retry as well. + const std::string ext_authz_config = R"EOF( + http_service: + server_uri: + uri: "ext_authz:9000" + cluster: "ext_authz" + timeout: 300s + authorization_response: + allowed_upstream_headers: + patterns: + - exact: baz + retry_policy: + retry_on: "retriable-4xx" + num_retries: 1 + retry_back_off: + base_interval: 0.01s + max_interval: 0.1s + failure_mode_allow: false + )EOF"; + TestUtility::loadFromYaml(ext_authz_config, proto_config_); + proto_config_.set_encode_raw_headers(encodeRawHeaders()); + + envoy::extensions::filters::network::http_connection_manager::v3::HttpFilter ext_authz_filter; + ext_authz_filter.set_name("envoy.filters.http.ext_authz"); + ext_authz_filter.mutable_typed_config()->PackFrom(proto_config_); + + config_helper_.prependFilter(MessageUtil::getJsonStringFromMessageOrError(ext_authz_filter)); + }); + + HttpIntegrationTest::initialize(); + + auto conn = makeClientConnection(lookupPort("http")); + codec_client_ = makeHttpConnection(std::move(conn)); + const auto headers = Http::TestRequestHeaderMapImpl{ + {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; + response_ = codec_client_->makeHeaderOnlyRequest(headers); + + // Wait for the first ext_authz request. + AssertionResult result = + fake_upstreams_.back()->waitForHttpConnection(*dispatcher_, fake_ext_authz_connection_); + RELEASE_ASSERT(result, result.message()); + result = fake_ext_authz_connection_->waitForNewStream(*dispatcher_, ext_authz_request_); + RELEASE_ASSERT(result, result.message()); + result = ext_authz_request_->waitForEndStream(*dispatcher_); + RELEASE_ASSERT(result, result.message()); + + // Send a 409 Conflict error response to trigger retry (retriable-4xx). + // Before the fixes we made, no retry happens because "retriable-4xx" was overridden by the + // hardcoded defaults. After the fix, retry should happen because user's "retriable-4xx" + // gets respected and not overridden. + Http::TestResponseHeaderMapImpl error_response_headers{{":status", "409"}}; + ext_authz_request_->encodeHeaders(error_response_headers, true); + + // Wait for the retry request to the ext_authz server. + // This will TIMEOUT before the fixes we made, and SUCCEED after the fixes. + FakeStreamPtr ext_authz_retry_request; + result = fake_ext_authz_connection_->waitForNewStream(*dispatcher_, ext_authz_retry_request); + RELEASE_ASSERT(result, result.message()); + result = ext_authz_retry_request->waitForEndStream(*dispatcher_); + RELEASE_ASSERT(result, result.message()); + + // Send a successful response on the retry. + Http::TestResponseHeaderMapImpl success_response_headers{ + {":status", "200"}, + {"baz", "test-value"}, + }; + ext_authz_retry_request->encodeHeaders(success_response_headers, true); + + // The request should now proceed to upstream. + result = fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_); + RELEASE_ASSERT(result, result.message()); + result = fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_); + RELEASE_ASSERT(result, result.message()); + result = upstream_request_->waitForEndStream(*dispatcher_); + RELEASE_ASSERT(result, result.message()); + + // Verify the request was modified by the successful ext_authz response. + EXPECT_THAT(upstream_request_->headers(), ContainsHeader("baz", "test-value")); + + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, true); + ASSERT_TRUE(response_->waitForEndStream()); + EXPECT_TRUE(response_->complete()); + EXPECT_EQ("200", response_->headers().getStatusValue()); + + cleanup(); +} + +// Test that when the runtime flag is disabled, we preserve the old behavior of overriding +// user-configured retry_on with hardcoded defaults. +TEST_P(ExtAuthzHttpIntegrationTest, HttpRetryPolicyOldBehaviorWithFlagDisabled) { + // Disable the runtime flag to test old behavior. + config_helper_.addRuntimeOverride( + "envoy.reloadable_features.ext_authz_http_client_retries_respect_user_retry_on", "false"); + + config_helper_.addConfigModifier([this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + auto* ext_authz_cluster = bootstrap.mutable_static_resources()->add_clusters(); + ext_authz_cluster->MergeFrom(bootstrap.static_resources().clusters()[0]); + ext_authz_cluster->set_name("ext_authz"); + + // Configure retry_on with "retriable-4xx" which should be ignored in old behavior. + // With the flag disabled, the hardcoded defaults "5xx,gateway-error,connect-failure,reset" + // override the user config, so 409 (4xx) should NOT trigger a retry. + const std::string ext_authz_config = R"EOF( + http_service: + server_uri: + uri: "ext_authz:9000" + cluster: "ext_authz" + timeout: 300s + authorization_response: + allowed_upstream_headers: + patterns: + - exact: baz + retry_policy: + retry_on: "retriable-4xx" + num_retries: 1 + retry_back_off: + base_interval: 0.01s + max_interval: 0.1s + failure_mode_allow: false + )EOF"; + TestUtility::loadFromYaml(ext_authz_config, proto_config_); + proto_config_.set_encode_raw_headers(encodeRawHeaders()); + + envoy::extensions::filters::network::http_connection_manager::v3::HttpFilter ext_authz_filter; + ext_authz_filter.set_name("envoy.filters.http.ext_authz"); + ext_authz_filter.mutable_typed_config()->PackFrom(proto_config_); + + config_helper_.prependFilter(MessageUtil::getJsonStringFromMessageOrError(ext_authz_filter)); + }); + + HttpIntegrationTest::initialize(); + + auto conn = makeClientConnection(lookupPort("http")); + codec_client_ = makeHttpConnection(std::move(conn)); + const auto headers = Http::TestRequestHeaderMapImpl{ + {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; + response_ = codec_client_->makeHeaderOnlyRequest(headers); + + // Wait for the first ext_authz request. + AssertionResult result = + fake_upstreams_.back()->waitForHttpConnection(*dispatcher_, fake_ext_authz_connection_); + RELEASE_ASSERT(result, result.message()); + result = fake_ext_authz_connection_->waitForNewStream(*dispatcher_, ext_authz_request_); + RELEASE_ASSERT(result, result.message()); + result = ext_authz_request_->waitForEndStream(*dispatcher_); + RELEASE_ASSERT(result, result.message()); + + // Send a 409 Conflict error response. + // With the flag disabled (old behavior), the user's "retriable-4xx" is overridden by + // hardcoded defaults, so NO retry should happen for this 4xx response. + Http::TestResponseHeaderMapImpl error_response_headers{{":status", "409"}}; + ext_authz_request_->encodeHeaders(error_response_headers, true); + + // The client should receive the denial response directly without any retry. + ASSERT_TRUE(response_->waitForEndStream()); + EXPECT_TRUE(response_->complete()); + EXPECT_EQ("409", response_->headers().getStatusValue()); + + cleanup(); +} + class ExtAuthzLocalReplyIntegrationTest : public HttpIntegrationTest, public TestWithParam { public: