Merge pull request kubernetes#114909 from aimuz/fix-114402

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

staging/src/k8s.io/kubectl/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 {
-					protocol = exposeProtocol
+				if exposeProtocols, found := portProtocolMap[stillPortString]; found {
+					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
 }

staging/src/k8s.io/kubectl/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",

staging/src/k8s.io/kubectl/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)
 

staging/src/k8s.io/kubectl/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
+}