diff --git a/internal/bootstrap/service.go b/internal/bootstrap/service.go index 2fd76e88..6b3aca93 100644 --- a/internal/bootstrap/service.go +++ b/internal/bootstrap/service.go @@ -75,6 +75,13 @@ func (s *Service) bootstrap(ctx context.Context, clusterName string, nonInteract actualClusterName = defaultClusterName } + // Step 0: Pre-flight the helm values file BEFORE creating the cluster. A + // malformed `argocd:` override (or unparseable YAML) otherwise costs a full + // cluster create before the chart install rejects the same file. + if err := chartServices.ValidateHelmValuesFile(); err != nil { + return err + } + // Step 1: Create cluster with suppressed UI and get the rest.Config kubeConfig, err := s.createClusterSuppressed(ctx, actualClusterName, verbose, nonInteractive) if err != nil { diff --git a/internal/chart/providers/argocd/assess.go b/internal/chart/providers/argocd/assess.go index a4cdad98..b7f5f4d8 100644 --- a/internal/chart/providers/argocd/assess.go +++ b/internal/chart/providers/argocd/assess.go @@ -74,6 +74,11 @@ var repoServerErrorPatterns = []string{ // those whose condition message points at repo-server trouble. issueCounts is // updated in place: incremented for apps showing a repo-server error and // cleared for apps that no longer do. +// +// Deterministic manifest errors (see fatalmanifest.go) are excluded even +// though they match "failed to generate manifest": restarting the repo-server +// cannot make a missing chart path appear, so counting them here only produced +// pointless recovery restarts before the fail-fast fired. func classifyAppIssues(apps []Application, issueCounts map[string]int) (unknown, conditionErrors []Application) { for _, app := range apps { if app.Health == ArgoCDStatusUnknown || app.Sync == ArgoCDStatusUnknown { @@ -81,7 +86,7 @@ func classifyAppIssues(apps []Application, issueCounts map[string]int) (unknown, } hasRepoErr := false - if app.Condition != "" { + if app.Condition != "" && !isDeterministicManifestError(app.Condition) { for _, p := range repoServerErrorPatterns { if strings.Contains(app.Condition, p) { hasRepoErr = true diff --git a/internal/chart/providers/argocd/fatalmanifest.go b/internal/chart/providers/argocd/fatalmanifest.go new file mode 100644 index 00000000..7126fc17 --- /dev/null +++ b/internal/chart/providers/argocd/fatalmanifest.go @@ -0,0 +1,151 @@ +package argocd + +import ( + "fmt" + "sort" + "strings" + "time" +) + +// Fail-fast for deterministic manifest-generation failures (0.4.9 verification +// observation): a legacy ref whose chart path does not exist at the pinned +// revision produces a persistent ComparisonError ("Manifest generation error +// ... no such file or directory"). No retry, repo-server restart, or amount of +// waiting can change that outcome — the repo-server fetched the repo fine, the +// content simply isn't there. The wait previously classified it as repo-server +// trouble (restarting the repo-server up to 3 times) and then rode out the +// full timeout (20+ minutes observed) before failing. + +// manifestErrorContexts identify a condition as a manifest-generation failure +// at all; only then are the deterministic patterns consulted. This keeps a +// stray "no such file or directory" from an unrelated condition from tripping +// the fail-fast. +var manifestErrorContexts = []string{ + "failed to generate manifest", + "Manifest generation error", + "Unable to generate manifests", +} + +// deterministicManifestPatterns are fragments that make a manifest-generation +// failure deterministic: the requested path/revision content does not exist, +// so the error is permanent for this install. Transient repo-server trouble +// (EOF, Unavailable, connection resets) must NOT be listed here — those stay +// on the recovery path in classifyAppIssues. +var deterministicManifestPatterns = []string{ + "no such file or directory", + "app path does not exist", +} + +// isDeterministicManifestError reports whether a condition message describes a +// manifest-generation failure that waiting cannot fix. +func isDeterministicManifestError(condition string) bool { + if condition == "" { + return false + } + inManifestContext := false + for _, c := range manifestErrorContexts { + if strings.Contains(condition, c) { + inManifestContext = true + break + } + } + if !inManifestContext { + return false + } + for _, p := range deterministicManifestPatterns { + if strings.Contains(condition, p) { + return true + } + } + return false +} + +// fatalManifestAfter is how long an application's deterministic manifest error +// must persist before the wait fails fast. Long enough to survive a one-off +// condition observed mid-sync (ArgoCD caches and re-evaluates manifests during +// the first waves); short enough to save the 20+ minute ride to the timeout. +const fatalManifestAfter = 2 * time.Minute + +// fatalManifestMinChecks is the minimum number of consecutive observations of +// the same deterministic error. Guards against a wall-clock threshold being +// crossed by two isolated sightings around a long connectivity gap. +const fatalManifestMinChecks = 5 + +// fatalManifestTracker records, per application, how long a deterministic +// manifest error has persisted. Mirrors stallTracker's shape: reset on change, +// forget on disappearance. +type fatalManifestTracker struct { + entries map[string]fatalManifestEntry +} + +type fatalManifestEntry struct { + since time.Time // when the deterministic error was first observed + checks int // consecutive observations +} + +func newFatalManifestTracker() *fatalManifestTracker { + return &fatalManifestTracker{entries: make(map[string]fatalManifestEntry)} +} + +// observe records this tick's applications and returns those whose +// deterministic manifest error has persisted past both thresholds. An app that +// stops showing the error (or becomes ready) is forgotten, so its clock starts +// fresh if the error ever returns. +func (t *fatalManifestTracker) observe(apps []Application, now time.Time) []Application { + var fatal []Application + seen := make(map[string]bool, len(apps)) + for _, app := range apps { + ready := app.Health == ArgoCDHealthHealthy && app.Sync == ArgoCDSyncSynced + if ready || !isDeterministicManifestError(app.Condition) { + continue + } + seen[app.Name] = true + e, ok := t.entries[app.Name] + if !ok { + e = fatalManifestEntry{since: now} + } + e.checks++ + t.entries[app.Name] = e + if e.checks >= fatalManifestMinChecks && now.Sub(e.since) >= fatalManifestAfter { + fatal = append(fatal, app) + } + } + for name := range t.entries { + if !seen[name] { + delete(t.entries, name) + } + } + sort.Slice(fatal, func(i, j int) bool { return fatal[i].Name < fatal[j].Name }) + return fatal +} + +// maxConditionInError bounds how much of a condition message lands in the +// fail-fast error; ArgoCD prefixes them with several layers of rpc wrapping. +const maxConditionInError = 300 + +// fatalManifestError renders the fail-fast error. When a non-default ref was +// requested it names the likely cause — the same legacy-ref situation +// refMismatchError covers, except here the children DID honor the pin and the +// pinned revision simply lacks the expected chart content. +func fatalManifestError(requestedRef string, apps []Application) error { + var b strings.Builder + fmt.Fprintf(&b, "%d application(s) cannot render their manifests from the deployed revision; "+ + "this error is deterministic — retrying or waiting cannot fix it:\n", len(apps)) + for _, app := range apps { + cond := app.Condition + if len(cond) > maxConditionInError { + cond = cond[:maxConditionInError] + "..." + } + fmt.Fprintf(&b, " - %s: %s\n", app.Name, cond) + } + if !defaultRefs[strings.ToLower(strings.TrimSpace(requestedRef))] { + fmt.Fprintf(&b, "The requested ref %q likely predates the chart layout this CLI expects "+ + "(the chart path does not exist at that revision).\n"+ + "Use a branch whose chart matches this CLI, or fix the applications' path/targetRevision by hand.", + requestedRef) + } else { + b.WriteString("The chart path does not exist at the deployed revision. " + + "Inspect the application source with: kubectl describe application " + apps[0].Name + " -n argocd") + } + return fmt.Errorf("%s", b.String()) +} diff --git a/internal/chart/providers/argocd/fatalmanifest_test.go b/internal/chart/providers/argocd/fatalmanifest_test.go new file mode 100644 index 00000000..83d4b71a --- /dev/null +++ b/internal/chart/providers/argocd/fatalmanifest_test.go @@ -0,0 +1,168 @@ +package argocd + +import ( + "strings" + "testing" + "time" +) + +// legacyRefCondition is the real-world shape of the ComparisonError a legacy +// ref produces: the repo is reachable, the chart path simply does not exist at +// the pinned revision. +const legacyRefCondition = "Failed to load target state: failed to generate manifest for source 1 of 1: " + + "rpc error: code = Unknown desc = Manifest generation error (cached): " + + "/tmp/repo/manifests/oss: no such file or directory" + +func TestIsDeterministicManifestError(t *testing.T) { + cases := []struct { + name string + condition string + want bool + }{ + {"legacy ref: path missing at revision", legacyRefCondition, true}, + {"app path does not exist variant", + "Failed to load target state: Unable to generate manifests in manifests/oss: " + + "rpc error: code = Unknown desc = manifests/oss: app path does not exist", true}, + {"transient repo-server EOF stays recoverable", + "failed to generate manifest for source 1 of 1: rpc error: EOF", false}, + {"transient Unavailable stays recoverable", + "Manifest generation error: rpc error: code = Unavailable desc = connection refused", false}, + {"deterministic fragment without manifest context does not trip", + "hook failed: /scripts/setup.sh: no such file or directory", false}, + {"empty condition", "", false}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := isDeterministicManifestError(c.condition); got != c.want { + t.Fatalf("isDeterministicManifestError(%q) = %v, want %v", c.condition, got, c.want) + } + }) + } +} + +// TestFatalManifestTracker_FailsAfterPersistence is the motivating case: the +// same deterministic error, tick after tick, must eventually be reported +// instead of riding the wait to its full timeout. +func TestFatalManifestTracker_FailsAfterPersistence(t *testing.T) { + tr := newFatalManifestTracker() + app := []Application{{Name: "oss", Health: "Degraded", Sync: "OutOfSync", Condition: legacyRefCondition}} + start := time.Now() + + // Enough checks but not enough wall-clock time → not fatal yet. + for i := 0; i < fatalManifestMinChecks+2; i++ { + if fatal := tr.observe(app, start.Add(time.Duration(i)*time.Second)); len(fatal) != 0 { + t.Fatalf("fatal before fatalManifestAfter elapsed (tick %d)", i) + } + } + + // Past the wall-clock threshold with the check count already met → fatal. + fatal := tr.observe(app, start.Add(fatalManifestAfter+time.Second)) + if len(fatal) != 1 || fatal[0].Name != "oss" { + t.Fatalf("fatal = %v, want [oss]", names(fatal)) + } +} + +// TestFatalManifestTracker_TimeAloneIsNotEnough: two isolated sightings around +// a long gap must not trip the fail-fast — the check count guards it. +func TestFatalManifestTracker_TimeAloneIsNotEnough(t *testing.T) { + tr := newFatalManifestTracker() + app := []Application{{Name: "oss", Health: "Degraded", Sync: "OutOfSync", Condition: legacyRefCondition}} + start := time.Now() + + tr.observe(app, start) + if fatal := tr.observe(app, start.Add(fatalManifestAfter+time.Minute)); len(fatal) != 0 { + t.Fatalf("fatal after only 2 checks, want none (min %d)", fatalManifestMinChecks) + } +} + +// TestFatalManifestTracker_ClearsWhenErrorResolves: an app whose condition +// goes away (or that becomes ready) starts a fresh clock if it ever returns. +func TestFatalManifestTracker_ClearsWhenErrorResolves(t *testing.T) { + tr := newFatalManifestTracker() + broken := []Application{{Name: "oss", Health: "Degraded", Sync: "OutOfSync", Condition: legacyRefCondition}} + start := time.Now() + + for i := 0; i < fatalManifestMinChecks; i++ { + tr.observe(broken, start.Add(time.Duration(i)*time.Second)) + } + + // Error clears for one tick — the entry must be forgotten. + tr.observe([]Application{{Name: "oss", Health: "Progressing", Sync: "OutOfSync"}}, start.Add(time.Minute)) + + // Error returns after the original wall-clock threshold: the fresh clock + // must keep it non-fatal. + if fatal := tr.observe(broken, start.Add(fatalManifestAfter+time.Minute)); len(fatal) != 0 { + t.Fatal("tracker must reset after the error resolves") + } +} + +// TestFatalManifestTracker_ReadyAppNeverFatal: a stale condition on an app +// that is currently Healthy+Synced must never fail the install. +func TestFatalManifestTracker_ReadyAppNeverFatal(t *testing.T) { + tr := newFatalManifestTracker() + app := []Application{{Name: "oss", Health: ArgoCDHealthHealthy, Sync: ArgoCDSyncSynced, Condition: legacyRefCondition}} + start := time.Now() + + for i := 0; i < fatalManifestMinChecks+1; i++ { + tr.observe(app, start.Add(time.Duration(i)*time.Second)) + } + if fatal := tr.observe(app, start.Add(fatalManifestAfter+time.Minute)); len(fatal) != 0 { + t.Fatal("ready app must never be reported fatal") + } +} + +func TestFatalManifestError_NonDefaultRefNamesTheRef(t *testing.T) { + err := fatalManifestError("release-1.0", []Application{ + {Name: "oss", Condition: legacyRefCondition}, + }) + msg := err.Error() + for _, want := range []string{"oss", "deterministic", `"release-1.0"`, "no such file or directory"} { + if !strings.Contains(msg, want) { + t.Errorf("error must contain %q, got:\n%s", want, msg) + } + } +} + +func TestFatalManifestError_DefaultRefSkipsRefHint(t *testing.T) { + err := fatalManifestError("main", []Application{{Name: "oss", Condition: legacyRefCondition}}) + msg := err.Error() + if strings.Contains(msg, "requested ref") { + t.Errorf("default ref must not produce the ref hint, got:\n%s", msg) + } + if !strings.Contains(msg, "kubectl describe application oss -n argocd") { + t.Errorf("default-ref message must give the inspect command, got:\n%s", msg) + } +} + +func TestFatalManifestError_TruncatesLongConditions(t *testing.T) { + long := legacyRefCondition + strings.Repeat(" pad", 200) + err := fatalManifestError("release-1.0", []Application{{Name: "oss", Condition: long}}) + if len(err.Error()) > maxConditionInError+500 { + t.Fatalf("condition not truncated, message length %d", len(err.Error())) + } +} + +// TestClassifyAppIssues_DeterministicErrorsBypassRecovery locks the recovery +// exclusion: a deterministic manifest error matches the broad "failed to +// generate manifest" repo-server pattern, but restarting the repo-server +// cannot fix it, so it must not be counted as a repo-server issue. +func TestClassifyAppIssues_DeterministicErrorsBypassRecovery(t *testing.T) { + counts := map[string]int{} + apps := []Application{ + {Name: "legacy", Health: "Degraded", Sync: "OutOfSync", Condition: legacyRefCondition}, + {Name: "transient", Health: "Progressing", Sync: "OutOfSync", + Condition: "failed to generate manifest for source 1 of 1: rpc error: EOF"}, + } + + _, condErrs := classifyAppIssues(apps, counts) + + if len(condErrs) != 1 || condErrs[0].Name != "transient" { + t.Fatalf("conditionErrors = %v, want [transient]", names(condErrs)) + } + if _, ok := counts["legacy"]; ok { + t.Error("deterministic error must not accumulate a recovery count") + } + if counts["transient"] != 1 { + t.Errorf("transient error must still count, got %v", counts) + } +} diff --git a/internal/chart/providers/argocd/values.go b/internal/chart/providers/argocd/values.go index b77d92f7..028af29e 100644 --- a/internal/chart/providers/argocd/values.go +++ b/internal/chart/providers/argocd/values.go @@ -3,6 +3,7 @@ package argocd import ( _ "embed" "fmt" + "os" "sort" "sigs.k8s.io/yaml" @@ -79,6 +80,31 @@ func MergedArgoCDValues(userValues map[string]interface{}) (string, []string, er return string(out), keys, nil } +// ValidateUserValuesFile is the pre-flight check for the user's values file: +// a missing file is fine (baseline install), but a file that exists must be +// readable, parse as YAML, and its `argocd:` key — when present — must be a +// mapping. Callers run this BEFORE any cluster work; previously a malformed +// override surfaced only mid-install, after a cluster create and behind an +// ArgoCD pod-diagnostics dump it had nothing to do with (0.4.9 verification +// observation). +func ValidateUserValuesFile(path string) error { + data, err := os.ReadFile(path) // #nosec G304 -- values path resolved from config/CLI, read as the invoking user + if err != nil { + if os.IsNotExist(err) { + return nil + } + return fmt.Errorf("reading values file %s: %w", path, err) + } + var m map[string]interface{} + if err := yaml.Unmarshal(data, &m); err != nil { + return fmt.Errorf("values file %s is not valid YAML: %w", path, err) + } + if _, _, err := MergedArgoCDValues(m); err != nil { + return fmt.Errorf("values file %s: %w", path, err) + } + return nil +} + // deepMerge overlays src onto dst in place: nested maps merge recursively; // scalars, lists, and new keys replace (helm's value-merge rule). func deepMerge(dst, src map[string]interface{}) { diff --git a/internal/chart/providers/argocd/values_preflight_test.go b/internal/chart/providers/argocd/values_preflight_test.go new file mode 100644 index 00000000..3977955f --- /dev/null +++ b/internal/chart/providers/argocd/values_preflight_test.go @@ -0,0 +1,69 @@ +package argocd + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func writeValuesFile(t *testing.T, content string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "openframe-helm-values.yaml") + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatalf("writing values file: %v", err) + } + return path +} + +func TestValidateUserValuesFile_MissingFileIsFine(t *testing.T) { + if err := ValidateUserValuesFile(filepath.Join(t.TempDir(), "nope.yaml")); err != nil { + t.Fatalf("missing file must validate (baseline install), got: %v", err) + } +} + +func TestValidateUserValuesFile_ValidOverridePasses(t *testing.T) { + path := writeValuesFile(t, "repository:\n branch: main\nargocd:\n dex:\n enabled: true\n") + if err := ValidateUserValuesFile(path); err != nil { + t.Fatalf("valid argocd mapping must pass, got: %v", err) + } +} + +func TestValidateUserValuesFile_NoArgoCDKeyPasses(t *testing.T) { + path := writeValuesFile(t, "repository:\n branch: main\n") + if err := ValidateUserValuesFile(path); err != nil { + t.Fatalf("file without argocd key must pass, got: %v", err) + } +} + +// TestValidateUserValuesFile_NonMappingArgoCDFails is the pre-flight case from +// the 0.4.9 verification: `argocd: [x]` (or a scalar, or typo'd indentation) +// must fail before any cluster work, naming both the file and the key. +func TestValidateUserValuesFile_NonMappingArgoCDFails(t *testing.T) { + for name, content := range map[string]string{ + "list": "argocd:\n - dex\n", + "scalar": "argocd: yes\n", + } { + t.Run(name, func(t *testing.T) { + path := writeValuesFile(t, content) + err := ValidateUserValuesFile(path) + if err == nil { + t.Fatal("non-mapping argocd must fail validation") + } + if !strings.Contains(err.Error(), path) || !strings.Contains(err.Error(), `"argocd"`) { + t.Errorf("error must name the file and the key, got: %v", err) + } + }) + } +} + +func TestValidateUserValuesFile_BrokenYAMLFails(t *testing.T) { + path := writeValuesFile(t, "argocd:\n\tdex: {enabled\n") + err := ValidateUserValuesFile(path) + if err == nil { + t.Fatal("unparseable YAML must fail validation, not silently pass") + } + if !strings.Contains(err.Error(), "not valid YAML") { + t.Errorf("error must say the YAML is invalid, got: %v", err) + } +} diff --git a/internal/chart/providers/argocd/wait.go b/internal/chart/providers/argocd/wait.go index a2669461..7d38c786 100644 --- a/internal/chart/providers/argocd/wait.go +++ b/internal/chart/providers/argocd/wait.go @@ -184,6 +184,11 @@ func (m *Manager) WaitForApplications(ctx context.Context, config config.ChartIn stragglerSyncTriggered := false stallHintShown := false + // Deterministic manifest-error tracking (see fatalmanifest.go): a legacy + // ref whose chart path does not exist at the pinned revision fails fast + // instead of riding the full timeout. + fatalManifest := newFatalManifestTracker() + // Repo-server issue tracking for recovery logic repoServerRecoveryAttempts := 0 maxRepoServerRecoveryAttempts := 3 // Increased from 2 for CI resilience @@ -366,15 +371,35 @@ func (m *Manager) WaitForApplications(ctx context.Context, config config.ChartIn lastNotReadyApps, lastReadyCount, lastTotalApps = notReadyApps, currentlyReady, totalApps lastNotReadyNames = assess.notReadyNames + // Fail fast on deterministic manifest errors (see fatalmanifest.go): + // once an app has shown the same "content missing at this revision" + // ComparisonError past the persistence thresholds, no amount of + // waiting or repo-server recovery changes the outcome. Checked before + // stall/recovery handling so neither wastes effort on a lost cause. + // One timestamp for all observe calls so recorded "since" values and + // staleness checks use the same tick. + now := time.Now() + if fatal := fatalManifest.observe(apps, now); len(fatal) > 0 { + spinnerMutex.Lock() + if !spinnerStopped && spinner != nil { + spinner.Fail("Applications cannot render manifests from the deployed revision") + spinnerStopped = true + } + spinnerMutex.Unlock() + + requestedRef := "" + if config.AppOfApps != nil { + requestedRef = config.AppOfApps.GitHubBranch + } + return fatalManifestError(requestedRef, fatal) + } + // Stall handling (finding N3, per-application): an app that has sat // OutOfSync-but-Healthy, bit-for-bit identical, for stallAfter will not // move on its own (autoSync off). Judged per-app so a noisy neighbour // flapping status can't keep resetting the clock on a stuck app (V5). // On the upgrade path, sync exactly those stragglers once; otherwise // print an actionable hint once instead of burning the full timeout. - // One timestamp for both calls so an app's recorded "since" and the - // staleness check use the same tick. - now := time.Now() stall.observe(apps, now) if stragglers := stall.stalledStragglers(apps, now); len(stragglers) > 0 { // The two branches are chosen by MODE, not by whether the sync has diff --git a/internal/chart/providers/helm/manager.go b/internal/chart/providers/helm/manager.go index f5d82c5d..e3df8298 100644 --- a/internal/chart/providers/helm/manager.go +++ b/internal/chart/providers/helm/manager.go @@ -254,7 +254,11 @@ func (h *HelmManager) installArgoCDHelm(ctx context.Context, cfg config.ChartIns // chart and carries the registry password). Overrides are announced because a // bad one can break the install. values := argocd.GetArgoCDValues() - if uv, path := userValues(cfg); uv != nil { + uv, path, err := userValues(cfg) + if err != nil { + return nil, err + } + if uv != nil { merged, overridden, err := argocd.MergedArgoCDValues(uv) if err != nil { return nil, fmt.Errorf("merging ArgoCD overrides from %s: %w", path, err) @@ -277,10 +281,11 @@ func (h *HelmManager) installArgoCDHelm(ctx context.Context, cfg config.ChartIns // userValues reads and parses the user's openframe-helm-values.yaml, returning // the parsed map and the path it read. It resolves the path from the config -// (explicit --values wins) or the default cwd location, and returns (nil, path) -// on a missing or unparseable file — a missing user file is normal, not an -// error, so the caller simply falls back to the baseline. -func userValues(cfg config.ChartInstallConfig) (map[string]interface{}, string) { +// (explicit --values wins) or the default cwd location. A MISSING file is +// normal (the caller falls back to the baseline), but a file that exists and +// cannot be read or parsed is an error: swallowing it silently dropped the +// user's intended `argocd:` override — the same silent-failure class as V3. +func userValues(cfg config.ChartInstallConfig) (map[string]interface{}, string, error) { path := "" if cfg.AppOfApps != nil { path = cfg.AppOfApps.ValuesFile @@ -290,13 +295,16 @@ func userValues(cfg config.ChartInstallConfig) (map[string]interface{}, string) } data, err := os.ReadFile(path) // #nosec G304 -- values path resolved from config/CLI, read as the invoking user if err != nil { - return nil, path + if os.IsNotExist(err) { + return nil, path, nil + } + return nil, path, fmt.Errorf("reading values file %s: %w", path, err) } var m map[string]interface{} if err := yaml.Unmarshal(data, &m); err != nil { - return nil, path + return nil, path, fmt.Errorf("values file %s is not valid YAML: %w", path, err) } - return m, path + return m, path, nil } // InstallArgoCDWithProgress installs ArgoCD using Helm with progress indicators @@ -442,8 +450,13 @@ func (h *HelmManager) InstallArgoCDWithProgress(ctx context.Context, config conf spinner.Stop() } - // Show diagnostic information about ArgoCD pods - h.showArgoCDDiagnostics(ctx, config.ClusterName) + // Show diagnostic information about ArgoCD pods — but only when helm + // actually ran (result != nil). A nil result means the values merge or + // parse failed before any helm call, and a pod dump for a pure + // configuration error only buries the real message. + if result != nil { + h.showArgoCDDiagnostics(ctx, config.ClusterName) + } // Include stdout and stderr output for better debugging // On Windows/WSL, stderr is redirected to stdout via 2>&1, so check both. diff --git a/internal/chart/providers/helm/uservalues_test.go b/internal/chart/providers/helm/uservalues_test.go new file mode 100644 index 00000000..70ad80d3 --- /dev/null +++ b/internal/chart/providers/helm/uservalues_test.go @@ -0,0 +1,85 @@ +package helm + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "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/shared/executor" +) + +func installConfigWithValuesFile(path string) config.ChartInstallConfig { + return config.ChartInstallConfig{ + ClusterName: "test", + AppOfApps: &models.AppOfAppsConfig{ValuesFile: path}, + } +} + +// TestInstallArgoCDHelm_BrokenValuesFileFails locks the honesty fix: an +// existing-but-unparseable values file must fail the install instead of +// silently dropping the user's `argocd:` override and deploying the baseline. +func TestInstallArgoCDHelm_BrokenValuesFileFails(t *testing.T) { + path := filepath.Join(t.TempDir(), "openframe-helm-values.yaml") + if err := os.WriteFile(path, []byte("argocd:\n\tbad: {yaml\n"), 0o600); err != nil { + t.Fatal(err) + } + + mock := executor.NewMockCommandExecutor() + m, _ := NewHelmManager(mock, nil, false) + + result, err := m.installArgoCDHelm(context.Background(), installConfigWithValuesFile(path)) + if err == nil { + t.Fatal("broken values file must fail the install") + } + if result != nil { + t.Error("helm must not run when the values file is unparseable (nil result gates the diagnostics dump)") + } + if !strings.Contains(err.Error(), path) { + t.Errorf("error must name the values file, got: %v", err) + } + for _, c := range mock.Commands() { + if c.Name == "helm" && len(c.Args) > 0 && c.Args[0] == "upgrade" { + t.Fatal("helm upgrade must not be attempted with a broken values file") + } + } +} + +// TestInstallArgoCDHelm_NonMappingArgoCDFails: the in-flow defense-in-depth +// check behind the pre-flight — a non-mapping `argocd:` fails before helm runs. +func TestInstallArgoCDHelm_NonMappingArgoCDFails(t *testing.T) { + path := filepath.Join(t.TempDir(), "openframe-helm-values.yaml") + if err := os.WriteFile(path, []byte("argocd: [dex]\n"), 0o600); err != nil { + t.Fatal(err) + } + + mock := executor.NewMockCommandExecutor() + m, _ := NewHelmManager(mock, nil, false) + + result, err := m.installArgoCDHelm(context.Background(), installConfigWithValuesFile(path)) + if err == nil { + t.Fatal("non-mapping argocd override must fail the install") + } + if result != nil { + t.Error("helm must not run on a merge error") + } +} + +// TestInstallArgoCDHelm_MissingValuesFileUsesBaseline: a missing user file +// stays normal — the install proceeds with the embedded baseline. +func TestInstallArgoCDHelm_MissingValuesFileUsesBaseline(t *testing.T) { + mock := executor.NewMockCommandExecutor() + m, _ := NewHelmManager(mock, nil, false) + + cfg := installConfigWithValuesFile(filepath.Join(t.TempDir(), "absent.yaml")) + if _, err := m.installArgoCDHelm(context.Background(), cfg); err != nil { + t.Fatalf("missing values file must not fail the install, got: %v", err) + } + up := findHelmUpgrade(t, mock.Commands()) + if len(up.Stdin) == 0 { + t.Fatal("baseline values must still be piped to helm") + } +} diff --git a/internal/chart/services/chart_service.go b/internal/chart/services/chart_service.go index 96be87c2..09ead1e0 100644 --- a/internal/chart/services/chart_service.go +++ b/internal/chart/services/chart_service.go @@ -9,6 +9,7 @@ import ( "time" "github.com/flamingo-stack/openframe-cli/internal/chart/prerequisites" + "github.com/flamingo-stack/openframe-cli/internal/chart/providers/argocd" "github.com/flamingo-stack/openframe-cli/internal/chart/providers/git" "github.com/flamingo-stack/openframe-cli/internal/chart/providers/helm" chartUI "github.com/flamingo-stack/openframe-cli/internal/chart/ui" @@ -206,6 +207,18 @@ func (w *InstallationWorkflow) ExecuteWithContext(parentCtx context.Context, req } } + // Step 1.5: Pre-flight the values that will feed the ArgoCD install. The + // values are fully parsed by now, so a malformed `argocd:` override fails + // here — before cluster selection and any cluster work — instead of + // surfacing mid-install behind a misleading ArgoCD pod-diagnostics dump + // (0.4.9 verification observation). The install-time check remains as + // defense in depth. + if path := chartConfig.TempHelmValuesPath; path != "" { + if err := argocd.ValidateUserValuesFile(path); err != nil { + return fmt.Errorf("helm values pre-flight failed: %w", err) + } + } + // Step 2: Resolve the install target. An explicit rest.Config from the // command layer (--context, or the interactive kube-context selector) IS // the target — running k3d cluster selection on top of it demanded a diff --git a/internal/chart/services/preflight.go b/internal/chart/services/preflight.go new file mode 100644 index 00000000..07016bd7 --- /dev/null +++ b/internal/chart/services/preflight.go @@ -0,0 +1,22 @@ +package services + +import ( + "fmt" + + "github.com/flamingo-stack/openframe-cli/internal/chart/providers/argocd" + "github.com/flamingo-stack/openframe-cli/internal/chart/utils/config" +) + +// ValidateHelmValuesFile pre-flights the default openframe-helm-values.yaml in +// the current directory. Bootstrap runs it BEFORE cluster creation: without it +// a malformed `argocd:` override costs a full k3d cluster create (minutes) +// before the chart install rejects the same file (0.4.9 verification +// observation). The install workflow re-validates the temp values file it +// actually feeds to helm; this is the earliest, cheapest gate. +func ValidateHelmValuesFile() error { + path := config.NewPathResolver().GetHelmValuesFile() + if err := argocd.ValidateUserValuesFile(path); err != nil { + return fmt.Errorf("helm values pre-flight failed: %w", err) + } + return nil +} diff --git a/internal/shared/executor/executor.go b/internal/shared/executor/executor.go index 31b5c449..5dc79a02 100644 --- a/internal/shared/executor/executor.go +++ b/internal/shared/executor/executor.go @@ -35,8 +35,10 @@ type WSLError struct { func (e *WSLError) Error() string { msg := fmt.Sprintf("WSL error during %s (exit code: %d)", e.Operation, e.ExitCode) - if e.Stderr != "" { - msg += fmt.Sprintf(": %s", e.Stderr) + // Same log-noise strip as CommandError: k3d/helm run via WSL on Windows, + // so this error carries the same logrus progress wall on failure. + if detail := errorDetail(e.Stderr); detail != "" { + msg += fmt.Sprintf(": %s", detail) } if e.Suggestion != "" { msg += fmt.Sprintf("\nSuggestion: %s", e.Suggestion) @@ -66,10 +68,11 @@ const maxStderrInError = 2000 func (e *CommandError) Error() string { msg := fmt.Sprintf("command failed: %s (exit code: %d)", e.Command, e.ExitCode) - if reason := strings.TrimSpace(e.Stderr); reason != "" { - if len(reason) > maxStderrInError { - reason = "..." + reason[len(reason)-maxStderrInError:] - } + // Informational logrus records (k3d's INFO[0000] progress wall) are + // stripped and the detail is bounded to maxStderrInError (both inside + // errorDetail) so the ERRO/FATA line that explains the failure stays + // visible; the full text remains in the Stderr field. + if reason := errorDetail(e.Stderr); reason != "" { return msg + ": " + reason } // No stderr (e.g. the child only wrote to stdout): fall back to the exec diff --git a/internal/shared/executor/lognoise.go b/internal/shared/executor/lognoise.go new file mode 100644 index 00000000..dca77217 --- /dev/null +++ b/internal/shared/executor/lognoise.go @@ -0,0 +1,60 @@ +package executor + +import ( + "regexp" + "strings" +) + +// Subprocess log-noise filtering (0.4.9 verification observation): k3d logs +// every step to stderr in logrus format, so a failed `k3d cluster create` +// embedded a wall of `INFO[0000] ...` progress lines into the user-facing +// error, drowning the one ERRO/FATA line that explains the failure. The lines +// print "outside the CLI's own formatting" because they arrive inside error +// text — no k3d output is ever streamed live (the executor captures both +// stdout and stderr). +// +// Only informational records are dropped; warnings, errors, fatals, and any +// non-logrus line pass through untouched. Helm and docker do not use logrus, +// so their stderr is unaffected. + +var ( + // Colored/TTY-style logrus records: `INFO[0007] Using config file...`. + logrusInfoLine = regexp.MustCompile(`^\s*(?:INFO|DEBU|TRAC)\[[^\]]*\]`) + // Plain logfmt records: `time="..." level=info msg="..."`. + logfmtInfoLine = regexp.MustCompile(`^\s*time="[^"]*" level=(?:info|debug|trace) `) +) + +// stripLogNoise removes informational logrus/logfmt records from subprocess +// output, preserving everything else (including logrus WARN/ERRO/FATA lines). +func stripLogNoise(s string) string { + if s == "" { + return s + } + lines := strings.Split(s, "\n") + kept := lines[:0] + for _, line := range lines { + if logrusInfoLine.MatchString(line) || logfmtInfoLine.MatchString(line) { + continue + } + kept = append(kept, line) + } + return strings.Join(kept, "\n") +} + +// errorDetail prepares captured stderr for embedding into an error message: +// log noise is stripped first; when NOTHING survives (the child only logged +// informational lines before dying) the raw text is kept — losing the only +// available detail would be worse than the noise. The result is bounded to +// maxStderrInError (keeping the tail, where the failure reason lands) so a +// chatty child cannot overwhelm the error; the full text stays available on +// the error's Stderr field. +func errorDetail(stderr string) string { + detail := strings.TrimSpace(stripLogNoise(stderr)) + if detail == "" { + detail = strings.TrimSpace(stderr) + } + if len(detail) > maxStderrInError { + detail = "..." + detail[len(detail)-maxStderrInError:] + } + return detail +} diff --git a/internal/shared/executor/lognoise_test.go b/internal/shared/executor/lognoise_test.go new file mode 100644 index 00000000..f34f821f --- /dev/null +++ b/internal/shared/executor/lognoise_test.go @@ -0,0 +1,125 @@ +package executor + +import ( + "errors" + "strings" + "testing" +) + +// k3dFailureStderr is the real-world shape of a failed `k3d cluster create`: +// a wall of INFO progress records with the actual reason in FATA at the end. +const k3dFailureStderr = `INFO[0000] Using config file /tmp/k3d-config.yaml +INFO[0000] Prep: Network +INFO[0001] Created network 'k3d-openframe-dev' +INFO[0002] Created image volume k3d-openframe-dev-images +WARN[0003] No node filter specified +INFO[0004] Starting new tools node... +ERRO[0060] Failed Cluster Start: Failed to start server k3d-openframe-dev-server-0 +FATA[0060] Cluster creation FAILED, all changes have been rolled back!` + +func TestStripLogNoise_DropsInfoKeepsWarningsAndErrors(t *testing.T) { + got := stripLogNoise(k3dFailureStderr) + + if strings.Contains(got, "INFO[") { + t.Errorf("INFO records must be stripped, got:\n%s", got) + } + for _, want := range []string{"WARN[0003]", "ERRO[0060]", "FATA[0060]"} { + if !strings.Contains(got, want) { + t.Errorf("%s must survive the strip, got:\n%s", want, got) + } + } +} + +func TestStripLogNoise_DropsLogfmtInfo(t *testing.T) { + in := `time="2026-07-14T10:00:00Z" level=info msg="Prep: Network" +time="2026-07-14T10:00:01Z" level=error msg="port already allocated"` + got := stripLogNoise(in) + + if strings.Contains(got, "level=info") { + t.Errorf("logfmt info records must be stripped, got:\n%s", got) + } + if !strings.Contains(got, "port already allocated") { + t.Errorf("logfmt error record must survive, got:\n%s", got) + } +} + +func TestStripLogNoise_PlainOutputUntouched(t *testing.T) { + in := "Error: INSTALLATION FAILED: context deadline exceeded\nsee logs for details" + if got := stripLogNoise(in); got != in { + t.Errorf("non-logrus output must pass through untouched, got:\n%s", got) + } +} + +// TestErrorDetail_AllNoiseFallsBackToRaw: when the child only logged INFO +// before dying, the noise is the only detail there is — keep it. +func TestErrorDetail_AllNoiseFallsBackToRaw(t *testing.T) { + in := "INFO[0000] Using config file\nINFO[0001] Prep: Network" + if got := errorDetail(in); got != strings.TrimSpace(in) { + t.Errorf("all-noise stderr must fall back to raw text, got:\n%s", got) + } +} + +// TestCommandError_FiltersK3dProgressWall locks the user-visible behavior: the +// error a failed k3d command produces names the failure, not 8 progress lines. +func TestCommandError_FiltersK3dProgressWall(t *testing.T) { + err := &CommandError{ + Command: "k3d cluster create --config /tmp/k3d-config.yaml", + ExitCode: 1, + Stderr: k3dFailureStderr, + cause: errors.New("exit status 1"), + } + msg := err.Error() + + if strings.Contains(msg, "INFO[") { + t.Errorf("INFO records must not reach the error message, got:\n%s", msg) + } + if !strings.Contains(msg, "FATA[0060] Cluster creation FAILED") { + t.Errorf("the FATA line must reach the error message, got:\n%s", msg) + } + // The full unfiltered text stays available on the field for verbose paths. + if !strings.Contains(err.Stderr, "INFO[0000]") { + t.Error("Stderr field must keep the full unfiltered output") + } +} + +// TestErrorDetail_TruncatesToBound: both CommandError and WSLError embed +// stderr via errorDetail, so the maxStderrInError bound must live there — +// WSLError previously had no truncation at all. +func TestErrorDetail_TruncatesToBound(t *testing.T) { + huge := strings.Repeat("x", 3*maxStderrInError) + " the actual reason" + got := errorDetail(huge) + + if len(got) > maxStderrInError+3 { // "..." prefix + t.Fatalf("detail not bounded: %d chars", len(got)) + } + if !strings.HasPrefix(got, "...") || !strings.HasSuffix(got, "the actual reason") { + t.Errorf("truncation must keep the tail with an ellipsis prefix, got: %.40s...%.40s", got, got[len(got)-40:]) + } +} + +func TestWSLError_TruncatesHugeStderr(t *testing.T) { + err := &WSLError{ + Operation: "executing k3d", + ExitCode: 1, + Stderr: strings.Repeat("y", 3*maxStderrInError), + } + if len(err.Error()) > maxStderrInError+200 { // message prefix + ellipsis + t.Fatalf("WSLError message not bounded: %d chars", len(err.Error())) + } +} + +func TestWSLError_FiltersLogNoise(t *testing.T) { + err := &WSLError{ + Operation: "executing k3d", + ExitCode: 1, + Stderr: k3dFailureStderr, + } + msg := err.Error() + + if strings.Contains(msg, "INFO[") { + t.Errorf("INFO records must not reach the WSL error message, got:\n%s", msg) + } + if !strings.Contains(msg, "FATA[0060]") { + t.Errorf("the FATA line must reach the WSL error message, got:\n%s", msg) + } +}