[vnet] refactor AppProvider for Windows implementation #51216
Conversation
nklaassen
added
no-changelog
github-actions
bot
added
size/md
tsh
| // Don't return an unexpected error so we can try to find the app in different clusters or forward the | ||
| // request upstream. | ||
| log.InfoContext(ctx, "Failed to list application servers", "error", err) | ||
| return nil, errNoTCPHandler |
There was a problem hiding this comment.
Since on error this method always logs err and then returns errNoTCPHandler, could we instead move this behavior up, always return plain err from resolveAppInfoForCluster and document why we're not using trace.Wrap?
There was a problem hiding this comment.
thanks for calling it out because on taking another look, after we know there's a matching app, i think this function should actually be returning the errors. updated to do so
lib/vnet/local_app_provider.go
Outdated
| // ClientApplication is the common interface implemented by each VNet client | ||
| // application: Connect and tsh. It provides methods to list user profiles, get | ||
| // cluster clients, issue app certificates, and report metrics and errors. | ||
| type ClientApplication interface { |
There was a problem hiding this comment.
ClientApplication is one of those names that at first seems weird, but makes sense once you understand what's happening. Though I have to say that the ClientApplicationService proto service seems to have a slightly better comment. It explicitly mentions the admin process, so I think it makes it easier to understand why this thing is named "client application".
There was a problem hiding this comment.
updated the comment, hopefully it's more clear
|
Side note, I really like git's https://gravitational.slack.com/archives/C08HF8E9F/p1705320635896619 |
|
friendly ping @hugoShaka or @vapopov, not necessarily looking for you guys to do a deep dive on vnet and understand everything that's changing here but i need another review on the code |
public-teleport-github-review-bot
bot
removed the request for review
from hugoShaka
nklaassen
force-pushed
the
nklaassen/refactor-appprovider
branch
from
a3a4c20 to
53684bd
Compare
|
@nklaassen See the table below for backport results.
|
Backport #51216 to branch/v17
Part of RFD 195.
This PR refactors the way that the VNet client applications (Connect and
tsh vnet) provide teleport clients and other functionality to the VNet library. The goal is to move toward an interface that can be implemented locally OR over gRPC so that the VNet Windows service can interact with the client application from another process.The current implementation (prior to this change) has both clients implementing an interface called
vnet.AppProvider. In this PR, each client now implementsvnet.ClientApplication.vnet.localAppProvidernow wrapsvnet.ClientApplicationto implement the now-unexportedvnet.appProvider. In a following PR, I will add avnet.remoteAppProviderthat also implements this interface but interacts with the client application over gRPC.This is basically a rewrite of lib/vnet/app_resolver.go so the diff may not be very useful there, but the new code on its own should be reviewable.
The parent PR #51215 adds the necessary proto files.