Check if the options are not empty if specified by cacti

[deniskin82]

No description provided.

[dbmiller]

That should work for username, password, and port. How does the the script behave if --server-id is passed with an empty string? Looks like if it is present it will trigger the heartbeat delay check. It doesn't look like it would break anything, but it is an unneeded query.

[deniskin82]

The script behaves fine because we actually don't use --server-id in our setup, simple master-slave. I think it won't work in case if heartbeat is TRUE. I'm going to update the script also to check if server-id is empty.

[dbmiller]

Think that takes care of the matter at hand. Tested and working.

[cezmunsta]

@deniskin82 @dbmiller validation of the input takes place in validate_options.

@deniskin82 please apply the following patch to your branch and test:

diff --git a/cacti/scripts/ss_get_mysql_stats.php b/cacti/scripts/ss_get_mysql_stats.php
index 126e080..983853f 100644
--- a/cacti/scripts/ss_get_mysql_stats.php
+++ b/cacti/scripts/ss_get_mysql_stats.php
@@ -183,6 +183,14 @@ function validate_options($options) {
          usage("Required option --$option is missing");
       }
    }
+
+   # Safety-net for missing values
+   foreach ( array('server-id', 'port', 'user') as $option ) {
+      if ( array_key_exists($option, $options) && empty($options[$option]) ) {
+         usage("Option --$option requires a value");
+      }
+   }
+
    foreach ( $options as $key => $val ) {
       if ( !in_array($key, $opts) ) {
          usage("Unknown option --$key");
@@ -252,14 +260,14 @@ function ss_get_mysql_stats( $options ) {
           $mysql_connection_timeout,
           $heartbeat, $heartbeat_table, $heartbeat_server_id, $heartbeat_utc;
 
-   $user = isset($options['user']) && !empty($options['user']) ? $options['user'] : $mysql_user;
-   $pass = isset($options['pass']) && !empty($options['pass']) ? $options['pass'] : $mysql_pass;
+   $user = isset($options['user']) ? $options['user'] : $mysql_user;
+   $pass = isset($options['pass']) ? $options['pass'] : $mysql_pass;
    $host = $options['host'];
-   $port = isset($options['port']) && !empty($options['port']) ? $options['port'] : $mysql_port;
+   $port = isset($options['port']) ? $options['port'] : $mysql_port;
    $socket = isset($options['socket']) ? $options['socket'] : $mysql_socket;
    $flags = isset($options['flags']) ? $options['flags'] : $mysql_flags;
    $connection_timeout = isset($options['connection-timeout']) ? $options['connection-timeout'] : $mysql_connection_timeout;
-   $heartbeat_server_id = isset($options['server-id']) && !empty($options['server-id']) ? $options['server-id'] : $heartbeat_server_id;
+   $heartbeat_server_id = isset($options['server-id']) ? $options['server-id'] : $heartbeat_server_id;
 
    $sanitized_host = str_replace(array(":", "/"), array("", "_"), $host);
    $cache_file = "$cache_dir/$sanitized_host-mysql_cacti_stats.txt" . ($port != 3306 ? ":$port" : '');

That will produce the following:

 ⇒ php cacti/scripts/ss_get_mysql_stats.php --host x --items x --server-id | head -n1
Option --server-id requires a value

[dbmiller]

Wow, if that function were a snake it would have bit me ...

@cezmunsta
Unfortunately, while the placement of the verification code in validate_options() makes sense, the logic in that section is if an option is not validated, then call usage(). The function usage() outputs the passed message along with the usage text, then terminates the script. This is very appropriate for a standalone script where the omitted values would lead to things not working, but it won't address the root cause in #40 and #46 in which the input method for cacti is passing empty argument values for which default values are present in the script.

The input method specification in Cacti takes the entire command line and has the placeholders for the option values. It appears that Cacti does a simple replacement for the placeholder, but has changed how it handles unspecified values. In 0.8.x, Cacti would send the unquoted string NULL for empty arguments which be translated by PHP as a literal NULL instead of the string 'NULL', consequently failing the isset() condition in ss_get_mysql_stats() and cause the internal value to be set to the script defaults. In v1.x, it now quotes the empty string value ('') which triggers the the isset() conditional causing the passed empty strings to be assigned to the internal variables.

Use of the patch "as-is" would remove the ability to use the .conf or in-script specification of options common to all monitored instances using the default input method specification. Additionally, the changes to the input method required to use the in-script data would remove the ability to have exceptions to the defaults. This may be desireable to bring the script in line with the Cacti documentation, but it will require changes for all users which have opted to store certain data in the script instead of within Cacti, regardless of Cacti version, as well as require changes to the documentation for the plugin (see the Configuration section on Installing Percona Monitoring Plugins for Cacti.

So I guess the larger issue at hand is whether the project should continue to support the documented configuration process, likely by moving the variable assignment decisions for $user, $pass, $port, $socket, $flags, $connection_timeout, and $heartbeat_server_id present in ss_get_mysql_stats() to either validate_options() or a new function invoked prior to it, or update the configuration process to bring it in line with Cacti Documentation and note what limitations exist on the use of, and which modifications to the template are necessary to use, default values present in the script.

[CountBrass]

What's the status on this, I really, really need to be able to graph MySQL stats for a couple of critical DBs.

[deniskin82]

@cezmunsta the only requirement is hostname and the rest are allowed to be empty and will/should be replaced by their default variables. Cacti uses ' ' single quotes to wrap a fields, from the log:

2017-07-21 13:50:13 - POLLER: Poller[1] Device[61] DS[2969] CMD: /usr/bin/php -q /var/www/cacti/scripts/ss_get_mysql_stats.php --host 'db04' --items gx,gy,gz,hg --user '' --pass '' --port '3316' --server-id '', output: gx:12184241176 gy:9027129348 gz:19951000184 hg:8775356769

That's what I have in data method input in Cacti:

<path_php_binary> -q <path_cacti>/scripts/ss_get_mysql_stats.php --host <hostname> --items gx,gy,gz,hg --user <username> --pass <password> --port <port> --server-id <serverid>

[gattancha]

Been having the same issue in 1.1.7 with MariaDB..
As I use the /etc/cacti/ss_get_mysql_stats.php.cnf to set a username and password, I simply removed
--user --pass --port --server-id
lines from the actual template and re-imported..
All now fine

Looks like it was still trying to pass the empty values rather than skipping them

I suspect that if you specifiy these values from within Cacti you'll be fine, but if you do it from config files, you'll need to clear those lines.

[JDavidRogers]

I was also having trouble with Cacti 1.1.28 passing empty parameters to the 1.1.7 ss_get_mysql_stats.php script, using PHP 5.6.32 and MariaDB 10.1.29. The script was checking them in the ss_get_mysql_stats() function with isset(), which returned true even though they were blank. I changed those isset() lines to use !empty() instead and the variables are now being correctly assigned from the configuration values.

function ss_get_mysql_stats( $options ) {
   # Process connection options.
   global $debug, $mysql_user, $mysql_pass, $cache_dir, $poll_time, $chk_options,
          $mysql_port, $mysql_socket, $mysql_flags,
          $mysql_ssl, $mysql_ssl_key, $mysql_ssl_cert, $mysql_ssl_ca,
          $mysql_connection_timeout,
          $heartbeat, $heartbeat_table, $heartbeat_server_id, $heartbeat_utc;

   $user = !empty($options['user']) ? $options['user'] : $mysql_user;
   $pass = !empty($options['pass']) ? $options['pass'] : $mysql_pass;
   $host = $options['host'];
   $port = !empty($options['port']) ? $options['port'] : $mysql_port;
   $socket = !empty($options['socket']) ? $options['socket'] : $mysql_socket;
   $flags = !empty($options['flags']) ? $options['flags'] : $mysql_flags;
   $connection_timeout = !empty($options['connection-timeout']) ? $options['connection-timeout'] : $mysql_connection_timeout;
   $heartbeat_server_id = !empty($options['server-id']) ? $options['server-id'] : $heartbeat_server_id;

[AllenRDCo]

Is there a reason this has not been introduced yet? I spent several hours trying to figure out why all my cacti MySQL graphs stopped working after updating to 1.1.8 (since we needed MySQL 5.7 support and 1.1.7 and prior do not support 5.7 apparently) and finally found this and the changes in the PR fixed the graphing. We too are using the ss_get_mysql_stats.php.cnf to pass in the mysql credentials. We're using Cacti 1.1.38 from the Debian Stretch backports.

[netniV]

The empty parameter problem affects a few of the scripts from persona when called from cacti. I created a PR myself but hadn’t seen this one back in August. They have not answered EITHER of these PRs it would seem.

[netniV]

#98

[cezmunsta]

The empty parameter problem affects a few of the scripts from persona when called from cacti. I created a PR myself but hadn’t seen this one back in August. They have not answered EITHER of these PRs it would seem.

@netniV Cacti support is likely to be removed from this project very soon. The Percona monitoring offering is now PMM - you can view an online demo

[netniV]

Can I ask why the support will be removed? There are plenty of users who use both.

[netniV]

I have now closed my PR since that support appears to have been removed though we have had users raise the issue. Slightly disappointed there was no response to the previous question, but if there is no support, this issue should also be closed.