#15 Refactor Docker setup and Mermaid rendering process#16
#15 Refactor Docker setup and Mermaid rendering process#16christopherdukart merged 4 commits intomainfrom
Conversation
- Updated Dockerfile to remove Node.js and mermaid-cli installation, now using Docker CLI for rendering. - Enhanced README and documentation to reflect the new sibling-container architecture for Mermaid diagram rendering. - Modified MermaidRenderer to utilize the official Mermaid CLI Docker image instead of requiring local installation. - Added docker-compose.yml for easier setup and management of services, including volume mapping for Mermaid rendering. - Updated installation instructions to require Docker instead of Node.js and npm for Mermaid diagram rendering.
There was a problem hiding this comment.
Pull request overview
This pull request refactors the Mermaid diagram rendering architecture from a Node.js-based approach (using @mermaid-js/mermaid-cli locally installed) to a Docker-based sibling-container architecture. The main application now spawns the official Mermaid CLI Docker container on-demand for rendering, eliminating the need for Node.js and npm dependencies.
Changes:
- Refactored MermaidRenderer to use Docker CLI for spawning sibling containers instead of invoking local mmdc binary
- Removed Node.js, npm, and mermaid-cli installation from Dockerfile, replaced with Docker CLI installation
- Added docker-compose.yml to simplify setup with proper volume and socket mounting configuration
- Updated all documentation (English and German) to reflect the new Docker-based architecture and setup requirements
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ConfluenceSynkMD/Services/MermaidRenderer.cs | Core refactoring to invoke mermaid-cli via Docker containers with volume mounting for file sharing |
| Dockerfile | Removed Node.js/Puppeteer/mermaid-cli dependencies, added Docker CLI binary for sibling container spawning |
| docker-compose.yml | New file providing reference configuration for volume and socket mounting |
| docs/en/admin/installation.md | Updated requirements: removed Node.js/npm, added Docker as dependency |
| docs/en/admin/docker.md | Added sibling-container architecture explanation and Docker Compose examples with volume/socket mounts |
| docs/en/user/diagrams.md | Updated Mermaid prerequisites from Node.js to Docker Engine |
| docs/de/admin/installation.md | German translation of updated installation requirements |
| docs/de/admin/docker.md | German translation of Docker deployment guide with new architecture |
| docs/de/user/diagrams.md | German translation of updated diagram prerequisites |
| README.md | Updated Docker usage section with sibling-container architecture explanation and volume mount examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Dockerfile
Outdated
| # It's recommended to run as non-root, but accessing /var/run/docker.sock | ||
| # requires specific group permissions which depend on the host machine. | ||
| # By default, we leave it as root to ensure docker socket access works seamlessly out of the box. | ||
| # If running as non-root is strictly required, users must pass --group-add when running the container. |
There was a problem hiding this comment.
Running the container as root (by commenting out the non-root user setup) creates a security risk. While the comment acknowledges this is for "seamless" Docker socket access, mounting /var/run/docker.sock already gives the container full Docker daemon access which is equivalent to root access on the host. Consider documenting this security implication more prominently in the documentation and recommending that users configure proper group permissions instead of defaulting to root. The --group-add approach mentioned in the comment should be documented with specific examples.
| # It's recommended to run as non-root, but accessing /var/run/docker.sock | |
| # requires specific group permissions which depend on the host machine. | |
| # By default, we leave it as root to ensure docker socket access works seamlessly out of the box. | |
| # If running as non-root is strictly required, users must pass --group-add when running the container. | |
| # SECURITY NOTE: | |
| # This image runs as root by default. When combined with mounting /var/run/docker.sock | |
| # into the container, this effectively grants root-equivalent access to the Docker host, | |
| # because the Docker CLI inside the container can fully control the Docker daemon. | |
| # Only run this image with the Docker socket mounted if you fully trust the workloads. | |
| # | |
| # Recommended: run as a non-root user and grant Docker group access from the host. | |
| # For example, assuming your host has a "docker" group: | |
| # | |
| # docker run --rm \ | |
| # -v /var/run/docker.sock:/var/run/docker.sock \ | |
| # --user 1001:1001 \ | |
| # --group-add $(getent group docker | cut -d: -f3) \ | |
| # my-image:tag | |
| # | |
| # To enable the non-root user inside the image, uncomment the lines below and ensure | |
| # the UID (1001) matches the one you pass via --user: |
| context: . | ||
| dockerfile: Dockerfile | ||
| # To allow ConfluenceSynkMD to spawn sibling containers (like mermaid-cli) | ||
| # we mount the docker socket from the host system. |
There was a problem hiding this comment.
Mounting the Docker socket (/var/run/docker.sock) grants the container full control over the Docker daemon, which is effectively root access to the host system. This is a significant security risk that should be prominently documented. The documentation should warn users about this security implication and suggest alternatives like Docker-in-Docker with proper isolation, or running the tool directly on the host instead of in a container when security is a concern. At minimum, add a security warning section explaining the risks of socket mounting.
| # we mount the docker socket from the host system. | |
| # we mount the Docker socket from the host system. | |
| # | |
| # SECURITY WARNING: | |
| # Mounting /var/run/docker.sock into this container grants it full control over the | |
| # Docker daemon on the host. This is effectively root-equivalent access to the host | |
| # system and should only be used in trusted, single-tenant environments. | |
| # | |
| # If security is a concern, consider one of the following instead: | |
| # - Run ConfluenceSynkMD directly on the host (without this container) so it can | |
| # access Docker locally without exposing the host socket to a container. | |
| # - Use Docker-in-Docker or a sidecar Docker daemon with proper isolation, rather | |
| # than binding the host's /var/run/docker.sock into this container. | |
| # - Avoid running this setup on shared or untrusted hosts. |
Dockerfile
Outdated
| # Install Docker CLI to allow spawning sibling containers for Mermaid rendering | ||
| ARG DOCKER_CLI_VERSION=24.0.9 | ||
| RUN apt-get update && apt-get install -y --no-install-recommends curl ca-certificates \ | ||
| && curl -fsSL https://download.docker.com/linux/static/stable/$(uname -m)/docker-${DOCKER_CLI_VERSION}.tgz | tar -xzC /usr/local/bin --strip-components=1 docker/docker \ | ||
| && rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
The Docker CLI version is pinned to 24.0.9, but as of early 2025, Docker CLI has released newer versions (25.x series). While pinning ensures reproducibility, this version may be missing security fixes or features. Consider updating to a more recent version or at minimum adding a comment explaining why this specific version is chosen. Also, the download URL doesn't verify the checksum of the downloaded binary, which is a security risk - consider adding SHA256 verification.
| # Install Docker CLI to allow spawning sibling containers for Mermaid rendering | |
| ARG DOCKER_CLI_VERSION=24.0.9 | |
| RUN apt-get update && apt-get install -y --no-install-recommends curl ca-certificates \ | |
| && curl -fsSL https://download.docker.com/linux/static/stable/$(uname -m)/docker-${DOCKER_CLI_VERSION}.tgz | tar -xzC /usr/local/bin --strip-components=1 docker/docker \ | |
| && rm -rf /var/lib/apt/lists/* | |
| # Install Docker CLI to allow spawning sibling containers for Mermaid rendering. | |
| # The CLI version is pinned for reproducibility; update periodically after checking compatibility. | |
| ARG DOCKER_CLI_VERSION=25.0.5 | |
| RUN set -eux; \ | |
| apt-get update; \ | |
| apt-get install -y --no-install-recommends curl ca-certificates; \ | |
| arch="$(uname -m)"; \ | |
| curl -fsSL -o /tmp/docker.tgz "https://download.docker.com/linux/static/stable/${arch}/docker-${DOCKER_CLI_VERSION}.tgz"; \ | |
| curl -fsSL -o /tmp/docker.tgz.sha256 "https://download.docker.com/linux/static/stable/${arch}/docker-${DOCKER_CLI_VERSION}.tgz.sha256"; \ | |
| sha256sum -c /tmp/docker.tgz.sha256; \ | |
| tar -xzC /usr/local/bin --strip-components=1 -f /tmp/docker.tgz docker/docker; \ | |
| rm /tmp/docker.tgz /tmp/docker.tgz.sha256; \ | |
| rm -rf /var/lib/apt/lists/* |
| using var process = Process.Start(psi) | ||
| ?? throw new InvalidOperationException( | ||
| "Failed to start mmdc process. Ensure @mermaid-js/mermaid-cli is installed and available on PATH."); | ||
| "Failed to start docker process. Ensure docker is installed and available on PATH or the docker socket is mounted."); |
There was a problem hiding this comment.
If the Docker daemon is not running or not accessible (e.g., socket permissions issue, daemon down), the error handling might not provide a clear message to the user. The Process.Start could fail with various exceptions (file not found for docker executable is caught, but socket permission errors would manifest differently). Consider adding more specific error handling to detect when Docker is installed but the daemon is not accessible, and provide actionable guidance like checking if the Docker daemon is running or if the socket has correct permissions.
|
|
||
| // mmdc must be available on PATH. | ||
| if (IsCommandAvailable("mmdc")) | ||
| var dockerImage = Environment.GetEnvironmentVariable("MERMAID_DOCKER_IMAGE") ?? "ghcr.io/mermaid-js/mermaid-cli/mermaid-cli"; |
There was a problem hiding this comment.
The MERMAID_DOCKER_IMAGE environment variable allows customizing the Docker image used for rendering, but there's no validation of the image name format or existence. While validation might be expensive (requiring a docker pull or inspect), consider at least logging which image is being used at Debug level so administrators can verify the correct image is configured. This would help troubleshoot issues when a custom or misspelled image name is provided.
| var dockerImage = Environment.GetEnvironmentVariable("MERMAID_DOCKER_IMAGE") ?? "ghcr.io/mermaid-js/mermaid-cli/mermaid-cli"; | |
| var dockerImage = Environment.GetEnvironmentVariable("MERMAID_DOCKER_IMAGE") ?? "ghcr.io/mermaid-js/mermaid-cli/mermaid-cli"; | |
| _logger.Debug("Using Mermaid Docker image: {DockerImage}", dockerImage); |
| | `--code-line-numbers` | `false` | Show line numbers in Confluence code block macros (alias: `--line-numbers`) | | ||
| | `--debug-line-markers` | `false` | Include source line numbers in conversion error messages for debugging | | ||
| | `--title-prefix <prefix>` | — | Prefix prepended to all page titles (e.g. `[AUTO] `) | | ||
| | `--title-prefix <prefix>` | — | Prefix prepended to all page titles (e.g. `[AUTO]`) | |
There was a problem hiding this comment.
The change removes a trailing space in the title-prefix example (from [AUTO] to [AUTO]). If this is intentional to show that the prefix should not include trailing space, it's a good fix. However, if users were previously using [AUTO] with a space and this changes the documented convention, it could be a subtle breaking change in how their page titles appear. Consider clarifying in the PR description whether this whitespace change is intentional.
| | `--title-prefix <prefix>` | — | Prefix prepended to all page titles (e.g. `[AUTO]`) | | |
| | `--title-prefix <prefix>` | — | Prefix prepended to all page titles (e.g. `[AUTO] `) | |
|
|
||
| ```yaml | ||
| --- | ||
| space_key: TEAM | ||
| --- | ||
| ``` | ||
|
|
||
| **Inline HTML comment:** | ||
|
|
There was a problem hiding this comment.
Two blank lines are added before the YAML frontmatter and inline HTML comment examples. While this improves visual separation, ensure this change is consistent with the documentation style throughout the repository. If other sections don't use blank lines before code examples, this creates an inconsistency.
| throw new InvalidOperationException( | ||
| "MERMAID_DOCKER_VOLUME must be set when running inside a container. " + | ||
| "Set MERMAID_DOCKER_VOLUME to the host-visible shared temp directory that is mounted into this container, and set TMPDIR to the corresponding in-container mount path so both refer to the same physical directory. " + | ||
| "For example: MERMAID_DOCKER_VOLUME=/host/path/temp and TMPDIR=/app/mermaid_temp (where /host/path/temp is mounted into the container at /app/mermaid_temp).", |
There was a problem hiding this comment.
When the application is running inside a container and MERMAID_DOCKER_VOLUME is not set, an exception is thrown with detailed instructions. However, the error message could be even more helpful by including a link to the documentation or suggesting the docker-compose.yml file as a working example. Users encountering this error are likely confused about the volume mapping architecture, and a reference to documentation would help them resolve it faster.
| "For example: MERMAID_DOCKER_VOLUME=/host/path/temp and TMPDIR=/app/mermaid_temp (where /host/path/temp is mounted into the container at /app/mermaid_temp).", | |
| "For example: MERMAID_DOCKER_VOLUME=/host/path/temp and TMPDIR=/app/mermaid_temp (where /host/path/temp is mounted into the container at /app/mermaid_temp). " + | |
| "For more information about the required volume mapping and a working configuration example, see the project documentation (for example, the Mermaid/Docker section in the README) or the provided docker-compose.yml file.", |
docs/de/admin/docker.md
Outdated
|
|
||
| services: | ||
| confluencesynk: | ||
| image: confluentsynkmd |
There was a problem hiding this comment.
The docker-compose.yml example contains a typo in the image name. It shows "confluentsynkmd" but the actual docker-compose.yml file in the repository root uses service name "confluencesynk" and builds the image instead of using a pre-built image. The documentation should either use the correct service name from the root docker-compose.yml or clearly indicate this is just an example with a different naming convention.
| image: confluentsynkmd | |
| build: . |
|
|
||
| // Determine how to invoke mmdc from PATH. | ||
| var (command, args) = ResolveMmdc(inputFile, outputFile, puppeteerConfig); | ||
| // Determine how to invoke Mermaid CLI via Docker. |
There was a problem hiding this comment.
The comment on line 45 still references "mmdc" which is outdated terminology. Since this refactoring moves from Node.js mmdc to Docker-based rendering, the comment should say "Determine how to invoke mermaid-cli via Docker" to be consistent with the new architecture.
| // Determine how to invoke Mermaid CLI via Docker. | |
| // Determine how to invoke mermaid-cli via Docker. |
…altung; füge Hinweise zur Verwendung des Docker-Sockets hinzu und erweitere die Dokumentation für nicht-root Container.
… Hinweise zu isolierten Docker-Daemonen und TMPDIR/MERMAID_DOCKER_VOLUME hinzu.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Summary
Describe what this PR changes and why.
Related Issues
Type of change
Validation
dotnet build ConfluenceSynkMD.slnx --configuration Releasedotnet test ConfluenceSynkMD.slnx --configuration Release --no-buildBreaking changes
Checklist