Skip to content
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.

Sign up for GitHub

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

Teleport Connect: Accept database name when setting up proxy #12173

Merged
merged 6 commits into from
Apr 26, 2022
Merged

Teleport Connect: Accept database name when setting up proxy #12173

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d4915d2
Add target_subresource_name to proto files
ravicious Apr 22, 2022
3933e03
Pass database name when creating certs and CLI command
ravicious Apr 22, 2022
c716dbe
Keep db flags together
ravicious Apr 25, 2022
a27be5c
Add missing db name to tlsca.RouteToDatabase
ravicious Apr 25, 2022
f64ab87
Keep consecutive order of fields in gateway proto
ravicious Apr 26, 2022
1349ed6
Merge branch 'master' into ravicious/db-name
ravicious Apr 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/teleterm/api/proto/v1/gateway.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ message Gateway {
string target_uri = 3;
// target_user is the target user
string target_user = 4;
// 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;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

… 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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, +1 for putting this at the end and keeping the fields ordered consecutively.

// local_address is the gateway address on localhost
string local_address = 5;
// local_port is the gateway address on localhost
Expand Down
1 change: 1 addition & 0 deletions lib/teleterm/api/proto/v1/service.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ message CreateGatewayRequest {
string target_uri = 1;
string target_user = 2;
string local_port = 3;
string target_subresource_name = 4;
}

message ListGatewaysRequest { repeated string cluster_ids = 1; }
Expand Down
41 changes: 27 additions & 14 deletions lib/teleterm/api/protogen/golang/v1/gateway.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading