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 package for Telegraf #189

Merged
merged 15 commits into from
May 9, 2017
Merged

Conversation

wrboyce
Copy link
Contributor

@wrboyce wrboyce commented Sep 16, 2016

No description provided.

@netgate-git-updates
Copy link

Before this pull request can be accepted you must first sign a CLA as described at https://www.pfsense.org/about-pfsense/#cla. Please read for more details.

netgate-git-updates pushed a commit that referenced this pull request Oct 16, 2016
Broker
  Fix TLS operation with websockets listeners and libwebsockets 2.x. Closes
  #186.
  Don.t disconnect client on HUP before reading the pending data. Closes #7.
  Fix some $SYS messages being incorrectly persisted. Closes #191.
  Support OpenSSL 1.1.0.
  Call fsync after persisting data to ensure it is correctly written. Closes
  #189.
  Fix persistence saving of subscription QoS on big-endian machines.
  Fix will retained flag handling on Windows. Closes #222.
  Broker now displays an error if it is unable to open the log file. Closes
  #234.

Client library
  Support OpenSSL 1.1.0.
  Fixed the C++ library not allowing SOCKS support to be used. Closes #198.
  Fix memory leak when verifying a server certificate with a subjectAltName
  section. Closes #237.

Build
  Don.t attempt to install docs when WITH_DOCS=no. Closes #184.

PR:		213532
Submitted by:	ohauer
Approved by:	maintainer <joe@thrallingpenguin.com>
MFH:		2016Q4
@rbgarga
Copy link
Member

rbgarga commented Oct 19, 2016

ops, accidentally closed due to upstream commit log

@rbgarga rbgarga reopened this Oct 19, 2016
@wrboyce
Copy link
Contributor Author

wrboyce commented Oct 19, 2016

Is there anything specifically holding this back from being merged?

rbgarga
rbgarga previously requested changes Dec 5, 2016
@@ -0,0 +1,41 @@
# $FreeBSD$

PORTNAME= pfSense-pkg-Telegraf
Copy link
Member

Choose a reason for hiding this comment

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

All variable assignments in this file should be followed by a TAB instead of spaces, otherwise portlint fails:

FATAL: Makefile: [3]: use a tab (not space) after a variable name
FATAL: Makefile: [4]: use a tab (not space) after a variable name
FATAL: Makefile: [5]: use a tab (not space) after a variable name
FATAL: Makefile: [6]: use a tab (not space) after a variable name
FATAL: Makefile: [7]: use a tab (not space) after a variable name
FATAL: Makefile: [8]: use a tab (not space) after a variable name
FATAL: Makefile: [10]: use a tab (not space) after a variable name
FATAL: Makefile: [11]: use a tab (not space) after a variable name
FATAL: Makefile: [13]: use a tab (not space) after a variable name
FATAL: Makefile: [15]: use a tab (not space) after a variable name
FATAL: Makefile: [17]: use a tab (not space) after a variable name
FATAL: Makefile: [18]: use a tab (not space) after a variable name
FATAL: Makefile: [20]: use a tab (not space) after a variable name
FATAL: Makefile: [21]: use a tab (not space) after a variable name

# $FreeBSD$

PORTNAME= pfSense-pkg-Telegraf
PORTVERSION= 0.0.13
Copy link
Member

Choose a reason for hiding this comment

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

Any special reason to start a new package with version 0.0.13? Why not simply 0.1?

@@ -0,0 +1,78 @@
<?php
require_once("globals.inc");
Copy link
Member

Choose a reason for hiding this comment

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

It's missing license text and copyright message here

<!DOCTYPE packagegui SYSTEM "../schema/packages.dtd">
<?xml-stylesheet type="text/xsl" href="../xsl/package.xsl"?>
<packagegui>
<copyright>...</copyright>
Copy link
Member

Choose a reason for hiding this comment

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

Missing license text and copyright notive


<name>telegraf</name>
<title>Services: Telegraf</title>
<description>...</description>
Copy link
Member

Choose a reason for hiding this comment

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

Missing description

<description>...</description>
<category>Network Management</category>
<include_file>/usr/local/pkg/telegraf.inc</include_file>

Copy link
Member

Choose a reason for hiding this comment

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

Please do not leave blank lines inside XML file

<name>telegraf</name>
<rcfile>telegraf</rcfile>
<executable>telegraf</executable>
<description>...</description>
Copy link
Member

Choose a reason for hiding this comment

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

Missing description

pkg/telegraf.inc
/etc/inc/priv/telegraf.priv.inc
%%DATADIR%%/info.xml
@dir /etc/inc/priv
Copy link
Member

Choose a reason for hiding this comment

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

Missing @dir /etc/inc

doktornotor added 5 commits January 21, 2017 22:35
- Add missing include, sort includes
- Honor $g['varrun_path'] and $g['varlog_path']
- Try to make the config creation code readable
- Other cleanups
@doktornotor
Copy link
Contributor

@wrboyce - Did a PR for the review comments here, kindly merge.

@rbgarga
Copy link
Member

rbgarga commented Feb 22, 2017

@wrboyce can you please merge @doktornotor Pull Request to your fork to bring in all fixes he did?

doktornotor and others added 2 commits February 22, 2017 12:29
@wrboyce
Copy link
Contributor Author

wrboyce commented Mar 15, 2017

@rbgarga merged, apologies for the delay!

@doktornotor thanks again.

<pfsensepkgs>
<package>
<name>Telegraf</name>
<descr><![CDATA[...]]></descr>
Copy link
Contributor

Choose a reason for hiding this comment

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

Still missing description

@@ -0,0 +1 @@
...
Copy link
Contributor

Choose a reason for hiding this comment

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

Still missing description

@wrboyce
Copy link
Contributor Author

wrboyce commented Apr 2, 2017

@doktornotor @rbgarga I believe all review concerns are addressed now.

@rbgarga rbgarga requested a review from jim-p April 12, 2017 11:59
@rbgarga rbgarga dismissed their stale review April 12, 2017 11:59

I'll review again

@rbgarga rbgarga self-requested a review April 12, 2017 11:59
@@ -0,0 +1 @@
Telegraf is an agent written in Go for collecting, processing, aggregating, and writing metrics.
Copy link
Member

Choose a reason for hiding this comment

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

pkg-descr cannot contain lines > 80 columns

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

write_rcfile(array(
"file" => "telegraf.sh",
"start" => "/usr/sbin/daemon -crP {$pidfile} /usr/local/bin/telegraf -config={$conffile} 2>> {$logfile}",
"stop" => "/bin/kill `/bin/cat {$pidfile}`"
Copy link
Member

Choose a reason for hiding this comment

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

What is going to happen if telegraf.log become a huge file? There are no processes taking care of it. Doesn't it support syslog?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't it support syslog?

It appears not. What is the preferred solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated to use 2> for now, while perhaps not ideal it at least mitigates any possible disk-fill issues.

Copy link
Contributor

@jim-p jim-p left a comment

Choose a reason for hiding this comment

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

Looks OK in general, but could benefit from input validation.

Also consider making the "interval" parameter a GUI field.

$cfg .= "\tusername = \"" . $telegraf_conf['influx_user'] . "\"\n";
}
if (!empty($telegraf_conf['influx_pass'])) {
$cfg .= "\tpassword = \"" . $telegraf_conf['influx_pass'] . "\"\n";
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it would fail if the password contains a ". This should probably use addslashes() or similar to allow that to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be fixed now.

@wrboyce
Copy link
Contributor Author

wrboyce commented Apr 12, 2017

I've added some rudimentary escaping of input values, and added interval as a GUI exposed option as requested.

@rbgarga rbgarga requested review from rbgarga and jim-p May 2, 2017 10:29
@rbgarga
Copy link
Member

rbgarga commented May 9, 2017

I'll merge it on 2.3.5-devel and 2.4.0-devel snapshots first, after it's well tested I'll cherry-pick it to 2.3.4-RELEASE

@netgate-git-updates netgate-git-updates merged commit 6845460 into pfsense:devel May 9, 2017
netgate-git-updates pushed a commit that referenced this pull request Sep 13, 2017
Feature request #86: Change meaning of "RequiredHeaders" such that
	header validity is always checked, but messages are only
	rejected on that basis when the flag is set.  Based
	on a patch from Andreas Schulze.
Feature request #127: Log SPF results when rejecting.  Requested
	by Patrick Wagner; patch from Andreas Schulze, follow-up
	patch from Juri Haberland.
Feature request #138: Inculde policy and disposition information
	in an Authentication-Results comment.  Based on a patch
	from Juri Haberland.
Feature request #139: Include the client host name if known
	in failure reports.  Suggested by Roland Turner;
	patch by Andreas Schulze.
Fix bug #95: Assume IPv6 for SPF operations.  Patch from Juri Haberland.
Fix bug #120: Fix control logic around the SPF result.
	Reported by Christophe Wolfhugel; patch from Andreas Schulze.
Fix bug #122: Don't skip the HELO milter phase when SPF is enabled.
	Reported by Christophe Wolfhugel.
Fix bug #157: Fix logging of implicit authserv-ids.  Reported
	by Andreas Schulze; patch from Juri Haberland.
Fix bug #158: Log ignored connections.  Patch from Andreas Schulze.
Fix bug #160: Fix "SyslogFacility" handling.  Patch from
	Juri Haberland.
Fix bug #163: Use a larger buffer for the raw MAIL FROM value.
	Based on a patch from Andreas Schulze.
Fix bug #174: Trim "!" suffixes from reporting addresses.  Problem
	noted by Juri Haberland.
Fix bug #186: When reloading the configuration file, the public
	suffix list was read in with the wrong comment indicator.
	Patch from Federico Omoto.
LIBOPENDMARC: Fix bug #115: Fix type mismatch.  Patch from
	Sebastian A. Siewior via Scott Kitterman.
LIBOPENDMARC: Fix bug #121: Fix IPv6 CIDR matching in SPF code.
	Patch from Christophe Wolfhugel.
LIBOPENDMARC: Fix bug #125: Compile time IPv6 fix.  Reported by
	Christophe Wolfhugel.
LIBOPENDMARC: Fix bug #131: Fix alignment bug.  Patch from
	Andreas Schulze.
LIBOPENDMARC: Fix bug #147: Fix stripping of whitespace from
	DMARC DNS records.  Based on a patch from Job Noorman.
LIBOPENDMARC: Fix bug #149: Apply "sp" setting, if present and
	applicable.  Patch from Petr Novak.
LIBOPENDMARC: Fix bug #154: Fix "rf" and "fo" processing logic.
LIBOPENDMARC: Fix bug #156: Fix variable name.  Patch by
	Andreas Schulze.
LIBOPENDMARC: Fix bug #165: Fix logic in checking which SPF
	identifier was used.  Patches from Marco Favero and
	Juri Haberland.
LIBOPENDMARC: Fix bug #167: Don't return "fail" when we should
	return "none".  Patch from Marco Favero.
REPORTS: Fix bug #134: Handle SMTP errors correctly.  Patch from
	Andreas Schulze.
REPORTS: Fix bug #141: Set the HELO parameter correctly.
	Reported by Alan Smith; patch from Andreas Schulze.
REPORTS: Fix bug #143: Fix logic in table truncation.
	Reported by Wayne Andersen; patch from Juri Haberland.
REPORTS: Fix bug #162: Always report "sp" in aggregate reports.
	Patch from Juri Haberland.
REPORTS: Fix bug #166: Fix report start/end time logic.
	Patch from Juri Haberland.
REPORTS: Fix bug #188: Don't delete inputs too early in
	opendmarc-reports.  Patch from Juri Haberland.
TOOLS: Fix bug #161: "Forensic" reports were renamed "Failure"
	reports.  Patch from Andreas Schulze.
TOOLS: Fix bug #164: Handle IPv6 test addresses.  Reported by
	Andreas Schulze; patch from Juri Haberland.
DOCS: Patch #189: Replace the DMARC RFC with an HTML page
	referencing the relevant specs, since Debian doesn't
	consider RFCs to be "free".  Patch from Scott Kitterman
	via Juri Haberland.

PR:		220902
Submitted by:	Dan Mahoney <freebsd@gushi.org> (maintainer), Lukasz Wasikowski <lukasz@wasikowski.net>
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.

5 participants