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 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/cmd/cluster/cleanup.go b/cmd/cluster/cleanup.go index d7c637cc..e7bdd1c5 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 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: 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/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/cmd/root.go b/cmd/root.go index 6020a208..821255ef 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -5,7 +5,9 @@ import ( "fmt" "os" "os/signal" + "runtime" "runtime/debug" + "strings" "syscall" "github.com/flamingo-stack/openframe-cli/cmd/app" @@ -13,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" @@ -96,26 +99,66 @@ 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{ 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.`, - Version: fmt.Sprintf("%s (%s) built on %s", versionInfo.Version, versionInfo.Commit, versionInfo.Date), + 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; 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 + // platform ride along because they are the first questions of any bug + // 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, + 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 c32510de..4a22fed6 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -5,13 +5,16 @@ import ( "io" "os" "path/filepath" + "runtime" "runtime/debug" "strings" "testing" "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" ) @@ -31,7 +34,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) } @@ -111,9 +114,33 @@ 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) + } + // 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) { + t.Errorf("expected version output to list pinned dependency %q, got:\n%s", dep, cmd.Version) + } } } diff --git a/docs/architecture/decisions.md b/docs/architecture/decisions.md index 7e417584..01ade042 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 @@ -136,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/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/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:** 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/cluster.go b/internal/cluster/models/cluster.go index 80e2cc46..fcc620ac 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" 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. type CloudConfig struct { Region string `json:"region"` 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/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..cef5fd07 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. 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Γ—. + 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..dd2b6a39 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. 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", 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/engine.go b/internal/cluster/providers/terraform/engine.go index c07c44e3..73735189 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,58 @@ 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; every +// run now appends its raw terraform JSON-UI stream here, so a failure hours +// 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 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) { + progress := newProgressWriter(e.verbose) + 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() {}, "" + } + fmt.Fprintf(f, "=== terraform %s β€” %s ===\n", op, time.Now().UTC().Format(time.RFC3339)) + return &bestEffortTee{progress: progress, sink: 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 +195,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 +218,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 +330,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..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" @@ -259,3 +260,67 @@ func TestNewEngine_VerboseWrapsRunnerForSelectiveStdout(t *testing.T) { assert.IsType(t, &tfexec.Terraform{}, quiet, "non-verbose needs no wrapper β€” stdout is never streamed") } + +// 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. +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") +} + +// 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") +} 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/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..831f9c04 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() { @@ -267,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) } } @@ -279,6 +269,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( @@ -289,7 +290,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. @@ -336,7 +337,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) @@ -349,7 +350,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/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") diff --git a/internal/cluster/ui/prompts.go b/internal/cluster/ui/prompts.go index 9f999fcb..71029be7 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 β€” 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) + } + 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