Services allow duplicate targetPort

[thockin]

It is possible to express a Service where multiple front-side ports target the same back-side port. This is not very useful and may cause trouble for implementations of Services.

apiVersion: v1
kind: Service
metadata:
  name: hostnames-2
spec:
  ports:
  - name: a
    port: 80
    targetPort: 9376
  - name: b
    port: 81
    targetPort: 9376
  selector:
    run: hostnames

[odeke-em]

@thockin just for the purpose of clarity, are you requesting that duplicate targetPorts
should be rejected or the opposite?
PS: I am new to Kubernetes so please pardon the dumb question :)

[thockin]

I think that should probably be rejected at validation unless we have a good reason WHY we need that.

…

On Thu, Jun 8, 2017 at 11:21 PM, Emmanuel T Odeke ***@***.***> wrote: @thockin <https://github.com/thockin> just for the purpose of clarity, are you requesting that duplicate targetPorts should be rejected or the opposite? PS: I am new to Kubernetes so please pardon the dumb question :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#47222 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVH6eTLB4Z_QYaAdxrdqqJy_NgVtNks5sCOR6gaJpZM4N07AA> .

[odeke-em]

Aye aye, thanks for the clarificiation!

[xiangpengzhao]

/assign

[k8s-ci-robot]

@xiangpengzhao: GitHub didn't allow me to assign the following users: xiangpengzhao.

Note that only kubernetes members can be assigned.

In response to this:

/assign

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[jhorwit2]

@thockin what problems did this cause with services? This change broke backwards compatibility for services that depended on this feature.

For example, lets take the load balancer service

kind: Service
apiVersion: v1
metadata:
  name: nginx-service
  annotations:
    service.beta.kubernetes.io/oci-load-balancer-ssl-ports: "443"
    service.beta.kubernetes.io/oci-load-balancer-tls-secret: ssl-certificate-secret
spec:
  selector:
    app: nginx
  type: LoadBalancer
  ports:
  - name: http
    port: 80
    targetPort: 80
  - name: https
    port: 443
    targetPort: 80

The load balancer here terminates SSL so the backend is always port 80; however, you want to make sure you support both protocols so having the same targetPort here is a valid use case.

[thockin]

This is an interesting (ab)use of Service. Not really what I had in mind - the LB in a Service is generally assumed to be L4. I'll try to dredge up the issues that this was causing.

@kubernetes/sig-network-api-reviews

[jhorwit2]

FYI, the AWS in-tree provider does non layer4 things as well.

[thockin]

One issue is that this REQUIRES connection tracking when we otherwise might not need it.

Consider two ServicePorts:

• my-service:80
• mys-service:443

Let's say we map both services to a backend on port 8080, but we pass-through the VIP, so the pod sees packets as dest = my-service:8080.

We MUST conntrack this VIP because we don't know whether to map the src port to 80 or 443. If service backends have unique TargetPorts, then we wouldn't have this issue. Port rewrites are at worst stateless, and at best can engage direct-return mode (no rewrites).

It's worth noting that the PR which fixes this, which is proposed for revert, doesn't quite go far enough.  It prevents a Service from using a given number twice and from using a given name twice, but not from using a number and a name that resolves to that number.  That is a much trickier (impossible to do statically) proposition. :(

Kind of a mess for getting high-perf Service networking.

[jhorwit2]

@thockin was this an issue that came up during performance testing or IRL?

[ese]

Anyone has a workaround to deal with this? I think there is a lot of people using AWS ELBs on this way

[tamalsaha]

@ese, if your use-case is Ingress, then you can set ingress.kubernetes.io/ssl-redirect: false. This should avoid generating the invalid Service spec. Obviously, you will lose the redirect behavior.

[microwaves]

This is not an abuse of Service, but a quite common and wide-adopted pattern, especially in the AWS world.

[k8s-ci-robot]

@xiangpengzhao: you can't re-open an issue/PR unless you authored it or you are assigned to it.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

@xiangpengzhao: you can't re-open an issue/PR unless you authored it or you are assigned to it.

In response to this:

/assign
/reopen
for further discussion

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[xiangpengzhao]

/reopen
for further discussion.

[caseydavenport]

@thockin I suspect we should probably revert the original PR. While I understand it might not have been the intention for Services, if I understand this correctly then it's a breaking change to a v1 API.

Can we at least point to any documentation that indicates this was a code bug rather than a supported behavior?

[caseydavenport]

Sentiment on today's sig call was that we should revert the original commit, since it broke lots of users, and then revisit to see if there's another solution.

[xiangpengzhao]

FYI: the revert PR is #53576.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close