Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DigitalOcean Version Doesn't Have Proper Annotations #8965

Closed
josiahbryan opened this issue Aug 24, 2022 · 11 comments · Fixed by #8966
Closed

DigitalOcean Version Doesn't Have Proper Annotations #8965

josiahbryan opened this issue Aug 24, 2022 · 11 comments · Fixed by #8966
Labels
kind/feature Categorizes issue or PR as related to a new feature. needs-priority triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@josiahbryan
Copy link

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"
@josiahbryan josiahbryan added the kind/bug Categorizes issue or PR as related to a bug. label Aug 24, 2022
@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels Aug 24, 2022
@longwuyuan
Copy link
Contributor

/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.

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed kind/bug Categorizes issue or PR as related to a bug. labels Aug 24, 2022
@josiahbryan
Copy link
Author

josiahbryan commented Aug 24, 2022 via email

@longwuyuan
Copy link
Contributor

longwuyuan commented Aug 24, 2022

  • 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 their integrated pre-existing observability stack like prometheus+grafana give them the info that 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 was 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

@josiahbryan
Copy link
Author

josiahbryan commented Aug 24, 2022 via email

@longwuyuan
Copy link
Contributor

ok thanks. I will submit the PR

@longwuyuan
Copy link
Contributor

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
Copy link
Author

josiahbryan commented Aug 24, 2022 via email

@longwuyuan
Copy link
Contributor

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
Copy link
Contributor

longwuyuan commented Aug 24, 2022

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
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 24, 2022
@techsachin1
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. needs-priority triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants