Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: show listening ports in port forward popup#4389

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
deansheather merged 7 commits intomainfromdean/listening-ports-ui
Oct 11, 2022

Conversation

deansheather
Copy link
Member

Adds a list of listening ports to the port forward popup in the UI. Uses the endpoint added in#4260

There is intentionally no loading spinner while loading the list of ports as I think it's not necessary (and on unsupported platforms this endpoint will always return an empty list, so you'd be showing a loading spinner that never does anything).

The process name is just the name of the binary. Unfortunately for node.js or python apps this will most likely benode orpython3, but there's not much we can do about that without massively complicating the code.

This is only supported onlinux_* andwindows_amd64. Darwin is entirely unsupported at the moment but we can look into adding it in the future.

image

ntimo and bpmct reacted with heart emoji
@deansheatherdeansheather requested a review froma team as acode ownerOctober 6, 2022 13:44
@deansheatherdeansheather requested review fromcode-asher andBrunoQuaresma and removed request fora team andcode-asherOctober 6, 2022 13:44
Copy link
Collaborator

@BrunoQuaresmaBrunoQuaresma left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@bpmct
Copy link
Member

bpmct commentedOct 6, 2022
edited
Loading

not merge blocking: If a workspace had a PostgreSQL server always running in the background, would it always display here despite there being no web interface? Wondering if there might be a reason a template might want to hide ports/processes in the dropdown OR a check if something is a web app.

@bpmct
Copy link
Member

Also, I assume this omits ports from thecoder_apps?

@deansheather
Copy link
MemberAuthor

@bpmct

If a workspace had a PostgreSQL server always running in the background, would it always display here despite there being no web interface?

Yes, all TCP ports>= 9 will show here. Some stuff might not be a HTTP server but we don't do any HEAD requests or anything as we don't want to send data to ports without the user opting into it first.

Wondering if there might be a reason a template might want to hide ports/processes in the dropdown OR a check if something is a web app.

We should do this in a follow up.

Also, I assume this omits ports from thecoder_apps?

No it doesn't but we probably could.

bpmct reacted with thumbs up emoji

Copy link
Member

@bpmctbpmct left a comment
edited
Loading

Choose a reason for hiding this comment

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

Huge fan of mentioning the service here. However, I find it quite odd that my workspace's code-server shows up here asnode:

Screen.Recording.2022-10-06.at.5.43.11.PM.mov

I don't think we should expose this to users until we deduplicate ports fromcoder_app. If we want to keep this PR frontend-only, perhaps we can conditionally show the component based onCODER_EXPERIMENTAL and fix via a later PR?

The other things I can think of aren't blocking but worth creating issues for:

  • non-web-based ports (e.g. postgres) probably shouldn't show up here. if we don't want to add health checks, perhaps we add an option to disable ports via the template?
  • We have some v1 users using self-signed https schemas which are rewritten. How would this be handled?

@deansheather
Copy link
MemberAuthor

I find it quite odd that my workspace's code-server shows up here as node

This is mentioned in the PR description. It's because code-server runs inside of node and right now we only log the name of the process executable. I think if we filter out apps then this doesn't become that big of a problem.

I don't think we should expose this to users until we deduplicate ports from coder_app.

I will add some code server-side to filter out ports that already have apps. It should be very easy.

non-web-based ports (e.g. postgres) probably shouldn't show up here. if we don't want to add health checks, perhaps we add an option to disable ports via the template?

Sounds good to me for a follow-up. We could also have a list of ports that we filter out hardcoded, including common database ports and other common TCP protocols like SSH, FTP that have distinct port numbers. I'll do that in this PR to mitigate the issue for now.

We have some v1 users using self-signed https schemas which are rewritten. How would this be handled?

Users can make an app instead. We only support HTTP via explicit port app URLs.

bpmct reacted with thumbs up emoji

@deansheather
Copy link
MemberAuthor

deansheather commentedOct 10, 2022
edited
Loading

@bpmct I have added backend code to filter out any ports in use by apps from the response, as well as common non-HTTP ports (like SSH, FTP, postgres etc.) as well as tests that this filtering works. You can see the list of filtered ports inagentconn.go

bpmct reacted with thumbs up emojibpmct reacted with hooray emoji

@github-actions
Copy link

github-actionsbot commentedOct 11, 2022
edited
Loading

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@deansheather
Copy link
MemberAuthor

I have read the CLA Document and I hereby sign the CLA

Copy link
Contributor

@f0sself0ssel left a comment

Choose a reason for hiding this comment

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

backend looks good

@deansheatherdeansheather merged commitb1a095e intomainOct 11, 2022
@deansheatherdeansheather deleted the dean/listening-ports-ui branchOctober 11, 2022 15:10
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsOct 11, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@BrunoQuaresmaBrunoQuaresmaBrunoQuaresma approved these changes

@f0sself0sself0ssel approved these changes

@bpmctbpmctbpmct approved these changes

@coadlercoadlerAwaiting requested review from coadler

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@deansheather@bpmct@BrunoQuaresma@f0ssel

[8]ページ先頭

©2009-2025 Movatter.jp