feat: support shared load balancer health probe mode. By setting `clu…

[nilo19]

…sterServiceLoadBalancerHealthProbeMode to shared, all cluster services will share one health probe targeting the kube-proxy port 10256 and /healthz by default. The health check port and path can be configured by clusterServiceSharedLoadBalancerHealthProbePort and clusterServiceSharedLoadBalancerHealthProbePort.

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Support shared load balancer health probe mode. By setting clusterServiceLoadBalancerHealthProbeMode to shared, all cluster services will share one health probe targeting the kube-proxy port 10256 and /healthz by default. The health check port and path can be configured by clusterServiceSharedLoadBalancerHealthProbePort and clusterServiceSharedLoadBalancerHealthProbePort.

Does this PR introduce a user-facing change?

feat: support shared load balancer health probe mode. By setting `clusterServiceLoadBalancerHealthProbeMode` to `shared`, all cluster services will share one health probe targeting the kube-proxy port 10256 and /healthz by default. The health check port and path can be configured by `clusterServiceSharedLoadBalancerHealthProbePort` and `clusterServiceSharedLoadBalancerHealthProbePort`.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[JoelSpeed]

@nilo19 Do you think at some point it would be possible/helpful to switch the default from default to shared? We know that for ETP cluster services that the current default is ineffective, so I would suggest that helping users by making a switch to a better default would be advantageous?

I think given the way this PR is implemented, we could merge it today, create a warning or some release note to tell users that we intend to change the default and then after an appropriate period, switch the default over, WDYT?

If we have an appetite for doing so, we should probably not use default as the name for the current LB health checking, but rather something more descriptive, eg, servicenodeport?

[damdo]

Author of the original PR (#3887) that his one is superseding here.
Thanks for putting up the PR @nilo19

I agree with @JoelSpeed on here, I think given the envisioned KEP-3836: Improve Kube-proxy ingress connectivity reliability, we should try and move away from the "default" behaviour with servicenodeport and move towards the shared port as the standard.

[nilo19]

/retest

[nilo19]

/retest

[nilo19]

@feiskyer what do you think of the suggestion?

[nilo19]

@dharmab @JoelSpeed I think it would be safer to use default as the default config and we can change to shared after the feature is stable.

[JoelSpeed]

@nilo19 yes that was also my suggestion, but I think changing the name away from default would make sense as otherwise in the future you'd have a non-default value, called default

[feiskyer]

change to "unshared" for clear?

[nilo19]

done

[feiskyer]

@nilo19 yes that was also my suggestion, but I think changing the name away from default would make sense as otherwise in the future you'd have a non-default value, called default

That's a good point. Let's change the "default" value to "unshared" here.

[JoelSpeed]

Can I suggest ServiceNodePort as a value, so you would have Shared and ServiceNodePort as the two values. Unshared doesn't really describe the behaviour to me, I think ServiceNodePort would be a better description right? Since in the current case, we always set the health checks based on the service's node port?

[feiskyer]

Can I suggest ServiceNodePort as a value, so you would have Shared and ServiceNodePort as the two values.

That's a good suggestion, thanks for actively reviewing.

[nilo19]

/retest

[feiskyer]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, nilo19

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [feiskyer,nilo19]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[keithmattix]

@feiskyer @nilo19 any chance we could backport this? If I understand correctly, users won't be able to use this functionality until they upgrade to k8s 1.29

[nilo19]

@keithmattix this feature will be ready in >=v1.28.5.

[keithmattix]

Ah great. Thanks!