dampcake commented on code in PR #13747:
URL: https://github.com/apache/druid/pull/13747#discussion_r1100528686
##########
helm/druid/templates/broker/roleBinding.yaml:
##########
@@ -0,0 +1,20 @@
+{{- if .Values.rbac.create }}
Review Comment:
Same question about checking enabled for the druid service, eg:
`.Values.broker.enabled`
##########
helm/druid/templates/broker/role.yaml:
##########
@@ -0,0 +1,21 @@
+{{- if .Values.rbac.create }}
Review Comment:
Should this also be checking `.Values.broker.enabled`? Feels like disabling
the broker should also disable these resources from being created.
##########
helm/druid/templates/broker/role.yaml:
##########
@@ -0,0 +1,21 @@
+{{- if .Values.rbac.create }}
+apiVersion: rbac.authorization.k8s.io/v1
+kind: Role
+metadata:
+ name: {{ template "druid.broker.fullname" . }}
+ labels:
+ app: {{ include "druid.name" . }}
+ chart: {{ include "druid.chart" . }}
+ component: {{ .Values.broker.name }}
+ release: {{ .Release.Name }}
+ heritage: {{ .Release.Service }}
+rules:
+ - apiGroups:
+ - ""
+ resources:
+ - pods
+ - configmaps
+ verbs:
+ - '*'
Review Comment:
Unclear on why the broker needs access to delete Pods, or really any of
these. Are these permissions only needed by the Overlord?
##########
helm/druid/templates/broker/deployment.yaml:
##########
@@ -41,11 +41,15 @@ spec:
app: {{ include "druid.name" . }}
release: {{ .Release.Name }}
component: {{ .Values.broker.name }}
- {{- with .Values.broker.podAnnotations }}
annotations:
-{{ toYaml . | indent 8 }}
+ druid.k8s.enablePatching: "true"
+ {{- with .Values.broker.podAnnotations }}
+ {{- toYaml . | nindent 8 }}
{{- end }}
spec:
+ {{- if .Values.broker.serviceAccount.create }}
Review Comment:
This blocks external creation of the serviceAccount. Most other charts allow
you to bring your own serviceAccount which may also make reusing the same one
for all druid service types easier.
I am wondering if we should default serviceAccount.name to unset and if
create is true it uses what the default would be. Then if serviceAccount.name
is set and serviceAccount.create is false we can still set the value here.
The charts I am used to using have a template like:
```
{{/*
Create the name of the forwarder service account to use
*/}}
{{- define "fluentd.forwarder.serviceAccountName" -}}
{{- if .Values.forwarder.serviceAccount.create -}}
{{ default (printf "%s-forwarder" (include "common.names.fullname" .))
.Values.forwarder.serviceAccount.name }}
{{- else -}}
{{ default "default" .Values.forwarder.serviceAccount.name }}
{{- end -}}
{{- end -}}
```
and then always set the serviceAccountName with the output of the template.
##########
helm/druid/templates/broker/serviceAccount.yaml:
##########
@@ -0,0 +1,20 @@
+{{- if .Values.broker.serviceAccount.create }}
Review Comment:
Same question about checking enabled for the druid service, eg:
`.Values.broker.enabled`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
