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 3 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
3 changes: 3 additions & 0 deletions changelog/12025.txt
@@ -0,0 +1,3 @@
```release-note:improvement
core: Add `prefix_filter` to telemetry config
```
39 changes: 38 additions & 1 deletion internalshared/configutil/telemetry.go
Expand Up @@ -4,9 +4,11 @@ import (
"context"
"errors"
"fmt"
"github.com/hashicorp/vault/sdk/helper/parseutil"
"log"
"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 +152,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 +269,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 +401,29 @@ 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 := parsePrefixFilter(opts.Config.PrefixFilter)

metrics.UpdateFilter(telemetryAllowedPrefixes, telemetryBlockedPrefixes)
return inm, wrapper, prometheusEnabled, nil
}

func parsePrefixFilter(prefixFilters []string) ([]string, []string) {
var telemetryAllowedPrefixes, telemetryBlockedPrefixes []string

for _, rule := range prefixFilters {
if rule == "" {
log.Print("Cannot have empty filter rule in prefix_filter")
continue
}
switch rule[0] {
case '+':
telemetryAllowedPrefixes = append(telemetryAllowedPrefixes, rule[1:])
case '-':
telemetryBlockedPrefixes = append(telemetryBlockedPrefixes, rule[1:])
default:
log.Printf("Filter rule must begin with either '+' or '-': %q", rule)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We never use the stdlib log in Vault. I would add an error return value to this func instead.

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 yeah I wasn't sure if I should use log here. I think the only reason I didn't do error return, is cause the equivalent consul code logs out a warning when the filter is improperly formatted versus failing https://github.com/hashicorp/consul/blob/main/agent/config/builder.go#L652, so I was looking to maintain that pattern. Maybe opts.UI.Warn would make sense https://github.com/hashicorp/vault/blob/main/internalshared/configutil/telemetry.go#L371 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}
}
return telemetryAllowedPrefixes, telemetryBlockedPrefixes
}
19 changes: 19 additions & 0 deletions internalshared/configutil/telemetry_test.go
@@ -0,0 +1,19 @@
package configutil

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestParsePrefixFilters(t *testing.T) {
prefixFilters := []string{"", "+vault.abc", "-vault.abc", "vault.abc"}

allowedPrefixes, blockedPrefixes := parsePrefixFilter(prefixFilters)

assert.Equal(t, len(allowedPrefixes), 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can say assert.Len to check the length with testify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up just doing an assert.Equal against an expected array, while refactoring the test
6a28af2

assert.Equal(t, allowedPrefixes[0], prefixFilters[1][1:])

assert.Equal(t, len(blockedPrefixes), 1)
assert.Equal(t, blockedPrefixes[0], prefixFilters[2][1:])
}
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` - This controls whether to allow metrics that have not been specified by the filter.
Copy link
Collaborator

@ncabatoff ncabatoff Jul 8, 2021

Choose a reason for hiding this comment

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

Please add (bool: true) to describe this param.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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