[stable/nginx-ingress] If clusterIPs are set, they will automatically be quoted

[naseemkullah]

What this PR does / why we need it:

So that if by chance clusterIP values are actually provided (this PR aside, these are values that should not even exist imho), they are automatically quoted.

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

• fixes #

Special notes for your reviewer:

Along the path to the current fix for the infamous clusterIP conflicts of having them commented out in the default values file, I proposed a fix that included removing the | quote in the templates. This proved to be useless, so I am adding that back for those (if anyone) that set their clusterIPs.

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

• DCO signed
• Chart Version bumped
• Variables are documented in the README.md
• Title of the PR starts with chart name (e.g. [stable/mychartname])

cc @ChiefAlexander @taharah

[ChiefAlexander]

/lgtm
Thanks!

[naseemkullah]

/retest

[ChiefAlexander]

Other changes have been merged in before yours. You will have to fix the conflicts and then bump the version of the chart.

[naseemkullah]

np @ChiefAlexander, done.

[ChiefAlexander]

Sorry about this, but yet another change went in before yours. You will have to update your pr again.

[ChiefAlexander]

/lgtm
Thanks!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ChiefAlexander, naseemkullah

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:

• stable/nginx-ingress/OWNERS [ChiefAlexander]

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

[cgroschupp]

@naseemkullah How is it possible to set ClusterIP to an empty string after this PR?

values.yaml:

defaultBackend:
  service:
    omitClusterIP: false
    clusterIP: '""'

With chart version 0.28.1, i get this:

clusterIP: ""

With chart version 0.28.2, i get this:

clusterIP: "\"\""

[naseemkullah]

@naseemkullah How is it possible to set ClusterIP to an empty string after this PR?

values.yaml:

defaultBackend:
  service:
    omitClusterIP: false
    clusterIP: '""'

With chart version 0.28.1, i get this:

clusterIP: ""

With chart version 0.28.2, i get this:

clusterIP: "\"\""

Hi @cgroschupp try clusterIP: "" or clusterIP: or clusterIP: null one of those should work.
I'm curious as to why though, is it beneficial to have clusterIP: "" in the resulting template?

[cgroschupp]

Hi @naseemkullah None of the variants produces an empty string. older kubernetes version requires an empty string in ClusterIP. Now is it possible to omit the ClusterIP. But you have to destroy the old service. Therefore the parameter omitClusterIP was introduced.

[naseemkullah]

My mistake @cgroschupp and I did not know older k8s versions required an empty string as ClusterIP.

By default the chart now does not template this, even without the omitClusterIP because in the default values file clusterIP: values are commented out altogether. If this does not play well with older versions of k8s we should maybe update the minimum k8s version required to run this chart as is.