Skip to content

[CC-4361] Add HCPMetricsBindSocketDir to Envoy bootstrap config#90

Merged
Achooo merged 4 commits into
mainfrom
cc-4361/hcp-metrics-bootstrap-config
Mar 14, 2023
Merged

[CC-4361] Add HCPMetricsBindSocketDir to Envoy bootstrap config#90
Achooo merged 4 commits into
mainfrom
cc-4361/hcp-metrics-bootstrap-config

Conversation

@Achooo
Copy link
Copy Markdown
Contributor

@Achooo Achooo commented Mar 7, 2023

This PR is very similar to hashicorp/consul#16511 in consul

Background

The HCP Cloud team is working on enabling observability metrics in HCP.
In particular, we want to forward Envoy metrics.

This PR aims to allow Envoy bootstrap configuration of 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.

Code changes

  • internal/bootstrap/bootstrap_config.go : Add a HCPMetricsBindSocketDir string type in the BootstrapConfig struct and a function appendHCPMetricsConfig to generate the socket name, configure the stats sink and cluster for metrics forwarding.
  • internal/bootstrap/bootstrap_config_test.go: add relevant tests to the above
  • pkg/consuldp/bootstrap_test.go: Add a test to generate a golden test file
  • internal/bootstrap/bootstrap_tpl.go : small change to move brackets [] into the template instead, which facilitates config generation

Next Steps

There will be a follow-up PR to generate a listener and cluster dynamically that will be listening on this 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

@Achooo Achooo force-pushed the cc-4361/hcp-metrics-bootstrap-config branch from 959a65e to c6f02df Compare March 7, 2023 19:40
@freddygv freddygv requested review from a team, hashi-derek and zalimeni and removed request for a team March 7, 2023 20:45
@DanStough
Copy link
Copy Markdown
Contributor

As we try to add support for VMs to consul-dataplane in the future, I believe using a Unix socket going to be problematic for Windows. Is this a cause for concern?

@Achooo
Copy link
Copy Markdown
Contributor Author

Achooo commented Mar 8, 2023

After some discussion, it looks like Envoy only supports at least Windows 10, see here, which supports unix sockets, here is the go support.

We'd like to move forward with this, revisit and test as needed when Windows support is in the works. We can always provide an alternative configuration mode for Windows in the worst case if the need arises.

Copy link
Copy Markdown
Contributor

@hashi-derek hashi-derek left a comment

Choose a reason for hiding this comment

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

This looks good after having a discussion with Freddy about the feature. Just putting some minor nits here that you don't necessarily have to address.

Comment thread internal/bootstrap/bootstrap_config_test.go Outdated
{{- if .StatsSinksJSON }}
"stats_sinks": {{ .StatsSinksJSON }},
"stats_sinks": [
{{ .StatsSinksJSON }}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rather than defining this as a single value and worrying about concatenating commas in the go code, couldn't we make this variable a slice of strings and do the , joining in the template?

Copy link
Copy Markdown
Contributor Author

@Achooo Achooo Mar 9, 2023

Choose a reason for hiding this comment

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

Yeah I agree the current state isn't ideal... I'd prefer to park this scope-wise!

I believe I'd have to convert the actual BootstrapConfig StatsConfigJSON to a list internally due to these lines

if c.StatsConfigJSON != "" {
// StatsConfig overridden explicitly
args.StatsConfigJSON = c.StatsConfigJSON
, where the args have the actual config reconciled. To convert it externally, this might be a breaking change as it is configurable?

These changes would also affect most test and quite a few areas of the code in bootstrap_config.go and boostrap_config_test.go.

My initial thought seeing this package was maybe there might be a better cleanup that could be done here using marshalling/unmarshalling, although this introduces with defining structs/models, it might reduce the code logic complexity and cleanup a most of these handlings in the code.

But yeah I totally agree the newline handling etc. is not super pretty, I'm also happy to maybe make a follow up PR if that works?

Comment thread internal/bootstrap/bootstrap_config.go Outdated
dir += "/"
}

path := fmt.Sprintf("%s%s_%s.sock",dir,args.Namespace,args.ProxyID)
Copy link
Copy Markdown
Contributor Author

@Achooo Achooo Mar 9, 2023

Choose a reason for hiding this comment

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

After testing locally with @freddygv , looks like the Namespace is always default, so we remove the if statement.

We discussed having a safety check to that normalizes to default (e.g. Consul equivalent) but removed it for code simplicity.

@Achooo Achooo marked this pull request as ready for review March 10, 2023 13:19
@Achooo Achooo requested a review from a team as a code owner March 10, 2023 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/1.1 Changes are backported to 1.1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants