diff --git a/cmd/cluster/create.go b/cmd/cluster/create.go index 8aa15dff..227ca9b9 100644 --- a/cmd/cluster/create.go +++ b/cmd/cluster/create.go @@ -6,6 +6,7 @@ import ( "os" osexec "os/exec" "strings" + "time" "github.com/flamingo-stack/openframe-cli/internal/cluster/discovery" "github.com/flamingo-stack/openframe-cli/internal/cluster/models" @@ -134,14 +135,21 @@ func cloudPlanPreview(ctx context.Context, config models.ClusterConfig) error { var ( infracostAvailableFn = terraform.InfracostAvailable infracostOfferFn = offerInfracostInstall - infracostLoginFn = offerInfracostLogin + infracostLoginFn = func() bool { return offerInfracostLogin(context.Background()) } ) +// infracostLoginTimeout bounds the `infracost auth login` browser flow so a +// hung browser/network step cannot hang the CLI forever. +const infracostLoginTimeout = 5 * time.Minute + // offerInfracostLogin runs the one-time `infracost auth login` (browser flow) // right inside the CLI, so the user never needs a separate console. Attached // straight to the terminal — the flow prints a URL and reads stdin, which the -// capturing executor would swallow. Returns whether a login was performed. -func offerInfracostLogin() bool { +// capturing executor would swallow. Bounded by a timeout derived from ctx so +// a hung browser flow cannot hang the CLI indefinitely and remains +// cancellable via the caller's own cancellation plumbing. Returns whether a +// login was performed. +func offerInfracostLogin(ctx context.Context) bool { if sharedUI.IsNonInteractive() { return false } @@ -150,11 +158,17 @@ func offerInfracostLogin() bool { if err != nil || !confirmed { return false } - login := osexec.Command("infracost", "auth", "login") + loginCtx, cancel := context.WithTimeout(ctx, infracostLoginTimeout) + defer cancel() + login := osexec.CommandContext(loginCtx, "infracost", "auth", "login") login.Stdin = os.Stdin login.Stdout = os.Stdout login.Stderr = os.Stderr if err := login.Run(); err != nil { + if loginCtx.Err() != nil { + pterm.Warning.Printf("infracost auth login timed out or was cancelled: %v\n", loginCtx.Err()) + return false + } pterm.Warning.Printf("infracost auth login failed: %v\n", err) return false } @@ -380,3 +394,7 @@ func validateGKEProjectFlag(ctx context.Context, exec executor.CommandExecutor, } return fmt.Errorf("GCP project %q is not among your accessible projects: %s", project, strings.Join(projects, ", ")) } +FILE>>> + +<< maxConditionInError { - cond = cond[:maxConditionInError] + "..." + cond = strings.ToValidUTF8(cond[:maxConditionInError], "") + "..." } fmt.Fprintf(&b, " - %s: %s\n", app.Name, cond) } diff --git a/internal/chart/providers/argocd/sync.go b/internal/chart/providers/argocd/sync.go index d9969eb0..4df8316d 100644 --- a/internal/chart/providers/argocd/sync.go +++ b/internal/chart/providers/argocd/sync.go @@ -3,6 +3,7 @@ package argocd import ( "context" "fmt" + "os" "sort" "strconv" "strings" @@ -133,6 +134,14 @@ const trackingInstanceLabel = "app.kubernetes.io/instance" // e.g. "argocd-apps:argoproj.io/Application:argocd/openframe-api". const trackingIDAnnotation = "argocd.argoproj.io/tracking-id" +// syncUnrelatedAppsEnvVar is the explicit opt-in required before +// syncChildApplications is allowed to fall back to syncing every Application in +// the namespace when neither tracking marker is present. Unset (or any value +// other than "true") means the fallback is refused rather than silently +// force-syncing Applications that may not belong to OpenFrame at all — see the +// fallback's warning for the risk this guards against. +const syncUnrelatedAppsEnvVar = "OPENFRAME_ALLOW_SYNC_UNRELATED_APPS" + // 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 @@ -155,10 +164,12 @@ func trackingOwner(labels, annotations map[string]string) string { // 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. +// Only when NEITHER marker is present on any Application does it consider +// falling back to every Application except the root — but on a shared ArgoCD +// instance that may touch Applications that are not OpenFrame-owned at all, so +// the fallback is refused unless explicitly opted into via +// OPENFRAME_ALLOW_SYNC_UNRELATED_APPS=true; otherwise it errors instead of +// silently force-syncing unrelated Applications. // // Children carrying the SyncGroupLabel are synced group-by-group (lowest // first), each group gated on the previous one converging to Healthy+Synced @@ -188,14 +199,22 @@ func (m *Manager) syncChildApplications(ctx context.Context, prune bool) error { } } if children == nil { + var untracked []unstructured.Unstructured for i := range list.Items { if list.Items[i].GetName() != AppOfAppsName { - children = append(children, list.Items[i]) + untracked = append(untracked, list.Items[i]) } } - if len(children) > 0 { - pterm.Warning.Printf("No applications carry the %s=%s tracking label; syncing all %d applications in %q\n", - trackingInstanceLabel, AppOfAppsName, len(children), ArgoCDNamespace) + if len(untracked) > 0 { + if os.Getenv(syncUnrelatedAppsEnvVar) != "true" { + return fmt.Errorf("no applications carry the %s=%s tracking label or %s tracking-id in namespace %q; "+ + "refusing to sync %d application(s) that cannot be confirmed as OpenFrame-owned "+ + "(set %s=true to opt in)", + trackingInstanceLabel, AppOfAppsName, trackingIDAnnotation, ArgoCDNamespace, len(untracked), syncUnrelatedAppsEnvVar) + } + pterm.Warning.Printf("No applications carry the %s=%s tracking label; syncing all %d applications in %q (opted in via %s)\n", + trackingInstanceLabel, AppOfAppsName, len(untracked), ArgoCDNamespace, syncUnrelatedAppsEnvVar) + children = untracked } } diff --git a/internal/chart/providers/git/auth.go b/internal/chart/providers/git/auth.go index 0bd78390..d59611c6 100644 --- a/internal/chart/providers/git/auth.go +++ b/internal/chart/providers/git/auth.go @@ -39,15 +39,30 @@ func extractGitAuth(rawURL string) gitAuth { // A single-field userinfo (e.g. https://@host, a common GitHub PAT // shorthand) carries the token as the username with no password. Treat it as // the token so it is used for auth (and masked in output) rather than - // silently stripped from the URL and dropped. + // silently stripped from the URL and dropped. This is a heuristic: a + // legitimate plain username with no embedded secret (e.g. + // https://someuser@host with credentials supplied out-of-band) will also be + // reinterpreted as a token here. We only apply the heuristic when the + // userinfo looks like a plausible token (long enough / not a simple word), + // to reduce the chance of masking a real, non-secret username in output. if !hasPassword { - token = username - username = "" + if looksLikeToken(username) { + token = username + username = "" + } } u.User = nil return gitAuth{cleanURL: u.String(), username: username, token: token} } +// looksLikeToken is a heuristic guard used by extractGitAuth to decide whether +// a bare (password-less) userinfo value should be treated as a secret token +// rather than a plain, non-secret username. Short, simple values are left as +// usernames so they are not needlessly masked in log output. +func looksLikeToken(s string) bool { + return len(s) >= 20 +} + // buildAuth returns the in-memory HTTP auth method for a private repository, or // nil for a public one. The token lives only in memory — never in the URL, // argv, or a credentials file. GitHub PAT auth expects the token as the diff --git a/internal/chart/providers/helm/argocd_wait.go b/internal/chart/providers/helm/argocd_wait.go index 3580cee3..9fdf83d9 100644 --- a/internal/chart/providers/helm/argocd_wait.go +++ b/internal/chart/providers/helm/argocd_wait.go @@ -64,7 +64,7 @@ func (h *HelmManager) waitForArgoCDDeployments(ctx context.Context, verbose bool // Check Deployments for _, name := range expectedDeployments { - _, err := h.kubeClient.AppsV1().Deployments("argocd").Get(ctx, name, metav1.GetOptions{}) + _, err := h.kubeClient.AppsV1().Deployments(argocd.ArgoCDNamespace).Get(ctx, name, metav1.GetOptions{}) if k8serrors.IsNotFound(err) { missingWorkloads = append(missingWorkloads, "deployment/"+name) @@ -77,7 +77,7 @@ func (h *HelmManager) waitForArgoCDDeployments(ctx context.Context, verbose bool // Check StatefulSets (application-controller in ArgoCD v3.x) for _, name := range expectedStatefulSets { - _, err := h.kubeClient.AppsV1().StatefulSets("argocd").Get(ctx, name, metav1.GetOptions{}) + _, err := h.kubeClient.AppsV1().StatefulSets(argocd.ArgoCDNamespace).Get(ctx, name, metav1.GetOptions{}) if k8serrors.IsNotFound(err) { missingWorkloads = append(missingWorkloads, "statefulset/"+name) @@ -345,3 +345,4 @@ func (h *HelmManager) verifyClusterConnectivity(ctx context.Context, config conf } return fmt.Errorf("cluster not reachable after retries: %w", lastErr) } + diff --git a/internal/chart/providers/helm/manager_test.go b/internal/chart/providers/helm/manager_test.go index d3421993..9975c605 100644 --- a/internal/chart/providers/helm/manager_test.go +++ b/internal/chart/providers/helm/manager_test.go @@ -2,14 +2,12 @@ package helm import ( "context" - "strings" "testing" "github.com/flamingo-stack/openframe-cli/internal/chart/utils/errors" "github.com/flamingo-stack/openframe-cli/internal/shared/executor" "github.com/stretchr/testify/assert" k8sfake "k8s.io/client-go/kubernetes/fake" - "k8s.io/client-go/rest" ) // createTestHelmManager creates a HelmManager for testing with a fake clientset @@ -23,89 +21,26 @@ func createTestHelmManager(exec executor.CommandExecutor) *HelmManager { } } -// testRestConfig returns a dummy rest.Config for use in tests -// This is not used in actual tests since createTestHelmManager creates the struct directly -var _ = &rest.Config{} // Used to ensure the import is not removed - -// MockExecutor implements CommandExecutor for testing -type MockExecutor struct { - commands [][]string - results map[string]*executor.CommandResult - errors map[string]error -} - -func NewMockExecutor() *MockExecutor { - return &MockExecutor{ - commands: make([][]string, 0), - results: make(map[string]*executor.CommandResult), - errors: make(map[string]error), - } -} - -func (m *MockExecutor) Execute(ctx context.Context, name string, args ...string) (*executor.CommandResult, error) { - command := append([]string{name}, args...) - m.commands = append(m.commands, command) - - commandStr := name - for _, arg := range args { - commandStr += " " + arg - } - - // Check for partial match for error handling (for complex commands) - for errKey, err := range m.errors { - if strings.Contains(commandStr, errKey) { - return nil, err - } - } - - if result, exists := m.results[commandStr]; exists { - return result, nil - } - - // Default success result - return &executor.CommandResult{ - ExitCode: 0, - Stdout: "", - Stderr: "", - }, nil -} - -func (m *MockExecutor) ExecuteWithOptions(ctx context.Context, options executor.ExecuteOptions) (*executor.CommandResult, error) { - return m.Execute(ctx, options.Command, options.Args...) -} - -func (m *MockExecutor) SetResult(command string, result *executor.CommandResult) { - m.results[command] = result -} - -func (m *MockExecutor) SetError(command string, err error) { - m.errors[command] = err -} - -func (m *MockExecutor) GetCommands() [][]string { - return m.commands -} - func TestHelmManager_IsHelmInstalled(t *testing.T) { tests := []struct { name string - setupMock func(*MockExecutor) + setupMock func(*executor.MockCommandExecutor) expectError bool }{ { name: "helm is installed", - setupMock: func(m *MockExecutor) { - m.SetResult("helm version --short", &executor.CommandResult{ + setupMock: func(m *executor.MockCommandExecutor) { + m.SetResponse("helm version --short", &executor.CommandResult{ ExitCode: 0, Stdout: "v3.12.0+g4f11b4a", - }) + }, nil) }, expectError: false, }, { name: "helm is not installed", - setupMock: func(m *MockExecutor) { - m.SetError("helm version --short", assert.AnError) + setupMock: func(m *executor.MockCommandExecutor) { + m.SetResponse("helm version --short", nil, assert.AnError) }, expectError: true, }, @@ -113,7 +48,7 @@ func TestHelmManager_IsHelmInstalled(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - mockExec := NewMockExecutor() + mockExec := executor.NewMockCommandExecutor() tt.setupMock(mockExec) manager := createTestHelmManager(mockExec) @@ -134,7 +69,7 @@ func TestHelmManager_IsChartInstalled(t *testing.T) { name string releaseName string namespace string - setupMock func(*MockExecutor) + setupMock func(*executor.MockCommandExecutor) expectResult bool expectError bool }{ @@ -142,11 +77,11 @@ func TestHelmManager_IsChartInstalled(t *testing.T) { name: "chart is installed", releaseName: "argocd", namespace: "argocd", - setupMock: func(m *MockExecutor) { - m.SetResult("helm list -q -n argocd -f argocd", &executor.CommandResult{ + setupMock: func(m *executor.MockCommandExecutor) { + m.SetResponse("helm list -q -n argocd -f argocd", &executor.CommandResult{ ExitCode: 0, Stdout: "argocd\n", - }) + }, nil) }, expectResult: true, expectError: false, @@ -155,11 +90,11 @@ func TestHelmManager_IsChartInstalled(t *testing.T) { name: "chart is not installed", releaseName: "argocd", namespace: "argocd", - setupMock: func(m *MockExecutor) { - m.SetResult("helm list -q -n argocd -f argocd", &executor.CommandResult{ + setupMock: func(m *executor.MockCommandExecutor) { + m.SetResponse("helm list -q -n argocd -f argocd", &executor.CommandResult{ ExitCode: 0, Stdout: "", - }) + }, nil) }, expectResult: false, expectError: false, @@ -168,8 +103,8 @@ func TestHelmManager_IsChartInstalled(t *testing.T) { name: "helm command fails", releaseName: "argocd", namespace: "argocd", - setupMock: func(m *MockExecutor) { - m.SetError("helm list -q -n argocd -f argocd", assert.AnError) + setupMock: func(m *executor.MockCommandExecutor) { + m.SetResponse("helm list -q -n argocd -f argocd", nil, assert.AnError) }, expectResult: false, expectError: true, @@ -178,7 +113,7 @@ func TestHelmManager_IsChartInstalled(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - mockExec := NewMockExecutor() + mockExec := executor.NewMockCommandExecutor() tt.setupMock(mockExec) manager := createTestHelmManager(mockExec) @@ -205,7 +140,7 @@ func TestHelmManager_GetChartStatus(t *testing.T) { name string releaseName string namespace string - setupMock func(*MockExecutor) + setupMock func(*executor.MockCommandExecutor) expectError bool wantStatus string wantVersion string @@ -215,11 +150,11 @@ func TestHelmManager_GetChartStatus(t *testing.T) { name: "successful status retrieval", releaseName: "argocd", namespace: "argocd", - setupMock: func(m *MockExecutor) { - m.SetResult(metadataCmd, &executor.CommandResult{ + setupMock: func(m *executor.MockCommandExecutor) { + m.SetResponse(metadataCmd, &executor.CommandResult{ ExitCode: 0, Stdout: `{"name":"argocd","namespace":"argocd","status":"deployed","version":"7.7.5","appVersion":"v2.13.0","revision":3}`, - }) + }, nil) }, wantStatus: "deployed", wantVersion: "7.7.5", @@ -232,11 +167,11 @@ func TestHelmManager_GetChartStatus(t *testing.T) { name: "a failed release is reported as failed", releaseName: "argocd", namespace: "argocd", - setupMock: func(m *MockExecutor) { - m.SetResult(metadataCmd, &executor.CommandResult{ + setupMock: func(m *executor.MockCommandExecutor) { + m.SetResponse(metadataCmd, &executor.CommandResult{ ExitCode: 0, Stdout: `{"name":"argocd","namespace":"argocd","status":"failed","version":"7.7.5","appVersion":"v2.13.0"}`, - }) + }, nil) }, wantStatus: "failed", wantVersion: "7.7.5", @@ -246,8 +181,8 @@ func TestHelmManager_GetChartStatus(t *testing.T) { name: "status command fails", releaseName: "argocd", namespace: "argocd", - setupMock: func(m *MockExecutor) { - m.SetError(metadataCmd, assert.AnError) + setupMock: func(m *executor.MockCommandExecutor) { + m.SetResponse(metadataCmd, nil, assert.AnError) }, expectError: true, }, @@ -255,8 +190,8 @@ func TestHelmManager_GetChartStatus(t *testing.T) { name: "unparseable output is an error, not a fabricated status", releaseName: "argocd", namespace: "argocd", - setupMock: func(m *MockExecutor) { - m.SetResult(metadataCmd, &executor.CommandResult{ExitCode: 0, Stdout: `not json`}) + setupMock: func(m *executor.MockCommandExecutor) { + m.SetResponse(metadataCmd, &executor.CommandResult{ExitCode: 0, Stdout: `not json`}, nil) }, expectError: true, }, @@ -264,7 +199,7 @@ func TestHelmManager_GetChartStatus(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - mockExec := NewMockExecutor() + mockExec := executor.NewMockCommandExecutor() tt.setupMock(mockExec) manager := createTestHelmManager(mockExec) diff --git a/internal/chart/providers/helm/path_windows.go b/internal/chart/providers/helm/path_windows.go index a8a57990..741af84e 100644 --- a/internal/chart/providers/helm/path_windows.go +++ b/internal/chart/providers/helm/path_windows.go @@ -27,7 +27,7 @@ func expandShortPath(path string) (string, error) { } // First call to get required buffer size - n, _, _ := procGetLongPathNameW.Call( + n, _, callErr := procGetLongPathNameW.Call( uintptr(unsafe.Pointer(pathPtr)), 0, 0, @@ -35,21 +35,21 @@ func expandShortPath(path string) (string, error) { if n == 0 { // GetLongPathNameW failed - path might not exist or other error - // Return original path as fallback - return path, nil + // Return original path as fallback, but surface the error for diagnostics + return path, callErr } // Allocate buffer and get the long path buf := make([]uint16, n) - n, _, _ = procGetLongPathNameW.Call( + n, _, callErr = procGetLongPathNameW.Call( uintptr(unsafe.Pointer(pathPtr)), uintptr(unsafe.Pointer(&buf[0])), uintptr(n), ) if n == 0 { - // Failed to get long path, return original - return path, nil + // Failed to get long path, return original path but surface the error + return path, callErr } return syscall.UTF16ToString(buf[:n]), nil diff --git a/internal/chart/utils/config/service.go b/internal/chart/utils/config/service.go index 979119bc..f04c047e 100644 --- a/internal/chart/utils/config/service.go +++ b/internal/chart/utils/config/service.go @@ -1,6 +1,7 @@ package config import ( + "fmt" "os" "github.com/flamingo-stack/openframe-cli/internal/chart/models" @@ -35,14 +36,14 @@ func (s *Service) GetPathResolver() *PathResolver { func (s *Service) Initialize() error { // Initialize shared system service if err := s.systemService.Initialize(); err != nil { - return err + return fmt.Errorf("failed to initialize system service: %w", err) } // Ensure certificate directory exists certDir := s.GetCertificateDirectory() if _, err := os.Stat(certDir); os.IsNotExist(err) { if err := os.MkdirAll(certDir, 0750); err != nil { - return err + return fmt.Errorf("failed to create certificate directory %q: %w", certDir, err) } } diff --git a/internal/cluster/discovery/eks.go b/internal/cluster/discovery/eks.go index 9bcf4fa3..25816db9 100644 --- a/internal/cluster/discovery/eks.go +++ b/internal/cluster/discovery/eks.go @@ -176,10 +176,12 @@ func (d *EKSDiscoverer) Discover(ctx context.Context) (Result, error) { res.Warnings = append(res.Warnings, fmt.Sprintf("%s: %v", label, err)) continue } - if arn != "" && seen[arn] { - continue // same cluster through another profile + if arn != "" { + if seen[arn] { + continue // same cluster through another profile + } + seen[arn] = true } - seen[arn] = true info.Context = matchEKSContext(contexts, arn, name) res.Clusters = append(res.Clusters, info) } diff --git a/internal/cluster/prerequisites/aws/aws.go b/internal/cluster/prerequisites/aws/aws.go index 24ab9e74..af3dcdcf 100644 --- a/internal/cluster/prerequisites/aws/aws.go +++ b/internal/cluster/prerequisites/aws/aws.go @@ -95,6 +95,7 @@ func (a *AwsInstaller) installLinux() error { {"yum", []string{"yum", "install", "-y", "awscli2"}}, {"pacman", []string{"pacman", "-S", "--noconfirm", "aws-cli-v2"}}, } + var attemptErrs []string for _, m := range managers { if !commandExists(m.name) { continue @@ -108,8 +109,13 @@ func (a *AwsInstaller) installLinux() error { // Older repos (e.g. Ubuntu 22.04) ship legacy v1, whose // `aws eks get-token` emits an auth API kubectl no longer accepts. return fmt.Errorf("the distro package installed AWS CLI v1, but the EKS flow needs v2. %s", a.GetInstallHelp()) + } else { + attemptErrs = append(attemptErrs, fmt.Sprintf("%s: %v", m.name, err)) } } + if len(attemptErrs) > 0 { + return fmt.Errorf("could not install the AWS CLI automatically (%s). %s", strings.Join(attemptErrs, "; "), a.GetInstallHelp()) + } return fmt.Errorf("could not install the AWS CLI automatically. %s", a.GetInstallHelp()) } diff --git a/internal/cluster/prerequisites/infracost/infracost.go b/internal/cluster/prerequisites/infracost/infracost.go index 032b8b94..d62e27b1 100644 --- a/internal/cluster/prerequisites/infracost/infracost.go +++ b/internal/cluster/prerequisites/infracost/infracost.go @@ -73,7 +73,7 @@ func (i *Installer) Install() error { ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() - fmt.Printf("Downloading verified infracost %s...\n", download.Infracost.Version) + pterm.Info.Printf("Downloading verified infracost %s...\n", download.Infracost.Version) if err := (download.Downloader{}).InstallVerifiedTarGz(ctx, asset, member, dest, 0o750); err != nil { return fmt.Errorf("verified infracost install failed: %w", err) } diff --git a/internal/cluster/prerequisites/installer_test.go b/internal/cluster/prerequisites/installer_test.go index 4c51aea5..84dfc83b 100644 --- a/internal/cluster/prerequisites/installer_test.go +++ b/internal/cluster/prerequisites/installer_test.go @@ -2,6 +2,8 @@ package prerequisites import ( "testing" + + "github.com/shipyard-run/shipyard/internal/shared/testutil" ) func TestNewInstaller(t *testing.T) { @@ -35,7 +37,7 @@ func TestInstallTool(t *testing.T) { } for _, invalidError := range invalidErrors { - if containsSubstring(errorStr, invalidError) { + if testutil.ContainsSubstring(errorStr, invalidError) { t.Errorf("Tool %s returned unexpected error: %v", tool, err) } } @@ -54,19 +56,6 @@ func TestInstallTool(t *testing.T) { } } -// Helper function to check if a string contains a substring -func containsSubstring(str, substr string) bool { - return len(str) >= len(substr) && - func() bool { - for i := 0; i <= len(str)-len(substr); i++ { - if str[i:i+len(substr)] == substr { - return true - } - } - return false - }() -} - // TestContainsTool covers the case-insensitive membership check used to detect // a freshly installed Docker that still needs the start/wait phase (B3). func TestContainsTool(t *testing.T) { diff --git a/internal/cluster/prerequisites/k3d/k3d.go b/internal/cluster/prerequisites/k3d/k3d.go index a27d6953..019ac24f 100644 --- a/internal/cluster/prerequisites/k3d/k3d.go +++ b/internal/cluster/prerequisites/k3d/k3d.go @@ -140,7 +140,7 @@ func (k *K3dInstaller) installVerified() error { ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() - fmt.Printf("Downloading verified k3d %s...\n", download.K3d.Version) + pterm.Info.Printf("Downloading verified k3d %s...\n", download.K3d.Version) path, err := (download.Downloader{}).InstallPinnedTool(ctx, download.K3d, binDir) if err != nil { return fmt.Errorf("verified k3d install failed: %w", err) diff --git a/internal/cluster/prerequisites/terraform/terraform.go b/internal/cluster/prerequisites/terraform/terraform.go index 9a2b528b..5322b3de 100644 --- a/internal/cluster/prerequisites/terraform/terraform.go +++ b/internal/cluster/prerequisites/terraform/terraform.go @@ -105,7 +105,7 @@ func (t *TerraformInstaller) installVerified() error { ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() - fmt.Printf("Downloading verified terraform %s...\n", download.Terraform.Version) + pterm.Info.Printf("Downloading verified terraform %s...\n", download.Terraform.Version) path, err := (download.Downloader{}).InstallPinnedTool(ctx, download.Terraform, binDir) if err != nil { return fmt.Errorf("verified terraform install failed: %w", err) diff --git a/internal/cluster/providers/eks/resumehint.go b/internal/cluster/providers/eks/resumehint.go index 70f6072d..63c84784 100644 --- a/internal/cluster/providers/eks/resumehint.go +++ b/internal/cluster/providers/eks/resumehint.go @@ -1,21 +1,16 @@ package eks +import "yy.foundation.im/base/internal/shared/errors" + // resumeHintError carries a resume instruction that survives the generic // interruption handler. On Ctrl+C that handler prints only "Operation cancelled // by user." and discards err.Error(), so a hint wrapped only as message text is // lost. internal/shared/errors surfaces the hint via the ResumeHint() method // even for an interrupted operation. (The GKE twin: gke/resumehint.go.) -type resumeHintError struct { - err error - hint string -} - -func (e *resumeHintError) Error() string { return e.err.Error() } -func (e *resumeHintError) Unwrap() error { return e.err } -func (e *resumeHintError) ResumeHint() string { return e.hint } +type resumeHintError = errors.ResumeHintError // withResumeHint attaches hint to err structurally (not just in the message // text), so it survives the interruption handler that drops err.Error(). func withResumeHint(err error, hint string) error { - return &resumeHintError{err: err, hint: hint} + return errors.WithResumeHint(err, hint) } diff --git a/internal/cluster/providers/eks/teardown.go b/internal/cluster/providers/eks/teardown.go index c9cbdb6f..0553f0ab 100644 --- a/internal/cluster/providers/eks/teardown.go +++ b/internal/cluster/providers/eks/teardown.go @@ -197,16 +197,26 @@ func releaseWorkloadResources(ctx context.Context, rec tfengine.Record) { }) } +// volumeClusterARNTagKey is the tag the EBS CSI driver stamps with the full +// EKS cluster ARN (extraVolumeTags in templates/main.tf). Unlike the cluster +// name, the ARN embeds the account and is unique per cluster instance, so it +// disambiguates a same-named EKS cluster re-created (or recorded stale) in the +// same region — the counterpart of GKE's disksInLocation scoping. +const volumeClusterARNTagKey = "openframe:cluster-arn" + // sweepOrphanedVolumes finds EBS volumes still tagged for this cluster after // the destroy. Post-destroy they are unambiguous orphans of a cluster that no // longer exists (describe-volumes is region-scoped, so a same-named cluster in // another region is out of reach by construction; the status=available filter -// additionally refuses anything still attached). It deletes them when the -// operator consents — standing consent via --force, or an interactive yes to -// the prompt — so the cluster leaves zero billable leftovers; otherwise it -// reports them with the exact cleanup command and never deletes cloud data -// without consent. Best-effort throughout. (The GKE twin sweeps Persistent -// Disks by label.) +// additionally refuses anything still attached). To also guard against a +// same-named cluster reusing the same region (e.g. a rename or a stale +// record), matches are additionally filtered by the cluster ARN tag, when +// available, which is unique per cluster instance — the counterpart of GKE's +// disksInLocation scoping. It deletes them when the operator consents — +// standing consent via --force, or an interactive yes to the prompt — so the +// cluster leaves zero billable leftovers; otherwise it reports them with the +// exact cleanup command and never deletes cloud data without consent. +// Best-effort throughout. (The GKE twin sweeps Persistent Disks by label.) func (p *Provider) sweepOrphanedVolumes(ctx context.Context, rec tfengine.Record, force bool) { if rec.Region == "" { return @@ -216,7 +226,7 @@ func (p *Provider) sweepOrphanedVolumes(ctx context.Context, rec tfengine.Record "--filters", fmt.Sprintf("Name=tag:%s,Values=%s", orphanVolumeTagKey, rec.Name), "Name=status,Values=available", - "--query", "Volumes[].VolumeId", "--output", "text"} + "--query", "Volumes[].{Id:VolumeId,ARN:Tags[?Key=='" + volumeClusterARNTagKey + "']|[0].Value}", "--output", "json"} if rec.Profile != "" { args = append(args, "--profile", rec.Profile) } @@ -224,7 +234,7 @@ func (p *Provider) sweepOrphanedVolumes(ctx context.Context, rec tfengine.Record if err != nil || res == nil { return } - volumes := parseVolumeIDs(res.Stdout) + volumes := parseVolumesInCluster(res.Stdout, rec.ClusterARN) if len(volumes) == 0 { return } @@ -299,3 +309,43 @@ func parseVolumeIDs(out string) []string { } return ids } + +// volumeEntry mirrors one element of the Volumes[].{Id,ARN} JSON projection +// produced by describe-volumes' --query in sweepOrphanedVolumes. +type volumeEntry struct { + Id string `json:"Id"` + ARN string `json:"ARN"` +} + +// parseVolumesInCluster parses the JSON volume/ARN pairs from describe-volumes +// and returns only the volume ids that belong to wantARN. This is the EKS +// counterpart of GKE's disksInLocation: the tag-based name filter alone can +// match a same-named cluster re-created (or recorded stale) in the same +// region, so any volume whose ARN tag is present must match the just-destroyed +// cluster's ARN to be considered an orphan of it. When wantARN is empty (ARN +// unavailable on the record) or a volume has no ARN tag (older clusters +// provisioned before the tag existed), the volume is kept — preserving prior +// behavior rather than silently hiding orphans this scoping cannot confirm. +func parseVolumesInCluster(out string, wantARN string) []string { + out = strings.TrimSpace(out) + if out == "" || out == "None" { + return nil + } + var entries []volumeEntry + if err := jsonUnmarshal([]byte(out), &entries); err != nil { + // Fall back to treating the output as the plain volume-id text format, + // preserving prior behavior if --query/--output ever mismatch. + return parseVolumeIDs(out) + } + var ids []string + for _, e := range entries { + if e.Id == "" { + continue + } + if wantARN != "" && e.ARN != "" && e.ARN != wantARN { + continue + } + ids = append(ids, e.Id) + } + return ids +} diff --git a/internal/cluster/providers/gke/teardown.go b/internal/cluster/providers/gke/teardown.go index b3cd8411..21611d6a 100644 --- a/internal/cluster/providers/gke/teardown.go +++ b/internal/cluster/providers/gke/teardown.go @@ -6,6 +6,7 @@ import ( "strings" "time" + "github.com/flamingo-stack/openframe-cli/internal/cluster/providers/common" tfengine "github.com/flamingo-stack/openframe-cli/internal/cluster/providers/terraform" sharedUI "github.com/flamingo-stack/openframe-cli/internal/shared/ui" "github.com/pterm/pterm" @@ -16,18 +17,11 @@ import ( "k8s.io/client-go/kubernetes" ) -// systemNamespaces and systemNamespacePrefixes are never deleted during -// teardown. They are the cluster's own control-plane/system namespaces (torn -// down with the cluster anyway) and — critically — kube-system hosts the GKE PD -// CSI controller that must keep running to delete the Persistent Disks as their -// PVCs go away. -var systemNamespaces = map[string]struct{}{ - "default": {}, - "kube-system": {}, - "kube-public": {}, - "kube-node-lease": {}, -} - +// systemNamespacePrefixes are never deleted during teardown, together with the +// common system namespaces. They are the cluster's own control-plane/system +// namespaces (torn down with the cluster anyway) and — critically — kube-system +// hosts the GKE PD CSI controller that must keep running to delete the +// Persistent Disks as their PVCs go away. var systemNamespacePrefixes = []string{"kube-", "gke-", "gmp-"} const ( @@ -44,15 +38,7 @@ const ( // isSystemNamespace reports whether ns is a cluster/system namespace that // teardown must never delete. func isSystemNamespace(ns string) bool { - if _, ok := systemNamespaces[ns]; ok { - return true - } - for _, p := range systemNamespacePrefixes { - if strings.HasPrefix(ns, p) { - return true - } - } - return false + return common.IsSystemNamespace(ns, systemNamespacePrefixes) } // appNamespacesToDelete returns the application namespaces (everything that is @@ -64,19 +50,7 @@ func isSystemNamespace(ns string) bool { // deleted, otherwise self-heal could recreate a StatefulSet (and its PVC) // mid-teardown. func appNamespacesToDelete(all []string) []string { - var argocd []string - var rest []string - for _, ns := range all { - if isSystemNamespace(ns) { - continue - } - if ns == "argocd" { - argocd = append(argocd, ns) - } else { - rest = append(rest, ns) - } - } - return append(argocd, rest...) + return common.AppNamespacesToDelete(all, systemNamespacePrefixes) } // countDeletablePVs counts PersistentVolumes whose reclaim policy is Delete. @@ -85,13 +59,7 @@ func appNamespacesToDelete(all []string) []string { // Retain-policy PVs are excluded on purpose — their disks are meant to survive, // and the post-destroy sweep reports (never silently drops) them. func countDeletablePVs(pvs []corev1.PersistentVolume) int { - var n int - for _, pv := range pvs { - if pv.Spec.PersistentVolumeReclaimPolicy == corev1.PersistentVolumeReclaimDelete { - n++ - } - } - return n + return common.CountDeletablePVs(pvs) } // releaseWorkloadDisks deletes every application namespace on the cluster and diff --git a/internal/cluster/service_test.go b/internal/cluster/service_test.go index 21efb3fe..e0ec2a47 100644 --- a/internal/cluster/service_test.go +++ b/internal/cluster/service_test.go @@ -45,8 +45,14 @@ func TestNewClusterService(t *testing.T) { } func TestClusterService_CreateCluster(t *testing.T) { - exec := createTestExecutor() - service := NewClusterService(exec) + mock := executor.NewMockCommandExecutor() + mockJSON := `[{"name":"test-cluster","serversCount":1,"serversRunning":1,"agentsCount":0,"agentsRunning":0,"nodes":[{"name":"k3d-test-cluster-server-0","role":"server","created":"2024-01-01T00:00:00Z"}]}]` + mock.SetResponse("k3d cluster list", &executor.CommandResult{ + ExitCode: 0, + Stdout: mockJSON, + Duration: 100, + }) + service := NewClusterService(mock) // Use a unique cluster name to avoid conflicts with existing clusters config := models.ClusterConfig{ @@ -57,9 +63,12 @@ func TestClusterService_CreateCluster(t *testing.T) { } _, err := service.CreateCluster(context.Background(), config) - // With mock executor, error can occur if cluster already exists or kubeconfig issues - // We just verify it doesn't panic - _ = err + if err != nil { + t.Fatalf("CreateCluster should succeed with mock executor, got error: %v", err) + } + if mock.GetCommandCount() == 0 { + t.Errorf("expected CreateCluster to execute at least one command, got: %v", mock.GetExecutedCommands()) + } } func TestClusterService_CreateCluster_CloudWithoutRegionFailsBeforeAnyCommand(t *testing.T) { diff --git a/internal/cluster/ui/wizard_test.go b/internal/cluster/ui/wizard_test.go index c2065f7f..77dc3cb8 100644 --- a/internal/cluster/ui/wizard_test.go +++ b/internal/cluster/ui/wizard_test.go @@ -1,6 +1,7 @@ package ui import ( + "strconv" "strings" "testing" @@ -110,19 +111,16 @@ func TestWizardValidation(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - // Simulate the validation function from the wizard + // This mirrors the actual validation logic in promptNodeCount: + // parse the input as an integer and check it falls within [1, 10]. validate := func(input string) error { - // This mimics the validation logic in promptNodeCount - if input == "abc" || input == "3.5" { + n, err := strconv.Atoi(input) + if err != nil { return assert.AnError } - if input == "0" || input == "-1" || input == "11" { + if n < 1 || n > 10 { return assert.AnError } - // For valid numeric inputs, parse and validate range - if input == "1" || input == "3" || input == "10" { - return nil - } return nil } diff --git a/internal/shared/errors/errors_test.go b/internal/shared/errors/errors_test.go index 60aa6416..d41e2eb0 100644 --- a/internal/shared/errors/errors_test.go +++ b/internal/shared/errors/errors_test.go @@ -374,17 +374,12 @@ func TestErrorHandler_NilHandling(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // Even a nil handler should not panic - this tests defensive programming - if tt.handler == nil { - // In this case we're testing that a nil handler would be handled gracefully - // In practice, the caller should ensure handler is not nil - assert.NotPanics(t, func() { - // Simulate defensive handling if needed - if tt.handler != nil { - tt.handler.HandleError(errors.New("test")) - } - }) - } + // A nil *ErrorHandler must not panic when HandleError is invoked on it, + // since HandleError only reads fields (e.g. h.verbose) and does not + // dereference the receiver unconditionally. + assert.NotPanics(t, func() { + tt.handler.HandleError(errors.New("test")) + }) }) } } diff --git a/internal/shared/files/cleanup.go b/internal/shared/files/cleanup.go index 73c0ff55..e2e477d1 100644 --- a/internal/shared/files/cleanup.go +++ b/internal/shared/files/cleanup.go @@ -56,14 +56,14 @@ func (fc *FileCleanup) restoreFilesForced(verbose bool, success bool) error { restoredCount := 0 var restoreErrs []error - restoreFailed := make(map[string]bool) - for _, backup := range fc.backups { + restoreFailed := make(map[int]bool) + for i, backup := range fc.backups { if err := fc.restoreFile(backup, verbose); err != nil { if verbose { pterm.Warning.Printf("Failed to restore %s: %v\n", backup.OriginalPath, err) } restoreErrs = append(restoreErrs, fmt.Errorf("restoring %s: %w", backup.OriginalPath, err)) - restoreFailed[backup.OriginalPath] = true + restoreFailed[i] = true continue } restoredCount++ @@ -97,8 +97,8 @@ func (fc *FileCleanup) RestoreFilesWithResult(verbose bool, success bool) error restoredCount := 0 var restoreErrs []error - restoreFailed := make(map[string]bool) - for _, backup := range fc.backups { + restoreFailed := make(map[int]bool) + for i, backup := range fc.backups { // For temporary files registered for success-only cleanup if fc.cleanupOnSuccess && !backup.FileExisted && !success { if verbose { @@ -112,7 +112,7 @@ func (fc *FileCleanup) RestoreFilesWithResult(verbose bool, success bool) error pterm.Warning.Printf("Failed to restore %s: %v\n", backup.OriginalPath, err) } restoreErrs = append(restoreErrs, fmt.Errorf("restoring %s: %w", backup.OriginalPath, err)) - restoreFailed[backup.OriginalPath] = true + restoreFailed[i] = true continue } restoredCount++ @@ -166,11 +166,13 @@ func (fc *FileCleanup) restoreFile(backup FileBackup, verbose bool) error { } // cleanupBackupFiles removes physical backup files. restoreFailed (keyed by -// OriginalPath) lists backups whose restore did NOT succeed: deleting those -// would destroy the only remaining copy of the original file. -func (fc *FileCleanup) cleanupBackupFiles(verbose bool, restoreFailed map[string]bool) { - for _, backup := range fc.backups { - if restoreFailed[backup.OriginalPath] { +// the index of the backup in fc.backups) lists backups whose restore did NOT +// succeed: deleting those would destroy the only remaining copy of the +// original file. Keying by index (rather than OriginalPath) avoids ambiguity +// when multiple FileBackup entries share the same OriginalPath. +func (fc *FileCleanup) cleanupBackupFiles(verbose bool, restoreFailed map[int]bool) { + for i, backup := range fc.backups { + if restoreFailed[i] { continue } if !backup.ContentOnly && backup.BackupPath != "" { diff --git a/internal/shared/selfupdate/update.go b/internal/shared/selfupdate/update.go index 2edada61..3a0ef306 100644 --- a/internal/shared/selfupdate/update.go +++ b/internal/shared/selfupdate/update.go @@ -96,7 +96,7 @@ type Updater struct { // through the progress callback: callers wire that to a spinner's // UpdateText, so the next step overwrites the line within one frame — the // "signature verification skipped" and "no rollback point" warnings were - // effectively invisible. nil → stderr. + // effectively invisible. nil → stderr via pterm. Warn func(string) } @@ -122,7 +122,7 @@ func (u Updater) warn(format string, args ...any) { u.Warn(msg) return } - fmt.Fprintln(os.Stderr, "WARNING: "+msg) + pterm.Warning.Println(msg) } // Check queries a release and compares it to the running version. When tag is @@ -237,7 +237,10 @@ func swapExecutable(ctx context.Context, exePath, newPath string, log func(strin return "", fmt.Errorf("backing up the current binary: %w", err) } if err := os.Rename(newPath, exePath); err != nil { - _ = os.Rename(backup, exePath) // roll back + if restoreErr := os.Rename(backup, exePath); restoreErr != nil { + return "", fmt.Errorf("installing the new binary failed (%w), AND restoring the original binary from backup also failed (%v); "+ + "the executable at %s may be missing — the previous binary can still be recovered from %s", err, restoreErr, exePath, backup) + } return "", fmt.Errorf("installing the new binary (rolled back): %w", err) } return backup, nil diff --git a/internal/shared/ui/status_theme.go b/internal/shared/ui/status_theme.go index e30ce9fe..884c8b91 100644 --- a/internal/shared/ui/status_theme.go +++ b/internal/shared/ui/status_theme.go @@ -100,6 +100,9 @@ var ansiSeq = regexp.MustCompile(`\x1b\[[0-9;]*m`) func (a *annotationWriter) Write(p []byte) (int, error) { n, err := a.inner.Write(p) + if err != nil { + return n, err + } msg := strings.TrimSpace(ansiSeq.ReplaceAllString(string(p), "")) // Drop the printer's own severity marker — the annotation level already // carries it. Exactly ONE marker is stripped (the printed line always diff --git a/tests/integration/common/cli_runner.go b/tests/integration/common/cli_runner.go index bc4c0f57..8248e71f 100644 --- a/tests/integration/common/cli_runner.go +++ b/tests/integration/common/cli_runner.go @@ -19,11 +19,12 @@ func InitializeCLI() error { // If already initialized and binary exists, check if it's newer than source if cliBinary != "" { if stat, err := os.Stat(cliBinary); err == nil { - // Check if binary is newer than main.go (simple check) + // Check if binary is newer than all source files under the project root := GetProjectRoot() - if mainStat, err := os.Stat(filepath.Join(root, "main.go")); err == nil { - if stat.ModTime().After(mainStat.ModTime()) { - return nil // Binary is newer than source, no rebuild needed + newest, err := latestSourceModTime(root) + if err == nil { + if stat.ModTime().After(newest) { + return nil // Binary is newer than all source, no rebuild needed } } else { return nil // Can't check source, assume binary is good @@ -57,6 +58,58 @@ func InitializeCLI() error { return nil } +// latestSourceModTime walks main.go plus the cmd/ and internal/ directories +// (if present) and returns the most recent modification time found among all +// .go files, so caching decisions account for changes anywhere in the module, +// not just main.go. +func latestSourceModTime(root string) (mostRecent time.Time, err error) { + checkFile := func(path string) error { + info, statErr := os.Stat(path) + if statErr != nil { + return statErr + } + if info.ModTime().After(mostRecent) { + mostRecent = info.ModTime() + } + return nil + } + + mainPath := filepath.Join(root, "main.go") + if statErr := checkFile(mainPath); statErr != nil { + return mostRecent, statErr + } + + dirs := []string{filepath.Join(root, "cmd"), filepath.Join(root, "internal")} + foundAny := false + for _, dir := range dirs { + if _, statErr := os.Stat(dir); statErr != nil { + continue + } + walkErr := filepath.Walk(dir, func(path string, info os.FileInfo, walkErr error) error { + if walkErr != nil { + return walkErr + } + if info.IsDir() { + return nil + } + if !strings.HasSuffix(info.Name(), ".go") { + return nil + } + foundAny = true + if info.ModTime().After(mostRecent) { + mostRecent = info.ModTime() + } + return nil + }) + if walkErr != nil { + return mostRecent, walkErr + } + } + _ = foundAny + + return mostRecent, nil +} + // CleanupCLI removes the test CLI binary func CleanupCLI() { if cliBinary != "" { diff --git a/tests/integration/common/cluster_management.go b/tests/integration/common/cluster_management.go index dafe5d96..b6ed4b53 100644 --- a/tests/integration/common/cluster_management.go +++ b/tests/integration/common/cluster_management.go @@ -7,6 +7,53 @@ import ( "time" ) +// testResourcePatterns is the shared list of substrings used to identify +// Docker resources (networks/containers) created by integration tests, so +// cleanup logic does not drift between the different cleanup helpers. +var testResourcePatterns = []string{ + "test", + "collision", + "interrupt", + "stress", + "multi", + "integration", +} + +// matchesTestResourcePattern reports whether name contains one of the known +// test-name substrings. +func matchesTestResourcePattern(name string) bool { + for _, pattern := range testResourcePatterns { + if strings.Contains(name, pattern) { + return true + } + } + return false +} + +// removeDockerResourcesByFilter lists docker resources of the given kind +// (e.g. "network" or "container") using the provided list/format args and a +// docker filter, then removes each listed resource whose name matches the +// known test-name substrings (unless requireMatch is false, in which case +// all listed resources are removed). +func removeDockerResourcesByFilter(listArgs []string, removeArgs func(name string) []string, requireMatch bool) { + cmd := exec.Command("docker", listArgs...) // #nosec G204 -- integration test harness runs the built CLI/tools with controlled args + output, err := cmd.Output() + if err != nil { + return + } + names := strings.Split(strings.TrimSpace(string(output)), "\n") + for _, name := range names { + if name == "" { + continue + } + if requireMatch && !matchesTestResourcePattern(name) { + continue + } + args := removeArgs(name) + _ = exec.Command("docker", args...).Run() // #nosec G204 -- integration test harness runs the built CLI/tools with controlled args + } +} + // GenerateTestClusterName creates a unique cluster name for testing func GenerateTestClusterName() string { return fmt.Sprintf("integration-test-%d", time.Now().Unix()) @@ -96,50 +143,31 @@ func CleanupAllTestClusters() { cleanupDockerResources() } -// cleanupDockerResources removes leftover k3d Docker networks and containers +// cleanupDockerResources removes leftover k3d Docker networks, containers, +// and registries that match known test-name substrings. func cleanupDockerResources() { // Clean up leftover k3d networks - cmd := exec.Command("docker", "network", "ls", "--filter", "name=k3d-", "--format", "{{.Name}}") - if output, err := cmd.Output(); err == nil { - networks := strings.Split(strings.TrimSpace(string(output)), "\n") - for _, network := range networks { - if network != "" && (strings.Contains(network, "test") || - strings.Contains(network, "collision") || - strings.Contains(network, "interrupt") || - strings.Contains(network, "stress") || - strings.Contains(network, "multi") || - strings.Contains(network, "integration")) { - _ = exec.Command("docker", "network", "rm", network).Run() // #nosec G204 -- integration test harness runs the built CLI/tools with controlled args - } - } - } + removeDockerResourcesByFilter( + []string{"network", "ls", "--filter", "name=k3d-", "--format", "{{.Name}}"}, + func(name string) []string { return []string{"network", "rm", name} }, + true, + ) // Clean up leftover k3d containers - cmd = exec.Command("docker", "ps", "-a", "--filter", "name=k3d-", "--format", "{{.Names}}") - if output, err := cmd.Output(); err == nil { - containers := strings.Split(strings.TrimSpace(string(output)), "\n") - for _, container := range containers { - if container != "" && (strings.Contains(container, "test") || - strings.Contains(container, "collision") || - strings.Contains(container, "interrupt") || - strings.Contains(container, "stress") || - strings.Contains(container, "multi") || - strings.Contains(container, "integration")) { - _ = exec.Command("docker", "rm", "-f", container).Run() // #nosec G204 -- integration test harness runs the built CLI/tools with controlled args - } - } - } - - // Clean up any leftover k3d registries that might conflict - cmd = exec.Command("docker", "ps", "-a", "--filter", "name=k3d-.*-registry", "--format", "{{.Names}}") - if output, err := cmd.Output(); err == nil { - registries := strings.Split(strings.TrimSpace(string(output)), "\n") - for _, registry := range registries { - if registry != "" { - _ = exec.Command("docker", "rm", "-f", registry).Run() // #nosec G204 -- integration test harness runs the built CLI/tools with controlled args - } - } - } + removeDockerResourcesByFilter( + []string{"ps", "-a", "--filter", "name=k3d-", "--format", "{{.Names}}"}, + func(name string) []string { return []string{"rm", "-f", name} }, + true, + ) + + // Clean up any leftover k3d registries that might conflict, restricted + // to registries matching known test-name substrings to avoid deleting + // unrelated k3d registries. + removeDockerResourcesByFilter( + []string{"ps", "-a", "--filter", "name=k3d-.*-registry", "--format", "{{.Names}}"}, + func(name string) []string { return []string{"rm", "-f", name} }, + true, + ) } // cleanupClusterSpecificResources removes Docker resources for a specific cluster @@ -149,13 +177,9 @@ func cleanupClusterSpecificResources(clusterName string) { _ = exec.Command("docker", "network", "rm", networkName).Run() // #nosec G204 -- integration test harness runs the built CLI/tools with controlled args // Remove specific cluster containers - cmd := exec.Command("docker", "ps", "-a", "--filter", fmt.Sprintf("name=k3d-%s", clusterName), "--format", "{{.Names}}") // #nosec G204 -- integration test harness runs the built CLI/tools with controlled args - if output, err := cmd.Output(); err == nil { - containers := strings.Split(strings.TrimSpace(string(output)), "\n") - for _, container := range containers { - if container != "" { - _ = exec.Command("docker", "rm", "-f", container).Run() // #nosec G204 -- integration test harness runs the built CLI/tools with controlled args - } - } - } + removeDockerResourcesByFilter( + []string{"ps", "-a", "--filter", fmt.Sprintf("name=k3d-%s", clusterName), "--format", "{{.Names}}"}, + func(name string) []string { return []string{"rm", "-f", name} }, + false, + ) } diff --git a/tests/testutil/patterns.go b/tests/testutil/patterns.go index 4fa44e2d..57f5955a 100644 --- a/tests/testutil/patterns.go +++ b/tests/testutil/patterns.go @@ -99,8 +99,8 @@ func testCommandCLI(t *testing.T, commandName string, cmdFunc func() *cobra.Comm // Test too many arguments should fail err = cmd.Args(cmd, []string{"arg1", "arg2", "arg3"}) - if err == nil && commandName != "list" { // list command typically accepts no args - t.Logf("Command %s accepts multiple arguments", commandName) + if commandName == "list" { // list command typically accepts no args + assert.Error(t, err, "Command %s should reject multiple arguments", commandName) } } }