Register Endpoint ports for ClusterIP services

[lkysow]

Previously, we were registering the Service's port instead of the
underlying endpoint port (as specified by targetPort).
If the two were different, this would result
in traffic being directed to the wrong port.

Also handles edge cases around ports being overridden via annotations.

Fixes #132

Some background:

• ClusterIP services can have a targetPort that specifies which port on the Pod that traffic should be routed to. We weren't taking that into account and always using port.

• We are registering each Endpoint as a service instance. The endpoints are the Pod IPs so obviously we need to use the Pod's port, i.e. targetPort.

• We support overriding which port we use via an annotation (https://www.consul.io/docs/platform/k8s/service-sync.html#service-ports). This can be set to a port name or a specific number. If set to a number, then we just use that number. If set to a name, we'd like to use that port name from the service:

  apiVersion: v1
  kind: Service
  metadata:
    name: nginx
  spec:
    selector:
      app: nginx
    ports:
      - protocol: TCP
        name: myname
        port: 8080
        targetPort: 80 # <<<<<< want to use this

  Unfortunately, targetPort can also be set to the name of the Pod's port:

  apiVersion: v1
  kind: Service
  metadata:
    name: nginx
  spec:
    selector:
      app: nginx
    ports:
      - protocol: TCP
        name: myname
        port: 8080
        targetPort: targetname # <<<<<< ugh

  In this case we need to instead use the named port from the Endpoint which will have an actual number since Kubernetes has resolved targetname for us.

[ishustava]

This looks good to me. I added a few comments/questions.

[ishustava]

This is not the code you changed, but I think it'd be good to change the type of port to uint16 rather than int. It should probably be the case for all port variables.

[lkysow]

This is the datatype on the consul api struct so we can't change it: https://github.com/hashicorp/consul/blob/master/api/agent.go#L71

Sometimes ports in consul get set to -1 to state that they're disable so that's probably why they use int. In the case of a service this wouldn't make any sense though.

[ishustava]

I see, thanks for clarifying.

[ishustava]

Would overridePortName be a better name for this to match overridePortNumber?

[lkysow]

Ahh good catch. At some point while coding this, I was using this variable if Service.targetPort was also a name so I didn't want to use override but I'm not doing this anymore. Will change.

[ishustava]

Is overriding the service port to something that isn't a registered endpoint with k8s a valid behavior?

[lkysow]

I think it's the best thing we can do in this situation. If you're setting the port to a specific number then we should use it, even if it's currently wrong. Maybe their next step is to add the containerPort.

[ishustava]

That makes sense.

[lkysow]

@ishustava I've addressed your comments, please take another look