-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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: Add service account #14917
feat: Add service account #14917
Conversation
@@ -53,6 +53,9 @@ spec: | |||
app: {{ template "superset.name" . }}-worker | |||
release: {{ .Release.Name }} | |||
spec: | |||
{{- if .Values.serviceAccount.create }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of guarding this with .Values.serviceAccount.create
, we should define the name of the SA to use in values (default of empty, or null) and then use the SA name in this condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done exactly this in my PR. Please review.
@@ -56,6 +56,9 @@ spec: | |||
app: {{ template "superset.name" . }} | |||
release: {{ .Release.Name }} | |||
spec: | |||
{{- if .Values.serviceAccount.create }} | |||
serviceAccountName: {{ template "superset.name" . }} | |||
{{- end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above
apiVersion: v1 | ||
kind: ServiceAccount | ||
metadata: | ||
name: {{ template "superset.name" . }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the SA name, we should allow the user to customize via values.yaml
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
# | ||
apiVersion: v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should Superset chart create Service Account? I suggest we simplify and assume that a user will create a SA with needed permissions. It may be done in several ways. For example it's more common to create IRSA in AWS with Terraform.
SUMMARY
I am adding this chart to my gke cluster, and I don't like the concept of adding my gcp service account secrets to a k8s secret and I plan to mount my service-account to the k8s service account as a workload