From eb7a17ea057e109185b806fac1315ce8c274b619 Mon Sep 17 00:00:00 2001 From: Mohammad Abdolirad Date: Mon, 3 Aug 2026 12:34:50 +0200 Subject: [PATCH 1/3] harden helm cache and shared cmd helpers - inject per-client ChartCache and typed helm error wrapping - surface stale deploy watcher status and use WithoutCancel - rename common to cmdopts and util to validate - harden shell executor quoting and add unit tests --- flake.nix | 2 +- go.mod | 5 +- go.sum | 12 +- internal/action/deploy_test.go | 4 + internal/cmd/{common => cmdopts}/cluster.go | 57 ++++- internal/cmd/cmdopts/cluster_test.go | 103 ++++++++ internal/cmd/{common => cmdopts}/context.go | 2 +- internal/cmd/{common => cmdopts}/doc.go | 6 +- internal/cmd/{common => cmdopts}/options.go | 4 +- internal/cmd/delete/delete.go | 4 +- internal/cmd/deploy/deploy.go | 22 +- internal/cmd/deploy/watcher.go | 65 +++-- internal/cmd/deploy/watcher_test.go | 41 ++++ internal/cmd/initialize/components.go | 16 +- internal/cmd/list/list.go | 4 +- internal/cmd/logs/logs.go | 4 +- internal/cmd/plan/plan.go | 18 +- internal/cmd/root.go | 4 +- internal/cmd/shell/executor.go | 93 ++++++-- internal/cmd/shell/executor_test.go | 153 ++++++++++++ internal/cmd/shell/shell.go | 4 +- internal/cmd/status/status.go | 4 +- internal/helm/cache.go | 236 ++++++------------- internal/helm/cache_test.go | 40 +++- internal/helm/doc.go | 6 +- internal/helm/generate.go | 37 ++- internal/helm/helm.go | 63 ++++- internal/helm/helm_error_test.go | 130 ++++++++++ internal/helm/render.go | 2 +- internal/{util => validate}/doc.go | 4 +- internal/{util => validate}/validate.go | 2 +- internal/{util => validate}/validate_test.go | 2 +- 32 files changed, 857 insertions(+), 292 deletions(-) rename internal/cmd/{common => cmdopts}/cluster.go (61%) create mode 100644 internal/cmd/cmdopts/cluster_test.go rename internal/cmd/{common => cmdopts}/context.go (98%) rename internal/cmd/{common => cmdopts}/doc.go (81%) rename internal/cmd/{common => cmdopts}/options.go (84%) create mode 100644 internal/cmd/shell/executor_test.go create mode 100644 internal/helm/helm_error_test.go rename internal/{util => validate}/doc.go (89%) rename internal/{util => validate}/validate.go (99%) rename internal/{util => validate}/validate_test.go (99%) diff --git a/flake.nix b/flake.nix index 6349c3a..b11697e 100644 --- a/flake.nix +++ b/flake.nix @@ -25,7 +25,7 @@ buildGoModule' = pkgs.buildGoModule.override { inherit go; }; - deployahVendorHash = "sha256-ixUXF5jnjoL67Mmv8t55QIV3Xv9QyDaFzwr+p0YJ0mM="; + deployahVendorHash = "sha256-CoOzDrJDWuJCBgN1PQoS3k28Mrg78tv4FxvMvZGn+Ms="; deployah = import ./nix/deployah.nix { buildGoModule = buildGoModule'; diff --git a/go.mod b/go.mod index 77d3bc2..72b650a 100644 --- a/go.mod +++ b/go.mod @@ -12,7 +12,7 @@ require ( github.com/fluxcd/pkg/envsubst v1.7.0 github.com/go-viper/mapstructure/v2 v2.5.0 github.com/gonvenience/ytbx v1.5.0 - github.com/google/go-containerregistry v0.21.7 + github.com/google/go-containerregistry v0.21.8 github.com/google/renameio/v2 v2.0.2 github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 github.com/homeport/dyff v1.12.0 @@ -30,6 +30,7 @@ require ( k8s.io/apiextensions-apiserver v0.36.3 k8s.io/apimachinery v0.36.3 k8s.io/client-go v0.36.3 + mvdan.cc/sh/v3 v3.13.1 nabat.dev v0.6.3 sigs.k8s.io/e2e-framework v0.7.0 sigs.k8s.io/kind v0.32.0 @@ -89,7 +90,7 @@ require ( github.com/cyphar/filepath-securejoin v0.6.1 // indirect github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect github.com/dlclark/regexp2/v2 v2.2.2 // indirect - github.com/docker/cli v29.5.3+incompatible // indirect + github.com/docker/cli v29.6.2+incompatible // indirect github.com/docker/docker-credential-helpers v0.9.8 // indirect github.com/docker/go-connections v0.8.1 // indirect github.com/docker/go-units v0.5.0 // indirect diff --git a/go.sum b/go.sum index 3701306..96af928 100644 --- a/go.sum +++ b/go.sum @@ -156,8 +156,8 @@ github.com/dlclark/regexp2 v1.12.0 h1:0j4c5qQmnC6XOWNjP3PIXURXN2gWx76rd3KvgdPkCz github.com/dlclark/regexp2 v1.12.0/go.mod h1:DHkYz0B9wPfa6wondMfaivmHpzrQ3v9q8cnmRbL6yW8= github.com/dlclark/regexp2/v2 v2.2.2 h1:MYWvNYw8okuqNhwTYO587EZMiDruVa2vhV6fsGpfya0= github.com/dlclark/regexp2/v2 v2.2.2/go.mod h1:avUrQvPaLz2DrFNHJF0taWAFFX2C1GMSSoeiqFjcBmU= -github.com/docker/cli v29.5.3+incompatible h1:nbEFfz774vBwQ5KRYv7c/AghjReqnGISvrRhzjV0evs= -github.com/docker/cli v29.5.3+incompatible/go.mod h1:JLrzqnKDaYBop7H2jaqPtU4hHvMKP+vjCwu2uszcLI8= +github.com/docker/cli v29.6.2+incompatible h1:/bjePvcbbFTnRrMfWJBY7AjfICdsiLVgHn6LwTVOcqw= +github.com/docker/cli v29.6.2+incompatible/go.mod h1:JLrzqnKDaYBop7H2jaqPtU4hHvMKP+vjCwu2uszcLI8= github.com/docker/docker-credential-helpers v0.9.8 h1:bIREROb7So6PRlq6KTtdS9MPEjC29OQRkFNlvK2OX8Q= github.com/docker/docker-credential-helpers v0.9.8/go.mod h1:v1S+hepowrQXITkEfw6o4+BMbGot02wiKpzWhGUZK6c= github.com/docker/go-connections v0.8.1 h1:JibmG5hULs5qXSr/cp/w3Pw5fZuStt4MOHMUExb29/M= @@ -245,6 +245,8 @@ github.com/go-openapi/testify/enable/yaml/v2 v2.6.0 h1:gGHwAJ0R/5jU8BEGDbfRNR3hL github.com/go-openapi/testify/enable/yaml/v2 v2.6.0/go.mod h1:tY+St1SGq4NFl0QIqdTY4aEdbChAHxhyB77XQi9iJCo= github.com/go-openapi/testify/v2 v2.6.0 h1:5PKH2HE7YJ/LuRPQGvSxBRlFXNQhSetBLlGAgUEu3ug= github.com/go-openapi/testify/v2 v2.6.0/go.mod h1:SgsVHtfooshd0tublTtJ50FPKhujf47YRqauXXOUxfw= +github.com/go-quicktest/qt v1.101.0 h1:O1K29Txy5P2OK0dGo59b7b0LR6wKfIhttaAhHUyn7eI= +github.com/go-quicktest/qt v1.101.0/go.mod h1:14Bz/f7NwaXPtdYEgzsx46kqSxVwTbzVZsDC26tQJow= github.com/go-sql-driver/mysql v1.8.1 h1:LedoTUt/eveggdHS9qUFC1EFSa8bU2+1pZjSRpvNJ1Y= github.com/go-sql-driver/mysql v1.8.1/go.mod h1:wEBSXgmK//2ZFJyE+qWnIsVGmvmEKlqwuVSjsCm7DZg= github.com/go-task/slim-sprig/v3 v3.0.0 h1:sUs3vkvUymDpBKi3qH1YSqBQk9+9D/8M2mN1vB6EwHI= @@ -295,8 +297,8 @@ github.com/google/go-cmp v0.5.0/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/ github.com/google/go-cmp v0.5.3/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= -github.com/google/go-containerregistry v0.21.7 h1:/vPFuVXDjtFREsVArW+0h1CIl5urnOhzei4X2DMW9IU= -github.com/google/go-containerregistry v0.21.7/go.mod h1:kjSbt7/zMsKLWfnHrIvKvhXHUw91jbe9DNjPPJ32gXE= +github.com/google/go-containerregistry v0.21.8 h1:Ig/zIsnztdCUNaiNNczE+MoP5xcyUMfvpvfOr1xyMLE= +github.com/google/go-containerregistry v0.21.8/go.mod h1:dP5XNKcL7kMFF/TB3LfvWmVhAcv7iqkHb3oDK8aauTo= github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= github.com/google/pprof v0.0.0-20260202012954-cb029daf43ef h1:xpF9fUHpoIrrjX24DURVKiwHcFpw19ndIs+FwTSMbno= github.com/google/pprof v0.0.0-20260202012954-cb029daf43ef/go.mod h1:MxpfABSjhmINe3F1It9d+8exIHFvUqtLIRCdOGNXqiI= @@ -700,6 +702,8 @@ k8s.io/streaming v0.36.3 h1:9rAaqBk0C0Pc7+/fqGekj07NV+/Xrew58p647A0JT8w= k8s.io/streaming v0.36.3/go.mod h1:z6fV3D+NVkoeqRMtWwlUZK6U17SY/LqNzOxWL6GyR/s= k8s.io/utils v0.0.0-20260707023825-cf1189d6abe3 h1:jVkFFVfXdXP74B/zbO3hM3hpSFD0xvhQ5U686DPurkE= k8s.io/utils v0.0.0-20260707023825-cf1189d6abe3/go.mod h1:M2s5JB1lIYP3jzZdorPLHXIPJzt9vv2muW5a6L9DtNM= +mvdan.cc/sh/v3 v3.13.1 h1:DP3TfgZhDkT7lerUdnp6PTGKyxxzz6T+cOlY/xEvfWk= +mvdan.cc/sh/v3 v3.13.1/go.mod h1:lXJ8SexMvEVcHCoDvAGLZgFJ9Wsm2sulmoNEXGhYZD0= nabat.dev v0.6.3 h1:SFYOSHJTMzJzacHyVzvnEq0IA95+r0PpxQr/aRjM2UY= nabat.dev v0.6.3/go.mod h1:A1OPeIUHLkDG8BOnO3rfeBubcw0iPJ8GY7MuB/0lBB0= oras.land/oras-go/v2 v2.6.2 h1:N04RXngAp1LJKTG6ifz3xHPipasEkWr+hFmInja5YKo= diff --git a/internal/action/deploy_test.go b/internal/action/deploy_test.go index b4d1833..e9346e6 100644 --- a/internal/action/deploy_test.go +++ b/internal/action/deploy_test.go @@ -40,7 +40,9 @@ var testManifest = &spec.Spec{ // Run loads the manifest and delegates to the deployer. func TestDeploy_Run(t *testing.T) { + t.Parallel() t.Run("succeeds when deployer and loader succeed", func(t *testing.T) { + t.Parallel() d := action.NewDeploy(&mockDeployer{}, &mockSpecLoader{m: testManifest}) m, err := d.Run(t.Context(), "prod", false) require.NoError(t, err) @@ -48,6 +50,7 @@ func TestDeploy_Run(t *testing.T) { }) t.Run("returns error when manifest loader fails", func(t *testing.T) { + t.Parallel() d := action.NewDeploy(&mockDeployer{}, &mockSpecLoader{err: fmt.Errorf("not found")}) _, err := d.Run(t.Context(), "prod", false) require.Error(t, err) @@ -55,6 +58,7 @@ func TestDeploy_Run(t *testing.T) { }) t.Run("returns error when deployer fails", func(t *testing.T) { + t.Parallel() d := action.NewDeploy(&mockDeployer{err: fmt.Errorf("helm error")}, &mockSpecLoader{m: testManifest}) _, err := d.Run(t.Context(), "prod", false) require.Error(t, err) diff --git a/internal/cmd/common/cluster.go b/internal/cmd/cmdopts/cluster.go similarity index 61% rename from internal/cmd/common/cluster.go rename to internal/cmd/cmdopts/cluster.go index 157fc1b..ad3b7be 100644 --- a/internal/cmd/common/cluster.go +++ b/internal/cmd/cmdopts/cluster.go @@ -12,38 +12,69 @@ // See the License for the specific language governing permissions and // limitations under the License. -package common +package cmdopts import ( "context" + "errors" "fmt" - "strings" + "net" + "slices" + "syscall" "k8s.io/client-go/kubernetes" + "k8s.io/client-go/tools/clientcmd" "deployah.dev/deployah/internal/k8s" "deployah.dev/deployah/internal/spec" ) +// clusterHintSuffix is appended to connectivity errors so deploy and plan +// share the same recovery guidance. +const clusterHintSuffix = "\n\nHint: the target cluster/context may be unavailable. For a local cluster, run 'deployah cluster up' (and pass --context kind-deployah or set the environment's 'context' field)." + // ClusterHint returns an actionable suffix for errors that look like the // target cluster or context is missing or unreachable. It returns an empty // string for unrelated errors. Shared by `deployah deploy` and `deployah // plan` so their connectivity error messages never drift apart. func ClusterHint(err error) string { - if err == nil { + if !isClusterUnreachable(err) { return "" } - msg := strings.ToLower(err.Error()) - switch { - case strings.Contains(msg, "context") && (strings.Contains(msg, "does not exist") || strings.Contains(msg, "not found")), - strings.Contains(msg, "connection refused"), - strings.Contains(msg, "dial tcp"), - strings.Contains(msg, "no configuration has been provided"), - strings.Contains(msg, "couldn't get current server api group list"): - return "\n\nHint: the target cluster/context may be unavailable. For a local cluster, run 'deployah cluster up' (and pass --context kind-deployah or set the environment's 'context' field)." - default: - return "" + return clusterHintSuffix +} + +// isClusterUnreachable reports whether err (or a wrapped cause) indicates a +// missing kubeconfig, unknown context, or network failure reaching the API. +func isClusterUnreachable(err error) bool { + if err == nil { + return false } + + // clientcmd.IsEmptyConfig / IsContextNotFound do not walk wrappers, so + // unwrap one level at a time before those checks. + for e := err; e != nil; e = errors.Unwrap(e) { + if clientcmd.IsEmptyConfig(e) || clientcmd.IsContextNotFound(e) { + return true + } + } + if multi, ok := err.(interface{ Unwrap() []error }); ok { + if slices.ContainsFunc(multi.Unwrap(), isClusterUnreachable) { + return true + } + } + + if opErr, ok := errors.AsType[*net.OpError](err); ok && opErr != nil { + return true + } + if dnsErr, ok := errors.AsType[*net.DNSError](err); ok && dnsErr != nil { + return true + } + if errno, ok := errors.AsType[syscall.Errno](err); ok && + (errno == syscall.ECONNREFUSED || errno == syscall.ECONNRESET) { + return true + } + return false } // HasExposeComponents reports whether any component in the spec declares an diff --git a/internal/cmd/cmdopts/cluster_test.go b/internal/cmd/cmdopts/cluster_test.go new file mode 100644 index 0000000..0bbe249 --- /dev/null +++ b/internal/cmd/cmdopts/cluster_test.go @@ -0,0 +1,103 @@ +// Copyright 2025 The Deployah Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cmdopts + +import ( + "errors" + "fmt" + "net" + "net/url" + "syscall" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/client-go/tools/clientcmd" +) + +// TestClusterHint verifies typed connectivity errors get a recovery hint. +func TestClusterHint(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + err error + wantHint bool + }{ + {name: "nil", err: nil, wantHint: false}, + {name: "unrelated", err: errors.New("chart render failed"), wantHint: false}, + {name: "empty config", err: clientcmd.ErrEmptyConfig, wantHint: true}, + {name: "wrapped empty config", err: fmt.Errorf("rest config: %w", clientcmd.ErrEmptyConfig), wantHint: true}, + {name: "no context", err: clientcmd.ErrNoContext, wantHint: true}, + { + name: "wrapped no context", + err: fmt.Errorf("target: %w", clientcmd.ErrNoContext), + wantHint: true, + }, + { + name: "op error connection refused", + err: &net.OpError{ + Op: "dial", + Net: "tcp", + Err: syscall.ECONNREFUSED, + }, + wantHint: true, + }, + { + name: "wrapped op error", + err: fmt.Errorf("helm client: %w", &net.OpError{ + Op: "dial", + Net: "tcp", + Err: syscall.ECONNREFUSED, + }), + wantHint: true, + }, + { + name: "url error wrapping dial", + err: &url.Error{ + Op: "Get", + URL: "https://127.0.0.1:6443", + Err: &net.OpError{Op: "dial", Net: "tcp", Err: syscall.ECONNREFUSED}, + }, + wantHint: true, + }, + { + name: "dns error", + err: &net.DNSError{ + Err: "no such host", + Name: "kubernetes.default", + }, + wantHint: true, + }, + { + name: "joined with unreachable", + err: errors.Join(errors.New("other"), syscall.ECONNREFUSED), + wantHint: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := ClusterHint(tt.err) + if tt.wantHint { + require.NotEmpty(t, got) + assert.Contains(t, got, "deployah cluster up") + return + } + assert.Empty(t, got) + }) + } +} diff --git a/internal/cmd/common/context.go b/internal/cmd/cmdopts/context.go similarity index 98% rename from internal/cmd/common/context.go rename to internal/cmd/cmdopts/context.go index 2d30acf..6a04b28 100644 --- a/internal/cmd/common/context.go +++ b/internal/cmd/cmdopts/context.go @@ -1,4 +1,4 @@ -package common +package cmdopts import ( "fmt" diff --git a/internal/cmd/common/doc.go b/internal/cmd/cmdopts/doc.go similarity index 81% rename from internal/cmd/common/doc.go rename to internal/cmd/cmdopts/doc.go index b7cb7f3..afc5793 100644 --- a/internal/cmd/common/doc.go +++ b/internal/cmd/cmdopts/doc.go @@ -12,9 +12,9 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package common defines flags and options shared by every Deployah command. +// Package cmdopts defines flags and helpers shared by every Deployah command. // // [GlobalOptions] captures persistent CLI flags such as namespace, kubeconfig, // spec path, debug mode, and operation timeout. The root command reads -// these values in its pre-run hook and builds a [runtime.Runtime] from them. -package common +// these values in its pre-run hook and builds a [session.Session] from them. +package cmdopts diff --git a/internal/cmd/common/options.go b/internal/cmd/cmdopts/options.go similarity index 84% rename from internal/cmd/common/options.go rename to internal/cmd/cmdopts/options.go index 4272c04..f552c47 100644 --- a/internal/cmd/common/options.go +++ b/internal/cmd/cmdopts/options.go @@ -1,9 +1,9 @@ -package common +package cmdopts import "time" // GlobalOptions are the persistent flags shared by all commands. -// Used exclusively in the OnPreRun hook to construct the runtime. +// Used exclusively in the OnPreRun hook to construct the session. type GlobalOptions struct { Namespace string `nabat:"namespace"` Kubeconfig string `nabat:"kubeconfig"` diff --git a/internal/cmd/delete/delete.go b/internal/cmd/delete/delete.go index 163c1f9..1f73a2a 100644 --- a/internal/cmd/delete/delete.go +++ b/internal/cmd/delete/delete.go @@ -9,7 +9,7 @@ import ( "sigs.k8s.io/yaml" "deployah.dev/deployah/internal/cli" - "deployah.dev/deployah/internal/cmd/common" + "deployah.dev/deployah/internal/cmd/cmdopts" "deployah.dev/deployah/internal/helm" "deployah.dev/deployah/internal/session" "deployah.dev/deployah/internal/spec" @@ -125,7 +125,7 @@ func runDelete(c *nabat.Context) error { if err != nil { return fmt.Errorf("target cluster: %w", err) } - common.WarnContextFallback(c, cluster, opts.Environment) + cmdopts.WarnContextFallback(c, cluster, opts.Environment) helmClient, err := cluster.Helm() if err != nil { return fmt.Errorf("helm client: %w", err) diff --git a/internal/cmd/deploy/deploy.go b/internal/cmd/deploy/deploy.go index 6631438..2b26e98 100644 --- a/internal/cmd/deploy/deploy.go +++ b/internal/cmd/deploy/deploy.go @@ -14,7 +14,7 @@ import ( "nabat.dev/nabat" "nabat.dev/theme" - "deployah.dev/deployah/internal/cmd/common" + "deployah.dev/deployah/internal/cmd/cmdopts" "deployah.dev/deployah/internal/extras" "deployah.dev/deployah/internal/k8s" "deployah.dev/deployah/internal/readiness" @@ -119,7 +119,7 @@ func runDeploy(c *nabat.Context, resolvedTheme theme.ResolvedTheme) error { c.Logger().Debug("spec loaded", "env", opts.Environment) // Fail closed when any component uses expose and platform is absent. - if platform == nil && common.HasExposeComponents(manifest) { + if platform == nil && cmdopts.HasExposeComponents(manifest) { return fmt.Errorf( "one or more components use expose blocks but no platform file was found; "+ "create %s or set DEPLOYAH_PLATFORM_FILE, or pass --platform-file", @@ -158,17 +158,17 @@ func runDeploy(c *nabat.Context, resolvedTheme theme.ResolvedTheme) error { helmClient, err := cluster.Helm() if err != nil { - return fmt.Errorf("helm client: %w%s", err, common.ClusterHint(err)) + return fmt.Errorf("helm client: %w%s", err, cmdopts.ClusterHint(err)) } // Fail fast before the spinner so a bad context surfaces as a clean error // rather than a panic (helm/helm#32183 is triggered by a second // IsReachable call inside InstallApp on an already-poisoned client). if reachErr := helmClient.IsReachable(); reachErr != nil { - return fmt.Errorf("%w%s", reachErr, common.ClusterHint(reachErr)) + return fmt.Errorf("%w%s", reachErr, cmdopts.ClusterHint(reachErr)) } - common.WarnContextFallback(c, cluster, opts.Environment) + cmdopts.WarnContextFallback(c, cluster, opts.Environment) // Fetch the Kubernetes clientset once and thread it through, so a // transient failure produces one consistent outcome for this invocation. @@ -180,7 +180,7 @@ func runDeploy(c *nabat.Context, resolvedTheme theme.ResolvedTheme) error { // Materialize self-signed TLS certs once, before any render, so the plan // render and the real apply see identical bytes (see applyDeploy). if resolvedSpec != nil { - if tlsErr := common.MaterializeSelfSignedTLS(c, k8sClient, k8sErr, cluster.Namespace(), resolvedSpec); tlsErr != nil { + if tlsErr := cmdopts.MaterializeSelfSignedTLS(c, k8sClient, k8sErr, cluster.Namespace(), resolvedSpec); tlsErr != nil { return fmt.Errorf("materialize self-signed TLS: %w", tlsErr) } } @@ -286,7 +286,7 @@ func computePlan(c *nabat.Context, helmClient session.HelmClient, cluster *sessi diff, result, cleanup, err := planengine.BuildPlan(c, helmClient, manifest, environment, cluster.Context(), resolved, postRenderer) if err != nil { cleanup() - return nil, fmt.Errorf("%w%s", err, common.ClusterHint(err)) + return nil, fmt.Errorf("%w%s", err, cmdopts.ClusterHint(err)) } return &deployPlan{diff: diff, result: result, cleanup: cleanup}, nil } @@ -355,11 +355,11 @@ func applyBundleCRDs(c *nabat.Context, sess *session.Session, cluster *session.C } restCfg, restErr := cluster.RESTConfig() if restErr != nil { - return stats, fmt.Errorf("rest config for CRDs: %w%s", restErr, common.ClusterHint(restErr)) + return stats, fmt.Errorf("rest config for CRDs: %w%s", restErr, cmdopts.ClusterHint(restErr)) } stats, crdErr := extras.ApplyCRDs(c, restCfg, bundle.CRDs, extras.Policy(opts.CRDs), sess.Timeout()) if crdErr != nil { - return stats, fmt.Errorf("apply CRDs: %w%s", crdErr, common.ClusterHint(crdErr)) + return stats, fmt.Errorf("apply CRDs: %w%s", crdErr, cmdopts.ClusterHint(crdErr)) } return stats, nil } @@ -372,7 +372,7 @@ func applyDeploy(c *nabat.Context, sess *session.Session, cluster *session.Clust defer verifyCleanup() } if err != nil { - return fmt.Errorf("render manifests: %w%s", err, common.ClusterHint(err)) + return fmt.Errorf("render manifests: %w%s", err, cmdopts.ClusterHint(err)) } // A mismatch means the chart is non-deterministic (e.g. embeds a // timestamp), so what was shown isn't what would actually be installed. @@ -433,7 +433,7 @@ func applyDeploy(c *nabat.Context, sess *session.Session, cluster *session.Clust c.Warn(fmt.Sprintf("[%s] %s: %s", w.Object, w.Reason, w.Message)) } } - return fmt.Errorf("deploy failed: %w%s", err, common.ClusterHint(err)) + return fmt.Errorf("deploy failed: %w%s", err, cmdopts.ClusterHint(err)) } summary := buildSummaryMsg(watcher) diff --git a/internal/cmd/deploy/watcher.go b/internal/cmd/deploy/watcher.go index 7c225e9..17a9eb7 100644 --- a/internal/cmd/deploy/watcher.go +++ b/internal/cmd/deploy/watcher.go @@ -16,7 +16,9 @@ package deploy import ( "context" + "errors" "fmt" + "log/slog" "sync" "time" @@ -33,6 +35,7 @@ const ( pollInterval = 3 * time.Second finalRefreshPoll = 1 * time.Second finalRefreshTimeout = 10 * time.Second + finalRefreshBudget = 2 * time.Second ) // ComponentStatus summarizes pod readiness for one Deployah component at @@ -47,9 +50,10 @@ type DeployWatcher struct { namespace string releaseName string - mu sync.Mutex - warnings []k8s.DeployEvent - summary []ComponentStatus + mu sync.Mutex + warnings []k8s.DeployEvent + summary []ComponentStatus + pollStale bool // true when the last readiness poll failed } // NewDeployWatcher creates a watcher for the given release. @@ -86,13 +90,13 @@ func (w *DeployWatcher) Run(ctx context.Context, st *nabat.Status) { for { select { case <-ctx.Done(): - w.finalRefresh(st) //nolint:contextcheck // ctx is canceled; finalRefresh creates its own timeout + w.finalRefresh(ctx, st) return case ev, ok := <-eventCh: if !ok { <-ctx.Done() - w.finalRefresh(st) //nolint:contextcheck // ctx is canceled; finalRefresh creates its own timeout + w.finalRefresh(ctx, st) return } w.trackWarning(ev) @@ -157,31 +161,49 @@ func (w *DeployWatcher) pushRow(st *nabat.Status, ev k8s.DeployEvent) { } // updateTitle sets the status header to the current pod readiness summary. +// When the last poll failed, the title marks status as stale so the operator +// can tell updates have stopped without the watcher exiting. func (w *DeployWatcher) updateTitle(st headerUpdater) { w.mu.Lock() defer w.mu.Unlock() + if w.pollStale { + if len(w.summary) == 0 { + st.SetTitle("pod status unavailable (retrying...)") + return + } + ready, total := readyTotal(w.summary) + st.SetTitle(fmt.Sprintf("pods %d/%d ready (status stale)", ready, total)) + return + } if len(w.summary) == 0 { st.SetTitle("waiting for pods...") return } - total, ready := 0, 0 - for _, s := range w.summary { - total += s.TotalPods + ready, total := readyTotal(w.summary) + st.SetTitle(fmt.Sprintf("pods %d/%d ready", ready, total)) +} + +// readyTotal sums ReadyPods and TotalPods across statuses. +func readyTotal(statuses []ComponentStatus) (ready, total int) { + for _, s := range statuses { ready += s.ReadyPods + total += s.TotalPods } - st.SetTitle(fmt.Sprintf("pods %d/%d ready", ready, total)) + return ready, total } // finalRefresh polls pod readiness until all pods are ready or // finalRefreshTimeout elapses, marking RowWarning on timeout. Called after -// the parent ctx is already canceled, hence the independent timeout. -func (w *DeployWatcher) finalRefresh(st *nabat.Status) { +// the parent ctx is already canceled; [context.WithoutCancel] preserves +// request-scoped values while detaching from that cancellation. +func (w *DeployWatcher) finalRefresh(ctx context.Context, st *nabat.Status) { deadline := time.After(finalRefreshTimeout) ticker := time.NewTicker(finalRefreshPoll) defer ticker.Stop() + base := context.WithoutCancel(ctx) for { - freshCtx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + freshCtx, cancel := context.WithTimeout(base, finalRefreshBudget) w.refreshPodStatus(freshCtx) cancel() w.updateTitle(st) @@ -207,15 +229,30 @@ func (w *DeployWatcher) allReady() bool { } // refreshPodStatus polls pods for the release via [readiness.Poll] and -// updates the per-component summary. Errors are silently ignored so the -// watcher stays passive. +// updates the per-component summary. Poll failures leave the previous +// summary in place, mark status stale for the title, and log at debug so +// the watcher stays passive without going silent. func (w *DeployWatcher) refreshPodStatus(ctx context.Context) { statuses, err := readiness.Poll(ctx, w.k8sClient, w.namespace, w.releaseName) if err != nil { + // Shutdown cancel is expected; other failures (including deadline) + // mark the title stale so the operator sees updates have stopped. + if errors.Is(err, context.Canceled) { + return + } + slog.DebugContext(ctx, "deploy watcher: pod status poll failed", + "err", err, + "namespace", w.namespace, + "release", w.releaseName, + ) + w.mu.Lock() + w.pollStale = true + w.mu.Unlock() return } w.mu.Lock() defer w.mu.Unlock() w.summary = statuses + w.pollStale = false } diff --git a/internal/cmd/deploy/watcher_test.go b/internal/cmd/deploy/watcher_test.go index f471500..f3564c3 100644 --- a/internal/cmd/deploy/watcher_test.go +++ b/internal/cmd/deploy/watcher_test.go @@ -66,6 +66,7 @@ func runStatus(t *testing.T, fn func(*nabat.Status), title string) string { // TestDeployWatcher_Warnings_CollectsOnlyWarningEvents verifies that // only Warning-type events appear in the warnings list. func TestDeployWatcher_Warnings_CollectsOnlyWarningEvents(t *testing.T) { + t.Parallel() w := newWatcher() w.trackWarning(makeDeployEvent("uid-norm", corev1.EventTypeNormal, "Scheduled", "pod/abc", "ok", 1)) @@ -82,6 +83,7 @@ func TestDeployWatcher_Warnings_CollectsOnlyWarningEvents(t *testing.T) { // TestDeployWatcher_Warnings_DeduplicatesOnUpdate verifies that // repeated warnings with the same UID update rather than duplicate. func TestDeployWatcher_Warnings_DeduplicatesOnUpdate(t *testing.T) { + t.Parallel() w := newWatcher() w.trackWarning(makeDeployEvent("uid-w", corev1.EventTypeWarning, "BackOff", "pod/api", "crash", 1)) @@ -95,6 +97,7 @@ func TestDeployWatcher_Warnings_DeduplicatesOnUpdate(t *testing.T) { // TestDeployWatcher_Summary_ReturnsCopy verifies that Summary // returns an independent copy that does not alias internal state. func TestDeployWatcher_Summary_ReturnsCopy(t *testing.T) { + t.Parallel() w := newWatcher() w.mu.Lock() w.summary = []ComponentStatus{{Name: "api", ReadyPods: 1, TotalPods: 2}} @@ -112,6 +115,7 @@ func TestDeployWatcher_Summary_ReturnsCopy(t *testing.T) { // TestDeployWatcher_PushRow_NormalEvent verifies that a Normal event row // appears in the rendered Status output with the expected fields. func TestDeployWatcher_PushRow_NormalEvent(t *testing.T) { + t.Parallel() w := newWatcher() ev := makeDeployEvent("uid-1", corev1.EventTypeNormal, "Scheduled", "pod/api-abc", "successfully assigned", 1) @@ -127,6 +131,7 @@ func TestDeployWatcher_PushRow_NormalEvent(t *testing.T) { // TestDeployWatcher_PushRow_WarningEvent verifies that a Warning event // renders with the warning icon. func TestDeployWatcher_PushRow_WarningEvent(t *testing.T) { + t.Parallel() w := newWatcher() ev := makeDeployEvent("uid-w", corev1.EventTypeWarning, "Failed", "pod/api-abc", "ErrImagePull", 1) @@ -141,6 +146,7 @@ func TestDeployWatcher_PushRow_WarningEvent(t *testing.T) { // TestDeployWatcher_PushRow_CountAppendedToMessage verifies that a count // greater than one is appended to the message cell as "(xN)". func TestDeployWatcher_PushRow_CountAppendedToMessage(t *testing.T) { + t.Parallel() w := newWatcher() ev := makeDeployEvent("uid-c", corev1.EventTypeWarning, "Failed", "pod/api-abc", "ErrImagePull", 3) @@ -154,6 +160,7 @@ func TestDeployWatcher_PushRow_CountAppendedToMessage(t *testing.T) { // TestDeployWatcher_PushRow_CountNotShownWhenOne verifies that a count // of 1 is not appended to the message. func TestDeployWatcher_PushRow_CountNotShownWhenOne(t *testing.T) { + t.Parallel() w := newWatcher() ev := makeDeployEvent("uid-one", corev1.EventTypeNormal, "Started", "pod/api", "container started", 1) @@ -167,6 +174,7 @@ func TestDeployWatcher_PushRow_CountNotShownWhenOne(t *testing.T) { // TestDeployWatcher_PushRow_UIDNotDisplayed verifies that the event UID used // as the row key does not appear in the rendered output. func TestDeployWatcher_PushRow_UIDNotDisplayed(t *testing.T) { + t.Parallel() w := newWatcher() uid := types.UID("uid-do-not-show-this-1234") ev := makeDeployEvent(uid, corev1.EventTypeNormal, "Scheduled", "pod/api", "assigned", 1) @@ -188,6 +196,7 @@ func (s *spyTitleSetter) SetTitle(title string) { s.title = title } // TestDeployWatcher_UpdateTitle_NoPods verifies that the title reads // "waiting for pods..." when no pods have been observed yet. func TestDeployWatcher_UpdateTitle_NoPods(t *testing.T) { + t.Parallel() w := newWatcher() spy := &spyTitleSetter{} @@ -199,6 +208,7 @@ func TestDeployWatcher_UpdateTitle_NoPods(t *testing.T) { // TestDeployWatcher_UpdateTitle_WithPods verifies that the title shows // a ready/total count when pod summary data is available. func TestDeployWatcher_UpdateTitle_WithPods(t *testing.T) { + t.Parallel() w := newWatcher() w.mu.Lock() w.summary = []ComponentStatus{{Name: "api", ReadyPods: 2, TotalPods: 3}} @@ -209,3 +219,34 @@ func TestDeployWatcher_UpdateTitle_WithPods(t *testing.T) { assert.Equal(t, "pods 2/3 ready", spy.title) } + +// TestDeployWatcher_UpdateTitle_StaleWithPods verifies that a failed poll +// surfaces a product-facing stale indicator while keeping the last counts. +func TestDeployWatcher_UpdateTitle_StaleWithPods(t *testing.T) { + t.Parallel() + w := newWatcher() + w.mu.Lock() + w.summary = []ComponentStatus{{Name: "api", ReadyPods: 2, TotalPods: 3}} + w.pollStale = true + w.mu.Unlock() + + spy := &spyTitleSetter{} + w.updateTitle(spy) + + assert.Equal(t, "pods 2/3 ready (status stale)", spy.title) +} + +// TestDeployWatcher_UpdateTitle_StaleWithoutPods verifies the unavailable +// title when a poll fails before any summary has been observed. +func TestDeployWatcher_UpdateTitle_StaleWithoutPods(t *testing.T) { + t.Parallel() + w := newWatcher() + w.mu.Lock() + w.pollStale = true + w.mu.Unlock() + + spy := &spyTitleSetter{} + w.updateTitle(spy) + + assert.Equal(t, "pod status unavailable (retrying...)", spy.title) +} diff --git a/internal/cmd/initialize/components.go b/internal/cmd/initialize/components.go index 1097a77..248d09b 100644 --- a/internal/cmd/initialize/components.go +++ b/internal/cmd/initialize/components.go @@ -10,7 +10,7 @@ import ( "nabat.dev/nabat" "deployah.dev/deployah/internal/spec" - "deployah.dev/deployah/internal/util" + "deployah.dev/deployah/internal/validate" ) func validateComponentNameUnique(name string, existing map[string]spec.Component) error { @@ -363,7 +363,7 @@ func collectComponentImage(c *nabat.Context, component *spec.Component, componen fmt.Sprintf("Image for %s", componentName), nabat.WithHint("nginx:latest"), nabat.WithValidate(func(s string) error { - return util.ValidateNonEmpty(s, "image") + return validate.ValidateNonEmpty(s, "image") }), ) if err != nil { @@ -560,20 +560,20 @@ func collectComponentAutoscaling(c *nabat.Context, component *spec.Component, co "Minimum number of replicas to maintain", nabat.WithHint(strconv.Itoa(DefaultMinReplicas)), nabat.WithDefault(strconv.Itoa(DefaultMinReplicas)), - nabat.WithValidate(func(s string) error { return util.ValidatePositiveInteger(s, "minimum replicas") }), + nabat.WithValidate(func(s string) error { return validate.ValidatePositiveInteger(s, "minimum replicas") }), ), nabat.WithFormField(&maxReplicasStr, "Maximum Replicas", "Maximum number of replicas allowed", nabat.WithHint(strconv.Itoa(DefaultMaxReplicas)), nabat.WithDefault(strconv.Itoa(DefaultMaxReplicas)), - nabat.WithValidate(func(s string) error { return util.ValidatePositiveInteger(s, "maximum replicas") }), + nabat.WithValidate(func(s string) error { return validate.ValidatePositiveInteger(s, "maximum replicas") }), ), ) if err != nil { return fmt.Errorf("failed to get autoscaling config: %w", err) } - if err = util.ValidateMinMaxReplicas(minReplicasStr, maxReplicasStr); err != nil { + if err = validate.ValidateMinMaxReplicas(minReplicasStr, maxReplicasStr); err != nil { return fmt.Errorf("autoscaling configuration error: %w", err) } @@ -612,17 +612,17 @@ func collectComponentCustomResources(c *nabat.Context, component *spec.Component nabat.WithFormField(&cpu, "CPU", "CPU resource (e.g., 500m, 1)", nabat.WithHint("500m"), - nabat.WithValidate(func(s string) error { return util.ValidateResourceString(s, "CPU") }), + nabat.WithValidate(func(s string) error { return validate.ValidateResourceString(s, "CPU") }), ), nabat.WithFormField(&memory, "Memory", "Memory resource (e.g., 512Mi, 1Gi)", nabat.WithHint("512Mi"), - nabat.WithValidate(func(s string) error { return util.ValidateResourceString(s, "Memory") }), + nabat.WithValidate(func(s string) error { return validate.ValidateResourceString(s, "Memory") }), ), nabat.WithFormField(&ephemeralStorage, "Ephemeral Storage", "Ephemeral storage (e.g., 1Gi, 2Gi)", nabat.WithHint("1Gi"), - nabat.WithValidate(func(s string) error { return util.ValidateResourceString(s, "EphemeralStorage") }), + nabat.WithValidate(func(s string) error { return validate.ValidateResourceString(s, "EphemeralStorage") }), ), ) if err != nil { diff --git a/internal/cmd/list/list.go b/internal/cmd/list/list.go index feca70b..3722edf 100644 --- a/internal/cmd/list/list.go +++ b/internal/cmd/list/list.go @@ -6,7 +6,7 @@ import ( "nabat.dev/nabat" "deployah.dev/deployah/internal/cli" - "deployah.dev/deployah/internal/cmd/common" + "deployah.dev/deployah/internal/cmd/cmdopts" "deployah.dev/deployah/internal/k8s" "deployah.dev/deployah/internal/session" ) @@ -54,7 +54,7 @@ func runList(c *nabat.Context) error { if err != nil { return fmt.Errorf("target cluster: %w", err) } - common.WarnContextFallback(c, cluster, opts.Environment) + cmdopts.WarnContextFallback(c, cluster, opts.Environment) helmClient, err := cluster.Helm() if err != nil { return fmt.Errorf("helm client: %w", err) diff --git a/internal/cmd/logs/logs.go b/internal/cmd/logs/logs.go index c6939a9..6e6be55 100644 --- a/internal/cmd/logs/logs.go +++ b/internal/cmd/logs/logs.go @@ -15,7 +15,7 @@ import ( "k8s.io/apimachinery/pkg/labels" "nabat.dev/nabat" - "deployah.dev/deployah/internal/cmd/common" + "deployah.dev/deployah/internal/cmd/cmdopts" "deployah.dev/deployah/internal/k8s" "deployah.dev/deployah/internal/session" ) @@ -100,7 +100,7 @@ func runLogs(c *nabat.Context) error { if err != nil { return fmt.Errorf("target cluster: %w", err) } - common.WarnContextFallback(c, cluster, opts.Environment) + cmdopts.WarnContextFallback(c, cluster, opts.Environment) clientset, err := cluster.Kubernetes() if err != nil { diff --git a/internal/cmd/plan/plan.go b/internal/cmd/plan/plan.go index 0b385db..d4f52b7 100644 --- a/internal/cmd/plan/plan.go +++ b/internal/cmd/plan/plan.go @@ -21,7 +21,7 @@ import ( "nabat.dev/nabat" "nabat.dev/theme" - "deployah.dev/deployah/internal/cmd/common" + "deployah.dev/deployah/internal/cmd/cmdopts" "deployah.dev/deployah/internal/drift" "deployah.dev/deployah/internal/extras" "deployah.dev/deployah/internal/k8s" @@ -139,7 +139,7 @@ func runPlan(c *nabat.Context, resolvedTheme theme.ResolvedTheme) error { return fmt.Errorf("load spec: %w", err) } - if platform == nil && common.HasExposeComponents(manifest) { + if platform == nil && cmdopts.HasExposeComponents(manifest) { return fmt.Errorf( "one or more components use expose blocks but no platform file was found; "+ "create %s or set DEPLOYAH_PLATFORM_FILE, or pass --platform-file", @@ -181,7 +181,7 @@ func runOffline(c *nabat.Context, sess *session.Session, platform *spec.Platform // Offline mode never contacts the cluster, so any self-signed TLS cert // is generated fresh (nil client) rather than fetched/reused -- a - // deliberate offline generation, not common.MaterializeSelfSignedTLS's + // deliberate offline generation, not cmdopts.MaterializeSelfSignedTLS's // fail-closed path for an online command that couldn't build a client. if resolvedSpec != nil { if tlsErr := k8s.MaterializeSelfSignedTLS(c, nil, "", resolvedSpec); tlsErr != nil { @@ -226,14 +226,14 @@ func runOnline(c *nabat.Context, sess *session.Session, platform *spec.PlatformC helmClient, err := cluster.Helm() if err != nil { - return fmt.Errorf("helm client: %w%s", err, common.ClusterHint(err)) + return fmt.Errorf("helm client: %w%s", err, cmdopts.ClusterHint(err)) } if reachErr := helmClient.IsReachable(); reachErr != nil { - return fmt.Errorf("%w%s", reachErr, common.ClusterHint(reachErr)) + return fmt.Errorf("%w%s", reachErr, cmdopts.ClusterHint(reachErr)) } - common.WarnContextFallback(c, cluster, opts.Environment) + cmdopts.WarnContextFallback(c, cluster, opts.Environment) // Materialize self-signed TLS certs once, before rendering, matching // deploy's determinism guarantee (a fresh keypair per render would make @@ -243,7 +243,7 @@ func runOnline(c *nabat.Context, sess *session.Session, platform *spec.PlatformC c.Logger().Debug("kubernetes client unavailable", "err", k8sErr) } if resolvedSpec != nil { - if tlsErr := common.MaterializeSelfSignedTLS(c, k8sClient, k8sErr, cluster.Namespace(), resolvedSpec); tlsErr != nil { + if tlsErr := cmdopts.MaterializeSelfSignedTLS(c, k8sClient, k8sErr, cluster.Namespace(), resolvedSpec); tlsErr != nil { return fmt.Errorf("materialize self-signed TLS: %w", tlsErr) } } @@ -261,7 +261,7 @@ func runOnline(c *nabat.Context, sess *session.Session, platform *spec.PlatformC p, result, cleanup, err := planengine.BuildPlan(c, helmClient, manifest, opts.Environment, cluster.Context(), resolvedSpec, postRenderer) defer cleanup() if err != nil { - return fmt.Errorf("%w%s", err, common.ClusterHint(err)) + return fmt.Errorf("%w%s", err, cmdopts.ClusterHint(err)) } if n := len(bundle.CRDs); n > 0 { @@ -270,7 +270,7 @@ func runOnline(c *nabat.Context, sess *session.Session, platform *spec.PlatformC if opts.Drift { if driftErr := checkDrift(c, cluster, p, result.Manifest); driftErr != nil { - return fmt.Errorf("check drift: %w%s", driftErr, common.ClusterHint(driftErr)) + return fmt.Errorf("check drift: %w%s", driftErr, cmdopts.ClusterHint(driftErr)) } } diff --git a/internal/cmd/root.go b/internal/cmd/root.go index cb495f4..85a65fd 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -28,7 +28,7 @@ import ( "nabat.dev/theme" "deployah.dev/deployah/internal/cmd/cluster" - "deployah.dev/deployah/internal/cmd/common" + "deployah.dev/deployah/internal/cmd/cmdopts" "deployah.dev/deployah/internal/cmd/delete" "deployah.dev/deployah/internal/cmd/deploy" "deployah.dev/deployah/internal/cmd/initialize" @@ -84,7 +84,7 @@ func NewApp(opts ...nabat.Option) *nabat.App { // Build runtime once from global flags and store in context for all commands. if err := app.OnPreRun(func(c *nabat.Context) error { - var opts common.GlobalOptions + var opts cmdopts.GlobalOptions if err := c.Bind(&opts); err != nil { return fmt.Errorf("binding global options: %w", err) } diff --git a/internal/cmd/shell/executor.go b/internal/cmd/shell/executor.go index 38caf00..929318b 100644 --- a/internal/cmd/shell/executor.go +++ b/internal/cmd/shell/executor.go @@ -7,12 +7,13 @@ import ( "os" "os/signal" "slices" - "strings" + "sync" "syscall" "golang.org/x/term" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/tools/remotecommand" + "mvdan.cc/sh/v3/syntax" "nabat.dev/nabat" "deployah.dev/deployah/internal/k8s" @@ -138,8 +139,10 @@ func (e *ShellExecutor) Execute(opts ExecuteOptions) error { selectedShell = availableShells[0] } - execCommand := e.buildExecCommand(selectedShell, opts.Command, opts.WorkDir) - + execCommand, err := buildExecCommand(selectedShell, opts.Command, opts.WorkDir) + if err != nil { + return err + } return e.execInContainer(pod.Name, containerName, execCommand) } @@ -228,19 +231,55 @@ func (e *ShellExecutor) detectAvailableShells(podName, containerName string) ([] return availableShells, nil } -// buildExecCommand builds the command to execute -func (e *ShellExecutor) buildExecCommand(shell, command, workdir string) []string { - if command != "" { - return []string{shell, "-c", command} +// shellQuote returns a POSIX-safe shell word for s using [syntax.Quote], so +// user-supplied paths cannot expand or run as code inside a `sh -c` script. +func shellQuote(s string) (string, error) { + quoted, err := syntax.Quote(s, syntax.LangPOSIX) + if err != nil { + return "", fmt.Errorf("quote %q: %w", s, err) } + return quoted, nil +} - cmd := []string{shell} - +// buildExecCommand builds the argv for pod exec. WorkDir and the shell name +// are quoted when spliced into a shell script. Command is intentional shell +// input from --command and is not re-quoted. +func buildExecCommand(shell, command, workdir string) ([]string, error) { + if command != "" { + if workdir != "" { + quotedDir, err := shellQuote(workdir) + if err != nil { + return nil, fmt.Errorf("workdir: %w", err) + } + return []string{shell, "-c", "cd " + quotedDir + " && " + command}, nil + } + return []string{shell, "-c", command}, nil + } if workdir != "" { - cmd = append(cmd, "-c", fmt.Sprintf("cd %s && exec %s", workdir, shell)) + quotedDir, err := shellQuote(workdir) + if err != nil { + return nil, fmt.Errorf("workdir: %w", err) + } + quotedShell, err := shellQuote(shell) + if err != nil { + return nil, fmt.Errorf("shell: %w", err) + } + return []string{shell, "-c", "cd " + quotedDir + " && exec " + quotedShell}, nil } + return []string{shell}, nil +} - return cmd +// isBrokenPipe reports whether err is a broken-pipe condition from the +// remotecommand stream, matching [syscall.EPIPE] through the wrap chain. +func isBrokenPipe(err error) bool { + if err == nil { + return false + } + if errors.Is(err, syscall.EPIPE) { + return true + } + var errno syscall.Errno + return errors.As(err, &errno) && errno == syscall.EPIPE } // execTest executes a test command to check if something exists @@ -312,7 +351,6 @@ func (e *ShellExecutor) execInContainer(podName, containerName string, cmd []str } var sizeQueue remotecommand.TerminalSizeQueue - var resizeCh chan os.Signal var q *terminalSizeQueue if isTTY { q = &terminalSizeQueue{ch: make(chan remotecommand.TerminalSize, 1)} @@ -322,18 +360,30 @@ func (e *ShellExecutor) execInContainer(podName, containerName string, cmd []str q.ch <- terminalSizeFromWH(w, h) } - resizeCh = make(chan os.Signal, 1) + resizeCh := make(chan os.Signal, 1) + done := make(chan struct{}) signal.Notify(resizeCh, syscall.SIGWINCH) - go func() { - for range resizeCh { - if w, h, sizeErr := term.GetSize(fd); sizeErr == nil { - q.ch <- terminalSizeFromWH(w, h) + var wg sync.WaitGroup + wg.Go(func() { + for { + select { + case <-done: + return + case <-resizeCh: + if w, h, sizeErr := term.GetSize(fd); sizeErr == nil { + select { + case q.ch <- terminalSizeFromWH(w, h): + case <-done: + return + } + } } } - }() + }) defer func() { signal.Stop(resizeCh) - close(resizeCh) + close(done) + wg.Wait() close(q.ch) }() } @@ -373,10 +423,7 @@ func (e *ShellExecutor) execInContainer(podName, containerName string, cmd []str return nil } - // The SPDY executor stringifies syscall.EPIPE before returning it, so - // errors.Is(err, syscall.EPIPE) is tried first and the string fallback - // handles cases where the errno is not preserved through the stream layer. - if errors.Is(err, syscall.EPIPE) || strings.Contains(err.Error(), "broken pipe") { + if isBrokenPipe(err) { return nil } diff --git a/internal/cmd/shell/executor_test.go b/internal/cmd/shell/executor_test.go new file mode 100644 index 0000000..a0e113a --- /dev/null +++ b/internal/cmd/shell/executor_test.go @@ -0,0 +1,153 @@ +// Copyright 2025 The Deployah Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package shell + +import ( + "errors" + "fmt" + "syscall" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestShellQuote verifies POSIX-safe quoting via shellQuote. +func TestShellQuote(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input string + want string + wantErr bool + }{ + {name: "safe path unquoted", input: "/app/src", want: `/app/src`}, + {name: "spaces", input: "/app/my dir", want: `'/app/my dir'`}, + {name: "metacharacters", input: `/tmp/foo; id`, want: `'/tmp/foo; id'`}, + {name: "command substitution", input: `/tmp/$(id)`, want: `'/tmp/$(id)'`}, + {name: "single quote uses doubles", input: `it's`, want: `"it's"`}, + {name: "empty", input: "", want: `''`}, + {name: "null byte", input: "a\x00b", wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, err := shellQuote(tt.input) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +// TestBuildExecCommand verifies workdir quoting and command composition. +func TestBuildExecCommand(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + shell string + command string + workdir string + want []string + wantErr bool + }{ + { + name: "interactive shell", + shell: "bash", + want: []string{"bash"}, + }, + { + name: "workdir only", + shell: "bash", + workdir: "/app/src", + want: []string{"bash", "-c", "cd /app/src && exec bash"}, + }, + { + name: "workdir with metacharacters", + shell: "sh", + workdir: `/tmp/foo; id`, + want: []string{"sh", "-c", "cd '/tmp/foo; id' && exec sh"}, + }, + { + name: "command only", + shell: "bash", + command: "ls -la", + want: []string{"bash", "-c", "ls -la"}, + }, + { + name: "command with workdir", + shell: "bash", + command: "ls -la", + workdir: "/app/src", + want: []string{"bash", "-c", "cd /app/src && ls -la"}, + }, + { + name: "workdir with single quote", + shell: "bash", + workdir: `/tmp/it's`, + want: []string{"bash", "-c", `cd "/tmp/it's" && exec bash`}, + }, + { + name: "workdir null byte", + shell: "bash", + workdir: "a\x00b", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, err := buildExecCommand(tt.shell, tt.command, tt.workdir) + if tt.wantErr { + require.Error(t, err) + assert.Nil(t, got) + return + } + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +// TestIsBrokenPipe verifies typed broken-pipe detection through wrapping. +func TestIsBrokenPipe(t *testing.T) { + t.Parallel() + + assert.False(t, isBrokenPipe(nil)) + assert.False(t, isBrokenPipe(errors.New("other error"))) + assert.False(t, isBrokenPipe(errors.New("write: broken pipe"))) + assert.True(t, isBrokenPipe(syscall.EPIPE)) + assert.True(t, isBrokenPipe(fmt.Errorf("stream: %w", syscall.EPIPE))) +} + +// TestTerminalSizeFromWH clamps negative and oversized terminal dimensions. +func TestTerminalSizeFromWH(t *testing.T) { + t.Parallel() + + sz := terminalSizeFromWH(80, 24) + require.Equal(t, uint16(80), sz.Width) + require.Equal(t, uint16(24), sz.Height) + + sz = terminalSizeFromWH(-1, 1<<20) + assert.Equal(t, uint16(0), sz.Width) + assert.Equal(t, uint16(^uint16(0)), sz.Height) +} diff --git a/internal/cmd/shell/shell.go b/internal/cmd/shell/shell.go index db5913c..2ca9093 100644 --- a/internal/cmd/shell/shell.go +++ b/internal/cmd/shell/shell.go @@ -5,7 +5,7 @@ import ( "nabat.dev/nabat" - "deployah.dev/deployah/internal/cmd/common" + "deployah.dev/deployah/internal/cmd/cmdopts" "deployah.dev/deployah/internal/session" ) @@ -69,7 +69,7 @@ func runShell(c *nabat.Context) error { if err != nil { return fmt.Errorf("target cluster: %w", err) } - common.WarnContextFallback(c, cluster, opts.Environment) + cmdopts.WarnContextFallback(c, cluster, opts.Environment) executor, err := NewShellExecutor(cluster, c) if err != nil { diff --git a/internal/cmd/status/status.go b/internal/cmd/status/status.go index ce48955..037f149 100644 --- a/internal/cmd/status/status.go +++ b/internal/cmd/status/status.go @@ -8,7 +8,7 @@ import ( "nabat.dev/nabat" "deployah.dev/deployah/internal/cli" - "deployah.dev/deployah/internal/cmd/common" + "deployah.dev/deployah/internal/cmd/cmdopts" "deployah.dev/deployah/internal/k8s" "deployah.dev/deployah/internal/session" ) @@ -54,7 +54,7 @@ func runStatus(c *nabat.Context) error { if err != nil { return fmt.Errorf("target cluster: %w", err) } - common.WarnContextFallback(c, cluster, opts.Environment) + cmdopts.WarnContextFallback(c, cluster, opts.Environment) helmClient, err := cluster.Helm() if err != nil { return fmt.Errorf("helm client: %w", err) diff --git a/internal/helm/cache.go b/internal/helm/cache.go index d39201a..5bc555b 100644 --- a/internal/helm/cache.go +++ b/internal/helm/cache.go @@ -16,32 +16,41 @@ import ( "deployah.dev/deployah/internal/spec" ) -// ChartCache represents a cached chart entry -type ChartCache struct { - Path string - CreatedAt time.Time +const defaultChartCacheTTL = 1 * time.Hour + +// chartCacheEntry is one prepared chart directory in a [ChartCache]. +type chartCacheEntry struct { + path string + createdAt time.Time } -// ChartCacheInfo provides detailed information about the chart cache -type ChartCacheInfo struct { - Count int `json:"count"` - TotalSize int64 `json:"totalSize"` - ChartHash string `json:"chartHash"` - TTL string `json:"ttl"` - CacheKeys []string `json:"cacheKeys,omitempty"` +// ChartCache stores prepared chart directories keyed by content hash. +// Each [Client] owns one instance (see [NewClient] and [WithChartCache]). +// There is no process-global cache. Methods are safe for concurrent use. +type ChartCache struct { + mu sync.RWMutex + entries map[string]*chartCacheEntry + ttl time.Duration + + hashOnce sync.Once + embeddedHash string + embeddedError error } -// chartCache is a global cache for prepared charts -var ( - chartCache = make(map[string]*ChartCache) - chartCacheMutex sync.RWMutex - chartCacheTTL = 1 * time.Hour // Cache TTL - charts expire after 1 hour - embeddedChartHash string // Cached hash of embedded chart templates - chartHashOnce sync.Once // Ensure embedded chart hash is computed only once -) +// NewChartCache returns an empty chart cache with the given TTL. +// A non-positive ttl falls back to one hour. +func NewChartCache(ttl time.Duration) *ChartCache { + if ttl <= 0 { + ttl = defaultChartCacheTTL + } + return &ChartCache{ + entries: make(map[string]*chartCacheEntry), + ttl: ttl, + } +} -// GenerateCacheKey creates a cache key from the resolved spec (or raw spec -// when resolved is nil), the target environment, and the embedded chart +// GenerateKey creates a cache key from the resolved spec (or raw spec when +// resolved is nil), the target environment, and this cache's embedded chart // template hash. // // environment must be part of the key: [PrepareChart] bakes the @@ -53,7 +62,7 @@ var ( // covers only the target-environment subset and ensures platform file changes // invalidate the cache. encoding/json sorts map keys deterministically since // Go 1.12, so the serialization is stable. -func GenerateCacheKey(manifest *spec.Spec, environment string, resolved *spec.ResolvedSpec) (string, error) { +func (c *ChartCache) GenerateKey(manifest *spec.Spec, environment string, resolved *spec.ResolvedSpec) (string, error) { var inputBytes []byte var err error if resolved != nil { @@ -65,8 +74,7 @@ func GenerateCacheKey(manifest *spec.Spec, environment string, resolved *spec.Re return "", fmt.Errorf("failed to marshal spec for hashing: %w", err) } - // Get hash of embedded chart templates to detect chart updates. - chartHash, err := getEmbeddedChartHash() + chartHash, err := c.embeddedChartHash() if err != nil { return "", fmt.Errorf("failed to generate embedded chart hash: %w", err) } @@ -78,182 +86,110 @@ func GenerateCacheKey(manifest *spec.Spec, environment string, resolved *spec.Re return hex.EncodeToString(finalHash[:]), nil } -// getEmbeddedChartHash generates a hash of the embedded chart templates -// This ensures cache invalidation when the base Deployah chart is updated -// The hash is computed only once per application run for performance -func getEmbeddedChartHash() (string, error) { - var err error - - // Use sync.Once to ensure the hash is computed only once per application run - chartHashOnce.Do(func() { +// embeddedChartHash returns a hash of the embedded chart templates, computed +// once per [ChartCache] instance so cache keys invalidate when the base +// Deployah chart changes. +func (c *ChartCache) embeddedChartHash() (string, error) { + c.hashOnce.Do(func() { hasher := sha256.New() - // Walk through all embedded chart files and hash their content walkErr := fs.WalkDir(ChartTemplateFS, "chart", func(path string, d fs.DirEntry, walkErr error) error { if walkErr != nil { return fmt.Errorf("failed to access embedded chart file %s: %w", path, walkErr) } - - // Skip directories, only hash file contents if d.IsDir() { return nil } - // Read file content data, readErr := ChartTemplateFS.ReadFile(path) if readErr != nil { return fmt.Errorf("failed to read embedded file %s: %w", path, readErr) } - // Include file path and content in hash to detect both content and structure changes hasher.Write([]byte(path)) hasher.Write(data) - return nil }) if walkErr != nil { - err = fmt.Errorf("failed to walk embedded chart directory: %w", walkErr) + c.embeddedError = fmt.Errorf("failed to walk embedded chart directory: %w", walkErr) return } - - embeddedChartHash = hex.EncodeToString(hasher.Sum(nil)) + c.embeddedHash = hex.EncodeToString(hasher.Sum(nil)) }) - if err != nil { - return "", fmt.Errorf("failed to generate embedded chart hash: %w", err) + if c.embeddedError != nil { + return "", c.embeddedError } - - return embeddedChartHash, nil + return c.embeddedHash, nil } -// GetCachedChart retrieves a cached chart if it exists and is valid -func GetCachedChart(cacheKey string) (string, bool) { - chartCacheMutex.RLock() - defer chartCacheMutex.RUnlock() +// get returns a cached chart path when the entry exists, is unexpired, and +// the directory is still on disk. +func (c *ChartCache) get(cacheKey string) (string, bool) { + c.mu.RLock() + defer c.mu.RUnlock() - cache, exists := chartCache[cacheKey] + entry, exists := c.entries[cacheKey] if !exists { return "", false } - - // Check if cache entry has expired - if time.Since(cache.CreatedAt) > chartCacheTTL { + if time.Since(entry.createdAt) > c.ttl { return "", false } - - // Verify the cached directory still exists - if _, err := os.Stat(cache.Path); errors.Is(err, fs.ErrNotExist) { + if _, err := os.Stat(entry.path); errors.Is(err, fs.ErrNotExist) { return "", false } - - return cache.Path, true + return entry.path, true } -// SetCachedChart stores a chart path in the cache -func SetCachedChart(cacheKey, chartPath string) { - chartCacheMutex.Lock() - defer chartCacheMutex.Unlock() - - chartCache[cacheKey] = &ChartCache{ - Path: chartPath, - CreatedAt: time.Now(), +// set stores a chart path in the cache. +func (c *ChartCache) set(cacheKey, chartPath string) { + c.mu.Lock() + defer c.mu.Unlock() + c.entries[cacheKey] = &chartCacheEntry{ + path: chartPath, + createdAt: time.Now(), } } -// CleanupExpiredCharts removes expired chart cache entries -func CleanupExpiredCharts() { - chartCacheMutex.Lock() - defer chartCacheMutex.Unlock() - - now := time.Now() - for key, cache := range chartCache { - if now.Sub(cache.CreatedAt) > chartCacheTTL { - // Remove expired chart directory - if err := os.RemoveAll(cache.Path); err != nil { - // Log error but continue cleanup - slog.Warn("failed to cleanup expired chart cache", "path", cache.Path, "err", err) - } - delete(chartCache, key) - } - } +// entryCount returns the number of cache entries (including expired ones +// not yet cleaned up). +func (c *ChartCache) entryCount() int { + c.mu.RLock() + defer c.mu.RUnlock() + return len(c.entries) } -// ClearChartCache clears all cached charts and removes their directories -func ClearChartCache() error { - chartCacheMutex.Lock() - defer chartCacheMutex.Unlock() +// cleanupExpired removes expired chart cache entries and their directories. +func (c *ChartCache) cleanupExpired() { + c.mu.Lock() + defer c.mu.Unlock() - var errs []error - for key, cache := range chartCache { - if err := os.RemoveAll(cache.Path); err != nil { - errs = append(errs, fmt.Errorf("failed to remove %s: %w", cache.Path, err)) - } - delete(chartCache, key) - } - - if len(errs) > 0 { - return fmt.Errorf("errors during cache cleanup: %w", errors.Join(errs...)) - } - - return nil -} - -// GetChartCacheStats returns statistics about the chart cache -func GetChartCacheStats() (count int, totalSize int64) { - chartCacheMutex.RLock() - defer chartCacheMutex.RUnlock() - - count = len(chartCache) - for _, cache := range chartCache { - if size, err := getDirSize(cache.Path); err == nil { - totalSize += size - } - } - - return count, totalSize -} - -// GetChartCacheInfo returns detailed information about the chart cache -func GetChartCacheInfo() (*ChartCacheInfo, error) { - chartCacheMutex.RLock() - defer chartCacheMutex.RUnlock() - - info := &ChartCacheInfo{ - Count: len(chartCache), - TTL: chartCacheTTL.String(), - } - - // Get embedded chart hash - if hash, err := getEmbeddedChartHash(); err == nil { - info.ChartHash = hash - } - - // Calculate total size and collect cache keys - for key, cache := range chartCache { - info.CacheKeys = append(info.CacheKeys, key) - if size, err := getDirSize(cache.Path); err == nil { - info.TotalSize += size + now := time.Now() + for key, entry := range c.entries { + if now.Sub(entry.createdAt) > c.ttl { + if err := os.RemoveAll(entry.path); err != nil { + slog.Warn("failed to cleanup expired chart cache", "path", entry.path, "err", err) + } + delete(c.entries, key) } } - - return info, nil } -// CreateChartCopy creates a copy of a cached chart directory to avoid conflicts -func CreateChartCopy(sourcePath string) (string, error) { +// createChartCopy creates a copy of a cached chart directory so callers can +// delete their returned path without removing the cache's backing directory. +func createChartCopy(sourcePath string) (string, error) { tmpDir, err := os.MkdirTemp("", "deployah-chart-copy-*") if err != nil { return "", fmt.Errorf("failed to create temp dir for chart copy: %w", err) } - // Copy the entire directory tree err = filepath.Walk(sourcePath, func(path string, info os.FileInfo, err error) error { if err != nil { return fmt.Errorf("failed to access path %s: %w", path, err) } - // Calculate relative path relPath, err := filepath.Rel(sourcePath, path) if err != nil { return fmt.Errorf("failed to calculate relative path for %s: %w", path, err) @@ -265,7 +201,6 @@ func CreateChartCopy(sourcePath string) (string, error) { return os.MkdirAll(destPath, info.Mode()) } - // Copy file sourceFile, err := os.Open(path) // #nosec G304,G122 -- path from filepath.Walk within source tree if err != nil { return fmt.Errorf("failed to open source file %s: %w", path, err) @@ -302,18 +237,3 @@ func CreateChartCopy(sourcePath string) (string, error) { return tmpDir, nil } - -// getDirSize calculates the total size of a directory -func getDirSize(path string) (int64, error) { - var size int64 - err := filepath.Walk(path, func(_ string, info os.FileInfo, err error) error { - if err != nil { - return fmt.Errorf("failed to access file in directory %s: %w", path, err) - } - if !info.IsDir() { - size += info.Size() - } - return nil - }) - return size, err -} diff --git a/internal/helm/cache_test.go b/internal/helm/cache_test.go index dd5e992..4082801 100644 --- a/internal/helm/cache_test.go +++ b/internal/helm/cache_test.go @@ -17,6 +17,7 @@ package helm import ( "os" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -33,6 +34,7 @@ import ( // hitting the cache. func TestPrepareChart_CacheSurvivesCallerCleanup(t *testing.T) { t.Parallel() + cache := NewChartCache(time.Hour) manifest := &spec.Spec{ APIVersion: "v1-alpha.2", Project: "cache-test", @@ -40,13 +42,13 @@ func TestPrepareChart_CacheSurvivesCallerCleanup(t *testing.T) { } require.NoError(t, spec.FillSpecWithDefaults(manifest, "v1-alpha.2")) - returnedPath, err := PrepareChart(t.Context(), manifest, "production", nil) + returnedPath, err := PrepareChart(t.Context(), manifest, "production", nil, cache) require.NoError(t, err) - key, err := GenerateCacheKey(manifest, "production", nil) + key, err := cache.GenerateKey(manifest, "production", nil) require.NoError(t, err) - cachedPath, found := GetCachedChart(key) + cachedPath, found := cache.get(key) require.True(t, found, "PrepareChart must register a cache entry on a miss") t.Cleanup(func() { if removeErr := os.RemoveAll(cachedPath); removeErr != nil { @@ -61,6 +63,36 @@ func TestPrepareChart_CacheSurvivesCallerCleanup(t *testing.T) { // exactly as InstallApp/RenderManifests's deferred cleanup does. require.NoError(t, os.RemoveAll(returnedPath)) - _, stillFound := GetCachedChart(key) + _, stillFound := cache.get(key) assert.True(t, stillFound, "the cache entry must survive the caller cleaning up its own returned copy") } + +// TestPrepareChart_RequiresCache verifies PrepareChart rejects a nil cache. +func TestPrepareChart_RequiresCache(t *testing.T) { + t.Parallel() + _, err := PrepareChart(t.Context(), &spec.Spec{Project: "x"}, "prod", nil, nil) + require.Error(t, err) + assert.ErrorContains(t, err, "chart cache is required") +} + +// TestChartCache_Isolation verifies two ChartCache instances do not share +// entries. +func TestChartCache_Isolation(t *testing.T) { + t.Parallel() + a := NewChartCache(time.Hour) + b := NewChartCache(time.Hour) + + a.set("key", t.TempDir()) + _, foundA := a.get("key") + _, foundB := b.get("key") + assert.True(t, foundA) + assert.False(t, foundB, "caches must not share entries") +} + +// TestNewClient_RejectsNilChartCache verifies WithChartCache(nil) fails fast. +func TestNewClient_RejectsNilChartCache(t *testing.T) { + t.Parallel() + _, err := NewClient(WithStorageDriver("memory"), WithChartCache(nil)) + require.Error(t, err) + assert.ErrorContains(t, err, "chart cache is required") +} diff --git a/internal/helm/doc.go b/internal/helm/doc.go index 804f09d..3a2248b 100644 --- a/internal/helm/doc.go +++ b/internal/helm/doc.go @@ -15,7 +15,7 @@ // Package helm generates Helm charts from Deployah specs and drives // install, upgrade, list, and delete operations against a cluster. // -// [PrepareChart] renders templates and values for an environment. -// [Client] wraps Helm v4 actions with Deployah-specific release naming, -// labels, and caching through [ChartCache]. +// [PrepareChart] renders templates and values for an environment into a +// caller-supplied [ChartCache]. [Client] wraps Helm v4 actions with +// Deployah-specific release naming, labels, and a per-client [ChartCache]. package helm diff --git a/internal/helm/generate.go b/internal/helm/generate.go index b3ef68d..169d2e6 100644 --- a/internal/helm/generate.go +++ b/internal/helm/generate.go @@ -5,6 +5,7 @@ import ( "context" "embed" "encoding/json" + "errors" "fmt" "io/fs" "maps" @@ -81,26 +82,42 @@ func GenerateReleaseName(projectName, environmentName string) string { // PrepareChart expands the embedded chart into a temporary directory, // rendering .gotmpl files with Go templates and Sprig functions, and returns -// the prepared chart root directory (cached across calls for identical charts). -func PrepareChart(ctx context.Context, manifest *spec.Spec, desiredEnvironment string, resolved *spec.ResolvedSpec) (string, error) { +// the prepared chart root directory. Identical charts are reused via cache. +// +// cache must be non-nil. If ctx is already canceled or past its deadline, +// PrepareChart returns [context.Canceled] or [context.DeadlineExceeded] +// immediately; chart expansion itself is not interrupted mid-flight. +// +// On a cache miss, every 10th entry may start a background goroutine that +// removes expired cache directories; that work outlives this call. +// +// Errors: [context.Canceled], [context.DeadlineExceeded], or a wrapped +// error when cache is nil or chart generation fails. +func PrepareChart(ctx context.Context, manifest *spec.Spec, desiredEnvironment string, resolved *spec.ResolvedSpec, cache *ChartCache) (string, error) { + if err := ctx.Err(); err != nil { + return "", err + } + if cache == nil { + return "", errors.New("chart cache is required") + } + // Generate comprehensive cache key based on resolved spec (or raw spec if // no platform resolution was performed), the target environment, and // embedded chart templates. - cacheKey, err := GenerateCacheKey(manifest, desiredEnvironment, resolved) + cacheKey, err := cache.GenerateKey(manifest, desiredEnvironment, resolved) if err != nil { return "", fmt.Errorf("failed to generate cache key: %w", err) } - if cachedPath, found := GetCachedChart(cacheKey); found { + if cachedPath, found := cache.get(cacheKey); found { // Return a copy of the cached chart to avoid conflicts with cleanup - return CreateChartCopy(cachedPath) + return createChartCopy(cachedPath) } // Cleanup expired cache entries periodically (every 10th call) // This is a simple approach to avoid goroutine overhead - count, _ := GetChartCacheStats() - if count > 0 && count%10 == 0 { - go CleanupExpiredCharts() + if count := cache.entryCount(); count > 0 && count%10 == 0 { + go cache.cleanupExpired() } const root = "chart" @@ -196,9 +213,9 @@ func PrepareChart(ctx context.Context, manifest *spec.Spec, desiredEnvironment s // tmpDir must never be handed out directly; return a copy so the cache // entry survives caller cleanup. - SetCachedChart(cacheKey, tmpDir) + cache.set(cacheKey, tmpDir) - return CreateChartCopy(tmpDir) + return createChartCopy(tmpDir) } // createComponentSubCharts creates sub-chart directories for each diff --git a/internal/helm/helm.go b/internal/helm/helm.go index 637f91c..62ab519 100644 --- a/internal/helm/helm.go +++ b/internal/helm/helm.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "log/slog" + "net" "os" "strings" "time" @@ -22,6 +23,7 @@ import ( "deployah.dev/deployah/internal/spec" v1 "helm.sh/helm/v4/pkg/release/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" ) var ( @@ -30,8 +32,18 @@ var ( // ErrReleaseNotFound is returned when a Helm release does not exist. ErrReleaseNotFound = errors.New("release not found") // ErrReleaseAlreadyExists is returned when a Helm release already exists. + // + // Today this sentinel is produced only for user-facing wording in + // [Client.wrapHelmError] (via typed Kubernetes/Helm drivers when + // available, otherwise string matching). No caller matches it with + // [errors.Is] yet; treat it as message classification until one does. ErrReleaseAlreadyExists = errors.New("release already exists") // ErrReleasePending is returned when a Helm release has an operation in progress. + // + // Today this sentinel is produced only for user-facing wording in + // [Client.wrapHelmError] via string matching on Helm's untyped pending + // messages. No caller matches it with [errors.Is] yet; treat it as + // message classification until one does. ErrReleasePending = errors.New("another operation is in progress") ) @@ -46,6 +58,7 @@ type Client struct { extraKubeconfigPaths []string storageDriver string debug bool + chartCache *ChartCache } // Option is a functional option for configuring the Helm client @@ -104,18 +117,31 @@ func WithDebug(keep bool) Option { } } +// WithChartCache sets the prepared-chart cache used by this client. +// cache must be non-nil; [NewClient] rejects a nil cache. +func WithChartCache(cache *ChartCache) Option { + return func(c *Client) { + c.chartCache = cache + } +} + // NewClient initializes Helm action configuration with functional options. // Default storage driver is "secret" if not specified. // Default timeout is 5 minutes if not specified. +// Each client gets its own [ChartCache] unless [WithChartCache] is set. func NewClient(opts ...Option) (*Client, error) { c := &Client{ storageDriver: "secret", timeout: 5 * time.Minute, + chartCache: NewChartCache(defaultChartCacheTTL), } for _, opt := range opts { opt(c) } + if c.chartCache == nil { + return nil, errors.New("chart cache is required") + } settings := cli.New() @@ -205,7 +231,7 @@ func (c *Client) InstallApp(ctx context.Context, manifest *spec.Spec, environmen "deployah.dev/version": manifest.APIVersion, } - chartPath, err := PrepareChart(ctx, manifest, environment, resolved) + chartPath, err := PrepareChart(ctx, manifest, environment, resolved, c.chartCache) if err != nil { return fmt.Errorf("failed to prepare chart: %w", err) } @@ -407,26 +433,45 @@ func releaserListToV1(rs []release.Releaser) ([]*v1.Release, error) { // sentinel branches wrap both the sentinel and the original err (via two // %w verbs) so callers can match on either the sentinel with [errors.Is] // or inspect the underlying Helm/Kubernetes error text. +// +// Typed Kubernetes and Helm storage errors are classified first. String +// matching remains only for Helm messages that still lack stable sentinels +// (notably pending operations). func (c *Client) wrapHelmError(operation, releaseName string, err error) error { if errors.Is(err, driver.ErrReleaseNotFound) { return fmt.Errorf("release '%s': %w: %w", releaseName, ErrReleaseNotFound, err) } - errMsg := err.Error() + if errors.Is(err, driver.ErrReleaseExists) { + return fmt.Errorf("release '%s': %w: %w", releaseName, ErrReleaseAlreadyExists, err) + } + + switch { + case apierrors.IsNotFound(err): + return fmt.Errorf("release '%s': %w: %w", releaseName, ErrReleaseNotFound, err) + case apierrors.IsAlreadyExists(err): + return fmt.Errorf("release '%s': %w: %w", releaseName, ErrReleaseAlreadyExists, err) + case apierrors.IsForbidden(err), apierrors.IsUnauthorized(err): + return fmt.Errorf("insufficient permissions for %s operation on release '%s': %w", operation, releaseName, err) + case apierrors.IsTimeout(err), apierrors.IsServerTimeout(err): + return fmt.Errorf("operation timed out for release '%s': %w", releaseName, err) + } - // Check for common error patterns and provide helpful messages. - // Helm and Kubernetes internals do not expose typed sentinels for most of - // these cases, so string matching is the only option. + var opErr *net.OpError + if errors.As(err, &opErr) { + return fmt.Errorf("unable to connect to Kubernetes cluster: %w", err) + } + + // Helm still surfaces some conditions as plain strings only. + // Timeout/forbidden/unauthorized/already-exists string arms are omitted + // here because the typed checks above cover those Kubernetes cases. + errMsg := err.Error() switch { case strings.Contains(errMsg, "not found"): return fmt.Errorf("release '%s': %w: %w", releaseName, ErrReleaseNotFound, err) case strings.Contains(errMsg, "another operation") || strings.Contains(errMsg, "pending"): return fmt.Errorf("release '%s': %w: %w", releaseName, ErrReleasePending, err) - case strings.Contains(errMsg, "timeout"): - return fmt.Errorf("operation timed out for release '%s': %w", releaseName, err) case strings.Contains(errMsg, "connection refused") || strings.Contains(errMsg, "dial"): return fmt.Errorf("unable to connect to Kubernetes cluster: %w", err) - case strings.Contains(errMsg, "forbidden") || strings.Contains(errMsg, "unauthorized"): - return fmt.Errorf("insufficient permissions for %s operation on release '%s': %w", operation, releaseName, err) case strings.Contains(errMsg, "already exists"): return fmt.Errorf("release '%s': %w: %w", releaseName, ErrReleaseAlreadyExists, err) default: diff --git a/internal/helm/helm_error_test.go b/internal/helm/helm_error_test.go new file mode 100644 index 0000000..37f7c3a --- /dev/null +++ b/internal/helm/helm_error_test.go @@ -0,0 +1,130 @@ +// Copyright 2025 The Deployah Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package helm + +import ( + "errors" + "fmt" + "net" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "helm.sh/helm/v4/pkg/storage/driver" + "k8s.io/apimachinery/pkg/runtime/schema" + + apierrors "k8s.io/apimachinery/pkg/api/errors" +) + +// TestWrapHelmError_TypedClassification verifies typed Helm and Kubernetes +// errors map to the expected sentinels and user-facing messages. +func TestWrapHelmError_TypedClassification(t *testing.T) { + t.Parallel() + c := &Client{} + + tests := []struct { + name string + err error + wantIs error + wantMsg string + }{ + { + name: "helm driver not found", + err: driver.ErrReleaseNotFound, + wantIs: ErrReleaseNotFound, + }, + { + name: "helm driver already exists", + err: driver.ErrReleaseExists, + wantIs: ErrReleaseAlreadyExists, + }, + { + name: "k8s not found", + err: apierrors.NewNotFound( + schema.GroupResource{Resource: "secrets"}, "myrel.v1", + ), + wantIs: ErrReleaseNotFound, + }, + { + name: "k8s already exists", + err: apierrors.NewAlreadyExists( + schema.GroupResource{Resource: "secrets"}, "myrel.v1", + ), + wantIs: ErrReleaseAlreadyExists, + }, + { + name: "k8s forbidden", + err: apierrors.NewForbidden( + schema.GroupResource{Resource: "secrets"}, "myrel.v1", + errors.New("denied"), + ), + wantMsg: "insufficient permissions", + }, + { + name: "k8s unauthorized", + err: apierrors.NewUnauthorized("bad token"), + wantMsg: "insufficient permissions", + }, + { + name: "k8s timeout", + err: apierrors.NewTimeoutError("slow", 1), + wantMsg: "operation timed out", + }, + { + name: "net op error", + err: &net.OpError{ + Op: "dial", + Net: "tcp", + Err: errors.New("connection refused"), + }, + wantMsg: "unable to connect to Kubernetes cluster", + }, + { + name: "helm pending string", + err: errors.New("another operation (install/upgrade/rollback) is in progress"), + wantIs: ErrReleasePending, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := c.wrapHelmError("upgrade", "app-prod", tt.err) + require.Error(t, got) + if tt.wantIs != nil { + assert.ErrorIs(t, got, tt.wantIs) + } + if tt.wantMsg != "" { + assert.ErrorContains(t, got, tt.wantMsg) + } + // Original error remains inspectable through the wrap. + assert.ErrorIs(t, got, tt.err) + }) + } +} + +// TestWrapHelmError_PreservesWrappedCause verifies classification sees through +// a wrapping layer to the underlying Kubernetes API error. +func TestWrapHelmError_PreservesWrappedCause(t *testing.T) { + t.Parallel() + c := &Client{} + cause := apierrors.NewForbidden( + schema.GroupResource{Resource: "secrets"}, "x", errors.New("no"), + ) + wrapped := fmt.Errorf("helm failed: %w", cause) + got := c.wrapHelmError("install", "app-prod", wrapped) + assert.ErrorIs(t, got, cause) + assert.ErrorContains(t, got, "insufficient permissions") +} diff --git a/internal/helm/render.go b/internal/helm/render.go index 5b971ad..21b1092 100644 --- a/internal/helm/render.go +++ b/internal/helm/render.go @@ -100,7 +100,7 @@ func (c *Client) RenderOffline(ctx context.Context, manifest *spec.Spec, environ // constructed with WithDebug(true), cleanup is a no-op and the temp dir is // left behind for inspection. func (c *Client) prepareAndLoadChart(ctx context.Context, manifest *spec.Spec, environment string, resolved *spec.ResolvedSpec) (ch *chart.Chart, chartPath string, cleanup func(), err error) { - chartPath, err = PrepareChart(ctx, manifest, environment, resolved) + chartPath, err = PrepareChart(ctx, manifest, environment, resolved, c.chartCache) if err != nil { return nil, "", nil, fmt.Errorf("failed to prepare chart: %w", err) } diff --git a/internal/util/doc.go b/internal/validate/doc.go similarity index 89% rename from internal/util/doc.go rename to internal/validate/doc.go index 594ea32..27833a4 100644 --- a/internal/util/doc.go +++ b/internal/validate/doc.go @@ -12,10 +12,10 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package util provides small validation helpers shared by CLI prompts and +// Package validate provides small validation helpers shared by CLI prompts and // spec processing. // // Functions such as [ValidatePositiveInteger], [ValidateNonEmpty], and // [ValidateResourceString] centralize input checks so interactive init and // configuration paths enforce the same rules. -package util +package validate diff --git a/internal/util/validate.go b/internal/validate/validate.go similarity index 99% rename from internal/util/validate.go rename to internal/validate/validate.go index 3236d06..7e19794 100644 --- a/internal/util/validate.go +++ b/internal/validate/validate.go @@ -1,4 +1,4 @@ -package util +package validate import ( "fmt" diff --git a/internal/util/validate_test.go b/internal/validate/validate_test.go similarity index 99% rename from internal/util/validate_test.go rename to internal/validate/validate_test.go index 68573f5..47d370a 100644 --- a/internal/util/validate_test.go +++ b/internal/validate/validate_test.go @@ -1,4 +1,4 @@ -package util +package validate import ( "testing" From 7d15eaa6448bc10cbbb3cba576a156cfa773d202 Mon Sep 17 00:00:00 2001 From: Mohammad Abdolirad Date: Mon, 3 Aug 2026 12:47:59 +0200 Subject: [PATCH 2/3] raise patch coverage for watcher, cache, shell - cover refreshPodStatus stale/cancel paths and finalRefresh success - exercise ChartCache expiry, cleanup, and createChartCopy - extract startTerminalResizeWatch for unit-tested SIGWINCH lifecycle --- internal/cmd/deploy/watcher_test.go | 116 ++++++++++++++++++++++++++++ internal/cmd/shell/executor.go | 71 +++++++++-------- internal/cmd/shell/executor_test.go | 51 ++++++++++++ internal/helm/cache_test.go | 98 +++++++++++++++++++++++ 4 files changed, 303 insertions(+), 33 deletions(-) diff --git a/internal/cmd/deploy/watcher_test.go b/internal/cmd/deploy/watcher_test.go index f3564c3..5c4001a 100644 --- a/internal/cmd/deploy/watcher_test.go +++ b/internal/cmd/deploy/watcher_test.go @@ -15,18 +15,24 @@ package deploy import ( + "context" + "errors" "testing" "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes/fake" "nabat.dev/nabat" "nabat.dev/nabat/nabattest" "deployah.dev/deployah/internal/k8s" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + clienttesting "k8s.io/client-go/testing" ) // newWatcher builds a DeployWatcher for tests. @@ -250,3 +256,113 @@ func TestDeployWatcher_UpdateTitle_StaleWithoutPods(t *testing.T) { assert.Equal(t, "pod status unavailable (retrying...)", spy.title) } + +func readyPod(name, component, release string) *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: "default", + Labels: map[string]string{ + "app.kubernetes.io/instance": release, + "deployah.dev/component": component, + }, + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + ContainerStatuses: []corev1.ContainerStatus{{Ready: true}}, + }, + } +} + +// TestDeployWatcher_RefreshPodStatus_Success verifies a successful poll +// updates the summary and clears a previous stale flag. +func TestDeployWatcher_RefreshPodStatus_Success(t *testing.T) { + t.Parallel() + cs := fake.NewSimpleClientset(readyPod("api-1", "api", "myapp-prod")) + w := NewDeployWatcher(cs, "default", "myapp-prod") + w.mu.Lock() + w.pollStale = true + w.mu.Unlock() + + w.refreshPodStatus(t.Context()) + + summary := w.Summary() + require.Len(t, summary, 1) + assert.Equal(t, "api", summary[0].Name) + assert.Equal(t, 1, summary[0].ReadyPods) + assert.Equal(t, 1, summary[0].TotalPods) + w.mu.Lock() + stale := w.pollStale + w.mu.Unlock() + assert.False(t, stale) +} + +// TestDeployWatcher_RefreshPodStatus_PollErrorMarksStale verifies API +// failures leave the prior summary intact and set pollStale. +func TestDeployWatcher_RefreshPodStatus_PollErrorMarksStale(t *testing.T) { + t.Parallel() + cs := fake.NewSimpleClientset() + cs.PrependReactor("list", "pods", func(clienttesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("api unavailable") + }) + w := NewDeployWatcher(cs, "default", "myapp-prod") + w.mu.Lock() + w.summary = []ComponentStatus{{Name: "api", ReadyPods: 1, TotalPods: 2}} + w.mu.Unlock() + + w.refreshPodStatus(t.Context()) + + summary := w.Summary() + require.Len(t, summary, 1) + assert.Equal(t, 1, summary[0].ReadyPods) + w.mu.Lock() + stale := w.pollStale + w.mu.Unlock() + assert.True(t, stale) +} + +// TestDeployWatcher_RefreshPodStatus_CanceledSkipsStale verifies shutdown +// cancellation does not mark status stale. +func TestDeployWatcher_RefreshPodStatus_CanceledSkipsStale(t *testing.T) { + t.Parallel() + cs := fake.NewSimpleClientset() + w := NewDeployWatcher(cs, "default", "myapp-prod") + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + w.refreshPodStatus(ctx) + + w.mu.Lock() + stale := w.pollStale + w.mu.Unlock() + assert.False(t, stale) +} + +// TestDeployWatcher_FinalRefresh_AllReady verifies finalRefresh exits once +// pods are ready even when the parent context is already canceled. +func TestDeployWatcher_FinalRefresh_AllReady(t *testing.T) { + t.Parallel() + cs := fake.NewSimpleClientset(readyPod("api-1", "api", "myapp-prod")) + w := NewDeployWatcher(cs, "default", "myapp-prod") + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + runStatus(t, func(st *nabat.Status) { + w.finalRefresh(ctx, st) + }, "deploy") + + assert.True(t, w.allReady()) + w.mu.Lock() + stale := w.pollStale + w.mu.Unlock() + assert.False(t, stale) +} + +// TestDeployWatcher_AllReady_EmptyIsFalse verifies no observed pods is not +// treated as ready. +func TestDeployWatcher_AllReady_EmptyIsFalse(t *testing.T) { + t.Parallel() + w := newWatcher() + assert.False(t, w.allReady()) +} diff --git a/internal/cmd/shell/executor.go b/internal/cmd/shell/executor.go index 929318b..056467c 100644 --- a/internal/cmd/shell/executor.go +++ b/internal/cmd/shell/executor.go @@ -50,6 +50,42 @@ func terminalSizeFromWH(w, h int) remotecommand.TerminalSize { return remotecommand.TerminalSize{Width: uint16(w), Height: uint16(h)} } +// startTerminalResizeWatch seeds q with the current terminal size, then +// forwards SIGWINCH updates until the returned stop func runs. getSize is +// usually [term.GetSize]; tests inject a stub. +func startTerminalResizeWatch(fd int, q *terminalSizeQueue, getSize func(int) (int, int, error)) (stop func()) { + if w, h, sizeErr := getSize(fd); sizeErr == nil { + q.ch <- terminalSizeFromWH(w, h) + } + + resizeCh := make(chan os.Signal, 1) + done := make(chan struct{}) + signal.Notify(resizeCh, syscall.SIGWINCH) + var wg sync.WaitGroup + wg.Go(func() { + for { + select { + case <-done: + return + case <-resizeCh: + if w, h, sizeErr := getSize(fd); sizeErr == nil { + select { + case q.ch <- terminalSizeFromWH(w, h): + case <-done: + return + } + } + } + } + }) + return func() { + signal.Stop(resizeCh) + close(done) + wg.Wait() + close(q.ch) + } +} + // ExecuteOptions configures an interactive or one-shot shell session. type ExecuteOptions struct { // ProjectName is the Deployah project name. @@ -351,41 +387,10 @@ func (e *ShellExecutor) execInContainer(podName, containerName string, cmd []str } var sizeQueue remotecommand.TerminalSizeQueue - var q *terminalSizeQueue if isTTY { - q = &terminalSizeQueue{ch: make(chan remotecommand.TerminalSize, 1)} + q := &terminalSizeQueue{ch: make(chan remotecommand.TerminalSize, 1)} sizeQueue = q - - if w, h, sizeErr := term.GetSize(fd); sizeErr == nil { - q.ch <- terminalSizeFromWH(w, h) - } - - resizeCh := make(chan os.Signal, 1) - done := make(chan struct{}) - signal.Notify(resizeCh, syscall.SIGWINCH) - var wg sync.WaitGroup - wg.Go(func() { - for { - select { - case <-done: - return - case <-resizeCh: - if w, h, sizeErr := term.GetSize(fd); sizeErr == nil { - select { - case q.ch <- terminalSizeFromWH(w, h): - case <-done: - return - } - } - } - } - }) - defer func() { - signal.Stop(resizeCh) - close(done) - wg.Wait() - close(q.ch) - }() + defer startTerminalResizeWatch(fd, q, term.GetSize)() } req := k8sClient.CoreV1().RESTClient().Post(). diff --git a/internal/cmd/shell/executor_test.go b/internal/cmd/shell/executor_test.go index a0e113a..41d0862 100644 --- a/internal/cmd/shell/executor_test.go +++ b/internal/cmd/shell/executor_test.go @@ -22,6 +22,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "k8s.io/client-go/tools/remotecommand" ) // TestShellQuote verifies POSIX-safe quoting via shellQuote. @@ -151,3 +152,53 @@ func TestTerminalSizeFromWH(t *testing.T) { assert.Equal(t, uint16(0), sz.Width) assert.Equal(t, uint16(^uint16(0)), sz.Height) } + +// TestTerminalSizeQueue_Next verifies Next returns sizes until the channel +// closes, then returns nil. +func TestTerminalSizeQueue_Next(t *testing.T) { + t.Parallel() + q := &terminalSizeQueue{ch: make(chan remotecommand.TerminalSize, 1)} + q.ch <- remotecommand.TerminalSize{Width: 80, Height: 24} + + sz := q.Next() + require.NotNil(t, sz) + assert.Equal(t, uint16(80), sz.Width) + assert.Equal(t, uint16(24), sz.Height) + + close(q.ch) + assert.Nil(t, q.Next()) +} + +// TestStartTerminalResizeWatch_SeedsAndStops verifies the watch seeds an +// initial size and that stop closes the queue after the resize goroutine exits. +func TestStartTerminalResizeWatch_SeedsAndStops(t *testing.T) { + t.Parallel() + q := &terminalSizeQueue{ch: make(chan remotecommand.TerminalSize, 2)} + calls := 0 + getSize := func(int) (int, int, error) { + calls++ + return 100, 40, nil + } + + stop := startTerminalResizeWatch(0, q, getSize) + sz := q.Next() + require.NotNil(t, sz) + assert.Equal(t, uint16(100), sz.Width) + assert.Equal(t, uint16(40), sz.Height) + + stop() + assert.Nil(t, q.Next()) + assert.GreaterOrEqual(t, calls, 1) +} + +// TestStartTerminalResizeWatch_GetSizeErrorSkipsSeed verifies a failing +// getSize leaves the queue empty until stop closes it. +func TestStartTerminalResizeWatch_GetSizeErrorSkipsSeed(t *testing.T) { + t.Parallel() + q := &terminalSizeQueue{ch: make(chan remotecommand.TerminalSize, 1)} + stop := startTerminalResizeWatch(0, q, func(int) (int, int, error) { + return 0, 0, errors.New("no tty") + }) + stop() + assert.Nil(t, q.Next()) +} diff --git a/internal/helm/cache_test.go b/internal/helm/cache_test.go index 4082801..70f2bc5 100644 --- a/internal/helm/cache_test.go +++ b/internal/helm/cache_test.go @@ -15,7 +15,9 @@ package helm import ( + "context" "os" + "path/filepath" "testing" "time" @@ -96,3 +98,99 @@ func TestNewClient_RejectsNilChartCache(t *testing.T) { require.Error(t, err) assert.ErrorContains(t, err, "chart cache is required") } + +// TestNewChartCache_NonPositiveTTLUsesDefault verifies a non-positive TTL +// falls back to the package default rather than expiring immediately. +func TestNewChartCache_NonPositiveTTLUsesDefault(t *testing.T) { + t.Parallel() + cache := NewChartCache(0) + dir := t.TempDir() + cache.set("k", dir) + path, found := cache.get("k") + require.True(t, found) + assert.Equal(t, dir, path) +} + +// TestChartCache_GetMissExpiredAndMissingDir verifies get returns false for +// unknown keys, TTL expiry, and deleted backing directories. +func TestChartCache_GetMissExpiredAndMissingDir(t *testing.T) { + t.Parallel() + cache := NewChartCache(time.Millisecond) + + _, found := cache.get("missing") + assert.False(t, found) + + dir := t.TempDir() + cache.set("expired", dir) + require.Eventually(t, func() bool { + _, ok := cache.get("expired") + return !ok + }, 50*time.Millisecond, time.Millisecond) + assert.Equal(t, 1, cache.entryCount(), "expired entries remain until cleanup") + + gone := filepath.Join(t.TempDir(), "removed") + cache.set("gone", gone) + _, found = cache.get("gone") + assert.False(t, found) +} + +// TestChartCache_CleanupExpired removes expired entries and their directories. +func TestChartCache_CleanupExpired(t *testing.T) { + t.Parallel() + cache := NewChartCache(time.Millisecond) + dir := t.TempDir() + keep := t.TempDir() + cache.set("old", dir) + cache.set("fresh", keep) + + require.Eventually(t, func() bool { + _, ok := cache.get("old") + return !ok + }, 50*time.Millisecond, time.Millisecond) + + // Refresh the fresh entry so only "old" is past TTL when cleanup runs. + cache.set("fresh", keep) + cache.cleanupExpired() + + assert.Equal(t, 1, cache.entryCount()) + _, found := cache.get("fresh") + assert.True(t, found) + _, err := os.Stat(dir) + assert.ErrorIs(t, err, os.ErrNotExist) +} + +// TestCreateChartCopy_CopiesFiles verifies createChartCopy duplicates file +// contents into a new temp directory, including nested paths. +func TestCreateChartCopy_CopiesFiles(t *testing.T) { + t.Parallel() + src := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(src, "templates"), 0o750)) + require.NoError(t, os.WriteFile(filepath.Join(src, "values.yaml"), []byte("x: 1"), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(src, "templates", "app.yaml"), []byte("kind: Pod"), 0o600)) + + dst, err := createChartCopy(src) + require.NoError(t, err) + t.Cleanup(func() { + if removeErr := os.RemoveAll(dst); removeErr != nil { + t.Logf("cleanup: remove chart copy: %v", removeErr) + } + }) + + got, err := os.ReadFile(filepath.Join(dst, "values.yaml")) // #nosec G304 -- path under test-controlled temp dir + require.NoError(t, err) + assert.Equal(t, []byte("x: 1"), got) + nested, err := os.ReadFile(filepath.Join(dst, "templates", "app.yaml")) // #nosec G304 -- path under test-controlled temp dir + require.NoError(t, err) + assert.Equal(t, []byte("kind: Pod"), nested) + assert.NotEqual(t, src, dst) +} + +// TestPrepareChart_CanceledContextReturnsImmediately verifies PrepareChart +// honors an already-canceled context before doing chart work. +func TestPrepareChart_CanceledContextReturnsImmediately(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(t.Context()) + cancel() + _, err := PrepareChart(ctx, &spec.Spec{Project: "x"}, "prod", nil, NewChartCache(time.Hour)) + require.ErrorIs(t, err, context.Canceled) +} From 9e27d36ef69761dfb245ac28de58c12fa744742d Mon Sep 17 00:00:00 2001 From: Mohammad Abdolirad Date: Mon, 3 Aug 2026 13:06:14 +0200 Subject: [PATCH 3/3] cover finalRefresh timeout and SIGWINCH - shrink final refresh waits in tests via package vars - exercise deadline warning path and terminal resize signal --- internal/cmd/deploy/watcher.go | 9 ++++++-- internal/cmd/deploy/watcher_test.go | 29 ++++++++++++++++++++++++ internal/cmd/shell/executor_test.go | 35 +++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 2 deletions(-) diff --git a/internal/cmd/deploy/watcher.go b/internal/cmd/deploy/watcher.go index 17a9eb7..ccf21dd 100644 --- a/internal/cmd/deploy/watcher.go +++ b/internal/cmd/deploy/watcher.go @@ -32,10 +32,15 @@ import ( ) const ( - pollInterval = 3 * time.Second + pollInterval = 3 * time.Second + finalRefreshBudget = 2 * time.Second +) + +// finalRefreshPoll and finalRefreshTimeout are package vars so unit tests can +// shrink the wait without sleeping for the production 10s timeout. +var ( finalRefreshPoll = 1 * time.Second finalRefreshTimeout = 10 * time.Second - finalRefreshBudget = 2 * time.Second ) // ComponentStatus summarizes pod readiness for one Deployah component at diff --git a/internal/cmd/deploy/watcher_test.go b/internal/cmd/deploy/watcher_test.go index 5c4001a..95655ef 100644 --- a/internal/cmd/deploy/watcher_test.go +++ b/internal/cmd/deploy/watcher_test.go @@ -359,6 +359,35 @@ func TestDeployWatcher_FinalRefresh_AllReady(t *testing.T) { assert.False(t, stale) } +// TestDeployWatcher_FinalRefresh_TimeoutMarksWarning verifies finalRefresh +// stops after the deadline when pods never become ready and marks the +// status completion as a warning. Not parallel: mutates package-level +// timeout vars. +func TestDeployWatcher_FinalRefresh_TimeoutMarksWarning(t *testing.T) { + origPoll := finalRefreshPoll + origTimeout := finalRefreshTimeout + finalRefreshPoll = 5 * time.Millisecond + finalRefreshTimeout = 20 * time.Millisecond + t.Cleanup(func() { + finalRefreshPoll = origPoll + finalRefreshTimeout = origTimeout + }) + + cs := fake.NewSimpleClientset() // no pods => never allReady + w := NewDeployWatcher(cs, "default", "myapp-prod") + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + runStatus(t, func(st *nabat.Status) { + w.finalRefresh(ctx, st) + }, "deploy") + + // Empty cluster never becomes ready, so exit implies the deadline branch + // (SetCompletion(RowWarning)) ran rather than the allReady early return. + assert.False(t, w.allReady()) +} + // TestDeployWatcher_AllReady_EmptyIsFalse verifies no observed pods is not // treated as ready. func TestDeployWatcher_AllReady_EmptyIsFalse(t *testing.T) { diff --git a/internal/cmd/shell/executor_test.go b/internal/cmd/shell/executor_test.go index 41d0862..e0f7301 100644 --- a/internal/cmd/shell/executor_test.go +++ b/internal/cmd/shell/executor_test.go @@ -17,6 +17,8 @@ package shell import ( "errors" "fmt" + "os" + "sync" "syscall" "testing" @@ -202,3 +204,36 @@ func TestStartTerminalResizeWatch_GetSizeErrorSkipsSeed(t *testing.T) { stop() assert.Nil(t, q.Next()) } + +// TestStartTerminalResizeWatch_SIGWINCHUpdatesSize verifies a SIGWINCH +// delivers a new size on the queue. Not parallel: [signal.Notify] is +// process-wide for SIGWINCH. +func TestStartTerminalResizeWatch_SIGWINCHUpdatesSize(t *testing.T) { + q := &terminalSizeQueue{ch: make(chan remotecommand.TerminalSize, 4)} + var mu sync.Mutex + call := 0 + getSize := func(int) (int, int, error) { + mu.Lock() + defer mu.Unlock() + call++ + if call == 1 { + return 80, 24, nil + } + return 120, 40, nil + } + + stop := startTerminalResizeWatch(0, q, getSize) + t.Cleanup(stop) + + seed := q.Next() + require.NotNil(t, seed) + assert.Equal(t, uint16(80), seed.Width) + assert.Equal(t, uint16(24), seed.Height) + + require.NoError(t, syscall.Kill(os.Getpid(), syscall.SIGWINCH)) + + resized := q.Next() + require.NotNil(t, resized) + assert.Equal(t, uint16(120), resized.Width) + assert.Equal(t, uint16(40), resized.Height) +}