Allow HCP metrics collection for Envoy proxies by Achooo · Pull Request #16511 · hashicorp/consul

Achooo · 2023-03-02T20:08:31Z

Description

Add a new envoy flag: envoy_hcp_metrics_bind_socket_dir, a directory where a unix socket will be created with the name <namespace>_<proxy_id>.sock to forward Envoy metrics.
If set, this will configure a local stats_sink and STATIC cluster to forward Envoy metrics to an HCP collector.

The HCP Cloud team is working on enabling observability metrics in HCP.

There will be a follow-up PR to generate a listener and cluster dynamically that will be listening on this unix socket.
That listener and cluster will be configured to route to collector instances through the service mesh.

Caveat: we needed to add this local listener indirection as a workaround due to this issue

Testing & Reproduction steps

Added unit tests
Tested locally by running consul, registering a proxy with the flag configured and starting Envoy with consul connect command.

PR Checklist

updated test coverage
external facing docs updated
not a security concern

freddygv · 2023-03-02T20:14:22Z

FYI @DanStough @wilkermichael we're thinking of holding off on merging this until after next week's patch releases.

The follow-up PR will be chained off of this one and will have the changelog entry.

Achooo · 2023-03-02T21:25:36Z


 	if len(stats_sinks) > 0 {
-		args.StatsSinksJSON = "[\n" + strings.Join(stats_sinks, ",\n") + "\n]"
+		args.StatsSinksJSON = strings.Join(stats_sinks, ",\n")


Moved the [ and ] to the template in order to add the HCP metrics sink and cluster configuration in the upper level function.

Achooo · 2023-03-02T21:26:08Z

 			wantArgs: BootstrapTplArgs{
 				StatsConfigJSON: defaultStatsConfigJSON,
-				StatsSinksJSON: `[{
+				StatsSinksJSON: `{


The tests below just remove the [ and ] which are now in the template, see this comment

Achooo · 2023-03-02T21:26:23Z

  },
  {{- if .StatsSinksJSON }}
-  "stats_sinks": {{ .StatsSinksJSON }},
+  "stats_sinks": [


Move the brackets into the template file

Related to https://github.com/hashicorp/consul/pull/16511/files#r1123725950

Achooo · 2023-03-02T21:27:45Z

 			return nil, fmt.Errorf("failed parsing Proxy.Config: %s", err)
 		}
+
+		if bsCfg.HCPMetricsBindPort < 0 || bsCfg.HCPMetricsBindPort > 65535 {


Validate the port

DanStough

LGTM

trujillo-adam

I left a suggestion on the docs, but I'm not convinced that it's the perfect way to describe the param.

trujillo-adam · 2023-03-02T23:54:29Z

+- `envoy_hcp_metrics_bind_port` - By setting this port, Envoy will be configured to
+  send metrics to an HCP collector. By default, this is disabled.


Suggested change

- `envoy_hcp_metrics_bind_port` - By setting this port, Envoy will be configured to

send metrics to an HCP collector. By default, this is disabled.

- `envoy_hcp_metrics_bind_port` - Specifies the port number for Envoy to send metrics to. HCP collectors can collect the metrics at this port. The port is not configured by default.

Attempted to reword this so that we are more concrete about how to configure the param. Better? Worse? No different?

Thank you so much for the review , and so sorry for the delay. We made some changes to our approach, see here. I tried to reword it based on your suggestions - which I liked.

I'm not 💯 on the new changes. Would love to get your feedback on the new version, and maybe @freddygv has thoughts here.

Achooo · 2023-03-07T15:39:29Z

+// appendHCPMetricsConfig generates config to enable a socket at path: <hcpMetricsBindSocketDir>/<namespace>_<proxy_id>.sock
+// or <hcpMetricsBindSocketDir>/<proxy_id>.sock, if namespace is empty.
+func appendHCPMetricsConfig(args *BootstrapTplArgs, hcpMetricsBindSocketDir string) {
+	dir := hcpMetricsBindSocketDir


Handle / addition if needed from user input.

Achooo · 2023-03-07T15:46:23Z

  In cases where either assumption is violated this flag will prevent the
  command attempting to resolve config from the local agent.

+- `envoy_hcp_metrics_bind_socket_dir` - Specifies the directory of a unix socket 


Would Specifies the directory where a unix socket is created be better here?

What creates the directory? The passive voice sounds as if either Envoy or Consul creates the directory. If that's true, then we should say something like "Specifies the directory where <Envoy/Consul> creates a unix socket. Envoy sends metrics to the socket so that HCP collectors can connect collect them."

Fixed that's much better, thank you.

Achooo · 2023-03-07T15:56:17Z

@freddygv had the idea of using a unix socket instead of a port to avoid ports collisions, and after realizing services run on the same container.

PR has been updated to:

Allow configuration of envoy_hcp_metrics_bind_socket_dir, a directory where the socket will be created with the name <namespace>_<proxy_id>.sock
Update to add a HCPMetricsBindSocketDir string type in the BootstrapConfig struct, the stats sink and cluster generation and fix relevant tests.
Remove port validation in envoy.go and update golden file
Update docs with the new flag name

See this commit 3896dfe

Achooo · 2023-03-08T16:03:17Z

See discussion on Windows support concern here: hashicorp/consul-dataplane#90 (comment)

TLDR: We'd like to move forward with this approach, and revisit at a later stage if needed.

freddygv · 2023-03-08T20:56:51Z

Since this is a common situation we have acl.NamespaceOrDefault to handle defaulting:
path := fmt.Sprintf("%s%s_%s.sock", dir, acl.NamespaceOrDefault(args.Namespace), args.ProxyID)

Using this function fails the tests for a set namespace, as they all get normalized to default. I will add a test in enterprise to cover the other namespace case.

loshz · 2023-03-10T17:30:41Z

I would use the path package here:

Suggested change

dir := hcpMetricsBindSocketDir

if !strings.HasSuffix(dir, "/") {

dir += "/"

}

// Normalize namespace to "default". This ensures we match the namespace behaviour in proxycfg package,

// where a dynamic listener will be created at the same socket path via xDS.

path := fmt.Sprintf("%s%s_%s.sock", dir, acl.NamespaceOrDefault(args.Namespace), args.ProxyID)

sock := fmt.Sprintf("%s_%s.sock", acl.NamespaceOrDefault(args.Namespace), args.ProxyID)

// Normalize namespace to "default". This ensures we match the namespace behaviour in proxycfg package,

// where a dynamic listener will be created at the same socket path via xDS.

path := path.Join(hcpMetricsBindSocketDir, sock)

thanks for the suggestion, fixed! That looks prettier 💯

…est cluster

Co-authored-by: Ashvitha Sridharan <ashvitha.sridharan@hashicorp.com> This commit builds on top of the previous one by configuring a dynamic listener at that statically configured socket. The changes below inject the HCP metrics collector as an upstream for connect proxies since it is intended to be a mesh service deployed onto the user's cluster. Why is there dynamic configuration when the cluster is statically defined at bootstrap time? - We want to secure the metrics stream using TLS, but the stats sink can only be defined in bootstrap config. With dynamic listeners/clusters we can use certificates issued by the connect CA, which aren't available at bootstrap time. - We want to intelligently route to the HCP collector. Configuring its address at bootstrap time limits our flexibility routing-wise compared to providing clusters/endpoints dynamically using xDS. More on this below. Why define the collector as an upstream in `proxycfg`? - Service discovery and routing logic is automatically taken care of, meaning that no code changes are required in the `xds` package. - Certificate management is taken care of. Each proxy will dial using the certificate of the proxy it represents, and the HCP collector can present its own certificate as well as check intentions. - Custom routing rules can be added for the collector using discovery chain config entries. Initially the collector is expected to be deployed to each admin partition, but in the future could be deployed centrally in the default partition. These config entries could be managed by HCP itself.

Achooo requested a review from a team March 2, 2023 20:08

Achooo requested a review from a team as a code owner March 2, 2023 20:08

Achooo requested review from DanStough, freddygv and wilkermichael and removed request for a team March 2, 2023 20:08

github-actions Bot added theme/cli Flags and documentation for the CLI interface type/docs Documentation needs to be created/updated/clarified labels Mar 2, 2023

Achooo force-pushed the cc-4361/hcp-metrics-bootstrap-config branch from 3dd9c99 to 655443b Compare March 2, 2023 20:12

vercel Bot deployed to Preview – consul March 2, 2023 20:16 View deployment

Achooo commented Mar 2, 2023

View reviewed changes

DanStough added the pr/no-changelog PR does not need a corresponding .changelog entry label Mar 2, 2023

DanStough approved these changes Mar 2, 2023

View reviewed changes

trujillo-adam reviewed Mar 2, 2023

View reviewed changes

Achooo force-pushed the cc-4361/hcp-metrics-bootstrap-config branch 3 times, most recently from 8dbbeae to 3896dfe Compare March 7, 2023 15:29

vercel Bot deployed to Preview – consul March 7, 2023 15:34 View deployment

Achooo requested a review from DanStough March 7, 2023 15:37

Achooo commented Mar 7, 2023

View reviewed changes

Achooo mentioned this pull request Mar 7, 2023

[CC-4361] Add HCPMetricsBindSocketDir to Envoy bootstrap config hashicorp/consul-dataplane#90

Merged

freddygv reviewed Mar 8, 2023

View reviewed changes

Achooo force-pushed the cc-4361/hcp-metrics-bootstrap-config branch from d5dc932 to 8c332b9 Compare March 8, 2023 21:15

freddygv requested a review from trujillo-adam March 9, 2023 17:03

freddygv mentioned this pull request Mar 9, 2023

[CC-4365] HCP Metrics xDS Config for Envoy #16585

Merged

3 tasks

Achooo force-pushed the cc-4361/hcp-metrics-bootstrap-config branch from 307c579 to 8cda1df Compare March 10, 2023 13:30

Achooo requested a review from hashi-derek March 10, 2023 13:34

hashi-derek approved these changes Mar 10, 2023

View reviewed changes

loshz reviewed Mar 10, 2023

View reviewed changes

vercel Bot deployed to Preview – consul March 10, 2023 19:25 View deployment

Achooo added 5 commits March 10, 2023 14:28

Add HCPMetricsBindPort to Envoy bootstrap config.

8ff4b6f

Switch to socket instead of port

a5498b9

Normalize the namespace to default

f0f7a3b

Address PR improvement comment: refactor constant for test sink and t…

28442ae

…est cluster

Address PR comments

4eb82e1

Achooo force-pushed the cc-4361/hcp-metrics-bootstrap-config branch from 8c65c58 to 4eb82e1 Compare March 10, 2023 19:28

vercel Bot deployed to Preview – consul March 10, 2023 19:33 View deployment

freddygv added the backport/1.15 label Mar 10, 2023

freddygv changed the title ~~[CC-4361] HCP Metrics Boostrap Config for Envoy~~ Allow HCP metrics collection for Envoy proxies Mar 10, 2023

freddygv merged commit f95ffe0 into main Mar 10, 2023

freddygv deleted the cc-4361/hcp-metrics-bootstrap-config branch March 10, 2023 20:52

hc-github-team-consul-core mentioned this pull request Mar 10, 2023

Backport of Allow HCP metrics collection for Envoy proxies into release/1.15.x #16609

Closed

3 tasks

freddygv mentioned this pull request Mar 10, 2023

Backport of Allow HCP metrics collection for Envoy proxies into release/1.15.x #16611

Merged

This was referenced Mar 14, 2023

Backport of [CC-4361] Add HCPMetricsBindSocketDir to Envoy bootstrap config into release/1.0.x hashicorp/consul-dataplane#91

Merged

Backport of [CC-4361] Add HCPMetricsBindSocketDir to Envoy bootstrap config into release/1.1.x hashicorp/consul-dataplane#92

Merged

		- `envoy_hcp_metrics_bind_port` - By setting this port, Envoy will be configured to
		send metrics to an HCP collector. By default, this is disabled.

	- `envoy_hcp_metrics_bind_port` - By setting this port, Envoy will be configured to
	send metrics to an HCP collector. By default, this is disabled.
	- `envoy_hcp_metrics_bind_port` - Specifies the port number for Envoy to send metrics to. HCP collectors can collect the metrics at this port. The port is not configured by default.

-	dir := hcpMetricsBindSocketDir
-	if !strings.HasSuffix(dir, "/") {
-		dir += "/"
-	}
-	// Normalize namespace to "default". This ensures we match the namespace behaviour in proxycfg package,
-	// where a dynamic listener will be created at the same socket path via xDS.
-	path := fmt.Sprintf("%s%s_%s.sock", dir, acl.NamespaceOrDefault(args.Namespace), args.ProxyID)
+	sock := fmt.Sprintf("%s_%s.sock", acl.NamespaceOrDefault(args.Namespace), args.ProxyID)
+	// Normalize namespace to "default". This ensures we match the namespace behaviour in proxycfg package,
+	// where a dynamic listener will be created at the same socket path via xDS.
+	path := path.Join(hcpMetricsBindSocketDir, sock)

Conversation

Achooo commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing & Reproduction steps

PR Checklist

Uh oh!

freddygv commented Mar 2, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Achooo Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Achooo Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanStough left a comment

Choose a reason for hiding this comment

Uh oh!

trujillo-adam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Achooo commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Achooo commented Mar 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loshz Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Achooo commented Mar 2, 2023 •

edited

Loading

Achooo Mar 2, 2023 •

edited

Loading

Achooo Mar 2, 2023 •

edited

Loading

Achooo commented Mar 7, 2023 •

edited

Loading

loshz Mar 10, 2023 •

edited

Loading