search

kedacore / charts

Helm charts for KEDA
Apache License 2.0
148 stars 205 forks source link

fix: ServiceAccount are correctly annotated #636

Closed JorTurFer closed 1 month ago

JorTurFer commented 2 months ago

NOTE: This other PR is in conflict as both updates the index, we will have to merge one and the rebase the other one

During the RBAC modifications, we introduced a but that suppressed the operator service account annotations. This can be critical for users who don't use podIdentity section but directly annotate the service account with pod identity information (basically, KEDA doesn't use pod identity on their cases)

This PR keeps the serviceaccount segregation, but instead of a fallback condition that includes the old serviceAccount.annotations only if the more new ones aren't set (which was failing because the values.yaml sets them) now we will place both together. When users migrate their yamls, they just need to add the new ones and remove the old ones, but as default, they won't have any breaking change

Checklist

Fixes https://github.com/kedacore/keda/issues/5758

jkremser commented 1 month ago

can you please elaborate on the issue here?

only if the more new ones aren't set (which was failing because the values.yaml sets them)

what do you mean by this exactly? User's values.yaml sets them? Or the default values.yaml that are bundled with the new version of the chart? Because if it was the latter, the issue would make sense to me, but these are empty:

so if not provided, the old format is tried

JorTurFer commented 1 month ago

Default values provided by chart's values.yaml set them to true: https://github.com/kedacore/charts/blob/f9f23e91d5b03059018f7eb8d0cc34112f52760a/keda/values.yaml#L290-L317

JorTurFer commented 1 month ago

This condition is failing because of that:

 {{- toYaml (.Values.serviceAccount.operator).annotations | default .Values.serviceAccount.annotations | nindent 4}}

As .Values.serviceAccount.operator is true, it takes the annotations although are empty, not using ever the default vale, breaking the old behaviour

jkremser commented 1 month ago

thanks again @JorTurFer for taking this one (lgtm)