Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion net/vnstat/Makefile
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
PLUGIN_NAME= vnstat
PLUGIN_VERSION= 1.3
PLUGIN_REVISION= 1
PLUGIN_REVISION= 2
Comment on lines 2 to +3
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

would be nicer to set version 1.4 and remove the revision since this is a big enough change

PLUGIN_COMMENT= Network traffic monitor
PLUGIN_DEPENDS= vnstat
PLUGIN_MAINTAINER= m.muenz@gmail.com
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,42 @@ public function yearlyAction()
return array("response" => $response);
}

/**
* list interfaces tracked by vnstat
* @return array
*/
public function dbiflistAction()
{
$backend = new Backend();
$response = trim($backend->configdRun("vnstat dbiflist"));
if (empty($response)) {
return array("interfaces" => array());
}
$interfaces = array_map('trim', explode("\n", $response));
$interfaces = array_values(array_filter($interfaces, function ($v) {
return !empty($v);
}));
Comment on lines +103 to +106
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

just how unreliable is the dbiflist output?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

i went back and forth on this, whether to use dbiflist vs enumerated opnsense interfaces.

if you previously had different interfaces and replaced your network card, the vnstat database would contain interfaces that don't intersect the current opnsense interfaces. does that matter? is that a concern?

  • the other option is to use the OPNsense interface list. thinking about it now, why would you want a dashboard widget for old interfaces no longer monitored by vnstat?
  • a third option is to only display the intersection of of dbiflist and opnsense interfaces?

i am open to any suggestions / recommendations here

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you dont know what exists or what users want to display, you can make it configurable. Here an example from the certificate widget in core:

https://github.com/opnsense/core/blob/06291661ef1290b2b6c7a30cd18c0d4a563a0cf0/src/opnsense/www/js/widgets/Certificates.js#L187

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

i mean it is configurable.. it shows of list of available interfaces

image

and then you can configure any you want to exclude

image

which i defaulted to non network interfaces like enc0, pflog0, etc.

there are 2 choices where to get the list of interfaces to display:

  • from OPNsense
  • from vnstat database

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry for not responding, I simply don't know anything about vnstat, it would be best if someone with experience here reviews this, e.g., the maintainer or a power user of the plugin.

return array("interfaces" => $interfaces);
}

/**
* retrieve vnstat data as structured JSON for a specific interface
* @return array
*/
public function jsonAction()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this action name is too generic

{
$iface = $this->request->get('iface');
if (empty($iface) || !preg_match('/^[a-zA-Z0-9_.]+$/', $iface)) {
return array("error" => "Invalid or missing interface name");
}
Comment on lines +117 to +119
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this seems spurious.. it's either an interface or not. even better to test the error when asking for garbage input (as an empty string can also be passed safely)

$backend = new Backend();
$response = json_decode($backend->configdRun("vnstat json " . escapeshellarg($iface)), true);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
$response = json_decode($backend->configdRun("vnstat json " . escapeshellarg($iface)), true);
$response = json_decode($backend->configdpRun('vnstat json', [$iface]), true);

should do the same but canonically

if ($response === null) {
return array("error" => "Failed to retrieve vnstat data");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

normally we use ['status' => 'failed'] everywhere. the error message is seldomly shown and not even translated

}
return $response;
}

/**
* remove database folder
* @return array
Expand Down
12 changes: 12 additions & 0 deletions net/vnstat/src/opnsense/service/conf/actions.d/actions_vnstat.conf
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,18 @@ parameters:
type:script_output
message:request Vnstat yearly status

[dbiflist]
command:/usr/local/bin/vnstat --dbiflist 1
parameters:
type:script_output
message:request Vnstat interface list

[json]
command:/usr/local/bin/vnstat --json --iface %s
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the %s here looks misplaced?

parameters:%s
type:script_output
message:request Vnstat json status

[resetdb]
command:rm -rf /var/lib/vnstat
parameters:
Expand Down
25 changes: 25 additions & 0 deletions net/vnstat/src/opnsense/www/js/widgets/Metadata/Vnstat.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<metadata>
<vnstat>
<filename>Vnstat.js</filename>
<endpoints>
<endpoint>/api/vnstat/service/dbiflist</endpoint>
<endpoint>/api/vnstat/service/json</endpoint>
</endpoints>
<translations>
<title>Vnstat Traffic</title>
<h_date>Date</h_date>
<h_rx>RX</h_rx>
<h_tx>TX</h_tx>
<h_total>Total</h_total>
<h_avg_rate>Avg Rate</h_avg_rate>
<period_hourly>Hourly</period_hourly>
<period_daily>Daily</period_daily>
<period_monthly>Monthly</period_monthly>
<period_yearly>Yearly</period_yearly>
<msg_no_data>No vnstat data available</msg_no_data>
<excluded_interfaces>Excluded Interfaces</excluded_interfaces>
<refresh_interval>Refresh Interval (minutes)</refresh_interval>
<show_avg_rate>Show Average Rate</show_avg_rate>
</translations>
</vnstat>
</metadata>
Loading