Skip to content

Add stub of lib/teleterm VNet gRPC service#39826

Merged
ravicious merged 5 commits intomasterfrom
r7s/vnet-stubs-1-grpc
Mar 29, 2024
Merged

Add stub of lib/teleterm VNet gRPC service#39826
ravicious merged 5 commits intomasterfrom
r7s/vnet-stubs-1-grpc

Conversation

@ravicious
Copy link
Copy Markdown
Member

This PR adds a new gRPC service to lib/teleterm. The Electron app is going to use this service to control VNet through the tsh daemon.

The RFD is still in review, but I don't expect the UI to change much. The stub and the service definition itself are very simple with just Start and Stop RPCs. I'm going to expand it with more comments as I add more stuff to it.

If you read through the UX section of the RFD, you'll see that from the perspective of Connect we don't need anything else other than Start/Stop. Information about new connections is going to flow through tshd events, but that's yet to be implemented.

What's noteworthy about this PR is that it's the first time we're adding a new service instead of cramming more RPCs into TerminalService. The cost of adding a new gRPC service is minimal (especially now after Grzegorz removed much of the boilerplate from the gRPC clients) compared to the benefits we get from separating responsibilities.

@ravicious ravicious added the no-changelog Indicates that a PR does not require a changelog entry label Mar 26, 2024
@ravicious ravicious requested a review from gzdunek March 26, 2024 11:41
@github-actions github-actions Bot requested review from avatus and ibeckermayer March 26, 2024 11:41
@ravicious ravicious removed the request for review from ibeckermayer March 26, 2024 11:43
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm curious if there is any performance difference of providing a separate transport to each service vs reusing the same one.
But we provide different loggers in the interceptors, so it wouldn't work for us anyway.

Copy link
Copy Markdown
Member Author

@ravicious ravicious Mar 26, 2024

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I can update the interceptor to use tshd as the name but I'll make sure it logs the service name as well.

Cool!

Copy link
Copy Markdown
Member Author

@ravicious ravicious Mar 28, 2024

Choose a reason for hiding this comment

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

The last two commits reuse a single transport and log service names.

I also changed the format of the logs a little bit. I went back to the old send/receive at the start of the log message which I think makes it easier to differentiate requests from responses (since it's at the start of the line). I stripped down as much characters from the message as I could and the service name is separated from the method name with a space, not with a slash. This way the console can put those two on separate lines, rather than usually just doing something like:

[tshd] info: receive
teleport.lib.teleterm.v1.TerminalService/GetCluster { … }

Now it's just:

[tshd] info: receive teleport.lib.teleterm.v1.TerminalService
GetCluster { … }

Let me know what you think.

logger

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Also, I noticed that the pin sensitive property filters out pinnedResources too. 💀 idk, I guess the easiest way to solve this would be to make a list with exceptions.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now it's just:

[tshd] info: receive teleport.lib.teleterm.v1.TerminalService
GetCluster { … }

I don't have a strong opinion, but this looks fine.
One more thing that we could do to improve readability would be to not stringify objects in the dev tools:
image
I was experimenting with this recently.

Also, I noticed that the pin sensitive property filters out pinnedResources too. 💀 idk, I guess the easiest way to solve this would be to make a list with exceptions.

Or simply match full property names, I'm not sure if we need to use .includes() 🤔

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I thought about not stringifying objects some time ago, but doesn't this conflicts with logging to a file or something? I remember I tried it a long time ago but I ran into some problems.

As for full property names, yeah, that's an option as well. I guess I initially wanted us to do substring matching just to be safe.

Copy link
Copy Markdown
Contributor

@gzdunek gzdunek Mar 28, 2024

Choose a reason for hiding this comment

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

I thought about not stringifying objects some time ago, but doesn't this conflicts with logging to a file or something?

No, I use different "logging transports" for the file and for dev tools. I will open a PR for that in some spare time
(and fix the bug with filtering "pinnedResources").

Copy link
Copy Markdown
Contributor

@gzdunek gzdunek left a comment

Choose a reason for hiding this comment

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

Thanks for reusing the same transport.

Would it be okay for you to wait for #39828 to be merged first? It will make my life easier when backporting that PR to branch/v15.
I'm going to do it in the morning.

@ravicious ravicious force-pushed the r7s/vnet-stubs-1-grpc branch from 4f000fc to 0b1d15e Compare March 29, 2024 12:21
@ravicious ravicious enabled auto-merge March 29, 2024 12:21
@ravicious ravicious added this pull request to the merge queue Mar 29, 2024
Merged via the queue into master with commit 468daf9 Mar 29, 2024
@ravicious ravicious deleted the r7s/vnet-stubs-1-grpc branch March 29, 2024 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a PR does not require a changelog entry size/md ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants