Alerting: Add options to configure TLS for HA using Redis #87567

fayzal-g · 2024-05-09T15:47:37Z

What is this feature?
Updates the unified_alerting configuration with additional fields to configure TLS on the Redis client when enabling alerting high availability using Redis.

The following fields have been added:

ha_redis_tls_enabled
ha_redis_tls_cert_path
ha_redis_tls_key_path
ha_redis_tls_ca_path
ha_redis_tls_server_name
ha_redis_tls_insecure_skip_verify
ha_redis_tls_cipher_suites
ha_redis_tls_min_version

Why do we need this feature?
Some users may have an instance of Redis that is only able to accept communications with TLS but it is currently not possible to configure client-side TLS on the Grafana side

Special notes for your reviewer:

Please check that:

It works as expected from a user's perspective.
If this is a pre-GA feature, it is behind a feature toggle.
The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

fayzal-g · 2024-05-10T14:44:27Z

conf/sample.ini

+# The maximum number of simultaneous redis connections.
+# ha_redis_max_conns = 5


Not related to this change but added it in to make it consistent with defaults.ini

alexweav

LGTM - everything below is a nitpick or a question

alexweav · 2024-05-10T15:52:19Z

conf/defaults.ini

+ha_redis_tls_enabled = false
+
+# Path to the PEM-encoded TLS client certificate file used to authenticate with the redis server.
+ha_redis_tls_cert_path =


Are these required, or can you leave them blank and the client will use the certificates installed in the OS?

Whichever the answer is, might be worth noting in the doc comment

Yup, they're required if TLS is enabled. Will update the docs to make this clear.

alexweav · 2024-05-10T15:59:37Z

pkg/services/ngalert/notifier/redis_peer_test.go

+			CertPath:   certPaths.clientCert,
+			KeyPath:    certPaths.clientKey,
+			CAPath:     certPaths.ca,
+			ServerName: "localhost",


Just curious, will miniredis retry on randomized ports if the default redis port is already taken on the host? Just wondering about test portability, because without port retry this could fail on a dev machine with an unrelated redis running.

I don't expect us to add dynamic ports as part of this PR, we could just implement it in the future if someone complains - just curious if it already works or not.

I don't believe there is port retry, from what I can see it will try a random port on localhost and error if it is already in use.

alexweav · 2024-05-10T15:59:42Z

pkg/services/ngalert/notifier/redis_peer_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func TestNewRedisPeerWithTLS(t *testing.T) {


this is a very cool test 👍

brendamuir · 2024-05-13T08:36:04Z

docs/sources/alerting/set-up/configure-high-availability/_index.md

@@ -66,6 +66,7 @@ database for HA and cannot support the meshing of all Grafana servers.
 1. Set `ha_redis_address` to the Redis server address Grafana should connect to.
 1. [Optional] Set the username and password if authentication is enabled on the redis server using `ha_redis_username` and `ha_redis_password`.
 1. [Optional] Set `ha_redis_prefix` to something unique if you plan to share the redis server with multiple Grafana instances.
+1. [Optional] Set `ha_redis_tls_enabled` to `true` and configure the corresponding `ha_redis_tls_*` fields to secure communications between Grafana and redis with TLS.


Suggested change

1. [Optional] Set `ha_redis_tls_enabled` to `true` and configure the corresponding `ha_redis_tls_*` fields to secure communications between Grafana and redis with TLS.

1. [Optional] Set `ha_redis_tls_enabled` to `true` and configure the corresponding `ha_redis_tls_*` fields to secure communications between Grafana and Redis with Transport Layer Security (TLS).

brendamuir · 2024-05-13T08:36:14Z

docs/sources/alerting/set-up/configure-high-availability/_index.md

@@ -66,6 +66,7 @@ database for HA and cannot support the meshing of all Grafana servers.
 1. Set `ha_redis_address` to the Redis server address Grafana should connect to.
 1. [Optional] Set the username and password if authentication is enabled on the redis server using `ha_redis_username` and `ha_redis_password`.
 1. [Optional] Set `ha_redis_prefix` to something unique if you plan to share the redis server with multiple Grafana instances.


Suggested change

1. [Optional] Set `ha_redis_prefix` to something unique if you plan to share the redis server with multiple Grafana instances.

1. [Optional] Set `ha_redis_prefix` to something unique if you plan to share the Redis server with multiple Grafana instances.

brendamuir · 2024-05-13T08:36:25Z

docs/sources/alerting/set-up/configure-high-availability/_index.md

@@ -66,6 +66,7 @@ database for HA and cannot support the meshing of all Grafana servers.
 1. Set `ha_redis_address` to the Redis server address Grafana should connect to.
 1. [Optional] Set the username and password if authentication is enabled on the redis server using `ha_redis_username` and `ha_redis_password`.


Suggested change

1. [Optional] Set the username and password if authentication is enabled on the redis server using `ha_redis_username` and `ha_redis_password`.

1. [Optional] Set the username and password if authentication is enabled on the Redis server using `ha_redis_username` and `ha_redis_password`.

stevesg

Nice work, thanks for doing this. Just one comment about configuration error handling and this looks good to go.

stevesg · 2024-05-13T10:24:03Z

pkg/services/ngalert/notifier/redis_peer.go

+	if cfg.tlsEnabled {
+		tlsClientConfig, err := cfg.tls.GetTLSConfig()
+		if err != nil {
+			logger.Error("Failed to get TLS config", "err", err)


I would return the error here, a quick skim at the errors from GetTLSConfig shows they are mostly configuration issues and are likely unrecoverable. I believe the argument for not failing on a failed Ping lower down the code, is because the failure is transient and could resolve after startup.

Suggested change

logger.Error("Failed to get TLS config", "err", err)

logger.Error("Failed to get TLS config", "err", err)

return nil, err

stevesg · 2024-05-13T10:30:50Z

pkg/setting/setting_unified_alerting.go

+	HARedisTLSEnabled              bool
+	HARedisTLSCertPath             string
+	HARedisTLSKeyPath              string
+	HARedisTLSCAPath               string
+	HARedisTLSServerName           string
+	HARedisTLSInsecureSkipVerify   bool
+	HARedisTLSCipherSuites         string
+	HARedisTLSMinVersion           string


[nit] We could use dstls.ClientConfig here, would save an instance of re-listing the options (here)

stevesg

Thanks for making the changes, looks ready.

JohnnyQQQQ

Looks good 🚀

ephemeral-instances-bot · 2024-05-14T13:22:37Z

Error building instance: Contact #proj-ephemeral-hg-instances if it is not a compile error. Logs

Error message

handling pull request closed event: deleting instance by slug: unexpected response status: status=502 responseBody=

<title>502 Server Error</title>

Error: Server Error

The server encountered a temporary error and could not complete your request.
Please try again in 30 seconds.

grafana-delivery-bot bot added this to the 11.1.x milestone May 9, 2024

grafana-pr-automation bot added area/backend type/docs labels May 9, 2024

fayzal-g commented May 10, 2024

View reviewed changes

fayzal-g changed the title ~~Add Alerting HA Redis Client TLS config~~ Alerting: Add options to configure TLS for HA using Redis May 10, 2024

fayzal-g added 4 commits May 10, 2024 15:57

Add Alerting HA Redis Client TLS configs

d449f1f

Add test to ping miniredis with mTLS

0cb0fa6

Update .ini files and docs

e7e7981

Add tests for unified alerting ha redis TLS settings

18de77c

fayzal-g force-pushed the alerting-redis-tls branch from 4a04b2f to 18de77c Compare May 10, 2024 14:59

fayzal-g added 2 commits May 10, 2024 16:02

Fix malformed go.sum

00e2c61

Add modowner

8e0ea03

fayzal-g marked this pull request as ready for review May 10, 2024 15:20

fayzal-g requested review from torkelo, a team and brendamuir as code owners May 10, 2024 15:20

fayzal-g requested review from rwwiv, JacobsonMT, yuri-tceretian, grobinson-grafana, JohnnyQQQQ, alexweav and stevesg and removed request for a team May 10, 2024 15:20

fayzal-g added the add to changelog label May 10, 2024

Fix lint error

70621da

alexweav approved these changes May 10, 2024

View reviewed changes

brendamuir reviewed May 13, 2024

View reviewed changes

stevesg reviewed May 13, 2024

View reviewed changes

Update docs and use dstls config

df54543

stevesg approved these changes May 14, 2024

View reviewed changes

JohnnyQQQQ approved these changes May 14, 2024

View reviewed changes

fayzal-g merged commit 7a2fbad into main May 14, 2024
14 checks passed

fayzal-g deleted the alerting-redis-tls branch May 14, 2024 13:21

reagankan mentioned this pull request May 29, 2024

HA Alerting: add TLS configuration to the Redis client for unified alerting high availability #88463

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alerting: Add options to configure TLS for HA using Redis #87567

Alerting: Add options to configure TLS for HA using Redis #87567

fayzal-g commented May 9, 2024 •

edited

fayzal-g May 10, 2024

alexweav left a comment

alexweav May 10, 2024

fayzal-g May 14, 2024

alexweav May 10, 2024

fayzal-g May 14, 2024

alexweav May 10, 2024

brendamuir May 13, 2024

brendamuir May 13, 2024

brendamuir May 13, 2024

stevesg left a comment

stevesg May 13, 2024

stevesg May 13, 2024

stevesg left a comment

JohnnyQQQQ left a comment

ephemeral-instances-bot bot commented May 14, 2024

Error: Server Error

The server encountered a temporary error and could not complete your request.
Please try again in 30 seconds.

		# The maximum number of simultaneous redis connections.
		# ha_redis_max_conns = 5

	1. [Optional] Set `ha_redis_tls_enabled` to `true` and configure the corresponding `ha_redis_tls_*` fields to secure communications between Grafana and redis with TLS.
	1. [Optional] Set `ha_redis_tls_enabled` to `true` and configure the corresponding `ha_redis_tls_*` fields to secure communications between Grafana and Redis with Transport Layer Security (TLS).

	1. [Optional] Set `ha_redis_prefix` to something unique if you plan to share the redis server with multiple Grafana instances.
	1. [Optional] Set `ha_redis_prefix` to something unique if you plan to share the Redis server with multiple Grafana instances.

		@@ -66,6 +66,7 @@ database for HA and cannot support the meshing of all Grafana servers.
		1. Set `ha_redis_address` to the Redis server address Grafana should connect to.
		1. [Optional] Set the username and password if authentication is enabled on the redis server using `ha_redis_username` and `ha_redis_password`.

	logger.Error("Failed to get TLS config", "err", err)
	logger.Error("Failed to get TLS config", "err", err)
	return nil, err

Alerting: Add options to configure TLS for HA using Redis #87567

Alerting: Add options to configure TLS for HA using Redis #87567

Conversation

fayzal-g commented May 9, 2024 • edited

Choose a reason for hiding this comment

alexweav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevesg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevesg left a comment

Choose a reason for hiding this comment

JohnnyQQQQ left a comment

Choose a reason for hiding this comment

ephemeral-instances-bot bot commented May 14, 2024

Error: Server Error

The server encountered a temporary error and could not complete your request.Please try again in 30 seconds.

fayzal-g commented May 9, 2024 •

edited

The server encountered a temporary error and could not complete your request.
Please try again in 30 seconds.