Check kubectl versions before attempting dashboard #53

pcalcado · 2017-12-15T08:21:59Z

Different versions of kubectl have different outputs for the $ kubectl proxy command, which sometimes causes $ conduit dashboard to hang. This adds a version check that will make sure that the client is at least at $version, currently set to 1.8.4, before attempting anything.

There's also a refactoring extracting business logic from the UI layer and adding lots of tests for it.

e.g. output:

 $   go build -o ~/tmp/conduit  cli/main.go  && ~/tmp/conduit dashboard
2017/12/15 23:45:02 Failed to start kubectl: Kubectl is on version [1 4 4], but version [1 8 4] or more recent is required

briansmith · 2017-12-15T22:44:10Z

This seems better than what we currently have. However, it seems like it's still really brittle to me. We're still relying on kubectl being in $PATH and we're going to be sensitive to the whims of the kubectl developers changing the output format of kubectl going forward. It seems much better to me to do the proxying ourselves instead of shelling out to kubectl, especially since we could, AFAICT, just copy/paste the proxying code directly from kubectl, which seems to be about the same size as this PR. However, I won't object to this going in as is if Kevin feels like this is OK. I'll let him make the approval call.

pcalcado · 2017-12-16T12:12:14Z

@briansmith Thanks.

Changing the underlying architecture wasn't the goal of the PR, the refactoring here means to extract logic spread in the UI and make it less brittle by both tests and enhanced error handling. Also, I am not sure I agree that a production-ready and we'll tested implementation would be similar in size to this PR.

klingerf

Nice refactor! And the tests are great. I just had a few pieces of misc feedback below. I agree that eventually replacing our use of kubectl with the client-go library would be a good direction in the future.

klingerf · 2017-12-18T21:54:10Z

cli/shell/kubectl.go

+	magicCharacterThatIndicatesProxyIsRunning = '\n'
+)
+
+var minimunKubectlVersionExpected = [3]int{1, 8, 4}


I'd like to set our minimum at 1.8.0. The magic character that we're dependent on was added here:

kubernetes/kubernetes@0daee3a#diff-595bfea7ed0dd0171e1f339a1f8bfcb6R155

And that commit made it into the 1.8.0 release.

klingerf · 2017-12-18T21:57:05Z

cli/shell/kubectl.go

+
+	if err != nil {
+		return version, err
+	}


You have two err != nil checks in a row here, and it's impossible for the second one to fire. I'd remove it. And I think you should wait to assign versionString until after the error check happens, i.e move line 39 to line 42.

klingerf · 2017-12-18T21:59:55Z

cli/shell/kubectl.go

+	bytes, err := kctl.sh.CombinedOutput("kubectl", "version", "--client", "--short")
+	versionString := string(bytes)
+	if err != nil {
+		return [3]int{}, fmt.Errorf("Error running kubectl Version. Output: %s Error: %v", versionString, err)


For consistency with the rest of your error checks here, I think you should return the existing version array, rather than instantiating a new one.

return version, fmt.Errorf("Error running kubectl Version. Output: %s Error: %v", versionString, err)

klingerf · 2017-12-18T22:01:04Z

cli/shell/kubectl.go

+
+	fmt.Println(kubectlOutput)
+	if err != nil {
+		return fmt.Errorf("Error waiting for kubectl to start the proxy. kubetl returned [%s], error: %v", kubectlOutput, err)


Typo here: kubetl => kubectl

klingerf · 2017-12-18T22:05:46Z

cli/shell/kubectl.go

+	}
+
+	if !isCompatibleVersion(minimunKubectlVersionExpected, actualVersion) {
+		return nil, fmt.Errorf("Kubectl is on version %v, but version %v or more recent is required", actualVersion, minimunKubectlVersionExpected)


TIOLI -- you could clean this up a bit if you added a helper to convert int[3] to version string. otherwise, as written, you'll end up with:

"Kubectl is on version [1 2 3], but version [2 3 4] or more recent is required"

pcalcado · 2017-12-19T00:33:02Z

@klingerf Thanks so much for the thorough read!

I've changed all the points. wrt embedding kubectl, I think that it is a generally good direction but there are other things to consider (e.g. are we going to have our embedded version different from the kubectl that the use has installed in their computer, are we committed to releasing a new build of CLI every time there is a new kubectl, what about the current architecture change going on with kubectl—whould we wait for it to be finished?) but irrespective of when/if we do it the first step should be isolating kubectl and kubeapi code in its own module and objects so that we can change the implementation without having to change client code.

klingerf

⭐️ Ok, awesome, thanks for making those updates! And I agree that moving this logic to the shell package makes sense, regardless of which direction we decide to go with interacting with kubectl going forward.

Following on linkerd#52, this change moves the `telemetry::metrics::tls_config_reload` module to `telemetry::tls_config_reload`. The `Fmt` type has been renamed to `Report`. Furthermore, two helper methods have been added to `metrics::Scopes` so that its internal representation need not be made public.

pcalcado force-pushed the phil/dashboard-should-work-older-versions branch from 51a2c1a to 7529a97 Compare December 15, 2017 12:47

pcalcado changed the title ~~WIP DO NOT MERGE Check kubectl versions before attempting anything~~ Check kubectl versions before attempting dashboard Dec 15, 2017

pcalcado requested review from klingerf and briansmith December 15, 2017 12:50

pcalcado self-assigned this Dec 15, 2017

pcalcado added the area/cli label Dec 15, 2017

klingerf reviewed Dec 18, 2017

View reviewed changes

Phil Calcado added 7 commits December 19, 2017 11:18

Introduce objects to manage kubectl and shell

07cf5f4

Make dashboard command use new shell & kubectl objects

7d261c7

Make compatible with version numbers like v1.9.0-beta.1

66a0c3d

Add version check for kubectl

41f4e9d

Refactor error to use proper method from fmt pa ckage

d2e0d0f

Make channel and error handling more idiomatic and safe

21ed875

Make version require 1.8.0

0f9055a

pcalcado force-pushed the phil/dashboard-should-work-older-versions branch from 694227c to 0f9055a Compare December 19, 2017 00:28

klingerf approved these changes Dec 19, 2017

View reviewed changes

pcalcado merged commit 6f9e8be into master Dec 19, 2017

pcalcado deleted the phil/dashboard-should-work-older-versions branch February 5, 2018 22:21

klingerf mentioned this pull request Feb 20, 2018

conduit dashboard does not use the port provided to open the dashboard url when -p is used #359

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check kubectl versions before attempting dashboard #53

Check kubectl versions before attempting dashboard #53

pcalcado commented Dec 15, 2017 •

edited

Loading

briansmith commented Dec 15, 2017

pcalcado commented Dec 16, 2017

klingerf left a comment

klingerf Dec 18, 2017

klingerf Dec 18, 2017

klingerf Dec 18, 2017

klingerf Dec 18, 2017

klingerf Dec 18, 2017

pcalcado commented Dec 19, 2017

klingerf left a comment

Check kubectl versions before attempting dashboard #53

Check kubectl versions before attempting dashboard #53

Conversation

pcalcado commented Dec 15, 2017 • edited Loading

briansmith commented Dec 15, 2017

pcalcado commented Dec 16, 2017

klingerf left a comment

Choose a reason for hiding this comment

klingerf Dec 18, 2017

Choose a reason for hiding this comment

klingerf Dec 18, 2017

Choose a reason for hiding this comment

klingerf Dec 18, 2017

Choose a reason for hiding this comment

klingerf Dec 18, 2017

Choose a reason for hiding this comment

klingerf Dec 18, 2017

Choose a reason for hiding this comment

pcalcado commented Dec 19, 2017

klingerf left a comment

Choose a reason for hiding this comment

pcalcado commented Dec 15, 2017 •

edited

Loading