Jdm/stack healthcheck marshaling (#1975)

* Simplify curl-based healthcheck parsing for stacks.

There was at least one issue parsing for the url to transform the stack health check
to the data needed for a health check in kubernetes. The issue discovered was
an out of bounds on a slice issue when there was no path present.

This commit fixes #1974

Signed-off-by: Jacob MacElroy <jacob@okteto.com>

* Removing leftover print statements.

pkg/model/stack_serializer.go

@@ -15,7 +15,7 @@ package model
 
 import (
 	"fmt"
-	"regexp"
+	"net/url"
 	"strconv"
 	"strings"
 	"time"
@@ -490,45 +490,63 @@ func validateHealthcheck(healthcheck *HealthCheck) error {
 }
 
 func translateHealtcheckCurlToHTTP(healthcheck *HealthCheck) {
-	localPortTestRegex := `^curl ((-f|--fail) )?'?((http|https)://)?(localhost|0.0.0.0):\d+(\/\w*)?'?$`
-	regexp, err := regexp.Compile(localPortTestRegex)
-	if err != nil {
+	// Join and then split the strings by space to ensure that
+	// each element in the string slice is a contiguous string with
+	// no spaces.
+	s := strings.Join(healthcheck.Test, " ")
+	testStrings := strings.Split(s, " ")
+
+	// There should be at least two strings, the curl binary and url.
+	if len(testStrings) < 2 {
 		return
 	}
-	testString := strings.Join(healthcheck.Test, " ")
-	if regexp.MatchString(testString) {
-		var firstSlashIndex, portStart int
-		if strings.Contains(testString, "://") {
-			testStringCopy := testString
-			for i := 0; i < 3; i++ {
-				firstSlashIndex += strings.Index(testStringCopy[firstSlashIndex:], "/") + 1
-			}
-			testStringCopy = testString
-			for i := 0; i < 2; i++ {
-				portStart += strings.Index(testStringCopy[portStart:], ":") + 1
-			}
-			portStart--
-			firstSlashIndex--
-		} else {
-			firstSlashIndex = strings.Index(testString, "/")
-			portStart = strings.Index(testString, ":")
-		}
 
-		var port, path string
-		if firstSlashIndex != -1 {
-			port = testString[portStart+1 : firstSlashIndex]
-			path = testString[firstSlashIndex:]
-		} else {
-			port = testString[portStart+1:]
-			path = "/"
+	if testStrings[0] != "curl" {
+		return
+	}
+
+	var checkURL *url.URL
+	for _, possibleURL := range testStrings[1:] {
+		if possibleURL == "-f" || possibleURL == "--fail" {
+			continue
 		}
-		p, err := strconv.Atoi(port)
-		if err != nil {
-			return
+
+		u, err := url.ParseRequestURI(possibleURL)
+		// It's possible to have a healthcheck url without the scheme. If
+		// that happens then we inject one and attempt parsing again.
+		if err != nil || u.Host == "" {
+			u, err = url.ParseRequestURI("https://" + possibleURL)
+			if err != nil {
+				u = nil
+				continue
+			}
 		}
-		healthcheck.HTTP = &HTTPHealtcheck{Path: path, Port: int32(p)}
-		healthcheck.Test = make(HealtcheckTest, 0)
+		checkURL = u
+		break
 	}
+
+	if checkURL == nil {
+		return
+	}
+
+	p := checkURL.Port()
+	if p == "" {
+		return
+	}
+	port, err := strconv.Atoi(p)
+	if err != nil {
+		return
+	}
+
+	var path string
+	if checkURL.Path == "" {
+		path = "/"
+	} else {
+		path = checkURL.Path
+	}
+
+	healthcheck.HTTP = &HTTPHealtcheck{Path: path, Port: int32(port)}
+	healthcheck.Test = make(HealtcheckTest, 0)
 }
 
 func getSvcPorts(public bool, rawPorts, rawExpose []PortRaw) (bool, []Port, error) {

pkg/model/stack_serializer_test.go

@@ -14,6 +14,7 @@
 package model
 
 import (
+	"fmt"
 	"os"
 	"reflect"
 	"testing"
@@ -26,6 +27,77 @@ import (
 	"k8s.io/utils/pointer"
 )
 
+func TestTranslateHealthcheckCurlToHttp(t *testing.T) {
+	for _, tc := range []struct {
+		name         string
+		test         []string
+		expectedTest HealtcheckTest
+		expectedHTTP HTTPHealtcheck
+	}{
+		{
+			name:         "RegularCurlWithZeros",
+			test:         []string{"curl http://0.0.0.0:4572"},
+			expectedTest: HealtcheckTest{},
+			expectedHTTP: HTTPHealtcheck{Path: "/", Port: int32(4572)},
+		},
+		{
+			name:         "RegularCurlWithLocalhost",
+			test:         []string{"curl http://localhost:4572"},
+			expectedTest: HealtcheckTest{},
+			expectedHTTP: HTTPHealtcheck{Path: "/", Port: int32(4572)},
+		},
+		{
+			name:         "RegularCurlTrailingSlash",
+			test:         []string{"curl http://0.0.0.0:4572/"},
+			expectedTest: HealtcheckTest{},
+			expectedHTTP: HTTPHealtcheck{Path: "/", Port: int32(4572)},
+		},
+		{
+			name:         "MissingScheme",
+			test:         []string{"curl 0.0.0.0:4572/readiness"},
+			expectedTest: HealtcheckTest{},
+			expectedHTTP: HTTPHealtcheck{Path: "/readiness", Port: int32(4572)},
+		},
+		{
+			name:         "MissingSchemeWithF",
+			test:         []string{"curl -f localhost:8080/"},
+			expectedTest: HealtcheckTest{},
+			expectedHTTP: HTTPHealtcheck{Path: "/", Port: int32(8080)},
+		},
+		{
+			name:         "NoTest",
+			test:         []string{"curl --fail 0.0.0.0:8080"},
+			expectedTest: HealtcheckTest{},
+			expectedHTTP: HTTPHealtcheck{Path: "/", Port: int32(8080)},
+		},
+		{
+			name:         "Readiness",
+			test:         []string{"curl https://0.0.0.0:8080/readiness"},
+			expectedTest: HealtcheckTest{},
+			expectedHTTP: HTTPHealtcheck{Path: "/readiness", Port: int32(8080)},
+		},
+		{
+			name:         "RegularCurlWithPath",
+			test:         []string{"curl http://0.0.0.0:4572/a/path/exists"},
+			expectedTest: HealtcheckTest{},
+			expectedHTTP: HTTPHealtcheck{Path: "/a/path/exists", Port: int32(4572)},
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			check := HealthCheck{
+				Test: tc.test,
+			}
+			translateHealtcheckCurlToHTTP(&check)
+			if !reflect.DeepEqual(check.Test, tc.expectedTest) {
+				t.Fatalf("expected %v but got %v", tc.expectedTest, check.Test)
+			}
+			if !reflect.DeepEqual(check.Test, tc.expectedTest) {
+				t.Fatalf("expected %v but got %v", tc.expectedHTTP, check.HTTP)
+			}
+		})
+	}
+}
+
 func Test_DeployReplicasUnmarshalling(t *testing.T) {
 	tests := []struct {
 		name      string
@@ -324,7 +396,8 @@ func Test_HealthcheckUnmarshalling(t *testing.T) {
 			}
 			if !tt.expectedError {
 				if !reflect.DeepEqual(s.Services["app"].Healtcheck, tt.expected) {
-					t.Fatalf("Expected %v, but got %v", tt.expected, s.Services["app"].Healtcheck)
+					fmt.Printf("GOT: %+v", s.Services["app"].Healtcheck.HTTP)
+					t.Fatalf("Expected %+v, but got %+v", tt.expected, s.Services["app"].Healtcheck)
 				}
 			}