-
Notifications
You must be signed in to change notification settings - Fork 746
Replace bind mount sources with environment placeholders in Docker Compose publishing #13223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
e293dc6
7f6e549
2580b5f
1e2c8ad
227dbfd
15a57ab
ffa24d4
7f35f75
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using Aspire.Hosting.ApplicationModel; | ||
|
|
||
| namespace Aspire.Hosting.Docker; | ||
|
|
||
| /// <summary> | ||
| /// Represents a captured environment variable that will be written to the .env file | ||
| /// adjacent to the Docker Compose file. | ||
| /// </summary> | ||
| public sealed class CapturedEnvironmentVariable | ||
| { | ||
| /// <summary> | ||
| /// Gets the name of the environment variable. | ||
| /// </summary> | ||
| public required string Name { get; init; } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the description for the environment variable. | ||
| /// </summary> | ||
| public string? Description { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the default value for the environment variable. | ||
| /// </summary> | ||
| public string? DefaultValue { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the source object that originated this environment variable. | ||
| /// This could be a <see cref="ParameterResource"/>, | ||
| /// <see cref="ContainerMountAnnotation"/>, or other source types. | ||
| /// </summary> | ||
| public object? Source { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the resource that this environment variable is associated with. | ||
| /// This is useful when the source is an annotation on a resource, allowing you to | ||
| /// identify which resource this environment variable is related to. | ||
| /// </summary> | ||
| public IResource? Resource { get; set; } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,31 +65,72 @@ private void ProcessEndpoints(DockerComposeServiceResource serviceResource) | |
| } | ||
| } | ||
|
|
||
| private static void ProcessVolumes(DockerComposeServiceResource serviceResource) | ||
| private void ProcessVolumes(DockerComposeServiceResource serviceResource) | ||
| { | ||
| if (!serviceResource.TargetResource.TryGetContainerMounts(out var mounts)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var bindMountIndex = 0; | ||
|
|
||
| foreach (var mount in mounts) | ||
| { | ||
| if (mount.Source is null || mount.Target is null) | ||
| { | ||
| throw new InvalidOperationException("Volume source and target must be set"); | ||
| } | ||
|
|
||
| var source = mount.Source; | ||
| var name = mount.Source; | ||
|
|
||
| // For bind mounts, create environment placeholders for the source path | ||
| // Skip the docker socket which should be left as-is for portability | ||
| if (mount.Type == ContainerMountType.BindMount && !IsDockerSocket(mount.Source)) | ||
| { | ||
| // Create environment variable name: {RESOURCE_NAME}_BINDMOUNT_{INDEX} | ||
| var envVarName = $"{serviceResource.Name.ToUpperInvariant().Replace("-", "_").Replace(".", "_")}_BINDMOUNT_{bindMountIndex}"; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Our "map X to env var name" logic is inconsistent and duplicated across a bunch of places. We should consider centralizing it and using the ATPI her.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do |
||
| bindMountIndex++; | ||
|
|
||
| // Add the placeholder to captured environment variables so it gets written to the .env file | ||
| // Use the original source path as the default value and pass the ContainerMountAnnotation as the source | ||
| var placeholder = environment.AddEnvironmentVariable( | ||
| envVarName, | ||
| description: $"Bind mount source for {serviceResource.Name}:{mount.Target}", | ||
| defaultValue: mount.Source, | ||
| source: mount, | ||
| resource: serviceResource.TargetResource); | ||
|
|
||
| // Log warning about host-specific path | ||
| logger.BindMountHostSpecificPath(serviceResource.Name, mount.Source, envVarName); | ||
|
|
||
| // Use the placeholder in the compose file | ||
| source = placeholder; | ||
| name = envVarName; | ||
| } | ||
|
|
||
| serviceResource.Volumes.Add(new Resources.ServiceNodes.Volume | ||
| { | ||
| Name = mount.Source, | ||
| Source = mount.Source, | ||
| Name = name, | ||
| Source = source, | ||
| Target = mount.Target, | ||
| Type = mount.Type == ContainerMountType.BindMount ? "bind" : "volume", | ||
| ReadOnly = mount.IsReadOnly | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Checks if the source path is the Docker socket path. | ||
| /// </summary> | ||
| private static bool IsDockerSocket(string source) | ||
| { | ||
| // Check for common Docker socket paths across different platforms | ||
| return source.Equals("/var/run/docker.sock", StringComparison.OrdinalIgnoreCase) || | ||
| source.Equals("//var/run/docker.sock", StringComparison.OrdinalIgnoreCase) || // WSL-style path | ||
| source.Equals(@"\\.\pipe\docker_engine", StringComparison.OrdinalIgnoreCase); // Windows named pipe | ||
| } | ||
|
|
||
| private static async Task ProcessEnvironmentVariablesAsync(DockerComposeServiceResource serviceResource, DistributedApplicationExecutionContext executionContext, CancellationToken cancellationToken) | ||
| { | ||
| if (serviceResource.TargetResource.TryGetAnnotationsOfType<EnvironmentCallbackAnnotation>(out var environmentCallbacks)) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should all these be { get; init; } instead?