-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Display full list of DNS zones in VNet panel #43195
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ravicious
force-pushed
the
r7s/vnet-connect-zones
branch
from
June 18, 2024 16:12
d0dd781
to
ca1920b
Compare
ravicious
force-pushed
the
r7s/vnet-connect-zones
branch
from
June 18, 2024 16:17
ca1920b
to
3d9ef44
Compare
ravicious
force-pushed
the
r7s/vnet-connect-zones
branch
from
June 18, 2024 16:19
3d9ef44
to
0987dd0
Compare
github-actions
bot
added
size/lg
tsh
tsh - Teleport's command line tool for logging into nodes running Teleport.
ui
labels
Jun 18, 2024
The PR includes both Go and JS changes, but I don't expect both of you to review both parts. I guess @nklaassen will be more interested in lib/teleterm changes and @gzdunek in the UI changes. |
ravicious
commented
Jun 18, 2024
ravicious
force-pushed
the
r7s/vnet-connect-zones
branch
from
June 19, 2024 10:57
0987dd0
to
6ed1f58
Compare
gzdunek
reviewed
Jun 19, 2024
I just copied stuff from the linked implementation of then.
gzdunek
approved these changes
Jun 20, 2024
nklaassen
approved these changes
Jun 20, 2024
// looks like, since the VNet admin process also fetches this data independently of the Electron | ||
// app. | ||
// | ||
// Just like the admin process, it skips root and leaf clusters for which DNS couldn't be fetched |
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.
Suggested change
// Just like the admin process, it skips root and leaf clusters for which DNS couldn't be fetched | |
// Just like the admin process, it skips root and leaf clusters for which the vnet_config couldn't be fetched |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Jun 20, 2024
@ravicious See the table below for backport results.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
backport/branch/v16
size/lg
tsh
tsh - Teleport's command line tool for logging into nodes running Teleport.
ui
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Demo recording
dns-zones.mov
At the moment, Connect only shows proxy hostnames of root clusters in the VNet panel. This PR makes it so that it uses the same logic as the VNet admin process to list DNS zones of clusters the user is currently logged into.
The first half of commits is related to
lib/teleterm
, the rest is related to the UI code.UX choices
The list is fetched each time the user opens the VNet panel.
To establish the DNS zones of a cluster, VNet needs to ping it and then fetch
vnet_config
. The response built from those two sources is cached for 5 minutes. For root clusters, VNet also needs to fetch the list of leaf clusters – this response isn't cached but could be in the future. So if someone repeatedly opens and closes the panel, we only ever fetch the list of leaf clusters.The VNet admin process has its own
ClusterConfigCache
. Every 10 seconds, the admin process updates the DNS configuration based on the state ofTELEPORT_HOME_DIR
, so essentially it asks the cache every 10 seconds for a config. This also means that it is able to configure VNet for new clusters or deconfigure it for stale clusters as the user logs in and out of clusters.For 95% of users, the DNS zones will always remain the same. They only ever use a single cluster. For the other 5% of users, the list should get updated whenever they log in or out of clusters. Hence why we fetch the list each time the panel gets opened, instead of maintaining a second layer of caching that reacts to cluster changes in the Electron app.
The fact that the list won't change for most users is also why on subsequent openings of the panel we optimistically show stale values. On top of that, the status indicator behind updating DNS zones does not kick in until a certain delay. All of this to make sure that those 95% of users have least annoying UI possible – they should observe no changes on subsequent openings of the panel.
Perf choices
In Connect,
ClusterConfigCache
is shared between the VNet service andNetworkStack
. SinceNetworkStack
requests a config on each new connection and Connect is going to request config for each cluster whenever the panel is opened, I figured it'd be best to share the cache.I'm not entirely sure if it's going to cause any issues, e.g. Connect wants to show the DNS zones in the UI, so it asks for a config, which causes
NetworkStack
to process a TCP connection for which it also needs a config. The reason it works is becauseNetworkStack
short-circuits when a connection is made to the cluster proxy hostname itself and does not fetch a config in that case.changelog: VNet panel in Teleport Connect now lists custom DNS zones and DNS zones from leaf clusters