From 5baa3a91af277d0f76d8a082735bb8c495fca220 Mon Sep 17 00:00:00 2001 From: cOmrade3267 Date: Thu, 18 Jun 2026 14:52:45 +0530 Subject: [PATCH 1/2] fix: block metrics-server install on Talos clusters via RequestedSplit - Add clusterType parameter to RequestedSplit - After checkAppPlan resolves the app name, check if clusterType is 'talos' and the resolved name is 'metrics-server'; if so, print a clear error and exit before the API call is made - checkAppPlan is unchanged from master (no side effects) - Pass kubernetesFindCluster.ClusterType at the call site in kubernetes_app_add.go - Add unit tests for RequestedSplit covering non-Talos pass-through and the os.Exit path note --- cmd/kubernetes/kubernetes_app_add.go | 2 +- utility/kubernetes.go | 19 ++- utility/kubernetes_test.go | 238 +++++++++++++++++++++++++-- 3 files changed, 242 insertions(+), 17 deletions(-) diff --git a/cmd/kubernetes/kubernetes_app_add.go b/cmd/kubernetes/kubernetes_app_add.go index 48dba63d..14468c20 100644 --- a/cmd/kubernetes/kubernetes_app_add.go +++ b/cmd/kubernetes/kubernetes_app_add.go @@ -43,7 +43,7 @@ var kubernetesAppAddCmd = &cobra.Command{ os.Exit(1) } - result := utility.RequestedSplit(appList, args[0]) + result := utility.RequestedSplit(appList, args[0], kubernetesFindCluster.ClusterType) configKubernetes := &civogo.KubernetesClusterConfig{ Applications: result, } diff --git a/utility/kubernetes.go b/utility/kubernetes.go index 1ceb94db..b8c28c82 100644 --- a/utility/kubernetes.go +++ b/utility/kubernetes.go @@ -151,6 +151,9 @@ func checkAppPlan(appList []civogo.KubernetesMarketplaceApplication, requested s os.Exit(1) } + // NOTE: when no plan is specified, planName == "" which never matches + // any plan label, so checkAppPlan always falls back to the default plan. + // This is pre-existing behaviour inherited from master. if len(appList[foundIndex].Plans) > 0 { allPlan := []string{} @@ -176,16 +179,30 @@ func checkAppPlan(appList []civogo.KubernetesMarketplaceApplication, requested s } // RequestedSplit is a function to split all app requested to be installed -func RequestedSplit(appList []civogo.KubernetesMarketplaceApplication, requested string) string { +func RequestedSplit(appList []civogo.KubernetesMarketplaceApplication, requested string, clusterType string) string { allsplit := strings.Split(requested, ",") allApp := []string{} for i := range allsplit { checkApp, err := checkAppPlan(appList, allsplit[i]) + // NOTE: checkAppPlan either returns a nil error or calls os.Exit(1) + // internally — it never returns a non-nil error here. This branch + // is effectively dead code inherited from master. if err != nil { fmt.Print(err) } + // Check Talos compatibility after resolving canonical app name + resolvedName := strings.SplitN(checkApp, ":", 2)[0] + if strings.EqualFold(clusterType, "talos") && + strings.EqualFold(resolvedName, "metrics-server") { + Error( + "The metrics-server marketplace application is not " + + "currently supported on Talos clusters.", + ) + os.Exit(1) + } + allApp = append(allApp, checkApp) } diff --git a/utility/kubernetes_test.go b/utility/kubernetes_test.go index 7e261b49..f3c492ea 100644 --- a/utility/kubernetes_test.go +++ b/utility/kubernetes_test.go @@ -6,6 +6,229 @@ import ( "github.com/civo/civogo" ) +// --- checkAppPlan tests --- + +func TestCheckAppPlanFindsAppByName(t *testing.T) { + appList := []civogo.KubernetesMarketplaceApplication{ + {Name: "redis"}, + {Name: "mysql"}, + {Name: "postgresql"}, + } + + result, err := checkAppPlan(appList, "mysql") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result != "mysql" { + t.Errorf("expected 'mysql', got '%s'", result) + } +} + +func TestCheckAppPlanFindsAppWithValidPlan(t *testing.T) { + appList := []civogo.KubernetesMarketplaceApplication{ + { + Name: "mysql", + Plans: []civogo.KubernetesMarketplacePlan{ + {Label: "5GB"}, + {Label: "10GB"}, + }, + }, + } + + result, err := checkAppPlan(appList, "mysql:10GB") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result != "mysql:10GB" { + t.Errorf("expected 'mysql:10GB', got '%s'", result) + } +} + +func TestCheckAppPlanReturnsDefaultForInvalidPlan(t *testing.T) { + appList := []civogo.KubernetesMarketplaceApplication{ + { + Name: "mysql", + Plans: []civogo.KubernetesMarketplacePlan{ + {Label: "5GB"}, + {Label: "10GB"}, + }, + }, + } + + result, err := checkAppPlan(appList, "mysql:999GB") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + // Invalid plan falls back to default (first plan) + if result != "mysql:5GB" { + t.Errorf("expected 'mysql:5GB', got '%s'", result) + } +} + +func TestCheckAppPlanNoPlanSpecifiedWithPlansAvailable(t *testing.T) { + appList := []civogo.KubernetesMarketplaceApplication{ + { + Name: "mysql", + Plans: []civogo.KubernetesMarketplacePlan{ + {Label: "5GB"}, + }, + }, + } + + result, err := checkAppPlan(appList, "mysql") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + // No plan specified and plan is empty string — doesn't match any plan, + // so it falls back to default + if result != "mysql:5GB" { + t.Errorf("expected 'mysql:5GB', got '%s'", result) + } +} + +func TestCheckAppPlanNoPlanSpecifiedNoPlansAvailable(t *testing.T) { + appList := []civogo.KubernetesMarketplaceApplication{ + {Name: "traefik"}, + } + + result, err := checkAppPlan(appList, "traefik") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result != "traefik" { + t.Errorf("expected 'traefik', got '%s'", result) + } +} + +func TestCheckAppPlanPartialNameMatch(t *testing.T) { + appList := []civogo.KubernetesMarketplaceApplication{ + {Name: "metrics-server"}, + {Name: "mysql"}, + } + + result, err := checkAppPlan(appList, "metrics") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + // No plans, returns requested string verbatim + if result != "metrics" { + t.Errorf("expected 'metrics', got '%s'", result) + } +} + +// --- RequestedSplit tests --- + +func TestRequestedSplitSingleApp(t *testing.T) { + appList := []civogo.KubernetesMarketplaceApplication{ + {Name: "redis"}, + {Name: "mysql"}, + } + + result := RequestedSplit(appList, "mysql", "k3s") + if result != "mysql" { + t.Errorf("expected 'mysql', got '%s'", result) + } +} + +func TestRequestedSplitMultipleApps(t *testing.T) { + appList := []civogo.KubernetesMarketplaceApplication{ + {Name: "redis"}, + {Name: "mysql"}, + {Name: "postgresql"}, + } + + result := RequestedSplit(appList, "redis,mysql", "k3s") + if result != "redis,mysql" { + t.Errorf("expected 'redis,mysql', got '%s'", result) + } +} + +func TestRequestedSplitAppWithValidPlan(t *testing.T) { + appList := []civogo.KubernetesMarketplaceApplication{ + { + Name: "mysql", + Plans: []civogo.KubernetesMarketplacePlan{ + {Label: "5GB"}, + {Label: "10GB"}, + }, + }, + } + + result := RequestedSplit(appList, "mysql:10GB", "k3s") + if result != "mysql:10GB" { + t.Errorf("expected 'mysql:10GB', got '%s'", result) + } +} + +func TestRequestedSplitAppWithInvalidPlanFallsBackToDefault(t *testing.T) { + appList := []civogo.KubernetesMarketplaceApplication{ + { + Name: "mysql", + Plans: []civogo.KubernetesMarketplacePlan{ + {Label: "5GB"}, + {Label: "10GB"}, + }, + }, + } + + result := RequestedSplit(appList, "mysql:999GB", "k3s") + expected := "mysql:5GB" + if result != expected { + t.Errorf("expected '%s', got '%s'", expected, result) + } +} + +func TestRequestedSplitAppWithoutPlanAndPlansAvailable(t *testing.T) { + appList := []civogo.KubernetesMarketplaceApplication{ + { + Name: "mysql", + Plans: []civogo.KubernetesMarketplacePlan{ + {Label: "5GB"}, + }, + }, + } + + // When no plan is specified and app has plans, find("") returns false, + // so checkAppPlan falls back to the default (first) plan. + result := RequestedSplit(appList, "mysql", "k3s") + if result != "mysql:5GB" { + t.Errorf("expected 'mysql:5GB', got '%s'", result) + } +} + +func TestRequestedSplitMultipleAppsWithPlans(t *testing.T) { + appList := []civogo.KubernetesMarketplaceApplication{ + {Name: "redis"}, + { + Name: "mysql", + Plans: []civogo.KubernetesMarketplacePlan{ + {Label: "5GB"}, + {Label: "10GB"}, + }, + }, + } + + result := RequestedSplit(appList, "redis,mysql:10GB", "k3s") + if result != "redis,mysql:10GB" { + t.Errorf("expected 'redis,mysql:10GB', got '%s'", result) + } +} + +// NOTE: The Talos + metrics-server blocking path calls os.Exit(1) and +// cannot be unit tested directly. Manual testing is required for that path. +func TestRequestedSplitNonTalosAllowsMetricsServer(t *testing.T) { + appList := []civogo.KubernetesMarketplaceApplication{ + {Name: "metrics-server"}, + } + + result := RequestedSplit(appList, "metrics-server", "k3s") + if result != "metrics-server" { + t.Errorf("expected 'metrics-server', got '%s'", result) + } +} + +// --- RemoveApplicationFromInstalledList tests --- + func TestRemoveApplicationFromInstalledListSimpleName(t *testing.T) { current := []civogo.KubernetesInstalledApplication{ { @@ -56,18 +279,3 @@ func TestRemoveApplicationFromInstalledListWithMultiple(t *testing.T) { t.Errorf("expected 'redis', got '%s'", ret) } } - -// func TestRemoveApplicationFromInstalledListWithPlan(t *testing.T) { -// current := []civogo.KubernetesInstalledApplication{ -// { -// Name: "mysql", -// }, -// } -// uninstall := "mysql" - -// ret := RemoveApplicationFromInstalledList(current, uninstall) - -// if ret != "mysql" { -// t.Errorf("expected 'mysql', got '%s'", ret) -// } -// } From d13575b32db103b8750a91eb917d29d57c95f96d Mon Sep 17 00:00:00 2001 From: cOmrade3267 Date: Thu, 18 Jun 2026 15:42:43 +0530 Subject: [PATCH 2/2] refactor: checkAppPlan returns index for canonical name comparison in RequestedSplit Per reviewer feedback: return foundIndex from checkAppPlan so that RequestedSplit can compare against appList[idx].Name (the canonical app name from the API) instead of re-parsing the resolved string with strings.SplitN. Benefits: - Eliminates the redundant SplitN call inside RequestedSplit - Correctly handles partial-name inputs: e.g. user types 'metrics', checkAppPlan resolves to index of 'metrics-server', so appList[idx].Name == 'metrics-server' and the Talos block fires - No change to the string returned for the Civo API (checkApp is still built the same way) Tests updated to capture the new (int, string, error) return and assert that appList[idx].Name matches the canonical app name. --- utility/kubernetes.go | 22 +++++++++++++--------- utility/kubernetes_test.go | 22 +++++++++++++++------- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/utility/kubernetes.go b/utility/kubernetes.go index b8c28c82..29150f32 100644 --- a/utility/kubernetes.go +++ b/utility/kubernetes.go @@ -127,8 +127,11 @@ func writeConfig(path string, data []byte, suppressMessage bool, mergeConfigs bo // checkAppPlan is the function to verify if the application to be installed in the cluster // has a plan or not, in case it has a plan but does not specify it, -// we use the first one in the list -func checkAppPlan(appList []civogo.KubernetesMarketplaceApplication, requested string) (string, error) { +// we use the first one in the list. +// It returns the index of the matched application in appList so that callers can +// access the canonical application name (appList[idx].Name) without re-parsing +// the resolved string. +func checkAppPlan(appList []civogo.KubernetesMarketplaceApplication, requested string) (int, string, error) { foundIndex := -1 parts := strings.SplitN(requested, ":", 2) appName := parts[0] @@ -164,10 +167,10 @@ func checkAppPlan(appList []civogo.KubernetesMarketplaceApplication, requested s _, found := find(allPlan, planName) if !found { YellowConfirm("the plan you gave doesn't exist for %s; we've picked a default one for you\n", appName) - return fmt.Sprintf("%s:%s", appName, appList[foundIndex].Plans[0].Label), nil + return foundIndex, fmt.Sprintf("%s:%s", appName, appList[foundIndex].Plans[0].Label), nil } - return requested, nil + return foundIndex, requested, nil } if planName != "" { @@ -175,7 +178,7 @@ func checkAppPlan(appList []civogo.KubernetesMarketplaceApplication, requested s os.Exit(1) } - return requested, nil + return foundIndex, requested, nil } // RequestedSplit is a function to split all app requested to be installed @@ -184,7 +187,7 @@ func RequestedSplit(appList []civogo.KubernetesMarketplaceApplication, requested allApp := []string{} for i := range allsplit { - checkApp, err := checkAppPlan(appList, allsplit[i]) + idx, checkApp, err := checkAppPlan(appList, allsplit[i]) // NOTE: checkAppPlan either returns a nil error or calls os.Exit(1) // internally — it never returns a non-nil error here. This branch // is effectively dead code inherited from master. @@ -192,10 +195,11 @@ func RequestedSplit(appList []civogo.KubernetesMarketplaceApplication, requested fmt.Print(err) } - // Check Talos compatibility after resolving canonical app name - resolvedName := strings.SplitN(checkApp, ":", 2)[0] + // Block metrics-server on Talos clusters using the canonical app name + // from appList[idx].Name — this correctly handles partial-name inputs + // (e.g. "metrics" resolves to appList[idx].Name == "metrics-server"). if strings.EqualFold(clusterType, "talos") && - strings.EqualFold(resolvedName, "metrics-server") { + strings.EqualFold(appList[idx].Name, "metrics-server") { Error( "The metrics-server marketplace application is not " + "currently supported on Talos clusters.", diff --git a/utility/kubernetes_test.go b/utility/kubernetes_test.go index f3c492ea..a0120eaa 100644 --- a/utility/kubernetes_test.go +++ b/utility/kubernetes_test.go @@ -15,13 +15,17 @@ func TestCheckAppPlanFindsAppByName(t *testing.T) { {Name: "postgresql"}, } - result, err := checkAppPlan(appList, "mysql") + idx, result, err := checkAppPlan(appList, "mysql") if err != nil { t.Fatalf("unexpected error: %v", err) } if result != "mysql" { t.Errorf("expected 'mysql', got '%s'", result) } + // Index must point to the matched entry + if appList[idx].Name != "mysql" { + t.Errorf("expected appList[idx].Name = 'mysql', got '%s'", appList[idx].Name) + } } func TestCheckAppPlanFindsAppWithValidPlan(t *testing.T) { @@ -35,7 +39,7 @@ func TestCheckAppPlanFindsAppWithValidPlan(t *testing.T) { }, } - result, err := checkAppPlan(appList, "mysql:10GB") + _, result, err := checkAppPlan(appList, "mysql:10GB") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -55,7 +59,7 @@ func TestCheckAppPlanReturnsDefaultForInvalidPlan(t *testing.T) { }, } - result, err := checkAppPlan(appList, "mysql:999GB") + _, result, err := checkAppPlan(appList, "mysql:999GB") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -75,7 +79,7 @@ func TestCheckAppPlanNoPlanSpecifiedWithPlansAvailable(t *testing.T) { }, } - result, err := checkAppPlan(appList, "mysql") + _, result, err := checkAppPlan(appList, "mysql") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -91,7 +95,7 @@ func TestCheckAppPlanNoPlanSpecifiedNoPlansAvailable(t *testing.T) { {Name: "traefik"}, } - result, err := checkAppPlan(appList, "traefik") + _, result, err := checkAppPlan(appList, "traefik") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -106,14 +110,18 @@ func TestCheckAppPlanPartialNameMatch(t *testing.T) { {Name: "mysql"}, } - result, err := checkAppPlan(appList, "metrics") + idx, result, err := checkAppPlan(appList, "metrics") if err != nil { t.Fatalf("unexpected error: %v", err) } - // No plans, returns requested string verbatim + // String returned is verbatim user input (no plans on this app) if result != "metrics" { t.Errorf("expected 'metrics', got '%s'", result) } + // But the index points to the CANONICAL app — metrics-server + if appList[idx].Name != "metrics-server" { + t.Errorf("expected appList[idx].Name = 'metrics-server', got '%s'", appList[idx].Name) + } } // --- RequestedSplit tests ---