Use IntOrString for HTTP health check ports

Clean up code to be more testable. Add test cases for named and numeric ports in HTTP health checks. Improve tests.
duda · Aug 11, 2014 · 7201227 · 7201227
1 parent e35dfed
commit 7201227
Show file tree

Hide file tree

Showing 8 changed files with 153 additions and 48 deletions.
diff --git a/pkg/api/types.go b/pkg/api/types.go
@@ -136,11 +136,11 @@ type EnvVar struct {
 
 // HTTPGetProbe describes a liveness probe based on HTTP Get requests.
 type HTTPGetProbe struct {
-	// Path to access on the http server
+	// Optional: Path to access on the HTTP server.
 	Path string `yaml:"path,omitempty" json:"path,omitempty"`
-	// Name or number of the port to access on the container
-	Port string `yaml:"port,omitempty" json:"port,omitempty"`
-	// Host name to connect to.  Optional, default: "localhost"
+	// Required: Name or number of the port to access on the container.
+	Port util.IntOrString `yaml:"port,omitempty" json:"port,omitempty"`
+	// Optional: Host name to connect to, defaults to the pod IP.
 	Host string `yaml:"host,omitempty" json:"host,omitempty"`
 }
 

diff --git a/pkg/api/v1beta1/types.go b/pkg/api/v1beta1/types.go
@@ -139,11 +139,11 @@ type EnvVar struct {
 
 // HTTPGetProbe describes a liveness probe based on HTTP Get requests.
 type HTTPGetProbe struct {
-	// Path to access on the http server
+	// Optional: Path to access on the HTTP server.
 	Path string `yaml:"path,omitempty" json:"path,omitempty"`
-	// Name or number of the port to access on the container
-	Port string `yaml:"port,omitempty" json:"port,omitempty"`
-	// Host name to connect to.  Optional, default: "localhost"
+	// Required: Name or number of the port to access on the container.
+	Port util.IntOrString `yaml:"port,omitempty" json:"port,omitempty"`
+	// Optional: Host name to connect to, defaults to the pod IP.
 	Host string `yaml:"host,omitempty" json:"host,omitempty"`
 }
 

diff --git a/pkg/health/health.go b/pkg/health/health.go
@@ -36,11 +36,11 @@ type HTTPGetInterface interface {
 	Get(url string) (*http.Response, error)
 }
 
-// Check checks if a GET request to the url succeeds.
+// DoHTTPCheck checks if a GET request to the url succeeds.
 // If the HTTP response code is successful (i.e. 400 > code >= 200), it returns Healthy.
 // If the HTTP response code is unsuccessful, it returns Unhealthy.
 // It returns Unknown and err if the HTTP communication itself fails.
-func Check(url string, client HTTPGetInterface) (Status, error) {
+func DoHTTPCheck(url string, client HTTPGetInterface) (Status, error) {
 	res, err := client.Get(url)
 	if err != nil {
 		return Unknown, err

diff --git a/pkg/health/health_check.go b/pkg/health/health_check.go
@@ -23,6 +23,7 @@ import (
 	"strconv"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 	"github.com/golang/glog"
 )
 
@@ -65,38 +66,62 @@ type HTTPHealthChecker struct {
 	client HTTPGetInterface
 }
 
-func (h *HTTPHealthChecker) findPort(container api.Container, portName string) int64 {
+// A helper function to look up a port in a container by name.
+// Returns the HostPort if found, -1 if not found.
+func findPortByName(container api.Container, portName string) int {
 	for _, port := range container.Ports {
 		if port.Name == portName {
-			// TODO This means you can only health check exposed ports
-			return int64(port.HostPort)
+			return port.HostPort
 		}
 	}
 	return -1
 }
 
-// HealthCheck checks if the container is healthy by trying sending HTTP Get requests to the container.
-func (h *HTTPHealthChecker) HealthCheck(currentState api.PodState, container api.Container) (Status, error) {
+// Get the components of the target URL.  For testability.
+func getURLParts(currentState api.PodState, container api.Container) (string, int, string, error) {
 	params := container.LivenessProbe.HTTPGet
 	if params == nil {
-		return Unknown, fmt.Errorf("Error, no HTTP parameters specified: %v", container)
+		return "", -1, "", fmt.Errorf("no HTTP parameters specified: %v", container)
 	}
-	port := h.findPort(container, params.Port)
-	if port == -1 {
-		var err error
-		port, err = strconv.ParseInt(params.Port, 10, 0)
-		if err != nil {
-			return Unknown, err
+	port := -1
+	switch params.Port.Kind {
+	case util.IntstrInt:
+		port = params.Port.IntVal
+	case util.IntstrString:
+		port = findPortByName(container, params.Port.StrVal)
+		if port == -1 {
+			// Last ditch effort - maybe it was an int stored as string?
+			var err error
+			if port, err = strconv.Atoi(params.Port.StrVal); err != nil {
+				return "", -1, "", err
+			}
 		}
 	}
+	if port == -1 {
+		return "", -1, "", fmt.Errorf("unknown port: %v", params.Port)
+	}
 	var host string
 	if len(params.Host) > 0 {
 		host = params.Host
 	} else {
 		host = currentState.PodIP
 	}
-	url := fmt.Sprintf("http://%s:%d%s", host, port, params.Path)
-	return Check(url, h.client)
+
+	return host, port, params.Path, nil
+}
+
+// Formats a URL from args.  For testability.
+func formatURL(host string, port int, path string) string {
+	return fmt.Sprintf("http://%s:%d%s", host, port, path)
+}
+
+// HealthCheck checks if the container is healthy by trying sending HTTP Get requests to the container.
+func (h *HTTPHealthChecker) HealthCheck(currentState api.PodState, container api.Container) (Status, error) {
+	host, port, path, err := getURLParts(currentState, container)
+	if err != nil {
+		return Unknown, err
+	}
+	return DoHTTPCheck(formatURL(host, port, path), h.client)
 }
 
 type TCPHealthChecker struct{}

diff --git a/pkg/health/health_check_test.go b/pkg/health/health_check_test.go
@@ -25,6 +25,7 @@ import (
 	"testing"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 )
 
 const statusServerEarlyShutdown = -1
@@ -59,7 +60,7 @@ func TestHealthChecker(t *testing.T) {
 		container := api.Container{
 			LivenessProbe: &api.LivenessProbe{
 				HTTPGet: &api.HTTPGetProbe{
-					Port: port,
+					Port: util.MakeIntOrStringFromString(port),
 					Path: "/foo/bar",
 					Host: host,
 				},
@@ -90,32 +91,91 @@ func TestFindPort(t *testing.T) {
 			},
 		},
 	}
-	checker := HTTPHealthChecker{}
-	want := int64(8080)
-	got := checker.findPort(container, "foo")
+	want := 8080
+	got := findPortByName(container, "foo")
 	if got != want {
 		t.Errorf("Expected %v, got %v", want, got)
 	}
 }
 
+func TestGetURLParts(t *testing.T) {
+	testCases := []struct {
+		probe *api.HTTPGetProbe
+		ok    bool
+		host  string
+		port  int
+		path  string
+	}{
+		{&api.HTTPGetProbe{Host: "", Port: util.MakeIntOrStringFromInt(-1), Path: ""}, false, "", -1, ""},
+		{&api.HTTPGetProbe{Host: "", Port: util.MakeIntOrStringFromString(""), Path: ""}, false, "", -1, ""},
+		{&api.HTTPGetProbe{Host: "", Port: util.MakeIntOrStringFromString("-1"), Path: ""}, false, "", -1, ""},
+		{&api.HTTPGetProbe{Host: "", Port: util.MakeIntOrStringFromString("not-found"), Path: ""}, false, "", -1, ""},
+		{&api.HTTPGetProbe{Host: "", Port: util.MakeIntOrStringFromString("found"), Path: ""}, true, "127.0.0.1", 93, ""},
+		{&api.HTTPGetProbe{Host: "", Port: util.MakeIntOrStringFromInt(76), Path: ""}, true, "127.0.0.1", 76, ""},
+		{&api.HTTPGetProbe{Host: "", Port: util.MakeIntOrStringFromString("118"), Path: ""}, true, "127.0.0.1", 118, ""},
+		{&api.HTTPGetProbe{Host: "hostname", Port: util.MakeIntOrStringFromInt(76), Path: "path"}, true, "hostname", 76, "path"},
+	}
+
+	for _, test := range testCases {
+		state := api.PodState{PodIP: "127.0.0.1"}
+		container := api.Container{
+			Ports: []api.Port{{Name: "found", HostPort: 93}},
+			LivenessProbe: &api.LivenessProbe{
+				HTTPGet: test.probe,
+				Type:    "http",
+			},
+		}
+		host, port, path, err := getURLParts(state, container)
+		if !test.ok && err == nil {
+			t.Errorf("Expected error for %+v, got %s:%d/%s", test, host, port, path)
+		}
+		if test.ok && err != nil {
+			t.Errorf("Unexpected error: %v", err)
+		}
+		if test.ok {
+			if host != test.host || port != test.port || path != test.path {
+				t.Errorf("Expected %s:%d/%s, got %s:%d/%s",
+					test.host, test.port, test.path, host, port, path)
+			}
+		}
+	}
+}
+
+func TestFormatURL(t *testing.T) {
+	testCases := []struct {
+		host   string
+		port   int
+		path   string
+		result string
+	}{
+		{"localhost", 93, "", "http://localhost:93"},
+		{"localhost", 93, "/path", "http://localhost:93/path"},
+	}
+	for _, test := range testCases {
+		url := formatURL(test.host, test.port, test.path)
+		if url != test.result {
+			t.Errorf("Expected %s, got %s", test.result, url)
+		}
+	}
+}
+
 func TestHTTPHealthChecker(t *testing.T) {
-	httpHealthCheckerTests := []struct {
+	testCases := []struct {
 		probe  *api.HTTPGetProbe
 		status int
 		health Status
 	}{
-		{&api.HTTPGetProbe{Host: "httptest"}, http.StatusOK, Healthy},
+		// The probe will be filled in below.  This is primarily testing that an HTTP GET happens.
 		{&api.HTTPGetProbe{}, http.StatusOK, Healthy},
+		{&api.HTTPGetProbe{}, -1, Unhealthy},
 		{nil, -1, Unknown},
-		{&api.HTTPGetProbe{Port: "-1"}, -1, Unknown},
 	}
 	hc := &HTTPHealthChecker{
 		client: &http.Client{},
 	}
-	for _, httpHealthCheckerTest := range httpHealthCheckerTests {
-		tt := httpHealthCheckerTest
+	for _, test := range testCases {
 		ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			w.WriteHeader(tt.status)
+			w.WriteHeader(test.status)
 		}))
 		u, err := url.Parse(ts.URL)
 		if err != nil {
@@ -127,28 +187,24 @@ func TestHTTPHealthChecker(t *testing.T) {
 		}
 		container := api.Container{
 			LivenessProbe: &api.LivenessProbe{
-				HTTPGet: tt.probe,
+				HTTPGet: test.probe,
 				Type:    "http",
 			},
 		}
 		params := container.LivenessProbe.HTTPGet
 		if params != nil {
-			if params.Port == "" {
-				params.Port = port
-			}
-			if params.Host == "httptest" {
-				params.Host = host
-			}
+			params.Port = util.MakeIntOrStringFromString(port)
+			params.Host = host
 		}
 		health, err := hc.HealthCheck(api.PodState{PodIP: host}, container)
-		if tt.health == Unknown && err == nil {
+		if test.health == Unknown && err == nil {
 			t.Errorf("Expected error")
 		}
-		if tt.health != Unknown && err != nil {
+		if test.health != Unknown && err != nil {
 			t.Errorf("Unexpected error: %v", err)
 		}
-		if health != tt.health {
-			t.Errorf("Expected %v, got %v", tt.health, health)
+		if health != test.health {
+			t.Errorf("Expected %v, got %v", test.health, health)
 		}
 	}
 }
@@ -228,7 +284,7 @@ func TestMuxHealthChecker(t *testing.T) {
 			},
 		}
 		container.LivenessProbe.Type = tt.probeType
-		container.LivenessProbe.HTTPGet.Port = port
+		container.LivenessProbe.HTTPGet.Port = util.MakeIntOrStringFromString(port)
 		container.LivenessProbe.HTTPGet.Host = host
 		health, err := mc.HealthCheck(api.PodState{}, container)
 		if err != nil {

diff --git a/pkg/registry/healthy_minion_registry.go b/pkg/registry/healthy_minion_registry.go
@@ -49,7 +49,7 @@ func (h *HealthyMinionRegistry) List() (currentMinions []string, err error) {
 		return result, err
 	}
 	for _, minion := range list {
-		status, err := health.Check(h.makeMinionURL(minion), h.client)
+		status, err := health.DoHTTPCheck(h.makeMinionURL(minion), h.client)
 		if err != nil {
 			glog.Errorf("%s failed health check with error: %s", minion, err)
 			continue
@@ -77,7 +77,7 @@ func (h *HealthyMinionRegistry) Contains(minion string) (bool, error) {
 	if !contains {
 		return false, nil
 	}
-	status, err := health.Check(h.makeMinionURL(minion), h.client)
+	status, err := health.DoHTTPCheck(h.makeMinionURL(minion), h.client)
 	if err != nil {
 		return false, err
 	}

diff --git a/pkg/util/util.go b/pkg/util/util.go
@@ -83,6 +83,16 @@ const (
 	IntstrString                   // The IntOrString holds a string.
 )
 
+// MakeIntOrStringFromInt creates an IntOrString object with an int value.
+func MakeIntOrStringFromInt(val int) IntOrString {
+	return IntOrString{Kind: IntstrInt, IntVal: val}
+}
+
+// MakeIntOrStringFromInt creates an IntOrString object with a string value.
+func MakeIntOrStringFromString(val string) IntOrString {
+	return IntOrString{Kind: IntstrString, StrVal: val}
+}
+
 // SetYAML implements the yaml.Setter interface.
 func (intstr *IntOrString) SetYAML(tag string, value interface{}) bool {
 	switch v := value.(type) {

diff --git a/pkg/util/util_test.go b/pkg/util/util_test.go
@@ -71,6 +71,20 @@ func TestHandleCrash(t *testing.T) {
 	}
 }
 
+func TestMakeIntOrStringFromInt(t *testing.T) {
+	i := MakeIntOrStringFromInt(93)
+	if i.Kind != IntstrInt || i.IntVal != 93 {
+		t.Errorf("Expected IntVal=93, got %+v", i)
+	}
+}
+
+func TestMakeIntOrStringFromString(t *testing.T) {
+	i := MakeIntOrStringFromString("76")
+	if i.Kind != IntstrString || i.StrVal != "76" {
+		t.Errorf("Expected StrVal=\"76\", got %+v", i)
+	}
+}
+
 type IntOrStringHolder struct {
 	IOrS IntOrString `json:"val" yaml:"val"`
 }