use client-go to port-forward to dashboard #76

prydonius · 2017-12-01T14:37:36Z

Now that we have updated to client-go v4.0.0, we can use the library to
port-forward to the ingress controller.

This implementation is based on what kubectl is doing (pre-spdy package). See https://github.com/kubernetes/kubernetes/blob/2612e0c78ad18ac87bbd200d547100cf99f36089/pkg/kubectl/cmd/portforward.go#L104

fixes #64

Now that we have updated to client-go v4.0.0, we can use the library to port-forward to the ingress controller.

prydonius · 2017-12-01T14:38:30Z

glide.yaml

+- package: github.com/emicklei/go-restful-swagger12
+  version: dcef7f55730566d41eae5db10e7d6981829720f6
+- package: github.com/go-openapi/spec
+  version: 6aced65f8501fe1217321abf0749d354824ba2ff


I had to lock these as the new versions are causing compilation errors

prydonius · 2017-12-01T14:39:26Z

cmd/dashboard.go

+	// Open the Dashboard in a browser when the port-forward is established
+	go func() {
+		<-readyChannel
+		openInBrowser(fmt.Sprintf("http://localhost:%d", localPort))


we now open the dashboard in the browser after the port-forward connection is established, so this fixes #64

prydonius · 2017-12-01T14:40:43Z

cmd/dashboard.go

+		return nil, err
+	}
+	config.APIPath = "/api"
+	config.GroupVersion = &v1.SchemeGroupVersion


We need to specifically set APIPath and GroupVersion otherwise the RestClient we get from this won't build the correct URL to port-forward.

prydonius · 2017-12-01T14:41:56Z

cmd/dashboard.go

 }

 func openInBrowser(url string) error {
+	fmt.Printf("Opening %s in your default browser...\n", url)


Thought this might be useful in case it doesn't open in the browser, the user can see what URL to go to.

andresmgot

Overall it LGTM, I left a few minor comments/questions. We could add some unit tests to cover the newPortforwarder (just to check that there are not any obvious error) but all this feature is calling for an end-to-end test at some point.

andresmgot · 2017-12-01T14:58:47Z

cmd/dashboard.go

+}
+
+func portforwardReqURL(config *rest.Config, podName string) (*url.URL, error) {
+	restClient, err := rest.RESTClientFor(config)


the restClient should be a parameter to make this function testable (even if you don't test it now). As a general rule, any function that creates a client cannot be tested.

+1 will change

andresmgot · 2017-12-01T15:01:03Z

cmd/dashboard.go

+	return portforward.New(dialer, ports, stopChannel, readyChannel, os.Stdout, os.Stderr)
+}
+
+func restClientConfig() (*rest.Config, error) {


you should probably move this function to the root file

This is a very specific client config just for building the port-forward URI, so I think it should stay here.

mm, I saw it generic, what is the specific part? maybe the config.NegotiatedSerializer? If that is the case I would specify somewhere that this restClient is specific for doing port-forward (apart from defining that in this file) so other people don't misuse it. (Since this function is available for the whole cmd package)

It's not generic, we're specifying the api path and group version so we get URLs prefixed with api/v1. I don't think we need to use this anywhere else do we? We should typically use the discovery API.

I think this looks like a regular v1 config. Can we just use clientset.CoreV1().RESTClient() (or whatever the correct function is) below?

Ah, so I wanted to, since this is actually what kubectl uses also, but @ngtuna suggested not using it in #41 (comment) to reduce the dependency. However, now that I think about it, I don't think this would be pulling in many more dependencies, it uses the same underlying stuff. @ngtuna what do you think about using clientset for both this and checking if the nginx-ingress pod is there too?

Yah I am seeing no problem now for using clientset as we generally used it in several functions. What I was saying meant that if we can do stuffs easily with the current-in-used resourceClient then we should keep using it

andresmgot · 2017-12-01T15:01:30Z

cmd/dashboard.go

+	}
+	config.APIPath = "/api"
+	config.GroupVersion = &v1.SchemeGroupVersion
+	config.NegotiatedSerializer = serializer.DirectCodecFactory{CodecFactory: scheme.Codecs}


I am curious, what is the goal of this?

No idea, it's a required field by RESTClientFor

It's necessary to get the apimachinary code to do the correct serialisation/reserialising of objects.

andresmgot · 2017-12-01T15:05:15Z

cmd/dashboard.go

-	return syscall.Exec(cmd, args, env)
+	req := restClient.Post().
+		Resource("pods").
+		Namespace(ingressNamespace).


the "ingressNamespace" seems confusing, I know that it points to kubeapps but it would be probably better if you make that var a parameter and call it podNamespace

How about ingressPodNamespace?

you would be unnecessarily limiting the scope of the function to be used with ingress pods, isn't it? As far as I can see you can use this to obtain the URL for any kind of pod.

I feel like we're trying to generalise this for no reason. Seems like a pre-optimisation, what other port-forward calls do we expect to make?

probably not, it is a generic suggestion

OK, unless you feel strongly about it, I think we can leave as-is. If we want to use this in more places in the future we can move it out of this file and add a parameter. Sound good?

andresmgot · 2017-12-01T15:10:27Z

cmd/dashboard.go

+		return nil, err
+	}
+
+	ports := []string{fmt.Sprintf("%d:80", localPort)}


shouldn't be 80 obtained somehow?

Maybe, but given we know for sure it's going to be port 80, I think we can avoid the extra API call.

well, other people (like me) may see it that obvious. For example you also know for sure what URL the port forward will use but anyway you are obtaining it just in case it changes (and for the sake of readability)

I would argue the difference between building the port-forward request URL is that we control the ingress controller manifests. We don't control the Kubernetes API though, so that could easily change from version to version.

I think Andres is just suggesting a comment, which seems reasonable. We don't want a future reader to have to work this out for themselves.

yeah, I just didn't know where the 80 came from

sorry I misunderstood, I thought you meant parsing the Service object to get the port. Will add a comment.

yes, that's what I meant, but as far as it is clear where it comes from I am fine

prydonius · 2017-12-01T15:27:14Z

R.E. unit tests, indeed it is quite difficult, but we may be able to test some of these functions. An end-to-end test is needed, I agree.

anguslees

Lgtm overall, nice improvement.

anguslees · 2017-12-01T22:02:45Z

cmd/dashboard.go

+		return nil, err
+	}
+
+	ports := []string{fmt.Sprintf("%d:80", localPort)}


I think Andres is just suggesting a comment, which seems reasonable. We don't want a future reader to have to work this out for themselves.

anguslees · 2017-12-01T22:14:18Z

cmd/dashboard.go

+	}
+	config.APIPath = "/api"
+	config.GroupVersion = &v1.SchemeGroupVersion
+	config.NegotiatedSerializer = serializer.DirectCodecFactory{CodecFactory: scheme.Codecs}


It's necessary to get the apimachinary code to do the correct serialisation/reserialising of objects.

anguslees · 2017-12-01T22:17:49Z

cmd/dashboard.go

+	return portforward.New(dialer, ports, stopChannel, readyChannel, os.Stdout, os.Stderr)
+}
+
+func restClientConfig() (*rest.Config, error) {


I think this looks like a regular v1 config. Can we just use clientset.CoreV1().RESTClient() (or whatever the correct function is) below?

prydonius · 2017-12-04T15:24:01Z

@ngtuna @andresmgot PTAL!

ngtuna · 2017-12-05T04:26:31Z

@prydonius the last commit lgtm

andresmgot · 2017-12-05T08:42:48Z

cmd/root.go

@@ -183,3 +186,34 @@ func generateEncodedRandomPassword(s int) (string, error) {
 	pw := base64.URLEncoding.EncodeToString(b)
 	return base64.URLEncoding.EncodeToString([]byte(pw)), nil
 }
+
+func clientForGroupVersionKind(pool dynamic.ClientPool, disco discovery.DiscoveryInterface, gvk schema.GroupVersionKind, namespace string) (*dynamic.ResourceClient, error) {


seems that you are not using these functions any more, you can delete them

No this func is still being used by others

I moved it to root.go because they're being used, yep

- Added single instance view - Moved bindings list to single instance view

use client-go to port-forward to dashboard

77529ce

Now that we have updated to client-go v4.0.0, we can use the library to port-forward to the ingress controller.

prydonius commented Dec 1, 2017

View reviewed changes

andresmgot approved these changes Dec 1, 2017

View reviewed changes

anguslees reviewed Dec 1, 2017

View reviewed changes

ngtuna approved these changes Dec 4, 2017

View reviewed changes

move to using clientset in dashboard command

ed02b60

andresmgot reviewed Dec 5, 2017

View reviewed changes

prydonius merged commit d64770f into vmware-tanzu:master Dec 6, 2017

prydonius deleted the real-dashboard-command branch December 6, 2017 09:24

prydonius pushed a commit to prydonius/kubeapps that referenced this pull request Mar 5, 2018

Instance View (vmware-tanzu#76)

5ef0d1e

- Added single instance view - Moved bindings list to single instance view

use client-go to port-forward to dashboard #76

use client-go to port-forward to dashboard #76

Conversation

prydonius commented Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prydonius commented Dec 1, 2017

anguslees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prydonius commented Dec 4, 2017

ngtuna commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prydonius commented Dec 1, 2017 •

edited

Loading

andresmgot Dec 1, 2017 •

edited

Loading