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

Fix forward_ports service and return useful nginx errors #265

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

0div
Copy link
Contributor

@0div 0div commented Jan 29, 2025

Description

Port forwaring

Use iptables to forward all localhost <> fc-ip ports instead of legacy port forwarding in envd-v0.0.1

They missing key seems to be to enable /proc/sys/net/ipv4/conf/eth0/route_localnet

Useful proxy error

When 502 happens at the session proxy (sandbox) level , we return a useful error depending on the user agent

Browser

Screenshot 2025-01-29 at 7 07 20 PM
# UA is browser
jonas@Jonass-MacBook-Pro js-sdk % curl -i -A "chrome" https://3000-iqs1m6tzbyvkz33vdvju8-6185a593.goulash.dev/
HTTP/2 502
server: nginx/1.27.0
date: Wed, 05 Feb 2025 00:08:03 GMT
content-type: text/html                <<<<<<<<<<<< 👀
content-length: 618
via: 1.1 google
alt-svc: h3=":443"; ma=2592000,h3-29=":443"; ma=2592000

<html><head><title>Error 502 - Closed Port</title><style>html{background-color:#18181b;display:flex;justify-content:center;align-items:center;height:100vh;margin:0}body{color:#ff8800;max-width:600px;text-align:center}a{color:#e57b00}</style></head><body style="margin:0;font-family:sans-serif"><div style="padding:1rem"><h1>Error 502 - Closed Port</h1><p>The sandbox with id <b>"iqs1m6tzbyvkz33vdvju8"</b> is running but port <b>":3000"  </b> is not open, try looking at the <a href="https://e2b.dev/docs/sdk-reference/cli/v1.0.9/sandbox#e2b-sandbox-logs">sandbox logs</a> for more information.</p></div></body></html>

API

# UA is not a browser
jonas@Jonass-MacBook-Pro js-sdk % curl -i -A "api" https://3000-iqs1m6tzbyvkz33vdvju8-6185a593.goulash.dev/
HTTP/2 502
server: nginx/1.27.0
date: Wed, 05 Feb 2025 00:06:39 GMT
content-type: application/json            <<<<<<<<<<<< 👀
content-length: 111
via: 1.1 google
alt-svc: h3=":443"; ma=2592000,h3-29=":443"; ma=2592000

{"error": "The sandbox is running but port is not open", "sandboxId": "iqs1m6tzbyvkz33vdvju8", "port": ":3000"}

Test

I built a template with this new provision and all js-sdk tests pass.

@0div 0div added the improvement Improvement for current functionality label Jan 29, 2025
@0div 0div self-assigned this Jan 29, 2025
…rror-indicating-that-the-sandbox-port-is-not-open-e2b-1329
@ValentaTomas
Copy link
Member

One consideration here—one of the other reasons why we had the old envd there was because of the compatibility with the old SDK for now.

Does this change works when the old env is still present and tries to also forward the ports?

@0div
Copy link
Contributor Author

0div commented Jan 29, 2025

Does this change works when the old env is still present and tries to also forward the ports?

It works with both running yeah, but :sad face: i was hoping we could stop using old envd for good

@0div 0div changed the title Fix forward_ports service and disable envd-v0.0.1 service Fix forward_ports service and return useful nginx errors Jan 30, 2025
@mlejva mlejva self-requested a review January 30, 2025 04:22
@mlejva
Copy link
Member

mlejva commented Feb 5, 2025

@0div can you please replace the error message with following code?

https://gist.github.com/mlejva/06d01a605eae36c9c330a8a6a5908afb

Just replace

<img src="https://hebbkx1anhila5yf.public.blob.vercel-storage.com/Symbol%20Gradient-Kr5pnWlK3ZhzBcRGf6Am4cNbJvY1Ge.svg" alt="Logo" class="logo">

with the actual SVG code please

The result should be this:

Screenshot 2025-02-04 at 8 46 18 PM

Feel free to play with the actual wording

@mlejva
Copy link
Member

mlejva commented Feb 5, 2025

Actually use this one please. I updated it to support dark theme as well.

Screenshot 2025-02-04 at 8 53 36 PM

@0div
Copy link
Contributor Author

0div commented Feb 5, 2025

Actually use this one please. I updated it to support dark theme as well.

Screenshot 2025-02-04 at 8 53 36 PM

I tried inline but it's too big, I'm gonna have to load it as a file in that case, gonna require some changes in nginx config and how we provision the nomad job.

@mlejva
Copy link
Member

mlejva commented Feb 5, 2025

Actually use this one please. I updated it to support dark theme as well.
Screenshot 2025-02-04 at 8 53 36 PM

I tried inline but it's too big, I'm gonna have to load it as a file in that case, gonna require some changes in nginx config and how we provision the nomad job.

Just to be sure we try all the options - have you tried minifying the code?

@0div
Copy link
Contributor Author

0div commented Feb 5, 2025

Actually use this one please. I updated it to support dark theme as well.
Screenshot 2025-02-04 at 8 53 36 PM

I tried inline but it's too big, I'm gonna have to load it as a file in that case, gonna require some changes in nginx config and how we provision the nomad job.

Just to be sure we try all the options - have you tried minifying the code?

Yep, doesn't seem to work, it can't find the closing quote even though i made sure there were no other quotes.

@mlejva
Copy link
Member

mlejva commented Feb 5, 2025

@0div if this is content length problem, I think this could be fixed by changing the default value the client_max_body_size property in the nginx config file https://nginx.org/en/docs/http/ngx_http_core_module.html#client_max_body_size

@jakubno
Copy link
Member

jakubno commented Feb 7, 2025

Can nginx do something like template file? To have the base template as html as then just replace variables?

@0div
Copy link
Contributor Author

0div commented Feb 9, 2025

Can nginx do something like template file? To have the base template as html as then just replace variables?

@jakubno I didn't find any obvious out-of-the-box way of doing it purely via nginx.

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

Successfully merging this pull request may close these issues.

4 participants