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.

Sign up for GitHub

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

Jump to bottom

Add link to Signalilo in integrations page #1589

Merged
merged 1 commit into from
Feb 17, 2021
Merged

Add link to Signalilo in integrations page #1589

merged 1 commit into from
Feb 17, 2021

Conversation

simu
Copy link
Contributor

@simu simu commented Mar 24, 2020 •
edited
Loading

This is just a small change adding our tool "Signalilo" which converts received notifications into Icinga2 services to the list of webhook-based integrations.

brian-brazil
brian-brazil reviewed Mar 24, 2020
View reviewed changes
Copy link
Contributor

@brian-brazil brian-brazil left a comment

Choose a reason for hiding this comment

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

You're taking secrets on the command line, which is not secure.

content/docs/operating/integrations.md Outdated
@@ -69,6 +69,7 @@ For notification mechanisms not natively supported by the Alertmanager, the
* [AWS SNS](https://github.com/DataReply/alertmanager-sns-forwarder)
* [DingTalk](https://github.com/timonwong/prometheus-webhook-dingtalk)
* [GELF](https://github.com/b-com-software-basis/alertmanager2gelf)
* [Icinga2 (Signalilo)](https://github.com/vshn/signalilo): creates services in Icinga2 based on received notifications
Copy link
Contributor

Choose a reason for hiding this comment

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

Just saying Icinga2 is enough

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need the description either

@simu
Copy link
Contributor Author

simu commented Mar 24, 2020

You're taking secrets on the command line, which is not secure.

We're aware of that and pass the secrets as environment vars from a Kubernetes secret in the Helm chart. Unfortunately kingpin doesn't seem to allow using only the envvar to provide secret arguments, so I've opened an issue to address the problem: vshn/signalilo#17

@brian-brazil
Copy link
Contributor

Let me know when that's resolved, our policy is not to list integrations with obvious security issues.

@simu
Copy link
Contributor Author

simu commented Mar 24, 2020

Can you elaborate on what you consider obvious security issues when it comes to handling secret arguments?

@brian-brazil
Copy link
Contributor

You're allowing secrets on the command line.

@simu
Copy link
Contributor Author

simu commented Mar 25, 2020

I was asking more generally as to what practices you consider obvious security issues, since I wasn't able to find any guidelines after a quick look in the repository.

For example, Is it OK to pass secrets in environment variables? In config files?

@brian-brazil
Copy link
Contributor

Both those methods are okay (though environment variables have their risks).

@roidelapluie
Copy link
Member

Maybe we should also wait for vshn/signalilo#15 to be resolved to people can use this effectively?

@brian-brazil
Copy link
Contributor

There's no documentation requirement, as long as the code isn't obviously broken that's good enough in that regard.

@roidelapluie
Copy link
Member

https://github.com/vshn/signalilo/blob/master/webhook/icinga.go#L20 -> is that an internal repo?

@roidelapluie
Copy link
Member 

$ go get github.com/vshn/signalilo
# cd .; git clone -- https://git.vshn.net/appuio/signalilo.git /home/roidelapluie/go/src/git.vshn.net/appuio/signalilo
Cloning into '/home/roidelapluie/go/src/git.vshn.net/appuio/signalilo'...
fatal: could not read Username for 'https://git.vshn.net': terminal prompts disabled
package git.vshn.net/appuio/signalilo/config: exit status 128
package git.vshn.net/appuio/signalilo/gc: cannot find package "git.vshn.net/appuio/signalilo/gc" in any of:
        /home/roidelapluie/godist/go/src/git.vshn.net/appuio/signalilo/gc (from $GOROOT)
        /home/roidelapluie/go/src/git.vshn.net/appuio/signalilo/gc (from $GOPATH)
package git.vshn.net/appuio/signalilo/webhook: cannot find package "git.vshn.net/appuio/signalilo/webhook" in any of:
        /home/roidelapluie/godist/go/src/git.vshn.net/appuio/signalilo/webhook (from $GOROOT)
        /home/roidelapluie/go/src/git.vshn.net/appuio/signalilo/webhook (from $GOPATH)

@simu
Copy link
Contributor Author

simu commented Mar 27, 2020

@roidelapluie no idea how we missed this during the opensourcing process, the only reason why the github build works is that go.mod wasn't updated.

@simu simu mentioned this pull request Mar 27, 2020
Closed
@beorn7 beorn7 added the exporters and integrations Requests for new entries in the list of exporters and integrations label Mar 27, 2020
@roidelapluie
Copy link
Member

The description still needs to be removed. Thanks.

@simu
Copy link
Contributor Author

simu commented Mar 30, 2020

@roidelapluie Can you elaborate on which description needs to be removed? I don't quite follow.

roidelapluie
roidelapluie reviewed Mar 30, 2020
View reviewed changes
content/docs/operating/integrations.md Outdated
@@ -69,6 +69,7 @@ For notification mechanisms not natively supported by the Alertmanager, the
* [AWS SNS](https://github.com/DataReply/alertmanager-sns-forwarder)
* [DingTalk](https://github.com/timonwong/prometheus-webhook-dingtalk)
* [GELF](https://github.com/b-com-software-basis/alertmanager2gelf)
* [Icinga2](https://github.com/vshn/signalilo): creates services in Icinga2 based on received notifications
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
* [Icinga2](https://github.com/vshn/signalilo): creates services in Icinga2 based on received notifications
* [Icinga2](https://github.com/vshn/signalilo)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh ok, I've adjusted that in the latest version of the commit.

@simu
Add link to Signalilo in integrations page
27b85c7 
Signalilo is a tool to convert Alertmanager notifications to Icinga2
services.

Signed-off-by: Simon Gerber <simon.gerber@vshn.ch>
@roidelapluie
Copy link
Member

Thanks. now only pending vshn/signalilo#17

@roidelapluie roidelapluie merged commit 93ea33f into prometheus:master Feb 17, 2021
@roidelapluie
Copy link
Member

I actually missed my last comment when merging. I hope that you will soon be able to address vshn/signalilo#17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brian-brazil brian-brazil brian-brazil left review comments

@roidelapluie roidelapluie roidelapluie left review comments

Assignees
No one assigned
Labels
exporters and integrations Requests for new entries in the list of exporters and integrations
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@simu @brian-brazil @roidelapluie @beorn7