From 19b2f33618f20a2ba3d22a5bb10b1444f3a62ed8 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Wed, 12 Aug 2026 00:20:44 -0500 Subject: [PATCH] oauth2: respect user-configured retry_on in retry_policy `Http::Utility::convertCoreToRouteRetryPolicy()` gives precedence to the caller-supplied `retry_on` over the one set on the `core.v3.RetryPolicy`, so the filter's hardcoded `"5xx,gateway-error,connect-failure,reset"` silently discarded whatever the user configured. - Pass an empty string as the default so the configured `retry_on` is honored, guarded by `envoy.reloadable_features.oauth2_client_retries_respect_user_retry_on` (defaults to true) so the previous behavior can be restored. This mirrors the equivalent fix already made for the HTTP ext_authz client. - Add unit tests covering the configured value reaching the parsed policy, the guard-off path preserving the legacy conditions, and a `retry_policy` that omits `retry_on` now retrying nothing rather than inheriting those four conditions. Four other callers of that helper still pass a non-empty literal and carry the same latent issue; they are left alone here. Risk Level: low Testing: unit tests added Docs Changes: N/A Signed-off-by: Derek Argueta --- .../oauth2__respect-user-retry-on.rst | 7 +++ source/common/runtime/runtime_features.cc | 1 + .../extensions/filters/http/oauth2/filter.cc | 12 +++- .../filters/http/oauth2/filter_test.cc | 62 +++++++++++++++++++ 4 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 changelogs/current/minor_behavior_changes/oauth2__respect-user-retry-on.rst diff --git a/changelogs/current/minor_behavior_changes/oauth2__respect-user-retry-on.rst b/changelogs/current/minor_behavior_changes/oauth2__respect-user-retry-on.rst new file mode 100644 index 0000000000000..5d546ff9c084c --- /dev/null +++ b/changelogs/current/minor_behavior_changes/oauth2__respect-user-retry-on.rst @@ -0,0 +1,7 @@ +The OAuth2 filter now respects user-configured ``retry_on`` in :ref:`retry_policy +`. Previously, the +value was overridden with ``5xx,gateway-error,connect-failure,reset``, so a configured ``retry_on`` +had no effect on requests to the OAuth server. A ``retry_policy`` which does not set ``retry_on`` +now retries nothing, rather than silently inheriting those four conditions. Controlled by runtime +flag ``envoy.reloadable_features.oauth2_client_retries_respect_user_retry_on`` (defaults to +``true``); set to ``false`` to preserve the old behavior. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index a34d868adcc5e..6b021e3e2a420 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -93,6 +93,7 @@ RUNTIME_GUARD(envoy_reloadable_features_map_http_stream_reset_to_tcp_rst); RUNTIME_GUARD(envoy_reloadable_features_match_headers_individually); RUNTIME_GUARD(envoy_reloadable_features_mcp_filter_use_new_metadata_namespace); RUNTIME_GUARD(envoy_reloadable_features_mobile_use_network_observer_registry); +RUNTIME_GUARD(envoy_reloadable_features_oauth2_client_retries_respect_user_retry_on); // OAuth2 filter cookie decryption: when true (the default), decrypt() accepts legacy CBC // ciphertexts via the legacy AES-256-CBC fallback. When false, only "gcm."-prefixed ciphertexts // decrypt; legacy CBC cookies are rejected and the affected users are redirected to the OAuth diff --git a/source/extensions/filters/http/oauth2/filter.cc b/source/extensions/filters/http/oauth2/filter.cc index 6c839cb1104c5..465c6b0c044de 100644 --- a/source/extensions/filters/http/oauth2/filter.cc +++ b/source/extensions/filters/http/oauth2/filter.cc @@ -779,8 +779,16 @@ FilterConfig::FilterConfig( } if (proto_config.has_retry_policy()) { - auto retry_policy = Http::Utility::convertCoreToRouteRetryPolicy( - proto_config.retry_policy(), "5xx,gateway-error,connect-failure,reset"); + // convertCoreToRouteRetryPolicy()'s retry_on argument is an override, not a fallback: a + // non-empty value replaces the configured retry_on outright, so "" is what lets the user's + // value through. With the guard off, the override restores the legacy hardcoded conditions. + const std::string retry_on_override = + Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.oauth2_client_retries_respect_user_retry_on") + ? "" + : "5xx,gateway-error,connect-failure,reset"; + auto retry_policy = Http::Utility::convertCoreToRouteRetryPolicy(proto_config.retry_policy(), + retry_on_override); // Use the null validation visitor for the backward compatibility. The proto should already // been validated during the config load. auto parsed_policy_or_error = Router::RetryPolicyImpl::create( diff --git a/test/extensions/filters/http/oauth2/filter_test.cc b/test/extensions/filters/http/oauth2/filter_test.cc index e702b9fb5a617..625ac87a0ef85 100644 --- a/test/extensions/filters/http/oauth2/filter_test.cc +++ b/test/extensions/filters/http/oauth2/filter_test.cc @@ -418,6 +418,34 @@ class OAuth2Test : public testing::Test { return makeFilterConfig(p, secret_reader).value(); } + // Builds a minimal valid config whose retry_policy carries `retry_on` and three retries. Used to + // pin down which retry conditions the OAuth server requests end up with. + FilterConfigSharedPtr getConfigWithRetryPolicy(const std::string& retry_on) { + envoy::extensions::filters::http::oauth2::v3::OAuth2Config p; + auto* endpoint = p.mutable_token_endpoint(); + endpoint->set_cluster("auth.example.com"); + endpoint->set_uri("auth.example.com/_oauth"); + endpoint->mutable_timeout()->set_seconds(1); + p.set_redirect_uri("%REQ(:scheme)%://%REQ(:authority)%" + TEST_CALLBACK); + p.mutable_redirect_path_matcher()->mutable_path()->set_exact(TEST_CALLBACK); + p.set_authorization_endpoint("https://auth.example.com/oauth/authorize/"); + p.mutable_signout_path()->mutable_path()->set_exact("/_signout"); + + auto* retry_policy = p.mutable_retry_policy(); + retry_policy->mutable_num_retries()->set_value(3); + retry_policy->set_retry_on(retry_on); + + auto credentials = p.mutable_credentials(); + credentials->set_client_id(TEST_CLIENT_ID); + credentials->mutable_token_secret()->set_name("secret"); + credentials->mutable_hmac_secret()->set_name("hmac"); + + MessageUtil::validate(p, ProtobufMessage::getStrictValidationVisitor()); + + auto secret_reader = std::make_shared(); + return makeFilterConfig(p, secret_reader).value(); + } + // Test helpers exposing private OAuth2Filter methods. OAuth2Filter declares // `friend class OAuth2Test`, but `TEST_F(OAuth2Test, ...)` expands to a class // *derived* from OAuth2Test, and C++ friendship is not inherited — so the @@ -727,6 +755,40 @@ TEST_F(OAuth2Test, InvalidAuthorizationEndpoint) { "OAuth2 filter: invalid authorization endpoint URL 'INVALID_URL' in config.")); } +// A configured retry_on reaches the parsed policy instead of being overridden by the filter. +TEST_F(OAuth2Test, RetryPolicyRespectsConfiguredRetryOn) { + auto config = getConfigWithRetryPolicy("connect-failure,refused-stream"); + + ASSERT_NE(config->retryPolicy(), nullptr); + EXPECT_EQ(config->retryPolicy()->retryOn(), Router::RetryPolicy::RETRY_ON_CONNECT_FAILURE | + Router::RetryPolicy::RETRY_ON_REFUSED_STREAM); + EXPECT_EQ(config->retryPolicy()->numRetries(), 3); +} + +// With the guard off, the legacy hardcoded conditions still override the configured retry_on. +TEST_F(OAuth2Test, RetryPolicyLegacyRetryOnOverride) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.oauth2_client_retries_respect_user_retry_on", "false"}}); + + auto config = getConfigWithRetryPolicy("connect-failure,refused-stream"); + + ASSERT_NE(config->retryPolicy(), nullptr); + EXPECT_EQ(config->retryPolicy()->retryOn(), Router::RetryPolicy::RETRY_ON_5XX | + Router::RetryPolicy::RETRY_ON_GATEWAY_ERROR | + Router::RetryPolicy::RETRY_ON_CONNECT_FAILURE | + Router::RetryPolicy::RETRY_ON_RESET); +} + +// A retry_policy that omits retry_on no longer inherits the legacy conditions, so nothing is +// retried: num_retries on its own does not enable retries. +TEST_F(OAuth2Test, RetryPolicyWithoutRetryOnRetriesNothing) { + auto config = getConfigWithRetryPolicy(""); + + ASSERT_NE(config->retryPolicy(), nullptr); + EXPECT_EQ(config->retryPolicy()->retryOn(), 0); +} + // Verifies that the OAuth config is created with a default value for auth_scopes field when it is // not set in proto/yaml. TEST_F(OAuth2Test, DefaultAuthScope) {