search

kubernetes / ingress-nginx

Ingress NGINX Controller for Kubernetes
https://kubernetes.github.io/ingress-nginx/
Apache License 2.0
17.5k stars 8.26k forks source link

DigitalOcean Version Doesn't Have Proper Annotations #8965

Closed josiahbryan closed 2 years ago

josiahbryan commented 2 years ago

What happened:

No data shows for HTTP response times on DO LB Graphs

What you expected to happen:

Wrong annotations

NGINX Ingress controller version (exec into the pod and run nginx-ingress-controller --version.):

ingress-nginx_controller-v1.2.1-do.yml

Proper annotations should be included with the YAML. Here's the right annotations to include:

  annotations:
    # Name for DO UI
    service.beta.kubernetes.io/do-loadbalancer-name: "<whatever>"
    # Based on https://github.com/digitalocean/digitalocean-cloud-controller-manager/blob/master/docs/controllers/services/examples/https-with-pass-through-nginx.yml
    service.beta.kubernetes.io/do-loadbalancer-protocol: "http"
    service.beta.kubernetes.io/do-loadbalancer-tls-ports: "443"
    service.beta.kubernetes.io/do-loadbalancer-tls-passthrough: "true"
    service.beta.kubernetes.io/do-loadbalancer-enable-proxy-protocol: "true"
    # Fix some issues for internal references - see https://github.com/digitalocean/digitalocean-cloud-controller-manager/blob/master/docs/controllers/services/annotations.md#servicebetakubernetesiodo-loadbalancer-hostname
    service.beta.kubernetes.io/do-loadbalancer-hostname: "ingress.vayadriving.com"
    # Per https://github.com/digitalocean/digitalocean-cloud-controller-manager/blob/master/docs/controllers/services/annotations.md#servicebetakubernetesiodo-loadbalancer-http-ports
    service.beta.kubernetes.io/do-loadbalancer-http-ports: "80"
    # Default is round_robin - https://github.com/digitalocean/digitalocean-cloud-controller-manager/blob/master/docs/controllers/services/annotations.md#servicebetakubernetesiodo-loadbalancer-algorithm
    service.beta.kubernetes.io/do-loadbalancer-algorithm: "least_connections"
longwuyuan commented 2 years ago

/remove-kind bug /kind feature

@josiahbryan I think this requires editing https://github.com/kubernetes/ingress-nginx/blob/main/hack/manifest-templates/provider/do/values.yaml and adding the new currently non-existing other annotations there. Please free to submit a PR.

But there is no proof like screenshots and configurations-snippets from a cluster that has these annotations and shows the metrics. Are you talking about the graphs of DO service or are you talking about graphs on Grafana/Prometheus etc ?

Also, do all DO users want these annotations ? I am not convinced that all users of DigitalOcean would want these annotations configured out of the box from the project.

josiahbryan commented 2 years ago

Without these annotations, graphs on the digital ocean dashboard for the LB show "No Data" on all the HTTP-related graphs. I would imagine that all users would expect and want data for graphs since this is an HTTP(S|2)-centric project (e.g. nginx) and the DO config is being offered.

Without the annotations specifying the HTTP ports, the graphs look something like the attached screenshot. Note the "No Data".

After going round and round with DO support, they finally figured out it was missing those annotations. Happy to provide the ticket that we interacted on, but they linked me to those docs in the YAML comments anyway, which document this requirement.

It's just super annoying that no mention of this is anywhere in the nginx ingress docs (this project) because out of the box, the DO LB looks "broken" in the graphs it's reporting, even though data flows. With the annotations, data shows in those graphs. It's that simple. I really don't care if you update the docs or not, I know now. I just submit this to help future users. I do believe all users who use DO with this project would DEFINITELY want to see data here. What do you think?

[image: image.png]

On Tue, Aug 23, 2022 at 10:21 PM Long Wu Yuan @.***> wrote:

/remove-kind bug /kind feature

@josiahbryan https://github.com/josiahbryan I think this requires editing https://github.com/kubernetes/ingress-nginx/blob/main/hack/manifest-templates/provider/do/values.yaml and adding the new currently non-existing other annotations there. Please free to submit a PR.

But there is no proof like screenshots and configurations-snippets from a cluster that has these annotations and shows the metrics. Are you talking about the graphs of DO service or are you talking about graphs on Grafana/Prometheus etc ?

Also, do all DO users want these annotations ? I am not convinced that all users of DigitalOcean would want these annotations configured out of the box from the project.

— Reply to this email directly, view it on GitHub https://github.com/kubernetes/ingress-nginx/issues/8965#issuecomment-1225133501, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABEZELBDLMECI5HVJ3Z2C7LV2WILVANCNFSM57NNHIPA . You are receiving this because you were mentioned.Message ID: @.***>

-- Josiah Bryan +1-765-215-0511 (Phone/SMS/WhatsApp) www.josiahbryan.com https://www.josiahbryan.com/?utm_source=sig @.***

longwuyuan commented 2 years ago

Thank you

josiahbryan commented 2 years ago

There is no cost implications. I'll let you submit the PR. I'm reporting this as an issue because it makes the software look broken out of the box. Good luck and have fun!

On Wed, Aug 24, 2022, 12:45 AM Long Wu Yuan @.***> wrote:

  • Are there cost implications to running those metrics ?
  • Multiple DO users did not ask for those annotations to be available out of the box so do some users of DO prefer not having those annotations out of the box because there integrated observability like prometheus+grafana give them the info those annotations provide. This we do not know and maybe have to ask/survey.
  • When the DO provider was added and the DO specific values file created, did the contributor knowingly not add those annotations for above mentioned reasons. We don't know. And its hard to find out.
  • In any case, please feel free to submit a PR to add those annotations to the DO specific values file, as I mentioned in my previous post. If the approvers think, its ok then it will be merged, and next time we generate static manifest for DO, the service object will have those annotations
  • The idea is not to block a change but to populate the issue with enough information to make better decisions. If there is cost implication and users have their own observability, then forcing this on users will not be an improvement, hence my comments

Thank you

— Reply to this email directly, view it on GitHub https://github.com/kubernetes/ingress-nginx/issues/8965#issuecomment-1225224381, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABEZELB6PRVECTIJDMMHKCTV2WZHDANCNFSM57NNHIPA . You are receiving this because you were mentioned.Message ID: @.***>

longwuyuan commented 2 years ago

ok thanks. I will submit the PR

longwuyuan commented 2 years ago

Need to get specific on 

service.beta.kubernetes.io/do-loadbalancer-name: "<whatever>"

Can you point me to a link for that annotation. Can't put "whatever" as value

josiahbryan commented 2 years ago

https://github.com/digitalocean/digitalocean-cloud-controller-manager/blob/master/docs/controllers/services/annotations.md#servicebetakubernetesiodo-loadbalancer-name

On Wed, Aug 24, 2022, 12:54 AM Long Wu Yuan @.***> wrote:

Need to get specific on

service.beta.kubernetes.io/do-loadbalancer-name: ""

Can you point me to a link for that annotation. Can't put "whatever" as value

— Reply to this email directly, view it on GitHub https://github.com/kubernetes/ingress-nginx/issues/8965#issuecomment-1225229536, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABEZELGJVHV2KVU6EXAL4T3V2W2LFANCNFSM57NNHIPA . You are receiving this because you were mentioned.Message ID: @.***>

longwuyuan commented 2 years ago

Also, this one 

service.beta.kubernetes.io/do-loadbalancer-tls-passthrough: "true"

I guess this one is for passthrough on the LB so TLS terminates on controller. Is your opinion that all users would want this

longwuyuan commented 2 years ago

I see that the existing LB will be renamed and also not using this annotation means it will set a name like a-<serviceUID>. I don't know if the out-of-the-box config should decide what name to set or to rename the existing LB.

Now, to me it seems that, an improvement would be to write about the usage of these additional annotations in docs, so that users can edit the service to their taste, after installation, as compared to forcing a name etc

longwuyuan commented 2 years ago

/triage accepted

techsachin1 commented 1 year ago

hi all. i have added service.beta.kubernetes.io/do-loadbalancer-enable-proxy-protocol: "true" in service file & use-proxy-protocol: "true" in config file because i have to whitelist some IP.. but when i apply this changes thers is all my api going to fall down and showing error---- Caused by: org.springframework.web.client.HttpClientErrorException: 500 I/O error on POST request for "https://api-ab.xyz token": Connection reset; nested exception is java.net.SocketException: Connection reset