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

Parameter list for lib/database.php:db_connect_real() is not correct in 3 places #4423

Closed
ctrowat opened this issue Oct 14, 2021 · 10 comments
Closed
Labels
bug Undesired behaviour resolved A fixed issue
Milestone

Comments

@ctrowat
Copy link
Contributor

ctrowat commented Oct 14, 2021

Describe the bug

/pollers.php:test_database_connection()
/lib/poller.php:poller_connect_to_remote()
/cactid.php:db_check_reconnect()

All do not have the correct parameter list when they call /lib/database.php:db_connect_real()
The change where SSL parameters were added to the function signature was not properly reflected in these call sites. This causes issues configuring remote pollers that use SSL for their database connections.

Call sites are here:
https://github.com/Cacti/cacti/blob/1.2.x/pollers.php#L766
https://github.com/Cacti/cacti/blob/1.2.x/lib/poller.php#L1132
https://github.com/Cacti/cacti/blob/1.2.x/cactid.php#L263

The first 2 call sites simply need the retries parameter slotted in, but the final one seems to predate the addition of the extra SSL parameters and that might be harder to fix.

@ctrowat ctrowat added bug Undesired behaviour unverified Some days we don't have a clue labels Oct 14, 2021
@TheWitness
Copy link
Member

Boy it would be nice if you could get a pull request together in the next 24 hours....

@ctrowat
Copy link
Contributor Author

ctrowat commented Oct 16, 2021

I thought the same, but when trying to put one together I wasn't sure how to properly get the parameters to use within cactid.php, and then started questioning all of the logic and default values for the number of retries for local and remote databases. Sometimes the defaults are 2, 5 or 20 and I have no strong feelings about what's reasonable. Further to that, does it make sense for the retries count to be the same for the remote poller as the main poller trying to connect to the remote poller's database? Also, there's no place in the poller table for the retries count so it requires database changes and that's why I feel completely out of my depth. Sorry for implying it was easy because after closer inspection it's not an easy fix.

For our cases I just hard-coded the retries count at 2 and ignored cactid since we don't use it but I'll be perfectly honest, it's a pretty low effort weak solution.

@TheWitness
Copy link
Member

@ctrowat, that's way too pithy for a Friday night ;) The global vs. the distributed nature of settings is something that I generally think of when someone tells me a story about how it would have been better if such and such were to happen during data collection. Situations like that are the well spring of new ideas. What I admire is that you took a deep dive, deep enough to find idiosyncrasies in the design (bugs), which I will use to do a review tomorrow. We are a small team, and everyone already has a full time job.

@ctrowat
Copy link
Contributor Author

ctrowat commented Oct 16, 2021

Thanks so much for looking into it. Maintaining a project like this can be so thankless and we really appreciate all the work that has gone in here.

I started my day today convinced that I could write up a quick pull request and hopefully have this fixed up in no time and when I got all the way down to the pollers table I had to throw in the towel. I'm a C#/.NET guy at heart and my PHP isn't good enough for me to put forward something I'd feel comfortable suggesting gets merged.

@TheWitness
Copy link
Member

@ctrowat, if you stick with it long enough, you will come to love it. I've programmed in literally dozens of languages throughout my career, and though some other languages have some minor advantages, when you combine the database API with the web services (aka LAMP), with all the front end frameworks today, and then add to it the robustness of those frameworks, php is just lights out not just an amazing language, but a framework of creative development.

I will also note that my son build webpages using an Adobe framework that is hands down "amazing" when it comes producing raw/visceral and dynamic web content. However, Cacti does not play in that space. It's a hammer, though a sophisticated one, and it only has to be so pretty.

@netniV
Copy link
Member

netniV commented Oct 16, 2021

And I on the otherhand disagree... I prefer languages to be properly typed, so to me PHP is more a script language like JavaScript or Powershell where variables can be any type. PHP is the equivalent of VB6 👍

PHP was a secondary language to me, but is one of the main ones I use these days aside from C# or JavaScript. It has it's place in the world just like everything.

We have been given more and more reviews of our code lately which is always good and was how I started out questioning everything @TheWitness had written hehe. I still will be the first to admit, I don't know all the ins and outs of the system but reviewing the code extends that as I ask more questions each time I submit more changes.

Sometimes people can read code well enough to question things, and that to me is an important aspect. Even if they have a wrong understanding it makes sure we know what we understand. They can't always submit changes and that's why we have to do it on their behalf sometimes but as @TheWitness said, we already have full time jobs which can be 30, 40, or 60+ hours to bring in our own paycheck before we turn our eyes back to cacti.

So, when people share their appreciation, we appreciate it too.

For now though, I've waffled too much, time to see what's changed.

@TheWitness
Copy link
Member

@netniV, Typing is one of those little things, and cloning a database connection after a fork present in perl but not in PHP, and more recently the lack of correct signal handling in PHP, there are others.... But proper Typing is for wimps ;) Going to be a nice day here today. Not sure how long I'm going to sit in front of this monitor.

@TheWitness
Copy link
Member

Testing went well so far, but need to create a simple perms for graph_templates as well. Not sure whey it was not created to begin with.

TheWitness added a commit that referenced this issue Oct 16, 2021
Parameter list for lib/database.php:db_connect_real() is not correct in 3 places
TheWitness added a commit that referenced this issue Oct 16, 2021
@TheWitness TheWitness added the resolved A fixed issue label Oct 16, 2021
@TheWitness TheWitness added this to the v1.2.19 milestone Oct 16, 2021
@TheWitness TheWitness removed the unverified Some days we don't have a clue label Oct 16, 2021
@TheWitness
Copy link
Member

Okay, tested this and have done upgrade testing. Not for a full install. Should be all verified in the next hour or so.

TheWitness added a commit that referenced this issue Oct 16, 2021
You need to be able to save the value ;)
@TheWitness
Copy link
Member

  1. Upgrade test passed
  2. Install test passed
  3. Edit/Save passed
  4. Audit passed

TheWitness added a commit that referenced this issue Oct 16, 2021
This is for the remote poller install.  Missed that in my original QA's.
@github-actions github-actions bot locked and limited conversation to collaborators Jan 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Undesired behaviour resolved A fixed issue
Projects
None yet
Development

No branches or pull requests

3 participants