search

bitnami / charts

Bitnami Helm Charts
https://bitnami.com
Other
9.01k stars 9.22k forks source link

[bitnami/kubernetes-event-exporter] Avoid using .Capabilities.APIVersions.Has, especially when a feature is explicitly requested #30101

Open akloss-cibo opened 1 week ago

akloss-cibo commented 1 week ago

Name and Version

bitnami/kubernetes-event-exporter 3.2.14 (and older)

What architecture are you using?

None

What steps will reproduce the bug?

cd bitnami-charts/bitnami-kuberentes-event-exporter && helm dep up && helm template . --set autoscaling.vpa.enabled=true

Are you using any custom parameters or values?

autoscaling.vpa.enabled=true

What is the expected behavior?

helm template . --set-autoscaling.vpa.enabled=true | grep VerticalPodAutoscaler` should include the VPA.

What do you see instead?

helm template . --set autoscaling.vpa.enabled=true | grep VerticalPodAutoscaler does not include the VPA.

Additional information

This (and many other bitnami charts) use this conditional around the VerticalPodAutoscaler resource:

{{- if and (.Capabilities.APIVersions.Has "autoscaling.k8s.io/v1/VerticalPodAutoscaler") .Values.autoscaling.vpa.enabled }}

However, when using common helm capabilities, like helm template (as used by ArgoCD) this capability is always false, meaning the user's intention of installing the VerticalPodAutoscaler as indicated by .Values.autoscaling.vpa.enabled is ignored. I believe ignoring the users intention here violates POLA and is not failing fast, in that the VPA will be silently dropped from the output resources instead of either being created as the user clearly intends, or fail to create because the VPA isn't installed in the target cluster. Both of these outcomes are better than the silent failure to create the VPA that occurs with the current solution.

javsalgar commented 1 week ago

Hi!

Thank you so much for reporting! Would you like to submit a PR changing that if statement?