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

[helm] Support getting data-source-name from an existing secret #396

Closed
allanger opened this issue Nov 17, 2023 · 12 comments
Closed

[helm] Support getting data-source-name from an existing secret #396

allanger opened this issue Nov 17, 2023 · 12 comments
Labels
enhancement New feature or request in progress

Comments

@allanger
Copy link
Contributor

Context

We are using db-operator that manages our databases, and it is also generating different connection strings for databases and puts them in the k8s secret.

Currently, after a secret is created, we need to get the data from it, put it to the sql_exporter values and then deploy the exporter.

Solution

It would be solved if a helm chart could mount a secret from k8s to the sql_exporter deployment and use it either as a part of config file, or just as a flag.

First solution would require building the config during the startup (most probably with an init container), and it doesn't sound like something that is easy to create and maintain

Second one would require a parameter builder in _helpers.tpl, and conditional secret mounts in the deployment, not that bad, I guess. But it also have a drawback, that if you jump into container and run ps, you will most probably see the whole connection string to a database, that also doesn't sound nice

Another option would be just to wait until this feature is released: #186
Than we could just use env vars for everything. I guess, it would be the best solution, actually

Notes

I'm going to work on that myself, once we understand which of solutions works best.

@allanger allanger added the enhancement New feature or request label Nov 17, 2023
@allanger allanger changed the title [help] Support getting data-source-name from an existing secret [helm] Support getting data-source-name from an existing secret Nov 17, 2023
@burningalchemist
Copy link
Owner

Hey @allanger,

Currently, there's SQLEXPORTER_TARGET_DSN environment variable that's used for DSN. This only works for a single-target mode, though. Maybe you could try to do something around this env if you think it makes sense.

Apart from that, yeah, here's the next milestone for v0.14. I think I'll start working on it next year (or by the end of this year). One of three features in the list is already implemented. So I could prioritise #186 then.

Could you please share which env variables you would use in the last solution?

Cheers!

@allanger
Copy link
Contributor Author

Ah, I guess I've missed the SQLEXPORTER_TARGET_DSN var. I guess I can just use it, and my problem will be solved, cause I'm only using single-target mode. But I think in general, it would be nice to support multi-target as well, though I'm not sure how

@burningalchemist
Copy link
Owner

Cool, I think it would still be better for the majority of cases. 👍

For multi-target mode, users need to provide way more information in one place, so integrating it with solutions like db-operator looks complicated from the beginning anyways. I have some ideas on the env-driven configuration, but need to prototype them first. So I think Helm integration for multi-targets makes sense to look at after #186 is delivered.

@allanger
Copy link
Contributor Author

We can think about adding some integration for db-operator, since I'm one of maintainers there :)

@burningalchemist
Copy link
Owner

Sounds good to me!

Time-wise, I'd like to finish v0.14 and then declare a feature freeze to work on test coverage, and finish with some static web site with a better documentation approach. So, if there's anything needed from my end to be supported by db-operator, please let me know (or feel free to drop some thoughts under Discussions/Ideas 👍 ).

@lhaussknecht
Copy link

Funny - yesterday I started on a PR for exactly this usecase :)

@allanger
Copy link
Contributor Author

I might be wrong, since checking from the phone. But are mounting the whole config from the existing secret? If so, it's not resolving the issue, because I need to set the connection string only.

A config from a secret is also something in wanted to add, but it's not what we actually need

@lhaussknecht
Copy link

lhaussknecht commented Nov 17, 2023 via email

@burningalchemist
Copy link
Owner

Cool! I guess there's no overlap then. 👍

@allanger
Copy link
Contributor Author

Actually, I'd say that this issue can be closed, because my problem was solved by #399

But should we wait for #397 ?

@burningalchemist
Copy link
Owner

burningalchemist commented Dec 11, 2023

Hey @allanger, let's keep this issue open since it's somewhat linked with #397 as well.
As for #397, I was carried away by my daily job, and couldn't arrange time to proceed - there are multiple deadlines at the end of the year. 🙂 I'll try to respond and suggest something sooner, so we can merge the PR. 👍

@burningalchemist
Copy link
Owner

Resolved by #399 and #446.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants