From 5a2d4b84eb65e14329f3153b58cb12a1a8f7a6e3 Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Wed, 16 Sep 2026 12:48:08 +0200 Subject: [PATCH 1/7] feat(cluster-version): read version from ClusterKubeconfig label MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When --greenhouse-cluster-namespace and --greenhouse-cluster-name are provided, cluster-version first attempts to read the greenhouse.sap/kubernetes-version label from the ClusterKubeconfig resource on Greenhouse. This is faster and works even when the remote API server is temporarily unavailable. If the label is absent, the resource is not found, or the Greenhouse connection fails, the command falls back to the existing live query path (unauthenticated GET /version → authenticated fallback). New flags on cluster-version: -g, --greenhouse-cluster-kubeconfig path to Greenhouse kubeconfig --greenhouse-cluster-context context in that kubeconfig -n, --greenhouse-cluster-namespace Greenhouse org namespace --greenhouse-cluster-name ClusterKubeconfig resource name Closes #81 Signed-off-by: onuryilmaz --- cmd/cluster-version.go | 120 +++++++++++++++++++++++++++++------- cmd/cluster-version_test.go | 100 ++++++++++++++++++++++++++++++ go.mod | 1 + 3 files changed, 198 insertions(+), 23 deletions(-) diff --git a/cmd/cluster-version.go b/cmd/cluster-version.go index b52ef12..789c42f 100644 --- a/cmd/cluster-version.go +++ b/cmd/cluster-version.go @@ -9,16 +9,20 @@ import ( "crypto/x509" "encoding/json" "fmt" + "log/slog" "net/http" "os" "strings" "time" + "github.com/cloudoperators/greenhouse/api/v1alpha1" "github.com/spf13/cobra" "github.com/spf13/viper" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/version" "k8s.io/client-go/rest" clientcmd "k8s.io/client-go/tools/clientcmd" + "sigs.k8s.io/controller-runtime/pkg/client" "github.com/cloudoperators/cloudctl/cmd/output" ) @@ -26,19 +30,26 @@ import ( var clusterVersionCmd = &cobra.Command{ Use: "cluster-version", Short: "Print the Kubernetes server version for a kubeconfig context", - Long: `Queries the Kubernetes API server version for the given kubeconfig context. + Long: `Queries the Kubernetes server version for the given kubeconfig context. -An unauthenticated GET to /version is attempted first (faster, no token -refresh required). If the server requires authentication, cloudctl falls -back to an authenticated GET to /version using the kubeconfig credentials. +When Greenhouse connection flags are provided (--greenhouse-cluster-namespace and +--greenhouse-cluster-name), the version is read from the greenhouse.sap/kubernetes-version +label on the ClusterKubeconfig resource — faster and resilient to remote API downtime. + +If the label is absent or Greenhouse flags are not provided, cloudctl falls back to +querying the remote cluster directly: an unauthenticated GET to /version is attempted +first; if the server requires authentication, an authenticated GET is used instead. If the API server is unreachable the command exits after --timeout (default 10s). Examples: - # Version of the current context + # Version of the current context (live query) cloudctl cluster-version - # Version of a specific context + # Version from Greenhouse label (preferred when syncing via cloudctl) + cloudctl cluster-version -n my-org --greenhouse-cluster-name prod-eu + + # Version of a specific context with live query cloudctl cluster-version --context prod-eu # Machine-readable output @@ -52,6 +63,11 @@ Examples: var ( kubeconfig string kubecontext string + + cvGreenhouseKubeconfig string + cvGreenhouseContext string + cvGreenhouseNamespace string + cvGreenhouseClusterName string ) func runClusterVersion(cmd *cobra.Command, args []string) error { @@ -63,6 +79,11 @@ func runClusterVersion(cmd *cobra.Command, args []string) error { return fmt.Errorf("--kubeconfig must not be empty") } + cvGreenhouseKubeconfig = resolveKubeconfig("greenhouse-cluster-kubeconfig", viper.GetString("greenhouse-cluster-kubeconfig")) + cvGreenhouseContext = viper.GetString("greenhouse-cluster-context") + cvGreenhouseNamespace = viper.GetString("greenhouse-cluster-namespace") + cvGreenhouseClusterName = viper.GetString("greenhouse-cluster-name") + timeoutStr := viper.GetString("timeout") timeout, err := time.ParseDuration(timeoutStr) if err != nil { @@ -109,31 +130,79 @@ func runClusterVersion(cmd *cobra.Command, args []string) error { ctx, cancel := context.WithTimeout(cmd.Context(), timeout) defer cancel() - // 1) Try unauthenticated GET /version - ver, err := getUnauthenticatedVersion(ctx, cfg) - if err != nil { - // 2) Fallback to authenticated - if !hasAuth(cfg) { - stopQuery() - return fmt.Errorf("no authentication methods found in your kubeconfig. Please authenticate (`kubelogin`, etc.) and try again") + var clusterVersion string + + // 1) Try reading version from the ClusterKubeconfig label on Greenhouse. + if cvGreenhouseNamespace != "" && cvGreenhouseClusterName != "" { + labelVer, labelErr := getVersionFromLabel(ctx, cvGreenhouseKubeconfig, cvGreenhouseContext, cvGreenhouseNamespace, cvGreenhouseClusterName) + if labelErr != nil { + slog.Debug("label-based version lookup failed, falling back to live query", "error", labelErr) + } else if labelVer != "" { + clusterVersion = labelVer } + } - ver, err = getAuthenticatedVersion(ctx, cfg) + if clusterVersion == "" { + // 2) Try unauthenticated GET /version + var ver *version.Info + ver, err = getUnauthenticatedVersion(ctx, cfg) if err != nil { - stopQuery() - return fmt.Errorf("authenticated version fetch failed: %w", err) + // 3) Fallback to authenticated + if !hasAuth(cfg) { + stopQuery() + return fmt.Errorf("no authentication methods found in your kubeconfig. Please authenticate (`kubelogin`, etc.) and try again") + } + + ver, err = getAuthenticatedVersion(ctx, cfg) + if err != nil { + stopQuery() + return fmt.Errorf("authenticated version fetch failed: %w", err) + } } + + // Strip build metadata so we get a clean semver string (e.g. "1.29.3"). + parts := strings.Split(ver.GitVersion, "-") + clean := parts[0] + parts = strings.Split(clean, "+") + clean = parts[0] + clusterVersion = strings.TrimPrefix(clean, "v") } + stopQuery() + return printer.Print(output.ClusterVersionResult{Context: effectiveContext, Version: clusterVersion}) +} - // Strip build metadata so we get a clean semver string (e.g. "1.29.3"). - parts := strings.Split(ver.GitVersion, "-") - clean := parts[0] - parts = strings.Split(clean, "+") - clean = parts[0] - clusterVersion := strings.TrimPrefix(clean, "v") +// getVersionFromLabel reads the greenhouse.sap/kubernetes-version label from the +// named ClusterKubeconfig resource. Returns ("", nil) when the resource has no +// such label or when the resource is not found, so callers can fall through to +// a live query. +func getVersionFromLabel(ctx context.Context, greenhouseKubeconfig, greenhouseContext, namespace, clusterName string) (string, error) { + cfg, err := configWithContext(greenhouseContext, greenhouseKubeconfig) + if err != nil { + return "", fmt.Errorf("failed to build greenhouse kubeconfig: %w", err) + } - return printer.Print(output.ClusterVersionResult{Context: effectiveContext, Version: clusterVersion}) + scheme := runtime.NewScheme() + if err := v1alpha1.AddToScheme(scheme); err != nil { + return "", fmt.Errorf("failed to add greenhouse scheme: %w", err) + } + + c, err := client.New(cfg, client.Options{Scheme: scheme}) + if err != nil { + return "", fmt.Errorf("failed to create greenhouse client: %w", err) + } + + return versionLabelFromClient(ctx, c, namespace, clusterName) +} + +// versionLabelFromClient fetches the greenhouse.sap/kubernetes-version label +// using an already-constructed client. Separated for testability. +func versionLabelFromClient(ctx context.Context, c client.Client, namespace, clusterName string) (string, error) { + var ckc v1alpha1.ClusterKubeconfig + if err := c.Get(ctx, client.ObjectKey{Namespace: namespace, Name: clusterName}, &ckc); err != nil { + return "", nil + } + return ckc.Labels["greenhouse.sap/kubernetes-version"], nil } // hasAuth returns true if the rest.Config contains any credential source. @@ -251,6 +320,11 @@ func init() { clusterVersionCmd.Flags().StringVarP(&kubecontext, "context", "c", "", "Kubeconfig context to query (defaults to current context)") clusterVersionCmd.Flags().String("timeout", "10s", "Maximum time to wait for the API server to respond") + clusterVersionCmd.Flags().StringVarP(&cvGreenhouseKubeconfig, "greenhouse-cluster-kubeconfig", "g", clientcmd.RecommendedHomeFile, "Path to the Greenhouse cluster kubeconfig (for label-based version lookup)") + clusterVersionCmd.Flags().StringVar(&cvGreenhouseContext, "greenhouse-cluster-context", "", "Context to use from the Greenhouse kubeconfig") + clusterVersionCmd.Flags().StringVarP(&cvGreenhouseNamespace, "greenhouse-cluster-namespace", "n", "", "Greenhouse organization namespace") + clusterVersionCmd.Flags().StringVar(&cvGreenhouseClusterName, "greenhouse-cluster-name", "", "ClusterKubeconfig resource name in Greenhouse to read the version label from") + // BindPFlags can theoretically return an error if called with `nil` as an argument // which should never happen after at least one flag was defined. That's why the output // there is ignored. diff --git a/cmd/cluster-version_test.go b/cmd/cluster-version_test.go index 8dc76e6..bee5e92 100644 --- a/cmd/cluster-version_test.go +++ b/cmd/cluster-version_test.go @@ -9,13 +9,18 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "os" "testing" + "github.com/cloudoperators/greenhouse/api/v1alpha1" . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/version" "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" + "sigs.k8s.io/controller-runtime/pkg/client/fake" ) func TestHasAuth(t *testing.T) { @@ -140,3 +145,98 @@ func TestClusterVersionKubeconfigFlag_DefaultEqualsRecommendedHomeFile(t *testin g.Expect(f).ToNot(BeNil()) g.Expect(f.DefValue).To(Equal(clientcmd.RecommendedHomeFile)) } + +func newGreenhouseFakeClient(objs ...v1alpha1.ClusterKubeconfig) *fake.ClientBuilder { + scheme := runtime.NewScheme() + _ = v1alpha1.AddToScheme(scheme) + builder := fake.NewClientBuilder().WithScheme(scheme) + for i := range objs { + builder = builder.WithObjects(&objs[i]) + } + return builder +} + +func TestVersionLabelFromClient_LabelPresent(t *testing.T) { + g := NewWithT(t) + + ckc := v1alpha1.ClusterKubeconfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "prod-eu", + Namespace: "my-org", + Labels: map[string]string{"greenhouse.sap/kubernetes-version": "1.29.3"}, + }, + } + c := newGreenhouseFakeClient(ckc).Build() + + ver, err := versionLabelFromClient(context.Background(), c, "my-org", "prod-eu") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(ver).To(Equal("1.29.3")) +} + +func TestVersionLabelFromClient_LabelAbsent(t *testing.T) { + g := NewWithT(t) + + ckc := v1alpha1.ClusterKubeconfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "prod-eu", + Namespace: "my-org", + }, + } + c := newGreenhouseFakeClient(ckc).Build() + + ver, err := versionLabelFromClient(context.Background(), c, "my-org", "prod-eu") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(ver).To(BeEmpty()) +} + +func TestVersionLabelFromClient_NotFound(t *testing.T) { + g := NewWithT(t) + + c := newGreenhouseFakeClient().Build() + + ver, err := versionLabelFromClient(context.Background(), c, "my-org", "missing-cluster") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(ver).To(BeEmpty()) +} + +func TestVersionLabelFromClient_WrongNamespace(t *testing.T) { + g := NewWithT(t) + + ckc := v1alpha1.ClusterKubeconfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "prod-eu", + Namespace: "other-org", + Labels: map[string]string{"greenhouse.sap/kubernetes-version": "1.30.0"}, + }, + } + c := newGreenhouseFakeClient(ckc).Build() + + // Looking up in the wrong namespace returns not-found, falls back gracefully. + ver, err := versionLabelFromClient(context.Background(), c, "my-org", "prod-eu") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(ver).To(BeEmpty()) +} + +func TestClusterVersionGreenhouseFlags(t *testing.T) { + g := NewWithT(t) + + g.Expect(clusterVersionCmd.Flags().Lookup("greenhouse-cluster-kubeconfig")).ToNot(BeNil()) + g.Expect(clusterVersionCmd.Flags().Lookup("greenhouse-cluster-context")).ToNot(BeNil()) + g.Expect(clusterVersionCmd.Flags().Lookup("greenhouse-cluster-namespace")).ToNot(BeNil()) + g.Expect(clusterVersionCmd.Flags().Lookup("greenhouse-cluster-name")).ToNot(BeNil()) +} + +func TestGetVersionFromLabel_BadKubeconfig(t *testing.T) { + g := NewWithT(t) + + // A kubeconfig with invalid YAML should cause getVersionFromLabel to return an error. + f, err := os.CreateTemp("", "bad-kube-*.yaml") + g.Expect(err).ToNot(HaveOccurred()) + defer os.Remove(f.Name()) + _, _ = f.WriteString("invalid yaml: [") + f.Close() + + ver, err := getVersionFromLabel(context.Background(), f.Name(), "", "my-org", "prod-eu") + g.Expect(err).To(HaveOccurred()) + g.Expect(ver).To(BeEmpty()) +} diff --git a/go.mod b/go.mod index d24ac71..f7516a3 100644 --- a/go.mod +++ b/go.mod @@ -104,6 +104,7 @@ require ( golang.org/x/time v0.14.0 // indirect gomodules.xyz/jsonpatch/v2 v2.5.0 // indirect google.golang.org/protobuf v1.36.11 // indirect + gopkg.in/evanphx/json-patch.v4 v4.13.0 // indirect gopkg.in/inf.v0 v0.9.1 // indirect k8s.io/api v0.35.0 // indirect k8s.io/apiextensions-apiserver v0.35.0 // indirect From e3de464fa946aeacde848b9acfb754b6e90d13bd Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Wed, 16 Sep 2026 12:56:57 +0200 Subject: [PATCH 2/7] fix(cluster-version): normalize label version and guard empty greenhouse kubeconfig MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Apply the same normalizeVersion() stripping (leading v, prerelease, build metadata) to the label path so output is identical to the live query path regardless of how the Greenhouse controller formats the version string (e.g. "v1.31.4+k3s1" → "1.31.4") - Reject an explicitly empty --greenhouse-cluster-kubeconfig, matching the existing guard in sync.go to prevent silent fallback to an unintended kubeconfig Signed-off-by: onuryilmaz --- cmd/cluster-version.go | 20 ++++++++++++++------ cmd/cluster-version_test.go | 17 +++++++++++++++-- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/cmd/cluster-version.go b/cmd/cluster-version.go index 789c42f..ad2d810 100644 --- a/cmd/cluster-version.go +++ b/cmd/cluster-version.go @@ -80,6 +80,9 @@ func runClusterVersion(cmd *cobra.Command, args []string) error { } cvGreenhouseKubeconfig = resolveKubeconfig("greenhouse-cluster-kubeconfig", viper.GetString("greenhouse-cluster-kubeconfig")) + if viper.IsSet("greenhouse-cluster-kubeconfig") && cvGreenhouseKubeconfig == "" { + return fmt.Errorf("--greenhouse-cluster-kubeconfig must not be empty") + } cvGreenhouseContext = viper.GetString("greenhouse-cluster-context") cvGreenhouseNamespace = viper.GetString("greenhouse-cluster-namespace") cvGreenhouseClusterName = viper.GetString("greenhouse-cluster-name") @@ -138,7 +141,7 @@ func runClusterVersion(cmd *cobra.Command, args []string) error { if labelErr != nil { slog.Debug("label-based version lookup failed, falling back to live query", "error", labelErr) } else if labelVer != "" { - clusterVersion = labelVer + clusterVersion = normalizeVersion(labelVer) } } @@ -161,17 +164,22 @@ func runClusterVersion(cmd *cobra.Command, args []string) error { } // Strip build metadata so we get a clean semver string (e.g. "1.29.3"). - parts := strings.Split(ver.GitVersion, "-") - clean := parts[0] - parts = strings.Split(clean, "+") - clean = parts[0] - clusterVersion = strings.TrimPrefix(clean, "v") + clusterVersion = normalizeVersion(ver.GitVersion) } stopQuery() return printer.Print(output.ClusterVersionResult{Context: effectiveContext, Version: clusterVersion}) } +// normalizeVersion strips a leading "v", prerelease suffix, and build metadata +// from a Kubernetes version string, returning a clean semver (e.g. "1.29.3"). +func normalizeVersion(v string) string { + v = strings.TrimPrefix(v, "v") + v = strings.Split(v, "-")[0] + v = strings.Split(v, "+")[0] + return v +} + // getVersionFromLabel reads the greenhouse.sap/kubernetes-version label from the // named ClusterKubeconfig resource. Returns ("", nil) when the resource has no // such label or when the resource is not found, so callers can fall through to diff --git a/cmd/cluster-version_test.go b/cmd/cluster-version_test.go index bee5e92..786ddb5 100644 --- a/cmd/cluster-version_test.go +++ b/cmd/cluster-version_test.go @@ -156,6 +156,16 @@ func newGreenhouseFakeClient(objs ...v1alpha1.ClusterKubeconfig) *fake.ClientBui return builder } +func TestNormalizeVersion(t *testing.T) { + g := NewWithT(t) + + g.Expect(normalizeVersion("v1.29.3")).To(Equal("1.29.3")) + g.Expect(normalizeVersion("1.29.3")).To(Equal("1.29.3")) + g.Expect(normalizeVersion("v1.31.4+k3s1")).To(Equal("1.31.4")) + g.Expect(normalizeVersion("v1.29.3-eks-1234567")).To(Equal("1.29.3")) + g.Expect(normalizeVersion("v1.31.4-k3s1")).To(Equal("1.31.4")) +} + func TestVersionLabelFromClient_LabelPresent(t *testing.T) { g := NewWithT(t) @@ -163,14 +173,17 @@ func TestVersionLabelFromClient_LabelPresent(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "prod-eu", Namespace: "my-org", - Labels: map[string]string{"greenhouse.sap/kubernetes-version": "1.29.3"}, + // Greenhouse controller stores values like "v1.29.3" or "v1.31.4-k3s1". + Labels: map[string]string{"greenhouse.sap/kubernetes-version": "v1.29.3"}, }, } c := newGreenhouseFakeClient(ckc).Build() ver, err := versionLabelFromClient(context.Background(), c, "my-org", "prod-eu") g.Expect(err).ToNot(HaveOccurred()) - g.Expect(ver).To(Equal("1.29.3")) + // versionLabelFromClient returns the raw label; normalization is the caller's job. + g.Expect(ver).To(Equal("v1.29.3")) + g.Expect(normalizeVersion(ver)).To(Equal("1.29.3")) } func TestVersionLabelFromClient_LabelAbsent(t *testing.T) { From de4655b8501d030472d254f81f9f58f13518450e Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Wed, 16 Sep 2026 15:42:36 +0200 Subject: [PATCH 3/7] fix(cluster-version): address errcheck lint findings in test Signed-off-by: onuryilmaz --- cmd/cluster-version_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/cluster-version_test.go b/cmd/cluster-version_test.go index 786ddb5..78a5e9a 100644 --- a/cmd/cluster-version_test.go +++ b/cmd/cluster-version_test.go @@ -245,9 +245,9 @@ func TestGetVersionFromLabel_BadKubeconfig(t *testing.T) { // A kubeconfig with invalid YAML should cause getVersionFromLabel to return an error. f, err := os.CreateTemp("", "bad-kube-*.yaml") g.Expect(err).ToNot(HaveOccurred()) - defer os.Remove(f.Name()) + defer func() { _ = os.Remove(f.Name()) }() _, _ = f.WriteString("invalid yaml: [") - f.Close() + g.Expect(f.Close()).To(Succeed()) ver, err := getVersionFromLabel(context.Background(), f.Name(), "", "my-org", "prod-eu") g.Expect(err).To(HaveOccurred()) From 4c2b87a5af8edc0386e6f1de233d2c6bfb48d822 Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Wed, 16 Sep 2026 16:04:27 +0200 Subject: [PATCH 4/7] fix(cluster-version): address three review findings - Use cmd.Flags().GetString() for Greenhouse flags instead of viper to avoid key collisions with sync.go's identically-named viper bindings - Give the label lookup its own context with half the total timeout so the live-query fallback always has a meaningful deadline if Greenhouse is slow to respond - Propagate non-NotFound errors from versionLabelFromClient via client.IgnoreNotFound so RBAC/network/timeout failures are observable at debug level rather than silently falling through Signed-off-by: onuryilmaz --- cmd/cluster-version.go | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/cmd/cluster-version.go b/cmd/cluster-version.go index ad2d810..72d8b2a 100644 --- a/cmd/cluster-version.go +++ b/cmd/cluster-version.go @@ -79,13 +79,16 @@ func runClusterVersion(cmd *cobra.Command, args []string) error { return fmt.Errorf("--kubeconfig must not be empty") } - cvGreenhouseKubeconfig = resolveKubeconfig("greenhouse-cluster-kubeconfig", viper.GetString("greenhouse-cluster-kubeconfig")) - if viper.IsSet("greenhouse-cluster-kubeconfig") && cvGreenhouseKubeconfig == "" { + // Read Greenhouse flags directly from the cobra flag set to avoid Viper key + // collisions with the identically-named flags registered by sync.go. + cvGreenhouseKubeconfig, _ = cmd.Flags().GetString("greenhouse-cluster-kubeconfig") + cvGreenhouseKubeconfig = resolveKubeconfig("greenhouse-cluster-kubeconfig", cvGreenhouseKubeconfig) + if cmd.Flags().Changed("greenhouse-cluster-kubeconfig") && cvGreenhouseKubeconfig == "" { return fmt.Errorf("--greenhouse-cluster-kubeconfig must not be empty") } - cvGreenhouseContext = viper.GetString("greenhouse-cluster-context") - cvGreenhouseNamespace = viper.GetString("greenhouse-cluster-namespace") - cvGreenhouseClusterName = viper.GetString("greenhouse-cluster-name") + cvGreenhouseContext, _ = cmd.Flags().GetString("greenhouse-cluster-context") + cvGreenhouseNamespace, _ = cmd.Flags().GetString("greenhouse-cluster-namespace") + cvGreenhouseClusterName, _ = cmd.Flags().GetString("greenhouse-cluster-name") timeoutStr := viper.GetString("timeout") timeout, err := time.ParseDuration(timeoutStr) @@ -136,8 +139,12 @@ func runClusterVersion(cmd *cobra.Command, args []string) error { var clusterVersion string // 1) Try reading version from the ClusterKubeconfig label on Greenhouse. + // Use half the total timeout so the live-query fallback always has a + // meaningful deadline even if the Greenhouse cluster is slow to respond. if cvGreenhouseNamespace != "" && cvGreenhouseClusterName != "" { - labelVer, labelErr := getVersionFromLabel(ctx, cvGreenhouseKubeconfig, cvGreenhouseContext, cvGreenhouseNamespace, cvGreenhouseClusterName) + labelCtx, labelCancel := context.WithTimeout(cmd.Context(), timeout/2) + labelVer, labelErr := getVersionFromLabel(labelCtx, cvGreenhouseKubeconfig, cvGreenhouseContext, cvGreenhouseNamespace, cvGreenhouseClusterName) + labelCancel() if labelErr != nil { slog.Debug("label-based version lookup failed, falling back to live query", "error", labelErr) } else if labelVer != "" { @@ -205,10 +212,12 @@ func getVersionFromLabel(ctx context.Context, greenhouseKubeconfig, greenhouseCo // versionLabelFromClient fetches the greenhouse.sap/kubernetes-version label // using an already-constructed client. Separated for testability. +// Returns ("", nil) only on not-found; other errors (RBAC, network, timeout) +// are propagated so the caller can log them and fall back to a live query. func versionLabelFromClient(ctx context.Context, c client.Client, namespace, clusterName string) (string, error) { var ckc v1alpha1.ClusterKubeconfig if err := c.Get(ctx, client.ObjectKey{Namespace: namespace, Name: clusterName}, &ckc); err != nil { - return "", nil + return "", client.IgnoreNotFound(err) } return ckc.Labels["greenhouse.sap/kubernetes-version"], nil } From e6881e5df43f6ba6060951542970c66be60f5d5c Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Thu, 17 Sep 2026 08:01:26 +0200 Subject: [PATCH 5/7] fix(cluster-version): use cmd.Flags().Changed for greenhouse kubeconfig resolution resolveKubeconfig internally calls viper.IsSet which can be polluted by sync.go's identical Viper key binding. Inline the same KUBECONFIG env-var fallback logic using cmd.Flags().Changed() so an explicit -g always wins regardless of what sync has bound to Viper. Signed-off-by: onuryilmaz --- cmd/cluster-version.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/cmd/cluster-version.go b/cmd/cluster-version.go index 72d8b2a..32324b8 100644 --- a/cmd/cluster-version.go +++ b/cmd/cluster-version.go @@ -81,10 +81,14 @@ func runClusterVersion(cmd *cobra.Command, args []string) error { // Read Greenhouse flags directly from the cobra flag set to avoid Viper key // collisions with the identically-named flags registered by sync.go. + // Use cmd.Flags().Changed() instead of viper.IsSet() for the same reason. cvGreenhouseKubeconfig, _ = cmd.Flags().GetString("greenhouse-cluster-kubeconfig") - cvGreenhouseKubeconfig = resolveKubeconfig("greenhouse-cluster-kubeconfig", cvGreenhouseKubeconfig) - if cmd.Flags().Changed("greenhouse-cluster-kubeconfig") && cvGreenhouseKubeconfig == "" { - return fmt.Errorf("--greenhouse-cluster-kubeconfig must not be empty") + if cmd.Flags().Changed("greenhouse-cluster-kubeconfig") { + if cvGreenhouseKubeconfig == "" { + return fmt.Errorf("--greenhouse-cluster-kubeconfig must not be empty") + } + } else if os.Getenv("KUBECONFIG") != "" { + cvGreenhouseKubeconfig = "" } cvGreenhouseContext, _ = cmd.Flags().GetString("greenhouse-cluster-context") cvGreenhouseNamespace, _ = cmd.Flags().GetString("greenhouse-cluster-namespace") From b1214269ff56422b6823d56bdad15af3b3d3f1d5 Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Thu, 17 Sep 2026 10:19:43 +0200 Subject: [PATCH 6/7] fix(cluster-version): restore env/config support and add command-level tests - Bind Greenhouse flags under cv.* viper keys so CLOUDCTL_CV_* env vars and .cloudctl.yaml config values work without colliding with sync.go's identically-named bindings; resolveKubeconfig is called with the cv.* key so KUBECONFIG env fallback still applies - Add clusterVersionLabelLookup function variable for test injection - Add two command-level integration tests: one verifies the label path short-circuits the live /version call; the other verifies a lookup error triggers the live-query fallback Signed-off-by: onuryilmaz --- cmd/cluster-version.go | 42 ++++++++------ cmd/cluster-version_test.go | 109 ++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 17 deletions(-) diff --git a/cmd/cluster-version.go b/cmd/cluster-version.go index 32324b8..02b4d79 100644 --- a/cmd/cluster-version.go +++ b/cmd/cluster-version.go @@ -79,20 +79,16 @@ func runClusterVersion(cmd *cobra.Command, args []string) error { return fmt.Errorf("--kubeconfig must not be empty") } - // Read Greenhouse flags directly from the cobra flag set to avoid Viper key - // collisions with the identically-named flags registered by sync.go. - // Use cmd.Flags().Changed() instead of viper.IsSet() for the same reason. - cvGreenhouseKubeconfig, _ = cmd.Flags().GetString("greenhouse-cluster-kubeconfig") - if cmd.Flags().Changed("greenhouse-cluster-kubeconfig") { - if cvGreenhouseKubeconfig == "" { - return fmt.Errorf("--greenhouse-cluster-kubeconfig must not be empty") - } - } else if os.Getenv("KUBECONFIG") != "" { - cvGreenhouseKubeconfig = "" + // Read Greenhouse flags from the cv.* viper keys, which are bound only to + // cluster-version's flags (not sync.go's), so env vars and config files + // work without colliding with sync's identically-named viper bindings. + cvGreenhouseKubeconfig = resolveKubeconfig("cv.greenhouse-cluster-kubeconfig", viper.GetString("cv.greenhouse-cluster-kubeconfig")) + if viper.IsSet("cv.greenhouse-cluster-kubeconfig") && cvGreenhouseKubeconfig == "" { + return fmt.Errorf("--greenhouse-cluster-kubeconfig must not be empty") } - cvGreenhouseContext, _ = cmd.Flags().GetString("greenhouse-cluster-context") - cvGreenhouseNamespace, _ = cmd.Flags().GetString("greenhouse-cluster-namespace") - cvGreenhouseClusterName, _ = cmd.Flags().GetString("greenhouse-cluster-name") + cvGreenhouseContext = viper.GetString("cv.greenhouse-cluster-context") + cvGreenhouseNamespace = viper.GetString("cv.greenhouse-cluster-namespace") + cvGreenhouseClusterName = viper.GetString("cv.greenhouse-cluster-name") timeoutStr := viper.GetString("timeout") timeout, err := time.ParseDuration(timeoutStr) @@ -147,7 +143,7 @@ func runClusterVersion(cmd *cobra.Command, args []string) error { // meaningful deadline even if the Greenhouse cluster is slow to respond. if cvGreenhouseNamespace != "" && cvGreenhouseClusterName != "" { labelCtx, labelCancel := context.WithTimeout(cmd.Context(), timeout/2) - labelVer, labelErr := getVersionFromLabel(labelCtx, cvGreenhouseKubeconfig, cvGreenhouseContext, cvGreenhouseNamespace, cvGreenhouseClusterName) + labelVer, labelErr := clusterVersionLabelLookup(labelCtx, cvGreenhouseKubeconfig, cvGreenhouseContext, cvGreenhouseNamespace, cvGreenhouseClusterName) labelCancel() if labelErr != nil { slog.Debug("label-based version lookup failed, falling back to live query", "error", labelErr) @@ -182,6 +178,10 @@ func runClusterVersion(cmd *cobra.Command, args []string) error { return printer.Print(output.ClusterVersionResult{Context: effectiveContext, Version: clusterVersion}) } +// clusterVersionLabelLookup is the function used to fetch the version label from +// Greenhouse. It is a variable so tests can substitute a fake implementation. +var clusterVersionLabelLookup = getVersionFromLabel + // normalizeVersion strips a leading "v", prerelease suffix, and build metadata // from a Kubernetes version string, returning a clean semver (e.g. "1.29.3"). func normalizeVersion(v string) string { @@ -346,8 +346,16 @@ func init() { clusterVersionCmd.Flags().StringVarP(&cvGreenhouseNamespace, "greenhouse-cluster-namespace", "n", "", "Greenhouse organization namespace") clusterVersionCmd.Flags().StringVar(&cvGreenhouseClusterName, "greenhouse-cluster-name", "", "ClusterKubeconfig resource name in Greenhouse to read the version label from") - // BindPFlags can theoretically return an error if called with `nil` as an argument - // which should never happen after at least one flag was defined. That's why the output - // there is ignored. + // Bind the shared flags (kubeconfig, context, timeout, output) to their standard viper keys. _ = viper.BindPFlags(clusterVersionCmd.Flags()) + + // Bind Greenhouse flags under a cv.* prefix so they do not collide with the + // identically-named flags registered by sync.go in the global viper instance. + // This lets CLOUDCTL_CV_GREENHOUSE_CLUSTER_* env vars and .cloudctl.yaml + // [cv] section override these flags without touching sync's bindings. + f := clusterVersionCmd.Flags() + _ = viper.BindPFlag("cv.greenhouse-cluster-kubeconfig", f.Lookup("greenhouse-cluster-kubeconfig")) + _ = viper.BindPFlag("cv.greenhouse-cluster-context", f.Lookup("greenhouse-cluster-context")) + _ = viper.BindPFlag("cv.greenhouse-cluster-namespace", f.Lookup("greenhouse-cluster-namespace")) + _ = viper.BindPFlag("cv.greenhouse-cluster-name", f.Lookup("greenhouse-cluster-name")) } diff --git a/cmd/cluster-version_test.go b/cmd/cluster-version_test.go index 78a5e9a..f58cf22 100644 --- a/cmd/cluster-version_test.go +++ b/cmd/cluster-version_test.go @@ -7,13 +7,17 @@ import ( "context" "crypto/tls" "encoding/json" + "fmt" "net/http" "net/http/httptest" "os" + "strings" "testing" "github.com/cloudoperators/greenhouse/api/v1alpha1" . "github.com/onsi/gomega" + "github.com/spf13/cobra" + "github.com/spf13/viper" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/version" @@ -253,3 +257,108 @@ func TestGetVersionFromLabel_BadKubeconfig(t *testing.T) { g.Expect(err).To(HaveOccurred()) g.Expect(ver).To(BeEmpty()) } + +// writeTLSKubeconfig writes a minimal kubeconfig that points at srv and returns its path. +// It uses insecure-skip-tls-verify so the test server's self-signed cert is accepted. +func writeTLSKubeconfig(t *testing.T, srv *httptest.Server) string { + t.Helper() + g := NewWithT(t) + + f, err := os.CreateTemp("", "kubeconfig-*.yaml") + g.Expect(err).ToNot(HaveOccurred()) + t.Cleanup(func() { _ = os.Remove(f.Name()) }) + + cfg := "apiVersion: v1\nkind: Config\nclusters:\n- cluster:\n server: " + srv.URL + "\n insecure-skip-tls-verify: true\n name: test\ncontexts:\n- context:\n cluster: test\n user: test\n name: test\ncurrent-context: test\nusers:\n- name: test\n user: {}\n" + _, err = f.WriteString(cfg) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(f.Close()).To(Succeed()) + return f.Name() +} + +// buildTestClusterVersionCmd returns a fresh cobra.Command wired to runClusterVersion +// with all flags registered, suitable for use in integration tests. +func buildTestClusterVersionCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "cluster-version", + SilenceUsage: true, + SilenceErrors: true, + RunE: runClusterVersion, + } + cmd.Flags().StringVarP(&kubeconfig, "kubeconfig", "k", clientcmd.RecommendedHomeFile, "") + cmd.Flags().StringVarP(&kubecontext, "context", "c", "", "") + cmd.Flags().String("timeout", "10s", "") + cmd.Flags().StringVarP(&cvGreenhouseKubeconfig, "greenhouse-cluster-kubeconfig", "g", clientcmd.RecommendedHomeFile, "") + cmd.Flags().StringVar(&cvGreenhouseContext, "greenhouse-cluster-context", "", "") + cmd.Flags().StringVarP(&cvGreenhouseNamespace, "greenhouse-cluster-namespace", "n", "", "") + cmd.Flags().StringVar(&cvGreenhouseClusterName, "greenhouse-cluster-name", "", "") + cmd.Flags().StringP("output", "o", "text", "") + _ = viper.BindPFlags(cmd.Flags()) + _ = viper.BindPFlag("cv.greenhouse-cluster-kubeconfig", cmd.Flags().Lookup("greenhouse-cluster-kubeconfig")) + _ = viper.BindPFlag("cv.greenhouse-cluster-context", cmd.Flags().Lookup("greenhouse-cluster-context")) + _ = viper.BindPFlag("cv.greenhouse-cluster-namespace", cmd.Flags().Lookup("greenhouse-cluster-namespace")) + _ = viper.BindPFlag("cv.greenhouse-cluster-name", cmd.Flags().Lookup("greenhouse-cluster-name")) + return cmd +} + +func TestRunClusterVersion_LabelPathShortCircuitsLiveQuery(t *testing.T) { + g := NewWithT(t) + + // Remote cluster server — must NOT be called when the label path succeeds. + liveCallCount := 0 + remoteSrv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + liveCallCount++ + t.Errorf("unexpected live /version call to remote cluster") + _ = json.NewEncoder(w).Encode(&version.Info{GitVersion: "v9.9.9"}) + })) + defer remoteSrv.Close() + + remoteKubeconfig := writeTLSKubeconfig(t, remoteSrv) + + // Inject a fake label lookup that returns a version without hitting any server. + original := clusterVersionLabelLookup + t.Cleanup(func() { clusterVersionLabelLookup = original }) + clusterVersionLabelLookup = func(_ context.Context, _, _, _, _ string) (string, error) { + return "v1.29.3", nil + } + + cmd := buildTestClusterVersionCmd() + cmd.SetArgs([]string{"--kubeconfig", remoteKubeconfig, "-n", "my-org", "--greenhouse-cluster-name", "prod-eu"}) + var out strings.Builder + cmd.SetOut(&out) + g.Expect(cmd.ExecuteContext(context.Background())).To(Succeed()) + g.Expect(out.String()).To(ContainSubstring("1.29.3")) + g.Expect(liveCallCount).To(Equal(0), "live /version should not have been called") +} + +func TestRunClusterVersion_LabelErrorFallsBackToLiveQuery(t *testing.T) { + g := NewWithT(t) + + // Remote cluster server — must be called as fallback when label lookup fails. + liveCallCount := 0 + remoteSrv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/version" { + liveCallCount++ + _ = json.NewEncoder(w).Encode(&version.Info{GitVersion: "v1.30.0"}) + } else { + http.NotFound(w, r) + } + })) + defer remoteSrv.Close() + + remoteKubeconfig := writeTLSKubeconfig(t, remoteSrv) + + // Inject a fake label lookup that always returns an error. + original := clusterVersionLabelLookup + t.Cleanup(func() { clusterVersionLabelLookup = original }) + clusterVersionLabelLookup = func(_ context.Context, _, _, _, _ string) (string, error) { + return "", fmt.Errorf("greenhouse unavailable") + } + + cmd := buildTestClusterVersionCmd() + cmd.SetArgs([]string{"--kubeconfig", remoteKubeconfig, "-n", "my-org", "--greenhouse-cluster-name", "prod-eu"}) + var out strings.Builder + cmd.SetOut(&out) + g.Expect(cmd.ExecuteContext(context.Background())).To(Succeed()) + g.Expect(out.String()).To(ContainSubstring("1.30.0")) + g.Expect(liveCallCount).To(BeNumerically(">=", 1), "live /version should have been called as fallback") +} From 58ec4047c55d985d0a9c87d7efb658bec9fab819 Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Thu, 17 Sep 2026 10:48:01 +0200 Subject: [PATCH 7/7] fix(cluster-version): use hyphen prefix for cv- viper keys The dot in cv.* viper keys was not handled by the global SetEnvKeyReplacer("-","_"), so CLOUDCTL_CV_GREENHOUSE_CLUSTER_NAMESPACE was silently ignored. Switching to cv-* (hyphen) means the replacer translates the env var name correctly. Signed-off-by: onuryilmaz --- cmd/cluster-version.go | 28 +++++++++++++++------------- cmd/cluster-version_test.go | 8 ++++---- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/cmd/cluster-version.go b/cmd/cluster-version.go index 02b4d79..5faa11d 100644 --- a/cmd/cluster-version.go +++ b/cmd/cluster-version.go @@ -79,16 +79,18 @@ func runClusterVersion(cmd *cobra.Command, args []string) error { return fmt.Errorf("--kubeconfig must not be empty") } - // Read Greenhouse flags from the cv.* viper keys, which are bound only to + // Read Greenhouse flags from the cv-* viper keys, which are bound only to // cluster-version's flags (not sync.go's), so env vars and config files // work without colliding with sync's identically-named viper bindings. - cvGreenhouseKubeconfig = resolveKubeconfig("cv.greenhouse-cluster-kubeconfig", viper.GetString("cv.greenhouse-cluster-kubeconfig")) - if viper.IsSet("cv.greenhouse-cluster-kubeconfig") && cvGreenhouseKubeconfig == "" { + // Using a hyphen separator (cv-*) rather than a dot ensures the global + // SetEnvKeyReplacer("-","_") maps CLOUDCTL_CV_GREENHOUSE_* correctly. + cvGreenhouseKubeconfig = resolveKubeconfig("cv-greenhouse-cluster-kubeconfig", viper.GetString("cv-greenhouse-cluster-kubeconfig")) + if viper.IsSet("cv-greenhouse-cluster-kubeconfig") && cvGreenhouseKubeconfig == "" { return fmt.Errorf("--greenhouse-cluster-kubeconfig must not be empty") } - cvGreenhouseContext = viper.GetString("cv.greenhouse-cluster-context") - cvGreenhouseNamespace = viper.GetString("cv.greenhouse-cluster-namespace") - cvGreenhouseClusterName = viper.GetString("cv.greenhouse-cluster-name") + cvGreenhouseContext = viper.GetString("cv-greenhouse-cluster-context") + cvGreenhouseNamespace = viper.GetString("cv-greenhouse-cluster-namespace") + cvGreenhouseClusterName = viper.GetString("cv-greenhouse-cluster-name") timeoutStr := viper.GetString("timeout") timeout, err := time.ParseDuration(timeoutStr) @@ -349,13 +351,13 @@ func init() { // Bind the shared flags (kubeconfig, context, timeout, output) to their standard viper keys. _ = viper.BindPFlags(clusterVersionCmd.Flags()) - // Bind Greenhouse flags under a cv.* prefix so they do not collide with the + // Bind Greenhouse flags under a cv-* prefix so they do not collide with the // identically-named flags registered by sync.go in the global viper instance. - // This lets CLOUDCTL_CV_GREENHOUSE_CLUSTER_* env vars and .cloudctl.yaml - // [cv] section override these flags without touching sync's bindings. + // Using a hyphen separator (not dot) ensures SetEnvKeyReplacer("-","_") maps + // CLOUDCTL_CV_GREENHOUSE_CLUSTER_* env vars to these keys correctly. f := clusterVersionCmd.Flags() - _ = viper.BindPFlag("cv.greenhouse-cluster-kubeconfig", f.Lookup("greenhouse-cluster-kubeconfig")) - _ = viper.BindPFlag("cv.greenhouse-cluster-context", f.Lookup("greenhouse-cluster-context")) - _ = viper.BindPFlag("cv.greenhouse-cluster-namespace", f.Lookup("greenhouse-cluster-namespace")) - _ = viper.BindPFlag("cv.greenhouse-cluster-name", f.Lookup("greenhouse-cluster-name")) + _ = viper.BindPFlag("cv-greenhouse-cluster-kubeconfig", f.Lookup("greenhouse-cluster-kubeconfig")) + _ = viper.BindPFlag("cv-greenhouse-cluster-context", f.Lookup("greenhouse-cluster-context")) + _ = viper.BindPFlag("cv-greenhouse-cluster-namespace", f.Lookup("greenhouse-cluster-namespace")) + _ = viper.BindPFlag("cv-greenhouse-cluster-name", f.Lookup("greenhouse-cluster-name")) } diff --git a/cmd/cluster-version_test.go b/cmd/cluster-version_test.go index f58cf22..a57fee3 100644 --- a/cmd/cluster-version_test.go +++ b/cmd/cluster-version_test.go @@ -293,10 +293,10 @@ func buildTestClusterVersionCmd() *cobra.Command { cmd.Flags().StringVar(&cvGreenhouseClusterName, "greenhouse-cluster-name", "", "") cmd.Flags().StringP("output", "o", "text", "") _ = viper.BindPFlags(cmd.Flags()) - _ = viper.BindPFlag("cv.greenhouse-cluster-kubeconfig", cmd.Flags().Lookup("greenhouse-cluster-kubeconfig")) - _ = viper.BindPFlag("cv.greenhouse-cluster-context", cmd.Flags().Lookup("greenhouse-cluster-context")) - _ = viper.BindPFlag("cv.greenhouse-cluster-namespace", cmd.Flags().Lookup("greenhouse-cluster-namespace")) - _ = viper.BindPFlag("cv.greenhouse-cluster-name", cmd.Flags().Lookup("greenhouse-cluster-name")) + _ = viper.BindPFlag("cv-greenhouse-cluster-kubeconfig", cmd.Flags().Lookup("greenhouse-cluster-kubeconfig")) + _ = viper.BindPFlag("cv-greenhouse-cluster-context", cmd.Flags().Lookup("greenhouse-cluster-context")) + _ = viper.BindPFlag("cv-greenhouse-cluster-namespace", cmd.Flags().Lookup("greenhouse-cluster-namespace")) + _ = viper.BindPFlag("cv-greenhouse-cluster-name", cmd.Flags().Lookup("greenhouse-cluster-name")) return cmd }