From 71527370de7b2976b93b5bd73e6dfa9bb3f4cf05 Mon Sep 17 00:00:00 2001 From: Mohammad Abdolirad Date: Tue, 11 Aug 2026 23:12:50 +0200 Subject: [PATCH] harden helm pending checks and CRD coverage - reject pending releases with typed Status.IsPending before chart prep - add CRD lifecycle e2e plus chart-cache and install-order unit tests - document CRD failure modes and tighten the Helm abstraction note --- docs/comparison.md | 7 +- docs/custom-manifests-and-crds.md | 10 + internal/cmd/deploy/deploy_flow_test.go | 29 +++ internal/e2e/e2e_test.go | 182 +++++++++++++++++- .../.deployah/crds/clusterwidget.yaml | 24 +++ .../e2e/testdata/crd-lifecycle/deployah.yaml | 10 + internal/helm/cache_test.go | 46 +++++ internal/helm/generate.go | 6 + internal/helm/helm.go | 104 +++++++--- internal/helm/helm_error_test.go | 148 +++++++++++++- internal/session/session.go | 14 +- 11 files changed, 537 insertions(+), 43 deletions(-) create mode 100644 internal/e2e/testdata/crd-lifecycle/.deployah/crds/clusterwidget.yaml create mode 100644 internal/e2e/testdata/crd-lifecycle/deployah.yaml diff --git a/docs/comparison.md b/docs/comparison.md index 12a153f..ef7fb9f 100644 --- a/docs/comparison.md +++ b/docs/comparison.md @@ -36,8 +36,11 @@ Kubernetes work. They **move it** to a platform team or to a server that runs inside your cluster. > **An honest note:** you do not need Helm to *use* Deployah. But the output is a -> real Helm release, so a little Helm knowledge helps if you want to debug deeply. -> For example, you can run `helm history` or `helm get` on what Deployah installed. +> real Helm release, so a little Helm knowledge helps when you debug the release +> itself (for example `helm history` or `helm get`). Custom CRDs under +> `.deployah/crds/` are another place the abstraction thins: they are applied +> outside the Helm release, with their own install policy. See +> [Custom manifests and CRDs](custom-manifests-and-crds.md). ## How much Helm you need (from most to least) diff --git a/docs/custom-manifests-and-crds.md b/docs/custom-manifests-and-crds.md index 4f29343..b6b2cf6 100644 --- a/docs/custom-manifests-and-crds.md +++ b/docs/custom-manifests-and-crds.md @@ -126,4 +126,14 @@ then applies the Helm release. If the Helm plan has no changes but left alone). CRDs are never pruned and are never deleted on uninstall. Extra manifests leave with the release. +## Failure modes + +CRD apply and the Helm release are **not** one atomic operation. Deployah +applies CRDs first, waits for `Established`, then installs or upgrades the +release. If the Helm step fails after CRDs succeed, those CRDs stay in the +cluster (Deployah never rolls them back). If CRD apply fails, Deployah does +not call Helm. Re-run `deployah deploy` after fixing the failure; already +present CRDs are left alone under `--crds create`, or updated under +`--crds create-replace`. + See the [README](../README.md) for the project overview and the other guides. diff --git a/internal/cmd/deploy/deploy_flow_test.go b/internal/cmd/deploy/deploy_flow_test.go index 06ff808..dee91b5 100644 --- a/internal/cmd/deploy/deploy_flow_test.go +++ b/internal/cmd/deploy/deploy_flow_test.go @@ -15,6 +15,7 @@ package deploy import ( + "errors" "path/filepath" "testing" @@ -396,6 +397,34 @@ func TestApplyDeploy_CallsInstallAfterEmptyCRDs(t *testing.T) { assert.Contains(t, stderr.String(), "Deployed") } +// TestApplyDeploy_PropagatesInstallErrorAfterCRDStep locks ordering: the CRD +// step runs first (empty list is a successful no-op), then InstallApp. A +// Helm failure after that step is returned to the caller. CRD survival +// across deployah delete is covered by the e2e CRD lifecycle test. +func TestApplyDeploy_PropagatesInstallErrorAfterCRDStep(t *testing.T) { + t.Parallel() + manifest := deployFlowManifestV1 + stub := &stubHelmClient{ + renderResults: []*render.RenderResult{testRenderResult(manifest)}, + installErr: errors.New("helm boom"), + } + cluster := newClusterWithStub(t, stub, nil) + sess := cluster.Session + planned := &deployPlan{ + diff: &planengine.Plan{Header: planengine.Header{Release: "web-production", Revision: 1}}, + result: testRenderResult(manifest), + cleanup: func() {}, + } + c := nabatContext(t) + opts := &Options{Environment: "production", CRDs: string(extras.PolicyCreate)} + + err := applyDeploy(c, sess, cluster, stub, nil, &spec.Spec{Project: "web"}, opts, nil, planned, nil, assertNever{}, &extras.Bundle{}, nil, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "deploy failed") + assert.Contains(t, err.Error(), "helm boom") + assert.Equal(t, 1, stub.installCallCount, "InstallApp must run after the CRD step") +} + // TestApplyDeploy_PropagatesCRDApplyError skips InstallApp when CRDs fail. func TestApplyDeploy_PropagatesCRDApplyError(t *testing.T) { t.Parallel() diff --git a/internal/e2e/e2e_test.go b/internal/e2e/e2e_test.go index 88c6e2d..fe69a3c 100644 --- a/internal/e2e/e2e_test.go +++ b/internal/e2e/e2e_test.go @@ -17,9 +17,11 @@ package e2e_test import ( + "context" "encoding/json" "errors" "fmt" + "io" "os" "os/exec" "path/filepath" @@ -45,6 +47,8 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + apiextensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -245,6 +249,84 @@ func (s *E2ESuite) TestStatefulScale() { assert.GreaterOrEqual(t, matched, 2, "expected per-pod PVCs after scale-up") } +const crdLifecycleName = "clusterwidgets.example.com" + +// TestCRDLifecycle covers CRD apply outside the Helm release: Established +// before install, idle-Helm re-apply, create vs create-replace, and survival +// across deployah delete. +func (s *E2ESuite) TestCRDLifecycle() { + t := s.T() + src := filepath.Join(s.testdataDir, "crd-lifecycle") + require.DirExists(t, src) + + dir := t.TempDir() + copyTree(t, src, dir) + t.Chdir(dir) + + ext := newApiextensionsClient(t, s.kcPath, "kind-deployah") + t.Cleanup(func() { + // t.Context() is canceled just before Cleanup runs (Go 1.24+), so + // teardown API calls need an independent context. + cleanupCtx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + // Best-effort: remove the fixture CRD so later suite runs stay clean. + if delCRDErr := ext.ApiextensionsV1().CustomResourceDefinitions().Delete( + cleanupCtx, crdLifecycleName, metav1.DeleteOptions{}); delCRDErr != nil { + t.Logf("cleanup CRD delete failed (non-fatal): %v", delCRDErr) + } + if delErr := runErr(t, "delete", "crd-lifecycle", "dev", + "--yes", "--wait", "--allow-missing-platform", + "--context", "kind-deployah"); delErr != nil { + t.Logf("cleanup delete failed (non-fatal): %v", delErr) + } + }) + + // First deploy installs the CRD and the release. + run(t, "deploy", "dev", "--context", "kind-deployah", "--yes", "--crds", "create") + crd := waitCRDEstablished(t, ext, crdLifecycleName) + assert.Equal(t, "crd-lifecycle", crd.Labels["e2e-marker"]) + + // Idle Helm plan must still visit CRDs (already present). Success messages + // go to stderr via nabat, so assert via a dedicated IO capture. + _, stderr := runCapture(t, "deploy", "dev", "--context", "kind-deployah", "--yes", "--crds", "create") + assert.Contains(t, stderr, "already present") + waitCRDEstablished(t, ext, crdLifecycleName) + + // --crds create leaves an existing CRD alone when the file changes. + patched := strings.Replace( + readFixtureFile(t, filepath.Join(dir, ".deployah", "crds", "clusterwidget.yaml")), + `e2e-marker: "crd-lifecycle"`, + `e2e-marker: "create-skipped"`, + 1, + ) + require.NoError(t, os.WriteFile( + filepath.Join(dir, ".deployah", "crds", "clusterwidget.yaml"), + []byte(patched), 0o600)) + run(t, "deploy", "dev", "--context", "kind-deployah", "--yes", "--crds", "create") + crd = getCRD(t, ext, crdLifecycleName) + assert.Equal(t, "crd-lifecycle", crd.Labels["e2e-marker"], + "--crds create must not replace an existing CRD") + + // --crds create-replace server-side-applies over the existing CRD. + run(t, "deploy", "dev", "--context", "kind-deployah", "--yes", "--crds", "create-replace") + require.NoError(t, wait.For(func(ctx context.Context) (bool, error) { + live, getErr := ext.ApiextensionsV1().CustomResourceDefinitions().Get( + ctx, crdLifecycleName, metav1.GetOptions{}) + if getErr != nil { + return false, getErr + } + return live.Labels["e2e-marker"] == "create-skipped", nil + }, wait.WithTimeout(2*time.Minute), wait.WithInterval(time.Second))) + + // CRDs are never pruned on uninstall. + run(t, "delete", "crd-lifecycle", "dev", + "--yes", "--wait", "--allow-missing-platform", + "--context", "kind-deployah") + _, err := ext.ApiextensionsV1().CustomResourceDefinitions().Get( + t.Context(), crdLifecycleName, metav1.GetOptions{}) + require.NoError(t, err, "CRD must survive deployah delete") +} + // TestDeployScenarios deploys each discovered fixture and asserts expect.yaml. func (s *E2ESuite) TestDeployScenarios() { for _, sc := range s.scenarios { @@ -492,18 +574,108 @@ func loadExpectations(t testing.TB, dir string) expectations { func run(t testing.TB, args ...string) string { t.Helper() - io, _, out, errOut := nabattest.NewIO() - app := cmd.NewApp(nabat.WithIO(io)) + stdout, _ := runCapture(t, args...) + return stdout +} + +// runCapture runs deployah and returns stdout and stderr on success. +func runCapture(t testing.TB, args ...string) (stdout, stderr string) { + t.Helper() + appIO, _, out, errOut := nabattest.NewIO() + app := cmd.NewApp(nabat.WithIO(appIO)) err := nabattest.Run(t, app, args) require.NoErrorf(t, err, "deployah %s\nstderr:\n%s", strings.Join(args, " "), errOut.String()) - return out.String() + return out.String(), errOut.String() +} + +func copyTree(t testing.TB, src, dst string) { + t.Helper() + err := filepath.WalkDir(src, func(path string, d os.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + rel, relErr := filepath.Rel(src, path) + if relErr != nil { + return relErr + } + target := filepath.Join(dst, rel) + if d.IsDir() { + return os.MkdirAll(target, 0o750) + } + in, openErr := os.Open(path) // #nosec G304 -- path under testdata/ + if openErr != nil { + return openErr + } + defer in.Close() //nolint:errcheck // read-only copy helper + out, createErr := os.OpenFile(target, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600) // #nosec G304 -- temp fixture copy + if createErr != nil { + return createErr + } + _, copyErr := io.Copy(out, in) + closeErr := out.Close() + if copyErr != nil { + return copyErr + } + return closeErr + }) + require.NoError(t, err) +} + +func readFixtureFile(t testing.TB, path string) string { + t.Helper() + raw, err := os.ReadFile(path) // #nosec G304 -- path under test-controlled temp dir + require.NoError(t, err) + return string(raw) +} + +func newApiextensionsClient(t testing.TB, kubeconfigPath, contextName string) apiextensionsclient.Interface { + t.Helper() + rules := clientcmd.NewDefaultClientConfigLoadingRules() + rules.ExplicitPath = kubeconfigPath + overrides := &clientcmd.ConfigOverrides{CurrentContext: contextName} + restCfg, err := clientcmd.NewNonInteractiveDeferredLoadingClientConfig( + rules, overrides).ClientConfig() + require.NoError(t, err) + cs, err := apiextensionsclient.NewForConfig(restCfg) + require.NoError(t, err) + return cs +} + +func getCRD(t testing.TB, ext apiextensionsclient.Interface, name string) *apiextensionsv1.CustomResourceDefinition { + t.Helper() + crd, err := ext.ApiextensionsV1().CustomResourceDefinitions().Get( + t.Context(), name, metav1.GetOptions{}) + require.NoError(t, err) + return crd +} + +func waitCRDEstablished(t testing.TB, ext apiextensionsclient.Interface, name string) *apiextensionsv1.CustomResourceDefinition { + t.Helper() + var latest *apiextensionsv1.CustomResourceDefinition + require.NoError(t, wait.For(func(ctx context.Context) (bool, error) { + crd, err := ext.ApiextensionsV1().CustomResourceDefinitions().Get( + ctx, name, metav1.GetOptions{}) + if err != nil { + return false, err + } + latest = crd + for _, cond := range crd.Status.Conditions { + if cond.Type == apiextensionsv1.Established && + cond.Status == apiextensionsv1.ConditionTrue { + return true, nil + } + } + return false, nil + }, wait.WithTimeout(2*time.Minute), wait.WithInterval(time.Second))) + require.NotNil(t, latest) + return latest } func runErr(t testing.TB, args ...string) error { t.Helper() - io, _, _, errOut := nabattest.NewIO() - app := cmd.NewApp(nabat.WithIO(io)) + appIO, _, _, errOut := nabattest.NewIO() + app := cmd.NewApp(nabat.WithIO(appIO)) err := nabattest.Run(t, app, args) if err == nil { return nil diff --git a/internal/e2e/testdata/crd-lifecycle/.deployah/crds/clusterwidget.yaml b/internal/e2e/testdata/crd-lifecycle/.deployah/crds/clusterwidget.yaml new file mode 100644 index 0000000..a068cd9 --- /dev/null +++ b/internal/e2e/testdata/crd-lifecycle/.deployah/crds/clusterwidget.yaml @@ -0,0 +1,24 @@ +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: clusterwidgets.example.com + labels: + e2e-marker: "crd-lifecycle" +spec: + group: example.com + scope: Cluster + names: + kind: ClusterWidget + plural: clusterwidgets + singular: clusterwidget + versions: + - name: v1 + served: true + storage: true + schema: + openAPIV3Schema: + type: object + properties: + spec: + type: object + x-kubernetes-preserve-unknown-fields: true diff --git a/internal/e2e/testdata/crd-lifecycle/deployah.yaml b/internal/e2e/testdata/crd-lifecycle/deployah.yaml new file mode 100644 index 0000000..9609419 --- /dev/null +++ b/internal/e2e/testdata/crd-lifecycle/deployah.yaml @@ -0,0 +1,10 @@ +apiVersion: v1-alpha.4 +project: crd-lifecycle +components: + web: + image: nginx:latest + port: 80 + environments: [dev] + resourcePreset: small +environments: + dev: {} diff --git a/internal/helm/cache_test.go b/internal/helm/cache_test.go index deb7317..122b853 100644 --- a/internal/helm/cache_test.go +++ b/internal/helm/cache_test.go @@ -194,3 +194,49 @@ func TestPrepareChart_CanceledContextReturnsImmediately(t *testing.T) { _, err := PrepareChart(ctx, &spec.Spec{Project: "x"}, "prod", nil, NewChartCache(time.Hour)) require.ErrorIs(t, err, context.Canceled) } + +// TestGenerateKey_ResolvedSpecContentInvalidates verifies that when resolved +// is non-nil, changes to resolved.Spec change the cache key. Callers must +// keep the separate manifest parameter consistent with resolved.Spec (see +// [PrepareChart]); a mismatched manifest would not invalidate the key on +// its own. +func TestGenerateKey_ResolvedSpecContentInvalidates(t *testing.T) { + t.Parallel() + cache := NewChartCache(time.Hour) + + base := &spec.Spec{ + APIVersion: "v1-alpha.4", + Project: "cache-key", + Components: map[string]spec.Component{"web": serviceComponent()}, + } + require.NoError(t, spec.FillSpecWithDefaults(base, "v1-alpha.4")) + + changed := &spec.Spec{ + APIVersion: "v1-alpha.4", + Project: "cache-key", + Components: map[string]spec.Component{ + "web": { + Role: spec.ComponentRoleService, + Image: "my-app:v2", + Port: 8080, + }, + }, + } + require.NoError(t, spec.FillSpecWithDefaults(changed, "v1-alpha.4")) + + resolvedA := &spec.ResolvedSpec{Spec: base, Env: spec.NormalizeEnv("production")} + resolvedB := &spec.ResolvedSpec{Spec: changed, Env: spec.NormalizeEnv("production")} + + keyA, err := cache.GenerateKey(base, "production", resolvedA) + require.NoError(t, err) + keyB, err := cache.GenerateKey(base, "production", resolvedB) + require.NoError(t, err) + assert.NotEqual(t, keyA, keyB, "resolved.Spec content must be part of the cache key") + + // Same resolved, different manifest argument: key is unchanged. This is + // why PrepareChart documents that manifest must match resolved.Spec. + keySameResolved, err := cache.GenerateKey(changed, "production", resolvedA) + require.NoError(t, err) + assert.Equal(t, keyA, keySameResolved, + "GenerateKey hashes resolved when non-nil, not the separate manifest parameter") +} diff --git a/internal/helm/generate.go b/internal/helm/generate.go index 02e8fe4..c94e9e0 100644 --- a/internal/helm/generate.go +++ b/internal/helm/generate.go @@ -88,6 +88,12 @@ func GenerateReleaseName(projectName, environmentName string) string { // PrepareChart returns [context.Canceled] or [context.DeadlineExceeded] // immediately; chart expansion itself is not interrupted mid-flight. // +// When resolved is non-nil, the cache key hashes resolved (including +// [spec.ResolvedSpec.Spec]), not the separate manifest parameter. Callers +// must pass a manifest consistent with resolved.Spec: chart rendering still +// reads component names and project from manifest, so a mismatched pair +// could reuse a stale chart. +// // On a cache miss, every 10th entry may start a background goroutine that // removes expired cache directories; that work outlives this call. // diff --git a/internal/helm/helm.go b/internal/helm/helm.go index e3c4669..2a7a15d 100644 --- a/internal/helm/helm.go +++ b/internal/helm/helm.go @@ -7,6 +7,7 @@ import ( "log/slog" "net" "os" + "slices" "strings" "time" @@ -33,17 +34,17 @@ var ( 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. + // Produced by [Client.wrapHelmError] from typed Helm/Kubernetes errors + // when available, otherwise from Helm's plain "already exists" message. + // Callers may match with [errors.Is]. 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. + // Only [Client.InstallApp] produces this sentinel, via a typed check of the + // newest revision's pending status (Status.IsPending) before upgrade. + // [Client.wrapHelmError] does not classify Helm's plain pending messages, so + // other action paths (and a rare race after the pre-check) surface those as + // generic helm failures. Callers may match with [errors.Is]. ErrReleasePending = errors.New("another operation is in progress") ) @@ -199,10 +200,14 @@ func (c *Client) Namespace() string { } // IsReachable reports whether the configured Kubernetes cluster is reachable. -// Also works around helm/helm#32183: Helm v4.2.0 panics on a second -// IsReachable call after the first one fails (typed-nil cached in -// getKubeClient), so calling this once before InstallApp keeps InstallApp -// from ever hitting that second call against a poisoned client. +// +// Also works around helm/helm#32183: Helm panics on a second IsReachable call +// after the first one fails (typed-nil cached in getKubeClient), so calling +// this once before InstallApp keeps InstallApp from ever hitting that second +// call against a poisoned client. Upstream merged the fix in +// https://github.com/helm/helm/pull/32184 (2026-06-18), but helm.sh/helm/v4 +// v4.2.3 still ships the buggy getKubeClient. Re-check getKubeClient on the +// next Helm bump; the pre-call can be removed once the pin includes the fix. func (c *Client) IsReachable() error { if err := c.config.KubeClient.IsReachable(); err != nil { return fmt.Errorf("%w: %w", ErrClusterUnreachable, err) @@ -231,6 +236,25 @@ func (c *Client) InstallApp(ctx context.Context, manifest *spec.Spec, environmen "deployah.dev/version": manifest.APIVersion, } + releaseName := GenerateReleaseName(manifest.Project, environment) + + // Decide install vs upgrade (and reject pending) before preparing the + // chart so a stuck pending release fails without chart work. + // History.Max is ignored by Helm (see [Client.GetReleaseHistory]), so + // sort by Version after a successful lookup rather than trusting order. + history := action.NewHistory(c.config) + histRels, histErr := history.Run(releaseName) + upgradeExisting := histErr == nil + if upgradeExisting { + rels, convErr := releaserListToV1(histRels) + if convErr != nil { + return fmt.Errorf("failed to convert release history: %w", convErr) + } + if newest := newestRelease(rels); newest != nil && newest.Info != nil && newest.Info.Status.IsPending() { + return fmt.Errorf("release '%s': %w", releaseName, ErrReleasePending) + } + } + chartPath, err := PrepareChart(ctx, manifest, environment, resolved, c.chartCache) if err != nil { return fmt.Errorf("failed to prepare chart: %w", err) @@ -245,7 +269,8 @@ func (c *Client) InstallApp(ctx context.Context, manifest *spec.Spec, environmen }() } - // Values are empty for now, but will be populated later + // Values stay empty: the chart's own values.yaml, written by + // [PrepareChart], already carries the mapped spec data. values := map[string]any{} ch, err := loader.Load(chartPath) @@ -253,15 +278,11 @@ func (c *Client) InstallApp(ctx context.Context, manifest *spec.Spec, environmen return fmt.Errorf("failed to load chart: %w", err) } - // Decide install vs upgrade by checking release history - history := action.NewHistory(c.config) - history.Max = 1 - _, histErr := history.Run(GenerateReleaseName(manifest.Project, environment)) - - if histErr != nil { - // Not found -> install. For other errors, proceed with install attempt as well + if !upgradeExisting { + // Not found -> install. For other history errors, proceed with install + // attempt as well. install := action.NewInstall(c.config) - install.ReleaseName = GenerateReleaseName(manifest.Project, environment) + install.ReleaseName = releaseName install.Namespace = c.settings.Namespace() install.CreateNamespace = true install.Timeout = c.timeout @@ -271,7 +292,7 @@ func (c *Client) InstallApp(ctx context.Context, manifest *spec.Spec, environmen install.PostRenderer = postRenderer if _, runErr := install.RunWithContext(ctx, ch, values); runErr != nil { - return c.wrapHelmError("install", GenerateReleaseName(manifest.Project, environment), runErr) + return c.wrapHelmError("install", releaseName, runErr) } return nil } @@ -284,13 +305,36 @@ func (c *Client) InstallApp(ctx context.Context, manifest *spec.Spec, environmen upgrade.WaitStrategy = kube.StatusWatcherStrategy upgrade.Labels = labels upgrade.PostRenderer = postRenderer - _, err = upgrade.RunWithContext(ctx, GenerateReleaseName(manifest.Project, environment), ch, values) + _, err = upgrade.RunWithContext(ctx, releaseName, ch, values) if err != nil { - return c.wrapHelmError("upgrade", GenerateReleaseName(manifest.Project, environment), err) + return c.wrapHelmError("upgrade", releaseName, err) } return nil } +// newestRelease returns the release with the highest Version, or nil when +// releases is empty. Helm does not guarantee history list order, so callers +// must sort rather than take index 0. +func newestRelease(releases []*v1.Release) *v1.Release { + if len(releases) == 0 { + return nil + } + sorted := slices.Clone(releases) + slices.SortFunc(sorted, func(a, b *v1.Release) int { + if a == nil && b == nil { + return 0 + } + if a == nil { + return 1 + } + if b == nil { + return -1 + } + return b.Version - a.Version + }) + return sorted[0] +} + // ListReleases returns release details in the current namespace. func (c *Client) ListReleases(ctx context.Context, selector labels.Selector) ([]*v1.Release, error) { req, err := labels.NewRequirement("deployah.dev/managed-by", selection.Equals, []string{"deployah"}) @@ -435,8 +479,9 @@ func releaserListToV1(rs []release.Releaser) ([]*v1.Release, error) { // 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). +// matching remains for Helm messages that still lack stable sentinels +// (not-found, already-exists, connection failures). Pending releases are +// rejected earlier by [Client.InstallApp] via Status.IsPending, not here. 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) @@ -461,14 +506,13 @@ func (c *Client) wrapHelmError(operation, releaseName string, err error) error { } // 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. + // Timeout/forbidden/unauthorized string arms are omitted because the + // typed checks above cover those Kubernetes cases. Pending is omitted + // because InstallApp rejects it with a typed Status.IsPending check. 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, "connection refused") || strings.Contains(errMsg, "dial"): return fmt.Errorf("unable to connect to Kubernetes cluster: %w", err) case strings.Contains(errMsg, "already exists"): diff --git a/internal/helm/helm_error_test.go b/internal/helm/helm_error_test.go index 37f7c3a..725c235 100644 --- a/internal/helm/helm_error_test.go +++ b/internal/helm/helm_error_test.go @@ -17,14 +17,26 @@ package helm import ( "errors" "fmt" + "io" "net" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "helm.sh/helm/v4/pkg/action" + "helm.sh/helm/v4/pkg/cli" + "helm.sh/helm/v4/pkg/release/common" + "helm.sh/helm/v4/pkg/storage" "helm.sh/helm/v4/pkg/storage/driver" "k8s.io/apimachinery/pkg/runtime/schema" + "deployah.dev/deployah/internal/spec" + + chartcommon "helm.sh/helm/v4/pkg/chart/common" + chart "helm.sh/helm/v4/pkg/chart/v2" + kubefake "helm.sh/helm/v4/pkg/kube/fake" + v1 "helm.sh/helm/v4/pkg/release/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" ) @@ -92,9 +104,26 @@ func TestWrapHelmError_TypedClassification(t *testing.T) { wantMsg: "unable to connect to Kubernetes cluster", }, { - name: "helm pending string", - err: errors.New("another operation (install/upgrade/rollback) is in progress"), - wantIs: ErrReleasePending, + name: "helm not found string", + err: errors.New("release: not found"), + wantIs: ErrReleaseNotFound, + }, + { + name: "helm connection refused string", + err: errors.New("connection refused"), + wantMsg: "unable to connect to Kubernetes cluster", + }, + { + name: "helm already exists string", + err: errors.New("cannot re-use a name that is still in use: already exists"), + wantIs: ErrReleaseAlreadyExists, + }, + { + // Pending is handled by InstallApp's typed Status.IsPending + // pre-check, not by string matching in wrapHelmError. + name: "helm pending string falls through", + err: errors.New("another operation (install/upgrade/rollback) is in progress"), + wantMsg: "helm upgrade failed", }, } @@ -128,3 +157,116 @@ func TestWrapHelmError_PreservesWrappedCause(t *testing.T) { assert.ErrorIs(t, got, cause) assert.ErrorContains(t, got, "insufficient permissions") } + +// TestNewestRelease_SortsByVersion verifies newestRelease does not trust +// Helm's unsorted history list order. +func TestNewestRelease_SortsByVersion(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in []*v1.Release + want int + }{ + {name: "empty", in: nil, want: 0}, + { + name: "unsorted", + in: []*v1.Release{ + {Version: 1}, + {Version: 3}, + {Version: 2}, + }, + want: 3, + }, + { + name: "already newest first", + in: []*v1.Release{ + {Version: 5}, + {Version: 1}, + }, + want: 5, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := newestRelease(tt.in) + if tt.want == 0 { + assert.Nil(t, got) + return + } + require.NotNil(t, got) + assert.Equal(t, tt.want, got.Version) + }) + } +} + +// TestInstallApp_PendingReleaseRejects pins the typed pending pre-check: a +// release whose newest revision is pending must fail with +// [ErrReleasePending] before Helm upgrade runs. +func TestInstallApp_PendingReleaseRejects(t *testing.T) { + t.Parallel() + + cfg := &action.Configuration{ + Releases: storage.Init(driver.NewMemory()), + KubeClient: &kubefake.PrintingKubeClient{Out: io.Discard}, + } + settings := cli.New() + settings.SetNamespace("default") + + c := &Client{ + config: cfg, + settings: settings, + timeout: time.Minute, + chartCache: NewChartCache(time.Hour), + } + + manifest := &spec.Spec{ + APIVersion: "v1-alpha.4", + Project: "pending-app", + Components: map[string]spec.Component{"web": serviceComponent()}, + } + require.NoError(t, spec.FillSpecWithDefaults(manifest, "v1-alpha.4")) + releaseName := GenerateReleaseName(manifest.Project, "production") + + now := time.Now() + ch := &chart.Chart{ + Metadata: &chart.Metadata{ + APIVersion: "v2", + Name: "hello", + Version: "0.1.0", + }, + Templates: []*chartcommon.File{ + {Name: "templates/hello", Data: []byte("hello: world")}, + }, + } + require.NoError(t, cfg.Releases.Create(&v1.Release{ + Name: releaseName, + Info: &v1.Info{ + FirstDeployed: now, + LastDeployed: now, + Status: common.StatusDeployed, + Description: "deployed", + }, + Chart: ch, + Version: 1, + Namespace: "default", + })) + require.NoError(t, cfg.Releases.Create(&v1.Release{ + Name: releaseName, + Info: &v1.Info{ + FirstDeployed: now, + LastDeployed: now, + Status: common.StatusPendingUpgrade, + Description: "preparing upgrade", + }, + Chart: ch, + Version: 2, + Namespace: "default", + })) + + err := c.InstallApp(t.Context(), manifest, "production", false, nil, nil) + require.Error(t, err) + assert.ErrorIs(t, err, ErrReleasePending) +} diff --git a/internal/session/session.go b/internal/session/session.go index 6930856..2025d28 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -436,6 +436,14 @@ func (cl *Cluster) Namespace() string { return DefaultNamespace } +// sessionForContext returns a shallow Session copy targeted at this +// cluster's resolved kube context, with cached clients cleared. Used by +// Helm, Kubernetes, and RESTConfig so the three paths share one clone +// helper. +func (cl *Cluster) sessionForContext() *Session { + return cl.cloneWithContext(cl.kubeContext) +} + // Helm returns a memoized Helm client targeted at the resolved cluster. func (cl *Cluster) Helm() (HelmClient, error) { cl.mu.Lock() @@ -443,7 +451,7 @@ func (cl *Cluster) Helm() (HelmClient, error) { if cl.helm != nil { return cl.helm, nil } - tmp := cl.cloneWithContext(cl.kubeContext) + tmp := cl.sessionForContext() c, err := tmp.helmFactory(tmp) if err != nil { return nil, fmt.Errorf("helm client (namespace=%q, kubeconfig=%q): %w", @@ -461,7 +469,7 @@ func (cl *Cluster) Kubernetes() (kubernetes.Interface, error) { if cl.k8s != nil { return cl.k8s, nil } - tmp := cl.cloneWithContext(cl.kubeContext) + tmp := cl.sessionForContext() cs, err := tmp.k8sFactory(tmp) if err != nil { return nil, fmt.Errorf("kubernetes client: %w", err) @@ -474,7 +482,7 @@ func (cl *Cluster) Kubernetes() (kubernetes.Interface, error) { func (cl *Cluster) RESTConfig() (*rest.Config, error) { cfg, err := rest.InClusterConfig() if err != nil { - tmp := cl.cloneWithContext(cl.kubeContext) + tmp := cl.sessionForContext() cfg, err = tmp.kubeconfigRESTConfig() if err != nil { return nil, fmt.Errorf("failed to build kubernetes config: %w (provide --kubeconfig or ensure KUBECONFIG/~/.kube/config is set)", err)