oauth2: respect user-configured retry_on in retry_policy

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
+<envoy_v3_api_field_extensions.filters.http.oauth2.v3.OAuth2Config.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.

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

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(

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<MockSecretReader>();
+    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) {