Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Services with same port, different protocol display wrongly in kubectl and have wrong merge key #39188

Open
thockin opened this issue Dec 23, 2016 · 60 comments
Labels
area/api kind/bug lifecycle/frozen priority/important-soon sig/api-machinery

Comments

@thockin
Copy link
Member

@thockin thockin commented Dec 23, 2016

User reported:

I am running a service with both TCP and UDP:

spec:
  type: NodePort
  ports:
    - protocol: UDP
      port: 30420
      nodePort: 30420
    - protocol: TCP
      port: 30420
      nodePort: 30420

but kubectl describe service shows only UDP.

Type:			NodePort
IP:			10.0.13.152
Port:			<unset>	30420/UDP
NodePort:		<unset>	30420/UDP
Endpoints:		10.244.4.49:30420

When I change the order then it shows only TCP.

This looks like using the wrong mergeKey, and indeed the code backs that up:

2508 type ServiceSpec struct {
2509 // The list of ports that are exposed by this service.
2510 // More info: http://kubernetes.io/docs/user-guide/services#virtual-ips-and-service-proxies
2511 Ports []ServicePort json:"ports,omitempty" patchStrategy:"merge" patchMergeKey:"port" protobuf:"bytes,1,rep,name=ports"

The key should probably be "name", though that can be empty if there is a single port only - is that a problem? @ymqytw ?

@mengqiy
Copy link
Contributor

@mengqiy mengqiy commented Dec 28, 2016

@thockin The validation code doesn't allow me to create two ports with names unspecified.

The Service "my-service" is invalid: 
* spec.ports[0].name: Required value
* spec.ports[1].name: Required value

What version did the user use?

@thockin
Copy link
Member Author

@thockin thockin commented Dec 28, 2016

@mengqiy
Copy link
Contributor

@mengqiy mengqiy commented Dec 28, 2016

Still, the port name seems like the more correct merge key

Agree.
If we want to use name as mergeKey, we should add some validation code to make sure the names are unique. And we need to take care of an empty name if there is a single port only.

@thockin This is a breaking change. So we should make this change in 1.6, right?

cc: @pwittrock

@thockin
Copy link
Member Author

@thockin thockin commented Dec 28, 2016

@mengqiy
Copy link
Contributor

@mengqiy mengqiy commented Dec 28, 2016

Can "" be a valid value for the case of single port?

Don't know yet. I need to look into it.

I guess changing merge key is technically a breaking change, but is there
any real impact?

Yes, like #36024(kubectl is broken, even with minor version skew)

@thockin
Copy link
Member Author

@thockin thockin commented Dec 28, 2016

@mengqiy
Copy link
Contributor

@mengqiy mengqiy commented Dec 28, 2016

Can "" be a valid value for the case of single port?

The mergeKey must present in the go struct to calculate patch. But object round tripping will drop empty field, which is name in this case. So simply change the mergeKeywill break the no-name single-port case.

for: "../svc.yaml": map: map[protocol:UDP port:30420 targetPort:0 nodePort:30432] does not contain declared merge key: name

We should to change the mergeKey, otherwise kubectl apply will do something wrong.
e.g. Creating a service using kubectl apply

apiVersion: v1
kind: Service
metadata:
  name: my-service
spec:
  type: NodePort
  ports:
    - protocol: UDP
      port: 30420
      name: updport
      nodePort: 30420
    - protocol: TCP
      port: 30420
      name: tcpport
      nodePort: 30420
  selector:
    app: MyApp

Make change to tcpport and apply the change

    - protocol: TCP
      port: 30420
      name: tcpport
      nodePort: 30000 # Change this nodePort

kubectl get the service back

apiVersion: v1
kind: Service
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"kind":"Service","apiVersion":"v1","metadata":{"name":"my-service","creationTimestamp":null},"spec":{"ports":[{"name":"updport","protocol":"UDP","port":30420,"targetPort":0,"nodePort":30420},{"name":"tcpport","protocol":"TCP","port":30420,"targetPort":0,"nodePort":30000}],"selector":{"app":"MyApp"},"type":"NodePort"},"status":{"loadBalancer":{}}}
  name: my-service
...
spec:
  clusterIP: 10.0.0.249
  ports:
  - name: updport
    nodePort: 30000 # Wrong change
    port: 30420
    protocol: UDP
    targetPort: 30420
  - name: tcpport
    nodePort: 30420
    port: 30420
    protocol: TCP
    targetPort: 30420
  selector:
    app: MyApp
  sessionAffinity: None
  type: NodePort
status:
  loadBalancer: {}

The change goes to the wrong place.

@thockin
Copy link
Member Author

@thockin thockin commented Dec 28, 2016

@mengqiy
Copy link
Contributor

@mengqiy mengqiy commented Dec 29, 2016

Can we have an empty mergekey automatically become a random string?

Yes, it's possible but it will make the logic complicated. Because apply need original config(in annotation), current config(on API server) and modified config(in user's file) to do 3-way diff.

I'm not sure if @pwittrock @bgrant0607 will like this. Need their comments before going further.

@bgrant0607
Copy link
Member

@bgrant0607 bgrant0607 commented Jan 10, 2017

"Random" string -- no.

We have other cases where a single field isn't sufficient as a merge key, such as lists of object references. I think treating all fields specified by the user's config as the key works in all cases I've thought of. I'd probably make that a new patch strategy. It would probably even be backward compatible.

@pwittrock
Copy link
Member

@pwittrock pwittrock commented Jan 12, 2017

@bgrant0607 Are you thinking this would support only the primitive fields as part of the unique/merge key? Or would this strategy only be valid for types that contain only primitive fields?

Based on your description, the merge key for this strategy would be dynamic and defined by the client.

Have we considered supporting merge keys that are multiple values, but otherwise treated the same as single-value merge keys (in this case it could be <port,protocol> or <port,destinationport,protocol>)?

@bgrant0607
Copy link
Member

@bgrant0607 bgrant0607 commented Jan 12, 2017

@pwittrock No, I was thinking we'd use all fields specified by the user as the key, including nested maps. Multiple keys would also work and may be less error-prone.

@thockin
Copy link
Member Author

@thockin thockin commented Jan 12, 2017

@pwittrock
Copy link
Member

@pwittrock pwittrock commented Jan 12, 2017

@thockin I was thinking something similar yesterday. Something like:

If len(patch list) == 1 && merge key is missing from patch list entry && len(dest list) == 1 && merge key is missing from dest list entry
-> Then merge into only entry in destination list

My biggest hesitation here was that it adds some complexity to the already poorly understood merge semantics. There would probably be some weird edge cases like what happens when we add the merge key to the existing entry. e.g.

If len(patch list) == 1 && merge key exists in the list entry && len(dest list) == 1 && merge key is missing from dest list entry
-> Then merge into only entry in destination list, setting the merge key

@pwittrock
Copy link
Member

@pwittrock pwittrock commented Mar 7, 2017

@ymqytw Can you make sure this gets resolved in 1.7?

@pwittrock pwittrock added this to the v1.7 milestone Mar 7, 2017
@mengqiy
Copy link
Contributor

@mengqiy mengqiy commented Mar 7, 2017

@pwittrock If using the approach in #39188 (comment), then yes.
But if we want support combined merge key, which contains multiple keys, then I think we need a proposal first.

@pwittrock
Copy link
Member

@pwittrock pwittrock commented Mar 7, 2017

Lets sync up with @thockin to determine the minimal requirements for correctness. I don't want to introduce a partial solution since it will create a maintenance burden. Once the merge-key audit is complete, lets come up with a proposal.

I think it is ok if we write a design proposal. I will make sure it gets appropriately reviewed within ~weeks.

@bgrant0607 bgrant0607 added the sig/cli label Mar 9, 2017
@thockin
Copy link
Member Author

@thockin thockin commented Mar 17, 2017

I've run into a lot of issues with ports being PATCHed wrongly through 1.6. Do we believe these issues are fixed now? I can't seem to repro them, but I wasn't dilligent in cataloging them all...

@mengqiy
Copy link
Contributor

@mengqiy mengqiy commented Mar 17, 2017

I guess the pain you have when PATCHing the ports is caused by the bug of using the wrong json pkg in the api server (#42488). It has been fixed by #42489.

issues with ports:

  • #42488 number conversion issue caused by using the wrong json pkg.
  • (this issue) mergeKey for ports (port #) is not unique. The ports will still be PATCHed wrongly if the user does something like #39188 (comment)

@jamiehannaford
Copy link
Contributor

@jamiehannaford jamiehannaford commented Apr 20, 2017

Has this been fixed? I'm seeing both ports on describe:

› kubectl describe svc nginx
Name:			nginx
Namespace:		default
Labels:			<none>
Annotations:		<none>
Selector:		app=nginx
Type:			NodePort
IP:			10.0.0.133
Port:			udp	30420/UDP
NodePort:		udp	30420/UDP
Endpoints:		172.17.0.4:30420
Port:			tcp	30420/TCP
NodePort:		tcp	30420/TCP
Endpoints:		172.17.0.4:30420
Session Affinity:	None
Events:			<none>

@mengqiy
Copy link
Contributor

@mengqiy mengqiy commented Apr 20, 2017

@jamiehannaford Not yet. describe is not affected by this issue.
Proposal in kubernetes/community#476. Review welcome.

@pwittrock pwittrock added this to the v1.8 milestone Jun 2, 2017
@pwittrock pwittrock removed this from the v1.7 milestone Jun 2, 2017
@brianpursley
Copy link
Member

@brianpursley brianpursley commented Jun 23, 2020

Yes, I think that would be part of option 1.

You still have existing services out there with a single port without a name though. I'm not sure how you can still allow those to be updated.

@alankwon
Copy link

@alankwon alankwon commented Jul 17, 2020

I am having same problem creating a ClusterIP service with same port for TCP and UDP... and reading through this thread, it sounds like it's not fixed after all this time. But, I'm puzzled by kube-dns under kube-system namespace. It's also a ClusterIP, but it seems to have same port for TCP and UDP:

NAME       TYPE        CLUSTER-IP     EXTERNAL-IP   PORT(S)         AGE
kube-dns   ClusterIP   10.100.200.2   <none>        53/UDP,53/TCP   60d

I looked at the service definition yaml (https://github.com/kelseyhightower/kubernetes-the-hard-way/blob/master/deployments/kube-dns.yaml#L15) and it's very simple. I have the same syntax for my service.yaml, but it only shows TCP... well, whichever is on top. Anybody know what's different about kube-dns that makes this work?

@apelisse
Copy link
Member

@apelisse apelisse commented Oct 23, 2020

@brianpursley You forgot 4. Use server-side apply and the new listMapKey that allow for multiple merge keys.

@ephesused
Copy link

@ephesused ephesused commented Oct 26, 2020

There's an aspect here that adds some complication. The discussion has been centered around definitions that are the same port but vary by protocol. However, the current behavior accepts yaml that has definitions that vary only by name. It won't function as one might expect, but the yaml is accepted.

Conceptually, there's an argument in support of multiple names for a single port: Ports have port aliases - I can refer to port 25 as either smtp or as mail.

@thockin
Copy link
Member Author

@thockin thockin commented Dec 9, 2020

This seems fixed in 1.20 (maybe before but I have 20 at hand :):

$ k describe svc hostnames
...
Type:                     NodePort
IP:                       10.0.0.201
Port:                     tcp  80/TCP
TargetPort:               9376/TCP
NodePort:                 tcp  30013/TCP
Endpoints:                10.64.0.83:9376
Port:                     udp  80/UDP
TargetPort:               9376/UDP
NodePort:                 udp  30013/UDP
Endpoints:                10.64.0.83:9376
...

Are there other aspects of this that are still pending?

@jaygorrell
Copy link

@jaygorrell jaygorrell commented Mar 17, 2021

@thockin You're able to patch both there and get the expected results?

Here is 1.20. This is where you'd expect a change, right?
https://github.com/kubernetes/kubernetes/blob/release-1.20/staging/src/k8s.io/api/core/v1/types.go#L4041

@apelisse
Copy link
Member

@apelisse apelisse commented Mar 17, 2021

With regards to port name aliasing (which I think is related to the second part of that issue's name, "wrong merge key"), the semantics going forward for this list is that the associative key is [port, protocol], which will make it impossible for people to use the"name aliasing", i.e. two identical ports (same port/protocol pair) with different names.

SSA will not work with this, and field management will fail on objects that don't follow the rule of unique associative keys. It's currently accepted by validation, but ssa code can't accept it. We should probably deprecate lists with duplicate port/protocol pairs in the future and eventually forbid them through validation (using some ratcheting of some sort).

@johnbwork
Copy link

@johnbwork johnbwork commented Apr 30, 2021

Kubernetes Version: v1.20.0+bafe72f

coredns.yaml

apiVersion: v1
kind: Service
metadata:
  name: coredns-svc
  namespace: coredns
spec:
  clusterIP: 10.10.10.10
  clusterIPs:
  - 10.10.10.10
  ports:
  - name: dns
    port: 53
    protocol: UDP
    targetPort: dns
  - name: metrics
    port: 9154
    protocol: TCP
    targetPort: metrics
  selector:
    app: coredns
  sessionAffinity: None
  type: ClusterIP

kubectl apply -f coredns.yaml

We now have a service that is working for UDP! We realize that we forgot a critical item for DNS, TCP! -- DOH!

kubectl edit coredns-svc

Add to ports section:

  - name: dns-tcp
    port: 53
    protocol: TCP
    targetPort: dns-tcp

Only one of the port 53 services will be realized on the service.

kubectl delete svc coredns-svc

newcoredns.yaml

apiVersion: v1
kind: Service
metadata:
  name: coredns-svc
  namespace: coredns
spec:
  clusterIP: 10.10.10.10
  clusterIPs:
  - 10.10.10.10
  ports:
  - name: dns
    port: 53
    protocol: UDP
    targetPort: dns
  - name: dns-tcp
    port: 53
    protocol: TCP
    targetPort: dns-tcp
  - name: metrics
    port: 9154
    protocol: TCP
    targetPort: metrics
  selector:
    app: coredns
  sessionAffinity: None
  type: ClusterIP

kubectl create -f newcoredns.yaml <---- WORKS - Get a service with TCP and UDP 53.

This issue only occurs when trying to patch a previously configured service.

Workaround - delete and re-create service <-- Not optimal, particularly if you are doing something that IPs matter - cluster admin at least can specify IP for a service as long as it hasn't been stomped -- (IPs sometimes matter for things like dns inside a cluster, which we are doing for reasons that are fully valid OK :-P )

@su225
Copy link
Contributor

@su225 su225 commented Jul 8, 2021

  1. Don't fix it. Fail service creation if you try to use the same port number with two protocols.

No. Not option (3) please. There are use cases for this - like DNS and supporting both H2-over-TLS and H3-over-QUIC and so on.

@karlkfi
Copy link
Contributor

@karlkfi karlkfi commented Oct 8, 2021

Was this actually fixed with SSA? How?

The API and docs still say the patchMergeKey is port: https://github.com/kubernetes/kubernetes/blob/release-1.20/staging/src/k8s.io/api/core/v1/types.go#L4041

I think the best way to fix this issue is to add compound key support:

  1. Add a new optional schema field for specifying a compound key (ex: patchMergeKeySet and x-kubernetes-patch-merge-key-set)
  2. Continue also setting the singleton key field (patchMergeKey & x-kubernetes-patch-merge-key)
  3. Update the strategic merge library (both client and server) to use the compound key if specified and fall back to the singleton key (like kustomize did: kubernetes-sigs/kustomize#3159).
  4. Add the new compound key to ServiceSpec.Ports, leaving the existing singleton key as-is.
  5. Deprecate the singleton key on ServiceSpec.Ports and remove it whenever the next schema version (core/v2) is released or when the deprecation policy allows it, whichever comes second.

This way legacy clients still function in the current (broken) way. And new clients can take advantage of the compound key.

@apelisse
Copy link
Member

@apelisse apelisse commented Oct 8, 2021

Was this actually fixed with SSA? How?

We fixed it more or less how you describe it, but only in SSA. The compound key is set through the x-kubernetes-list-map-keys field list.

@david-yu
Copy link

@david-yu david-yu commented Oct 26, 2021

Hi @apelisse I'm curious to know if the change you mentioned above to leverage that compound key needs to be pulled into the Helm CLI as well so folks utilizing Helm for deployment won't run into this issue. I was following along since one of our customers hit this when running helm upgrade on their Consul on Kubernetes deployment, and we noticed on the stateful set the following error. Our Consul DNS port is exposed over both UDP and TCP for the same port number on that statefulset. I can open an issue on the Helm CLI repo, but was curious if there was follow up need on each client to accommodate the new compound key x-kubernetes-list-map-keys. I have not been able to find any reference to that compound key in the code FWIW.

Warning  FailedCreate      2s (x14 over 44s)  statefulset-controller  create Pod server-server-2 in StatefulSet server-server 
failed error: Pod "server-server-2" is invalid: spec.containers[0].ports[6].name: Duplicate value: "dns-udp"

@apelisse
Copy link
Member

@apelisse apelisse commented Oct 27, 2021

I'm not familiar enough with helm to know how deployments are applied, but we might be able to leverage the compound key if they use server-side apply. I'll inquire about that.

@apelisse
Copy link
Member

@apelisse apelisse commented Oct 27, 2021

cc @seans3, might be related to our discussion today :-)

@juur
Copy link

@juur juur commented Dec 30, 2021

This bug still exists and made it impossible for me to use Consul's agents - ports are exposed (with different name) on both TCP and UDP only one was working. Again, applying a config or a applying a patch didn't fix it - I had to destroy the daemonset and recreate it, only then was the hostPort for both UDP and TCP preserved (at least in 1.23.1)

@yeya24
Copy link

@yeya24 yeya24 commented Apr 13, 2022

Hello @apelisse, can you please describe more how to specify x-kubernetes-list-map-keys when using client-go or strategic patch library?
We are hitting this issue when using ArgoCD and it uses the library above. We are seeking for a way to get it working with SSA. Thanks

@apelisse
Copy link
Member

@apelisse apelisse commented Apr 13, 2022

This is a property of the type rather than a property that you would set when you apply, so the author has to consider that when they create their CRD.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api kind/bug lifecycle/frozen priority/important-soon sig/api-machinery
Projects
None yet
Development

No branches or pull requests