Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

inject envoy_telemetry_bind_socket_dir proxy config when telemetry collector is enabled #2143

Merged
merged 3 commits into from May 30, 2023
Merged

inject envoy_telemetry_bind_socket_dir proxy config when telemetry collector is enabled #2143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a799ad
inject envoy_telemetry_bind_socket_dir proxy config when telemetry co…
nickethier May 17, 2023
9a95c9f
use metrics.enableTelemetryCollector value to gate controller logic
nickethier May 30, 2023
af05804
add changelog entry and unit test
nickethier May 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions .changelog/2143.txt
View file
Open in desktop
@@ -0,0 +1,4 @@

```release-note:feature
consul-telemetry-collector: Configure envoy proxy config during registration when consul-telemetry-collector is enabled.
```
1 change: 1 addition & 0 deletions charts/consul/templates/connect-inject-deployment.yaml
View file
Open in desktop
Expand Up @@ -257,6 +257,7 @@ spec:
{{- if and .Values.global.tls.enabled .Values.global.tls.enableAutoEncrypt }}
-enable-auto-encrypt \
{{- end }}
-enable-telemetry-collector={{ .Values.global.metrics.enableTelemetryCollector}} \
startupProbe:
httpGet:
path: /readyz/ready
Expand Down
13 changes: 13 additions & 0 deletions charts/consul/test/unit/connect-inject-deployment.bats
View file
Open in desktop
Expand Up @@ -211,6 +211,19 @@ load _helpers
[ "${actual}" = "true" ]
}

@test "connectInject/Deployment: metrics.enableTelemetryCollector can be configured" {
cd `chart_dir`
local cmd=$(helm template \
-s templates/connect-inject-deployment.yaml \
--set 'connectInject.enabled=true' \
--set 'connectInject.metrics.enableTelemetryCollector=true' \
. | tee /dev/stderr |
yq '.spec.template.spec.containers[0].command' | tee /dev/stderr)

local actual=$(echo "$cmd" |
yq 'any(contains("-enable-telemetry-collector=true"))' | tee /dev/stderr)
[ "${actual}" = "true" ]
}
#--------------------------------------------------------------------
# consul and consul-dataplane images

Expand Down
19 changes: 16 additions & 3 deletions control-plane/connect-inject/controllers/endpoints/endpoints_controller.go
View file
Open in desktop
Expand Up @@ -44,9 +44,10 @@ const (
terminatingGateway = "terminating-gateway"
ingressGateway = "ingress-gateway"

kubernetesSuccessReasonMsg = "Kubernetes health checks passing"
envoyPrometheusBindAddr = "envoy_prometheus_bind_addr"
defaultNS = "default"
kubernetesSuccessReasonMsg = "Kubernetes health checks passing"
envoyPrometheusBindAddr = "envoy_prometheus_bind_addr"
envoyTelemetryCollectorBindSocketDir = "envoy_telemetry_collector_bind_socket_dir"
defaultNS = "default"

// clusterIPTaggedAddressName is the key for the tagged address to store the service's cluster IP and service port
// in Consul. Note: This value should not be changed without a corresponding change in Consul.
Expand Down Expand Up @@ -119,6 +120,10 @@ type Controller struct {
// to Consul client agents.
EnableAutoEncrypt bool

// EnableTelemetryCollector controls whether the proxy service should be registered
// with config to enable telemetry forwarding.
EnableTelemetryCollector bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to be toggleable on a per pod basis through annotations? Most of our features allow you to turn a feature on globally through the helm chart or per pod using annotations.

An example, is how we check if metrics is enabled on the annotation (https://github.com/hashicorp/consul-k8s/blob/main/control-plane/connect-inject/metrics/metrics_configuration.go#L72)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats an interesting idea. I guess if I was operating a cluster I wouldn't want consumers to be able to disable telemetry. We currently assume the feature is either enabled or disabled cluster wide.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be up for exploring adding this in the future though.


MetricsConfig metrics.Config
Log logr.Logger

Expand Down Expand Up @@ -482,6 +487,10 @@ func (r *Controller) createServiceRegistrations(pod corev1.Pod, serviceEndpoints
proxyConfig.Config[envoyPrometheusBindAddr] = prometheusScrapeListener
}

if r.EnableTelemetryCollector {
proxyConfig.Config[envoyTelemetryCollectorBindSocketDir] = "/consul/connect-inject"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker but I am thinking that maybe the directory should be /consul/telemetry-collector ? Doesn't it create a .sock file?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does If we want to create a dedicated emptyDir volume for it we can. I figured it was a bit heavy handed given we already have a dir for connect related coordination.

}

if consulServicePort > 0 {
proxyConfig.LocalServiceAddress = "127.0.0.1"
proxyConfig.LocalServicePort = consulServicePort
Expand Down Expand Up @@ -761,6 +770,10 @@ func (r *Controller) createGatewayRegistrations(pod corev1.Pod, serviceEndpoints
}
}

if r.EnableTelemetryCollector {
service.Proxy.Config[envoyTelemetryCollectorBindSocketDir] = "/consul/service"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above. Maybe /consul/telemetry-collector?

}

serviceRegistration := &api.CatalogRegistration{
Node: common.ConsulNodeNameFromK8sNode(pod.Spec.NodeName),
Address: pod.Status.HostIP,
Expand Down
6 changes: 6 additions & 0 deletions control-plane/subcommand/inject-connect/command.go
View file
Open in desktop
Expand Up @@ -118,6 +118,9 @@ type Command struct {

flagEnableAutoEncrypt bool

// Consul telemetry collector
flagEnableTelemetryCollector bool

// Consul DNS flags.
flagEnableConsulDNS bool
flagResourcePrefix string
Expand Down Expand Up @@ -203,6 +206,8 @@ func (c *Command) init() {
"Enables updating the CABundle on the webhook within this controller rather than using the web cert manager.")
c.flagSet.BoolVar(&c.flagEnableAutoEncrypt, "enable-auto-encrypt", false,
"Indicates whether TLS with auto-encrypt should be used when talking to Consul clients.")
c.flagSet.BoolVar(&c.flagEnableTelemetryCollector, "enable-telemetry-collector", false,
"Indicates whether proxies should be registered with configuration to enable forwarding metrics to consul-telemetry-collector")
c.flagSet.StringVar(&c.flagLogLevel, "log-level", zapcore.InfoLevel.String(),
fmt.Sprintf("Log verbosity level. Supported values (in order of detail) are "+
"%q, %q, %q, and %q.", zapcore.DebugLevel.String(), zapcore.InfoLevel.String(), zapcore.WarnLevel.String(), zapcore.ErrorLevel.String()))
Expand Down Expand Up @@ -449,6 +454,7 @@ func (c *Command) Run(args []string) int {
ReleaseName: c.flagReleaseName,
ReleaseNamespace: c.flagReleaseNamespace,
EnableAutoEncrypt: c.flagEnableAutoEncrypt,
EnableTelemetryCollector: c.flagEnableTelemetryCollector,
Context: ctx,
}).SetupWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create controller", "controller", endpoints.Controller{})
Expand Down