From 52797e949544243a4e00e17cd360a9d984395806 Mon Sep 17 00:00:00 2001 From: Jason Ginchereau Date: Mon, 26 Jan 2026 11:57:20 -1000 Subject: [PATCH] Fix tunnel creation ID conflict handling --- cs/src/Management/TunnelManagementClient.cs | 83 ++++++------------- .../TunnelManagementClientTests.cs | 56 +++++++++++++ ts/src/connections/package.json | 4 +- ts/src/management/package.json | 2 +- .../management/tunnelManagementHttpClient.ts | 71 ++++------------ ts/test/tunnels-test/tunnelManagementTests.ts | 70 ++++++++++++++++ 6 files changed, 169 insertions(+), 117 deletions(-) diff --git a/cs/src/Management/TunnelManagementClient.cs b/cs/src/Management/TunnelManagementClient.cs index 60e46a3c..3e1e9762 100644 --- a/cs/src/Management/TunnelManagementClient.cs +++ b/cs/src/Management/TunnelManagementClient.cs @@ -279,8 +279,8 @@ private static void ValidateHttpHandler(HttpMessageHandler httpHandler) { throw new NotSupportedException( $"Unsupported HTTP handler type: {httpHandler?.GetType().Name}. " + - "HTTP handler chain must consist of 0 or more DelegatingHandlers " + - "ending with a HttpClientHandler."); + $"HTTP handler chain must consist of 0 or more {nameof(DelegatingHandler)}s " + + $"ending with a {nameof(HttpClientHandler)} or {nameof(SocketsHttpHandler)}."); } } @@ -1055,14 +1055,15 @@ public async Task CreateTunnelAsync( Requires.NotNull(tunnel, nameof(tunnel)); options ??= new TunnelRequestOptions(); options.AdditionalHeaders ??= new List>(); - options.AdditionalHeaders = options.AdditionalHeaders.Append(new KeyValuePair("If-None-Match", "*")); + options.AdditionalHeaders = options.AdditionalHeaders.Append( + new KeyValuePair("If-None-Match", "*")); var tunnelId = tunnel.TunnelId; var idGenerated = string.IsNullOrEmpty(tunnelId); if (idGenerated) { tunnel.TunnelId = IdGeneration.GenerateTunnelId(); } - for (int retries = 0; retries <= CreateNameRetries; retries++) + for (int retries = 0; ; retries++) { try { @@ -1079,25 +1080,14 @@ public async Task CreateTunnelAsync( PreserveAccessTokens(tunnel, result); return result!; } - catch (UnauthorizedAccessException) when (idGenerated && retries < CreateNameRetries) // The tunnel ID was already taken. + catch (InvalidOperationException ex) + when (ex.InnerException is HttpRequestException hrex && + hrex.StatusCode == HttpStatusCode.Conflict && + idGenerated && retries < CreateNameRetries) // The tunnel ID was already taken. { tunnel.TunnelId = IdGeneration.GenerateTunnelId(); } } - - // This code is unreachable, but the compiler still requires it. - var result2 = await this.SendTunnelRequestAsync( - HttpMethod.Put, - tunnel, - ManageAccessTokenScope, - path: null, - query: GetApiQuery(), - options, - ConvertTunnelForRequest(tunnel), - cancellation, - true); - PreserveAccessTokens(tunnel, result2); - return result2!; } /// @@ -1108,48 +1098,25 @@ public async Task CreateOrUpdateTunnelAsync( { Requires.NotNull(tunnel, nameof(tunnel)); - var tunnelId = tunnel.TunnelId; - var idGenerated = string.IsNullOrEmpty(tunnelId); - if (idGenerated) + if (string.IsNullOrEmpty(tunnel.TunnelId)) { - tunnel.TunnelId = IdGeneration.GenerateTunnelId(); - } - for (int retries = 0; retries <= CreateNameRetries; retries++) - { - try - { - var result = await this.SendTunnelRequestAsync( - HttpMethod.Put, - tunnel, - ManageAccessTokenScope, - path: null, - query: GetApiQuery(), - options, - ConvertTunnelForRequest(tunnel), - cancellation, - true); - PreserveAccessTokens(tunnel, result); - return result!; - } - catch (UnauthorizedAccessException) when (idGenerated && retries < 3) // The tunnel ID was already taken. - { - tunnel.TunnelId = IdGeneration.GenerateTunnelId(); - } + // When no tunnel ID is specified, a randomly-generated ID should not unintionally + // update an existing tunnel. + return await CreateTunnelAsync(tunnel, options, cancellation); } - // This code is unreachable, but the compiler still requires it. - var result2 = await this.SendTunnelRequestAsync( - HttpMethod.Put, - tunnel, - ManageAccessTokenScope, - path: null, - query: GetApiQuery(), - options, - ConvertTunnelForRequest(tunnel), - cancellation, - true); - PreserveAccessTokens(tunnel, result2); - return result2!; + var result = await this.SendTunnelRequestAsync( + HttpMethod.Put, + tunnel, + ManageAccessTokenScope, + path: null, + query: GetApiQuery(), + options, + ConvertTunnelForRequest(tunnel), + cancellation, + true); + PreserveAccessTokens(tunnel, result); + return result!; } /// diff --git a/cs/test/TunnelsSDK.Test/TunnelManagementClientTests.cs b/cs/test/TunnelsSDK.Test/TunnelManagementClientTests.cs index c1ae8e62..0bcf1f8e 100644 --- a/cs/test/TunnelsSDK.Test/TunnelManagementClientTests.cs +++ b/cs/test/TunnelsSDK.Test/TunnelManagementClientTests.cs @@ -144,6 +144,62 @@ public async Task PreserveAccessTokens() TunnelAccessScopes.Manage, "manage-token-2"), item)); // updated } + [Fact] + public async Task CreateTunnelRetriesOnGeneratedIdConflict() + { + var requestTunnel = new Tunnel + { + ClusterId = ClusterId, + + // Tunnel ID is not set, so the client will generate one. + }; + + var callCount = 0; + string firstTunnelId = null; + string secondTunnelId = null; + + var handler = new MockHttpMessageHandler( + async (message, ct) => + { + callCount++; + Assert.NotNull(message.Content); + + var sentTunnel = await message.Content!.ReadFromJsonAsync(cancellationToken: ct); + Assert.NotNull(sentTunnel); + Assert.False(string.IsNullOrEmpty(sentTunnel!.TunnelId)); + + if (callCount == 1) + { + firstTunnelId = sentTunnel.TunnelId; + var conflictResult = new HttpResponseMessage(HttpStatusCode.Conflict); + conflictResult.RequestMessage = message; + return conflictResult; + } + + secondTunnelId = sentTunnel.TunnelId; + + var responseTunnel = new Tunnel + { + TunnelId = sentTunnel.TunnelId, + ClusterId = ClusterId, + }; + + var result = new HttpResponseMessage(HttpStatusCode.OK); + result.Content = JsonContent.Create(responseTunnel); + return result; + }); + + var client = new TunnelManagementClient(this.userAgent, null, this.tunnelServiceUri, handler); + + var resultTunnel = await client.CreateTunnelAsync(requestTunnel, options: null, this.timeout); + + Assert.Equal(2, callCount); + Assert.NotNull(firstTunnelId); + Assert.NotNull(secondTunnelId); + Assert.Equal(secondTunnelId, resultTunnel.TunnelId); + Assert.Equal(resultTunnel.TunnelId, requestTunnel.TunnelId); + } + [Fact] public async Task HandleFirewallResponse() { diff --git a/ts/src/connections/package.json b/ts/src/connections/package.json index 89b40383..fab103f3 100644 --- a/ts/src/connections/package.json +++ b/ts/src/connections/package.json @@ -18,8 +18,8 @@ "buffer": "^5.2.1", "debug": "^4.1.1", "vscode-jsonrpc": "^4.0.0", - "@microsoft/dev-tunnels-contracts": "^1.3.6", - "@microsoft/dev-tunnels-management": "^1.3.6", + "@microsoft/dev-tunnels-contracts": "^1.3.7", + "@microsoft/dev-tunnels-management": "^1.3.7", "@microsoft/dev-tunnels-ssh": "^3.12.12", "@microsoft/dev-tunnels-ssh-tcp": "^3.12.12", "uuid": "^3.3.3", diff --git a/ts/src/management/package.json b/ts/src/management/package.json index 2bc2a21b..47c13026 100644 --- a/ts/src/management/package.json +++ b/ts/src/management/package.json @@ -18,7 +18,7 @@ "buffer": "^5.2.1", "debug": "^4.1.1", "vscode-jsonrpc": "^4.0.0", - "@microsoft/dev-tunnels-contracts": "^1.3.6", + "@microsoft/dev-tunnels-contracts": "^1.3.7", "axios": "^1.8.4" } } diff --git a/ts/src/management/tunnelManagementHttpClient.ts b/ts/src/management/tunnelManagementHttpClient.ts index 09cabd48..94d8c150 100644 --- a/ts/src/management/tunnelManagementHttpClient.ts +++ b/ts/src/management/tunnelManagementHttpClient.ts @@ -302,7 +302,7 @@ export class TunnelManagementHttpClient implements TunnelManagementClient { if (idGenerated) { tunnel.tunnelId = IdGeneration.generateTunnelId(); } - for (let i = 0;i<=createNameRetries; i++){ + for (let retries = 0; ; retries++){ try { const result = (await this.sendTunnelRequest( 'PUT', @@ -320,7 +320,9 @@ export class TunnelManagementHttpClient implements TunnelManagementClient { parseTunnelDates(result); return result; } catch (error) { - if (idGenerated) { + if ((error as AxiosError)?.response?.status === 409 && + idGenerated && retries < createNameRetries + ) { // The tunnel ID was generated and there was a conflict. // Try again with a new ID. tunnel.tunnelId = IdGeneration.generateTunnelId(); @@ -329,73 +331,30 @@ export class TunnelManagementHttpClient implements TunnelManagementClient { } } } - - const result2 = (await this.sendTunnelRequest( - 'PUT', - tunnel, - manageAccessTokenScope, - undefined, - undefined, - options, - this.convertTunnelForRequest(tunnel), - undefined, - cancellation, - true, - ))!; - preserveAccessTokens(tunnel, result2); - parseTunnelDates(result2); - return result2; } public async createOrUpdateTunnel(tunnel: Tunnel, options?: TunnelRequestOptions, cancellation?: CancellationToken): Promise { const tunnelId = tunnel.tunnelId; - const idGenerated = tunnelId === undefined || tunnelId === null || tunnelId === ''; - if (idGenerated) { - tunnel.tunnelId = IdGeneration.generateTunnelId(); - } - for (let i = 0;i<=createNameRetries; i++){ - try { - const result = (await this.sendTunnelRequest( - 'PUT', - tunnel, - manageAccessTokenScope, - undefined, - undefined, - options, - this.convertTunnelForRequest(tunnel), - undefined, - cancellation, - true, - ))!; - preserveAccessTokens(tunnel, result); - parseTunnelDates(result); - return result; - } catch (error) { - if (idGenerated) { - // The tunnel ID was generated and there was a conflict. - // Try again with a new ID. - tunnel.tunnelId = IdGeneration.generateTunnelId(); - } else { - throw error; - } - } + if (!tunnelId) { + // When no tunnel ID is specified, a randomly-generated ID should not unintionally + // update an existing tunnel. + return this.createTunnel(tunnel, options, cancellation); } - - const result2 = (await this.sendTunnelRequest( + const result = (await this.sendTunnelRequest( 'PUT', tunnel, manageAccessTokenScope, undefined, - "forceCreate=true", + undefined, options, this.convertTunnelForRequest(tunnel), undefined, cancellation, true, ))!; - preserveAccessTokens(tunnel, result2); - parseTunnelDates(result2); - return result2; + preserveAccessTokens(tunnel, result); + parseTunnelDates(result); + return result; } public async updateTunnel(tunnel: Tunnel, options?: TunnelRequestOptions, cancellation?: CancellationToken): Promise { @@ -745,8 +704,8 @@ export class TunnelManagementHttpClient implements TunnelManagementClient { this.raiseReportProgress(TunnelProgress.CompletedSendTunnelRequest); return result; } catch (error) { - if (/certificate/i.test((error as AxiosError).message)) { - const originalErrorMessage = (error as AxiosError).message; + if (/certificate/i.test((error as AxiosError).message)) { + const originalErrorMessage = (error as AxiosError).message; throw new Error("Tunnel service HTTPS certificate is invalid. This may be caused by the use of a " + "self-signed certificate or a firewall intercepting the connection. " + originalErrorMessage + ". "); } diff --git a/ts/test/tunnels-test/tunnelManagementTests.ts b/ts/test/tunnels-test/tunnelManagementTests.ts index f4add5f3..bc23fcb5 100644 --- a/ts/test/tunnels-test/tunnelManagementTests.ts +++ b/ts/test/tunnels-test/tunnelManagementTests.ts @@ -245,6 +245,76 @@ export class TunnelManagementTests { assert.strictEqual(resultTunnel.accessTokens['manage'], 'manage-token-2'); // updated } + @test + public async createTunnelRetriesOnGeneratedIdConflict() { + const requestTunnel = { + clusterId: 'clusterId', + }; + + let callCount = 0; + let firstTunnelId: string | undefined; + let secondTunnelId: string | undefined; + + const conflictError = new AxiosError(); + conflictError.config = { + url: TunnelManagementTests.testServiceUri, + headers: new AxiosHeaders(), + }; + conflictError.response = { + status: 409, + statusText: 'Conflict', + headers: new AxiosHeaders(), + data: undefined, + config: conflictError.config, + }; + + const originalAxiosRequest = (this.managementClient).axiosRequest; + (this.managementClient).axiosRequest = async ( + config: AxiosRequestConfig, + cancellation: CancellationToken, + ): Promise => { + this.lastRequest = { + method: config.method as Method, + uri: config.url || '', + data: config.data, + config, + }; + callCount++; + + const sentTunnel = config.data as Tunnel; + if (callCount === 1) { + firstTunnelId = sentTunnel?.tunnelId; + throw conflictError; + } + + secondTunnelId = sentTunnel?.tunnelId; + + return { + data: { + tunnelId: sentTunnel?.tunnelId, + clusterId: requestTunnel.clusterId, + }, + status: 200, + statusText: 'OK', + headers: {}, + config, + } as AxiosResponse; + }; + + try { + const resultTunnel = await this.managementClient.createTunnel(requestTunnel); + + assert.strictEqual(callCount, 2); + assert.ok(firstTunnelId); + assert.ok(secondTunnelId); + assert.notStrictEqual(firstTunnelId, secondTunnelId); + assert.strictEqual(resultTunnel.tunnelId, secondTunnelId); + assert.strictEqual(requestTunnel.tunnelId, secondTunnelId); + } finally { + (this.managementClient).axiosRequest = originalAxiosRequest; + } + } + @test public async handleFirewallResponse() { const requestTunnel = {