Update chart and tests to Helm 3 #146
Conversation
|
|
There was a problem hiding this comment.
Thanks for the patch! And apologies for the delay. This is a good bit of work, though there are some changes required before we can merge this, mostly having to do with the tests. Let me know if you’re interested in working on this further, otherwise I can take it over and push it through.
Besides the comments below, it looks like the injector unit tests are failing and need to be updated for helm 3 (see https://github.com/hashicorp/vault-helm/blob/master/CONTRIBUTING.md#testing).
Several of the server unit tests seem to be failing on this branch, so in general I'd suggest staying with the existing test style (more along the lines of PR #129). See my comment here as an example: https://github.com/hashicorp/vault-helm/pull/146/files#diff-fb42a918692d6e6de881b59d1c280a10R31
| @@ -1,4 +1,4 @@ | |||
| apiVersion: v1 | |||
| apiVersion: v2 | |||
There was a problem hiding this comment.
Please leave this as v1 for now.
| apiVersion: v2 | |
| apiVersion: v1 |
| # annotations | ||
| ``` | ||
|
|
||
| - Name the test based on what it's testing in the following format (this will be its first line): | ||
| ``` | ||
| @test "<section being tested>: <short description of the test case>" { | ||
| ``` | ||
|
|
||
| When adding tests to an existing file, the first section will be the same as the other tests in the file. | ||
|
|
||
| #### Test Details | ||
|
|
||
| [Bats](https://github.com/bats-core/bats-core) provides a way to run commands in a shell and inspect the output in an automated way. | ||
| In all of the tests in this repo, the base command being run is [helm template](https://docs.helm.sh/helm/#helm-template) which turns the templated files into straight yaml output. | ||
| In this way, we're able to test that the various conditionals in the templates render as we would expect. | ||
|
|
||
| Each test defines the files that should be rendered using the `-x` flag, then it might adjust chart values by adding `--set` flags as well. | ||
| The output from this `helm template` command is then piped to [yq](https://pypi.org/project/yq/). | ||
| `yq` allows us to pull out just the information we're interested in, either by referencing its position in the yaml file directly or giving information about it (like its length). | ||
| The `-r` flag can be used with `yq` to return a raw string instead of a quoted one which is especially useful when looking for an exact match. | ||
|
|
||
| The test passes or fails based on the conditional at the end that is in square brackets, which is a comparison of our expected value and the output of `helm template` piped to `yq`. | ||
|
|
||
| The `| tee /dev/stderr ` pieces direct any terminal output of the `helm template` and `yq` commands to stderr so that it doesn't interfere with `bats`. | ||
|
|
||
| #### Test Examples | ||
|
|
||
| Here are some examples of common test patterns: | ||
|
|
||
| - Check that a value is disabled by default | ||
|
|
||
| ``` | ||
| @test "ui/Service: no type by default" { | ||
| cd `chart_dir` | ||
| local actual=$(helm template \ | ||
| -x templates/ui-service.yaml \ | ||
| . | tee /dev/stderr | | ||
| yq -r '.spec.type' | tee /dev/stderr) | ||
| [ "${actual}" = "null" ] | ||
| } | ||
| ``` | ||
|
|
||
| In this example, nothing is changed from the default templates (no `--set` flags), then we use `yq` to retrieve the value we're checking, `.spec.type`. | ||
| This output is then compared against our expected value (`null` in this case) in the assertion `[ "${actual}" = "null" ]`. | ||
|
|
||
|
|
||
| - Check that a template value is rendered to a specific value | ||
| ``` | ||
| @test "ui/Service: specified type" { | ||
| cd `chart_dir` | ||
| local actual=$(helm template \ | ||
| -x templates/ui-service.yaml \ | ||
| --set 'ui.serviceType=LoadBalancer' \ | ||
| . | tee /dev/stderr | | ||
| yq -r '.spec.type' | tee /dev/stderr) | ||
| [ "${actual}" = "LoadBalancer" ] | ||
| } | ||
| ``` | ||
|
|
||
| This is very similar to the last example, except we've changed a default value with the `--set` flag and correspondingly changed the expected value. | ||
|
|
||
| - Check that a template value contains several values | ||
| ``` | ||
| @test "server/standalone-StatefulSet: custom resources" { | ||
| cd `chart_dir` | ||
| local actual=$(helm template \ | ||
| -x templates/server-statefulset.yaml \ | ||
| --set 'server.standalone.enabled=true' \ | ||
| --set 'server.resources.requests.memory=256Mi' \ | ||
| --set 'server.resources.requests.cpu=250m' \ | ||
| . | tee /dev/stderr | | ||
| yq -r '.spec.template.spec.containers[0].resources.requests.memory' | tee /dev/stderr) | ||
| [ "${actual}" = "256Mi" ] | ||
|
|
||
| local actual=$(helm template \ | ||
| -x templates/server-statefulset.yaml \ | ||
| --set 'server.standalone.enabled=true' \ | ||
| --set 'server.resources.limits.memory=256Mi' \ | ||
| --set 'server.resources.limits.cpu=250m' \ | ||
| . | tee /dev/stderr | | ||
| yq -r '.spec.template.spec.containers[0].resources.limits.memory' | tee /dev/stderr) | ||
| [ "${actual}" = "256Mi" ] | ||
| ``` | ||
|
|
||
| *Note:* If testing more than two conditions, it would be good to separate the `helm template` part of the command from the `yq` sections to reduce redundant work. | ||
|
|
||
| - Check that an entire template file is not rendered | ||
| ``` | ||
| @test "syncCatalog/Deployment: disabled by default" { | ||
| cd `chart_dir` | ||
| run helm template \ | ||
| -x templates/server-statefulset.yaml \ | ||
| --set 'global.enabled=false' \ | ||
| . | ||
| [ "$status" -eq 1 ] | ||
| } | ||
| ``` | ||
| Here we are checking to ensure that the template isn't rendered by verifying that `helm template` exits with an error code. | ||
|
|
||
| *Note:* This test will not work correctly if you put any comments before the conditional statement. |
There was a problem hiding this comment.
It looks like this info (from Testing on down) was moved into the CONTRIBUTING.md doc recently, so I think these additions are no longer necessary, please remove.
| {{ template "vault.mode" . }} | ||
| {{- if and (and (eq (.Values.global.enabled | toString) "true") (eq .mode "ha")) (eq (.Values.server.ha.disruptionBudget.enabled | toString) "true") -}} | ||
| # PodDisruptionBudget to prevent degrading the server cluster through | ||
| # voluntary cluster changes. |
There was a problem hiding this comment.
Thanks for cleaning up the generated yaml templates!
| @@ -37,7 +37,7 @@ RUN curl -LO https://storage.googleapis.com/kubernetes-release/release/$(curl -s | |||
| mv ./kubectl /usr/local/bin/kubectl | |||
|
|
|||
| # helm | |||
| RUN curl https://raw.githubusercontent.com/kubernetes/helm/master/scripts/get | bash | |||
| RUN curl https://raw.githubusercontent.com/kubernetes/helm/master/scripts/get-helm-3 | bash | |||
There was a problem hiding this comment.
Looks like this lives under the helm org now, per https://helm.sh/docs/intro/install/#from-script:
| RUN curl https://raw.githubusercontent.com/kubernetes/helm/master/scripts/get-helm-3 | bash | |
| RUN curl https://raw.githubusercontent.com/helm/helm/master/scripts/get-helm-3 | bash |
| @test "server/ClusterRoleBinding: enabled by default" { | ||
| cd `chart_dir` | ||
| local actual=$(helm template \ | ||
| -x templates/server-clusterrolebinding.yaml \ | ||
| run helm template \ | ||
| --show-only templates/server-clusterrolebinding.yaml \ | ||
| --set 'server.dev.enabled=true' \ | ||
| . | tee /dev/stderr | | ||
| yq 'length > 0' | tee /dev/stderr) | ||
| [ "${actual}" = "true" ] | ||
| . | ||
| [ "$status" -eq 1 ] | ||
|
|
||
| local actual=$(helm template \ | ||
| -x templates/server-clusterrolebinding.yaml \ | ||
| run helm template \ | ||
| --show-only templates/server-clusterrolebinding.yaml \ | ||
| --set 'server.ha.enabled=true' \ | ||
| . | tee /dev/stderr | | ||
| yq 'length > 0' | tee /dev/stderr) | ||
| [ "${actual}" = "true" ] | ||
| . | ||
| [ "$status" -eq 1 ] | ||
|
|
||
| local actual=$(helm template \ | ||
| -x templates/server-clusterrolebinding.yaml \ | ||
| . | tee /dev/stderr | | ||
| yq 'length > 0' | tee /dev/stderr) | ||
| [ "${actual}" = "true" ] | ||
| run helm template \ | ||
| --show-only templates/server-clusterrolebinding.yaml \ | ||
| . | ||
| [ "$status" -eq 1 ] | ||
| } |
There was a problem hiding this comment.
I don't think any of the helm template calls in server/ClusterRoleBinding: enabled by default should be exiting with code 1, which would indicate an error. And since they should be checking that yaml was produced in the output, I think the yq checks are worth leaving in. So I'd suggest that the only change here be swapping -x for --show-only, something like:
@test "server/ClusterRoleBinding: enabled by default" {
cd `chart_dir`
local actual=$(helm template \
--show-only templates/server-clusterrolebinding.yaml \
--set 'server.dev.enabled=true' \
. | tee /dev/stderr |
yq 'length > 0' | tee /dev/stderr)
[ "${actual}" = "true" ]
local actual=$(helm template \
--show-only templates/server-clusterrolebinding.yaml \
--set 'server.ha.enabled=true' \
. | tee /dev/stderr |
yq 'length > 0' | tee /dev/stderr)
[ "${actual}" = "true" ]
local actual=$(helm template \
--show-only templates/server-clusterrolebinding.yaml \
. | tee /dev/stderr |
yq 'length > 0' | tee /dev/stderr)
[ "${actual}" = "true" ]
}| --set 'server.authDelegator.enabled=true' \ | ||
| --show-only templates/server-clusterrolebinding.yaml \ |
There was a problem hiding this comment.
Should these two lines be removed? They look redundant.
| run helm template \ | ||
| --show-only templates/server-clusterrolebinding.yaml \ | ||
| --set 'global.enabled=false' \ | ||
| . | tee /dev/stderr | | ||
| yq 'length > 0' | tee /dev/stderr) | ||
| [ "${actual}" = "false" ] | ||
| . | ||
| [ "$status" -eq 1 ] |
There was a problem hiding this comment.
This is an interesting idea, since the clusterrolebinding template will be empty, though I would suggest keeping the existing style for consistency between tests. Something like this?
local actual=$( (helm template \
--show-only templates/server-clusterrolebinding.yaml \
--set 'global.enabled=false' \
. || echo "---") | tee /dev/stderr |
yq 'length > 0' | tee /dev/stderr)
[ "${actual}" = "false" ]
|
Thanks again for the contribution! We've decided to continue this in another PR. Superseded by #195 |
I have updated the chart for Helm v3 along with all unit and acceptance tests. This is a breaking change in that Helm v3 is required to install the chart, and any existing deployments would need to be migrated from v2 to v3 via instructions here: https://helm.sh/docs/topics/v2_v3_migration/. I opted to make a clean switch instead of trying to support/verify both Helm v2 and v3. I could probably rework it to support both versions, but it would require some decisions on if separate unit/acceptance tests will be maintained for both. Let me know your thoughts and if this is something you want to move forward with.