From a44229ef26b377bf8271016fea34112f178f6cc4 Mon Sep 17 00:00:00 2001 From: Max Jonas Werner Date: Wed, 6 Dec 2023 12:11:18 +0100 Subject: [PATCH] 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 --- cmd/gitops/check/cmd.go | 41 ++++++++---- pkg/services/check/check.go | 64 ++++-------------- pkg/services/check/check_suite_test.go | 13 ---- pkg/services/check/check_test.go | 92 ++++++++++++++++++-------- 4 files changed, 107 insertions(+), 103 deletions(-) delete mode 100644 pkg/services/check/check_suite_test.go diff --git a/cmd/gitops/check/cmd.go b/cmd/gitops/check/cmd.go index 601b6d7d155..ead3e75c6f6 100644 --- a/cmd/gitops/check/cmd.go +++ b/cmd/gitops/check/cmd.go @@ -4,13 +4,20 @@ 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", @@ -18,21 +25,31 @@ func GetCommand(opts *config.Options) *cobra.Command { # 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 -} diff --git a/pkg/services/check/check.go b/pkg/services/check/check.go index 367226171cd..f7df10f1375 100644 --- a/pkg/services/check/check.go +++ b/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 } diff --git a/pkg/services/check/check_suite_test.go b/pkg/services/check/check_suite_test.go deleted file mode 100644 index c58229d1776..00000000000 --- a/pkg/services/check/check_suite_test.go +++ /dev/null @@ -1,13 +0,0 @@ -package check - -import ( - "testing" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -func TestCheck(t *testing.T) { - RegisterFailHandler(Fail) - RunSpecs(t, "Check Suite") -} diff --git a/pkg/services/check/check_test.go b/pkg/services/check/check_test.go index 2575a122b5f..d2ea0a0fa72 100644 --- a/pkg/services/check/check_test.go +++ b/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)) + }) + } +}