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

Support UTF-8 label matchers: Add compat package with feature flag and use in amtool #3483

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
ed5998d
Add adapter package for parser feature flag
grobinson-grafana Aug 24, 2023
2c311db
Fix lint
grobinson-grafana Aug 24, 2023
5fde3a0
Add tests
grobinson-grafana Sep 6, 2023
7516580
Fix missing license
grobinson-grafana Sep 6, 2023
075bd06
Fix lint
grobinson-grafana Sep 7, 2023
f958675
Use adapter in amtool
grobinson-grafana Sep 7, 2023
9282da4
Create feature flag to disable new label matchers
grobinson-grafana Sep 8, 2023
8744682
Fix missing feature in AllowedFlags
grobinson-grafana Sep 8, 2023
cdab2c4
Rename adapter to compat
grobinson-grafana Sep 11, 2023
4ec38da
Fix lint
grobinson-grafana Sep 11, 2023
86f0851
Add two option flag
grobinson-grafana Sep 12, 2023
2278d78
Fix lint
grobinson-grafana Sep 12, 2023
846a933
Update warning log on fallback
grobinson-grafana Sep 12, 2023
6e76633
Use same error when parsing matchers plural
grobinson-grafana Sep 13, 2023
d2e0b07
Fix TestQuerySilence test
grobinson-grafana Sep 13, 2023
b1f8e9c
Add check for braces to parseLabels
grobinson-grafana Sep 14, 2023
0fa923e
Remove extra : in error message
grobinson-grafana Sep 14, 2023
c8691b4
Add verbose logging and suggestions to warning logs
grobinson-grafana Sep 15, 2023
794361d
Fix check for braces in wrong place
grobinson-grafana Sep 15, 2023
5371e4a
Fix incorrect parser used
grobinson-grafana Sep 21, 2023
9dbd07d
Put enabled at the end of the sentence
grobinson-grafana Sep 23, 2023
9543033
Use single line when emitting logs
grobinson-grafana Sep 23, 2023
40d3e53
Move comment inside if
grobinson-grafana Sep 23, 2023
f85a75e
Update log line to include both labels and matchers
grobinson-grafana Sep 23, 2023
1efe79b
Rename stable to classic
grobinson-grafana Sep 23, 2023
5f49101
Remove functions from utils.go
grobinson-grafana Sep 25, 2023
ff1c279
Fix tests
grobinson-grafana Oct 11, 2023
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
40 changes: 30 additions & 10 deletions cli/alert_add.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,18 @@ package cli

import (
"context"
"errors"
"fmt"
"strconv"
"time"

"github.com/alecthomas/kingpin/v2"
"github.com/go-openapi/strfmt"

"github.com/prometheus/alertmanager/api/v2/client/alert"
"github.com/prometheus/alertmanager/api/v2/models"
"github.com/prometheus/alertmanager/matchers/compat"
"github.com/prometheus/alertmanager/pkg/labels"
)

type alertAddCmd struct {
Expand Down Expand Up @@ -73,29 +77,45 @@ func (a *alertAddCmd) addAlert(ctx context.Context, _ *kingpin.ParseContext) err
if len(a.labels) > 0 {
// Allow the alertname label to be defined implicitly as the first argument rather
// than explicitly as a key=value pair.
if _, err := parseLabels([]string{a.labels[0]}); err != nil {
a.labels[0] = fmt.Sprintf("alertname=%s", a.labels[0])
if _, err := compat.Matcher(a.labels[0]); err != nil {
a.labels[0] = fmt.Sprintf("alertname=%s", strconv.Quote(a.labels[0]))
grobinson-grafana marked this conversation as resolved.
Show resolved Hide resolved
}
}

labels, err := parseLabels(a.labels)
if err != nil {
return err
ls := make(models.LabelSet, len(a.labels))
for _, l := range a.labels {
matcher, err := compat.Matcher(l)
if err != nil {
return err
}
if matcher.Type != labels.MatchEqual {
return errors.New("labels must be specified as key=value pairs")
}
ls[matcher.Name] = matcher.Value
}

annotations, err := parseLabels(a.annotations)
if err != nil {
return err
annotations := make(models.LabelSet, len(a.annotations))
for _, a := range a.annotations {
matcher, err := compat.Matcher(a)
if err != nil {
return err
}
if matcher.Type != labels.MatchEqual {
return errors.New("annotations must be specified as key=value pairs")
}
annotations[matcher.Name] = matcher.Value
}

var startsAt, endsAt time.Time
if a.start != "" {
var err error
startsAt, err = time.Parse(time.RFC3339, a.start)
if err != nil {
return err
}
}
if a.end != "" {
var err error
endsAt, err = time.Parse(time.RFC3339, a.end)
if err != nil {
return err
Expand All @@ -105,7 +125,7 @@ func (a *alertAddCmd) addAlert(ctx context.Context, _ *kingpin.ParseContext) err
pa := &models.PostableAlert{
Alert: models.Alert{
GeneratorURL: strfmt.URI(a.generatorURL),
Labels: labels,
Labels: ls,
},
Annotations: annotations,
StartsAt: strfmt.DateTime(startsAt),
Expand All @@ -116,6 +136,6 @@ func (a *alertAddCmd) addAlert(ctx context.Context, _ *kingpin.ParseContext) err

amclient := NewAlertmanagerClient(alertmanagerURL)

_, err = amclient.Alert.PostAlerts(alertParams)
_, err := amclient.Alert.PostAlerts(alertParams)
return err
}
4 changes: 2 additions & 2 deletions cli/alert_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import (

"github.com/prometheus/alertmanager/api/v2/client/alert"
"github.com/prometheus/alertmanager/cli/format"
"github.com/prometheus/alertmanager/pkg/labels"
"github.com/prometheus/alertmanager/matchers/compat"
)

type alertQueryCmd struct {
Expand Down Expand Up @@ -80,7 +80,7 @@ func (a *alertQueryCmd) queryAlerts(ctx context.Context, _ *kingpin.ParseContext
// the user wants alertname=<arg> and prepend `alertname=` to
// the front.
m := a.matcherGroups[0]
_, err := labels.ParseMatcher(m)
_, err := compat.Matcher(m)
grobinson-grafana marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
a.matcherGroups[0] = fmt.Sprintf("alertname=%s", m)
}
Expand Down
27 changes: 25 additions & 2 deletions cli/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,13 @@ import (
"net/url"
"os"
"path"
"strings"
"time"

"github.com/alecthomas/kingpin/v2"
"github.com/go-kit/log"
"github.com/go-kit/log/level"
clientruntime "github.com/go-openapi/runtime/client"
"github.com/go-openapi/strfmt"
promconfig "github.com/prometheus/common/config"
"github.com/prometheus/common/version"
Expand All @@ -29,8 +33,8 @@ import (
"github.com/prometheus/alertmanager/api/v2/client"
"github.com/prometheus/alertmanager/cli/config"
"github.com/prometheus/alertmanager/cli/format"

clientruntime "github.com/go-openapi/runtime/client"
"github.com/prometheus/alertmanager/featurecontrol"
"github.com/prometheus/alertmanager/matchers/compat"
)

var (
Expand All @@ -40,11 +44,27 @@ var (
timeout time.Duration
httpConfigFile string
versionCheck bool
featureFlags string

configFiles = []string{os.ExpandEnv("$HOME/.config/amtool/config.yml"), "/etc/amtool/config.yml"}
legacyFlags = map[string]string{"comment_required": "require-comment"}
)

func initMatchersCompat(_ *kingpin.ParseContext) error {
grobinson-grafana marked this conversation as resolved.
Show resolved Hide resolved
logger := log.NewLogfmtLogger(os.Stdout)
if verbose {
logger = level.NewFilter(logger, level.AllowDebug())
} else {
logger = level.NewFilter(logger, level.AllowInfo())
}
featureConfig, err := featurecontrol.NewFlags(logger, featureFlags)
if err != nil {
kingpin.Fatalf("error parsing the feature flag list: %v\n", err)
}
compat.InitFromFlags(logger, featureConfig)
return nil
}

func requireAlertManagerURL(pc *kingpin.ParseContext) error {
// Return without error if any help flag is set.
for _, elem := range pc.Elements {
Expand Down Expand Up @@ -137,6 +157,7 @@ func Execute() {
app.Flag("timeout", "Timeout for the executed command").Default("30s").DurationVar(&timeout)
app.Flag("http.config.file", "HTTP client configuration file for amtool to connect to Alertmanager.").PlaceHolder("<filename>").ExistingFileVar(&httpConfigFile)
app.Flag("version-check", "Check alertmanager version. Use --no-version-check to disable.").Default("true").BoolVar(&versionCheck)
app.Flag("enable-feature", fmt.Sprintf("Experimental features to enable. The flag can be repeated to enable multiple features. Valid options: %s", strings.Join(featurecontrol.AllowedFlags, ", "))).Default("").StringVar(&featureFlags)
grobinson-grafana marked this conversation as resolved.
Show resolved Hide resolved

app.Version(version.Print("amtool"))
app.GetFlag("help").Short('h')
Expand All @@ -154,6 +175,8 @@ func Execute() {
configureConfigCmd(app)
configureTemplateCmd(app)

app.Action(initMatchersCompat)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
app.Action(initMatchersCompat)
// Matchers parsing initialization happens as a kingpin.Action to guarantee this is run after Kingpin had computed the flags but before running any commands
app.Action(initMatchersCompat)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is more or less documented in the doc comments for app.Action:

Action callback to call when all values are populated and parsing is complete, but before any command, flag or argument actions.

If you still want it to be documented here I'm happy to make that change, but just wanted to share this information in case it changes your opinion.


err = resolver.Bind(app, os.Args[1:])
if err != nil {
kingpin.Fatalf("%v\n", err)
Expand Down
18 changes: 12 additions & 6 deletions cli/silence_add.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"errors"
"fmt"
"os/user"
"strconv"
"time"

"github.com/alecthomas/kingpin/v2"
Expand All @@ -26,6 +27,8 @@ import (

"github.com/prometheus/alertmanager/api/v2/client/silence"
"github.com/prometheus/alertmanager/api/v2/models"
"github.com/prometheus/alertmanager/matchers/compat"
"github.com/prometheus/alertmanager/pkg/labels"
)

func username() string {
Expand Down Expand Up @@ -92,17 +95,20 @@ func (c *silenceAddCmd) add(ctx context.Context, _ *kingpin.ParseContext) error
// If the parser fails then we likely don't have a (=|=~|!=|!~) so lets
// assume that the user wants alertname=<arg> and prepend `alertname=`
// to the front.
_, err := parseMatchers([]string{c.matchers[0]})
_, err := compat.Matcher(c.matchers[0])
if err != nil {
c.matchers[0] = fmt.Sprintf("alertname=%s", c.matchers[0])
c.matchers[0] = fmt.Sprintf("alertname=%s", strconv.Quote(c.matchers[0]))
}
}

matchers, err := parseMatchers(c.matchers)
if err != nil {
return err
matchers := make([]labels.Matcher, 0, len(c.matchers))
for _, s := range c.matchers {
m, err := compat.Matcher(s)
if err != nil {
return err
}
matchers = append(matchers, *m)
}

if len(matchers) < 1 {
return fmt.Errorf("no matchers specified")
}
Expand Down
7 changes: 4 additions & 3 deletions cli/silence_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,15 @@ import (
"context"
"errors"
"fmt"
"strconv"
"time"

kingpin "github.com/alecthomas/kingpin/v2"

"github.com/prometheus/alertmanager/api/v2/client/silence"
"github.com/prometheus/alertmanager/api/v2/models"
"github.com/prometheus/alertmanager/cli/format"
"github.com/prometheus/alertmanager/pkg/labels"
"github.com/prometheus/alertmanager/matchers/compat"
)

type silenceQueryCmd struct {
Expand Down Expand Up @@ -98,9 +99,9 @@ func (c *silenceQueryCmd) query(ctx context.Context, _ *kingpin.ParseContext) er
// If the parser fails then we likely don't have a (=|=~|!=|!~) so lets
// assume that the user wants alertname=<arg> and prepend `alertname=`
// to the front.
_, err := labels.ParseMatcher(c.matchers[0])
_, err := compat.Matcher(c.matchers[0])
if err != nil {
c.matchers[0] = fmt.Sprintf("alertname=%s", c.matchers[0])
c.matchers[0] = fmt.Sprintf("alertname=%s", strconv.Quote(c.matchers[0]))
}
}

Expand Down
15 changes: 12 additions & 3 deletions cli/test_routing.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import (

"github.com/prometheus/alertmanager/api/v2/models"
"github.com/prometheus/alertmanager/dispatch"
"github.com/prometheus/alertmanager/matchers/compat"
"github.com/prometheus/alertmanager/pkg/labels"
)

const routingTestHelp = `Test alert routing
Expand Down Expand Up @@ -80,9 +82,16 @@ func (c *routingShow) routingTestAction(ctx context.Context, _ *kingpin.ParseCon
mainRoute := dispatch.NewRoute(cfg.Route, nil)

// Parse labels to LabelSet.
ls, err := parseLabels(c.labels)
if err != nil {
kingpin.Fatalf("Failed to parse labels: %v\n", err)
ls := make(models.LabelSet, len(c.labels))
for _, l := range c.labels {
matcher, err := compat.Matcher(l)
if err != nil {
kingpin.Fatalf("Failed to parse labels: %v\n", err)
}
if matcher.Type != labels.MatchEqual {
kingpin.Fatalf("%s\n", "Labels must be specified as key=value pairs")
}
ls[matcher.Name] = matcher.Value
}

if c.debugTree {
Expand Down
37 changes: 1 addition & 36 deletions cli/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import (
"net/url"
"os"

kingpin "github.com/alecthomas/kingpin/v2"
"github.com/alecthomas/kingpin/v2"
"github.com/prometheus/common/model"

"github.com/prometheus/alertmanager/api/v2/client/general"
Expand All @@ -29,22 +29,6 @@ import (
"github.com/prometheus/alertmanager/pkg/labels"
)

// parseMatchers parses a list of matchers (cli arguments).
func parseMatchers(inputMatchers []string) ([]labels.Matcher, error) {
matchers := make([]labels.Matcher, 0, len(inputMatchers))

grobinson-grafana marked this conversation as resolved.
Show resolved Hide resolved
for _, v := range inputMatchers {
matcher, err := labels.ParseMatcher(v)
if err != nil {
return []labels.Matcher{}, err
}

matchers = append(matchers, *matcher)
}

return matchers, nil
}

// getRemoteAlertmanagerConfigStatus returns status responsecontaining configuration from remote Alertmanager
func getRemoteAlertmanagerConfigStatus(ctx context.Context, alertmanagerURL *url.URL) (*models.AlertmanagerStatus, error) {
amclient := NewAlertmanagerClient(alertmanagerURL)
Expand Down Expand Up @@ -94,25 +78,6 @@ func convertClientToCommonLabelSet(cls models.LabelSet) model.LabelSet {
return mls
}

// parseLabels parses a list of labels (cli arguments).
func parseLabels(inputLabels []string) (models.LabelSet, error) {
labelSet := make(models.LabelSet, len(inputLabels))

for _, l := range inputLabels {
matcher, err := labels.ParseMatcher(l)
grobinson-grafana marked this conversation as resolved.
Show resolved Hide resolved
grobinson-grafana marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return models.LabelSet{}, err
}
if matcher.Type != labels.MatchEqual {
return models.LabelSet{}, errors.New("labels must be specified as key=value pairs")
}

labelSet[matcher.Name] = matcher.Value
}

return labelSet, nil
}

// TypeMatchers only valid for when you are going to add a silence
func TypeMatchers(matchers []labels.Matcher) models.Matchers {
typeMatchers := make(models.Matchers, len(matchers))
Expand Down