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.

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

[VAULT-2776] Add prefix_filter option to Vault #12025

Merged
merged 6 commits into from Jul 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .gitignore
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions changelog/12025.txt
@@ -0,0 +1,3 @@
```release-note:improvement
core: Add `prefix_filter` to telemetry config
```
41 changes: 41 additions & 0 deletions 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",
ncabatoff marked this conversation as resolved.
Show resolved Hide resolved
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)
}
})
}
4 changes: 4 additions & 0 deletions command/server/config_test_helpers.go
Expand Up @@ -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 {
Expand Down
@@ -0,0 +1,7 @@
disable_mlock = true
ui = true

telemetry {
statsd_address = "foo"
filter_default = false
}
@@ -0,0 +1,7 @@
disable_mlock = true
ui = true

telemetry {
statsd_address = "foo"
prefix_filter = ["-vault.expire", "-vault.audit", "+vault.expire.num_irrevocable_leases"]
}
41 changes: 40 additions & 1 deletion internalshared/configutil/telemetry.go
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
53 changes: 53 additions & 0 deletions 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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's fine with the cases you have now, but I consider it best practice when you have an expected error field in your test case type to do something like

if err != nil { compare with expected error }
else { assert expected error empty}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah that makes sense! added that here 6c2ad99

assert.EqualError(t, err, tc.expectedErrStr)
} else {
assert.Equal(t, "", tc.expectedErrStr)
assert.Equal(t, tc.expectedAllowedPrefixes, allowedPrefixes)

assert.Equal(t, tc.expectedBlockedPrefixes, blockedPrefixes)
}
}
})
}
11 changes: 11 additions & 0 deletions website/content/docs/configuration/telemetry.mdx
Expand Up @@ -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`

Expand Down