digitalocean / digitalocean-cloud-controller-manager

Kubernetes cloud-controller-manager for DigitalOcean (beta)
Apache License 2.0
529 stars 150 forks source link

tls-ports annotation is ignored when using http2 #254

Closed andykent closed 5 years ago

andykent commented 5 years ago

Just updated my cluster hopping to switch to http2 but when changing from http to http2 it also switched port 80 to http2 which I don't think is the expected behaviour.

The docs state...

service.beta.kubernetes.io/do-loadbalancer-tls-ports Specify which ports of the loadbalancer should use the https or http2 protocol.

But the following config produces a LB with http2 as the protocol for both ports 80 and 443...

apiVersion: v1
kind: Service
metadata:
  annotations:
    service.beta.kubernetes.io/do-loadbalancer-algorithm: least_connections
    service.beta.kubernetes.io/do-loadbalancer-certificate-id: xxx
    service.beta.kubernetes.io/do-loadbalancer-healthcheck-path: /_health
    service.beta.kubernetes.io/do-loadbalancer-healthcheck-protocol: http
    service.beta.kubernetes.io/do-loadbalancer-protocol: http2
    service.beta.kubernetes.io/do-loadbalancer-redirect-http-to-https: "true"
    service.beta.kubernetes.io/do-loadbalancer-tls-ports: "443"
  labels:
    app: days
spec:
  ports:
  - name: http
    port: 80
    targetPort: 4000
  - name: https
    port: 443
    targetPort: 4000

Switching the protocol to http has the expected behaviour of port 80 - http, port 443 - https.

timoreimann commented 5 years ago

@andykent thanks for bringing this to our attention.

I'm thinking that we can solve this by supporting another annotation to specify the HTTP/2 ports, similar to what we have today for HTTPS. That would allow everyone to define just the ports they want for each protocol.

The implementation should be pretty straight forward. Let me know if you have any thoughts on that.

timoreimann commented 5 years ago

The above mentioned PR will be shipped on DOKS with the next round of image releases.

andykent commented 5 years ago

Thanks for the quick fix.

As a user though I do think these annotations are getting more and more confusing and edge case-y. Not sure about backwards compatibility but is it worth exploring support for a more unified annotation to specify all port/protocols in one place. Maybe something like this...

service.beta.kubernetes.io/do-loadbalancer-protocols: http:80, https:443, http2:8080

Just a thought.

timoreimann commented 5 years ago

@andykent I very much agree with you on annotations becoming unwieldy -- it's something we are also feeling and want to improve on.

Coming up with a more consistent annotation syntax would be one way. Another one that I think would be worthwhile exploring is to introduce a CRD. That would allow for better opportunities to validate input and support programmatic access.

I just created #256 to track this effort and will copy your suggestion as well as the CRD idea over there. Feel free to continue discussing on that issue.

Thanks!

andykent commented 5 years ago

Great, thanks! I will keep an eager eye on #256 then an add any further ideas that come up over there.

Thanks again.