Snmp Trap service #929

titilambert · 2016-09-20T21:25:19Z

Required for all non-trivial PRs

Rebased/mergable
Tests pass
CHANGELOG.md updated
Sign CLA (if not already signed)

titilambert · 2016-09-21T17:49:14Z

@nathanielc ready for first review

nathanielc

Overall the PR is organized well, I just have a few questions about the workflow and structure of traps. See my comments in-line.

nathanielc · 2016-09-21T17:58:20Z

pipeline/alert.go

+//    stream
+//         |alert()
+//             .snmp()
+//             .trap('1.3.6.1.2.1.1.7', 'i', {{ Index field value }})


This example isn't valid template syntax. Can you update it to be accurate?

nathanielc · 2016-09-21T18:02:56Z

pipeline/alert.go

+
+	// List of email recipients.
+	// tick:ignore
+	TrapList [][]interface{} `tick:"Trap"`


What information does a trap need?

Based on the configuration and the docs, it looks like traps have a these fields:

Target IP

Target Port

OID

Version

Community

Anything else?

Maybe changing the syntax up a bit would make it more clear:

|alert() .snmpTrap() .ip('x.x.x.x') .port(9863) .oid('1.3.6.1.2.1.1.7') .version('v3') .community('asdf') .snmpTrap() .ip('x.x.x.x') .port(2656) .oid('1.3.6.1.2.1.1.7') .version('v2') .snmpTrap() // defaults to config values if not specified .oid('1.3.6.1.2.1.1.7')

See last comment

nathanielc · 2016-09-21T18:03:23Z

pipeline/alert.go

+//
+// Passing addresses to the `email` property directly or using the `email.to` property is the same.
+// tick:property
+func (h *SnmpHandler) Trap(trap ...interface{}) *SnmpHandler {


This comment needs to be updated to be SNMP related, its still the email comments.

nathanielc · 2016-09-21T18:05:13Z

services/snmp/config.go

+	Version string `toml:"version"`
+	// Whether all alerts should automatically use stateChangesOnly mode.
+	// Only applies if global is also set.
+	StateChangesOnly bool `toml:"state-changes-only"`


Should we also add the Global option like other handlers have? Or does that not work well in the context of SNMP alerts?

I don't see how this could be global. You need to add data to your snmp trap

nathanielc · 2016-09-21T18:05:42Z

services/snmp/service.go

+
+	var varBinds snmpgo.VarBinds
+	for _, trap := range traps {
+		fmt.Printf("\n\n\nREERRRR %s", trap)


Remove this debug logging.

nathanielc · 2016-09-21T18:06:34Z

services/snmp/service.go

+func (s *Service) Alert(traps [][]interface{}, level kapacitor.AlertLevel) error {
+	address := s.targetIp + strconv.Itoa(s.targetPort)
+	snmp, err := snmpgo.NewSNMP(snmpgo.SNMPArguments{
+		Version:   snmpgo.V2c,


Why is there a version option in the config if it is being hardcoded here?

nathanielc · 2016-09-21T18:10:08Z

task_master.go

@@ -82,6 +82,10 @@ type TaskMaster struct {
 		StateChangesOnly() bool
 		Alert(channel, message string, level AlertLevel) error
 	}
+	SnmpService interface {
+		StateChangesOnly() bool
+		Alert(trap [][]interface{}, level AlertLevel) error


Do snmp traps support a message concept? Meaning can they contain a short string of some short or do they only support the alert level? Also how do you know the context of the alert? For example, say you are triggering alerts on CPU usage per host, when the NMS receives the alert how does it know which host triggered the alert and that it is a CPU alert?

I am only marginally familiar with SNMP so if I am missing something obvious please enlighten me. Thanks

See last comment

titilambert · 2016-09-22T12:36:15Z

@nathanielc With all your comments, I understood that this service was not clear. So I renamed it to snmptrap.
In fact, a SNMPtrap contains at leat one data. You can add data to your trap to add more context.
The following example should be more clear:

    |alert()
        .id('kapacitor/{{ index .Tags "host"}}/{{ .Name }}')
        .message('{{ .Level}}: {{ .ID }} - memory: {{ index .Fields "used_percent" }}%')
        .warn(lambda: "used_percent" > 50 )
        .crit(lambda: "used_percent" > 95 )
        .snmpTrap()
          .data('1.3.6.1.4.1.1.1', 's', '{{ .index .Tags "host" }}' )
          .data('1.3.6.1.4.1.1.2', 'i', '{{if eq .Level "OK"}}0{{else}}{{if eq .Level "WARNING"}}44443{{else}}{{if eq .Level "CRITICAL"}}5{{end}}{{end}}{{end}}')
          .data('1.3.6.1.4.1.1.3', 's', '{{ .Message }}' )
          .data('1.3.6.1.4.1.1.5', 's', '{{ .Level }}' )
          .data('1.3.6.1.4.1.1.6', 's', '50' )
          .data('1.3.6.1.4.1.1.7', 's', '{{ index .Fields "used_percent" }}' )

The NMS will receive the trap and process it as ONE message with several fields. One field for one oid.

I think those new names is more clear than the previous one.

titilambert · 2016-09-26T17:00:51Z

@nathanielc any news on that one ?

nathanielc · 2016-09-26T21:16:50Z

@titilambert

OK so One field for one oid. this makes sense now. But what about all the settings in the config file like ip version etc. How can they be overwritten by the TICKscript?

titilambert · 2016-09-27T15:22:37Z

@nathanielc I didn't do that because the community is like a password, so I thought it's better to not expose it in TICK scripts
What do you think about it ?

titilambert · 2016-09-28T15:39:02Z

I found a small issue in the trap sent, I will send an update today

titilambert · 2016-09-28T16:46:53Z

@nathanielc Issue fix with last commit: 28f0c16

titilambert · 2016-10-04T16:06:58Z

@nathanielc this PR is ready

nathanielc

Can you add tests in the integrations package?

You will find tests in there for the other alert handlers.

How can I test this locally? Is there a simple SNMP server I can stand up to test this works as expected?

nathanielc · 2016-10-04T18:08:31Z

pipeline/alert.go

+//          .data('1.3.6.1.4.1..1.7', 's', '{{ index .Fields "used_percent" }}' )
+//
+// tick:property
+func (h *SnmpTrapHandler) Data(data ...interface{}) *SnmpTrapHandler {


Are these arguments always strings? And are there always three of them?

If so should the signature change to Data(oid, rawType, value string)? Not sure what the names should be...

nathanielc · 2016-10-04T18:11:18Z

services/snmptrap/service.go

+		return errors.New("Version 3 not supported yet")
+	default:
+		return errors.New("Bad snmp version should be: '1', '2c' or '3'")
+	}


It doesn't add value to add a version config option and then only support a single version. How hard is it to support the other versions? From what I saw it looked like the snmp package has support for various versions.

I don't know, I can't test snmp v3 :/

nathanielc · 2016-10-04T18:13:28Z

pipeline/alert.go

+//
+// tick:property
+func (h *SnmpTrapHandler) Data(data ...interface{}) *SnmpTrapHandler {
+	// TODO check element validity


You can add a validate method to the SnmpTrapHandler, and call if from the AlertNode validate method.

nathanielc · 2016-10-04T18:14:25Z

pipeline/alert.go

+	TrapOid string
+	// List of trap data.
+	// tick:ignore
+	DataList [][]interface{} `tick:"Data"`


What is the structure of the data? Is there a reason we need to use an interface{} or can this be converted to a list of structs?

Hello ! in fact, I'm just trying to get [][]string but I don't know how to get that
from alert.go line 250

tpattrtmpl, err := text.New("trap").Parse(attr.(string))

How can I get string from this stuff ?

@nathanielc in fact, I think that the struct could be [][3]string
Example:

|alert() .snmpTrap('MAINOID') .data('SUBOID1', 's', '{{ .Message }}') .data('SUBOID2', 'i', '{{if eq .Level "OK"}}0{{else}}{{if eq .Level "WARNING"}}3{{end}}{{end}}') .data('SubOid3', 's', '{{ .Name }}')

titilambert · 2016-12-12T14:46:32Z

Hello @nathanielc !
I have now time to finish this PR !

titilambert · 2016-12-13T17:38:55Z

@nathanielc I just added tests

nathanielc · 2016-12-15T16:57:20Z

@titilambert Thanks! I am rounding the bend on a large refactor for the alert handlers system, ( I hope to have the majority of the work done by tomorrow 🙏 ). As a result I hope to take a look at this next week. It will take some work to refactor it a bit for the changes I am making , if you have time next week as well I can point you in the right direction, otherwise I can make the changes later.

djsly · 2016-12-16T15:11:23Z

@nathanielc thanks for keeping a priority in this PR. Lets us know when the refactoring will be completed asap. we would absolutely need this in 1.1.2 if possible since we are also waiting for some bugs fixes from this release.

nathanielc · 2017-01-05T17:40:33Z

@djsly @titilambert The Alert system work is basically complete at this point. I am going to start looking into this PR again. Can you give me push access to your fork so that I can make changes? See https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

titilambert · 2017-01-05T18:05:15Z

@nathanielc hello, Thanks for your reply !
I just clicked on "Allow edits from maintainers."
Let me know if you can edit the PR.

nathanielc · 2017-01-05T18:10:51Z

@titilambert Thanks! Looks like it is working, I was able to rebase and push to your snmp branch.

nathanielc · 2017-01-06T16:05:00Z

@titilambert @djsly I have most things migrated over to the new alerting system but I have a few questions for things that did not make sense to me as I dug around in the code.

The snmptrap.Service does not do anything with the alert Level. What should it do? Maybe create a string var bind with a definable Oid?
Why are we hardcoding the TimeTicks 1000 var bind for every trap?

For now I removed the logic around configuring the version since only one version is supported. When we need/add support for other versions we can add it back in. Once I am out of the weeds I'll push up my changes so you can take a look.

nathanielc · 2017-01-06T18:48:14Z

@titilambert @djsly OK I have tests passing most of the time ;), but the bad news is the github.com/k-sone/snmpg package is giving me some issues. I know that you had originally used a different package and then switched to this one. Can you provide some details as to why you chose this package?

The issues I am running into only effect testing so they are not super critical but I we need to have stable tests.

I can consistently get the TrapServer to panic when generating a request ID. Also the TrapServer type doesn't have a graceful shutdown (granted that is difficult to do with UDP) but the shutdown process is racy and causes data races like:

WARNING: DATA RACE
Write at 0x00c420148b90 by goroutine 7:
  github.com/influxdata/kapacitor/vendor/github.com/k-sone/snmpgo.(*packetTransport).Close()
      /home/nathanielc/projects/kapacitor/src/github.com/influxdata/kapacitor/vendor/github.com/k-sone/snmpgo/transport.go:76+0xe4
  github.com/influxdata/kapacitor/vendor/github.com/k-sone/snmpgo.(*TrapServer).Close()
      /home/nathanielc/projects/kapacitor/src/github.com/influxdata/kapacitor/vendor/github.com/k-sone/snmpgo/server.go:229 +0x7f
  github.com/influxdata/kapacitor/services/snmptrap/snmptraptest.(*Server).Close()
      /home/nathanielc/projects/kapacitor/src/github.com/influxdata/kapacitor/services/snmptrap/snmptraptest/snmptraptest.go:59 +0x107
  github.com/influxdata/kapacitor/integrations.TestStream_AlertSNMPTrap()
      /home/nathanielc/projects/kapacitor/src/github.com/influxdata/kapacitor/integrations/streamer_test.go:7587 +0x6b6
  testing.tRunner()
      /usr/lib/go/src/testing/testing.go:610 +0xc9

Previous read at 0x00c420148b90 by goroutine 8:
  github.com/influxdata/kapacitor/vendor/github.com/k-sone/snmpgo.(*packetTransport).Listen()
      /home/nathanielc/projects/kapacitor/src/github.com/influxdata/kapacitor/vendor/github.com/k-sone/snmpgo/transport.go:26+0x3f
  github.com/influxdata/kapacitor/vendor/github.com/k-sone/snmpgo.(*TrapServer).Serve()
      /home/nathanielc/projects/kapacitor/src/github.com/influxdata/kapacitor/vendor/github.com/k-sone/snmpgo/server.go:197 +0x132
  github.com/influxdata/kapacitor/services/snmptrap/snmptraptest.NewServer.func1()
      /home/nathanielc/projects/kapacitor/src/github.com/influxdata/kapacitor/services/snmptrap/snmptraptest/snmptraptest.go:41 +0x8f

At this point we have a few options:

Submit PRs to the k-sone/snmpgo package
Choose a different package

Again since I know you played around with other packages too I would like your input before moving forward. Thanks!

nathanielc · 2017-01-06T21:35:14Z

I took a look around and the k-sone/snmpgo package seems to be the best so I put together a PR to fix the races in the server logic. See k-sone/snmpgo#13

I also protected access to the snmp client since it is not designed for concurrent use either.

At this point tests are passing locally without races.

djsly · 2017-01-08T23:01:38Z

@nathanielc thanks for this work on the PR! Would there be anything you would want @titilambert or myself to look at to complete this merge process ?

nathanielc · 2017-01-09T16:44:21Z

@djsly @titilambert

There are two things I would like you to look at before I merge:

Look at the examples in the tests and make sure they make sense (aka aren't misleading or confusing to a new user). While I think I understand how SNMP traps work now, I don't have all the context of how they are typically used, so if one of the examples is just odd I'd like to fix it.
Test it out locally, this is the most important. I have refactored the way it works a little bit so I want to make sure it still fits your use case.

And one question:

Is the community value considered secret? Meaning, its basically the auth key for V2 traps, so should we treat it like a password?

EDIT: I read back through all the comments on this PR and found:

I didn't do that because the community is like a password, so I thought it's better to not expose it in TICK scripts

So I'll make the change to treat it like a password.

Other than that the PR to the snmpgo repo was merged over the weekend so we have no blockers.

titilambert · 2017-01-09T18:19:57Z

etc/kapacitor/kapacitor.conf

@@ -210,6 +210,17 @@ default-retention-policy = ""
  # meaning alerts will only be sent if the alert state changes.
  state-changes-only = false

+[snmp]


titilambert · 2017-01-09T18:20:59Z

pipeline/alert.go

+// Example:
+//    [snmptrap]
+//      enabled = true
+//      host = "127.0.0.1:9162"


addr = "127.0.0.1:9162"

titilambert · 2017-01-09T18:21:17Z

pipeline/alert.go

+// Example:
+//    stream
+//         |alert()
+//             .snmpTrap()


You HAVE to put an OID in the snmpTrap Node

titilambert · 2017-01-09T18:22:40Z

@nathanielc Big Thanks !
I just tested it and this is working !
I made some comment about example and docs...

nathanielc · 2017-01-09T21:55:16Z

@titilambert Thanks for the review! I'll merge on green

…com/k-sone/snmpgo subrepo: subdir: "vendor/github.com/k-sone/snmpgo" merged: "1cd4bf6f9" upstream: origin: "https://github.com/k-sone/snmpgo.git" branch: "master" commit: "1cd4bf6f9" git-subrepo: version: "0.3.0" origin: "???" commit: "???"

Snmp Trap service

titilambert mentioned this pull request Sep 20, 2016

Add SNMPtrap service #923

Closed

titilambert force-pushed the snmp branch 4 times, most recently from d0c3519 to 49b2a5a Compare September 21, 2016 17:06

titilambert changed the title ~~[WIP] Snmp Trap service~~ Snmp Trap service Sep 21, 2016

nathanielc suggested changes Sep 21, 2016

View reviewed changes

titilambert force-pushed the snmp branch 2 times, most recently from ee01b91 to 4a0e7b0 Compare September 22, 2016 12:25

nathanielc suggested changes Oct 4, 2016

View reviewed changes

titilambert force-pushed the snmp branch from 28f0c16 to 4db815b Compare December 12, 2016 14:45

titilambert force-pushed the snmp branch 3 times, most recently from 29306aa to a3c9193 Compare December 13, 2016 15:56

nathanielc force-pushed the snmp branch from 484d429 to e2c44d1 Compare January 5, 2017 18:09

nathanielc force-pushed the snmp branch from 95c591b to bf8e558 Compare January 6, 2017 18:40

nathanielc force-pushed the snmp branch from a2dd175 to 5fe4787 Compare January 9, 2017 16:44

nathanielc force-pushed the snmp branch from 5fe4787 to 4aa7f4d Compare January 9, 2017 16:54

nathanielc approved these changes Jan 9, 2017

View reviewed changes

titilambert commented Jan 9, 2017

View reviewed changes

Add snmptrap service

0b18a7b

nathanielc force-pushed the snmp branch from 4aa7f4d to 73249a8 Compare January 9, 2017 21:54

nathanielc added 2 commits January 9, 2017 14:57

rework changes for the new alert system

1cc8c5f

nathanielc force-pushed the snmp branch from 80abd1b to 14d9ee1 Compare January 9, 2017 21:57

nathanielc merged commit 14d9ee1 into influxdata:master Jan 9, 2017

nathanielc added a commit that referenced this pull request Jan 9, 2017

Merged pull request #929 from titilambert/snmp

3dc789b

Snmp Trap service

Snmp Trap service #929

Snmp Trap service #929

Conversation

titilambert commented Sep 20, 2016 • edited Loading

Required for all non-trivial PRs

titilambert commented Sep 21, 2016

nathanielc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanielc Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

titilambert commented Sep 22, 2016 • edited Loading

titilambert commented Sep 26, 2016

nathanielc commented Sep 26, 2016

titilambert commented Sep 27, 2016 • edited Loading

titilambert commented Sep 28, 2016

titilambert commented Sep 28, 2016 • edited Loading

titilambert commented Oct 4, 2016

nathanielc left a comment

Choose a reason for hiding this comment

nathanielc Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

titilambert commented Dec 12, 2016

titilambert commented Dec 13, 2016

nathanielc commented Dec 15, 2016

djsly commented Dec 16, 2016

nathanielc commented Jan 5, 2017

titilambert commented Jan 5, 2017

nathanielc commented Jan 5, 2017

nathanielc commented Jan 6, 2017

nathanielc commented Jan 6, 2017

nathanielc commented Jan 6, 2017

djsly commented Jan 8, 2017

nathanielc commented Jan 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

titilambert commented Jan 9, 2017

nathanielc commented Jan 9, 2017

titilambert commented Sep 20, 2016 •

edited

Loading

nathanielc Sep 21, 2016 •

edited

Loading

titilambert commented Sep 22, 2016 •

edited

Loading

titilambert commented Sep 27, 2016 •

edited

Loading

titilambert commented Sep 28, 2016 •

edited

Loading

nathanielc Oct 4, 2016 •

edited

Loading

nathanielc commented Jan 9, 2017 •

edited

Loading