From de448c7ee61881980e42d173da68d68cd6350601 Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Thu, 6 Aug 2026 05:36:27 +0300 Subject: [PATCH 1/8] fix(eks): make the documented create flow produce a working cluster; surface real terraform errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The root cause of three broken-create failure modes was one floating module range: '~> 21.0' resolved to v21.24.1, whose changed defaults broke create in three independent ways. Template: - pin terraform-aws-modules/eks to 21.24.1 and /vpc to 6.6.1 exactly; guard test rejects ranges - default kubernetes_version to a concrete 1.33 — module v21 cannot plan with a null version (Invalid count argument on the documented no-flag create) - declare vpc-cni (before_compute), kube-proxy, coredns: module v21 sets bootstrap_self_managed_addons=false, so an undeclared CNI meant permanently NotReady nodes - set IMDS hop limit 2 on the node group: the v21 default of 1 cut the (non-hostNetwork) EBS CSI controller off from the node role - enable the CSI addon's defaultStorageClass: since EKS 1.30 AWS ships no default class, so every PVC stayed Pending - default instance type m7i-flex.large (Free-Tier-eligible drop-in for m6i.large): new AWS accounts refuse non-eligible types after a ~30-minute opaque ASG wait Error surfacing: - splitCause kept only the deepest error ('exit status 1') and discarded the terraform stderr appended after it — the actual failure reason now reaches the default, non-verbose panel - verbose mode no longer tees terraform's machine-readable output (the 641 KB 'show -json' line) into the terminal; init/plan human output still streams via a selective wrapper Commands and docs: - app status/access/uninstall now reject a stray positional cluster name instead of silently running against the current kube-context - --ref examples match the chart repo's real tag scheme; machine-type defaults and the EKS out-of-the-box description updated --- cmd/app/.install.md | 2 +- cmd/app/access.go | 1 + cmd/app/app.go | 15 ++++ cmd/app/install.go | 7 +- cmd/app/status.go | 1 + cmd/app/status_access_test.go | 27 +++++++ cmd/app/uninstall.go | 1 + docs/getting-started/cloud-clusters.md | 10 ++- docs/reference/architecture/overview.md | 2 +- internal/cluster/models/flags.go | 2 +- .../cluster/providers/eks/provider_test.go | 2 - internal/cluster/providers/eks/template.go | 5 +- .../providers/eks/template_guard_test.go | 57 ++++++++++++++ .../cluster/providers/eks/templates/main.tf | 75 +++++++++++++++---- .../cluster/providers/terraform/engine.go | 42 ++++++++++- .../providers/terraform/engine_test.go | 24 ++++++ internal/cluster/ui/wizard.go | 4 +- internal/shared/errors/errors.go | 9 ++- internal/shared/errors/errors_test.go | 25 +++++++ 19 files changed, 283 insertions(+), 28 deletions(-) diff --git a/cmd/app/.install.md b/cmd/app/.install.md index fbe9a8ca..7ea36de6 100644 --- a/cmd/app/.install.md +++ b/cmd/app/.install.md @@ -36,7 +36,7 @@ openframe app install --non-interactive # Deploy a specific branch or release tag openframe app install --ref develop -openframe app install --ref v1.2.3 +openframe app install --ref 1.0.48 # Target an explicit kube-context (scriptable) openframe app install --context my-context diff --git a/cmd/app/access.go b/cmd/app/access.go index 5bc146bc..2b715491 100644 --- a/cmd/app/access.go +++ b/cmd/app/access.go @@ -25,6 +25,7 @@ password, and the command to open the ArgoCD UI locally. Examples: openframe app access openframe app access --context k3d-openframe-dev`, + Args: noPositionalArgs, RunE: runAccessCommand, Annotations: map[string]string{"readonly": "true"}, } diff --git a/cmd/app/app.go b/cmd/app/app.go index 8f5d4c88..afa22874 100644 --- a/cmd/app/app.go +++ b/cmd/app/app.go @@ -1,10 +1,25 @@ package app import ( + "fmt" + "strings" + "github.com/flamingo-stack/openframe-cli/internal/shared/ui" "github.com/spf13/cobra" ) +// noPositionalArgs rejects stray positional arguments loudly, naming the flag +// that does the targeting. Without it cobra defaults to ArbitraryArgs, and +// `openframe app status my-eks` silently ran against whatever the current +// kube-context happened to be — for uninstall, destructively so. +func noPositionalArgs(cmd *cobra.Command, args []string) error { + if len(args) > 0 { + return fmt.Errorf("%q takes no positional arguments (got %q) — use --context to target a cluster", + cmd.CommandPath(), strings.Join(args, " ")) + } + return nil +} + // GetAppCmd returns the app command and its subcommands. func GetAppCmd() *cobra.Command { cmd := &cobra.Command{ diff --git a/cmd/app/install.go b/cmd/app/install.go index 37a59a31..e819f988 100644 --- a/cmd/app/install.go +++ b/cmd/app/install.go @@ -35,7 +35,7 @@ Examples: openframe app install my-cluster # Install on specific cluster openframe app install --non-interactive # Use existing openframe-helm-values.yaml (CI/CD) openframe app install --ref develop # Deploy a branch - openframe app install --ref v1.2.3 # Deploy a release tag`, argocd.ArgoCDChartVersion), + openframe app install --ref 1.0.48 # Deploy a release tag`, argocd.ArgoCDChartVersion), RunE: runInstallCommand, SilenceErrors: true, // Errors are handled by our custom error handler SilenceUsage: true, // Don't show usage on errors @@ -218,7 +218,10 @@ func addInstallFlags(cmd *cobra.Command) { cmd.Flags().BoolP("force", "f", false, "Force installation even if charts already exist") cmd.Flags().Bool("dry-run", false, "Show what would be installed without executing") cmd.Flags().String("github-repo", chartmodels.RepoOSSTenant, "GitHub repository URL") - cmd.Flags().StringP("ref", "r", "", "Git ref (branch or release tag, e.g. v1.2.3) to deploy") + // The example must match the chart repository's actual tag scheme (1.0.48, + // no v prefix) — a help text advertising v1.2.3 produced exactly the failing + // `--ref v…` invocations it seemed to endorse. + cmd.Flags().StringP("ref", "r", "", "Git ref (branch or release tag, e.g. 1.0.48) to deploy") cmd.Flags().String("cert-dir", "", "Certificate directory (auto-detected if not provided)") cmd.Flags().Bool("non-interactive", false, "Skip all prompts, use existing openframe-helm-values.yaml") cmd.Flags().StringP("context", "c", "", "Kube-context to install into (skips interactive selection)") diff --git a/cmd/app/status.go b/cmd/app/status.go index e6a42053..f907b463 100644 --- a/cmd/app/status.go +++ b/cmd/app/status.go @@ -28,6 +28,7 @@ sync/health, summarizes overall readiness, and prints how to sign in. Examples: openframe app status openframe app status --context k3d-openframe-dev`, + Args: noPositionalArgs, RunE: runStatusCommand, Annotations: map[string]string{"readonly": "true"}, } diff --git a/cmd/app/status_access_test.go b/cmd/app/status_access_test.go index e228050b..cf54245c 100644 --- a/cmd/app/status_access_test.go +++ b/cmd/app/status_access_test.go @@ -139,3 +139,30 @@ func TestStatusCommand_WatchRejectsMachineOutput(t *testing.T) { t.Fatalf("expected the watch/output conflict error, got %v", err) } } + +// A stray positional argument must fail loudly, not be silently discarded: +// `openframe app status my-eks` used to run against whatever the current +// kube-context was — masked in the linear create→install flow, wrong (and for +// uninstall destructive) the moment the context had moved elsewhere. +func TestContextTargetedCommands_RejectPositionalArgs(t *testing.T) { + for _, build := range []func() *cobra.Command{getStatusCmd, getAccessCmd, getUninstallCmd} { + cmd := build() + t.Run(cmd.Name(), func(t *testing.T) { + if cmd.Args == nil { + t.Fatal("Args validator missing — cobra defaults to ArbitraryArgs and a cluster name is silently ignored") + } + err := cmd.Args(cmd, []string{"my-eks"}) + if err == nil { + t.Fatal("a positional cluster name must be rejected") + } + for _, want := range []string{"my-eks", "--context"} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("error %q must mention %q", err.Error(), want) + } + } + if err := cmd.Args(cmd, nil); err != nil { + t.Fatalf("no positional args must stay valid, got %v", err) + } + }) + } +} diff --git a/cmd/app/uninstall.go b/cmd/app/uninstall.go index 8247a9b6..a368366f 100644 --- a/cmd/app/uninstall.go +++ b/cmd/app/uninstall.go @@ -27,6 +27,7 @@ Examples: openframe app uninstall openframe app uninstall --context k3d-openframe-dev openframe app uninstall --yes --delete-namespace`, + Args: noPositionalArgs, RunE: runUninstallCommand, } cmd.Flags().StringP("context", "c", "", "Kube-context to use (defaults to the current context)") diff --git a/docs/getting-started/cloud-clusters.md b/docs/getting-started/cloud-clusters.md index 089e07d5..2d07c0c6 100644 --- a/docs/getting-started/cloud-clusters.md +++ b/docs/getting-started/cloud-clusters.md @@ -84,8 +84,14 @@ minutes; the CLI streams per-resource progress. GKE nodes are private (no external IPs, egress via Cloud NAT) with a public control-plane endpoint, so the flow works in organizations enforcing `restrict_vm_external_ips`. EKS clusters get a dedicated VPC (2 AZs, nodes in private subnets behind a single -NAT gateway) and the `aws-ebs-csi-driver` addon, so PersistentVolumeClaims -work out of the box. +NAT gateway), the core addons (`vpc-cni`, `kube-proxy`, `coredns`) and the +`aws-ebs-csi-driver` addon with a default gp3 StorageClass, so networking and +PersistentVolumeClaims work out of the box. The generated Terraform pins the +upstream EKS/VPC modules to exact versions — upstream default changes arrive +only with a deliberate CLI release, never mid-`create`. The default node type +is `m7i-flex.large`, which is Free-Tier-eligible: a brand-new AWS account (its +Free plan refuses non-eligible instance types) can run the documented flow +unchanged. When it finishes, your kubeconfig gets a context named after the cluster and it becomes the current context — `kubectl get nodes` just works (authentication runs through short-lived tokens via `aws eks get-token` / diff --git a/docs/reference/architecture/overview.md b/docs/reference/architecture/overview.md index c9d7343d..fab7600b 100644 --- a/docs/reference/architecture/overview.md +++ b/docs/reference/architecture/overview.md @@ -412,7 +412,7 @@ openframe cluster create --type k3d --nodes 1 --skip-wizard openframe app install # Interactive context picker openframe app install -c k3d-openframe-dev # Explicit context openframe app install --non-interactive # CI (reuse existing values file) -openframe app install --ref v1.2.3 # Deploy specific tag +openframe app install --ref 1.0.48 # Deploy specific tag openframe app install --dry-run # Preview only ``` diff --git a/internal/cluster/models/flags.go b/internal/cluster/models/flags.go index 6c6fb82a..893af03f 100644 --- a/internal/cluster/models/flags.go +++ b/internal/cluster/models/flags.go @@ -77,7 +77,7 @@ func AddCreateFlags(cmd *cobra.Command, flags *CreateFlags) { cmd.Flags().StringVar(&flags.Region, "region", "", "Cloud region (required for cloud types)") cmd.Flags().StringVar(&flags.Profile, "profile", "", "AWS credentials profile (eks only)") cmd.Flags().StringVar(&flags.Project, "project", "", "GCP project (required for --type gke)") - cmd.Flags().StringVar(&flags.MachineType, "machine-type", "", "Node instance type (cloud only; defaults: m6i.large on eks, e2-standard-4 on gke)") + cmd.Flags().StringVar(&flags.MachineType, "machine-type", "", "Node instance type (cloud only; defaults: m7i-flex.large on eks, e2-standard-4 on gke)") cmd.Flags().IntVar(&flags.MinNodes, "min-nodes", 0, "Node group minimum size (cloud only, at least 1; default 1)") cmd.Flags().IntVar(&flags.MaxNodes, "max-nodes", 0, "Node group maximum size (cloud only, at least 1; default 4)") cmd.Flags().BoolVar(&flags.Spot, "spot", false, "Use spot capacity for nodes (cloud only)") diff --git a/internal/cluster/providers/eks/provider_test.go b/internal/cluster/providers/eks/provider_test.go index b4699441..3ca712b2 100644 --- a/internal/cluster/providers/eks/provider_test.go +++ b/internal/cluster/providers/eks/provider_test.go @@ -246,9 +246,7 @@ func TestTfvarsFor_VersionMapping(t *testing.T) { func TestTemplateEmbedsModulePins(t *testing.T) { tf := string(mainTF) assert.Contains(t, tf, `source = "terraform-aws-modules/eks/aws"`) - assert.Contains(t, tf, `version = "~> 21.0"`) assert.Contains(t, tf, `source = "terraform-aws-modules/vpc/aws"`) - assert.Contains(t, tf, `version = "~> 6.0"`) assert.Contains(t, tf, "enable_cluster_creator_admin_permissions = true") } diff --git a/internal/cluster/providers/eks/template.go b/internal/cluster/providers/eks/template.go index a1fa2fd1..43b819aa 100644 --- a/internal/cluster/providers/eks/template.go +++ b/internal/cluster/providers/eks/template.go @@ -38,7 +38,10 @@ func tfvarsFor(config models.ClusterConfig) (tfvars, error) { version := strings.TrimPrefix(config.K8sVersion, "v") if version == "latest" { - version = "" // template maps empty to the EKS default (its latest) + // Omitted from tfvars → the template's pinned default applies. Not the + // EKS-side "latest": module v21 cannot plan with a null version, so the + // template always carries a concrete one. + version = "" } if version != "" && !eksVersionRE.MatchString(version) { return tfvars{}, models.NewInvalidConfigError("version", config.K8sVersion, diff --git a/internal/cluster/providers/eks/template_guard_test.go b/internal/cluster/providers/eks/template_guard_test.go index a5de1f6b..ce939516 100644 --- a/internal/cluster/providers/eks/template_guard_test.go +++ b/internal/cluster/providers/eks/template_guard_test.go @@ -1,6 +1,7 @@ package eks import ( + "regexp" "strings" "testing" @@ -43,6 +44,62 @@ func TestTemplate_OperatorCanReachTheCluster(t *testing.T) { assert.Contains(t, src, "enable_cluster_creator_admin_permissions = true") } +// EKS module v21 bootstraps NO addons itself (bootstrap_self_managed_addons = +// false): everything a working cluster needs must be declared. Without vpc-cni +// no pod gets a network and nodes never become Ready; without kube-proxy no +// Service routes; without coredns nothing resolves. This guard fails the +// moment any of them — or the CNI's before_compute ordering — leaves the +// template. +func TestTemplate_DeclaresCoreAddons(t *testing.T) { + src := string(mainTF) + for _, addon := range []string{"vpc-cni", "kube-proxy", "coredns"} { + assert.Containsf(t, src, addon, + "module v21 installs no addons by itself — %s must be declared or the cluster is born broken", addon) + } + assert.Contains(t, src, "before_compute = true", + "vpc-cni must install before the node group, or nodes wait on a missing CNI") +} + +// The EBS CSI controller is a regular pod reaching the node role via IMDS — +// one hop more than host network. Module v21 defaults the hop limit to 1, +// which cuts the controller off ("no EC2 IMDS role found") and no volume ever +// binds. Separately, since EKS 1.30 AWS ships no default StorageClass, so the +// addon must create one or every PVC with no explicit class stays Pending. +func TestTemplate_CSIControllerCanActuallyProvision(t *testing.T) { + src := string(mainTF) + assert.Contains(t, src, "http_put_response_hop_limit = 2", + "IMDS hop limit must be 2 or the (non-hostNetwork) CSI controller cannot reach the node role") + assert.Contains(t, src, "defaultStorageClass", + "since EKS 1.30 there is no default StorageClass unless the CSI addon creates one") +} + +// Module versions must be exact pins. A floating "~> 21.0" resolved to a new +// minor whose changed defaults broke create in three independent ways; the +// docs also promise pinned modules. +func TestTemplate_ModuleVersionsAreExactPins(t *testing.T) { + src := string(mainTF) + moduleVersions := regexp.MustCompile( + `source\s*=\s*"terraform-aws-modules/[^"]+"\s*\n\s*version\s*=\s*"([^"]+)"`, + ).FindAllStringSubmatch(src, -1) + require.Len(t, moduleVersions, 2, "expected exactly the eks and vpc module blocks") + for _, m := range moduleVersions { + assert.Regexpf(t, `^\d+\.\d+\.\d+$`, m[1], + "module versions must be exact pins, bumped deliberately — a range (%q) lets upstream default changes arrive unannounced", m[1]) + } +} + +// The kubernetes_version default must be a concrete version: module v21 gates +// a data source on `kubernetes_version == null` and fails at plan time when +// the value is unknown ("Invalid count argument") — the documented no-flags +// create must be able to plan. +func TestTemplate_KubernetesVersionHasConcreteDefault(t *testing.T) { + src := string(mainTF) + assert.NotContains(t, src, `var.kubernetes_version != ""`, + "kubernetes_version must never map to null — module v21 cannot plan with an unknown version") + assert.Regexp(t, `variable "kubernetes_version" \{[^}]*default\s*=\s*"\d+\.\d+"`, src, + "the kubernetes_version variable must default to a concrete .") +} + // Subnets must reach the EKS module as real module.vpc references (not string // literals), so terraform keeps the destroy-ordering edge from the cluster to // the VPC — the reason the GKE template needs an explicit depends_on is exactly diff --git a/internal/cluster/providers/eks/templates/main.tf b/internal/cluster/providers/eks/templates/main.tf index 88bd38c3..a0ec309a 100644 --- a/internal/cluster/providers/eks/templates/main.tf +++ b/internal/cluster/providers/eks/templates/main.tf @@ -1,7 +1,14 @@ # Root module generated by the OpenFrame CLI (do not edit by hand — the CLI # owns this workspace). Provisions a self-contained EKS cluster: dedicated VPC -# (2 AZs, single NAT) + EKS with one managed node group and the EBS CSI driver -# addon. All inputs come from terraform.tfvars.json next to this file. +# (2 AZs, single NAT) + EKS with one managed node group, the core addons +# (vpc-cni, kube-proxy, coredns) and the EBS CSI driver addon. All inputs come +# from terraform.tfvars.json next to this file. +# +# Module versions are EXACT pins, not ranges. A floating "~> 21.0" once +# resolved to a new minor whose changed defaults broke cluster creation in +# three independent ways (no bootstrapped addons, IMDS hop limit 1, gated +# kubernetes_version data source). Bump the pins deliberately, re-running the +# full create/delete pass afterwards. terraform { required_version = ">= 1.15.0" @@ -22,15 +29,22 @@ variable "profile" { default = "" } -# Kubernetes . (e.g. "1.33"); empty means the EKS default. +# Kubernetes .. The default is a concrete version, not null: +# EKS module v21 gates a data source on `kubernetes_version == null`, which +# fails at plan time ("Invalid count argument") — the version must be known +# before apply. variable "kubernetes_version" { type = string - default = "" + default = "1.33" } +# m7i-flex.large (2 vCPU / 8 GB): the Free-Tier-eligible drop-in for +# m6i.large. New AWS accounts start on the Free plan, which refuses to launch +# non-eligible types — with m6i.large the node group dies after an ASG launch +# failure ~30 minutes in, on exactly the first-run path. variable "instance_type" { type = string - default = "m6i.large" + default = "m7i-flex.large" } variable "min_nodes" { @@ -75,7 +89,7 @@ locals { module "vpc" { source = "terraform-aws-modules/vpc/aws" - version = "~> 6.0" + version = "6.6.1" name = "${var.cluster_name}-vpc" cidr = "10.0.0.0/16" @@ -97,10 +111,10 @@ module "vpc" { module "eks" { source = "terraform-aws-modules/eks/aws" - version = "~> 21.0" + version = "21.24.1" name = var.cluster_name - kubernetes_version = var.kubernetes_version != "" ? var.kubernetes_version : null + kubernetes_version = var.kubernetes_version vpc_id = module.vpc.vpc_id subnet_ids = module.vpc.private_subnets @@ -125,21 +139,52 @@ module "eks" { iam_role_additional_policies = { ebs_csi = "arn:aws:iam::aws:policy/service-role/AmazonEBSCSIDriverPolicy" } + + # The CSI controller reaches that role via IMDS, and it is NOT on host + # network — one extra network hop. The module's v21 default hop limit of + # 1 cuts it off ("no EC2 IMDS role found", controller crash-loops, no + # volume ever binds); 2 restores pod access while keeping IMDSv2 required. + metadata_options = { + http_endpoint = "enabled" + http_tokens = "required" + http_put_response_hop_limit = 2 + } } } - # Since Kubernetes 1.23 the in-tree EBS provisioner is gone: without this - # addon every PersistentVolumeClaim stays Pending forever, so the OpenFrame - # platform (Kafka, MongoDB, Cassandra, …) could never come up. EKS installs - # coredns/kube-proxy/vpc-cni by itself; the CSI driver it does not. + # EKS module v21 sets bootstrap_self_managed_addons = false: EKS itself + # installs NOTHING, so every addon a working cluster needs must be declared + # here. Without vpc-cni no pod ever gets a network and the nodes never + # become Ready; before_compute installs it ahead of the node group so nodes + # come up working instead of waiting on a broken CNI. addons = { + vpc-cni = { + most_recent = true + before_compute = true + } + kube-proxy = { + most_recent = true + } + coredns = { + most_recent = true + } + + # Since Kubernetes 1.23 the in-tree EBS provisioner is gone: without this + # addon every PersistentVolumeClaim stays Pending forever, so the OpenFrame + # platform (Kafka, MongoDB, Cassandra, …) could never come up. aws-ebs-csi-driver = { most_recent = true - # Tag CSI-provisioned volumes with the cluster name: they are created - # OUTSIDE terraform state, and this tag is the only handle the CLI's - # post-destroy orphan sweep has on them (see teardown.go). configuration_values = jsonencode({ + # Since EKS 1.30 AWS no longer marks the legacy gp2 class as default: + # a PVC with no explicit storageClassName stays Pending forever. The + # addon can create a default gp3 class itself (addon >= 1.31.0). + defaultStorageClass = { + enabled = true + } controller = { + # Tag CSI-provisioned volumes with the cluster name: they are created + # OUTSIDE terraform state, and this tag is the only handle the CLI's + # post-destroy orphan sweep has on them (see teardown.go). extraVolumeTags = { "openframe:cluster" = var.cluster_name } diff --git a/internal/cluster/providers/terraform/engine.go b/internal/cluster/providers/terraform/engine.go index 9c5b8283..aa169f85 100644 --- a/internal/cluster/providers/terraform/engine.go +++ b/internal/cluster/providers/terraform/engine.go @@ -65,14 +65,54 @@ func NewEngine(verbose bool) *Engine { return nil, fmt.Errorf("initializing terraform runner: %w", err) } if verbose { - tf.SetStdout(os.Stdout) tf.SetStderr(os.Stderr) + return &verboseRunner{Terraform: tf}, nil } return tf, nil }, } } +// verboseRunner streams terraform's human-readable output (init, plan) to the +// terminal in verbose mode WITHOUT tee-ing the machine-readable commands. +// Holding tf.SetStdout(os.Stdout) for the runner's whole life did exactly +// that: tfexec merges its JSON parse buffer with the configured stdout, so +// `terraform show -json` dumped the entire plan — one 641 KB line, CA cert +// and user-data included — into the terminal, burying the very errors +// --verbose exists to reveal. Stdout is therefore enabled only around the +// human-output commands; ApplyJSON/DestroyJSON are unaffected either way +// (they pipe stdout to their own progress writer). +type verboseRunner struct { + *tfexec.Terraform +} + +// withStdout runs fn with terraform's human stdout streaming to the terminal, +// then silences it again. tfexec lazily runs `terraform version -json` before +// the first command of an instance — priming it first keeps even that blob +// off the terminal. +func (r *verboseRunner) withStdout(ctx context.Context, fn func() error) error { + if _, _, err := r.Terraform.Version(ctx, false); err != nil { + return err + } + r.Terraform.SetStdout(os.Stdout) + defer r.Terraform.SetStdout(io.Discard) + return fn() +} + +func (r *verboseRunner) Init(ctx context.Context, opts ...tfexec.InitOption) error { + return r.withStdout(ctx, func() error { return r.Terraform.Init(ctx, opts...) }) +} + +func (r *verboseRunner) Plan(ctx context.Context, opts ...tfexec.PlanOption) (bool, error) { + var changes bool + err := r.withStdout(ctx, func() error { + var planErr error + changes, planErr = r.Terraform.Plan(ctx, opts...) + return planErr + }) + return changes, err +} + // NewEngineWithRunner is the test constructor. func NewEngineWithRunner(newRunner func(workdir string) (Runner, error)) *Engine { return &Engine{newRunner: newRunner} diff --git a/internal/cluster/providers/terraform/engine_test.go b/internal/cluster/providers/terraform/engine_test.go index 87f6db9b..39cfe492 100644 --- a/internal/cluster/providers/terraform/engine_test.go +++ b/internal/cluster/providers/terraform/engine_test.go @@ -229,3 +229,27 @@ func TestEngine_PlanCarriesPlanJSON(t *testing.T) { // The machine-readable plan feeds the optional infracost estimate. assert.Contains(t, string(summary.PlanJSON), "google_container_cluster.primary") } + +// Verbose must wrap the production runner so human output (init, plan) +// streams while machine-readable commands stay off the terminal — a global +// SetStdout(os.Stdout) used to tee the entire `terraform show -json` plan +// (one 641 KB line) into a verbose session. The stub binary is never +// executed; the test only exercises runner construction. +func TestNewEngine_VerboseWrapsRunnerForSelectiveStdout(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + binDir := filepath.Join(home, "bin") + require.NoError(t, os.MkdirAll(binDir, 0o750)) + require.NoError(t, os.WriteFile(filepath.Join(binDir, "terraform"), []byte("#!/bin/sh\n"), 0o750)) // #nosec G306 -- must be executable for LookPath + t.Setenv("PATH", binDir) + + verbose, err := NewEngine(true).newRunner(t.TempDir()) + require.NoError(t, err) + assert.IsType(t, &verboseRunner{}, verbose, + "verbose must stream selectively via verboseRunner, not hold stdout for every command") + + quiet, err := NewEngine(false).newRunner(t.TempDir()) + require.NoError(t, err) + assert.IsType(t, &tfexec.Terraform{}, quiet, + "non-verbose needs no wrapper — stdout is never streamed") +} diff --git a/internal/cluster/ui/wizard.go b/internal/cluster/ui/wizard.go index e652face..361da5c0 100644 --- a/internal/cluster/ui/wizard.go +++ b/internal/cluster/ui/wizard.go @@ -118,7 +118,9 @@ func (w *ConfigWizard) Run() (ClusterConfig, error) { w.config.Region = region } - defaultMachine := "m6i.large" + // Mirrors the template default: the Free-Tier-eligible drop-in for + // m6i.large, so the wizard's suggestion also works on a new AWS account. + defaultMachine := "m7i-flex.large" if clusterType == models.ClusterTypeGKE { defaultMachine = "e2-standard-4" } diff --git a/internal/shared/errors/errors.go b/internal/shared/errors/errors.go index 169b4b4d..91eab893 100644 --- a/internal/shared/errors/errors.go +++ b/internal/shared/errors/errors.go @@ -182,6 +182,13 @@ func genericHint(err error) string { // cause. "create failed for cluster X: quota exceeded" → ("create failed for // cluster X", "quota exceeded"). A chain of one keeps everything in the // headline (first line) with any remaining lines as the cause block. +// +// The cause is everything from the deepest error onward, not the deepest +// error's text alone: tfexec wraps a bare *exec.ExitError and APPENDS +// terraform's stderr after it, so the deepest text is "exit status 1" while +// the actual reason (quota, eligibility, …) lives in the tail. Cutting the +// cause at the deepest text alone showed the user "cause: exit status 1" +// after a 30-minute apply and threw the explanation away. func splitCause(err error) (headline, cause string) { full := err.Error() deepest := err @@ -199,7 +206,7 @@ func splitCause(err error) (headline, cause string) { if idx := strings.LastIndex(full, causeText); idx > 0 && causeText != "" { head := strings.TrimRight(strings.TrimSuffix(full[:idx], ": "), ": \n") if head != "" { - return head, causeText + return head, strings.TrimSpace(full[idx:]) } } } diff --git a/internal/shared/errors/errors_test.go b/internal/shared/errors/errors_test.go index 53a0350f..d12cd197 100644 --- a/internal/shared/errors/errors_test.go +++ b/internal/shared/errors/errors_test.go @@ -219,6 +219,31 @@ func TestErrorHandler_HandleError_GenericError(t *testing.T) { } } +func TestSplitCause_SimpleChain(t *testing.T) { + err := fmt.Errorf("create failed for cluster X: %w", errors.New("quota exceeded")) + headline, cause := splitCause(err) + assert.Equal(t, "create failed for cluster X", headline) + assert.Equal(t, "quota exceeded", cause) +} + +// tfexec wraps a bare *exec.ExitError and appends terraform's stderr AFTER it: +// err.Error() is "exit status 1\nError: ". The deepest error +// alone is the useless "exit status 1" — the cause must keep the tail, or a +// 30-minute apply failure renders as "cause: exit status 1" with the actual +// reason (quota, Free Tier eligibility, …) discarded. +func TestSplitCause_KeepsTextAfterDeepestError(t *testing.T) { + exitErr := errors.New("exit status 1") + tfErr := fmt.Errorf("%w\nError: creating EC2 Instance: InvalidParameterCombination - not eligible for Free Tier", exitErr) + err := fmt.Errorf("cluster create operation failed for 'my-eks': %w", + fmt.Errorf("terraform apply failed: %w", tfErr)) + + headline, cause := splitCause(err) + assert.Equal(t, "cluster create operation failed for 'my-eks': terraform apply failed", headline) + assert.Contains(t, cause, "exit status 1") + assert.Contains(t, cause, "not eligible for Free Tier", + "the informative tail after the deepest error must reach the user") +} + // resumeHintStub carries a resume hint the way the gke provider's error does. type resumeHintStub struct { err error From 7a07e1235e20da4ad6051f07df161db067d29ae8 Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Thu, 6 Aug 2026 06:53:53 +0300 Subject: [PATCH 2/8] fix(app): preflight the chart --ref before ArgoCD is installed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A typo'd --ref used to surface only at clone time — AFTER certificates were refreshed, the argo-cd helm release deployed and the API port awaited — leaving the cluster mutated with ArgoCD and no applications. - git.Repository.ValidateRef: one ls-remote round-trip (go-git remote list, same transport and auth as the clone) checks the ref exists as a branch or tag before anything touches the cluster - Installer runs the preflight first when app-of-apps is configured; the production validator is the git repository provider, injected through a ChartService seam so orchestration tests stay off the network - BranchNotFoundError now carries the refs the repository DOES offer; the error panel lists branches in full and the version-sorted top tags (1.0.9 < 1.0.48 — plain string sort would invert them), capped so hundreds of release tags cannot bury the branches --- internal/chart/providers/git/repository.go | 42 +++++++++++ .../chart/providers/git/repository_test.go | 34 +++++++++ internal/chart/services/chart_service.go | 17 ++++- .../services/install_orchestration_test.go | 65 +++++++++++++---- internal/chart/services/installer.go | 20 ++++++ internal/chart/utils/types/interfaces.go | 8 +++ internal/shared/errors/errors.go | 70 +++++++++++++++++++ internal/shared/errors/errors_test.go | 27 +++++++ 8 files changed, 266 insertions(+), 17 deletions(-) diff --git a/internal/chart/providers/git/repository.go b/internal/chart/providers/git/repository.go index abf6ce17..7b88621b 100644 --- a/internal/chart/providers/git/repository.go +++ b/internal/chart/providers/git/repository.go @@ -9,8 +9,11 @@ import ( "strings" "github.com/flamingo-stack/openframe-cli/internal/chart/models" + sharedErrors "github.com/flamingo-stack/openframe-cli/internal/shared/errors" gogit "github.com/go-git/go-git/v5" + gitconfig "github.com/go-git/go-git/v5/config" "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v5/storage/memory" "github.com/pterm/pterm" ) @@ -75,6 +78,45 @@ func (r *Repository) CloneChartRepository(ctx context.Context, config *models.Ap return nil, fmt.Errorf("failed to clone repository: %s", maskToken(lastErr.Error(), auth.token)) } +// ValidateRef checks — in one ls-remote round-trip, before anything touches +// the cluster — that the configured ref exists in the chart repository as a +// branch or tag. The clone used to be the first place a bad --ref surfaced, +// AFTER ArgoCD was already installed: certificates refreshed, helm release +// deployed, API port awaited, then "branch does not exist" — leaving the +// cluster mutated with no applications. A missing ref returns a +// *sharedErrors.BranchNotFoundError carrying the refs the repository DOES +// offer, so the error can answer the next question instead of prompting it. +func (r *Repository) ValidateRef(ctx context.Context, config *models.AppOfAppsConfig) error { + auth := extractGitAuth(config.GitHubRepo) + remote := gogit.NewRemote(memory.NewStorage(), &gitconfig.RemoteConfig{ + Name: "origin", + URLs: []string{auth.cleanURL}, + }) + refs, err := remote.ListContext(ctx, &gogit.ListOptions{Auth: auth.buildAuth()}) + if err != nil { + // Same transport and auth as the clone: whatever broke here would have + // broken the clone too, only after the cluster had been mutated. + return fmt.Errorf("could not list refs of the chart repository: %s", maskToken(err.Error(), auth.token)) + } + + var branches, tags []string + for _, ref := range refs { + name := ref.Name() + switch { + case name.IsBranch(): + branches = append(branches, name.Short()) + case name.IsTag(): + tags = append(tags, name.Short()) + } + } + for _, existing := range append(branches, tags...) { + if existing == config.GitHubBranch { + return nil + } + } + return sharedErrors.NewBranchNotFoundErrorWithRefs(config.GitHubBranch, branches, tags) +} + // chartResult validates that chartPath exists inside the freshly cloned tempDir // and returns the CloneResult, cleaning up on failure. func (r *Repository) chartResult(tempDir, chartSubPath string) (*CloneResult, error) { diff --git a/internal/chart/providers/git/repository_test.go b/internal/chart/providers/git/repository_test.go index 705565b1..da02404f 100644 --- a/internal/chart/providers/git/repository_test.go +++ b/internal/chart/providers/git/repository_test.go @@ -9,6 +9,7 @@ import ( "time" "github.com/flamingo-stack/openframe-cli/internal/chart/models" + sharedErrors "github.com/flamingo-stack/openframe-cli/internal/shared/errors" gogit "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/plumbing/object" "github.com/stretchr/testify/require" @@ -148,3 +149,36 @@ func ofcredCount(t *testing.T) int { require.NoError(t, err) return len(matches) } + +// ValidateRef is the pre-install ls-remote preflight: a bad --ref must be +// caught in one round-trip BEFORE ArgoCD is installed onto the cluster. +func TestValidateRef_BranchAndTag(t *testing.T) { + url, branch := makeLocalRepo(t, "manifests/app-of-apps") + tagLocalRepo(t, url, "1.0.48") + repo := NewRepository() + + for _, ref := range []string{branch, "1.0.48"} { + require.NoError(t, repo.ValidateRef(context.Background(), &models.AppOfAppsConfig{ + GitHubRepo: url, + GitHubBranch: ref, + }), "existing ref %q must validate", ref) + } +} + +func TestValidateRef_MissingRefListsAvailable(t *testing.T) { + url, branch := makeLocalRepo(t, "manifests/app-of-apps") + tagLocalRepo(t, url, "1.0.48") + repo := NewRepository() + + err := repo.ValidateRef(context.Background(), &models.AppOfAppsConfig{ + GitHubRepo: url, + GitHubBranch: "v1.4.0", + }) + require.Error(t, err) + + var bnfErr *sharedErrors.BranchNotFoundError + require.ErrorAs(t, err, &bnfErr, "a missing ref must be a BranchNotFoundError so the handler renders it") + require.Equal(t, "v1.4.0", bnfErr.Branch) + require.Contains(t, bnfErr.Branches, branch, "the error must carry the refs the repository DOES offer") + require.Contains(t, bnfErr.Tags, "1.0.48") +} diff --git a/internal/chart/services/chart_service.go b/internal/chart/services/chart_service.go index 25d59055..12dbb097 100644 --- a/internal/chart/services/chart_service.go +++ b/internal/chart/services/chart_service.go @@ -63,9 +63,19 @@ type ChartService struct { // to exercise the install orchestration without a cluster (see // install_orchestration_test.go). Kept unexported on purpose — the public // constructors' signatures and behavior are unchanged. - newFileCleanup func() installFileCleanup - installServices installServicesFactory - installRetryPolicy sharedErrors.RetryPolicy + newFileCleanup func() installFileCleanup + installServices installServicesFactory + installRetryPolicy sharedErrors.RetryPolicy + installRefValidator types.GitRefValidator +} + +// installRefValidatorOrDefault returns the injected --ref preflight, or the +// production git repository provider. +func (cs *ChartService) installRefValidatorOrDefault() types.GitRefValidator { + if cs.installRefValidator != nil { + return cs.installRefValidator + } + return cs.gitRepository } // fileCleanupOrDefault returns the injected file-cleanup factory's product, or @@ -573,6 +583,7 @@ func (w *InstallationWorkflow) performInstallation(ctx context.Context, config c installer := &Installer{ argoCDService: argoCDService, appOfAppsService: appOfAppsService, + refValidator: w.chartService.installRefValidatorOrDefault(), } err = installer.InstallChartsWithContext(ctx, config) diff --git a/internal/chart/services/install_orchestration_test.go b/internal/chart/services/install_orchestration_test.go index 7a9fb788..ffdf36be 100644 --- a/internal/chart/services/install_orchestration_test.go +++ b/internal/chart/services/install_orchestration_test.go @@ -7,6 +7,7 @@ import ( "testing" "time" + chartModels "github.com/flamingo-stack/openframe-cli/internal/chart/models" "github.com/flamingo-stack/openframe-cli/internal/chart/utils/config" "github.com/flamingo-stack/openframe-cli/internal/chart/utils/types" sharedErrors "github.com/flamingo-stack/openframe-cli/internal/shared/errors" @@ -65,15 +66,29 @@ func (s *spyFileCleanup) called(name string) bool { return false } +// stubRefValidator fakes the --ref ls-remote preflight: the orchestration +// tests must not reach the network (the production validator would try to +// list refs of the fake GitHub URL). +type stubRefValidator struct { + err error + calls int +} + +func (s *stubRefValidator) ValidateRef(context.Context, *chartModels.AppOfAppsConfig) error { + s.calls++ + return s.err +} + // orchestrationHarness wires a real ChartService (real HelmManager, real // Installer, real retry executor) with faked install collaborators. type orchestrationHarness struct { - svc *ChartService - argoCD *MockArgoCDService - appOfApps *MockAppOfAppsService - cleanup *spyFileCleanup - order []string - installCfg *config.ChartInstallConfig // config seen by the collaborator factory + svc *ChartService + argoCD *MockArgoCDService + appOfApps *MockAppOfAppsService + cleanup *spyFileCleanup + refValidator *stubRefValidator + order []string + installCfg *config.ChartInstallConfig // config seen by the collaborator factory } // step returns a mock Run-callback recording invocation order across fakes. @@ -91,12 +106,14 @@ func newOrchestrationHarness(t *testing.T) *orchestrationHarness { } h := &orchestrationHarness{ - svc: svc, - argoCD: new(MockArgoCDService), - appOfApps: new(MockAppOfAppsService), - cleanup: &spyFileCleanup{real: files.NewFileCleanup()}, + svc: svc, + argoCD: new(MockArgoCDService), + appOfApps: new(MockAppOfAppsService), + cleanup: &spyFileCleanup{real: files.NewFileCleanup()}, + refValidator: &stubRefValidator{}, } svc.newFileCleanup = func() installFileCleanup { return h.cleanup } + svc.installRefValidator = h.refValidator svc.installServices = func(_ *ChartService, cfg config.ChartInstallConfig) (types.ArgoCDService, types.AppOfAppsService, error) { h.installCfg = &cfg return h.argoCD, h.appOfApps, nil @@ -261,6 +278,24 @@ func TestInstallWithContext_CollaboratorFactoryErrorPropagates(t *testing.T) { requireTempValuesGone(t, h) } +// TestInstallWithContext_BadRefFailsBeforeAnythingTouchesTheCluster is the +// regression guard: a ref that does not exist in the chart repository must +// fail the install in the preflight — before ArgoCD is installed — and must +// not be retried. The bad ref used to surface only at clone time, AFTER +// ArgoCD was deployed, leaving the cluster mutated with no applications. +func TestInstallWithContext_BadRefFailsBeforeAnythingTouchesTheCluster(t *testing.T) { + h := newOrchestrationHarness(t) + h.refValidator.err = sharedErrors.NewBranchNotFoundErrorWithRefs("v1.4.0", []string{"main"}, []string{"1.0.48"}) + + err := h.svc.InstallWithContext(context.Background(), installRequest()) + + var bnfErr *sharedErrors.BranchNotFoundError + assert.True(t, stderrors.As(err, &bnfErr), "the BranchNotFoundError must surface unwrapped, got: %v", err) + assert.Equal(t, 1, h.refValidator.calls, "a definitive bad-ref verdict must not be retried") + h.argoCD.AssertNotCalled(t, "Install", mock.Anything, mock.Anything) + h.appOfApps.AssertNotCalled(t, "Install", mock.Anything, mock.Anything) +} + // TestInstallWithContextDeferred_ResolvesHelmManagerThenInstalls covers the // deferred entry point: the service starts without a HelmManager (standalone // install) and the workflow initializes it from the request's rest.Config @@ -273,12 +308,14 @@ func TestInstallWithContextDeferred_ResolvesHelmManagerThenInstalls(t *testing.T t.Fatalf("NewChartServiceDeferred: %v", err) } h := &orchestrationHarness{ - svc: svc, - argoCD: new(MockArgoCDService), - appOfApps: new(MockAppOfAppsService), - cleanup: &spyFileCleanup{real: files.NewFileCleanup()}, + svc: svc, + argoCD: new(MockArgoCDService), + appOfApps: new(MockAppOfAppsService), + cleanup: &spyFileCleanup{real: files.NewFileCleanup()}, + refValidator: &stubRefValidator{}, } svc.newFileCleanup = func() installFileCleanup { return h.cleanup } + svc.installRefValidator = h.refValidator svc.installServices = func(cs *ChartService, cfg config.ChartInstallConfig) (types.ArgoCDService, types.AppOfAppsService, error) { // By collaborator-construction time the deferred HelmManager must exist. assert.NotNil(t, cs.helmManager, "deferred HelmManager must be initialized before collaborators are built") diff --git a/internal/chart/services/installer.go b/internal/chart/services/installer.go index 14d51f22..fa047bb9 100644 --- a/internal/chart/services/installer.go +++ b/internal/chart/services/installer.go @@ -14,10 +14,30 @@ import ( type Installer struct { argoCDService types.ArgoCDService appOfAppsService types.AppOfAppsService + // refValidator preflights the chart ref before anything touches the + // cluster; nil skips the preflight (tests, callers without a repo). + refValidator types.GitRefValidator } // InstallChartsWithContext handles the complete chart installation process with context support func (i *Installer) InstallChartsWithContext(ctx context.Context, config config.ChartInstallConfig) error { + // A bad --ref must fail HERE, in seconds — not after ArgoCD has been + // installed: the clone was the first place a typo'd ref surfaced, leaving + // the cluster with ArgoCD deployed and no applications. + if config.HasAppOfApps() && i.refValidator != nil { + appConfig := *config.AppOfApps + if appConfig.GitHubBranch == "" { + appConfig.GitHubBranch = "main" // mirror the app-of-apps default + } + if err := i.refValidator.ValidateRef(ctx, &appConfig); err != nil { + var bnfErr *sharedErrors.BranchNotFoundError + if stderrors.As(err, &bnfErr) { + return err // renders its own actionable panel; don't wrap + } + return errors.WrapAsChartError("preflight", "chart repository", err).WithCluster(config.ClusterName) + } + } + // Install ArgoCD first if err := i.argoCDService.Install(ctx, config); err != nil { return errors.WrapAsChartError("installation", "ArgoCD", err).WithCluster(config.ClusterName) diff --git a/internal/chart/utils/types/interfaces.go b/internal/chart/utils/types/interfaces.go index aa3295fb..487e50f8 100644 --- a/internal/chart/utils/types/interfaces.go +++ b/internal/chart/utils/types/interfaces.go @@ -45,6 +45,14 @@ type AppOfAppsService interface { GetStatus(ctx context.Context, namespace string) (models.ChartInfo, error) } +// GitRefValidator preflights that a chart ref exists in the remote repository +// — one ls-remote round-trip — so a typo'd --ref fails in seconds, BEFORE +// ArgoCD is installed onto the cluster. Implemented by the git repository +// provider. +type GitRefValidator interface { + ValidateRef(ctx context.Context, config *models.AppOfAppsConfig) error +} + // InstallationRequest contains all parameters for chart installation type InstallationRequest struct { Args []string diff --git a/internal/shared/errors/errors.go b/internal/shared/errors/errors.go index 91eab893..ab232c13 100644 --- a/internal/shared/errors/errors.go +++ b/internal/shared/errors/errors.go @@ -4,6 +4,8 @@ import ( "context" stderrors "errors" "fmt" + "sort" + "strconv" "strings" "github.com/flamingo-stack/openframe-cli/internal/shared/executor" @@ -117,6 +119,9 @@ func (eh *ErrorHandler) handleCommandError(err *executor.CommandError, outer err // from a config file or a default rather than from something the user typed. func (eh *ErrorHandler) handleBranchNotFoundError(err *BranchNotFoundError) { pterm.Error.Printfln("Branch %q does not exist in the chart repository", err.Branch) + if available := err.AvailableSummary(); available != "" { + pterm.Info.Printfln("Available refs — %s", available) + } pterm.Info.Println("Check the ref, or pass an existing one with --ref (e.g. --ref main)") } @@ -269,6 +274,12 @@ func (eh *ErrorHandler) isUserInterruption(err error) bool { // BranchNotFoundError represents a branch not found error type BranchNotFoundError struct { Branch string + // Branches and Tags list what the repository actually offers, when the + // caller had them (the ls-remote preflight does; the clone path does not). + // They turn "check the ref" into an answer: the chart repo's tag scheme is + // not guessable (262 tags, none with a v prefix) — see AvailableSummary. + Branches []string + Tags []string } func (e *BranchNotFoundError) Error() string { @@ -280,6 +291,65 @@ func NewBranchNotFoundError(branch string) *BranchNotFoundError { return &BranchNotFoundError{Branch: branch} } +// NewBranchNotFoundErrorWithRefs creates a branch not found error that also +// carries the refs the repository does offer, for actionable display. +func NewBranchNotFoundErrorWithRefs(branch string, branches, tags []string) *BranchNotFoundError { + return &BranchNotFoundError{Branch: branch, Branches: branches, Tags: tags} +} + +// maxTagsShown caps the tag listing: the chart repository carries hundreds of +// tags, and a wall of them would bury the branches (the refs most users want). +const maxTagsShown = 10 + +// AvailableSummary renders the refs the repository offers as one human line, +// or "" when the error does not carry them. Branches are listed in full +// (there are few); tags are version-sorted and capped at the highest +// maxTagsShown with a "+N more" marker. +func (e *BranchNotFoundError) AvailableSummary() string { + if len(e.Branches) == 0 && len(e.Tags) == 0 { + return "" + } + var parts []string + if len(e.Branches) > 0 { + branches := append([]string(nil), e.Branches...) + sort.Strings(branches) + parts = append(parts, "branches: "+strings.Join(branches, ", ")) + } + if len(e.Tags) > 0 { + tags := append([]string(nil), e.Tags...) + sort.Slice(tags, func(i, j int) bool { return versionLess(tags[j], tags[i]) }) + more := "" + if len(tags) > maxTagsShown { + more = fmt.Sprintf(", … +%d more", len(tags)-maxTagsShown) + tags = tags[:maxTagsShown] + } + parts = append(parts, "tags: "+strings.Join(tags, ", ")+more) + } + return strings.Join(parts, "; ") +} + +// versionLess orders version-like strings numerically per dot-separated part +// ("1.0.9" < "1.0.48"), falling back to string order for non-numeric parts — +// plain sort.Strings would rank 1.0.9 above 1.0.48. +func versionLess(a, b string) bool { + as, bs := strings.Split(strings.TrimPrefix(a, "v"), "."), strings.Split(strings.TrimPrefix(b, "v"), ".") + for i := 0; i < len(as) && i < len(bs); i++ { + an, aerr := strconv.Atoi(as[i]) + bn, berr := strconv.Atoi(bs[i]) + switch { + case aerr == nil && berr == nil: + if an != bn { + return an < bn + } + default: + if as[i] != bs[i] { + return as[i] < bs[i] + } + } + } + return len(as) < len(bs) +} + // HandleGlobalError provides a global error handling entry point // This should be used by all command RunE functions to ensure consistent error handling func HandleGlobalError(err error, verbose bool) error { diff --git a/internal/shared/errors/errors_test.go b/internal/shared/errors/errors_test.go index d12cd197..60aa6416 100644 --- a/internal/shared/errors/errors_test.go +++ b/internal/shared/errors/errors_test.go @@ -450,3 +450,30 @@ func TestGenericHint_K3dCreateSpecialCase(t *testing.T) { assert.Contains(t, hint, "docker info") assert.Contains(t, hint, "6550") } + +// AvailableSummary turns "check the ref" into an answer: it lists what the +// repository actually offers. Tags sort numerically per part (1.0.9 < 1.0.48 — +// plain string sort would invert them) and are capped so hundreds of release +// tags cannot bury the branches. +func TestBranchNotFoundError_AvailableSummary(t *testing.T) { + t.Run("empty without refs", func(t *testing.T) { + assert.Empty(t, NewBranchNotFoundError("x").AvailableSummary()) + }) + + t.Run("branches and version-sorted tags", func(t *testing.T) { + e := NewBranchNotFoundErrorWithRefs("v1.4.0", []string{"main", "develop"}, []string{"1.0.9", "1.0.48", "0.9.1"}) + s := e.AvailableSummary() + assert.Equal(t, "branches: develop, main; tags: 1.0.48, 1.0.9, 0.9.1", s) + }) + + t.Run("tags capped with more-marker", func(t *testing.T) { + tags := make([]string, 0, 25) + for i := 1; i <= 25; i++ { + tags = append(tags, fmt.Sprintf("1.0.%d", i)) + } + s := NewBranchNotFoundErrorWithRefs("x", nil, tags).AvailableSummary() + assert.Contains(t, s, "1.0.25", "the highest tags must be the ones shown") + assert.Contains(t, s, "+15 more") + assert.NotContains(t, s, "1.0.1,", "low tags beyond the cap must be elided") + }) +} From 0d2bce64306a86708c72251e60d1a2086df4d318 Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Thu, 6 Aug 2026 07:09:39 +0300 Subject: [PATCH 3/8] fix(cluster): verify health before the success box claims Ready; stop list drifting from reality MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The creation box printed a hardcoded green 'Ready' and the configured node count on the provisioner's exit code alone; it would render identically over NotReady nodes or a cluster that cannot bind a single PVC. The box now verifies via client-go (brief poll, 45s cap): every node Ready and a default StorageClass present render 'Ready' with the observed fraction; anything else renders 'Provisioned — n/m nodes Ready' (title downgraded to ⚠️) with a warning naming the specific gap; an unreachable API says 'health not verified' instead of guessing. The list's CONTEXT column was fabricated from the cluster name even when no kubeconfig entry existed (plan-stage failure). It is now derived from the kubeconfig via tfengine.KubeconfigHasContext. A resumed create also flips the record to 'creating' so list stops reporting the previous attempt's 'Failed' while an apply is running. On the transition to Ready the record's CreatedAt is stamped fresh, so a resumed create no longer reports the first failed attempt's timestamp forever. --- internal/cluster/health_test.go | 94 +++++++++++++ internal/cluster/providers/eks/provider.go | 17 ++- .../cluster/providers/eks/provider_test.go | 56 ++++++++ internal/cluster/providers/gke/provider.go | 17 ++- .../providers/terraform/kubecontext.go | 23 ++++ .../providers/terraform/kubecontext_test.go | 35 +++++ internal/cluster/service.go | 128 ++++++++++++++++-- 7 files changed, 360 insertions(+), 10 deletions(-) create mode 100644 internal/cluster/health_test.go create mode 100644 internal/cluster/providers/terraform/kubecontext.go create mode 100644 internal/cluster/providers/terraform/kubecontext_test.go diff --git a/internal/cluster/health_test.go b/internal/cluster/health_test.go new file mode 100644 index 00000000..6a44dd79 --- /dev/null +++ b/internal/cluster/health_test.go @@ -0,0 +1,94 @@ +package cluster + +import ( + "context" + "errors" + "testing" + + "github.com/pterm/pterm" + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + storagev1 "k8s.io/api/storage/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/fake" +) + +// The creation box used to print a hardcoded green "Ready" on the +// provisioner's exit code alone — identically over NotReady nodes or a +// cluster with no default StorageClass. These tests pin the verified +// rendering. + +func node(name string, ready bool) *corev1.Node { + status := corev1.ConditionFalse + if ready { + status = corev1.ConditionTrue + } + return &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Status: corev1.NodeStatus{Conditions: []corev1.NodeCondition{ + {Type: corev1.NodeReady, Status: status}, + }}, + } +} + +func storageClass(name string, isDefault bool) *storagev1.StorageClass { + sc := &storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{Name: name}} + if isDefault { + sc.Annotations = map[string]string{"storageclass.kubernetes.io/is-default-class": "true"} + } + return sc +} + +func TestObserveClusterHealth(t *testing.T) { + t.Run("all ready with default storage class", func(t *testing.T) { + client := fake.NewSimpleClientset(node("a", true), node("b", true), storageClass("gp3", true)) + h := observeClusterHealth(context.Background(), client) + assert.True(t, h.healthy()) + assert.Equal(t, 2, h.readyNodes) + assert.Equal(t, 2, h.totalNodes) + }) + + t.Run("not-ready node breaks health", func(t *testing.T) { + client := fake.NewSimpleClientset(node("a", true), node("b", false), storageClass("gp3", true)) + h := observeClusterHealth(context.Background(), client) + assert.False(t, h.healthy()) + assert.Equal(t, 1, h.readyNodes) + assert.Equal(t, 2, h.totalNodes) + }) + + t.Run("missing default storage class breaks health", func(t *testing.T) { + // A StorageClass exists (legacy gp2) but none is default, so every + // PVC would stay Pending — the box must not say Ready. + client := fake.NewSimpleClientset(node("a", true), storageClass("gp2", false)) + h := observeClusterHealth(context.Background(), client) + assert.False(t, h.healthy()) + assert.False(t, h.hasDefaultStorageClass) + }) + + t.Run("no nodes is not healthy", func(t *testing.T) { + client := fake.NewSimpleClientset(storageClass("gp3", true)) + assert.False(t, observeClusterHealth(context.Background(), client).healthy()) + }) +} + +func TestSummaryStatusLines(t *testing.T) { + t.Run("verified healthy renders Ready", func(t *testing.T) { + status, nodes := summaryStatusLines(clusterHealth{readyNodes: 3, totalNodes: 3, hasDefaultStorageClass: true}, 3) + assert.Contains(t, pterm.RemoveColorFromString(status), "Ready") + assert.Equal(t, "3/3 Ready", nodes) + }) + + t.Run("unhealthy renders the observed fraction, never Ready", func(t *testing.T) { + status, nodes := summaryStatusLines(clusterHealth{readyNodes: 0, totalNodes: 3, hasDefaultStorageClass: true}, 3) + plain := pterm.RemoveColorFromString(status) + assert.Contains(t, plain, "0/3 nodes Ready") + assert.NotEqual(t, "Ready", plain) + assert.Equal(t, "0/3 Ready", nodes) + }) + + t.Run("unreachable API says so instead of guessing", func(t *testing.T) { + status, nodes := summaryStatusLines(clusterHealth{verifyErr: errors.New("dial tcp: timeout")}, 3) + assert.Contains(t, pterm.RemoveColorFromString(status), "not verified") + assert.Equal(t, "3 (configured)", nodes) + }) +} diff --git a/internal/cluster/providers/eks/provider.go b/internal/cluster/providers/eks/provider.go index f0848b11..083f76a1 100644 --- a/internal/cluster/providers/eks/provider.go +++ b/internal/cluster/providers/eks/provider.go @@ -211,6 +211,11 @@ func (p *Provider) CreateCluster(ctx context.Context, config models.ClusterConfi if err := tfengine.WriteModule(ws.TerraformDir(), mainTF, vars); err != nil { return nil, err } + if !freshWorkspace { + // A resumed create is creating again — `cluster list` must not keep + // reporting the previous attempt's "Failed" while an apply is running. + _ = ws.SetStatus(tfengine.StatusCreating) + } if err := p.engine.Init(ctx, ws.TerraformDir()); err != nil { _ = ws.SetStatus(tfengine.StatusFailed) @@ -271,6 +276,9 @@ func (p *Provider) CreateCluster(ctx context.Context, config models.ClusterConfi return nil, models.NewClusterOperationError("create", config.Name, err) } record.Status = tfengine.StatusReady + // CREATED means "when this cluster became Ready": a resumed create must + // not keep the first, failed attempt's timestamp forever. + record.CreatedAt = time.Now().UTC() if err := ws.WriteRecord(record); err != nil { return nil, err } @@ -398,11 +406,18 @@ func (p *Provider) GetKubeconfig(ctx context.Context, name string, clusterType m // infoFor maps a registry record onto the shared ClusterInfo shape. func infoFor(rec tfengine.Record) models.ClusterInfo { + // The kubeconfig context exists only once a successful create merged it — + // report what the kubeconfig holds, never a value fabricated from the + // cluster name (a plan-stage failure has no context at all). + kubeContext := "" + if tfengine.KubeconfigHasContext(rec.Name) { + kubeContext = rec.Name + } return models.ClusterInfo{ Name: rec.Name, Type: models.ClusterTypeEKS, Source: models.SourceOpenframe, - Context: rec.Name, + Context: kubeContext, Profile: rec.Profile, Region: rec.Region, Status: rec.Status.Title(), diff --git a/internal/cluster/providers/eks/provider_test.go b/internal/cluster/providers/eks/provider_test.go index 3ca712b2..cceb71bf 100644 --- a/internal/cluster/providers/eks/provider_test.go +++ b/internal/cluster/providers/eks/provider_test.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "testing" + "time" "github.com/flamingo-stack/openframe-cli/internal/cluster/models" tfengine "github.com/flamingo-stack/openframe-cli/internal/cluster/providers/terraform" @@ -377,3 +378,58 @@ func TestPlanCluster_ExistingWorkspacePreviewsResumeWithoutSideEffects(t *testin require.NoError(t, err) assert.Equal(t, `{"version":4}`, string(afterState)) } + +// The CONTEXT column must reflect the kubeconfig, not the cluster name: a +// plan-stage failure leaves no kubeconfig entry, and the list used to print +// one anyway. +func TestInfoFor_ContextComesFromKubeconfig(t *testing.T) { + kubeconfig := filepath.Join(t.TempDir(), "config") + require.NoError(t, os.WriteFile(kubeconfig, []byte(` +apiVersion: v1 +kind: Config +contexts: +- name: merged-eks + context: + cluster: merged-eks + user: merged-eks +`), 0o600)) + t.Setenv("KUBECONFIG", kubeconfig) + + assert.Equal(t, "merged-eks", infoFor(tfengine.Record{Name: "merged-eks"}).Context, + "a context the kubeconfig holds must be reported") + assert.Empty(t, infoFor(tfengine.Record{Name: "failed-at-plan"}).Context, + "no kubeconfig entry — the list must not fabricate a context from the name") +} + +// A resumed create must end Ready with a FRESH CreatedAt: the row used to +// keep the first failed attempt's timestamp forever, and stay "Failed" in +// list while the resume was running. +func TestCreateCluster_ResumeRefreshesStatusAndCreatedAt(t *testing.T) { + t.Setenv("KUBECONFIG", filepath.Join(t.TempDir(), "kubeconfig")) + base := t.TempDir() + mock := executor.NewMockCommandExecutor() + + providerWith := func(applyErr error) *Provider { + calls := &[]string{} + engine := tfengine.NewEngineWithRunner(func(workdir string) (tfengine.Runner, error) { + return &fakeRunner{calls: calls, applyErr: applyErr}, nil + }) + return NewWithDeps(engine, tfengine.NewRegistry(base), mock) + } + + _, err := providerWith(errors.New("quota exceeded")).CreateCluster(context.Background(), eksConfig("demo")) + require.Error(t, err) + failed, err := tfengine.NewRegistry(base).Get("demo") + require.NoError(t, err) + require.Equal(t, tfengine.StatusFailed, failed.Status) + + time.Sleep(10 * time.Millisecond) // make the CreatedAt refresh observable + _, err = providerWith(nil).CreateCluster(context.Background(), eksConfig("demo")) + require.NoError(t, err) + + resumed, err := tfengine.NewRegistry(base).Get("demo") + require.NoError(t, err) + assert.Equal(t, tfengine.StatusReady, resumed.Status) + assert.True(t, resumed.CreatedAt.After(failed.CreatedAt), + "CREATED must reflect when the cluster became Ready, not the first failed attempt") +} diff --git a/internal/cluster/providers/gke/provider.go b/internal/cluster/providers/gke/provider.go index 7e8cbc1a..39a927bb 100644 --- a/internal/cluster/providers/gke/provider.go +++ b/internal/cluster/providers/gke/provider.go @@ -292,6 +292,11 @@ func (p *Provider) CreateCluster(ctx context.Context, config models.ClusterConfi if err := tfengine.WriteModule(ws.TerraformDir(), mainTF, vars); err != nil { return nil, err } + if !freshWorkspace { + // A resumed create is creating again — `cluster list` must not keep + // reporting the previous attempt's "Failed" while an apply is running. + _ = ws.SetStatus(tfengine.StatusCreating) + } if err := p.engine.Init(ctx, ws.TerraformDir()); err != nil { _ = ws.SetStatus(tfengine.StatusFailed) @@ -358,6 +363,9 @@ func (p *Provider) CreateCluster(ctx context.Context, config models.ClusterConfi return nil, models.NewClusterOperationError("create", config.Name, err) } record.Status = tfengine.StatusReady + // CREATED means "when this cluster became Ready": a resumed create must + // not keep the first, failed attempt's timestamp forever. + record.CreatedAt = time.Now().UTC() if err := ws.WriteRecord(record); err != nil { return nil, err } @@ -482,11 +490,18 @@ func (p *Provider) GetKubeconfig(ctx context.Context, name string, clusterType m // infoFor maps a registry record onto the shared ClusterInfo shape. func infoFor(rec tfengine.Record) models.ClusterInfo { + // The kubeconfig context exists only once a successful create merged it — + // report what the kubeconfig holds, never a value fabricated from the + // cluster name (a plan-stage failure has no context at all). + kubeContext := "" + if tfengine.KubeconfigHasContext(rec.Name) { + kubeContext = rec.Name + } return models.ClusterInfo{ Name: rec.Name, Type: models.ClusterTypeGKE, Source: models.SourceOpenframe, - Context: rec.Name, + Context: kubeContext, Project: rec.Project, Region: rec.Region, Status: rec.Status.Title(), diff --git a/internal/cluster/providers/terraform/kubecontext.go b/internal/cluster/providers/terraform/kubecontext.go new file mode 100644 index 00000000..a52761ca --- /dev/null +++ b/internal/cluster/providers/terraform/kubecontext.go @@ -0,0 +1,23 @@ +package terraform + +import ( + "k8s.io/client-go/tools/clientcmd" +) + +// KubeconfigHasContext reports whether the user's kubeconfig (default loading +// rules: $KUBECONFIG or ~/.kube/config) actually contains a context with the +// given name. +// +// The cluster list's CONTEXT column must be derived from this, never from the +// cluster name: the context is merged only after a successful create, so a +// workspace whose apply failed at plan time has NO kubeconfig entry — yet the +// list used to print one fabricated from the name. Any read error counts as +// "no context": inventing one is the failure mode this exists to prevent. +func KubeconfigHasContext(name string) bool { + cfg, err := clientcmd.NewDefaultClientConfigLoadingRules().Load() + if err != nil || cfg == nil { + return false + } + _, ok := cfg.Contexts[name] + return ok +} diff --git a/internal/cluster/providers/terraform/kubecontext_test.go b/internal/cluster/providers/terraform/kubecontext_test.go new file mode 100644 index 00000000..234aca84 --- /dev/null +++ b/internal/cluster/providers/terraform/kubecontext_test.go @@ -0,0 +1,35 @@ +package terraform + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// KubeconfigHasContext must report what the kubeconfig actually holds — the +// list's CONTEXT column is derived from it, and used to fabricate a context +// for clusters whose create failed at plan time. +func TestKubeconfigHasContext(t *testing.T) { + kubeconfig := filepath.Join(t.TempDir(), "config") + require.NoError(t, os.WriteFile(kubeconfig, []byte(` +apiVersion: v1 +kind: Config +contexts: +- name: my-eks + context: + cluster: my-eks + user: my-eks +`), 0o600)) + t.Setenv("KUBECONFIG", kubeconfig) + + assert.True(t, KubeconfigHasContext("my-eks")) + assert.False(t, KubeconfigHasContext("never-created"), + "a context absent from the kubeconfig must not be reported") + + t.Setenv("KUBECONFIG", filepath.Join(t.TempDir(), "missing")) + assert.False(t, KubeconfigHasContext("my-eks"), + "an unreadable kubeconfig means no context, not an invented one") +} diff --git a/internal/cluster/service.go b/internal/cluster/service.go index d779c8ae..b90d10e3 100644 --- a/internal/cluster/service.go +++ b/internal/cluster/service.go @@ -19,6 +19,7 @@ import ( "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" @@ -210,9 +211,10 @@ func (s *ClusterService) CreateCluster(ctx context.Context, config models.Cluste pterm.Success.Printf("Cluster '%s' created successfully\n", config.Name) } - // Get and display cluster status + // Get and display cluster status, verified against the live API — the box + // must never assert "Ready" on the provisioner's exit code alone. if clusterInfo, statusErr := mgr.GetClusterStatus(ctx, config.Name); statusErr == nil { - s.displayClusterCreationSummary(clusterInfo) + s.displayClusterCreationSummary(ctx, clusterInfo, restConfig) } // Show next steps @@ -748,20 +750,116 @@ func apiServerLine(endpoint string) string { return "API: " + endpoint } -// displayClusterCreationSummary displays a summary after cluster creation -func (s *ClusterService) displayClusterCreationSummary(info models.ClusterInfo) { +// clusterHealth is what a post-create verification actually observed. +// verifyErr is a failure to LOOK (unreachable API), which is different from +// looking and seeing something unhealthy. +type clusterHealth struct { + readyNodes, totalNodes int + hasDefaultStorageClass bool + verifyErr error +} + +// healthy reports whether everything checked out: every node Ready (and at +// least one exists) and a default StorageClass present — without it every +// PersistentVolumeClaim of the platform (Kafka, MongoDB, Cassandra, …) stays +// Pending forever. +func (h clusterHealth) healthy() bool { + return h.verifyErr == nil && h.totalNodes > 0 && h.readyNodes == h.totalNodes && h.hasDefaultStorageClass +} + +// verifyClusterHealth checks the just-created cluster with client-go: node +// readiness and the presence of a default StorageClass. It polls briefly — +// nodes registered a moment ago may still be flipping to Ready — and returns +// the last observation rather than waiting indefinitely. +func verifyClusterHealth(ctx context.Context, restConfig *rest.Config) clusterHealth { + client, err := kubernetes.NewForConfig(restConfig) + if err != nil { + return clusterHealth{verifyErr: err} + } + ctx, cancel := context.WithTimeout(ctx, 45*time.Second) + defer cancel() + + var h clusterHealth + for { + h = observeClusterHealth(ctx, client) + if h.healthy() { + return h + } + select { + case <-ctx.Done(): + return h // report the truth as last seen, not an error + case <-time.After(3 * time.Second): + } + } +} + +// observeClusterHealth performs one health observation via the given client. +func observeClusterHealth(ctx context.Context, client kubernetes.Interface) clusterHealth { + nodes, err := client.CoreV1().Nodes().List(ctx, metav1.ListOptions{}) + if err != nil { + return clusterHealth{verifyErr: err} + } + h := clusterHealth{totalNodes: len(nodes.Items)} + for _, n := range nodes.Items { + for _, cond := range n.Status.Conditions { + if cond.Type == corev1.NodeReady && cond.Status == corev1.ConditionTrue { + h.readyNodes++ + break + } + } + } + classes, err := client.StorageV1().StorageClasses().List(ctx, metav1.ListOptions{}) + if err != nil { + h.verifyErr = err + return h + } + for _, sc := range classes.Items { + if sc.Annotations["storageclass.kubernetes.io/is-default-class"] == "true" { + h.hasDefaultStorageClass = true + break + } + } + return h +} + +// summaryStatusLines renders the STATUS and NODES rows of the creation box +// from what was VERIFIED, never from the provisioner's exit code alone: the +// old hardcoded "Ready" printed identically over NotReady nodes or a cluster +// that could not bind a single PVC. +func summaryStatusLines(h clusterHealth, configuredNodes int) (status, nodes string) { + switch { + case h.verifyErr != nil: + return pterm.Yellow("Provisioned (health not verified — API unreachable)"), + fmt.Sprintf("%d (configured)", configuredNodes) + case h.healthy(): + return pterm.Green("Ready"), fmt.Sprintf("%d/%d Ready", h.readyNodes, h.totalNodes) + default: + return pterm.Yellow(fmt.Sprintf("Provisioned — %d/%d nodes Ready", h.readyNodes, h.totalNodes)), + fmt.Sprintf("%d/%d Ready", h.readyNodes, h.totalNodes) + } +} + +// displayClusterCreationSummary displays a summary after cluster creation. +// The STATUS row states what was verified via the API, not assumed. +func (s *ClusterService) displayClusterCreationSummary(ctx context.Context, info models.ClusterInfo, restConfig *rest.Config) { pterm.DefaultBasicText.Println() + health := clusterHealth{verifyErr: fmt.Errorf("no rest config")} + if restConfig != nil { + health = verifyClusterHealth(ctx, restConfig) + } + statusLine, nodesLine := summaryStatusLines(health, info.NodeCount) + // Create a clean box for the summary boxContent := fmt.Sprintf( "NAME: %s\n"+ "TYPE: %s\n"+ "STATUS: %s\n"+ - "NODES: %d", + "NODES: %s", pterm.Bold.Sprint(info.Name), strings.ToUpper(string(info.Type)), - pterm.Green("Ready"), - info.NodeCount, + statusLine, + nodesLine, ) // The k3d- Docker network is k3d-specific; a cloud cluster shows its // region instead so the box never invents a k3d network for GKE/EKS. @@ -772,10 +870,24 @@ func (s *ClusterService) displayClusterCreationSummary(info models.ClusterInfo) } boxContent += "\n" + apiServerLine(s.apiServerEndpoint(info.Name)) + title := " ✅ Cluster Created " + if !health.healthy() { + // The infrastructure applied, but "Created" with a green Ready would + // overstate what is known — keep the title honest too. + title = " ⚠️ Cluster Provisioned " + } pterm.DefaultBox. - WithTitle(" ✅ Cluster Created "). + WithTitle(title). WithTitleTopCenter(). Println(boxContent) + + // Name the specific gap, so the user is not left diffing a yellow word. + if health.verifyErr == nil && !health.hasDefaultStorageClass { + pterm.Warning.Println("No default StorageClass — every PersistentVolumeClaim will stay Pending until one is set") + } + if health.verifyErr == nil && health.totalNodes > 0 && health.readyNodes < health.totalNodes { + pterm.Warning.Println("Some nodes are not Ready yet — check them with: kubectl get nodes") + } } // showNextSteps displays clean next steps after cluster creation From 9c7ae8c826bf4cb7c5bed68b5e4dc814c16020aa Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Thu, 6 Aug 2026 10:51:26 +0300 Subject: [PATCH 4/8] =?UTF-8?q?fix:=20address=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20lint,=20tighter=20guards,=20portable=20fixture,=20c?= =?UTF-8?q?ontext=20matching?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - engine.go: drop redundant .Terraform selectors on promoted methods (staticcheck QF1008); the Init/Plan overrides keep the explicit selector, which is what prevents self-recursion - template_guard_test: bind the addon guard to the actual map entries via regex — the addon names also appear in comments, which could satisfy a plain Contains; before_compute is now required inside the vpc-cni block specifically - engine_test: create a terraform.exe fixture on Windows — LookPath there resolves only PATHEXT extensions, so the bare stub is invisible - cluster list context: beyond the plain cluster name, recognize the conventional context names (EKS: the ARN 'aws eks update-kubeconfig' writes, matched by prefix/suffix since the record has no account id; GKE: gcloud's gke___) — the same candidate shapes discovery matches --- internal/cluster/providers/eks/provider.go | 26 +++++++++++++----- .../cluster/providers/eks/provider_test.go | 25 +++++++++++++++++ .../providers/eks/template_guard_test.go | 8 +++--- internal/cluster/providers/gke/provider.go | 26 +++++++++++++----- .../cluster/providers/gke/provider_test.go | 25 +++++++++++++++++ .../cluster/providers/terraform/engine.go | 9 ++++--- .../providers/terraform/engine_test.go | 6 +++++ .../providers/terraform/kubecontext.go | 27 ++++++++++++++++--- 8 files changed, 129 insertions(+), 23 deletions(-) diff --git a/internal/cluster/providers/eks/provider.go b/internal/cluster/providers/eks/provider.go index 083f76a1..79b2ef79 100644 --- a/internal/cluster/providers/eks/provider.go +++ b/internal/cluster/providers/eks/provider.go @@ -404,15 +404,27 @@ func (p *Provider) GetKubeconfig(ctx context.Context, name string, clusterType m return string(data), nil } -// infoFor maps a registry record onto the shared ClusterInfo shape. -func infoFor(rec tfengine.Record) models.ClusterInfo { - // The kubeconfig context exists only once a successful create merged it — - // report what the kubeconfig holds, never a value fabricated from the - // cluster name (a plan-stage failure has no context at all). - kubeContext := "" +// kubeContextFor resolves the kubeconfig context that reaches this cluster, +// or "" when none exists — never a value fabricated from the cluster name (a +// plan-stage failure has no context at all). Beyond the plain name our merge +// writes, it recognizes the ARN-named context `aws eks update-kubeconfig` +// creates (arn:aws:eks:::cluster/) — the same +// candidate shapes discovery's matchEKSContext uses; the account id is not in +// the record, so the ARN is matched by its fixed prefix and suffix. +func kubeContextFor(rec tfengine.Record) string { if tfengine.KubeconfigHasContext(rec.Name) { - kubeContext = rec.Name + return rec.Name } + prefix := "arn:aws:eks:" + rec.Region + ":" + suffix := ":cluster/" + rec.Name + return tfengine.KubeconfigContextMatching(func(name string) bool { + return strings.HasPrefix(name, prefix) && strings.HasSuffix(name, suffix) + }) +} + +// infoFor maps a registry record onto the shared ClusterInfo shape. +func infoFor(rec tfengine.Record) models.ClusterInfo { + kubeContext := kubeContextFor(rec) return models.ClusterInfo{ Name: rec.Name, Type: models.ClusterTypeEKS, diff --git a/internal/cluster/providers/eks/provider_test.go b/internal/cluster/providers/eks/provider_test.go index cceb71bf..583e3fad 100644 --- a/internal/cluster/providers/eks/provider_test.go +++ b/internal/cluster/providers/eks/provider_test.go @@ -433,3 +433,28 @@ func TestCreateCluster_ResumeRefreshesStatusAndCreatedAt(t *testing.T) { assert.True(t, resumed.CreatedAt.After(failed.CreatedAt), "CREATED must reflect when the cluster became Ready, not the first failed attempt") } + +// A cluster reachable only through the ARN-named context that +// `aws eks update-kubeconfig` writes must still show that context — same +// candidate shapes as discovery's matchEKSContext. +func TestInfoFor_MatchesARNContext(t *testing.T) { + kubeconfig := filepath.Join(t.TempDir(), "config") + require.NoError(t, os.WriteFile(kubeconfig, []byte(` +apiVersion: v1 +kind: Config +contexts: +- name: arn:aws:eks:us-east-1:719857072830:cluster/my-eks + context: + cluster: c + user: u +`), 0o600)) + t.Setenv("KUBECONFIG", kubeconfig) + + rec := tfengine.Record{Name: "my-eks", Region: "us-east-1"} + assert.Equal(t, "arn:aws:eks:us-east-1:719857072830:cluster/my-eks", infoFor(rec).Context) + + assert.Empty(t, infoFor(tfengine.Record{Name: "other", Region: "us-east-1"}).Context, + "an ARN context for a different cluster must not match") + assert.Empty(t, infoFor(tfengine.Record{Name: "my-eks", Region: "eu-west-1"}).Context, + "an ARN context in a different region must not match") +} diff --git a/internal/cluster/providers/eks/template_guard_test.go b/internal/cluster/providers/eks/template_guard_test.go index ce939516..5a10496d 100644 --- a/internal/cluster/providers/eks/template_guard_test.go +++ b/internal/cluster/providers/eks/template_guard_test.go @@ -52,12 +52,14 @@ func TestTemplate_OperatorCanReachTheCluster(t *testing.T) { // template. func TestTemplate_DeclaresCoreAddons(t *testing.T) { src := string(mainTF) + // Match the addon map ENTRIES, not the raw source: the addon names also + // appear in comments, which must not be able to satisfy this guard. for _, addon := range []string{"vpc-cni", "kube-proxy", "coredns"} { - assert.Containsf(t, src, addon, + assert.Regexpf(t, `(?m)^\s*`+regexp.QuoteMeta(addon)+`\s*=\s*\{`, src, "module v21 installs no addons by itself — %s must be declared or the cluster is born broken", addon) } - assert.Contains(t, src, "before_compute = true", - "vpc-cni must install before the node group, or nodes wait on a missing CNI") + assert.Regexp(t, `(?ms)^\s*vpc-cni\s*=\s*\{[^}]*^\s*before_compute\s*=\s*true`, src, + "vpc-cni itself must install before the node group, or nodes wait on a missing CNI") } // The EBS CSI controller is a regular pod reaching the node role via IMDS — diff --git a/internal/cluster/providers/gke/provider.go b/internal/cluster/providers/gke/provider.go index 39a927bb..9e809fe6 100644 --- a/internal/cluster/providers/gke/provider.go +++ b/internal/cluster/providers/gke/provider.go @@ -488,15 +488,27 @@ func (p *Provider) GetKubeconfig(ctx context.Context, name string, clusterType m return string(data), nil } -// infoFor maps a registry record onto the shared ClusterInfo shape. -func infoFor(rec tfengine.Record) models.ClusterInfo { - // The kubeconfig context exists only once a successful create merged it — - // report what the kubeconfig holds, never a value fabricated from the - // cluster name (a plan-stage failure has no context at all). - kubeContext := "" +// kubeContextFor resolves the kubeconfig context that reaches this cluster, +// or "" when none exists — never a value fabricated from the cluster name (a +// plan-stage failure has no context at all). Beyond the plain name our merge +// writes, it recognizes the gke___ context `gcloud +// container clusters get-credentials` creates — the same candidate shape +// discovery's matchContext uses; the location (zone or region) is matched +// loosely since the record stores only the region. +func kubeContextFor(rec tfengine.Record) string { if tfengine.KubeconfigHasContext(rec.Name) { - kubeContext = rec.Name + return rec.Name } + prefix := "gke_" + rec.Project + "_" + suffix := "_" + rec.Name + return tfengine.KubeconfigContextMatching(func(name string) bool { + return strings.HasPrefix(name, prefix) && strings.HasSuffix(name, suffix) + }) +} + +// infoFor maps a registry record onto the shared ClusterInfo shape. +func infoFor(rec tfengine.Record) models.ClusterInfo { + kubeContext := kubeContextFor(rec) return models.ClusterInfo{ Name: rec.Name, Type: models.ClusterTypeGKE, diff --git a/internal/cluster/providers/gke/provider_test.go b/internal/cluster/providers/gke/provider_test.go index ba9c5123..a8160404 100644 --- a/internal/cluster/providers/gke/provider_test.go +++ b/internal/cluster/providers/gke/provider_test.go @@ -588,3 +588,28 @@ func TestPlanCluster_ExistingWorkspacePreviewsResumeWithoutSideEffects(t *testin require.NoError(t, err) assert.Equal(t, `{"version":4}`, string(afterState)) } + +// A cluster reachable only through the gke___ +// context that `gcloud container clusters get-credentials` writes must still +// show that context — same candidate shape as discovery's matchContext. +func TestInfoFor_MatchesGcloudContext(t *testing.T) { + kubeconfig := filepath.Join(t.TempDir(), "config") + require.NoError(t, os.WriteFile(kubeconfig, []byte(` +apiVersion: v1 +kind: Config +contexts: +- name: gke_my-project_us-central1-a_my-gke + context: + cluster: c + user: u +`), 0o600)) + t.Setenv("KUBECONFIG", kubeconfig) + + rec := tfengine.Record{Name: "my-gke", Project: "my-project", Region: "us-central1"} + assert.Equal(t, "gke_my-project_us-central1-a_my-gke", infoFor(rec).Context) + + assert.Empty(t, infoFor(tfengine.Record{Name: "other", Project: "my-project"}).Context, + "a gcloud context for a different cluster must not match") + assert.Empty(t, infoFor(tfengine.Record{Name: "my-gke", Project: "other-project"}).Context, + "a gcloud context in a different project must not match") +} diff --git a/internal/cluster/providers/terraform/engine.go b/internal/cluster/providers/terraform/engine.go index aa169f85..c07c44e3 100644 --- a/internal/cluster/providers/terraform/engine.go +++ b/internal/cluster/providers/terraform/engine.go @@ -91,14 +91,17 @@ type verboseRunner struct { // the first command of an instance — priming it first keeps even that blob // off the terminal. func (r *verboseRunner) withStdout(ctx context.Context, fn func() error) error { - if _, _, err := r.Terraform.Version(ctx, false); err != nil { + if _, _, err := r.Version(ctx, false); err != nil { return err } - r.Terraform.SetStdout(os.Stdout) - defer r.Terraform.SetStdout(io.Discard) + r.SetStdout(os.Stdout) + defer r.SetStdout(io.Discard) return fn() } +// The overrides below must call through r.Terraform explicitly — a bare +// r.Init/r.Plan would recurse into the override itself. + func (r *verboseRunner) Init(ctx context.Context, opts ...tfexec.InitOption) error { return r.withStdout(ctx, func() error { return r.Terraform.Init(ctx, opts...) }) } diff --git a/internal/cluster/providers/terraform/engine_test.go b/internal/cluster/providers/terraform/engine_test.go index 39cfe492..10ff54a0 100644 --- a/internal/cluster/providers/terraform/engine_test.go +++ b/internal/cluster/providers/terraform/engine_test.go @@ -7,6 +7,7 @@ import ( "io" "os" "path/filepath" + "runtime" "testing" "github.com/hashicorp/terraform-exec/tfexec" @@ -241,6 +242,11 @@ func TestNewEngine_VerboseWrapsRunnerForSelectiveStdout(t *testing.T) { binDir := filepath.Join(home, "bin") require.NoError(t, os.MkdirAll(binDir, 0o750)) require.NoError(t, os.WriteFile(filepath.Join(binDir, "terraform"), []byte("#!/bin/sh\n"), 0o750)) // #nosec G306 -- must be executable for LookPath + if runtime.GOOS == "windows" { + // Windows LookPath resolves only PATHEXT extensions, so the bare + // "terraform" fixture is invisible there — provide terraform.exe too. + require.NoError(t, os.WriteFile(filepath.Join(binDir, "terraform.exe"), []byte("stub"), 0o750)) // #nosec G306 + } t.Setenv("PATH", binDir) verbose, err := NewEngine(true).newRunner(t.TempDir()) diff --git a/internal/cluster/providers/terraform/kubecontext.go b/internal/cluster/providers/terraform/kubecontext.go index a52761ca..9f80ed02 100644 --- a/internal/cluster/providers/terraform/kubecontext.go +++ b/internal/cluster/providers/terraform/kubecontext.go @@ -1,6 +1,8 @@ package terraform import ( + "sort" + "k8s.io/client-go/tools/clientcmd" ) @@ -14,10 +16,29 @@ import ( // list used to print one fabricated from the name. Any read error counts as // "no context": inventing one is the failure mode this exists to prevent. func KubeconfigHasContext(name string) bool { + return KubeconfigContextMatching(func(have string) bool { return have == name }) != "" +} + +// KubeconfigContextMatching returns the first kubeconfig context (in sorted +// order, so the result is deterministic) accepted by match, or "". It lets +// providers recognize the conventional context names beyond the plain cluster +// name — e.g. the ARN-named context `aws eks update-kubeconfig` writes, or +// gcloud's gke___ — mirroring the candidate shapes +// the discovery package matches. +func KubeconfigContextMatching(match func(name string) bool) string { cfg, err := clientcmd.NewDefaultClientConfigLoadingRules().Load() if err != nil || cfg == nil { - return false + return "" + } + names := make([]string, 0, len(cfg.Contexts)) + for name := range cfg.Contexts { + names = append(names, name) + } + sort.Strings(names) + for _, name := range names { + if match(name) { + return name + } } - _, ok := cfg.Contexts[name] - return ok + return "" } From cbcc9e5fd75cb433628d15ec2fafb7e5a3dc5579 Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Thu, 6 Aug 2026 11:01:51 +0300 Subject: [PATCH 5/8] ci: work around Go 1.26 windows-amd64 runtime crash in unit tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Windows unit-test leg dies at random with 'Exception 0xc0000005 / unknown pc' — a Go 1.26.0/1.26.1 runtime bug corrupting return addresses on the stack during GC (suspected Green Tea GC; golang/go#77975, release-blocking, fix targeted at a patch release). The argocd test binary happened to be the victim this run; any package can be next. Our code is not implicated: the suite is race-clean under -race and the crash is inside the runtime during test cleanup. Disable the new GC via GOEXPERIMENT=nogreenteagc on the Windows leg only; drop this once a fixed Go patch release is picked up. --- .github/workflows/test.yml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 9011ff7b..56ba01f7 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -141,6 +141,14 @@ jobs: - name: Run unit tests shell: bash + env: + # Go 1.26.0/1.26.1 crash memory at random on windows-amd64 — a + # runtime/GC bug corrupting return addresses during stack scanning + # (suspected Green Tea GC; golang/go#77975, release-blocking). It + # bit this job as "Exception 0xc0000005 / unknown pc" in the argocd + # test binary. Disable the new GC on the Windows leg until a fixed + # Go patch release lands, then drop this. + GOEXPERIMENT: ${{ matrix.os == 'windows' && 'nogreenteagc' || '' }} run: | echo "===================================================================" echo "=== TEST: unit tests (root, cmd, internal, tests/testutil, tests/scripts)" From a8bada0eaba5a5522fbeaf19c443e2aaa45695fb Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Thu, 6 Aug 2026 11:19:30 +0300 Subject: [PATCH 6/8] =?UTF-8?q?build:=20pin=20toolchain=20go1.26.5=20?= =?UTF-8?q?=E2=80=94=20the=20windows-amd64=20GC=20crash=20fix=20shipped=20?= =?UTF-8?q?in=201.26.2?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit golang/go#77975 (return addresses corrupted during GC stack scanning, 'Exception 0xc0000005 / unknown pc' at random on windows-amd64) was fixed in Go 1.26.2 (backport golang/go#78041, closed). CI and goreleaser both resolve the toolchain from go.mod via go-version-file, so the toolchain directive is the single switch that moves unit tests, e2e AND the shipped Windows release binaries off the buggy 1.26.0 runtime. Drop the GOEXPERIMENT=nogreenteagc workaround from the Windows unit-test leg — with a fixed toolchain it only kept a non-default GC configuration alive in CI. --- .github/workflows/test.yml | 8 -------- go.mod | 6 ++++++ 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 56ba01f7..9011ff7b 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -141,14 +141,6 @@ jobs: - name: Run unit tests shell: bash - env: - # Go 1.26.0/1.26.1 crash memory at random on windows-amd64 — a - # runtime/GC bug corrupting return addresses during stack scanning - # (suspected Green Tea GC; golang/go#77975, release-blocking). It - # bit this job as "Exception 0xc0000005 / unknown pc" in the argocd - # test binary. Disable the new GC on the Windows leg until a fixed - # Go patch release lands, then drop this. - GOEXPERIMENT: ${{ matrix.os == 'windows' && 'nogreenteagc' || '' }} run: | echo "===================================================================" echo "=== TEST: unit tests (root, cmd, internal, tests/testutil, tests/scripts)" diff --git a/go.mod b/go.mod index 8a25fc72..2d958aef 100644 --- a/go.mod +++ b/go.mod @@ -2,6 +2,12 @@ module github.com/flamingo-stack/openframe-cli go 1.26.0 +// 1.26.0/1.26.1 crash at random on windows-amd64 (return addresses corrupted +// during GC stack scanning — golang/go#77975); the fix shipped in 1.26.2. +// CI and releases resolve the toolchain from this file, so this line is what +// keeps the shipped Windows binaries off the buggy runtimes. +toolchain go1.26.5 + require ( github.com/charmbracelet/bubbletea v1.3.10 github.com/charmbracelet/huh v1.0.0 From 170dfab451688ae449d0ca2ccbca024028e0b81d Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Thu, 6 Aug 2026 11:33:03 +0300 Subject: [PATCH 7/8] chore: apply go get -u ./... --- go.mod | 4 ++-- go.sum | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index 2d958aef..60dac980 100644 --- a/go.mod +++ b/go.mod @@ -60,7 +60,7 @@ require ( github.com/charmbracelet/x/term v0.2.2 // indirect github.com/clipperhouse/displaywidth v0.11.0 // indirect github.com/clipperhouse/uax29/v2 v2.7.0 // indirect - github.com/cloudflare/circl v1.6.4 // indirect + github.com/cloudflare/circl v1.6.5 // indirect github.com/containerd/console v1.0.5 // indirect github.com/cyberphone/json-canonicalization v0.0.0-20241213102144-19d51d7fe467 // indirect github.com/cyphar/filepath-securejoin v0.7.0 // indirect @@ -102,7 +102,7 @@ require ( github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8 // indirect github.com/google/certificate-transparency-go v1.3.3 // indirect github.com/google/gnostic-models v0.7.1 // indirect - github.com/google/go-containerregistry v0.21.8 // indirect + github.com/google/go-containerregistry v0.21.9 // indirect github.com/google/uuid v1.6.0 // indirect github.com/gookit/color v1.6.1 // indirect github.com/grpc-ecosystem/grpc-gateway/v2 v2.30.0 // indirect diff --git a/go.sum b/go.sum index a2d832d4..ad039cd3 100644 --- a/go.sum +++ b/go.sum @@ -139,8 +139,8 @@ github.com/clipperhouse/displaywidth v0.11.0 h1:lBc6kY44VFw+TDx4I8opi/EtL9m20WSE github.com/clipperhouse/displaywidth v0.11.0/go.mod h1:bkrFNkf81G8HyVqmKGxsPufD3JhNl3dSqnGhOoSD/o0= github.com/clipperhouse/uax29/v2 v2.7.0 h1:+gs4oBZ2gPfVrKPthwbMzWZDaAFPGYK72F0NJv2v7Vk= github.com/clipperhouse/uax29/v2 v2.7.0/go.mod h1:EFJ2TJMRUaplDxHKj1qAEhCtQPW2tJSwu5BF98AuoVM= -github.com/cloudflare/circl v1.6.4 h1:pOXuDTCEYyzydgUpQ0CQz3LsinKjiSk6nNP5Lt5K64U= -github.com/cloudflare/circl v1.6.4/go.mod h1:YxarevkLlbaHuWsxG6vmYNWBEsSp4pnp7j+4VljMavY= +github.com/cloudflare/circl v1.6.5 h1:O64F26HEqNhznd/hrC5KZXVKYuKM2rx4deZDTc4ihQA= +github.com/cloudflare/circl v1.6.5/go.mod h1:h5LNyxAc5nTue9DS5jT+48en2PSDYt3zdGnz5OstK6c= github.com/codahale/rfc6979 v0.0.0-20141003034818-6a90f24967eb h1:EDmT6Q9Zs+SbUoc7Ik9EfrFqcylYqgPZ9ANSbTAntnE= github.com/codahale/rfc6979 v0.0.0-20141003034818-6a90f24967eb/go.mod h1:ZjrT6AXHbDs86ZSdt/osfBi5qfexBrKUdONk989Wnk4= github.com/containerd/console v1.0.5 h1:R0ymNeydRqH2DmakFNdmjR2k0t7UPuiOV/N/27/qqsc= @@ -270,8 +270,8 @@ github.com/google/gnostic-models v0.7.1 h1:SisTfuFKJSKM5CPZkffwi6coztzzeYUhc3v4y github.com/google/gnostic-models v0.7.1/go.mod h1:whL5G0m6dmc5cPxKc5bdKdEN3UjI7OUGxBlw57miDrQ= github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= -github.com/google/go-containerregistry v0.21.8 h1:Ig/zIsnztdCUNaiNNczE+MoP5xcyUMfvpvfOr1xyMLE= -github.com/google/go-containerregistry v0.21.8/go.mod h1:dP5XNKcL7kMFF/TB3LfvWmVhAcv7iqkHb3oDK8aauTo= +github.com/google/go-containerregistry v0.21.9 h1:F+D4uZ3iA3DLMJLfhaqMdHJbzeqm/216WGQq2dokuLs= +github.com/google/go-containerregistry v0.21.9/go.mod h1:dP5XNKcL7kMFF/TB3LfvWmVhAcv7iqkHb3oDK8aauTo= github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= github.com/google/gofuzz v1.2.0 h1:xRy4A+RhZaiKjJ1bPfwQ8sedCA+YS2YcCHW6ec7JMi0= github.com/google/gofuzz v1.2.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= From a4300fba47e26ce23f68be6be46afc2f58fac0c0 Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Thu, 6 Aug 2026 11:43:46 +0300 Subject: [PATCH 8/8] fix(gke): require the recorded region in gcloud-context matching The gke___ match checked only project and name: a same-name, same-project cluster in ANOTHER region satisfied the prefix/suffix too, and the lexically first kubeconfig context won regardless of the record. The location segment must now be the recorded region itself (regional cluster) or a zone inside it (region + "-" prefix, e.g. us-central1-a); us-east10 does not false-match us-east1 since its next rune is not a hyphen. --- internal/cluster/providers/gke/provider.go | 13 ++++++-- .../cluster/providers/gke/provider_test.go | 30 +++++++++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/internal/cluster/providers/gke/provider.go b/internal/cluster/providers/gke/provider.go index 9e809fe6..da888460 100644 --- a/internal/cluster/providers/gke/provider.go +++ b/internal/cluster/providers/gke/provider.go @@ -493,8 +493,11 @@ func (p *Provider) GetKubeconfig(ctx context.Context, name string, clusterType m // plan-stage failure has no context at all). Beyond the plain name our merge // writes, it recognizes the gke___ context `gcloud // container clusters get-credentials` creates — the same candidate shape -// discovery's matchContext uses; the location (zone or region) is matched -// loosely since the record stores only the region. +// discovery's matchContext uses. The record stores only the region, so the +// location segment must be the region itself (regional cluster) or a zone +// inside it ("us-central1-a") — without that check, a same-name same-project +// cluster in ANOTHER region could satisfy the prefix/suffix alone and the +// lexically first context would win. func kubeContextFor(rec tfengine.Record) string { if tfengine.KubeconfigHasContext(rec.Name) { return rec.Name @@ -502,7 +505,11 @@ func kubeContextFor(rec tfengine.Record) string { prefix := "gke_" + rec.Project + "_" suffix := "_" + rec.Name return tfengine.KubeconfigContextMatching(func(name string) bool { - return strings.HasPrefix(name, prefix) && strings.HasSuffix(name, suffix) + if !strings.HasPrefix(name, prefix) || !strings.HasSuffix(name, suffix) { + return false + } + location := strings.TrimSuffix(strings.TrimPrefix(name, prefix), suffix) + return location == rec.Region || strings.HasPrefix(location, rec.Region+"-") }) } diff --git a/internal/cluster/providers/gke/provider_test.go b/internal/cluster/providers/gke/provider_test.go index a8160404..ade1dd73 100644 --- a/internal/cluster/providers/gke/provider_test.go +++ b/internal/cluster/providers/gke/provider_test.go @@ -612,4 +612,34 @@ contexts: "a gcloud context for a different cluster must not match") assert.Empty(t, infoFor(tfengine.Record{Name: "my-gke", Project: "other-project"}).Context, "a gcloud context in a different project must not match") + assert.Empty(t, infoFor(tfengine.Record{Name: "my-gke", Project: "my-project", Region: "europe-west1"}).Context, + "a gcloud context in a different region must not match") +} + +// A same-name, same-project cluster living in TWO regions must resolve to the +// recorded region's context — the prefix/suffix alone accepts both, and the +// lexically first (europe before us) would otherwise win regardless of the +// record. +func TestInfoFor_SameNameAcrossRegionsPicksRecordedRegion(t *testing.T) { + kubeconfig := filepath.Join(t.TempDir(), "config") + require.NoError(t, os.WriteFile(kubeconfig, []byte(` +apiVersion: v1 +kind: Config +contexts: +- name: gke_my-project_europe-west1_my-gke + context: + cluster: c + user: u +- name: gke_my-project_us-central1-a_my-gke + context: + cluster: c + user: u +`), 0o600)) + t.Setenv("KUBECONFIG", kubeconfig) + + assert.Equal(t, "gke_my-project_us-central1-a_my-gke", + infoFor(tfengine.Record{Name: "my-gke", Project: "my-project", Region: "us-central1"}).Context) + assert.Equal(t, "gke_my-project_europe-west1_my-gke", + infoFor(tfengine.Record{Name: "my-gke", Project: "my-project", Region: "europe-west1"}).Context, + "the regional (zone-less) context form must match its region exactly") }