From 0ad49a9880b1c2aef3d6bbd7cc1926e1f4b1d752 Mon Sep 17 00:00:00 2001
From: Luke Kysow <1034429+lkysow@users.noreply.github.com>
Date: Tue, 9 Nov 2021 16:06:47 -0800
Subject: [PATCH] Fix logic for enabling ui metrics (#841)

* Fix logic for enabling ui metrics

Previously ui metrics would be enabled when
`global.metrics.enabled=false` and `ui.metrics.enabled=-`. Now that
would cause UI metrics to be disabled which makes sense since "-" means
inherit from `global.metrics.enabled`.

The logic is now to enable ui metrics when:

```
ui.enabled &&
  (
    (global.metrics.enabled == true && ui.metrics.enabled == "-") ||
    (ui.metrics.enabled == true)
  )
```
---
 CHANGELOG.md                                  | 12 ++++++----
 .../templates/server-config-configmap.yaml    |  2 +-
 .../test/unit/server-config-configmap.bats    | 22 +++++++++++++++++++
 .../consul/test/unit/server-statefulset.bats  |  6 ++---
 4 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 15df277aa1..6f26d88bef 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,10 +1,17 @@
 ## UNRELEASED
 
+BREAKING CHANGES:
+* Previously [UI metrics](https://www.consul.io/docs/connect/observability/ui-visualization) would be enabled when
+  `global.metrics=false` and `ui.metrics.enabled=-`. If you are no longer seeing UI metrics,
+  set `global.metrics=true` or `ui.metrics.enabled=true`. [[GH-841](https://github.com/hashicorp/consul-k8s/pull/841)]
+
 IMPROVEMENTS:
 * Control Plane
   * TLS: Support PKCS1 and PKCS8 private keys for Consul certificate authority. [[GH-843](https://github.com/hashicorp/consul-k8s/pull/843)]
 * CLI
   * Delete jobs, cluster roles, and cluster role bindings on `uninstall`. [[GH-820](https://github.com/hashicorp/consul-k8s/pull/820)]
+* Helm Chart
+  * Add `component` labels to all resources. [[GH-840](https://github.com/hashicorp/consul-k8s/pull/840)]
 
 BUG FIXES:
 * Control Plane
@@ -18,10 +25,7 @@ BUG FIXES:
   * **(Consul Enterprise only)** Error on Helm install if a reserved name is used for the admin partition name or a
     Consul destination namespace for connect or catalog sync. [[GH-846](https://github.com/hashicorp/consul-k8s/pull/846)]
   * Truncate Persistent Volume Claim names when namespace names are too long. [[GH-799](https://github.com/hashicorp/consul-k8s/pull/799)]
-
-IMPROVEMENTS:
-* Helm Chart
-  * Add `component` labels to all resources. [[GH-840](https://github.com/hashicorp/consul-k8s/pull/840)]
+  * Fix issue where UI metrics would be enabled when `global.metrics=false` and `ui.metrics.enabled=-`. [[GH-841](https://github.com/hashicorp/consul-k8s/pull/841)]
 
 ## 0.36.0 (November 02, 2021)
 
diff --git a/charts/consul/templates/server-config-configmap.yaml b/charts/consul/templates/server-config-configmap.yaml
index 4cadf0c303..83b31755fc 100644
--- a/charts/consul/templates/server-config-configmap.yaml
+++ b/charts/consul/templates/server-config-configmap.yaml
@@ -28,7 +28,7 @@ data:
       }
     }
   {{- end }}
-  {{- if (and .Values.ui.enabled (or .Values.ui.metrics.enabled (and .Values.global.metrics.enabled (eq (.Values.ui.metrics.enabled | toString) "-")))) }}
+  {{- if (and .Values.ui.enabled (or (eq "true" (.Values.ui.metrics.enabled | toString) ) (and .Values.global.metrics.enabled (eq "-" (.Values.ui.metrics.enabled | toString))))) }}
   ui-config.json: |-
     {
       "ui_config": {
diff --git a/charts/consul/test/unit/server-config-configmap.bats b/charts/consul/test/unit/server-config-configmap.bats
index 25a08da37c..f7fd6ce033 100755
--- a/charts/consul/test/unit/server-config-configmap.bats
+++ b/charts/consul/test/unit/server-config-configmap.bats
@@ -86,6 +86,28 @@ load _helpers
   [ "${actual}" = "true" ]
 }
 
+@test "server/ConfigMap: does not create ui config when .ui.enabled=false and .ui.metrics.enabled=true" {
+  cd `chart_dir`
+  local actual=$(helm template \
+      -s templates/server-config-configmap.yaml  \
+      --set 'ui.enabled=false' \
+      --set 'ui.metrics.enabled=false' \
+      . | tee /dev/stderr |
+      yq -r '.data["ui-config.json"] | length > 0' | tee /dev/stderr)
+  [ "${actual}" = "false" ]
+}
+
+@test "server/ConfigMap: does not create ui config when .ui.enabled=true and .global.metrics.enabled=false" {
+  cd `chart_dir`
+  local actual=$(helm template \
+      -s templates/server-config-configmap.yaml  \
+      --set 'ui.enabled=true' \
+      --set 'global.metrics.enabled=false' \
+      . | tee /dev/stderr |
+      yq -r '.data["ui-config.json"] | length > 0' | tee /dev/stderr)
+  [ "${actual}" = "false" ]
+}
+
 @test "server/ConfigMap: does not create ui config when .ui.enabled=true and .ui.metrics.enabled=false" {
   cd `chart_dir`
   local actual=$(helm template \
diff --git a/charts/consul/test/unit/server-statefulset.bats b/charts/consul/test/unit/server-statefulset.bats
index 6936bce1e0..647ad2994d 100755
--- a/charts/consul/test/unit/server-statefulset.bats
+++ b/charts/consul/test/unit/server-statefulset.bats
@@ -686,7 +686,7 @@ load _helpers
       -s templates/server-statefulset.yaml  \
       . | tee /dev/stderr |
       yq -r '.spec.template.metadata.annotations."consul.hashicorp.com/config-checksum"' | tee /dev/stderr)
-  [ "${actual}" = b56ac97f873dd8c41c779964e24a458c58fc41404fa2642d3b12b24cd2091e43 ]
+  [ "${actual}" = 2c5397272acdc6fe5b079bf25c846c5a17f474603c794c64e7226ce0690625f7 ]
 }
 
 @test "server/StatefulSet: adds config-checksum annotation when extraConfig is provided" {
@@ -696,7 +696,7 @@ load _helpers
       --set 'server.extraConfig="{\"hello\": \"world\"}"' \
       . | tee /dev/stderr |
       yq -r '.spec.template.metadata.annotations."consul.hashicorp.com/config-checksum"' | tee /dev/stderr)
-  [ "${actual}" = d637bd5c2a3738c2885d483fd003d59badc164883897e6032ac2dbe543bb6539 ]
+  [ "${actual}" = b0d22cb051216505edc0e61b57f9eacc0d7e15b24719d815842df88f06f1abe0 ]
 }
 
 @test "server/StatefulSet: adds config-checksum annotation when config is updated" {
@@ -706,7 +706,7 @@ load _helpers
       --set 'global.acls.manageSystemACLs=true' \
       . | tee /dev/stderr |
       yq -r '.spec.template.metadata.annotations."consul.hashicorp.com/config-checksum"' | tee /dev/stderr)
-  [ "${actual}" = daf925cbf6af12cc87f5d7791370e28dddbe9ca78d8ad9fc963e558c60a333e7 ]
+  [ "${actual}" = 7772975be982e25cc8df101375374e2ba672a55737f8f1580011e0d88d8752a8 ]
 }
 
 #--------------------------------------------------------------------