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

feat: support BoundServiceAccountToken triggerAuth provider #701

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

maxcao13
Copy link

@maxcao13 maxcao13 commented Nov 6, 2024

Updates CRDs to support BoundServiceAccountToken trigger auth provider/source. Also adds a helm value of type array called serviceAccountTokenCreationRoles which allows you to add objects of name and namespace corresponding to service accounts in the cluster that you'd like the keda-operator to be able to request service account tokens from.

Checklist

  • I have verified that my change is according to the deprecations & breaking changes policy
  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • [N/A] README is updated with new configuration values (if applicable) learn more
  • A PR is opened to update KEDA core (repo) (if applicable, ie. when deployment manifests are modified)

Related to kedacore/keda#6272

@maxcao13 maxcao13 requested a review from a team as a code owner November 6, 2024 00:39
@maxcao13 maxcao13 force-pushed the bound-service-account-token branch from 9a06a5d to 923b1d2 Compare November 29, 2024 18:45
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.

As default, KEDA doesn't have permission in the RBAC to create these tokens (and not granting it as default is worth IMHO), but maybe we could add an option to include the required permissions if users enable them (requiring explicit activation to include the required extra RBAC)

@maxcao13 maxcao13 force-pushed the bound-service-account-token branch from 923b1d2 to bac167e Compare February 4, 2025 07:40
@maxcao13
Copy link
Author

maxcao13 commented Feb 4, 2025

Added a helm value of serviceAccountTokenCreationRoles which allows you to add objects of name and namespace corresponding to service accounts in the cluster that you'd like the keda-operator to be able to request service account tokens from. This is in: bac167e

I'm sort of new to writing helm charts, so please let me know if a restructure or renaming is needed here.

@@ -146,4 +146,51 @@ subjects:
- kind: ServiceAccount
name: {{ (.Values.serviceAccount.operator).name | default .Values.serviceAccount.name }}
namespace: {{ .Release.Namespace }}
{{- if .Values.serviceAccountTokenCreationRoles }}
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure minimal-rbac.yaml is the correct place for this

@jkremser could you please suggest the best approach here?

Copy link
Author

@maxcao13 maxcao13 Feb 12, 2025

Choose a reason for hiding this comment

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

@zroubalik @wozniakjan Based off the KEDA meeting on Feb 11, I used #728 as a reference, but IDK if that was the intended direction for this PR (since I wasn't present). I also don't want to take credit for this change so if we are fine with this, maybe we can just merge that PR first.

keda/values.yaml Outdated Show resolved Hide resolved
@maxcao13 maxcao13 force-pushed the bound-service-account-token branch from bac167e to 3fe5b3c Compare February 12, 2025 20:41
@maxcao13 maxcao13 changed the title feat: update CRDs for BoundServiceAccountToken triggerAuth provider feat: support BoundServiceAccountToken triggerAuth provider Feb 12, 2025
@@ -140,6 +140,7 @@ their default values.
| `operator.replicaCount` | int | `1` | Capability to configure the number of replicas for KEDA operator. While you can run more replicas of our operator, only one operator instance will be the leader and serving traffic. You can run multiple replicas, but they will not improve the performance of KEDA, it could only reduce downtime during a failover. Learn more in [our documentation](https://keda.sh/docs/latest/operate/cluster/#high-availability). |
| `operator.revisionHistoryLimit` | int | `10` | ReplicaSets for this Deployment you want to retain (Default: 10) |
| `permissions.operator.restrict.namesAllowList` | list | `[]` | Array of strings denoting what secrets the KEDA operator will be able to read, this takes into account also the configured `watchNamespace`. the default is an empty array -> no restriction on the secret name |
| `permissions.operator.restrict.saTokens` | bool | `true` | Restrict Service Account Token Creation Access for KEDA operator |
Copy link
Member

Choose a reason for hiding this comment

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

I liked the previous approach, creating a RBAC permission service account by service account within a range. Just using a flag is quite open taking into account that these permissions allow to act in behalf of other service accounts, escalating privileges in the cluster. Using the range is aligned with the approach that we took for custom resources, where you can specify them one by one if you want (being more restrictive in this case and requiring to grant one by one)
Personally I prefer the range approach, but maybe @zroubalik or @wozniakjan have other opinion here

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