hetznercloud / hcloud-cloud-controller-manager

Kubernetes cloud-controller-manager for Hetzner Cloud
Apache License 2.0
740 stars 118 forks source link

Inconsistency in the naming for load balancer annotation. #656

Open DeprecatedLuke opened 5 months ago

DeprecatedLuke commented 5 months ago

https://github.com/hetznercloud/hcloud-cloud-controller-manager/blob/e2b0ed63f6cd5a7499d356bacd784ded41df5f2a/internal/annotation/load_balancer.go#L161

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 commented 5 months ago

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 commented 5 months ago

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 commented 2 months ago

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: