upgrading to 1.8 broke Service backwards compatibility by not allowing the same target port multiple times

[jhorwit2]

Is this a BUG REPORT or FEATURE REQUEST?:

/kind bug
/sig api
/sig network

What happened:

Prior to 1.8 it was possible to specify the same targetPort in the service definition multiple times. In 1.8, validation was added that prohibited this.

What you expected to happen:
I expect backwards compatibility for GA resources, so having the same targetPort multiple times should be allowed. This is a valid use case IMO for target ports. If you take the example below, the load balancer terminates SSL, so that manifest allows the nginx pods to receive http on port 80 while the load balancer serves traffic on 80 & 443.

How to reproduce it (as minimally and precisely as possible):

kubectl apply -f - <<EOF 
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
EOF

Anything else we need to know?:

Original ticket for validation: #47222

[jhorwit2]

cc @thockin @xiangpengzhao

[jhorwit2]

/kind bug

/area api

[xiangpengzhao]

Sounds reasonable. @thockin WDYT?

[cmluciano]

@kubernetes/sig-network-api-reviews

[fwynyk]

Hi, same problem here. Trying to create a service with helm, both 80 and 443 ports using same targetPort, and got this error:

Error: UPGRADE FAILED: failed to create resource: Service "#####" is invalid: spec.ports[1].targetPort: Duplicate value: intstr.IntOrString{Type:0, IntVal:8000, StrVal:""}

$ kubectl version
Server Version: version.Info{Major:"1", Minor:"8", GitVersion:"v1.8.2", GitCommit:"bdaeafa71f6c7c04636251031f93464384d54963", GitTreeState:"clean", BuildDate:"2017-10-24T19:38:10Z", GoVersion:"go1.8.3", Compiler:"gc", Platform:"linux/amd64"}

[caseydavenport]

Feels reasonable, and it seems this is impacting enough people to warrant reverting the original change.

@thockin WDYT?

[tamalsaha]

Given Service objects are v1, how a breaking change like this was accepted?

[caseydavenport]

/assign

[etiennetremel]

For the one looking for a temporary way to fix it you have to mix named port with value, see below:

kind: Service
apiVersion: v1
metadata:
  name: nginx
spec:
  selector:
    app: nginx
  type: LoadBalancer
  ports:
  - name: http
    port: 80
    targetPort: http
  - name: https
    port: 443
    targetPort: 80

[cchildress]

This seems to be the PR that shipped this.

Why would we want to validate that there are no duplicate target ports anyway? There should be plenty of IP addresses free inside the cluster IP ranges on most clusters (or you have a different problem on your hands) and there are plenty of high-range ports free for the NodePort type. The NAT path through the node should be safe.

I'm not sure why this validation is needed. It seems perfectly safe and sane to me. Plus this is a pretty severe breaking change for the ingress-nginx sidecar for pods.