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

[FEAT] fully functioning terminal #2586

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

LEstradioto
Copy link
Contributor

@LEstradioto LEstradioto commented Jun 18, 2024

closes #2298

  • Enables Web Terminal to every Command Center
  • A new container (terminal-coolify) is added to Coolify Networks to run a node Websocket

How it works:

  • The new container is a nodeJs server (though we could improve this later if we want to use PHP only, here is my comments on this)
  • A reverse-proxy enables port 6002 without firewall need (just like soketi is working)
  • XTerm.js is the front-end Web terminal enhanced with copy/paste, flowcontrol, resizing, mobile ok
  • A note about security and about destinations/servers
  • The connection happens as usually was done via ssh. Only difference is the RequestTTY=yes option passed by as args. To enable direct connection between socket and terminal using PTY

  • websocket server and node-pty integration with coolify servers and containers
  • reversed-proxy websocket port 6002, no need to firewall anymore
  • enables copy/paste on terminal and text selection fixed with css zoom attribute tweak
  • FEAT: Implement flow control as recommended by xterm documentation (is this really important?). Consider using socket.io or sockette for better performance and reliability.
  • ensure better resizing to xterm (window resize and mobile)
  • rethink about getAllActiveContainersUUID()
  • Double check security
  • Test destinations
  • Add it to any other Command center = ExecuteContainerCommand
  • Refactor and add it to Prod

@LEstradioto LEstradioto mentioned this pull request Jun 18, 2024
@ayntk-ai
Copy link

Looks great. I know that this is maybe not possible but adding an extra container to each application deployed is not ideal, could this container just be deployed once on the coolify level? And also each stack/ app and services have their own network for isolation so I am not sure if 'netowork -coolify' will work

@Thijmen
Copy link
Contributor

Thijmen commented Jun 18, 2024

@ayntk-ai It already looks like it is only one container per Coolify instance. Correct me if I am wrong, @LEstradioto

@LEstradioto
Copy link
Contributor Author

LEstradioto commented Jun 18, 2024

Yep, it resides with coolify network, so it should be only one per instance. About isolation, I used the same logic implemented before with Command Center sending the commands to containers, but, the difference is the long running attached process with support of Websocket.

Without Websocket, Ive tried to use PHP Eventloops, but I couldnt resolve.

Another way would be to re-use Laravel Pusher/Echo, and soketi, but they dont provide SDK to re-use the same process (that would be perfect because we already have a 'coolify realtime container running')

Anyway still need a separate process

@Thijmen
Copy link
Contributor

Thijmen commented Jun 18, 2024

Do you think you can add traefik routing as well, so we don't have to open ports on production?
I'm more than happy to take a swing at adding it.

@ayntk-ai
Copy link

@ayntk-ai It already looks like it is only one container per Coolify instance. Correct me if I am wrong, @LEstradioto

Really amazing. What about the network settings: In coolify there is a different destionation/network for each service, will this just work with the network -coolify setting and then the Terminal will show up on each container and on the full server?

@LEstradioto
Copy link
Contributor Author

@Thijmen I tried to route it to avoid open a new port. I can't get it working.
It should be easy to at least forward eg Path Rule '/ws' to terminal container, but Im not understanding how

@ayntk-ai I couldnt test on different sever, but probably would work since main host is like a root/sudo... right?

@ayntk-ai
Copy link

@LEstradioto I am not sure, but I think the problem might be that the terminal container needs to be redeployed for each new service/application we spin up, as each of these comes with its own network: network-uuid, this is automatic, and we can also manually add networks to services by selecting predefined networks. Maybe I am wrong and the - coolify network adds all networks automatically created by coolify (which would be nice)

@LEstradioto
Copy link
Contributor Author

ps.: my styling is awful 😭

Added some milestone to improve at title

@LEstradioto
Copy link
Contributor Author

@Thijmen I think I did it ! Dont need to open firewall port anymore, reverse proxy seems to work. Tested on localhost only though.

@LEstradioto
Copy link
Contributor Author

LEstradioto commented Jun 20, 2024

now help wanted about three things:

  • here, on dispatch, the ssh command is sent from livewire to JS then sent to websocket, I couldnt hijack the websocket directly on livewire to protect the ssh string, is this a problem?
  • is it needed to check for auth before connect to a container? because ssh command do this on its own, doesnt?
  • test about what @ayntk-ai said, if on extras destinations, terminal will be re-deployed

@ayntk-ai
Copy link

3 things that would be nice for this:

  • A full log of all the commands we have run in the terminal (with automatic log rotation and deletion)
  • A snippet store -> so we can store most used commands as snippets and easily execute them with one click (not have to retype them every time)
  • Run commands on multiple containers at the same time -> could be achieved with the tags system --> But maybe not that smart for security?

@ayntk-ai
Copy link

now help wanted about three things:

  • here, on dispatch, the ssh command is sent from livewire to JS then sent to websocket, I couldnt hijack the websocket directly on livewire to protect the ssh string, is this a problem?
  • is it needed to check for auth before connect to a container? because ssh command do this on its own, doesnt?
  • test about what @ayntk-ai said, if on extras destinations, terminal will be re-deployed

I think @LEstradioto could use your help with these above @Thijmen

@LEstradioto
Copy link
Contributor Author

now help wanted about three things:

  • here, on dispatch, the ssh command is sent from livewire to JS then sent to websocket, I couldnt hijack the websocket directly on livewire to protect the ssh string, is this a problem?
  • is it needed to check for auth before connect to a container? because ssh command do this on its own, doesnt?
  • test about what @ayntk-ai said, if on extras destinations, terminal will be re-deployed

So, answering myself here:

Accessing the container's SSH string requires knowing the container's name/uuid, which is unlikely without prior access. Users on this page can likely modify containers in various ways. Authentication before connection isn't necessary since the route already handles it.

@LEstradioto
Copy link
Contributor Author

3 things that would be nice for this:

  • A full log of all the commands we have run in the terminal (with automatic log rotation and deletion)
  • A snippet store -> so we can store most used commands as snippets and easily execute them with one click (not have to retype them every time)
  • Run commands on multiple containers at the same time -> could be achieved with the tags system --> But maybe not that smart for security?

Initially, I thought this ideas was useful. However, it seems more suited for a development environment, which is not typical for this kind of terminal, since it would require a better dev setup. This might be better addressed in another pull request or discussions to understand its use cases. I couldn't find a good use for it myself, so I'm unsure.

@Thijmen
Copy link
Contributor

Thijmen commented Jun 24, 2024

I think @ayntk-ai would make a great product owner, I am absolutely loving the ideas! 🙌

@LEstradioto
Copy link
Contributor Author

I think it's done!

I would appreciate a review on this, especially the last commit on Docker Compose. I'm not entirely sure if this meets your expectations in all aspects @andrasbacsai 🙏

Regarding destinations, I added servers using the Coolify Dashboard, and everything ran smoothly. I haven't tested with Swarm, but it should work as well. I also didn't test with a new network as @ayntk-ai mentioned earlier. In my tests, Coolify manages everything under one network and isolates by teams only. Am I wrong? I'm probably just tired now. Going to sleep, bye! 🤣

@ayntk-ai
Copy link

@LEstradioto I will try it out once it is released. I think you should merge it into next and remove WIP.

@LEstradioto LEstradioto marked this pull request as ready for review June 25, 2024 15:29
@LEstradioto LEstradioto marked this pull request as draft June 25, 2024 15:50
@LEstradioto LEstradioto force-pushed the feat--terminal-pty branch 3 times, most recently from 3065677 to 5e4a3bb Compare June 25, 2024 16:13
@LEstradioto LEstradioto marked this pull request as ready for review June 25, 2024 18:31
@LEstradioto LEstradioto changed the title [WIP] Feat: terminal pty [FEAT] Feat: terminal pty Jun 25, 2024
@LEstradioto LEstradioto changed the title [FEAT] Feat: terminal pty [FEAT] fully functioning terminal Jun 25, 2024
@LEstradioto
Copy link
Contributor Author

Updated OP with a brief "How it works" section to make it easier for @andrasbacsai to review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants