Fix flow to support docker driver properly for mac, issue #13736

ckannon · ckannon · commit 83aad3965fad · 2022-03-07T18:24:46.000-05:00
diff --git a/cmd/minikube/cmd/service.go b/cmd/minikube/cmd/service.go
@@ -17,6 +17,7 @@ limitations under the License.
 package cmd
 
 import (
+	"bytes"
 	"errors"
 	"fmt"
 	"net/url"
@@ -103,9 +104,8 @@ var serviceCmd = &cobra.Command{
 		}
 
 		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 +128,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)
+			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 +154,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 +172,72 @@ 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)
+
+		//mutate response urls to HTTPS if needed
+		urls = mutateURLs(svc.Name, urls)
+
 		if err != nil {
 			exit.Error(reason.SvcTunnelStart, "error starting tunnel", 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 {
+	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,
+		})
+		httpsUrl, _ := service.OptionallyHTTPSFormattedURLString(doc.String(), https)
+		formattedUrls = append(formattedUrls, httpsUrl)
+	}
+
+	return formattedUrls
+}
+
+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 +247,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)
 		}
 	}
diff --git a/cmd/minikube/cmd/service_test.go b/cmd/minikube/cmd/service_test.go
diff --git a/pkg/minikube/tunnel/kic/service_tunnel.go b/pkg/minikube/tunnel/kic/service_tunnel.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: false,
 	}
 }
 
@@ -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)
diff --git a/pkg/minikube/tunnel/kic/ssh_conn.go b/pkg/minikube/tunnel/kic/ssh_conn.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
 }
diff --git a/test/integration/functional_test.go b/test/integration/functional_test.go
@@ -1399,6 +1399,11 @@ func validateProfileCmd(ctx context.Context, t *testing.T, profile string) {
 	})
 }
 
+func killContext(cmdContext *exec.Cmd, killDelay time.Duration) {
+	time.Sleep(killDelay)
+	cmdContext.Process.Signal(os.Interrupt)
+}
+
 // validateServiceCmd asserts basic "service" command functionality
 func validateServiceCmd(ctx context.Context, t *testing.T, profile string) {
 	defer PostMortemLogs(t, profile)
@@ -1460,18 +1465,18 @@ func validateServiceCmd(ctx context.Context, t *testing.T, profile string) {
 		t.Errorf("expected 'service list' to contain *hello-node* but got -%q-", rr.Stdout.String())
 	}
 
+	t.Logf("Needs port forward: %t, %s", NeedsPortForward(), runtime.GOOS)
+
+	// docs: Run `minikube service` with `--https --url` to make sure the HTTPS endpoint URL of the service is printed
+	cmdContext := exec.CommandContext(ctx, Target(), "-p", profile, "service", "--namespace=default", "--https", "--url", "hello-node")
 	if NeedsPortForward() {
-		t.Skipf("test is broken for port-forwarded drivers: https://github.com/kubernetes/minikube/issues/7383")
+		go killContext(cmdContext, 2*time.Second)
 	}
 
-	// docs: Run `minikube service` with `--https --url` to make sure the HTTPS endpoint URL of the service is printed
-	rr, err = Run(t, exec.CommandContext(ctx, Target(), "-p", profile, "service", "--namespace=default", "--https", "--url", "hello-node"))
-	if err != nil {
+	rr, err = Run(t, cmdContext)
+	if err != nil && !strings.Contains(err.Error(), "interrupt") {
 		t.Fatalf("failed to get service url. args %q : %v", rr.Command(), err)
 	}
-	if rr.Stderr.String() != "" {
-		t.Errorf("expected stderr to be empty but got *%q* . args %q", rr.Stderr, rr.Command())
-	}
 
 	splits := strings.Split(rr.Stdout.String(), "|")
 	var endpoint string
@@ -1491,17 +1496,26 @@ func validateServiceCmd(ctx context.Context, t *testing.T, profile string) {
 		t.Errorf("expected scheme for %s to be 'https' but got %q", endpoint, u.Scheme)
 	}
 
+	cmdContext = exec.CommandContext(ctx, Target(), "-p", profile, "service", "hello-node", "--url", "--format={{.IP}}")
+	if NeedsPortForward() {
+		go killContext(cmdContext, 2*time.Second)
+	}
 	// docs: Run `minikube service` with `--url --format={{.IP}}` to make sure the IP address of the service is printed
-	rr, err = Run(t, exec.CommandContext(ctx, Target(), "-p", profile, "service", "hello-node", "--url", "--format={{.IP}}"))
-	if err != nil {
+	rr, err = Run(t, cmdContext)
+	if err != nil && !strings.Contains(err.Error(), "interrupt") {
 		t.Errorf("failed to get service url with custom format. args %q: %v", rr.Command(), err)
 	}
+
 	if strings.TrimSpace(rr.Stdout.String()) != u.Hostname() {
 		t.Errorf("expected 'service --format={{.IP}}' output to be -%q- but got *%q* . args %q.", u.Hostname(), rr.Stdout.String(), rr.Command())
 	}
 
+	cmdContext = exec.CommandContext(ctx, Target(), "-p", profile, "service", "hello-node", "--url")
+	if NeedsPortForward() {
+		go killContext(cmdContext, 2*time.Second)
+	}
 	// docs: Run `minikube service` with a regular `--url` to make sure the HTTP endpoint URL of the service is printed
-	rr, err = Run(t, exec.CommandContext(ctx, Target(), "-p", profile, "service", "hello-node", "--url"))
+	rr, err = Run(t, cmdContext)
 	if err != nil {
 		t.Errorf("failed to get service url. args: %q: %v", rr.Command(), err)
 	}
@@ -1518,6 +1532,10 @@ func validateServiceCmd(ctx context.Context, t *testing.T, profile string) {
 		t.Fatalf("expected scheme to be -%q- got scheme: *%q*", "http", u.Scheme)
 	}
 
+	if NeedsPortForward() {
+		t.Skipf("test is broken for port-forwarded drivers: https://github.com/kubernetes/minikube/issues/7383")
+	}
+
 	t.Logf("Attempting to fetch %s ...", endpoint)
 
 	// docs: Make sure we can hit the endpoint URL with an HTTP GET request
