From d973a847b3a276015e556f8c420a05f1cbbdeaf9 Mon Sep 17 00:00:00 2001 From: Nitya Dhanushkodi Date: Tue, 30 Mar 2021 22:33:40 -0700 Subject: [PATCH] Refactor metrics merging to work with endpoints-controller Previously, all metrics configuration was dealt with in the mutating webhook handler. Now, since service/proxy registration happens in endpoints-controller, some of the metrics configuration needs to be used in endpoints-controller as well. There is still some functionality in the handler that also needs metrics configuration, such as adding prometheus annotations and running the consul sidecar. This refactor pulls out common configuration for metrics into an InjectConfiguration struct and adds the methods for getting values from flags and annotations to that struct, so it can be commonly used between endpoints-controller and the webhook handler. --- connect-inject/common_inject_configuration.go | 160 ++++++ .../common_inject_configuration_test.go | 445 +++++++++++++++++ connect-inject/consul_sidecar.go | 10 +- connect-inject/consul_sidecar_test.go | 10 +- connect-inject/container_init.go | 24 +- connect-inject/endpoints_controller.go | 25 +- connect-inject/handler.go | 169 +------ connect-inject/handler_test.go | 467 +----------------- subcommand/inject-connect/command.go | 59 ++- 9 files changed, 706 insertions(+), 663 deletions(-) create mode 100644 connect-inject/common_inject_configuration.go create mode 100644 connect-inject/common_inject_configuration_test.go diff --git a/connect-inject/common_inject_configuration.go b/connect-inject/common_inject_configuration.go new file mode 100644 index 0000000000..87e2bdfd8a --- /dev/null +++ b/connect-inject/common_inject_configuration.go @@ -0,0 +1,160 @@ +package connectinject + +import ( + "fmt" + "strconv" + + corev1 "k8s.io/api/core/v1" +) + +type InjectConfiguration struct { + DefaultEnableMetrics bool + DefaultEnableMetricsMerging bool + DefaultMergedMetricsPort string + DefaultPrometheusScrapePort string + DefaultPrometheusScrapePath string +} + +// enableMetrics returns the default value in the handler, or overrides that +// with the annotation if provided. +func (ic InjectConfiguration) enableMetrics(pod corev1.Pod) (bool, error) { + enabled := ic.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 (ic InjectConfiguration) enableMetricsMerging(pod corev1.Pod) (bool, error) { + enabled := ic.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 (ic InjectConfiguration) mergedMetricsPort(pod corev1.Pod) (string, error) { + return determineAndValidatePort(pod, annotationMergedMetricsPort, ic.DefaultMergedMetricsPort, false) +} + +// prometheusScrapePort returns the default value in the handler, or overrides +// that with the annotation if provided. +func (ic InjectConfiguration) prometheusScrapePort(pod corev1.Pod) (string, error) { + return determineAndValidatePort(pod, annotationPrometheusScrapePort, ic.DefaultPrometheusScrapePort, false) +} + +// prometheusScrapePath returns the default value in the handler, or overrides +// that with the annotation if provided. +func (ic InjectConfiguration) prometheusScrapePath(pod corev1.Pod) string { + if raw, ok := pod.Annotations[annotationPrometheusScrapePath]; ok && raw != "" { + return raw + } + + return ic.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 (ic InjectConfiguration) 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 (ic InjectConfiguration) 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 (ic InjectConfiguration) shouldRunMergedMetricsServer(pod corev1.Pod) (bool, error) { + enableMetrics, err := ic.enableMetrics(pod) + if err != nil { + return false, err + } + enableMetricsMerging, err := ic.enableMetricsMerging(pod) + if err != nil { + return false, err + } + serviceMetricsPort, err := ic.serviceMetricsPort(pod) + if err != nil { + return false, err + } + + // Don't need to check error here since serviceMetricsPort has been + // validated by calling ic.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/common_inject_configuration_test.go b/connect-inject/common_inject_configuration_test.go new file mode 100644 index 0000000000..18b3a80ffa --- /dev/null +++ b/connect-inject/common_inject_configuration_test.go @@ -0,0 +1,445 @@ +package connectinject + +import ( + "testing" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" +) + +func TestInjectConfigurationEnableMetrics(t *testing.T) { + cases := []struct { + Name string + Pod func(*corev1.Pod) *corev1.Pod + InjectConfiguration InjectConfiguration + Expected bool + Err string + }{ + { + Name: "Metrics enabled via handler", + Pod: func(pod *corev1.Pod) *corev1.Pod { + return pod + }, + InjectConfiguration: InjectConfiguration{ + DefaultEnableMetrics: true, + }, + Expected: true, + Err: "", + }, + { + Name: "Metrics enabled via annotation", + Pod: func(pod *corev1.Pod) *corev1.Pod { + pod.Annotations[annotationEnableMetrics] = "true" + return pod + }, + InjectConfiguration: InjectConfiguration{ + 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 + }, + InjectConfiguration: InjectConfiguration{ + 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) + ic := tt.InjectConfiguration + + actual, err := ic.enableMetrics(*tt.Pod(minimal())) + + if tt.Err == "" { + require.Equal(tt.Expected, actual) + require.NoError(err) + } else { + require.EqualError(err, tt.Err) + } + }) + } +} + +func TestInjectConfigurationEnableMetricsMerging(t *testing.T) { + cases := []struct { + Name string + Pod func(*corev1.Pod) *corev1.Pod + InjectConfiguration InjectConfiguration + Expected bool + Err string + }{ + { + Name: "Metrics merging enabled via handler", + Pod: func(pod *corev1.Pod) *corev1.Pod { + return pod + }, + InjectConfiguration: InjectConfiguration{ + DefaultEnableMetricsMerging: true, + }, + Expected: true, + Err: "", + }, + { + Name: "Metrics merging enabled via annotation", + Pod: func(pod *corev1.Pod) *corev1.Pod { + pod.Annotations[annotationEnableMetricsMerging] = "true" + return pod + }, + InjectConfiguration: InjectConfiguration{ + 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 + }, + InjectConfiguration: InjectConfiguration{ + 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) + ic := tt.InjectConfiguration + + actual, err := ic.enableMetricsMerging(*tt.Pod(minimal())) + + if tt.Err == "" { + require.Equal(tt.Expected, actual) + require.NoError(err) + } else { + require.EqualError(err, tt.Err) + } + }) + } +} + +func TestInjectConfigurationServiceMetricsPort(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) + ic := InjectConfiguration{} + + actual, err := ic.serviceMetricsPort(*tt.Pod(minimal())) + + require.Equal(tt.Expected, actual) + require.NoError(err) + }) + } +} + +func TestInjectConfigurationServiceMetricsPath(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) + ic := InjectConfiguration{} + + actual := ic.serviceMetricsPath(*tt.Pod(minimal())) + + require.Equal(tt.Expected, actual) + }) + } +} + +func TestInjectConfigurationPrometheusScrapePath(t *testing.T) { + cases := []struct { + Name string + Pod func(*corev1.Pod) *corev1.Pod + InjectConfiguration InjectConfiguration + Expected string + }{ + { + Name: "Defaults to the handler's value", + Pod: func(pod *corev1.Pod) *corev1.Pod { + return pod + }, + InjectConfiguration: InjectConfiguration{ + 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 + }, + InjectConfiguration: InjectConfiguration{ + 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) + ic := tt.InjectConfiguration + + actual := ic.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 TestInjectConfigurationShouldRunMergedMetricsServer(t *testing.T) { + cases := []struct { + Name string + Pod func(*corev1.Pod) *corev1.Pod + InjectConfiguration InjectConfiguration + 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 + }, + InjectConfiguration: InjectConfiguration{ + 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 + }, + InjectConfiguration: InjectConfiguration{ + DefaultEnableMetrics: true, + DefaultEnableMetricsMerging: true, + }, + Expected: false, + }, + } + + for _, tt := range cases { + t.Run(tt.Name, func(t *testing.T) { + require := require.New(t) + ic := tt.InjectConfiguration + + actual, err := ic.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 TestInjectConfigurationDetermineAndValidatePort(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) + } + }) + } +} diff --git a/connect-inject/consul_sidecar.go b/connect-inject/consul_sidecar.go index 5f1477d222..f0b2958533 100644 --- a/connect-inject/consul_sidecar.go +++ b/connect-inject/consul_sidecar.go @@ -12,7 +12,7 @@ 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) + run, err := h.InjectConfig.shouldRunMergedMetricsServer(pod) if err != nil { return corev1.Container{}, err } @@ -24,15 +24,15 @@ func (h *Handler) consulSidecar(pod corev1.Pod) (corev1.Container, error) { } // Configure consul sidecar with the appropriate metrics flags. - mergedMetricsPort, err := h.mergedMetricsPort(pod) + mergedMetricsPort, err := h.InjectConfig.mergedMetricsPort(pod) if err != nil { return corev1.Container{}, err } - serviceMetricsPath := h.serviceMetricsPath(pod) + serviceMetricsPath := h.InjectConfig.serviceMetricsPath(pod) // Don't need to check the error since it's checked in the call to - // h.shouldRunMergedMetricsServer() above. - serviceMetricsPort, _ := h.serviceMetricsPort(pod) + // h.InjectConfig.shouldRunMergedMetricsServer() above. + serviceMetricsPort, _ := h.InjectConfig.serviceMetricsPort(pod) command := []string{ "consul-k8s", diff --git a/connect-inject/consul_sidecar_test.go b/connect-inject/consul_sidecar_test.go index 0c0aad7aa6..c3b40574fe 100644 --- a/connect-inject/consul_sidecar_test.go +++ b/connect-inject/consul_sidecar_test.go @@ -31,10 +31,12 @@ var ( // 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", + InjectConfig: InjectConfiguration{ + DefaultEnableMetrics: true, + DefaultEnableMetricsMerging: true, + }, } container, err := handler.consulSidecar(corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ diff --git a/connect-inject/container_init.go b/connect-inject/container_init.go index 37b38c12da..22e0d6518b 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) + run, err := h.InjectConfig.shouldRunMergedMetricsServer(pod) if err != nil { return corev1.Container{}, err } if run { - prometheusScrapePath := h.prometheusScrapePath(pod) - mergedMetricsPort, err := h.mergedMetricsPort(pod) + prometheusScrapePath := h.InjectConfig.prometheusScrapePath(pod) + mergedMetricsPort, err := h.InjectConfig.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..d9bcdd3031 100644 --- a/connect-inject/endpoints_controller.go +++ b/connect-inject/endpoints_controller.go @@ -47,6 +47,7 @@ type EndpointsController struct { ReleaseName string // ReleaseNamespace is the namespace where Consul is installed. ReleaseNamespace string + InjectConfig InjectConfiguration Log logr.Logger Scheme *runtime.Scheme context.Context @@ -225,7 +226,29 @@ 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: nil, // TODO: add config for metrics (upcoming PR) + } + + // TODO fixup comment and extract into method and write tests + // 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 := r.InjectConfig.enableMetrics(pod) + if err != nil { + return nil, nil, err + } + prometheusScrapePort, err := r.InjectConfig.prometheusScrapePort(pod) + if err != nil { + return nil, nil, err + } + prometheusScrapeListener := fmt.Sprintf("0.0.0.0:%s", prometheusScrapePort) + if enableMetrics { + proxyConfig.Config = map[string]interface{}{ + "envoy_prometheus_bind_addr": prometheusScrapeListener, + } } if servicePort > 0 { diff --git a/connect-inject/handler.go b/connect-inject/handler.go index 56e6350022..c0c71bc4ee 100644 --- a/connect-inject/handler.go +++ b/connect-inject/handler.go @@ -116,11 +116,11 @@ type Handler struct { // 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 + //DefaultEnableMetrics bool + //DefaultEnableMetricsMerging bool + //DefaultMergedMetricsPort string + //DefaultPrometheusScrapePort string + //DefaultPrometheusScrapePath string // Resource settings for init container. All of these fields // will be populated by the defaults provided in the initial flags. @@ -130,6 +130,8 @@ type Handler struct { // will be populated by the defaults provided in the initial flags. ConsulSidecarResources corev1.ResourceRequirements + InjectConfig InjectConfiguration + // Log Log hclog.Logger @@ -218,7 +220,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.InjectConfig.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 +354,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.InjectConfig.enableMetrics(*pod) if err != nil { return err } - prometheusScrapePort, err := h.prometheusScrapePort(*pod) + prometheusScrapePort, err := h.InjectConfig.prometheusScrapePort(*pod) if err != nil { return err } - prometheusScrapePath := h.prometheusScrapePath(*pod) + prometheusScrapePath := h.InjectConfig.prometheusScrapePath(*pod) if enableMetrics { pod.Annotations[annotationPrometheusScrape] = "true" @@ -456,68 +375,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..ba29ec4f71 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(), + InjectConfig: InjectConfiguration{ + DefaultEnableMetrics: true, + DefaultEnableMetricsMerging: true, + }, + decoder: decoder, }, admission.Request{ AdmissionRequest: admissionv1.AdmissionRequest{ @@ -663,252 +665,6 @@ func minimal() *corev1.Pod { } } -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 +674,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", + InjectConfig: InjectConfiguration{ + DefaultEnableMetrics: true, + DefaultPrometheusScrapePort: "20200", + DefaultPrometheusScrapePath: "/metrics", + }, }, Expected: map[string]string{ annotationPrometheusScrape: "true", @@ -931,9 +689,11 @@ func TestHandlerPrometheusAnnotations(t *testing.T) { { Name: "Does not set annotations if metrics are not enabled", Handler: Handler{ - DefaultEnableMetrics: false, - DefaultPrometheusScrapePort: "20200", - DefaultPrometheusScrapePath: "/metrics", + InjectConfig: InjectConfiguration{ + DefaultEnableMetrics: false, + DefaultPrometheusScrapePort: "20200", + DefaultPrometheusScrapePath: "/metrics", + }, }, Expected: map[string]string{}, }, @@ -953,197 +713,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/subcommand/inject-connect/command.go b/subcommand/inject-connect/command.go index 97e24b6fd7..6c5e48e386 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 } + injectConfig := connectinject.InjectConfiguration{ + 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, + InjectConfig: injectConfig, 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, + InjectConfig: injectConfig, + Log: logger.Named("handler"), }}) // todo: Add tests in case it's not refactored to not have any signal handling