feat(load-balancer): Set IPMode to "Proxy" if load balancer is configured to use proxy protocol (#727)

[simonhammes]

To Do

• Add unit tests for new behavior

Happy to leave this until kubernetes/enhancements#1860 is in GA :)

Fixes #727

---

Now cert-manager can successfully execute the self-check when the ingress controller is configured to use the proxy protocol 🚀

[codecov]

Codecov Report

Attention: Patch coverage is 88.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 71.94%. Comparing base (c99a161) to head (1c3c7b9).

Files with missing lines	Patch %	Lines
hcloud/load_balancers.go	88.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #736      +/-   ##
==========================================
- Coverage   72.00%   71.94%   -0.06%     
==========================================
  Files          31       30       -1     
  Lines        2650     2495     -155     
==========================================
- Hits         1908     1795     -113     
+ Misses        553      526      -27     
+ Partials      189      174      -15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[apricote]

Happy to leave this until kubernetes/enhancements#1860 is in GA :)

As long as the API does not error when the field is present I think we can merge. AFAICT the field was added in 1.29, so we need to wait until HCCM does not support <1.29, or add a feature gate for this.

Our support policy states that we only support maintained versions of Kubernetes, so we can drop 1.27 and 1.28 from our tests and compatibility when 1.28 goes EOL on 2024-10-28.

This is the major downside of HCCM not being released in tandem with Kubernetes.

[apricote]

We could also check the server version and only add the field on 1.29+: https://pkg.go.dev/k8s.io/client-go@v0.31.0/discovery#ServerVersionInterface

[simonhammes]

As long as the API does not error when the field is present I think we can merge. AFAICT the field was added in 1.29, so we need to wait until HCCM does not support <1.29, or add a feature gate for this.

I believe it was indeed added in 1.29, but the feature gate was only enabled by default in 1.30.

I'll test this later against older versions and verify whether this breaks anything. Or did you already check that?

---

We could also check the server version and only add the field on 1.29+: https://pkg.go.dev/k8s.io/client-go@v0.31.0/discovery#ServerVersionInterface

Thanks for the hint!

[apricote]

I did not yet, and unfortunately our GitHub Actions test suite does not work for PRs from forks :(

[simonhammes]

I did not yet, and unfortunately our GitHub Actions test suite does not work for PRs from forks :(

No worries, thankfully https://github.com/hetznercloud/kubernetes-dev-env works very well 😄

---

I did not test 1.27 as it is already EOL.

v1.28.13+k3s1 caused warnings (warnings.go:70] unknown field "status.loadBalancer.ingress[0].ipMode")

v1.29.8+k3s1 simply ignored the field ✔️

I'll try to add a check to only add the field on 1.29+

[simonhammes]

I believe the unit tests currently fail because GetConfig() errors, which causes EnsureLoadBalancer() to error, which in turn causes a nil pointer dereference inside the tests.

Is more mocks the solution, or should I just log any errors and return false from checkIPModeSupport()?

[simonhammes]

Would appreciate some guidance on this :)

[apricote]

I would prefer not to add the sigs.k8s.io/controller-runtime dependency. We primarily use k8s.io/cloud-provider and k8s.io/client-go.

We are already being passed a Client Builder in cloud.Initialize(). We can save that in the cloud struct and use it to create clients where necessary (ref: https://github.com/kubernetes/cloud-provider-aws/blob/d7e05d57709cd46297490b51ce0dd11a54dbea35/pkg/providers/v1/aws.go#L678)

I would also prefer to only check the version once on startup, but that might cause issues when the Kubernetes version is updated without restarting the Pod.

---

Regarding the unit tests, at that point you could mock the client that is being passed to loadBalancers{} in the test.

[simonhammes]

@lukasmetzner Thank you for completing my work in #783 🚀