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

Ipset input plugin #3346

Merged
merged 9 commits into from Jan 31, 2018
Merged

Ipset input plugin #3346

merged 9 commits into from Jan 31, 2018

Conversation

sajoupa
Copy link
Contributor

@sajoupa sajoupa commented Oct 17, 2017

Required for all PRs:

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

The ipset input plugin allows collecting metrics from Linux ipset counters.

@danielnelson danielnelson added this to the 1.6.0 milestone Dec 1, 2017
There are 3 ways to grant telegraf the right to run ipset:
* Run as root (strongly discouraged)
* Use sudo
* Configure systemd to run telegraf with CAP_NET_ADMIN and CAP_NET_RAW capabilities.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this will work? Are capabilities inherited?

## set show_all_sets = true to gather them all.
show_all_sets = false
## Adjust your sudo settings appropriately if using this option ("sudo ipset save")
## TODO: can we replace this with systemd privileges ? CAP_NET_ADMIN should DTRT
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this answers my question above, if we don't know if this will work don't add it.

}

lines := strings.Split(list, "\n")
for _, line := range lines {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use an bufio.Scanner to iterate over lines

continue
}

data := strings.Split(line, " ")
Copy link
Contributor

Choose a reason for hiding this comment

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

strings.Fields(line)

}

data := strings.Split(line, " ")
if data[0] == "add" && (data[4] != "0" || ips.ShowAllSets == true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will panic if not enough fields, check for the correct number of splits.

return string(out), err
}

type setLister func() (string, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Define this above with the struct

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

Choose a reason for hiding this comment

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

You can remove all of the build flags, it won't hurt to allow this to be built on all platforms.

}

for i, tt := range tests {
i++
Copy link
Contributor

Choose a reason for hiding this comment

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

fields [][]map[string]interface{}
err error
}{
{ // 1 - 0 sets => no results
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a name field to the test struct and set the names based on these comments using subtests

[[inputs.ipset]]
## By default, we only show sets which have already matched at least 1 packet.
## set show_all_sets = true to gather them all.
show_all_sets = false
Copy link
Contributor

Choose a reason for hiding this comment

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

I would call this include_unmatched_sets

 - Add a timeout for ipset execution
 - Use a bufio Scanner to parse ipset output
 - Add names to test cases
 - Remove build flags
@sajoupa
Copy link
Contributor Author

sajoupa commented Jan 23, 2018

Thanks @danielnelson for the review.
The commit I just pushed addresses all the issues you pointed.
Also, I can confirm that systemd capabilities allow the service to run as intended, with an unprivileged user.

include_unmatched_sets = false
## Adjust your sudo settings appropriately if using this option ("sudo ipset save")
use_sudo = false
## The default timeout of 1s for ss execution can be overridden here:
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace ss with ipset, here and also remember to update the same in the README

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! Fixed.

@danielnelson
Copy link
Contributor

What about this comment: #3346 (comment)

@sajoupa
Copy link
Contributor Author

sajoupa commented Jan 31, 2018

@danielnelson danielnelson merged commit 7b36518 into influxdata:master Jan 31, 2018
@danielnelson
Copy link
Contributor

Ah, sorry was confused by the comment not collapsing. I merged this in and it will be included in 1.6, thanks!

@sajoupa sajoupa mentioned this pull request Feb 13, 2018
3 tasks
maxunt pushed a commit that referenced this pull request Jun 26, 2018
@sajoupa sajoupa deleted the ipset-input-plugin branch February 2, 2022 07:14
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.

None yet

2 participants