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 new iptables plugin #1471

Closed
wants to merge 1 commit into from
Closed

Add new iptables plugin #1471

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 9, 2016

Required for all PRs:

  • CHANGELOG.md updated
  • Sign CLA (if not already signed)
  • README.md updated (if adding a new plugin)

The iptables plugin aims at monitoring bytes and packet counters
matching a given set of iptables rules.

Typically the user would set a dedicated monitoring chain into a given
iptables table, and add the rules to monitor to this chain. The plugin
will allow to focus on the counters for this particular table/chain.

@ghost
Copy link
Author

ghost commented Jul 9, 2016

Hello,
Where should I put the changelog info, everything looks ready for 1.0 release ... Should I put it there too ?
Cheers.

if len(mv) == 0 || len(mv[0]) != 5 {
continue
}
tags := map[string]string{"table": ipt.Table, "chain": mchain[1], "ruleid": strconv.Itoa(i + 1)}
Copy link
Contributor

@phemmer phemmer Jul 9, 2016

Choose a reason for hiding this comment

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

I don't think using the line number as an identifier tag is a good idea. From the perspective of someone looking at the data in influxdb (or wherever the telegraf data is going), ruleid=4 is going to mean next to nothing. Also, if another iptables rule is inserted, then line 4 becomes line 5, and tracking the metrics over time gets screwed up. It also may not be consistent between hosts (one host's rule 4 may not be the same as another host's rule 4).

Copy link
Author

Choose a reason for hiding this comment

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

I agree. I am using comments exclusively myself to identify the rules. I considered using the rule number as a fallback for rules that do not have a comment. For the sake of clarify I suggest to ignore rules that do not have a comment. What do you think ? Any other suggestion to distinguish a rule from another ?

Copy link
Contributor

@sparrc sparrc Aug 11, 2016

Choose a reason for hiding this comment

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

@ririsoft is it common not to have a comment? I'm fine with ignoring uncommented lines as long as it's not too huge of a burden on users, though it will need to be documented

Copy link
Author

Choose a reason for hiding this comment

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

@sparrc: I have no idea if it is common or not. I can add an option to let the user ignore rules without comment or not as he prefer. If rules without comment are to be included then the ruleid will be the rule number. What do you think ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure the "rule id" should ever be used, that seems like it can only lead to confusion among users, and can also lead to bad data when rules are added/deleted. If there is no way to uniquely identify it then I think it's best to skip it.

@ghost
Copy link
Author

ghost commented Aug 18, 2016

I am preparing a new version of the plugin that will:

  • Discard rules without a comment
  • Update the documentation with information for using systemd capabilities to run the plugin without the need for root or sudo. It will let the possibility to run iptables through sudo though, for users who do not want to give too much capabilities to telegraf.

It will be available next week.

@sparrc
Copy link
Contributor

sparrc commented Aug 19, 2016

sounds great, thank you @ririsoft

@ghost
Copy link
Author

ghost commented Aug 22, 2016

Pull request is now completed. The failing CI does not sound related to this development.
Do not hesitate to ask for a rebase and/or squash and if you would like an update in the changelog too.
Cheers.

@sparrc
Copy link
Contributor

sparrc commented Aug 30, 2016

OK, thanks @ririsoft, can you:

  1. rebase your changes
  2. update the CHANGELOG (under release 1.1)
  3. add your plugin to the list of available plugins on the main repo README?

The iptables plugin aims at monitoring bytes and packet counters
matching a given set of iptables rules.

Typically the user would set a dedicated monitoring chain into a given
iptables table, and add the rules to monitor to this chain. The plugin
will allow to focus on the counters for this particular table/chain.
@ghost
Copy link
Author

ghost commented Aug 30, 2016

Sorry I messed up with the CHANGELOG.md but it should be ok now: I rebased and squashed all the porcelain. Thank you very much for your kind help on this PR.

@sparrc sparrc closed this in 094eda2 Aug 31, 2016
@ghost ghost deleted the iptables branch August 31, 2016 11:22
jackzampolin pushed a commit that referenced this pull request Sep 1, 2016
The iptables plugin aims at monitoring bytes and packet counters
matching a given set of iptables rules.

Typically the user would set a dedicated monitoring chain into a given
iptables table, and add the rules to monitor to this chain. The plugin
will allow to focus on the counters for this particular table/chain.

closes #1471
@ghost ghost mentioned this pull request Mar 1, 2017
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.

None yet

2 participants