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

Inconsistency in the naming for load balancer annotation. #656

Open
DeprecatedLuke opened this issue Jun 1, 2024 · 3 comments · May be fixed by #750
Open

Inconsistency in the naming for load balancer annotation. #656

DeprecatedLuke opened this issue Jun 1, 2024 · 3 comments · May be fixed by #750
Labels
bug Something isn't working

Comments

@DeprecatedLuke
Copy link

LBSvcRedirectHTTP Name = "load-balancer.hetzner.cloud/http-redirect-http"

Isn't this supposed to be http-redirect-https? Most guides also say that it's http-redirect-https, but in the code itself it's http-redirect-http.

@apricote apricote added the bug Something isn't working label Jun 18, 2024
@apricote
Copy link
Member

That should indeed be http-redirect-https. Though changing this now would be a breaking change. Perhaps best to add the correct option besides and use that if its set and fall back to the old annotation otherwise.

@DeprecatedLuke
Copy link
Author

DeprecatedLuke commented Jun 18, 2024

I would be more concerned about people using http-redirect-https (as it is shown in the docs) in a misconfigured way causing it to break after an update. Perhaps just have a new annotation called ssl-redirect (same as ingress-nginx).

@apricote
Copy link
Member

After looking at it again, I understand why http-redirect-http was chosen in the first place. The API schema for this is { "http": {"redirect-http": true }}, so the annotation was chosen to be equivalent.

To help with some confusion, but keep it mostly consistent with the API we would prefer the following solution:

  • Add new annotation "load-balancer.hetzner.cloud/http-redirect-https"
  • If specified use the value from (new) http-redirect-https
  • If the new annotation is not specified, use the value of (old) http-redirect-http

@apricote apricote linked a pull request Sep 17, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants