Fix segmentation violation when IsEqual == nil

[jmdacruz]

Running amtool silence query is currently broken with a segmentation fault. This PR adds a default value for IsEqual on the labelsMatcher function. The model.Matcher struct defines IsEqual as omitempty:

alertmanager/api/v2/models/matcher.go

Line 35 in fd0929b

IsEqual *bool `json:"isEqual,omitempty"`

which means that this pointer could be nil. The labelsMatcher function is not checking for nil, which causes a segmentation fault:

alertmanager/cli/format/format.go

Lines 53 to 67 in fd0929b

	func labelsMatcher(m models.Matcher) *labels.Matcher {
	var t labels.MatchType
	switch {
	case !*m.IsRegex && *m.IsEqual:
	t = labels.MatchEqual
	case !*m.IsRegex && !*m.IsEqual:
	t = labels.MatchNotEqual
	case *m.IsRegex && *m.IsEqual:
	t = labels.MatchRegexp
	case *m.IsRegex && !*m.IsEqual:
	t = labels.MatchNotRegexp
	}
	return &labels.Matcher{Type: t, Name: *m.Name, Value: *m.Value}
	}

[roidelapluie]

Thank you. How can we reproduce this? 1. in tests and 2. pratically when using the command line?

Thanks!

[jmdacruz]

For (1), I would suggest a unit test such as this one (format_test.go). I can add this to the PR if it sounds reasonable, and we can add coverage for the other cases (note that IsRegex could also be nil on this function, so we could fix that too):

package format

import (
	"reflect"
	"testing"

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

func TestTabelsMatcher(t *testing.T) {
	trueValue := true

	tests := map[string]struct {
		value    string
		input    models.Matcher
		expected *labels.Matcher
	}{
		"nil IsEqual": {
			value:    "some value",
			input:    models.Matcher{IsEqual: nil, IsRegex: &trueValue},
			expected: &labels.Matcher{Type: labels.MatchNotRegexp},
		},
	}

	for name, test := range tests {
		test.input.Name = &name
		test.expected.Name = name
		test.input.Value = &test.value
		test.expected.Value = test.value
		t.Run(name, func(t *testing.T) {
			got := labelsMatcher(test.input)
			if !reflect.DeepEqual(got, test.expected) {
				t.Fail()
			}
		})
	}

}

For (2), I was able to reproduce the issue by simply running amtool --alertmanager.url=... silence query in an environment where I don't have any sort of configuration file. After applying the patch on this PR, everything seems to be working as expected.

[jmdacruz]

@roidelapluie I just amended the commit by also fixing the case of a nil value for IsRegex, and added a test to cover all combinations

[jmdacruz]

@roidelapluie I created the following docker-compose.yml file in order to test this:

version: "3.8"
services:
  alertmanager:
    image: prom/alertmanager:v0.21.0
    ports: 
    - 9093:9093
  setup:
    image: prom/alertmanager:v0.22.2
    entrypoint: amtool
    depends_on:
    - alertmanager
    command:
    - --alertmanager.url=http://alertmanager:9093
    - silence
    - add
    - --author=jmdacruz
    - --duration=90d
    - --comment="foobar"
    - namespace=~foobar
  test:
    image: prom/alertmanager:v0.22.2
    entrypoint: amtool
    depends_on:
    - alertmanager
    command:
    - --alertmanager.url=http://alertmanager:9093
    - silence
    - query

Version v0.21.0 is the one I have on my internal server. I run this test file as follows:

# Bring up the alert manager instance on the docker-compose
docker-compose up -d alertmanager
# Provision a silence
docker-compose up setup
# Run the test
docker-compose up test

This should trigger the following exception:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x8da822]

goroutine 1 [running]:
github.com/prometheus/alertmanager/cli/format.labelsMatcher(...)
      /app/cli/format/format.go:56
github.com/prometheus/alertmanager/cli/format.simpleFormatMatcher(0x0, 0xc0002b44d8, 0xc00064b250, 0xc00064b260, 0xc0000a45d0, 0x28)
      /app/cli/format/format_simple.go:98 +0x102
github.com/prometheus/alertmanager/cli/format.simpleFormatMatchers(0xc00026e120, 0x1, 0x4, 0x1, 0x1)
      /app/cli/format/format_simple.go:92 +0x98
github.com/prometheus/alertmanager/cli/format.(*SimpleFormatter).FormatSilences(0xc000323130, 0xc000386000, 0xb, 0x10, 0xc000330408, 0x134bc01)
      /app/cli/format/format_simple.go:48 +0x1de
github.com/prometheus/alertmanager/cli.(*silenceQueryCmd).query(0xc000485d40, 0xeb7de8, 0xc0004051a0, 0xc0005a4240, 0xc0004051a0, 0xc000403030)
      /app/cli/silence_query.go:143 +0x7fa
github.com/prometheus/alertmanager/cli.execWithTimeout.func1(0xc0005a4240, 0x0, 0x0)
      /app/cli/utils.go:154 +0xa4
gopkg.in/alecthomas/kingpin%2ev2.(*actionMixin).applyActions(0xc000162618, 0xc0005a4240, 0x0, 0x0)
      /go/pkg/mod/gopkg.in/alecthomas/kingpin.v2@v2.2.6/actions.go:28 +0x6d
gopkg.in/alecthomas/kingpin%2ev2.(*Application).applyActions(0xc0005a00f0, 0xc0005a4240, 0x0, 0x0)
      /go/pkg/mod/gopkg.in/alecthomas/kingpin.v2@v2.2.6/app.go:557 +0xdf
gopkg.in/alecthomas/kingpin%2ev2.(*Application).execute(0xc0005a00f0, 0xc0005a4240, 0xc00040e9e0, 0x2, 0x2, 0x0, 0x0, 0x0, 0x0)
      /go/pkg/mod/gopkg.in/alecthomas/kingpin.v2@v2.2.6/app.go:390 +0x95
gopkg.in/alecthomas/kingpin%2ev2.(*Application).Parse(0xc0005a00f0, 0xc000030110, 0x3, 0x3, 0x3, 0x0, 0x0, 0x10)
      /go/pkg/mod/gopkg.in/alecthomas/kingpin.v2@v2.2.6/app.go:222 +0x228
github.com/prometheus/alertmanager/cli.Execute()
      /app/cli/root.go:122 +0x925
main.main()
      /app/cmd/amtool/main.go:19 +0x25

[jmdacruz]

@roidelapluie I've updated my previous comment with the proper way of reproducing this issue

[roidelapluie]

Thanks, I have left a comment.

[roidelapluie]

Now I am actually wondering if we should support it. You could create silences to 0.21 that would not be supported. So you would ask amtool to create a silence for env!=prod and it would create a silence env=prod.

I think we should to our best so that old clients can talk to new alertmanager, I do not think we should do the opposite.

What do you think?

[roidelapluie]

So maybe instead of guessing or panicking we should error properly.

[jmdacruz]

I think it makes sense @roidelapluie. I'm quite new to amtool, but it looks like in the long run it might be beneficial for the client to check the version of the server for compatibility (if there's a known breaking change). In the meantime, erroring out is probably the best alternative. I can give this a try on this PR.

That said, I noticed something interesting while trying different combinations, let me know if this behavior is expected: I modified my docker-compose.yml test file so that the version of alertmanager and amtool is the same using TAG as an environment variable for the image version, and I added a matcher on the test:

version: "3.8"
services:
  alertmanager:
    image: prom/alertmanager:${TAG}
    ports: 
    - 9093:9093
  setup:
    image: prom/alertmanager:${TAG}
    entrypoint: amtool
    depends_on:
    - alertmanager
    command:
    - --alertmanager.url=http://alertmanager:9093
    - silence
    - add
    - --author=jmdacruz
    - --duration=90d
    - --comment="foobar"
    - namespace=~foobar
  test:
    image: prom/alertmanager:${TAG}
    entrypoint: amtool
    depends_on:
    - alertmanager
    command:
    - --alertmanager.url=http://alertmanager:9093
    - silence
    - query
    - namespace=~.+

I get two distinct results when running with version v0.21.0 and v0.22.2:

export TAG=v0.21.0
docker-compose up -d alertmanager
docker-compose up setup
docker-compose up test

The above finds the silence:

ID                                    Matchers           Ends At                  Created By  Comment   
ffaa2b92-86ef-4569-b63b-94fd297991fb  namespace=~foobar  2021-10-04 17:06:34 UTC  jmdacruz    "foobar"

but with v0.22.2 I get an empty result:

export TAG=v0.22.2
docker-compose up -d alertmanager
docker-compose up setup
docker-compose up test

[roidelapluie]

Note: In the long run we will release a 1.0 release and the clients should be compatible across 1.x releases.

[jmdacruz]

In that case, then maybe surfacing the error for now should be enough. I'll work on changing the PR, see what I can come up with.

[roidelapluie]

The silence not found is expected. The silence has - namespace=foobar, not - namespace=.+
not sure why it works on previous releases

[roidelapluie]

Done in #2668, thanks!

[jmdacruz]

No problem! glad to be a contributor!