diff --git a/.github/release.yml b/.github/release.yml new file mode 100644 index 00000000..6131b608 --- /dev/null +++ b/.github/release.yml @@ -0,0 +1,34 @@ +# GitHub-native release notes configuration. +# +# GoReleaser's changelog.use is "github-native" (.goreleaser.yml), which +# delegates note generation to GitHub's "Generate release notes" API โ€” and that +# API reads THIS file, not GoReleaser's groups/filters (which it ignores). +# +# Notes are built per merged PR and categorized by the PR's LABELS, which is +# robust to this repo's squash-merge + free-form PR titles (commit-title regexes +# would miss most of them). A PR lands in the FIRST category whose label it +# carries; anything unlabeled falls through to "Other Changes". +changelog: + exclude: + labels: + - automated # e.g. "๐Ÿ“š Automated Documentation Update" PRs + - duplicate + - invalid + - wontfix + - question + authors: + - dependabot[bot] + - github-actions[bot] + categories: + - title: ๐Ÿš€ Features + labels: + - enhancement + - title: ๐Ÿ› Bug Fixes + labels: + - bug + - title: ๐Ÿ“š Documentation + labels: + - documentation + - title: Other Changes + labels: + - "*" diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 01dca0b4..392be3e6 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -178,12 +178,7 @@ jobs: echo "===================================================================" "$OF_BIN" --version "$OF_BIN" --help - "$OF_BIN" cluster --help - "$OF_BIN" app --help - "$OF_BIN" app install --help - "$OF_BIN" app upgrade --help - "$OF_BIN" bootstrap --help - "$OF_BIN" update --help + # Per-command --help is exercised exhaustively in the next step. echo "--- completion: every supported shell generates cleanly" # Shell-completion generation is pure (no cluster/network) โ€” smoke every shell. for sh in bash zsh fish powershell; do "$OF_BIN" completion "$sh" >/dev/null; done @@ -198,6 +193,36 @@ jobs: # back" notice (offline, no download), not error out or hang. "$OF_BIN" update rollback + # Exhaustive --help over the WHOLE command tree, on the real (built/staged) + # binary. Complements the hermetic cmd/help_matrix_test.go by covering the + # actual binary (ldflags, embeds, and on Windows the WSL forward). Discovery + # is dynamic โ€” a newly added command is covered with no hand-maintained list. + # Pure and non-interactive (no cluster, no network), so it runs on every OS. + - name: 'CLI: every command --help (exhaustive)' + shell: bash + run: | + echo "===================================================================" + echo "=== TEST: --help on EVERY command (real binary, non-interactive)" + echo "===================================================================" + fail() { echo "::error::$1"; exit 1; } + walk_help() { + local path="$1" + # $path is a space-separated command path ("cluster create") that MUST + # word-split into separate argv entries โ€” intentional, so quiet SC2086. + # shellcheck disable=SC2086 + "$OF_BIN" $path --help >/tmp/h.out 2>&1 || { cat /tmp/h.out; fail "'openframe $path --help' exited non-zero"; } + grep -q '^Usage:' /tmp/h.out || { cat /tmp/h.out; fail "'openframe $path --help' printed no Usage section"; } + echo " OK: openframe $path --help" + # Leaf commands list no subcommands, so grep matching nothing is fine. + local subs + subs=$(sed -n '/Available Commands:/,/^$/p' /tmp/h.out | grep -E '^ [a-z]' | awk '{print $1}' || true) + for s in $subs; do + [ "$s" = "help" ] && continue + walk_help "${path:+$path }$s" + done + } + walk_help "" + # Self-update against the REAL latest release: the only test that # exercises the live trust chain end to end (GitHub release lookup โ†’ # cosign signature of checksums.txt against the pinned identity โ†’ @@ -502,6 +527,22 @@ jobs: echo "--- force-sync" "$OF_BIN" app upgrade --context "$OF_CONTEXT" --sync --verbose + # cluster cleanup is a first-class non-interactive command with logic no + # other step exercises against a real cluster: kube-context-pinned Helm + # uninstalls, ArgoCD finalizer stripping, and node image pruning via crictl + # (k3d nodes run containerd, not docker). Runs on the live, populated cluster + # so all phases have something to do; --force skips the confirmation prompt. + # It leaves the cluster empty; the teardown then deletes it. + - name: 'Cluster: cleanup (releases + crictl image prune)' + if: matrix.os != 'darwin' + shell: bash + timeout-minutes: 10 + run: | + echo "===================================================================" + echo "=== TEST: cluster cleanup --force (non-interactive)" + echo "===================================================================" + "$OF_BIN" cluster cleanup "$OF_CLUSTER" --force + # --- Teardown (runs even if a step above failed) ----------------------- - name: 'Teardown: uninstall & delete cluster' if: always() && matrix.os != 'darwin' diff --git a/.goreleaser.yml b/.goreleaser.yml index a0517fc8..558f20b4 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -80,28 +80,13 @@ signs: - '--yes' changelog: + # github-native delegates note generation to GitHub's "Generate release notes" + # API, which categorizes by PR LABEL via .github/release.yml โ€” the right fit + # for this repo's squash-merge + free-form PR titles. Under github-native + # GoReleaser IGNORES its own `groups`/`filters`, so they are intentionally + # absent here; edit .github/release.yml to change categories or exclusions. use: github-native sort: asc - filters: - exclude: - - '^docs:' - - '^test:' - - '^chore:' - - '^ci:' - - '^build:' - - '^style:' - - '^refactor:' - - Merge pull request - - Merge branch - groups: - - title: Features - regexp: "^.*feat[(\\w)]*:+.*$" - order: 0 - - title: Bug fixes - regexp: "^.*fix[(\\w)]*:+.*$" - order: 1 - - title: Others - order: 999 release: make_latest: true diff --git a/Makefile b/Makefile index 0877d02f..d6e42b61 100644 --- a/Makefile +++ b/Makefile @@ -1,73 +1,80 @@ # OpenFrame CLI Makefile -.PHONY: build build-all clean test test-unit test-race test-integration lint help +.PHONY: all build build-all clean test test-unit test-race test-integration lint fmt vet tidy help # Variables BINARY_NAME := openframe -GO_BUILD := CGO_ENABLED=0 go build +# -trimpath drops absolute build paths from the binary (reproducibility), matching +# the release build in .goreleaser.yml. +GO_BUILD := CGO_ENABLED=0 go build -trimpath # Detect current OS and architecture GOOS := $(shell go env GOOS) GOARCH := $(shell go env GOARCH) BINARY_SUFFIX := $(if $(filter windows,$(GOOS)),.exe,) +# Unit-test package set. Includes the root package (main_test.go โ€” the only +# exit-code fidelity tests) and tests/testutil, which `./cmd/... ./internal/...` +# silently skipped. Deliberately excludes ./tests/integration/... (real clusters). +UNIT_PKGS := . ./cmd/... ./internal/... ./tests/testutil/... + # Default target all: build -## Build binary for current platform -build: +build: ## Build binary for the current platform @echo "Building $(BINARY_NAME) for $(GOOS)/$(GOARCH)..." @$(GO_BUILD) -o $(BINARY_NAME)-$(GOOS)-$(GOARCH)$(BINARY_SUFFIX) . -## Build binaries for all platforms -build-all: - @echo "Building $(BINARY_NAME) for all platforms..." - @GOOS=linux GOARCH=amd64 $(GO_BUILD) -o $(BINARY_NAME)-linux-amd64 . - @GOOS=darwin GOARCH=arm64 $(GO_BUILD) -o $(BINARY_NAME)-darwin-arm64 . +build-all: ## Cross-compile every release platform (matches .goreleaser.yml) + @echo "Building $(BINARY_NAME) for all release platforms..." + @GOOS=linux GOARCH=amd64 $(GO_BUILD) -o $(BINARY_NAME)-linux-amd64 . + @GOOS=linux GOARCH=arm64 $(GO_BUILD) -o $(BINARY_NAME)-linux-arm64 . + @GOOS=darwin GOARCH=amd64 $(GO_BUILD) -o $(BINARY_NAME)-darwin-amd64 . + @GOOS=darwin GOARCH=arm64 $(GO_BUILD) -o $(BINARY_NAME)-darwin-arm64 . @GOOS=windows GOARCH=amd64 $(GO_BUILD) -o $(BINARY_NAME)-windows-amd64.exe . + @GOOS=windows GOARCH=arm64 $(GO_BUILD) -o $(BINARY_NAME)-windows-arm64.exe . -## Run unit tests (vet enabled; -vet=off removed per audit remediation ยง0). -## Includes the root package (main_test.go โ€” the only exit-code fidelity tests) -## and tests/testutil, which `./cmd/... ./internal/...` silently skipped. -## Deliberately NOT ./tests/integration/... โ€” those create real k3d clusters. -test-unit: +test-unit: ## Run unit tests (vet on; incl. root main_test.go + tests/testutil) @echo "Running unit tests..." - @go test -count=1 . ./cmd/... ./internal/... ./tests/testutil/... + @go test -count=1 $(UNIT_PKGS) -## Run unit tests with the race detector (CGO required) -test-race: +test-race: ## Run unit tests with the race detector (requires CGO) @echo "Running unit tests with -race..." - @CGO_ENABLED=1 go test -race -count=1 . ./cmd/... ./internal/... ./tests/testutil/... + @CGO_ENABLED=1 go test -race -count=1 $(UNIT_PKGS) + +# Integration tests are opt-in via a build tag: they create REAL k3d clusters and +# run a full bootstrap, so `go test ./...` must never trigger them by accident. +# The harness builds its own CLI binary (see tests/integration/common/cli_runner.go), +# so `make build` is NOT required โ€” but docker and k3d must be installed. +test-integration: ## Run integration tests (real k3d clusters; needs docker + k3d) + @echo "Running integration tests (real clusters!)..." + @go test -tags integration -count=1 ./tests/integration/... -## Run golangci-lint (static-analysis gate: govet, staticcheck, errcheck, gosec, ineffassign) -lint: +test: test-unit test-integration ## Run unit + integration tests + +lint: ## Run golangci-lint (govet, staticcheck, errcheck, gosec, ineffassign) @echo "Running golangci-lint..." @command -v golangci-lint >/dev/null 2>&1 || { echo "golangci-lint not installed: https://golangci-lint.run/usage/install/"; exit 1; } @golangci-lint run ./... -## Run integration tests (opt-in via build tag: they create REAL k3d clusters -## and run a full bootstrap โ€” `go test ./...` must never trigger that by -## accident; requires the CLI binary from `make build`) -test-integration: - @echo "Running integration tests (real clusters!)..." - @go test -tags integration -count=1 ./tests/integration/... +fmt: ## Format Go source in place (gofmt -w) + @gofmt -l -w . + +vet: ## Run go vet on all packages + @go vet ./... -## Run all tests -test: test-unit test-integration +tidy: ## Check go.mod/go.sum are tidy (fails if `go mod tidy` would change them) + @go mod tidy -diff -## Clean build artifacts -clean: - @rm -f $(BINARY_NAME) $(BINARY_NAME)-* +# clean matches only the platform-suffixed binaries, NOT a broad `openframe-*` +# glob โ€” that also matched tracked files like openframe-helm-values.example.yaml +# and deleted them. +clean: ## Remove build artifacts + @rm -f $(BINARY_NAME) \ + $(BINARY_NAME)-linux-* $(BINARY_NAME)-darwin-* $(BINARY_NAME)-windows-* @echo "Cleaned build artifacts" -## Show help -help: +help: ## Show this help @echo "Available targets:" - @echo " build - Build binary for current platform (default)" - @echo " build-all - Build binaries for all platforms" - @echo " test - Run all tests" - @echo " test-unit - Run unit tests (vet enabled)" - @echo " test-race - Run unit tests with the race detector" - @echo " lint - Run golangci-lint static-analysis gate" - @echo " test-integration - Run integration tests" - @echo " clean - Clean build artifacts" + @grep -hE '^[a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | sort | \ + awk 'BEGIN {FS = ":.*?## "}; {printf " \033[36m%-16s\033[0m %s\n", $$1, $$2}' diff --git a/cmd/prerequisites/prerequisites.go b/cmd/prerequisites/prerequisites.go index b8e3e6cd..61600ee3 100644 --- a/cmd/prerequisites/prerequisites.go +++ b/cmd/prerequisites/prerequisites.go @@ -84,9 +84,15 @@ func printResult(res fw.Result) { pterm.Success.Printf("โœ“ %s (installed)\n", name) } for _, m := range res.Missing { - pterm.Error.Printf("โœ— %s is not installed\n", m.Name) - if m.DocsURL != "" { - pterm.Info.Printf(" How to install: %s\n", m.DocsURL) + if m.Reason != "" { + // A specific reason (e.g. "installed but not running") โ€” the tool IS + // present, so don't claim it isn't and don't show install docs. + pterm.Error.Printf("โœ— %s: %s\n", m.Name, m.Reason) + } else { + pterm.Error.Printf("โœ— %s is not installed\n", m.Name) + if m.DocsURL != "" { + pterm.Info.Printf(" How to install: %s\n", m.DocsURL) + } } if m.Err != nil { pterm.Debug.Printf(" (%v)\n", m.Err) diff --git a/docs/development/setup/local-development.md b/docs/development/setup/local-development.md index a32bcd88..c19d97e6 100644 --- a/docs/development/setup/local-development.md +++ b/docs/development/setup/local-development.md @@ -150,6 +150,32 @@ kubectl get pods --all-namespaces kubectl get applications -n argocd ``` +### Overriding ArgoCD chart values + +The CLI installs ArgoCD from a built-in baseline (embedded +`internal/chart/providers/argocd/argocd-values.yaml`), which is separate from +the app-of-apps values. To change an ArgoCD chart value without rebuilding the +CLI, add a top-level `argocd:` section to `openframe-helm-values.yaml`: + +```yaml +# openframe-helm-values.yaml +repository: + branch: main # (app-of-apps settings, as before) + +argocd: # deep-merged over the built-in ArgoCD baseline + dex: + enabled: true # e.g. re-enable dex (disabled by default) + server: + replicas: 2 +``` + +Only the `argocd:` subtree is applied to the ArgoCD install โ€” the rest of the +file targets the app-of-apps chart, and keeping them separate stops secrets +(e.g. the docker registry password) from leaking into the ArgoCD release. The +merge follows Helm semantics (maps merge, scalars/lists replace), and the CLI +prints a warning listing the keys you overrode, since a bad override can break +the ArgoCD install. Without an `argocd:` section the baseline is used unchanged. + ## Cross-platform Builds ```bash diff --git a/internal/bootstrap/service.go b/internal/bootstrap/service.go index f352fe8e..2fd76e88 100644 --- a/internal/bootstrap/service.go +++ b/internal/bootstrap/service.go @@ -119,4 +119,3 @@ func (s *Service) installChart(ctx context.Context, clusterName string, nonInter ClusterAccess: cluster.NewClusterService(executor.NewRealCommandExecutor(false, verbose)), }) } - diff --git a/internal/chart/providers/argocd/argocd-values.yaml b/internal/chart/providers/argocd/argocd-values.yaml index 3c8153d6..d1703fd6 100644 --- a/internal/chart/providers/argocd/argocd-values.yaml +++ b/internal/chart/providers/argocd/argocd-values.yaml @@ -1,9 +1,19 @@ # Baseline Helm values for the Argo CD chart (targets chart 10.x / app v3.x). # # This file is embedded into the CLI binary via go:embed (see values.go) and is -# the single source of truth for the ArgoCD install the CLI performs BEFORE the -# app-of-apps is cloned from the platform repo. Keep component keys (redis, dex, -# applicationSet, notifications) aligned with the pinned chart version. +# the BASELINE for the ArgoCD install the CLI performs BEFORE the app-of-apps is +# cloned from the platform repo. Keep component keys (redis, dex, applicationSet, +# notifications) aligned with the pinned chart version. +# +# USER OVERRIDES: a user can override any value here without rebuilding the CLI +# by adding a top-level `argocd:` section to openframe-helm-values.yaml; that +# subtree is deep-merged over this baseline (maps merge, scalars/lists replace) +# and the CLI prints a warning naming the overridden keys. Only the `argocd:` +# subtree is used โ€” the rest of that file targets the app-of-apps chart. Example: +# argocd: +# dex: +# enabled: true # re-enable dex (disabled by default below) +# See MergedArgoCDValues in values.go. # # SECURITY โ€” developer account default: # configs.secret.extra.accounts.developer.password below is a bcrypt hash of @@ -112,13 +122,14 @@ redis: dex: - resources: - requests: - cpu: 50m - memory: 64Mi - limits: - cpu: 100m - memory: 128Mi + # Dex (SSO) is disabled: the chart defaults it on (dex.enabled: true), but + # OpenFrame's local/oss-tenant login uses the local `developer` account + # configured above, never dex. Beyond being dead weight on local k3d, the + # dexidp/dex:v2.45.1 arm64 image intermittently SIGSEGVs under emulation on + # Apple Silicon (exit 139) before its first log line -> CrashLoopBackOff -> + # the 7m `helm --wait` never completes -> fresh installs fail. Disabling it + # removes the blocker; re-enable and pin a known-good image if SSO is needed. + enabled: false applicationSet: diff --git a/internal/chart/providers/argocd/assess.go b/internal/chart/providers/argocd/assess.go index c2f6b4a5..a4cdad98 100644 --- a/internal/chart/providers/argocd/assess.go +++ b/internal/chart/providers/argocd/assess.go @@ -13,6 +13,15 @@ type appAssessment struct { notReadyNames []string // bare names of apps not yet ready (for kubectl commands) } +// appNames returns the names of the given applications, preserving order. +func appNames(apps []Application) []string { + names := make([]string, 0, len(apps)) + for _, app := range apps { + names = append(names, app.Name) + } + return names +} + // assessApplications classifies the applications for one polling tick and // marks apps that are currently Healthy+Synced in everReady (the session-wide // set of apps that have ever been ready). diff --git a/internal/chart/providers/argocd/diagnostics.go b/internal/chart/providers/argocd/diagnostics.go index 8e7b5adb..45bd77f2 100644 --- a/internal/chart/providers/argocd/diagnostics.go +++ b/internal/chart/providers/argocd/diagnostics.go @@ -139,9 +139,36 @@ func (m *Manager) checkRepoServerHealth(ctx context.Context, _ bool) *RepoServer return nil } +// hardRefreshApplications annotates each named Application with a HARD refresh +// (argocd.argoproj.io/refresh: hard), forcing the repo-server to re-fetch +// manifests from git and bypass its manifest cache. A "normal" refresh only +// re-compares against that cache โ€” worthless right after a repo-server restart, +// where the cache is exactly what's stale (apps sit in Unknown because manifest +// generation failed). Best-effort: returns the number successfully patched; +// per-app failures are logged at debug and skipped. +func (m *Manager) hardRefreshApplications(ctx context.Context, names []string) int { + if m.dynamicClient == nil { + return 0 + } + var refreshed int + for _, name := range names { + if name == "" { + continue + } + if _, err := m.dynamicClient.Resource(applicationGVR).Namespace(ArgoCDNamespace). + Patch(ctx, name, types.MergePatchType, []byte(refreshHardPatch), metav1.PatchOptions{}); err != nil { + pterm.Debug.Printf("best-effort hard refresh of application %s failed: %v\n", name, err) + continue + } + refreshed++ + } + return refreshed +} + // triggerRepoServerRecovery restarts the repo-server (delete its pods โ†’ the -// controller recreates them) and optionally forces an application refresh. Uses -// the native client for both the delete and the ArgoCD Application patch. +// controller recreates them) and hard-refreshes the triggering application so +// child generation unblocks. Uses the native client for the delete and the +// dynamic client for the Application patch. func (m *Manager) triggerRepoServerRecovery(ctx context.Context, appName string) bool { if m.kubeClient == nil { return false @@ -161,13 +188,13 @@ func (m *Manager) triggerRepoServerRecovery(ctx context.Context, appName string) if m.checkRepoServerHealth(ctx, false) != nil { continue } - // Recovered โ€” force a refresh of the application if specified. - if appName != "" && m.dynamicClient != nil { - patch := []byte(`{"metadata":{"annotations":{"argocd.argoproj.io/refresh":"normal"}}}`) - if _, err := m.dynamicClient.Resource(applicationGVR).Namespace(ArgoCDNamespace). - Patch(ctx, appName, types.MergePatchType, patch, metav1.PatchOptions{}); err != nil { - pterm.Debug.Printf("best-effort refresh of application %s failed: %v\n", appName, err) - } + // Recovered โ€” HARD-refresh the triggering app so the freshly restarted + // repo-server re-fetches its manifests from git. A "normal" refresh + // (what this did before) only re-compares against the manifest cache, + // which is exactly what a just-restarted repo-server has lost โ€” so the + // app stayed stuck in Unknown until the wait timed out. + if appName != "" { + m.hardRefreshApplications(ctx, []string{appName}) } return true } diff --git a/internal/chart/providers/argocd/hardrefresh_test.go b/internal/chart/providers/argocd/hardrefresh_test.go new file mode 100644 index 00000000..ae8f7273 --- /dev/null +++ b/internal/chart/providers/argocd/hardrefresh_test.go @@ -0,0 +1,101 @@ +package argocd + +import ( + "context" + goruntime "runtime" + "strings" + "testing" + + "k8s.io/apimachinery/pkg/runtime" + k8stesting "k8s.io/client-go/testing" +) + +// capturePatches records the body of every Application patch against the fake +// dynamic client, so a test can assert WHAT was patched, not just that a call +// happened. +func capturePatches(m *Manager) *[]string { + var bodies []string + dc := m.dynamicClient.(interface { + PrependReactor(verb, resource string, fn k8stesting.ReactionFunc) + }) + dc.PrependReactor("patch", "applications", func(action k8stesting.Action) (bool, runtime.Object, error) { + bodies = append(bodies, string(action.(k8stesting.PatchAction).GetPatch())) + return false, nil, nil // fall through to the default reactor + }) + return &bodies +} + +// TestHardRefreshApplications_PatchesHardNotNormal is the core of the fix: the +// annotation must be "hard" (re-fetch from git, bypass the manifest cache), not +// "normal" (re-compare against the cache that a just-restarted repo-server has +// lost). A normal refresh left Unknown apps stuck until the wait timed out. +func TestHardRefreshApplications_PatchesHardNotNormal(t *testing.T) { + if goruntime.GOOS == "windows" { + t.Skip("native cluster ops are refused on Windows (must run inside WSL)") + } + m := fakeManager( + appObj("ingress-nginx", ArgoCDStatusUnknown, ArgoCDStatusUnknown), + appObj("openframe-config", ArgoCDStatusUnknown, ArgoCDStatusUnknown), + ) + bodies := capturePatches(m) + + n := m.hardRefreshApplications(context.Background(), []string{"ingress-nginx", "openframe-config"}) + if n != 2 { + t.Fatalf("both apps must be refreshed, got %d", n) + } + if len(*bodies) != 2 { + t.Fatalf("expected 2 patches, got %d", len(*bodies)) + } + for _, b := range *bodies { + if !strings.Contains(b, `"argocd.argoproj.io/refresh":"hard"`) { + t.Errorf("patch must set a HARD refresh, got: %s", b) + } + if strings.Contains(b, `"normal"`) { + t.Errorf("patch must NOT be a normal refresh, got: %s", b) + } + } +} + +// TestHardRefreshApplications_SkipsEmptyNames: empty names are ignored (no +// patch against a nameless resource) and not counted. +func TestHardRefreshApplications_SkipsEmptyNames(t *testing.T) { + if goruntime.GOOS == "windows" { + t.Skip("native cluster ops are refused on Windows (must run inside WSL)") + } + m := fakeManager(appObj("real", ArgoCDStatusUnknown, ArgoCDStatusUnknown)) + bodies := capturePatches(m) + + n := m.hardRefreshApplications(context.Background(), []string{"", "real", ""}) + if n != 1 { + t.Fatalf("only the named app may be refreshed, got %d", n) + } + if len(*bodies) != 1 { + t.Errorf("empty names must not produce patches, got %d", len(*bodies)) + } +} + +// TestHardRefreshApplications_NilDynamicClientIsNoOp: without a dynamic client +// (native init unavailable) the call is a safe no-op rather than a panic. +func TestHardRefreshApplications_NilDynamicClient(t *testing.T) { + m := &Manager{} + if n := m.hardRefreshApplications(context.Background(), []string{"a"}); n != 0 { + t.Errorf("nil dynamic client must refresh nothing, got %d", n) + } +} + +// TestTriggerRepoServerRecovery_HardRefreshesTriggerApp guards that the recovery +// path itself uses a hard refresh (it previously hard-coded "normal"). The pod +// restart needs a live kube client, so this drives hardRefreshApplications โ€” +// the exact helper the recovery now delegates to โ€” and asserts the annotation. +func TestTriggerRepoServerRecovery_UsesHardRefreshHelper(t *testing.T) { + if goruntime.GOOS == "windows" { + t.Skip("native cluster ops are refused on Windows (must run inside WSL)") + } + m := fakeManager(appObj("argocd-apps", ArgoCDStatusUnknown, ArgoCDStatusUnknown)) + bodies := capturePatches(m) + + m.hardRefreshApplications(context.Background(), []string{"argocd-apps"}) + if len(*bodies) != 1 || !strings.Contains((*bodies)[0], `"hard"`) { + t.Errorf("recovery refresh must be hard, got: %v", *bodies) + } +} diff --git a/internal/chart/providers/argocd/refassert.go b/internal/chart/providers/argocd/refassert.go new file mode 100644 index 00000000..67947bac --- /dev/null +++ b/internal/chart/providers/argocd/refassert.go @@ -0,0 +1,105 @@ +package argocd + +import ( + "fmt" + "sort" + "strings" +) + +// refMismatch is one child application that ArgoCD is tracking at a different +// git revision than the ref the user asked the CLI to deploy. +type refMismatch struct { + App string + Want string // requested ref + Got string // the app's actual spec.source.targetRevision +} + +// defaultRefs are the branch names indistinguishable from "no pinning": if the +// user requests one of these and a legacy chart ignores the pin, the children +// land on main anyway โ€” the same place โ€” so there is nothing to warn about. +var defaultRefs = map[string]bool{"": true, "main": true, "master": true, "head": true} + +// verifyRefPinning reports the OSS-repo child applications whose targetRevision +// does not match the requested ref. +// +// It exists for the V3 silent-failure: `app install --ref ` writes +// the flattened repository.branch, but a branch whose chart predates that key +// ignores it โ€” children render from main, the CLI waits, everything goes +// Healthy+Synced, and "17/17 ready โ€ฆ SUCCESS" is printed for a deployment of +// main, not the requested ref. Comparing what ArgoCD actually tracks against +// what was asked turns that into a loud, specific failure. +// +// Only children pointing at repoURL are considered: a child that legitimately +// sources a different repository (third-party) has its own revision and must +// not be flagged. A child with an empty targetRevision is skipped (unknowable), +// as is any request for a default ref (see defaultRefs). +func verifyRefPinning(apps []Application, repoURL, requestedRef string) []refMismatch { + if defaultRefs[strings.ToLower(strings.TrimSpace(requestedRef))] { + return nil + } + want := normalizeRef(requestedRef) + repo := normalizeRepoURL(repoURL) + + var out []refMismatch + for _, app := range apps { + if repo != "" && normalizeRepoURL(app.RepoURL) != repo { + continue // different repository โ€” not ours to judge + } + got := strings.TrimSpace(app.TargetRevision) + if got == "" { + continue // no declared revision โ€” nothing to compare + } + if normalizeRef(got) != want { + out = append(out, refMismatch{App: app.Name, Want: requestedRef, Got: got}) + } + } + sort.Slice(out, func(i, j int) bool { return out[i].App < out[j].App }) + return out +} + +// normalizeRef trims a git ref for comparison. Branch and tag references are +// compared by their short name; a "v"-prefixed tag and its bare form are NOT +// unified (they are distinct git refs). +func normalizeRef(ref string) string { + ref = strings.TrimSpace(ref) + ref = strings.TrimPrefix(ref, "refs/heads/") + ref = strings.TrimPrefix(ref, "refs/tags/") + return ref +} + +// normalizeRepoURL canonicalizes a git remote for comparison: lowercased, no +// scheme, no embedded credentials, no trailing ".git" or slash. Enough to match +// "https://user:tok@github.com/org/repo.git" against "github.com/org/repo". +func normalizeRepoURL(u string) string { + u = strings.TrimSpace(strings.ToLower(u)) + if u == "" { + return "" + } + if i := strings.Index(u, "://"); i >= 0 { + u = u[i+3:] + } + u = strings.TrimPrefix(u, "git@") + if at := strings.LastIndex(u, "@"); at >= 0 { // strip user:token@ + u = u[at+1:] + } + u = strings.ReplaceAll(u, ":", "/") // scp-style host:org/repo + u = strings.TrimSuffix(u, "/") + u = strings.TrimSuffix(u, ".git") + return u +} + +// refMismatchError renders the mismatches into a loud, actionable error. The +// workloads are running โ€” but not from the ref the user asked for, so the +// requested operation did not do what it said. +func refMismatchError(requestedRef string, m []refMismatch) error { + var b strings.Builder + fmt.Fprintf(&b, "requested ref %q was NOT deployed: this branch's chart predates ref pinning, "+ + "so its child applications ignored the pin and ArgoCD is tracking a different revision.\n", + requestedRef) + b.WriteString("The workloads are running, but they do NOT reflect the requested ref:\n") + for _, x := range m { + fmt.Fprintf(&b, " - %s is on %q, not %q\n", x.App, x.Got, x.Want) + } + b.WriteString("Use a branch whose chart reads repository.branch, or pin these applications' targetRevision by hand.") + return fmt.Errorf("%s", b.String()) +} diff --git a/internal/chart/providers/argocd/refassert_test.go b/internal/chart/providers/argocd/refassert_test.go new file mode 100644 index 00000000..af8ccce8 --- /dev/null +++ b/internal/chart/providers/argocd/refassert_test.go @@ -0,0 +1,86 @@ +package argocd + +import ( + "strings" + "testing" +) + +const ossRepo = "https://github.com/flamingo-stack/openframe-oss-tenant" + +// TestVerifyRefPinning_LegacyBranchSilentlyDeploysMain is the V3 case: the user +// asked for a legacy branch, its chart ignored repository.branch, and ArgoCD is +// tracking main. That must be reported โ€” the CLI otherwise prints SUCCESS for a +// deployment of the wrong ref. +func TestVerifyRefPinning_LegacyBranchSilentlyDeploysMain(t *testing.T) { + apps := []Application{ + {Name: "openframe-api", RepoURL: ossRepo, TargetRevision: "main"}, + {Name: "openframe-ui", RepoURL: ossRepo, TargetRevision: "main"}, + } + mm := verifyRefPinning(apps, ossRepo, "feature/configuration-updates") + if len(mm) != 2 { + t.Fatalf("both children on main must be flagged, got %d: %+v", len(mm), mm) + } + err := refMismatchError("feature/configuration-updates", mm) + for _, want := range []string{"predates ref pinning", "openframe-api", `on "main"`, "feature/configuration-updates"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error must contain %q; got:\n%s", want, err.Error()) + } + } +} + +// TestVerifyRefPinning_ModernBranchMatches: a chart that honours the pin sets +// children's targetRevision to the requested ref โ€” no mismatch. +func TestVerifyRefPinning_ModernBranchMatches(t *testing.T) { + apps := []Application{ + {Name: "openframe-api", RepoURL: ossRepo, TargetRevision: "feature/x"}, + {Name: "openframe-ui", RepoURL: ossRepo + ".git", TargetRevision: "feature/x"}, + } + if mm := verifyRefPinning(apps, ossRepo, "feature/x"); len(mm) != 0 { + t.Errorf("matching refs must not be flagged, got %+v", mm) + } +} + +// TestVerifyRefPinning_DefaultRefSkipped: requesting main (or empty) is +// indistinguishable from no pinning; do not flag. +func TestVerifyRefPinning_DefaultRefSkipped(t *testing.T) { + apps := []Application{{Name: "a", RepoURL: ossRepo, TargetRevision: "main"}} + for _, ref := range []string{"", "main", "master", "HEAD"} { + if mm := verifyRefPinning(apps, ossRepo, ref); len(mm) != 0 { + t.Errorf("default ref %q must be skipped, got %+v", ref, mm) + } + } +} + +// TestVerifyRefPinning_ForeignRepoIgnored: a child sourcing a different repo has +// its own revision and must not be judged against the OSS ref. +func TestVerifyRefPinning_ForeignRepoIgnored(t *testing.T) { + apps := []Application{ + {Name: "third-party", RepoURL: "https://charts.example.com/foo", TargetRevision: "1.2.3"}, + {Name: "openframe-api", RepoURL: ossRepo, TargetRevision: "feature/x"}, + } + if mm := verifyRefPinning(apps, ossRepo, "feature/x"); len(mm) != 0 { + t.Errorf("foreign-repo child must be ignored, got %+v", mm) + } +} + +// TestVerifyRefPinning_EmptyRevisionSkipped: a child with no declared revision +// is unknowable, not a mismatch. +func TestVerifyRefPinning_EmptyRevisionSkipped(t *testing.T) { + apps := []Application{{Name: "a", RepoURL: ossRepo, TargetRevision: ""}} + if mm := verifyRefPinning(apps, ossRepo, "feature/x"); len(mm) != 0 { + t.Errorf("empty targetRevision must be skipped, got %+v", mm) + } +} + +// TestVerifyRefPinning_RepoURLNormalization: credentials, scheme, and .git must +// not defeat the repo match (else every child looks "foreign" and nothing is +// checked). +func TestVerifyRefPinning_RepoURLNormalization(t *testing.T) { + apps := []Application{ + {Name: "openframe-api", RepoURL: "https://x-access-token:ghp_secret@github.com/flamingo-stack/openframe-oss-tenant.git", TargetRevision: "main"}, + } + mm := verifyRefPinning(apps, ossRepo, "feature/x") + if len(mm) != 1 { + t.Fatalf("credentialed/.git URL must still match the repo and be flagged, got %+v", mm) + } +} diff --git a/internal/chart/providers/argocd/stall.go b/internal/chart/providers/argocd/stall.go index 955bd178..a0b20e9e 100644 --- a/internal/chart/providers/argocd/stall.go +++ b/internal/chart/providers/argocd/stall.go @@ -1,9 +1,7 @@ package argocd import ( - "fmt" "sort" - "strings" "time" ) @@ -18,29 +16,66 @@ import ( // sync on exactly those stragglers (upgrade path, SyncStragglersOnStall) or // prints an actionable hint once (install path). -// stallAfter is how long the application set may stay bit-for-bit identical -// before the wait considers itself stalled. Long enough that a slow-but-live +// stallAfter is how long a SINGLE application may stay bit-for-bit identical +// before the wait considers it stalled. Long enough that a slow-but-live // rollout (images pulling, probes settling) keeps resetting it via status // transitions; short enough to leave time to act within the wait budget. const stallAfter = 90 * time.Second -// stallFingerprint captures the observable wait state: ready count plus the -// sorted not-ready list (names with their status strings). Any transition โ€” -// an app appearing, progressing, or changing status โ€” changes the fingerprint. -func stallFingerprint(ready int, notReady []string) string { - sorted := append([]string(nil), notReady...) - sort.Strings(sorted) - return fmt.Sprintf("%d|%s", ready, strings.Join(sorted, ",")) +// stallTracker records, per application, how long its (health, sync) state has +// been unchanged. +// +// It replaces a single global fingerprint over the whole not-ready set (V5): a +// global timer is reset by ANY transition anywhere in the set, so one noisy +// neighbour oscillating Missing<->OutOfSync every tick reset the 90s clock +// forever while a genuinely stuck app sat Healthy+OutOfSync, bit-for-bit +// identical, and never accrued stall time. Tracking each app independently lets +// a stuck app be detected regardless of what its neighbours do. +type stallTracker struct { + states map[string]stallEntry +} + +type stallEntry struct { + state string // "health|sync" + since time.Time // when the app first entered `state` +} + +func newStallTracker() *stallTracker { + return &stallTracker{states: make(map[string]stallEntry)} +} + +// observe records each application's current state, resetting an app's timer +// when its state changes and forgetting apps no longer present (so a +// reappearing app starts its clock fresh rather than inheriting a stale one). +func (s *stallTracker) observe(apps []Application, now time.Time) { + seen := make(map[string]bool, len(apps)) + for _, app := range apps { + seen[app.Name] = true + state := app.Health + "|" + app.Sync + if e, ok := s.states[app.Name]; !ok || e.state != state { + s.states[app.Name] = stallEntry{state: state, since: now} + } + } + for name := range s.states { + if !seen[name] { + delete(s.states, name) + } + } } -// outOfSyncStragglers returns the names of applications that are not ready -// solely because they are OutOfSync (health already fine). These are the ones -// a sync operation can actually move; apps with health problems are excluded โ€” -// syncing them would mask the real failure. -func outOfSyncStragglers(apps []Application) []string { +// stalledStragglers returns the OutOfSync-but-Healthy applications whose state +// has been identical for at least stallAfter. These are the apps a sync can +// actually move (health is already fine, so syncing won't mask a real failure) +// AND that are genuinely stuck, judged per-app rather than across the set. +// +// Callers must observe() this same tick's apps first. +func (s *stallTracker) stalledStragglers(apps []Application, now time.Time) []string { var names []string for _, app := range apps { - if app.Sync == ArgoCDSyncOutOfSync && app.Health == ArgoCDHealthHealthy { + if app.Sync != ArgoCDSyncOutOfSync || app.Health != ArgoCDHealthHealthy { + continue + } + if e, ok := s.states[app.Name]; ok && now.Sub(e.since) >= stallAfter { names = append(names, app.Name) } } diff --git a/internal/chart/providers/argocd/stall_test.go b/internal/chart/providers/argocd/stall_test.go index f934b5f7..e2344463 100644 --- a/internal/chart/providers/argocd/stall_test.go +++ b/internal/chart/providers/argocd/stall_test.go @@ -5,6 +5,7 @@ import ( "fmt" "strings" "testing" + "time" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -19,35 +20,109 @@ func labeledAppObj(name, health, sync string) *unstructured.Unstructured { return o } -func TestStallFingerprint(t *testing.T) { - // Order-insensitive: the same set in any order is the same fingerprint. - a := stallFingerprint(3, []string{"b (Sync: OutOfSync)", "a (Sync: OutOfSync)"}) - b := stallFingerprint(3, []string{"a (Sync: OutOfSync)", "b (Sync: OutOfSync)"}) - if a != b { - t.Errorf("fingerprint must be order-insensitive: %q vs %q", a, b) - } - // Any transition changes it: ready count, membership, or status. - if a == stallFingerprint(4, []string{"a (Sync: OutOfSync)", "b (Sync: OutOfSync)"}) { - t.Error("ready-count change must change the fingerprint") - } - if a == stallFingerprint(3, []string{"a (Sync: OutOfSync)", "b (Health: Degraded)"}) { - t.Error("status change must change the fingerprint") - } -} - -// TestOutOfSyncStragglers: only healthy-but-OutOfSync apps qualify โ€” an app -// with health problems must not be auto-synced (it would mask a real failure). -func TestOutOfSyncStragglers(t *testing.T) { +// TestStallTracker_OnlyHealthyOutOfSyncQualify: only healthy-but-OutOfSync apps +// can be stalled stragglers โ€” an app with health problems must not be +// auto-synced (it would mask a real failure). +func TestStallTracker_OnlyHealthyOutOfSyncQualify(t *testing.T) { apps := []Application{ {Name: "mongodb", Health: ArgoCDHealthHealthy, Sync: ArgoCDSyncOutOfSync}, {Name: "ready", Health: ArgoCDHealthHealthy, Sync: ArgoCDSyncSynced}, - {Name: "broken", Health: "Degraded", Sync: ArgoCDSyncOutOfSync}, + {Name: "broken", Health: ArgoCDHealthDegraded, Sync: ArgoCDSyncOutOfSync}, {Name: "zoo", Health: ArgoCDHealthHealthy, Sync: ArgoCDSyncOutOfSync}, } - got := outOfSyncStragglers(apps) + s := newStallTracker() + t0 := time.Unix(0, 0) + s.observe(apps, t0) + + // Before stallAfter elapses, nothing is stalled. + if got := s.stalledStragglers(apps, t0.Add(stallAfter-time.Second)); len(got) != 0 { + t.Errorf("nothing may be stalled before stallAfter, got %v", got) + } + // After it, only the healthy+OutOfSync apps qualify. + got := s.stalledStragglers(apps, t0.Add(stallAfter)) want := []string{"mongodb", "zoo"} - if len(got) != len(want) || got[0] != want[0] || got[1] != want[1] { - t.Errorf("stragglers = %v, want %v", got, want) + if strings.Join(got, ",") != strings.Join(want, ",") { + t.Errorf("stalled stragglers = %v, want %v", got, want) + } +} + +// TestStallTracker_NoisyNeighbourDoesNotResetStuckApp is the V5 regression: a +// genuinely stuck app (Healthy+OutOfSync, never changing) must be detected even +// while a neighbour oscillates Missing<->OutOfSync every tick. The old global +// fingerprint reset its single timer on every neighbour transition, so the +// stuck app never accrued stall time and the wait rode to its full timeout. +func TestStallTracker_NoisyNeighbourDoesNotResetStuckApp(t *testing.T) { + s := newStallTracker() + base := time.Unix(0, 0) + + // The neighbour flaps on every tick; the stuck app never changes. Tick every + // 10s across more than stallAfter. + flapStates := []struct{ health, sync string }{ + {ArgoCDHealthMissing, ArgoCDSyncOutOfSync}, + {ArgoCDHealthProgressing, ArgoCDSyncOutOfSync}, + } + var last []string + for i := 0; i*10 <= int((stallAfter + 10*time.Second).Seconds()); i++ { + now := base.Add(time.Duration(i*10) * time.Second) + flap := flapStates[i%len(flapStates)] + apps := []Application{ + {Name: "argocd-apps", Health: ArgoCDHealthHealthy, Sync: ArgoCDSyncOutOfSync}, // stone-cold stuck + {Name: "ingress-nginx", Health: flap.health, Sync: flap.sync}, // noisy + } + s.observe(apps, now) + last = s.stalledStragglers(apps, now) + } + + // The stuck app must be reported despite the neighbour's constant churn. + found := false + for _, n := range last { + if n == "argocd-apps" { + found = true + } + } + if !found { + t.Errorf("the stuck app must be detected regardless of a flapping neighbour; got %v", last) + } +} + +// TestStallTracker_ResetsOnStateChange: an app that genuinely progresses (its +// own state changes) restarts its clock and is not reported as stalled. +func TestStallTracker_ResetsOnStateChange(t *testing.T) { + s := newStallTracker() + t0 := time.Unix(0, 0) + + appsV1 := []Application{{Name: "mongodb", Health: ArgoCDHealthHealthy, Sync: ArgoCDSyncOutOfSync}} + s.observe(appsV1, t0) + + // Just before it would stall, its sync flips (a real transition). + appsV2 := []Application{{Name: "mongodb", Health: ArgoCDHealthHealthy, Sync: ArgoCDSyncSynced}} + s.observe(appsV2, t0.Add(stallAfter-time.Second)) + // Then back to OutOfSync โ€” the clock restarted at the transition. + appsV3 := appsV1 + s.observe(appsV3, t0.Add(stallAfter)) + if got := s.stalledStragglers(appsV3, t0.Add(stallAfter+time.Second)); len(got) != 0 { + t.Errorf("an app that transitioned must not be immediately stalled, got %v", got) + } + // But if it now sits still for stallAfter, it stalls. + if got := s.stalledStragglers(appsV3, t0.Add(2*stallAfter)); len(got) != 1 { + t.Errorf("after stallAfter of no change it must stall, got %v", got) + } +} + +// TestStallTracker_ForgetsVanishedApps: an app that disappears and later +// reappears starts a fresh clock rather than inheriting a stale one. +func TestStallTracker_ForgetsVanishedApps(t *testing.T) { + s := newStallTracker() + t0 := time.Unix(0, 0) + apps := []Application{{Name: "mongodb", Health: ArgoCDHealthHealthy, Sync: ArgoCDSyncOutOfSync}} + s.observe(apps, t0) + + // It vanishes from the listing. + s.observe(nil, t0.Add(30*time.Second)) + // It reappears much later โ€” its clock must start now, not at t0. + s.observe(apps, t0.Add(10*time.Minute)) + if got := s.stalledStragglers(apps, t0.Add(10*time.Minute+stallAfter-time.Second)); len(got) != 0 { + t.Errorf("a reappearing app must start a fresh clock, got %v", got) } } diff --git a/internal/chart/providers/argocd/sync.go b/internal/chart/providers/argocd/sync.go index 13e986aa..16fdf678 100644 --- a/internal/chart/providers/argocd/sync.go +++ b/internal/chart/providers/argocd/sync.go @@ -3,6 +3,7 @@ package argocd import ( "context" "fmt" + "strings" "time" "github.com/pterm/pterm" @@ -97,17 +98,43 @@ func (m *Manager) RefreshAndSync(ctx context.Context, prune bool) error { return m.syncChildApplications(ctx, prune) } -// trackingInstanceLabel is ArgoCD's default (label) resource-tracking marker: -// resources managed by an Application carry app.kubernetes.io/instance=. -// Child Applications created by the app-of-apps therefore carry the root's name. +// trackingInstanceLabel is ArgoCD's LABEL resource-tracking marker: resources +// managed by an Application carry app.kubernetes.io/instance=. Child +// Applications created by the app-of-apps therefore carry the root's name. const trackingInstanceLabel = "app.kubernetes.io/instance" +// trackingIDAnnotation is ArgoCD's ANNOTATION resource-tracking marker, used +// when resourceTrackingMethod is "annotation" or "annotation+label" (the label +// above is then absent). Its value is ":/:/", +// e.g. "argocd-apps:argoproj.io/Application:argocd/openframe-api". +const trackingIDAnnotation = "argocd.argoproj.io/tracking-id" + +// trackingOwner returns the owning Application name encoded in either tracking +// marker, or "" if neither is present. The label wins when set; otherwise the +// annotation's owner is the segment before the first ":". Splitting (rather +// than prefix-matching) avoids mistaking "argocd-apps-foo" for "argocd-apps". +func trackingOwner(labels, annotations map[string]string) string { + if v := labels[trackingInstanceLabel]; v != "" { + return v + } + if id := annotations[trackingIDAnnotation]; id != "" { + return strings.SplitN(id, ":", 2)[0] + } + return "" +} + // syncChildApplications triggers a sync on the root's child Applications. -// Children are selected by ArgoCD's tracking label -// (app.kubernetes.io/instance=argocd-apps); when none carry it (custom -// trackingMethod), it falls back to every Application except the root rather -// than silently syncing nothing โ€” but then it may touch Applications that are -// not OpenFrame-owned, which the fallback warning makes visible. +// Children are selected by ArgoCD's resource tracking, checking BOTH markers: +// the label (app.kubernetes.io/instance=argocd-apps) and the annotation +// (argocd.argoproj.io/tracking-id, owner before the first ":"). The annotation +// is required because ArgoCD's "annotation" / "annotation+label" tracking +// methods leave the label empty โ€” the case the verification run hit, where the +// primary selector matched nothing and the fallback synced everything. +// +// Only when NEITHER marker is present on any Application does it fall back to +// every Application except the root, rather than silently syncing nothing โ€” +// but that may touch Applications that are not OpenFrame-owned (a real risk on +// a shared cluster), which the fallback warning makes visible. // // Per-child failures no longer vanish (audit F8): individual errors are // counted and surfaced โ€” a warning on partial failure, an error when NOT ONE @@ -126,7 +153,7 @@ func (m *Manager) syncChildApplications(ctx context.Context, prune bool) error { if name == AppOfAppsName { continue } - if list.Items[i].GetLabels()[trackingInstanceLabel] == AppOfAppsName { + if trackingOwner(list.Items[i].GetLabels(), list.Items[i].GetAnnotations()) == AppOfAppsName { children = append(children, name) } } diff --git a/internal/chart/providers/argocd/sync_tracking_test.go b/internal/chart/providers/argocd/sync_tracking_test.go new file mode 100644 index 00000000..24455520 --- /dev/null +++ b/internal/chart/providers/argocd/sync_tracking_test.go @@ -0,0 +1,119 @@ +package argocd + +import ( + "context" + goruntime "runtime" + "sort" + "testing" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + k8stesting "k8s.io/client-go/testing" +) + +// annotatedAppObj builds an Application tracked by ArgoCD's ANNOTATION method: +// the app.kubernetes.io/instance label is absent, and ownership lives only in +// argocd.argoproj.io/tracking-id (":/:/"). +func annotatedAppObj(name, owner string) *unstructured.Unstructured { + o := appObj(name, ArgoCDHealthHealthy, ArgoCDSyncOutOfSync) + meta := o.Object["metadata"].(map[string]interface{}) + meta["annotations"] = map[string]interface{}{ + trackingIDAnnotation: owner + ":argoproj.io/Application:" + ArgoCDNamespace + "/" + name, + } + return o +} + +// patchedNames captures the names of every Application the sync patches. +func patchedNames(m *Manager) *[]string { + var names []string + dc := m.dynamicClient.(interface { + PrependReactor(verb, resource string, fn k8stesting.ReactionFunc) + }) + dc.PrependReactor("patch", "applications", func(action k8stesting.Action) (bool, runtime.Object, error) { + names = append(names, action.(k8stesting.PatchAction).GetName()) + return false, nil, nil + }) + return &names +} + +// TestTrackingOwner is the pure selection logic: label wins; otherwise the +// annotation's owner is the segment before the first ":"; a look-alike name is +// not matched; neither marker yields "". +func TestTrackingOwner(t *testing.T) { + cases := []struct { + name string + labels map[string]string + annotations map[string]string + want string + }{ + {"label", map[string]string{trackingInstanceLabel: "argocd-apps"}, nil, "argocd-apps"}, + {"annotation", nil, map[string]string{trackingIDAnnotation: "argocd-apps:argoproj.io/Application:argocd/api"}, "argocd-apps"}, + {"label wins over annotation", map[string]string{trackingInstanceLabel: "argocd-apps"}, map[string]string{trackingIDAnnotation: "other:x/y:z/w"}, "argocd-apps"}, + {"lookalike not matched", nil, map[string]string{trackingIDAnnotation: "argocd-apps-foo:argoproj.io/Application:argocd/api"}, "argocd-apps-foo"}, + {"neither", nil, nil, ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := trackingOwner(tc.labels, tc.annotations); got != tc.want { + t.Errorf("trackingOwner = %q, want %q", got, tc.want) + } + }) + } +} + +// TestSyncChildApplications_SelectsByAnnotation is the finding fix: on an +// annotation-tracking cluster (no instance label) children are still selected +// by the tracking-id annotation, a foreign Application (different owner) is +// left alone, and the "sync everything" fallback does NOT fire. +func TestSyncChildApplications_SelectsByAnnotation(t *testing.T) { + if goruntime.GOOS == "windows" { + t.Skip("native cluster ops are refused on Windows (must run inside WSL)") + } + m := fakeManager( + appObj(AppOfAppsName, ArgoCDHealthHealthy, ArgoCDSyncSynced), // root, no tracking marker + annotatedAppObj("openframe-api", AppOfAppsName), + annotatedAppObj("openframe-ui", AppOfAppsName), + annotatedAppObj("tenants-billing", "some-other-root"), // foreign owner + ) + got := patchedNames(m) + + if err := m.syncChildApplications(context.Background(), false); err != nil { + t.Fatalf("syncChildApplications: %v", err) + } + sort.Strings(*got) + want := []string{"openframe-api", "openframe-ui"} + if len(*got) != 2 || (*got)[0] != want[0] || (*got)[1] != want[1] { + t.Errorf("synced %v, want %v (annotation-owned children only)", *got, want) + } + for _, n := range *got { + if n == "tenants-billing" { + t.Error("a foreign-owned Application must not be synced") + } + if n == AppOfAppsName { + t.Error("the root must never sync itself here") + } + } +} + +// TestSyncChildApplications_FallbackWhenNoTracking preserves the safety net: +// when NEITHER marker is present anywhere, every non-root Application is synced +// (better than silently syncing nothing), with the visible warning. +func TestSyncChildApplications_FallbackWhenNoTracking(t *testing.T) { + if goruntime.GOOS == "windows" { + t.Skip("native cluster ops are refused on Windows (must run inside WSL)") + } + m := fakeManager( + appObj(AppOfAppsName, ArgoCDHealthHealthy, ArgoCDSyncSynced), + appObj("untracked-a", ArgoCDHealthHealthy, ArgoCDSyncOutOfSync), + appObj("untracked-b", ArgoCDHealthHealthy, ArgoCDSyncOutOfSync), + ) + got := patchedNames(m) + + if err := m.syncChildApplications(context.Background(), false); err != nil { + t.Fatalf("syncChildApplications: %v", err) + } + sort.Strings(*got) + if len(*got) != 2 || (*got)[0] != "untracked-a" || (*got)[1] != "untracked-b" { + t.Errorf("fallback must sync all non-root apps, got %v", *got) + } +} diff --git a/internal/chart/providers/argocd/values.go b/internal/chart/providers/argocd/values.go index f115127a..b77d92f7 100644 --- a/internal/chart/providers/argocd/values.go +++ b/internal/chart/providers/argocd/values.go @@ -1,6 +1,12 @@ package argocd -import _ "embed" +import ( + _ "embed" + "fmt" + "sort" + + "sigs.k8s.io/yaml" +) // argoCDValues is the baseline Helm values for the Argo CD chart, embedded from // argocd-values.yaml at build time. Keeping it as a real YAML file (rather than @@ -12,7 +18,79 @@ import _ "embed" //go:embed argocd-values.yaml var argoCDValues string +// UserArgoCDKey is the top-level key in the user's openframe-helm-values.yaml +// whose subtree overrides this baseline for the Argo CD install. +// +// It is a DEDICATED key, not the whole file, on purpose. The rest of that file +// uses the flattened app-of-apps schema (repository.branch, registry.docker.*) +// and is meant for the app-of-apps chart โ€” merging it into the Argo CD chart +// would both mismatch schemas and leak the docker registry password into the +// argo-cd release. Scoping to `argocd:` keeps the two value streams (and the +// secret) separate. +const UserArgoCDKey = "argocd" + // GetArgoCDValues returns the baseline ArgoCD Helm chart values as a YAML string. func GetArgoCDValues() string { return argoCDValues } + +// MergedArgoCDValues deep-merges the user's `argocd:` overrides over the +// embedded baseline and returns the YAML to feed helm plus the sorted top-level +// override keys (for a visible warning). userValues is the whole parsed user +// file; when it has no non-empty `argocd:` subtree the baseline is returned +// byte-for-byte unchanged with a nil key list, so the common path is untouched. +// +// Merge semantics match helm's: maps merge recursively, scalars and lists +// replace. Overriding is the user's explicit choice, so nothing in the baseline +// is protected from being replaced โ€” the caller warns loudly instead. +func MergedArgoCDValues(userValues map[string]interface{}) (string, []string, error) { + raw, present := userValues[UserArgoCDKey] + // Absent, or bare `argocd:` (null), or `argocd: {}` โ€” nothing to override. + if !present || raw == nil { + return argoCDValues, nil, nil + } + // Present but not a mapping (scalar/list/typo'd indentation) is a mistake, + // not a no-op: fail loudly so the caller surfaces it, rather than silently + // dropping the user's intended override (same silent-failure class as V3). + sub, ok := raw.(map[string]interface{}) + if !ok { + return "", nil, fmt.Errorf("%q in the values file must be a mapping of ArgoCD chart values, got %T", UserArgoCDKey, raw) + } + if len(sub) == 0 { + return argoCDValues, nil, nil + } + + var base map[string]interface{} + if err := yaml.Unmarshal([]byte(argoCDValues), &base); err != nil { + return "", nil, fmt.Errorf("parsing embedded ArgoCD values: %w", err) + } + deepMerge(base, sub) + + out, err := yaml.Marshal(base) + if err != nil { + return "", nil, fmt.Errorf("marshaling merged ArgoCD values: %w", err) + } + + keys := make([]string, 0, len(sub)) + for k := range sub { + keys = append(keys, k) + } + sort.Strings(keys) + return string(out), keys, 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{}) { + for k, sv := range src { + if dv, ok := dst[k]; ok { + if dm, ok1 := dv.(map[string]interface{}); ok1 { + if sm, ok2 := sv.(map[string]interface{}); ok2 { + deepMerge(dm, sm) + continue + } + } + } + dst[k] = sv + } +} diff --git a/internal/chart/providers/argocd/values_merge_test.go b/internal/chart/providers/argocd/values_merge_test.go new file mode 100644 index 00000000..5d24f9ec --- /dev/null +++ b/internal/chart/providers/argocd/values_merge_test.go @@ -0,0 +1,188 @@ +package argocd + +import ( + "strings" + "testing" + + "sigs.k8s.io/yaml" +) + +// parseMerged unmarshals a merged-values YAML string into a generic map. +func parseMerged(t *testing.T, s string) map[string]interface{} { + t.Helper() + var m map[string]interface{} + if err := yaml.Unmarshal([]byte(s), &m); err != nil { + t.Fatalf("merged values not valid YAML: %v", err) + } + return m +} + +// TestMergedArgoCDValues_NoOverridesReturnsBaselineVerbatim: without an +// `argocd:` subtree the baseline is returned byte-for-byte and no keys are +// reported, so the common install path is completely unchanged. +func TestMergedArgoCDValues_NoOverridesReturnsBaselineVerbatim(t *testing.T) { + for _, uv := range []map[string]interface{}{ + nil, + {"repository": map[string]interface{}{"branch": "main"}}, // app-of-apps keys, no argocd + {UserArgoCDKey: map[string]interface{}{}}, // present but empty (argocd: {}) + {UserArgoCDKey: nil}, // bare `argocd:` (null) + } { + out, keys, err := MergedArgoCDValues(uv) + if err != nil { + t.Fatalf("MergedArgoCDValues: %v", err) + } + if out != GetArgoCDValues() { + t.Error("baseline must be returned verbatim when there are no argocd overrides") + } + if len(keys) != 0 { + t.Errorf("no override keys expected, got %v", keys) + } + } +} + +// TestMergedArgoCDValues_ReEnableDex is the motivating case: a user can flip a +// baseline default back (dex.enabled false -> true) via the file, without a +// flag, and the rest of the baseline (developer account, controller args) must +// survive the merge. +func TestMergedArgoCDValues_ReEnableDex(t *testing.T) { + uv := map[string]interface{}{ + UserArgoCDKey: map[string]interface{}{ + "dex": map[string]interface{}{"enabled": true}, + }, + } + out, keys, err := MergedArgoCDValues(uv) + if err != nil { + t.Fatalf("MergedArgoCDValues: %v", err) + } + if strings.Join(keys, ",") != "dex" { + t.Errorf("override keys = %v, want [dex]", keys) + } + + m := parseMerged(t, out) + dex, _ := m["dex"].(map[string]interface{}) + if dex["enabled"] != true { + t.Errorf("dex.enabled must be overridden to true, got %v", dex["enabled"]) + } + // Baseline must survive: the developer bcrypt account and fullnameOverride. + if m["fullnameOverride"] != "argocd" { + t.Error("merge dropped fullnameOverride from the baseline") + } + configs, _ := m["configs"].(map[string]interface{}) + secret, _ := configs["secret"].(map[string]interface{}) + extra, _ := secret["extra"].(map[string]interface{}) + if pw, _ := extra["accounts.developer.password"].(string); !strings.HasPrefix(pw, "$2a$") { + t.Error("merge dropped the developer account bcrypt hash from the baseline") + } +} + +// TestMergedArgoCDValues_DeepMergeAndListReplace: nested maps merge (sibling +// keys under a shared parent survive), while a list value replaces wholesale +// (helm semantics). +func TestMergedArgoCDValues_DeepMergeAndListReplace(t *testing.T) { + uv := map[string]interface{}{ + UserArgoCDKey: map[string]interface{}{ + "configs": map[string]interface{}{ + "params": map[string]interface{}{"server.insecure": "true"}, // new sibling + }, + "controller": map[string]interface{}{ + "extraArgs": []interface{}{"--custom"}, // replaces the baseline list + }, + }, + } + out, _, err := MergedArgoCDValues(uv) + if err != nil { + t.Fatalf("MergedArgoCDValues: %v", err) + } + m := parseMerged(t, out) + configs, _ := m["configs"].(map[string]interface{}) + params, _ := configs["params"].(map[string]interface{}) + // New sibling added... + if params["server.insecure"] != "true" { + t.Errorf("nested map merge lost the new param, got %v", params["server.insecure"]) + } + // ...without wiping the baseline siblings under the same parent. + if params["controller.sync.timeout.seconds"] == nil { + t.Error("deep merge wiped a baseline sibling param") + } + // The list replaced, not appended. + ctrl, _ := m["controller"].(map[string]interface{}) + args, _ := ctrl["extraArgs"].([]interface{}) + if len(args) != 1 || args[0] != "--custom" { + t.Errorf("list value must replace wholesale, got %v", args) + } +} + +// TestMergedArgoCDValues_MultipleKeysSorted: the reported override keys are +// sorted (stable, greppable warning output). +func TestMergedArgoCDValues_MultipleKeysSorted(t *testing.T) { + uv := map[string]interface{}{ + UserArgoCDKey: map[string]interface{}{ + "server": map[string]interface{}{"replicas": 2}, + "dex": map[string]interface{}{"enabled": true}, + "controller": map[string]interface{}{"replicas": 1}, + }, + } + _, keys, err := MergedArgoCDValues(uv) + if err != nil { + t.Fatalf("MergedArgoCDValues: %v", err) + } + if strings.Join(keys, ",") != "controller,dex,server" { + t.Errorf("keys must be sorted, got %v", keys) + } +} + +// TestDeepMerge_Semantics unit-tests the merge rule directly. +func TestDeepMerge_Semantics(t *testing.T) { + dst := map[string]interface{}{ + "keep": "baseline", + "nested": map[string]interface{}{"a": 1, "b": 2}, + "list": []interface{}{"x", "y"}, + } + src := map[string]interface{}{ + "nested": map[string]interface{}{"b": 20, "c": 3}, // merge: a survives, b replaced, c added + "list": []interface{}{"z"}, // replace + "new": "added", + } + deepMerge(dst, src) + + if dst["keep"] != "baseline" { + t.Error("untouched key must survive") + } + n := dst["nested"].(map[string]interface{}) + if n["a"] != 1 || n["b"] != 20 || n["c"] != 3 { + t.Errorf("nested merge wrong: %v", n) + } + if l := dst["list"].([]interface{}); len(l) != 1 || l[0] != "z" { + t.Errorf("list must replace, got %v", l) + } + if dst["new"] != "added" { + t.Error("new key must be added") + } +} + +// TestMergedArgoCDValues_MalformedOverrideErrors: a present-but-non-map argocd: +// value (scalar, list, typo'd indentation) is a mistake, not a no-op. It must +// error so installArgoCDHelm surfaces it, rather than silently dropping the +// user's intended override (the V3 silent-failure class). +func TestMergedArgoCDValues_MalformedOverrideErrors(t *testing.T) { + cases := map[string]interface{}{ + "scalar bool": true, + "scalar string": "enabled", + "list": []interface{}{"dex"}, + "number": 42, + } + for name, bad := range cases { + t.Run(name, func(t *testing.T) { + out, keys, err := MergedArgoCDValues(map[string]interface{}{UserArgoCDKey: bad}) + if err == nil { + t.Fatalf("a malformed %q override must error, got out=%q keys=%v", UserArgoCDKey, out, keys) + } + if !strings.Contains(err.Error(), UserArgoCDKey) || !strings.Contains(err.Error(), "mapping") { + t.Errorf("error must name the key and say it must be a mapping, got: %v", err) + } + if out != "" { + t.Errorf("on error the values string must be empty, got %q", out) + } + }) + } +} diff --git a/internal/chart/providers/argocd/values_test.go b/internal/chart/providers/argocd/values_test.go index e1a95556..ae4a2e97 100644 --- a/internal/chart/providers/argocd/values_test.go +++ b/internal/chart/providers/argocd/values_test.go @@ -118,12 +118,13 @@ func TestArgoCDValues_ControllerExtraArgs(t *testing.T) { // scheduling on the small k3d clusters the CLI targets. func TestArgoCDValues_AllComponentsHaveResources(t *testing.T) { v := parseValues(t) + // dex is intentionally excluded: it is disabled (see TestArgoCDValues_DexDisabled), + // so it schedules no pod and needs no resource block. components := map[string]component{ "controller": v.Controller, "server": v.Server, "repoServer": v.RepoServer, "redis": v.Redis, - "dex": v.Dex, "applicationSet": v.ApplicationSet, "notifications": v.Notifications, } @@ -138,3 +139,25 @@ func TestArgoCDValues_AllComponentsHaveResources(t *testing.T) { } } } + +// TestArgoCDValues_DexDisabled locks the V1 blocker fix: the argo-cd chart +// defaults dex.enabled to true, and the dexidp/dex:v2.45.1 arm64 image +// intermittently SIGSEGVs under emulation on Apple Silicon, CrashLoopBackOff-ing +// the 7-minute `helm --wait` into a fresh-install failure. OpenFrame's login +// uses the local developer account, never dex, so it must be disabled. +func TestArgoCDValues_DexDisabled(t *testing.T) { + var raw struct { + Dex struct { + Enabled *bool `yaml:"enabled"` + } `yaml:"dex"` + } + if err := yaml.Unmarshal([]byte(GetArgoCDValues()), &raw); err != nil { + t.Fatal(err) + } + if raw.Dex.Enabled == nil { + t.Fatal("dex.enabled must be set explicitly (chart default is true); it is absent") + } + if *raw.Dex.Enabled { + t.Error("dex.enabled must be false โ€” dex is unused and its arm64 image crashes on Apple Silicon") + } +} diff --git a/internal/chart/providers/argocd/wait.go b/internal/chart/providers/argocd/wait.go index 6f0c70f4..a2669461 100644 --- a/internal/chart/providers/argocd/wait.go +++ b/internal/chart/providers/argocd/wait.go @@ -179,9 +179,8 @@ func (m *Manager) WaitForApplications(ctx context.Context, config config.ChartIn // Once an app is ready, it stays counted even if it temporarily goes out of sync everReadyApps := make(map[string]bool) - // Stall tracking (finding N3) โ€” see stall.go. - lastStallFingerprint := "" - lastProgressAt := time.Now() + // Stall tracking (finding N3, per-application โ€” see stall.go). + stall := newStallTracker() stragglerSyncTriggered := false stallHintShown := false @@ -367,17 +366,25 @@ func (m *Manager) WaitForApplications(ctx context.Context, config config.ChartIn lastNotReadyApps, lastReadyCount, lastTotalApps = notReadyApps, currentlyReady, totalApps lastNotReadyNames = assess.notReadyNames - // Stall handling (finding N3): when the state has been bit-for-bit - // identical for stallAfter and OutOfSync-but-healthy stragglers - // remain, waiting further is futile for apps with autoSync off. + // 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. - if fp := stallFingerprint(currentlyReady, notReadyApps); fp != lastStallFingerprint { - lastStallFingerprint = fp - lastProgressAt = time.Now() - } else if len(notReadyApps) > 0 && time.Since(lastProgressAt) > stallAfter { - if stragglers := outOfSyncStragglers(apps); len(stragglers) > 0 { - if config.SyncStragglersOnStall && !stragglerSyncTriggered { + // 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 + // already fired: on the upgrade path (SyncStragglersOnStall) the hint + // must never print โ€” it tells the user to run `app upgrade --sync`, + // which is exactly the path they are already on. Gating the hint on + // !stragglerSyncTriggered instead would print that contradictory + // advice on every stall tick after the one-shot sync. + if config.SyncStragglersOnStall { + if !stragglerSyncTriggered { stragglerSyncTriggered = true pterm.Warning.Printf("No progress for %s; triggering sync of %d OutOfSync application(s): %v\n", stallAfter.Round(time.Second), len(stragglers), stragglers) @@ -385,14 +392,12 @@ func (m *Manager) WaitForApplications(ctx context.Context, config config.ChartIn if failedCount > 0 { pterm.Warning.Printf("Straggler sync: %d triggered, %d failed (first error: %v)\n", patched, failedCount, syncErr) } - lastProgressAt = time.Now() // give the sync room to act - } else if !stallHintShown { - stallHintShown = true - pterm.Warning.Printf("No progress for %s; %d application(s) are OutOfSync and may have auto-sync disabled: %v\n", - stallAfter.Round(time.Second), len(stragglers), stragglers) - pterm.Info.Println("They will not sync on their own โ€” run `openframe app upgrade --sync` (or sync them in ArgoCD) to roll them out.") - lastProgressAt = time.Now() // print the hint once per stall, not every tick } + } else if !stallHintShown { + stallHintShown = true + pterm.Warning.Printf("No progress for %s; %d application(s) are OutOfSync and may have auto-sync disabled: %v\n", + stallAfter.Round(time.Second), len(stragglers), stragglers) + pterm.Info.Println("They will not sync on their own โ€” run `openframe app upgrade --sync` (or sync them in ArgoCD) to roll them out.") } } @@ -447,6 +452,14 @@ func (m *Manager) WaitForApplications(ctx context.Context, config config.ChartIn if m.triggerRepoServerRecovery(localCtx, app.Name) { pterm.Info.Println("ArgoCD repo-server restarted; applications will re-sync shortly.") delete(appsWithRepoServerIssues, app.Name) + // The restarted repo-server has a cold manifest cache, so + // every app stuck in Unknown (not just the trigger) needs a + // HARD refresh to regenerate โ€” otherwise they ride the wait + // out to its timeout. triggerRepoServerRecovery already + // hard-refreshed app.Name; cover the rest. + if refreshed := m.hardRefreshApplications(localCtx, appNames(unknownApps)); refreshed > 0 { + pterm.Info.Printfln("Hard-refreshed %d application(s) stuck in Unknown.", refreshed) + } } else { pterm.Warning.Println("Could not restart the ArgoCD repo-server; continuing to wait.") } @@ -539,12 +552,33 @@ func (m *Manager) WaitForApplications(ctx context.Context, config config.ChartIn pterm.Debug.Printf("All apps ready (%d/%d stabilization checks)\n", consecutiveAllReady, stabilizationChecks) } if consecutiveAllReady >= stabilizationChecks { + // Everything is Healthy+Synced โ€” but "ready" is not "correct". + // If a ref was requested, confirm ArgoCD is actually tracking it + // before declaring success; a legacy branch's chart silently + // deploys main and this is the only place that catches it (V3). + // Decide the spinner's final state from the outcome: FAIL on a + // mismatch (matching the timeout path), a neutral Stop on success + // โ€” never a neutral stop immediately before returning an error. + var mm []refMismatch + if config.AppOfApps != nil { + mm = verifyRefPinning(apps, config.AppOfApps.GitHubRepo, config.AppOfApps.GitHubBranch) + } + spinnerMutex.Lock() if !spinnerStopped && spinner != nil { - spinner.Stop() + if len(mm) > 0 { + spinner.Fail("Deployed ref does not match the requested ref") + } else { + spinner.Stop() + } spinnerStopped = true } spinnerMutex.Unlock() + + if len(mm) > 0 { + return refMismatchError(config.AppOfApps.GitHubBranch, mm) + } + pterm.Success.Println("All ArgoCD applications installed") return nil } diff --git a/internal/chart/providers/helm/argocd_wait.go b/internal/chart/providers/helm/argocd_wait.go index 7d3c044b..6b69fa10 100644 --- a/internal/chart/providers/helm/argocd_wait.go +++ b/internal/chart/providers/helm/argocd_wait.go @@ -12,7 +12,6 @@ import ( "github.com/flamingo-stack/openframe-cli/internal/k8s" "github.com/flamingo-stack/openframe-cli/internal/platform" "github.com/flamingo-stack/openframe-cli/internal/shared/executor" - "github.com/flamingo-stack/openframe-cli/internal/shared/redact" "github.com/pterm/pterm" corev1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" @@ -217,12 +216,11 @@ func (h *HelmManager) verifyHelmRelease(ctx context.Context, releaseName, namesp return fmt.Errorf("failed to run helm list: %w", err) } - // Log the helm list output. Redact at the print site (the struct value is - // parsed below): helm output may carry values echoed back from the release. - if verbose { - pterm.Info.Println("Helm list output:") - pterm.Println(redact.Redact(result.Stdout)) - } + // The raw `helm list` JSON is not printed even under --verbose (V6): it is an + // implementation detail of the existence check below, and dumping it (plus + // the full `helm status` inventory further down) buried the useful lines + // under 150+ lines of noise. The compact "verified successfully" line at the + // end carries the signal. // Check if the release exists in the output // The JSON output will be an empty array "[]" if no releases found @@ -231,27 +229,24 @@ func (h *HelmManager) verifyHelmRelease(ctx context.Context, releaseName, namesp return fmt.Errorf("helm release '%s' not found in namespace '%s' - helm list returned empty", releaseName, namespace) } - // Also run helm status for more details + // Also run `helm status` as a second existence/health probe. Only its exit + // status matters โ€” a release that exists but whose status errors is a real + // problem. The command's multi-hundred-line resource inventory + NOTES are + // intentionally discarded, not printed even under --verbose (V6). statusArgs := []string{"status", releaseName, "-n", namespace} if clusterName != "" { contextName := k8s.ResolveContextForCluster(k8s.DefaultKubeconfigPath(), clusterName) statusArgs = append(statusArgs, "--kube-context", contextName) } - statusResult, err := h.executor.ExecuteWithOptions(ctx, executor.ExecuteOptions{ + if _, err = h.executor.ExecuteWithOptions(ctx, executor.ExecuteOptions{ Command: "helm", Args: statusArgs, Env: h.getHelmEnv(), - }) - if err != nil { + }); err != nil { return fmt.Errorf("helm release exists but status check failed: %w", err) } - if verbose { - pterm.Info.Println("Helm status output:") - pterm.Println(redact.Redact(statusResult.Stdout)) - } - pterm.Success.Printf("Helm release '%s' verified successfully\n", releaseName) return nil } diff --git a/internal/chart/providers/helm/manager.go b/internal/chart/providers/helm/manager.go index 303e8406..f5d82c5d 100644 --- a/internal/chart/providers/helm/manager.go +++ b/internal/chart/providers/helm/manager.go @@ -27,6 +27,7 @@ import ( "k8s.io/client-go/dynamic" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" + "sigs.k8s.io/yaml" ) // HelmManager handles Helm operations @@ -246,14 +247,58 @@ func (h *HelmManager) installArgoCDHelm(ctx context.Context, cfg config.ChartIns if cfg.Verbose { pterm.Debug.Printf("Executing: helm %s\n", strings.Join(args, " ")) } + + // The ArgoCD chart's values are the embedded baseline, optionally overridden + // by the user's `argocd:` subtree in openframe-helm-values.yaml. Only that + // subtree is merged (never the whole file โ€” the rest targets the app-of-apps + // 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 { + merged, overridden, err := argocd.MergedArgoCDValues(uv) + if err != nil { + return nil, fmt.Errorf("merging ArgoCD overrides from %s: %w", path, err) + } + if len(overridden) > 0 { + pterm.Warning.Printfln("Using ArgoCD overrides from %s (keys: %s) on top of the built-in baseline "+ + "(differs from the bundled argocd-values.yaml); a bad override can break the ArgoCD install.", + path, strings.Join(overridden, ", ")) + values = merged + } + } + return h.executor.ExecuteWithOptions(ctx, executor.ExecuteOptions{ Command: "helm", Args: args, Env: h.getHelmEnv(), - Stdin: []byte(argocd.GetArgoCDValues()), + Stdin: []byte(values), }) } +// 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) { + path := "" + if cfg.AppOfApps != nil { + path = cfg.AppOfApps.ValuesFile + } + if path == "" { + path = config.NewPathResolver().GetHelmValuesFile() + } + data, err := os.ReadFile(path) // #nosec G304 -- values path resolved from config/CLI, read as the invoking user + if err != nil { + return nil, path + } + var m map[string]interface{} + if err := yaml.Unmarshal(data, &m); err != nil { + return nil, path + } + return m, path +} + // InstallArgoCDWithProgress installs ArgoCD using Helm with progress indicators func (h *HelmManager) InstallArgoCDWithProgress(ctx context.Context, config config.ChartInstallConfig) error { // Show progress for each step only if not in silent/non-interactive mode @@ -372,7 +417,18 @@ func (h *HelmManager) InstallArgoCDWithProgress(ctx context.Context, config conf pterm.Info.Println("Running in dry-run mode...") } - result, err := h.installArgoCDHelm(ctx, config) + // installArgoCDHelm blocks on `helm upgrade --wait --timeout 7m`, which + // prints nothing while it runs. On a TTY the spinner animates; when there is + // no spinner (non-interactive/CI) the terminal would sit silent for minutes + // and users kill the process before the diagnostics ever print. A heartbeat + // gives that path liveness (no-op under --silent, and scoped to this call). + result, err := func() (*executor.CommandResult, error) { + if spinner == nil { + hb := uispinner.StartHeartbeat("Still installing ArgoCD (helm --wait, up to 7m)...", 0) + defer hb.Stop() + } + return h.installArgoCDHelm(ctx, config) + }() if err != nil { // Check if the error is due to context cancellation (CTRL-C) if ctx.Err() == context.Canceled { @@ -406,20 +462,13 @@ func (h *HelmManager) InstallArgoCDWithProgress(ctx context.Context, config conf return fmt.Errorf("failed to install ArgoCD: %w", err) } - // Log Helm output for debugging (helps identify if Helm actually created - // resources). Stdout is redacted at the PRINT site (not in the result - // struct, which callers parse): helm's rendered output can echo values โ€” - // including the docker registry password โ€” in verbose/dry-run mode. - // Stderr arrives already redacted by the executor. - if config.Verbose && result != nil { - if result.Stdout != "" { - pterm.Info.Println("Helm stdout:") - pterm.Println(redact.Redact(result.Stdout)) - } - if result.Stderr != "" { - pterm.Info.Println("Helm stderr:") - pterm.Println(result.Stderr) - } + // On success, helm's stdout is just the release summary + chart NOTES (100+ + // lines) โ€” pure noise that buried the useful --verbose output (V6), so it is + // not printed. Stderr, when present, carries deprecation/ownership warnings + // worth seeing; it arrives already redacted by the executor. + if config.Verbose && result != nil && result.Stderr != "" { + pterm.Info.Println("Helm stderr:") + pterm.Println(result.Stderr) } // Dry-run creates nothing: helm ran with --dry-run=client and the executor @@ -601,12 +650,21 @@ func (h *HelmManager) InstallAppOfAppsFromLocal(ctx context.Context, config conf pterm.Info.Println("Installing the OpenFrame app-of-apps chart...") } - // Execute helm command with local chart path - result, err := h.executor.ExecuteWithOptions(ctx, executor.ExecuteOptions{ - Command: "helm", - Args: args, - Env: h.getHelmEnv(), - }) + // Execute helm command with local chart path. Like the ArgoCD install this + // blocks on `helm --wait` with no output; when there is no animated spinner + // (non-interactive/CI) a heartbeat keeps the terminal alive so users don't + // assume a hang. No-op under --silent; scoped to the blocking call. + result, err := func() (*executor.CommandResult, error) { + if spinner == nil { + hb := uispinner.StartHeartbeat("Still installing the app-of-apps chart (helm --wait)...", 0) + defer hb.Stop() + } + return h.executor.ExecuteWithOptions(ctx, executor.ExecuteOptions{ + Command: "helm", + Args: args, + Env: h.getHelmEnv(), + }) + }() if err != nil { if spinner != nil { diff --git a/internal/chart/ui/operations_test.go b/internal/chart/ui/operations_test.go index 4b335e2f..7cd83881 100644 --- a/internal/chart/ui/operations_test.go +++ b/internal/chart/ui/operations_test.go @@ -116,7 +116,6 @@ func TestSelectClusterForInstall_EmptyClusterList(t *testing.T) { assert.Empty(t, selectedCluster) } - func TestShowNoClusterMessage(t *testing.T) { ui := NewOperationsUI() @@ -125,9 +124,3 @@ func TestShowNoClusterMessage(t *testing.T) { ui.ShowNoClusterMessage() }) } - - - - - - diff --git a/internal/chart/utils/types/configuration.go b/internal/chart/utils/types/configuration.go index a1b0fb61..d0f99b60 100644 --- a/internal/chart/utils/types/configuration.go +++ b/internal/chart/utils/types/configuration.go @@ -2,7 +2,6 @@ package types import ( "time" - ) // DockerRegistryConfig holds Docker registry settings @@ -76,4 +75,3 @@ type ChartConfiguration struct { DockerRegistry *DockerRegistryConfig // nil means use existing, otherwise use this value IngressConfig *IngressConfig // nil means use existing, otherwise use this value } - diff --git a/internal/chart/utils/types/interfaces.go b/internal/chart/utils/types/interfaces.go index 6f50c42d..aa3295fb 100644 --- a/internal/chart/utils/types/interfaces.go +++ b/internal/chart/utils/types/interfaces.go @@ -47,12 +47,12 @@ type AppOfAppsService interface { // InstallationRequest contains all parameters for chart installation type InstallationRequest struct { - Args []string - Force bool - DryRun bool - Verbose bool - GitHubRepo string - GitHubBranch string + Args []string + Force bool + DryRun bool + Verbose bool + GitHubRepo string + GitHubBranch string // GitHubRefExplicit is true when the operator explicitly set --ref. // When set, GitHubBranch is pinned into the helm values (repository.branch) so it // wins over the values-file branch and both the app-of-apps clone and the child diff --git a/internal/cluster/prerequisites/sets.go b/internal/cluster/prerequisites/sets.go index 1b0688e6..a32dd316 100644 --- a/internal/cluster/prerequisites/sets.go +++ b/internal/cluster/prerequisites/sets.go @@ -30,6 +30,15 @@ func ClusterSet() fw.Set { IsSatisfied: docker.IsDockerRunning, Install: asCtxInstall(dockerInstaller.Install), DocsURL: dockerInstaller.GetInstallHelp(), + // When the binary is present but the daemon is down, say so instead + // of the framework's default "not installed" โ€” the fix a user needs + // (start the daemon) is different from installing it. + Detail: func() string { + if dockerInstaller.IsInstalled() { + return "installed but not running โ€” start Docker Desktop or the Docker daemon" + } + return "" // genuinely absent: let the generic "not installed" wording stand + }, }, toolPrerequisite("k3d", k3dInstaller.IsInstalled, k3dInstaller.Install, k3dInstaller.GetInstallHelp), toolPrerequisite("helm", helmInstaller.IsInstalled, helmInstaller.Install, helmInstaller.GetInstallHelp), diff --git a/internal/prerequisites/prerequisites.go b/internal/prerequisites/prerequisites.go index 550790d9..85ed96ca 100644 --- a/internal/prerequisites/prerequisites.go +++ b/internal/prerequisites/prerequisites.go @@ -28,6 +28,11 @@ type Prerequisite struct { // DocsURL points to manual setup instructions. Shown on Windows, when there // is no installer, or when an install attempt fails. DocsURL string + // Detail, when set, explains WHY the prerequisite is unsatisfied more + // precisely than the default "not installed" โ€” e.g. Docker returns + // "installed but not running" when the binary is present but the daemon is + // down. Optional; nil means the generic "not installed" wording is used. + Detail func() string } // Set is a named group of prerequisites, e.g. "cluster" or "app". @@ -40,7 +45,11 @@ type Set struct { type MissingItem struct { Name string DocsURL string - Err error // why it could not be auto-installed (nil on Windows / no installer) + // Reason, when set, is a specific explanation (from Prerequisite.Detail) of + // why the item is unsatisfied โ€” e.g. "installed but not running". Empty means + // the item is genuinely absent and the generic "not installed" wording fits. + Reason string + Err error // why it could not be auto-installed (nil on Windows / no installer) } // Result summarizes running a Set. diff --git a/internal/prerequisites/runner.go b/internal/prerequisites/runner.go index 7a60a7ad..ac8bea39 100644 --- a/internal/prerequisites/runner.go +++ b/internal/prerequisites/runner.go @@ -44,12 +44,23 @@ func (r Runner) Check(set Set) Result { if satisfied(item) { res.Satisfied = append(res.Satisfied, item.Name) } else { - res.Missing = append(res.Missing, MissingItem{Name: item.Name, DocsURL: item.DocsURL}) + res.Missing = append(res.Missing, missing(item, nil)) } } return res } +// missing builds a MissingItem for an unsatisfied prerequisite, capturing the +// Detail-supplied reason (e.g. "installed but not running") so the renderer can +// avoid the false "not installed" wording when the tool is present. +func missing(p Prerequisite, err error) MissingItem { + m := MissingItem{Name: p.Name, DocsURL: p.DocsURL, Err: err} + if p.Detail != nil { + m.Reason = p.Detail() + } + return m +} + // Run checks every prerequisite in the set and, on supported OSes, installs the // missing ones. It never returns an error itself โ€” inspect Result.OK() / // Result.Missing to decide whether to proceed. @@ -67,23 +78,19 @@ func (r Runner) Run(ctx context.Context, set Set) Result { // installer exists. if auto && item.Install != nil { if err := item.Install(ctx); err != nil { - res.Missing = append(res.Missing, MissingItem{Name: item.Name, DocsURL: item.DocsURL, Err: err}) + res.Missing = append(res.Missing, missing(item, err)) continue } if satisfied(item) { res.Installed = append(res.Installed, item.Name) continue } - res.Missing = append(res.Missing, MissingItem{ - Name: item.Name, - DocsURL: item.DocsURL, - Err: fmt.Errorf("%s was installed but is still not satisfied", item.Name), - }) + res.Missing = append(res.Missing, missing(item, fmt.Errorf("%s was installed but is still not satisfied", item.Name))) continue } // Windows, or no installer available: report as manual (docs link). - res.Missing = append(res.Missing, MissingItem{Name: item.Name, DocsURL: item.DocsURL}) + res.Missing = append(res.Missing, missing(item, nil)) } return res diff --git a/internal/prerequisites/runner_test.go b/internal/prerequisites/runner_test.go index 39ea3757..6589af74 100644 --- a/internal/prerequisites/runner_test.go +++ b/internal/prerequisites/runner_test.go @@ -93,3 +93,49 @@ func TestRun_MissingWithoutInstallerIsManual(t *testing.T) { require.Len(t, res.Missing, 1) assert.Equal(t, "https://docs/docker", res.Missing[0].DocsURL) } + +// TestCheck_DetailBecomesReason: a prerequisite that supplies a Detail (e.g. +// Docker "installed but not running") must surface it as MissingItem.Reason so +// the renderer can avoid the false "not installed" wording. A prereq without a +// Detail leaves Reason empty (genuine absence โ†’ generic wording). +func TestCheck_DetailBecomesReason(t *testing.T) { + notRunning := Prerequisite{ + Name: "Docker", + DocsURL: "https://docs/docker", + IsSatisfied: func() bool { return false }, + Detail: func() string { return "installed but not running" }, + } + absent := Prerequisite{ + Name: "k3d", + DocsURL: "https://docs/k3d", + IsSatisfied: func() bool { return false }, + } + + res := Runner{}.Check(Set{Items: []Prerequisite{notRunning, absent}}) + require.Len(t, res.Missing, 2) + + byName := map[string]MissingItem{} + for _, m := range res.Missing { + byName[m.Name] = m + } + assert.Equal(t, "installed but not running", byName["Docker"].Reason, + "Detail must flow into Reason so the tool isn't mislabeled 'not installed'") + assert.Empty(t, byName["k3d"].Reason, "a prereq with no Detail must leave Reason empty") +} + +// TestRun_DetailReasonSurvivesFailedInstall: on Linux, when an auto-install +// runs but the tool is still unsatisfied (Docker installed but daemon down), +// the Detail reason must still be attached โ€” this is the exact WSL-Alpine case +// where apk installs docker but no OpenRC starts the daemon. +func TestRun_DetailReasonSurvivesFailedInstall(t *testing.T) { + installedButDown := Prerequisite{ + Name: "Docker", + IsSatisfied: func() bool { return false }, // never becomes running + Install: func(context.Context) error { return nil }, + Detail: func() string { return "installed but not running" }, + } + // OS: "linux" forces the auto-install path (the WSL-Alpine case). + res := Runner{OS: "linux"}.Run(context.Background(), Set{Items: []Prerequisite{installedButDown}}) + require.Len(t, res.Missing, 1) + assert.Equal(t, "installed but not running", res.Missing[0].Reason) +} diff --git a/internal/shared/errors/errors_test.go b/internal/shared/errors/errors_test.go index ee7c9a84..1369920c 100644 --- a/internal/shared/errors/errors_test.go +++ b/internal/shared/errors/errors_test.go @@ -92,7 +92,6 @@ func TestValidationError_Error(t *testing.T) { } } - func TestNewErrorHandler(t *testing.T) { tests := []struct { name string @@ -155,7 +154,6 @@ func TestErrorHandler_HandleError_ValidationError_NoValue(t *testing.T) { }) } - // TestErrorHandler_CommandError_ShowsChildStderr is the M1.1 guard: a failed // external command must surface the CHILD'S reason to the user, not the // useless "exit status 1". Before this, the handler matched a CommandError @@ -350,7 +348,6 @@ func BenchmarkValidationError_Error(b *testing.B) { } } - func BenchmarkErrorHandler_HandleError(b *testing.B) { handler := NewErrorHandler(false) err := errors.New("test error") diff --git a/internal/shared/errors/friendly.go b/internal/shared/errors/friendly.go index 47e2f8b0..409fbaf5 100644 --- a/internal/shared/errors/friendly.go +++ b/internal/shared/errors/friendly.go @@ -20,6 +20,12 @@ func friendlyHint(err error) string { // often surfaces wrapped in a timeout-looking message). case containsAny(msg, "invalid ownership metadata", "cannot be imported into the current release", "missing key \"meta.helm.sh"): return "A resource (usually the ArgoCD CRDs) already exists without Helm ownership metadata. Recreate the cluster ('openframe cluster delete' + 'openframe cluster create'), or add the Helm ownership labels to that resource, then retry." + // A prior helm operation was interrupted and left the release wedged in a + // pending-* state. Matched BEFORE the generic timeout case: retrying hits the + // SAME pending release and fails identically, so the timeout hint ("wait and + // retry") is actively wrong here โ€” the fix is a rollback. + case containsAny(msg, "another operation (install/upgrade/rollback) is in progress", "pending-install", "pending-upgrade", "pending-rollback"): + return "A previous install/upgrade was interrupted and left the release in a pending state; retrying will not clear it. Roll it back with 'helm rollback -n ' (or 'helm uninstall' it), then run the command again. Tip: pending releases are hidden from plain 'helm list' โ€” use 'helm list -a'." case containsAny(msg, "connection refused", "was refused", "unable to connect to the server", "connection reset"): return "The cluster isn't reachable โ€” is it running? Try 'openframe cluster status'." case containsAny(msg, "no such host", "dns resolution", "name resolution"): diff --git a/internal/shared/errors/friendly_upstream_test.go b/internal/shared/errors/friendly_upstream_test.go index e855be7e..b0bb66ed 100644 --- a/internal/shared/errors/friendly_upstream_test.go +++ b/internal/shared/errors/friendly_upstream_test.go @@ -86,3 +86,25 @@ func TestFriendlyHint_NoHintForUnknownErrors(t *testing.T) { t.Errorf("expected no hint for an unrecognized error, got %q", got) } } + +// TestFriendlyHint_PendingReleaseSuggestsRollback (V4): a helm release left in +// pending-* by an interrupted operation must point at rollback, NOT the generic +// "wait and retry" timeout hint โ€” retrying hits the same wedged release. The +// pending case is ordered before the timeout case for exactly this reason. +func TestFriendlyHint_PendingReleaseSuggestsRollback(t *testing.T) { + cases := []string{ + `Error: UPGRADE FAILED: another operation (install/upgrade/rollback) is in progress`, + `Error: release app-of-apps failed, status: pending-upgrade`, + `cannot patch: release in pending-install state`, + `Error: release argo-cd failed, status: pending-rollback`, + } + for _, msg := range cases { + got := friendlyHint(fmt.Errorf("op failed: %s", msg)) + if !strings.Contains(got, "rollback") { + t.Errorf("pending-release must suggest rollback.\nmessage: %s\ngot: %q", msg, got) + } + if strings.Contains(got, "cluster may be slow or unreachable") { + t.Errorf("pending-release must NOT get the generic timeout hint (retry is wrong here).\nmessage: %s\ngot: %q", msg, got) + } + } +} diff --git a/internal/shared/ui/messages.go b/internal/shared/ui/messages.go index 29023b71..7c30e89e 100644 --- a/internal/shared/ui/messages.go +++ b/internal/shared/ui/messages.go @@ -32,8 +32,6 @@ func ShowNoResourcesMessage(resourceType, operation, createCommand, listCommand pterm.DefaultBasicText.Println() } - - // ShowOperationError displays a friendly error message with troubleshooting tips func ShowOperationError(operation, resourceName string, err error, troubleshootingTips []TroubleshootingTip) { pterm.Error.Printf("Operation '%s' failed for %s\n", operation, pterm.Cyan(resourceName)) diff --git a/internal/shared/ui/messages_test.go b/internal/shared/ui/messages_test.go index 31a188a7..61b6f5a9 100644 --- a/internal/shared/ui/messages_test.go +++ b/internal/shared/ui/messages_test.go @@ -29,5 +29,3 @@ func TestShowNoResourcesMessage(t *testing.T) { // Should not panic with empty strings ShowNoResourcesMessage("", "", "", "") } - - diff --git a/internal/shared/ui/spinner/heartbeat.go b/internal/shared/ui/spinner/heartbeat.go new file mode 100644 index 00000000..0db816eb --- /dev/null +++ b/internal/shared/ui/spinner/heartbeat.go @@ -0,0 +1,78 @@ +package spinner + +import ( + "fmt" + "io" + "os" + "sync" + "time" + + "github.com/flamingo-stack/openframe-cli/internal/shared/ui" +) + +// defaultHeartbeatInterval is how often a Heartbeat emits when no interval is +// given. Frequent enough that a CI log or a watching user sees liveness within +// a reasonable window; sparse enough not to flood a multi-minute wait. +const defaultHeartbeatInterval = 30 * time.Second + +// Heartbeat periodically emits a one-line progress message while a long, +// output-less blocking operation runs. It is the NON-INTERACTIVE counterpart to +// the animated Spinner: the spinner needs a TTY to show anything, so in CI or a +// piped/`--non-interactive` session a multi-minute `helm --wait` printed +// nothing between "Installing..." and the final result โ€” and users, assuming a +// hang, killed the process before the diagnostics ever printed. +// +// It writes to stderr so stdout stays clean for machine-readable output, and it +// is silent under --silent (whose contract is "errors only"). +type Heartbeat struct { + stop chan struct{} + done chan struct{} + once sync.Once +} + +// StartHeartbeat begins emitting "