From ad97cddb3ed0020788742ccdd312e28ca7f1cdff Mon Sep 17 00:00:00 2001 From: AdoHe Date: Thu, 12 May 2016 00:07:07 -0400 Subject: [PATCH] fix expose multi protocols issue --- docs/man/man1/kubectl-expose.1 | 4 +- docs/user-guide/kubectl/kubectl_expose.md | 4 +- docs/yaml/kubectl/kubectl_expose.yaml | 3 +- pkg/kubectl/cmd/cmd_test.go | 1 + pkg/kubectl/cmd/expose.go | 15 ++++- pkg/kubectl/cmd/expose_test.go | 59 +++++++++++++++++++ pkg/kubectl/cmd/util/factory.go | 42 +++++++++++++ pkg/kubectl/cmd/util/factory_test.go | 42 +++++++++++++ pkg/kubectl/generate.go | 28 +++++++++ pkg/kubectl/service.go | 31 +++++++++- pkg/kubectl/service_test.go | 72 +++++++++++++++++++++++ 11 files changed, 293 insertions(+), 8 deletions(-) diff --git a/docs/man/man1/kubectl-expose.1 b/docs/man/man1/kubectl-expose.1 index 4cca5d8407d..afc6104a940 100644 --- a/docs/man/man1/kubectl-expose.1 +++ b/docs/man/man1/kubectl-expose.1 @@ -89,8 +89,8 @@ Possible resources include (case insensitive): The port that the service should serve on. Copied from the resource being exposed, if unspecified .PP -\fB\-\-protocol\fP="TCP" - The network protocol for the service to be created. Default is 'tcp'. +\fB\-\-protocol\fP="" + The network protocol for the service to be created. Default is 'TCP'. .PP \fB\-\-record\fP=false diff --git a/docs/user-guide/kubectl/kubectl_expose.md b/docs/user-guide/kubectl/kubectl_expose.md index fc06ef603df..5e391d5ebcc 100644 --- a/docs/user-guide/kubectl/kubectl_expose.md +++ b/docs/user-guide/kubectl/kubectl_expose.md @@ -97,7 +97,7 @@ kubectl expose deployment nginx --port=80 --target-port=8000 --output-version="": Output the formatted object with the given group version (for ex: 'extensions/v1beta1'). --overrides="": An inline JSON override for the generated object. If this is non-empty, it is used to override the generated object. Requires that the object supply a valid apiVersion field. --port="": The port that the service should serve on. Copied from the resource being exposed, if unspecified - --protocol="TCP": The network protocol for the service to be created. Default is 'tcp'. + --protocol="": The network protocol for the service to be created. Default is 'TCP'. --record[=false]: Record current kubectl command in the resource annotation. -R, --recursive[=false]: If true, process directory recursively. --save-config[=false]: If true, the configuration of current object will be saved in its annotation. This is useful when you want to perform kubectl apply on this object in the future. @@ -143,7 +143,7 @@ kubectl expose deployment nginx --port=80 --target-port=8000 * [kubectl](kubectl.md) - kubectl controls the Kubernetes cluster manager -###### Auto generated by spf13/cobra on 3-May-2016 +###### Auto generated by spf13/cobra on 11-May-2016 [![Analytics](https://kubernetes-site.appspot.com/UA-36037335-10/GitHub/docs/user-guide/kubectl/kubectl_expose.md?pixel)]() diff --git a/docs/yaml/kubectl/kubectl_expose.yaml b/docs/yaml/kubectl/kubectl_expose.yaml index 138f95afb99..608d1fdec3d 100644 --- a/docs/yaml/kubectl/kubectl_expose.yaml +++ b/docs/yaml/kubectl/kubectl_expose.yaml @@ -59,9 +59,8 @@ options: usage: | The port that the service should serve on. Copied from the resource being exposed, if unspecified - name: protocol - default_value: TCP usage: | - The network protocol for the service to be created. Default is 'tcp'. + The network protocol for the service to be created. Default is 'TCP'. - name: record default_value: "false" usage: Record current kubectl command in the resource annotation. diff --git a/pkg/kubectl/cmd/cmd_test.go b/pkg/kubectl/cmd/cmd_test.go index 500b7a7d242..965c226fc7b 100644 --- a/pkg/kubectl/cmd/cmd_test.go +++ b/pkg/kubectl/cmd/cmd_test.go @@ -315,6 +315,7 @@ func NewAPIFactory() (*cmdutil.Factory, *testFactory, runtime.Codec) { rf := cmdutil.NewFactory(nil) f.MapBasedSelectorForObject = rf.MapBasedSelectorForObject f.PortsForObject = rf.PortsForObject + f.ProtocolsForObject = rf.ProtocolsForObject f.LabelsForObject = rf.LabelsForObject f.CanBeExposed = rf.CanBeExposed return f, t, testapi.Default.Codec() diff --git a/pkg/kubectl/cmd/expose.go b/pkg/kubectl/cmd/expose.go index 068a624fcf2..82251e541c7 100644 --- a/pkg/kubectl/cmd/expose.go +++ b/pkg/kubectl/cmd/expose.go @@ -101,7 +101,7 @@ func NewCmdExposeService(f *cmdutil.Factory, out io.Writer) *cobra.Command { } cmdutil.AddPrinterFlags(cmd) cmd.Flags().String("generator", "service/v2", "The name of the API generator to use. There are 2 generators: 'service/v1' and 'service/v2'. The only difference between them is that service port in v1 is named 'default', while it is left unnamed in v2. Default is 'service/v2'.") - cmd.Flags().String("protocol", "TCP", "The network protocol for the service to be created. Default is 'tcp'.") + cmd.Flags().String("protocol", "", "The network protocol for the service to be created. Default is 'TCP'.") cmd.Flags().String("port", "", "The port that the service should serve on. Copied from the resource being exposed, if unspecified") cmd.Flags().String("type", "", "Type for this service: ClusterIP, NodePort, or LoadBalancer. Default is 'ClusterIP'.") // TODO: remove create-external-load-balancer in code on or after Aug 25, 2016. @@ -198,6 +198,19 @@ func RunExpose(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []str params["ports"] = strings.Join(ports, ",") } } + + // Always try to derive protocols from the exposed object, may use + // different protocols for different ports. + if _, found := params["protocol"]; found { + protocolsMap, err := f.ProtocolsForObject(info.Object) + if err != nil { + return cmdutil.UsageError(cmd, fmt.Sprintf("couldn't find protocol via introspection: %s", err)) + } + if protocols := kubectl.MakeProtocols(protocolsMap); !kubectl.IsZero(protocols) { + params["protocols"] = protocols + } + } + if kubectl.IsZero(params["labels"]) { labels, err := f.LabelsForObject(info.Object) if err != nil { diff --git a/pkg/kubectl/cmd/expose_test.go b/pkg/kubectl/cmd/expose_test.go index 6bad24357f4..0ce314c06a3 100644 --- a/pkg/kubectl/cmd/expose_test.go +++ b/pkg/kubectl/cmd/expose_test.go @@ -304,6 +304,65 @@ func TestRunExposeService(t *testing.T) { }, status: 200, }, + { + name: "expose-multiprotocol-object", + args: []string{"service", "foo"}, + ns: "test", + calls: map[string]string{ + "GET": "/namespaces/test/services/foo", + "POST": "/namespaces/test/services", + }, + input: &api.Service{ + ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: "", Labels: map[string]string{"svc": "multiport"}}, + Spec: api.ServiceSpec{ + Ports: []api.ServicePort{ + { + Protocol: api.ProtocolTCP, + Port: 80, + TargetPort: intstr.FromInt(80), + }, + { + Protocol: api.ProtocolUDP, + Port: 8080, + TargetPort: intstr.FromInt(8080), + }, + { + Protocol: api.ProtocolUDP, + Port: 8081, + TargetPort: intstr.FromInt(8081), + }, + }, + }, + }, + flags: map[string]string{"selector": "svc=fromfoo", "generator": "service/v2", "name": "fromfoo", "dry-run": "true"}, + output: &api.Service{ + ObjectMeta: api.ObjectMeta{Name: "fromfoo", Namespace: "", Labels: map[string]string{"svc": "multiport"}}, + Spec: api.ServiceSpec{ + Ports: []api.ServicePort{ + { + Name: "port-1", + Protocol: api.ProtocolTCP, + Port: 80, + TargetPort: intstr.FromInt(80), + }, + { + Name: "port-2", + Protocol: api.ProtocolUDP, + Port: 8080, + TargetPort: intstr.FromInt(8080), + }, + { + Name: "port-3", + Protocol: api.ProtocolUDP, + Port: 8081, + TargetPort: intstr.FromInt(8081), + }, + }, + Selector: map[string]string{"svc": "fromfoo"}, + }, + }, + status: 200, + }, } for _, test := range tests { diff --git a/pkg/kubectl/cmd/util/factory.go b/pkg/kubectl/cmd/util/factory.go index 2cdeba14a56..b4a7df9a534 100644 --- a/pkg/kubectl/cmd/util/factory.go +++ b/pkg/kubectl/cmd/util/factory.go @@ -109,6 +109,8 @@ type Factory struct { MapBasedSelectorForObject func(object runtime.Object) (string, error) // PortsForObject returns the ports associated with the provided object PortsForObject func(object runtime.Object) ([]string, error) + // ProtocolsForObject returns the mapping associated with the provided object + ProtocolsForObject func(object runtime.Object) (map[string]string, error) // LabelsForObject returns the labels associated with the provided object LabelsForObject func(object runtime.Object) (map[string]string, error) // LogsForObject returns a request for the logs associated with the provided object @@ -423,6 +425,27 @@ func NewFactory(optionalClientConfig clientcmd.ClientConfig) *Factory { return nil, fmt.Errorf("cannot extract ports from %v", gvk) } }, + ProtocolsForObject: func(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 *api.ReplicationController: + return getProtocols(t.Spec.Template.Spec), nil + case *api.Pod: + return getProtocols(t.Spec), nil + case *api.Service: + return getServiceProtocols(t.Spec), nil + case *extensions.Deployment: + return getProtocols(t.Spec.Template.Spec), nil + case *extensions.ReplicaSet: + return getProtocols(t.Spec.Template.Spec), nil + default: + gvk, err := api.Scheme.ObjectKind(object) + if err != nil { + return nil, err + } + return nil, fmt.Errorf("cannot extract protocols from %v", gvk) + } + }, LabelsForObject: func(object runtime.Object) (map[string]string, error) { return meta.NewAccessor().Labels(object) }, @@ -744,6 +767,16 @@ func getPorts(spec api.PodSpec) []string { return result } +func getProtocols(spec api.PodSpec) map[string]string { + result := make(map[string]string) + for _, container := range spec.Containers { + for _, port := range container.Ports { + result[strconv.Itoa(int(port.ContainerPort))] = string(port.Protocol) + } + } + return result +} + // Extracts the ports exposed by a service from the given service spec. func getServicePorts(spec api.ServiceSpec) []string { result := []string{} @@ -753,6 +786,15 @@ func getServicePorts(spec api.ServiceSpec) []string { return result } +// Extracts the protocols exposed by a service from the given service spec. +func getServiceProtocols(spec api.ServiceSpec) map[string]string { + result := make(map[string]string) + for _, servicePort := range spec.Ports { + result[strconv.Itoa(int(servicePort.Port))] = string(servicePort.Protocol) + } + return result +} + type clientSwaggerSchema struct { c *client.Client cacheDir string diff --git a/pkg/kubectl/cmd/util/factory_test.go b/pkg/kubectl/cmd/util/factory_test.go index 07fb3a7fd6e..7645164d0ac 100644 --- a/pkg/kubectl/cmd/util/factory_test.go +++ b/pkg/kubectl/cmd/util/factory_test.go @@ -102,6 +102,48 @@ func TestPortsForObject(t *testing.T) { } } +func TestProtocolsForObject(t *testing.T) { + f := NewFactory(nil) + + pod := &api.Pod{ + ObjectMeta: api.ObjectMeta{Name: "baz", Namespace: "test", ResourceVersion: "12"}, + Spec: api.PodSpec{ + Containers: []api.Container{ + { + Ports: []api.ContainerPort{ + { + ContainerPort: 101, + Protocol: api.ProtocolTCP, + }, + { + ContainerPort: 102, + Protocol: api.ProtocolUDP, + }, + }, + }, + }, + }, + } + + expected := "101/TCP,102/UDP" + protocolsMap, err := f.ProtocolsForObject(pod) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + got := kubectl.MakeProtocols(protocolsMap) + expectedSlice := strings.Split(expected, ",") + gotSlice := strings.Split(got, ",") + + sort.Strings(expectedSlice) + sort.Strings(gotSlice) + + for i, protocol := range gotSlice { + if protocol != expectedSlice[i] { + t.Fatalf("Protocols mismatch! Expected %s, got %s", expectedSlice[i], protocol) + } + } +} + func TestLabelsForObject(t *testing.T) { f := NewFactory(nil) diff --git a/pkg/kubectl/generate.go b/pkg/kubectl/generate.go index 6a71b619d80..ea254bcb548 100644 --- a/pkg/kubectl/generate.go +++ b/pkg/kubectl/generate.go @@ -134,6 +134,34 @@ func MakeParams(cmd *cobra.Command, params []GeneratorParam) map[string]interfac return result } +func MakeProtocols(protocols map[string]string) string { + out := []string{} + for key, value := range protocols { + out = append(out, fmt.Sprintf("%s/%s", key, value)) + } + return strings.Join(out, ",") +} + +func ParseProtocols(protocols interface{}) (map[string]string, error) { + protocolsString, isString := protocols.(string) + if !isString { + return nil, fmt.Errorf("expected string, found %v", protocols) + } + if len(protocolsString) == 0 { + return nil, fmt.Errorf("no protocols passed") + } + portProtocolMap := map[string]string{} + protocolsSlice := strings.Split(protocolsString, ",") + for ix := range protocolsSlice { + portProtocol := strings.Split(protocolsSlice[ix], "/") + if len(portProtocol) != 2 { + return nil, fmt.Errorf("unexpected port protocol mapping: %s", protocolsSlice[ix]) + } + portProtocolMap[portProtocol[0]] = portProtocol[1] + } + return portProtocolMap, nil +} + func MakeLabels(labels map[string]string) string { out := []string{} for key, value := range labels { diff --git a/pkg/kubectl/service.go b/pkg/kubectl/service.go index 2ffd72af6f7..f67b3a11b49 100644 --- a/pkg/kubectl/service.go +++ b/pkg/kubectl/service.go @@ -65,6 +65,9 @@ func paramNames() []GeneratorParam { {"load-balancer-ip", false}, {"type", false}, {"protocol", false}, + // protocols will be used to keep port-protocol mapping derived from + // exposed object + {"protocols", false}, {"container-port", false}, // alias of target-port {"target-port", false}, {"port-name", false}, @@ -112,6 +115,15 @@ func generate(genericParams map[string]interface{}) (runtime.Object, error) { // Leave the port unnamed. servicePortName = "" } + + protocolsString, found := params["protocols"] + var portProtocolMap map[string]string + if found && len(protocolsString) > 0 { + portProtocolMap, err = ParseProtocols(protocolsString) + if err != nil { + return nil, err + } + } // ports takes precedence over port since it will be // specified only when the user hasn't specified a port // via --port and the exposed object has multiple ports. @@ -122,6 +134,7 @@ func generate(genericParams map[string]interface{}) (runtime.Object, error) { return nil, fmt.Errorf("'port' is a required parameter.") } } + portStringSlice := strings.Split(portString, ",") for i, stillPortString := range portStringSlice { port, err := strconv.Atoi(stillPortString) @@ -134,10 +147,26 @@ func generate(genericParams map[string]interface{}) (runtime.Object, error) { if len(portStringSlice) > 1 { name = fmt.Sprintf("port-%d", i+1) } + protocol := params["protocol"] + + switch { + case len(protocol) == 0 && len(portProtocolMap) == 0: + // Default to TCP, what the flag was doing previously. + protocol = "TCP" + case len(protocol) > 0 && len(portProtocolMap) > 0: + // User has specified the --protocol while exposing a multiprotocol resource + // We should stomp multiple protocols with the one specified ie. do nothing + 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 + } + } ports = append(ports, api.ServicePort{ Name: name, Port: int32(port), - Protocol: api.Protocol(params["protocol"]), + Protocol: api.Protocol(protocol), }) } diff --git a/pkg/kubectl/service_test.go b/pkg/kubectl/service_test.go index 80144239e3d..e65df05b5b3 100644 --- a/pkg/kubectl/service_test.go +++ b/pkg/kubectl/service_test.go @@ -404,6 +404,78 @@ func TestGenerateService(t *testing.T) { }, }, }, + { + generator: ServiceGeneratorV2{}, + params: map[string]interface{}{ + "selector": "foo=bar", + "name": "test", + "ports": "80,8080", + "protocols": "8080/UDP", + }, + expected: api.Service{ + ObjectMeta: api.ObjectMeta{ + Name: "test", + }, + Spec: api.ServiceSpec{ + Selector: map[string]string{ + "foo": "bar", + }, + Ports: []api.ServicePort{ + { + Name: "port-1", + Port: 80, + Protocol: api.ProtocolTCP, + TargetPort: intstr.FromInt(80), + }, + { + Name: "port-2", + Port: 8080, + Protocol: api.ProtocolUDP, + TargetPort: intstr.FromInt(8080), + }, + }, + }, + }, + }, + { + generator: ServiceGeneratorV2{}, + params: map[string]interface{}{ + "selector": "foo=bar", + "name": "test", + "ports": "80,8080,8081", + "protocols": "8080/UDP,8081/TCP", + }, + expected: api.Service{ + ObjectMeta: api.ObjectMeta{ + Name: "test", + }, + Spec: api.ServiceSpec{ + Selector: map[string]string{ + "foo": "bar", + }, + Ports: []api.ServicePort{ + { + Name: "port-1", + Port: 80, + Protocol: api.ProtocolTCP, + TargetPort: intstr.FromInt(80), + }, + { + Name: "port-2", + Port: 8080, + Protocol: api.ProtocolUDP, + TargetPort: intstr.FromInt(8080), + }, + { + Name: "port-3", + Port: 8081, + Protocol: api.ProtocolTCP, + TargetPort: intstr.FromInt(8081), + }, + }, + }, + }, + }, } for _, test := range tests { obj, err := test.generator.Generate(test.params)