Merge pull request #13756 from kargakis/make-external-svcs-exposable

expose: Avoid selector resolution if a selector is not needed

pkg/kubectl/cmd/cmd_test.go

@@ -24,6 +24,7 @@ import (
 	"io/ioutil"
 	"os"
 	"reflect"
+	"strconv"
 	"testing"
 	"time"
 
@@ -192,10 +193,12 @@ func NewAPIFactory() (*cmdutil.Factory, *testFactory, runtime.Codec) {
 	}
 	generators := map[string]kubectl.Generator{
 		"run/v1":       kubectl.BasicReplicationController{},
+		"run-pod/v1":   kubectl.BasicPod{},
 		"service/v1":   kubectl.ServiceGeneratorV1{},
 		"service/v2":   kubectl.ServiceGeneratorV2{},
+		"service/test": testServiceGenerator{},
 	}
-	return &cmdutil.Factory{
+	f := &cmdutil.Factory{
 		Object: func() (meta.RESTMapper, runtime.ObjectTyper) {
 			return testapi.Default.RESTMapper(), api.Scheme
 		},
@@ -228,7 +231,12 @@ func NewAPIFactory() (*cmdutil.Factory, *testFactory, runtime.Codec) {
 			generator, ok := generators[name]
 			return generator, ok
 		},
-	}, t, testapi.Default.Codec()
+	}
+
+	rf := cmdutil.NewFactory(nil)
+	f.PodSelectorForObject = rf.PodSelectorForObject
+
+	return f, t, testapi.Default.Codec()
 }
 
 func objBody(codec runtime.Codec, obj runtime.Object) io.ReadCloser {
@@ -561,3 +569,111 @@ func TestNormalizationFuncGlobalExistence(t *testing.T) {
 		t.Fatal("child and root commands should have the same normalization functions")
 	}
 }
+
+type testServiceGenerator struct{}
+
+func (testServiceGenerator) ParamNames() []kubectl.GeneratorParam {
+	return []kubectl.GeneratorParam{
+		{"default-name", true},
+		{"name", false},
+		{"port", true},
+		{"labels", false},
+		{"public-ip", false},
+		{"create-external-load-balancer", false},
+		{"type", false},
+		{"protocol", false},
+		{"container-port", false}, // alias of target-port
+		{"target-port", false},
+		{"port-name", false},
+		{"session-affinity", false},
+	}
+}
+
+func (testServiceGenerator) Generate(genericParams map[string]interface{}) (runtime.Object, error) {
+	params := map[string]string{}
+	for key, value := range genericParams {
+		strVal, isString := value.(string)
+		if !isString {
+			return nil, fmt.Errorf("expected string, saw %v for '%s'", value, key)
+		}
+		params[key] = strVal
+	}
+	labelsString, found := params["labels"]
+	var labels map[string]string
+	var err error
+	if found && len(labelsString) > 0 {
+		labels, err = kubectl.ParseLabels(labelsString)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	name, found := params["name"]
+	if !found || len(name) == 0 {
+		name, found = params["default-name"]
+		if !found || len(name) == 0 {
+			return nil, fmt.Errorf("'name' is a required parameter.")
+		}
+	}
+	portString, found := params["port"]
+	if !found {
+		return nil, fmt.Errorf("'port' is a required parameter.")
+	}
+	port, err := strconv.Atoi(portString)
+	if err != nil {
+		return nil, err
+	}
+	servicePortName, found := params["port-name"]
+	if !found {
+		// Leave the port unnamed.
+		servicePortName = ""
+	}
+	service := api.Service{
+		ObjectMeta: api.ObjectMeta{
+			Name:   name,
+			Labels: labels,
+		},
+		Spec: api.ServiceSpec{
+			Ports: []api.ServicePort{
+				{
+					Name:     servicePortName,
+					Port:     port,
+					Protocol: api.Protocol(params["protocol"]),
+				},
+			},
+		},
+	}
+	targetPort, found := params["target-port"]
+	if !found {
+		targetPort, found = params["container-port"]
+	}
+	if found && len(targetPort) > 0 {
+		if portNum, err := strconv.Atoi(targetPort); err != nil {
+			service.Spec.Ports[0].TargetPort = util.NewIntOrStringFromString(targetPort)
+		} else {
+			service.Spec.Ports[0].TargetPort = util.NewIntOrStringFromInt(portNum)
+		}
+	} else {
+		service.Spec.Ports[0].TargetPort = util.NewIntOrStringFromInt(port)
+	}
+	if params["create-external-load-balancer"] == "true" {
+		service.Spec.Type = api.ServiceTypeLoadBalancer
+	}
+	if len(params["external-ip"]) > 0 {
+		service.Spec.ExternalIPs = []string{params["external-ip"]}
+	}
+	if len(params["type"]) != 0 {
+		service.Spec.Type = api.ServiceType(params["type"])
+	}
+	if len(params["session-affinity"]) != 0 {
+		switch api.ServiceAffinity(params["session-affinity"]) {
+		case api.ServiceAffinityNone:
+			service.Spec.SessionAffinity = api.ServiceAffinityNone
+		case api.ServiceAffinityClientIP:
+			service.Spec.SessionAffinity = api.ServiceAffinityClientIP
+		default:
+			return nil, fmt.Errorf("unknown session affinity: %s", params["session-affinity"])
+		}
+	}
+	return &service, nil
+}

pkg/kubectl/cmd/expose.go

@@ -129,14 +129,18 @@ func RunExpose(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []str
 	names := generator.ParamNames()
 	params := kubectl.MakeParams(cmd, names)
 	params["default-name"] = info.Name
-	if s, found := params["selector"]; !found || kubectl.IsZero(s) || cmdutil.GetFlagInt(cmd, "port") < 1 {
+
-		if kubectl.IsZero(s) {
+	// For objects that need a pod selector, derive it from the exposed object in case a user
+	// didn't explicitly specify one via --selector
+	if s, found := params["selector"]; found && kubectl.IsZero(s) {
 		s, err := f.PodSelectorForObject(inputObject)
 		if err != nil {
 			return cmdutil.UsageError(cmd, fmt.Sprintf("couldn't find selectors via --selector flag or introspection: %s", err))
 		}
 		params["selector"] = s
 	}
+
+	if cmdutil.GetFlagInt(cmd, "port") < 1 {
 		noPorts := true
 		for _, param := range names {
 			if param.Name == "port" {

pkg/kubectl/cmd/expose_test.go

@@ -39,10 +39,9 @@ func TestRunExposeService(t *testing.T) {
 		output   runtime.Object
 		expected string
 		status   int
-		podSelector string
 	}{
 		{
-			name: "expose-service-from-service-no-selector",
+			name: "expose-service-from-service-no-selector-defined",
 			args: []string{"service", "baz"},
 			ns:   "test",
 			calls: map[string]string{
@@ -51,27 +50,25 @@ func TestRunExposeService(t *testing.T) {
 			},
 			input: &api.Service{
 				ObjectMeta: api.ObjectMeta{Name: "baz", Namespace: "test", ResourceVersion: "12"},
-				TypeMeta:   api.TypeMeta{Kind: "Service", APIVersion: "v1"},
 				Spec: api.ServiceSpec{
 					Selector: map[string]string{"app": "go"},
 				},
 			},
-			podSelector: "app=go",
 			flags: map[string]string{"protocol": "UDP", "port": "14", "name": "foo", "labels": "svc=test"},
 			output: &api.Service{
-				ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: "test", ResourceVersion: "12", Labels: map[string]string{"svc": "test"}},
+				ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: "", Labels: map[string]string{"svc": "test"}},
-				TypeMeta:   api.TypeMeta{Kind: "Service", APIVersion: "v1"},
 				Spec: api.ServiceSpec{
 					Ports: []api.ServicePort{
 						{
-							Name:     "default",
+							Protocol:   api.ProtocolUDP,
-							Protocol: api.Protocol("UDP"),
 							Port:       14,
+							TargetPort: util.NewIntOrStringFromInt(14),
 						},
 					},
 					Selector: map[string]string{"app": "go"},
 				},
 			},
+			expected: "service \"foo\" exposed",
 			status:   200,
 		},
 		{
@@ -84,26 +81,25 @@ func TestRunExposeService(t *testing.T) {
 			},
 			input: &api.Service{
 				ObjectMeta: api.ObjectMeta{Name: "baz", Namespace: "test", ResourceVersion: "12"},
-				TypeMeta:   api.TypeMeta{Kind: "Service", APIVersion: "v1"},
 				Spec: api.ServiceSpec{
 					Selector: map[string]string{"app": "go"},
 				},
 			},
 			flags: map[string]string{"selector": "func=stream", "protocol": "UDP", "port": "14", "name": "foo", "labels": "svc=test"},
 			output: &api.Service{
-				ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: "test", ResourceVersion: "12", Labels: map[string]string{"svc": "test"}},
+				ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: "", Labels: map[string]string{"svc": "test"}},
-				TypeMeta:   api.TypeMeta{Kind: "Service", APIVersion: "v1"},
 				Spec: api.ServiceSpec{
 					Ports: []api.ServicePort{
 						{
-							Name:     "default",
+							Protocol:   api.ProtocolUDP,
-							Protocol: api.Protocol("UDP"),
 							Port:       14,
+							TargetPort: util.NewIntOrStringFromInt(14),
 						},
 					},
 					Selector: map[string]string{"func": "stream"},
 				},
 			},
+			expected: "service \"foo\" exposed",
 			status:   200,
 		},
 		{
@@ -116,7 +112,6 @@ func TestRunExposeService(t *testing.T) {
 			},
 			input: &api.Service{
 				ObjectMeta: api.ObjectMeta{Name: "mayor", Namespace: "default", ResourceVersion: "12"},
-				TypeMeta:   api.TypeMeta{Kind: "Service", APIVersion: "v1"},
 				Spec: api.ServiceSpec{
 					Selector: map[string]string{"run": "this"},
 				},
@@ -124,19 +119,19 @@ func TestRunExposeService(t *testing.T) {
 			// No --name flag specified below. Service will use the rc's name passed via the 'default-name' parameter
 			flags: map[string]string{"selector": "run=this", "port": "80", "labels": "runas=amayor"},
 			output: &api.Service{
-				ObjectMeta: api.ObjectMeta{Name: "mayor", Namespace: "default", ResourceVersion: "12", Labels: map[string]string{"runas": "amayor"}},
+				ObjectMeta: api.ObjectMeta{Name: "mayor", Namespace: "", Labels: map[string]string{"runas": "amayor"}},
-				TypeMeta:   api.TypeMeta{Kind: "Service", APIVersion: "v1"},
 				Spec: api.ServiceSpec{
 					Ports: []api.ServicePort{
 						{
-							Name:     "default",
+							Protocol:   api.ProtocolTCP,
-							Protocol: api.Protocol("TCP"),
 							Port:       80,
+							TargetPort: util.NewIntOrStringFromInt(80),
 						},
 					},
 					Selector: map[string]string{"run": "this"},
 				},
 			},
+			expected: "service \"mayor\" exposed",
 			status:   200,
 		},
 		{
@@ -149,20 +144,17 @@ func TestRunExposeService(t *testing.T) {
 			},
 			input: &api.Service{
 				ObjectMeta: api.ObjectMeta{Name: "baz", Namespace: "test", ResourceVersion: "12"},
-				TypeMeta:   api.TypeMeta{Kind: "Service", APIVersion: "v1"},
 				Spec: api.ServiceSpec{
 					Selector: map[string]string{"app": "go"},
 				},
 			},
 			flags: map[string]string{"selector": "func=stream", "protocol": "UDP", "port": "14", "name": "foo", "labels": "svc=test", "create-external-load-balancer": "true"},
 			output: &api.Service{
-				ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: "test", ResourceVersion: "12", Labels: map[string]string{"svc": "test"}},
+				ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: "", Labels: map[string]string{"svc": "test"}},
-				TypeMeta:   api.TypeMeta{Kind: "Service", APIVersion: "v1"},
 				Spec: api.ServiceSpec{
 					Ports: []api.ServicePort{
 						{
-							Name:       "default",
+							Protocol:   api.ProtocolUDP,
-							Protocol:   api.Protocol("UDP"),
 							Port:       14,
 							TargetPort: util.NewIntOrStringFromInt(14),
 						},
@@ -171,6 +163,7 @@ func TestRunExposeService(t *testing.T) {
 					Type:     api.ServiceTypeLoadBalancer,
 				},
 			},
+			expected: "service \"foo\" exposed",
 			status:   200,
 		},
 		{
@@ -183,20 +176,17 @@ func TestRunExposeService(t *testing.T) {
 			},
 			input: &api.Service{
 				ObjectMeta: api.ObjectMeta{Name: "baz", Namespace: "test", ResourceVersion: "12"},
-				TypeMeta:   api.TypeMeta{Kind: "Service", APIVersion: "v1"},
 				Spec: api.ServiceSpec{
 					Selector: map[string]string{"app": "go"},
 				},
 			},
 			flags: map[string]string{"selector": "func=stream", "protocol": "UDP", "port": "14", "name": "foo", "labels": "svc=test", "create-external-load-balancer": "true", "session-affinity": "ClientIP"},
 			output: &api.Service{
-				ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: "test", ResourceVersion: "12", Labels: map[string]string{"svc": "test"}},
+				ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: "", Labels: map[string]string{"svc": "test"}},
-				TypeMeta:   api.TypeMeta{Kind: "Service", APIVersion: "v1"},
 				Spec: api.ServiceSpec{
 					Ports: []api.ServicePort{
 						{
-							Name:       "default",
+							Protocol:   api.ProtocolUDP,
-							Protocol:   api.Protocol("UDP"),
 							Port:       14,
 							TargetPort: util.NewIntOrStringFromInt(14),
 						},
@@ -206,6 +196,38 @@ func TestRunExposeService(t *testing.T) {
 					SessionAffinity: api.ServiceAffinityClientIP,
 				},
 			},
+			expected: "service \"foo\" exposed",
+			status:   200,
+		},
+		{
+			name: "expose-external-service",
+			args: []string{"service", "baz"},
+			ns:   "test",
+			calls: map[string]string{
+				"GET":  "/namespaces/test/services/baz",
+				"POST": "/namespaces/test/services",
+			},
+			input: &api.Service{
+				ObjectMeta: api.ObjectMeta{Name: "baz", Namespace: "test", ResourceVersion: "12"},
+				Spec: api.ServiceSpec{
+					Ports: []api.ServicePort{},
+				},
+			},
+			// Even if we specify --selector, since service/test doesn't need one it will ignore it
+			flags: map[string]string{"selector": "svc=fromexternal", "port": "90", "labels": "svc=fromexternal", "name": "frombaz", "generator": "service/test"},
+			output: &api.Service{
+				ObjectMeta: api.ObjectMeta{Name: "frombaz", Namespace: "", Labels: map[string]string{"svc": "fromexternal"}},
+				Spec: api.ServiceSpec{
+					Ports: []api.ServicePort{
+						{
+							Protocol:   api.ProtocolTCP,
+							Port:       90,
+							TargetPort: util.NewIntOrStringFromInt(90),
+						},
+					},
+				},
+			},
+			expected: "service \"frombaz\" exposed",
 			status:   200,
 		},
 	}
@@ -228,8 +250,6 @@ func TestRunExposeService(t *testing.T) {
 			}),
 		}
 		tf.Namespace = test.ns
-		f.PodSelectorForObject = func(obj runtime.Object) (string, error) { return test.podSelector, nil }
-
 		buf := bytes.NewBuffer([]byte{})
 
 		cmd := NewCmdExposeService(f, buf)
@@ -238,11 +258,10 @@ func TestRunExposeService(t *testing.T) {
 			cmd.Flags().Set(flag, value)
 		}
 		cmd.Run(cmd, test.args)
-		if len(test.expected) > 0 {
+
-			out := buf.String()
+		out, expectedOut := buf.String(), test.expected
-			if !strings.Contains(out, test.expected) {
+		if !strings.Contains(out, expectedOut) {
-				t.Errorf("%s: unexpected output: %s", test.name, out)
+			t.Errorf("%s: Unexpected output! Expected\n%s\ngot\n%s", test.name, expectedOut, out)
-			}
 		}
 	}
 }