From c305e76b3039007e3d7c8b4a6387f9dd1c3c2687 Mon Sep 17 00:00:00 2001 From: Curt Bushko Date: Fri, 21 Oct 2022 19:14:23 -0400 Subject: [PATCH] Update based on PR review --- CHANGELOG.md | 7 +- .../bases/static-metrics-app/deployment.yaml | 2 - acceptance/tests/metrics/metrics_test.go | 13 +- .../ingress-gateways-deployment.yaml | 3 +- .../templates/mesh-gateway-deployment.yaml | 3 +- .../terminating-gateways-deployment.yaml | 3 +- .../consul_dataplane_sidecar.go | 31 +++-- .../consul_dataplane_sidecar_test.go | 127 +++++++++++++++++- 8 files changed, 163 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d9f8c23ce..48bf140853 100644 --- 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 index 146730479e..a3020ddb47 100644 --- 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 index 76db3f4594..dc7c32434f 100644 --- 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 index f37abf816b..d79e069b92 100644 --- 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 index 1ba39e2d17..b92e9a4b62 100644 --- 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 index c00fb49991..8cb9005d96 100644 --- 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 index abcd0a1d9f..936d27df0a 100644 --- 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 index 4d2e051a80..906f1346aa 100644 --- 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) + } + }) + } +}