ext_authz: fixes a bug in the retry_policy to respective user's config (#42232)

Author: agrawroh

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 <envoy_v3_api_field_config.core.v3.RetryPolicy.retry_on>`. 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

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);

source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc

@@ -112,9 +112,16 @@ absl::StatusOr<Router::RetryPolicyConstSharedPtr>
 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);

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<Network::Address::IpVersion> {
 public: