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

AutomountServiceAccountToken property implemented #258

Merged
merged 1 commit into from
Jul 4, 2022
Merged

AutomountServiceAccountToken property implemented #258

merged 1 commit into from
Jul 4, 2022

Conversation

ph311o
Copy link
Contributor

@ph311o ph311o commented Mar 24, 2022

Added new property "automountServiceAccountToken" in ServiceAccounts and Deployments necessary for restricted environments.

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • README is updated with new configuration values (if applicable)

Fixes #

@ph311o ph311o requested a review from a team as a code owner March 24, 2022 09:35
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a quick review, thanks for the change!

@zroubalik Do you want to have these changes in our generated YAML files?

keda/Chart.yaml Outdated Show resolved Hide resolved
keda/templates/12-keda-deployment.yaml Outdated Show resolved Hide resolved
keda/templates/22-metrics-deployment.yaml Outdated Show resolved Hide resolved
keda/values.yaml Outdated Show resolved Hide resolved
Signed-off-by: Christian Kuhn <phello@gmx.de>
@JorTurFer
Copy link
Member

Hey,
I'm looking for information about this feature and I can't find any clear use case (or most probably, I'm not understanding them).
Could you explain your use case?

@ph311o
Copy link
Contributor Author

ph311o commented Apr 21, 2022

@JorTurFer Hi. Our UseCase is as follow:

In general Kubernetes automounts service account credentials which allows any compromised pod to run API commands against the cluster. We are restricted to stop this behaviour by specifying automountServiceAccountToken: false in ServiceAccount. This is a global rule so we cannot whitelist the helm chart.

As Keda and metrics deployments/pods need API access we need to opt in for these two with automountServiceAccountToken: true in both deployment.yamls

When you set both ServiceAccount and Deployments automountServiceAccountToken: true it's the same like not specifying at all so I am not sure if it makes sense to specify it in the installation.yaml. We need it in Helm Chart as said to disable default ServiceAccount behaviour and ensure functionality.

Also see:

@JorTurFer
Copy link
Member

JorTurFer commented Apr 21, 2022

I got the reasons (I don't understand the document in German but I guess that it's an official regulation).
In this case, what is the difference between having automountServiceAccountToken true or false in the service account if we are only using it for deployments?
I mean, maybe this is a non-optional improvement, if the behavior is the same just disabling it at service account and enabling them in deployments... @kedacore/keda-helm-contributors ?

@ph311o ph311o requested a review from tomkerkhove April 22, 2022 23:03
@ph311o
Copy link
Contributor Author

ph311o commented May 9, 2022

@JorTurFer Do you need more informations from my side or do we wait for more opinions by other maintainers? In general you can disable it in ServiceAccount and enable it in Deployments hardcoded. But then everybody who would use the Keda ServiceAccount perhaps for other deployments could not longer access API. This was the reason why I decided to implement a switch in values.yaml for ServiceAccount.

@zroubalik
Copy link
Member

I think it is okay to have an optional switch, ie. default to the existing behavior.

@JorTurFer
Copy link
Member

I think it is okay to have an optional switch, ie. default to the existing behavior.

So we can merge this PR because it keeps the current behaviour as default behaviour, can't?

@ph311o
Copy link
Contributor Author

ph311o commented May 19, 2022

@JorTurFer Yes absolutely it keeps current behaviour. Would be great if anybody could merge it for next release. Thank you.

@tomkerkhove
Copy link
Member

Would love to have a sign-off of @zroubalik or @JorTurFer as well though

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

F**k, I missed this PR between the notifications :(
If the behavior doesn't change, I don't have any problem to merge it

@tomkerkhove tomkerkhove merged commit 0646721 into kedacore:main Jul 4, 2022
@tomkerkhove
Copy link
Member

Thank you @ph311o and sorry for being slow on our end!

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.

4 participants