-
Notifications
You must be signed in to change notification settings - Fork 2.1k
[Connect] Add Server Features to GetCluster #17497
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
Changes from all commits
a2c7789
0b1b6a0
65c76f1
681d7c9
94337d9
3b82f5d
6366801
fb06ef7
477d058
f7581dd
6671265
42e9b7b
2b8be28
5d08317
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -33,6 +33,10 @@ message Cluster { | |||||||||||||||||||||||||||
| bool leaf = 5; | ||||||||||||||||||||||||||||
| // User is the cluster access control list of the logged-in user | ||||||||||||||||||||||||||||
| LoggedInUser logged_in_user = 7; | ||||||||||||||||||||||||||||
| // features describes the auth servers features | ||||||||||||||||||||||||||||
| // Only present in situations where detailed | ||||||||||||||||||||||||||||
| // information is queried from the auth server. | ||||||||||||||||||||||||||||
| Features features = 8; | ||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just like with the struct in the It might be better to introduce a new message, extending
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same remarks as above, I think this is alright. A few counter-examples in our codebase:
|
||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // LoggedInUser describes a logged-in user | ||||||||||||||||||||||||||||
|
|
@@ -90,3 +94,9 @@ message ResourceAccess { | |||||||||||||||||||||||||||
| // delete determines "delete" access | ||||||||||||||||||||||||||||
| bool delete = 5; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Features describes the auth servers features | ||||||||||||||||||||||||||||
| message Features { | ||||||||||||||||||||||||||||
| // AdvancedAccessWorkflows enables search-based access requests | ||||||||||||||||||||||||||||
| bool advanced_access_workflows = 1; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,8 +30,10 @@ option go_package = "github.com/gravitational/teleport/lib/teleterm/v1"; | |
| // TerminalService describes Teleterm service | ||
| service TerminalService { | ||
| // ListRootClusters lists root clusters | ||
| // Does not include detailed cluster information that would require a network request. | ||
| rpc ListRootClusters(ListClustersRequest) returns (ListClustersResponse); | ||
| // ListLeafClusters lists leaf clusters | ||
| // Does not include detailed cluster information that would require a network request. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add a comment to |
||
| rpc ListLeafClusters(ListLeafClustersRequest) returns (ListClustersResponse); | ||
| // GetAllDatabases lists all databases without pagination | ||
| rpc GetAllDatabases(GetAllDatabasesRequest) returns (GetAllDatabasesResponse); | ||
|
|
@@ -89,7 +91,8 @@ service TerminalService { | |
|
|
||
| // GetAuthSettings returns cluster auth settigns | ||
| rpc GetAuthSettings(GetAuthSettingsRequest) returns (AuthSettings); | ||
| // GetCluster returns a cluster | ||
| // GetCluster returns cluster. Makes a network request and includes detailed | ||
| // information about enterprise features availabed on the connected auth server | ||
| rpc GetCluster(GetClusterRequest) returns (Cluster); | ||
| // Login logs in a user to a cluster | ||
| rpc Login(LoginRequest) returns (EmptyResponse); | ||
|
|
||
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 not to include the entire ping response? It seems like knowing things like the version, whether or not it's a FIPS server, and if there are any license warnings might also be useful.
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 reason besides wanting to keep the API concise and tidy to what is needed for this PR. Not opposed to adding in the extra, unused responses to prevent extra PRs in the future.
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 actually recommended Michael to only include what he absolutely needs; it's easy to add to APIs, but hard to remove from them.