Feature | Ignore enhanced routing tokens when feature disabled by server#3971
Feature | Ignore enhanced routing tokens when feature disabled by server#3971mdaigle wants to merge 1 commit intodev/mdaigle/enhanced-routing-2from
Conversation
There was a problem hiding this comment.
Pull request overview
Ports the enhanced-routing ACK-handling fix from #3922 to main so that enhanced routing ENVCHANGE tokens are only honored when the server actually acknowledges (and enables) the EnhancedRoutingSupport feature.
Changes:
- Updated
SqlConnectionInternalrouting logic to ignore enhanced routing info when the feature isn’t acknowledged/enabled by the server. - Adjusted enhanced routing simulated-server unit tests by refactoring setup and adding sync/async coverage for the routed-success case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs | Ignores enhanced routing tokens unless EnhancedRoutingSupport is acknowledged/enabled; allows processing EnhancedRoutingSupport ACK even when routing info already received. |
| src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionEnhancedRoutingTests.cs | Refactors routing tests with a helper server harness and adds theory-based coverage for sync/async routed connections. |
| // Check if we received enhanced routing info, but not the ack for the feature. | ||
| // In this case, we should ignore the routing info and connect to the current server. | ||
| if (!string.IsNullOrEmpty(RoutingInfo.DatabaseName) && !IsEnhancedRoutingSupportEnabled) | ||
| { | ||
| SqlClientEventSource.Log.TryTraceEvent( | ||
| $"SqlInternalConnectionTds.LoginNoFailover | " + | ||
| $"Ignoring enhanced routing info because the server did not acknowledge the feature."); | ||
| RoutingInfo = null; | ||
| break; | ||
| } |
There was a problem hiding this comment.
IsEnhancedRoutingSupportEnabled is never reset between login attempts. In a multi-hop redirect scenario, the flag could remain true from a previous server that acknowledged enhanced routing, causing the client to incorrectly honor enhanced routing info from a subsequent server that did not acknowledge (or disabled) the feature. Consider resetting IsEnhancedRoutingSupportEnabled (and any related per-server feature-negotiation state) at the start of each AttemptOneLogin / login attempt so the decision to honor enhanced routing tokens is based only on the current server’s ACK.
| // Check if we received enhanced routing info, but not the ack for the feature. | ||
| // In this case, we should ignore the routing info and connect to the current server. | ||
| if (!string.IsNullOrEmpty(RoutingInfo.DatabaseName) && !IsEnhancedRoutingSupportEnabled) | ||
| { | ||
| SqlClientEventSource.Log.TryTraceEvent( | ||
| $"SqlInternalConnectionTds.LoginWithFailover | " + | ||
| $"Ignoring enhanced routing info because the server did not acknowledge the feature."); | ||
| RoutingInfo = null; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Same issue as in LoginNoFailover: because IsEnhancedRoutingSupportEnabled is not reset per login attempt, it can carry over across redirect hops and cause enhanced routing tokens to be honored even when the current server didn’t acknowledge/enable the feature. Reset the flag at the start of each login attempt (e.g., in AttemptOneLogin) so routing decisions reflect only the current server’s negotiation result.
| [Theory] | ||
| [InlineData(FeatureExtensionBehavior.DoNotAcknowledge)] | ||
| [InlineData(FeatureExtensionBehavior.Disabled)] | ||
| public void ServerDoesNotRoute(FeatureExtensionBehavior behavior) | ||
| { | ||
| // Arrange | ||
| using TdsServer server = new(new()); | ||
| server.Start(); | ||
|
|
||
| string routingDatabaseName = Guid.NewGuid().ToString(); | ||
| bool clientProvidedCorrectDatabase = false; | ||
| server.OnLogin7Validated = loginToken => | ||
| { | ||
| clientProvidedCorrectDatabase = null == loginToken.Database; | ||
| }; | ||
|
|
||
| RoutingTdsServer router = new( | ||
| new RoutingTdsServerArguments() | ||
| { | ||
| RoutingTCPHost = "localhost", | ||
| RoutingTCPPort = (ushort)server.EndPoint.Port, | ||
| RequireReadOnly = false | ||
| }); | ||
| router.Start(); | ||
| router.EnhancedRoutingBehavior = FeatureExtensionBehavior.DoNotAcknowledge; | ||
|
|
||
| string connectionString = (new SqlConnectionStringBuilder() | ||
| { | ||
| DataSource = $"localhost,{router.EndPoint.Port}", | ||
| Encrypt = false, | ||
| ConnectTimeout = 10000 | ||
| }).ConnectionString; | ||
| using TestRoutingServers servers = new(behavior); | ||
|
|
||
| // Act | ||
| using SqlConnection connection = new(connectionString); | ||
| using SqlConnection connection = new(servers.ConnectionString); | ||
| connection.Open(); | ||
|
|
There was a problem hiding this comment.
ServerDoesNotRoute only exercises the synchronous Open() path. Since the product change affects connection establishment/routing logic, add coverage for OpenAsync() as well (e.g., include a useAsync parameter like RoutedConnection) to ensure behavior matches in both sync and async code paths.
1 participant
Ports #3922 to main