diff --git a/connect-inject/consul_sidecar.go b/connect-inject/consul_sidecar.go index 5f1477d222..6ffc8035b0 100644 --- a/connect-inject/consul_sidecar.go +++ b/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{ diff --git a/connect-inject/consul_sidecar_test.go b/connect-inject/consul_sidecar_test.go index 0c0aad7aa6..3ebec360eb 100644 --- a/connect-inject/consul_sidecar_test.go +++ b/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") -} diff --git a/connect-inject/container_init.go b/connect-inject/container_init.go index 37b38c12da..91f4970493 100644 --- a/connect-inject/container_init.go +++ b/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 } diff --git a/connect-inject/endpoints_controller.go b/connect-inject/endpoints_controller.go index a5d4600741..8bb0b55d90 100644 --- a/connect-inject/endpoints_controller.go +++ b/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 { diff --git a/connect-inject/endpoints_controller_test.go b/connect-inject/endpoints_controller_test.go index b27c153659..b144756369 100644 --- a/connect-inject/endpoints_controller_test.go +++ b/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", diff --git a/connect-inject/handler.go b/connect-inject/handler.go index 56e6350022..1f917a0339 100644 --- a/connect-inject/handler.go +++ b/connect-inject/handler.go @@ -22,10 +22,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) -const ( - defaultServiceMetricsPath = "/metrics" -) - var ( codecs = serializer.NewCodecFactory(runtime.NewScheme()) deserializer = codecs.UniversalDeserializer() @@ -113,14 +109,10 @@ type Handler struct { DefaultProxyMemoryRequest resource.Quantity DefaultProxyMemoryLimit resource.Quantity - // Default metrics settings. These will configure where Prometheus scrapes - // metrics from, and whether to run a merged metrics endpoint on the consul - // sidecar. These can be overridden via pod annotations. - DefaultEnableMetrics bool - DefaultEnableMetricsMerging bool - DefaultMergedMetricsPort string - DefaultPrometheusScrapePort string - DefaultPrometheusScrapePath string + // MetricsConfig contains metrics configuration from the inject-connect command and has methods to determine whether + // configuration should come from the default flags or annotations. The handler uses this to configure prometheus + // annotations and the merged metrics server. + MetricsConfig MetricsConfig // Resource settings for init container. All of these fields // will be populated by the defaults provided in the initial flags. @@ -218,7 +210,7 @@ func (h *Handler) Handle(_ context.Context, req admission.Request) admission.Res // (that functionality lives in the endpoints-controller), // we only need the consul sidecar to run the metrics merging server. // First, determine if we need to run the metrics merging server. - shouldRunMetricsMerging, err := h.shouldRunMergedMetricsServer(pod) + shouldRunMetricsMerging, err := h.MetricsConfig.shouldRunMergedMetricsServer(pod) if err != nil { h.Log.Error("Error determining if metrics merging server should be run", "err", err, "Request Name", req.Name) return admission.Errored(http.StatusInternalServerError, fmt.Errorf("error determining if metrics merging server should be run: %s", err)) @@ -352,101 +344,18 @@ func (h *Handler) defaultAnnotations(pod *corev1.Pod) error { return nil } -// enableMetrics returns the default value in the handler, or overrides that -// with the annotation if provided. -func (h *Handler) enableMetrics(pod corev1.Pod) (bool, error) { - enabled := h.DefaultEnableMetrics - if raw, ok := pod.Annotations[annotationEnableMetrics]; ok && raw != "" { - enableMetrics, err := strconv.ParseBool(raw) - if err != nil { - return false, fmt.Errorf("%s annotation value of %s was invalid: %s", annotationEnableMetrics, raw, err) - } - enabled = enableMetrics - } - return enabled, nil -} - -// enableMetricsMerging returns the default value in the handler, or overrides -// that with the annotation if provided. -func (h *Handler) enableMetricsMerging(pod corev1.Pod) (bool, error) { - enabled := h.DefaultEnableMetricsMerging - if raw, ok := pod.Annotations[annotationEnableMetricsMerging]; ok && raw != "" { - enableMetricsMerging, err := strconv.ParseBool(raw) - if err != nil { - return false, fmt.Errorf("%s annotation value of %s was invalid: %s", annotationEnableMetricsMerging, raw, err) - } - enabled = enableMetricsMerging - } - return enabled, nil -} - -// mergedMetricsPort returns the default value in the handler, or overrides -// that with the annotation if provided. -func (h *Handler) mergedMetricsPort(pod corev1.Pod) (string, error) { - return determineAndValidatePort(pod, annotationMergedMetricsPort, h.DefaultMergedMetricsPort, false) -} - -// prometheusScrapePort returns the default value in the handler, or overrides -// that with the annotation if provided. -func (h *Handler) prometheusScrapePort(pod corev1.Pod) (string, error) { - return determineAndValidatePort(pod, annotationPrometheusScrapePort, h.DefaultPrometheusScrapePort, false) -} - -// prometheusScrapePath returns the default value in the handler, or overrides -// that with the annotation if provided. -func (h *Handler) prometheusScrapePath(pod corev1.Pod) string { - if raw, ok := pod.Annotations[annotationPrometheusScrapePath]; ok && raw != "" { - return raw - } - - return h.DefaultPrometheusScrapePath -} - -// serviceMetricsPort returns the port the service exposes metrics on. This will -// default to the port used to register the service with Consul, and can be -// overridden with the annotation if provided. -func (h *Handler) serviceMetricsPort(pod corev1.Pod) (string, error) { - // The annotationPort is the port used to register the service with Consul. - // If that has been set, it'll be used as the port for getting service - // metrics as well, unless overridden by the service-metrics-port annotation. - if raw, ok := pod.Annotations[annotationPort]; ok && raw != "" { - // The service metrics port can be privileged if the service author has - // written their service in such a way that it expects to be able to use - // privileged ports. So, the port metrics are exposed on the service can - // be privileged. - return determineAndValidatePort(pod, annotationServiceMetricsPort, raw, true) - } - - // If the annotationPort is not set, the serviceMetrics port will be 0 - // unless overridden by the service-metrics-port annotation. If the service - // metrics port is 0, the consul sidecar will not run a merged metrics - // server. - return determineAndValidatePort(pod, annotationServiceMetricsPort, "0", true) -} - -// serviceMetricsPath returns a default of /metrics, or overrides -// that with the annotation if provided. -func (h *Handler) serviceMetricsPath(pod corev1.Pod) string { - if raw, ok := pod.Annotations[annotationServiceMetricsPath]; ok && raw != "" { - return raw - } - - return defaultServiceMetricsPath -} - -// prometheusAnnotations returns the Prometheus scraping configuration -// annotations. It returns a nil map if metrics are not enabled and annotations -// should not be set. +// prometheusAnnotations sets the Prometheus scraping configuration +// annotations on the Pod. func (h *Handler) prometheusAnnotations(pod *corev1.Pod) error { - enableMetrics, err := h.enableMetrics(*pod) + enableMetrics, err := h.MetricsConfig.enableMetrics(*pod) if err != nil { return err } - prometheusScrapePort, err := h.prometheusScrapePort(*pod) + prometheusScrapePort, err := h.MetricsConfig.prometheusScrapePort(*pod) if err != nil { return err } - prometheusScrapePath := h.prometheusScrapePath(*pod) + prometheusScrapePath := h.MetricsConfig.prometheusScrapePath(*pod) if enableMetrics { pod.Annotations[annotationPrometheusScrape] = "true" @@ -456,68 +365,6 @@ func (h *Handler) prometheusAnnotations(pod *corev1.Pod) error { return nil } -// shouldRunMergedMetricsServer returns whether we need to run a merged metrics -// server. This is used to configure the consul sidecar command, and the init -// container, so it can pass appropriate arguments to the consul connect envoy -// command. -func (h *Handler) shouldRunMergedMetricsServer(pod corev1.Pod) (bool, error) { - enableMetrics, err := h.enableMetrics(pod) - if err != nil { - return false, err - } - enableMetricsMerging, err := h.enableMetricsMerging(pod) - if err != nil { - return false, err - } - serviceMetricsPort, err := h.serviceMetricsPort(pod) - if err != nil { - return false, err - } - - // Don't need to check error here since serviceMetricsPort has been - // validated by calling h.serviceMetricsPort above - smp, _ := strconv.Atoi(serviceMetricsPort) - - if enableMetrics && enableMetricsMerging && smp > 0 { - return true, nil - } - return false, nil -} - -// determineAndValidatePort behaves as follows: -// If the annotation exists, validate the port and return it. -// If the annotation does not exist, return the default port. -// If the privileged flag is true, it will allow the port to be in the -// privileged port range of 1-1023. Otherwise, it will only allow ports in the -// unprivileged range of 1024-65535. -func determineAndValidatePort(pod corev1.Pod, annotation string, defaultPort string, privileged bool) (string, error) { - if raw, ok := pod.Annotations[annotation]; ok && raw != "" { - port, err := portValue(pod, raw) - if err != nil { - return "", fmt.Errorf("%s annotation value of %s is not a valid integer", annotation, raw) - } - - if privileged && (port < 1 || port > 65535) { - return "", fmt.Errorf("%s annotation value of %d is not in the valid port range 1-65535", annotation, port) - } else if !privileged && (port < 1024 || port > 65535) { - return "", fmt.Errorf("%s annotation value of %d is not in the unprivileged port range 1024-65535", annotation, port) - } - - // if the annotation exists, return the validated port - return fmt.Sprint(port), nil - } - - // if the annotation does not exist, return the default - if defaultPort != "" { - port, err := portValue(pod, defaultPort) - if err != nil { - return "", fmt.Errorf("%s is not a valid port on the pod %s", defaultPort, pod.Name) - } - return fmt.Sprint(port), nil - } - return "", nil -} - // consulNamespace returns the namespace that a service should be // registered in based on the namespace options. It returns an // empty string if namespaces aren't enabled. diff --git a/connect-inject/handler_test.go b/connect-inject/handler_test.go index b76f861b20..98c73f7b91 100644 --- a/connect-inject/handler_test.go +++ b/connect-inject/handler_test.go @@ -342,12 +342,14 @@ func TestHandlerHandle(t *testing.T) { { "when metrics merging is enabled, we should inject the consul-sidecar and add prometheus annotations", Handler{ - Log: hclog.Default().Named("handler"), - AllowK8sNamespacesSet: mapset.NewSetWith("*"), - DenyK8sNamespacesSet: mapset.NewSet(), - DefaultEnableMetrics: true, - DefaultEnableMetricsMerging: true, - decoder: decoder, + Log: hclog.Default().Named("handler"), + AllowK8sNamespacesSet: mapset.NewSetWith("*"), + DenyK8sNamespacesSet: mapset.NewSet(), + MetricsConfig: MetricsConfig{ + DefaultEnableMetrics: true, + DefaultEnableMetricsMerging: true, + }, + decoder: decoder, }, admission.Request{ AdmissionRequest: admissionv1.AdmissionRequest{ @@ -641,274 +643,6 @@ func TestHandlerDefaultAnnotations(t *testing.T) { } } -func minimal() *corev1.Pod { - return &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "minimal", - Annotations: map[string]string{ - annotationService: "foo", - }, - }, - - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "web", - }, - { - Name: "web-side", - }, - }, - }, - } -} - -func TestHandlerEnableMetrics(t *testing.T) { - cases := []struct { - Name string - Pod func(*corev1.Pod) *corev1.Pod - Handler Handler - Expected bool - Err string - }{ - { - Name: "Metrics enabled via handler", - Pod: func(pod *corev1.Pod) *corev1.Pod { - return pod - }, - Handler: Handler{ - DefaultEnableMetrics: true, - }, - Expected: true, - Err: "", - }, - { - Name: "Metrics enabled via annotation", - Pod: func(pod *corev1.Pod) *corev1.Pod { - pod.Annotations[annotationEnableMetrics] = "true" - return pod - }, - Handler: Handler{ - DefaultEnableMetrics: false, - }, - Expected: true, - Err: "", - }, - { - Name: "Metrics configured via invalid annotation", - Pod: func(pod *corev1.Pod) *corev1.Pod { - pod.Annotations[annotationEnableMetrics] = "not-a-bool" - return pod - }, - Handler: Handler{ - DefaultEnableMetrics: false, - }, - Expected: false, - Err: "consul.hashicorp.com/enable-metrics annotation value of not-a-bool was invalid: strconv.ParseBool: parsing \"not-a-bool\": invalid syntax", - }, - } - - for _, tt := range cases { - t.Run(tt.Name, func(t *testing.T) { - require := require.New(t) - h := tt.Handler - - actual, err := h.enableMetrics(*tt.Pod(minimal())) - - if tt.Err == "" { - require.Equal(tt.Expected, actual) - require.NoError(err) - } else { - require.EqualError(err, tt.Err) - } - }) - } -} - -func TestHandlerEnableMetricsMerging(t *testing.T) { - cases := []struct { - Name string - Pod func(*corev1.Pod) *corev1.Pod - Handler Handler - Expected bool - Err string - }{ - { - Name: "Metrics merging enabled via handler", - Pod: func(pod *corev1.Pod) *corev1.Pod { - return pod - }, - Handler: Handler{ - DefaultEnableMetricsMerging: true, - }, - Expected: true, - Err: "", - }, - { - Name: "Metrics merging enabled via annotation", - Pod: func(pod *corev1.Pod) *corev1.Pod { - pod.Annotations[annotationEnableMetricsMerging] = "true" - return pod - }, - Handler: Handler{ - DefaultEnableMetricsMerging: false, - }, - Expected: true, - Err: "", - }, - { - Name: "Metrics merging configured via invalid annotation", - Pod: func(pod *corev1.Pod) *corev1.Pod { - pod.Annotations[annotationEnableMetricsMerging] = "not-a-bool" - return pod - }, - Handler: Handler{ - DefaultEnableMetricsMerging: false, - }, - Expected: false, - Err: "consul.hashicorp.com/enable-metrics-merging annotation value of not-a-bool was invalid: strconv.ParseBool: parsing \"not-a-bool\": invalid syntax", - }, - } - - for _, tt := range cases { - t.Run(tt.Name, func(t *testing.T) { - require := require.New(t) - h := tt.Handler - - actual, err := h.enableMetricsMerging(*tt.Pod(minimal())) - - if tt.Err == "" { - require.Equal(tt.Expected, actual) - require.NoError(err) - } else { - require.EqualError(err, tt.Err) - } - }) - } -} - -func TestHandlerServiceMetricsPort(t *testing.T) { - cases := []struct { - Name string - Pod func(*corev1.Pod) *corev1.Pod - Expected string - }{ - { - Name: "Prefers annotationServiceMetricsPort", - Pod: func(pod *corev1.Pod) *corev1.Pod { - pod.Annotations[annotationPort] = "1234" - pod.Annotations[annotationServiceMetricsPort] = "9000" - return pod - }, - Expected: "9000", - }, - { - Name: "Uses annotationPort of annotationServiceMetricsPort is not set", - Pod: func(pod *corev1.Pod) *corev1.Pod { - pod.Annotations[annotationPort] = "1234" - return pod - }, - Expected: "1234", - }, - { - Name: "Is set to 0 if neither annotationPort nor annotationServiceMetricsPort is set", - Pod: func(pod *corev1.Pod) *corev1.Pod { - return pod - }, - Expected: "0", - }, - } - - for _, tt := range cases { - t.Run(tt.Name, func(t *testing.T) { - require := require.New(t) - h := Handler{} - - actual, err := h.serviceMetricsPort(*tt.Pod(minimal())) - - require.Equal(tt.Expected, actual) - require.NoError(err) - }) - } -} - -func TestHandlerServiceMetricsPath(t *testing.T) { - cases := []struct { - Name string - Pod func(*corev1.Pod) *corev1.Pod - Expected string - }{ - { - Name: "Defaults to /metrics", - Pod: func(pod *corev1.Pod) *corev1.Pod { - return pod - }, - Expected: "/metrics", - }, - { - Name: "Uses annotationServiceMetricsPath when set", - Pod: func(pod *corev1.Pod) *corev1.Pod { - pod.Annotations[annotationServiceMetricsPath] = "/custom-metrics-path" - return pod - }, - Expected: "/custom-metrics-path", - }, - } - - for _, tt := range cases { - t.Run(tt.Name, func(t *testing.T) { - require := require.New(t) - h := Handler{} - - actual := h.serviceMetricsPath(*tt.Pod(minimal())) - - require.Equal(tt.Expected, actual) - }) - } -} - -func TestHandlerPrometheusScrapePath(t *testing.T) { - cases := []struct { - Name string - Pod func(*corev1.Pod) *corev1.Pod - Handler Handler - Expected string - }{ - { - Name: "Defaults to the handler's value", - Pod: func(pod *corev1.Pod) *corev1.Pod { - return pod - }, - Handler: Handler{ - DefaultPrometheusScrapePath: "/default-prometheus-scrape-path", - }, - Expected: "/default-prometheus-scrape-path", - }, - { - Name: "Uses annotationPrometheusScrapePath when set", - Pod: func(pod *corev1.Pod) *corev1.Pod { - pod.Annotations[annotationPrometheusScrapePath] = "/custom-scrape-path" - return pod - }, - Handler: Handler{ - DefaultPrometheusScrapePath: "/default-prometheus-scrape-path", - }, - Expected: "/custom-scrape-path", - }, - } - - for _, tt := range cases { - t.Run(tt.Name, func(t *testing.T) { - require := require.New(t) - h := tt.Handler - - actual := h.prometheusScrapePath(*tt.Pod(minimal())) - - require.Equal(tt.Expected, actual) - }) - } -} - func TestHandlerPrometheusAnnotations(t *testing.T) { cases := []struct { Name string @@ -918,9 +652,11 @@ func TestHandlerPrometheusAnnotations(t *testing.T) { { Name: "Sets the correct prometheus annotations on the pod if metrics are enabled", Handler: Handler{ - DefaultEnableMetrics: true, - DefaultPrometheusScrapePort: "20200", - DefaultPrometheusScrapePath: "/metrics", + MetricsConfig: MetricsConfig{ + DefaultEnableMetrics: true, + DefaultPrometheusScrapePort: "20200", + DefaultPrometheusScrapePath: "/metrics", + }, }, Expected: map[string]string{ annotationPrometheusScrape: "true", @@ -931,9 +667,11 @@ func TestHandlerPrometheusAnnotations(t *testing.T) { { Name: "Does not set annotations if metrics are not enabled", Handler: Handler{ - DefaultEnableMetrics: false, - DefaultPrometheusScrapePort: "20200", - DefaultPrometheusScrapePath: "/metrics", + MetricsConfig: MetricsConfig{ + DefaultEnableMetrics: false, + DefaultPrometheusScrapePort: "20200", + DefaultPrometheusScrapePath: "/metrics", + }, }, Expected: map[string]string{}, }, @@ -953,197 +691,6 @@ func TestHandlerPrometheusAnnotations(t *testing.T) { } } -// This test only needs unique cases not already handled in tests for -// h.enableMetrics, h.enableMetricsMerging, and h.serviceMetricsPort. -func TestHandlerShouldRunMergedMetricsServer(t *testing.T) { - cases := []struct { - Name string - Pod func(*corev1.Pod) *corev1.Pod - Handler Handler - Expected bool - }{ - { - Name: "Returns true when metrics and metrics merging are enabled, and the service metrics port is greater than 0", - Pod: func(pod *corev1.Pod) *corev1.Pod { - pod.Annotations[annotationPort] = "1234" - return pod - }, - Handler: Handler{ - DefaultEnableMetrics: true, - DefaultEnableMetricsMerging: true, - }, - Expected: true, - }, - { - Name: "Returns false when service metrics port is 0", - Pod: func(pod *corev1.Pod) *corev1.Pod { - pod.Annotations[annotationPort] = "0" - return pod - }, - Handler: Handler{ - DefaultEnableMetrics: true, - DefaultEnableMetricsMerging: true, - }, - Expected: false, - }, - } - - for _, tt := range cases { - t.Run(tt.Name, func(t *testing.T) { - require := require.New(t) - h := tt.Handler - - actual, err := h.shouldRunMergedMetricsServer(*tt.Pod(minimal())) - - require.Equal(tt.Expected, actual) - require.NoError(err) - }) - } -} - -// Tests determineAndValidatePort, which in turn tests the -// prometheusScrapePort() and mergedMetricsPort() functions because their logic -// is just to call out to determineAndValidatePort(). -func TestHandlerDetermineAndValidatePort(t *testing.T) { - cases := []struct { - Name string - Pod func(*corev1.Pod) *corev1.Pod - Annotation string - Privileged bool - DefaultPort string - Expected string - Err string - }{ - { - Name: "Valid annotation", - Pod: func(pod *corev1.Pod) *corev1.Pod { - pod.Annotations["consul.hashicorp.com/test-annotation-port"] = "1234" - return pod - }, - Annotation: "consul.hashicorp.com/test-annotation-port", - Privileged: false, - Expected: "1234", - Err: "", - }, - { - Name: "Uses default when there's no annotation", - Pod: func(pod *corev1.Pod) *corev1.Pod { - return pod - }, - Annotation: "consul.hashicorp.com/test-annotation-port", - Privileged: false, - DefaultPort: "4321", - Expected: "4321", - Err: "", - }, - { - Name: "Gets the value of the named default port when there's no annotation", - Pod: func(pod *corev1.Pod) *corev1.Pod { - pod.Spec.Containers[0].Ports = []corev1.ContainerPort{ - { - Name: "web-port", - ContainerPort: 2222, - }, - } - return pod - }, - Annotation: "consul.hashicorp.com/test-annotation-port", - Privileged: false, - DefaultPort: "web-port", - Expected: "2222", - Err: "", - }, - { - Name: "Errors if the named default port doesn't exist on the pod", - Pod: func(pod *corev1.Pod) *corev1.Pod { - return pod - }, - Annotation: "consul.hashicorp.com/test-annotation-port", - Privileged: false, - DefaultPort: "web-port", - Expected: "", - Err: "web-port is not a valid port on the pod minimal", - }, - { - Name: "Gets the value of the named port", - Pod: func(pod *corev1.Pod) *corev1.Pod { - pod.Annotations["consul.hashicorp.com/test-annotation-port"] = "web-port" - pod.Spec.Containers[0].Ports = []corev1.ContainerPort{ - { - Name: "web-port", - ContainerPort: 2222, - }, - } - return pod - }, - Annotation: "consul.hashicorp.com/test-annotation-port", - Privileged: false, - DefaultPort: "4321", - Expected: "2222", - Err: "", - }, - { - Name: "Invalid annotation (not an integer)", - Pod: func(pod *corev1.Pod) *corev1.Pod { - pod.Annotations["consul.hashicorp.com/test-annotation-port"] = "not-an-int" - return pod - }, - Annotation: "consul.hashicorp.com/test-annotation-port", - Privileged: false, - Expected: "", - Err: "consul.hashicorp.com/test-annotation-port annotation value of not-an-int is not a valid integer", - }, - { - Name: "Invalid annotation (integer not in port range)", - Pod: func(pod *corev1.Pod) *corev1.Pod { - pod.Annotations["consul.hashicorp.com/test-annotation-port"] = "100000" - return pod - }, - Annotation: "consul.hashicorp.com/test-annotation-port", - Privileged: true, - Expected: "", - Err: "consul.hashicorp.com/test-annotation-port annotation value of 100000 is not in the valid port range 1-65535", - }, - { - Name: "Invalid annotation (integer not in unprivileged port range)", - Pod: func(pod *corev1.Pod) *corev1.Pod { - pod.Annotations["consul.hashicorp.com/test-annotation-port"] = "22" - return pod - }, - Annotation: "consul.hashicorp.com/test-annotation-port", - Privileged: false, - Expected: "", - Err: "consul.hashicorp.com/test-annotation-port annotation value of 22 is not in the unprivileged port range 1024-65535", - }, - { - Name: "Privileged ports allowed", - Pod: func(pod *corev1.Pod) *corev1.Pod { - pod.Annotations["consul.hashicorp.com/test-annotation-port"] = "22" - return pod - }, - Annotation: "consul.hashicorp.com/test-annotation-port", - Privileged: true, - Expected: "22", - Err: "", - }, - } - - for _, tt := range cases { - t.Run(tt.Name, func(t *testing.T) { - require := require.New(t) - - actual, err := determineAndValidatePort(*tt.Pod(minimal()), tt.Annotation, tt.DefaultPort, tt.Privileged) - - if tt.Err == "" { - require.NoError(err) - require.Equal(tt.Expected, actual) - } else { - require.EqualError(err, tt.Err) - } - }) - } -} - // Test portValue function func TestHandlerPortValue(t *testing.T) { cases := []struct { diff --git a/connect-inject/metrics_configuration.go b/connect-inject/metrics_configuration.go new file mode 100644 index 0000000000..cdfc5d0b6e --- /dev/null +++ b/connect-inject/metrics_configuration.go @@ -0,0 +1,206 @@ +package connectinject + +import ( + "errors" + "fmt" + "strconv" + + corev1 "k8s.io/api/core/v1" +) + +// MetricsConfig represents configuration common to connect-inject components related to metrics. +type MetricsConfig struct { + DefaultEnableMetrics bool + DefaultEnableMetricsMerging bool + DefaultMergedMetricsPort string + DefaultPrometheusScrapePort string + DefaultPrometheusScrapePath string +} + +type metricsPorts struct { + mergedPort string + servicePort string + servicePath string +} + +const ( + defaultServiceMetricsPath = "/metrics" +) + +// mergedMetricsServerConfiguration is called when running a merged metrics server and used to return ports necessary to +// configure the merged metrics server. +func (mc MetricsConfig) mergedMetricsServerConfiguration(pod corev1.Pod) (metricsPorts, error) { + run, err := mc.shouldRunMergedMetricsServer(pod) + if err != nil { + return metricsPorts{}, 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 metricsPorts{}, errors.New("metrics merging should be enabled in order to return the metrics server configuration") + } + + // Configure consul sidecar with the appropriate metrics flags. + mergedMetricsPort, err := mc.mergedMetricsPort(pod) + if err != nil { + return metricsPorts{}, err + } + + // Don't need to check the error since it's checked in the call to + // mc.shouldRunMergedMetricsServer() above. + serviceMetricsPort, _ := mc.serviceMetricsPort(pod) + + serviceMetricsPath := mc.serviceMetricsPath(pod) + + metricsPorts := metricsPorts{ + mergedPort: mergedMetricsPort, + servicePort: serviceMetricsPort, + servicePath: serviceMetricsPath, + } + return metricsPorts, nil +} + +// enableMetrics returns whether metrics are enabled either via the default value in the handler, or if it's been +// overridden via the annotation. +func (mc MetricsConfig) enableMetrics(pod corev1.Pod) (bool, error) { + enabled := mc.DefaultEnableMetrics + if raw, ok := pod.Annotations[annotationEnableMetrics]; ok && raw != "" { + enableMetrics, err := strconv.ParseBool(raw) + if err != nil { + return false, fmt.Errorf("%s annotation value of %s was invalid: %s", annotationEnableMetrics, raw, err) + } + enabled = enableMetrics + } + return enabled, nil +} + +// enableMetricsMerging returns whether metrics merging functionality is enabled either via the default value in the +// handler, or if it's been overridden via the annotation. +func (mc MetricsConfig) enableMetricsMerging(pod corev1.Pod) (bool, error) { + enabled := mc.DefaultEnableMetricsMerging + if raw, ok := pod.Annotations[annotationEnableMetricsMerging]; ok && raw != "" { + enableMetricsMerging, err := strconv.ParseBool(raw) + if err != nil { + return false, fmt.Errorf("%s annotation value of %s was invalid: %s", annotationEnableMetricsMerging, raw, err) + } + enabled = enableMetricsMerging + } + return enabled, nil +} + +// mergedMetricsPort returns the port to run the merged metrics server on, either via the default value in the handler, +// or if it's been overridden via the annotation. It also validates the port is in the unprivileged port range. +func (mc MetricsConfig) mergedMetricsPort(pod corev1.Pod) (string, error) { + return determineAndValidatePort(pod, annotationMergedMetricsPort, mc.DefaultMergedMetricsPort, false) +} + +// prometheusScrapePort returns the port for Prometheus to scrape from, either via the default value in the handler, or +// if it's been overridden via the annotation. It also validates the port is in the unprivileged port range. +func (mc MetricsConfig) prometheusScrapePort(pod corev1.Pod) (string, error) { + return determineAndValidatePort(pod, annotationPrometheusScrapePort, mc.DefaultPrometheusScrapePort, false) +} + +// prometheusScrapePath returns the path for Prometheus to scrape from, either via the default value in the handler, or +// if it's been overridden via the annotation. +func (mc MetricsConfig) prometheusScrapePath(pod corev1.Pod) string { + if raw, ok := pod.Annotations[annotationPrometheusScrapePath]; ok && raw != "" { + return raw + } + + return mc.DefaultPrometheusScrapePath +} + +// serviceMetricsPort returns the port the service exposes metrics on. This will +// default to the port used to register the service with Consul, and can be +// overridden with the annotation if provided. +func (mc MetricsConfig) serviceMetricsPort(pod corev1.Pod) (string, error) { + // The annotationPort is the port used to register the service with Consul. + // If that has been set, it'll be used as the port for getting service + // metrics as well, unless overridden by the service-metrics-port annotation. + if raw, ok := pod.Annotations[annotationPort]; ok && raw != "" { + // The service metrics port can be privileged if the service author has + // written their service in such a way that it expects to be able to use + // privileged ports. So, the port metrics are exposed on the service can + // be privileged. + return determineAndValidatePort(pod, annotationServiceMetricsPort, raw, true) + } + + // If the annotationPort is not set, the serviceMetrics port will be 0 + // unless overridden by the service-metrics-port annotation. If the service + // metrics port is 0, the consul sidecar will not run a merged metrics + // server. + return determineAndValidatePort(pod, annotationServiceMetricsPort, "0", true) +} + +// serviceMetricsPath returns a default of /metrics, or overrides +// that with the annotation if provided. +func (mc MetricsConfig) serviceMetricsPath(pod corev1.Pod) string { + if raw, ok := pod.Annotations[annotationServiceMetricsPath]; ok && raw != "" { + return raw + } + + return defaultServiceMetricsPath +} + +// shouldRunMergedMetricsServer returns whether we need to run a merged metrics +// server. This is used to configure the consul sidecar command, and the init +// container, so it can pass appropriate arguments to the consul connect envoy +// command. +func (mc MetricsConfig) shouldRunMergedMetricsServer(pod corev1.Pod) (bool, error) { + enableMetrics, err := mc.enableMetrics(pod) + if err != nil { + return false, err + } + enableMetricsMerging, err := mc.enableMetricsMerging(pod) + if err != nil { + return false, err + } + serviceMetricsPort, err := mc.serviceMetricsPort(pod) + if err != nil { + return false, err + } + + // Don't need to check error here since serviceMetricsPort has been + // validated by calling mc.serviceMetricsPort above. + smp, _ := strconv.Atoi(serviceMetricsPort) + + if enableMetrics && enableMetricsMerging && smp > 0 { + return true, nil + } + return false, nil +} + +// determineAndValidatePort behaves as follows: +// If the annotation exists, validate the port and return it. +// If the annotation does not exist, return the default port. +// If the privileged flag is true, it will allow the port to be in the +// privileged port range of 1-1023. Otherwise, it will only allow ports in the +// unprivileged range of 1024-65535. +func determineAndValidatePort(pod corev1.Pod, annotation string, defaultPort string, privileged bool) (string, error) { + if raw, ok := pod.Annotations[annotation]; ok && raw != "" { + port, err := portValue(pod, raw) + if err != nil { + return "", fmt.Errorf("%s annotation value of %s is not a valid integer", annotation, raw) + } + + if privileged && (port < 1 || port > 65535) { + return "", fmt.Errorf("%s annotation value of %d is not in the valid port range 1-65535", annotation, port) + } else if !privileged && (port < 1024 || port > 65535) { + return "", fmt.Errorf("%s annotation value of %d is not in the unprivileged port range 1024-65535", annotation, port) + } + + // If the annotation exists, return the validated port. + return fmt.Sprint(port), nil + } + + // If the annotation does not exist, return the default. + if defaultPort != "" { + port, err := portValue(pod, defaultPort) + if err != nil { + return "", fmt.Errorf("%s is not a valid port on the pod %s", defaultPort, pod.Name) + } + return fmt.Sprint(port), nil + } + return "", nil +} diff --git a/connect-inject/metrics_configuration_test.go b/connect-inject/metrics_configuration_test.go new file mode 100644 index 0000000000..c07eaaf1b1 --- /dev/null +++ b/connect-inject/metrics_configuration_test.go @@ -0,0 +1,527 @@ +package connectinject + +import ( + "testing" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestMetricsConfigEnableMetrics(t *testing.T) { + cases := []struct { + Name string + Pod func(*corev1.Pod) *corev1.Pod + MetricsConfig MetricsConfig + Expected bool + Err string + }{ + { + Name: "Metrics enabled via handler", + Pod: func(pod *corev1.Pod) *corev1.Pod { + return pod + }, + MetricsConfig: MetricsConfig{ + DefaultEnableMetrics: true, + }, + Expected: true, + Err: "", + }, + { + Name: "Metrics enabled via annotation", + Pod: func(pod *corev1.Pod) *corev1.Pod { + pod.Annotations[annotationEnableMetrics] = "true" + return pod + }, + MetricsConfig: MetricsConfig{ + DefaultEnableMetrics: false, + }, + Expected: true, + Err: "", + }, + { + Name: "Metrics configured via invalid annotation", + Pod: func(pod *corev1.Pod) *corev1.Pod { + pod.Annotations[annotationEnableMetrics] = "not-a-bool" + return pod + }, + MetricsConfig: MetricsConfig{ + DefaultEnableMetrics: false, + }, + Expected: false, + Err: "consul.hashicorp.com/enable-metrics annotation value of not-a-bool was invalid: strconv.ParseBool: parsing \"not-a-bool\": invalid syntax", + }, + } + + for _, tt := range cases { + t.Run(tt.Name, func(t *testing.T) { + require := require.New(t) + mc := tt.MetricsConfig + + actual, err := mc.enableMetrics(*tt.Pod(minimal())) + + if tt.Err == "" { + require.Equal(tt.Expected, actual) + require.NoError(err) + } else { + require.EqualError(err, tt.Err) + } + }) + } +} + +func TestMetricsConfigEnableMetricsMerging(t *testing.T) { + cases := []struct { + Name string + Pod func(*corev1.Pod) *corev1.Pod + MetricsConfig MetricsConfig + Expected bool + Err string + }{ + { + Name: "Metrics merging enabled via handler", + Pod: func(pod *corev1.Pod) *corev1.Pod { + return pod + }, + MetricsConfig: MetricsConfig{ + DefaultEnableMetricsMerging: true, + }, + Expected: true, + Err: "", + }, + { + Name: "Metrics merging enabled via annotation", + Pod: func(pod *corev1.Pod) *corev1.Pod { + pod.Annotations[annotationEnableMetricsMerging] = "true" + return pod + }, + MetricsConfig: MetricsConfig{ + DefaultEnableMetricsMerging: false, + }, + Expected: true, + Err: "", + }, + { + Name: "Metrics merging configured via invalid annotation", + Pod: func(pod *corev1.Pod) *corev1.Pod { + pod.Annotations[annotationEnableMetricsMerging] = "not-a-bool" + return pod + }, + MetricsConfig: MetricsConfig{ + DefaultEnableMetricsMerging: false, + }, + Expected: false, + Err: "consul.hashicorp.com/enable-metrics-merging annotation value of not-a-bool was invalid: strconv.ParseBool: parsing \"not-a-bool\": invalid syntax", + }, + } + + for _, tt := range cases { + t.Run(tt.Name, func(t *testing.T) { + require := require.New(t) + mc := tt.MetricsConfig + + actual, err := mc.enableMetricsMerging(*tt.Pod(minimal())) + + if tt.Err == "" { + require.Equal(tt.Expected, actual) + require.NoError(err) + } else { + require.EqualError(err, tt.Err) + } + }) + } +} + +func TestMetricsConfigServiceMetricsPort(t *testing.T) { + cases := []struct { + Name string + Pod func(*corev1.Pod) *corev1.Pod + Expected string + }{ + { + Name: "Prefers annotationServiceMetricsPort", + Pod: func(pod *corev1.Pod) *corev1.Pod { + pod.Annotations[annotationPort] = "1234" + pod.Annotations[annotationServiceMetricsPort] = "9000" + return pod + }, + Expected: "9000", + }, + { + Name: "Uses annotationPort of annotationServiceMetricsPort is not set", + Pod: func(pod *corev1.Pod) *corev1.Pod { + pod.Annotations[annotationPort] = "1234" + return pod + }, + Expected: "1234", + }, + { + Name: "Is set to 0 if neither annotationPort nor annotationServiceMetricsPort is set", + Pod: func(pod *corev1.Pod) *corev1.Pod { + return pod + }, + Expected: "0", + }, + } + + for _, tt := range cases { + t.Run(tt.Name, func(t *testing.T) { + require := require.New(t) + mc := MetricsConfig{} + + actual, err := mc.serviceMetricsPort(*tt.Pod(minimal())) + + require.Equal(tt.Expected, actual) + require.NoError(err) + }) + } +} + +func TestMetricsConfigServiceMetricsPath(t *testing.T) { + cases := []struct { + Name string + Pod func(*corev1.Pod) *corev1.Pod + Expected string + }{ + { + Name: "Defaults to /metrics", + Pod: func(pod *corev1.Pod) *corev1.Pod { + return pod + }, + Expected: "/metrics", + }, + { + Name: "Uses annotationServiceMetricsPath when set", + Pod: func(pod *corev1.Pod) *corev1.Pod { + pod.Annotations[annotationServiceMetricsPath] = "/custom-metrics-path" + return pod + }, + Expected: "/custom-metrics-path", + }, + } + + for _, tt := range cases { + t.Run(tt.Name, func(t *testing.T) { + require := require.New(t) + mc := MetricsConfig{} + + actual := mc.serviceMetricsPath(*tt.Pod(minimal())) + + require.Equal(tt.Expected, actual) + }) + } +} + +func TestMetricsConfigPrometheusScrapePath(t *testing.T) { + cases := []struct { + Name string + Pod func(*corev1.Pod) *corev1.Pod + MetricsConfig MetricsConfig + Expected string + }{ + { + Name: "Defaults to the handler's value", + Pod: func(pod *corev1.Pod) *corev1.Pod { + return pod + }, + MetricsConfig: MetricsConfig{ + DefaultPrometheusScrapePath: "/default-prometheus-scrape-path", + }, + Expected: "/default-prometheus-scrape-path", + }, + { + Name: "Uses annotationPrometheusScrapePath when set", + Pod: func(pod *corev1.Pod) *corev1.Pod { + pod.Annotations[annotationPrometheusScrapePath] = "/custom-scrape-path" + return pod + }, + MetricsConfig: MetricsConfig{ + DefaultPrometheusScrapePath: "/default-prometheus-scrape-path", + }, + Expected: "/custom-scrape-path", + }, + } + + for _, tt := range cases { + t.Run(tt.Name, func(t *testing.T) { + require := require.New(t) + mc := tt.MetricsConfig + + actual := mc.prometheusScrapePath(*tt.Pod(minimal())) + + require.Equal(tt.Expected, actual) + }) + } +} + +// This test only needs unique cases not already handled in tests for +// h.enableMetrics, h.enableMetricsMerging, and h.serviceMetricsPort. +func TestMetricsConfigShouldRunMergedMetricsServer(t *testing.T) { + cases := []struct { + Name string + Pod func(*corev1.Pod) *corev1.Pod + MetricsConfig MetricsConfig + Expected bool + }{ + { + Name: "Returns true when metrics and metrics merging are enabled, and the service metrics port is greater than 0", + Pod: func(pod *corev1.Pod) *corev1.Pod { + pod.Annotations[annotationPort] = "1234" + return pod + }, + MetricsConfig: MetricsConfig{ + DefaultEnableMetrics: true, + DefaultEnableMetricsMerging: true, + }, + Expected: true, + }, + { + Name: "Returns false when service metrics port is 0", + Pod: func(pod *corev1.Pod) *corev1.Pod { + pod.Annotations[annotationPort] = "0" + return pod + }, + MetricsConfig: MetricsConfig{ + DefaultEnableMetrics: true, + DefaultEnableMetricsMerging: true, + }, + Expected: false, + }, + } + + for _, tt := range cases { + t.Run(tt.Name, func(t *testing.T) { + require := require.New(t) + mc := tt.MetricsConfig + + actual, err := mc.shouldRunMergedMetricsServer(*tt.Pod(minimal())) + + require.Equal(tt.Expected, actual) + require.NoError(err) + }) + } +} + +// Tests determineAndValidatePort, which in turn tests the +// prometheusScrapePort() and mergedMetricsPort() functions because their logic +// is just to call out to determineAndValidatePort(). +func TestMetricsConfigDetermineAndValidatePort(t *testing.T) { + cases := []struct { + Name string + Pod func(*corev1.Pod) *corev1.Pod + Annotation string + Privileged bool + DefaultPort string + Expected string + Err string + }{ + { + Name: "Valid annotation", + Pod: func(pod *corev1.Pod) *corev1.Pod { + pod.Annotations["consul.hashicorp.com/test-annotation-port"] = "1234" + return pod + }, + Annotation: "consul.hashicorp.com/test-annotation-port", + Privileged: false, + Expected: "1234", + Err: "", + }, + { + Name: "Uses default when there's no annotation", + Pod: func(pod *corev1.Pod) *corev1.Pod { + return pod + }, + Annotation: "consul.hashicorp.com/test-annotation-port", + Privileged: false, + DefaultPort: "4321", + Expected: "4321", + Err: "", + }, + { + Name: "Gets the value of the named default port when there's no annotation", + Pod: func(pod *corev1.Pod) *corev1.Pod { + pod.Spec.Containers[0].Ports = []corev1.ContainerPort{ + { + Name: "web-port", + ContainerPort: 2222, + }, + } + return pod + }, + Annotation: "consul.hashicorp.com/test-annotation-port", + Privileged: false, + DefaultPort: "web-port", + Expected: "2222", + Err: "", + }, + { + Name: "Errors if the named default port doesn't exist on the pod", + Pod: func(pod *corev1.Pod) *corev1.Pod { + return pod + }, + Annotation: "consul.hashicorp.com/test-annotation-port", + Privileged: false, + DefaultPort: "web-port", + Expected: "", + Err: "web-port is not a valid port on the pod minimal", + }, + { + Name: "Gets the value of the named port", + Pod: func(pod *corev1.Pod) *corev1.Pod { + pod.Annotations["consul.hashicorp.com/test-annotation-port"] = "web-port" + pod.Spec.Containers[0].Ports = []corev1.ContainerPort{ + { + Name: "web-port", + ContainerPort: 2222, + }, + } + return pod + }, + Annotation: "consul.hashicorp.com/test-annotation-port", + Privileged: false, + DefaultPort: "4321", + Expected: "2222", + Err: "", + }, + { + Name: "Invalid annotation (not an integer)", + Pod: func(pod *corev1.Pod) *corev1.Pod { + pod.Annotations["consul.hashicorp.com/test-annotation-port"] = "not-an-int" + return pod + }, + Annotation: "consul.hashicorp.com/test-annotation-port", + Privileged: false, + Expected: "", + Err: "consul.hashicorp.com/test-annotation-port annotation value of not-an-int is not a valid integer", + }, + { + Name: "Invalid annotation (integer not in port range)", + Pod: func(pod *corev1.Pod) *corev1.Pod { + pod.Annotations["consul.hashicorp.com/test-annotation-port"] = "100000" + return pod + }, + Annotation: "consul.hashicorp.com/test-annotation-port", + Privileged: true, + Expected: "", + Err: "consul.hashicorp.com/test-annotation-port annotation value of 100000 is not in the valid port range 1-65535", + }, + { + Name: "Invalid annotation (integer not in unprivileged port range)", + Pod: func(pod *corev1.Pod) *corev1.Pod { + pod.Annotations["consul.hashicorp.com/test-annotation-port"] = "22" + return pod + }, + Annotation: "consul.hashicorp.com/test-annotation-port", + Privileged: false, + Expected: "", + Err: "consul.hashicorp.com/test-annotation-port annotation value of 22 is not in the unprivileged port range 1024-65535", + }, + { + Name: "Privileged ports allowed", + Pod: func(pod *corev1.Pod) *corev1.Pod { + pod.Annotations["consul.hashicorp.com/test-annotation-port"] = "22" + return pod + }, + Annotation: "consul.hashicorp.com/test-annotation-port", + Privileged: true, + Expected: "22", + Err: "", + }, + } + + for _, tt := range cases { + t.Run(tt.Name, func(t *testing.T) { + require := require.New(t) + + actual, err := determineAndValidatePort(*tt.Pod(minimal()), tt.Annotation, tt.DefaultPort, tt.Privileged) + + if tt.Err == "" { + require.NoError(err) + require.Equal(tt.Expected, actual) + } else { + require.EqualError(err, tt.Err) + } + }) + } +} + +// Tests mergedMetricsServerConfiguration happy path and error case not covered by other MetricsConfig tests. +func TestMetricsConfigMergedMetricsServerConfiguration(t *testing.T) { + cases := []struct { + Name string + Pod func(*corev1.Pod) *corev1.Pod + MetricsConfig MetricsConfig + ExpectedMergedMetricsPort string + ExpectedServiceMetricsPort string + ExpectedServiceMetricsPath string + ExpErr string + }{ + { + Name: "Returns merged metrics server configuration correctly", + Pod: func(pod *corev1.Pod) *corev1.Pod { + pod.Annotations[annotationPort] = "1234" + return pod + }, + MetricsConfig: MetricsConfig{ + DefaultEnableMetrics: true, + DefaultEnableMetricsMerging: true, + DefaultMergedMetricsPort: "12345", + }, + ExpectedMergedMetricsPort: "12345", + ExpectedServiceMetricsPort: "1234", + ExpectedServiceMetricsPath: "/metrics", + }, + { + Name: "Returns an error when merged metrics server shouldn't run", + Pod: func(pod *corev1.Pod) *corev1.Pod { + pod.Annotations[annotationPort] = "0" + return pod + }, + MetricsConfig: MetricsConfig{ + DefaultEnableMetrics: true, + DefaultEnableMetricsMerging: false, + }, + ExpErr: "metrics merging should be enabled in order to return the metrics server configuration", + }, + } + + for _, tt := range cases { + t.Run(tt.Name, func(t *testing.T) { + require := require.New(t) + mc := tt.MetricsConfig + + metricsPorts, err := mc.mergedMetricsServerConfiguration(*tt.Pod(minimal())) + + if tt.ExpErr != "" { + require.Equal(tt.ExpErr, err.Error()) + } else { + require.NoError(err) + require.Equal(tt.ExpectedMergedMetricsPort, metricsPorts.mergedPort) + require.Equal(tt.ExpectedServiceMetricsPort, metricsPorts.servicePort) + require.Equal(tt.ExpectedServiceMetricsPath, metricsPorts.servicePath) + } + }) + } +} + +func minimal() *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "minimal", + Annotations: map[string]string{ + annotationService: "foo", + }, + }, + + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "web", + }, + { + Name: "web-side", + }, + }, + }, + } +} diff --git a/subcommand/inject-connect/command.go b/subcommand/inject-connect/command.go index 97e24b6fd7..86f8e8b182 100644 --- a/subcommand/inject-connect/command.go +++ b/subcommand/inject-connect/command.go @@ -406,6 +406,14 @@ func (c *Command) Run(args []string) int { return 1 } + metricsConfig := connectinject.MetricsConfig{ + DefaultEnableMetrics: c.flagDefaultEnableMetrics, + DefaultEnableMetricsMerging: c.flagDefaultEnableMetricsMerging, + DefaultMergedMetricsPort: c.flagDefaultMergedMetricsPort, + DefaultPrometheusScrapePort: c.flagDefaultPrometheusScrapePort, + DefaultPrometheusScrapePath: c.flagDefaultPrometheusScrapePath, + } + if err = (&connectinject.EndpointsController{ Client: mgr.GetClient(), ConsulClient: c.consulClient, @@ -417,6 +425,7 @@ func (c *Command) Run(args []string) int { Scheme: mgr.GetScheme(), ReleaseName: c.flagReleaseName, ReleaseNamespace: c.flagReleaseNamespace, + MetricsConfig: metricsConfig, Context: ctx, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", connectinject.EndpointsController{}) @@ -427,33 +436,29 @@ func (c *Command) Run(args []string) int { mgr.GetWebhookServer().Register("/mutate", &webhook.Admission{Handler: &connectinject.Handler{ - ConsulClient: c.consulClient, - ImageConsul: c.flagConsulImage, - ImageEnvoy: c.flagEnvoyImage, - EnvoyExtraArgs: c.flagEnvoyExtraArgs, - ImageConsulK8S: c.flagConsulK8sImage, - RequireAnnotation: !c.flagDefaultInject, - AuthMethod: c.flagACLAuthMethod, - ConsulCACert: string(consulCACert), - DefaultProxyCPURequest: sidecarProxyCPURequest, - DefaultProxyCPULimit: sidecarProxyCPULimit, - DefaultProxyMemoryRequest: sidecarProxyMemoryRequest, - DefaultProxyMemoryLimit: sidecarProxyMemoryLimit, - DefaultEnableMetrics: c.flagDefaultEnableMetrics, - DefaultEnableMetricsMerging: c.flagDefaultEnableMetricsMerging, - DefaultMergedMetricsPort: c.flagDefaultMergedMetricsPort, - DefaultPrometheusScrapePort: c.flagDefaultPrometheusScrapePort, - DefaultPrometheusScrapePath: c.flagDefaultPrometheusScrapePath, - InitContainerResources: initResources, - ConsulSidecarResources: consulSidecarResources, - EnableNamespaces: c.flagEnableNamespaces, - AllowK8sNamespacesSet: allowK8sNamespaces, - DenyK8sNamespacesSet: denyK8sNamespaces, - ConsulDestinationNamespace: c.flagConsulDestinationNamespace, - EnableK8SNSMirroring: c.flagEnableK8SNSMirroring, - K8SNSMirroringPrefix: c.flagK8SNSMirroringPrefix, - CrossNamespaceACLPolicy: c.flagCrossNamespaceACLPolicy, - Log: logger.Named("handler"), + ConsulClient: c.consulClient, + ImageConsul: c.flagConsulImage, + ImageEnvoy: c.flagEnvoyImage, + EnvoyExtraArgs: c.flagEnvoyExtraArgs, + ImageConsulK8S: c.flagConsulK8sImage, + RequireAnnotation: !c.flagDefaultInject, + AuthMethod: c.flagACLAuthMethod, + ConsulCACert: string(consulCACert), + DefaultProxyCPURequest: sidecarProxyCPURequest, + DefaultProxyCPULimit: sidecarProxyCPULimit, + DefaultProxyMemoryRequest: sidecarProxyMemoryRequest, + DefaultProxyMemoryLimit: sidecarProxyMemoryLimit, + InitContainerResources: initResources, + ConsulSidecarResources: consulSidecarResources, + EnableNamespaces: c.flagEnableNamespaces, + AllowK8sNamespacesSet: allowK8sNamespaces, + DenyK8sNamespacesSet: denyK8sNamespaces, + ConsulDestinationNamespace: c.flagConsulDestinationNamespace, + EnableK8SNSMirroring: c.flagEnableK8SNSMirroring, + K8SNSMirroringPrefix: c.flagK8SNSMirroringPrefix, + CrossNamespaceACLPolicy: c.flagCrossNamespaceACLPolicy, + MetricsConfig: metricsConfig, + Log: logger.Named("handler"), }}) // todo: Add tests in case it's not refactored to not have any signal handling