Skip to content
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

Add ...serviceAccount.annotations config for our k8s ServiceAccounts #2236

Merged
merged 2 commits into from
Jun 7, 2021

Conversation

AndreaGiardini
Copy link
Contributor

I am using this helm chart on GKE. The preferred method to authorize GKE workloads is using Google workload identity ( https://cloud.google.com/kubernetes-engine/docs/how-to/workload-identity )

In order to use workload identity it's necessary to add a specific annotation( iam.gke.io/gcp-service-account ) to the k8s serviceaccount. In this way we can link the k8s serviceaccount with google's service account.

Adding annotations to the hub's serviceAccount was not possible with the current helm chart so I implemented it in this PR.

Let me know if you have any question

@welcome
Copy link

welcome bot commented May 31, 2021

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.
welcome
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! 🎉

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

Feel free to come with suggestions, but I need more time to think myself still

Comment on lines 6 to 9
annotations:
{{- with .Values.rbac.serviceAccount.annotations }}
{{- . | toYaml | nindent 4 }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

This would render to...

annotations:
labels:
  # ...

If rbac.serviceAccount.annotations is falsy, and that would be invalid k8s YAML specification. This on the other hand would be okay.

Suggested change
annotations:
{{- with .Values.rbac.serviceAccount.annotations }}
{{- . | toYaml | nindent 4 }}
{{- end }}
{{- with .Values.rbac.serviceAccount.annotations }}
annotations:
{{- . | toYaml | nindent 4 }}
{{- end }}

Comment on lines 140 to 141
serviceAccount:
annotations: {}
Copy link
Member

Choose a reason for hiding this comment

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

There are more service accounts because we have more pods that all need som permissions against the k8s-apiserver, and I think we would need annotations specific to the service accounts.

So, something related to hub must be part of the config of this annotation, so its not applied to all serviceaccounts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will follow your suggestion. I added it to the rbac section because it was it changes files in the rbac yml file but I have nothing against moving it to the hub section

@consideRatio
Copy link
Member

I think I'd like our configuration to follow the configuration locations suggested here: https://helm.sh/docs/chart_best_practices/rbac/

So, hub.serviceAccount.annotations would be the configuration location of relevance then I figure.

@AndreaGiardini
Copy link
Contributor Author

AndreaGiardini commented Jun 2, 2021

@consideRatio Thank you for the feedback. I implemented the changes you requested. Can you have a second look please?

@AndreaGiardini
Copy link
Contributor Author

@consideRatio sorry for pinging again, I would love to see this in v1.0.0 :) let me know if there is something else missing

@consideRatio
Copy link
Member

consideRatio commented Jun 7, 2021

@AndreaGiardini thanks for your work, I'll help get it merged!

This is what remain in my mind and I figure I'll push a commit for this later today if you haven't already:

  • schema.yaml entry about this configuration
  • lint-and-validate-values.yaml entry for this configuration

@consideRatio consideRatio merged commit 1fc57db into jupyterhub:main Jun 7, 2021
@welcome
Copy link

welcome bot commented Jun 7, 2021

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@AndreaGiardini AndreaGiardini deleted the rbac_sa_annotations branch June 7, 2021 14:10
@AndreaGiardini
Copy link
Contributor Author

Many thanks @consideRatio !

@consideRatio consideRatio changed the title Add annotations to rbac serviceAccount Add ...serviceAccount.annotations config for our k8s ServiceAccounts Jun 7, 2021
@consideRatio
Copy link
Member

Thank you @AndreaGiardini!

consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants