Do not define loadBalancerIP on service unless set in values

[benyanke]

Pull Request

Description of the change

This PR sets the .Values.service.loadBalancerIP field as optional, and does not define it in the service if it's not set in the values.

This is because some LoadBalancer implementations allow or even require this value to remain unset, and while it may sometimes be fine to leave it set but blank, this could have unexpected impacts depending on your cluster configuration.

In particular, MetalLB will leave the service <pending> if the loadBalancerIP is set to nil, and hardcoding it to a value in the values breaks the ability for MetalLB's IPAM tooling to be used for dynamic address allocation. I suspect other LoadBalancer implementations may have similar behaviors.

Benefits

The primary benefit is more flexible usage with more LoadBalancer implementations.

Possible drawbacks

None I can think of, however I am open to discussion.

Applicable issues

• fixes Do not define loadBalancerIP on service unless set in values #428

Additional information

For easy review on the output, here is the before and after:

After - No LB set

Note how the loadBalancerIP is now gone when it's not defined, for more flexible usage.

$ helm template nextcloud . --set service.type=LoadBalancer | grep Service -A 15
kind: Service
metadata:
  name: nextcloud
  labels:
    app.kubernetes.io/name: nextcloud
    helm.sh/chart: nextcloud-3.6.0
    app.kubernetes.io/instance: nextcloud
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/component: app
spec:
  type: LoadBalancer
  ports:
  - port: 8080
    targetPort: 80
    protocol: TCP
    name: http

After - LB Set

Note how the loadBalancerIP still works as before when it's defined, for full backward compatibility.

$ helm template nextcloud . --set service.type=LoadBalancer --set service.loadBalancerIP=1.2.3.4 | grep Service -A 15
kind: Service
metadata:
  name: nextcloud
  labels:
    app.kubernetes.io/name: nextcloud
    helm.sh/chart: nextcloud-3.6.0
    app.kubernetes.io/instance: nextcloud
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/component: app
spec:
  type: LoadBalancer
  loadBalancerIP: 1.2.3.4
  ports:
  - port: 8080
    targetPort: 80
    protocol: TCP

After - Forcing Previous Behavior

And if you still wish to template out the field with nil in the value as before, that is still supported by explicitly setting to nil.

$ helm template nextcloud . --set service.type=LoadBalancer --set service.loadBalancerIP=nil | grep Service -A 15
kind: Service
metadata:
  name: nextcloud
  labels:
    app.kubernetes.io/name: nextcloud
    helm.sh/chart: nextcloud-3.5.22
    app.kubernetes.io/instance: nextcloud
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/component: app
spec:
  type: LoadBalancer
  loadBalancerIP: nil
  ports:
  - port: 8080
    targetPort: 80
    protocol: TCP

Before

Note the nil in the loadBalancerIP field.

$ helm template nextcloud . --set service.type=LoadBalancer | grep Service -A 15
kind: Service
metadata:
  name: nextcloud
  labels:
    app.kubernetes.io/name: nextcloud
    helm.sh/chart: nextcloud-3.6.0
    app.kubernetes.io/instance: nextcloud
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/component: app
spec:
  type: LoadBalancer
  loadBalancerIP: nil
  ports:
  - port: 8080
    targetPort: 80
    protocol: TCP
    name: http

Checklist

• DCO has been signed off on the commit.
• Chart version bumped in Chart.yaml according to semver.
• (optional) Variables are documented in the README.md

[benyanke]

Thanks for the approval @provokateurin - is there anything else you need from me to get this merged?

[provokateurin]

No, I'm just not experienced with the loadbalancer configuration so I want another review from @jessebot

[jessebot]

takin a peek :D

[jessebot]

so this looks ok to me. I cloned benyanke:issue-428-b and did a helm template as is, and then again with service.type=LoadBalancer in the values.yaml and they both look correct. I've been using metallb and haven't seen issues with this in the past, but the pool I use is so small, and even with metallb, I still use clusterIP for my service type. Still, I think this is fair to merge :)

[jessebot]

Requesting just a bit of an update to use "" instead of omitting the value entirely. The change in templates/service.yaml is still good though :)

[benyanke]

Hi @jessebot - I've made the requested changes above.

[jessebot]

@benyanke awesome, thanks for your hard work and patience on this! :) After the linter runs, I'll get this merged!

[provokateurin]

Can we please squash this mess? 🙈

[jessebot]

Can we please squash this mess? 🙈

Absolutely! :) I always squash and merge so that the history looks a bit cleaner and then users can always inspect the PR for more granular commit data if they choose.

A maintainer can change the repo to only allow squash and merge through the general settings on this repo, but I don't have permissions for that, so right now I manually select it.

[benyanke]

Absolutely agree, squash merging is the way to go.