From 9d94a1aab058e6983ba0b7fb73fb03b58292bb2a Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Fri, 7 Aug 2026 15:37:41 +0300 Subject: [PATCH 01/13] fix(cluster): scope cleanup to image pruning only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 'cluster cleanup' promised to free disk space but actually tore the whole platform down: it deleted every ArgoCD Application, uninstalled every Helm release in every namespace (including non-OpenFrame ones), force-stripped finalizers from stuck Applications, and deleted the argocd/openframe namespaces — all behind a confirmation prompt that never mentioned any of it. That is how a routine cleanup destroyed a working install (17 apps) on demo-local-1. Cleanup now does exactly what its help text says: prune unused container images inside each k3d node. Platform teardown remains where it already lives, honestly named and properly scoped: 'app uninstall' (removes only the app-of-apps and argo-cd releases, asks an explicit question) and 'cluster delete'. - drop the Application-delete / Helm-uninstall / finalizer-strip / namespace phases and their helpers from the cluster service, along with the ApplicationCleaner injection in the cleanup command - reword the confirmation prompt to state what cleanup does ('Prune unused container images... Installed apps are not touched') - fix the help text and the --force description (no more 'aggressive cleanup'), update docs - delete the tests of the removed phases; rescope the summary/result tests --- cmd/cluster/cleanup.go | 30 +-- docs/architecture/decisions.md | 3 +- docs/getting-started/first-steps.md | 2 +- internal/cluster/cleanup_finalizers_test.go | 131 ---------- internal/cluster/cleanup_helm_test.go | 163 ------------ internal/cluster/cleanup_result_test.go | 65 ----- internal/cluster/cleanup_safety_test.go | 35 --- internal/cluster/models/cleanup.go | 20 +- internal/cluster/models/flags.go | 2 +- internal/cluster/service.go | 272 +------------------- internal/cluster/service_test.go | 2 +- internal/cluster/ui/cleanup_summary_test.go | 36 +-- internal/cluster/ui/operations.go | 22 +- 13 files changed, 51 insertions(+), 732 deletions(-) delete mode 100644 internal/cluster/cleanup_finalizers_test.go delete mode 100644 internal/cluster/cleanup_helm_test.go delete mode 100644 internal/cluster/cleanup_result_test.go delete mode 100644 internal/cluster/cleanup_safety_test.go diff --git a/cmd/cluster/cleanup.go b/cmd/cluster/cleanup.go index d7c637cc..fe884233 100644 --- a/cmd/cluster/cleanup.go +++ b/cmd/cluster/cleanup.go @@ -3,13 +3,10 @@ package cluster import ( "fmt" - "github.com/flamingo-stack/openframe-cli/internal/chart/providers/argocd" "github.com/flamingo-stack/openframe-cli/internal/cluster/models" "github.com/flamingo-stack/openframe-cli/internal/cluster/prerequisites" "github.com/flamingo-stack/openframe-cli/internal/cluster/ui" "github.com/flamingo-stack/openframe-cli/internal/cluster/utils" - "github.com/flamingo-stack/openframe-cli/internal/shared/executor" - "github.com/pterm/pterm" "github.com/spf13/cobra" ) @@ -19,11 +16,13 @@ func getCleanupCmd() *cobra.Command { cleanupCmd := &cobra.Command{ Use: "cleanup [NAME]", - Short: "Clean up unused cluster resources", - Long: `Remove unused images and resources from cluster nodes. + Short: "Prune unused container images from cluster nodes", + Long: `Reclaim disk space by pruning unused container images inside each cluster node. -Cleans up Docker images and resources, freeing disk space. -Useful for development clusters with many builds. +Only images no container references are removed. Installed applications, Helm +releases and namespaces are never touched — to remove the OpenFrame platform +use 'openframe app uninstall', to remove the whole cluster use +'openframe cluster delete'. Examples: openframe cluster cleanup @@ -90,24 +89,9 @@ func runCleanupCluster(cmd *cobra.Command, args []string) error { } } - // Inject the ArgoCD-backed application cleaner (composition root: only the - // command layer may import both the cluster and the chart subsystems). - // Without it, cleanup skips the Application delete/finalizer-strip phases and - // the argocd namespace can stay stuck in Terminating. Best-effort: a cluster - // that is unreachable or has no ArgoCD simply cleans up without it. - if cfg, cerr := service.GetRestConfig(clusterName); cerr == nil { - if mgr, merr := argocd.NewManagerWithConfig(executor.NewRealCommandExecutor(false, globalFlags.Global.Verbose), cfg); merr == nil { - service = service.WithApplicationCleaner(mgr) - } else if globalFlags.Global.Verbose { - pterm.Warning.Printf("ArgoCD cleanup unavailable: %v\n", merr) - } - } else if globalFlags.Global.Verbose { - pterm.Warning.Printf("Cluster not reachable for ArgoCD cleanup: %v\n", cerr) - } - // Execute cluster cleanup through service layer. A nil error with failed // phases is a partial cleanup: the summary names what was left behind. - result, err := service.CleanupCluster(cmd.Context(), clusterName, clusterType, utils.GetGlobalFlags().Global.Verbose, utils.GetGlobalFlags().Cleanup.Force) + result, err := service.CleanupCluster(cmd.Context(), clusterName, clusterType, utils.GetGlobalFlags().Global.Verbose) if err != nil { operationsUI.ShowOperationError("cleanup", clusterName, err) return err diff --git a/docs/architecture/decisions.md b/docs/architecture/decisions.md index 7e417584..1c7c7a00 100644 --- a/docs/architecture/decisions.md +++ b/docs/architecture/decisions.md @@ -52,7 +52,8 @@ The cluster is always a local k3d cluster. - `openframe cluster create|delete|list|status|cleanup` — cluster lifecycle. `create` **only creates the cluster**; it never installs the app. (Verb is - `create`; there is no `apply`.) `cleanup` removes unused cluster resources. + `create`; there is no `apply`.) `cleanup` only prunes unused container images + on the nodes; removing the platform is `app uninstall`'s job. - `openframe app install|upgrade|status|access|uninstall` — installs and operates the OpenFrame app on an existing, online cluster. `upgrade` re-deploys the app-of-apps at a new git ref (`--ref`) or forces an ArgoCD hard refresh + sync diff --git a/docs/getting-started/first-steps.md b/docs/getting-started/first-steps.md index ebbab2b7..00b41ee5 100644 --- a/docs/getting-started/first-steps.md +++ b/docs/getting-started/first-steps.md @@ -66,7 +66,7 @@ openframe cluster create my-second-cluster # Delete a cluster openframe cluster delete my-second-cluster -# Clean up leftover resources from a failed cluster +# Reclaim disk space by pruning unused container images on cluster nodes openframe cluster cleanup ``` diff --git a/internal/cluster/cleanup_finalizers_test.go b/internal/cluster/cleanup_finalizers_test.go deleted file mode 100644 index ca4d110f..00000000 --- a/internal/cluster/cleanup_finalizers_test.go +++ /dev/null @@ -1,131 +0,0 @@ -package cluster - -import ( - "context" - "fmt" - "testing" - "time" - - "github.com/flamingo-stack/openframe-cli/internal/cluster/models" - "github.com/flamingo-stack/openframe-cli/internal/shared/executor" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// recordingCleaner records the order in which the cleanup phases call it, -// appending to a shared trace so it can be interleaved with the helm calls. -type recordingCleaner struct { - trace *[]string - deleteErr error - clearErr error - deleted int - cleared int - deleteCall int - clearCall int -} - -func (r *recordingCleaner) DeleteApplications(context.Context) (int, error) { - r.deleteCall++ - *r.trace = append(*r.trace, "delete-applications") - return r.deleted, r.deleteErr -} - -func (r *recordingCleaner) RemoveApplicationFinalizers(context.Context) (int, error) { - r.clearCall++ - *r.trace = append(*r.trace, "clear-finalizers") - return r.cleared, r.clearErr -} - -// tracingExecutor records helm uninstalls into the same trace as the cleaner. -type tracingExecutor struct { - *executor.MockCommandExecutor - trace *[]string -} - -func (t *tracingExecutor) Execute(ctx context.Context, name string, args ...string) (*executor.CommandResult, error) { - if name == "helm" && len(args) > 0 && args[0] == "uninstall" { - *t.trace = append(*t.trace, "helm-uninstall") - } - return t.MockCommandExecutor.Execute(ctx, name, args...) -} - -func newTracingExecutor(trace *[]string) *tracingExecutor { - mock := executor.NewMockCommandExecutor() - mock.SetResponse("helm list", &executor.CommandResult{ - ExitCode: 0, - Stdout: `[{"name":"argo-cd","namespace":"argocd"}]`, - Duration: time.Millisecond, - }) - return &tracingExecutor{MockCommandExecutor: mock, trace: trace} -} - -// TestCleanup_ApplicationPhasesBracketTheHelmUninstall locks the ordering the -// whole fix depends on: ArgoCD Applications are deleted while the controller -// still runs (so it cascades workload cleanup), and their resources-finalizer -// is stripped only AFTER the helm uninstall removed the controller — nothing -// else can clear it, so a Terminating CR would otherwise pin the namespace. -func TestCleanup_ApplicationPhasesBracketTheHelmUninstall(t *testing.T) { - var trace []string - exec := newTracingExecutor(&trace) - cleaner := &recordingCleaner{trace: &trace, deleted: 3, cleared: 2} - - service := NewClusterService(exec).WithApplicationCleaner(cleaner) - _, _ = service.CleanupCluster(context.Background(), "test-cluster", models.ClusterTypeK3d, false, false) - - require.GreaterOrEqual(t, len(trace), 3, "trace: %v", trace) - assert.Equal(t, "delete-applications", trace[0], "applications must be deleted first: %v", trace) - - helmIdx, clearIdx := -1, -1 - for i, step := range trace { - switch step { - case "helm-uninstall": - if helmIdx == -1 { - helmIdx = i - } - case "clear-finalizers": - clearIdx = i - } - } - require.NotEqual(t, -1, helmIdx, "helm uninstall must run: %v", trace) - require.NotEqual(t, -1, clearIdx, "finalizers must be cleared: %v", trace) - assert.Greater(t, clearIdx, helmIdx, - "finalizers must be stripped AFTER the ArgoCD controller is uninstalled: %v", trace) - - assert.Equal(t, 1, cleaner.deleteCall) - assert.Equal(t, 1, cleaner.clearCall) -} - -// TestCleanup_WithoutCleanerStillRuns: the cleaner is optional — a cluster -// without OpenFrame (or an unreachable one) must still get the helm/namespace/ -// docker phases rather than failing. -func TestCleanup_WithoutCleanerStillRuns(t *testing.T) { - var trace []string - exec := newTracingExecutor(&trace) - - service := NewClusterService(exec) // no cleaner injected - _, err := service.CleanupCluster(context.Background(), "test-cluster", models.ClusterTypeK3d, false, false) - require.NoError(t, err) - assert.Contains(t, trace, "helm-uninstall", "helm phase must still run: %v", trace) - assert.NotContains(t, trace, "delete-applications") - assert.NotContains(t, trace, "clear-finalizers") -} - -// TestCleanup_CleanerErrorsAreNonFatal: the platform-cleanup phases are -// best-effort — a cluster where ArgoCD was never installed (or the API errors) -// must not abort the rest of the cleanup. -func TestCleanup_CleanerErrorsAreNonFatal(t *testing.T) { - var trace []string - exec := newTracingExecutor(&trace) - cleaner := &recordingCleaner{ - trace: &trace, - deleteErr: fmt.Errorf("no argocd CRD"), - clearErr: fmt.Errorf("no argocd CRD"), - } - - service := NewClusterService(exec).WithApplicationCleaner(cleaner) - _, err := service.CleanupCluster(context.Background(), "test-cluster", models.ClusterTypeK3d, false, false) - require.NoError(t, err, - "cleaner failures must not fail the cleanup") - assert.Contains(t, trace, "helm-uninstall", "helm phase must still run after a cleaner error: %v", trace) - assert.Equal(t, 1, cleaner.clearCall, "the finalizer phase must run even if the delete phase failed") -} diff --git a/internal/cluster/cleanup_helm_test.go b/internal/cluster/cleanup_helm_test.go deleted file mode 100644 index 1f0a84ab..00000000 --- a/internal/cluster/cleanup_helm_test.go +++ /dev/null @@ -1,163 +0,0 @@ -package cluster - -import ( - "context" - "testing" - "time" - - "github.com/flamingo-stack/openframe-cli/internal/cluster/models" - "github.com/flamingo-stack/openframe-cli/internal/shared/executor" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// helmArgvsOf returns the argv of every helm invocation recorded by the mock. -func helmArgvsOf(mock *executor.MockCommandExecutor) [][]string { - var out [][]string - for _, rc := range mock.Commands() { - if rc.Name == "helm" { - out = append(out, rc.Args) - } - } - return out -} - -// hasFlagValue reports whether argv contains the flag immediately followed by value. -func hasFlagValue(argv []string, flag, value string) bool { - for i := 0; i+1 < len(argv); i++ { - if argv[i] == flag && argv[i+1] == value { - return true - } - } - return false -} - -// TestCleanupHelmReleases_PinsKubeContext is the T0-1 regression guard: every -// helm call issued by cleanup must carry --kube-context for the cluster being -// cleaned. Without the pin, helm acts on the kubeconfig's CURRENT context — -// switching context to a production cluster and running `cluster cleanup` -// would uninstall every release there. -func TestCleanupHelmReleases_PinsKubeContext(t *testing.T) { - mock := executor.NewMockCommandExecutor() - // Real `helm list --output json` emits a single-line JSON array. - mock.SetResponse("helm list", &executor.CommandResult{ - ExitCode: 0, - Stdout: `[{"name":"argo-cd","namespace":"argocd","status":"deployed"},{"name":"openframe","namespace":"openframe","status":"deployed"}]`, - Duration: time.Millisecond, - }) - service := NewClusterService(mock) - - _, err := service.cleanupHelmReleases(context.Background(), "k3d-test-cluster", false, false) - require.NoError(t, err) - - argvs := helmArgvsOf(mock) - require.NotEmpty(t, argvs, "cleanup must invoke helm") - for _, argv := range argvs { - assert.Truef(t, hasFlagValue(argv, "--kube-context", "k3d-test-cluster"), - "every helm call must pin --kube-context k3d-test-cluster, got: %v", argv) - } - - // Both releases are uninstalled, each in its own namespace. - var uninstalls [][]string - for _, argv := range argvs { - if len(argv) > 0 && argv[0] == "uninstall" { - uninstalls = append(uninstalls, argv) - } - } - require.Len(t, uninstalls, 2, "one uninstall per listed release") - assert.Equal(t, "argo-cd", uninstalls[0][1]) - assert.True(t, hasFlagValue(uninstalls[0], "--namespace", "argocd")) - assert.Equal(t, "openframe", uninstalls[1][1]) - assert.True(t, hasFlagValue(uninstalls[1], "--namespace", "openframe")) - - // No --wait, ever: app-of-apps Application CRs carry ArgoCD's - // resources-finalizer, and with the controller itself being uninstalled - // --wait would block for helm's default 5m per release. - for _, argv := range uninstalls { - assert.NotContainsf(t, argv, "--wait", "cleanup uninstall must be fire-and-forget: %v", argv) - } -} - -// TestCleanupHelmReleases_ForceAddsIgnoreNotFound locks the force-mode flag. -func TestCleanupHelmReleases_ForceAddsIgnoreNotFound(t *testing.T) { - mock := executor.NewMockCommandExecutor() - mock.SetResponse("helm list", &executor.CommandResult{ - ExitCode: 0, - Stdout: `[{"name":"argo-cd","namespace":"argocd"}]`, - Duration: time.Millisecond, - }) - service := NewClusterService(mock) - - _, err := service.cleanupHelmReleases(context.Background(), "k3d-x", false, true) - require.NoError(t, err) - - found := false - for _, argv := range helmArgvsOf(mock) { - if len(argv) > 0 && argv[0] == "uninstall" { - found = true - assert.Contains(t, argv, "--ignore-not-found") - } - } - assert.True(t, found, "expected an uninstall call") -} - -// TestCleanupHelmReleases_EmptyList: nothing to uninstall on "[]" or empty output. -func TestCleanupHelmReleases_EmptyList(t *testing.T) { - for name, stdout := range map[string]string{"empty-array": "[]", "blank": ""} { - t.Run(name, func(t *testing.T) { - mock := executor.NewMockCommandExecutor() - mock.SetResponse("helm list", &executor.CommandResult{ExitCode: 0, Stdout: stdout, Duration: time.Millisecond}) - service := NewClusterService(mock) - - _, err := service.cleanupHelmReleases(context.Background(), "k3d-x", false, false) - require.NoError(t, err) - for _, argv := range helmArgvsOf(mock) { - assert.NotEqual(t, "uninstall", argv[0], "no uninstall may run for an empty release list") - } - }) - } -} - -// TestCleanupHelmReleases_RefusesWithoutContext: a missing kube-context must be -// a hard error, never a fall-through to the current context. -func TestCleanupHelmReleases_RefusesWithoutContext(t *testing.T) { - mock := executor.NewMockCommandExecutor() - service := NewClusterService(mock) - - _, err := service.cleanupHelmReleases(context.Background(), "", false, false) - require.Error(t, err) - assert.Zero(t, mock.GetCommandCount(), "no command may run without an explicit kube-context") -} - -// TestCleanupHelmReleases_GarbageOutputErrors: unparseable helm output must -// surface as an error instead of being half-parsed (the old code split the -// JSON on ":" and produced garbage namespaces). -func TestCleanupHelmReleases_GarbageOutputErrors(t *testing.T) { - mock := executor.NewMockCommandExecutor() - mock.SetResponse("helm list", &executor.CommandResult{ExitCode: 0, Stdout: "not json", Duration: time.Millisecond}) - service := NewClusterService(mock) - - _, err := service.cleanupHelmReleases(context.Background(), "k3d-x", false, false) - require.Error(t, err) - for _, argv := range helmArgvsOf(mock) { - assert.NotEqual(t, "uninstall", argv[0], "no uninstall may run on unparseable output") - } -} - -// TestCleanupCluster_HelmPhasePinsKubeContext exercises the full CleanupCluster -// entry point: whatever context resolution yields, the helm phase must never -// issue a helm call without --kube-context. -func TestCleanupCluster_HelmPhasePinsKubeContext(t *testing.T) { - mock := executor.NewMockCommandExecutor() - mock.SetResponse("helm list", &executor.CommandResult{ExitCode: 0, Stdout: "[]", Duration: time.Millisecond}) - service := NewClusterService(mock) - - // K8s/Docker phases run against the mock too and are allowed to no-op/fail. - _, _ = service.CleanupCluster(context.Background(), "test-cluster", models.ClusterTypeK3d, false, false) - - argvs := helmArgvsOf(mock) - require.NotEmpty(t, argvs, "cleanup must reach the helm phase") - for _, argv := range argvs { - assert.Containsf(t, argv, "--kube-context", "helm call without --kube-context: %v", argv) - } -} diff --git a/internal/cluster/cleanup_result_test.go b/internal/cluster/cleanup_result_test.go deleted file mode 100644 index c86b3871..00000000 --- a/internal/cluster/cleanup_result_test.go +++ /dev/null @@ -1,65 +0,0 @@ -package cluster - -import ( - "context" - "testing" - "time" - - "github.com/flamingo-stack/openframe-cli/internal/shared/executor" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// TestCleanupHelmReleases_CountsAndReportsPartialFailure (M2.1): a release that -// fails to uninstall must NOT be counted as removed, and the phase must report -// the failure. Cleanup used to return nil unconditionally, so the summary -// printed "Freed up disk space" whether or not anything was freed. -func TestCleanupHelmReleases_CountsAndReportsPartialFailure(t *testing.T) { - mock := executor.NewMockCommandExecutor() - mock.SetResponse("helm list", &executor.CommandResult{ - ExitCode: 0, - Stdout: `[{"name":"argo-cd","namespace":"argocd"},{"name":"openframe","namespace":"openframe"}]`, - Duration: time.Millisecond, - }) - // One of the two uninstalls fails. - mock.SetResponse("helm uninstall openframe", &executor.CommandResult{ - ExitCode: 1, - Stderr: "release: not found", - Duration: time.Millisecond, - }) - service := NewClusterService(mock) - - removed, err := service.cleanupHelmReleases(context.Background(), "k3d-test", false, false) - - assert.Equal(t, 1, removed, "only the release that actually uninstalled may be counted") - require.Error(t, err, "a failed uninstall must be reported, not swallowed") - assert.Contains(t, err.Error(), "openframe", "the failure must name the release that survived") -} - -// TestCleanupHelmReleases_CountsCleanRun is the control: nothing failed, so the -// count matches and no error is reported. -func TestCleanupHelmReleases_CountsCleanRun(t *testing.T) { - mock := executor.NewMockCommandExecutor() - mock.SetResponse("helm list", &executor.CommandResult{ - ExitCode: 0, - Stdout: `[{"name":"argo-cd","namespace":"argocd"},{"name":"openframe","namespace":"openframe"}]`, - Duration: time.Millisecond, - }) - service := NewClusterService(mock) - - removed, err := service.cleanupHelmReleases(context.Background(), "k3d-test", false, false) - require.NoError(t, err) - assert.Equal(t, 2, removed) -} - -// TestCleanupHelmReleases_EmptyClusterRemovesNothing: an empty cluster must -// report zero removals rather than an implied success. -func TestCleanupHelmReleases_EmptyClusterRemovesNothing(t *testing.T) { - mock := executor.NewMockCommandExecutor() - mock.SetResponse("helm list", &executor.CommandResult{ExitCode: 0, Stdout: `[]`}) - service := NewClusterService(mock) - - removed, err := service.cleanupHelmReleases(context.Background(), "k3d-test", false, false) - require.NoError(t, err) - assert.Zero(t, removed) -} diff --git a/internal/cluster/cleanup_safety_test.go b/internal/cluster/cleanup_safety_test.go deleted file mode 100644 index 43d8c45f..00000000 --- a/internal/cluster/cleanup_safety_test.go +++ /dev/null @@ -1,35 +0,0 @@ -package cluster - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -// TestFilterProtectedNamespaces_NeverIncludesProtected is the I7 regression -// guard: the cleanup namespace list must never include a protected/system -// namespace, even if one is added to the raw list by mistake. cleanup now -// deletes namespaces via client-go through exactly this filtered list. -func TestFilterProtectedNamespaces_NeverIncludesProtected(t *testing.T) { - // A raw list deliberately tainted with every protected namespace. - raw := []string{"argocd", "kube-system", "openframe", "kube-public", "kube-node-lease", "default", "my-app"} - - got := filterProtectedNamespaces(raw) - - for _, protected := range []string{"kube-system", "kube-public", "kube-node-lease", "default"} { - assert.NotContainsf(t, got, protected, "protected namespace %q must be filtered out", protected) - } - // Non-protected namespaces survive. - for _, ns := range []string{"argocd", "openframe", "my-app"} { - assert.Containsf(t, got, ns, "non-protected namespace %q must survive", ns) - } -} - -func TestIsProtectedNamespace(t *testing.T) { - for _, ns := range []string{"kube-system", "kube-public", "kube-node-lease", "default"} { - assert.Truef(t, isProtectedNamespace(ns), "%s must be protected", ns) - } - for _, ns := range []string{"argocd", "openframe", "my-app"} { - assert.Falsef(t, isProtectedNamespace(ns), "%s must not be protected", ns) - } -} diff --git a/internal/cluster/models/cleanup.go b/internal/cluster/models/cleanup.go index eb60c079..7613fcbf 100644 --- a/internal/cluster/models/cleanup.go +++ b/internal/cluster/models/cleanup.go @@ -6,18 +6,13 @@ import "fmt" // report facts instead of a fixed script. // // Cleanup is best-effort by design: every phase swallows its own error so that -// a half-installed or partly-unreachable cluster can still be torn down. The -// old code paired that with a summary that unconditionally printed "Removed -// unused Docker images / Freed up disk space / Optimized cluster performance", -// so a run in which every phase failed was indistinguishable from a clean one. -// Counting the work and collecting the failures is what makes the best-effort -// contract honest. +// a partly-unreachable cluster can still be pruned. The old code paired that +// with a summary that unconditionally printed "Removed unused Docker images / +// Freed up disk space / Optimized cluster performance", so a run in which +// every phase failed was indistinguishable from a clean one. Counting the work +// and collecting the failures is what makes the best-effort contract honest. type CleanupResult struct { - ApplicationsDeleted int - FinalizersCleared int - ReleasesRemoved int - NamespacesDeleted int - NodesPruned int + NodesPruned int // Failures holds one human-readable line per phase that did not complete. // A non-empty Failures with a nil error is the normal "partial cleanup" @@ -32,8 +27,7 @@ func (r *CleanupResult) AddFailure(phase string, err error) { // Removed reports the total number of objects cleanup actually removed. func (r CleanupResult) Removed() int { - return r.ApplicationsDeleted + r.FinalizersCleared + r.ReleasesRemoved + - r.NamespacesDeleted + r.NodesPruned + return r.NodesPruned } // Partial reports whether at least one phase failed. Cleanup still succeeded diff --git a/internal/cluster/models/flags.go b/internal/cluster/models/flags.go index 6cb3e025..c97d578f 100644 --- a/internal/cluster/models/flags.go +++ b/internal/cluster/models/flags.go @@ -104,7 +104,7 @@ func AddDeleteFlags(cmd *cobra.Command, flags *DeleteFlags) { // AddCleanupFlags adds cleanup-specific flags to a command func AddCleanupFlags(cmd *cobra.Command, flags *CleanupFlags) { - cmd.Flags().BoolVarP(&flags.Force, "force", "f", false, "Skip confirmation prompt and enable aggressive cleanup (remove all images, volumes, networks)") + cmd.Flags().BoolVarP(&flags.Force, "force", "f", false, "Skip confirmation prompt") } // ValidateClusterName validates cluster name according to Kubernetes naming conventions diff --git a/internal/cluster/service.go b/internal/cluster/service.go index b90d10e3..db826c28 100644 --- a/internal/cluster/service.go +++ b/internal/cluster/service.go @@ -2,7 +2,6 @@ package cluster import ( "context" - "encoding/json" "fmt" "os" "strconv" @@ -14,13 +13,11 @@ import ( "github.com/flamingo-stack/openframe-cli/internal/cluster/provider" uiCluster "github.com/flamingo-stack/openframe-cli/internal/cluster/ui" "github.com/flamingo-stack/openframe-cli/internal/k8s" - "github.com/flamingo-stack/openframe-cli/internal/platform" "github.com/flamingo-stack/openframe-cli/internal/shared/executor" "github.com/flamingo-stack/openframe-cli/internal/shared/ui" "github.com/flamingo-stack/openframe-cli/internal/shared/ui/spinner" "github.com/pterm/pterm" corev1 "k8s.io/api/core/v1" - k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" @@ -32,40 +29,12 @@ import ( // a decision, not an accident (audit follow-up). const bootstrapNodeCount = 4 -// ApplicationCleaner removes the ArgoCD Application CRs that own the platform -// workloads, and strips the resources-finalizer from any left in Terminating. -// -// Cleanup needs both, in that order around the Helm uninstall: Applications -// must be deleted while the ArgoCD controller still runs (so it cascades the -// workload cleanup), and the finalizers must be stripped afterwards, once the -// controller — the only thing that could clear them — is gone. Otherwise the -// CRs sit in Terminating forever and pin the argocd namespace. -// -// It is an interface because internal/cluster must not import internal/chart: -// the ArgoCD-backed implementation is injected by the command layer, exactly -// like ClusterAccess in the app subsystem. -type ApplicationCleaner interface { - DeleteApplications(ctx context.Context) (int, error) - RemoveApplicationFinalizers(ctx context.Context) (int, error) -} - // ClusterService provides cluster configuration and management operations // This handles cluster lifecycle operations and configuration management type ClusterService struct { manager provider.Provider executor executor.CommandExecutor suppressUI bool // Suppress interactive UI elements for automation - // appCleaner, when set, lets cleanup remove ArgoCD Applications before the - // Helm uninstall and strip their finalizers afterwards. Optional: nil means - // the Helm/namespace phases run as before (the CRs may then stay stuck). - appCleaner ApplicationCleaner -} - -// WithApplicationCleaner injects the ArgoCD-backed application cleaner used by -// the cleanup flow. Returns the service for chaining. -func (s *ClusterService) WithApplicationCleaner(c ApplicationCleaner) *ClusterService { - s.appCleaner = c - return s } // isTerminalEnvironment checks if we're running in a proper terminal @@ -332,12 +301,19 @@ func (s *ClusterService) DetectClusterType(name string) (models.ClusterType, err } // CleanupCluster handles cluster cleanup business logic. The returned -// CleanupResult reports what was actually removed and which phases failed; a +// CleanupResult reports what was actually pruned and which phases failed; a // nil error with a non-empty Failures list is a partial cleanup. -func (s *ClusterService) CleanupCluster(ctx context.Context, name string, clusterType models.ClusterType, verbose bool, force bool) (models.CleanupResult, error) { +// +// Cleanup reclaims disk space and nothing else. It used to also delete every +// ArgoCD Application, every Helm release and the argocd/openframe namespaces — +// a full platform teardown hiding behind a "free disk space" help text, which +// is how a routine cleanup destroyed a working install. Tearing the platform +// down is `app uninstall`'s job; tearing the cluster down is `cluster +// delete`'s. +func (s *ClusterService) CleanupCluster(ctx context.Context, name string, clusterType models.ClusterType, verbose bool) (models.CleanupResult, error) { switch clusterType { case models.ClusterTypeK3d: - return s.cleanupK3dCluster(ctx, name, verbose, force) + return s.cleanupK3dCluster(ctx, name, verbose) case models.ClusterTypeEKS, models.ClusterTypeGKE: return models.CleanupResult{}, fmt.Errorf("cleanup is not supported for cloud clusters; use 'openframe cluster delete %s' to tear the cluster down", name) default: @@ -345,70 +321,15 @@ func (s *ClusterService) CleanupCluster(ctx context.Context, name string, cluste } } -// cleanupK3dCluster handles K3d-specific cleanup. -// -// Every phase is best-effort: a failure is recorded and the next phase still -// runs, because a partly-installed cluster must remain tearable-down. Failures -// are surfaced (not just under --verbose) so "cleanup completed" never hides a -// phase that did nothing. -func (s *ClusterService) cleanupK3dCluster(ctx context.Context, clusterName string, verbose bool, force bool) (models.CleanupResult, error) { +// cleanupK3dCluster reclaims disk on a k3d cluster by pruning unused container +// images inside each node. Failures are surfaced (not just under --verbose) so +// "cleanup completed" never hides a phase that did nothing. +func (s *ClusterService) cleanupK3dCluster(ctx context.Context, clusterName string, verbose bool) (models.CleanupResult, error) { if verbose { pterm.Info.Printf("Starting cleanup of cluster: %s\n", clusterName) } var result models.CleanupResult - // 1. Delete the ArgoCD Applications WHILE the ArgoCD controller is still - // running, so it cascades the workload cleanup itself. Best-effort: a - // cluster without OpenFrame installed simply has none. - if s.appCleaner != nil { - deleted, err := s.appCleaner.DeleteApplications(ctx) - switch { - case err != nil: - result.AddFailure("ArgoCD applications", err) - default: - result.ApplicationsDeleted = deleted - if deleted > 0 && verbose { - pterm.Info.Printf("Deleted %d ArgoCD application(s)\n", deleted) - } - } - } - - // 2. Clean up Helm releases (including ArgoCD) — pinned to this cluster's - // kube-context. Without the pin helm operates on the kubeconfig's CURRENT - // context, which may be a different (even production) cluster. - kubeContext := k8s.ResolveContextForCluster(k8s.DefaultKubeconfigPath(), clusterName) - removed, err := s.cleanupHelmReleases(ctx, kubeContext, verbose, force) - result.ReleasesRemoved = removed - if err != nil { - result.AddFailure("Helm releases", err) - } - - // 3. ArgoCD is gone now, so nothing is left to clear its resources-finalizer. - // Strip it from any Application still in Terminating — otherwise those CRs - // (and the argocd namespace deleted in the next phase) never get reaped. - if s.appCleaner != nil { - cleared, err := s.appCleaner.RemoveApplicationFinalizers(ctx) - switch { - case err != nil: - result.AddFailure("application finalizers", err) - default: - result.FinalizersCleared = cleared - if cleared > 0 && verbose { - pterm.Info.Printf("Cleared finalizers on %d stuck application(s)\n", cleared) - } - } - } - - // 4. Clean up Kubernetes resources in common namespaces - deletedNS, err := s.cleanupKubernetesResources(ctx, clusterName, verbose, force) - result.NamespacesDeleted = deletedNS - if err != nil { - result.AddFailure("Kubernetes namespaces", err) - } - - // 5. Reclaim disk by pruning unused container images inside each node. - // Not gated on force: removing images no container references is safe, and - // reclaiming disk is the whole point of `cluster cleanup`. pruned, err := s.cleanupNodeImages(ctx, clusterName, verbose) result.NodesPruned = pruned if err != nil { @@ -422,171 +343,6 @@ func (s *ClusterService) cleanupK3dCluster(ctx context.Context, clusterName stri return result, nil } -// helmRelease is the subset of `helm list --output json` we consume. -type helmRelease struct { - Name string `json:"name"` - Namespace string `json:"namespace"` -} - -// cleanupHelmReleases removes all Helm releases from the cluster identified by -// kubeContext. The explicit --kube-context on every helm call is what keeps -// cleanup scoped to that cluster (T0-1): helm otherwise acts on the -// kubeconfig's current context, whatever the user last switched to. -// It returns the number of releases actually uninstalled. A release that fails -// to uninstall is counted as a failure, not as removed. -func (s *ClusterService) cleanupHelmReleases(ctx context.Context, kubeContext string, verbose bool, force bool) (int, error) { - if kubeContext == "" { - return 0, fmt.Errorf("refusing to cleanup Helm releases without an explicit kube-context") - } - - result, err := s.executor.Execute(ctx, "helm", "list", "--all-namespaces", "--output", "json", "--kube-context", kubeContext) - if err != nil { - return 0, fmt.Errorf("failed to list Helm releases: %w", err) - } - - var releases []helmRelease - if out := strings.TrimSpace(result.Stdout); out != "" { - if err := json.Unmarshal([]byte(out), &releases); err != nil { - return 0, fmt.Errorf("failed to parse helm list output: %w", err) - } - } - if len(releases) == 0 { - if verbose { - pterm.Info.Println("No Helm releases found to cleanup") - } - return 0, nil - } - - var removed int - var failed []string - for _, release := range releases { - if release.Name == "" || release.Namespace == "" { - continue - } - - if verbose { - pterm.Info.Printf("Uninstalling Helm release: %s (namespace %s)\n", release.Name, release.Namespace) - } - - // Aggressive uninstall, deliberately WITHOUT --wait: the releases here - // include argo-cd and app-of-apps, whose Application CRs carry ArgoCD's - // resources-finalizer. Once the ArgoCD controller is being removed it - // can no longer clear that finalizer, so --wait would block for helm's - // default 5m PER RELEASE (see UninstallRelease in - // internal/chart/providers/helm for the same rationale). - // - // The Application CRs left in Terminating are reaped by the - // finalizer-stripping phase that runs right after this one (see - // cleanupK3dCluster step 3), mirroring `app uninstall`. - args := []string{"uninstall", release.Name, "--namespace", release.Namespace, "--kube-context", kubeContext, "--no-hooks"} - if force { - // Add even more aggressive flags when force is enabled - args = append(args, "--ignore-not-found") - } - if _, err := s.executor.Execute(ctx, "helm", args...); err != nil { - failed = append(failed, release.Name) - if verbose { - pterm.Warning.Printf("Failed to uninstall release %s: %v\n", release.Name, err) - } - } else { - removed++ - if verbose { - pterm.Success.Printf("Uninstalled Helm release: %s\n", release.Name) - } - } - } - - if len(failed) > 0 { - return removed, fmt.Errorf("%d of %d release(s) could not be uninstalled: %s", - len(failed), len(releases), strings.Join(failed, ", ")) - } - return removed, nil -} - -// protectedNamespaces must never be deleted by cleanup, regardless of --force. -// Deleting any of these can render the cluster unrecoverable or destroy -// unrelated workloads (audit I7/M3). -var protectedNamespaces = map[string]struct{}{ - "kube-system": {}, - "kube-public": {}, - "kube-node-lease": {}, - "default": {}, -} - -// isProtectedNamespace reports whether ns must never be deleted. -func isProtectedNamespace(ns string) bool { - _, ok := protectedNamespaces[ns] - return ok -} - -// filterProtectedNamespaces returns raw with every protected/system namespace -// removed. It is the I7 defense-in-depth guard: even if a protected namespace is -// added to a cleanup list by mistake, it can never be deleted. -func filterProtectedNamespaces(raw []string) []string { - out := make([]string, 0, len(raw)) - for _, ns := range raw { - if !isProtectedNamespace(ns) { - out = append(out, ns) - } - } - return out -} - -// cleanupKubernetesResources removes namespaces created by OpenFrame components -// via the native Kubernetes client (client-go). It never touches -// protected/system namespaces. -// It returns the number of namespaces whose deletion was accepted by the API -// server. -func (s *ClusterService) cleanupKubernetesResources(ctx context.Context, clusterName string, verbose bool, _ bool) (int, error) { - // On Windows the cluster lives in WSL and must be reached from inside WSL. - if err := platform.WSLClusterHint("clean up OpenFrame namespaces"); err != nil { - return 0, err - } - - // TLS policy is the provider's mint-time decision: k3d marks its local - // rest.Config insecure itself (verify.go), and a future cloud provider's - // config must NOT be downgraded here. - restConfig, err := s.manager.GetRestConfig(ctx, clusterName) - if err != nil { - return 0, fmt.Errorf("failed to get cluster config for cleanup: %w", err) - } - client, err := kubernetes.NewForConfig(restConfig) - if err != nil { - return 0, fmt.Errorf("failed to create kubernetes client: %w", err) - } - - // Namespaces created by OpenFrame component installs. System namespaces are - // intentionally absent and are additionally filtered (I7 defense-in-depth). - var deleted int - var failed []string - for _, namespace := range filterProtectedNamespaces([]string{"argocd", "openframe"}) { - if _, err := client.CoreV1().Namespaces().Get(ctx, namespace, metav1.GetOptions{}); err != nil { - continue // doesn't exist (or unreachable) — skip - } - - if verbose { - pterm.Info.Printf("Cleaning up namespace: %s\n", namespace) - } - - if err := client.CoreV1().Namespaces().Delete(ctx, namespace, metav1.DeleteOptions{}); err != nil && !k8serrors.IsNotFound(err) { - failed = append(failed, namespace) - if verbose { - pterm.Warning.Printf("Failed to delete namespace %s: %v\n", namespace, err) - } - } else { - deleted++ - if verbose { - pterm.Success.Printf("Deleted namespace: %s\n", namespace) - } - } - } - - if len(failed) > 0 { - return deleted, fmt.Errorf("could not delete namespace(s): %s", strings.Join(failed, ", ")) - } - return deleted, nil -} - // cleanupNodeImages reclaims disk by removing unused container images inside // each k3d node. It returns the number of nodes pruned without error. // diff --git a/internal/cluster/service_test.go b/internal/cluster/service_test.go index 21250b92..21efb3fe 100644 --- a/internal/cluster/service_test.go +++ b/internal/cluster/service_test.go @@ -205,7 +205,7 @@ func TestClusterService_CleanupCluster(t *testing.T) { exec := createTestExecutor() service := NewClusterService(exec) - _, err := service.CleanupCluster(context.Background(), "test-cluster", models.ClusterTypeK3d, false, false) + _, err := service.CleanupCluster(context.Background(), "test-cluster", models.ClusterTypeK3d, false) if err != nil { t.Errorf("CleanupCluster should not error: %v", err) } diff --git a/internal/cluster/ui/cleanup_summary_test.go b/internal/cluster/ui/cleanup_summary_test.go index 4ed04b2d..91415cfd 100644 --- a/internal/cluster/ui/cleanup_summary_test.go +++ b/internal/cluster/ui/cleanup_summary_test.go @@ -32,55 +32,45 @@ func captureUI(t *testing.T, fn func()) string { // TestShowCleanupSummary_ReportsRealCounts (M2.1): the summary must describe // what happened. The old one printed "Removed unused Docker images / Freed up // disk space / Optimized cluster performance" unconditionally — the same text -// whether cleanup removed twenty objects or none. +// whether cleanup pruned twenty nodes or none. func TestShowCleanupSummary_ReportsRealCounts(t *testing.T) { ui := NewOperationsUI() out := captureUI(t, func() { - ui.ShowCleanupSummary("dev", models.CleanupResult{ - ApplicationsDeleted: 3, - ReleasesRemoved: 2, - NamespacesDeleted: 1, - }) + ui.ShowCleanupSummary("dev", models.CleanupResult{NodesPruned: 3}) }) - for _, want := range []string{"3 ArgoCD application(s)", "2 Helm release(s)", "1 namespace(s)"} { - if !strings.Contains(out, want) { - t.Errorf("summary must report %q; got:\n%s", want, out) - } - } - // Counts that are zero are not printed as noise. - if strings.Contains(out, "node(s) pruned") { - t.Errorf("a zero count must not be listed; got:\n%s", out) + if !strings.Contains(out, "3 node(s)") { + t.Errorf("summary must report the pruned node count; got:\n%s", out) } if strings.Contains(out, "Freed up disk space") { t.Errorf("the summary must not claim un-measured outcomes; got:\n%s", out) } } -// TestShowCleanupSummary_EmptyClusterSaysSo: removing nothing must read as -// "nothing to remove", not as a list of accomplishments. +// TestShowCleanupSummary_EmptyClusterSaysSo: pruning nothing must read as +// "nothing to prune", not as a list of accomplishments. func TestShowCleanupSummary_EmptyClusterSaysSo(t *testing.T) { out := captureUI(t, func() { NewOperationsUI().ShowCleanupSummary("dev", models.CleanupResult{}) }) - if !strings.Contains(out, "Nothing to remove") { + if !strings.Contains(out, "Nothing to prune") { t.Errorf("an empty cleanup must say so; got:\n%s", out) } } // TestShowCleanupSummary_PartialFailureIsVisible: cleanup swallows phase errors -// by design so a broken cluster can still be torn down. That is only safe if -// the user is told which phases failed — otherwise "cleanup completed" is a lie -// and the leftover resources are a surprise. +// by design so a partly-unreachable cluster can still be pruned. That is only +// safe if the user is told which phases failed — otherwise "cleanup completed" +// is a lie and the leftover images are a surprise. func TestShowCleanupSummary_PartialFailureIsVisible(t *testing.T) { - result := models.CleanupResult{ReleasesRemoved: 1} - result.AddFailure("Kubernetes namespaces", errors.New("connection refused")) + result := models.CleanupResult{NodesPruned: 1} + result.AddFailure("Container images", errors.New("connection refused")) out := captureUI(t, func() { NewOperationsUI().ShowCleanupSummary("dev", result) }) if strings.Contains(out, "cleanup completed") { t.Errorf("a partial cleanup must not be reported as completed; got:\n%s", out) } - for _, want := range []string{"finished with problems", "Kubernetes namespaces", "connection refused", "some resources may remain"} { + for _, want := range []string{"finished with problems", "Container images", "connection refused", "some resources may remain"} { if !strings.Contains(out, want) { t.Errorf("summary must surface %q; got:\n%s", want, out) } diff --git a/internal/cluster/ui/operations.go b/internal/cluster/ui/operations.go index c6f2ce2e..5b59163f 100644 --- a/internal/cluster/ui/operations.go +++ b/internal/cluster/ui/operations.go @@ -196,10 +196,12 @@ func (ui *OperationsUI) SelectClusterForCleanup(clusters []models.ClusterInfo, a } // confirmCleanup asks for user confirmation before cleaning up a cluster. +// The prompt says exactly what cleanup does — prune unused images — so nobody +// confirms it expecting (or fearing) a platform teardown. // Non-interactive sessions fail fast with a --force hint instead of blocking. func (ui *OperationsUI) confirmCleanup(clusterName string) (bool, error) { return sharedUI.RequireConfirmation( - fmt.Sprintf("Are you sure you want to cleanup cluster '%s'?", pterm.Cyan(clusterName)), + fmt.Sprintf("Prune unused container images on cluster '%s'? Installed apps are not touched.", pterm.Cyan(clusterName)), "--force", false) } @@ -242,23 +244,9 @@ func (ui *OperationsUI) ShowCleanupSummary(clusterName string, result models.Cle // to stdout and survive --silent, whose contract is "errors only". pterm.DefaultBasicText.Println() if result.Removed() == 0 { - pterm.Info.Println("Nothing to remove: the cluster had no OpenFrame resources left.") + pterm.Info.Println("Nothing to prune: no cluster nodes had unused images.") } else { - pterm.Info.Printf("Removed:\n") - for _, line := range []struct { - n int - label string - }{ - {result.ApplicationsDeleted, "ArgoCD application(s)"}, - {result.FinalizersCleared, "stuck application finalizer(s) cleared"}, - {result.ReleasesRemoved, "Helm release(s)"}, - {result.NamespacesDeleted, "namespace(s)"}, - {result.NodesPruned, "node(s) pruned of unused container images"}, - } { - if line.n > 0 { - pterm.DefaultBasicText.Printf(" %d %s\n", line.n, line.label) - } - } + pterm.Info.Printf("Pruned unused container images on %d node(s)\n", result.NodesPruned) } if result.Partial() { From 6cc69c5dd756b624ddcaafbee851cecc2d26d89b Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Fri, 7 Aug 2026 15:45:10 +0300 Subject: [PATCH 02/13] fix(ui): stop the cloud delete box claiming full resource cleanup The GKE lifecycle report (addendum 2026-07-28) flagged a contradiction: the 'Cluster Deleted' box printed 'RESOURCES: Cleaned up' directly under the orphan-disk warning listing PVC-provisioned disks that survived the destroy. Those disks live outside terraform state; the sweep right above the box is what reports or (with consent) deletes them. The RESOURCES row now claims only what the path verified: k3d keeps 'Cleaned up' (delete removes everything the cluster owned), cloud types say 'Terraform-managed destroyed (leftovers, if any, reported above)'. Pinned by a test. Also align the cluster group help with cleanup's new image-prune-only scope. --- cmd/cluster/cluster.go | 2 +- internal/cluster/ui/operations.go | 13 +++++++++- internal/cluster/ui/operations_test.go | 36 ++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/cmd/cluster/cluster.go b/cmd/cluster/cluster.go index 16f44985..26aa0df1 100644 --- a/cmd/cluster/cluster.go +++ b/cmd/cluster/cluster.go @@ -25,7 +25,7 @@ This command group provides cluster lifecycle management functionality: • list - Show all managed clusters • status - Display detailed cluster information • use - Switch the kubectl context to a cluster - • cleanup - Remove unused images and resources + • cleanup - Prune unused container images from cluster nodes Supports K3d clusters for local development and Google GKE / AWS EKS for cloud deployments. diff --git a/internal/cluster/ui/operations.go b/internal/cluster/ui/operations.go index 5b59163f..79e0b339 100644 --- a/internal/cluster/ui/operations.go +++ b/internal/cluster/ui/operations.go @@ -267,6 +267,17 @@ func (ui *OperationsUI) ShowOperationSuccess(operation, clusterName string, clus case "delete": pterm.Success.Printf("Cluster '%s' deleted successfully\n", pterm.Cyan(clusterName)) + // The RESOURCES row must only claim what this path verified. A k3d + // delete removes everything the cluster owned. A cloud delete destroys + // the terraform-managed resources — PVC-provisioned disks live outside + // the state, and the provider's orphan sweep has already reported (or + // deleted) any survivors right above this box; "Cleaned up" printed + // under that warning would contradict it. + resources := pterm.Gray("Cleaned up") + if clusterType != models.ClusterTypeK3d { + resources = pterm.Gray("Terraform-managed destroyed (leftovers, if any, reported above)") + } + // Show detailed deletion box pterm.DefaultBasicText.Println() boxContent := fmt.Sprintf( @@ -277,7 +288,7 @@ func (ui *OperationsUI) ShowOperationSuccess(operation, clusterName string, clus pterm.Bold.Sprint(clusterName), strings.ToUpper(string(clusterType)), pterm.Red("Deleted"), - pterm.Gray("Cleaned up"), + resources, ) pterm.DefaultBox. diff --git a/internal/cluster/ui/operations_test.go b/internal/cluster/ui/operations_test.go index 71f873ce..75cb24f7 100644 --- a/internal/cluster/ui/operations_test.go +++ b/internal/cluster/ui/operations_test.go @@ -1,10 +1,13 @@ package ui import ( + "bytes" "errors" + "strings" "testing" "github.com/flamingo-stack/openframe-cli/internal/cluster/models" + "github.com/pterm/pterm" ) func TestOperationsUI_SelectClusterForOperation(t *testing.T) { @@ -150,6 +153,39 @@ func TestOperationsUI_ShowOperationSuccess(t *testing.T) { }) } +// TestShowOperationSuccess_ResourcesRowIsBackendHonest: a cloud delete's box +// must not claim "Cleaned up" — PVC-provisioned disks live outside terraform +// state, and the orphan sweep printed right above the box may just have +// reported survivors. Only k3d, where delete removes everything the cluster +// owned, keeps the unqualified claim. +func TestShowOperationSuccess_ResourcesRowIsBackendHonest(t *testing.T) { + captureBox := func(fn func()) string { + var buf bytes.Buffer + box := pterm.DefaultBox + defer func() { pterm.DefaultBox = box }() + pterm.DefaultBox = *pterm.DefaultBox.WithWriter(&buf) + fn() + return buf.String() + } + + gke := captureBox(func() { + NewOperationsUI().ShowOperationSuccess("delete", "dev", models.ClusterTypeGKE) + }) + if strings.Contains(gke, "Cleaned up") { + t.Errorf("a cloud delete box must not claim full resource cleanup; got:\n%s", gke) + } + if !strings.Contains(gke, "reported above") { + t.Errorf("a cloud delete box must point at the sweep report; got:\n%s", gke) + } + + k3d := captureBox(func() { + NewOperationsUI().ShowOperationSuccess("delete", "dev", models.ClusterTypeK3d) + }) + if !strings.Contains(k3d, "Cleaned up") { + t.Errorf("a k3d delete box keeps the cleaned-up claim; got:\n%s", k3d) + } +} + func TestOperationsUI_ShowOperationError(t *testing.T) { ui := NewOperationsUI() testErr := errors.New("test error message") From 936fb38dc015801ac56178921f697268b683413d Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Fri, 7 Aug 2026 15:59:36 +0300 Subject: [PATCH 03/13] feat(terraform): append every apply/destroy to a per-workspace terraform.log MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Long cloud operations left the terminal scrollback as their only record. The engine now tees terraform's raw JSON-UI stream into terraform.log next to the workspace state — each run appends under a timestamped header — and a failed apply/destroy names the log path in its error. Best-effort: an unwritable directory never blocks the operation. --- .../cluster/providers/terraform/engine.go | 50 ++++++++++++++++--- .../providers/terraform/engine_test.go | 36 +++++++++++++ 2 files changed, 80 insertions(+), 6 deletions(-) diff --git a/internal/cluster/providers/terraform/engine.go b/internal/cluster/providers/terraform/engine.go index c07c44e3..b56be78c 100644 --- a/internal/cluster/providers/terraform/engine.go +++ b/internal/cluster/providers/terraform/engine.go @@ -8,6 +8,7 @@ import ( "os" "os/exec" "path/filepath" + "time" "github.com/flamingo-stack/openframe-cli/internal/shared/download" "github.com/hashicorp/terraform-exec/tfexec" @@ -133,6 +134,37 @@ func (e *Engine) Init(ctx context.Context, dir string) error { return nil } +// OpLogName is the per-workspace record of terraform apply/destroy runs. Long +// cloud operations used to leave the terminal as their only record (report +// M8); every run now appends its raw terraform JSON-UI stream here, so a +// failure hours later is still diagnosable. +const OpLogName = "terraform.log" + +// opSinks builds the writer an apply/destroy streams into: the progress +// writer, teed into dir's terraform.log when it can be opened. Logging is +// best-effort — a directory that cannot take the log (read-only, gone) must +// never block the operation, so the fallback is the progress writer alone. +// The returned close is always safe to call; logPath is empty when no log is +// being written. +func (e *Engine) opSinks(dir, op string) (w io.Writer, close func(), logPath string) { + progress := newProgressWriter(e.verbose) + f, err := os.OpenFile(filepath.Join(dir, OpLogName), os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0o600) + if err != nil { + return progress, func() {}, "" + } + fmt.Fprintf(f, "=== terraform %s — %s ===\n", op, time.Now().UTC().Format(time.RFC3339)) + return io.MultiWriter(progress, f), func() { _ = f.Close() }, f.Name() +} + +// opFailure wraps a failed apply/destroy, pointing at the full log when one +// was written — the terminal scrollback must not be the only record. +func opFailure(op string, logPath string, err error) error { + if logPath != "" { + return fmt.Errorf("terraform %s failed (full log: %s): %w", op, logPath, err) + } + return fmt.Errorf("terraform %s failed: %w", op, err) +} + // Apply runs terraform apply in dir, streaming per-resource progress lines // (via terraform's machine-readable -json output) so a 15-minute cloud apply // is never a silent wait. It is idempotent: re-running after a partial @@ -142,8 +174,10 @@ func (e *Engine) Apply(ctx context.Context, dir string) error { if err != nil { return err } - if err := tf.ApplyJSON(ctx, newProgressWriter(e.verbose)); err != nil { - return fmt.Errorf("terraform apply failed: %w", err) + w, closeLog, logPath := e.opSinks(dir, "apply") + defer closeLog() + if err := tf.ApplyJSON(ctx, w); err != nil { + return opFailure("apply", logPath, err) } return nil } @@ -163,8 +197,10 @@ func (e *Engine) Destroy(ctx context.Context, dir string) error { if err != nil { return err } - if err := tf.DestroyJSON(ctx, newProgressWriter(e.verbose)); err != nil { - return fmt.Errorf("terraform destroy failed: %w", err) + w, closeLog, logPath := e.opSinks(dir, "destroy") + defer closeLog() + if err := tf.DestroyJSON(ctx, w); err != nil { + return opFailure("destroy", logPath, err) } return nil } @@ -273,8 +309,10 @@ func (e *Engine) ApplyPlan(ctx context.Context, dir, planFile string) error { if err != nil { return err } - if err := tf.ApplyJSON(ctx, newProgressWriter(e.verbose), tfexec.DirOrPlan(planFile)); err != nil { - return fmt.Errorf("terraform apply failed: %w", err) + w, closeLog, logPath := e.opSinks(dir, "apply") + defer closeLog() + if err := tf.ApplyJSON(ctx, w, tfexec.DirOrPlan(planFile)); err != nil { + return opFailure("apply", logPath, err) } return nil } diff --git a/internal/cluster/providers/terraform/engine_test.go b/internal/cluster/providers/terraform/engine_test.go index 10ff54a0..6f7a34fe 100644 --- a/internal/cluster/providers/terraform/engine_test.go +++ b/internal/cluster/providers/terraform/engine_test.go @@ -259,3 +259,39 @@ func TestNewEngine_VerboseWrapsRunnerForSelectiveStdout(t *testing.T) { assert.IsType(t, &tfexec.Terraform{}, quiet, "non-verbose needs no wrapper — stdout is never streamed") } + +// TestEngine_ApplyWritesOpLog (report M8): a long cloud operation must leave a +// file record, not just terminal scrollback. Apply tees terraform's raw +// JSON-UI stream into the workspace's terraform.log, and a failure names the +// log path. +func TestEngine_ApplyWritesOpLog(t *testing.T) { + dir := t.TempDir() + stream := `{"@level":"info","@message":"module.gke: Creating...","type":"apply_start"}` + "\n" + f := &fakeRunner{applyJSON: stream} + e := engineWith(f) + + require.NoError(t, e.Apply(context.Background(), dir)) + + logged, err := os.ReadFile(filepath.Join(dir, OpLogName)) + require.NoError(t, err, "apply must write %s in the workspace dir", OpLogName) + assert.Contains(t, string(logged), "=== terraform apply", "each run starts with a header") + assert.Contains(t, string(logged), "module.gke: Creating...", "the raw stream is preserved") + + // A second run appends rather than truncates: the log is the workspace's + // operation history. + require.NoError(t, e.Apply(context.Background(), dir)) + logged2, err := os.ReadFile(filepath.Join(dir, OpLogName)) + require.NoError(t, err) + assert.Greater(t, len(logged2), len(logged), "runs append, never truncate") +} + +// TestEngine_ApplyFailureNamesTheLog: the error must point at the full record. +func TestEngine_ApplyFailureNamesTheLog(t *testing.T) { + dir := t.TempDir() + f := &fakeRunner{apply: errors.New("quota exceeded")} + e := engineWith(f) + + err := e.Apply(context.Background(), dir) + require.Error(t, err) + assert.Contains(t, err.Error(), OpLogName, "the failure must name the log file") +} From 619c385a5e2ca7d955cd5a34fa2db6134411db6f Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Fri, 7 Aug 2026 15:59:36 +0300 Subject: [PATCH 04/13] feat(gke): shared-project safety and honest cost/node UX from the verification report MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the design findings of the GKE lifecycle verification report (the retry hint for failed infracost downloads and the GCP project picker were already implemented): Project APIs are no longer terraform-managed. As google_project_service resources, every cluster workspace in a shared project claimed ownership of the same project-level toggles and every destroy planned their removal. The CLI now enables the required APIs (compute, container) with one idempotent 'gcloud services enable' before terraform runs — create-time step, owned by no cluster's state. A denied enable falls back to an enabled-state probe so deploy-only identities on an already-configured project still proceed; genuinely missing APIs stop the create with the exact manual command. Dry-run is untouched: plan keeps zero project side effects. Every display of a regional (--ha) cluster shows the per-zone math. 'Nodes: 3' that silently provisioned 9 (regional counts are per zone) now reads '3 per zone × 3 zones = 9 total (regional)' in the configuration summary and wizard; the workspace record gains an HA field so cluster list/status report the real node total for regional clusters. The cost warning now advertises --spot for test clusters (typically 60-90% off the node cost) whenever a cloud config doesn't already use it, and the summaries show a Spot row when it does. Template propagation was verified: GKE passes spot=var.spot, EKS maps it to capacity_type=SPOT. --- internal/cluster/models/cluster.go | 7 +++ .../gke/gcloud_command_contract_test.go | 45 +++++++++++++++ internal/cluster/providers/gke/provider.go | 57 ++++++++++++++++++- .../providers/gke/template_guard_test.go | 18 ++---- .../cluster/providers/gke/templates/main.tf | 16 ++---- .../cluster/providers/terraform/workspace.go | 4 ++ internal/cluster/ui/operations.go | 8 ++- internal/cluster/ui/prompts.go | 29 ++++++++++ internal/cluster/ui/wizard_steps.go | 8 ++- 9 files changed, 166 insertions(+), 26 deletions(-) diff --git a/internal/cluster/models/cluster.go b/internal/cluster/models/cluster.go index 80e2cc46..9e919c12 100644 --- a/internal/cluster/models/cluster.go +++ b/internal/cluster/models/cluster.go @@ -48,6 +48,13 @@ type ClusterConfig struct { Cloud *CloudConfig `json:"cloud,omitempty"` } +// GKERegionalZones is how many zones GKE spreads a regional cluster across by +// default (the module passes no explicit node locations). A regional node +// pool's node count is PER ZONE, so every display of an HA cluster's nodes +// must show the ×zones math — "Nodes: 3" that silently provisions 9 was a +// verification-report finding (S2). +const GKERegionalZones = 3 + // CloudConfig holds the provider-agnostic knobs for a managed cloud cluster. type CloudConfig struct { Region string `json:"region"` diff --git a/internal/cluster/providers/gke/gcloud_command_contract_test.go b/internal/cluster/providers/gke/gcloud_command_contract_test.go index ea4ff14f..131cf37d 100644 --- a/internal/cluster/providers/gke/gcloud_command_contract_test.go +++ b/internal/cluster/providers/gke/gcloud_command_contract_test.go @@ -44,6 +44,35 @@ func TestGcloudCommandContract(t *testing.T) { {"gcloud", "projects", "describe", "my-project", "--format=value(projectId)"}, }, }, + { + name: "project services: single idempotent enable of the required APIs", + run: func(t *testing.T, p *Provider) { + require.NoError(t, p.ensureProjectServices(context.Background(), "my-project")) + }, + want: [][]string{ + {"gcloud", "services", "enable", "compute.googleapis.com", "container.googleapis.com", + "--project", "my-project"}, + }, + }, + { + name: "project services: denied enable falls back to an enabled-state probe", + prepare: func(mock *executor.MockCommandExecutor) { + mock.SetResponse("services enable", &executor.CommandResult{ + ExitCode: 1, Stderr: "PERMISSION_DENIED"}) + mock.SetResponse("services list", &executor.CommandResult{ + ExitCode: 0, Stdout: "compute.googleapis.com\ncontainer.googleapis.com\n"}) + }, + run: func(t *testing.T, p *Provider) { + // Both APIs are already on — a deploy-only identity may proceed. + require.NoError(t, p.ensureProjectServices(context.Background(), "my-project")) + }, + want: [][]string{ + {"gcloud", "services", "enable", "compute.googleapis.com", "container.googleapis.com", + "--project", "my-project"}, + {"gcloud", "services", "list", "--enabled", "--project", "my-project", + "--format=value(config.name)"}, + }, + }, { name: "name-collision preflight: project-wide (location-unscoped) name filter", run: func(t *testing.T, p *Provider) { @@ -113,3 +142,19 @@ func TestExecConfig_PinsPluginContract(t *testing.T) { assert.Equal(t, clientcmdapi.NeverExecInteractiveMode, cfg.InteractiveMode) assert.True(t, cfg.ProvideClusterInfo) } + +// TestEnsureProjectServices_FailsWhenAPIsAreOff: the enabled-state fallback is +// only an escape hatch for deploy-only identities on an already-configured +// project. When a required API is genuinely off and cannot be enabled, create +// must stop with the exact manual command, not proceed into a terraform apply +// that fails minutes later. +func TestEnsureProjectServices_FailsWhenAPIsAreOff(t *testing.T) { + mock := executor.NewMockCommandExecutor() + mock.SetResponse("services enable", &executor.CommandResult{ExitCode: 1, Stderr: "PERMISSION_DENIED"}) + mock.SetResponse("services list", &executor.CommandResult{ExitCode: 0, Stdout: "compute.googleapis.com\n"}) + p := NewWithDeps(nil, nil, mock) + + err := p.ensureProjectServices(context.Background(), "my-project") + require.Error(t, err, "a missing required API must stop the create") + assert.Contains(t, err.Error(), "gcloud services enable", "the error must carry the manual fix") +} diff --git a/internal/cluster/providers/gke/provider.go b/internal/cluster/providers/gke/provider.go index da888460..be2f3c86 100644 --- a/internal/cluster/providers/gke/provider.go +++ b/internal/cluster/providers/gke/provider.go @@ -101,6 +101,47 @@ func (p *Provider) preflightCredentials(ctx context.Context, project string) err return nil } +// requiredServices are the project APIs a GKE cluster needs active. They used +// to be google_project_service resources inside EVERY cluster workspace, so +// two clusters in one shared project claimed ownership of the same +// project-level toggles and every destroy planned their removal (report M5). +// Enabling is now this create-time step: idempotent, and owned by no cluster's +// terraform state. +var requiredServices = []string{"compute.googleapis.com", "container.googleapis.com"} + +// ensureProjectServices enables the required project APIs before terraform +// runs. Enabling needs the serviceusage permission, which a +// deploy-only identity may lack — so a failed enable is only fatal when the +// APIs are actually off: already-enabled APIs let the create proceed. +func (p *Provider) ensureProjectServices(ctx context.Context, project string) error { + args := append([]string{"services", "enable"}, requiredServices...) + args = append(args, "--project", project) + if _, err := p.executor.Execute(ctx, "gcloud", args...); err == nil { + return nil + } + + res, listErr := p.executor.Execute(ctx, "gcloud", "services", "list", "--enabled", + "--project", project, "--format=value(config.name)") + if listErr == nil && res != nil { + enabled := make(map[string]struct{}) + for _, line := range strings.Split(res.Stdout, "\n") { + enabled[strings.TrimSpace(line)] = struct{}{} + } + missing := false + for _, svc := range requiredServices { + if _, ok := enabled[svc]; !ok { + missing = true + break + } + } + if !missing { + return nil // enable was denied, but everything needed is already on + } + } + return fmt.Errorf("required GCP APIs could not be enabled on project %s — enable them once with an authorized identity:\n gcloud services enable %s --project %s", + project, strings.Join(requiredServices, " "), project) +} + // preflightNameCollision refuses to create a cluster whose name already // exists in the target project but has no openframe workspace: terraform // would build the VPC first and then fail mid-apply on the duplicate cluster, @@ -244,6 +285,12 @@ func (p *Provider) CreateCluster(ctx context.Context, config models.ClusterConfi if err := p.preflightCredentials(ctx, config.Cloud.Project); err != nil { return nil, err } + // Enable the required project APIs here, not in the module (see + // requiredServices). Deliberately absent from PlanCluster: a dry-run must + // keep classic plan semantics — zero side effects on the project. + if err := p.ensureProjectServices(ctx, config.Cloud.Project); err != nil { + return nil, err + } if err := p.ensureZone(ctx, &config); err != nil { return nil, err } @@ -268,6 +315,7 @@ func (p *Provider) CreateCluster(ctx context.Context, config models.ClusterConfi Project: config.Cloud.Project, K8sVersion: vars.KubernetesVersion, NodeCount: config.NodeCount, + HA: config.Cloud.HA, CreatedAt: time.Now().UTC(), } if err := ws.Scaffold(record, mainTF, vars); err != nil { @@ -349,6 +397,7 @@ func (p *Provider) CreateCluster(ctx context.Context, config models.ClusterConfi // --nodes) reports the first attempt's values from list/status forever. record.Region = config.Cloud.Region record.NodeCount = config.NodeCount + record.HA = config.Cloud.HA record.K8sVersion = vars.KubernetesVersion endpoint, err := tfengine.StringOutput(outputs, "cluster_endpoint") if err != nil { @@ -516,6 +565,12 @@ func kubeContextFor(rec tfengine.Record) string { // infoFor maps a registry record onto the shared ClusterInfo shape. func infoFor(rec tfengine.Record) models.ClusterInfo { kubeContext := kubeContextFor(rec) + // A regional cluster's recorded count is per zone; list/status must show + // the real node total, not the flag value that under-reports it 3× (S2). + nodeCount := rec.NodeCount + if rec.HA { + nodeCount = rec.NodeCount * models.GKERegionalZones + } return models.ClusterInfo{ Name: rec.Name, Type: models.ClusterTypeGKE, @@ -524,7 +579,7 @@ func infoFor(rec tfengine.Record) models.ClusterInfo { Project: rec.Project, Region: rec.Region, Status: rec.Status.Title(), - NodeCount: rec.NodeCount, + NodeCount: nodeCount, K8sVersion: rec.K8sVersion, CreatedAt: rec.CreatedAt, } diff --git a/internal/cluster/providers/gke/template_guard_test.go b/internal/cluster/providers/gke/template_guard_test.go index 872c162c..5a31dcac 100644 --- a/internal/cluster/providers/gke/template_guard_test.go +++ b/internal/cluster/providers/gke/template_guard_test.go @@ -29,20 +29,14 @@ func TestTemplate_NeverManagesTheProject(t *testing.T) { "GKE template must not declare %s — the project must stay a read-only input, never a managed/destroyable resource", decl) } - // Belt and braces: no google_project* resource of ANY kind. The only - // project-scoped resource we allow is google_project_service (API - // enablement), and even that must never disable APIs on destroy. + // Belt and braces: no google_project* resource of ANY kind — including + // google_project_service. API enablement moved to a create-time gcloud step + // (ensureProjectServices) precisely so no cluster workspace owns + // project-level state in a shared project (report M5). projectResRE := regexp.MustCompile(`resource\s+"(google_project[a-z_]*)"`) for _, m := range projectResRE.FindAllStringSubmatch(src, -1) { - assert.Equalf(t, "google_project_service", m[1], - "unexpected project-scoped resource %q in the GKE template; only google_project_service is permitted", m[1]) - } - - // google_project_service must keep disable_on_destroy=false so a cluster - // teardown never switches off project APIs other workloads depend on. - if assert.Contains(t, src, `resource "google_project_service"`) { - assert.Contains(t, src, "disable_on_destroy = false", - "google_project_service must set disable_on_destroy=false so destroy never disables project APIs") + assert.Failf(t, "project-scoped resource in the GKE template", + "%q must not be managed per-cluster: project-level state belongs to no single cluster (M5)", m[1]) } } diff --git a/internal/cluster/providers/gke/templates/main.tf b/internal/cluster/providers/gke/templates/main.tf index 174c8baa..03aae8c7 100644 --- a/internal/cluster/providers/gke/templates/main.tf +++ b/internal/cluster/providers/gke/templates/main.tf @@ -76,15 +76,11 @@ provider "google" { } } -# GKE needs these services active in the project; disable_on_destroy=false so -# a cluster teardown never switches off APIs other workloads may use. -resource "google_project_service" "required" { - for_each = toset(["compute.googleapis.com", "container.googleapis.com"]) - - service = each.value - disable_on_destroy = false -} - +# The required project APIs (compute, container) are enabled by the CLI before +# terraform runs — deliberately NOT managed here. As google_project_service +# resources, every cluster workspace in a shared project claimed ownership of +# the same project-level toggles, and every destroy planned their removal +# (report M5). Project-level state belongs to no single cluster. module "network" { source = "terraform-google-modules/network/google" version = "~> 18.0" @@ -112,8 +108,6 @@ module "network" { } ] } - - depends_on = [google_project_service.required] } # Private nodes have no external IPs (required by orgs enforcing the diff --git a/internal/cluster/providers/terraform/workspace.go b/internal/cluster/providers/terraform/workspace.go index 8abb30df..cb4eaa9d 100644 --- a/internal/cluster/providers/terraform/workspace.go +++ b/internal/cluster/providers/terraform/workspace.go @@ -57,6 +57,10 @@ type Record struct { Project string `json:"project,omitempty"` // GCP K8sVersion string `json:"k8s_version,omitempty"` NodeCount int `json:"node_count"` + // HA records a regional (multi-zone) cluster, where NodeCount is per zone + // rather than a total. Absent (false) on zonal clusters and on records + // predating this field — those display NodeCount as-is. + HA bool `json:"ha,omitempty"` CreatedAt time.Time `json:"created_at"` Endpoint string `json:"endpoint,omitempty"` CACert string `json:"ca_cert,omitempty"` // base64, as EKS emits it diff --git a/internal/cluster/ui/operations.go b/internal/cluster/ui/operations.go index 79e0b339..77baeba2 100644 --- a/internal/cluster/ui/operations.go +++ b/internal/cluster/ui/operations.go @@ -335,7 +335,7 @@ func (ui *OperationsUI) ShowConfigurationSummary(config models.ClusterConfig, dr // "silent" output (verification report saw the leak and graded it silent). pterm.DefaultBasicText.Printf(" Name: %s\n", pterm.Cyan(config.Name)) pterm.DefaultBasicText.Printf(" Type: %s\n", string(config.Type)) - pterm.DefaultBasicText.Printf(" Nodes: %d\n", config.NodeCount) + pterm.DefaultBasicText.Printf(" Nodes: %s\n", NodesLine(config)) if config.K8sVersion != "" { pterm.DefaultBasicText.Printf("Version: %s\n", config.K8sVersion) @@ -348,7 +348,13 @@ func (ui *OperationsUI) ShowConfigurationSummary(config models.ClusterConfig, dr if config.Cloud.MachineType != "" { pterm.DefaultBasicText.Printf("Instance: %s\n", config.Cloud.MachineType) } + if config.Cloud.Spot { + pterm.DefaultBasicText.Printf(" Spot: yes (spot-capacity nodes)\n") + } pterm.Warning.Println(CostHint(config.Type)) + if hint := SpotHint(config); hint != "" { + pterm.Info.Println(hint) + } } pterm.DefaultBasicText.Println() diff --git a/internal/cluster/ui/prompts.go b/internal/cluster/ui/prompts.go index 9f999fcb..40a5756e 100644 --- a/internal/cluster/ui/prompts.go +++ b/internal/cluster/ui/prompts.go @@ -2,6 +2,7 @@ package ui import ( "fmt" + "strconv" "strings" "github.com/flamingo-stack/openframe-cli/internal/cluster/models" @@ -83,6 +84,34 @@ func CostHint(clusterType models.ClusterType) string { } } +// NodesLine renders a config's node count honestly: a regional (--ha) cluster +// provisions its count PER ZONE, so "3" would silently mean 9 nodes and ~3× +// the expected bill (verification-report finding S2 — the summary said 3, GCP +// came up with 9). Zonal clusters keep the plain number. +func NodesLine(config models.ClusterConfig) string { + if config.Cloud == nil || !config.Cloud.HA { + return strconv.Itoa(config.NodeCount) + } + return fmt.Sprintf("%d per zone × %d zones = %d total (regional)", + config.NodeCount, models.GKERegionalZones, config.NodeCount*models.GKERegionalZones) +} + +// SpotHint nudges test-cluster users toward spot capacity, next to the cost +// warning — the flag already exists but nothing advertised it. Empty when spot +// is already on (nothing to suggest) or for non-cloud types. Like CostHint it +// carries no exact price: the discount range is the provider's own +// (preemptible/spot pricing), not a figure this CLI computes. +func SpotHint(config models.ClusterConfig) string { + if config.Cloud == nil || config.Cloud.Spot { + return "" + } + switch config.Type { + case models.ClusterTypeEKS, models.ClusterTypeGKE: + return "Tip: for test clusters, --spot runs nodes on spot capacity (typically 60-90% off the node cost)" + } + return "" +} + // ConfirmTypedClusterName requires the user to re-type the cluster name // before a cloud destroy — a stronger gate than yes/no, because the action // deletes billed infrastructure irreversibly. diff --git a/internal/cluster/ui/wizard_steps.go b/internal/cluster/ui/wizard_steps.go index 317c885a..529faf6c 100644 --- a/internal/cluster/ui/wizard_steps.go +++ b/internal/cluster/ui/wizard_steps.go @@ -241,7 +241,7 @@ func (ws *WizardSteps) ConfirmConfiguration(config models.ClusterConfig) (bool, {"Setting", "Value"}, {"Cluster Name", config.Name}, {"Cluster Type", string(config.Type)}, - {"Node Count", strconv.Itoa(config.NodeCount)}, + {"Node Count", NodesLine(config)}, {"Kubernetes Version", config.K8sVersion}, } if config.Cloud != nil { @@ -255,10 +255,16 @@ func (ws *WizardSteps) ConfirmConfiguration(config models.ClusterConfig) (bool, if config.Cloud.MachineType != "" { data = append(data, []string{"Instance Type", config.Cloud.MachineType}) } + if config.Cloud.Spot { + data = append(data, []string{"Spot Nodes", "yes"}) + } } if config.Cloud != nil { pterm.Warning.Println(CostHint(config.Type)) + if hint := SpotHint(config); hint != "" { + pterm.Info.Println(hint) + } } // Use pterm for consistent styling From d3c472d79b30d55adee13af1e1fad96ccc009715 Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Fri, 7 Aug 2026 16:02:27 +0300 Subject: [PATCH 05/13] docs(cli): rewrite the root help to describe the CLI as it is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The old text pitched the tool against shell scripts it replaced long ago and praised its own design instead of telling the user what the commands do. The new help states the actual surface: local k3d and Terraform-backed GKE/EKS provisioning, the app-of-apps install, the typical local and cloud flows, and what each command group covers — including cleanup's real (image-prune-only) scope and the plan/confirm safety of cloud creates. --- cmd/root.go | 38 +++++++++++++++++++++++--------------- cmd/root_test.go | 2 +- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index 6020a208..2d987fbd 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -100,21 +100,29 @@ func GetRootCmd(versionInfo VersionInfo) *cobra.Command { func buildRootCommand(versionInfo VersionInfo) *cobra.Command { rootCmd := &cobra.Command{ Use: "openframe", - Short: "OpenFrame CLI - Kubernetes cluster bootstrapping and chart deployment", - Long: `OpenFrame CLI - Interactive Kubernetes Platform Bootstrapper - -OpenFrame CLI replaces the shell scripts with a modern, interactive terminal UI -for managing OpenFrame Kubernetes deployments. Built following best practices -for CLI design with wizard-style interactive prompts. - -Key Features: - - Interactive Wizard - Step-by-step guided setup - - Cluster Management - local K3d and cloud GKE / AWS EKS clusters - - Helm Integration - App-of-Apps pattern with ArgoCD - - Prerequisite Checking - Validates tools before running - -The CLI provides both interactive modes for new users and flag-based -operation for automation and power users.`, + Short: "OpenFrame CLI - provision Kubernetes clusters and deploy the OpenFrame platform", + Long: `OpenFrame CLI - Kubernetes Platform Bootstrapper + +Provision a Kubernetes cluster — local k3d for development, or cloud GKE/EKS +via Terraform — install the OpenFrame platform onto it (ArgoCD app-of-apps), +and manage the full lifecycle: prerequisites, status, upgrades, teardown. + +Typical flows: + openframe bootstrap # local: k3d cluster + platform in one step + openframe cluster create --type gke # cloud: plan, confirm, provision... + openframe app install # ...then install the platform onto it + +Command groups: + cluster create, delete, list, status, use, cleanup (prune node images) + app install, upgrade, status, access, uninstall + bootstrap cluster create + app install in one step + prerequisites check and install required tools (--type k3d|eks|gke) + update update this CLI to a newer release + +Every command runs interactively by default (wizards, confirmations) and +non-interactively with flags for CI and automation. Cloud creates show a full +terraform plan (and an infracost estimate, when installed) before anything is +applied; deletes require typed confirmation and clean up after themselves.`, Version: fmt.Sprintf("%s (%s) built on %s", versionInfo.Version, versionInfo.Commit, versionInfo.Date), // Silence errors and usage globally - we handle our own error display SilenceErrors: true, diff --git a/cmd/root_test.go b/cmd/root_test.go index c32510de..e0285721 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -31,7 +31,7 @@ func TestRootCommand(t *testing.T) { t.Errorf("expected Use to be 'openframe', got %q", cmd.Use) } - expectedShort := "OpenFrame CLI - Kubernetes cluster bootstrapping and chart deployment" + expectedShort := "OpenFrame CLI - provision Kubernetes clusters and deploy the OpenFrame platform" if cmd.Short != expectedShort { t.Errorf("expected Short to be %q, got %q", expectedShort, cmd.Short) } From fc555ab2fee6b1f4729f43150e8900756d995488 Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Fri, 7 Aug 2026 16:06:14 +0300 Subject: [PATCH 06/13] docs: sync guides with the report-driven changes on this branch - GKE workflow: project APIs are enabled in preflight via gcloud (not terraform), the dry-run example no longer lists google_project_service and reflects the smaller zonal plan, the cluster is zonal by default (--ha for regional, with the per-zone node math spelled out), --spot is called out next to the cost note, and delete now describes the disk-release phase and the consented orphan sweep - Cloud clusters reference: --ha and --spot documented, delete's release-and-sweep behavior explained, terraform.log added to the workspace contents and troubleshooting - Architecture decisions (D8): terraform.log listed among workspace files - Reference overview + README: cleanup described as image pruning only --- README.md | 2 +- docs/architecture/decisions.md | 3 +- docs/getting-started/cloud-clusters.md | 28 ++++++++++----- docs/getting-started/gke-workflow.md | 45 ++++++++++++++++--------- docs/reference/architecture/overview.md | 2 +- 5 files changed, 54 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index d6c91c4a..39f1e242 100644 --- a/README.md +++ b/README.md @@ -32,7 +32,7 @@ OpenFrame CLI is part of the broader [OpenFrame](https://openframe.ai) ecosystem - **Lifecycle operations**: Create, delete, list, and monitor Kubernetes clusters - **K3D integration**: Lightweight Kubernetes for development and testing - **Status monitoring**: Real-time cluster health and resource monitoring -- **Easy cleanup**: Remove clusters and associated resources with simple commands +- **Easy teardown**: `cluster delete` removes a cluster and its resources; `cluster cleanup` reclaims disk by pruning unused node images ### 📦 Chart & Application Management - **Helm chart installation**: Streamlined chart deployment with dependency management diff --git a/docs/architecture/decisions.md b/docs/architecture/decisions.md index 1c7c7a00..01ade042 100644 --- a/docs/architecture/decisions.md +++ b/docs/architecture/decisions.md @@ -137,7 +137,8 @@ terraform, that is a different BUSL use profile and needs its own review. ## D8 — Local terraform state in per-cluster workspaces Each cloud cluster owns a workspace under `~/.openframe/clusters//`: -the generated root module, `terraform.tfvars.json`, local state, and a +the generated root module, `terraform.tfvars.json`, local state, a +`terraform.log` every apply/destroy appends its output stream to, and a `cluster.json` registry record (type, status, endpoint/CA). The registry is what makes cloud clusters visible to `list`/`status`/`delete` without cloud API calls, and the state file is the only pointer to billed resources — so a diff --git a/docs/getting-started/cloud-clusters.md b/docs/getting-started/cloud-clusters.md index 2d07c0c6..8d2cf12a 100644 --- a/docs/getting-started/cloud-clusters.md +++ b/docs/getting-started/cloud-clusters.md @@ -74,8 +74,11 @@ openframe cluster create my-gke --type gke --project my-project --region us-cent Useful flags: `--machine-type`, `--min-nodes` / `--max-nodes` (autoscaler bounds; defaults 1 / 4, must be at least 1 — an explicit 0 is rejected), -`--spot`, `--profile` (AWS), `--nodes` (initial size), `--version` -(`.`, e.g. `1.33`). +`--spot` (spot-capacity nodes, typically 60–90% off the node cost — the cost +warning suggests it for test clusters), `--profile` (AWS), `--nodes` (initial +size), `--version` (`.`, e.g. `1.33`), `--ha` (GKE: regional +control plane and nodes; the node count is then **per zone**, and every +summary shows the `N per zone × 3 zones` math). In interactive sessions the CLI first shows the full Terraform plan and asks for approval (the `terraform apply` shape; what you approve is exactly what @@ -118,9 +121,11 @@ throwaway directory. ## Where the state lives Each cloud cluster owns a workspace in `~/.openframe/clusters//`: the -generated Terraform module and the state file. The state is the only pointer -to your billed cloud resources — the workspace is never deleted on a failed -create, only after a successful delete. +generated Terraform module, the state file, and a `terraform.log` that every +apply/destroy appends its full output stream to (so a long operation leaves a +record beyond the terminal). The state is the only pointer to your billed +cloud resources — the workspace is never deleted on a failed create, only +after a successful delete. - **A create failed or was interrupted?** Re-run the same `cluster create` — it resumes where it stopped. @@ -143,8 +148,12 @@ openframe app install # install OpenFrame onto the current conte credentials via gcloud when the kubeconfig has no entry yet, and activates the gcloud configuration matching the cluster's project. -`cluster delete --force` skips the typed confirmation (for CI). `cluster -cleanup` does not apply to cloud clusters — use `delete`. +`cluster delete` tears down more than the terraform state: application +namespaces are removed first so PVC-backed disks/volumes are reclaimed while +the nodes still run, and anything that survives the destroy is swept up +afterwards — listed and deleted with your consent. `--force` skips the typed +confirmation and consents to that sweep (for CI). `cluster cleanup` does not +apply to cloud clusters — use `delete`. ## Troubleshooting @@ -160,4 +169,7 @@ cleanup` does not apply to cloud clusters — use `delete`. `cluster create ` to resume, or `cluster delete ` to tear down what was partially created. - **Verbose Terraform output** — add `--verbose` to stream Terraform's own - logs during create/delete. + logs during create/delete. Either way, the full stream of every + apply/destroy is appended to + `~/.openframe/clusters//terraform/terraform.log`, and a failed + operation names that path. diff --git a/docs/getting-started/gke-workflow.md b/docs/getting-started/gke-workflow.md index 92b5eb94..d97687c7 100644 --- a/docs/getting-started/gke-workflow.md +++ b/docs/getting-started/gke-workflow.md @@ -22,12 +22,12 @@ without registering the cluster: ```bash openframe cluster create my-gke --type gke \ --project my-project --region us-central1 --skip-wizard --dry-run -# + google_project_service.required["container.googleapis.com"] # + module.network.module.vpc.google_compute_network.network +# + module.network.module.subnets.google_compute_subnetwork.subnetwork["us-central1/my-gke-subnet"] # + module.gke.google_container_cluster.primary # + module.gke.google_container_node_pool.pools["default"] # ... -# Plan: 27 to add, 0 to change, 0 to destroy +# Plan: 11 to add, 0 to change, 0 to destroy ``` If Terraform is not installed yet, the preview is skipped with a note — it @@ -52,7 +52,10 @@ openframe cluster create my-gke --type gke \ ``` Useful extras: `--machine-type e2-standard-8`, `--min-nodes 1 --max-nodes 6`, -`--spot`, `--version 1.33`, `--nodes 4`. +`--spot` (spot-capacity nodes, typically 60–90% off the node cost — good for +test clusters), `--version 1.33`, `--nodes 4`, `--ha` (regional control plane +and nodes — note that a regional node count is **per zone**: `--nodes 3 --ha` +provisions 3 × 3 zones = 9 nodes, and the CLI displays exactly that math). What happens, in order — no manual steps in between: @@ -63,21 +66,26 @@ What happens, in order — no manual steps in between: uses Application Default Credentials, it offers `gcloud auth application-default login` too. CI/non-interactive sessions never get prompts — they fail with the exact command to run. -3. **Preflight**: project access is verified, and the CLI refuses to proceed - if a cluster with this name already exists in the project but was not - created by openframe (it will never touch clusters it does not own). +3. **Preflight**: project access is verified; the required project APIs + (Compute Engine, Kubernetes Engine) are enabled with one idempotent + `gcloud services enable` — deliberately outside Terraform, so no cluster's + state owns project-level toggles in a shared project; and the CLI refuses + to proceed if a cluster with this name already exists in the project but + was not created by openframe (it will never touch clusters it does not + own). 4. **Plan & confirm** (interactive sessions): the full Terraform plan is shown — every resource to be created and the summary line — and you are asked to approve it before anything is applied, exactly like `terraform apply`. What you approve is what runs (the saved plan is applied, not a re-plan). Declining a brand-new create leaves no trace. Non-interactive sessions auto-approve, as before. -5. **Provision** (~10–15 min): required project APIs, a dedicated VPC with - pod/service ranges, a Cloud NAT for egress, and a regional GKE cluster +5. **Provision** (~10–15 min): a dedicated VPC with pod/service ranges, a + Cloud NAT for egress, and a **zonal** GKE cluster (regional with `--ha`) with **private nodes** (no external IPs — compatible with orgs enforcing `restrict_vm_external_ips`) behind a public control-plane endpoint, streamed as per-resource progress lines. Add `--verbose` for raw - Terraform output. + Terraform output; the full stream is also appended to + `~/.openframe/clusters/my-gke/terraform/terraform.log` either way. 6. **Kubeconfig**: a context named exactly `my-gke` is merged into your kubeconfig (existing contexts are never overwritten) and made current. @@ -134,13 +142,20 @@ Want the state to survive your machine? Create with ```bash openframe cluster delete my-gke -# → asks you to re-type "my-gke", then terraform destroy removes the -# cluster, node pool, and VPC; the workspace and kubeconfig context are -# cleaned up afterwards +# → asks you to re-type "my-gke", then: +# 1. app namespaces are deleted first, so the CSI driver reclaims the +# PVC-backed Persistent Disks while the nodes still run (those disks +# live outside the terraform state and would otherwise survive as +# billable orphans) +# 2. terraform destroy removes the cluster, node pool, and VPC +# 3. any disk that still survived is swept up: listed, and deleted with +# your consent (interactive prompt, or automatically with --force) +# 4. the workspace and kubeconfig context are cleaned up ``` -`--force` skips the typed confirmation (CI). `cluster cleanup` does not apply -to cloud clusters — use `delete`. +`--force` skips the typed confirmation and consents to the orphan-disk +cleanup (CI). `cluster cleanup` does not apply to cloud clusters — use +`delete`. ## Troubleshooting @@ -151,7 +166,7 @@ to cloud clusters — use `delete`. | "already exists ... not managed by openframe" | the name is taken by a cluster openframe does not own — pick another name | | "kubeconfig context ... refusing to overwrite" | a same-named context points elsewhere — rename it or pick another cluster name | | create failed mid-way | re-run the same create to resume, or delete to tear down (state is never lost) | -| want Terraform's own logs | add `--verbose` | +| want Terraform's own logs | add `--verbose`, or read `~/.openframe/clusters//terraform/terraform.log` — every apply/destroy appends its full stream there | See [Cloud Clusters](./cloud-clusters.md) for the reference (flags, state model, EKS status) and `docs/architecture/decisions.md` (D5, D7, D8) for the diff --git a/docs/reference/architecture/overview.md b/docs/reference/architecture/overview.md index fab7600b..47484830 100644 --- a/docs/reference/architecture/overview.md +++ b/docs/reference/architecture/overview.md @@ -385,7 +385,7 @@ openframe bootstrap --verbose # Show detailed ArgoCD sync progress | `delete [NAME]` | Delete a cluster and its resources | `openframe cluster delete dev --force` | | `list` | List all managed clusters | `openframe cluster list -o json` | | `status [NAME]` | Show detailed cluster status | `openframe cluster status dev -o yaml` | -| `cleanup [NAME]` | Remove unused images and resources | `openframe cluster cleanup dev --force` | +| `cleanup [NAME]` | Prune unused container images from cluster nodes | `openframe cluster cleanup dev --force` | **`cluster create` flags:** From ee98253b2688ff84f1e500f191ef76b896634fb4 Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Fri, 7 Aug 2026 16:10:28 +0300 Subject: [PATCH 07/13] feat(cli): include toolchain and platform in --version output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 'dev () built on ' answered which build but not on what — the Go version and os/arch are the first questions of any bug report about a downloaded release. The version stays the first whitespace token, which is the contract selfupdate's rollback parses for its label; the suffix rides after the date. Pinned by the root-command test. --- cmd/root.go | 10 +++++++++- cmd/root_test.go | 13 ++++++++++--- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index 2d987fbd..1d374fae 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "os/signal" + "runtime" "runtime/debug" "syscall" @@ -123,7 +124,14 @@ Every command runs interactively by default (wizards, confirmations) and non-interactively with flags for CI and automation. Cloud creates show a full terraform plan (and an infracost estimate, when installed) before anything is applied; deletes require typed confirmation and clean up after themselves.`, - Version: fmt.Sprintf("%s (%s) built on %s", versionInfo.Version, versionInfo.Commit, versionInfo.Date), + // The version MUST stay the first whitespace token: selfupdate's + // rollback labels the saved binary by parsing `--version` output that + // way (binaryVersion in internal/shared/selfupdate). The toolchain and + // platform ride along because they are the first questions of any bug + // report about a downloaded release. + Version: fmt.Sprintf("%s (%s) built on %s — %s %s/%s", + versionInfo.Version, versionInfo.Commit, versionInfo.Date, + runtime.Version(), runtime.GOOS, runtime.GOARCH), // Silence errors and usage globally - we handle our own error display SilenceErrors: true, SilenceUsage: true, diff --git a/cmd/root_test.go b/cmd/root_test.go index e0285721..acec8125 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -5,6 +5,7 @@ import ( "io" "os" "path/filepath" + "runtime" "runtime/debug" "strings" "testing" @@ -111,9 +112,15 @@ func TestGetRootCmd(t *testing.T) { t.Error("Short description should not be empty") } - expectedVersion := "test-version (test-commit) built on test-date" - if cmd.Version != expectedVersion { - t.Errorf("expected version %q, got %q", expectedVersion, cmd.Version) + // The version must stay the FIRST whitespace token — selfupdate's rollback + // labels the saved binary by parsing `--version` output that way — followed + // by the commit/date and the toolchain/platform suffix. + expectedPrefix := "test-version (test-commit) built on test-date — " + if !strings.HasPrefix(cmd.Version, expectedPrefix) { + t.Errorf("expected version to start with %q, got %q", expectedPrefix, cmd.Version) + } + if !strings.Contains(cmd.Version, runtime.GOOS+"/"+runtime.GOARCH) { + t.Errorf("expected version to name the platform, got %q", cmd.Version) } } From 102ee0fb2024cb9970929aaed4cc0d3985e0b923 Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Fri, 7 Aug 2026 16:13:58 +0300 Subject: [PATCH 08/13] feat(cli): list pinned dependency versions in --version output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --version now answers 'which terraform/helm/k3d/mkcert/infracost does this build install, and which ArgoCD chart does it deploy' — straight from the single sources of truth (the PinnedTool definitions and the ArgoCD chart constant), so the block can never drift from what the installers actually pin. The version string keeps its first-token contract for selfupdate's rollback label parsing. --- cmd/root.go | 32 +++++++++++++++++++++++++++++--- cmd/root_test.go | 15 +++++++++++++++ 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index 1d374fae..f447bf7e 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -7,6 +7,7 @@ import ( "os/signal" "runtime" "runtime/debug" + "strings" "syscall" "github.com/flamingo-stack/openframe-cli/cmd/app" @@ -14,6 +15,7 @@ import ( "github.com/flamingo-stack/openframe-cli/cmd/cluster" "github.com/flamingo-stack/openframe-cli/cmd/prerequisites" "github.com/flamingo-stack/openframe-cli/cmd/update" + "github.com/flamingo-stack/openframe-cli/internal/chart/providers/argocd" "github.com/flamingo-stack/openframe-cli/internal/shared/config" "github.com/flamingo-stack/openframe-cli/internal/shared/download" "github.com/flamingo-stack/openframe-cli/internal/shared/selfupdate" @@ -97,6 +99,27 @@ func GetRootCmd(versionInfo VersionInfo) *cobra.Command { return buildRootCommand(versionInfo) } +// pinnedDependencies renders the versions this build installs (verified, +// checksum-pinned downloads) and deploys — so `--version` answers not just +// "which CLI" but "which terraform/helm/argocd comes with it". Sources: the +// PinnedTool definitions in internal/shared/download and the ArgoCD chart pin +// in internal/chart/providers/argocd. +func pinnedDependencies() string { + var b strings.Builder + b.WriteString("Pinned dependencies (installed verified at exactly these versions):\n") + for _, dep := range []struct{ name, version string }{ + {"terraform", download.Terraform.Version}, + {"helm", download.Helm.Version}, + {"k3d", download.K3d.Version}, + {"mkcert", download.Mkcert.Version}, + {"infracost", download.Infracost.Version + " (optional, cost estimates)"}, + {"argo-cd", "chart " + argocd.ArgoCDChartVersion}, + } { + fmt.Fprintf(&b, " %-10s %s\n", dep.name, dep.version) + } + return strings.TrimRight(b.String(), "\n") +} + // buildRootCommand constructs the root command with given version info func buildRootCommand(versionInfo VersionInfo) *cobra.Command { rootCmd := &cobra.Command{ @@ -128,10 +151,13 @@ applied; deletes require typed confirmation and clean up after themselves.`, // rollback labels the saved binary by parsing `--version` output that // way (binaryVersion in internal/shared/selfupdate). The toolchain and // platform ride along because they are the first questions of any bug - // report about a downloaded release. - Version: fmt.Sprintf("%s (%s) built on %s — %s %s/%s", + // report about a downloaded release; the pinned-dependency block below + // them answers the second ("which terraform/helm/argocd does this build + // install?") without digging through the source. + Version: fmt.Sprintf("%s (%s) built on %s — %s %s/%s\n\n%s", versionInfo.Version, versionInfo.Commit, versionInfo.Date, - runtime.Version(), runtime.GOOS, runtime.GOARCH), + runtime.Version(), runtime.GOOS, runtime.GOARCH, + pinnedDependencies()), // Silence errors and usage globally - we handle our own error display SilenceErrors: true, SilenceUsage: true, diff --git a/cmd/root_test.go b/cmd/root_test.go index acec8125..cbe7c3a0 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -12,7 +12,9 @@ import ( "github.com/pterm/pterm" + "github.com/flamingo-stack/openframe-cli/internal/chart/providers/argocd" "github.com/flamingo-stack/openframe-cli/internal/shared/config" + "github.com/flamingo-stack/openframe-cli/internal/shared/download" "github.com/flamingo-stack/openframe-cli/internal/shared/ui" "github.com/flamingo-stack/openframe-cli/tests/testutil" ) @@ -122,6 +124,19 @@ func TestGetRootCmd(t *testing.T) { if !strings.Contains(cmd.Version, runtime.GOOS+"/"+runtime.GOARCH) { t.Errorf("expected version to name the platform, got %q", cmd.Version) } + // The pinned-dependency block: --version must answer "which + // terraform/helm/argocd does this build install" from the single sources + // of truth (download pins, argocd chart pin), never hardcoded copies. + for _, dep := range []string{ + "terraform " + download.Terraform.Version, + "helm " + download.Helm.Version, + "k3d " + download.K3d.Version, + "argo-cd chart " + argocd.ArgoCDChartVersion, + } { + if !strings.Contains(cmd.Version, dep) { + t.Errorf("expected version output to list pinned dependency %q, got:\n%s", dep, cmd.Version) + } + } } func TestSystemService(t *testing.T) { From 3a59a07378a7ffe956931a8ead68c82baa9bb655 Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Fri, 7 Aug 2026 16:34:15 +0300 Subject: [PATCH 09/13] chore: drop verification-report index references from code comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comments explained themselves; the M/S index tags pointed at an external report the next reader does not have. The prose keeps the why — the tags go. --- internal/cluster/models/cluster.go | 4 ++-- internal/cluster/providers/gke/provider.go | 8 ++++---- internal/cluster/providers/gke/template_guard_test.go | 4 ++-- internal/cluster/providers/terraform/engine.go | 6 +++--- internal/cluster/providers/terraform/engine_test.go | 2 +- internal/cluster/ui/prompts.go | 4 ++-- 6 files changed, 14 insertions(+), 14 deletions(-) diff --git a/internal/cluster/models/cluster.go b/internal/cluster/models/cluster.go index 9e919c12..fcc620ac 100644 --- a/internal/cluster/models/cluster.go +++ b/internal/cluster/models/cluster.go @@ -51,8 +51,8 @@ type ClusterConfig struct { // GKERegionalZones is how many zones GKE spreads a regional cluster across by // default (the module passes no explicit node locations). A regional node // pool's node count is PER ZONE, so every display of an HA cluster's nodes -// must show the ×zones math — "Nodes: 3" that silently provisions 9 was a -// verification-report finding (S2). +// must show the ×zones math — "Nodes: 3" silently provisioning 9 is exactly +// the surprise this constant exists to prevent. const GKERegionalZones = 3 // CloudConfig holds the provider-agnostic knobs for a managed cloud cluster. diff --git a/internal/cluster/providers/gke/provider.go b/internal/cluster/providers/gke/provider.go index be2f3c86..cef5fd07 100644 --- a/internal/cluster/providers/gke/provider.go +++ b/internal/cluster/providers/gke/provider.go @@ -104,9 +104,9 @@ func (p *Provider) preflightCredentials(ctx context.Context, project string) err // requiredServices are the project APIs a GKE cluster needs active. They used // to be google_project_service resources inside EVERY cluster workspace, so // two clusters in one shared project claimed ownership of the same -// project-level toggles and every destroy planned their removal (report M5). -// Enabling is now this create-time step: idempotent, and owned by no cluster's -// terraform state. +// project-level toggles and every destroy planned their removal. Enabling is +// now this create-time step: idempotent, and owned by no cluster's terraform +// state. var requiredServices = []string{"compute.googleapis.com", "container.googleapis.com"} // ensureProjectServices enables the required project APIs before terraform @@ -566,7 +566,7 @@ func kubeContextFor(rec tfengine.Record) string { func infoFor(rec tfengine.Record) models.ClusterInfo { kubeContext := kubeContextFor(rec) // A regional cluster's recorded count is per zone; list/status must show - // the real node total, not the flag value that under-reports it 3× (S2). + // the real node total, not the flag value that under-reports it 3×. nodeCount := rec.NodeCount if rec.HA { nodeCount = rec.NodeCount * models.GKERegionalZones diff --git a/internal/cluster/providers/gke/template_guard_test.go b/internal/cluster/providers/gke/template_guard_test.go index 5a31dcac..dd2b6a39 100644 --- a/internal/cluster/providers/gke/template_guard_test.go +++ b/internal/cluster/providers/gke/template_guard_test.go @@ -32,11 +32,11 @@ func TestTemplate_NeverManagesTheProject(t *testing.T) { // Belt and braces: no google_project* resource of ANY kind — including // google_project_service. API enablement moved to a create-time gcloud step // (ensureProjectServices) precisely so no cluster workspace owns - // project-level state in a shared project (report M5). + // project-level state in a shared project. projectResRE := regexp.MustCompile(`resource\s+"(google_project[a-z_]*)"`) for _, m := range projectResRE.FindAllStringSubmatch(src, -1) { assert.Failf(t, "project-scoped resource in the GKE template", - "%q must not be managed per-cluster: project-level state belongs to no single cluster (M5)", m[1]) + "%q must not be managed per-cluster: project-level state belongs to no single cluster", m[1]) } } diff --git a/internal/cluster/providers/terraform/engine.go b/internal/cluster/providers/terraform/engine.go index b56be78c..4bc3a84d 100644 --- a/internal/cluster/providers/terraform/engine.go +++ b/internal/cluster/providers/terraform/engine.go @@ -135,9 +135,9 @@ func (e *Engine) Init(ctx context.Context, dir string) error { } // OpLogName is the per-workspace record of terraform apply/destroy runs. Long -// cloud operations used to leave the terminal as their only record (report -// M8); every run now appends its raw terraform JSON-UI stream here, so a -// failure hours later is still diagnosable. +// cloud operations used to leave the terminal as their only record; every +// run now appends its raw terraform JSON-UI stream here, so a failure hours +// later is still diagnosable. const OpLogName = "terraform.log" // opSinks builds the writer an apply/destroy streams into: the progress diff --git a/internal/cluster/providers/terraform/engine_test.go b/internal/cluster/providers/terraform/engine_test.go index 6f7a34fe..0e951a59 100644 --- a/internal/cluster/providers/terraform/engine_test.go +++ b/internal/cluster/providers/terraform/engine_test.go @@ -260,7 +260,7 @@ func TestNewEngine_VerboseWrapsRunnerForSelectiveStdout(t *testing.T) { "non-verbose needs no wrapper — stdout is never streamed") } -// TestEngine_ApplyWritesOpLog (report M8): a long cloud operation must leave a +// TestEngine_ApplyWritesOpLog: a long cloud operation must leave a // file record, not just terminal scrollback. Apply tees terraform's raw // JSON-UI stream into the workspace's terraform.log, and a failure names the // log path. diff --git a/internal/cluster/ui/prompts.go b/internal/cluster/ui/prompts.go index 40a5756e..71029be7 100644 --- a/internal/cluster/ui/prompts.go +++ b/internal/cluster/ui/prompts.go @@ -86,8 +86,8 @@ func CostHint(clusterType models.ClusterType) string { // NodesLine renders a config's node count honestly: a regional (--ha) cluster // provisions its count PER ZONE, so "3" would silently mean 9 nodes and ~3× -// the expected bill (verification-report finding S2 — the summary said 3, GCP -// came up with 9). Zonal clusters keep the plain number. +// the expected bill — a verification pass hit exactly that (the summary said +// 3, GCP came up with 9). Zonal clusters keep the plain number. func NodesLine(config models.ClusterConfig) string { if config.Cloud == nil || !config.Cloud.HA { return strconv.Itoa(config.NodeCount) From f54a7d5fc21f9dcbb0e404428904cd777b8379b4 Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Fri, 7 Aug 2026 16:38:42 +0300 Subject: [PATCH 10/13] chore(lint): annotate the op-log open for gosec G304 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The path is a CLI-managed workspace directory — the same trust boundary as the terraform state written next to it — joined with a constant filename. --- internal/cluster/providers/terraform/engine.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/cluster/providers/terraform/engine.go b/internal/cluster/providers/terraform/engine.go index 4bc3a84d..abc62eb6 100644 --- a/internal/cluster/providers/terraform/engine.go +++ b/internal/cluster/providers/terraform/engine.go @@ -148,7 +148,7 @@ const OpLogName = "terraform.log" // being written. func (e *Engine) opSinks(dir, op string) (w io.Writer, close func(), logPath string) { progress := newProgressWriter(e.verbose) - f, err := os.OpenFile(filepath.Join(dir, OpLogName), os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0o600) + f, err := os.OpenFile(filepath.Join(dir, OpLogName), os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0o600) // #nosec G304 -- dir is a CLI-managed workspace directory (same trust as the terraform state written next to it), joined with a constant filename if err != nil { return progress, func() {}, "" } From 651874ca6ee1c9ee8829ca4000980d1b50f7fa8e Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Fri, 7 Aug 2026 16:47:34 +0300 Subject: [PATCH 11/13] fix(ux): tighten help/hint wording flagged in review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - cleanup help: 'Only images no container references' was missing its relative pronoun — now 'Only images that no container references are removed', split into plain sentences - cleanup partial-failure hint: stop suggesting a --force re-run; the flag only skips the confirmation prompt, so it implied a more aggressive retry that doesn't exist - root help: cloud deletes 'clean up after themselves' overstated it — they destroy the terraform-managed resources and report leftovers they could not remove - root version test: also pin the Go toolchain, mkcert and infracost lines, so dropping any advertised field fails the test --- cmd/cluster/cleanup.go | 6 +++--- cmd/root.go | 3 ++- cmd/root_test.go | 5 +++++ internal/cluster/ui/operations.go | 4 +++- 4 files changed, 13 insertions(+), 5 deletions(-) diff --git a/cmd/cluster/cleanup.go b/cmd/cluster/cleanup.go index fe884233..e7bdd1c5 100644 --- a/cmd/cluster/cleanup.go +++ b/cmd/cluster/cleanup.go @@ -19,9 +19,9 @@ func getCleanupCmd() *cobra.Command { Short: "Prune unused container images from cluster nodes", Long: `Reclaim disk space by pruning unused container images inside each cluster node. -Only images no container references are removed. Installed applications, Helm -releases and namespaces are never touched — to remove the OpenFrame platform -use 'openframe app uninstall', to remove the whole cluster use +Only images that no container references are removed. Installed applications, +Helm releases and namespaces are never touched. To remove the OpenFrame +platform use 'openframe app uninstall'; to remove the whole cluster use 'openframe cluster delete'. Examples: diff --git a/cmd/root.go b/cmd/root.go index f447bf7e..821255ef 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -146,7 +146,8 @@ Command groups: Every command runs interactively by default (wizards, confirmations) and non-interactively with flags for CI and automation. Cloud creates show a full terraform plan (and an infracost estimate, when installed) before anything is -applied; deletes require typed confirmation and clean up after themselves.`, +applied; cloud deletes require typed confirmation, destroy the +terraform-managed resources, and report any leftovers they could not remove.`, // The version MUST stay the first whitespace token: selfupdate's // rollback labels the saved binary by parsing `--version` output that // way (binaryVersion in internal/shared/selfupdate). The toolchain and diff --git a/cmd/root_test.go b/cmd/root_test.go index cbe7c3a0..4a22fed6 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -127,10 +127,15 @@ func TestGetRootCmd(t *testing.T) { // The pinned-dependency block: --version must answer "which // terraform/helm/argocd does this build install" from the single sources // of truth (download pins, argocd chart pin), never hardcoded copies. + if !strings.Contains(cmd.Version, runtime.Version()) { + t.Errorf("expected version to name the Go toolchain, got %q", cmd.Version) + } for _, dep := range []string{ "terraform " + download.Terraform.Version, "helm " + download.Helm.Version, "k3d " + download.K3d.Version, + "mkcert " + download.Mkcert.Version, + "infracost " + download.Infracost.Version, "argo-cd chart " + argocd.ArgoCDChartVersion, } { if !strings.Contains(cmd.Version, dep) { diff --git a/internal/cluster/ui/operations.go b/internal/cluster/ui/operations.go index 77baeba2..831f9c04 100644 --- a/internal/cluster/ui/operations.go +++ b/internal/cluster/ui/operations.go @@ -255,7 +255,9 @@ func (ui *OperationsUI) ShowCleanupSummary(clusterName string, result models.Cle for _, f := range result.Failures { pterm.DefaultBasicText.Printf(" • %s\n", f) } - pterm.Info.Printf("Re-run with --force, or delete the cluster: openframe cluster delete %s\n", clusterName) + // A plain re-run hint: --force only skips the confirmation prompt, so + // suggesting it here would imply a more aggressive retry that doesn't exist. + pterm.Info.Printf("Re-run the cleanup, or delete the cluster: openframe cluster delete %s\n", clusterName) } } From 611f7aa09b75f112b2b543fc7475ae6774769b50 Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Fri, 7 Aug 2026 16:50:29 +0300 Subject: [PATCH 12/13] fix(terraform): never let a terraform.log write failure fail the run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit io.MultiWriter propagates a sink error, and exec.Cmd returns a stdout-writer error from Wait — so a disk filling up after the log file opened would report a completed apply/destroy as FAILED while terraform had actually changed resources. The log stream now goes through a best-effort tee: progress output is authoritative, the file sink is dropped on its first write failure and never re-enabled. The log records the operation; it must not decide its outcome. --- .../cluster/providers/terraform/engine.go | 27 +++++++++++++++-- .../providers/terraform/engine_test.go | 29 +++++++++++++++++++ 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/internal/cluster/providers/terraform/engine.go b/internal/cluster/providers/terraform/engine.go index abc62eb6..73735189 100644 --- a/internal/cluster/providers/terraform/engine.go +++ b/internal/cluster/providers/terraform/engine.go @@ -140,10 +140,31 @@ func (e *Engine) Init(ctx context.Context, dir string) error { // later is still diagnosable. const OpLogName = "terraform.log" +// bestEffortTee mirrors every chunk into the log sink while it works and +// drops the sink on its first write failure. It exists because io.MultiWriter +// propagates a sink error: exec.Cmd returns a stdout-writer error from Wait, +// so a disk filling up mid-apply would report the terraform run as FAILED +// while terraform actually completed and changed resources. The log is a +// record of the operation — it must never decide its outcome. +type bestEffortTee struct { + progress io.Writer + sink io.Writer // nil once a write failed; never re-enabled +} + +func (t *bestEffortTee) Write(p []byte) (int, error) { + if t.sink != nil { + if _, err := t.sink.Write(p); err != nil { + t.sink = nil + } + } + return t.progress.Write(p) +} + // opSinks builds the writer an apply/destroy streams into: the progress // writer, teed into dir's terraform.log when it can be opened. Logging is -// best-effort — a directory that cannot take the log (read-only, gone) must -// never block the operation, so the fallback is the progress writer alone. +// best-effort end to end — a directory that cannot take the log (read-only, +// gone) skips it, and a write failure after opening only stops the mirroring +// (bestEffortTee above); neither may ever fail the operation itself. // The returned close is always safe to call; logPath is empty when no log is // being written. func (e *Engine) opSinks(dir, op string) (w io.Writer, close func(), logPath string) { @@ -153,7 +174,7 @@ func (e *Engine) opSinks(dir, op string) (w io.Writer, close func(), logPath str return progress, func() {}, "" } fmt.Fprintf(f, "=== terraform %s — %s ===\n", op, time.Now().UTC().Format(time.RFC3339)) - return io.MultiWriter(progress, f), func() { _ = f.Close() }, f.Name() + return &bestEffortTee{progress: progress, sink: f}, func() { _ = f.Close() }, f.Name() } // opFailure wraps a failed apply/destroy, pointing at the full log when one diff --git a/internal/cluster/providers/terraform/engine_test.go b/internal/cluster/providers/terraform/engine_test.go index 0e951a59..693c614c 100644 --- a/internal/cluster/providers/terraform/engine_test.go +++ b/internal/cluster/providers/terraform/engine_test.go @@ -1,6 +1,7 @@ package terraform import ( + "bytes" "context" "encoding/json" "errors" @@ -295,3 +296,31 @@ func TestEngine_ApplyFailureNamesTheLog(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), OpLogName, "the failure must name the log file") } + +// failingWriter errors on every write, simulating a log file on a filesystem +// that filled up or vanished after the file was opened. +type failingWriter struct{ writes int } + +func (f *failingWriter) Write(p []byte) (int, error) { + f.writes++ + return 0, errors.New("no space left on device") +} + +// TestBestEffortTee_LogFailureNeverFailsTheRun: a terraform.log write error +// must not propagate — exec.Cmd would surface it from Wait and report a +// completed apply as failed. The sink is dropped on first failure; progress +// keeps flowing. +func TestBestEffortTee_LogFailureNeverFailsTheRun(t *testing.T) { + var progress bytes.Buffer + sink := &failingWriter{} + tee := &bestEffortTee{progress: &progress, sink: sink} + + n, err := tee.Write([]byte("line 1\n")) + require.NoError(t, err, "a log-sink failure must not surface from the tee") + assert.Equal(t, len("line 1\n"), n) + + _, err = tee.Write([]byte("line 2\n")) + require.NoError(t, err) + assert.Equal(t, 1, sink.writes, "the sink is dropped after its first failure, not retried") + assert.Equal(t, "line 1\nline 2\n", progress.String(), "progress output continues past the log failure") +} From bc0af6767ffc43a62038b732e3cc9cb28864f25c Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Fri, 7 Aug 2026 16:56:41 +0300 Subject: [PATCH 13/13] ci: parse only the first line of --version in the update/rollback test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --version output grew a pinned-dependencies block, so it is multi-line now. The test parsed it with a bare 'cut -d" " -f1', which cuts EVERY line and made the comparison fail on all platforms. The version contract is unchanged — first token of the first line (same as selfupdate's rollback label parser) — so the checks now take head -n1 before cut. The release smoke test greps the whole output and needed no change. --- .github/workflows/test.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 9011ff7b..8b648ffd 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -325,17 +325,17 @@ jobs: WORK="$(mktemp -d)"; export HOME="$WORK/home"; mkdir -p "$HOME" go build -ldflags "-X github.com/flamingo-stack/openframe-cli/cmd.version=0.0.1" -o "$WORK/openframe" . OF="$WORK/openframe" - [ "$("$OF" --version | cut -d' ' -f1)" = "0.0.1" ] || { echo "::error::ldflags version injection broken"; exit 1; } + [ "$("$OF" --version | head -n1 | cut -d' ' -f1)" = "0.0.1" ] || { echo "::error::ldflags version injection broken"; exit 1; } echo "--- update to the latest release (verifies the cosign bundle)" "$OF" update --yes - got="$("$OF" --version | cut -d' ' -f1)" + got="$("$OF" --version | head -n1 | cut -d' ' -f1)" [ "$got" = "$LATEST" ] || { echo "::error::after update --version is $got, want $LATEST"; exit 1; } echo "updated 0.0.1 -> $got" echo "--- rollback restores the previous binary (offline)" "$OF" update rollback --yes - back="$("$OF" --version | cut -d' ' -f1)" + back="$("$OF" --version | head -n1 | cut -d' ' -f1)" [ "$back" = "0.0.1" ] || { echo "::error::after rollback --version is $back, want 0.0.1"; exit 1; } echo "--- rollback again: nothing left to restore, clean exit" @@ -346,7 +346,7 @@ jobs: W2="$(mktemp -d)"; HOME="$W2/home"; mkdir -p "$HOME" go build -ldflags "-X github.com/flamingo-stack/openframe-cli/cmd.version=0.0.1" -o "$W2/openframe" . HOME="$W2/home" "$W2/openframe" update "$spelling" --yes - v="$(HOME="$W2/home" "$W2/openframe" --version | cut -d' ' -f1)" + v="$(HOME="$W2/home" "$W2/openframe" --version | head -n1 | cut -d' ' -f1)" [ "$v" = "$LATEST" ] || { echo "::error::update $spelling landed on $v, want $LATEST"; exit 1; } echo "OK: update $spelling -> $v" done