aicam commented on code in PR #8986:
URL: https://github.com/apache/texera/pull/8986#discussion_r4365611811
##########
bin/k8s/templates/base/workflow-computing-unit-manager/workflow-computing-unit-manager-service-account.yaml:
##########
@@ -63,3 +63,30 @@ roleRef:
kind: Role
name: {{ .Values.workflowComputingUnitManager.name }}
apiGroup: rbac.authorization.k8s.io
+
+---
+# Read-only and cluster-wide for the admin cluster-capacity endpoint: nodes
are cluster-scoped and
+# every namespace's pods take their capacity, so the namespaced Role above
cannot cover it.
+# Named for the release because a ClusterRole is cluster-scoped.
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+ name: {{ .Release.Name }}-{{ .Values.workflowComputingUnitManager.name
}}-cluster-capacity
Review Comment:
ClusterRole and ClusterRoleBinding are cluster-scoped, so their names must
be unique across the whole cluster, but Helm release names are only unique per
namespace. Two releases with the same name in different namespaces (e.g. `helm
install texera -n team-a` and `helm install texera -n team-b`) render the same
name here, and the second install fails:
```
Error: ClusterRole "texera-...-cluster-capacity" exists and cannot be
imported into the current release:
invalid ownership metadata; annotation validation error: key
"meta.helm.sh/release-namespace" must equal "team-b": current value is "team-a"
```
Even if forced through, both releases would share one object, and
uninstalling either would delete it and break the other's capacity endpoint
(403).
Including `.Release.Namespace` makes the name unique per release. The same
change is needed on the ClusterRoleBinding name and its `roleRef.name`
(suggestions below), and the comment on line 70 could say "Named for the
release and its namespace".
```suggestion
name: {{ .Release.Name }}-{{ .Release.Namespace }}-{{
.Values.workflowComputingUnitManager.name }}-cluster-capacity
```
##########
bin/k8s/templates/base/workflow-computing-unit-manager/workflow-computing-unit-manager-service-account.yaml:
##########
@@ -63,3 +63,30 @@ roleRef:
kind: Role
name: {{ .Values.workflowComputingUnitManager.name }}
apiGroup: rbac.authorization.k8s.io
+
+---
+# Read-only and cluster-wide for the admin cluster-capacity endpoint: nodes
are cluster-scoped and
+# every namespace's pods take their capacity, so the namespaced Role above
cannot cover it.
+# Named for the release because a ClusterRole is cluster-scoped.
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+ name: {{ .Release.Name }}-{{ .Values.workflowComputingUnitManager.name
}}-cluster-capacity
+rules:
+ - apiGroups: [""]
+ resources: ["nodes", "pods"]
+ verbs: ["list"]
+
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRoleBinding
+metadata:
+ name: {{ .Release.Name }}-{{ .Values.workflowComputingUnitManager.name
}}-cluster-capacity
Review Comment:
Same fix here so the names stay in sync.
```suggestion
name: {{ .Release.Name }}-{{ .Release.Namespace }}-{{
.Values.workflowComputingUnitManager.name }}-cluster-capacity
```
##########
bin/k8s/templates/base/workflow-computing-unit-manager/workflow-computing-unit-manager-service-account.yaml:
##########
@@ -63,3 +63,30 @@ roleRef:
kind: Role
name: {{ .Values.workflowComputingUnitManager.name }}
apiGroup: rbac.authorization.k8s.io
+
+---
+# Read-only and cluster-wide for the admin cluster-capacity endpoint: nodes
are cluster-scoped and
+# every namespace's pods take their capacity, so the namespaced Role above
cannot cover it.
+# Named for the release because a ClusterRole is cluster-scoped.
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+ name: {{ .Release.Name }}-{{ .Values.workflowComputingUnitManager.name
}}-cluster-capacity
+rules:
+ - apiGroups: [""]
+ resources: ["nodes", "pods"]
+ verbs: ["list"]
+
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRoleBinding
+metadata:
+ name: {{ .Release.Name }}-{{ .Values.workflowComputingUnitManager.name
}}-cluster-capacity
+subjects:
+ - kind: ServiceAccount
+ name: {{ .Values.workflowComputingUnitManager.serviceAccountName }}
+ namespace: {{ .Release.Namespace }}
+roleRef:
+ kind: ClusterRole
+ name: {{ .Release.Name }}-{{ .Values.workflowComputingUnitManager.name
}}-cluster-capacity
Review Comment:
Same fix here so the names stay in sync.
```suggestion
name: {{ .Release.Name }}-{{ .Release.Namespace }}-{{
.Values.workflowComputingUnitManager.name }}-cluster-capacity
```
--
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]
