Conversation
Added Access Graph proto file definitions. This GRPC service defines the schema Teleport will use to talk to Access Graph service.
|
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
jakule
added
the
no-changelog
| // ID is the unique ID of the node. | ||
| string id = 1; | ||
| // Kind is the kind of the node, ex: "user", "user_group", "resource", etc. | ||
| string kind = 2; |
There was a problem hiding this comment.
Do we generally make these comments match the casing of the field? like id instead of ID?
There was a problem hiding this comment.
I follow the official Proto3 guideline https://protobuf.dev/programming-guides/proto3/
For the new code (that is outside the api/proto/legacy buf linter forces us to follow this style.
There was a problem hiding this comment.
FYI, we do require some comments, but technically you don't need to use Go-style in here (unless I'm missing something). Meaning you don't need to start with the field name first.
Edit: correct link.
|
Isn't this that one proto service I reviewed mistakenly a while back? Are you looking for a proto review now? |
| } | ||
|
|
||
| // GetFileRequest is a request to get a file. | ||
| message GetFileRequest { |
There was a problem hiding this comment.
What file requests are used for?
There was a problem hiding this comment.
Good question. TAG (the new service that will be using this proto) comes with its UI. This endpoint will be used to serve JS/CSS files from TAG through Teleport Proxy to the user.
There was a problem hiding this comment.
This could use a lot more documentation, its purpose and what gets returned isn't clear at all.
I'd be also concerned that this RPC could become a dumping group for a large number of features.
Why not static serving, or representing routes in an HTTP server, or an enumeration of the possible files to fetch?
There was a problem hiding this comment.
I think that ideally we would remove this endpoint. Currently the flow is client -> Teleport Proxy -> TAG. The files that are returned here live in TAG in a similar way that webassets live in proxy. I only created is as we already have GRPC connection and proxying HTTP request would require opening additional port or multiplexing the connection.
@codingllama That's the same proto (plus some minor modifications). This time, we're merging stuff into master from our dev branch. I think I addressed all the comments that you had, but I would appreciate another look to make sure that this version is ok. |
codingllama
left a comment
codingllama
left a comment
There was a problem hiding this comment.
Reviewed! Apologies for the delay.
| // AccessGraphService is a service for interacting the access graph service. | ||
| service AccessGraphService { | ||
| // Query queries the access graph. | ||
| // Currently only used by WebUI. |
There was a problem hiding this comment.
I'm not sure this kind of comment belongs here, as ideally the RPCs are meant to be widely useful and fulfill a complete set of system operations.
Rename the service proto file to better describe its content.
codingllama
left a comment
codingllama
left a comment
There was a problem hiding this comment.
2nd pass done. There are a number of open comments that I think could be addressed, plus a few new ones. It looks to me that the design could use more polish, there's enough that still feels vague or ad-hoc.
Let me know if you think this is useful or if you'd rather go ahead - in case it's latter, feel free to merge. We can get to the comments at some other point.
| string from = 1; | ||
| // to is the ID of the node the edge is to. | ||
| string to = 2; | ||
| // type is the type of the edge, e.g. "member_of", "belongs_to", etc. |
There was a problem hiding this comment.
I still think an enum would serve better here, but your call.
codingllama
left a comment
codingllama
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments.
4 participants
Added Access Graph proto file definitions. This GRPC service defines the schema Teleport will use to talk to Access Graph service.