Refactor metrics merging to work with endpoints-controller

connect-inject/consul_sidecar.go

@@ -1,7 +1,6 @@
 package connectinject
 
 import (
-	"errors"
 	"fmt"
 
 	corev1 "k8s.io/api/core/v1"
@@ -12,36 +11,19 @@ import (
 // It always disables service registration because for connect we no longer
 // need to keep services registered as this is handled in the endpoints-controller.
 func (h *Handler) consulSidecar(pod corev1.Pod) (corev1.Container, error) {
-	run, err := h.shouldRunMergedMetricsServer(pod)
+	metricsPorts, err := h.MetricsConfig.mergedMetricsServerConfiguration(pod)
 	if err != nil {
 		return corev1.Container{}, err
 	}
 
-	// This should never happen because we only call this function in the handler if
-	// we need to run the metrics merging server. This check is here just in case.
-	if !run {
-		return corev1.Container{}, errors.New("metrics merging should be enabled in order to inject the consul-sidecar")
-	}
-
-	// Configure consul sidecar with the appropriate metrics flags.
-	mergedMetricsPort, err := h.mergedMetricsPort(pod)
-	if err != nil {
-		return corev1.Container{}, err
-	}
-	serviceMetricsPath := h.serviceMetricsPath(pod)
-
-	// Don't need to check the error since it's checked in the call to
-	// h.shouldRunMergedMetricsServer() above.
-	serviceMetricsPort, _ := h.serviceMetricsPort(pod)
-
 	command := []string{
 		"consul-k8s",
 		"consul-sidecar",
 		"-enable-service-registration=false",
 		"-enable-metrics-merging=true",
-		fmt.Sprintf("-merged-metrics-port=%s", mergedMetricsPort),
-		fmt.Sprintf("-service-metrics-port=%s", serviceMetricsPort),
-		fmt.Sprintf("-service-metrics-path=%s", serviceMetricsPath),
+		fmt.Sprintf("-merged-metrics-port=%s", metricsPorts.mergedPort),
+		fmt.Sprintf("-service-metrics-port=%s", metricsPorts.servicePort),
+		fmt.Sprintf("-service-metrics-path=%s", metricsPorts.servicePath),
 	}
 
 	return corev1.Container{

connect-inject/consul_sidecar_test.go

@@ -6,35 +6,19 @@ import (
 	"github.com/hashicorp/go-hclog"
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-var (
-	consulSidecarResources = corev1.ResourceRequirements{
-		Requests: corev1.ResourceList{
-			corev1.ResourceCPU:    resource.MustParse("10m"),
-			corev1.ResourceMemory: resource.MustParse("25Mi"),
-		},
-		Limits: corev1.ResourceList{
-			corev1.ResourceCPU:    resource.MustParse("20m"),
-			corev1.ResourceMemory: resource.MustParse("50Mi"),
-		},
-	}
-)
-
-// NOTE: This is tested here rather than in handler_test because doing it there
-// would require a lot of boilerplate to get at the underlying patches that would
-// complicate understanding the tests (which are simple).
-
 // Test that if the conditions for running a merged metrics server are true,
 // that we pass the metrics flags to consul sidecar.
 func TestConsulSidecar_MetricsFlags(t *testing.T) {
 	handler := Handler{
-		Log:                         hclog.Default().Named("handler"),
-		ImageConsulK8S:              "hashicorp/consul-k8s:9.9.9",
-		DefaultEnableMetrics:        true,
-		DefaultEnableMetricsMerging: true,
+		Log:            hclog.Default().Named("handler"),
+		ImageConsulK8S: "hashicorp/consul-k8s:9.9.9",
+		MetricsConfig: MetricsConfig{
+			DefaultEnableMetrics:        true,
+			DefaultEnableMetricsMerging: true,
+		},
 	}
 	container, err := handler.consulSidecar(corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
@@ -59,22 +43,3 @@ func TestConsulSidecar_MetricsFlags(t *testing.T) {
 	require.Contains(t, container.Command, "-service-metrics-port=8080")
 	require.Contains(t, container.Command, "-service-metrics-path=/metrics")
 }
-
-// Test that the Consul sidecar errors when metrics merging is disabled.
-func TestConsulSidecar_ErrorsWhenMetricsMergingIsDisabled(t *testing.T) {
-	handler := Handler{
-		Log:                    hclog.Default().Named("handler"),
-		ImageConsulK8S:         "hashicorp/consul-k8s:9.9.9",
-		ConsulSidecarResources: consulSidecarResources,
-	}
-	_, err := handler.consulSidecar(corev1.Pod{
-		Spec: corev1.PodSpec{
-			Containers: []corev1.Container{
-				{
-					Name: "web",
-				},
-			},
-		},
-	})
-	require.EqualError(t, err, "metrics merging should be enabled in order to inject the consul-sidecar")
-}

connect-inject/container_init.go

@@ -77,34 +77,16 @@ func (h *Handler) containerInit(pod corev1.Pod, k8sNamespace string) (corev1.Con
 		return corev1.Container{}, fmt.Errorf("serviceAccountName %q does not match service name %q", pod.Spec.ServiceAccountName, data.ServiceName)
 	}
 
-	// If metrics are enabled, the init container should set up
-	// envoy_prometheus_bind_addr so there's a listener on 0.0.0.0 that points
-	// to a metrics backend. The metrics backend is determined by the call to
-	// h.shouldRunMergedMetricsServer(). If there is a merged metrics server,
-	// the backend would be that server. If we are not running the merged
-	// metrics server, the backend should just be the Envoy metrics endpoint.
-	enableMetrics, err := h.enableMetrics(pod)
-	if err != nil {
-		return corev1.Container{}, err
-	}
-	data.EnableMetrics = enableMetrics
-
-	prometheusScrapePort, err := h.prometheusScrapePort(pod)
-	if err != nil {
-		return corev1.Container{}, err
-	}
-	data.PrometheusScrapeListener = fmt.Sprintf("0.0.0.0:%s", prometheusScrapePort)
-
 	// This determines how to configure the consul connect envoy command: what
 	// metrics backend to use and what path to expose on the
 	// envoy_prometheus_bind_addr listener for scraping.
-	run, err := h.shouldRunMergedMetricsServer(pod)
+	metricsServer, err := h.MetricsConfig.shouldRunMergedMetricsServer(pod)
 	if err != nil {
 		return corev1.Container{}, err
 	}
-	if run {
-		prometheusScrapePath := h.prometheusScrapePath(pod)
-		mergedMetricsPort, err := h.mergedMetricsPort(pod)
+	if metricsServer {
+		prometheusScrapePath := h.MetricsConfig.prometheusScrapePath(pod)
+		mergedMetricsPort, err := h.MetricsConfig.mergedMetricsPort(pod)
 		if err != nil {
 			return corev1.Container{}, err
 		}

connect-inject/endpoints_controller.go

@@ -25,9 +25,10 @@ import (
 )
 
 const (
-	MetaKeyPodName         = "pod-name"
-	MetaKeyKubeServiceName = "k8s-service-name"
-	MetaKeyKubeNS          = "k8s-namespace"
+	MetaKeyPodName          = "pod-name"
+	MetaKeyKubeServiceName  = "k8s-service-name"
+	MetaKeyKubeNS           = "k8s-namespace"
+	envoyPrometheusBindAddr = "envoy_prometheus_bind_addr"
 )
 
 type EndpointsController struct {
@@ -47,6 +48,7 @@ type EndpointsController struct {
 	ReleaseName string
 	// ReleaseNamespace is the namespace where Consul is installed.
 	ReleaseNamespace string
+	MetricsConfig    MetricsConfig
 	Log              logr.Logger
 	Scheme           *runtime.Scheme
 	context.Context
@@ -225,7 +227,25 @@ func (r *EndpointsController) createServiceRegistrations(pod corev1.Pod, service
 	proxyConfig := &api.AgentServiceConnectProxyConfig{
 		DestinationServiceName: serviceName,
 		DestinationServiceID:   serviceID,
-		Config:                 nil, // TODO: add config for metrics (upcoming PR)
+		Config:                 make(map[string]interface{}),
+	}
+
+	// If metrics are enabled, the proxyConfig should set envoy_prometheus_bind_addr to a listener on 0.0.0.0 on
+	// the prometheusScrapePort that points to a metrics backend. The backend for this listener will be determined by
+	// the envoy bootstrapping command (consul connect envoy) configuration in the init container. If there is a merged
+	// metrics server, the backend would be that server. If we are not running the merged metrics server, the backend
+	// should just be the Envoy metrics endpoint.
+	enableMetrics, err := r.MetricsConfig.enableMetrics(pod)
+	if err != nil {
+		return nil, nil, err
+	}
+	if enableMetrics {
+		prometheusScrapePort, err := r.MetricsConfig.prometheusScrapePort(pod)
+		if err != nil {
+			return nil, nil, err
+		}
+		prometheusScrapeListener := fmt.Sprintf("0.0.0.0:%s", prometheusScrapePort)
+		proxyConfig.Config[envoyPrometheusBindAddr] = prometheusScrapeListener
 	}
 
 	if servicePort > 0 {

connect-inject/endpoints_controller_test.go

@@ -515,7 +515,7 @@ func TestReconcileCreateEndpoint(t *testing.T) {
 			},
 		},
 		{
-			name:          "Every configurable field set: port, different Consul service name, meta, tags, upstreams",
+			name:          "Every configurable field set: port, different Consul service name, meta, tags, upstreams, metrics",
 			consulSvcName: "different-consul-svc-name",
 			k8sObjects: func() []runtime.Object {
 				pod1 := createPod("pod1", "1.2.3.4", true)
@@ -526,6 +526,8 @@ func TestReconcileCreateEndpoint(t *testing.T) {
 				pod1.Annotations[annotationTags] = "abc,123"
 				pod1.Annotations[annotationConnectTags] = "def,456"
 				pod1.Annotations[annotationUpstreams] = "upstream1:1234"
+				pod1.Annotations[annotationEnableMetrics] = "true"
+				pod1.Annotations[annotationPrometheusScrapePort] = "12345"
 				endpoint := &corev1.Endpoints{
 					ObjectMeta: metav1.ObjectMeta{
 						Name:      "service-created",
@@ -585,6 +587,9 @@ func TestReconcileCreateEndpoint(t *testing.T) {
 								LocalBindPort:   1234,
 							},
 						},
+						Config: map[string]interface{}{
+							"envoy_prometheus_bind_addr": "0.0.0.0:12345",
+						},
 					},
 					ServiceMeta: map[string]string{
 						"name":                 "abc",