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

NodePort allocation conflates TCP and UDP #20092

Closed
matthiasr opened this Issue Jan 25, 2016 · 15 comments

Comments

Projects
None yet
6 participants
@matthiasr
Member

matthiasr commented Jan 25, 2016

Consider a service like

apiVersion: v1
kind: Service
metadata:
  name: dns
spec:
  selector:
    
  clusterIP: 
  type: NodePort
  ports:
  - name: dns
    port: 53
    nodePort: 30053
    protocol: UDP
  - name: dns-tcp
    port: 53
    nodePort: 30053
    protocol: TCP

The API refuses to accept this service, stating

Service "dns" is invalid: spec.ports[1].nodePort: invalid value '30053', Details: provided port is already allocated

It works fine without nodePort. It works when using different ports. The iptables rules installed (in --proxy-mode=iptables mode) are TCP/UDP specific.

I'm not sure if this is deliberate but not well documented, or a bug? I don't see a reason not to allow a UDP and TCP node port to have the same number.

@matthiasr

This comment has been minimized.

Show comment
Hide comment
@matthiasr

matthiasr Jan 25, 2016

Member

For context, I am trying to make the cluster DNS available to legacy systems with a bit of forwarding, and DNS really really wants to have UDP and TCP on the same port number.

Member

matthiasr commented Jan 25, 2016

For context, I am trying to make the cluster DNS available to legacy systems with a bit of forwarding, and DNS really really wants to have UDP and TCP on the same port number.

@adohe

This comment has been minimized.

Show comment
Hide comment
@adohe

adohe Jan 25, 2016

Member

@matthiasr what is your kubernetes version?

Member

adohe commented Jan 25, 2016

@matthiasr what is your kubernetes version?

@matthiasr

This comment has been minimized.

Show comment
Hide comment
@matthiasr

matthiasr Jan 25, 2016

Member

This is on 1.1.4. From a quick reading of the code in master this hasn't changed though.

Member

matthiasr commented Jan 25, 2016

This is on 1.1.4. From a quick reading of the code in master this hasn't changed though.

@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin

thockin Jan 25, 2016

Member

This was intentionally omitted due to laziness and complexity. We hoped
nobody would need it, since NodePorts are generally allocated
automatically. It's mostly going to be a matter of adding a switch to look
at 2 different saved allocation maps and figuring out how to transition.

On Mon, Jan 25, 2016 at 9:00 AM, Matthias Rampke notifications@github.com
wrote:

This is on 1.1.4. From a quick reading of the code in master
https://github.com/kubernetes/kubernetes/tree/99f301d0b7e8a4c1cbf5c787d412970acb245695/pkg/registry/service/
this hasn't changed though.


Reply to this email directly or view it on GitHub
#20092 (comment)
.

Member

thockin commented Jan 25, 2016

This was intentionally omitted due to laziness and complexity. We hoped
nobody would need it, since NodePorts are generally allocated
automatically. It's mostly going to be a matter of adding a switch to look
at 2 different saved allocation maps and figuring out how to transition.

On Mon, Jan 25, 2016 at 9:00 AM, Matthias Rampke notifications@github.com
wrote:

This is on 1.1.4. From a quick reading of the code in master
https://github.com/kubernetes/kubernetes/tree/99f301d0b7e8a4c1cbf5c787d412970acb245695/pkg/registry/service/
this hasn't changed though.


Reply to this email directly or view it on GitHub
#20092 (comment)
.

@srkiNZ84

This comment has been minimized.

Show comment
Hide comment
@srkiNZ84

srkiNZ84 May 26, 2016

I've also hit this issue (see: https://stackoverflow.com/questions/37449121/how-to-expose-kube-dns-service-for-queries-outside-cluster) when trying to expose DNS to queries from outside the cluster.

Note that in my setup, the "NodePort" is different for UDP and TCP but only UDP appears to work (it's the first to be defined?).

srkiNZ84 commented May 26, 2016

I've also hit this issue (see: https://stackoverflow.com/questions/37449121/how-to-expose-kube-dns-service-for-queries-outside-cluster) when trying to expose DNS to queries from outside the cluster.

Note that in my setup, the "NodePort" is different for UDP and TCP but only UDP appears to work (it's the first to be defined?).

@stephanheinze

This comment has been minimized.

Show comment
Hide comment
@stephanheinze

stephanheinze Jul 28, 2016

I also run into this issue.
I've got a service that wants to expose TCP and UDP port with the same number.

Any plans to include this issue in any roadmap?

stephanheinze commented Jul 28, 2016

I also run into this issue.
I've got a service that wants to expose TCP and UDP port with the same number.

Any plans to include this issue in any roadmap?

@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin

thockin Jul 28, 2016

Member

Help wanted. We're buried in high-priority bugs and features. I ACK that this behaves imperfectly, but I and my team will not have time to look at it in the immediate future.

Member

thockin commented Jul 28, 2016

Help wanted. We're buried in high-priority bugs and features. I ACK that this behaves imperfectly, but I and my team will not have time to look at it in the immediate future.

@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin
Member

thockin commented Jul 28, 2016

@adohe

This comment has been minimized.

Show comment
Hide comment
@adohe

adohe Jul 29, 2016

Member

@thockin @matthiasr I think the root cause is here, when serviceport allocator attempts to reserve the provided port, it just use the port number not consider the port protocol. This is a bug of service port allocator I believe.

Member

adohe commented Jul 29, 2016

@thockin @matthiasr I think the root cause is here, when serviceport allocator attempts to reserve the provided port, it just use the port number not consider the port protocol. This is a bug of service port allocator I believe.

@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin

thockin Jul 29, 2016

Member

"bug", but yes. We'd need to store parallel bitmaps per protocol. Even
then, it's not obvious when to use the same port number in different
protocols, and whether that is REQUIRED or not. If you have a Service with
port 53 in TCP and UDP, and I can't find any port number that is available
in both protocols, should I:

a) fail and reject the REST operation
b) choose a different port number
c) retry forever and hang the REST operation
d) accept the REST operation but log an event saying I failed to do my job

NodePort was really made for load-balancers, where the port number doesn't
matter. Humans using it is "unexpected".

On Thu, Jul 28, 2016 at 9:37 PM, TonyAdo notifications@github.com wrote:

@thockin https://github.com/thockin @matthiasr
https://github.com/matthiasr I think the root cause is here
https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/service/portallocator/allocator.go#L82,
when serviceport allocator attempts to reserve the provided port, it just
use the port number not consider the port protocol. This is a bug of
service port allocator I believe.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#20092 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVMGhHPhZqgQMtDXuv-zbi7vfEvT6ks5qaYOWgaJpZM4HLvL2
.

Member

thockin commented Jul 29, 2016

"bug", but yes. We'd need to store parallel bitmaps per protocol. Even
then, it's not obvious when to use the same port number in different
protocols, and whether that is REQUIRED or not. If you have a Service with
port 53 in TCP and UDP, and I can't find any port number that is available
in both protocols, should I:

a) fail and reject the REST operation
b) choose a different port number
c) retry forever and hang the REST operation
d) accept the REST operation but log an event saying I failed to do my job

NodePort was really made for load-balancers, where the port number doesn't
matter. Humans using it is "unexpected".

On Thu, Jul 28, 2016 at 9:37 PM, TonyAdo notifications@github.com wrote:

@thockin https://github.com/thockin @matthiasr
https://github.com/matthiasr I think the root cause is here
https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/service/portallocator/allocator.go#L82,
when serviceport allocator attempts to reserve the provided port, it just
use the port number not consider the port protocol. This is a bug of
service port allocator I believe.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#20092 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVMGhHPhZqgQMtDXuv-zbi7vfEvT6ks5qaYOWgaJpZM4HLvL2
.

@matthiasr

This comment has been minimized.

Show comment
Hide comment
@matthiasr

matthiasr Jul 29, 2016

Member

fail and reject the REST operation

this is what I'd expect. Port exhaustion is not really a temporary

condition (won't resolve unless some other service is deleted).

Matthias Rampke
Engineer

SoundCloud Ltd. | Rheinsberger Str. 76/77, 10115 Berlin, Germany | +49 173
6395215

Managing Director: Alexander Ljung | Incorporated in England & Wales with
Company No. 6343600 | Local Branch Office | AG Charlottenburg | HRB 110657B

Member

matthiasr commented Jul 29, 2016

fail and reject the REST operation

this is what I'd expect. Port exhaustion is not really a temporary

condition (won't resolve unless some other service is deleted).

Matthias Rampke
Engineer

SoundCloud Ltd. | Rheinsberger Str. 76/77, 10115 Berlin, Germany | +49 173
6395215

Managing Director: Alexander Ljung | Incorporated in England & Wales with
Company No. 6343600 | Local Branch Office | AG Charlottenburg | HRB 110657B

@adohe

This comment has been minimized.

Show comment
Hide comment
@adohe

adohe Jul 31, 2016

Member

I am afraid PR #29771 will not fix this issue. We have to store parallel bitmaps per protocol. I will open a new PR to fix this issue.

Member

adohe commented Jul 31, 2016

I am afraid PR #29771 will not fix this issue. We have to store parallel bitmaps per protocol. I will open a new PR to fix this issue.

@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin

thockin Aug 1, 2016

Member

This alone is insufficient, but we might get away without storing two maps by simply saying

  • the port map is unified across protocols
  • if a service has 2 ports that use the same port number but different protocols, allocate the first nodePort and use the same value for the 2nd port

This will prevent the case of ports being used in one protocol but not the other and failing to allocate.

Something like:

ServicePortToNodePort := map[int][int]{}
for i, sp := svc.Spec.Ports {
    np := ServicePortToNodePort[sp.Port]
    if np == 0 {
        np = allocateUnusedNodePort()
        ServicePortToNodePort[sp.Port] = np
    }
    sp.NodePort = np
}

I haven;t tried too hard to break it, but something like that MIGHT work.

Member

thockin commented Aug 1, 2016

This alone is insufficient, but we might get away without storing two maps by simply saying

  • the port map is unified across protocols
  • if a service has 2 ports that use the same port number but different protocols, allocate the first nodePort and use the same value for the 2nd port

This will prevent the case of ports being used in one protocol but not the other and failing to allocate.

Something like:

ServicePortToNodePort := map[int][int]{}
for i, sp := svc.Spec.Ports {
    np := ServicePortToNodePort[sp.Port]
    if np == 0 {
        np = allocateUnusedNodePort()
        ServicePortToNodePort[sp.Port] = np
    }
    sp.NodePort = np
}

I haven;t tried too hard to break it, but something like that MIGHT work.

@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin

thockin Aug 1, 2016

Member

Additionally, if you have to store multiple bitmaps, you have a serious hassle on your hands for upgrading cluster in-place. Better to avoid it if you can.

Member

thockin commented Aug 1, 2016

Additionally, if you have to store multiple bitmaps, you have a serious hassle on your hands for upgrading cluster in-place. Better to avoid it if you can.

@adohe

This comment has been minimized.

Show comment
Hide comment
@adohe

adohe Aug 1, 2016

Member

@thockin sure, I will think about this more.

Member

adohe commented Aug 1, 2016

@thockin sure, I will think about this more.

k8s-merge-robot added a commit that referenced this issue Aug 26, 2016

Merge pull request #30253 from AdoHe/fix_nodeport
Automatic merge from submit-queue

Allow services which use same port, different protocol to use the same nodePort for both

fix #20092

@thockin @smarterclayton ptal.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment