Skip to content
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

Surface target ports in tsh and Connect #49688

Merged
merged 7 commits into from
Dec 9, 2024
Merged

Surface target ports in tsh and Connect #49688

merged 7 commits into from
Dec 9, 2024

Conversation

ravicious
Copy link
Member

This PR makes it so that both tsh apps ls and Connect are capable of showing target ports supported by TCP apps. See the UI section of the RFD.

It also makes sure that any VNet modals or notifications in Connect include the target port where possible.

$ tsh apps ls
Application      Description Type Public Address                      Target Ports    Labels
---------------- ----------- ---- ----------------------------------- --------------- --------
dumper                       HTTP dumper.teleport-local.dev
example1         Example app HTTP example1.teleport-local.dev                         env=test
simplehttpserver             HTTP simplehttpserver.teleport-local.dev
tcp-postgres                 TCP  tcp-postgres.teleport-local.dev
waldo                        TCP  waldo.teleport-local.dev            4080-4090, 5095 env=prod
Connect available ports

How to test

To test the display of target ports, you need to compile teleport from this branch and then add any bogus multi-port TCP app – the ports don't have to work.

    - name: multi-port-example
      uri: tcp://localhost
      tcp_ports:
        - port: 5434 # Postgres
        - port: 8765 # HTTP echo
        - port: 63636
          end_port: 64321

To test the notification and the modal, you can set up any kind of HTTP app on 8765, start VNet and then try to curl the app at multi-port-example.<your cluster>:8765. Just make sure the address of the app is not in your /etc/hosts. Turning on per-session MFA and then closing its modal will let you test the notification. The modal can be seen in the story or by first curling the app, waiting for the cert to expire and then attempting to curl it again – it's important that you curl it at least once before the cert expires.

@ravicious ravicious added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v17 labels Dec 3, 2024
@ravicious ravicious requested review from avatus and gzdunek December 3, 2024 11:49
@github-actions github-actions bot added size/md tsh tsh - Teleport's command line tool for logging into nodes running Teleport. ui labels Dec 3, 2024
@avatus
Copy link
Contributor

avatus commented Dec 3, 2024

Everything works great for me. A couple things to consider (not asking for changes)

  1. I know that having something like 20 different ports is mostly likely going to be handled via a port range, but this list could get super large. I doubt it ever will, but if it did, we have a pattern we use to have an input to search through them. Again, I doubt this will ever happen.

  2. I'm able to create an app with duplicate ports. This doesn't seem to affect anything so I don't really think it matters but just thought i'd point it out. It does add noise to the menu but, besides filtering out dupes (which is probably more work than its worth), I don't think there is much to be done.

Screenshot 2024-12-03 at 9 29 03 AM

@ravicious
Copy link
Member Author

ravicious commented Dec 3, 2024

@avatus Thanks. Yeah, I was thinking about adding the search input, I saw your PR with that. The problem is that here the search would need to be smarter than it is now. E.g. if you have a port range 2000-3000, I'd expect that if I type "2345", it's going to show up. This is the same reason why I haven't implemented a port select in the search bar.

So for now all this UI element is supposed to do is just let you see what ports are available in the first place. I doubt (I hope?) that people are going to be used multi-port apps through those ports – I'd expect most of the time the port number is going to match some pre-defined port in a 3rd-party client. But people could end up wanting to treat those ports like pets not cattle, e.g., they might want to give them names. So we'll see how this will end up being used.

Regarding duplicates, that's a conscious decision:

In order to keep the implementation simple, the user is allowed to duplicate port numbers and ranges. The only validation in place is going to be that any given port number and port range start and end must be within 1–65535 and that the start of a port range must be less than the end (…)

Otherwise we'd have to consider stuff like overlapping ranges or port numbers that already included in other ranges (e.g., 1000-2000, 1337).

@avatus
Copy link
Contributor

avatus commented Dec 3, 2024

they might want to give them names

i actually really liked the comments left in your 'example' app spec next to each port describing their purpose. To be honest, seeing that finally made multi-port "click" for me. I could see naming being very valuable in the future.

Copy link
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.

I tested it locally, didn't find any issues.

@ravicious ravicious changed the base branch from r7s/ports-in-tsh-base to master December 9, 2024 15:13
@ravicious ravicious enabled auto-merge December 9, 2024 15:14
@ravicious ravicious added this pull request to the merge queue Dec 9, 2024
Merged via the queue into master with commit f497513 Dec 9, 2024
44 checks passed
@ravicious ravicious deleted the r7s/ports-in-tsh branch December 9, 2024 16:25
@public-teleport-github-review-bot

@ravicious See the table below for backport results.

Branch Result
branch/v17 Failed

ravicious added a commit that referenced this pull request Dec 10, 2024
* tsh: Print target ports when returning multi-port apps

* Add TCP ports to App message in lib/teleterm

* Connect: Add port selector for multi-port TCP apps

* Add controls to ActionButtons story

* Add control for rendering lots of menu items in ActionButtons story

* Include target port in VNet modals and notifications in Connect

* Empty commit to trigger CI
ravicious added a commit that referenced this pull request Dec 17, 2024
* tsh: Print target ports when returning multi-port apps

* Add TCP ports to App message in lib/teleterm

* Connect: Add port selector for multi-port TCP apps

* Add controls to ActionButtons story

* Add control for rendering lots of menu items in ActionButtons story

* Include target port in VNet modals and notifications in Connect

* Empty commit to trigger CI
github-merge-queue bot pushed a commit that referenced this pull request Dec 17, 2024
* Surface target ports in tsh and Connect (#49688)

* tsh: Print target ports when returning multi-port apps

* Add TCP ports to App message in lib/teleterm

* Connect: Add port selector for multi-port TCP apps

* Add controls to ActionButtons story

* Add control for rendering lots of menu items in ActionButtons story

* Include target port in VNet modals and notifications in Connect

* Empty commit to trigger CI

* VNet: Refuse conn if local port doesn't match app spec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/md tsh tsh - Teleport's command line tool for logging into nodes running Teleport. ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants