server vs. interface db_mysql.inc.php

Recently mysql flag support was added back to server/lib/classes/db_mysql.inc.php, but not to interface/lib/classes/db_mysql.inc.php, and I'm looking at the differences between those, and have questions.

I wonder if those two could be made identical for simplicity.

The differences I see are:
- the interface one supports a simple db table prefix used for importing dns/vpop, easy enough to add to the server version
- it also has a securityScan() function, which may not be a bad idea to use in server anyways? or would likely not be a problem if that function were never called
- the server one has utility function getDatabaseSize(), which is probably harmless for interface, as it won't be used
- interface has an unused function insertFromArray(), can probably just drop it?
- server has datalogError(), interface has datalogStatus()

Most of the other differences seem pretty straightforward, but would anyone familiar with those mind running a quick diff against the two and see if anything important is overlooked? And/or is it likely not even worth the effort to consolodate those?

Thanks...

 

I agree that we should try to unify the two MySQL libs for easier maintenance. The insertFromArray function is sued by some addon's, so we should not remove it. Adding securityScan to the server version should be fine, but we'll have to test it if there are queries that fail on the server when in use and modify them if required or partially disable the security scan for such queries. For the other differences, it should be fine to add them to the 'other' version even if not used in the opposite part.

 

Looks like the installer has its own version, too (install/lib/mysql.lib.php), which of course makes sense, but probably should also make that work as the identical file.

For the interface/server, would you prefer a single file that's included by both (and if so, a preferred location for that file)? Or just keep the file includes/locations as they are and simply have a duplicate copy of db_mysql.inc.php ?

 

I would prefer to keep the interface and server part self contained which means that we should duplicate the file. But we should add a note in the file that edits have to be made in two locations.

In regard to the installer mysql lib, it is probably quite different from the other two files, but it should be possible to use the same lin that is used for interface and server there too.

 

the 'interface' version has this str_replace() which is not in 'server': https://git.ispconfig.org/ispconfig...r/interface/lib/classes/db_mysql.inc.php#L131

Do you know if that's needed for something in the one and not the other, or anything about that offhand?

 

That should be added to the server version as well. I noticed that the function "public function query_all_array($sQuery = '') {" is correct in server, but not on the interface.

 

one more question, check the two db::tableInfo() versions, 'interface' calls $app->db->queryAllRecords(), where 'server' calls $go_api->db->queryAllRecords() - I don't see $go_api defined anywhere at all, so I'm guessing the 'interface' version is correct? It looks like tableInfo() is never called from 'server' code, so maybe $go_api is defined in an addon like insertFromArray above.

Thanks...

 

The one from interface must be correct as the base object is $app. $go_api is used in ISPConfig 2, it might be an old, maybe even unused, function.

 

The db_new_link option seems to be unused and I think unnecessary as the link is encapsulated within the db object, so "new db(whatever)" is always a new link - do you forsee any problems (eg. in external code) with removing it ?

The prefix in "interface" is only used in 2 places, the powerdns and vpopmail importers, and only as a means to pass alternate db connection parameters - I'll rewrite the importers to just pass db connection info in the "new db(blah)", but again, is there any external code that needs that prefix to remain in place? If not I'll just drop it, as it seems to be just a workaround that's no longer needed. (And come to think of it, any place which does use that prefix will need updated anyways, because the function arguments changed, so likely it can be dropped.)

 

Looks like server/plugins-available/mysql_clientdb_plugin.inc.php has a set of db connection/query code all its own and I've not looked into updating, so my guess is you couldn't require SSL for the dbispconfig user on a database server, or it won't be able to create/manage databases and database users. Other than that, I think I'm pretty close to sending a pull request to merge those two files.

I've done some testing and fixed every issue I've found, but this touches everyone's db connection logic, so make sure some other folks test this good, too.

I have only updated the stable-3.1 branch so far, which is what I'm using and is more or less the newest. I might try to apply all changes against master and see how much cleanup / fixing is required, but I don't have master installed anywhere to actually test things.

 

Thank you very much for your efforts!

The plugin is always installed on the same server that manages the databases (localhost), so I wonder if it's really necessary to enable SSL there.

No need to update the master, all changes are migrated from stable to master branch from time to time.

 

The main reason I can think of would be if someone wanted to make SSL required for their mysql users; they couldn't do that with the current plugin code (because it doesn't use client flags, so it won't ever connect with ssl).

On a related note (of requiring ssl for mysql users), in my testing (debian 9, default mariadb with yassl, not openssl) you cannot use ssl on the mysql unix socket, but if you change the host from 'localhost' to '127.0.0.1' it'll use tcp and ssl works fine then.

Another benefit of using the db class in that plugin is the reconnect/retry logic, though I'd guess retries are already handled by ispconfig's datalog/cron system (if creating the db fails, it'll be retried at the next cron run?).