Merge pull request hashicorp#282 from hashicorp/inject-grpc

Prevent incorrect connectInject config
ndhanushkodi · Nov 27, 2019 · 9942f19 · 9942f19
2 parents bb4bfa3 + f92eac1
commit 9942f19
Show file tree

Hide file tree

Showing 13 changed files with 74 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,12 @@
+## Unreleased
+
+BREAKING CHANGES
+  * `client.grpc` defaults to `true` now instead of `false`. This is to make it
+     harder to misconfigure Connect [[GH-282](https://github.com/hashicorp/consul-helm/pull/282)]
+
+     If you do not wish to enable gRPC for clients, set `client.grpc` to
+     `false` in your local values file.
+
 ## 0.12.0 (Oct 28, 2019)
 
 IMPROVEMENTS:

diff --git a/templates/connect-inject-deployment.yaml b/templates/connect-inject-deployment.yaml
@@ -1,5 +1,7 @@
 # The deployment for running the Connect sidecar injector
 {{- if (or (and (ne (.Values.connectInject.enabled | toString) "-") .Values.connectInject.enabled) (and (eq (.Values.connectInject.enabled | toString) "-") .Values.global.enabled)) }}
+{{- if not (or (and (ne (.Values.client.enabled | toString) "-") .Values.client.enabled) (and (eq (.Values.client.enabled | toString) "-") .Values.global.enabled)) }}{{ fail "clients must be enabled for connect injection" }}{{ end }}
+{{- if not .Values.client.grpc }}{{ fail "client.grpc must be true for connect injection" }}{{ end }}
 apiVersion: apps/v1
 kind: Deployment
 metadata:

diff --git a/test/unit/client-daemonset.bats b/test/unit/client-daemonset.bats
@@ -90,23 +90,23 @@ load _helpers
 #--------------------------------------------------------------------
 # grpc
 
-@test "client/DaemonSet: grpc is disabled by default" {
+@test "client/DaemonSet: grpc is enabled by default" {
   cd `chart_dir`
   local actual=$(helm template \
       -x templates/client-daemonset.yaml  \
       . | tee /dev/stderr |
       yq '.spec.template.spec.containers[0].command | any(contains("grpc"))' | tee /dev/stderr)
-  [ "${actual}" = "false" ]
+  [ "${actual}" = "true" ]
 }
 
-@test "client/DaemonSet: grpc can be enabled" {
+@test "client/DaemonSet: grpc can be disabled" {
   cd `chart_dir`
   local actual=$(helm template \
       -x templates/client-daemonset.yaml  \
-      --set 'client.grpc=true' \
+      --set 'client.grpc=false' \
       . | tee /dev/stderr |
       yq '.spec.template.spec.containers[0].command | any(contains("grpc"))' | tee /dev/stderr)
-  [ "${actual}" = "true" ]
+  [ "${actual}" = "false" ]
 }
 
 #--------------------------------------------------------------------

diff --git a/test/unit/connect-inject-authmethod-clusterrole.bats b/test/unit/connect-inject-authmethod-clusterrole.bats
@@ -16,6 +16,7 @@ load _helpers
   local actual=$(helm template \
       -x templates/connect-inject-authmethod-clusterrole.yaml  \
       --set 'global.enabled=false' \
+      --set 'client.enabled=true' \
       --set 'connectInject.enabled=true' \
       --set 'global.bootstrapACLs=true' \
       . | tee /dev/stderr |

diff --git a/test/unit/connect-inject-authmethod-clusterrolebinding.bats b/test/unit/connect-inject-authmethod-clusterrolebinding.bats
@@ -16,6 +16,7 @@ load _helpers
   local actual=$(helm template \
       -x templates/connect-inject-authmethod-clusterrolebinding.yaml  \
       --set 'global.enabled=false' \
+      --set 'client.enabled=true' \
       --set 'connectInject.enabled=true' \
       --set 'global.bootstrapACLs=true' \
       . | tee /dev/stderr |

diff --git a/test/unit/connect-inject-authmethod-serviceaccount.bats b/test/unit/connect-inject-authmethod-serviceaccount.bats
@@ -16,6 +16,7 @@ load _helpers
   local actual=$(helm template \
       -x templates/connect-inject-authmethod-serviceaccount.yaml  \
       --set 'global.enabled=false' \
+      --set 'client.enabled=true' \
       --set 'connectInject.enabled=true' \
       --set 'global.bootstrapACLs=true' \
       . | tee /dev/stderr |

diff --git a/test/unit/connect-inject-clusterrole.bats b/test/unit/connect-inject-clusterrole.bats
@@ -16,6 +16,7 @@ load _helpers
   local actual=$(helm template \
       -x templates/connect-inject-clusterrole.yaml  \
       --set 'global.enabled=false' \
+      --set 'client.enabled=true' \
       --set 'connectInject.enabled=true' \
       . | tee /dev/stderr |
       yq -s 'length > 0' | tee /dev/stderr)

diff --git a/test/unit/connect-inject-clusterrolebinding.bats b/test/unit/connect-inject-clusterrolebinding.bats
@@ -16,6 +16,7 @@ load _helpers
   local actual=$(helm template \
       -x templates/connect-inject-clusterrolebinding.yaml  \
       --set 'global.enabled=false' \
+      --set 'client.enabled=true' \
       --set 'connectInject.enabled=true' \
       . | tee /dev/stderr |
       yq -s 'length > 0' | tee /dev/stderr)

diff --git a/test/unit/connect-inject-deployment.bats b/test/unit/connect-inject-deployment.bats
@@ -11,11 +11,12 @@ load _helpers
   [ "${actual}" = "false" ]
 }
 
-@test "connectInject/Deployment: enable with global.enabled false" {
+@test "connectInject/Deployment: enable with global.enabled false, client.enabled true" {
   cd `chart_dir`
   local actual=$(helm template \
       -x templates/connect-inject-deployment.yaml  \
       --set 'global.enabled=false' \
+      --set 'client.enabled=true' \
       --set 'connectInject.enabled=true' \
       . | tee /dev/stderr |
       yq 'length > 0' | tee /dev/stderr)
@@ -42,6 +43,48 @@ load _helpers
   [ "${actual}" = "false" ]
 }
 
+@test "connectInject/Deployment: fails if global.enabled=false" {
+  cd `chart_dir`
+  run helm template \
+      -x templates/connect-inject-deployment.yaml  \
+      --set 'global.enabled=false' \
+      --set 'connectInject.enabled=true' .
+  [ "$status" -eq 1 ]
+  [[ "$output" =~ "clients must be enabled for connect injection" ]]
+}
+
+@test "connectInject/Deployment: fails if global.enabled=true and client.enabled=false" {
+  cd `chart_dir`
+  run helm template \
+      -x templates/connect-inject-deployment.yaml  \
+      --set 'global.enabled=true' \
+      --set 'client.enabled=false' \
+      --set 'connectInject.enabled=true' .
+  [ "$status" -eq 1 ]
+  [[ "$output" =~ "clients must be enabled for connect injection" ]]
+}
+
+@test "connectInject/Deployment: fails if global.enabled=false and client.enabled=false" {
+  cd `chart_dir`
+  run helm template \
+      -x templates/connect-inject-deployment.yaml  \
+      --set 'global.enabled=false' \
+      --set 'client.enabled=false' \
+      --set 'connectInject.enabled=true' .
+  [ "$status" -eq 1 ]
+  [[ "$output" =~ "clients must be enabled for connect injection" ]]
+}
+
+@test "connectInject/Deployment: fails if client.grpc=false" {
+  cd `chart_dir`
+  run helm template \
+      -x templates/connect-inject-deployment.yaml  \
+      --set 'client.grpc=false' \
+      --set 'connectInject.enabled=true' .
+  [ "$status" -eq 1 ]
+  [[ "$output" =~ "client.grpc must be true for connect injection" ]]
+}
+
 #--------------------------------------------------------------------
 # consul and envoy images
 

diff --git a/test/unit/connect-inject-mutatingwebhook.bats b/test/unit/connect-inject-mutatingwebhook.bats
@@ -16,6 +16,7 @@ load _helpers
   local actual=$(helm template \
       -x templates/connect-inject-mutatingwebhook.yaml  \
       --set 'global.enabled=false' \
+      --set 'client.enabled=true' \
       --set 'connectInject.enabled=true' \
       . | tee /dev/stderr |
       yq 'length > 0' | tee /dev/stderr)

diff --git a/test/unit/connect-inject-service.bats b/test/unit/connect-inject-service.bats
@@ -16,6 +16,7 @@ load _helpers
   local actual=$(helm template \
       -x templates/connect-inject-service.yaml  \
       --set 'global.enabled=false' \
+      --set 'client.enabled=true' \
       --set 'connectInject.enabled=true' \
       . | tee /dev/stderr |
       yq 'length > 0' | tee /dev/stderr)

diff --git a/test/unit/connect-inject-serviceaccount.bats b/test/unit/connect-inject-serviceaccount.bats
@@ -16,6 +16,7 @@ load _helpers
   local actual=$(helm template \
       -x templates/connect-inject-serviceaccount.yaml  \
       --set 'global.enabled=false' \
+      --set 'client.enabled=true' \
       --set 'connectInject.enabled=true' \
       . | tee /dev/stderr |
       yq -s 'length > 0' | tee /dev/stderr)

diff --git a/values.yaml b/values.yaml
@@ -167,9 +167,9 @@ client:
   image: null
   join: null
 
-  # grpc should be set to true if the gRPC listener should be enabled.
+  # If true, Consul's gRPC port will be exposed (see https://www.consul.io/docs/agent/options.html#grpc_port).
   # This should be set to true if connectInject or meshGateway is enabled.
-  grpc: false
+  grpc: true
 
   # Resource requests, limits, etc. for the client cluster placement. This
   # should map directly to the value of the resources field for a PodSpec,
@@ -308,7 +308,8 @@ ui:
 # enabled then set the node selection so that it chooses a node with a
 # Consul agent.
 syncCatalog:
-  # True if you want to enable the catalog sync. "-" for default.
+  # True if you want to enable the catalog sync. Set to "-" to inherit from
+  # global.enabled.
   enabled: false
   image: null
   default: true # true will sync by default, otherwise requires annotation
@@ -374,6 +375,8 @@ syncCatalog:
 
 # ConnectInject will enable the automatic Connect sidecar injector.
 connectInject:
+  # True if you want to enable connect injection. Set to "-" to inherit from
+  # global.enabled.
   enabled: false
   image: null # image for consul-k8s that contains the injector
   default: false # true will inject by default, otherwise requires annotation