Teleport Connect: Accept database name when setting up proxy #12173
Conversation
| // target_subresource_name points at a subresource of the remote resource, for example a | ||
| // database name on a database server. | ||
| string target_subresource_name = 9; |
There was a problem hiding this comment.
Picking the name wasn't easy. target_name is already reserved for as the name for the database server you're connecting to. On top of that, the fields in the gateway code in lib/teleterm try to not have db-specific names, so I couldn't name it just target_database_name.
I think we wanted to avoid adding db-specific stuff there in case we want to use proxies for more than databases, but we'll see how it'll actually pan out in the future.
There was a problem hiding this comment.
I don't have either a better name proposal that target_subresource_name if we want to drop the reference to db specific fields but I think that in the future where the gateway will be responsible for handling many different gateway types like proxy aws it would be more readable to aggregate those field in oneOf or create a separate message holding those fields instead of handling them by generic naming but for now I'm ok with current version.
Also can we keep proto filed number in ascending order ?
There was a problem hiding this comment.
… it would be more readable to aggregate those field in oneOf or create a separate message holding those fields instead of handling them by generic naming
I was thinking of the second option too. It's almost like designing a database table, in a way.
The current implementation is a shot in the dark to be honest, it should become more clear what kind of stuff we need to share and what not once we actually get to implementing some other kinds of proxies.
Also can we keep proto filed number in ascending order ?
message Gateway already has fields 1–8 and I wanted to add 9th. Instead of putting it as the last field though, I put it next to a field that's more closely related to it. From your experience, do you generally append them to the end?
There was a problem hiding this comment.
Yeah, +1 for putting this at the end and keeping the fields ordered consecutively.
ravicious
force-pushed
the
ravicious/db-name
branch
from
ad5763f to
3933e03
Compare
| // target_subresource_name points at a subresource of the remote resource, for example a | ||
| // database name on a database server. | ||
| string target_subresource_name = 9; |
There was a problem hiding this comment.
I don't have either a better name proposal that target_subresource_name if we want to drop the reference to db specific fields but I think that in the future where the gateway will be responsible for handling many different gateway types like proxy aws it would be more readable to aggregate those field in oneOf or create a separate message holding those fields instead of handling them by generic naming but for now I'm ok with current version.
Also can we keep proto filed number in ascending order ?
* Add target_subresource_name to proto files * Pass database name when creating certs and CLI command
* Add target_subresource_name to proto files * Pass database name when creating certs and CLI command
Teleport Connect used to not ask the user for the database name. This made the whole flow awkward in situations where the user tried to run the generated CLI command, but the DB client returned an error because the user didn't have access to the db under the default name.
This change lets Teleport Connect pass the optional database name to the tsh daemon.
If you want to test this with Teleport Connect, you must build it off of the gravitational/webapps#757.
Teleport.Connect.db.name.mov