Update based on PR review

hashicorp · Oct 21, 2022 · c305e76 · c305e76
1 parent 51637c3
commit c305e76
Show file tree

Hide file tree

Showing 8 changed files with 163 additions and 26 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,11 +10,14 @@ IMPROVEMENTS:
   * Remove deprecated annotation `service.alpha.kubernetes.io/tolerate-unready-endpoints: "true"` in the `server-service` template. [[GH-1619](https://github.com/hashicorp/consul-k8s/pull/1619)]
   * Support `minAvailable` on connect injector `PodDisruptionBudget`. [[GH-1557](https://github.com/hashicorp/consul-k8s/pull/1557)]
   * Add `tolerations` and `nodeSelector` to Server ACL init jobs and `nodeSelector` to Webhook cert manager. [[GH-1581](https://github.com/hashicorp/consul-k8s/pull/1581)]
-
-
 * Control plane
   * Support merged metrics with consul-dataplane. [[GH-1635](https://github.com/hashicorp/consul-k8s/pull/1635)]
 
+BREAKING_CHANGES:
+
+* Helm:
+  * Removal of `consulSidecarContainer` from values file as there is no longer a consul sidecar [[GH-1635](https://github.com/hashicorp/consul-k8s/pull/1635)]
+
 ## 1.0.0-beta3 (October 12, 2022)
 
 FEATURES:

diff --git a/acceptance/tests/fixtures/bases/static-metrics-app/deployment.yaml b/acceptance/tests/fixtures/bases/static-metrics-app/deployment.yaml
@@ -14,8 +14,6 @@ spec:
       annotations:
         'consul.hashicorp.com/connect-inject': 'true'
         'consul.hashicorp.com/connect-service': 'server'
-        'consul.hashicorp.com/service-metrics-path': '/metrics'
-        'consul.hashicorp.com/service-metrics-port': '9090'
       labels:
         app: static-metrics-app
     spec:

diff --git a/acceptance/tests/metrics/metrics_test.go b/acceptance/tests/metrics/metrics_test.go
@@ -34,7 +34,10 @@ func TestComponentMetrics(t *testing.T) {
 		"global.datacenter":                 "dc1",
 		"global.metrics.enabled":            "true",
 		"global.metrics.enableAgentMetrics": "true",
-		"client.enabled":                    "true",
+		// Agents have been removed but there could potentially be customers that are still running them. We
+		// are using client.enabled to cover that scenario and to make sure agent metrics still works with
+		// consul-dataplane.
+		"client.enabled": "true",
 
 		"connectInject.enabled": "true",
 		"controller.enabled":    "true",
@@ -81,11 +84,13 @@ func TestComponentMetrics(t *testing.T) {
 	require.NoError(t, err)
 	require.Contains(t, metricsOutput, `consul_acl_ResolveToken{quantile="0.5"}`)
 
-	// Ingress Gateway Metrics
+	logger.Log(t, "ingress gateway metrics")
 	assertGatewayMetricsEnabled(t, ctx, ns, "ingress-gateway", `envoy_cluster_assignment_stale{local_cluster="ingress-gateway",consul_source_service="ingress-gateway"`)
-	// Terminating Gateway Metrics
+
+	logger.Log(t, "terminating gateway metrics")
 	assertGatewayMetricsEnabled(t, ctx, ns, "terminating-gateway", `envoy_cluster_assignment_stale{local_cluster="terminating-gateway",consul_source_service="terminating-gateway"`)
-	// Mesh Gateway Metrics
+
+	logger.Log(t, "mesh gateway metrics")
 	assertGatewayMetricsEnabled(t, ctx, ns, "mesh-gateway", `envoy_cluster_assignment_stale{local_cluster="mesh-gateway",consul_source_service="mesh-gateway"`)
 }
 

diff --git a/charts/consul/templates/ingress-gateways-deployment.yaml b/charts/consul/templates/ingress-gateways-deployment.yaml
@@ -292,8 +292,7 @@ spec:
             -log-level={{ default $root.Values.global.logLevel }} \
             -log-json={{ $root.Values.global.logJSON }} \
             {{- if (and $root.Values.global.metrics.enabled $root.Values.global.metrics.enableGatewayMetrics) }}
-            -telemetry-prom-scrape-path={{ $root.Values.connectInject.metrics.defaultPrometheusScrapePath }} \
-            -telemetry-prom-merge-port={{ $root.Values.connectInject.metrics.defaultMergedMetricsPort }}
+            -telemetry-prom-scrape-path={{ $root.Values.connectInject.metrics.defaultPrometheusScrapePath }}
             {{- end }}
         livenessProbe:
           tcpSocket:

diff --git a/charts/consul/templates/mesh-gateway-deployment.yaml b/charts/consul/templates/mesh-gateway-deployment.yaml
@@ -242,8 +242,7 @@ spec:
             -log-level={{ default .Values.global.logLevel }} \
             -log-json={{ .Values.global.logJSON }} \
             {{- if (and .Values.global.metrics.enabled .Values.global.metrics.enableGatewayMetrics) }}
-            -telemetry-prom-scrape-path={{ .Values.connectInject.metrics.defaultPrometheusScrapePath }} \
-            -telemetry-prom-merge-port={{ .Values.connectInject.metrics.defaultMergedMetricsPort }}
+            -telemetry-prom-scrape-path={{ .Values.connectInject.metrics.defaultPrometheusScrapePath }}
             {{- end }}
         livenessProbe:
           tcpSocket:

diff --git a/charts/consul/templates/terminating-gateways-deployment.yaml b/charts/consul/templates/terminating-gateways-deployment.yaml
@@ -283,8 +283,7 @@ spec:
                 -log-level={{ default $root.Values.global.logLevel }} \
                 -log-json={{ $root.Values.global.logJSON }} \
                 {{- if (and $root.Values.global.metrics.enabled $root.Values.global.metrics.enableGatewayMetrics) }}
-                -telemetry-prom-scrape-path={{ $root.Values.connectInject.metrics.defaultPrometheusScrapePath }} \
-                -telemetry-prom-merge-port={{ $root.Values.connectInject.metrics.defaultMergedMetricsPort }}
+                -telemetry-prom-scrape-path={{ $root.Values.connectInject.metrics.defaultPrometheusScrapePath }}
                 {{- end }}
           livenessProbe:
             tcpSocket:

diff --git a/control-plane/connect-inject/consul_dataplane_sidecar.go b/control-plane/connect-inject/consul_dataplane_sidecar.go
@@ -203,35 +203,39 @@ func (w *MeshWebhook) getContainerSidecarCommand(namespace corev1.Namespace, mpi
 		cmd = append(cmd, "-telemetry-prom-scrape-path="+prometheusScrapePath,
 			"-telemetry-prom-merge-port="+mergedMetricsPort)
 
-		serviceMetricsPath := pod.Annotations[annotationServiceMetricsPath]
-		serviceMetricsPort := pod.Annotations[annotationServiceMetricsPort]
+		serviceMetricsPath := w.MetricsConfig.serviceMetricsPath(pod)
+		serviceMetricsPort, err := w.MetricsConfig.serviceMetricsPort(pod)
+		if err != nil {
+			return nil, fmt.Errorf("unable to determine if service metrics port: %w", err)
+		}
+
 		if serviceMetricsPath != "" && serviceMetricsPort != "" {
 			cmd = append(cmd, "-telemetry-prom-service-metrics-url="+fmt.Sprintf("http://127.0.0.1:%s%s", serviceMetricsPort, serviceMetricsPath))
 		}
 
 		// Pull the TLS config from the relevant annotations.
 		var prometheusCAFile string
-		if prometheusCAFile, ok := pod.Annotations[annotationPrometheusCAFile]; ok && prometheusCAFile != "" {
-			cmd = append(cmd, "-telemetry-prom-ca-certs-file="+prometheusCAFile)
+		if raw, ok := pod.Annotations[annotationPrometheusCAFile]; ok && raw != "" {
+			prometheusCAFile = raw
 		}
 
 		var prometheusCAPath string
-		if prometheusCAPath, ok := pod.Annotations[annotationPrometheusCAPath]; ok && prometheusCAPath != "" {
-			cmd = append(cmd, "-telemetry-prom-ca-certs-path="+prometheusCAPath)
+		if raw, ok := pod.Annotations[annotationPrometheusCAPath]; ok && raw != "" {
+			prometheusCAPath = raw
 		}
 
 		var prometheusCertFile string
-		if prometheusCertFile, ok := pod.Annotations[annotationPrometheusCertFile]; ok && prometheusCertFile != "" {
-			cmd = append(cmd, "-telemetry-prom-cert-file="+prometheusCertFile)
+		if raw, ok := pod.Annotations[annotationPrometheusCertFile]; ok && raw != "" {
+			prometheusCertFile = raw
 		}
 
 		var prometheusKeyFile string
-		if prometheusKeyFile, ok := pod.Annotations[annotationPrometheusKeyFile]; ok && prometheusKeyFile != "" {
-			cmd = append(cmd, "-telemetry-prom-key-file="+prometheusKeyFile)
+		if raw, ok := pod.Annotations[annotationPrometheusKeyFile]; ok && raw != "" {
+			prometheusKeyFile = raw
 		}
 
 		// Validate required Prometheus TLS config is present if set.
-		if prometheusCertFile != "" || prometheusKeyFile != "" || prometheusCAFile != "" || prometheusCAPath != "" {
+		if prometheusCAFile != "" || prometheusCAPath != "" || prometheusCertFile != "" || prometheusKeyFile != "" {
 			if prometheusCAFile == "" && prometheusCAPath == "" {
 				return nil, fmt.Errorf("must set one of %q or %q when providing prometheus TLS config", annotationPrometheusCAFile, annotationPrometheusCAPath)
 			}
@@ -241,6 +245,11 @@ func (w *MeshWebhook) getContainerSidecarCommand(namespace corev1.Namespace, mpi
 			if prometheusKeyFile == "" {
 				return nil, fmt.Errorf("must set %q when providing prometheus TLS config", annotationPrometheusKeyFile)
 			}
+			// TLS config has been validated, add them to the consul-dataplane cmd args
+			cmd = append(cmd, "-telemetry-prom-ca-certs-file="+prometheusCAFile,
+				"-telemetry-prom-ca-certs-path="+prometheusCAPath,
+				"-telemetry-prom-cert-file="+prometheusCertFile,
+				"-telemetry-prom-key-file="+prometheusKeyFile)
 		}
 	}
 

diff --git a/control-plane/connect-inject/consul_dataplane_sidecar_test.go b/control-plane/connect-inject/consul_dataplane_sidecar_test.go
@@ -167,7 +167,8 @@ func TestHandlerConsulDataplaneSidecar(t *testing.T) {
 				"/bin/sh", "-ec",
 				"consul-dataplane -addresses=\"1.1.1.1\" -grpc-port=" + strconv.Itoa(w.ConsulConfig.GRPCPort) +
 					" -proxy-service-id=$(cat /consul/connect-inject/proxyid) " +
-					"-service-node-name=k8s-service-mesh -log-level=" + w.LogLevel + " -log-json=" + strconv.FormatBool(w.LogJSON) + " -envoy-concurrency=0" + c.additionalExpCmdArgs}
+					"-service-node-name=k8s-service-mesh -log-level=" + w.LogLevel + " -log-json=" + strconv.FormatBool(w.LogJSON) + " -envoy-concurrency=0" + c.additionalExpCmdArgs,
+			}
 			require.Equal(t, expCmd, container.Command)
 
 			if w.AuthMethod != "" {
@@ -888,3 +889,127 @@ func TestHandlerConsulDataplaneSidecar_Resources(t *testing.T) {
 		})
 	}
 }
+
+func TestHandlerConsulDataplaneSidecar_Metrics(t *testing.T) {
+	cases := []struct {
+		name       string
+		pod        corev1.Pod
+		expCmdArgs string
+		expErr     string
+	}{
+		{
+			name:       "default",
+			pod:        corev1.Pod{},
+			expCmdArgs: "",
+		},
+		{
+			name: "turning on merged metrics",
+			pod: corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						annotationService:              "web",
+						annotationEnableMetrics:        "true",
+						annotationEnableMetricsMerging: "true",
+						annotationMergedMetricsPort:    "20100",
+						annotationPort:                 "1234",
+						annotationPrometheusScrapePath: "/scrape-path",
+					},
+				},
+			},
+			expCmdArgs: "-telemetry-prom-scrape-path=/scrape-path -telemetry-prom-merge-port=20100 -telemetry-prom-service-metrics-url=http://127.0.0.1:1234/metrics",
+		},
+		{
+			name: "merged metrics with TLS enabled",
+			pod: corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						annotationService:              "web",
+						annotationEnableMetrics:        "true",
+						annotationEnableMetricsMerging: "true",
+						annotationMergedMetricsPort:    "20100",
+						annotationPort:                 "1234",
+						annotationPrometheusScrapePath: "/scrape-path",
+						annotationPrometheusCAFile:     "/certs/ca.crt",
+						annotationPrometheusCAPath:     "/certs/ca",
+						annotationPrometheusCertFile:   "/certs/server.crt",
+						annotationPrometheusKeyFile:    "/certs/key.pem",
+					},
+				},
+			},
+			expCmdArgs: "-telemetry-prom-scrape-path=/scrape-path -telemetry-prom-merge-port=20100 -telemetry-prom-service-metrics-url=http://127.0.0.1:1234/metrics -telemetry-prom-ca-certs-file=/certs/ca.crt -telemetry-prom-ca-certs-path=/certs/ca -telemetry-prom-cert-file=/certs/server.crt -telemetry-prom-key-file=/certs/key.pem",
+		},
+		{
+			name: "merge metrics with TLS enabled, missing CA gives an error",
+			pod: corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						annotationService:              "web",
+						annotationEnableMetrics:        "true",
+						annotationEnableMetricsMerging: "true",
+						annotationMergedMetricsPort:    "20100",
+						annotationPort:                 "1234",
+						annotationPrometheusScrapePath: "/scrape-path",
+						annotationPrometheusCertFile:   "/certs/server.crt",
+						annotationPrometheusKeyFile:    "/certs/key.pem",
+					},
+				},
+			},
+			expCmdArgs: "",
+			expErr:     fmt.Sprintf("must set one of %q or %q when providing prometheus TLS config", annotationPrometheusCAFile, annotationPrometheusCAPath),
+		},
+		{
+			name: "merge metrics with TLS enabled, missing cert gives an error",
+			pod: corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						annotationService:              "web",
+						annotationEnableMetrics:        "true",
+						annotationEnableMetricsMerging: "true",
+						annotationMergedMetricsPort:    "20100",
+						annotationPort:                 "1234",
+						annotationPrometheusScrapePath: "/scrape-path",
+						annotationPrometheusCAFile:     "/certs/ca.crt",
+						annotationPrometheusKeyFile:    "/certs/key.pem",
+					},
+				},
+			},
+			expCmdArgs: "",
+			expErr:     fmt.Sprintf("must set %q when providing prometheus TLS config", annotationPrometheusCertFile),
+		},
+		{
+			name: "merge metrics with TLS enabled, missing key file gives an error",
+			pod: corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						annotationService:              "web",
+						annotationEnableMetrics:        "true",
+						annotationEnableMetricsMerging: "true",
+						annotationMergedMetricsPort:    "20100",
+						annotationPort:                 "1234",
+						annotationPrometheusScrapePath: "/scrape-path",
+						annotationPrometheusCAPath:     "/certs/ca",
+						annotationPrometheusCertFile:   "/certs/server.crt",
+					},
+				},
+			},
+			expCmdArgs: "",
+			expErr:     fmt.Sprintf("must set %q when providing prometheus TLS config", annotationPrometheusKeyFile),
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			h := MeshWebhook{
+				ConsulConfig: &consul.Config{HTTPPort: 8500, GRPCPort: 8502},
+			}
+			container, err := h.consulDataplaneSidecar(testNS, c.pod, multiPortInfo{})
+			if c.expErr != "" {
+				require.NotNil(t, err)
+				require.Contains(t, err.Error(), c.expErr)
+			} else {
+				require.NoError(t, err)
+				require.Contains(t, container.Command[2], c.expCmdArgs)
+			}
+		})
+	}
+}