From 1be73acfeaf6592561ad34f6d164863689b172e3 Mon Sep 17 00:00:00 2001 From: Mitch Denny Date: Sat, 7 Dec 2024 16:09:01 +1100 Subject: [PATCH] Handle missing settings.json for Codespaces/Devcontainers (#6880) --- .devcontainer/contributing/devcontainer.json | 3 +- .../Dashboard/DashboardLifecycleHook.cs | 6 ++- ...DevcontainerPortForwardingLifecycleHook.cs | 6 ++- ...elper.cs => DevcontainerSettingsWriter.cs} | 51 ++++++++++++++----- .../DistributedApplicationBuilder.cs | 1 + .../Dashboard/DashboardLifecycleHookTests.cs | 5 +- 6 files changed, 53 insertions(+), 19 deletions(-) rename src/Aspire.Hosting/Devcontainers/{DevcontainerPortForwardingHelper.cs => DevcontainerSettingsWriter.cs} (51%) diff --git a/.devcontainer/contributing/devcontainer.json b/.devcontainer/contributing/devcontainer.json index ec9874e7fe..8e11ab9b23 100644 --- a/.devcontainer/contributing/devcontainer.json +++ b/.devcontainer/contributing/devcontainer.json @@ -41,7 +41,8 @@ "extensions": [ "ms-dotnettools.csdevkit", "ms-azuretools.vscode-bicep", - "ms-azuretools.azure-dev" + "ms-azuretools.azure-dev", + "GitHub.copilot" ], "settings": { "remote.autoForwardPorts": true, diff --git a/src/Aspire.Hosting/Dashboard/DashboardLifecycleHook.cs b/src/Aspire.Hosting/Dashboard/DashboardLifecycleHook.cs index 19e8bdad02..26ae417137 100644 --- a/src/Aspire.Hosting/Dashboard/DashboardLifecycleHook.cs +++ b/src/Aspire.Hosting/Dashboard/DashboardLifecycleHook.cs @@ -34,7 +34,9 @@ internal sealed class DashboardLifecycleHook(IConfiguration configuration, IHostApplicationLifetime hostApplicationLifetime, CodespacesUrlRewriter codespaceUrlRewriter, IOptions codespacesOptions, - IOptions devcontainersOptions) : IDistributedApplicationLifecycleHook, IAsyncDisposable + IOptions devcontainersOptions, + DevcontainerSettingsWriter settingsWriter + ) : IDistributedApplicationLifecycleHook, IAsyncDisposable { private Task? _dashboardLogsTask; private CancellationTokenSource? _dashboardLogsCts; @@ -249,7 +251,7 @@ private void ConfigureAspireDashboardResource(IResource dashboardResource) if (codespacesOptions.Value.IsCodespace || devcontainersOptions.Value.IsDevcontainer) { - await DevcontainerPortForwardingHelper.SetPortAttributesAsync( + await settingsWriter.SetPortAttributesAsync( firstDashboardUrl.Port, firstDashboardUrl.Scheme, "aspire-dashboard", diff --git a/src/Aspire.Hosting/Devcontainers/DevcontainerPortForwardingLifecycleHook.cs b/src/Aspire.Hosting/Devcontainers/DevcontainerPortForwardingLifecycleHook.cs index 007fd5a284..4881c41b12 100644 --- a/src/Aspire.Hosting/Devcontainers/DevcontainerPortForwardingLifecycleHook.cs +++ b/src/Aspire.Hosting/Devcontainers/DevcontainerPortForwardingLifecycleHook.cs @@ -14,12 +14,14 @@ internal sealed class DevcontainerPortForwardingLifecycleHook : IDistributedAppl private readonly ILogger _hostingLogger; private readonly IOptions _codespacesOptions; private readonly IOptions _devcontainersOptions; + private readonly DevcontainerSettingsWriter _settingsWriter; - public DevcontainerPortForwardingLifecycleHook(ILoggerFactory loggerFactory, IOptions codespacesOptions, IOptions devcontainersOptions) + public DevcontainerPortForwardingLifecycleHook(ILoggerFactory loggerFactory, IOptions codespacesOptions, IOptions devcontainersOptions, DevcontainerSettingsWriter settingsWriter) { _hostingLogger = loggerFactory.CreateLogger("Aspire.Hosting"); _codespacesOptions = codespacesOptions; _devcontainersOptions = devcontainersOptions; + _settingsWriter = settingsWriter; } public async Task AfterEndpointsAllocatedAsync(DistributedApplicationModel appModel, CancellationToken cancellationToken) @@ -60,7 +62,7 @@ public async Task AfterEndpointsAllocatedAsync(DistributedApplicationModel appMo // and writing it each time. Its like this for now beause I need to use the logic // in a few places (here and when we print out the Dashboard URL) - but will need // to come back and optimize this to support some kind of batching. - await DevcontainerPortForwardingHelper.SetPortAttributesAsync( + await _settingsWriter.SetPortAttributesAsync( endpoint.AllocatedEndpoint!.Port, endpoint.UriScheme, $"{resource.Name}-{endpoint.Name}", diff --git a/src/Aspire.Hosting/Devcontainers/DevcontainerPortForwardingHelper.cs b/src/Aspire.Hosting/Devcontainers/DevcontainerSettingsWriter.cs similarity index 51% rename from src/Aspire.Hosting/Devcontainers/DevcontainerPortForwardingHelper.cs rename to src/Aspire.Hosting/Devcontainers/DevcontainerSettingsWriter.cs index 38bc0ccbbd..b77b546488 100644 --- a/src/Aspire.Hosting/Devcontainers/DevcontainerPortForwardingHelper.cs +++ b/src/Aspire.Hosting/Devcontainers/DevcontainerSettingsWriter.cs @@ -3,31 +3,36 @@ using System.Globalization; using System.Text.Json.Nodes; +using Aspire.Hosting.Devcontainers.Codespaces; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; namespace Aspire.Hosting.Devcontainers; -internal class DevcontainerPortForwardingHelper +internal class DevcontainerSettingsWriter(ILogger logger, IOptions codespaceOptions, IOptions devcontainerOptions) { private const string CodespaceSettingsPath = "/home/vscode/.vscode-remote/data/Machine/settings.json"; private const string LocalDevcontainerSettingsPath = "/home/vscode/.vscode-server/data/Machine/settings.json"; private const string PortAttributesFieldName = "remote.portsAttributes"; - private const int WriteLockTimeoutMs = 2000; - private static readonly SemaphoreSlim s_writeLock = new SemaphoreSlim(1); + private const int WriteLockTimeoutMs = 2000; + private readonly SemaphoreSlim _writeLock = new SemaphoreSlim(1); - public static async Task SetPortAttributesAsync(int port, string protocol, string label, CancellationToken cancellationToken = default) + public async Task SetPortAttributesAsync(int port, string protocol, string label, CancellationToken cancellationToken = default) { ArgumentNullException.ThrowIfNullOrEmpty(protocol); ArgumentNullException.ThrowIfNullOrEmpty(label); var settingsPath = GetSettingsPath(); - var acquired = await s_writeLock.WaitAsync(WriteLockTimeoutMs, cancellationToken).ConfigureAwait(false); + var acquired = await _writeLock.WaitAsync(WriteLockTimeoutMs, cancellationToken).ConfigureAwait(false); if (!acquired) { throw new DistributedApplicationException($"Failed to acquire semaphore for settings file: {settingsPath}"); } + await EnsureSettingsFileExists(settingsPath, cancellationToken).ConfigureAwait(false); + var settingsContent = await File.ReadAllTextAsync(settingsPath, cancellationToken).ConfigureAwait(false); var settings = (JsonObject)JsonObject.Parse(settingsContent)!; @@ -62,24 +67,46 @@ public static async Task SetPortAttributesAsync(int port, string protocol, strin settingsContent = settings.ToString(); await File.WriteAllTextAsync(settingsPath, settingsContent, cancellationToken).ConfigureAwait(false); - s_writeLock.Release(); + _writeLock.Release(); - static string GetSettingsPath() + string GetSettingsPath() { // For some reason the machine settings path is different between Codespaces and local Devcontainers - // so we probe for it here. We could use options to figure out which one to look for here but that - // would require taking a dependency on the options system here which seems like overkill. - if (File.Exists(CodespaceSettingsPath)) + // so we decide which one to use here based on the options. + if (codespaceOptions.Value.IsCodespace) { return CodespaceSettingsPath; } - else if (File.Exists(LocalDevcontainerSettingsPath)) + else if (devcontainerOptions.Value.IsDevcontainer) { return LocalDevcontainerSettingsPath; } else { - throw new DistributedApplicationException("Could not find a devcontainer settings file."); + throw new DistributedApplicationException("Codespaces or Devcontainer not detected."); + } + } + + async Task EnsureSettingsFileExists(string path, CancellationToken cancellationToken) + { + try + { + if (!File.Exists(path)) + { + // The extra ceremony here is to avoid accidentally overwriting the file if it was + // created after we checked for its existence. If the file exists when we go to write + // it then we will throw and log a warning, but otherwise continue executing. + using var stream = File.Open(path, FileMode.CreateNew); + using var writer = new StreamWriter(stream); + await writer.WriteAsync("{}".AsMemory(), cancellationToken).ConfigureAwait(false); + } + } + catch (IOException ex) when (ex.Message == $"The file '{path}' already exists.") + { + // This is OK, but it should be rare enough that if it starts happening we probably + // want to know about it in logs that end users submit so we know to take a closer + // look at what is going on. + logger.LogWarning("Race condition detected when creating Devcontainer settings file: {Path}", path); } } } diff --git a/src/Aspire.Hosting/DistributedApplicationBuilder.cs b/src/Aspire.Hosting/DistributedApplicationBuilder.cs index ae7860c781..889f4eed99 100644 --- a/src/Aspire.Hosting/DistributedApplicationBuilder.cs +++ b/src/Aspire.Hosting/DistributedApplicationBuilder.cs @@ -294,6 +294,7 @@ public DistributedApplicationBuilder(DistributedApplicationOptions options) _innerBuilder.Services.AddSingleton(); _innerBuilder.Services.AddHostedService(); _innerBuilder.Services.TryAddEnumerable(ServiceDescriptor.Singleton, ConfigureDevcontainersOptions>()); + _innerBuilder.Services.AddSingleton(); _innerBuilder.Services.TryAddLifecycleHook(); Eventing.Subscribe(BuiltInDistributedApplicationEventSubscriptionHandlers.InitializeDcpAnnotations); diff --git a/tests/Aspire.Hosting.Tests/Dashboard/DashboardLifecycleHookTests.cs b/tests/Aspire.Hosting.Tests/Dashboard/DashboardLifecycleHookTests.cs index 310f93dbd8..880b28e08a 100644 --- a/tests/Aspire.Hosting.Tests/Dashboard/DashboardLifecycleHookTests.cs +++ b/tests/Aspire.Hosting.Tests/Dashboard/DashboardLifecycleHookTests.cs @@ -108,7 +108,7 @@ private static DashboardLifecycleHook CreateHook( { codespacesOptions ??= Options.Create(new CodespacesOptions()); devcontainersOptions ??= Options.Create(new DevcontainersOptions()); - + var settingsWriter = new DevcontainerSettingsWriter(NullLogger.Instance, codespacesOptions, devcontainersOptions); var rewriter = new CodespacesUrlRewriter(codespacesOptions); return new DashboardLifecycleHook( @@ -124,7 +124,8 @@ private static DashboardLifecycleHook CreateHook( new TestHostApplicationLifetime(), rewriter, codespacesOptions, - devcontainersOptions + devcontainersOptions, + settingsWriter ); }