Skip to content

Feat(helm): Modify the existing serviceAccount for use by router and lora-controller #1005

Open
ghkdwlgns612 wants to merge 2 commits into
vllm-project:mainfrom
ghkdwlgns612:feat/router-lora-sa
Open

Feat(helm): Modify the existing serviceAccount for use by router and lora-controller #1005
ghkdwlgns612 wants to merge 2 commits into
vllm-project:mainfrom
ghkdwlgns612:feat/router-lora-sa

Conversation

@ghkdwlgns612

Copy link
Copy Markdown
Contributor

Summary

I wanted to use pre-existing service accounts directly for the router and lora-controller Deployments.
To support this, when serviceAccountName is explicitly set, the chart now skips creating ServiceAccount, Role/ClusterRole, and RoleBinding/ClusterRoleBinding resources.

Root Cause

The previous Helm templates assumed chart-managed identities and RBAC by default, so they always rendered service-account and RBAC resources when router/lora-controller were enabled. This did not support environments where service accounts and permissions are provisioned externally.

Changes

  • Add routerSpec.serviceAccountName field to values.yaml— when set, use the specified SA for the router deployment
  • Add loraController.serviceAccountName field to values.yaml— when set, use the specified SA for the lora-controller deployment
  • Skip ServiceAccount creation when a custom serviceAccountName is provided
  • Skip Role/RoleBinding creation for router when a custom serviceAccountName is provided
  • Skip ClusterRole/ClusterRoleBinding creation for lora-controller when a custom serviceAccountName is provided

…nd lora-controller to skip SA/Role/RoleBinding creation

Signed-off-by: ghkdwlgns612 <ghkdwlgns612@naver.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request adds support for configuring custom service accounts for the router and LoRA controller deployments in the Helm chart, skipping the creation of the default ServiceAccount resources when custom names are provided. The feedback suggests a key improvement: instead of skipping the creation of RBAC roles and role bindings when a custom service account is specified, the chart should still manage and create these RBAC resources but dynamically bind them to the custom service account. This ensures that application-specific permissions are maintained by the chart while allowing integration with externally managed service accounts.

Comment thread helm/templates/role.yaml
@@ -1,4 +1,4 @@
{{- if .Values.routerSpec.enableRouter -}}
{{- if and .Values.routerSpec.enableRouter (not .Values.routerSpec.serviceAccountName) -}}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Decoupling ServiceAccount creation from Role/ClusterRole creation is a standard Helm best practice. In many enterprise environments, the ServiceAccount is pre-created externally (e.g., to associate it with AWS IRSA or GCP Workload Identity), but the Helm chart is still expected to manage and create the application-specific RBAC roles and bindings.

By skipping Role creation when serviceAccountName is set, users are forced to manually define and maintain these complex RBAC rules externally, which is error-prone and hard to maintain during chart upgrades.

Instead, we should always create the Role and RoleBinding resources when the component is enabled, but bind them to the custom serviceAccountName if provided.

{{- if .Values.routerSpec.enableRouter -}}

Comment thread helm/templates/role.yaml
{{- end }}

{{- if .Values.loraController.enableLoraController }}
{{- if and .Values.loraController.enableLoraController (not .Values.loraController.serviceAccountName) }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Similarly for the lora-controller, we should not skip the ClusterRole creation when a custom serviceAccountName is provided. This ensures that the required permissions are still managed by the chart while using the externally provisioned service account.

{{- if .Values.loraController.enableLoraController }}

Comment on lines +1 to 15
{{- if and .Values.routerSpec.enableRouter (not .Values.routerSpec.serviceAccountName) -}}
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
name: {{ .Release.Name }}-deployment-access-binding
namespace: {{ .Release.Namespace }}
subjects:
- kind: ServiceAccount
name: {{ .Release.Name }}-router-service-account
name: {{ printf "%s-router-service-account" .Release.Name }}
namespace: {{ .Release.Namespace }}
roleRef:
kind: Role
name: {{ .Release.Name }}-reader
apiGroup: rbac.authorization.k8s.io
{{- end }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

To support using a custom serviceAccountName while still letting the chart manage the RoleBinding, we should keep the RoleBinding enabled and dynamically set the subject's name to the custom service account if provided.

{{- if .Values.routerSpec.enableRouter -}}
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  name: {{ .Release.Name }}-deployment-access-binding
  namespace: {{ .Release.Namespace }}
subjects:
  - kind: ServiceAccount
    name: {{ .Values.routerSpec.serviceAccountName | default (printf "%s-router-service-account" .Release.Name) }}
    namespace: {{ .Release.Namespace }}
roleRef:
  kind: Role
  name: {{ .Release.Name }}-reader
  apiGroup: rbac.authorization.k8s.io
{{- end }}

{{- end }}

{{- if .Values.loraController.enableLoraController }}
{{- if and .Values.loraController.enableLoraController (not .Values.loraController.serviceAccountName) }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Keep the ClusterRoleBinding enabled even when a custom serviceAccountName is provided.

{{- if .Values.loraController.enableLoraController }}

Comment on lines 30 to 33
subjects:
- kind: ServiceAccount
name: {{ .Release.Name }}-lora-controller
name: {{ printf "%s-lora-controller" .Release.Name }}
namespace: {{ .Release.Namespace }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Bind the ClusterRoleBinding to the custom serviceAccountName if specified.

subjects:
- kind: ServiceAccount
  name: {{ .Values.loraController.serviceAccountName | default (printf "%s-lora-controller" .Release.Name) }}
  namespace: {{ .Release.Namespace }}

Comment on lines 47 to 50
subjects:
- kind: ServiceAccount
name: {{ .Release.Name }}-lora-controller
name: {{ printf "%s-lora-controller" .Release.Name }}
namespace: {{ .Release.Namespace }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Bind the leader election ClusterRoleBinding to the custom serviceAccountName if specified.

subjects:
- kind: ServiceAccount
  name: {{ .Values.loraController.serviceAccountName | default (printf "%s-lora-controller" .Release.Name) }}
  namespace: {{ .Release.Namespace }}

Comment on lines +25 to +34
- it: should NOT create Role and RoleBinding when custom serviceAccountName is set
release:
name: vllm
set:
routerSpec:
enableRouter: true
serviceAccountName: my-existing-sa
asserts:
- hasDocuments:
count: 0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Since we want to keep the Role and RoleBinding creation enabled and bind them to the custom service account, we should update this test to assert that they are created and bound to the custom service account name.

  - it: should create Role and RoleBinding with custom serviceAccountName when specified
    release:
      name: vllm
    set:
      routerSpec:
        enableRouter: true
        serviceAccountName: my-existing-sa
    asserts:
    - containsDocument:
        kind: Role
        apiVersion: rbac.authorization.k8s.io/v1
        name: vllm-reader
      template: role.yaml
    - containsDocument:
        kind: RoleBinding
        apiVersion: rbac.authorization.k8s.io/v1
        name: vllm-deployment-access-binding
      template: rolebinding.yaml
    - equal:
        path: subjects[0].name
        value: my-existing-sa
      documentIndex: 0
      template: rolebinding.yaml

Comment on lines +65 to +76
- it: should NOT create ClusterRole and ClusterRoleBinding when lora serviceAccountName is set
release:
name: vllm
set:
routerSpec:
enableRouter: false
loraController:
enableLoraController: true
serviceAccountName: my-existing-lora-sa
asserts:
- hasDocuments:
count: 0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Update this test to assert that the ClusterRole and ClusterRoleBinding are created and bound to the custom my-existing-lora-sa service account.

  - it: should create ClusterRole and ClusterRoleBinding with custom serviceAccountName when specified
    release:
      name: vllm
    set:
      routerSpec:
        enableRouter: false
      loraController:
        enableLoraController: true
        serviceAccountName: my-existing-lora-sa
    asserts:
    - equal:
        path: metadata.name
        value: vllm-lora-controller
      documentIndex: 0
      template: role.yaml
    - equal:
        path: metadata.name
        value: vllm-lora-controller
      documentIndex: 0
      template: rolebinding.yaml
    - equal:
        path: subjects[0].name
        value: my-existing-lora-sa
      documentIndex: 0
      template: rolebinding.yaml

Signed-off-by: ghkdwlgns612 <ghkdwlgns612@naver.com>
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.

1 participant