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/go.mod b/go.mod index 8a25fc72..60dac980 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 @@ -54,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 @@ -96,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= 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/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/models/flags.go b/internal/cluster/models/flags.go index 3c116ebe..6cb3e025 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.go b/internal/cluster/providers/eks/provider.go index f0848b11..79b2ef79 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 } @@ -396,13 +404,32 @@ func (p *Provider) GetKubeconfig(ctx context.Context, name string, clusterType m return string(data), nil } +// 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) { + 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, 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 b4699441..583e3fad 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" @@ -246,9 +247,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") } @@ -379,3 +378,83 @@ 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") +} + +// 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.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..5a10496d 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,64 @@ 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) + // 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.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.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 — +// 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/gke/provider.go b/internal/cluster/providers/gke/provider.go index 7e8cbc1a..da888460 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 } @@ -480,13 +488,39 @@ func (p *Provider) GetKubeconfig(ctx context.Context, name string, clusterType m return string(data), nil } +// 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 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 + } + prefix := "gke_" + rec.Project + "_" + suffix := "_" + rec.Name + return tfengine.KubeconfigContextMatching(func(name string) bool { + 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+"-") + }) +} + // 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, Source: models.SourceOpenframe, - Context: rec.Name, + Context: kubeContext, Project: rec.Project, Region: rec.Region, Status: rec.Status.Title(), diff --git a/internal/cluster/providers/gke/provider_test.go b/internal/cluster/providers/gke/provider_test.go index ba9c5123..ade1dd73 100644 --- a/internal/cluster/providers/gke/provider_test.go +++ b/internal/cluster/providers/gke/provider_test.go @@ -588,3 +588,58 @@ 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") + 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") +} diff --git a/internal/cluster/providers/terraform/engine.go b/internal/cluster/providers/terraform/engine.go index 9c5b8283..c07c44e3 100644 --- a/internal/cluster/providers/terraform/engine.go +++ b/internal/cluster/providers/terraform/engine.go @@ -65,14 +65,57 @@ 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.Version(ctx, false); err != nil { + return err + } + 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...) }) +} + +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..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" @@ -229,3 +230,32 @@ 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 + 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()) + 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/providers/terraform/kubecontext.go b/internal/cluster/providers/terraform/kubecontext.go new file mode 100644 index 00000000..9f80ed02 --- /dev/null +++ b/internal/cluster/providers/terraform/kubecontext.go @@ -0,0 +1,44 @@ +package terraform + +import ( + "sort" + + "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 { + 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 "" + } + 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 + } + } + return "" +} 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 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..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)") } @@ -182,6 +187,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 +211,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:]) } } } @@ -262,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 { @@ -273,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 53a0350f..60aa6416 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 @@ -425,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") + }) +}