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

Helm chart 1.0 #194

Merged
merged 15 commits into from
Aug 5, 2021
Merged
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/cr.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
release-name-template: "helm-chart-{{ .Name }}-{{ .Version }}"
27 changes: 27 additions & 0 deletions .github/workflows/helm-chart-release.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
name: Release Helm Charts

on:
push:
branches:
- master
paths:
- "charts/**"

jobs:
release:
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v2
with:
fetch-depth: 0
- name: Configure Git
run: |
git config user.name "$GITHUB_ACTOR"
git config user.email "$GITHUB_ACTOR@users.noreply.github.com"
- name: Run chart-releaser
uses: helm/chart-releaser-action@v1.2.1
env:
CR_TOKEN: "${{ secrets.GITHUB_TOKEN }}"
with:
config: .github/cr.yaml
15 changes: 1 addition & 14 deletions .github/workflows/release.yaml
Original file line number Diff line number Diff line change
@@ -3,17 +3,14 @@ on:
push:
# Sequence of patterns matched against refs/tags
tags:
- 'v*' # Push events to matching v*, i.e. v1.0, v20.15.10
- "v*" # Push events to matching v*, i.e. v1.0, v20.15.10
jobs:
build:
name: Release
runs-on: ubuntu-latest
steps:
- name: Checkout code
uses: actions/checkout@v1
- name: Create Helm chart
run: |
tar cvzf helm-chart.tgz helm
- name: Create Release
id: create-release
uses: actions/create-release@v1
@@ -28,13 +25,3 @@ jobs:
See [CHANGELOG](https://github.com/kubernetes-sigs/aws-fsx-csi-driver/blob/master/CHANGELOG-0.x.md) for full list of changes
draft: false
prerelease: false
- name: Upload Release Asset
id: upload-release-asset
uses: actions/upload-release-asset@v1
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
with:
upload_url: ${{ steps.create-release.outputs.upload_url }}
asset_path: ./helm-chart.tgz
asset_name: helm-chart.tgz
asset_content_type: application/gzip
18 changes: 17 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
@@ -14,13 +14,14 @@

PKG=github.com/kubernetes-sigs/aws-fsx-csi-driver
IMAGE?=amazon/aws-fsx-csi-driver
VERSION=v0.4.0-dirty
VERSION=v0.4.0
GIT_COMMIT?=$(shell git rev-parse HEAD)
BUILD_DATE?=$(shell date -u +"%Y-%m-%dT%H:%M:%SZ")
LDFLAGS?="-X ${PKG}/pkg/driver.driverVersion=${VERSION} -X ${PKG}/pkg/driver.gitCommit=${GIT_COMMIT} -X ${PKG}/pkg/driver.buildDate=${BUILD_DATE}"
GO111MODULE=on
GOPROXY=direct
GOPATH=$(shell go env GOPATH)
GOOS=$(shell go env GOOS)

.EXPORT_ALL_VARIABLES:

@@ -29,6 +30,14 @@ aws-fsx-csi-driver:
mkdir -p bin
CGO_ENABLED=0 GOOS=linux go build -ldflags ${LDFLAGS} -o bin/aws-fsx-csi-driver ./cmd/

bin /tmp/helm:
@mkdir -p $@

bin/helm: | /tmp/helm bin
@curl -o /tmp/helm/helm.tar.gz -sSL https://get.helm.sh/helm-v3.5.3-${GOOS}-amd64.tar.gz
@tar -zxf /tmp/helm/helm.tar.gz -C bin --strip-components=1
@rm -rf /tmp/helm/*

.PHONY: verify
verify:
./hack/verify-all
@@ -66,3 +75,10 @@ image-release:
.PHONY: push-release
push-release:
docker push $(IMAGE):$(VERSION)

generate-kustomize: bin/helm
wongma7 marked this conversation as resolved.
Show resolved Hide resolved
cd charts/aws-fsx-csi-driver && ../../bin/helm template kustomize . -s templates/csidriver.yaml > ../../deploy/kubernetes/base/csidriver.yaml
wongma7 marked this conversation as resolved.
Show resolved Hide resolved
cd charts/aws-fsx-csi-driver && ../../bin/helm template kustomize . -s templates/node-daemonset.yaml > ../../deploy/kubernetes/base/node-daemonset.yaml
cd charts/aws-fsx-csi-driver && ../../bin/helm template kustomize . -s templates/node-serviceaccount.yaml > ../../deploy/kubernetes/base/node-serviceaccount.yaml
cd charts/aws-fsx-csi-driver && ../../bin/helm template kustomize . -s templates/controller-deployment.yaml > ../../deploy/kubernetes/base/controller-deployment.yaml
cd charts/aws-fsx-csi-driver && ../../bin/helm template kustomize . -s templates/controller-serviceaccount.yaml > ../../deploy/kubernetes/base/controller-serviceaccount.yaml
13 changes: 13 additions & 0 deletions charts/aws-fsx-csi-driver/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# Helm chart

# v1.0.0
* Remove support for Helm 2
* Reorganize values to be more consistent with EFS and EBS helm charts
* controllerService -> controller
* nodeService -> node
* Add node.serviceAccount
* Add dnsPolicy and dnsConfig
* Add imagePullSecrets
* Remove extraArgs, securityContext, podSecurityContext
* Bump sidecar images to support kubernetes >=1.20
* Require kubernetes >=1.17
4 changes: 2 additions & 2 deletions charts/aws-fsx-csi-driver/Chart.yaml
Original file line number Diff line number Diff line change
@@ -2,8 +2,8 @@ apiVersion: v2
appVersion: "0.4.0"
name: aws-fsx-csi-driver
description: A Helm chart for AWS FSx for Lustre CSI Driver
version: 0.3.0
kubeVersion: ">=1.14.0-0"
version: 1.0.0
kubeVersion: ">=1.17.0-0"
home: https://github.com/kubernetes-sigs/aws-fsx-csi-driver
sources:
- https://github.com/kubernetes-sigs/aws-fsx-csi-driver
33 changes: 13 additions & 20 deletions charts/aws-fsx-csi-driver/templates/_helpers.tpl
Original file line number Diff line number Diff line change
@@ -2,7 +2,7 @@
{{/*
Expand the name of the chart.
*/}}
{{- define "helm.name" -}}
{{- define "aws-fsx-csi-driver.name" -}}
{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" -}}
{{- end -}}

@@ -11,7 +11,7 @@ Create a default fully qualified app name.
We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
If release name contains chart name it will be used as a full name.
*/}}
{{- define "helm.fullname" -}}
{{- define "aws-fsx-csi-driver.fullname" -}}
{{- if .Values.fullnameOverride -}}
{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" -}}
{{- else -}}
@@ -27,37 +27,30 @@ If release name contains chart name it will be used as a full name.
{{/*
Create chart name and version as used by the chart label.
*/}}
{{- define "helm.chart" -}}
{{- define "aws-fsx-csi-driver.chart" -}}
{{- printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimSuffix "-" -}}
{{- end -}}

{{/*
Common labels
*/}}
{{- define "helm.labels" -}}
helm.sh/chart: {{ include "helm.chart" . }}
{{ include "helm.selectorLabels" . }}
{{- define "aws-fsx-csi-driver.labels" -}}
{{ include "aws-fsx-csi-driver.selectorLabels" . }}
{{- if ne .Release.Name "kustomize" }}
helm.sh/chart: {{ include "aws-fsx-csi-driver.chart" . }}
{{- if .Chart.AppVersion }}
app.kubernetes.io/version: {{ .Chart.AppVersion | quote }}
{{- end }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
{{- end }}
{{- end -}}

{{/*
Selector labels
Common selector labels
*/}}
{{- define "helm.selectorLabels" -}}
app.kubernetes.io/name: {{ include "helm.name" . }}
{{- define "aws-fsx-csi-driver.selectorLabels" -}}
app.kubernetes.io/name: {{ include "aws-fsx-csi-driver.name" . }}
{{- if ne .Release.Name "kustomize" }}
app.kubernetes.io/instance: {{ .Release.Name }}
{{- end -}}

{{/*
Create the name of the service account to use
*/}}
{{- define "helm.serviceAccountName" -}}
{{- if .Values.serviceAccount.create -}}
{{ default (include "helm.fullname" .) .Values.serviceAccount.name }}
{{- else -}}
{{ default "default" .Values.serviceAccount.name }}
{{- end -}}
{{- end }}
{{- end -}}
105 changes: 105 additions & 0 deletions charts/aws-fsx-csi-driver/templates/controller-deployment.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
apiVersion: apps/v1
kind: Deployment
metadata:
name: fsx-csi-controller
labels:
{{- include "aws-fsx-csi-driver.labels" . | nindent 4 }}
spec:
replicas: {{ .Values.controller.replicaCount }}
selector:
matchLabels:
app: fsx-csi-controller

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 an else in the selectorLabels helper template.

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'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

Copy link
Contributor Author

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..

  1. I installed the current helm chart v0.1.0 using the README command
    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)
  2. I ran helm upgrade --install aws-fsx-csi-driver ./charts/aws-fsx-csi-driver
  3. helm recreated the deployment
k get deployment
NAME                 READY   UP-TO-DATE   AVAILABLE   AGE
fsx-csi-controller   1/1     1            1           57s

Copy link

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.

{{- include "aws-fsx-csi-driver.selectorLabels" . | nindent 6 }}
template:
metadata:
labels:
app: fsx-csi-controller
{{- include "aws-fsx-csi-driver.labels" . | nindent 8 }}
spec:
{{- if .Values.imagePullSecrets }}
imagePullSecrets:
{{- range .Values.imagePullSecrets }}
- name: {{ . }}
{{- end }}
{{- end }}
nodeSelector:
kubernetes.io/os: linux
{{- with .Values.controller.nodeSelector }}
{{- toYaml . | nindent 8 }}
{{- end }}
hostNetwork: true
serviceAccountName: {{ .Values.controller.serviceAccount.name }}
priorityClassName: system-cluster-critical
tolerations:
wongma7 marked this conversation as resolved.
Show resolved Hide resolved
- key: CriticalAddonsOnly
wongma7 marked this conversation as resolved.
Show resolved Hide resolved
operator: Exists
containers:
- name: fsx-plugin
image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}"
wongma7 marked this conversation as resolved.
Show resolved Hide resolved
imagePullPolicy: {{ .Values.image.pullPolicy }}
args:
- --endpoint=$(CSI_ENDPOINT)
env:
- name: CSI_ENDPOINT
value: unix:///var/lib/csi/sockets/pluginproxy/csi.sock
- name: AWS_ACCESS_KEY_ID
valueFrom:
secretKeyRef:
name: aws-secret
key: key_id
optional: true
- name: AWS_SECRET_ACCESS_KEY
valueFrom:
secretKeyRef:
name: aws-secret
key: access_key
optional: true
volumeMounts:
- name: socket-dir
mountPath: /var/lib/csi/sockets/pluginproxy/
ports:
- name: healthz
containerPort: 9910
protocol: TCP
livenessProbe:
httpGet:
path: /healthz
port: healthz
initialDelaySeconds: 10
timeoutSeconds: 3
periodSeconds: 2
failureThreshold: 5
{{- with .Values.controller.resources }}
resources:
{{- toYaml . | nindent 12 }}
{{- end }}
- name: csi-provisioner
image: {{ printf "%s:%s" .Values.sidecars.provisioner.image.repository .Values.sidecars.provisioner.image.tag }}
args:
- --csi-address=$(ADDRESS)
- --timeout=5m
env:
- name: ADDRESS
value: /var/lib/csi/sockets/pluginproxy/csi.sock
volumeMounts:
- name: socket-dir
mountPath: /var/lib/csi/sockets/pluginproxy/
{{- with default .Values.controller.resources .Values.sidecars.provisioner.resources }}
resources:
{{- toYaml . | nindent 12 }}
{{- end }}
- name: liveness-probe
image: {{ printf "%s:%s" .Values.sidecars.livenessProbe.image.repository .Values.sidecars.livenessProbe.image.tag }}
args:
- --csi-address=/csi/csi.sock
- --health-port=9910
volumeMounts:
- name: socket-dir
mountPath: /csi
{{- with default .Values.controller.resources .Values.sidecars.livenessProbe.resources }}
resources:
{{- toYaml . | nindent 12 }}
{{- end }}
volumes:
- name: socket-dir
emptyDir: {}
Original file line number Diff line number Diff line change
@@ -1,22 +1,23 @@
{{- if .Values.serviceAccount.create -}}
{{- if .Values.controller.serviceAccount.create }}
apiVersion: v1
kind: ServiceAccount
metadata:
name: {{ include "helm.serviceAccountName" . }}
name: {{ .Values.controller.serviceAccount.name }}
labels:
{{- include "helm.labels" . | nindent 4 }}
{{- with .Values.serviceAccount.annotations }}
{{- include "aws-fsx-csi-driver.labels" . | nindent 4 }}
{{- with .Values.controller.serviceAccount.annotations }}
annotations:
{{- toYaml . | nindent 4 }}
{{- end }}
{{- end }}

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.

Copy link
Contributor Author

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

Copy link

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

---

kind: ClusterRole
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: fsx-csi-external-provisioner-role
labels:
{{- include "helm.labels" . | nindent 4 }}
{{- include "aws-fsx-csi-driver.labels" . | nindent 4 }}
rules:
- apiGroups: [""]
resources: ["persistentvolumes"]
@@ -46,13 +47,12 @@ apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: fsx-csi-external-provisioner-binding
labels:
{{- include "helm.labels" . | nindent 4 }}
{{- include "aws-fsx-csi-driver.labels" . | nindent 4 }}
subjects:
- kind: ServiceAccount
name: {{ include "helm.serviceAccountName" . }}
name: {{ .Values.controller.serviceAccount.name }}
namespace: {{ .Release.Namespace }}
roleRef:
kind: ClusterRole
name: fsx-csi-external-provisioner-role
apiGroup: rbac.authorization.k8s.io
{{- end -}}
Loading