Merge branch 'master' into master

CHANGELOG.md

@@ -2,14 +2,15 @@
 
 IMPROVEMENTS:
 
-  * RBAC support for `syncCatalog`. This is enabled by default if the catalog
-    sync is enabled but can be controlled with `syncCatalog.rbac.enabled`.
-    This will create the `ClusterRole` and `ClusterRoleBinding` necessary
-    for the catalog sync. [GH-29]
+  * RBAC support for `syncCatalog`. This will create the `ClusterRole`, `ClusterRoleBinding`
+    and `ServiceAccount` that is necessary for the catalog sync. [GH-29]
   * client: agents now have the node name set to the actual K8S node name [GH-14]
   * RBAC support for `connectInject`. This will create a `ClusterRole`, `ClusterRoleBinding`,
     and `ServiceAccount` that is necessary for the connect injector to automatically generate
     TLS certificates to interact with the Kubernetes API.
+  * Server affinity is now configurable. This makes it easier to run an entire
+    Consul cluster on Minikube. [GH-13]
+  * Liveness probes are now http calls, reducing errors in the logs.
 
 BUG FIXES:
 
@@ -18,6 +19,8 @@ BUG FIXES:
   * Add missing continuation characters to long commands [GH-26].
   * connectInject: set the correct namespace for the MutatingWebhookConfiguration
     so that deployments work in non-default namespaces. [GH-41]
+  * Provide a valid `maxUnavailable` value when replicas=1. [GH-58]
+  * Correctly sets server resource requirements.
 
 ## 0.3.0 (October 11, 2018)
 

README.md

@@ -66,3 +66,114 @@ that can be used to quickly bring up a GKE cluster and configure
 `kubectl` and `helm` locally. This can be used to quickly spin up a test
 cluster for acceptance tests. Unit tests _do not_ require a running Kubernetes
 cluster.
+
+### Writing Unit Tests
+
+Changes to the Helm chart should be accompanied by appropriate unit tests.
+
+#### Formatting
+
+- Put tests in the test file in the same order as the variables appear in the `values.yaml`. 
+- Start tests for a chart value with a header that says what is being tested, like this:
+    ```
+    #--------------------------------------------------------------------
+    # 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.service.type=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 "syncCatalog/Deployment: to-k8s only" {
+      cd `chart_dir`
+      local actual=$(helm template \
+          -x templates/sync-catalog-deployment.yaml  \
+          --set 'syncCatalog.enabled=true' \
+          --set 'syncCatalog.toConsul=false' \
+          . | tee /dev/stderr |
+          yq '.spec.template.spec.containers[0].command | any(contains("-to-consul=false"))' | tee /dev/stderr)
+      [ "${actual}" = "true" ]
+
+      local actual=$(helm template \
+          -x templates/sync-catalog-deployment.yaml  \
+          --set 'syncCatalog.enabled=true' \
+          --set 'syncCatalog.toConsul=false' \
+          . | tee /dev/stderr |
+          yq '.spec.template.spec.containers[0].command | any(contains("-to-k8s"))' | tee /dev/stderr)
+      [ "${actual}" = "false" ]
+    }
+    ```
+    In this case, the same command is run twice in the same test.
+    This can be used to look for several things in the same field, or to check that something is not present that shouldn't be.
+
+    *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`
+      local actual=$(helm template \
+          -x templates/sync-catalog-deployment.yaml  \
+          . | tee /dev/stderr |
+          yq 'length > 0' | tee /dev/stderr)
+      [ "${actual}" = "false" ]
+    }
+    ```
+    Here we are check the length of the command output to see if the anything is rendered.
+    This style can easily be switched to check that a file is rendered instead.

templates/_helpers.tpl

@@ -36,9 +36,12 @@ Compute the maximum number of unavailable replicas for the PodDisruptionBudget.
 This defaults to (n/2)-1 where n is the number of members of the server cluster.
 Special case of replica equaling 3 and allowing a minor disruption of 1 otherwise
 use the integer value
+Add a special case for replicas=1, where it should default to 0 as well.
 */}}
 {{- define "consul.pdb.maxUnavailable" -}}
-{{- if .Values.server.disruptionBudget.maxUnavailable -}}
+{{- if eq (int .Values.server.replicas) 1 -}}
+{{ 0 }}
+{{- else if .Values.server.disruptionBudget.maxUnavailable -}}
 {{ .Values.server.disruptionBudget.maxUnavailable -}}
 {{- else -}}
 {{- if eq (int .Values.server.replicas) 3 -}}

templates/client-daemonset.yaml

@@ -89,7 +89,7 @@ spec:
                 -data-dir=/consul/data \
                 {{- if (.Values.client.join) and (gt (len .Values.client.join) 0) }}
                 {{- range $value := .Values.client.join }}
-                -retry-join={{ $value }} \
+                -retry-join="{{ $value }}" \
                 {{- end }}
                 {{- else }}
                 {{- if .Values.server.enabled }}
@@ -145,6 +145,8 @@ spec:
                 - |
                   curl http://127.0.0.1:8500/v1/status/leader 2>/dev/null | \
                   grep -E '".+"'
+          {{- if .Values.client.resources }}
           resources:
-{{ toYaml .Values.client.resources | indent 12 }}
+            {{ tpl .Values.client.resources . | nindent 12 | trim }}
+          {{- end }}
 {{- end }}

templates/connect-inject-deployment.yaml

@@ -57,8 +57,10 @@ spec:
                 -tls-auto-hosts=${CONSUL_FULLNAME}-connect-injector-svc,${CONSUL_FULLNAME}-connect-injector-svc.${NAMESPACE},${CONSUL_FULLNAME}-connect-injector-svc.${NAMESPACE}.svc
 {{- end }}
           livenessProbe:
-            tcpSocket:
+            httpGet:
+              path: /health/ready
               port: 8080
+              scheme: HTTPS
             failureThreshold: 2
             initialDelaySeconds: 1
             periodSeconds: 2

templates/server-statefulset.yaml

@@ -146,6 +146,10 @@ spec:
             periodSeconds: 3
             successThreshold: 1
             timeoutSeconds: 5
+          {{- if .Values.server.resources }}
+          resources:
+            {{ tpl .Values.server.resources . | nindent 12 | trim }}
+          {{- end }}
   volumeClaimTemplates:
     - metadata:
         name: data

templates/sync-catalog-cluster-role-binding.yaml

@@ -1,6 +1,5 @@
-{{- $rbacEnabled := (or (and (ne (.Values.syncCatalog.rbac.enabled | toString) "-") .Values.syncCatalog.rbac.enabled) (and (eq (.Values.syncCatalog.rbac.enabled | toString) "-") .Values.global.enabled)) }}
 {{- $syncEnabled := (or (and (ne (.Values.syncCatalog.enabled | toString) "-") .Values.syncCatalog.enabled) (and (eq (.Values.syncCatalog.enabled | toString) "-") .Values.global.enabled)) }}
-{{- if (and $rbacEnabled $syncEnabled) }}
+{{- if $syncEnabled }}
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRoleBinding
 metadata:

templates/sync-catalog-cluster-role.yaml

@@ -1,6 +1,5 @@
-{{- $rbacEnabled := (or (and (ne (.Values.syncCatalog.rbac.enabled | toString) "-") .Values.syncCatalog.rbac.enabled) (and (eq (.Values.syncCatalog.rbac.enabled | toString) "-") .Values.global.enabled)) }}
 {{- $syncEnabled := (or (and (ne (.Values.syncCatalog.enabled | toString) "-") .Values.syncCatalog.enabled) (and (eq (.Values.syncCatalog.enabled | toString) "-") .Values.global.enabled)) }}
-{{- if (and $rbacEnabled $syncEnabled) }}
+{{- if $syncEnabled }}
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole
 metadata:

templates/sync-catalog-deployment.yaml

@@ -1,5 +1,4 @@
-# The deployment for running the Connect sidecar injector
-{{- $rbacEnabled := (or (and (ne (.Values.syncCatalog.rbac.enabled | toString) "-") .Values.syncCatalog.rbac.enabled) (and (eq (.Values.syncCatalog.rbac.enabled | toString) "-") .Values.global.enabled)) }}
+# The deployment for running the sync-catalog pod
 {{- if (or (and (ne (.Values.syncCatalog.enabled | toString) "-") .Values.syncCatalog.enabled) (and (eq (.Values.syncCatalog.enabled | toString) "-") .Values.global.enabled)) }}
 apiVersion: apps/v1
 kind: Deployment
@@ -28,9 +27,7 @@ spec:
       annotations:
         "consul.hashicorp.com/connect-inject": "false"
     spec:
-      {{- if $rbacEnabled }}
       serviceAccountName: {{ template "consul.fullname" . }}-sync-catalog
-      {{- end }}
       containers:
         - name: consul-sync-catalog
           image: "{{ default .Values.global.imageK8S .Values.syncCatalog.image }}"

templates/sync-catalog-service-account.yaml

@@ -1,6 +1,5 @@
-{{- $rbacEnabled := (or (and (ne (.Values.syncCatalog.rbac.enabled | toString) "-") .Values.syncCatalog.rbac.enabled) (and (eq (.Values.syncCatalog.rbac.enabled | toString) "-") .Values.global.enabled)) }}
 {{- $syncEnabled := (or (and (ne (.Values.syncCatalog.enabled | toString) "-") .Values.syncCatalog.enabled) (and (eq (.Values.syncCatalog.enabled | toString) "-") .Values.global.enabled)) }}
-{{- if (and $rbacEnabled $syncEnabled) }}
+{{- if $syncEnabled }}
 apiVersion: v1
 kind: ServiceAccount
 metadata:

test/unit/client-daemonset.yaml → test/unit/client-daemonset.bats

@@ -94,6 +94,28 @@ load _helpers
   [ "${actual}" = "true" ]
 }
 
+#--------------------------------------------------------------------
+# resources
+
+@test "client/DaemonSet: no resources defined by default" {
+  cd `chart_dir`
+  local actual=$(helm template \
+      -x templates/client-daemonset.yaml  \
+      . | tee /dev/stderr |
+      yq -r '.spec.template.spec.containers[0].resources' | tee /dev/stderr)
+  [ "${actual}" = "null" ]
+}
+
+@test "client/DaemonSet: resources can be set" {
+  cd `chart_dir`
+  local actual=$(helm template \
+      -x templates/client-daemonset.yaml  \
+      --set 'client.resources=foo' \
+      . | tee /dev/stderr |
+      yq -r '.spec.template.spec.containers[0].resources' | tee /dev/stderr)
+  [ "${actual}" = "foo" ]
+}
+
 #--------------------------------------------------------------------
 # extraVolumes
 

test/unit/server-disruptionbudget.bats

@@ -11,7 +11,7 @@ load _helpers
   [ "${actual}" = "true" ]
 }
 
-@test "server/DisruptionBudget: enable with global.enabled false" {
+@test "server/DisruptionBudget: enabled with global.enabled=false" {
   cd `chart_dir`
   local actual=$(helm template \
       -x templates/server-disruptionbudget.yaml  \
@@ -22,7 +22,7 @@ load _helpers
   [ "${actual}" = "true" ]
 }
 
-@test "server/DisruptionBudget: disable with server.enabled" {
+@test "server/DisruptionBudget: disabled with server.enabled=false" {
   cd `chart_dir`
   local actual=$(helm template \
       -x templates/server-disruptionbudget.yaml  \
@@ -32,7 +32,7 @@ load _helpers
   [ "${actual}" = "false" ]
 }
 
-@test "server/DisruptionBudget: disable with server.disruptionBudget.enabled" {
+@test "server/DisruptionBudget: disabled with server.disruptionBudget.enabled=false" {
   cd `chart_dir`
   local actual=$(helm template \
       -x templates/server-disruptionbudget.yaml  \
@@ -42,7 +42,7 @@ load _helpers
   [ "${actual}" = "false" ]
 }
 
-@test "server/DisruptionBudget: disable with global.enabled" {
+@test "server/DisruptionBudget: disabled with global.enabled=false" {
   cd `chart_dir`
   local actual=$(helm template \
       -x templates/server-disruptionbudget.yaml  \
@@ -52,7 +52,20 @@ load _helpers
   [ "${actual}" = "false" ]
 }
 
-@test "server/DisruptionBudget: correct maxUnavailable with n=3" {
+#--------------------------------------------------------------------
+# maxUnavailable
+
+@test "server/DisruptionBudget: correct maxUnavailable with replicas=1" {
+  cd `chart_dir`
+  local actual=$(helm template \
+      -x templates/server-disruptionbudget.yaml  \
+      --set 'server.replicas=1' \
+      . | tee /dev/stderr |
+      yq '.spec.maxUnavailable' | tee /dev/stderr)
+  [ "${actual}" = "0" ]
+}
+
+@test "server/DisruptionBudget: correct maxUnavailable with replicas=3" {
   cd `chart_dir`
   local actual=$(helm template \
       -x templates/server-disruptionbudget.yaml  \
@@ -111,4 +124,4 @@ load _helpers
       . | tee /dev/stderr |
       yq '.spec.maxUnavailable' | tee /dev/stderr)
   [ "${actual}" = "3" ]
-}
+}