Skip to content

[helm]: Include default helpers, rename using fullname #1828

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.

Sign up for GitHub

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

Open
cabrinha wants to merge 12 commits into
base: main
Choose a base branch
from
+292 −237
Open

[helm]: Include default helpers, rename using fullname #1828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f8923f6
[helm]: Include default helpers, rename using fullname
cabrinha Dec 6, 2025
a614b77
Add vLLM Lora testing scripts (#1829)
Jeffwan Dec 6, 2025
5681d08
[Misc] Improve Lora edge cases test coverage (#1830)
Jeffwan Dec 6, 2025
7838814
Fixup service account naming and annotations
cabrinha Dec 6, 2025
03ed287
Merge branch 'main' into helpers-fullname
cabrinha Dec 6, 2025
e06468a
serviceAccount logic
cabrinha Dec 6, 2025
2087972
redis password
cabrinha Dec 6, 2025
fd39fe9
webhook-server-cert needs a hard-coded name
cabrinha Dec 7, 2025
6fb27af
add var to simplify redis init checks
cabrinha Dec 7, 2025
d80781c
separator
cabrinha Dec 7, 2025
83a57e2
redisHost
cabrinha Dec 8, 2025
b7c4399
Merge branch 'main' into helpers-fullname
cabrinha Dec 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 101 additions & 38 deletions dist/chart/templates/_helpers.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,4 @@
{{- define "chart.name" -}}
{{- if .Chart }}
{{- if .Chart.Name }}
{{- .Chart.Name | trunc 63 | trimSuffix "-" }}
{{- else if .Values.nameOverride }}
{{ .Values.nameOverride | trunc 63 | trimSuffix "-" }}
{{- else }}
aibrix
{{- end }}
{{- else }}
aibrix
{{- end }}
{{- end }}


{{- define "chart.labels" -}}
{{- if .Chart.AppVersion -}}
app.kubernetes.io/version: {{ .Chart.AppVersion | quote }}
{{- end }}
{{- if .Chart.Version }}
helm.sh/chart: {{ .Chart.Version | quote }}
{{- end }}
app.kubernetes.io/name: {{ include "chart.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
{{- end }}


{{- define "chart.selectorLabels" -}}
app.kubernetes.io/name: {{ include "chart.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
{{- end }}


{{- define "chart.hasMutatingWebhooks" -}}
{{- define "aibrix.hasMutatingWebhooks" -}}
{{- $d := dict "found" false -}}
{{- range . -}}
{{- if eq .type "mutating" -}}
Expand All @@ -42,7 +8,7 @@ app.kubernetes.io/instance: {{ .Release.Name }}
{{- $d.found -}}
{{- end -}}

{{- define "chart.hasValidatingWebhooks" -}}
{{- define "aibrix.hasValidatingWebhooks" -}}
{{- $d := dict "found" false -}}
{{- range . -}}
{{- if eq .type "validating" -}}
Expand All @@ -55,10 +21,107 @@ app.kubernetes.io/instance: {{ .Release.Name }}
{{/*
Renders imagePullSecrets block
*/}}
{{- define "chart.imagePullSecrets" -}}
{{- define "aibrix.imagePullSecrets" -}}
{{- $secrets := .componentSecrets | default .globalSecrets -}}
{{- if $secrets -}}
imagePullSecrets:
{{- toYaml $secrets | nindent 2 }}
{{- end -}}
{{- end -}}
{{- end -}}

{{/*
Expand the name of the chart.
*/}}
{{- define "aibrix.name" -}}
{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }}
{{- end }}

{{/*
Create a default fully qualified app name.
We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
If release name contains chart name it will be used as a full name.
*/}}
{{- define "aibrix.fullname" -}}
{{- if .Values.fullnameOverride }}
{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" }}
{{- else }}
{{- $name := default .Chart.Name .Values.nameOverride }}
{{- if contains $name .Release.Name }}
{{- .Release.Name | trunc 63 | trimSuffix "-" }}
{{- else }}
{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" }}
{{- end }}
{{- end }}
{{- end }}

{{/*
Create chart name and version as used by the chart label.
*/}}
{{- define "aibrix.chart" -}}
{{- printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimSuffix "-" }}
{{- end }}

{{/*
Common labels
*/}}
{{- define "aibrix.labels" -}}
helm.sh/chart: {{ include "aibrix.chart" . }}
{{ include "aibrix.selectorLabels" . }}
{{- if .Chart.AppVersion }}
app.kubernetes.io/version: {{ .Chart.AppVersion | quote }}
{{- end }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
{{- end }}

{{/*
Selector labels
*/}}
{{- define "aibrix.selectorLabels" -}}
app.kubernetes.io/name: {{ include "aibrix.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
{{- end }}

{{/*
Create the name of the controller manager service account
*/}}
{{- define "aibrix.controllerManager.serviceAccountName" -}}
{{- if .Values.controllerManager.serviceAccount.create -}}
{{- .Values.controllerManager.serviceAccount.name | default (printf "%s-controller-manager" (include "aibrix.fullname" .)) -}}
{{- else -}}
{{- .Values.controllerManager.serviceAccount.name | default "default" -}}
{{- end -}}
{{- end -}}

{{/*
Create the name of the gateway plugin service account
*/}}
{{- define "aibrix.gatewayPlugin.serviceAccountName" -}}
{{- if .Values.gatewayPlugin.serviceAccount.create -}}
{{- .Values.gatewayPlugin.serviceAccount.name | default (printf "%s-gateway-plugin" (include "aibrix.fullname" .)) -}}
{{- else -}}
{{- .Values.gatewayPlugin.serviceAccount.name | default "default" -}}
{{- end -}}
{{- end -}}


{{/*
Create the name of the gpu optimizer service account
*/}}
{{- define "aibrix.gpuOptimizer.serviceAccountName" -}}
{{- if .Values.gpuOptimizer.serviceAccount.create -}}
{{- .Values.gpuOptimizer.serviceAccount.name | default (printf "%s-gpu-optimizer" (include "aibrix.fullname" .)) -}}
{{- else -}}
{{- .Values.gpuOptimizer.serviceAccount.name | default "default" -}}
{{- end -}}
{{- end -}}

{{/*
Create the name of the metadata service service account
*/}}
{{- define "aibrix.metadata.serviceAccountName" -}}
{{- if .Values.metadata.serviceAccount.create -}}
{{- .Values.metadata.serviceAccount.name | default (printf "%s-metadata-service" (include "aibrix.fullname" .)) -}}
{{- else -}}
{{- .Values.metadata.serviceAccount.name | default "default" -}}
{{- end -}}
{{- end -}}
13 changes: 6 additions & 7 deletions dist/chart/templates/controller-manager/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,21 @@ kind: Deployment
metadata:
labels:
app.kubernetes.io/component: aibrix-controller-manager
{{- include "chart.labels" . | nindent 4 }}
name: aibrix-controller-manager
namespace: {{ .Release.Namespace }}
{{- include "aibrix.labels" . | nindent 4 }}
name: {{ include "aibrix.fullname" . }}-controller-manager
Copy link
Collaborator

@Jeffwan Jeffwan Dec 7, 2025

Choose a reason for hiding this comment

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

any concerns to use fixed component name?

Copy link
Author

@cabrinha cabrinha Dec 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If anyone wants to deploy more than 1 instance of aibrix chart to their cluster or rename it to something like aibrix-dev or something else, the fullname helper works here. Most helm charts use the fullname helper, which defaults the the chart name or the helm release name.

spec:
replicas: {{ .Values.controllerManager.replicas }}
selector:
matchLabels:
{{- include "chart.selectorLabels" . | nindent 6 }}
{{- include "aibrix.selectorLabels" . | nindent 6 }}
app.kubernetes.io/component: aibrix-controller-manager
template:
metadata:
labels:
{{- include "chart.labels" . | nindent 8 }}
{{- include "aibrix.labels" . | nindent 8 }}
app.kubernetes.io/component: aibrix-controller-manager
spec:
{{- include "chart.imagePullSecrets" (dict "componentSecrets" .Values.controllerManager.imagePullSecrets "globalSecrets" .Values.global.imagePullSecrets) | nindent 6 }}
{{- include "aibrix.imagePullSecrets" (dict "componentSecrets" .Values.controllerManager.imagePullSecrets "globalSecrets" .Values.global.imagePullSecrets) | nindent 6 }}
containers:
- args:
- --leader-elect
Expand Down Expand Up @@ -53,7 +52,7 @@ spec:
readOnly: true
securityContext:
runAsNonRoot: true
serviceAccountName: aibrix-controller-manager
serviceAccountName: {{ include "aibrix.controllerManager.serviceAccountName" . }}
terminationGracePeriodSeconds: 10
volumes:
- name: cert
Expand Down
39 changes: 16 additions & 23 deletions dist/chart/templates/controller-manager/rbac.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
apiVersion: v1
kind: ServiceAccount
metadata:
name: {{ include "aibrix.controllerManager.serviceAccountName" . }}
annotations:
{{- toYaml .Values.controllerManager.serviceAccount.annotations | nindent 4 }}
labels:
{{- include "chart.labels" . | nindent 4 }}
name: aibrix-controller-manager
namespace: {{ .Release.Namespace }}
{{- include "aibrix.labels" . | nindent 4 }}
---

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
name: aibrix-controller-manager-clusterrole
name: {{ include "aibrix.fullname" . }}-controller-manager
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 6, 2025

Choose a reason for hiding this comment

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

medium

While it's valid to have a ClusterRole and ClusterRoleBinding with the same name, it can be confusing. The previous naming convention used suffixes like -clusterrole and -clusterrolebinding which improved clarity. It's a good practice to use distinct names for different resource kinds.

This pattern of identical names for different kinds appears throughout the RBAC files in this PR. For example, the leader-election Role and RoleBinding in this file also share a name.

Could you re-introduce suffixes for clarity across all RBAC resources? For this ClusterRole, I suggest:

  name: {{ include "aibrix.fullname" . }}-controller-manager-clusterrole

Copy link
Author

@cabrinha cabrinha Dec 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is redundant because you can clearly see the differences between objects without the object type as a suffix on the name.

kubectl get role
kubectl get clusterrole
kubectl get rolebinding
kubectl get clusterrolebinding

Imagine if every k8s object had a suffix of it's kind. aibrix-deployment, aibrix-service, etc. So, I disagree here.

Copy link
Collaborator

@Jeffwan Jeffwan Dec 7, 2025

Choose a reason for hiding this comment

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

It works, we just need to make sure that upgrade experiences still smooth. Consider it will create new objects, I do not have concerns here

labels:
{{- include "chart.labels" . | nindent 4 }}
{{- include "aibrix.labels" . | nindent 4 }}
rules:
- apiGroups:
- ""
Expand Down Expand Up @@ -301,33 +301,28 @@ rules:
- list
- update
- watch


---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
labels:
{{- include "chart.labels" . | nindent 4 }}
name: aibrix-controller-manager-clusterrolebinding
{{- include "aibrix.labels" . | nindent 4 }}
name: {{ include "aibrix.fullname" . }}-controller-manager
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: aibrix-controller-manager-clusterrole
name: {{ include "aibrix.fullname" . }}-controller-manager
subjects:
- kind: ServiceAccount
name: aibrix-controller-manager
name: {{ include "aibrix.controllerManager.serviceAccountName" . }}
namespace: {{ .Release.Namespace }}


---
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: aibrix-controller-manager-leader-election-role
namespace: {{ .Release.Namespace }}
name: {{ include "aibrix.fullname" . }}-controller-manager-leader-election
labels:
{{- include "chart.labels" . | nindent 4 }}
{{- include "aibrix.labels" . | nindent 4 }}
rules:
- apiGroups:
- ""
Expand Down Expand Up @@ -360,20 +355,18 @@ rules:
verbs:
- create
- patch

---
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
name: aibrix-controller-manager-leader-election-rolebinding
namespace: {{ .Release.Namespace }}
name: {{ include "aibrix.fullname" . }}-controller-manager-leader-election
labels:
{{- include "chart.labels" . | nindent 4 }}
{{- include "aibrix.labels" . | nindent 4 }}
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: Role
name: aibrix-controller-manager-leader-election-role
name: {{ include "aibrix.fullname" . }}-controller-manager-leader-election
subjects:
- kind: ServiceAccount
name: aibrix-controller-manager
name: {{ include "aibrix.controllerManager.serviceAccountName" . }}
namespace: {{ .Release.Namespace }}
9 changes: 4 additions & 5 deletions dist/chart/templates/controller-manager/service.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
apiVersion: v1
kind: Service
metadata:
name: aibrix-controller-manager-metrics-service
namespace: {{ .Release.Namespace }}
name: {{ include "aibrix.fullname" . }}-controller-manager-metrics
annotations:
prometheus.io/path: /metrics
prometheus.io/port: "8080"
prometheus.io/scrape: "true"
labels:
{{- include "chart.labels" . | nindent 4 }}
{{- include "aibrix.labels" . | nindent 4 }}
app.kubernetes.io/component: aibrix-controller-manager
spec:
ports:
Expand All @@ -17,5 +16,5 @@ spec:
protocol: TCP
targetPort: 8080
selector:
{{- include "chart.selectorLabels" . | nindent 4 }}
app.kubernetes.io/component: aibrix-controller-manager
{{- include "aibrix.selectorLabels" . | nindent 4 }}
app.kubernetes.io/component: aibrix-controller-manager
Loading