Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ServiceAccount are correctly annotated #636

Merged
merged 7 commits into from
May 3, 2024

Conversation

JorTurFer
Copy link
Member

@JorTurFer JorTurFer commented May 1, 2024

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 kedacore/keda#5758

@JorTurFer JorTurFer requested a review from a team as a code owner May 1, 2024 08:36
@JorTurFer JorTurFer mentioned this pull request May 1, 2024
3 tasks
Signed-off-by: Jorge Turrado <[email protected]>
@jkremser
Copy link
Contributor

jkremser commented May 3, 2024

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
Copy link
Member Author

Default values provided by chart's values.yaml set them to true:

charts/keda/values.yaml

Lines 290 to 317 in f9f23e9

serviceAccount:
operator:
# -- Specifies whether a service account should be created
create: true
# -- The name of the service account to use.
name: keda-operator
# -- Specifies whether a service account should automount API-Credentials
automountServiceAccountToken: true
# -- Annotations to add to the service account
annotations: {}
metricServer:
# -- Specifies whether a service account should be created
create: true
# -- The name of the service account to use.
name: keda-metrics-server
# -- Specifies whether a service account should automount API-Credentials
automountServiceAccountToken: true
# -- Annotations to add to the service account
annotations: {}
webhooks:
# -- Specifies whether a service account should be created
create: true
# -- The name of the service account to use.
name: keda-webhook
# -- Specifies whether a service account should automount API-Credentials
automountServiceAccountToken: true
# -- Annotations to add to the service account
annotations: {}

@JorTurFer
Copy link
Member Author

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

Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
@jkremser
Copy link
Contributor

jkremser commented May 3, 2024

thanks again @JorTurFer for taking this one
(lgtm)

README.md Show resolved Hide resolved
@JorTurFer JorTurFer merged commit 407afac into kedacore:main May 3, 2024
37 checks passed
@JorTurFer JorTurFer deleted the fix-annotations branch May 3, 2024 14:04
QuentinBisson pushed a commit to giantswarm/keda-upstream that referenced this pull request Jul 1, 2024
* fix: ServiceAccount are correctly annotated

Signed-off-by: Jorge Turrado <[email protected]>

* Rebase main changes

Signed-off-by: Jorge Turrado <[email protected]>

* update missing add-on version

Signed-off-by: Jorge Turrado <[email protected]>

* revert some changes

Signed-off-by: Jorge Turrado <[email protected]>

* revert some changes

Signed-off-by: Jorge Turrado <[email protected]>

* revert some changes

Signed-off-by: Jorge Turrado <[email protected]>

* repack the helm chart

Signed-off-by: Jorge Turrado <[email protected]>

---------

Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: QuentinBisson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Annotations on keda-operator service account not reflecting
3 participants