Do not define loadBalancerIP on service unless set in values

[benyanke]

TLDR: Closed and replaced by #430

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

[provokateurin]

You need to add the Signed-of-by tag to all your commits (git commit -s)

[jessebot]

Adding to Kate's comment, since you have four commits, you can do:

git rebase HEAD~4 --signoff
git push --force

[provokateurin]

Oh I didn't know you can signoff using rebase as well, that's pretty neat :)

[benyanke]

replaced by #430

[benyanke]

@jessebot @provokateurin - had trouble getting signing working properly, simply created a new branch with the changes. Can you review #430?