Skip to content

alloy_readiness_check_use_https #359

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

Merged
merged 11 commits into from
May 9, 2025

Conversation

piotr-g
Copy link
Contributor

@piotr-g piotr-g commented May 5, 2025

Configure protocol schema of the ready endpoint

@CLAassistant
Copy link

CLAassistant commented May 5, 2025

CLA assistant check
All committers have signed the CLA.

@voidquark
Copy link
Collaborator

Can you also add the variable to the README and explain that it only enables validation for https, since the actual TLS configuration must be provided in the main configuration file itself?

@voidquark
Copy link
Collaborator

@piotr-g

I was considering the variable name, but ansible_server_tls_enabled could be misleading for some users. In reality, this variable does not enable TLS in Alloy. It only affects how the readiness check is performed.

That's why I prefer a name that clearly reflects its actual purpose: alloy_readiness_check_use_https.

For the README, I have also suggestion:
This boolean variable determines whether the readiness check for the Alloy server should use HTTPS or HTTP when validating the /-/ready endpoint. This variable does not enable TLS on the Alloy server itself.

Once this is in place, I think we're good to merge 🚀

@piotr-g
Copy link
Contributor Author

piotr-g commented May 8, 2025

@piotr-g

I was considering the variable name, but ansible_server_tls_enabled could be misleading for some users. In reality, this variable does not enable TLS in Alloy. It only affects how the readiness check is performed.

That's why I prefer a name that clearly reflects its actual purpose: alloy_readiness_check_use_https.

For the README, I have also suggestion: This boolean variable determines whether the readiness check for the Alloy server should use HTTPS or HTTP when validating the /-/ready endpoint. This variable does not enable TLS on the Alloy server itself.

Once this is in place, I think we're good to merge 🚀

You're right I just used the same variable to enable the server. Besides that the check will fail if the var is set to true and tls is not configured for the internal server like that:

{% if alloy_server_tls_enabled %}
http {
  tls {
    cert_file = "/etc/ssl/certs/{{ ansible_hostname }}-chain.pem"
    key_file  = "/etc/ssl/private/{{ ansible_hostname }}.key"
  }
}
{% endif %}

I'm going to change the var name and add suggested comment.

@piotr-g piotr-g changed the title alloy_server_tls_enabled alloy_readiness_check_use_https May 9, 2025
@piotr-g piotr-g requested a review from voidquark May 9, 2025 10:26
@voidquark
Copy link
Collaborator

Looks good now. Thanks for your contribution 🚀

@voidquark voidquark merged commit f195bda into grafana:main May 9, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants