From 1454d5d174f7b4a675d73a956b42633d0c5b2dbd Mon Sep 17 00:00:00 2001
From: Deyuan Deng <deyuan.deng@gmail.com>
Date: Mon, 6 Oct 2014 20:49:20 -0400
Subject: [PATCH] Improve replication controller errro handling.

---
 pkg/kubecfg/kubecfg.go      | 22 +++++++++++--------
 pkg/kubecfg/kubecfg_test.go | 44 ++++++++++++++++++++++++++++++++++---
 2 files changed, 54 insertions(+), 12 deletions(-)

diff --git a/pkg/kubecfg/kubecfg.go b/pkg/kubecfg/kubecfg.go
index d4d9c1ffb89..8a0853d136a 100644
--- a/pkg/kubecfg/kubecfg.go
+++ b/pkg/kubecfg/kubecfg.go
@@ -158,9 +158,9 @@ func ResizeController(ctx api.Context, name string, replicas int, client client.
 	return nil
 }
 
-func portsFromString(spec string) []api.Port {
+func portsFromString(spec string) ([]api.Port, error) {
 	if spec == "" {
-		return []api.Port{}
+		return []api.Port{}, nil
 	}
 	parts := strings.Split(spec, ",")
 	var result []api.Port
@@ -168,7 +168,7 @@ func portsFromString(spec string) []api.Port {
 		pieces := strings.Split(part, ":")
 		if len(pieces) < 1 || len(pieces) > 2 {
 			glog.Infof("Bad port spec: %s", part)
-			continue
+			return nil, fmt.Errorf("Bad port spec: %s", part)
 		}
 		host := 0
 		container := 0
@@ -177,28 +177,28 @@ func portsFromString(spec string) []api.Port {
 			container, err = strconv.Atoi(pieces[0])
 			if err != nil {
 				glog.Errorf("Container port is not integer: %s %v", pieces[0], err)
-				continue
+				return nil, err
 			}
 		} else {
 			host, err = strconv.Atoi(pieces[0])
 			if err != nil {
 				glog.Errorf("Host port is not integer: %s %v", pieces[0], err)
-				continue
+				return nil, err
 			}
 			container, err = strconv.Atoi(pieces[1])
 			if err != nil {
 				glog.Errorf("Container port is not integer: %s %v", pieces[1], err)
-				continue
+				return nil, err
 			}
 		}
 		if container < 1 {
 			glog.Errorf("Container port is not valid: %d", container)
-			continue
+			return nil, err
 		}
 
 		result = append(result, api.Port{ContainerPort: container, HostPort: host})
 	}
-	return result
+	return result, nil
 }
 
 // RunController creates a new replication controller named 'name' which creates 'replicas' pods running 'image'.
@@ -206,6 +206,10 @@ func RunController(ctx api.Context, image, name string, replicas int, client cli
 	if servicePort > 0 && !util.IsDNSLabel(name) {
 		return fmt.Errorf("Service creation requested, but an invalid name for a service was provided (%s). Service names must be valid DNS labels.", name)
 	}
+	ports, err := portsFromString(portSpec)
+	if err != nil {
+		return err
+	}
 	controller := &api.ReplicationController{
 		JSONBase: api.JSONBase{
 			ID: name,
@@ -223,7 +227,7 @@ func RunController(ctx api.Context, image, name string, replicas int, client cli
 							{
 								Name:  strings.ToLower(name),
 								Image: image,
-								Ports: portsFromString(portSpec),
+								Ports: ports,
 							},
 						},
 					},
diff --git a/pkg/kubecfg/kubecfg_test.go b/pkg/kubecfg/kubecfg_test.go
index 399a85f330b..ad1de9f9dd8 100644
--- a/pkg/kubecfg/kubecfg_test.go
+++ b/pkg/kubecfg/kubecfg_test.go
@@ -121,6 +121,27 @@ func TestRunController(t *testing.T) {
 	}
 }
 
+func TestRunControllerWithWrongArgs(t *testing.T) {
+	fakeClient := client.Fake{}
+	name := "name"
+	image := "foo/bar"
+	replicas := 3
+	err := RunController(api.NewDefaultContext(), image, name, replicas, &fakeClient, "8080:", -1)
+	if err == nil {
+		t.Errorf("Unexpected non-error: %#v", fakeClient.Actions)
+	}
+	RunController(api.NewDefaultContext(), image, name, replicas, &fakeClient, "8080:80", -1)
+	if len(fakeClient.Actions) != 1 || fakeClient.Actions[0].Action != "create-controller" {
+		t.Errorf("Unexpected actions: %#v", fakeClient.Actions)
+	}
+	controller := fakeClient.Actions[0].Value.(*api.ReplicationController)
+	if controller.ID != name ||
+		controller.DesiredState.Replicas != replicas ||
+		controller.DesiredState.PodTemplate.DesiredState.Manifest.Containers[0].Image != image {
+		t.Errorf("Unexpected controller: %#v", controller)
+	}
+}
+
 func TestRunControllerWithService(t *testing.T) {
 	fakeClient := client.Fake{}
 	name := "name"
@@ -273,7 +294,7 @@ func TestLoadAuthInfo(t *testing.T) {
 }
 
 func TestMakePorts(t *testing.T) {
-	var testCases = []struct {
+	var successTestCases = []struct {
 		spec  string
 		ports []api.Port
 	}{
@@ -290,10 +311,27 @@ func TestMakePorts(t *testing.T) {
 			[]api.Port{},
 		},
 	}
-	for _, tt := range testCases {
-		ports := portsFromString(tt.spec)
+	for _, tt := range successTestCases {
+		ports, err := portsFromString(tt.spec)
 		if !reflect.DeepEqual(ports, tt.ports) {
 			t.Errorf("Expected %#v, got %#v", tt.ports, ports)
 		}
+		if err != nil {
+			t.Errorf("Unexpected error: %v", err)
+		}
+	}
+
+	var failTestCases = []struct {
+		spec string
+	}{
+		{"8080:"},
+		{":80"},
+		{":"},
+	}
+	for _, tt := range failTestCases {
+		_, err := portsFromString(tt.spec)
+		if err == nil {
+			t.Errorf("Unexpected non-error")
+		}
 	}
 }