Add UpdateClustersCommand for cluster certificate updates#46
Conversation
Introduces UpdateClustersCommand to update cluster certificates across VDK clusters, including logic to update TLS secrets and trigger gateway restarts. Integrates the new command into the update command group and registers it in the service provider. Also adds an alias 'k8s' to UpdateKindVersionInfoCommand.
There was a problem hiding this comment.
Pull request overview
Adds a new update clusters CLI subcommand to automate TLS certificate propagation across VDK-managed kind clusters and integrates it into the existing update command group and DI registration.
Changes:
- Introduces
UpdateClustersCommandto scan VDK clusters, update Vega-managed TLS secrets, and trigger gateway deployment rollouts after updates. - Wires the new command into the
updatecommand group and registers it inServiceProviderBuilder. - Adds a
k8salias forUpdateKindVersionInfoCommand.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| cli/src/Vdk/Commands/UpdateClustersCommand.cs | Implements cluster-wide TLS secret update logic and gateway restart behavior. |
| cli/src/Vdk/Commands/UpdateCommand.cs | Adds clusters as a subcommand under update. |
| cli/src/Vdk/ServiceProviderBuilder.cs | Registers UpdateClustersCommand in DI. |
| cli/src/Vdk/Commands/UpdateKindVersionInfoCommand.cs | Adds k8s alias for usability. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (verbose) | ||
| { | ||
| _console.WriteLine($"[DEBUG] Failed to list secrets in namespace '{nsName}': {ex.Message}"); |
There was a problem hiding this comment.
When listing secrets fails, this path returns without emitting any warning/error unless --verbose is set. That can lead to misleading output later (e.g., "All certificates are up to date") even though the namespace wasn’t checked. Consider logging at least a warning/error regardless of verbosity and/or tracking failures so the final summary reflects partial/failed scans.
| if (verbose) | |
| { | |
| _console.WriteLine($"[DEBUG] Failed to list secrets in namespace '{nsName}': {ex.Message}"); | |
| _console.WriteError($"Failed to list secrets in namespace '{nsName}' in cluster '{clusterName}': {ex.Message}"); | |
| if (verbose) | |
| { | |
| _console.WriteLine($"[DEBUG] Exception while listing secrets in namespace '{nsName}' in cluster '{clusterName}': {ex}"); |
| private async Task UpdateNamespaceCertificates( | ||
| IKubernetesClient client, | ||
| string clusterName, | ||
| string nsName, | ||
| byte[] localCert, |
There was a problem hiding this comment.
clusterName is passed into this method but never used. Either remove the parameter (and stop passing it) or use it in log/error messages so output is unambiguous when processing multiple clusters.
| private async Task RestartGatewayApiGateways( | ||
| IKubernetesClient client, | ||
| string nsName, | ||
| HashSet<string> secretNames, | ||
| bool verbose) | ||
| { | ||
| try | ||
| { | ||
| // Get deployments in the namespace that might be gateway controllers | ||
| var deployments = client.List<V1Deployment>(nsName); | ||
|
|
||
| foreach (var deployment in deployments) | ||
| { | ||
| var deploymentName = deployment.Metadata?.Name ?? ""; | ||
|
|
||
| // Look for gateway-related deployments | ||
| if (deploymentName.Contains("gateway", StringComparison.OrdinalIgnoreCase) || | ||
| deploymentName.Contains("envoy", StringComparison.OrdinalIgnoreCase) || | ||
| deploymentName.Contains("ingress", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| if (verbose) | ||
| { | ||
| _console.WriteLine($"[DEBUG] Found potential gateway deployment: {nsName}/{deploymentName}"); | ||
| } | ||
|
|
||
| // Trigger a rollout restart by updating an annotation | ||
| await RolloutRestartDeployment(client, deployment, verbose); | ||
| } |
There was a problem hiding this comment.
This code unconditionally restarts any deployment whose name contains "gateway"/"envoy"/"ingress" within namespaces where a secret was updated, but it does not use secretNames at all. This can cause unnecessary/incorrect restarts (and secretNames will be an unused-parameter warning). Consider filtering restarts to deployments that actually reference the updated secrets (e.g., volumes/Projected volumes referencing those secrets) or remove the parameter and adjust the logic/comment accordingly.
| // Try to find Gateway resources (gateway.networking.k8s.io) | ||
| await RestartGatewayApiGateways(client, nsName, secretsInNs, verbose); | ||
|
|
||
| // Also check for Ingress resources that might reference the secrets | ||
| await RestartIngressControllers(client, nsName, secretsInNs, verbose); |
There was a problem hiding this comment.
The comment says "Try to find Gateway resources (gateway.networking.k8s.io)", but the implementation is only scanning deployment names. Please update the comment (or implement actual Gateway API lookups) to avoid misleading future maintainers.
| await Task.CompletedTask; | ||
| } |
There was a problem hiding this comment.
await Task.CompletedTask; is redundant here and in other helper methods in this file. If the method has no real async work, prefer a non-async method returning Task.CompletedTask (or make it synchronous) to avoid unnecessary state machine/await noise.
| foreach (var tlsEntry in tls) | ||
| { | ||
| if (!string.IsNullOrEmpty(tlsEntry.SecretName) && secretNames.Contains(tlsEntry.SecretName)) | ||
| { | ||
| _console.WriteLine($" Ingress '{nsName}/{ingressName}' references updated secret '{tlsEntry.SecretName}'"); | ||
|
|
||
| if (verbose) | ||
| { | ||
| _console.WriteLine($"[DEBUG] Ingress controller should automatically pick up the new certificate"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var ns in namespaces) | ||
| { | ||
| var nsName = ns.Metadata?.Name; | ||
| if (string.IsNullOrEmpty(nsName)) continue; | ||
|
|
||
| if (verbose) | ||
| { | ||
| _console.WriteLine($"[DEBUG] Scanning namespace: {nsName}"); | ||
| } | ||
|
|
||
| await UpdateNamespaceCertificates(client, clusterName, nsName, localCert, localKey, verbose, updatedSecrets); | ||
| } |
There was a problem hiding this comment.
This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
This pull request introduces a new
UpdateClustersCommandto the CLI, enabling automated updating of TLS certificates across all VDK-managed Kubernetes clusters and restarting relevant gateway deployments if changes are detected. The command is integrated into the update command group and registered in the dependency injection system. Minor improvements are also made to existing update commands.New cluster certificate update functionality:
UpdateClustersCommandto automate checking and updating TLS certificates in all VDK clusters, updating only Vega-managed secrets, and triggering gateway restarts when secrets are updated. Includes verbose/debug output and robust error handling. (cli/src/Vdk/Commands/UpdateClustersCommand.cs)CLI integration and registration:
UpdateClustersCommandin the dependency injection container for use throughout the CLI. (cli/src/Vdk/ServiceProviderBuilder.cs)UpdateClustersCommandas a subcommand under the mainUpdateCommand, making it accessible via the CLI. (cli/src/Vdk/Commands/UpdateCommand.cs)Minor improvements:
k8sto theUpdateKindVersionInfoCommandfor improved usability. (cli/src/Vdk/Commands/UpdateKindVersionInfoCommand.cs)