Skip to content

amtool: Add silence display subcommand, add silence-ids validation, fix nil pointer dereference.#2566

Closed
yurkeen wants to merge 3 commits intoprometheus:mainfrom
yurkeen:amtool-silence-display
Closed

amtool: Add silence display subcommand, add silence-ids validation, fix nil pointer dereference.#2566
yurkeen wants to merge 3 commits intoprometheus:mainfrom
yurkeen:amtool-silence-display

Conversation

@yurkeen
Copy link

@yurkeen yurkeen commented May 2, 2021

This change brings missing amtool functionality, which allows to request and display silence information by user-specified silence ID. Silence IDs are verified to be valid UUIDs, and when they are, retrieved from alertmanager. Before this change a user would need to filter silences by silence ID using other tools (think grep, jq, etc).

Naming for the subcommand, i.e. display, was chosen over show to not bring confusion to users, given that amtool already has query. Happy to change if any objections.

This PR also adds validation for silence-ids supplied to the silence display, silence update and silence expire subcommands, so when supplied values are not valid UUIDs, error is returned.

Additionally, this PR fixes a nil pointer dereference bug which happens due to the change in #2471, where the IsEqual *bool field of the Matcher model is optional and at the same time is intended to have true as its default value. Therefore, when a Matcher – with its IsEqual field set to nil– is received by the format/labelsMatcher() func, amtool panics.

Edit: typos

yurkeen added 3 commits May 2, 2021 15:19
This change brings missing amtool functionality displaying silence information by silence ID. Silence IDs are verified to be UUIDs, and when they are, retreieved from alertmanager.

Signed-off-by: Yury Evtikhov <yury@cloudflare.com>
This change adds validation for silence-ids supplied to the `silence update` and `silence expire` subcommands of amtool. When supplied silence-ids are not UUIDs, error is returned.

Signed-off-by: Yury Evtikhov <yury@cloudflare.com>
The changes introduced in prometheus#2471 (see prometheus@2b6315f#diff-4975284c2279cb0d4dbc76c701ea7c35725245a92caaf2e8be8e5b5e56cf1ab0) allow generated `Matcher.IsEqual` can become nil (see https://github.com/prometheus/alertmanager/blob/2b6315f399b95c4e327dcd5f5eb10866ef844252/api/v2/models/matcher.go#L35), as its default value is considered  true now. Checking for nil value of this field and default it to `true` in the `labelsMatcher()` func fixes nil pointer dereference.

Signed-off-by: Yury Evtikhov <yury@cloudflare.com>
@yurkeen yurkeen force-pushed the amtool-silence-display branch from d4f846b to eb4dead Compare May 2, 2021 14:19
@yurkeen
Copy link
Author

yurkeen commented May 5, 2021

@simonpasquier or @w0rm – may I seek your reviews, when possible? Thanks!

@angelinelee22
Copy link

Hi,
I've been running into panic: runtime error: invalid memory address or nil pointer dereference whenever I run amtool silence query with either simple or extended formatting. Wanted to confirm if this is related to the above, although it runs fine with the -q quiet tag or -o json otherwise. If so, the issue has rendered amtool silence query unusable. It'd be great if this could be pushed along. Thanks!

@yurkeen
Copy link
Author

yurkeen commented Jul 22, 2021

Hi @angelinelee22,

It is very likely that you are facing the bug covered by this PR, if your alertmanager version is v0.22.0 or older, where #2602 was not yet fixed.

@angelinelee22
Copy link

Hi @yurkeen,
Sorry for missing this. I am actually using 0.22.2 on the darwin/amd64 platform and can confirm I'm still facing this issue.

@yurkeen
Copy link
Author

yurkeen commented Aug 27, 2021

Nill pointer dereference fixed in #2668. Since the rest did not get any attention, I'm closing the PR.

@yurkeen yurkeen closed this Aug 27, 2021
@yurkeen yurkeen deleted the amtool-silence-display branch August 27, 2021 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants