Add support for named port, better docs for TLS nginx Ingress

[aledbf]

fixes #743
fixes #781
fixes #858
fixes #871
closes #872
fixes #876

[aledbf]

ping @bprashanth

[aledbf]

In case of services with namedPorts the Ingress controller add an annotation to the service

$ kubectl get ing
NAME        RULE           BACKEND   ADDRESS
demo-port   -                        172.17.4.99
            foo.bar.com
            /              echoheaders-xtp:echo-port

$ kubectl get svc echoheaders-multiple -o yaml
apiVersion: v1
kind: Service
metadata:
  annotations:
    kubernetes.io/ingress-named-ports: '{"echo-port":"8080"}'
  labels:
    app: echoheaders
  name: echoheaders-multiple
  namespace: default
spec:
  clusterIP: 10.3.0.214
  ports:
  - name: demo
    port: 80
    protocol: TCP
    targetPort: echo-port
  - name: demo2
    port: 81
    protocol: TCP
    targetPort: 8080
  selector:
    app: echoheaders

[bprashanth]

What happens if this update fails? (eg: resource version conflict). And if it passes, will we actuallly just come back in this function and relist pods again?

[aledbf]

requeue added in case of error during update

[bprashanth]

I'm ok with the pod list if we can restrict it to just when the user has the named targetport.Other stuff looks fine.

[alanhartless]

It seems something with this has broken generating a correct nginx.conf for me. My nginx.conf ends up with something like:

    upstream default-web- {
        least_conn;
        server 127.0.0.1:8181;

    }

    upstream upstream-default-backend {
        least_conn;
        server 10.0.0.3:8080;

    }

It's like it's not finding the ports for the web service.

apiVersion: v1
kind: Service
metadata:
  name: web
  labels:
    name: web
spec:
  ports:
    - port: 80
      targetPort: 8080
      name: http
  selector:
    name: web

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: foo-bar-net
spec:
  tls:
  - hosts:
    - foo.bar.net
    secretName: foo-bar-tls
  rules:
  - host: foo.bar.net
    http:
      paths:
      -
        path: /
        backend:
          serviceName: web
          servicePort: 80

I'm double checking to make sure my local patched ok and to see if I can figure something out in the mean time but wanted to mention it in case the cause was something obvious. I don't see any errors in the log.

[alanhartless]

Just an update - if I change the servicePort to a named port, then my conf added the name correct:

    upstream default-web-http {
        least_conn;
        server 127.0.0.1:8181;

    }```

Maybe that part is just cosmetic. Still not finding my web services though so continuing to dig into that. 

[nottix]

@bprashanth any plan to integrate this pull request?

[bprashanth]

Yeah I'll make another pass sometime today since it looks like Manual updated the commits after my last comment.

[bprashanth]

can you bail early (before pod list) if none of these ports is a string?

If we can't selectively list only when there's a named port, I'd suggest setting up a pod watch, eg:

    podStore.Store, podController = framework.NewInformer(
        &cache.ListWatch{
            ListFunc: func(options api.ListOptions) (runtime.Object, error) {
                return e.client.Core().Pods(api.NamespaceAll).List(options)
            },
            WatchFunc: func(options api.ListOptions) (watch.Interface, error) {
                return e.client.Core().Pods(api.NamespaceAll).Watch(options)
            },
        },
        &api.Pod{},
        resyncPeriod(),
        framework.ResourceEventHandlerFuncs{
            AddFunc:    addPod,
            UpdateFunc: updatePod,
            DeleteFunc: deletePod,
        },
    )

And then you have a local cache you can list for pods:
https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/replication/replication_controller.go#L542

[bprashanth]

can we ditch the extra indent and continue when ok?

[aledbf]

done

[bprashanth]

Sorry for the delay, my only reservation at this point is that checkSvcForUpdate is icky, I'd vastly prefer if it didn't update services. If you can either do as suggested or explain why we can't just handle this through the normal ingress sync routine, I'll be ok with it.

[aledbf]

@bprashanth testing doing the update in getEndpoints

[aledbf]

ping @bprashanth please review

[bprashanth]

please add a comment about what mappings, since the name is very general and it returns a map. Or rename the method.

[bprashanth]

So much better, thanks!
LGTM but for the one naming nit

[aledbf]

@bprashanth done

[bprashanth]

LGTM

[sekka1]

Just seen all of this new stuff today. Was trying to upgrade our old ingress (container version 0.2). When will a new container be build from this? It seems gcr.io/google_containers/nginx-ingress-controller:0.6 does not have the hsts fix in it. I set the flag in the config map to false and it has no effect.

[bprashanth]

@sekka1 please try with 0.61, it contains #849

[sekka1]

Thanks @bprashanth it is working now.

[bprashanth]

Thank @simonswine and @aledbf, I just pushed the image :)