Conversation
Detect and remove incorrectly-created certificate directories (common on Mac when Docker mounts missing paths) and validate certificate files before use. Add Validate/Fix helpers in UpdateClustersCommand and ReverseProxyClient, update reverse proxy creation to restart a stopped proxy or create it with proper file mappings, and restart the proxy when configs change. Expose Restart on IDockerEngine and implement it in FallbackDockerEngine and LocalDockerClient. Also switch Docker container lookups to use name filters/NAMES instead of the vega-component label, fix async removal/start calls, adjust CoreDNS rewrite insertion to before the kubernetes block, and update tests. Minor change: update registry image reference to ghcr.io/project-zot/zot:v2.1.0.
There was a problem hiding this comment.
Pull request overview
This PR enhances certificate handling for Docker mounts and adds container restart functionality. On macOS, Docker can incorrectly create directories when mounting paths that don't exist; this change detects and removes such directories before attempting to use certificate files. The PR also adds a Restart method to the Docker engine interface and switches from nginx -s reload to container restart for applying configuration changes. Additionally, it migrates Docker container lookups from label-based filtering to name-based filtering and updates the CoreDNS configuration patching to insert rewrite rules before (instead of after) the kubernetes block.
Changes:
- Added certificate path validation and directory cleanup logic in
ReverseProxyClientandUpdateClustersCommand - Implemented
Restartmethod inIDockerEngine,LocalDockerClient, andFallbackDockerEngine - Updated reverse proxy creation to restart stopped containers instead of recreating them
- Changed Docker container lookup from
vega-componentlabel to name-based filtering - Modified CoreDNS rewrite insertion to occur before the kubernetes block for proper DNS evaluation order
- Updated registry image to multi-architecture variant (
ghcr.io/project-zot/zot:v2.1.0)
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| cli/src/Vdk/Services/ReverseProxyClient.cs | Added certificate validation/fix logic, updated Create() to handle stopped containers, changed ReloadConfigs() to use Restart, updated CoreDNS patching logic |
| cli/src/Vdk/Services/LocalDockerClient.cs | Switched from label-based to name-based container lookups, fixed async await on container removal, added Restart implementation |
| cli/src/Vdk/Services/IDockerEngine.cs | Added Restart method to interface |
| cli/src/Vdk/Services/FallbackDockerEngine.cs | Implemented Restart method using docker restart command |
| cli/src/Vdk/Commands/UpdateClustersCommand.cs | Added FixCertificatePathIfDirectory helper to detect and remove incorrectly-created cert directories |
| cli/src/Vdk/Constants/Containers.cs | Updated registry image to multi-architecture variant |
| cli/tests/Vdk.Tests/ReverseProxyClientTests.cs | Updated test to verify CoreDNS rewrite insertion before kubernetes block |
Comments suppressed due to low confidence (1)
cli/src/Vdk/Services/LocalDockerClient.cs:95
- Multiple async operations are being blocked synchronously using
GetAwaiter().GetResult(). This pattern can potentially cause deadlocks in certain contexts and is generally considered an anti-pattern.
While this may work in the current context, consider making the Exists, Delete, and Restart methods async (returning Task<bool>) to properly propagate async/await throughout the call chain. This would align better with the async nature of the Docker.DotNet library and avoid potential threading issues.
public bool Exists(string name, bool checkRunning = true)
{
IList<ContainerListResponse> containers = _dockerClient.Containers.ListContainersAsync(
new ContainersListParameters()
{
Filters = new Dictionary<string, IDictionary<string, bool>> { { "name", new Dictionary<string, bool> { { name, true } } } },
All = true,
}).GetAwaiter().GetResult();
var container = containers.FirstOrDefault(x => x.Names.Any(n => n.TrimStart('/') == name));
if (container == null) return false;
if (checkRunning == false) return true;
return container.State == "running" ||
// this should be started, lets do it
_dockerClient.Containers.StartContainerAsync(container.ID, new ContainerStartParameters() { }).GetAwaiter().GetResult();
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private bool ValidateAndFixCertificatePath(string path) | ||
| { | ||
| // Check if path exists as a directory (incorrect state) | ||
| if (Directory.Exists(path)) | ||
| { | ||
| _console.WriteWarning($"Certificate path '{path}' exists as a directory instead of a file. Removing..."); | ||
| try | ||
| { | ||
| _docker.Run(Containers.ProxyImage, Containers.ProxyName, | ||
| new[] { new PortMapping() { HostPort = ReverseProxyHostPort, ContainerPort = 443 } }, | ||
| null, | ||
| new[] | ||
| { | ||
| new FileMapping() { Destination = "/etc/nginx/conf.d/vega.conf", Source = conf.FullName }, | ||
| new FileMapping() { Destination = "/etc/certs/fullchain.pem", Source = fullChain.FullName }, | ||
| new FileMapping() { Destination = "/etc/certs/privkey.pem", Source = privKey.FullName }, | ||
| }, | ||
| null); | ||
| Directory.Delete(path, recursive: true); | ||
| } | ||
| catch (Exception e) | ||
| catch (Exception ex) | ||
| { | ||
| Console.WriteLine($"Error creating reverse proxy: {e}"); | ||
| _console.WriteError($"Failed to remove directory '{path}': {ex.Message}"); | ||
| return false; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| _console.WriteLine("Vega VDK Proxy already created"); | ||
| } | ||
|
|
||
| // Now check if the file exists | ||
| return File.Exists(path); | ||
| } |
There was a problem hiding this comment.
The certificate validation logic is duplicated between ValidateAndFixCertificatePath() in ReverseProxyClient.cs and FixCertificatePathIfDirectory() in UpdateClustersCommand.cs. Both methods perform the same task: checking if a path exists as a directory and removing it.
Consider extracting this logic into a shared utility method or service to avoid duplication and ensure consistency. This would make maintenance easier and reduce the risk of the two implementations diverging over time.
| public bool Restart(string name) | ||
| { | ||
| var container = _dockerClient.Containers.ListContainersAsync( | ||
| new ContainersListParameters() | ||
| { | ||
| Filters = new Dictionary<string, IDictionary<string, bool>> { { "name", new Dictionary<string, bool> { { name, true } } } }, | ||
| All = true, | ||
| }).GetAwaiter().GetResult().FirstOrDefault(x => x.Names.Any(n => n.TrimStart('/') == name)); | ||
|
|
||
| if (container == null) return false; | ||
| _dockerClient.Containers.RestartContainerAsync(container.ID, new ContainerRestartParameters()).GetAwaiter().GetResult(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The new Restart method lacks test coverage. Consider adding tests for this new functionality, especially since the codebase has tests for other IDockerEngine implementations (e.g., FallbackDockerEngineTests.cs tests Run, Exists, Delete, Stop, and Exec operations).
| private bool ValidateAndFixCertificatePath(string path) | ||
| { | ||
| // Check if path exists as a directory (incorrect state) | ||
| if (Directory.Exists(path)) | ||
| { | ||
| _console.WriteWarning($"Certificate path '{path}' exists as a directory instead of a file. Removing..."); | ||
| try | ||
| { | ||
| _docker.Run(Containers.ProxyImage, Containers.ProxyName, | ||
| new[] { new PortMapping() { HostPort = ReverseProxyHostPort, ContainerPort = 443 } }, | ||
| null, | ||
| new[] | ||
| { | ||
| new FileMapping() { Destination = "/etc/nginx/conf.d/vega.conf", Source = conf.FullName }, | ||
| new FileMapping() { Destination = "/etc/certs/fullchain.pem", Source = fullChain.FullName }, | ||
| new FileMapping() { Destination = "/etc/certs/privkey.pem", Source = privKey.FullName }, | ||
| }, | ||
| null); | ||
| Directory.Delete(path, recursive: true); | ||
| } | ||
| catch (Exception e) | ||
| catch (Exception ex) | ||
| { | ||
| Console.WriteLine($"Error creating reverse proxy: {e}"); | ||
| _console.WriteError($"Failed to remove directory '{path}': {ex.Message}"); | ||
| return false; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| _console.WriteLine("Vega VDK Proxy already created"); | ||
| } | ||
|
|
||
| // Now check if the file exists | ||
| return File.Exists(path); | ||
| } |
There was a problem hiding this comment.
The new ValidateAndFixCertificatePath method lacks test coverage. This is an important utility function that modifies the filesystem by detecting and removing incorrectly-created certificate directories.
The codebase has extensive tests for ReverseProxyClient in ReverseProxyClientTests.cs (testing methods like PatchCoreDns, InitConfFile, etc.), so adding tests for this new method would align with the existing testing patterns. Consider adding tests that verify:
- The method returns false when the path is a directory and can be removed
- The method returns true when the path is a valid file
- The method handles errors gracefully when directory removal fails
| if (!ValidateAndFixCertificatePath(fullChainPath) || | ||
| !ValidateAndFixCertificatePath(privKeyPath)) | ||
| { | ||
| _console.WriteError("Certificate files are missing. Cannot create TLS secret."); |
There was a problem hiding this comment.
The error message "Certificate files are missing. Cannot create TLS secret." could be more specific about which certificate file(s) are missing. Since the validation checks two separate files (fullchain.pem and privkey.pem), indicating which one failed would help users troubleshoot the issue more efficiently.
Consider updating the error message to specify which file(s) are missing or failed validation, for example: "Certificate file is missing or invalid: {path}"
| if (!ValidateAndFixCertificatePath(fullChainPath) || | |
| !ValidateAndFixCertificatePath(privKeyPath)) | |
| { | |
| _console.WriteError("Certificate files are missing. Cannot create TLS secret."); | |
| var invalidCertificatePaths = new List<string>(); | |
| if (!ValidateAndFixCertificatePath(fullChainPath)) | |
| { | |
| invalidCertificatePaths.Add(fullChainPath); | |
| } | |
| if (!ValidateAndFixCertificatePath(privKeyPath)) | |
| { | |
| invalidCertificatePaths.Add(privKeyPath); | |
| } | |
| if (invalidCertificatePaths.Count > 0) | |
| { | |
| _console.WriteError( | |
| $"Certificate file(s) are missing or invalid: {string.Join(", ", invalidCertificatePaths)}. Cannot create TLS secret."); |
| public bool Restart(string name) | ||
| { | ||
| return RunProcess("docker", $"restart {name}", out _, out _); | ||
| } |
There was a problem hiding this comment.
The new Restart method lacks test coverage. The existing test file FallbackDockerEngineTests.cs has tests for other Docker operations (Run, Exists, Delete, Stop, Exec), but doesn't include tests for the newly added Restart functionality.
Consider adding a test that verifies the Restart method works correctly, similar to the existing tests for other Docker operations.
| if (!ValidateAndFixCertificatePath(fullChain.FullName) || | ||
| !ValidateAndFixCertificatePath(privKey.FullName)) | ||
| { | ||
| _console.WriteError("Certificate files are missing. Please ensure Certs/fullchain.pem and Certs/privkey.pem exist."); |
There was a problem hiding this comment.
The error handling here returns early when certificate files are missing, which prevents the Docker container from being created. However, the error message suggests the user should "ensure" the files exist, but doesn't provide guidance on how to create or obtain these certificate files.
Consider enhancing the error message to include actionable instructions, such as pointing to documentation on how to generate or obtain the required certificate files, or providing a command to generate self-signed certificates for development purposes.
| _console.WriteError("Certificate files are missing. Please ensure Certs/fullchain.pem and Certs/privkey.pem exist."); | |
| _console.WriteError("Certificate files are missing. The reverse proxy cannot be started without TLS certificates."); | |
| _console.WriteError("Expected files: Certs/fullchain.pem and Certs/privkey.pem"); | |
| _console.WriteError("For local development, you can create self-signed certificates with:"); | |
| _console.WriteError(" mkdir -p Certs && openssl req -x509 -nodes -newkey rsa:2048 -keyout Certs/privkey.pem -out Certs/fullchain.pem -days 365"); | |
| _console.WriteError("For production, use certificates issued by a trusted CA and refer to the Vega VDK documentation on TLS certificates."); |
| return container.State == "running" || | ||
| // this should be started, lets do it | ||
| _dockerClient.Containers.StartContainerAsync(container.ID, new ContainerStartParameters() { }).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
The Exists method has inconsistent behavior between implementations of IDockerEngine:
FallbackDockerEngine.Exists()only checks existence without side effectsLocalDockerClient.Exists()attempts to auto-start stopped containers whencheckRunning=true(line 92-94)
This inconsistency can lead to different behavior depending on which Docker engine is in use. The auto-start behavior in LocalDockerClient appears to be the problematic one, as it prevents the restart logic in ReverseProxyClient.Create() from working correctly.
Consider removing the auto-start behavior from LocalDockerClient.Exists() to maintain consistency across implementations and make the method behavior match its name (it should only check existence, not start containers).
| return container.State == "running" || | |
| // this should be started, lets do it | |
| _dockerClient.Containers.StartContainerAsync(container.ID, new ContainerStartParameters() { }).GetAwaiter().GetResult(); | |
| // When checkRunning is true, only report whether the container is running; | |
| // do not start or otherwise modify container state. | |
| return container.State == "running"; |
1 participant
Detect and remove incorrectly-created certificate directories (common on Mac when Docker mounts missing paths) and validate certificate files before use. Add Validate/Fix helpers in UpdateClustersCommand and ReverseProxyClient, update reverse proxy creation to restart a stopped proxy or create it with proper file mappings, and restart the proxy when configs change. Expose Restart on IDockerEngine and implement it in FallbackDockerEngine and LocalDockerClient. Also switch Docker container lookups to use name filters/NAMES instead of the vega-component label, fix async removal/start calls, adjust CoreDNS rewrite insertion to before the kubernetes block, and update tests. Minor change: update registry image reference to ghcr.io/project-zot/zot:v2.1.0.