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

Show useful 502 message for not running/existent sandbox #231

Conversation

0div
Copy link
Contributor

@0div 0div commented Jan 7, 2025

Description

[re-using previous work from @dobrac in #224]

To return a meaningful error during the DNS resolution in API from client-proxy we route the traffic to a "dummy" server that serves the exception if the sandbox cannot be found.
Per feedback, also split the DNS package between api/ and orchestrator/ , with only the latter having the special "sandbox not found" routing.

Test

Description by Callstackai

This PR introduces a new DNS handling feature that provides a meaningful 502 error message when a sandbox is not running or does not exist. It also refactors the DNS package structure and updates the configuration for the client proxy.

Diagrams of code changes
sequenceDiagram
    participant Client
    participant DNS Server
    participant Records Map
    
    Note over DNS Server: Initialized with empty records map
    
    alt Add Sandbox Record
        Client->>DNS Server: Add(sandboxID, ip)
        DNS Server->>Records Map: Insert(hostname, ip)
    end

    alt DNS Request
        Client->>DNS Server: DNS Query
        DNS Server->>Records Map: Get(sandboxID)
        alt Record Found
            Records Map-->>DNS Server: Return IP
            DNS Server-->>Client: Response with Sandbox IP
        else Record Not Found
            Records Map-->>DNS Server: Not Found
            DNS Server-->>Client: Response with 127.0.0.1
        end
    end

    alt Remove Sandbox Record
        Client->>DNS Server: Remove(sandboxID, ip)
        DNS Server->>Records Map: Remove Entry
    end
Loading
Files Changed
FileSummary
packages/api/internal/dns/server.goAdded a new DNS server implementation that handles sandbox requests and returns appropriate IP addresses or a default routing IP.
packages/api/internal/orchestrator/orchestrator.goUpdated import path for the DNS package to reflect the new structure.
packages/nomad/proxies/client.confModified the Nginx configuration to return a 404 error with a message for invalid sandbox URLs and added a mock server for handling non-existent sandboxes.
packages/orchestrator/cmd/mock-sandbox/mock.goUpdated import path for the DNS package to reflect the new structure.
packages/orchestrator/cmd/mock-snapshot/mock.goUpdated import path for the DNS package to reflect the new structure.
packages/orchestrator/internal/sandbox/sandbox.goUpdated import path for the DNS package to reflect the new structure.
packages/orchestrator/internal/server/main.goUpdated import path for the DNS package to reflect the new structure.

This PR includes files in programming languages that we currently do not support. We have not reviewed files with the extensions .conf. See list of supported languages.

@ValentaTomas ValentaTomas added the improvement Improvement for current functionality label Jan 7, 2025
@0div 0div marked this pull request as ready for review January 15, 2025 17:07
…rror-indicating-that-the-sandbox-is-not-running-when-e2b-1327
Copy link
Member

@jakubno jakubno left a comment

Choose a reason for hiding this comment

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

the DNS is now shared for API and orchestrator. I think it will behave the same, because there's nothing running on 3003 port, but it would be better and cleaner to split it

packages/nomad/proxies/client.conf Outdated Show resolved Hide resolved
@0div
Copy link
Contributor Author

0div commented Jan 16, 2025

@jakubno split the DNS package between api/ and orchestrator/ , with only the latter having the special "sandbox not found" routing

@0div 0div requested a review from jakubno January 16, 2025 19:31
Copy link
Contributor

@callstackai callstackai bot left a comment

Choose a reason for hiding this comment

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

Key Issues

Setting a TTL of 0 can lead to performance issues due to excessive DNS queries; consider using a positive TTL value for caching. The code lacks validation for empty DNS questions, which could result in undefined behavior. Assumptions about the format of q.Name may cause errors if the expected '-' character is missing; implement validation or safer parsing.

Copy link
Contributor

@callstackai callstackai bot left a comment

Choose a reason for hiding this comment

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

Key Issues

The code fails to handle cases where a sandbox is not found, potentially leading to empty DNS responses and unexpected client behavior, unlike the original code which returned a default IP.

@mlejva mlejva removed the request for review from ValentaTomas January 16, 2025 20:19
@0div 0div merged commit 8d1235f into main Jan 16, 2025
1 check passed
@0div 0div deleted the return-error-indicating-that-the-sandbox-is-not-running-when-e2b-1327 branch January 16, 2025 20:20
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