Fix gitops check

The `--short` flag has been removed from `kubectl version` in 1.28
(https://github.com/kubernetes/kubernetes/blob/7fe31be11fbe9b44af262d5f5cffb1e73648aa96/CHANGELOG/CHANGELOG-1.28.md#L1718)
so the command obviously fails now.

This commit changes the behaviour of the `gitops check` command to
create a client-go DiscoveryClient and use that to retrieve the server
version. That way we don't have to rely on forking a `kubectl` process
and the output being consistent.

The code is now much cleaner, easier to read and properly tested.

closes #4157

Signed-off-by: Max Jonas Werner <mail@makk.es>

cmd/gitops/check/cmd.go

@@ -4,35 +4,52 @@ import (
 	"fmt"
 
 	"github.com/weaveworks/weave-gitops/cmd/gitops/check/oidcconfig"
+	"github.com/weaveworks/weave-gitops/cmd/gitops/cmderrors"
 	"github.com/weaveworks/weave-gitops/cmd/gitops/config"
+	"github.com/weaveworks/weave-gitops/pkg/run"
 	"github.com/weaveworks/weave-gitops/pkg/services/check"
+	"k8s.io/cli-runtime/pkg/genericclioptions"
+	"k8s.io/client-go/discovery"
 
 	"github.com/spf13/cobra"
 )
 
 func GetCommand(opts *config.Options) *cobra.Command {
+
+	var kubeConfigArgs *genericclioptions.ConfigFlags
+
 	cmd := &cobra.Command{
 		Use:   "check",
 		Short: "Validates flux compatibility",
 		Example: `
 # Validate flux and kubernetes compatibility
 gitops check
 `,
-		RunE: runCmd,
+		RunE: func(cmd *cobra.Command, args []string) error {
+			kubeConfigArgs = run.GetKubeConfigArgs()
+			kubeConfigArgs.AddFlags(cmd.Flags())
+
+			cfg, err := kubeConfigArgs.ToRESTConfig()
+			if err != nil {
+				return err
+			}
+
+			c, err := discovery.NewDiscoveryClientForConfig(cfg)
+			if err != nil {
+				return cmderrors.ErrGetKubeClient
+			}
+			output, err := check.KubernetesVersion(c)
+			if err != nil {
+				return err
+			}
+
+			fmt.Println(output)
+
+			return nil
+		},
 	}
 
 	cmd.AddCommand(oidcconfig.OIDCConfigCommand(opts))
 
 	return cmd
 }
-
-func runCmd(_ *cobra.Command, _ []string) error {
-	output, err := check.Pre()
-	if err != nil {
-		return err
-	}
-
-	fmt.Println(output)
-
-	return nil
-}

pkg/services/check/check.go

@@ -2,71 +2,33 @@ package check
 
 import (
 	"fmt"
-	"os/exec"
-	"strings"
 
 	"github.com/Masterminds/semver/v3"
+	"k8s.io/client-go/discovery"
 )
 
 const (
 	kubernetesConstraints = ">=1.20.6-0"
 )
 
-// Pre runs pre-install checks
-func Pre() (string, error) {
-	k8sOutput, err := runKubernetesCheck()
+// KubernetesVersion checks if the Kubernetes version of the client is recent enough and
+// returns a proper string explaining the result of the check. An error is returned
+// if the check could not be performed (e.g. the cluster is not reachable).
+func KubernetesVersion(c discovery.DiscoveryInterface) (string, error) {
+	v, err := c.ServerVersion()
 	if err != nil {
-		return "", err
+		return "", fmt.Errorf("failed getting server version: %w", err)
 	}
 
-	return k8sOutput, nil
-}
-
-func runKubernetesCheck() (string, error) {
-	versionOutput, err := exec.Command("kubectl", "version", "--short").CombinedOutput()
-	if err != nil {
-		return "", fmt.Errorf("unable to get kubernetes version: %w", err)
-	}
-
-	v, err := parseVersion(string(versionOutput))
+	sv, err := semver.NewVersion(v.GitVersion)
 	if err != nil {
-		return "", fmt.Errorf("kubernetes version can't be determined: %w", err)
-	}
-
-	return checkKubernetesVersion(v)
-}
-
-func checkKubernetesVersion(version *semver.Version) (string, error) {
-	var valid bool
-
-	var vrange string
-
-	c, _ := semver.NewConstraint(kubernetesConstraints)
-	if c.Check(version) {
-		valid = true
-		vrange = kubernetesConstraints
-	}
-
-	if !valid {
-		return "", fmt.Errorf("✗ kubernetes version %s does not match %s", version.Original(), kubernetesConstraints)
-	}
-
-	return fmt.Sprintf("✔ Kubernetes %s %s", version.String(), vrange), nil
-}
-
-func parseVersion(text string) (*semver.Version, error) {
-	version := ""
-	lines := strings.Split(text, "\n")
-
-	for _, line := range lines {
-		if strings.Contains(line, "Server") {
-			version = strings.Replace(line, "Server Version: v", "", 1)
-		}
+		return "", fmt.Errorf("failed parsing server version %q: %w", v.GitVersion, err)
 	}
 
-	if _, err := semver.StrictNewVersion(version); err != nil {
-		return nil, err
+	cons, _ := semver.NewConstraint(kubernetesConstraints)
+	if !cons.Check(sv) {
+		return "", fmt.Errorf("✗ kubernetes version %s does not match %s", sv.Original(), kubernetesConstraints)
 	}
 
-	return semver.NewVersion(version)
+	return fmt.Sprintf("✔ Kubernetes %s %s", sv.String(), kubernetesConstraints), nil
 }

pkg/services/check/check_test.go

@@ -1,40 +1,78 @@
-package check
+package check_test
 
 import (
-	"github.com/Masterminds/semver/v3"
+	"errors"
+	"testing"
+
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/version"
+	fakediscovery "k8s.io/client-go/discovery/fake"
+	fakeclientset "k8s.io/client-go/kubernetes/fake"
+	kubetesting "k8s.io/client-go/testing"
 
-	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"github.com/weaveworks/weave-gitops/pkg/services/check"
 )
 
-var _ = Describe("Check kubernetes version", func() {
-	It("should show kuberetes is a valid version", func() {
-		version, err := semver.NewVersion("1.21.1")
-		Expect(err).ShouldNot(HaveOccurred())
-
-		output, err := checkKubernetesVersion(version)
-		Expect(err).ShouldNot(HaveOccurred())
+func TestKubernetesVersionWithError(t *testing.T) {
+	g := NewWithT(t)
 
-		Expect(output).To(Equal("✔ Kubernetes 1.21.1 >=1.20.6-0"))
+	expectedError := errors.New("an error occurred")
+	fakeClient := fakeclientset.NewSimpleClientset()
+	fakeClient.Discovery().(*fakediscovery.FakeDiscovery).PrependReactor("*", "*", func(action kubetesting.Action) (handled bool, ret runtime.Object, err error) {
+		return true, nil, expectedError
 	})
 
-	It("should fail with version does not match", func() {
-		version, err := semver.NewVersion("1.19.1")
-		Expect(err).ShouldNot(HaveOccurred())
+	res, err := check.KubernetesVersion(fakeClient.Discovery())
 
-		_, err = checkKubernetesVersion(version)
-		Expect(err.Error()).Should(Equal("✗ kubernetes version 1.19.1 does not match >=1.20.6-0"))
-	})
-})
+	g.Expect(err).To(MatchError(ContainSubstring(expectedError.Error())))
+	g.Expect(res).To(BeEmpty())
+}
 
-var _ = Describe("parse version", func() {
-	It("should parse version", func() {
-		expectedVersion, err := semver.NewVersion("1.21.1")
-		Expect(err).ShouldNot(HaveOccurred())
+func TestKubernetesVersion(t *testing.T) {
+	tests := []struct {
+		name          string
+		serverVersion string
+		expectedErr   string
+		expectedRes   string
+	}{
+		{
+			name:          "server version satisfies constraint",
+			serverVersion: "v1.28.4",
+			expectedRes:   `✔ Kubernetes 1.28.4 >=1.20.6-0`,
+		},
+		{
+			name:          "server version too low",
+			serverVersion: "v1.20.5",
+			expectedErr:   `✗ kubernetes version v1\.20\.5 does not match >=1\.20\.6-0`,
+		},
+		{
+			name:          "server version not semver compliant",
+			serverVersion: "1.x",
+			expectedErr:   `"1.x".*Invalid Semantic Version`,
+		},
+	}
 
-		output, err := parseVersion("Server Version: v1.21.1")
-		Expect(err).ShouldNot(HaveOccurred())
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+			client := fakeclientset.NewSimpleClientset()
+			fakeDiscovery, ok := client.Discovery().(*fakediscovery.FakeDiscovery)
+			if !ok {
+				t.Fatalf("couldn't convert Discovery() to *FakeDiscovery")
+			}
 
-		Expect(output).To(Equal(expectedVersion))
-	})
-})
+			fakeDiscovery.FakedServerVersion = &version.Info{
+				GitVersion: tt.serverVersion,
+			}
+
+			res, err := check.KubernetesVersion(client.Discovery())
+
+			if tt.expectedErr != "" {
+				g.Expect(err).To(MatchError(MatchRegexp(tt.expectedErr)))
+			}
+
+			g.Expect(res).To(Equal(tt.expectedRes))
+		})
+	}
+}