-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add Redis docs #10693
Add Redis docs #10693
Conversation
hostname, run: | ||
|
||
```code | ||
$ tctl auth sign --format=redis --host=redis.example.com --out=server --ttl=2190h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason we default to 3 months? Am I correct in thinking that after 3 months the Redis integration would no longer work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benarent It requires you to generate a new cert, but we're using the same value in all our DB integrations, example from Mongo https://goteleport.com/docs/ver/9.0/database-access/guides/mysql-self-hosted/
If we want to change it I think we should create a separate issue and change the ttl for all integrations.
@r0mant @smallinsky Any comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why 2190h
was picked but agree that If we want to change that we should do this be a separate PR where values for all DBs will be updated.
This guide will help you to: | ||
|
||
- Install and configure Teleport. | ||
- Configure mutual TLS authentication between Teleport and your Redis database. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only have some brief firsthand experience with Redis, but I usually see it described as an in-memory data store rather than a database. Does "database" refer to a specific configuration of Redis, or can this guide also apply to Redis when configured as a cache (i.e. with maxmemory
and eviction policies set), or even other possibilities like Redis streams?
In other words, would it make more sense to just say "Redis" or "your Redis deployment," rather than "your Redis database"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that "Redis" makes sense in most places. I was thinking about using "Redis instance" as this is confusing when referring to Redis in cluster.
ACL GENPASS | ||
``` | ||
|
||
or use OpenSSL if you have it installed on your machine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When do we recommend using OpenSSL vs. ACL GENPASS
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this I think is the tricky one. Redis documentation recommends using ACL GENPASS
to generate password. The problem with this is that you need a Redis running to run that command, so it's not the best advice when you're reading a guide on how to setup a Redis cluster and you're at the beginning, but I wanted to mention it somehow that Redis has such option (I don't mind removing it if you think it's confusing). The best idea that I have is to use OpenSSL to generate password, as I'd assume that now everyone knows how to create a "good password".
@r0mant @smallinsky Any objections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, I think that mentioning ACL GENPASS
if fine.
export CERTS_DIR=/path/to/certs/ | ||
|
||
redis-cli --user alice --cluster-replicas 1 --tls --cluster-yes \ | ||
--cluster create ${IP1}:7001 ${IP2}:7002 ${IP3}:7003 ${IP4}:7004 ${IP5}:7005 ${IP6}:7006 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't see any IP1=
bash calls so at first glance the reference to unset $IP1
var is a bit misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but I've got no better idea rather than add a note with something like "export your Redis IPs as environment variables IP1...IP6". Do you think that would help or for example using some "fake" IPs in this case make more sense?
e298b8d
to
9e2c7c9
Compare
@@ -454,6 +454,10 @@ | |||
"title": "SQL Server (Preview)", | |||
"slug": "/database-access/guides/sql-server-ad/" | |||
}, | |||
{ | |||
"title": "Self-Hosted Redis", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Maybe move this above SQL Server section? To keep it grouped with other self-hosted guides and also move the preview SQL Server to the bottom.
Update Redis docs
Co-authored-by: Paul Gottschling <[email protected]>
Co-authored-by: Marek Smoliński <[email protected]>
f2732bf
to
80117ee
Compare
|
||
Click `Add Database Manually`. Use `127.0.0.1` as the `Host` and port printed by `tsh proxy db`. | ||
|
||
<Admonition type="note"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this Admonition be regular text that comes before the "Click..." instruction above? Otherwise, readers will need to follow these instructions out of order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll move it.
|
||
![Redis Insight Startup Screen](../../../img/database-access/guides/redis/redisinsight-startup.png) | ||
|
||
Click `Add Database Manually`. Use `127.0.0.1` as the `Host` and port printed by `tsh proxy db`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the second sentence means. Does tsh proxy db
return a port to include in a Redis Insight field? If so, we should change this to:
Run the following command to start a proxy that you can use to connect
to your Redis instance:
\`\`\`code (remove the escapes)
$ tsh proxy db
\`\`\`
Copy the port returned from this command and use it for the `Port`
field. Use `127.0.0.1` for the `Host` field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not that simple. First you need to login to the database. Then call the tsh db proxy
. This is explained at the top of the page https://goteleport.com/docs/database-access/guides/gui-clients/#get-connection-information. I don't think it's needed to repeat it here. I can add a link to the top of that page to make it more clear.
|
||
![Redis Insight Configuration](../../../img/database-access/guides/redis/redisinsight-add-config.png) | ||
|
||
Next, check the `Use TLS` and `Verify TLS Certificates` boxes and copy the CA certificate returned by `tsh proxy db`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should include the instructions to copy the certificate when we first tell the reader to run tsh proxy db
. Otherwise, they may have cleared the terminal at this point. (Unless the proxy runs in the foreground? I haven't run this command myself.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, tsh proxy
runs in the foreground. Here is an example:
% tsh proxy db redis
Started DB proxy on 127.0.0.1:51823
Use following credentials to connect to the redis proxy:
ca_file=/Users/jnyckowski/.tsh/keys/example.com/cas/example.com.pem
cert_file=/Users/jnyckowski/.tsh/keys/example.com/bob-db/example.com/redis-x509.pem
key_file=/Users/jnyckowski/.tsh/keys/example.com/bob
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's helpful, thanks. To make this clearer, I think we should replace line 210 with something like the following (removing the code fence escapes):
Run the following command to start the Database Access proxy for your
Redis instance.
\`\`\`code
$ tsh proxy db redis
Started DB proxy on 127.0.0.1:51823
Use following credentials to connect to the redis proxy:
ca_file=path/to/example.com.pem
cert_file=path/to/example.com/redis-x509.pem
key_file=path/to/example.com/bob
\`\`\`
In Redis Insight, click `Add Database Manually`. Use `127.0.0.1` as the
`Host`. Copy the port printed by `tsh proxy db` and use it for the
`Port` field.
in the Redis documentation for more details. | ||
|
||
<Admonition type="note" title="Create Redis Cluster"> | ||
When you're configuring Redis Cluster you need to create the cluster using `redis-cli`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you're configuring Redis Cluster you need to create the cluster using `redis-cli`. | |
When you're configuring Redis Cluster to use the TLS credentials we generated earlier, you need to create the cluster using `redis-cli`. |
It took me a second to realize why this Admonition was being included here. I suggested linking it to the preceding text more strongly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this sentence is correct. This step is needed as the final cluster configuration step and it's not related to the TLS keys that we generated earlier. Check my comment below about splitting the docs. I think this should help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good to me!
</TabItem> | ||
</Tabs> | ||
|
||
To connect to a particular database instance, first retrieve its certificate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From here until the "Supported Redis commands" header, nearly every line appears to be another box, which makes this part of the guide slightly hard to read. I think we can remove the <Admonition></Admonition>
tags from the tip and the two notes while preserving the text as regular body text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally think that the current version is hard to follow and it's easy to miss a step as there are many "Only Redis Cluster" blocks. What do you think about splitting this guide into two: Redis and Redis Cluster as a separate pages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good to me! What do you think, @r0mant @smallinsky?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no objections. We can have "Redis Standalone" and "Redis Cluster" pages.
Co-authored-by: Paul Gottschling <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakule Let's merge this and backport so it doesn't block the release and then split the guide in Standalone/Cluster pages as a follow up.
Add Redis documentation on how to setup Redis. Co-authored-by: Paul Gottschling <[email protected]> Co-authored-by: Marek Smoliński <[email protected]>
Add Redis documentation. Backport #10693 to branch/v9 Co-authored-by: Paul Gottschling <[email protected]> Co-authored-by: Marek Smoliński <[email protected]>
Documentation for Redis integration, covers:
Related to #10053