Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions internal/bootstrap/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
7 changes: 6 additions & 1 deletion internal/chart/providers/argocd/assess.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,19 @@ 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 {
unknown = append(unknown, app)
}

hasRepoErr := false
if app.Condition != "" {
if app.Condition != "" && !isDeterministicManifestError(app.Condition) {
for _, p := range repoServerErrorPatterns {
if strings.Contains(app.Condition, p) {
hasRepoErr = true
Expand Down
151 changes: 151 additions & 0 deletions internal/chart/providers/argocd/fatalmanifest.go
Original file line number Diff line number Diff line change
@@ -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())
}
168 changes: 168 additions & 0 deletions internal/chart/providers/argocd/fatalmanifest_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
26 changes: 26 additions & 0 deletions internal/chart/providers/argocd/values.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package argocd
import (
_ "embed"
"fmt"
"os"
"sort"

"sigs.k8s.io/yaml"
Expand Down Expand Up @@ -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{}) {
Expand Down
Loading