From e5116a39c96a78511fc8c0da4730d3262b2c121c Mon Sep 17 00:00:00 2001 From: aimuz Date: Mon, 19 Jun 2023 09:58:54 +0800 Subject: [PATCH] fix: kubectl expose fails for apps with same-port, different-protocol Fixed: https://github.com/kubernetes/kubernetes/issues/114402 Signed-off-by: aimuz --- .../k8s.io/kubectl/pkg/cmd/expose/expose.go | 44 +++- .../kubectl/pkg/cmd/expose/expose_test.go | 34 +++ .../pkg/polymorphichelpers/interface.go | 10 + .../multiprotocolsforobject.go | 95 ++++++++ .../multiprotocolsforobject_test.go | 213 ++++++++++++++++++ .../pkg/polymorphichelpers/portsforobject.go | 20 +- .../polymorphichelpers/portsforobject_test.go | 78 ++++++- 7 files changed, 475 insertions(+), 19 deletions(-) create mode 100644 staging/src/k8s.io/kubectl/pkg/polymorphichelpers/multiprotocolsforobject.go create mode 100644 staging/src/k8s.io/kubectl/pkg/polymorphichelpers/multiprotocolsforobject_test.go diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/expose/expose.go b/staging/src/k8s.io/kubectl/pkg/cmd/expose/expose.go index 81e47c1c5d2..183cd8786e9 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/expose/expose.go +++ b/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 @@ -219,7 +218,7 @@ func (o *ExposeServiceOptions) Complete(f cmdutil.Factory, cmd *cobra.Command) e 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() @@ -304,7 +303,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 } @@ -318,13 +317,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 } @@ -399,7 +396,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 { @@ -443,8 +440,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{ @@ -534,12 +543,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], "/") @@ -552,7 +571,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 } diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/expose/expose_test.go b/staging/src/k8s.io/kubectl/pkg/cmd/expose/expose_test.go index e954671e521..f8b20067651 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/expose/expose_test.go +++ b/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", diff --git a/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/interface.go b/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/interface.go index 83e13714f0c..89c4e9facf6 100644 --- a/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/interface.go +++ b/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) diff --git a/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/multiprotocolsforobject.go b/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/multiprotocolsforobject.go new file mode 100644 index 00000000000..9d161aac30f --- /dev/null +++ b/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 +} diff --git a/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/multiprotocolsforobject_test.go b/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/multiprotocolsforobject_test.go new file mode 100644 index 00000000000..dd128998206 --- /dev/null +++ b/staging/src/k8s.io/kubectl/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) + } + }) + + } +} diff --git a/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/portsforobject.go b/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/portsforobject.go index 6cc9a2a4e51..c0a8cb6bff6 100644 --- a/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/portsforobject.go +++ b/staging/src/k8s.io/kubectl/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 } diff --git a/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/portsforobject_test.go b/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/portsforobject_test.go index da5f21e6c01..79cd3d61c5e 100644 --- a/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/portsforobject_test.go +++ b/staging/src/k8s.io/kubectl/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) { - t.Errorf("expected ports %v, but got %v", expectedPorts, actual) + if !reflect.DeepEqual(actual, test.expected) { + t.Errorf("expected ports %v, but got %v", test.expected, actual) } } }