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

Add input plugin for OpenBSD/FreeBSD pf #3405

Merged
merged 6 commits into from
Nov 30, 2017
Merged

Conversation

nferch
Copy link
Contributor

@nferch nferch commented Oct 29, 2017

per #2263

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@nferch nferch force-pushed the master branch 4 times, most recently from 02d0ad2 to 7b57521 Compare October 29, 2017 18:22
@danielnelson danielnelson added this to the 1.5.0 milestone Oct 30, 2017
@nferch
Copy link
Contributor Author

nferch commented Nov 16, 2017

Any feedback or things missing from this PR aside from the CLA?

```
> ./telegraf --config telegraf.conf --input-filter pf --test
* Plugin: inputs.pf, Collection 1
> pf,host=columbia entries=2i 1507492593000000000
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct? I'm expecting more fields

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will update

@@ -0,0 +1,3 @@
// +build !freebsd
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove all the build flags and always build the plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

had repeated what had been done in the iptables plugin, but makes sense as pf is on more than FreeBSD.

var anyTableHeaderRE = regexp.MustCompile("^[A-Z]")

func (pf *PF) parsePfctlOutput(pfoutput string, acc telegraf.Accumulator) error {
lines := strings.Split(pfoutput, "\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend using a bufio.Scanner to deal with non-unix line endings. Might make it easier to pass the scanner into child functions too instead of using a index range.

}

func (pf *PF) callPfctl() (string, error) {
c, err := pf.buildPfctlCmd()
Copy link
Contributor

Choose a reason for hiding this comment

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

This should only be ran once on startup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will fix

}
out, oerr := c.Output()
if oerr != nil {
return string(out), fmt.Errorf("error running %s: %s: %s", pfctlCommand, oerr, oerr.(*exec.ExitError).Stderr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to guard on the type assertion



- pf
- entries (integer, count)
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to add the other fields that are collected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will update.


func (pf *StateTable) getTags() ([]string, error) {
tags := []string{}
structVal := reflect.ValueOf(pf).Elem()
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it is worth using reflect in this case, reflect is slow, hard to understand, and there are not so many fields that we can't just do it directly.

@@ -0,0 +1,65 @@
# PF Plugin

The pf plugin gathers counters from the FreeBSD/OpenBSD pf firewall.
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking through the code I don't see where we are parsing the counters, it looks like we are just collecting the state table?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

more precisely, number of entries and counters from the state table. will update.

@nferch
Copy link
Contributor Author

nferch commented Nov 17, 2017

Thanks for the review!

@nferch
Copy link
Contributor Author

nferch commented Nov 17, 2017

Think I addressed all the comments, PTAL.

@danielnelson danielnelson merged commit f92a4f5 into influxdata:master Nov 30, 2017
aromeyer pushed a commit to aromeyer/telegraf that referenced this pull request May 19, 2018
maxunt pushed a commit that referenced this pull request Jun 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants