Merge pull request #13756 from ckannon/fixes/13746

ServiceCmd restructuring to repair docker/port-forward issues

Author: medyagh

cmd/minikube/cmd/service.go

@@ -17,6 +17,7 @@ limitations under the License.
 package cmd
 
 import (
+	"bytes"
 	"errors"
 	"fmt"
 	"net/url"
@@ -102,10 +103,14 @@ var serviceCmd = &cobra.Command{
 			services = newServices
 		}
 
+		if services == nil || len(services) == 0 {
+			exit.Message(reason.SvcNotFound, `Service '{{.service}}' was not found in '{{.namespace}}' namespace.
+You may select another namespace by using 'minikube service {{.service}} -n <namespace>'. Or list out all the services using 'minikube service list'`, out.V{"service": args[0], "namespace": namespace})
+		}
+
 		var data [][]string
-		var openUrls []string
 		for _, svc := range services {
-			openUrls, err := service.WaitForService(co.API, co.Config.Name, namespace, svc.Name, serviceURLTemplate, true, https, wait, interval)
+			openUrls, err := service.WaitForService(co.API, co.Config.Name, namespace, svc.Name, serviceURLTemplate, serviceURLMode, https, wait, interval)
 
 			if err != nil {
 				var s *service.SVCNotFoundError
@@ -128,35 +133,21 @@ You may select another namespace by using 'minikube service {{.service}} -n <nam
 				}
 
 				data = append(data, []string{svc.Namespace, svc.Name, servicePortNames, serviceURLs})
-			}
-		}
 
-		if (!serviceURLMode && serviceURLFormat != defaultServiceFormatTemplate && !all) || all {
-			service.PrintServiceList(os.Stdout, data)
-		} else if serviceURLMode && !all {
-			for _, u := range data {
-				out.String(fmt.Sprintf("%s\n", u[3]))
+				if serviceURLMode && !driver.NeedsPortForward(co.Config.Driver) {
+					out.String(fmt.Sprintf("%s\n", serviceURLs))
+				}
 			}
 		}
 
-		if driver.NeedsPortForward(co.Config.Driver) {
-			startKicServiceTunnel(args, services, cname, co.Config.Driver)
-			return
-		}
-
-		if !serviceURLMode && !all && len(args) == 1 {
-			openURLs(args[0], openUrls)
+		if driver.NeedsPortForward(co.Config.Driver) && services != nil {
+			startKicServiceTunnel(services, cname, co.Config.Driver)
+		} else if !serviceURLMode {
+			openURLs(data)
 		}
 	},
 }
 
-func shouldOpen(args []string) bool {
-	if !serviceURLMode && !all && len(args) == 1 {
-		return true
-	}
-	return false
-}
-
 func init() {
 	serviceCmd.Flags().StringVarP(&namespace, "namespace", "n", "default", "The service namespace")
 	serviceCmd.Flags().BoolVar(&serviceURLMode, "url", false, "Display the Kubernetes service URL in the CLI instead of opening it in the default browser")
@@ -168,7 +159,7 @@ func init() {
 	serviceCmd.PersistentFlags().StringVar(&serviceURLFormat, "format", defaultServiceFormatTemplate, "Format to output service URL in. This format will be applied to each url individually and they will be printed one at a time.")
 }
 
-func startKicServiceTunnel(args []string, services service.URLs, configName, driverName string) {
+func startKicServiceTunnel(services service.URLs, configName, driverName string) {
 	ctrlC := make(chan os.Signal, 1)
 	signal.Notify(ctrlC, os.Interrupt)
 
@@ -186,35 +177,81 @@ func startKicServiceTunnel(args []string, services service.URLs, configName, dri
 		sshPort := strconv.Itoa(port)
 		sshKey := filepath.Join(localpath.MiniPath(), "machines", configName, "id_rsa")
 
-		serviceTunnel := kic.NewServiceTunnel(sshPort, sshKey, clientset.CoreV1())
+		serviceTunnel := kic.NewServiceTunnel(sshPort, sshKey, clientset.CoreV1(), serviceURLMode)
 		urls, err := serviceTunnel.Start(svc.Name, namespace)
+
 		if err != nil {
 			exit.Error(reason.SvcTunnelStart, "error starting tunnel", err)
 		}
+		// mutate response urls to HTTPS if needed
+		urls, err = mutateURLs(svc.Name, urls)
+
+		if err != nil {
+			exit.Error(reason.SvcTunnelStart, "error creatings urls", err)
+		}
+
 		defer serviceTunnel.Stop()
+		svc.URLs = urls
 		data = append(data, []string{namespace, svc.Name, "", strings.Join(urls, "\n")})
 	}
 
 	time.Sleep(1 * time.Second)
 
-	if !serviceURLMode && serviceURLFormat != defaultServiceFormatTemplate && !all {
+	if !serviceURLMode {
 		service.PrintServiceList(os.Stdout, data)
+	} else {
+		for _, row := range data {
+			out.String(fmt.Sprintf("%s\n", row[3]))
+		}
 	}
 
-	if shouldOpen(args) {
-		openURLs(services[0].Name, services[0].URLs)
+	if !serviceURLMode {
+		openURLs(data)
 	}
 
 	out.WarningT("Because you are using a Docker driver on {{.operating_system}}, the terminal needs to be open to run it.", out.V{"operating_system": runtime.GOOS})
 
 	<-ctrlC
 }
 
-func openURLs(svc string, urls []string) {
+func mutateURLs(serviceName string, urls []string) ([]string, error) {
+	formattedUrls := make([]string, 0)
+	for _, rawURL := range urls {
+		var doc bytes.Buffer
+		parsedURL, err := url.Parse(rawURL)
+		if err != nil {
+			exit.Error(reason.SvcTunnelStart, "No valid URL found for tunnel.", err)
+		}
+		port, err := strconv.Atoi(parsedURL.Port())
+		if err != nil {
+			exit.Error(reason.SvcTunnelStart, "No valid port found for tunnel.", err)
+		}
+		err = serviceURLTemplate.Execute(&doc, struct {
+			IP   string
+			Port int32
+			Name string
+		}{
+			parsedURL.Hostname(),
+			int32(port),
+			serviceName,
+		})
+
+		if err != nil {
+			return nil, err
+		}
+
+		httpsURL, _ := service.OptionallyHTTPSFormattedURLString(doc.String(), https)
+		formattedUrls = append(formattedUrls, httpsURL)
+	}
+
+	return formattedUrls, nil
+}
+
+func openURLs(urls [][]string) {
 	for _, u := range urls {
-		_, err := url.Parse(u)
+		_, err := url.Parse(u[3])
 		if err != nil {
-			klog.Warningf("failed to parse url %q: %v (will not open)", u, err)
+			klog.Warningf("failed to parse url %q: %v (will not open)", u[3], err)
 			out.String(fmt.Sprintf("%s\n", u))
 			continue
 		}
@@ -224,8 +261,8 @@ func openURLs(svc string, urls []string) {
 			continue
 		}
 
-		out.Styled(style.Celebrate, "Opening service {{.namespace_name}}/{{.service_name}} in default browser...", out.V{"namespace_name": namespace, "service_name": svc})
-		if err := browser.OpenURL(u); err != nil {
+		out.Styled(style.Celebrate, "Opening service {{.namespace_name}}/{{.service_name}} in default browser...", out.V{"namespace_name": namespace, "service_name": u[1]})
+		if err := browser.OpenURL(u[3]); err != nil {
 			exit.Error(reason.HostBrowser, fmt.Sprintf("open url failed: %s", u), err)
 		}
 	}

cmd/minikube/cmd/service_test.go

@@ -30,18 +30,20 @@ import (
 
 // ServiceTunnel ...
 type ServiceTunnel struct {
-	sshPort string
-	sshKey  string
-	v1Core  typed_core.CoreV1Interface
-	sshConn *sshConn
+	sshPort        string
+	sshKey         string
+	v1Core         typed_core.CoreV1Interface
+	sshConn        *sshConn
+	suppressStdOut bool
 }
 
 // NewServiceTunnel ...
-func NewServiceTunnel(sshPort, sshKey string, v1Core typed_core.CoreV1Interface) *ServiceTunnel {
+func NewServiceTunnel(sshPort, sshKey string, v1Core typed_core.CoreV1Interface, suppressStdOut bool) *ServiceTunnel {
 	return &ServiceTunnel{
-		sshPort: sshPort,
-		sshKey:  sshKey,
-		v1Core:  v1Core,
+		sshPort:        sshPort,
+		sshKey:         sshKey,
+		v1Core:         v1Core,
+		suppressStdOut: suppressStdOut,
 	}
 }
 
@@ -58,6 +60,7 @@ func (t *ServiceTunnel) Start(svcName, namespace string) ([]string, error) {
 	}
 
 	go func() {
+		t.sshConn.suppressStdOut = t.suppressStdOut
 		err = t.sshConn.startAndWait()
 		if err != nil {
 			klog.Errorf("error starting ssh tunnel: %v", err)

pkg/minikube/tunnel/kic/service_tunnel.go

@@ -30,11 +30,12 @@ import (
 )
 
 type sshConn struct {
-	name       string
-	service    string
-	cmd        *exec.Cmd
-	ports      []int
-	activeConn bool
+	name           string
+	service        string
+	cmd            *exec.Cmd
+	ports          []int
+	activeConn     bool
+	suppressStdOut bool
 }
 
 func createSSHConn(name, sshPort, sshKey string, resourcePorts []int32, resourceIP string, resourceName string) *sshConn {
@@ -139,7 +140,9 @@ func createSSHConnWithRandomPorts(name, sshPort, sshKey string, svc *v1.Service)
 }
 
 func (c *sshConn) startAndWait() error {
-	out.Step(style.Running, "Starting tunnel for service {{.service}}.", out.V{"service": c.service})
+	if !c.suppressStdOut {
+		out.Step(style.Running, "Starting tunnel for service {{.service}}.", out.V{"service": c.service})
+	}
 
 	err := c.cmd.Start()
 	if err != nil {
@@ -159,14 +162,18 @@ func (c *sshConn) startAndWait() error {
 func (c *sshConn) stop() error {
 	if c.activeConn {
 		c.activeConn = false
-		out.Step(style.Stopping, "Stopping tunnel for service {{.service}}.", out.V{"service": c.service})
+		if !c.suppressStdOut {
+			out.Step(style.Stopping, "Stopping tunnel for service {{.service}}.", out.V{"service": c.service})
+		}
 		err := c.cmd.Process.Kill()
 		if err == os.ErrProcessDone {
 			// No need to return an error here
 			return nil
 		}
 		return err
 	}
-	out.Step(style.Stopping, "Stopped tunnel for service {{.service}}.", out.V{"service": c.service})
+	if !c.suppressStdOut {
+		out.Step(style.Stopping, "Stopped tunnel for service {{.service}}.", out.V{"service": c.service})
+	}
 	return nil
 }