Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Helm chart 1.0 #194
Helm chart 1.0 #194
Changes from 6 commits
ead1645
745ca5a
c99a8cc
a4674b2
9f76863
8355ae3
de19088
e63dda3
69de92c
f9c1b50
1702134
3fd96fc
0200261
a6e7303
34ec8f5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Adding this will cause upgrade issues since
matchLabels
is immutable. It looks like maybe it is being added to ensure that is continues to be included on the kustomize manifests. If that is the case, I would move if from here to anelse
in the selectorLabels helper template.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'll test out the upgrade case, since I am bumping major version of helm chart I am not sure without testing how helm deals with it. I just cargo culted how ebs does it TBH: https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/master/charts/aws-ebs-csi-driver/templates/controller.yaml#L12
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.
this change is safe because helm somehow knows to delete and recreate the deployment which also deletes and recreates the pods..
a. (I manually edited the deployment and daemonset to have hostNetwork and to point to image 0.4.0 instead of 0.3.0 because they were crashlooping as I have instance metadata disabled)
helm upgrade --install aws-fsx-csi-driver ./charts/aws-fsx-csi-driver
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 didn't notice that the name of the deployment was changing. That explains the delete and create behavior.
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.
Moving this end here will create the ClusterRole and ClusterRoleBinding regardless of whether the service account is created. I am not 100% sure what will happen with the ClusterRoleBinding since the service account it is referencing won't exist if
.Values.controller.serviceAccount.create
is false. Seems like this end should move back to the end of the template.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.
It's intentional to match how ebs does it https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/master/charts/aws-ebs-csi-driver/templates/serviceaccount-csi-controller.yaml (ebs splits up the serviceaccount from the associated RBAC bindings whereas here it's same fille...I kind of prefer keeping serviceaccount and associated RBAC in same file, but for ebs there are a LOT of RBAC things). The reasoning is that even if .Values.controller.serviceAccount.create is false, .Values.controller.serviceAccount.name is respected. An example use-case is on EKS if I take advantage of the "IAM for service accounts" feature then I will probably want to create the serviceAccount myself because I need it to have a special annotation. But I still need the ClusterRole and ClusterRoleBinding to exist for my serviceAccount whether it is created by me or by helm
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.
Personally, I would just add the annotation to
controller.serviceAccount.annotations
, which is what I did for the EBS chart. I do, however, see what you are saying about some users potentially wanting to use a pre-existing service account.If this chart isn't creating a service account and therefore expects that one already exists it might be worth using the lookup function to validate that it does exist and fail if it doesn't.
https://helm.sh/docs/chart_template_guide/functions_and_pipelines/#using-the-lookup-function