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 000000000000..5d546ff9c084 --- /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 47c237d4f271..bf6da662e90c 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -101,6 +101,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 6c839cb1104c..465c6b0c044d 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 e702b9fb5a61..625ac87a0ef8 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) {