Add .create and .name to serviceAccount config, and decouple rbac.enable from the service accounts

[martimors]

This feature introduces a new .serviceAccount.create for all
ServiceAccount resources, which defaults to true. This is a breaking
change for all users with rbac.enabled set to false, as these users will get some ServiceAccount resources created where they previously ran as the default user.

For context, please see the discussion at #2691 .

The use-case for this feature is to allow users who use rbac.enabled=false to provision serviceAccounts without rbac resources like Role and RoleBinding. It also limits the rbac.enabled to only refer to resources that relate to the actual rbac API in kubernetes.

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

[martimors]

So seems the CI pipeline adds some commits, but I think a squash merge of this PR should fix that. Please let me know if you'd prefer I rebase+force push the branch with the auto-fixes squashed into one commit.

[consideRatio]

Wiee hey @dingobar, thanks for working on this topic!

You can pull and manually fixup/squash the formatting commits if you want, but you can also leave them in. I use pre-commit locally to ensure my commits are formatted locally overall, for notes on how to do that see the notes in the .pre-commit-config.yaml file.

I've added a lengthier note about the kind of changes I think makes sense to bundle in a single PR in #2691 (comment). What do you think about expanding this PR a bit to include what's described there?

[consideRatio]

The - to the right and left of the {{ and }} are related to "whitespace chomping".

A rule of thumb to avoid rendering surplus new lines etc we've tried to follow is:

1. Always whitespace chomp left
2. Never whitespace chomp right, except for when it's before any previous content has been rendered in the template, then always whitespace chomp right as well.

For this line, we should also whitespace chomp right to follow that rule. It is not going to make things break etc, but it makes the output contain less surplus new lines when rendering it.

[martimors]

I've implemented this across all rbac.yaml and serviceaccount.yaml files.

[martimors]

@consideRatio I've implemented the changes now, so that a custom [...].serviceAccount.name can also be given. The current logic is this:

1. If rbac.enabled=false, no Role, RoleBinding, ClusterRole and ClusterRoleBinding resources are created. This has no impact on the creation of ServiceAccount resources. Setting [...].serviceAccount.create=false and rbac.enabled=true will most likely cause an error, because the *RoleBinding resources won't be able to find the referenced serviceaccount. I think this is ok.
2. If [...].serviceAccount.create=false, no ServiceAccount resource is created, and none is referenced by Deployment or Job resources either.
3. If [...].serviceAccount.create=true and [...].serviceAccount.name=null, a serviceaccount is created with a default name corresponding to the name of the Deployment/Job that ends up using it.
4. If [...].serviceAccount.create=true and [...].serviceAccount.name="foobar"`, a serviceaccount is created and referenced with the given name.

Known limitations/considerations:

• If the user provides the same name for two serviceaccounts, there will be a naming conflict in the namespace and I'm not sure how the deployment will behave. I think we can look past this edge-case.
• Currently, I have not implemented functionality to allow referencing a serviceaccount by name without creating it. I'm not sure what a good pattern to introduce this would be, but I have an idea of what would be the most clean. We could remove the possibility of running without a serviceaccount (ie. using the default for the namespace). Then we could just remove the if/else around the serviceAccount: field of Deployment/Job resources and always reference a serviceaccount no matter the value of serviceAccount.create. I would prefer this to any other solution to this but it's unfortunately another breaking change in case someone is currently running with rbac.enabled=false and giving special permission to the namespace default serviceaccount. What do you think?

Turned out that quite a few changes were necessary to get this far so will pause here for some feedback.

[consideRatio]

Thanks for documenting your work so clearly @dingobar!!

Various config combinations seems ok

I'm considering various combinations of these configurations. I was a bit surprised about the idea of creating a SA with a specified name, but yeah - that is probably reasonable. Those that wants to specify an existing SA, which is something I think people have wanted historically, can would then set create=false and name=something.

1. rbac.enabled=true
   We create the Role and RoleBinding, so we need to be able to reference a SA, otherwise we have a guaranteed failure.

   create	name	note
   false	something	User creates SA with specified name
   false	null	Broken configuration - RoleBindings have no SA to point to
   true	something	Chart creates SA with specified name
   true	null	Chart creates SA with default name

2. rbac.enabled=false

   create	name	note
   false	something	User creates SA with specified name
   false	null	Possibly not broken as this is what you would have pre-rbac permissions were around, but it is probably not a configuration people have these days.
   true	something	Chart creates SA with specified name
   true	null	Chart creates SA with default name

These options seems fine to me. I considered if we should add logic to error loudly if this is configured to the broken configuration state, but I think such validation logic in the schema.yaml file, or having a conditional fail command in NOTES.txt would be a bit messy to do for all service accounts.

Change request 1

Users will need to be allowed to specify a SA without the helm chart creating one. Due to that, I suggest that the following changes are made.

# from pod specifications where we specify serviceAccountName
-      {{- if .Values.hub.serviceAccount.create }}
-      serviceAccountName: {{ include "jupyterhub.hub-serviceaccount.fullname" . }}
+      {{- with include "jupyterhub.hub-serviceaccount.fullname" . }}
+      serviceAccountName: {{ . }}
       {{- end }}

# from _helpers-names.tpl
 {{- define "jupyterhub.hub-serviceaccount.fullname" -}}
-    {{- .Values.hub.serviceAccount.name | default (include "jupyterhub.hub.fullname" .) }}
+   {{- if .Values.hub.serviceAccount.name }}
+   {{- .Values.hub.serviceAccount.name }}
+   {{- else if .Values.hub.serviceAccount.create }}
+   {{- include "jupyterhub.hub.fullname" . }}
+   {{- end }}
 {{- end }}

Change request 2

There is a section you can find by searching for {{- define "jupyterhub.name-templates" -}} that should have entries for all resources. Update that list with the new .fullname named templates you have created.

Change request 3

All resources in this helm chart skip the leading --- in the file besides a few of these you now add. Remove those that doesn't act as separators between resources.

[desaintmartin]

We collided on this one, I've just made #2737 that has just been closed, here are my comments from the work I've done.

[desaintmartin]

Suggested change

	{{- .Values.hub.serviceAccount.name | default (include "jupyterhub.hub.fullname" .) }}
	{{- if .Values.hub.serviceAccount.create -}}
	{{ default (include "jupyterhub.hub.fullname" .) .Values.hub.serviceAccount.name }}
	{{- else -}}
	{{ default "default" .Values.hub.serviceAccount.name }}
	{{- end -}}

As it is done in helm boilerplate?

[consideRatio]

This change includes two parts of relevance:

1. serviceAccountName is always set, even when .create is false and .name is unspecified.
2. Style change, writing default <default value> <primary value> instead of <primary value> | default <default value>.
3. Style change, whitespace chomping practices

I don't want to accept the style changes as they are consistent across the Helm chart in a manner, but I think its acceptable to agree on the change 1 as it reduces the complexity of our template logic, making it easier to read.

@dingobar I'll let you decide about 1 as you also would be the person to do the work and I find both options fine.

[martimors]

While I see the original functionality good and easy to understand, I buy the argument that since the default helm-chart base also sets "default" as the serviceAccount in the pods we should also do it.

[martimors]

Thanks for documenting your work so clearly @dingobar!!

Various config combinations seems ok

[...]

Change request 1

Users will need to be allowed to specify a SA without the helm chart creating one. Due to that, I suggest that the following changes are made.

[...]

Change request 2

There is a section you can find by searching for {{- define "jupyterhub.name-templates" -}} that should have entries for all resources. Update that list with the new .fullname named templates you have created.

Change request 3

All resources in this helm chart skip the leading --- in the file besides a few of these you now add. Remove those that doesn't act as separators between resources.

I believe these changes are implemented now. @desaintmartin I'm not able to re-request a review from you but feel free.

[martimors]

I've included the last CRs now. Hopefully we can merge this now.

[consideRatio]

Thank you @dingobar and @desaintmartin for your work on this!!!!!! ❤️ 🎉 🌻

[welcome]

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️