mgr/influx: Add InfluxDB SSL Option #19374
Conversation
|
Hmm, I have ##19229 open with a bunch more fixes. Should we maybe combine this into one PR? |
src/pybind/mgr/influx/module.py
Outdated
| ssl = False | ||
|
|
||
| # Verify https cert | ||
| verify_ssl = self.get_config("verify_ssl", default="false") |
There was a problem hiding this comment.
Can we make verify_ssl true by default? It feels a bit dangerous to have a "ssl=true" setting that by default doesn't validate the certs.
There was a problem hiding this comment.
the influxdb client also defaults to false
https://github.com/influxdata/influxdb-python/blob/master/influxdb/client.py#L71
symptog
force-pushed
the
master
branch
2 times, most recently
from
a0491a5
to
950ea0c
Compare
src/pybind/mgr/influx/module.py
Outdated
| @@ -45,6 +45,8 @@ class Module(MgrModule): | |||
| 'username': None, | |||
| 'password': None, | |||
| 'interval': 5 | |||
| 'ssl': 'false', | |||
| 'verify_ssl': 'false', | |||
There was a problem hiding this comment.
Shouldn't they both be a boolean instead of a string?
There was a problem hiding this comment.
The get_config command returns strings and the influxdb-client expects bool.
In every case I need to cast to bool.
If I would set the defaults to True I would need one more cast, because:
True == 'true'
False| self.config['ssl'] = ssl.lower() == 'true' | ||
| verify_ssl = \ | ||
| self.get_config("verify_ssl", default=self.config_keys['verify_ssl']) | ||
| self.config['verify_ssl'] = verify_ssl.lower() == 'true' |
There was a problem hiding this comment.
I see you cast from String to Bool here, but how does that work when somebody changes the configuration on runtime?
There was a problem hiding this comment.
I did not recognized this dynamic option. I will fix this.
Do we want verify_ssl to be true by default?
|
Sorry, I was a bit busy with a lot of things. Looking good to me! LGTM |
Add possibility to connect to InfluxDB via https. Also adding the option for verifying the https cert. Signed-off-by: Tobias Gall <tobias.gall@mailbox.org>
Add possibility to connect to InfluxDB via https.
Also adding the option for verifying the https cert.