NETWAYS / ansible-collection-elasticstack

A collection to install and manage the Elastic Stack
GNU General Public License v3.0
9 stars 8 forks source link

Use elasticsearch_network_host for connection tests if defined #315

Closed ivareri closed 4 months ago

ivareri commented 4 months ago

When elasticsearch_network_host is set to something not including localhost, all connection tests fails.

This uses elasticsearch_network_host for connections if it is defined, and falls back to localhost if not.

(There might be more places this change should be made, but the role works for me with this fix)

widhalmt commented 4 months ago

Thanks for your contribution! The more we can configure, the better. :-)

widhalmt commented 4 months ago

If you want, I can propose a change so the variable will work as a list instead of a string. Then you can fix your PR and we can merge it.

widhalmt commented 4 months ago

When looking at the code, this seems to be even more complicated than I thought. What if someone uses macros like _local_ or _site_ in their elasticsearch_network_host configuration? curl and uri can't use that. Any idea?

widhalmt commented 4 months ago

I opened #317 to work on a larger scale on this.

For now I guess the easiest solution would be if you changed your change request and introduced a new variable like elasticsearch_api_host. Set it to localhost in https://github.com/NETWAYS/ansible-collection-elasticstack/tree/main/roles/elasticsearch/defaults and add a line of documentation to https://github.com/NETWAYS/ansible-collection-elasticstack/blob/main/docs/role-elasticsearch.md .

Maybe we can come up with a way to automatically fill it depending on elasticsearch_network_host automatically.

I really don't like it but if you have a look at #317 I guess you see why I'm struggling with a clean solution.

The reason why I proposed api instead of http or bind_host is that these can also take arrays so we would merely be shifting the problem.

ivareri commented 4 months ago

Well duh, I had a feeling this was too neat of a fix.

I agree, elasticsearch_api_host is ugly, but probably the best solution. I'll update the PR.

ivareri commented 4 months ago

Note: I have not had a chance to do any extensive testing on this.

I changed all the places I found that does connection checks, so it should hopefully be consistent across the role.