From 9b5e89bd34cc58801c9c1d0945dff707367ad43f Mon Sep 17 00:00:00 2001
From: Pratyoy Mukhopadhyay <35388175+pmmukh@users.noreply.github.com>
Date: Fri, 9 Jul 2021 14:49:53 -0500
Subject: [PATCH] [VAULT-2776] Add prefix_filter option to Vault (#12025)

* [VAULT-2776] Add prefix_filter support to vault

* [VAULT-2776] Add filter_default config, update docs

* [VAULT-2776] Add changelog file

* [VAULT-2776] Update telemetry tests and error handling

* [VAULT-2776] Add test fixtures, update test

* [VAULT-2776] Update gitignore hcl filter
---
 .gitignore                                    |  2 +-
 changelog/12025.txt                           |  3 ++
 command/server/config_telemetry_test.go       | 41 ++++++++++++++
 command/server/config_test_helpers.go         |  4 ++
 .../telemetry/filter_default_override.hcl     |  7 +++
 .../telemetry/valid_prefix_filter.hcl         |  7 +++
 internalshared/configutil/telemetry.go        | 41 +++++++++++++-
 internalshared/configutil/telemetry_test.go   | 53 +++++++++++++++++++
 .../content/docs/configuration/telemetry.mdx  | 11 ++++
 9 files changed, 167 insertions(+), 2 deletions(-)
 create mode 100644 changelog/12025.txt
 create mode 100644 command/server/config_telemetry_test.go
 create mode 100644 command/server/test-fixtures/telemetry/filter_default_override.hcl
 create mode 100644 command/server/test-fixtures/telemetry/valid_prefix_filter.hcl
 create mode 100644 internalshared/configutil/telemetry_test.go

diff --git a/.gitignore b/.gitignore
index e0af2f4a2..64d9e2245 100644
--- a/.gitignore
+++ b/.gitignore
@@ -48,7 +48,7 @@ Vagrantfile
 # Configs
 *.hcl
 !command/agent/config/test-fixtures/*.hcl
-!command/server/test-fixtures/*.hcl
+!command/server/test-fixtures/**/*.hcl
 
 
 .DS_Store
diff --git a/changelog/12025.txt b/changelog/12025.txt
new file mode 100644
index 000000000..79f14b17c
--- /dev/null
+++ b/changelog/12025.txt
@@ -0,0 +1,3 @@
+```release-note:improvement
+core: Add `prefix_filter` to telemetry config
+```
\ No newline at end of file
diff --git a/command/server/config_telemetry_test.go b/command/server/config_telemetry_test.go
new file mode 100644
index 000000000..3f2e3cff4
--- /dev/null
+++ b/command/server/config_telemetry_test.go
@@ -0,0 +1,41 @@
+package server
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestMetricFilterConfigs(t *testing.T) {
+	t.Parallel()
+	cases := []struct {
+		configFile            string
+		expectedFilterDefault *bool
+		expectedPrefixFilter  []string
+	}{
+		{
+			"./test-fixtures/telemetry/valid_prefix_filter.hcl",
+			nil,
+			[]string{"-vault.expire", "-vault.audit", "+vault.expire.num_irrevocable_leases"},
+		},
+		{
+			"./test-fixtures/telemetry/filter_default_override.hcl",
+			boolPointer(false),
+			[]string(nil),
+		},
+	}
+	t.Run("validate metric filter configs", func(t *testing.T) {
+		t.Parallel()
+
+		for _, tc := range cases {
+			config, err := LoadConfigFile(tc.configFile)
+
+			if err != nil {
+				t.Fatalf("Error encountered when loading config %+v", err)
+			}
+
+			assert.Equal(t, tc.expectedFilterDefault, config.SharedConfig.Telemetry.FilterDefault)
+			assert.Equal(t, tc.expectedPrefixFilter, config.SharedConfig.Telemetry.PrefixFilter)
+		}
+	})
+}
diff --git a/command/server/config_test_helpers.go b/command/server/config_test_helpers.go
index fc7fc910a..e40f6c836 100644
--- a/command/server/config_test_helpers.go
+++ b/command/server/config_test_helpers.go
@@ -16,6 +16,10 @@ import (
 	"github.com/hashicorp/vault/internalshared/configutil"
 )
 
+func boolPointer(x bool) *bool {
+	return &x
+}
+
 func testConfigRaftRetryJoin(t *testing.T) {
 	config, err := LoadConfigFile("./test-fixtures/raft_retry_join.hcl")
 	if err != nil {
diff --git a/command/server/test-fixtures/telemetry/filter_default_override.hcl b/command/server/test-fixtures/telemetry/filter_default_override.hcl
new file mode 100644
index 000000000..04e55f646
--- /dev/null
+++ b/command/server/test-fixtures/telemetry/filter_default_override.hcl
@@ -0,0 +1,7 @@
+disable_mlock = true
+ui            = true
+
+telemetry {
+  statsd_address = "foo"
+  filter_default = false
+}
\ No newline at end of file
diff --git a/command/server/test-fixtures/telemetry/valid_prefix_filter.hcl b/command/server/test-fixtures/telemetry/valid_prefix_filter.hcl
new file mode 100644
index 000000000..814dd1c82
--- /dev/null
+++ b/command/server/test-fixtures/telemetry/valid_prefix_filter.hcl
@@ -0,0 +1,7 @@
+disable_mlock = true
+ui            = true
+
+telemetry {
+  statsd_address = "foo"
+  prefix_filter = ["-vault.expire", "-vault.audit", "+vault.expire.num_irrevocable_leases"]
+}
\ No newline at end of file
diff --git a/internalshared/configutil/telemetry.go b/internalshared/configutil/telemetry.go
index 3fd42aa60..fe00cc87b 100644
--- a/internalshared/configutil/telemetry.go
+++ b/internalshared/configutil/telemetry.go
@@ -4,9 +4,10 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"github.com/hashicorp/vault/sdk/helper/parseutil"
 	"time"
 
+	"github.com/hashicorp/vault/sdk/helper/parseutil"
+
 	monitoring "cloud.google.com/go/monitoring/apiv3"
 	"github.com/armon/go-metrics"
 	"github.com/armon/go-metrics/circonus"
@@ -150,6 +151,14 @@ type Telemetry struct {
 
 	// Whether or not telemetry should add labels for namespaces
 	LeaseMetricsNameSpaceLabels bool `hcl:"add_lease_metrics_namespace_labels"`
+
+	// FilterDefault is the default for whether to allow a metric that's not
+	// covered by the prefix filter.
+	FilterDefault *bool `hcl:"filter_default"`
+
+	// PrefixFilter is a list of filter rules to apply for allowing
+	// or blocking metrics by prefix.
+	PrefixFilter []string `hcl:"prefix_filter"`
 }
 
 func (t *Telemetry) Validate(source string) []ConfigError {
@@ -259,6 +268,9 @@ func SetupTelemetry(opts *SetupTelemetryOpts) (*metrics.InmemSink, *metricsutil.
 	metricsConf := metrics.DefaultConfig(opts.ServiceName)
 	metricsConf.EnableHostname = !opts.Config.DisableHostname
 	metricsConf.EnableHostnameLabel = opts.Config.EnableHostnameLabel
+	if opts.Config.FilterDefault != nil {
+		metricsConf.FilterDefault = *opts.Config.FilterDefault
+	}
 
 	// Configure the statsite sink
 	var fanout metrics.FanoutSink
@@ -388,5 +400,32 @@ func SetupTelemetry(opts *SetupTelemetryOpts) (*metrics.InmemSink, *metricsutil.
 	wrapper.TelemetryConsts.LeaseMetricsNameSpaceLabels = opts.Config.LeaseMetricsNameSpaceLabels
 	wrapper.TelemetryConsts.NumLeaseMetricsTimeBuckets = opts.Config.NumLeaseMetricsTimeBuckets
 
+	// Parse the metric filters
+	telemetryAllowedPrefixes, telemetryBlockedPrefixes, err := parsePrefixFilter(opts.Config.PrefixFilter)
+
+	if err != nil {
+		return nil, nil, false, err
+	}
+
+	metrics.UpdateFilter(telemetryAllowedPrefixes, telemetryBlockedPrefixes)
 	return inm, wrapper, prometheusEnabled, nil
 }
+
+func parsePrefixFilter(prefixFilters []string) ([]string, []string, error) {
+	var telemetryAllowedPrefixes, telemetryBlockedPrefixes []string
+
+	for _, rule := range prefixFilters {
+		if rule == "" {
+			return nil, nil, fmt.Errorf("Cannot have empty filter rule in prefix_filter")
+		}
+		switch rule[0] {
+		case '+':
+			telemetryAllowedPrefixes = append(telemetryAllowedPrefixes, rule[1:])
+		case '-':
+			telemetryBlockedPrefixes = append(telemetryBlockedPrefixes, rule[1:])
+		default:
+			return nil, nil, fmt.Errorf("Filter rule must begin with either '+' or '-': %q", rule)
+		}
+	}
+	return telemetryAllowedPrefixes, telemetryBlockedPrefixes, nil
+}
diff --git a/internalshared/configutil/telemetry_test.go b/internalshared/configutil/telemetry_test.go
new file mode 100644
index 000000000..dda74711d
--- /dev/null
+++ b/internalshared/configutil/telemetry_test.go
@@ -0,0 +1,53 @@
+package configutil
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestParsePrefixFilters(t *testing.T) {
+	t.Parallel()
+	cases := []struct {
+		inputFilters            []string
+		expectedErrStr          string
+		expectedAllowedPrefixes []string
+		expectedBlockedPrefixes []string
+	}{
+		{
+			[]string{""},
+			"Cannot have empty filter rule in prefix_filter",
+			[]string(nil),
+			[]string(nil),
+		},
+		{
+			[]string{"vault.abc"},
+			"Filter rule must begin with either '+' or '-': \"vault.abc\"",
+			[]string(nil),
+			[]string(nil),
+		},
+		{
+			[]string{"+vault.abc", "-vault.bcd"},
+			"",
+			[]string{"vault.abc"},
+			[]string{"vault.bcd"},
+		},
+	}
+	t.Run("validate metric filter configs", func(t *testing.T) {
+		t.Parallel()
+
+		for _, tc := range cases {
+
+			allowedPrefixes, blockedPrefixes, err := parsePrefixFilter(tc.inputFilters)
+
+			if err != nil {
+				assert.EqualError(t, err, tc.expectedErrStr)
+			} else {
+				assert.Equal(t, "", tc.expectedErrStr)
+				assert.Equal(t, tc.expectedAllowedPrefixes, allowedPrefixes)
+
+				assert.Equal(t, tc.expectedBlockedPrefixes, blockedPrefixes)
+			}
+		}
+	})
+}
diff --git a/website/content/docs/configuration/telemetry.mdx b/website/content/docs/configuration/telemetry.mdx
index 8aca53d12..fe6bbf069 100644
--- a/website/content/docs/configuration/telemetry.mdx
+++ b/website/content/docs/configuration/telemetry.mdx
@@ -51,6 +51,17 @@ The following options are available on all telemetry configurations.
 - `add_lease_metrics_namespace_labels` `(bool: false)` - If this value is set to true, then `vault.expire.leases.by_expiration`
   will break down expiring leases by both time and namespace. This parameter is disabled by default because enabling it can lead
   to a large-cardinality metric.
+- `filter_default` `(bool: true)` - This controls whether to allow metrics that have not been specified by the filter.
+  Defaults to `true`, which will allow all metrics when no filters are provided.
+  When set to `false` with no filters, no metrics will be sent.
+- `prefix_filter` `(string array: [])` - This is a list of filter rules to apply for allowing/blocking metrics by
+  prefix in the following format:
+  ```json
+  ["+vault.token", "-vault.expire", "+vault.expire.num_leases"]
+  ```
+  A leading "**+**" will enable any metrics with the given prefix, and a leading "**-**" will block them. 
+  If there is overlap between two rules, the more specific rule will take precedence. Blocking will take priority if the same prefix is listed multiple times.
+
 
 ### `statsite`