From 0a9620458ff1dcc37f4e2272cfa4fcf7ba810adc Mon Sep 17 00:00:00 2001 From: Max Jonas Werner Date: Wed, 6 Dec 2023 15:03:05 +0100 Subject: [PATCH] Fix `gitops check` (#4158) * 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 * Bump supported K8s version The support policy of Weave GitOps is to "test Weave GitOps against the latest supported Kubernetes releases" which means that only 1.26, 1.27 and 1.28 are supported at this point. This change doesn't prevent Weave GitOps from being run on older versions of Kubernetes as the constraint is only used by the `gitops check` command which is purely informational. Signed-off-by: Max Jonas Werner --- cmd/gitops/check/cmd.go | 40 +++++++---- pkg/services/check/check.go | 66 ++++-------------- pkg/services/check/check_suite_test.go | 13 ---- pkg/services/check/check_test.go | 92 ++++++++++++++++++-------- 4 files changed, 107 insertions(+), 104 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 601b6d7d15..4819f26327 100644 --- a/cmd/gitops/check/cmd.go +++ b/cmd/gitops/check/cmd.go @@ -4,13 +4,19 @@ 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 +24,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 367226171c..87dcb70324 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" + kubernetesConstraints = ">=1.26" ) -// 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, 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 c58229d177..0000000000 --- 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 2575a122b5..1f7c11375d 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.`, + }, + { + name: "server version too low", + serverVersion: "v1.20.5", + expectedErr: `✗ kubernetes version v1\.20\.5 does not match >=1\.`, + }, + { + 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(MatchRegexp(tt.expectedRes)) + }) + } +}