fix: kubectl expose fails for apps with same-port, different-protocol

Fixed: https://github.com/kubernetes/kubernetes/issues/114402
Signed-off-by: aimuz <mr.imuz@gmail.com>

Kubernetes-commit: e5116a39c96a78511fc8c0da4730d3262b2c121c

pkg/cmd/expose/expose.go

@@ -38,7 +38,6 @@ import (
 	"k8s.io/cli-runtime/pkg/printers"
 	"k8s.io/cli-runtime/pkg/resource"
 	cmdutil "k8s.io/kubectl/pkg/cmd/util"
-	"k8s.io/kubectl/pkg/generate"
 	"k8s.io/kubectl/pkg/polymorphichelpers"
 	"k8s.io/kubectl/pkg/scheme"
 	"k8s.io/kubectl/pkg/util"
@@ -123,7 +122,7 @@ type ExposeServiceOptions struct {
 	CanBeExposed              polymorphichelpers.CanBeExposedFunc
 	MapBasedSelectorForObject func(runtime.Object) (string, error)
 	PortsForObject            polymorphichelpers.PortsForObjectFunc
-	ProtocolsForObject        func(runtime.Object) (map[string]string, error)
+	ProtocolsForObject        polymorphichelpers.MultiProtocolsWithForObjectFunc
 
 	Namespace string
 	Mapper    meta.RESTMapper
@@ -276,7 +275,7 @@ func (o *ExposeServiceOptions) Complete(f cmdutil.Factory) error {
 	o.ClientForMapping = f.ClientForMapping
 	o.CanBeExposed = polymorphichelpers.CanBeExposedFn
 	o.MapBasedSelectorForObject = polymorphichelpers.MapBasedSelectorForObjectFn
-	o.ProtocolsForObject = polymorphichelpers.ProtocolsForObjectFn
+	o.ProtocolsForObject = polymorphichelpers.MultiProtocolsForObjectFn
 	o.PortsForObject = polymorphichelpers.PortsForObjectFn
 
 	o.Mapper, err = f.ToRESTMapper()
@@ -361,7 +360,7 @@ func (o *ExposeServiceOptions) RunExpose(cmd *cobra.Command, args []string) erro
 		if err != nil {
 			return fmt.Errorf("couldn't find protocol via introspection: %v", err)
 		}
-		if protocols := generate.MakeProtocols(protocolsMap); !generate.IsZero(protocols) {
+		if protocols := makeProtocols(protocolsMap); len(protocols) > 0 {
 			o.Protocols = protocols
 		}
 
@@ -375,13 +374,11 @@ func (o *ExposeServiceOptions) RunExpose(cmd *cobra.Command, args []string) erro
 
 		// Generate new object
 		service, err := o.createService()
-
 		if err != nil {
 			return err
 		}
 
 		overrideService, err := o.NewOverrider(&corev1.Service{}).Apply(service)
-
 		if err != nil {
 			return err
 		}
@@ -455,7 +452,7 @@ func (o *ExposeServiceOptions) createService() (*corev1.Service, error) {
 		}
 	}
 
-	var portProtocolMap map[string]string
+	var portProtocolMap map[string][]string
 	if o.Protocols != "" {
 		portProtocolMap, err = parseProtocols(o.Protocols)
 		if err != nil {
@@ -499,8 +496,20 @@ func (o *ExposeServiceOptions) createService() (*corev1.Service, error) {
 			case len(protocol) == 0 && len(portProtocolMap) > 0:
 				// no --protocol and we expose a multiprotocol resource
 				protocol = "TCP" // have the default so we can stay sane
-				if exposeProtocol, found := portProtocolMap[stillPortString]; found {
+				if exposeProtocols, found := portProtocolMap[stillPortString]; found {
-					protocol = exposeProtocol
+					if len(exposeProtocols) == 1 {
+						protocol = exposeProtocols[0]
+						break
+					}
+					for _, exposeProtocol := range exposeProtocols {
+						name := fmt.Sprintf("port-%d-%s", i+1, strings.ToLower(exposeProtocol))
+						ports = append(ports, corev1.ServicePort{
+							Name:     name,
+							Port:     int32(port),
+							Protocol: corev1.Protocol(exposeProtocol),
+						})
+					}
+					continue
 				}
 			}
 			ports = append(ports, corev1.ServicePort{
@@ -590,12 +599,22 @@ func parseLabels(labelSpec string) (map[string]string, error) {
 	return labels, nil
 }
 
+func makeProtocols(protocols map[string][]string) string {
+	var out []string
+	for key, value := range protocols {
+		for _, s := range value {
+			out = append(out, fmt.Sprintf("%s/%s", key, s))
+		}
+	}
+	return strings.Join(out, ",")
+}
+
 // parseProtocols turns a string representation of a protocols set into a map[string]string
-func parseProtocols(protocols string) (map[string]string, error) {
+func parseProtocols(protocols string) (map[string][]string, error) {
 	if len(protocols) == 0 {
 		return nil, fmt.Errorf("no protocols passed")
 	}
-	portProtocolMap := map[string]string{}
+	portProtocolMap := map[string][]string{}
 	protocolsSlice := strings.Split(protocols, ",")
 	for ix := range protocolsSlice {
 		portProtocol := strings.Split(protocolsSlice[ix], "/")
@@ -608,7 +627,8 @@ func parseProtocols(protocols string) (map[string]string, error) {
 		if len(portProtocol[1]) == 0 {
 			return nil, fmt.Errorf("unexpected empty protocol")
 		}
-		portProtocolMap[portProtocol[0]] = portProtocol[1]
+		port := portProtocol[0]
+		portProtocolMap[port] = append(portProtocolMap[port], portProtocol[1])
 	}
 	return portProtocolMap, nil
 }

pkg/cmd/expose/expose_test.go

@@ -1688,6 +1688,40 @@ func TestGenerateService(t *testing.T) {
 				},
 			},
 		},
+		// Fixed #114402 kubectl expose fails for apps with same-port, different-protocol
+		"test #114402": {
+			selector:  "foo=bar,baz=blah",
+			name:      "test",
+			clusterIP: "None",
+			protocols: "53/TCP,53/UDP",
+			port:      "53",
+			expected: &corev1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+				},
+				Spec: corev1.ServiceSpec{
+					Selector: map[string]string{
+						"foo": "bar",
+						"baz": "blah",
+					},
+					Ports: []corev1.ServicePort{
+						{
+							Name:       "port-1-tcp",
+							Port:       53,
+							Protocol:   corev1.ProtocolTCP,
+							TargetPort: intstr.FromInt(53),
+						},
+						{
+							Name:       "port-1-udp",
+							Port:       53,
+							Protocol:   corev1.ProtocolUDP,
+							TargetPort: intstr.FromInt(53),
+						},
+					},
+					ClusterIP: corev1.ClusterIPNone,
+				},
+			},
+		},
 		"check selector": {
 			name:       "test",
 			protocol:   "SCTP",

pkg/polymorphichelpers/interface.go

@@ -67,11 +67,21 @@ var MapBasedSelectorForObjectFn MapBasedSelectorForObjectFunc = mapBasedSelector
 
 // ProtocolsForObjectFunc will call the provided function on the protocols for the object,
 // return nil-map if no protocols for the object, or return an error.
+// Deprecated: use PortsProtocolsForObjectFunc instead.
+// When the same port has different protocols, data will be lost
 type ProtocolsForObjectFunc func(object runtime.Object) (map[string]string, error)
 
 // ProtocolsForObjectFn gives a way to easily override the function for unit testing if needed
+// Deprecated: use MultiProtocolsForObjectFn instead.
 var ProtocolsForObjectFn ProtocolsForObjectFunc = protocolsForObject
 
+// MultiProtocolsWithForObjectFunc will call the provided function on the protocols for the object,
+// return nil-map if no protocols for the object, or return an error.
+type MultiProtocolsWithForObjectFunc func(object runtime.Object) (map[string][]string, error)
+
+// MultiProtocolsForObjectFn gives a way to easily override the function for unit testing if needed
+var MultiProtocolsForObjectFn MultiProtocolsWithForObjectFunc = multiProtocolsForObject
+
 // PortsForObjectFunc returns the ports associated with the provided object
 type PortsForObjectFunc func(object runtime.Object) ([]string, error)
 

pkg/polymorphichelpers/multiprotocolsforobject.go

@@ -0,0 +1,95 @@
+/*
+Copyright 2018 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package polymorphichelpers
+
+import (
+	"fmt"
+	"strconv"
+
+	appsv1 "k8s.io/api/apps/v1"
+	appsv1beta1 "k8s.io/api/apps/v1beta1"
+	appsv1beta2 "k8s.io/api/apps/v1beta2"
+	corev1 "k8s.io/api/core/v1"
+	extensionsv1beta1 "k8s.io/api/extensions/v1beta1"
+	"k8s.io/apimachinery/pkg/runtime"
+)
+
+func multiProtocolsForObject(object runtime.Object) (map[string][]string, error) {
+	// TODO: replace with a swagger schema based approach (identify pod selector via schema introspection)
+	switch t := object.(type) {
+	case *corev1.ReplicationController:
+		return getMultiProtocols(t.Spec.Template.Spec), nil
+
+	case *corev1.Pod:
+		return getMultiProtocols(t.Spec), nil
+
+	case *corev1.Service:
+		return getServiceMultiProtocols(t.Spec), nil
+
+	case *extensionsv1beta1.Deployment:
+		return getMultiProtocols(t.Spec.Template.Spec), nil
+	case *appsv1.Deployment:
+		return getMultiProtocols(t.Spec.Template.Spec), nil
+	case *appsv1beta2.Deployment:
+		return getMultiProtocols(t.Spec.Template.Spec), nil
+	case *appsv1beta1.Deployment:
+		return getMultiProtocols(t.Spec.Template.Spec), nil
+
+	case *extensionsv1beta1.ReplicaSet:
+		return getMultiProtocols(t.Spec.Template.Spec), nil
+	case *appsv1.ReplicaSet:
+		return getMultiProtocols(t.Spec.Template.Spec), nil
+	case *appsv1beta2.ReplicaSet:
+		return getMultiProtocols(t.Spec.Template.Spec), nil
+
+	default:
+		return nil, fmt.Errorf("cannot extract protocols from %T", object)
+	}
+}
+
+func getMultiProtocols(spec corev1.PodSpec) map[string][]string {
+	result := make(map[string][]string)
+	var protocol corev1.Protocol
+	for _, container := range spec.Containers {
+		for _, port := range container.Ports {
+			// Empty protocol must be defaulted (TCP)
+			protocol = corev1.ProtocolTCP
+			if len(port.Protocol) > 0 {
+				protocol = port.Protocol
+			}
+			p := strconv.Itoa(int(port.ContainerPort))
+			result[p] = append(result[p], string(protocol))
+		}
+	}
+	return result
+}
+
+// Extracts the protocols exposed by a service from the given service spec.
+func getServiceMultiProtocols(spec corev1.ServiceSpec) map[string][]string {
+	result := make(map[string][]string)
+	var protocol corev1.Protocol
+	for _, servicePort := range spec.Ports {
+		// Empty protocol must be defaulted (TCP)
+		protocol = corev1.ProtocolTCP
+		if len(servicePort.Protocol) > 0 {
+			protocol = servicePort.Protocol
+		}
+		p := strconv.Itoa(int(servicePort.Port))
+		result[p] = append(result[p], string(protocol))
+	}
+	return result
+}

pkg/polymorphichelpers/multiprotocolsforobject_test.go

@@ -0,0 +1,213 @@
+/*
+Copyright 2018 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package polymorphichelpers
+
+import (
+	"reflect"
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+	extensionsv1beta1 "k8s.io/api/extensions/v1beta1"
+	"k8s.io/apimachinery/pkg/runtime"
+)
+
+func TestMultiProtocolsForObject(t *testing.T) {
+	tests := []struct {
+		name      string
+		object    runtime.Object
+		expectErr bool
+		expected  map[string][]string
+	}{
+		{
+			name: "pod with TCP protocol",
+			object: &corev1.Pod{
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{
+						{
+							Ports: []corev1.ContainerPort{
+								{
+									ContainerPort: 101,
+									Protocol:      "TCP",
+								},
+							},
+						},
+					},
+				},
+			},
+			expected: map[string][]string{"101": {"TCP"}},
+		},
+		// No protocol--should default to TCP.
+		{
+			name: "pod with no protocol",
+			object: &corev1.Pod{
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{
+						{
+							Ports: []corev1.ContainerPort{
+								{
+									ContainerPort: 101,
+								},
+							},
+						},
+					},
+				},
+			},
+			expected: map[string][]string{"101": {"TCP"}},
+		},
+		{
+			name: "pod with same-port,different-protocol",
+			object: &corev1.Pod{
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{
+						{
+							Ports: []corev1.ContainerPort{
+								{
+									ContainerPort: 101,
+									Protocol:      "TCP",
+								},
+								{
+									ContainerPort: 101,
+									Protocol:      "UDP",
+								},
+							},
+						},
+					},
+				},
+			},
+			expected: map[string][]string{"101": {"TCP", "UDP"}},
+		},
+		{
+			name: "service with TCP protocol",
+			object: &corev1.Service{
+				Spec: corev1.ServiceSpec{
+					Ports: []corev1.ServicePort{
+						{
+							Port:     101,
+							Protocol: "TCP",
+						},
+					},
+				},
+			},
+			expected: map[string][]string{"101": {"TCP"}},
+		},
+		// No protocol for service port--default to TCP
+		{
+			name: "service with no protocol",
+			object: &corev1.Service{
+				Spec: corev1.ServiceSpec{
+					Ports: []corev1.ServicePort{
+						{
+							Port: 101,
+						},
+					},
+				},
+			},
+			expected: map[string][]string{"101": {"TCP"}},
+		},
+		{
+			name: "replication with TCP protocol",
+			object: &corev1.ReplicationController{
+				Spec: corev1.ReplicationControllerSpec{
+					Template: &corev1.PodTemplateSpec{
+						Spec: corev1.PodSpec{
+							Containers: []corev1.Container{
+								{
+									Ports: []corev1.ContainerPort{
+										{
+											ContainerPort: 101,
+											Protocol:      "TCP",
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			expected: map[string][]string{"101": {"TCP"}},
+		},
+		{
+			name: "deployment with TCP protocol",
+			object: &extensionsv1beta1.Deployment{
+				Spec: extensionsv1beta1.DeploymentSpec{
+					Template: corev1.PodTemplateSpec{
+						Spec: corev1.PodSpec{
+							Containers: []corev1.Container{
+								{
+									Ports: []corev1.ContainerPort{
+										{
+											ContainerPort: 101,
+											Protocol:      "TCP",
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			expected: map[string][]string{"101": {"TCP"}},
+		},
+		{
+			name: "replicaset with TCP protocol",
+			object: &extensionsv1beta1.ReplicaSet{
+				Spec: extensionsv1beta1.ReplicaSetSpec{
+					Template: corev1.PodTemplateSpec{
+						Spec: corev1.PodSpec{
+							Containers: []corev1.Container{
+								{
+									Ports: []corev1.ContainerPort{
+										{
+											ContainerPort: 101,
+											Protocol:      "TCP",
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			expected: map[string][]string{"101": {"TCP"}},
+		},
+		{
+			name:      "unsupported object",
+			object:    &corev1.Node{},
+			expectErr: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			actual, err := multiProtocolsForObject(test.object)
+			if test.expectErr {
+				if err == nil {
+					t.Error("unexpected non-error")
+				}
+				return
+			}
+			if !test.expectErr && err != nil {
+				t.Errorf("unexpected error: %v", err)
+				return
+			}
+			if !reflect.DeepEqual(actual, test.expected) {
+				t.Errorf("expected ports %v, but got %v", test.expected, actual)
+			}
+		})
+
+	}
+}

pkg/polymorphichelpers/portsforobject.go

@@ -60,19 +60,31 @@ func portsForObject(object runtime.Object) ([]string, error) {
 }
 
 func getPorts(spec corev1.PodSpec) []string {
-	result := []string{}
+	var result []string
+	exists := map[string]struct{}{}
 	for _, container := range spec.Containers {
 		for _, port := range container.Ports {
-			result = append(result, strconv.Itoa(int(port.ContainerPort)))
+			// remove duplicate ports
+			key := strconv.Itoa(int(port.ContainerPort))
+			if _, ok := exists[key]; !ok {
+				exists[key] = struct{}{}
+				result = append(result, key)
+			}
 		}
 	}
 	return result
 }
 
 func getServicePorts(spec corev1.ServiceSpec) []string {
-	result := []string{}
+	var result []string
+	exists := map[string]struct{}{}
 	for _, servicePort := range spec.Ports {
-		result = append(result, strconv.Itoa(int(servicePort.Port)))
+		// remove duplicate ports
+		key := strconv.Itoa(int(servicePort.Port))
+		if _, ok := exists[key]; !ok {
+			exists[key] = struct{}{}
+			result = append(result, key)
+		}
 	}
 	return result
 }

pkg/polymorphichelpers/portsforobject_test.go

@@ -30,6 +30,7 @@ func TestPortsForObject(t *testing.T) {
 	tests := []struct {
 		object    runtime.Object
 		expectErr bool
+		expected  []string
 	}{
 		{
 			object: &corev1.Pod{
@@ -45,6 +46,74 @@ func TestPortsForObject(t *testing.T) {
 					},
 				},
 			},
+			expected: []string{"101"},
+		},
+		{
+			object: &corev1.Pod{
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{
+						{
+							Ports: []corev1.ContainerPort{
+								{
+									ContainerPort: 101,
+								},
+								{
+									ContainerPort: 102,
+								},
+							},
+						},
+					},
+				},
+			},
+			expected: []string{"101", "102"},
+		},
+		{
+			object: &corev1.Pod{
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{
+						{
+							Ports: []corev1.ContainerPort{
+								{
+									ContainerPort: 101,
+									Protocol:      corev1.ProtocolTCP,
+								},
+								{
+									ContainerPort: 101,
+									Protocol:      corev1.ProtocolUDP,
+								},
+								{
+									ContainerPort: 102,
+								},
+							},
+						},
+					},
+				},
+			},
+			expected: []string{"101", "102"},
+		},
+		{
+			object: &corev1.Pod{
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{
+						{
+							Ports: []corev1.ContainerPort{
+								{
+									ContainerPort: 101,
+									Protocol:      corev1.ProtocolTCP,
+								},
+								{
+									ContainerPort: 102,
+								},
+								{
+									ContainerPort: 101,
+									Protocol:      corev1.ProtocolUDP,
+								},
+							},
+						},
+					},
+				},
+			},
+			expected: []string{"101", "102"},
 		},
 		{
 			object: &corev1.Service{
@@ -56,6 +125,7 @@ func TestPortsForObject(t *testing.T) {
 					},
 				},
 			},
+			expected: []string{"101"},
 		},
 		{
 			object: &corev1.ReplicationController{
@@ -75,6 +145,7 @@ func TestPortsForObject(t *testing.T) {
 					},
 				},
 			},
+			expected: []string{"101"},
 		},
 		{
 			object: &extensionsv1beta1.Deployment{
@@ -94,6 +165,7 @@ func TestPortsForObject(t *testing.T) {
 					},
 				},
 			},
+			expected: []string{"101"},
 		},
 		{
 			object: &extensionsv1beta1.ReplicaSet{
@@ -113,13 +185,13 @@ func TestPortsForObject(t *testing.T) {
 					},
 				},
 			},
+			expected: []string{"101"},
 		},
 		{
 			object:    &corev1.Node{},
 			expectErr: true,
 		},
 	}
-	expectedPorts := []string{"101"}
 
 	for _, test := range tests {
 		actual, err := portsForObject(test.object)
@@ -133,8 +205,8 @@ func TestPortsForObject(t *testing.T) {
 			t.Errorf("unexpected error: %v", err)
 			continue
 		}
-		if !reflect.DeepEqual(actual, expectedPorts) {
+		if !reflect.DeepEqual(actual, test.expected) {
-			t.Errorf("expected ports %v, but got %v", expectedPorts, actual)
+			t.Errorf("expected ports %v, but got %v", test.expected, actual)
 		}
 	}
 }