Skip to content

Bind mounts support for docker compose (#245)#286

Open
dombrovsky wants to merge 1 commit intoprom3theu5:mainfrom
dombrovsky:dombrovsky/245
Open

Bind mounts support for docker compose (#245)#286
dombrovsky wants to merge 1 commit intoprom3theu5:mainfrom
dombrovsky:dombrovsky/245

Conversation

@dombrovsky
Copy link
Contributor

@dombrovsky dombrovsky commented Feb 15, 2025

User description

resolves #245


PR Type

Enhancement, Bug fix


Description

  • Added support for bind mounts in Docker Compose manifests.

  • Introduced BindMount model and related interface for resource handling.

  • Enhanced volume mapping logic to include bind mounts.

  • Fixed issue where bind mounts were ignored during manifest application.


Changes walkthrough 📝

Relevant files
Enhancement
GenerateDockerComposeManifestAction.cs
Refined volume creation logic for Docker Compose                 

src/Aspirate.Commands/Actions/Manifests/GenerateDockerComposeManifestAction.cs

  • Updated volume creation logic to exclude paths containing '/'.
  • Ensured only valid volume names are added to the list.
  • +1/-1     
    ContainerProcessor.cs
    Support bind mounts in compose entry creation                       

    src/Aspirate.Processors/Resources/AbstractProcessors/ContainerProcessor.cs

  • Updated CreateComposeEntry to include bind mounts in volume mapping.
  • Used OutputPath for relative path resolution in bind mounts.
  • +1/-1     
    ResourceExtensions.cs
    Extend volume mapping to include bind mounts                         

    src/Aspirate.Shared/Extensions/ResourceExtensions.cs

  • Enhanced MapComposeVolumes to handle bind mounts.
  • Added relative path resolution for bind mount sources.
  • Refactored logic to differentiate volumes and bind mounts.
  • +9/-7     
    BindMount.cs
    Add `BindMount` model for bind mount support                         

    src/Aspirate.Shared/Models/AspireManifests/Components/V0/BindMount.cs

  • Introduced BindMount class for bind mount configurations.
  • Added properties for source, target, and read-only flag.
  • +13/-0   
    ContainerResource.cs
    Extend `ContainerResource` to support bind mounts               

    src/Aspirate.Shared/Models/AspireManifests/Components/V0/Container/ContainerResource.cs

  • Added BindMounts property to ContainerResource.
  • Implemented IResourceWithBindMounts interface.
  • +6/-2     
    IResourceWithBindMounts.cs
    Add interface for resources with bind mounts                         

    src/Aspirate.Shared/Models/AspireManifests/Interfaces/IResourceWithBindMounts.cs

  • Introduced IResourceWithBindMounts interface.
  • Defined BindMounts property for resources.
  • +6/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @qodo-code-review
    Copy link

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    245 - PR Code Verified

    Compliant requirements:

    • Support bind mount configurations in container deployments
    • Support the format: .WithBindMount("../envoy/envoy.yaml", "/etc/envoy/envoy.yaml")

    Requires further human verification:

    • Properly apply bind mount configurations when using aspirate
    • Verify that custom configurations via bind mounts work as expected
    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    File Access:
    The bind mount implementation could potentially expose sensitive files from the host system to the container if proper validation of mount paths is not implemented. Consider adding path validation and restrictions on allowed mount locations.

    ⚡ Recommended focus areas for review

    Volume Filtering

    The new volume filtering logic might incorrectly exclude valid bind mount paths. The condition !volume.Split(':')[0].Contains('/') could prevent legitimate bind mount paths from being processed.

    volumes.AddRange(service.Volumes.Where(volume => !volume.Split(':')[0].Contains('/')).Select(volume => new Volume { Name = volume.Split(':')[0] }));
    Path Handling

    The relative path construction for bind mounts assumes the path will be relative to the output directory. This might not work correctly for all scenarios, especially with different directory structures.

    composeVolumes.AddRange(resourceWithBindMounts.BindMounts.Select(volume => $"./{Path.GetRelativePath(outputPath, volume.Source).Replace(Path.DirectorySeparatorChar, '/')}:{volume.Target}"));

    @qodo-code-review
    Copy link

    qodo-code-review bot commented Feb 15, 2025

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add null safety checks

    Add null check for volume.Split(':')[0] to prevent potential
    NullReferenceException if volume string is malformed

    src/Aspirate.Commands/Actions/Manifests/GenerateDockerComposeManifestAction.cs [79]

    -volumes.AddRange(service.Volumes.Where(volume => !volume.Split(':')[0].Contains('/')).Select(volume => new Volume { Name = volume.Split(':')[0] }));
    +volumes.AddRange(service.Volumes.Where(volume => !string.IsNullOrEmpty(volume) && volume.Contains(':') && !volume.Split(':')[0].Contains('/')).Select(volume => new Volume { Name = volume.Split(':')[0] }));

    [Suggestion has been applied]

    Suggestion importance[1-10]: 8

    __

    Why: The suggestion addresses a critical potential runtime error by adding null and format validation checks before accessing the volume string, preventing NullReferenceException in production.

    Medium
    Validate bind mount paths

    Add null check for volume.Source and volume.Target to prevent
    NullReferenceException when accessing bind mount paths

    src/Aspirate.Shared/Extensions/ResourceExtensions.cs [63]

    -composeVolumes.AddRange(resourceWithBindMounts.BindMounts.Select(volume => $"./{Path.GetRelativePath(outputPath, volume.Source).Replace(Path.DirectorySeparatorChar, '/')}:{volume.Target}"));
    +composeVolumes.AddRange(resourceWithBindMounts.BindMounts.Where(volume => !string.IsNullOrEmpty(volume.Source) && !string.IsNullOrEmpty(volume.Target)).Select(volume => $"./{Path.GetRelativePath(outputPath, volume.Source).Replace(Path.DirectorySeparatorChar, '/')}:{volume.Target}"));

    [Suggestion has been applied]

    Suggestion importance[1-10]: 8

    __

    Why: The suggestion prevents potential NullReferenceException by validating Source and Target properties before processing bind mount paths, which is crucial for system stability.

    Medium

    @prom3theu5
    Copy link
    Owner

    Thanks buddy
    Hope things are ok for you and your family

    I've merged in your other PR so you will need to rebase this one more time (Sorry)

    @dombrovsky
    Copy link
    Contributor Author

    Thanks buddy Hope things are ok for you and your family

    I've merged in your other PR so you will need to rebase this one more time (Sorry)

    Sure mate! Thanks for maintaining this awesome tool! My pleasure to contribute.

    if (service.Volumes is not null)
    {
    volumes.AddRange(service.Volumes.Select(volume => new Volume { Name = volume.Split(':')[0] }));
    volumes.AddRange(service.Volumes.Where(volume => !volume.Split(':')[0].Contains('/')).Select(volume => new Volume { Name = volume.Split(':')[0] }));

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Suggestion: Add null safety checks

    Suggested change
    volumes.AddRange(service.Volumes.Where(volume => !volume.Split(':')[0].Contains('/')).Select(volume => new Volume { Name = volume.Split(':')[0] }));
    volumes.AddRange(service.Volumes.Where(volume => !string.IsNullOrEmpty(volume) && volume.Contains(':') && !volume.Split(':')[0].Contains('/')).Select(volume => new Volume { Name = volume.Split(':')[0] }));

    composeVolumes.AddRange(resourceWithVolumes.Volumes.Where(x=>!string.IsNullOrWhiteSpace(x.Name)).Select(volume => $"{volume.Name}:{volume.Target}"));
    if (resource.Value is IResourceWithBindMounts resourceWithBindMounts)
    {
    composeVolumes.AddRange(resourceWithBindMounts.BindMounts.Select(volume => $"./{Path.GetRelativePath(outputPath, volume.Source).Replace(Path.DirectorySeparatorChar, '/')}:{volume.Target}"));

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Suggestion: Validate bind mount paths

    Suggested change
    composeVolumes.AddRange(resourceWithBindMounts.BindMounts.Select(volume => $"./{Path.GetRelativePath(outputPath, volume.Source).Replace(Path.DirectorySeparatorChar, '/')}:{volume.Target}"));
    composeVolumes.AddRange(resourceWithBindMounts.BindMounts.Where(volume => !string.IsNullOrEmpty(volume.Source) && !string.IsNullOrEmpty(volume.Target)).Select(volume => $"./{Path.GetRelativePath(outputPath, volume.Source).Replace(Path.DirectorySeparatorChar, '/')}:{volume.Target}"));

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    Aspirate apply doesn't take BindMount custom configurations

    2 participants