Custom probes only work if the original probe is explicitly disabled

[orgads]

Name and Version

all

What steps will reproduce the bug?

• Configure customLivenessProbe, customReadinessProbe or customStartupProbe
• Do not set enabled: false for the matching default probe.

Are you using any custom parameters or values?

No response

What is the expected behavior?

The custom probe should be activated.

What do you see instead?

It is not activated, unless I explicitly set default: false for the default probe.

Additional information

Sample template:

          {{- if .Values.livenessProbe.enabled }}
          livenessProbe: {{- include "common.tplvalues.render" (dict "value" (omit .Values.livenessProbe "enabled") "context" $) | nindent 12 }}
            exec:
              ...
          {{- else if .Values.customLivenessProbe }}
          livenessProbe: {{- include "common.tplvalues.render" (dict "value" .Values.customLivenessProbe "context" $) | nindent 12 }}
          {{- end }}

So if the user configured customLivenessProbe but doesn't livenessProbe.enabled to false, the custom probe is not effective.

This is unintuitive. If I set a custom probe, I expect it to just work, regardless of other options.

I pushed a fix for rabbitmq: https://github.com/bitnami/charts/pull/12303, but I still have some concerns.

1. This pattern is widely used across all the bitnami images. It should either be fixed in all of them, or none.
2. Was it an intentional design choice, or was this case just overlooked?
3. Changing the order is not completely backwards compatible. If a user intended to configure a custom probe, and still not use it, by leaving the default enabled, or explicitly setting enabled: true for the default, this case will break. I can't find a good reason for a user to do that though.

Thanks!

[javsalgar]

Hi,

We added the customLivenessProbe forcing the enabled=false in the other probe as a way to not make a major change in the charts. This was the first design decision and we wanted to gather feedback from the users. If this first choice was unintuitive, we are happy to change it to an easier way to do it.

It's true that this change should be extended to all charts in order to make it consistent, so we should work to add this improvement in all of our charts.

About the case 3 in your concerns, I also don't find a reason to create a custom probe but not enable it.

[orgads]

Ok, proposed PR:

• #12374

[orgads]

The script for reference:

require 'set'

chart_updated = Set.new
Dir.glob('**/templates/**/*').each do |file|
  next unless File.file?(file)
  s = File.read(file)
  if s.gsub!(/\{\{- if\s+(?:and )?(\S*Probe\.enabled\s+\}\}\n[\s\S]+?)\{\{- else if\s+(\S*\.custom\w+Probe\s+\}\}\n[\s\S]+?)(?=\{\{- end \}\})/, '{{- if \2{{- else if \1')
    File.write(file, s)
    dir = file.split('/')[0..1].join('/')
    next if !dir.start_with?('bitnami/') || chart_updated.include?(dir)
    chart = File.read("#{dir}/Chart.yaml")
    chart.sub!(/(?<=^version: ).*/) { |ver| ver.split('.').tap { |v| v[-1] = v[-1].to_i + 1 }.join('.') }
    File.write("#{dir}/Chart.yaml", chart)
    chart_updated.add(dir)
  end
end

[orgads]

Finally!

The script for reference (it is hidden in all the ref comments above):

require 'set'

chart_updated = Set.new
Dir.glob('**/templates/**/*').each do |file|
  next unless File.file?(file)
  s = File.read(file)
  if s.gsub!(/\{\{- if\s+(?:and )?(\S*Probe\.enabled\s+\}\}\n[\s\S]+?)\{\{- else if\s+(\S*\.custom\w+Probe\s+\}\}\n[\s\S]+?)(?=\{\{- end \}\})/, '{{- if \2{{- else if \1')
    File.write(file, s)
    dir = file.split('/')[0..1].join('/')
    next if !dir.start_with?('bitnami/') || chart_updated.include?(dir)
    chart = File.read("#{dir}/Chart.yaml")
    chart.sub!(/(?<=^version: ).*/) { |ver| ver.split('.').tap { |v| v[-1] = v[-1].to_i + 1 }.join('.') }
    File.write("#{dir}/Chart.yaml", chart)
    chart_updated.add(dir)
  end
end